diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index 1abd1944990..94b7e651278 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Add rate limit (429) handling with automatic retry in authentication flow ([#6993](https://github.com/MetaMask/core/pull/6993)) + - Update authentication services to throw `RateLimitedError` when encountering 429 responses. + - Improve Authentication errors by adding the HTTP code in Error messages. + - Add rate limit retry logic to `SRPJwtBearerAuth` with configurable cooldown via `rateLimitRetry.cooldownDefaultMs` option (defaults to 10 seconds). + - Non-429 errors are thrown immediately without retry, delegating retry logic to consumers. + ## [26.0.0] ### Changed diff --git a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/flow-srp.test.ts b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/flow-srp.test.ts new file mode 100644 index 00000000000..3ba99e17847 --- /dev/null +++ b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/flow-srp.test.ts @@ -0,0 +1,174 @@ +import { SRPJwtBearerAuth } from './flow-srp'; +import { + AuthType, + type AuthConfig, + type LoginResponse, + type UserProfile, +} from './types'; +import * as timeUtils from './utils/time'; +import { Env, Platform } from '../../shared/env'; +import { RateLimitedError } from '../errors'; + +jest.setTimeout(15000); + +// Mock the time utilities to avoid real delays in tests +jest.mock('./utils/time', () => ({ + delay: jest.fn(), +})); + +const mockDelay = timeUtils.delay as jest.MockedFunction< + typeof timeUtils.delay +>; + +// Mock services +const mockGetNonce = jest.fn(); +const mockAuthenticate = jest.fn(); +const mockAuthorizeOIDC = jest.fn(); + +jest.mock('./services', () => ({ + authenticate: (...args: unknown[]) => mockAuthenticate(...args), + authorizeOIDC: (...args: unknown[]) => mockAuthorizeOIDC(...args), + getNonce: (...args: unknown[]) => mockGetNonce(...args), + getUserProfileLineage: jest.fn(), +})); + +describe('SRPJwtBearerAuth rate limit handling', () => { + const config: AuthConfig & { type: AuthType.SRP } = { + type: AuthType.SRP, + env: Env.DEV, + platform: Platform.EXTENSION, + }; + + // Mock data constants + const MOCK_PROFILE: UserProfile = { + profileId: 'p1', + metaMetricsId: 'm1', + identifierId: 'i1', + }; + + const MOCK_NONCE_RESPONSE = { + nonce: 'nonce-1', + identifier: 'identifier-1', + expiresIn: 60, + }; + + const MOCK_AUTH_RESPONSE = { + token: 'jwt-token', + expiresIn: 60, + profile: MOCK_PROFILE, + }; + + const MOCK_OIDC_RESPONSE = { + accessToken: 'access', + expiresIn: 60, + obtainedAt: Date.now(), + }; + + // Helper to create a rate limit error + const createRateLimitError = (retryAfterMs?: number) => + new RateLimitedError('rate limited', retryAfterMs); + + const createAuth = (overrides?: { cooldownDefaultMs?: number }) => { + const store: { value: LoginResponse | null } = { value: null }; + + const auth = new SRPJwtBearerAuth(config, { + storage: { + getLoginResponse: async () => store.value, + setLoginResponse: async (val) => { + store.value = val; + }, + }, + signing: { + getIdentifier: async () => 'identifier-1', + signMessage: async () => 'signature-1', + }, + rateLimitRetry: overrides, + }); + + return { auth, store }; + }; + + beforeEach(() => { + jest.clearAllMocks(); + mockGetNonce.mockResolvedValue(MOCK_NONCE_RESPONSE); + mockAuthenticate.mockResolvedValue(MOCK_AUTH_RESPONSE); + mockAuthorizeOIDC.mockResolvedValue(MOCK_OIDC_RESPONSE); + }); + + it('coalesces concurrent calls into a single login attempt', async () => { + const { auth } = createAuth(); + + const p1 = auth.getAccessToken(); + const p2 = auth.getAccessToken(); + const p3 = auth.getAccessToken(); + + const [t1, t2, t3] = await Promise.all([p1, p2, p3]); + + expect(t1).toBe('access'); + expect(t2).toBe('access'); + expect(t3).toBe('access'); + + // single sequence of service calls + expect(mockGetNonce).toHaveBeenCalledTimes(1); + expect(mockAuthenticate).toHaveBeenCalledTimes(1); + expect(mockAuthorizeOIDC).toHaveBeenCalledTimes(1); + }); + + it('applies cooldown and retries once on 429 with Retry-After', async () => { + const { auth } = createAuth({ cooldownDefaultMs: 20 }); + + let first = true; + mockAuthenticate.mockImplementation(async () => { + // eslint-disable-next-line jest/no-conditional-in-test + if (first) { + first = false; + throw createRateLimitError(20); + } + return MOCK_AUTH_RESPONSE; + }); + + const p1 = auth.getAccessToken(); + const p2 = auth.getAccessToken(); + + const [t1, t2] = await Promise.all([p1, p2]); + expect(t1).toBe('access'); + expect(t2).toBe('access'); + + // Should retry after rate limit error + expect(mockAuthenticate).toHaveBeenCalledTimes(2); + // Should apply cooldown delay + expect(mockDelay).toHaveBeenCalledWith(20); + }); + + it('throws 429 after exhausting one retry', async () => { + const { auth } = createAuth({ cooldownDefaultMs: 20 }); + + mockAuthenticate.mockRejectedValue(createRateLimitError(20)); + + await expect(auth.getAccessToken()).rejects.toThrow('rate limited'); + + // Should attempt initial + one retry = 2 attempts + expect(mockAuthenticate).toHaveBeenCalledTimes(2); + // Should apply cooldown delay once + expect(mockDelay).toHaveBeenCalledTimes(1); + }); + + it('throws transient errors immediately without retry', async () => { + const { auth, store } = createAuth(); + + // Force a login by clearing session + store.value = null; + + const transientError = new Error('transient network error'); + mockAuthenticate.mockRejectedValue(transientError); + + await expect(auth.getAccessToken()).rejects.toThrow( + 'transient network error', + ); + + // Should NOT retry on transient errors + expect(mockAuthenticate).toHaveBeenCalledTimes(1); + // Should NOT apply any delay + expect(mockDelay).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/flow-srp.ts b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/flow-srp.ts index b8e7d68904b..36052e5bd63 100644 --- a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/flow-srp.ts +++ b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/flow-srp.ts @@ -16,8 +16,9 @@ import type { UserProfile, UserProfileLineage, } from './types'; +import * as timeUtils from './utils/time'; import type { MetaMetricsAuth } from '../../shared/types/services'; -import { ValidationError } from '../errors'; +import { ValidationError, RateLimitedError } from '../errors'; import { getMetaMaskProviderEIP6963 } from '../utils/eip-6963-metamask-provider'; import { MESSAGE_SIGNING_SNAP, @@ -30,6 +31,9 @@ import { validateLoginResponse } from '../utils/validate-login-response'; type JwtBearerAuth_SRP_Options = { storage: AuthStorageOptions; signing?: AuthSigningOptions; + rateLimitRetry?: { + cooldownDefaultMs?: number; // default cooldown when 429 has no Retry-After + }; }; const getDefaultEIP6963Provider = async () => { @@ -64,13 +68,19 @@ const getDefaultEIP6963SigningOptions = ( export class SRPJwtBearerAuth implements IBaseAuth { readonly #config: AuthConfig; - readonly #options: Required; + readonly #options: { + storage: AuthStorageOptions; + signing: AuthSigningOptions; + }; readonly #metametrics?: MetaMetricsAuth; // Map to store ongoing login promises by entropySourceId readonly #ongoingLogins = new Map>(); + // Default cooldown when 429 has no Retry-After header + readonly #cooldownDefaultMs: number; + #customProvider?: Eip1193Provider; constructor( @@ -89,6 +99,10 @@ export class SRPJwtBearerAuth implements IBaseAuth { getDefaultEIP6963SigningOptions(this.#customProvider), }; this.#metametrics = options.metametrics; + + // Apply rate limit retry config if provided + this.#cooldownDefaultMs = + options.rateLimitRetry?.cooldownDefaultMs ?? 10000; } setCustomProvider(provider: Eip1193Provider) { @@ -225,7 +239,7 @@ export class SRPJwtBearerAuth implements IBaseAuth { } // Create a new login promise - const loginPromise = this.#performLogin(entropySourceId); + const loginPromise = this.#loginWithRetry(entropySourceId); // Store the promise in the map this.#ongoingLogins.set(loginKey, loginPromise); @@ -240,6 +254,35 @@ export class SRPJwtBearerAuth implements IBaseAuth { } } + async #loginWithRetry(entropySourceId?: string): Promise { + // Allow max 2 attempts: initial + one retry on 429 + for (let attempt = 0; attempt < 2; attempt += 1) { + try { + return await this.#performLogin(entropySourceId); + } catch (e) { + // Only retry on rate-limit (429) errors + if (!RateLimitedError.isRateLimitError(e)) { + throw e; + } + + // If we've exhausted attempts (>= 1 retry), rethrow + if (attempt >= 1) { + throw e; + } + + // Wait for Retry-After or default cooldown + const waitMs = + (e as RateLimitedError).retryAfterMs ?? this.#cooldownDefaultMs; + await timeUtils.delay(waitMs); + + // Loop continues to retry + } + } + + // Should never reach here due to loop logic, but TypeScript needs a return + throw new Error('Unexpected: login loop exhausted without result'); + } + #createSrpLoginRawMessage( nonce: string, publicKey: string, diff --git a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/services.ts b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/services.ts index 3bc4c91265f..6bf123824c8 100644 --- a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/services.ts +++ b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/services.ts @@ -13,8 +13,69 @@ import { PairError, SignInError, ValidationError, + RateLimitedError, } from '../errors'; +/** + * Parse Retry-After header into milliseconds if possible. + * Supports seconds or HTTP-date formats. + * + * @param retryAfterHeader - The Retry-After header value (seconds or HTTP-date) + * @returns The retry delay in milliseconds, or null if parsing fails + */ +function parseRetryAfter(retryAfterHeader: string | null): number | null { + if (!retryAfterHeader) { + return null; + } + const seconds = Number(retryAfterHeader); + if (!Number.isNaN(seconds)) { + return seconds * 1000; + } + const date = Date.parse(retryAfterHeader); + if (!Number.isNaN(date)) { + const diff = date - Date.now(); + return diff > 0 ? diff : null; + } + return null; +} + +/** + * Handle HTTP error responses with rate limiting support. + * + * @param response - The HTTP response object + * @param errorPrefix - Optional prefix for the error message + * @throws RateLimitedError for 429 responses + * @throws Error for other error responses + */ +async function handleErrorResponse( + response: Response, + errorPrefix?: string, +): Promise { + const { status } = response; + const retryAfterHeader = response.headers.get('Retry-After'); + const retryAfterMs = parseRetryAfter(retryAfterHeader); + + const responseBody = (await response.json()) as + | ErrorMessage + | { error_description: string; error: string }; + + const message = + 'message' in responseBody + ? responseBody.message + : responseBody.error_description; + const { error } = responseBody; + + if (status === 429) { + throw new RateLimitedError( + `HTTP 429: ${message} (error: ${error})`, + retryAfterMs ?? undefined, + ); + } + + const prefix = errorPrefix ? `${errorPrefix} ` : ''; + throw new Error(`${prefix}HTTP ${status} error: ${message}, error: ${error}`); +} + export const NONCE_URL = (env: Env) => `${getEnvUrls(env).authApiUrl}/api/v2/nonce`; @@ -91,12 +152,13 @@ export async function pairIdentifiers( }); if (!response.ok) { - const responseBody = (await response.json()) as ErrorMessage; - throw new Error( - `HTTP error message: ${responseBody.message}, error: ${responseBody.error}`, - ); + await handleErrorResponse(response); } } catch (e) { + // Re-throw RateLimitedError to preserve 429 status and retry metadata + if (RateLimitedError.isRateLimitError(e)) { + throw e; + } /* istanbul ignore next */ const errorMessage = e instanceof Error ? e.message : JSON.stringify(e ?? ''); @@ -118,10 +180,7 @@ export async function getNonce(id: string, env: Env): Promise { try { const nonceResponse = await fetch(nonceUrl.toString()); if (!nonceResponse.ok) { - const responseBody = (await nonceResponse.json()) as ErrorMessage; - throw new Error( - `HTTP error message: ${responseBody.message}, error: ${responseBody.error}`, - ); + await handleErrorResponse(nonceResponse); } const nonceJson = await nonceResponse.json(); @@ -131,6 +190,10 @@ export async function getNonce(id: string, env: Env): Promise { expiresIn: nonceJson.expires_in, }; } catch (e) { + // Re-throw RateLimitedError to preserve 429 status and retry metadata + if (RateLimitedError.isRateLimitError(e)) { + throw e; + } /* istanbul ignore next */ const errorMessage = e instanceof Error ? e.message : JSON.stringify(e ?? ''); @@ -169,13 +232,7 @@ export async function authorizeOIDC( }); if (!response.ok) { - const responseBody = (await response.json()) as { - error_description: string; - error: string; - }; - throw new Error( - `HTTP error: ${responseBody.error_description}, error code: ${responseBody.error}`, - ); + await handleErrorResponse(response); } const accessTokenResponse = await response.json(); @@ -185,6 +242,10 @@ export async function authorizeOIDC( obtainedAt: Date.now(), }; } catch (e) { + // Re-throw RateLimitedError to preserve 429 status and retry metadata + if (RateLimitedError.isRateLimitError(e)) { + throw e; + } /* istanbul ignore next */ const errorMessage = e instanceof Error ? e.message : JSON.stringify(e ?? ''); @@ -237,10 +298,7 @@ export async function authenticate( }); if (!response.ok) { - const responseBody = (await response.json()) as ErrorMessage; - throw new Error( - `${authType} login HTTP error: ${responseBody.message}, error code: ${responseBody.error}`, - ); + await handleErrorResponse(response, `${authType} login`); } const loginResponse = await response.json(); @@ -254,6 +312,10 @@ export async function authenticate( }, }; } catch (e) { + // Re-throw RateLimitedError to preserve 429 status and retry metadata + if (RateLimitedError.isRateLimitError(e)) { + throw e; + } /* istanbul ignore next */ const errorMessage = e instanceof Error ? e.message : JSON.stringify(e ?? ''); @@ -283,16 +345,17 @@ export async function getUserProfileLineage( }); if (!response.ok) { - const responseBody = (await response.json()) as ErrorMessage; - throw new Error( - `HTTP error message: ${responseBody.message}, error: ${responseBody.error}`, - ); + await handleErrorResponse(response, 'profile lineage'); } const profileJson: UserProfileLineage = await response.json(); return profileJson; } catch (e) { + // Re-throw RateLimitedError to preserve 429 status and retry metadata + if (RateLimitedError.isRateLimitError(e)) { + throw e; + } /* istanbul ignore next */ const errorMessage = e instanceof Error ? e.message : JSON.stringify(e ?? ''); diff --git a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/utils/time.ts b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/utils/time.ts new file mode 100644 index 00000000000..80d95f84347 --- /dev/null +++ b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/utils/time.ts @@ -0,0 +1,12 @@ +/** + * Delays execution for the specified number of milliseconds. + * + * @param ms - Number of milliseconds to delay + * @returns Promise that resolves after the delay + */ +export async function delay(ms: number): Promise { + if (ms <= 0) { + return; + } + await new Promise((resolve) => setTimeout(resolve, ms)); +} diff --git a/packages/profile-sync-controller/src/sdk/errors.ts b/packages/profile-sync-controller/src/sdk/errors.ts index 40ce5bc7788..0b7dbe35666 100644 --- a/packages/profile-sync-controller/src/sdk/errors.ts +++ b/packages/profile-sync-controller/src/sdk/errors.ts @@ -46,3 +46,31 @@ export class NotFoundError extends Error { this.name = 'NotFoundError'; } } + +export class RateLimitedError extends Error { + readonly status = 429; + + readonly retryAfterMs?: number; + + constructor(message: string, retryAfterMs?: number) { + super(message); + this.name = 'RateLimitedError'; + this.retryAfterMs = retryAfterMs; + } + + /** + * Check if an unknown error is a rate limit error (429 status). + * + * @param e - The error to check + * @returns True if the error is a rate limit error + */ + static isRateLimitError(e: unknown): e is RateLimitedError { + return ( + e instanceof RateLimitedError || + (typeof e === 'object' && + e !== null && + 'status' in e && + (e as { status: number }).status === 429) + ); + } +}