fix(middleware): preserve headers for empty override value - #2767
Conversation
|
commit: |
|
/bigbonk review for issues |
Performance benchmarksCompared 1 improved · 0 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(middleware): preserve headers for empty override value
Verdict: LGTM. The fix is correct and matches Next.js behavior. Verified against Next.js source and confirmed all call sites handle the change safely.
Correctness confirmed
The core change — treating an empty x-middleware-override-headers value as "no override" (return null) — matches Next.js exactly. In resolve-routes.ts, the entire request-header mutation block is guarded by a truthiness check:
if (middlewareHeaders['x-middleware-override-headers']) { // "" is falsy → skippedSo NextResponse.next({ request: { headers: new Headers() } }) (which serializes to x-middleware-override-headers: "") leaves the incoming request unchanged. Before this PR, getOverrideHeaderNames returned [] for "", which drove buildRequestHeadersFromMiddlewareResponse down the "complete override set" path and produced an empty Headers — dropping every request header (auth, cookies, etc.). This was a real bug; the fix is right.
Call-site safety verified
All five consumers of buildRequestHeadersFromMiddlewareResponse gate on a falsy/null return and leave the request untouched, so returning null (rather than empty Headers) is safe everywhere:
shims/headers.ts:651-653—if (!nextHeaders && ...) return;thenif (nextHeaders)config/config-matchers.ts:460—if (nextHeaders) { ... }server/app-middleware.ts:125—if (!nextHeaders) return request;server/app-route-handler-runtime.ts:310— already handled the pre-existingnullcase
Tests
Good coverage: a focused helper unit test (red before the change) plus a real external-rewrite proxy integration test asserting authorization/cookie/x-keep survive. Both pass locally, along with tests/app-router-external-rewrite.test.ts and tests/pages-request-pipeline.test.ts. vp check is clean.
Non-blocking note: one theoretical divergence
There is a narrow edge case where vinext and Next.js still differ: an empty override value ("") combined with x-middleware-request-* forwarded headers present. In that case vinext takes the overrideHeaderNames === null && forwardedHeaders.size === 0 guard as false (line 89), clones the base headers, and applies the forwarded headers on top — whereas Next.js skips the whole mutation block and applies nothing.
This is not reachable through vinext's own producer: encodeMiddlewareRequestHeaders derives both the override list and the x-middleware-request-* entries from the same requestHeaders, so an empty override implies zero forwarded headers. It's only constructible via manually/externally crafted headers, so it's low risk and I would not block on it. If you want strict parity for external inputs, getOverrideHeaderNames returning a distinct "empty = no override" sentinel could let buildRequestHeadersFromMiddlewareResponse short-circuit to null even when stray x-middleware-request-* headers exist. Optional — a comment noting the assumption would also suffice.
|
Review posted to PR #2767. SummaryVerdict: LGTM — the fix is correct and matches Next.js behavior. What the PR doesTreats an empty What I verified
One non-blocking noteI flagged a narrow theoretical divergence: empty override |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(middleware): preserve headers for empty override value
Verdict: LGTM. The fix is correct, matches Next.js exactly, and the latest two commits go further than the earlier /bigbonk pass saw — they close the one theoretical divergence that review flagged as non-blocking. Verified against Next.js source, all five call sites, and dev/prod/Pages/App parity.
Correctness confirmed against Next.js
The whole request-header mutation block in resolve-routes.ts is gated on a truthiness check:
if (middlewareHeaders['x-middleware-override-headers']) { // "" and missing are both falsy → skippedThe PR's final form guards identically with if (!rawOverrideHeader) return null;, so:
- Empty override (
"") — emitted byNextResponse.next({ request: { headers: new Headers() } })— returnsnull, leaving the request unchanged. This is the reported bug: previouslygetOverrideHeaderNames("")returned[], drove the "complete override set" path, and produced an emptyHeadersthat dropped every request header (auth, cookies). Fixed. - Missing override — also returns
null. Note this is a subtle behavior change from the previous code, which cloned base headers and applied forwardedx-middleware-request-*on top. Next.js skips the block entirely for a missing header, so the old vinext path actually diverged; the new code now matches Next.js. Safe in practice because vinext's ownencodeMiddlewareRequestHeadersnever emitsx-middleware-request-*without a corresponding non-empty override entry.
The earlier non-blocking note is now resolved
The previous review flagged "empty override "" plus stray forwarded headers" as a remaining divergence (vinext would clone base + apply forwarded; Next.js skips). Commits 4330cc2 and 6e860c2 collapse the guard to a single !rawOverrideHeader check, so stray x-middleware-request-* values are now ignored for both empty and missing overrides — strict parity even for externally crafted headers. The new request-pipeline.test.ts cases assert exactly this.
Call-site safety verified (all five)
Every consumer treats null as "leave the request unchanged", so returning null instead of empty Headers is safe everywhere:
shims/headers.ts:651-665—if (!nextHeaders && ...) return;then guardedif (nextHeaders)config/config-matchers.ts:460—if (nextHeaders) { rebuild request }, else keeps originalserver/app-middleware.ts:125—if (!nextHeaders) return request;server/app-route-handler-runtime.ts:312-314—requestHeaders ? rebuild : input
Parity across servers
Pages Router (pages-request-pipeline.ts:472) delegates to the same applyMiddlewareRequestHeaders → buildRequestHeadersFromMiddlewareResponse, and App Router / route handlers / the headers shim all call the shared helper. There is no divergent duplicate implementation in prod-server, dev-server, or the worker entry, so this single fix covers every runtime.
Tests & checks
tests/request-pipeline.test.ts— unit coverage for empty-override, empty+stray, and missing+stray, all returningnull.tests/shims.test.ts— real external-rewrite proxy integration test assertingauthorization/cookie/x-keepsurvive an empty override; plus existing cookie-mutation tests correctly updated to include the now-requiredx-middleware-override-headersvalue (previously those relied on the missing-override clone path, so the update is a genuine correctness fix, not just churn).- Locally: helper unit tests, shims integration test,
tests/app-router-external-rewrite.test.ts+tests/pages-request-pipeline.test.ts(94 passed), andvp checkon the source all green.
Minor / optional (non-blocking)
- The renamed
_baseHeadersparameter is now entirely unused in the function body (the "clone base" path is gone). Keeping the signature stable for the five callers is fine, but a one-line JSDoc@paramnote that it's intentionally retained-for-signature-parity would save the next reader a double-take. Purely cosmetic.
Nice work — the extra two commits took this from "correct for the reported case" to "strict Next.js parity, including hostile inputs."
|
Review posted to PR #2767. SummaryVerdict: LGTM — the fix is correct and achieves strict Next.js parity. What the PR doesTreats an empty What I verified
One cosmetic note (non-blocking)The renamed |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(middleware): preserve headers for empty override value
Verdict: LGTM. The fix is correct, achieves strict Next.js parity, and is well-tested across every runtime. I verified the change against the current Next.js resolve-routes.ts source, all call sites, the producer invariant, and dev/prod parity.
Correctness confirmed against Next.js source
Fetched resolve-routes.ts from canary. The entire request-header mutation block is gated on a truthiness check:
if (middlewareHeaders['x-middleware-override-headers']) { // "" and missing → skipped
// delete req.headers not in the override set
// then apply x-middleware-request-<key> for each key in the set
}The PR's final guard, if (!rawOverrideHeader) return null;, matches this exactly:
- Empty override (
"") — emitted byNextResponse.next({ request: { headers: new Headers() } })— returnsnull, leaving the request unchanged. This is the reported bug: previouslygetOverrideHeaderNames("")returned[], drove the "complete override set" path, and produced an emptyHeadersthat dropped every incoming header (auth, cookies). Fixed. - Missing override — also returns
null. Note this is a genuine behavior change: the old code cloned base headers and applied forwardedx-middleware-request-*on top. Next.js skips the block entirely for a missing header, so the old vinext path actually diverged. The new code matches Next.js.
I also confirmed the per-key semantics match: for an override key with no corresponding x-middleware-request-<key> value, both Next.js (req.headers[key] = undefined) and vinext (if (value !== undefined) skip) leave the header absent.
The producer invariant makes the removed branch safe
encodeMiddlewareRequestHeaders (shims/server.ts:318,331) is the only producer of x-middleware-request-*, and it always sets x-middleware-override-headers to the joined list of every key in requestHeaders. So empty override ⇒ zero forwarded headers, and non-empty override ⇒ forwarded headers present. The "missing/empty override + stray forwarded headers" combination is not producible by real middleware — only by externally crafted headers — and the PR now handles even that case with strict parity. The removed cloneHeaders branch was effectively dead for real requests.
Call-site safety verified (all five)
Every consumer treats null as "leave the request unchanged," so returning null instead of empty Headers is safe:
shims/headers.ts:651—if (!nextHeaders && ...) return;then guardedif (nextHeaders)config/config-matchers.ts:460—if (nextHeaders) { rebuild }, else keep originalserver/app-middleware.ts:125—if (!nextHeaders) return request;server/app-route-handler-runtime.ts:312—requestHeaders ? rebuild : inputentries/app-rsc-entry.ts:1270— passes the same helper through codegen
Cookie tests correctly updated (not churn)
The three applyMiddlewareRequestHeaders cookie tests in shims.test.ts now add x-middleware-override-headers: "cookie". This is a correctness fix, not churn: they previously relied on the missing-override clone path that couldn't occur from real middleware. Separately, I confirmed the middleware-set cookies are visible test (line 4189) works via the independent x-middleware-set-cookie / mergeMiddlewareSetCookies path, so it is unaffected by the override change.
Parity across servers
Pages Router prod (pages-request-pipeline.ts:472 → applyMiddlewareRequestHeaders → shared helper), App Router, route handlers, and the headers shim all delegate to the single buildRequestHeadersFromMiddlewareResponse. rg found no divergent duplicate implementation in prod-server, dev-server, or the worker entry, so this one fix covers every runtime.
Tests & checks (run locally)
tests/request-pipeline.test.tsbuildRequestHeadersFromMiddlewareResponse— 3 passed (empty override, empty+stray, missing+stray all returnnull).tests/shims.test.ts"empty override"— 1 passed (external-rewrite proxy integration assertingauthorization/cookie/x-keepsurvive).tests/app-router-external-rewrite.test.ts— 6 passed, including the new "stray forwarded header without override list" end-to-end case that forgesx-middleware-request-x-addedin the fixture and confirms it is ignored.vp check packages/vinext/src/utils/middleware-request-headers.ts— clean.
Minor / optional (non-blocking)
_baseHeadersis now entirely unused in the body (the clone path is gone). Keeping the signature stable for the five callers + the codegen entry is the right call; a one-line@paramnote that it's retained-for-signature-parity would save the next reader a double-take. Purely cosmetic.
Nice work — the fix correctly identifies that the old missing-override clone path was itself a Next.js divergence, and the added integration test covering hostile inputs is a good touch.
|
Review posted to PR #2767. SummaryVerdict: LGTM — the fix is correct and achieves strict Next.js parity across all runtimes. What the PR doesRequires a truthy What I verified independently
One non-blocking note
|
|
/bigbonk review for issues |
1 participant
Summary
x-middleware-override-headersvalue before translating middleware request-header overridesx-middleware-request-*values under their literal names on downstream requests and responses; skip empty valuesNext.js parity
Next.js handles these headers in two stages. The override translation block runs only when
x-middleware-override-headersis truthy. Its later generic middleware-header merge copies any truthy values that were not consumed by that list under their literal names.This was confirmed both from source and with a black-box Next.js 16.2.7 App Route repro, which observed the original logical header alongside the literal unconsumed protocol header.
Validation
vp checkpasses for every changed source and test file