fix: sanitize untrusted terminal output and markdown link/image URLs - #91
Conversation
Model/tool output reached two sinks unsanitized. In the web UI, solid-markdown defaults to transformLinkUri: null, so a prompt-injected javascript: href renders live (one click -> script in the app origin -> reads the ZeroID key from localStorage -> drives the authenticated socket); remote image src is a zero-click exfil channel. In the TUI, model content is written to scrollback without stripping escapes, so an embedded OSC 52 can rewrite the user's clipboard and cursor/screen/DCS sequences can corrupt the terminal. - web/src/lib/sanitize-url.ts: safeLinkUri (scheme allowlist, defeats control-char obfuscation) + safeImageUri (drops remote images); wired onto MessageRow's SolidMarkdown. - src/tui/ansi/codes.ts: sanitizeTerminalOutput strips OSC (except OSC-8), DCS/APC/PM/SOS, non-SGR CSI, and unsafe C0 while preserving SGR + OSC-8; wired into ScrollbackWriter.#writeAtomic (the single TTY write chokepoint). Tests: bun sanitize-terminal (10) + vitest sanitize-url (9). Full suites green. The legacy readline client (src/terminal/client.ts) shares the class and is deferred to a follow-up (needs a testability refactor). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds terminal output sanitization for untrusted escape sequences in scrollback writes, plus markdown URI sanitization for transcript links and images, with tests covering allowed and blocked forms. ChangesTerminal Output Sanitization
Estimated code review effort: 3 (Moderate) | ~25 minutes Markdown URL Sanitization
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #91 +/- ##
==========================================
+ Coverage 66.86% 66.91% +0.04%
==========================================
Files 65 65
Lines 11165 11181 +16
==========================================
+ Hits 7466 7482 +16
Misses 3699 3699
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web/src/lib/sanitize-url.test.ts (1)
53-56: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd regression coverage for protocol-relative bypass.
None of the existing cases cover
//attacker.example/p.gif(no scheme, leading double-slash). This exact input bypassessafeImageUri's remote-fetch block (see comment onsanitize-url.ts); add it here alongside the existing remote-image test to prevent regression once fixed.✅ Suggested addition
it("blocks remote images to prevent zero-click exfiltration", () => { expect(safeImageUri("https://attacker.example/p.gif?leak=secret")).toBe(""); expect(safeImageUri("http://attacker.example/p.gif")).toBe(""); + expect(safeImageUri("//attacker.example/p.gif")).toBe(""); });🤖 Prompt for 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. In `@web/src/lib/sanitize-url.test.ts` around lines 53 - 56, Add regression coverage in the safeImageUri test block for protocol-relative remote URLs by asserting that a value like "//attacker.example/p.gif" is rejected the same way as the existing http/https cases. Update the existing remote-image test in sanitize-url.test.ts alongside safeImageUri so the suite covers this bypass pattern and prevents regressions in the remote-fetch block.
🤖 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 `@src/tui/ansi/codes.ts`:
- Around line 121-126: The ANSI sanitizer currently removes complete CSI/ESC
sequences in codes.ts but leaves an incomplete trailing ESC [ introducer at EOF,
which can be carried into the next write. Update sanitizeTerminalOutput by
extending the existing ANSI_CSI_NON_SGR / ANSI_ESC_OTHER handling so any
dangling ESC or ESC [ with optional params/intermediates at the end of the chunk
is stripped too, keeping the behavior aligned with the current CSI/SGR parsing
rules.
In `@web/src/lib/sanitize-url.ts`:
- Around line 59-80: The safeImageUri sanitizer currently allows
protocol-relative URLs because the root-relative shortcut treats any leading
slash as safe. Update safeImageUri to explicitly reject URLs starting with "//"
before the existing first === "/" check, so network-path references are blocked
while preserving true root-relative and anchor handling.
---
Nitpick comments:
In `@web/src/lib/sanitize-url.test.ts`:
- Around line 53-56: Add regression coverage in the safeImageUri test block for
protocol-relative remote URLs by asserting that a value like
"//attacker.example/p.gif" is rejected the same way as the existing http/https
cases. Update the existing remote-image test in sanitize-url.test.ts alongside
safeImageUri so the suite covers this bypass pattern and prevents regressions in
the remote-fetch block.
🪄 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 Plus
Run ID: e28ffbe2-3f66-46c4-9093-8a420c77b626
📒 Files selected for processing (6)
src/tests/sanitize-terminal.test.tssrc/tui/ansi/codes.tssrc/tui/ansi/scrollback-writer.tsweb/src/components/transcript/MessageRow.tsxweb/src/lib/sanitize-url.test.tsweb/src/lib/sanitize-url.ts
Addresses two CodeRabbit review findings on the sanitizers: - safeImageUri treated `//host/x.png` (and backslash variants `/\`, `\\`) as same-origin root-relative, but protocol-relative URLs fetch cross-origin under the page scheme — a zero-click exfil bypass of the remote-image block. Now rejected before the root-relative allowance. - sanitizeTerminalOutput left a dangling `ESC [` / `ESC [params` at end of a chunk, which the next streamed write could complete into a CSI across the boundary. Now strips a trailing incomplete escape introducer too. Tests added for both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…log (#117) The 0.2.0 entry only covered the protocol/packages train (#100-#116) and missed ten PRs that also ship in this release: the untrusted-content sanitization and cross-tenant memory fixes (#91, #93 — now under a proper Security heading), the performance run (#94-#99), and the model catalog work (#78, #79). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…#122) Follow-up to #91, which wired sanitizeTerminalOutput into the TUI and web renderers but deferred the legacy `codeoid attach` readline client. That client wrote model/tool content straight to the TTY — assistant content, streaming deltas, identity names, tool names/descriptions — without stripping terminal-control escapes, so an embedded OSC 52 / cursor / DCS sequence reached the terminal (the same vector #91 closed elsewhere). The per-message rendering was a closure inside attachSession, unreachable from a unit test. Extract it into a pure, exported renderStreamMessage (carrying the streaming/approval bookkeeping in an explicit state object) that sanitizes every untrusted field via sanitizeTerminalOutput. Output is byte-for-byte identical to the previous inline rendering; the attach loop now just writes what the function returns. Closes #92 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
What
Two security fixes for untrusted-content → dangerous-sink paths surfaced in a code audit. Both close a channel where model/tool output could reach a sink unsanitized.
1. Web: allowlist markdown link/image URLs (XSS)
SolidMarkdownis invoked with onlyremarkPlugins, and solid-markdown's default istransformLinkUri: null— so hrefs from untrusted assistant/tool output render unsanitized. A prompt-injected[x](javascript:…)is one click from running script in the app origin, which can read the persisted ZeroID key fromlocalStorageand drive the authenticated socket (session.approve, …). Remoteimages are a zero-click exfiltration channel.web/src/lib/sanitize-url.ts:safeLinkUri(scheme allowlist — http/https/mailto/tel + relative; blocksjavascript:/data:/vbscript:/file:, and control-char scheme obfuscation likejava\tscript:) andsafeImageUri(same, plus drops remote http/https image src to stop zero-click exfil; allows relative +data:image/).MarkdownBlock<SolidMarkdown>inMessageRow.tsx.2. Terminal: strip control sequences from untrusted output before the TTY (ANSI/OSC injection)
Model/tool content is written to the terminal scrollback without stripping escapes, so content containing
ESC]52;…(OSC 52) could rewrite the user's clipboard, and cursor/screen/DCS sequences could corrupt the terminal.sanitizeTerminalOutput()insrc/tui/ansi/codes.ts: removes OSC (except OSC-8), DCS/APC/PM/SOS, non-SGR CSI (cursor/screen/mode), otherESCsequences, and unsafe C0 controls — while preserving the SGR color/style and OSC-8 file-link hyperlinks the renderer itself emits. (This is the security counterpart to the existingstripAnsi, which is for width math and misses cursor CSI / C0.)ScrollbackWriter.#writeAtomic— the single chokepoint through which every banner, finalized render, and streamed delta reaches the terminal.Tests
src/tests/sanitize-terminal.test.ts(bun, 10 tests): OSC 52 / title / cursor-CSI / DCS / C0 stripped; SGR + OSC-8 preserved;ScrollbackWriterneutralizes an OSC 52 smuggled in a streamed delta.web/src/lib/sanitize-url.test.ts(vitest, 9 tests): scheme allowlist, obfuscation defeat, remote-image block, data:image allow.bun test(742),tsc,biome; webtsc,eslint,vitest(151).Scope / follow-up
This wires the primary terminal renderer (the Ink
ScrollbackWriter) and the web UI. The legacy readline clientsrc/terminal/client.ts(used bycodeoid attach) writes model content to stdout on the same class of path and needs the same treatment — deferred to a follow-up because making its inline stream handler testable is a small refactor beyond this change. Tracked separately; the security advisory for the terminal-injection issue notes it as a remaining vector.Related: this addresses the web-XSS and ANSI/OSC items from the audit's private security advisories.
Summary by CodeRabbit
New Features
Bug Fixes