From 30c3047b6267ec17c6fb3ab26fbc0272e5f8315b Mon Sep 17 00:00:00 2001 From: DrBanks82 Date: Fri, 11 Sep 2026 21:03:25 -0400 Subject: [PATCH] fix(browse): state save|load carries localStorage, so a saved login restores MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `browse state save ` wrote cookies and URLs only. Every token-in-localStorage login — Supabase, Firebase, most SPA auth — was therefore unrestorable: `state load` replayed hundreds of cookies, printed a success line, and handed back a signed-OUT browser. Nothing errored. The failure surfaced later as an unexplained redirect to a sign-in page, which reads as an expired session rather than a save that never captured the session at all. The omission was deliberate, behind the comment "not localStorage — breaks on load-before-navigate". That reasoning is now stale: BrowserManager.restoreState navigates each tab to its saved URL FIRST and applies storage after, so there is no load-before-navigate window. The sibling persistence path (session-persist.ts, #778) already persists per-tab storage through that same restore; only the manual named-state path was left behind. - meta-commands.ts: `state save` persists per-tab `storage`; `state load` restores it instead of hardcoding `storage: null`. Both success lines now report the localStorage key count, so "0 localStorage keys" is a visible tell that a file cannot restore a login rather than a silent redirect later. The plaintext warning now says the file can contain auth tokens, because it now can. - session-persist.ts: extracted `sanitizeTabStorage()` and pointed `deserializeSessionState` at it, so the persistence restore path and `state load` validate the same way — the single-source-of-truth rule that file already applies to `isInternalCookieDomain`/`filterSessionCookies`. String keys and string values only: restore hands this to `localStorage.setItem` inside `page.evaluate`, so a tampered file must not get a non-string coerced in. Pre-fix state files have no `storage` key and load exactly as before. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01D4gvxB6ytnEuPkpVBtK2bg --- browse/src/meta-commands.ts | 35 +++- browse/src/session-persist.ts | 32 +++- browse/test/state-save-localstorage.test.ts | 167 ++++++++++++++++++++ 3 files changed, 222 insertions(+), 12 deletions(-) create mode 100644 browse/test/state-save-localstorage.test.ts diff --git a/browse/src/meta-commands.ts b/browse/src/meta-commands.ts index a2bc1f4d2e..fdad3f3b96 100644 --- a/browse/src/meta-commands.ts +++ b/browse/src/meta-commands.ts @@ -20,7 +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 { filterSessionCookies, sanitizeTabStorage } from './session-persist'; import type { Frame } from 'playwright'; /** Tokenize a pipe segment respecting double-quoted strings. */ @@ -916,15 +916,27 @@ export async function handleMetaCommand( if (action === 'save') { const state = await bm.saveState(); - // V1: cookies + URLs only (not localStorage — breaks on load-before-navigate) + // cookies + per-tab url/isActive/storage — the same v1 shape + // serializeSessionState writes (session-persist.ts). loadedHtml, + // loadedHtmlWaitUntil and owner stay in-memory-only. + // + // localStorage IS saved. It used to be dropped, on the reasoning that + // storage cannot be written before navigating to its origin — true, but + // restoreState already navigates each tab to its saved URL FIRST and + // applies storage after, so the ordering problem does not arise. Dropping + // it silently broke every token-in-localStorage login (Supabase, Firebase, + // most SPA auth): `state load` restored 400 cookies, reported success, and + // handed back a signed-OUT browser with no error anywhere. const saveData = { version: 1, savedAt: new Date().toISOString(), cookies: state.cookies, - pages: state.pages.map(p => ({ url: p.url, isActive: p.isActive })), + pages: state.pages.map(p => ({ url: p.url, isActive: p.isActive, storage: p.storage })), }; writeSecureFile(statePath, JSON.stringify(saveData, null, 2)); - return `State saved: ${statePath} (${state.cookies.length} cookies, ${state.pages.length} pages)\n⚠️ Cookies stored in plaintext. Delete when no longer needed.`; + const localKeys = state.pages.reduce( + (n, p) => n + Object.keys(p.storage?.localStorage ?? {}).length, 0); + return `State saved: ${statePath} (${state.cookies.length} cookies, ${state.pages.length} pages, ${localKeys} localStorage keys)\n⚠️ Cookies and localStorage stored in plaintext — this file can contain auth tokens. Delete when no longer needed.`; } if (action === 'load') { @@ -963,10 +975,21 @@ export async function handleMetaCommand( pages: data.pages.map((p: any) => ({ url: typeof p.url === 'string' ? p.url : '', isActive: Boolean(p.isActive), - storage: null, + // Storage goes through the same validator as the persistence restore + // path (sanitizeTabStorage): string keys and string values only, so a + // tampered file cannot smuggle a non-string into localStorage.setItem. + // Files written before storage was saved have no `storage` key and + // restore exactly as they did — cookies only. + storage: sanitizeTabStorage(p.storage), })), }); - return `State loaded: ${data.cookies.length} cookies, ${data.pages.length} pages`; + const loadedKeys = data.pages.reduce( + (n: number, p: any) => n + Object.keys(sanitizeTabStorage(p.storage)?.localStorage ?? {}).length, 0); + // Name the localStorage count: a "0 localStorage keys" line is the visible + // tell that this file predates storage capture and cannot restore a + // token-based login, instead of that failure showing up later as an + // unexplained redirect to a sign-in page. + return `State loaded: ${data.cookies.length} cookies, ${data.pages.length} pages, ${loadedKeys} localStorage keys`; } throw new Error('Usage: state save|load '); diff --git a/browse/src/session-persist.ts b/browse/src/session-persist.ts index 3c6627bcbc..12ab8f002b 100644 --- a/browse/src/session-persist.ts +++ b/browse/src/session-persist.ts @@ -97,6 +97,31 @@ export function filterSessionCookies(cookies: unknown[]): BrowserState['cookies' }) as BrowserState['cookies']; } +/** + * Validate one tab's storage blob off disk. Shared by the persistence restore + * path here AND `state load` (meta-commands.ts) — same single-source-of-truth + * rule as isInternalCookieDomain/filterSessionCookies above. + * + * localStorage is what carries token-based auth (Supabase, Firebase, most SPA + * auth keeps its session there, not in a cookie), so dropping it is the + * difference between a restored login and a state file that reports success + * and hands back a signed-out browser. Values must be strings: Playwright's + * page.evaluate serializes them straight into localStorage.setItem, and a + * non-string from a tampered file would be coerced rather than rejected. + */ +export function sanitizeTabStorage(raw: any): BrowserState['pages'][number]['storage'] { + if (!raw || typeof raw !== 'object') return null; + const pick = (v: any): Record => { + if (!v || typeof v !== 'object') return {}; + const out: Record = {}; + for (const [k, val] of Object.entries(v)) { + if (typeof k === 'string' && typeof val === 'string') out[k] = val; + } + return out; + }; + return { localStorage: pick(raw.localStorage), sessionStorage: pick(raw.sessionStorage) }; +} + /** * Parse + validate the on-disk shape into a BrowserState. Returns null for * anything malformed (corrupt JSON, wrong version, missing arrays). @@ -116,12 +141,7 @@ export function deserializeSessionState(raw: string): BrowserState | null { pages: data.pages.map((p: any) => ({ url: typeof p?.url === 'string' ? p.url : '', isActive: Boolean(p?.isActive), - storage: p?.storage && typeof p.storage === 'object' - ? { - localStorage: typeof p.storage.localStorage === 'object' && p.storage.localStorage ? p.storage.localStorage : {}, - sessionStorage: typeof p.storage.sessionStorage === 'object' && p.storage.sessionStorage ? p.storage.sessionStorage : {}, - } - : null, + storage: sanitizeTabStorage(p?.storage), // NEVER accept loadedHtml / loadedHtmlWaitUntil / owner from disk. })), }; diff --git a/browse/test/state-save-localstorage.test.ts b/browse/test/state-save-localstorage.test.ts new file mode 100644 index 0000000000..a302df6c6a --- /dev/null +++ b/browse/test/state-save-localstorage.test.ts @@ -0,0 +1,167 @@ +/** + * `state save|load` must carry localStorage (#778 follow-on). + * + * #778 ("auth state is lost across separate invocations") was closed by the + * opt-in session-persistence path, which does persist per-tab storage. The + * MANUAL path — `browse state save ` / `state load ` — kept + * writing cookies and URLs only, behind a comment reading "not localStorage — + * breaks on load-before-navigate". + * + * That reasoning no longer holds: BrowserManager.restoreState navigates each + * tab to its saved URL FIRST and applies storage after, so there is no + * load-before-navigate window. Meanwhile the omission silently broke every + * token-in-localStorage login — Supabase, Firebase, most SPA auth keeps its + * session there, not in a cookie. `state load` restored hundreds of cookies, + * printed a success line, and handed back a signed-OUT browser with no error + * anywhere. The failure surfaced much later as an unexplained redirect to a + * sign-in page. + * + * Suites: + * 1. sanitizeTabStorage units — the shared disk-shape validator. + * 2. Real-Chromium round-trip through handleMetaCommand: localStorage + * written before `state save` is readable after `state load`. + * 3. Backward compatibility: a pre-fix file (no `storage` key) still loads. + * + * These fail on the old tree: suite 1's module export is absent, and suites + * 2/3 get `null` storage back. + */ + +import { afterAll, beforeAll, describe, expect, test } from 'bun:test'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { sanitizeTabStorage } from '../src/session-persist'; + +// Per-FILE Chromium profile, for the same reason session-persist.test.ts +// isolates one: sharing a profile dir with a sibling file's daemon kills one +// side's Chromium via ProcessSingleton on user-data-dir. +const ORIGINAL_CHROMIUM_PROFILE = process.env.CHROMIUM_PROFILE; +const ORIGINAL_STATE_FILE = process.env.BROWSE_STATE_FILE; +let CHROMIUM_PROFILE_DIR: string | undefined; +let tmpRoot: string; + +beforeAll(() => { + CHROMIUM_PROFILE_DIR = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-state-ls-profile-')); + process.env.CHROMIUM_PROFILE = CHROMIUM_PROFILE_DIR; + tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-state-ls-')); + // resolveConfig() derives stateDir from BROWSE_STATE_FILE's parent, so this + // keeps `state save` inside the tmp tree instead of the real ~/.gstack. + process.env.BROWSE_STATE_FILE = path.join(tmpRoot, 'browse.json'); +}); + +afterAll(() => { + if (ORIGINAL_CHROMIUM_PROFILE === undefined) delete process.env.CHROMIUM_PROFILE; + else process.env.CHROMIUM_PROFILE = ORIGINAL_CHROMIUM_PROFILE; + if (ORIGINAL_STATE_FILE === undefined) delete process.env.BROWSE_STATE_FILE; + else process.env.BROWSE_STATE_FILE = ORIGINAL_STATE_FILE; + if (CHROMIUM_PROFILE_DIR) { try { fs.rmSync(CHROMIUM_PROFILE_DIR, { recursive: true, force: true }); } catch {} } + if (tmpRoot) { try { fs.rmSync(tmpRoot, { recursive: true, force: true }); } catch {} } +}); + +describe('sanitizeTabStorage (units)', () => { + test('null for anything that is not an object', () => { + expect(sanitizeTabStorage(null)).toBeNull(); + expect(sanitizeTabStorage(undefined)).toBeNull(); + expect(sanitizeTabStorage('nope')).toBeNull(); + expect(sanitizeTabStorage(42)).toBeNull(); + }); + + test('keeps string entries in both stores', () => { + const out = sanitizeTabStorage({ + localStorage: { 'sb-proj-auth-token': '{"access_token":"x"}' }, + sessionStorage: { tab: '1' }, + }); + expect(out!.localStorage['sb-proj-auth-token']).toBe('{"access_token":"x"}'); + expect(out!.sessionStorage.tab).toBe('1'); + }); + + test('drops non-string values rather than coercing them', () => { + // The restore path hands this straight to localStorage.setItem inside + // page.evaluate. A tampered file must not get an object or a function + // stringified into the page's storage. + const out = sanitizeTabStorage({ + localStorage: { good: 'v', num: 1, obj: { a: 1 }, nil: null }, + sessionStorage: null, + }); + expect(Object.keys(out!.localStorage)).toEqual(['good']); + expect(out!.sessionStorage).toEqual({}); + }); + + test('a missing store becomes an empty object, never undefined', () => { + const out = sanitizeTabStorage({}); + expect(out).toEqual({ localStorage: {}, sessionStorage: {} }); + }); +}); + +describe('state save|load round-trip (real Chromium)', () => { + test('localStorage written before save is readable after load', async () => { + const { BrowserManager } = await import('../src/browser-manager'); + const { handleMetaCommand } = await import('../src/meta-commands'); + const { startTestServer } = await import('./test-server'); + const { server, url } = startTestServer(0); + const noop = async () => {}; + + const bm1 = new BrowserManager(); + await bm1.launch(); + try { + const page = bm1.getPage(); + await page.goto(`${url}/basic.html`, { waitUntil: 'domcontentloaded' }); + await page.evaluate(() => { + localStorage.setItem('auth_marker', 'still-logged-in'); + }); + const saved = await handleMetaCommand('state', ['save', 'rt'], bm1, noop); + // The success line names the localStorage count: a "0 localStorage keys" + // reading is the visible tell that a login cannot be restored from this + // file, instead of that showing up later as a sign-in redirect. + expect(saved).toContain('1 localStorage keys'); + } finally { + await bm1.close(); + } + + const statePath = path.join(tmpRoot, 'browse-states', 'rt.json'); + const onDisk = JSON.parse(fs.readFileSync(statePath, 'utf-8')); + expect(onDisk.pages[0].storage.localStorage.auth_marker).toBe('still-logged-in'); + if (process.platform !== 'win32') { + // The file now carries auth tokens, not just cookies. Owner-only matters more. + expect(fs.statSync(statePath).mode & 0o777).toBe(0o600); + } + + const bm2 = new BrowserManager(); + await bm2.launch(); + try { + const loaded = await handleMetaCommand('state', ['load', 'rt'], bm2, noop); + expect(loaded).toContain('1 localStorage keys'); + const page = bm2.getPage(); + const marker = await page.evaluate(() => localStorage.getItem('auth_marker')); + expect(marker).toBe('still-logged-in'); + } finally { + await bm2.close(); + server.stop(true); + } + }, 60_000); + + test('a pre-fix state file with no storage key still loads', async () => { + const { BrowserManager } = await import('../src/browser-manager'); + const { handleMetaCommand } = await import('../src/meta-commands'); + const noop = async () => {}; + + // Exactly what `state save` wrote before this change: no `storage` anywhere. + const dir = path.join(tmpRoot, 'browse-states'); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, 'oldshape.json'), JSON.stringify({ + version: 1, + savedAt: new Date().toISOString(), + cookies: [], + pages: [{ url: '', isActive: true }], + })); + + const bm = new BrowserManager(); + await bm.launch(); + try { + const loaded = await handleMetaCommand('state', ['load', 'oldshape'], bm, noop); + expect(loaded).toContain('0 localStorage keys'); + } finally { + await bm.close(); + } + }, 60_000); +});