Apply mXSS-safe SAFE_FOR_XML on untrusted re-inflation paths

[jeremy]

What

Stored Trix HTML is re-inflated back into the editor on load (editor.loadHTML) and on reparse (composition.replaceHTML). Both parsed through DOMPurify with the default config, which leaves SAFE_FOR_XML off — so the untrusted storage round-trip is not defended against mutation-XSS. composition.insertHTML already opts into { purifyOptions: { SAFE_FOR_XML: true } } per call; this extends the same idiom to the two remaining re-inflation entry points.

Why this is scoped per-call, not a global flip

A global SAFE_FOR_XML: true in config/dompurify.js would reintroduce the regression fixed in #1213. Attachment content is serialized into the data-trix-attachment attribute, and with config.action_view.annotate_rendered_view_with_filenames on, that content carries Rails view-annotation comments like <!-- BEGIN app/views/users/_user.html.erb -->. DOMPurify's SAFE_FOR_XML attribute-value guard drops any attribute whose value contains a comment terminator (-->, --!>, ]>, or a raw </style-style close), so the whole data-trix-attachment attribute is stripped and the attachment silently disappears on the round-trip. That guard runs before DOMPurify consults forceKeepAttr, so the existing data-trix keep-hook alone does not save it.

The preservation hook

The uponSanitizeAttribute hook in html_sanitizer.js now, for data-trix-* attributes, neutralizes only the copy DOMPurify inspects for its XML-safety guard and then sets forceKeepAttr, which keeps the original value verbatim (the neutralized copy is never written to the DOM). These are data attributes — always entity-escaped on serialization, never re-parsed as markup — so retaining them is mXSS-safe. The neutralization is scoped to data-trix-*; XML-unsafe values on ordinary attributes are still stripped by SAFE_FOR_XML.

Changes

• models/editor.js — loadHTML parses with SAFE_FOR_XML: true
• models/composition.js — replaceHTML parses with SAFE_FOR_XML: true
• models/html_sanitizer.js — preservation hook for serialized data-trix-* attribute values

Tests

• html_sanitizer_test.js — data-trix-* comment markers preserved under SAFE_FOR_XML; XML-unsafe values on non-data-trix attributes still stripped (scoping proof)
• serialization_test.js — round-trip: mXSS payload neutralized; attachment comment survives
• attachment_test.js — re-inflation keeps a comment-bearing attachment and leaves its content byte-identical

yarn test (web-test-runner / Playwright Chromium): 506 tests, 477 passed, 0 failed, 29 pre-existing skips.

Follow-up (deliberately held)

npm publish + the bc3 consumer bump are gated follow-ups and are not part of this PR.

[Copilot]

Pull request overview

Hardens stored HTML re-inflation with DOMPurify’s XML-safe mode while attempting to preserve serialized attachment comments.

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

Changes:

• Enables SAFE_FOR_XML for editor loading and composition reparsing.
• Adds preservation logic for data-trix-* attributes.
• Adds sanitizer, serialization, and attachment regression tests.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/trix/models/html_sanitizer.js	Preserves serialized Trix attributes under XML-safe sanitization.
src/trix/models/editor.js	Enables XML-safe parsing when loading HTML.
src/trix/models/composition.js	Enables XML-safe parsing during DOM reparsing.
src/test/unit/serialization_test.js	Tests safe parsing and serialization round-trips.
src/test/unit/html_sanitizer_test.js	Tests XML-unsafe attribute handling.
src/test/unit/attachment_test.js	Tests preservation of comment-bearing attachments.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jeremy]

This finding doesn't hold up — the change introduces no regression on the attachment-content path. Details, since it's the load-bearing concern here:

The data-trix-attachment attribute already survived this parse before the PR. On main, editor.loadHTML / composition.replaceHTML parse under the base config (SAFE_FOR_XML: false), and the pre-existing uponSanitizeAttribute hook already forceKeepAttrs every data-trix-* attribute. So the attachment attribute — content and all — passed through the outer parse identically before this change. Enabling SAFE_FOR_XML: true here would otherwise drop that attribute (the #1213 regression); the neutralization exists solely to keep preservation matching main. It exposes nothing that main didn't already keep.

forceKeepAttr keeps the original value verbatim — the neutralized copy is never written. In DOMPurify 3.4.2 (purify.cjs.js): the SAFE_FOR_XML guard at L1195 tests the hook-mutated copy (which we empty of XML-unsafe sequences), and forceKeepAttr at L1205 continues before the write-back at L1248, so the attribute node is never rewritten. The original value stays on the node; the neutralized copy is discarded. This is also asserted directly by the attachment tests, which check getContent() equals the original comment-bearing content byte-for-byte.

AttachmentView's content re-parse is unchanged by this PR. AttachmentView.createNodes still calls HTMLSanitizer.setHTML(content) under the base config (SAFE_FOR_XML: false) exactly as on main. That path is independently covered by the existing, passing paste data-trix-attachment unsafe html div overload system test.

End-to-end evidence. Loading <div data-trix-attachment='{"content":"</style><img src=x onerror=…>"}'></div> through editor.loadHTML on this branch renders the content to a benign <img src="x"> — zero elements with an event-handler attribute, and the payload never executes — verified in Chromium, Firefox, and WebKit. The onerror persists only as inert, entity-escaped text inside the serialized data-trix-attachment JSON; it is never live markup.

Applying SAFE_FOR_XML to the attachment-content re-parse itself would be a separate hardening on a different path (and would need its own #1213-style preservation, since content there is HTML rather than a data-trix-* attribute) — out of scope for this PR, which mirrors the existing per-call SAFE_FOR_XML idiom (insertHTML) on the loadHTML/replaceHTML re-inflation entry points. Leaving this thread open for a maintainer's eyes given its severity label, but the described regression does not occur.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 51d9b84c8c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Keep SAFE_FOR_XML active for attachment markup

When stored or pasted HTML contains an attacker-controlled data-trix-attachment, replacing the XML-unsafe sequences in the hook event and then force-keeping the original attribute bypasses the very SAFE_FOR_XML check enabled by this change. The value is not merely inert data: HTMLParser.processElement JSON-parses it, and AttachmentView.createNodes reparses its content as markup via HTMLSanitizer.setHTML with the default SAFE_FOR_XML: false. This therefore reopens the mutation-XSS class covered by the existing paste data-trix-attachment unsafe html div overload system test—particularly for supported resolutions such as DOMPurify 3.2.x—so the exemption should not retain arbitrary raw closing-tag sequences just to preserve comment-bearing attachment content.

Useful? React with 👍 / 👎.

[jeremy]

This finding doesn't hold up — the change introduces no regression on the attachment-content path. Details, since it's the load-bearing concern here:

The data-trix-attachment attribute already survived this parse before the PR. On main, editor.loadHTML / composition.replaceHTML parse under the base config (SAFE_FOR_XML: false), and the pre-existing uponSanitizeAttribute hook already forceKeepAttrs every data-trix-* attribute. So the attachment attribute — content and all — passed through the outer parse identically before this change. Enabling SAFE_FOR_XML: true here would otherwise drop that attribute (the #1213 regression); the neutralization exists solely to keep preservation matching main. It exposes nothing that main didn't already keep.

forceKeepAttr keeps the original value verbatim — the neutralized copy is never written. In DOMPurify 3.4.2 (purify.cjs.js): the SAFE_FOR_XML guard at L1195 tests the hook-mutated copy (which we empty of XML-unsafe sequences), and forceKeepAttr at L1205 continues before the write-back at L1248, so the attribute node is never rewritten. The original value stays on the node; the neutralized copy is discarded. This is also asserted directly by the attachment tests, which check getContent() equals the original comment-bearing content byte-for-byte.

AttachmentView's content re-parse is unchanged by this PR. AttachmentView.createNodes still calls HTMLSanitizer.setHTML(content) under the base config (SAFE_FOR_XML: false) exactly as on main. That path is independently covered by the existing, passing paste data-trix-attachment unsafe html div overload system test.

End-to-end evidence. Loading <div data-trix-attachment='{"content":"</style><img src=x onerror=…>"}'></div> through editor.loadHTML on this branch renders the content to a benign <img src="x"> — zero elements with an event-handler attribute, and the payload never executes — verified in Chromium, Firefox, and WebKit. The onerror persists only as inert, entity-escaped text inside the serialized data-trix-attachment JSON; it is never live markup.

Applying SAFE_FOR_XML to the attachment-content re-parse itself would be a separate hardening on a different path (and would need its own #1213-style preservation, since content there is HTML rather than a data-trix-* attribute) — out of scope for this PR, which mirrors the existing per-call SAFE_FOR_XML idiom (insertHTML) on the loadHTML/replaceHTML re-inflation entry points. Leaving this thread open for a maintainer's eyes given its severity label, but the described regression does not occur.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/trix/models/html_sanitizer.js:34

• This bypasses the XML-safety check for the entire data-trix-attachment JSON value, but attachment content is not terminal data: html_parser.js:198 parses it into an attachment and attachment_view.js:38 later reparses it as HTML with the repository default SAFE_FOR_XML: false (config/dompurify.js:3). An attacker can therefore place an mXSS sequence in serialized attachment content, have this hook preserve it past the protected outer parse, and reach a second unprotected parse. Apply SAFE_FOR_XML: true when rendering attachment content (and test a nested mXSS payload through the actual editor render), or narrowly preserve only data that cannot be reparsed as markup.

    data.attrValue = data.attrValue.replace(XML_UNSAFE_ATTRIBUTE_VALUE, "")
    data.forceKeepAttr = true

src/test/unit/serialization_test.js:20

• This helper manually supplies the new option instead of exercising either changed production entry point. Because Editor.loadHTML and Composition.replaceHTML only add this wiring, these tests still pass if either production change is removed. Add system coverage that sends the payload through editor.loadHTML and through a DOM mutation that invokes replaceHTML, then verifies the rendered/serialized result.

  const reinflate = (html) => {
    const document = HTMLParser.parse(html, { purifyOptions: { SAFE_FOR_XML: true } }).getDocument()
    return serializeToContentType(document, "text/html")

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/trix/models/html_sanitizer.js:34

• This bypass leaves nested attachment HTML outside the new mXSS protection. data-trix-attachment is JSON-decoded by html_parser.js:41-44, then its content is re-parsed by AttachmentView through HTMLSanitizer.setHTML without SAFE_FOR_XML (attachment_view.js:37-38), while the default remains false. An mXSS payload inside content therefore skips the outer guard here and reaches another unsafe sanitize/serialize/reparse cycle; the new system test covers only an ordinary onerror payload. Apply SAFE_FOR_XML to that second parse and add a nested mXSS regression test before exempting this attribute.

    data.attrValue = data.attrValue.replace(XML_UNSAFE_ATTRIBUTE_VALUE, "")
    data.forceKeepAttr = true