-
Notifications
You must be signed in to change notification settings - Fork 294
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
feat: allow per-module choice for vm context #521
Conversation
Is there a reason that the function is not passed in at Line 338 in 294ce23
|
Good catch. Nope, I just missed it. |
lib/resolver-compat.js
Outdated
@@ -291,6 +291,8 @@ function resolverFromOptions(vm, options, override, compiler) { | |||
|
|||
const builtins = genBuiltinsFromOptions(vm, builtinOpt, mockOpt, override); | |||
|
|||
const pathContext = options.pathContext || (() => context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to pass the pathContext
function through the context
option so that the context
option then allows to be "host" | "sandbox" | pathContextCallback
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's definitely a cleaner option. Will change.
@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? |
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? |
@XmiliaH Did something cause you to stop replying? |
No particular reason. A method to test if a object is a proxy can be found here: Lines 15 to 23 in 4f63dc2
|
Won't a module in the sandbox and a module in the host both be VMProxies? |
Only host modules are wrapped in a VMProxy. No object in the sandbox sees another object in the same sandbox as a VMProxy. |
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. |
You should add a context(module) {
if (module === 'mocha') return 'host';
if (module === 'module1') return 'sandbox';
} |
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? |
But it does as the |
I had updated the module paths. What I meant was that it still results in both exported objects being a proxy. |
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
|
Thanks for that explanation! |
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. |
I updated with your latest idea. Anything else that needs to change here? |
Currently I do not see a new commit. |
You don't see e085219? |
Yes, I do see that commit, but it does not address my latest comments. |
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? |
FYI - I just synchronized with your latest. |
Yes, I added some review comments you should be able to see.
|
Oh, odd. I don't see any comments. I tried to address your comments. This better? |
Looks good to me. Thank you. |
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.