From 69f827348175d5040393f2f872c2a288bcde1ea7 Mon Sep 17 00:00:00 2001 From: Emily Hedlund Date: Tue, 3 Feb 2026 16:26:00 -0500 Subject: [PATCH] feat(core): require user consent before MCP server OAuth (#18132) --- packages/core/src/code_assist/oauth2.test.ts | 98 +++------------- packages/core/src/code_assist/oauth2.ts | 50 +-------- packages/core/src/index.ts | 1 + packages/core/src/mcp/oauth-provider.test.ts | 60 +++++++++- packages/core/src/mcp/oauth-provider.ts | 13 ++- packages/core/src/utils/authConsent.test.ts | 111 +++++++++++++++++++ packages/core/src/utils/authConsent.ts | 60 ++++++++++ 7 files changed, 255 insertions(+), 138 deletions(-) create mode 100644 packages/core/src/utils/authConsent.test.ts create mode 100644 packages/core/src/utils/authConsent.ts diff --git a/packages/core/src/code_assist/oauth2.test.ts b/packages/core/src/code_assist/oauth2.test.ts index 1ef5fc2f06..2cdbdad3cb 100644 --- a/packages/core/src/code_assist/oauth2.test.ts +++ b/packages/core/src/code_assist/oauth2.test.ts @@ -9,7 +9,6 @@ import type { Mock } from 'vitest'; import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { getOauthClient, - getConsentForOauth, resetOauthClientForTesting, clearCachedCredentialFile, clearOauthClientCache, @@ -30,10 +29,7 @@ import { FORCE_ENCRYPTED_FILE_ENV_VAR } from '../mcp/token-storage/index.js'; import { GEMINI_DIR, homedir as pathsHomedir } from '../utils/paths.js'; import { debugLogger } from '../utils/debugLogger.js'; import { writeToStdout } from '../utils/stdio.js'; -import { - FatalAuthenticationError, - FatalCancellationError, -} from '../utils/errors.js'; +import { FatalCancellationError } from '../utils/errors.js'; import process from 'node:process'; import { coreEvents } from '../utils/events.js'; @@ -1255,6 +1251,18 @@ describe('oauth2', () => { stdinOnSpy.mockRestore(); stdinRemoveListenerSpy.mockRestore(); }); + + it('should throw FatalCancellationError when consent is denied', async () => { + vi.spyOn(coreEvents, 'emitConsentRequest').mockImplementation( + (payload) => { + payload.onConfirm(false); + }, + ); + + await expect( + getOauthClient(AuthType.LOGIN_WITH_GOOGLE, mockConfig), + ).rejects.toThrow(FatalCancellationError); + }); }); describe('clearCachedCredentialFile', () => { @@ -1515,84 +1523,4 @@ describe('oauth2', () => { expect(fs.existsSync(credsPath)).toBe(true); // The unencrypted file should remain }); }); - - describe('getConsentForOauth', () => { - it('should use coreEvents when listeners are present', async () => { - vi.restoreAllMocks(); - const mockEmitConsentRequest = vi.spyOn(coreEvents, 'emitConsentRequest'); - const mockListenerCount = vi - .spyOn(coreEvents, 'listenerCount') - .mockReturnValue(1); - - mockEmitConsentRequest.mockImplementation((payload) => { - payload.onConfirm(true); - }); - - const result = await getConsentForOauth(); - - expect(result).toBe(true); - expect(mockEmitConsentRequest).toHaveBeenCalled(); - - mockListenerCount.mockRestore(); - mockEmitConsentRequest.mockRestore(); - }); - - it('should use readline when no listeners are present and stdin is a TTY', async () => { - vi.restoreAllMocks(); - const mockListenerCount = vi - .spyOn(coreEvents, 'listenerCount') - .mockReturnValue(0); - const originalIsTTY = process.stdin.isTTY; - Object.defineProperty(process.stdin, 'isTTY', { - value: true, - configurable: true, - }); - - const mockReadline = { - on: vi.fn((event, callback) => { - if (event === 'line') { - callback('y'); - } - }), - close: vi.fn(), - }; - (readline.createInterface as Mock).mockReturnValue(mockReadline); - - const result = await getConsentForOauth(); - - expect(result).toBe(true); - expect(readline.createInterface).toHaveBeenCalled(); - expect(writeToStdout).toHaveBeenCalledWith( - expect.stringContaining('Do you want to continue? [Y/n]: '), - ); - - mockListenerCount.mockRestore(); - Object.defineProperty(process.stdin, 'isTTY', { - value: originalIsTTY, - configurable: true, - }); - }); - - it('should throw FatalAuthenticationError when no listeners and not a TTY', async () => { - vi.restoreAllMocks(); - const mockListenerCount = vi - .spyOn(coreEvents, 'listenerCount') - .mockReturnValue(0); - const originalIsTTY = process.stdin.isTTY; - Object.defineProperty(process.stdin, 'isTTY', { - value: false, - configurable: true, - }); - - await expect(getConsentForOauth()).rejects.toThrow( - FatalAuthenticationError, - ); - - mockListenerCount.mockRestore(); - Object.defineProperty(process.stdin, 'isTTY', { - value: originalIsTTY, - configurable: true, - }); - }); - }); }); diff --git a/packages/core/src/code_assist/oauth2.ts b/packages/core/src/code_assist/oauth2.ts index a0bd86c174..0e4cb50ab6 100644 --- a/packages/core/src/code_assist/oauth2.ts +++ b/packages/core/src/code_assist/oauth2.ts @@ -45,6 +45,7 @@ import { exitAlternateScreen, } from '../utils/terminal.js'; import { coreEvents, CoreEvent } from '../utils/events.js'; +import { getConsentForOauth } from '../utils/authConsent.js'; export const authEvents = new EventEmitter(); @@ -269,7 +270,7 @@ async function initOauthClient( await triggerPostAuthCallbacks(client.credentials); } else { - const userConsent = await getConsentForOauth(); + const userConsent = await getConsentForOauth('Code Assist login required.'); if (!userConsent) { throw new FatalCancellationError('Authentication cancelled by user.'); } @@ -377,53 +378,6 @@ async function initOauthClient( return client; } -export async function getConsentForOauth(): Promise { - const prompt = - 'Code Assist login required. Opening authentication page in your browser. '; - - if (coreEvents.listenerCount(CoreEvent.ConsentRequest) === 0) { - if (!process.stdin.isTTY) { - throw new FatalAuthenticationError( - 'Code Assist login required, but interactive consent could not be obtained.\n' + - 'Please run Gemini CLI in an interactive terminal to authenticate, or use NO_BROWSER=true for manual authentication.', - ); - } - return getOauthConsentNonInteractive(prompt); - } - - return getOauthConsentInteractive(prompt); -} - -async function getOauthConsentNonInteractive(prompt: string) { - const rl = readline.createInterface({ - input: process.stdin, - output: createWorkingStdio().stdout, - terminal: true, - }); - - const fullPrompt = prompt + 'Do you want to continue? [Y/n]: '; - writeToStdout(`\n${fullPrompt}`); - - return new Promise((resolve) => { - rl.on('line', (answer) => { - rl.close(); - resolve(['y', ''].includes(answer.trim().toLowerCase())); - }); - }); -} - -async function getOauthConsentInteractive(prompt: string) { - const fullPrompt = prompt + '\n\nDo you want to continue?'; - return new Promise((resolve) => { - coreEvents.emitConsentRequest({ - prompt: fullPrompt, - onConfirm: (confirmed: boolean) => { - resolve(confirmed); - }, - }); - }); -} - export async function getOauthClient( authType: AuthType, config: Config, diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index f63c189014..41c11961fd 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -68,6 +68,7 @@ export * from './utils/gitUtils.js'; export * from './utils/editor.js'; export * from './utils/quotaErrorDetection.js'; export * from './utils/userAccountManager.js'; +export * from './utils/authConsent.js'; export * from './utils/googleQuotaErrors.js'; export * from './utils/fileUtils.js'; export * from './utils/planUtils.js'; diff --git a/packages/core/src/mcp/oauth-provider.test.ts b/packages/core/src/mcp/oauth-provider.test.ts index cda9b4f712..1d2859d3f5 100644 --- a/packages/core/src/mcp/oauth-provider.test.ts +++ b/packages/core/src/mcp/oauth-provider.test.ts @@ -33,6 +33,9 @@ vi.mock('../utils/events.js', () => ({ emitConsoleLog: vi.fn(), }, })); +vi.mock('../utils/authConsent.js', () => ({ + getConsentForOauth: vi.fn(() => Promise.resolve(true)), +})); import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import * as http from 'node:http'; @@ -43,6 +46,7 @@ import type { OAuthClientRegistrationResponse, } from './oauth-provider.js'; import { MCPOAuthProvider } from './oauth-provider.js'; +import { getConsentForOauth } from '../utils/authConsent.js'; import type { OAuthToken } from './token-storage/types.js'; import { MCPOAuthTokenStorage } from './oauth-token-storage.js'; import { @@ -51,6 +55,7 @@ import { type OAuthProtectedResourceMetadata, } from './oauth-utils.js'; import { coreEvents } from '../utils/events.js'; +import { FatalCancellationError } from '../utils/errors.js'; // Mock fetch globally const mockFetch = vi.fn(); @@ -1198,11 +1203,62 @@ describe('MCPOAuthProvider', () => { undefined, ); - expect(coreEvents.emitFeedback).toHaveBeenCalledWith( - 'info', + expect(getConsentForOauth).toHaveBeenCalledWith( expect.stringContaining('production-server'), ); }); + + it('should call openBrowserSecurely when consent is granted', async () => { + vi.mocked(getConsentForOauth).mockResolvedValue(true); + + vi.mocked(http.createServer).mockImplementation((handler) => { + setTimeout(() => { + const req = { + url: '/oauth/callback?code=code&state=bW9ja19zdGF0ZV8xNl9ieXRlcw', + } as http.IncomingMessage; + const res = { + writeHead: vi.fn(), + end: vi.fn(), + } as unknown as http.ServerResponse; + (handler as http.RequestListener)(req, res); + }, 0); + return mockHttpServer as unknown as http.Server; + }); + mockHttpServer.listen.mockImplementation((_port, callback) => + callback?.(), + ); + mockFetch.mockResolvedValue( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify(mockTokenResponse), + json: mockTokenResponse, + }), + ); + + const authProvider = new MCPOAuthProvider(); + await authProvider.authenticate('test-server', mockConfig); + + expect(mockOpenBrowserSecurely).toHaveBeenCalled(); + }); + + it('should throw FatalCancellationError when consent is denied', async () => { + vi.mocked(getConsentForOauth).mockResolvedValue(false); + mockHttpServer.listen.mockImplementation((_port, callback) => + callback?.(), + ); + + // Use fake timers to avoid hanging from the 5-minute timeout in startCallbackServer + vi.useFakeTimers(); + + const authProvider = new MCPOAuthProvider(); + await expect( + authProvider.authenticate('test-server', mockConfig), + ).rejects.toThrow(FatalCancellationError); + + expect(mockOpenBrowserSecurely).not.toHaveBeenCalled(); + vi.useRealTimers(); + }); }); describe('refreshAccessToken', () => { diff --git a/packages/core/src/mcp/oauth-provider.ts b/packages/core/src/mcp/oauth-provider.ts index 5947c6edf7..9f6ee36c2f 100644 --- a/packages/core/src/mcp/oauth-provider.ts +++ b/packages/core/src/mcp/oauth-provider.ts @@ -11,10 +11,11 @@ import { URL } from 'node:url'; import { openBrowserSecurely } from '../utils/secure-browser-launcher.js'; import type { OAuthToken } from './token-storage/types.js'; import { MCPOAuthTokenStorage } from './oauth-token-storage.js'; -import { getErrorMessage } from '../utils/errors.js'; +import { getErrorMessage, FatalCancellationError } from '../utils/errors.js'; import { OAuthUtils, ResourceMismatchError } from './oauth-utils.js'; import { coreEvents } from '../utils/events.js'; import { debugLogger } from '../utils/debugLogger.js'; +import { getConsentForOauth } from '../utils/authConsent.js'; export const OAUTH_DISPLAY_MESSAGE_EVENT = 'oauth-display-message' as const; @@ -898,8 +899,14 @@ export class MCPOAuthProvider { mcpServerUrl, ); - displayMessage(`Authentication required for MCP Server: '${serverName}' -→ Opening your browser for OAuth sign-in... + const userConsent = await getConsentForOauth( + `Authentication required for MCP Server: '${serverName}.'`, + ); + if (!userConsent) { + throw new FatalCancellationError('Authentication cancelled by user.'); + } + + displayMessage(`→ Opening your browser for OAuth sign-in... If the browser does not open, copy and paste this URL into your browser: ${authUrl} diff --git a/packages/core/src/utils/authConsent.test.ts b/packages/core/src/utils/authConsent.test.ts new file mode 100644 index 0000000000..1db8e105bc --- /dev/null +++ b/packages/core/src/utils/authConsent.test.ts @@ -0,0 +1,111 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { Mock } from 'vitest'; +import readline from 'node:readline'; +import process from 'node:process'; +import { coreEvents } from './events.js'; +import { getConsentForOauth } from './authConsent.js'; +import { FatalAuthenticationError } from './errors.js'; +import { writeToStdout } from './stdio.js'; + +vi.mock('node:readline'); +vi.mock('./stdio.js', () => ({ + writeToStdout: vi.fn(), + createWorkingStdio: vi.fn(() => ({ + stdout: process.stdout, + stderr: process.stderr, + })), +})); + +describe('getConsentForOauth', () => { + it('should use coreEvents when listeners are present', async () => { + vi.restoreAllMocks(); + const mockEmitConsentRequest = vi.spyOn(coreEvents, 'emitConsentRequest'); + const mockListenerCount = vi + .spyOn(coreEvents, 'listenerCount') + .mockReturnValue(1); + + mockEmitConsentRequest.mockImplementation((payload) => { + payload.onConfirm(true); + }); + + const result = await getConsentForOauth('Login required.'); + + expect(result).toBe(true); + expect(mockEmitConsentRequest).toHaveBeenCalledWith( + expect.objectContaining({ + prompt: expect.stringContaining( + 'Login required. Opening authentication page in your browser.', + ), + }), + ); + + mockListenerCount.mockRestore(); + mockEmitConsentRequest.mockRestore(); + }); + + it('should use readline when no listeners are present and stdin is a TTY', async () => { + vi.restoreAllMocks(); + const mockListenerCount = vi + .spyOn(coreEvents, 'listenerCount') + .mockReturnValue(0); + const originalIsTTY = process.stdin.isTTY; + Object.defineProperty(process.stdin, 'isTTY', { + value: true, + configurable: true, + }); + + const mockReadline = { + on: vi.fn((event, callback) => { + if (event === 'line') { + callback('y'); + } + }), + close: vi.fn(), + }; + (readline.createInterface as Mock).mockReturnValue(mockReadline); + + const result = await getConsentForOauth('Login required.'); + + expect(result).toBe(true); + expect(readline.createInterface).toHaveBeenCalled(); + expect(writeToStdout).toHaveBeenCalledWith( + expect.stringContaining( + 'Login required. Opening authentication page in your browser.', + ), + ); + + mockListenerCount.mockRestore(); + Object.defineProperty(process.stdin, 'isTTY', { + value: originalIsTTY, + configurable: true, + }); + }); + + it('should throw FatalAuthenticationError when no listeners and not a TTY', async () => { + vi.restoreAllMocks(); + const mockListenerCount = vi + .spyOn(coreEvents, 'listenerCount') + .mockReturnValue(0); + const originalIsTTY = process.stdin.isTTY; + Object.defineProperty(process.stdin, 'isTTY', { + value: false, + configurable: true, + }); + + await expect(getConsentForOauth('Login required.')).rejects.toThrow( + FatalAuthenticationError, + ); + + mockListenerCount.mockRestore(); + Object.defineProperty(process.stdin, 'isTTY', { + value: originalIsTTY, + configurable: true, + }); + }); +}); diff --git a/packages/core/src/utils/authConsent.ts b/packages/core/src/utils/authConsent.ts new file mode 100644 index 0000000000..859eaf10f3 --- /dev/null +++ b/packages/core/src/utils/authConsent.ts @@ -0,0 +1,60 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import readline from 'node:readline'; +import { CoreEvent, coreEvents } from './events.js'; +import { FatalAuthenticationError } from './errors.js'; +import { createWorkingStdio, writeToStdout } from './stdio.js'; + +/** + * Requests consent from the user for OAuth login. + * Handles both TTY and non-TTY environments. + */ +export async function getConsentForOauth(prompt: string): Promise { + const finalPrompt = prompt + ' Opening authentication page in your browser. '; + + if (coreEvents.listenerCount(CoreEvent.ConsentRequest) === 0) { + if (!process.stdin.isTTY) { + throw new FatalAuthenticationError( + 'Interactive consent could not be obtained.\n' + + 'Please run Gemini CLI in an interactive terminal to authenticate, or use NO_BROWSER=true for manual authentication.', + ); + } + return getOauthConsentNonInteractive(finalPrompt); + } + + return getOauthConsentInteractive(finalPrompt); +} + +async function getOauthConsentNonInteractive(prompt: string) { + const rl = readline.createInterface({ + input: process.stdin, + output: createWorkingStdio().stdout, + terminal: true, + }); + + const fullPrompt = prompt + 'Do you want to continue? [Y/n]: '; + writeToStdout(`\n${fullPrompt}`); + + return new Promise((resolve) => { + rl.on('line', (answer) => { + rl.close(); + resolve(['y', ''].includes(answer.trim().toLowerCase())); + }); + }); +} + +async function getOauthConsentInteractive(prompt: string) { + const fullPrompt = prompt + '\n\nDo you want to continue?'; + return new Promise((resolve) => { + coreEvents.emitConsentRequest({ + prompt: fullPrompt, + onConfirm: (confirmed: boolean) => { + resolve(confirmed); + }, + }); + }); +}