starred/orca
1
0
Fork 0
mirror of https://github.com/stablyai/orca.git synced 2026-10-21 00:03:46 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(browser): correct auth-UA popup opt-out and viewport override tracking

Browse Source 
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.
This commit is contained in:
Brennan Benson
2026-08-07 00:47:08 -07:00
parent 0b90e8d4e5
commit 0cb77edaa3
2 changed files with 164 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/main/browser/browser-manager.test.ts
+110 -5
View File
@@ -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<typeof vi.fn>
): Record<string, unknown> | 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)
src/main/browser/browser-manager.ts
+54 -57
View File
@@ -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<T>(browserTabId: string, op: () => Promise<T>): Promise<T> {
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<void> {
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<void> {
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<boolean> {
// 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 {