Bun.serve: honor If-Range on file, directory and handler file responses - #43571
Conversation
A Range request with an If-Range validator that does not match the response got a 206 anyway. A client that resumes a download after the file changed on disk then joins bytes of two versions. RangeRequest::from_request now takes the validators of the representation and ignores Range when If-Range does not match them (RFC 9110 section 13.1.5). An entity-tag uses the strong comparison. An HTTP-date must equal Last-Modified. FileRoute and DirectoryRoute pass their ETag and Last-Modified. RequestContext keeps the If-Range header and compares it in do_sendfile with the ETag and Last-Modified that the handler set on its Response.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for one more day.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for one more day. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 7 minutes for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (8)
Comment |
|
Updated 8:12 PM PT - Sep 19th, 2026
✅ @robobun, your commit e980c42ad7cdf4cf334757bd854eb787cbb43195 passed in 🧪 To try this PR locally: bunx bun-pr 43571That installs a local version of the PR into your bun-43571 --bun |
|
Status Reproduced on 1.4.3 (canary With this PR all three answer Fail-before: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked whether the new if_range: Option<Box<[u8]>> on RequestContext is released on every lifecycle exit — deinit never touches it, but release_request_context hands the slot to HiveArray::put, which runs drop_in_place on the whole struct (and the heap-fallback path goes through heap::destroy), so the Box is freed on both pool paths. The weak-tag-never-matches behavior on { dir } routes and the DQUOTE-in-first-3-bytes classification were also looked at and are consistent with RFC 9110 §8.8.3.2 given uWS delivers header values already trimmed.
Extended reasoning...
Three confirmed findings are already attached inline (asctime/TZ-dependent parse_http_date on client bytes, the new Serve::Done hang trigger via if-range in FileRoute::serve, and second-granularity Last-Modified comparison on { dir } routes), so a human look is already signalled. This note only records the additional concern I ruled out from reading the code: RequestContext::deinit takes &self and explicitly clears other owned buffers but not if_range, which would look like a leak; however the context is returned via server.release_request_context -> HiveArray::put, which calls core::ptr::drop_in_place on the slot (and bun_core::heap::destroy for heap-fallback contexts), so the Box<[u8]> is dropped exactly once on both paths. memory_cost also counts it. Not approving given the open findings.
Date.parse reads a date with no zone, such as the obsolete asctime form, in the local time of the server. The result of the comparison with Last-Modified then depends on the time zone of the server. A date with no zone now never matches, so the response is the full body.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I re-reviewed the latest push and found no new bugs; the GMT-suffix commit addresses the time-zone point from the earlier round. Because this changes response semantics on every Range request across three serving paths and adds a header lookup to RequestContext::create, a human look is still worthwhile.
What was reviewed this round:
- The new owning
if_range: Option<Box<[u8]>>onRequestContext: freed on both release paths (HiveArray::putrunsdrop_in_place; the heap fallback runsdestroy), and only allocated whenRangeparsed to something usable. if_range_matcheson adversarial values: empty / short header is safe (len().min(3)slice), weak tag on either side and absentETag/Last-Modifiedall fall to the full-body branch; no unwrap on client bytes.- Handler path (
if_range_matches_response) reads validators from the handler's own response headers only, so a barenew Response(Bun.file())never emits a 206 against anIf-Range, matching the tests.
Extended reasoning...
Overview
The PR adds RFC 9110 §13.1.5 If-Range evaluation to the three places Bun.serve honors Range: FileRoute::serve, DirectoryRoute::on, and RequestContext::do_sendfile for handler-returned Response(Bun.file(...)). RangeRequest::from_request grows etag/last_modified_ms parameters and returns Result::None (full 200 body, no 416) when the validator fails; ETag::if_range does strong comparison; RequestContext gains an owned copy of the If-Range header for the async handler case. Tests cover the file route, sync and async handlers, directory routes, HEAD, weak tags, the three date forms, 416 suppression, and a resume-after-rewrite scenario. Since the prior review, one commit (883db62) restricts the HTTP-date path to values ending in GMT, which closes the asctime/local-timezone mismatch that was raised inline last round.
Security risks
if_range_matches operates on client-controlled bytes. I checked the slicing (&if_range[..if_range.len().min(3)]) for empty and 1-2 byte inputs, the ends_with(b"GMT") guard, and that parse_http_date returning None falls to "no match" rather than panicking. The entity-tag path reuses the existing ETag::parse. On the handler path, the ETag header is copied via to_utf8().into_owned() before use, so no borrowed JSC string escapes. Nothing here changes authentication, path resolution, or what file is served; the worst outcome of a wrong comparison is a 200 instead of a 206 or vice versa. No security concern beyond ordinary input hygiene, which looks sound.
Level of scrutiny
Moderate. It is a ~100-line native change to the HTTP server, including RequestContext::create, which runs for every request; the added cost for requests without Range is one already-existing raw_from_request call plus a branch. The new owning field is the main memory-safety question: I traced release_request_context to HiveArray::put, which calls drop_in_place, and to the heap fallback destroy, so the Box<[u8]> is freed with the rest of the context and is also counted in memory_cost. The FileRoute change reorders the etag/last-modified computation before range resolution and adds if-range as a trigger for the fallible last_modified_date(); that extends an existing Err → Serve::Done pattern that was already raised inline in the previous round and is not repeated here.
Other factors
The exit reason was dry_streak, so the hunt ran to exhaustion rather than being cut off. The test matrix is reasonably complete for the behavior, including negative cases (weak tag on either side, date before/after, no validators on the response, If-Range without Range). I did not approve because the change alters observable HTTP behavior on a hot path across three code paths, one earlier inline thread of mine has no corresponding code change in this push, and the PR relies on Date.parse semantics via parse_http_date that a concurrently open PR is replacing; those are judgment calls a maintainer should weigh.
Still open from earlier reviews (2):
- Unresolved: 2 minor or pre-existing.
2 participants
Problem
Bun.serveignores theIf-Rangerequest header. ARangerequest whose validator does not match the response still gets206 Partial Content. A client that resumes a download after the file changed joins bytes of two versions.FileRoute::serve(FileRoute.rs:371),DirectoryRoute::on(DirectoryRoute.rs:161) andRequestContext::do_sendfile(RequestContext.rs:1909).Fix
RangeRequest::from_requesttakes theETagandLast-Modifiedof the response. IfIf-Rangedoes not match them, it returns no range, and the route sends the full200body. File routes and{ dir }routes use it.If-Range.do_sendfilecompares it with theETagandLast-Modifiedthe handler set on its Response. Without them, the response is the full body.Last-Modified.test/js/bun/http/bun-serve-file.test.ts(45 new cases, 33 fail on 1.4.3) andtest/js/bun/http/serve-directory-routes.test.ts. Alsobun-serve-static,serve-if-none-match,serve-http2,serve-http3. Self-reviewed: 2 concerns raised, 2 addressed.Background
If-Rangecarries the validator (an ETag or a Last-Modified date) of the partial copy that the client has. It means: send the range only if the file is still that version, otherwise send all of it.W/prefix). The ETag of a{ dir }route is weak, so only the date form can match there.do_sendfileserves a handler'snew Response(Bun.file(p)). It can run after the uWS request object is gone, so the context copies the header.Notes
Repro on 1.4.3.
/dir/*is a{ dir }route,/fileis a file route, and thefetchhandler returnsnew Response(Bun.file(p)). Each request sendsRange: bytes=0-3andIf-Range: "does-not-match":main, not because of the fix. This PR also covers{ dir }routes.jsc_hooks::parse_http_datewith a strict HTTP-date parser forIf-Modified-SinceandIf-Unmodified-Since. TheIf-Rangedate usesparse_http_date, like the other date preconditions onmain. The PR that lands second should switch it.If-Rangealso turns off416, because the server ignores theRangeheader as a whole.If-Rangewithout a usableRange(absent, multi-range, malformed) has no effect, and the request context does not copy it.If-Rangetag never matches, as in Gonet/http. A client that sends the weak ETag of a{ dir }route gets the full file. That is correct, but it is not a resume. nginx compares the bytes of the two tags, and npmsenddoes a substring match. The docs now say that the date is the validator to resume with on a{ dir }route.GMT.parse_http_dateisDate.parse, which reads a date with no zone (the obsolete asctime form) in the server's local time. Such a date never matches, so the result does not depend on the server's time zone. The IMF-fixdate and rfc850 forms match. The strict parser of Parse If-Modified-Since and If-Unmodified-Since as RFC 9110 HTTP-date #41502 can accept asctime as GMT.net/http. A file that is rewritten twice in one second keeps the sameLast-Modified, and the server cannot see that. RFC 9110 §8.8.2.2 gives that check to the client: it may use aLast-Modifiedas a strong validator only when the response'sDateis later than it.Rangeonmain(it does not reachdo_sendfile). The HEAD cases cover the file route and the{ dir }route.Range: one branch inRequestContext::create, and one more header lookup inFileRoute::serve. The copy happens only whenRangeparses andIf-Rangeis present.it.eachnames did not say what theIf-Rangevalue was.node:http2client,http2: trueserver) gives the same results as HTTP/1.1 on all three paths.bun-serve-file,serve-directory-routes,bun-serve-static,serve-if-none-match,serve-http2,serve-http3,regression/issue/29181, theContent-Rangeblock ofserve.test.ts, andtest/internal/source-lints/.pollable Bun.file(...)tests inbun-serve-file.test.tstake 2.6 to 6.5 s under the debug ASAN build in my container, so they can exceed the default 5 s timeout when the host is busy. Five interleaved runs of the base binary and the fixed binary show the same times for both.no test proof · iteration 3 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/http/serve-directory-routes.test.ts, test/js/bun/http/bun-serve-file.test.ts