Escape angle brackets in attachment JSON so pasted attachments survive DOMPurify - #1338
Conversation
02743cc to
aafa512
Compare
There was a problem hiding this comment.
Pull request overview
Escapes angle brackets in attachment JSON before rendering and sanitization, preventing DOMPurify from dropping attachments during paste.
Changes:
- Added lossless JSON angle-bracket escaping.
- Applied escaping during attachment rendering and pre-sanitization.
- Added comprehensive unit and system regression coverage.
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 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/trix/views/attachment_view.js |
Escapes emitted attachment JSON attributes. |
src/trix/models/html_sanitizer.js |
Escapes valid legacy JSON before DOMPurify. |
src/trix/core/helpers/strings.js |
Adds the escaping helper. |
src/test/unit/html_sanitizer_test.js |
Covers DOMPurify trigger sequences. |
src/test/unit/html_parser_test.js |
Tests attachment parsing during paste. |
src/test/unit/helpers/strings_test.js |
Tests escaping and round-trip behavior. |
src/test/unit/document_view_test.js |
Verifies safely rendered attributes. |
src/test/unit.js |
Registers the new helper tests. |
src/test/test_helpers/fixtures/fixtures.js |
Updates expected attachment markup. |
src/test/test_helpers/editor_helpers.js |
Adds stored attachment HTML generation. |
src/test/system/pasting_test.js |
Adds end-to-end paste regressions. |
action_text-trix/app/assets/javascripts/trix.js |
Updates the generated browser bundle. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
2515e30 to
affbe0f
Compare
affbe0f to
62eab4c
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62eab4c646
ℹ️ 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".
62eab4c to
c044c40
Compare
Bumps [immutable](https://github.com/immutable-js/immutable-js) from 5.1.5 to 5.1.9. - [Release notes](https://github.com/immutable-js/immutable-js/releases) - [Changelog](https://github.com/immutable-js/immutable-js/blob/main/CHANGELOG.md) - [Commits](immutable-js/immutable-js@v5.1.5...v5.1.9) --- updated-dependencies: - dependency-name: immutable dependency-version: 5.1.9 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.
…e 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 with an embedded <style> block is the common case: in HEY, 67 of 153 real mail bodies lost their embedded content on paste. 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.
#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.
c1e9df8 to
8dd9a1f
Compare
Stacked on #1337 (base:
fix-data-trix-attributes-safe-for-xml). This diff is only the layer on top; merge #1337 first.The bug
Copying an attachment out of Trix and pasting it back silently dropped it whenever the attachment JSON contained
</style>or another sequence DOMPurify'sSAFE_FOR_XMLmode treats as a raw-text or comment terminator. Paste runs the clipboard'stext/htmlthroughHTMLParserunderSAFE_FOR_XML, and DOMPurify's attribute rule removes the wholedata-trix-attachmentattribute on a match, before theforceKeepAttrset by Trix'suponSanitizeAttributehook is honored. Quoted mail with an embedded<style>block is the common case: in HEY, 67 of 153 real mail bodies lost their embedded content on paste. Dragging the same content was lossless.What #1337 does, and what this adds
#1337 stashes every
data-trix-*value inuponSanitizeAttributeand puts it back inafterSanitizeAttributesonce DOMPurify has dropped it. That stops the loss at the sanitizer for everydata-trix-*attribute, without changing the HTML Trix emits.This PR removes the trigger from the value itself, so nothing has to be put back:
AttachmentViewwritesdata-trix-attachmentanddata-trix-attributeswith<and>escaped as</>. Trix's own HTML then never carries aSAFE_FOR_XMLtrigger, so it survives a paste into any Trix build — including releases without either fix — and into anything else that runs DOMPurify in that mode.HTMLSanitizerrewrites those two attributes the same way before DOMPurify runs, so stored, server-rendered and older-Trix HTML with literal brackets is safe by construction: the rule has nothing to match and the restore in Pasting an embedded-content attachment may destroy it #1337 never has to fire for them.The rewrite is lossless: in JSON text, angle brackets only occur inside string literals, where the escapes spell the same characters, so
JSON.parsereads back the same value. Only values that already parse as JSON are rewritten; a malformed value is left alone, sinceHTMLParserignores it either way.Why both layers belong: #1337 is the general control — it covers every
data-trix-*attribute, including ones that don't hold JSON — and this PR makes the two JSON attributes safe before that control is needed, on both ends of the pipe, so correctness doesn't rest on DOMPurify's hook ordering or on the consumer having the hook at all.What the rebase changed
process_timeout) are byte-identical to the ones in Pasting an embedded-content attachment may destroy it #1337 and dropped out of the rebase; Pasting an embedded-content attachment may destroy it #1337 is green with them, so there is nothing to split into a separate PR.keeps Trix attributes containing markup when sanitizing for XMLnow asserts parse-equality plus no raw angle brackets instead of byte-identity, because the value comes back escaped under this layer. Its other tests (html_parser_test,pasting_test) pass unchanged.html_sanitizer.jsand its test describe the composition rather than each fix on its own.Proof
helpers/strings_test: escapes every bracket, round-trips throughJSON.parse(backslashes before brackets, pre-escaped input, surrogate pairs, nesting), idempotent.html_sanitizer_test: attachment and caption JSON containing each of the twelve sequences in DOMPurify's regex survive underSAFE_FOR_XMLwith no raw brackets; a</style>attachment nested inside another attachment's content survives; malformed JSON is untouched.html_parser_test: the pasted markup parses back to an attachment with the original content and caption, including the nested case.document_view_test: rendered attachment JSON carries no raw brackets and parses back to the original.pasting_test(system): pasting the editor's ownvaluefor a<style>-bearing attachment with a</style>caption yields a second, identical attachment; the same for stored-shape markup with literal brackets.attachment_view.jsalone fails the render tests, revertinghtml_sanitizer.jsalone fails the sanitizer and parser tests.