Sanitizer campaign: #1227 unwound and re-filed — needs human review and a soak

[jeremy]

Handoff for the six open PRs that replace #1227. Nothing is merged and nothing is released. Everything below is filed, verified, and waiting on people.

What happened

On 2026-08-11, #1225 and #1227 were merged to main in a single unattended session. The code was sound. The scope and the process were not:

• Stop configuring the shared DOMPurify, and give each editor its own allowlist #1227 carried six separable concerns in one branch — a dependency floor bump with security semantics, a new consumer CSP requirement, an allowlist widening, the instance isolation itself, per-editor config, and a build-tooling one-liner. 15 files, +603/−19, about 5× the repo's median PR (3 files, ~99 lines).
• Nobody could enumerate what was being merged. The Trusted Types policy and the dompurify bump were noticed hours later, while rewriting the PR description.
• No human review, and no manual verification.

The mechanical cost is specific to this repo: there is no CHANGELOG. Release bodies are auto-generated lists of PR titles, and in ~60 releases none has ever flagged a breaking change or a consumer action. Shipping 0.9.30 as-is would have shown consumers only "Stop configuring the shared DOMPurify…" — the CSP requirement and the dependency floor move invisible. Over-scoping the PR silently deletes the release note. Splitting it is what puts it back.

What was done

main is at 8c64aa45, which reverts #1227 whole and is CI-green. #1225 is untouched — one concern, four files, within house norms. This was free: nothing was published (npm and RubyGems are still on 0.9.29), and every consumer pins an exact version, so no consumer saw anything.

Re-filed as five draft PRs plus the rebased #1226. Merge in order:

	PR	Concern	Consumer action
1	#1228	prepare builds dist on install	⚠️ see below — does not work under yarn 1
2	#1229	dompurify ^3.3.0 → ^3.4.13 + url validation	javascript: in an attachment url now dropped
3	#1230	alt/width/height in attachment content	none
4	#1231	Own DOMPurify instance + per-editor allowlist	none
5	#1232	lexxy Trusted Types policy name	⚠️ CSP entry if you enforce TT
6	#1226	SAFE_FOR_XML on re-inflation	none

Only 5 genuinely depends on 4; they are stacked in merge order so each PR's diff is exactly its own concern.

The decomposition changed no production code — but the review round since then did, deliberately. As originally pushed, the stack's src/, package.json and yarn.lock were byte-identical to the reverted merge c1db29fd (the only differences being new docs and eight relocated unit tests). That is no longer true: fixing the data:-scoping security bug in #1229 changed src/config/dom_purify.js by ~64 lines, and #1232 changed a ternary to a conditional. Those are real, intended divergences from what #1227 shipped — see the review-round summary below. If you are auditing "what did the decomposition change vs the revert", diff against the first pushed SHA of each branch, not the current one.

integration/sanitizer-campaign (4c7d4126) is the whole stack as one unit; it is the same commit as #1226's head, so #1226's CI run is its CI run.

What the QA found

The two skipped steps — a real browser and a real consumer — each found something.

1. #1232's headline claim was false. Under require-trusted-types-for 'script' in Chromium, the editor fails to initialise whether or not lexxy is allowlisted, at html_helper.js:20 — Lexxy's own DOMParser.parseFromString, which no DOMPurify policy covers. There are ~10 such unwrapped sinks. This is long-standing and untouched by the series; what was wrong was the claim. Worse, #1227 had the caveat in a comment and the rewrite deleted it. Title, body, source comment and docs all corrected. Making Lexxy work under TT is separate work and wants its own issue.

2. #1228 does not work for bc3 or fizzy. yarn 1.22 does not run a git dependency's prepare; npm does. Both consumers use yarn 1.22, so github:basecamp/lexxy#<sha> installs with no dist/. My suggestion is to publish an alpha via the existing yarn release:alpha instead, which every package manager can install. Details and options on the PR.

3. #1231 fixes a live bug in bc3. app/javascript/rich_text/sanitize_attachment_content.js sanitizes with the DOMPurify singleton and a per-call config restricted to ["bc-mention","span","img"], on the chat pending-line and Trix paste paths. Under 0.9.29 that config is silently ignored once an editor connects — <a href> and <p> survive where bc3 intends them stripped. Demonstrated with bc3's real config; table on the PR.

4. #1226's system test proved nothing, and its hook may be dead code. Reverting that PR's entire src/ leaves its system test green. preserveSerializedContentHook can only fire on a nested attachment, which no production path currently produces. Two ways out — show the path is real, or drop the hook and keep the safeForXml: true opt-in — and the second also closes the open dompurify exact-pin thread. Needs a human call; detail on #1226.

5. Pre-existing, filed for information, not blocking: an attachment url with javascript: is dropped from the saved value but still reaches img[src] in the editor DOM. Not exploitable (browsers don't execute it there) and strictly better than before #1229.

What passed

• CI green on Build dist on install so a branch or SHA can be depended on #1228, Move to dompurify 3.4.13, and validate attachment URLs ourselves #1229, Keep an image's alternative text and intrinsic size in attachment content #1230, Give Lexxy its own DOMPurify instance, and an allowlist per editor #1231, Claim a "lexxy" Trusted Types policy instead of racing the host for "dompurify" #1232.
• Apply mXSS-safe SAFE_FOR_XML on attachment content re-inflation #1226 / integration/sanitizer-campaign (4c7d4126): green, 12/12. It first failed Browser tests (chromium) on leak.test.js; that is a pre-existing flake, not this branch — 4 runs on main at 8c64aa45 failed twice, once reporting a delta of -28 nodes, which is not a leak by any reading. Same 2-of-3 pattern locally on both branches. It passed on re-run with no code change. Worth deflaking separately: it will keep costing re-runs, and a test that reports negative leaks cannot detect a real one.
• Browser, Chromium, integration branch: two editors with differing configs each keep their own allowlist; the one denying content strips it and the one allowing it keeps it (the named must-pass regression); attachment data: urls survive and javascript: is dropped from the value; the CSP-refused path is caught and degrades as designed.
• bc3, against a build of the integration branch: 73 system tests and 132 rich-text/mention/attachment tests, 0 failures. Local only — the scratch worktree is restored to 0.9.29 and nothing was committed or pushed to bc3.

What still needs a human

1. External review of all six PRs. They are drafts with Copilot requested. This is the step whose absence caused all of the above.
2. Decide Build dist on install so a branch or SHA can be depended on #1228 — publish an alpha instead, or keep the one-liner with an honest title.
3. Decide Apply mXSS-safe SAFE_FOR_XML on attachment content re-inflation #1226 — prove the nested-attachment path, or drop the hook.
4. Soak. Then merge 1 → 2 → 3 → 4 → 5 → Apply mXSS-safe SAFE_FOR_XML on attachment content re-inflation #1226.
5. Release, and write the notes by hand this once. Titles carry the two consumer actions, but a release with a CSP requirement in it deserves more than a generated list.
6. Then bump bc3 — including moving the dompurify pin in both dependencies and resolutions, or bc3 stays on 3.4.0 regardless of what lexxy asks for. yarn warns; it does not fail.

Known accepted cost

lexxy is public (1,208 stars, 114 forks) and #1225/#1227 already publicly describe sanitizer bypasses that are still unfixed in the released 0.9.29. The revert does not close that window — only a release does. Deliberately not releasing yet means accepting it for the length of the soak. That is a real trade and someone should own it consciously rather than inherit it.

Cleanup

integration/qa-build (4b0c9b9d) is integration/sanitizer-campaign plus a force-added dist/, built solely so yarn 1 could install it for the bc3 QA. Not for merge. Delete it once the campaign lands.

[jeremy]

Review round: 11 findings across the six PRs

Copilot on all five new PRs, plus Copilot and Codex on #1226. Outcome: 6 fixed, 3 declined with reasoning, 1 escalated, 1 half-and-half. All branches rebased and re-pushed; integration branch is now 1259920a.

The one that mattered

#1229 had a real security widening, and it was not the one the PR was about. ADD_URI_SAFE_ATTR is attribute-name-wide, so the data: exception granted because an attachment's url becomes an <img src> reached every tag. Confirmed before fixing:

<x-widget url="data:text/html,<script>alert(1)</script>">   → survived intact

Any extension declaring url inherited it, and home/docs/extensions.md's own worked example is an <iframe>. The hook now reads currentNode: the attachment element keeps data:, everything else gets DOMPurify's standard policy. Mutation-checked both ways.

That one is worth noting for the campaign's own post-mortem: it was introduced by #1227, survived the original unattended merge, survived my decomposition, and was caught by a bot review — the step that had been skipped.

Fixed

• Move to dompurify 3.4.13, and validate attachment URLs ourselves #1229 — data: exception scoped to the attachment element (above); matrix: restored to the non-attachment path; the false "exactly what DOMPurify allows on an img src" claim corrected; new attachment_url_round_trip_test.rb covering editor → save → render → re-edit, which action_text_load_test stops short of.
• Keep an image's alternative text and intrinsic size in attachment content #1230 — the Action Text agreement check intersected the blanket allowlist with a hardcoded %w[ alt ], so a newly blanket-allowed attribute sat outside it while the header claimed otherwise. Now checks the whole list minus one pinned exemption. Also: the comment said Action Text drops style; it keeps it.
• Give Lexxy its own DOMPurify instance, and an allowlist per editor #1231 — added the content allow/deny pair the PR description names as the must-pass regression and did not actually test.
• Claim a "lexxy" Trusted Types policy instead of racing the host for "dompurify" #1232 — ternary → plain conditional per STYLE.md:60; added unit coverage for the CSP-refusal and no-Trusted-Types paths, which the PR claimed to have verified but had only checked by hand.

Declined, with reasoning on the threads

• Build dist on install so a branch or SHA can be depended on #1228 ×2 — prepack/prepublishOnly would defeat the PR's only purpose (neither runs for git installs); and prepare calling yarn build:npm would make a consumer's install depend on a package-manager binary to save one duplicated string. Both threads note the bigger open question: the script doesn't work under yarn 1 anyway.
• Give Lexxy its own DOMPurify instance, and an allowlist per editor #1231 — a multi-editor Capybara round trip. The server leg is editor-agnostic: Loofah sees one HTML string and has no concept of how many editors produced it, so it would duplicate the existing round-trip coverage. The multi-editor half is covered in a real browser by the Playwright fixture.
• Keep an image's alternative text and intrinsic size in attachment content #1230 — declined dropping the source-read for an explicit Ruby list, which STYLE.md:319 would otherwise argue for. The property is that two allowlists in two languages agree; a Ruby copy keeps passing while Lexxy widens underneath it, converting a caught drift into an uncaught one. Reasoning is now in the file. Left unresolved — reasonable people may take the other side.

Escalated — this is the one blocking #1226

Four independent findings are the same finding, so I stopped rather than writing a fourth variation. Codex ×1, Copilot ×2, and my own mutation test all say preserveSerializedContentHook is not on a path the code takes. Full write-up: #1226 (comment)

The decision is binary and someone has to make it: prove nested attachments are a real shape and add the failing case, or drop the hook and keep the independently-justified safeForXml: true opt-in. Option 2 also dissolves the dompurify exact-pin thread, now raised twice, because XML_UNSAFE_ATTRIBUTE_VALUE exists only to serve that hook. My read is option 2; I have not acted on it.

Verification after the rebase

161 vitest, 89 Rails runs / 349 assertions, 0 failures across both suites, yarn lint and bin/rubocop clean. Each commit in the stack checked individually so no intermediate commit is broken. CI re-running.

[jeremy]

CI settled after the review round: 12/12 green on all six PRs — #1228, #1229, #1230, #1231, #1232, #1226 — including both Rails system-test legs and all three browser legs. No re-runs needed this time; the leak.test.js chromium flake didn't recur.

integration/sanitizer-campaign is 1259920a, the same commit as #1226's head, so that CI run covers the integration branch too.

Still nothing merged and nothing released: main is at the revert (8c64aa45) and 0.9.29 remains the published version. The blocking decision is unchanged — the preserveSerializedContentHook call on #1226, plus the #1228 question.

[jeremy]

Made the calls I'd been parking

Directed to stop deferring judgment calls to a human oracle when I have the full frame, I've now acted on them. Every review thread across all six PRs is resolved.

#1226 — dropped the hook (214e8dac). This was the one genuinely blocking decision, and I'd been treating it as above my pay grade when the analysis was already conclusive and four reviewers agreed. Removed preserveSerializedContentHook, its registration, XML_UNSAFE_ATTRIBUTE_VALUE, and the synthetic outer-element tests — net −148 lines. Confirmed safe twice (reverting all of src leaves the system test green; disabling just the hook leaves the real-path re-inflation test green), and it's the safer default besides: for a nested attachment the hook would force-keep an XML-unsafe value on an element never re-sanitized by its own node. Kept the safeForXml opt-in, which stands on its own. This also closed the two dompurify-exact-pin threads — deleting the hook deleted the mirror of the private predicate, so there's nothing left to pin against.

So #1226 is now what it should have been: turn on mXSS-safe mode for re-inflation, plus its real-path test. Nothing else.

Engineering-judgment declines, resolved with reasoning as the final word (not parked): #1228 ×2 (prepack defeats the purpose; build:npm reuse taxes consumer installs), #1230 (source-read stays — it's the only thing that catches cross-language drift), #1231 (multi-editor server leg is editor-agnostic; both sides of the seam already covered), #1226 Playwright-repro (repo rule routes to Capybara; I can't produce a real mutation payload, and a "nothing executed" test passes for the wrong reason), and the new fixture thread (hello_james is the sanctioned mention fixture; the rule doesn't fit round-trip tests of saved attachments).

One convention note for a human, deliberately not actioned by me: AGENTS.md:53-55 lists only empty/hello_world as round-trip starting points, but three attachment round-trip tests legitimately need a fixture carrying a saved attachment (hello_james). A one-line clarification would stop that thread recurring on every such PR. I left it out of #1226 rather than expand a scoped PR to edit a repo-wide priors doc — flagging it here instead.

Net effect on the campaign: the two blocking decisions in my earlier "what needs a human" list are now made — the hook is dropped (my call, reversible, consensus-backed), and the only remaining PR-level open question is #1228 (keep the script vs publish an alpha), which is a release-mechanics choice I've recommended on but which genuinely wants an owner. Everything else is reviewed, resolved, and green.

Re-verifying CI on the hook-removal push now.