fix: preserve middleware request header deletions#382
Conversation
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Middleware Request Header Deletion Support
Overall this is a well-structured change. The centralization of the encode/decode logic into middleware-request-headers.ts is the right call — having that protocol scattered across 4+ files was a maintenance liability. The x-middleware-override-headers approach matches Next.js's own protocol, and the tests are well-ported from Next.js's test suite.
I have a few concerns, mostly around edge cases in the core codec and a potential regression in the !hasAppDir guard.
Architecture summary (for future readers)
The key insight: previously, the middleware protocol could only add or modify request headers via x-middleware-request-<name>. There was no way to express deletion — if a header existed on the original request, it would persist even if middleware removed it from NextResponse.next({ request: { headers } }).
This PR adds x-middleware-override-headers (a comma-separated allowlist of header names) alongside the existing x-middleware-request-* headers. When present, only headers in the allowlist are included in the rebuilt request — anything not listed is effectively deleted.
The protocol is applied consistently across all 4 server paths: App Router RSC entry, Pages Router dev (index.ts), Pages Router prod (config-matchers.ts via prod-server.ts), and the Pages production codegen entry (pages-server-entry.ts).
| if (value !== undefined) { | ||
| nextHeaders.set(key, value); | ||
| } | ||
| } |
There was a problem hiding this comment.
Edge case: override list names a header that middleware didn't forward.
If x-middleware-override-headers contains a key (e.g. host) but there's no corresponding x-middleware-request-host, line 81 silently skips it. This means the header is deleted even though the override list says to keep it.
In Next.js, encodeMiddlewareRequestHeaders always writes a x-middleware-request-* entry for every key in the override list, so this shouldn't happen in normal usage. But it could trip up callers who construct the protocol manually or if a bug introduces a mismatch.
Consider falling back to the base header when the forwarded value is missing:
| } | |
| const value = forwardedHeaders.get(key) ?? baseHeaders.get(key); | |
| if (value !== undefined && value !== null) { | |
| nextHeaders.set(key, value); | |
| } |
This would be more defensive. Not blocking on this — the current behavior is correct for the happy path.
There was a problem hiding this comment.
Keeping the stricter behavior here.
If the override list and the forwarded x-middleware-request-* set disagree, that means the middleware protocol was serialized inconsistently. Falling back to baseHeaders would silently convert a corrupted protocol into an additive merge, which is exactly the ambiguity this PR is removing.
In normal usage vinext now emits both from the same encoder, so the mismatch should be unrepresentable. If we ever want extra hardening here, I would rather make the mismatch explicit than resurrect the old "missing means unchanged" behavior.
| requestHeaders: Headers, | ||
| ): void { | ||
| const overrideHeaderNames = [...requestHeaders.keys()]; | ||
| targetHeaders.set(MIDDLEWARE_OVERRIDE_HEADERS, overrideHeaderNames.join(",")); |
There was a problem hiding this comment.
Nit: Headers.keys() can return duplicate keys when a header has multiple values (some implementations). The comma-joined x-middleware-override-headers string could contain duplicates. Not a correctness issue since the decode side just iterates the list, but worth noting.
| // request headers here leaks App Router middleware state into | ||
| // the pages-shell config matcher, which can misroute app-only | ||
| // fallback rewrites through the pages handler. | ||
| if (!hasAppDir) { |
There was a problem hiding this comment.
This guard concerns me. The !hasAppDir check means that in a Pages-only project, dev middleware header deletions are applied to req.headers. But the comment says App Router projects skip this because the RSC entry reruns middleware.
What about mixed app/pages projects where the request actually matches a Pages route? In that case:
hasAppDiristrue, so this block is skipped- The RSC entry will re-run middleware and apply headers in its own ALS context
- But
req.headers(the Node IncomingMessage) is not updated with deletions - The Pages SSR handler reads
req.headersdirectly (viagetServerSideProps'sreqobject)
So if you have a mixed project and middleware deletes a header on a route that falls through to the Pages handler, the deletion won't be visible in getServerSideProps.
The new fallback-routing guard at line 2188-2191 might paper over this by routing unmatched pages to the RSC handler, but that's only for fallback rewrites specifically. A direct Pages route hit in a mixed project would still have stale req.headers.
Is there a test covering this scenario? If it's intentionally deferred, a TODO comment would help.
There was a problem hiding this comment.
Fixed in c9db67f.
The host dev shell now keeps the rebuilt middleware request headers separate from the shared Node req.headers, uses that rebuilt set for config matching, and only materializes it onto req.headers once a Pages API/page handler actually owns the request. That preserves mixed app/pages fallback behavior while making direct Pages getServerSideProps hits in mixed projects see header deletions as expected.
Covered by the new mixed-router test in tests/app-router.test.ts for /pages-header-override-delete.
| const fallbackMatch = matchRoute(fallbackRewrite.split("?")[0], routes); | ||
| if (!fallbackMatch && hasAppDir) { | ||
| return next(); | ||
| } |
There was a problem hiding this comment.
This fallback routing change feels tangentially related to the header deletion fix. The PR description mentions "mixed app/pages fallback rewrites do not misroute through the pages shell", but this introduces new routing behavior beyond header handling.
Could this cause a regression where a valid Pages route reachable only via fallback rewrite now 404s in a mixed project? matchRoute checks the Pages router's route table — if the fallback rewrite targets an App Router route, it correctly falls through to next(). But if it targets a dynamically-added Pages route not yet in the routes array (e.g., during HMR), it would incorrectly fall through too.
This deserves its own test case (a mixed app/pages project with a fallback rewrite targeting a pages route).
There was a problem hiding this comment.
Fixed in c9db67f.
I added a mixed app/pages fallback regression test that rewrites /mw-gated-fallback-pages to a Pages route after middleware injects the gating cookie. That pins the intended behavior: app-only fallback rewrites fall through to App Router, but fallback rewrites targeting a real Pages route still dispatch through the Pages handler.
| } | ||
|
|
||
| // If middleware modified the cookie header, rebuild the cookies map. | ||
| ctx.cookies.clear(); |
There was a problem hiding this comment.
Good change — always clearing cookies before rebuilding. Previously, if middleware deleted the cookie header entirely, the old cookies would persist in the map because the rebuild only triggered when newCookieHeader !== null. Now ctx.cookies.clear() runs unconditionally, which is correct.
| var rwHeaders = new Headers(); | ||
| for (var [k, v] of response.headers) { | ||
| if (!k.startsWith("x-middleware-") || k.startsWith("x-middleware-request-")) rwHeaders.append(k, v); | ||
| if (!k.startsWith("x-middleware-") || k === "x-middleware-override-headers" || k.startsWith("x-middleware-request-")) rwHeaders.append(k, v); |
There was a problem hiding this comment.
This line is getting quite long and hard to scan. Consider extracting the filter condition into a small helper function in the codegen template, similar to shouldKeepMiddlewareHeader in the runtime code. Not blocking.
| return nextHeaders; | ||
| } | ||
|
|
||
| export function shouldKeepMiddlewareHeader(key: string): boolean { |
There was a problem hiding this comment.
Nit: this function is used in middleware.ts but is also a logical fit for the codegen in pages-server-entry.ts. Right now the codegen inlines the same logic (key === "x-middleware-override-headers" || key.startsWith("x-middleware-request-")). If this module were importable from the production bundle, you could DRY up the codegen too. Not blocking for this PR, but worth considering for future cleanup.
| }); | ||
|
|
||
| describe("middleware request header overrides", () => { | ||
| // Ported from Next.js: test/e2e/middleware-request-header-overrides/test/index.test.ts |
There was a problem hiding this comment.
Good test coverage. The unit tests for both config-matchers and next/headers paths are thorough. The integration tests in app-router.test.ts and pages-router.test.ts cover the full middleware-to-render pipeline.
One gap: there's no test for the case where middleware adds headers but does not delete any (i.e., the backwards-compatible case where x-middleware-override-headers contains all original headers plus new ones). The existing tests may cover this implicitly via the general middleware header-forwarding tests, but an explicit assertion that the new encodeMiddlewareRequestHeaders path doesn't regress the "add-only" case would be valuable.
There was a problem hiding this comment.
Added in c9db67f.
There is now an explicit add-only codec test in tests/shims.test.ts that exercises x-middleware-override-headers with the full original header set plus a new header. It asserts that existing headers survive unchanged and the added header is present, so the new encoding path is pinned against regressions in the non-deletion case.
|
Review submitted. Here's a summary of the key points: Good:
Key concerns raised:
|
NathanDrake2406
force-pushed
the
fix/middleware-request-header-deletions
branch
from
f859bbb to
c9db67f
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes middleware request header override handling so downstream routing/rendering can observe header deletions (not just additions/updates), by serializing a complete override set via x-middleware-override-headers and using a shared encode/decode helper across the stack.
Changes:
- Introduces a shared middleware request-header override codec (
encodeMiddlewareRequestHeaders,buildRequestHeadersFromMiddlewareResponse,shouldKeepMiddlewareHeader). - Updates shims, middleware runners, config matchers, and dev server routing to rebuild the effective request headers (including deletions) from middleware response headers.
- Adds/extends integration tests and fixtures for App Router, Pages Router, and mixed app/pages fallback rewrite behavior.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/shims.test.ts | Adds unit tests for override header serialization and downstream rebuild behavior. |
| tests/pages-router.test.ts | Adds dev + prod integration coverage for deleting credential headers before Pages handling. |
| tests/fixtures/pages-basic/pages/header-override-delete.tsx | New Pages route fixture that prints observed request headers. |
| tests/fixtures/pages-basic/middleware.ts | Adds middleware path that deletes authorization/cookie and injects a marker header. |
| tests/fixtures/app-basic/pages/pages-header-override-delete.tsx | New Pages route in mixed app/pages fixture for deletion verification. |
| tests/fixtures/app-basic/next.config.ts | Adds a fallback rewrite rule gated on a middleware-injected cookie targeting a Pages route. |
| tests/fixtures/app-basic/middleware.ts | Adds middleware paths for header deletion and for injecting cookie used by mixed fallback rewrite test. |
| tests/fixtures/app-basic/app/header-override-delete/page.tsx | New App Router page validating headers/cookies after middleware deletions. |
| tests/app-router.test.ts | Adds App Router integration tests for deletions + mixed app/pages fallback rewrite behavior. |
| tests/snapshots/entry-templates.test.ts.snap | Updates generated entry templates to preserve x-middleware-override-headers through middleware handling. |
| packages/vinext/src/shims/server.ts | Uses shared encoder for NextResponse.next() and NextResponse.rewrite() request header overrides. |
| packages/vinext/src/shims/headers.ts | Rebuilds live headers from middleware response using shared decoder. |
| packages/vinext/src/server/middleware.ts | Uses shouldKeepMiddlewareHeader to retain override metadata while stripping other x-middleware-* internals. |
| packages/vinext/src/server/middleware-request-headers.ts | New shared codec for encoding/decoding middleware request header overrides (including deletions). |
| packages/vinext/src/index.ts | Rebuilds downstream Node request headers from middleware override codec; adjusts fallback rewrite dispatch logic for mixed projects. |
| packages/vinext/src/entries/pages-server-entry.ts | Keeps x-middleware-override-headers in production middleware runner output for downstream decoding. |
| packages/vinext/src/config/config-matchers.ts | Rebuilds Request headers from middleware response overrides for has/missing evaluation and downstream routing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If middleware modified the cookie header, rebuild the cookies map. | ||
| ctx.cookies.clear(); | ||
| const newCookieHeader = ctx.headers.get("cookie"); | ||
| if (newCookieHeader !== null) { | ||
| ctx.cookies.clear(); | ||
| for (const part of newCookieHeader.split(";")) { |
There was a problem hiding this comment.
applyMiddlewareRequestHeaders() now clears ctx.cookies unconditionally. Because HeadersContext.cookies is lazily materialized (via a getter in headersContextFromRequest()), calling ctx.cookies.clear() here forces cookie parsing even when middleware did not touch request headers and even when there is no cookie header. Consider only rebuilding the cookies map when nextHeaders is non-null and the cookie header changed, or update the logic to avoid materializing cookies unless needed (to preserve the laziness described below in the same file).
There was a problem hiding this comment.
Adjusted in 1c24e7c.
applyMiddlewareRequestHeaders() now returns early when no middleware request-header override was decoded, and only rebuilds the cookie map when the cookie header value actually changed. That preserves the lazy cookie-materialization behavior from headersContextFromRequest() for add-only/non-cookie middleware updates while still correctly clearing stale cookies when middleware deletes or rewrites the cookie header.
| static rewrite(destination: string | URL, init?: MiddlewareResponseInit): NextResponse { | ||
| const url = typeof destination === "string" ? destination : destination.toString(); | ||
| const headers = new Headers(init?.headers); | ||
| headers.set("x-middleware-rewrite", url); | ||
| if (init?.request?.headers) { | ||
| encodeMiddlewareRequestHeaders(headers, init.request.headers); | ||
| } | ||
| return new NextResponse(null, { ...init, headers }); |
There was a problem hiding this comment.
NextResponse.rewrite() now encodes request header overrides via encodeMiddlewareRequestHeaders(), but there isn't a test covering this new behavior (only next() is exercised for x-middleware-override-headers). Please add a shim-level test that calls NextResponse.rewrite(..., { request: { headers } }) and asserts x-middleware-override-headers and the expected x-middleware-request-* headers are produced (and ideally a downstream rebuild path consumes them).
There was a problem hiding this comment.
Added in 1c24e7c.
There is now a shim-level test for NextResponse.rewrite(..., { request: { headers } }) that asserts x-middleware-override-headers plus the expected forwarded x-middleware-request-* headers are emitted.
| const nextHeaders = buildRequestHeadersFromMiddlewareResponse( | ||
| ctx.headers, | ||
| middlewareResponseHeaders, | ||
| ); |
There was a problem hiding this comment.
buildRequestHeadersFromMiddlewareResponse() can replace the entire live header set when x-middleware-override-headers is present (enabling downstream deletions). The JSDoc above this function still says it only "replaces the corresponding entries". Please update the comment to reflect the new semantics (full rebuild vs additive patch), so future maintainers don't misinterpret why omitted headers disappear.
There was a problem hiding this comment.
Updated in 1c24e7c.
The JSDoc now describes the actual behavior: middleware request-header overrides are decoded into the resulting request header set, and when an override list is present omitted headers are deleted as part of the rebuild.
|
/bigbonk re-review |
There was a problem hiding this comment.
Review: Middleware Request Header Deletion Support
This is a well-executed change that fills a real gap in the middleware protocol. The centralized codec in middleware-request-headers.ts is clean, the x-middleware-override-headers protocol matches Next.js semantics, and the change is applied consistently across all four server paths (App Router RSC entry, Pages Router dev, Pages Router prod, Pages codegen). The test coverage is thorough — unit tests for both codec paths, integration tests across App Router and Pages Router, plus the mixed app/pages fallback regression test.
The author has addressed all prior review feedback well: the lazy cookie materialization is preserved, the JSDoc is updated, the NextResponse.rewrite() test was added, and the add-only backwards-compatibility test was included.
I have a few remaining observations, none blocking.
Architecture note for future readers
The deferred applyRequestHeadersToNodeRequest pattern in index.ts is worth understanding: in mixed app/pages projects (hasAppDir === true), middleware header overrides are not immediately materialized onto req.headers. Instead, middlewareRequestHeaders is kept as a separate Headers object, used directly for config condition matching (via reqCtxHeaders), and only written to req.headers at the point where a Pages route actually handles the request. This avoids polluting the shared Node request for the App Router path (which re-runs middleware in its own RSC entry).
| requestHeaders: Headers, | ||
| ): void { | ||
| const overrideHeaderNames = [...requestHeaders.keys()]; | ||
| targetHeaders.set(MIDDLEWARE_OVERRIDE_HEADERS, overrideHeaderNames.join(",")); |
There was a problem hiding this comment.
Nit: Headers.keys() returns lowercased keys per the spec, so duplicates are only possible if the same header name was .append()-ed multiple times (in which case .keys() yields the key once per combined value in most implementations, but the spec doesn't guarantee deduplication). In practice this is fine because the decode side iterates the list and forwardedHeaders.get() handles combined values. Just noting for posterity that if a weird runtime yields duplicates, the comma-joined string would contain them, but it wouldn't cause incorrect behavior — just a slightly longer header value.
| } | ||
|
|
||
| for (const key of overrideHeaderNames) { | ||
| const value = forwardedHeaders.get(key); |
There was a problem hiding this comment.
The strict behavior here (silently dropping a header when the override list names it but no x-middleware-request-* entry exists) is the right call, as the author explained. One minor observation: forwardedHeaders is a Map<string, string>, and Map.get() returns undefined when the key is missing. The check value !== undefined is correct. But if someone ever changes this to use Headers instead of Map, Headers.get() returns null for missing keys, so this check would need updating. A comment like // Map.get() returns undefined for missing keys would prevent a future subtle bug.
| result.responseHeaders, | ||
| ); | ||
|
|
||
| if (middlewareRequestHeaders && !hasAppDir) { |
There was a problem hiding this comment.
Good fix for the mixed project case. The !hasAppDir guard ensures the shared req.headers object is only mutated immediately for Pages-only projects, while mixed projects defer materialization until a Pages route actually owns the request. This preserves the App Router's ability to re-run middleware independently in its RSC entry.
One edge case to be aware of: if custom connect middleware added after vinext's handler reads req.headers, it will see the original (pre-middleware) headers in mixed projects until a Pages route match triggers applyRequestHeadersToNodeRequest. This seems fine since vinext's handler is typically the last in the chain, but worth noting.
| const fallbackMatch = matchRoute(fallbackRewrite.split("?")[0], routes); | ||
| if (!fallbackMatch && hasAppDir) { | ||
| return next(); | ||
| } |
There was a problem hiding this comment.
This is a meaningful behavioral change: previously, all fallback rewrites in mixed projects fell through to next() (the App Router RSC handler). Now, fallback rewrites that match a Pages route are dispatched to the Pages handler directly.
This is correct — a fallback rewrite targeting a known Pages route should be handled by the Pages handler, not sent to the RSC entry which would 404 on it. The new test at line 1988 in app-router.test.ts pins this. The only scenario where this could regress is if a Pages route is dynamically added during HMR and routes hasn't been rescanned yet, but that's a pre-existing limitation of the matchRoute approach.
| var rwHeaders = new Headers(); | ||
| for (var [k, v] of response.headers) { | ||
| if (!k.startsWith("x-middleware-") || k.startsWith("x-middleware-request-")) rwHeaders.append(k, v); | ||
| if (!k.startsWith("x-middleware-") || k === "x-middleware-override-headers" || k.startsWith("x-middleware-request-")) rwHeaders.append(k, v); |
There was a problem hiding this comment.
This line is getting quite dense. Consider extracting the filter into a named helper in the codegen template (similar to how the runtime uses shouldKeepMiddlewareHeader). Something like:
| if (!k.startsWith("x-middleware-") || k === "x-middleware-override-headers" || k.startsWith("x-middleware-request-")) rwHeaders.append(k, v); | |
| if (!k.startsWith("x-middleware-") || k === "x-middleware-override-headers" || k.startsWith("x-middleware-request-")) rwHeaders.append(k, v); |
Actually, the current form is fine for codegen since it avoids an extra function definition in the generated output. If this condition appears in more places in the codegen template in the future, it would be worth DRYing up via shouldKeepMiddlewareHeader imported at runtime (as the prior reviewer suggested). Not blocking.
| } | ||
| if (!nextHeaders) return; | ||
|
|
||
| ctx.headers = nextHeaders; |
There was a problem hiding this comment.
Direct reassignment of ctx.headers works because HeadersContext.headers is typed as Headers (not readonly). Worth noting that this bypasses the lazy mutable proxy created in headersContextFromRequest — after this line, ctx.headers is a plain Headers instance, not a Proxy. This is correct behavior (middleware has rewritten the full header set, so laziness is no longer useful), but downstream code that holds a reference to the old proxy will be stale. Since applyMiddlewareRequestHeaders runs before any route handler reads headers, this is safe.
|
Review submitted successfully. Here's a summary of my review: Verdict: Approved The PR cleanly centralizes the middleware request header encode/decode protocol into Key observations (none blocking):
|
Summary
x-middleware-override-headersTests