From 868196c4d2f406452e76fd829064505dcc92d9fb Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 10 Aug 2026 22:23:30 -0700 Subject: [PATCH 1/5] Apply mXSS-safe SAFE_FOR_XML on attachment content re-inflation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-inflating stored attachment content is an untrusted storage round-trip: the custom attachment node re-parses the serialized `content` HTML straight into the live editor DOM. That call now opts into DOMPurify's mXSS-safe mode. SAFE_FOR_XML drops any attribute whose value could close a comment or a raw-text element, and a serialized `content` attribute is exactly that shape — it carries arbitrary HTML, including Rails view-annotation comments. So a hook force-keeps it, inspecting a neutralized copy while the original value is what reaches the DOM. Rewritten against per-editor configs, which is why this is not the version that was written before #1227. The earlier design kept a module-level set of which tags allow `content`, populated as a side effect of buildConfig(). buildConfig now runs once per editor, so that set would hold whichever editor connected last while the hook fired under a different editor's config — and the two would disagree in both directions. Editor B with attachments off connecting last would gate to false and strip editor A's content under SAFE_FOR_XML, destroying the attachment on the round trip: the exact regression this change exists to prevent, reintroduced by connection order. Reversed, the hook would set forceKeepAttr, which continues past _isValidAttribute entirely, keeping `content` on an element whose in-force config denied it. Neither shows up in a single-editor test, and both survive a clean merge — the hook body and the declaration merge without conflict; only the assignment collides. The hook is stateless instead. DOMPurify passes the resolved config to every hook as a third argument, and with nothing calling setConfig any more, _parseConfig runs on each sanitize() — so that config is the calling editor's, not a stale one. buildConfig already exposes the per-tag predicate as ADD_ATTR, which is precisely the question the hook needs answered, so the gate reads it directly. sanitize() derives the safe-XML config by spreading this editor's config rather than a module base, which keeps TRUSTED_TYPES_POLICY, ADD_URI_SAFE_ATTR and the per-editor allowlist that keying on the editor was for. Two new tests cover the pair, each registering the other editor last so a stale gate is what fails them. Verified by mutation: dropping the ADD_ATTR gate fails one, deriving the config from the fallback fails two, and ignoring safeForXml fails one. 152 unit tests pass. Also notes the registration order against #1225's stimulus hook. forceKeepAttr beats keepAttr, so widening this hook past `content` would silently defeat it. The attribute sets are disjoint today and no test links them, so it is written down at the registration site rather than left to be rediscovered. --- src/config/dom_purify.js | 76 +++++++++++++++++++ src/helpers/sanitization_helper.js | 19 ++++- .../custom_action_text_attachment_node.js | 6 +- .../content_reinflation_sanitization.test.js | 52 +++++++++++++ .../unit/helpers/sanitization_helper.test.js | 53 +++++++++++++ 5 files changed, 203 insertions(+), 3 deletions(-) create mode 100644 test/javascript/unit/editor/attachments/content_reinflation_sanitization.test.js diff --git a/src/config/dom_purify.js b/src/config/dom_purify.js index 30d264e6a..bd9199b7d 100644 --- a/src/config/dom_purify.js +++ b/src/config/dom_purify.js @@ -156,8 +156,84 @@ function styleFilterHook(_currentNode, hookEvent) { } } +// DOMPurify's SAFE_FOR_XML guard drops any attribute whose value contains an +// XML-unsafe sequence — a comment terminator (`-->`, `--!>`, `]>`) or a raw +// ``) +// into the `content` attribute of an . Under SAFE_FOR_XML +// that value trips the guard and the whole attribute is stripped, silently losing +// the attachment on the storage round-trip. +// +// The content attribute is inert: it is always entity-escaped on serialization and +// is only ever re-parsed — and re-sanitized — by the attachment node's own renderer, +// so preserving it here is mXSS-safe. This regexp mirrors DOMPurify's own +// SAFE_FOR_XML attribute-value check, so it is coupled to the dompurify version in +// package.json — verified identical against 3.4.13's check apart from the /g needed +// for replace(). +const XML_UNSAFE_ATTRIBUTE_VALUE = + /((--!?|])>)|<\/(style|script|title|xmp|textarea|noscript|iframe|noembed|noframes)/gi + +// Narrowed to the attachment element itself, via the same isAttachmentTag the url +// hook uses — both are asking the one question that matters, "is this our element", +// and the answer has to stay the same for both. The justification for the bypass +// below is specific to attachment content: it is entity-escaped on serialization and +// only ever re-parsed by the attachment node's own renderer, which re-sanitizes it. +// Extensions may declare `content` on any tag they like (see home/docs/extensions.md), +// and for `{ tag: "x-widget", attributes: ["content"] }` none of that reasoning holds +// — whatever renders x-widget is free to treat the value as markup. +// +// No fallback to the literal `action-text-attachment`: when a consumer renames the +// element (bc3 uses `bc-attachment`), that name is no longer imported or rendered by +// the attachment node, so exempting it would grant the bypass to a tag nothing here +// treats as an attachment. The configured name is the only one that means anything. +// +// Stateless: the per-editor config arrives as the hook's third argument. +// +// This is the whole reason the hook cannot keep its own copy of which tags allow +// `content`. buildConfig() now runs once per editor, so a module-level set would be +// whichever editor connected last, and it would desynchronise from the config this +// call is actually running under — in both directions. Editor B (attachments off) +// connecting last would strip editor A's content; the reverse would force-keep +// `content` on an element whose in-force config denied it. +// +// DOMPurify passes the resolved config to every hook (`hook.call(DOMPurify, node, +// data, CONFIG)`), and because nothing calls setConfig any more, _parseConfig runs on +// each sanitize() — so CONFIG is this call's config, not a stale one. buildConfig +// already exposes the per-tag predicate as ADD_ATTR, which is exactly the question +// being asked here. +function preserveSerializedContentHook(currentNode, hookEvent, config) { + const tag = currentNode?.nodeName?.toLowerCase() + + // forceKeepAttr bypasses DOMPurify's per-tag attribute allowlist, so it is scoped + // to a tag this config actually granted `content` on. Anything else — a + // non-attachment tag, or an attachment tag configured without `content` because + // attachments are disabled — falls through to normal validation and is stripped. + // The bypass never leaks past what the calling editor's config already permits. + if (hookEvent.attrName === "content" && isAttachmentTag(tag) && config?.ADD_ATTR?.("content", tag)) { + // Neutralize only the copy DOMPurify inspects for its XML-safety guard. The + // original value is what reaches the DOM: forceKeepAttr makes _sanitizeAttributes + // `continue`, which skips the _setAttributeValue at the end of the loop, so the + // attribute is left exactly as parsed. + // + // Replaced with a space rather than an empty string, because deleting a match can + // join its neighbours into a *new* unsafe sequence that String#replace will not + // rescan. `foo--bar` collapses to `foo-->bar`, which still trips the guard + // — and the guard runs before the forceKeepAttr check, so the attribute would be + // dropped despite this hook. A separator makes that impossible for every + // alternation in the pattern. + hookEvent.attrValue = hookEvent.attrValue.replace(XML_UNSAFE_ATTRIBUTE_VALUE, " ") + hookEvent.forceKeepAttr = true + } +} + DOMPurify.addHook("uponSanitizeAttribute", styleFilterHook) DOMPurify.addHook("uponSanitizeAttribute", attachmentUriFilterHook) +// Registered ahead of stimulusAttributeFilterHook, which sets keepAttr = false. +// forceKeepAttr wins over keepAttr in DOMPurify, so if this hook's attribute set +// ever widened past `content` to overlap that one's (`data-controller`, +// `data-action`) it would silently defeat it. They are disjoint today and no test +// links them, so the ordering is noted here rather than left to be rediscovered. +DOMPurify.addHook("uponSanitizeAttribute", preserveSerializedContentHook) const FORBIDDEN_STIMULUS_ATTRIBUTES = [ "data-controller", "data-action" ] diff --git a/src/helpers/sanitization_helper.js b/src/helpers/sanitization_helper.js index 55ee7ffec..83e81422a 100644 --- a/src/helpers/sanitization_helper.js +++ b/src/helpers/sanitization_helper.js @@ -32,6 +32,21 @@ 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. +// +// The serialized `content` attribute survives SAFE_FOR_XML via +// preserveSerializedContentHook in config/dom_purify, which reads the same config +// off the hook's third argument. +export function sanitize(html, editor, { safeForXml = false } = {}) { + const config = configs.get(editor) ?? fallbackConfig + + return DOMPurify.sanitize(html, safeForXml ? { ...config, SAFE_FOR_XML: true } : config) } diff --git a/src/nodes/custom_action_text_attachment_node.js b/src/nodes/custom_action_text_attachment_node.js index f9a969903..b1f25a8bb 100644 --- a/src/nodes/custom_action_text_attachment_node.js +++ b/src/nodes/custom_action_text_attachment_node.js @@ -78,7 +78,11 @@ 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. The serialized `content` + // attribute survives that via preserveSerializedContentHook. + figure.insertAdjacentHTML("beforeend", sanitize(this.innerHtml, editor, { safeForXml: true })) const deleteButton = createElement("lexxy-node-delete-button") figure.appendChild(deleteButton) diff --git a/test/javascript/unit/editor/attachments/content_reinflation_sanitization.test.js b/test/javascript/unit/editor/attachments/content_reinflation_sanitization.test.js new file mode 100644 index 000000000..530e7c335 --- /dev/null +++ b/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) => + `` + + // 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 = "<p title="--><img src=x onerror=alert(1)>">hi</p>" + 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("-->") + 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 = "<!-- BEGIN app/views/users/_user.html.erb --><span>Chris</span><!-- END app/views/users/_user.html.erb -->" + 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") + }) +}) diff --git a/test/javascript/unit/helpers/sanitization_helper.test.js b/test/javascript/unit/helpers/sanitization_helper.test.js index 1f51bdc5f..bc0c5c4bd 100644 --- a/test/javascript/unit/helpers/sanitization_helper.test.js +++ b/test/javascript/unit/helpers/sanitization_helper.test.js @@ -98,6 +98,59 @@ test("an editor denying attachment content strips it when another editor allows expect(sanitized).toContain("action-text-attachment") }) +// The mXSS-safe re-inflation path, across two editors. +// +// SAFE_FOR_XML drops any attribute whose value could close a comment or a raw-text +// element, which is exactly what a serialized `content` attribute looks like — so a +// hook force-keeps it. That bypass has to be scoped to the config of the editor +// making the call, and this is the pair of tests that holds it there. +// +// Both register the *other* editor last on purpose. A hook keeping its own copy of +// which tags allow `content` would hold whichever config was built most recently, and +// each test below catches one direction of that desync: the first loses an +// attachment that should have survived, the second force-keeps `content` on an +// element whose config denied it. Neither shows up without two editors in play, +// which is why single-editor coverage was not enough. +const withContent = [ { tag: "action-text-attachment", attributes: [ "content", "content-type", "sgid" ] } ] +const serialized = '' + +test("safe-XML re-inflation keeps content for the editor whose config allows it", () => { + const allows = { name: "attachments on" } + const denies = { name: "attachments off" } + + setSanitizerConfig(allows, withContent) + setSanitizerConfig(denies, [ "p" ]) // registered last + + // Under module-level state this is the silent data-loss direction: the attachment + // loses its content and is destroyed on the round trip. + expect(sanitize(serialized, allows, { safeForXml: true })).toContain("content=") +}) + +test("safe-XML re-inflation strips content for the editor whose config denies it", () => { + const denies = { name: "attachments off" } + const allows = { name: "attachments on" } + + setSanitizerConfig(denies, [ { tag: "action-text-attachment", attributes: [ "content-type", "sgid" ] } ]) + setSanitizerConfig(allows, withContent) // registered last + + // The allowlist-bypass direction: forceKeepAttr skips _isValidAttribute entirely, + // so a stale gate would keep `content` on an element the in-force config refused. + const sanitized = sanitize(serialized, denies, { safeForXml: true }) + + expect(sanitized).not.toContain("content=\"") + expect(sanitized).toContain("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("a", 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. From 41a429d0f78488fdcdd9442efd321877bdfce0dc Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 10 Aug 2026 22:32:46 -0700 Subject: [PATCH 2/5] Cover the attachment content round trip with a system test AGENTS.md asks for a Capybara editor -> save -> render -> re-edit test whenever sanitization rules change, and this changes them. The unit tests only see the first hop; Loofah gets a say on the server, and a stage that drops the content attribute there would look identical to the bug this PR fixes. The dummy partial now emits an HTML comment, which is what makes the test mean anything. A comment is precisely the value SAFE_FOR_XML rejects, and it is not contrived: Rails emits view annotations of this shape when annotate_rendered_view_with_filenames is on, which is how comments end up inside real attachment content. Verified it reaches the serialized attribute rather than being swallowed as an ERB comment. The test asserts the comment survives in the submitted value before saving and again after re-editing, with the rendered page checked in between. --- test/dummy/app/views/people/_person.html.erb | 8 +++ .../attachment_content_round_trip_test.rb | 60 +++++++++++++++++++ 2 files changed, 68 insertions(+) create mode 100644 test/system/attachment_content_round_trip_test.rb diff --git a/test/dummy/app/views/people/_person.html.erb b/test/dummy/app/views/people/_person.html.erb index d075edac0..4d0c6cc8c 100644 --- a/test/dummy/app/views/people/_person.html.erb +++ b/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. %> + <%= person.name %><%= person.name %> (<%= person.initials %>) diff --git a/test/system/attachment_content_round_trip_test.rb b/test/system/attachment_content_round_trip_test.rb new file mode 100644 index 000000000..7930abdbf --- /dev/null +++ b/test/system/attachment_content_round_trip_test.rb @@ -0,0 +1,60 @@ +require "application_system_test_case" + +# Attachment content is re-sanitized in mXSS-safe mode every time the attachment +# renders in the editor, and DOMPurify's SAFE_FOR_XML guard rejects any attribute +# value that could close a comment. A serialized `content` attribute routinely +# contains one: Rails view annotations land inside the partial an attachment +# renders, so the guard would drop the whole attribute and the attachment with it. +# +# A hook keeps the attribute, which makes this a round-trip property rather than a +# client-side one. The editor, the saved value, the rendered page and the re-edited +# document all have to agree, and Loofah on the server gets a say between them. +# Unit tests only see the first hop; this is the test that would catch a server +# stage dropping the attribute. +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 is the hop this PR changes: content is re-inflated under + # SAFE_FOR_XML. Without the preservation hook the attachment loses its + # content here and the mention disappears. + assert_mention_in_editor person + assert_comment_in_saved_value + end + + private + def assert_mention_in_editor(person) + within find_editor.selector do + assert_selector %(bc-mention[gid="#{person.to_gid}"]), 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 From 144f0e6384c5a0279c915c29cc4b5303d0e0d732 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 10 Aug 2026 22:38:42 -0700 Subject: [PATCH 3/5] Neutralize with a separator, so removals cannot merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting a match can join its neighbours into a new unsafe sequence, and String#replace does not rescan what it just produced. `foo--bar` collapses to `foo-->bar`, which still trips SAFE_FOR_XML. That check runs before the forceKeepAttr check, so the attribute was dropped by the hook meant to keep it. Replacing with a space makes it impossible for every alternation in the pattern. The substitution is only ever applied to the copy DOMPurify inspects, so this does not change what reaches the DOM. The regression test runs the real sanitizer end to end over every alternation — a comment terminator, a bracket terminator, and raw-text closing tags — so it also detects the version coupling. XML_UNSAFE_ATTRIBUTE_VALUE mirrors a check private to DOMPurify, and package.json allows any ^3.4.13. If a later release widens that check, the attribute gets dropped and this test fails on the version that did it. That seemed better than pinning an exact version, which would also refuse security patches for the dependency this file exists to configure. Verified the test fails with the old empty-string neutralizer. --- .../unit/helpers/sanitization_helper.test.js | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/test/javascript/unit/helpers/sanitization_helper.test.js b/test/javascript/unit/helpers/sanitization_helper.test.js index bc0c5c4bd..21ffee5de 100644 --- a/test/javascript/unit/helpers/sanitization_helper.test.js +++ b/test/javascript/unit/helpers/sanitization_helper.test.js @@ -141,6 +141,38 @@ test("safe-XML re-inflation strips content for the editor whose config denies it expect(sanitized).toContain("action-text-attachment") }) +// This also stands in for the version coupling. XML_UNSAFE_ATTRIBUTE_VALUE mirrors +// a check private to DOMPurify, and package.json allows any ^3.4.13, so a later +// release could widen that check and leave our neutralizer behind. Rather than +// pinning an exact version — which would also refuse security patches for the +// dependency this whole file is about — these cases run the real sanitizer end to +// end. If DOMPurify starts rejecting something we do not neutralize, the attribute +// is dropped and this test fails on the version that introduced it. +// +// The values below cover every alternation in the pattern: a comment terminator, a +// bracket terminator, and a raw-text closing tag. +// +// They also cover the merge case. Neutralizing by deletion can join a match's +// neighbours into a *new* unsafe sequence, and String#replace does not rescan what +// it just produced. Deleting `bar` leaves `foo-->bar`, +// which still trips the guard — and the guard runs before the forceKeepAttr check, +// so the attribute would be dropped by the very hook meant to keep it. +test("keeps content whose unsafe sequences would merge when removed", () => { + const merging = { name: "attachments on" } + setSanitizerConfig(merging, withContent) + + for (const value of [ + "foo--bar", "-->", "a]>b", + "plain --> comment", "bracket ]> close", "ok", + "", "", "--!>" + ]) { + const html = `/g, ">")}">` + + expect(sanitize(html, merging, { safeForXml: true }), `lost content for ${value}`) + .toContain("content=") + } +}) + test("safe-XML mode keeps the per-editor allowlist and the rest of the config", () => { const allows = { name: "attachments on" } setSanitizerConfig(allows, withContent) From 4019e0e6986bbad9a932e3f24c40c0b706582c3f Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 10 Aug 2026 23:33:28 -0700 Subject: [PATCH 4/5] Assert what re-inflation actually leaves in the editor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bc-mention 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, so DOMPurify drops the tag and keeps its children. The assertion could not hold on any run. Assert on the attachment element carrying the rendered name instead, which is the property the round trip is about: lose the content on any hop and the attachment comes back empty. Also correct the header. Reverting this branch's src/ leaves the test green, so it does not cover preserveSerializedContentHook and should not claim to — no hop here sanitizes a `content` attribute under SAFE_FOR_XML. --- .../attachment_content_round_trip_test.rb | 42 +++++++++++++------ 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/test/system/attachment_content_round_trip_test.rb b/test/system/attachment_content_round_trip_test.rb index 7930abdbf..156749a00 100644 --- a/test/system/attachment_content_round_trip_test.rb +++ b/test/system/attachment_content_round_trip_test.rb @@ -1,16 +1,21 @@ require "application_system_test_case" # Attachment content is re-sanitized in mXSS-safe mode every time the attachment -# renders in the editor, and DOMPurify's SAFE_FOR_XML guard rejects any attribute -# value that could close a comment. A serialized `content` attribute routinely -# contains one: Rails view annotations land inside the partial an attachment -# renders, so the guard would drop the whole attribute and the attachment with it. +# renders in the editor, so a comment-bearing `content` attribute — 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 the attribute. # -# A hook keeps the attribute, which makes this a round-trip property rather than a -# client-side one. The editor, the saved value, the rendered page and the re-edited -# document all have to agree, and Loofah on the server gets a say between them. -# Unit tests only see the first hop; this is the test that would catch a server -# stage dropping the attribute. +# What this does NOT cover, despite what it looks like: preserveSerializedContentHook. +# That hook fires on a `content` attribute sanitized under SAFE_FOR_XML, and no hop +# here is one. The saved value is read with SAFE_FOR_XML false, and the safe-XML call +# in CustomActionTextAttachmentNode#createDOM is handed the *inner* markup, which +# carries no `content` attribute of its own — only a nested attachment would. Verified +# by reverting this PR's src/ entirely: the test still passes. Sanitizer coverage for +# the hook lives in test/javascript/unit/helpers/sanitization_helper.test.js, which +# calls sanitize() with a shape createDOM does not currently produce. class AttachmentContentRoundTripTest < ApplicationSystemTestCase COMMENT = "BEGIN app/views/people/_person.html.erb" @@ -33,17 +38,28 @@ class AttachmentContentRoundTripTest < ApplicationSystemTestCase click_on "Edit this post" wait_for_editor - # The re-edit is the hop this PR changes: content is re-inflated under - # SAFE_FOR_XML. Without the preservation hook the attachment loses its - # content here and the mention disappears. + # 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 %(bc-mention[gid="#{person.to_gid}"]), text: person.name, visible: :all + assert_selector %(action-text-attachment[content-type="application/vnd.actiontext.mention"]), + text: person.name, visible: :all end end From 214e8dac129de358a5210bc670fa7caa9672675b Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 11 Aug 2026 22:56:57 -0700 Subject: [PATCH 5/5] =?UTF-8?q?Drop=20the=20content=20preservation=20hook?= =?UTF-8?q?=20=E2=80=94=20it=20guards=20a=20shape=20production=20never=20p?= =?UTF-8?q?roduces?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review converged here from four directions (Codex once, Copilot twice, and a mutation test of my own): preserveSerializedContentHook is not on any path the code takes. It fires only when a `content` attribute is sanitized under SAFE_FOR_XML, and nothing does that. The serialized `content` attribute is produced only by exportDOM, where SAFE_FOR_XML is off; the one safeForXml call site, CustomActionTextAttachmentNode#createDOM, is handed the decoded *inner* markup, which has no `content` attribute of its own. Only a nested attachment — an action-text-attachment inside another's content — would reach the hook, and nothing here produces or supports one. Confirmed empirically twice: reverting this PR's whole src/ leaves the system test green, and disabling just this hook leaves the real-path re-inflation test (content_reinflation_sanitization) green — only the synthetic tests that feed sanitize() a raw content attribute directly fail. Dropping it is not merely neutral, it is the safer default. If a nested attachment ever were stored, the hook would forceKeepAttr an XML-unsafe value on an element that is not re-sanitized by its own node; without it, the mXSS guard does its job. So there is no case in which keeping it is the safer choice. Removed: the hook, its registration, XML_UNSAFE_ATTRIBUTE_VALUE, and the synthetic outer-element tests. That also deletes the mirror of a DOMPurify private predicate, which closes the separate thread about pinning dompurify exactly — there is no longer a private check to stay aligned with. Kept, because it stands on its own: the { safeForXml: true } opt-in in createDOM. Re-inflating stored content is an untrusted round-trip and running it in DOMPurify's mXSS-safe mode is a genuine hardening, covered on the real editor path by content_reinflation_sanitization.test.js (drops a comment-breakout payload; preserves legitimate comment-bearing content), mutation-checked against reverting the opt-in. isAttachmentTag stays too — the url hook uses it. --- src/config/dom_purify.js | 76 ------------------ src/helpers/sanitization_helper.js | 4 - .../custom_action_text_attachment_node.js | 6 +- .../unit/helpers/sanitization_helper.test.js | 79 ++----------------- .../attachment_content_round_trip_test.rb | 25 +++--- 5 files changed, 21 insertions(+), 169 deletions(-) diff --git a/src/config/dom_purify.js b/src/config/dom_purify.js index bd9199b7d..30d264e6a 100644 --- a/src/config/dom_purify.js +++ b/src/config/dom_purify.js @@ -156,84 +156,8 @@ function styleFilterHook(_currentNode, hookEvent) { } } -// DOMPurify's SAFE_FOR_XML guard drops any attribute whose value contains an -// XML-unsafe sequence — a comment terminator (`-->`, `--!>`, `]>`) or a raw -// ``) -// into the `content` attribute of an . Under SAFE_FOR_XML -// that value trips the guard and the whole attribute is stripped, silently losing -// the attachment on the storage round-trip. -// -// The content attribute is inert: it is always entity-escaped on serialization and -// is only ever re-parsed — and re-sanitized — by the attachment node's own renderer, -// so preserving it here is mXSS-safe. This regexp mirrors DOMPurify's own -// SAFE_FOR_XML attribute-value check, so it is coupled to the dompurify version in -// package.json — verified identical against 3.4.13's check apart from the /g needed -// for replace(). -const XML_UNSAFE_ATTRIBUTE_VALUE = - /((--!?|])>)|<\/(style|script|title|xmp|textarea|noscript|iframe|noembed|noframes)/gi - -// Narrowed to the attachment element itself, via the same isAttachmentTag the url -// hook uses — both are asking the one question that matters, "is this our element", -// and the answer has to stay the same for both. The justification for the bypass -// below is specific to attachment content: it is entity-escaped on serialization and -// only ever re-parsed by the attachment node's own renderer, which re-sanitizes it. -// Extensions may declare `content` on any tag they like (see home/docs/extensions.md), -// and for `{ tag: "x-widget", attributes: ["content"] }` none of that reasoning holds -// — whatever renders x-widget is free to treat the value as markup. -// -// No fallback to the literal `action-text-attachment`: when a consumer renames the -// element (bc3 uses `bc-attachment`), that name is no longer imported or rendered by -// the attachment node, so exempting it would grant the bypass to a tag nothing here -// treats as an attachment. The configured name is the only one that means anything. -// -// Stateless: the per-editor config arrives as the hook's third argument. -// -// This is the whole reason the hook cannot keep its own copy of which tags allow -// `content`. buildConfig() now runs once per editor, so a module-level set would be -// whichever editor connected last, and it would desynchronise from the config this -// call is actually running under — in both directions. Editor B (attachments off) -// connecting last would strip editor A's content; the reverse would force-keep -// `content` on an element whose in-force config denied it. -// -// DOMPurify passes the resolved config to every hook (`hook.call(DOMPurify, node, -// data, CONFIG)`), and because nothing calls setConfig any more, _parseConfig runs on -// each sanitize() — so CONFIG is this call's config, not a stale one. buildConfig -// already exposes the per-tag predicate as ADD_ATTR, which is exactly the question -// being asked here. -function preserveSerializedContentHook(currentNode, hookEvent, config) { - const tag = currentNode?.nodeName?.toLowerCase() - - // forceKeepAttr bypasses DOMPurify's per-tag attribute allowlist, so it is scoped - // to a tag this config actually granted `content` on. Anything else — a - // non-attachment tag, or an attachment tag configured without `content` because - // attachments are disabled — falls through to normal validation and is stripped. - // The bypass never leaks past what the calling editor's config already permits. - if (hookEvent.attrName === "content" && isAttachmentTag(tag) && config?.ADD_ATTR?.("content", tag)) { - // Neutralize only the copy DOMPurify inspects for its XML-safety guard. The - // original value is what reaches the DOM: forceKeepAttr makes _sanitizeAttributes - // `continue`, which skips the _setAttributeValue at the end of the loop, so the - // attribute is left exactly as parsed. - // - // Replaced with a space rather than an empty string, because deleting a match can - // join its neighbours into a *new* unsafe sequence that String#replace will not - // rescan. `foo--bar` collapses to `foo-->bar`, which still trips the guard - // — and the guard runs before the forceKeepAttr check, so the attribute would be - // dropped despite this hook. A separator makes that impossible for every - // alternation in the pattern. - hookEvent.attrValue = hookEvent.attrValue.replace(XML_UNSAFE_ATTRIBUTE_VALUE, " ") - hookEvent.forceKeepAttr = true - } -} - DOMPurify.addHook("uponSanitizeAttribute", styleFilterHook) DOMPurify.addHook("uponSanitizeAttribute", attachmentUriFilterHook) -// Registered ahead of stimulusAttributeFilterHook, which sets keepAttr = false. -// forceKeepAttr wins over keepAttr in DOMPurify, so if this hook's attribute set -// ever widened past `content` to overlap that one's (`data-controller`, -// `data-action`) it would silently defeat it. They are disjoint today and no test -// links them, so the ordering is noted here rather than left to be rediscovered. -DOMPurify.addHook("uponSanitizeAttribute", preserveSerializedContentHook) const FORBIDDEN_STIMULUS_ATTRIBUTES = [ "data-controller", "data-action" ] diff --git a/src/helpers/sanitization_helper.js b/src/helpers/sanitization_helper.js index 83e81422a..a0d96fe8b 100644 --- a/src/helpers/sanitization_helper.js +++ b/src/helpers/sanitization_helper.js @@ -41,10 +41,6 @@ export function setSanitizerConfig(editor, allowedTags) { // 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. -// -// The serialized `content` attribute survives SAFE_FOR_XML via -// preserveSerializedContentHook in config/dom_purify, which reads the same config -// off the hook's third argument. export function sanitize(html, editor, { safeForXml = false } = {}) { const config = configs.get(editor) ?? fallbackConfig diff --git a/src/nodes/custom_action_text_attachment_node.js b/src/nodes/custom_action_text_attachment_node.js index b1f25a8bb..1a77b84b6 100644 --- a/src/nodes/custom_action_text_attachment_node.js +++ b/src/nodes/custom_action_text_attachment_node.js @@ -80,8 +80,10 @@ export class CustomActionTextAttachmentNode extends DecoratorNode { // allowlist rather than whichever editor connected most recently. // // this.innerHtml is untrusted stored content being re-inflated into the editor, - // so it goes through DOMPurify's mXSS-safe mode. The serialized `content` - // attribute survives that via preserveSerializedContentHook. + // 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") diff --git a/test/javascript/unit/helpers/sanitization_helper.test.js b/test/javascript/unit/helpers/sanitization_helper.test.js index 21ffee5de..e6aac4e13 100644 --- a/test/javascript/unit/helpers/sanitization_helper.test.js +++ b/test/javascript/unit/helpers/sanitization_helper.test.js @@ -98,80 +98,13 @@ test("an editor denying attachment content strips it when another editor allows expect(sanitized).toContain("action-text-attachment") }) -// The mXSS-safe re-inflation path, across two editors. -// -// SAFE_FOR_XML drops any attribute whose value could close a comment or a raw-text -// element, which is exactly what a serialized `content` attribute looks like — so a -// hook force-keeps it. That bypass has to be scoped to the config of the editor -// making the call, and this is the pair of tests that holds it there. -// -// Both register the *other* editor last on purpose. A hook keeping its own copy of -// which tags allow `content` would hold whichever config was built most recently, and -// each test below catches one direction of that desync: the first loses an -// attachment that should have survived, the second force-keeps `content` on an -// element whose config denied it. Neither shows up without two editors in play, -// which is why single-editor coverage was not enough. +// 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 = '' - -test("safe-XML re-inflation keeps content for the editor whose config allows it", () => { - const allows = { name: "attachments on" } - const denies = { name: "attachments off" } - - setSanitizerConfig(allows, withContent) - setSanitizerConfig(denies, [ "p" ]) // registered last - - // Under module-level state this is the silent data-loss direction: the attachment - // loses its content and is destroyed on the round trip. - expect(sanitize(serialized, allows, { safeForXml: true })).toContain("content=") -}) - -test("safe-XML re-inflation strips content for the editor whose config denies it", () => { - const denies = { name: "attachments off" } - const allows = { name: "attachments on" } - - setSanitizerConfig(denies, [ { tag: "action-text-attachment", attributes: [ "content-type", "sgid" ] } ]) - setSanitizerConfig(allows, withContent) // registered last - - // The allowlist-bypass direction: forceKeepAttr skips _isValidAttribute entirely, - // so a stale gate would keep `content` on an element the in-force config refused. - const sanitized = sanitize(serialized, denies, { safeForXml: true }) - - expect(sanitized).not.toContain("content=\"") - expect(sanitized).toContain("action-text-attachment") -}) - -// This also stands in for the version coupling. XML_UNSAFE_ATTRIBUTE_VALUE mirrors -// a check private to DOMPurify, and package.json allows any ^3.4.13, so a later -// release could widen that check and leave our neutralizer behind. Rather than -// pinning an exact version — which would also refuse security patches for the -// dependency this whole file is about — these cases run the real sanitizer end to -// end. If DOMPurify starts rejecting something we do not neutralize, the attribute -// is dropped and this test fails on the version that introduced it. -// -// The values below cover every alternation in the pattern: a comment terminator, a -// bracket terminator, and a raw-text closing tag. -// -// They also cover the merge case. Neutralizing by deletion can join a match's -// neighbours into a *new* unsafe sequence, and String#replace does not rescan what -// it just produced. Deleting `bar` leaves `foo-->bar`, -// which still trips the guard — and the guard runs before the forceKeepAttr check, -// so the attribute would be dropped by the very hook meant to keep it. -test("keeps content whose unsafe sequences would merge when removed", () => { - const merging = { name: "attachments on" } - setSanitizerConfig(merging, withContent) - - for (const value of [ - "foo--bar", "-->", "a]>b", - "plain --> comment", "bracket ]> close", "ok", - "", "", "--!>" - ]) { - const html = `/g, ">")}">` - - expect(sanitize(html, merging, { safeForXml: true }), `lost content for ${value}`) - .toContain("content=") - } -}) +const serialized = '' test("safe-XML mode keeps the per-editor allowlist and the rest of the config", () => { const allows = { name: "attachments on" } diff --git a/test/system/attachment_content_round_trip_test.rb b/test/system/attachment_content_round_trip_test.rb index 156749a00..7316f9c39 100644 --- a/test/system/attachment_content_round_trip_test.rb +++ b/test/system/attachment_content_round_trip_test.rb @@ -1,21 +1,18 @@ require "application_system_test_case" # Attachment content is re-sanitized in mXSS-safe mode every time the attachment -# renders in the editor, so a comment-bearing `content` attribute — 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 the attribute. +# 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. # -# What this does NOT cover, despite what it looks like: preserveSerializedContentHook. -# That hook fires on a `content` attribute sanitized under SAFE_FOR_XML, and no hop -# here is one. The saved value is read with SAFE_FOR_XML false, and the safe-XML call -# in CustomActionTextAttachmentNode#createDOM is handed the *inner* markup, which -# carries no `content` attribute of its own — only a nested attachment would. Verified -# by reverting this PR's src/ entirely: the test still passes. Sanitizer coverage for -# the hook lives in test/javascript/unit/helpers/sanitization_helper.test.js, which -# calls sanitize() with a shape createDOM does not currently produce. +# 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"