Skip to content

Cancel the default paste when Trix handles a paste as a file paste - #1342

Open
jeremy wants to merge 1 commit into
mainfrom
security/prevent-default-on-file-paste
Open

Cancel the default paste when Trix handles a paste as a file paste#1342
jeremy wants to merge 1 commit into
mainfrom
security/prevent-default-on-file-paste

Conversation

@jeremy

@jeremy jeremy commented Aug 30, 2026

Copy link
Copy Markdown
Member

What

When a paste carries a file alongside HTML (a common mixed-clipboard shape — e.g. an image plus its text/html representation), insertFromPaste's processableFilePaste branch inserted the file but did not call preventDefault(). Every other branch in insertFromPaste (the URL, plain-text, and HTML branches) cancels the browser's default paste; this one didn't.

The result: Trix inserted the file and the browser's default action inserted the accompanying clipboard HTML directly into the contenteditable — unsanitized — until the next editor redraw replaced it with sanitized DOM.

Why it matters

The safety of that markup depends entirely on the redraw running and replacing it. If the redraw is disrupted, the browser-inserted markup survives in the live DOM and is later serialized back into the editor's value. Trix's serializer re-inflates data-trix-serialized-attributes into real element attributes, so surviving attacker markup on that path can be turned into executable content. Relying on a post-hoc redraw to clean up unsanitized DOM that was never supposed to be inserted is the wrong invariant.

The fix

Add this.event.preventDefault() to the file-paste branch so Trix fully owns the paste — the browser never inserts the accompanying clipboard HTML, and no untrusted markup lands in the editor DOM in the first place. This also matches the documented intent to prioritize files over HTML on paste (#1148), rather than inserting both.

Test

src/test/system/level_2_input_test.js adds a case dispatching a mixed file + text/html paste and asserting the beforeinput/insertFromPaste default is canceled, the file is inserted as an attachment, and none of the clipboard HTML's attributes survive into the editor value. Red before the one-line change, green after. Full suite green (473 passed).

Note for maintainers

This closes the paste entry vector. As defense in depth, the serializer's re-inflation of data-trix-serialized-attributes into arbitrary attributes (src/trix/core/serialization.js) remains a sharp edge that trusts sanitized DOM; it's worth a hard look at that sink independently, since its only legitimate producer emits a single src override.

Copilot AI balanced review requested due to automatic review settings August 30, 2026 03:39
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-08-30T05:02:55.894712Z 369c832 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Prevents browser-default HTML insertion when Trix handles mixed file/HTML pastes.

Changes:

  • Cancels default paste behavior for processable file pastes.
  • Updates the bundled Action Text asset.
  • Adds regression coverage for mixed file/HTML clipboard data.

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 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/trix/controllers/level_2_input_controller.js Cancels the browser’s default file-paste action.
src/test/system/level_2_input_test.js Tests cancellation and safe attachment insertion.
action_text-trix/app/assets/javascripts/trix.js Synchronizes the bundled implementation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@jeremy
jeremy force-pushed the security/prevent-default-on-file-paste branch 2 times, most recently from c7e067f to 369c832 Compare August 30, 2026 05:00
When a paste carries a file alongside HTML, Trix inserts the file but
left the browser's default paste uncanceled, so the browser also
inserted the accompanying clipboard HTML directly into the editor —
unsanitized — until the next redraw replaced it. If that redraw is
disrupted, the browser-inserted markup survives in the live DOM and is
later serialized back into the editor value, which can turn attacker
markup into executable content.

The URL and HTML branches of insertFromPaste already call
preventDefault. The file branch now does too, so Trix fully owns the
paste and untrusted clipboard HTML never lands in the editor DOM. This
also matches the documented intent to prioritize files over HTML on
paste (#1148), rather than inserting both.
@rosa
rosa force-pushed the security/prevent-default-on-file-paste branch from 369c832 to f8cc0dd Compare September 9, 2026 07:44
@rosa

rosa commented Sep 9, 2026

Copy link
Copy Markdown
Member

🤖 Rebased onto main@47004013 (clean, no conflicts).

Review change: the commit message claimed "Every other insertFromPaste branch already calls preventDefault". That isn't true — the dataTransferIsPlainText branch doesn't, and never has. Reworded to name the two branches that do (URL and HTML). The code change itself is untouched.

Tests (Node 18.20.8, Playwright Chromium):

  • yarn test → 502 tests, 473 passed, 0 failed, 29 skipped
  • yarn build is idempotent, so the "generated npm changes are not checked-in" step passes
  • action_text-trix bin/rails test:all → 1 run, 1 assertion, 0 failures

Negative control: reverting the one-line preventDefault() fails Level 2 Input > pasting a file alongside HTML cancels the browser's default paste. Real differential, not a tautology.

Siblings checked, no change proposed: level_0_input_controller.js calls preventDefault() unconditionally at the end of its paste handler, so every branch there is already covered. The one remaining uncancelled branch in level_2_input_controller.js is the plain-text one, and dataTransferIsPlainText only returns true when the clipboard's HTML flavour has no element children at all, so the browser's default insert there cannot introduce markup.

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 main (run 34325807733) and that job fails there too, with JSON::GeneratorError: detected duplicate key "latency" inside Rails' system-test network-emulation helper, followed by a net::ERR_INTERNET_DISCONNECTED cascade through the remaining system tests. The job last passed on 2026-09-06 (#1352), so this arrived from upstream rails/rails main since then. It needs its own fix; nothing on this branch can address it.

One Action Text cell (4.0, 8-1-stable) also failed on the first run with Ferrum::ProcessTimeoutError: Browser did not produce websocket url within 10 seconds and passed on re-run. That is the flake Jorge's process_timeout: 60 commit in #1337/#1338 fixes; this branch doesn't carry it, and neither does #1350, which hits the same cells.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants