From dfc8c45b1d27751e8768aafbcd370a9335832cbf Mon Sep 17 00:00:00 2001
From: Jeremy Daer <jeremy@37signals.com>
Date: Tue, 8 Sep 2026 23:12:02 +0200
Subject: [PATCH] Apply mXSS-safe SAFE_FOR_XML on untrusted re-inflation paths

Stored Trix HTML is re-inflated back into the editor on load
(editor.loadHTML) and on reparse (composition.replaceHTML). Both parse
under DOMPurify's default config, which leaves SAFE_FOR_XML off and so
does not defend the storage round-trip against mutation XSS. insertHTML
already opts into SAFE_FOR_XML per call; extend the same idiom to the two
remaining re-inflation entry points.

Scope it per call rather than flipping config.dompurify globally, which
would reintroduce #1213: attachment content serialized into
data-trix-attachment can carry Rails view-annotation comments, and
SAFE_FOR_XML drops any attribute whose value contains a comment
terminator, silently deleting the attachment. The uponSanitizeAttribute
hook runs that guard against a neutralized copy of the value so
forceKeepAttr keeps the original.

The tests assert the security invariant rather than element shape: no
event-handler attribute, no <script>, no execution. What a neutralized
payload parses into differs between browsers; whether it can execute does
not.
---
 .../app/assets/javascripts/trix.js            | 19 ++++++++-
 src/test/system.js                            |  1 +
 src/test/system/reinflation_security_test.js  | 39 +++++++++++++++++++
 src/test/unit/attachment_test.js              | 14 +++++++
 src/test/unit/html_sanitizer_test.js          | 17 ++++++++
 src/test/unit/serialization_test.js           | 31 +++++++++++++++
 src/trix/models/composition.js                |  3 +-
 src/trix/models/editor.js                     |  5 ++-
 src/trix/models/html_sanitizer.js             |  9 +++++
 9 files changed, 134 insertions(+), 4 deletions(-)
 create mode 100644 src/test/system/reinflation_security_test.js

diff --git a/action_text-trix/app/assets/javascripts/trix.js b/action_text-trix/app/assets/javascripts/trix.js
index 7951d35db..25067fc36 100644
--- a/action_text-trix/app/assets/javascripts/trix.js
+++ b/action_text-trix/app/assets/javascripts/trix.js
@@ -4248,6 +4248,12 @@ $\
   }
   var purify = createDOMPurify();
 
+  // Mirrors DOMPurify's own SAFE_FOR_XML attribute-value check. Trix serializes attachment
+  // content into data-trix-attachment, and that content legitimately carries HTML comments
+  // (basecamp/trix#1213), so the guard runs against a neutralized copy while forceKeepAttr
+  // keeps the original value. Attribute values serialize quote-delimited, so angle brackets
+  // inside them stay inert when the sanitized HTML is reparsed.
+  const XML_UNSAFE_ATTRIBUTE_VALUE = /((--!?|])>)|<\/(style|script|title|xmp|textarea|noscript|iframe|noembed|noframes)/gi;
   purify.addHook("uponSanitizeAttribute", function (node, data) {
     if (data.attrName === "data-trix-serialized-attributes") {
       data.keepAttr = false;
@@ -4255,6 +4261,7 @@ $\
     }
     const allowedAttributePattern = /^data-trix-/;
     if (allowedAttributePattern.test(data.attrName)) {
+      data.attrValue = data.attrValue.replace(XML_UNSAFE_ATTRIBUTE_VALUE, "");
       data.forceKeepAttr = true;
     }
   });
@@ -10213,7 +10220,12 @@ $\
       return this.notifyDelegateOfInsertionAtRange([startPosition, endPosition]);
     }
     replaceHTML(html) {
-      const document = HTMLParser.parse(html).getDocument().copyUsingObjectsFromDocument(this.document);
+      const parsedDocument = HTMLParser.parse(html, {
+        purifyOptions: {
+          SAFE_FOR_XML: true
+        }
+      }).getDocument();
+      const document = parsedDocument.copyUsingObjectsFromDocument(this.document);
       const locationRange = this.getLocationRange({
         strict: false
       });
@@ -10990,7 +11002,10 @@ $\
     loadHTML() {
       let html = arguments.length > 0 && arguments[0] !== undefined ? arguments[0] : "";
       const document = HTMLParser.parse(html, {
-        referenceElement: this.element
+        referenceElement: this.element,
+        purifyOptions: {
+          SAFE_FOR_XML: true
+        }
       }).getDocument();
       return this.loadDocument(document);
     }
diff --git a/src/test/system.js b/src/test/system.js
index 68fbd7d0b..b6d0f8257 100644
--- a/src/test/system.js
+++ b/src/test/system.js
@@ -18,5 +18,6 @@ import "test/system/list_formatting_test"
 import "test/system/morphing_test"
 import "test/system/mutation_input_test"
 import "test/system/pasting_test"
+import "test/system/reinflation_security_test"
 import "test/system/text_formatting_test"
 import "test/system/undo_test"
diff --git a/src/test/system/reinflation_security_test.js b/src/test/system/reinflation_security_test.js
new file mode 100644
index 000000000..cd6dab723
--- /dev/null
+++ b/src/test/system/reinflation_security_test.js
@@ -0,0 +1,39 @@
+import { assert, test, testGroup } from "test/test_helper"
+
+import { delay } from "../test_helpers/timing_helpers"
+
+// Drives the real re-inflation entry point rather than the sanitizer in isolation. The
+// assertions are about the security invariant, not element shape: browsers disagree on what
+// a neutralized payload parses into, but none of them may leave a live handler behind.
+testGroup("Re-inflation security (editor.loadHTML)", { template: "editor_empty" }, () => {
+  const loadAndAssertInert = async (html) => {
+    window.reinflationXSS = 0
+    getEditorElement().editor.loadHTML(html)
+    await delay(20)
+
+    const element = getEditorElement()
+    assert.equal(
+      element.querySelectorAll("[onerror], [onload], [onclick]").length, 0,
+      `live event handler survived re-inflation: ${element.innerHTML}`
+    )
+    assert.notOk(element.querySelector("script"), `script survived re-inflation: ${element.innerHTML}`)
+    assert.equal(window.reinflationXSS, 0, "re-inflated payload executed")
+
+    delete window.reinflationXSS
+  }
+
+  test("neutralizes a mutation-XSS payload loaded through the editor", async () => {
+    await loadAndAssertInert(
+      "<noscript><p title=\"</noscript><img src=x onerror=window.reinflationXSS=(window.reinflationXSS||0)+1>\">"
+    )
+  })
+
+  test("sanitizes attacker-controlled attachment content on re-inflation", async () => {
+    const attachment = {
+      contentType: "text/html5",
+      content: "</style><img src=x onerror=window.reinflationXSS=(window.reinflationXSS||0)+1>HELLO",
+    }
+    const html = `<div data-trix-attachment='${JSON.stringify(attachment).replace(/'/g, "&#39;")}'></div>`
+    await loadAndAssertInert(html)
+  })
+})
diff --git a/src/test/unit/attachment_test.js b/src/test/unit/attachment_test.js
index 190d682a0..5f7755a03 100644
--- a/src/test/unit/attachment_test.js
+++ b/src/test/unit/attachment_test.js
@@ -1,5 +1,6 @@
 import { assert, test, testGroup } from "test/test_helper"
 import Attachment from "trix/models/attachment"
+import HTMLParser from "trix/models/html_parser"
 
 testGroup("Attachment", () => {
   const previewableTypes = "image image/gif image/png image/jpg image/webp".split(" ")
@@ -26,4 +27,17 @@ testGroup("Attachment", () => {
     attrs = { previewable: false, contentType: previewableTypes[0] }
     assert.notOk(createAttachment(attrs).isPreviewable())
   })
+
+  // Regression: basecamp/trix#1213
+  test("parses an attachment with comment-bearing content under SAFE_FOR_XML", () => {
+    const content = "<!-- BEGIN app/views/users/_user.html.erb --><span>Chris</span><!-- END app/views/users/_user.html.erb -->"
+    const attributes = { contentType: "application/octet-stream", content, sgid: "abc123" }
+    const html = `<div><figure data-trix-attachment='${JSON.stringify(attributes)}'></figure></div>`
+
+    const document = HTMLParser.parse(html, { purifyOptions: { SAFE_FOR_XML: true } }).getDocument()
+    const attachments = document.getAttachments()
+
+    assert.equal(attachments.length, 1, "attachment was dropped during re-inflation")
+    assert.equal(attachments[0].getContent(), content, "attachment content comments were altered")
+  })
 })
diff --git a/src/test/unit/html_sanitizer_test.js b/src/test/unit/html_sanitizer_test.js
index d1dbab3c7..4d9ebd5cc 100644
--- a/src/test/unit/html_sanitizer_test.js
+++ b/src/test/unit/html_sanitizer_test.js
@@ -27,6 +27,23 @@ testGroup("HTMLSanitizer", () => {
     assert.ok(sanitized.includes("data-trix-attachment"))
   })
 
+  // Regression: basecamp/trix#1213. Attachment content can carry HTML comments, and
+  // SAFE_FOR_XML would otherwise drop the whole attribute for containing "-->".
+  test("preserves data-trix-* attribute values with comment markers under SAFE_FOR_XML", () => {
+    const content = "<!-- BEGIN app/views/users/_user.html.erb --><span>Chris</span><!-- END app/views/users/_user.html.erb -->"
+    const html = `<figure data-trix-attachment="${content.replace(/"/g, "&quot;")}"></figure>`
+    const sanitized = HTMLSanitizer.sanitize(html, { purifyOptions: { SAFE_FOR_XML: true } }).body.innerHTML
+    assert.ok(sanitized.includes("data-trix-attachment"), `attachment attribute lost: ${sanitized}`)
+    assert.ok(sanitized.includes("BEGIN app/views/users/_user.html.erb"), `comment marker lost: ${sanitized}`)
+  })
+
+  test("still strips XML-unsafe values on non-data-trix attributes under SAFE_FOR_XML", () => {
+    const html = "<div class=\"foo--></div><img src=x onerror=alert(1)>\">hi</div>"
+    const sanitized = HTMLSanitizer.sanitize(html, { purifyOptions: { SAFE_FOR_XML: true } }).body.innerHTML
+    assert.notOk(/onerror/i.test(sanitized), `mXSS payload survived: ${sanitized}`)
+    assert.notOk(sanitized.includes("foo--"), `XML-unsafe class value survived: ${sanitized}`)
+  })
+
   test("keeps custom tags configured for DOMPurify", () => {
     const config = {
       ADD_TAGS: [ "custom-tag" ],
diff --git a/src/test/unit/serialization_test.js b/src/test/unit/serialization_test.js
index 8f844b4b5..fd29448aa 100644
--- a/src/test/unit/serialization_test.js
+++ b/src/test/unit/serialization_test.js
@@ -1,4 +1,5 @@
 import { serializeToContentType } from "trix/core/serialization"
+import HTMLParser from "trix/models/html_parser"
 import { assert, eachFixture, test, testGroup } from "test/test_helper"
 
 testGroup("serializeToContentType", () => {
@@ -10,3 +11,33 @@ testGroup("serializeToContentType", () => {
     }
   })
 })
+
+testGroup("re-inflation round-trip (SAFE_FOR_XML)", () => {
+  const reinflate = (html) => {
+    const document = HTMLParser.parse(html, { purifyOptions: { SAFE_FOR_XML: true } }).getDocument()
+    return serializeToContentType(document, "text/html")
+  }
+
+  // Reinflating the output a second time proves the sanitized form is a fixed point that
+  // cannot mutate back into an executable one. Element shape is browser-dependent, so only
+  // the executable vectors are asserted on.
+  test("neutralizes a mutation-XSS payload after round-trip", () => {
+    const payload = "<noscript><p title=\"</noscript><img src=x onerror=alert(1)>\">"
+    const once = reinflate(payload)
+    const twice = reinflate(once)
+    for (const output of [ once, twice ]) {
+      assert.notOk(/onerror/i.test(output), `mXSS onerror survived: ${output}`)
+      assert.notOk(/\son\w+\s*=/i.test(output), `mXSS event handler survived: ${output}`)
+      assert.notOk(/<script/i.test(output), `mXSS script survived: ${output}`)
+    }
+  })
+
+  test("preserves HTML comments serialized inside an attachment after round-trip", () => {
+    const content = "<!-- BEGIN app/views/users/_user.html.erb --><span>Chris</span><!-- END app/views/users/_user.html.erb -->"
+    const attachment = { contentType: "application/octet-stream", content, sgid: "abc123" }
+    const html = `<div><figure data-trix-attachment='${JSON.stringify(attachment)}'></figure></div>`
+    const output = reinflate(html)
+    assert.ok(output.includes("data-trix-attachment"), `attachment lost on round-trip: ${output}`)
+    assert.ok(output.includes("BEGIN app/views/users/_user.html.erb"), `attachment comment lost: ${output}`)
+  })
+})
diff --git a/src/trix/models/composition.js b/src/trix/models/composition.js
index ad61bb358..3b9bfd3e8 100644
--- a/src/trix/models/composition.js
+++ b/src/trix/models/composition.js
@@ -140,7 +140,8 @@ export default class Composition extends BasicObject {
   }
 
   replaceHTML(html) {
-    const document = HTMLParser.parse(html).getDocument().copyUsingObjectsFromDocument(this.document)
+    const parsedDocument = HTMLParser.parse(html, { purifyOptions: { SAFE_FOR_XML: true } }).getDocument()
+    const document = parsedDocument.copyUsingObjectsFromDocument(this.document)
     const locationRange = this.getLocationRange({ strict: false })
     const selectedRange = this.document.rangeFromLocationRange(locationRange)
     this.setDocument(document)
diff --git a/src/trix/models/editor.js b/src/trix/models/editor.js
index 8092b1052..59b4e5099 100644
--- a/src/trix/models/editor.js
+++ b/src/trix/models/editor.js
@@ -20,7 +20,10 @@ export default class Editor {
   }
 
   loadHTML(html = "") {
-    const document = HTMLParser.parse(html, { referenceElement: this.element }).getDocument()
+    const document = HTMLParser.parse(html, {
+      referenceElement: this.element,
+      purifyOptions: { SAFE_FOR_XML: true },
+    }).getDocument()
     return this.loadDocument(document)
   }
 
diff --git a/src/trix/models/html_sanitizer.js b/src/trix/models/html_sanitizer.js
index b7254a026..6709bd81d 100644
--- a/src/trix/models/html_sanitizer.js
+++ b/src/trix/models/html_sanitizer.js
@@ -4,6 +4,14 @@ import { nodeIsAttachmentElement, removeNode, tagName, walkTree } from "trix/cor
 import DOMPurify from "dompurify"
 import * as config from "trix/config"
 
+// Mirrors DOMPurify's own SAFE_FOR_XML attribute-value check. Trix serializes attachment
+// content into data-trix-attachment, and that content legitimately carries HTML comments
+// (basecamp/trix#1213), so the guard runs against a neutralized copy while forceKeepAttr
+// keeps the original value. Attribute values serialize quote-delimited, so angle brackets
+// inside them stay inert when the sanitized HTML is reparsed.
+const XML_UNSAFE_ATTRIBUTE_VALUE =
+  /((--!?|])>)|<\/(style|script|title|xmp|textarea|noscript|iframe|noembed|noframes)/gi
+
 DOMPurify.addHook("uponSanitizeAttribute", function (node, data) {
   if (data.attrName === "data-trix-serialized-attributes") {
     data.keepAttr = false
@@ -12,6 +20,7 @@ DOMPurify.addHook("uponSanitizeAttribute", function (node, data) {
 
   const allowedAttributePattern = /^data-trix-/
   if (allowedAttributePattern.test(data.attrName)) {
+    data.attrValue = data.attrValue.replace(XML_UNSAFE_ATTRIBUTE_VALUE, "")
     data.forceKeepAttr = true
   }
 })