From 283ec90a258704cbcd5b459be838c29e8f9bcfe6 Mon Sep 17 00:00:00 2001 From: tt-a1i <53142663+tt-a1i@users.noreply.github.com> Date: Sun, 21 Jun 2026 02:09:20 +0800 Subject: [PATCH] fix(desktop): validate generic oauth token responses --- .../src/auth/__tests__/generic-oauth.test.ts | 160 ++++++++++++++++++ .../packages/shared/src/auth/generic-oauth.ts | 65 +++++-- 2 files changed, 212 insertions(+), 13 deletions(-) create mode 100644 packages/desktop/packages/shared/src/auth/__tests__/generic-oauth.test.ts diff --git a/packages/desktop/packages/shared/src/auth/__tests__/generic-oauth.test.ts b/packages/desktop/packages/shared/src/auth/__tests__/generic-oauth.test.ts new file mode 100644 index 00000000000..9be0ed3cbac --- /dev/null +++ b/packages/desktop/packages/shared/src/auth/__tests__/generic-oauth.test.ts @@ -0,0 +1,160 @@ +import { afterEach, beforeEach, describe, expect, it } from 'bun:test'; +import { exchangeGenericOAuth, refreshGenericOAuthToken } from '../generic-oauth.ts'; +import type { OAuthExchangeParams } from '../oauth-flow-types.ts'; + +const exchangeParams: OAuthExchangeParams = { + code: 'code-123', + codeVerifier: 'verifier-123', + tokenEndpoint: 'https://auth.example.com/oauth/token', + clientId: 'client-123', + clientSecret: 'secret-123', + redirectUri: 'https://app.example.com/callback', +}; + +function mockTokenResponse(body: unknown, contentType = 'application/json') { + globalThis.fetch = (async () => + new Response( + typeof body === 'string' ? body : JSON.stringify(body), + { status: 200, headers: { 'Content-Type': contentType } }, + )) as unknown as typeof globalThis.fetch; +} + +describe('generic OAuth token responses', () => { + let originalFetch: typeof globalThis.fetch; + + beforeEach(() => { + originalFetch = globalThis.fetch; + }); + + afterEach(() => { + globalThis.fetch = originalFetch; + }); + + it('exchanges JSON token responses with numeric expires_in', async () => { + mockTokenResponse({ + access_token: 'access-token', + refresh_token: 'refresh-token', + expires_in: 3600, + }); + + const result = await exchangeGenericOAuth(exchangeParams); + + expect(result.success).toBe(true); + expect(result.accessToken).toBe('access-token'); + expect(result.refreshToken).toBe('refresh-token'); + expect(result.expiresAt).toBeGreaterThan(Date.now()); + }); + + it('exchanges form-encoded token responses with string expires_in', async () => { + mockTokenResponse( + 'access_token=access-token&refresh_token=refresh-token&expires_in=3600', + 'application/x-www-form-urlencoded', + ); + + const result = await exchangeGenericOAuth(exchangeParams); + + expect(result.success).toBe(true); + expect(result.accessToken).toBe('access-token'); + expect(result.refreshToken).toBe('refresh-token'); + expect(result.expiresAt).toBeGreaterThan(Date.now()); + }); + + it('rejects exchange responses without access_token', async () => { + mockTokenResponse({ refresh_token: 'refresh-token', expires_in: 3600 }); + + const result = await exchangeGenericOAuth(exchangeParams); + + expect(result.success).toBe(false); + expect(result.error).toContain('missing access_token'); + }); + + it('rejects exchange responses with malformed expires_in', async () => { + mockTokenResponse({ access_token: 'access-token', expires_in: '3600abc' }); + + const result = await exchangeGenericOAuth(exchangeParams); + + expect(result.success).toBe(false); + expect(result.error).toContain('invalid expires_in'); + }); + + it('rejects unsafe JSON number expires_in values', async () => { + mockTokenResponse({ access_token: 'access-token', expires_in: Number.MAX_SAFE_INTEGER + 1 }); + + const result = await exchangeGenericOAuth(exchangeParams); + + expect(result.success).toBe(false); + expect(result.error).toContain('invalid expires_in'); + }); + + it('preserves zero-second expiries instead of treating them as missing', async () => { + mockTokenResponse({ access_token: 'access-token', expires_in: 0 }); + const before = Date.now(); + + const result = await exchangeGenericOAuth(exchangeParams); + + expect(result.success).toBe(true); + expect(result.expiresAt).toBeGreaterThanOrEqual(before); + expect(result.expiresAt).toBeLessThanOrEqual(Date.now()); + }); + + it('rejects refresh responses without access_token', async () => { + mockTokenResponse({ refresh_token: 'refresh-token', expires_in: 3600 }); + + await expect( + refreshGenericOAuthToken( + 'refresh-token', + 'https://auth.example.com/oauth/token', + 'client-123', + 'secret-123', + ), + ).rejects.toThrow('missing access_token'); + }); + + it('refreshes JSON token responses with refresh_token and numeric expires_in', async () => { + mockTokenResponse({ + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + expires_in: 3600, + }); + + const result = await refreshGenericOAuthToken( + 'old-refresh-token', + 'https://auth.example.com/oauth/token', + 'client-123', + 'secret-123', + ); + + expect(result.accessToken).toBe('new-access-token'); + expect(result.refreshToken).toBe('new-refresh-token'); + expect(result.expiresAt).toBeGreaterThan(Date.now()); + }); + + it('preserves zero-second expiries on refresh responses', async () => { + mockTokenResponse({ access_token: 'new-access-token', expires_in: 0 }); + const before = Date.now(); + + const result = await refreshGenericOAuthToken( + 'old-refresh-token', + 'https://auth.example.com/oauth/token', + 'client-123', + 'secret-123', + ); + + expect(result.accessToken).toBe('new-access-token'); + expect(result.expiresAt).toBeGreaterThanOrEqual(before); + expect(result.expiresAt).toBeLessThanOrEqual(Date.now()); + }); + + it('rejects refresh responses with malformed expires_in', async () => { + mockTokenResponse({ access_token: 'access-token', expires_in: '1e3' }); + + await expect( + refreshGenericOAuthToken( + 'refresh-token', + 'https://auth.example.com/oauth/token', + 'client-123', + 'secret-123', + ), + ).rejects.toThrow('invalid expires_in'); + }); +}); diff --git a/packages/desktop/packages/shared/src/auth/generic-oauth.ts b/packages/desktop/packages/shared/src/auth/generic-oauth.ts index 484bca32b9f..18a198b9022 100644 --- a/packages/desktop/packages/shared/src/auth/generic-oauth.ts +++ b/packages/desktop/packages/shared/src/auth/generic-oauth.ts @@ -17,7 +17,7 @@ import { generatePKCE, generateState } from './pkce.ts'; * GitHub (and some other providers) return form-encoded unless you send Accept: application/json. * We send Accept: application/json but tolerate form-encoded as a fallback. */ -function parseTokenResponse(body: string, contentType: string | null): Record { +function parseTokenResponse(body: string, contentType: string | null): Record { if (contentType?.includes('application/json')) { return JSON.parse(body); } @@ -31,6 +31,43 @@ function parseTokenResponse(body: string, contentType: string | null): Record, field: string): string | undefined { + const value = data[field]; + return typeof value === 'string' && value.length > 0 + ? value + : undefined; +} + +function readAccessToken(data: Record): string | null { + return readStringField(data, 'access_token') ?? null; +} + +function parseExpiresIn(data: Record): number | undefined { + const raw = data.expires_in; + if (raw == null || raw === '') return undefined; + + if (typeof raw === 'number') { + if (Number.isSafeInteger(raw) && raw >= 0) return raw; + throw new Error('Token response has invalid expires_in'); + } + + const trimmed = String(raw).trim(); + if (!/^\d+$/.test(trimmed)) { + throw new Error('Token response has invalid expires_in'); + } + + const value = Number(trimmed); + if (!Number.isSafeInteger(value)) { + throw new Error('Token response has invalid expires_in'); + } + + return value; +} + +function expiresAtFromSeconds(expiresIn: number | undefined): number | undefined { + return expiresIn == null ? undefined : Date.now() + expiresIn * 1000; +} + // ============================================================ // Prepare // ============================================================ @@ -123,16 +160,19 @@ export async function exchangeGenericOAuth(params: OAuthExchangeParams): Promise const data = parseTokenResponse(responseBody, response.headers.get('content-type')); if (data.error) { - return { success: false, error: `OAuth error: ${data.error} — ${data.error_description ?? ''}` }; + return { success: false, error: `OAuth error: ${String(data.error)} — ${String(data.error_description ?? '')}` }; } - const expiresIn = data.expires_in ? parseInt(data.expires_in, 10) : undefined; + const accessToken = readAccessToken(data); + if (!accessToken) { + return { success: false, error: 'Token exchange response missing access_token' }; + } return { success: true, - accessToken: data.access_token, - refreshToken: data.refresh_token, - expiresAt: expiresIn ? Date.now() + expiresIn * 1000 : undefined, + accessToken, + refreshToken: readStringField(data, 'refresh_token'), + expiresAt: expiresAtFromSeconds(parseExpiresIn(data)), oauthClientId: params.clientId, oauthClientSecret: params.clientSecret, }; @@ -186,18 +226,17 @@ export async function refreshGenericOAuthToken( const data = parseTokenResponse(responseBody, response.headers.get('content-type')); if (data.error) { - throw new Error(`OAuth refresh error: ${data.error} — ${data.error_description ?? ''}`); + throw new Error(`OAuth refresh error: ${String(data.error)} — ${String(data.error_description ?? '')}`); } - if (!data.access_token) { + const accessToken = readAccessToken(data); + if (!accessToken) { throw new Error('Token refresh response missing access_token'); } - const expiresIn = data.expires_in ? parseInt(data.expires_in, 10) : undefined; - return { - accessToken: data.access_token, - refreshToken: data.refresh_token, - expiresAt: expiresIn ? Date.now() + expiresIn * 1000 : undefined, + accessToken, + refreshToken: readStringField(data, 'refresh_token'), + expiresAt: expiresAtFromSeconds(parseExpiresIn(data)), }; }