From 3e112a2e4724e05593da63e12c0322afb00bed1d Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Wed, 28 Oct 2026 20:38:32 -0700 Subject: [PATCH] fix(browser): keep native popup admission within opener session (#35397) Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> --- ...owser-manager-popup-openerless-tab.test.ts | 10 +-- .../browser-manager-popup-routing.test.ts | 2 +- .../browser-popup-native-window.test.ts | 66 +++++++++++++------ .../browser/browser-popup-native-window.ts | 17 +++-- 4 files changed, 64 insertions(+), 31 deletions(-) diff --git a/src/main/browser/browser-manager-popup-openerless-tab.test.ts b/src/main/browser/browser-manager-popup-openerless-tab.test.ts index d2a2e7fa15a..7f65e843d34 100644 --- a/src/main/browser/browser-manager-popup-openerless-tab.test.ts +++ b/src/main/browser/browser-manager-popup-openerless-tab.test.ts @@ -2,7 +2,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' const browserMocks = vi.hoisted(() => ({ appGetPathMock: vi.fn(() => '/downloads'), - appPrependOnceListenerMock: vi.fn(), + appPrependListenerMock: vi.fn(), shellOpenExternalMock: vi.fn(), browserWindowFromWebContentsMock: vi.fn(), menuBuildFromTemplateMock: vi.fn(), @@ -18,7 +18,7 @@ const browserMocks = vi.hoisted(() => ({ vi.mock('electron', () => ({ app: { - prependOnceListener: browserMocks.appPrependOnceListenerMock, + prependListener: browserMocks.appPrependListenerMock, off: vi.fn(), getPath: browserMocks.appGetPathMock }, @@ -42,7 +42,7 @@ import { } from './browser-manager-test-harness' const { - appPrependOnceListenerMock, + appPrependListenerMock, guestOnMock, guestSetWindowOpenHandlerMock, webContentsFromIdMock, @@ -123,7 +123,7 @@ describe('tab-shaped OAuth opens without an opener', () => { beforeEach(() => { resetBrowserManagerMocks(browserMocks) resetBrowserManagerState() - appPrependOnceListenerMock.mockReset() + appPrependListenerMock.mockReset() rendererSendMock.mockReset() openPopupWithOriginBarMock.mockReturnValue({ contentWebContents: popupContents(null), @@ -198,7 +198,7 @@ describe('tab-shaped OAuth opens without an opener', () => { }) expect(response).toMatchObject({ action: 'allow' }) const onWindowCreated: (event: unknown, window: unknown) => void = - appPrependOnceListenerMock.mock.calls.at(-1)?.[1] + appPrependListenerMock.mock.calls.at(-1)?.[1] const nativeWindow = { webContents: popupContents(null), isDestroyed: vi.fn(() => false), diff --git a/src/main/browser/browser-manager-popup-routing.test.ts b/src/main/browser/browser-manager-popup-routing.test.ts index e2df6c0b766..976c2278f74 100644 --- a/src/main/browser/browser-manager-popup-routing.test.ts +++ b/src/main/browser/browser-manager-popup-routing.test.ts @@ -17,7 +17,7 @@ const browserMocks = vi.hoisted(() => ({ vi.mock('electron', () => ({ app: { - prependOnceListener: vi.fn(), + prependListener: vi.fn(), off: vi.fn(), getPath: browserMocks.appGetPathMock }, diff --git a/src/main/browser/browser-popup-native-window.test.ts b/src/main/browser/browser-popup-native-window.test.ts index 4a6c8625768..f360ce2e9d7 100644 --- a/src/main/browser/browser-popup-native-window.test.ts +++ b/src/main/browser/browser-popup-native-window.test.ts @@ -1,5 +1,6 @@ import { EventEmitter } from 'node:events' import { afterEach, describe, expect, it, vi } from 'vitest' +import { isNativeBrowserPopupWindow } from './native-browser-popup-windows' const { events } = await vi.hoisted(async () => { const { EventEmitter } = await import('node:events') @@ -212,27 +213,50 @@ describe('native popup admission', () => { expect(onFirst).not.toHaveBeenCalled() }) - it.fails.each([true, false])( - 'KNOWN BLOCKER: an unrelated app window receives neither policy nor disposal (same session: %s)', - async (sameSession) => { - const opener = nativeWindow() - const unrelated = nativeWindow() - if (sameSession) { - unrelated.contents.session = opener.contents.session - } - const onCreated = vi.fn(() => unrelated.browserWindow.webContents) - allowNativeBrowserPopup({ - opener: opener.browserWindow.webContents, - options: { show: false }, - onCreated - }) - events.emit('browser-window-created', {}, unrelated.browserWindow) - await Promise.resolve() - expect(onCreated).not.toHaveBeenCalled() - expect(unrelated.contents.close).not.toHaveBeenCalled() - expect(unrelated.window.destroy).not.toHaveBeenCalled() - } - ) + it('ignores a different-session app window without consuming the legitimate child admission', async () => { + const opener = nativeWindow() + const unrelated = nativeWindow() + const child = nativeWindow() + child.contents.session = opener.contents.session + const onCreated = vi.fn(() => child.browserWindow.webContents) + allowNativeBrowserPopup({ + opener: opener.browserWindow.webContents, + options: { show: false }, + onCreated + }) + events.emit('browser-window-created', {}, unrelated.browserWindow) + expect(onCreated).not.toHaveBeenCalled() + expect(isNativeBrowserPopupWindow(unrelated.browserWindow)).toBe(false) + events.emit('browser-window-created', {}, child.browserWindow) + await Promise.resolve() + opener.contents.emit('did-create-window', child.browserWindow) + expect(onCreated).toHaveBeenCalledExactlyOnceWith( + expect.objectContaining({ nativeWindow: child.browserWindow }) + ) + expect(isNativeBrowserPopupWindow(child.browserWindow)).toBe(true) + expect(isNativeBrowserPopupWindow(unrelated.browserWindow)).toBe(false) + expect(unrelated.contents.close).not.toHaveBeenCalled() + expect(unrelated.window.destroy).not.toHaveBeenCalled() + expect(child.contents.close).not.toHaveBeenCalled() + expect(child.window.destroy).not.toHaveBeenCalled() + }) + + it.fails('KNOWN BLOCKER: an unrelated same-session app window receives neither policy nor disposal', async () => { + const opener = nativeWindow() + const unrelated = nativeWindow() + unrelated.contents.session = opener.contents.session + const onCreated = vi.fn(() => unrelated.browserWindow.webContents) + allowNativeBrowserPopup({ + opener: opener.browserWindow.webContents, + options: { show: false }, + onCreated + }) + events.emit('browser-window-created', {}, unrelated.browserWindow) + await Promise.resolve() + expect(onCreated).not.toHaveBeenCalled() + expect(unrelated.contents.close).not.toHaveBeenCalled() + expect(unrelated.window.destroy).not.toHaveBeenCalled() + }) it('handles duplicate did-create-window delivery idempotently for an already marked child', async () => { const opener = nativeWindow() diff --git a/src/main/browser/browser-popup-native-window.ts b/src/main/browser/browser-popup-native-window.ts index 7b73f6cb176..8dff4aa1e2a 100644 --- a/src/main/browser/browser-popup-native-window.ts +++ b/src/main/browser/browser-popup-native-window.ts @@ -36,11 +36,12 @@ export function allowNativeBrowserPopup(input: { canPresent?: () => boolean onCreated: (options: PopupChildWindowOptions) => WebContents }): Electron.WindowOpenHandlerResponse { + const openerSession = input.opener.session const mayPresent = !isBackgroundLaunch() && input.options.show !== false const admit = (window: BrowserWindow): void => { markNativeBrowserPopupWindow(window) try { - if (input.opener.isDestroyed() || window.webContents.session !== input.opener.session) { + if (input.opener.isDestroyed() || window.webContents.session !== openerSession) { rejectNativeBrowserPopup(window) return } @@ -55,7 +56,7 @@ export function allowNativeBrowserPopup(input: { window.isDestroyed() || window.webContents.isDestroyed() || input.opener.isDestroyed() || - window.webContents.session !== input.opener.session || + window.webContents.session !== openerSession || (input.canPresent && !input.canPresent()) ) { return @@ -70,11 +71,19 @@ export function allowNativeBrowserPopup(input: { } let admitted = false const onCreated = (_event: Electron.Event, window: BrowserWindow): void => { + if ( + window.isDestroyed() || + window.webContents.isDestroyed() || + window.webContents.session !== openerSession + ) { + return + } + app.off('browser-window-created', onCreated) admitted = true admit(window) } // Prepend prevents another browser-window-created listener from navigating the child first. - app.prependOnceListener('browser-window-created', onCreated) + app.prependListener('browser-window-created', onCreated) // A canceled native creation must not grant the next unrelated window this admission. queueMicrotask(() => { app.off('browser-window-created', onCreated) @@ -93,7 +102,7 @@ export function allowNativeBrowserPopup(input: { useContentSize: true, minWidth: MIN_POPUP_CONTENT_WIDTH, minHeight: MIN_POPUP_CONTENT_HEIGHT, - webPreferences: { ...input.options.webPreferences, session: input.opener.session } + webPreferences: { ...input.options.webPreferences, session: openerSession } } } }