fix: rebuild reqCtx for rewrites after middleware#303
Conversation
afterFiles and fallback rewrites run after middleware in the App Router execution order. Their has/missing conditions should evaluate against middleware-modified headers, not the original pre-middleware request. Introduces postMwReqCtx (prod-server, deploy.ts) and __postMwReqCtx (app-dev-server) built after x-middleware-request-* headers are unpacked. headers, redirects, and beforeFiles rewrites keep using the original pre-middleware reqCtx, matching Next.js execution order. Adds getHeadersContext() to the headers shim so app-dev-server can read the ALS-mutated headers context for the post-middleware snapshot. Test: afterFiles rewrite with has:[cookie:mw-user] is skipped without middleware injection and matches after middleware injects the cookie.
commit: |
|
In App Router execution order, beforeFiles rewrites run after middleware (step 4 vs step 3). deploy.ts processes them after middleware runs, so they should use postMwReqCtx like afterFiles and fallback. prod-server.ts (Pages Router) correctly keeps reqCtx for beforeFiles — the Pages Router order has no middleware step before beforeFiles.
…d has/missing regression tests - Move beforeFiles rewrite block after middleware execution so it sees middleware-injected cookies/headers (matches Next.js App Router order: middleware → beforeFiles → filesystem → afterFiles → fallback) - Fix Map→Record bug in __buildPostMwRequestContext: ctx.cookies is a Map<string,string> but RequestContext.cookies must be Record<string,string>; convert with Object.fromEntries() so cookie lookups work correctly - Update .rsc-suffix stripping test to reflect new code path (cleanPathname is now used directly by beforeFiles rather than a separate __rewritePathname) - Add regression tests: beforeFiles and fallback rewrite has/missing conditions see middleware-injected cookies (app-router); fixtures wire up mw-gated-before and mw-gated-fallback routes + middleware cookie injection
Per Next.js docs, the execution order is: headers → redirects → Middleware → beforeFiles → filesystem → afterFiles → fallback. This applies to both App Router and Pages Router. prod-server.ts was incorrectly using the pre-middleware reqCtx for beforeFiles matching; switch to postMwReqCtx. Add regression test: beforeFiles has/missing conditions on the production Pages Router server now correctly see middleware-injected cookies.
|
/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: fix: rebuild reqCtx for afterFiles/fallback rewrites after middleware
Solid fix. The core problem is well-understood: in Next.js execution order, afterFiles and fallback rewrites (and beforeFiles per the docs) run after middleware, so their has/missing conditions must evaluate against the middleware-modified request, not the original pre-middleware snapshot.
What the PR does correctly
Consistent three-server fix. The change is applied to all three request-handling codepaths that need it:
app-dev-server.ts— introduces__buildPostMwRequestContext()reading from the live ALSHeadersContextafterapplyMiddlewareRequestHeaders()runsprod-server.ts— rebuildspostMwReqCtxviarequestContextFromRequest(webRequest)after unpackingx-middleware-request-*headers into the mutablewebRequestdeploy.ts(Workers entry) — clones the immutable WorkersRequestwith new headers, then rebuildspostMwReqCtxfrom the clone
All three pass postMwReqCtx to afterFiles, fallback, and beforeFiles rewrites while keeping the original reqCtx for headers and redirects — which matches Next.js semantics.
getHeadersContext() export. The new export in headers.ts is minimal and clean — it reads from the existing ALS state and returns the current HeadersContext (or null). No new state introduced.
__buildPostMwRequestContext() fallback. When getHeadersContext() returns null (no middleware, or middleware didn't modify anything), it falls back to __buildRequestContext(request), which is the right behavior — no middleware means pre/post are identical.
Test coverage. Both App Router and Pages Router get tests that verify:
- Without
?mw-auth→ middleware doesn't inject cookie →has:[cookie:...]condition fails → no rewrite → 404 - With
?mw-auth→ middleware injects cookie → condition matches → rewrite to/about→ 200
The tests cover beforeFiles (App Router), afterFiles (Pages Router), and fallback (App Router), which is thorough.
Minor observations
Self-review comment addressed? james-elicx left a self-review suggesting let cleanPathname = pathname.replace(/\.rsc$/, ""); — looking at the generated code at line 1387, it currently uses let cleanPathname = pathname.replace(/\\.rsc$/, ""); with the double-backslash, which is correct since it's inside a template literal string that gets emitted as JavaScript (the \\ becomes \ in the output). The self-review suggestion to use a single \ would produce a regex that matches literal .rsc in the emitted code, which is what we want. Both are functionally equivalent since the generated code goes through string interpolation, but the single backslash is cleaner for readability. Either way, not a bug.
cookiesRecord conversion in __buildPostMwRequestContext(). The Object.fromEntries(ctx.cookies) conversion is necessary because the generated __checkSingleCondition function accesses cookies via ctx.cookies[condition.key] (bracket notation on a plain object), not Map.get(). Good that this is documented in the comment.
beforeFiles uses postMwReqCtx in all three servers. This is consistent with Next.js docs which state the execution order as: headers → redirects → Middleware → beforeFiles → filesystem → afterFiles → fallback. However, it's worth noting that the PR description says "headers, redirects, and beforeFiles rewrites keep using the original pre-middleware context" but the actual implementation passes postMwReqCtx to beforeFiles as well, which is correct per the Next.js docs. The PR description may need a minor correction on this point, but the code is right.
All CI checks passing (E2E, deploys, smoke tests). LGTM.
|
Review posted. Here's a summary: Approved PR #303 — The fix correctly rebuilds the request context after middleware runs so that Key findings:
|
james-elicx
changed the title
1 participant
All three rewrite types —
beforeFiles,afterFiles, andfallback— run after middleware per the Next.js execution order:This applies to both App Router and Pages Router (confirmed in the Next.js 14 docs).
What was wrong
has/missingconditions on rewrite rules were evaluated against the pre-middleware request context, so middleware-injected cookies/headers were invisible to the matcher.Additionally,
beforeFilesrewrites were running before middleware inapp-dev-server.ts— a pre-existing ordering bug, now fixed.What changed
app-dev-server.ts: MovedbeforeFilesblock to after middleware execution. Added__buildPostMwRequestContext()helper that reads the ALS-stored headers context (post-middleware) and convertsMap<string,string>→Record<string,string>for cookie lookups. All three rewrite types now use this post-middleware context.prod-server.ts:beforeFilesnow usespostMwReqCtx(was incorrectly using the pre-middleware snapshot).headersandredirectscorrectly retain the pre-middleware context.deploy.ts: Already correct forafterFiles/fallback;beforeFilesalso switched topostMwReqCtx.shims/headers.ts: ExportedgetHeadersContext()for use in the post-middleware context helper.Tests added
Regression tests covering all three rewrite types across both routers:
afterFilesprod-server.ts)beforeFilesapp-dev-server.ts)beforeFilesprod-server.ts)fallbackapp-dev-server.ts)Each test verifies: without
?mw-auththe rule does not match (404); with?mw-authmiddleware injects the cookie and the rule matches (200, rewrites to/about).