diff --git a/src/lib/config.ts b/src/lib/config.ts index 86abc08e..42f5141d 100644 --- a/src/lib/config.ts +++ b/src/lib/config.ts @@ -339,27 +339,9 @@ export async function resolveApiKeyAsync( // Fall through: profile may not be migrated yet (api_key still in file) } - // File-based storage (or unmigrated profile in mixed state) if (creds) { const entry = creds.profiles[profile]; if (entry?.api_key) { - // Auto-migrate: move plaintext key to secure storage if available - const backend = await getCredentialBackend(); - if (backend.isSecure) { - try { - await backend.set(SERVICE_NAME, profile, entry.api_key); - creds.profiles[profile] = { - ...(entry.permission && { permission: entry.permission }), - }; - creds.storage = 'secure_storage'; - writeCredentials(creds); - process.stderr.write( - `Notice: API key for profile "${profile}" has been moved to ${backend.name}\n`, - ); - } catch { - // Non-fatal — plaintext key still works - } - } return { key: entry.api_key, source: 'config', diff --git a/src/lib/credential-backends/macos.ts b/src/lib/credential-backends/macos.ts index d88d08dc..b3578d17 100644 --- a/src/lib/credential-backends/macos.ts +++ b/src/lib/credential-backends/macos.ts @@ -1,18 +1,71 @@ -import { execFile } from 'node:child_process'; +import { execFile, spawn } from 'node:child_process'; import type { CredentialBackend } from '../credential-store'; function run( cmd: string, - args: string[], + args: readonly string[], ): Promise<{ stdout: string; stderr: string; code: number | null }> { return new Promise((resolve) => { - execFile(cmd, args, { timeout: 5000 }, (err, stdout, stderr) => { + execFile(cmd, [...args], { timeout: 5000 }, (err, stdout, stderr) => { const code = err && 'code' in err ? (err.code as number | null) : 0; resolve({ stdout: stdout ?? '', stderr: stderr ?? '', code }); }); }); } +function runWithStdin( + cmd: string, + args: readonly string[], + stdinData: string, +): Promise<{ stdout: string; stderr: string; code: number | null }> { + return new Promise((resolve) => { + const child = spawn(cmd, [...args], { + stdio: ['pipe', 'pipe', 'pipe'], + timeout: 5000, + }); + let stdout = ''; + let stderr = ''; + child.stdout?.on('data', (data: Buffer) => { + stdout += data.toString(); + }); + child.stderr?.on('data', (data: Buffer) => { + stderr += data.toString(); + }); + child.on('close', (code) => { + resolve({ stdout, stderr, code }); + }); + child.on('error', () => { + resolve({ stdout: '', stderr: 'Failed to spawn process', code: 1 }); + }); + child.stdin?.on('error', () => {}); + child.stdin?.write(stdinData); + child.stdin?.end(); + }); +} + +function buildKeychainSetScript( + service: string, + account: string, + secret: string, +): string { + return [ + 'ObjC.import("Security");', + `var passwordData = $(${JSON.stringify(secret)}).dataUsingEncoding($.NSUTF8StringEncoding);`, + 'var query = $.NSMutableDictionary.alloc.init;', + 'query.setObjectForKey($.kSecClassGenericPassword, $.kSecClass);', + `query.setObjectForKey($(${JSON.stringify(service)}), $.kSecAttrService);`, + `query.setObjectForKey($(${JSON.stringify(account)}), $.kSecAttrAccount);`, + 'var updateAttrs = $.NSMutableDictionary.alloc.init;', + 'updateAttrs.setObjectForKey(passwordData, $.kSecValueData);', + 'var status = $.SecItemUpdate(query, updateAttrs);', + 'if (status === -25300) {', + ' query.setObjectForKey(passwordData, $.kSecValueData);', + ' status = $.SecItemAdd(query, null);', + '}', + 'if (status !== 0) { throw new Error("Keychain error: " + status); }', + ].join('\n'); +} + export class MacOSBackend implements CredentialBackend { name = 'macOS Keychain'; readonly isSecure = true; @@ -38,24 +91,12 @@ export class MacOSBackend implements CredentialBackend { } async set(service: string, account: string, secret: string): Promise { - // Note: The macOS `security` command does not support reading passwords from - // stdin — `-w` without a value triggers an interactive TTY prompt, and `-X` - // (hex) is still a CLI arg visible in `ps`. There is no safe way to pass the - // secret without it appearing briefly in the process list during the execFile - // call (~5s timeout). This is the same approach used by tools like `gh` - // (GitHub CLI) and `1password-cli` when interacting with the macOS keychain - // via the `security` command. - // -U updates if exists, creates if not - const { code, stderr } = await run('/usr/bin/security', [ - 'add-generic-password', - '-s', - service, - '-a', - account, - '-w', - secret, - '-U', - ]); + const script = buildKeychainSetScript(service, account, secret); + const { code, stderr } = await runWithStdin( + '/usr/bin/osascript', + ['-l', 'JavaScript'], + script, + ); if (code !== 0) { throw new Error( `Failed to store credential in macOS Keychain: ${stderr.trim()}`, diff --git a/tests/lib/config-async.test.ts b/tests/lib/config-async.test.ts index a5d7c364..4305f0ab 100644 --- a/tests/lib/config-async.test.ts +++ b/tests/lib/config-async.test.ts @@ -133,6 +133,7 @@ describe('resolveApiKeyAsync', () => { source: 'config', profile: 'default', }); + expect(mockBackend.set).not.toHaveBeenCalled(); }); test('returns null when keychain has no entry', async () => { @@ -167,6 +168,43 @@ describe('resolveApiKeyAsync', () => { const result = await resolveApiKeyAsync(); expect(result).toBeNull(); }); + + test('does not auto-migrate plaintext keys to secure storage on read', async () => { + const configDir = join(tmpDir, 'resend'); + mkdirSync(configDir, { recursive: true }); + writeFileSync( + join(configDir, 'credentials.json'), + JSON.stringify({ + active_profile: 'default', + profiles: { default: { api_key: 're_plaintext_key' } }, + }), + ); + + const mockBackend = { + get: vi.fn().mockResolvedValue(null), + set: vi.fn(), + delete: vi.fn(), + isAvailable: vi.fn().mockResolvedValue(true), + name: 'mock-backend', + isSecure: true, + }; + + vi.resetModules(); + vi.doMock('../../src/lib/credential-store', () => ({ + getCredentialBackend: vi.fn().mockResolvedValue(mockBackend), + SERVICE_NAME: 'resend-cli', + resetCredentialBackend: vi.fn(), + })); + + const { resolveApiKeyAsync } = await import('../../src/lib/config'); + const result = await resolveApiKeyAsync(); + expect(result).toEqual({ + key: 're_plaintext_key', + source: 'config', + profile: 'default', + }); + expect(mockBackend.set).not.toHaveBeenCalled(); + }); }); describe('storeApiKeyAsync', () => { diff --git a/tests/lib/credential-backends/macos.test.ts b/tests/lib/credential-backends/macos.test.ts new file mode 100644 index 00000000..3cae43fd --- /dev/null +++ b/tests/lib/credential-backends/macos.test.ts @@ -0,0 +1,118 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('node:child_process', () => { + const mockExecFile = vi.fn( + ( + _cmd: string, + _args: string[], + _opts: unknown, + cb: (err: null, stdout: string, stderr: string) => void, + ) => { + cb(null, '', ''); + }, + ); + + const mockStdin = { + on: vi.fn(), + write: vi.fn(), + end: vi.fn(), + }; + + const mockSpawn = vi.fn(() => ({ + stdin: mockStdin, + stdout: { + on: vi.fn((_event: string, _cb: (data: Buffer) => void) => {}), + }, + stderr: { + on: vi.fn(), + }, + on: vi.fn((event: string, cb: (code: number) => void) => { + if (event === 'close') { + setTimeout(() => cb(0), 0); + } + }), + })); + + return { execFile: mockExecFile, spawn: mockSpawn }; +}); + +describe('MacOSBackend', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('set() passes secret via stdin to osascript, NOT in process args', async () => { + const { spawn } = await import('node:child_process'); + const { MacOSBackend } = await import( + '../../../src/lib/credential-backends/macos' + ); + const backend = new MacOSBackend(); + + await backend.set('resend-cli', 'default', 're_secret_key_1234'); + + expect(spawn).toHaveBeenCalled(); + const spawnCall = vi.mocked(spawn).mock.calls[0]; + const cmd = spawnCall[0] as string; + const args = spawnCall[1] as string[]; + + expect(cmd).toBe('/usr/bin/osascript'); + expect(args).toContain('-l'); + expect(args).toContain('JavaScript'); + + expect(args).not.toContain('re_secret_key_1234'); + + const mockChild = vi.mocked(spawn).mock.results[0].value as { + stdin: { write: ReturnType }; + }; + const writtenScript = mockChild.stdin.write.mock.calls[0][0] as string; + expect(writtenScript).toContain('SecItemUpdate'); + expect(writtenScript).toContain('SecItemAdd'); + expect(writtenScript).toContain('re_secret_key_1234'); + }); + + it('set() includes service and account in the JXA script', async () => { + const { spawn } = await import('node:child_process'); + const { MacOSBackend } = await import( + '../../../src/lib/credential-backends/macos' + ); + const backend = new MacOSBackend(); + + await backend.set('resend-cli', 'my-profile', 're_test_key'); + + const mockChild = vi.mocked(spawn).mock.results[0].value as { + stdin: { write: ReturnType }; + }; + const writtenScript = mockChild.stdin.write.mock.calls[0][0] as string; + expect(writtenScript).toContain('resend-cli'); + expect(writtenScript).toContain('my-profile'); + }); + + it('get() uses execFile with security command', async () => { + const { execFile } = await import('node:child_process'); + const { MacOSBackend } = await import( + '../../../src/lib/credential-backends/macos' + ); + const backend = new MacOSBackend(); + + await backend.get('resend-cli', 'default'); + + expect(execFile).toHaveBeenCalled(); + const args = vi.mocked(execFile).mock.calls[0][1] as string[]; + expect(args).toContain('find-generic-password'); + expect(args).toContain('-w'); + }); + + it('delete() uses execFile with security command', async () => { + const { execFile } = await import('node:child_process'); + const { MacOSBackend } = await import( + '../../../src/lib/credential-backends/macos' + ); + const backend = new MacOSBackend(); + + await backend.delete('resend-cli', 'default'); + + expect(execFile).toHaveBeenCalled(); + const args = vi.mocked(execFile).mock.calls[0][1] as string[]; + expect(args).toContain('delete-generic-password'); + }); +});