fix(middleware): align cookies and external rewrites#919
fix(middleware): align cookies and external rewrites#919NathanDrake2406 wants to merge 1 commit intocloudflare:mainfrom
Conversation
commit: |
There was a problem hiding this comment.
Pull request overview
This PR improves Next.js parity in vinext’s middleware pipeline by (1) making middleware cookie mutations observable during the same render via x-middleware-set-cookie, and (2) preserving/proxying cross-origin middleware rewrites as true external destinations instead of collapsing them to local paths.
Changes:
- Mirror
NextResponse.cookies.set()/delete()mutations intox-middleware-set-cookieand merge that internal header into the request cookie context socookies()can see middleware-set values in the same render. - Preserve absolute external rewrite destinations from middleware and proxy them (including request body + middleware request-header overrides) across App Router dev/prod, Pages Router, and deploy output.
- Strip internal
x-middleware-*headers from finalized client responses, route handler responses, and external upstream proxy requests.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/shims.test.ts | Adds coverage for same-render middleware cookie visibility, x-middleware-set-cookie emission, external rewrite preservation, and internal header stripping. |
| tests/pages-router.test.ts | Adds Pages Router integration tests for preserving external middleware rewrite destinations and stripping internal cookie headers. |
| tests/fixtures/pages-basic/middleware.ts | Adds fixture routes for external middleware rewrite and a blocked response that sets cookies. |
| tests/fixtures/app-basic/middleware.ts | Adds App Router fixture route for external middleware rewrite + request header overrides. |
| tests/deploy.test.ts | Asserts deploy entry proxies external middleware rewrites before local route handling. |
| tests/app-router.test.ts | Adds assertions for generated RSC entry external middleware rewrite proxying + an e2e proxy test with body/headers. |
| tests/app-route-handler-response.test.ts | Verifies internal middleware headers are stripped during route handler response finalization. |
| tests/snapshots/entry-templates.test.ts.snap | Updates snapshots for new external middleware rewrite proxy helpers in generated entries. |
| packages/vinext/src/shims/server.ts | Switches NextResponse cookies to a middleware-aware implementation that syncs x-middleware-set-cookie. |
| packages/vinext/src/shims/headers.ts | Merges x-middleware-set-cookie into the request cookie map during applyMiddlewareRequestHeaders(). |
| packages/vinext/src/server/prod-server.ts | Proxies external middleware rewrites early in the Pages Router prod pipeline and merges middleware headers into proxy responses. |
| packages/vinext/src/server/middleware.ts | Preserves absolute external rewrite URLs, and strips internal middleware headers from custom middleware responses. |
| packages/vinext/src/server/middleware-request-headers.ts | Keeps x-middleware-set-cookie alongside request-override headers for post-middleware processing. |
| packages/vinext/src/server/app-route-handler-response.ts | Strips x-middleware-* headers from finalized route handler responses and excludes them from cached header sets. |
| packages/vinext/src/index.ts | Applies middleware request-header overrides before proxying external rewrites in dev Pages Router handling. |
| packages/vinext/src/entries/pages-server-entry.ts | Updates generated Pages Router server entry to preserve external rewrite destinations and strip internal headers on custom responses. |
| packages/vinext/src/entries/app-rsc-entry.ts | Adds external middleware rewrite proxying (inline + forwarded dev middleware context) with middleware request-header override support. |
| packages/vinext/src/deploy.ts | Proxies external middleware rewrites early in the worker entry path and merges middleware headers into proxy responses. |
| packages/vinext/src/config/config-matchers.ts | Ensures external proxy requests never forward x-vinext-mw-ctx (and continues stripping x-middleware-*). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function mergeMiddlewareSetCookies(ctx: HeadersContext, rawHeader: string | null): boolean { | ||
| if (rawHeader === null) return false; | ||
|
|
||
| let merged = false; | ||
| for (const setCookie of splitMiddlewareSetCookieHeader(rawHeader)) { | ||
| const entry = setCookieNameValue(setCookie); | ||
| if (!entry) continue; | ||
| ctx.cookies.set(entry.name, entry.value); | ||
| merged = true; | ||
| } |
There was a problem hiding this comment.
mergeMiddlewareSetCookies() only merges the name/value portion of each Set-Cookie string into ctx.cookies. That means cookies deleted in middleware (e.g. via NextResponse.cookies.delete(), which emits an expired Set-Cookie) will still be visible to cookies() in the same render as an empty-string value rather than being removed. Consider detecting deletion semantics (e.g. Max-Age=0 and/or Expires=Thu, 01 Jan 1970 00:00:00 GMT) and calling ctx.cookies.delete(name) instead of set(name, "") in that case, to match request-cookie behavior after a delete.
There was a problem hiding this comment.
I am going to leave this as-is for parity with Next.js. Next merges middleware-set cookies by parsing x-middleware-set-cookie into ResponseCookies and then transferring each cookie with existingCookies.set(cookie), not by interpreting expiry metadata as a request-cookie delete: https://github.com/vercel/next.js/blob/16e5f9e685188751f1f4fb085f6e9e729f409950/packages/next/src/server/async-storage/request-store.ts#L92-L104
The compiled RequestCookies.set() only stores the cookie name/value and rewrites the Cookie header; it does not inspect expires or maxAge: https://github.com/vercel/next.js/blob/16e5f9e685188751f1f4fb085f6e9e729f409950/packages/next/src/compiled/@edge-runtime/cookies/index.js#L212-L222
So treating an expired middleware cookie as a deletion here would be a deliberate divergence from the Next request-store merge path rather than a parity fix.
| // ��─ 6. Apply beforeFiles rewrites from next.config.js ───────── | ||
| if (isExternalUrl(resolvedUrl)) { | ||
| const proxyResponse = await proxyExternalRequest(request, resolvedUrl); | ||
| return mergeHeaders(proxyResponse, middlewareHeaders, middlewareRewriteStatus); |
There was a problem hiding this comment.
In the external-middleware-rewrite proxy path, mergeHeaders(proxyResponse, middlewareHeaders, middlewareRewriteStatus) will override the upstream proxy response status whenever middleware used a non-200 rewrite status. This differs from the App Router external-middleware-rewrite proxy implementation (which returns the upstream status), and can change behavior if the upstream returns (for example) 404/500. If the goal is to preserve upstream status for external middleware rewrites, pass undefined here (or gate the override on same-origin rewrites only).
| return mergeHeaders(proxyResponse, middlewareHeaders, middlewareRewriteStatus); | |
| return mergeHeaders(proxyResponse, middlewareHeaders, undefined); |
| const mergedResponse = mergeWebResponse( | ||
| middlewareHeaders, | ||
| proxyResponse, | ||
| middlewareRewriteStatus, |
There was a problem hiding this comment.
For external middleware rewrites, mergeWebResponse(..., proxyResponse, middlewareRewriteStatus) will override the upstream proxy status when middleware sets a custom rewrite status. The App Router proxy path in this PR preserves the upstream status for external rewrites, so this is inconsistent and may break parity (e.g. upstream 404 becomes middleware 403). Consider not applying middlewareRewriteStatus when resolvedUrl is external, or otherwise documenting/aligning the intended behavior.
| middlewareRewriteStatus, | |
| undefined, |
Middleware cookies set through NextResponse only updated Set-Cookie, so cookies() in the same request could not observe values written by middleware. External middleware rewrites were normalized to pathname and search, so cross-origin destinations were routed locally instead of being proxied. Mirror middleware cookie mutations into x-middleware-set-cookie and merge that internal header back into the request cookie store. Preserve cross-origin rewrite URLs through middleware execution and proxy them across app, pages, prod, and deploy request paths, while stripping internal middleware headers from client and upstream responses.
4e367f9 to
4062653
Compare
What this changes
Middleware cookie writes made through
NextResponse.cookies.set()are now mirrored intox-middleware-set-cookieand merged back into the request cookie store, socookies()can observe middleware-set values in the same render.External middleware rewrites now preserve the full absolute destination and proxy the request instead of collapsing the URL to
pathname + search. This covers App Router dev, Pages Router generated entry handling, Pages production, deploy output, and the dev external proxy path.Why
Next.js mirrors middleware cookie writes into
x-middleware-set-cookieand later merges that internal header into the request store before rendering. Without that internal header, auth and session flows that set a cookie in middleware and read it in the same render observeundefined.Next.js also treats cross-origin middleware rewrites as external proxy destinations. vinext was stripping the origin from middleware rewrite URLs, which made
NextResponse.rewrite("https://api.example.com/...")route locally and often return a local 404.Next.js references
NextResponsemirrors cookie mutations intox-middleware-set-cookiemergeMiddlewareCookiesmergesx-middleware-set-cookieinto request cookies beforecookies()reads themresolve-routesapplies middleware request-header overrides and keepsx-middleware-set-cookieinternalresolve-routestreats protocol rewrite destinations as finished external rewritesrouter-serverproxies finished protocol destinations throughproxyRequestApproach
NextResponsecookie handling that syncsSet-Cookieintox-middleware-set-cookie.x-middleware-set-cookieinto the request cookie context while preserving existing request-header override behavior.proxyExternalRequest.x-middleware-*headers, includingx-middleware-set-cookie, from client responses and external upstream requests.Validation
vp test run tests/shims.test.ts tests/app-route-handler-response.test.ts tests/deploy.test.tsvp test run tests/app-router.test.ts -t "external middleware rewrites|proxies external URLs returned by middleware rewrites with body and headers|external rewrite proxy credential forwarding"vp test run tests/pages-router.test.ts -t "runMiddleware preserves external middleware rewrite destinations|runMiddleware strips internal cookie headers from custom responses|middleware rewrite status"vp test run tests/deploy.test.ts tests/pages-router.test.ts -t "proxies external middleware rewrites before local route handling|preserves upstream status for external middleware rewrites in production|runMiddleware preserves external middleware rewrite destinations|runMiddleware strips internal cookie headers from custom responses|middleware rewrite status"vp test run tests/entry-templates.test.tsvia commit hook, updating snapshotsvp checkvp run vinext#buildRisks / follow-ups
External middleware rewrites intentionally preserve the upstream proxy response status. That matches the Next.js external proxy path, where the middleware rewrite status is not carried into
proxyRequest.