From f7c27b71ab106696e55dd69bafe84154fd836dc3 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 17 Aug 2026 01:00:16 -0700 Subject: [PATCH] fix(browser): fail closed and serialize cookie imports --- .../browser-cookie-clear-store.test.ts | 17 ++ .../browser/browser-cookie-clear-store.ts | 20 +- .../browser/browser-cookie-import-clear.ts | 15 +- .../browser-cookie-import-concurrency.test.ts | 256 ++++++++++++++++++ ...r-cookie-import-partition-fidelity.test.ts | 44 +++ src/main/browser/browser-cookie-import.ts | 61 +++-- .../browser-cookie-source-partition.test.ts | 18 ++ .../browser-cookie-source-partition.ts | 11 +- 8 files changed, 402 insertions(+), 40 deletions(-) create mode 100644 src/main/browser/browser-cookie-import-concurrency.test.ts diff --git a/src/main/browser/browser-cookie-clear-store.test.ts b/src/main/browser/browser-cookie-clear-store.test.ts index de1f5ce98c1..3ea4aa55b72 100644 --- a/src/main/browser/browser-cookie-clear-store.test.ts +++ b/src/main/browser/browser-cookie-clear-store.test.ts @@ -72,6 +72,23 @@ describe('cookie clear CDP identities', () => { ).toThrow(/Could not snapshot cookie identity/) }) + it('fails closed instead of making an opaque CDP partition restorable as unpartitioned', () => { + expect(() => + cookieClearIdentitiesFromCdp( + [{ cookie: chipsCookie, url: 'https://app.acme-chips.test/' }], + [ + { + name: 'chips-auth', + value: 'keep-me', + domain: 'app.acme-chips.test', + path: '/', + partitionKeyOpaque: true + } + ] + ) + ).toThrow(/Could not snapshot cookie identity/) + }) + it.each([ { hasCrossSiteAncestor: true }, { topLevelSite: 'not-a-site', hasCrossSiteAncestor: true }, diff --git a/src/main/browser/browser-cookie-clear-store.ts b/src/main/browser/browser-cookie-clear-store.ts index e3a6cde3702..db0604caccf 100644 --- a/src/main/browser/browser-cookie-clear-store.ts +++ b/src/main/browser/browser-cookie-clear-store.ts @@ -26,6 +26,7 @@ type CdpCookie = { expires?: number sameSite?: string partitionKey?: CdpCookiePartitionKey | null + partitionKeyOpaque?: boolean } type CookieClearDebugger = { @@ -54,18 +55,17 @@ function cdpSameSite(sameSite: Cookie['sameSite']): 'Strict' | 'Lax' | 'None' | } function electronSameSite(sameSite: string | undefined): Cookie['sameSite'] { - if (sameSite === 'Strict') { - return 'strict' - } - if (sameSite === 'None') { - return 'no_restriction' + if (sameSite === 'Strict' || sameSite === 'None') { + return sameSite === 'Strict' ? 'strict' : 'no_restriction' } return sameSite === 'Lax' ? 'lax' : 'unspecified' } -function partitionKeyFromCdp( - partitionKey: CdpCookiePartitionKey | null | undefined -): CookieClearPartitionKey | undefined { +function partitionKeyFromCdp(cookie: CdpCookie): CookieClearPartitionKey | undefined { + if (cookie.partitionKeyOpaque === true) { + throw new Error('Could not snapshot cookie identity for an atomic clear') + } + const partitionKey = cookie.partitionKey if (partitionKey === undefined) { return undefined } @@ -108,7 +108,7 @@ function indexCdpCookies(cookies: readonly CdpCookie[]): Map>() +const mutationLocks = new WeakMap>() function cookieClearKey(url: string, name: string): string { return JSON.stringify([url, name]) @@ -74,17 +74,22 @@ export function identitiesFromClearCookies( })) } -export async function withCookieClearLock(owner: object, run: () => Promise): Promise { - const previous = clearLocks.get(owner) ?? Promise.resolve() +export async function acquireCookieMutationLock(owner: object): Promise<() => void> { + const previous = mutationLocks.get(owner) ?? Promise.resolve() let release!: () => void const current = new Promise((resolve) => { release = resolve }) - clearLocks.set( + mutationLocks.set( owner, previous.then(() => current) ) await previous + return release +} + +export async function withCookieMutationLock(owner: object, run: () => Promise): Promise { + const release = await acquireCookieMutationLock(owner) try { return await run() } finally { @@ -169,7 +174,7 @@ export async function removeTransplantableCookies( targetSession: CookieClearSession, preserveFamilies: ReadonlySet = new Set() ): Promise { - return withCookieClearLock(targetSession, async () => { + return withCookieMutationLock(targetSession, async () => { const store = targetSession.cookies const initialCookies = await store.get({}) if (initialCookies.length === 0) { diff --git a/src/main/browser/browser-cookie-import-concurrency.test.ts b/src/main/browser/browser-cookie-import-concurrency.test.ts new file mode 100644 index 00000000000..ae7f557e011 --- /dev/null +++ b/src/main/browser/browser-cookie-import-concurrency.test.ts @@ -0,0 +1,256 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { + appGetPathMock, + clearPendingCookieImportMock, + sessionFromPartitionMock, + setPendingCookieImportMock +} = vi.hoisted(() => ({ + appGetPathMock: vi.fn(), + clearPendingCookieImportMock: vi.fn(), + sessionFromPartitionMock: vi.fn(), + setPendingCookieImportMock: vi.fn() +})) + +vi.mock('electron', () => ({ + app: { getPath: appGetPathMock }, + dialog: { showOpenDialog: vi.fn() }, + session: { fromPartition: sessionFromPartitionMock } +})) +vi.mock('node:child_process', () => ({ execFileSync: vi.fn() })) +vi.mock('../codex-accounts/fs-utils', async () => { + const { copyFileSync } = await import('node:fs') + return { + copyFileWithWindowsRetry: (sourcePath: string, destinationPath: string) => { + if (destinationPath.includes('cookie-import-staging')) { + throw new Error('staging intentionally unavailable') + } + copyFileSync(sourcePath, destinationPath) + } + } +}) +vi.mock('./browser-session-registry', () => ({ + browserSessionRegistry: { + setPendingCookieImport: setPendingCookieImportMock, + clearPendingCookieImport: clearPendingCookieImportMock + } +})) +vi.mock('./browser-cookie-clear-store', () => ({ + openCookieClearStore: (targetSession: TestSession) => ({ + 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: (identities: Record[]) => + targetSession.restore(identities), + writeCookieIdentity: (identity: Record) => targetSession.write(identity), + dispose: () => undefined + }) +})) + +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { dirname, join } from 'node:path' +import { importCookiesFromBrowser, importCookiesFromFile } from './browser-cookie-import' +import { createChromiumCookieTestDatabase } from './browser-cookie-import-test-database' + +type JarCookie = { + domain: string + name: string + value: string + path: string + secure: boolean + sameSite: 'unspecified' +} + +type TestSession = { + cookies: { + get: (filter: object) => Promise + remove: (url: string, name: string) => Promise + set: (details: Record) => Promise + flushStore: () => Promise + } + clearData: () => Promise + getStoragePath: () => string + restore: (identities: Record[]) => Promise + write: (identity: Record) => Promise +} + +function deferred(): { promise: Promise; resolve: () => void } { + let resolve!: () => void + const promise = new Promise((done) => { + resolve = done + }) + return { promise, resolve } +} + +function jarCookie(name: string, value: string): JarCookie { + return { + domain: '.example.com', + name, + value, + path: '/', + secure: true, + sameSite: 'unspecified' + } +} + +function replaceCoordinate(jar: JarCookie[], identity: Record): void { + const index = jar.findIndex((cookie) => cookie.name === identity.name) + const next = jarCookie(identity.name as string, identity.value as string) + if (index === -1) { + jar.push(next) + } else { + jar[index] = next + } +} + +function makeSession( + storagePath: string, + jar: JarCookie[], + write: (identity: Record) => Promise +): TestSession { + return { + cookies: { + get: async () => [...jar], + remove: async (_url, name) => { + for (let index = jar.length - 1; index >= 0; index -= 1) { + if (jar[index]?.name === name) { + jar.splice(index, 1) + } + } + }, + set: async () => undefined, + flushStore: async () => undefined + }, + clearData: async () => { + jar.splice(0) + }, + getStoragePath: () => storagePath, + restore: async (identities) => { + for (const identity of identities) { + replaceCoordinate(jar, identity) + } + }, + write + } +} + +function chromeBrowser(cookiesPath: string) { + return { + family: 'chrome' as const, + label: 'Google Chrome', + cookiesPath, + keychainService: 'Chrome Safe Storage', + keychainAccount: 'Chrome', + profiles: [{ name: 'Default', directory: 'Default' }], + selectedProfile: 'Default' + } +} + +describe('cookie import mutation transactions', () => { + let root: string + + beforeEach(() => { + root = mkdtempSync(join(tmpdir(), 'orca-cookie-import-concurrency-')) + appGetPathMock.mockReset().mockReturnValue(join(root, 'userData')) + sessionFromPartitionMock.mockReset() + setPendingCookieImportMock.mockReset() + clearPendingCookieImportMock.mockReset() + }) + + afterEach(() => { + rmSync(root, { recursive: true, force: true }) + }) + + it('prevents a stale validated-import rollback from deleting a successful concurrent import', async () => { + const jar = [jarCookie('original', 'old')] + const failureReached = deferred() + const releaseFailure = deferred() + const secondTargetReady = deferred() + const session = makeSession(join(root, 'target'), jar, async (identity) => { + if (identity.name === 'reject-after-first') { + failureReached.resolve() + await releaseFailure.promise + throw new Error('rejected') + } + replaceCoordinate(jar, identity) + }) + let targetCalls = 0 + sessionFromPartitionMock.mockImplementation(() => { + targetCalls += 1 + if (targetCalls === 2) { + secondTargetReady.resolve() + } + return session + }) + const firstPath = join(root, 'first.json') + const secondPath = join(root, 'second.json') + writeFileSync( + firstPath, + JSON.stringify([ + { domain: '.example.com', name: 'session', value: 'first', secure: true }, + { domain: '.example.com', name: 'reject-after-first', value: 'x', secure: true } + ]) + ) + writeFileSync( + secondPath, + JSON.stringify([{ domain: '.example.com', name: 'session', value: 'second', secure: true }]) + ) + + const firstImport = importCookiesFromFile(firstPath, 'persist:test') + await failureReached.promise + const secondImport = importCookiesFromFile(secondPath, 'persist:test') + await secondTargetReady.promise + await new Promise((resolve) => setImmediate(resolve)) + releaseFailure.resolve() + const [firstResult, secondResult] = await Promise.all([firstImport, secondImport]) + + expect(firstResult.ok).toBe(false) + expect(secondResult.ok).toBe(true) + expect(jar).toEqual([jarCookie('session', 'second')]) + }) + + it('keeps native clear and writes atomic without relying on staged replay', async () => { + const targetPath = join(root, 'target') + const targetCookiesPath = join(targetPath, 'Network', 'Cookies') + mkdirSync(dirname(targetCookiesPath), { recursive: true }) + createChromiumCookieTestDatabase(targetCookiesPath, []).close() + const firstSource = join(root, 'first-source', 'Cookies') + const secondSource = join(root, 'second-source', 'Cookies') + createChromiumCookieTestDatabase(firstSource, [ + { domain: '.example.com', name: 'from-first', value: 'first' } + ]).close() + createChromiumCookieTestDatabase(secondSource, [ + { domain: '.example.com', name: 'from-second', value: 'second' } + ]).close() + const jar = [jarCookie('original', 'old')] + const firstWriteReached = deferred() + const releaseFirstWrite = deferred() + const session = makeSession(targetPath, jar, async (identity) => { + if (identity.name === 'from-first') { + firstWriteReached.resolve() + await releaseFirstWrite.promise + } + replaceCoordinate(jar, identity) + }) + sessionFromPartitionMock.mockReturnValue(session) + const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') + + try { + const firstImport = importCookiesFromBrowser(chromeBrowser(firstSource), 'persist:test') + await firstWriteReached.promise + const secondImport = importCookiesFromBrowser(chromeBrowser(secondSource), 'persist:test') + await new Promise((resolve) => setImmediate(resolve)) + releaseFirstWrite.resolve() + const [firstResult, secondResult] = await Promise.all([firstImport, secondImport]) + + expect(firstResult.ok).toBe(true) + expect(secondResult.ok).toBe(true) + expect(jar).toEqual([jarCookie('from-second', 'second')]) + expect(setPendingCookieImportMock).not.toHaveBeenCalled() + } finally { + platformSpy.mockRestore() + } + }) +}) diff --git a/src/main/browser/browser-cookie-import-partition-fidelity.test.ts b/src/main/browser/browser-cookie-import-partition-fidelity.test.ts index 88eaceb48b3..d7c6401eb42 100644 --- a/src/main/browser/browser-cookie-import-partition-fidelity.test.ts +++ b/src/main/browser/browser-cookie-import-partition-fidelity.test.ts @@ -221,6 +221,50 @@ describe('validated import partition fidelity', () => { expect(remove).not.toHaveBeenCalled() }) + it('preserves a populated family and creates no staged replay for an opaque JSON partition', async () => { + const targetJar = [ + { + name: 'live-session', + value: 'must-survive', + domain: '.preserved.example', + path: '/', + secure: true, + sameSite: 'lax' + } + ] + const remove = vi.fn(async (_url: string, name: string) => { + const index = targetJar.findIndex((cookie) => cookie.name === name) + if (index !== -1) { + targetJar.splice(index, 1) + } + }) + sessionFromPartitionMock.mockReturnValue({ + cookies: { get: vi.fn(async () => targetJar), remove, set: unreachableCookieSet } + }) + const filePath = writeCookieFile([ + { + domain: '.preserved.example', + name: 'opaque-session', + value: 'do-not-downgrade', + secure: true, + partitionKeyOpaque: true + }, + { domain: 'sub.preserved.example', name: 'sibling', value: 'do-not-write', secure: true } + ]) + + const result = await importCookiesFromFile(filePath, 'persist:test') + + expect(result.ok && result.summary).toMatchObject({ + importedCookies: 0, + skippedCookies: 2, + partitionSkippedCookies: 2 + }) + expect(targetJar).toEqual([expect.objectContaining({ name: 'live-session' })]) + expect(remove).not.toHaveBeenCalled() + expect(cookieWriteMock).not.toHaveBeenCalled() + expect(setPendingCookieImportMock).not.toHaveBeenCalled() + }) + it.each([ ['null', null], ['empty string', ''] diff --git a/src/main/browser/browser-cookie-import.ts b/src/main/browser/browser-cookie-import.ts index 39303d207b7..be2e7362742 100644 --- a/src/main/browser/browser-cookie-import.ts +++ b/src/main/browser/browser-cookie-import.ts @@ -85,8 +85,9 @@ import { type ReplacedImportedDomainCookies } from './browser-cookie-import-policy' import { + acquireCookieMutationLock, removeTransplantableCookies, - withCookieClearLock, + withCookieMutationLock, type CookieClearStore, type CookieImportWriteStore } from './browser-cookie-import-clear' @@ -468,6 +469,7 @@ type RawCookieEntry = { sameSite?: unknown expirationDate?: unknown partitionKey?: unknown + partitionKeyOpaque?: unknown } // Why (STA-4300): `partition` is required, not optional, so every source that builds a cookie has to @@ -572,7 +574,7 @@ function validateCookieEntry(raw: RawCookieEntry): ValidatedCookie | null { httpOnly: raw.httpOnly === true || raw.httpOnly === 1, sameSite: normalizeSameSite(raw.sameSite), expirationDate, - partition: readJsonCookiePartition(raw.partitionKey) + partition: readJsonCookiePartition(raw.partitionKey, raw.partitionKeyOpaque) } } @@ -582,6 +584,7 @@ type CookieImportSessionStore = CookieClearStore & CookieImportWriteStore & { di type CookieImportTarget = { partition: string + mutationOwner: object openWriteStore: () => CookieImportSessionStore } @@ -593,6 +596,7 @@ function cookieImportTarget(targetPartition: string): CookieImportTarget { const targetSession = session.fromPartition(targetPartition) return { partition: targetPartition, + mutationOwner: targetSession, openWriteStore: () => openCookieClearStore(targetSession) } } @@ -663,6 +667,9 @@ async function importValidatedCookies( const cookieClearStore = plan.writes.length > 0 ? target.openWriteStore() : null if (cookieClearStore) { + // Why: replace, writes, and rollback are one live-jar transaction; a stale rollback must not + // remove a later import that already reported success. + const releaseMutationLock = await acquireCookieMutationLock(target.mutationOwner) let replaced: ReplacedImportedDomainCookies | null = null try { if (mode === 'replace-imported-domains') { @@ -723,7 +730,11 @@ async function importValidatedCookies( } } } finally { - cookieClearStore.dispose() + try { + cookieClearStore.dispose() + } finally { + releaseMutationLock() + } } } @@ -2013,8 +2024,10 @@ export async function importCookiesFromBrowser( // the same CDP identities — cookies.set() cannot express the partition either one reads. const cookieClearStore = openCookieClearStore(targetSession) try { - await withCookieClearLock(targetSession, () => - removeTransplantableCookies( + // Why: this lock covers only the live jar; staging and cold-start replay keep their existing + // semantics while clear and writes can no longer interleave with another import. + await withCookieMutationLock(targetSession, async () => { + await removeTransplantableCookies( { cookies: cookieClearStore, clearData: (options) => targetSession.clearData(options), @@ -2027,28 +2040,28 @@ export async function importCookiesFromBrowser( // snapshot taken from it, so they are never submitted to any mutation. nativePlan.skippedFamilies ) - ) - diag( - ` cleared existing session cookies before loading ${decryptedCookies.length} imported cookies` - ) + diag( + ` cleared existing session cookies before loading ${decryptedCookies.length} imported cookies` + ) - const writable: SourceCookieToWrite[] = [] - for (const cookie of decryptedCookies) { - const url = deriveUrl(cookie.domain, cookie.secure) - if (!url) { - memoryFailed++ - continue + const writable: SourceCookieToWrite[] = [] + for (const cookie of decryptedCookies) { + const url = deriveUrl(cookie.domain, cookie.secure) + if (!url) { + memoryFailed++ + continue + } + writable.push({ ...cookie, url }) } - writable.push({ ...cookie, url }) - } - // Why: a rejected cookie here falls back to the staged cold-start replay rather than - // unwinding the import, so one failure must not stop the rest from loading. - const phase = await writeImportedCookies(cookieClearStore, writable, { - stopOnFailure: false, - log: diag + // Why: a rejected cookie here falls back to the staged cold-start replay rather than + // unwinding the import, so one failure must not stop the rest from loading. + const phase = await writeImportedCookies(cookieClearStore, writable, { + stopOnFailure: false, + log: diag + }) + memoryLoaded = phase.importedCount + memoryFailed += phase.writeRejected }) - memoryLoaded = phase.importedCount - memoryFailed += phase.writeRejected } finally { cookieClearStore.dispose() } diff --git a/src/main/browser/browser-cookie-source-partition.test.ts b/src/main/browser/browser-cookie-source-partition.test.ts index 4f0733a6f48..2bb9659cbeb 100644 --- a/src/main/browser/browser-cookie-source-partition.test.ts +++ b/src/main/browser/browser-cookie-source-partition.test.ts @@ -113,6 +113,24 @@ describe('readJsonCookiePartition', () => { expect(readJsonCookiePartition(undefined)).toEqual({ status: 'unpartitioned' }) }) + it('refuses an opaque partition key even when its key object is absent or populated', () => { + expect(readJsonCookiePartition(undefined, true)).toEqual({ + status: 'unreadable', + reason: 'partition key was opaque' + }) + expect( + readJsonCookiePartition( + { topLevelSite: 'https://top.example', hasCrossSiteAncestor: true }, + true + ).status + ).toBe('unreadable') + }) + + it('accepts the explicit non-opaque CDP shape and rejects a malformed opaque flag', () => { + expect(readJsonCookiePartition(undefined, false)).toEqual({ status: 'unpartitioned' }) + expect(readJsonCookiePartition(undefined, 'false').status).toBe('unreadable') + }) + it.each([null, ''])('refuses a present but empty partitionKey (%s)', (partitionKey) => { expect(readJsonCookiePartition(partitionKey).status).toBe('unreadable') }) diff --git a/src/main/browser/browser-cookie-source-partition.ts b/src/main/browser/browser-cookie-source-partition.ts index c4fb0c513d4..d1fba33b94e 100644 --- a/src/main/browser/browser-cookie-source-partition.ts +++ b/src/main/browser/browser-cookie-source-partition.ts @@ -127,7 +127,16 @@ export function readFirefoxRowPartition( * exporters that emit only `topLevelSite` (or the legacy CDP string form) carry no ancestor bit, and * guessing it silently misfiles the cookie. */ -export function readJsonCookiePartition(raw: unknown): SourcePartitionRead { +export function readJsonCookiePartition( + raw: unknown, + partitionKeyOpaque: unknown = undefined +): SourcePartitionRead { + if (partitionKeyOpaque === true) { + return { status: 'unreadable', reason: 'partition key was opaque' } + } + if (partitionKeyOpaque !== undefined && typeof partitionKeyOpaque !== 'boolean') { + return { status: 'unreadable', reason: 'partitionKeyOpaque was not a boolean' } + } if (raw === undefined) { return UNPARTITIONED }