fix(security): RQ-2426 — sandbox rule code (QuickJS-WASM)#112
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughThe PR adds QuickJS runtime dependencies, introduces sandbox globals for guest execution, rewrites Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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>
Rule "Code" runs in the QuickJS-WASM sandbox (a bare ES realm), so any web/Node
global a script used in the old full-host environment is otherwise missing. This
adds a compatibility layer so existing scripts keep working, without weakening the
isolation boundary — only JSON-serialisable data crosses the host edge.
Capabilities, by mechanism:
- Pure in-guest JS shims (no host contact): URL, URLSearchParams, TextEncoder/
Decoder, structuredClone, atob/btoa, Buffer, Blob, FormData, Request, Response,
Headers, setTimeout/setInterval/clearTimeout/clearInterval, queueMicrotask,
performance.
- Host bridges (copy-in/copy-out only, no host object exposed):
- crypto: randomUUID, getRandomValues, createHash, createHmac, subtle.digest
- fetch: real HTTP via host fetch, driven by a guest-promise + pump loop.
- XMLHttpRequest (async) layered on the fetch bridge.
- require('crypto') maps to the crypto bridge; any other require(...) throws a
guided error (fs/process/etc. stay absent by design).
fetch policy: http(s) URLs only; credentials: 'omit' (a shared rule cannot ride
the user's ambient cookies/sessions).
Engine: import from quickjs-emscripten-core (not the umbrella quickjs-emscripten),
whose auto-loader statically references every WASM variant and breaks bundlers
(the desktop's webpack). core + the single embedded variant is bundler-safe.
Refactor: the in-guest source strings moved to utils/sandbox-globals.ts
(guest realm) so utils/index.ts is host orchestration only.
Deliberate limitations:
- WebSocket: constructor throws — a persistent connection can't outlive a
per-request execution.
- Synchronous XMLHttpRequest (open(..., false)): throws — QuickJS can't block;
use async / fetch.
- crypto.subtle: only digest implemented; sign/verify/importKey/generateKey
deferred (CryptoKey objects can't cross the copy boundary). HMAC signing is
available via require('crypto').createHmac.
- setTimeout fires once via a microtask (delay not honoured); setInterval is a
no-op (returns an id) so a per-request rule cannot spin forever.
- fetch: FormData/Blob multipart parts cross as text (binary parts best-effort).
No SSRF/private-IP hardening yet (tracked separately).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/utils/sandbox-globals.ts (1)
224-234: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPreserve byte semantics in the crypto compatibility layer.
randomBytesreturns a plain array, so common code likerequire("crypto").randomBytes(16).toString("hex")won’t behave like Node.createHash/createHmacalso stringify byte inputs and keys, which can produce incorrect digests/signatures.Also applies to: 427-428
🤖 Prompt for 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. In `@src/utils/sandbox-globals.ts` around lines 224 - 234, The crypto compatibility layer is treating byte data like plain strings/arrays, so `randomBytes`, `createHash`, and the related `createHmac` path need to preserve Node-like byte semantics. Update the implementations in `sandbox-globals` so `randomBytes` returns a byte-like value that supports `toString("hex")`, and ensure `createHash`/`createHmac` pass raw byte input and keys through without forcing `String(...)` coercion before calling `__hostCrypto`.
🤖 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 `@src/utils/sandbox-globals.ts`:
- Around line 329-335: The timer shim in sandbox-globals is currently masking
real timing behavior by making setTimeout fire immediately and ignoring ms,
which can break retry/backoff logic; update the timer handling around
g.setTimeout/g.clearTimeout/g.setInterval/g.clearInterval to either throw for
unsupported timers or wire them to real delay-aware host scheduling instead of
microtask-based execution.
- Around line 255-264: `hostFetchOp` currently allows only http/https and
timeouts, but still permits SSRF to private, loopback, and metadata IPs. Update
the request flow in `hostFetchOp` (before the host `fetch` call) to resolve
`parsedUrl.hostname`, detect any resolved IPs in private/link-local/loopback
ranges, and reject them with an error. Keep the existing protocol allow-list and
size/timeout handling intact, and make sure the check runs for every URL before
issuing the fetch.
---
Nitpick comments:
In `@src/utils/sandbox-globals.ts`:
- Around line 224-234: The crypto compatibility layer is treating byte data like
plain strings/arrays, so `randomBytes`, `createHash`, and the related
`createHmac` path need to preserve Node-like byte semantics. Update the
implementations in `sandbox-globals` so `randomBytes` returns a byte-like value
that supports `toString("hex")`, and ensure `createHash`/`createHmac` pass raw
byte input and keys through without forcing `String(...)` coercion before
calling `__hostCrypto`.
🪄 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: 5eaadfe4-2c23-4109-9433-6e6d03342ac8
⛔ Files ignored due to path filters (4)
dist/utils/index.jsis excluded by!**/dist/**dist/utils/sandbox-globals.d.tsis excluded by!**/dist/**dist/utils/sandbox-globals.jsis excluded by!**/dist/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.jsonsrc/utils/index.tssrc/utils/sandbox-globals.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/index.ts
Comment on lines
+255
to
+264
| g.fetch = function (url, init) { | ||
| init = init || {}; | ||
| var hdrs = init.headers || {}; | ||
| if (hdrs && typeof hdrs.forEach === "function" && hdrs.__h) { var o = {}; hdrs.forEach(function (v, k) { o[k] = v; }); hdrs = o; } | ||
| var req = JSON.stringify({ | ||
| url: String(url), | ||
| method: init.method || "GET", | ||
| headers: hdrs, | ||
| body: init.body != null ? String(init.body) : undefined | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the host fetch bridge for URL allow/deny policy, timeout, and response-size limits.
rg -n -C8 '\bhostFetchOp\b|__hostFetch|fetch\(' srcRepository: requestly/requestly-proxy
Length of output: 11589
Strengthen hostFetchOp by blocking private network and metadata endpoints.
The hostFetchOp function in src/utils/index.ts (lines 143–188) correctly enforces protocol allow-listing (http/https only) and applies FETCH_TIMEOUT_MS (10s) and MAX_FETCH_BODY_BYTES (25MB) limits. However, it lacks explicit SSRF protections. The function currently relies on the underlying host environment to resolve hostnames, leaving it vulnerable to private network or cloud metadata endpoint access (e.g., 169.254.169.254, 127.0.0.1). Add logic to resolve parsedUrl.hostname and reject any resulting IP addresses falling within private, loopback, or link-local ranges before invoking the host fetch.
🤖 Prompt for 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.
In `@src/utils/sandbox-globals.ts` around lines 255 - 264, `hostFetchOp` currently
allows only http/https and timeouts, but still permits SSRF to private,
loopback, and metadata IPs. Update the request flow in `hostFetchOp` (before the
host `fetch` call) to resolve `parsedUrl.hostname`, detect any resolved IPs in
private/link-local/loopback ranges, and reject them with an error. Keep the
existing protocol allow-list and size/timeout handling intact, and make sure the
check runs for every URL before issuing the fetch.
setTimeout previously fired once on a microtask and ignored `ms`, silently turning retry/backoff code into a hot loop (hammering endpoints, ignoring rate limits) and breaking delay-based ordering — a quiet behavioral surprise for existing rules. Add a __hostTimer bridge: the guest setTimeout now waits the real delay via a host timer, clamped host-side to the execution's remaining wall-clock budget (so a timer can never outlast the run), driven by the existing guest-promise + pump loop. clearTimeout cancels the pending callback. setInterval stays a no-op (a repeating timer can't outlive a per-request execution); queueMicrotask stays a microtask. Chose delay-aware scheduling over throwing on setTimeout: throwing would break the common benign `setTimeout(fn, 0)` yield and library-internal timer use. Verified: 120ms timer elapses ~123ms; delay ordering (fast before slow); backoff loop accumulates real delay; clearTimeout cancels; setTimeout(0) still yields. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Regroup the in-guest source from "when it was added" (POLYFILLS/BRIDGE_SHIMS/ EXTRA_SHIMS) to "what it is", as dependency-ordered, self-describing blocks: ENCODING, BINARY, URL, HTTP_TYPES, CLONE, CRYPTO, NETWORK, TIMERS, HARNESS — composed into a single exported SANDBOX_PRELUDE (index.ts now imports one constant instead of four). - Factor the byte helpers (utf8/hex/base64) into one shared __rqb namespace instead of being trapped in the EXTRA block. - Merge the awkward fetch re-wrap (base fetch + a second wrapper via __origFetch) into a single body-aware fetch. - Round out Headers (set/append/delete). No behavior change — full capability suite (fetch + policy, crypto incl. hmac/subtle.digest, Buffer, URL, encoding, structuredClone, Blob/FormData/ Response, multipart, XHR, real-delay setTimeout, WebSocket guard, isolation, sharedState) passes identically. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nsrCodes
approved these changes
Jun 25, 2026
Previously any sandbox failure was swallowed: a prelude (our shim) error was
disposed silently, user throws/timeouts degraded to a generic "Returned value is
not a string", nothing reached host logs/Sentry, and synchronous user throws
leaked out as untracked top-level eval errors. A regressed polyfill would have
been invisible in production.
Now executeUserFunction categorises and surfaces every failure:
- Eval the prelude (our shims) SEPARATELY from the user wrapper, so a shim error
is unambiguously ours (kind "prelude") and is dumped, not disposed.
- Run the user fn inside a `.then` so SYNC throws become rejections captured by
`.catch` (→ __OUTPUT.error) like async ones, instead of leaking out.
- Throw a typed SandboxError { kind: "prelude" | "user" | "timeout" } carrying the
real message; success still returns the result string. A CPU-deadline interrupt
is classified as "timeout" (not a user logic error).
- reportSandboxError(): always console.error("[rq-sandbox]", kind, msg); prelude/
timeout also go to Sentry (guarded — may be uninitialised). user → console only.
Processors now show the real error and a distinct code: 188 = sandbox-internal
(our bug), 187 = the rule author's code.
Verified: prelude error reported (not swallowed); user sync + async throws surface
the real message; infinite loop → timeout; happy paths for all APIs unaffected.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/index.ts (1)
277-345: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCap in-flight host bridge operations.
A rule can enqueue many
fetch()orsetTimeout()calls before the CPU interrupt fires; those host promises/timers/network requests live outside the QuickJS memory limit. Add a smallMAX_INFLIGHT_HOST_OPSguard and fail further bridge calls once reached.🤖 Prompt for 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. In `@src/utils/index.ts` around lines 277 - 345, The host bridge functions can enqueue too many pending async operations before the CPU interrupt stops execution, so add a small MAX_INFLIGHT_HOST_OPS limit to cap outstanding work. Update the __hostFetch and __hostTimer paths in the bridge setup to check the current inflight array size before pushing new promises, and return a failure/throw a host error once the limit is reached. Keep the guard centralized near the inflight tracking in src/utils/index.ts so both fetch and timer calls are covered consistently.
♻️ Duplicate comments (1)
src/utils/index.ts (1)
250-254: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftProtect
$sharedStateacross async host waits.The snapshot is still taken before an
await: user code can hitfetch/setTimeout, the pump loop yields at Line 416, and another rule can commit shared state before Line 459 overwrites it with this stale snapshot. Use an async mutex/CAS-style update around the full read→execute→write path, or merge against the latest state beforesetSharedState.Also applies to: 416-419, 458-459
🤖 Prompt for 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. In `@src/utils/index.ts` around lines 250 - 254, Protect the shared state update in the flow around getSharedStateCopy, the async host wait, and setSharedState by making the read→execute→write sequence atomic. Use an async mutex or compare-and-swap style guard in the logic that snapshots shared state, yields during the host wait, and later writes back, so a stale sharedStateJson cannot overwrite newer changes from another rule. If full atomicity is not possible, refresh or merge against the latest state immediately before setSharedState in the same execution path.
🤖 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
`@src/components/proxy-middleware/rule_action_processor/processors/modify_request_processor.js`:
- Around line 71-83: The failure path in modify_request_processor’s catch block
is leaking internal sandbox details because modify_request writes the returned
string into ctx.rq_request_body and forwards it upstream. Update the error
handling around modify_request so execution failures do not replace the outgoing
request body with the raw sandbox message; instead, preserve the original body
or route the diagnostic to a developer-facing channel, and guard access to
error.message for non-Error throws. Use modify_request_processor and
modify_request as the key symbols when locating the fix.
In `@src/utils/sandbox-globals.ts`:
- Around line 371-375: The sandbox crypto shim’s randomBytes() currently returns
a plain array, so update the nodeCrypto.randomBytes implementation in
sandbox-globals to wrap the host-provided bytes with the sandbox Buffer before
returning. Keep the existing host call and JSON parsing, but convert the parsed
bytes into a Buffer-compatible value so callers of
require("crypto").randomBytes() can use standard Node Buffer methods like
toString("hex").
---
Outside diff comments:
In `@src/utils/index.ts`:
- Around line 277-345: The host bridge functions can enqueue too many pending
async operations before the CPU interrupt stops execution, so add a small
MAX_INFLIGHT_HOST_OPS limit to cap outstanding work. Update the __hostFetch and
__hostTimer paths in the bridge setup to check the current inflight array size
before pushing new promises, and return a failure/throw a host error once the
limit is reached. Keep the guard centralized near the inflight tracking in
src/utils/index.ts so both fetch and timer calls are covered consistently.
---
Duplicate comments:
In `@src/utils/index.ts`:
- Around line 250-254: Protect the shared state update in the flow around
getSharedStateCopy, the async host wait, and setSharedState by making the
read→execute→write sequence atomic. Use an async mutex or compare-and-swap style
guard in the logic that snapshots shared state, yields during the host wait, and
later writes back, so a stale sharedStateJson cannot overwrite newer changes
from another rule. If full atomicity is not possible, refresh or merge against
the latest state immediately before setSharedState in the same execution path.
🪄 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: cea1e224-151e-4c17-b992-4681b16aa39f
⛔ Files ignored due to path filters (6)
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/**dist/utils/sandbox-globals.d.tsis excluded by!**/dist/**dist/utils/sandbox-globals.jsis excluded by!**/dist/**
📒 Files selected for processing (4)
src/components/proxy-middleware/rule_action_processor/processors/modify_request_processor.jssrc/components/proxy-middleware/rule_action_processor/processors/modify_response_processor.jssrc/utils/index.tssrc/utils/sandbox-globals.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/proxy-middleware/rule_action_processor/processors/modify_response_processor.js
Comment on lines
71
to
83
| } catch (error) { | ||
| // Function parsed but failed to execute | ||
| // Function parsed but failed to execute. Code 188 = sandbox-internal (our shim | ||
| // broke); 187 = the rule author's code. error.message now carries the real | ||
| // sandbox error (previously swallowed). | ||
| const code = error && error.kind === "prelude" ? 188 : 187; | ||
| return modify_request( | ||
| ctx, | ||
| "Can't execute Requestly function. Please recheck. Error Code 187. Actual Error: " + | ||
| "Can't execute Requestly function. Please recheck. Error Code " + | ||
| code + | ||
| ". Actual Error: " + | ||
| error.message | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
On execution failure the request body is overwritten with the raw sandbox error and forwarded upstream.
modify_request assigns ctx.rq_request_body = new_req, so this catch replaces the outgoing request body with the error string — now including the raw error.message. Unlike modify_response_processor.js, where the message is surfaced back to the developer, here it is sent to the origin server, leaking internal sandbox detail. Also, for non-Error throws error.message will render as undefined.
Consider guarding the message and reconsidering whether the upstream request body should carry sandbox internals.
Verify how rq_request_body is consumed after this processor runs (i.e., whether it is forwarded to the origin):
#!/bin/bash
rg -nP -C3 '\brq_request_body\b' --type=js🤖 Prompt for 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.
In
`@src/components/proxy-middleware/rule_action_processor/processors/modify_request_processor.js`
around lines 71 - 83, The failure path in modify_request_processor’s catch block
is leaking internal sandbox details because modify_request writes the returned
string into ctx.rq_request_body and forwards it upstream. Update the error
handling around modify_request so execution failures do not replace the outgoing
request body with the raw sandbox message; instead, preserve the original body
or route the diagnostic to a developer-facing channel, and guard access to
error.message for non-Error throws. Use modify_request_processor and
modify_request as the key symbols when locating the fix.
The host bridge returns the random bytes as a plain JSON array (only data crosses
the sandbox boundary), so randomBytes(n) handed the guest a plain Array and
randomBytes(16).toString('hex') produced comma-joined decimals instead of hex.
Wrap the crossed-over bytes in the guest Buffer shim so the return type matches
Node (toString('hex'/'base64'/'utf8') work). Entropy is unchanged (real host
node:crypto); only the guest-side container type is restored.
Addresses CodeRabbit review on PR #112.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
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-core+ the 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 alternatives don't fit:worker_threadscannot create a Worker in an Electron renderer, and the proxy runs in the desktop's background renderer.Security model — the boundary
Only JSON-serialisable data crosses the host edge. No host object is ever handed to the guest, so there is no live reference to walk back to the host realm. Host bridges (crypto, fetch) take copied data in and return copied data out; pure shims never touch the host at all.
Web / Node API compatibility layer
A bare QuickJS realm has none of the web/Node globals scripts used in the old full-host environment. This restores them so existing rule scripts keep working, without weakening the boundary.
Pure in-guest JS shims (no host contact):
URL,URLSearchParams,TextEncoder/TextDecoder,structuredClone,atob/btoa,Buffer,Blob,FormData,Request,Response,Headers,setTimeout/setInterval/clearTimeout/clearInterval,queueMicrotask,performance,console(captured intoconsoleLogs).Host bridges (copy-in / copy-out only):
crypto—randomUUID,getRandomValues,createHash,createHmac,subtle.digestfetch— real HTTP via the host, driven by a guest-promise + pump-loop (works on the sync QuickJS variant; avoids the asyncify teardown race)XMLHttpRequest(async) — layered on the fetch bridgerequire('crypto')→ the crypto bridge; any otherrequire(...)throws a guided errorfetchpolicy:http/httpsURLs only;credentials: 'omit'(a shared rule can't ride the user's ambient cookies/sessions).Engine note: imports from
quickjs-emscripten-core(not the umbrellaquickjs-emscripten, whose auto-loader statically references every WASM variant and breaks the desktop's webpack bundle). core + the single embedded variant is bundler-safe — same choice as@requestly/sandbox-node.Limitations (by design)
WebSocketXMLHttpRequest(open(...,false))fetchcrypto.subtledigest;sign/verify/importKey/generateKeydeferred (CryptoKey can't cross the copy boundary). HMAC signing viarequire('crypto').createHmacsetIntervalsetTimeoutdoes honor the real delay (clamped to the execution's wall-clock budget)fetchbodiesFormData/Blobmultipart parts cross as text (binary best-effort)fetchegressCode layout
src/utils/index.ts— host side: module/context lifecycle, the__hostCrypto/__hostFetchbridge handlers, the pump loop,executeUserFunction(per-step CPU interrupt + 15s wall-clock cap incl. async I/O, 128 MB mem cap).isValidFunctionStringcompiles vianew Functionwithout calling it (parse-only; novm).src/utils/sandbox-globals.ts— guest side: all the in-guest source (SANDBOX_POLYFILLS,SANDBOX_BRIDGE_SHIMS,SANDBOX_EXTRA_SHIMS,SANDBOX_SETUP).consolecaptured,$sharedStateread + written back (snapshot read after the lastawaitso the read-modify-write is atomic).Error handling & observability
Sandbox failures are categorised and surfaced (previously they were swallowed — a regressed shim degraded to a generic 187 with no signal).
.then, so synchronous throws are captured like async ones (no longer leak out as untracked eval errors).executeUserFunctionthrows a typedSandboxError { kind: "prelude" | "user" | "timeout" }carrying the real message; success still returns the result. A CPU-deadline interrupt is classifiedtimeout.reportSandboxError():console.error("[rq-sandbox]", kind, msg)(host-visible) for all kinds;prelude/timeoutalsoSentry.captureExceptiontaggedsandbox: <kind>(guarded — Sentry may be uninitialised).user→ console only.Verified
fetchpolicy.fetch/XHR/crypto(hash/hmac/subtle.digest)/Buffer/Blob/FormData/Request/Response/timers/URL/encoding all pass;file:///ftp://blocked;WebSocketthrows;$sharedStatepersists across requests;process/require('fs')/constructor-escape all unreachable.Downstream / ship order
Desktop needs only a proxy dep bump (folded into #358) — no desktop code change. Ships with the Node-24
writeHeadfix (#113), which is required for interception on Electron 42 (Node 24). Merge #112 + #113 → publish proxy → bump dep in #358.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores