fix(auth): accept epoch-changing token refreshes - #9560
Conversation
|
Synced this branch with current |
|
Added an end-to-end negative regression in Validation: 25/25 focused auth/API tests passed with no TypeScript errors; targeted ESLint, Prettier, and |
|
Added a backward-compatibility regression for sessions carrying JWTs minted before the token_epoch claim existed (commit 5cad312). The end-to-end refresh test now covers both explicit epoch 0 and an omitted epoch claim, and verifies that an epoch 1 replacement is accepted inside the normal one-minute throttle, synchronizes the media cookie, and dispatches tokenRefreshed. Validation: 6 auth test files / 40 tests passed with no type errors; targeted ESLint and Prettier checks passed; git diff --check passed. |
|
The new |
lstein
left a comment
There was a problem hiding this comment.
Thanks for this. The fix is correct for the case #9541 describes, and I couldn't break the bypass itself:
- Only the password-change reply can skip the throttle. The sliding-window middleware issues new tokens from the database's current epoch, and only after
resolve_authorized_userhas accepted the request token. A routine refresh therefore always has the same session key as the token it replaces. The only reply that can carry a different epoch comes from_issue_replacement_token. - Old tokens without an epoch claim read as epoch 0 on both the client and the server, so they stay throttled.
- A replacement for a different user, or one that can't be decoded, stays throttled, and
shouldAcceptRefreshedTokenstill has to pass as well. - Both password-change screens (
PATCH /auth/meand an admin resetting their own password) go throughdynamicBaseQuery. - The new tests do protect the fix. Undoing the check inside the lock fails 2 tests; dropping the
tokensBelongToSameUsercondition fails 1.
Requested change: test that routine refreshes still work outside the throttle window
This PR moves the throttle into shouldThrottleRefreshedToken, and nothing tests the path most likely to break: a routine refresh with the same epoch, arriving after the 60s window. This mutation passes the entire frontend suite (173 files, 2,289 tests):
export const shouldThrottleRefreshedToken = (requestToken: string, refreshedToken: string): boolean => {
- if (!isTokenRefreshThrottled()) {
+ if (false) {
return false;
}With that change every same-epoch refresh is throttled forever, so the sliding session window stops working and every session hard-expires. Please add a test for it. A unit test for shouldThrottleRefreshedToken(tokenFor('user', 1, 1), tokenFor('user', 2, 1)) returning false once Date.now() is more than 60s past the last accepted refresh would be enough. It would be even better to also add a dynamicBaseQuery test showing a same-epoch replacement is committed outside the window, to go with your existing inside-the-window test.
Non-blocking
1. The replacement can still be lost when the throttle is idle. I reproduced this with a throwaway test. It's the other side of #9541:
- The tab holds T0, and no refresh has been accepted recently.
- A background mutating request (for example the client-state POST in
driver.ts) was sent with T0 before the password change and processed before the epoch changed. The middleware returns a routine refresh with the old epoch. - That refresh gets the media-auth lock first and commits, so
auth_tokennow holds the refreshed token. - The password-change reply's
acceptRefreshedToken(replacement, T0, ...)passes the throttle check, butshouldAcceptRefreshedTokenrequireslocalStorage.auth_token === T0exactly, so the replacement is dropped. - The stored token has the old epoch, so the next request gets a 401 and
sessionExpiredLogoutfires.
The exact-match check in shouldAcceptRefreshedToken is the second place a replacement gets dropped. One option: when the replacement advances the epoch, also accept it if the stored token has the same getTokenSessionKey as requestToken. The stored token is then just a routine refresh of the token the request was sent with. This isn't caused by this PR, so it can go in a follow-up.
2. A 401 in the gap before the replacement is saved. This comes from reading the code; I haven't reproduced it. The server changes the epoch before the client saves the replacement. In that gap, any request sent with T0 gets a 401 while T0 is still the stored token, so shouldEndSessionForUnauthorized(T0) is true. The request can come from a background refetch in this tab or from another tab, since tabs share localStorage. sessionExpiredLogout then deletes auth_token, and the replacement is dropped. The gap includes a client-side media-cookie request that isn't needed on this path, because the route already sets the cookie with _set_media_cookie. This existed before the PR and is just for awareness.
3. Test details.
- The PR description says the integration test verifies that the media cookie is synchronized before
tokenRefreshedis dispatched. The test only checks that there were two fetches and one dispatch, not their order. Recording the order in a shared array would cover that. vi.stubGlobal('fetch', ...)in the newauth.test.tstests is never undone, so the stub carries into thegetCurrentUsertests after it. Those pass today, but callingvi.unstubAllGlobals()forfetchin anafterEach(and then re-stubbinglocalStorage/windowas needed) would keep the tests independent.
|
Addressed the requested throttle-window coverage in
Validation: 6 auth test files / 38 tests passed with no type errors; targeted ESLint and Prettier checks passed; |
lstein
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. This addresses the requested change, and I verified the claims independently at eed8f3586.
- The production diff against
mainis byte-identical to the version I reviewed at5cad31203, so the fix itself stands as reviewed. - The outside-window coverage is now there at both levels (the
shouldThrottleRefreshedTokenunit test and thedynamicBaseQuerycommit test). I re-applied theif (false)mutation from my review: it now fails exactly those two tests, where before it passed the whole suite. - The new ordering assertion is load-bearing. Moving
dispatch(tokenRefreshed(...))ahead of the media-cookie sync fails both epoch-change tests. - Per-test stub isolation resolves the
fetchstub leaking into thegetCurrentUsertest. - 48/48 auth tests pass locally, and every CI check on the head commit is green.
Agreed that the idle-throttle race belongs in a follow-up rather than here; I've filed it as #9598 with the triggering sequence and a suggested fix, for whoever picks it up.
Closes #9541.
Summary
dynamicBaseQuery-> media-cookie sync -> Redux dispatch pathRelated Issues / Discussions
Closes #9541
QA Instructions
pnpm exec vitest run(170 files, 2,227 tests)pnpm run lint:tscpnpm run lint:dpdmpnpm exec eslint --max-warnings=0 src/features/auth/store/authTokenRefresh.ts src/features/auth/store/authTokenRefresh.test.ts src/services/api/index.ts src/services/api/endpoints/auth.test.tspnpm exec prettier --check src/features/auth/store/authTokenRefresh.ts src/features/auth/store/authTokenRefresh.test.ts src/services/api/index.ts src/services/api/endpoints/auth.test.tsThe integration regression marks a routine refresh as recently accepted, returns a same-user token with an incremented
token_epoch, and verifies that the media cookie is synchronized beforetokenRefreshedis dispatched. Unit coverage keeps same-epoch, cross-user, and unreadable replacements throttled.Merge Plan
No special merge handling is required.
Checklist