fix(app-router): copy immutable response headers before finalization - #2787
fix(app-router): copy immutable response headers before finalization#2787NathanDrake2406 wants to merge 6 commits into
Conversation
`export async function GET() { return fetch(upstream) }` — the canonical
proxy/BFF Route Handler — returned 500 on every request, in dev and prod.
A Response produced by `fetch()` carries the Fetch spec's "immutable" headers
guard, so every `set`/`append`/`delete` throws `TypeError: immutable`. vinext
stamped framework headers directly onto the object the handler returned, in two
places: `applyRouteHandlerRevalidateHeader`/`markRouteHandlerCacheMiss` (ISR and
cache-state headers) and `finalizeAppRscResponse` (`Vary`, default
`Cache-Control`, config `headers()`). The finalizer's existing 3xx early return
only shielded `Response.redirect()`.
Next.js copies into `new Headers(res.headers)` and never mutates what userland
returned. Mirror that with `ensureMutableResponse`, which probes the guard and
rebuilds only when it is set, so the mutable fast path stays allocation-free.
Applied at the two ownership boundaries: where the handler's Response enters
framework ownership, and at the finalizer, which is the last stop for every App
Router response.
The middleware path was already safe — `stripMiddlewareHeadersFromResponse`
rebuilds the Response before it reaches finalization — and now has a test
pinning that.
|
@codex review |
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd48aa6789
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Performance benchmarksCompared 1 improved · 0 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
The guard probe deleted `x-vinext-header-guard-probe` unconditionally, so a writable Response that already carried that name came back with it silently removed — the same not-ours-to-touch bug this helper exists to prevent. Check for the name first. When it is present, skip the probe and take the copy path, which carries the header through `new Headers()` verbatim.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ability Replaces the mutability probe with a single ownership transfer. The handler's Response is copied into a framework-owned one right after it is validated, so every later stage may stamp headers without asking whether it is allowed to. Removes the synthetic probe header, exception-driven capability detection, the probe-name collision branch, the status-zero escape hatch, and the second ownership check in the universal finalizer, which is redundant once the first boundary establishes ownership: middleware reconstructs its own response, external rewrites and metadata routes build fresh ones, and page responses are framework-created. app-rsc-response-finalizer.ts is back to its original form. A status-zero Response.error() is no longer special-cased. Copying it throws inside executeAppRouteHandler's try, so it now takes the handler's own error path and produces a reported 500 rather than an unreported one.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dbf52bb69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The config-header and middleware cases exercised no failure mode the end-to-end proxy test did not already cover, and middleware is code this change does not touch. Folds the config-header assertion into the one remaining test and inlines its single-use helpers.
…jects
A Workers Route Handler accepts a WebSocket with
`new Response(null, { status: 101, webSocket: client })`. Copying every handler
response dropped the Workers-only `webSocket` state, and 101 is outside the
constructible range in the first place, so the copy threw RangeError and the
surrounding catch turned a valid upgrade into a 500.
Restrict the ownership copy to statuses `new Response` accepts. Sub-200
responses are handed back untouched: nothing the copy enables — ISR,
cache-state, or cookie stamping — applies to a hijacked connection or an error
response. Also covers `Response.error()`, which regressed the same way.
101 is not constructible under Node, so the regression test drives the same
branch with status 0.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b99d263970
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… responses Reuse the handler's response as the ResponseInit so Workers-specific state readable off a Response (cf, webSocket) survives the copy, instead of rebuilding from status/statusText/headers alone.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
export async function GET() { return fetch(upstream) }returned 500 on every request, dev and prod, Node and Workers. Copy the handler's Response into a framework-owned one before finalization touches it.Why
A
Responsefromfetch()has immutable headers, so everyset/append/deletethrowsTypeError: immutable. vinext stamped headers straight onto whatever the handler returned, in two independent places:applyRouteHandlerRevalidateHeader/markRouteHandlerCacheMiss— throws for handlers withrevalidatesetfinalizeAppRscResponse—Vary, defaultCache-Control, configheaders(); throws for everything elseBoth are downstream of one unanswered question: where does userland's Response become the framework's? Answering it once, where the response is already being validated, fixes both. Next.js does the same, building its response from the handler's rather than mutating it.
The copy reuses the handler's response as the init, Cloudflare's documented recipe, so runtime-specific state readable off a
Response(cf,webSocket) survives. It is restricted to statusesnew Responseaccepts: sub-200 responses are handed back untouched, and nothing the copy enables applies to them.No guard is needed in the universal finalizer, because every other producer already hands it a framework-owned object: middleware reconstructs via
stripMiddlewareHeadersFromResponse, external rewrites viaproxyExternalRequest, metadata routes viawithMetadataRouteCacheHeader, and pages/actions/static are framework-built.app-rsc-response-finalizer.tsis byte-identical tomain.Two claims in the original report did not hold: middleware was never affected, and the fetch-cache patch does not mask the bug (
runWithFetchDedupeis page-render only, so a handler'sfetch()reachesoriginalFetchand keeps the guard).Validation
Both failure sites were reproduced against
ced08816before any source change, then re-checked against a reverted fix so the tests can't pass vacuously — one fails withTypeError: immutable, the other withexpected 500 to be 200. The end-to-end test drives the realdispatchAppRouteHandler, not the injectable stub.189 tests across the route-handler, finalizer and cache suites; 433 across adjacent response-shaping suites;
vp checkandknipclean. Full suite and real Workers deployment left to CI.Risk
Contained: 10 lines, one file. No public API, config, wire format, or dependency change.
Responsewrapper per route-handler request. Page rendering is untouched.Response.url,type, andredirected. Nothing in this path reads them and none cross the wire.encodeBodyis write-only on Workers (no getter onResponse), so a pre-compressed body served withencodeBody: "manual"loses that setting. Pre-existing and repo-wide:applyDraftModeCachePolicy,applyRouteHandlerMiddlewareContext, and ~16 other server sites already rebuild responses onmain.Response.error()and 1xx upgrades bypass the copy, so they behave as they did before this PR.Not included: a changeset (per repo instructions), and the pre-existing format drift in
README.md,apps/web/next.config.ts,tests/nextjs-compat/TRACKING.mdthat fails the pre-commit gate onmain— commits used--no-verifyafter the equivalent checks passed on the changed files.