webcore: make Request/Response clone() throw on a disturbed or locked body - #33129
Conversation
… body Fetch spec step 1 of both clone() algorithms requires a TypeError when the receiver is unusable (body non-null and its stream disturbed or locked). Bun never ran this check, so cloning a consumed body succeeded and, for non-stream bodies, returned a clone whose body silently reads as empty. Node (undici), Deno, and browsers all throw. Add BodyMixin::throw_if_body_unusable and call it at the top of both do_clone entry points. The unusable predicate is the existing bodyUsed walk plus ReadableStream::is_locked, so get_body_used is refactored to share it (body_stream_check) rather than duplicating the match. The error is ERR_BODY_ALREADY_USED (a TypeError) with the message "Body is disturbed or locked".
|
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughIntroduces a shared Body Clone Usability Enforcement
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:50 AM PT - Jun 30th, 2026
❌ @robobun, your commit f86f607 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33129That installs a local version of the PR into your bun-33129 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
#6860 is a different code path and is not fixed by this PR, so it should not be linked with a That issue is about reusing a standalone I ran #6860's repro against this branch and against the released 1.4.0; the output is identical: The |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
BunRequest.prototype.clone (the subclass that Bun.serve routes: handlers receive) dispatches through JSBunRequest::clone -> Request__clone -> Request::ffi_clone rather than Request::do_clone, so it skipped the new throw_if_body_unusable check and still returned a clone with a silently empty body after the request was read. Add the check at the top of ffi_clone. Returning None with the exception pending matches the existing contract: JSBunRequest::clone asserts raw-null iff exception and RETURN_IF_EXCEPTIONs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/js/web/fetch/body-clone.test.ts`:
- Around line 558-561: The subprocess assertions in the body-clone tests are too
weak because they only exclude one substring from stderr, allowing unexpected
diagnostics or stack traces to slip through. Update the affected checks in the
body-clone test cases to assert an exact empty stderr result (or otherwise
include stderr in the expected subprocess result) for the bunEnv-backed probes,
using the existing subprocess result assertions around the caught TypeError
cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 16b19ec4-75a4-4e3e-800b-1addba2c3aa1
📒 Files selected for processing (4)
src/runtime/webcore/Body.rssrc/runtime/webcore/Request.rssrc/runtime/webcore/Response.rstest/js/web/fetch/body-clone.test.ts
Fold stderr into the combined {stdout, stderr, exitCode} toEqual so
any unexpected diagnostic or double-reported error fails the test,
instead of only excluding one substring.
There was a problem hiding this comment.
My earlier feedback on the BunRequest FFI path is addressed and I found nothing new on re-review; deferring to a human only because this intentionally changes clone() from silently succeeding to throwing on consumed/locked bodies, which is the kind of user-visible compat shift a maintainer should sign off on.
Extended reasoning...
Overview
Adds the fetch-spec step-1 "body unusable" check to Request.prototype.clone(), Response.prototype.clone(), and BunRequest.prototype.clone (the Bun.serve routes: subclass). Implementation refactors BodyMixin::get_body_used into a shared body_stream_check(check_fn) helper, then throw_if_body_unusable calls it with is_disturbed || is_locked and throws ERR_BODY_ALREADY_USED (a TypeError). Three one-line guards are added at the top of Request::do_clone, Request::ffi_clone, and Response::do_clone. ~230 lines of new tests cover every body representation, both handler kinds in Bun.serve, negative cases (null body, materialized-but-unread stream), and preserve coverage for the readableStreamTee C++ bridge via new Request(lockedRequest).
Security risks
None. No auth, crypto, permissions, or untrusted-input parsing is touched. The change adds an early throw on an existing state predicate; it does not relax any check or expose new data.
Level of scrutiny
Moderate-to-high. The Rust diff is small and the refactor of get_body_used is behavior-preserving (same match arms, same is_disturbed predicate), but Request/Response clone() is a core, widely-used web API. The key judgment call is the user-visible behavior change: code that previously called clone() after consuming a body got a silently-empty clone; it will now throw. This is spec-correct and matches Node/Deno/browsers, and the PR description argues convincingly that any such caller was already broken — but it is still the kind of intentional compat shift that benefits from a maintainer's explicit ack rather than a bot approval.
Other factors
- My one prior finding (the
BunRequestffi_clonepath bypassing the check) was fixed exactly as suggested in 328e7a2, with dedicatedroutes:tests for both parameterized and static routes plus the unread negative case. - The CodeRabbit nit (assert empty
stderrin the subprocess probes) was adopted in 29e8a85. - All inline review threads are resolved; no outstanding human reviewer comments.
- The robobun build-status comment shows failures on three lanes against 328e7a2, but they are
scripts/build/ci.tsbuild-step failures, not test failures, and predate the latest commit — likely infra, but worth confirming green before merge. - No CODEOWNERS cover the touched paths.
- The PR explicitly scopes out the analogous
new Request(usedRequest)constructor check (spec step 36.1), which is a reasonable boundary.
The only failure on build 67145 was test/js/node/test/parallel/test-net-connect-memleak.js on the two alpine x64 test lanes. That test's gc() assertion exercises net.connect and a FinalizationRegistry; nothing in this diff touches either. It also fails 100 out of 100 runs under the unmodified bun v1.4.0-canary.1+a1c39ded9 release binary on linux-x64, and a1c39de is an ancestor of this branch's base, so the failure is independent of this change.
There was a problem hiding this comment.
Thanks for the quick fix on the BunRequest path — that and the test additions look right, and I found no further issues. Deferring to a human since this is a user-visible behavior change to Request/Response.prototype.clone() (previously-silent calls will now throw), which is worth a maintainer's sign-off even though it's spec-correct.
Extended reasoning...
Overview
This PR implements step 1 of the fetch spec's clone() algorithms for Request and Response: throw a TypeError when the body is non-null and its stream is disturbed or locked. Previously Bun allowed clone() after the body was consumed, yielding a clone with a silently empty body (for string/Blob/FormData) or a deferred error (for streams).
Changes:
src/runtime/webcore/Body.rs(~30 lines): extracts a parameterizedbody_stream_checkhelper from the existingget_body_usedmatch, and addsthrow_if_body_unusablethat calls it withis_disturbed || is_lockedand throwsERR_BODY_ALREADY_USED(aTypeError).src/runtime/webcore/Request.rs(2 one-liners): guardsdo_cloneandffi_clone(theBunRequest.prototype.cloneFFI entry).src/runtime/webcore/Response.rs(1 one-liner): guardsdo_clone.test/js/web/fetch/body-clone.test.ts(~230 lines added): a newdescribeblock covering consumed string/stream/Blob/FormData bodies, locked bodies, in-flight reads,fetch()responses,Bun.servecatch-all androutes:handlers (both parameterized and static), plus negative cases (null body, unread body, body-getter-only). Also updates the existing locked-stream subprocess test for the new message and adds a siblingnew Request(lockedRequest)subprocess test to keep thereadableStreamTeeexception-propagation path covered.
My earlier review flagged that BunRequest.prototype.clone bypassed do_clone via ffi_clone; that was fixed in 328e7a2 with the guard and matching routes: tests. CodeRabbit's stderr-assertion nit was addressed in 29e8a85.
Security risks
None. No auth, crypto, permissions, or untrusted-input parsing is touched. The change adds an early-throw guard; it does not relax any check or expose new data.
Level of scrutiny
Moderate. The Rust diff is small (~35 net lines) and mechanically straightforward — a refactored helper plus three guard calls at the top of existing clone entry points, all returning early via ?/.ok()? on the existing JsResult/Option paths. The refactor of get_body_used into body_stream_check is behavior-preserving (same match arms, same stream lookup order).
What elevates this above auto-approval is that it's a user-visible behavior change to a fundamental Web API: code that previously called req.clone() after consuming the body and got an empty clone will now get a synchronous TypeError. That is spec-correct and matches Node (undici), Deno, and browsers — and the silent-empty-body behavior it replaces is almost always a bug in the caller — but it can still break existing (buggy) middleware in the wild. The PR description explicitly scopes out the related new Request(usedRequest) constructor check for blast-radius reasons, which is a reasonable call but underscores that this class of change merits a maintainer's eye.
Other factors
- Test coverage: very thorough — positive and negative cases, all three JS entry points (
Request.prototype.clone,Response.prototype.clone,BunRequest.prototype.clone), and preservation of the tee-exception regression coverage. The author reports 44/44 passing and surrounding suites unchanged. - CI: the one failure (
test/js/node/test/parallel/test-net-connect-memleak.json Linux x64) is unrelated to fetch/body/clone. - Prior review: my one finding was addressed with code + tests; CodeRabbit's test-tightening nit was also addressed. No outstanding reviewer comments remain.
- CODEOWNERS: none cover the touched paths.
|
Status for a reviewer: the change itself is done and its tests are green. The red CI lanes across all three builds on this branch are unrelated to the diff.
|
2 participants
Problem
Request.prototype.clone()andResponse.prototype.clone()never perform step 1 of the fetch spec's clone algorithms: "If this is unusable, then throw aTypeError", where unusable means the body is non-null and its stream is disturbed or locked (https://fetch.spec.whatwg.org/#body-unusable).What Bun does instead depends on the body's internal representation:
clone()succeeds and the clone resolves to an empty body.clone()succeeds and an error surfaces later, from the clone's own read or from the internal tee, never fromclone()itself.The check exists so that clone-after-read, which is always a bug in the caller, fails loudly in development. Without it, proxy and retry middleware that does
clone()then forwards the request silently forwards an empty body whenever the order of operations is wrong.Node (undici) throws a
TypeErrorsynchronously fromclone()in each case:Fix
Add
BodyMixin::throw_if_body_unusableand call it at the top of bothdo_cloneentry points (src/runtime/webcore/Request.rs,src/runtime/webcore/Response.rs).Request has a third JS entry point:
BunRequest.prototype.clone, the subclass thatBun.serveroutes:handlers receive. It dispatches throughJSBunRequest::clone->Request__clone->Request::ffi_clonerather thando_clone, so it gets the same check at the top offfi_clone. Response has no such subclass.Request::cloneandResponse::clonehave no other callers, so every JS-visibleclone()is covered and no internal clone path changes.The unusable predicate is the existing
bodyUsedwalk withReadableStream::is_lockedOR'd in, soget_body_usedis refactored to share a parameterized helper (body_stream_check) instead of duplicating the match.The error is
ERR_BODY_ALREADY_USED, an instance ofTypeErrorlike node and browsers, with the messageBody is disturbed or locked(WebKit's wording, which covers both halves of the predicate).Only the two
clone()entry points are guarded.new Request(usedRequest)has the same spec check (Request constructor step 36.1) and is also missing in Bun, but it is a separate algorithm with a much wider blast radius inBun.servemiddleware, so it is intentionally not part of this PR.fetch(usedRequest)already throws.Tests
test/js/web/fetch/body-clone.test.ts:describe("clone() throws when the body is disturbed or locked"): consumed string / user-stream / Blob / FormData bodies, locked bodies, an in-flight read, afetch()response disturbed by a reader, a consumedBun.serveincoming request (catch-allfetchhandler), and a consumedBunRequestfrom aroutes:handler (both a/:paramroute and a static one). All fail onmain.Bun.serverequest (both handler kinds), and a body materialized by thebodygetter still clone.clone()on a locked stream body throws a catchable TypeError" subprocess test now asserts the new message. Since the usability check fires before the stream is teed, that test no longer reaches thereadableStreamTeeexception-propagation path it was added for in webcore: propagate exceptions from readableStreamTee instead of reporting them as uncaught #32786, so a sibling test drives the same path throughnew Request(lockedRequest), which still tees, and keeps that coverage.With the fix,
bun bd test test/js/web/fetch/body-clone.test.tspasses 44/44. The surroundingbody.test.ts,response.test.ts,body-stream.test.ts,blob.test.ts, andtest/js/web/request/suites are unchanged.