Conversation
…estores `browse state save <name>` 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, garrytan#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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D4gvxB6ytnEuPkpVBtK2bg
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
Author
|
Closing — this was scope drift on my end. The fix is running locally and upstreaming it isn't worth the maintainer's review time. Details stay in the thread if anyone hits the same thing: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why (in your own words)
browse state save <name>saves cookies and URLs but not localStorage. That makes it useless for the auth it looks like it handles: Supabase, Firebase and most SPA auth keep the session in localStorage, not in a cookie. Sostate loadreplays hundreds of cookies, prints a success line, and hands back a signed-out browser. Nothing errors.I hit this during real work tonight. A saved operator session wouldn't restore, and the symptom — landing on
/auth— looks exactly like an expired token, so I went down the wrong path twice: first "the state file is stale, re-save it" (re-saved from a live signed-in daemon, same result), then "a stuck Chromium is holding the profile, kill it and sign in again". The second one was about to throw away a working signed-in browser to fix a save that was never going to work. The actual cause only showed up when I dumped the file and found nooriginskey at all.The omission was deliberate, behind
// V1: cookies + URLs only (not localStorage — breaks on load-before-navigate). That reasoning is stale:BrowserManager.restoreStatenavigates each tab to its saved URL first and applies storage after, so there's 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.Live evidence
Before — the file has no storage at all:
Before — save from a live signed-in daemon, load into a fresh one, still signed out:
After — localStorage round-trips between two separate daemons:
After — a pre-fix file (no
storagekey) still loads, and now says so:That
0 localStorage keysis the second half of the fix. It's the visible tell that a given file cannot restore a login, instead of that fact surfacing later as an unexplained sign-in redirect.Tests — red on the old tree for the right reason, then green:
Scope
browse/src/meta-commands.ts—state savepersists per-tabstorage;state loadrestores it instead of hardcodingstorage: null; both success lines report the localStorage key count; the plaintext warning now says the file can contain auth tokens, because it now can.browse/src/session-persist.ts— extractedsanitizeTabStorage()and pointeddeserializeSessionStateat it, so the persistence restore path andstate loadvalidate identically. String keys and string values only: restore hands this tolocalStorage.setIteminsidepage.evaluate, so a tampered file must not get a non-string coerced in. Same single-source-of-truth rule the file already applies toisInternalCookieDomain/filterSessionCookies.browse/test/state-save-localstorage.test.ts— new. Units for the validator, a real-Chromium round trip throughhandleMetaCommand, and a backward-compat case.meta-commands.tsreverted;session-persist.test.tsstill green.session-persist.test.ts);sessionStorageend-to-end — it rides the same field and validator as localStorage and is covered by units, but I only exercised localStorage against a real login; headed mode, which keeps its own persistent Chromium profile and never used this path.Liveness proof (required)
Pending — the repo owner will attach it. This PR was prepared by Claude Code in a session driven by @DrBanks82; I'm not going to fabricate a screenshot whose stated purpose is confirming a human is behind the PR. Opening as a draft until it's attached.
Checklist
GSTACK PRtyped live into a real surface (not edited onto the image) — pending, see abovestate save|load. Full reproduction in Live evidence above.🤖 Generated with Claude Code
https://claude.ai/code/session_01D4gvxB6ytnEuPkpVBtK2bg