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