fix(server): reject unsupported static asset methods - #2714
Conversation
commit: |
![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: fc914369bc
ℹ️ 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".
|
|
||
| const resolved = await resolveStaticFile(staticFile); | ||
| if (!resolved) return false; | ||
| if (rejectUnsupportedStaticMethod(req, res)) return true; |
There was a problem hiding this comment.
For production requests such as POST /robots.txt, this new uncached-path check is never reached: the Pages Router's serveFilesystemRoute callback returns false for every non-GET/HEAD request at prod-server.ts:2001, while the App Router's resolvePublicFileRoute does the same before producing a static-file signal. Consequently, real public files still fall through to page routing and usually return 404 rather than the intended 405; the added uncached test only calls this private helper directly and does not exercise either request pipeline. Remove or revise those upstream method gates so existing public files reach this check while missing files still fall through.
AGENTS.md reference: AGENTS.md:L183-L192
Useful? React with 👍 / 👎.
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: reject unsupported static asset methods
Solid, well-tested change with good dev/prod/Worker parity coverage. I verified the four request pipelines (App RSC handler, Pages pipeline, Node prod-server, Worker fetchWorkerFilesystemRoute) and ran the targeted unit suites locally — all pass (248 + 8 tests green).
Codex's P2 concern is resolved
The earlier Codex review (against fc914369bc) flagged that the prod-server serveFilesystemRoute method gate blocked existing public files from reaching the 405 path. In the current commit that gate has been removed (prod-server.ts:2014-2022 no longer checks req.method), so tryServeStatic reaches rejectUnsupportedStaticMethod for mutations on existing files, and missing files still return false and fall through to routing. The resolvePublicFileRoute change (match-first, then reject) fixes the App Router path symmetrically. The added integration tests (pages-router.test.ts, app-router-production-server.test.ts) exercise the real pipelines, not just the private helper. This concern looks fully addressed.
Observations (non-blocking)
-
Scope is broader than the title.
sanitizeMethodNotAllowedHeadersruns for everymethodNotAllowedResponsecaller, including the static-page/SSG 405 responses inapp-page-method.tsandpages-page-method.ts. Previously those responses set noContent-Type; now they always gettext/plain; charset=utf-8and strip stale body headers. This is an improvement (prevents a middleware Content-Type leaking into a 405) and I found no test asserting the old absence, but it's a wider behavioral change than "static asset methods" implies — worth a line in the changelog/commit body. -
App Router dev is not covered. In App Router dev, requests with a
.in the path fall through to Vite's built-in static middleware (index.ts:4770) before the RSC handler runs, so aPOSTto an existing App Router public file in dev is still served by Vite for all methods rather than returning 405. The PR body lists "Pages dev" but not "App dev"; if that's a deliberate gap it would be good to state it explicitly (the Pages-dev intercept atindex.ts:4626is gated onhasPagesDir).
Nice work on the path-traversal guard in isExistingDevPublicFile and the bodyless HEAD probe in the Worker path.
| // then converted to the framework's deterministic 405 response. | ||
| const assetRequest = isRetrievalMethod | ||
| ? new Request(assetUrl, request) | ||
| : new Request(assetUrl, { method: "HEAD", headers: request.headers }); |
There was a problem hiding this comment.
Minor: the bodyless HEAD probe still forwards the original request's headers, which for a POST may include content-length / content-type describing a body that the HEAD request no longer carries. It's benign for the CF asset binding (which only needs the path), but forwarding a content-length on a bodyless HEAD is a little surprising. Consider dropping body-describing headers from the probe for clarity:
| : new Request(assetUrl, { method: "HEAD", headers: request.headers }); | |
| const assetRequest = isRetrievalMethod | |
| ? new Request(assetUrl, request) | |
| : new Request(assetUrl, { method: "HEAD" }); |
If any of the forwarded headers (e.g. Accept-Encoding, If-None-Match) are actually needed for the existence probe, keep them explicitly rather than passing the whole set.
| if (!hasBasePath(pathname, basePath)) return next(); | ||
| pathname = stripBasePath(pathname, basePath); | ||
| } | ||
| if (!isExistingDevPublicFile(pathname)) return next(); |
There was a problem hiding this comment.
This dev intercept is gated on hasPagesDir, so App Router-only dev apps don't get 405s on public-file mutations (the request falls through to Vite's static middleware — see the .-in-path skip at line 4770). That matches the PR body listing "Pages dev" but omitting "App dev". Worth an inline comment noting the App-dev gap so a future reader doesn't assume parity, or a follow-up issue.
|
Review posted successfully to PR #2714. SummaryI reviewed PR #2714 ( Verdict: Solid, well-tested change. No blocking issues. Key findings posted as a batch review:
I praised the path-traversal guard in |
|
/bigbonk review for issues |
|
Now let me examine the path-traversal guard closely. One concern: if Let me verify the decoding: |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: reject unsupported static asset methods
Independent re-review. I verified all four request pipelines (App RSC / Pages pipeline wrapper / Node prod-server / Worker fetchWorkerFilesystemRoute) and ran the targeted suites locally — all green (http-error-responses + request-pipeline + pages-request-pipeline + serve-static = 248 passed; deploy fetchWorkerFilesystemRoute = 8 passed). vp check on the four changed source files is clean.
Verdict: correct and well-tested. No blocking issues. The design (establish filesystem match, then reject unsupported methods, after middleware) matches the documented Next.js ordering and is applied symmetrically across pipelines.
Verified correct
- 405/status merge is right.
runPagesRequestpassesundefinedfor the status override when it detects a static 405 (pages-request-pipeline.ts:363-372), so a middleware-set status (e.g. the test's 404) can't clobber the 405.mergeHeadersstill folds middleware/security headers andSet-Cookiein, thensanitizeMethodNotAllowedHeadersre-strips stale body headers. Good. - Existence-first ordering in
tryServeStatic— therejectUnsupportedStaticMethodcall sits afterif (!entry) return false/if (!resolved) return false, so missing files fall through to routing and only real files 405. Matches the PR's Next.js-parity claim. - Path-traversal guard in
isExistingDevPublicFileis sound (completing the analysis a prior run left unfinished):normalizePathnameForRouteMatchStrictdecodes each/-split segment, so%2e%2ebecomes a literal..segment;path.resolve(devPublicDir, ".${pathname}")then collapses it, and thestartsWith(\${devPublicDir}/`)check rejects anything that escapes the public dir. Root (/) resolves todevPublicDiritself, fails the/-suffixed prefix check, and returns false.fs.statSync(...).isFile()` further ensures only real files match. No escape. publicDir: falseis handled (devPublicDir === nullshort-circuits) and covered by a test.- Worker bodyless HEAD probe never forwards the mutating method or body, and cancels the discarded probe body — nicely done.
Observations (non-blocking)
The first two echo earlier bonk/Codex notes and remain worth a line in the commit body; the third is a fresh observation.
-
Scope is broader than the title.
sanitizeMethodNotAllowedHeadersnow runs for everymethodNotAllowedResponsecaller, including the static-page/SSG 405s inapp-page-method.ts:60andpages-page-method.ts:62. Those now always emitContent-Type: text/plain; charset=utf-8and drop body headers instead of inheriting middleware'sContent-Type. This is an improvement (no more middleware Content-Type leaking onto a 405 body) and no existing test asserted the old absence — but it's wider than "static asset methods" and deserves a mention in the changelog/commit body. -
App Router dev is a deliberate gap. The pre-Vite dev intercept (
index.ts:4626) is gated onhasPagesDir, so an App-Router-only dev app still lets Vite's static middleware serve public-file mutations for all methods. App Router production is covered (viaresolvePublicFileRoute), so this is dev-only. Worth an inline comment or follow-up issue so a future reader doesn't assume parity. -
/_next/imageand App-Router build assets now short-circuit to 405 for mutations. BecauserejectUnsupportedStaticMethodlives insidetryServeStatic, aPOSTto an existing/_next/static/*asset (prod-server.ts:1459) or to/_next/image?url=...for an existing source image (prod-server.ts:1494) now returns 405 beforemissingBuildAssetfallthrough / middleware. This is consistent with the PR's "match first, then reject" design and with Next.js (the image optimizer only handles GET), so I believe it's correct — but it's a slightly wider surface than the "public/ files" framing in the PR body, and there's no dedicated test for the/_next/imagemutation case. Consider adding one to lock the behavior in.
Nice work overall — the four-pipeline parity and the traversal/HEAD-probe details are handled carefully.
| if (!hasBasePath(pathname, basePath)) return next(); | ||
| pathname = stripBasePath(pathname, basePath); | ||
| } | ||
| if (!isExistingDevPublicFile(pathname)) return next(); |
There was a problem hiding this comment.
This dev intercept is gated on hasPagesDir, so App-Router-only dev apps don't get 405s on public-file mutations — the request falls through to Vite's static middleware and is served for all methods. App Router production is covered via resolvePublicFileRoute, so this is a dev-only gap. Worth an inline comment noting it so a future reader doesn't assume dev/App parity, or a follow-up issue.
| // then converted to the framework's deterministic 405 response. | ||
| const assetRequest = isRetrievalMethod | ||
| ? new Request(assetUrl, request) | ||
| : new Request(assetUrl, { method: "HEAD", headers: request.headers }); |
There was a problem hiding this comment.
Minor: the bodyless HEAD probe forwards the original request's full header set, which for a POST/PUT includes content-length/content-type describing a body the HEAD request no longer carries. Benign for the CF asset binding (it only needs the path), but forwarding a content-length on a bodyless HEAD is surprising. Consider dropping body-describing headers:
| : new Request(assetUrl, { method: "HEAD", headers: request.headers }); | |
| const assetRequest = isRetrievalMethod | |
| ? new Request(assetUrl, request) | |
| : new Request(assetUrl, { method: "HEAD" }); |
If a specific header (If-None-Match, Accept-Encoding) is genuinely needed for the existence probe, copy it explicitly rather than the whole set.
|
Review posted successfully to PR #2714. SummaryI did an independent re-review of PR #2714 ( Verdict: correct and well-tested. No blocking issues. Key points in the posted review: Verified correct:
Non-blocking observations:
Two inline suggestions: the App-dev gap comment and the bodyless-HEAD header-forwarding cleanup. |
# Conflicts: # packages/vinext/src/server/prod-server.ts
# Conflicts: # packages/vinext/src/server/prod-server.ts
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: reject unsupported static asset methods
Correct and well-tested change. I verified the four request pipelines (App RSC, Pages pipeline wrapper, Node prod-server, Worker fetchWorkerFilesystemRoute) and ran the targeted suites locally — all green (264 passed across http-error-responses + request-pipeline + pages-request-pipeline + serve-static).
Verdict: no correctness blockers. The design (establish the filesystem match, then reject unsupported methods, after middleware) matches the documented Next.js ordering and is applied symmetrically across pipelines. Existence-first ordering in tryServeStatic (if (!entry) return false before the method check) and resolvePublicFileRoute (match before reject) is correct, so misses still fall through to routing. The path-traversal guard in isExistingDevPublicFile is sound (encoded .. collapses to a literal segment, path.resolve + startsWith("${devPublicDir}/") rejects escapes, .isFile() gates on real files), and the Worker bodyless HEAD probe never forwards the mutating method or body.
Performance: new ASSETS probe on every Worker mutation (worth a look)
The change to fetchWorkerFilesystemRoute's phase gate is broader than static-asset handling. Previously phase === "direct" short-circuited to false for all methods — so a POST/PUT/DELETE at the direct phase never touched the asset binding. Now it only short-circuits for retrieval methods, so every non-GET/HEAD request to a non-/api path performs an extra env.ASSETS.fetch() HEAD probe at step 8b (pages-request-pipeline.ts:553, which runs for every request post-middleware) before falling through to page/API routing.
That means a POST /checkout, form submission to a page route, etc. now always incurs an extra ASSETS round-trip that returns 404 before continuing. AGENTS.md's performance guidance is explicit about keeping the common request path lightweight and gating feature-specific work behind cheap request/route metadata. Consider whether the existence probe can be gated on cheaper metadata first (e.g. the known public-file set / a .-in-path heuristic, mirroring the dev handler's isFilePathRequest gate) so page-route mutations skip the ASSETS probe entirely. The dev path (index.ts:5427) has the same shape but uses a local fs.statSync, which is cheaper but still runs on every mutation.
Non-blocking observations (echoing prior reviews)
- Scope is broader than the title.
sanitizeMethodNotAllowedHeadersnow runs for everymethodNotAllowedResponsecaller, including static-page/SSG 405s inapp-page-method.ts/pages-page-method.ts. Those now always emitContent-Type: text/plain; charset=utf-8and drop body headers. This is an improvement, but wider than "static asset methods" — worth a line in the commit body / changelog. - App Router dev is a deliberate gap. The pre-Vite intercept (
index.ts:4626) is gated onhasPagesDir, so App-Router-only dev apps still let Vite's static middleware serve public-file mutations for all methods. Prod is covered viaresolvePublicFileRoute. Worth an inline comment or follow-up so a future reader doesn't assume dev/App parity. /_next/imageand/_next/static/*mutations now short-circuit to 405. Because the reject lives insidetryServeStatic, a POST to an existing build asset now 405s. Consistent with Next.js (image optimizer is GET-only) and the design, but there's no dedicated/_next/imagemutation test to lock it in.
Nice work on the four-pipeline parity and the traversal/HEAD-probe details.
| if ( | ||
| phase === "direct" || | ||
| (request.method !== "GET" && request.method !== "HEAD") || | ||
| (phase === "direct" && isRetrievalMethod) || |
There was a problem hiding this comment.
This phase gate is now much wider on the mutation path. Before, phase === "direct" short-circuited to false for every method, so a POST/PUT/DELETE never touched the asset binding. Now every non-GET/HEAD request to a non-/api path reaches the HEAD probe below at the direct phase (serveFilesystemRoute(pathname, "direct") at line 553 runs for every post-middleware request), so a POST /checkout to a page route incurs an extra env.ASSETS.fetch() round-trip that returns 404 before routing continues.
Per AGENTS.md's performance guidance (keep the common request path lightweight; gate feature-specific work behind cheap request/route metadata), consider gating the existence probe on cheaper metadata first — e.g. only probe when the path could plausibly be a public file (a .-in-path heuristic mirroring the dev handler's isFilePathRequest, or the known public-file set) — so page-route mutations skip the ASSETS probe entirely.
| if (!hasBasePath(pathname, basePath)) return next(); | ||
| pathname = stripBasePath(pathname, basePath); | ||
| } | ||
| if (!isExistingDevPublicFile(pathname)) return next(); |
There was a problem hiding this comment.
This dev intercept is gated on hasPagesDir, so App-Router-only dev apps don't get 405s on public-file mutations — the request falls through to Vite's static middleware and is served for all methods. App Router production is covered via resolvePublicFileRoute, so this is a dev-only gap. Worth an inline comment noting it so a future reader doesn't assume dev/App parity, or a follow-up issue.
| // then converted to the framework's deterministic 405 response. | ||
| const assetRequest = isRetrievalMethod | ||
| ? new Request(assetUrl, request) | ||
| : new Request(assetUrl, { method: "HEAD", headers: request.headers }); |
There was a problem hiding this comment.
The bodyless HEAD probe forwards the original request's full header set, which for a POST/PUT includes content-length/content-type describing a body the HEAD request no longer carries. Benign for the CF asset binding (it only needs the path), but forwarding a content-length on a bodyless HEAD is surprising. Consider dropping body-describing headers:
| : new Request(assetUrl, { method: "HEAD", headers: request.headers }); | |
| const assetRequest = isRetrievalMethod | |
| ? new Request(assetUrl, request) | |
| : new Request(assetUrl, { method: "HEAD" }); |
If a specific header (If-None-Match, Accept-Encoding) is genuinely needed for the existence probe, copy it explicitly rather than the whole set.
|
Review posted successfully to PR #2714. SummaryI did an independent review of PR #2714 ( Verdict: no correctness blockers. The design is sound and well-tested, with correct existence-first ordering across pipelines, a sound path-traversal guard, and a proper bodyless HEAD probe. Fresh finding (the main one)New ASSETS probe on every Worker mutation. The Non-blocking (echoing prior reviews)
Two inline suggestions accompany the summary: the App-dev gap comment and the bodyless-HEAD header-forwarding cleanup, plus the performance flag on the phase gate. |
2 participants
Summary
Allow: GET, HEADonly when an existing static asset receives a method other than GET or HEADpublicDir: falsein developmentNext.js parity
Next.js establishes the filesystem match before rejecting unsupported methods. Vinext now does the same: real static files return 405, while missing paths continue through page/API/dynamic routing. Middleware and config routing still run before public-file handling.
Cloudflare default asset-first routing can answer a direct existing-asset mutation before the vinext Worker runs; that platform response is already 405 but does not expose vinext's canonical body/headers. The vinext handling in this PR applies when the Worker runs first and when the Worker resolves rewritten/static targets itself.
Test plan
vp checkon all changed source and testsvp test run tests/http-error-responses.test.ts tests/request-pipeline.test.ts tests/pages-request-pipeline.test.ts tests/serve-static.test.ts tests/deploy.test.ts(552 passed)publicDir: false(2 passed)