diff --git a/browse/src/meta-commands.ts b/browse/src/meta-commands.ts index 4cd296492..64c2e3314 100644 --- a/browse/src/meta-commands.ts +++ b/browse/src/meta-commands.ts @@ -20,6 +20,7 @@ import * as path from 'path'; import { writeSecureFile, mkdirSecure } from './file-permissions'; import { TEMP_DIR } from './platform'; import { resolveConfig } from './config'; +import { filterSessionCookies } from './session-persist'; import type { Frame } from 'playwright'; /** Tokenize a pipe segment respecting double-quoted strings. */ @@ -959,15 +960,13 @@ export async function handleMetaCommand( if (!Array.isArray(data.cookies) || !Array.isArray(data.pages)) { throw new Error('Invalid state file: expected cookies and pages arrays'); } - // Validate and filter cookies — reject malformed or internal-network cookies - const validatedCookies = data.cookies.filter((c: any) => { - if (typeof c !== 'object' || !c) return false; - if (typeof c.name !== 'string' || typeof c.value !== 'string') return false; - if (typeof c.domain !== 'string' || !c.domain) return false; - const d = c.domain.startsWith('.') ? c.domain.slice(1) : c.domain; - if (d === 'localhost' || d.endsWith('.internal') || d === '169.254.169.254') return false; - return true; - }); + // Validate and filter cookies via the shared hygiene filter in + // session-persist.ts (isInternalCookieDomain): rejects malformed + // cookies and internal-network domains — localhost, *.internal, + // loopback literals (127.x, ::1), and link-local/cloud-metadata + // (169.254.x) — that a tampered state file could use to reach local + // services or the metadata endpoint. + const validatedCookies = filterSessionCookies(data.cookies); if (validatedCookies.length < data.cookies.length) { console.warn(`[browse] Filtered ${data.cookies.length - validatedCookies.length} invalid cookies from state file`); } diff --git a/browse/src/session-persist.ts b/browse/src/session-persist.ts index 3f4b9fd03..3c6627bcb 100644 --- a/browse/src/session-persist.ts +++ b/browse/src/session-persist.ts @@ -66,18 +66,34 @@ export function serializeSessionState(state: BrowserState): string { } /** - * Same cookie hygiene as `state load` (meta-commands.ts, kept in sync by - * comment there): drop malformed cookies and internal-network domains a - * tampered file could use to reach localhost services or cloud metadata. + * True when a cookie domain points at an internal-network target a tampered + * state file could use to reach localhost services, *.internal hosts, or + * cloud metadata: `localhost`, `*.internal`, IPv4 loopback literals + * (127.0.0.0/8), IPv6 loopback (`::1`, `[::1]`), and link-local/metadata + * (169.254.0.0/16, which covers 169.254.169.254). Leading-dot domain + * variants (`.127.0.0.1`) are normalized before matching. Single source of + * truth for the persistence restore path here AND `state load` + * (meta-commands.ts). + */ +export function isInternalCookieDomain(domain: string): boolean { + const d = domain.startsWith('.') ? domain.slice(1) : domain; + if (d === 'localhost' || d.endsWith('.internal')) return true; + if (d === '::1' || d === '[::1]') return true; // IPv6 loopback + if (/^127\./.test(d)) return true; // IPv4 loopback block + if (/^169\.254\./.test(d)) return true; // link-local incl. cloud metadata + return false; +} + +/** + * Cookie hygiene shared with `state load` (meta-commands.ts): drop malformed + * cookies and internal-network domains (see isInternalCookieDomain). */ export function filterSessionCookies(cookies: unknown[]): BrowserState['cookies'] { return cookies.filter((c: any) => { if (typeof c !== 'object' || !c) return false; if (typeof c.name !== 'string' || typeof c.value !== 'string') return false; if (typeof c.domain !== 'string' || !c.domain) return false; - const d = c.domain.startsWith('.') ? c.domain.slice(1) : c.domain; - if (d === 'localhost' || d.endsWith('.internal') || d === '169.254.169.254') return false; - return true; + return !isInternalCookieDomain(c.domain); }) as BrowserState['cookies']; } @@ -118,21 +134,33 @@ export function deserializeSessionState(raw: string): BrowserState | null { export async function persistSessionState(bm: BrowserManager, filePath: string): Promise { if (bm.getConnectionMode() !== 'launched') return; const state = await bm.saveState(); - writeSecureFile(filePath, serializeSessionState(state)); + // Atomic replace: stage the new snapshot beside the target, then rename + // over it. A crash mid-write must never destroy the previous good + // snapshot — surviving crashes is the point of this feature. + const tmpPath = `${filePath}.tmp`; + writeSecureFile(tmpPath, serializeSessionState(state)); + try { + fs.renameSync(tmpPath, filePath); + } catch (err) { + safeUnlinkQuiet(tmpPath); + throw err; + } } /** - * Restore a persisted session into a freshly launched manager. Returns true - * when state was restored, false when there was nothing (or corrupt data — - * which is warned, deleted, and skipped rather than blocking launch). - * restoreState re-validates every URL before navigating. + * Restore a persisted session into a freshly launched manager. Returns the + * restored (already-filtered) state so callers can log counts without an + * extra saveState() round-trip, or null when there was nothing to restore + * (missing file, or corrupt data — which is warned, quarantined, and skipped + * rather than blocking launch). restoreState re-validates every URL before + * navigating. */ -export async function restoreSessionState(bm: BrowserManager, filePath: string): Promise { +export async function restoreSessionState(bm: BrowserManager, filePath: string): Promise { let raw: string; try { raw = fs.readFileSync(filePath, 'utf-8'); } catch (err: any) { - if (err?.code === 'ENOENT') return false; + if (err?.code === 'ENOENT') return null; throw err; } const state = deserializeSessionState(raw); @@ -141,10 +169,10 @@ export async function restoreSessionState(bm: BrowserManager, filePath: string): // 3-week-later bug report is reconstructable from the artifact. console.warn(`[browse] SESSION_STATE_INVALID: corrupt ${filePath} moved to .corrupt; starting fresh`); quarantineCorrupt(filePath); - return false; + return null; } // launch() opens one blank tab; replace it rather than restoring alongside. await bm.closeAllPages(); await bm.restoreState(state); - return true; + return state; } diff --git a/browse/test/busy-daemon-recovery.test.ts b/browse/test/busy-daemon-recovery.test.ts index 248ebd893..93f8d079c 100644 --- a/browse/test/busy-daemon-recovery.test.ts +++ b/browse/test/busy-daemon-recovery.test.ts @@ -88,44 +88,47 @@ describe('#1781 busy-daemon recovery (CLI integration)', () => { const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-busy-')); const stateFile = path.join(tmpDir, 'browse.json'); const daemon = await startWedgedDaemon(); + try { + // A live process standing in for the daemon PID. If the CLI takes the + // dead path it SIGTERMs this child — the aliveness assert catches it. + daemonPidChild = spawn('sleep', ['60'], { stdio: 'ignore' }); + const daemonPid = daemonPidChild.pid!; - // A live process standing in for the daemon PID. If the CLI takes the - // dead path it SIGTERMs this child — the aliveness assert catches it. - daemonPidChild = spawn('sleep', ['60'], { stdio: 'ignore' }); - const daemonPid = daemonPidChild.pid!; + const stateContent = { + pid: daemonPid, + port: daemon.port, + token: 'busy-test-token', + startedAt: new Date().toISOString(), + serverPath: '', + mode: 'launched' as const, + }; + fs.writeFileSync(stateFile, JSON.stringify(stateContent, null, 2)); - const stateContent = { - pid: daemonPid, - port: daemon.port, - token: 'busy-test-token', - startedAt: new Date().toISOString(), - serverPath: '', - mode: 'launched' as const, - }; - fs.writeFileSync(stateFile, JSON.stringify(stateContent, null, 2)); + const env: Record = {}; + for (const [k, v] of Object.entries(process.env)) { + if (v !== undefined) env[k] = v; + } + env.BROWSE_STATE_FILE = stateFile; - const env: Record = {}; - for (const [k, v] of Object.entries(process.env)) { - if (v !== undefined) env[k] = v; + const result = await runCli(['status'], env); + + // Recovered: retried the same command against the same daemon instance. + expect(result.code).toBe(0); + expect(result.stdout).toContain(`RECOVERED ${BOOT_ID}`); + // The fork's CLI announces the busy retry on stderr; ours retries at the + // probe layer without a message. Either is fine — the load-bearing + // behavior is retry-without-kill, asserted below. + expect(daemon.commandRequests).toBe(2); // wedged once, served once + + // Never killed, never restarted — tab/cookie state intact. + expect(result.stderr).not.toContain('Restarting'); + expect(isProcessAlive(daemonPid)).toBe(true); + expect(JSON.parse(fs.readFileSync(stateFile, 'utf-8'))).toEqual(stateContent); + } finally { + // Cleanup must run even when an assertion throws — otherwise a failed + // run leaks the wedged fake daemon and the tmp dir. + await daemon.close(); + fs.rmSync(tmpDir, { recursive: true, force: true }); } - env.BROWSE_STATE_FILE = stateFile; - - const result = await runCli(['status'], env); - - // Recovered: retried the same command against the same daemon instance. - expect(result.code).toBe(0); - expect(result.stdout).toContain(`RECOVERED ${BOOT_ID}`); - // The fork's CLI announces the busy retry on stderr; ours retries at the - // probe layer without a message. Either is fine — the load-bearing - // behavior is retry-without-kill, asserted below. - expect(daemon.commandRequests).toBe(2); // wedged once, served once - - // Never killed, never restarted — tab/cookie state intact. - expect(result.stderr).not.toContain('Restarting'); - expect(isProcessAlive(daemonPid)).toBe(true); - expect(JSON.parse(fs.readFileSync(stateFile, 'utf-8'))).toEqual(stateContent); - - await daemon.close(); - fs.rmSync(tmpDir, { recursive: true, force: true }); }, 30_000); }); diff --git a/browse/test/session-persist.test.ts b/browse/test/session-persist.test.ts index 6a4ab89dc..d9eaa5d9f 100644 --- a/browse/test/session-persist.test.ts +++ b/browse/test/session-persist.test.ts @@ -78,21 +78,27 @@ describe('session-persist units', () => { test('cookie filter drops malformed + internal-network domains', () => { const kept = filterSessionCookies([ { name: 'ok', value: 'v', domain: 'example.com' }, + { name: 'ok2', value: 'v', domain: '.example.com' }, // leading-dot public domain kept { name: 'bad1', value: 'v', domain: 'localhost' }, { name: 'bad2', value: 'v', domain: '.corp.internal' }, { name: 'bad3', value: 'v', domain: '169.254.169.254' }, - { name: 'bad4', value: 42, domain: 'example.com' }, + { name: 'bad4', value: 'v', domain: '169.254.1.2' }, // whole link-local block, not just metadata + { name: 'bad5', value: 'v', domain: '127.0.0.1' }, // IPv4 loopback literal + { name: 'bad6', value: 'v', domain: '.127.0.0.1' }, // leading-dot loopback variant + { name: 'bad7', value: 'v', domain: '::1' }, // IPv6 loopback + { name: 'bad8', value: 'v', domain: '[::1]' }, // bracketed IPv6 loopback + { name: 'bad9', value: 42, domain: 'example.com' }, null, ]); - expect(kept.map((c: any) => c.name)).toEqual(['ok']); + expect(kept.map((c: any) => c.name)).toEqual(['ok', 'ok2']); }); - test('restoreSessionState: missing file → false, corrupt file → quarantined to .corrupt', async () => { + test('restoreSessionState: missing file → null, corrupt file → quarantined to .corrupt', async () => { const bmNeverCalled = { closeAllPages() { throw new Error('must not restore'); } } as any; - expect(await restoreSessionState(bmNeverCalled, path.join(tmpRoot, 'nope.json'))).toBe(false); + expect(await restoreSessionState(bmNeverCalled, path.join(tmpRoot, 'nope.json'))).toBeNull(); const corrupt = path.join(tmpRoot, 'corrupt.json'); fs.writeFileSync(corrupt, '{oops'); - expect(await restoreSessionState(bmNeverCalled, corrupt)).toBe(false); + expect(await restoreSessionState(bmNeverCalled, corrupt)).toBeNull(); expect(fs.existsSync(corrupt)).toBe(false); // moved aside, won't block every future launch expect(fs.existsSync(`${corrupt}.corrupt`)).toBe(true); // forensic artifact kept (R3) }); @@ -106,10 +112,47 @@ describe('session-persist units', () => { await persistSessionState(bm, file); expect(fs.existsSync(file)).toBe(false); }); + + test('persist writes atomically: no .tmp left behind, file parses', async () => { + const file = path.join(tmpRoot, 'atomic.json'); + const state: BrowserState = { + cookies: [{ name: 'sid', value: 'abc', domain: 'example.com' } as any], + pages: [{ url: 'https://example.com', isActive: true, storage: null }], + }; + const bm = { getConnectionMode: () => 'launched', saveState: async () => state } as any; + await persistSessionState(bm, file); + expect(fs.existsSync(`${file}.tmp`)).toBe(false); // staged copy renamed away + const parsed = JSON.parse(fs.readFileSync(file, 'utf-8')); + expect(parsed.cookies[0].name).toBe('sid'); + }); + + test('a failed snapshot write preserves the previous good snapshot', async () => { + // chmod-based read-only dirs don't bind on Windows or when running as root. + if (process.platform === 'win32' || process.getuid?.() === 0) return; + const dir = path.join(tmpRoot, 'ro'); + fs.mkdirSync(dir); + const file = path.join(dir, 'session-state.json'); + const goodState: BrowserState = { + cookies: [], + pages: [{ url: 'https://good.example', isActive: true, storage: null }], + }; + const bm = { getConnectionMode: () => 'launched', saveState: async () => goodState } as any; + await persistSessionState(bm, file); + fs.chmodSync(dir, 0o500); // next .tmp write throws EACCES mid-persist + try { + await expect(persistSessionState(bm, file)).rejects.toThrow(); + // The crash-mid-write scenario the feature exists to survive: the + // previous good snapshot is untouched and still parses. + const parsed = JSON.parse(fs.readFileSync(file, 'utf-8')); + expect(parsed.pages[0].url).toBe('https://good.example'); + } finally { + fs.chmodSync(dir, 0o700); + } + }); }); describe('session-persist round-trip (real Chromium)', () => { - test('cookie + localStorage + URL survive teardown → relaunch', async () => { + test('cookie + localStorage + URL survive teardown → relaunch; loopback cookies dropped', async () => { const { BrowserManager } = await import('../src/browser-manager'); const { startTestServer } = await import('./test-server'); const { server, url } = startTestServer(0); @@ -120,8 +163,14 @@ describe('session-persist round-trip (real Chromium)', () => { try { const page = bm1.getPage(); await page.goto(`${url}/basic.html`, { waitUntil: 'domcontentloaded' }); + // Real-site cookie: set on the context for a non-loopback domain (the + // restore hygiene filter deliberately drops loopback/link-local + // domains, so a 127.0.0.1 test-server cookie can't stand in for it). + await page.context().addCookies([ + { name: 'session_marker', value: 'alive-after-restart', domain: 'example.com', path: '/' }, + ]); await page.evaluate(() => { - document.cookie = 'session_marker=alive-after-restart; path=/'; + document.cookie = 'loopback_marker=must-be-dropped; path=/'; // 127.0.0.1 host cookie localStorage.setItem('auth_marker', 'still-logged-in'); }); await persistSessionState(bm1, stateFile); @@ -137,15 +186,22 @@ describe('session-persist round-trip (real Chromium)', () => { const bm2 = new BrowserManager(); await bm2.launch(); try { - expect(await restoreSessionState(bm2, stateFile)).toBe(true); + const restored = await restoreSessionState(bm2, stateFile); + expect(restored).not.toBeNull(); + expect(restored!.pages.length).toBe(1); // counts derivable without a saveState() round-trip + // Hygiene filter applied at restore: the real-site cookie survives, + // the loopback cookie does not. + expect(restored!.cookies.map((c: any) => c.name)).toEqual(['session_marker']); const page = bm2.getPage(); expect(page.url()).toContain('/basic.html'); const marker = await page.evaluate(() => ({ cookie: document.cookie, auth: localStorage.getItem('auth_marker'), })); - expect(marker.cookie).toContain('session_marker=alive-after-restart'); + expect(marker.cookie).not.toContain('loopback_marker'); // dropped by isInternalCookieDomain expect(marker.auth).toBe('still-logged-in'); + const restoredCookies = await page.context().cookies('https://example.com'); + expect(restoredCookies.map((c) => `${c.name}=${c.value}`)).toContain('session_marker=alive-after-restart'); } finally { await bm2.close(); server.stop(true); @@ -162,6 +218,38 @@ describe('server wiring (static tripwire)', () => { expect(SERVER_SRC).toContain('sessionPersistIntervalMs()'); }); + test('start() restores in the background AFTER the port binds (CLI readiness must not wait)', () => { + // Restore re-creates tabs with up-to-15s goto timeouts; the CLI gives up + // at 8s. A restore that runs before Bun.serve() makes every $B command + // report "Server failed to start" on one slow saved URL. + const serveAt = SERVER_SRC.indexOf('const server = Bun.serve('); + const restoreAt = SERVER_SRC.indexOf('restoreSessionState(browserManager'); + expect(serveAt).toBeGreaterThan(-1); + expect(restoreAt).toBeGreaterThan(serveAt); + }); + + test('interval snapshots carry an in-flight guard (no overlapping persists)', () => { + expect(SERVER_SRC).toContain('persistInFlight'); + }); + + test('interval ticks are gated on isShuttingDown (belt half of the shutdown ordering fix)', () => { + // A tick that fires during browser teardown snapshots a degraded state + // (zero tabs) over the good final snapshot. The handle-clear in shutdown() + // is the suspenders; this gate is the belt for a tick already scheduled. + const tickerAt = SERVER_SRC.indexOf('sessionPersistInterval = setInterval('); + expect(tickerAt).toBeGreaterThan(-1); + const tickerBlock = SERVER_SRC.slice(tickerAt, tickerAt + 500); + expect(tickerBlock).toContain('if (isShuttingDown) return;'); + }); + + test('shutdown() clears the persist ticker BEFORE the final snapshot (suspenders half)', () => { + const shutdownStart = SERVER_SRC.indexOf('async function shutdown('); + const clearAt = SERVER_SRC.indexOf('clearInterval(sessionPersistInterval)', shutdownStart); + const persistAt = SERVER_SRC.indexOf('persistSessionState(cfgBrowserManager', shutdownStart); + expect(clearAt).toBeGreaterThan(shutdownStart); + expect(persistAt).toBeGreaterThan(clearAt); + }); + test('shutdown() takes a final snapshot BEFORE closing the browser', () => { const shutdownStart = SERVER_SRC.indexOf('async function shutdown('); const persistAt = SERVER_SRC.indexOf('persistSessionState(cfgBrowserManager', shutdownStart); @@ -169,4 +257,12 @@ describe('server wiring (static tripwire)', () => { expect(persistAt).toBeGreaterThan(shutdownStart); expect(closeAt).toBeGreaterThan(persistAt); }); + + test('shutdown() snapshot is deadlined — a wedged page.evaluate cannot hang shutdown', () => { + const shutdownStart = SERVER_SRC.indexOf('async function shutdown('); + const closeAt = SERVER_SRC.indexOf('await cfgBrowserManager.close()', shutdownStart); + const raceAt = SERVER_SRC.indexOf('Promise.race', shutdownStart); + expect(raceAt).toBeGreaterThan(shutdownStart); + expect(raceAt).toBeLessThan(closeAt); + }); });