fix(auth): keep a password-change token when a routine refresh lands first - #9599
Conversation
…first shouldAcceptRefreshedToken required the stored token to be byte-equal to the token the request was sent with. If a routine sliding-window refresh of the same session committed while a password change was in flight, the password change's epoch-advancing replacement was dropped, the stored token kept the revoked epoch, and the next request logged the user out. Also accept an epoch-advancing replacement when the stored token is the same session (user id and epoch) as the request token. The auth_generation check is unchanged, so logins, logouts and user switches are still rejected. Closes invoke-ai#9598
…fresh The unit tests exercise shouldAcceptRefreshedToken directly, so dropping refreshedToken from any of the three acceptRefreshedToken call sites went unnoticed. Drive acceptRefreshedToken through both orderings from invoke-ai#9598: the routine refresh commits before the replacement arrives (the check before the lock), and the replacement queues on the lock behind it (the check inside the lock). Reverting any one call site now fails a test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
lstein
left a comment
There was a problem hiding this comment.
Thanks for this. It closes #9598, and I reviewed it adversarially against the sequences in the issue. No blockers.
What I verified
- Both orderings are fixed. With the real
acceptRefreshedToken, the password-change replacement survives both when the routine refresh commits before it arrives (the check before the lock) and when it waits on the lock behind that refresh (the check inside the lock). - "Strictly newer than both" holds on the backend. A routine refresh from
SlidingWindowTokenMiddlewarecan never move to a new epoch.resolve_authorized_userrefuses a stale epoch, and the new token is minted from that same record. So the only tokens that change the epoch come from_issue_replacement_token(PATCH /auth/meand an admin resetting their own password). - Tokens that should be rejected still are.
- Another user's token, or a logout (no stored token), fails the session-key comparison.
- A login in any tab is caught by the shared
auth_generationcheck. - A stale routine refresh that arrives after the replacement is committed is still rejected, because it isn't an epoch replacement and its bytes don't match.
- Legacy tokens with no epoch claim are read as epoch 0 on both sides, and tokens with no user id fall back to byte equality as before.
The commit I pushed (a9cff3c)
The new tests call shouldAcceptRefreshedToken directly, so I checked whether the wiring in services/api/index.ts is covered. It wasn't: removing refreshedToken from any one of the three acceptRefreshedToken call sites still passed every test. I added two tests to services/api/endpoints/auth.test.ts that run acceptRefreshedToken through both orderings. Reverting any one call site now fails at least one of them. The queued test covers the check inside the lock specifically: it still passes when only the check before the lock is reverted. Both tests also pass with navigator.locks removed, so the fallback lock path is covered too.
Non-blocking
- The rule is "the stored token is still the request's session", not "the newest epoch wins". Say two password changes are submitted at once and both authenticate at epoch 0 (the update is
token_epoch + 1). If the epoch-1 replacement arrives before the epoch-2 one, the revoked epoch-1 token stays stored.mainbehaves the same way and the timing is unlikely, so I'm only noting it. - As you say in the description, the other half of #9598 is still open: any request sent with the old token that gets a 401 in the gap triggers
sessionExpiredLogout, and the media-cookie request on that path is redundant. That can be its own PR.
Summary
Fix for the second drop path described in #9598. When the refresh throttle is idle and a routine sliding-window refresh of the same session commits while a password change is in flight, the password change's epoch-advancing replacement
Ris dropped. That happens becauseshouldAcceptRefreshedTokenrequireslocalStorage.auth_tokento be byte-equal to the token the request was sent with. The stored token keeps the revoked epoch, and the next request 401s and signs the user out.This follows the approach suggested in the issue:
shouldAcceptRefreshedToken(requestToken, requestGeneration, refreshedToken?)still accepts when the stored token is byte-equal torequestToken.refreshedTokenis the same user's epoch-advancing replacement and the stored token has the samegetTokenSessionKey(user id + epoch) asrequestToken. In that case the stored token is only a routine slide of the token the request was sent with, andRis strictly newer than both.auth_generationcheck is unchanged and still runs first, so logins, logouts and other-tab adoptions are still rejected.isEpochReplacementand shared withshouldThrottleRefreshedToken, which keeps the same behavior.acceptRefreshedTokenpassesrefreshedTokenat all three checks (the pre-lock check, the in-lock check and the commit check).I left out the media-cookie round-trip observation from the issue. It narrows the window but doesn't close it, and it seemed better as its own change.
Related Issues / Discussions
Closes #9598. Follow-up to #9541 / #9560.
QA Instructions
New cases in
authTokenRefresh.test.ts:T0, storedT0'at the same epoch, replacementRat a new epoch). It fails onmainand passes with this change.I ran:
vitest --no-watch: 176 files, 2339 tests passedprettier --checkandeslint --max-warnings=0on the changed files, andtsc --noEmit: cleanMerge Plan
N/A
Checklist
What's Newcopy (if doing a release after this PR) (N/A)