diff --git a/src/main/extensions/install.ts b/src/main/extensions/install.ts index d6dc26251..ef1150c20 100644 --- a/src/main/extensions/install.ts +++ b/src/main/extensions/install.ts @@ -610,18 +610,32 @@ async function finalizeInstall( bundleDir: string, provenance: { origin: 'github' | 'local'; repo: string; ref: string; sha256: string }, promptConsent?: ConsentPrompt, + firstInstall = false, ): Promise { - // Consent gate. If the extension requests capabilities beyond Tier 0, the user - // must approve them BEFORE the bundle moves into place — declining aborts the - // install, so nothing is left behind. A Tier-0-only extension installs with no - // prompt, matching the "repo name is the trust decision" stance. + // Consent gate. The user must approve BEFORE the bundle moves into place — + // declining aborts the install, so nothing is left behind. + // + // Two reasons to prompt, and Tier 0 needs the second one (#1049 re-review): + // + // 1. The manifest asks for capabilities. Always prompted, always was. + // 2. This is a FIRST install — the user just typed a repo or picked a + // folder. Tier 0 used to install in silence here, on the stance that + // "the repo name is the trust decision"; the stance is right and the + // silence contradicted it, because nothing ever rendered that name where + // the user could compare it. `owner/repo` copied from a page can carry + // invisible characters, and the extension row that shows the source + // appears only after the code is installed. + // + // A RELOAD or UPDATE re-runs a source already recorded in the ledger, so it + // stays silent for Tier 0: the choice was made, and prompting on every + // rebuild would make extension development miserable for no new information. const permissions = manifest.permissions ?? [] - if (permissions.length > 0) { + if (permissions.length > 0 || firstInstall) { const approved = promptConsent ? await promptConsent(manifest) : false if (!approved) { - throw new InstallError( - `Installation of ${manifest.name} was declined — its requested capabilities were not granted.`, - ) + throw new InstallError(permissions.length > 0 + ? `Installation of ${manifest.name} was declined — its requested capabilities were not granted.` + : `Installation of ${manifest.name} was declined.`) } } @@ -733,6 +747,9 @@ export async function installExtension( repoInput: string, promptConsent?: ConsentPrompt, options?: InstallOptions, + /** True when the user just typed this repo; false for an Update that + * re-runs the one already recorded. See finalizeInstall. */ + firstInstall = false, ): Promise { const repo = normalizeRepo(repoInput) // Credential first, before any network: the disabled path must call NOTHING @@ -765,6 +782,7 @@ export async function installExtension( staging, { origin: 'github', repo, ref: source.ref, sha256 }, promptConsent, + firstInstall, ) } finally { // .catch: `force` only suppresses ENOENT. A staging tree containing a @@ -790,6 +808,9 @@ export async function installExtension( export async function installExtensionFromPath( sourceDir: string, promptConsent?: ConsentPrompt, + /** True when the user just picked this folder; false when reinstalling the + * path already recorded in the ledger. See finalizeInstall. */ + firstInstall = false, ): Promise { let sourceReal: string try { @@ -866,6 +887,7 @@ export async function installExtensionFromPath( staging, { origin: 'local', repo: sourceReal, ref: 'local', sha256 }, promptConsent, + firstInstall, ) } finally { // .catch: `force` only suppresses ENOENT. A staging tree containing a diff --git a/src/main/ipc/extensions.ts b/src/main/ipc/extensions.ts index a2dad042e..637fc256c 100644 --- a/src/main/ipc/extensions.ts +++ b/src/main/ipc/extensions.ts @@ -17,6 +17,7 @@ import type { ExtensionInstallResult, ExtensionListEntry, } from '@shared/types/extensions.js' +import { withVisibleControls } from '@shared/text/visibleControls.js' // The capability-consent dialog, shared by both install paths (GitHub + local // folder). A blocking, OS-native dialog on purpose: granting an extension @@ -55,6 +56,30 @@ function consentPromptFor(evt: IpcMainInvokeEvent, source: string): ConsentPromp return async manifest => { const win = BrowserWindow.fromWebContents(evt.sender) const permissions = manifest.permissions ?? [] + // A manifest that asks for NOTHING still gets a dialog (#1049 re-review). + // Tier 0 used to install in silence, so nothing ever showed the user which + // folder or repository they were about to run code from — and the + // extension row that does show it appears only afterwards. The wording + // drops the capability paragraph, because there is nothing to grant; the + // decision is the source. + if (permissions.length === 0) { + const plain = { + type: 'question' as const, + buttons: ['Cancel', 'Install'], + defaultId: 0, + cancelId: 0, + title: 'Install extension', + message: `Install ${withVisibleControls(manifest.id)} from ${withVisibleControls(source)}?`, + detail: + `"${withVisibleControls(manifest.name)}" requests no capabilities: it cannot read or change ` + + `project files and has no network access.\n\n` + + `Install it only if you trust ${withVisibleControls(source)}.`, + } + const plainResult = win + ? await dialog.showMessageBox(win, plain) + : await dialog.showMessageBox(plain) + return plainResult.response === 1 + } const detail = permissions.map(cap => ` • ${CAPABILITY_DISCLOSURE[cap]}`).join('\n') const canWrite = permissions.includes('fs.write') @@ -73,12 +98,16 @@ function consentPromptFor(evt: IpcMainInvokeEvent, source: string): ConsentPromp // one thing the dialog did not show. `manifest.name` is attacker-chosen and // only length-bounded, so it is presented as a claim about an identity // (`id`), never as the identity itself. - message: `Install ${manifest.id} from ${source}?`, + // Every interpolated field here is attacker-chosen (#1049 review). + message: `Install ${withVisibleControls(manifest.id)} from ${withVisibleControls(source)}?`, detail: - `"${manifest.name}" wants these capabilities:\n\n${detail}\n\n` + + `"${withVisibleControls(manifest.name)}" wants these capabilities:\n\n${withVisibleControls(detail)}\n\n` + `${canWrite ? 'It can change project files.' : 'It cannot change project files.'} ` + `It has no network access. ` + - `Install it only if you trust ${source}.`, + // The same value, twice, and the second one was raw: a source string + // with a bidi override could therefore spoof the sentence that carries + // the whole trust decision (#1049 re-review). + `Install it only if you trust ${withVisibleControls(source)}.`, } const result = win ? await dialog.showMessageBox(win, options) @@ -162,9 +191,11 @@ export function registerExtensionsIpc(): void { // credential upgrade without knowing it exists. async (evt, repo: string, useGithubCliAuth?: boolean): Promise => { try { + // firstInstall: `owner/repo` typed (or pasted) just now. A pasted one + // can carry invisible characters, and this dialog is where they show. const record = await installExtension(repo, consentPromptFor(evt, repo.trim()), { githubCliAuth: useGithubCliAuth !== false, - }) + }, true) return { ok: true, entry: { ...record, present: true } } } catch (error) { return { ok: false, error: error instanceof Error ? error.message : String(error) } @@ -189,7 +220,9 @@ export function registerExtensionsIpc(): void { const dir = picked.filePaths[0] if (picked.canceled || !dir) return { ok: false, error: 'No folder selected.' } try { - const record = await installExtensionFromPath(dir, consentPromptFor(evt, dir)) + // firstInstall: the user picked this folder just now, and nothing has + // shown them its name in a form that reveals invisible characters. + const record = await installExtensionFromPath(dir, consentPromptFor(evt, dir), true) return { ok: true, entry: { ...record, present: true } } } catch (error) { return { ok: false, error: error instanceof Error ? error.message : String(error) } @@ -228,6 +261,43 @@ export function registerExtensionsIpc(): void { } }) + // Re-install a GITHUB extension from the `owner/repo` already recorded in its + // ledger row — the counterpart of update-local, and for the same reason. + // + // WHY this exists instead of the Update button re-calling `extensions:install` + // with `entry.repo` (#1049 round 9): that handler hardcodes `firstInstall=true`, + // because everything reaching it IS a first install — a string the user just + // typed or pasted into the box, which is exactly when an invisible-character + // repo name has to be shown before code runs. Routing Update through it made + // every Tier-0 update prompt again, which is the noise the firstInstall split + // was introduced to avoid: the source was chosen once, approved once, and is + // now read back from OUR ledger, not from the renderer. + // + // The renderer names an id, never a repo, so this cannot be turned into + // "install any repository on the renderer's say-so". Everything else is + // unchanged: normalizeRepo, download, tree/entry containment, and an + // unconditional consent prompt for any manifest that requests capabilities. + ipcMain.handle( + 'extensions:update-github', + async (evt, id: string, useGithubCliAuth?: boolean): Promise => { + if (!isValidExtensionId(id)) return { ok: false, error: 'Unknown extension.' } + const installed = await listInstalledExtensions() + const entry = installed.find(candidate => candidate.manifest.id === id) + if (!entry) return { ok: false, error: 'Extension is no longer installed.' } + if (entry.origin !== 'github') { + return { ok: false, error: 'This extension was loaded from a folder; use Reload.' } + } + try { + const record = await installExtension(entry.repo, consentPromptFor(evt, entry.repo), { + githubCliAuth: useGithubCliAuth !== false, + }) + return { ok: true, entry: { ...record, present: true } } + } catch (error) { + return { ok: false, error: error instanceof Error ? error.message : String(error) } + } + }, + ) + ipcMain.handle('extensions:remove', async (_evt, id: string): Promise => { await removeExtension(id) }) diff --git a/src/main/ipc/extensionsUpdate.system.test.ts b/src/main/ipc/extensionsUpdate.system.test.ts new file mode 100644 index 000000000..aacf43026 --- /dev/null +++ b/src/main/ipc/extensionsUpdate.system.test.ts @@ -0,0 +1,226 @@ +import { execFile } from 'child_process' +import { mkdir, mkdtemp, readFile, rm, writeFile } from 'fs/promises' +import { tmpdir } from 'os' +import { join } from 'path' +import { promisify } from 'util' +import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +// --------------------------------------------------------------------------- +// SYSTEM tier: the registered `extensions:*` handlers, the REAL install +// pipeline (network stub → real tar → real containment checks → real ledger) +// and the REAL consent gate. Nothing about the decision under test is mocked: +// the dialog is recorded rather than faked away, so "did the user get asked?" +// is answered by counting actual prompts the pipeline raised. +// +// WHAT THIS PINS (#1049 round 9). The consent gate has two reasons to prompt: +// a manifest that requests capabilities, and a FIRST install — a repo string +// the user just typed, which is the only moment its name can be shown (with +// invisible characters escaped) before that repo's code runs. An Update re-runs +// a source already recorded in OUR ledger, so for a tier-0 manifest it must +// stay silent; otherwise every rebuild-and-update cycle asks again and the +// prompt becomes the thing users click through without reading. +// +// Routing Update back through `extensions:install` broke that split, because +// that handler treats its argument as unseen by construction. These tests fail +// against that shape: the tier-0 update prompts. +// --------------------------------------------------------------------------- + +const run = promisify(execFile) + +const root = await mkdtemp(join(tmpdir(), 'agent-code-ext-update-')) +const stateRoot = join(root, 'state') + +vi.mock('@main/storage/paths.js', () => ({ + STATE_DIR: stateRoot, + EXTENSIONS_DIR: join(stateRoot, 'extensions'), + EXTENSIONS_LOCKFILE: join(stateRoot, 'extensions.json'), + EXTENSION_STATE_DIR: join(stateRoot, 'extension-state'), +})) + +// Every registered handler, keyed by channel — the fake ipcMain is a registry, +// not a behaviour stub, so the tests invoke the same function Electron would. +const handlers = new Map Promise>() +// Every dialog the pipeline actually raised. `message` carries the source being +// approved, which is what distinguishes the two prompts. +const prompts: string[] = [] +let dialogResponse = 1 // the affirmative button in both consent dialogs + +vi.mock('electron', () => ({ + ipcMain: { + handle: (channel: string, handler: (evt: unknown, ...args: unknown[]) => Promise) => { + handlers.set(channel, handler) + }, + }, + // No window: consentPromptFor falls back to the window-less dialog call, + // which is the same decision path minus the parent. + BrowserWindow: { fromWebContents: () => null, getAllWindows: () => [] }, + dialog: { + showMessageBox: async (options: { message?: string }) => { + prompts.push(String(options?.message ?? '')) + return { response: dialogResponse } + }, + showOpenDialog: async () => ({ canceled: true, filePaths: [] }), + }, + protocol: { handle: () => {} }, + net: { fetch: async (url: string) => new Response(await readFile(new URL(url))) }, +})) + +const { registerExtensionsIpc } = await import('./extensions.js') +const { listInstalledExtensions, writeLedger, readLedger } = await import('@main/extensions/ledger.js') + +registerExtensionsIpc() + +function invoke(channel: string, ...args: unknown[]): Promise { + const handler = handlers.get(channel) + if (!handler) throw new Error(`No handler registered for ${channel}`) + return handler({ sender: {} }, ...args) +} + +type InstallResult = { ok: true; entry: { manifest: { id: string; version: string } } } | { ok: false; error: string } + +/** + * Build a real `.tar.gz` shaped like GitHub's: one wrapper directory that + * `--strip-components=1` removes. Written with the system tar so the archive + * the installer extracts is a real archive, not a fixture of one. + * + * WHY the manifest is written here rather than copied from + * `testing/fixtures/extensions/`: the only published manifest captured there + * (Timer 0.3.1) requests `sessions.observe`, and the case under test is the + * TIER-0 one — an empty `permissions` array. That is manifest schema, not + * recorded provider behaviour, so composing it is honest; the permissioned + * half below uses the real fixture. + */ +async function buildTarball(name: string, manifest: Record): Promise { + const stage = join(root, 'sources', name) + const wrapper = join(stage, `${name}-abcdef0`) + await rm(stage, { recursive: true, force: true }) + await mkdir(join(wrapper, 'dist'), { recursive: true }) + await writeFile(join(wrapper, 'agent-code.extension.json'), JSON.stringify(manifest)) + await writeFile(join(wrapper, 'dist/index.js'), 'export function activate() {}') + const archive = join(stage, 'bundle.tar.gz') + await run('tar', ['-czf', archive, '-C', stage, `${name}-abcdef0`]) + return readFile(archive) +} + +/** Serve the release probe and the tarball; everything else 404s. */ +function stubNetwork(tarball: Buffer, tag = 'v1.0.0'): void { + vi.stubGlobal('fetch', vi.fn(async (url: string | URL) => { + const target = String(url) + if (target.includes('/releases/latest')) { + return new Response( + JSON.stringify({ tag_name: tag, tarball_url: 'https://fixture.invalid/bundle.tar.gz' }), + { status: 200 }, + ) + } + if (target.includes('fixture.invalid')) { + return new Response(new Uint8Array(tarball), { status: 200 }) + } + return new Response('{}', { status: 404 }) + })) +} + +const TIER0 = { + id: 'quiet', name: 'Quiet', description: 'Tier-0 fixture', version: '1.0.0', + apiVersion: 1, entry: 'dist/index.js', permissions: [], + contributes: { views: [{ id: 'quiet.main', title: 'Main', mount: 'panel' }] }, +} + +beforeEach(() => { + prompts.length = 0 + dialogResponse = 1 +}) + +afterEach(async () => { + vi.unstubAllGlobals() + await rm(stateRoot, { recursive: true, force: true }) +}) + +afterAll(async () => { + await rm(root, { recursive: true, force: true }) +}) + +describe('extension update routing (#1049)', () => { + it('prompts on a typed tier-0 install and stays silent on the update that follows', async () => { + stubNetwork(await buildTarball('quiet', TIER0)) + + // 1. The user types `owner/repo`. Tier 0 or not, this asks: it is the only + // place the repo name is rendered before its code runs. + const installed = (await invoke('extensions:install', 'owner/quiet', false)) as InstallResult + expect(installed.ok).toBe(true) + expect(prompts).toHaveLength(1) + expect(prompts[0]).toContain('owner/quiet') + + // 2. Update. Same extension, same recorded repo, nothing new to disclose. + prompts.length = 0 + stubNetwork(await buildTarball('quiet', { ...TIER0, version: '1.1.0' }), 'v1.1.0') + const updated = (await invoke('extensions:update-github', 'quiet', false)) as InstallResult + expect(updated.ok).toBe(true) + if (updated.ok) expect(updated.entry.manifest.version).toBe('1.1.0') + expect(prompts).toEqual([]) + + // The ledger holds one row, at the new generation, still pointing at the + // repo the user approved — the update re-ran the recorded source. + const rows = await listInstalledExtensions() + expect(rows).toHaveLength(1) + expect(rows[0].repo).toBe('owner/quiet') + expect(rows[0].manifest.version).toBe('1.1.0') + }) + + it('still prompts on update when the manifest requests capabilities', async () => { + // The real published Timer manifest — it asks for `sessions.observe`, so + // this half is pinned against something that actually shipped. + const timer = JSON.parse( + await readFile( + join(import.meta.dirname, '../../../testing/fixtures/extensions/timer-0.3.1.agent-code.extension.json'), + 'utf8', + ), + ) as Record + expect((timer.permissions as string[]).length).toBeGreaterThan(0) + + stubNetwork(await buildTarball('timer', timer)) + expect(((await invoke('extensions:install', 'owner/timer', false)) as InstallResult).ok).toBe(true) + + prompts.length = 0 + stubNetwork(await buildTarball('timer', timer)) + expect(((await invoke('extensions:update-github', timer.id as string, false)) as InstallResult).ok).toBe(true) + // A capability grant is re-asked on every install of any kind: the grant is + // bound to the bytes being published, and this call is publishing new ones. + expect(prompts).toHaveLength(1) + }) + + it('declining the update leaves the installed generation untouched', async () => { + stubNetwork(await buildTarball('quiet', TIER0)) + await invoke('extensions:install', 'owner/quiet', false) + const before = await readLedger() + + // A tier-0 update never prompts, so make the manifest request a capability + // to reach the dialog, then refuse it. + prompts.length = 0 + dialogResponse = 0 + stubNetwork(await buildTarball('quiet', { ...TIER0, version: '2.0.0', permissions: ['sessions.observe'] }), 'v2.0.0') + const declined = (await invoke('extensions:update-github', 'quiet', false)) as InstallResult + expect(declined.ok).toBe(false) + expect(prompts).toHaveLength(1) + expect(await readLedger()).toEqual(before) + }) + + it('refuses an id that is not installed, or was loaded from a folder', async () => { + expect(await invoke('extensions:update-github', 'nothing-here', false)).toEqual({ + ok: false, + error: 'Extension is no longer installed.', + }) + + // A local row's `repo` is an absolute folder path; handing it to the GitHub + // installer would fail normalizeRepo. The handlers are a matched pair and + // each refuses the other's origin by name, so the UI can say which button. + stubNetwork(await buildTarball('quiet', TIER0)) + await invoke('extensions:install', 'owner/quiet', false) + const rows = await readLedger() + await writeLedger(rows.map(row => ({ ...row, origin: 'local' as const, repo: '/tmp/quiet' }))) + + expect(await invoke('extensions:update-github', 'quiet', false)).toEqual({ + ok: false, + error: 'This extension was loaded from a folder; use Reload.', + }) + }) +}) diff --git a/src/main/workflows/createWorkflowService.ts b/src/main/workflows/createWorkflowService.ts index a9f182f2b..d7845cc8e 100644 --- a/src/main/workflows/createWorkflowService.ts +++ b/src/main/workflows/createWorkflowService.ts @@ -13,6 +13,7 @@ import { ElectronWorkflowWorkerLauncher } from '@main/workflows/ElectronWorkflow import { resolveClaudeAgentType } from '@main/workflows/ClaudeAgentTypeResolver.js' import { prepareGitWorkflowWorktree } from '@main/workflows/GitWorkflowWorktree.js' import { WorkflowSourceApprovalStore } from '@main/workflows/WorkflowSourceApprovalStore.js' +import { withVisibleControls } from '@shared/text/visibleControls.js' export async function createWorkflowService(options: { isCodexCliUpdateReserved?: () => boolean @@ -69,9 +70,13 @@ export async function createWorkflowService(options: { const result = await dialog.showMessageBox({ type: 'warning', title: 'Approve workflow source', - message: `Allow ${source.workflowName} to run agents?`, + // Repository-controlled, so it is escaped (#1049 review): the hash + // binds the grant to exact bytes, but the IDENTITY beside it is what + // the user reads, and a reordered one can describe a different source + // than the bytes being approved. + message: `Allow ${withVisibleControls(source.workflowName)} to run agents?`, detail: [ - `Source: ${source.canonicalIdentity}`, + `Source: ${withVisibleControls(source.canonicalIdentity)}`, `SHA-256: ${source.sourceHash}`, '', 'This approval applies only to these exact bytes. Editing the workflow will ask again.', diff --git a/src/preload/api/extensions.ts b/src/preload/api/extensions.ts index 80c852c27..42c7e4fe8 100644 --- a/src/preload/api/extensions.ts +++ b/src/preload/api/extensions.ts @@ -94,6 +94,13 @@ export const extensionsApi = { extensionsUpdateLocal: (id: string): Promise => ipcRenderer.invoke('extensions:update-local', id), + // Re-install a GitHub extension from the `owner/repo` recorded at install + // time. Same shape as update-local — id in, main reads the source — so that + // Update re-runs an approved choice instead of arriving as a fresh repo + // string, which is what makes the first-install consent prompt skippable here. + extensionsUpdateGithub: (id: string, useGithubCliAuth?: boolean): Promise => + ipcRenderer.invoke('extensions:update-github', id, useGithubCliAuth), + extensionsRemove: (id: string): Promise => ipcRenderer.invoke('extensions:remove', id), // Reads the set of capabilities a user granted an extension, for the frame broker diff --git a/src/providers/claude/renderer/PermissionPromptModal.tsx b/src/providers/claude/renderer/PermissionPromptModal.tsx index 2b40d8943..5ba378bde 100644 --- a/src/providers/claude/renderer/PermissionPromptModal.tsx +++ b/src/providers/claude/renderer/PermissionPromptModal.tsx @@ -5,6 +5,7 @@ import { DialogDescription, DialogTitle, } from '@renderer/components/ui/dialog' +import { withVisibleControls } from '@shared/text/visibleControls' type PermissionPromptState = { title?: string @@ -46,7 +47,7 @@ export function PermissionPromptModal({ state, onSend }: Props) {
!
- {title} + {withVisibleControls(title)} Review the requested tool and choose whether Claude may continue. @@ -62,7 +63,7 @@ export function PermissionPromptModal({ state, onSend }: Props) {
{state.command && ( 
-              {state.command}
+              {withVisibleControls(state.command)}
)} {state.options && state.options.length > 0 && ( @@ -72,7 +73,7 @@ export function PermissionPromptModal({ state, onSend }: Props) { key={`${option.key}:${option.label}`} className={index === state.selectedIndex ? 'text-ink' : undefined} > - {option.key}. {option.label} + {option.key}. {withVisibleControls(option.label)}
))}
diff --git a/src/providers/claude/renderer/ResumePromptModal.tsx b/src/providers/claude/renderer/ResumePromptModal.tsx index 1dae85dc8..42f3505b7 100644 --- a/src/providers/claude/renderer/ResumePromptModal.tsx +++ b/src/providers/claude/renderer/ResumePromptModal.tsx @@ -1,4 +1,5 @@ import { useCallback, useEffect, useRef, useState } from 'react' +import { withVisibleControls } from '@shared/text/visibleControls' type Props = { prompt: { @@ -104,7 +105,11 @@ export function ResumePromptModal({ prompt, onSend, interactionActive }: Props) outline-none ">
- This session is {prompt.sessionAgeText ?? 'older'} old and {prompt.tokenCountText ?? 'many'} tokens. + {/* Scraped from Claude's own screen between two anchors, so the + parser accepts whatever sits there — including controls that + reorder the size and age this decision is made on (#1049 + re-review). */} + This session is {withVisibleControls(prompt.sessionAgeText ?? 'older')} old and {withVisibleControls(prompt.tokenCountText ?? 'many')} tokens.
diff --git a/src/providers/claude/renderer/TrustDialogModal.tsx b/src/providers/claude/renderer/TrustDialogModal.tsx index 68cba4e89..19caa6201 100644 --- a/src/providers/claude/renderer/TrustDialogModal.tsx +++ b/src/providers/claude/renderer/TrustDialogModal.tsx @@ -5,6 +5,7 @@ import { DialogDescription, DialogTitle, } from '@renderer/components/ui/dialog' +import { withVisibleControls } from '@shared/text/visibleControls' // WHY the modal takes intent callbacks instead of an onSend(bytes) writer: // this component used to write a bare '\r' for accept, assuming Claude Code @@ -48,7 +49,7 @@ export function TrustDialogModal({ state, onAccept, onDecline }: Props) {

Claude Code is about to access:

{state.workspace && ( 
-              {state.workspace}
+              {withVisibleControls(state.workspace)}
)}

diff --git a/src/providers/claude/renderer/components/ask-user-question/AskUserQuestionRow.tsx b/src/providers/claude/renderer/components/ask-user-question/AskUserQuestionRow.tsx index 6a4de5218..72daebd50 100644 --- a/src/providers/claude/renderer/components/ask-user-question/AskUserQuestionRow.tsx +++ b/src/providers/claude/renderer/components/ask-user-question/AskUserQuestionRow.tsx @@ -22,6 +22,7 @@ import { useAnswerSubmissionStore, useAnsweredViaMessageStore, } from '@providers/claude/renderer/components/ask-user-question/answeredViaMessageStore' +import { withVisibleControls } from '@shared/text/visibleControls' // Native in-feed renderer for Claude Code's `AskUserQuestion` tool. // @@ -408,12 +409,18 @@ export function AskUserQuestionRow({

{q.header ? ( - {q.header} + {/* DISPLAY only — the resolver still sends the option's own + bytes, and the transcript keeps the original text. What + is escaped is what the user READS before clicking, which + an agent authors: `Run ./check.sh` and + `Run ./check.sh` are different commands that render + identically (#1049 re-review). */} + {withVisibleControls(q.header)} ) : null} {q.question ? (
- {q.question} + {withVisibleControls(q.question)}
) : null}
@@ -453,9 +460,9 @@ export function AskUserQuestionRow({ {q.multiSelect ? (isSelected ? '[x]' : '[ ]') : isSelected ? '(*)' : `${oi + 1}.`} - {opt.label} + {withVisibleControls(opt.label)} {opt.description ? ( - {opt.description} + {withVisibleControls(opt.description)} ) : null} diff --git a/src/providers/codex/renderer/CodexApprovalModal.tsx b/src/providers/codex/renderer/CodexApprovalModal.tsx index 94afb6350..3aa37d020 100644 --- a/src/providers/codex/renderer/CodexApprovalModal.tsx +++ b/src/providers/codex/renderer/CodexApprovalModal.tsx @@ -1,4 +1,5 @@ import { useCallback, useEffect, useRef, useState } from 'react' +import { withVisibleControls } from '@shared/text/visibleControls' // CodexApprovalPane — inline approval prompt rendered inside the pane, // matching how Codex's TUI draws it in the bottom pane. @@ -89,7 +90,13 @@ export function CodexApprovalModal({ approval, onSend, interactionActive }: Prop if (!approval) return null - const command = approval.command.join(' ').trim() + // Escape BEFORE trimming, never after. `./check.sh\r` is a filename whose + // last byte is CR — the exact trick #1049 exists to expose — and `.trim()` + // removes CR, so trimming first deleted the evidence and left two different + // commands rendering identically (#1049 re-review). After escaping, the CR + // is the visible text `⟨U+000D CR⟩`, which trim leaves alone, and ordinary + // surrounding whitespace is still tidied. + const command = withVisibleControls(approval.command.join(' ')).trim() return (
" line */} {approval.reason && (
- Reason: {approval.reason} + Reason: {withVisibleControls(approval.reason)}
)} @@ -146,7 +153,7 @@ export function CodexApprovalModal({ approval, onSend, interactionActive }: Prop {command && (
$ - {command} + {withVisibleControls(command)}
)} @@ -164,7 +171,7 @@ export function CodexApprovalModal({ approval, onSend, interactionActive }: Prop ›{' '} - {i + 1}. {opt} + {i + 1}. {withVisibleControls(opt)} ({DEFAULT_HINTS[i] ?? ''})
))} diff --git a/src/providers/codex/renderer/conditions/CodexTrustDialogModal.tsx b/src/providers/codex/renderer/conditions/CodexTrustDialogModal.tsx index f40e6a023..8a2af49d9 100644 --- a/src/providers/codex/renderer/conditions/CodexTrustDialogModal.tsx +++ b/src/providers/codex/renderer/conditions/CodexTrustDialogModal.tsx @@ -6,6 +6,7 @@ import { DialogDescription, DialogTitle, } from '@renderer/components/ui/dialog' +import { withVisibleControls } from '@shared/text/visibleControls' type Props = { state: { workspace?: string } | null @@ -60,7 +61,7 @@ export function CodexTrustDialogModal({ state, actions, dispatch }: Props) {

Codex is about to work in:

{state.workspace && ( 
-              {state.workspace}
+              {withVisibleControls(state.workspace)}
)}

diff --git a/src/providers/grok/renderer/conditions/views.tsx b/src/providers/grok/renderer/conditions/views.tsx index e0f18cd7d..1aaa780f2 100644 --- a/src/providers/grok/renderer/conditions/views.tsx +++ b/src/providers/grok/renderer/conditions/views.tsx @@ -31,6 +31,7 @@ import { DialogDescription, DialogTitle, } from '@renderer/components/ui/dialog' +import { withVisibleControls } from '@shared/text/visibleControls' // Per-provider kind→state binding: eraseRegistry checks the registry literal // against this, so filing a view under the wrong kind is a compile error. @@ -72,7 +73,9 @@ function ConditionButtons({ }} variant={reject ? 'outline' : 'default'} > - {action.label} + {/* Provider-supplied, and it is the text the user reads to decide + WHICH grant they are giving (#1049 re-review). */} + {withVisibleControls(action.label)} ) })} @@ -134,7 +137,7 @@ export const grokPermissionView = defineView< Grok is requesting permission {state.title ? ( <> - {' '}for {state.title} + {' '}for {withVisibleControls(state.title)} ) : null} . @@ -157,7 +160,7 @@ export const grokQuestionView = defineView< {state.text ? ( 

-            {state.text}
+            {withVisibleControls(state.text)}
) : (

Grok is waiting for a response.

@@ -183,7 +186,10 @@ export const grokPlanApprovalView = defineView< {state.planContent ? ( 
-            {state.planContent}
+            {/* The plan is the thing being approved; a reordering override in
+                it misrepresents what the user is authorising (#1049
+                re-review). */}
+            {withVisibleControls(state.planContent)}
) : (

Grok is waiting for plan approval.

diff --git a/src/providers/opencode/renderer/conditions/opencodePermissionView.renderer.test.tsx b/src/providers/opencode/renderer/conditions/opencodePermissionView.renderer.test.tsx index 4079fea5f..589055a30 100644 --- a/src/providers/opencode/renderer/conditions/opencodePermissionView.renderer.test.tsx +++ b/src/providers/opencode/renderer/conditions/opencodePermissionView.renderer.test.tsx @@ -81,6 +81,64 @@ describe('opencode permission modal on a recorded 1.18.30 ask', () => { expect(subject.className).toMatch(/max-h-/) }) + it('renders a bidi override as a visible escape, so the command cannot lie about itself (#1029)', () => { + // Trojan Source, CVE-2021-42574: U+202E reorders the glyphs a browser + // draws without changing the bytes the shell runs, so a prompt-injected + // model can make a destructive command read as a harmless one. This modal + // is frequently the only place the command is shown, so what it renders + // IS the user's evidence. DERIVED from the recording: only the command + // changes, to the canonical attack shape. + const rec = recording() + const spoofed = 'rm -rf ~/work \u202E# this is fine\u202C' + for (const { event } of rec.sse) { + if (event.type === 'permission.asked') event.properties = { ...event.properties, metadata: { command: spoofed } } + } + mount(permissionStateFrom(rec)) + const rendered = screen.getByText((_, element) => element?.tagName === 'PRE' && (element.textContent ?? '').includes('rm -rf')) + expect(rendered.textContent).toContain('⟨U+202E RLO⟩') + expect(rendered.textContent).toContain('⟨U+202C PDF⟩') + // The override itself must not survive into the DOM, or the browser + // reorders the line exactly as the attack intends. + expect(rendered.textContent).not.toContain('\u202E') + }) + + it('escapes the persistent grant\'s scope too, since that is what "Allow always" authorises (#1049 review)', () => { + // The command is only half of the decision: "Allow always covers " + // describes what the grant will keep allowing, for this agent and its + // subagents. A reordered pattern misdescribes that scope. + const rec = recording() + for (const { event } of rec.sse) { + if (event.type === 'permission.asked') { + event.properties = { ...event.properties, metadata: { command: 'ls -1' }, pattern: ['ls \u202E rm -rf *'] } + } + } + mount(permissionStateFrom(rec)) + const always = screen.getByText(/Allow always covers/) + expect(always.textContent).not.toContain('\u202E') + }) + + it('escapes the WILDCARD grant scope, the broadest one we offer (#1049 re-review)', () => { + // `always: ['*']` renders a different branch — "every + // request" — and that branch was left unescaped while the pattern branch + // beside it was fixed. It is also the worst one to lose: the wildcard is + // the broadest grant in the modal, so the permission name is the only + // thing telling the user what they are signing away. + const rec = recording() + for (const { event } of rec.sse) { + if (event.type === 'permission.asked') { + // The recorded payload's own field names: `permission` and `always` + // sit beside `metadata`, and the dispatcher folds the whole payload + // into the state's metadata. `always: ['*']` is the wildcard shape + // OpenCode really sends for edit/write/MCP asks. + event.properties = { ...event.properties, permission: 'bash \u202E harmless', always: ['*'] } + } + } + mount(permissionStateFrom(rec)) + const always = screen.getByText(/Allow always covers/) + expect(always.textContent).not.toContain('\u202E') + expect(always.textContent).toContain('U+202E') + }) + it('shows the command behind a default-permission external_directory ask, not just the directory', () => { // #1026 review: OpenCode's DEFAULT rules allow bash and ask only for // external_directory, so for most users this is THE shell-command diff --git a/src/providers/opencode/renderer/conditions/views.tsx b/src/providers/opencode/renderer/conditions/views.tsx index 84e19aeeb..f3c6b7c80 100644 --- a/src/providers/opencode/renderer/conditions/views.tsx +++ b/src/providers/opencode/renderer/conditions/views.tsx @@ -30,6 +30,7 @@ import { DialogDescription, DialogTitle, } from '@renderer/components/ui/dialog' +import { withVisibleControls } from '@shared/text/visibleControls' // Per-provider kind→state binding (see CodexStateByKind for the rationale // — eraseRegistry checks the registry literal against this, so filing a @@ -167,7 +168,7 @@ export const opencodePermissionView = defineView< `python3 -c` legible; the height cap keeps the buttons on screen. */} 
-              {state.title}
+              {withVisibleControls(state.title)}
) : ( @@ -187,7 +188,7 @@ export const opencodePermissionView = defineView< <>

Command:

-                {command}
+                {withVisibleControls(command)}
) @@ -214,16 +215,20 @@ export const opencodePermissionView = defineView< {always.includes('*') ? ( <> Allow always covers{' '} - every {permission ? {permission} : 'such'} request{' '} + {/* The wildcard branch names the SCOPE the grant will cover, + so it is the one a spoofed permission name would misstate + most profitably — `*` is the broadest grant we offer + (#1049 re-review found this branch unescaped). */} + every {permission ? {withVisibleControls(permission)} : 'such'} request{' '} from this agent and its subagents until this agent restarts. ) : ( <> - Allow always covers {permission ? <>{permission}{' '} : null} + Allow always covers {permission ? <>{withVisibleControls(permission)}{' '} : null} {always.map((pattern, index) => ( {index > 0 ? ', ' : ''} - {pattern} + {withVisibleControls(pattern)} ))}{' '} for this agent and its subagents until this agent restarts. @@ -254,7 +259,10 @@ export const opencodeQuestionView = defineView< // height, and Escape and outside-click are disabled, so a long // question must never push the only button (Reject) off-screen. 
-            {state.text}
+            {/* Reject-only today, so no affirmative grant hangs off it — but
+                it is still a provider-authored question the user answers, and
+                the escape costs nothing (#1049 re-review). */}
+            {withVisibleControls(state.text)}
) : (

OpenCode is waiting for a response.

diff --git a/src/providers/shared/renderer/conditions/inlineConditionOwnership.renderer.test.tsx b/src/providers/shared/renderer/conditions/inlineConditionOwnership.renderer.test.tsx index d1241b931..f3c566ab2 100644 --- a/src/providers/shared/renderer/conditions/inlineConditionOwnership.renderer.test.tsx +++ b/src/providers/shared/renderer/conditions/inlineConditionOwnership.renderer.test.tsx @@ -107,3 +107,23 @@ describe('pane-local condition keyboard ownership', () => { expect(request).toHaveBeenCalledOnce() }) }) + +describe('what an approval modal SHOWS is part of the decision (#1049)', () => { + it('keeps a trailing carriage return visible instead of trimming it away', async () => { + // `./check.sh\r` is a filename whose last byte is CR: a shell runs a + // DIFFERENT file than `./check.sh`, and the two render identically. The + // first fix escaped the command but trimmed it first, and `.trim()` + // removes CR — so the modal went on showing the safe-looking name for the + // command Codex would actually run (#1049 re-review). + const { CodexApprovalModal } = await import('@providers/codex/renderer/CodexApprovalModal') + render( + undefined)} + interactionActive={false} + />, + ) + const strip = screen.getByRole('group', { name: 'Codex command approval options' }) + expect(strip.textContent).toContain('U+000D CR') + }) +}) diff --git a/src/renderer/src/apps/host/installedExtensionsState.renderer.test.tsx b/src/renderer/src/apps/host/installedExtensionsState.renderer.test.tsx index d3e0964a2..0e2e9887f 100644 --- a/src/renderer/src/apps/host/installedExtensionsState.renderer.test.tsx +++ b/src/renderer/src/apps/host/installedExtensionsState.renderer.test.tsx @@ -77,6 +77,36 @@ describe('one ordered extension catalog per window', () => { expect(useAppStore.getState().installedExtensions).toHaveLength(1) }) + // Update must name an ID, never a repo string. The handler behind + // `extensionsInstall` treats its argument as a repo the user just typed and + // therefore always shows the consent dialog — routing Update through it made + // every Tier-0 update prompt again (#1049 round 9). Both origins now hand + // main an id and let it read the recorded source out of its own ledger. + it('updates through the ledger-backed path for each origin, never through install', async () => { + const github: ExtensionListEntry = { ...entry(), origin: 'github', repo: 'owner/timer' } + window.api.extensionsList = vi.fn().mockResolvedValue([github]) + window.api.extensionsInstall = vi.fn() + window.api.extensionsUpdateGithub = vi.fn().mockResolvedValue({ ok: true, entry: github }) + const githubView = render() + await screen.findByText('Timer') + fireEvent.click(screen.getByRole('button', { name: 'Update' })) + // The gh-credential setting rides along (see the install call); the ID is + // what this pins — main resolves the repo itself. + await waitFor(() => { expect(window.api.extensionsUpdateGithub).toHaveBeenCalled() }) + expect(vi.mocked(window.api.extensionsUpdateGithub).mock.calls[0]![0]).toBe('timer') + expect(window.api.extensionsInstall).not.toHaveBeenCalled() + + githubView.unmount() + + const local = entry() + window.api.extensionsList = vi.fn().mockResolvedValue([local]) + window.api.extensionsUpdateLocal = vi.fn().mockResolvedValue({ ok: true, entry: local }) + render() + fireEvent.click(await screen.findByRole('button', { name: 'Reload' })) + await waitFor(() => { expect(window.api.extensionsUpdateLocal).toHaveBeenCalledWith('timer') }) + expect(window.api.extensionsInstall).not.toHaveBeenCalled() + }) + it('a delayed removal acknowledgement cannot discard a later reinstall', () => { acceptExtensionPublication([entry('new-generation')]) forgetRemovedExtension(entry('old-generation')) diff --git a/src/renderer/src/apps/ui/AppsSettingsRow.tsx b/src/renderer/src/apps/ui/AppsSettingsRow.tsx index 6c6b350af..fc2857a13 100644 --- a/src/renderer/src/apps/ui/AppsSettingsRow.tsx +++ b/src/renderer/src/apps/ui/AppsSettingsRow.tsx @@ -4,6 +4,7 @@ import { useAppStore } from '@renderer/app-state/hooks' import { forgetRemovedExtension, refreshInstalledExtensions } from '@renderer/apps/host/installedExtensionsState' import type { ExtensionListEntry } from '@shared/types/extensions' +import { withVisibleControls } from '@shared/text/visibleControls' /** * Settings → Extensions. Install from a GitHub repository, list what is installed, @@ -35,14 +36,21 @@ export function AppsSettingsRow() { void refresh() }, [refresh]) - // `target` is explicit rather than always read from `repo` so the Update button - // can install a specific entry's repo. The previous version did - // `setRepo(entry.repo); void install()`, but `setRepo` is async and `install` - // closed over the OLD `repo`, so Update installed the empty/last-typed value — - // the no-op bug. Passing the target directly removes the closure dependency. + // FIRST install only — the repo comes from the text box, i.e. a string the + // user just typed or pasted. That is the whole distinction main's + // `extensions:install` handler acts on: it treats its argument as unseen and + // always shows the consent dialog, so the name gets rendered (with invisible + // characters made visible) before any of that repo's code runs. Update no + // longer routes through here for exactly that reason — it re-runs a source + // already in the ledger, via extensions:update-github. + // + // It used to take a `target` so Update could pass `entry.repo`; an earlier + // version did `setRepo(entry.repo); void install()` and, because `setRepo` is + // async while `install` closed over the OLD `repo`, installed the + // empty/last-typed value. Neither shape is needed now that Update names an id. const install = useCallback( - async (target?: string) => { - const repoTarget = (target ?? repo).trim() + async () => { + const repoTarget = repo.trim() if (!repoTarget || busy) return setBusy(true) @@ -57,7 +65,7 @@ export function AppsSettingsRow() { useAppStore.getState().settings.extensionsGithubCliAuth, ) if (result.ok) { - if (target === undefined) setRepo('') + setRepo('') setNotice(`Installed ${result.entry.manifest.name} ${result.entry.manifest.version}`) } else { setError(result.error) @@ -80,32 +88,45 @@ export function AppsSettingsRow() { const update = useCallback( async (entry: ExtensionListEntry) => { if (busy) return - // Dispatch on how it was installed. A local extension's `repo` is an absolute - // folder path, so feeding it to the GitHub installer failed normalizeRepo every - // single time — and "rebuild, click Update" is the entire dev loop this install - // path exists for, so Update was broken for exactly the users who need it most. - if (entry.origin === 'local') { - setBusy(true) - setError(null) - setNotice(null) - try { - const result = await window.api.extensionsUpdateLocal(entry.manifest.id) - if (result.ok) { - setNotice(`Reloaded ${result.entry.manifest.name} ${result.entry.manifest.version}`) - } else { - setError(result.error) - } - } catch (updateError) { - setError(updateError instanceof Error ? updateError.message : String(updateError)) - } finally { - setBusy(false) - await refresh() + setBusy(true) + setError(null) + setNotice(null) + try { + // Dispatch on how it was installed, but BOTH branches hand main only the + // extension id and let it read the recorded source back out of its own + // ledger. + // + // The local branch has to: a local extension's `repo` is an absolute + // folder path, so feeding it to the GitHub installer failed normalizeRepo + // every single time — and "rebuild, click Update" is the entire dev loop + // that install path exists for. + // + // The GitHub branch used to call install(entry.repo) instead, which works + // but arrives at a handler that treats everything as a first install and + // therefore prompts for consent on every update, even for a Tier-0 + // manifest (#1049 round 9). Going through the ledger keeps the split the + // consent gate is built on: a typed repo prompts, a recorded one does not. + const result = + entry.origin === 'local' + ? await window.api.extensionsUpdateLocal(entry.manifest.id) + : await window.api.extensionsUpdateGithub( + entry.manifest.id, + useAppStore.getState().settings.extensionsGithubCliAuth, + ) + if (result.ok) { + const verb = entry.origin === 'local' ? 'Reloaded' : 'Installed' + setNotice(`${verb} ${result.entry.manifest.name} ${result.entry.manifest.version}`) + } else { + setError(result.error) } - return + } catch (updateError) { + setError(updateError instanceof Error ? updateError.message : String(updateError)) + } finally { + setBusy(false) + await refresh() } - await install(entry.repo) }, - [busy, install, refresh], + [busy, refresh], ) const remove = useCallback( @@ -219,8 +240,13 @@ export function AppsSettingsRow() { >
- {entry.manifest.name} - {entry.manifest.version} + {/* Manifest text and the source path are repository-authored + and only length-bounded, and a tier-0 install or reload + never reaches the native consent dialog — so this row IS + the approval surface for Reload, Update and Remove + (#1049 re-review). */} + {withVisibleControls(entry.manifest.name)} + {withVisibleControls(entry.manifest.version)} {/* A ledger row whose bundle is gone. Shown rather than filtered: the fix is reinstalling from the recorded repo, and hiding it would leave the user wondering where the extension went. */} @@ -235,11 +261,13 @@ export function AppsSettingsRow() { · failed to start ) : null}
-
{entry.manifest.description}
+
{withVisibleControls(entry.manifest.description)}
{/* A local install's `repo` is a folder path, and "…@ local" read as a broken ref. Say which kind of install it is instead. */} - {entry.origin === 'local' ? `local folder · ${entry.repo}` : `${entry.repo} @ ${entry.ref}`} + {entry.origin === 'local' + ? `local folder · ${withVisibleControls(entry.repo)}` + : `${withVisibleControls(entry.repo)} @ ${withVisibleControls(entry.ref)}`}
{failures.find(failure => failure.id === entry.manifest.id) ? (
diff --git a/src/renderer/src/features/ai-workspace/ui/AiWorkspaceFileList.tsx b/src/renderer/src/features/ai-workspace/ui/AiWorkspaceFileList.tsx index 432e930b8..21ad7b4d2 100644 --- a/src/renderer/src/features/ai-workspace/ui/AiWorkspaceFileList.tsx +++ b/src/renderer/src/features/ai-workspace/ui/AiWorkspaceFileList.tsx @@ -3,6 +3,7 @@ import { useState } from 'react' import type { AiWorkspaceFileEntry } from '@mcp/shared/aiWorkspaceTypes' import { FileIcon } from '@renderer/features/editor/lib/fileIcon' import { basename } from '@renderer/features/editor/lib/path' +import { withVisibleControls } from '@shared/text/visibleControls' type AiWorkspaceFileListProps = { title: string @@ -51,7 +52,9 @@ export function AiWorkspaceFileList({ return (