From 3c6fa6e333e27f7e01c0d842b71eeef24046d284 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Mon, 26 Jan 2026 11:41:36 +0100 Subject: [PATCH 1/7] Handle non-JSON error responses in authentication services --- packages/profile-sync-controller/CHANGELOG.md | 5 + .../services.test.ts | 757 ++++++++++++++++++ .../sdk/authentication-jwt-bearer/services.ts | 150 ++-- 3 files changed, 838 insertions(+), 74 deletions(-) create mode 100644 packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/services.test.ts diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index d94b874157e..e187534ae59 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Centralize authentication error handling into a single `handleServiceError` helper for consistent error management across all service functions ([#XXXX](https://github.com/MetaMask/core/pull/XXXX)) + - This fixes authentication services crashing when server returns non-JSON error responses. + ### Changed - Bump `@metamask/snaps-controllers` from `^14.0.1` to `^17.2.0` ([#7550](https://github.com/MetaMask/core/pull/7550)) diff --git a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/services.test.ts b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/services.test.ts new file mode 100644 index 00000000000..e74f17b0441 --- /dev/null +++ b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/services.test.ts @@ -0,0 +1,757 @@ +import { + getNonce, + authenticate, + authorizeOIDC, + pairIdentifiers, + getUserProfileLineage, + NONCE_URL, + OIDC_TOKEN_URL, + SRP_LOGIN_URL, + SIWE_LOGIN_URL, + PAIR_IDENTIFIERS, + PROFILE_LINEAGE_URL, +} from './services'; +import { AuthType } from './types'; +import { Env, Platform } from '../../shared/env'; +import { + NonceRetrievalError, + SignInError, + PairError, + RateLimitedError, +} from '../errors'; + +// Mock global fetch +const mockFetch = jest.fn(); +global.fetch = mockFetch; + +// Store original Response +const OriginalResponse = global.Response; + +// Create mock responses that pass instanceof checks +const createMockResponse = ( + body: unknown, + options: { + ok?: boolean; + status?: number; + headers?: Record; + jsonShouldFail?: boolean; + textShouldFail?: boolean; + } = {}, +): Response => { + const { + ok = true, + status = 200, + headers = {}, + jsonShouldFail = false, + textShouldFail = false, + } = options; + + const textValue = typeof body === 'string' ? body : JSON.stringify(body); + + const mockResponse = { + ok, + status, + headers: new Headers(headers), + json: async () => { + if (jsonShouldFail) { + throw new SyntaxError('Unexpected token'); + } + return body; + }, + text: async () => { + if (textShouldFail) { + throw new Error('Text read error'); + } + return textValue; + }, + clone: function (): Response { + return createMockResponse(body, options); + }, + }; + + // Make it pass instanceof Response check + Object.setPrototypeOf(mockResponse, OriginalResponse.prototype); + return mockResponse as Response; +}; + +describe('services', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + describe('URL builders', () => { + it('should build correct NONCE_URL', () => { + expect(NONCE_URL(Env.DEV)).toBe( + 'https://authentication.dev-api.cx.metamask.io/api/v2/nonce', + ); + }); + + it('should build correct OIDC_TOKEN_URL', () => { + expect(OIDC_TOKEN_URL(Env.DEV)).toBe( + 'https://oidc.dev-api.cx.metamask.io/oauth2/token', + ); + }); + + it('should build correct SRP_LOGIN_URL', () => { + expect(SRP_LOGIN_URL(Env.DEV)).toBe( + 'https://authentication.dev-api.cx.metamask.io/api/v2/srp/login', + ); + }); + + it('should build correct SIWE_LOGIN_URL', () => { + expect(SIWE_LOGIN_URL(Env.DEV)).toBe( + 'https://authentication.dev-api.cx.metamask.io/api/v2/siwe/login', + ); + }); + + it('should build correct PAIR_IDENTIFIERS', () => { + expect(PAIR_IDENTIFIERS(Env.DEV)).toBe( + 'https://authentication.dev-api.cx.metamask.io/api/v2/identifiers/pair', + ); + }); + + it('should build correct PROFILE_LINEAGE_URL', () => { + expect(PROFILE_LINEAGE_URL(Env.DEV)).toBe( + 'https://authentication.dev-api.cx.metamask.io/api/v2/profile/lineage', + ); + }); + }); + + describe('getNonce', () => { + it('should return nonce data on success', async () => { + const mockResponse = createMockResponse({ + nonce: 'test-nonce', + identifier: 'test-identifier', + expires_in: 3600, + }); + mockFetch.mockResolvedValue(mockResponse); + + const result = await getNonce('test-id', Env.DEV); + + expect(result).toEqual({ + nonce: 'test-nonce', + identifier: 'test-identifier', + expiresIn: 3600, + }); + expect(mockFetch).toHaveBeenCalledWith( + expect.stringContaining('identifier=test-id'), + ); + }); + + it('should throw NonceRetrievalError on network failure', async () => { + mockFetch.mockRejectedValue(new Error('Network error')); + + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + NonceRetrievalError, + ); + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + 'Failed to get nonce: Network error', + ); + }); + + it('should throw NonceRetrievalError with HTTP status on error response with JSON body', async () => { + const mockResponse = createMockResponse( + { message: 'Invalid identifier', error: 'invalid_request' }, + { ok: false, status: 400 }, + ); + mockFetch.mockResolvedValue(mockResponse); + + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + NonceRetrievalError, + ); + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + 'Failed to get nonce: HTTP 400 - Invalid identifier (error: invalid_request)', + ); + }); + + it('should throw NonceRetrievalError with text body when JSON parsing fails', async () => { + const mockResponse = createMockResponse('Bad Gateway Error', { + ok: false, + status: 502, + jsonShouldFail: true, + }); + mockFetch.mockResolvedValue(mockResponse); + + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + NonceRetrievalError, + ); + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + 'Failed to get nonce: HTTP 502 - Bad Gateway Error (error: non_json_response)', + ); + }); + + it('should throw NonceRetrievalError with fallback message when response is unparseable', async () => { + const mockResponse = createMockResponse('', { + ok: false, + status: 500, + jsonShouldFail: true, + textShouldFail: true, + }); + mockFetch.mockResolvedValue(mockResponse); + + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + NonceRetrievalError, + ); + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + 'Failed to get nonce: HTTP 500 - Unable to parse error response (error: unparseable_response)', + ); + }); + + it('should throw RateLimitedError on 429 response', async () => { + const mockResponse = createMockResponse( + { message: 'Too many requests', error: 'rate_limited' }, + { ok: false, status: 429, headers: { 'Retry-After': '60' } }, + ); + mockFetch.mockResolvedValue(mockResponse); + + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + RateLimitedError, + ); + const error = await getNonce('test-id', Env.DEV).catch((e) => e); + expect(error.retryAfterMs).toBe(60000); + }); + + it('should throw RateLimitedError with HTTP-date Retry-After header', async () => { + const futureDate = new Date(Date.now() + 30000).toUTCString(); + const mockResponse = createMockResponse( + { message: 'Too many requests', error: 'rate_limited' }, + { ok: false, status: 429, headers: { 'Retry-After': futureDate } }, + ); + mockFetch.mockResolvedValue(mockResponse); + + const error = await getNonce('test-id', Env.DEV).catch((e) => e); + expect(error).toBeInstanceOf(RateLimitedError); + expect(error.retryAfterMs).toBeGreaterThan(0); + expect(error.retryAfterMs).toBeLessThanOrEqual(30000); + }); + + it('should throw RateLimitedError without retryAfterMs when header is missing', async () => { + const mockResponse = createMockResponse( + { message: 'Too many requests', error: 'rate_limited' }, + { ok: false, status: 429 }, + ); + mockFetch.mockResolvedValue(mockResponse); + + const error = await getNonce('test-id', Env.DEV).catch((e) => e); + expect(error).toBeInstanceOf(RateLimitedError); + expect(error.retryAfterMs).toBeUndefined(); + }); + + it('should throw NonceRetrievalError when success response has invalid JSON', async () => { + const mockResponse = createMockResponse( + {}, + { + ok: true, + status: 200, + jsonShouldFail: true, + }, + ); + mockFetch.mockResolvedValue(mockResponse); + + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + NonceRetrievalError, + ); + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + 'Failed to get nonce: Unexpected token', + ); + }); + + it('should handle error_description format in error response', async () => { + const mockResponse = createMockResponse( + { + error_description: 'The identifier is invalid', + error: 'invalid_identifier', + }, + { ok: false, status: 400 }, + ); + mockFetch.mockResolvedValue(mockResponse); + + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + 'Failed to get nonce: HTTP 400 - The identifier is invalid (error: invalid_identifier)', + ); + }); + + it('should truncate long text responses', async () => { + const longText = 'A'.repeat(200); + const mockResponse = createMockResponse(longText, { + ok: false, + status: 500, + jsonShouldFail: true, + }); + mockFetch.mockResolvedValue(mockResponse); + + const error = await getNonce('test-id', Env.DEV).catch((e) => e); + expect(error.message).toContain('A'.repeat(150)); + expect(error.message.length).toBeLessThan(250); + }); + }); + + describe('authenticate', () => { + const mockAuthResponse = { + token: 'jwt-token', + expires_in: 3600, + profile: { + identifier_id: 'id-1', + metametrics_id: 'mm-1', + profile_id: 'profile-1', + }, + }; + + it('should return authentication data on success with SRP', async () => { + const mockResponse = createMockResponse(mockAuthResponse); + mockFetch.mockResolvedValue(mockResponse); + + const result = await authenticate( + 'raw-message', + 'signature', + AuthType.SRP, + Env.DEV, + ); + + expect(result).toEqual({ + token: 'jwt-token', + expiresIn: 3600, + profile: { + identifierId: 'id-1', + metaMetricsId: 'mm-1', + profileId: 'profile-1', + }, + }); + expect(mockFetch).toHaveBeenCalledWith( + expect.stringContaining('/srp/login'), + expect.objectContaining({ + method: 'POST', + body: JSON.stringify({ + signature: 'signature', + raw_message: 'raw-message', + }), + }), + ); + }); + + it('should return authentication data on success with SiWE', async () => { + const mockResponse = createMockResponse(mockAuthResponse); + mockFetch.mockResolvedValue(mockResponse); + + await authenticate('raw-message', 'signature', AuthType.SiWE, Env.DEV); + + expect(mockFetch).toHaveBeenCalledWith( + expect.stringContaining('/siwe/login'), + expect.any(Object), + ); + }); + + it('should include metametrics when provided', async () => { + const mockResponse = createMockResponse(mockAuthResponse); + mockFetch.mockResolvedValue(mockResponse); + + const mockMetametrics = { + getMetaMetricsId: jest.fn().mockResolvedValue('mm-id'), + agent: Platform.EXTENSION as Platform.EXTENSION, + }; + + await authenticate( + 'raw-message', + 'signature', + AuthType.SRP, + Env.DEV, + mockMetametrics, + ); + + expect(mockFetch).toHaveBeenCalledWith( + expect.any(String), + expect.objectContaining({ + body: JSON.stringify({ + signature: 'signature', + raw_message: 'raw-message', + metametrics: { + metametrics_id: 'mm-id', + agent: Platform.EXTENSION, + }, + }), + }), + ); + }); + + it('should throw SignInError on network failure', async () => { + mockFetch.mockRejectedValue(new Error('Connection refused')); + + await expect( + authenticate('raw-message', 'signature', AuthType.SRP, Env.DEV), + ).rejects.toThrow(SignInError); + await expect( + authenticate('raw-message', 'signature', AuthType.SRP, Env.DEV), + ).rejects.toThrow('SRP login failed: Connection refused'); + }); + + it('should throw SignInError on error response', async () => { + const mockResponse = createMockResponse( + { message: 'Invalid signature', error: 'auth_failed' }, + { ok: false, status: 401 }, + ); + mockFetch.mockResolvedValue(mockResponse); + + await expect( + authenticate('raw-message', 'signature', AuthType.SRP, Env.DEV), + ).rejects.toThrow(SignInError); + await expect( + authenticate('raw-message', 'signature', AuthType.SRP, Env.DEV), + ).rejects.toThrow( + 'SRP login failed: HTTP 401 - Invalid signature (error: auth_failed)', + ); + }); + + it('should throw RateLimitedError on 429 response', async () => { + const mockResponse = createMockResponse( + { message: 'Rate limited', error: 'too_many_requests' }, + { ok: false, status: 429, headers: { 'Retry-After': '120' } }, + ); + mockFetch.mockResolvedValue(mockResponse); + + const error = await authenticate( + 'raw-message', + 'signature', + AuthType.SRP, + Env.DEV, + ).catch((e) => e); + + expect(error).toBeInstanceOf(RateLimitedError); + expect(error.retryAfterMs).toBe(120000); + }); + }); + + describe('authorizeOIDC', () => { + const mockOIDCResponse = { + access_token: 'access-token-123', + expires_in: 7200, + }; + + it('should return access token on success', async () => { + const mockResponse = createMockResponse(mockOIDCResponse); + mockFetch.mockResolvedValue(mockResponse); + + const before = Date.now(); + const result = await authorizeOIDC( + 'jwt-token', + Env.DEV, + Platform.EXTENSION, + ); + const after = Date.now(); + + expect(result.accessToken).toBe('access-token-123'); + expect(result.expiresIn).toBe(7200); + expect(result.obtainedAt).toBeGreaterThanOrEqual(before); + expect(result.obtainedAt).toBeLessThanOrEqual(after); + }); + + it('should send correct request body', async () => { + const mockResponse = createMockResponse(mockOIDCResponse); + mockFetch.mockResolvedValue(mockResponse); + + await authorizeOIDC('jwt-token', Env.DEV, Platform.EXTENSION); + + expect(mockFetch).toHaveBeenCalledWith( + expect.stringContaining('/oauth2/token'), + expect.objectContaining({ + method: 'POST', + headers: expect.any(Headers), + }), + ); + + const callArgs = mockFetch.mock.calls[0]; + const body = callArgs[1].body; + expect(body).toContain( + 'grant_type=urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Ajwt-bearer', + ); + expect(body).toContain('assertion=jwt-token'); + }); + + it('should throw SignInError on network failure', async () => { + mockFetch.mockRejectedValue(new Error('CORS error')); + + await expect( + authorizeOIDC('jwt-token', Env.DEV, Platform.EXTENSION), + ).rejects.toThrow(SignInError); + await expect( + authorizeOIDC('jwt-token', Env.DEV, Platform.EXTENSION), + ).rejects.toThrow('Unable to get access token: CORS error'); + }); + + it('should throw SignInError on error response', async () => { + const mockResponse = createMockResponse( + { error_description: 'Invalid assertion', error: 'invalid_grant' }, + { ok: false, status: 400 }, + ); + mockFetch.mockResolvedValue(mockResponse); + + await expect( + authorizeOIDC('jwt-token', Env.DEV, Platform.EXTENSION), + ).rejects.toThrow(SignInError); + await expect( + authorizeOIDC('jwt-token', Env.DEV, Platform.EXTENSION), + ).rejects.toThrow( + 'Unable to get access token: HTTP 400 - Invalid assertion (error: invalid_grant)', + ); + }); + + it('should throw RateLimitedError on 429 response', async () => { + const mockResponse = createMockResponse( + { message: 'Rate limited', error: 'too_many_requests' }, + { ok: false, status: 429 }, + ); + mockFetch.mockResolvedValue(mockResponse); + + await expect( + authorizeOIDC('jwt-token', Env.DEV, Platform.EXTENSION), + ).rejects.toThrow(RateLimitedError); + }); + }); + + describe('pairIdentifiers', () => { + const mockLogins = [ + { + signature: 'sig-1', + raw_message: 'msg-1', + encrypted_storage_key: 'key-1', + identifier_type: 'SRP' as const, + }, + ]; + + it('should complete successfully on 200 response', async () => { + const mockResponse = createMockResponse({}, { ok: true, status: 200 }); + mockFetch.mockResolvedValue(mockResponse); + + await expect( + pairIdentifiers('nonce-123', mockLogins, 'access-token', Env.DEV), + ).resolves.toBeUndefined(); + + expect(mockFetch).toHaveBeenCalledWith( + expect.any(URL), + expect.objectContaining({ + method: 'POST', + headers: { + 'Content-Type': 'application/json', + Authorization: 'Bearer access-token', + }, + body: JSON.stringify({ + nonce: 'nonce-123', + logins: mockLogins, + }), + }), + ); + }); + + it('should throw PairError on network failure', async () => { + mockFetch.mockRejectedValue(new Error('Network timeout')); + + await expect( + pairIdentifiers('nonce-123', mockLogins, 'access-token', Env.DEV), + ).rejects.toThrow(PairError); + await expect( + pairIdentifiers('nonce-123', mockLogins, 'access-token', Env.DEV), + ).rejects.toThrow('Unable to pair identifiers: Network timeout'); + }); + + it('should throw PairError on error response', async () => { + const mockResponse = createMockResponse( + { message: 'Invalid nonce', error: 'invalid_request' }, + { ok: false, status: 400 }, + ); + mockFetch.mockResolvedValue(mockResponse); + + await expect( + pairIdentifiers('nonce-123', mockLogins, 'access-token', Env.DEV), + ).rejects.toThrow(PairError); + await expect( + pairIdentifiers('nonce-123', mockLogins, 'access-token', Env.DEV), + ).rejects.toThrow( + 'Unable to pair identifiers: HTTP 400 - Invalid nonce (error: invalid_request)', + ); + }); + + it('should throw RateLimitedError on 429 response', async () => { + const mockResponse = createMockResponse( + { message: 'Rate limited', error: 'too_many_requests' }, + { ok: false, status: 429, headers: { 'Retry-After': '30' } }, + ); + mockFetch.mockResolvedValue(mockResponse); + + const error = await pairIdentifiers( + 'nonce-123', + mockLogins, + 'access-token', + Env.DEV, + ).catch((e) => e); + + expect(error).toBeInstanceOf(RateLimitedError); + expect(error.retryAfterMs).toBe(30000); + }); + }); + + describe('getUserProfileLineage', () => { + const mockLineageResponse = { + profile_id: 'profile-123', + created_at: '2024-01-01T00:00:00Z', + lineage: [ + { + metametrics_id: 'mm-1', + agent: Platform.EXTENSION, + created_at: '2024-01-01T00:00:00Z', + updated_at: '2024-01-02T00:00:00Z', + counter: 5, + }, + ], + }; + + it('should return profile lineage on success', async () => { + const mockResponse = createMockResponse(mockLineageResponse); + mockFetch.mockResolvedValue(mockResponse); + + const result = await getUserProfileLineage(Env.DEV, 'access-token'); + + expect(result).toEqual(mockLineageResponse); + expect(mockFetch).toHaveBeenCalledWith( + expect.any(URL), + expect.objectContaining({ + method: 'GET', + headers: { + Authorization: 'Bearer access-token', + }, + }), + ); + }); + + it('should throw SignInError on network failure', async () => { + mockFetch.mockRejectedValue(new Error('DNS resolution failed')); + + await expect( + getUserProfileLineage(Env.DEV, 'access-token'), + ).rejects.toThrow(SignInError); + await expect( + getUserProfileLineage(Env.DEV, 'access-token'), + ).rejects.toThrow('Failed to get profile lineage: DNS resolution failed'); + }); + + it('should throw SignInError on error response', async () => { + const mockResponse = createMockResponse( + { message: 'Unauthorized', error: 'invalid_token' }, + { ok: false, status: 401 }, + ); + mockFetch.mockResolvedValue(mockResponse); + + await expect( + getUserProfileLineage(Env.DEV, 'access-token'), + ).rejects.toThrow(SignInError); + await expect( + getUserProfileLineage(Env.DEV, 'access-token'), + ).rejects.toThrow( + 'Failed to get profile lineage: HTTP 401 - Unauthorized (error: invalid_token)', + ); + }); + + it('should throw RateLimitedError on 429 response', async () => { + const mockResponse = createMockResponse( + { message: 'Rate limited', error: 'too_many_requests' }, + { ok: false, status: 429 }, + ); + mockFetch.mockResolvedValue(mockResponse); + + await expect( + getUserProfileLineage(Env.DEV, 'access-token'), + ).rejects.toThrow(RateLimitedError); + }); + + it('should handle non-JSON error response', async () => { + const mockResponse = createMockResponse('Service Unavailable', { + ok: false, + status: 503, + jsonShouldFail: true, + }); + mockFetch.mockResolvedValue(mockResponse); + + await expect( + getUserProfileLineage(Env.DEV, 'access-token'), + ).rejects.toThrow(SignInError); + await expect( + getUserProfileLineage(Env.DEV, 'access-token'), + ).rejects.toThrow( + 'Failed to get profile lineage: HTTP 503 - Service Unavailable (error: non_json_response)', + ); + }); + }); + + describe('parseRetryAfter edge cases', () => { + it('should handle past HTTP-date by returning null (no delay)', async () => { + const pastDate = new Date(Date.now() - 10000).toUTCString(); + const mockResponse = createMockResponse( + { message: 'Rate limited', error: 'rate_limited' }, + { ok: false, status: 429, headers: { 'Retry-After': pastDate } }, + ); + mockFetch.mockResolvedValue(mockResponse); + + const error = await getNonce('test-id', Env.DEV).catch((e) => e); + expect(error).toBeInstanceOf(RateLimitedError); + expect(error.retryAfterMs).toBeUndefined(); + }); + + it('should handle invalid Retry-After header', async () => { + const mockResponse = createMockResponse( + { message: 'Rate limited', error: 'rate_limited' }, + { ok: false, status: 429, headers: { 'Retry-After': 'invalid-value' } }, + ); + mockFetch.mockResolvedValue(mockResponse); + + const error = await getNonce('test-id', Env.DEV).catch((e) => e); + expect(error).toBeInstanceOf(RateLimitedError); + expect(error.retryAfterMs).toBeUndefined(); + }); + }); + + describe('handleServiceError edge cases', () => { + it('should handle non-Error thrown values', async () => { + mockFetch.mockRejectedValue('string error'); + + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + NonceRetrievalError, + ); + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + 'Failed to get nonce: string error', + ); + }); + + it('should handle null thrown values', async () => { + mockFetch.mockRejectedValue(null); + + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + NonceRetrievalError, + ); + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + 'Failed to get nonce: null', + ); + }); + + it('should handle undefined thrown values', async () => { + mockFetch.mockRejectedValue(undefined); + + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + NonceRetrievalError, + ); + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + 'Failed to get nonce: undefined', + ); + }); + + it('should handle empty text response', async () => { + const mockResponse = createMockResponse('', { + ok: false, + status: 500, + jsonShouldFail: true, + }); + mockFetch.mockResolvedValue(mockResponse); + + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + 'Failed to get nonce: HTTP 500 - Non-JSON error response (error: non_json_response)', + ); + }); + }); +}); 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 1659b281523..75fa51e6e08 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 @@ -41,40 +41,78 @@ function parseRetryAfter(retryAfterHeader: string | null): number | null { } /** - * Handle HTTP error responses with rate limiting support. + * Extracts error details from a Response object. * * @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 + * @returns Formatted error message with HTTP status and response body */ -async function handleErrorResponse( - response: Response, - errorPrefix?: string, -): Promise { +async function getResponseErrorMessage(response: Response): 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 === HTTP_STATUS_CODES.TOO_MANY_REQUESTS) { - throw new RateLimitedError( - `HTTP ${HTTP_STATUS_CODES.TOO_MANY_REQUESTS}: ${message} (error: ${error})`, - retryAfterMs ?? undefined, - ); + const clonedResponse = response.clone(); + + let message = 'Unknown error'; + let error = 'unknown'; + + try { + const responseBody = (await response.json()) as + | ErrorMessage + | { error_description: string; error: string }; + + message = + 'message' in responseBody + ? responseBody.message + : responseBody.error_description; + error = responseBody.error; + } catch { + try { + const textContent = await clonedResponse.text(); + message = textContent + ? textContent.slice(0, 150) + : 'Non-JSON error response'; + error = 'non_json_response'; + } catch { + message = 'Unable to parse error response'; + error = 'unparseable_response'; + } } - const prefix = errorPrefix ? `${errorPrefix} ` : ''; - throw new Error(`${prefix}HTTP ${status} error: ${message}, error: ${error}`); + return `HTTP ${status} - ${message} (error: ${error})`; +} + +/** + * Handles any error from service calls consistently. + * Supports Response objects (non-OK HTTP responses), regular Errors, and unknown types. + * + * @param e - The caught error (can be Response, Error, or unknown) + * @param errorPrefix - Context prefix for the error message + * @param ErrorClass - The domain-specific error class to throw + * @throws The appropriate error type (RateLimitedError for 429, otherwise ErrorClass) + */ +async function handleServiceError( + e: unknown, + errorPrefix: string, + ErrorClass: new (message: string) => Error, +): Promise { + // Handle non-OK HTTP responses + if (e instanceof Response) { + const { status } = e; + const responseMessage = await getResponseErrorMessage(e); + + if (status === HTTP_STATUS_CODES.TOO_MANY_REQUESTS) { + const retryAfterHeader = e.headers.get('Retry-After'); + const retryAfterMs = parseRetryAfter(retryAfterHeader); + throw new RateLimitedError( + `${errorPrefix}: ${responseMessage}`, + retryAfterMs ?? undefined, + ); + } + + throw new ErrorClass(`${errorPrefix}: ${responseMessage}`); + } + + // Handle regular errors (network failures, JSON parsing, etc.) + const errorMessage = e instanceof Error ? e.message : String(e); + throw new ErrorClass(`${errorPrefix}: ${errorMessage}`); } export const NONCE_URL = (env: Env) => @@ -153,17 +191,10 @@ export async function pairIdentifiers( }); if (!response.ok) { - await handleErrorResponse(response); + throw 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 ?? ''); - throw new PairError(`unable to pair identifiers: ${errorMessage}`); + return handleServiceError(e, 'Unable to pair identifiers', PairError); } } @@ -181,7 +212,7 @@ export async function getNonce(id: string, env: Env): Promise { try { const nonceResponse = await fetch(nonceUrl.toString()); if (!nonceResponse.ok) { - await handleErrorResponse(nonceResponse); + throw nonceResponse; } const nonceJson = await nonceResponse.json(); @@ -191,14 +222,7 @@ 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 ?? ''); - throw new NonceRetrievalError(`failed to generate nonce: ${errorMessage}`); + return handleServiceError(e, 'Failed to get nonce', NonceRetrievalError); } } @@ -233,7 +257,7 @@ export async function authorizeOIDC( }); if (!response.ok) { - await handleErrorResponse(response); + throw response; } const accessTokenResponse = await response.json(); @@ -243,14 +267,7 @@ 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 ?? ''); - throw new SignInError(`unable to get access token: ${errorMessage}`); + return handleServiceError(e, 'Unable to get access token', SignInError); } } @@ -299,7 +316,7 @@ export async function authenticate( }); if (!response.ok) { - await handleErrorResponse(response, `${authType} login`); + throw response; } const loginResponse = await response.json(); @@ -313,14 +330,7 @@ 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 ?? ''); - throw new SignInError(`unable to perform SRP login: ${errorMessage}`); + return handleServiceError(e, `${authType} login failed`, SignInError); } } @@ -346,20 +356,12 @@ export async function getUserProfileLineage( }); if (!response.ok) { - await handleErrorResponse(response, 'profile lineage'); + throw response; } 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 ?? ''); - throw new SignInError(`failed to get profile lineage: ${errorMessage}`); + return handleServiceError(e, 'Failed to get profile lineage', SignInError); } } From a44bbdfd7fcf85078fbb662859c6abfb44c374ce Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Mon, 26 Jan 2026 11:42:44 +0100 Subject: [PATCH 2/7] fix: update CHANGELOG --- packages/profile-sync-controller/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index e187534ae59..5284bf42247 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Centralize authentication error handling into a single `handleServiceError` helper for consistent error management across all service functions ([#XXXX](https://github.com/MetaMask/core/pull/XXXX)) +- Centralize authentication error handling into a single `handleServiceError` helper for consistent error management across all service functions ([#7721](https://github.com/MetaMask/core/pull/7721)) - This fixes authentication services crashing when server returns non-JSON error responses. ### Changed From 55ce45a11bc4cbe647a9ad7844f343e76ba19bed Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Mon, 26 Jan 2026 11:49:06 +0100 Subject: [PATCH 3/7] fix: update CHANGELOG --- packages/profile-sync-controller/CHANGELOG.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index 5284bf42247..d0594631948 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -7,13 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Fixed +### Changed - Centralize authentication error handling into a single `handleServiceError` helper for consistent error management across all service functions ([#7721](https://github.com/MetaMask/core/pull/7721)) - This fixes authentication services crashing when server returns non-JSON error responses. - -### Changed - - Bump `@metamask/snaps-controllers` from `^14.0.1` to `^17.2.0` ([#7550](https://github.com/MetaMask/core/pull/7550)) - Bump `@metamask/snaps-sdk` from `^9.0.0` to `^10.3.0` ([#7550](https://github.com/MetaMask/core/pull/7550)) - Bump `@metamask/snaps-utils` from `^11.0.0` to `^11.7.0` ([#7550](https://github.com/MetaMask/core/pull/7550)) From 30eb25740ec9844e75d164d27e8b9101a4b1c295 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Mon, 26 Jan 2026 14:24:26 +0100 Subject: [PATCH 4/7] fix: lint errors & logic choices --- packages/profile-sync-controller/CHANGELOG.md | 2 +- .../services.test.ts | 69 ++++++--- .../sdk/authentication-jwt-bearer/services.ts | 135 ++++++++++++------ 3 files changed, 138 insertions(+), 68 deletions(-) diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index d0594631948..689a771ebc8 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- Centralize authentication error handling into a single `handleServiceError` helper for consistent error management across all service functions ([#7721](https://github.com/MetaMask/core/pull/7721)) +- Centralize authentication error handling into a single `throwServiceError` helper for consistent error management across all service functions ([#7721](https://github.com/MetaMask/core/pull/7721)) - This fixes authentication services crashing when server returns non-JSON error responses. - Bump `@metamask/snaps-controllers` from `^14.0.1` to `^17.2.0` ([#7550](https://github.com/MetaMask/core/pull/7550)) - Bump `@metamask/snaps-sdk` from `^9.0.0` to `^10.3.0` ([#7550](https://github.com/MetaMask/core/pull/7550)) diff --git a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/services.test.ts b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/services.test.ts index e74f17b0441..dd3f60b432e 100644 --- a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/services.test.ts +++ b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/services.test.ts @@ -48,30 +48,41 @@ const createMockResponse = ( const textValue = typeof body === 'string' ? body : JSON.stringify(body); + // Create a Headers-like object with get() method + const headersObj = { + ...headers, + get: (name: string): string | null => { + const key = Object.keys(headers).find( + (k) => k.toLowerCase() === name.toLowerCase(), + ); + return key ? headers[key] : null; + }, + }; + const mockResponse = { ok, status, - headers: new Headers(headers), - json: async () => { + headers: headersObj, + json: async (): Promise => { if (jsonShouldFail) { throw new SyntaxError('Unexpected token'); } return body; }, - text: async () => { + text: async (): Promise => { if (textShouldFail) { throw new Error('Text read error'); } return textValue; }, - clone: function (): Response { + clone: (): Response => { return createMockResponse(body, options); }, }; // Make it pass instanceof Response check Object.setPrototypeOf(mockResponse, OriginalResponse.prototype); - return mockResponse as Response; + return mockResponse as unknown as Response; }; describe('services', () => { @@ -128,7 +139,7 @@ describe('services', () => { const result = await getNonce('test-id', Env.DEV); - expect(result).toEqual({ + expect(result).toStrictEqual({ nonce: 'test-nonce', identifier: 'test-identifier', expiresIn: 3600, @@ -207,7 +218,9 @@ describe('services', () => { await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( RateLimitedError, ); - const error = await getNonce('test-id', Env.DEV).catch((e) => e); + const error = await getNonce('test-id', Env.DEV).catch( + (caughtError) => caughtError, + ); expect(error.retryAfterMs).toBe(60000); }); @@ -219,7 +232,9 @@ describe('services', () => { ); mockFetch.mockResolvedValue(mockResponse); - const error = await getNonce('test-id', Env.DEV).catch((e) => e); + const error = await getNonce('test-id', Env.DEV).catch( + (caughtError) => caughtError, + ); expect(error).toBeInstanceOf(RateLimitedError); expect(error.retryAfterMs).toBeGreaterThan(0); expect(error.retryAfterMs).toBeLessThanOrEqual(30000); @@ -232,7 +247,9 @@ describe('services', () => { ); mockFetch.mockResolvedValue(mockResponse); - const error = await getNonce('test-id', Env.DEV).catch((e) => e); + const error = await getNonce('test-id', Env.DEV).catch( + (caughtError) => caughtError, + ); expect(error).toBeInstanceOf(RateLimitedError); expect(error.retryAfterMs).toBeUndefined(); }); @@ -280,7 +297,9 @@ describe('services', () => { }); mockFetch.mockResolvedValue(mockResponse); - const error = await getNonce('test-id', Env.DEV).catch((e) => e); + const error = await getNonce('test-id', Env.DEV).catch( + (caughtError) => caughtError, + ); expect(error.message).toContain('A'.repeat(150)); expect(error.message.length).toBeLessThan(250); }); @@ -308,7 +327,7 @@ describe('services', () => { Env.DEV, ); - expect(result).toEqual({ + expect(result).toStrictEqual({ token: 'jwt-token', expiresIn: 3600, profile: { @@ -413,7 +432,7 @@ describe('services', () => { 'signature', AuthType.SRP, Env.DEV, - ).catch((e) => e); + ).catch((caughtError) => caughtError); expect(error).toBeInstanceOf(RateLimitedError); expect(error.retryAfterMs).toBe(120000); @@ -454,12 +473,12 @@ describe('services', () => { expect.stringContaining('/oauth2/token'), expect.objectContaining({ method: 'POST', - headers: expect.any(Headers), + headers: expect.any(Object), }), ); const callArgs = mockFetch.mock.calls[0]; - const body = callArgs[1].body; + const { body } = callArgs[1]; expect(body).toContain( 'grant_type=urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Ajwt-bearer', ); @@ -521,9 +540,13 @@ describe('services', () => { const mockResponse = createMockResponse({}, { ok: true, status: 200 }); mockFetch.mockResolvedValue(mockResponse); - await expect( - pairIdentifiers('nonce-123', mockLogins, 'access-token', Env.DEV), - ).resolves.toBeUndefined(); + const result = await pairIdentifiers( + 'nonce-123', + mockLogins, + 'access-token', + Env.DEV, + ); + expect(result).toBeUndefined(); expect(mockFetch).toHaveBeenCalledWith( expect.any(URL), @@ -581,7 +604,7 @@ describe('services', () => { mockLogins, 'access-token', Env.DEV, - ).catch((e) => e); + ).catch((caughtError) => caughtError); expect(error).toBeInstanceOf(RateLimitedError); expect(error.retryAfterMs).toBe(30000); @@ -609,7 +632,7 @@ describe('services', () => { const result = await getUserProfileLineage(Env.DEV, 'access-token'); - expect(result).toEqual(mockLineageResponse); + expect(result).toStrictEqual(mockLineageResponse); expect(mockFetch).toHaveBeenCalledWith( expect.any(URL), expect.objectContaining({ @@ -689,7 +712,9 @@ describe('services', () => { ); mockFetch.mockResolvedValue(mockResponse); - const error = await getNonce('test-id', Env.DEV).catch((e) => e); + const error = await getNonce('test-id', Env.DEV).catch( + (caughtError) => caughtError, + ); expect(error).toBeInstanceOf(RateLimitedError); expect(error.retryAfterMs).toBeUndefined(); }); @@ -701,7 +726,9 @@ describe('services', () => { ); mockFetch.mockResolvedValue(mockResponse); - const error = await getNonce('test-id', Env.DEV).catch((e) => e); + const error = await getNonce('test-id', Env.DEV).catch( + (caughtError) => caughtError, + ); expect(error).toBeInstanceOf(RateLimitedError); expect(error.retryAfterMs).toBeUndefined(); }); 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 75fa51e6e08..ffd1d11412d 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 @@ -56,6 +56,7 @@ async function getResponseErrorMessage(response: Response): Promise { try { const responseBody = (await response.json()) as | ErrorMessage + // eslint-disable-next-line @typescript-eslint/naming-convention | { error_description: string; error: string }; message = @@ -80,57 +81,71 @@ async function getResponseErrorMessage(response: Response): Promise { } /** - * Handles any error from service calls consistently. - * Supports Response objects (non-OK HTTP responses), regular Errors, and unknown types. + * Type guard to check if an object is a Response-like object. * - * @param e - The caught error (can be Response, Error, or unknown) + * @param obj - The object to check + * @returns True if the object is a Response-like object, false otherwise + */ +const isErrorResponse = (obj: unknown): obj is Response => + typeof obj === 'object' && + obj !== null && + 'status' in obj && + 'headers' in obj; + +/** + * Throws a domain-specific error for service failures. + * Handles both HTTP error responses and regular errors (network failures, etc.). + * For HTTP 429, throws RateLimitedError with Retry-After header parsing. + * + * @param error - The error (Response object or caught error) * @param errorPrefix - Context prefix for the error message * @param ErrorClass - The domain-specific error class to throw - * @throws The appropriate error type (RateLimitedError for 429, otherwise ErrorClass) + * @throws RateLimitedError for 429, otherwise ErrorClass */ -async function handleServiceError( - e: unknown, +async function throwServiceError( + error: unknown, errorPrefix: string, ErrorClass: new (message: string) => Error, ): Promise { - // Handle non-OK HTTP responses - if (e instanceof Response) { - const { status } = e; - const responseMessage = await getResponseErrorMessage(e); - - if (status === HTTP_STATUS_CODES.TOO_MANY_REQUESTS) { - const retryAfterHeader = e.headers.get('Retry-After'); - const retryAfterMs = parseRetryAfter(retryAfterHeader); - throw new RateLimitedError( - `${errorPrefix}: ${responseMessage}`, - retryAfterMs ?? undefined, - ); - } + // Not a Response-like object - handle as regular error + if (!isErrorResponse(error)) { + const errorMessage = error instanceof Error ? error.message : String(error); + throw new ErrorClass(`${errorPrefix}: ${errorMessage}`); + } - throw new ErrorClass(`${errorPrefix}: ${responseMessage}`); + // Handle HTTP error response + const response = error; + const { status } = response; + const responseMessage = await getResponseErrorMessage(response); + + if (status === HTTP_STATUS_CODES.TOO_MANY_REQUESTS) { + const retryAfterHeader = response.headers.get('Retry-After'); + const retryAfterMs = parseRetryAfter(retryAfterHeader); + throw new RateLimitedError( + `${errorPrefix}: ${responseMessage}`, + retryAfterMs ?? undefined, + ); } - // Handle regular errors (network failures, JSON parsing, etc.) - const errorMessage = e instanceof Error ? e.message : String(e); - throw new ErrorClass(`${errorPrefix}: ${errorMessage}`); + throw new ErrorClass(`${errorPrefix}: ${responseMessage}`); } -export const NONCE_URL = (env: Env) => +export const NONCE_URL = (env: Env): string => `${getEnvUrls(env).authApiUrl}/api/v2/nonce`; -export const PAIR_IDENTIFIERS = (env: Env) => +export const PAIR_IDENTIFIERS = (env: Env): string => `${getEnvUrls(env).authApiUrl}/api/v2/identifiers/pair`; -export const OIDC_TOKEN_URL = (env: Env) => +export const OIDC_TOKEN_URL = (env: Env): string => `${getEnvUrls(env).oidcApiUrl}/oauth2/token`; -export const SRP_LOGIN_URL = (env: Env) => +export const SRP_LOGIN_URL = (env: Env): string => `${getEnvUrls(env).authApiUrl}/api/v2/srp/login`; -export const SIWE_LOGIN_URL = (env: Env) => +export const SIWE_LOGIN_URL = (env: Env): string => `${getEnvUrls(env).authApiUrl}/api/v2/siwe/login`; -export const PROFILE_LINEAGE_URL = (env: Env) => +export const PROFILE_LINEAGE_URL = (env: Env): string => `${getEnvUrls(env).authApiUrl}/api/v2/profile/lineage`; const getAuthenticationUrl = (authType: AuthType, env: Env): string => { @@ -155,8 +170,11 @@ type NonceResponse = { type PairRequest = { signature: string; + // eslint-disable-next-line @typescript-eslint/naming-convention raw_message: string; + // eslint-disable-next-line @typescript-eslint/naming-convention encrypted_storage_key: string; + // eslint-disable-next-line @typescript-eslint/naming-convention identifier_type: 'SIWE' | 'SRP'; }; @@ -191,10 +209,15 @@ export async function pairIdentifiers( }); if (!response.ok) { - throw response; + return throwServiceError( + response, + 'Unable to pair identifiers', + PairError, + ); } - } catch (e) { - return handleServiceError(e, 'Unable to pair identifiers', PairError); + return undefined; + } catch (error) { + return throwServiceError(error, 'Unable to pair identifiers', PairError); } } @@ -212,7 +235,11 @@ export async function getNonce(id: string, env: Env): Promise { try { const nonceResponse = await fetch(nonceUrl.toString()); if (!nonceResponse.ok) { - throw nonceResponse; + return throwServiceError( + nonceResponse, + 'Failed to get nonce', + NonceRetrievalError, + ); } const nonceJson = await nonceResponse.json(); @@ -221,8 +248,8 @@ export async function getNonce(id: string, env: Env): Promise { identifier: nonceJson.identifier, expiresIn: nonceJson.expires_in, }; - } catch (e) { - return handleServiceError(e, 'Failed to get nonce', NonceRetrievalError); + } catch (error) { + return throwServiceError(error, 'Failed to get nonce', NonceRetrievalError); } } @@ -240,9 +267,9 @@ export async function authorizeOIDC( platform: Platform, ): Promise { const grantType = 'urn:ietf:params:oauth:grant-type:jwt-bearer'; - const headers = new Headers({ + const headers = { 'Content-Type': 'application/x-www-form-urlencoded', - }); + }; const urlEncodedBody = new URLSearchParams(); urlEncodedBody.append('grant_type', grantType); @@ -257,7 +284,11 @@ export async function authorizeOIDC( }); if (!response.ok) { - throw response; + return throwServiceError( + response, + 'Unable to get access token', + SignInError, + ); } const accessTokenResponse = await response.json(); @@ -266,8 +297,8 @@ export async function authorizeOIDC( expiresIn: accessTokenResponse.expires_in, obtainedAt: Date.now(), }; - } catch (e) { - return handleServiceError(e, 'Unable to get access token', SignInError); + } catch (error) { + return throwServiceError(error, 'Unable to get access token', SignInError); } } @@ -316,7 +347,11 @@ export async function authenticate( }); if (!response.ok) { - throw response; + return throwServiceError( + response, + `${authType} login failed`, + SignInError, + ); } const loginResponse = await response.json(); @@ -329,8 +364,8 @@ export async function authenticate( profileId: loginResponse.profile.profile_id, }, }; - } catch (e) { - return handleServiceError(e, `${authType} login failed`, SignInError); + } catch (error) { + return throwServiceError(error, `${authType} login failed`, SignInError); } } @@ -356,12 +391,20 @@ export async function getUserProfileLineage( }); if (!response.ok) { - throw response; + return throwServiceError( + response, + 'Failed to get profile lineage', + SignInError, + ); } const profileJson: UserProfileLineage = await response.json(); return profileJson; - } catch (e) { - return handleServiceError(e, 'Failed to get profile lineage', SignInError); + } catch (error) { + return throwServiceError( + error, + 'Failed to get profile lineage', + SignInError, + ); } } From 503bfa20b67986266eb6b779865bdd0887f87360 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Mon, 26 Jan 2026 14:33:18 +0100 Subject: [PATCH 5/7] fix: cursor feedback --- .../src/sdk/authentication-jwt-bearer/services.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 ffd1d11412d..635d28c1a95 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 @@ -63,7 +63,7 @@ async function getResponseErrorMessage(response: Response): Promise { 'message' in responseBody ? responseBody.message : responseBody.error_description; - error = responseBody.error; + error = responseBody.error ?? 'unknown'; } catch { try { const textContent = await clonedResponse.text(); From 8697d46555aee24cee35353100adf5c8f2ac4e54 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Mon, 26 Jan 2026 14:49:08 +0100 Subject: [PATCH 6/7] fix: cursor feedback + coverage --- .../services.test.ts | 15 +++++++ .../sdk/authentication-jwt-bearer/services.ts | 41 ++++++++++++++----- 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/services.test.ts b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/services.test.ts index dd3f60b432e..2ff8306d733 100644 --- a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/services.test.ts +++ b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/services.test.ts @@ -288,6 +288,21 @@ describe('services', () => { ); }); + it('should handle missing error field in JSON response', async () => { + const mockResponse = createMockResponse( + { + message: 'Something went wrong', + // no error field + }, + { ok: false, status: 500 }, + ); + mockFetch.mockResolvedValue(mockResponse); + + await expect(getNonce('test-id', Env.DEV)).rejects.toThrow( + 'Failed to get nonce: HTTP 500 - Something went wrong (error: unknown)', + ); + }); + it('should truncate long text responses', async () => { const longText = 'A'.repeat(200); const mockResponse = createMockResponse(longText, { 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 635d28c1a95..bf1612cbf87 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 @@ -107,6 +107,11 @@ async function throwServiceError( errorPrefix: string, ErrorClass: new (message: string) => Error, ): Promise { + // Re-throw RateLimitedError as-is (don't wrap it) + if (error instanceof RateLimitedError) { + throw error; + } + // Not a Response-like object - handle as regular error if (!isErrorResponse(error)) { const errorMessage = error instanceof Error ? error.message : String(error); @@ -209,7 +214,7 @@ export async function pairIdentifiers( }); if (!response.ok) { - return throwServiceError( + return await throwServiceError( response, 'Unable to pair identifiers', PairError, @@ -217,7 +222,11 @@ export async function pairIdentifiers( } return undefined; } catch (error) { - return throwServiceError(error, 'Unable to pair identifiers', PairError); + return await throwServiceError( + error, + 'Unable to pair identifiers', + PairError, + ); } } @@ -235,7 +244,7 @@ export async function getNonce(id: string, env: Env): Promise { try { const nonceResponse = await fetch(nonceUrl.toString()); if (!nonceResponse.ok) { - return throwServiceError( + return await throwServiceError( nonceResponse, 'Failed to get nonce', NonceRetrievalError, @@ -249,7 +258,11 @@ export async function getNonce(id: string, env: Env): Promise { expiresIn: nonceJson.expires_in, }; } catch (error) { - return throwServiceError(error, 'Failed to get nonce', NonceRetrievalError); + return await throwServiceError( + error, + 'Failed to get nonce', + NonceRetrievalError, + ); } } @@ -284,7 +297,7 @@ export async function authorizeOIDC( }); if (!response.ok) { - return throwServiceError( + return await throwServiceError( response, 'Unable to get access token', SignInError, @@ -298,7 +311,11 @@ export async function authorizeOIDC( obtainedAt: Date.now(), }; } catch (error) { - return throwServiceError(error, 'Unable to get access token', SignInError); + return await throwServiceError( + error, + 'Unable to get access token', + SignInError, + ); } } @@ -347,7 +364,7 @@ export async function authenticate( }); if (!response.ok) { - return throwServiceError( + return await throwServiceError( response, `${authType} login failed`, SignInError, @@ -365,7 +382,11 @@ export async function authenticate( }, }; } catch (error) { - return throwServiceError(error, `${authType} login failed`, SignInError); + return await throwServiceError( + error, + `${authType} login failed`, + SignInError, + ); } } @@ -391,7 +412,7 @@ export async function getUserProfileLineage( }); if (!response.ok) { - return throwServiceError( + return await throwServiceError( response, 'Failed to get profile lineage', SignInError, @@ -401,7 +422,7 @@ export async function getUserProfileLineage( const profileJson: UserProfileLineage = await response.json(); return profileJson; } catch (error) { - return throwServiceError( + return await throwServiceError( error, 'Failed to get profile lineage', SignInError, From 2f175fcc6e134ca24dfb790d0d1d14c9c2ee999d Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Mon, 26 Jan 2026 15:04:30 +0100 Subject: [PATCH 7/7] fix: update suppressions --- eslint-suppressions.json | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/eslint-suppressions.json b/eslint-suppressions.json index d36f8b79fc3..cffa6ebba22 100644 --- a/eslint-suppressions.json +++ b/eslint-suppressions.json @@ -1699,20 +1699,6 @@ "count": 1 } }, - "packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/services.ts": { - "@typescript-eslint/explicit-function-return-type": { - "count": 6 - }, - "@typescript-eslint/naming-convention": { - "count": 4 - }, - "id-length": { - "count": 5 - }, - "no-restricted-globals": { - "count": 1 - } - }, "packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/types.ts": { "@typescript-eslint/naming-convention": { "count": 5