diff --git a/src/main/browser/browser-cookie-clear-store.test.ts b/src/main/browser/browser-cookie-clear-store.test.ts new file mode 100644 index 00000000000..e44d71c15dc --- /dev/null +++ b/src/main/browser/browser-cookie-clear-store.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it } from 'vitest' +import type { Cookie } from 'electron' +import { + cdpRestoreParamsFromIdentity, + cookieClearIdentitiesFromCdp +} from './browser-cookie-clear-store' + +const chipsCookie: Cookie = { + domain: 'app.acme-chips.test', + name: 'chips-auth', + path: '/', + secure: true, + sameSite: 'no_restriction', + value: 'keep-me' +} + +describe('cookie clear CDP identities', () => { + it('captures a CHIPS partition key for restore and never invents one', () => { + const identities = cookieClearIdentitiesFromCdp( + [{ cookie: chipsCookie, url: 'https://app.acme-chips.test/' }], + [ + { + name: 'chips-auth', + value: 'keep-me', + domain: 'app.acme-chips.test', + path: '/', + secure: true, + sameSite: 'None', + partitionKey: { topLevelSite: 'https://top.example', hasCrossSiteAncestor: true } + } + ] + ) + + expect(identities).toEqual([ + expect.objectContaining({ + name: 'chips-auth', + partitionKey: { topLevelSite: 'https://top.example', hasCrossSiteAncestor: true } + }) + ]) + expect(cdpRestoreParamsFromIdentity(identities[0])).toEqual( + expect.objectContaining({ + name: 'chips-auth', + sameSite: 'None', + partitionKey: { topLevelSite: 'https://top.example', hasCrossSiteAncestor: true } + }) + ) + }) + + it('fails closed when CDP cannot identify a removable cookie', () => { + expect(() => + cookieClearIdentitiesFromCdp( + [{ cookie: chipsCookie, url: 'https://app.acme-chips.test/' }], + [] + ) + ).toThrow(/Could not snapshot cookie identity/) + }) +}) diff --git a/src/main/browser/browser-cookie-clear-store.ts b/src/main/browser/browser-cookie-clear-store.ts new file mode 100644 index 00000000000..f0817cb5830 --- /dev/null +++ b/src/main/browser/browser-cookie-clear-store.ts @@ -0,0 +1,255 @@ +import { BrowserWindow, webContents, type Cookie, type Session } from 'electron' +import { acquireElectronDebugger } from './electron-debugger-lease' +import { normalizeCookieDomain } from './browser-cookie-import-policy' +import type { + CookieClearIdentity, + CookieClearPartitionKey, + CookieClearStore +} from './browser-cookie-import-clear' + +type CdpCookiePartitionKey = { + topLevelSite?: string + hasCrossSiteAncestor?: boolean +} + +type CdpCookie = { + name: string + value: string + domain?: string + path?: string + secure?: boolean + httpOnly?: boolean + session?: boolean + expires?: number + sameSite?: string + partitionKey?: CdpCookiePartitionKey +} + +type CookieClearDebugger = { + sendCommand: (method: string, params?: Record) => Promise +} + +type CookieClearSession = { + debugger: CookieClearDebugger + dispose: () => void +} + +function findPartitionWebContents(targetSession: Session) { + return webContents + .getAllWebContents() + .find((contents) => !contents.isDestroyed() && contents.session === targetSession) +} + +function cdpSameSite(sameSite: Cookie['sameSite']): 'Strict' | 'Lax' | 'None' { + if (sameSite === 'strict') { + return 'Strict' + } + if (sameSite === 'no_restriction') { + return 'None' + } + return 'Lax' +} + +function electronSameSite(sameSite: string | undefined): Cookie['sameSite'] { + if (sameSite === 'Strict') { + return 'strict' + } + if (sameSite === 'None') { + return 'no_restriction' + } + return sameSite === 'Lax' ? 'lax' : 'unspecified' +} + +function partitionKeyFromCdp( + partitionKey: CdpCookiePartitionKey | undefined +): CookieClearPartitionKey | undefined { + const topLevelSite = partitionKey?.topLevelSite + if (!topLevelSite) { + return undefined + } + return { + topLevelSite, + hasCrossSiteAncestor: partitionKey.hasCrossSiteAncestor === true + } +} + +function sameCookieScope(cookie: Cookie, cdpCookie: CdpCookie): boolean { + const cookieDomain = cookie.domain ? normalizeCookieDomain(cookie.domain) : null + const cdpDomain = cdpCookie.domain ? normalizeCookieDomain(cdpCookie.domain) : null + return ( + cookie.name === cdpCookie.name && + cookieDomain !== null && + cookieDomain === cdpDomain && + (cookie.path || '/') === (cdpCookie.path || '/') + ) +} + +function identityFromCdpCookie( + url: string, + cookie: Cookie, + cdpCookie: CdpCookie +): CookieClearIdentity { + const partitionKey = partitionKeyFromCdp(cdpCookie.partitionKey) + return { + url, + name: cdpCookie.name, + value: cdpCookie.value, + domain: cdpCookie.domain, + hostOnly: cookie.hostOnly, + path: cdpCookie.path, + secure: cdpCookie.secure, + httpOnly: cdpCookie.httpOnly, + sameSite: electronSameSite(cdpCookie.sameSite), + ...(cdpCookie.session === true || cdpCookie.expires == null + ? {} + : { expirationDate: cdpCookie.expires }), + ...(partitionKey ? { partitionKey } : {}) + } +} + +async function attachCookieClearSession(targetSession: Session): Promise { + const existing = findPartitionWebContents(targetSession) + const window = existing + ? null + : new BrowserWindow({ + show: false, + webPreferences: { + session: targetSession, + sandbox: true, + contextIsolation: true, + nodeIntegration: false + } + }) + try { + if (window) { + await window.loadURL('data:text/html,cookie-clear') + } + const contents = existing ?? window?.webContents + if (!contents || contents.isDestroyed()) { + throw new Error('Could not attach to the cookie session for an atomic clear') + } + const lease = acquireElectronDebugger(contents) + try { + await contents.debugger.sendCommand('Network.enable') + } catch (error) { + lease.release() + throw error + } + return { + debugger: contents.debugger, + dispose: () => { + lease.release() + window?.destroy() + } + } + } catch (error) { + window?.destroy() + throw error + } +} + +export function cookieClearIdentitiesFromCdp( + cookies: readonly { cookie: Cookie; url: string }[], + cdpCookies: readonly CdpCookie[] +): CookieClearIdentity[] { + const identities: CookieClearIdentity[] = [] + const seen = new Set() + for (const item of cookies) { + const matches = cdpCookies.filter((cdpCookie) => sameCookieScope(item.cookie, cdpCookie)) + if (matches.length === 0) { + throw new Error('Could not snapshot cookie identity for an atomic clear') + } + for (const match of matches) { + const key = JSON.stringify([ + item.url, + match.name, + match.domain, + match.path, + partitionKeyFromCdp(match.partitionKey) ?? null + ]) + if (seen.has(key)) { + continue + } + seen.add(key) + identities.push(identityFromCdpCookie(item.url, item.cookie, match)) + } + } + return identities +} + +export function cdpRestoreParamsFromIdentity( + identity: CookieClearIdentity +): Record { + return { + url: identity.url, + name: identity.name, + value: identity.value, + ...(identity.hostOnly ? {} : { domain: identity.domain }), + ...(identity.path ? { path: identity.path } : {}), + secure: identity.secure, + httpOnly: identity.httpOnly, + sameSite: cdpSameSite(identity.sameSite), + ...(identity.expirationDate ? { expires: identity.expirationDate } : {}), + ...(identity.partitionKey ? { partitionKey: identity.partitionKey } : {}) + } +} + +function cdpCookiesFromCommand(value: unknown): CdpCookie[] { + if (typeof value !== 'object' || value === null || !('cookies' in value)) { + return [] + } + const cookies = value.cookies + return Array.isArray(cookies) ? cookies : [] +} + +function cdpSetCookieSucceeded(value: unknown): boolean { + if (typeof value !== 'object' || value === null || !('success' in value)) { + return true + } + return value.success !== false +} + +async function snapshotClearIdentitiesFromCdp( + cookieDebugger: CookieClearDebugger, + cookies: readonly { cookie: Cookie; url: string }[] +): Promise { + const result = await cookieDebugger.sendCommand('Network.getAllCookies') + return cookieClearIdentitiesFromCdp(cookies, cdpCookiesFromCommand(result)) +} + +async function restoreClearIdentitiesWithCdp( + cookieDebugger: CookieClearDebugger, + identities: readonly CookieClearIdentity[] +): Promise { + for (const identity of identities) { + const result = await cookieDebugger.sendCommand( + 'Network.setCookie', + cdpRestoreParamsFromIdentity(identity) + ) + if (!cdpSetCookieSucceeded(result)) { + throw new Error(`Could not restore cookie ${identity.name}`) + } + } +} + +export function openCookieClearStore( + targetSession: Session +): CookieClearStore & { dispose: () => void } { + let attached: CookieClearSession | null = null + const attach = async () => { + attached ??= await attachCookieClearSession(targetSession) + return attached + } + return { + get: (filter) => targetSession.cookies.get(filter), + remove: (url, name) => targetSession.cookies.remove(url, name), + snapshotClearIdentities: async (cookies) => + snapshotClearIdentitiesFromCdp((await attach()).debugger, cookies), + restoreClearIdentities: async (identities) => + restoreClearIdentitiesWithCdp((await attach()).debugger, identities), + dispose: () => { + attached?.dispose() + attached = null + } + } +} diff --git a/src/main/browser/browser-cookie-import-clear-atomicity.test.ts b/src/main/browser/browser-cookie-import-clear-atomicity.test.ts new file mode 100644 index 00000000000..927df9b145b --- /dev/null +++ b/src/main/browser/browser-cookie-import-clear-atomicity.test.ts @@ -0,0 +1,179 @@ +import { describe, expect, it, vi } from 'vitest' +import type { Cookie } from 'electron' +import { + identitiesFromClearCookies, + removeTransplantableCookies, + type CookieClearIdentity, + type CookieClearSession +} from './browser-cookie-import-clear' + +function cookie(domain: string, name: string, path = '/', secure = true): Cookie { + return { + domain, + name, + path, + secure, + sameSite: 'unspecified', + value: `${name}-value` + } +} + +function createJarSession( + initial: Cookie[], + options: { + failOn?: string + restoreError?: Error + snapshot?: CookieClearSession['snapshotClearIdentities'] + } = {} +) { + let jar = [...initial] + const removedNames: string[] = [] + const session: CookieClearSession & { + names: () => string[] + removedNames: () => string[] + } = { + cookies: { + get: async () => [...jar], + remove: async (_url, name) => { + if (name === (options.failOn ?? 'stale')) { + throw new Error('cookie store unavailable') + } + removedNames.push(name) + jar = jar.filter((entry) => entry.name !== name) + } + }, + clearData: async () => { + throw new Error('storage busy') + }, + snapshotClearIdentities: + options.snapshot ?? (async (items) => identitiesFromClearCookies(items)), + restoreClearIdentities: async (identities) => { + if (options.restoreError) { + throw options.restoreError + } + for (const identity of identities) { + if (jar.some((entry) => entry.name === identity.name)) { + continue + } + jar.push(cookie(identity.domain ?? '', identity.name, identity.path, identity.secure)) + } + }, + names: () => jar.map((entry) => entry.name).sort(), + removedNames: () => [...removedNames] + } + return session +} + +describe('STA-4090 failed full cookie clear', () => { + it('does not permanently delete a cookie removed before another removal rejects', async () => { + const session = createJarSession([ + cookie('.google.com', 'SID'), + cookie('.example.com', 'removed-first', '/one'), + cookie('.other.test', 'stale', '/two') + ]) + + await expect(removeTransplantableCookies(session)).rejects.toThrow( + /existing cookies were restored/ + ) + + expect(session.removedNames()).toEqual(['removed-first']) + expect(session.names()).toEqual(['SID', 'removed-first', 'stale']) + }) + + it('does not start removing when a cookie identity cannot be snapshotted', async () => { + const session = createJarSession( + [cookie('.example.com', 'removed-first'), cookie('.other.test', 'stale')], + { + snapshot: async (items) => + identitiesFromClearCookies(items.filter(({ cookie: entry }) => entry.name !== 'stale')) + } + ) + + await expect(removeTransplantableCookies(session)).rejects.toThrow( + /the session was left unchanged/ + ) + expect(session.removedNames()).toEqual([]) + expect(session.names()).toEqual(['removed-first', 'stale']) + }) + + it('reports a partial clear only when restore also fails', async () => { + const session = createJarSession( + [cookie('.example.com', 'removed-first'), cookie('.other.test', 'stale')], + { restoreError: new Error('restore rejected') } + ) + + await expect(removeTransplantableCookies(session)).rejects.toThrow( + /the session was left partially cleared/ + ) + expect(session.names()).toEqual(['stale']) + }) + + it('never restores through cookies.set for a partitioned identity', async () => { + const set = vi.fn() + const identities: CookieClearIdentity[] = [] + const session: CookieClearSession = { + cookies: { + get: async () => [cookie('.example.com', 'removed-first'), cookie('.other.test', 'stale')], + remove: async (_url, name) => { + if (name === 'stale') { + throw new Error('cookie store unavailable') + } + } + }, + clearData: async () => { + throw new Error('storage busy') + }, + snapshotClearIdentities: async (items) => + identitiesFromClearCookies(items).map((identity) => + identity.name === 'removed-first' + ? { + ...identity, + partitionKey: { topLevelSite: 'https://top.example', hasCrossSiteAncestor: true } + } + : identity + ), + restoreClearIdentities: async (restored) => { + identities.push(...restored) + } + } + + await expect(removeTransplantableCookies(session)).rejects.toThrow( + /existing cookies were restored/ + ) + expect(set).not.toHaveBeenCalled() + expect(identities).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + name: 'removed-first', + partitionKey: { topLevelSite: 'https://top.example', hasCrossSiteAncestor: true } + }), + expect.objectContaining({ name: 'stale' }) + ]) + ) + }) + + it('serializes concurrent clears on the same session', async () => { + const activeClears: number[] = [] + let inClear = 0 + const session: CookieClearSession = { + cookies: { + get: async () => [cookie('.example.com', 'session')], + remove: async () => { + inClear += 1 + activeClears.push(inClear) + await Promise.resolve() + inClear -= 1 + } + }, + clearData: async () => { + throw new Error('storage busy') + }, + snapshotClearIdentities: async (items) => identitiesFromClearCookies(items), + restoreClearIdentities: async () => undefined + } + + await Promise.all([removeTransplantableCookies(session), removeTransplantableCookies(session)]) + + expect(activeClears).toEqual([1, 1]) + }) +}) diff --git a/src/main/browser/browser-cookie-import-clear.ts b/src/main/browser/browser-cookie-import-clear.ts new file mode 100644 index 00000000000..369ad5d432d --- /dev/null +++ b/src/main/browser/browser-cookie-import-clear.ts @@ -0,0 +1,192 @@ +import type { Cookie, Cookies, Session } from 'electron' +import { mapSettledWithConcurrency } from '../../shared/map-with-concurrency' +import { + cookieRemovalUrl, + isNonTransplantableCookieDomain, + NON_TRANSPLANTABLE_CLEAR_EXCLUDED_ORIGINS, + normalizeCookieDomain +} from './browser-cookie-import-policy' + +const COOKIE_CLEAR_CONCURRENCY = 8 + +export type CookieClearPartitionKey = { + topLevelSite: string + hasCrossSiteAncestor: boolean +} + +export type CookieClearIdentity = { + url: string + name: string + value: string + domain?: string + hostOnly?: boolean + path?: string + secure?: boolean + httpOnly?: boolean + sameSite: Cookie['sameSite'] + expirationDate?: number + partitionKey?: CookieClearPartitionKey +} + +export type CookieClearStore = Pick & { + snapshotClearIdentities( + cookies: readonly { cookie: Cookie; url: string }[] + ): Promise + restoreClearIdentities(identities: readonly CookieClearIdentity[]): Promise +} + +// Why (STA-4061): 'set' stays out so the lossy partition-dropping reconstruction cannot return. +export type CookieClearSession = { + cookies: Pick + clearData: Session['clearData'] + snapshotClearIdentities: CookieClearStore['snapshotClearIdentities'] + restoreClearIdentities: CookieClearStore['restoreClearIdentities'] +} + +const clearLocks = new WeakMap>() + +function cookieClearKey(url: string, name: string): string { + return JSON.stringify([url, name]) +} + +export function identitiesFromClearCookies( + cookies: readonly { cookie: Cookie; url: string }[] +): CookieClearIdentity[] { + return cookies.map(({ cookie, url }) => ({ + url, + name: cookie.name, + value: cookie.value, + domain: cookie.domain, + hostOnly: cookie.hostOnly, + path: cookie.path, + secure: cookie.secure, + httpOnly: cookie.httpOnly, + sameSite: cookie.sameSite, + expirationDate: cookie.expirationDate + })) +} + +export async function withCookieClearLock(owner: object, run: () => Promise): Promise { + const previous = clearLocks.get(owner) ?? Promise.resolve() + let release!: () => void + const current = new Promise((resolve) => { + release = resolve + }) + clearLocks.set( + owner, + previous.then(() => current) + ) + await previous + try { + return await run() + } finally { + release() + } +} + +function removableCookieEntries(cookies: readonly Cookie[]): { cookie: Cookie; url: string }[] { + const removable: { cookie: Cookie; url: string }[] = [] + for (const cookie of cookies) { + if (isNonTransplantableCookieDomain(cookie.domain ?? '')) { + continue + } + const domain = cookie.domain ? normalizeCookieDomain(cookie.domain) : null + const url = domain ? cookieRemovalUrl(cookie, domain) : null + if (!url) { + continue + } + removable.push({ cookie, url }) + } + return removable +} + +function assertClearIdentitiesCoverRemovable( + removable: readonly { cookie: Cookie; url: string }[], + identities: readonly CookieClearIdentity[] +): void { + const covered = new Set(identities.map((identity) => cookieClearKey(identity.url, identity.name))) + for (const item of removable) { + if (!covered.has(cookieClearKey(item.url, item.cookie.name))) { + throw new Error('Could not clear existing cookies; the session was left unchanged') + } + } +} + +function groupRemovableCookies( + removable: readonly { cookie: Cookie; url: string }[] +): Map { + const groups = new Map() + for (const item of removable) { + const key = cookieClearKey(item.url, item.cookie.name) + const group = groups.get(key) ?? [] + group.push(item) + groups.set(key, group) + } + return groups +} + +async function restoreClearedCookies( + targetSession: CookieClearSession, + identities: readonly CookieClearIdentity[], + failures: unknown[] +): Promise { + try { + await targetSession.restoreClearIdentities(identities.toReversed()) + } catch (restoreError) { + throw new AggregateError( + [...failures, restoreError], + 'Could not clear existing cookies; the session was left partially cleared' + ) + } + throw new AggregateError( + failures, + 'Could not clear existing cookies; existing cookies were restored' + ) +} + +export async function removeTransplantableCookies( + targetSession: CookieClearSession +): Promise { + return withCookieClearLock(targetSession, async () => { + const store = targetSession.cookies + const initialCookies = await store.get({}) + if (initialCookies.length === 0) { + return + } + + const initialRemovable = removableCookieEntries(initialCookies) + const identities = await targetSession.snapshotClearIdentities(initialRemovable) + assertClearIdentitiesCoverRemovable(initialRemovable, identities) + + try { + // Why (STA-4065): excludeOrigins keeps the google.com family, including partitioned + // cookies, so one call replaces a remove() per cookie on the ordinary import path. + 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. + } + + const existingCookies = await store.get({}) + const removableGroups = groupRemovableCookies(removableCookieEntries(existingCookies)) + const results = await mapSettledWithConcurrency( + [...removableGroups.values()], + COOKIE_CLEAR_CONCURRENCY, + async (group) => { + // Why: identical removal coordinates must stay ordered instead of racing. + for (const { cookie, url } of group) { + await store.remove(url, cookie.name) + } + } + ) + const failures = results.flatMap((result) => + result.status === 'rejected' ? [result.reason] : [] + ) + if (failures.length > 0) { + await restoreClearedCookies(targetSession, identities, failures) + } + }) +} 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 431ca758f2f..a213d42f16a 100644 --- a/src/main/browser/browser-cookie-import-google-exclusion.test.ts +++ b/src/main/browser/browser-cookie-import-google-exclusion.test.ts @@ -30,6 +30,78 @@ vi.mock('electron', () => ({ dialog: { showOpenDialog: vi.fn() }, session: { fromPartition: sessionFromPartitionMock } })) +vi.mock('./browser-cookie-clear-store', () => ({ + openCookieClearStore: (targetSession: { + cookies: { + get: (filter: object) => Promise + remove: (url: string, name: string) => Promise + set?: (details: Record) => Promise + } + }) => ({ + get: (filter: object) => targetSession.cookies.get(filter), + remove: (url: string, name: string) => targetSession.cookies.remove(url, name), + snapshotClearIdentities: async ( + items: { + cookie: { + name: string + value: string + domain?: string + path?: string + secure?: boolean + httpOnly?: boolean + sameSite: string + expirationDate?: number + hostOnly?: boolean + } + url: string + }[] + ) => + items.map(({ cookie, url }) => ({ + url, + name: cookie.name, + value: cookie.value, + domain: cookie.domain, + hostOnly: cookie.hostOnly, + path: cookie.path, + secure: cookie.secure, + httpOnly: cookie.httpOnly, + sameSite: cookie.sameSite, + expirationDate: cookie.expirationDate + })), + restoreClearIdentities: async ( + identities: { + url: string + name: string + value: string + domain?: string + path?: string + secure?: boolean + httpOnly?: boolean + sameSite: string + expirationDate?: number + hostOnly?: boolean + }[] + ) => { + if (!targetSession.cookies.set) { + return + } + for (const identity of identities.toReversed()) { + await targetSession.cookies.set({ + url: identity.url, + name: identity.name, + value: identity.value, + ...(identity.hostOnly ? {} : { domain: identity.domain }), + ...(identity.path ? { path: identity.path } : {}), + secure: identity.secure, + httpOnly: identity.httpOnly, + sameSite: identity.sameSite, + ...(identity.expirationDate ? { expirationDate: identity.expirationDate } : {}) + }) + } + }, + dispose: () => undefined + }) +})) import { importCookiesFromBrowser, importCookiesFromFile } from './browser-cookie-import' import { createChromiumCookieTestDatabase } from './browser-cookie-import-test-database' @@ -268,22 +340,29 @@ describe('native Chromium import excludes the Google cookie family', () => { ]) }) - // 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 () => { + // Why (STA-4090): a rejected removal must restore cookies already deleted in the same clear + // without writing imported rows or touching the live Google session. + it('fails the import without permanently deleting a cookie removed before a later rejection', async () => { const sourceCookiesPath = seedSource([ { domain: '.example.com', name: 'session', value: 'new' } ]) seedTarget([{ domain: '.example.com', name: 'stale', value: 'stale' }]) - cookiesGetMock.mockResolvedValue([ + let jar = [ existingCookie('.google.com', 'SID'), existingCookie('.example.com', 'removed-first'), existingCookie('.other.test', 'stale') - ]) + ] + cookiesGetMock.mockImplementation(async () => [...jar]) cookiesRemoveMock.mockImplementation(async (_url: string, name: string) => { if (name === 'stale') { throw new Error('cookie store unavailable') } + jar = jar.filter((entry) => entry.name !== name) + }) + cookiesSetMock.mockImplementation(async (details: { domain?: string; name: string }) => { + if (!jar.some((entry) => entry.name === details.name)) { + jar.push(existingCookie(details.domain ?? '.example.com', details.name)) + } }) clearDataMock.mockRejectedValue(new Error('storage busy')) @@ -293,7 +372,10 @@ describe('native Chromium import excludes the Google cookie family', () => { expect(result.ok || result.reason).toContain('Could not clear existing cookies') expect(clearDataMock).toHaveBeenCalledOnce() expect(cookiesRemoveMock.mock.calls.map(([, name]) => name)).toEqual(['removed-first', 'stale']) - expect(cookiesSetMock).not.toHaveBeenCalled() + expect(cookiesSetMock.mock.calls.map(([details]) => details.name)).toEqual( + expect.arrayContaining(['removed-first']) + ) + expect(jar.map((entry) => entry.name).sort()).toEqual(['SID', 'removed-first', 'stale']) 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 bbd0d9a8a25..574e27fbfc5 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 @@ -31,11 +31,11 @@ const EXPECTED_PARTITION_KEY = { hasCrossSiteAncestor: true } -function buildFixtureMain(policyPath: string, resultPath: string): string { +function buildFixtureMain(clearPath: string, resultPath: string): string { return ` const { app, BrowserWindow, session } = require('electron') const { writeFileSync } = require('node:fs') -const { removeTransplantableCookies } = require(${JSON.stringify(policyPath)}) +const { removeTransplantableCookies } = require(${JSON.stringify(clearPath)}) const resultPath = ${JSON.stringify(resultPath)} let currentStep = 'starting' const mark = (step) => { @@ -83,24 +83,54 @@ async function run() { await targetSession.cookies.set({ url: 'https://accounts.google.com/', name: 'SID', value: 'live', 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) - } - } - - // 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. + // Why: the bulk clear is the ordinary path now, so rejecting it routes this fixture onto + // the per-cookie fallback where a later removal can fail after earlier ones succeeded. let bulkClearCalls = 0 const clearSession = { - cookies: store, + cookies: { + get: (filter) => targetSession.cookies.get(filter), + remove: async (url, name) => { + if (name === 'victim') throw new Error('forced victim removal failure') + return targetSession.cookies.remove(url, name) + } + }, clearData: async () => { bulkClearCalls++ throw new Error('forced bulk clear failure') + }, + snapshotClearIdentities: async (cookies) => { + const { cookies: cdpCookies } = await debug.sendCommand('Network.getAllCookies') + return cookies.flatMap((item) => { + const matches = cdpCookies.filter((cdpCookie) => cdpCookie.name === item.cookie.name) + if (matches.length === 0) throw new Error('Could not snapshot cookie identity for an atomic clear') + return matches.map((cdpCookie) => ({ + url: item.url, + name: cdpCookie.name, + value: cdpCookie.value, + domain: cdpCookie.domain, + path: cdpCookie.path, + secure: cdpCookie.secure, + httpOnly: cdpCookie.httpOnly, + sameSite: cdpCookie.sameSite === 'None' ? 'no_restriction' : 'lax', + partitionKey: cdpCookie.partitionKey + })) + }) + }, + restoreClearIdentities: async (identities) => { + for (const identity of identities) { + const result = await debug.sendCommand('Network.setCookie', { + url: identity.url, + name: identity.name, + value: identity.value, + domain: identity.domain, + path: identity.path, + secure: identity.secure, + httpOnly: identity.httpOnly, + sameSite: identity.sameSite === 'no_restriction' ? 'None' : 'Lax', + partitionKey: identity.partitionKey + }) + if (result && result.success === false) throw new Error('CDP restore rejected ' + identity.name) + } } } @@ -140,7 +170,7 @@ run().catch((error) => { 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 clearPath = join(root, 'browser-cookie-import-clear.cjs') const resultPath = join(root, 'result.json') const fixturePath = join(root, 'main.cjs') await buildVite({ @@ -149,16 +179,16 @@ async function runFixture(): Promise { build: { emptyOutDir: false, lib: { - entry: join(process.cwd(), 'src/main/browser/browser-cookie-import-policy.ts'), + entry: join(process.cwd(), 'src/main/browser/browser-cookie-import-clear.ts'), formats: ['cjs'], - fileName: () => 'browser-cookie-import-policy.cjs' + fileName: () => 'browser-cookie-import-clear.cjs' }, outDir: root, target: 'node20', rollupOptions: { external: ['electron', /^node:/] } } }) - writeFileSync(fixturePath, buildFixtureMain(policyPath, resultPath)) + writeFileSync(fixturePath, buildFixtureMain(clearPath, 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 @@ -178,20 +208,18 @@ async function runFixture(): Promise { } describe('non-Google partitioned cookie under a failed Electron cookie clear', () => { - it('never resurrects a removed partitioned cookie', async () => { + // Why (STA-4090): a later fallback rejection must not permanently drop cookies already + // removed in the same clear — including CHIPS cookies cookies.set() cannot round-trip. + it('keeps already-removed CHIPS and ordinary cookies after a later removal rejects', async () => { const result = await runFixture() expect(result.beforePartitionKey).toEqual(EXPECTED_PARTITION_KEY) - // 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. - expect( - result.remainingChips, - `chips-auth survived: ${JSON.stringify(result.remainingChips)}` - ).toEqual([]) - expect(result.remainingPlain).toEqual([]) + expect(result.remainingChips).toEqual([ + { name: 'chips-auth', value: 'keep-me', partitionKey: EXPECTED_PARTITION_KEY } + ]) + expect(result.remainingPlain).toEqual([{ name: 'plain', value: 'stale' }]) }, 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 51a1a6107d5..ad83fd75950 100644 --- a/src/main/browser/browser-cookie-import-policy.test.ts +++ b/src/main/browser/browser-cookie-import-policy.test.ts @@ -1,12 +1,15 @@ import { describe, expect, it, vi, type Mock } from 'vitest' import { DatabaseSync } from 'node:sqlite' import type { Cookie } from 'electron' +import { + identitiesFromClearCookies, + removeTransplantableCookies +} from './browser-cookie-import-clear' import { isGoogleSourceBoundCookie, isNonTransplantableCookieDomain, NON_TRANSPLANTABLE_HOST_KEY_SQL, normalizeCookieDomain, - removeTransplantableCookies, replaceCookiesForImportedDomains } from './browser-cookie-import-policy' @@ -225,12 +228,20 @@ describe('removeTransplantableCookies', () => { ...overrides } const clearData = overrides.clearData ?? vi.fn().mockResolvedValue(undefined) + const restoreClearIdentities = vi.fn().mockResolvedValue(undefined) return { - session: { cookies: store, clearData }, + session: { + cookies: store, + clearData, + snapshotClearIdentities: async (items: Parameters[0]) => + identitiesFromClearCookies(items), + restoreClearIdentities + }, get: store.get, remove: store.remove, set: store.set, - clearData + clearData, + restoreClearIdentities } } @@ -343,10 +354,9 @@ describe('removeTransplantableCookies', () => { 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 () => { - const { session, remove, set } = clearSession( + // Why (STA-4090): a failed fallback must restore through captured identities, never cookies.set. + it('restores removed cookies through captured identities when another removal fails', async () => { + const { session, remove, set, restoreClearIdentities } = clearSession( [ cookie('.google.com', 'SID'), cookie('.example.com', 'first', '/one'), @@ -364,10 +374,14 @@ describe('removeTransplantableCookies', () => { ) await expect(removeTransplantableCookies(session)).rejects.toThrow( - 'the session was left partially cleared' + 'existing cookies were restored' ) expect(remove).toHaveBeenCalledTimes(3) expect(set).not.toHaveBeenCalled() + expect(restoreClearIdentities).toHaveBeenCalledOnce() + expect( + restoreClearIdentities.mock.calls[0][0].map((identity: { name: string }) => identity.name) + ).toEqual(expect.arrayContaining(['first', 'second', 'third'])) }) it('bounds parallel removals so large cookie jars do not clear serially or fan out', async () => { diff --git a/src/main/browser/browser-cookie-import-policy.ts b/src/main/browser/browser-cookie-import-policy.ts index 6f81407f4a9..9fa3bc5f9b7 100644 --- a/src/main/browser/browser-cookie-import-policy.ts +++ b/src/main/browser/browser-cookie-import-policy.ts @@ -1,6 +1,5 @@ -import type { Cookie, Cookies, Session } from 'electron' +import type { Cookie, Cookies } from 'electron' import { parse as parseDomain } from 'psl' -import { mapSettledWithConcurrency } from '../../shared/map-with-concurrency' const GOOGLE_SOURCE_BOUND_COOKIE_NAMES = new Set([ 'SIDCC', @@ -66,10 +65,9 @@ export function normalizeCookieImportDomain(domain: string): string | null { // its cookies via the accounts.youtube.com relay, so excluding it would silently drop imports // users actually asked for. const NON_TRANSPLANTABLE_DOMAINS = ['google.com'] as const -const NON_TRANSPLANTABLE_CLEAR_EXCLUDED_ORIGINS = NON_TRANSPLANTABLE_DOMAINS.map( +export const NON_TRANSPLANTABLE_CLEAR_EXCLUDED_ORIGINS = NON_TRANSPLANTABLE_DOMAINS.map( (root) => `https://${root}` ) -const COOKIE_CLEAR_CONCURRENCY = 8 export function isNonTransplantableCookieDomain(domain: string): boolean { const normalized = normalizeCookieDomain(domain) @@ -159,7 +157,7 @@ function overlapsImportedDomain( return domainSuffixes(domain).some((suffix) => scopes.descendantRoots.has(suffix)) } -function cookieRemovalUrl(cookie: Cookie, domain: string): string | null { +export function cookieRemovalUrl(cookie: Cookie, domain: string): string | null { try { const url = new URL(`${cookie.secure ? 'https' : 'http'}://${domain}/`) url.pathname = cookie.path?.startsWith('/') ? cookie.path : '/' @@ -201,83 +199,6 @@ export async function restoreImportedDomainCookies( } } -// Why (STA-4061): 'set' stays out so the lossy partition-dropping reconstruction cannot return. -export type CookieClearSession = { - cookies: Pick - clearData: Session['clearData'] -} - -// 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. -// 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 initialCookies = await store.get({}) - if (initialCookies.length === 0) { - return - } - - // 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 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 ?? '')) { - continue - } - const domain = cookie.domain ? normalizeCookieDomain(cookie.domain) : null - const url = domain ? cookieRemovalUrl(cookie, domain) : null - if (!url) { - continue - } - const key = JSON.stringify([url, cookie.name]) - const group = removableGroups.get(key) ?? [] - group.push({ cookie, url }) - removableGroups.set(key, group) - } - - const results = await mapSettledWithConcurrency( - [...removableGroups.values()], - COOKIE_CLEAR_CONCURRENCY, - async (group) => { - // Why: identical removal coordinates must stay ordered instead of racing. - for (const { cookie, url } of group) { - await store.remove(url, cookie.name) - } - } - ) - const failures = results.flatMap((result) => - result.status === 'rejected' ? [result.reason] : [] - ) - if (failures.length > 0) { - throw new AggregateError( - failures, - 'Could not clear existing cookies; the session was left partially cleared' - ) - } -} - export async function replaceCookiesForImportedDomains( store: Pick, importedDomains: readonly string[] diff --git a/src/main/browser/browser-cookie-import-replacement.test.ts b/src/main/browser/browser-cookie-import-replacement.test.ts index db70922ccb5..a727a8275f5 100644 --- a/src/main/browser/browser-cookie-import-replacement.test.ts +++ b/src/main/browser/browser-cookie-import-replacement.test.ts @@ -26,6 +26,21 @@ vi.mock('electron', () => ({ dialog: { showOpenDialog: vi.fn() }, session: { fromPartition: sessionFromPartitionMock } })) +vi.mock('./browser-cookie-clear-store', () => ({ + openCookieClearStore: (targetSession: { + cookies: { + get: (filter: object) => Promise + remove: (url: string, name: string) => Promise + } + }) => ({ + get: (filter: object) => targetSession.cookies.get(filter), + remove: (url: string, name: string) => targetSession.cookies.remove(url, name), + snapshotClearIdentities: async (items: { cookie: Record; url: string }[]) => + items.map(({ cookie, url }) => ({ url, ...cookie })), + restoreClearIdentities: async () => undefined, + dispose: () => undefined + }) +})) import { importCookiesFromBrowser, importCookiesFromFile } from './browser-cookie-import' import { createChromiumCookieTestDatabase } from './browser-cookie-import-test-database' diff --git a/src/main/browser/browser-cookie-import.test.ts b/src/main/browser/browser-cookie-import.test.ts index 90514321d54..6ec09edf8aa 100644 --- a/src/main/browser/browser-cookie-import.test.ts +++ b/src/main/browser/browser-cookie-import.test.ts @@ -44,6 +44,21 @@ vi.mock('electron', () => ({ dialog: { showOpenDialog: dialogShowOpenDialogMock }, session: { fromPartition: sessionFromPartitionMock } })) +vi.mock('./browser-cookie-clear-store', () => ({ + openCookieClearStore: (targetSession: { + cookies: { + get: (filter: object) => Promise + remove: (url: string, name: string) => Promise + } + }) => ({ + get: (filter: object) => targetSession.cookies.get(filter), + remove: (url: string, name: string) => targetSession.cookies.remove(url, name), + snapshotClearIdentities: async (items: { cookie: Record; url: string }[]) => + items.map(({ cookie, url }) => ({ url, ...cookie })), + restoreClearIdentities: async () => undefined, + dispose: () => undefined + }) +})) import { buildChromiumCookieInsertParams, diff --git a/src/main/browser/browser-cookie-import.ts b/src/main/browser/browser-cookie-import.ts index 3c3a701f953..e841f3168d8 100644 --- a/src/main/browser/browser-cookie-import.ts +++ b/src/main/browser/browser-cookie-import.ts @@ -80,11 +80,12 @@ import { NON_TRANSPLANTABLE_HOST_KEY_SQL, normalizeCookieDomain, normalizeCookieImportDomain, - removeTransplantableCookies, replaceCookiesForImportedDomains, restoreImportedDomainCookies, type CookieImportMode } from './browser-cookie-import-policy' +import { removeTransplantableCookies, withCookieClearLock } from './browser-cookie-import-clear' +import { openCookieClearStore } from './browser-cookie-clear-store' import { createChromiumCookieSnapshot, type ChromiumCookieSnapshot @@ -1780,7 +1781,20 @@ 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 removeTransplantableCookies(targetSession) + const cookieClearStore = openCookieClearStore(targetSession) + try { + await withCookieClearLock(targetSession, () => + removeTransplantableCookies({ + cookies: cookieClearStore, + clearData: (options) => targetSession.clearData(options), + snapshotClearIdentities: (cookies) => cookieClearStore.snapshotClearIdentities(cookies), + restoreClearIdentities: (identities) => + cookieClearStore.restoreClearIdentities(identities) + }) + ) + } finally { + cookieClearStore.dispose() + } diag( ` cleared existing session cookies before loading ${decryptedCookies.length} imported cookies` )