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.

Sign up for GitHub

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

Jump to bottom

Jest-internal sandbox escape hatch #9907

Merged
merged 8 commits into from
May 25, 2020
Merged

Jest-internal sandbox escape hatch #9907

merged 8 commits into from
May 25, 2020

Conversation

jeysal
Copy link
Contributor

@jeysal jeysal commented Apr 27, 2020 •
edited
Loading

Summary

Closes #9898 (implements the runtime part) - to be easily usable, we will also need a Babel plugin to generate the require(resolve()) structure for our Jest-internal code.

Test plan

Tests added - including a test verifying that this is ignored for external modules to make sure users cannot use this to escape the sandbox.

The first commits adds some general runtime require.resolve test cases unrelated to the change.

@jeysal jeysal requested review from cpojer and SimenB April 27, 2020 22:41
@jeysal jeysal changed the title Runtime require resolve outside Jest-internal sandbox escape hatch Apr 27, 2020
SimenB
SimenB reviewed Apr 27, 2020
View reviewed changes
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

great stuff 😀

packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
packages/jest-runtime/src/__tests__/runtime_require_resolve.test.ts Outdated Show resolved Hide resolved
SimenB
SimenB approved these changes Apr 28, 2020
View reviewed changes
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

I like this - turned out quite minimal. Needs a changelog entry 😀

Do you wanna land this without using it?

@jeysal
Copy link
Contributor Author

jeysal commented Apr 28, 2020

Yeah I think it's fine to land without usages, we can adapt or revert in any way we like later if something else goes wrong, since it's all internal.
I wanna start trying it out on things like #7792 and perhaps look into general perf optimization for any big deps we may load in the sandbox.
Only if it actually works well we can proceed to further steps like making a Babel plugin to make it easier.

@SimenB
Copy link
Member

SimenB commented Apr 28, 2020

Only if it actually works well we can proceed to further steps like making a Babel plugin to make it easier.

Good call

@SimenB
Copy link
Member

SimenB commented Apr 28, 2020

You could just rebase #7792 on this and play with it if we want some assurance it's a feasible solution

@jeysal
Copy link
Contributor Author

jeysal commented Apr 28, 2020

Will pollute the diff there a bit but can do yeah, it's otherwise finished so..

@SimenB
Copy link
Member

SimenB commented Apr 28, 2020

I've added a bunch of cache clearing on master, so might even happen that PR passes now...

@jeysal jeysal mentioned this pull request Apr 28, 2020
Merged
@jeysal
use Symbol key for resolve option
a2a05bd 
This means other tools (e.g. Yarn PnP) should ignore the unknown option
@jeysal
Copy link
Contributor Author

jeysal commented Apr 28, 2020

#7792 (rebased on this and adding a usage) has proven that this works. With all the added Babel and Prettier deps required from the outside, the OOM on circus is avoided and so is the 15-20% overall test run performance degradation.

@jeysal
Copy link
Contributor Author

jeysal commented Apr 28, 2020

The next steps after this (before #7792) will be to land a PR with the first usage (in jest-snapshot) and the Babel plugin to make it easy

@jeysal
Copy link
Contributor Author

jeysal commented Apr 28, 2020

Needs a changelog entry

Sure you want that entry? No one is supposed to (be able to) use any of this 😅

@jeysal
Copy link
Contributor Author

jeysal commented Apr 28, 2020

Almost forgot my one todo, done now:
Added a test guarding against requiring self-constructed jest-main:// paths. This catches e.g. moving the requireInternalModule logic to requireModule, which would give users access to the escape hatch.

SimenB
SimenB approved these changes Apr 28, 2020
View reviewed changes
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

I really like this 👍

packages/jest-runtime/src/index.ts Show resolved Hide resolved
packages/jest-runtime/src/__tests__/runtime_require_resolve.test.ts Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented Apr 28, 2020

Needs a changelog entry

Sure you want that entry? No one is supposed to (be able to) use any of this 😅

The changelog entry is mostly to have a checklist for release and changes that might impact things. "Added way to break out of sandbox" is definitely part of that, even if we think we've made it impossible for user code 😀 But if we detect 2 months from now some instanceof is broken, it'd be great to find this in the changelog rather than jsut the commit history

@jeysal
Merge remote-tracking branch 'upstream/master' into runtime-require-r…
0b3e556 
…esolve-outside

* upstream/master: (106 commits)
  docs: fix jest-diff example (jestjs#10067)
  Cleanup `displayName` type (jestjs#10049)
  docs: correct confusing filename in `enableAutomock` example (jestjs#10055)
  chore: minor optimize getTransformer (jestjs#10050)
  chore: fix TestUtils.ts to match the types (jestjs#10034)
  Minor test name typo fix (jestjs#10033)
  chore: upgrade to typescript 3.9 (jestjs#10031)
  feat: CLI argument to filter tests by projects (jestjs#8612)
  chore: bump `istanbul-lib-instrument` (jestjs#10009)
  feat: support config files exporting (`async`) `function`s (jestjs#10001)
  fix: add missing haste-map dep to jest-snapshot (jestjs#10008)
  🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉 (jestjs#10000)
  Fix typo in dependency warning (jestjs#10006)
  chore: add missing comma (jestjs#9999)
  fix: Control no diff message color with diff options (jestjs#9997)
  fix(jest-jasmine2): fix Error message (jestjs#9990)
  docs: fix jest-object ids for docusaurs (jestjs#9994)
  docs: fix Configuration, JestPlatform and JestObjectAPI docs for 26 (jestjs#9985)
  Add migration notes for breaking changes (jestjs#9978)
  chore: fix date and heading in blog post (jestjs#9977)
  ...
@jeysal
changelog
6c4b188
@jeysal
Copy link
Contributor Author

jeysal commented May 25, 2020

Merged master and added changelog entry. I think this is good to go now. When I find time I will follow up with a Babel plugin and then start introducing usages of this, for the uglier inline snapshots PR and maybe for perf optimization PRs

@jeysal jeysal merged commit eff3eaa into jestjs:master May 25, 2020
@jeysal jeysal deleted the runtime-require-resolve-outside branch May 25, 2020 22:41
@github-actions
Copy link

This pull request 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.
Reviewers

@cpojer cpojer cpojer left review comments

@SimenB SimenB SimenB approved these changes

Assignees
No one assigned
Labels
cla signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RFC] Jest-internal sandbox escape hatch
4 participants
@jeysal @SimenB @cpojer @facebook-github-bot