fix(security): RQ-2426 — Safe (sandbox) + Dev script-execution modes#118
Open
dinex-dev wants to merge 10 commits into
Open
fix(security): RQ-2426 — Safe (sandbox) + Dev script-execution modes#118dinex-dev wants to merge 10 commits into
dinex-dev wants to merge 10 commits into
Conversation
"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>
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>
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>
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>
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>
…for code rules Re-lands the QuickJS-WASM sandbox for "code" Modify Request/Response rules (previously #112, reverted by #115) as the DEFAULT, and adds an opt-in DEV mode that runs rule code via the legacy new Function() with full host access. - types: ProxyConfig.devScriptMode (default false = safe) - utils: executeUserFunction dispatches by mode — sandboxed (QuickJS) vs legacy (full access). Only devMode === true selects dev (fail-safe). - proxy-middleware/rq-proxy: per-request mode stamped from proxyConfig, live setDevScriptMode() setter (no restart), loud warning on enable. - processors: pass mode (never derived from rule content); on a sandbox-blocked error in safe mode, append a link to the Developer Script Mode doc. - includes the #113 Node-24/Electron-42 writeHead guards (already on master). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WalkthroughAdds QuickJS dependencies and a sandbox prelude with guest implementations for common encoding, binary, URL, HTTP, crypto, network, timer, and cloning APIs. User functions now run in safe sandbox mode by default, with explicit developer mode retaining host execution. Proxy configuration and runtime APIs propagate the mode per request. Request and response processors use compile-only validation and structured error handling, while response paths guard against repeated header writes. Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.3)src/components/proxy-middleware/rule_action_processor/processors/modify_request_processor.jsFile contains syntax errors that prevent linting: Line 1: Illegal use of an import declaration outside of a module; Line 3: Illegal use of an import declaration outside of a module; Line 6: Illegal use of an import declaration outside of a module; Line 7: Illegal use of an import declaration outside of a module; Line 8: Illegal use of an import declaration outside of a module; Line 106: Illegal use of an export declaration outside of a module src/components/proxy-middleware/index.jsFile contains syntax errors that prevent linting: Line 3: Illegal use of an import declaration outside of a module; Line 4: Illegal use of an import declaration outside of a module; Line 5: Illegal use of an import declaration outside of a module; Line 16: Illegal use of an import declaration outside of a module; Line 17: Illegal use of an import declaration outside of a module; Line 18: Illegal use of an import declaration outside of a module; Line 19: Illegal use of an import declaration outside of a module; Line 20: Illegal use of an import declaration outside of a module; Line 21: Illegal use of an import declaration outside of a module; Line 22: Illegal use of an import declaration outside of a module; Line 23: Illegal use of an import declaration outside of a module; Line 24: Illegal use of an import declaration outside of a module; Line 28: Illegal use of an export declaration outside of a module; Line 510: Illegal use of an export declaration outside of a module src/components/proxy-middleware/rule_action_processor/processors/modify_response_processor.jsFile contains syntax errors that prevent linting: Line 1: Illegal use of an import declaration outside of a module; Line 3: Illegal use of an import declaration outside of a module; Line 6: Illegal use of an import declaration outside of a module; Line 7: Illegal use of an import declaration outside of a module; Line 8: Illegal use of an import declaration outside of a module; Line 9: Illegal use of an import declaration outside of a module; Line 15: Illegal use of an import declaration outside of a module; Line 200: Illegal use of an export declaration outside of a module 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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/index.js`:
- Around line 163-168: Update the devScriptMode assignment in the proxy
middleware to enable script execution only when ProxyConfig.devScriptMode is
explicitly true. Remove the process.env.RQ_SCRIPT_EXECUTION_MODE fallback,
preserving false behavior when configuration is unset or explicitly disabled.
In
`@src/components/proxy-middleware/rule_action_processor/processors/modify_request_processor.js`:
- Around line 85-90: Normalize caught values before constructing error messages
in the request and response processors. In modify_request_processor.js lines
85-90 and modify_response_processor.js lines 179-184, derive the message using
the error’s message when available and otherwise stringify the caught value,
safely handling null and non-Error throws; use that normalized message in the
existing error text.
- Around line 78-80: Accept empty-string rule results by validating only the
string type before calling modify_request in
src/components/proxy-middleware/rule_action_processor/processors/modify_request_processor.js:78-80,
and apply the same typeof-only check before modify_response in
src/components/proxy-middleware/rule_action_processor/processors/modify_response_processor.js:172-174;
retain the existing errors for non-string results.
In `@src/utils/index.ts`:
- Around line 194-217: Update hostFetchOp to prevent SSRF by enforcing an
explicit safe destination allowlist or validating resolved addresses for the
initial URL and every redirect hop. Block localhost, private, link-local, cloud
metadata, and other internal destinations while preserving HTTP/HTTPS support,
and ensure redirects are inspected rather than followed unchecked.
- Around line 254-268: Serialize the $sharedState read-modify-write transaction
across both safe and legacy executor paths: protect each snapshot, user
execution/awaits, and setSharedState commit with a shared mutex or equivalent
versioned compare-and-swap. Update the flow around the visible
getSharedStateCopy() and setSharedState() calls, including the additional
ranges, so concurrent rules cannot overwrite changes from another execution.
- Around line 218-220: Update the fetch handling around resp.arrayBuffer() to
consume the response as a stream, enforce MAX_FETCH_BODY_BYTES while reading,
and abort immediately when the limit is exceeded instead of buffering the full
body. Track concurrent and total fetch calls at the guest/request execution
scope, reject calls beyond those limits, and retain each request’s
AbortController so the execution cleanup finally block aborts all outstanding
requests after the guest settles.
- Around line 516-525: Update the console capture flow around
generatedFunction(args) to use try/finally, ensuring consoleCapture.stop()
always executes when synchronous execution throws or the awaited promise
rejects. Preserve finalResponse assignment and consoleLogs retrieval after
successful cleanup.
In `@src/utils/sandbox-globals.ts`:
- Around line 378-390: Update the createHash and createHmac bridge
implementations, including the corresponding code at the additional affected
section, to preserve binary input. Replace String(d) and String(key) coercion
with byte-safe base64 marshalling and explicit encoding metadata, and ensure the
host-side bridge decodes those bytes before hashing, HMAC computation, or upload
handling.
🪄 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: 903d5ec3-b014-4f63-b792-fc609cd70fb5
⛔ Files ignored due to path filters (12)
dist/components/proxy-middleware/index.d.tsis excluded by!**/dist/**dist/components/proxy-middleware/index.jsis excluded by!**/dist/**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/rq-proxy.d.tsis excluded by!**/dist/**dist/rq-proxy.jsis excluded by!**/dist/**dist/types/index.d.tsis 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/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.jsonsrc/components/proxy-middleware/index.jssrc/components/proxy-middleware/rule_action_processor/processors/modify_request_processor.jssrc/components/proxy-middleware/rule_action_processor/processors/modify_response_processor.jssrc/rq-proxy.tssrc/types/index.tssrc/utils/index.tssrc/utils/sandbox-globals.ts
Comment on lines
+163
to
+168
| const cfgDevScriptMode = this.proxyConfig?.devScriptMode; | ||
| ctx.rq.devScriptMode = | ||
| cfgDevScriptMode === true || | ||
| (cfgDevScriptMode === undefined && | ||
| typeof process !== "undefined" && | ||
| process?.env?.RQ_SCRIPT_EXECUTION_MODE === "dev"); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not silently enable full-host execution from the environment.
When configuration is unset, RQ_SCRIPT_EXECUTION_MODE=dev enables the legacy RCE-capable path despite the cross-layer contract requiring ProxyConfig.devScriptMode === true. Remove the fallback, or convert it into explicit configuration before proxy startup with equivalent user-visible consent.
🤖 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/index.js` around lines 163 - 168, Update the
devScriptMode assignment in the proxy middleware to enable script execution only
when ProxyConfig.devScriptMode is explicitly true. Remove the
process.env.RQ_SCRIPT_EXECUTION_MODE fallback, preserving false behavior when
configuration is unset or explicitly disabled.
Comment on lines
78
to
80
| if (finalRequest && typeof finalRequest === "string") { | ||
| return modify_request(ctx, finalRequest); | ||
| } else throw new Error("Returned value is not a string"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Accept empty-string rule results.
The truthiness check rejects "", so rules intended to clear a body instead inject an execution-error message.
src/components/proxy-middleware/rule_action_processor/processors/modify_request_processor.js#L78-L80: check onlytypeof finalRequest === "string".src/components/proxy-middleware/rule_action_processor/processors/modify_response_processor.js#L172-L174: check onlytypeof finalResponse === "string".
📍 Affects 2 files
src/components/proxy-middleware/rule_action_processor/processors/modify_request_processor.js#L78-L80(this comment)src/components/proxy-middleware/rule_action_processor/processors/modify_response_processor.js#L172-L174
🤖 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 78 - 80, Accept empty-string rule results by validating only the
string type before calling modify_request in
src/components/proxy-middleware/rule_action_processor/processors/modify_request_processor.js:78-80,
and apply the same typeof-only check before modify_response in
src/components/proxy-middleware/rule_action_processor/processors/modify_response_processor.js:172-174;
retain the existing errors for non-string results.
Comment on lines
+85
to
+90
| const code = error && error.kind === "prelude" ? 188 : 187; | ||
| let message = | ||
| "Can't execute Requestly function. Please recheck. Error Code " + | ||
| code + | ||
| ". Actual Error: " + | ||
| error.message; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Normalize caught values before reading .message.
Developer-mode functions can throw strings, objects, or null; the current catch path can report undefined or throw again.
src/components/proxy-middleware/rule_action_processor/processors/modify_request_processor.js#L85-L90: derive a safe message withString(error?.message ?? error).src/components/proxy-middleware/rule_action_processor/processors/modify_response_processor.js#L179-L184: apply the same normalization.
📍 Affects 2 files
src/components/proxy-middleware/rule_action_processor/processors/modify_request_processor.js#L85-L90(this comment)src/components/proxy-middleware/rule_action_processor/processors/modify_response_processor.js#L179-L184
🤖 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 85 - 90, Normalize caught values before constructing error messages
in the request and response processors. In modify_request_processor.js lines
85-90 and modify_response_processor.js lines 179-184, derive the message using
the error’s message when available and otherwise stringify the caught value,
safely handling null and non-Error throws; use that normalized message in the
existing error text.
Comment on lines
+194
to
+217
| async function hostFetchOp(req: any): Promise<any> { | ||
| const hostFetch: any = (globalThis as any).fetch; | ||
| if (typeof hostFetch !== "function") { | ||
| throw new Error("fetch is not available in this environment"); | ||
| } | ||
| let parsedUrl: URL; | ||
| try { | ||
| parsedUrl = new URL(String(req.url)); | ||
| } catch { | ||
| throw new Error("Invalid URL"); | ||
| } | ||
| if (parsedUrl.protocol !== "http:" && parsedUrl.protocol !== "https:") { | ||
| throw new Error("Only http and https URLs are allowed in sandboxed rules"); | ||
| } | ||
| const controller = new AbortController(); | ||
| const timer = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS); | ||
| try { | ||
| const resp = await hostFetch(parsedUrl.toString(), { | ||
| method: req.method || "GET", | ||
| headers: req.headers || {}, | ||
| body: req.body, | ||
| signal: controller.signal, | ||
| credentials: "omit", | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Block private and internal destinations in the fetch bridge.
Scheme validation still permits SSRF to localhost, private networks, link-local/cloud metadata endpoints, and private redirect targets. credentials: "omit" does not prevent this. Validate resolved addresses and every redirect hop, or enforce an explicit destination allowlist.
🤖 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 194 - 217, Update hostFetchOp to prevent
SSRF by enforcing an explicit safe destination allowlist or validating resolved
addresses for the initial URL and every redirect hop. Block localhost, private,
link-local, cloud metadata, and other internal destinations while preserving
HTTP/HTTPS support, and ensure redirects are inspected rather than followed
unchecked.
Comment on lines
+218
to
+220
| const buf = await resp.arrayBuffer(); | ||
| if (buf.byteLength > MAX_FETCH_BODY_BYTES) { | ||
| throw new Error("response body exceeds sandbox size limit"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Enforce fetch limits before buffering and cancel orphaned requests.
arrayBuffer() allocates the complete response before applying the 25 MB cap. A rule can also start unawaited fetches that continue after the guest settles and is disposed. Stream and abort at the limit, cap concurrent/total calls, and abort outstanding controllers in finally.
Also applies to: 306-329, 421-429, 473-475
🤖 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 218 - 220, Update the fetch handling around
resp.arrayBuffer() to consume the response as a stream, enforce
MAX_FETCH_BODY_BYTES while reading, and abort immediately when the limit is
exceeded instead of buffering the full body. Track concurrent and total fetch
calls at the guest/request execution scope, reject calls beyond those limits,
and retain each request’s AbortController so the execution cleanup finally block
aborts all outstanding requests after the guest settles.
Comment on lines
+254
to
+268
| // Read the $sharedState snapshot AFTER the last await. Everything from here | ||
| // to setSharedState() below runs synchronously (no further yields), so the | ||
| // read-modify-write is atomic w.r.t. the event loop. Reading before the | ||
| // await would let a concurrent executeUserFunction commit in the gap, and | ||
| // this call's stale snapshot would then clobber it (last-writer-wins). | ||
| let sharedStateJson = "{}"; | ||
| try { | ||
| sharedStateJson = JSON.stringify( | ||
| GlobalStateProvider.getInstance().getSharedStateCopy() ?? {} | ||
| ); | ||
| } catch { | ||
| sharedStateJson = "{}"; | ||
| } | ||
|
|
||
| const vm = QuickJS.newContext(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Serialize $sharedState read-modify-write operations.
The safe executor yields for host I/O after taking its snapshot, and the legacy executor yields while awaiting user promises. Concurrent rules can therefore read the same version and overwrite each other. Use a shared mutex or versioned compare-and-swap across snapshot, execution, and commit in both modes.
Also applies to: 425-428, 467-468, 509-530
🤖 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 254 - 268, Serialize the $sharedState
read-modify-write transaction across both safe and legacy executor paths:
protect each snapshot, user execution/awaits, and setSharedState commit with a
shared mutex or equivalent versioned compare-and-swap. Update the flow around
the visible getSharedStateCopy() and setSharedState() calls, including the
additional ranges, so concurrent rules cannot overwrite changes from another
execution.
Comment on lines
+516
to
+525
| const consoleCapture = new ConsoleCapture(); | ||
| consoleCapture.start(true); | ||
|
|
||
| let finalResponse = generatedFunction(args); | ||
| if (utilTypes.isPromise(finalResponse)) { | ||
| finalResponse = await finalResponse; | ||
| } | ||
|
|
||
| consoleCapture.stop(); | ||
| const consoleLogs = consoleCapture.getCaptures(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section with line numbers.
sed -n '470,560p' src/utils/index.ts | cat -nRepository: requestly/requestly-proxy
Length of output: 4051
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find surrounding callers and any other console capture cleanup patterns.
rg -n "executeUserFunctionLegacy|consoleCapture\.stop\(\)|new ConsoleCapture\(" src/utils/index.ts src -g '!**/dist/**'Repository: requestly/requestly-proxy
Length of output: 705
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "executeUserFunctionLegacy|consoleCapture\.stop\(\)|new ConsoleCapture\(" src/utils/index.ts src -g '!**/dist/**'Repository: requestly/requestly-proxy
Length of output: 705
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate ConsoleCapture definition and inspect its implementation.
fd -a "ConsoleCapture" .
rg -n "class ConsoleCapture|function ConsoleCapture|export .*ConsoleCapture" src .Repository: requestly/requestly-proxy
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "ConsoleCapture" .Repository: requestly/requestly-proxy
Length of output: 310
Guard console capture cleanup with try/finally
If generatedFunction(args) throws or returns a rejected promise, consoleCapture.stop() never runs and console interception stays active for later requests. Wrap the execution and cleanup in try/finally.
🤖 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 516 - 525, Update the console capture flow
around generatedFunction(args) to use try/finally, ensuring
consoleCapture.stop() always executes when synchronous execution throws or the
awaited promise rejects. Preserve finalResponse assignment and consoleLogs
retrieval after successful cleanup.
Comment on lines
+378
to
+390
| createHash: function (algo) { | ||
| var buf = ""; | ||
| return { | ||
| update: function (d) { buf += String(d); return this; }, | ||
| digest: function (enc) { return JSON.parse(__hostCrypto(JSON.stringify({ op: "hash", algo: algo, data: buf, encoding: enc || "hex" }))).digest; } | ||
| }; | ||
| }, | ||
| createHmac: function (algo, key) { | ||
| var buf = ""; | ||
| return { | ||
| update: function (d) { buf += String(d); return this; }, | ||
| digest: function (enc) { return JSON.parse(__hostCrypto(JSON.stringify({ op: "hmac", algo: algo, key: String(key), data: buf, encoding: enc || "hex" }))).digest; } | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep binary data binary across the host bridges.
String(d)/String(key) hashes comma-delimited byte values, while Blob/FormData bytes are UTF-8-decoded before fetch. This changes hashes/HMACs and corrupts binary uploads. Marshal bytes as base64 with explicit encoding metadata and decode host-side.
Also applies to: 408-431
🤖 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 378 - 390, Update the createHash
and createHmac bridge implementations, including the corresponding code at the
additional affected section, to preserve binary input. Replace String(d) and
String(key) coercion with byte-safe base64 marshalling and explicit encoding
metadata, and ensure the host-side bridge decodes those bytes before hashing,
HMAC computation, or upload handling.
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
1 participant
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.
RQ-2426 — Safe / Dev script-execution modes for code rules
Ticket: RQ-2426 (part of RQ-3555) · PR 1 of 3 (proxy → desktop → web)
What
"Code" Modify Request/Response rules run rule-supplied JS. This makes the secure
QuickJS-WASM sandbox the default (no
require/process/fs) and adds an opt-inDeveloper Script Mode that runs rule code via the legacy
new Function()withfull host access.
ProxyConfig.devScriptMode(defaultfalse= safe). Only an explicittrueenables dev mode — fail-safe against missing/corrupt config.
executeUserFunctiondispatches by mode; mode is stamped per-request fromproxy config and never derived from rule content.
setDevScriptMode()setter — applies on the running proxy, no restart.Developer Script Mode doc.
Notes for reviewers
default, with dev mode as the escape hatch. Please confirm the original revert
reason is addressed.
writeHeadguards (already on master); thisbranch is behind master and will need a rebase before merge.
the Electron 42 desktop app (safe-mode confirmed on a live rule).
Release order
Merge → publish
@requestly/requestly-proxy→ desktop PR bumps the dep → web PR.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes