fetch: reject already-used Request stream bodies before connecting - #36499
Conversation
fetch(request) where request has a ReadableStream body that was already consumed would open a TCP connection and write the full request head (POST /path HTTP/1.1 + Transfer-Encoding: chunked) before rejecting with ERR_STREAM_CANNOT_PIPE. Each retry left a half-open, unterminated chunked request at the origin. The pre-flight body check in fetch_impl consulted PendingValue::is_disturbed, which looks at body_get_cached (only populated once .body is accessed) and Locked.readable, but check_body_stream_ref had already migrated the stream out of Locked.readable into the JS stream cache slot. So the disturbed stream was never found until ResumableSink::init_exact_refs ran, after the head was on the wire. Check is_disturbed/is_locked on the actual ReadableStream returned by get_body_readable_stream (which consults the stream cache slot) before wrapping it as the request body, so the TypeError is thrown before any I/O. This matches the spec unusable check used by Request.clone() and node/undici.
|
Reproduced and fixed: pre-flight check now finds the stream in the JS |
WalkthroughFetch now rejects disturbed or locked ChangesRequest body validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — narrow pre-flight guard, well-tested. The inline nit on the locked-body test's connections assertion is worth tidying but not blocking.
Extended reasoning...
Overview
Adds an 8-line is_disturbed || is_locked guard in fetch_impl's Request-body extraction path, immediately after get_body_readable_stream returns the stream and immediately before it is wrapped as HTTPRequestBody::ReadableStream. Two new tests in body-mixin-errors.test.ts assert the origin sees no extra connections and the rejection is TypeError/ERR_BODY_ALREADY_USED.
Security risks
None. This is a stricter pre-flight rejection using existing error-code machinery; no new parsing, no new I/O, no auth/crypto surface.
Level of scrutiny
Medium — fetch.rs is hot-path code, but the change is a pure guard clause on a path that would already fail later (in ResumableSink::init_exact_refs, post-connect). The successful path is unchanged: the guard sits exactly where the stream is first obtained, uses the same ErrorCode::BODY_ALREADY_USED / message as the sibling already_used branch just above, and ERR_BODY_ALREADY_USED maps to TypeError in ErrorCode.ts so the tests' instanceof TypeError assertions hold.
Other factors
- Verified the sibling
init.bodypath (HTTPRequestBody::from_js) already has its own disturbed check and does not go through thestream-cache-slot migration that caused this bug, so the fix correctly targets only the Request-object arm. - Tests use
port: 0,it.concurrent, socket cleanup infinally, and a probe fetch to synchronize the connection-count assertion in the primary test. - One nit was flagged (the locked-body test's
connections === 0check lacks the same probe barrier); it's non-blocking because theTypeError/ERR_BODY_ALREADY_USEDassertions already distinguish fixed from unfixed builds — the connection count is defense-in-depth.
Mirrors the sibling test so the 'no network I/O' claim can actually fail on a regressed build (the server's connection callback can lag the fetch rejection otherwise).
There was a problem hiding this comment.
LGTM — thanks for adding the probe barrier to the locked-body test in c4d6191. The remaining inline note about extracting a shared withConnectionCountingServer helper is a style nit, not blocking.
Extended reasoning...
Overview
8-line addition to fetch_impl in src/runtime/webcore/fetch.rs: after get_body_readable_stream returns the Request's stream, check is_disturbed || is_locked and reject with BODY_ALREADY_USED before wrapping it as the outbound body. Two new tests in test/js/web/fetch/body-mixin-errors.test.ts assert the origin sees no phantom connections and the rejection is TypeError / ERR_BODY_ALREADY_USED.
Correctness
The predicate and error shape mirror the existing spec-unusable checks at Body.rs:1018 (body init), Body.rs:1886 (throw_if_body_unusable used by Request.clone/Response.clone), and ResumableSink.rs:160 (the post-connect check this PR pre-empts). is_disturbed/is_locked are the same non-throwing FFI accessors used at every sibling site, so no new exception-scope handling is needed. The check sits inside the existing BodyValue::Locked branch, so string/Blob/empty bodies are unaffected, and it runs before FetchTasklet::queue so no HTTP thread work is scheduled.
Security risks
None. The change makes fetch reject earlier on an already-invalid path, eliminating half-open chunked requests at the origin — strictly less network activity than before.
Level of scrutiny
Low-to-moderate. The runtime change is small, additive, and pattern-matched against three existing call sites; it does not touch memory ownership, threading, or the happy path. The tests are the larger surface and were the focus of both review rounds.
Other factors
- My prior nit (missing probe barrier on the locked-body connection assertion) was addressed in c4d6191 and the author confirmed it now fails
Expected: 1, Received: 2on a pre-fix build. - PR description shows the tests fail on both ASAN-debug and release builds without the fix and pass with it.
- The one remaining inline finding (duplicated ~30-line server scaffolding across the two new tests) is a code-style nit that doesn't affect correctness; a
withConnectionCountingServerhelper would be nice but isn't merge-blocking.
Deduplicates the server/probe scaffolding shared by the two stream-body pre-flight tests, matching the withTruncatedBodyServer pattern already used in this file.
There was a problem hiding this comment.
LGTM — both earlier nits (probe barrier, helper extraction) are addressed in c4d6191 and 5739a2e.
What was reviewed:
- The 8-line guard mirrors the exact
is_disturbed || is_lockedspec-unusable check already used atBody.rs:1018,Body.rs:1886(throw_if_body_unusable), andResumableSink.rs:160;is_disturbed/is_lockedare non-throwing FFI slot reads. get_body_readable_streamwas already called at this site, so the new check adds no side effects — it just gates on the returned stream before wrapping it.withConnectionCountingServernow owns the probe-fetch barrier and awaitedserver.close(); both tests share it, are hermetic (port 0, local net.Server), and assert exact error class + code.
Extended reasoning...
Overview
The PR adds an 8-line pre-flight check in fetch_impl (src/runtime/webcore/fetch.rs) that rejects a Request whose ReadableStream body is disturbed or locked with TypeError/ERR_BODY_ALREADY_USED before any network I/O. Two new tests in test/js/web/fetch/body-mixin-errors.test.ts verify the origin sees no phantom connections for both the consumed-stream and locked-stream cases, using a shared withConnectionCountingServer helper with a probe-fetch synchronization barrier.
Security risks
None. The change tightens validation (rejects earlier), does not touch TLS/auth/credential paths, and does not expand accepted input. The tests use a local net.Server on port 0 with no external network access.
Level of scrutiny
Low-to-moderate. This is a small, focused correctness fix in the fetch pre-flight path. The guard is inserted between an existing get_body_readable_stream call and the existing HTTPRequestBody::ReadableStream wrap — no new call, no new state, no exception-handling requirement (both is_disturbed and is_locked return plain bool via FFI slot reads). The identical predicate + error code are already used at three sibling sites in the same subsystem (Body.rs:1018 for body-init streams, Body.rs:1886 for clone(), ResumableSink.rs:160 for the late sink check this PR pre-empts), so the pattern is well-precedented.
Other factors
I left two prior review rounds on this PR — a missing probe barrier in the locked-body test and duplicated ~30-line test scaffolding — and both were addressed (c4d6191, 5739a2e). The extracted helper follows the file's existing withTruncatedBodyServer convention and awaits server.close(). The author confirmed the locked-body test fails with Expected: 1, Received: 2 on a pre-fix build after the barrier was added. The bug hunting system found nothing this run. No outstanding reviewer comments remain.
Problem
fetch(request)whererequesthas aReadableStreambody that was already consumed opens a TCP connection and writes the full request head (POST /path HTTP/1.1+Transfer-Encoding: chunked) before rejecting withERR_STREAM_CANNOT_PIPE. Each retry leaves a half-open, unterminated chunked request at the origin (0 body bytes, no0\r\n\r\n).Node/undici throws
TypeErrorbefore any network activity. String/Blob-body used Requests already reject pre-flight in Bun; only the stream-body path has this bug.Cause
The pre-flight
already_usedcheck infetch_implconsultsPendingValue::is_disturbed::<Request>, which looks atbody_get_cached(only populated once.bodyis accessed) andLocked.readable. Butcheck_body_stream_ref(run fromRequest::to_js) migrates the stream out ofLocked.readableinto the JSstreamcache slot, so neither lookup finds it. The disturbed stream isn't checked untilResumableSink::init_exact_refsruns, which is after the HTTP thread has connected and written the request head.Fix
Check
is_disturbed || is_lockedon the actualReadableStreamreturned byget_body_readable_stream(which consults thestreamcache slot) before wrapping it as the request body. This is the same spec unusable check thatRequest.prototype.cloneapplies viathrow_if_body_unusable, and throwsTypeErrorwithERR_BODY_ALREADY_USEDbefore any I/O.Verification
New tests in
test/js/web/fetch/body-mixin-errors.test.tsassert the origin sees exactly the expected connection count (2: first fetch + probe) and that re-fetches reject withTypeError/ERR_BODY_ALREADY_USED. A second test covers the locked-but-not-disturbed case.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 1
evidence per changed file