From 876d091602ddd63fdeb0df56db3092c73e385250 Mon Sep 17 00:00:00 2001 From: Arya Gummadi Date: Wed, 3 Sep 2025 13:51:29 -0700 Subject: [PATCH] fix(auth): improve Google OAuth error handling and prevent empty error messages (#7539) --- packages/core/src/code_assist/oauth2.test.ts | 357 +++++++++++++++++++ packages/core/src/code_assist/oauth2.ts | 141 ++++++-- 2 files changed, 465 insertions(+), 33 deletions(-) diff --git a/packages/core/src/code_assist/oauth2.test.ts b/packages/core/src/code_assist/oauth2.test.ts index f7d199772f..01ec0fe6b9 100644 --- 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' }; diff --git a/packages/core/src/code_assist/oauth2.ts b/packages/core/src/code_assist/oauth2.ts index 38be80f020..142a3791f0 100644 --- 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((_, 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 { 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 { 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 { 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 { 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), + ); } }