From 729f385c077c172e2e0b31cab1caa1abb35abd97 Mon Sep 17 00:00:00 2001 From: villahernandez-coder Date: Tue, 6 Oct 2026 20:03:28 +0000 Subject: [PATCH 1/5] fix(auth): prevent infinite verification and OAuth retry loops --- packages/cli/src/ui/AppContainer.tsx | 6 +- packages/cli/src/ui/auth/AuthDialog.tsx | 4 +- packages/cli/src/ui/auth/useAuth.test.tsx | 16 ++++++ packages/cli/src/ui/auth/useAuth.ts | 10 ++++ .../ui/components/ValidationDialog.test.tsx | 56 +++++++++++++++++++ .../src/ui/components/ValidationDialog.tsx | 19 +++++-- .../auth-provider/oauth2-provider.test.ts | 36 ++++++++++++ .../agents/auth-provider/oauth2-provider.ts | 37 +++++++++--- packages/core/src/code_assist/setup.test.ts | 23 ++++++++ packages/core/src/code_assist/setup.ts | 9 ++- packages/core/src/utils/googleQuotaErrors.ts | 7 ++- packages/core/src/utils/oauth-flow.test.ts | 15 +++++ packages/core/src/utils/oauth-flow.ts | 22 +++++++- packages/core/src/utils/retry.test.ts | 47 ++++++++++++++++ packages/core/src/utils/retry.ts | 10 +++- 15 files changed, 296 insertions(+), 21 deletions(-) diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index 801e230c8aa..0cb3bdbf032 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -743,6 +743,10 @@ export const AppContainer = (props: AppContainerProps) => { } }, [authState, authContext, setAuthState]); + const handleShowAuthSelection = useCallback(() => { + setAuthState(AuthState.Updating); + }, [setAuthState]); + const { proQuotaRequest, handleProQuotaChoice, @@ -760,7 +764,7 @@ export const AppContainer = (props: AppContainerProps) => { paidTier, settings, setModelSwitchedFromQuotaError, - onShowAuthSelection: () => setAuthState(AuthState.Updating), + onShowAuthSelection: handleShowAuthSelection, errorVerbosity: settings.merged.ui.errorVerbosity, }); diff --git a/packages/cli/src/ui/auth/AuthDialog.tsx b/packages/cli/src/ui/auth/AuthDialog.tsx index e5b08e6cc2d..123e43363d2 100644 --- a/packages/cli/src/ui/auth/AuthDialog.tsx +++ b/packages/cli/src/ui/auth/AuthDialog.tsx @@ -162,8 +162,8 @@ export function AuthDialog({ if (error) { onAuthError(error); } else { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - onSelect(authMethod, SettingScope.User); + onAuthError(null); + await onSelect(authMethod, SettingScope.User); } }; diff --git a/packages/cli/src/ui/auth/useAuth.test.tsx b/packages/cli/src/ui/auth/useAuth.test.tsx index d512846ee2f..2a7c09d5216 100644 --- a/packages/cli/src/ui/auth/useAuth.test.tsx +++ b/packages/cli/src/ui/auth/useAuth.test.tsx @@ -331,5 +331,21 @@ describe('useAuth', () => { expect(result.current.authError).not.toContain('Failed to login'); expect(result.current.authState).toBe(AuthState.Updating); }); + + it('should transition to Updating without setting authError on ChangeAuthRequestedError or ValidationCancelledError', async () => { + const { ChangeAuthRequestedError } = await import( + '@google/gemini-cli-core' + ); + const { result } = await renderHook(() => + useAuthCommand(createSettings(AuthType.LOGIN_WITH_GOOGLE), mockConfig), + ); + + await act(async () => { + deferredRefreshAuth.reject(new ChangeAuthRequestedError()); + }); + + expect(result.current.authError).toBeNull(); + expect(result.current.authState).toBe(AuthState.Updating); + }); }); }); diff --git a/packages/cli/src/ui/auth/useAuth.ts b/packages/cli/src/ui/auth/useAuth.ts index caa9ed2c4b8..37e7631f638 100644 --- a/packages/cli/src/ui/auth/useAuth.ts +++ b/packages/cli/src/ui/auth/useAuth.ts @@ -13,6 +13,8 @@ import { debugLogger, isAccountSuspendedError, ProjectIdRequiredError, + ChangeAuthRequestedError, + ValidationCancelledError, } from '@google/gemini-cli-core'; import { getErrorMessage } from '@google/gemini-cli-core'; import { AuthState } from '../types.js'; @@ -141,6 +143,14 @@ export const useAuthCommand = ( setAuthError(null); setAuthState(AuthState.Authenticated); } catch (e) { + if ( + e instanceof ChangeAuthRequestedError || + e instanceof ValidationCancelledError + ) { + setAuthError(null); + setAuthState(AuthState.Updating); + return; + } const suspendedError = isAccountSuspendedError(e); if (suspendedError) { setAccountSuspensionInfo({ diff --git a/packages/cli/src/ui/components/ValidationDialog.test.tsx b/packages/cli/src/ui/components/ValidationDialog.test.tsx index 11e559ebfdc..c411486b7c1 100644 --- a/packages/cli/src/ui/components/ValidationDialog.test.tsx +++ b/packages/cli/src/ui/components/ValidationDialog.test.tsx @@ -189,6 +189,62 @@ describe('ValidationDialog', () => { expect(lastFrame()).toContain('Waiting for verification...'); unmount(); }); + + it('should resolve verify after Enter in waiting state without resetting timer on re-render', async () => { + const { lastFrame, waitUntilReady, rerender, unmount } = await render( + , + ); + + const onSelect = (RadioButtonSelect as Mock).mock.calls[0][0].onSelect; + await act(async () => { + await onSelect('verify'); + }); + await waitUntilReady(); + + expect(lastFrame()).toContain('Waiting for verification...'); + + vi.useFakeTimers(); + try { + // Press Enter to confirm verification in browser is complete + await act(async () => { + mockKeypressHandler({ + name: 'enter', + ctrl: false, + shift: false, + alt: false, + cmd: false, + insertable: false, + sequence: '\r', + }); + }); + + // Advance 300ms, then simulate parent re-render with a new inline onChoice callback + await act(async () => { + await vi.advanceTimersByTimeAsync(300); + }); + const newOnChoice = vi.fn(); + rerender( + , + ); + + // Advance remaining 200ms — timer should fire at 500ms total without having been reset + await act(async () => { + await vi.advanceTimersByTimeAsync(200); + }); + + expect(newOnChoice).toHaveBeenCalledTimes(1); + expect(newOnChoice).toHaveBeenCalledWith('verify'); + } finally { + vi.useRealTimers(); + } + unmount(); + }); }); describe('headless mode', () => { diff --git a/packages/cli/src/ui/components/ValidationDialog.tsx b/packages/cli/src/ui/components/ValidationDialog.tsx index b6c9ab213e7..a0305e631f6 100644 --- a/packages/cli/src/ui/components/ValidationDialog.tsx +++ b/packages/cli/src/ui/components/ValidationDialog.tsx @@ -5,7 +5,7 @@ */ import type React from 'react'; -import { useState, useEffect, useCallback } from 'react'; +import { useState, useEffect, useCallback, useRef } from 'react'; import { Box, Text } from 'ink'; import { RadioButtonSelect } from './shared/RadioButtonSelect.js'; import { theme } from '../semantic-colors.js'; @@ -36,6 +36,9 @@ export function ValidationDialog({ const keyMatchers = useKeyMatchers(); const [state, setState] = useState('choosing'); const [errorMessage, setErrorMessage] = useState(''); + const onChoiceRef = useRef(onChoice); + onChoiceRef.current = onChoice; + const hasCompletedRef = useRef(false); const items = [ { @@ -54,7 +57,7 @@ export function ValidationDialog({ useKeypress( (key) => { if (keyMatchers[Command.ESCAPE](key) || keyMatchers[Command.QUIT](key)) { - onChoice('cancel'); + onChoiceRef.current('cancel'); return true; } else if (state === 'waiting' && keyMatchers[Command.RETURN](key)) { // User confirmed verification is complete - transition to 'complete' state @@ -66,16 +69,20 @@ export function ValidationDialog({ { isActive: state !== 'complete' }, ); - // When state becomes 'complete', show success message briefly then proceed + // When state becomes 'complete', show success message briefly then proceed. + // Use onChoiceRef so parent re-renders do not reset the 500ms timer. useEffect(() => { - if (state === 'complete') { + if (state === 'complete' && !hasCompletedRef.current) { const timer = setTimeout(() => { - onChoice('verify'); + if (!hasCompletedRef.current) { + hasCompletedRef.current = true; + onChoiceRef.current('verify'); + } }, 500); return () => clearTimeout(timer); } return undefined; - }, [state, onChoice]); + }, [state]); const handleSelect = useCallback( async (choice: ValidationIntent) => { diff --git a/packages/core/src/agents/auth-provider/oauth2-provider.test.ts b/packages/core/src/agents/auth-provider/oauth2-provider.test.ts index a40b242d41f..ea62e0e5b52 100644 --- a/packages/core/src/agents/auth-provider/oauth2-provider.test.ts +++ b/packages/core/src/agents/auth-provider/oauth2-provider.test.ts @@ -399,6 +399,26 @@ describe('OAuth2AuthProvider', () => { /requires authorization_url and token_url/, ); }); + + it('should deduplicate concurrent interactive auth requests', async () => { + const provider = new OAuth2AuthProvider(createConfig(), 'test-agent'); + const storage = getTokenStorage(); + storage.getCredentials.mockResolvedValue(null); + + await provider.initialize(); + + const [headers1, headers2, headers3] = await Promise.all([ + provider.headers(), + provider.headers(), + provider.headers(), + ]); + + expect(headers1).toEqual({ Authorization: 'Bearer new-access-token' }); + expect(headers2).toEqual({ Authorization: 'Bearer new-access-token' }); + expect(headers3).toEqual({ Authorization: 'Bearer new-access-token' }); + expect(vi.mocked(startCallbackServer)).toHaveBeenCalledTimes(1); + expect(vi.mocked(exchangeCodeForToken)).toHaveBeenCalledTimes(1); + }); }); describe('shouldRetryWithHeaders', () => { @@ -482,6 +502,22 @@ describe('OAuth2AuthProvider', () => { const result = await provider.shouldRetryWithHeaders({}, res401); expect(result).toBeDefined(); }); + + it('should reset retry count when cached token is used on subsequent headers() call', async () => { + const provider = new OAuth2AuthProvider(createConfig(), 'test-agent'); + const res401 = new Response(null, { status: 401 }); + + // Consume both retries + await provider.shouldRetryWithHeaders({}, res401); + await provider.shouldRetryWithHeaders({}, res401); + + // A subsequent normal request uses the valid cached token, resetting the retry counter + await provider.headers(); + + // A later 401 should be allowed to retry again + const result = await provider.shouldRetryWithHeaders({}, res401); + expect(result).toBeDefined(); + }); }); describe('token persistence', () => { diff --git a/packages/core/src/agents/auth-provider/oauth2-provider.ts b/packages/core/src/agents/auth-provider/oauth2-provider.ts index c93a5b63dbc..50dd6404fb8 100644 --- a/packages/core/src/agents/auth-provider/oauth2-provider.ts +++ b/packages/core/src/agents/auth-provider/oauth2-provider.ts @@ -38,6 +38,7 @@ export class OAuth2AuthProvider extends BaseA2AAuthProvider { private readonly tokenStorage: MCPOAuthTokenStorage; private cachedToken: OAuthToken | null = null; + private pendingAuthPromise: Promise | null = null; /** Resolved OAuth URLs — may come from config or agent card. */ private authorizationUrl: string | undefined; @@ -86,18 +87,34 @@ export class OAuth2AuthProvider extends BaseA2AAuthProvider { /** * Return an Authorization header with a valid Bearer token. - * Refreshes or triggers interactive auth as needed. + * Refreshes or triggers interactive auth as needed, deduplicating concurrent requests. */ override async headers(): Promise { - // 1. Valid cached token → return immediately. + // 1. Valid cached token → return immediately and reset retry counter. if ( this.cachedToken && !this.tokenStorage.isTokenExpired(this.cachedToken) ) { + this.authRetryCount = 0; return { Authorization: `Bearer ${this.cachedToken.accessToken}` }; } - // 2. Expired but has refresh token → attempt silent refresh. + // 2. Coalesce concurrent token refresh / interactive auth requests. + if (!this.pendingAuthPromise) { + this.pendingAuthPromise = this.acquireToken().finally(() => { + this.pendingAuthPromise = null; + }); + } + + const token = await this.pendingAuthPromise; + return { Authorization: `Bearer ${token.accessToken}` }; + } + + /** + * Refresh an expired token or run interactive browser-based authentication. + */ + private async acquireToken(): Promise { + // 1. Expired but has refresh token → attempt silent refresh. if ( this.cachedToken?.refreshToken && this.tokenUrl && @@ -119,19 +136,20 @@ export class OAuth2AuthProvider extends BaseA2AAuthProvider { this.cachedToken.refreshToken, ); await this.persistToken(); - return { Authorization: `Bearer ${this.cachedToken.accessToken}` }; + return this.cachedToken; } catch (error) { debugLogger.debug( `[OAuth2AuthProvider] Refresh failed, falling back to interactive flow: ${getErrorMessage(error)}`, ); // Clear stale credentials and fall through to interactive flow. + this.cachedToken = null; await this.tokenStorage.deleteCredentials(this.agentName); } } - // 3. No valid token → interactive browser-based auth. + // 2. No valid token → interactive browser-based auth. this.cachedToken = await this.authenticateInteractively(); - return { Authorization: `Bearer ${this.cachedToken.accessToken}` }; + return this.cachedToken; } /** @@ -246,7 +264,12 @@ export class OAuth2AuthProvider extends BaseA2AAuthProvider { `Authentication required for A2A agent: '${this.agentName}'.`, ); if (!consent) { - throw new FatalCancellationError('Authentication cancelled by user.'); + const cancelError = new FatalCancellationError( + 'Authentication cancelled by user.', + ); + callbackServer.response.catch(() => {}); + callbackServer.cancel?.(cancelError); + throw cancelError; } coreEvents.emitFeedback( diff --git a/packages/core/src/code_assist/setup.test.ts b/packages/core/src/code_assist/setup.test.ts index a76525470f4..a8ba72a4392 100644 --- a/packages/core/src/code_assist/setup.test.ts +++ b/packages/core/src/code_assist/setup.test.ts @@ -415,6 +415,29 @@ describe('setupUser', () => { 'LoadCodeAssist returned empty response', ); }); + + it('should bound verification retries and throw ValidationRequiredError if verification repeatedly fails', async () => { + mockLoad.mockResolvedValue({ + currentTier: null, + ineligibleTiers: [ + { + reasonMessage: 'Verify please', + reasonCode: 'VALIDATION_REQUIRED', + tierId: UserTierId.STANDARD, + tierName: 'standard', + validationUrl: 'https://verify', + }, + ], + }); + + mockValidationHandler.mockResolvedValue('verify'); + + await expect(setupUser({} as OAuth2Client, mockConfig)).rejects.toThrow( + ValidationRequiredError, + ); + expect(mockValidationHandler).toHaveBeenCalledTimes(3); + expect(mockLoad).toHaveBeenCalledTimes(4); + }); }); }); diff --git a/packages/core/src/code_assist/setup.ts b/packages/core/src/code_assist/setup.ts index 6d4cbfd9c07..3019c36d60b 100644 --- a/packages/core/src/code_assist/setup.ts +++ b/packages/core/src/code_assist/setup.ts @@ -171,6 +171,8 @@ async function _doSetupUser( }; const validationHandler = config.getValidationHandler(); + const MAX_VALIDATION_ATTEMPTS = 3; + let validationAttempts = 0; let loadRes: LoadCodeAssistResponse; while (true) { @@ -186,7 +188,12 @@ async function _doSetupUser( validateLoadCodeAssistResponse(loadRes); break; } catch (e) { - if (e instanceof ValidationRequiredError && validationHandler) { + if ( + e instanceof ValidationRequiredError && + validationHandler && + validationAttempts < MAX_VALIDATION_ATTEMPTS + ) { + validationAttempts++; const intent = await validationHandler( e.validationLink, e.validationDescription, diff --git a/packages/core/src/utils/googleQuotaErrors.ts b/packages/core/src/utils/googleQuotaErrors.ts index dcdbeae1e51..d9902a86504 100644 --- a/packages/core/src/utils/googleQuotaErrors.ts +++ b/packages/core/src/utils/googleQuotaErrors.ts @@ -191,7 +191,12 @@ function classifyValidationRequiredError( // Fallback to ErrorInfo metadata if Help detail not found if (!validationLink) { - validationLink = errorInfo.metadata?.['validation_link']; + validationLink = + errorInfo.metadata?.['validation_link'] ?? + errorInfo.metadata?.['validation_url']; + } + if (!learnMoreUrl) { + learnMoreUrl = errorInfo.metadata?.['validation_learn_more_url']; } return new ValidationRequiredError( diff --git a/packages/core/src/utils/oauth-flow.test.ts b/packages/core/src/utils/oauth-flow.test.ts index e87b8ded5e8..e76f09a98f4 100644 --- a/packages/core/src/utils/oauth-flow.test.ts +++ b/packages/core/src/utils/oauth-flow.test.ts @@ -466,6 +466,21 @@ describe('oauth-flow', () => { vi.useRealTimers(); } }); + + it('should close the server and reject response when cancel is called', async () => { + const server = startCallbackServer('cancel-state'); + await server.port; + + const responseResult = server.response.then( + () => new Error('Expected rejection'), + (e: Error) => e, + ); + + server.cancel?.(new Error('Cancelled by caller')); + + const error = await responseResult; + expect(error.message).toBe('Cancelled by caller'); + }); }); describe('exchangeCodeForToken', () => { diff --git a/packages/core/src/utils/oauth-flow.ts b/packages/core/src/utils/oauth-flow.ts index 9c7029639d0..a3039a82caf 100644 --- a/packages/core/src/utils/oauth-flow.ts +++ b/packages/core/src/utils/oauth-flow.ts @@ -149,6 +149,7 @@ export function startCallbackServer( ): { port: Promise; response: Promise; + cancel?: (reason?: Error) => void; } { let portResolve: (port: number) => void; let portReject: (error: Error) => void; @@ -158,6 +159,14 @@ export function startCallbackServer( }); let timeoutId: NodeJS.Timeout | undefined; + const abortController = new AbortController(); + + const clearCallbackTimeout = () => { + if (timeoutId !== undefined) { + clearTimeout(timeoutId); + timeoutId = undefined; + } + }; const responsePromise = new Promise( (resolve, reject) => { @@ -190,6 +199,7 @@ export function startCallbackServer( `); + clearCallbackTimeout(); server.close(); reject(new Error(`OAuth error: ${error}`)); return; @@ -204,6 +214,7 @@ export function startCallbackServer( if (state !== expectedState) { res.writeHead(400); res.end('Invalid state parameter'); + clearCallbackTimeout(); server.close(); reject(new Error('State mismatch - possible CSRF attack')); return; @@ -221,9 +232,11 @@ export function startCallbackServer( `); + clearCallbackTimeout(); server.close(); resolve({ code, state }); } catch (error) { + clearCallbackTimeout(); server.close(); reject(error); } @@ -231,6 +244,7 @@ export function startCallbackServer( ); server.on('error', (error) => { + clearCallbackTimeout(); portReject(error); reject(error); }); @@ -264,7 +278,6 @@ export function startCallbackServer( portResolve(serverPort); // Resolve port promise immediately }); - const abortController = new AbortController(); timeoutId = setTimeout( () => { abortController.abort(new Error('OAuth callback timeout')); @@ -274,12 +287,14 @@ export function startCallbackServer( timeoutId.unref(); const onAbort = () => { + clearCallbackTimeout(); server.close(); reject(abortController.signal.reason); }; abortController.signal.addEventListener('abort', onAbort, { once: true }); server.on('close', () => { + clearCallbackTimeout(); abortController.signal.removeEventListener('abort', onAbort); }); }, @@ -288,6 +303,11 @@ export function startCallbackServer( return { port: portPromise, response: responsePromise, + cancel: (reason?: Error) => { + if (!abortController.signal.aborted) { + abortController.abort(reason ?? new Error('OAuth callback cancelled')); + } + }, }; } diff --git a/packages/core/src/utils/retry.test.ts b/packages/core/src/utils/retry.test.ts index 58cd3d890bb..887604649eb 100644 --- a/packages/core/src/utils/retry.test.ts +++ b/packages/core/src/utils/retry.test.ts @@ -15,6 +15,7 @@ import { debugLogger } from './debugLogger.js'; import { TerminalQuotaError, RetryableQuotaError, + ValidationRequiredError, } from './googleQuotaErrors.js'; import { PREVIEW_GEMINI_MODEL } from '../config/models.js'; import type { ModelPolicy } from '../availability/modelPolicy.js'; @@ -1055,4 +1056,50 @@ describe('retryWithBackoff', () => { expect(onPersistent429).toHaveBeenCalledTimes(1); }); }); + + describe('ValidationRequiredError handling', () => { + it('should retry and resolve when onValidationRequired returns verify', async () => { + const validationError = new ValidationRequiredError( + 'Validation required', + undefined, + 'https://accounts.google.com/verify', + 'Please verify', + ); + const fn = vi + .fn() + .mockRejectedValueOnce(validationError) + .mockResolvedValueOnce('verified-success'); + const onValidationRequired = vi.fn().mockResolvedValue('verify'); + + const result = await retryWithBackoff(fn, { + maxAttempts: 2, + onValidationRequired, + }); + + expect(result).toBe('verified-success'); + expect(onValidationRequired).toHaveBeenCalledTimes(1); + expect(fn).toHaveBeenCalledTimes(2); + }); + + it('should bound verification retries to prevent an infinite verification loop', async () => { + const validationError = new ValidationRequiredError( + 'Validation required', + undefined, + 'https://accounts.google.com/verify', + 'Please verify', + ); + const fn = vi.fn().mockRejectedValue(validationError); + const onValidationRequired = vi.fn().mockResolvedValue('verify'); + + await expect( + retryWithBackoff(fn, { + maxAttempts: 2, + onValidationRequired, + }), + ).rejects.toThrow(ValidationRequiredError); + + expect(onValidationRequired).toHaveBeenCalledTimes(3); + expect(fn).toHaveBeenCalledTimes(4); + }); + }); }); diff --git a/packages/core/src/utils/retry.ts b/packages/core/src/utils/retry.ts index be7d7deacef..e80e4bd65ab 100644 --- a/packages/core/src/utils/retry.ts +++ b/packages/core/src/utils/retry.ts @@ -295,7 +295,9 @@ export async function retryWithBackoff( let attempt = 0; let capacityAttempts = 0; + let validationAttempts = 0; const MAX_SILENT_CAPACITY_ATTEMPTS = 3; + const MAX_VALIDATION_ATTEMPTS = 3; let currentDelay = initialDelayMs; const throwIfAborted = () => { if (signal?.aborted) { @@ -421,11 +423,15 @@ export async function retryWithBackoff( // Handle ValidationRequiredError - user needs to verify before proceeding if (classifiedError instanceof ValidationRequiredError) { - if (onValidationRequired) { + if ( + onValidationRequired && + validationAttempts < MAX_VALIDATION_ATTEMPTS + ) { + validationAttempts++; try { const intent = await onValidationRequired(classifiedError); if (intent === 'verify') { - // User verified, retry the request + // User verified, retry the request (bounded by MAX_VALIDATION_ATTEMPTS) attempt = 0; currentDelay = initialDelayMs; continue; From 410ed870b39ed4cc916255e41fc9855244e15ecc Mon Sep 17 00:00:00 2001 From: villahernandez-coder Date: Tue, 6 Oct 2026 22:49:48 +0000 Subject: [PATCH 2/5] fix(auth): address code review feedback on retry counter reset and onSelect error handling --- packages/cli/src/ui/auth/AuthDialog.test.tsx | 25 ++++++++++++++++++- packages/cli/src/ui/auth/AuthDialog.tsx | 6 ++++- .../auth-provider/oauth2-provider.test.ts | 20 +++++++++++++++ .../agents/auth-provider/oauth2-provider.ts | 8 ++++-- 4 files changed, 55 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/ui/auth/AuthDialog.test.tsx b/packages/cli/src/ui/auth/AuthDialog.test.tsx index 40ec0b301da..043c7219509 100644 --- a/packages/cli/src/ui/auth/AuthDialog.test.tsx +++ b/packages/cli/src/ui/auth/AuthDialog.test.tsx @@ -16,7 +16,12 @@ import { type Mock, } from 'vitest'; import { AuthDialog } from './AuthDialog.js'; -import { AuthType, type Config, debugLogger } from '@google/gemini-cli-core'; +import { + AuthType, + clearCachedCredentialFile, + type Config, + debugLogger, +} from '@google/gemini-cli-core'; import type { LoadedSettings } from '../../config/settings.js'; import { AuthState } from '../types.js'; import { RadioButtonSelect } from '../components/shared/RadioButtonSelect.js'; @@ -371,6 +376,24 @@ describe('AuthDialog', () => { vi.useRealTimers(); unmount(); }); + + it('catches errors thrown during onSelect and forwards them to onAuthError', async () => { + mockedValidateAuthMethod.mockResolvedValue(null); + vi.mocked(clearCachedCredentialFile).mockRejectedValueOnce( + new Error('Failed to clear credentials'), + ); + + const { unmount } = await renderWithProviders(); + const { onSelect: handleAuthSelect } = + mockedRadioButtonSelect.mock.calls[0][0]; + await handleAuthSelect(AuthType.LOGIN_WITH_GOOGLE); + + expect(props.onAuthError).toHaveBeenCalledWith(null); + expect(props.onAuthError).toHaveBeenCalledWith( + 'Failed to clear credentials', + ); + unmount(); + }); }); it('displays authError when provided', async () => { diff --git a/packages/cli/src/ui/auth/AuthDialog.tsx b/packages/cli/src/ui/auth/AuthDialog.tsx index 123e43363d2..0e6001524d0 100644 --- a/packages/cli/src/ui/auth/AuthDialog.tsx +++ b/packages/cli/src/ui/auth/AuthDialog.tsx @@ -163,7 +163,11 @@ export function AuthDialog({ onAuthError(error); } else { onAuthError(null); - await onSelect(authMethod, SettingScope.User); + try { + await onSelect(authMethod, SettingScope.User); + } catch (e) { + onAuthError(e instanceof Error ? e.message : String(e)); + } } }; diff --git a/packages/core/src/agents/auth-provider/oauth2-provider.test.ts b/packages/core/src/agents/auth-provider/oauth2-provider.test.ts index ea62e0e5b52..bc42095a9b9 100644 --- a/packages/core/src/agents/auth-provider/oauth2-provider.test.ts +++ b/packages/core/src/agents/auth-provider/oauth2-provider.test.ts @@ -518,6 +518,26 @@ describe('OAuth2AuthProvider', () => { const result = await provider.shouldRetryWithHeaders({}, res401); expect(result).toBeDefined(); }); + + it('should reset retry count when a new token is acquired on subsequent headers() call', async () => { + const provider = new OAuth2AuthProvider(createConfig(), 'test-agent'); + const storage = getTokenStorage(); + const res401 = new Response(null, { status: 401 }); + + // Consume both retries + await provider.shouldRetryWithHeaders({}, res401); + await provider.shouldRetryWithHeaders({}, res401); + + // Simulate token expiring before the next normal request + storage.isTokenExpired.mockReturnValueOnce(true); + + // Acquiring a fresh token via headers() resets the retry counter + await provider.headers(); + + // A later 401 should be allowed to retry again + const result = await provider.shouldRetryWithHeaders({}, res401); + expect(result).toBeDefined(); + }); }); describe('token persistence', () => { diff --git a/packages/core/src/agents/auth-provider/oauth2-provider.ts b/packages/core/src/agents/auth-provider/oauth2-provider.ts index 50dd6404fb8..841409dec61 100644 --- a/packages/core/src/agents/auth-provider/oauth2-provider.ts +++ b/packages/core/src/agents/auth-provider/oauth2-provider.ts @@ -107,6 +107,7 @@ export class OAuth2AuthProvider extends BaseA2AAuthProvider { } const token = await this.pendingAuthPromise; + this.authRetryCount = 0; return { Authorization: `Bearer ${token.accessToken}` }; } @@ -167,7 +168,8 @@ export class OAuth2AuthProvider extends BaseA2AAuthProvider { if (this.authRetryCount >= BaseA2AAuthProvider.MAX_AUTH_RETRIES) { return undefined; } - this.authRetryCount++; + const nextRetryCount = this.authRetryCount + 1; + this.authRetryCount = nextRetryCount; debugLogger.debug( '[OAuth2AuthProvider] Auth failure, clearing token and re-authenticating', @@ -175,7 +177,9 @@ export class OAuth2AuthProvider extends BaseA2AAuthProvider { this.cachedToken = null; await this.tokenStorage.deleteCredentials(this.agentName); - return this.headers(); + const headers = await this.headers(); + this.authRetryCount = nextRetryCount; + return headers; } // --------------------------------------------------------------------------- From 466c271b2eb41a6dc29bb83a2a0a8fd683b880d1 Mon Sep 17 00:00:00 2001 From: villahernandez-coder Date: Tue, 6 Oct 2026 23:03:53 +0000 Subject: [PATCH 3/5] fix(auth): deduplicate concurrent 401/403 retries in shouldRetryWithHeaders --- .../auth-provider/oauth2-provider.test.ts | 39 +++++++++++++++++++ .../agents/auth-provider/oauth2-provider.ts | 31 ++++++++++++++- 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/packages/core/src/agents/auth-provider/oauth2-provider.test.ts b/packages/core/src/agents/auth-provider/oauth2-provider.test.ts index bc42095a9b9..e12b32f88f5 100644 --- a/packages/core/src/agents/auth-provider/oauth2-provider.test.ts +++ b/packages/core/src/agents/auth-provider/oauth2-provider.test.ts @@ -538,6 +538,45 @@ describe('OAuth2AuthProvider', () => { const result = await provider.shouldRetryWithHeaders({}, res401); expect(result).toBeDefined(); }); + + it('should not increment retry count or delete credentials again when concurrent requests fail with the same stale token', async () => { + const provider = new OAuth2AuthProvider(createConfig(), 'test-agent'); + const storage = getTokenStorage(); + + storage.getCredentials.mockResolvedValue({ + serverName: 'test-agent', + token: { accessToken: 'stale-token', tokenType: 'Bearer' }, + updatedAt: Date.now(), + }); + storage.isTokenExpired.mockReturnValue(false); + + await provider.initialize(); + + const staleReq = { + headers: { Authorization: 'Bearer stale-token' }, + }; + const res401 = new Response(null, { status: 401 }); + + // Three concurrent requests that all used 'stale-token' fail with 401 + const [retry1, retry2, retry3] = await Promise.all([ + provider.shouldRetryWithHeaders(staleReq, res401), + provider.shouldRetryWithHeaders(staleReq, res401), + provider.shouldRetryWithHeaders(staleReq, res401), + ]); + + expect(retry1).toEqual({ Authorization: 'Bearer new-access-token' }); + expect(retry2).toEqual({ Authorization: 'Bearer new-access-token' }); + expect(retry3).toEqual({ Authorization: 'Bearer new-access-token' }); + expect(storage.deleteCredentials).toHaveBeenCalledTimes(1); + + // Because the 3 concurrent failures were for the same stale token, only 1 retry + // was consumed, so another subsequent 401 with the new token can still retry. + const nextRetry = await provider.shouldRetryWithHeaders( + { headers: { Authorization: 'Bearer new-access-token' } }, + res401, + ); + expect(nextRetry).toBeDefined(); + }); }); describe('token persistence', () => { diff --git a/packages/core/src/agents/auth-provider/oauth2-provider.ts b/packages/core/src/agents/auth-provider/oauth2-provider.ts index 841409dec61..5dea09d039c 100644 --- a/packages/core/src/agents/auth-provider/oauth2-provider.ts +++ b/packages/core/src/agents/auth-provider/oauth2-provider.ts @@ -153,11 +153,18 @@ export class OAuth2AuthProvider extends BaseA2AAuthProvider { return this.cachedToken; } + /** + * Return the maximum number of consecutive authentication retries allowed. + */ + protected getMaxAuthRetries(): number { + return BaseA2AAuthProvider.MAX_AUTH_RETRIES; + } + /** * On 401/403, clear the cached token and re-authenticate (up to MAX_AUTH_RETRIES). */ override async shouldRetryWithHeaders( - _req: RequestInit, + req: RequestInit, res: Response, ): Promise { if (res.status !== 401 && res.status !== 403) { @@ -165,7 +172,27 @@ export class OAuth2AuthProvider extends BaseA2AAuthProvider { return undefined; } - if (this.authRetryCount >= BaseA2AAuthProvider.MAX_AUTH_RETRIES) { + const reqHeaders = new Headers(req.headers); + const reqToken = reqHeaders.get('Authorization')?.split(' ')[1]; + + // If the token used in the request is already different from our current cached token + // (or if the cached token has already been cleared), another concurrent request has + // already initiated the re-authentication flow. We can safely retry with the new + // token without incrementing the retry count or clearing credentials again. + if ( + reqToken && + (!this.cachedToken || reqToken !== this.cachedToken.accessToken) + ) { + debugLogger.debug( + '[OAuth2AuthProvider] Token already cleared or updated by another request, retrying with new token', + ); + const retryCount = this.authRetryCount; + const headers = await this.headers(); + this.authRetryCount = Math.max(this.authRetryCount, retryCount); + return headers; + } + + if (this.authRetryCount >= this.getMaxAuthRetries()) { return undefined; } const nextRetryCount = this.authRetryCount + 1; From 63360c30777c2f7a629be3cf1063017b6b3a0dc6 Mon Sep 17 00:00:00 2001 From: villahernandez-coder Date: Wed, 7 Oct 2026 14:56:04 +0000 Subject: [PATCH 4/5] test(cli): set existing auth type before testing onSelect credential clear error --- packages/cli/src/ui/auth/AuthDialog.test.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/cli/src/ui/auth/AuthDialog.test.tsx b/packages/cli/src/ui/auth/AuthDialog.test.tsx index 043c7219509..530ad0725ff 100644 --- a/packages/cli/src/ui/auth/AuthDialog.test.tsx +++ b/packages/cli/src/ui/auth/AuthDialog.test.tsx @@ -379,6 +379,7 @@ describe('AuthDialog', () => { it('catches errors thrown during onSelect and forwards them to onAuthError', async () => { mockedValidateAuthMethod.mockResolvedValue(null); + props.settings.merged.security.auth.selectedType = AuthType.USE_GEMINI; vi.mocked(clearCachedCredentialFile).mockRejectedValueOnce( new Error('Failed to clear credentials'), ); From 36cb0acf8c6eb1e334d92bbf94ec59e16a9410c2 Mon Sep 17 00:00:00 2001 From: villahernandez-coder Date: Wed, 7 Oct 2026 21:28:19 +0000 Subject: [PATCH 5/5] fix(core): check server.listening before closing in OAuth callback abort handler --- packages/core/src/utils/oauth-flow.test.ts | 14 ++++++++++++++ packages/core/src/utils/oauth-flow.ts | 4 +++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/packages/core/src/utils/oauth-flow.test.ts b/packages/core/src/utils/oauth-flow.test.ts index bd7b2ea4834..8e454cdd3e3 100644 --- a/packages/core/src/utils/oauth-flow.test.ts +++ b/packages/core/src/utils/oauth-flow.test.ts @@ -848,6 +848,20 @@ describe('oauth-flow', () => { const error = await responseResult; expect(error.message).toBe('Cancelled by caller'); }); + + it('should not throw if cancel is called after the server has already closed', async () => { + const server = startCallbackServer('my-state'); + const port = await server.port; + + await realFetch( + `http://localhost:${port}${REDIRECT_PATH}?code=auth-code-123&state=my-state`, + ); + await server.response; + + expect(() => { + server.cancel?.(new Error('Cancelled after close')); + }).not.toThrow(); + }); }); describe('exchangeCodeForToken', () => { diff --git a/packages/core/src/utils/oauth-flow.ts b/packages/core/src/utils/oauth-flow.ts index 80b0e24506b..dd524a76dbe 100644 --- a/packages/core/src/utils/oauth-flow.ts +++ b/packages/core/src/utils/oauth-flow.ts @@ -406,7 +406,9 @@ export function startCallbackServer( const onAbort = () => { clearCallbackTimeout(); - server.close(); + if (server.listening) { + server.close(); + } reject(abortController.signal.reason); }; abortController.signal.addEventListener('abort', onAbort, { once: true });