fix(browser): re-read the cookie jar after a rejected bulk clear

clearData can empty part of the jar before rejecting, so the per-cookie
fallback was working from a snapshot taken before the attempt: it retried
removals for cookies that were already gone and could not see cookies that
arrived during the clear. Re-read on rejection only.

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