diff --git a/src/main/browser/browser-cookie-import-partition-rollback.electron.test.ts b/src/main/browser/browser-cookie-import-partition-rollback.electron.test.ts index e62b43c8697..f3b4a49ed8d 100644 --- a/src/main/browser/browser-cookie-import-partition-rollback.electron.test.ts +++ b/src/main/browser/browser-cookie-import-partition-rollback.electron.test.ts @@ -35,7 +35,7 @@ function buildFixtureMain(policyPath: string, resultPath: string): string { return ` const { app, BrowserWindow, session } = require('electron') const { writeFileSync } = require('node:fs') -const { removeAllCookiesExcept, isNonTransplantableCookieDomain } = require(${JSON.stringify(policyPath)}) +const { removeTransplantableCookies } = require(${JSON.stringify(policyPath)}) const resultPath = ${JSON.stringify(resultPath)} let currentStep = 'starting' const mark = (step) => { @@ -93,18 +93,20 @@ async function run() { } } + // Why: the bulk clear is the ordinary path now, so rejecting it is what routes this fixture onto + // the per-cookie fallback where a partial failure — and the old rollback — could happen at all. let bulkClearCalls = 0 const clearSession = { cookies: store, - clearData: async (options) => { + clearData: async () => { bulkClearCalls++ - return targetSession.clearData(options) + throw new Error('forced bulk clear failure') } } let clearError = null try { - await removeAllCookiesExcept(clearSession, (cookie) => isNonTransplantableCookieDomain(cookie.domain ?? '')) + await removeTransplantableCookies(clearSession) } catch (error) { clearError = String(error?.message || error) } @@ -180,8 +182,9 @@ describe('non-Google partitioned cookie under a failed Electron cookie clear', ( const result = await runFixture() expect(result.beforePartitionKey).toEqual(EXPECTED_PARTITION_KEY) - // Why (STA-4065): a bulk clear here would make every assertion below vacuous. - expect(result.bulkClearCalls).toBe(0) + // Why (STA-4065): the fallback is only reachable once the bulk clear has been tried and failed; + // if it ever succeeded here the partial-failure assertions below would be vacuous. + expect(result.bulkClearCalls).toBe(1) expect(result.remainingExcluded.map(({ name }) => name)).toEqual(['SID']) expect(result.clearError).toContain('Could not clear existing cookies') // STA-4061: rollback rebuilt this cookie through cookies.set, which drops partitionKey. diff --git a/src/main/browser/browser-cookie-import-policy.test.ts b/src/main/browser/browser-cookie-import-policy.test.ts index a5f0b89eacf..7b2fad0bfdc 100644 --- a/src/main/browser/browser-cookie-import-policy.test.ts +++ b/src/main/browser/browser-cookie-import-policy.test.ts @@ -6,7 +6,7 @@ import { isNonTransplantableCookieDomain, NON_TRANSPLANTABLE_HOST_KEY_SQL, normalizeCookieDomain, - removeAllCookiesExcept, + removeTransplantableCookies, replaceCookiesForImportedDomains } from './browser-cookie-import-policy' @@ -214,7 +214,9 @@ type CookieClearMocks = { clearData: Mock } -describe('removeAllCookiesExcept', () => { +describe('removeTransplantableCookies', () => { + const rejectingBulkClear = () => vi.fn().mockRejectedValue(new Error('storage busy')) + function clearSession(cookies: Cookie[], overrides: Partial = {}) { const store = { get: vi.fn().mockResolvedValue(cookies), @@ -232,10 +234,9 @@ describe('removeAllCookiesExcept', () => { } } - const excludesNonTransplantable = (existingCookie: Cookie): boolean => - isNonTransplantableCookieDomain(existingCookie.domain ?? '') - - it('never removes or reconstructs excluded Google cookies', async () => { + // Why (STA-4065): the bulk call is the ordinary path even when the jar holds cookies to keep — + // excludeOrigins preserves the whole google.com family, verified against real Electron. + it('clears a jar holding Google cookies in one call that excludes them', async () => { const { session, remove, set, clearData } = clearSession([ cookie('.google.com', 'SID'), cookie('accounts.google.com', 'ACCOUNT'), @@ -243,13 +244,12 @@ describe('removeAllCookiesExcept', () => { cookie('other.test', 'tracker', '/scoped') ]) - await removeAllCookiesExcept(session, excludesNonTransplantable) + await removeTransplantableCookies(session) - expect(clearData).not.toHaveBeenCalled() - expect(remove.mock.calls).toEqual([ - ['https://example.com/', 'session'], - ['https://other.test/scoped', 'tracker'] + expect(clearData.mock.calls).toEqual([ + [{ dataTypes: ['cookies'], excludeOrigins: ['https://google.com'] }] ]) + expect(remove).not.toHaveBeenCalled() expect(set).not.toHaveBeenCalled() }) @@ -260,7 +260,7 @@ describe('removeAllCookiesExcept', () => { cookie('notgoogle.com', 'lookalike') ]) - await removeAllCookiesExcept(session, excludesNonTransplantable) + await removeTransplantableCookies(session) expect(clearData.mock.calls).toEqual([ [{ dataTypes: ['cookies'], excludeOrigins: ['https://google.com'] }] @@ -272,7 +272,7 @@ describe('removeAllCookiesExcept', () => { it('touches nothing when the jar is already empty', async () => { const { session, remove, clearData } = clearSession([]) - await removeAllCookiesExcept(session, excludesNonTransplantable) + await removeTransplantableCookies(session) expect(clearData).not.toHaveBeenCalled() expect(remove).not.toHaveBeenCalled() @@ -283,7 +283,7 @@ describe('removeAllCookiesExcept', () => { { ...cookie('.example.com', 'session'), domain: '' } ]) - await removeAllCookiesExcept(session, excludesNonTransplantable) + await removeTransplantableCookies(session) expect(clearData).toHaveBeenCalledOnce() }) @@ -291,10 +291,10 @@ describe('removeAllCookiesExcept', () => { it('falls back to per-cookie removal when the bulk clear rejects', async () => { const { session, remove, clearData } = clearSession( [cookie('.example.com', 'session'), cookie('other.test', 'tracker')], - { clearData: vi.fn().mockRejectedValue(new Error('storage busy')) } + { clearData: rejectingBulkClear() } ) - await removeAllCookiesExcept(session, excludesNonTransplantable) + await removeTransplantableCookies(session) expect(clearData).toHaveBeenCalledOnce() expect(remove.mock.calls).toEqual([ @@ -303,6 +303,24 @@ describe('removeAllCookiesExcept', () => { ]) }) + // Why: the fallback carries the same exclusion as the bulk call, so a rejected clearData must + // not become the path that finally deletes a live Google session. + it('still preserves Google cookies on the per-cookie fallback', async () => { + const { session, remove, set } = clearSession( + [ + cookie('.google.com', 'SID'), + cookie('accounts.google.com', 'ACCOUNT'), + cookie('.example.com', 'session') + ], + { clearData: rejectingBulkClear() } + ) + + await removeTransplantableCookies(session) + + expect(remove.mock.calls).toEqual([['https://example.com/', 'session']]) + expect(set).not.toHaveBeenCalled() + }) + // Why (STA-4061): reconstructing a removed cookie loses its partition key, and the snapshot // cannot say which cookies had one, so a failed clear must stay failed. it('never reconstructs removed cookies when another removal fails', async () => { @@ -314,6 +332,7 @@ describe('removeAllCookiesExcept', () => { cookie('.example.com', 'third', '/three') ], { + clearData: rejectingBulkClear(), remove: vi.fn().mockImplementation(async (_url: string, name: string) => { if (name === 'second') { throw new Error('store unavailable') @@ -322,7 +341,7 @@ describe('removeAllCookiesExcept', () => { } ) - await expect(removeAllCookiesExcept(session, excludesNonTransplantable)).rejects.toThrow( + await expect(removeTransplantableCookies(session)).rejects.toThrow( 'the session was left partially cleared' ) expect(remove).toHaveBeenCalledTimes(3) @@ -342,6 +361,7 @@ describe('removeAllCookiesExcept', () => { ...Array.from({ length: 12 }, (_, index) => cookie('.example.com', `${index}`)) ], { + clearData: rejectingBulkClear(), remove: vi.fn().mockImplementation(async () => { active++ maxActive = Math.max(maxActive, active) @@ -351,7 +371,7 @@ describe('removeAllCookiesExcept', () => { } ) - const clearing = removeAllCookiesExcept(session, excludesNonTransplantable) + const clearing = removeTransplantableCookies(session) await vi.waitFor(() => expect(remove).toHaveBeenCalledTimes(8)) expect(maxActive).toBe(8) releaseRemovals?.() @@ -373,6 +393,7 @@ describe('removeAllCookiesExcept', () => { { ...cookie('example.com', 'session'), hostOnly: true } ], { + clearData: rejectingBulkClear(), remove: vi .fn() .mockImplementationOnce(() => firstReleased) @@ -380,7 +401,7 @@ describe('removeAllCookiesExcept', () => { } ) - const clearing = removeAllCookiesExcept(session, excludesNonTransplantable) + const clearing = removeTransplantableCookies(session) await vi.waitFor(() => expect(remove).toHaveBeenCalledOnce()) releaseFirst?.() await clearing diff --git a/src/main/browser/browser-cookie-import-policy.ts b/src/main/browser/browser-cookie-import-policy.ts index f1555b78799..315b5eccdd6 100644 --- a/src/main/browser/browser-cookie-import-policy.ts +++ b/src/main/browser/browser-cookie-import-policy.ts @@ -211,32 +211,36 @@ export type CookieClearSession = { // can downgrade a partitioned (CHIPS) cookie into an unpartitioned one — and nothing in the // snapshot says which cookies are at risk. A partially cleared jar is a retryable import failure; // a downgraded cookie is unrecoverable auth-state corruption that survives restart. -export async function removeAllCookiesExcept( - targetSession: CookieClearSession, - isExcluded: (cookie: Cookie) => boolean +// Why (STA-4065): the exclusion is module state rather than a parameter so the predicate and the +// origins the bulk clear preserves cannot drift apart — a caller-supplied predicate that disagreed +// with NON_TRANSPLANTABLE_DOMAINS would silently delete a cookie the bulk call is meant to keep. +export async function removeTransplantableCookies( + targetSession: CookieClearSession ): Promise { const store = targetSession.cookies const existingCookies = await store.get({}) + if (existingCookies.length === 0) { + return + } - // Why: an unfiltered get() is the whole jar, so no exclusion in it means nothing to preserve and - // one bulk clear can replace a remove() per cookie. An empty jar needs neither. - if (existingCookies.length > 0 && !existingCookies.some(isExcluded)) { - try { - // Why: exclusions protect a non-transplantable cookie arriving between snapshot and clear. - await targetSession.clearData({ - dataTypes: ['cookies'], - excludeOrigins: NON_TRANSPLANTABLE_CLEAR_EXCLUDED_ORIGINS - }) - return - } catch { - // Why: a rejected bulk clear can still have emptied part of the jar, so fall through to the - // per-cookie path and let it re-remove whatever survived. - } + // Why (STA-4065): measured on Electron 43, excludeOrigins preserves the whole registrable + // family — host, leading-dot, subdomain, and partitioned Google cookies — so one call replaces a + // remove() per cookie even when the jar holds cookies to keep. That is the ordinary case here: + // this import exists for Google, so a Google cookie is usually present. + try { + await targetSession.clearData({ + dataTypes: ['cookies'], + excludeOrigins: NON_TRANSPLANTABLE_CLEAR_EXCLUDED_ORIGINS + }) + return + } catch { + // Why: a rejected bulk clear can still have emptied part of the jar, so fall through to the + // per-cookie path and let it re-remove whatever survived. } const removableGroups = new Map() for (const cookie of existingCookies) { - if (isExcluded(cookie)) { + if (isNonTransplantableCookieDomain(cookie.domain ?? '')) { continue } const domain = cookie.domain ? normalizeCookieDomain(cookie.domain) : null diff --git a/src/main/browser/browser-cookie-import.ts b/src/main/browser/browser-cookie-import.ts index d9a9d3ac087..3c3a701f953 100644 --- a/src/main/browser/browser-cookie-import.ts +++ b/src/main/browser/browser-cookie-import.ts @@ -80,7 +80,7 @@ import { NON_TRANSPLANTABLE_HOST_KEY_SQL, normalizeCookieDomain, normalizeCookieImportDomain, - removeAllCookiesExcept, + removeTransplantableCookies, replaceCookiesForImportedDomains, restoreImportedDomainCookies, type CookieImportMode @@ -1780,9 +1780,7 @@ export async function importCookiesFromBrowser( // Why: clear stale cookies first; mixing them with the imported set makes sites reject the // session. Non-transplantable families are exempt — nothing was imported for them, and their // live session is the only one that works. - await removeAllCookiesExcept(targetSession, (cookie) => - isNonTransplantableCookieDomain(cookie.domain ?? '') - ) + await removeTransplantableCookies(targetSession) diag( ` cleared existing session cookies before loading ${decryptedCookies.length} imported cookies` )