From 58c642f27bc535521025eeefcceccd215a748f8f Mon Sep 17 00:00:00 2001 From: arnauda-gh <59512940+arnauda-gh@users.noreply.github.com> Date: Mon, 13 Jul 2026 09:05:45 +0200 Subject: [PATCH] fix(auth): coalesce concurrent token refresh requests Coalesces concurrent OAuth token refreshes using a Promise-based lock map keyed by the refresh token. This prevents the app from firing duplicate refresh requests with the same expired refresh token, which causes replay revocations on OIDC providers (e.g. Authelia/Hydra) enforcing strict rotation. Also optimizes persistRefreshedTokens and ensureFreshCredentials to prevent redundant SecureStore/disk writes when concurrent operations complete. Fixes #14 --- src/api/jmap-client.ts | 1 + src/api/unified-inbox.ts | 5 +- src/lib/__tests__/oauth.test.ts | 89 +++++++++++++++++++++++++++++++++ src/lib/oauth.ts | 77 +++++++++++++++++----------- 4 files changed, 141 insertions(+), 31 deletions(-) create mode 100644 src/lib/__tests__/oauth.test.ts diff --git a/src/api/jmap-client.ts b/src/api/jmap-client.ts index 22143eab..c70269d5 100644 --- a/src/api/jmap-client.ts +++ b/src/api/jmap-client.ts @@ -151,6 +151,7 @@ export class JMAPClient { private async persistRefreshedTokens(next: OAuthTokens): Promise { if (!this.credentials) return; + if (this.credentials.accessToken === next.accessToken) return; this.credentials = { ...this.credentials, accessToken: next.accessToken, diff --git a/src/api/unified-inbox.ts b/src/api/unified-inbox.ts index 75b441fd..1e5619f9 100644 --- a/src/api/unified-inbox.ts +++ b/src/api/unified-inbox.ts @@ -101,7 +101,10 @@ async function ensureFreshCredentials( tokenEndpoint: next.tokenEndpoint, clientId: next.clientId, }; - await jmapClient.setStoredCredentials(accountId, updated); + const current = await jmapClient.getStoredCredentials(accountId); + if (!current || current.accessToken !== updated.accessToken) { + await jmapClient.setStoredCredentials(accountId, updated); + } return updated; } catch { // Fall back to the existing (possibly expired) token; the request will diff --git a/src/lib/__tests__/oauth.test.ts b/src/lib/__tests__/oauth.test.ts new file mode 100644 index 00000000..d31dd751 --- /dev/null +++ b/src/lib/__tests__/oauth.test.ts @@ -0,0 +1,89 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const { mockSecureFetch } = vi.hoisted(() => ({ + mockSecureFetch: vi.fn(), +})); + +vi.mock('../client-cert', () => ({ + secureFetch: mockSecureFetch, +})); + +import { refreshOAuthAccessToken, type OAuthTokens } from '../oauth'; + +describe('oauth token refresh deduplication', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('should only send a single HTTP request when refreshOAuthAccessToken is called concurrently with the same refresh token', async () => { + const tokens: OAuthTokens = { + accessToken: 'old-access', + refreshToken: 'same-refresh-token', + expiresAt: 123456, + tokenEndpoint: 'https://auth.example.com/token', + clientId: 'client-id-123', + }; + + // Set up a mock response that takes a brief moment to resolve + mockSecureFetch.mockImplementation(async () => { + await new Promise((resolve) => setTimeout(resolve, 50)); + return { + ok: true, + status: 200, + json: async () => ({ + access_token: 'new-access-token', + refresh_token: 'rotated-refresh-token', + expires_in: 3600, + }), + } as Response; + }); + + // Fire two refreshes concurrently + const [res1, res2] = await Promise.all([ + refreshOAuthAccessToken(tokens), + refreshOAuthAccessToken(tokens), + ]); + + // Check that secureFetch was called exactly once + expect(mockSecureFetch).toHaveBeenCalledTimes(1); + + // Verify both calls received the exact same updated token bundle + expect(res1.accessToken).toBe('new-access-token'); + expect(res1.refreshToken).toBe('rotated-refresh-token'); + expect(res2.accessToken).toBe('new-access-token'); + expect(res2.refreshToken).toBe('rotated-refresh-token'); + + // Fire another refresh after the first completes (now the map should be empty for this token) + mockSecureFetch.mockClear(); + const res3 = await refreshOAuthAccessToken(tokens); + expect(mockSecureFetch).toHaveBeenCalledTimes(1); + expect(res3.accessToken).toBe('new-access-token'); + }); + + it('should propagate errors to all waiters if the HTTP request fails', async () => { + const tokens: OAuthTokens = { + accessToken: 'old-access', + refreshToken: 'failing-refresh-token', + expiresAt: 123456, + tokenEndpoint: 'https://auth.example.com/token', + clientId: 'client-id-123', + }; + + mockSecureFetch.mockImplementation(async () => { + await new Promise((resolve) => setTimeout(resolve, 50)); + return { + ok: false, + status: 400, + } as Response; + }); + + // Fire two refreshes concurrently and check that both reject with the same error + const promise1 = refreshOAuthAccessToken(tokens); + const promise2 = refreshOAuthAccessToken(tokens); + + await expect(promise1).rejects.toThrow('Token refresh failed: 400'); + await expect(promise2).rejects.toThrow('Token refresh failed: 400'); + + expect(mockSecureFetch).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/lib/oauth.ts b/src/lib/oauth.ts index 015027be..fea94ed2 100644 --- a/src/lib/oauth.ts +++ b/src/lib/oauth.ts @@ -207,6 +207,8 @@ export async function redeemPairingCode(webmailUrl: string, code: string): Promi }; } +const activeRefreshes = new Map>(); + // OAuth refresh — exchanges the refresh token at the original token endpoint // for a new access token. Returns the updated bundle so the caller can // persist it. @@ -214,37 +216,52 @@ export async function refreshOAuthAccessToken(tokens: OAuthTokens): Promise { + try { + const body = new URLSearchParams({ + grant_type: 'refresh_token', + refresh_token: tokens.refreshToken!, + client_id: tokens.clientId, + }); + const response = await secureFetch(tokens.tokenEndpoint, { + method: 'POST', + headers: { + 'Content-Type': 'application/x-www-form-urlencoded', + Accept: 'application/json', + }, + body: body.toString(), + }); + if (!response.ok) { + throw new HandoffError(`Token refresh failed: ${response.status}`); + } + const data = (await response.json()) as { + access_token?: string; + refresh_token?: string; + expires_in?: number; + }; + if (!data.access_token) { + throw new HandoffError('Token refresh response missing access_token'); + } + return { + accessToken: data.access_token, + refreshToken: data.refresh_token ?? tokens.refreshToken!, + expiresAt: data.expires_in ? Date.now() + data.expires_in * 1000 : undefined, + tokenEndpoint: tokens.tokenEndpoint, + clientId: tokens.clientId, + }; + } finally { + activeRefreshes.delete(cacheKey); + } + })(); + activeRefreshes.set(cacheKey, promise); } - const data = (await response.json()) as { - access_token?: string; - refresh_token?: string; - expires_in?: number; - }; - if (!data.access_token) { - throw new HandoffError('Token refresh response missing access_token'); - } - return { - accessToken: data.access_token, - refreshToken: data.refresh_token ?? tokens.refreshToken, - expiresAt: data.expires_in ? Date.now() + data.expires_in * 1000 : undefined, - tokenEndpoint: tokens.tokenEndpoint, - clientId: tokens.clientId, - }; + + return promise; } WebBrowser.maybeCompleteAuthSession();