From d3d5ad4d9fa48b2bade0b47886096fbca14f236f Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 11 Jul 2026 19:22:06 -0700 Subject: [PATCH] Revert "fix(browser): narrow native popup allow to genuine OAuth popups" This reverts commit fe1486f01165bb59d02f415d3565df0535d6908b. --- src/main/browser/browser-manager.test.ts | 122 +---------------------- src/main/browser/browser-manager.ts | 12 +-- 2 files changed, 7 insertions(+), 127 deletions(-) diff --git a/src/main/browser/browser-manager.test.ts b/src/main/browser/browser-manager.test.ts index 33d0919f53d..c854d1f0acb 100644 --- a/src/main/browser/browser-manager.test.ts +++ b/src/main/browser/browser-manager.test.ts @@ -162,17 +162,11 @@ describe('browserManager', () => { const handler = guestSetWindowOpenHandlerMock.mock.calls[0][0] as (details: { url: string features?: string - disposition?: string }) => { action: 'allow' | 'deny' } - // Why: 'new-window' is Chromium's disposition for a windowed popup - // (window.open with features) — the OAuth pattern that earns a native child. - expect(handler({ url: 'about:blank', disposition: 'new-window' })).toMatchObject({ - action: 'allow' - }) + expect(handler({ url: 'about:blank' })).toMatchObject({ action: 'allow' }) expect( handler({ url: 'https://example.com/login', - disposition: 'new-window', features: 'alwaysOnTop=yes,frame=no,fullscreen=yes,kiosk=yes,modal=yes,transparent=yes' }) ).toEqual({ @@ -199,99 +193,6 @@ describe('browserManager', () => { expect(rendererSendMock).not.toHaveBeenCalled() }) - it('routes generic target=_blank navigations to the OS browser instead of a native child', () => { - const rendererSendMock = vi.fn() - const guest = { - id: 1103, - isDestroyed: vi.fn(() => false), - getType: vi.fn(() => 'webview'), - setBackgroundThrottling: guestSetBackgroundThrottlingMock, - setWindowOpenHandler: guestSetWindowOpenHandlerMock, - on: guestOnMock, - off: guestOffMock, - openDevTools: guestOpenDevToolsMock - } - webContentsFromIdMock.mockImplementation((id: number) => { - if (id === guest.id) { - return guest - } - if (id === rendererWebContentsId) { - return { isDestroyed: vi.fn(() => false), send: rendererSendMock } - } - return null - }) - - browserManager.attachGuestPolicies(guest as never) - browserManager.registerGuest({ - browserPageId: 'browser-1', - webContentsId: guest.id, - rendererWebContentsId - }) - - const handler = guestSetWindowOpenHandlerMock.mock.calls[0][0] as (details: { - url: string - disposition?: string - }) => { action: 'allow' | 'deny' } - - // A plain target=_blank link (foreground-tab) must not earn native chrome - // even though the guest owns a registered browser tab. - expect( - handler({ url: 'https://evil.example.com/spam', disposition: 'foreground-tab' }) - ).toEqual({ action: 'deny' }) - expect(shellOpenExternalMock).toHaveBeenCalledWith('https://evil.example.com/spam') - expect(rendererSendMock).toHaveBeenCalledWith('browser:popup', { - browserPageId: 'browser-1', - origin: 'https://evil.example.com', - action: 'opened-external' - }) - - // A background-tab (ctrl/cmd+click) navigation is treated the same way. - expect( - handler({ url: 'https://other.example.com/page', disposition: 'background-tab' }) - ).toEqual({ action: 'deny' }) - expect(shellOpenExternalMock).toHaveBeenCalledWith('https://other.example.com/page') - }) - - it('does not grant a native child for a registered guest when disposition is absent', () => { - const rendererSendMock = vi.fn() - const guest = { - id: 1104, - isDestroyed: vi.fn(() => false), - getType: vi.fn(() => 'webview'), - setBackgroundThrottling: guestSetBackgroundThrottlingMock, - setWindowOpenHandler: guestSetWindowOpenHandlerMock, - on: guestOnMock, - off: guestOffMock, - openDevTools: guestOpenDevToolsMock - } - webContentsFromIdMock.mockImplementation((id: number) => { - if (id === guest.id) { - return guest - } - if (id === rendererWebContentsId) { - return { isDestroyed: vi.fn(() => false), send: rendererSendMock } - } - return null - }) - - browserManager.attachGuestPolicies(guest as never) - browserManager.registerGuest({ - browserPageId: 'browser-1', - webContentsId: guest.id, - rendererWebContentsId - }) - - const handler = guestSetWindowOpenHandlerMock.mock.calls[0][0] as (details: { - url: string - disposition?: string - }) => { action: 'allow' | 'deny' } - - // Why: only an explicit 'new-window' disposition is treated as a popup; - // anything else falls through to the external-browser fallback. - expect(handler({ url: 'https://example.com/login' })).toEqual({ action: 'deny' }) - expect(shellOpenExternalMock).toHaveBeenCalledWith('https://example.com/login') - }) - it('blocks unsafe popup URLs for registered guests', () => { const rendererSendMock = vi.fn() const guest = { @@ -1087,19 +988,11 @@ describe('browserManager', () => { const childWindowOpenHandler = childSetWindowOpenHandlerMock.mock.calls[0][0] as (details: { url: string - disposition?: string }) => { action: 'allow' | 'deny' } - expect( - childWindowOpenHandler({ - url: 'https://identity.example.com/login', - disposition: 'new-window' - }) - ).toMatchObject({ + expect(childWindowOpenHandler({ url: 'https://identity.example.com/login' })).toMatchObject({ action: 'allow' }) - expect( - childWindowOpenHandler({ url: 'file:///etc/passwd', disposition: 'new-window' }) - ).toEqual({ action: 'deny' }) + expect(childWindowOpenHandler({ url: 'file:///etc/passwd' })).toEqual({ action: 'deny' }) expect(rendererSendMock).toHaveBeenCalledWith('browser:popup', { browserPageId: 'browser-1', origin: 'null', @@ -1171,14 +1064,9 @@ describe('browserManager', () => { childDidCreateWindowHandler?.({ webContents: cleanupChildGuest }) expect(managerState.popupOwnerContextByGuestId.has(cleanupChildGuest.id)).toBe(true) const cleanupChildWindowOpenHandler = cleanupChildGuest.setWindowOpenHandler.mock - .calls[0][0] as (details: { url: string; disposition?: string }) => { - action: 'allow' | 'deny' - } + .calls[0][0] as (details: { url: string }) => { action: 'allow' | 'deny' } expect( - cleanupChildWindowOpenHandler({ - url: 'https://identity.example.com/continue', - disposition: 'new-window' - }) + cleanupChildWindowOpenHandler({ url: 'https://identity.example.com/continue' }) ).toMatchObject({ action: 'allow' }) const cleanupChildDestroyedHandler = cleanupChildOnMock.mock.calls.find( ([event]) => event === 'destroyed' diff --git a/src/main/browser/browser-manager.ts b/src/main/browser/browser-manager.ts index bc222ca66d2..18cddd8c78c 100644 --- a/src/main/browser/browser-manager.ts +++ b/src/main/browser/browser-manager.ts @@ -626,7 +626,7 @@ export class BrowserManager { this.attachGuestPolicies(window.webContents, this.resolvePopupOwnerContext(guest.id)) } guest.on('did-create-window', handleDidCreateWindow) - guest.setWindowOpenHandler(({ url, disposition }) => { + guest.setWindowOpenHandler(({ url }) => { const browserTabId = this.resolveBrowserTabIdForGuestWebContentsId(guest.id) const browserUrl = normalizeBrowserNavigationUrl(url) const externalUrl = normalizeExternalBrowserUrl(url) @@ -634,15 +634,7 @@ export class BrowserManager { // Why: file URLs are valid for user-opened in-pane previews, but remote // content must not create native child windows targeting local paths. const canOpenAsChild = Boolean(externalUrl || browserUrl === ORCA_BROWSER_BLANK_URL) - // Why: only grant a native Electron child to genuine popups. Chromium - // reports 'new-window' when a page calls window.open() with window - // features (or the user shift-clicks) — the OAuth/consent-dialog pattern. - // Ordinary target=_blank clicks and featureless window.open() arrive as - // 'foreground-tab'/'background-tab'; letting those spawn native windows - // would let any embedded page create deceptive or spammy Orca chrome, so - // route them to the OS browser (external branch) instead. - const isNativePopupRequest = disposition === 'new-window' - if (browserTabId && canOpenAsChild && isNativePopupRequest) { + if (browserTabId && canOpenAsChild) { // Why: OAuth may request ordinary size/position features, but browser // content must not create deceptive or inescapable native chrome. return { action: 'allow', overrideBrowserWindowOptions: SAFE_POPUP_WINDOW_OPTIONS }