fix(middleware): stop restoring credentials middleware deleted before external rewrites

[NathanDrake2406]

The decision boundary

x-middleware-override-headers is the complete post-middleware header set, not a diff.

• NextResponse.next/rewrite encode every key of the Headers object middleware passes (response.ts — handleMiddlewareField).

• Next.js then deletes any request header not in that list (resolve-routes.ts):

  // Delete headers.
  for (const key of Object.keys(req.headers)) {
    if (!overriddenHeaders.has(key)) {
      delete req.headers[key]
    }
  }

There is no tombstone for a deleted header because none is needed. Absence is the tombstone.

The bug

preserveCredentialHeaders (#1121) read a short override list as a partial override and copied the base request's cookie/authorization back in. But the documented deletion pattern produces exactly such a list:

const headers = new Headers(request.headers)
headers.delete('cookie')
return NextResponse.rewrite(externalUrl, { request: { headers } })

cookie is simply absent from the override list — so the option resurrected it. The option was enabled only for external rewrites, so proxyExternalRequest then forwarded first-party session cookies and bearer tokens to a cross-origin target, despite the app explicitly stripping them.

Same-origin restoration would have been merely a parity bug; gating it to cross-origin destinations turned it into a credential leak.

Why removal, not a narrower guard

The "partial override" the option guarded against cannot occur. encodeMiddlewareRequestHeaders is the only producer of the override list in the codebase, and it always emits the full key set:

const overrideHeaderNames = [...requestHeaders.keys()];
targetHeaders.set(MIDDLEWARE_OVERRIDE_HEADERS, overrideHeaderNames.join(","));

#1121's own "explicit deletion" test passed a list naming authorization,cookie with no corresponding x-middleware-request-* values — an encoding the public API never emits — so it never exercised the real deletion path. Deleting the option restores Next.js-exact semantics and removes the parameter from three call sites.

Scenario-level behavior

Middleware does	Override list	Before	After
NextResponse.next() (no request)	not sent	credentials forwarded	credentials forwarded (unchanged)
clones headers, adds one	full set incl. cookie	credentials forwarded	credentials forwarded (unchanged)
clones headers, deletes cookie	full set minus cookie	🔴 cookie restored and sent cross-origin	✅ cookie dropped

Only the third row changes.

Validation

Both regression tests were confirmed failing before the fix, passing after:

• tests/app-router-external-rewrite.test.ts — end to end through the dev server; fixture middleware deletes cookie/authorization before an external rewrite, mock upstream asserts neither arrives. Pre-fix: expected 'session=secret123' to be undefined.
• tests/shims.test.ts — the proxyExternalMiddlewareRewrite boundary, driven through the real NextResponse.rewrite API against a live node:http upstream. Pre-fix: expected 'Bearer secret' to be undefined.
• tests/pages-request-pipeline.test.ts — Pages Router pipeline, both the no-override-list (credentials forwarded) and deleted-from-override-list (dropped) cases.
• tests/request-pipeline.test.ts — unit coverage for both override-list states, replacing the two fix(middleware): preserve credentials for external override rewrites #1121 tests that encoded the buggy expectation.

pnpm run check passes (format, lint, types, Next.js type sync, shim types).

Risk / review path

Behavior change is confined to the one row above — restoring a header the app deleted. Any app relying on the old behavior was relying on a credential leak.

Suggested review order: middleware-request-headers.ts (the semantics), then the three call sites (mechanical parameter removal), then the tests.

[NathanDrake2406]

@codex review

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2739

npm i https://pkg.pr.new/create-vinext-app@2739

npm i https://pkg.pr.new/@vinext/types@2739

npm i https://pkg.pr.new/vinext@2739

commit: ebc8fe2

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: 3649e02fb9

ℹ️ 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".

[github-actions]

Performance benchmarks

Compared ebc8fe2 against base 25dc2f3 using alternating same-runner rounds. Next.js was unchanged and skipped.

1 improved · 0 regressed · 5 within ±1.5%

Scenario	Framework	Baseline	Current	Change
Client bundle size (gzip)	vinext	134.3 KB	134.3 KB	⚫ -0.0%
Client entry size (gzip)	vinext	121.9 KB	121.9 KB	⚫ +0.0%
Dev server cold start	vinext	2.91 s	2.85 s	🟢 -2.0%
Production build time	vinext	3.15 s	3.16 s	⚫ +0.3%
RSC entry closure size (gzip)	vinext	111.2 KB	111.3 KB	⚫ +0.0%
Server bundle size (gzip)	vinext	188.9 KB	188.9 KB	⚫ -0.0%

View detailed results and traces

_{🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head}

[james-elicx]

/bigbonk review for issues

[ask-bonk]

UnknownError: ProviderInitError

github run

[ask-bonk]

@james-elicx Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

[james-elicx]

/bigbonk review for issues

[ask-bonk]

UnknownError: ProviderInitError

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: LGTM — correct and well-justified

Verified the core claim against the Next.js source. resolve-routes.ts (getResolveRoutes) does exactly what the PR describes:

if (middlewareHeaders['x-middleware-override-headers']) {
  // Delete headers.
  for (const key of Object.keys(req.headers)) {
    if (!overriddenHeaders.has(key)) {
      delete req.headers[key]
    }
  }
  // Update or add headers.
  ...
}

The override list is the complete post-middleware header set, and absence = deletion. Restoring cookie/authorization from the base request contradicted this and, because it was gated to external rewrites only, leaked first-party credentials cross-origin. Removing preserveCredentialHeaders restores Next.js-exact semantics.

Verification performed

• encodeMiddlewareRequestHeaders is the only producer of x-middleware-override-headers (confirmed via grep), and it always emits the full key set ([...requestHeaders.keys()]). It is only invoked when init.request.headers is present (shims/server.ts:317, :331). So the "partial override list" #1121 guarded against genuinely cannot occur through the public API — deleting the option is the right call, not a narrower guard.
• The null override path is preserved (middleware-request-headers.ts:84-95): NextResponse.next()/rewrite() without a request field emits no override header, so base headers pass through unchanged. This matches Next.js (the deletion block is entirely skipped when the header is absent).
• Other consumers were already on default behavior. app-route-handler-runtime.ts:310, shims/headers.ts:646, and app-rsc-entry.ts:1270 never passed preserveCredentialHeaders, so unifying on the default introduces no divergence for them.
• No unused imports. isExternalUrl is still used in 8 other places in pages-request-pipeline.ts.
• Tests pass and are meaningful. Ran all four affected suites locally — the new tests drive real NextResponse.rewrite/dev-server paths rather than hand-encoding override lists, which is what let the #1121 bug slip through:
  • tests/request-pipeline.test.ts -t buildRequestHeadersFromMiddlewareResponse — 2 passed
  • tests/pages-request-pipeline.test.ts -t "external rewrite credential headers" — 2 passed
  • tests/shims.test.ts -t "does not forward credentials the middleware deleted" — 1 passed
  • tests/app-router-external-rewrite.test.ts -t "does not send credentials" — 1 passed
• vp check clean on all four changed source files (format, lint, types).

Notes (non-blocking)

• The two new coverage cases correctly capture both override-list states (no-list = forward, absent-from-list = drop), which is exactly the boundary #1121 got wrong. Good.
• Fixture change reuses the existing /middleware-external-rewrite case cleanly; Headers lowercasing means delete("cookie")/delete("authorization") correctly strip the incoming Cookie/Authorization.

No issues found. Approve.

[ask-bonk]

Review posted successfully. Summary of my findings:

Review outcome: Approved — no issues found

This is a correct, well-reasoned security fix. I verified the central claim against Next.js source and confirmed the implementation.

What the PR does: Removes the preserveCredentialHeaders option (added in #1121) which resurrected cookie/authorization headers that middleware had explicitly deleted before an external rewrite — leaking first-party credentials cross-origin.

Verification I performed:

• Fetched Next.js resolve-routes.ts and confirmed the override list is the complete post-middleware header set (absence = deletion), exactly as the PR argues.
• Confirmed encodeMiddlewareRequestHeaders is the only producer of the override header, and it always emits the full key set — so the "partial override" the option guarded against cannot occur. Removal (not a narrower guard) is correct.
• Verified the null-override path (NextResponse.next() without request) still passes base headers through unchanged, matching Next.js.
• Confirmed the three unmodified consumers (app-route-handler-runtime.ts, shims/headers.ts, app-rsc-entry.ts) never used the option, so no divergence is introduced.
• Confirmed no unused imports (isExternalUrl still used 8× in the pages pipeline).
• Ran all four affected test suites locally — all pass. The new tests drive real NextResponse.rewrite/dev-server paths instead of hand-encoding override lists, which is exactly why the fix(middleware): preserve credentials for external override rewrites #1121 bug slipped through before.
• vp check clean on all changed source files.

The review was submitted as an APPROVE with detailed reasoning inline on PR #2739.

github run