mirror of
https://github.com/Kong/insomnia.git
synced 2026-10-11 16:43:22 -04:00
Jack Kavanaghandkwburns-kong
7ddf333610
* wip: sendRequest bridge fixes for QuickJS sandbox — known unresolved crash
Not working yet. insomnia.sendRequest() consistently crashes under the
real Electron/Worker/QuickJS environment with:
Aborted(Assertion failed: list_empty(&rt->gc_obj_list), at:
../../vendor/quickjs/quickjs.c,2036,JS_FreeRuntime)
on the FIRST real network round trip (via the
insomnia-templating-worker-database:// fetch bridge to
network.sendRequestWithoutSideEffects). This never reproduces in vitest
(mocked fetch, or a real Node http server) — only under real Electron
fetch() timing inside the dedicated Worker.
This commit includes three targeted fixes attempted so far, none of
which resolved it on their own or together:
1. insomnia.sendRequest('url string') omitted `headers` entirely; the
host handler doesn't default it, crashing
createConfiguredCurlInstance's `headers.find(...)` user-agent lookup.
This one IS a real, confirmed bug (unit-tested regression added) —
just not sufficient by itself to fix the crash above.
2. Deferred sendRequestViaFetch by one Promise.resolve().then() tick so
it doesn't run nested inside the QuickJS C-to-JS callback frame
(matching host-bridge.ts's installHostBridge pattern for the
template-tag sandbox).
3. executePendingJobs() returns a DisposableResult that must itself be
disposed — discarding it leaked a handle. Fixed in both
installSendRequestBridge and driveTaskToCompletion.
4. Added a disposedRef guard so a late-arriving fetch response after
vm.dispose() is a safe no-op instead of touching a dead context.
Also includes the smoke test (quickjs-script-sandbox.test.ts) that
reproduces the crash reliably — the second test in that file (worker
UI-responsiveness) passes; the sendRequest round-trip test does not.
Branched off PR 2 (37d19c1da8) per the rollout plan —
https://gist.github.com/jackkav/3ebf8768bf84be024a3a138874919354 — so
this can be picked up independently without blocking PR 0/PR 1 landing.
Next step for whoever picks this up: bisect further with instrumented
console.error tracing of vm.alive at each step (deferred here, not
committed) to find exactly which call touches vm after disposal — my
three fixes were reasonable hypotheses but didn't close it.
* test: add __sendRequest to the globalThis allowlist after rebase
The merged globalThis-leak test's ALLOWED_EXTRA_GLOBALS predates this
branch's sendRequest bridge landing back on top of develop's rebase target;
without this the merged test fails since __sendRequest is now a real
bridged global.
* fix(quickjs): free bridge promise resolvers before disposing the VM
`vm.newPromise()` allocates three JSValues — the promise plus its `resolve`
and `reject` functions — and quickjs-emscripten frees the two functions only
from inside `resolve()`/`reject()`. `installSendRequestBridge` returned the
promise handle but could skip both (the `disposedRef` guard did exactly that),
so a `sendRequest()` still in flight at teardown left two live function objects
in the runtime and `JS_FreeRuntime` aborted on
`assert(list_empty(&rt->gc_obj_list))` — a native Emscripten abort that takes
down the Worker and the WASM module shared with the template-tag sandbox.
Track every unsettled deferred and dispose it before `vm.dispose()`, which is
the cleanup the library documents for this case. Guard the late-response path
on `vm.alive`/`deferred.alive` instead of a hand-rolled flag.
Also plumb the templating-db auth token from `run-script-quickjs.ts` through
the worker into the engine — it was accepted but never passed, so every bridge
fetch was rejected 401 by the protocol's auth gate.
The crash reproduces in vitest after all: the mocked fetch in the existing
tests settles in the same microtask checkpoint as the call, so the deferred was
always settled before teardown. Settling it on a later macrotask reproduces the
exact production abort, so the smoke test no longer needs `test.fail()`.
* fix(quickjs): return the response value from sendRequestWithoutSideEffects
The handler returned `new Response(JSON.stringify(result))` while every other
handler in `pluginToMainAPI` returns a plain value and lets the caller wrap it.
`resolveDbByKey` does `JSON.stringify(await handler(body))`, so a `Response`
stringified to `"{}"` — the sendRequest bridge received an empty object, and
`response.code` / `response.body` were undefined. The template-tag sandbox's
`network.sendRequestWithoutSideEffects` path was broken the same way.
Also record a second, unrelated `gc_obj_list` abort that the teardown fix does
not address and that predates this branch: allocation-heavy work performed
inside `executePendingJobs()` (anything after the script's first `await`) leaves
live GC objects and aborts at `JS_FreeRuntime`, from ~100k objects up. Reduced
to a reproducer with no bridge, no host functions, no interrupt handler, no
memory limit and no `resolvePromise`; unaffected by every host-side mitigation
tried, and present on both quickjs-emscripten sync variants. Tracked as a
`.fails()` test so it flips red when an engine bump fixes it.
The sendRequest bridge is what makes that latent engine bug routine — it is the
first thing to put multi-megabyte host data into the VM.
Corrects the "later run proves the module survived" comment: a fresh context on
the same WASM module succeeds even after an abort, so that assertion never
detected anything.
* fix(quickjs): address Copilot review feedback on sendRequest bridge
- Normalize request.header (singular, per RequestOptions) as well as
request.headers, and accept {key, value} header entries alongside
{name, value}.
- Coerce request.url to a string, since RequestOptions allows a
Url-like object there and the host handler needs a plain string.
- sendRequestViaFetch no longer unconditionally JSON.parses the
response body — a non-JSON body (empty response, HTML error page,
etc.) now surfaces a clear "non-JSON response"/status-based error
instead of an opaque SyntaxError masking the real failure.
- The callback-style rejection handler no longer assumes err is an
Error instance (guards on err && err.message).
The authToken concern from the same review round was already fixed by
5ad0278188 on this branch (plumbed from run-script-quickjs.ts through
the worker into the engine) before this commit.
Added unit test coverage for all four fixes.
* fix(quickjs): survive the WASM teardown abort instead of losing the run
Allocating ~100k+ objects after a script's first `await` — which a multi-MB
`insomnia.sendRequest()` response body does on its own — leaves QuickJS's
runtime un-freeable, and `JS_FreeRuntime` aborts the WASM module. The script
has already finished by then and everything we return is plain host-side JS, so
the abort was destroying a run whose result was intact.
Catch it at teardown, warn into the script's logs, and report it through a new
`onEngineFault` callback. Catching inside `finally` means a genuine script error
or timeout still wins. The worker forwards the fault as `engineFaulted` and the
client retires that worker, so no later script runs on a module that aborted.
Retirement is drain-aware: `self.onmessage` doesn't serialize, so a second
script can be mid-execution. The worker leaves rotation immediately but is only
terminated once its in-flight runs settle, and the crash handler now rejects
only its own worker's runs rather than every pending call.
Verified before implementing: the abort is catchable, host-collected data
survives it intact, and the module is still *correct* afterwards — arithmetic,
JSON, string and global operations all check out across repeated runs. Retiring
the worker anyway is defensive, since that only samples the behaviour we
happened to test.
This is containment, not a fix. The underlying defect is in quickjs-emscripten's
WASM build, not in QuickJS: the identical vendored engine source built natively
is clean well past the failing size at every optimization level, with and
without -fwrapv, with CONFIG_STACK_CHECK, and with the same raw-context
intrinsic subset the shim uses — while both WASM engines abort at both
optimization levels. Tracked upstream as
https://github.com/justjake/quickjs-emscripten/issues/269.
* fix(quickjs): close three sendRequest bridge findings (#10404)
* fix(quickjs): reject caller-controlled caCertficatePath/authentication in sendRequestWithoutSideEffects
* fix(quickjs): bound the script sandbox's runtime stack size
Unbounded guest recursion overflowed the host WASM stack before QuickJS's own
depth check could fire, masking the real error behind a fatal gc_obj_list
assertion at vm.dispose(). setMaxStackSize(256KB) lets QuickJS's own check win.
* fix(quickjs): deny multipart/file-upload bodies in the sendRequest bridge
network.sendRequestWithoutSideEffects's cacert/authentication fields are already
pinned regardless of caller input, but its `body` was forwarded unvalidated. A
multipart/form-data body's file parts (body.params[].fileName) reach
buildMultipart, which reads each part's fileName off disk with no ownership/
allowlist check -- unlike every other local-file read reachable from a request
definition. A script reaches this by calling the raw __sendRequest bridge global
directly, bypassing insomnia.sendRequest()'s wrapper, which only ever builds a
text/plain body.
Scoped the fix to the QuickJS bridge itself (assertSupportedSendRequestBody,
run before the bridge's fetch() is dispatched) rather than the shared handler,
since that handler is also used by the legacy hidden-window script sandbox and
the template-tag sandbox, each with its own already-privileged execution model
that should keep working as-is.
Two regression tests: one drives the real QuickJS bridge end-to-end and confirms
the request is now denied before it ever reaches curlRequest; the other drives
the legacy (non-QuickJS) sendRequestWithoutSideEffects implementation directly
and confirms it's unaffected -- the same multipart body still goes through.
---------
Co-authored-by: kwburns-kong <kyle.burns@konghq.com>