From 6462ef7a761973f62ec8967b1e59cba1a1cec033 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 12 Aug 2026 19:34:10 -0700 Subject: [PATCH] fix(browser): re-read the cookie jar after a rejected bulk clear clearData can empty part of the jar before rejecting, so the per-cookie fallback was working from a snapshot taken before the attempt: it retried removals for cookies that were already gone and could not see cookies that arrived during the clear. Re-read on rejection only. Also repairs three fixtures that mocked the session with clearStorageData after the call moved to clearData. The missing method threw, the catch swallowed it, and every one of those tests silently asserted per-cookie behaviour while believing it covered exclusion. Records the registrable-domain invariant at NON_TRANSPLANTABLE_DOMAINS, since excludeOrigins derives one origin per entry, and extends the real-Electron fixture to prove http-only and domain-scoped Google cookies survive alongside the partitioned one. --- ...ser-cookie-import-google-exclusion.test.ts | 20 +++++++------- ...import-partition-rollback.electron.test.ts | 4 +-- ...r-cookie-import-partition.electron.test.ts | 27 ++++++++++++++----- .../browser-cookie-import-policy.test.ts | 22 +++++++++++++++ .../browser/browser-cookie-import-policy.ts | 11 +++++--- .../browser-cookie-import-replacement.test.ts | 8 +++--- 6 files changed, 66 insertions(+), 26 deletions(-) diff --git a/src/main/browser/browser-cookie-import-google-exclusion.test.ts b/src/main/browser/browser-cookie-import-google-exclusion.test.ts index 3cc329a14d7..431ca758f2f 100644 --- a/src/main/browser/browser-cookie-import-google-exclusion.test.ts +++ b/src/main/browser/browser-cookie-import-google-exclusion.test.ts @@ -120,7 +120,7 @@ describe('file import excludes the Google cookie family', () => { }) describe('native Chromium import excludes the Google cookie family', () => { - let clearStorageDataMock: ReturnType + let clearDataMock: ReturnType let cookiesGetMock: ReturnType let cookiesRemoveMock: ReturnType let cookiesSetMock: ReturnType @@ -134,7 +134,7 @@ describe('native Chromium import excludes the Google cookie family', () => { execFileSyncMock.mockImplementation(() => { throw new Error('OS browser version lookup unavailable') }) - clearStorageDataMock = vi.fn().mockResolvedValue(undefined) + clearDataMock = vi.fn().mockResolvedValue(undefined) cookiesGetMock = vi.fn().mockResolvedValue([]) cookiesRemoveMock = vi.fn().mockResolvedValue(undefined) cookiesSetMock = vi.fn().mockResolvedValue(undefined) @@ -145,7 +145,7 @@ describe('native Chromium import excludes the Google cookie family', () => { remove: cookiesRemoveMock, set: cookiesSetMock }, - clearStorageData: clearStorageDataMock + clearData: clearDataMock }) platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') }) @@ -166,7 +166,7 @@ describe('native Chromium import excludes the Google cookie family', () => { createChromiumCookieTestDatabase(targetCookiesPath, rows).close() } - it('never clears or reconstructs live Google cookies before importing', async () => { + it('bulk clears while excluding live Google cookies before importing', async () => { const sourceCookiesPath = seedSource([ { domain: '.google.com', name: 'SID', value: 'transplanted-sid' }, { domain: '.example.com', name: 'session', value: 'new' } @@ -186,9 +186,10 @@ describe('native Chromium import excludes the Google cookie family', () => { googleCookiesSkipped: 1, domains: ['example.com'] }) - expect(clearStorageDataMock).not.toHaveBeenCalled() - expect(cookiesRemoveMock).toHaveBeenCalledOnce() - expect(cookiesRemoveMock).toHaveBeenCalledWith('https://example.com/', 'stale') + expect(clearDataMock.mock.calls).toEqual([ + [{ dataTypes: ['cookies'], excludeOrigins: ['https://google.com'] }] + ]) + expect(cookiesRemoveMock).not.toHaveBeenCalled() expect(cookiesSetMock.mock.calls.map(([details]) => details.domain)).toEqual(['.example.com']) }) @@ -239,7 +240,7 @@ describe('native Chromium import excludes the Google cookie family', () => { domains: [] }) expect(execFileSyncMock).not.toHaveBeenCalled() - expect(clearStorageDataMock).not.toHaveBeenCalled() + expect(clearDataMock).not.toHaveBeenCalled() expect(cookiesSetMock).not.toHaveBeenCalled() }) @@ -284,12 +285,13 @@ describe('native Chromium import excludes the Google cookie family', () => { throw new Error('cookie store unavailable') } }) + clearDataMock.mockRejectedValue(new Error('storage busy')) const result = await importCookiesFromBrowser(chromeBrowser(sourceCookiesPath), 'persist:test') expect(result).toMatchObject({ ok: false }) expect(result.ok || result.reason).toContain('Could not clear existing cookies') - expect(clearStorageDataMock).not.toHaveBeenCalled() + expect(clearDataMock).toHaveBeenCalledOnce() expect(cookiesRemoveMock.mock.calls.map(([, name]) => name)).toEqual(['removed-first', 'stale']) expect(cookiesSetMock).not.toHaveBeenCalled() expect(setPendingCookieImportMock).not.toHaveBeenCalled() 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 f3b4a49ed8d..bbd0d9a8a25 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 @@ -78,8 +78,8 @@ async function run() { await targetSession.cookies.set({ url: 'https://plain.example/', name: 'plain', value: 'stale', secure: true }) await targetSession.cookies.set({ url: 'https://victim.example/', name: 'victim', value: 'stale', secure: true }) - // Why (STA-4065): an excluded cookie is what forces the per-cookie path; without one the bulk - // fast path clears the jar in a single call and never exercises a partial-failure clear. + // Why: keep a live excluded cookie in the partial-failure fixture so the fallback proves it + // preserves that cookie while deleting the removable ones it can reach. await targetSession.cookies.set({ url: 'https://accounts.google.com/', name: 'SID', value: 'live', secure: true }) mark('removable cookies set') diff --git a/src/main/browser/browser-cookie-import-partition.electron.test.ts b/src/main/browser/browser-cookie-import-partition.electron.test.ts index 5fa7c0d4fc0..e844341e12f 100644 --- a/src/main/browser/browser-cookie-import-partition.electron.test.ts +++ b/src/main/browser/browser-cookie-import-partition.electron.test.ts @@ -20,7 +20,6 @@ type FixtureResult = { before: Record after: Record electronCookieKeys: string[] - unfilteredNames: string[] importResult: Record remainingNames: string[] } @@ -75,13 +74,26 @@ async function run() { value: 'remove-me', secure: true }) + // Why: the excluded origin is HTTPS, so prove registrable-domain matching also keeps HTTP. + await targetSession.cookies.set({ + url: 'http://mail.google.com/', + name: 'http-google', + value: 'live-http-value', + secure: false + }) + await targetSession.cookies.set({ + url: 'https://google.com/', + domain: '.google.com', + name: 'domain-google', + value: 'live-domain-value', + secure: true + }) mark('ordinary cookie set') const beforeCookies = (await debug.sendCommand('Network.getAllCookies')).cookies const before = beforeCookies.find((cookie) => cookie.name === 'partitioned-google') const electronCookie = (await targetSession.cookies.get({ name: 'partitioned-google' }))[0] if (!before || !electronCookie) throw new Error('Partitioned fixture cookie was not created') - const unfilteredNames = (await targetSession.cookies.get({})).map((cookie) => cookie.name).sort() mark('cookies read') const importResult = await importCookiesFromBrowser({ @@ -102,7 +114,6 @@ async function run() { before, after, electronCookieKeys: Object.keys(electronCookie).sort(), - unfilteredNames, importResult, remainingNames: afterCookies.map((cookie) => cookie.name).sort() })) @@ -187,14 +198,16 @@ describe('native Chromium excluded partition cookie under Electron', () => { hasCrossSiteAncestor: true }) expect(result.electronCookieKeys).not.toContain('partitionKey') - // Why: the bulk-clear fast path is gated on an unfiltered get() seeing every excluded cookie, - // so a partitioned one going missing here would silently arm a jar-wide wipe. - expect(result.unfilteredNames).toEqual(['partitioned-google', 'stale']) expect(result.after).toEqual(result.before) expect(result.importResult).toMatchObject({ ok: true, summary: { importedCookies: 1, domains: ['example.com'] } }) - expect(result.remainingNames).toEqual(['imported', 'partitioned-google']) + expect(result.remainingNames).toEqual([ + 'domain-google', + 'http-google', + 'imported', + 'partitioned-google' + ]) }, 90_000) }) diff --git a/src/main/browser/browser-cookie-import-policy.test.ts b/src/main/browser/browser-cookie-import-policy.test.ts index 7b2fad0bfdc..51a1a6107d5 100644 --- a/src/main/browser/browser-cookie-import-policy.test.ts +++ b/src/main/browser/browser-cookie-import-policy.test.ts @@ -303,6 +303,28 @@ describe('removeTransplantableCookies', () => { ]) }) + it('re-reads the jar after a rejected bulk clear', async () => { + const beforeAttempt = [cookie('.removed.test', 'gone-before-fallback')] + const afterAttempt = [ + cookie('.google.com', 'SID'), + cookie('.survivor.test', 'survived'), + cookie('.arrived.test', 'arrived-during-clear') + ] + const get = vi.fn().mockResolvedValueOnce(beforeAttempt).mockResolvedValueOnce(afterAttempt) + const { session, remove } = clearSession(beforeAttempt, { + get, + clearData: rejectingBulkClear() + }) + + await removeTransplantableCookies(session) + + expect(get).toHaveBeenCalledTimes(2) + expect(remove.mock.calls).toEqual([ + ['https://survivor.test/', 'survived'], + ['https://arrived.test/', 'arrived-during-clear'] + ]) + }) + // 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 () => { diff --git a/src/main/browser/browser-cookie-import-policy.ts b/src/main/browser/browser-cookie-import-policy.ts index 315b5eccdd6..6f81407f4a9 100644 --- a/src/main/browser/browser-cookie-import-policy.ts +++ b/src/main/browser/browser-cookie-import-policy.ts @@ -59,6 +59,8 @@ export function normalizeCookieImportDomain(domain: string): string | null { // it is copied. Signing in directly inside Orca is the only path that produces a working // session, so an import must never write these cookies and never remove them either — the // live session is always more valuable than anything an import could put in its place. +// Entries must be canonical lowercase ASCII (punycode) registrable domains, never subdomains or +// public suffixes, because clearData derives one excluded origin and matches at that boundary. // Adding a site is one entry here. // youtube.com is deliberately NOT listed: YouTube accepts a transplanted session and re-issues // its cookies via the accounts.youtube.com relay, so excluding it would silently drop imports @@ -218,8 +220,8 @@ export async function removeTransplantableCookies( targetSession: CookieClearSession ): Promise { const store = targetSession.cookies - const existingCookies = await store.get({}) - if (existingCookies.length === 0) { + const initialCookies = await store.get({}) + if (initialCookies.length === 0) { return } @@ -234,10 +236,11 @@ export async function removeTransplantableCookies( }) 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: a rejected bulk clear can still have changed the jar, so the fallback must act on the + // survivors rather than stale removal coordinates from before the attempt. } + const existingCookies = await store.get({}) const removableGroups = new Map() for (const cookie of existingCookies) { if (isNonTransplantableCookieDomain(cookie.domain ?? '')) { diff --git a/src/main/browser/browser-cookie-import-replacement.test.ts b/src/main/browser/browser-cookie-import-replacement.test.ts index 8b5c07bb057..db70922ccb5 100644 --- a/src/main/browser/browser-cookie-import-replacement.test.ts +++ b/src/main/browser/browser-cookie-import-replacement.test.ts @@ -168,7 +168,7 @@ describe('validated cookie replacement', () => { }) describe('native Chromium integrity-cookie accounting', () => { - let clearStorageDataMock: ReturnType + let clearDataMock: ReturnType let cookiesSetMock: ReturnType let tmpDir: string @@ -180,7 +180,7 @@ describe('native Chromium integrity-cookie accounting', () => { }) clearPendingCookieImportMock.mockClear() setPendingCookieImportMock.mockClear() - clearStorageDataMock = vi.fn().mockResolvedValue(undefined) + clearDataMock = vi.fn().mockResolvedValue(undefined) cookiesSetMock = vi.fn().mockResolvedValue(undefined) sessionFromPartitionMock.mockReset().mockReturnValue({ cookies: { @@ -189,7 +189,7 @@ describe('native Chromium integrity-cookie accounting', () => { remove: vi.fn().mockResolvedValue(undefined), set: cookiesSetMock }, - clearStorageData: clearStorageDataMock + clearData: clearDataMock }) }) @@ -252,7 +252,7 @@ describe('native Chromium integrity-cookie accounting', () => { googleCookiesSkipped: 2, domains: [] }) - expect(clearStorageDataMock).not.toHaveBeenCalled() + expect(clearDataMock).not.toHaveBeenCalled() expect(cookiesSetMock).not.toHaveBeenCalled() expect(setPendingCookieImportMock).not.toHaveBeenCalled() expect(clearPendingCookieImportMock).not.toHaveBeenCalled()