-
Notifications
You must be signed in to change notification settings - Fork 324
fix(shims): skip body copy for GET/HEAD in NextRequest constructor #887
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4963,6 +4963,47 @@ describe("NextURL basePath and locale properties", () => { | |||||||||||||
| expect(req.nextUrl.pathname).toBe("/dashboard"); | ||||||||||||||
| expect(req.nextUrl.href).toBe("http://localhost/app/fr/dashboard"); | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| // Regression: in Cloudflare Workers, an incoming GET/HEAD request that | ||||||||||||||
| // carries Content-Length or Transfer-Encoding framing exposes | ||||||||||||||
| // `request.body` as a non-null ReadableStream. Copying that into super()'s | ||||||||||||||
| // init made the Request constructor throw | ||||||||||||||
| // "Request with a GET or HEAD method cannot have a body.", which broke | ||||||||||||||
| // every middleware invocation for affected traffic (e.g. email-pixel GETs). | ||||||||||||||
| // We reproduce the same shape in Node.js by overriding the body getter on a | ||||||||||||||
| // normal Request instance. | ||||||||||||||
| it("NextRequest does not throw when input Request has a non-null body on GET/HEAD", async () => { | ||||||||||||||
| const { NextRequest } = await import("../packages/vinext/src/shims/server.js"); | ||||||||||||||
|
|
||||||||||||||
| const withFakeBody = (method: "GET" | "HEAD"): Request => { | ||||||||||||||
| const req = new Request("http://localhost/x", { method }); | ||||||||||||||
| Object.defineProperty(req, "body", { | ||||||||||||||
| configurable: true, | ||||||||||||||
| get: () => new ReadableStream(), | ||||||||||||||
| }); | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The getter returns a fresh
Suggested change
|
||||||||||||||
| return req; | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| const getReq = withFakeBody("GET"); | ||||||||||||||
| expect(getReq.body).not.toBeNull(); | ||||||||||||||
| const wrappedGet = new NextRequest(getReq); | ||||||||||||||
| // Body must not be forwarded into the wrapped request on GET/HEAD. | ||||||||||||||
| expect(wrappedGet.body).toBeNull(); | ||||||||||||||
| expect(wrappedGet.method).toBe("GET"); | ||||||||||||||
|
|
||||||||||||||
| const headReq = withFakeBody("HEAD"); | ||||||||||||||
| const wrappedHead = new NextRequest(headReq); | ||||||||||||||
| expect(wrappedHead.body).toBeNull(); | ||||||||||||||
| expect(wrappedHead.method).toBe("HEAD"); | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider also asserting the original symptom directly, since the current 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 |
||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| it("NextRequest preserves body for non-GET/HEAD methods", async () => { | ||||||||||||||
| const { NextRequest } = await import("../packages/vinext/src/shims/server.js"); | ||||||||||||||
| 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"); | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: this mirrors Next.js's own unit test at |
||||||||||||||
| }); | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| // --------------------------------------------------------------------------- | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: the method comparison is correct because the
Requestconstructor normalizesmethodto uppercase per spec, soreq.methodis always"GET"/"HEAD"/ etc. Worth a brief inline comment noting that invariant, since the check silently depends on it.Also: if a caller passes
requestInitwithmethod: "GET"while the input Request is POST,passBodywill be computed from the POST method (so body is spread in) and then themethod: "GET"fromrequestInitoverrides — which would re-trigger the originalTypeError. Pre-existing edge case, not introduced here, but worth noting. ComputingpassBodyfromrequestInit?.method ?? req.methodwould also cover that case: