Store the token fallback under its own key so it can actually be read - #34868
Draft
hughns wants to merge 6 commits into
Draft
Store the token fallback under its own key so it can actually be read#34868hughns wants to merge 6 commits into
hughns wants to merge 6 commits into
Conversation
When an IndexedDB write failed, persistTokenInStorage fell back to writing the token to localStorage under the primary storage key. But getStoredToken reads IndexedDB first and only consults localStorage when IndexedDB is empty - which, for a rotation, it never is. The fallback was therefore unreachable: the stale IndexedDB value shadowed it permanently. With rotating refresh tokens this is fatal and silent. A single failed idbSave leaves the client presenting an already-consumed refresh token on its next start, the server rejects it with a 4xx, and the session is destroyed. Write the fallback to a distinct `<key>_fallback` key instead, and prefer it on read. It is only ever written when an IndexedDB write failed, and is cleared as soon as one succeeds, so it is always at least as new as IndexedDB. The primary key in localStorage keeps its existing meaning - a pre-IndexedDB legacy token, which may well be older than IndexedDB - so the migration path is unchanged and the change is safe on upgrade. Also: - log the write failure. This was silent, so the moment a session became doomed left no trace in rageshakes at all. - clear any stale IndexedDB entry after a failed write, so a client that does not know about the fallback key reads "no token" rather than an outdated one. - write the fallback before clearing IndexedDB, so there is never a moment where neither store holds a token. - deep-copy the store in the tests' initIdbMock. Several tests share one fixture object, so the new idbDelete leaked deletions between tests. Part of #34866 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Before the previous commit, a failed IndexedDB write put the token into localStorage under the *primary* storage key. Those values are now unreachable: getStoredToken prefers the dedicated fallback key, and otherwise reads IndexedDB, only consulting the primary key when IndexedDB is empty. So an affected profile is left with a token sitting in localStorage in plain text, which nothing will ever read or remove - which is precisely what the pickle key exists to avoid. Previously the legacy migration path would eventually sweep these up; it no longer gets the chance. Remove it once IndexedDB has answered, and log the fact. We deliberately do not *use* it: the old code never cleared it after a later successful write, so it may be older than the IndexedDB copy, and preferring it could demote a working session to a dead token - the exact failure the previous commit fixes. The warning also acts as a marker for profiles which hit the old bug, so prevalence can be measured from rageshakes rather than guessed at. Part of #34866 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Write the fallback *before* touching IndexedDB below, so that there is never a moment | ||
| // where neither store holds a token. | ||
| if (token) { | ||
| localStorage.setItem(fallbackStorageKey, token); |
Member
Author
There was a problem hiding this comment.
The behaviour to store the token in plaintext is existing.
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
2 participants
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.
Fixes #34866
When an IndexedDB write failed,
persistTokenInStorage()fell back to writing the token to localStorage under the primary storage key. But getStoredToken reads IndexedDB first and only consults localStorage when the IndexedDB returns no value. The fallback was therefore sometimes unreachable: the stale IndexedDB value shadowed it permanently.This PR writes the fallback to a distinct
<key>_fallbacklocalStorage key instead, and prefers it on read. It is only ever written when an IndexedDB write failed, and is cleared assoon as one succeeds, so it is always at least as new as IndexedDB. The primary key in localStorage keeps its existing meaning - a pre-IndexedDB legacy token, which may well be older than IndexedDB - so the migration path is unchanged and the change is safe on upgrade.
Also:
Checklist
public/exportedsymbols have accurate TSDoc documentation.