Apply mXSS-safe SAFE_FOR_XML on attachment content re-inflation - #1226
Apply mXSS-safe SAFE_FOR_XML on attachment content re-inflation#1226jeremy wants to merge 3 commits into
Conversation
Stored attachment content is re-inflated back into the editor by the custom attachment node's createDOM, which sanitizes the serialized `content` HTML and injects it into the live editor DOM. That sanitize ran under the base DOMPurify config, which leaves SAFE_FOR_XML off, so the untrusted storage round-trip was not defended against mutation-XSS. Opt into DOMPurify's mXSS-safe mode per call on this re-inflation path rather than flipping the base config. A global SAFE_FOR_XML: true would reintroduce the very regression the base config's comment guards against: attachment content is serialized into the `content` attribute of an <action-text-attachment>, and DOMPurify's SAFE_FOR_XML attribute-value guard drops any attribute whose value contains an XML-unsafe sequence (a comment terminator like `-->`, from Rails view annotations, or a raw tag close). That would strip `content` and silently lose the attachment. Preserve the serialized `content` attribute under SAFE_FOR_XML with an uponSanitizeAttribute hook: neutralize only the copy DOMPurify inspects for its XML-safety guard, then forceKeepAttr keeps the original value verbatim. The content attribute is inert — always entity-escaped on serialization and only ever re-parsed and re-sanitized by the attachment node's own renderer — so keeping it is mXSS-safe. XML-unsafe values on ordinary attributes are still stripped. DOMPurify ignores a per-call config while a persistent config is set, so sanitize() briefly swaps in the SAFE_FOR_XML config and restores the base config afterward. Regression tests cover both invariants: the serialized content attribute survives sanitization under SAFE_FOR_XML, and attachment content is sanitized on the re-inflation round-trip.
There was a problem hiding this comment.
Pull request overview
Adds per-call mXSS-safe sanitization when restoring stored attachment HTML.
Changes:
- Adds scoped
SAFE_FOR_XMLconfiguration swapping. - Preserves serialized attachment content attributes.
- Adds sanitization and attachment re-inflation tests.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/config/dom_purify.js |
Adds serialized-content preservation hook. |
src/helpers/sanitization_helper.js |
Supports per-call XML-safe sanitization. |
src/nodes/custom_action_text_attachment_node.js |
Enables XML-safe attachment re-inflation. |
test/javascript/unit/helpers/sanitization_helper.test.js |
Tests sanitizer configuration behavior. |
test/javascript/unit/editor/attachments/content_reinflation_sanitization.test.js |
Tests attachment content restoration. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| test("neutralizes an XSS payload smuggled through attachment content", async () => { | ||
| const payload = ""<img src=x onerror=alert(document.domain)>"" | ||
| editorElement = await createTestEditor({ value: attachment(payload) }) |
There was a problem hiding this comment.
Fair point — that payload is a plain onerror the base config already strips, so it doesn't isolate the SAFE_FOR_XML contribution. I dug into whether a non-vacuous version is achievable and what I found is worth recording:
SAFE_FOR_XML: truedoes produce a real, cross-browser differential. For<noscript><p title="</noscript><img src=x onerror=alert(1)>">, DOMPurify keeps<p title="…onerror…">underfalseand drops the attribute entirely (<p></p>) undertrue— identical in Chromium, Firefox, and WebKit. So the option is not a no-op.- But that differential is an inert attribute (the handler is trapped in a quoted
title; it doesn't execute), so a "did it execute?" assertion can't distinguish the two configs with these payloads. - Critically, the differential does not reproduce under jsdom (this suite's env). I verified both configs yield byte-identical output there, because the mutation depends on the real browser parser. So a Vitest unit test structurally cannot demonstrate the
SAFE_FOR_XMLcontribution — strengthening the jsdom payload won't help.
A genuinely non-vacuous regression test would need the browser Playwright suite and a payload whose mutation actually executes under the base config (not just leaves an inert attribute) — which the known inert-attribute cases here don't give. I've flagged that as a follow-up rather than shipping a jsdom test that looks like it proves the guarantee but can't. The Vitest tests remain as behavioral coverage (the safe-XML path runs and the serialized content attribute is preserved), and the scoping fix on this file is covered by its new negative test.
| 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) }) |
There was a problem hiding this comment.
Flagged for human review. Acknowledging the AGENTS.md convention that serialization changes get a Capybara editor→save→render→re-edit test. Two things shape whether it belongs in this PR:
- This change is client-side only — it adjusts DOMPurify (client) sanitization on the attachment re-inflation path. It does not touch the Loofah (server) or
highlightCode()(rendered view) stages. The round-trip risk this PR itself introduces is the client-sideSAFE_FOR_XMLattribute-drop, and the Vitest suite covers exactly that: it asserts the comment-bearing serializedcontentattribute survives sanitization (both base and safe-XML configs). A Loofah/rendered-view drop would be a pre-existing behavior unchanged by this diff. - A full Capybara round-trip is still a reasonable addition, but it needs the dummy-app system-test harness and would mainly exercise stages this PR doesn't change.
Flagging for a maintainer to decide whether to add the Capybara test here or as a follow-up, rather than adding a heavier system test that overlaps the unchanged server stages.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 894f866c38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| test("neutralizes an XSS payload smuggled through attachment content", async () => { | ||
| const payload = ""<img src=x onerror=alert(document.domain)>"" | ||
| editorElement = await createTestEditor({ value: attachment(payload) }) |
There was a problem hiding this comment.
This test runs under jsdom and uses a basic onerror payload that the previous sanitizer already removed, so it would pass before this commit and does not exercise the browser parser mutations that SAFE_FOR_XML is intended to prevent. Add a real mXSS payload to the Playwright suite and confirm that it fails against the parent revision before relying on it as the security regression test.
AGENTS.md reference: AGENTS.md:L63-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair point — that payload is a plain onerror the base config already strips, so it doesn't isolate the SAFE_FOR_XML contribution. I dug into whether a non-vacuous version is achievable and what I found is worth recording:
SAFE_FOR_XML: truedoes produce a real, cross-browser differential. For<noscript><p title="</noscript><img src=x onerror=alert(1)>">, DOMPurify keeps<p title="…onerror…">underfalseand drops the attribute entirely (<p></p>) undertrue— identical in Chromium, Firefox, and WebKit. So the option is not a no-op.- But that differential is an inert attribute (the handler is trapped in a quoted
title; it doesn't execute), so a "did it execute?" assertion can't distinguish the two configs with these payloads. - Critically, the differential does not reproduce under jsdom (this suite's env). I verified both configs yield byte-identical output there, because the mutation depends on the real browser parser. So a Vitest unit test structurally cannot demonstrate the
SAFE_FOR_XMLcontribution — strengthening the jsdom payload won't help.
A genuinely non-vacuous regression test would need the browser Playwright suite and a payload whose mutation actually executes under the base config (not just leaves an inert attribute) — which the known inert-attribute cases here don't give. I've flagged that as a follow-up rather than shipping a jsdom test that looks like it proves the guarantee but can't. The Vitest tests remain as behavioral coverage (the safe-XML path runs and the serialized content attribute is preserved), and the scoping fix on this file is covered by its new negative test.
| 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) }) |
There was a problem hiding this comment.
Despite its name, this test only loads a value into a client-side editor and reads the client export; it never saves a dummy-app post or passes the attachment through Loofah, rendered Action Text, and re-editing. Because this change alters sanitization and preservation of the serialized content attribute, a server-stage regression could still silently remove the attachment, so add the required Capybara save → render → re-edit test.
AGENTS.md reference: AGENTS.md:L53-L55
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Flagged for human review. Acknowledging the AGENTS.md convention that serialization changes get a Capybara editor→save→render→re-edit test. Two things shape whether it belongs in this PR:
- This change is client-side only — it adjusts DOMPurify (client) sanitization on the attachment re-inflation path. It does not touch the Loofah (server) or
highlightCode()(rendered view) stages. The round-trip risk this PR itself introduces is the client-sideSAFE_FOR_XMLattribute-drop, and the Vitest suite covers exactly that: it asserts the comment-bearing serializedcontentattribute survives sanitization (both base and safe-XML configs). A Loofah/rendered-view drop would be a pre-existing behavior unchanged by this diff. - A full Capybara round-trip is still a reasonable addition, but it needs the dummy-app system-test harness and would mainly exercise stages this PR doesn't change.
Flagging for a maintainer to decide whether to add the Capybara test here or as a follow-up, rather than adding a heavier system test that overlaps the unchanged server stages.
preserveSerializedContentHook set forceKeepAttr on any element bearing a content attribute. forceKeepAttr bypasses DOMPurify's tag-scoped attribute allowlist, so this retained an attacker-controlled content attribute on elements (e.g. <span content=...>) whose allowlist does not permit it — content is a registered allowed attribute only on the attachment tag. Restrict the preservation to the attachment element, so the forceKeepAttr bypass never applies elsewhere while the serialized attachment content is still preserved under SAFE_FOR_XML. Add a negative test asserting content on a non-attachment element is dropped.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
test/javascript/unit/editor/attachments/content_reinflation_sanitization.test.js:20
- This is a regular
img[onerror]payload, which the previous DOMPurify call already removes withSAFE_FOR_XML: false; therefore the test passes before this fix and does not regress the reported mutation-XSS path. Add a real mXSS payload that fails againstmainand exercise it in Playwright, since browser parsing/mutation behavior is not represented reliably by jsdom. This is required by the bug-regression and browser-test guidance inAGENTS.md:64-66andSTYLE.md:313-319.
test("neutralizes an XSS payload smuggled through attachment content", async () => {
const payload = ""<img src=x onerror=alert(document.domain)>""
editorElement = await createTestEditor({ value: attachment(payload) })
test/javascript/unit/editor/attachments/content_reinflation_sanitization.test.js:31
- This only round-trips through the in-memory editor value; it never crosses Action Text/Loofah persistence, rendered output, and re-editing. Because this PR changes sanitization and serialized attachment content,
AGENTS.md:53-55explicitly requires a Capybara system test covering editor → save → render → re-edit. Add that test so comment-bearingcontentis proven not to disappear in the actual storage path.
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) })
Checking the attachment tag name alone still force-kept content when a caller registered the attachment tag but omitted content from its attributes (e.g. the attachments-disabled setup, where the tag is importable but its attributes are not registered). forceKeepAttr then bypassed the caller's explicit omission. Track, in buildConfig, the set of tags the current config actually allows content on, and gate the preservation hook on membership. The bypass now never leaks past what the config permits: an attachment tag without content in its allowlist has the attribute stripped like any other. Add a test for that configuration.
| const tag = currentNode?.nodeName?.toLowerCase() | ||
| const isAllowedContent = hookEvent.attrName === "content" && contentAllowedTags.has(tag) |
What
Stored attachment content is re-inflated back into the editor by
CustomActionTextAttachmentNode#createDOM, which sanitizes the serializedcontentHTML and injects it into the live editor DOM. Thatsanitize()ran under the base DOMPurify config, which leavesSAFE_FOR_XMLoff — so the untrusted storage round-trip was not defended against mutation-XSS. This opts into DOMPurify's mXSS-safe mode per call on that re-inflation path.Why this is scoped per-call, not a global flip
The base config sets
SAFE_FOR_XML: falsedeliberately (see its comment) so serialized-HTML attributes survive. A globalSAFE_FOR_XML: truewould reintroduce exactly that regression: attachment content is serialized into thecontentattribute of an<action-text-attachment>, and DOMPurify'sSAFE_FOR_XMLattribute-value guard drops any attribute whose value contains an XML-unsafe sequence — a comment terminator (-->,--!>,]>, e.g. from Rails view annotations<!-- BEGIN app/views/... -->) or a raw</style-style close. That stripscontentand the attachment silently disappears on the round-trip. That guard runs before DOMPurify consultsforceKeepAttr, so a keep-hook alone is not enough.The preservation hook
A new
uponSanitizeAttributehook (preserveSerializedContentHook) neutralizes only the copy DOMPurify inspects for its XML-safety guard, then setsforceKeepAttr, which keeps the originalcontentvalue verbatim (the neutralized copy is never written to the DOM). Thecontentattribute is inert — always entity-escaped on serialization and only ever re-parsed and re-sanitized by the attachment node's own renderer — so preserving it is mXSS-safe. XML-unsafe values on ordinary attributes are still stripped.DOMPurify ignores a per-call config while a persistent config is set (
setConfig), sosanitize(html, { safeForXml: true })briefly swaps in the SAFE_FOR_XML config and restores the base config afterward.Changes
config/dom_purify.js— preservation hook for the serializedcontentattribute; clarify why the base config keepsSAFE_FOR_XML: falsehelpers/sanitization_helper.js—sanitize(html, { safeForXml })per-call opt-in via config swapnodes/custom_action_text_attachment_node.js—createDOMsanitizes re-inflated content withsafeForXml: trueTests
helpers/sanitization_helper.test.js— content attribute preserved underSAFE_FOR_XML(and in the base config); XML-unsafe values on non-content attributes still stripped (scoping proof, non-vacuous:titleis otherwise allowed)editor/attachments/content_reinflation_sanitization.test.js— full editor round-trip: XSS payload in attachment content neutralized; comment-bearing content survivesyarn test(vitest / jsdom): 132 tests, all passing.Follow-up (deliberately held)
The dual publish (npm
@37signals/lexxy+ Ruby gem) and thebc3/fizzyconsumer bumps are gated follow-ups and are not part of this PR.