fix(browse): session snapshots are atomic and the cookie filter drops loopback IP literals

A crash mid-write destroyed the previous good snapshot — the exact
scenario persistence exists to survive; writes now go tmp+rename. The
internal-network cookie filter gains 127.*/::1/169.254.* (a tampered
state file could previously hand loopback-service cookies back to the
browser), and 'state load' imports the shared filter instead of
maintaining a comment-synced copy. Test cleanup made exception-safe.

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
+        // Validate and filter cookies via the shared hygiene filter in
-        const validatedCookies = data.cookies.filter((c: any) => {
+        // session-persist.ts (isInternalCookieDomain): rejects malformed
-          if (typeof c !== 'object' || !c) return false;
+        // cookies and internal-network domains — localhost, *.internal,
-          if (typeof c.name !== 'string' || typeof c.value !== 'string') return false;
+        // loopback literals (127.x, ::1), and link-local/cloud-metadata
-          if (typeof c.domain !== 'string' || !c.domain) return false;
+        // (169.254.x) — that a tampered state file could use to reach local
-          const d = c.domain.startsWith('.') ? c.domain.slice(1) : c.domain;
+        // services or the metadata endpoint.
-          if (d === 'localhost' || d.endsWith('.internal') || d === '169.254.169.254') return false;
+        const validatedCookies = filterSessionCookies(data.cookies);
-          return true;
-        });
         if (validatedCookies.length < data.cookies.length) {
           console.warn(`[browse] Filtered ${data.cookies.length - validatedCookies.length} invalid cookies from state file`);
         }

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
+ * True when a cookie domain points at an internal-network target a tampered
- * comment there): drop malformed cookies and internal-network domains a
+ * state file could use to reach localhost services, *.internal hosts, or
- * tampered file could use to reach localhost services or cloud metadata.
+ * 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;
+    return !isInternalCookieDomain(c.domain);
-    if (d === 'localhost' || d.endsWith('.internal') || d === '169.254.169.254') return false;
-    return true;
   }) as BrowserState['cookies'];
 }
 
@@ -118,21 +134,33 @@ export function deserializeSessionState(raw: string): BrowserState | null {
 export async function persistSessionState(bm: BrowserManager, filePath: string): Promise<void> {
   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
+ * Restore a persisted session into a freshly launched manager. Returns the
- * when state was restored, false when there was nothing (or corrupt data —
+ * restored (already-filtered) state so callers can log counts without an
- * which is warned, deleted, and skipped rather than blocking launch).
+ * extra saveState() round-trip, or null when there was nothing to restore
- * restoreState re-validates every URL before navigating.
+ * (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<boolean> {
+export async function restoreSessionState(bm: BrowserManager, filePath: string): Promise<BrowserState | null> {
   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;
 }

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
+      const stateContent = {
-    // dead path it SIGTERMs this child — the aliveness assert catches it.
+        pid: daemonPid,
-    daemonPidChild = spawn('sleep', ['60'], { stdio: 'ignore' });
+        port: daemon.port,
-    const daemonPid = daemonPidChild.pid!;
+        token: 'busy-test-token',
+        startedAt: new Date().toISOString(),
+        serverPath: '',
+        mode: 'launched' as const,
+      };
+      fs.writeFileSync(stateFile, JSON.stringify(stateContent, null, 2));
 
-    const stateContent = {
+      const env: Record<string, string> = {};
-      pid: daemonPid,
+      for (const [k, v] of Object.entries(process.env)) {
-      port: daemon.port,
+        if (v !== undefined) env[k] = v;
-      token: 'busy-test-token',
+      }
-      startedAt: new Date().toISOString(),
+      env.BROWSE_STATE_FILE = stateFile;
-      serverPath: '',
-      mode: 'launched' as const,
-    };
-    fs.writeFileSync(stateFile, JSON.stringify(stateContent, null, 2));
 
-    const env: Record<string, string> = {};
+      const result = await runCli(['status'], env);
-    for (const [k, v] of Object.entries(process.env)) {
+
-      if (v !== undefined) env[k] = v;
+      // 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);
 });

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);
+  });
 });