fix(paste): neutralize bracketed-paste markers in pasted payloads - #280
Conversation
All three UIs wrapped pasted text in ESC[200~ ... ESC[201~ when DEC mode 2004 is active without inspecting the payload, so clipboard or dropped content containing ESC[201~ terminated the bracketed region early and the tail reached the shell as directly-typed input — a newline then executes it. Found by CodeRabbit on #279 (in relocated code); a cross-UI audit showed iced, GTK, and Swift each hand-rolled the same unsafe wrap. Adds roost_ui_model::bracketed_paste::wrap, used by both Rust UIs, and a hand-mirrored Swift BracketedPaste.swift with shared test vectors (shell_escape.rs <-> ShellEscape.swift precedent). Sanitization scans the OUTPUT tail after each byte so a removal cannot splice its neighbours into a fresh marker. shell_escape now drops ESC as well, closing the same smuggling route through crafted filenames on the file-drop path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 4 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe change adds shared bracketed-paste wrappers for Rust and macOS, removes embedded paste markers, updates paste delivery paths, and removes ESC characters during shell escaping. Tests cover framing, marker handling, UTF-8 content, and ESC removal. ChangesPaste sanitization and integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PasteEntryPoint
participant BracketedPasteWrapper
participant TerminalCallback
PasteEntryPoint->>BracketedPasteWrapper: wrap payload for selected bracketed mode
BracketedPasteWrapper-->>PasteEntryPoint: sanitized framed or plain bytes
PasteEntryPoint->>TerminalCallback: deliver paste bytes
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/roost-ui-model/src/shell_escape.rs`:
- Around line 15-27: Reject paths containing U+001B before shell escaping so
distinct filenames cannot collapse to identical PTY input. Update the
drop-content path handling in drop_content.rs and the macOS drop handling in
TerminalView.swift, and ensure the escape implementations in shell_escape.rs and
ShellEscape.swift are not relied on to silently remove ESC; apply the same
rejection behavior consistently at both platforms.
In `@mac/Tests/RoostTests/ShellEscapeTests.swift`:
- Around line 51-58: Update the added test in ShellEscapeTests to use
swift-testing rather than XCTest: convert the containing ShellEscapeTests suite
to `@Suite` and its test methods to `@Test` as needed, replacing XCTAssertEqual
assertions with `#expect` while preserving both escape-byte vectors and expected
results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d483a101-8f57-4a7a-a466-99f4fa4e13a6
📒 Files selected for processing (10)
crates/roost-iced/src/app/interactions.rscrates/roost-linux/src/terminal_view.rscrates/roost-ui-model/src/bracketed_paste.rscrates/roost-ui-model/src/lib.rscrates/roost-ui-model/src/shell_escape.rsmac/Sources/Roost/BracketedPaste.swiftmac/Sources/Roost/ShellEscape.swiftmac/Sources/Roost/TerminalView.swiftmac/Tests/RoostTests/BracketedPasteTests.swiftmac/Tests/RoostTests/ShellEscapeTests.swift
Left by removing a scratch adversarial test locally; caught by CI's cargo fmt --check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
CodeRabbit review on #280: silently dropping ESC in shell_escape meant the escaped path no longer referred to the real file, and two distinct filenames could collapse to identical PTY input. drop_content already rejects newline-bearing paths, so ESC now follows that precedent on both sides and escape() is a pure escaper again (bodies byte-identical to main). bracketed_paste::wrap remains the paste-boundary defense, so the injection fix is unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
CI run 30796756743 aborted swiftpm-testing-helper with SIGABRT and no failing test — the swift-testing runner bug ShellEscapeTests.swift's header already documents for swarms of trivially fast value-checks under Xcode 26.x. Same assertions and vectors, separate harness. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
1 participant
The bug (security, pre-existing, all three UIs)
When DEC mode 2004 is active, all three UIs wrapped pasted text in
ESC[200~ … ESC[201~without inspecting the payload. Clipboard or dropped content containingESC[201~terminates the bracketed region early, and everything after it reaches the shell as directly typed input — a trailing newline executes it. Copying crafted text from a web page is enough.CodeRabbit flagged this on #279 (in code that PR merely relocated). A cross-UI audit found the same hole independently hand-rolled in each UI:
app/interactions.rspaste_bytesterminal_view.rspaste_text_intoTerminalView.swiftsendBracketedPasteSecondary route:
shell_escape(Rust + its Swift mirror) escaped shell metacharacters but not ESC, so a maliciously-named dropped file smuggled the same sequence through the file-drop path.The fix
roost_ui_model::bracketed_paste::wrap(text, bracketed)— one shared implementation for both Rust UIs, replacing two hand-rolled wraps. Sanitization scans the output tail after each byte, not the input: an input-side pass lets a removal splice its neighbours into a fresh marker (ESC[20+ESC[200~+0~reassembles). Verified against six splice/nesting attack vectors.mac/Sources/Roost/BracketedPaste.swift— hand-mirrored with byte-identical semantics and the same test vectors, following the establishedshell_escape.rs↔ShellEscape.swiftconvention (cross-referencing doc comments both ways). TakesDatabecause Swift's image/multi-file paste paths join before wrapping.shell_escapedrops ESC (removal, not backslash — a backslash doesn't neutralize it for the VT parser), Rust and Swift, with the vector added to both suites. Defense in depth: file drops now get sanitized at both the escape and paste boundaries.Semantics: empty → zero bytes; unbracketed → verbatim (no region to escape); bracketed → markers removed as contiguous 6-byte sequences, partial prefixes preserved as ordinary payload, UTF-8 either side untouched.
One behavior alignment: Swift previously emitted a bare
ESC[200~ESC[201~for an empty payload; it now emits nothing, matching Rust. All four Swift call sites already guard emptiness.Testing
cargo test -p roost-ui-model278 passed;-p roost-iced137;-p roost-linuxall suites greencd mac && swift test— 698 tests, 29 suites passed (incl. newBracketedPasteTests,ShellEscapeTests.testDropsEscapeByte)-D warnings+cargo fmt --checkclean (the 3roost-linuxclippytype_complexityerrors are pre-existing onmain— verified with these changes stashed)Note
Left open for your review rather than merged — it touches the Swift daily driver and wasn't a requested change.
🤖 Generated with Claude Code
https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
Summary by CodeRabbit