Rebuild request context after middleware modifies headers#288
Rebuild request context after middleware modifies headers#288southpolesteve merged 2 commits intomainfrom
Conversation
|
/bigbonk review this |
commit: |
|
There was a problem hiding this comment.
Good bug fix — the analysis is correct and the prod-server fix is clean. However, per the AGENTS.md guidelines ("When fixing a bug in any of these files, check whether the same bug exists in the others. Do not leave known bugs as follow-ups — fix them in the same PR"), this same bug exists in two other server implementations and should be fixed here.
The fix itself (prod-server.ts) — looks good
The approach is straightforward: change const to let, rebuild reqCtx after x-middleware-request-* headers are unpacked. The placement is correct — after the header unpacking loop (lines 832-841) and before any config matcher calls. The test and fixture change cover the scenario well.
Same bug exists in deploy.ts (generated Cloudflare Worker entry)
packages/vinext/src/deploy.ts line 547 builds reqCtx as const before middleware runs, and never rebuilds it after x-middleware-request-* headers are unpacked (lines 587-610). The stale reqCtx is then passed to matchHeaders (line 616), matchRedirect (line 624), and matchRewrite downstream.
The fix should mirror what was done in prod-server.ts: change const reqCtx to let reqCtx and add reqCtx = requestContextFromRequest(request); after line 610 (after the middleware header unpacking block).
Same bug in index.ts (dev server) — subtler variant
The dev server (packages/vinext/src/index.ts, lines 2770-2785) builds reqCtx after middleware runs, which is good. But it builds it from req.headers — the original Node.js IncomingMessage headers — without first unpacking x-middleware-request-* headers back into req.headers. So middleware-injected cookies via NextResponse.next({ request: { headers } }) won't be visible to config matchers in dev mode either.
The dev server currently applies middleware response headers directly to res (lines 2754-2758) but doesn't unpack x-middleware-request-* headers into req.headers before building reqCtx. Consider adding the same unpacking step.
Minor observations
- The middleware fixture cookie injection logic (line 44-45) handles the empty-string case correctly with the ternary — nice.
- The test is placed in the right describe block ("Production server next.config.js features") and follows the existing pattern from PR #47.
- The comment on the initial
let reqCtxassignment (lines 760-762) explaining the rebuild is helpful for future readers.
Please fix the deploy.ts and index.ts parity issues in this PR before merging.
|
The review has been posted. Here's a summary: The prod-server.ts fix is correct — rebuilding Requested changes for two parity bugs:
Per the project's AGENTS.md: "When fixing a bug in any of these files, check whether the same bug exists in the others. Do not leave known bugs as follow-ups — fix them in the same PR." |
The production server built the RequestContext for has/missing condition matching before middleware ran. When middleware modified request headers (e.g. injecting cookies via x-middleware-request-cookie), config rules for redirects, rewrites, and custom headers evaluated against the original pre-middleware values instead of the updated ones. Move the reqCtx construction to after middleware header unpacking so that config conditions see the current request state.
Address review feedback: the dev server (index.ts) was not unpacking x-middleware-request-* headers into req.headers before building reqCtx, so middleware-injected cookies were invisible to config has/missing conditions. The Cloudflare Worker entry (deploy.ts) had the same stale reqCtx as prod-server.ts: built once before middleware, never rebuilt. Dev server: unpack x-middleware-request-* headers into req.headers and strip x-middleware-* internal headers from the response. Deploy entry: change const to let, rebuild reqCtx after middleware header unpacking.
ef46d47 to
846648c
Compare
|
Pushed parity fixes for both missing call sites:
All three server implementations (prod-server.ts, deploy.ts, index.ts) now rebuild reqCtx after middleware modifies request headers. /bigbonk review this |
There was a problem hiding this comment.
Review: Parity fixes for deploy.ts and index.ts
Both parity fixes correctly address the reviewer's feedback.
deploy.ts — Correct
The fix at line 617 (reqCtx = requestContextFromRequest(request)) is placed exactly right: after the x-middleware-request-* header unpacking block (lines 588-611) that clones the request with updated headers, and before the config matchers (matchHeaders at line 623, matchRedirect downstream). The const → let change at line 548 enables the reassignment. This mirrors the prod-server.ts pattern exactly.
index.ts — Correct and better than the prod-server approach
This fix is the most interesting of the three. Rather than adding an explicit reqCtx rebuild, it unpacks x-middleware-request-* headers directly into req.headers (the Node.js IncomingMessage) at line 2794. Since reqCtx is already built from req.headers after this block (lines 2812-2825), it naturally picks up middleware-modified values without needing a second requestContextFromRequest() call.
The three-way branch at lines 2792-2797 is clean:
x-middleware-request-*→ unpack intoreq.headers(the actual request header name)x-middleware-*(other internal headers) → strip entirely, don't leak to client- Everything else → apply to response as before
This also fixes an additional bug where the old code was applying all middleware response headers (including internal x-middleware-* ones) to the response via res.appendHeader. Those internal headers should never reach clients.
Parity across all three servers
| Server | reqCtx built after middleware? | x-middleware-request-* unpacked? | x-middleware-* stripped from response? |
|---|---|---|---|
| prod-server.ts | ✓ (explicit rebuild at line 855) | ✓ (lines 840-849) | ✓ (lines 846-848) |
| deploy.ts | ✓ (explicit rebuild at line 617) | ✓ (lines 588-611) | ✓ (lines 595-597) |
| index.ts | ✓ (built from req.headers after unpack) | ✓ (lines 2792-2794) | ✓ (line 2795) |
Test results
Ran the full test suite: 50/52 test files passed, 2178 tests passed. The 2 failures are pre-existing ecosystem.test.ts and static-export.test.ts infrastructure issues (vinext package export resolution), unrelated to this PR.
LGTM — all three server implementations are now consistent.
|
Review posted and PR approved. Here's the summary: All three parity fixes are correct:
All three servers now consistently: (1) unpack |
|
@southpolesteve i was just testing this in the Next.js dev server and I believe it's the opposite behaviour to what they do. The docs confirm this - https://nextjs.org/docs/13/pages/building-your-application/routing/middleware#matching-paths headers and redirects (from config) are evaluated before middleware, which makes sense since they should be based on the incoming request. |
…)" This reverts commit 8f89555.
Summary
RequestContextforhas/missingcondition matching before middleware ran. When middleware modified request headers (e.g. injecting cookies viax-middleware-request-cookie), config rules for redirects, rewrites, and custom headers evaluated against the original pre-middleware values instead of the updated ones.reqCtxconstruction to after middleware header unpacking so that config conditions see the current request state.Changes
packages/vinext/src/server/prod-server.ts: ChangedreqCtxfromconsttoletand added a secondrequestContextFromRequest()call after middleware headers are unpacked into the web request.tests/fixtures/pages-basic/middleware.ts: Added a path (/about?inject-login) that injects alogged-in=1cookie via middleware request headers.tests/pages-router.test.ts: New test verifying that confighas/missingconditions see middleware-injected cookies.