From 6f579934dbe18f4fbe30870ea163d28b040e4de7 Mon Sep 17 00:00:00 2001 From: Emily Hedlund Date: Fri, 6 Mar 2026 13:56:08 -0500 Subject: [PATCH] feat(core): implement unified KeychainService and migrate token storage (#21344) --- packages/core/src/index.ts | 2 + .../keychain-token-storage.test.ts | 525 +++++------------- .../token-storage/keychain-token-storage.ts | 239 ++------ .../core/src/services/keychainService.test.ts | 183 ++++++ packages/core/src/services/keychainService.ts | 147 +++++ packages/core/src/services/keychainTypes.ts | 38 ++ 6 files changed, 553 insertions(+), 581 deletions(-) create mode 100644 packages/core/src/services/keychainService.test.ts create mode 100644 packages/core/src/services/keychainService.ts create mode 100644 packages/core/src/services/keychainTypes.ts diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index c4a9965e41..00b0ef4296 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -123,6 +123,8 @@ export * from './services/sessionSummaryUtils.js'; export * from './services/contextManager.js'; export * from './services/trackerService.js'; export * from './services/trackerTypes.js'; +export * from './services/keychainService.js'; +export * from './services/keychainTypes.js'; export * from './skills/skillManager.js'; export * from './skills/skillLoader.js'; 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 8b402ff7cd..2192abbc45 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 @@ -5,54 +5,41 @@ */ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import type { KeychainTokenStorage } from './keychain-token-storage.js'; +import { KeychainTokenStorage } from './keychain-token-storage.js'; import type { OAuthCredentials } from './types.js'; +import { KeychainService } from '../../services/keychainService.js'; import { coreEvents } from '../../utils/events.js'; - -// Hoist the mock to be available in the vi.mock factory -const mockKeytar = vi.hoisted(() => ({ - getPassword: vi.fn(), - setPassword: vi.fn(), - deletePassword: vi.fn(), - findCredentials: vi.fn(), -})); - -const mockServiceName = 'service-name'; -const mockCryptoRandomBytesString = 'random-string'; - -// Mock the dynamic import of 'keytar' -vi.mock('keytar', () => ({ - default: mockKeytar, -})); - -vi.mock('node:crypto', async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - randomBytes: vi.fn(() => ({ - toString: vi.fn(() => mockCryptoRandomBytesString), - })), - }; -}); - -vi.mock('../../utils/events.js', () => ({ - coreEvents: { - emitFeedback: vi.fn(), - emitTelemetryKeychainAvailability: vi.fn(), - }, -})); +import { KEYCHAIN_TEST_PREFIX } from '../../services/keychainTypes.js'; describe('KeychainTokenStorage', () => { let storage: KeychainTokenStorage; + const mockServiceName = 'service-name'; + let storageState: Map; - beforeEach(async () => { - vi.resetAllMocks(); - // Reset the internal state of the keychain-token-storage module - vi.resetModules(); - const { KeychainTokenStorage } = await import( - './keychain-token-storage.js' - ); + beforeEach(() => { + vi.clearAllMocks(); storage = new KeychainTokenStorage(mockServiceName); + storageState = new Map(); + + // Use stateful spies to verify logic behaviorally + vi.spyOn(KeychainService.prototype, 'getPassword').mockImplementation( + async (account) => storageState.get(account) ?? null, + ); + vi.spyOn(KeychainService.prototype, 'setPassword').mockImplementation( + async (account, value) => { + storageState.set(account, value); + }, + ); + vi.spyOn(KeychainService.prototype, 'deletePassword').mockImplementation( + async (account) => storageState.delete(account), + ); + vi.spyOn(KeychainService.prototype, 'findCredentials').mockImplementation( + async () => + Array.from(storageState.entries()).map(([account, password]) => ({ + account, + password, + })), + ); }); afterEach(() => { @@ -70,375 +57,149 @@ describe('KeychainTokenStorage', () => { updatedAt: Date.now(), } as OAuthCredentials; - describe('checkKeychainAvailability', () => { - it('should return true if keytar is available and functional', async () => { - mockKeytar.setPassword.mockResolvedValue(undefined); - mockKeytar.getPassword.mockResolvedValue('test'); - mockKeytar.deletePassword.mockResolvedValue(true); - - const isAvailable = await storage.checkKeychainAvailability(); - expect(isAvailable).toBe(true); - expect(mockKeytar.setPassword).toHaveBeenCalledWith( - mockServiceName, - `__keychain_test__${mockCryptoRandomBytesString}`, - 'test', - ); - expect(mockKeytar.getPassword).toHaveBeenCalledWith( - mockServiceName, - `__keychain_test__${mockCryptoRandomBytesString}`, - ); - expect(mockKeytar.deletePassword).toHaveBeenCalledWith( - mockServiceName, - `__keychain_test__${mockCryptoRandomBytesString}`, - ); - }); - - it('should return false if keytar fails to set password', async () => { - const error = new Error('write error'); - mockKeytar.setPassword.mockRejectedValue(error); - const isAvailable = await storage.checkKeychainAvailability(); - expect(isAvailable).toBe(false); - }); - - it('should return false if retrieved password does not match', async () => { - mockKeytar.setPassword.mockResolvedValue(undefined); - mockKeytar.getPassword.mockResolvedValue('wrong-password'); - mockKeytar.deletePassword.mockResolvedValue(true); - const isAvailable = await storage.checkKeychainAvailability(); - expect(isAvailable).toBe(false); - }); - - it('should cache the availability result', async () => { - mockKeytar.setPassword.mockResolvedValue(undefined); - mockKeytar.getPassword.mockResolvedValue('test'); - mockKeytar.deletePassword.mockResolvedValue(true); - - await storage.checkKeychainAvailability(); - await storage.checkKeychainAvailability(); - - expect(mockKeytar.setPassword).toHaveBeenCalledTimes(1); - }); - }); - - describe('with keychain unavailable', () => { - beforeEach(async () => { - // Force keychain to be unavailable - mockKeytar.setPassword.mockRejectedValue(new Error('keychain error')); - await storage.checkKeychainAvailability(); - }); - - it('getCredentials should throw', async () => { - await expect(storage.getCredentials('server')).rejects.toThrow( - 'Keychain is not available', - ); - }); - - it('setCredentials should throw', async () => { - await expect(storage.setCredentials(validCredentials)).rejects.toThrow( - 'Keychain is not available', - ); - }); - - it('deleteCredentials should throw', async () => { - await expect(storage.deleteCredentials('server')).rejects.toThrow( - 'Keychain is not available', - ); - }); - - it('listServers should throw', async () => { - await expect(storage.listServers()).rejects.toThrow( - 'Keychain is not available', - ); - }); - - it('getAllCredentials should throw', async () => { - await expect(storage.getAllCredentials()).rejects.toThrow( - 'Keychain is not available', - ); - }); - }); - describe('with keychain available', () => { - beforeEach(async () => { - mockKeytar.setPassword.mockResolvedValue(undefined); - mockKeytar.getPassword.mockResolvedValue('test'); - mockKeytar.deletePassword.mockResolvedValue(true); - await storage.checkKeychainAvailability(); - // Reset mocks after availability check - vi.resetAllMocks(); + beforeEach(() => { + vi.spyOn(KeychainService.prototype, 'isAvailable').mockResolvedValue( + true, + ); }); - describe('getCredentials', () => { - it('should return null if no credentials are found', async () => { - mockKeytar.getPassword.mockResolvedValue(null); - const result = await storage.getCredentials('test-server'); - expect(result).toBeNull(); - expect(mockKeytar.getPassword).toHaveBeenCalledWith( - mockServiceName, - 'test-server', - ); - }); + it('should store and retrieve credentials correctly', async () => { + await storage.setCredentials(validCredentials); + const retrieved = await storage.getCredentials('test-server'); - it('should return credentials if found and not expired', async () => { - mockKeytar.getPassword.mockResolvedValue( - JSON.stringify(validCredentials), - ); - const result = await storage.getCredentials('test-server'); - expect(result).toEqual(validCredentials); - }); - - it('should return null if credentials have expired', async () => { - const expiredCreds = { - ...validCredentials, - token: { ...validCredentials.token, expiresAt: Date.now() - 1000 }, - }; - mockKeytar.getPassword.mockResolvedValue(JSON.stringify(expiredCreds)); - const result = await storage.getCredentials('test-server'); - expect(result).toBeNull(); - }); - - it('should throw if stored data is corrupted JSON', async () => { - mockKeytar.getPassword.mockResolvedValue('not-json'); - await expect(storage.getCredentials('test-server')).rejects.toThrow( - 'Failed to parse stored credentials for test-server', - ); - }); + expect(retrieved?.token.accessToken).toBe('access-token'); + expect(retrieved?.serverName).toBe('test-server'); }); - describe('setCredentials', () => { - it('should save credentials to keychain', async () => { - vi.useFakeTimers(); - mockKeytar.setPassword.mockResolvedValue(undefined); - await storage.setCredentials(validCredentials); - expect(mockKeytar.setPassword).toHaveBeenCalledWith( - mockServiceName, - 'test-server', - JSON.stringify({ ...validCredentials, updatedAt: Date.now() }), - ); - }); + it('should return null if no credentials are found or they are expired', async () => { + expect(await storage.getCredentials('missing')).toBeNull(); - it('should throw if saving to keychain fails', async () => { - mockKeytar.setPassword.mockRejectedValue( - new Error('keychain write error'), - ); - await expect(storage.setCredentials(validCredentials)).rejects.toThrow( - 'keychain write error', - ); - }); + const expiredCreds = { + ...validCredentials, + token: { ...validCredentials.token, expiresAt: Date.now() - 1000 }, + }; + await storage.setCredentials(expiredCreds); + expect(await storage.getCredentials('test-server')).toBeNull(); }); - describe('deleteCredentials', () => { - it('should delete credentials from keychain', async () => { - mockKeytar.deletePassword.mockResolvedValue(true); - await storage.deleteCredentials('test-server'); - expect(mockKeytar.deletePassword).toHaveBeenCalledWith( - mockServiceName, - 'test-server', - ); - }); - - it('should throw if no credentials were found to delete', async () => { - mockKeytar.deletePassword.mockResolvedValue(false); - await expect(storage.deleteCredentials('test-server')).rejects.toThrow( - 'No credentials found for test-server', - ); - }); - - it('should throw if deleting from keychain fails', async () => { - mockKeytar.deletePassword.mockRejectedValue( - new Error('keychain delete error'), - ); - await expect(storage.deleteCredentials('test-server')).rejects.toThrow( - 'keychain delete error', - ); - }); + it('should throw if stored data is corrupted JSON', async () => { + storageState.set('bad-server', 'not-json'); + await expect(storage.getCredentials('bad-server')).rejects.toThrow( + /Failed to parse/, + ); }); - describe('listServers', () => { - it('should return a list of server names', async () => { - mockKeytar.findCredentials.mockResolvedValue([ - { account: 'server1', password: '' }, - { account: 'server2', password: '' }, - ]); - const result = await storage.listServers(); - expect(result).toEqual(['server1', 'server2']); + it('should list servers and filter internal keys', async () => { + await storage.setCredentials(validCredentials); + await storage.setCredentials({ + ...validCredentials, + serverName: 'server2', }); + storageState.set(`${KEYCHAIN_TEST_PREFIX}internal`, '...'); + storageState.set('__secret__key', '...'); - it('should not include internal test keys in the server list', async () => { - mockKeytar.findCredentials.mockResolvedValue([ - { account: 'server1', password: '' }, - { - account: `__keychain_test__${mockCryptoRandomBytesString}`, - password: '', - }, - { account: 'server2', password: '' }, - ]); - const result = await storage.listServers(); - expect(result).toEqual(['server1', 'server2']); - }); - - it('should return an empty array on error', async () => { - 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, - ); - }); + const servers = await storage.listServers(); + expect(servers).toEqual(['test-server', 'server2']); }); - describe('getAllCredentials', () => { - it('should return a map of all valid credentials and emit feedback for invalid ones', async () => { - const creds2 = { - ...validCredentials, - serverName: 'server2', - }; - const expiredCreds = { - ...validCredentials, - serverName: 'expired-server', - token: { ...validCredentials.token, expiresAt: Date.now() - 1000 }, - }; - const structurallyInvalidCreds = { - serverName: 'invalid-server', - }; + it('should handle getAllCredentials with individual parse errors', async () => { + await storage.setCredentials(validCredentials); + storageState.set('bad', 'not-json'); + const emitFeedbackSpy = vi.spyOn(coreEvents, 'emitFeedback'); - mockKeytar.findCredentials.mockResolvedValue([ - { - account: 'test-server', - password: JSON.stringify(validCredentials), - }, - { account: 'server2', password: JSON.stringify(creds2) }, - { - account: 'expired-server', - password: JSON.stringify(expiredCreds), - }, - { account: 'bad-server', password: 'not-json' }, - { - account: 'invalid-server', - password: JSON.stringify(structurallyInvalidCreds), - }, - ]); - - const result = await storage.getAllCredentials(); - expect(result.size).toBe(2); - expect(result.get('test-server')).toEqual(validCredentials); - expect(result.get('server2')).toEqual(creds2); - 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, - ); - }); + const result = await storage.getAllCredentials(); + expect(result.size).toBe(1); + expect(emitFeedbackSpy).toHaveBeenCalled(); }); - describe('clearAll', () => { - it('should delete all credentials for the service', async () => { - mockKeytar.findCredentials.mockResolvedValue([ - { account: 'server1', password: '' }, - { account: 'server2', password: '' }, - ]); - mockKeytar.deletePassword.mockResolvedValue(true); + it('should aggregate errors in clearAll', async () => { + storageState.set('s1', '...'); + storageState.set('s2', '...'); - await storage.clearAll(); + // Aggregating a system error (rejection) + vi.spyOn(KeychainService.prototype, 'deletePassword') + .mockResolvedValueOnce(true) + .mockRejectedValueOnce(new Error('system fail')); - expect(mockKeytar.deletePassword).toHaveBeenCalledTimes(2); - expect(mockKeytar.deletePassword).toHaveBeenCalledWith( - mockServiceName, - 'server1', - ); - expect(mockKeytar.deletePassword).toHaveBeenCalledWith( - mockServiceName, - 'server2', - ); - }); + await expect(storage.clearAll()).rejects.toThrow( + /Failed to clear some credentials: system fail/, + ); - it('should throw an aggregated error if deletions fail', async () => { - mockKeytar.findCredentials.mockResolvedValue([ - { account: 'server1', password: '' }, - { account: 'server2', password: '' }, - ]); - mockKeytar.deletePassword - .mockResolvedValueOnce(true) - .mockRejectedValueOnce(new Error('delete failed')); + // Aggregating a 'not found' error (returns false) + vi.spyOn(KeychainService.prototype, 'deletePassword') + .mockResolvedValueOnce(true) + .mockResolvedValueOnce(false); - await expect(storage.clearAll()).rejects.toThrow( - 'Failed to clear some credentials: delete failed', - ); - }); + await expect(storage.clearAll()).rejects.toThrow( + /Failed to clear some credentials: No credentials found/, + ); }); - describe('Secrets', () => { - it('should set and get a secret', async () => { - mockKeytar.setPassword.mockResolvedValue(undefined); - mockKeytar.getPassword.mockResolvedValue('secret-value'); + it('should manage secrets with prefix independently', async () => { + await storage.setSecret('key1', 'val1'); + await storage.setCredentials(validCredentials); - await storage.setSecret('secret-key', 'secret-value'); - const value = await storage.getSecret('secret-key'); + expect(await storage.getSecret('key1')).toBe('val1'); + expect(await storage.listSecrets()).toEqual(['key1']); + expect(await storage.listServers()).not.toContain('key1'); + }); + }); - expect(mockKeytar.setPassword).toHaveBeenCalledWith( - mockServiceName, - '__secret__secret-key', - 'secret-value', - ); - expect(mockKeytar.getPassword).toHaveBeenCalledWith( - mockServiceName, - '__secret__secret-key', - ); - expect(value).toBe('secret-value'); - }); + describe('unavailability handling', () => { + beforeEach(() => { + vi.spyOn(KeychainService.prototype, 'isAvailable').mockResolvedValue( + false, + ); + vi.spyOn(KeychainService.prototype, 'getPassword').mockRejectedValue( + new Error('Keychain is not available'), + ); + vi.spyOn(KeychainService.prototype, 'setPassword').mockRejectedValue( + new Error('Keychain is not available'), + ); + vi.spyOn(KeychainService.prototype, 'deletePassword').mockRejectedValue( + new Error('Keychain is not available'), + ); + vi.spyOn(KeychainService.prototype, 'findCredentials').mockRejectedValue( + new Error('Keychain is not available'), + ); + }); - it('should delete a secret', async () => { - mockKeytar.deletePassword.mockResolvedValue(true); - await storage.deleteSecret('secret-key'); - expect(mockKeytar.deletePassword).toHaveBeenCalledWith( - mockServiceName, - '__secret__secret-key', - ); - }); + it.each([ + { method: 'getCredentials', args: ['s'] }, + { method: 'setCredentials', args: [validCredentials] }, + { method: 'deleteCredentials', args: ['s'] }, + { method: 'clearAll', args: [] }, + ])( + '$method should propagate unavailability error', + async ({ method, args }) => { + await expect( + ( + storage as unknown as Record< + string, + (...args: unknown[]) => Promise + > + )[method](...args), + ).rejects.toThrow('Keychain is not available'); + }, + ); - it('should list secrets', async () => { - mockKeytar.findCredentials.mockResolvedValue([ - { account: '__secret__secret1', password: '' }, - { account: '__secret__secret2', password: '' }, - { account: 'server1', password: '' }, - ]); - const secrets = await storage.listSecrets(); - expect(secrets).toEqual(['secret1', 'secret2']); - }); - - it('should not list secrets in listServers', async () => { - mockKeytar.findCredentials.mockResolvedValue([ - { account: '__secret__secret1', password: '' }, - { account: 'server1', password: '' }, - ]); - const servers = await storage.listServers(); - expect(servers).toEqual(['server1']); - }); + it.each([ + { method: 'listServers' }, + { method: 'getAllCredentials' }, + { method: 'listSecrets' }, + ])('$method should emit feedback and return empty', async ({ method }) => { + const emitFeedbackSpy = vi.spyOn(coreEvents, 'emitFeedback'); + expect( + await (storage as unknown as Record Promise>)[ + method + ](), + ).toEqual(method === 'getAllCredentials' ? new Map() : []); + expect(emitFeedbackSpy).toHaveBeenCalledWith( + 'error', + expect.any(String), + expect.any(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 4be0d082e5..d0b4990279 100644 --- a/packages/core/src/mcp/token-storage/keychain-token-storage.ts +++ b/packages/core/src/mcp/token-storage/keychain-token-storage.ts @@ -4,70 +4,30 @@ * SPDX-License-Identifier: Apache-2.0 */ -import * as crypto from 'node:crypto'; import { BaseTokenStorage } from './base-token-storage.js'; import type { OAuthCredentials, SecretStorage } from './types.js'; import { coreEvents } from '../../utils/events.js'; -import { KeychainAvailabilityEvent } from '../../telemetry/types.js'; - -interface Keytar { - getPassword(service: string, account: string): Promise; - setPassword( - service: string, - account: string, - password: string, - ): Promise; - deletePassword(service: string, account: string): Promise; - findCredentials( - service: string, - ): Promise>; -} - -const KEYCHAIN_TEST_PREFIX = '__keychain_test__'; -const SECRET_PREFIX = '__secret__'; +import { KeychainService } from '../../services/keychainService.js'; +import { + KEYCHAIN_TEST_PREFIX, + SECRET_PREFIX, +} from '../../services/keychainTypes.js'; export class KeychainTokenStorage extends BaseTokenStorage implements SecretStorage { - private keychainAvailable: boolean | null = null; - private keytarModule: Keytar | null = null; - private keytarLoadAttempted = false; + private readonly keychainService: KeychainService; - async getKeytar(): Promise { - // If we've already tried loading (successfully or not), return the result - if (this.keytarLoadAttempted) { - return this.keytarModule; - } - - this.keytarLoadAttempted = true; - - try { - // Try to import keytar without any timeout - let the OS handle it - const moduleName = 'keytar'; - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - const module = await import(moduleName); - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - this.keytarModule = module.default || module; - } catch (_) { - //Keytar is optional so we shouldn't raise an error of log anything. - } - return this.keytarModule; + constructor(serviceName: string) { + super(serviceName); + this.keychainService = new KeychainService(serviceName); } async getCredentials(serverName: string): Promise { - if (!(await this.checkKeychainAvailability())) { - throw new Error('Keychain is not available'); - } - - const keytar = await this.getKeytar(); - if (!keytar) { - throw new Error('Keytar module not available'); - } - try { const sanitizedName = this.sanitizeServerName(serverName); - const data = await keytar.getPassword(this.serviceName, sanitizedName); + const data = await this.keychainService.getPassword(sanitizedName); if (!data) { return null; @@ -90,15 +50,6 @@ export class KeychainTokenStorage } async setCredentials(credentials: OAuthCredentials): Promise { - if (!(await this.checkKeychainAvailability())) { - throw new Error('Keychain is not available'); - } - - const keytar = await this.getKeytar(); - if (!keytar) { - throw new Error('Keytar module not available'); - } - this.validateCredentials(credentials); const sanitizedName = this.sanitizeServerName(credentials.serverName); @@ -108,24 +59,12 @@ export class KeychainTokenStorage }; const data = JSON.stringify(updatedCredentials); - await keytar.setPassword(this.serviceName, sanitizedName, data); + await this.keychainService.setPassword(sanitizedName, data); } async deleteCredentials(serverName: string): Promise { - if (!(await this.checkKeychainAvailability())) { - throw new Error('Keychain is not available'); - } - - const keytar = await this.getKeytar(); - if (!keytar) { - throw new Error('Keytar module not available'); - } - const sanitizedName = this.sanitizeServerName(serverName); - const deleted = await keytar.deletePassword( - this.serviceName, - sanitizedName, - ); + const deleted = await this.keychainService.deletePassword(sanitizedName); if (!deleted) { throw new Error(`No credentials found for ${serverName}`); @@ -133,17 +72,8 @@ export class KeychainTokenStorage } async listServers(): Promise { - if (!(await this.checkKeychainAvailability())) { - throw new Error('Keychain is not available'); - } - - const keytar = await this.getKeytar(); - if (!keytar) { - throw new Error('Keytar module not available'); - } - try { - const credentials = await keytar.findCredentials(this.serviceName); + const credentials = await this.keychainService.findCredentials(); return credentials .filter( (cred) => @@ -162,20 +92,9 @@ export class KeychainTokenStorage } async getAllCredentials(): Promise> { - if (!(await this.checkKeychainAvailability())) { - throw new Error('Keychain is not available'); - } - - const keytar = await this.getKeytar(); - if (!keytar) { - throw new Error('Keytar module not available'); - } - const result = new Map(); try { - const credentials = ( - await keytar.findCredentials(this.serviceName) - ).filter( + const credentials = (await this.keychainService.findCredentials()).filter( (c) => !c.account.startsWith(KEYCHAIN_TEST_PREFIX) && !c.account.startsWith(SECRET_PREFIX), @@ -208,119 +127,48 @@ export class KeychainTokenStorage } async clearAll(): Promise { - if (!(await this.checkKeychainAvailability())) { - throw new Error('Keychain is not available'); - } - - const servers = this.keytarModule - ? await this.keytarModule - .findCredentials(this.serviceName) - .then((creds) => creds.map((c) => c.account)) - .catch((error: Error) => { - throw new Error( - `Failed to list servers for clearing: ${error.message}`, - ); - }) - : []; - const errors: Error[] = []; - - for (const server of servers) { - try { - await this.deleteCredentials(server); - } catch (error) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - errors.push(error as Error); - } - } - - if (errors.length > 0) { - throw new Error( - `Failed to clear some credentials: ${errors.map((e) => e.message).join(', ')}`, - ); - } - } - - // Checks whether or not a set-get-delete cycle with the keychain works. - // Returns false if any operation fails. - async checkKeychainAvailability(): Promise { - if (this.keychainAvailable !== null) { - return this.keychainAvailable; - } - try { - const keytar = await this.getKeytar(); - if (!keytar) { - this.keychainAvailable = false; - return false; + const credentials = await this.keychainService.findCredentials(); + const errors: Error[] = []; + + for (const cred of credentials) { + try { + await this.deleteCredentials(cred.account); + } catch (error) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + errors.push(error as Error); + } } - const testAccount = `${KEYCHAIN_TEST_PREFIX}${crypto.randomBytes(8).toString('hex')}`; - const testPassword = 'test'; - - await keytar.setPassword(this.serviceName, testAccount, testPassword); - const retrieved = await keytar.getPassword(this.serviceName, testAccount); - const deleted = await keytar.deletePassword( - this.serviceName, - testAccount, + if (errors.length > 0) { + throw new Error( + `Failed to clear some credentials: ${errors.map((e) => e.message).join(', ')}`, + ); + } + } catch (error) { + coreEvents.emitFeedback( + 'error', + 'Failed to clear credentials from keychain', + error, ); - - const success = deleted && retrieved === testPassword; - this.keychainAvailable = success; - - coreEvents.emitTelemetryKeychainAvailability( - new KeychainAvailabilityEvent(success), - ); - - return success; - } catch (_error) { - this.keychainAvailable = false; - - // Do not log the raw error message to avoid potential PII leaks - // (e.g. from OS-level error messages containing file paths) - coreEvents.emitTelemetryKeychainAvailability( - new KeychainAvailabilityEvent(false), - ); - - return false; + throw error; } } async isAvailable(): Promise { - return this.checkKeychainAvailability(); + return this.keychainService.isAvailable(); } async setSecret(key: string, value: string): Promise { - if (!(await this.checkKeychainAvailability())) { - throw new Error('Keychain is not available'); - } - const keytar = await this.getKeytar(); - if (!keytar) { - throw new Error('Keytar module not available'); - } - await keytar.setPassword(this.serviceName, `${SECRET_PREFIX}${key}`, value); + await this.keychainService.setPassword(`${SECRET_PREFIX}${key}`, value); } async getSecret(key: string): Promise { - if (!(await this.checkKeychainAvailability())) { - throw new Error('Keychain is not available'); - } - const keytar = await this.getKeytar(); - if (!keytar) { - throw new Error('Keytar module not available'); - } - return keytar.getPassword(this.serviceName, `${SECRET_PREFIX}${key}`); + return this.keychainService.getPassword(`${SECRET_PREFIX}${key}`); } async deleteSecret(key: string): Promise { - if (!(await this.checkKeychainAvailability())) { - throw new Error('Keychain is not available'); - } - const keytar = await this.getKeytar(); - if (!keytar) { - throw new Error('Keytar module not available'); - } - const deleted = await keytar.deletePassword( - this.serviceName, + const deleted = await this.keychainService.deletePassword( `${SECRET_PREFIX}${key}`, ); if (!deleted) { @@ -329,15 +177,8 @@ export class KeychainTokenStorage } async listSecrets(): Promise { - if (!(await this.checkKeychainAvailability())) { - throw new Error('Keychain is not available'); - } - const keytar = await this.getKeytar(); - if (!keytar) { - throw new Error('Keytar module not available'); - } try { - const credentials = await keytar.findCredentials(this.serviceName); + const credentials = await this.keychainService.findCredentials(); return credentials .filter((cred) => cred.account.startsWith(SECRET_PREFIX)) .map((cred) => cred.account.substring(SECRET_PREFIX.length)); diff --git a/packages/core/src/services/keychainService.test.ts b/packages/core/src/services/keychainService.test.ts new file mode 100644 index 0000000000..4ab59a5369 --- /dev/null +++ b/packages/core/src/services/keychainService.test.ts @@ -0,0 +1,183 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest'; +import { KeychainService } from './keychainService.js'; +import { coreEvents } from '../utils/events.js'; +import { debugLogger } from '../utils/debugLogger.js'; + +type MockKeychain = { + getPassword: Mock | undefined; + setPassword: Mock | undefined; + deletePassword: Mock | undefined; + findCredentials: Mock | undefined; +}; + +const mockKeytar: MockKeychain = { + getPassword: vi.fn(), + setPassword: vi.fn(), + deletePassword: vi.fn(), + findCredentials: vi.fn(), +}; + +vi.mock('keytar', () => ({ default: mockKeytar })); + +vi.mock('../utils/events.js', () => ({ + coreEvents: { emitTelemetryKeychainAvailability: vi.fn() }, +})); + +vi.mock('../utils/debugLogger.js', () => ({ + debugLogger: { log: vi.fn() }, +})); + +describe('KeychainService', () => { + let service: KeychainService; + const SERVICE_NAME = 'test-service'; + let passwords: Record = {}; + + beforeEach(() => { + vi.clearAllMocks(); + service = new KeychainService(SERVICE_NAME); + passwords = {}; + + // Stateful mock implementation to verify behavioral correctness + mockKeytar.setPassword?.mockImplementation((_svc, acc, val) => { + passwords[acc] = val; + return Promise.resolve(); + }); + mockKeytar.getPassword?.mockImplementation((_svc, acc) => + Promise.resolve(passwords[acc] ?? null), + ); + mockKeytar.deletePassword?.mockImplementation((_svc, acc) => { + const exists = !!passwords[acc]; + delete passwords[acc]; + return Promise.resolve(exists); + }); + mockKeytar.findCredentials?.mockImplementation(() => + Promise.resolve( + Object.entries(passwords).map(([account, password]) => ({ + account, + password, + })), + ), + ); + }); + + describe('isAvailable', () => { + it('should return true and emit telemetry on successful functional test', async () => { + const available = await service.isAvailable(); + + expect(available).toBe(true); + expect(mockKeytar.setPassword).toHaveBeenCalled(); + expect(coreEvents.emitTelemetryKeychainAvailability).toHaveBeenCalledWith( + expect.objectContaining({ available: true }), + ); + }); + + it('should return false, log error, and emit telemetry on failed functional test', async () => { + mockKeytar.setPassword?.mockRejectedValue(new Error('locked')); + + const available = await service.isAvailable(); + + expect(available).toBe(false); + expect(debugLogger.log).toHaveBeenCalledWith( + expect.stringContaining('encountered an error'), + 'locked', + ); + expect(coreEvents.emitTelemetryKeychainAvailability).toHaveBeenCalledWith( + expect.objectContaining({ available: false }), + ); + }); + + it('should return false, log validation error, and emit telemetry on module load failure', async () => { + const originalMock = mockKeytar.getPassword; + mockKeytar.getPassword = undefined; // Break schema + + const available = await service.isAvailable(); + + expect(available).toBe(false); + expect(debugLogger.log).toHaveBeenCalledWith( + expect.stringContaining('failed structural validation'), + expect.objectContaining({ getPassword: expect.any(Array) }), + ); + expect(coreEvents.emitTelemetryKeychainAvailability).toHaveBeenCalledWith( + expect.objectContaining({ available: false }), + ); + + mockKeytar.getPassword = originalMock; + }); + + it('should log failure if functional test cycle returns false', async () => { + mockKeytar.getPassword?.mockResolvedValue('wrong-password'); + + const available = await service.isAvailable(); + + expect(available).toBe(false); + expect(debugLogger.log).toHaveBeenCalledWith( + expect.stringContaining('functional verification failed'), + ); + }); + + it('should cache the result and handle concurrent initialization attempts once', async () => { + await Promise.all([ + service.isAvailable(), + service.isAvailable(), + service.isAvailable(), + ]); + + expect(mockKeytar.setPassword).toHaveBeenCalledTimes(1); + }); + }); + + describe('Password Operations', () => { + beforeEach(async () => { + await service.isAvailable(); + vi.clearAllMocks(); + }); + + it('should store, retrieve, and delete passwords correctly', async () => { + await service.setPassword('acc1', 'secret1'); + await service.setPassword('acc2', 'secret2'); + + expect(await service.getPassword('acc1')).toBe('secret1'); + expect(await service.getPassword('acc2')).toBe('secret2'); + + const creds = await service.findCredentials(); + expect(creds).toHaveLength(2); + expect(creds).toContainEqual({ account: 'acc1', password: 'secret1' }); + + expect(await service.deletePassword('acc1')).toBe(true); + expect(await service.getPassword('acc1')).toBeNull(); + expect(await service.findCredentials()).toHaveLength(1); + }); + + it('getPassword should return null if key is missing', async () => { + expect(await service.getPassword('missing')).toBeNull(); + }); + }); + + describe('When Unavailable', () => { + beforeEach(() => { + mockKeytar.setPassword?.mockRejectedValue(new Error('Unavailable')); + }); + + it.each([ + { method: 'getPassword', args: ['acc'] }, + { method: 'setPassword', args: ['acc', 'val'] }, + { method: 'deletePassword', args: ['acc'] }, + { method: 'findCredentials', args: [] }, + ])('$method should throw a consistent error', async ({ method, args }) => { + await expect( + ( + service as unknown as Record< + string, + (...args: unknown[]) => Promise + > + )[method](...args), + ).rejects.toThrow('Keychain is not available'); + }); + }); +}); diff --git a/packages/core/src/services/keychainService.ts b/packages/core/src/services/keychainService.ts new file mode 100644 index 0000000000..ed28218c11 --- /dev/null +++ b/packages/core/src/services/keychainService.ts @@ -0,0 +1,147 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import * as crypto from 'node:crypto'; +import { coreEvents } from '../utils/events.js'; +import { KeychainAvailabilityEvent } from '../telemetry/types.js'; +import { debugLogger } from '../utils/debugLogger.js'; +import { + type Keychain, + KeychainSchema, + KEYCHAIN_TEST_PREFIX, +} from './keychainTypes.js'; + +/** + * Service for interacting with OS-level secure storage (e.g. keytar). + */ +export class KeychainService { + // Track an ongoing initialization attempt to avoid race conditions. + private initializationPromise?: Promise; + + /** + * @param serviceName Unique identifier for the app in the OS keychain. + */ + constructor(private readonly serviceName: string) {} + + async isAvailable(): Promise { + return (await this.getKeychain()) !== null; + } + + /** + * Retrieves a secret for the given account. + * @throws Error if the keychain is unavailable. + */ + async getPassword(account: string): Promise { + const keychain = await this.getKeychainOrThrow(); + return keychain.getPassword(this.serviceName, account); + } + + /** + * Securely stores a secret. + * @throws Error if the keychain is unavailable. + */ + async setPassword(account: string, value: string): Promise { + const keychain = await this.getKeychainOrThrow(); + await keychain.setPassword(this.serviceName, account, value); + } + + /** + * Removes a secret from the keychain. + * @returns true if the secret was deleted, false otherwise. + * @throws Error if the keychain is unavailable. + */ + async deletePassword(account: string): Promise { + const keychain = await this.getKeychainOrThrow(); + return keychain.deletePassword(this.serviceName, account); + } + + /** + * Lists all account/secret pairs stored under this service. + * @throws Error if the keychain is unavailable. + */ + async findCredentials(): Promise< + Array<{ account: string; password: string }> + > { + const keychain = await this.getKeychainOrThrow(); + return keychain.findCredentials(this.serviceName); + } + + private async getKeychainOrThrow(): Promise { + const keychain = await this.getKeychain(); + if (!keychain) { + throw new Error('Keychain is not available'); + } + return keychain; + } + + private getKeychain(): Promise { + return (this.initializationPromise ??= this.initializeKeychain()); + } + + // High-level orchestration of the loading and testing cycle. + private async initializeKeychain(): Promise { + let resultKeychain: Keychain | null = null; + + try { + const keychainModule = await this.loadKeychainModule(); + if (keychainModule) { + if (await this.isKeychainFunctional(keychainModule)) { + resultKeychain = keychainModule; + } else { + debugLogger.log('Keychain functional verification failed'); + } + } + } catch (error) { + // Avoid logging full error objects to prevent PII exposure. + const message = error instanceof Error ? error.message : String(error); + debugLogger.log('Keychain initialization encountered an error:', message); + } + + coreEvents.emitTelemetryKeychainAvailability( + new KeychainAvailabilityEvent(resultKeychain !== null), + ); + + return resultKeychain; + } + + // Low-level dynamic loading and structural validation. + private async loadKeychainModule(): Promise { + const moduleName = 'keytar'; + const module: unknown = await import(moduleName); + const potential = (this.isRecord(module) && module['default']) || module; + + const result = KeychainSchema.safeParse(potential); + if (result.success) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + return potential as Keychain; + } + + debugLogger.log( + 'Keychain module failed structural validation:', + result.error.flatten().fieldErrors, + ); + return null; + } + + private isRecord(obj: unknown): obj is Record { + return typeof obj === 'object' && obj !== null; + } + + // Performs a set-get-delete cycle to verify keychain functionality. + private async isKeychainFunctional(keychain: Keychain): Promise { + const testAccount = `${KEYCHAIN_TEST_PREFIX}${crypto.randomBytes(8).toString('hex')}`; + const testPassword = 'test'; + + await keychain.setPassword(this.serviceName, testAccount, testPassword); + const retrieved = await keychain.getPassword(this.serviceName, testAccount); + const deleted = await keychain.deletePassword( + this.serviceName, + testAccount, + ); + + return deleted && retrieved === testPassword; + } +} diff --git a/packages/core/src/services/keychainTypes.ts b/packages/core/src/services/keychainTypes.ts new file mode 100644 index 0000000000..a643f763e4 --- /dev/null +++ b/packages/core/src/services/keychainTypes.ts @@ -0,0 +1,38 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { z } from 'zod'; + +/** + * Interface for OS-level secure storage operations. + * Note: Method names must match the underlying library (e.g. keytar) + * to support correct dynamic loading and schema validation. + */ +export interface Keychain { + getPassword(service: string, account: string): Promise; + setPassword( + service: string, + account: string, + password: string, + ): Promise; + deletePassword(service: string, account: string): Promise; + findCredentials( + service: string, + ): Promise>; +} + +/** + * Zod schema to validate that a module satisfies the Keychain interface. + */ +export const KeychainSchema = z.object({ + getPassword: z.function(), + setPassword: z.function(), + deletePassword: z.function(), + findCredentials: z.function(), +}); + +export const KEYCHAIN_TEST_PREFIX = '__keychain_test__'; +export const SECRET_PREFIX = '__secret__';