[robobun]

Problem

• comment.text = s rejects --> only. The HTML tokenizer also ends a comment at --!>, and at a leading > or ->. After one of these, the rest of s is markup, for example a live <img onerror>.
• The cause is lol-html Comment::set_text (vendor/lolhtml/src/rewritable_units/tokens/comment.rs:70), which checks only text.contains("-->"). Its error: Comment text shouldn't contain comment closing sequence (`-->`). Upstream fixed it in cloudflare/lol-html@ba25359 (v3.0.0, a security fix).

Fix

• Pin oven-sh/lol-html at a4a3d0c: main's pin (725ce49) plus a cherry-pick of that commit, one commit in one file.
• The setter now throws Comment text shouldn't contain a comment-closing sequence. for all four forms. Near misses (--!, x--, a->b) stay accepted.
• Verified: test/js/workerd/html-rewriter.test.js (8 of 19 new cases fail on bun 1.4.3), the other html-rewriter* suites, bundler_html.

Background

• HTMLRewriter binds lol-html, built from the oven-sh/lol-html fork at the commit in scripts/build/deps/lolhtml.ts. A comment has no escape syntax, so the setter can only reject.
• The fork's bun branch is now on v3.0.1 and has upstream's ba25359. If main moves there (HTMLRewriter: match nested and compound :not() selectors (lol-html v3.0.1) #43266), this fix comes with it, and only the tests here stay useful.

Downsides

• Text that was accepted now throws, for example comment.text = "> quoted". An uncaught throw fails the rewrite.
• The error text for --> changed. Code that matches the old string no longer matches.
• comment.text = scans the text once more: 31 to 50 ns per call at 40 bytes (lol-html release build).

Notes

Repro on bun 1.4.3-canary.1 (367d939). Main at bf80d21 pins the same lol-html commit.

const set = s => new HTMLRewriter().on("p", { comments(c) { c.text = s; } }).transform("<p><!--c--></p>");
set("x--><img src=x onerror=alert(1)>");  // throws
set("x--!><img src=x onerror=alert(1)>"); // <p><!--x--!><img src=x onerror=alert(1)>--></p>
set("><img src=x onerror=alert(1)>");     // <p><!--><img src=x onerror=alert(1)>--></p>
set("-><img src=x onerror=alert(1)>");    // <p><!---><img src=x onerror=alert(1)>--></p>

With this PR all four throw.

Why four forms: after <!-- the tokenizer is in the comment start state. > there, or -> (comment start dash state), emits an empty comment ("abrupt closing of empty comment"). Later, -- moves to the comment end state, where > emits the comment and ! moves to the comment end bang state, where > also emits it ("incorrectly closed comment"). Every other path appends to the comment data. lol-html's own parser follows the same states, which is what the second parse in the test shows.

The two --> cases in the new test fail on 1.4.3 only because upstream reworded the error. The other six fail because the setter does not throw.

Pin history. The first version of this PR pinned ef9f6fd, the same cherry-pick on top of ea6125b (linear-depth descendant matching, #41479), because ea6125b was the head of the fork's bun branch then. That commit slows selector matching on ordinary pages, which is #41479's trade to make, not this PR's. The fork's bun branch has since been rebased onto v3.0.1, so no v2.7.2-line commit can be on it. The pin is now main's own commit plus the one cherry-pick. It lives on the fork branch robobun/comment-closing-sequences-v2.7.2. oven-sh/lol-html#6 (the PR against the old bun head) is closed, because the new bun already has upstream's commit.

The cherry-pick differs from upstream ba25359 in two places (git range-diff): the CHANGELOG.md hunk is dropped (no v3.0.0 section on this line), and set_text keeps the v2.7.2 BytesCow::from_str_without_replacements(..) plus into_owned(). lol-html checks on a4a3d0c: cargo test --features=integration_test 170 + 3 + 32 + 2 pass.

Cost numbers: a release build (LTO) of lol-html alone, 20,000 comments per document, Rust handler calls set_text on each, median of 41 runs, three rounds, old and new alternated. Per call, set_text goes from 30.9 to 49.6 ns (40 bytes), 82 to 114 ns (400 bytes), 2.13 to 2.25 us (4 KB). In Bun the JS handler call around it costs far more than either.

This pin edits the same line of lolhtml.ts as #41475, #41479, #41968 and #43266.

---

no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/workerd/html-rewriter.test.js

[robobun]

Status: reproduced on bun 1.4.3-canary.1. comment.text = "x--!><img src=x onerror=alert(1)>", and the same with a leading > or ->, is accepted, and a second parse of the output sees an img element with src and onerror. The fix is upstream commit cloudflare/lol-html@ba25359. This PR pins a4a3d0c, which is the commit main pins now (725ce49) plus that one cherry-pick (fork diff, one file). The new cases in test/js/workerd/html-rewriter.test.js fail on stock bun (8 of 19) and pass with the pin.

[robobun]

^{Updated 8:32 AM PT - Sep 22nd, 2026}

✅ @robobun, your commit 1678db9a664754e4efc62d9ff21ee341874b3236 passed in Build #119765! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 42646

That installs a local version of the PR into your bun-42646 executable, so you can run:

bun-42646 --bun

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 12bb7baa-6531-425b-a170-cb85c09fb71a

📥 Commits

Reviewing files that changed from the base of the PR and between 2abf36b and 1678db9.

📒 Files selected for processing (2)

• scripts/build/deps/lolhtml.ts
• test/js/workerd/html-rewriter.test.js

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.

---

Walkthrough

The PR updates the pinned lol-html commit and adds tests for comment.text validation and round-tripping.

Changes

lol-html comment validation

Layer / File(s)	Summary
Dependency update and comment validation scripts/build/deps/lolhtml.ts, test/js/workerd/html-rewriter.test.js	The pinned lol-html commit changes. Tests reject invalid comment terminators and verify that valid comment text round-trips without creating extra elements.

Suggested reviewers: jarred-sumner

Priority: ➖ Normal

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the main change: rejecting all comment-closing sequences assigned through HTMLRewriter comment.text.
Description check	✅ Passed	The description explains the problem, fix, verification, security impact, dependency pin, and trade-offs. It does not use the template headings exactly, but it contains the required information and is…

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

I reviewed this and didn't find any bugs. Because the actual behavioral change lives in the oven-sh/lol-html fork commit (a cherry-pick with two hand-resolved conflicts, and which also pulls in ea6125b), a human should eyeball that fork PR before landing — this diff only shows the pin and the tests.

What was reviewed:

• LOLHTML_COMMIT bump: constant name unchanged so .github/workflows/update-lolhtml.yml's sed still matches; no other occurrences of the old hash remain in the repo.
• New it.each blocks in html-rewriter.test.js: the four rejected forms match the HTML tokenizer's comment-exit states (-->, --!>, leading >/->), the exact error message is asserted, and each accepted near-miss is round-tripped through a second parse to prove exactly one comment and no extra elements — the combined-object .toEqual and negative-contract check follow the repo's test conventions.

<details>
<summary>Extended reasoning...</summary>

Overview

The PR bumps the pinned oven-sh/lol-html fork commit in scripts/build/deps/lolhtml.ts from 725ce499… to ef9f6fd0… and adds a 33-line test block to test/js/workerd/html-rewriter.test.js. The fork commit is described as a cherry-pick of upstream cloudflare/lol-html@ ba25359 (a v3.0.0 security fix) onto the fork's v2.7.2-based bun branch, tightening Comment::set_text to reject --!> anywhere and a leading > or -> in addition to the already-rejected -->. The pin also picks up an unrelated fork commit (ea6125b for #41479) because the branch is linear.

Security risks

The change closes an HTML injection / XSS vector: before this bump, comment.text = "x--!><img onerror=…>" would let the <img> escape the comment. The direction is strictly tightening validation on user-controlled input, so the risk is not that the new check is unsafe but that (a) the cherry-pick onto the fork was resolved incorrectly (the PR notes two manual conflict resolutions in set_text and CHANGELOG.md), or (b) the pin picks up other unreviewed changes on the fork branch. Neither is visible in this repo's diff — the code that actually enforces the check lives in vendor/lolhtml and is fetched by commit hash at build time.

Level of scrutiny

Medium-high. The in-repo diff is trivial and the tests are well-constructed (parameterized, exact error message, accepted near-misses with a re-parse round-trip proving no element leaks, single combined .toEqual). But per the repo's dependency-bump guidance, a human should confirm the fork commit is merged to the bun branch (not an ephemeral PR head) and that the cherry-pick's conflict resolution in rewritable_units/tokens/comment.rs matches upstream's semantics — that review has to happen against oven-sh/lol-html#6, not this diff.

Other factors

No CODEOWNERS entries cover scripts/build/deps/ or test/js/workerd/. The old commit hash has no remaining references anywhere in the repo, and the LOLHTML_COMMIT constant name is unchanged so the update-lolhtml.yml workflow's sed pattern continues to match. The PR notes textual conflicts with #41479 and #41968 on the same line, which whoever merges second will need to re-pin. The test placement (existing html-rewriter.test.js, inside the existing "tagName, endTag.name, and comment.text setters" describe block) follows the repo's test-organization rules.

</details>

[robobun]

For the review of the fork side, these are the two things to look at.

What the pin moves over. 725ce49...ef9f6fd holds two commits:

• ea6125b: already on the fork's bun branch (selectors_vm: make stray end tags and descendant matching linear in depth (cherry-pick of upstream 87f7987) lol-html#2, merged). Bump lol-html so descendant selector matching is linear in nesting depth #41479 pins it.
• ef9f6fd: the head of Reject all comment-closing sequences in Comment::set_text (cherry-pick of upstream ba25359) lol-html#6, which is open. It is a fast-forward of bun, so a merge without squash keeps this sha and puts the pin on the bun branch.

How the cherry-pick differs from upstream. git range-diff of upstream ba25359 against ef9f6fd shows two differences and nothing else:

• The CHANGELOG.md hunk is dropped (the branch has no v3.0.0 section).
• In src/rewritable_units/tokens/comment.rs, the moved set_text body keeps the two lines this branch already had: BytesCow::from_str_without_replacements(text, self.encoding) and self.text = text.into_owned(). Upstream has owned_from_str_without_replacements(..) and self.text = text there, an API that v2.7.2 does not have.

The new check (contains_comment_closing_sequence), the error text, and the upstream unit test are byte for byte the same as upstream. The lol-html cargo test job on this PR ran them against the pinned commit and passed.

[robobun]

The pin changed in 1678db9, and two statements in my earlier comment no longer hold.

• New pin: a4a3d0c is 725ce49 (the commit main pins now) plus the one cherry-pick of cloudflare/lol-html@ba25359. The fork diff is one commit in src/rewritable_units/tokens/comment.rs. The old pin ef9f6fd also carried ea6125b (linear-depth descendant matching, Bump lol-html so descendant selector matching is linear in nesting depth #41479). That commit changes selector matching cost on ordinary pages, so it stays with Bump lol-html so descendant selector matching is linear in nesting depth #41479 and is no longer part of this PR.
• The fork's bun branch: it was rebased onto v3.0.1 on 2026-09-17 and now has upstream's own ba25359. So no commit of the v2.7.2 line can be "a fast-forward of bun" any more, and Reject all comment-closing sequences in Comment::set_text (cherry-pick of upstream ba25359) lol-html#6 has nothing left to merge. I closed it. The pinned commit lives on the fork branch robobun/comment-closing-sequences-v2.7.2.
• Relation to HTMLRewriter: match nested and compound :not() selectors (lol-html v3.0.1) #43266: that PR moves main to the v3.0.1 line, which has the same set_text check. If it lands first, only the tests here stay useful. They assert upstream's error text, so they hold on both lines.

Verified again with the new pin: test/js/workerd/html-rewriter.test.js (205 pass, the 19 new cases included, 8 of them fail on stock bun), html-rewriter-end-error, html-rewriter-leak, html-rewriter-doctype, htmlrewriter-additional-bugs, bundler_html. lol-html's own suite on a4a3d0c: 170 + 3 + 32 + 2 pass.

[claude]

I reviewed the re-pinned version of this PR and found no bugs in the diff itself; because the behavior change lives in a fork commit outside this repo and the PR text no longer matches the code, a human should confirm the pin before merging.

What was reviewed:

• scripts/build/deps/lolhtml.ts: the old 725ce499 pin has no other occurrences in the repo (grep), so nothing else needs updating; LOLHTML_UPSTREAM_BASE stays at v2.7.2, which is consistent with a cherry-pick rather than a rebase.
• The PR description still says the pin is ef9f6fd, but the diff pins a4a3d0ce (per the latest commit message: 725ce49 + cherry-pick of cloudflare/lol-html ba25359 only, without ea6125b). I could not fetch the fork from this checkout, so whether a4a3d0ce contains exactly that is unverified here — worth a quick look at oven-sh/lol-html, and the description should be updated to match.
• html-rewriter.test.js: the new cases assert the exact new error message, and no existing test asserts the old (\-->`)wording. The accepted-list round-trip through a second parse checks the negative contract. The-->cases fail on the old pin only because of the reworded message; the--!>/>/->` cases are the real regression coverage.

Extended reasoning...

The diff is a one-line lol-html fork commit bump in scripts/build/deps/lolhtml.ts plus 19 new test cases in test/js/workerd/html-rewriter.test.js; no src/ code changes. It touches a security-relevant surface: HTMLRewriter comment.text previously let --!>, leading > and leading -> terminate the comment and inject markup, and the fix is a cherry-picked upstream security commit. Deferring rather than approving because the fixed code is in the vendored fork commit that is not in this checkout and could not be fetched here, the PR description names a different SHA (ef9f6fd) than the one pinned (a4a3d0ce), and the PR's own evidence block reports no local test run (deferred to CI).

[robobun]

Two answers for the reviewer of the pin.

The description. It names a4a3d0c in the Fix section, with the compare link. I updated it a few minutes after the push, so the review read the earlier text. ef9f6fd now appears only in the Notes, as the earlier pin.

What a4a3d0c contains. 725ce49...a4a3d0c is 1 commit ahead and 0 behind, and it changes one file: src/rewritable_units/tokens/comment.rs (+46 -22). That file is byte for byte the same as at ef9f6fd. Against upstream ba25359 the patch differs only in the two BytesCow lines of set_text that the v2.7.2 base needs, and the CHANGELOG.md hunk is dropped. ea6125b is not in the range.

The lol-html cargo test job on this head ran lol-html's suite against the pinned commit and passed.

[robobun]

A note on the pin here. ef9f6fd sits on ea6125b, the v2.7.2 cherry-pick of lol-html 87f7987. That commit updates a map of open element names for every element, and an ordinary page pays for it: +6% to +19% in a Bun perf run, +11% to +20% in lol-html alone on a 1 MiB page.

oven-sh/lol-html#11 removes that cost (the map exists only while the open stack is deeper than 64) and keeps the pathological inputs linear. It is on the v3.0.1 line of the fork, which #43266 moves Bun to. #43266 also brings the comment.text change of this PR (it is in lol-html 3.0.0) with the same tests.

If this PR is wanted on the v2.7.2 line instead, the one commit of oven-sh/lol-html#11 needs a backport onto ef9f6fd first. It touches only src/selectors_vm/stack.rs.