Module.wrap override breaks shadowing for globals

[zyrolasting]

Create a src.js with the following.

let console = 1;

node src.js runs without error because function-level shadowing of globals is permitted. That program would break with SyntaxError: Identifier 'require' has already been declared if you replace console with require.

lya breaks with the the same error for console due to its Module.wrap override, regardless if it uses var or let as a variable declarator. This is because it injects known global identifiers into the same module function's scope.

Is there a note somewhere that tells users that they cannot shadow any global identifiers used by Lya in this way?

[GNtousakis]

Hey Sage! Thanks for the report!
I don't think we have documented this specific issue in the repo. Although re-declaration with var should be possible.

When i run the following snippet with lya, it fails.

let console = 1;

But when i change let to var the following snippet passes.

var console = 1;

This is possible because of an option in the code base that changes the way we declare the global identifiers in the prologue that we injected to every imported module. This happens in the following sections of the src.js file. By default this option is false.

lya/src/core.js

Lines 25 to 26 in 01a4163

	// This holds the string of the transformations inside modules
	const declaration = (lyaConfig.enableWith === false) ? 'var' : 'let';

lya/src/core.js

Lines 418 to 421 in 01a4163

	const setDeclaration = (name) => {
	prologue += declaration + ' ' + name +
	' = localGlobal["' + name +'"];\n';
	};

[zyrolasting]

Thanks for the quick response! I'm aware of the option, but this error reproduces if you use var and enableWith is true, because then the var is hoisted above generated lets.

But when i change let to var the following snippet passes...

Yes, but I would not expect a user to change their code for the sake of a dynamic analysis tool. A program that uses let does not work regardless of the value of enableWith. So it seems like the only way to shadow a global in a module-level declaration is when you use var and set enableWith to false.

Are you saying the point of this option is to create a workaround for the error? If so, what workaround would exist if the user does not have to change their code?

[GNtousakis]

Thanks for the quick response! I'm aware of the option, but this error reproduces if you use var and enableWith is true, because then the var is hoisted above generated lets.

Yes, that is true and logical. It is not so much a bug as the way that node.js operates. I mean the way that lya is written, it creates a prologue that re-declares global variable in module-level scope. The only possible ways to declare the variables are with var,let and const. In our use-case:

• const doesn't work for us
• let is one option, that has the advantage of not interfering with the with functionality that we have implemented, but allows only one declaration of a variable. The following fail in any node.js program:

let pizza = 'amazing';
let pizza = 'amazing';

• var is another options that allows to mitigate the problem with the double declaration, but interferes with the functionality of with. This will pass:

var pizza = 'amazing';
var pizza = 'amazing';

In our error case, i don't think there is a way to first declare something with var and then again with let and let it pass. Same thing vise-versa.

Yes, but I would not expect a user to change their code for the sake of a dynamic analysis tool. A program that uses let does not work regardless of the value of enableWith. So it seems like the only way to shadow a global in a module-level declaration is when you use var and set enableWith to false.

Yes, I agree. I don't expect the user to change their code for their dynamic analysis tool, but i think it is logical to expect some configurations to be passed to the dynamic analysis tool. There is only one way to mitigate this specific scenario without changing the users code, and that is to use the --prop-exclude functionality while running the dynamic analysis tool to exclude console from the analysis.

Are you saying the point of this option is to create a workaround for the error? If so, what workaround would exist if the user does not have to change their code?

The point of this option is to provide way to disable the with functionality (provides a deeper analysis on global variables scope) and at the same time provide a workaround for the error.

[zyrolasting]

@GNtousakis Your summary of the intended usage of --prop-exclude was related to what I was asking for. I already understood the rest, and my concern was not in how Node.js works. I tried to make that clear in the OP when talking about require.

There is only one way to mitigate this specific scenario...

If core.js was built around runInContext instead of runInThisContext, I suspect this issue can be avoided because the exact globals you want to pass in are properly instrumented. My understanding from @nvasilakis is that there is some reason why that wasn't used, but I haven't heard the specifics yet. I verified that the code runs in the same V8 isolate, so I'd appreciate help understanding why manually extending CommonJS and reserving module-local identifiers is the answer.

Failing that, I suspect the issue can also be resolved by wrapping module code in a second IIFE, but I would need to understand how that impacts what you meant by "provides a deeper analysis on global variables scope".

[zyrolasting]

understanding why manually extending CommonJS and reserving module-local identifiers is the answer.

To add: It appears the reason behind runInThisContext is because a require function is used as an entry point for analysis, and Node.js calls vm.runInThisContext as a side-effect of require. So when I talk about building around runInContext, I'm thinking of an override like this.

function override(context) {
   vm.runInThisContext = function runInThisContext(code, options) {
    return vm.runInContext(code, context, options); 
  }
}

Doing that creates a path where you don't need to tinker with CommonJS to make it work, and avoid the scoping issues.