diff --git a/src/main/browser/browser-cookie-clear-preserve.test.ts b/src/main/browser/browser-cookie-clear-preserve.test.ts index 9c93e783c79..061289b86dd 100644 --- a/src/main/browser/browser-cookie-clear-preserve.test.ts +++ b/src/main/browser/browser-cookie-clear-preserve.test.ts @@ -5,6 +5,7 @@ import { type CookieClearIdentity, type CookieClearSession } from './browser-cookie-import-clear' +import { importedDomainScope } from './browser-cookie-import-policy' /** * A jar that actually holds cookies. @@ -16,36 +17,33 @@ import { */ function jar(initial: Cookie[]) { let cookies = [...initial] - const clearDataMock = vi.fn(async () => { - // Why: the real bulk clear removes everything it is not told to exclude. - cookies = [] - }) const removeMock = vi.fn(async (url: string, name: string) => { const host = new URL(url).hostname cookies = cookies.filter( (c) => !(c.name === name && (c.domain ?? '').replace(/^\./, '') === host.replace(/^\./, '')) ) }) + const snapshotMock = vi.fn(async (items: readonly { cookie: Cookie; url: string }[]) => + items.map(({ cookie, url }) => ({ + url, + name: cookie.name, + value: cookie.value, + domain: cookie.domain, + path: cookie.path + })) + ) const session: CookieClearSession = { cookies: { get: async () => [...cookies], remove: removeMock }, - clearData: clearDataMock, - snapshotClearIdentities: async (items) => - items.map(({ cookie, url }) => ({ - url, - name: cookie.name, - value: cookie.value, - domain: cookie.domain, - path: cookie.path - })) as CookieClearIdentity[], + snapshotClearIdentities: snapshotMock, restoreClearIdentities: async () => undefined } as unknown as CookieClearSession return { session, - clearDataMock, removeMock, + snapshotMock, names: () => cookies.map((c) => c.name).sort(), get: (name: string) => cookies.find((c) => c.name === name) } @@ -54,18 +52,29 @@ function jar(initial: Cookie[]) { const cookie = (domain: string, name: string, value = `${name}-live`): Cookie => ({ domain, name, value, path: '/', secure: true }) as Cookie +const coordinatesOf = (calls: readonly [string, string][]): string[] => + calls.map(([url, name]) => `${url}|${name}`) + describe('removeTransplantableCookies — preserved families on a POPULATED jar', () => { - it('clears everything when nothing is preserved (unchanged behaviour)', async () => { + it('clears every in-scope domain through per-coordinate removals when nothing is preserved', async () => { const target = jar([ cookie('.mixed.example', 'live-session'), cookie('.other.example', 'stale') ]) - await removeTransplantableCookies(target.session) + await removeTransplantableCookies( + target.session, + new Set(), + importedDomainScope(['mixed.example', 'other.example']) + ) expect(target.names()).toEqual([]) - // Why: the ordinary import must keep using the single bulk call. - expect(target.clearDataMock).toHaveBeenCalledOnce() + // Why (STA-4797): the bulk clearData shortcut is gone — an import with nothing preserved still + // clears one coordinate at a time, drawn from the frozen plan the snapshot is taken from. + expect(coordinatesOf(target.removeMock.mock.calls)).toEqual([ + 'https://mixed.example/|live-session', + 'https://other.example/|stale' + ]) }) it('leaves a preserved family untouched while clearing everything else', async () => { @@ -75,7 +84,11 @@ describe('removeTransplantableCookies — preserved families on a POPULATED jar' cookie('.other.example', 'stale') ]) - await removeTransplantableCookies(target.session, new Set(['mixed.example'])) + await removeTransplantableCookies( + target.session, + new Set(['mixed.example']), + importedDomainScope(['mixed.example', 'sub.mixed.example', 'other.example']) + ) // The whole family survives — apex AND subdomain — and it survives byte-identically. expect(target.names()).toEqual(['apex-session', 'sub-session']) @@ -83,20 +96,29 @@ describe('removeTransplantableCookies — preserved families on a POPULATED jar' expect(target.get('sub-session')?.value).toBe('sub-session-live') }) - it('never calls bulk clearData when a family is preserved', async () => { - // Why (§4.3a): clearData removes everything outside excludeOrigins and its own contract admits - // a rejection may already have emptied part of the jar. A preserved family is deliberately - // absent from the snapshot, so a partial delete then a rejection would destroy it with nothing - // able to restore it. The only safe answer is not to call it. + it('submits no coordinate to a removal the snapshot cannot restore', async () => { + // Why (§4.3a): the removed set must equal the restorable set. The bulk clearData path could not + // honour that — it deletes by exclusion, and its own contract admits a rejection may already + // have emptied part of the jar, so a preserved family absent from the snapshot would be + // destroyed with nothing able to put it back. Removal is per-coordinate for that reason, and + // every coordinate it submits comes from the same frozen plan the snapshot was taken from. const target = jar([ cookie('.mixed.example', 'live-session'), cookie('.other.example', 'stale') ]) - await removeTransplantableCookies(target.session, new Set(['mixed.example'])) + await removeTransplantableCookies( + target.session, + new Set(['mixed.example']), + importedDomainScope(['mixed.example', 'other.example']) + ) - expect(target.clearDataMock).not.toHaveBeenCalled() - expect(target.removeMock).toHaveBeenCalled() + const removed = coordinatesOf(target.removeMock.mock.calls) + const snapshotted = target.snapshotMock.mock.calls.flatMap(([items]) => + items.map(({ cookie: c, url }) => `${url}|${c.name}`) + ) + expect(removed).toEqual(['https://other.example/|stale']) + expect(snapshotted).toEqual(removed) }) it('never submits a preserved coordinate to a removal', async () => { @@ -106,7 +128,11 @@ describe('removeTransplantableCookies — preserved families on a POPULATED jar' cookie('.other.example', 'stale') ]) - await removeTransplantableCookies(target.session, new Set(['mixed.example'])) + await removeTransplantableCookies( + target.session, + new Set(['mixed.example']), + importedDomainScope(['mixed.example', 'sub.mixed.example', 'other.example']) + ) const removedNames = target.removeMock.mock.calls.map((call) => call[1]) expect(removedNames).toEqual(['stale']) @@ -135,7 +161,11 @@ describe('removeTransplantableCookies — preserved families on a POPULATED jar' } } as unknown as CookieClearSession - await removeTransplantableCookies(session, new Set(['mixed.example'])) + await removeTransplantableCookies( + session, + new Set(['mixed.example']), + importedDomainScope(['mixed.example', 'other.example']) + ) expect(snapshotted).toEqual(['stale']) }) @@ -145,7 +175,11 @@ describe('removeTransplantableCookies — preserved families on a POPULATED jar' // the live 127.0.0.1 session would not match the preserve set and would be erased. const target = jar([cookie('127.0.0.1', 'loopback-session'), cookie('.other.example', 'stale')]) - await removeTransplantableCookies(target.session, new Set(['127.0.0.1'])) + await removeTransplantableCookies( + target.session, + new Set(['127.0.0.1']), + importedDomainScope(['127.0.0.1', 'other.example']) + ) expect(target.names()).toEqual(['loopback-session']) }) @@ -153,7 +187,11 @@ describe('removeTransplantableCookies — preserved families on a POPULATED jar' it('preserves a single-label host family', async () => { const target = jar([cookie('localhost', 'dev-session'), cookie('.other.example', 'stale')]) - await removeTransplantableCookies(target.session, new Set(['localhost'])) + await removeTransplantableCookies( + target.session, + new Set(['localhost']), + importedDomainScope(['localhost', 'other.example']) + ) expect(target.names()).toEqual(['dev-session']) }) @@ -161,7 +199,11 @@ describe('removeTransplantableCookies — preserved families on a POPULATED jar' it('does not preserve a different family that merely shares a suffix', async () => { const target = jar([cookie('.kept.example', 'kept'), cookie('.other.example', 'stale')]) - await removeTransplantableCookies(target.session, new Set(['kept.example'])) + await removeTransplantableCookies( + target.session, + new Set(['kept.example']), + importedDomainScope(['kept.example', 'other.example']) + ) expect(target.names()).toEqual(['kept']) }) @@ -183,19 +225,20 @@ describe('removeTransplantableCookies — preserved families on a POPULATED jar' cookie('.preserved.example', 'live-session'), ...Array.from({ length: 12 }, (_, index) => cookie('.other.example', `stale-${index}`)) ] - const clearData = vi.fn() const session = { cookies: { get: async () => cookies, remove }, - clearData, snapshotClearIdentities: async (items: { cookie: Cookie; url: string }[]) => items.map(({ cookie: entry, url }) => ({ url, ...entry })), restoreClearIdentities: async () => undefined } as unknown as CookieClearSession - const clearing = removeTransplantableCookies(session, new Set(['preserved.example'])) + const clearing = removeTransplantableCookies( + session, + new Set(['preserved.example']), + importedDomainScope(['preserved.example', 'other.example']) + ) await vi.waitFor(() => expect(remove).toHaveBeenCalledTimes(8)) expect(maxActive).toBe(8) - expect(clearData).not.toHaveBeenCalled() releaseRemovals?.() await clearing diff --git a/src/main/browser/browser-cookie-import-clear-atomicity.test.ts b/src/main/browser/browser-cookie-import-clear-atomicity.test.ts index cc3f67e5117..d7991b3be5a 100644 --- a/src/main/browser/browser-cookie-import-clear-atomicity.test.ts +++ b/src/main/browser/browser-cookie-import-clear-atomicity.test.ts @@ -6,6 +6,7 @@ import { type CookieClearIdentity, type CookieClearSession } from './browser-cookie-import-clear' +import { importedDomainScope } from './browser-cookie-import-policy' function cookie(domain: string, name: string, path = '/', secure = true): Cookie { return { @@ -49,15 +50,15 @@ function createJarSession( jar = jar.filter((entry) => entry.name !== name) } }, - clearData: async () => { - // Why: a cookie the user creates mid-clear lands before the rejection surfaces. + // Why: the snapshot runs after the plan is frozen and before the first removal, which is + // exactly where a cookie the user creates mid-clear lands. Injecting the arrival here is what + // makes a re-read of the jar visible: a widened plan would sweep it up. + snapshotClearIdentities: async (items) => { if (options.arrivalDuringClear) { jar.push(options.arrivalDuringClear) } - throw new Error('storage busy') + return options.snapshot ? await options.snapshot(items) : identitiesFromClearCookies(items) }, - snapshotClearIdentities: - options.snapshot ?? (async (items) => identitiesFromClearCookies(items)), restoreClearIdentities: async (identities) => { restoredNames.push(...identities.map((identity) => identity.name)) if (options.restoreError) { @@ -85,9 +86,15 @@ describe('STA-4090 failed full cookie clear', () => { cookie('.other.test', 'stale', '/two') ]) - await expect(removeTransplantableCookies(session)).rejects.toThrow( - /existing cookies were restored/ - ) + await expect( + removeTransplantableCookies( + session, + new Set(), + // google.com is in the import scope on purpose: the non-transplantable exemption, not the + // scope, has to be what keeps SID out of the removal plan. + importedDomainScope(['google.com', 'example.com', 'other.test']) + ) + ).rejects.toThrow(/existing cookies were restored/) expect(session.removedNames()).toEqual(['removed-first']) expect(session.names()).toEqual(['SID', 'removed-first', 'stale']) @@ -102,9 +109,13 @@ describe('STA-4090 failed full cookie clear', () => { } ) - await expect(removeTransplantableCookies(session)).rejects.toThrow( - /the session was left unchanged/ - ) + await expect( + removeTransplantableCookies( + session, + new Set(), + importedDomainScope(['example.com', 'other.test']) + ) + ).rejects.toThrow(/the session was left unchanged/) expect(session.removedNames()).toEqual([]) expect(session.names()).toEqual(['removed-first', 'stale']) }) @@ -115,9 +126,13 @@ describe('STA-4090 failed full cookie clear', () => { { restoreError: new Error('restore rejected') } ) - await expect(removeTransplantableCookies(session)).rejects.toThrow( - /the session was left partially cleared/ - ) + await expect( + removeTransplantableCookies( + session, + new Set(), + importedDomainScope(['example.com', 'other.test']) + ) + ).rejects.toThrow(/the session was left partially cleared/) expect(session.names()).toEqual(['stale']) }) @@ -132,9 +147,6 @@ describe('STA-4090 failed full cookie clear', () => { } } }, - clearData: async () => { - throw new Error('storage busy') - }, snapshotClearIdentities: async (items) => identitiesFromClearCookies(items).map((identity) => identity.name === 'removed-first' @@ -149,9 +161,13 @@ describe('STA-4090 failed full cookie clear', () => { } } - await expect(removeTransplantableCookies(session)).rejects.toThrow( - /existing cookies were restored/ - ) + await expect( + removeTransplantableCookies( + session, + new Set(), + importedDomainScope(['example.com', 'other.test']) + ) + ).rejects.toThrow(/existing cookies were restored/) expect(identities).toEqual( expect.arrayContaining([ expect.objectContaining({ @@ -171,9 +187,15 @@ describe('STA-4090 failed full cookie clear', () => { { arrivalDuringClear: cookie('.arrived.test', 'fresh-login') } ) - await expect(removeTransplantableCookies(session)).rejects.toThrow( - /existing cookies were restored/ - ) + await expect( + removeTransplantableCookies( + session, + new Set(), + // arrived.test is inside the import scope, so only the frozen plan can be what spares the + // login — being out of scope must not stand in for it. + importedDomainScope(['example.com', 'other.test', 'arrived.test']) + ) + ).rejects.toThrow(/existing cookies were restored/) expect(session.removedNames()).toEqual(['removed-first']) expect(session.names()).toEqual(['fresh-login', 'removed-first', 'stale']) @@ -197,13 +219,14 @@ describe('STA-4090 failed full cookie clear', () => { jar = jar.filter((entry) => entry.name !== name) } }, - clearData: async () => { - // The site re-sets the same cookie while the bulk clear is failing. + snapshotClearIdentities: async (items) => { + const identities = identitiesFromClearCookies(items) + // The site re-sets the same cookie after the plan is frozen but before its coordinate is + // emptied. jar = jar.filter((entry) => entry.name !== 'session') jar.push(valueCookie('.example.com', 'session', 'mid-clear')) - throw new Error('storage busy') + return identities }, - snapshotClearIdentities: async (items) => identitiesFromClearCookies(items), restoreClearIdentities: async (identities) => { restored.push(...identities) for (const identity of identities) { @@ -215,9 +238,13 @@ describe('STA-4090 failed full cookie clear', () => { } } - await expect(removeTransplantableCookies(session)).rejects.toThrow( - /existing cookies were restored/ - ) + await expect( + removeTransplantableCookies( + session, + new Set(), + importedDomainScope(['example.com', 'other.test']) + ) + ).rejects.toThrow(/existing cookies were restored/) expect(jar.map((entry) => [entry.name, entry.value])).toEqual([ ['stale', 'stale-value'], @@ -239,14 +266,15 @@ describe('STA-4090 failed full cookie clear', () => { inClear -= 1 } }, - clearData: async () => { - throw new Error('storage busy') - }, snapshotClearIdentities: async (items) => identitiesFromClearCookies(items), restoreClearIdentities: async () => undefined } - await Promise.all([removeTransplantableCookies(session), removeTransplantableCookies(session)]) + const scope = importedDomainScope(['example.com']) + await Promise.all([ + removeTransplantableCookies(session, new Set(), scope), + removeTransplantableCookies(session, new Set(), scope) + ]) 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 index 9f364d097d0..af79c4249ed 100644 --- a/src/main/browser/browser-cookie-import-clear.ts +++ b/src/main/browser/browser-cookie-import-clear.ts @@ -1,9 +1,10 @@ -import type { Cookie, Cookies, Session } from 'electron' +import type { Cookie, Cookies } from 'electron' import { mapSettledWithConcurrency } from '../../shared/map-with-concurrency' +import type { ImportedDomainScope } from './browser-cookie-import-policy' import { cookieRemovalUrl, + domainIsInImportedScope, isNonTransplantableCookieDomain, - NON_TRANSPLANTABLE_CLEAR_EXCLUDED_ORIGINS, normalizeCookieDomain, registrableFamily } from './browser-cookie-import-policy' @@ -44,9 +45,11 @@ export type CookieImportWriteStore = Pick & { } // Why (STA-4061): 'set' stays out so the lossy partition-dropping reconstruction cannot return. +// Why (STA-4797): 'clearData' stays out for the same structural reason. It can only express +// "everything except these origins", never "only the domains this import replaces", so any route +// back to it is a route back to wiping the whole partition. export type CookieClearSession = { cookies: Pick - clearData: Session['clearData'] snapshotClearIdentities: CookieClearStore['snapshotClearIdentities'] restoreClearIdentities: CookieClearStore['restoreClearIdentities'] } @@ -107,13 +110,25 @@ export async function withCookieMutationLock(owner: object, run: () => Promis function removableCookieEntries( cookies: readonly Cookie[], - preserveFamilies: ReadonlySet + preserveFamilies: ReadonlySet, + importScope: ImportedDomainScope ): { cookie: Cookie; url: string }[] { const removable: { cookie: Cookie; url: string }[] = [] for (const cookie of cookies) { if (isNonTransplantableCookieDomain(cookie.domain ?? '')) { continue } + // Why (STA-4797): a cookie for a site this import never mentions is not stale — it is the + // user's live session, and signing them out of it buys the import nothing. The scope test + // comes before the removal-URL derivation below so an unaddressable cookie parked in some + // unrelated corner of the jar cannot fail an import that was never going to touch it. + const scopedDomain = cookie.domain ? normalizeCookieDomain(cookie.domain) : null + if ( + scopedDomain === null || + !domainIsInImportedScope(importScope, scopedDomain, cookie.hostOnly === true) + ) { + continue + } // Why (STA-4300 I2): a family whose partition could not be read faithfully is neither written // nor removed. Filtering HERE keeps it out of the removal plan and — because the CDP snapshot // is taken from this same list — out of the restore set too, so it is never submitted to any @@ -124,12 +139,7 @@ function removableCookieEntries( continue } } - const domain = cookie.domain ? normalizeCookieDomain(cookie.domain) : null - const url = domain ? cookieRemovalUrl(cookie, domain) : null - if (!url) { - throw new Error('Could not clear existing cookies; the session was left unchanged') - } - removable.push({ cookie, url }) + removable.push({ cookie, url: cookieRemovalUrl(cookie, scopedDomain) }) } return removable } @@ -179,7 +189,7 @@ async function restoreClearedCookies( } /** - * Clears the transplantable cookies from a jar. + * Clears the cookies an import is about to replace, for the domains it is importing. * * Why (STA-4601): this takes the mutation lock on the object it is PASSED, which serialises direct * callers that hand it a real Session — pinned by "serializes concurrent clears on the same @@ -188,19 +198,27 @@ async function restoreClearedCookies( * them. That is deliberate and safe: the importer holds the real per-partition lock, keyed on the * Electron Session, across its whole clear-and-write transaction — a scope this function cannot * see. Do not remove the importer's outer lock on the assumption that this one covers it. + * + * Why (STA-4797): importScope is required, not defaulted. A default would be the whole jar again, + * and the whole-jar clear is the defect — an import of three sites signed the user out of every + * other site in the partition. Every caller has to name what it is about to replace. */ export async function removeTransplantableCookies( targetSession: CookieClearSession, - preserveFamilies: ReadonlySet = new Set() + preserveFamilies: ReadonlySet, + importScope: ImportedDomainScope ): Promise { return withCookieMutationLock(targetSession, async () => { const store = targetSession.cookies + if (importScope.exact.size === 0) { + return + } const initialCookies = await store.get({}) if (initialCookies.length === 0) { return } - const initialRemovable = removableCookieEntries(initialCookies, preserveFamilies) + const initialRemovable = removableCookieEntries(initialCookies, preserveFamilies, importScope) if (initialRemovable.length === 0) { return } @@ -213,28 +231,13 @@ export async function removeTransplantableCookies( // no-op, so the stale plan costs nothing; only its narrowness matters. const removalGroups = [...groupRemovableCookies(initialRemovable).values()] - // Why (STA-4300 §4.3a): bulk clearData removes everything outside excludeOrigins, and its own - // contract admits a rejection may already have emptied part of the jar. Handing it a - // dynamically derived preserve list cannot be made safe: a partial delete followed by a - // rejection would destroy a preserved family with no identity to restore it from, because the - // preserved coordinates are deliberately absent from the snapshot. So when anything is - // preserved we do not use the bulk path at all — the frozen per-coordinate plan, which already - // excludes those families, becomes the primary path. Nothing is preserved on the ordinary - // import, so that path keeps today's single clearData call unchanged. - if (preserveFamilies.size === 0) { - 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. - } - } - + // Why (STA-4797): the bulk clearData shortcut is gone. It clears by exclusion, so the only + // scope it could express was "everything except google.com" — the defect itself. Narrowing it + // to an include list would not help either: clearData matches at the registrable-domain + // boundary, so it would still take host-only siblings this import does not replace, and a + // partial delete followed by a rejection would destroy them with no identity to restore from. + // The frozen per-coordinate plan is now the only path, and it is a small one — it covers the + // imported domains rather than the jar. const results = await mapSettledWithConcurrency( removalGroups, COOKIE_CLEAR_CONCURRENCY, diff --git a/src/main/browser/browser-cookie-import-concurrency.test.ts b/src/main/browser/browser-cookie-import-concurrency.test.ts index 0da6242cf8c..776ded2f7e7 100644 --- a/src/main/browser/browser-cookie-import-concurrency.test.ts +++ b/src/main/browser/browser-cookie-import-concurrency.test.ts @@ -277,7 +277,9 @@ describe('two concurrent imports into one partition', () => { describe('two concurrent NATIVE imports into one partition', () => { let tmpDir: string let events: string[] + let cookiesRemoveMock: ReturnType let cookiesSetMock: ReturnType + let clearDataMock: ReturnType let releaseFirstWrite: () => void let platformSpy: { mockRestore: () => void } let stagingCopyCount: number @@ -309,7 +311,11 @@ describe('two concurrent NATIVE imports into one partition', () => { copyFileSync(source, destination) }) // Why: an EMPTY jar makes removeTransplantableCookies return before it clears, so the clear - // would never run and a clearData assertion would pass vacuously. Seed it. + // would never run and the clear assertions would pass vacuously. Seed it. + // Why (STA-4797): the seed has to sit on the domains these two imports actually bring over. + // The clear is scoped to the imported domains now, so a jar holding only an unrelated site is + // the empty-jar case wearing a disguise — nothing would be removed and the detector, which + // reads "did B clear yet?" off the removals, would measure nothing. snapshotClearIdentitiesMock .mockReset() .mockImplementation(async (items: { cookie: Record; url: string }[]) => @@ -328,22 +334,25 @@ describe('two concurrent NATIVE imports into one partition', () => { }) // Why (STA-4300): same guard as the file path — the native writes go through CDP identities. cookiesSetMock = vi.fn() + cookiesRemoveMock = vi.fn(async (_url: string, name: string) => { + events.push(`remove:${name}`) + }) + clearDataMock = vi.fn(async () => { + events.push('clearData') + }) const stableSession = { cookies: { - get: vi - .fn() - .mockResolvedValue([ - { domain: '.stale.example', name: 'stale', value: 'v', path: '/', secure: true } - ]), - remove: vi.fn().mockResolvedValue(undefined), + get: vi.fn().mockResolvedValue([ + { domain: '.a.example', name: 'old-a', value: 'v', path: '/', secure: true }, + { domain: '.b.example', name: 'old-b', value: 'v', path: '/', secure: true } + ]), + remove: cookiesRemoveMock, set: cookiesSetMock, flushStore: vi.fn(async () => { events.push('flushStore') }) }, - clearData: vi.fn(async () => { - events.push('clearData') - }), + clearData: clearDataMock, // Why (STA-4300): the importer asks the Session where its storage lives instead of rebuilding // the path from the partition string, so the stub has to answer. getStoragePath: () => join(tmpDir, 'userData', 'Partitions', 'native-conc'), @@ -386,10 +395,13 @@ describe('two concurrent NATIVE imports into one partition', () => { // The first import is parked inside its write. The second must not stage a stale image while // that transaction is active — this assertion fails if the staging copy sits outside the lock. expect(events).not.toContain('copy:second') - // Nor may it flush, clear, or write the live jar while that transaction is active. + // The first import clears only its imported domain, while the second may not flush, remove, + // or write the live jar until that transaction finishes. + expect(events).toContain('remove:old-a') + expect(events).not.toContain('remove:old-b') expect(events.filter((e) => e === 'flushStore')).toHaveLength(1) - expect(events.filter((e) => e === 'clearData')).toHaveLength(1) expect(events).not.toContain('set:second') + expect(clearDataMock).not.toHaveBeenCalled() releaseFirstWrite() const [resultA, resultB] = await Promise.all([first, second]) @@ -401,7 +413,8 @@ describe('two concurrent NATIVE imports into one partition', () => { // once per import, not just that the end state looks right. expect(writeCookieIdentityMock).toHaveBeenCalledTimes(2) expect(cookiesSetMock).not.toHaveBeenCalled() - expect(events.indexOf('set:first')).toBeLessThan(events.lastIndexOf('clearData')) + expect(cookiesRemoveMock.mock.calls.map(([, name]) => name)).toEqual(['old-a', 'old-b']) + expect(events.indexOf('set:first')).toBeLessThan(events.indexOf('remove:old-b')) expect(events.indexOf('copy:second')).toBeGreaterThan(events.indexOf('set:first')) }) }) 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 48af8c4329b..1cc13450cff 100644 --- a/src/main/browser/browser-cookie-import-google-exclusion.test.ts +++ b/src/main/browser/browser-cookie-import-google-exclusion.test.ts @@ -243,7 +243,7 @@ describe('native Chromium import excludes the Google cookie family', () => { createChromiumCookieTestDatabase(targetCookiesPath, rows).close() } - it('bulk clears while excluding live Google cookies before importing', async () => { + it('clears the imported domain coordinate by coordinate and never a Google one', async () => { const sourceCookiesPath = seedSource([ { domain: '.google.com', name: 'SID', value: 'transplanted-sid' }, { domain: '.example.com', name: 'session', value: 'new' } @@ -263,10 +263,10 @@ describe('native Chromium import excludes the Google cookie family', () => { googleCookiesSkipped: 1, domains: ['example.com'] }) - expect(clearDataMock.mock.calls).toEqual([ - [{ dataTypes: ['cookies'], excludeOrigins: ['https://google.com'] }] - ]) - expect(cookiesRemoveMock).not.toHaveBeenCalled() + // Why (STA-4797): the bulk clear is gone, so the live Google session survives by never being + // named as a removal coordinate rather than by riding an excludeOrigins exemption. + expect(clearDataMock).not.toHaveBeenCalled() + expect(cookiesRemoveMock.mock.calls).toEqual([['https://example.com/', 'stale']]) expect(cookiesSetMock.mock.calls.map(([details]) => details.domain)).toEqual(['.example.com']) }) @@ -352,10 +352,13 @@ describe('native Chromium import excludes the Google cookie family', () => { { domain: '.example.com', name: 'session', value: 'new' } ]) seedTarget([{ domain: '.example.com', name: 'stale', value: 'stale' }]) + // Why (STA-4797): the rejecting cookie has to sit on a domain this import actually replaces. + // Parked on an unrelated site it is out of the import scope, never enters the removal plan, + // and the rejection the case exists to exercise never happens. let jar = [ existingCookie('.google.com', 'SID'), existingCookie('.example.com', 'removed-first'), - existingCookie('.other.test', 'stale') + existingCookie('.example.com', 'stale') ] cookiesGetMock.mockImplementation(async () => [...jar]) cookiesRemoveMock.mockImplementation(async (_url: string, name: string) => { @@ -369,13 +372,12 @@ describe('native Chromium import excludes the Google cookie family', () => { jar.push(existingCookie(details.domain ?? '.example.com', details.name)) } }) - 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(clearDataMock).toHaveBeenCalledOnce() + expect(clearDataMock).not.toHaveBeenCalled() expect(cookiesRemoveMock.mock.calls.map(([, name]) => name)).toEqual(['removed-first', 'stale']) expect(cookiesSetMock.mock.calls.map(([details]) => details.name)).toEqual( expect.arrayContaining(['removed-first']) 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 9237d0dfbdb..b573cfbd116 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 @@ -20,7 +20,6 @@ type CdpCookie = { name: string; value: string; partitionKey?: Record | undefined clearError: string | null - bulkClearCalls: number remainingChips: CdpCookie[] remainingPlain: CdpCookie[] remainingExcluded: CdpCookie[] @@ -35,7 +34,7 @@ function buildFixtureMain(bundlePath: string, resultPath: string): string { return ` const { app, BrowserWindow, session } = require('electron') const { writeFileSync } = require('node:fs') -const { openCookieClearStore, removeTransplantableCookies } = require(${JSON.stringify(bundlePath)}) +const { openCookieClearStore, removeTransplantableCookies, importedDomainScope } = require(${JSON.stringify(bundlePath)}) const resultPath = ${JSON.stringify(resultPath)} let currentStep = 'starting' const mark = (step) => { @@ -81,9 +80,8 @@ async function run() { await targetSession.cookies.set({ url: 'https://accounts.google.com/', name: 'SID', value: 'live', secure: true }) mark('removable cookies set') - // 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 + // Why (STA-4797): the per-coordinate plan is the only removal path, so a rejection partway + // through it is exactly the failure this fixture needs — no bulk clear to force off first. const cookieClearStore = openCookieClearStore(targetSession) const clearSession = { cookies: { @@ -93,17 +91,21 @@ async function run() { return targetSession.cookies.remove(url, name) } }, - clearData: async () => { - bulkClearCalls++ - throw new Error('forced bulk clear failure') - }, snapshotClearIdentities: (cookies) => cookieClearStore.snapshotClearIdentities(cookies), restoreClearIdentities: (identities) => cookieClearStore.restoreClearIdentities(identities) } + // Why: the import these removals stand in for writes all three fixture domains, so all three + // are in scope. accounts.google.com stays out by policy, not by scope. + const importScope = importedDomainScope([ + 'app.acme-chips.test', + 'plain.example', + 'victim.example', + 'accounts.google.com' + ]) let clearError = null try { - await removeTransplantableCookies(clearSession) + await removeTransplantableCookies(clearSession, new Set(), importScope) } catch (error) { clearError = String(error?.message || error) } finally { @@ -119,7 +121,6 @@ async function run() { writeFileSync(resultPath, JSON.stringify({ beforePartitionKey: beforeChips.partitionKey, clearError, - bulkClearCalls, remainingChips: project('chips-auth'), remainingPlain: project('plain'), remainingExcluded: project('SID') @@ -147,7 +148,8 @@ async function runFixture(): Promise { bundleEntryPath, [ `export { openCookieClearStore } from ${JSON.stringify(join(process.cwd(), 'src/main/browser/browser-cookie-clear-store.ts'))}`, - `export { removeTransplantableCookies } from ${JSON.stringify(join(process.cwd(), 'src/main/browser/browser-cookie-import-clear.ts'))}` + `export { removeTransplantableCookies } from ${JSON.stringify(join(process.cwd(), 'src/main/browser/browser-cookie-import-clear.ts'))}`, + `export { importedDomainScope } from ${JSON.stringify(join(process.cwd(), 'src/main/browser/browser-cookie-import-policy.ts'))}` ].join('\n') ) await buildVite({ @@ -191,7 +193,6 @@ describe('non-Google partitioned cookie under a failed Electron cookie clear', ( const result = await runFixture() expect(result.beforePartitionKey).toEqual(EXPECTED_PARTITION_KEY) - expect(result.bulkClearCalls).toBe(1) expect(result.remainingExcluded.map(({ name }) => name)).toEqual(['SID']) expect(result.clearError).toContain('Could not clear existing cookies') expect(result.remainingChips).toEqual([ 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 e844341e12f..c992421871e 100644 --- a/src/main/browser/browser-cookie-import-partition.electron.test.ts +++ b/src/main/browser/browser-cookie-import-partition.electron.test.ts @@ -68,10 +68,20 @@ async function run() { partitionKey: { topLevelSite: 'https://example.com', hasCrossSiteAncestor: true } }) mark('partitioned cookie set') + // Why (STA-4797): a stale cookie on a domain the import DOES bring over. Narrowing the clear + // must not stop it removing this one, or imported sites inherit a session the site rejects. + await targetSession.cookies.set({ + url: 'https://example.com/', + name: 'superseded', + value: 'remove-me', + secure: true + }) + // Why (STA-4797): a live session for a site the import never mentions. It must still be here + // afterwards — a real Electron jar proving the import no longer signs the user out of it. await targetSession.cookies.set({ url: 'https://stale.example/', name: 'stale', - value: 'remove-me', + value: 'keep-me', secure: true }) // Why: the excluded origin is HTTPS, so prove registrable-domain matching also keeps HTTP. @@ -207,7 +217,8 @@ describe('native Chromium excluded partition cookie under Electron', () => { 'domain-google', 'http-google', 'imported', - 'partitioned-google' + 'partitioned-google', + '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 25204fcd1c7..c1ea2ef2ccd 100644 --- a/src/main/browser/browser-cookie-import-policy.test.ts +++ b/src/main/browser/browser-cookie-import-policy.test.ts @@ -1,5 +1,4 @@ import { describe, expect, it, vi, type Mock } from 'vitest' -import { DatabaseSync } from 'node:sqlite' import type { Cookie } from 'electron' import { identitiesFromClearCookies, @@ -7,9 +6,9 @@ import { type CookieClearIdentity } from './browser-cookie-import-clear' import { + importedDomainScope, isGoogleSourceBoundCookie, isNonTransplantableCookieDomain, - NON_TRANSPLANTABLE_HOST_KEY_SQL, normalizeCookieDomain, replaceCookiesForImportedDomains } from './browser-cookie-import-policy' @@ -233,47 +232,13 @@ describe('isNonTransplantableCookieDomain', () => { }) }) -describe('NON_TRANSPLANTABLE_HOST_KEY_SQL', () => { - it('selects the google.com family and nothing that merely looks like it', () => { - const db = new DatabaseSync(':memory:') - db.exec('CREATE TABLE cookies (host_key TEXT)') - for (const hostKey of [ - 'google.com', - '.google.com', - 'accounts.google.com', - 'withgoogle.com', - 'google.com.evil.example', - '.youtube.com', - '.linear.app' - ]) { - db.prepare('INSERT INTO cookies (host_key) VALUES (?)').run(hostKey) - } - - const matched = db - .prepare( - `SELECT host_key FROM cookies WHERE ${NON_TRANSPLANTABLE_HOST_KEY_SQL} ORDER BY host_key` - ) - .all() as { host_key: string }[] - db.close() - - expect(matched.map((row) => row.host_key)).toEqual([ - '.google.com', - 'accounts.google.com', - 'google.com' - ]) - }) -}) - type CookieClearMocks = { get: Mock remove: Mock set: Mock - clearData: Mock } describe('removeTransplantableCookies', () => { - const rejectingBulkClear = () => vi.fn().mockRejectedValue(new Error('storage busy')) - function clearSession(cookies: Cookie[], overrides: Partial = {}) { const store = { get: vi.fn().mockResolvedValue(cookies), @@ -281,7 +246,6 @@ describe('removeTransplantableCookies', () => { set: vi.fn().mockResolvedValue(undefined), ...overrides } - const clearData = overrides.clearData ?? vi.fn().mockResolvedValue(undefined) const restoreClearIdentities = vi.fn().mockResolvedValue(undefined) const snapshotClearIdentities = vi.fn( async (items: Parameters[0]) => @@ -290,109 +254,105 @@ describe('removeTransplantableCookies', () => { return { session: { cookies: store, - clearData, snapshotClearIdentities, restoreClearIdentities }, get: store.get, remove: store.remove, set: store.set, - clearData, snapshotClearIdentities, restoreClearIdentities } } - // Why (STA-4065): the bulk call is the ordinary path even when the jar holds cookies to keep — - // excludeOrigins preserves the whole google.com family, verified against real Electron. - it('clears a jar holding Google cookies in one call that excludes them', async () => { - const { session, remove, set, clearData } = clearSession([ + // Why (STA-4065/STA-4797): google.com is in the import scope in these fixtures on purpose — the + // non-transplantable exemption, not the scope, has to be what keeps the family out of the plan. + it('removes the imported coordinates one by one and leaves the Google family alone', async () => { + const { session, remove, set } = clearSession([ cookie('.google.com', 'SID'), cookie('accounts.google.com', 'ACCOUNT'), cookie('.example.com', 'session'), cookie('other.test', 'tracker', '/scoped') ]) - await removeTransplantableCookies(session) + await removeTransplantableCookies( + session, + new Set(), + importedDomainScope(['google.com', 'example.com', 'other.test']) + ) - expect(clearData.mock.calls).toEqual([ - [{ dataTypes: ['cookies'], excludeOrigins: ['https://google.com'] }] + expect(remove.mock.calls).toEqual([ + ['https://example.com/', 'session'], + ['https://other.test/scoped', 'tracker'] ]) - expect(remove).not.toHaveBeenCalled() expect(set).not.toHaveBeenCalled() }) - it('bulk clears in one call when the jar holds nothing to preserve', async () => { - const { session, remove, set, clearData } = clearSession([ + it('removes every in-scope coordinate when the jar holds nothing to preserve', async () => { + const { session, remove, set } = clearSession([ cookie('.example.com', 'session'), cookie('other.test', 'tracker', '/scoped'), cookie('notgoogle.com', 'lookalike') ]) - await removeTransplantableCookies(session) + await removeTransplantableCookies( + session, + new Set(), + importedDomainScope(['example.com', 'other.test', 'notgoogle.com']) + ) - expect(clearData.mock.calls).toEqual([ - [{ dataTypes: ['cookies'], excludeOrigins: ['https://google.com'] }] + expect(remove.mock.calls).toEqual([ + ['https://example.com/', 'session'], + ['https://other.test/scoped', 'tracker'], + ['https://notgoogle.com/', 'lookalike'] ]) - expect(remove).not.toHaveBeenCalled() expect(set).not.toHaveBeenCalled() }) it('touches nothing when the jar is already empty', async () => { - const { session, remove, clearData } = clearSession([]) + const { session, remove, snapshotClearIdentities } = clearSession([]) - await removeTransplantableCookies(session) + await removeTransplantableCookies(session, new Set(), importedDomainScope(['example.com'])) - expect(clearData).not.toHaveBeenCalled() + expect(snapshotClearIdentities).not.toHaveBeenCalled() expect(remove).not.toHaveBeenCalled() }) - it('does not attach or clear when the jar contains only excluded cookies', async () => { - const { session, snapshotClearIdentities, clearData, remove } = clearSession([ + it('does not attach or remove when the jar contains only excluded cookies', async () => { + const { session, snapshotClearIdentities, remove } = clearSession([ cookie('.google.com', 'SID'), cookie('accounts.google.com', 'ACCOUNT') ]) - await removeTransplantableCookies(session) + await removeTransplantableCookies(session, new Set(), importedDomainScope(['google.com'])) expect(snapshotClearIdentities).not.toHaveBeenCalled() - expect(clearData).not.toHaveBeenCalled() expect(remove).not.toHaveBeenCalled() }) - it('does not mutate when a transplantable cookie cannot be represented for rollback', async () => { - const { session, clearData, remove, restoreClearIdentities } = clearSession([ + // Why (STA-4797): a cookie with no usable domain cannot be in the scope of any import, so it is + // skipped rather than failing the clear — the scope test runs before the removal-URL derivation + // precisely so an unaddressable cookie in an unrelated corner of the jar cannot abort an import + // that was never going to touch it. Either way it is never mutated. + it('skips a cookie that cannot be represented for rollback without mutating anything', async () => { + const { session, remove, snapshotClearIdentities, restoreClearIdentities } = clearSession([ { ...cookie('.example.com', 'session'), domain: '' } ]) - await expect(removeTransplantableCookies(session)).rejects.toThrow(/session was left unchanged/) + await expect( + removeTransplantableCookies(session, new Set(), importedDomainScope(['example.com'])) + ).resolves.toBeUndefined() - expect(clearData).not.toHaveBeenCalled() + expect(snapshotClearIdentities).not.toHaveBeenCalled() expect(remove).not.toHaveBeenCalled() expect(restoreClearIdentities).not.toHaveBeenCalled() }) - it('falls back to per-cookie removal when the bulk clear rejects', async () => { - const { session, remove, clearData } = clearSession( - [cookie('.example.com', 'session'), cookie('other.test', 'tracker')], - { clearData: rejectingBulkClear() } - ) - - await removeTransplantableCookies(session) - - expect(clearData).toHaveBeenCalledOnce() - expect(remove.mock.calls).toEqual([ - ['https://example.com/', 'session'], - ['https://other.test/', 'tracker'] - ]) - }) - - // Why (STA-4170): the fallback may only mutate what the identity snapshot can undo. Re-reading - // the jar here widened the removal set past the restore set. Re-removing a cookie the partial - // bulk clear already deleted is a harmless no-op, so the narrower stale plan costs nothing. - it('removes only the pre-clear snapshot after a rejected bulk clear', async () => { + // Why (STA-4170): the clear may only mutate what the identity snapshot can undo, so the plan is + // frozen from a single read. Re-reading the jar widened the removal set past the restore set. + it('removes only the frozen pre-clear plan and never re-reads the jar', async () => { const beforeAttempt = [ - cookie('.removed.test', 'gone-before-fallback'), + cookie('.removed.test', 'gone-before-clear'), cookie('.survivor.test', 'survived') ] const get = vi @@ -403,24 +363,25 @@ describe('removeTransplantableCookies', () => { cookie('.arrived.test', 'arrived-during-clear') ]) .mockResolvedValueOnce(beforeAttempt) - const { session, remove, clearData } = clearSession(beforeAttempt, { - get, - clearData: rejectingBulkClear() - }) + const { session, remove } = clearSession(beforeAttempt, { get }) - await removeTransplantableCookies(session) + await removeTransplantableCookies( + session, + new Set(), + // arrived.test is in scope: only the frozen plan may be what spares it. + importedDomainScope(['removed.test', 'survivor.test', 'arrived.test', 'google.com']) + ) - expect(clearData).toHaveBeenCalledOnce() expect(get).toHaveBeenCalledOnce() expect(remove.mock.calls).toEqual([ - ['https://removed.test/', 'gone-before-fallback'], + ['https://removed.test/', 'gone-before-clear'], ['https://survivor.test/', 'survived'] ]) }) // Why (STA-4170): arrival plus a later removal failure is the exact shape that deleted a login // the user had just completed and still reported restoration. Mutated set must equal restore set. - it('never touches a cookie that arrives while a rejected clear falls back', async () => { + it('never touches a cookie that arrives after the removal plan is frozen', async () => { const beforeAttempt = [ cookie('.example.com', 'first', '/one'), cookie('.example.com', 'second', '/two') @@ -431,7 +392,6 @@ describe('removeTransplantableCookies', () => { .mockResolvedValueOnce(beforeAttempt) const { session, remove, restoreClearIdentities } = clearSession(beforeAttempt, { get, - clearData: rejectingBulkClear(), remove: vi.fn().mockImplementation(async (_url: string, name: string) => { if (name === 'second') { throw new Error('store unavailable') @@ -439,9 +399,13 @@ describe('removeTransplantableCookies', () => { }) }) - await expect(removeTransplantableCookies(session)).rejects.toThrow( - 'existing cookies were restored' - ) + await expect( + removeTransplantableCookies( + session, + new Set(), + importedDomainScope(['example.com', 'arrived.test']) + ) + ).rejects.toThrow('existing cookies were restored') expect(remove.mock.calls).toEqual([ ['https://example.com/one', 'first'], @@ -454,25 +418,26 @@ describe('removeTransplantableCookies', () => { expect([...restored].sort()).toEqual(['first', 'second']) }) - // Why: the fallback carries the same exclusion as the bulk call, so a rejected clearData must - // not become the path that finally deletes a live Google session. - it('still preserves Google cookies on the per-cookie fallback', async () => { - const { session, remove, set } = clearSession( - [ - cookie('.google.com', 'SID'), - cookie('accounts.google.com', 'ACCOUNT'), - cookie('.example.com', 'session') - ], - { clearData: rejectingBulkClear() } - ) + // Why: the exemption is enforced per coordinate, so no removal may ever be the path that finally + // deletes a live Google session. + it('preserves Google cookies on the per-coordinate removal path', async () => { + const { session, remove, set } = clearSession([ + cookie('.google.com', 'SID'), + cookie('accounts.google.com', 'ACCOUNT'), + cookie('.example.com', 'session') + ]) - await removeTransplantableCookies(session) + await removeTransplantableCookies( + session, + new Set(), + importedDomainScope(['google.com', 'example.com']) + ) expect(remove.mock.calls).toEqual([['https://example.com/', 'session']]) expect(set).not.toHaveBeenCalled() }) - // Why (STA-4090): a failed fallback must restore through captured identities, never cookies.set. + // Why (STA-4090): a failed clear 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( [ @@ -482,7 +447,6 @@ describe('removeTransplantableCookies', () => { cookie('.example.com', 'third', '/three') ], { - clearData: rejectingBulkClear(), remove: vi.fn().mockImplementation(async (_url: string, name: string) => { if (name === 'second') { throw new Error('store unavailable') @@ -491,9 +455,13 @@ describe('removeTransplantableCookies', () => { } ) - await expect(removeTransplantableCookies(session)).rejects.toThrow( - 'existing cookies were restored' - ) + await expect( + removeTransplantableCookies( + session, + new Set(), + importedDomainScope(['google.com', 'example.com']) + ) + ).rejects.toThrow('existing cookies were restored') expect(remove).toHaveBeenCalledTimes(3) expect(set).not.toHaveBeenCalled() expect(restoreClearIdentities).toHaveBeenCalledOnce() @@ -515,7 +483,6 @@ describe('removeTransplantableCookies', () => { ...Array.from({ length: 12 }, (_, index) => cookie('.example.com', `${index}`)) ], { - clearData: rejectingBulkClear(), remove: vi.fn().mockImplementation(async () => { active++ maxActive = Math.max(maxActive, active) @@ -525,7 +492,11 @@ describe('removeTransplantableCookies', () => { } ) - const clearing = removeTransplantableCookies(session) + const clearing = removeTransplantableCookies( + session, + new Set(), + importedDomainScope(['google.com', 'example.com']) + ) await vi.waitFor(() => expect(remove).toHaveBeenCalledTimes(8)) expect(maxActive).toBe(8) releaseRemovals?.() @@ -547,7 +518,6 @@ describe('removeTransplantableCookies', () => { { ...cookie('example.com', 'session'), hostOnly: true } ], { - clearData: rejectingBulkClear(), remove: vi .fn() .mockImplementationOnce(() => firstReleased) @@ -555,7 +525,11 @@ describe('removeTransplantableCookies', () => { } ) - const clearing = removeTransplantableCookies(session) + const clearing = removeTransplantableCookies( + session, + new Set(), + importedDomainScope(['google.com', 'example.com']) + ) 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 29c5e97ee5e..04a54a47b15 100644 --- a/src/main/browser/browser-cookie-import-policy.ts +++ b/src/main/browser/browser-cookie-import-policy.ts @@ -96,15 +96,12 @@ export function normalizeCookieImportDomain(domain: string): string | null { // 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. +// public suffixes: isNonTransplantableCookieDomain exempts an entry and everything under it, and +// the staged image's clear matches the same way. 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 // users actually asked for. const NON_TRANSPLANTABLE_DOMAINS = ['google.com'] as const -export const NON_TRANSPLANTABLE_CLEAR_EXCLUDED_ORIGINS = NON_TRANSPLANTABLE_DOMAINS.map( - (root) => `https://${root}` -) export function isNonTransplantableCookieDomain(domain: string): boolean { const normalized = normalizeCookieDomain(domain) @@ -116,12 +113,6 @@ export function isNonTransplantableCookieDomain(domain: string): boolean { ) } -// Why: Chromium stores host_key lowercase as 'google.com', '.google.com' or 'sub.google.com'; -// the LIKE pattern covers the leading-dot row and cannot match lookalikes ('withgoogle.com'). -export const NON_TRANSPLANTABLE_HOST_KEY_SQL = NON_TRANSPLANTABLE_DOMAINS.map( - (root) => `host_key = '${root}' OR host_key LIKE '%.${root}'` -).join(' OR ') - // Why: subsumed by the domain exclusion above for google.com — kept because it is the general // rule for rotation-only cookies and applies to any family added without a full exclusion. export function isGoogleSourceBoundCookie(name: string, domain: string): boolean { @@ -150,11 +141,17 @@ function importDomainAncestors(domain: string): string[] { return ancestors } -function importedDomainScopes(domains: readonly string[]): { +// Why (STA-4797): one scope object, shared by every clear that precedes an import — path B's +// replacement, the native path's live-jar clear, and the staged image's delete. Each one used to +// name its own scope (or, on the native path, none at all), which is how the two import paths came +// to disagree about what an import is allowed to destroy. +export type ImportedDomainScope = { exact: Set ancestors: Set descendantRoots: Set -} { +} + +export function importedDomainScope(domains: readonly string[]): ImportedDomainScope { const exact = new Set() const ancestors = new Set() const descendantRoots = new Set() @@ -180,28 +177,30 @@ function importedDomainScopes(domains: readonly string[]): { return { exact, ancestors, descendantRoots } } -function overlapsImportedDomain( - cookie: Cookie, +// Why: hostOnly is passed rather than read off a Cookie because the staged image scopes the same +// way from a raw Chromium host_key, where the leading dot is the only host-only marker there is. +export function domainIsInImportedScope( + scope: ImportedDomainScope, domain: string, - scopes: ReturnType + hostOnly: boolean ): boolean { - if (scopes.exact.has(domain)) { + if (scope.exact.has(domain)) { return true } - if (cookie.hostOnly !== true && scopes.ancestors.has(domain)) { + if (!hostOnly && scope.ancestors.has(domain)) { return true } - return domainSuffixes(domain).some((suffix) => scopes.descendantRoots.has(suffix)) + return domainSuffixes(domain).some((suffix) => scope.descendantRoots.has(suffix)) } -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 : '/' - return url.toString() - } catch { - return null - } +// Why: takes a normalizeCookieDomain output, never a raw cookie domain. That host already parsed +// as a URL hostname, and assigning pathname cannot throw, so this always builds — both callers +// used to carry a branch for a null that could not happen, one of them a throw that failed an +// import for a reason it could never actually hit. +export function cookieRemovalUrl(cookie: Cookie, normalizedDomain: string): string { + const url = new URL(`${cookie.secure ? 'https' : 'http'}://${normalizedDomain}/`) + url.pathname = cookie.path?.startsWith('/') ? cookie.path : '/' + return url.toString() } // Why (STA-4097): 'set' stays out so the partition-dropping reconstruction cannot return. @@ -240,8 +239,8 @@ export async function replaceCookiesForImportedDomains( store: ImportedDomainReplaceStore, importedDomains: readonly string[] ): Promise { - const scopes = importedDomainScopes(importedDomains) - if (scopes.exact.size === 0) { + const scope = importedDomainScope(importedDomains) + if (scope.exact.size === 0) { return { removed: [], identities: [] } } @@ -252,13 +251,10 @@ export async function replaceCookiesForImportedDomains( const removable: { cookie: Cookie; url: string }[] = [] for (const cookie of existingCookies) { const domain = cookie.domain ? normalizeCookieDomain(cookie.domain) : null - if (!domain || !overlapsImportedDomain(cookie, domain, scopes)) { + if (!domain || !domainIsInImportedScope(scope, domain, cookie.hostOnly === true)) { continue } - const url = cookieRemovalUrl(cookie, domain) - if (url) { - removable.push({ cookie, url }) - } + removable.push({ cookie, url: cookieRemovalUrl(cookie, domain) }) } if (removable.length === 0) { return { removed: [], identities: [] } diff --git a/src/main/browser/browser-cookie-import-scope.test.ts b/src/main/browser/browser-cookie-import-scope.test.ts new file mode 100644 index 00000000000..33381f915d6 --- /dev/null +++ b/src/main/browser/browser-cookie-import-scope.test.ts @@ -0,0 +1,289 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type * as NodeFs from 'node:fs' + +const { + appGetPathMock, + execFileSyncMock, + sessionFromPartitionMock, + setPendingCookieImportMock, + clearPendingCookieImportMock, + writeCookieIdentityMock +} = vi.hoisted(() => ({ + appGetPathMock: vi.fn(), + execFileSyncMock: vi.fn(), + sessionFromPartitionMock: vi.fn(), + setPendingCookieImportMock: vi.fn(), + clearPendingCookieImportMock: vi.fn(), + writeCookieIdentityMock: vi.fn() +})) + +vi.mock('./browser-session-registry', () => ({ + browserSessionRegistry: { + setPendingCookieImport: setPendingCookieImportMock, + clearPendingCookieImport: clearPendingCookieImportMock + } +})) +vi.mock('node:child_process', () => ({ execFileSync: execFileSyncMock })) +vi.mock('node:fs', async (importOriginal) => await importOriginal()) +vi.mock('electron', () => ({ + app: { getPath: appGetPathMock }, + BrowserWindow: { fromWebContents: vi.fn() }, + 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, + writeCookieIdentity: writeCookieIdentityMock, + dispose: () => undefined + }) +})) + +import { DatabaseSync } from 'node:sqlite' +import { existsSync, mkdtempSync, rmSync } from 'node:fs' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import { importCookiesFromBrowser, type DetectedBrowser } from './browser-cookie-import' +import { createChromiumCookieTestDatabase } from './browser-cookie-import-test-database' + +// Why (STA-4797): every other end-to-end fixture in this module starts with an empty target jar, +// which is precisely why four gates — review, readiness audit, Electron QA and CI — passed an +// import that signed the user out of every site in the partition. A jar with sessions for domains +// OUTSIDE the import set is the only fixture that can observe the defect at all. +function populatedTargetJar(): Record[] { + return [ + // The site from the Electron QA report: a working login the import never mentions. + { + name: 'rack.session', + value: 'live-login', + domain: 'the-internet.herokuapp.com', + hostOnly: true, + path: '/', + secure: true, + httpOnly: true, + sameSite: 'lax' + }, + // An unrelated site whose cookie has no derivable removal URL. Out of scope, so it must not + // fail the import either. + { + name: 'orphan', + value: 'no-domain', + domain: '', + hostOnly: true, + path: '/', + secure: false, + httpOnly: false, + sameSite: 'no_restriction' + }, + // Google is exempt by policy (STA-3811) and stays exempt. + { + name: 'SID', + value: 'google-live', + domain: '.google.com', + hostOnly: false, + path: '/', + secure: true, + httpOnly: true, + sameSite: 'no_restriction' + }, + // In scope: the import brings github.com, so this stale cookie is what the clear is FOR. + { + name: 'user_session', + value: 'stale-github', + domain: '.github.com', + hostOnly: false, + path: '/', + secure: true, + httpOnly: true, + sameSite: 'lax' + } + ] +} + +function chromeBrowser(cookiesPath: string): DetectedBrowser { + return { + family: 'chrome', + label: 'Google Chrome', + cookiesPath, + keychainService: 'Chrome Safe Storage', + keychainAccount: 'Chrome', + profiles: [{ name: 'Default', directory: 'Default' }], + selectedProfile: 'Default' + } +} + +describe('native cookie import clear scope', () => { + let tmpDir: string + let cookiesRemoveMock: ReturnType + let sourceCookiesPath: string + let targetCookiesPath: string + + beforeEach(() => { + tmpDir = mkdtempSync(join(tmpdir(), 'orca-cookie-import-scope-')) + sourceCookiesPath = join(tmpDir, 'Chrome', 'Default', 'Network', 'Cookies') + targetCookiesPath = join(tmpDir, 'userData', 'Partitions', 'test', 'Network', 'Cookies') + cookiesRemoveMock = vi.fn().mockResolvedValue(undefined) + writeCookieIdentityMock.mockReset() + writeCookieIdentityMock.mockResolvedValue(undefined) + appGetPathMock.mockReset() + appGetPathMock.mockReturnValue(join(tmpDir, 'userData')) + setPendingCookieImportMock.mockClear() + clearPendingCookieImportMock.mockClear() + execFileSyncMock.mockReset() + execFileSyncMock.mockImplementation(() => { + throw new Error('OS credential commands are unavailable in this test') + }) + sessionFromPartitionMock.mockReset() + sessionFromPartitionMock.mockReturnValue({ + cookies: { + get: vi.fn().mockResolvedValue(populatedTargetJar()), + set: vi.fn(async () => undefined), + remove: cookiesRemoveMock, + flushStore: vi.fn().mockResolvedValue(undefined) + }, + clearData: vi.fn().mockResolvedValue(undefined), + setUserAgent: vi.fn(), + getStoragePath: () => join(tmpDir, 'userData', 'Partitions', 'test') + }) + }) + + afterEach(() => { + rmSync(tmpDir, { recursive: true, force: true }) + }) + + it('leaves sessions for sites outside the import set signed in', async () => { + createChromiumCookieTestDatabase(sourceCookiesPath, [ + { domain: '.github.com', name: 'user_session', value: 'imported-github' } + ]).close() + createChromiumCookieTestDatabase(targetCookiesPath, []).close() + + const result = await importCookiesFromBrowser(chromeBrowser(sourceCookiesPath), 'persist:test') + + expect(result.ok).toBe(true) + const removedNames = cookiesRemoveMock.mock.calls.map(([, name]) => name) + expect(removedNames).toEqual(['user_session']) + expect(cookiesRemoveMock).toHaveBeenCalledWith('https://github.com/', 'user_session') + // The three assertions the whole ticket is about. + expect(removedNames).not.toContain('rack.session') + expect(removedNames).not.toContain('SID') + expect(removedNames).not.toContain('orphan') + }) + + it('clears a subdomain of an imported domain, and nothing under a sibling registrable domain', async () => { + sessionFromPartitionMock.mockReturnValue({ + cookies: { + get: vi.fn().mockResolvedValue([ + { + name: 'gist-session', + value: 'stale', + domain: 'gist.github.com', + hostOnly: true, + path: '/', + secure: true, + httpOnly: true, + sameSite: 'lax' + }, + { + name: 'lookalike', + value: 'live', + domain: 'notgithub.com', + hostOnly: true, + path: '/', + secure: true, + httpOnly: true, + sameSite: 'lax' + } + ]), + set: vi.fn(async () => undefined), + remove: cookiesRemoveMock, + flushStore: vi.fn().mockResolvedValue(undefined) + }, + clearData: vi.fn().mockResolvedValue(undefined), + setUserAgent: vi.fn(), + getStoragePath: () => join(tmpDir, 'userData', 'Partitions', 'test') + }) + createChromiumCookieTestDatabase(sourceCookiesPath, [ + { domain: '.github.com', name: 'user_session', value: 'imported-github' } + ]).close() + createChromiumCookieTestDatabase(targetCookiesPath, []).close() + + const result = await importCookiesFromBrowser(chromeBrowser(sourceCookiesPath), 'persist:test') + + expect(result.ok).toBe(true) + expect(cookiesRemoveMock.mock.calls.map(([, name]) => name)).toEqual(['gist-session']) + }) + + // Why: cold-start replay uses the staged image after the live import has returned. An in-memory + // assertion cannot prove its rows and recorded merge scope preserve unrelated sessions. + it('keeps out-of-scope rows in the staged cold-start image', async () => { + // Why: no unaddressable cookie in this jar — the staged image is what this case is about, so + // the live clear must run to completion rather than aborting on something else. + sessionFromPartitionMock.mockReturnValue({ + cookies: { + get: vi + .fn() + .mockResolvedValue(populatedTargetJar().filter((cookie) => cookie.domain !== '')), + set: vi.fn(async () => undefined), + remove: cookiesRemoveMock, + flushStore: vi.fn().mockResolvedValue(undefined) + }, + clearData: vi.fn().mockResolvedValue(undefined), + setUserAgent: vi.fn(), + getStoragePath: () => join(tmpDir, 'userData', 'Partitions', 'test') + }) + writeCookieIdentityMock.mockRejectedValue(new Error('rejected so the staged image is kept')) + createChromiumCookieTestDatabase(sourceCookiesPath, [ + { domain: '.github.com', name: 'user_session', value: 'imported-github' } + ]).close() + createChromiumCookieTestDatabase( + targetCookiesPath, + [ + { domain: 'the-internet.herokuapp.com', name: 'rack.session', value: 'live-login' }, + { domain: '.google.com', name: 'SID', value: 'google-live' }, + { domain: '.github.com', name: 'user_session', value: 'stale-github' } + ], + { journalMode: 'wal' } + ).close() + + const result = await importCookiesFromBrowser(chromeBrowser(sourceCookiesPath), 'persist:test') + + expect(result.ok).toBe(true) + expect(setPendingCookieImportMock).toHaveBeenCalledTimes(1) + const stagedPath = setPendingCookieImportMock.mock.calls[0][1] as string + expect(existsSync(`${stagedPath}-wal`)).toBe(false) + const staged = new DatabaseSync(stagedPath, { readOnly: true }) + try { + expect( + staged.prepare('SELECT domain, format_version FROM orca_cookie_import_scope').all() + ).toEqual([{ domain: 'github.com', format_version: 1 }]) + const rows = ( + staged + .prepare('SELECT host_key, name, value FROM cookies ORDER BY host_key, name') + .all() as { host_key: string; name: string; value: string | Uint8Array }[] + ).map((row) => ({ + ...row, + // Why: a staged row copies the source value column verbatim, which node:sqlite hands back + // as bytes when it was written from a buffer. + value: typeof row.value === 'string' ? row.value : Buffer.from(row.value).toString('latin1') + })) + expect(rows).toEqual([ + // The stale github.com row was replaced by the imported one — the clear that is justified. + { host_key: '.github.com', name: 'user_session', value: 'imported-github' }, + { host_key: '.google.com', name: 'SID', value: 'google-live' }, + // Still signed in, one restart later. + { host_key: 'the-internet.herokuapp.com', name: 'rack.session', value: 'live-login' } + ]) + } finally { + staged.close() + } + }) +}) diff --git a/src/main/browser/browser-cookie-import.ts b/src/main/browser/browser-cookie-import.ts index c39a831a6bb..22d4dc5e270 100644 --- a/src/main/browser/browser-cookie-import.ts +++ b/src/main/browser/browser-cookie-import.ts @@ -77,9 +77,9 @@ import { browserSessionRegistry } from './browser-session-registry' import { isGoogleSourceBoundCookie, isNonTransplantableCookieDomain, - NON_TRANSPLANTABLE_HOST_KEY_SQL, normalizeCookieDomain, normalizeCookieImportDomain, + importedDomainScope, replaceCookiesForImportedDomains, type CookieImportMode, type ReplacedImportedDomainCookies @@ -111,6 +111,7 @@ import { type ChromiumCookieSnapshot } from './chromium-cookie-snapshot' import { resolveChromiumCookiesPath } from './chromium-cookie-path' +import { prepareStagedCookiesForImport } from './browser-cookie-staged-import' import { copyFileWithWindowsRetry } from '../codex-accounts/fs-utils' // --------------------------------------------------------------------------- @@ -1587,7 +1588,8 @@ export async function importCookiesFromBrowser( // Why: cookies.set() rejects many valid values (bytes > 0x7F); instead write plaintext to the `value` column, which CookieMonster reads raw when `encrypted_value` is empty and re-encrypts on flush in packaged builds. - // Why: CookieMonster overwrites the live DB on flush, so stage a populated copy and swap it in at next cold start. + // Why: CookieMonster can reject otherwise valid imported bytes, so stage a populated copy whose + // imported-domain rows can be merged into the live DB on the next cold start. const targetSession = session.fromPartition(targetPartition) // Why (STA-4601): native imports mutate the live jar and their staged image before the old // clear/write lock was reached. Hold the per-partition lock from the first flush through staging, @@ -1705,16 +1707,16 @@ export async function importCookiesFromBrowser( // transient AV handle can make opening it throw — degrade instead of killing the import. try { stagingDb = new DatabaseSync(stagingCookiesPath) + // Why (STA-4797): a new-format stage must be one self-contained file. Otherwise a lost WAL + // can erase its scope marker and make cold-start replay mistake it for a legacy whole-image + // import, restoring the unrelated-cookie data loss this format is meant to prevent. + stagingDb.exec('PRAGMA journal_mode = DELETE') targetColumnInfo = stagingDb .prepare('PRAGMA table_info(cookies)') .all() as ChromiumCookieColumnInfo[] const targetCols: string[] = targetColumnInfo.map((r) => r.name) colList = targetCols.join(', ') placeholders = targetCols.map(() => '?').join(', ') - // Why: the staged DB replaces the whole live DB at cold start, so it is a clear step - // like any other — keep the live non-transplantable rows in it rather than replaying - // a wipe the in-memory path was not allowed to perform. - stagingDb.exec(`DELETE FROM cookies WHERE NOT (${NON_TRANSPLANTABLE_HOST_KEY_SQL})`) } catch (err) { diag(` staging database unusable, restart fallback disabled: ${String(err)}`) stagingAvailable = false @@ -1846,8 +1848,8 @@ export async function importCookiesFromBrowser( disableStaging('staged database exposed no cookies columns') } - // Why (§4.3b): a staged image is a whole-DB replacement on next start, so it cannot represent - // "preserve this family". When anything is preserved, this import gets no cold-start fallback. + // Why: keep the existing conservative fallback boundary for family-level omissions. Expanding + // partial-import restart behavior is separate from narrowing what a staged replay may replace. if (nativePlan.skippedFamilies.size > 0) { disableStaging( `${nativePlan.skippedFamilies.size} preserved cookie families cannot be represented in a staged image` @@ -1955,12 +1957,29 @@ export async function importCookiesFromBrowser( }) } + for (const { entry } of scanned) { + domainSet.add(entry.domain.startsWith('.') ? entry.domain.slice(1) : entry.domain) + } + // Why (STA-4797): the import may only destroy what it is replacing. Naming the scope from the + // plan — the same rows the writes come from — is what keeps the removal set from drifting past + // the write set, and it is derived here rather than at the clear because the staged image below + // has to be cleared to the identical scope. + const importScope = importedDomainScope([...domainSet]) + + // Why (STA-4797): the staged image must carry the same imported-domain scope as the live clear. + // Cold-start replay uses it to replace only those rows and preserve newer unrelated sessions. + if (stagingDb && insertStmt) { + try { + prepareStagedCookiesForImport(stagingDb, importScope) + } catch (err) { + disableStaging(String(err)) + } + } + // EMIT: everything downstream derives from the plan, so there is no second place a row can // leak in. for (const { entry, sourceRow } of scanned) { decryptedCookies.push(entry) - const cleanDomain = entry.domain.startsWith('.') ? entry.domain.slice(1) : entry.domain - domainSet.add(cleanDomain) if (insertStmt && targetColumnInfo) { try { const params = buildChromiumCookieInsertParams( @@ -2036,9 +2055,12 @@ export async function importCookiesFromBrowser( diag(` staging skipped: ${imported} cookies will load in-memory only`) } - // 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. + // Why: clear stale cookies for the domains being imported 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. + // Why (STA-4797): every other site in the partition is exempt too. The rationale above reaches + // only as far as the domains this import writes; beyond them a clear has nothing to reconcile + // and only signs the user out of sessions the import was never about. // Why (STA-4300): one store spans the clear and the writes, so both halves of the import speak // the same CDP identities — cookies.set() cannot express the partition either one reads. const cookieClearStore = openCookieClearStore(targetSession) @@ -2048,7 +2070,6 @@ export async function importCookiesFromBrowser( await removeTransplantableCookies( { cookies: cookieClearStore, - clearData: (options) => targetSession.clearData(options), snapshotClearIdentities: (cookies) => cookieClearStore.snapshotClearIdentities(cookies), restoreClearIdentities: (identities) => cookieClearStore.restoreClearIdentities(identities) @@ -2056,10 +2077,11 @@ export async function importCookiesFromBrowser( // Why (STA-4300): the families this import declined to write must not be removed either. // Passing them here keeps their coordinates out of the removal plan AND out of the CDP // snapshot taken from it, so they are never submitted to any mutation. - nativePlan.skippedFamilies + nativePlan.skippedFamilies, + importScope ) diag( - ` cleared existing session cookies before loading ${decryptedCookies.length} imported cookies` + ` cleared existing cookies for ${domainSet.size} imported domains before loading ${decryptedCookies.length} imported cookies` ) const writable: SourceCookieToWrite[] = [] diff --git a/src/main/browser/browser-cookie-staged-import.ts b/src/main/browser/browser-cookie-staged-import.ts new file mode 100644 index 00000000000..c013ee58934 --- /dev/null +++ b/src/main/browser/browser-cookie-staged-import.ts @@ -0,0 +1,208 @@ +import { DatabaseSync } from 'node:sqlite' +import type { ImportedDomainScope } from './browser-cookie-import-policy' +import { + domainIsInImportedScope, + importedDomainScope, + isNonTransplantableCookieDomain, + normalizeCookieDomain, + normalizeCookieImportDomain +} from './browser-cookie-import-policy' + +const IMPORT_SCOPE_TABLE = 'orca_cookie_import_scope' +const IMPORT_SCOPE_FORMAT_VERSION = 1 +export const SCOPED_COOKIE_IMPORT_FORMAT = `scoped-v${IMPORT_SCOPE_FORMAT_VERSION}` + +type StagedHostKeyRow = { host_key: unknown } +type StagedScopeRow = { domain: unknown; format_version: unknown } +type SqliteColumnRow = { name: unknown } +type SqliteTableRow = { sql: unknown } + +function quotedIdentifier(identifier: string): string { + return `"${identifier.replaceAll('"', '""')}"` +} + +function cookieColumns(database: DatabaseSync, schema: 'main' | 'staged_import'): string[] { + return (database.prepare(`PRAGMA ${schema}.table_info(cookies)`).all() as SqliteColumnRow[]) + .map((row) => row.name) + .filter((name): name is string => typeof name === 'string') +} + +function cookieTableSql(database: DatabaseSync, schema: 'main' | 'staged_import'): string | null { + const row = database + .prepare(`SELECT sql FROM ${schema}.sqlite_master WHERE type = 'table' AND name = 'cookies'`) + .get() as SqliteTableRow | undefined + return typeof row?.sql === 'string' ? row.sql : null +} + +function isHostKeyInScope(hostKey: unknown, scope: ImportedDomainScope): hostKey is string { + if (typeof hostKey !== 'string' || isNonTransplantableCookieDomain(hostKey)) { + return false + } + const domain = normalizeCookieDomain(hostKey) + return domain !== null && domainIsInImportedScope(scope, domain, !hostKey.startsWith('.')) +} + +function readImportedScope( + database: DatabaseSync, + schema: 'main' | 'staged_import' +): ImportedDomainScope | null { + const marker = database + .prepare(`SELECT 1 AS present FROM ${schema}.sqlite_master WHERE type = 'table' AND name = ?`) + .get(IMPORT_SCOPE_TABLE) + if (!marker) { + return null + } + const scopeRows = database + .prepare( + `SELECT domain, format_version FROM ${schema}.${quotedIdentifier( + IMPORT_SCOPE_TABLE + )} ORDER BY domain` + ) + .all() as StagedScopeRow[] + const scopeDomains: string[] = [] + for (const row of scopeRows) { + if ( + row.format_version !== IMPORT_SCOPE_FORMAT_VERSION || + typeof row.domain !== 'string' || + normalizeCookieImportDomain(row.domain) !== row.domain + ) { + throw new Error('Staged cookie import has an invalid domain scope') + } + scopeDomains.push(row.domain) + } + if (scopeDomains.length === 0) { + throw new Error('Staged cookie import has an invalid domain scope') + } + return importedDomainScope(scopeDomains) +} + +// Why (STA-4797): the staged image is a copy of the live jar that may be replayed on the next cold +// start. Record the exact normalized import scope inside it so replay can merge only these rows into +// the then-current jar instead of replacing unrelated sessions with an older snapshot. +export function prepareStagedCookiesForImport( + stagingDb: DatabaseSync, + importScope: ImportedDomainScope +): void { + if (importScope.exact.size === 0) { + return + } + stagingDb.exec( + `CREATE TABLE IF NOT EXISTS ${quotedIdentifier(IMPORT_SCOPE_TABLE)} (` + + 'domain TEXT PRIMARY KEY, format_version INTEGER NOT NULL)' + ) + stagingDb.exec(`DELETE FROM ${quotedIdentifier(IMPORT_SCOPE_TABLE)}`) + const insertScope = stagingDb.prepare( + `INSERT INTO ${quotedIdentifier(IMPORT_SCOPE_TABLE)} (domain, format_version) VALUES (?, ?)` + ) + for (const domain of importScope.exact) { + insertScope.run(domain, IMPORT_SCOPE_FORMAT_VERSION) + } + + const hostKeys = ( + stagingDb.prepare('SELECT DISTINCT host_key FROM cookies').all() as StagedHostKeyRow[] + ).map((row) => row.host_key) + const deleteByHostKey = stagingDb.prepare('DELETE FROM cookies WHERE host_key = ?') + for (const hostKey of hostKeys) { + if (isHostKeyInScope(hostKey, importScope)) { + deleteByHostKey.run(hostKey) + } + } +} + +export function isScopedStagedCookieImport(stagedCookiesPath: string): boolean { + const database = new DatabaseSync(stagedCookiesPath, { readOnly: true }) + try { + return readImportedScope(database, 'main') !== null + } finally { + database.close() + } +} + +export function removeCookieImportScopeMarker(liveCookiesPath: string): void { + const database = new DatabaseSync(liveCookiesPath, { timeout: 1_000 }) + try { + database.exec(`DROP TABLE IF EXISTS ${quotedIdentifier(IMPORT_SCOPE_TABLE)}`) + } finally { + database.close() + } +} + +/** + * Applies a scoped staged import and returns false for a legacy unmarked whole-image replay. + * + * Why: one transaction covers every scoped delete and insert. A failure or crash therefore leaves + * the current live jar unchanged; after a committed crash, replay is idempotent and still cannot + * touch an unrelated host key. + */ +export function applyScopedStagedCookieImport( + liveCookiesPath: string, + stagedCookiesPath: string +): boolean { + const database = new DatabaseSync(liveCookiesPath, { timeout: 1_000 }) + let attached = false + let transactionOpen = false + try { + database.prepare('ATTACH DATABASE ? AS staged_import').run(stagedCookiesPath) + attached = true + const scope = readImportedScope(database, 'staged_import') + if (!scope) { + return false + } + + const liveColumns = cookieColumns(database, 'main') + const stagedColumns = cookieColumns(database, 'staged_import') + if ( + !liveColumns.includes('host_key') || + cookieTableSql(database, 'main') !== cookieTableSql(database, 'staged_import') || + liveColumns.length !== stagedColumns.length || + liveColumns.some((column, index) => column !== stagedColumns[index]) + ) { + throw new Error('Staged cookie import has an incompatible cookie schema') + } + // Why: partial column intersections can silently erase a partition key or fill a newly required + // column with the wrong default after an app update. Exact schema parity is the safe boundary. + const columnList = liveColumns.map(quotedIdentifier).join(', ') + database.exec('BEGIN IMMEDIATE') + transactionOpen = true + const hostKeys = database + .prepare('SELECT host_key FROM main.cookies UNION SELECT host_key FROM staged_import.cookies') + .all() as StagedHostKeyRow[] + const deleteLiveHost = database.prepare('DELETE FROM main.cookies WHERE host_key = ?') + const insertStagedHost = database.prepare( + `INSERT OR REPLACE INTO main.cookies (${columnList}) ` + + `SELECT ${columnList} FROM staged_import.cookies WHERE host_key = ?` + ) + + for (const { host_key: hostKey } of hostKeys) { + if (!isHostKeyInScope(hostKey, scope)) { + continue + } + deleteLiveHost.run(hostKey) + insertStagedHost.run(hostKey) + } + // A missing-live replay may have copied a full marked image on an earlier interrupted start. + // Never leave that marker in Chromium's live DB where a downgrade could propagate it. + database.exec(`DROP TABLE IF EXISTS main.${quotedIdentifier(IMPORT_SCOPE_TABLE)}`) + database.exec('COMMIT') + transactionOpen = false + return true + } catch (error) { + if (transactionOpen) { + try { + database.exec('ROLLBACK') + } catch { + /* closing the database rolls back any transaction SQLite still owns */ + } + } + throw error + } finally { + if (attached) { + try { + database.exec('DETACH DATABASE staged_import') + } catch { + /* close releases the attached file even after a failed transaction */ + } + } + database.close() + } +} diff --git a/src/main/browser/browser-session-cookie-staging.scoped.test.ts b/src/main/browser/browser-session-cookie-staging.scoped.test.ts new file mode 100644 index 00000000000..c5eba68c6d4 --- /dev/null +++ b/src/main/browser/browser-session-cookie-staging.scoped.test.ts @@ -0,0 +1,270 @@ +import { copyFileSync, existsSync, mkdtempSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { DatabaseSync } from 'node:sqlite' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { appGetPathMock } = vi.hoisted(() => ({ appGetPathMock: vi.fn() })) + +vi.mock('electron', () => ({ app: { getPath: appGetPathMock } })) + +import { importedDomainScope } from './browser-cookie-import-policy' +import { createChromiumCookieTestDatabase } from './browser-cookie-import-test-database' +import { prepareStagedCookiesForImport } from './browser-cookie-staged-import' +import { + applyPendingBrowserCookieImports, + setPendingBrowserCookieImport +} from './browser-session-cookie-staging' +import { loadBrowserSessionMeta, persistBrowserSessionMeta } from './browser-session-meta-store' + +const PARTITION = 'persist:test' + +type CookieRow = { host_key: string; name: string; value: string } + +function insertCookie(database: DatabaseSync, domain: string, name: string, value: string): void { + database + .prepare( + `INSERT INTO cookies ( + creation_utc, host_key, top_frame_site_key, name, value, encrypted_value, path, + expires_utc, is_secure, is_httponly, samesite, source_scheme, source_port, + last_update_utc, has_cross_site_ancestor + ) VALUES (1, ?, '', ?, ?, X'', '/', 0, 0, 0, 0, 0, -1, 0, 0)` + ) + .run(domain, name, value) +} + +function readCookies(databasePath: string): CookieRow[] { + const database = new DatabaseSync(databasePath, { readOnly: true }) + try { + return database + .prepare('SELECT host_key, name, value FROM cookies ORDER BY host_key, name') + .all() as CookieRow[] + } finally { + database.close() + } +} + +describe('scoped pending browser cookie imports', () => { + let tmpDir: string + let livePath: string + let metaPath: string + let stagedPath: string + + beforeEach(() => { + tmpDir = mkdtempSync(join(tmpdir(), 'orca-scoped-cookie-replay-')) + livePath = join(tmpDir, 'Partitions', 'test', 'Network', 'Cookies') + metaPath = join(tmpDir, 'browser-session-meta.json') + stagedPath = join(tmpDir, 'staged-cookies') + appGetPathMock.mockReset() + appGetPathMock.mockReturnValue(tmpDir) + }) + + afterEach(() => { + rmSync(tmpDir, { recursive: true, force: true }) + }) + + function markPending(): void { + setPendingBrowserCookieImport({ + resolveMetadataPath: () => metaPath, + defaultPartition: PARTITION, + partition: PARTITION, + stagingDbPath: stagedPath + }) + } + + function applyPending(): void { + applyPendingBrowserCookieImports({ + resolveMetadataPath: () => metaPath, + defaultPartition: PARTITION, + activeOrcaProfileId: 'test-profile' + }) + } + + function createScopedStage(): void { + copyFileSync(livePath, stagedPath) + const staged = new DatabaseSync(stagedPath) + try { + prepareStagedCookiesForImport(staged, importedDomainScope(['a.example.com'])) + insertCookie(staged, '.a.example.com', 'new-a', 'imported') + } finally { + staged.close() + } + } + + it('preserves unrelated cookies changed after staging while merging imported domains', () => { + createChromiumCookieTestDatabase( + livePath, + [ + { domain: '.example.com', name: 'parent', value: 'stale-parent' }, + { domain: '.a.example.com', name: 'old-a', value: 'stale' }, + { domain: 'child.a.example.com', name: 'child', value: 'stale-child' }, + { domain: '.unrelated.example.com', name: 'session', value: 'before-stage' } + ], + { journalMode: 'wal' } + ).close() + createScopedStage() + expect(existsSync(`${stagedPath}-wal`)).toBe(false) + + const changedLive = new DatabaseSync(livePath) + changedLive + .prepare("UPDATE cookies SET value = 'rotated-after-stage' WHERE name = 'session'") + .run() + insertCookie(changedLive, '.another.example.com', 'new-login', 'created-after-stage') + changedLive.close() + markPending() + + applyPending() + + expect(readCookies(livePath)).toEqual([ + { host_key: '.a.example.com', name: 'new-a', value: 'imported' }, + { host_key: '.another.example.com', name: 'new-login', value: 'created-after-stage' }, + { host_key: '.unrelated.example.com', name: 'session', value: 'rotated-after-stage' } + ]) + expect(existsSync(stagedPath)).toBe(false) + expect(loadBrowserSessionMeta(() => metaPath, PARTITION).pendingCookieImports).toEqual({}) + }) + + it('rolls back the merge and keeps the staged retry when an insert fails', () => { + createChromiumCookieTestDatabase(livePath, [ + { domain: '.a.example.com', name: 'old-a', value: 'keep-on-failure' }, + { domain: '.unrelated.example.com', name: 'session', value: 'live' } + ]).close() + createScopedStage() + const rejectingLive = new DatabaseSync(livePath) + rejectingLive.exec(` + CREATE TRIGGER reject_staged_cookie BEFORE INSERT ON cookies + WHEN NEW.name = 'new-a' + BEGIN + SELECT RAISE(ABORT, 'forced staged insert failure'); + END + `) + rejectingLive.close() + markPending() + + applyPending() + + expect(readCookies(livePath)).toEqual([ + { host_key: '.a.example.com', name: 'old-a', value: 'keep-on-failure' }, + { host_key: '.unrelated.example.com', name: 'session', value: 'live' } + ]) + expect(existsSync(stagedPath)).toBe(true) + expect(loadBrowserSessionMeta(() => metaPath, PARTITION).pendingCookieImports).toEqual({ + [PARTITION]: { format: 'scoped-v1', path: stagedPath } + }) + // Why: both SQLite handles must be closed after failure so Windows can read the retry file. + expect(() => copyFileSync(stagedPath, `${stagedPath}.copy`)).not.toThrow() + }) + + it('copies a scoped image when the live cookie database no longer exists', () => { + createChromiumCookieTestDatabase(livePath, [ + { domain: '.a.example.com', name: 'old-a', value: 'stale' }, + { domain: '.unrelated.example.com', name: 'session', value: 'preserved' } + ]).close() + createScopedStage() + rmSync(livePath) + markPending() + + applyPending() + + // With neither modern nor legacy DB present, replay uses Chromium's legacy fallback path. + const replayedPath = join(tmpDir, 'Partitions', 'test', 'Cookies') + expect(readCookies(replayedPath)).toEqual([ + { host_key: '.a.example.com', name: 'new-a', value: 'imported' }, + { host_key: '.unrelated.example.com', name: 'session', value: 'preserved' } + ]) + const replayed = new DatabaseSync(replayedPath, { readOnly: true }) + expect( + replayed + .prepare("SELECT name FROM sqlite_master WHERE type = 'table' AND name LIKE 'orca_%'") + .all() + ).toEqual([]) + replayed.close() + expect(existsSync(stagedPath)).toBe(false) + }) + + it('keeps whole-image replay for legacy staged databases without a scope marker', () => { + createChromiumCookieTestDatabase(livePath, [ + { domain: '.live.example', name: 'live', value: 'replaced' } + ]).close() + createChromiumCookieTestDatabase(stagedPath, [ + { domain: '.legacy.example', name: 'legacy', value: 'pending' } + ]).close() + persistBrowserSessionMeta(() => metaPath, PARTITION, { + pendingCookieImports: { [PARTITION]: stagedPath }, + pendingCookieDbPath: stagedPath + }) + + applyPending() + + expect(readCookies(livePath)).toEqual([ + { host_key: '.legacy.example', name: 'legacy', value: 'pending' } + ]) + expect(existsSync(stagedPath)).toBe(false) + expect(loadBrowserSessionMeta(() => metaPath, PARTITION).pendingCookieImports).toEqual({}) + }) + + it('fails closed across cookie schema drift without mutating the live jar', () => { + createChromiumCookieTestDatabase(livePath, [ + { domain: '.a.example.com', name: 'old-a', value: 'keep-on-mismatch' } + ]).close() + createScopedStage() + const changedSchema = new DatabaseSync(livePath) + changedSchema.exec("ALTER TABLE cookies ADD COLUMN future_required TEXT NOT NULL DEFAULT 'v'") + changedSchema.close() + markPending() + + applyPending() + + expect(readCookies(livePath)).toEqual([ + { host_key: '.a.example.com', name: 'old-a', value: 'keep-on-mismatch' } + ]) + expect(existsSync(stagedPath)).toBe(true) + expect(loadBrowserSessionMeta(() => metaPath, PARTITION).pendingCookieImports).toEqual({ + [PARTITION]: { format: 'scoped-v1', path: stagedPath } + }) + }) + + it('fails closed on an unknown staged scope format', () => { + createChromiumCookieTestDatabase(livePath, [ + { domain: '.a.example.com', name: 'old-a', value: 'keep-on-unknown-format' } + ]).close() + createScopedStage() + const invalidStage = new DatabaseSync(stagedPath) + invalidStage.exec('UPDATE orca_cookie_import_scope SET format_version = 999') + invalidStage.close() + markPending() + + applyPending() + + expect(readCookies(livePath)).toEqual([ + { host_key: '.a.example.com', name: 'old-a', value: 'keep-on-unknown-format' } + ]) + expect(existsSync(stagedPath)).toBe(true) + expect(loadBrowserSessionMeta(() => metaPath, PARTITION).pendingCookieImports).toEqual({ + [PARTITION]: { format: 'scoped-v1', path: stagedPath } + }) + }) + + it('leaves a future pending-import format untouched', () => { + createChromiumCookieTestDatabase(livePath, [ + { domain: '.a.example.com', name: 'old-a', value: 'keep-for-newer-build' } + ]).close() + createScopedStage() + persistBrowserSessionMeta(() => metaPath, PARTITION, { + pendingCookieImports: { + [PARTITION]: { format: 'scoped-v2', path: stagedPath } + }, + pendingCookieDbPath: null + }) + + applyPending() + + expect(readCookies(livePath)).toEqual([ + { host_key: '.a.example.com', name: 'old-a', value: 'keep-for-newer-build' } + ]) + expect(existsSync(stagedPath)).toBe(true) + expect(loadBrowserSessionMeta(() => metaPath, PARTITION).pendingCookieImports).toEqual({ + [PARTITION]: { format: 'scoped-v2', path: stagedPath } + }) + }) +}) diff --git a/src/main/browser/browser-session-cookie-staging.ts b/src/main/browser/browser-session-cookie-staging.ts index 70ba92f8ec4..192c55a178a 100644 --- a/src/main/browser/browser-session-cookie-staging.ts +++ b/src/main/browser/browser-session-cookie-staging.ts @@ -1,8 +1,19 @@ import { app } from 'electron' import { copyFileSync, existsSync, mkdirSync, unlinkSync } from 'node:fs' import { join } from 'node:path' -import { loadBrowserSessionMeta, persistBrowserSessionMeta } from './browser-session-meta-store' +import { + loadBrowserSessionMeta, + persistBrowserSessionMeta, + type PendingBrowserCookieImport +} from './browser-session-meta-store' import { isValidPersistedBrowserSessionProfile } from './browser-session-persisted-profile-validation' +import { renameFileWithWindowsRetry } from '../codex-accounts/fs-utils' +import { + applyScopedStagedCookieImport, + isScopedStagedCookieImport, + removeCookieImportScopeMarker, + SCOPED_COOKIE_IMPORT_FORMAT +} from './browser-cookie-staged-import' import { resolveChromiumCookiesPath } from './chromium-cookie-path' type PendingCookieImportTarget = { @@ -11,6 +22,16 @@ type PendingCookieImportTarget = { defaultPartition: string } +function legacyPendingPath(entry: PendingBrowserCookieImport | undefined): string | null { + return typeof entry === 'string' ? entry : null +} + +function scopedPendingPath(entry: PendingBrowserCookieImport): string | null { + return typeof entry !== 'string' && entry.format === SCOPED_COOKIE_IMPORT_FORMAT + ? entry.path + : null +} + function partitionCookiesPath(partition: string): string { const partitionName = partition.replace('persist:', '') const partitionDir = join(app.getPath('userData'), 'Partitions', partitionName) @@ -18,6 +39,20 @@ function partitionCookiesPath(partition: string): string { return resolveChromiumCookiesPath(partitionDir) ?? join(partitionDir, 'Cookies') } +function consumeStagedCookieImport(stagedPath: string): void { + // Why: metadata persistence is best-effort. Move the replay source out of its recorded path + // before updating metadata so a simultaneous metadata-write failure cannot replay it next start. + const consumedPath = `${stagedPath}.consumed` + renameFileWithWindowsRetry(stagedPath, consumedPath) + for (const path of [consumedPath, `${stagedPath}-wal`, `${stagedPath}-shm`]) { + try { + unlinkSync(path) + } catch { + /* the recorded replay path is already absent */ + } + } +} + // Why: must run before any session.fromPartition() so CookieMonster reads the staged cookies instead of overwriting them from its in-memory DB. export function applyPendingBrowserCookieImports({ resolveMetadataPath, @@ -39,11 +74,18 @@ export function applyPendingBrowserCookieImports({ } const remainingEntries = { ...meta.pendingCookieImports } - for (const [partition, stagedPath] of pendingEntries) { + for (const [partition, pendingEntry] of pendingEntries) { if (!knownPartitions.has(partition)) { delete remainingEntries[partition] continue } + const scopedPath = scopedPendingPath(pendingEntry) + const stagedPath = typeof pendingEntry === 'string' ? pendingEntry : scopedPath + // Why: future formats must remain pending for a newer build instead of being replayed as a + // legacy whole image or silently discarded by an older one. + if (!stagedPath) { + continue + } if (!existsSync(stagedPath)) { delete remainingEntries[partition] continue @@ -52,36 +94,50 @@ export function applyPendingBrowserCookieImports({ const liveCookiesPath = partitionCookiesPath(partition) try { mkdirSync(join(liveCookiesPath, '..'), { recursive: true }) - copyFileSync(stagedPath, liveCookiesPath) - // Why: stale WAL/SHM sidecars would corrupt CookieMonster's read of the freshly swapped DB. - let sidecarCopyFailed = false - for (const suffix of ['-wal', '-shm']) { - try { - unlinkSync(liveCookiesPath + suffix) - } catch { - /* may not exist */ + // Why: a scoped stage is still a complete valid image. If the live DB was removed, copying + // it cannot overwrite a newer unrelated cookie and avoids creating an empty DB that can + // never satisfy the scoped merge's schema check. + const liveCookiesExist = existsSync(liveCookiesPath) + const markedScopedImage = isScopedStagedCookieImport(stagedPath) + if (scopedPath && !markedScopedImage) { + throw new Error('Scoped cookie import is missing its scope marker') + } + const copiedScopedImage = !liveCookiesExist && markedScopedImage + const appliedScopedImport = + liveCookiesExist && markedScopedImage + ? applyScopedStagedCookieImport(liveCookiesPath, stagedPath) + : false + if (!appliedScopedImport) { + // Why: staged imports written before scoped replay existed have no marker. Preserve their + // existing whole-image behavior so an update does not strand an already pending import. + copyFileSync(stagedPath, liveCookiesPath) + // Why: stale WAL/SHM sidecars would corrupt CookieMonster's read of the freshly swapped DB. + let sidecarCopyFailed = false + for (const suffix of ['-wal', '-shm']) { + try { + unlinkSync(liveCookiesPath + suffix) + } catch { + /* may not exist */ + } + const stagingSidecar = stagedPath + suffix + if (!existsSync(stagingSidecar)) { + continue + } + try { + copyFileSync(stagingSidecar, liveCookiesPath + suffix) + } catch { + sidecarCopyFailed = true + } } - const stagingSidecar = stagedPath + suffix - if (!existsSync(stagingSidecar)) { + if (sidecarCopyFailed) { + // Why: sidecar copy failed → inconsistent replay; keep this entry for retry. continue } - try { - copyFileSync(stagingSidecar, liveCookiesPath + suffix) - } catch { - sidecarCopyFailed = true - } - } - if (sidecarCopyFailed) { - // Why: sidecar copy failed → inconsistent replay; keep this entry for retry. - continue - } - for (const ext of ['', '-wal', '-shm']) { - try { - unlinkSync(`${stagedPath}${ext}`) - } catch { - /* best-effort */ + if (copiedScopedImage) { + removeCookieImportScopeMarker(liveCookiesPath) } } + consumeStagedCookieImport(stagedPath) delete remainingEntries[partition] } catch { // Why: keep this entry for retry — one partition's failed replay shouldn't drop unrelated entries. @@ -89,7 +145,7 @@ export function applyPendingBrowserCookieImports({ } persistBrowserSessionMeta(resolveMetadataPath, defaultPartition, { pendingCookieImports: remainingEntries, - pendingCookieDbPath: remainingEntries[defaultPartition] ?? null + pendingCookieDbPath: legacyPendingPath(remainingEntries[defaultPartition]) }) } catch { // best-effort — if this fails, CookieMonster loads the old DB @@ -103,10 +159,13 @@ export function setPendingBrowserCookieImport({ stagingDbPath }: PendingCookieImportTarget & { partition: string; stagingDbPath: string }): void { const meta = loadBrowserSessionMeta(resolveMetadataPath, defaultPartition) - const pendingCookieImports = { ...meta.pendingCookieImports, [partition]: stagingDbPath } + const pendingCookieImports = { + ...meta.pendingCookieImports, + [partition]: { format: SCOPED_COOKIE_IMPORT_FORMAT, path: stagingDbPath } + } persistBrowserSessionMeta(resolveMetadataPath, defaultPartition, { pendingCookieImports, - pendingCookieDbPath: pendingCookieImports[defaultPartition] ?? null + pendingCookieDbPath: legacyPendingPath(pendingCookieImports[defaultPartition]) }) } @@ -121,11 +180,21 @@ export function clearPendingBrowserCookieImport({ return } const pendingCookieImports = { ...meta.pendingCookieImports } - const stagedPath = pendingCookieImports[partition] + const pendingEntry = pendingCookieImports[partition] + const stagedPath = typeof pendingEntry === 'string' ? pendingEntry : pendingEntry.path + // Why: metadata writes and file removal are both best-effort. Consuming the recorded path first + // and then persisting its removal gives either operation a chance to prevent a stale replay. + if (existsSync(stagedPath)) { + try { + consumeStagedCookieImport(stagedPath) + } catch { + /* metadata removal below is the fallback */ + } + } delete pendingCookieImports[partition] persistBrowserSessionMeta(resolveMetadataPath, defaultPartition, { pendingCookieImports, - pendingCookieDbPath: pendingCookieImports[defaultPartition] ?? null + pendingCookieDbPath: legacyPendingPath(pendingCookieImports[defaultPartition]) }) for (const suffix of ['', '-wal', '-shm']) { try { diff --git a/src/main/browser/browser-session-meta-store.ts b/src/main/browser/browser-session-meta-store.ts index 6fb8eeb3a15..8aeb7c36422 100644 --- a/src/main/browser/browser-session-meta-store.ts +++ b/src/main/browser/browser-session-meta-store.ts @@ -2,6 +2,13 @@ import { mkdirSync, readFileSync, renameSync, writeFileSync } from 'node:fs' import { dirname } from 'node:path' import type { BrowserSessionProfile } from '../../shared/browser-workspace-types' +export type PendingBrowserCookieImport = + | string + | { + format: string + path: string + } + // Why: no userAgent fields — the session UA is always derived from the running // engine at startup (clean or native), never persisted. Imports before Aug 2026 // stored a synthesized source-browser UA here; persistMeta drops those legacy @@ -9,7 +16,7 @@ import type { BrowserSessionProfile } from '../../shared/browser-workspace-types export type BrowserSessionMeta = { defaultSource: BrowserSessionProfile['source'] pendingCookieDbPath: string | null - pendingCookieImports: Record + pendingCookieImports: Record profiles: BrowserSessionProfile[] } @@ -25,10 +32,20 @@ export function loadBrowserSessionMeta( const data = JSON.parse(raw) const legacyPendingCookieDbPath = typeof data?.pendingCookieDbPath === 'string' ? data.pendingCookieDbPath : null - const pendingCookieImports: Record = - data && typeof data.pendingCookieImports === 'object' && data.pendingCookieImports - ? { ...data.pendingCookieImports } - : {} + const pendingCookieImports: Record = {} + if (data && typeof data.pendingCookieImports === 'object' && data.pendingCookieImports) { + for (const [partition, entry] of Object.entries(data.pendingCookieImports)) { + if ( + typeof entry === 'string' || + (entry && + typeof entry === 'object' && + typeof (entry as { format?: unknown }).format === 'string' && + typeof (entry as { path?: unknown }).path === 'string') + ) { + pendingCookieImports[partition] = entry as PendingBrowserCookieImport + } + } + } if (legacyPendingCookieDbPath && !pendingCookieImports[defaultPartition]) { pendingCookieImports[defaultPartition] = legacyPendingCookieDbPath } diff --git a/src/main/browser/browser-session-registry.persistence.test.ts b/src/main/browser/browser-session-registry.persistence.test.ts index 193379239fc..cb9229174ed 100644 --- a/src/main/browser/browser-session-registry.persistence.test.ts +++ b/src/main/browser/browser-session-registry.persistence.test.ts @@ -122,6 +122,30 @@ function installModuleMocks( cleanElectronUserAgent: vi.fn((ua: string) => ua.replace(/\s*Electron\/\S+/, '')), setupClientHintsOverride: setupClientHintsOverrideMock })) + // This suite models replay with an in-memory filesystem. The real file-backed SQLite merge has + // dedicated coverage; these fixtures are legacy unmarked images and keep the copy path. + vi.doMock('./browser-cookie-staged-import', () => ({ + SCOPED_COOKIE_IMPORT_FORMAT: 'scoped-v1', + applyScopedStagedCookieImport: vi.fn(() => false), + isScopedStagedCookieImport: vi.fn(() => false), + removeCookieImportScopeMarker: vi.fn() + })) + vi.doMock('../codex-accounts/fs-utils', () => ({ + renameFileWithWindowsRetry: vi.fn((source: string, target: string) => { + const sourceKey = fsKey(source) + const targetKey = fsKey(target) + if (!fsState.present.has(sourceKey)) { + throw new Error('ENOENT') + } + const value = fsState.files.get(sourceKey) + fsState.present.delete(sourceKey) + fsState.files.delete(sourceKey) + fsState.present.add(targetKey) + if (value !== undefined) { + fsState.files.set(targetKey, value) + } + }) + })) return { sessionFromPartitionMock, @@ -257,10 +281,13 @@ describe('BrowserSessionRegistry persistence', () => { ) const written = JSON.parse(fsState.files.get(META_PATH) ?? '{}') - expect(written.pendingCookieDbPath).toBe('/staged/default') + expect(written.pendingCookieDbPath).toBeNull() expect(written.pendingCookieImports).toEqual({ - 'persist:orca-browser': '/staged/default', - 'persist:orca-browser-session-aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa': '/staged/imported' + 'persist:orca-browser': { format: 'scoped-v1', path: '/staged/default' }, + 'persist:orca-browser-session-aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa': { + format: 'scoped-v1', + path: '/staged/imported' + } }) }) diff --git a/src/main/browser/browser-session-registry.ts b/src/main/browser/browser-session-registry.ts index f8db8ab54a0..b5e1ff7dc50 100644 --- a/src/main/browser/browser-session-registry.ts +++ b/src/main/browser/browser-session-registry.ts @@ -236,9 +236,10 @@ class BrowserSessionRegistry { const meta = this.loadPersistedMeta() const pendingCookieImports = { ...meta.pendingCookieImports } delete pendingCookieImports[profile.partition] + const defaultPendingImport = pendingCookieImports[this.defaultPartition] this.persistMeta({ pendingCookieImports, - pendingCookieDbPath: pendingCookieImports[this.defaultPartition] ?? null + pendingCookieDbPath: typeof defaultPendingImport === 'string' ? defaultPendingImport : null }) // Why: clear the partition's storage so deleting a profile doesn't leave orphaned cookies/cache behind.