From 25996ae037c5d05a1cee515ae9f1c187986f6c4d Mon Sep 17 00:00:00 2001
From: shishu314 <shishu_1998@yahoo.com>
Date: Fri, 24 Oct 2025 13:52:07 -0400
Subject: [PATCH] fix(security) - Use emitFeedback (#11961)

Co-authored-by: gemini-cli-robot <gemini-cli-robot@google.com>
---
 .../keychain-token-storage.test.ts            | 44 +++++++++++++++++--
 .../token-storage/keychain-token-storage.ts   | 20 ++++++---
 2 files changed, 56 insertions(+), 8 deletions(-)

diff --git a/packages/core/src/mcp/token-storage/keychain-token-storage.test.ts b/packages/core/src/mcp/token-storage/keychain-token-storage.test.ts
index 5b34ed01b5..3b97902f19 100644
--- a/packages/core/src/mcp/token-storage/keychain-token-storage.test.ts
+++ b/packages/core/src/mcp/token-storage/keychain-token-storage.test.ts
@@ -7,6 +7,7 @@
 import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
 import type { KeychainTokenStorage } from './keychain-token-storage.js';
 import type { OAuthCredentials } from './types.js';
+import { coreEvents } from '../../utils/events.js';
 
 // Hoist the mock to be available in the vi.mock factory
 const mockKeytar = vi.hoisted(() => ({
@@ -30,6 +31,12 @@ vi.mock('node:crypto', () => ({
   })),
 }));
 
+vi.mock('../../utils/events.js', () => ({
+  coreEvents: {
+    emitFeedback: vi.fn(),
+  },
+}));
+
 describe('KeychainTokenStorage', () => {
   let storage: KeychainTokenStorage;
 
@@ -82,7 +89,8 @@ describe('KeychainTokenStorage', () => {
     });
 
     it('should return false if keytar fails to set password', async () => {
-      mockKeytar.setPassword.mockRejectedValue(new Error('write error'));
+      const error = new Error('write error');
+      mockKeytar.setPassword.mockRejectedValue(error);
       const isAvailable = await storage.checkKeychainAvailability();
       expect(isAvailable).toBe(false);
     });
@@ -265,14 +273,20 @@ describe('KeychainTokenStorage', () => {
       });
 
       it('should return an empty array on error', async () => {
-        mockKeytar.findCredentials.mockRejectedValue(new Error('find error'));
+        const error = new Error('find error');
+        mockKeytar.findCredentials.mockRejectedValue(error);
         const result = await storage.listServers();
         expect(result).toEqual([]);
+        expect(coreEvents.emitFeedback).toHaveBeenCalledWith(
+          'error',
+          'Failed to list servers from keychain',
+          error,
+        );
       });
     });
 
     describe('getAllCredentials', () => {
-      it('should return a map of all valid credentials', async () => {
+      it('should return a map of all valid credentials and emit feedback for invalid ones', async () => {
         const creds2 = {
           ...validCredentials,
           serverName: 'server2',
@@ -310,6 +324,30 @@ describe('KeychainTokenStorage', () => {
         expect(result.has('expired-server')).toBe(false);
         expect(result.has('bad-server')).toBe(false);
         expect(result.has('invalid-server')).toBe(false);
+
+        expect(coreEvents.emitFeedback).toHaveBeenCalledWith(
+          'error',
+          'Failed to parse credentials for bad-server',
+          expect.any(SyntaxError),
+        );
+        expect(coreEvents.emitFeedback).toHaveBeenCalledWith(
+          'error',
+          'Failed to parse credentials for invalid-server',
+          expect.any(Error),
+        );
+      });
+
+      it('should emit feedback and return empty map if findCredentials fails', async () => {
+        const error = new Error('find all error');
+        mockKeytar.findCredentials.mockRejectedValue(error);
+
+        const result = await storage.getAllCredentials();
+        expect(result.size).toBe(0);
+        expect(coreEvents.emitFeedback).toHaveBeenCalledWith(
+          'error',
+          'Failed to get all credentials from keychain',
+          error,
+        );
       });
     });
 
diff --git a/packages/core/src/mcp/token-storage/keychain-token-storage.ts b/packages/core/src/mcp/token-storage/keychain-token-storage.ts
index 70eccbadf5..aa8cee2e9d 100644
--- a/packages/core/src/mcp/token-storage/keychain-token-storage.ts
+++ b/packages/core/src/mcp/token-storage/keychain-token-storage.ts
@@ -7,6 +7,7 @@
 import * as crypto from 'node:crypto';
 import { BaseTokenStorage } from './base-token-storage.js';
 import type { OAuthCredentials } from './types.js';
+import { coreEvents } from '../../utils/events.js';
 
 interface Keytar {
   getPassword(service: string, account: string): Promise<string | null>;
@@ -42,7 +43,7 @@ export class KeychainTokenStorage extends BaseTokenStorage {
       const module = await import(moduleName);
       this.keytarModule = module.default || module;
     } catch (error) {
-      console.error(error);
+      coreEvents.emitFeedback('error', "Failed to load 'keytar' module", error);
     }
     return this.keytarModule;
   }
@@ -139,7 +140,11 @@ export class KeychainTokenStorage extends BaseTokenStorage {
         .filter((cred) => !cred.account.startsWith(KEYCHAIN_TEST_PREFIX))
         .map((cred: { account: string }) => cred.account);
     } catch (error) {
-      console.error('Failed to list servers from keychain:', error);
+      coreEvents.emitFeedback(
+        'error',
+        'Failed to list servers from keychain',
+        error,
+      );
       return [];
     }
   }
@@ -167,14 +172,19 @@ export class KeychainTokenStorage extends BaseTokenStorage {
             result.set(cred.account, data);
           }
         } catch (error) {
-          console.error(
-            `Failed to parse credentials for ${cred.account}:`,
+          coreEvents.emitFeedback(
+            'error',
+            `Failed to parse credentials for ${cred.account}`,
             error,
           );
         }
       }
     } catch (error) {
-      console.error('Failed to get all credentials from keychain:', error);
+      coreEvents.emitFeedback(
+        'error',
+        'Failed to get all credentials from keychain',
+        error,
+      );
     }
 
     return result;