diff --git a/src/renderer/utils/auth/state.test.ts b/src/renderer/utils/auth/state.test.ts new file mode 100644 index 000000000..191a792b1 --- /dev/null +++ b/src/renderer/utils/auth/state.test.ts @@ -0,0 +1,22 @@ +import { generateOAuthState } from './state'; + +const UUID_V4 = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; + +describe('renderer/utils/auth/state.ts', () => { + describe('generateOAuthState', () => { + it('should return a v4 UUID', () => { + expect(generateOAuthState()).toMatch(UUID_V4); + }); + + it('should return different values across calls', () => { + expect(generateOAuthState()).not.toBe(generateOAuthState()); + }); + + it('should use crypto.randomUUID', () => { + const spy = vi.spyOn(globalThis.crypto, 'randomUUID'); + generateOAuthState(); + expect(spy).toHaveBeenCalledOnce(); + spy.mockRestore(); + }); + }); +}); diff --git a/src/renderer/utils/auth/state.ts b/src/renderer/utils/auth/state.ts new file mode 100644 index 000000000..3b88de7c3 --- /dev/null +++ b/src/renderer/utils/auth/state.ts @@ -0,0 +1,13 @@ +/** + * Generate a cryptographically random OAuth 2.0 `state` value. + * + * `crypto.randomUUID()` is backed by the platform CSPRNG and yields 122 random + * bits — ample for a per-attempt, single-use value that an attacker cannot + * guess in order to bind the client to a different account. Using the platform + * primitive avoids hand-rolled random-value encoding entirely. + * + * @returns A random UUID to send as the OAuth `state` parameter. + */ +export function generateOAuthState(): string { + return crypto.randomUUID(); +} diff --git a/src/renderer/utils/forges/github/flows.test.ts b/src/renderer/utils/forges/github/flows.test.ts index 8b38641be..e9adc14fb 100644 --- a/src/renderer/utils/forges/github/flows.test.ts +++ b/src/renderer/utils/forges/github/flows.test.ts @@ -38,6 +38,7 @@ const exchangeWebFlowCodeMock = exchangeWebFlowCode as unknown as MockedFunction describe('renderer/utils/forges/github/flows.ts', () => { vi.spyOn(logger, 'rendererLogInfo').mockImplementation(vi.fn()); const rendererLogErrorSpy = vi.spyOn(logger, 'rendererLogError').mockImplementation(vi.fn()); + const rendererLogWarnSpy = vi.spyOn(logger, 'rendererLogWarn').mockImplementation(vi.fn()); const openExternalLinkSpy = vi.spyOn(comms, 'openExternalLink').mockImplementation(vi.fn()); beforeEach(() => { @@ -150,11 +151,12 @@ describe('renderer/utils/forges/github/flows.ts', () => { }; it('should call performGitHubWebOAuth using custom oauth app - success oauth flow', async () => { + let deliverCallback: (callbackUrl: string) => void = () => {}; window.gitify.onAuthCallback = vi.fn().mockImplementation((callback) => { - callback('gitify://oauth?code=123-456'); + deliverCallback = callback; }); - const res = await flows.performGitHubWebOAuth({ + const resultPromise = flows.performGitHubWebOAuth({ clientId: 'BYO_CLIENT_ID' as ClientID, clientSecret: 'BYO_CLIENT_SECRET' as ClientSecret, hostname: 'my.git.com' as Hostname, @@ -167,6 +169,16 @@ describe('renderer/utils/forges/github/flows.ts', () => { ), ); + // The authorization URL must carry a per-attempt state, and the login + // only completes when the callback echoes that same value back. + const authorizeUrl = new URL(openExternalLinkSpy.mock.calls[0][0] as string); + const state = authorizeUrl.searchParams.get('state'); + expect(state).toBeTruthy(); + + deliverCallback(`gitify://oauth?code=123-456&state=${state}`); + + const res = await resultPromise; + expect(window.gitify.onAuthCallback).toHaveBeenCalledTimes(1); expect(window.gitify.onAuthCallback).toHaveBeenCalledWith(expect.any(Function)); @@ -174,14 +186,89 @@ describe('renderer/utils/forges/github/flows.ts', () => { expect(res.authCode).toBe('123-456'); }); + it('does not resolve when the callback state does not match the generated state', async () => { + let deliverCallback: (callbackUrl: string) => void = () => {}; + window.gitify.onAuthCallback = vi.fn().mockImplementation((callback) => { + deliverCallback = callback; + }); + + let resolved = false; + const resultPromise = flows.performGitHubWebOAuth(webAuthOptions).then((res) => { + resolved = true; + return res; + }); + + const authorizeUrl = new URL(openExternalLinkSpy.mock.calls[0][0] as string); + const state = authorizeUrl.searchParams.get('state'); + + deliverCallback('gitify://oauth?code=attacker-code&state=not-the-real-state'); + deliverCallback('gitify://oauth?code=attacker-code-no-state'); + + // Give any pending microtasks a chance to run, then confirm neither + // injected callback settled the login. + await Promise.resolve(); + await Promise.resolve(); + expect(resolved).toBe(false); + expect(rendererLogWarnSpy).toHaveBeenCalledTimes(2); + + // The still-pending attempt completes once the matching state arrives. + deliverCallback(`gitify://oauth?code=the-real-code&state=${state}`); + const res = await resultPromise; + expect(res.authCode).toBe('the-real-code'); + }); + + it('ignores an unparseable callback URL instead of tearing down the pending login', async () => { + let deliverCallback: (callbackUrl: string) => void = () => {}; + window.gitify.onAuthCallback = vi.fn().mockImplementation((callback) => { + deliverCallback = callback; + }); + + let settled = false; + const resultPromise = flows.performGitHubWebOAuth(webAuthOptions).then( + (res) => { + settled = true; + return res; + }, + (err: unknown) => { + settled = true; + throw err; + }, + ); + + // `gitify://[` passes the protocol handler's `gitify://` prefix check but + // is not a parseable URL, so it must be ignored rather than rejecting the + // login before the state is ever compared. + deliverCallback('gitify://['); + + await Promise.resolve(); + await Promise.resolve(); + expect(settled).toBe(false); + expect(rendererLogWarnSpy).toHaveBeenCalledTimes(1); + + // Settle the still-pending attempt so no listener is left dangling. + const authorizeUrl = new URL(openExternalLinkSpy.mock.calls[0][0] as string); + const state = authorizeUrl.searchParams.get('state'); + deliverCallback(`gitify://oauth?code=real-code&state=${state}`); + + await expect(resultPromise).resolves.toMatchObject({ authCode: 'real-code' }); + }); + it('should call performGitHubWebOAuth - failure', async () => { + let deliverCallback: (callbackUrl: string) => void = () => {}; window.gitify.onAuthCallback = vi.fn().mockImplementation((callback) => { - callback( - 'gitify://auth?error=invalid_request&error_description=The+redirect_uri+is+missing+or+invalid.&error_uri=https://docs.github.com/en/developers/apps/troubleshooting-oauth-errors', - ); + deliverCallback = callback; }); - await expect(async () => await flows.performGitHubWebOAuth(webAuthOptions)).rejects.toEqual( + const resultPromise = flows.performGitHubWebOAuth(webAuthOptions); + + const authorizeUrl = new URL(openExternalLinkSpy.mock.calls[0][0] as string); + const state = authorizeUrl.searchParams.get('state'); + + deliverCallback( + `gitify://oauth?state=${state}&error=invalid_request&error_description=The+redirect_uri+is+missing+or+invalid.&error_uri=https://docs.github.com/en/developers/apps/troubleshooting-oauth-errors`, + ); + + await expect(resultPromise).rejects.toEqual( new Error( "Oops! Something went wrong and we couldn't log you in using GitHub. Please try again. Reason: The redirect_uri is missing or invalid. Docs: https://docs.github.com/en/developers/apps/troubleshooting-oauth-errors", ), @@ -198,6 +285,40 @@ describe('renderer/utils/forges/github/flows.ts', () => { expect(window.gitify.onAuthCallback).toHaveBeenCalledWith(expect.any(Function)); }); + it('ignores a callback error whose state does not match the attempt', async () => { + let deliverCallback: (callbackUrl: string) => void = () => {}; + window.gitify.onAuthCallback = vi.fn().mockImplementation((callback) => { + deliverCallback = callback; + }); + + let settled = false; + const resultPromise = flows.performGitHubWebOAuth(webAuthOptions).then( + (res) => { + settled = true; + return res; + }, + (err: unknown) => { + settled = true; + throw err; + }, + ); + + deliverCallback('gitify://oauth?error=access_denied'); + deliverCallback('gitify://oauth?error=access_denied&state=not-the-real-state'); + + await Promise.resolve(); + await Promise.resolve(); + expect(settled).toBe(false); + expect(rendererLogWarnSpy).toHaveBeenCalledTimes(2); + + // Settle the still-pending attempt so no listener is left dangling. + const authorizeUrl = new URL(openExternalLinkSpy.mock.calls[0][0] as string); + const state = authorizeUrl.searchParams.get('state'); + deliverCallback(`gitify://oauth?code=real-code&state=${state}`); + + await expect(resultPromise).resolves.toMatchObject({ authCode: 'real-code' }); + }); + describe('exchangeAuthCodeForAccessToken', () => { const authCode = '123-456' as AuthCode; diff --git a/src/renderer/utils/forges/github/flows.ts b/src/renderer/utils/forges/github/flows.ts index b054a8611..6b7fe1d22 100644 --- a/src/renderer/utils/forges/github/flows.ts +++ b/src/renderer/utils/forges/github/flows.ts @@ -13,7 +13,8 @@ import type { AuthCode, Hostname, Link, Token } from '../../../types'; import type { AuthResponse, DeviceFlowSession, LoginOAuthWebOptions } from '../../auth/types'; import { getRecommendedScopeNames } from '../../auth/scopes'; -import { rendererLogError, rendererLogInfo, toError } from '../../core/logger'; +import { generateOAuthState } from '../../auth/state'; +import { rendererLogError, rendererLogInfo, rendererLogWarn, toError } from '../../core/logger'; import { openExternalLink } from '../../system/comms'; import { getGitHubAuthBaseUrl } from './auth'; @@ -29,16 +30,32 @@ type DeviceFlowErrorResponse = { * Opens the GitHub authorization URL in the user's browser, then waits for the * app's custom `gitify://oauth` callback to receive the authorization code. * + * The login is bound to a per-attempt random `state` that is sent with the + * authorization request and must be echoed by the callback. A callback + * carrying a missing or mismatched `state` is logged and ignored, so a + * `gitify://oauth` link delivered from outside this attempt cannot bind the + * client to a different GitHub account or tear down the pending login. + * * @param authOptions - The OAuth App client configuration and target hostname. * @returns Resolves with the authorization code and options on success. */ export function performGitHubWebOAuth(authOptions: LoginOAuthWebOptions): Promise { return new Promise((resolve, reject) => { - const { url } = getWebFlowAuthorizationUrl({ + // A per-attempt, unguessable value this app itself tracks and checks on + // callback, so a `gitify://oauth` callback carrying an authorization code + // this specific login attempt never requested (e.g. one an attacker + // obtained for their own GitHub account) is rejected instead of silently + // accepted and exchanged for a token. + // + // Read the state back from the library's result rather than trusting the + // value echoed to us, so the string compared on callback is exactly the one + // the library embedded in the authorization URL. + const { url, state: expectedState } = getWebFlowAuthorizationUrl({ clientType: 'oauth-app', clientId: authOptions.clientId, scopes: getRecommendedScopeNames(), allowSignup: false, + state: generateOAuthState(), request: request.defaults({ baseUrl: `https://${authOptions.hostname}`, }), @@ -56,17 +73,37 @@ export function performGitHubWebOAuth(authOptions: LoginOAuthWebOptions): Promis try { url = new URL(callbackUrl); } catch { - unsubscribeAuthCallback?.(); - reject(new Error(`Received an invalid authentication callback URL: ${callbackUrl}`)); + // An unparseable callback cannot carry the state this attempt + // generated, so treat it like any other unmatched callback: log and + // ignore rather than tearing down the pending login. Otherwise a + // forged `gitify://` URL (e.g. `gitify://[`) would abort a legitimate + // login even though the state check never runs. + rendererLogWarn( + 'renderer:auth-callback', + 'Ignored an unparseable authentication callback URL', + ); return; } const type = url.hostname; const code = url.searchParams.get('code'); + const state = url.searchParams.get('state'); const error = url.searchParams.get('error'); const errorDescription = url.searchParams.get('error_description'); const errorUri = url.searchParams.get('error_uri'); + // A callback belongs to this login attempt only when it echoes the state + // this attempt generated. Anything else (missing or mismatched state) is + // logged and ignored, leaving the pending login intact so an injected + // callback can neither bind another account nor tear down the attempt. + if (state !== expectedState) { + rendererLogWarn( + 'renderer:auth-callback', + 'Ignored an authentication callback with a missing or mismatched state parameter', + ); + return; + } + if (code && type === 'oauth') { unsubscribeAuthCallback?.(); resolve({