diff --git a/src/main/browser/browser-manager.test.ts b/src/main/browser/browser-manager.test.ts index c854d1f0acb..33d0919f53d 100644 --- a/src/main/browser/browser-manager.test.ts +++ b/src/main/browser/browser-manager.test.ts @@ -162,11 +162,17 @@ describe('browserManager', () => { const handler = guestSetWindowOpenHandlerMock.mock.calls[0][0] as (details: { url: string features?: string + disposition?: string }) => { action: 'allow' | 'deny' } - expect(handler({ url: 'about:blank' })).toMatchObject({ action: 'allow' }) + // 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: 'https://example.com/login', + disposition: 'new-window', features: 'alwaysOnTop=yes,frame=no,fullscreen=yes,kiosk=yes,modal=yes,transparent=yes' }) ).toEqual({ @@ -193,6 +199,99 @@ 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 = { @@ -988,11 +1087,19 @@ 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' })).toMatchObject({ + expect( + childWindowOpenHandler({ + url: 'https://identity.example.com/login', + disposition: 'new-window' + }) + ).toMatchObject({ action: 'allow' }) - expect(childWindowOpenHandler({ url: 'file:///etc/passwd' })).toEqual({ action: 'deny' }) + expect( + childWindowOpenHandler({ url: 'file:///etc/passwd', disposition: 'new-window' }) + ).toEqual({ action: 'deny' }) expect(rendererSendMock).toHaveBeenCalledWith('browser:popup', { browserPageId: 'browser-1', origin: 'null', @@ -1064,9 +1171,14 @@ 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 }) => { action: 'allow' | 'deny' } + .calls[0][0] as (details: { url: string; disposition?: string }) => { + action: 'allow' | 'deny' + } expect( - cleanupChildWindowOpenHandler({ url: 'https://identity.example.com/continue' }) + cleanupChildWindowOpenHandler({ + url: 'https://identity.example.com/continue', + disposition: 'new-window' + }) ).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 18cddd8c78c..bc222ca66d2 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 }) => { + guest.setWindowOpenHandler(({ url, disposition }) => { const browserTabId = this.resolveBrowserTabIdForGuestWebContentsId(guest.id) const browserUrl = normalizeBrowserNavigationUrl(url) const externalUrl = normalizeExternalBrowserUrl(url) @@ -634,7 +634,15 @@ 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) - if (browserTabId && canOpenAsChild) { + // 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) { // 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 }