From c2104a14fbd0de383a2ecd2e70889252bef36c33 Mon Sep 17 00:00:00 2001 From: shishu314 Date: Fri, 24 Oct 2025 14:07:11 -0400 Subject: [PATCH] fix(security) - Use emitFeedback instead of console error (#11948) Co-authored-by: gemini-cli-robot --- .../core/src/mcp/oauth-token-storage.test.ts | 47 +++++++++++++------ packages/core/src/mcp/oauth-token-storage.ts | 17 +++++-- 2 files changed, 45 insertions(+), 19 deletions(-) diff --git a/packages/core/src/mcp/oauth-token-storage.test.ts b/packages/core/src/mcp/oauth-token-storage.test.ts index cd8841aaee..16abf5a6ad 100644 --- a/packages/core/src/mcp/oauth-token-storage.test.ts +++ b/packages/core/src/mcp/oauth-token-storage.test.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { coreEvents } from '@google/gemini-cli-core'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { promises as fs } from 'node:fs'; import * as path from 'node:path'; @@ -33,6 +34,12 @@ vi.mock('../config/storage.js', () => ({ }, })); +vi.mock('@google/gemini-cli-core', () => ({ + coreEvents: { + emitFeedback: vi.fn(), + }, +})); + const mockHybridTokenStorage = { listServers: vi.fn(), setCredentials: vi.fn(), @@ -72,7 +79,6 @@ describe('MCPOAuthTokenStorage', () => { tokenStorage = new MCPOAuthTokenStorage(); vi.clearAllMocks(); - vi.spyOn(console, 'error'); }); afterEach(() => { @@ -87,7 +93,7 @@ describe('MCPOAuthTokenStorage', () => { const tokens = await tokenStorage.getAllCredentials(); expect(tokens.size).toBe(0); - expect(console.error).not.toHaveBeenCalled(); + expect(coreEvents.emitFeedback).not.toHaveBeenCalled(); }); it('should load tokens from file successfully', async () => { @@ -110,8 +116,10 @@ describe('MCPOAuthTokenStorage', () => { const tokens = await tokenStorage.getAllCredentials(); expect(tokens.size).toBe(0); - expect(console.error).toHaveBeenCalledWith( + expect(coreEvents.emitFeedback).toHaveBeenCalledWith( + 'error', expect.stringContaining('Failed to load MCP OAuth tokens'), + expect.any(Error), ); }); @@ -122,8 +130,10 @@ describe('MCPOAuthTokenStorage', () => { const tokens = await tokenStorage.getAllCredentials(); expect(tokens.size).toBe(0); - expect(console.error).toHaveBeenCalledWith( - expect.stringContaining('Failed to load MCP OAuth tokens'), + expect(coreEvents.emitFeedback).toHaveBeenCalledWith( + 'error', + 'Failed to load MCP OAuth tokens: Permission denied', + error, ); }); }); @@ -188,8 +198,10 @@ describe('MCPOAuthTokenStorage', () => { tokenStorage.saveToken('test-server', mockToken), ).rejects.toThrow('Disk full'); - expect(console.error).toHaveBeenCalledWith( - expect.stringContaining('Failed to save MCP OAuth token'), + expect(coreEvents.emitFeedback).toHaveBeenCalledWith( + 'error', + 'Failed to save MCP OAuth token: Disk full', + writeError, ); }); }); @@ -277,12 +289,15 @@ describe('MCPOAuthTokenStorage', () => { vi.mocked(fs.readFile).mockResolvedValue( JSON.stringify([mockCredentials]), ); - vi.mocked(fs.unlink).mockRejectedValue(new Error('Permission denied')); + const unlinkError = new Error('Permission denied'); + vi.mocked(fs.unlink).mockRejectedValue(unlinkError); await tokenStorage.deleteCredentials('test-server'); - expect(console.error).toHaveBeenCalledWith( - expect.stringContaining('Failed to remove MCP OAuth token'), + expect(coreEvents.emitFeedback).toHaveBeenCalledWith( + 'error', + 'Failed to remove MCP OAuth token: Permission denied', + unlinkError, ); }); }); @@ -347,16 +362,19 @@ describe('MCPOAuthTokenStorage', () => { await tokenStorage.clearAll(); - expect(console.error).not.toHaveBeenCalled(); + expect(coreEvents.emitFeedback).not.toHaveBeenCalled(); }); it('should handle other file errors gracefully', async () => { - vi.mocked(fs.unlink).mockRejectedValue(new Error('Permission denied')); + const unlinkError = new Error('Permission denied'); + vi.mocked(fs.unlink).mockRejectedValue(unlinkError); await tokenStorage.clearAll(); - expect(console.error).toHaveBeenCalledWith( - expect.stringContaining('Failed to clear MCP OAuth tokens'), + expect(coreEvents.emitFeedback).toHaveBeenCalledWith( + 'error', + 'Failed to clear MCP OAuth tokens: Permission denied', + unlinkError, ); }); }); @@ -368,7 +386,6 @@ describe('MCPOAuthTokenStorage', () => { tokenStorage = new MCPOAuthTokenStorage(); vi.clearAllMocks(); - vi.spyOn(console, 'error'); }); afterEach(() => { diff --git a/packages/core/src/mcp/oauth-token-storage.ts b/packages/core/src/mcp/oauth-token-storage.ts index d9d98ff417..66ccba29b6 100644 --- a/packages/core/src/mcp/oauth-token-storage.ts +++ b/packages/core/src/mcp/oauth-token-storage.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { coreEvents } from '@google/gemini-cli-core'; import { promises as fs } from 'node:fs'; import * as path from 'node:path'; import { Storage } from '../config/storage.js'; @@ -68,8 +69,10 @@ export class MCPOAuthTokenStorage implements TokenStorage { } catch (error) { // File doesn't exist or is invalid, return empty map if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { - console.error( + coreEvents.emitFeedback( + 'error', `Failed to load MCP OAuth tokens: ${getErrorMessage(error)}`, + error, ); } } @@ -102,8 +105,10 @@ export class MCPOAuthTokenStorage implements TokenStorage { { mode: 0o600 }, // Restrict file permissions ); } catch (error) { - console.error( + coreEvents.emitFeedback( + 'error', `Failed to save MCP OAuth token: ${getErrorMessage(error)}`, + error, ); throw error; } @@ -181,8 +186,10 @@ export class MCPOAuthTokenStorage implements TokenStorage { }); } } catch (error) { - console.error( + coreEvents.emitFeedback( + 'error', `Failed to remove MCP OAuth token: ${getErrorMessage(error)}`, + error, ); } } @@ -216,8 +223,10 @@ export class MCPOAuthTokenStorage implements TokenStorage { await fs.unlink(tokenFile); } catch (error) { if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { - console.error( + coreEvents.emitFeedback( + 'error', `Failed to clear MCP OAuth tokens: ${getErrorMessage(error)}`, + error, ); } }