Node: Evaluate node core modules within a `vm.Context`

Created on 18 Feb 2020  路  8Comments  路  Source: nodejs/node

Is your feature request related to a problem? Please describe.

One of Jest's longest-standing issue are about globals in tests being different from Node's, see https://github.com/facebook/jest/issues/2549.

Short example illustrating the problem:

const {createContext, compileFunction} = require('vm');

const context = createContext({});

const code = `
  const fs = require('fs');
  const assert = require('assert');

  try {
    fs.readFileSync('/some/faulty/path', 'utf8')
  } catch (error) {
    assert(error instanceof Error)
  }
`;

const compiledFunction = compileFunction(code, ['require'], {
  parsingContext: context,
});

compiledFunction(require);

Run this example, and the instanceof test will fail. Remove parsingContext option, and it will pass.

Describe the solution you'd like

Jest implements its own require etc, which works great most of the time, but in the case of node core modules, it fallbacks to requiring the module outside of the context. AFAIK the JS source of the node core modules are embedded in the binary of Node, so we cannot read them and pass them though compileFunction, SourceTextModule or some such ourselves so the globals would be the same.

It would be great if Node provided some API allowing the JS of core modules to be evaluated within a vm.Context.

(I had originally given up on this, but seeing as #30709 solves the super old #855 I thought maybe we might be lucky enough that this is possible as well? 馃榾)

Describe alternatives you've considered

We have tried to mess around with setting Symbol.hasInstance and just injecting globals from the outside (although that breaks the encapsulation we're aiming for by using separate Contexts in the first place) without luck. It's also a moving target as we'd have to add that property to all globals.


FWIW the Jest issue has a $500 bounty which I'd be happy to give to someone solving this in core, or the JS Foundation if that's more correct.

feature request module

Most helpful comment

@SimenB By cross-context operations I mean for example, passing a sandbox into the createContext() call and access what's in sandbox from the inside, or returning something from an invocation of a compiled function evaluated on the context and use that returned result from the outside. That could introduce subtle issues when these objects are passed into builtins (on either side) as things like brand checks and instanceof checks may fail and most builtins aren't really prepared to handle objects created from a different context gracefully. But from the description in the OP and https://github.com/nodejs/node/issues/31852#issuecomment-588076155 it seems the use case of Jest does not require something like this, I think an option for this kind of use case would be fairly doable (compared to the kind that needs cross-context interop), though it would still need some internal cleanups as some internal hooks in Node.js currently assume they are evaluated in the main context (most ENVIRONMENT_STRONG_PERSISTENT_VALUES, I think) and depend on references to the set of things in the main context.

All 8 comments

I think we could expose this somewhat painlessly, whether we want to or not being a separate question.

Ah, that's promising! What would be the reasons to not add this (beyond a larger api surface)?

I鈥檝e talked about this recently with other people, partially in the context of https://github.com/nodejs/node/issues/28823. This is definitely doable, but it鈥檚 also going to be a lot of work.

Thanks for that link, that's also something that would be great for us! We currently inject them manually, but they pose the same issues (instanceof often failing and the fact people can mutate those globals and it'll leak between tests). Current code: https://github.com/facebook/jest/blob/9fbe3c5bd07002d569a1b4037d53556244a728cd/packages/jest-environment-node/src/index.ts#L38-L64

If cross-context support is not necessary, this can be done with an explicit option to include these globals during context creation, which tells Node.js to run the corresponding setups and create a different set of builtins for the context (from the top of my head, it should also be possible to make the option switch on/off Node.js-specific non browser globals). I believe it would be more difficult, or at least require more serialization/deserialization overhead, if we want the builtins from different contexts to interoperate with each other, however.

AFAIK the JS source of the node core modules are embedded in the binary of Node, so we cannot read them and pass them though compileFunction, SourceTextModule or some such ourselves so the globals would be the same.

The source code is actually available through one of the process.bindings but they cannot be evaluated properly in the user land anyway because they need a set of internals to be passed into the compiled function to finish the setup.

You're referring to the node globals part (#28823) right? I'm not 100% sure what you mean with cross-context. I'm _reasonably_ sure we wouldn't want any cross context stuff, as long as we could still override them from the outside. For example we can install fake timers and interact with them from the outside. But we wouldn't want 2 context's messing with each other, beyond the place creating a context being able to access it. Is that considered cross-context and would make it harder?

@SimenB By cross-context operations I mean for example, passing a sandbox into the createContext() call and access what's in sandbox from the inside, or returning something from an invocation of a compiled function evaluated on the context and use that returned result from the outside. That could introduce subtle issues when these objects are passed into builtins (on either side) as things like brand checks and instanceof checks may fail and most builtins aren't really prepared to handle objects created from a different context gracefully. But from the description in the OP and https://github.com/nodejs/node/issues/31852#issuecomment-588076155 it seems the use case of Jest does not require something like this, I think an option for this kind of use case would be fairly doable (compared to the kind that needs cross-context interop), though it would still need some internal cleanups as some internal hooks in Node.js currently assume they are evaluated in the main context (most ENVIRONMENT_STRONG_PERSISTENT_VALUES, I think) and depend on references to the set of things in the main context.

Ah okay, thanks for elaborating! The use case in the OP should be perfectly fine with that limitation, then.

As for globals in user code, that limitation would present issues for us I think. For example our require implementation is passed from the parent context. The example in the OP is a bit too simplified perhaps, instead compiledFunction(require) what we _really_ do is compiledFunction(ourCustomRequireImplementation), and that function needs to be created from the outside. We'll also want to control (and replace dynamically) setTimeout and friends from the parent context for mocked timers. I'm personally fine with the function we pass in not being instanceof Function (as it's not today anyways), but that might not be a trade-off that core can make or limitation it can accept if adding a new API for this?

However, the code snippet I linked to would not be affected as in that case we just want what's inside the context to look like what's outside - we don't actually need to know what they are or access them.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

Brekmister picture Brekmister  路  3Comments

vsemozhetbyt picture vsemozhetbyt  路  3Comments

danielstaleiny picture danielstaleiny  路  3Comments

mcollina picture mcollina  路  3Comments

seishun picture seishun  路  3Comments