chore: migrate Node.js implementation for js-sandbox to isolated-vm
#3973
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const command = `node ${CLI_PATH} ${args}`; | ||
|
|
||
| return new Promise((resolve) => | ||
| exec(command, options, (error, stdout, stderr) => |
Check warning
Code scanning / CodeQL
Shell command built from environment values
jamesgeorge007
force-pushed
the
sandbox-vulnerability-remediation
branch
from
4595ad6
to
7dd5848
Compare
jamesgeorge007
changed the title
js-sandbox to isolated-vmNode.js implementation for js-sandbox to isolated-vm
jamesgeorge007
force-pushed
the
sandbox-vulnerability-remediation
branch
7 times, most recently
from
c74603c
to
fe9232c
Compare
jamesgeorge007
commented
Apr 15, 2024
jamesgeorge007
force-pushed
the
sandbox-vulnerability-remediation
branch
from
a1a860f
to
5ef7751
Compare
jamesgeorge007
requested review from
AndrewBastin and
liyasthomas
as code owners
jamesgeorge007
force-pushed
the
sandbox-vulnerability-remediation
branch
from
5ef7751
to
20e5954
Compare
jamesgeorge007
force-pushed
the
release/2024.3.1
branch
from
9095738
to
375d532
Compare
jamesgeorge007
force-pushed
the
sandbox-vulnerability-remediation
branch
2 times, most recently
from
b20e0bb
to
3523656
Compare
- Wrap `pw` methods in `ivm.Reference` for transferring to the isolate context. - Ensure pre-request script methods are executed via `applySync` calls in the isolate context.
Since Windows doesn't respect the shebang line, we'd need to specify node as the command with the CLI binary while running tests
- Remove the `testing` directory and move the tests within it to root. - Rename the `envs` directory to `env` to follow a consistent convention indicating the nested scripting API objects.
- Organize imports. - Update CI config. - Bump dependent actions. - Fix a typo while referring to the Node.js version and run tests on LTS. - Move `Setup node` step above `Setup pnpm`. - Bump CLI version.
Rename `~/src/utils.ts` to `~/src/shared-utils` to differentiate from platform-specific utility functions (`~/src/node/utils.ts`).
Add safeguards to prevent test failure if the endpoint results in a non `200` status code.
jamesgeorge007
force-pushed
the
sandbox-vulnerability-remediation
branch
from
c84c4d6
to
8321fc8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR aims at migrating the sandbox implementation for
Node.jsfrom node:vm to isolated-vm.Ref: GHSA-qmmm-73r2-f8xr
Closes HFE-475.
Changes
isolated-vmas a peer dependency (there were issues with not being able to requireisolated-vmwhen it was made a direct dependency) tojs-sandboxand mark it optional since it's specific to theNode.jsimplementation. Also, it is added as a direct dependency on the CLI.Jest(support for ESM is experimental) toVitestforjs-sandbox.js-sandbox.node,web) instead of the type of scripts (pre-request&test-runner) easily accommodating platform-specific utility functions.~/src/utils.tsat the root compiling utility functions reused across platform implementations is renamed to~/src/shared-utilsto differentiate between platform-specific utils~/src/node/utils.ts.testingdirectory from the test suite and enforce a naming convention for the directories based on the scripting API namespace objects (env,expect, etc).pw.expect(expectVal)is stringified with an additional propertyisStringifiedWithinIsolateif it's a non-primitive while transferring to the isolate context as required byisolated-vm. The value is parsed at thepw.expectmethod definition after checking for the above additional property to differentiate between stringification done while transferring to the isolate context and an original value. The newly introduced property is removed before consumption.Node.jsversion is20and above from the CLI supplying the--no-node-snapshotflag tonodeas required byisolated-vm, Ref and later proceed with the argument parsing phase.folderswithin collections while validating against the versioned entity schema and translating to the latest version. Follow up of feat: add extended support for versioned entities in the CLI #3912.200status code).Setup nodestep aboveSetup pnpm.Checks