fix: verify terminal clipboard writes so success never lies (#5611)#6579
fix: verify terminal clipboard writes so success never lies (#5611)#6579nwparker wants to merge 2 commits into
Conversation
Terminal copy paths (Ctrl+Shift+C, right-click, context-menu copy, copy-on-select, and OSC 52) wrote to the clipboard via window.api.ui.writeClipboardText but swallowed write failures and never checked that the OS clipboard actually changed. The main-process clipboard:writeText handler called clipboard.writeText() and returned without reading back, so a stale OS clipboard (Electron returns without throwing even when the write did not land, seen on Windows with Copilot conversation text) looked like success while the clipboard was unchanged. - clipboard-ipc-handlers.ts: after the existing trusted-sender auth and text-size-limit checks, write then read back the standard clipboard and throw on mismatch. The X11 PRIMARY "selection" clipboard is deliberately not verified, since ownership can pass to another app and a read-back would surface false failures on Linux primary-selection copies. - terminal-selection-copy.ts: shared helper so every renderer copy path propagates a write rejection instead of silently claiming success, and only clears the xterm selection after the write resolves. - osc52-clipboard.ts: add onWriteFailure so a TUI yank that never reaches the host clipboard surfaces a toast instead of a silent no-op. - osc52-clipboard-blocked-toast.ts: add showOsc52ClipboardFailedToast. Supersedes community PR #5634. Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com>
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (24)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Verification evidenceUnit tests (vitest)Key regression coverage added:
GatesLive Electron verification (CDP attach to this worktree's dev build)Attached to instance // standard clipboard (Ctrl+Shift+C / right-click / context-menu / copy-on-select all use this)
await window.api.ui.writeClipboardText(marker)
const readBack = await window.api.ui.readClipboardText()
// selection clipboard (Linux PRIMARY path)
await window.api.ui.writeSelectionClipboardText(marker2)Result: { "stdWriteError": null, "stdReadBackMatches": true, "selWriteError": null }
Before vs after (reasoning)
Forcing a genuine OS-level clipboard write failure is not reproducible on macOS in this environment, so the throw-on-mismatch path is proven by the unit test rather than a live induced failure; the live run proves the verification does not regress legitimate copies on either clipboard. |
Takeover updatePushed Outcome:
Validation:
|
Zero-tradeoff follow-upAudited the remaining clipboard-race question.
Validation: No code changes were needed in this follow-up. |
|
@nwparker Do you know when this fix will be merged? Will it make it into the next release? I'm experiencing the same file copying issue on Windows. |
2 participants
🧑🤝🧑 Impact & ELI5
1) What's broken today (ELI5). When you select text inside a terminal pane in Orca — for example, an answer from a Copilot/agent conversation — and copy it (Ctrl+C, right-click, or copy-on-select), Orca cheerfully says "Copied to clipboard" and turns blue. But on Windows the text never actually lands on the system clipboard: paste it into another app and you get nothing, or the old contents. So the app is lying about success. This was reported on Windows; it's a real "I copied this and lost it" trust-breaker for anyone using the terminal/agent panes there, not just a cosmetic glitch.
2) What this PR does (ELI5). Think of it like a printer that flashes "Done!" the instant you hit print, without ever checking whether paper came out. Terminal copies now use a verified desktop write: after writing the text, Orca reads the standard clipboard back and confirms the words that come out match the words that went in. If they don't match, it stops calling it a success. The verification is scoped to terminal copy paths, so unrelated app copy buttons do not pay a new read-back cost or get a new rejection mode. Failed terminal selection copies leave the text highlighted and show a small error notice; failed agent/TUI OSC 52 copies also surface a toast instead of silently doing nothing. So: terminal copy success now means genuinely pasteable, while the wider clipboard API keeps its previous lightweight behavior unless a caller opts into verification.
3) Tradeoffs / regressions — none known. No PR-introduced tradeoff remains from this fix. The original broad caveat is gone: Orca no longer adds a read-back to every app clipboard write. Only desktop terminal text-copy paths that need the "Copied means pasteable" guarantee opt into verification. Paired web clients keep the browser clipboard write promise as their success signal because browser read permission is a separate permission surface.
OS clipboard boundary (audited, not a PR tradeoff). Electron's
clipboardAPI exposes read/write/format calls, but no sequence number, change event, ownership token, or atomic "write and prove this exact owner still owns the clipboard" operation. Orca verifies the observable user contract immediately after a desktop terminal copy: the standard text clipboard must match the copied text. If another app or clipboard manager changes the standard clipboard before that read-back, the copied text is no longer pasteable at verification time, so Orca correctly withholds success instead of reporting a verified copy. The Linux X11 PRIMARY "selection" clipboard remains deliberately exempt, because ownership normally passes to whichever app last selected text.Manual failures are now visible. Ctrl/shortcut copy, context-menu copy, right-click copy, copy-on-select, and OSC 52 all keep failed terminal selection text available to retry and surface a one-time failure toast instead of silently swallowing the verified-write rejection.
Summary
Fixes #5611.
Copying selected terminal text (including Copilot/agent conversation text) showed a success state while the OS clipboard stayed unchanged — reported on Windows. Every terminal copy path (Ctrl+Shift+C, right-click copy, context-menu copy, copy-on-select, and OSC 52) wrote via
window.api.ui.writeClipboardTextbut swallowed write failures with an empty.catchand never confirmed the OS clipboard actually changed. The main-processclipboard:writeTexthandler calledclipboard.writeText()and returned without reading back, so a stale OS clipboard (Electron returns without throwing even when the write did not land) looked like success.This makes terminal copy "success" mean pasteable:
clipboard-ipc-handlers.ts— keeps the existing trusted-sender auth and text-size-limit checks, then verifies only when the caller passes{ verify: true }. Default app text writes stay single-write; verified standard-clipboard writes read back and throwClipboard write verification failedon mismatch. The X11 PRIMARYselectionclipboard is deliberately not verified, because its ownership can pass to whichever app last selected text.terminal-clipboard-write.ts(new) — terminal desktop copies opt into verified writes; paired web clients keep the browser clipboard write promise instead of forcing a browser read permission.terminal-selection-copy.ts(new) — one shared helper that all renderer terminal selection copy paths route through, so a write rejection propagates and the xterm selection is cleared only after the write resolves.terminal-clipboard-copy-failure-toast.ts(new) — manual terminal copy failures now show a one-time toast instead of quietly disappearing.osc52-clipboard.ts— addsonWriteFailureso a TUI yank (tmux/nvim/fzf/Copilot OSC 52) that never reaches the host clipboard surfaces a toast instead of a silent no-op.osc52-clipboard-blocked-toast.ts— addsshowOsc52ClipboardFailedToast.This supersedes community PR #5634 (adopted, rebased onto current
main, and reconciled to keep the current async IPC handlers, theassertTrustedClipboardSender/text-size-limit security checks, thestore: Storesignature, and the newshouldWritePrimarySelectioncopy-on-select branch — the verbatim PR would have dropped those and failed to compile). It does not close #5634.Screenshots
This is a clipboard-state bug; the user-visible regression (false "success" with an unchanged clipboard) has no static visual surface to screenshot. Verified live in the running Electron build instead — see the PR comment with CDP evidence (a real
clipboard:writeTextwrite now reads back asmatches: truewith no false error, and theselectionpath does not throw a false verification failure).Testing
pnpm exec vitest run --config config/vitest.config.ts src/main/window/clipboard-ipc-handlers.test.ts src/renderer/src/components/terminal-pane/terminal-selection-copy.test.ts src/renderer/src/components/terminal-pane/terminal-clipboard-write.test.ts src/renderer/src/components/terminal-pane/terminal-clipboard-copy-failure-toast.test.ts src/renderer/src/components/terminal-pane/osc52-clipboard.test.ts src/renderer/src/components/terminal-pane/osc52-clipboard-blocked-toast.test.ts— 6 files / 53 tests passedpnpm exec oxlint <touched TypeScript files>— cleanpnpm run typecheck:node,pnpm run typecheck:cli,pnpm run typecheck:web— all passpnpm run verify:localization-catalogandpnpm run verify:localization-coverage— passpnpm build(not run; targeted tests, lint, localization, and type gates pass)AI Review Report
Self-reviewed adversarially against
git merge-base origin/main HEAD.await, so large-payload yielding still happens first. Default app clipboard writes no longer read back; terminal callers opt in where the success UI needs proof.metaKey, no path-separator assumptions, no new platform-specific code.Security Audit
Adopted code re-audited assuming the original author was untrusted.
assertTrustedClipboardSender(event)is preserved on every handler; the unauthorized-sender test still passes. The opt-in read-back uses Electron's already-permittedclipboard.readText()only to compare against what we just wrote — it returns nothing new to the renderer and is not used by default app writes.assertClipboardTextWriteWithinLimitWithYield(text, options)still runs before any write; oversized-write rejection tests still pass.eval,child_process/exec, fs access, secret access, path traversal, or new runtime dependencies introduced. New renderer import is still a type-only@xterm/xterm.Notes
translate()keys total for OSC 52 and manual terminal-copy failure toasts, registered across all locale catalogs viapnpm run sync:localization-catalog.Made with Orca 🐋