Apply mXSS-safe SAFE_FOR_XML on untrusted re-inflation paths

action_text-trix/app/assets/javascripts/trix.js

@@ -4248,13 +4248,30 @@ $\
   }
   var purify = createDOMPurify();
 
+  // DOMPurify's SAFE_FOR_XML guard removes any attribute whose value contains an
+  // XML-unsafe sequence (a comment terminator like `-->`/`--!>`, `]>`, or a raw
+  // `</style`-style tag close). Trix serializes attachment content — including any
+  // Rails view-annotation comments such as `<!-- BEGIN app/views/... -->` — inside
+  // the `data-trix-attachment` data attribute (see basecamp/trix#1213). Under
+  // SAFE_FOR_XML that attribute value trips this guard, so the whole attribute is
+  // dropped and the attachment silently disappears on the storage round-trip.
+  //
+  // These are data attributes: their values are always entity-escaped on
+  // serialization and never re-parsed as markup, so keeping them is mXSS-safe.
+  // This regexp mirrors DOMPurify's own SAFE_FOR_XML attribute-value check.
+  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;
       return;
     }
     const allowedAttributePattern = /^data-trix-/;
     if (allowedAttributePattern.test(data.attrName)) {
+      // Preserve serialized Trix data attributes (e.g. attachment content with
+      // comments) even under SAFE_FOR_XML. We neutralize only the copy DOMPurify
+      // inspects for its XML-safety guard; forceKeepAttr then keeps the *original*
+      // value verbatim, so the neutralized copy is never written to the DOM.
+      data.attrValue = data.attrValue.replace(XML_UNSAFE_ATTRIBUTE_VALUE, "");
       data.forceKeepAttr = true;
     }
   });
@@ -10213,7 +10230,14 @@ $\
       return this.notifyDelegateOfInsertionAtRange([startPosition, endPosition]);
     }
     replaceHTML(html) {
-      const document = HTMLParser.parse(html).getDocument().copyUsingObjectsFromDocument(this.document);
+      // Reparsing the live editor DOM is an untrusted re-inflation path, so run
+      // DOMPurify's mXSS-safe mode. Serialized `data-trix-*` attachment data
+      // (including comments) is preserved by the sanitizer hook (basecamp/trix#1213).
+      const document = HTMLParser.parse(html, {
+        purifyOptions: {
+          SAFE_FOR_XML: true
+        }
+      }).getDocument().copyUsingObjectsFromDocument(this.document);
       const locationRange = this.getLocationRange({
         strict: false
       });
@@ -10989,8 +11013,15 @@ $\
     }
     loadHTML() {
       let html = arguments.length > 0 && arguments[0] !== undefined ? arguments[0] : "";
+      // Re-inflating stored HTML is an untrusted storage round-trip, so run
+      // DOMPurify's mXSS-safe mode here. Attachment content serialized in
+      // `data-trix-*` attributes is preserved by the sanitizer's uponSanitizeAttribute
+      // hook (see basecamp/trix#1213).
       const document = HTMLParser.parse(html, {
-        referenceElement: this.element
+        referenceElement: this.element,
+        purifyOptions: {
+          SAFE_FOR_XML: true
+        }
       }).getDocument();
       return this.loadDocument(document);
     }

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"

src/test/system/reinflation_security_test.js

@@ -0,0 +1,46 @@
+import { assert, test, testGroup } from "test/test_helper"
+
+import { delay } from "../test_helpers/timing_helpers"
+
+// getEditorElement is installed as a global test helper (see trix/core/helpers/global),
+// mirroring how the other system tests reach the live editor.
+
+// Exercises the real re-inflation entry point rather than the sanitizer in isolation:
+// editor.loadHTML re-parses stored HTML under DOMPurify's mXSS-safe mode
+// (SAFE_FOR_XML: true) and renders it into the live editor, including any attachment
+// content re-parsed by AttachmentView. These assertions target the security invariant —
+// no executable handler and no <script> reach the live DOM, and nothing executes.
+// Handler stripping is a browser-independent DOMPurify guarantee, so unlike element
+// shape the assertions are parser-agnostic.
+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)
+  })
+})

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,18 @@ testGroup("Attachment", () => {
     attrs = { previewable: false, contentType: previewableTypes[0] }
     assert.notOk(createAttachment(attrs).isPreviewable())
   })
+
+  // Regression: basecamp/trix#1213. Re-inflating stored HTML under SAFE_FOR_XML
+  // must keep an attachment whose serialized content carries HTML comments.
+  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")
+  })
 })

src/test/unit/html_sanitizer_test.js

@@ -27,6 +27,28 @@ testGroup("HTMLSanitizer", () => {
     assert.ok(sanitized.includes("data-trix-attachment"))
   })
 
+  // Regression: basecamp/trix#1213. Attachment content serialized into a
+  // data-trix-attachment attribute can contain Rails view-annotation comments
+  // like `<!-- BEGIN app/views/... -->`. DOMPurify's SAFE_FOR_XML guard would
+  // otherwise drop the whole attribute (its value contains `-->`), silently
+  // removing the attachment on the storage round-trip.
+  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", () => {
+    // The preservation hook is scoped to data-trix-* attributes only. A comment
+    // terminator smuggled into a normally-allowed attribute must still be dropped.
+    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" ],

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,39 @@ testGroup("serializeToContentType", () => {
     }
   })
 })
+
+// Exercises the untrusted storage round-trip: stored HTML re-inflated through the
+// same SAFE_FOR_XML: true path editor.loadHTML uses, then serialized back out.
+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")
+  }
+
+  // The security invariant is that no executable vector survives the round-trip:
+  // no `onerror`, no event-handler attribute, and no `<script>`. Whether a *neutralized*
+  // bare element survives is browser-parser-dependent and is NOT a security property, so we
+  // don't assert on element presence: Firefox parses this payload such that a handler-stripped
+  // `<img src="x">` remains and Trix promotes it to a benign image attachment, while Chromium
+  // collapses the payload entirely. Re-inflating the sanitized output a second time proves it
+  // is a stable fixed point that cannot mutate back into an executable form.
+  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}`)
+  })
+})

src/trix/models/composition.js

@@ -140,7 +140,10 @@ export default class Composition extends BasicObject {
   }
 
   replaceHTML(html) {
-    const document = HTMLParser.parse(html).getDocument().copyUsingObjectsFromDocument(this.document)
+    // Reparsing the live editor DOM is an untrusted re-inflation path, so run
+    // DOMPurify's mXSS-safe mode. Serialized `data-trix-*` attachment data
+    // (including comments) is preserved by the sanitizer hook (basecamp/trix#1213).
+    const document = HTMLParser.parse(html, { purifyOptions: { SAFE_FOR_XML: true } }).getDocument().copyUsingObjectsFromDocument(this.document)
     const locationRange = this.getLocationRange({ strict: false })
     const selectedRange = this.document.rangeFromLocationRange(locationRange)
     this.setDocument(document)

src/trix/models/editor.js

@@ -20,7 +20,14 @@ export default class Editor {
   }
 
   loadHTML(html = "") {
-    const document = HTMLParser.parse(html, { referenceElement: this.element }).getDocument()
+    // Re-inflating stored HTML is an untrusted storage round-trip, so run
+    // DOMPurify's mXSS-safe mode here. Attachment content serialized in
+    // `data-trix-*` attributes is preserved by the sanitizer's uponSanitizeAttribute
+    // hook (see basecamp/trix#1213).
+    const document = HTMLParser.parse(html, {
+      referenceElement: this.element,
+      purifyOptions: { SAFE_FOR_XML: true },
+    }).getDocument()
     return this.loadDocument(document)
   }
 

src/trix/models/html_sanitizer.js

@@ -4,6 +4,20 @@ import { nodeIsAttachmentElement, removeNode, tagName, walkTree } from "trix/cor
 import DOMPurify from "dompurify"
 import * as config from "trix/config"
 
+// DOMPurify's SAFE_FOR_XML guard removes any attribute whose value contains an
+// XML-unsafe sequence (a comment terminator like `-->`/`--!>`, `]>`, or a raw
+// `</style`-style tag close). Trix serializes attachment content — including any
+// Rails view-annotation comments such as `<!-- BEGIN app/views/... -->` — inside
+// the `data-trix-attachment` data attribute (see basecamp/trix#1213). Under
+// SAFE_FOR_XML that attribute value trips this guard, so the whole attribute is
+// dropped and the attachment silently disappears on the storage round-trip.
+//
+// These are data attributes: their values are always entity-escaped on
+// serialization and never re-parsed as markup, so keeping them is mXSS-safe.
+// This regexp mirrors DOMPurify's own SAFE_FOR_XML attribute-value check.
+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 +26,11 @@ DOMPurify.addHook("uponSanitizeAttribute", function (node, data) {
 
   const allowedAttributePattern = /^data-trix-/
   if (allowedAttributePattern.test(data.attrName)) {
+    // Preserve serialized Trix data attributes (e.g. attachment content with
+    // comments) even under SAFE_FOR_XML. We neutralize only the copy DOMPurify
+    // inspects for its XML-safety guard; forceKeepAttr then keeps the *original*
+    // value verbatim, so the neutralized copy is never written to the DOM.
+    data.attrValue = data.attrValue.replace(XML_UNSAFE_ATTRIBUTE_VALUE, "")
     data.forceKeepAttr = true
   }
 })