fix(dev): preserve public asset conditional requests - #2711
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: e5271927a5
ℹ️ 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 timestamp = Date.parse(trimmed); | ||
| return Number.isFinite(timestamp) && Math.floor(mtimeMs / 1000) * 1000 <= timestamp; |
There was a problem hiding this comment.
If-Range only permits an HTTP-date, but Date.parse() also accepts formats such as 12/31/2099 and 2099-12-31. A client resuming an older download with such an invalid future validator will therefore receive 206 bytes from the current file instead of the required full 200 response, potentially combining bytes from different file versions. Validate the HTTP-date grammar before comparing the timestamp.
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 |
3988cd5 to
4db2e69
Compare
# Conflicts: # packages/vinext/src/server/prod-server.ts
|
/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
Overall this is a well-structured, thoroughly tested implementation of RFC 9110 byte ranges for the Node production static server. The parseByteRange / ifRangeAllowsRange / parseHttpDate helpers are cleanly separated and well unit-tested, and dev/prod parity is handled on both the startup-cache and filesystem-fallback paths.
The Codex bot comment is already addressed (stale)
The automated Codex review (against e5271927a5) flagged that If-Range might honor non-HTTP dates like 12/31/2099 via Date.parse(). The current head resolves this: ifRangeAllowsRange routes through the strict parseHttpDate grammar (http-date.ts), and tests/http-range.test.ts explicitly asserts 12/31/2099 and 2099-12-31 return false. No action needed there.
Correctness spot-checks (all pass)
- 304 (If-None-Match) is evaluated before the range branch, so a conditional GET with a matching validator returns 304 not 206 — matches RFC precedence and is covered by tests.
- Ranges are served from the identity representation while
Vary: Accept-Encodingis preserved; weak ETags correctly causeIf-Rangeto fail closed (full 200), avoiding cross-representation byte mixing. resolveRequestedRangedisables ranges for any non-200 base status, so error/custom responses can't be partial.- Large wire integers are handled losslessly via the
"overflow"sentinel.
Main concern: scope
The PR title/body is about byte ranges, but the single largest change is dev-public-etag.ts (551 lines) plus its two test files (~700 lines) — a complex dev-server public-file ETag folding subsystem (case/normalization-insensitive filesystem aliasing, symlink cycle handling, Unicode case expansion). This is only loosely related to byte-range support (shared matchesIfNoneMatch / Last-Modified plumbing) and materially increases review surface and long-term maintenance cost. Consider splitting it into its own PR so each change can be reviewed and reverted independently. See inline notes below.
|
|
||
| const simpleCasePatterns = new Map<string, RegExp>(); | ||
|
|
||
| export function createDevPublicFileEtags( |
There was a problem hiding this comment.
This 551-line module (dev public-file ETag folding across case/normalization-insensitive filesystems, symlink cycle detection, Unicode case-expansion aliasing) is essentially a separate feature from static byte ranges. It's the bulk of the diff yet unrelated to the PR's stated purpose. Recommend extracting it into its own PR — it would get more focused review and keeps the byte-range change small and independently revertable.
| else node.realPath = null; | ||
| } | ||
| if (redirect) { | ||
| if (node.redirect === undefined || node.redirect === redirect) node.redirect = redirect; |
There was a problem hiding this comment.
Likely a copy/paste bug: the redirect conflict branch sets node.realPath = null instead of node.redirect. When two different redirects collide this poisons realPath (which may be unset) rather than marking the redirect as ambiguous, so node.redirect retains the first value and can still be followed. Compare with the realPath branch just above (line 270-271) which correctly nulls its own field.
| if (node.redirect === undefined || node.redirect === redirect) node.redirect = redirect; | |
| if (redirect) { | |
| if (node.redirect === undefined || node.redirect === redirect) node.redirect = redirect; | |
| else node.redirect = undefined; | |
| } |
| ); | ||
| const fiftyYearsFromNow = new Date(now); | ||
| fiftyYearsFromNow.setUTCFullYear(now.getUTCFullYear() + 50); | ||
| if (candidateTimestamp > fiftyYearsFromNow.getTime()) year -= 100; |
There was a problem hiding this comment.
Minor: when the RFC 850 date has an invalid calendar value (e.g. 31-Feb), candidateTimestamp is NaN, so NaN > fiftyYearsFromNow is false and the year is left unadjusted. The final timestampFromHttpDateParts still returns NaN via its own validation, so the result is correct — but the two-pass structure is subtle. A short comment noting that the candidate pass is only a year-boundary probe (and that invalid dates are rejected by the second pass) would help future readers.
|
Review submitted successfully. Here's a summary of what I found and posted to PR #2711: Review summaryOverall: A well-structured, thoroughly tested RFC 9110 byte-range implementation. The Findings posted:
Correctness spot-checks (all pass): 304 precedence over 206, identity-representation range serving with weak-ETag fail-closed, ranges disabled for non-200 base statuses, and lossless large-integer handling. Ran the |
# Conflicts: # packages/vinext/src/server/prod-server.ts # tests/http-conditional.test.ts
# Conflicts: # packages/vinext/src/server/http-date.ts # packages/vinext/src/server/http-range.ts # packages/vinext/src/server/prod-server.ts # tests/app-router-production-server.test.ts # tests/http-date.test.ts # tests/http-range.test.ts # tests/serve-static.test.ts
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review
I reviewed the actual diff against main (not the branch commit history), and there is a significant mismatch between what this PR claims to do and what it changes.
🚩 Blocking: PR title/description do not match the diff
The PR is titled "feat(server): support static asset byte ranges" and the body describes byte-range parsing in detail (parseByteRange, 206 Partial Content, 416 Range Not Satisfiable, If-Range, http-range.ts, http-date.ts, Accept-Ranges, etc.).
But the diff against main contains zero byte-range code. git diff origin/main...HEAD --stat is:
packages/vinext/src/index.ts | 42 ++
packages/vinext/src/server/dev-public-etag.ts | 551 ++++++++++++++++++++++++++
tests/dev-public-etag-vite.test.ts | 363 ++++++++
tests/dev-public-etag.test.ts | 390 ++++++++++
tests/e2e/pages-router/script.spec.ts | 37 ++
The byte-range files the description references (http-range.ts, http-date.ts) exist in the tree but are not part of this PR — they were merged separately via #2710 / #2713 / #2715. The branch's first commit is e5271927 feat(server): support static asset byte ranges, but that work already landed on main through other PRs. What is left unmerged here is entirely the dev-server public-file ETag folding subsystem.
This matters concretely for this repo:
- Per
AGENTS.md, changesets are auto-generated from the Conventional Commit / PR title. Merging this as-is produces a changelog entryfeat(server): support static asset byte rangesfor a diff that adds no byte-range code. The changelog will be wrong. - Anyone reviewing based on the description reviews the wrong feature.
Action: Retitle to describe the real change (e.g. feat(dev): weak/case-folded If-None-Match for public assets) and rewrite the body. Given the repo's stacked-PR guidance for high-risk work, this 551-line dev subsystem should also stand on its own PR.
Scope / complexity
The net change is a large dev-only subsystem: Unicode case-folding, NFD/NFC normalization-insensitive aliasing, symlink cycle detection, and an 8-candidate bucket cap — all to pre-resolve a client If-None-Match into sirv's exact weak-ETag string so its 304 path fires. It's dev-only and well tested, but it's a lot of surface area and long-term maintenance for a 304 optimization. Worth confirming this complexity is warranted vs. a narrower approach.
Correctness spot-checks (pass)
- The previously-flagged copy/paste bug (
node.realPath = nullin the redirect branch) is already fixed atdev-public-etag.ts:273-276— the redirect branch correctly nullsnode.redirect. Stale. - Ran
tests/dev-public-etag.test.tslocally: 12 passed, 4 skipped (platform-gated).vp checkclean on both source files. - Security boundary looks OK:
resolveDevPublicIfNoneMatchonly ever returns an ETag string already present inetagsByRealPath(indexed from within the public tree) and never serves content, so even a mis-resolution yields at worst a spurious 304, not a content leak.
Minor inline notes below.
| const leftTokens = expandCaseTokens(normalizedLeft); | ||
| return ( | ||
| leftTokens.length === candidate.expandedCaseTokens.length && | ||
| leftTokens.every((token, index) => |
There was a problem hiding this comment.
The .every((token, index) => ...) callback parameter index shadows the outer index: DevPublicFileEtagIndex. It works today only because the outer index isn't referenced inside the callback, but shadowing a load-bearing name like this is a maintenance hazard — a future edit that needs the outer index here would silently pick up the array index. Consider renaming the callback param:
| leftTokens.every((token, index) => | |
| leftTokens.every((token, tokenIndex) => | |
| simpleCaseEquivalent(token, candidate.expandedCaseTokens[tokenIndex]!), |
| segment: string; | ||
| node: DevPublicPathNode; | ||
| simpleCasePattern: RegExp; | ||
| expandedCaseTokens: string[]; |
There was a problem hiding this comment.
simpleCasePatterns is a module-level Map that is never evicted, so every distinct case token seen across the process lifetime accumulates a compiled RegExp. In dev this is effectively bounded by the number of distinct public path segments, so it's likely fine, but a module-global unbounded cache is worth a brief comment noting the bound (or scoping it to the index) so it isn't mistaken for a leak later.
|
Review submitted successfully to PR #2711. SummaryThe most important finding is a title/description vs. diff mismatch that the prior automated reviews missed because they reviewed the branch's commit history rather than the actual diff against Blocking issueThe PR is titled "feat(server): support static asset byte ranges" and its body describes byte-range parsing in detail, but the actual diff against This matters because, per Other notes
Confirmed passing
|
james-elicx
changed the title
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review
Reviewed the actual diff against main. This is a well-structured, thoroughly tested dev-only fix: it pre-resolves a client If-None-Match into sirv's exact weak-ETag string so Vite's public-file 304 path fires for equivalent strong/weak/list validators and for filesystem aliases (case, Unicode normalization, symlinks). Ran tests/dev-public-etag.test.ts locally (12 passed, 4 platform-skipped) and vp check on the source file — clean.
Title/description mismatch is now resolved
The PR is retitled fix(dev): preserve public asset conditional requests and the body now describes the dev public-asset fix, matching the diff. The changelog-correctness blocker raised in earlier reviews (byte-range title over a non-byte-range diff) no longer applies — the byte-range work landed via #2710/#2713/#2715 and is not in this diff. Good.
Correctness spot-checks (pass)
- The previously-flagged copy/paste bug (
node.realPath = nullin the redirect branch) is fixed atdev-public-etag.ts:273-276— the redirect branch correctly nullsnode.redirect. Stale. - Security boundary is sound:
resolveDevPublicIfNoneMatchonly ever returns an ETag already indexed from within the public tree and never serves content, so a mis-resolution yields at worst a spurious 304, never a content leak. Ambiguous case/normalization aliases and redirect collisions fail closed (returnundefined), verified by thefails ... closedtests. - Weak/strong/list/wildcard
If-None-Matchnormalization routes through the shared RFC 9110matchesIfNoneMatch, and non-GET/HEAD methods, missing files, and out-of-basePath requests are left untouched. viteUsesStatLookupis detected once at startup and preserved across rebuilds, mirroring Vite's own one-time stat-lookup decision (covered by the "keeps Vite's exact/stat lookup mode" tests).
Scope / maintenance note
The net change is a large dev-only subsystem (Unicode case-folding, NFD/NFC normalization-insensitive aliasing, symlink-cycle detection, an 8-candidate bucket cap) for what is ultimately a 304 optimization in development. It's well-tested and fails closed, but it's a lot of surface area and long-term maintenance. Per the repo's stacked-PR guidance for high-risk work, this could stand as its own focused PR (it already is, effectively). Worth confirming the complexity is warranted vs. a narrower approach that only folds the common exact-spelling case.
Minor inline notes below — none blocking.
| const leftTokens = expandCaseTokens(normalizedLeft); | ||
| return ( | ||
| leftTokens.length === candidate.expandedCaseTokens.length && | ||
| leftTokens.every((token, index) => |
There was a problem hiding this comment.
The .every((token, index) => ...) callback parameter index shadows the outer index: DevPublicFileEtagIndex. It's harmless today only because the outer index isn't referenced inside the callback, but shadowing a load-bearing name like this is a maintenance hazard — a future edit needing the outer index here would silently bind to the array index instead. Rename the callback param:
| leftTokens.every((token, index) => | |
| leftTokens.every((token, tokenIndex) => | |
| simpleCaseEquivalent(token, candidate.expandedCaseTokens[tokenIndex]!), |
| expandedCaseTokens: string[]; | ||
| }; | ||
|
|
||
| const simpleCasePatterns = new Map<string, RegExp>(); |
There was a problem hiding this comment.
simpleCasePatterns is a module-level Map that is never evicted, so every distinct case token seen across the process lifetime accumulates a compiled RegExp. In dev this is effectively bounded by the number of distinct public path segment characters, so it's fine in practice — but a module-global unbounded cache is worth a one-line comment noting that bound (or scoping it to the index) so a future reader doesn't mistake it for a leak.
| // Vite converts native separators on Windows before applying POSIX path | ||
| // normalization. `toSlash` is platform-gated, so a literal backslash | ||
| // remains a valid filename character on POSIX. | ||
| pathname = normalizePath(toSlash(decodeURI(pathname))); |
There was a problem hiding this comment.
This uses decodeURI rather than Next.js's segment-wise decodePathParams/decodeURIComponent, so %2F etc. stay encoded and the structural .. in %2e%2e gets resolved by normalizePath. That appears to intentionally mirror sirv's own decode behavior (and the Vite integration tests exercise the real decode path end-to-end), so this is likely correct — flagging only so a future reader knows the divergence from the request-pipeline decoder is deliberate and parity-driven, not an oversight.
|
Review submitted successfully to PR #2711. SummaryI reviewed the actual diff against Key conclusions
Minor inline notes (non-blocking)
Scope noteThe net change is a large dev-only case-folding/normalization/symlink subsystem for a 304 optimization. Well-tested and fails closed, but worth confirming the complexity is warranted vs. a narrower exact-spelling approach. |
2 participants
What changed
Why
Vite public-file serving delegates to sirv, whose development ETags use file size and mtime. Equivalent request validators and filesystem aliases could miss sirv exact identity and return 200 instead of 304. This normalizes only matching GET and HEAD validators to the exact indexed weak ETag while leaving non-matching requests untouched.
Scope note
This branch originally implemented static byte ranges. That work landed independently through #2715 while the branch was being prepared. The current diff against main is only the development public-asset conditional-request fix described above.
Validation