webrecorder / wabac.js

wabac.js - Web Archive Browsing Augmentation Client
https://replayweb.page
GNU Affero General Public License v3.0
95 stars 17 forks source link

Javascript global 'let' variable declaration scope bug. #156

Open ARiedijk opened 6 months ago

ARiedijk commented 6 months ago

A scope issue related to JavaScript variables, particularly with the let keyword, within the context of a function that is modifying the global scope. The parseLetConstGlobals(text) function in is intended to handle global const and let declarations by prefixing them with self., effectively making them properties of the global object (window in a browser). However, this approach leads to unexpected behavior in script.


const globalConst = "const";
let globalLet = "let";
let globalLetChanged = "x";

function loading() {
   console.log("loading" + globalLetChanged);
   globalLetChanged = "changed";
}

window.addEventListener("load", function(){
  loading();
});

This script declares global variables (globalConst, globalLet, globalLetChanged) and a function loading that modifies globalLetChanged. An event listener is attached to the window's load event, which calls loading.

After modification by parseLetConstGlobals function: These variable declarations have a 'self.' addition, this leads to an encapsulation issue.

{
  // ... original script ...
}

self.globalConst = globalConst;
self.globalLet = globalLet;
self.globalLetChanged = globalLetChanged;

Scope Issue: The problem arises because the globalLetChanged variable inside the script and the self.globalLetChanged variable are not the same. When loading modifies globalLetChanged, it only changes the script-local variable, not the global one (self.globalLetChanged). Consequently, any other script that checks the value of globalLetChanged (thinking it's global) finds it unchanged.

ikreymer commented 6 months ago

Yes, this is still a situation where the fix for webrecorder/wombat#82 is not complete. Don't have a good fix at the moment - I assume you have examples where this is causing a problem? Generally, the need for this is to address 'bad' code which accesses undeclared let globals that were declared in a different file - hopefully such code is on the decrease.

Some options could be injecting the self.X = Y assignment after the initial assignment, or converting to var, but those have other consequences. Would be good to see some real world examples where this is an issue if you have some.

ARiedijk commented 6 months ago

I understand the problem and didn't see a good solution myself. I have no real world examples. As soon as I come across these I will forward them.

On Tue, Jan 23, 2024 at 3:44 AM Ilya Kreymer @.***> wrote:

Yes, this is still a situation where the fix for webrecorder/wombat#82 https://github.com/webrecorder/wombat/issues/82 is not complete. Don't have a good fix at the moment - I assume you have examples where this is causing a problem? Generally, the need for this is to address 'bad' code which accesses undeclared let globals that were declared in a different file - hopefully such code is on the decrease.

Some options could be injecting the self.X = Y assignment after the initial assignment, or converting to var, but those have other consequences. Would be good to see some real world examples where this is an issue if you have some.

— Reply to this email directly, view it on GitHub https://github.com/webrecorder/wabac.js/issues/156#issuecomment-1905195303, or unsubscribe https://github.com/notifications/unsubscribe-auth/AVHZHRVKQVSVEDDELAM77ZTYP4P2PAVCNFSM6AAAAABB6QE5GWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBVGE4TKMZQGM . You are receiving this because you authored the thread.Message ID: @.***>

-- Ad Riedijk Team Lead & Senior Software Engineer [image: Pagefreezer] https://www.pagefreezer.com/

office 1 888 916 3999 <1-888-916-3999>

@.***

pagefreezer.com https://www.pagefreezer.com/ [image: facebook] https://www.facebook.com/PageFreezerSoftwareInc [image: twitter] https://twitter.com/PageFreezer [image: linkedin] https://www.linkedin.com/company/pagefreezer-software-inc- [image: youtube] https://www.youtube.com/user/PageFreezer

CONFIDENTIAL: This message is intended only for the use of the individual(s), company, organization or agency to whom or to which it is addressed. The message may contain information that is privileged, confidential and may be exempt from disclosure of any kind under applicable law. If you have received this message and you are not the intended recipient, you are notified that any dissemination, distribution or copying of this communication is strictly prohibited. Furthermore, the use of any personal or confidential information found herein may violate privacy laws and other applicable laws in your jurisdiction and beyond. If you have received this communication in error, please notify Pagefreezer immediately by email at @.***

ikreymer commented 5 months ago

I hoping that this is something that we will see less and less off, especially with the move to using modules and more strict mode / better code that doesn't rely on using implicit globals from different modules (which is why we need the self. in the first place)