fix: patch global Response with a proxy instead of a subclass - #984
Conversation
Replacing globalThis.Response with a subclass leaves Response.prototype owning nothing but `constructor`. Libraries that reflect over the prototype's own properties to build their own Response-alike then wire up nothing and fall through to the native getters with a foreign `this`. srvx's node adapter does exactly this (lazyInherit + setPrototypeOf), so its FastResponse threw "Cannot read private member #state from an object whose class did not declare it" on any native-inherited member. That broke TanStack Start dev servers using srvx as the response handler. Wrapping the native class in a Proxy keeps Response.prototype pointing at the real native prototype and hands back genuine native instances, while the construct trap still scans bodies for leaks. This also removes the need for the Symbol.hasInstance override and the setPrototypeOf call in Response.json, since instanceof and the returned prototype are now correct on their own. Fixes #983
|
The changes in this PR will be included in the next version bump.
|
📦 Bundle size
dist/ only; native binaries are versioned separately and not counted here. |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The constructor proxy fixes native prototype reflection, but its Response.json interception introduces a concrete compatibility regression that should be addressed before merging.
Reviewed changes in 56749fc7, covering the complete three-file patch and the affected runtime behavior.
- Native prototype preservation: Replaces the patched subclass with a constructor proxy so reflection-based response implementations retain access to native prototype members.
- Leak scanning hooks: Keeps constructor-body scanning, virtualizes the patch marker, and routes static
Response.jsonreads through a scanner. - Regression coverage and release metadata: Adds focused tests for reflection, native instances, subclasses, and JSON responses, plus a patch changeset.
azure/gpt-5.6-sol ｜ 𝕏
commit: |
|
Pushed commit Task list (5/5 completed)
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review at effbb900:
- Restored test isolation: Captured and restored the native
Response.jsondescriptor so proxy-forwarded mutations do not leak between tests. - Removed dead test setup: Dropped deletion of the virtual
_patchedByVarlockmarker before restoring the native constructor. - Corrected console patching: Removed the duplicate
infoentry sopatchGlobalConsole()wrapsconsole.infoonce.
azure/gpt-5.6-sol ｜ 𝕏
2 participants
Fixes #983
The problem
@preventLeakbroke srvx-based servers (TanStack Start'sFastResponse, Nitro) with:Root cause
Not srvx-specific in principle: it's a conflict between
patchGlobalResponse()and any library that reflects overResponse.prototype.The patch replaced
globalThis.Responsewith a subclass, and a subclass's.prototypeowns nothing butconstructor. All the real members (body,text(),clone(),bodyUsed, ...) live one level up on the native prototype.srvx's node adapter snapshots
globalThis.Responseat import time and builds itsFastResponsevialazyInherit(), which walks onlyObject.getOwnPropertyNames(source). Against the subclass prototype it finds nothing, forwards nothing, then doesObject.setPrototypeOf(NodeResponse.prototype, NativeResponse.prototype). SofastResponse.bodyfalls straight through to undici's native getter with aNodeResponsereceiver.Ordering is exactly what the Vite integration produces: it patches at config-load time, well before the dev server imports srvx.
Node version changes the message, not the bug: Node 24's undici uses
#state(the reported error), Node 22 uses a symbol and reportsCannot read properties of undefined (reading 'body').The fix
Wrap the native class in a
Proxywith aconstructtrap instead of subclassing it.Response.prototypestays the real native prototype, so reflection-based wrappers see what they expect, and every instance is a genuine nativeResponsewith its internal slots intact.Two cleanups fall out of this:
Symbol.hasInstanceoverride from fix: patchGlobalResponse breaks fetch() instanceof Response checks #385 is removed.instanceofworks natively now, because the proxy'sprototypeis the native one.Object.setPrototypeOf(r, Response.prototype)call inResponse.jsonis removed, for the same reason.Leak detection is unchanged
Verified end-to-end against a live srvx server:
new Response(secret)-> caught by the construct trapResponseentirely, writes straight tonodeRes.write) -> still caught bypatchGlobalServerResponsenew NativeResponse(...)-> now routes through the trap tooBefore/after on the same server, Node 24:
Tests
Four regression tests added. The key one asserts
Response.prototypekeeps its native own-property set after patching, which is the invariant srvx depends on; the others cover native instance construction, subclassing, andResponse.json.For reviewers
I could not scaffold a real TanStack Start app on Windows to confirm there is no second failure behind this one. The srvx incompatibility is real and fixed, but if the reporter still hits an error after this ships, it would be a separate issue.
Review follow-ups
Response.jsonwrites through the proxy land on the native class, so the suite now restores the originaljsondescriptor between tests. Also dropped a now-deaddelete _patchedByVarlock(the flag is virtual, served by the proxy'sgettrap).'info'entry inpatch-console.ts's method list that causedconsole.infoto be wrapped twice.