fix(browser): prevent window.close guest crashes (#11910)

* fix(browser): prevent window.close guest crashes

* fix(browser): guard close before inline scripts

* fix(browser): preserve explicit window close policy
This commit is contained in:
Neil 2026-08-01 03:08:11 -07:00 committed by GitHub
parent e531e796b2
commit c79b859758
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
18 changed files with 340 additions and 12 deletions

View File

@ -206,6 +206,8 @@ export const electronViteConfig: UserConfig = {
external: isExternalMainModule,
input: {
index: resolve('src/main/index.ts'),
// Why: sandboxed webview preloads cannot load Rollup helper chunks.
'browser-window-close-preload': resolve('src/preload/browser-window-close.ts'),
'daemon-entry': resolve('src/main/daemon/daemon-entry.ts'),
'plugin-host-entry': resolve('src/main/plugins/plugin-host-entry.ts'),
'computer-sidecar': resolve('src/main/computer/sidecar-entry.ts'),

View File

@ -1512,6 +1512,38 @@ describe('browserManager', () => {
expect(browserManager.getGuestWebContentsId('browser-destroyed-before-register')).toBeNull()
})
it('removes a destroyed primary guest from its tab registration maps', () => {
const guest = {
id: 306,
isDestroyed: vi.fn(() => false),
getType: vi.fn(() => 'webview'),
setBackgroundThrottling: guestSetBackgroundThrottlingMock,
setWindowOpenHandler: guestSetWindowOpenHandlerMock,
on: guestOnMock,
off: guestOffMock,
openDevTools: guestOpenDevToolsMock
}
webContentsFromIdMock.mockReturnValue(guest)
browserManager.attachGuestPolicies(guest as never)
browserManager.registerGuest({
browserPageId: 'browser-destroyed-after-register',
webContentsId: guest.id,
rendererWebContentsId
})
const destroyedHandler = guestOnMock.mock.calls.find(
([event]) => event === 'destroyed'
)?.[1] as (() => void) | undefined
destroyedHandler?.()
expect(browserManager.getGuestWebContentsId('browser-destroyed-after-register')).toBeNull()
const managerState = browserManager as unknown as {
tabIdByWebContentsId: Map<number, string>
}
expect(managerState.tabIdByWebContentsId.has(guest.id)).toBe(false)
})
it('fully unregisters stale guests discovered during authorization', () => {
const guest = {
id: 305,

View File

@ -640,7 +640,6 @@ export class BrowserManager {
// Why: bot detectors probe APIs that differ in Electron webviews; inject overrides each load so manual browsing passes.
const disposeAntiDetection = this.injectAntiDetection(guest)
// Why: disable throttling so background screenshots still get frames; else the compositor stalls and capture returns empty.
guest.setBackgroundThrottling(false)
const installClickedLinkRouting = (): void => {
@ -940,11 +939,15 @@ export class BrowserManager {
private retireStaleGuestWebContents(previousWebContentsId: number): void {
// Why: after a renderer-process swap, stop the dead guest id resolving to the live page so stale callbacks don't hit the wrong session.
this.cleanupGuestPolicyAttachment(previousWebContentsId)
this.tabIdByWebContentsId.delete(previousWebContentsId)
}
private cleanupGuestPolicyAttachment(guestWebContentsId: number): void {
const isPrimaryGuest = this.tabIdByWebContentsId.has(guestWebContentsId)
const browserTabId = this.tabIdByWebContentsId.get(guestWebContentsId)
const isPrimaryGuest = browserTabId !== undefined
if (browserTabId && this.webContentsIdByTabId.get(browserTabId) === guestWebContentsId) {
this.webContentsIdByTabId.delete(browserTabId)
}
this.tabIdByWebContentsId.delete(guestWebContentsId)
this.certificateTrustController?.onGuestRetired(guestWebContentsId)
const policyCleanup = this.policyCleanupByGuestId.get(guestWebContentsId)
if (policyCleanup) {

View File

@ -74,6 +74,8 @@ import {
} from './createMainWindow'
import { ipcMain } from 'electron'
import { shouldRecoverRendererAfterProcessGone } from '../crash-reporting/process-gone-classification'
import { BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD } from '../../shared/browser-window-close-policy'
import { fileURLToPath } from 'node:url'
function withPlatform<T>(platform: NodeJS.Platform, run: () => T): T {
const original = process.platform
@ -248,6 +250,7 @@ describe('createMainWindow', () => {
expect(allowBlankPrefs).toMatchObject({
disableHtmlFullscreenWindowResize: true,
partition: 'persist:orca-browser',
preload: expect.stringMatching(/browser-window-close-preload\.js$/),
sandbox: true
})
@ -262,6 +265,36 @@ describe('createMainWindow', () => {
const guest = { marker: 'guest' }
windowHandlers['did-attach-webview']({} as never, guest as never)
expect(attachGuestPoliciesMock).toHaveBeenCalledWith(guest)
const allowWindowCloseEvent = { preventDefault: vi.fn() }
const allowWindowCloseParams = {
src: 'data:text/html,',
preload: BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD
}
const allowWindowClosePrefs = { partition: 'persist:orca-browser' }
windowHandlers['will-attach-webview'](
allowWindowCloseEvent as never,
allowWindowClosePrefs as never,
allowWindowCloseParams as never
)
expect(allowWindowCloseEvent.preventDefault).not.toHaveBeenCalled()
expect(allowWindowCloseParams.preload).toBeUndefined()
expect(allowWindowClosePrefs).not.toHaveProperty('preload')
const allowWindowClosePathPrefs = {
partition: 'persist:orca-browser',
preload: fileURLToPath(BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD)
}
windowHandlers['will-attach-webview'](
{ preventDefault: vi.fn() } as never,
allowWindowClosePathPrefs as never,
{ src: 'data:text/html,', preload: '' } as never
)
expect(allowWindowClosePathPrefs).not.toHaveProperty('preload')
const cliGuest = { marker: 'cli-guest' }
windowHandlers['did-attach-webview']({} as never, cliGuest as never)
expect(attachGuestPoliciesMock).toHaveBeenLastCalledWith(cliGuest)
})
it('sets platform-specific titlebar and frame options for every desktop platform', () => {

View File

@ -10,6 +10,7 @@ import {
screen
} from 'electron'
import { join } from 'node:path'
import { fileURLToPath } from 'node:url'
import { is } from '@electron-toolkit/utils'
import type { Store } from '../persistence'
import { getAppIconPath } from '../app-icon'
@ -18,6 +19,7 @@ import { browserSessionRegistry } from '../browser/browser-session-registry'
import { translateMain } from '../i18n/main-i18n'
import { normalizeBrowserNavigationUrl } from '../../shared/browser-url'
import { ORCA_BROWSER_GUEST_WEB_PREFERENCES } from '../../shared/browser-guest-web-preferences'
import { BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD } from '../../shared/browser-window-close-policy'
import { isCrashReportReason } from '../../shared/crash-reporting'
import {
DEFAULT_RENDERER_RECOVERY_MAX_RECOVERIES,
@ -441,6 +443,8 @@ export function createMainWindow(
// so register it with the window's other navigation policy.
registerPluginPanelNavigationGuard(mainWindow.webContents)
const browserWindowClosePreload = join(__dirname, 'browser-window-close-preload.js')
const browserWindowCloseAllowedPreloadPath = fileURLToPath(BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD)
mainWindow.webContents.on('will-attach-webview', (event, webPreferences, params) => {
const src = typeof params.src === 'string' ? params.src : ''
const normalizedSrc = normalizeBrowserNavigationUrl(src)
@ -452,7 +456,18 @@ export function createMainWindow(
return
}
delete webPreferences.preload
const allowWindowClose = [params.preload, webPreferences.preload].some(
(preload) =>
preload === BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD ||
preload === browserWindowCloseAllowedPreloadPath
)
delete params.preload
if (allowWindowClose) {
delete webPreferences.preload
} else {
// Why: preload runs in the page's main world before inline scripts can call window.close().
webPreferences.preload = browserWindowClosePreload
}
// Why: older Electron builds expose preloadURL alongside preload; delete both so the guest can't inherit the main preload bridge.
delete (webPreferences as Record<string, unknown>).preloadURL
webPreferences.nodeIntegration = false

View File

@ -0,0 +1,15 @@
export function installBrowserWindowCloseGuard(): void {
const ignoreWindowClose = (): void => {}
try {
Object.defineProperty(window, 'close', {
configurable: false,
enumerable: false,
writable: false,
value: ignoreWindowClose
})
} catch {
try {
window.close = ignoreWindowClose
} catch {}
}
}

View File

@ -0,0 +1,20 @@
import { afterEach, describe, expect, it, vi } from 'vitest'
import { installBrowserWindowCloseGuard } from './browser-window-close-installation'
describe('browser window close preload', () => {
afterEach(() => {
vi.unstubAllGlobals()
})
it('installs a non-replaceable window.close no-op in the page world', () => {
const nativeClose = vi.fn()
vi.stubGlobal('window', { close: nativeClose })
installBrowserWindowCloseGuard()
expect(window.close()).toBeUndefined()
expect(window.close).not.toBe(nativeClose)
expect(Reflect.set(window, 'close', nativeClose)).toBe(false)
})
})

View File

@ -0,0 +1,7 @@
import { installBrowserWindowCloseGuard } from './browser-window-close-installation'
import type { ContextBridge } from 'electron'
// Why: raw require keeps the sandboxed preload standalone in the main-process CJS build.
const { contextBridge } = require('electron') as { contextBridge: ContextBridge }
contextBridge.executeInMainWorld({ func: installBrowserWindowCloseGuard })

View File

@ -3693,6 +3693,7 @@ function BrowserPagePane({
container,
inputLocked: inputLockedRef.current,
webviewPartition,
allowWindowClose: browserTab.allowWindowClose === true,
resolveContainer: () =>
ensureBrowserPageViewport(browserTab.id, workspaceId)?.container ?? null
})

View File

@ -1,6 +1,7 @@
// @vitest-environment happy-dom
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import { ORCA_BROWSER_GUEST_WEB_PREFERENCES_ATTRIBUTE } from '../../../../shared/browser-guest-web-preferences'
import { BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD } from '../../../../shared/browser-window-close-policy'
const registryMocks = vi.hoisted(() => ({
destroyPersistentWebview: vi.fn(),
@ -103,4 +104,22 @@ describe('BrowserPane webview preferences', () => {
expect(ensuredWebview?.webview.style.pointerEvents).toBe('none')
expect(refreshedContainer.lastElementChild).toBe(ensuredWebview?.webview as unknown as Element)
})
it('marks explicitly allowed CLI pages before the guest attaches', () => {
const container = createContainer('cli-page')
const ensuredWebview = ensureBrowserPageWebview({
browserTabId: 'browser-page-cli',
container,
inputLocked: false,
webviewPartition: 'persist:orca-browser',
allowWindowClose: true,
resolveContainer: () => container
})
expect(ensuredWebview?.webview.getAttribute('preload')).toBe(
BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD
)
expect(BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD).toMatch(/^file:/)
})
})

View File

@ -1,4 +1,5 @@
import { ORCA_BROWSER_GUEST_WEB_PREFERENCES_ATTRIBUTE } from '../../../../shared/browser-guest-web-preferences'
import { BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD } from '../../../../shared/browser-window-close-policy'
import {
destroyPersistentWebview,
registerPersistentWebview,
@ -10,12 +11,14 @@ export function ensureBrowserPageWebview({
container,
inputLocked,
webviewPartition,
allowWindowClose,
resolveContainer
}: {
browserTabId: string
container: HTMLDivElement
inputLocked: boolean
webviewPartition: string
allowWindowClose?: boolean
resolveContainer: () => HTMLDivElement | null
}): { container: HTMLDivElement; created: boolean; webview: Electron.WebviewTag } | null {
let webview = webviewRegistry.get(browserTabId)
@ -46,6 +49,10 @@ export function ensureBrowserPageWebview({
webview = document.createElement('webview') as Electron.WebviewTag
webview.setAttribute('partition', webviewPartition)
if (allowWindowClose) {
// Why: main consumes and removes this marker in will-attach-webview before the guest sees any preload.
webview.setAttribute('preload', BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD)
}
webview.setAttribute('allowpopups', '')
// Why: Electron spreads the webpreferences keys verbatim, so the shared
// camelCase attribute must stay intact for fullscreen containment to work.

View File

@ -1077,6 +1077,11 @@ describe('useIpcEvents browser tab create routing', () => {
expect(acquireBrowserAutomationVisibility).toHaveBeenCalledWith('page-new')
expect(acquireBrowserAutomationVisibility).not.toHaveBeenCalledWith('page-active')
expect(state.createBrowserTab).toHaveBeenCalledWith(
'wt-1',
'https://example.com',
expect.objectContaining({ allowWindowClose: true })
)
expect(replyTabCreate).toHaveBeenCalledWith({
requestId: 'req-create',
browserPageId: 'page-new'

View File

@ -2266,6 +2266,7 @@ export function useIpcEvents(): void {
// Agent/automation opens stay in the background (activate:false) in the active browser group.
const workspace = store.createBrowserTab(worktreeId, data.url, {
title: data.url,
allowWindowClose: true,
targetGroupId: data.activate ? undefined : activeBrowserUnifiedTab?.groupId,
sessionProfileId: data.sessionProfileId,
sessionPartition: data.sessionPartition,

View File

@ -57,6 +57,7 @@ import { buildValidWorktreeIdsForSessionHydration } from './degraded-repo-worktr
type CreateBrowserTabOptions = {
activate?: boolean
title?: string
allowWindowClose?: boolean
sessionProfileId?: string | null
sessionPartition?: string | null
// Place the new tab in a specific group (e.g. "Open Preview to the Side"); defaults to the worktree's active group.
@ -69,6 +70,7 @@ type CreateBrowserTabOptions = {
type CreateBrowserPageOptions = {
activate?: boolean
title?: string
allowWindowClose?: boolean
browserRuntimeEnvironmentId?: string | null
}
@ -341,7 +343,8 @@ function buildBrowserPage(
worktreeId: string,
url: string,
title?: string,
browserRuntimeEnvironmentId?: string | null
browserRuntimeEnvironmentId?: string | null,
allowWindowClose?: boolean
): BrowserPage {
const normalizedUrl = normalizeUrl(url)
return {
@ -357,6 +360,7 @@ function buildBrowserPage(
canGoForward: false,
loadError: null,
createdAt: Date.now(),
...(allowWindowClose !== undefined ? { allowWindowClose } : {}),
...(browserRuntimeEnvironmentId !== undefined ? { browserRuntimeEnvironmentId } : {})
}
}
@ -554,7 +558,8 @@ export const createBrowserSlice: StateCreator<AppState, [], [], BrowserSlice> =
worktreeId,
url,
options?.title,
options?.browserRuntimeEnvironmentId
options?.browserRuntimeEnvironmentId,
options?.allowWindowClose
)
// Why: with no explicit profile, inherit the user's default so a Settings preference applies to new tabs.
const sessionProfileId =
@ -902,13 +907,15 @@ export const createBrowserSlice: StateCreator<AppState, [], [], BrowserSlice> =
activate: true,
sessionProfileId,
sessionPartition,
browserRuntimeEnvironmentId: firstPage.browserRuntimeEnvironmentId
browserRuntimeEnvironmentId: firstPage.browserRuntimeEnvironmentId,
allowWindowClose: firstPage.allowWindowClose
})
for (const p of restPages) {
get().createBrowserPage(restored.id, p.url, {
activate: false,
title: p.title,
allowWindowClose: p.allowWindowClose,
browserRuntimeEnvironmentId: p.browserRuntimeEnvironmentId
})
}
@ -987,7 +994,8 @@ export const createBrowserSlice: StateCreator<AppState, [], [], BrowserSlice> =
workspace.worktreeId,
url,
options?.title,
options?.browserRuntimeEnvironmentId
options?.browserRuntimeEnvironmentId,
options?.allowWindowClose
)
set((s) => {
@ -1169,6 +1177,7 @@ export const createBrowserSlice: StateCreator<AppState, [], [], BrowserSlice> =
return get().createBrowserPage(workspaceId, pageToRestore.url, {
title: pageToRestore.title,
activate: true,
allowWindowClose: pageToRestore.allowWindowClose,
browserRuntimeEnvironmentId: pageToRestore.browserRuntimeEnvironmentId
})
},

View File

@ -0,0 +1,2 @@
// Why: this marker carries the CLI's explicit close policy across <webview> attach before the guest exists.
export const BROWSER_WINDOW_CLOSE_ALLOWED_PRELOAD = 'file:///__orca_window_close_allowed__'

View File

@ -982,6 +982,8 @@ export type BrowserPage = {
canGoForward: boolean
loadError: BrowserLoadError | null
createdAt: number
// Why: CLI-created pages retain Chromium's native window.close behavior; ordinary embedded pages are guarded.
allowWindowClose?: boolean
// Why: remote-owned worktrees can still host client-local fallback browser
// pages until headless remote runtimes support real browser panes.
browserRuntimeEnvironmentId?: string | null

View File

@ -225,6 +225,7 @@ const browserPageSchema = z.object({
canGoForward: z.boolean(),
loadError: browserLoadErrorSchema.nullable(),
createdAt: z.number(),
allowWindowClose: z.boolean().optional(),
// Why: explicit null marks a browser page as client-local even when its
// worktree is remote-owned; older sessions omit it and keep inferred runtime.
browserRuntimeEnvironmentId: z.string().nullable().optional(),

View File

@ -29,10 +29,11 @@ async function createBrowserTab(
page: Parameters<typeof getActiveWorktreeId>[0],
worktreeId: string,
url?: string,
title = 'New Browser Tab'
title = 'New Browser Tab',
allowWindowClose?: boolean
): Promise<CreatedBrowserTab | null> {
return page.evaluate(
({ targetWorktreeId, targetUrl, targetTitle }) => {
({ targetWorktreeId, targetUrl, targetTitle, allowClose }) => {
const store = window.__store
if (!store) {
return null
@ -44,12 +45,18 @@ async function createBrowserTab(
targetUrl ?? state.browserDefaultUrl ?? 'about:blank',
{
title: targetTitle,
activate: true
activate: true,
allowWindowClose: allowClose
}
)
return { id: tab.id, pageId: tab.activePageId ?? null }
},
{ targetWorktreeId: worktreeId, targetUrl: url, targetTitle: title }
{
targetWorktreeId: worktreeId,
targetUrl: url,
targetTitle: title,
allowClose: allowWindowClose
}
)
}
@ -199,6 +206,71 @@ async function startBrowserLinkServer(): Promise<{
}
}
async function startBrowserWindowCloseServer(): Promise<{
url: string
sourceUrl: string
close: () => Promise<void>
}> {
const server = createServer((request, response) => {
const origin = `http://127.0.0.1:${(server.address() as AddressInfo).port}`
const pathname = new URL(request.url ?? '/', origin).pathname
response.writeHead(200, { 'Content-Type': 'text/html; charset=utf-8' })
if (pathname === '/source') {
response.end(`
<!doctype html>
<html>
<head><title>Close link source</title></head>
<body><a id="window-close-link" href="${origin}/window-close" target="_blank">Open close page</a></body>
</html>
`)
return
}
response.end(`
<!doctype html>
<html>
<head><title>Window close repro</title></head>
<body>
<p id="s">Attempting close</p>
<script>
window.close()
setTimeout(() => {
document.getElementById('s').textContent =
'window.close() was blocked (expected).'
}, 200)
</script>
</body>
</html>
`)
})
await new Promise<void>((resolve) => server.listen(0, '127.0.0.1', resolve))
const port = (server.address() as AddressInfo).port
return {
url: `http://127.0.0.1:${port}/window-close`,
sourceUrl: `http://127.0.0.1:${port}/source`,
close: () => closeServer(server)
}
}
async function readBrowserWindowCloseStatus(
page: Parameters<typeof getActiveWorktreeId>[0],
browserTabId: string
): Promise<string> {
return page.evaluate(async (targetBrowserTabId) => {
const slot = document.querySelector(`[data-browser-overlay-tab-id="${targetBrowserTabId}"]`)
const webview = slot?.querySelector('webview') as Electron.WebviewTag | null
if (!webview) {
return 'webview missing'
}
try {
return (await webview.executeJavaScript(
'document.querySelector("#s")?.textContent ?? "status missing"'
)) as string
} catch {
return 'guest lost'
}
}, browserTabId)
}
async function closeServer(server: Server): Promise<void> {
await new Promise<void>((resolve, reject) =>
server.close((error) => {
@ -689,6 +761,88 @@ test.describe('Browser Tab', () => {
}
})
test('blocked window.close in a link-created tab does not break tab switching', async ({
orcaPage
}) => {
const closeServer = await startBrowserWindowCloseServer()
try {
const worktreeId = (await getActiveWorktreeId(orcaPage))!
const neighboringTab = await createBrowserTab(
orcaPage,
worktreeId,
'about:blank',
'Neighboring tab'
)
const sourceTab = await createBrowserTab(
orcaPage,
worktreeId,
closeServer.sourceUrl,
'Close link source'
)
expect(neighboringTab?.id).toBeTruthy()
expect(sourceTab?.id).toBeTruthy()
await clickBrowserLink(orcaPage, sourceTab!.id, '#window-close-link')
let closeTabId: string | null = null
await expect
.poll(async () => {
const tabs = await getBrowserTabs(orcaPage, worktreeId)
closeTabId = tabs.find((tab) => tab.url === closeServer.url)?.id ?? null
return closeTabId
})
.not.toBeNull()
await orcaPage.locator(`[data-tab-id="${neighboringTab!.id}"]`).click()
await expect.poll(async () => getActiveTabType(orcaPage), { timeout: 5_000 }).toBe('browser')
await expect
.poll(() => readBrowserWindowCloseStatus(orcaPage, closeTabId!), { timeout: 5_000 })
.toContain('window.close() was blocked')
} finally {
await closeServer.close()
}
})
test('explicitly allowed browser tabs retain replaceable window.close', async ({ orcaPage }) => {
const closeServer = await startBrowserWindowCloseServer()
try {
const worktreeId = (await getActiveWorktreeId(orcaPage))!
const allowedTab = await createBrowserTab(
orcaPage,
worktreeId,
closeServer.sourceUrl,
'Allowed close tab',
true
)
expect(allowedTab?.id).toBeTruthy()
await expect
.poll(
() =>
orcaPage.evaluate(async (targetBrowserTabId) => {
const slot = document.querySelector(
`[data-browser-overlay-tab-id="${targetBrowserTabId}"]`
)
const webview = slot?.querySelector('webview') as Electron.WebviewTag | null
if (!webview) {
return 'webview missing'
}
try {
return (await webview.executeJavaScript(`(() => {
window.close = () => 'replacement-called'
return window.close() === 'replacement-called' ? 'replacement-called' : 'blocked'
})()`)) as string
} catch {
return 'guest unavailable'
}
}, allowedTab!.id),
{ timeout: 5_000 }
)
.toBe('replacement-called')
} finally {
await closeServer.close()
}
})
/**
* User Prompt:
* - Browser works and also retains state when switching tabs etc.