fix(shims): skip body copy for GET/HEAD in NextRequest constructor#887
fix(shims): skip body copy for GET/HEAD in NextRequest constructor#887agm151515 wants to merge 1 commit intocloudflare:mainfrom
Conversation
The NextRequest constructor unconditionally forwarded req.body to super() when constructing from a Request, which throws "Request with a GET or HEAD method cannot have a body." whenever the input request's body is a non-null ReadableStream. In Cloudflare Workers, incoming GET/HEAD requests expose body as a non-null stream when the request carries Content-Length or Transfer-Encoding framing (e.g. some email image-proxy fetchers of tracking pixels), which made middleware throw and respond 500 on every affected request. Gates body/duplex behind a method check. Regression test constructs an input Request whose body getter returns a non-null stream and asserts NextRequest wraps it without throwing.
|
/bigbonk review |
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the detailed bug report and reproduction steps — this is a real issue and the root-cause analysis is accurate. The fix is minimal and the regression test is good. Unit tests pass locally (vp test run tests/shims.test.ts → 817/817).
A few points:
1. Same bug pattern exists in config/config-matchers.ts — please fix in this PR
applyMiddlewareRequestHeaders in packages/vinext/src/config/config-matchers.ts:496-502 reconstructs the request the exact same way:
request = new Request(request.url, {
method: request.method,
headers: nextHeaders,
body: request.body,
// @ts-expect-error — duplex needed for streaming request bodies
duplex: request.body ? "half" : undefined,
});This has the identical failure mode: an incoming GET/HEAD with Content-Length / Transfer-Encoding framing (which exposes a non-null body ReadableStream in Workers) will throw TypeError: Request with a GET or HEAD method cannot have a body.. It runs on the post-middleware path in both server/prod-server.ts:1500 and deploy.ts:720, so production traffic hitting any route with middleware + config headers/rewrites will 500 for the same email-pixel / tracking GET requests.
Per AGENTS.md ("Always check dev and prod server parity" / "Do not leave known bugs as 'follow-ups' — fix them in the same PR"), this parallel bug site should be fixed here. Suggested patch:
const passBody = request.method !== "GET" && request.method !== "HEAD";
request = new Request(request.url, {
method: request.method,
headers: nextHeaders,
...(passBody
? {
body: request.body,
// @ts-expect-error — duplex needed for streaming request bodies
duplex: request.body ? "half" : undefined,
}
: {}),
});Please also add a matching regression test that exercises applyMiddlewareRequestHeaders with a GET request that has a non-null body.
2. Simpler alternative worth considering (not a blocker)
Next.js itself sidesteps this whole class of bug by just doing super(input, init) directly when input instanceof Request (source). Modern workerd and undici handle new Request(request, init) correctly per the Fetch spec — including the method/body compatibility handoff. The existing "to avoid Node.js undici issues" comment in shims/server.ts predates undici's fix and is now stale.
Switching to Next.js's pattern would cover additional edge cases (e.g. new NextRequest(postRequest, { method: "GET" }), signal forwarding, method normalization) for free. Up to you whether to do it in this PR or follow up separately.
3. Edge case left unfixed (minor, pre-existing)
If a caller does new NextRequest(postRequest, { method: "GET" }), passBody is computed from req.method (POST) so body is still spread in, and then method: "GET" in requestInit overrides and the constructor throws. Pre-existing, not introduced by this PR. Would be resolved automatically by option 2 above.
4. Parity with Next.js test suite
For reference, Next.js's own test/unit/web-runtime/next-request.test.ts confirms that new NextRequest(postRequestWithBody) preserving the body is the expected behavior, so the positive POST test here matches upstream. Consider adding a // Ported from Next.js: ... comment for future maintainers.
On the commit message / style: this already matches the repo's conventional-commit style — no changes needed.
| body: req.body, | ||
| // @ts-expect-error - duplex is not in RequestInit type but needed for streams | ||
| duplex: req.body ? "half" : undefined, | ||
| ...(passBody ? { body: req.body, duplex: req.body ? "half" : undefined } : {}), |
There was a problem hiding this comment.
Minor: the method comparison is correct because the Request constructor normalizes method to uppercase per spec, so req.method is always "GET" / "HEAD" / etc. Worth a brief inline comment noting that invariant, since the check silently depends on it.
Also: if a caller passes requestInit with method: "GET" while the input Request is POST, passBody will be computed from the POST method (so body is spread in) and then the method: "GET" from requestInit overrides — which would re-trigger the original TypeError. Pre-existing edge case, not introduced here, but worth noting. Computing passBody from requestInit?.method ?? req.method would also cover that case:
| ...(passBody ? { body: req.body, duplex: req.body ? "half" : undefined } : {}), | |
| ...(() => { | |
| // Request constructor normalizes method to uppercase, so comparing | |
| // against "GET" / "HEAD" is spec-safe. Also honor `requestInit.method` | |
| // if present — otherwise an override like `{ method: "GET" }` on a | |
| // POST input would re-trigger the original TypeError. | |
| const method = (requestInit?.method ?? req.method).toUpperCase(); | |
| const passBody = method !== "GET" && method !== "HEAD"; | |
| return passBody ? { body: req.body, duplex: req.body ? "half" : undefined } : {}; | |
| })(), |
| Object.defineProperty(req, "body", { | ||
| configurable: true, | ||
| get: () => new ReadableStream(), | ||
| }); |
There was a problem hiding this comment.
The getter returns a fresh ReadableStream() on every access, so getReq.body !== getReq.body. Harmless for the current assertions, but fragile — a future reader who calls getReq.body more than once in setup will get surprising behavior. Consider a value descriptor instead:
| }); | |
| const stream = new ReadableStream(); | |
| Object.defineProperty(req, "body", { | |
| configurable: true, | |
| value: stream, | |
| }); |
| const headReq = withFakeBody("HEAD"); | ||
| const wrappedHead = new NextRequest(headReq); | ||
| expect(wrappedHead.body).toBeNull(); | ||
| expect(wrappedHead.method).toBe("HEAD"); |
There was a problem hiding this comment.
Consider also asserting the original symptom directly, since the current wrappedGet.body / wrappedHead.body assertions only cover it implicitly (the constructor would have thrown before reaching them):
expect(() => new NextRequest(getReq)).not.toThrow();
expect(() => new NextRequest(headReq)).not.toThrow();That makes the regression intent match the bug report verbatim and will survive future refactors that change how body is exposed on the wrapped request.
| const post = new Request("http://localhost/x", { method: "POST", body: "hello" }); | ||
| const wrapped = new NextRequest(post); | ||
| expect(wrapped.method).toBe("POST"); | ||
| expect(await wrapped.text()).toBe("hello"); |
There was a problem hiding this comment.
Nit: this mirrors Next.js's own unit test at test/unit/web-runtime/next-request.test.ts. Per AGENTS.md ("Searching the Next.js Test Suite"), it's worth adding a // Ported from Next.js: ... comment with the link, so future maintainers know the positive-path behavior is tied to upstream parity rather than ad-hoc.
|
Review posted successfully: #887 (review) SummaryPosted a batch review on PR #887 with the following feedback: Overall verdict: The fix is correct and addresses a real production bug. Unit tests pass locally (817/817 in Key findings:
|
2 participants
Summary
When
NextRequestis constructed from an existingRequest, itsconstructor forwards
body: req.bodytosuper()unconditionally.The Fetch spec (and workerd) throw
TypeError: Request with a GET or HEAD method cannot have a body.whenever
init.bodyis non-null and the method isGET/HEAD.In Cloudflare Workers, incoming
GET/HEADrequests exposerequest.bodyas a non-nullReadableStreamwhenever the requestcarries
Content-LengthorTransfer-Encodingframing. That happensin the wild — e.g. some email image-proxy fetchers of tracking pixels
send
GETwith aContent-Lengthheader — so the proxy/middlewareblock in the generated RSC entry throws on every affected request,
logs
[vinext] Middleware error: TypeError: …, and returns 500.Reproduced via
wrangler dev --localagainst a minimal worker:Fix gates
body/duplexbehind a method check. Behavior forPOST/PUT/PATCH/DELETEetc. is unchanged.Test plan
tests/shims.test.tsreproduces the bugin Node (by overriding
bodyon a normal Request viaObject.defineProperty) — fails before the fix, passes after.NextRequesttests still pass (pnpm test:unit -t NextRequest→ 21 passed).pnpm test:unit→ 2971/2971).pnpm fmt:checkandpnpm lintclean on modified files.Downstream context for maintainers: this was observed in production on
scoregap.com with vinext 0.0.43. Filed locally as GitHub issue #35 in
our app repo. Happy to adjust style/commit-msg to match project
conventions.