Repository navigation
fix(sirv): send the requested byte range instead of the whole file - #330
Conversation
`send()` computed the range offsets but never passed them to `createReadStream`, so a `Range` request answered with a partial `Content-Length`/`Content-Range` while writing the whole file to the socket. Node does not enforce the mismatch, so the surplus bytes stay in the connection buffer and the next response on a keep-alive connection is parsed out of them. Range parsing is rewritten around a single regex so that suffix ranges (`bytes=-10`), zero offsets (`bytes=0-0`) and unsatisfiable ranges are handled per RFC 9110, and so that ranges we do not implement (multi-range, non-`bytes` units, syntax errors) are ignored in favour of the full representation instead of being answered with a wrong 206. `Accept-Ranges: bytes` is now advertised on identity responses, and a `Range` over a precompressed variant is ignored rather than yielding an undecodable partial gzip/brotli stream.
🦋 Changeset detectedLatest commit: 43d6bca The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughSirv now supports single-byte HTTP Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SirvMiddleware
participant send
participant FileSystem
Client->>SirvMiddleware: GET file with Range header
SirvMiddleware->>send: build file response
send->>FileSystem: stream requested byte range
FileSystem-->>send: partial file bytes
send-->>Client: 206 response with range metadata
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/sirv/src/middleware.ts (1)
105-110: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value416 response drops previously computed headers (ETag, Last-Modified, Content-Type).
newHeaders(spread fromheadersat Line 98, containingETag/Last-Modified/Content-Type) is discarded here; the 416 response only ever carriesContent-Range. Many servers (e.g. Apache/nginx) preserve validators on 416 so a client can tell whether the resource changed without a full re-fetch. If you adopt this, make sure to still exclude the full-fileContent-LengththatnewHeaderscarries, since the 416 body isnull.♻️ Optional: preserve validators on 416, excluding stale Content-Length
if (range === "unsatisfiable") { + const { "Content-Length": _omit, ...preservedHeaders } = newHeaders; return new Response(null, { status: 416, - headers: { "Content-Range": `bytes */${stats.size}` }, + headers: { ...preservedHeaders, "Content-Range": `bytes */${stats.size}` }, }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sirv/src/middleware.ts` around lines 105 - 110, Update the range === "unsatisfiable" response to preserve the applicable headers from newHeaders, including ETag, Last-Modified, and Content-Type, while explicitly excluding its full-file Content-Length. Retain the 416 status and Content-Range bytes */${stats.size} header, and keep the null response body.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/sirv/src/middleware.ts`:
- Around line 105-110: Update the range === "unsatisfiable" response to preserve
the applicable headers from newHeaders, including ETag, Last-Modified, and
Content-Type, while explicitly excluding its full-file Content-Length. Retain
the 416 status and Content-Range bytes */${stats.size} header, and keep the null
response body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61c4097d-ad03-4450-b5ce-ae7781ce58d2
📒 Files selected for processing (5)
.changeset/tricky-moons-shake.mdpackages/sirv/src/middleware.tspackages/sirv/tests/helpers.tspackages/sirv/tests/ranges.spec.tspackages/sirv/tests/sirv.spec.ts
Problem
send()computed the range offsets into anoptsobject and then never passed it tocreateReadStream. EveryRangerequest therefore answered with a partialContent-LengthandContent-Rangewhile writing the entire file to the socket.Captured off the wire for
Range: bytes=0-10against a 124-byte file:All 124 bytes are on the wire under
content-length: 11. Node does not reject the mismatch, so the surplus 113 bytes stay in the connection buffer and the next response on a keep-alive connection is parsed out of them — Node's own client answersParse Error: Expected HTTP/, RTSP/ or ICE/. This affects any client that sendsRange: media seeking, PDF viewers, download resumption.The parsing was independently broken, because
parseInt(...) || fallbacktreats a valid0as absent andsplit("-")cannot express the grammar:bytes=-10(last 10 bytes)bytes 0-10/124bytes 114-123/124bytes=0-0(first byte)bytes 0-123/124— whole filebytes 0-0/124bytes=-0bytes=abc-defbytes=0-1,5-6items=0-10Fix
Range parsing moves into
parseRange(), built on a single regex, with a three-way result: a range,nullfor "valid but unsatisfiable" (416), orundefinedfor "must be ignored" (RFC 9110 §14.2 — a unit other thanbytes, a syntax error, or a multi-range request we do not answer). The resolved offsets are then actually passed tocreateReadStream.Two behaviour additions:
Accept-Ranges: bytesis now advertised on identity responses. Previously it was only set on a 206, so a client had no way to learn ranges were supported. This changes two existing assertions insirv.spec.ts, which used the absence ofAccept-Rangeson a plain 200 as a proxy for "no per-request header leaked"; they now assert the header is present while still assertingContent-Rangedoes not leak.Rangeover a precompressed variant is ignored. Slicing a.gz/.brwould hand the client a partial gzip/brotli stream it cannot decode, so those responses are served whole and omitAccept-Ranges.The lenient
bytes=2spelling (no trailing-) that the existing suite pins keeps working.Tests
packages/sirv/tests/ranges.spec.ts— 11 tests: body-matches-Content-Lengthfor start/middle ranges in bothdevand production modes, each parsing case in the table above,Accept-Rangeson a 200, and the precompressed-variant rule.The body assertions go through a new
sendRawhelper (node:http) rather thanfetch, becausefetchrejects on exactly theContent-Lengthmismatch under test and would hide it.pnpm testinpackages/sirv: 87 passed. Typecheck and Biome clean.Summary by CodeRabbit
416response.