Repository navigation
Conversation
…he and mode with them
…oes not match it fetch() accepted `integrity` and checked nothing. Now fetch_impl parses the option (fetch/sri.rs) and gives the metadata to FetchTasklet, which receives the whole body and compares its digest before it creates a Response. A body that does not match, or a response without a body, rejects with a TypeError. data: URLs and blob: URLs of an in-memory Blob are checked the same way. A request whose body fetch cannot check is refused: a file: URL, a blob: URL of a file or an S3 object, an upgrade, and an S3 upload with a stream body. Request stores the option in a boxed field and returns it from its `integrity` getter. A Request that is the input of fetch() or of new Request() gives the stored value, whatever property the object has.
…and blob: URLs, and on Request
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 6 minutes. View limit details
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 |
|
Updated 11:57 AM PT - Oct 16th, 2026
✅ @robobun, your commit 22d03d5b71d79f47775ea00539b9babc2562b7e0 passed in 🧪 To try this PR locally: bunx bun-pr 45561That installs a local version of the PR into your bun-45561 --bun |
|
Status Reproduced on main (5866b5f) with this script: import { createHash } from "node:crypto";
const server = Bun.serve({ port: 0, fetch: () => new Response("evil\n") });
const integrity = "sha256-" + createHash("sha256").update("real\n").digest("base64");
const outcome = await fetch(server.url, { integrity }).then(
async response => `resolved with ${JSON.stringify(await response.text())}`,
error => `rejected: ${error.name}: ${error.message}`,
);
console.log("fetch with a wrong digest:", outcome);
console.log("Request.integrity:", JSON.stringify(new Request(server.url, { integrity }).integrity));
server.stop(true);Output on main: Output with this PR (#45561): |
![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 the new Request.integrity owning field across every lifecycle path (all five constructors, finalize, clone_into, memory-cost reporting) and found it initialized, copied and cleared consistently; the reject_on_exception fail-closed guard, which rejects any exit of fetch_impl that returns an unrejected promise while the parsed metadata is still unconsumed; and the three-state handling of integrity (undefined falls through to the input Request's stored value, "" parses to None and suppresses it, a non-empty string is read from the init object and never from a JS-visible getter on a Request input).
Extended reasoning...
The change implements fetch()/Request integrity (SRI): a new parser/hasher in src/runtime/webcore/fetch/sri.rs, a boxed field on Request, option extraction in fetch.rs with a fail-closed guard, and whole-body buffering plus a digest check in FetchTasklet before a Response exists, with data:/blob:/file:/upgrade/S3-stream paths handled explicitly. It is a security-relevant feature on the fetch hot path, so the inline findings and the ruled-out list already signal that a human should weigh the design trade-offs (JS-thread hashing, full-body buffering, HEAD/204/304 rejection); this note only records the additional lifecycle and fail-closed checks that were examined and found consistent.
| let rejection = if let Some(integrity) = self.integrity.take() { | ||
| self.check_integrity(&integrity) |
There was a problem hiding this comment.
🟡 (optional) Every fetch() with integrity now freezes the JS event loop for the time it takes to hash the whole body, roughly 0.7 to 1.5 ms per MiB, so a 500 MiB response stalls all other requests in that process for about a second. check_integrity at FetchTasklet.rs:1144 runs integrity.check over scheduled_response_buffer synchronously inside on_progress_update, while the tasklet mutex is held. The base branch resolves at the head and never hashes. Fix: hash incrementally as chunks arrive (on the HTTP thread in the callback at FetchTasklet.rs:2686, or in a work-pool job) so the JS thread only compares a finished digest, instead of one synchronous pass over the complete body under the mutex.
Why this was flagged
Trigger: fetch(url, { integrity: 'sha256-...' }) for any body larger than a few MiB, entering through on_progress_update at src/runtime/webcore/fetch/FetchTasklet.rs:1143-1144 on the final progress tick. check_integrity (FetchTasklet.rs:1219-1244) calls Metadata::check (src/runtime/webcore/fetch/sri.rs:136-153), which calls Algorithm::digest over the whole buffered body on the JS thread. The PR's own numbers are 41 to 117 ms of timer stall per 64 MiB; that scales linearly, so hundreds of MiB means seconds of event-loop stall in a server that handles other requests. On the base branch the body is never hashed and the promise resolves at the head, so no such stall exists. This runs once per integrity fetch, at production rate for any service that fetches signed assets or packages. The dismissal accepted it because the author documented it; the cost is still a new unbounded synchronous stall. Remedy: incremental hashing off the JS thread, keeping only the final compare under the mutex.
Verification: Any fetch(url, { integrity }) whose body is large triggers it. on_progress_update (src/runtime/webcore/fetch/FetchTasklet.rs:928) takes self.mutex.lock() at line 931 and, on the final tick, calls self.check_integrity(&integrity) at line 1143-1144, which runs hasher.update(bytes) over the entire buffered body synchronously. The base branch has no such stall.
| @@ -0,0 +1,951 @@ | |||
| // fetch() checks the response body against the `integrity` option | |||
There was a problem hiding this comment.
🟡 nit (optional): maintainers get a new 951-line test file for fetch behaviour, which the root CLAUDE.md test-organization rule asks to avoid. CLAUDE.md:49 says a fetch change goes in test/js/web/fetch/fetch.test.ts and "Do not create a new file". Fix: either move the new tables into test/js/web/fetch/fetch.test.ts (and the Request cases already sit in request.test.ts), or state in the PR why a feature-sized suite with its own servers warrants its own file; the wpt/ driver and fetch-args.test.ts additions are fine as they are.
Why this was flagged
The diff adds test/js/web/fetch/fetch-integrity.test.ts as a new file for a fetch() option. The applicable root CLAUDE.md:49 reads: "Default: add your test to the existing test file for the code you're changing. Do not create a new file. A fetch bug goes in test/js/web/fetch/fetch.test.ts". No exception in CLAUDE.md:60 applies (that one is for regression issue files). The base branch has no such file; the consequence is only discoverability and a convention slip, not a behavioural defect, so this is a nit.
Verification: nit. Triggering condition: unconditional — the diff adds a new file. git diff --stat shows test/js/web/fetch/fetch-integrity.test.ts | 951 ++++ as a wholly new file. Root CLAUDE.md:49 reads verbatim: "Default: add your test to the existing test file for the code you're changing. Do not create a new file." The PR description gives no reason for a separate file. Nothing behavioural breaks.
Fixes #5650
Problem
fetch(url, { integrity })checks nothing. A body that does not match the digest reaches the caller.Request.integrityis always"".fetch_impl(src/runtime/webcore/fetch.rs) never reads the option, and nothing hashes a body.Fix
fetch_implreads and parsesintegrity(fetch/sri.rs).FetchTasklettakes the whole body and compares the digest before it creates aResponse. A mismatch, or a response without a body, rejects with aTypeError.data:and in-memoryblob:URLs get the same check. An uncheckable request rejects:file:, file-backedblob:, upgrades.test/js/web/fetch/fetch-integrity.test.ts, WPTintegrity.sub.any.js(16 of 18),fetch-args.test.ts,request.test.ts.Background
FetchTaskletis the JS-thread object behind onefetch(). Withoutintegrity, the promise resolves at the head and the body streams later.sha256-,sha384-orsha512-digests. Only the strongest algorithm counts.fetch_implresult and a digest on the HTTP thread. Both let aResponseexist first.Downsides
integritywith HEAD, 204, 205 or 304.integrity, the whole body stays in memory and is hashed on the JS thread: 0.67 ms per MiB (sha256), 1.48 (sha512).fetch(url): 5847 to 5809 instructions, 0 allocations added.fetch(request): 5955 to 6096.Request: 104 to 112 bytes, same allocator size class. Binary: +13,824 bytes.Notes
What a caller without
integritypaysRelease builds of main (5866b5f) and of this branch (22d03d5),
BUN_JSC_useJIT=0. Instructions are single-stepped with gdb from the entry of the host function to its return, callees included. The request goes to a closed port, so this is the synchronous part of the call. Each cell is the first counted call of a process (call 5).fetch(url)fetch(url, {})fetch(url, { method })fetch(url, { method, headers })fetch(request)fetch(request, {})fetch(requestSubclassInstance)fetch(url, requestAsInit)fetch({ url })fetch("data:,x")fetch("data:,x", {})new Request(url)new Request(url, {})new Request(url, { method })new Request(request)new Request(requestSubclassInstance)request.clone()RequestBun__fetchshape has a second level about 1,380 instructions higher on some later calls of one process, on both builds (fetch(url): 7232 and 7196 on main, 7239 here).integrityread on an init object is about 210 instructions and no allocation. To pay for it,redirectin fetch, andredirect,cacheandmodeinnew Request, are now read with common strings. Each of those by-name lookups was amallocand afree.fetch(request)is +87 to +141 (6042, 6051 and 6096 in three processes of one binary; main gave 5955 each time). The new checks are 16 of them. The rest is astarts_with_asciicall that the compiler no longer inlines (+19), code layout insidefetch_impl, and a libc copy routine that differs by process.fetch("data:,x", init)pays the read in full (+218), because main reads nothing from the init on that path.Requestismi_malloc(104)→mi_malloc(112), andmi_usable_sizeis 112 for both.FetchTaskletismi_malloc(832)→mi_malloc(840), usable size 896 for both.estimateShallowMemoryUsageOf(new Request(url))is 193 → 201.unsafe.integrityis the first key thatfetchandnew Requestread from an init. The order of the other keys does not change.What a caller with
integritypaysBufferAllmode, the mode thatarrayBuffer()on a plain response uses. The HTTP thread decodes the bytes as they arrive and the transport never pauses. The check is one hash pass on the JS thread when the last byte is in.integrity, 41 to 55 ms with sha256, 97 to 117 ms with sha512.arrayBuffer(): 230 to 323 MiB withintegrity, 264 to 360 MiB without.Differences from Node, on purpose
node-fetchandundicimodules call Bun's fetch, so they now enforceintegrity. Thenode-fetchpackage on npm ignores the option.TypeErrorwithcode: "IntegrityCheckFailed",path(the URL of the response) and a message that has the digest of the body that arrived. A value that is not a digest of its algorithm has a message of its own.Not in this PR
file:URLs and ofblob:URLs of aBun.file()or an S3 file. The first version of this branch had both (a work-pool job and a read before the check). The self-review found should-fix problems in each: an abort did not settle the promise while the job ran, and no signal stopped the read. They also held all the newunsafe. Each is a follow-up.server.fetch()does not readintegrity. Thecredentialsandmodeoptions stay ignored (Request has wrong default for credential property #17052). Bun has no opaque responses, so the twono-corssubtests of the WPT file aretodo.src/install/integrity.rsis not touched. install: fail closed on unusable npm integrity (pin computed sha512; reject invalid manifest/lockfile hashes) #31327 changes that parser.Related
Requestkeeps the string in a boxed field and stays in its size class, the option is read with a common string,file:andblob:no longer skip the check, and the metadata cannot leavefetch_implunchecked (reject_on_exceptionrejects if it does).integrityonRequest, as an inline string. The two PRs conflict inRequest.rs, and the second one to land has to rebase. This PR does theintegritypart of Request getters return fixed values, ignoring init: referrer, referrerPolicy, integrity, keepalive #30124.unsafefromFetchTasklet.rsandfetch.rs. This PR adds none. Its edits there are one block inon_progress_update, one inget, and the option read and two helpers infetch.rs.Self-review: the 4 concerns that are not addressed
Responsethat is built with the metadata still present is adebug_assert!inon_resolve. The check and the call are 30 lines apart in one function, andfetch.rshas a runtime guard for its own exits.integrityin abun build --compileexecutable, and no test goes through a proxy.packages/bun-typeshas no Bun-specific JSDoc forintegrity. The member exists through lib.dom and undici-types.Tests
fetch-integrity.test.tshas 91 failures of 169, the WPT file 4 of 16,request.test.ts7 andfetch-args.test.ts7. On this branch: 322 pass, 2 todo, 0 fail in the four files.fetch-integrity.test.tsfail.fetch-leak.test.ts(31 pass),blob.test.ts(147),fetch-gzip.test.ts(89) pass on both.body.test.ts: 776 pass here, 775 and 1 timeout on main.fetch-backpressure.test.ts: 127 pass on main, 126 and 1 timeout here, and that test passes 3 of 3 alone on both.fetch.test.ts: 375 pass and 18 fail on both (5 s timeouts of theutf16 (with gc)tests, and tests that need the internet).request-clone-leak.test.tsfails on both (5 tests on main, 4 here: its RSS bound and 5 s timeouts under ASAN).test/internal/source-lints/: 180 pass. The 2 that fail are timeouts insema-parser-recursion.test.ts, a lint of the JS parser.perf,valgrind,straceandbloatyare not in the container that measured this. Instruction counts are from gdb single-stepping, sizes fromsizeandmi_usable_size.no test proof · iteration 12 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/fetch-integrity.test.ts