This repository was archived by the owner on Jul 8, 2026. It is now read-only.
SC-308993: tolerate stale rotated refresh tokens - #163
Merged
mdthorpe-sc merged 1 commit intoMar 3, 2026
Merged
mdthorpe-sc merged 1 commit into
mdthorpe-sc merged 1 commit into
Conversation
📝 WalkthroughWalkthroughThe pull request introduces server-side refresh token rotation support to the authentication provider. A new test validates the rotation flow where an expired refresh token triggers a server error and subsequent exchanges use newly returned refresh tokens. The implementation adds a refresh token cache map and updates token exchange logic to track and resolve token aliases across multiple refresh cycles. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/auth/provider.ts (1)
511-537:⚠️ Potential issue | 🟠 MajorRefresh alias chain can get stuck one generation behind.
When
refreshTokenis aliased, the request usesrefreshTokenToUse, but cache update uses the originalrefreshToken. That leavesrefreshTokenToUsepotentially pointing at an older entry after the next rotation, causing avoidableinvalid_granton later calls.🔧 Proposed fix
- const tokens = normalizeTokensForClient((await response.json()) as OAuthTokens); - cacheIssuedToken(tokens, client.client_id, refreshToken); + const tokens = normalizeTokensForClient((await response.json()) as OAuthTokens); + const cachedEntry = cacheIssuedToken(tokens, client.client_id, refreshTokenToUse); + if (refreshTokenToUse !== refreshToken) { + issuedRefreshTokens.set(refreshToken, cachedEntry); + } return tokens;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auth/provider.ts` around lines 511 - 537, The refresh alias caching is using the original refreshToken when calling cacheIssuedToken, which leaves the alias chain one generation behind; change the cache update to use the actual token sent to the upstream (refreshTokenToUse) or the new returned refresh token as the cache key so the alias map (issuedRefreshTokens) is updated for the token that was used (refer to issuedRefreshTokens, refreshTokenToUse, and cacheIssuedToken); ensure you pass refreshTokenToUse (or the new tokens.refresh_token if you intend to key by the returned token) into cacheIssuedToken so subsequent refreshes follow the correct alias chain.
🧹 Nitpick comments (1)
src/auth/provider.test.ts (1)
214-296: Extend this regression to cover intermediate-token reuse.Current assertions validate one stale-token hop, but not that
new-refresh-tokenis remapped after the second rotation. Adding that third-step assertion would catch alias-chain regressions.✅ Suggested test extension
test("accepts a previously issued refresh token after server-side rotation", async () => { let oldRefreshTokenCalls = 0; + let newRefreshTokenCalls = 0; const fetchMock = mock(async (_url: string | URL, init?: RequestInit) => { const body = typeof init?.body === "string" ? init.body : ""; const params = new URLSearchParams(body); const refreshToken = params.get("refresh_token"); @@ if (refreshToken === "new-refresh-token") { - return new Response( - JSON.stringify({ - access_token: "access-token-3", - refresh_token: "newer-refresh-token", - scope: "openid", - }), - { - status: 200, - headers: { "Content-Type": "application/json" }, - }, - ); + newRefreshTokenCalls += 1; + if (newRefreshTokenCalls === 1) { + return new Response( + JSON.stringify({ + access_token: "access-token-3", + refresh_token: "newer-refresh-token", + scope: "openid", + }), + { + status: 200, + headers: { "Content-Type": "application/json" }, + }, + ); + } + return new Response( + JSON.stringify({ + error: "invalid_grant", + error_description: "Refresh token already used", + }), + { + status: 400, + headers: { "Content-Type": "application/json" }, + }, + ); + } + + if (refreshToken === "newer-refresh-token") { + return new Response( + JSON.stringify({ + access_token: "access-token-4", + refresh_token: "latest-refresh-token", + scope: "openid", + }), + { + status: 200, + headers: { "Content-Type": "application/json" }, + }, + ); } @@ expect(secondTokens.access_token).toBe("access-token-3"); expect(secondTokens.refresh_token).toBe("newer-refresh-token"); - expect(fetchMock).toHaveBeenCalledTimes(2); + + const thirdTokens = await provider.exchangeRefreshToken( + { + client_id: "test-client-id", + redirect_uris: [], + } as never, + "new-refresh-token", + ); + + expect(thirdTokens.access_token).toBe("access-token-4"); + expect(thirdTokens.refresh_token).toBe("latest-refresh-token"); + expect(fetchMock).toHaveBeenCalledTimes(3); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auth/provider.test.ts` around lines 214 - 296, Add a third exchange to assert intermediate-token reuse: after the existing secondTokens call, call provider.exchangeRefreshToken(...) with "new-refresh-token" (store as thirdTokens) and assert thirdTokens.access_token === "access-token-3" and thirdTokens.refresh_token === "newer-refresh-token"; also update the fetchMock call count expectation to account for the extra request. This ensures provider.exchangeRefreshToken correctly remaps the intermediate token and that fetchMock, firstTokens, secondTokens, and thirdTokens are checked accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/auth/provider.ts`:
- Around line 511-537: The refresh alias caching is using the original
refreshToken when calling cacheIssuedToken, which leaves the alias chain one
generation behind; change the cache update to use the actual token sent to the
upstream (refreshTokenToUse) or the new returned refresh token as the cache key
so the alias map (issuedRefreshTokens) is updated for the token that was used
(refer to issuedRefreshTokens, refreshTokenToUse, and cacheIssuedToken); ensure
you pass refreshTokenToUse (or the new tokens.refresh_token if you intend to key
by the returned token) into cacheIssuedToken so subsequent refreshes follow the
correct alias chain.
---
Nitpick comments:
In `@src/auth/provider.test.ts`:
- Around line 214-296: Add a third exchange to assert intermediate-token reuse:
after the existing secondTokens call, call provider.exchangeRefreshToken(...)
with "new-refresh-token" (store as thirdTokens) and assert
thirdTokens.access_token === "access-token-3" and thirdTokens.refresh_token ===
"newer-refresh-token"; also update the fetchMock call count expectation to
account for the extra request. This ensures provider.exchangeRefreshToken
correctly remaps the intermediate token and that fetchMock, firstTokens,
secondTokens, and thirdTokens are checked accordingly.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Summary
Testing
Summary by CodeRabbit
Tests
Bug Fixes