fix(rewrites): serve static files from public/ when rewrite target is a .html path#217
fix(rewrites): serve static files from public/ when rewrite target is a .html path#217yunus25jmi1 wants to merge 1 commit intocloudflare:mainfrom
Conversation
|
@southpolesteve @elithrar kindly review the changes. |
|
Kindly review the changes. @southpolesteve |
01837a4 to
098e14a
Compare
commit: |
|
/bigbonk review |
There was a problem hiding this comment.
Thanks for working on this — the use case (rewriting clean URLs to static .html files in public/) is a real gap. The overall approach of checking the filesystem after rewrites fail to match a route is reasonable. However, there are several issues that should be addressed before merging.
Summary of issues
-
Path traversal guard missing in dev server paths —
prod-server.tsusestryServeStatic()which has traversal protection, but the two dev-server paths (inindex.tsandapp-dev-server.ts) do not. While the rewrite destination is developer-controlled,cleanPathnamein the RSC entry can be influenced by request URLs after middleware processing. -
MIME type map duplication — The MIME type mapping is now maintained in three separate places with inconsistent entries. This will drift over time.
-
publicDirfallback is wrong forsrc/applayouts —path.dirname(appDir)produces/project/src/publicwhenappDiris/project/src/app. The Viterootis always passed from the call site, but the fallback is misleading and would be silently wrong if ever hit. -
String concatenation instead of
path.joinin generated RSC code — The generated code usespublicDir + cleanPathnamewhich doesn't normalize the path. -
deploy.ts(Cloudflare Workers Pages Router entry) not updated — Per AGENTS.md: "When fixing a bug in any of these files, check whether the same bug exists in the others." The Cloudflare Workers entry (deploy.ts) has the same afterFiles/fallback rewrite logic without static file serving. This is likely fine because Cloudflare'sASSETSbinding serves static files at the platform level, but it should be explicitly acknowledged. -
Missing
navigationContextcleanup in RSC entry — When returning the static fileResponse,setHeadersContext(null)andsetNavigationContext(null)are not called, unlike all other early-return paths nearby.
| const allowedOrigins = config?.allowedOrigins ?? []; | ||
| // Compute the public/ directory path for serving static files after rewrites. | ||
| // appDir is something like /project/app or /project/src/app; root is the Vite root. | ||
| const publicDir = path.join(root ?? path.dirname(appDir), "public").replace(/\\/g, "/"); |
There was a problem hiding this comment.
The fallback path.dirname(appDir) is incorrect when appDir is /project/src/app — it would produce /project/src/public instead of /project/public. In practice root is always passed from the call site in index.ts, so this won't be hit at runtime. But the fallback is misleading. Consider throwing or logging a warning if root is not provided, rather than silently computing a wrong path.
|
Review submitted with REQUEST_CHANGES. Here's a summary of the feedback: Key issues identified:
|
|
I noticed in the issue that they were using a nested route, but we're only reproducing with a top-level route - it might be worth adding a nested one as well to ensure we're covering their scenario. |
|
@james-elicx As I go through the issues, I'm going to working on it after dinner. Kindly review the remaining PR. |
…flare#199) Address code review feedback: - Extract shared MIME type map (server/mime.ts) eliminating 3x duplication - Add path traversal guard using resolve + startsWith pattern - Use path.extname() instead of string splitting - Use path.join/resolve in generated RSC entry instead of string concat - Clean up navigation/headers context before returning static response - Add nested route test (public/auth/no-access.html via rewrite)
098e14a to
8f8e18a
Compare
|
@southpolesteve @james-elicx The fixes is pushed. Kindly review the changes. |
|
/bigbonk review |
There was a problem hiding this comment.
Good work on this PR — the use case is real (rewriting to static .html in public/) and the overall approach is sound. The shared MIME map is a nice improvement over the previous state. There are a few issues to address before merging.
Summary
-
includes(".")in prod-server.ts is too broad — The afterFiles and fallback checks inprod-server.tsstill useincludes(".")instead ofpath.extname(), which will trigger unnecessarytryServeStaticfilesystem lookups on URLs like/api/v2.0/data. The Pages Router dev server (index.ts) and the App Router dev server (generated RSC entry) both correctly usepath.extname(). This should be consistent. -
Pages Router dev server only checks static files at the very end — In
index.ts, the static file check runs after both afterFiles and fallback rewrites fail to match a route. Butprod-server.tscorrectly checks immediately after the afterFiles rewrite produces a.htmlpath. This means in dev, if afterFiles rewrites/footo/foo.htmland there's no route match, the code tries fallback rewrites before checking the filesystem. This isn't a correctness bug (the test passes because fallback rewrites won't match either), but it's a behavioral inconsistency with prod and does an unnecessary fallback rewrite attempt. Consider checking right after afterFiles rewrites resolve, matching the prod-server pattern. -
publicDirfallback is misleading — The comment correctly notes thatpath.dirname(appDir)is wrong forsrc/applayouts, but the fallbackroot ?? path.resolve(appDir, "..")is still present. Sincerootis always passed from the call site, this won't be hit in practice, but it would be cleaner to either throw or use a more defensive default.
| // If the rewritten path has a file extension, it may point to a static | ||
| // file in public/ (copied to clientDir during build). Try to serve it | ||
| // directly before falling through to SSR (which would return 404). | ||
| if (resolvedPathname.includes(".") && tryServeStatic(req, res, clientDir, resolvedPathname, compress)) { |
There was a problem hiding this comment.
includes(".") is too broad here — it triggers tryServeStatic on any URL containing a dot (e.g. /api/v2.0/data). Both index.ts and the generated RSC entry use path.extname() for this check. This should be consistent:
| if (resolvedPathname.includes(".") && tryServeStatic(req, res, clientDir, resolvedPathname, compress)) { | |
| if (path.extname(resolvedPathname) && tryServeStatic(req, res, clientDir, resolvedPathname, compress)) { |
| } | ||
| // Check if fallback targets a static file in public/ | ||
| const fallbackPathname = fallbackRewrite.split("?")[0]; | ||
| if (fallbackPathname.includes(".") && tryServeStatic(req, res, clientDir, fallbackPathname, compress)) { |
There was a problem hiding this comment.
Same includes(".") issue as above:
| if (fallbackPathname.includes(".") && tryServeStatic(req, res, clientDir, fallbackPathname, compress)) { | |
| if (path.extname(fallbackPathname) && tryServeStatic(req, res, clientDir, fallbackPathname, compress)) { |
| // corresponds to a static file in the public/ directory (e.g., after a | ||
| // rewrite: /auth/no-access → /auth/no-access.html). Serve it directly so | ||
| // we don't hit the SSR 404 path. | ||
| const resolvedStaticPath = resolvedUrl.split("?")[0]; |
There was a problem hiding this comment.
This static file check runs at the very end, after fallback rewrites have already been tried and failed. In contrast, prod-server.ts checks right after afterFiles rewrites resolve a .html path (line 901) and again after fallback rewrites (line 923). The dev server should match the prod behavior:
- Check for static file right after afterFiles rewrites resolve (before
matchRoute) - Check again after fallback rewrites resolve
This avoids unnecessary fallback rewrite attempts and keeps dev/prod parity — something AGENTS.md explicitly calls out as important.
| // (e.g. /project/src/public instead of /project/public). | ||
| const publicDir = path.join(root ?? path.resolve(appDir, ".."), "public").replace(/\\/g, "/"); | ||
| // Build import map for all page and layout files | ||
| const imports: string[] = []; |
There was a problem hiding this comment.
The fallback root ?? path.resolve(appDir, "..") is documented as wrong for src/app layouts, but still present. Since root is always passed from index.ts, this code path won't execute in normal usage. But if someone adds a new call site without root, they'll get a silently wrong path. Consider making this defensive:
| const imports: string[] = []; | |
| const publicDir = path.join(root ?? (() => { console.warn("[vinext] generateRscEntry: root not provided, public/ path may be incorrect"); return path.resolve(appDir, ".."); })(), "public").replace(/\\/g, "/"); |
Or more simply, just assert that root is provided and throw if not.
| if (__extname) { | ||
| const __publicRoot = ${JSON.stringify(publicDir)}; | ||
| const __publicFilePath = __nodePath.resolve(__publicRoot, "." + cleanPathname); | ||
| // Path traversal guard — resolved path must stay inside public/ |
There was a problem hiding this comment.
Minor: this traversal guard uses hardcoded "/" as the separator. On Windows (if anyone runs the dev server there), path.resolve returns backslash-separated paths, so startsWith(__publicRoot + "/") would fail. Consider using __nodePath.sep instead:
if (__publicFilePath.startsWith(__publicRoot + __nodePath.sep)) {The index.ts version correctly uses path.sep. The generated RSC entry should match.
|
Review submitted. Here's a summary of what I flagged: Review Summary (REQUEST_CHANGES)5 inline comments across 3 files:
|
Fixes #199.
Problem
When
next.configrewrites() map a clean URL to a static.htmlfile inpublic/(a common pattern for serving pre-built static pages like/auth/no-access→/auth/no-access.html), vinext was returning 404 because no Next.js route matches.htmlpaths.Root Cause
After
afterFilesrewrites resolve a path to*.html, the routing logic in all three server paths would find no matching app/pages route for the.htmlURL and return 404, without ever checking the filesystem'spublic/directory.Fix
Three server paths updated:
Pages Router dev (
packages/vinext/src/index.ts): After afterFiles/fallback rewrites, before the finalhandler()call, check if the resolved URL points to a file inpublic/and serve it directly.App Router dev (
packages/vinext/src/server/app-dev-server.ts): The generated RSC entry now checkspublic/for the rewritten pathname before rendering the 404 page. Added a new optionalrootparameter togenerateRscEntry()so the public directory path is embedded in the generated virtual module.Production server (
packages/vinext/src/server/prod-server.ts): After afterFiles rewrites and after fallback rewrites produce a path with a file extension,tryServeStatic()is called against the builtclientDir(which containspublic/files) before passing to SSR.Also added a
staticMimeType()helper inindex.tsto ensure correctContent-Typeheaders.Tests
app-router.test.tsandpages-router.test.ts:GET /static-html-page(rewritten →/static-html-page.html) returns 200 with correct HTML content andtext/htmlContent-Type.app-router.test.ts:generateRscEntry()embeds thepublic/path in the generated code.public/static-html-page.htmlfiles added to bothapp-basicandpages-basicfixtures;next.configrewrites updated.What async rewrites() flat array does
Per Next.js semantics, when
rewrites()returns a flat array, all rules go intoafterFiles. vinext already handles this correctly viaresolveNextConfig(). This PR fixes the serving of the rewritten destination when it's a static file.