diff --git a/src/lib/__tests__/oauth-pairing.test.ts b/src/lib/__tests__/oauth-pairing.test.ts index 978ce3b0..23e5c9a4 100644 --- a/src/lib/__tests__/oauth-pairing.test.ts +++ b/src/lib/__tests__/oauth-pairing.test.ts @@ -6,6 +6,7 @@ const { mockSecureFetch } = vi.hoisted(() => ({ vi.mock('../client-cert', () => ({ secureFetch: mockSecureFetch, + fetchWithoutForeignRedirect: mockSecureFetch, })); import { diff --git a/src/lib/__tests__/oauth-redirects.test.ts b/src/lib/__tests__/oauth-redirects.test.ts new file mode 100644 index 00000000..b1c4c1ac --- /dev/null +++ b/src/lib/__tests__/oauth-redirects.test.ts @@ -0,0 +1,151 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import * as WebBrowser from 'expo-web-browser'; + +import { setManualFetchForTests } from '../client-cert'; +import { HandoffError, TransientRefreshError, refreshOAuthAccessToken, resetRefreshRotations } from '../oauth'; +import { discoverOAuthMetadata, loginWithPkce, revokeRefreshToken, type OAuthMetadata } from '../oauth-native'; + +// The refresh token, the code + verifier and the metadata that decides where +// they go must not follow a redirect to another host. One fake network sits +// behind both fetches: the one told `redirect: 'manual'` (Expo's on a device) +// gets the 3xx back; React Native's own follows a 307/308 anywhere with the +// method and body, as OkHttp does. + +const SERVER = 'https://mail.example.com'; +// An external identity provider: its token endpoint is on another host than +// the mail server, and that is fine. +const IDP = 'https://id.example.net'; +const EVIL = 'https://evil.example'; + +interface Hit { url: string; body?: string } +let hits: Hit[]; +let routes: Record Response>; + +function json(body: unknown, status = 200): Response { + return new Response(JSON.stringify(body), { status, headers: { 'Content-Type': 'application/json' } }); +} + +function redirect(location: string, status = 307): Response { + return new Response(null, { status, headers: { Location: location } }); +} + +async function network(url: string, init?: RequestInit): Promise { + hits.push({ url, body: typeof init?.body === 'string' ? init.body : undefined }); + const route = routes[url.split('?')[0]]; + const response = route ? route() : json({ error: 'not found' }, 404); + const location = response.headers.get('location'); + if (location && init?.redirect !== 'manual') return network(location, init); + return response; +} + +const evilHits = () => hits.filter((h) => h.url.startsWith(EVIL)); + +beforeEach(() => { + hits = []; + routes = {}; + resetRefreshRotations(); + setManualFetchForTests(network); + vi.stubGlobal('fetch', vi.fn(network)); +}); +afterEach(() => { + setManualFetchForTests(undefined); + vi.unstubAllGlobals(); +}); + +const tokens = { + accessToken: 'a1', + refreshToken: 'r1', + expiresAt: Date.now() + 5_000, + tokenEndpoint: `${IDP}/token`, + clientId: 'bulwark-webmail', +}; + +describe('token refresh', () => { + it('does not take the refresh token along a redirect to another host', async () => { + routes[`${IDP}/token`] = () => redirect(`${EVIL}/token`); + + await expect(refreshOAuthAccessToken(tokens)).rejects.toBeInstanceOf(TransientRefreshError); + + expect(evilHits()).toEqual([]); + }); + + it("follows a redirect within the token endpoint's own host, not the server's", async () => { + routes[`${IDP}/token`] = () => redirect(`${IDP}/oauth/token`, 308); + routes[`${IDP}/oauth/token`] = () => json({ access_token: 'a2', refresh_token: 'r2', expires_in: 3600 }); + + const next = await refreshOAuthAccessToken(tokens); + + expect(next.accessToken).toBe('a2'); + expect(hits.map((h) => h.url)).toEqual([`${IDP}/token`, `${IDP}/oauth/token`]); + }); +}); + +describe('PKCE code exchange', () => { + it('does not take the code and verifier along a redirect to another host', async () => { + vi.mocked(WebBrowser.openAuthSessionAsync).mockImplementationOnce(async (authUrl: string) => ({ + type: 'success', + url: `bulwarkmobile://auth/callback?code=c1&state=${new URL(authUrl).searchParams.get('state')}`, + })); + routes[`${IDP}/token`] = () => redirect(`${EVIL}/token`); + const metadata: OAuthMetadata = { authorization_endpoint: `${IDP}/authorize`, token_endpoint: `${IDP}/token` }; + + await expect(loginWithPkce(SERVER, metadata, { clientId: 'bulwark-webmail' })).rejects.toBeInstanceOf(HandoffError); + + expect(hits.map((h) => h.url)).toEqual([`${IDP}/token`]); + expect(evilHits()).toEqual([]); + }); +}); + +describe('revocation', () => { + it('reaches an identity provider on another host but not a redirect away from it', async () => { + routes[`${SERVER}/.well-known/oauth-authorization-server`] = () => json({ + authorization_endpoint: `${IDP}/authorize`, + token_endpoint: `${IDP}/token`, + revocation_endpoint: `${IDP}/revoke`, + }); + routes[`${IDP}/revoke`] = () => redirect(`${EVIL}/revoke`); + + await revokeRefreshToken(SERVER, { ...tokens, source: 'native' }); + + expect(hits.map((h) => h.url)).toContain(`${IDP}/revoke`); + expect(evilHits()).toEqual([]); + }); +}); + +describe('metadata discovery', () => { + it('ignores metadata reached through a redirect to another host', async () => { + routes[`${SERVER}/.well-known/oauth-authorization-server`] = () => + redirect(`${EVIL}/.well-known/oauth-authorization-server`, 302); + routes[`${EVIL}/.well-known/oauth-authorization-server`] = () => json({ + authorization_endpoint: `${EVIL}/authorize`, + token_endpoint: `${EVIL}/token`, + registration_endpoint: `${EVIL}/register`, + }); + + expect(await discoverOAuthMetadata(SERVER)).toBeNull(); + expect(evilHits()).toEqual([]); + }); + + it('ignores it where the platform followed the redirect anyway', async () => { + // No fetch that takes `redirect: 'manual'`: the answer's url gives the + // redirect away. + setManualFetchForTests(null); + vi.stubGlobal('fetch', vi.fn(async (url: string) => { + hits.push({ url }); + const response = json({ authorization_endpoint: `${EVIL}/authorize`, token_endpoint: `${EVIL}/token` }); + Object.defineProperty(response, 'url', { value: `${EVIL}/.well-known/oauth-authorization-server` }); + return response; + })); + + expect(await discoverOAuthMetadata(SERVER)).toBeNull(); + }); + + it('takes metadata from the server itself', async () => { + routes[`${SERVER}/.well-known/oauth-authorization-server`] = () => json({ + authorization_endpoint: `${IDP}/authorize`, + token_endpoint: `${IDP}/token`, + }); + + expect((await discoverOAuthMetadata(SERVER))?.token_endpoint).toBe(`${IDP}/token`); + }); +}); diff --git a/src/lib/__tests__/oauth.test.ts b/src/lib/__tests__/oauth.test.ts index c5fd4f2e..e03c8893 100644 --- a/src/lib/__tests__/oauth.test.ts +++ b/src/lib/__tests__/oauth.test.ts @@ -6,6 +6,7 @@ const { mockSecureFetch } = vi.hoisted(() => ({ vi.mock('../client-cert', () => ({ secureFetch: mockSecureFetch, + fetchWithoutForeignRedirect: mockSecureFetch, })); import { forgetRefreshRotations, refreshOAuthAccessToken, resetRefreshRotations, type OAuthTokens } from '../oauth'; @@ -125,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 82cb6b2c..70b58a47 100644 --- a/src/lib/__tests__/stored-token-refresh.test.ts +++ b/src/lib/__tests__/stored-token-refresh.test.ts @@ -5,7 +5,7 @@ const { mockSecureFetch, store } = vi.hoisted(() => ({ store: new Map(), })); -vi.mock('../client-cert', () => ({ secureFetch: mockSecureFetch })); +vi.mock('../client-cert', () => ({ secureFetch: mockSecureFetch, fetchWithoutForeignRedirect: mockSecureFetch })); vi.mock('../../api/jmap-client', () => ({ jmapClient: { getStoredCredentials: vi.fn(async (id: string) => (store.has(id) ? structuredClone(store.get(id)) : null)), @@ -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-native.ts b/src/lib/oauth-native.ts index accf6858..9b8ed8dd 100644 --- a/src/lib/oauth-native.ts +++ b/src/lib/oauth-native.ts @@ -11,7 +11,7 @@ // the hand-off produces, so `connectWithOAuth` and the refresh path apply. import * as WebBrowser from 'expo-web-browser'; -import { secureFetch } from './client-cert'; +import { fetchWithoutForeignRedirect, followedOnlyAllowedRedirects, mayFollowRedirect, secureFetch } from './client-cert'; import { HANDOFF_REDIRECT_URI, HandoffError, HandoffCancelledError, type OAuthTokens } from './oauth'; import { generateCodeChallenge, @@ -51,7 +51,9 @@ function sameHost(a: string, b: string): boolean { /** * RFC 8414 / OIDC discovery on the mail server origin. Returns null when the * server advertises no authorization server (plain Stalwart without OAuth, - * or a non-Stalwart JMAP server). + * or a non-Stalwart JMAP server). The document decides where registration, + * the sign-in page and the code exchange go, so one reached through a + * redirect to another host doesn't count. */ export async function discoverOAuthMetadata(serverUrl: string): Promise { const base = serverUrl.replace(/\/+$/, ''); @@ -62,8 +64,11 @@ export async function discoverOAuthMetadata(serverUrl: string): Promise; if ( typeof data.authorization_endpoint === 'string' && @@ -239,9 +244,11 @@ export async function loginWithPkce( redirect_uri: redirectUri, code_verifier: verifier, }); + // The code and verifier follow a redirect only within the token endpoint's + // own host, which may be an identity provider's rather than the server's. let response: Response; try { - response = await secureFetch(metadata.token_endpoint, { + response = await fetchWithoutForeignRedirect(metadata.token_endpoint, { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded', Accept: 'application/json' }, body: body.toString(), @@ -284,7 +291,7 @@ export async function revokeRefreshToken(serverUrl: string, tokens: OAuthTokens) token_type_hint: 'refresh_token', client_id: tokens.clientId, }); - await secureFetch(endpoint, { + await fetchWithoutForeignRedirect(endpoint, { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, body: body.toString(), diff --git a/src/lib/oauth.ts b/src/lib/oauth.ts index 3c0e2ef4..9e4c7678 100644 --- a/src/lib/oauth.ts +++ b/src/lib/oauth.ts @@ -1,5 +1,5 @@ import * as WebBrowser from 'expo-web-browser'; -import { secureFetch } from './client-cert'; +import { fetchWithoutForeignRedirect, secureFetch } from './client-cert'; import { randomHex } from './random'; // Webmail-mediated login. The app opens the webmail's normal login page with @@ -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 @@ -623,9 +645,12 @@ export async function refreshOAuthAccessToken(tokens: OAuthTokens): Promise { feeds: useCalendarSubscriptionsStore.getState().subscriptions.map((s) => s.id), }); - it('keeps the recent searches and calendar feeds while an account is still signed in', async () => { + it("keeps the recent searches and the other accounts' calendar feeds while an account is still signed in", async () => { await signedIn(A, B); useSearchHistoryStore.setState({ recentSearches: ['invoice'] }); useCalendarSubscriptionsStore.setState({ subscriptions: [feed(A), feed(B)] }); @@ -269,7 +269,69 @@ describe('what a sign-out leaves behind', () => { await useAuthStore.getState().logout(); await settle(); - expect(leftovers()).toEqual({ searches: ['invoice'], feeds: [`sub-${A}`, `sub-${B}`] }); + expect(leftovers()).toEqual({ searches: ['invoice'], feeds: [`sub-${B}`] }); + }); + + it("drops a removed account's calendar feeds right away", async () => { + await signedIn(A, B, C); + useCalendarSubscriptionsStore.setState({ subscriptions: [feed(A), feed(B), feed(C)] }); + + await useAuthStore.getState().removeAccount(C); + + expect(leftovers().feeds).toEqual([`sub-${A}`, `sub-${B}`]); + }); + + it('drops the feeds of an account whose sign-in is refused on the switch to it', async () => { + await signedIn(A, B); + useCalendarSubscriptionsStore.setState({ subscriptions: [feed(A), feed(B)] }); + client.loadErrors = { [B]: new AuthenticationError('Invalid credentials') }; + + await useAuthStore.getState().switchAccount(B); + + expect(useAccountStore.getState().accounts.map((a) => a.id)).toEqual([A]); + expect(leftovers().feeds).toEqual([`sub-${A}`]); + }); + + it('clears them when a switch evicts the last account', async () => { + // Signed out of the active account while the last one's server was + // away; it is refused when the user picks it. + await signedIn(A); + useAuthStore.setState({ isAuthenticated: false, session: null, activeAccountId: null, accountId: null }); + client.served = null; + useSearchHistoryStore.setState({ recentSearches: ['invoice'] }); + useCalendarSubscriptionsStore.setState({ subscriptions: [feed(A)] }); + client.loadErrors = { [A]: new AuthenticationError('Invalid credentials') }; + + await useAuthStore.getState().switchAccount(A); + + expect(useAccountStore.getState().accounts).toEqual([]); + expect(leftovers()).toEqual({ searches: [], feeds: [] }); + }); + + it('clears them when the restore on launch evicts the last account', async () => { + await signedIn(A); + useAuthStore.setState({ isAuthenticated: false, session: null, activeAccountId: null, accountId: null }); + useSearchHistoryStore.setState({ recentSearches: ['invoice'] }); + useCalendarSubscriptionsStore.setState({ subscriptions: [feed(A)] }); + client.loadErrors = { [A]: new AuthenticationError('Invalid credentials') }; + + expect(await useAuthStore.getState().restoreSession()).toBe(false); + + expect(useAccountStore.getState().accounts).toEqual([]); + expect(leftovers()).toEqual({ searches: [], feeds: [] }); + }); + + it('clears them when a session retry evicts the last account', async () => { + await signedIn(A); + useAuthStore.setState({ session: null }); + useSearchHistoryStore.setState({ recentSearches: ['invoice'] }); + useCalendarSubscriptionsStore.setState({ subscriptions: [feed(A)] }); + client.loadErrors = { [A]: new AuthenticationError('Invalid credentials') }; + + expect(await useAuthStore.getState().retrySession()).toBe(false); + + expect(useAccountStore.getState().accounts).toEqual([]); + expect(leftovers()).toEqual({ searches: [], feeds: [] }); }); it('clears them when the last account signs out', async () => { diff --git a/src/stores/auth-store.ts b/src/stores/auth-store.ts index 7312e9d0..3dfbf739 100644 --- a/src/stores/auth-store.ts +++ b/src/stores/auth-store.ts @@ -147,6 +147,27 @@ function clearDeviceWideAccountData(): void { useCalendarSubscriptionsStore.getState().clearAll(); } +// An account left the registry: signed out, or evicted because its +// credentials are gone or refused. Its own calendar feeds go now, since feed +// URLs are often secret; feeds made before they recorded their account stay +// until nobody is signed in. When it was the last account, the device-wide +// data goes too. +async function forgetRemovedAccount(accountId: string): Promise { + try { + const feeds = useCalendarSubscriptionsStore; + // On a cold start the stored list may not have been read yet; written + // before that, the filtered empty default would replace it. + if (!feeds.persist.hasHydrated()) await feeds.persist.rehydrate(); + const { subscriptions } = feeds.getState(); + if (subscriptions.some((s) => s.localAccountId === accountId)) { + feeds.setState({ subscriptions: subscriptions.filter((s) => s.localAccountId !== accountId) }); + } + } catch (err) { + console.warn('[auth-store] could not drop the calendar feeds of a removed account:', err); + } + if (useAccountStore.getState().accounts.length === 0) clearDeviceWideAccountData(); +} + // Drop the named account from the email cache, then reset the (per-session, // not yet per-account) contacts and calendar stores. Used by logout when // signing one account out while others remain. @@ -719,6 +740,7 @@ export const useAuthStore = create((set, get) => ({ await revokeStoredRefreshToken(currentId); await jmapClient.clearAccountCredentials(currentId); accountStore.removeAccount(currentId); + await forgetRemovedAccount(currentId); } else { await jmapClient.logout(); } @@ -877,6 +899,7 @@ export const useAuthStore = create((set, get) => ({ useEmailStore.getState().removeAccount(accountId); restorePrevious(); set({ isLoading: false, error: 'Session expired for this account' }); + await forgetRemovedAccount(accountId); return; } } catch (err) { @@ -888,6 +911,7 @@ export const useAuthStore = create((set, get) => ({ useEmailStore.getState().removeAccount(accountId); restorePrevious(); set({ isLoading: false, error: 'Session expired for this account' }); + await forgetRemovedAccount(accountId); return; } // NetworkError or anything else - keep the previous active account @@ -943,6 +967,7 @@ export const useAuthStore = create((set, get) => ({ useEmailStore.getState().removeAccount(accountId); clearViewerCaches(); accountStore.removeAccount(accountId); + await forgetRemovedAccount(accountId); }, restoreSession: async () => { @@ -1004,6 +1029,7 @@ export const useAuthStore = create((set, get) => ({ // No stored credentials (or corrupt) — genuine logout. accountStore.removeAccount(target.id); useEmailStore.getState().removeAccount(target.id); + await forgetRemovedAccount(target.id); set({ isLoading: false, hasRestoredSession: true }); return false; } @@ -1038,6 +1064,7 @@ export const useAuthStore = create((set, get) => ({ await jmapClient.clearAccountCredentials(target.id).catch(() => undefined); accountStore.removeAccount(target.id); useEmailStore.getState().removeAccount(target.id); + await forgetRemovedAccount(target.id); set({ isLoading: false, hasRestoredSession: true, error: 'Session expired' }); return false; } @@ -1107,6 +1134,7 @@ export const useAuthStore = create((set, get) => ({ // Now we know the credentials are bad — fall back to logout flow. await jmapClient.clearAccountCredentials(activeAccountId).catch(() => undefined); accountStore.removeAccount(activeAccountId); + await forgetRemovedAccount(activeAccountId); set({ isAuthenticated: false, isLoading: false,