From 0cb77edaa32ae18e8314c2ecc7963102e80f19ad Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 7 Aug 2026 00:47:08 -0700 Subject: [PATCH] fix(browser): correct auth-UA popup opt-out and viewport override tracking Three corrections to the auth-host UA work: - Popup child windows get attachGuestPolicies but are never in tabIdByWebContentsId, so the native-UA opt-out missed them. A native profile's popup was handed the Firefox UA while its session skips setupClientHintsOverride entirely, sending the raw Electron UA on the wire. Resolve the mode through the popup's owner tab. - setViewportOverride mutated its tracking map before the CDP writes landed. A failed clear stranded a still-standing override with no map entry, so navigation could never correct it. Mutate only after the writes succeed. - Drop the viewport op queue added for a cross-writer race that does not exist: debugger.sendCommand dispatches in call order, so the later write always wins. The real defect was post-await staleness in the resolved URL, which the shared navigation target already fixes. --- src/main/browser/browser-manager.test.ts | 115 ++++++++++++++++++++++- src/main/browser/browser-manager.ts | 111 +++++++++++----------- 2 files changed, 164 insertions(+), 62 deletions(-) diff --git a/src/main/browser/browser-manager.test.ts b/src/main/browser/browser-manager.test.ts index 30adfe83f75..1032fd2b68f 100644 --- a/src/main/browser/browser-manager.test.ts +++ b/src/main/browser/browser-manager.test.ts @@ -1910,6 +1910,65 @@ describe('browserManager', () => { expect(setUserAgent).not.toHaveBeenCalled() }) + // Why: popup child windows get attachGuestPolicies but are never entered into tabIdByWebContentsId, + // so a direct lookup of the UA mode misses the native opt-out. That is worse than doing nothing — + // native sessions skip setupClientHintsOverride, so the popup would send the raw Electron UA on the + // wire while navigator.userAgent claimed Firefox. Google sign-in popups are a first-class surface. + it('leaves the UA untouched on auth hosts for a popup owned by a native-UA profile', () => { + const ownerGuest = { + id: 415, + isDestroyed: vi.fn(() => false), + getType: vi.fn(() => 'webview'), + setBackgroundThrottling: guestSetBackgroundThrottlingMock, + setWindowOpenHandler: guestSetWindowOpenHandlerMock, + on: guestOnMock, + off: guestOffMock, + openDevTools: guestOpenDevToolsMock, + getURL: vi.fn(() => 'https://accounts.google.com/'), + getUserAgent: vi.fn(() => guestBaseUserAgent), + setUserAgent: vi.fn(), + session: { getUserAgent: vi.fn(() => guestBaseUserAgent) } + } + webContentsFromIdMock.mockReturnValue(ownerGuest) + browserManager.attachGuestPolicies(ownerGuest as never) + browserManager.registerGuest({ + browserPageId: 'browser-native-popup-owner', + webContentsId: ownerGuest.id, + rendererWebContentsId, + userAgentMode: 'native' + }) + + // The popup carries its own listeners so its handler is unambiguous. + const popupOn = vi.fn() + const popupSetUserAgent = vi.fn() + const popupGuest = { + id: 416, + isDestroyed: vi.fn(() => false), + getType: vi.fn(() => 'window'), + setBackgroundThrottling: guestSetBackgroundThrottlingMock, + setWindowOpenHandler: guestSetWindowOpenHandlerMock, + on: popupOn, + off: guestOffMock, + openDevTools: guestOpenDevToolsMock, + getURL: vi.fn(() => 'https://accounts.google.com/'), + getUserAgent: vi.fn(() => guestBaseUserAgent), + setUserAgent: popupSetUserAgent, + session: { getUserAgent: vi.fn(() => guestBaseUserAgent) } + } + browserManager.attachGuestPolicies(popupGuest as never, { + browserTabId: 'browser-native-popup-owner', + rootGuestWebContentsId: ownerGuest.id + }) + + const popupDidStartNavigation = popupOn.mock.calls.find( + ([event]) => event === 'did-start-navigation' + )?.[1] as (event: unknown, url: string, isInPlace: boolean, isMainFrame: boolean) => void + expect(popupDidStartNavigation).toBeDefined() + + popupDidStartNavigation(null, 'https://accounts.google.com/v3/signin/identifier', false, true) + expect(popupSetUserAgent).not.toHaveBeenCalled() + }) + it('queues permission denials and download requests until the guest registers', () => { const rendererSendMock = vi.fn() const guest = { @@ -3119,11 +3178,12 @@ describe('browserManager', () => { }) }) - // Why: setViewportOverride and the navigation path are two independent writers of the same CDP - // command. Verified against real Electron 43.1.0: did-start-navigation fires while an awaited - // sendCommand is still pending, and getURL() keeps reporting the OUTGOING page until commit — - // so a preset resuming mid-navigation resolves the wrong host unless both writers share a chain - // and a single URL source. + // Why: not an ordering race — debugger.sendCommand dispatches in call order over one channel, so + // the later-issued write always wins. The defect is post-await staleness: verified against real + // Electron 43.1.0, did-start-navigation fires while an awaited sendCommand is still pending, and + // getURL() keeps reporting the OUTGOING page until commit. A preset resuming mid-navigation + // therefore resolves the wrong host and wins with the wrong value. Both writers must resolve the + // host from the in-flight navigation target instead. function lastUserAgentOverride( debuggerSendCommand: ReturnType ): Record | undefined { @@ -3305,6 +3365,51 @@ describe('browserManager', () => { }) }) + // Why: a failed clear leaves the CDP override standing on the target. Dropping the tracking entry + // first makes it untracked, so the navigation path can never correct it again and the tab carries + // a Chrome-shaped navigator.userAgent onto the auth hosts — the exact failure this PR prevents. + it('keeps tracking the standing override when the CDP clear fails', async () => { + const { guest, debuggerSendCommand } = makeGuest(4254, 'https://example.com/') + webContentsFromIdMock.mockReturnValue(guest) + browserManager.attachGuestPolicies(guest as never) + browserManager.registerGuest({ + browserPageId: 'tab-failed-clear', + webContentsId: guest.id as number, + rendererWebContentsId + }) + const didStartNavigation = guestOnMock.mock.calls.find( + ([event]) => event === 'did-start-navigation' + )?.[1] as (event: unknown, url: string, isInPlace: boolean, isMainFrame: boolean) => void + + await browserManager.setViewportOverride('tab-failed-clear', { + width: 1024, + height: 768, + deviceScaleFactor: 1, + mobile: false + }) + await flushViewportOps() + + // The clear fails partway — e.g. DevTools took the debugger, or the guest is tearing down. + debuggerSendCommand.mockImplementation((method: string) => + method === 'Emulation.clearDeviceMetricsOverride' + ? Promise.reject(new Error('debugger detached')) + : Promise.resolve(undefined) + ) + await expect(browserManager.setViewportOverride('tab-failed-clear', null)).resolves.toBe( + false + ) + await flushViewportOps() + + // The override is still standing on the target, so navigation must still be able to correct it. + debuggerSendCommand.mockImplementation(() => Promise.resolve(undefined)) + debuggerSendCommand.mockClear() + didStartNavigation(null, 'https://accounts.google.com/v3/signin/identifier', false, true) + await flushViewportOps() + expect(debuggerSendCommand).toHaveBeenCalledWith('Emulation.setUserAgentOverride', { + userAgent: googleAuthUserAgent() + }) + }) + it('does not touch the UA override on navigation when no preset is standing', async () => { const { guest, debuggerSendCommand } = makeGuest(4248) webContentsFromIdMock.mockReturnValue(guest) diff --git a/src/main/browser/browser-manager.ts b/src/main/browser/browser-manager.ts index 54bc447ce82..11b767588a5 100644 --- a/src/main/browser/browser-manager.ts +++ b/src/main/browser/browser-manager.ts @@ -929,7 +929,12 @@ export class BrowserManager { // of the whole clean-UA path, so they keep their untouched identity everywhere. private applyGoogleAuthUserAgent(guest: Electron.WebContents, url: string): void { const browserPageId = this.tabIdByWebContentsId.get(guest.id) - const mode = browserPageId ? this.userAgentModeByPageId.get(browserPageId) : undefined + // Why: popup child windows get these policies but are never in tabIdByWebContentsId, so a direct + // lookup misses the native-UA opt-out and would hand a native profile's popup the Firefox UA. + // That is worse than doing nothing: native sessions skip setupClientHintsOverride entirely, so + // the popup would send the raw Electron UA on the wire while navigator.userAgent claims Firefox. + const ownerTabId = this.resolveBrowserTabIdForGuestWebContentsId(guest.id) + const mode = ownerTabId ? this.userAgentModeByPageId.get(ownerTabId) : undefined if (mode === 'native') { return } @@ -944,8 +949,10 @@ export class BrowserManager { // navigation never touches the session UA. guest.setUserAgent(guest.session.getUserAgent()) } + // Why: gate on the DIRECT page id, not ownerTabId — a popup has no device-metrics override of + // its own, so inheriting the owner tab's preset UA would pair a mobile UA with a desktop viewport. if (browserPageId) { - this.reapplyViewportUserAgentOverride(guest, browserPageId) + this.reapplyViewportUserAgentOverride(guest, browserPageId, url) } } @@ -963,63 +970,42 @@ export class BrowserManager { return this.pendingNavigationUrlByTabId.get(browserTabId) ?? guest.getURL() } - // Why: setViewportOverride and the navigation path both write Emulation.setUserAgentOverride for - // the same tab. Sharing one chain makes the last-enqueued write the one that lands; without it the - // two interleave and the tab can keep the Firefox UA off the auth hosts, or the Chrome-shaped - // preset UA while on them — the exact mismatch this scope exists to remove. - private enqueueViewportOp(browserTabId: string, op: () => Promise): Promise { - const prev = this.viewportOpsByTabId.get(browserTabId) ?? Promise.resolve() - const next = prev.catch(() => {}).then(op) - this.viewportOpsByTabId.set(browserTabId, next) - // Why: only clear if we're still the tail; a later call may have replaced the entry, and deleting would break serialization. - void next - .catch(() => {}) - .then(() => { - if (this.viewportOpsByTabId.get(browserTabId) === next) { - this.viewportOpsByTabId.delete(browserTabId) - } - }) - return next - } - // Why: Emulation.setUserAgentOverride is set once and stands across every later navigation, // outranking setUserAgent for navigator.userAgent. A viewport preset applied before reaching an // auth host would otherwise pin navigator.userAgent to the Chrome-shaped preset UA while the // request header says Firefox — the two-layer disagreement this scope exists to remove. private reapplyViewportUserAgentOverride( guest: Electron.WebContents, - browserTabId: string + browserTabId: string, + url: string ): void { - // Why: enqueue unconditionally — a setViewportOverride already queued ahead may be about to - // install (or clear) the override, so the standing-preset check belongs inside the chain. - void this.enqueueViewportOp(browserTabId, () => - this.sendViewportUserAgentOverride(guest, browserTabId) - ).catch(() => {}) - } - - private async sendViewportUserAgentOverride( - guest: Electron.WebContents, - browserTabId: string - ): Promise { const mobile = this.viewportUaOverrideMobileByTabId.get(browserTabId) if (mobile === undefined) { return } - try { - if (guest.isDestroyed() || !guest.debugger.isAttached()) { - return - } - await guest.debugger.sendCommand( - 'Emulation.setUserAgentOverride', - buildViewportUserAgentOverride({ - url: this.resolveTabNavigationUrl(guest, browserTabId), - mobile, - baseUserAgent: cleanElectronUserAgent(guest.getUserAgent()) - }) - ) - } catch { - // guest may be mid-teardown or the debugger taken over by DevTools + // Why: no queue needed — debugger.sendCommand dispatches in call order over one channel, so the + // later-issued write wins. What matters is that both writers resolve the SAME host, which they + // now do via the navigation target rather than the stale committed URL. + void this.sendViewportUserAgentOverride(guest, browserTabId, mobile, url).catch(() => {}) + } + + private async sendViewportUserAgentOverride( + guest: Electron.WebContents, + browserTabId: string, + mobile: boolean, + url?: string + ): Promise { + if (guest.isDestroyed() || !guest.debugger.isAttached()) { + return } + await guest.debugger.sendCommand( + 'Emulation.setUserAgentOverride', + buildViewportUserAgentOverride({ + url: url ?? this.resolveTabNavigationUrl(guest, browserTabId), + mobile, + baseUserAgent: cleanElectronUserAgent(guest.getUserAgent()) + }) + ) } private createPopupChildWindowWithOriginBar( @@ -1559,11 +1545,20 @@ export class BrowserManager { browserTabId: string, override: BrowserViewportOverride | null ): Promise { - // Why: chain per-tab so rapid toggles — and navigations racing them — don't interleave CDP - // commands; the last-enqueued write wins. - return this.enqueueViewportOp(browserTabId, () => - this.doSetViewportOverrideImpl(browserTabId, override) - ) + // Why: chain per-tab so rapid toggles don't interleave CDP commands and the last-requested override wins. + const prev = this.viewportOpsByTabId.get(browserTabId) ?? Promise.resolve() + const next = prev + .catch(() => {}) + .then(() => this.doSetViewportOverrideImpl(browserTabId, override)) + this.viewportOpsByTabId.set(browserTabId, next) + try { + return await next + } finally { + // Why: only clear if we're still the tail; a later call may have replaced the entry, and deleting would break serialization. + if (this.viewportOpsByTabId.get(browserTabId) === next) { + this.viewportOpsByTabId.delete(browserTabId) + } + } } async setAnnotationViewportBridge( @@ -1656,14 +1651,13 @@ export class BrowserManager { }) // Why: viewport sizing must not override a profile's explicit native-UA identity. if (this.userAgentModeByPageId.get(browserTabId) !== 'native') { - // Why: remember the standing override so later navigations can re-issue it; it outranks - // setUserAgent and would otherwise pin the auth-host identity to whatever was set here. - this.viewportUaOverrideMobileByTabId.set(browserTabId, override.mobile) // Why: same sender as the navigation path, so both resolve the tab's host identically. - await this.sendViewportUserAgentOverride(guest, browserTabId) + await this.sendViewportUserAgentOverride(guest, browserTabId, override.mobile) + // Why: only claim a standing override once the CDP write actually landed — recording it + // first would leave the map asserting an override that was never installed. + this.viewportUaOverrideMobileByTabId.set(browserTabId, override.mobile) } } else { - this.viewportUaOverrideMobileByTabId.delete(browserTabId) await dbg.sendCommand('Emulation.clearDeviceMetricsOverride', {}) await dbg.sendCommand('Emulation.setTouchEmulationEnabled', { enabled: false, @@ -1671,6 +1665,9 @@ export class BrowserManager { }) // Why: passing an empty string restores the session default UA. await dbg.sendCommand('Emulation.setUserAgentOverride', { userAgent: '' }) + // Why: drop tracking only after the clear landed. Deleting first strands a still-standing + // CDP UA override with no map entry, so the navigation path can never correct it again. + this.viewportUaOverrideMobileByTabId.delete(browserTabId) } return true } catch {