Apply mXSS-safe SAFE_FOR_XML on attachment content re-inflation

src/helpers/sanitization_helper.js

@@ -32,6 +32,17 @@ export function setSanitizerConfig(editor, allowedTags) {
   configs.set(editor, fallbackConfig)
 }
 
-export function sanitize(html, editor) {
-  return DOMPurify.sanitize(html, configs.get(editor) ?? fallbackConfig)
+// Re-inflating stored attachment content is an untrusted storage round-trip, so that
+// call site opts into DOMPurify's mXSS-safe mode with { safeForXml: true }.
+//
+// The safe-XML config is derived from *this editor's* config rather than a module
+// base, and by spreading it rather than rebuilding: that keeps TRUSTED_TYPES_POLICY
+// and ADD_URI_SAFE_ATTR, and keeps the per-editor allowlist that the whole point of
+// keying on the editor was to preserve. Building from a shared base here would
+// quietly reintroduce the last-editor-wins bug on the one path that handles
+// untrusted content.
+export function sanitize(html, editor, { safeForXml = false } = {}) {
+  const config = configs.get(editor) ?? fallbackConfig
+
+  return DOMPurify.sanitize(html, safeForXml ? { ...config, SAFE_FOR_XML: true } : config)
 }

src/nodes/custom_action_text_attachment_node.js

@@ -78,7 +78,13 @@ export class CustomActionTextAttachmentNode extends DecoratorNode {
 
     // The editor is passed through so this content is sanitized with its own
     // allowlist rather than whichever editor connected most recently.
-    figure.insertAdjacentHTML("beforeend", sanitize(this.innerHtml, editor))
+    //
+    // this.innerHtml is untrusted stored content being re-inflated into the editor,
+    // so it goes through DOMPurify's mXSS-safe mode. What is sanitized here is the
+    // decoded inner markup, which carries no serialized `content` attribute of its
+    // own — that attribute is only ever produced by exportDOM, where SAFE_FOR_XML
+    // is off — so mXSS-safe mode is free to be strict on this hop.
+    figure.insertAdjacentHTML("beforeend", sanitize(this.innerHtml, editor, { safeForXml: true }))
 
     const deleteButton = createElement("lexxy-node-delete-button")
     figure.appendChild(deleteButton)

test/dummy/app/views/people/_person.html.erb

@@ -2,4 +2,12 @@
 <%# custom attachment get sanitized on re-edit, and alt/width/height on it are %>
 <%# what attachment_content_images_test asserts survive the round trip. A data: %>
 <%# URI keeps the fixture self-contained and off the network. %>
+<%# %>
+<%# The HTML comment is load-bearing too, and is emitted rather than written as %>
+<%# an ERB comment so it reaches the serialized content attribute. It stands in %>
+<%# for the view annotations Rails emits with annotate_rendered_view_with_filenames, %>
+<%# which is how comments turn up inside real attachment content. A comment is %>
+<%# the value DOMPurify's SAFE_FOR_XML guard rejects, so this is what %>
+<%# attachment_content_round_trip_test needs in order to mean anything. %>
+<!-- BEGIN app/views/people/_person.html.erb -->
 <bc-mention gid="<%= person.to_gid %>"><img src="data:image/gif;base64,R0lGODlhAQABAAAAACw=" alt="<%= person.name %>" width="20" height="20" class="avatar"><em><%= person.name %></em> (<strong><%= person.initials %></strong>)</bc-mention>

test/javascript/unit/editor/attachments/content_reinflation_sanitization.test.js

@@ -0,0 +1,52 @@
+import { afterEach, describe, expect, test } from "vitest"
+import { createTestEditor, destroyTestEditor, tick } from "../../helpers/editor_helper"
+
+let editorElement
+
+afterEach(async () => {
+  await destroyTestEditor(editorElement)
+})
+
+// Re-inflating stored attachment content is an untrusted storage round-trip. The
+// custom attachment node re-parses the serialized `content` HTML into the live
+// editor DOM, so it must be sanitized in DOMPurify's mXSS-safe mode while keeping
+// legitimate content (including comments) intact.
+describe("attachment content re-inflation sanitization", () => {
+  const attachment = (content) =>
+    `<action-text-attachment content-type="text/html" sgid="abc123" content="${content}"></action-text-attachment>`
+
+  // The payload has to be one SAFE_FOR_XML actually decides, or the test passes
+  // with the change reverted and proves nothing. An ordinary `onerror` doesn't
+  // qualify: DOMPurify's tag and attribute allowlists remove it either way.
+  //
+  // A comment terminator inside an attribute value does. SAFE_FOR_XML drops any
+  // attribute whose value could close a comment or a raw-text element when the
+  // sanitized markup is re-serialized and parsed again — which is exactly what
+  // re-inflating stored content does.
+  test("drops an attribute whose value can break out of a comment", async () => {
+    const payload = "&lt;p title=&quot;--&gt;&lt;img src=x onerror=alert(1)&gt;&quot;&gt;hi&lt;/p&gt;"
+    editorElement = await createTestEditor({ value: attachment(payload) })
+    await tick()
+
+    const figure = editorElement.querySelector("action-text-attachment, [content-type]")
+    expect(figure, "attachment was dropped on re-inflation").not.toBeNull()
+
+    // Verified by reverting the safeForXml opt-in in createDOM: without it the
+    // title survives verbatim and this assertion fails.
+    expect(figure.innerHTML).not.toContain("--&gt;")
+    expect(figure.innerHTML).not.toMatch(/onerror/i)
+    expect(figure.textContent).toContain("hi")
+  })
+
+  test("preserves legitimate comment-bearing attachment content after round-trip", async () => {
+    const content = "&lt;!-- BEGIN app/views/users/_user.html.erb --&gt;&lt;span&gt;Chris&lt;/span&gt;&lt;!-- END app/views/users/_user.html.erb --&gt;"
+    editorElement = await createTestEditor({ value: attachment(content) })
+    await tick()
+
+    // The attachment survives, and its exported value still carries the content.
+    expect(editorElement.value).toContain("action-text-attachment")
+    expect(editorElement.value).toContain("BEGIN app/views/users/_user.html.erb")
+    // The rendered inner content is present in the editor DOM.
+    expect(editorElement.textContent).toContain("Chris")
+  })
+})

test/javascript/unit/helpers/sanitization_helper.test.js

@@ -98,6 +98,24 @@ test("an editor denying attachment content strips it when another editor allows
   expect(sanitized).toContain("action-text-attachment")
 })
 
+// The mXSS-safe re-inflation opt-in derives its config from *this editor*, not a
+// module base — spreading buildConfig's result rather than rebuilding, so the
+// per-editor allowlist and the rest of the config survive the SAFE_FOR_XML flip.
+// Building from a shared base here would reintroduce the last-editor-wins bug on
+// the one path that handles untrusted content.
+const withContent = [ { tag: "action-text-attachment", attributes: [ "content", "content-type", "sgid" ] } ]
+const serialized = '<action-text-attachment content-type="text/html" sgid="x" content="&lt;span&gt;hi&lt;/span&gt;"></action-text-attachment>'
+
+test("safe-XML mode keeps the per-editor allowlist and the rest of the config", () => {
+  const allows = { name: "attachments on" }
+  setSanitizerConfig(allows, withContent)
+
+  // Deriving the safe-XML config from a module base rather than this editor's config
+  // would drop the allowlist along with everything else buildConfig put in it.
+  expect(sanitize("<span>a</span>", allows, { safeForXml: true })).toBe("a")
+  expect(sanitize(serialized, allows, { safeForXml: true })).toContain("action-text-attachment")
+})
+
 // Every DOMPurify instance claims a policy named `dompurify` under Trusted Types,
 // and TT rejects a duplicate — so on a page with a host sanitizer, ours would get
 // none, and an unsigned instance throws at DOMParser rather than degrading.

test/system/attachment_content_round_trip_test.rb

@@ -0,0 +1,73 @@
+require "application_system_test_case"
+
+# Attachment content is re-sanitized in mXSS-safe mode every time the attachment
+# renders in the editor, so comment-bearing content — Rails view annotations land
+# inside the partial an attachment renders — has to survive the whole loop: the
+# editor, the saved value, the rendered page and the re-edited document all have to
+# agree, with Loofah on the server getting a say between them. Unit tests only ever
+# see the first hop; this is the test that would catch a server stage dropping it.
+#
+# The content survives without any special force-keep. The serialized `content`
+# attribute is only produced by exportDOM, where SAFE_FOR_XML is off, so it is never
+# subject to the mXSS guard; the safe-XML call in CustomActionTextAttachmentNode#createDOM
+# is handed the *decoded inner* markup, which has no `content` attribute of its own.
+# The client-side re-inflation guard is covered directly in
+# test/javascript/unit/editor/attachments/content_reinflation_sanitization.test.js.
+class AttachmentContentRoundTripTest < ApplicationSystemTestCase
+  COMMENT = "BEGIN app/views/people/_person.html.erb"
+
+  test "comment-bearing attachment content survives save, render and re-edit" do
+    person = people(:james)
+
+    visit edit_post_path(posts(:hello_james))
+    wait_for_editor
+
+    assert_comment_in_saved_value
+    assert_mention_in_editor person
+
+    click_on "Update Post"
+
+    # The rendered page: the attachment has been through Loofah on the way in.
+    within "article.post" do
+      assert_selector %(bc-mention[gid="#{person.to_gid}"]), text: person.name
+    end
+
+    click_on "Edit this post"
+    wait_for_editor
+
+    # The re-edit: content is re-inflated under SAFE_FOR_XML, and the attachment
+    # renders from it. Lose the content on any hop and the attachment comes back
+    # empty here.
+    assert_mention_in_editor person
+    assert_comment_in_saved_value
+  end
+
+  private
+    # Asserted on the attachment element rather than on bc-mention, which never
+    # reaches the editor DOM: an editor's allowlist is its importable tags plus
+    # whatever its extensions declare, and the dummy app declares no
+    # allowedElements for bc-mention. DOMPurify drops an unlisted tag and keeps
+    # its children, so what re-inflation leaves behind is the rendered mention —
+    # the avatar and the name — inside the attachment. That is the property this
+    # test is after: lose the content and the attachment renders empty.
+    #
+    # The gid is not dropped, only relocated: mention_round_trip_test asserts it
+    # on the rendered page and in the serialized content, which is where it lives.
+    def assert_mention_in_editor(person)
+      within find_editor.selector do
+        assert_selector %(action-text-attachment[content-type="application/vnd.actiontext.mention"]),
+          text: person.name, visible: :all
+      end
+    end
+
+    # The value the form submits: attachment content re-serialized after sanitizing.
+    def assert_comment_in_saved_value
+      attachment = Capybara.string(find_editor.value)
+        .find(%(action-text-attachment[content-type="application/vnd.actiontext.mention"]))
+
+      content = CGI.unescapeHTML(attachment["content"])
+
+      assert_includes content, COMMENT,
+        "the comment inside attachment content was dropped, which is what SAFE_FOR_XML does without the preservation hook"
+    end
+end