From 236cb738306278e096c64ad5b498c21e2425edbf Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Thu, 10 Sep 2026 22:39:05 -0700 Subject: [PATCH] fix(browser): avoid duplicate Google auth requests --- .../browser/browser-google-auth-ua.test.ts | 23 +++++++ src/main/browser/browser-google-auth-ua.ts | 13 ++++ .../browser-manager-auth-user-agent.test.ts | 37 +++++------ ...browser-manager-guest-navigation-policy.ts | 2 +- .../browser/browser-manager-navigation.ts | 64 +++++++++---------- .../browser-manager-viewport-override.test.ts | 27 ++++---- .../browser/browser-session-registry.test.ts | 40 ++++++++++++ src/main/browser/browser-session-ua.ts | 8 +-- 8 files changed, 141 insertions(+), 73 deletions(-) diff --git a/src/main/browser/browser-google-auth-ua.test.ts b/src/main/browser/browser-google-auth-ua.test.ts index a1ed1370418..22109648541 100644 --- a/src/main/browser/browser-google-auth-ua.test.ts +++ b/src/main/browser/browser-google-auth-ua.test.ts @@ -4,6 +4,7 @@ import { googleAuthUserAgent, isGoogleAuthUrl, setUserAgentHeader, + shouldUseGoogleAuthIdentity, stripClientHints } from './browser-google-auth-ua' @@ -33,6 +34,28 @@ describe('googleAuthUserAgent', () => { }) }) +describe('shouldUseGoogleAuthIdentity', () => { + it('includes cross-host subresources referred by an auth document', () => { + expect( + shouldUseGoogleAuthIdentity( + 'https://www.gstatic.com/accounts/signin.js', + 'https://accounts.google.com/v3/signin/identifier', + 'script' + ) + ).toBe(true) + }) + + it('excludes a post-auth main-frame exit even when the auth document referred it', () => { + expect( + shouldUseGoogleAuthIdentity( + 'https://mail.google.com/', + 'https://accounts.google.com/v3/signin/identifier', + 'mainFrame' + ) + ).toBe(false) + }) +}) + describe('stripClientHints', () => { it('removes every sec-ch-ua* header regardless of case, keeps others', () => { const headers: Record = { diff --git a/src/main/browser/browser-google-auth-ua.ts b/src/main/browser/browser-google-auth-ua.ts index e9b802f6d70..e5139827d72 100644 --- a/src/main/browser/browser-google-auth-ua.ts +++ b/src/main/browser/browser-google-auth-ua.ts @@ -20,6 +20,19 @@ export function isGoogleAuthUrl(rawUrl: string): boolean { } } +export function shouldUseGoogleAuthIdentity( + url: string, + referrer: string, + resourceType: string +): boolean { + if (isGoogleAuthUrl(url)) { + return true + } + // Why: early cross-host subresources can leave before the WebContents Firefox override lands; + // the auth referrer identifies their owning flow. Main-frame exits must restore the profile UA. + return resourceType !== 'mainFrame' && isGoogleAuthUrl(referrer) +} + // Why: rv:/Gecko/Firefox tokens must line up with a real released build and the // platform token must match the host OS, or the UA is internally inconsistent and // itself a bot tell. diff --git a/src/main/browser/browser-manager-auth-user-agent.test.ts b/src/main/browser/browser-manager-auth-user-agent.test.ts index 8f5267bf03b..75f0a5fa068 100644 --- a/src/main/browser/browser-manager-auth-user-agent.test.ts +++ b/src/main/browser/browser-manager-auth-user-agent.test.ts @@ -209,10 +209,13 @@ describe('browserManager', () => { setUserAgent.mockClear() didStartNavigation(null, 'https://accounts.google.com/v3/signin/identifier', false, true) - expect(setUserAgent).toHaveBeenLastCalledWith(googleAuthUserAgent()) + expect(setUserAgent).not.toHaveBeenCalled() + expect(sendCommand).toHaveBeenCalledWith('Emulation.setUserAgentOverride', { + userAgent: googleAuthUserAgent() + }) - // A redirect off the auth host must derive the CDP write from the session UA, not the stale - // Firefox WebContents UA installed by the direct navigation. + // A redirect off the auth host must derive the CDP write from the session UA, not the standing + // Firefox CDP override installed by the direct navigation. sendCommand.mockClear() willRedirect(null, 'https://myaccount.google.com/', false, true) const uaOverrideIndex = sendCommand.mock.calls.findIndex( @@ -555,9 +558,9 @@ describe('browserManager', () => { ) }) - // Why: a direct load reaches did-start-navigation before the request is dispatched, so the - // WebContents write is safe there and must stay — CDP is the redirect-path mechanism only. - it('still uses the WebContents UA write for navigations that are not redirects', () => { + // Why: WebContents.setUserAgent() after a direct navigation starts replays that document and all + // of its subresources. The auth identity must use the cancel-free CDP path for direct loads too. + it('retargets direct auth navigation over CDP without writing the WebContents UA', () => { const guest = { id: 420, isDestroyed: vi.fn(() => false), @@ -587,18 +590,14 @@ describe('browserManager', () => { didStartNavigation(null, 'https://accounts.google.com/v3/signin/identifier', false, true) - expect(guest.setUserAgent).toHaveBeenLastCalledWith(googleAuthUserAgent()) - expect(guest.debugger.sendCommand).not.toHaveBeenCalledWith( - 'Emulation.setUserAgentOverride', - expect.anything() - ) + expect(guest.setUserAgent).not.toHaveBeenCalled() + expect(guest.debugger.sendCommand).toHaveBeenCalledWith('Emulation.setUserAgentOverride', { + userAgent: googleAuthUserAgent() + }) }) - // Why: the direct-navigation branch still writes the Firefox UA through WebContents.setUserAgent, - // and nothing ever restores it once the guest switches to the CDP override. A viewport preset that - // read getUserAgent() back as its base identity would therefore republish Firefox on every ordinary - // host — the wire UA saying Firefox while sec-ch-ua still says Chrome, the exact cross-layer tell - // this scope exists to remove. + // Why: a viewport preset must derive its base from the session identity, independent of the + // standing auth CDP override, or it can republish Firefox on an ordinary host. it('keeps a viewport preset on the session identity after an auth-host visit', async () => { const { guest, debuggerSendCommand } = makeViewportGuest(9001) webContentsFromIdMock.mockReturnValue(guest) @@ -625,8 +624,10 @@ describe('browserManager', () => { didStartNavigation(null, 'https://accounts.google.com/v3/signin/identifier', false, true) await flushViewportOps() - // The direct branch pins the WebContents UA to Firefox and never restores it. - expect((guest.getUserAgent as () => string)()).toBe(googleAuthUserAgent()) + expect(guest.setUserAgent).not.toHaveBeenCalled() + expect(debuggerSendCommand).toHaveBeenLastCalledWith('Emulation.setUserAgentOverride', { + userAgent: googleAuthUserAgent() + }) willRedirect({ preventDefault: vi.fn() }, 'https://myaccount.google.com/', false, true) await flushViewportOps() diff --git a/src/main/browser/browser-manager-guest-navigation-policy.ts b/src/main/browser/browser-manager-guest-navigation-policy.ts index abacd268640..10f716013c0 100644 --- a/src/main/browser/browser-manager-guest-navigation-policy.ts +++ b/src/main/browser/browser-manager-guest-navigation-policy.ts @@ -35,7 +35,7 @@ export abstract class BrowserManagerGuestNavigationPolicy extends BrowserManager return } this.updatePendingNavigationForRedirect(guest.id, url) - this.applyGoogleAuthUserAgent(guest, url, { duringRedirect: true }) + this.applyGoogleAuthUserAgent(guest, url) } const didFailLoadHandler = ( diff --git a/src/main/browser/browser-manager-navigation.ts b/src/main/browser/browser-manager-navigation.ts index 9a61ec50e49..1169ea2e7b6 100644 --- a/src/main/browser/browser-manager-navigation.ts +++ b/src/main/browser/browser-manager-navigation.ts @@ -74,11 +74,7 @@ export abstract class BrowserManagerNavigation extends BrowserManagerVisibility // must be matched here per navigation or the two layers disagree — itself a bot tell. // Restores the session's base identity off the auth hosts. Native-UA profiles opt out // of the whole clean-UA path, so they keep their untouched identity everywhere. - protected applyGoogleAuthUserAgent( - guest: Electron.WebContents, - url: string, - options: { duringRedirect?: boolean } = {} - ): void { + protected applyGoogleAuthUserAgent(guest: Electron.WebContents, url: string): void { const browserPageId = this.tabIdByWebContentsId.get(guest.id) // 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. @@ -110,32 +106,24 @@ export abstract class BrowserManagerNavigation extends BrowserManagerVisibility : null let authOverrideIssuedOverCdp = false if (nextUa !== null && nextUa !== currentUa) { - // Why: WebContents.setUserAgent() during a redirect makes Chromium cancel the in-flight - // navigation (ERR_ABORTED) and replay the original request, which a POST-started OAuth chain - // cannot survive — the sign-in lands on a blank tab. CDP retargets navigator.userAgent without - // touching the navigation, and it outranks the WebContents UA from then on, so a guest that - // switches to it stays on it. The wire UA never depended on this write: - // setupGoogleAuthUserAgentOverride rewrites User-Agent per request for auth-host URLs on its own. - if (options.duringRedirect === true || overrideState !== undefined) { - if (this.canOverrideUserAgentOverCdp(guest)) { - authOverrideIssuedOverCdp = true - // Why: go through the viewport builder rather than writing nextUa raw, so both CDP writers - // resolve one identity for this URL — Firefox on auth hosts, the profile's clean base off - // them, any mobile preset preserved. Writing the session UA directly would put the - // unlaundered Electron token back on the wire. - void this.applyAuthUserAgentOverrideOverCdp( - guest, - (browserPageId ? this.viewportUaOverrideMobileByTabId.get(browserPageId) : undefined) ?? - false, - url, - nextUa - ) - } - // Why: with no debugger there is no way to retarget the identity without cancelling the - // redirect. A stale navigator.userAgent is recoverable; a dead navigation is not. - } else { - guest.setUserAgent(nextUa) + // Why: WebContents.setUserAgent() after navigation starts cancels and replays the document, + // including direct loads. CDP retargets navigator.userAgent without duplicating its requests. + if (this.ensureAuthUserAgentDebugger(guest)) { + authOverrideIssuedOverCdp = true + // Why: go through the viewport builder rather than writing nextUa raw, so both CDP writers + // resolve one identity for this URL — Firefox on auth hosts, the profile's clean base off + // them, any mobile preset preserved. Writing the session UA directly would put the + // unlaundered Electron token back on the wire. + void this.applyAuthUserAgentOverrideOverCdp( + guest, + (browserPageId ? this.viewportUaOverrideMobileByTabId.get(browserPageId) : undefined) ?? + false, + url, + nextUa + ) } + // Why: with no debugger there is no cancel-free way to retarget navigator.userAgent. The + // request layer still presents Firefox; a stale JS value is preferable to replaying the page. } // 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. @@ -144,9 +132,15 @@ export abstract class BrowserManagerNavigation extends BrowserManagerVisibility } } - protected canOverrideUserAgentOverCdp(guest: Electron.WebContents): boolean { + protected ensureAuthUserAgentDebugger(guest: Electron.WebContents): boolean { try { - return !guest.isDestroyed() && guest.debugger.isAttached() + if (guest.isDestroyed()) { + return false + } + if (!guest.debugger.isAttached()) { + guest.debugger.attach('1.3') + } + return true } catch { return false } @@ -158,7 +152,7 @@ export abstract class BrowserManagerNavigation extends BrowserManagerVisibility url: string, userAgent: string ): Promise { - if (!this.canOverrideUserAgentOverCdp(guest)) { + if (!this.ensureAuthUserAgentDebugger(guest)) { return Promise.resolve(false) } const state = this.authUserAgentOverrideStateByGuestId.get(guest.id) ?? { @@ -277,8 +271,8 @@ export abstract class BrowserManagerNavigation extends BrowserManagerVisibility buildViewportUserAgentOverride({ url: url ?? this.resolveTabNavigationUrl(guest), mobile, - // Why: the session UA is the profile's stable base identity. guest.getUserAgent() is not: - // applyGoogleAuthUserAgent leaves it pinned to the Firefox auth UA once a guest switches to + // Why: the session UA is the profile's stable base identity. guest.getUserAgent() does not + // expose the standing CDP auth override once a guest switches to // the CDP override, so reading it back here would republish that identity on ordinary hosts. baseUserAgent: cleanElectronUserAgent(baseUserAgent ?? guest.session.getUserAgent()) }) diff --git a/src/main/browser/browser-manager-viewport-override.test.ts b/src/main/browser/browser-manager-viewport-override.test.ts index 0228d8f9ed8..d78ef52715d 100644 --- a/src/main/browser/browser-manager-viewport-override.test.ts +++ b/src/main/browser/browser-manager-viewport-override.test.ts @@ -49,8 +49,7 @@ import { import { createViewportGuestFactory, flushViewportOps, - GUEST_CLEAN_UA, - GUEST_ELECTRON_UA + GUEST_CLEAN_UA } from './browser-manager-viewport-test-fixtures' const { guestOnMock, webContentsFromIdMock } = browserMocks @@ -377,7 +376,7 @@ describe('browserManager', () => { didFailLoad(null, -3, 'Aborted', 'https://accounts.google.com/', true) await flushViewportOps() - expect(guest.setUserAgent).toHaveBeenLastCalledWith(GUEST_ELECTRON_UA) + expect(guest.setUserAgent).not.toHaveBeenCalled() expect(lastUserAgentOverride(debuggerSendCommand)).toEqual({ userAgent: GUEST_CLEAN_UA }) // A later preset must also resolve the committed, non-auth URL. @@ -636,7 +635,7 @@ describe('browserManager', () => { await presetDone }) - it('does not reinstall a preset while its final UA clear is in flight', async () => { + it('preserves the auth-owned UA while a preset UA clear is in flight', async () => { const { guest, debuggerSendCommand } = makeGuest(4258, 'https://example.com/') webContentsFromIdMock.mockReturnValue(guest) browserManager.attachGuestPolicies(guest as never) @@ -671,7 +670,7 @@ describe('browserManager', () => { debuggerSendCommand.mockClear() didStartNavigation(null, 'https://accounts.google.com/', false, true) await flushViewportOps() - expect(debuggerSendCommand).not.toHaveBeenCalledWith('Emulation.setUserAgentOverride', { + expect(debuggerSendCommand).toHaveBeenCalledWith('Emulation.setUserAgentOverride', { userAgent: googleAuthUserAgent() }) @@ -725,7 +724,7 @@ describe('browserManager', () => { }) }) - it('does not touch the UA override on navigation when no preset is standing', async () => { + it('applies the auth-owned UA on navigation when no preset is standing', async () => { const { guest, debuggerSendCommand } = makeGuest(4248) webContentsFromIdMock.mockReturnValue(guest) browserManager.attachGuestPolicies(guest as never) @@ -740,13 +739,12 @@ describe('browserManager', () => { didStartNavigation(null, 'https://accounts.google.com/', false, true) await flushViewportOps() - expect(debuggerSendCommand).not.toHaveBeenCalledWith( - 'Emulation.setUserAgentOverride', - expect.anything() - ) + expect(debuggerSendCommand).toHaveBeenCalledWith('Emulation.setUserAgentOverride', { + userAgent: googleAuthUserAgent() + }) }) - it('stops re-issuing the UA override once the preset is cleared', async () => { + it('keeps applying the auth-owned UA once the preset is cleared', async () => { const { guest, debuggerSendCommand } = makeGuest(4249) webContentsFromIdMock.mockReturnValue(guest) browserManager.attachGuestPolicies(guest as never) @@ -770,10 +768,9 @@ describe('browserManager', () => { debuggerSendCommand.mockClear() didStartNavigation(null, 'https://accounts.google.com/', false, true) await flushViewportOps() - expect(debuggerSendCommand).not.toHaveBeenCalledWith( - 'Emulation.setUserAgentOverride', - expect.anything() - ) + expect(debuggerSendCommand).toHaveBeenCalledWith('Emulation.setUserAgentOverride', { + userAgent: googleAuthUserAgent() + }) }) it('leaves the UA override alone on navigation for native-UA profiles', async () => { diff --git a/src/main/browser/browser-session-registry.test.ts b/src/main/browser/browser-session-registry.test.ts index 5c628723777..2e12b21f090 100644 --- a/src/main/browser/browser-session-registry.test.ts +++ b/src/main/browser/browser-session-registry.test.ts @@ -608,6 +608,46 @@ describe('BrowserSessionRegistry', () => { expect(modified['sec-ch-ua-mobile']).toBeUndefined() }) + it('presents the Firefox identity on cross-host subresources referred by Google auth', () => { + const callback = vi.fn() + install()( + { + url: 'https://www.gstatic.com/accounts/signin.js', + referrer: 'https://accounts.google.com/v3/signin/identifier', + resourceType: 'script', + requestHeaders: { + 'User-Agent': 'Chrome/150', + 'sec-ch-ua': 'browser-owned', + 'sec-ch-ua-platform': '"macOS"' + } + }, + callback + ) + + const modified = callback.mock.calls[0][0].requestHeaders + expect(modified['User-Agent']).toBe(googleAuthUserAgent()) + expect(modified['sec-ch-ua']).toBeUndefined() + expect(modified['sec-ch-ua-platform']).toBeUndefined() + }) + + it('keeps the session identity on a post-auth main frame referred by Google auth', () => { + const callback = vi.fn() + install()( + { + url: 'https://mail.google.com/', + referrer: 'https://accounts.google.com/v3/signin/identifier', + resourceType: 'mainFrame', + requestHeaders: { 'User-Agent': 'Chrome/150', 'sec-ch-ua': 'browser-owned' } + }, + callback + ) + + expect(callback.mock.calls[0][0].requestHeaders).toEqual({ + 'User-Agent': 'Chrome/150', + 'sec-ch-ua': 'browser-owned' + }) + }) + it('keeps the session identity on Google app subdomains', () => { const callback = vi.fn() install()( diff --git a/src/main/browser/browser-session-ua.ts b/src/main/browser/browser-session-ua.ts index 5e3a81a0a7a..5dc16ff0169 100644 --- a/src/main/browser/browser-session-ua.ts +++ b/src/main/browser/browser-session-ua.ts @@ -4,8 +4,8 @@ import type { ViewportUserAgentOverride } from './browser-viewport-user-agent' import { currentUserAgent, googleAuthUserAgent, - isGoogleAuthUrl, setUserAgentHeader, + shouldUseGoogleAuthIdentity, stripClientHints } from './browser-google-auth-ua' @@ -92,10 +92,10 @@ export function setupGoogleAuthUserAgentOverride( typeof sess.getUserAgent === 'function' ? cleanElectronUserAgent(sess.getUserAgent()) : (requestUserAgent ?? '') - if (isGoogleAuthUrl(details.url)) { + if (shouldUseGoogleAuthIdentity(details.url, details.referrer, details.resourceType)) { // Why: present a Firefox identity on Google's sign-in hosts so the user logs - // in inside the app and Google issues self-refreshing bound cookies. Strip - // sec-ch-ua* because real Firefox sends none. + // in inside the app and Google issues self-refreshing bound cookies. Auth-page + // subresources share that identity even before the WebContents override lands. setUserAgentHeader(headers, firefoxUa) stripClientHints(headers) callback({ requestHeaders: headers })