fix(app-router): serve public files reached through afterFiles and fallback rewrites - #3429
AhmedElBanna80 wants to merge 9 commits into
Conversation
…llback rewrites The App Router handler checked public files only for the pathname left after beforeFiles rewrites. After an afterFiles or fallback rewrite it re-ran the metadata-route and Pages checks but not the public-file check, so a rewrite whose destination is a file in public/ fell through to the not-found page. Next.js re-checks the filesystem, public files first, after each of those rewrites. Run the existing public-file check after afterFiles and fallback rewrites as well.
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
Check /_next/image, public files, then metadata routes (Next.js filesystem order) after every afterFiles and fallback rewrite, including the out-of-basePath Server Action rewrite loop, and read image params from the rewritten query. Serve static-file signals in plain Node `vinext dev`: @vitejs/plugin-rsc writes the App Router Response straight to Node, so rewrites to public files returned an empty 200. The dev host now scopes a public-file server to each request and the handler resolves signals through it.
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues Please review exact head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| if (!filesystemRouteEligible) return null; | ||
| if (isImageOptimizationPath(cleanPathname)) { | ||
| // Rewrites may supply the image parameters, so read the resolved query. | ||
| const imageRedirect = resolveDevImageRedirect( |
There was a problem hiding this comment.
[P1] Preserve the configured image optimizer after rewrites
This handles every rewritten /_next/image target with the dev passthrough redirect, even in production runtimes. On Cloudflare, the outer entry only invokes handleConfiguredImageOptimization when the original request pathname is /_next/image; an afterFiles rewrite such as /hero -> /_next/image?url=/hero.jpg&w=640&q=75 reaches this branch instead and returns a 302 to the source image, bypassing the configured Images optimizer and its security/format behavior. Late image matches need to be surfaced back to the host optimizer (or optimized here), rather than always using resolveDevImageRedirect.
|
|
||
| // Match Vite/sirv: HEAD evaluates validators and ranges like GET, then omits the body. | ||
| const body = | ||
| request.method === "HEAD" ? null : (await fsp.readFile(filePath)).subarray(start, end + 1); |
There was a problem hiding this comment.
[P2] Stream only the requested file range
readFile loads the entire asset before slicing it. A one-byte range request to a large rewritten public asset therefore reads and retains the whole file in memory, unlike Vite/sirv which streams the selected range. This makes a rewritten video or archive capable of exhausting the dev process with a few requests; use a file stream limited to start/end and a web-stream bridge instead.
There was a problem hiding this comment.
Looks like 12e8c7d already switched this path to fs.createReadStream(filePath, { start, end }), and that is what the current head has. 801146f adds a regression test for it in tests/dev-static-file-server.test.ts (a 10-byte range on a 5 MiB file opens a bounded stream and never goes through readFile).
| ) { | ||
| const devStaticFileServerStorage = getDevStaticFileServerStorage(); | ||
| const serveStaticFile: DevStaticFileServer = (pathname, request) => | ||
| serveDevPublicFile(devPublicDir, pathname, request); |
There was a problem hiding this comment.
[P2] Preserve Vite configured response headers
The replacement path does not receive server.config.server.headers. Vite applies those headers in viteServePublicMiddleware, so with server.headers: { "Content-Security-Policy": "default-src none" }, /file.txt includes the header while an alias rewritten to /file.txt silently drops it. Pass the configured headers into the bridge and merge them with the generated asset headers.
| const headers = new Headers({ | ||
| "Accept-Ranges": "bytes", | ||
| "Cache-Control": "no-cache", | ||
| "Content-Type": contentTypeForPath(filePath), |
There was a problem hiding this comment.
[P2] Match Vite public-file MIME handling
contentTypeForPath is the smaller production table and does not match Vite public middleware. For example Vite explicitly serves public .ts/.tsx/.mts files as text/javascript, while a rewrite to the same file now returns application/octet-stream; even .txt differs because sirv emits text/plain rather than adding the charset used by this table. This can break rewritten scripts. Reuse Vite-compatible MIME lookup/overrides here.
| const root = path.resolve(publicDir); | ||
| const filePath = path.resolve(root, `.${pathname}`); | ||
| const relativePath = path.relative(root, filePath); | ||
| if (!relativePath || relativePath.startsWith("..") || path.isAbsolute(relativePath)) { |
There was a problem hiding this comment.
[P2] Do not reject in-root names beginning with two dots
relativePath.startsWith("..") also matches valid files such as public/..foo.txt. Such a file is included by scanPublicFileRoutes and served by Vite directly, but an alias rewritten to /..foo.txt returns 404 through this bridge. Restrict the traversal check to relativePath === ".." or a ../ path segment plus the absolute-path check.
|
Submitted one review with 5 actionable inline findings. Targeted tests passed: 18/18. |
…ost handlers - Late /_next/image matches return an image-optimization signal so Workers with an optimizer and the Node prod server optimize/serve the image instead of the unoptimized redirect; the signal survives the response stage transport. - Dev public files mirror Vite's public middleware: mrmime content types with Vite's JavaScript override, server.headers, streamed bodies, and in-root names starting with two dots.
|
/bigbonk review for issues Please review exact head |
2 similar comments
|
/bigbonk review for issues Please review exact head |
|
/bigbonk review for issues Please review exact head |
Validation: 768 targeted tests passed. |
… checks Restore the existing /_next/image redirect exactly as it was and leave image optimization reached through afterFiles and fallback rewrites out of this change, which covers public files reached through rewrites.
|
Addressed the review of
Targeted unit tests (803) and |
|
/bigbonk review for issues Please review exact head |
| configuredHeaders?: OutgoingHttpHeaders, | ||
| ): Promise<Response> { | ||
| const root = path.resolve(publicDir); | ||
| const filePath = path.resolve(root, `.${pathname}`); |
There was a problem hiding this comment.
[P2] Decode the routed pathname before resolving it on disk
Vite decodes the URI pathname once before its public-file lookup, and the production tryServeStatic path likewise performs a filesystem decode after decoding the signal. Here the encoded public-route key is resolved verbatim: for a literal public/logo%2Fdark.txt, a rewritten request carrying /logo%252Fdark.txt matches publicFiles, but this code looks for a disk file named logo%252Fdark.txt and returns 404 instead of logo%2Fdark.txt. Safely decode the pathname once (with Vite-compatible malformed-URI handling) before path.resolve, and cover a literal-percent filename.
There was a problem hiding this comment.
Fixed in 97e9db3: the dev server now decodes the route once with decodeURI (keeping malformed paths raw, as sirv does) before resolving on disk. Covered /logo%252Fdark.txt → public/logo%2Fdark.txt, /hello%20copy.txt, and a malformed escape returning 404.
|
Submitted 1 actionable inline finding. Validation: 292 targeted tests passed; two suites could not load because local dependencies lacked newly added |
Public-file route keys are URL-encoded. Decode them once with decodeURI, keeping malformed paths raw as sirv does, before resolving them on disk.
|
/bigbonk review for issues Please review exact head |
| filesystemRouteEligible = true; | ||
| const claimedRscCacheBustingRedirect = await validateClaimedOutsideBasePathRsc(); | ||
| if (claimedRscCacheBustingRedirect) return claimedRscCacheBustingRedirect; | ||
| const rewrittenFilesystemResponse = await resolveFilesystemRoute(); |
There was a problem hiding this comment.
[P2] Preserve metadata-route method handling in the early action rewrite loop
This new filesystem check runs inside the special out-of-basePath Server Action path. If a POST carrying Next-Action is rewritten first to /robots.txt (before a later rewrite would reach its page), resolveFilesystemRoute() now calls renderMetadataRouteIfMatched() and returns the metadata body with 200. The metadata handler receives no method and does not enforce allowed methods, whereas Next.js generated metadata routes expose retrieval methods and answer this POST with 405. The public-file branch already guards GET/HEAD; apply equivalent method handling before dispatching metadata here so the rewrite can stop at the filesystem match without serving GET content to an action POST.
There was a problem hiding this comment.
Thanks. I checked this against the base (517fca9aa), and the missing 405 is pre-existing, so I'm leaving it out of this PR.
- Why it isn't new: metadata route dispatch (
renderMetadataRouteIfMatched,metadata-route-response.ts) never looks at the request method.- On main, a direct
POST /robots.txtalready gets 200 with the metadata body. - So does an action POST whose late rewrite ends at a metadata route, because the post-loop metadata check serves it.
- On main, a direct
- What this PR changes: the action rewrite chain now stops at the first filesystem match instead of going on to later rewrites. That matches Next.js
resolve-routes, which stops there as well and then answers 405 because the generated metadata route module only exports GET. - The actual gap: vinext's metadata routes need method-not-allowed handling for every entry path. That will be a separate change.
Adding a GET/HEAD guard only in this loop would just move the action on to a later rewrite. That diverges from Next.js in a different way, and the direct-request case would still be wrong.
|
Submitted 1 actionable inline finding. Validation: 3 focused tests passed. Two dev suites could not load because local dependencies lacked |
…ading the whole file
2 participants
Problem
In the App Router, an
afterFilesorfallbackrewrite whose destination is a file inpublic/returns the 404 page. AbeforeFilesrewrite to the same file is served, and the Pages Router serves all three phases.With
app/layout.jsx+app/page.jsx,vinext build && vinext startonmain:/before/file.txt/after/file.txt/fallback/file.txt/after/nope.txt(no such file)Next.js re-checks the filesystem after each
afterFilesandfallbackrewrite (checkTrue→fsChecker.getIteminpackages/next/src/server/lib/router-utils/resolve-routes.ts), with public files checked before app and page files.test/e2e/i18n-ignore-rewrite-source-localerelies on this.Cause
createAppRscHandlercallsresolvePublicFileRouteonce, for the pathname left afterbeforeFilesrewrites. TheafterFilesandfallbackloops re-run the metadata-route and Pages checks after a rewrite, but not the public-file check, so the destination is only matched against App routes and falls through to not-found.Fix
Move the existing public-file check into a small local helper and call it after each
afterFilesandfallbackrewrite, right after the metadata-route check, which is the same position it has for the original pathname. The response is the same static-file signal used today, so method handling (405 for non-GET/HEAD) and host-specific asset serving are unchanged.Test
Adds two
it.eachcases (afterFiles,fallback) totests/app-rsc-handler.test.ts:DELETEto the same rewritten path returns 405.All four fail on
main(404) and pass with the change.pnpm run checkpasses, as doapp-rsc-handler,request-pipeline,app-router-production-server,entry-templates,app-worker-stagesandpages-i18n-public-rewrite. I also checked end to end with the fixture above undervinext start.Refs #3427 (under Nitro, rewritten public files are still not served in any phase; that needs an asset source for the Nitro host and is tracked there).