HTMLRewriter: pass text bytes through unchanged when a text handler only observes - #35933
HTMLRewriter: pass text bytes through unchanged when a text handler only observes#35933robobun wants to merge 5 commits into
Conversation
…nly observes
Registering a no-op text handler (on(sel, {text(){}}) or onDocument({text(){}}))
rewrote every byte that is not valid UTF-8 in the covered text into U+FFFD in
the output, while the same rewriter with only element/comments/end handlers
emitted the input byte-for-byte. A legacy-encoded page, a mislabeled binary, or
WTF-8 content was silently corrupted the moment a text callback was added.
lol-html's TextChunk path decodes the raw bytes to a &str (lossy) for the
handler, then re-emits the decoded string. Comments and tags keep the original
bytes and re-emit those when unmodified; text chunks did not.
Carry the raw input slice that each decoded chunk came from through to
TextChunk, and have serialize_self emit that slice when the handler has not
called set_str/as_mut_str. before/after/replace/remove continue to work since
they act on mutations, not on the chunk text.
WalkthroughChangesRaw input bytes are carried through lolhtml text decoding and stored on Raw text byte passthrough
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:42 AM PT - Jul 26th, 2026
❌ @robobun, your commit 9c5e0c8 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35933That installs a local version of the PR into your bun-35933 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
…with text The previous revision emitted an extra empty-text handler call for a text node that is only an incomplete UTF-8 prefix (e.g. <p>\xE2</p>), and left raw/text misaligned by up to 3 bytes when encoding_rs held an incomplete sequence across a feed_text boundary. TextDecoder now owns the <=3 raw bytes that encoding_rs has buffered and attributes them to the next emitted chunk's raw, so the handler sees the same chunks it always did and each chunk's raw is exactly the bytes its text was decoded from. utf8_incomplete_tail_len computes the buffered length via std from_utf8's error_len()==None, which matches encoding_rs's WHATWG UTF-8 state machine bit-for-bit. Adds a 26-test matrix covering truncated 2/3/4-byte prefixes immediately before a close tag, a text node that is only a truncated lead byte, and the >1KB fast-path/slow-path split, plus a test that pins the handler's observed chunk sequence to today's behavior.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Locks in that a conditional replace on one of two text chunks straddling a UTF-8 code point produces valid UTF-8 either way, and that a write carrying only an incomplete lead byte never fires the handler with an empty non-last chunk.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@patches/lolhtml/text-chunk-raw-passthrough.patch`:
- Around line 103-116: Update the raw passthrough logic around consumed, tail,
and utf8_incomplete_tail_len so incomplete decoder sequences are handled for
every supported active encoding, not only UTF_8. Generalize tail-length
calculation using the current encoding’s decoder behavior, or disable raw
passthrough for non-UTF-8 encodings; preserve correct carry handling across
write boundaries and avoid emitting split sequence bytes.
- Around line 72-73: Update the UTF-8 fast path around split_utf8_start so it
cannot discard a non-empty pending_raw prefix: either prepend pending_raw to the
emitted raw slice before returning Some(Cow::Borrowed(...)), or bypass this fast
path until pending_raw has been emitted. Replace the debug_assert-only guard,
preserving all deferred bytes in release builds.
In `@test/js/workerd/html-rewriter.test.js`:
- Around line 742-751: Update the parameterized cases around transform to track
handler invocation counts for every observer setup except the “no handlers”
control. Increment the relevant counter inside each element, comments, text, and
onDocument handler, then assert each counter is greater than zero alongside the
existing byte-output assertion so the tests detect handlers being skipped.
- Around line 1305-1356: Add JavaScript coverage for cross-write UTF-8 buffering
by extending the rewrite test helpers around `rewrite` to transform a
`ReadableStream` whose chunks split a multi-byte sequence, rather than always
using `new Response(src)` with one buffer. Include a case where a neighboring
chunk is replaced or modified, and assert the streamed output matches the
expected replacement behavior while exercising `pending_raw` across the write
boundary.
🪄 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: 622c7c41-58f8-4806-8567-faced769e9d6
📒 Files selected for processing (3)
patches/lolhtml/text-chunk-raw-passthrough.patchscripts/build/deps/lolhtml.tstest/js/workerd/html-rewriter.test.js
For non-UTF-8 document encodings the decoder slow path now passes raw=None and falls back to re-encoding the decoded text, which is the pre-patch behaviour: the tail accounting is UTF-8-specific and Bun only ever builds the rewriter with UTF-8. The fast path's ASCII prefix is byte-identical in every ASCII-compatible encoding so it keeps its borrowed raw slice. The earlier 7-case observer block duplicated the larger matrix further down without proving the handler fired; dropped it and moved its one unique case (text outside the selector) into the matrix block.
|
CI status: the only hard failure is the The two test annotations ( The html-rewriter test suite itself passes on every lane that has finished. The diff is ready for review. |
There was a problem hiding this comment.
My earlier concern about utf8_incomplete_tail_len coverage is resolved — the Rust unit test in the patch (unmodified_chunk_raw_bytes_stay_code_point_aligned_across_writes) is the load-bearing check, and I confirmed BufferOutputSink::run_output_sink does a single write(bytes) so a JS-level split test would not reach the carry path. No new issues found; deferring to a human because this is a behavioral change to text serialization implemented as a vendored-dependency patch.
Checked: utf8_incomplete_tail_len agrees with encoding_rs's WHATWG UTF-8 state machine on the surrogate/overlong 2nd-byte edge cases (ED A0, F0 80 → tail=0); set_str/as_mut_str clear raw so mutated chunks re-encode; replace()/remove() go through mutations and skip serialize_self (test confirms); the patches: field follows the existing pattern used by libuv/boringssl/highway; non-UTF-8 encodings fall back to raw = None and Bun only builds with utf_8().
Extended reasoning...
Overview
The PR patches the vendored cloudflare/lol-html crate so that TextChunk carries the raw input bytes it was decoded from and serialize_self re-emits those bytes verbatim when the handler did not mutate the text. Previously, registering any text handler forced a lossy decode→re-encode round-trip that replaced non-UTF-8 bytes with U+FFFD even for pure observers. Three files: a new 387-line patch under patches/lolhtml/, a one-line patches: addition to scripts/build/deps/lolhtml.ts, and 122 lines of new tests in test/js/workerd/html-rewriter.test.js plus three Rust unit tests inside the patch itself.
Security risks
None identified. The change relaxes output transcoding to pass input bytes through unchanged; it does not parse, allocate from, or trust any new user-controlled quantity. pending_raw is bounded at ≤3 bytes for UTF-8 and cleared on every last_in_text_node. The serialize_self change writes raw bytes directly to the output sink, which is what every other token type (tags, comments, doctypes) already does when unmodified.
Level of scrutiny
This warrants human review. It patches a vendored dependency's serialization path with subtle UTF-8 boundary accounting: utf8_incomplete_tail_len must exactly agree with encoding_rs's internal decoder state on every lead/continuation validity case, and pending_raw must stay code-point-aligned with the decoded text across the fast path, slow-path loop, skipped-handler branch, and flush. The reasoning in the patch comments and PR description is careful and I did not find a counterexample, but this is the kind of change where an off-by-one produces mojibake only on specific byte patterns at specific chunk boundaries. It also changes user-visible behavior (a text handler that reads .text and does nothing now leaves invalid bytes in the output rather than sanitizing them), which is arguably more correct but is a semantic shift a maintainer should sign off on.
Other factors
All four prior review threads (mine on raw/text misalignment and on carry-mechanism coverage, coderabbit's on non-UTF-8 encodings and on handler-invocation proof) are resolved with code changes or verified explanations. The 33 JS tests fail on USE_SYSTEM_BUN=1 and pass on the debug build; cargo test --lib in the vendored crate passes (146 tests). The cross-write carry is only reachable at the lol-html layer because Bun buffers the whole body into one write(), so the Rust unit tests are the appropriate place for that coverage. The patches: wiring matches the established pattern used by libuv, boringssl, highway, and others.
2 participants
Repro
Registering a no-op
texthandler (element oronDocument) rewrote every byte that is not valid UTF-8 in the covered text into U+FFFD in the output, while the same rewriter with onlyelement/comments/endhandlers (or none) emitted the input byte-for-byte. A legacy-encoded page, a mislabeled binary, or WTF-8 content was silently corrupted the moment a text callback was added, with no error.Cause
lol-html's
TextChunkpath decodes the raw lexeme bytes into a&strviaencoding_rs(lossy: invalid sequences become U+FFFD) so the handler can read.text, thenserialize_selfre-emits that decoded string. Comments, tags and doctypes keep the original raw bytes and write them back when the handler has not mutated them; text chunks did not, so the decode-then-reencode round-trip was always applied.Fix
Patch the vendored lol-html (
patches/lolhtml/text-chunk-raw-passthrough.patch):TextChunkgainsraw: Option<Cow<'i, [u8]>>;set_str/as_mut_strclear it, andserialize_selfwritesrawverbatim when present.TextDecoder::feed_textpasses each decode step's raw input slice alongside the decoded&str. For UTF-8 it computes exactly which tail bytesencoding_rskept as internal state (an incomplete sequence at end-of-input, at most 3 bytes) viastd::str::from_utf8'serror_len() == None, which agrees with encoding_rs's WHATWG UTF-8 state machine on every lead/2nd-byte validity case. Those bytes are owned inTextDecoder::pending_rawand prefixed onto the next emitted chunk'sraw, so each chunk'srawis exactly the bytes itstextwas decoded from and the set of chunks the handler observes is unchanged.before/after/replace/removeare unaffected (they act onmutations, not on the chunk's own text).text.textstill returns the lossy-decoded string to JS.Verification
USE_SYSTEM_BUN=1 bun test test/js/workerd/html-rewriter.test.js: 28 of the 33 new tests fail (the 5 that pass are regression guards asserting mutation still works).bun bd test test/js/workerd/html-rewriter.test.js: 102 pass, 0 fail.bun bd testacross the other fourhtml-rewriter*test files: 11 pass, 0 fail.cargo test --libinvendor/lolhtml: 146 pass.The 33 new tests cover: lone continuation bytes; multiple invalid bytes; mixed valid/invalid UTF-8; truncated 2/3/4-byte sequences immediately before a close tag (the bytes encoding_rs holds as state and flushes on the
lastInTextNodechunk); a text node that is only a truncated lead byte; invalid bytes before and after the 1 KB fast-path cutoff;ScriptDatatext; multiple sibling text nodes;.textreads still seeing U+FFFD while output stays raw;before()/after()keeping the chunk body raw;replace()/remove()still dropping it; and the handler's observed chunk sequence pinned to today's behavior.The fix lives in
patches/andscripts/build/deps/(notsrc/) since the bug is in the vendored dependency, so theUSE_SYSTEM_BUN=1run above is the fail-before proof.[stamp-90s] gate passed · iteration 1 · 3 files touched
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 0 rejected · iteration 1
evidence per changed file