diff --git a/src/helpers/sanitization_helper.js b/src/helpers/sanitization_helper.js index 55ee7ffec..a0d96fe8b 100644 --- a/src/helpers/sanitization_helper.js +++ b/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) } diff --git a/src/nodes/custom_action_text_attachment_node.js b/src/nodes/custom_action_text_attachment_node.js index f9a969903..1a77b84b6 100644 --- a/src/nodes/custom_action_text_attachment_node.js +++ b/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) 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/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..e6aac4e13 100644 --- a/test/javascript/unit/helpers/sanitization_helper.test.js +++ b/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 = '' + +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. 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..7316f9c39 --- /dev/null +++ b/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