diff --git a/src/lib/__tests__/oauth.test.ts b/src/lib/__tests__/oauth.test.ts index deed5e1e..e03c8893 100644 --- a/src/lib/__tests__/oauth.test.ts +++ b/src/lib/__tests__/oauth.test.ts @@ -126,6 +126,38 @@ describe('oauth token refresh deduplication', () => { expect(mockSecureFetch).not.toHaveBeenCalled(); }); + it('does not bring back a chain forgotten while its refresh was in flight', async () => { + const tokens: OAuthTokens = { + accessToken: 'a1', + refreshToken: 'r1', + tokenEndpoint: 'https://auth.example.com/token', + clientId: 'bulwark-webmail', + }; + let answer!: () => void; + mockSecureFetch.mockImplementationOnce(async () => { + await new Promise((resolve) => { answer = resolve; }); + return { + ok: true, + status: 200, + json: async () => ({ access_token: 'a2', refresh_token: 'r2', expires_in: 3600 }), + } as Response; + }); + + const inFlight = refreshOAuthAccessToken(tokens); + await vi.waitFor(() => expect(mockSecureFetch).toHaveBeenCalledTimes(1)); + // The account signs out before the token endpoint answers. + forgetRefreshRotations('r1'); + answer(); + await inFlight; + + // A stale copy refreshed later doesn't get the signed-out account's + // successor, nor one of the successor's. + mockSecureFetch.mockResolvedValue({ ok: false, status: 400 } as Response); + await expect(refreshOAuthAccessToken(tokens)).rejects.toThrow('400'); + await expect(refreshOAuthAccessToken({ ...tokens, accessToken: 'a2', refreshToken: 'r2' })).rejects.toThrow('400'); + expect(mockSecureFetch).toHaveBeenCalledTimes(3); + }); + it('keeps refreshing with the same token on a server that does not rotate', async () => { const tokens: OAuthTokens = { accessToken: 'a1', diff --git a/src/lib/__tests__/stored-token-refresh.test.ts b/src/lib/__tests__/stored-token-refresh.test.ts index 1682c6c7..70b58a47 100644 --- a/src/lib/__tests__/stored-token-refresh.test.ts +++ b/src/lib/__tests__/stored-token-refresh.test.ts @@ -14,7 +14,7 @@ vi.mock('../../api/jmap-client', () => ({ })); import { jmapClient, type StoredCredentials } from '../../api/jmap-client'; -import { resetRefreshRotations } from '../oauth'; +import { forgetRefreshRotations, refreshOAuthAccessToken, resetRefreshRotations } from '../oauth'; import { freshStoredCredentials } from '../stored-token-refresh'; const ID = 'u@example.org@https://mail.example.com'; @@ -99,6 +99,25 @@ describe('freshStoredCredentials', () => { expect(jmapClient.setStoredCredentials).not.toHaveBeenCalled(); }); + it('hands out nothing when the account signs out while the refresh runs', async () => { + store.set(ID, bundle(1)); + const rotate = mockSecureFetch.getMockImplementation()!; + mockSecureFetch.mockImplementationOnce(async (url: string, init: RequestInit) => { + // The sign-out, as the auth store does it, before the endpoint answers. + forgetRefreshRotations('r1'); + store.delete(ID); + return rotate(url, init); + }); + + await expect(freshStoredCredentials(ID, bundle(1))).rejects.toThrow(); + + expect(store.has(ID)).toBe(false); + // Nor does a stale copy of the bundle get the successor out of memory. + await expect( + refreshOAuthAccessToken({ accessToken: 'a1', refreshToken: 'r1', tokenEndpoint: 'https://mail.example.com/auth/token', clientId: 'swc1.x' }), + ).rejects.toThrow('400'); + }); + it('does not overwrite a newer bundle stored while the refresh ran', async () => { store.set(ID, bundle(1)); // The iOS notification extension rotates the grant meanwhile (it can't diff --git a/src/lib/oauth.ts b/src/lib/oauth.ts index 48a25031..9e4c7678 100644 --- a/src/lib/oauth.ts +++ b/src/lib/oauth.ts @@ -547,9 +547,26 @@ const ROTATED_MAX = 64; // A successor whose access token has less left than this is refreshed in // turn rather than handed out. const SUCCESSOR_MIN_LIFETIME_MS = 60_000; +// Refresh tokens whose chain was dropped on sign-out. A refresh that was in +// flight then returns afterwards and must not put the chain back. +const forgotten = new Set(); +const FORGOTTEN_MAX = 256; + +function markForgotten(refreshToken: string): void { + forgotten.delete(refreshToken); + if (forgotten.size >= FORGOTTEN_MAX) { + const oldest = forgotten.values().next().value; + if (oldest !== undefined) forgotten.delete(oldest); + } + forgotten.add(refreshToken); +} function rememberRotation(previous: string, next: OAuthTokens): void { if (!next.refreshToken || next.refreshToken === previous) return; + if (forgotten.has(previous)) { + markForgotten(next.refreshToken); + return; + } if (!rotatedTo.has(previous) && rotatedTo.size >= ROTATED_MAX) { const oldest = rotatedTo.keys().next().value; if (oldest !== undefined) rotatedTo.delete(oldest); @@ -572,13 +589,15 @@ function latestRotation(refreshToken: string): OAuthTokens | null { /** Test hook. */ export function resetRefreshRotations(): void { rotatedTo.clear(); + forgotten.clear(); } /** * Drop the rotation chain `refreshToken` is part of, its predecessors and * successors both, when its account signs out. A stale copy of the bundle * refreshed later would otherwise get the successor from here and could - * write the removed account's tokens back. + * write the removed account's tokens back. A refresh of the chain still in + * flight doesn't add it again when it returns. */ export function forgetRefreshRotations(refreshToken: string | undefined): void { if (!refreshToken) return; @@ -593,7 +612,10 @@ export function forgetRefreshRotations(refreshToken: string | undefined): void { if (next.refreshToken && !chain.has(next.refreshToken)) { chain.add(next.refreshToken); grew = true; } } } - for (const token of chain) rotatedTo.delete(token); + for (const token of chain) { + rotatedTo.delete(token); + markForgotten(token); + } } // OAuth refresh — exchanges the refresh token at the original token endpoint diff --git a/src/lib/stored-token-refresh.ts b/src/lib/stored-token-refresh.ts index 006dfb0b..adc995ca 100644 --- a/src/lib/stored-token-refresh.ts +++ b/src/lib/stored-token-refresh.ts @@ -16,7 +16,7 @@ // someone else wrote. import { jmapClient, type StoredCredentials } from '../api/jmap-client'; -import { refreshOAuthAccessToken } from './oauth'; +import { forgetRefreshRotations, refreshOAuthAccessToken } from './oauth'; const TOKEN_REFRESH_LEEWAY_MS = 60_000; @@ -29,7 +29,8 @@ function expiresSoon(expiresAt: number | undefined): boolean { * stored when needed. Password credentials come back as they are; so does * an account whose credentials are gone from storage (signed out). A failed * refresh returns what there is, and the request that uses it fails on its - * own. + * own. When the account signs out while the refresh runs, its fresh tokens + * are dropped and this throws. */ export async function freshStoredCredentials( accountId: string, @@ -58,6 +59,7 @@ export async function freshStoredCredentials( if (!expiresSoon(base.expiresAt)) return base; } + let signedOut = false; try { const next = await refreshOAuthAccessToken({ accessToken: base.accessToken!, @@ -75,11 +77,20 @@ export async function freshStoredCredentials( clientId: next.clientId, }; const current = await jmapClient.getStoredCredentials(accountId); - if (current && current.refreshToken === base.refreshToken && current.accessToken !== tokens.accessToken) { + if (!current) { + // Signed out meanwhile: the sign-out forgot the chain before this + // refresh returned. Nobody may use the tokens it bought. + forgetRefreshRotations(base.refreshToken); + forgetRefreshRotations(tokens.refreshToken); + signedOut = true; + throw new Error('Account signed out'); + } + if (current.refreshToken === base.refreshToken && current.accessToken !== tokens.accessToken) { await jmapClient.setStoredCredentials(accountId, { ...current, ...tokens }); } return { ...base, ...tokens }; - } catch { + } catch (err) { + if (signedOut) throw err; return base; } }