fix(auth): revoke explorer credentials even when the epoch write fails - #1660
Closed
dawsontoth wants to merge 7 commits into
Closed
fix(auth): revoke explorer credentials even when the epoch write fails#1660dawsontoth wants to merge 7 commits into
dawsontoth wants to merge 7 commits into
Conversation
…er render The login form dropped the typed credentials from a `useEffect` keyed on a `pending` -> `idle` status transition. React flushes passive effects in a Scheduler task *after* the commit they belong to, and `shouldYieldToHost()` splits the two into separate macrotasks whenever a commit blows the 5ms frame budget. In that ordering the clear arrives a commit late: a user who starts typing the next credentials as soon as the panel says "Credential set" has the username wiped out from under them, which disables the submit button on an empty field. `runMint` now reports how it finished (`applied` / `discarded` / `failed`) and the form clears off that promise, so the clear batches into the same commit as the status and credential updates. Only `applied` clears: a `discarded` mint no longer owns the form, and re-picking the auth type mid-flight supersedes a mint without unmounting it, so clearing there would blank credentials the user had already retyped -- the same defect one race narrower. The one clear that *should* beat live typing is a sign-out, which the deleted effect covered incidentally (revocation drove status back to `idle`). A revocation counter now keys the form instead, so typed credentials cannot outlive the session they were meant for whether or not a mint was in flight. The counter advances only on a newly-observed epoch for this entity: `onExplorerAuthInvalidated` names no entity and fires on any sign-out, so an unconditional bump would blank a form belonging to an instance nobody signed out of. Guards are mutation-checked, none by timeout: restoring the effect fails the re-auth test in 64ms on `expected 'dave' to be ''`; widening the clear to `discarded` fails the superseded-mint test on `expected '' to be 'bob'`; dropping the revocation key fails the sign-out test on `expected 'alice' to be ''`; bumping revocation unconditionally fails the cross-entity test on `expected '' to be 'alice'`. Verified: 20 consecutive full-suite runs green (it reproduced twice in 12 runs before this). Refs #1655 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…render `useRef(authStore.getExplorerAuthEpoch(entityId))` evaluates its initializer on every render, not just the first, and reading the epoch does a `localStorage.getItem` plus a `JSON.parse`. `useResizableSidebar` keeps the sidebar width in `useState` and calls `setWidth` from `mousemove`, so a sidebar drag re-renders `ApiExplorer` continuously -- putting that parse on exactly the path whose own comment says writing localStorage per `mousemove` "would stutter". The effect below already baselines the ref on mount and on an `entityId` change, which is the only point the value is read, so the eager initializer was redundant as well as costly. That baseline is load-bearing and was untested: an entity that mounts on a non-zero epoch would otherwise read the next unrelated sign-out as a change to itself and blank the form. Covered now, and mutation-checked -- dropping the baseline fails the new test on `expected '' to be 'alice'`. Refs #1655 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three review findings on #1656, all sharing one cause: the explorer's sign-out signal names no entity, so every explorer acted on every sign-out. `onExplorerAuthInvalidated` now compares the epoch for the `id` it was given — the parameter existed but was ignored (`_id`) — so an unrelated instance's sign-out no longer increments this entity's `attemptRef`, which was silently discarding its in-flight mint. It also notifies in-process subscribers on a same-tab write, which fires no `storage` event of its own, so a same-tab sign-out now empties the form directly rather than waiting for the cancelled mint to settle. The mint's epoch-mismatch branch no longer bumps revocation: the subscription owns that now, and a second bump would remount the form again and take any credentials retyped since with it. `credentialIsCurrent` re-read the epoch during render, so the localStorage parse was still on the sidebar-resize path that 81a642f claimed to clear -- that commit removed a `useRef` initializer and missed this one. The epoch is held in state and advanced by the subscription instead. An `applied` mint no longer clears unconditionally. The inputs stay live while a mint runs, so a user can submit Alice's credentials and start on Bob's before a slow mint returns; clearing on Alice's success erased a draft it never saw. The form counts edits and compares at settle, so the clear stays tied to the values actually submitted. Each guard is mutation-checked: ignoring the edit generation fails on `expected '' to be 'bob'`; dropping the store's epoch comparison fails three unrelated-entity tests; dropping the same-tab notification fails on `expected 'alice' to be ''`. Refs #1655 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`onExplorerAuthInvalidated` read its baseline epoch, then registered its listener. A sign-out dispatched between the two reached nobody -- the event was gone, and no later one was coming -- so the subscriber sat on a revoked generation it believed was current. `ApiExplorer` had the same shape one level up, seeding its cached epoch before subscribing. Both now re-check once subscribed. The store compares against its baseline after registering, which is a no-op unless the gap actually swallowed an event, and the explorer reads its epoch after the subscription is live. Reordering the baseline to after `addEventListener` was the first attempt and is worse: `fire` closes over it, so an event arriving in that window throws a TDZ `ReferenceError` rather than being missed quietly. The new store test caught that, which is the reason it exists. Covers the store's subscription directly for the first time -- entity scoping, the global-logout wildcard, unsubscribe, and the race above. The race test stages the sign-out *before* `addEventListener` registers, because staging it after lets the ordinary event path satisfy it: written the obvious way it passes against the unfixed code, and only fails on `expected 1 times, got 0` in this order. Refs #1655 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The explorer's sign-out generation lives only in localStorage, so when the write throws -- storage disabled by policy, or full -- the generation never moves. Every later comparison then reads the signed-out credential as current: `credentialIsCurrent` matches, an in-flight mint's epoch check sees no change and writes its token back, and the subscription installed for sign-outs sees no change either, so nothing clears the form. The credential stays usable for the life of the tab, which is the one case where the durable record is exactly what cannot be relied on. `authStore` now keeps a per-entity count of sign-outs it could not record, and adds it to the epoch. It advances only in the `catch`, so nothing changes on the normal path. Being memory-only it is lost on reload, which fails safe: a credential stamped under a fallback generation compares as stale afterwards and is withheld rather than resurrected. Found by a cross-model review round on #1656 and deliberately left out of it -- that PR fixes a flaky test and the races around it, and this needed a change to the epoch's shape rather than to its callers. Mutation-checked both ways: dropping the `catch` bump and dropping the fallback from the epoch sum each fail the new test on `expected +0 to be 1`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces an in-memory fallback mechanism (explorerEpochFallback) in AuthStore to ensure that explorer credentials are still revoked within the current tab when a durable write to localStorage fails (e.g., due to storage being disabled or full). A corresponding unit test has been added to verify this behavior. The review feedback suggests initializing the fallback record with Object.create(null) instead of {} to prevent potential issues with dynamic keys matching built-in prototype properties.
Base automatically changed from
claude/apiexplorer-flaky-test-login-9ce001
to
stage
August 28, 2026 15:21
Recording the revocation in memory is not enough on its own. `signOutAllLocally` bumps the global epoch precisely for entities absent from `potentiallyAuthenticated`, which are therefore never reached by `forgetEntitySettings` -- so when the durable write also fails, the sequence is: the fallback count withholds the credential, the tab reloads, the count is gone, the durable generation never moved, and the stamped `authGeneration` matches again. The signed-out credential comes back and is sent. So drop the credential outright rather than only noting that it was revoked. sessionStorage is a separate store and is usually still writable when localStorage is not, and a deleted credential outlives the reload that loses the count. This corrects the claim in the previous commit's message: the memory-only fallback is fail-safe on the paths that purge alongside the bump, not on the global-logout path, which is the one it was most needed for. Also switches the fallback map to a null prototype, per review: unlike the durable map beside it, which type-guards every read, this one used `?? 0` -- which an inherited `toString` would sail straight through. Mutation-checked: dropping the purge leaves the token in sessionStorage and fails the new test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#1656 merged by rebase, so stage carries rebased copies of this branch's parent commits. Both conflicts are that duplication: stage holds the #1656 versions of `authStore.ts` and the explorer-invalidation test, this branch holds the same content plus the epoch fallback and its coverage. Verified `git diff stage` over both files is exactly this branch's additions and nothing of stage's is lost, so each was resolved to this branch's side. Also corrects the `explorerEpochFallback` doc comment, which still claimed the memory-only count fails safe across a reload -- the claim the previous commit disproved and replaced with destroying the credential outright. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Contributor
Author
|
Closing unmerged — the premise is wrong. Filed the real bug as #1662 (P1). A cross-model round on It is worse one level up: that throw propagates out of Three things worth recording, since they are why this looked finished:
The branch is left in place if any of it is useful to #1662, but it should not be merged as-is. |
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
1 participant
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.
Follow-up to #1656, now merged;
stagehas been merged in, so the diff here is this branch's own work.A cross-model round on #1656 surfaced this and I deliberately left it out: that PR fixes a flaky test and the ordering races around it, and this needs a change to the shape of the explorer's sign-out generation rather than to its callers.
The problem
The generation lives only in
localStorage. When the write throws — storage disabled by policy, or full —writeExplorerInvalidationswallows it and the generation never moves. Everything downstream compares against a number that cannot change, so a sign-out silently does nothing:credentialIsCurrentkeeps matching, so the credential is still attached to "Try it out" requests.The credential stays usable for the life of the tab. It is the one situation where the durable record is exactly what can't be relied on, and it was the only thing consulted.
Pre-existing — the swallow predates #1656 — but #1656 made the subscription depend on that comparison too, so it is worth closing now rather than later.
The change
authStorekeeps a per-entity count of sign-outs it could not record and adds it to the epoch:It advances only inside the
catch, so the normal path is byte-for-byte unchanged.The count is memory-only, and on its own that is not enough — which is worth spelling out, because the first version of this PR claimed it was.
A credential stamped under a fallback generation does not compare as stale after a reload; it compares as current again, because the count is gone and the durable generation never moved.
pruneStaleEntitySettingscompares that same restored epoch at bootstrap, so it does not strip it either. The signed-out credential comes back and is sent.Three of the four epoch-bump sites happen to be preceded by
forgetEntitySettings, which hides this. The fourth isbumpExplorerAuthEpochAll, whose own comment says it exists precisely for entities absent frompotentiallyAuthenticated— the ones nothing else purges. That is the path where the fallback was most needed and least sufficient.So the revocation is not merely recorded, it is enforced: when the durable write fails the credential is destroyed outright.
sessionStorageis a separate store and is usually still writable whenlocalStorageis not, and a deleted credential outlives the reload that drops the count.Verification
The new test drives a real sign-out (
signOutLocally) withStorage.prototype.setItemthrowingQuotaExceededError, and asserts both that the epoch advanced and that the subscriber fired.A second test covers the resurrection path directly: an entity with a stored credential that is absent from
potentiallyAuthenticated, a global logout withsetItemthrowing, asserting the token is gone fromsessionStorage.Mutation-checked three ways — dropping the
catchbump, dropping the fallback from the epoch sum, and dropping the purge — each fails.(One trap worth recording: spying on
localStorage.setItemis silently inert under jsdom, which proxies throughStorage.prototype. Written that way the test fails against the fixed code and looks like the fix does not work.)tsc -b,oxlint,dprint checkclean; 327 files, 2715 passed.What this does not do
It does not make the explorer work with storage disabled — settings and credentials live in
sessionStorage, so the feature is degraded regardless. It only ensures a sign-out is honored rather than silently ignored.It also assumes
sessionStorageis writable whenlocalStorageis not. If both are dead there is no stored credential to revoke in the first place, so the case is moot; if onlysessionStoragedies the purge throws and is swallowed bysettings.ts, leaving the in-memory count as the sole guard — degraded, but no worse than before this PR.🤖 Generated with Claude Code