diff --git a/src/main/claude-accounts/keychain.test.ts b/src/main/claude-accounts/keychain.test.ts index b03c790ba..f0ec1dd40 100644 --- a/src/main/claude-accounts/keychain.test.ts +++ b/src/main/claude-accounts/keychain.test.ts @@ -45,6 +45,7 @@ describe('Claude Keychain credentials', () => { }) afterEach(() => { + vi.useRealTimers() if (originalPlatform) { Object.defineProperty(process, 'platform', originalPlatform) } @@ -175,6 +176,36 @@ describe('Claude Keychain credentials', () => { ]) }) + it('rejects when a keychain read never reports completion', async () => { + vi.useFakeTimers() + const configDir = '/tmp/orca-claude-login-test' + const killMock = vi.fn() + execFileMock.mockImplementationOnce(() => ({ kill: killMock }) as never) + + let settled = false + let rejected: unknown + const readPromise = readActiveClaudeKeychainCredentialsStrict(configDir).then( + (credentials) => { + settled = true + return credentials + }, + (error: unknown) => { + settled = true + rejected = error + return null + } + ) + + await vi.advanceTimersByTimeAsync(3000) + + expect(settled).toBe(true) + await readPromise + expect(rejected).toEqual( + expect.objectContaining({ message: 'security timed out after 3000ms' }) + ) + expect(killMock).toHaveBeenCalled() + }) + it('deletes both scoped and legacy active credentials for config-dir cleanup', async () => { const configDir = '/tmp/orca-claude-login-test' const scopedService = serviceForConfigDir(configDir) diff --git a/src/main/claude-accounts/keychain.ts b/src/main/claude-accounts/keychain.ts index ff9afdc66..910e1786a 100644 --- a/src/main/claude-accounts/keychain.ts +++ b/src/main/claude-accounts/keychain.ts @@ -3,6 +3,12 @@ import { createHash } from 'node:crypto' const ACTIVE_CLAUDE_SERVICE = 'Claude Code-credentials' const ORCA_CLAUDE_SERVICE = 'Orca Claude Code Managed Credentials' +const KEYCHAIN_COMMAND_TIMEOUT_MS = 3_000 + +type SecurityCommandResult = { + stdout: string + stderr: string +} export async function readActiveClaudeKeychainCredentials( configDir?: string @@ -97,30 +103,25 @@ async function readKeychainPassword(service: string, account: string): Promise { - execFile( - 'security', - ['find-generic-password', '-s', service, '-a', account, '-w'], - { timeout: 3_000 }, - (error, stdout, stderr) => { - if (!error && stdout.trim()) { - resolve(stdout.trim()) - return - } - const message = `${stderr} ${error?.message ?? ''}`.toLowerCase() - const code = (error as { code?: unknown } | null)?.code - if ( - code === 44 || - message.includes('could not be found') || - message.includes('not be found') - ) { - resolve(null) - return - } - reject(error ?? new Error(`Could not read macOS Keychain item ${service}/${account}.`)) - } - ) - }) + try { + const { stdout } = await execSecurityCommand([ + 'find-generic-password', + '-s', + service, + '-a', + account, + '-w' + ]) + if (stdout.trim()) { + return stdout.trim() + } + throw new Error(`Could not read macOS Keychain item ${service}/${account}.`) + } catch (error) { + if (isKeychainNotFoundError(error)) { + return null + } + throw error + } } async function writeKeychainPassword( @@ -152,26 +153,81 @@ function execSecurity( args: string[], options?: { ignoreFailure?: boolean; ignoreNotFound?: boolean } ): Promise { - return new Promise((resolve, reject) => { - execFile('security', args, { timeout: 3_000 }, (error, _stdout, stderr) => { - if (!error) { - resolve() - return - } - const code = (error as { code?: unknown }).code - const message = `${stderr} ${error.message}`.toLowerCase() - if ( - options?.ignoreNotFound && - (code === 44 || message.includes('could not be found') || message.includes('not be found')) - ) { - resolve() - return - } - if (!options?.ignoreFailure) { - reject(error) - return - } - resolve() - }) + return execSecurityCommand(args).then(undefined, (error: unknown) => { + if (options?.ignoreNotFound && isKeychainNotFoundError(error)) { + return + } + if (!options?.ignoreFailure) { + throw error + } + }) +} + +function isKeychainNotFoundError(error: unknown): boolean { + const code = + error && typeof error === 'object' && 'code' in error + ? (error as { code?: unknown }).code + : undefined + const message = + error && typeof error === 'object' + ? `${(error as { stderr?: unknown }).stderr ?? ''} ${ + (error as { message?: unknown }).message ?? '' + }`.toLowerCase() + : String(error).toLowerCase() + return code === 44 || message.includes('could not be found') || message.includes('not be found') +} + +function execSecurityCommand(args: string[]): Promise { + return new Promise((resolve, reject) => { + let settled = false + let child: ReturnType | undefined + const timer = setTimeout(() => { + if (settled) { + return + } + settled = true + child?.kill() + reject( + Object.assign(new Error(`security timed out after ${KEYCHAIN_COMMAND_TIMEOUT_MS}ms`), { + code: 'ETIMEDOUT', + stderr: '' + }) + ) + }, KEYCHAIN_COMMAND_TIMEOUT_MS) + + const settle = (callback: () => void): void => { + if (settled) { + return + } + settled = true + clearTimeout(timer) + callback() + } + + // Why: Node's execFile timeout only signals the `security` process; a + // stuck callback would otherwise leave auth/keychain operations pending. + try { + child = execFile( + 'security', + args, + { timeout: KEYCHAIN_COMMAND_TIMEOUT_MS }, + (error, stdout, stderr) => { + if (error) { + settle(() => + reject( + Object.assign(error, { + stdout: String(stdout), + stderr: String(stderr) + }) + ) + ) + return + } + settle(() => resolve({ stdout: String(stdout), stderr: String(stderr) })) + } + ) + } catch (error) { + settle(() => reject(error)) + } }) }