diff --git a/src/renderer/utils/forges/github/flows.test.ts b/src/renderer/utils/forges/github/flows.test.ts index 8b38641be..8ba33a90d 100644 --- a/src/renderer/utils/forges/github/flows.test.ts +++ b/src/renderer/utils/forges/github/flows.test.ts @@ -150,11 +150,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 +168,14 @@ describe('renderer/utils/forges/github/flows.ts', () => { ), ); + // The callback must echo back the same `state` this attempt generated; + // read it from the authorization URL just asserted above. + const authorizeUrl = new URL(openExternalLinkSpy.mock.calls[0][0] as string); + const state = authorizeUrl.searchParams.get('state'); + 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,6 +183,40 @@ 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; + }); + + 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 nothing + // resolved the promise from either mismatched callback. + await Promise.resolve(); + await Promise.resolve(); + expect(resolved).toBe(false); + + // Deliver the real callback so the promise settles and the test + // doesn't leave a dangling unhandled rejection. + const authorizeUrl = new URL( + openExternalLinkSpy.mock.calls[openExternalLinkSpy.mock.calls.length - 1][0] as string, + ); + const state = authorizeUrl.searchParams.get('state'); + deliverCallback(`gitify://oauth?code=the-real-code&state=${state}`); + const res = await resultPromise; + expect(res.authCode).toBe('the-real-code'); + }); + it('should call performGitHubWebOAuth - failure', async () => { window.gitify.onAuthCallback = vi.fn().mockImplementation((callback) => { callback( diff --git a/src/renderer/utils/forges/github/flows.ts b/src/renderer/utils/forges/github/flows.ts index b054a8611..6b5db01a3 100644 --- a/src/renderer/utils/forges/github/flows.ts +++ b/src/renderer/utils/forges/github/flows.ts @@ -23,6 +23,17 @@ type DeviceFlowErrorResponse = { error_uri: string; }; +/** + * Generates a cryptographically random OAuth `state` value for one web-flow + * login attempt, using `crypto.getRandomValues` per this codebase's + * secure-coding convention (see `utils/core/random.ts`). + */ +function generateOAuthState(): string { + const buf = new Uint8Array(16); + crypto.getRandomValues(buf); + return Array.from(buf, (b) => b.toString(16).padStart(2, '0')).join(''); +} + /** * Initiate a GitHub OAuth Web Flow (OAuth App) authentication. * @@ -34,11 +45,19 @@ type DeviceFlowErrorResponse = { */ export function performGitHubWebOAuth(authOptions: LoginOAuthWebOptions): Promise { return new Promise((resolve, reject) => { + // 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. + const expectedState = generateOAuthState(); + const { url } = getWebFlowAuthorizationUrl({ clientType: 'oauth-app', clientId: authOptions.clientId, scopes: getRecommendedScopeNames(), allowSignup: false, + state: expectedState, request: request.defaults({ baseUrl: `https://${authOptions.hostname}`, }), @@ -63,17 +82,28 @@ export function performGitHubWebOAuth(authOptions: LoginOAuthWebOptions): Promis 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'); - if (code && type === 'oauth') { + if (code && type === 'oauth' && state === expectedState) { unsubscribeAuthCallback?.(); resolve({ authMethod: 'OAuth App', authCode: code as AuthCode, authOptions: authOptions, }); + } else if (code && type === 'oauth') { + // A code arrived but its state doesn't match this login attempt's + // own value (or carries none) - do not treat it as this login + // completing, since accepting it would let a code obtained outside + // this flow (e.g. for a different GitHub account) be exchanged for + // a token as if it belonged to this session. + rendererLogError( + 'renderer:auth-callback', + 'Rejected an authentication callback with a missing or mismatched state parameter', + ); } else if (error) { unsubscribeAuthCallback?.(); reject(