When using the sandbox option, scripts always parse/run in strict mode, even when they don't specify the use strict; directive.

[benjamincburns]

Description

This description was edited from its original text to reflect the latest understanding of the issue.

I'm running some code in a sandboxed NodeVM. The source-map-support module, which is a transitive dependency of my sandboxed code, is written with the expectation that the this keyword will resolve to global by default (non-strict mode processing). Neither my script, nor this module include the use strict; directive.

Repro code

sandboxed.js:

const vm2 = require("vm2");
const path = require("path");

const vm = new vm2.NodeVM({
  require: {
    external: true,
    context: "sandbox",
    root: __dirname
  }
});

vm.require(path.join(__dirname, "./payload.js"));

payload.js:

global.globalVar = "hello world";

function foo() {
  console.log(this.globalVar);
}

foo();

Repro Explanation

I'd expect that running node sandboxed.js would produce the same result as running node payload.js. Instead, sandboxed.js fails with TypeError [Error]: Cannot read property 'globalVar' of undefined. This failure occurs at the console.log line inside of the foo function.

[benjamincburns]

This issue is stopping me from using ts-node with vm2 while running in sandbox mode. The error I encounter is in the source-map-support module. I'd switch back to host mode, but that doesn't seem to preserve the global context for transitively required modules.

[benjamincburns]

By any chance does sandbox somehow force strict mode? If so, that might explain this behavior.

[XmiliaH]

Yes, every module required in the sandbox is evaluated in strict mode. Therefore, only the this on the top level of the module is bound to the global and this of functions in the module behave the normal strict mode way.

[benjamincburns]

Is strict more required for the sandbox to function correctly? If so, would you care to share why that is (apologies if I missed it in the README or another issue comment)? If not, is there some other reason to be forcing strict mode instead of only enabling it when the use strict; directive is present?

[XmiliaH]

This was the behavior in NodeVM for a long time. I can't tell you why this is. It can be circumvented with the following class

class NonStrictNodeVM extends NodeVM {
	run(script, options) {
		if (typeof(options) === 'object' && options !== null) {
			options = Object.assign({}, options);
			options.strict = false;
		}
		return super.run(script, options);
	}
}

[benjamincburns]

Thanks for that. That does appear to be a viable workaround for this issue. I think however that, assuming that strict mode isn't necessary for security, it'd align better with user expectation if vm2 didn't force strict mode by default. As such I'll leave this issue open as a sort of petition for that change.

Now I have a new issue, but I'll raise that separately.

[benjamincburns]

@XmiliaH are you able to say whether a PR would be accepted that 1) makes the default value of options.strict false in sandbox mode, and 2) makes it possible to force strict mode via options passed via NodeVM?

I realize this would be a breaking change, however I think this would likely align better with most users' expected behavior w.r.t. strict mode/parsing.

[XmiliaH]

I already have a patch which adds options.require.strict which specifies how required modules are loaded (see #448).

[benjamincburns]

Oh that's great, thanks. I'll comment any further thoughts on that PR.

In the meantime unless you prefer to close this, I'll leave it open until #448 is merged.