fix(security): approval surfaces show reordering and invisible characters as escapes - #1049
Merged
Merged
Conversation
…ters as escapes Closes #1029, found in #1026's review. Trojan Source (CVE-2021-42574): Unicode bidi overrides and isolates reorder the glyphs a browser draws without changing the bytes a shell runs, so a prompt-injected model can propose a command that READS as harmless and RUNS as something else — the canonical demonstration makes a destructive command appear to sit inside a comment. Zero-width characters hide text outright, and a lone CR scrolls the dangerous half of a line out of a <pre>. Nothing in src/ neutralised any of it, and the permission modal is frequently the only place a command is ever shown, so what it renders IS the user's evidence. One helper, src/shared/text/visibleControls.ts, renders each such code point as a visible marker (⟨U+202E RLO⟩). It ESCAPES rather than strips: the approval must show what will actually run, and deleting the characters would make the modal disagree with the command the provider executes — a second, quieter lie. Its table names what each character does, since "it is in a range" is not a reason a reader can check. Applied to every approval surface: Claude's permission prompt and trust dialog, Codex's approval modal and trust dialog, OpenCode's permission subject and command, and Grok's permission title. Tests: the helper's contract (the canonical attack string, isolates, zero-width, a lone CR versus a real CRLF, and several non-Latin scripts left untouched), plus the REAL OpenCode permission modal driven from its recorded 1.18.30 ask with only the command replaced by the attack shape. The modal test fails without the change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y authorisation surface Codex review of #1049, six findings: 1. High. The hand-picked table was still short by variation selectors, tag characters, Mongolian and Khmer controls, the invisible math operators and the combining grapheme joiner — each of which makes two different commands render identically (review reproduced `./check.sh` and `./check.sh<U+FE0F>` executing different files). The rule is now Unicode's `Default_Ignorable_Code_Point`, the property that MEANS "renders as nothing", with readable names kept for the characters a report will actually contain. 2. High. The persistent grant was still spoofable: OpenCode's `always` patterns and permission name, Codex's clickable options and reason, and Claude's title and option labels all rendered raw. Those fields describe the authorisation itself, independently of the command. 3. Medium. "CRLF is just a line ending" is a Windows text assumption, and this is a command: bash reads `./check.sh\r\n` as a filename ending in CR, and the review executed that file. Every CR is escaped now; LF is untouched. 4. Medium. Native consent dialogs were outside the protection — workflow-source approval (repository-controlled name and identity) and extension install consent (attacker-chosen manifest fields). 5. Medium. Destructive confirmation lists showed model-controlled titles and paths raw, so a reordered title misrepresents WHICH session a bulk close is about to terminate. 6. Medium. The queued-prompt preview and its dialog, which #1029 names explicitly. The escape happens before truncation, so a marker cannot be cut in half. Combining accents stay untouched: they render, and escaping every combining mark would make ordinary prose unreadable. Tests: the widened rule (a variation selector, CGJ, Mongolian, Khmer, an invisible operator, a tag character, a musical control), CRLF, ordinary accents left alone, and the OpenCode `always` scope escaped in the real recorded modal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…till raw Round-2 review. The Unicode table itself checked out against DerivedCoreProperties (15/17/18, 4,174 code points, no gaps), but coverage did not: - OpenCode's WILDCARD grant branch (`always: ['*']`) rendered the permission name raw. That branch describes the broadest grant we offer, so its scope line is the most profitable thing to spoof; the earlier test exercised the pattern branch beside it. - Codex trimmed BEFORE escaping, and `.trim()` removes CR — so `./check.sh\r`, the exact trick this work exists to expose, still rendered as `./check.sh`. - A single-target close confirmation shows no target list, so its summary is the only identity the user sees before authorising a kill. It was raw. - Bulk close rendered project labels and directory paths raw beside the checkboxes that choose what dies. - Grok rendered provider-supplied option labels and approved plan content raw. - The extension consent dialog escaped `source` in its heading and then interpolated it raw into "Install it only if you trust …". Three new tests, each failing first: the wildcard grant scope (recorded 1.18.30 ask, replayed through the real dispatcher), the Codex carriage return, and the single-target close summary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round-3 sweep. The six named gaps were fixed; this closes the thirteen the reviewer's full sweep then found. All the same defect: text an agent or a repository authors, rendered raw in the sentence the user reads before granting something. P1: Root Agent Code Management named its recipient raw — a title like `Trusted auditor<U+200B>` renders as a different, trusted agent immediately before the broadest grant this app offers. P2: Claude's interactive question picker (header, question, option labels and descriptions — display only, the resolver payload and the transcript keep the original bytes); the editor's delete and close-with-unsaved-changes dialogs, where `report.txt<U+200B>` is a different file; the installed-skill install and update review, where a repository supplies descriptions, source paths and file lists; bulk provider switch and the provider-switch picker; rewind. P3: new-agent project choice, project merge, the OpenCode and Grok question bodies (reject-only today), and Claude's resume prompt, whose age and token strings are scraped from its screen between two anchors. A regression test covers the P1. `tsc -b` clean; the pre-existing imageAttachment corpus failure is unrelated (it fails identically on main). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # src/renderer/src/features/workspace/ui/NewAgentInDialog.tsx # src/renderer/src/features/workspace/ui/ProviderSwitchPickerModal.tsx
…ed text Round-4 review, two more in the same panel. A repository supplies its own default branch name, its notices and its warnings, and all three sit in the lines a user reads before approving an install, an update or an Enable: `main<U+200B>` reads as `main`, and a warning about `scripts/check.sh<U+200B>` reads as one about `scripts/check.sh` while the collapsed file list beside it already showed the real name. The installed skill's description and source URL are escaped for the same reason — they are the identity next to Enable, Disable and Remove. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # src/renderer/src/workspace/tile-tree/TileLeaf/QueueStrip.tsx
Round-5 review. The path comes from the provider's own config resolution — CLAUDE_CONFIG_DIR=~/.claude<U+200B> makes a different directory read as ~/.claude — and it identifies the file the buttons beside it reveal and OVERWRITE. Both the installation row and the confirm() text are escaped; every action still uses the target id. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round-6 sweep. Key names and provider names are user-typed and the validator permits invisible characters, so `production` and `production<U+200B>` coexist and read identically — in the list and in the confirmation that destroys a provider's whole key set. Skill deployment paths sit beside the controls that reveal, remove and forget them, and CLAUDE_CONFIG_DIR can make a different directory read as ~/.claude. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round 7 stopped spot-checking and enumerated every place the app asks a user to authorise, approve, confirm, delete, install, grant or overwrite — renderer dialogs, strips, pickers, action rows, and every main-process dialog call site. Ten were still raw: - the explorer's own two-click Delete, which for a clean file IS the confirmation — the escaped dialog only appears when a dirty buffer is at stake; - the editor conflict strip's Overwrite / Reload / Recreate, identified only by the adjacent tab; - AI Workspace clear and delete, where the names are model-authored; - extension Reload / Update / Remove rows, where a tier-0 install never reaches the native consent dialog at all; - remote device Revoke, where the client chooses its own name; - the generated skill and conventions previews read immediately before Save & Enable deploys them to every provider; - the directory picker, because choosing a working directory IS the Codex trust decision (ensureCodexProjectTrust runs on it before spawn); - rewind's prompt list, which is interactive there — a click replaces the live pane, so the escape happens at that call site and the read-only View Prompts surface is untouched; - goal-loop and workflow continuation controls, beside Resume / Raise cap / Cancel; - the local template, theme, tab-order and pin pickers. Conversation rows are escaped per SEGMENT, because the search-match offsets index the raw string and escaping changes its length. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r-0 install Round-8 review. The editor tab's full-path tooltip, the AI Workspace attachment path, provenance and description, the extension manifest version and the prompt template description were each still raw beside their actions. The working-directory input keeps its raw text — it is the user's own, and rewriting it while they type would be its own bug — and gains an escaped read-only echo, rendered ONLY when the path contains something invisible, so a pasted `/tmp/project<U+200B>` cannot reach "open and trust this directory" with its identity shown nowhere. And a tier-0 extension no longer installs in silence. The stance was "the repo name is the trust decision"; the silence contradicted it, because nothing ever rendered that name where the user could compare it — the row that shows the source appears only after the code is installed. A FIRST install now always asks, with a prompt that says the extension requests no capabilities. A reload or update re-runs a source already recorded in the ledger and stays silent, so extension development keeps its loop. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e install box Round-9 review. `extensions:install` treats its argument as a repo string the user just typed, so it always shows the consent dialog — that prompt is the only place a pasted `owner/repo` gets rendered with its invisible characters escaped before that repo's code runs. The Update button routed through it, which meant every update of a tier-0 extension prompted again, and a prompt raised on every rebuild is a prompt people click through. `extensions:update-github` is the counterpart of `extensions:update-local`: the renderer names an extension ID, main reads the recorded `owner/repo` out of its own ledger, and the first-install reason to prompt does not apply. A manifest requesting capabilities still prompts, on every install of any kind, because the grant binds to the bytes being published. `install()` in the settings row loses its `target` parameter with the last caller that needed it. Tests drive the registered handlers over the real install pipeline (stubbed network, real tar, real containment checks, real ledger, real consent gate) and count the dialogs it actually raised. Both halves of the gate are pinned by mutation: routing the update as a first install fails the silence assertion, and dropping `|| firstInstall` fails the typed-install assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Juliusolsson05
added a commit
that referenced
this pull request
Sep 20, 2026
…entory Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1029, found in #1026's review.
The attack
Trojan Source (CVE-2021-42574). Unicode bidi overrides and isolates reorder the glyphs a browser draws without changing the bytes a shell runs. A prompt-injected model can therefore propose a command that reads as harmless and executes as something else — the canonical demonstration makes a destructive command appear to sit inside a comment:
Zero-width characters hide text outright, and a lone
\rscrolls the dangerous half of a line out of a<pre>. Nothing insrc/neutralised any of it, and the permission modal is frequently the only place a command is ever shown — so what it renders is the user's evidence.The fix
One helper,
src/shared/text/visibleControls.ts, renders each such code point as a visible marker:⟨U+202E RLO⟩.It escapes rather than strips, deliberately: the approval has to show what will actually run, and deleting the characters would make the modal disagree with the command the provider executes — a second, quieter lie. The table names what each character does, because "it is in a range" is not a reason a future reader can check.
Applied to every approval surface:
Tests
EventDispatcher, with only the command replaced by the attack shape. It asserts the override renders as⟨U+202E RLO⟩and that the raw override never reaches the DOM.Fail-first: the modal test fails with the view's call removed.
npx tsc -bis clean; 89 provider suites pass (the one failure,imageAttachment, is the known local env failure that also fails onorigin/main).🤖 Generated with Claude Code