fix: filter internal Next.js headers from inbound requests#1056
Conversation
Next.js strips internal headers (x-nextjs-data, x-matched-path, x-now-route-matches, x-next-resume-state-length, and x-middleware-*) via filterInternalHeaders() at the router-server entry point before any handler or middleware sees the request. This prevents attackers from forging these headers to influence routing or impersonate internal data fetches. vinext was not filtering any of these headers from inbound requests, so all four reached middleware as user-controlled request headers. Ported from Next.js INTERNAL_HEADERS list and filterInternalHeaders(): - https://github.com/vercel/next.js/blob/canary/packages/next/src/server/lib/server-ipc/utils.ts - https://github.com/vercel/next.js/blob/canary/packages/next/src/server/lib/router-server.ts Added filterInternalHeaders() to server/request-pipeline.ts and wired it into all request entry points: - App Router handler (app-rsc-handler.ts) - CF Worker App Router entry (app-router-entry.ts) - Node prod-server (nodeToWebRequest + Pages Router path) - Dev server Pages Router middleware path (index.ts) - Generated Pages Router worker entry (deploy.ts) Filtering runs before middleware, config matching, and routing so no subsystem sees forged internal headers. The x-middleware-* headers are stripped from requests but NOT from middleware responses — the middleware protocol (which reads x-middleware-* from Response headers, not request headers) continues to work correctly.
commit: |
Request.headers is immutable in Cloudflare Workers / Miniflare / workerd environments. The previous in-place mutation via headers.delete() threw 'Can't modify immutable headers', causing every request to 500 in the cloudflare-dev E2E suite. Changed filterInternalHeaders() to always return a new Headers copy (never mutates the input). Call sites that use request.headers now rebuild the Request with the filtered headers, matching the existing pattern in applyMiddlewareRequestHeaders() at config-matchers.ts:495-502.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31e22ad169
ℹ️ 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".
| request = new Request(request.url, { | ||
| method: request.method, | ||
| headers: filteredHeaders, | ||
| body: request.body, |
There was a problem hiding this comment.
Rebuilding the incoming Worker request with new Request(request.url, { method, headers, body }) drops non-header request metadata (notably redirect, signal, and Worker-specific cf data), because a URL-based constructor defaults redirect to follow instead of the original incoming manual mode. This can change app behavior for middleware/route code that forwards the request (fetch(request)), relies on client-disconnect abort signals, or reads Cloudflare request context. The same pattern appears in other new filtering call sites, so this should be switched to cloning from the original Request object (overriding only headers) to keep runtime semantics intact.
Useful? React with 👍 / 👎.
handleAppRscRequest() is an internal handoff point, not a trust boundary. Filtering here broke the x-vinext-mw-ctx handoff in hybrid app+pages dev mode because cloned Request objects could drop coordination state from the connect handler, causing middleware to run twice. Moved filtering to the top of createAppRscHandler(), the true App Router request boundary, before headersContextFromRequest() and requestContextFromRequest() capture any context. This also fixes the security gap where forged headers were captured in the request context before filtering ran.
|
@codex help me fix CI pls |
|
To use Codex here, create an environment for this repo. |
|
@codex help me fix CI pls |
|
To use Codex here, create an environment for this repo. |
…rsc-handler srvx's NodeRequestHeaders reads from rawHeaders for iteration but falls back to req.headers for .get() / .has(). In the dev server we add x-vinext-mw-ctx to req.headers after the Request is built, so it is visible to .get() but lost when filterInternalHeaders iterates. Read it BEFORE iterating so applyForwardedMiddlewareContext can skip middleware and avoid double execution. Fixes CI failure on PR cloudflare#1056.
cloneRequestWithHeaders now copies the Workers cf property in both the primary (try) path and the fallback path. Added 8 unit tests locking down method, URL, redirect, signal, body, cf, and combined metadata preservation to prevent regression to the broken URL-constructor pattern.
|
/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: filter internal Next.js headers from inbound requests
Good security fix. The INTERNAL_HEADERS list matches Next.js canary exactly, the cloneRequestWithHeaders helper correctly addresses the Codex bot's earlier feedback about preserving Request metadata, and all CI is green. A few observations below — mostly nits and one minor suggestion.
Double-filtering
Every App Router path calls filterInternalHeaders twice: once at the entry point (worker entry, prod-server, or dev connect middleware) and once in createAppRscHandler. This is harmless — the second pass finds nothing to strip — and the comment in app-rsc-handler.ts explains it's intentional defense-in-depth. That said, if this ever becomes a hot path concern, consider a Set<string> for the lookup (see inline comment).
Overall
The change is correct, well-tested, and comprehensive across all entry points. The x-vinext-mw-ctx save-and-restore in app-rsc-handler.ts is a nice detail that avoids breaking the hybrid dev-mode middleware forwarding. Ship it.
| export function filterInternalHeaders(headers: Headers): Headers { | ||
| const filtered = new Headers(); | ||
| for (const [key, value] of headers) { | ||
| if (!INTERNAL_HEADERS.includes(key.toLowerCase())) { |
There was a problem hiding this comment.
Nit: INTERNAL_HEADERS.includes() is an O(n) linear scan on every header of every request. With 10 internal headers and typical requests carrying ~15-20 headers, that's ~150-200 string comparisons per request (and this runs twice on App Router paths due to the defense-in-depth pattern in createAppRscHandler).
Not a real problem at this scale, but if you ever want to tighten it:
| if (!INTERNAL_HEADERS.includes(key.toLowerCase())) { | |
| if (!internalHeaderSet.has(key.toLowerCase())) { |
with a const internalHeaderSet = new Set(INTERNAL_HEADERS) defined at module scope. Low priority — just noting for future.
| const mwCtx = rawRequest.headers.get("x-vinext-mw-ctx"); | ||
| const filteredHeaders = filterInternalHeaders(rawRequest.headers); | ||
| if (mwCtx !== null) { | ||
| filteredHeaders.set("x-vinext-mw-ctx", mwCtx); |
There was a problem hiding this comment.
The save-and-restore of x-vinext-mw-ctx is correct and necessary — good catch on the srvx NodeRequestHeaders iteration quirk.
One edge: filterInternalHeaders copies headers by iterating rawRequest.headers, which srvx's NodeRequestHeaders implements via rawHeaders. If any middleware adds x-vinext-mw-ctx to req.headers after the Request is built (which index.ts:2566 does), iteration won't see it, but .get() will — which is exactly the scenario the comment describes. The explicit .get() before iteration is the right fix.
Worth noting: x-vinext-mw-ctx is only consumed in non-production mode (app-middleware.ts:130 checks NODE_ENV !== 'production'), so even if an attacker forges it in production, it's a no-op. But stripping Next.js internal headers while preserving vinext's own internal header is still the right layering.
| // (which reads req.headers directly) must see clean headers. | ||
| const nodeRequestHeaders = filterInternalHeaders(rawHeaders); | ||
| for (const header of INTERNAL_HEADERS) { | ||
| delete req.headers[header]; |
There was a problem hiding this comment.
This is the only call site that mutates the Node req.headers object in addition to filtering the Headers copy. This is necessary because Pages Router SSR reads req.headers directly (not the Web Headers object), but it's a subtle asymmetry worth a brief comment explaining why both operations are needed.
The existing comment says "Both the middleware Request ... and the SSR handler ... must see clean headers" which partially covers it, but the SSR handler reads req.headers (the Node object), not the Headers instance — that's the actual reason for the delete loop. Consider making that explicit:
| delete req.headers[header]; | |
| for (const header of INTERNAL_HEADERS) { | |
| // SSR handlers read req.headers (the Node object) directly, | |
| // so internal headers must be removed from it too. | |
| delete req.headers[header]; | |
| } |
| return filtered; | ||
| } | ||
|
|
||
| function getRequestCf(request: Request): unknown | undefined { |
There was a problem hiding this comment.
Nit: getRequestCf returns unknown | undefined which simplifies to just unknown. The function body also has a redundant conditional — Reflect.get already returns undefined when the property doesn't exist, so the === undefined ? undefined : cf check is a no-op. This is fine for clarity but could be simplified to:
function getRequestCf(request: Request): unknown {
return Reflect.get(request, "cf");
}Not blocking.
| // https://github.com/vercel/next.js/blob/canary/packages/next/src/server/lib/router-server.ts | ||
|
|
||
| describe("INTERNAL_HEADERS", () => { | ||
| it("matches Next.js's exact header list", () => { |
There was a problem hiding this comment.
This test is a good safeguard against drift, but it's asserting the exact same array literal that defines INTERNAL_HEADERS in the source — if someone adds a header in one place and forgets the other, only the test would catch it. That's the intent, but consider adding a comment like "If this test fails, update BOTH this test AND the source list" to make the maintenance contract explicit.
|
Review submitted. Here's a summary of the review: Overall: Ship it. This is a correct and well-implemented security fix. Key findings:
Inline comments were mostly nits: |
|
/bigbonk review |
There was a problem hiding this comment.
Review: filter internal Next.js headers from inbound requests
This is a correct and well-structured security fix. The implementation faithfully ports Next.js's filterInternalHeaders behavior, covers all request entry points, and the cloneRequestWithHeaders helper properly addresses the Codex bot's earlier feedback about preserving Request metadata.
What I verified
- Header list —
INTERNAL_HEADERSmatches Next.js canary'sserver-ipc/utils.tsexactly. - Entry point coverage — All five request paths (App Router worker entry, App Router RSC handler, Pages Router dev connect middleware, Pages Router prod-server
nodeToWebRequest, Pages Router prod-serverstartPagesRouterServer) filter before any middleware/handler runs. cloneRequestWithHeaders— The try/catch pattern handles both Workers (clone-from-Request) and Node/undici (manual init) correctly. Thecfre-attach viaObject.definePropertyis sound — in Workers the clone may already havecf, but re-defining it with the same value is harmless and ensures correctness in the fallback path.x-vinext-mw-ctxpreservation — The save-before-filter pattern inapp-rsc-handler.tscorrectly handles the srvxNodeRequestHeadersiteration quirk. This header is only consumed in dev mode, so even if an attacker forges it in production, it's a no-op.- Tests — Good coverage: exact list parity, case-insensitivity, no-op on clean headers, subset stripping, forged middleware headers, empty headers, and comprehensive
cloneRequestWithHeadersmetadata preservation.
The previous review's nits (Set vs includes, redundant getRequestCf conditional, comment clarifications) are all valid but non-blocking. One minor additional observation below.
Ship it.
| // new Request() does not copy Workers-specific cf, so re-attach it. | ||
| Object.defineProperty(cloned, "cf", { | ||
| value: cf, | ||
| enumerable: true, |
There was a problem hiding this comment.
Minor: In the Workers runtime, new Request(request, { headers }) does preserve cf on the clone. So in the happy path (line 616), the Object.defineProperty here overwrites cf with the identical value. This is harmless — the configurable: true on the original means defineProperty succeeds — but it's worth knowing this is a redundant-but-safe operation in the Workers case and only truly necessary for the catch/fallback path.
Not blocking — just noting for future readers.
| // Builds a new Headers — Request.headers is immutable in Workers. | ||
| { | ||
| const filteredHeaders = filterInternalHeaders(request.headers); | ||
| request = cloneRequestWithHeaders(request, filteredHeaders); |
There was a problem hiding this comment.
Note: this request then flows into rscHandler (line 74) which calls createAppRscHandler, where filterInternalHeaders runs again. The double-filter is intentional defense-in-depth (documented in app-rsc-handler.ts), but the second pass is guaranteed to be a no-op here since the same headers were just stripped. Correct as-is.
| // (which reads req.headers directly) must see clean headers. | ||
| const nodeRequestHeaders = filterInternalHeaders(rawHeaders); | ||
| for (const header of INTERNAL_HEADERS) { | ||
| delete req.headers[header]; |
There was a problem hiding this comment.
+1 to the previous review's suggestion here. The delete req.headers[header] loop is necessary because the Pages Router SSR handler reads the Node IncomingMessage.headers object directly (not the Web Headers instance). A one-line comment clarifying that would save future readers from wondering why both operations are needed:
| delete req.headers[header]; | |
| // Pages Router SSR reads req.headers (Node object) directly, | |
| // so internal headers must be removed from it too. | |
| delete req.headers[header]; |
|
Review submitted with approval. Here's a summary: Approved — this is a correct and comprehensive security fix. Key findings:
I left 3 inline comments: a note about redundant-but-harmless |
2 participants
Summary
filterInternalHeaders()to strip internal Next.js headers from inbound requests at all entry points, matching Next.js behaviorx-nextjs-data,x-matched-path,x-now-route-matches,x-next-resume-state-length, andx-middleware-*headers to influence routing or impersonate internal stateProblem
Next.js calls
filterInternalHeaders()at the router-server entry point before any handler or middleware sees the request. This strips theINTERNAL_HEADERSlist from the request.vinext was not filtering any of these headers. All four headers reached middleware as user-controlled request headers.
Fix
Added
INTERNAL_HEADERSconstant andfilterInternalHeaders(headers: Headers)toserver/request-pipeline.ts, ported directly from Next.js. Wired into every request entry point:server/app-rsc-handler.tsserver/app-router-entry.tsserver/prod-server.ts(nodeToWebRequest + Pages path)index.tsdeploy.tsFiltering runs before middleware, config matching, and routing — no subsystem sees forged internal headers. The
x-middleware-*headers are stripped from requests but NOT from middleware responses — the middleware protocol (which readsx-middleware-*fromResponse.headers, notRequest.headers) continues to work correctly.Tests
Added 8 unit tests in
tests/request-pipeline.test.ts:INTERNAL_HEADERSx-middleware-rewrite+x-middleware-nextstrippedAll related tests pass (request-pipeline: 78, deploy: 210, app-router: 300, features: 267, pages-router: 200).