Conversation
WalkthroughHTMLRewriter now rejects disturbed locked response bodies. Body buffering also materializes Blob and File stream sources. Regression tests cover Bun.file streams and reused Blob or File bodies. ChangesHTMLRewriter body handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I checked that ValueBufferer has no callers other than HTMLRewriter (so the new is_disturbed guard in buffer_locked_body_value can't regress other body consumers), and traced the recursive self.run(value, None) after re-tagging *value to a Blob variant — it hits the Value::Blob | InternalBlob | WTFStringImpl arm and cannot re-enter Locked. Deferring to a human on the memory-ownership side of the new to_any_blob recovery path in Body.rs.
Extended reasoning...
The change replaces a user-reachable unreachable!() with a proper TypeError plus a defense-in-depth recovery path, and ValueBufferer is HTMLRewriter-only so blast radius is contained. The subprocess tests follow harness conventions and assert exact error text. The one open question worth a maintainer glance is the stream.to_any_blob → self.readable_stream_ref.deinit() → recursive run() sequence: it looks correct (the stream's done() runs inside to_any_blob, and the extracted AnyBlob owns its store), but it's a new control-flow shape in body-buffering code that touches store ownership.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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 `@src/runtime/webcore/Body.rs`:
- Around line 2487-2492: In the readable_stream handling path, validate
stream.is_locked(self.global) and stream.is_disturbed(self.global) before
assigning Value::Used. Ensure a failed transform() on a locked but unread stream
leaves the Response body usable after the lock is released, and add regression
coverage for that scenario.
🪄 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: bdb6681f-8bd0-4b1b-8a7d-c486cd584eca
📒 Files selected for processing (3)
src/runtime/api/html_rewriter.rssrc/runtime/webcore/Body.rstest/js/workerd/html-rewriter.test.js
There was a problem hiding this comment.
LGTM — narrow crash fix replacing a user-reachable unreachable!() with the same "Body already used" error that .text()/.json()/.clone() already return.
What was reviewed:
buffer_locked_body_value: the newis_disturbedguard mirrors every other body consumer; theSource::Blob|Filearm now recovers viato_any_bloband re-entersrun()on the existingValue::Blobpath (bounded, one level), falling back toUnsupportedStreamType— strictly safer than the panic it replaces.transform_: the pre-check reusesget_body_readable_stream(already called downstream inBufferOutputSink::init) and matches the adjacentValue::Usederror path.readable_stream_refownership:deinit()on the recovery path,Dropon the error path — no leak.- Tests: subprocess-based, drain stdout/stderr/exited concurrently, assert exact TypeError message for both Blob and Bun.file bodies; the earlier timeout and comment-length nits were addressed in b191979 / b419349.
Extended reasoning...
Overview
Three files: src/runtime/api/html_rewriter.rs (+9), src/runtime/webcore/Body.rs (+20/-4), test/js/workerd/html-rewriter.test.js (+63). The fix stops HTMLRewriter.transform() from aborting the process when handed a Response whose body stream has been partially read then had its reader released — a state where the stream is disturbed but not locked, and its Blob/File source has already detached its store. Previously this reached an unreachable!() in ValueBufferer::buffer_locked_body_value; now it throws the same "Response body already used" TypeError that .text()/.json()/.clone() produce for the same input.
Security risks
None. No untrusted-input parsing, no new allocation sizing, no auth/crypto/permissions surface. The change converts a process-aborting panic into a catchable TypeError, which is strictly a robustness improvement (a reachable panic on user input is a DoS per REVIEW.md).
Level of scrutiny
Medium. Body.rs is core Request/Response plumbing, but the change is confined to one match arm that was previously unreachable!() plus one added disjunct on an existing guard. I traced the new recursion self.run(value, None) after setting *value = Value::Blob/InternalBlob/WTFStringImpl: run() calls to_blob_if_possible() (no-op for these variants) then matches the Blob arm and invokes on_finished_buffering synchronously — one bounded hop into an existing, well-exercised path. On the Err(UnsupportedStreamType) fallback, readable_stream_ref is released by ValueBufferer::drop (line 2261). to_any_blob takes &mut self (hence the mut stream binding change), and its own is_disturbed check is redundant with the guard added at line 2490 — belt-and-suspenders, not a correctness dependency.
Other factors
All prior review feedback is addressed: my timeout nit (b191979), both comment-cop flags and my comment-length nit (02ce98d, b419349), and CodeRabbit's "consumed before rejecting" finding was withdrawn as pre-existing behavior out of scope for this crash fix. The PR body notes #36697 will replace ValueBufferer entirely; this is the minimal interim fix. Tests follow harness conventions (bunEnv/bunExe, concurrent pipe drain, stdout/stderr asserted before exitCode, tmpdirSync matching the neighboring (from file) test), and the PR evidence shows they fail on main (subprocess aborts) and pass with the fix on both debug+ASAN and release.
|
@robobun fix conflicts or close |
transform() on a Response whose body stream has been partially read (getReader() + read() + releaseLock()) used to abort with unreachable!() in ValueBufferer::buffer_locked_body_value; #36733 replaced that path and it now throws ERR_STREAM_ALREADY_FINISHED from wire_input's is_disturbed guard. Check is_disturbed in transform_ alongside the existing Value::Used check so a disturbed body throws the same 'Response body already used' TypeError as .text()/.json()/.clone() and as a body consumed by a previous transform(). Adds coverage for Bun.file().stream() bodies and the partial-read-before-transform scenario.
b419349 to
519cef1
Compare
robobun
changed the title
|
Rebased on 87e168f (#36733). That PR removed |
There was a problem hiding this comment.
LGTM — narrow, pattern-following fix with all prior feedback addressed.
What was reviewed:
- The
is_disturbedguard intransform_mirrors the identicalLocked → get_body_readable_stream → is_disturbedidiom used across Body.rs (~6 sites) and fetch.rs; the error message matches the adjacentValue::Usedcheck. - Post-rebase onto #36733 the Body.rs /
ValueBuffererchanges were correctly dropped (that code no longer exists);begin_transformline ~1100 already guards disturbed streams, so the in-process test no longer risks a crash — verifier confirmed the subprocess→in-process conversion is sound. - Timeout override, comment-length, and CodeRabbit's locked-body ordering concern were all addressed/withdrawn; Jarred's "fix conflicts" request appears satisfied by the rebase.
Extended reasoning...
Overview
After rebasing onto #36733 (which replaced ValueBufferer with the SinkHandle streaming path), this PR is now just 9 lines in src/runtime/api/html_rewriter.rs plus two tests. The Rust change adds an early is_disturbed check in transform_() so that transforming a Response whose body has been partially read throws a synchronous TypeError: Response body already used — matching .text(), .json(), .clone(), and the adjacent Value::Used branch — instead of returning a Response whose body later rejects with the less consistent ERR_STREAM_ALREADY_FINISHED from the pipe's line-1100 guard.
Security risks
None. This adds an earlier rejection on an already-detected invalid state; no new input parsing, no allocation, no unsafe. is_disturbed is already used in a non-throwing boolean context at html_rewriter.rs:1100 and throughout Body.rs.
Level of scrutiny
Low. The three-line nested-if is a verbatim copy of the pattern at Body.rs:1786-1790, 1920-1924, 1970-1974, 2025-2029, 2076-2080 and fetch.rs:1240-1244. The error string reuses the exact wording from the Value::Used branch four lines above. Tests follow the file's existing conventions (tmpdirSync, bunExe -e subprocess, it.each).
Other factors
All prior review threads are resolved: the 15s timeout was dropped (b191979), the long Body.rs comment was removed (b419349, and that file is no longer in the diff anyway), and CodeRabbit withdrew its locked-body ordering finding after the author showed it was pre-existing and out of scope. The bug-hunting system found nothing; two candidate concerns about the in-process test conversion were verified as non-issues (post-rebase, without the fix the test fails cleanly rather than aborting the process, since the unreachable!() no longer exists). Jarred's only outstanding request was to fix merge conflicts, which the rebase did.
This branch has not been deployed
2 participants
HTMLRewriter.transform()on aResponsewhose body stream has been partially read (getReader()+read()+releaseLock()) used to abort the process withinternal error: entered unreachable codeinValueBufferer::buffer_locked_body_value. #36733 replaced that code path and it now throwsERR_STREAM_ALREADY_FINISHEDfromwire_input'sis_disturbedguard, so the crash is gone; this PR is the residual consistency fix plus coverage.Fix
transform_checksis_disturbedon aLockedbody's stream alongside the existingValue::Usedcheck, so a disturbed body throws the sameResponse body already usedTypeError as.text()/.json()/.clone()and as a body consumed by a previoustransform()(the latter is already asserted by thetransform() marks the input body usedtest at the bottom of the file).Tests
(from Bun.file().stream()): subprocess coverage for the originally reported shape (transform rewrites aResponse(Bun.file(path).stream())body).throws on a disturbed Response body: partially reads a Blob body and a Bun.file body beforetransform()and assertsTypeError: Response body already used. Fails on main without thetransform_check (throwsError/ERR_STREAM_ALREADY_FINISHEDinstead), passes with it.Full
html-rewriter.test.jssuite: 124 pass, 0 fail.[stamp-90s] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file