feat: allow per-module choice for vm context

[blakebyrnes]

This PR exposes an underlying feature to choose the context of each module, instead of pre-determining the vm-wide setting. This feature allows you to load certain modules into the sandbox, while allowing most to remain in the host.

[XmiliaH]

Is there a reason that the function is not passed in at https://github.com/patriksimek/vm2/blob/294ce23a55aa63f67d1385df35888a85eead82a7/lib/resolver-compat.js#L338

[blakebyrnes]

Is there a reason that the function is not passed in at

https://github.com/patriksimek/vm2/blob/294ce23a55aa63f67d1385df35888a85eead82a7/lib/resolver-compat.js#L338

Good catch. Nope, I just missed it.

[blakebyrnes]

@XmiliaH I'm making this change because I have a library running in VM that is trying to track line numbers where the "sandbox" code is calling from. The StackTraces don't cross the boundary of the "vm" module if the initiating code isn't inside the sandbox. I briefly tried to add a feature to track the calling stack trace anytime the bridge was crossed, but it ended up getting stuck in loops. Is the bridge the right way to do that if I try again?

[blakebyrnes]

I tried to add a unit test for this change, but it seems like it's not actually testing anything. Do you have any suggestions how to test that a module is loaded in sandbox vs host?

[blakebyrnes]

@XmiliaH Did something cause you to stop replying?

[XmiliaH]

No particular reason. A method to test if a object is a proxy can be found here: https://github.com/patriksimek/vm2/blob/4f63dc23fecabc79ee1501fde6e9e83c524d6466/test/vm.js#L15-L23

[blakebyrnes]

Won't a module in the sandbox and a module in the host both be VMProxies?

[XmiliaH]

Only host modules are wrapped in a VMProxy. No object in the sandbox sees another object in the same sandbox as a VMProxy.

[blakebyrnes]

const vm = new NodeVM({
    require: {
        external: {
            modules: ['mocha', 'module1'],
        },
        context(module) {
            if (module === 'mocha') return 'host';
            if (module === 'module1') return 'sandbox';
        }
    }
});
function isVMProxy(obj) {
    const key = {};
    const proto = Object.getPrototypeOf(obj);
    if (!proto) return undefined;
    proto.isVMProxy = key;
    const proxy = obj.isVMProxy !== key;
    delete proto.isVMProxy;
    return proxy;
}
console.log({
    module1: !!isVMProxy(vm.run("require('module1')", __filename)),
    mocha: !!isVMProxy(vm.run("require('mocha')", __filename))
});
assert.ok(vm.run("require('module1')", __filename));
assert.ok(vm.run("require('mocha')", __filename));

This shows both as proxies. But I suspect it's because the require has to go through the bridge to the outside. I think I'm just not following.

[XmiliaH]

You should add a console.log here and look at module. It is not the name of the module but the resolved path to the file that will be loaded.

context(module) {
    if (module === 'mocha') return 'host';
    if (module === 'module1') return 'sandbox';
}

[blakebyrnes]

True, but that doesn't actually change the result. If I test the result of doing a vm.run on a require, it will tell me it's a proxy. Do I need to run the "isVmProxy" as code inside the vm.run command?

[XmiliaH]

True, but that doesn't actually change the result.

But it does as the context method returns undefined for both modules and default to host, so both modules will be loaded in the host and get a VMProxy.

[blakebyrnes]

    const vm = new NodeVM({
    require: {
        external: {
            modules: ['mocha', 'module1'],
            transitive: true,
        },
        context(module) {
            console.log(module);
            if (module.includes('mocha')) return 'host';
            if (module.includes('module1')) return 'sandbox';
        }
    }
});
function isVMProxy(obj) {
    const key = {};
    const proto = Object.getPrototypeOf(obj);
    if (!proto) return undefined;
    proto.isVMProxy = key;
    const proxy = obj.isVMProxy !== key;
    delete proto.isVMProxy;
    return proxy;
}
assert.equal(isVMProxy(vm.run("require('mocha')", __filename)), true);
assert.equal(isVMProxy(vm.run("require('module1')", __filename)), false);

I had updated the module paths. What I meant was that it still results in both exported objects being a proxy.

[XmiliaH]

The following code will work:

const vm = new NodeVM({
    require: {
        external: {
            modules: ['mocha', 'module1'],
            transitive: true,
        },
        context(module) {
            if (module.includes('mocha')) return 'host';
            return 'sandbox';
        }
    }
});
function isVMProxy(obj) {
    const key = {};
    const proto = Object.getPrototypeOf(obj);
    if (!proto) return undefined;
    proto.isVMProxy = key;
    const proxy = obj.isVMProxy !== key;
    delete proto.isVMProxy;
    return proxy;
}

assert.equal(isVMProxy(vm.run("module.exports = require('mocha')", __filename)), false);
assert.equal(isVMProxy(vm.run("module.exports = require('module1')", __filename)), true);

There were multiple issues 1) vm.run needs to export the results, it does not return the last expression value as the VM2 vm does. 2) module1 did reexport module2 which was loaded in the host too, it needs to be loaded in the sandbox as module1 just forwards the module2 object. 3) For the mocha module no proxy is expected as it is loaded from the host but the proxy is expected for module1 and module2.

[blakebyrnes]

Thanks for that explanation!

[XmiliaH]

I think it would be better to change the documentation about the context function to state that the first parameter is a filename and make that clear. Furthermore, I was thinking if the host default for the function version should be used as I am of the opinion that this is the wrong default and should be sandbox. I would also like to change the default for the option but that would not be backwards compatible.

[blakebyrnes]

I updated with your latest idea. Anything else that needs to change here?

[XmiliaH]

Currently I do not see a new commit.

[blakebyrnes]

You don't see e085219?

[XmiliaH]

Yes, I do see that commit, but it does not address my latest comments.

[blakebyrnes]

Maybe I misunderstood something? The default is now sandbox, and the documentation references that the module name is the full path to the filename. Was there something else?

[blakebyrnes]

FYI - I just synchronized with your latest.

[XmiliaH]

Yes, I added some review comments you should be able to see. But in any case:

• README still has a require.pathContext option which was merged with require.context.
• pathContextCallback is never called with json and is not really the extension but how it is loaded.
• const pathContext = typeof options.context === 'function' ? ((module, ext) => options.context(module, ext) || 'sandbox') : (() => context);: I think it would be better to use

  const pathContext = typeof context === 'function' ? context : (() => context);

  and modify all the calles to pathContext from ==="sandbox" to !=="host". This would ensure that modules with invalid returns are loaded in the sandbox and should not hold the whole options object alive for to long.

[blakebyrnes]

Oh, odd. I don't see any comments. I tried to address your comments. This better?

[XmiliaH]

Looks good to me. Thank you.