fix(auth): improve Google OAuth error handling and prevent empty error messages (#7539)

2026-03-10 22:21:22 -07:00 · 2025-09-03 13:51:29 -07:00
parent af99989c91
commit 876d091602
2 changed files with 465 additions and 33 deletions
--- a/packages/core/src/code_assist/oauth2.test.ts
+++ b/packages/core/src/code_assist/oauth2.test.ts
@@ -520,6 +520,363 @@ describe('oauth2', () => {
    });
  });

+  describe('error handling', () => {
+    it('should handle browser launch failure with FatalAuthenticationError', async () => {
+      const mockError = new Error('Browser launch failed');
+      (open as Mock).mockRejectedValue(mockError);
+
+      const mockOAuth2Client = {
+        generateAuthUrl: vi.fn().mockReturnValue('https://example.com/auth'),
+        on: vi.fn(),
+      } as unknown as OAuth2Client;
+      (OAuth2Client as unknown as Mock).mockImplementation(
+        () => mockOAuth2Client,
+      );
+
+      await expect(
+        getOauthClient(AuthType.LOGIN_WITH_GOOGLE, mockConfig),
+      ).rejects.toThrow('Failed to open browser: Browser launch failed');
+    });
+
+    it('should handle authentication timeout with proper error message', async () => {
+      const mockAuthUrl = 'https://example.com/auth';
+      const mockOAuth2Client = {
+        generateAuthUrl: vi.fn().mockReturnValue(mockAuthUrl),
+        on: vi.fn(),
+      } as unknown as OAuth2Client;
+      (OAuth2Client as unknown as Mock).mockImplementation(
+        () => mockOAuth2Client,
+      );
+
+      (open as Mock).mockImplementation(async () => ({ on: vi.fn() }) as never);
+
+      const mockHttpServer = {
+        listen: vi.fn(),
+        close: vi.fn(),
+        on: vi.fn(),
+        address: () => ({ port: 3000 }),
+      };
+      (http.createServer as Mock).mockImplementation(
+        () => mockHttpServer as unknown as http.Server,
+      );
+
+      // Mock setTimeout to trigger timeout immediately
+      const originalSetTimeout = global.setTimeout;
+      global.setTimeout = vi.fn(
+        (callback) => (callback(), {} as unknown as NodeJS.Timeout),
+      ) as unknown as typeof setTimeout;
+
+      await expect(
+        getOauthClient(AuthType.LOGIN_WITH_GOOGLE, mockConfig),
+      ).rejects.toThrow(
+        'Authentication timed out after 5 minutes. The browser tab may have gotten stuck in a loading state. Please try again or use NO_BROWSER=true for manual authentication.',
+      );
+
+      global.setTimeout = originalSetTimeout;
+    });
+
+    it('should handle OAuth callback errors with descriptive messages', async () => {
+      const mockAuthUrl = 'https://example.com/auth';
+      const mockOAuth2Client = {
+        generateAuthUrl: vi.fn().mockReturnValue(mockAuthUrl),
+        on: vi.fn(),
+      } as unknown as OAuth2Client;
+      (OAuth2Client as unknown as Mock).mockImplementation(
+        () => mockOAuth2Client,
+      );
+
+      (open as Mock).mockImplementation(async () => ({ on: vi.fn() }) as never);
+
+      let requestCallback!: http.RequestListener;
+      let serverListeningCallback: (value: unknown) => void;
+      const serverListeningPromise = new Promise(
+        (resolve) => (serverListeningCallback = resolve),
+      );
+
+      const mockHttpServer = {
+        listen: vi.fn((_port: number, _host: string, callback?: () => void) => {
+          if (callback) callback();
+          serverListeningCallback(undefined);
+        }),
+        close: vi.fn(),
+        on: vi.fn(),
+        address: () => ({ port: 3000 }),
+      };
+      (http.createServer as Mock).mockImplementation((cb) => {
+        requestCallback = cb;
+        return mockHttpServer as unknown as http.Server;
+      });
+
+      const clientPromise = getOauthClient(
+        AuthType.LOGIN_WITH_GOOGLE,
+        mockConfig,
+      );
+      await serverListeningPromise;
+
+      // Test OAuth error with description
+      const mockReq = {
+        url: '/oauth2callback?error=access_denied&error_description=User+denied+access',
+      } as http.IncomingMessage;
+      const mockRes = {
+        writeHead: vi.fn(),
+        end: vi.fn(),
+      } as unknown as http.ServerResponse;
+
+      await expect(async () => {
+        await requestCallback(mockReq, mockRes);
+        await clientPromise;
+      }).rejects.toThrow(
+        'Google OAuth error: access_denied. User denied access',
+      );
+    });
+
+    it('should handle OAuth error without description', async () => {
+      const mockAuthUrl = 'https://example.com/auth';
+      const mockOAuth2Client = {
+        generateAuthUrl: vi.fn().mockReturnValue(mockAuthUrl),
+        on: vi.fn(),
+      } as unknown as OAuth2Client;
+      (OAuth2Client as unknown as Mock).mockImplementation(
+        () => mockOAuth2Client,
+      );
+
+      (open as Mock).mockImplementation(async () => ({ on: vi.fn() }) as never);
+
+      let requestCallback!: http.RequestListener;
+      let serverListeningCallback: (value: unknown) => void;
+      const serverListeningPromise = new Promise(
+        (resolve) => (serverListeningCallback = resolve),
+      );
+
+      const mockHttpServer = {
+        listen: vi.fn((_port: number, _host: string, callback?: () => void) => {
+          if (callback) callback();
+          serverListeningCallback(undefined);
+        }),
+        close: vi.fn(),
+        on: vi.fn(),
+        address: () => ({ port: 3000 }),
+      };
+      (http.createServer as Mock).mockImplementation((cb) => {
+        requestCallback = cb;
+        return mockHttpServer as unknown as http.Server;
+      });
+
+      const clientPromise = getOauthClient(
+        AuthType.LOGIN_WITH_GOOGLE,
+        mockConfig,
+      );
+      await serverListeningPromise;
+
+      // Test OAuth error without description
+      const mockReq = {
+        url: '/oauth2callback?error=server_error',
+      } as http.IncomingMessage;
+      const mockRes = {
+        writeHead: vi.fn(),
+        end: vi.fn(),
+      } as unknown as http.ServerResponse;
+
+      await expect(async () => {
+        await requestCallback(mockReq, mockRes);
+        await clientPromise;
+      }).rejects.toThrow(
+        'Google OAuth error: server_error. No additional details provided',
+      );
+    });
+
+    it('should handle token exchange failure with descriptive error', async () => {
+      const mockAuthUrl = 'https://example.com/auth';
+      const mockCode = 'test-code';
+      const mockState = 'test-state';
+
+      const mockOAuth2Client = {
+        generateAuthUrl: vi.fn().mockReturnValue(mockAuthUrl),
+        getToken: vi.fn().mockRejectedValue(new Error('Token exchange failed')),
+        on: vi.fn(),
+      } as unknown as OAuth2Client;
+      (OAuth2Client as unknown as Mock).mockImplementation(
+        () => mockOAuth2Client,
+      );
+
+      vi.spyOn(crypto, 'randomBytes').mockReturnValue(mockState as never);
+      (open as Mock).mockImplementation(async () => ({ on: vi.fn() }) as never);
+
+      let requestCallback!: http.RequestListener;
+      let serverListeningCallback: (value: unknown) => void;
+      const serverListeningPromise = new Promise(
+        (resolve) => (serverListeningCallback = resolve),
+      );
+
+      const mockHttpServer = {
+        listen: vi.fn((_port: number, _host: string, callback?: () => void) => {
+          if (callback) callback();
+          serverListeningCallback(undefined);
+        }),
+        close: vi.fn(),
+        on: vi.fn(),
+        address: () => ({ port: 3000 }),
+      };
+      (http.createServer as Mock).mockImplementation((cb) => {
+        requestCallback = cb;
+        return mockHttpServer as unknown as http.Server;
+      });
+
+      const clientPromise = getOauthClient(
+        AuthType.LOGIN_WITH_GOOGLE,
+        mockConfig,
+      );
+      await serverListeningPromise;
+
+      const mockReq = {
+        url: `/oauth2callback?code=${mockCode}&state=${mockState}`,
+      } as http.IncomingMessage;
+      const mockRes = {
+        writeHead: vi.fn(),
+        end: vi.fn(),
+      } as unknown as http.ServerResponse;
+
+      await expect(async () => {
+        await requestCallback(mockReq, mockRes);
+        await clientPromise;
+      }).rejects.toThrow(
+        'Failed to exchange authorization code for tokens: Token exchange failed',
+      );
+    });
+
+    it('should handle fetchAndCacheUserInfo failure gracefully', async () => {
+      const mockAuthUrl = 'https://example.com/auth';
+      const mockCode = 'test-code';
+      const mockState = 'test-state';
+      const mockTokens = {
+        access_token: 'test-access-token',
+        refresh_token: 'test-refresh-token',
+      };
+
+      const mockOAuth2Client = {
+        generateAuthUrl: vi.fn().mockReturnValue(mockAuthUrl),
+        getToken: vi.fn().mockResolvedValue({ tokens: mockTokens }),
+        setCredentials: vi.fn(),
+        getAccessToken: vi
+          .fn()
+          .mockResolvedValue({ token: 'test-access-token' }),
+        on: vi.fn(),
+      } as unknown as OAuth2Client;
+      (OAuth2Client as unknown as Mock).mockImplementation(
+        () => mockOAuth2Client,
+      );
+
+      vi.spyOn(crypto, 'randomBytes').mockReturnValue(mockState as never);
+      (open as Mock).mockImplementation(async () => ({ on: vi.fn() }) as never);
+
+      // Mock fetch to fail
+      (global.fetch as Mock).mockResolvedValue({
+        ok: false,
+        status: 500,
+        statusText: 'Internal Server Error',
+      } as unknown as Response);
+
+      const consoleErrorSpy = vi
+        .spyOn(console, 'error')
+        .mockImplementation(() => {});
+
+      let requestCallback!: http.RequestListener;
+      let serverListeningCallback: (value: unknown) => void;
+      const serverListeningPromise = new Promise(
+        (resolve) => (serverListeningCallback = resolve),
+      );
+
+      const mockHttpServer = {
+        listen: vi.fn((_port: number, _host: string, callback?: () => void) => {
+          if (callback) callback();
+          serverListeningCallback(undefined);
+        }),
+        close: vi.fn(),
+        on: vi.fn(),
+        address: () => ({ port: 3000 }),
+      };
+      (http.createServer as Mock).mockImplementation((cb) => {
+        requestCallback = cb;
+        return mockHttpServer as unknown as http.Server;
+      });
+
+      const clientPromise = getOauthClient(
+        AuthType.LOGIN_WITH_GOOGLE,
+        mockConfig,
+      );
+      await serverListeningPromise;
+
+      const mockReq = {
+        url: `/oauth2callback?code=${mockCode}&state=${mockState}`,
+      } as http.IncomingMessage;
+      const mockRes = {
+        writeHead: vi.fn(),
+        end: vi.fn(),
+      } as unknown as http.ServerResponse;
+
+      await requestCallback(mockReq, mockRes);
+      const client = await clientPromise;
+
+      // Authentication should succeed even if fetchAndCacheUserInfo fails
+      expect(client).toBe(mockOAuth2Client);
+      expect(consoleErrorSpy).toHaveBeenCalledWith(
+        'Failed to fetch user info:',
+        500,
+        'Internal Server Error',
+      );
+
+      consoleErrorSpy.mockRestore();
+    });
+
+    it('should handle user code authentication failure with descriptive error', async () => {
+      const mockConfigWithNoBrowser = {
+        getNoBrowser: () => true,
+        getProxy: () => 'http://test.proxy.com:8080',
+        isBrowserLaunchSuppressed: () => true,
+      } as unknown as Config;
+
+      const mockOAuth2Client = {
+        generateCodeVerifierAsync: vi.fn().mockResolvedValue({
+          codeChallenge: 'test-challenge',
+          codeVerifier: 'test-verifier',
+        }),
+        generateAuthUrl: vi.fn().mockReturnValue('https://example.com/auth'),
+        getToken: vi
+          .fn()
+          .mockRejectedValue(new Error('Invalid authorization code')),
+        on: vi.fn(),
+      } as unknown as OAuth2Client;
+      (OAuth2Client as unknown as Mock).mockImplementation(
+        () => mockOAuth2Client,
+      );
+
+      const mockReadline = {
+        question: vi.fn((_query, callback) => callback('invalid-code')),
+        close: vi.fn(),
+      };
+      (readline.createInterface as Mock).mockReturnValue(mockReadline);
+
+      const consoleLogSpy = vi
+        .spyOn(console, 'log')
+        .mockImplementation(() => {});
+      const consoleErrorSpy = vi
+        .spyOn(console, 'error')
+        .mockImplementation(() => {});
+
+      await expect(
+        getOauthClient(AuthType.LOGIN_WITH_GOOGLE, mockConfigWithNoBrowser),
+      ).rejects.toThrow('Failed to authenticate with user code.');
+
+      expect(consoleErrorSpy).toHaveBeenCalledWith(
+        'Failed to authenticate with authorization code:',
+        'Invalid authorization code',
+      );
+
+      consoleLogSpy.mockRestore();
+      consoleErrorSpy.mockRestore();
+    });
+  });
+
  describe('clearCachedCredentialFile', () => {
    it('should clear cached credentials and Google account', async () => {
      const cachedCreds = { refresh_token: 'test-token' };
--- a/packages/core/src/code_assist/oauth2.ts
+++ b/packages/core/src/code_assist/oauth2.ts
@@ -97,8 +97,9 @@ async function initOauthClient(
    if (!userAccountManager.getCachedGoogleAccount()) {
      try {
        await fetchAndCacheUserInfo(client);
-      } catch {
+      } catch (error) {
        // Non-fatal, continue with existing auth.
+        console.warn('Failed to fetch user info:', getErrorMessage(error));
      }
    }
    console.log('Loaded cached credentials.');
@@ -164,23 +165,39 @@ async function initOauthClient(
      // Without this, if `open` fails to spawn a process (e.g., `xdg-open` is not found
      // in a minimal Docker container), it will emit an unhandled 'error' event,
      // causing the entire Node.js process to crash.
-      childProcess.on('error', (_) => {
+      childProcess.on('error', (error) => {
        console.error(
          'Failed to open browser automatically. Please try running again with NO_BROWSER=true set.',
        );
-        throw new FatalAuthenticationError('Failed to open browser.');
+        console.error('Browser error details:', getErrorMessage(error));
      });
    } catch (err) {
      console.error(
        'An unexpected error occurred while trying to open the browser:',
-        err,
-        '\nPlease try running again with NO_BROWSER=true set.',
+        getErrorMessage(err),
+        '\nThis might be due to browser compatibility issues or system configuration.',
+        '\nPlease try running again with NO_BROWSER=true set for manual authentication.',
+      );
+      throw new FatalAuthenticationError(
+        `Failed to open browser: ${getErrorMessage(err)}`,
      );
-      throw new FatalAuthenticationError('Failed to open browser.');
    }
    console.log('Waiting for authentication...');

-    await webLogin.loginCompletePromise;
+    // Add timeout to prevent infinite waiting when browser tab gets stuck
+    const authTimeout = 5 * 60 * 1000; // 5 minutes timeout
+    const timeoutPromise = new Promise<never>((_, reject) => {
+      setTimeout(() => {
+        reject(
+          new FatalAuthenticationError(
+            'Authentication timed out after 5 minutes. The browser tab may have gotten stuck in a loading state. ' +
+              'Please try again or use NO_BROWSER=true for manual authentication.',
+          ),
+        );
+      }, authTimeout);
+    });
+
+    await Promise.race([webLogin.loginCompletePromise, timeoutPromise]);
  }

  return client;
@@ -236,7 +253,11 @@ async function authWithUserCode(client: OAuth2Client): Promise<boolean> {
      redirect_uri: redirectUri,
    });
    client.setCredentials(tokens);
-  } catch (_error) {
+  } catch (error) {
+    console.error(
+      'Failed to authenticate with authorization code:',
+      getErrorMessage(error),
+    );
    return false;
  }
  return true;
@@ -265,7 +286,11 @@ async function authWithWeb(client: OAuth2Client): Promise<OauthWebLogin> {
        if (req.url!.indexOf('/oauth2callback') === -1) {
          res.writeHead(HTTP_REDIRECT, { Location: SIGN_IN_FAILURE_URL });
          res.end();
-          reject(new Error('Unexpected request: ' + req.url));
+          reject(
+            new FatalAuthenticationError(
+              'OAuth callback not received. Unexpected request: ' + req.url,
+            ),
+          );
        }
        // acquire the code from the querystring, and close the web server.
        const qs = new url.URL(req.url!, 'http://localhost:3000').searchParams;
@@ -273,41 +298,87 @@ async function authWithWeb(client: OAuth2Client): Promise<OauthWebLogin> {
          res.writeHead(HTTP_REDIRECT, { Location: SIGN_IN_FAILURE_URL });
          res.end();

-          reject(new Error(`Error during authentication: ${qs.get('error')}`));
+          const errorCode = qs.get('error');
+          const errorDescription =
+            qs.get('error_description') || 'No additional details provided';
+          reject(
+            new FatalAuthenticationError(
+              `Google OAuth error: ${errorCode}. ${errorDescription}`,
+            ),
+          );
        } else if (qs.get('state') !== state) {
          res.end('State mismatch. Possible CSRF attack');

-          reject(new Error('State mismatch. Possible CSRF attack'));
+          reject(
+            new FatalAuthenticationError(
+              'OAuth state mismatch. Possible CSRF attack or browser session issue.',
+            ),
+          );
        } else if (qs.get('code')) {
-          const { tokens } = await client.getToken({
-            code: qs.get('code')!,
-            redirect_uri: redirectUri,
-          });
-          client.setCredentials(tokens);
-          // Retrieve and cache Google Account ID during authentication
          try {
-            await fetchAndCacheUserInfo(client);
-          } catch (error) {
-            console.error(
-              'Failed to retrieve Google Account ID during authentication:',
-              error,
-            );
-            // Don't fail the auth flow if Google Account ID retrieval fails
-          }
+            const { tokens } = await client.getToken({
+              code: qs.get('code')!,
+              redirect_uri: redirectUri,
+            });
+            client.setCredentials(tokens);

-          res.writeHead(HTTP_REDIRECT, { Location: SIGN_IN_SUCCESS_URL });
-          res.end();
-          resolve();
+            // Retrieve and cache Google Account ID during authentication
+            try {
+              await fetchAndCacheUserInfo(client);
+            } catch (error) {
+              console.warn(
+                'Failed to retrieve Google Account ID during authentication:',
+                getErrorMessage(error),
+              );
+              // Don't fail the auth flow if Google Account ID retrieval fails
+            }
+
+            res.writeHead(HTTP_REDIRECT, { Location: SIGN_IN_SUCCESS_URL });
+            res.end();
+            resolve();
+          } catch (error) {
+            res.writeHead(HTTP_REDIRECT, { Location: SIGN_IN_FAILURE_URL });
+            res.end();
+            reject(
+              new FatalAuthenticationError(
+                `Failed to exchange authorization code for tokens: ${getErrorMessage(error)}`,
+              ),
+            );
+          }
        } else {
-          reject(new Error('No code found in request'));
+          reject(
+            new FatalAuthenticationError(
+              'No authorization code received from Google OAuth. Please try authenticating again.',
+            ),
+          );
        }
      } catch (e) {
-        reject(e);
+        // Provide more specific error message for unexpected errors during OAuth flow
+        if (e instanceof FatalAuthenticationError) {
+          reject(e);
+        } else {
+          reject(
+            new FatalAuthenticationError(
+              `Unexpected error during OAuth authentication: ${getErrorMessage(e)}`,
+            ),
+          );
+        }
      } finally {
        server.close();
      }
    });
-    server.listen(port, host);
+
+    server.listen(port, host, () => {
+      // Server started successfully
+    });
+
+    server.on('error', (err) => {
+      reject(
+        new FatalAuthenticationError(
+          `OAuth callback server error: ${getErrorMessage(err)}`,
+        ),
+      );
+    });
  });

  return {
@@ -368,8 +439,12 @@ async function loadCachedCredentials(client: OAuth2Client): Promise<boolean> {
      await client.getTokenInfo(token);

      return true;
-    } catch (_) {
-      // Ignore and try next path.
+    } catch (error) {
+      // Log specific error for debugging, but continue trying other paths
+      console.debug(
+        `Failed to load credentials from ${keyFile}:`,
+        getErrorMessage(error),
+      );
    }
  }