fix(security): RQ-2426 — sandbox rule code (QuickJS-WASM)#112
fix(security): RQ-2426 — sandbox rule code (QuickJS-WASM)#112dinex-dev wants to merge 4 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 6 minutes and 26 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
WalkthroughThe PR replaces the previous host-realm Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
"Code"-type Modify Request/Response rules ran rule-supplied JS via
`new Function(...)` directly in the proxy's Node process (full require/process/
fs/child_process). Code rules travel between users (shared lists, import/export,
team sync), so this was a supply-chain RCE primitive.
Rule code now runs inside QuickJS compiled to WebAssembly (quickjs-emscripten,
single-file embedded variant). QuickJS is a separate JS engine in the WASM
sandbox: no require/process/fs/global, and no prototype path back to the host
(constructor-escape blocked). Only injected primitives are reachable.
Why QuickJS-WASM (not isolated-vm or worker_threads + vm):
- isolated-vm is a native addon with no build for a supported Electron's V8
(6.x too old for V8 13, 7.x needs Node 26).
- worker_threads cannot create a Worker in an Electron renderer ("The V8
platform ... does not support creating Workers"), and the proxy runs in the
desktop app's background renderer.
QuickJS-WASM is pure WASM+JS — builds nowhere natively and runs in the renderer.
- src/utils/index.ts: executeUserFunction runs in QuickJS; 5s deadline interrupt,
128MB cap. isValidFunctionString compiles via `new Function` WITHOUT calling it
(parse-only, no execution). getFunctionFromString removed.
- both Modify Request/Response processors: validate -> pass the source string.
- contract preserved: returns a string (objects JSON-stringified), promises
awaited, console captured as {type,args}, $sharedState read + written back.
- intentional gap: no fetch/Buffer/timers (fetch needs the asyncify variant +
async host bridge — a follow-up; QuickJS can do it safely).
Verified: sandbox harness 13/13 (Node 24); instantiates + runs + blocks
host access inside the Electron 42 renderer; before/after exploit probe flips
from RCE/file/env/process access to fully blocked.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dinex-dev
force-pushed
the
fix/RQ-2426-sandbox-worker-vm
branch
from
77c31fd to
2f32940
Compare
dinex-dev
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Line 3: The version field in package.json has been downgraded from 1.5.0 to
1.4.0, which violates semantic versioning and will cause issues with release
management tools. Update the version value to a forward version number that is
higher than the previously published 1.5.0 (such as 1.5.1 or 1.6.0, depending on
whether this is a patch or minor release) to maintain proper version ordering
for releases.
In `@src/utils/index.ts`:
- Around line 167-168: The call to executePendingJobs() on the vm.runtime object
returns a result containing a QuickJSHandle that must be properly disposed to
prevent VM memory leaks. Capture the return value from executePendingJobs() and
check if it contains an error handle, then ensure the handle is disposed by
calling the appropriate disposal method on the returned result before
proceeding. This will prevent memory leaks when deadline interrupts or job
errors occur.
- Around line 114-120: The issue is a race condition where the snapshot read via
GlobalStateProvider.getInstance().getSharedStateCopy() occurs before an await
statement, allowing concurrent invocations to modify shared state via
setSharedState() while control is yielded, causing stale state to overwrite
concurrent updates when execution resumes. Move the entire try-catch block
containing the getSharedStateCopy() call to execute after the await
getQuickJSModule() statement completes, so that the snapshot is captured after
the async module resolution rather than before it. This closes the interleaving
window and ensures the snapshot reflects current state at the point it will be
used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2509111-22a8-4f66-8744-b17a2a20643b
⛔ Files ignored due to path filters (5)
dist/components/proxy-middleware/rule_action_processor/processors/modify_request_processor.jsis excluded by!**/dist/**dist/components/proxy-middleware/rule_action_processor/processors/modify_response_processor.jsis excluded by!**/dist/**dist/utils/index.d.tsis excluded by!**/dist/**dist/utils/index.jsis excluded by!**/dist/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/components/proxy-middleware/rule_action_processor/processors/modify_request_processor.jssrc/components/proxy-middleware/rule_action_processor/processors/modify_response_processor.jssrc/utils/index.ts
QuickJS is a bare ECMAScript engine — it lacks the Web/Node globals rule code commonly expects. Add them as PURE-JS shims that run inside the sandbox, built only from QuickJS built-ins so no host object crosses the boundary (same safety model as atob/btoa; verified require/process stay undefined in-sandbox): - URL / URLSearchParams (regex-based parser; common cases — protocol/host/port/ path/search/hash/origin, searchParams, basic relative-base resolution) - TextEncoder / TextDecoder (UTF-8) - structuredClone (deep clone, preserves Date/Map/Set, handles cycles) - crypto.randomUUID / crypto.getRandomValues NOTE on crypto: this is a Math.random-based STOPGAP — NOT cryptographically secure (no entropy source in the WASM realm). Fine for ids/non-security random; secure crypto should be a host bridge (follow-up), matching @requestly/sandbox-node's byte-identical-to-host crypto approach. Aligns with the API client's QuickJS sandbox model: pure-JS shims for computation-only APIs, host bridge reserved for capability APIs (crypto/fetch). URL is hand-rolled (sandbox-node doesn't expose URL — uses a regex internally — so this is a superset of theirs). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The QuickJS sandbox commit was assembled from a checkpoint based on an older master and inadvertently reverted package.json/package-lock from 1.5.0 to 1.4.0. Restore to 1.5.0 so the branch's only package.json delta vs master is the added quickjs-emscripten deps. Dependency versions left untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Read the $sharedState snapshot after the last await so the snapshot->eval-> setSharedState read-modify-write is atomic w.r.t. the event loop. Reading it before the await let a concurrent executeUserFunction commit in the gap, whose update this call's stale snapshot would then clobber (last-writer-wins). - Capture executePendingJobs() result and dispose its error handle (carried on a job error / deadline interrupt) instead of discarding it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
What
Sandboxes "Code"-type Modify Request/Response rules. They previously ran rule-supplied JS via
new Function(...)directly in the proxy's Node process (fullrequire/process/fs/child_process). Code rules travel between users (shared lists, import/export, team sync) → supply-chain RCE.Rule code now runs inside QuickJS compiled to WebAssembly (
quickjs-emscripten, single-file embedded variant).Why QuickJS-WASM
QuickJS is a separate JS engine in the WASM sandbox — no
require/process/fs/global, and no prototype path back to the host (constructor-escape blocked). Only explicitly-injected primitives are reachable. The two alternatives don't work here:worker_threadscannot create a Worker in an Electron renderer ("The V8 platform … does not support creating Workers"), and the proxy runs in the desktop app's background renderer.QuickJS-WASM is pure WASM+JS — builds nowhere natively and runs fine in the renderer (verified).
Changes
src/utils/index.ts:executeUserFunctionruns the rule in QuickJS — 5s deadline interrupt, 128 MB cap.isValidFunctionStringcompiles vianew Functionwithout calling it (parse-only, no execution; novm).getFunctionFromStringremoved.consolecaptured as{type,args},$sharedStateread + written back.Intentional gap
No
fetch/Buffer/timers in rule code.fetchwould need the asyncify QuickJS variant + an async host bridge — a follow-up (QuickJS can do it safely, unlike worker+vm).Verified
typeof process/require= undefined in-sandbox).Downstream
Desktop needs only a proxy dep bump (folded into #358) — no desktop code change. Ships alongside the Node-24
writeHeadfix (#113).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores