From dcc03fd57a37c578e7db96db32420bb700fd4d85 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 12 Aug 2026 18:01:21 -0700 Subject: [PATCH] fix(browser): never resurrect cookies after a failed import clear (STA-4061) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit removeAllCookiesExcept rolled a partial clear back by rebuilding each removed cookie with cookies.set. Electron's cookies.get() omits partitionKey and cookies.set() drops it, so a non-Google CHIPS cookie that had been removed came back as an ordinary unpartitioned cookie — silent, restart-proof auth-state corruption on a code path that reports failure. The exclusion comment already stated the invariant ("Electron cannot round-trip partition identity"); it was only applied to the Google family. Extend it to the rollback: a failed clear now stays failed, which is retryable, instead of reconstructing cookies whose identity cannot be reproduced. --- ...ser-cookie-import-google-exclusion.test.ts | 6 +- ...import-partition-rollback.electron.test.ts | 175 ++++++++++++++++++ .../browser-cookie-import-policy.test.ts | 17 +- .../browser/browser-cookie-import-policy.ts | 22 +-- 4 files changed, 199 insertions(+), 21 deletions(-) create mode 100644 src/main/browser/browser-cookie-import-partition-rollback.electron.test.ts 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 d1e13737b1d..3cc329a14d7 100644 --- a/src/main/browser/browser-cookie-import-google-exclusion.test.ts +++ b/src/main/browser/browser-cookie-import-google-exclusion.test.ts @@ -267,7 +267,9 @@ describe('native Chromium import excludes the Google cookie family', () => { ]) }) - it('fails the import without reconstructing Google cookies when selective removal rejects', async () => { + // Why (STA-4061): reconstruction drops partition keys, so a rejected removal must not rebuild + // anything — not the excluded Google rows, and not the non-Google rows that were already gone. + it('fails the import without reconstructing any cookie when selective removal rejects', async () => { const sourceCookiesPath = seedSource([ { domain: '.example.com', name: 'session', value: 'new' } ]) @@ -289,7 +291,7 @@ describe('native Chromium import excludes the Google cookie family', () => { expect(result.ok || result.reason).toContain('Could not clear existing cookies') expect(clearStorageDataMock).not.toHaveBeenCalled() expect(cookiesRemoveMock.mock.calls.map(([, name]) => name)).toEqual(['removed-first', 'stale']) - expect(cookiesSetMock.mock.calls.map(([details]) => details.name)).toEqual(['removed-first']) + 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 new file mode 100644 index 00000000000..d0014a2d6de --- /dev/null +++ b/src/main/browser/browser-cookie-import-partition-rollback.electron.test.ts @@ -0,0 +1,175 @@ +import { spawnSync } from 'node:child_process' +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { createRequire } from 'node:module' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterAll, describe, expect, it } from 'vitest' +import { build as buildVite } from 'vite' + +const electronBinary = createRequire(import.meta.url)('electron') as string +const fixtureRoots: string[] = [] + +afterAll(() => { + for (const root of fixtureRoots) { + rmSync(root, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }) + } +}) + +type CdpCookie = { name: string; value: string; partitionKey?: Record } + +type FixtureResult = { + beforePartitionKey: Record | undefined + clearError: string | null + remainingChips: CdpCookie[] + remainingPlain: CdpCookie[] +} + +const EXPECTED_PARTITION_KEY = { + topLevelSite: 'https://top.example', + hasCrossSiteAncestor: true +} + +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 resultPath = ${JSON.stringify(resultPath)} +let currentStep = 'starting' +const mark = (step) => { + currentStep = step + writeFileSync(resultPath, JSON.stringify({ step })) +} + +async function run() { + const timeout = setTimeout(() => { + writeFileSync(resultPath, JSON.stringify({ step: 'timed out after ' + currentStep })) + app.exit(1) + }, 20000) + await app.whenReady() + mark('ready') + const partition = 'persist:partition-rollback-cookie-test' + const targetSession = session.fromPartition(partition) + const window = new BrowserWindow({ show: false, webPreferences: { partition } }) + mark('window created') + await window.loadURL('data:text/html,cookie rollback fixture') + mark('window loaded') + const debug = window.webContents.debugger + debug.attach('1.3') + mark('debugger attached') + await debug.sendCommand('Network.enable') + mark('network enabled') + + // Only CDP can create a partitioned cookie; Electron's cookies API has no partitionKey. + await debug.sendCommand('Network.setCookie', { + url: 'https://app.acme-chips.test/', + name: 'chips-auth', + value: 'keep-me', + secure: true, + sameSite: 'None', + partitionKey: ${JSON.stringify(EXPECTED_PARTITION_KEY)} + }) + const beforeChips = (await debug.sendCommand('Network.getAllCookies')).cookies + .find((cookie) => cookie.name === 'chips-auth') + if (!beforeChips || !beforeChips.partitionKey) throw new Error('CHIPS fixture cookie was not stored partitioned') + mark('partitioned cookie set') + + 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 }) + mark('removable cookies set') + + const store = { + get: (filter) => targetSession.cookies.get(filter), + set: (details) => targetSession.cookies.set(details), + // Why: one rejecting removal is what drove the old rollback; the rest still succeed. + remove: async (url, name) => { + if (name === 'victim') throw new Error('forced victim removal failure') + return targetSession.cookies.remove(url, name) + } + } + + let clearError = null + try { + await removeAllCookiesExcept(store, (cookie) => isNonTransplantableCookieDomain(cookie.domain ?? '')) + } catch (error) { + clearError = String(error?.message || error) + } + mark('clear finished') + + const afterCookies = (await debug.sendCommand('Network.getAllCookies')).cookies + const project = (name) => afterCookies + .filter((cookie) => cookie.name === name) + .map((cookie) => ({ name: cookie.name, value: cookie.value, partitionKey: cookie.partitionKey })) + clearTimeout(timeout) + writeFileSync(resultPath, JSON.stringify({ + beforePartitionKey: beforeChips.partitionKey, + clearError, + remainingChips: project('chips-auth'), + remainingPlain: project('plain') + })) + debug.detach() + window.destroy() + app.exit(0) +} + +run().catch((error) => { + writeFileSync(resultPath, JSON.stringify({ step: currentStep, error: String(error?.stack || error) })) + app.exit(1) +}) +` +} + +async function runFixture(): Promise { + const root = mkdtempSync(join(tmpdir(), 'orca-partition-rollback-')) + fixtureRoots.push(root) + const policyPath = join(root, 'browser-cookie-import-policy.cjs') + const resultPath = join(root, 'result.json') + const fixturePath = join(root, 'main.cjs') + await buildVite({ + configFile: false, + logLevel: 'silent', + build: { + emptyOutDir: false, + lib: { + entry: join(process.cwd(), 'src/main/browser/browser-cookie-import-policy.ts'), + formats: ['cjs'], + fileName: () => 'browser-cookie-import-policy.cjs' + }, + outDir: root, + target: 'node20', + rollupOptions: { external: ['electron', /^node:/] } + } + }) + writeFileSync(fixturePath, buildFixtureMain(policyPath, resultPath)) + const { ELECTRON_RUN_AS_NODE: _electronRunAsNode, ...env } = process.env + const electronArgs = [fixturePath, `--user-data-dir=${join(root, 'profile')}`] + const executable = process.platform === 'linux' ? 'xvfb-run' : electronBinary + const args = + process.platform === 'linux' + ? ['--auto-servernum', electronBinary, ...electronArgs, '--no-sandbox'] + : electronArgs + const run = spawnSync(executable, args, { + encoding: 'utf8', + env, + timeout: 60_000 + }) + const fixtureResult = existsSync(resultPath) ? readFileSync(resultPath, 'utf8') : 'no result' + expect(run.error).toBeUndefined() + expect(run.status, `${fixtureResult}\n${run.stdout}\n${run.stderr}`).toBe(0) + return JSON.parse(fixtureResult) as FixtureResult +} + +describe('non-Google partitioned cookie under a failed Electron cookie clear', () => { + it('never resurrects a removed cookie without its partition key', async () => { + const result = await runFixture() + + expect(result.beforePartitionKey).toEqual(EXPECTED_PARTITION_KEY) + expect(result.clearError).toContain('Could not clear existing cookies') + // STA-4061: rollback rebuilt this cookie through cookies.set, which drops partitionKey. + expect( + result.remainingChips.filter((cookie) => !cookie.partitionKey), + `chips-auth was downgraded to unpartitioned: ${JSON.stringify(result.remainingChips)}` + ).toEqual([]) + expect(result.remainingPlain).toEqual([]) + }, 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 96d023fd2f5..f007c07d8ff 100644 --- a/src/main/browser/browser-cookie-import-policy.test.ts +++ b/src/main/browser/browser-cookie-import-policy.test.ts @@ -220,7 +220,7 @@ describe('removeAllCookiesExcept', () => { const remove = vi.fn().mockResolvedValue(undefined) const set = vi.fn().mockResolvedValue(undefined) - await removeAllCookiesExcept({ get, remove, set }, (existingCookie) => + await removeAllCookiesExcept({ get, remove }, (existingCookie) => isNonTransplantableCookieDomain(existingCookie.domain ?? '') ) @@ -231,7 +231,9 @@ describe('removeAllCookiesExcept', () => { expect(set).not.toHaveBeenCalled() }) - it('restores removed non-Google cookies when another removal fails', async () => { + // 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 () => { const get = vi .fn() .mockResolvedValue([ @@ -248,12 +250,12 @@ describe('removeAllCookiesExcept', () => { const set = vi.fn().mockResolvedValue(undefined) await expect( - removeAllCookiesExcept({ get, remove, set }, (existingCookie) => + removeAllCookiesExcept({ get, remove }, (existingCookie) => isNonTransplantableCookieDomain(existingCookie.domain ?? '') ) - ).rejects.toThrow('Could not clear existing cookies') + ).rejects.toThrow('the session was left partially cleared') expect(remove).toHaveBeenCalledTimes(3) - expect(set.mock.calls.map(([details]) => details.name).sort()).toEqual(['first', 'third']) + expect(set).not.toHaveBeenCalled() }) it('bounds parallel removals so large cookie jars do not clear serially or fan out', async () => { @@ -276,7 +278,7 @@ describe('removeAllCookiesExcept', () => { }) const set = vi.fn().mockResolvedValue(undefined) - const clearing = removeAllCookiesExcept({ get, remove, set }, () => false) + const clearing = removeAllCookiesExcept({ get, remove }, () => false) await vi.waitFor(() => expect(remove).toHaveBeenCalledTimes(8)) expect(maxActive).toBe(8) releaseRemovals?.() @@ -301,9 +303,8 @@ describe('removeAllCookiesExcept', () => { .fn() .mockImplementationOnce(() => firstReleased) .mockResolvedValueOnce(undefined) - const set = vi.fn().mockResolvedValue(undefined) - const clearing = removeAllCookiesExcept({ get, remove, set }, () => false) + const clearing = removeAllCookiesExcept({ get, remove }, () => false) 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 4fa7f44ddd9..27be9512218 100644 --- a/src/main/browser/browser-cookie-import-policy.ts +++ b/src/main/browser/browser-cookie-import-policy.ts @@ -197,8 +197,13 @@ export async function restoreImportedDomainCookies( } // Why: Electron cannot round-trip partition identity, so excluded cookies must never be removed. +// Why (STA-4061): the same gap forbids rolling a partial clear back. cookies.get() omits +// partitionKey and cookies.set() silently drops it, so every reconstruction is a coin flip that +// 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( - store: Pick, + store: Pick, isExcluded: (cookie: Cookie) => boolean ): Promise { const existingCookies = await store.get({}) @@ -218,7 +223,6 @@ export async function removeAllCookiesExcept( removableGroups.set(key, group) } - const removedCookies: Cookie[] = [] const results = await mapSettledWithConcurrency( [...removableGroups.values()], COOKIE_CLEAR_CONCURRENCY, @@ -226,22 +230,18 @@ export async function removeAllCookiesExcept( // Why: identical removal coordinates must stay ordered instead of racing. for (const { cookie, url } of group) { await store.remove(url, cookie.name) - removedCookies.push(cookie) } } ) const failures = results.flatMap((result) => result.status === 'rejected' ? [result.reason] : [] ) - if (failures.length === 0) { - return + if (failures.length > 0) { + throw new AggregateError( + failures, + 'Could not clear existing cookies; the session was left partially cleared' + ) } - try { - await restoreImportedDomainCookies(store, removedCookies) - } catch (restoreError) { - throw new AggregateError([...failures, restoreError], 'Cookie clearing and rollback failed') - } - throw new AggregateError(failures, 'Could not clear existing cookies') } export async function replaceCookiesForImportedDomains(