webcore: throw when new Request() copies a Request whose body is used or locked

RO
1mo ago
+184-17

Fixes #44307

Problem

  • new Request(usedRequest) and new Request(url, usedRequest) return a Request with an empty body when the input's body is consumed. The Fetch spec requires a TypeError ("If input is unusable, then throw a TypeError"). Node throws. server.fetch(usedRequest) sends an empty body for the same reason.
  • Both constructor forms take the single-value arm of Request::construct_into (src/runtime/webcore/Request.rs:1089) and call Request::clone_into, which has no usability check. Value::clone_with_readable_stream (src/runtime/webcore/Body.rs:1601) then clones a Used body as Empty.

Fix

  • clone_into now runs throw_if_body_unusable first. Every copy of a Request passes through it: the constructor, Request.prototype.clone, BunRequest.prototype.clone, and server.fetch(request). The duplicate pre-checks in do_clone and ffi_clone are removed.
  • The two-argument arms (a Request or a Response as init) run the same check before they tee the input body. Before, they threw by accident: the JS body getter allocated a closed stand-in stream and Value::from_js rejected it. The same arms now copy an empty-string body instead of sharing the input's stream through that getter.
  • server.fetch(request) rejects the returned promise on this error, like every other bad argument to it.
  • The error is Bun's existing clone() error: TypeError: Body is disturbed or locked, code ERR_BODY_ALREADY_USED. This is deliberate: one message for one condition. Node uses two different texts for the two constructor forms.
  • Verified: test/js/web/fetch/body-clone.test.ts (new describe, 9 of 13 cases fail on 1.4.3). Also test/js/web/request/, body.test.ts, body-stream.test.ts, response.test.ts, the Bun.serve body tests.

Background

  • BodyValue (Body.rs) is the body state shared by Request and Response: Null, Empty, a blob or string, Locked (a stream), Used, or Error. "Unusable" means the body is non-null and its stream is disturbed or locked (body_stream_check).
  • Request::clone_into builds a Request from another one. construct_into calls it for a Request as input or init. Request::clone() calls it for the JS clone() methods and for server.fetch(request).
  • Considered a check only in construct_into before clone_into: it leaves server.fetch(usedRequest) broken and keeps three separate pre-checks around one clone function. Considered an Err from the Value layer: Value has no access to the owner's JS stream cache, which decides "disturbed or locked".

Downsides

  • Behavior change: new Request(usedRequest) and new Request(url, usedRequest) now throw, and server.fetch(usedRequest) rejects, where they returned an empty-body Request. Code that relied on the silent empty copy has to clone before it reads. This is the spec and Node behavior.
  • Message change: new Request(usedRequest, {}) and new Request(url, usedResponse) now throw Body is disturbed or locked (code ERR_BODY_ALREADY_USED) instead of Body object should not be disturbed or locked (no code). Both are TypeError.
  • Cost per copy of an unread Request: one match on the body value, and for a stream body one is_disturbed_or_locked check that clone() already paid. No new allocation. The two-argument arms drop one stand-in ReadableStream allocation on the throw path and on the empty-body path.
Notes
  • Self-reviewed: 3 required changes raised, 3 addressed (the Response-as-init arm, a Used arm in Value::clone_with_readable_stream, and the message statement above). The Used arm was removed again after review: every caller runs the owner's check first, so it was unreachable.
  • Open robobun PRs #35858 (tee to transfer for new Request(request)) and #37033 (internal-state copy) also add a usability check in the constructor arms, inside much larger changes. Neither touches clone_into, do_clone, ffi_clone, or server.fetch. This PR is the smallest change that fixes the report. Both can rebase onto it.
  • new Request(used, { body: "x" }) keeps working: init gave a body, so the spec does not throw. A consumed GET request has a null body and still copies in every form.
  • The test that exercised the C++ readableStreamTee exception path through new Request(lockedRequest) now exercises a locked Response as init, and expects the usability error, since no constructor path tees a locked stream any more.
  • Repro rows, bun 1.4.3 then this branch (node v26 in parentheses):
    • new Request(used): "", now TypeError (TypeError)
    • new Request(url, used): "", now TypeError (TypeError)
    • new Request(used, {}): TypeError, unchanged (TypeError)
    • new Request(used, { body: "x" }): "x", unchanged ("x")
    • server.fetch(used): body "" sent, now rejects with TypeError (no equivalent)
    • new Request(emptyStringBodyRequest, {}) then read both: the second read rejected Body already used, now ["", ""]. This is a regression on main: 1.4.2 reads "" from both. Node v26.3.0 does not read both: it transfers the body, so the input's read rejects (Body is unusable: Body has already been read) and the copy reads "". #35858 covers the transfer.
  • Not changed: new Request(used, { body: null }) copies with a null body. The spec treats body: null as "init gave no body" and throws. #37033 covers that case.
  • Also ran: test/js/web/request/request.test.ts, request-subclass.test.ts, request-clone-leak.test.ts, test/js/web/fetch/body.test.ts, body-stream.test.ts, response.test.ts, test/js/bun/http/serve-body-leak.test.ts, bun-serve-body-json-async.test.ts, serve-stream-body-error.test.ts, test/regression/issue/02368.test.ts.

no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/body-clone.test.ts