diff --git a/src/main/browser/grab-guest-script.test.ts b/src/main/browser/grab-guest-script.test.ts index 17f9f95b483..a86764a5733 100644 --- a/src/main/browser/grab-guest-script.test.ts +++ b/src/main/browser/grab-guest-script.test.ts @@ -1,5 +1,8 @@ +import { types } from 'node:util' +import { runInNewContext } from 'node:vm' import { describe, expect, it } from 'vitest' import { buildGuestOverlayScript } from './grab-guest-script' +import { clampGrabPayload } from './browser-grab-payload' describe('buildGuestOverlayScript', () => { it('returns a non-empty string for arm action', () => { @@ -158,3 +161,199 @@ describe('buildGuestOverlayScript', () => { expect(script).not.toContain('ariaLabelledBy.split(/\\s+/)') }) }) + +// Regression coverage for issue #9947: Zone.js swaps the global Promise for a +// non-native thenable, which executeJavaScript won't unwrap — so a page-global +// `new Promise(...)` crossed as its raw `__zone_symbol__*` wrapper, not { page, target }. +describe('awaitClick under a Zone.js-patched global Promise', () => { + type Executor = (resolve: (value: T) => void, reject: (reason: unknown) => void) => void + + // Native promises are kept off the instance so its own enumerable keys match + // Zone.js exactly: ['__zone_symbol__state', '__zone_symbol__value']. + const nativeOf = new WeakMap>() + + /** A non-native thenable that mimics Zone.js's ZoneAwarePromise wrapper. */ + class ZoneAwarePromiseLike { + __zone_symbol__state: unknown = null + __zone_symbol__value: unknown = undefined + + constructor(executor: Executor) { + nativeOf.set( + this, + new Promise((res, rej) => { + executor( + (value) => { + this.__zone_symbol__state = true + this.__zone_symbol__value = value + res(value) + }, + (reason) => { + this.__zone_symbol__state = false + this.__zone_symbol__value = reason + rej(reason) + } + ) + }) + ) + } + + // Why: real Zone.js defines `get [Symbol.toStringTag]() { return 'Promise' }`, + // so an Object.prototype.toString check is fooled into seeing a promise. The + // boundary below deliberately uses the brand-based V8 check instead, and the + // control test asserts this tag does NOT let the wrapper masquerade as native. + get [Symbol.toStringTag](): string { + return 'Promise' + } + + // oxlint-disable-next-line unicorn/no-thenable -- intentionally a non-native thenable modeling Zone.js's ZoneAwarePromise + then( + onFulfilled?: ((value: unknown) => unknown) | null, + onRejected?: ((reason: unknown) => unknown) | null + ): Promise { + const native = nativeOf.get(this) as Promise + return native.then(onFulfilled ?? undefined, onRejected ?? undefined) + } + } + + /** + * Models Electron's boundary: it awaits only genuine V8 promises and + * serializes anything else — including non-native thenables — by value. + * `util.types.isPromise` is V8's brand-based IsPromise (what Electron uses): + * realm-independent and unforgeable, unlike an Object.prototype.toString / + * Symbol.toStringTag check that a ZoneAwarePromise would defeat. + */ + async function crossExecuteJavaScriptBoundary(completionValue: unknown): Promise { + if (types.isPromise(completionValue)) { + return await (completionValue as Promise) + } + return { ...(completionValue as Record) } + } + + const validPayload = (): Record => ({ + // A minimal but structurally valid payload — page + target are what + // clampGrabPayload requires and what the bug stripped away. + page: { title: 'Angular App' }, + target: { tagName: 'button' }, + nearbyText: [], + ancestorPath: [], + screenshot: null + }) + + function armGrabHarness(options?: { + extractPayload?: () => unknown + getCurrentElement?: () => unknown + }): { + window: { __orcaGrab: Record } + click: () => void + contextmenu: () => void + cancel: () => void + } { + const handlers: Record void> = {} + const noopEvent = { + preventDefault(): void {}, + stopPropagation(): void {}, + stopImmediatePropagation(): void {} + } + const grab: Record = { + host: { + addEventListener(type: string, fn: (event: unknown) => void): void { + handlers[type] = fn + }, + removeEventListener(): void {} + }, + extractPayload: options?.extractPayload ?? validPayload, + getCurrentElement: options?.getCurrentElement ?? ((): unknown => ({})), + freezeHighlight(): void {}, + cleanup(): void {} + } + const window = { __orcaGrab: grab } + return { + window, + click: () => handlers.click?.(noopEvent), + contextmenu: () => handlers.contextmenu?.(noopEvent), + // cancelAwait is installed on __orcaGrab by the script itself at runtime. + cancel: () => (window.__orcaGrab.cancelAwait as (() => void) | undefined)?.() + } + } + + const runAwaitClick = (harness: ReturnType): unknown => + runInNewContext(buildGuestOverlayScript('awaitClick'), { + window: harness.window, + Promise: ZoneAwarePromiseLike, + Error + }) + + it('returns the payload through a native async promise, not the page-global Promise', () => { + const script = buildGuestOverlayScript('awaitClick') + expect(script).toContain('(async function()') + expect(script).toContain('return await new Promise(') + }) + + it('resolves { page, target } across the boundary despite ZoneAwarePromise', async () => { + const harness = armGrabHarness() + const completion = runAwaitClick(harness) + + // The async IIFE hands Electron an intrinsic promise even though the global + // Promise is a non-native thenable — so the boundary unwraps it. + expect(types.isPromise(completion)).toBe(true) + + harness.click() + const received = await crossExecuteJavaScriptBoundary(completion) + + expect(received).toHaveProperty('page') + expect(received).toHaveProperty('target') + expect(received).not.toHaveProperty('__zone_symbol__value') + expect(clampGrabPayload(received)).not.toBeNull() + }) + + it('resolves the context-menu marker across the boundary despite ZoneAwarePromise', async () => { + const harness = armGrabHarness() + const completion = runAwaitClick(harness) + + harness.contextmenu() + const received = (await crossExecuteJavaScriptBoundary(completion)) as Record + + expect(received).toHaveProperty('__orcaContextMenu', true) + expect(received.payload).toHaveProperty('page') + expect(clampGrabPayload(received.payload)).not.toBeNull() + }) + + it('resolves the teardown cancel marker across the boundary despite ZoneAwarePromise', async () => { + const harness = armGrabHarness() + const completion = runAwaitClick(harness) + + harness.cancel() + const received = await crossExecuteJavaScriptBoundary(completion) + + expect(received).toEqual({ __orcaCancelled: true }) + }) + + it('rejects across the boundary when selection fails despite ZoneAwarePromise', async () => { + // getCurrentElement -> null drives onClick's reject(new Error('cancelled')), + // which must surface as a rejected intrinsic promise (not a serialized value) + // so the controller classifies it as a cancellation rather than a payload. + const harness = armGrabHarness({ getCurrentElement: () => null }) + const completion = runAwaitClick(harness) + + harness.click() + await expect(crossExecuteJavaScriptBoundary(completion)).rejects.toThrow('cancelled') + }) + + it('control: a bare page-global new Promise would cross as the raw wrapper', async () => { + // Proves the harness detects the regression: without the async wrapper the + // completion value is the ZoneAwarePromise itself, which the boundary + // serializes to __zone_symbol__* fields with no page/target. + const bare = runInNewContext('new Promise(function(r){ r({ page: {}, target: {} }); })', { + Promise: ZoneAwarePromiseLike + }) + // Symbol.toStringTag='Promise' fools a toString check — exactly why the + // boundary must use the brand-based IsPromise, which still rejects it. + expect(Object.prototype.toString.call(bare)).toBe('[object Promise]') + expect(types.isPromise(bare)).toBe(false) + + const received = await crossExecuteJavaScriptBoundary(bare) + expect(received).not.toHaveProperty('page') + expect(received).toHaveProperty('__zone_symbol__value') + expect(clampGrabPayload(received)).toBeNull() + }) +}) diff --git a/src/main/browser/grab-guest-script.ts b/src/main/browser/grab-guest-script.ts index 8841fb6cf97..d1d089be1af 100644 --- a/src/main/browser/grab-guest-script.ts +++ b/src/main/browser/grab-guest-script.ts @@ -823,80 +823,90 @@ const ARM_SCRIPT = `(function() { })()` // awaitClick: resolve when the user clicks the overlay; stopPropagation + pointer-events:all keep the click off the page. -const AWAIT_CLICK_SCRIPT = `new Promise(function(resolve, reject) { - 'use strict'; - var grab = window.__orcaGrab; - if (!grab) { - reject(new Error('Grab not armed')); - return; - } - - function extractSelectedPayload(el) { - try { - return grab.extractPayload(el); - } catch (error) { - grab.cleanup(); - reject(error instanceof Error ? error : new Error('Failed to extract element context')); - return null; - } - } - - function onClick(e) { - e.preventDefault(); - e.stopPropagation(); - e.stopImmediatePropagation(); - grab.host.removeEventListener('click', onClick, true); - grab.host.removeEventListener('contextmenu', onContext, true); - var el = grab.getCurrentElement(); - if (!el) { - grab.cleanup(); - reject(new Error('cancelled')); +const AWAIT_CLICK_SCRIPT = `(async function() { + // Why: hand the click result to executeJavaScript through a native (intrinsic) + // Promise. On pages that replace the global Promise with a non-native thenable + // — e.g. Angular Zone.js's ZoneAwarePromise — a bare \`new Promise(...)\` is not + // recognized as a promise by Electron, so its raw wrapper object (exposing + // __zone_symbol__state/__value instead of { page, target }) crosses the boundary + // and main rejects it as an invalid payload structure. An async function's + // promise comes from the engine intrinsic that page code cannot reassign, so + // Electron always unwraps it to the resolved payload. + return await new Promise(function(resolve, reject) { + 'use strict'; + var grab = window.__orcaGrab; + if (!grab) { + reject(new Error('Grab not armed')); return; } - var payload = extractSelectedPayload(el); - if (!payload) return; - // Why: freeze the highlight instead of removing it so the user sees - // which element was selected while the copy menu is shown. Teardown - // happens later when the renderer calls setGrabMode(false) or re-arms. - grab.freezeHighlight(); - resolve(payload); - } - function onContext(e) { - // Why: right-click resolves with the payload wrapped in a context-menu - // marker so the renderer can show the full action dropdown instead of - // auto-copying. This gives users a deliberate path to screenshot and - // other secondary actions while keeping left-click as the fast copy path. - e.preventDefault(); - e.stopPropagation(); - e.stopImmediatePropagation(); - grab.host.removeEventListener('click', onClick, true); - grab.host.removeEventListener('contextmenu', onContext, true); - var el = grab.getCurrentElement(); - if (!el) { - grab.cleanup(); - reject(new Error('cancelled')); - return; + function extractSelectedPayload(el) { + try { + return grab.extractPayload(el); + } catch (error) { + grab.cleanup(); + reject(error instanceof Error ? error : new Error('Failed to extract element context')); + return null; + } } - var payload = extractSelectedPayload(el); - if (!payload) return; - grab.freezeHighlight(); - resolve({ __orcaContextMenu: true, payload: payload }); - } - grab.host.addEventListener('click', onClick, true); - grab.host.addEventListener('contextmenu', onContext, true); + function onClick(e) { + e.preventDefault(); + e.stopPropagation(); + e.stopImmediatePropagation(); + grab.host.removeEventListener('click', onClick, true); + grab.host.removeEventListener('contextmenu', onContext, true); + var el = grab.getCurrentElement(); + if (!el) { + grab.cleanup(); + reject(new Error('cancelled')); + return; + } + var payload = extractSelectedPayload(el); + if (!payload) return; + // Why: freeze the highlight instead of removing it so the user sees + // which element was selected while the copy menu is shown. Teardown + // happens later when the renderer calls setGrabMode(false) or re-arms. + grab.freezeHighlight(); + resolve(payload); + } - // Store cancel hook so teardown can settle the Promise - grab.cancelAwait = function() { - grab.host.removeEventListener('click', onClick, true); - grab.host.removeEventListener('contextmenu', onContext, true); - grab.cleanup(); - // Why: teardown cancellation is a normal user flow; resolving a marker - // avoids a noisy guest-console Error while main still treats it as cancel. - resolve({ __orcaCancelled: true }); - }; -})` + function onContext(e) { + // Why: right-click resolves with the payload wrapped in a context-menu + // marker so the renderer can show the full action dropdown instead of + // auto-copying. This gives users a deliberate path to screenshot and + // other secondary actions while keeping left-click as the fast copy path. + e.preventDefault(); + e.stopPropagation(); + e.stopImmediatePropagation(); + grab.host.removeEventListener('click', onClick, true); + grab.host.removeEventListener('contextmenu', onContext, true); + var el = grab.getCurrentElement(); + if (!el) { + grab.cleanup(); + reject(new Error('cancelled')); + return; + } + var payload = extractSelectedPayload(el); + if (!payload) return; + grab.freezeHighlight(); + resolve({ __orcaContextMenu: true, payload: payload }); + } + + grab.host.addEventListener('click', onClick, true); + grab.host.addEventListener('contextmenu', onContext, true); + + // Store cancel hook so teardown can settle the Promise + grab.cancelAwait = function() { + grab.host.removeEventListener('click', onClick, true); + grab.host.removeEventListener('contextmenu', onContext, true); + grab.cleanup(); + // Why: teardown cancellation is a normal user flow; resolving a marker + // avoids a noisy guest-console Error while main still treats it as cancel. + resolve({ __orcaCancelled: true }); + }; + }); +})()` const FINALIZE_SCRIPT = `(function() { 'use strict'; @@ -933,8 +943,9 @@ const TEARDOWN_SCRIPT = `(function() { 'use strict'; var grab = window.__orcaGrab; if (!grab) return true; - // If there's an active awaitClick Promise, cancel it so the - // executeJavaScript call in main rejects and settles the grab op. + // If there's an active awaitClick Promise, cancel it: cancelAwait resolves + // it with the __orcaCancelled marker so the executeJavaScript call in main + // settles the grab op as a cancellation. if (grab.cancelAwait) { grab.cancelAwait(); } else { diff --git a/src/renderer/src/lib/agent-background-session-test-state.ts b/src/renderer/src/lib/agent-background-session-test-state.ts index b338493c411..fae70ea4e3c 100644 --- a/src/renderer/src/lib/agent-background-session-test-state.ts +++ b/src/renderer/src/lib/agent-background-session-test-state.ts @@ -30,7 +30,10 @@ export type AgentBackgroundSessionTestState = { { id: string; repoId: string; projectId: string; path: string; displayName: string }[] > tabsByWorktree: Record - terminalLayoutsByTabId: Record }> + terminalLayoutsByTabId: Record< + string, + { ptyIdsByLeafId?: Record } + > ptyIdsByTabId: Record sshConnectionStates: Map transientClearedAgentStatusConnectionIds: Record