fix(rewrites): include middleware headers in static file responses#776
fix(rewrites): include middleware headers in static file responses#776yunus25jmi1 wants to merge 2 commits intocloudflare:mainfrom
Conversation
yunus25jmi1
commented
Apr 4, 2026
yunus25jmi1
commented
- Add CONTENT_TYPES allowlist for MIME type detection
- Apply middleware-set headers (like Set-Cookie) to static file responses
- Serve static files for beforeFiles and fallback rewrite targets
commit: |
|
/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 Summary
This PR addresses a real gap — middleware headers (e.g. Set-Cookie) being dropped on static file responses, and rewrite targets that resolve to static files not being served. However, the PR has several issues that need to be resolved before it can be merged.
1. Branch is severely outdated — will not merge cleanly
This branch is based on a very old commit (95905ec, PR #602). The main branch has since undergone significant refactoring:
prod-server.tswas refactored to useStaticFileCache(in-memory pre-computed metadata cache with ETag support) — thetryServeStaticfunction is nowasyncwith a different signatureCONTENT_TYPESwas extracted to a separatestatic-file-cache.tsmodule- Various other changes to the request handling pipeline
The changes to prod-server.ts will not apply to current main at all. The code this PR modifies no longer exists in that form.
Action required: Rebase onto current main and re-implement the changes against the current codebase.
2. Behavioral regression in the static asset skip logic (index.ts)
The old guard was:
if (pathname.includes(".") && !pathname.endsWith(".html")) {
return next();
}The new guard is:
if (ext && ext !== ".html" && CONTENT_TYPES[ext]) {
applyDeferredMwHeaders(res, deferredMwResponseHeaders);
return next();
}The CONTENT_TYPES[ext] check means files with extensions not in the allowlist (e.g. .xml, .txt, .pdf, .mp4, .webm, .zip, .csv, .yaml, .toml, .wasm) will fall through to the Pages Router SSR handler instead of being served as static files by Vite. This is a regression — previously any file with a dot in the path was passed to Vite's static middleware.
3. applyDeferredMwHeaders is called before middleware runs
The function is hoisted above the middleware execution block and deferredMwResponseHeaders is initialized to null. At the point where the static asset check runs (line 2727-2732), middleware has not yet executed — deferredMwResponseHeaders is still null. So the applyDeferredMwHeaders(res, deferredMwResponseHeaders) call is always a no-op in the current code position. The comment says "If middleware was run, apply its headers" but middleware hasn't run at that point in the control flow.
For this to work, the static file check would need to be moved after middleware execution, or middleware would need to run before the static file bailout — but that changes the execution order semantics.
4. Duplicate CONTENT_TYPES constant
The PR adds a CONTENT_TYPES map to index.ts that's identical to the one already in prod-server.ts. On current main, this was further extracted to static-file-cache.ts. Adding a duplicate creates a maintenance burden — when one is updated, the other can easily be forgotten.
5. No tests
Per the project's AGENTS.md, tests should be added first. This PR includes no tests for the new behavior. Specifically:
- No test verifying middleware
Set-Cookieheaders appear on static file responses - No test verifying that
beforeFiles/fallbackrewrites targeting static files serve those files - No test verifying the changed extension-checking behavior
| ".avif": "image/avif", | ||
| ".map": "application/json", | ||
| ".rsc": "text/x-component", | ||
| }; |
There was a problem hiding this comment.
This CONTENT_TYPES map is a duplicate of the one in prod-server.ts (line 226). On current main, it has been further extracted into server/static-file-cache.ts. Adding a second copy creates a maintenance hazard — when one is updated the other is easily forgotten.
After rebasing onto main, import CONTENT_TYPES from static-file-cache.ts instead of duplicating it.
| // If middleware was run, apply its headers (Set-Cookie, etc.) | ||
| // before Vite's built-in static-file middleware sends the file. | ||
| // This ensures public/ asset responses have middleware headers. | ||
| applyDeferredMwHeaders(res, deferredMwResponseHeaders); |
There was a problem hiding this comment.
This applyDeferredMwHeaders() call is always a no-op. At this point in the control flow, middleware has not yet executed — deferredMwResponseHeaders is still null (initialized on line 2625). Middleware runs ~100 lines below (line 2840). So the comment "If middleware was run, apply its headers" is inaccurate.
For middleware headers to actually appear on static file responses, either:
- Move the static-file bailout to after the middleware block, or
- Run middleware before the extension check.
Both options change the execution order, so this needs careful thought about whether it matches Next.js semantics. Per the Next.js execution order, middleware runs before filesystem routes — so option (1) or (2) would actually be more correct for public/ directory files.
| if (pathname.includes(".") && !pathname.endsWith(".html")) { | ||
| const [pathnameWithExt] = url.split("?"); | ||
| const ext = path.extname(pathnameWithExt); | ||
| if (ext && ext !== ".html" && CONTENT_TYPES[ext]) { |
There was a problem hiding this comment.
Behavioral regression: the old guard was pathname.includes(".") && !pathname.endsWith(".html") which matched any file with a dot in its path. The new guard requires the extension to be in the CONTENT_TYPES allowlist.
This means files with unlisted extensions (.xml, .txt, .pdf, .mp4, .webm, .zip, .csv, .yaml, .wasm, etc.) will no longer be passed to Vite's static middleware and will instead fall through to the Pages Router SSR handler, likely resulting in 404s or broken responses for those file types.
If the goal is to apply middleware headers to static files, the extension allowlist approach is the wrong tool — keep the original broad check and apply headers conditionally:
| if (ext && ext !== ".html" && CONTENT_TYPES[ext]) { | |
| if (ext && ext !== ".html") { |
| let middlewareRequestHeaders: Headers | null = null; | ||
| let deferredMwResponseHeaders: [string, string][] | null = null; | ||
|
|
||
| const applyDeferredMwHeaders = ( |
There was a problem hiding this comment.
The refactored applyDeferredMwHeaders now takes explicit response and headers parameters instead of closing over res and deferredMwResponseHeaders. While this is a reasonable API improvement, note that adding the x-middleware-* header filter here changes behavior at every call site — the original applyDeferredMwHeaders() did not filter x-middleware-* headers.
On main, the middleware response header collection already strips x-middleware-* headers when building deferredMwResponseHeaders (see the if (!key.startsWith("x-middleware-")) check around line 2925). So the filter in applyDeferredMwHeaders is redundant for the existing call sites but acts as defense-in-depth. This is fine, but worth noting for correctness — double-filtering is harmless but the original code path already handled it.
| resolvedPathname = rewritten.split("?")[0]; | ||
|
|
||
| if ( | ||
| path.extname(resolvedPathname) && |
There was a problem hiding this comment.
This change won't apply to current main. The tryServeStatic function on main is now async (returns Promise<boolean>), accepts a StaticFileCache parameter, and has ETag/304 support. The entire startPagesRouterServer function has been refactored.
After rebasing, this logic needs to be re-implemented using the new tryServeStatic signature and staticCache parameter.
| return; | ||
| } | ||
| const fallbackPathname = fallbackRewrite.split("?")[0]; | ||
| if ( |
There was a problem hiding this comment.
Same issue as the beforeFiles block above — this code targets an older version of prod-server.ts. After rebasing, re-implement against the current tryServeStatic API.
Also note: the afterFiles rewrite block in prod-server.ts on main (around line 1340) doesn't have this static-file check either, so you'll want to add it there too for consistency.
|
Review posted successfully. Here's a summary of the findings: Requested changes on PR #776 with 6 inline comments and a detailed summary. The key issues:
|
Add static file serving for beforeFiles rewrite targets in the Pages Router production server. This matches Next.js behavior where beforeFiles rewrites can resolve to static files in public/ or other filesystem paths. The fix passes middleware headers (including Set-Cookie) to the static file response, ensuring middleware-set headers appear on static assets. This resolves the issue from PR cloudflare#776 where beforeFiles rewrites that target static files were not being served correctly.
yunus25jmi1
force-pushed
the
fix/static-file-rewrites-middleware-headers
branch
3 times, most recently
from
f286def to
8e0c35b
Compare
- Add CONTENT_TYPES allowlist for MIME type detection - Apply middleware-set headers (like Set-Cookie) to static file responses - Serve static files for beforeFiles and fallback rewrite targets
yunus25jmi1
force-pushed
the
fix/static-file-rewrites-middleware-headers
branch
from
8e0c35b to
f64ea9f
Compare
Add static file serving for beforeFiles rewrite targets in the Pages Router production server. This matches Next.js behavior where beforeFiles rewrites can resolve to static files in public/ or other filesystem paths. The fix passes middleware headers (including Set-Cookie) to the static file response, ensuring middleware-set headers appear on static assets. This mirrors the existing behavior for afterFiles and fallback rewrites.
2 participants
