From cc848254c88f12b54aae3e011f344510c2adb7fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Wed, 23 Sep 2026 11:45:08 +0200 Subject: [PATCH 1/2] fix(mobile): stop sign-in retry loop on terminal auth failures https://github.com/Kilo-Org/cloud/pull/6624 --- .../lib/auth/auth-context.lifecycle.test.tsx | 135 ++++++++++- apps/mobile/src/lib/auth/auth-context.test.ts | 14 +- apps/mobile/src/lib/auth/auth-context.tsx | 32 ++- apps/mobile/src/lib/auth/auth-fetch.test.ts | 46 ++++ apps/mobile/src/lib/auth/auth-fetch.ts | 47 +++- .../src/lib/auth/auth-response-class.test.ts | 159 ++++++++++++ .../src/lib/auth/auth-response-class.ts | 148 ++++++++++++ apps/mobile/src/lib/auth/credentials.ts | 91 ++++++- .../src/lib/auth/device-auth-poll.test.ts | 40 +++ apps/mobile/src/lib/auth/device-auth-poll.ts | 14 +- .../mobile/src/lib/auth/poll-response.test.ts | 8 + apps/mobile/src/lib/auth/poll-response.ts | 16 +- .../src/lib/auth/refresh-terminal.test.ts | 228 ++++++++++++++++++ 13 files changed, 947 insertions(+), 31 deletions(-) create mode 100644 apps/mobile/src/lib/auth/auth-response-class.test.ts create mode 100644 apps/mobile/src/lib/auth/auth-response-class.ts create mode 100644 apps/mobile/src/lib/auth/refresh-terminal.test.ts diff --git a/apps/mobile/src/lib/auth/auth-context.lifecycle.test.tsx b/apps/mobile/src/lib/auth/auth-context.lifecycle.test.tsx index 88a14eca4b7..e6ea145e484 100644 --- a/apps/mobile/src/lib/auth/auth-context.lifecycle.test.tsx +++ b/apps/mobile/src/lib/auth/auth-context.lifecycle.test.tsx @@ -421,7 +421,7 @@ type AuthContextValue = { isSigningOut: boolean; restoreFailed: boolean; retryRestore: () => void; - signIn: (token: string) => Promise; + signIn: (token: string, refreshToken?: string, expiresIn?: number) => Promise; signOut: (ended?: boolean) => Promise; }; @@ -1482,6 +1482,139 @@ describe('bootstrap and foreground race fencing', () => { unmount(); }); + it('regression: a refused refresh from a superseded epoch does not sign out the newer session', async () => { + const { getCtx, unmount } = await mountProvider(); + + // A session with a refresh token and an expiry inside the refresh margin, + // so a foreground event initiates a proactive refresh. + await act(async () => { + await getCtx().signIn('active-token', 'active-refresh', 3600); + }); + + const listeners = hoisted.appState.addEventListener.mock.calls; + const eventListener = listeners.at(-1)?.[1]; + + // Serve the foreground's expiry read and the refresh's refresh-token read. + // eslint-disable-next-line require-await -- mock returning a resolved promise + hoisted.secureStore.getItemAsync.mockImplementation(async (key: string) => { + if (key === 'token-expires-at') { + return String(Date.now() + 60_000); + } + if (key === 'refresh-token') { + return 'active-refresh'; + } + return null; + }); + + const fetchSpy = vi + .spyOn(globalThis, 'fetch') + .mockResolvedValue(Response.json({ error: 'INVALID_REFRESH_TOKEN' }, { status: 401 })); + + // Hold the terminal clear open so the epoch can move inside it: this is the + // window where the refresh still returns refused for the session that + // owned it. + const { promise: clearGate, resolve: releaseClear } = Promise.withResolvers(); + hoisted.secureStore.deleteItemAsync.mockImplementationOnce(async () => { + await clearGate; + }); + + await act(async () => { + eventListener?.('active'); + await Promise.resolve(); + }); + + // Wait until the clear is in flight, then move the epoch: a newer session + // now owns the tree while the old refresh is still inside its clear. + const authEpoch = await import('@/lib/auth/auth-epoch'); + let flushes = 0; + while (hoisted.secureStore.deleteItemAsync.mock.calls.length === 0 && flushes < 50) { + flushes += 1; + // eslint-disable-next-line no-await-in-loop -- sequential flush until the clear is in flight + await act(async () => { + await new Promise(resolve => { + void setTimeout(resolve, 0); + }); + }); + } + expect(hoisted.secureStore.deleteItemAsync).toHaveBeenCalled(); + expect(fetchSpy).toHaveBeenCalled(); + + authEpoch.bumpAuthEpoch(); + releaseClear(undefined); + + await act(async () => { + await new Promise(resolve => { + void setTimeout(resolve, 0); + }); + }); + + // The stale refusal must not tear down the newer session. + expect(getCtx().sessionEnded).toBe(false); + expect(getCtx().token).toBe('active-token'); + + fetchSpy.mockRestore(); + unmount(); + }); + + it('regression: the refusal-triggered sign-out cleanup still authenticates with the owner token', async () => { + const { getCtx, unmount } = await mountProvider(); + + // A session with a refresh token and an expiry inside the refresh margin, + // so a foreground event initiates a proactive refresh. + await act(async () => { + await getCtx().signIn('active-token', 'active-refresh', 3600); + }); + + const listeners = hoisted.appState.addEventListener.mock.calls; + const eventListener = listeners.at(-1)?.[1]; + + // Serve the foreground's expiry read and the refresh's refresh-token read. + // eslint-disable-next-line require-await -- mock returning a resolved promise + hoisted.secureStore.getItemAsync.mockImplementation(async (key: string) => { + if (key === 'token-expires-at') { + return String(Date.now() + 60_000); + } + if (key === 'refresh-token') { + return 'active-refresh'; + } + return null; + }); + + const fetchSpy = vi + .spyOn(globalThis, 'fetch') + .mockResolvedValue(Response.json({ error: 'INVALID_REFRESH_TOKEN' }, { status: 401 })); + + // The 401 clear runs before the refusal-triggered sign-out. The owner must + // still serve the token to runLogoutCleanup's revoke/unregister, which run + // before the epoch bump and read the Authorization header through + // `getAuthTokenForRequest`. + const tokens: typeof TokenOwnerModule = await import('./token-owner'); + let cleanupToken: string | null | 'unset' = 'unset'; + logoutCleanupMock.runLogoutCleanup.mockImplementationOnce(async () => { + cleanupToken = await tokens.getAuthTokenForRequest(); + }); + + await act(async () => { + eventListener?.('active'); + await Promise.resolve(); + }); + + await vi.waitFor(() => { + expect(logoutCleanupMock.runLogoutCleanup).toHaveBeenCalled(); + }); + await act(async () => { + await new Promise(resolve => { + void setTimeout(resolve, 0); + }); + }); + + // The remote cleanup authenticated with the session's own access token. + expect(cleanupToken).toBe('active-token'); + + fetchSpy.mockRestore(); + unmount(); + }); + it('regression: a signed-out launch re-runs the OS search clear the teardown cannot await', async () => { // The beforeEach leaves every read null: the launch positively restored no // session, which is the retry for a teardown-time clear that failed or a diff --git a/apps/mobile/src/lib/auth/auth-context.test.ts b/apps/mobile/src/lib/auth/auth-context.test.ts index 69fd1b3b3da..c937e9431c2 100644 --- a/apps/mobile/src/lib/auth/auth-context.test.ts +++ b/apps/mobile/src/lib/auth/auth-context.test.ts @@ -98,7 +98,7 @@ vi.mock('react-native', () => ({ import { exchangeLegacyToken } from '@/lib/auth/exchange-legacy-token'; import { performRefresh, persistSignInCredentialsAtEpoch } from '@/lib/auth/credentials'; -import { bumpAuthEpoch } from '@/lib/auth/auth-epoch'; +import { bumpAuthEpoch, currentAuthEpoch } from '@/lib/auth/auth-epoch'; import { AUTH_TOKEN_KEY, LEGACY_EXCHANGE_DONE_KEY, @@ -130,7 +130,11 @@ describe('performRefresh', () => { it('returns refused when no refresh token is stored', async () => { const outcome = await performRefresh(); - expect(outcome).toEqual({ ok: false, refused: true }); + expect(outcome).toEqual({ + ok: false, + refused: true, + sessionVersion: currentAuthEpoch(), + }); }); it('returns refused when the server responds with 401', async () => { @@ -140,7 +144,11 @@ describe('performRefresh', () => { ); const outcome = await performRefresh(); - expect(outcome).toEqual({ ok: false, refused: true }); + expect(outcome).toEqual({ + ok: false, + refused: true, + sessionVersion: currentAuthEpoch(), + }); }); it('returns transient when the server responds with a non-401 error', async () => { diff --git a/apps/mobile/src/lib/auth/auth-context.tsx b/apps/mobile/src/lib/auth/auth-context.tsx index 523850db6a8..078a445f670 100644 --- a/apps/mobile/src/lib/auth/auth-context.tsx +++ b/apps/mobile/src/lib/auth/auth-context.tsx @@ -584,7 +584,16 @@ export function AuthProvider({ children }: { readonly children: ReactNode }) { return; } - if (!outcome.ok && outcome.refused && !isSignedOutReference.current) { + // A refusal belongs to the session that owned the refresh. The epoch can + // move while the refresh awaited — or this call can join an in-flight + // refresh from an older session — so re-check before tearing down: a + // newer session must not be signed out by an older refusal. + if ( + !outcome.ok && + outcome.refused && + isCurrentAuthEpoch(outcome.sessionVersion) && + !isSignedOutReference.current + ) { await signOut(true); } }; @@ -628,9 +637,24 @@ export function AuthProvider({ children }: { readonly children: ReactNode }) { if (outcome.ok && isCurrentAuthEpoch(outcome.sessionVersion)) { setToken(outcome.token); + return; + } + // A refused refresh means the stored pair is gone. The proactive path + // used to keep the dead token and wait for "the next real 401", which + // is exactly the retry the refresh refused: every foreground asked + // again. Stop the loop here and send the person to sign in. A + // transient failure leaves the session alone. The refusal is scoped + // to the session that owned it: the epoch can move while the clear + // inside the refresh awaits, so a refusal from a superseded session + // must not sign out the newer one. + if ( + !outcome.ok && + outcome.refused && + isCurrentAuthEpoch(outcome.sessionVersion) && + !isSignedOutReference.current + ) { + await signOut(true); } - // Transient or refused: do not sign out — the user did not trigger - // an authenticated request. Let the next real 401 handle it. } catch { // A rejected expiry read (or refresh) must not escape as an // unhandled rejection. Return silently, exactly as the null-expiry @@ -642,7 +666,7 @@ export function AuthProvider({ children }: { readonly children: ReactNode }) { return () => { subscription.remove(); }; - }, [token]); + }, [signOut, token]); const value = useMemo( () => ({ diff --git a/apps/mobile/src/lib/auth/auth-fetch.test.ts b/apps/mobile/src/lib/auth/auth-fetch.test.ts index 98bb9320660..4d03eb14c9f 100644 --- a/apps/mobile/src/lib/auth/auth-fetch.test.ts +++ b/apps/mobile/src/lib/auth/auth-fetch.test.ts @@ -1,6 +1,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; import { AUTH_REQUEST_TIMEOUT_MS, postAuth } from '@/lib/auth/auth-fetch'; +import { resetAuthTerminalReports } from '@/lib/auth/auth-response-class'; // Mock @/lib/config to avoid pulling in react-native at module import time. vi.mock('@/lib/config', () => ({ @@ -24,8 +25,10 @@ vi.mock('expo-application', () => ({ describe('postAuth', () => { afterEach(() => { + vi.clearAllMocks(); vi.restoreAllMocks(); vi.useRealTimers(); + resetAuthTerminalReports(); }); it('forwards ssoOrganizationId on a non-ok SSO_ERROR response', async () => { @@ -39,6 +42,8 @@ describe('postAuth', () => { ok: false, errorCode: 'SSO_ERROR', ssoOrganizationId: 'org_1', + httpStatus: 400, + terminal: true, }); }); @@ -53,6 +58,44 @@ describe('postAuth', () => { ok: false, errorCode: 'BLOCKED', ssoOrganizationId: undefined, + httpStatus: 400, + terminal: true, + }); + }); + + it('classifies a 401 on a credential route as terminal', async () => { + vi.spyOn(globalThis, 'fetch').mockResolvedValueOnce( + Response.json({ error: 'INVALID_CODE' }, { status: 401 }) + ); + + const result = await postAuth('/api/auth/native/token', { provider: 'email' }); + + expect(result).toEqual({ + ok: false, + errorCode: 'INVALID_CODE', + ssoOrganizationId: undefined, + httpStatus: 401, + terminal: true, + }); + }); + + it('keeps a 429 retryable and carries the server Retry-After', async () => { + vi.spyOn(globalThis, 'fetch').mockResolvedValueOnce( + Response.json( + { error: 'TOO_MANY_ATTEMPTS' }, + { status: 429, headers: { 'retry-after': '7' } } + ) + ); + + const result = await postAuth('/api/auth/native/otp', { email: 'user@example.com' }); + + expect(result).toEqual({ + ok: false, + errorCode: 'TOO_MANY_ATTEMPTS', + ssoOrganizationId: undefined, + httpStatus: 429, + terminal: false, + retryAfterMs: 7000, }); }); @@ -80,6 +123,7 @@ describe('postAuth', () => { ok: false, errorCode: 'TIMEOUT', ssoOrganizationId: undefined, + terminal: false, }); expect(signals).toHaveLength(1); expect(signals[0]?.aborted).toBe(true); @@ -108,6 +152,7 @@ describe('postAuth', () => { ok: false, errorCode: 'TIMEOUT', ssoOrganizationId: undefined, + terminal: false, }); }); @@ -120,6 +165,7 @@ describe('postAuth', () => { ok: false, errorCode: undefined, ssoOrganizationId: undefined, + terminal: false, }); }); }); diff --git a/apps/mobile/src/lib/auth/auth-fetch.ts b/apps/mobile/src/lib/auth/auth-fetch.ts index 68027e31fa8..ae9fe68ef13 100644 --- a/apps/mobile/src/lib/auth/auth-fetch.ts +++ b/apps/mobile/src/lib/auth/auth-fetch.ts @@ -2,6 +2,7 @@ import { z } from 'zod'; import { API_BASE_URL } from '@/lib/config'; import { clearAttestKeyOnRefusal } from '@/lib/auth/admission'; +import { classifyAuthResponse, reportAuthTerminalFailure } from '@/lib/auth/auth-response-class'; import { parseAuthError } from '@/lib/auth/native-auth-contract'; import { buildClientMetadataHeaders } from '@/lib/client-metadata'; @@ -14,13 +15,30 @@ const stringCodeErrorSchema = z.object({ code: z.string() }); */ export const AUTH_REQUEST_TIMEOUT_MS = 15_000; +export type PostAuthFailure = { + ok: false; + errorCode: string | undefined; + ssoOrganizationId: string | undefined; + /** The HTTP status the route answered with; absent when it never answered. */ + httpStatus?: number; + /** True for a refusal retrying cannot change (a 401, or a 4xx with no retry guidance). */ + terminal?: boolean; + /** The server's own back-off from a 429/503 `Retry-After`, when it sent one. */ + retryAfterMs?: number; +}; + /** * Minimal fetch helper for auth endpoints. Returns success with parsed body - * or failure with an optional error code and SSO organization id. + * or failure with an optional error code, SSO organization id, and the + * classification of the refusal. * * Every native auth POST routes through here, so this is also where a refused * admission drops the stored App Attest key id. Putting it here rather than at - * each caller means a new sign-in path cannot forget it. + * each caller means a new sign-in path cannot forget it. A terminal refusal + * (a 401, or a 4xx with no retry guidance) is classified here and reported + * once with a stable fingerprint, instead of once per retry. The stored + * credential is cleared where a stored credential can exist and a 401 proves + * it dead — the refresh rotation in `credentials.ts`. * * `extraHeaders` carries a caller's own auth header; the login routes take * none, and only the authenticated passkey registration route sets it. @@ -29,10 +47,7 @@ export async function postAuth( path: string, body: unknown, extraHeaders?: Record -): Promise< - | { ok: true; data: unknown } - | { ok: false; errorCode: string | undefined; ssoOrganizationId: string | undefined } -> { +): Promise<{ ok: true; data: unknown } | PostAuthFailure> { const controller = new AbortController(); const timeout = setTimeout(() => { controller.abort(); @@ -65,7 +80,22 @@ export async function postAuth( if (!response.ok) { const parsed = parseAuthError(json); await clearAttestKeyOnRefusal(parsed?.code); - return { ok: false, errorCode: parsed?.code, ssoOrganizationId: parsed?.ssoOrganizationId }; + const classified = classifyAuthResponse({ + path, + status: response.status, + retryAfterHeader: response.headers.get('retry-after'), + }); + if (classified.kind === 'terminal') { + reportAuthTerminalFailure(path, classified.status); + } + return { + ok: false, + errorCode: parsed?.code, + ssoOrganizationId: parsed?.ssoOrganizationId, + httpStatus: response.status, + terminal: classified.kind === 'terminal', + retryAfterMs: classified.kind === 'retry' ? classified.retryAfterMs : undefined, + }; } return { ok: true, data: json }; @@ -77,6 +107,9 @@ export async function postAuth( ok: false, errorCode: controller.signal.aborted ? 'TIMEOUT' : undefined, ssoOrganizationId: undefined, + httpStatus: undefined, + terminal: false, + retryAfterMs: undefined, }; } finally { clearTimeout(timeout); diff --git a/apps/mobile/src/lib/auth/auth-response-class.test.ts b/apps/mobile/src/lib/auth/auth-response-class.test.ts new file mode 100644 index 00000000000..25c2afc85ff --- /dev/null +++ b/apps/mobile/src/lib/auth/auth-response-class.test.ts @@ -0,0 +1,159 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { + authTerminalFingerprint, + classifyAuthResponse, + parseRetryAfterMs, + reportAuthTerminalFailure, + resetAuthTerminalReports, + shouldReportAuthTerminalFailure, +} from '@/lib/auth/auth-response-class'; +import { setTelemetrySink, type TelemetryEvent } from '@/lib/telemetry/error-sink'; + +describe('parseRetryAfterMs', () => { + afterEach(() => { + vi.useRealTimers(); + }); + + it('reads delta-seconds', () => { + expect(parseRetryAfterMs('30')).toBe(30_000); + expect(parseRetryAfterMs('0')).toBe(0); + expect(parseRetryAfterMs(' 5 ')).toBe(5000); + }); + + it('reads an HTTP-date relative to now', () => { + const now = Date.parse('2026-09-23T02:00:00Z'); + expect(parseRetryAfterMs('Wed, 23 Sep 2026 02:00:45 GMT', now)).toBe(45_000); + }); + + it('floors a date already in the past at zero', () => { + const now = Date.parse('2026-09-23T02:00:00Z'); + expect(parseRetryAfterMs('Wed, 23 Sep 2026 01:59:00 GMT', now)).toBe(0); + }); + + it('returns undefined for absent or unparseable values', () => { + expect(parseRetryAfterMs(undefined)).toBeUndefined(); + expect(parseRetryAfterMs(null)).toBeUndefined(); + expect(parseRetryAfterMs('')).toBeUndefined(); + expect(parseRetryAfterMs('later please')).toBeUndefined(); + }); +}); + +describe('classifyAuthResponse', () => { + it('classifies a 2xx as success', () => { + expect(classifyAuthResponse({ path: '/api/auth/native/token', status: 200 })).toEqual({ + kind: 'success', + }); + }); + + it('classifies a 401 on the refresh route as terminal and clears the stored pair', () => { + expect(classifyAuthResponse({ path: '/api/auth/native/refresh', status: 401 })).toEqual({ + kind: 'terminal', + status: 401, + clearCredential: true, + }); + }); + + it('keeps a 401 on native/token terminal without clearing the stored pair', () => { + // native/token takes no bearer: its 401 refuses the attempt's provider + // credential (a code, a ticket, an ID token), not the stored session. + expect(classifyAuthResponse({ path: '/api/auth/native/token', status: 401 })).toEqual({ + kind: 'terminal', + status: 401, + clearCredential: false, + }); + }); + + it('keeps a 401 on a non-credential route terminal without clearing the session', () => { + expect(classifyAuthResponse({ path: '/api/auth/passkey/authenticate', status: 401 })).toEqual({ + kind: 'terminal', + status: 401, + clearCredential: false, + }); + }); + + it('classifies a 429 as retryable and carries Retry-After', () => { + expect( + classifyAuthResponse({ path: '/api/auth/native/otp', status: 429, retryAfterHeader: '12' }) + ).toEqual({ kind: 'retry', status: 429, retryAfterMs: 12_000 }); + expect(classifyAuthResponse({ path: '/api/auth/native/otp', status: 429 })).toEqual({ + kind: 'retry', + status: 429, + retryAfterMs: undefined, + }); + }); + + it('classifies 5xx as retryable', () => { + expect(classifyAuthResponse({ path: '/api/auth/native/token', status: 503 })).toEqual({ + kind: 'retry', + status: 503, + retryAfterMs: undefined, + }); + }); + + it.each([400, 403, 404, 409, 425])( + 'classifies a 4xx with no retry guidance (%i) as terminal and never retries it', + status => { + expect(classifyAuthResponse({ path: '/api/auth/native/token', status })).toEqual({ + kind: 'terminal', + status, + clearCredential: false, + }); + } + ); +}); + +describe('terminal failure reporting', () => { + afterEach(() => { + setTelemetrySink(null); + resetAuthTerminalReports(); + }); + + it('uses one stable fingerprint for every repeat of the same failure', () => { + expect(authTerminalFingerprint('/api/auth/native/token', 401)).toEqual( + authTerminalFingerprint('/api/auth/native/token', 401) + ); + expect(authTerminalFingerprint('/api/auth/native/token', 401)).not.toEqual( + authTerminalFingerprint('/api/auth/native/refresh', 401) + ); + }); + + it('reports each terminal failure once, not once per retry', () => { + const events: TelemetryEvent[] = []; + setTelemetrySink(event => { + events.push(event); + }); + + reportAuthTerminalFailure('/api/auth/native/token', 401); + reportAuthTerminalFailure('/api/auth/native/token', 401); + reportAuthTerminalFailure('/api/auth/native/token', 401); + + expect(events).toHaveLength(1); + expect(events[0]?.level).toBe('error'); + expect(events[0]?.fingerprint).toEqual(['auth-terminal', '/api/auth/native/token', '401']); + }); + + it('reports a distinct terminal failure separately', () => { + const events: TelemetryEvent[] = []; + setTelemetrySink(event => { + events.push(event); + }); + + reportAuthTerminalFailure('/api/auth/native/token', 401); + reportAuthTerminalFailure('/api/auth/native/refresh', 401); + + expect(events).toHaveLength(2); + }); + + it('reports nothing when no sink is installed', () => { + expect(() => { + reportAuthTerminalFailure('/api/auth/native/token', 401); + }).not.toThrow(); + }); + + it('marks the first terminal failure as reportable and later repeats as not', () => { + resetAuthTerminalReports(); + expect(shouldReportAuthTerminalFailure('/api/auth/native/refresh', 401)).toBe(true); + expect(shouldReportAuthTerminalFailure('/api/auth/native/refresh', 401)).toBe(false); + }); +}); diff --git a/apps/mobile/src/lib/auth/auth-response-class.ts b/apps/mobile/src/lib/auth/auth-response-class.ts new file mode 100644 index 00000000000..f458bb68ec3 --- /dev/null +++ b/apps/mobile/src/lib/auth/auth-response-class.ts @@ -0,0 +1,148 @@ +/** + * One classification for a sign-in HTTP response, shared by the native auth + * POST helper (`auth-fetch`) and the refresh rotation (`credentials`). + * + * The production defect this encodes: a terminal refusal (the credential is + * gone, or the server sent a 4xx with no retry guidance) was treated as a + * transient one, so the client kept the dead credential and kept asking — the + * same request reported over and over. A terminal answer must stop the loop, + * clear the credential it proves dead, and put the person back on sign-in. + * + * Pure and dependency-free apart from the telemetry sink, so both the fetch + * boundary and the node test environment can import it. + */ + +import { captureTelemetry } from '@/lib/telemetry/error-sink'; + +/** + * The one route whose 401 proves the *stored* credential is gone. + * + * `native/refresh` consumes the stored refresh token, so a 401 there (unknown, + * expired, or reused refresh token — see the frozen route contract) is the + * pair itself being refused: it is cleared, the loop stops, and the person + * signs in again. A 401 on `native/token` is different: that route takes no + * bearer, so it refuses the provider credential the *attempt* presented (an + * email code, a one-time ticket, an Apple/Google ID token). It is terminal for + * that attempt — never retried — but it is not proof a stored session is dead, + * and clearing one on a mistyped code would sign out a healthy device. + */ +const STORED_CREDENTIAL_ROUTE = '/api/auth/native/refresh'; + +export type AuthResponseClass = + /** 2xx: the route answered. */ + | { readonly kind: 'success' } + /** + * Do not retry. `clearCredential` is true when the refusal proves the stored + * pair is dead (a 401 on the refresh route). + */ + | { readonly kind: 'terminal'; readonly status: number; readonly clearCredential: boolean } + /** Retryable: a 429 carries the server's own back-off, if it named one. */ + | { readonly kind: 'retry'; readonly status: number; readonly retryAfterMs: number | undefined }; + +/** + * `Retry-After` as milliseconds from `now`. Accepts both forms RFC 9110 allows + * — delta-seconds and an HTTP-date — and returns `undefined` for anything else + * so an unparseable header never invents a wait. A date in the past is 0. + */ +export function parseRetryAfterMs( + header: string | null | undefined, + now: number = Date.now() +): number | undefined { + if (header === null || header === undefined) { + return undefined; + } + const trimmed = header.trim(); + if (trimmed === '') { + return undefined; + } + if (/^\d+$/u.test(trimmed)) { + return Number(trimmed) * 1000; + } + const parsed = Date.parse(trimmed); + if (Number.isNaN(parsed)) { + return undefined; + } + return Math.max(0, parsed - now); +} + +/** + * Classify a sign-in response. A 401 is terminal (and clears the stored + * credential on a credential route). A 429 or 5xx is retryable and carries the + * server's `Retry-After` when it sent one. Every other 4xx — and the 1xx/3xx a + * sign-in route never returns — is terminal: retrying it cannot change it. + */ +export function classifyAuthResponse(args: { + path: string; + status: number; + retryAfterHeader?: string | null; + now?: number; +}): AuthResponseClass { + const { path, status } = args; + if (status >= 200 && status < 300) { + return { kind: 'success' }; + } + if (status === 401) { + return { kind: 'terminal', status, clearCredential: path === STORED_CREDENTIAL_ROUTE }; + } + if (status === 408 || status === 429 || status >= 500) { + return { + kind: 'retry', + status, + retryAfterMs: parseRetryAfterMs(args.retryAfterHeader, args.now), + }; + } + return { kind: 'terminal', status, clearCredential: false }; +} + +/** + * The stable fingerprint of one terminal sign-in failure. It names the route + * and the status only — never the retry count, a duration, or a token — so + * every repeat collapses into the same issue instead of counting once per try. + */ +export function authTerminalFingerprint(route: string, status: number): string[] { + return ['auth-terminal', route, String(status)]; +} + +// One report per terminal failure per app process, keyed by the same route and +// status the fingerprint names. The retry loop is what produced a report per +// try; once it stops, this is the belt-and-braces that keeps a late duplicate +// from opening a second event for the same dead credential. +const reportedTerminalFailures = new Set(); + +/** Test seam: forget the process's terminal reports. */ +export function resetAuthTerminalReports(): void { + reportedTerminalFailures.clear(); +} + +/** True when this exact terminal failure has not been reported yet. */ +export function shouldReportAuthTerminalFailure(route: string, status: number): boolean { + const key = `${route}:${status}`; + if (reportedTerminalFailures.has(key)) { + return false; + } + reportedTerminalFailures.add(key); + return true; +} + +/** + * Report one terminal sign-in failure, at most once per process, with the + * stable fingerprint above. Never throws; a no-op until a telemetry sink is + * installed. + */ +export function reportAuthTerminalFailure(route: string, status: number): void { + if (!shouldReportAuthTerminalFailure(route, status)) { + return; + } + captureTelemetry({ + level: 'error', + error: new Error(`Terminal sign-in failure: ${status} on ${route}`), + tags: { + 'error.subsystem': 'auth', + 'auth.outcome': 'terminal', + 'auth.route': route, + 'http.status': status, + }, + contexts: { auth: { route, status, outcome: 'terminal' } }, + fingerprint: authTerminalFingerprint(route, status), + }); +} diff --git a/apps/mobile/src/lib/auth/credentials.ts b/apps/mobile/src/lib/auth/credentials.ts index 57faa2e10a9..ae6f2ec9c50 100644 --- a/apps/mobile/src/lib/auth/credentials.ts +++ b/apps/mobile/src/lib/auth/credentials.ts @@ -2,6 +2,7 @@ import * as SecureStore from 'expo-secure-store'; import { API_BASE_URL } from '@/lib/config'; import { currentAuthEpoch, isCurrentAuthEpoch } from '@/lib/auth/auth-epoch'; +import { classifyAuthResponse, reportAuthTerminalFailure } from '@/lib/auth/auth-response-class'; import { parseTokenPair } from '@/lib/auth/native-auth-contract'; import { readStoredValueWithRetry } from '@/lib/auth/secure-store-read'; import { isSignOutTeardownActive, setActiveToken } from '@/lib/auth/token-owner'; @@ -38,11 +39,29 @@ type RefreshSuccess = { expiresIn: number; sessionVersion: number; }; -type RefreshRefused = { ok: false; refused: true; superseded?: false }; -type RefreshTransient = { ok: false; refused: false; superseded?: false }; +type RefreshRefused = { + ok: false; + refused: true; + superseded?: false; + /** The session that owned the refusal. A handler must drop the refusal when + * this is no longer the current epoch: the epoch can move while the clear + * awaits, and signing out then would tear down the newer session. */ + sessionVersion: number; +}; +type RefreshTransient = { + ok: false; + refused: false; + superseded?: false; + /** The server's own back-off from a 429/503 `Retry-After`, when it sent one. */ + retryAfterMs?: number; +}; type RefreshSuperseded = { ok: false; refused: false; superseded: true }; export type RefreshOutcome = RefreshSuccess | RefreshRefused | RefreshTransient | RefreshSuperseded; +// The one refresh route. Named so the classification fingerprints and the +// request path can never drift apart. +const REFRESH_PATH = '/api/auth/native/refresh'; + // Proactive refresh window: refresh when the token expires within 5 minutes. export const REFRESH_MARGIN_MS = 5 * 60 * 1000; @@ -52,6 +71,45 @@ export async function writeCredentials(write: () => Promise): Promise { return result; } +/** + * Delete the stored bearer pair. + * + * Called when a 401 proves the credential is gone: keeping the dead pair is + * what makes every later request try again and fail again. The deletes run on + * the serialized credential queue, so a sign-in or sign-out that moved the + * epoch first wins and this clear is skipped rather than resurrecting a + * teardown. `epoch` is the session that owned the refusal, captured before the + * request that produced it. + * + * The in-memory owner is deliberately NOT dropped here. Sign-out's teardown + * clears it, and until then it must keep serving: the refusal-triggered + * sign-out's remote cleanup (device-session revoke, push-token unregister) + * runs BEFORE the epoch bump and reads the owner for its Authorization header + * (`getAuthTokenForRequest`). Emptying the owner here would send that cleanup + * unauthenticated and fail it. Dropping the stored refresh token is what stops + * the loop: the next refresh finds none and is refused without a request. + */ +async function clearStoredCredentialsAtEpoch(epoch: number): Promise { + const superseded = (): boolean => !isCurrentAuthEpoch(epoch) || isSignOutTeardownActive(); + if (superseded()) { + return; + } + await writeCredentials(async () => { + if (superseded()) { + return; + } + // Best effort, like sign-out's credential batch: a keychain rejection must + // not escape into doRefresh's catch, which would downgrade this terminal + // 401 into a retryable outcome and restart the very loop it must stop. The + // refusal is reported by the caller regardless. + await Promise.allSettled([ + SecureStore.deleteItemAsync(AUTH_TOKEN_KEY, IOS_BEARER_SECURE_STORE_OPTIONS), + SecureStore.deleteItemAsync(REFRESH_TOKEN_KEY, IOS_BEARER_SECURE_STORE_OPTIONS), + SecureStore.deleteItemAsync(TOKEN_EXPIRES_AT_KEY, IOS_BEARER_SECURE_STORE_OPTIONS), + ]); + }); +} + /** * Persists a credential pair through the serialized credential write queue. * Returns whether the pair was published to the token owner. A newer sign-in @@ -153,14 +211,14 @@ async function doRefresh(): Promise { } if (!storedRefreshToken) { // No refresh token: cannot recover from this 401 — sign out. - return { ok: false, refused: true }; + return { ok: false, refused: true, sessionVersion }; } // Bound the refresh network I/O at the control-plane deadline so a hung // backend can never leave the refresh (or a sign-out queueing behind its // credential write) waiting forever. const response = await withDeadline(CONTROL_PLANE_DEADLINE_MS, async signal => { - const res = await fetch(`${API_BASE_URL}/api/auth/native/refresh`, { + const res = await fetch(`${API_BASE_URL}${REFRESH_PATH}`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ refreshToken: storedRefreshToken }), @@ -173,12 +231,27 @@ async function doRefresh(): Promise { return { ok: false, refused: false, superseded: true }; } - // 401 means the refresh token is expired or revoked — permanent failure. - if (response.status === 401) { - return { ok: false, refused: true }; - } - if (!response.ok) { + // Classify before deciding: a 401 is terminal (the stored pair is gone), + // a 429/5xx is retryable and carries the server's own back-off, and any + // other 4xx is terminal without a retry guidance to honour. + const classified = classifyAuthResponse({ + path: REFRESH_PATH, + status: response.status, + retryAfterHeader: response.headers.get('retry-after'), + }); + if (classified.kind === 'terminal') { + if (classified.clearCredential) { + await clearStoredCredentialsAtEpoch(sessionVersion); + } + reportAuthTerminalFailure(REFRESH_PATH, classified.status); + return { ok: false, refused: true, sessionVersion }; + } + if (classified.kind === 'retry') { + return { ok: false, refused: false, retryAfterMs: classified.retryAfterMs }; + } + // A 2xx that failed `response.ok` is impossible; fall through to the + // retryable answer rather than inventing a fourth outcome. return { ok: false, refused: false }; } diff --git a/apps/mobile/src/lib/auth/device-auth-poll.test.ts b/apps/mobile/src/lib/auth/device-auth-poll.test.ts index 06dbd632e27..0b90d594577 100644 --- a/apps/mobile/src/lib/auth/device-auth-poll.test.ts +++ b/apps/mobile/src/lib/auth/device-auth-poll.test.ts @@ -89,4 +89,44 @@ describe('startDeviceAuthPoll', () => { expect(cleanup).toHaveBeenCalled(); expect(setState).toHaveBeenCalled(); }); + + it('honours a Retry-After longer than its own backoff on a throttled poll', async () => { + fetchMock.mockResolvedValue( + Response.json( + { error: 'TOO_MANY_ATTEMPTS' }, + { status: 429, headers: { 'retry-after': '90' } } + ) + ); + makePoll(); + // First scheduled tick at 3s is throttled. + await vi.advanceTimersByTimeAsync(3000); + expect(fetchMock).toHaveBeenCalledTimes(1); + // Our own backoff would fire again at 6s; the server asked for 90s. + await vi.advanceTimersByTimeAsync(7000); + expect(fetchMock).toHaveBeenCalledTimes(1); + await vi.advanceTimersByTimeAsync(83_000); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it('caps a Retry-After longer than the remaining budget by the time left', async () => { + fetchMock.mockResolvedValue( + Response.json( + { error: 'TOO_MANY_ATTEMPTS' }, + { status: 429, headers: { 'retry-after': '90' } } + ) + ); + // 10s of the 5-minute budget remain when the poll starts. + const { cleanup, setState } = makePoll({ + startedAt: Date.now() - (5 * 60 * 1000 - 10_000), + }); + // The first scheduled tick at 3s is throttled with a 90s Retry-After. + await vi.advanceTimersByTimeAsync(3000); + expect(fetchMock).toHaveBeenCalledTimes(1); + // Only 7s of the budget are left, so the wait is capped by that instead of + // the whole budget: the timeout surfaces at the budget, not 90s later. + await vi.advanceTimersByTimeAsync(7000); + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(cleanup).toHaveBeenCalled(); + expect(setState).toHaveBeenCalled(); + }); }); diff --git a/apps/mobile/src/lib/auth/device-auth-poll.ts b/apps/mobile/src/lib/auth/device-auth-poll.ts index 45db863e9f0..54f6f7fc8f0 100644 --- a/apps/mobile/src/lib/auth/device-auth-poll.ts +++ b/apps/mobile/src/lib/auth/device-auth-poll.ts @@ -60,7 +60,7 @@ export function startDeviceAuthPoll(params: { }; const runTick = async () => { - if (Date.now() - startedAt > POLL_OVERALL_TIMEOUT_MS) { + if (Date.now() - startedAt >= POLL_OVERALL_TIMEOUT_MS) { cleanup(); setState(previous => errorDeviceAuthState(code, i18n.t('authErrors.signInTimedOut'), previous.verificationUrl) @@ -119,7 +119,7 @@ export function startDeviceAuthPoll(params: { return; } - const outcome = classifyPollResponse(response.status); + const outcome = classifyPollResponse(response.status, response.headers.get('retry-after')); // eslint-disable-next-line typescript-eslint/switch-exhaustiveness-check switch (outcome.status) { @@ -139,7 +139,15 @@ export function startDeviceAuthPoll(params: { } case 'retry': { retryDelay = Math.min(retryDelay * 2, POLL_MAX_INTERVAL_MS); - scheduleNext(retryDelay); + // A throttled poll waits as long as the server asked, never less + // than our own backoff, and never past the overall poll budget: the + // wait is capped by the time left, so a Retry-After longer than the + // remaining budget cannot schedule a tick after the budget. The + // boundary check above is inclusive so a wait capped to exactly the + // remaining time times out on that tick instead of polling again. + const wait = Math.max(retryDelay, outcome.retryAfterMs ?? 0); + const remaining = POLL_OVERALL_TIMEOUT_MS - (Date.now() - startedAt); + scheduleNext(Math.min(wait, Math.max(0, remaining))); return; } case 'error': { diff --git a/apps/mobile/src/lib/auth/poll-response.test.ts b/apps/mobile/src/lib/auth/poll-response.test.ts index efc76727159..c139b404cd8 100644 --- a/apps/mobile/src/lib/auth/poll-response.test.ts +++ b/apps/mobile/src/lib/auth/poll-response.test.ts @@ -36,4 +36,12 @@ describe('classifyPollResponse', () => { it.each([429, 500, 503])('retries with backoff on %i', httpStatus => { expect(classifyPollResponse(httpStatus)).toEqual({ status: 'retry' }); }); + + it('carries the server Retry-After on a throttled poll', () => { + expect(classifyPollResponse(429, '8')).toEqual({ status: 'retry', retryAfterMs: 8000 }); + }); + + it('keeps retrying without a wait when Retry-After is unparseable', () => { + expect(classifyPollResponse(429, 'soon')).toEqual({ status: 'retry' }); + }); }); diff --git a/apps/mobile/src/lib/auth/poll-response.ts b/apps/mobile/src/lib/auth/poll-response.ts index af462b60ff9..675441045dd 100644 --- a/apps/mobile/src/lib/auth/poll-response.ts +++ b/apps/mobile/src/lib/auth/poll-response.ts @@ -1,6 +1,7 @@ // Pure classification of a device-auth poll response's HTTP status. Kept // free of any react-native/expo imports so it can be unit tested directly. +import { parseRetryAfterMs } from '@/lib/auth/auth-response-class'; import { i18n } from '@/i18n'; type PollOutcome = @@ -9,9 +10,13 @@ type PollOutcome = | { readonly status: 'denied'; readonly message: string } | { readonly status: 'expired'; readonly message: string } | { readonly status: 'error'; readonly message: string } - | { readonly status: 'retry' }; + | { readonly status: 'retry'; readonly retryAfterMs?: number }; -export function classifyPollResponse(httpStatus: number): PollOutcome { +export function classifyPollResponse( + httpStatus: number, + retryAfterHeader?: string | null, + now?: number +): PollOutcome { if (httpStatus === 200) { return { status: 'approved' }; } @@ -24,9 +29,12 @@ export function classifyPollResponse(httpStatus: number): PollOutcome { if (httpStatus === 410) { return { status: 'expired', message: i18n.t('authErrors.codeExpired') }; } - // 429/5xx are transient (rate limiting or a flaky server) — keep polling. + // 429/5xx are transient (rate limiting or a flaky server) — keep polling, + // and honour the server's own `Retry-After` when it named one instead of + // only our own backoff. if (httpStatus === 429 || httpStatus >= 500) { - return { status: 'retry' }; + const retryAfterMs = parseRetryAfterMs(retryAfterHeader, now); + return retryAfterMs === undefined ? { status: 'retry' } : { status: 'retry', retryAfterMs }; } // Any other 4xx (400, 401, ...) is not something retrying will fix — and // 1xx/3xx are statuses this endpoint never returns, so treat them the same. diff --git a/apps/mobile/src/lib/auth/refresh-terminal.test.ts b/apps/mobile/src/lib/auth/refresh-terminal.test.ts new file mode 100644 index 00000000000..1f9b52243a3 --- /dev/null +++ b/apps/mobile/src/lib/auth/refresh-terminal.test.ts @@ -0,0 +1,228 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const store = new Map(); + +/* eslint-disable import/first */ +// vi.mock is hoisted by Vitest before the real import resolves. This suite +// exercises only the refresh rotation, so the sign-out teardown graph that +// credentials.test.ts has to stub is never loaded here. +vi.mock('expo-secure-store', () => ({ + WHEN_UNLOCKED_THIS_DEVICE_ONLY: 'WHEN_UNLOCKED_THIS_DEVICE_ONLY', + getItemAsync: vi.fn(async (key: string) => { + await Promise.resolve(); + return store.get(key) ?? null; + }), + setItemAsync: vi.fn(async (key: string, value: string) => { + await Promise.resolve(); + store.set(key, value); + }), + deleteItemAsync: vi.fn(async (key: string) => { + await Promise.resolve(); + store.delete(key); + }), +})); + +// `@/lib/config` is stubbed once for the whole pure project by vitest.setup.ts +// (its real module needs the baked `extra`), exactly as credentials.test.ts +// relies on it. + +import { resetAuthTerminalReports } from '@/lib/auth/auth-response-class'; +import { bumpAuthEpoch, currentAuthEpoch, isCurrentAuthEpoch } from '@/lib/auth/auth-epoch'; +import { performRefresh } from '@/lib/auth/credentials'; +import { + clearActiveToken, + getAuthTokenForRequest, + setActiveToken, + setSignOutTeardownActive, +} from '@/lib/auth/token-owner'; +import { setTelemetrySink, type TelemetryEvent } from '@/lib/telemetry/error-sink'; +import { AUTH_TOKEN_KEY, REFRESH_TOKEN_KEY, TOKEN_EXPIRES_AT_KEY } from '@/lib/storage-keys'; +import * as SecureStore from 'expo-secure-store'; +/* eslint-enable import/first */ + +async function flushMicrotasks(): Promise { + await new Promise(resolve => { + setImmediate(resolve); + }); +} + +function recordEvents(): TelemetryEvent[] { + const events: TelemetryEvent[] = []; + setTelemetrySink(event => { + events.push(event); + }); + return events; +} + +describe('refresh terminal classification', () => { + beforeEach(() => { + vi.clearAllMocks(); + store.clear(); + clearActiveToken(); + setSignOutTeardownActive(false); + bumpAuthEpoch(); + }); + + afterEach(() => { + setTelemetrySink(null); + resetAuthTerminalReports(); + vi.unstubAllGlobals(); + }); + + it('clears the stored pair and reports once when the server answers 401', async () => { + store.set(AUTH_TOKEN_KEY, 'dead-token'); + store.set(REFRESH_TOKEN_KEY, 'dead-refresh'); + store.set(TOKEN_EXPIRES_AT_KEY, '123'); + const fetchSpy = vi + .spyOn(globalThis, 'fetch') + .mockResolvedValue(Response.json({ error: 'INVALID_REFRESH_TOKEN' }, { status: 401 })); + const events = recordEvents(); + + // Two foregrounds against the same dead credential: the first stops the + // loop and clears, the second has no refresh token left to offer. + const first = await performRefresh(); + const second = await performRefresh(); + + expect(first).toEqual({ + ok: false, + refused: true, + sessionVersion: currentAuthEpoch(), + }); + expect(second).toEqual({ + ok: false, + refused: true, + sessionVersion: currentAuthEpoch(), + }); + expect(fetchSpy).toHaveBeenCalledTimes(1); + expect(store.get(AUTH_TOKEN_KEY)).toBeUndefined(); + expect(store.get(REFRESH_TOKEN_KEY)).toBeUndefined(); + expect(store.get(TOKEN_EXPIRES_AT_KEY)).toBeUndefined(); + // The terminal failure is reported once, not once per attempt. + expect(events).toHaveLength(1); + expect(events[0]?.fingerprint).toEqual(['auth-terminal', '/api/auth/native/refresh', '401']); + }); + + it('keeps the in-memory owner serving after the 401 clear so sign-out cleanup can authenticate', async () => { + store.set(AUTH_TOKEN_KEY, 'dead-token'); + store.set(REFRESH_TOKEN_KEY, 'dead-refresh'); + store.set(TOKEN_EXPIRES_AT_KEY, '123'); + setActiveToken('dead-token', null); + vi.spyOn(globalThis, 'fetch').mockResolvedValue( + Response.json({ error: 'INVALID_REFRESH_TOKEN' }, { status: 401 }) + ); + + const outcome = await performRefresh(); + + expect(outcome).toEqual({ + ok: false, + refused: true, + sessionVersion: currentAuthEpoch(), + }); + // The stored pair is gone, so the next refresh is refused without a + // request and the loop is stopped... + expect(store.get(REFRESH_TOKEN_KEY)).toBeUndefined(); + // ...but the in-memory owner must keep serving until sign-out's teardown + // clears it: runLogoutCleanup's revoke/unregister run BEFORE the epoch + // bump and read this same token for their Authorization header. + await expect(getAuthTokenForRequest()).resolves.toBe('dead-token'); + }); + + it('keeps a 429 transient, carries Retry-After, and keeps the pair', async () => { + store.set(AUTH_TOKEN_KEY, 'live-token'); + store.set(REFRESH_TOKEN_KEY, 'live-refresh'); + vi.spyOn(globalThis, 'fetch').mockResolvedValue( + Response.json( + { error: 'TOO_MANY_ATTEMPTS' }, + { status: 429, headers: { 'retry-after': '9' } } + ) + ); + + const outcome = await performRefresh(); + + expect(outcome).toEqual({ ok: false, refused: false, retryAfterMs: 9000 }); + expect(store.get(AUTH_TOKEN_KEY)).toBe('live-token'); + expect(store.get(REFRESH_TOKEN_KEY)).toBe('live-refresh'); + }); + + it('stops on a 4xx with no retry guidance instead of retrying it', async () => { + store.set(AUTH_TOKEN_KEY, 'live-token'); + store.set(REFRESH_TOKEN_KEY, 'live-refresh'); + vi.spyOn(globalThis, 'fetch').mockResolvedValue(new Response('bad request', { status: 400 })); + + const outcome = await performRefresh(); + + expect(outcome).toEqual({ + ok: false, + refused: true, + sessionVersion: currentAuthEpoch(), + }); + // A non-401 terminal refusal is not proof the credential is dead, so the + // pair stays until sign-out clears it. + expect(store.get(AUTH_TOKEN_KEY)).toBe('live-token'); + expect(store.get(REFRESH_TOKEN_KEY)).toBe('live-refresh'); + }); + + it('still refuses and reports when the keychain delete rejects on a 401', async () => { + store.set(AUTH_TOKEN_KEY, 'dead-token'); + store.set(REFRESH_TOKEN_KEY, 'dead-refresh'); + store.set(TOKEN_EXPIRES_AT_KEY, '123'); + vi.spyOn(globalThis, 'fetch').mockResolvedValue( + Response.json({ error: 'INVALID_REFRESH_TOKEN' }, { status: 401 }) + ); + // The keychain rejects the first delete. The terminal refusal must survive + // it: without the guard the rejection would reach doRefresh's catch, be + // downgraded to a retryable outcome, and leave the dead pair in place so + // every foreground retries the same 401. + vi.mocked(SecureStore.deleteItemAsync).mockRejectedValueOnce(new Error('keychain unavailable')); + const events = recordEvents(); + + const outcome = await performRefresh(); + + expect(outcome).toEqual({ + ok: false, + refused: true, + sessionVersion: currentAuthEpoch(), + }); + expect(events).toHaveLength(1); + expect(events[0]?.fingerprint).toEqual(['auth-terminal', '/api/auth/native/refresh', '401']); + }); + + it('scopes a refusal to the session that owned it when the epoch moves mid-clear', async () => { + store.set(AUTH_TOKEN_KEY, 'dead-token'); + store.set(REFRESH_TOKEN_KEY, 'dead-refresh'); + vi.spyOn(globalThis, 'fetch').mockResolvedValue( + Response.json({ error: 'INVALID_REFRESH_TOKEN' }, { status: 401 }) + ); + const { promise: clearGate, resolve: releaseClear } = Promise.withResolvers(); + // Hold the first delete open so a newer session can land inside the clear. + vi.mocked(SecureStore.deleteItemAsync).mockImplementationOnce(async (key: string) => { + await clearGate; + store.delete(key); + }); + const ownedEpoch = currentAuthEpoch(); + + const refresh = performRefresh(); + // Wait until the refresh reached the clear (the hanging delete) before + // moving the epoch: that is the window where the refusal is still returned + // even though the session that owned it is gone. + let flushes = 0; + while (vi.mocked(SecureStore.deleteItemAsync).mock.calls.length === 0 && flushes < 50) { + flushes += 1; + // eslint-disable-next-line no-await-in-loop -- sequential flush until the delete is in flight + await flushMicrotasks(); + } + expect(SecureStore.deleteItemAsync).toHaveBeenCalled(); + bumpAuthEpoch(); + releaseClear(undefined); + + const outcome = await refresh; + + expect(outcome.ok).toBe(false); + if (!outcome.ok && outcome.refused) { + expect(outcome.sessionVersion).toBe(ownedEpoch); + } + // The handler's guard reads exactly this: the refusal is stale, so it must + // not sign out the session that replaced it. + expect(isCurrentAuthEpoch(ownedEpoch)).toBe(false); + }); +}); From 7a13bc7a720f82591660cf91cfb5000f250dec6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Wed, 23 Sep 2026 22:46:27 +0000 Subject: [PATCH 2/2] fix: kwf-fix-review-f6b4 patch delivery --- .../src/lib/auth/device-auth-poll.test.ts | 21 +++++++++++++++++++ apps/mobile/src/lib/auth/device-auth-poll.ts | 21 ++++++++++++++++++- 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/apps/mobile/src/lib/auth/device-auth-poll.test.ts b/apps/mobile/src/lib/auth/device-auth-poll.test.ts index 0b90d594577..0065675f490 100644 --- a/apps/mobile/src/lib/auth/device-auth-poll.test.ts +++ b/apps/mobile/src/lib/auth/device-auth-poll.test.ts @@ -80,6 +80,27 @@ describe('startDeviceAuthPoll', () => { expect(fetchMock).toHaveBeenCalledTimes(2); }); + it('does not poll early when foregrounded during a Retry-After cooldown', async () => { + fetchMock.mockResolvedValue( + Response.json( + { error: 'TOO_MANY_ATTEMPTS' }, + { status: 429, headers: { 'retry-after': '90' } } + ) + ); + const { poll } = makePoll(); + // First scheduled tick at 3s is throttled with a 90s Retry-After. + await vi.advanceTimersByTimeAsync(3000); + expect(fetchMock).toHaveBeenCalledTimes(1); + // Foreground well after the 1s guard but before the cooldown expires: the + // server asked us to wait, so no poll may be sent early. + await vi.advanceTimersByTimeAsync(10_000); + poll.pollNow(); + expect(fetchMock).toHaveBeenCalledTimes(1); + // The cooldown still ends on the server's schedule, not later. + await vi.advanceTimersByTimeAsync(80_000); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + it('times out immediately when startedAt is already past the overall timeout', async () => { const { setState, cleanup } = makePoll({ startedAt: Date.now() - 5 * 60 * 1000 - 1000, diff --git a/apps/mobile/src/lib/auth/device-auth-poll.ts b/apps/mobile/src/lib/auth/device-auth-poll.ts index 54f6f7fc8f0..81d83f7da78 100644 --- a/apps/mobile/src/lib/auth/device-auth-poll.ts +++ b/apps/mobile/src/lib/auth/device-auth-poll.ts @@ -42,6 +42,14 @@ export function startDeviceAuthPoll(params: { let timeoutId: ReturnType | undefined = undefined; let inFlight = false; let lastTickStartedAt = 0; + // The server's own throttle deadline, in `Date.now()` milliseconds. A + // foreground poll (`pollNow`) must not send a request before it: resuming + // the app is not the server's permission to poll again. Only a retry that + // named a `Retry-After` records a deadline; our own backoff stays + // bypassable so a resume still polls promptly while the server is merely + // pending. Always at or before the retry timer it was scheduled with, so + // it is never stale while a timer is pending. + let throttledUntil = 0; const scheduleNext = (delay: number) => { timeoutId = setTimeout(() => { @@ -147,7 +155,12 @@ export function startDeviceAuthPoll(params: { // remaining time times out on that tick instead of polling again. const wait = Math.max(retryDelay, outcome.retryAfterMs ?? 0); const remaining = POLL_OVERALL_TIMEOUT_MS - (Date.now() - startedAt); - scheduleNext(Math.min(wait, Math.max(0, remaining))); + const delay = Math.min(wait, Math.max(0, remaining)); + // Record the server's deadline for a foreground poll. Capped by the + // scheduled delay, so a Retry-After longer than the remaining budget + // never pushes the deadline past the tick that times the poll out. + throttledUntil = Date.now() + Math.min(outcome.retryAfterMs ?? 0, delay); + scheduleNext(delay); return; } case 'error': { @@ -178,6 +191,12 @@ export function startDeviceAuthPoll(params: { if (inFlight || Date.now() - lastTickStartedAt < 1000) { return; } + // A foreground transition is not the server's permission to poll again. + // While a Retry-After throttle is in force, leave the retry timer it was + // scheduled with in place instead of ticking: the server asked us to wait. + if (Date.now() < throttledUntil) { + return; + } if (timeoutId) { clearTimeout(timeoutId); timeoutId = undefined;