fix: validate OAuth state on the web-flow callback

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();
+    });
+  });
+});

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();
+}

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,21 +169,106 @@ 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));
 
       expect(res.authMethod).toBe('OAuth App');
       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;
 

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<AuthResponse> {
   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({