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

[RFC] Jest-internal sandbox escape hatch #9898

Closed
jeysal opened this issue Apr 27, 2020 · 4 comments · Fixed by #9907
Closed

[RFC] Jest-internal sandbox escape hatch #9898

jeysal opened this issue Apr 27, 2020 · 4 comments · Fixed by #9907

Comments

@jeysal
Copy link
Contributor

jeysal commented Apr 27, 2020

Motivation

This stems from #7792, where we need to load multiple dependencies outside of the sandbox from jest-snapshot running within the sandbox. Passing things in from the outside is becoming very tedious, and even passing in a requireOutside function doesn't seem easy enough - it should be easy for our own code to make the right call on when to import from outside the sandbox for correctness (not being influenced by mocking/module mutation from user code) and perf (not creating many new module instances).

jest-snapshot will now need @babel/{core,traverse,types,generator} and prettier from outside the sandbox. I can also imagine that in future extensions of inline snapshot logic, and other parts of Jest loaded inside the sandbox, we may want to require outside dependencies, both for performance and for correctness (not requiring mocks or modules modified by the user). @SimenB has also mentioned that it would be nice if e.g. inside expect could get an outside jest-diff to improve performance.
I also want to avoid this requirement introducing lots of boilerplate into our packages because we have to pass either imported dependency objects or requireOutside functions around, so I'd like to have a way for Jest's own packages to require outside modules more easily, while still preventing user code from doing so (i.e. no globalThis.__requireOutside()). It should be easy for us to correctly require external dependencies, but as close to impossible as we can get for the user.

🚀 Feature Proposal

Here's my proposal, slightly wild but I think the downsides can actually be mitigated:
jest-runtime adds a require(moduleName, {outside: true}) to the createRequireImplementation() return value, but only for modules required as internal (fortunately we already know this info) to make sure user code doesn't get it.
We already have code around this, the require implementation for internal modules is already different in that it, firstly, requires further modules as internal as well, and secondly, never considers mocks.
This require interface extension is compatible with the official Node require, so loading Jest packages using outside: true when already outside of a sandbox will still work. This applies both for us, if we load util code sometimes inside and sometimes outside a sandbox, and for external users of jest-diff etc.
Because the outside option will (CAN) only be used by Jest's internal modules that also makes it okay in the unlikely event that Node ever decides to add a second parameter to require - we can refactor our interface and all usages to make it compatible again.

Variants

Node API compatibility

@SimenB voiced concerns regarding Node API compatibility if we extend require.
A possible way to fix this:
We have a Babel plugin (applied to Jest's internal modules and ideally just a macro) transform requireOutside('/module.js') to require(require.resolve('/module.js', {outsideJestVm: true})). The runtime's resolve returns 'jest-outside-vm:///module.js'. This protocol is handled by the runtime's require, prompting it to use the real require to retrieve the module. This should be a 100% compliant require implementation, unless you consider coming up with extra options forbidden.
This does expose a way for users to break outside of the sandbox again, if they know the protocol. To secure against this, we can generate a random crypto number on runtime startup and attach that to the protocol, or even an HMAC over the path using the number, so that in case we leak one such URL it cannot be used to require any file from outside the VM.

@jeysal jeysal changed the title Importing dependencies from outside the sandbox from Jest's internal code within the sandbox [RFC] Jest-internal sandbox escape hatch Apr 27, 2020
@SimenB
Copy link
Member

SimenB commented Apr 27, 2020

it should be easy for our own code to make the right call on when to import from outside the sandbox for correctness (not being influenced by mocking/module mutation from user code) and perf (not creating many new module instances).

The require in internal modules is never affected by mocks (require injected in modules loaded via requireInternalModule is always requireInternalModule, which doesn't consider mocks), so this is purely a perf thing.

Because the outside option will (CAN) only be used by Jest's internal modules that also makes it okay in the unlikely event that Node ever decides to add a second parameter to require - we can refactor our interface and all usages to make it compatible again.

My main concern here is that if we have this running for some time, node 18 might introduce this second param, and then all old versions of Jest would be broken unless we backported some fix (which I don't think we would ever do). So I'm in favour of the second option you suggest.

This does expose a way for users to break outside of the sandbox again, if they know the protocol.

Not really - we can throw if that protocol is passed to the normal require rather than the one backed by requireInternalModule

@jeysal
Copy link
Contributor Author

jeysal commented Apr 27, 2020

The require in internal modules is never affected by mocks (require injected in modules loaded via requireInternalModule is always requireInternalModule, which doesn't consider mocks), so this is purely a perf thing.

Oh, true.
Reminds me of one aspect though: We can only use this inside of requireInternalModule cascades like circus' jestAdapterInit. E.g. if we decide to import the "real" outside pretty-format from jest-diff (not saying we want to, haven't checked if that's dangerous because of global types or so), this doesn't mean that the user importing jest-diff will get that, because their test run is not in an internal context. Don't see that causing problems though?

Not really - we can throw if that protocol is passed to the normal require rather than the one backed by requireInternalModule

Oh, so maybe it's actually quite simple. The thing providing the security here is really the internal runtime context - if only that can give you special require.resolve tokens, only that can also accept special tokens in the require 👍

@SimenB
Copy link
Member

SimenB commented Apr 27, 2020

Don't see that causing problems though?

Nah, I think it's fine (and the behavior we want).

Oh, so maybe it's actually quite simple.

Yeah, should be pretty clean.

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants