From dd95a29e62355eda800d81275295aa2d136d19ec Mon Sep 17 00:00:00 2001 From: Neil Date: Sun, 18 Oct 2026 22:26:05 -0700 Subject: [PATCH 01/11] fix(ssh): a cancelled or revoked host's browser command stops on the desktop orcad now tells a cancel-capable desktop when the CLI gave up on a forwarded browser command, and a running host command on the desktop checks revocation before every side-effecting step (each helper CDP message and each direct input dispatch). --- .../browser/agent-browser-bridge-execution.ts | 3 + ...ent-browser-bridge-host-revocation.test.ts | 154 ++++++++++++++++++ ...ent-browser-bridge-interaction-commands.ts | 4 + .../agent-browser-bridge-mouse-commands.ts | 4 + .../agent-browser-bridge-pointer-commands.ts | 3 + .../browser/agent-browser-bridge-queue.ts | 50 ++++-- .../browser/cdp-ws-proxy-command-gate.test.ts | 80 +++++++++ src/main/browser/cdp-ws-proxy.ts | 16 ++ .../desktop-browser-bridge-connection.ts | 23 ++- .../desktop-browser-bridge-manager.test.ts | 40 +++++ src/main/ipc/desktop-browser-bridge-wiring.ts | 4 +- .../desktop-browser-bridge-registry.test.ts | 70 +++++++- .../desktop-browser-bridge-registry.ts | 17 ++ .../remote-cli-browser-scope.ts | 14 ++ .../rpc/methods/desktop-browser-bridge.ts | 9 +- src/shared/desktop-browser-bridge-protocol.ts | 9 +- 16 files changed, 482 insertions(+), 18 deletions(-) create mode 100644 src/main/browser/agent-browser-bridge-host-revocation.test.ts create mode 100644 src/main/browser/cdp-ws-proxy-command-gate.test.ts diff --git a/src/main/browser/agent-browser-bridge-execution.ts b/src/main/browser/agent-browser-bridge-execution.ts index 31f5a191ede4..4b59a5d7284b 100644 --- a/src/main/browser/agent-browser-bridge-execution.ts +++ b/src/main/browser/agent-browser-bridge-execution.ts @@ -19,6 +19,7 @@ import { type SessionState, type ResolvedBrowserCommandTarget } from './agent-browser-bridge-types' +import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' export abstract class AgentBrowserBridgeExecution extends AgentBrowserBridgeTabs { protected abstract destroySession( @@ -83,6 +84,8 @@ export abstract class AgentBrowserBridgeExecution extends AgentBrowserBridgeTabs throw this.createPageUnavailableError(sessionName) } + // Why: a command can spawn several helpers in turn; none may start after a host's revocation. + assertRemoteCliBrowserAccess(session.webContentsId) this.reinitializeIfDaemonIdledOut(sessionName, session) session.lastCommandAt = Date.now() diff --git a/src/main/browser/agent-browser-bridge-host-revocation.test.ts b/src/main/browser/agent-browser-bridge-host-revocation.test.ts new file mode 100644 index 000000000000..61daec83638d --- /dev/null +++ b/src/main/browser/agent-browser-bridge-host-revocation.test.ts @@ -0,0 +1,154 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' + +const { execFileMock, webContentsFromIdMock, existsSyncMock, readFileSyncMock, stdinWrites } = + vi.hoisted(() => ({ + execFileMock: vi.fn(), + webContentsFromIdMock: vi.fn(), + existsSyncMock: vi.fn(() => false), + readFileSyncMock: vi.fn(() => Buffer.from('')), + stdinWrites: [] as string[] + })) + +vi.mock('child_process', () => ({ execFile: execFileMock })) +vi.mock('fs', () => ({ + existsSync: existsSyncMock, + readFileSync: readFileSyncMock, + accessSync: vi.fn(), + chmodSync: vi.fn(), + constants: { X_OK: 1 } +})) +vi.mock('os', () => ({ platform: () => 'darwin', arch: () => 'arm64' })) +vi.mock('electron', () => ({ + app: { getPath: vi.fn(() => '/app'), getAppPath: vi.fn(() => '/project'), isPackaged: false }, + webContents: { fromId: webContentsFromIdMock } +})) +const { CdpWsProxyMock } = vi.hoisted(() => { + const instances: unknown[] = [] + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const MockClass = vi.fn().mockImplementation(function (this: any, _wc: unknown) { + this._wc = _wc + this.start = vi.fn(async () => 'ws://127.0.0.1:9222') + this.stop = vi.fn(async () => {}) + this.getPort = vi.fn(() => 9222) + this.setCommandGate = vi.fn() + instances.push(this) + }) + return { CdpWsProxyMock: Object.assign(MockClass, { instances }) } +}) +vi.mock('./cdp-ws-proxy', () => ({ CdpWsProxy: CdpWsProxyMock })) + +import { AgentBrowserBridge } from './agent-browser-bridge' +import { runInRemoteCliBrowserScope } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' +import { + createFakeAgentBrowserChild, + mockBrowserManager, + mockWebContents, + overrideBridgeWebContentsLookup, + resetAgentBrowserBridgeMocks, + type ExecFileCallback +} from './agent-browser-bridge-test-harness' + +overrideBridgeWebContentsLookup(AgentBrowserBridge.prototype, webContentsFromIdMock) + +const WORKSPACE = 'repo-host::/srv/app' + +function dispatchedMouseEvents(wc: ReturnType): unknown[] { + return wc.debugger.sendCommand.mock.calls.filter((call) => call[0] === 'Input.dispatchMouseEvent') +} + +describe('AgentBrowserBridge under a host revocation', () => { + let bridge: AgentBrowserBridge + + beforeEach(() => { + resetAgentBrowserBridgeMocks({ + webContentsFromIdMock, + existsSyncMock, + readFileSyncMock, + stdinWrites, + cdpWsProxyInstances: CdpWsProxyMock.instances + }) + bridge = new AgentBrowserBridge(mockBrowserManager()) + bridge.setActiveTab(100) + }) + + it('never presses the mouse once the host is revoked during a running click', async () => { + const wc = mockWebContents(100) + let answerHitTest = (): void => {} + wc.debugger.sendCommand.mockImplementation(async (method: string) => { + if (method === 'Runtime.evaluate') { + await new Promise((resolve) => { + answerHitTest = resolve + }) + return { result: { value: { x: 10, y: 20, adjusted: false, handled: false } } } + } + return {} + }) + webContentsFromIdMock.mockReturnValue(wc) + const revoked = new AbortController() + + const click = runInRemoteCliBrowserScope( + WORKSPACE, + () => bridge.mouseClick(10, 20, 'left', undefined, 'tab-1', 18), + revoked.signal + ) + await vi.waitFor(() => + expect(wc.debugger.sendCommand).toHaveBeenCalledWith('Runtime.evaluate', expect.anything()) + ) + revoked.abort() + answerHitTest() + + await expect(click).rejects.toMatchObject({ code: 'desktop_browser_bridge_unavailable' }) + expect(dispatchedMouseEvents(wc)).toHaveLength(0) + }) + + it('closes the page proxy to a running helper click when the host is revoked', async () => { + const helperCallbacks: ExecFileCallback[] = [] + execFileMock.mockImplementation( + (_bin: string, args: string[], _opts: unknown, cb: ExecFileCallback) => { + if (args.includes('click')) { + helperCallbacks.push(cb) + } else { + cb(null, JSON.stringify({ success: true, data: null }), '') + } + return createFakeAgentBrowserChild({ stdin: { on: vi.fn(), end: vi.fn() } }) + } + ) + const revoked = new AbortController() + + const click = runInRemoteCliBrowserScope( + WORKSPACE, + () => bridge.click('@e1', undefined, 'tab-1'), + revoked.signal + ) + await vi.waitFor(() => expect(helperCallbacks).toHaveLength(1)) + const proxy = CdpWsProxyMock.instances[0] as { setCommandGate: ReturnType } + const gate = proxy.setCommandGate.mock.calls.at(-1)?.[0] as (() => void) | null + expect(gate).toBeTypeOf('function') + expect(() => gate!()).not.toThrow() + + revoked.abort() + // The helper's next CDP step reaches the proxy and is refused there. + expect(() => gate!()).toThrow('stopped sharing') + helperCallbacks[0]!( + null, + JSON.stringify({ success: false, error: 'The Orca desktop stopped sharing its browser' }), + '' + ) + + await expect(click).rejects.toMatchObject({ code: 'desktop_browser_bridge_unavailable' }) + expect(proxy.setCommandGate).toHaveBeenLastCalledWith(null) + }) + + it('leaves a desktop-local command ungated', async () => { + execFileMock.mockImplementation( + (_bin: string, _args: string[], _opts: unknown, cb: ExecFileCallback) => { + cb(null, JSON.stringify({ success: true, data: { clicked: '@e1' } }), '') + return createFakeAgentBrowserChild({ stdin: { on: vi.fn(), end: vi.fn() } }) + } + ) + + await expect(bridge.click('@e1', undefined, 'tab-1')).resolves.toEqual({ clicked: '@e1' }) + const proxy = CdpWsProxyMock.instances[0] as { setCommandGate: ReturnType } + expect(proxy.setCommandGate).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/browser/agent-browser-bridge-interaction-commands.ts b/src/main/browser/agent-browser-bridge-interaction-commands.ts index 688ab013cfed..a617aa0e49fd 100644 --- a/src/main/browser/agent-browser-bridge-interaction-commands.ts +++ b/src/main/browser/agent-browser-bridge-interaction-commands.ts @@ -17,6 +17,7 @@ import { parseCdpKeyEvent, imeFallbackKeyEvent } from './cdp-keyboard-us-layout' import { dispatchPageKeyEvent, type GuestCdpSender } from './guest-direct-key-input' import { sendGuestCdpCommand } from './guest-cdp-command' import { AgentBrowserBridgeCaptureCommands } from './agent-browser-bridge-capture-commands' +import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' export abstract class AgentBrowserBridgeInteractionCommands extends AgentBrowserBridgeCaptureCommands { async hover( @@ -208,6 +209,8 @@ export abstract class AgentBrowserBridgeInteractionCommands extends AgentBrowser sendGuestCdpCommand(wc, method, params, sessionId) const guardFocus = (): Promise<() => void> => this.browserManager.guardUserFocusForAgentInput(target.webContentsId) + // Why each step: a host's access can be revoked while an earlier await is pending. + assertRemoteCliBrowserAccess(target.webContentsId) await dispatchPageKeyEvent( wc, { @@ -220,6 +223,7 @@ export abstract class AgentBrowserBridgeInteractionCommands extends AgentBrowser send, guardFocus ) + assertRemoteCliBrowserAccess(target.webContentsId) await dispatchPageKeyEvent( wc, { diff --git a/src/main/browser/agent-browser-bridge-mouse-commands.ts b/src/main/browser/agent-browser-bridge-mouse-commands.ts index 1aaef42a16cb..2fd0a456fe86 100644 --- a/src/main/browser/agent-browser-bridge-mouse-commands.ts +++ b/src/main/browser/agent-browser-bridge-mouse-commands.ts @@ -8,6 +8,7 @@ import { } from './agent-browser-bridge-mouse' import { acquireElectronDebugger } from './electron-debugger-lease' import { AgentBrowserBridgePointerCommands } from './agent-browser-bridge-pointer-commands' +import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' export abstract class AgentBrowserBridgeMouseCommands extends AgentBrowserBridgePointerCommands { // ── Mouse commands ── @@ -47,6 +48,8 @@ export abstract class AgentBrowserBridgeMouseCommands extends AgentBrowserBridge // Why: land the tap as one atomic op — separate move/down/up CLI calls visibly hover and can miss small controls. // Why: mobile-emulated BrowserViews can ignore CDP mouse clicks, so the runtime may already have activated DOM controls. if (!point.handled) { + // Why each step: a host's access can be revoked while an earlier await is pending. + assertRemoteCliBrowserAccess(target.webContentsId) await wc.debugger.sendCommand('Input.dispatchMouseEvent', { type: 'mousePressed', x: point.x, @@ -56,6 +59,7 @@ export abstract class AgentBrowserBridgeMouseCommands extends AgentBrowserBridge modifiers: cdpModifiers, clickCount: 1 }) + assertRemoteCliBrowserAccess(target.webContentsId) await wc.debugger.sendCommand('Input.dispatchMouseEvent', { type: 'mouseReleased', x: point.x, diff --git a/src/main/browser/agent-browser-bridge-pointer-commands.ts b/src/main/browser/agent-browser-bridge-pointer-commands.ts index d79e0a919a0c..276cf443559c 100644 --- a/src/main/browser/agent-browser-bridge-pointer-commands.ts +++ b/src/main/browser/agent-browser-bridge-pointer-commands.ts @@ -13,6 +13,7 @@ import { acquireElectronDebugger } from './electron-debugger-lease' import { AgentBrowserBridgeInputCommands } from './agent-browser-bridge-input-commands' import { isEmbeddedGuest } from './guest-direct-key-input' import { enablePageFocusEmulation } from './agent-input-focus-guard' +import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' type CdpPointerEventParams = { type: 'mouseMoved' | 'mousePressed' | 'mouseReleased' | 'mouseWheel' @@ -64,6 +65,8 @@ export abstract class AgentBrowserBridgePointerCommands extends AgentBrowserBrid if (press) { releaseFocusGuard = await this.focusPageForPress(wc, target.webContentsId) } + // Why after the focus await: a host's access can be revoked while it is pending. + assertRemoteCliBrowserAccess(target.webContentsId) await wc.debugger.sendCommand('Input.dispatchMouseEvent', params) return result } catch (error) { diff --git a/src/main/browser/agent-browser-bridge-queue.ts b/src/main/browser/agent-browser-bridge-queue.ts index bf9826119691..e22eadaf09e8 100644 --- a/src/main/browser/agent-browser-bridge-queue.ts +++ b/src/main/browser/agent-browser-bridge-queue.ts @@ -6,7 +6,10 @@ import type { EnqueueTargetedCommandOptions, ResolvedBrowserCommandTarget } from './agent-browser-bridge-types' -import { captureRemoteCliBrowserRevocation } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' +import { + captureRemoteCliBrowserRevocation, + captureRemoteCliBrowserScope +} from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' export abstract class AgentBrowserBridgeQueue extends AgentBrowserBridgeShutdown { // Why: route tab switch through the command queue so it can't race in-flight commands targeting the old tab. @@ -61,6 +64,7 @@ export abstract class AgentBrowserBridgeQueue extends AgentBrowserBridgeShutdown ): Promise { this.assertCommandAdmission() const assertNotRevoked = captureRemoteCliBrowserRevocation() + const bridged = captureRemoteCliBrowserScope() assertNotRevoked() const { browserPageId: pageId } = this.resolveCommandTarget( worktreeId, @@ -78,15 +82,20 @@ export abstract class AgentBrowserBridgeQueue extends AgentBrowserBridgeShutdown queue.push({ execute: () => { assertNotRevoked() - return this.executeQueuedCommand( - worktreeId, - pageId, - (session, target) => { - assertNotRevoked(target.webContentsId) - return execute(session, target) - }, - options - ) + const run = (): Promise => + this.executeQueuedCommand( + worktreeId, + pageId, + (session, target) => { + const gate = (): void => assertNotRevoked(target.webContentsId) + gate() + return bridged + ? this.runRevocableCommand(session, gate, () => execute(session, target)) + : execute(session, target) + }, + options + ) + return bridged ? bridged(run) : run() }, resolve: resolve as (value: unknown) => void, reject @@ -111,6 +120,27 @@ export abstract class AgentBrowserBridgeQueue extends AgentBrowserBridgeShutdown return execute(sessionName, target) } + /** + * Runs a host's command so a revocation mid-command stops it: the page's CDP proxy refuses every + * later step the helper sends, and a step that failed that way reports the revocation. + */ + private async runRevocableCommand( + sessionName: string, + gate: () => void, + execute: () => Promise + ): Promise { + const proxy = this.sessions.get(sessionName)?.proxy + proxy?.setCommandGate(gate) + try { + return await execute() + } catch (error) { + gate() + throw error + } finally { + proxy?.setCommandGate(null) + } + } + protected async processQueue(sessionName: string): Promise { if (this.processingQueues.has(sessionName)) { return diff --git a/src/main/browser/cdp-ws-proxy-command-gate.test.ts b/src/main/browser/cdp-ws-proxy-command-gate.test.ts new file mode 100644 index 000000000000..e56e44b4b143 --- /dev/null +++ b/src/main/browser/cdp-ws-proxy-command-gate.test.ts @@ -0,0 +1,80 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { CdpWsProxy } from './cdp-ws-proxy' +import { + connect, + createMockWebContents, + getSendCommandMethods, + sendAndReceive, + type MockWebContents +} from './cdp-ws-proxy-test-harness' + +vi.mock('electron', () => ({ + webContents: { fromId: vi.fn() } +})) + +describe('CdpWsProxy command gate', () => { + let mock: MockWebContents + let proxy: CdpWsProxy + let endpoint: string + + beforeEach(async () => { + mock = createMockWebContents() + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the mock implements every WebContents member the proxy calls. + proxy = new CdpWsProxy(mock.webContents as never, () => () => {}) + endpoint = await proxy.start() + }) + + afterEach(async () => { + await proxy.stop() + }) + + it('refuses every later helper step once the gate closes, without sending it to the page', async () => { + let revoked = false + proxy.setCommandGate(() => { + if (revoked) { + throw new Error('The Orca desktop stopped sharing its browser before this command ran.') + } + }) + const client = await connect(endpoint) + const press = { type: 'mousePressed', x: 1, y: 1, button: 'left', clickCount: 1 } + + const allowed = await sendAndReceive(client, { + id: 1, + method: 'Input.dispatchMouseEvent', + params: press + }) + revoked = true + const refused = await sendAndReceive(client, { + id: 2, + method: 'Input.dispatchMouseEvent', + params: { ...press, type: 'mouseReleased' } + }) + + expect(allowed.error).toBeUndefined() + expect(refused).toMatchObject({ id: 2, error: { message: expect.stringContaining('stopped') } }) + expect( + mock.webContents.debugger.sendCommand.mock.calls.filter( + (call) => call[0] === 'Input.dispatchMouseEvent' + ) + ).toHaveLength(1) + client.close() + }) + + it('forwards commands again once the gate is cleared', async () => { + proxy.setCommandGate(() => { + throw new Error('revoked') + }) + proxy.setCommandGate(null) + const client = await connect(endpoint) + + const response = await sendAndReceive(client, { + id: 3, + method: 'Accessibility.getFullAXTree', + params: {} + }) + + expect(response.error).toBeUndefined() + expect(getSendCommandMethods(mock)).toContain('Accessibility.getFullAXTree') + client.close() + }) +}) diff --git a/src/main/browser/cdp-ws-proxy.ts b/src/main/browser/cdp-ws-proxy.ts index b074a61eae3c..675ec49350ed 100644 --- a/src/main/browser/cdp-ws-proxy.ts +++ b/src/main/browser/cdp-ws-proxy.ts @@ -27,6 +27,7 @@ export class CdpWsProxy { private readonly domFocusReplay: CdpDomFocusReplay private readonly pageCapture: CdpPageCaptureCommands private readonly input: CdpPageInputCommands + private commandGate: (() => void) | null = null constructor( private readonly webContents: WebContents, @@ -159,6 +160,11 @@ export class CdpWsProxy { client?.close() } + /** While set, every client command must pass it first; a throw refuses the command unsent. */ + setCommandGate(gate: (() => void) | null): void { + this.commandGate = gate + } + private clearClientState(): void { // Why: session and focus state belongs to one websocket and must not cross client replacement. this.domFocusReplay.clear() @@ -178,6 +184,16 @@ export class CdpWsProxy { } const clientId = msg.id this.responder.recordRequestSessionId(client, clientId, msg) + try { + this.commandGate?.() + } catch (error) { + this.responder.sendError( + clientId, + error instanceof Error ? error.message : String(error), + client + ) + return + } if (this.discovery.handleCommand(client, clientId, msg)) { return diff --git a/src/main/browser/desktop-browser-bridge/desktop-browser-bridge-connection.ts b/src/main/browser/desktop-browser-bridge/desktop-browser-bridge-connection.ts index 391c96216fb5..12de9ee580f1 100644 --- a/src/main/browser/desktop-browser-bridge/desktop-browser-bridge-connection.ts +++ b/src/main/browser/desktop-browser-bridge/desktop-browser-bridge-connection.ts @@ -28,6 +28,7 @@ export class DesktopBrowserBridgeConnection { private attachmentId: string | null = null private closed = false private readonly revoked = new AbortController() + private readonly cancellations = new Map() constructor( private readonly options: { @@ -102,15 +103,29 @@ export class DesktopBrowserBridgeConnection { this.lose(new Error('desktop_browser_bridge_request_before_ready')) return } + if (parsed.data.type === 'cancel') { + this.cancellations.get(parsed.data.requestId)?.abort() + return + } void this.answer(this.attachmentId, parsed.data) } private async answer(attachmentId: string, request: DesktopBrowserBridgeRequest): Promise { // Why: once orcad has stopped waiting, a command still queued here must not run late. - const signal = request.timeoutMs - ? AbortSignal.any([this.revoked.signal, AbortSignal.timeout(request.timeoutMs)]) - : this.revoked.signal - const outcome = await this.options.execute(request, signal) + const cancelled = new AbortController() + this.cancellations.set(request.requestId, cancelled) + const signals = [this.revoked.signal, cancelled.signal] + if (request.timeoutMs) { + signals.push(AbortSignal.timeout(request.timeoutMs)) + } + let outcome: DesktopBrowserBridgeOutcome + try { + outcome = await this.options.execute(request, AbortSignal.any(signals)) + } finally { + if (this.cancellations.get(request.requestId) === cancelled) { + this.cancellations.delete(request.requestId) + } + } const sendRequest = this.subscription?.sendRequest if (this.closed || this.attachmentId !== attachmentId || !sendRequest) { return diff --git a/src/main/browser/desktop-browser-bridge/desktop-browser-bridge-manager.test.ts b/src/main/browser/desktop-browser-bridge/desktop-browser-bridge-manager.test.ts index 2af864680d8d..cac10e444169 100644 --- a/src/main/browser/desktop-browser-bridge/desktop-browser-bridge-manager.test.ts +++ b/src/main/browser/desktop-browser-bridge/desktop-browser-bridge-manager.test.ts @@ -315,4 +315,44 @@ describe('DesktopBrowserBridgeManager', () => { expect(seen!.aborted).toBe(false) await vi.waitFor(() => expect(seen!.aborted).toBe(true)) }) + + it('stops a queued or running command when orcad cancels it, and stays attached', async () => { + const h = harness() + let seen: AbortSignal | null = null + let finish = (): void => {} + h.execute.mockImplementationOnce(async (_candidate, _request, signal) => { + seen = signal + await new Promise((resolve) => { + finish = resolve + }) + return { ok: true, result: {} } + }) + h.manager.reconcile() + await vi.waitFor(() => expect(h.subscriptions).toHaveLength(1)) + const socket = h.subscriptions[0]! + socket.callbacks.onResponse(event({ type: 'ready', attachmentId: 'att-1' })) + socket.callbacks.onResponse( + event({ type: 'request', requestId: 'r', method: 'browser.click', params: {} }) + ) + await vi.waitFor(() => expect(seen).not.toBeNull()) + expect(seen!.aborted).toBe(false) + + socket.callbacks.onResponse(event({ type: 'cancel', requestId: 'r' })) + + expect(seen!.aborted).toBe(true) + expect(h.manager.isAttached('env-a')).toBe(true) + finish() + }) + + it('ignores a cancel for a request it is not running', async () => { + const h = harness() + h.manager.reconcile() + await vi.waitFor(() => expect(h.subscriptions).toHaveLength(1)) + const socket = h.subscriptions[0]! + socket.callbacks.onResponse(event({ type: 'ready', attachmentId: 'att-1' })) + + socket.callbacks.onResponse(event({ type: 'cancel', requestId: 'unknown' })) + + expect(h.manager.isAttached('env-a')).toBe(true) + }) }) diff --git a/src/main/ipc/desktop-browser-bridge-wiring.ts b/src/main/ipc/desktop-browser-bridge-wiring.ts index d7537233422c..e5c10f3a8c1e 100644 --- a/src/main/ipc/desktop-browser-bridge-wiring.ts +++ b/src/main/ipc/desktop-browser-bridge-wiring.ts @@ -5,6 +5,7 @@ import { randomUUID } from 'node:crypto' import { webContents, type BrowserWindow } from 'electron' import { + DESKTOP_BROWSER_BRIDGE_CANCEL_CAPABILITY, DESKTOP_BROWSER_BRIDGE_RUNTIME_CAPABILITY, isDesktopBrowserBridgeMethod } from '../../shared/desktop-browser-bridge-protocol' @@ -135,7 +136,8 @@ export function installDesktopBrowserBridge(options: { { clientCapabilities: [ ...ELECTRON_REMOTE_RUNTIME_CLIENT_CAPABILITIES, - DESKTOP_BROWSER_BRIDGE_RUNTIME_CAPABILITY + DESKTOP_BROWSER_BRIDGE_RUNTIME_CAPABILITY, + DESKTOP_BROWSER_BRIDGE_CANCEL_CAPABILITY ] } ) diff --git a/src/main/runtime/desktop-browser-bridge/desktop-browser-bridge-registry.test.ts b/src/main/runtime/desktop-browser-bridge/desktop-browser-bridge-registry.test.ts index 53cf377a205d..a0e98c890d35 100644 --- a/src/main/runtime/desktop-browser-bridge/desktop-browser-bridge-registry.test.ts +++ b/src/main/runtime/desktop-browser-bridge/desktop-browser-bridge-registry.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from 'vitest' +import { describe, expect, it, vi } from 'vitest' import type { DesktopBrowserBridgeEvent } from '../../../shared/desktop-browser-bridge-protocol' import { DesktopBrowserBridgeRegistry, @@ -31,4 +31,72 @@ describe('DesktopBrowserBridgeRegistry', () => { expect(events[0]).toMatchObject({ type: 'request', timeoutMs: 50_000 }) }) + + it('tells a cancel-capable desktop to drop a command the CLI cancelled', async () => { + const registry = new DesktopBrowserBridgeRegistry() + const events: DesktopBrowserBridgeEvent[] = [] + registry.attach({ + connectionId: 'c', + pairedDeviceId: 'd', + acceptsCancel: true, + emit: (event) => events.push(event) + }) + const cli = new AbortController() + + const pending = registry.forward( + { method: 'browser.click', params: {} }, + { timeoutMs: 60_000, signal: cli.signal } + ) + cli.abort() + + await expect(pending).rejects.toMatchObject({ code: 'browser_aborted' }) + const sent = events[0] + expect(sent?.type).toBe('request') + expect(events[1]).toEqual({ + type: 'cancel', + requestId: sent?.type === 'request' ? sent.requestId : '' + }) + }) + + it('never sends a cancel to an older desktop, which would drop its attachment on it', async () => { + const registry = new DesktopBrowserBridgeRegistry() + const events: DesktopBrowserBridgeEvent[] = [] + registry.attach({ connectionId: 'c', pairedDeviceId: 'd', emit: (event) => events.push(event) }) + const cli = new AbortController() + + const pending = registry.forward( + { method: 'browser.click', params: {} }, + { timeoutMs: 60_000, signal: cli.signal } + ) + cli.abort() + + await expect(pending).rejects.toMatchObject({ code: 'browser_aborted' }) + expect(events.map((event) => event.type)).toEqual(['request']) + }) + + it('cancels on the desktop when orcad stops waiting at its deadline', async () => { + vi.useFakeTimers() + try { + const registry = new DesktopBrowserBridgeRegistry() + const events: DesktopBrowserBridgeEvent[] = [] + registry.attach({ + connectionId: 'c', + pairedDeviceId: 'd', + acceptsCancel: true, + emit: (event) => events.push(event) + }) + const pending = registry.forward( + { method: 'browser.click', params: {} }, + { timeoutMs: 60_000 } + ) + pending.catch(() => {}) + + await vi.advanceTimersByTimeAsync(60_000) + + await expect(pending).rejects.toMatchObject({ code: 'browser_timeout' }) + expect(events.map((event) => event.type)).toEqual(['request', 'cancel']) + } finally { + vi.useRealTimers() + } + }) }) diff --git a/src/main/runtime/desktop-browser-bridge/desktop-browser-bridge-registry.ts b/src/main/runtime/desktop-browser-bridge/desktop-browser-bridge-registry.ts index 0f10f2423cf1..3434827d0d75 100644 --- a/src/main/runtime/desktop-browser-bridge/desktop-browser-bridge-registry.ts +++ b/src/main/runtime/desktop-browser-bridge/desktop-browser-bridge-registry.ts @@ -22,6 +22,8 @@ type Attachment = { attachmentId: string connectionId: string pairedDeviceId: string + /** True when the desktop declared it understands `cancel`; older desktops never receive one. */ + acceptsCancel?: boolean emit: (event: DesktopBrowserBridgeEvent) => void } @@ -75,12 +77,26 @@ export class DesktopBrowserBridgeRegistry { this.pending.delete(requestId) options.signal?.removeEventListener('abort', onAbort) } + let emitted = false + // Why: without this a command still queued on the desktop runs after the CLI gave up on it. + const cancelOnDesktop = (): void => { + if (!emitted || !attachment.acceptsCancel || !this.attachments.includes(attachment)) { + return + } + try { + attachment.emit({ type: 'cancel', requestId }) + } catch { + // The socket is gone, so the desktop's own detach revokes the command. + } + } const onAbort = (): void => { settle() + cancelOnDesktop() reject(new BrowserError('browser_aborted', 'The browser command was cancelled.')) } const timer = setTimeout(() => { settle() + cancelOnDesktop() reject( new BrowserError('browser_timeout', 'The desktop did not answer the browser command.') ) @@ -115,6 +131,7 @@ export class DesktopBrowserBridgeRegistry { ), ...(request.workspaceId ? { workspaceId: request.workspaceId } : {}) }) + emitted = true } catch { this.pending.get(requestId)?.reject(unavailableError()) } diff --git a/src/main/runtime/desktop-browser-bridge/remote-cli-browser-scope.ts b/src/main/runtime/desktop-browser-bridge/remote-cli-browser-scope.ts index 2a1ad031dde1..695c3fdfff88 100644 --- a/src/main/runtime/desktop-browser-bridge/remote-cli-browser-scope.ts +++ b/src/main/runtime/desktop-browser-bridge/remote-cli-browser-scope.ts @@ -55,6 +55,20 @@ export function captureRemoteCliBrowserRevocation(): (webContentsId?: number) => } } +/** + * Captured where a browser command is queued: null outside a bridged command, else a runner that + * puts queued work back inside that scope, so its checks still see a later revocation. + */ +export function captureRemoteCliBrowserScope(): ((run: () => T) => T) | null { + const active = scope.getStore() + return active ? (run) => scope.run(active, run) : null +} + +/** Throws inside a bridged command once it was revoked or `webContentsId` stopped being admitted. */ +export function assertRemoteCliBrowserAccess(webContentsId?: number): void { + captureRemoteCliBrowserRevocation()(webContentsId) +} + /** Null outside a bridged command; inside one, the scoped workspace or a refusal. */ export function resolveRemoteCliBrowserScopeSelector( selector: string diff --git a/src/main/runtime/rpc/methods/desktop-browser-bridge.ts b/src/main/runtime/rpc/methods/desktop-browser-bridge.ts index 0014cc3ef33b..468bab72c5fa 100644 --- a/src/main/runtime/rpc/methods/desktop-browser-bridge.ts +++ b/src/main/runtime/rpc/methods/desktop-browser-bridge.ts @@ -1,4 +1,5 @@ import { + DESKTOP_BROWSER_BRIDGE_CANCEL_CAPABILITY, DESKTOP_BROWSER_BRIDGE_RUNTIME_CAPABILITY, DesktopBrowserBridgeAttachParams, DesktopBrowserBridgeResultParams @@ -35,7 +36,13 @@ export const DESKTOP_BROWSER_BRIDGE_RPC_METHODS = [ handler: async (params, context, emit) => { const caller = requireBridgeCaller(context) const { runtime, signal } = context - const handle = getDesktopBrowserBridgeRegistry(runtime).attach({ ...caller, emit }) + const acceptsCancel = + context.clientCapabilities?.includes(DESKTOP_BROWSER_BRIDGE_CANCEL_CAPABILITY) ?? false + const handle = getDesktopBrowserBridgeRegistry(runtime).attach({ + ...caller, + acceptsCancel, + emit + }) let release = (): void => {} const released = new Promise((resolve) => { release = resolve diff --git a/src/shared/desktop-browser-bridge-protocol.ts b/src/shared/desktop-browser-bridge-protocol.ts index 17425962c6b6..4b739f7d420c 100644 --- a/src/shared/desktop-browser-bridge-protocol.ts +++ b/src/shared/desktop-browser-bridge-protocol.ts @@ -9,6 +9,11 @@ import { z } from 'zod' export const DESKTOP_BROWSER_BRIDGE_RUNTIME_CAPABILITY = 'browser.desktopBridge.v1' +/** + * Declared by a desktop that understands a `cancel` event. orcad never sends one to a desktop + * without it: an older desktop drops its whole attachment on an event type it cannot parse. + */ +export const DESKTOP_BROWSER_BRIDGE_CANCEL_CAPABILITY = 'browser.desktopBridge.cancel.v1' export const DESKTOP_BROWSER_BRIDGE_UNAVAILABLE_CODE = 'desktop_browser_bridge_unavailable' export const DESKTOP_BROWSER_BRIDGE_NOT_FORWARDED_CODE = 'desktop_browser_bridge_not_forwarded' @@ -120,7 +125,9 @@ export type DesktopBrowserBridgeRequest = z.infer From 17eb104c7c1ea49a1a13c0866998e817b3ba8982 Mon Sep 17 00:00:00 2001 From: Neil Date: Sun, 18 Oct 2026 22:39:50 -0700 Subject: [PATCH 02/11] fix(browser): keep local commands out of a host's scope, gate each helper on its proxy --- .../browser/agent-browser-bridge-execution.ts | 27 ++++++- ...ent-browser-bridge-host-revocation.test.ts | 71 ++++++++++++++++++- ...ent-browser-bridge-interaction-commands.ts | 30 ++++++-- .../agent-browser-bridge-mouse-commands.ts | 4 +- .../browser/agent-browser-bridge-queue.ts | 23 ++---- .../agent-browser-bridge-state-commands.ts | 2 + .../desktop-browser-bridge-dispatch.test.ts | 45 +++++++++++- .../remote-cli-browser-scope.ts | 14 ++-- 8 files changed, 184 insertions(+), 32 deletions(-) diff --git a/src/main/browser/agent-browser-bridge-execution.ts b/src/main/browser/agent-browser-bridge-execution.ts index 4b59a5d7284b..027c12a243e7 100644 --- a/src/main/browser/agent-browser-bridge-execution.ts +++ b/src/main/browser/agent-browser-bridge-execution.ts @@ -19,7 +19,11 @@ import { type SessionState, type ResolvedBrowserCommandTarget } from './agent-browser-bridge-types' -import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' +import { + assertRemoteCliBrowserAccess, + captureRemoteCliBrowserRevocation, + captureRemoteCliBrowserScope +} from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' export abstract class AgentBrowserBridgeExecution extends AgentBrowserBridgeTabs { protected abstract destroySession( @@ -86,6 +90,27 @@ export abstract class AgentBrowserBridgeExecution extends AgentBrowserBridgeTabs // Why: a command can spawn several helpers in turn; none may start after a host's revocation. assertRemoteCliBrowserAccess(session.webContentsId) + const gated = captureRemoteCliBrowserScope().bridged + if (gated) { + // Why on the proxy: the helper's daemon drives the page step by step through it. + const assertNotRevoked = captureRemoteCliBrowserRevocation() + session.proxy.setCommandGate(() => assertNotRevoked(session.webContentsId)) + } + try { + return await this.runGatedAgentBrowser(sessionName, session, commandArgs, execOptions) + } finally { + if (gated) { + session.proxy.setCommandGate(null) + } + } + } + + private async runGatedAgentBrowser( + sessionName: string, + session: SessionState, + commandArgs: string[], + execOptions?: AgentBrowserExecOptions + ): Promise { this.reinitializeIfDaemonIdledOut(sessionName, session) session.lastCommandAt = Date.now() diff --git a/src/main/browser/agent-browser-bridge-host-revocation.test.ts b/src/main/browser/agent-browser-bridge-host-revocation.test.ts index 61daec83638d..991c853d37e3 100644 --- a/src/main/browser/agent-browser-bridge-host-revocation.test.ts +++ b/src/main/browser/agent-browser-bridge-host-revocation.test.ts @@ -101,7 +101,7 @@ describe('AgentBrowserBridge under a host revocation', () => { expect(dispatchedMouseEvents(wc)).toHaveLength(0) }) - it('closes the page proxy to a running helper click when the host is revoked', async () => { + it('gates the page proxy against a running helper click once the host is revoked', async () => { const helperCallbacks: ExecFileCallback[] = [] execFileMock.mockImplementation( (_bin: string, args: string[], _opts: unknown, cb: ExecFileCallback) => { @@ -151,4 +151,73 @@ describe('AgentBrowserBridge under a host revocation', () => { const proxy = CdpWsProxyMock.instances[0] as { setCommandGate: ReturnType } expect(proxy.setCommandGate).not.toHaveBeenCalled() }) + + it('still runs a desktop-local command queued behind a host command that was revoked', async () => { + const wc = mockWebContents(100) + let answerHitTest = (): void => {} + wc.debugger.sendCommand.mockImplementation(async (method: string) => { + if (method === 'Runtime.evaluate') { + await new Promise((resolve) => { + answerHitTest = resolve + }) + return { result: { value: { x: 10, y: 20, adjusted: false, handled: false } } } + } + return {} + }) + webContentsFromIdMock.mockReturnValue(wc) + const revoked = new AbortController() + const hostClick = runInRemoteCliBrowserScope( + WORKSPACE, + () => bridge.mouseClick(10, 20, 'left', undefined, 'tab-1', 18), + revoked.signal + ) + hostClick.catch(() => {}) + await vi.waitFor(() => + expect(wc.debugger.sendCommand).toHaveBeenCalledWith('Runtime.evaluate', expect.anything()) + ) + + const localMove = bridge.mouseMove(5, 6, undefined, 'tab-1') + revoked.abort() + answerHitTest() + + await expect(hostClick).rejects.toMatchObject({ code: 'desktop_browser_bridge_unavailable' }) + await expect(localMove).resolves.toBeDefined() + expect(dispatchedMouseEvents(wc)).toEqual([ + ['Input.dispatchMouseEvent', expect.objectContaining({ type: 'mouseMoved', x: 5, y: 6 })] + ]) + }) + + it('never hit-tests (which can activate the element) once revoked during the focus wait', async () => { + let grantFocus = (): void => {} + const release = vi.fn() + const guardUserFocusForAgentInput = vi.fn( + () => + new Promise<() => void>((resolve) => { + grantFocus = () => resolve(release) + }) + ) + bridge = new AgentBrowserBridge( + mockBrowserManager(undefined, undefined, { guardUserFocusForAgentInput }) + ) + bridge.setActiveTab(100) + const wc = mockWebContents(100) + wc.getType.mockReturnValue('webview') + wc.debugger.sendCommand.mockResolvedValue({}) + webContentsFromIdMock.mockReturnValue(wc) + const revoked = new AbortController() + + const click = runInRemoteCliBrowserScope( + WORKSPACE, + () => bridge.mouseClick(10, 20, 'left', undefined, 'tab-1', 18), + revoked.signal + ) + await vi.waitFor(() => expect(guardUserFocusForAgentInput).toHaveBeenCalled()) + revoked.abort() + grantFocus() + + await expect(click).rejects.toMatchObject({ code: 'desktop_browser_bridge_unavailable' }) + expect(wc.debugger.sendCommand).not.toHaveBeenCalledWith('Runtime.evaluate', expect.anything()) + expect(dispatchedMouseEvents(wc)).toHaveLength(0) + expect(release).toHaveBeenCalled() + }) }) diff --git a/src/main/browser/agent-browser-bridge-interaction-commands.ts b/src/main/browser/agent-browser-bridge-interaction-commands.ts index a617aa0e49fd..2afebf25a28e 100644 --- a/src/main/browser/agent-browser-bridge-interaction-commands.ts +++ b/src/main/browser/agent-browser-bridge-interaction-commands.ts @@ -205,12 +205,28 @@ export abstract class AgentBrowserBridgeInteractionCommands extends AgentBrowser let releaseDebugger = (): void => {} try { releaseDebugger = acquireElectronDebugger(wc).release - const send: GuestCdpSender = (method, params, sessionId) => - sendGuestCdpCommand(wc, method, params, sessionId) - const guardFocus = (): Promise<() => void> => - this.browserManager.guardUserFocusForAgentInput(target.webContentsId) - // Why each step: a host's access can be revoked while an earlier await is pending. - assertRemoteCliBrowserAccess(target.webContentsId) + // Why around every await: a host's access can be revoked while one is pending, and + // key delivery runs further input and edit commands after each. + const assertAccess = (): void => assertRemoteCliBrowserAccess(target.webContentsId) + const send: GuestCdpSender = async (method, params, sessionId) => { + assertAccess() + const result = await sendGuestCdpCommand(wc, method, params, sessionId) + assertAccess() + return result + } + const guardFocus = async (): Promise<() => void> => { + const release = await this.browserManager.guardUserFocusForAgentInput( + target.webContentsId + ) + try { + assertAccess() + } catch (error) { + release() + throw error + } + return release + } + assertAccess() await dispatchPageKeyEvent( wc, { @@ -223,7 +239,7 @@ export abstract class AgentBrowserBridgeInteractionCommands extends AgentBrowser send, guardFocus ) - assertRemoteCliBrowserAccess(target.webContentsId) + assertAccess() await dispatchPageKeyEvent( wc, { diff --git a/src/main/browser/agent-browser-bridge-mouse-commands.ts b/src/main/browser/agent-browser-bridge-mouse-commands.ts index 2fd0a456fe86..310a2820cac7 100644 --- a/src/main/browser/agent-browser-bridge-mouse-commands.ts +++ b/src/main/browser/agent-browser-bridge-mouse-commands.ts @@ -40,6 +40,9 @@ export abstract class AgentBrowserBridgeMouseCommands extends AgentBrowserBridge let releaseFocusGuard = (): void => {} try { releaseFocusGuard = await this.focusPageForPress(wc, target.webContentsId) + // Why each step: a host's access can be revoked while an earlier await is pending, and + // the hit test below may itself activate the element. + assertRemoteCliBrowserAccess(target.webContentsId) const point = cdpButton === 'left' ? // Why: DOM activation can't carry Cmd/Ctrl/Alt/Shift, so modifier clicks use the adjusted point and let CDP dispatch the event. @@ -48,7 +51,6 @@ export abstract class AgentBrowserBridgeMouseCommands extends AgentBrowserBridge // Why: land the tap as one atomic op — separate move/down/up CLI calls visibly hover and can miss small controls. // Why: mobile-emulated BrowserViews can ignore CDP mouse clicks, so the runtime may already have activated DOM controls. if (!point.handled) { - // Why each step: a host's access can be revoked while an earlier await is pending. assertRemoteCliBrowserAccess(target.webContentsId) await wc.debugger.sendCommand('Input.dispatchMouseEvent', { type: 'mousePressed', diff --git a/src/main/browser/agent-browser-bridge-queue.ts b/src/main/browser/agent-browser-bridge-queue.ts index e22eadaf09e8..872c7083ff64 100644 --- a/src/main/browser/agent-browser-bridge-queue.ts +++ b/src/main/browser/agent-browser-bridge-queue.ts @@ -64,7 +64,7 @@ export abstract class AgentBrowserBridgeQueue extends AgentBrowserBridgeShutdown ): Promise { this.assertCommandAdmission() const assertNotRevoked = captureRemoteCliBrowserRevocation() - const bridged = captureRemoteCliBrowserScope() + const origin = captureRemoteCliBrowserScope() assertNotRevoked() const { browserPageId: pageId } = this.resolveCommandTarget( worktreeId, @@ -89,13 +89,13 @@ export abstract class AgentBrowserBridgeQueue extends AgentBrowserBridgeShutdown (session, target) => { const gate = (): void => assertNotRevoked(target.webContentsId) gate() - return bridged - ? this.runRevocableCommand(session, gate, () => execute(session, target)) + return origin.bridged + ? this.reportRevocation(gate, () => execute(session, target)) : execute(session, target) }, options ) - return bridged ? bridged(run) : run() + return origin.run(run) }, resolve: resolve as (value: unknown) => void, reject @@ -120,24 +120,13 @@ export abstract class AgentBrowserBridgeQueue extends AgentBrowserBridgeShutdown return execute(sessionName, target) } - /** - * Runs a host's command so a revocation mid-command stops it: the page's CDP proxy refuses every - * later step the helper sends, and a step that failed that way reports the revocation. - */ - private async runRevocableCommand( - sessionName: string, - gate: () => void, - execute: () => Promise - ): Promise { - const proxy = this.sessions.get(sessionName)?.proxy - proxy?.setCommandGate(gate) + /** A host command whose step was refused mid-way by a revocation reports the revocation. */ + private async reportRevocation(gate: () => void, execute: () => Promise): Promise { try { return await execute() } catch (error) { gate() throw error - } finally { - proxy?.setCommandGate(null) } } diff --git a/src/main/browser/agent-browser-bridge-state-commands.ts b/src/main/browser/agent-browser-bridge-state-commands.ts index 779c6930887c..0e1ed4779aea 100644 --- a/src/main/browser/agent-browser-bridge-state-commands.ts +++ b/src/main/browser/agent-browser-bridge-state-commands.ts @@ -16,6 +16,7 @@ import { BrowserError } from './cdp-bridge' import { parseShellArgs, stripAgentBrowserTargetArgs } from './agent-browser-bridge-process' import { AgentBrowserBridgeInteractionCommands } from './agent-browser-bridge-interaction-commands' import { sendGuestCdpCommand } from './guest-cdp-command' +import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' export abstract class AgentBrowserBridgeStateCommands extends AgentBrowserBridgeInteractionCommands { // ── Cookie commands ── @@ -109,6 +110,7 @@ export abstract class AgentBrowserBridgeStateCommands extends AgentBrowserBridge mobile }) // Why: BrowserView's compositor can keep the old host size after a metrics-only resize, cropping remote screencast clients. + assertRemoteCliBrowserAccess(target.webContentsId) await sendGuestCdpCommand(wc, 'Emulation.setVisibleSize', { width, height }).catch(() => {}) return { diff --git a/src/main/runtime/desktop-browser-bridge/desktop-browser-bridge-dispatch.test.ts b/src/main/runtime/desktop-browser-bridge/desktop-browser-bridge-dispatch.test.ts index 79725b7dcbe5..00d4ac308bbd 100644 --- a/src/main/runtime/desktop-browser-bridge/desktop-browser-bridge-dispatch.test.ts +++ b/src/main/runtime/desktop-browser-bridge/desktop-browser-bridge-dispatch.test.ts @@ -2,6 +2,7 @@ import '../rpc/unused-default-rpc-methods.test-fixture' import { afterEach, describe, expect, it, vi } from 'vitest' import { z } from 'zod' import { + DESKTOP_BROWSER_BRIDGE_CANCEL_CAPABILITY, DESKTOP_BROWSER_BRIDGE_RUNTIME_CAPABILITY, DESKTOP_BROWSER_BRIDGE_SETTING_LABEL } from '../../../shared/desktop-browser-bridge-protocol' @@ -54,6 +55,7 @@ function orcadRuntime(options: { localBrowser?: boolean } = {}) { method, params, caller: context.caller, + signal: context.signal, hostEnabled: true, registry: getDesktopBrowserBridgeRegistry(runtime), localBrowserAvailable: options.localBrowser === true, @@ -73,7 +75,12 @@ const DESKTOP = { clientCapabilities: [DESKTOP_BROWSER_BRIDGE_RUNTIME_CAPABILITY] } -async function attachDesktop(dispatcher: RpcDispatcher, options = DESKTOP) { +async function attachDesktop( + dispatcher: RpcDispatcher, + options: + | typeof DESKTOP + | (Omit & { clientCapabilities: string[] }) = DESKTOP +) { const events: Record[] = [] const done = dispatcher.dispatchStreaming( { @@ -287,4 +294,40 @@ describe('desktop browser bridge on orcad', () => { expect(response).toMatchObject({ ok: true, result: { url: 'local' } }) expect(events).toHaveLength(1) }) + + it.each([ + { + name: 'a cancel-capable desktop hears', + capabilities: [DESKTOP_BROWSER_BRIDGE_CANCEL_CAPABILITY], + cancels: true + }, + { name: 'an older desktop never hears', capabilities: [], cancels: false } + ])('$name that the CLI cancelled a forwarded command', async ({ capabilities, cancels }) => { + setDesktopBrowserBridgeHostEnabled(true) + const { runtime } = orcadRuntime() + const dispatcher = new RpcDispatcher({ runtime, methods: METHODS }) + const { events } = await attachDesktop(dispatcher, { + ...DESKTOP, + clientCapabilities: [DESKTOP_BROWSER_BRIDGE_RUNTIME_CAPABILITY, ...capabilities] + }) + const cli = new AbortController() + + const call = dispatcher.dispatch( + { + id: 'cli-goto', + authToken: 'owner', + method: 'browser.goto', + params: { url: 'https://example.com', worktree: 'path:/srv/app' } + }, + { signal: cli.signal } + ) + await vi.waitFor(() => expect(events).toHaveLength(2)) + const forwarded = ForwardedEvent.parse(events[1]) + cli.abort() + + await expect(call).resolves.toMatchObject({ ok: false }) + expect(events.slice(2)).toEqual( + cancels ? [{ type: 'cancel', requestId: forwarded.requestId }] : [] + ) + }) }) diff --git a/src/main/runtime/desktop-browser-bridge/remote-cli-browser-scope.ts b/src/main/runtime/desktop-browser-bridge/remote-cli-browser-scope.ts index 695c3fdfff88..025640bc5a82 100644 --- a/src/main/runtime/desktop-browser-bridge/remote-cli-browser-scope.ts +++ b/src/main/runtime/desktop-browser-bridge/remote-cli-browser-scope.ts @@ -56,12 +56,18 @@ export function captureRemoteCliBrowserRevocation(): (webContentsId?: number) => } /** - * Captured where a browser command is queued: null outside a bridged command, else a runner that - * puts queued work back inside that scope, so its checks still see a later revocation. + * Captured where a browser command is queued: `run` puts the queued work back in exactly that + * scope (none for a desktop-local command), never the queue runner's, which may be another host's. */ -export function captureRemoteCliBrowserScope(): ((run: () => T) => T) | null { +export function captureRemoteCliBrowserScope(): { + bridged: boolean + run(work: () => T): T +} { const active = scope.getStore() - return active ? (run) => scope.run(active, run) : null + return { + bridged: active !== undefined, + run: (work) => (active ? scope.run(active, work) : scope.exit(work)) + } } /** Throws inside a bridged command once it was revoked or `webContentsId` stopped being admitted. */ From c221a4b5fad037833556a6c310123013f771cd50 Mon Sep 17 00:00:00 2001 From: Neil Date: Sun, 18 Oct 2026 22:52:57 -0700 Subject: [PATCH 03/11] fix(browser): hold a host-driven page at its input sinks instead of per call site --- .../browser/agent-browser-bridge-execution.ts | 27 +--- ...ent-browser-bridge-host-revocation.test.ts | 28 +--- ...ent-browser-bridge-interaction-commands.ts | 28 +--- .../agent-browser-bridge-mouse-commands.ts | 12 +- .../agent-browser-bridge-pointer-commands.ts | 6 +- .../browser/agent-browser-bridge-queue.ts | 16 +- .../agent-browser-bridge-state-commands.ts | 2 - src/main/browser/agent-input-injection.ts | 7 + .../browser/cdp-ws-proxy-command-gate.test.ts | 80 ---------- .../cdp-ws-proxy-host-admission.test.ts | 138 ++++++++++++++++++ src/main/browser/cdp-ws-proxy.ts | 10 +- src/main/browser/guest-cdp-command.ts | 7 + src/main/browser/guest-direct-key-input.ts | 4 +- src/main/browser/remote-cli-page-admission.ts | 29 ++++ 14 files changed, 217 insertions(+), 177 deletions(-) delete mode 100644 src/main/browser/cdp-ws-proxy-command-gate.test.ts create mode 100644 src/main/browser/cdp-ws-proxy-host-admission.test.ts create mode 100644 src/main/browser/remote-cli-page-admission.ts diff --git a/src/main/browser/agent-browser-bridge-execution.ts b/src/main/browser/agent-browser-bridge-execution.ts index 027c12a243e7..9c09d1f5fc19 100644 --- a/src/main/browser/agent-browser-bridge-execution.ts +++ b/src/main/browser/agent-browser-bridge-execution.ts @@ -11,6 +11,7 @@ import { } from './agent-browser-bridge-process' import { translateResult } from './agent-browser-bridge-result' import { AgentBrowserBridgeTabs } from './agent-browser-bridge-tabs' +import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' import { ORCA_TAB_SESSION_PREFIX } from './agent-browser-orphan-sweep' import { canSkipAgentBrowserSessionReset } from './agent-browser-session-reset' import { @@ -19,11 +20,6 @@ import { type SessionState, type ResolvedBrowserCommandTarget } from './agent-browser-bridge-types' -import { - assertRemoteCliBrowserAccess, - captureRemoteCliBrowserRevocation, - captureRemoteCliBrowserScope -} from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' export abstract class AgentBrowserBridgeExecution extends AgentBrowserBridgeTabs { protected abstract destroySession( @@ -90,27 +86,6 @@ export abstract class AgentBrowserBridgeExecution extends AgentBrowserBridgeTabs // Why: a command can spawn several helpers in turn; none may start after a host's revocation. assertRemoteCliBrowserAccess(session.webContentsId) - const gated = captureRemoteCliBrowserScope().bridged - if (gated) { - // Why on the proxy: the helper's daemon drives the page step by step through it. - const assertNotRevoked = captureRemoteCliBrowserRevocation() - session.proxy.setCommandGate(() => assertNotRevoked(session.webContentsId)) - } - try { - return await this.runGatedAgentBrowser(sessionName, session, commandArgs, execOptions) - } finally { - if (gated) { - session.proxy.setCommandGate(null) - } - } - } - - private async runGatedAgentBrowser( - sessionName: string, - session: SessionState, - commandArgs: string[], - execOptions?: AgentBrowserExecOptions - ): Promise { this.reinitializeIfDaemonIdledOut(sessionName, session) session.lastCommandAt = Date.now() diff --git a/src/main/browser/agent-browser-bridge-host-revocation.test.ts b/src/main/browser/agent-browser-bridge-host-revocation.test.ts index 991c853d37e3..edf86e4c9df8 100644 --- a/src/main/browser/agent-browser-bridge-host-revocation.test.ts +++ b/src/main/browser/agent-browser-bridge-host-revocation.test.ts @@ -30,7 +30,6 @@ const { CdpWsProxyMock } = vi.hoisted(() => { this.start = vi.fn(async () => 'ws://127.0.0.1:9222') this.stop = vi.fn(async () => {}) this.getPort = vi.fn(() => 9222) - this.setCommandGate = vi.fn() instances.push(this) }) return { CdpWsProxyMock: Object.assign(MockClass, { instances }) } @@ -39,6 +38,7 @@ vi.mock('./cdp-ws-proxy', () => ({ CdpWsProxy: CdpWsProxyMock })) import { AgentBrowserBridge } from './agent-browser-bridge' import { runInRemoteCliBrowserScope } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' +import { assertRemoteCliPageAdmitted } from './remote-cli-page-admission' import { createFakeAgentBrowserChild, mockBrowserManager, @@ -101,7 +101,7 @@ describe('AgentBrowserBridge under a host revocation', () => { expect(dispatchedMouseEvents(wc)).toHaveLength(0) }) - it('gates the page proxy against a running helper click once the host is revoked', async () => { + it('holds the page against a running helper click once the host is revoked', async () => { const helperCallbacks: ExecFileCallback[] = [] execFileMock.mockImplementation( (_bin: string, args: string[], _opts: unknown, cb: ExecFileCallback) => { @@ -121,14 +121,11 @@ describe('AgentBrowserBridge under a host revocation', () => { revoked.signal ) await vi.waitFor(() => expect(helperCallbacks).toHaveLength(1)) - const proxy = CdpWsProxyMock.instances[0] as { setCommandGate: ReturnType } - const gate = proxy.setCommandGate.mock.calls.at(-1)?.[0] as (() => void) | null - expect(gate).toBeTypeOf('function') - expect(() => gate!()).not.toThrow() + expect(() => assertRemoteCliPageAdmitted(100)).not.toThrow() revoked.abort() - // The helper's next CDP step reaches the proxy and is refused there. - expect(() => gate!()).toThrow('stopped sharing') + // The helper's next CDP step reaches the page's proxy and is refused there. + expect(() => assertRemoteCliPageAdmitted(100)).toThrow('stopped sharing') helperCallbacks[0]!( null, JSON.stringify({ success: false, error: 'The Orca desktop stopped sharing its browser' }), @@ -136,20 +133,7 @@ describe('AgentBrowserBridge under a host revocation', () => { ) await expect(click).rejects.toMatchObject({ code: 'desktop_browser_bridge_unavailable' }) - expect(proxy.setCommandGate).toHaveBeenLastCalledWith(null) - }) - - it('leaves a desktop-local command ungated', async () => { - execFileMock.mockImplementation( - (_bin: string, _args: string[], _opts: unknown, cb: ExecFileCallback) => { - cb(null, JSON.stringify({ success: true, data: { clicked: '@e1' } }), '') - return createFakeAgentBrowserChild({ stdin: { on: vi.fn(), end: vi.fn() } }) - } - ) - - await expect(bridge.click('@e1', undefined, 'tab-1')).resolves.toEqual({ clicked: '@e1' }) - const proxy = CdpWsProxyMock.instances[0] as { setCommandGate: ReturnType } - expect(proxy.setCommandGate).not.toHaveBeenCalled() + expect(() => assertRemoteCliPageAdmitted(100)).not.toThrow() }) it('still runs a desktop-local command queued behind a host command that was revoked', async () => { diff --git a/src/main/browser/agent-browser-bridge-interaction-commands.ts b/src/main/browser/agent-browser-bridge-interaction-commands.ts index 2afebf25a28e..688ab013cfed 100644 --- a/src/main/browser/agent-browser-bridge-interaction-commands.ts +++ b/src/main/browser/agent-browser-bridge-interaction-commands.ts @@ -17,7 +17,6 @@ import { parseCdpKeyEvent, imeFallbackKeyEvent } from './cdp-keyboard-us-layout' import { dispatchPageKeyEvent, type GuestCdpSender } from './guest-direct-key-input' import { sendGuestCdpCommand } from './guest-cdp-command' import { AgentBrowserBridgeCaptureCommands } from './agent-browser-bridge-capture-commands' -import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' export abstract class AgentBrowserBridgeInteractionCommands extends AgentBrowserBridgeCaptureCommands { async hover( @@ -205,28 +204,10 @@ export abstract class AgentBrowserBridgeInteractionCommands extends AgentBrowser let releaseDebugger = (): void => {} try { releaseDebugger = acquireElectronDebugger(wc).release - // Why around every await: a host's access can be revoked while one is pending, and - // key delivery runs further input and edit commands after each. - const assertAccess = (): void => assertRemoteCliBrowserAccess(target.webContentsId) - const send: GuestCdpSender = async (method, params, sessionId) => { - assertAccess() - const result = await sendGuestCdpCommand(wc, method, params, sessionId) - assertAccess() - return result - } - const guardFocus = async (): Promise<() => void> => { - const release = await this.browserManager.guardUserFocusForAgentInput( - target.webContentsId - ) - try { - assertAccess() - } catch (error) { - release() - throw error - } - return release - } - assertAccess() + const send: GuestCdpSender = (method, params, sessionId) => + sendGuestCdpCommand(wc, method, params, sessionId) + const guardFocus = (): Promise<() => void> => + this.browserManager.guardUserFocusForAgentInput(target.webContentsId) await dispatchPageKeyEvent( wc, { @@ -239,7 +220,6 @@ export abstract class AgentBrowserBridgeInteractionCommands extends AgentBrowser send, guardFocus ) - assertAccess() await dispatchPageKeyEvent( wc, { diff --git a/src/main/browser/agent-browser-bridge-mouse-commands.ts b/src/main/browser/agent-browser-bridge-mouse-commands.ts index 310a2820cac7..d20b73e194a3 100644 --- a/src/main/browser/agent-browser-bridge-mouse-commands.ts +++ b/src/main/browser/agent-browser-bridge-mouse-commands.ts @@ -7,8 +7,9 @@ import { resolveMobileTouchClickPoint } from './agent-browser-bridge-mouse' import { acquireElectronDebugger } from './electron-debugger-lease' -import { AgentBrowserBridgePointerCommands } from './agent-browser-bridge-pointer-commands' +import { sendGuestCdpCommand } from './guest-cdp-command' import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' +import { AgentBrowserBridgePointerCommands } from './agent-browser-bridge-pointer-commands' export abstract class AgentBrowserBridgeMouseCommands extends AgentBrowserBridgePointerCommands { // ── Mouse commands ── @@ -40,8 +41,7 @@ export abstract class AgentBrowserBridgeMouseCommands extends AgentBrowserBridge let releaseFocusGuard = (): void => {} try { releaseFocusGuard = await this.focusPageForPress(wc, target.webContentsId) - // Why each step: a host's access can be revoked while an earlier await is pending, and - // the hit test below may itself activate the element. + // Why: the hit test below can activate the element, and access may be revoked meanwhile. assertRemoteCliBrowserAccess(target.webContentsId) const point = cdpButton === 'left' @@ -51,8 +51,7 @@ export abstract class AgentBrowserBridgeMouseCommands extends AgentBrowserBridge // Why: land the tap as one atomic op — separate move/down/up CLI calls visibly hover and can miss small controls. // Why: mobile-emulated BrowserViews can ignore CDP mouse clicks, so the runtime may already have activated DOM controls. if (!point.handled) { - assertRemoteCliBrowserAccess(target.webContentsId) - await wc.debugger.sendCommand('Input.dispatchMouseEvent', { + await sendGuestCdpCommand(wc, 'Input.dispatchMouseEvent', { type: 'mousePressed', x: point.x, y: point.y, @@ -61,8 +60,7 @@ export abstract class AgentBrowserBridgeMouseCommands extends AgentBrowserBridge modifiers: cdpModifiers, clickCount: 1 }) - assertRemoteCliBrowserAccess(target.webContentsId) - await wc.debugger.sendCommand('Input.dispatchMouseEvent', { + await sendGuestCdpCommand(wc, 'Input.dispatchMouseEvent', { type: 'mouseReleased', x: point.x, y: point.y, diff --git a/src/main/browser/agent-browser-bridge-pointer-commands.ts b/src/main/browser/agent-browser-bridge-pointer-commands.ts index 276cf443559c..1377fd843696 100644 --- a/src/main/browser/agent-browser-bridge-pointer-commands.ts +++ b/src/main/browser/agent-browser-bridge-pointer-commands.ts @@ -10,10 +10,10 @@ import { type CdpPointerState } from './cdp-pointer-input' import { acquireElectronDebugger } from './electron-debugger-lease' +import { sendGuestCdpCommand } from './guest-cdp-command' import { AgentBrowserBridgeInputCommands } from './agent-browser-bridge-input-commands' import { isEmbeddedGuest } from './guest-direct-key-input' import { enablePageFocusEmulation } from './agent-input-focus-guard' -import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' type CdpPointerEventParams = { type: 'mouseMoved' | 'mousePressed' | 'mouseReleased' | 'mouseWheel' @@ -65,9 +65,7 @@ export abstract class AgentBrowserBridgePointerCommands extends AgentBrowserBrid if (press) { releaseFocusGuard = await this.focusPageForPress(wc, target.webContentsId) } - // Why after the focus await: a host's access can be revoked while it is pending. - assertRemoteCliBrowserAccess(target.webContentsId) - await wc.debugger.sendCommand('Input.dispatchMouseEvent', params) + await sendGuestCdpCommand(wc, 'Input.dispatchMouseEvent', params) return result } catch (error) { Object.assign(state, preDispatch) diff --git a/src/main/browser/agent-browser-bridge-queue.ts b/src/main/browser/agent-browser-bridge-queue.ts index 872c7083ff64..e4aa8869dc6e 100644 --- a/src/main/browser/agent-browser-bridge-queue.ts +++ b/src/main/browser/agent-browser-bridge-queue.ts @@ -6,6 +6,7 @@ import type { EnqueueTargetedCommandOptions, ResolvedBrowserCommandTarget } from './agent-browser-bridge-types' +import { runWithRemoteCliPageAdmission } from './remote-cli-page-admission' import { captureRemoteCliBrowserRevocation, captureRemoteCliBrowserScope @@ -90,7 +91,7 @@ export abstract class AgentBrowserBridgeQueue extends AgentBrowserBridgeShutdown const gate = (): void => assertNotRevoked(target.webContentsId) gate() return origin.bridged - ? this.reportRevocation(gate, () => execute(session, target)) + ? this.runAdmitted(target.webContentsId, gate, () => execute(session, target)) : execute(session, target) }, options @@ -120,10 +121,17 @@ export abstract class AgentBrowserBridgeQueue extends AgentBrowserBridgeShutdown return execute(sessionName, target) } - /** A host command whose step was refused mid-way by a revocation reports the revocation. */ - private async reportRevocation(gate: () => void, execute: () => Promise): Promise { + /** + * Holds the page for a host's command: every later page-affecting step, including the helper's + * through the CDP proxy, is refused once the host is revoked, and the command reports that. + */ + private async runAdmitted( + webContentsId: number, + gate: () => void, + execute: () => Promise + ): Promise { try { - return await execute() + return await runWithRemoteCliPageAdmission(webContentsId, gate, execute) } catch (error) { gate() throw error diff --git a/src/main/browser/agent-browser-bridge-state-commands.ts b/src/main/browser/agent-browser-bridge-state-commands.ts index 0e1ed4779aea..779c6930887c 100644 --- a/src/main/browser/agent-browser-bridge-state-commands.ts +++ b/src/main/browser/agent-browser-bridge-state-commands.ts @@ -16,7 +16,6 @@ import { BrowserError } from './cdp-bridge' import { parseShellArgs, stripAgentBrowserTargetArgs } from './agent-browser-bridge-process' import { AgentBrowserBridgeInteractionCommands } from './agent-browser-bridge-interaction-commands' import { sendGuestCdpCommand } from './guest-cdp-command' -import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' export abstract class AgentBrowserBridgeStateCommands extends AgentBrowserBridgeInteractionCommands { // ── Cookie commands ── @@ -110,7 +109,6 @@ export abstract class AgentBrowserBridgeStateCommands extends AgentBrowserBridge mobile }) // Why: BrowserView's compositor can keep the old host size after a metrics-only resize, cropping remote screencast clients. - assertRemoteCliBrowserAccess(target.webContentsId) await sendGuestCdpCommand(wc, 'Emulation.setVisibleSize', { width, height }).catch(() => {}) return { diff --git a/src/main/browser/agent-input-injection.ts b/src/main/browser/agent-input-injection.ts index e5ec1cc6b4a9..edbae47af509 100644 --- a/src/main/browser/agent-input-injection.ts +++ b/src/main/browser/agent-input-injection.ts @@ -1,4 +1,5 @@ import type { WebContents } from 'electron' +import { assertRemoteCliPageAdmitted } from './remote-cli-page-admission' // Why: Orca's guest shortcut and grab handlers act on keys the page receives, and an unhandled key // can reach the app menu. Keys an agent injects must do neither (#20595), whichever path sends them. @@ -8,11 +9,17 @@ export function isAgentInjectingInput(webContents: WebContents): boolean { return (injectionDepth.get(webContents) ?? 0) > 0 } +/** Refuses agent input to a page a revoked SSH host's command still holds. */ +export function assertAgentInputAdmitted(webContents: WebContents): void { + assertRemoteCliPageAdmitted(webContents.id) +} + /** Marks input sent during `run` as the agent's: Orca shortcuts and app menu items ignore it. */ export async function withAgentInputInjection( webContents: WebContents, run: () => Promise ): Promise { + assertAgentInputAdmitted(webContents) const depth = injectionDepth.get(webContents) ?? 0 injectionDepth.set(webContents, depth + 1) if (depth === 0) { diff --git a/src/main/browser/cdp-ws-proxy-command-gate.test.ts b/src/main/browser/cdp-ws-proxy-command-gate.test.ts deleted file mode 100644 index e56e44b4b143..000000000000 --- a/src/main/browser/cdp-ws-proxy-command-gate.test.ts +++ /dev/null @@ -1,80 +0,0 @@ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' -import { CdpWsProxy } from './cdp-ws-proxy' -import { - connect, - createMockWebContents, - getSendCommandMethods, - sendAndReceive, - type MockWebContents -} from './cdp-ws-proxy-test-harness' - -vi.mock('electron', () => ({ - webContents: { fromId: vi.fn() } -})) - -describe('CdpWsProxy command gate', () => { - let mock: MockWebContents - let proxy: CdpWsProxy - let endpoint: string - - beforeEach(async () => { - mock = createMockWebContents() - // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the mock implements every WebContents member the proxy calls. - proxy = new CdpWsProxy(mock.webContents as never, () => () => {}) - endpoint = await proxy.start() - }) - - afterEach(async () => { - await proxy.stop() - }) - - it('refuses every later helper step once the gate closes, without sending it to the page', async () => { - let revoked = false - proxy.setCommandGate(() => { - if (revoked) { - throw new Error('The Orca desktop stopped sharing its browser before this command ran.') - } - }) - const client = await connect(endpoint) - const press = { type: 'mousePressed', x: 1, y: 1, button: 'left', clickCount: 1 } - - const allowed = await sendAndReceive(client, { - id: 1, - method: 'Input.dispatchMouseEvent', - params: press - }) - revoked = true - const refused = await sendAndReceive(client, { - id: 2, - method: 'Input.dispatchMouseEvent', - params: { ...press, type: 'mouseReleased' } - }) - - expect(allowed.error).toBeUndefined() - expect(refused).toMatchObject({ id: 2, error: { message: expect.stringContaining('stopped') } }) - expect( - mock.webContents.debugger.sendCommand.mock.calls.filter( - (call) => call[0] === 'Input.dispatchMouseEvent' - ) - ).toHaveLength(1) - client.close() - }) - - it('forwards commands again once the gate is cleared', async () => { - proxy.setCommandGate(() => { - throw new Error('revoked') - }) - proxy.setCommandGate(null) - const client = await connect(endpoint) - - const response = await sendAndReceive(client, { - id: 3, - method: 'Accessibility.getFullAXTree', - params: {} - }) - - expect(response.error).toBeUndefined() - expect(getSendCommandMethods(mock)).toContain('Accessibility.getFullAXTree') - client.close() - }) -}) diff --git a/src/main/browser/cdp-ws-proxy-host-admission.test.ts b/src/main/browser/cdp-ws-proxy-host-admission.test.ts new file mode 100644 index 000000000000..f2de9b8fdc8f --- /dev/null +++ b/src/main/browser/cdp-ws-proxy-host-admission.test.ts @@ -0,0 +1,138 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { CdpWsProxy } from './cdp-ws-proxy' +import { runWithRemoteCliPageAdmission } from './remote-cli-page-admission' +import { + connect, + createMockWebContents, + getSendCommandMethods, + sendAndReceive, + type MockWebContents +} from './cdp-ws-proxy-test-harness' + +vi.mock('electron', () => ({ + webContents: { fromId: vi.fn() } +})) + +const PAGE_ID = 7 +const REVOKED = 'The Orca desktop stopped sharing its browser before this command ran.' + +function mouseEvents(mock: MockWebContents): string[] { + return mock.webContents.debugger.sendCommand.mock.calls + .filter((call) => call[0] === 'Input.dispatchMouseEvent') + .map((call) => String(call[1]?.type)) +} + +describe('CdpWsProxy under a host command', () => { + let mock: MockWebContents + let proxy: CdpWsProxy + let endpoint: string + let guardFocus: () => Promise<() => void> + + beforeEach(async () => { + mock = createMockWebContents() + guardFocus = async () => () => {} + const page = { ...mock.webContents, id: PAGE_ID } + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the mock implements every WebContents member the proxy calls. + proxy = new CdpWsProxy( + page as never, + () => () => {}, + () => guardFocus() + ) + endpoint = await proxy.start() + }) + + afterEach(async () => { + await proxy.stop() + }) + + it('refuses every helper step after the host is revoked, without sending it to the page', async () => { + let revoked = false + let finish = (): void => {} + const held = runWithRemoteCliPageAdmission( + PAGE_ID, + () => { + if (revoked) { + throw new Error(REVOKED) + } + }, + () => new Promise((resolve) => (finish = resolve)) + ) + const client = await connect(endpoint) + const press = { type: 'mouseMoved', x: 1, y: 1 } + + const allowed = await sendAndReceive(client, { + id: 1, + method: 'Input.dispatchMouseEvent', + params: press + }) + revoked = true + const refused = await sendAndReceive(client, { + id: 2, + method: 'Input.dispatchMouseEvent', + params: press + }) + + expect(allowed.error).toBeUndefined() + expect(refused).toMatchObject({ id: 2, error: { message: REVOKED } }) + expect(mouseEvents(mock)).toEqual(['mouseMoved']) + finish() + await held + client.close() + }) + + it('drops a press it accepted before the revocation but had not delivered yet', async () => { + mock.webContents.getType.mockReturnValue('webview') + let grantFocus = (): void => {} + guardFocus = () => new Promise((resolve) => (grantFocus = () => resolve(() => {}))) + let revoked = false + let finish = (): void => {} + const held = runWithRemoteCliPageAdmission( + PAGE_ID, + () => { + if (revoked) { + throw new Error(REVOKED) + } + }, + () => new Promise((resolve) => (finish = resolve)) + ) + const client = await connect(endpoint) + + const reply = sendAndReceive(client, { + id: 3, + method: 'Input.dispatchMouseEvent', + params: { type: 'mousePressed', x: 1, y: 1, button: 'left', clickCount: 1 } + }) + await vi.waitFor(() => + expect(getSendCommandMethods(mock)).toContain('Emulation.setFocusEmulationEnabled') + ) + revoked = true + grantFocus() + + await expect(reply).resolves.toMatchObject({ id: 3, error: { message: REVOKED } }) + expect(mouseEvents(mock)).toEqual([]) + finish() + await held + client.close() + }) + + it('forwards commands again once the host command ends', async () => { + await runWithRemoteCliPageAdmission( + PAGE_ID, + () => { + throw new Error(REVOKED) + }, + async () => {} + ) + const client = await connect(endpoint) + + const response = await sendAndReceive(client, { + id: 4, + method: 'Accessibility.getFullAXTree', + params: {} + }) + + expect(response.error).toBeUndefined() + expect(getSendCommandMethods(mock)).toContain('Accessibility.getFullAXTree') + client.close() + }) +}) diff --git a/src/main/browser/cdp-ws-proxy.ts b/src/main/browser/cdp-ws-proxy.ts index 675ec49350ed..79c4dd63f7c8 100644 --- a/src/main/browser/cdp-ws-proxy.ts +++ b/src/main/browser/cdp-ws-proxy.ts @@ -12,6 +12,7 @@ import type { CapturePaintHold } from './cdp-screenshot' import type { AgentInputFocusGuard } from './agent-input-focus-guard' import { CdpPageInputCommands } from './cdp-page-input-commands' import { isTrustedCdpLoopbackRequest } from './cdp-loopback-request-guard' +import { assertRemoteCliPageAdmitted } from './remote-cli-page-admission' export class CdpWsProxy { private httpServer: Server | null = null @@ -27,7 +28,6 @@ export class CdpWsProxy { private readonly domFocusReplay: CdpDomFocusReplay private readonly pageCapture: CdpPageCaptureCommands private readonly input: CdpPageInputCommands - private commandGate: (() => void) | null = null constructor( private readonly webContents: WebContents, @@ -160,11 +160,6 @@ export class CdpWsProxy { client?.close() } - /** While set, every client command must pass it first; a throw refuses the command unsent. */ - setCommandGate(gate: (() => void) | null): void { - this.commandGate = gate - } - private clearClientState(): void { // Why: session and focus state belongs to one websocket and must not cross client replacement. this.domFocusReplay.clear() @@ -184,8 +179,9 @@ export class CdpWsProxy { } const clientId = msg.id this.responder.recordRequestSessionId(client, clientId, msg) + // Why here too: a revoked host's helper is refused even its read-only steps, so it stops at once. try { - this.commandGate?.() + assertRemoteCliPageAdmitted(this.webContents.id) } catch (error) { this.responder.sendError( clientId, diff --git a/src/main/browser/guest-cdp-command.ts b/src/main/browser/guest-cdp-command.ts index ed7735df2d87..ee0ea7e35850 100644 --- a/src/main/browser/guest-cdp-command.ts +++ b/src/main/browser/guest-cdp-command.ts @@ -1,5 +1,6 @@ import type { WebContents } from 'electron' import { BrowserError } from './browser-error' +import { assertRemoteCliPageAdmitted } from './remote-cli-page-admission' // Why: Chromium resizes the page's view for these without checking the view still exists // (WebContentsImpl::SetDeviceEmulationSize). A crashed renderer takes its view with it until the @@ -10,6 +11,7 @@ const VIEW_RESIZING_CDP_METHODS: ReadonlySet = new Set([ ]) type GuestCdpTarget = Pick & { + id?: number debugger: Pick } @@ -24,6 +26,11 @@ export function sendGuestCdpCommand( params?: Record, sessionId?: string ): Promise { + try { + assertRemoteCliPageAdmitted(guest.id) + } catch (error) { + return Promise.reject(error) + } // Why same task as the send: isCrashed() flips in the same Chromium task that drops the view, // so checking right before sendCommand leaves no gap for the renderer to die in between. if (VIEW_RESIZING_CDP_METHODS.has(method) && (guest.isDestroyed() || guest.isCrashed())) { diff --git a/src/main/browser/guest-direct-key-input.ts b/src/main/browser/guest-direct-key-input.ts index dd2d559a6755..1bc01444cdfd 100644 --- a/src/main/browser/guest-direct-key-input.ts +++ b/src/main/browser/guest-direct-key-input.ts @@ -1,6 +1,6 @@ import type { InputEvent, KeyboardInputEvent, WebContents } from 'electron' import { enablePageFocusEmulation, type AgentInputFocusGuard } from './agent-input-focus-guard' -import { withAgentInputInjection } from './agent-input-injection' +import { assertAgentInputAdmitted, withAgentInputInjection } from './agent-input-injection' import { resolveFocusedFrameRoute, settleWithin, @@ -195,6 +195,8 @@ async function waitForPageToHandleInput(send: GuestCdpSender): Promise { } function runEditCommand(webContents: WebContents, command: GuestEditCommand): void { + // Why: it runs after awaiting the page, so a host's access may have been revoked meanwhile. + assertAgentInputAdmitted(webContents) switch (command) { case 'selectAll': webContents.selectAll() diff --git a/src/main/browser/remote-cli-page-admission.ts b/src/main/browser/remote-cli-page-admission.ts new file mode 100644 index 000000000000..e159851782dc --- /dev/null +++ b/src/main/browser/remote-cli-page-admission.ts @@ -0,0 +1,29 @@ +/** + * The pages an SSH host's command is driving right now, each with the check that refuses its next + * step once that host is revoked. Every page-affecting sink (guest CDP sends, native input events, + * edit commands) consults it, so no queued or deferred step of a running command outlives the + * revocation. + */ +const admissions = new Map void>() + +export async function runWithRemoteCliPageAdmission( + webContentsId: number, + admit: () => void, + run: () => Promise +): Promise { + admissions.set(webContentsId, admit) + try { + return await run() + } finally { + if (admissions.get(webContentsId) === admit) { + admissions.delete(webContentsId) + } + } +} + +/** Throws while a revoked host's command still holds this page; a no-op otherwise. */ +export function assertRemoteCliPageAdmitted(webContentsId: number | undefined): void { + if (webContentsId !== undefined) { + admissions.get(webContentsId)?.() + } +} From 258875864905a73c97ec3dc761c475d4ae97aa8f Mon Sep 17 00:00:00 2001 From: Neil Date: Sun, 18 Oct 2026 22:56:58 -0700 Subject: [PATCH 04/11] test(browser): checked mock types in the host revocation suites --- .../browser/agent-browser-bridge-host-revocation.test.ts | 8 +++++--- src/main/browser/cdp-ws-proxy-host-admission.test.ts | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/main/browser/agent-browser-bridge-host-revocation.test.ts b/src/main/browser/agent-browser-bridge-host-revocation.test.ts index edf86e4c9df8..e6e703d9ad48 100644 --- a/src/main/browser/agent-browser-bridge-host-revocation.test.ts +++ b/src/main/browser/agent-browser-bridge-host-revocation.test.ts @@ -6,7 +6,7 @@ const { execFileMock, webContentsFromIdMock, existsSyncMock, readFileSyncMock, s webContentsFromIdMock: vi.fn(), existsSyncMock: vi.fn(() => false), readFileSyncMock: vi.fn(() => Buffer.from('')), - stdinWrites: [] as string[] + stdinWrites: new Array() })) vi.mock('child_process', () => ({ execFile: execFileMock })) @@ -24,8 +24,10 @@ vi.mock('electron', () => ({ })) const { CdpWsProxyMock } = vi.hoisted(() => { const instances: unknown[] = [] - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const MockClass = vi.fn().mockImplementation(function (this: any, _wc: unknown) { + const MockClass = vi.fn().mockImplementation(function ( + this: Record, + _wc: unknown + ) { this._wc = _wc this.start = vi.fn(async () => 'ws://127.0.0.1:9222') this.stop = vi.fn(async () => {}) diff --git a/src/main/browser/cdp-ws-proxy-host-admission.test.ts b/src/main/browser/cdp-ws-proxy-host-admission.test.ts index f2de9b8fdc8f..9405e82edc5c 100644 --- a/src/main/browser/cdp-ws-proxy-host-admission.test.ts +++ b/src/main/browser/cdp-ws-proxy-host-admission.test.ts @@ -32,8 +32,8 @@ describe('CdpWsProxy under a host command', () => { mock = createMockWebContents() guardFocus = async () => () => {} const page = { ...mock.webContents, id: PAGE_ID } - // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the mock implements every WebContents member the proxy calls. proxy = new CdpWsProxy( + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the mock implements every WebContents member the proxy calls. page as never, () => () => {}, () => guardFocus() From 22f3f454cb5d39168496e6bcd8b4b50354692765 Mon Sep 17 00:00:00 2001 From: Neil Date: Sun, 18 Oct 2026 23:03:21 -0700 Subject: [PATCH 05/11] fix(browser): refuse only a host's own traffic, never the desktop's page commands --- src/main/browser/agent-input-injection.ts | 4 ++- src/main/browser/cdp-debugger-channel.ts | 7 +++++ .../cdp-ws-proxy-host-admission.test.ts | 27 +++++++++++++++++++ src/main/browser/guest-cdp-command.ts | 5 ++-- 4 files changed, 40 insertions(+), 3 deletions(-) diff --git a/src/main/browser/agent-input-injection.ts b/src/main/browser/agent-input-injection.ts index edbae47af509..3ae92f7bb989 100644 --- a/src/main/browser/agent-input-injection.ts +++ b/src/main/browser/agent-input-injection.ts @@ -1,5 +1,6 @@ import type { WebContents } from 'electron' import { assertRemoteCliPageAdmitted } from './remote-cli-page-admission' +import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' // Why: Orca's guest shortcut and grab handlers act on keys the page receives, and an unhandled key // can reach the app menu. Keys an agent injects must do neither (#20595), whichever path sends them. @@ -9,8 +10,9 @@ export function isAgentInjectingInput(webContents: WebContents): boolean { return (injectionDepth.get(webContents) ?? 0) > 0 } -/** Refuses agent input to a page a revoked SSH host's command still holds. */ +/** Refuses agent input that a revoked SSH host's command sends, directly or through its helper. */ export function assertAgentInputAdmitted(webContents: WebContents): void { + assertRemoteCliBrowserAccess(webContents.id) assertRemoteCliPageAdmitted(webContents.id) } diff --git a/src/main/browser/cdp-debugger-channel.ts b/src/main/browser/cdp-debugger-channel.ts index 18620aa6c6d3..82da834ca1db 100644 --- a/src/main/browser/cdp-debugger-channel.ts +++ b/src/main/browser/cdp-debugger-channel.ts @@ -1,6 +1,7 @@ import { WebSocket } from 'ws' import type { WebContents } from 'electron' import { acquireElectronDebugger, type ElectronDebuggerLease } from './electron-debugger-lease' +import { assertRemoteCliPageAdmitted } from './remote-cli-page-admission' import type { CdpClientResponseWriter } from './cdp-client-response-writer' import type { CdpSyntheticSessionRegistry } from './cdp-synthetic-session-registry' import { sendGuestCdpCommand } from './guest-cdp-command' @@ -93,6 +94,12 @@ export class CdpDebuggerChannel { params: Record, sessionId?: string ): Promise { + // Why: the proxy's queued input outlives the message's admission; a revoked host's must not land. + try { + assertRemoteCliPageAdmitted(this.webContents.id) + } catch (error) { + return Promise.reject(error) + } return sessionId ? sendGuestCdpCommand(this.webContents, method, params, sessionId) : sendGuestCdpCommand(this.webContents, method, params) diff --git a/src/main/browser/cdp-ws-proxy-host-admission.test.ts b/src/main/browser/cdp-ws-proxy-host-admission.test.ts index 9405e82edc5c..c73787caad1a 100644 --- a/src/main/browser/cdp-ws-proxy-host-admission.test.ts +++ b/src/main/browser/cdp-ws-proxy-host-admission.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' import { CdpWsProxy } from './cdp-ws-proxy' import { runWithRemoteCliPageAdmission } from './remote-cli-page-admission' +import { sendGuestCdpCommand } from './guest-cdp-command' import { connect, createMockWebContents, @@ -135,4 +136,30 @@ describe('CdpWsProxy under a host command', () => { expect(getSendCommandMethods(mock)).toContain('Accessibility.getFullAXTree') client.close() }) + + it('never refuses the desktop its own page commands while a revoked host command unwinds', async () => { + let finish = (): void => {} + const held = runWithRemoteCliPageAdmission( + PAGE_ID, + () => { + throw new Error(REVOKED) + }, + () => new Promise((resolve) => (finish = resolve)) + ) + + await expect( + sendGuestCdpCommand( + { ...mock.webContents, id: PAGE_ID }, + 'Emulation.setDeviceMetricsOverride', + { + width: 400, + height: 300, + deviceScaleFactor: 1, + mobile: false + } + ) + ).resolves.toEqual({}) + finish() + await held + }) }) diff --git a/src/main/browser/guest-cdp-command.ts b/src/main/browser/guest-cdp-command.ts index ee0ea7e35850..8847038607c2 100644 --- a/src/main/browser/guest-cdp-command.ts +++ b/src/main/browser/guest-cdp-command.ts @@ -1,6 +1,6 @@ import type { WebContents } from 'electron' import { BrowserError } from './browser-error' -import { assertRemoteCliPageAdmitted } from './remote-cli-page-admission' +import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' // Why: Chromium resizes the page's view for these without checking the view still exists // (WebContentsImpl::SetDeviceEmulationSize). A crashed renderer takes its view with it until the @@ -27,7 +27,8 @@ export function sendGuestCdpCommand( sessionId?: string ): Promise { try { - assertRemoteCliPageAdmitted(guest.id) + // Why: only a host's own command runs in its scope; desktop-owned sends are never refused here. + assertRemoteCliBrowserAccess(guest.id) } catch (error) { return Promise.reject(error) } From 7a71662cf970b59980b840169d059d62b8e1eb8d Mon Sep 17 00:00:00 2001 From: Neil Date: Sun, 18 Oct 2026 23:33:36 -0700 Subject: [PATCH 06/11] fix(browser): a host's proxy never keeps its scope, and a revoked click still lets go of the button --- ...ent-browser-bridge-host-revocation.test.ts | 56 ++++++++++++++++++- .../browser/agent-browser-bridge-lifecycle.ts | 5 +- src/main/browser/cdp-debugger-channel.ts | 2 +- .../cdp-ws-proxy-host-admission.test.ts | 24 ++++++++ src/main/browser/cdp-ws-proxy.ts | 2 +- src/main/browser/guest-cdp-command.ts | 5 +- src/main/browser/remote-cli-page-admission.ts | 23 +++++++- .../remote-cli-browser-scope.ts | 5 ++ 8 files changed, 115 insertions(+), 7 deletions(-) diff --git a/src/main/browser/agent-browser-bridge-host-revocation.test.ts b/src/main/browser/agent-browser-bridge-host-revocation.test.ts index e6e703d9ad48..74a647754b0e 100644 --- a/src/main/browser/agent-browser-bridge-host-revocation.test.ts +++ b/src/main/browser/agent-browser-bridge-host-revocation.test.ts @@ -39,7 +39,10 @@ const { CdpWsProxyMock } = vi.hoisted(() => { vi.mock('./cdp-ws-proxy', () => ({ CdpWsProxy: CdpWsProxyMock })) import { AgentBrowserBridge } from './agent-browser-bridge' -import { runInRemoteCliBrowserScope } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' +import { + captureRemoteCliBrowserScope, + runInRemoteCliBrowserScope +} from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' import { assertRemoteCliPageAdmitted } from './remote-cli-page-admission' import { createFakeAgentBrowserChild, @@ -206,4 +209,55 @@ describe('AgentBrowserBridge under a host revocation', () => { expect(dispatchedMouseEvents(wc)).toHaveLength(0) expect(release).toHaveBeenCalled() }) + + it('still releases a press that reached the page before the host was revoked', async () => { + const wc = mockWebContents(100) + const revoked = new AbortController() + wc.debugger.sendCommand.mockImplementation( + async (method: string, params?: { type?: string }) => { + if (method === 'Runtime.evaluate') { + return { result: { value: { x: 10, y: 20, adjusted: false, handled: false } } } + } + if (params?.type === 'mousePressed') { + revoked.abort() + } + return {} + } + ) + webContentsFromIdMock.mockReturnValue(wc) + + await runInRemoteCliBrowserScope( + WORKSPACE, + () => bridge.mouseClick(10, 20, 'left', undefined, 'tab-1', 18), + revoked.signal + ).catch(() => {}) + + expect(dispatchedMouseEvents(wc)).toEqual([ + ['Input.dispatchMouseEvent', expect.objectContaining({ type: 'mousePressed' })], + ['Input.dispatchMouseEvent', expect.objectContaining({ type: 'mouseReleased' })] + ]) + }) + + it("starts the page's CDP proxy outside the host's scope, so it outlives that host's request", async () => { + let startedInHostScope: boolean | undefined + CdpWsProxyMock.mockImplementationOnce(function (this: Record) { + this.start = vi.fn(async () => { + startedInHostScope = captureRemoteCliBrowserScope().bridged + return 'ws://127.0.0.1:9222' + }) + this.stop = vi.fn(async () => {}) + this.getPort = vi.fn(() => 9222) + CdpWsProxyMock.instances.push(this) + }) + execFileMock.mockImplementation( + (_bin: string, _args: string[], _opts: unknown, cb: ExecFileCallback) => { + cb(null, JSON.stringify({ success: true, data: null }), '') + return createFakeAgentBrowserChild({ stdin: { on: vi.fn(), end: vi.fn() } }) + } + ) + + await runInRemoteCliBrowserScope(WORKSPACE, () => bridge.click('@e1', undefined, 'tab-1')) + + expect(startedInHostScope).toBe(false) + }) }) diff --git a/src/main/browser/agent-browser-bridge-lifecycle.ts b/src/main/browser/agent-browser-bridge-lifecycle.ts index fdd1d387017a..e76fa42c6095 100644 --- a/src/main/browser/agent-browser-bridge-lifecycle.ts +++ b/src/main/browser/agent-browser-bridge-lifecycle.ts @@ -4,6 +4,7 @@ import { ORCA_TAB_SESSION_PREFIX } from './agent-browser-orphan-sweep' import { AgentBrowserBridgeRawProcess } from './agent-browser-bridge-raw-process' import type { AgentBrowserCleanupOptions } from './agent-browser-bridge-types' import { AGENT_BROWSER_CLEANUP_TIMEOUT_MS } from './agent-browser-bridge-types' +import { runOutsideRemoteCliBrowserScope } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' export abstract class AgentBrowserBridgeLifecycle extends AgentBrowserBridgeRawProcess { async onTabClosed(webContentsId: number): Promise { @@ -126,7 +127,9 @@ export abstract class AgentBrowserBridgeLifecycle extends AgentBrowserBridgeRawP }) } - const promise = createSession() + // Why: the proxy's sockets keep the async scope they were created in; a host's would refuse + // every later command on this page, the desktop's included, after that host's request ended. + const promise = runOutsideRemoteCliBrowserScope(createSession) this.pendingSessionCreation.set(sessionName, promise) try { await promise diff --git a/src/main/browser/cdp-debugger-channel.ts b/src/main/browser/cdp-debugger-channel.ts index 82da834ca1db..0f25009fa1e6 100644 --- a/src/main/browser/cdp-debugger-channel.ts +++ b/src/main/browser/cdp-debugger-channel.ts @@ -96,7 +96,7 @@ export class CdpDebuggerChannel { ): Promise { // Why: the proxy's queued input outlives the message's admission; a revoked host's must not land. try { - assertRemoteCliPageAdmitted(this.webContents.id) + assertRemoteCliPageAdmitted(this.webContents.id, method, params) } catch (error) { return Promise.reject(error) } diff --git a/src/main/browser/cdp-ws-proxy-host-admission.test.ts b/src/main/browser/cdp-ws-proxy-host-admission.test.ts index c73787caad1a..1b9f6246d8f3 100644 --- a/src/main/browser/cdp-ws-proxy-host-admission.test.ts +++ b/src/main/browser/cdp-ws-proxy-host-admission.test.ts @@ -81,6 +81,30 @@ describe('CdpWsProxy under a host command', () => { client.close() }) + it('still delivers a release after the revocation, so no button stays held on the page', async () => { + let finish = (): void => {} + const held = runWithRemoteCliPageAdmission( + PAGE_ID, + () => { + throw new Error(REVOKED) + }, + () => new Promise((resolve) => (finish = resolve)) + ) + const client = await connect(endpoint) + + const response = await sendAndReceive(client, { + id: 5, + method: 'Input.dispatchMouseEvent', + params: { type: 'mouseReleased', x: 1, y: 1, button: 'left', clickCount: 1 } + }) + + expect(response.error).toBeUndefined() + expect(mouseEvents(mock)).toEqual(['mouseReleased']) + finish() + await held + client.close() + }) + it('drops a press it accepted before the revocation but had not delivered yet', async () => { mock.webContents.getType.mockReturnValue('webview') let grantFocus = (): void => {} diff --git a/src/main/browser/cdp-ws-proxy.ts b/src/main/browser/cdp-ws-proxy.ts index 79c4dd63f7c8..abd3c1d738f6 100644 --- a/src/main/browser/cdp-ws-proxy.ts +++ b/src/main/browser/cdp-ws-proxy.ts @@ -181,7 +181,7 @@ export class CdpWsProxy { this.responder.recordRequestSessionId(client, clientId, msg) // Why here too: a revoked host's helper is refused even its read-only steps, so it stops at once. try { - assertRemoteCliPageAdmitted(this.webContents.id) + assertRemoteCliPageAdmitted(this.webContents.id, msg.method, msg.params) } catch (error) { this.responder.sendError( clientId, diff --git a/src/main/browser/guest-cdp-command.ts b/src/main/browser/guest-cdp-command.ts index 8847038607c2..4910c89591e2 100644 --- a/src/main/browser/guest-cdp-command.ts +++ b/src/main/browser/guest-cdp-command.ts @@ -1,6 +1,7 @@ import type { WebContents } from 'electron' import { BrowserError } from './browser-error' import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' +import { isInputReleaseCommand } from './remote-cli-page-admission' // Why: Chromium resizes the page's view for these without checking the view still exists // (WebContentsImpl::SetDeviceEmulationSize). A crashed renderer takes its view with it until the @@ -28,7 +29,9 @@ export function sendGuestCdpCommand( ): Promise { try { // Why: only a host's own command runs in its scope; desktop-owned sends are never refused here. - assertRemoteCliBrowserAccess(guest.id) + if (!isInputReleaseCommand(method, params)) { + assertRemoteCliBrowserAccess(guest.id) + } } catch (error) { return Promise.reject(error) } diff --git a/src/main/browser/remote-cli-page-admission.ts b/src/main/browser/remote-cli-page-admission.ts index e159851782dc..e5ce281e4ef2 100644 --- a/src/main/browser/remote-cli-page-admission.ts +++ b/src/main/browser/remote-cli-page-admission.ts @@ -22,8 +22,27 @@ export async function runWithRemoteCliPageAdmission( } /** Throws while a revoked host's command still holds this page; a no-op otherwise. */ -export function assertRemoteCliPageAdmitted(webContentsId: number | undefined): void { - if (webContentsId !== undefined) { +export function assertRemoteCliPageAdmitted( + webContentsId: number | undefined, + method?: string, + params?: Record +): void { + if (webContentsId !== undefined && !isInputReleaseCommand(method, params)) { admissions.get(webContentsId)?.() } } + +/** Ends input a command already began; refusing it would leave a button or key held on the page. */ +export function isInputReleaseCommand(method?: string, params?: Record): boolean { + const type = params?.type + switch (method) { + case 'Input.dispatchMouseEvent': + return type === 'mouseReleased' + case 'Input.dispatchKeyEvent': + return type === 'keyUp' + case 'Input.dispatchTouchEvent': + return type === 'touchEnd' || type === 'touchCancel' + default: + return false + } +} diff --git a/src/main/runtime/desktop-browser-bridge/remote-cli-browser-scope.ts b/src/main/runtime/desktop-browser-bridge/remote-cli-browser-scope.ts index 025640bc5a82..ff510c30d6e9 100644 --- a/src/main/runtime/desktop-browser-bridge/remote-cli-browser-scope.ts +++ b/src/main/runtime/desktop-browser-bridge/remote-cli-browser-scope.ts @@ -70,6 +70,11 @@ export function captureRemoteCliBrowserScope(): { } } +/** Runs `work` in no host's scope, so long-lived resources it creates never inherit one. */ +export function runOutsideRemoteCliBrowserScope(work: () => T): T { + return scope.exit(work) +} + /** Throws inside a bridged command once it was revoked or `webContentsId` stopped being admitted. */ export function assertRemoteCliBrowserAccess(webContentsId?: number): void { captureRemoteCliBrowserRevocation()(webContentsId) From db86301925d8a151fba4f27ed77f4894945c21d9 Mon Sep 17 00:00:00 2001 From: Neil Date: Sun, 18 Oct 2026 23:57:15 -0700 Subject: [PATCH 07/11] fix(browser): a revoked command may release only what it pressed, on every input path --- ...ent-browser-bridge-host-revocation.test.ts | 23 +- src/main/browser/agent-input-injection.ts | 21 +- src/main/browser/cdp-page-input-commands.ts | 6 +- .../cdp-ws-proxy-host-admission.test.ts | 69 +++++- src/main/browser/guest-cdp-command.ts | 12 +- .../browser/guest-direct-key-input.test.ts | 3 +- src/main/browser/guest-direct-key-input.ts | 201 +++--------------- src/main/browser/guest-key-event-plan.ts | 162 ++++++++++++++ src/main/browser/remote-cli-page-admission.ts | 78 +++++-- 9 files changed, 355 insertions(+), 220 deletions(-) create mode 100644 src/main/browser/guest-key-event-plan.ts diff --git a/src/main/browser/agent-browser-bridge-host-revocation.test.ts b/src/main/browser/agent-browser-bridge-host-revocation.test.ts index 74a647754b0e..e00a012433b4 100644 --- a/src/main/browser/agent-browser-bridge-host-revocation.test.ts +++ b/src/main/browser/agent-browser-bridge-host-revocation.test.ts @@ -213,17 +213,20 @@ describe('AgentBrowserBridge under a host revocation', () => { it('still releases a press that reached the page before the host was revoked', async () => { const wc = mockWebContents(100) const revoked = new AbortController() - wc.debugger.sendCommand.mockImplementation( - async (method: string, params?: { type?: string }) => { - if (method === 'Runtime.evaluate') { - return { result: { value: { x: 10, y: 20, adjusted: false, handled: false } } } - } - if (params?.type === 'mousePressed') { - revoked.abort() - } - return {} + wc.debugger.sendCommand.mockImplementation(async (method: string, params?: unknown) => { + if (method === 'Runtime.evaluate') { + return { result: { value: { x: 10, y: 20, adjusted: false, handled: false } } } } - ) + if ( + typeof params === 'object' && + params !== null && + 'type' in params && + params.type === 'mousePressed' + ) { + revoked.abort() + } + return {} + }) webContentsFromIdMock.mockReturnValue(wc) await runInRemoteCliBrowserScope( diff --git a/src/main/browser/agent-input-injection.ts b/src/main/browser/agent-input-injection.ts index 3ae92f7bb989..57e1f3ef18c2 100644 --- a/src/main/browser/agent-input-injection.ts +++ b/src/main/browser/agent-input-injection.ts @@ -1,5 +1,8 @@ import type { WebContents } from 'electron' -import { assertRemoteCliPageAdmitted } from './remote-cli-page-admission' +import { + assertRemoteCliPageAdmitted, + isHeldRemoteCliInputRelease +} from './remote-cli-page-admission' import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' // Why: Orca's guest shortcut and grab handlers act on keys the page receives, and an unhandled key @@ -10,18 +13,24 @@ export function isAgentInjectingInput(webContents: WebContents): boolean { return (injectionDepth.get(webContents) ?? 0) > 0 } +/** The CDP input event being delivered, so a revoked command can still release what it pressed. */ +export type AgentInputEvent = { method: string; params: Record } + /** Refuses agent input that a revoked SSH host's command sends, directly or through its helper. */ -export function assertAgentInputAdmitted(webContents: WebContents): void { - assertRemoteCliBrowserAccess(webContents.id) - assertRemoteCliPageAdmitted(webContents.id) +export function assertAgentInputAdmitted(webContents: WebContents, input?: AgentInputEvent): void { + if (!input || !isHeldRemoteCliInputRelease(webContents.id, input.method, input.params)) { + assertRemoteCliBrowserAccess(webContents.id) + } + assertRemoteCliPageAdmitted(webContents.id, input?.method, input?.params) } /** Marks input sent during `run` as the agent's: Orca shortcuts and app menu items ignore it. */ export async function withAgentInputInjection( webContents: WebContents, - run: () => Promise + run: () => Promise, + input?: AgentInputEvent ): Promise { - assertAgentInputAdmitted(webContents) + assertAgentInputAdmitted(webContents, input) const depth = injectionDepth.get(webContents) ?? 0 injectionDepth.set(webContents, depth + 1) if (depth === 0) { diff --git a/src/main/browser/cdp-page-input-commands.ts b/src/main/browser/cdp-page-input-commands.ts index 89da2e803fd1..57e0bf15fe5d 100644 --- a/src/main/browser/cdp-page-input-commands.ts +++ b/src/main/browser/cdp-page-input-commands.ts @@ -85,8 +85,10 @@ export class CdpPageInputCommands { return await this.debuggerChannel.sendDebuggerCommand(method, params, sessionId) } const send = (): Promise => - withAgentInputInjection(this.webContents, () => - this.debuggerChannel.sendDebuggerCommand(method, params, sessionId) + withAgentInputInjection( + this.webContents, + () => this.debuggerChannel.sendDebuggerCommand(method, params, sessionId), + { method, params } ) const routedByFocus = !sessionId && isFocusRoutedCdpInput(method) if (!routedByFocus && !isFocusTakingCdpInput(method, params)) { diff --git a/src/main/browser/cdp-ws-proxy-host-admission.test.ts b/src/main/browser/cdp-ws-proxy-host-admission.test.ts index 1b9f6246d8f3..2aa79f997f70 100644 --- a/src/main/browser/cdp-ws-proxy-host-admission.test.ts +++ b/src/main/browser/cdp-ws-proxy-host-admission.test.ts @@ -81,27 +81,74 @@ describe('CdpWsProxy under a host command', () => { client.close() }) - it('still delivers a release after the revocation, so no button stays held on the page', async () => { + function holdPage(): { revoke: () => void; release: () => Promise } { + let revoked = false let finish = (): void => {} const held = runWithRemoteCliPageAdmission( PAGE_ID, () => { - throw new Error(REVOKED) + if (revoked) { + throw new Error(REVOKED) + } }, () => new Promise((resolve) => (finish = resolve)) ) + return { + revoke: () => (revoked = true), + release: async () => { + finish() + await held + } + } + } + + const MOUSE = 'Input.dispatchMouseEvent' + const KEY = 'Input.dispatchKeyEvent' + const pressed = { type: 'mousePressed', x: 1, y: 1, button: 'left', clickCount: 1 } + const released = { type: 'mouseReleased', x: 1, y: 1, button: 'left', clickCount: 1 } + const shiftDown = { type: 'rawKeyDown', key: 'Shift', code: 'ShiftLeft', modifiers: 8 } + const shiftUp = { type: 'keyUp', key: 'Shift', code: 'ShiftLeft' } + + it.each(['window', 'webview'])( + 'still releases a button and key pressed before the revocation (%s page)', + async (pageType) => { + mock.webContents.getType.mockReturnValue(pageType) + const page = holdPage() + const client = await connect(endpoint) + + await sendAndReceive(client, { id: 5, method: MOUSE, params: pressed }) + await sendAndReceive(client, { id: 6, method: KEY, params: shiftDown }) + page.revoke() + const mouseUp = await sendAndReceive(client, { id: 7, method: MOUSE, params: released }) + const keyUp = await sendAndReceive(client, { id: 8, method: KEY, params: shiftUp }) + + expect(mouseUp.error).toBeUndefined() + expect(keyUp.error).toBeUndefined() + expect(mouseEvents(mock)).toEqual(['mousePressed', 'mouseReleased']) + const keyUps = [ + ...mock.webContents.sendInputEvent.mock.calls.map((call) => call[0].type), + ...mock.webContents.debugger.sendCommand.mock.calls + .filter((call) => call[0] === KEY) + .map((call) => String(call[1]?.type)) + ].filter((type) => type === 'keyUp') + expect(keyUps).toHaveLength(1) + await page.release() + client.close() + } + ) + + it('refuses a release of something the revoked command never pressed', async () => { + const page = holdPage() const client = await connect(endpoint) + page.revoke() - const response = await sendAndReceive(client, { - id: 5, - method: 'Input.dispatchMouseEvent', - params: { type: 'mouseReleased', x: 1, y: 1, button: 'left', clickCount: 1 } - }) + const mouseUp = await sendAndReceive(client, { id: 9, method: MOUSE, params: released }) + const keyUp = await sendAndReceive(client, { id: 10, method: KEY, params: shiftUp }) - expect(response.error).toBeUndefined() - expect(mouseEvents(mock)).toEqual(['mouseReleased']) - finish() - await held + expect(mouseUp).toMatchObject({ error: { message: REVOKED } }) + expect(keyUp).toMatchObject({ error: { message: REVOKED } }) + expect(mouseEvents(mock)).toEqual([]) + await page.release() client.close() }) diff --git a/src/main/browser/guest-cdp-command.ts b/src/main/browser/guest-cdp-command.ts index 4910c89591e2..482b29fc8adc 100644 --- a/src/main/browser/guest-cdp-command.ts +++ b/src/main/browser/guest-cdp-command.ts @@ -1,7 +1,10 @@ import type { WebContents } from 'electron' import { BrowserError } from './browser-error' -import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' -import { isInputReleaseCommand } from './remote-cli-page-admission' +import { + assertRemoteCliBrowserAccess, + captureRemoteCliBrowserScope +} from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' +import { isHeldRemoteCliInputRelease, noteRemoteCliInputPress } from './remote-cli-page-admission' // Why: Chromium resizes the page's view for these without checking the view still exists // (WebContentsImpl::SetDeviceEmulationSize). A crashed renderer takes its view with it until the @@ -29,9 +32,12 @@ export function sendGuestCdpCommand( ): Promise { try { // Why: only a host's own command runs in its scope; desktop-owned sends are never refused here. - if (!isInputReleaseCommand(method, params)) { + if (!isHeldRemoteCliInputRelease(guest.id, method, params)) { assertRemoteCliBrowserAccess(guest.id) } + if (captureRemoteCliBrowserScope().bridged) { + noteRemoteCliInputPress(guest.id, method, params) + } } catch (error) { return Promise.reject(error) } diff --git a/src/main/browser/guest-direct-key-input.test.ts b/src/main/browser/guest-direct-key-input.test.ts index 1c14b08905cf..6fdf71ebd871 100644 --- a/src/main/browser/guest-direct-key-input.test.ts +++ b/src/main/browser/guest-direct-key-input.test.ts @@ -2,11 +2,10 @@ import { describe, expect, it, vi } from 'vitest' import type { WebContents } from 'electron' import { dispatchPageKeyEvent, - planCdpKeyEvent, - planInsertText, routePageTextInsertion, type GuestCdpSender } from './guest-direct-key-input' +import { planCdpKeyEvent, planInsertText } from './guest-key-event-plan' function fakePage(type: string) { const page = { diff --git a/src/main/browser/guest-direct-key-input.ts b/src/main/browser/guest-direct-key-input.ts index 1bc01444cdfd..b16d03df9469 100644 --- a/src/main/browser/guest-direct-key-input.ts +++ b/src/main/browser/guest-direct-key-input.ts @@ -1,4 +1,4 @@ -import type { InputEvent, KeyboardInputEvent, WebContents } from 'electron' +import type { WebContents } from 'electron' import { enablePageFocusEmulation, type AgentInputFocusGuard } from './agent-input-focus-guard' import { assertAgentInputAdmitted, withAgentInputInjection } from './agent-input-injection' import { @@ -7,179 +7,21 @@ import { type GuestSessionCdpSender } from './guest-focused-frame-route' import { sendFrameSessionInput } from './guest-frame-input-sessions' +import { planCdpKeyEvent, planInsertText, type GuestEditCommand } from './guest-key-event-plan' // Why: Chromium routes main-session CDP key and text input on a guest by the // window's focus, so it lands in whatever the user is typing into (#20595). Events sent to // the guest's own widget reach its page without moving focus. // See docs/reference/agent-browser-focus-isolation.md. -type ElectronModifier = NonNullable[number] - -// Why: Electron honours this NativeWebKeyboardEvent flag although its typings omit it; an -// unhandled agent key must not be redispatched to Orca's window or application menu. -type GuestKeyboardEvent = KeyboardInputEvent & { skipIfUnhandled: true } - -type GuestEditCommand = 'selectAll' | 'copy' | 'cut' | 'paste' | 'undo' | 'redo' | 'delete' - -type GuestKeyPlan = { - events: GuestKeyboardEvent[] - // Why: macOS editing shortcuts arrive as CDP `commands`; widget key events skip the - // browser-side key bindings that would run them. - commands: GuestEditCommand[] -} - export type GuestCdpSender = GuestSessionCdpSender -const CDP_MODIFIER_NAMES: [number, ElectronModifier][] = [ - [1, 'alt'], - [2, 'control'], - [4, 'meta'], - [8, 'shift'] -] - -// CDP `key` -> Electron accelerator key code, for keys whose names differ or are not single characters. -const ELECTRON_KEY_CODES: Record = { - Enter: 'Enter', - Tab: 'Tab', - Backspace: 'Backspace', - Delete: 'Delete', - Escape: 'Escape', - ' ': 'Space', - ArrowUp: 'Up', - ArrowDown: 'Down', - ArrowLeft: 'Left', - ArrowRight: 'Right', - Home: 'Home', - End: 'End', - PageUp: 'PageUp', - PageDown: 'PageDown', - Insert: 'Insert', - CapsLock: 'CapsLock', - NumLock: 'NumLock', - ScrollLock: 'ScrollLock', - PrintScreen: 'PrintScreen', - Shift: 'Shift', - Control: 'Control', - Alt: 'Alt', - Meta: 'Meta' -} - -const GUEST_EDIT_COMMANDS: ReadonlySet = new Set([ - 'selectAll', - 'copy', - 'cut', - 'paste', - 'undo', - 'redo', - 'delete' -]) - -function isGuestEditCommand(command: unknown): command is GuestEditCommand { - return typeof command === 'string' && GUEST_EDIT_COMMANDS.has(command) -} - // Why: getType() is 'webview' only for guests hosted in an Orca window; offscreen and // headless pages have no host frame tree, so CDP input already reaches them. export function isEmbeddedGuest(webContents: WebContents): boolean { return webContents.getType() === 'webview' } -function electronKeyCode(key: unknown): string | null { - if (typeof key !== 'string') { - return null - } - if (Object.hasOwn(ELECTRON_KEY_CODES, key)) { - return ELECTRON_KEY_CODES[key] - } - if (/^F([1-9]|1\d|2[0-4])$/.test(key)) { - return key - } - // Why: Electron derives key/code from a US layout, which covers printable ASCII only. - return key.length === 1 && key >= '!' && key <= '~' ? key : null -} - -function electronModifiers(mask: unknown): ElectronModifier[] { - const bits = typeof mask === 'number' ? mask : 0 - return CDP_MODIFIER_NAMES.filter(([bit]) => (bits & bit) !== 0).map(([, name]) => name) -} - -// Why: Blink drops control characters from char events ("a\nb" types "ab"), so text that -// carries them cannot be typed this way and must take the fallback. -function typeableText(text: unknown): text is string { - return ( - typeof text === 'string' && - text.length > 0 && - Array.from(text).every((char) => { - const code = char.charCodeAt(0) - return code >= 0x20 && code !== 0x7f - }) - ) -} - -function charEvents(text: string, modifiers: ElectronModifier[]): GuestKeyboardEvent[] { - return Array.from(text, (char) => ({ - type: 'char', - keyCode: char, - modifiers, - skipIfUnhandled: true - })) -} - -/** Translates one CDP Input.dispatchKeyEvent into guest widget events, or null when it cannot. */ -export function planCdpKeyEvent(params: Record): GuestKeyPlan | null { - const commandsParam = params.commands ?? [] - if (!Array.isArray(commandsParam) || !commandsParam.every(isGuestEditCommand)) { - return null - } - const commands = commandsParam - const modifiers = electronModifiers(params.modifiers) - const { type, text } = params - if (type === 'char') { - return typeableText(text) ? { events: charEvents(text, modifiers), commands } : null - } - if (type !== 'keyDown' && type !== 'rawKeyDown' && type !== 'keyUp') { - return null - } - const keyCode = electronKeyCode(params.key) - // Why: a named key's text is a control character (Tab "\t", Backspace "\b") that the key - // event already acts on; only Enter's "\r" char is needed, to submit forms and insert newlines. - const isNamedKey = - typeof params.key === 'string' && - params.key !== ' ' && - Object.hasOwn(ELECTRON_KEY_CODES, params.key) - if (type === 'keyDown' && !isNamedKey && typeof text === 'string' && text !== '') { - if (!typeableText(text)) { - return null - } - // Why: a non-US-layout character has no Electron key code; the char event alone types it. - const down: GuestKeyboardEvent[] = - keyCode === null ? [] : [{ type: 'keyDown', keyCode, modifiers, skipIfUnhandled: true }] - return { events: [...down, ...charEvents(text, modifiers)], commands } - } - if (keyCode === null) { - // Why: a character outside the US layout went down as a char event alone; its keyUp has no - // widget form, and the fallback would focus the guest just to release a key never pressed. - const isCharacterKey = typeof params.key === 'string' && Array.from(params.key).length === 1 - return type === 'keyUp' && isCharacterKey ? { events: [], commands } : null - } - const event: GuestKeyboardEvent = { - type: type === 'keyUp' ? 'keyUp' : 'keyDown', - keyCode, - modifiers, - skipIfUnhandled: true - } - const enterChar: GuestKeyboardEvent[] = - type === 'keyDown' && params.key === 'Enter' && (text === '\r' || text === '\n') - ? [{ type: 'char', keyCode: '\r', modifiers, skipIfUnhandled: true }] - : [] - return { events: [event, ...enterChar], commands } -} - -/** Translates CDP Input.insertText into guest widget char events, or null when it cannot. */ -export function planInsertText(text: unknown): GuestKeyboardEvent[] | null { - return typeableText(text) ? charEvents(text, []) : null -} - // Why: sendInputEvent returns before the page handles the event, but CDP input replies only after. // A task-queue round trip in the page lets callers read the result of what they just typed. async function waitForPageToHandleInput(send: GuestCdpSender): Promise { @@ -244,8 +86,10 @@ async function sendToFrameSession( method: string, params: Record ): Promise { - return await withAgentInputInjection(webContents, () => - sendFrameSessionInput(webContents, send, sessionId, method, params) + return await withAgentInputInjection( + webContents, + () => sendFrameSessionInput(webContents, send, sessionId, method, params), + { method, params } ) } @@ -262,6 +106,7 @@ export async function dispatchPageKeyEvent( if (!isEmbeddedGuest(webContents)) { return await send('Input.dispatchKeyEvent', params) } + const keyInput = { method: 'Input.dispatchKeyEvent', params } const route = await resolveFocusedFrameRoute(webContents, send) if (route.kind === 'session') { // Why: Tab from a frame's last field can move focus out of the guest into Orca's window. @@ -287,17 +132,21 @@ export async function dispatchPageKeyEvent( ? await guardFocus() : () => {} try { - await withAgentInputInjection(webContents, async () => { - for (const event of plan.events) { - webContents.sendInputEvent(event) - } - if (plan.commands.length > 0 || plan.events.length > 0) { - await waitForPageToHandleInput(send) - } - for (const command of plan.commands) { - runEditCommand(webContents, command) - } - }) + await withAgentInputInjection( + webContents, + async () => { + for (const event of plan.events) { + webContents.sendInputEvent(event) + } + if (plan.commands.length > 0 || plan.events.length > 0) { + await waitForPageToHandleInput(send) + } + for (const command of plan.commands) { + runEditCommand(webContents, command) + } + }, + keyInput + ) } finally { release() } @@ -305,7 +154,11 @@ export async function dispatchPageKeyEvent( } const release = await focusGuestForCdpInput(webContents, send, guardFocus) try { - return await withAgentInputInjection(webContents, () => send('Input.dispatchKeyEvent', params)) + return await withAgentInputInjection( + webContents, + () => send('Input.dispatchKeyEvent', params), + keyInput + ) } finally { release() } diff --git a/src/main/browser/guest-key-event-plan.ts b/src/main/browser/guest-key-event-plan.ts new file mode 100644 index 000000000000..32fe6f2036a3 --- /dev/null +++ b/src/main/browser/guest-key-event-plan.ts @@ -0,0 +1,162 @@ +import type { InputEvent, KeyboardInputEvent } from 'electron' + +/** Translates CDP key and text input into Electron guest widget events (#20595). */ + +type ElectronModifier = NonNullable[number] + +// Why: Electron honours this NativeWebKeyboardEvent flag although its typings omit it; an +// unhandled agent key must not be redispatched to Orca's window or application menu. +export type GuestKeyboardEvent = KeyboardInputEvent & { skipIfUnhandled: true } + +export type GuestEditCommand = 'selectAll' | 'copy' | 'cut' | 'paste' | 'undo' | 'redo' | 'delete' + +type GuestKeyPlan = { + events: GuestKeyboardEvent[] + // Why: macOS editing shortcuts arrive as CDP `commands`; widget key events skip the + // browser-side key bindings that would run them. + commands: GuestEditCommand[] +} + +const CDP_MODIFIER_NAMES: [number, ElectronModifier][] = [ + [1, 'alt'], + [2, 'control'], + [4, 'meta'], + [8, 'shift'] +] + +// CDP `key` -> Electron accelerator key code, for keys whose names differ or are not single characters. +const ELECTRON_KEY_CODES: Record = { + Enter: 'Enter', + Tab: 'Tab', + Backspace: 'Backspace', + Delete: 'Delete', + Escape: 'Escape', + ' ': 'Space', + ArrowUp: 'Up', + ArrowDown: 'Down', + ArrowLeft: 'Left', + ArrowRight: 'Right', + Home: 'Home', + End: 'End', + PageUp: 'PageUp', + PageDown: 'PageDown', + Insert: 'Insert', + CapsLock: 'CapsLock', + NumLock: 'NumLock', + ScrollLock: 'ScrollLock', + PrintScreen: 'PrintScreen', + Shift: 'Shift', + Control: 'Control', + Alt: 'Alt', + Meta: 'Meta' +} + +const GUEST_EDIT_COMMANDS: ReadonlySet = new Set([ + 'selectAll', + 'copy', + 'cut', + 'paste', + 'undo', + 'redo', + 'delete' +]) + +function isGuestEditCommand(command: unknown): command is GuestEditCommand { + return typeof command === 'string' && GUEST_EDIT_COMMANDS.has(command) +} + +function electronKeyCode(key: unknown): string | null { + if (typeof key !== 'string') { + return null + } + if (Object.hasOwn(ELECTRON_KEY_CODES, key)) { + return ELECTRON_KEY_CODES[key] + } + if (/^F([1-9]|1\d|2[0-4])$/.test(key)) { + return key + } + // Why: Electron derives key/code from a US layout, which covers printable ASCII only. + return key.length === 1 && key >= '!' && key <= '~' ? key : null +} + +function electronModifiers(mask: unknown): ElectronModifier[] { + const bits = typeof mask === 'number' ? mask : 0 + return CDP_MODIFIER_NAMES.filter(([bit]) => (bits & bit) !== 0).map(([, name]) => name) +} + +// Why: Blink drops control characters from char events ("a\nb" types "ab"), so text that +// carries them cannot be typed this way and must take the fallback. +function typeableText(text: unknown): text is string { + return ( + typeof text === 'string' && + text.length > 0 && + Array.from(text).every((char) => { + const code = char.charCodeAt(0) + return code >= 0x20 && code !== 0x7f + }) + ) +} + +function charEvents(text: string, modifiers: ElectronModifier[]): GuestKeyboardEvent[] { + return Array.from(text, (char) => ({ + type: 'char', + keyCode: char, + modifiers, + skipIfUnhandled: true + })) +} + +/** Translates one CDP Input.dispatchKeyEvent into guest widget events, or null when it cannot. */ +export function planCdpKeyEvent(params: Record): GuestKeyPlan | null { + const commandsParam = params.commands ?? [] + if (!Array.isArray(commandsParam) || !commandsParam.every(isGuestEditCommand)) { + return null + } + const commands = commandsParam + const modifiers = electronModifiers(params.modifiers) + const { type, text } = params + if (type === 'char') { + return typeableText(text) ? { events: charEvents(text, modifiers), commands } : null + } + if (type !== 'keyDown' && type !== 'rawKeyDown' && type !== 'keyUp') { + return null + } + const keyCode = electronKeyCode(params.key) + // Why: a named key's text is a control character (Tab "\t", Backspace "\b") that the key + // event already acts on; only Enter's "\r" char is needed, to submit forms and insert newlines. + const isNamedKey = + typeof params.key === 'string' && + params.key !== ' ' && + Object.hasOwn(ELECTRON_KEY_CODES, params.key) + if (type === 'keyDown' && !isNamedKey && typeof text === 'string' && text !== '') { + if (!typeableText(text)) { + return null + } + // Why: a non-US-layout character has no Electron key code; the char event alone types it. + const down: GuestKeyboardEvent[] = + keyCode === null ? [] : [{ type: 'keyDown', keyCode, modifiers, skipIfUnhandled: true }] + return { events: [...down, ...charEvents(text, modifiers)], commands } + } + if (keyCode === null) { + // Why: a character outside the US layout went down as a char event alone; its keyUp has no + // widget form, and the fallback would focus the guest just to release a key never pressed. + const isCharacterKey = typeof params.key === 'string' && Array.from(params.key).length === 1 + return type === 'keyUp' && isCharacterKey ? { events: [], commands } : null + } + const event: GuestKeyboardEvent = { + type: type === 'keyUp' ? 'keyUp' : 'keyDown', + keyCode, + modifiers, + skipIfUnhandled: true + } + const enterChar: GuestKeyboardEvent[] = + type === 'keyDown' && params.key === 'Enter' && (text === '\r' || text === '\n') + ? [{ type: 'char', keyCode: '\r', modifiers, skipIfUnhandled: true }] + : [] + return { events: [event, ...enterChar], commands } +} + +/** Translates CDP Input.insertText into guest widget char events, or null when it cannot. */ +export function planInsertText(text: unknown): GuestKeyboardEvent[] | null { + return typeableText(text) ? charEvents(text, []) : null +} diff --git a/src/main/browser/remote-cli-page-admission.ts b/src/main/browser/remote-cli-page-admission.ts index e5ce281e4ef2..9d3e5a6e8984 100644 --- a/src/main/browser/remote-cli-page-admission.ts +++ b/src/main/browser/remote-cli-page-admission.ts @@ -4,45 +4,99 @@ * edit commands) consults it, so no queued or deferred step of a running command outlives the * revocation. */ -const admissions = new Map void>() +type Admission = { + admit: () => void + /** Buttons, keys and touches this command pressed; only their release outlives a revocation. */ + held: Set +} + +const admissions = new Map() export async function runWithRemoteCliPageAdmission( webContentsId: number, admit: () => void, run: () => Promise ): Promise { - admissions.set(webContentsId, admit) + const admission: Admission = { admit, held: new Set() } + admissions.set(webContentsId, admission) try { return await run() } finally { - if (admissions.get(webContentsId) === admit) { + if (admissions.get(webContentsId) === admission) { admissions.delete(webContentsId) } } } -/** Throws while a revoked host's command still holds this page; a no-op otherwise. */ +/** + * Throws while a revoked host's command still holds this page; a no-op otherwise. Pass the input + * being sent so a release of what this command pressed still lands instead of leaving it held. + */ export function assertRemoteCliPageAdmitted( webContentsId: number | undefined, method?: string, params?: Record ): void { - if (webContentsId !== undefined && !isInputReleaseCommand(method, params)) { - admissions.get(webContentsId)?.() + const admission = webContentsId === undefined ? undefined : admissions.get(webContentsId) + if (!admission) { + return + } + const input = describeInput(method, params) + if (input && !input.press && admission.held.has(input.id)) { + return + } + admission.admit() + if (input?.press) { + admission.held.add(input.id) + } +} + +/** True for a release of something the command now holding this page pressed. */ +export function isHeldRemoteCliInputRelease( + webContentsId: number | undefined, + method: string, + params?: Record +): boolean { + const input = describeInput(method, params) + return ( + input !== null && + !input.press && + webContentsId !== undefined && + (admissions.get(webContentsId)?.held.has(input.id) ?? false) + ) +} + +/** Records a press the command holding this page sent outside the admission gates. */ +export function noteRemoteCliInputPress( + webContentsId: number | undefined, + method: string, + params?: Record +): void { + const input = describeInput(method, params) + if (input?.press && webContentsId !== undefined) { + admissions.get(webContentsId)?.held.add(input.id) } } -/** Ends input a command already began; refusing it would leave a button or key held on the page. */ -export function isInputReleaseCommand(method?: string, params?: Record): boolean { +function describeInput( + method: string | undefined, + params: Record | undefined +): { id: string; press: boolean } | null { const type = params?.type switch (method) { case 'Input.dispatchMouseEvent': - return type === 'mouseReleased' + return type === 'mousePressed' || type === 'mouseReleased' + ? { id: `mouse:${String(params?.button ?? 'left')}`, press: type === 'mousePressed' } + : null case 'Input.dispatchKeyEvent': - return type === 'keyUp' + return type === 'keyDown' || type === 'rawKeyDown' || type === 'keyUp' + ? { id: `key:${String(params?.code ?? params?.key ?? '')}`, press: type !== 'keyUp' } + : null case 'Input.dispatchTouchEvent': - return type === 'touchEnd' || type === 'touchCancel' + return type === 'touchStart' || type === 'touchEnd' || type === 'touchCancel' + ? { id: 'touch', press: type === 'touchStart' } + : null default: - return false + return null } } From 9046357e7205660a64216f0ea2df9faa11d8a86f Mon Sep 17 00:00:00 2001 From: Neil Date: Mon, 19 Oct 2026 00:05:17 -0700 Subject: [PATCH 08/11] fix(browser): refuse every revoked step, and let the desktop release what a revoked command left held --- ...ent-browser-bridge-host-revocation.test.ts | 11 +- .../browser/agent-browser-bridge-queue.ts | 6 +- src/main/browser/agent-input-injection.ts | 21 ++- src/main/browser/cdp-debugger-channel.ts | 2 +- .../cdp-ws-proxy-host-admission.test.ts | 50 ++++-- src/main/browser/cdp-ws-proxy.ts | 2 +- src/main/browser/guest-cdp-command.ts | 15 +- src/main/browser/guest-direct-key-input.ts | 14 ++ src/main/browser/remote-cli-page-admission.ts | 147 +++++++++++------- 9 files changed, 167 insertions(+), 101 deletions(-) diff --git a/src/main/browser/agent-browser-bridge-host-revocation.test.ts b/src/main/browser/agent-browser-bridge-host-revocation.test.ts index e00a012433b4..5f12d82d5498 100644 --- a/src/main/browser/agent-browser-bridge-host-revocation.test.ts +++ b/src/main/browser/agent-browser-bridge-host-revocation.test.ts @@ -210,7 +210,7 @@ describe('AgentBrowserBridge under a host revocation', () => { expect(release).toHaveBeenCalled() }) - it('still releases a press that reached the page before the host was revoked', async () => { + it('lets go off the page of a press that landed just before the host was revoked', async () => { const wc = mockWebContents(100) const revoked = new AbortController() wc.debugger.sendCommand.mockImplementation(async (method: string, params?: unknown) => { @@ -229,15 +229,16 @@ describe('AgentBrowserBridge under a host revocation', () => { }) webContentsFromIdMock.mockReturnValue(wc) - await runInRemoteCliBrowserScope( + const click = runInRemoteCliBrowserScope( WORKSPACE, () => bridge.mouseClick(10, 20, 'left', undefined, 'tab-1', 18), revoked.signal - ).catch(() => {}) + ) + await expect(click).rejects.toMatchObject({ code: 'desktop_browser_bridge_unavailable' }) expect(dispatchedMouseEvents(wc)).toEqual([ - ['Input.dispatchMouseEvent', expect.objectContaining({ type: 'mousePressed' })], - ['Input.dispatchMouseEvent', expect.objectContaining({ type: 'mouseReleased' })] + ['Input.dispatchMouseEvent', expect.objectContaining({ type: 'mousePressed', x: 10 })], + ['Input.dispatchMouseEvent', expect.objectContaining({ type: 'mouseReleased', x: -1, y: -1 })] ]) }) diff --git a/src/main/browser/agent-browser-bridge-queue.ts b/src/main/browser/agent-browser-bridge-queue.ts index e4aa8869dc6e..719eb43ad70a 100644 --- a/src/main/browser/agent-browser-bridge-queue.ts +++ b/src/main/browser/agent-browser-bridge-queue.ts @@ -7,6 +7,7 @@ import type { ResolvedBrowserCommandTarget } from './agent-browser-bridge-types' import { runWithRemoteCliPageAdmission } from './remote-cli-page-admission' +import { releaseHeldPageInput } from './guest-direct-key-input' import { captureRemoteCliBrowserRevocation, captureRemoteCliBrowserScope @@ -131,7 +132,10 @@ export abstract class AgentBrowserBridgeQueue extends AgentBrowserBridgeShutdown execute: () => Promise ): Promise { try { - return await runWithRemoteCliPageAdmission(webContentsId, gate, execute) + return await runWithRemoteCliPageAdmission(webContentsId, gate, execute, (release) => { + const wc = this.getWebContents(webContentsId) + return wc ? releaseHeldPageInput(wc, release) : Promise.resolve() + }) } catch (error) { gate() throw error diff --git a/src/main/browser/agent-input-injection.ts b/src/main/browser/agent-input-injection.ts index 57e1f3ef18c2..9f5783e1d4c0 100644 --- a/src/main/browser/agent-input-injection.ts +++ b/src/main/browser/agent-input-injection.ts @@ -1,8 +1,5 @@ import type { WebContents } from 'electron' -import { - assertRemoteCliPageAdmitted, - isHeldRemoteCliInputRelease -} from './remote-cli-page-admission' +import { assertRemoteCliPageAdmitted, noteRemoteCliInput } from './remote-cli-page-admission' import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' // Why: Orca's guest shortcut and grab handlers act on keys the page receives, and an unhandled key @@ -13,15 +10,13 @@ export function isAgentInjectingInput(webContents: WebContents): boolean { return (injectionDepth.get(webContents) ?? 0) > 0 } -/** The CDP input event being delivered, so a revoked command can still release what it pressed. */ +/** The CDP input event being delivered, so a revocation can release what a host pressed. */ export type AgentInputEvent = { method: string; params: Record } /** Refuses agent input that a revoked SSH host's command sends, directly or through its helper. */ -export function assertAgentInputAdmitted(webContents: WebContents, input?: AgentInputEvent): void { - if (!input || !isHeldRemoteCliInputRelease(webContents.id, input.method, input.params)) { - assertRemoteCliBrowserAccess(webContents.id) - } - assertRemoteCliPageAdmitted(webContents.id, input?.method, input?.params) +export function assertAgentInputAdmitted(webContents: WebContents): void { + assertRemoteCliBrowserAccess(webContents.id) + assertRemoteCliPageAdmitted(webContents.id) } /** Marks input sent during `run` as the agent's: Orca shortcuts and app menu items ignore it. */ @@ -30,7 +25,11 @@ export async function withAgentInputInjection( run: () => Promise, input?: AgentInputEvent ): Promise { - assertAgentInputAdmitted(webContents, input) + assertAgentInputAdmitted(webContents) + if (input) { + // Why before run: widget events land synchronously and nothing else records them. + noteRemoteCliInput(webContents.id, input.method, input.params) + } const depth = injectionDepth.get(webContents) ?? 0 injectionDepth.set(webContents, depth + 1) if (depth === 0) { diff --git a/src/main/browser/cdp-debugger-channel.ts b/src/main/browser/cdp-debugger-channel.ts index 0f25009fa1e6..82da834ca1db 100644 --- a/src/main/browser/cdp-debugger-channel.ts +++ b/src/main/browser/cdp-debugger-channel.ts @@ -96,7 +96,7 @@ export class CdpDebuggerChannel { ): Promise { // Why: the proxy's queued input outlives the message's admission; a revoked host's must not land. try { - assertRemoteCliPageAdmitted(this.webContents.id, method, params) + assertRemoteCliPageAdmitted(this.webContents.id) } catch (error) { return Promise.reject(error) } diff --git a/src/main/browser/cdp-ws-proxy-host-admission.test.ts b/src/main/browser/cdp-ws-proxy-host-admission.test.ts index 2aa79f997f70..c8ed4b4aeee1 100644 --- a/src/main/browser/cdp-ws-proxy-host-admission.test.ts +++ b/src/main/browser/cdp-ws-proxy-host-admission.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' import { CdpWsProxy } from './cdp-ws-proxy' import { runWithRemoteCliPageAdmission } from './remote-cli-page-admission' import { sendGuestCdpCommand } from './guest-cdp-command' +import { releaseHeldPageInput } from './guest-direct-key-input' import { connect, createMockWebContents, @@ -17,6 +18,15 @@ vi.mock('electron', () => ({ const PAGE_ID = 7 const REVOKED = 'The Orca desktop stopped sharing its browser before this command ran.' +function keyUps(mock: MockWebContents): string[] { + return [ + ...mock.webContents.sendInputEvent.mock.calls.map((call) => call[0].type), + ...mock.webContents.debugger.sendCommand.mock.calls + .filter((call) => call[0] === 'Input.dispatchKeyEvent') + .map((call) => String(call[1]?.type)) + ].filter((type) => type === 'keyUp') +} + function mouseEvents(mock: MockWebContents): string[] { return mock.webContents.debugger.sendCommand.mock.calls .filter((call) => call[0] === 'Input.dispatchMouseEvent') @@ -91,7 +101,9 @@ describe('CdpWsProxy under a host command', () => { throw new Error(REVOKED) } }, - () => new Promise((resolve) => (finish = resolve)) + () => new Promise((resolve) => (finish = resolve)), + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the mock implements every WebContents member the release path calls. + (release) => releaseHeldPageInput({ ...mock.webContents, id: PAGE_ID } as never, release) ) return { revoke: () => (revoked = true), @@ -110,7 +122,7 @@ describe('CdpWsProxy under a host command', () => { const shiftUp = { type: 'keyUp', key: 'Shift', code: 'ShiftLeft' } it.each(['window', 'webview'])( - 'still releases a button and key pressed before the revocation (%s page)', + 'refuses a revoked release, then lets go of the button and key itself (%s page)', async (pageType) => { mock.webContents.getType.mockReturnValue(pageType) const page = holdPage() @@ -122,21 +134,35 @@ describe('CdpWsProxy under a host command', () => { const mouseUp = await sendAndReceive(client, { id: 7, method: MOUSE, params: released }) const keyUp = await sendAndReceive(client, { id: 8, method: KEY, params: shiftUp }) - expect(mouseUp.error).toBeUndefined() - expect(keyUp.error).toBeUndefined() - expect(mouseEvents(mock)).toEqual(['mousePressed', 'mouseReleased']) - const keyUps = [ - ...mock.webContents.sendInputEvent.mock.calls.map((call) => call[0].type), - ...mock.webContents.debugger.sendCommand.mock.calls - .filter((call) => call[0] === KEY) - .map((call) => String(call[1]?.type)) - ].filter((type) => type === 'keyUp') - expect(keyUps).toHaveLength(1) + expect(mouseUp).toMatchObject({ error: { message: REVOKED } }) + expect(keyUp).toMatchObject({ error: { message: REVOKED } }) + expect(mouseEvents(mock)).toEqual(['mousePressed']) + expect(keyUps(mock)).toHaveLength(0) await page.release() + // Off the page, so letting go completes no click the host had started. + expect(mock.webContents.debugger.sendCommand).toHaveBeenCalledWith( + MOUSE, + expect.objectContaining({ type: 'mouseReleased', x: -1, y: -1 }) + ) + expect(mouseEvents(mock)).toEqual(['mousePressed', 'mouseReleased']) + expect(keyUps(mock)).toHaveLength(1) client.close() } ) + it('lets go of nothing when the command released what it pressed before the revocation', async () => { + const page = holdPage() + const client = await connect(endpoint) + + await sendAndReceive(client, { id: 11, method: MOUSE, params: pressed }) + await sendAndReceive(client, { id: 12, method: MOUSE, params: released }) + page.revoke() + await page.release() + + expect(mouseEvents(mock)).toEqual(['mousePressed', 'mouseReleased']) + client.close() + }) + it('refuses a release of something the revoked command never pressed', async () => { const page = holdPage() const client = await connect(endpoint) diff --git a/src/main/browser/cdp-ws-proxy.ts b/src/main/browser/cdp-ws-proxy.ts index abd3c1d738f6..79c4dd63f7c8 100644 --- a/src/main/browser/cdp-ws-proxy.ts +++ b/src/main/browser/cdp-ws-proxy.ts @@ -181,7 +181,7 @@ export class CdpWsProxy { this.responder.recordRequestSessionId(client, clientId, msg) // Why here too: a revoked host's helper is refused even its read-only steps, so it stops at once. try { - assertRemoteCliPageAdmitted(this.webContents.id, msg.method, msg.params) + assertRemoteCliPageAdmitted(this.webContents.id) } catch (error) { this.responder.sendError( clientId, diff --git a/src/main/browser/guest-cdp-command.ts b/src/main/browser/guest-cdp-command.ts index 482b29fc8adc..cee48121a8a2 100644 --- a/src/main/browser/guest-cdp-command.ts +++ b/src/main/browser/guest-cdp-command.ts @@ -1,10 +1,7 @@ import type { WebContents } from 'electron' import { BrowserError } from './browser-error' -import { - assertRemoteCliBrowserAccess, - captureRemoteCliBrowserScope -} from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' -import { isHeldRemoteCliInputRelease, noteRemoteCliInputPress } from './remote-cli-page-admission' +import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' +import { noteRemoteCliInput } from './remote-cli-page-admission' // Why: Chromium resizes the page's view for these without checking the view still exists // (WebContentsImpl::SetDeviceEmulationSize). A crashed renderer takes its view with it until the @@ -32,12 +29,7 @@ export function sendGuestCdpCommand( ): Promise { try { // Why: only a host's own command runs in its scope; desktop-owned sends are never refused here. - if (!isHeldRemoteCliInputRelease(guest.id, method, params)) { - assertRemoteCliBrowserAccess(guest.id) - } - if (captureRemoteCliBrowserScope().bridged) { - noteRemoteCliInputPress(guest.id, method, params) - } + assertRemoteCliBrowserAccess(guest.id) } catch (error) { return Promise.reject(error) } @@ -51,6 +43,7 @@ export function sendGuestCdpCommand( ) ) } + noteRemoteCliInput(guest.id, method, params) return Promise.resolve( sessionId === undefined ? guest.debugger.sendCommand(method, params) diff --git a/src/main/browser/guest-direct-key-input.ts b/src/main/browser/guest-direct-key-input.ts index b16d03df9469..b2f93c0094f2 100644 --- a/src/main/browser/guest-direct-key-input.ts +++ b/src/main/browser/guest-direct-key-input.ts @@ -8,6 +8,8 @@ import { } from './guest-focused-frame-route' import { sendFrameSessionInput } from './guest-frame-input-sessions' import { planCdpKeyEvent, planInsertText, type GuestEditCommand } from './guest-key-event-plan' +import { sendGuestCdpCommand } from './guest-cdp-command' +import type { HeldInputRelease } from './remote-cli-page-admission' // Why: Chromium routes main-session CDP key and text input on a guest by the // window's focus, so it lands in whatever the user is typing into (#20595). Events sent to @@ -164,6 +166,18 @@ export async function dispatchPageKeyEvent( } } +/** Lets go of what a revoked host's command left pressed; a key goes where the focus routes it. */ +export function releaseHeldPageInput( + webContents: WebContents, + release: HeldInputRelease +): Promise { + const send: GuestCdpSender = (method, params, sessionId) => + sendGuestCdpCommand(webContents, method, params, sessionId) + return release.method === 'Input.dispatchKeyEvent' + ? dispatchPageKeyEvent(webContents, release.params, send) + : send(release.method, release.params) +} + export type TextInsertionRoute = { deliver: (params: Record) => Promise // Why: the fallback holds borrowed focus from routing until delivery; an insert abandoned in diff --git a/src/main/browser/remote-cli-page-admission.ts b/src/main/browser/remote-cli-page-admission.ts index 9d3e5a6e8984..cecf2079b884 100644 --- a/src/main/browser/remote-cli-page-admission.ts +++ b/src/main/browser/remote-cli-page-admission.ts @@ -4,20 +4,31 @@ * edit commands) consults it, so no queued or deferred step of a running command outlives the * revocation. */ +import { runOutsideRemoteCliBrowserScope } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' + +/** The CDP input event that lets go of something a command pressed. */ +export type HeldInputRelease = { method: string; params: Record } + type Admission = { admit: () => void - /** Buttons, keys and touches this command pressed; only their release outlives a revocation. */ - held: Set + /** What this command pressed and has not released, keyed by button, key or touch. */ + held: Map } const admissions = new Map() +/** + * Runs a host's command holding the page. If the host is revoked mid-press, the command's own + * release is refused like every other step, so the desktop lets go of what it left held once the + * command ends: from outside the host's scope, with routing probes no longer refused. + */ export async function runWithRemoteCliPageAdmission( webContentsId: number, admit: () => void, - run: () => Promise + run: () => Promise, + releaseHeld?: (release: HeldInputRelease) => Promise ): Promise { - const admission: Admission = { admit, held: new Set() } + const admission: Admission = { admit, held: new Map() } admissions.set(webContentsId, admission) try { return await run() @@ -25,78 +36,96 @@ export async function runWithRemoteCliPageAdmission( if (admissions.get(webContentsId) === admission) { admissions.delete(webContentsId) } + if (releaseHeld && admission.held.size > 0 && isRevoked(admit)) { + await runOutsideRemoteCliBrowserScope(() => releaseAll(admission.held, releaseHeld)) + } } } -/** - * Throws while a revoked host's command still holds this page; a no-op otherwise. Pass the input - * being sent so a release of what this command pressed still lands instead of leaving it held. - */ -export function assertRemoteCliPageAdmitted( +/** Throws while a revoked host's command still holds this page; a no-op otherwise. */ +export function assertRemoteCliPageAdmitted(webContentsId: number | undefined): void { + if (webContentsId !== undefined) { + admissions.get(webContentsId)?.admit() + } +} + +/** Records input that reached a page a host's command holds, so a revocation can release it. */ +export function noteRemoteCliInput( webContentsId: number | undefined, - method?: string, + method: string, params?: Record ): void { const admission = webContentsId === undefined ? undefined : admissions.get(webContentsId) - if (!admission) { - return - } - const input = describeInput(method, params) - if (input && !input.press && admission.held.has(input.id)) { - return - } - admission.admit() - if (input?.press) { - admission.held.add(input.id) + if (admission) { + trackInput(admission, method, params) } } -/** True for a release of something the command now holding this page pressed. */ -export function isHeldRemoteCliInputRelease( - webContentsId: number | undefined, - method: string, - params?: Record -): boolean { - const input = describeInput(method, params) - return ( - input !== null && - !input.press && - webContentsId !== undefined && - (admissions.get(webContentsId)?.held.has(input.id) ?? false) - ) +function isRevoked(admit: () => void): boolean { + try { + admit() + return false + } catch { + return true + } } -/** Records a press the command holding this page sent outside the admission gates. */ -export function noteRemoteCliInputPress( - webContentsId: number | undefined, - method: string, - params?: Record -): void { - const input = describeInput(method, params) - if (input?.press && webContentsId !== undefined) { - admissions.get(webContentsId)?.held.add(input.id) +async function releaseAll( + held: Map, + releaseHeld: (release: HeldInputRelease) => Promise +): Promise { + for (const release of held.values()) { + try { + await releaseHeld(release) + } catch { + // Best effort: the page may be gone, and nothing else is waiting on this release. + } } } -function describeInput( +function trackInput( + admission: Admission, method: string | undefined, params: Record | undefined -): { id: string; press: boolean } | null { +): void { const type = params?.type - switch (method) { - case 'Input.dispatchMouseEvent': - return type === 'mousePressed' || type === 'mouseReleased' - ? { id: `mouse:${String(params?.button ?? 'left')}`, press: type === 'mousePressed' } - : null - case 'Input.dispatchKeyEvent': - return type === 'keyDown' || type === 'rawKeyDown' || type === 'keyUp' - ? { id: `key:${String(params?.code ?? params?.key ?? '')}`, press: type !== 'keyUp' } - : null - case 'Input.dispatchTouchEvent': - return type === 'touchStart' || type === 'touchEnd' || type === 'touchCancel' - ? { id: 'touch', press: type === 'touchStart' } - : null - default: - return null + if (!params || typeof type !== 'string') { + return + } + if ( + method === 'Input.dispatchMouseEvent' && + (type === 'mousePressed' || type === 'mouseReleased') + ) { + const id = `mouse:${String(params.button ?? 'left')}` + if (type === 'mouseReleased') { + admission.held.delete(id) + return + } + // Why off the page: letting go there completes no click or drop the host had started. + admission.held.set(id, { + method, + params: { type: 'mouseReleased', x: -1, y: -1, button: params.button ?? 'left', buttons: 0 } + }) + } else if ( + method === 'Input.dispatchKeyEvent' && + ['keyDown', 'rawKeyDown', 'keyUp'].includes(type) + ) { + const id = `key:${String(params.code ?? params.key ?? '')}` + if (type === 'keyUp') { + admission.held.delete(id) + return + } + const { key, code, windowsVirtualKeyCode, nativeVirtualKeyCode } = params + admission.held.set(id, { + method, + params: { type: 'keyUp', key, code, windowsVirtualKeyCode, nativeVirtualKeyCode } + }) + } else if (method === 'Input.dispatchTouchEvent' && type.startsWith('touch')) { + if (type === 'touchStart') { + // Why cancel: a cancelled touch completes no tap the host had started. + admission.held.set('touch', { method, params: { type: 'touchCancel', touchPoints: [] } }) + } else if (type === 'touchEnd' || type === 'touchCancel') { + admission.held.delete('touch') + } } } From 750bb420117274a1d09e70bb2ad21bcbd9a8df4d Mon Sep 17 00:00:00 2001 From: Neil Date: Mon, 19 Oct 2026 00:14:46 -0700 Subject: [PATCH 09/11] fix(browser): retire a revoked command's helper daemon before letting go of the page; release held input with a lease, synced pointer state and the focus guard --- ...ent-browser-bridge-host-revocation.test.ts | 21 +++++++++++ .../browser/agent-browser-bridge-lifecycle.ts | 8 +++-- .../browser/agent-browser-bridge-queue.ts | 35 ++++++++++++++++--- .../browser/agent-browser-bridge-types.ts | 2 ++ src/main/browser/agent-input-injection.ts | 17 ++++++--- .../cdp-ws-proxy-host-admission.test.ts | 5 ++- src/main/browser/guest-direct-key-input.ts | 26 +++++++++++--- src/main/browser/remote-cli-page-admission.ts | 9 +++-- 8 files changed, 104 insertions(+), 19 deletions(-) diff --git a/src/main/browser/agent-browser-bridge-host-revocation.test.ts b/src/main/browser/agent-browser-bridge-host-revocation.test.ts index 5f12d82d5498..3d63908452ed 100644 --- a/src/main/browser/agent-browser-bridge-host-revocation.test.ts +++ b/src/main/browser/agent-browser-bridge-host-revocation.test.ts @@ -128,6 +128,19 @@ describe('AgentBrowserBridge under a host revocation', () => { await vi.waitFor(() => expect(helperCallbacks).toHaveLength(1)) expect(() => assertRemoteCliPageAdmitted(100)).not.toThrow() + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: every instance the hoisted mock constructs has a vi.fn stop. + const proxy = CdpWsProxyMock.instances.at(-1) as { stop: ReturnType } + let heldWhenRetired = false + proxy.stop.mockImplementation(async () => { + heldWhenRetired = (() => { + try { + assertRemoteCliPageAdmitted(100) + return false + } catch { + return true + } + })() + }) revoked.abort() // The helper's next CDP step reaches the page's proxy and is refused there. expect(() => assertRemoteCliPageAdmitted(100)).toThrow('stopped sharing') @@ -139,6 +152,14 @@ describe('AgentBrowserBridge under a host revocation', () => { await expect(click).rejects.toMatchObject({ code: 'desktop_browser_bridge_unavailable' }) expect(() => assertRemoteCliPageAdmitted(100)).not.toThrow() + // The helper's daemon outlives its client, so it is retired before the page is let go. + expect(heldWhenRetired).toBe(true) + expect(execFileMock).toHaveBeenCalledWith( + expect.anything(), + expect.arrayContaining(['close']), + expect.anything(), + expect.anything() + ) }) it('still runs a desktop-local command queued behind a host command that was revoked', async () => { diff --git a/src/main/browser/agent-browser-bridge-lifecycle.ts b/src/main/browser/agent-browser-bridge-lifecycle.ts index e76fa42c6095..78a6327baff5 100644 --- a/src/main/browser/agent-browser-bridge-lifecycle.ts +++ b/src/main/browser/agent-browser-bridge-lifecycle.ts @@ -160,7 +160,9 @@ export abstract class AgentBrowserBridgeLifecycle extends AgentBrowserBridgeRawP const session = this.sessions.get(sessionName) if (!session) { - this.rejectQueuedCommandsForClosedSession(sessionName) + if (!options.keepQueuedCommands) { + this.rejectQueuedCommandsForClosedSession(sessionName) + } return } @@ -168,7 +170,9 @@ export abstract class AgentBrowserBridgeLifecycle extends AgentBrowserBridgeRawP this.pendingSessionCreation.delete(sessionName) // Why: queued commands would hang forever if we just delete the queue — drain and reject them. - this.rejectQueuedCommandsForClosedSession(sessionName) + if (!options.keepQueuedCommands) { + this.rejectQueuedCommandsForClosedSession(sessionName) + } if (session.activeProcess) { // Why: rejecting the queue isn't enough for an in-flight command — kill the process so callers don't wait out the exec timeout. diff --git a/src/main/browser/agent-browser-bridge-queue.ts b/src/main/browser/agent-browser-bridge-queue.ts index 719eb43ad70a..276b874aa4fc 100644 --- a/src/main/browser/agent-browser-bridge-queue.ts +++ b/src/main/browser/agent-browser-bridge-queue.ts @@ -6,11 +6,13 @@ import type { EnqueueTargetedCommandOptions, ResolvedBrowserCommandTarget } from './agent-browser-bridge-types' -import { runWithRemoteCliPageAdmission } from './remote-cli-page-admission' +import { isAdmissionRevoked, runWithRemoteCliPageAdmission } from './remote-cli-page-admission' +import { AGENT_BROWSER_CLEANUP_TIMEOUT_MS } from './agent-browser-bridge-types' import { releaseHeldPageInput } from './guest-direct-key-input' import { captureRemoteCliBrowserRevocation, - captureRemoteCliBrowserScope + captureRemoteCliBrowserScope, + runOutsideRemoteCliBrowserScope } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' export abstract class AgentBrowserBridgeQueue extends AgentBrowserBridgeShutdown { @@ -92,7 +94,9 @@ export abstract class AgentBrowserBridgeQueue extends AgentBrowserBridgeShutdown const gate = (): void => assertNotRevoked(target.webContentsId) gate() return origin.bridged - ? this.runAdmitted(target.webContentsId, gate, () => execute(session, target)) + ? this.runAdmitted(session, target.webContentsId, gate, () => + execute(session, target) + ) : execute(session, target) }, options @@ -127,14 +131,35 @@ export abstract class AgentBrowserBridgeQueue extends AgentBrowserBridgeShutdown * through the CDP proxy, is refused once the host is revoked, and the command reports that. */ private async runAdmitted( + sessionName: string, webContentsId: number, gate: () => void, execute: () => Promise ): Promise { + const held = async (): Promise => { + try { + return await execute() + } finally { + if (isAdmissionRevoked(gate)) { + // Why: the helper's daemon outlives its killed client and keeps retrying; retire it while + // the page is still held, or its next step lands once the hold is gone. + await runOutsideRemoteCliBrowserScope(() => + this.destroySession(sessionName, { + closeTimeoutMs: AGENT_BROWSER_CLEANUP_TIMEOUT_MS, + keepQueuedCommands: true + }) + ) + } + } + } try { - return await runWithRemoteCliPageAdmission(webContentsId, gate, execute, (release) => { + return await runWithRemoteCliPageAdmission(webContentsId, gate, held, (release) => { const wc = this.getWebContents(webContentsId) - return wc ? releaseHeldPageInput(wc, release) : Promise.resolve() + return wc + ? releaseHeldPageInput(wc, release, () => + this.browserManager.guardUserFocusForAgentInput(webContentsId) + ) + : Promise.resolve() }) } catch (error) { gate() diff --git a/src/main/browser/agent-browser-bridge-types.ts b/src/main/browser/agent-browser-bridge-types.ts index 2d4236afdbd8..ad5d1747a6c4 100644 --- a/src/main/browser/agent-browser-bridge-types.ts +++ b/src/main/browser/agent-browser-bridge-types.ts @@ -43,6 +43,8 @@ export type ResolvedBrowserCommandTarget = { export type AgentBrowserCleanupOptions = { closeTimeoutMs?: number + /** Retire only the helper and proxy; commands waiting on the page still run, on a fresh session. */ + keepQueuedCommands?: boolean } export type BrowserMouseModifier = 'cmd' | 'ctrl' | 'alt' | 'shift' diff --git a/src/main/browser/agent-input-injection.ts b/src/main/browser/agent-input-injection.ts index 9f5783e1d4c0..658d06bc7f27 100644 --- a/src/main/browser/agent-input-injection.ts +++ b/src/main/browser/agent-input-injection.ts @@ -1,5 +1,9 @@ import type { WebContents } from 'electron' -import { assertRemoteCliPageAdmitted, noteRemoteCliInput } from './remote-cli-page-admission' +import { + assertRemoteCliPageAdmitted, + isInputRelease, + noteRemoteCliInput +} from './remote-cli-page-admission' import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' // Why: Orca's guest shortcut and grab handlers act on keys the page receives, and an unhandled key @@ -26,8 +30,9 @@ export async function withAgentInputInjection( input?: AgentInputEvent ): Promise { assertAgentInputAdmitted(webContents) - if (input) { - // Why before run: widget events land synchronously and nothing else records them. + // Why: a press counts as held once it may have landed; a release only once it surely did. + const releasing = input !== undefined && isInputRelease(input.params) + if (input && !releasing) { noteRemoteCliInput(webContents.id, input.method, input.params) } const depth = injectionDepth.get(webContents) ?? 0 @@ -36,7 +41,11 @@ export async function withAgentInputInjection( webContents.setIgnoreMenuShortcuts(true) } try { - return await run() + const result = await run() + if (input && releasing) { + noteRemoteCliInput(webContents.id, input.method, input.params) + } + return result } finally { const remaining = (injectionDepth.get(webContents) ?? 1) - 1 if (remaining > 0) { diff --git a/src/main/browser/cdp-ws-proxy-host-admission.test.ts b/src/main/browser/cdp-ws-proxy-host-admission.test.ts index c8ed4b4aeee1..fe89c912e0e9 100644 --- a/src/main/browser/cdp-ws-proxy-host-admission.test.ts +++ b/src/main/browser/cdp-ws-proxy-host-admission.test.ts @@ -103,7 +103,10 @@ describe('CdpWsProxy under a host command', () => { }, () => new Promise((resolve) => (finish = resolve)), // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the mock implements every WebContents member the release path calls. - (release) => releaseHeldPageInput({ ...mock.webContents, id: PAGE_ID } as never, release) + (release) => + releaseHeldPageInput({ ...mock.webContents, id: PAGE_ID } as never, release, () => + guardFocus() + ) ) return { revoke: () => (revoked = true), diff --git a/src/main/browser/guest-direct-key-input.ts b/src/main/browser/guest-direct-key-input.ts index b2f93c0094f2..08e9d2649abf 100644 --- a/src/main/browser/guest-direct-key-input.ts +++ b/src/main/browser/guest-direct-key-input.ts @@ -9,6 +9,9 @@ import { import { sendFrameSessionInput } from './guest-frame-input-sessions' import { planCdpKeyEvent, planInsertText, type GuestEditCommand } from './guest-key-event-plan' import { sendGuestCdpCommand } from './guest-cdp-command' +import { acquireElectronDebugger } from './electron-debugger-lease' +import { normalizeCdpPointerButton } from './agent-browser-bridge-mouse' +import { cdpPointerStateFor, releaseCdpPointerButton } from './cdp-pointer-input' import type { HeldInputRelease } from './remote-cli-page-admission' // Why: Chromium routes main-session CDP key and text input on a guest by the @@ -167,15 +170,28 @@ export async function dispatchPageKeyEvent( } /** Lets go of what a revoked host's command left pressed; a key goes where the focus routes it. */ -export function releaseHeldPageInput( +export async function releaseHeldPageInput( webContents: WebContents, - release: HeldInputRelease + release: HeldInputRelease, + guardFocus: AgentInputFocusGuard ): Promise { const send: GuestCdpSender = (method, params, sessionId) => sendGuestCdpCommand(webContents, method, params, sessionId) - return release.method === 'Input.dispatchKeyEvent' - ? dispatchPageKeyEvent(webContents, release.params, send) - : send(release.method, release.params) + // Why: the command's own debugger lease may already be gone, detaching the debugger. + const lease = acquireElectronDebugger(webContents) + try { + if (release.method === 'Input.dispatchKeyEvent') { + return await dispatchPageKeyEvent(webContents, release.params, send, guardFocus) + } + const result = await send(release.method, release.params) + if (release.method === 'Input.dispatchMouseEvent') { + const button = normalizeCdpPointerButton(String(release.params.button)) + releaseCdpPointerButton(cdpPointerStateFor(webContents), button) + } + return result + } finally { + lease.release() + } } export type TextInsertionRoute = { diff --git a/src/main/browser/remote-cli-page-admission.ts b/src/main/browser/remote-cli-page-admission.ts index cecf2079b884..d94f9dea2431 100644 --- a/src/main/browser/remote-cli-page-admission.ts +++ b/src/main/browser/remote-cli-page-admission.ts @@ -36,7 +36,7 @@ export async function runWithRemoteCliPageAdmission( if (admissions.get(webContentsId) === admission) { admissions.delete(webContentsId) } - if (releaseHeld && admission.held.size > 0 && isRevoked(admit)) { + if (releaseHeld && admission.held.size > 0 && isAdmissionRevoked(admit)) { await runOutsideRemoteCliBrowserScope(() => releaseAll(admission.held, releaseHeld)) } } @@ -61,7 +61,8 @@ export function noteRemoteCliInput( } } -function isRevoked(admit: () => void): boolean { +/** True once the host behind this admission check was revoked. */ +export function isAdmissionRevoked(admit: () => void): boolean { try { admit() return false @@ -83,6 +84,10 @@ async function releaseAll( } } +export function isInputRelease(params: Record): boolean { + return ['mouseReleased', 'keyUp', 'touchEnd', 'touchCancel'].includes(String(params.type)) +} + function trackInput( admission: Admission, method: string | undefined, From 480cb34aafcabd02af166871c702ff60c6083b40 Mon Sep 17 00:00:00 2001 From: Neil Date: Mon, 19 Oct 2026 00:22:18 -0700 Subject: [PATCH 10/11] fix(browser): deferred proxy input keeps its hold, releases count once they land, and a revoked helper is retired at once with its routes kept --- ...ent-browser-bridge-host-revocation.test.ts | 2 + .../browser/agent-browser-bridge-lifecycle.ts | 4 ++ .../browser/agent-browser-bridge-queue.ts | 27 +++++++---- .../cdp-ws-proxy-host-admission.test.ts | 45 +++++++++++++++++++ src/main/browser/cdp-ws-proxy.ts | 9 +++- src/main/browser/guest-cdp-command.ts | 16 +++++-- src/main/browser/remote-cli-page-admission.ts | 10 +++++ .../remote-cli-browser-scope.ts | 5 +++ 8 files changed, 104 insertions(+), 14 deletions(-) diff --git a/src/main/browser/agent-browser-bridge-host-revocation.test.ts b/src/main/browser/agent-browser-bridge-host-revocation.test.ts index 3d63908452ed..f75234734848 100644 --- a/src/main/browser/agent-browser-bridge-host-revocation.test.ts +++ b/src/main/browser/agent-browser-bridge-host-revocation.test.ts @@ -144,6 +144,8 @@ describe('AgentBrowserBridge under a host revocation', () => { revoked.abort() // The helper's next CDP step reaches the page's proxy and is refused there. expect(() => assertRemoteCliPageAdmitted(100)).toThrow('stopped sharing') + // Retired on the revocation itself, not when the helper's client times out. + await vi.waitFor(() => expect(proxy.stop).toHaveBeenCalled()) helperCallbacks[0]!( null, JSON.stringify({ success: false, error: 'The Orca desktop stopped sharing its browser' }), diff --git a/src/main/browser/agent-browser-bridge-lifecycle.ts b/src/main/browser/agent-browser-bridge-lifecycle.ts index 78a6327baff5..1900c5767ef5 100644 --- a/src/main/browser/agent-browser-bridge-lifecycle.ts +++ b/src/main/browser/agent-browser-bridge-lifecycle.ts @@ -168,6 +168,10 @@ export abstract class AgentBrowserBridgeLifecycle extends AgentBrowserBridgeRawP this.sessions.delete(sessionName) this.pendingSessionCreation.delete(sessionName) + // Why: a retired session's page stays open, so the fresh one must keep its interception. + if (options.keepQueuedCommands && session.activeInterceptPatterns.length > 0) { + this.pendingInterceptRestore.set(sessionName, [...session.activeInterceptPatterns]) + } // Why: queued commands would hang forever if we just delete the queue — drain and reject them. if (!options.keepQueuedCommands) { diff --git a/src/main/browser/agent-browser-bridge-queue.ts b/src/main/browser/agent-browser-bridge-queue.ts index 276b874aa4fc..e63b2842e446 100644 --- a/src/main/browser/agent-browser-bridge-queue.ts +++ b/src/main/browser/agent-browser-bridge-queue.ts @@ -11,6 +11,7 @@ import { AGENT_BROWSER_CLEANUP_TIMEOUT_MS } from './agent-browser-bridge-types' import { releaseHeldPageInput } from './guest-direct-key-input' import { captureRemoteCliBrowserRevocation, + captureRemoteCliBrowserRevokedSignal, captureRemoteCliBrowserScope, runOutsideRemoteCliBrowserScope } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' @@ -136,19 +137,27 @@ export abstract class AgentBrowserBridgeQueue extends AgentBrowserBridgeShutdown gate: () => void, execute: () => Promise ): Promise { + // Why: the helper's daemon outlives its killed client and keeps retrying; retire it while the + // page is still held, or its next step lands once the hold is gone. Retiring on the signal also + // frees the page's queue now instead of at the helper's exec timeout. + let retiring: Promise | null = null + const retire = (): Promise => + (retiring ??= runOutsideRemoteCliBrowserScope(() => + this.destroySession(sessionName, { + closeTimeoutMs: AGENT_BROWSER_CLEANUP_TIMEOUT_MS, + keepQueuedCommands: true + }) + )) + const signal = captureRemoteCliBrowserRevokedSignal() + const onRevoked = (): void => void retire() + signal?.addEventListener('abort', onRevoked, { once: true }) const held = async (): Promise => { try { return await execute() } finally { - if (isAdmissionRevoked(gate)) { - // Why: the helper's daemon outlives its killed client and keeps retrying; retire it while - // the page is still held, or its next step lands once the hold is gone. - await runOutsideRemoteCliBrowserScope(() => - this.destroySession(sessionName, { - closeTimeoutMs: AGENT_BROWSER_CLEANUP_TIMEOUT_MS, - keepQueuedCommands: true - }) - ) + signal?.removeEventListener('abort', onRevoked) + if (retiring || isAdmissionRevoked(gate)) { + await retire() } } } diff --git a/src/main/browser/cdp-ws-proxy-host-admission.test.ts b/src/main/browser/cdp-ws-proxy-host-admission.test.ts index fe89c912e0e9..f373de75506b 100644 --- a/src/main/browser/cdp-ws-proxy-host-admission.test.ts +++ b/src/main/browser/cdp-ws-proxy-host-admission.test.ts @@ -216,6 +216,51 @@ describe('CdpWsProxy under a host command', () => { client.close() }) + it('still refuses a deferred press after the revoked command let go of the page', async () => { + mock.webContents.getType.mockReturnValue('webview') + let grantFocus = (): void => {} + guardFocus = () => new Promise((resolve) => (grantFocus = () => resolve(() => {}))) + const page = holdPage() + const client = await connect(endpoint) + + const reply = sendAndReceive(client, { id: 13, method: MOUSE, params: pressed }) + await vi.waitFor(() => + expect(getSendCommandMethods(mock)).toContain('Emulation.setFocusEmulationEnabled') + ) + page.revoke() + await page.release() + grantFocus() + + await expect(reply).resolves.toMatchObject({ id: 13, error: { message: REVOKED } }) + expect(mouseEvents(mock)).toEqual([]) + client.close() + }) + + it('lets go of a button whose own release failed to land', async () => { + const page = holdPage() + const client = await connect(endpoint) + const send = mock.webContents.debugger.sendCommand.getMockImplementation() + mock.webContents.debugger.sendCommand.mockImplementationOnce(async () => ({})) + mock.webContents.debugger.sendCommand.mockImplementationOnce(async () => { + throw new Error('target closed') + }) + + await sendAndReceive(client, { id: 14, method: MOUSE, params: pressed }) + const failed = await sendAndReceive(client, { id: 15, method: MOUSE, params: released }) + if (send) { + mock.webContents.debugger.sendCommand.mockImplementation(send) + } + page.revoke() + await page.release() + + expect(failed.error).toBeDefined() + expect(mock.webContents.debugger.sendCommand).toHaveBeenCalledWith( + MOUSE, + expect.objectContaining({ type: 'mouseReleased', x: -1, y: -1 }) + ) + client.close() + }) + it('forwards commands again once the host command ends', async () => { await runWithRemoteCliPageAdmission( PAGE_ID, diff --git a/src/main/browser/cdp-ws-proxy.ts b/src/main/browser/cdp-ws-proxy.ts index 79c4dd63f7c8..163e857c01d0 100644 --- a/src/main/browser/cdp-ws-proxy.ts +++ b/src/main/browser/cdp-ws-proxy.ts @@ -12,7 +12,10 @@ import type { CapturePaintHold } from './cdp-screenshot' import type { AgentInputFocusGuard } from './agent-input-focus-guard' import { CdpPageInputCommands } from './cdp-page-input-commands' import { isTrustedCdpLoopbackRequest } from './cdp-loopback-request-guard' -import { assertRemoteCliPageAdmitted } from './remote-cli-page-admission' +import { + assertRemoteCliPageAdmitted, + runUnderRemoteCliPageAdmission +} from './remote-cli-page-admission' export class CdpWsProxy { private httpServer: Server | null = null @@ -97,7 +100,9 @@ export class CdpWsProxy { this.closeClient() this.client = ws const onMessage = (data: WebSocket.RawData): void => { - this.handleClientMessage(ws, data.toString()) + runUnderRemoteCliPageAdmission(this.webContents.id, () => + this.handleClientMessage(ws, data.toString()) + ) } const onClose = (): void => { detach() diff --git a/src/main/browser/guest-cdp-command.ts b/src/main/browser/guest-cdp-command.ts index cee48121a8a2..f3845e552a93 100644 --- a/src/main/browser/guest-cdp-command.ts +++ b/src/main/browser/guest-cdp-command.ts @@ -1,7 +1,7 @@ import type { WebContents } from 'electron' import { BrowserError } from './browser-error' import { assertRemoteCliBrowserAccess } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' -import { noteRemoteCliInput } from './remote-cli-page-admission' +import { isInputRelease, noteRemoteCliInput } from './remote-cli-page-admission' // Why: Chromium resizes the page's view for these without checking the view still exists // (WebContentsImpl::SetDeviceEmulationSize). A crashed renderer takes its view with it until the @@ -43,10 +43,20 @@ export function sendGuestCdpCommand( ) ) } - noteRemoteCliInput(guest.id, method, params) - return Promise.resolve( + // Why: a press counts as held once it may have landed; a release only once it surely did. + const releasing = params !== undefined && isInputRelease(params) + if (!releasing) { + noteRemoteCliInput(guest.id, method, params) + } + const sent = Promise.resolve( sessionId === undefined ? guest.debugger.sendCommand(method, params) : guest.debugger.sendCommand(method, params, sessionId) ) + return releasing + ? sent.then((result: unknown) => { + noteRemoteCliInput(guest.id, method, params) + return result + }) + : sent } diff --git a/src/main/browser/remote-cli-page-admission.ts b/src/main/browser/remote-cli-page-admission.ts index d94f9dea2431..c80979150835 100644 --- a/src/main/browser/remote-cli-page-admission.ts +++ b/src/main/browser/remote-cli-page-admission.ts @@ -4,6 +4,7 @@ * edit commands) consults it, so no queued or deferred step of a running command outlives the * revocation. */ +import { AsyncLocalStorage } from 'node:async_hooks' import { runOutsideRemoteCliBrowserScope } from '../runtime/desktop-browser-bridge/remote-cli-browser-scope' /** The CDP input event that lets go of something a command pressed. */ @@ -16,6 +17,8 @@ type Admission = { } const admissions = new Map() +/** The admission a proxy message arrived under; its deferred steps keep it after the hold ends. */ +const messageAdmission = new AsyncLocalStorage<() => void>() /** * Runs a host's command holding the page. If the host is revoked mid-press, the command's own @@ -47,6 +50,13 @@ export function assertRemoteCliPageAdmitted(webContentsId: number | undefined): if (webContentsId !== undefined) { admissions.get(webContentsId)?.admit() } + messageAdmission.getStore()?.() +} + +/** Runs a proxy message's handling, deferred steps included, under the page's current hold. */ +export function runUnderRemoteCliPageAdmission(webContentsId: number, handle: () => T): T { + const admission = admissions.get(webContentsId) + return admission ? messageAdmission.run(admission.admit, handle) : handle() } /** Records input that reached a page a host's command holds, so a revocation can release it. */ diff --git a/src/main/runtime/desktop-browser-bridge/remote-cli-browser-scope.ts b/src/main/runtime/desktop-browser-bridge/remote-cli-browser-scope.ts index ff510c30d6e9..469f27d07e7a 100644 --- a/src/main/runtime/desktop-browser-bridge/remote-cli-browser-scope.ts +++ b/src/main/runtime/desktop-browser-bridge/remote-cli-browser-scope.ts @@ -70,6 +70,11 @@ export function captureRemoteCliBrowserScope(): { } } +/** The signal that fires when the host behind the current command is revoked, if any. */ +export function captureRemoteCliBrowserRevokedSignal(): AbortSignal | undefined { + return scope.getStore()?.revoked +} + /** Runs `work` in no host's scope, so long-lived resources it creates never inherit one. */ export function runOutsideRemoteCliBrowserScope(work: () => T): T { return scope.exit(work) From ed650ffa27f41858c7d7e07a2ef3ca7e653a1a0c Mon Sep 17 00:00:00 2001 From: Neil Date: Mon, 19 Oct 2026 00:31:31 -0700 Subject: [PATCH 11/11] test(browser): keep the mock page cast on its SAFETY line --- src/main/browser/cdp-ws-proxy-host-admission.test.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/main/browser/cdp-ws-proxy-host-admission.test.ts b/src/main/browser/cdp-ws-proxy-host-admission.test.ts index f373de75506b..ef43cdc249ef 100644 --- a/src/main/browser/cdp-ws-proxy-host-admission.test.ts +++ b/src/main/browser/cdp-ws-proxy-host-admission.test.ts @@ -92,6 +92,8 @@ describe('CdpWsProxy under a host command', () => { }) function holdPage(): { revoke: () => void; release: () => Promise } { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the mock implements every WebContents member the release path calls. + const page = { ...mock.webContents, id: PAGE_ID } as never let revoked = false let finish = (): void => {} const held = runWithRemoteCliPageAdmission( @@ -102,11 +104,7 @@ describe('CdpWsProxy under a host command', () => { } }, () => new Promise((resolve) => (finish = resolve)), - // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the mock implements every WebContents member the release path calls. - (release) => - releaseHeldPageInput({ ...mock.webContents, id: PAGE_ID } as never, release, () => - guardFocus() - ) + (release) => releaseHeldPageInput(page, release, () => guardFocus()) ) return { revoke: () => (revoked = true),