Pasting an embedded-content attachment may destroy it - #1337
Conversation
There was a problem hiding this comment.
Pull request overview
Preserves Trix attachment attributes containing XML-sensitive markup during sanitization.
Changes:
- Stashes and restores
data-trix-*attributes. - Continues stripping serialized and non-Trix unsafe attributes.
- Adds unit, parser, and paste regression 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 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/trix/models/html_sanitizer.js |
Restores permitted Trix attributes after DOMPurify sanitization. |
src/test/unit/html_sanitizer_test.js |
Tests attribute preservation and removal. |
src/test/unit/html_parser_test.js |
Tests parsing embedded attachment markup. |
src/test/system/pasting_test.js |
Tests pasting embedded HTML attachments. |
action_text-trix/app/assets/javascripts/trix.js |
Updates the bundled sanitizer implementation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| DOMPurify.addHook("afterSanitizeAttributes", function (node) { | ||
| stashedAttributes.forEach(([ name, value ]) => { | ||
| if (value !== null && !node.hasAttribute(name)) { | ||
| node.setAttribute(name, value) | ||
| } |
rails/rails main's yarn.lock now pulls errorstacks@2.4.2, which declares node >= 24, so `yarn install --frozen-lockfile` in the Rails clone fails on Node 18 before any test runs. Same fix as #1337.
Ferrum::ProcessTimeoutError ("Browser did not produce websocket url
within 10 seconds") fails the Action Text matrix intermittently on loaded
runners. Same fix as #1337.
438aa12 to
976f806
Compare
#1337 stashed every data-trix-* attribute DOMPurify's SAFE_FOR_XML pass dropped and restored it in afterSanitizeAttributes. The escaping added here makes that unnecessary: sanitizeElement escapes the angle brackets in the JSON attachment attributes before DOMPurify runs, so SAFE_FOR_XML never drops them and forceKeepAttr keeps them. The blanket restore's only remaining effect was re-admitting malformed or non-JSON data-trix-* values the pass deliberately dropped, so remove it and the module-global stash and rely on escaping. Non-JSON attachment attributes are unusable on read anyway. Full suite green.
#1337 stashed every data-trix-* attribute DOMPurify's SAFE_FOR_XML pass dropped and restored it in afterSanitizeAttributes. The escaping added here makes that unnecessary: sanitizeElement escapes the angle brackets in the JSON attachment attributes before DOMPurify runs, so SAFE_FOR_XML never drops them and forceKeepAttr keeps them. The blanket restore's only remaining effect was re-admitting malformed or non-JSON data-trix-* values the pass deliberately dropped, so remove it and the module-global stash and rely on escaping. Non-JSON attachment attributes are unusable on read anyway. Full suite green.
DOMPurify removes an attribute whose value contains `</style>`, `</title>`, `</textarea>`, `-->` or `]>` before it honors `forceKeepAttr`, so the hook that protects `data-trix-*` never takes effect under `SAFE_FOR_XML`. Stash those values and restore them in `afterSanitizeAttributes` instead.
Ferrum's 10 second default process_timeout is too tight on loaded CI runners, where Chrome intermittently fails to publish its websocket URL in time and the whole matrix cell errors before running.
976f806 to
c55b6d8
Compare
#1337 stashed every data-trix-* attribute DOMPurify's SAFE_FOR_XML pass dropped and restored it in afterSanitizeAttributes. The escaping added here makes that unnecessary: sanitizeElement escapes the angle brackets in the JSON attachment attributes before DOMPurify runs, so SAFE_FOR_XML never drops them and forceKeepAttr keeps them. The blanket restore's only remaining effect was re-admitting malformed or non-JSON data-trix-* values the pass deliberately dropped, so remove it and the module-global stash and rely on escaping. Non-JSON attachment attributes are unusable on read anyway.
|
🤖 Rebased onto main@47004013 (clean, no conflicts). No content changes: this is Jorge's branch, and the one open review finding (the stash being a module global rather than per-node) is dissolved by #1338 rather than something worth rewriting here. Tests (Node 18.20.8, Playwright Chromium):
Negative control: removing the stash/restore hook fails 3 tests — State of play for whoever merges: #1338 is stacked on this branch and carries both of these commits verbatim, so merging #1338 lands this work with its authorship intact; #1338 then replaces the stash/restore with escaping the angle brackets in the JSON before DOMPurify sees them. If that's the plan, this PR is superseded — and this branch must not be deleted before #1338 is either merged or retargeted at One thing that could be split out: "Give Chrome longer to boot in system tests" is a CI-stability change to This PR had no independent adversarial review round in this pass — the reviewer pool was saturated. Everything above is my own review plus the negative control named. CI after this push: 20 of 21 checks green, including Browser tests (Sauce: Windows Chrome, Firefox and Edge, 476 passed each) and the full Action Text matrix. The one red check, "Downstream Rails integration tests", is not from this branch. I dispatched a control run of the same workflow on unmodified Browser tests also failed on the first run purely on Sauce infrastructure ("The browser was unable to create and start a test page after 120000ms", on the Android chrome target) while every desktop browser reported 476 passed, 0 failed. Green on re-run. |
…e DOMPurify (#1338) * Escape angle brackets in attachment JSON so pasted attachments survive DOMPurify Copying an attachment out of Trix and pasting it back silently dropped it whenever the attachment JSON contained "</style>" or another sequence DOMPurify's SAFE_FOR_XML mode treats as a raw-text or comment terminator. Paste runs the clipboard's text/html through HTMLParser under SAFE_FOR_XML, and DOMPurify's attribute rule removes the whole data-trix-attachment attribute on a match, before the forceKeepAttr set by Trix's uponSanitizeAttribute hook is honored. Quoted mail carrying an embedded <style> block is the common case. Dragging the same content was lossless. Escape "<" and ">" inside the JSON as "\u003c" and "\u003e" at both ends: AttachmentView emits data-trix-attachment and data-trix-attributes that way, so Trix's own HTML never carries a trigger sequence, and HTMLSanitizer rewrites those attributes before DOMPurify sees them, so stored, server- rendered and older-Trix HTML with literal brackets survives too. In JSON text angle brackets only occur inside string literals, where the escapes spell the same characters, so JSON.parse reads back the same value; the sanitizer only rewrites values that already parse as JSON. * Drop the now-redundant post-sanitize attribute restore #1337 stashed every data-trix-* attribute DOMPurify's SAFE_FOR_XML pass dropped and restored it in afterSanitizeAttributes. The escaping added here makes that unnecessary: sanitizeElement escapes the angle brackets in the JSON attachment attributes before DOMPurify runs, so SAFE_FOR_XML never drops them and forceKeepAttr keeps them. The blanket restore's only remaining effect was re-admitting malformed or non-JSON data-trix-* values the pass deliberately dropped, so remove it and the module-global stash and rely on escaping. Non-JSON attachment attributes are unusable on read anyway. * Cut clipboard bytes after the closing html tag without cutting attachments Pasted HTML is trimmed at "</html>" because Windows browsers can append clipboard bytes after it, which the parser would otherwise append to the body as text. The trim ran as a plain string replace, so a "</html>" inside an attribute value cut the string mid-attribute and dropped everything from that element on. Attachment JSON hits this whenever the content is a full HTML document, as stored and server-rendered attachments carry it with literal angle brackets: pasting a quoted mail attachment lost the attachment and everything after it. Scan for the closing tag as a tag, skipping over quoted attribute values and comments, so only the real end tag ends the document. * Test that a Trix attribute that isn't JSON is removed under SAFE_FOR_XML * Keep the closing html tag scan linear The tag pattern let whitespace and an unquoted attribute value match the same characters more than one way, so a pasted string such as "<A !=" with " !=" repeated took exponential time to reject: 50 seconds for 125 characters. Give every part of a tag a single way to match, and let an unterminated comment, quoted value or tag run to the end of the string as the HTML tokenizer does, so a scan never restarts inside one. * Find the closing html tag with the browser's tokenizer A regex that knows about attribute values and comments still isn't the tokenizer: a "</html>" inside a style or textarea element, or a comment closed with "--!>", was taken for the closing tag, and each such case would need its own rule. Ask the browser instead. Swap every "</html" in the string for a marker start tag and parse once; the first marker that comes out as an element was tokenized as a tag, so that is where the document ends, and any marker inside an attribute value, a comment or raw text stays text. One extra parse, only when the string contains "</html" at all, and no String.prototype.matchAll for the Safari 12.1 target. * Keep the closing html tag marker inert wherever it lands The marker carried its offset in a quoted attribute value, and that quote changed the tokenizer's state when the marker landed inside a quoted attribute value: with two "</html>" in one value, the first marker closed the value and its element, and the second was parsed as a real tag. A quoted mail chain with two HTML documents in one attachment hit this. Write the offset unquoted, so the marker carries nothing that changes state in any context. Accept only markers whose offset points at a "</html" in the source, so an element of the same name supplied by the input can't stand in for one, and bound the tag name by HTML's ASCII whitespace rather than the JavaScript \s class, which the tokenizer doesn't share. * Name the closing html tag marker per call An element in the input with the marker's name and an offset pointing at a "</html" inside an attribute value passed for a marker and cut the paste there. Give the marker a name chosen per call instead of checking offsets after the fact: nothing in the input can carry a name it doesn't know, so every element found is one the scan inserted. * Forge the marker at the offset it names --------- Co-authored-by: Jeremy Daer <jeremykemper@mac.com>
Pasting (or otherwise inserting) an attachment whose
data-trix-attachmentvalue contains</style>,</title>,</textarea>,-->or]>destroys the attachment: the figure and the attachment are gone, and the content lands as flattened text. Embedded email content is the common case — a<style>block, or an Outlook conditional comment ending in<![endif]-->.Trix already declares the intent to protect its own attributes with an
uponSanitizeAttributehook that setsforceKeepAttrfor/^data-trix-/. The hook never takes effect: DOMPurify checks the attribute value againstSAFE_FOR_XMLandcontinues — dropping the attribute — before it reaches theforceKeepAttrguard. That order is deliberate upstream (DOMPurifyfa542df7, shipped in 3.1.6, "safer hooks"), and it is unchanged through the current release, soforceKeepAttris the wrong lever here rather than something a version bump fixes.Only
Composition#insertHTMLpassesSAFE_FOR_XML: true, which is why loading a document keeps the attachment while pasting the same markup loses it.So instead of relying on
forceKeepAttr, stash thedata-trix-*values inuponSanitizeAttributeand restore them inafterSanitizeAttributes, once DOMPurify has finished with the node. Nothing else is rescued: every other attribute still loses these values underSAFE_FOR_XML, anddata-trix-serialized-attributesis still stripped.