Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to load modules in non strict mode #448

Merged
merged 3 commits into from Aug 28, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
Expand Up @@ -144,6 +144,8 @@ Unlike `VM`, `NodeVM` allows you to require modules in the same way that you wou
* `require.context` - `host` (default) to require modules in the host and proxy them into the sandbox. `sandbox` to load, compile, and require modules in the sandbox. Except for `events`, built-in modules are always required in the host and proxied into the sandbox.
* `require.import` - An array of modules to be loaded into NodeVM on start.
* `require.resolve` - An additional lookup function in case a module wasn't found in one of the traditional node lookup paths.
* `require.customRequire` - Use instead of the `require` function to load modules from the host.
* `require.strict` - `false` to disable loading modules in strict mode (default: `true`).
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does the value of require.strict impact the interpretation of code that includes (or doesn't include) a use strict; directive?

I think it might help to clarify that here, as by the current wording I'd assume that use strict; will be ignored when require.strict is set to false.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the following be better?
false to not force strict mode on modules loaded by require (default: true).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and please forgive this one last grumble about the default value being true here... 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default is true to have backwards compatibility.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth the breaking change to have better compatibility with a vanilla node runtime, IMO. But I rest my case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point I would like to bump the minor version and change some default settings to be more secure and reasonable. This would include:

  • Setting allowAsync depending on timeout. If timeout is set disable async by default.
  • Remove coffeescript as compiler.
  • Make require.resolve required to load host modules. (Makes it easier for bundler as there won't be a dynamic require any more).
  • Change require.context to sandbox by default.
  • Setting require.strict depending on strict setting.

* `nesting` - **WARNING**: Allowing this is a security risk as scripts can create a NodeVM which can require any host module. `true` to enable VMs nesting (default: `false`).
* `wrapper` - `commonjs` (default) to wrap script into CommonJS wrapper, `none` to retrieve value returned by the script.
* `argv` - Array to be passed to `process.argv`.
Expand Down
2 changes: 2 additions & 0 deletions index.d.ts
Expand Up @@ -24,6 +24,8 @@ export interface VMRequire {
resolve?: (moduleName: string, parentDirname: string) => string | undefined;
/** Custom require to require host and built-in modules. */
customRequire?: (id: string) => any;
/** Load modules in strict mode. (default: true) */
strict?: boolean;
}

/**
Expand Down
1 change: 1 addition & 0 deletions lib/nodevm.js
Expand Up @@ -184,6 +184,7 @@ class NodeVM extends VM {
* @param {resolveCallback} [options.require.resolve] - An additional lookup function in case a module wasn't
* found in one of the traditional node lookup paths.
* @param {customRequire} [options.require.customRequire=require] - Custom require to require host and built-in modules.
* @param {boolean} [option.require.strict=true] - Load required modules in strict mode.
* @param {boolean} [options.nesting=false] -
* <b>WARNING: Allowing this is a security risk as scripts can create a NodeVM which can require any host module.</b>
* Allow nesting of VMs.
Expand Down
11 changes: 6 additions & 5 deletions lib/resolver-compat.js
Expand Up @@ -46,8 +46,8 @@ function makeExternalMatcher(obj) {

class LegacyResolver extends DefaultResolver {

constructor(builtinModules, checkPath, globalPaths, pathContext, customResolver, hostRequire, compiler, externals, allowTransitive) {
super(builtinModules, checkPath, globalPaths, pathContext, customResolver, hostRequire, compiler);
constructor(builtinModules, checkPath, globalPaths, pathContext, customResolver, hostRequire, compiler, strict, externals, allowTransitive) {
super(builtinModules, checkPath, globalPaths, pathContext, customResolver, hostRequire, compiler, strict);
this.externals = externals;
this.currMod = undefined;
this.trustedMods = new WeakMap();
Expand Down Expand Up @@ -282,7 +282,8 @@ function resolverFromOptions(vm, options, override, compiler) {
root: rootPaths,
resolve: customResolver,
customRequire: hostRequire = defaultRequire,
context = 'host'
context = 'host',
strict = true,
} = options;

const builtins = genBuiltinsFromOptions(vm, builtinOpt, mockOpt, override);
Expand Down Expand Up @@ -325,7 +326,7 @@ function resolverFromOptions(vm, options, override, compiler) {
}

if (typeof externalOpt !== 'object') {
return new DefaultResolver(builtins, checkPath, [], () => context, newCustomResolver, hostRequire, compiler);
return new DefaultResolver(builtins, checkPath, [], () => context, newCustomResolver, hostRequire, compiler, strict);
}

let transitive = false;
Expand All @@ -336,7 +337,7 @@ function resolverFromOptions(vm, options, override, compiler) {
transitive = context === 'sandbox' && externalOpt.transitive;
}
externals = external.map(makeExternalMatcher);
return new LegacyResolver(builtins, checkPath, [], () => context, newCustomResolver, hostRequire, compiler, externals, transitive);
return new LegacyResolver(builtins, checkPath, [], () => context, newCustomResolver, hostRequire, compiler, strict, externals, transitive);
}

exports.resolverFromOptions = resolverFromOptions;
5 changes: 3 additions & 2 deletions lib/resolver.js
Expand Up @@ -140,12 +140,13 @@ class Resolver {

class DefaultResolver extends Resolver {

constructor(builtinModules, checkPath, globalPaths, pathContext, customResolver, hostRequire, compiler) {
constructor(builtinModules, checkPath, globalPaths, pathContext, customResolver, hostRequire, compiler, strict) {
super(builtinModules, globalPaths, hostRequire);
this.checkPath = checkPath;
this.pathContext = pathContext;
this.customResolver = customResolver;
this.compiler = compiler;
this.strict = strict;
this.packageCache = {__proto__: null};
this.scriptCache = {__proto__: null};
}
Expand Down Expand Up @@ -200,7 +201,7 @@ class DefaultResolver extends Resolver {
this.checkAccess(mod, filename);
if (this.pathContext(filename, 'js') === 'sandbox') {
const script = this.readScript(filename);
vm.run(script, {filename, strict: true, module: mod, wrapper: 'none', dirname: mod.path});
vm.run(script, {filename, strict: this.strict, module: mod, wrapper: 'none', dirname: mod.path});
} else {
const m = this.hostRequire(filename);
mod.exports = vm.readonly(m);
Expand Down