-
Notifications
You must be signed in to change notification settings - Fork 210
fix(rewrites): serve static files from public/ when rewrite target is a .html path #217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -40,6 +40,7 @@ import { scanMetadataFiles } from "./server/metadata-routes.js"; | |||||||||||||
| import { staticExportPages } from "./build/static-export.js"; | ||||||||||||||
| import { detectPackageManager } from "./utils/project.js"; | ||||||||||||||
| import { asyncHooksStubPlugin } from "./plugins/async-hooks-stub.js"; | ||||||||||||||
| import { mimeType } from "./server/mime.js"; | ||||||||||||||
| import tsconfigPaths from "vite-tsconfig-paths"; | ||||||||||||||
| import react, { Options as VitePluginReactOptions } from "@vitejs/plugin-react"; | ||||||||||||||
| import MagicString from "magic-string"; | ||||||||||||||
|
|
@@ -2343,7 +2344,7 @@ hydrate(); | |||||||||||||
| headers: nextConfig?.headers, | ||||||||||||||
| allowedOrigins: nextConfig?.serverActionsAllowedOrigins, | ||||||||||||||
| allowedDevOrigins: nextConfig?.allowedDevOrigins, | ||||||||||||||
| }, instrumentationPath); | ||||||||||||||
| }, instrumentationPath, root); | ||||||||||||||
| } | ||||||||||||||
| if (id === RESOLVED_APP_SSR_ENTRY && hasAppDir) { | ||||||||||||||
| return generateSsrEntry(); | ||||||||||||||
|
|
@@ -2969,10 +2970,32 @@ hydrate(); | |||||||||||||
| nextConfig.rewrites.afterFiles, | ||||||||||||||
| reqCtx, | ||||||||||||||
| ); | ||||||||||||||
| if (afterRewrite) resolvedUrl = afterRewrite; | ||||||||||||||
| if (afterRewrite) { | ||||||||||||||
| // External rewrite from afterFiles — proxy to external URL | ||||||||||||||
| if (isExternalUrl(afterRewrite)) { | ||||||||||||||
| await proxyExternalRewriteNode(req, res, afterRewrite); | ||||||||||||||
| return; | ||||||||||||||
| } | ||||||||||||||
| resolvedUrl = afterRewrite; | ||||||||||||||
| // If the rewritten path has a file extension, it may point to a | ||||||||||||||
| // static file in public/. Serve it directly before route matching | ||||||||||||||
| // (which would miss it and SSR would return 404). | ||||||||||||||
| const afterFilesPathname = afterRewrite.split("?")[0]; | ||||||||||||||
| if (path.extname(afterFilesPathname)) { | ||||||||||||||
| const resolvedPublicDir = path.resolve(root, "public"); | ||||||||||||||
| const publicFilePath = path.resolve(root, "public", "." + afterFilesPathname); | ||||||||||||||
| if (publicFilePath.startsWith(resolvedPublicDir + path.sep) && fs.existsSync(publicFilePath) && fs.statSync(publicFilePath).isFile()) { | ||||||||||||||
|
Comment on lines
+2985
to
+2987
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit:
Suggested change
Same pattern applies to the fallback block at lines 3030-3032. |
||||||||||||||
| const content = fs.readFileSync(publicFilePath); | ||||||||||||||
| const ext = (path.extname(afterFilesPathname).slice(1)).toLowerCase(); | ||||||||||||||
| res.writeHead(200, { "Content-Type": mimeType(ext) }); | ||||||||||||||
| res.end(content); | ||||||||||||||
| return; | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // External rewrite from afterFiles — proxy to external URL | ||||||||||||||
| // External rewrite (from beforeFiles) — proxy to external URL | ||||||||||||||
| if (isExternalUrl(resolvedUrl)) { | ||||||||||||||
| await proxyExternalRewriteNode(req, res, resolvedUrl); | ||||||||||||||
| return; | ||||||||||||||
|
|
@@ -3001,6 +3024,19 @@ hydrate(); | |||||||||||||
| await proxyExternalRewriteNode(req, res, fallbackRewrite); | ||||||||||||||
| return; | ||||||||||||||
| } | ||||||||||||||
| // Check if fallback targets a static file in public/ | ||||||||||||||
| const fallbackPathname = fallbackRewrite.split("?")[0]; | ||||||||||||||
| if (path.extname(fallbackPathname)) { | ||||||||||||||
| const resolvedPublicDir = path.resolve(root, "public"); | ||||||||||||||
| const publicFilePath = path.resolve(root, "public", "." + fallbackPathname); | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same redundant
Suggested change
|
||||||||||||||
| if (publicFilePath.startsWith(resolvedPublicDir + path.sep) && fs.existsSync(publicFilePath) && fs.statSync(publicFilePath).isFile()) { | ||||||||||||||
| const content = fs.readFileSync(publicFilePath); | ||||||||||||||
| const ext = (path.extname(fallbackPathname).slice(1)).toLowerCase(); | ||||||||||||||
| res.writeHead(200, { "Content-Type": mimeType(ext) }); | ||||||||||||||
| res.end(content); | ||||||||||||||
| return; | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| await handler(req, res, fallbackRewrite, mwStatus); | ||||||||||||||
| return; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -7,13 +7,15 @@ | |||||
| * the SSR entry for HTML generation. | ||||||
| */ | ||||||
| import fs from "node:fs"; | ||||||
| import path from "node:path"; | ||||||
| import { fileURLToPath } from "node:url"; | ||||||
| import type { AppRoute } from "../routing/app-router.js"; | ||||||
| import type { MetadataFileRoute } from "./metadata-routes.js"; | ||||||
| import type { NextRedirect, NextRewrite, NextHeader } from "../config/next-config.js"; | ||||||
| import { generateDevOriginCheckCode } from "./dev-origin-check.js"; | ||||||
| import { generateSafeRegExpCode, generateMiddlewareMatcherCode, generateNormalizePathCode } from "./middleware-codegen.js"; | ||||||
| import { isProxyFile } from "./middleware.js"; | ||||||
| import { MIME_TYPES } from "./mime.js"; | ||||||
|
|
||||||
| /** | ||||||
| * Resolved config options relevant to App Router request handling. | ||||||
|
|
@@ -50,13 +52,19 @@ export function generateRscEntry( | |||||
| trailingSlash?: boolean, | ||||||
| config?: AppRouterConfig, | ||||||
| instrumentationPath?: string | null, | ||||||
| root?: string, | ||||||
| ): string { | ||||||
| const bp = basePath ?? ""; | ||||||
| const ts = trailingSlash ?? false; | ||||||
| const redirects = config?.redirects ?? []; | ||||||
| const rewrites = config?.rewrites ?? { beforeFiles: [], afterFiles: [], fallback: [] }; | ||||||
| const headers = config?.headers ?? []; | ||||||
| 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. | ||||||
| // We require `root` for correctness — path.dirname(appDir) is wrong for src/app layouts | ||||||
| // (e.g. /project/src/public instead of /project/public). | ||||||
| const publicDir = root ? path.join(root, "public") : null; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When Consider either:
if (!root) console.warn("[vinext] generateRscEntry: root not provided, static file serving after rewrites will be disabled"); |
||||||
| // Build import map for all page and layout files | ||||||
| const imports: string[] = []; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fallback
Suggested change
Or more simply, just assert that |
||||||
| const importMap: Map<string, string> = new Map(); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When Consider either making |
||||||
|
|
@@ -207,6 +215,8 @@ ${slotEntries.join(",\n")} | |||||
| }); | ||||||
|
|
||||||
| return ` | ||||||
| import __nodeFs from "node:fs"; | ||||||
| import __nodePath from "node:path"; | ||||||
| import { | ||||||
| renderToReadableStream, | ||||||
| decodeReply, | ||||||
|
|
@@ -1825,6 +1835,27 @@ async function _handleRequest(request, __reqCtx, _mwCtx) { | |||||
| } | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dev/prod parity issue: This static file check only runs inside The current flow in the App Router dev path:
The check should be inserted right after |
||||||
|
|
||||||
| if (!match) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dev/prod parity issue — This is the third review cycle flagging this, and it remains the most important structural issue in the PR. This static file check only runs inside Concrete flow difference in the App Router dev path:
The fix is to insert a static file check right after Per AGENTS.md: "When fixing a bug in any of these files, check whether the same bug exists in the others. Do not leave known bugs as 'follow-ups' — fix them in the same PR." The parity requirement applies equally to behavioral consistency across the three server implementations. |
||||||
| // If the path has a file extension (e.g. /auth/no-access.html after a rewrite), | ||||||
| // check whether it corresponds to a static file in public/ before returning 404. | ||||||
| const __extname = __nodePath.extname(cleanPathname); | ||||||
| if (__extname && ${JSON.stringify(publicDir)} !== null) { | ||||||
| const __publicRoot = ${JSON.stringify(publicDir)}; | ||||||
| const __publicFilePath = __nodePath.resolve(__publicRoot, "." + cleanPathname); | ||||||
| // Path traversal guard — resolved path must stay inside public/ | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: this traversal guard uses hardcoded if (__publicFilePath.startsWith(__publicRoot + __nodePath.sep)) {The |
||||||
| if (__publicFilePath.startsWith(__publicRoot + __nodePath.sep)) { | ||||||
| try { | ||||||
| const __stat = __nodeFs.statSync(__publicFilePath); | ||||||
| if (__stat.isFile()) { | ||||||
| const __content = __nodeFs.readFileSync(__publicFilePath); | ||||||
| const __ext = __extname.slice(1).toLowerCase(); | ||||||
| const __mimeTypes = ${JSON.stringify(MIME_TYPES)}; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This Something like: // At module scope in the generated code
const __mimeTypes = { ... };Then reference |
||||||
| setHeadersContext(null); | ||||||
| setNavigationContext(null); | ||||||
| return new Response(__content, { status: 200, headers: { "Content-Type": __mimeTypes[__ext] ?? "application/octet-stream" } }); | ||||||
| } | ||||||
| } catch { /* file doesn't exist or not readable */ } | ||||||
| } | ||||||
| } | ||||||
| // Render custom not-found page if available, otherwise plain 404 | ||||||
| const notFoundResponse = await renderNotFoundPage(null, isRscRequest, request); | ||||||
| if (notFoundResponse) return notFoundResponse; | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| /** | ||
| * Shared MIME type map for serving static files. | ||
| * | ||
| * Used by index.ts (Pages Router dev), app-dev-server.ts (generated RSC entry), | ||
| * and prod-server.ts (production server). Centralised here to avoid drift. | ||
| * | ||
| * Keys are bare extensions (no leading dot). Use `mimeType()` for lookup. | ||
| */ | ||
| export const MIME_TYPES: Record<string, string> = { | ||
| html: "text/html; charset=utf-8", | ||
| htm: "text/html; charset=utf-8", | ||
| css: "text/css", | ||
| js: "application/javascript", | ||
| mjs: "application/javascript", | ||
| cjs: "application/javascript", | ||
| json: "application/json", | ||
| txt: "text/plain", | ||
| xml: "application/xml", | ||
| svg: "image/svg+xml", | ||
| png: "image/png", | ||
| jpg: "image/jpeg", | ||
| jpeg: "image/jpeg", | ||
| gif: "image/gif", | ||
| webp: "image/webp", | ||
| avif: "image/avif", | ||
| ico: "image/x-icon", | ||
| woff: "font/woff", | ||
| woff2: "font/woff2", | ||
| ttf: "font/ttf", | ||
| otf: "font/otf", | ||
| eot: "application/vnd.ms-fontobject", | ||
| pdf: "application/pdf", | ||
| map: "application/json", | ||
| mp4: "video/mp4", | ||
| webm: "video/webm", | ||
| mp3: "audio/mpeg", | ||
| ogg: "audio/ogg", | ||
| }; | ||
|
|
||
| /** | ||
| * Look up a MIME type by bare extension (no leading dot). | ||
| * Returns "application/octet-stream" for unknown extensions. | ||
| */ | ||
| export function mimeType(ext: string): string { | ||
| return MIME_TYPES[ext] ?? "application/octet-stream"; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ import type { RequestContext } from "../config/config-matchers.js"; | |
| import { IMAGE_OPTIMIZATION_PATH, IMAGE_CONTENT_SECURITY_POLICY, parseImageParams, isSafeImageContentType, DEFAULT_DEVICE_SIZES, DEFAULT_IMAGE_SIZES, type ImageConfig } from "./image-optimization.js"; | ||
| import { normalizePath } from "./normalize-path.js"; | ||
| import { computeLazyChunks } from "../index.js"; | ||
| import { mimeType } from "./mime.js"; | ||
|
|
||
| /** Convert a Node.js IncomingMessage into a ReadableStream for Web Request body. */ | ||
| function readNodeStream(req: IncomingMessage): ReadableStream<Uint8Array> { | ||
|
|
@@ -184,27 +185,11 @@ function sendCompressed( | |
| } | ||
| } | ||
|
|
||
| /** Content-type lookup for static assets. */ | ||
| const CONTENT_TYPES: Record<string, string> = { | ||
| ".js": "application/javascript", | ||
| ".mjs": "application/javascript", | ||
| ".css": "text/css", | ||
| ".html": "text/html", | ||
| ".json": "application/json", | ||
| ".png": "image/png", | ||
| ".jpg": "image/jpeg", | ||
| ".jpeg": "image/jpeg", | ||
| ".gif": "image/gif", | ||
| ".svg": "image/svg+xml", | ||
| ".ico": "image/x-icon", | ||
| ".woff": "font/woff", | ||
| ".woff2": "font/woff2", | ||
| ".ttf": "font/ttf", | ||
| ".eot": "application/vnd.ms-fontobject", | ||
| ".webp": "image/webp", | ||
| ".avif": "image/avif", | ||
| ".map": "application/json", | ||
| }; | ||
| /** Content-type lookup for static assets using the shared MIME map. */ | ||
| function contentType(ext: string): string { | ||
| // ext comes from path.extname() so it has a leading dot (e.g. ".js") | ||
| return mimeType(ext.startsWith(".") ? ext.slice(1) : ext); | ||
| } | ||
|
|
||
| /** | ||
| * Try to serve a static file from the client build directory. | ||
|
|
@@ -247,7 +232,7 @@ function tryServeStatic( | |
| } | ||
|
|
||
| const ext = path.extname(staticFile); | ||
| const ct = CONTENT_TYPES[ext] ?? "application/octet-stream"; | ||
| const ct = contentType(ext); | ||
| const isHashed = pathname.startsWith("/assets/"); | ||
| const cacheControl = isHashed | ||
| ? "public, max-age=31536000, immutable" | ||
|
|
@@ -566,7 +551,7 @@ async function startAppRouterServer(options: AppRouterServerOptions) { | |
| // Block SVG and other unsafe content types by checking the file extension. | ||
| // SVG is only allowed when dangerouslyAllowSVG is enabled in next.config.js. | ||
| const ext = path.extname(params.imageUrl).toLowerCase(); | ||
| const ct = CONTENT_TYPES[ext] ?? "application/octet-stream"; | ||
| const ct = contentType(ext); | ||
| if (!isSafeImageContentType(ct, imageConfig?.dangerouslyAllowSVG)) { | ||
| res.writeHead(400); | ||
| res.end("The requested resource is not an allowed image type"); | ||
|
|
@@ -733,7 +718,7 @@ async function startPagesRouterServer(options: PagesRouterServerOptions) { | |
| // Block SVG and other unsafe content types. | ||
| // SVG is only allowed when dangerouslyAllowSVG is enabled. | ||
| const ext = path.extname(params.imageUrl).toLowerCase(); | ||
| const ct = CONTENT_TYPES[ext] ?? "application/octet-stream"; | ||
| const ct = contentType(ext); | ||
| if (!isSafeImageContentType(ct, pagesImageConfig?.dangerouslyAllowSVG)) { | ||
| res.writeHead(400); | ||
| res.end("The requested resource is not an allowed image type"); | ||
|
|
@@ -967,6 +952,12 @@ async function startPagesRouterServer(options: PagesRouterServerOptions) { | |
| } | ||
| resolvedUrl = rewritten; | ||
| resolvedPathname = rewritten.split("?")[0]; | ||
| // 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 (path.extname(resolvedPathname) && tryServeStatic(req, res, clientDir, resolvedPathname, compress)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good — using |
||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -984,6 +975,11 @@ async function startPagesRouterServer(options: PagesRouterServerOptions) { | |
| await sendWebResponse(proxyResponse, req, res, compress); | ||
| return; | ||
| } | ||
| // Check if fallback targets a static file in public/ | ||
| const fallbackPathname = fallbackRewrite.split("?")[0]; | ||
| if (path.extname(fallbackPathname) && tryServeStatic(req, res, clientDir, fallbackPathname, compress)) { | ||
| return; | ||
| } | ||
| response = await renderPage(webRequest, fallbackRewrite, ssrManifest); | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| <!DOCTYPE html> | ||
| <html> | ||
| <head><title>No Access</title></head> | ||
| <body>Access denied from nested static HTML</body> | ||
| </html> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| <!DOCTYPE html> | ||
| <html lang="en"> | ||
| <head><meta charset="UTF-8"><title>Static HTML Page</title></head> | ||
| <body><h1>Hello from static HTML</h1></body> | ||
| </html> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| <!DOCTYPE html> | ||
| <html> | ||
| <head><title>No Access</title></head> | ||
| <body>Access denied from nested static HTML</body> | ||
| </html> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| <!DOCTYPE html> | ||
| <html lang="en"> | ||
| <head><meta charset="UTF-8"><title>Static HTML Page</title></head> | ||
| <body><h1>Hello from static HTML</h1></body> | ||
| </html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path.resolve(root, "public")is computed forresolvedPublicDiron the line above, thenpath.resolve(root, "public", ...)is called again here instead of reusingresolvedPublicDir. Minor but easy to clean up:Same pattern applies to the fallback block at line 3031.