diff --git a/src/lib/config.ts b/src/lib/config.ts index 86abc08..9acf968 100644 --- a/src/lib/config.ts +++ b/src/lib/config.ts @@ -384,34 +384,36 @@ export async function storeApiKeyAsync( } const backend = await getCredentialBackend(); - const isFileBackend = !backend.isSecure; - if (isFileBackend) { - // Do NOT clear a pre-existing `storage: 'secure_storage'` marker here. - // Other profiles may still have their keys in secure storage. - // resolveApiKeyAsync already falls through from secure storage to file-based - // lookup, so keeping the marker is safe and avoids orphaning those profiles. + if (!backend.isSecure) { const configPath = storeApiKey(apiKey, profile, permission); return { configPath, backend }; } - // Store in secure backend await backend.set(SERVICE_NAME, profile, apiKey); - // Update credentials file: mark storage as secure, keep profile entry (without api_key) - const creds = readCredentials() || { - active_profile: 'default', - profiles: {}, - }; - creds.storage = 'secure_storage'; - creds.profiles[profile] = { ...(permission && { permission }) }; + try { + const creds = readCredentials() || { + active_profile: 'default', + profiles: {}, + }; + creds.storage = 'secure_storage'; + creds.profiles[profile] = { ...(permission && { permission }) }; + + if (Object.keys(creds.profiles).length === 1) { + creds.active_profile = profile; + } - if (Object.keys(creds.profiles).length === 1) { - creds.active_profile = profile; + const configPath = writeCredentials(creds); + return { configPath, backend }; + } catch (fileError) { + try { + await backend.delete(SERVICE_NAME, profile); + } catch { + // intentionally empty + } + throw fileError; } - - const configPath = writeCredentials(creds); - return { configPath, backend }; } export async function removeApiKeyAsync(profileName?: string): Promise { @@ -423,14 +425,34 @@ export async function removeApiKeyAsync(profileName?: string): Promise { creds?.active_profile || 'default'; + let removedKey: string | null = null; + if (creds?.storage === 'secure_storage') { const backend = await getCredentialBackend(); if (backend.isSecure) { - await backend.delete(SERVICE_NAME, profile); + removedKey = await backend.get(SERVICE_NAME, profile); + const deleted = await backend.delete(SERVICE_NAME, profile); + if (!deleted) { + throw new Error( + `Failed to remove credential for profile "${profile}" from ${backend.name}`, + ); + } } } - return removeApiKey(profile); + try { + return removeApiKey(profile); + } catch (fileError) { + if (removedKey) { + const backend = await getCredentialBackend(); + try { + await backend.set(SERVICE_NAME, profile, removedKey); + } catch { + // intentionally empty + } + } + throw fileError; + } } export async function removeAllApiKeysAsync(): Promise { @@ -440,15 +462,23 @@ export async function removeAllApiKeysAsync(): Promise { if (creds?.storage === 'secure_storage') { const backend = await getCredentialBackend(); if (backend.isSecure) { - await Promise.all( - Object.keys(creds.profiles).map((profile) => - backend.delete(SERVICE_NAME, profile), - ), + const profiles = Object.keys(creds.profiles); + const results = await Promise.all( + profiles.map(async (profile) => ({ + profile, + deleted: await backend.delete(SERVICE_NAME, profile), + })), ); + + const failed = results.filter((r) => !r.deleted); + if (failed.length > 0) { + throw new Error( + `Failed to remove credentials from ${backend.name} for profiles: ${failed.map((r) => r.profile).join(', ')}`, + ); + } } } - // Remove credentials file (may already be gone if only secure storage) if (existsSync(configPath)) { unlinkSync(configPath); } @@ -467,7 +497,31 @@ export async function renameProfileAsync( const key = await backend.get(SERVICE_NAME, oldName); if (key) { await backend.set(SERVICE_NAME, newName, key); - await backend.delete(SERVICE_NAME, oldName); + + const deleted = await backend.delete(SERVICE_NAME, oldName); + if (!deleted) { + try { + await backend.delete(SERVICE_NAME, newName); + } catch { + // intentionally empty + } + throw new Error( + `Failed to remove old credential for profile "${oldName}" from ${backend.name}`, + ); + } + + try { + renameProfile(oldName, newName); + } catch (fileError) { + try { + await backend.set(SERVICE_NAME, oldName, key); + await backend.delete(SERVICE_NAME, newName); + } catch { + // intentionally empty + } + throw fileError; + } + return; } } } diff --git a/tests/lib/config-async.test.ts b/tests/lib/config-async.test.ts index a5d7c36..0f9e7c3 100644 --- a/tests/lib/config-async.test.ts +++ b/tests/lib/config-async.test.ts @@ -1,7 +1,7 @@ -import { mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { existsSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { captureTestEnv } from '../helpers'; describe('resolveApiKeyAsync', () => { @@ -27,20 +27,20 @@ describe('resolveApiKeyAsync', () => { vi.restoreAllMocks(); }); - test('returns flag value without touching backend', async () => { + it('returns flag value without touching backend', async () => { const { resolveApiKeyAsync } = await import('../../src/lib/config'); const result = await resolveApiKeyAsync('re_flag_key'); expect(result).toEqual({ key: 're_flag_key', source: 'flag' }); }); - test('returns env value without touching backend', async () => { + it('returns env value without touching backend', async () => { process.env.RESEND_API_KEY = 're_env_key'; const { resolveApiKeyAsync } = await import('../../src/lib/config'); const result = await resolveApiKeyAsync(); expect(result).toEqual({ key: 're_env_key', source: 'env' }); }); - test('reads from file when storage is not keychain', async () => { + it('reads from file when storage is not keychain', async () => { const configDir = join(tmpDir, 'resend'); mkdirSync(configDir, { recursive: true }); writeFileSync( @@ -60,7 +60,7 @@ describe('resolveApiKeyAsync', () => { }); }); - test('reads from credential backend when storage is keychain', async () => { + it('reads from credential backend when storage is keychain', async () => { const configDir = join(tmpDir, 'resend'); mkdirSync(configDir, { recursive: true }); writeFileSync( @@ -98,7 +98,7 @@ describe('resolveApiKeyAsync', () => { expect(mockBackend.get).toHaveBeenCalledWith('resend-cli', 'default'); }); - test('falls back to file api_key when keychain has no entry but file does (mixed state)', async () => { + it('falls back to file api_key when keychain has no entry but file does (mixed state)', async () => { const configDir = join(tmpDir, 'resend'); mkdirSync(configDir, { recursive: true }); writeFileSync( @@ -135,7 +135,7 @@ describe('resolveApiKeyAsync', () => { }); }); - test('returns null when keychain has no entry', async () => { + it('returns null when keychain has no entry', async () => { const configDir = join(tmpDir, 'resend'); mkdirSync(configDir, { recursive: true }); writeFileSync( @@ -189,7 +189,7 @@ describe('storeApiKeyAsync', () => { vi.restoreAllMocks(); }); - test('stores key in file backend when keychain unavailable', async () => { + it('stores key in file backend when keychain unavailable', async () => { vi.resetModules(); vi.doUnmock('../../src/lib/credential-store'); const { storeApiKeyAsync } = await import('../../src/lib/config'); @@ -197,6 +197,39 @@ describe('storeApiKeyAsync', () => { expect(configPath).toContain('credentials.json'); expect(backend.isSecure).toBe(false); }); + + it('rolls back backend secret when credentials file write fails', async () => { + const configDir = join(tmpDir, 'resend'); + mkdirSync(configDir, { recursive: true }); + const credPath = join(configDir, 'credentials.json'); + mkdirSync(credPath); + + const mockBackend = { + get: vi.fn(), + set: vi.fn().mockResolvedValue(undefined), + delete: vi.fn().mockResolvedValue(true), + 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 { storeApiKeyAsync } = await import('../../src/lib/config'); + await expect(storeApiKeyAsync('re_test_key', 'prod')).rejects.toThrow(); + + expect(mockBackend.set).toHaveBeenCalledWith( + 'resend-cli', + 'prod', + 're_test_key', + ); + expect(mockBackend.delete).toHaveBeenCalledWith('resend-cli', 'prod'); + }); }); describe('removeApiKeyAsync', () => { @@ -219,7 +252,7 @@ describe('removeApiKeyAsync', () => { vi.restoreAllMocks(); }); - test('calls backend.delete when backend is secure', async () => { + it('calls backend.delete when backend is secure', async () => { const configDir = join(tmpDir, 'resend'); mkdirSync(configDir, { recursive: true }); writeFileSync( @@ -232,9 +265,9 @@ describe('removeApiKeyAsync', () => { ); const mockBackend = { - get: vi.fn(), + get: vi.fn().mockResolvedValue('re_stored_key'), set: vi.fn(), - delete: vi.fn().mockResolvedValue(undefined), + delete: vi.fn().mockResolvedValue(true), isAvailable: vi.fn().mockResolvedValue(true), name: 'mock-backend', isSecure: true, @@ -252,7 +285,7 @@ describe('removeApiKeyAsync', () => { expect(mockBackend.delete).toHaveBeenCalledWith('resend-cli', 'default'); }); - test('skips backend.delete when backend is not secure', async () => { + it('skips backend.delete when backend is not secure', async () => { const configDir = join(tmpDir, 'resend'); mkdirSync(configDir, { recursive: true }); writeFileSync( @@ -289,6 +322,90 @@ describe('removeApiKeyAsync', () => { expect(creds?.profiles.default).toBeUndefined(); expect(creds?.profiles.other).toBeDefined(); }); + + it('throws when backend.delete returns false', async () => { + const configDir = join(tmpDir, 'resend'); + mkdirSync(configDir, { recursive: true }); + writeFileSync( + join(configDir, 'credentials.json'), + JSON.stringify({ + active_profile: 'default', + storage: 'secure_storage', + profiles: { default: {}, other: {} }, + }), + ); + + const mockBackend = { + get: vi.fn().mockResolvedValue('re_stored_key'), + set: vi.fn(), + delete: vi.fn().mockResolvedValue(false), + 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 { removeApiKeyAsync, readCredentials } = await import( + '../../src/lib/config' + ); + await expect(removeApiKeyAsync('default')).rejects.toThrow( + 'Failed to remove credential', + ); + const creds = readCredentials(); + expect(creds?.profiles.default).toBeDefined(); + }); + + it('restores backend key when file removal fails after backend delete', async () => { + const configDir = join(tmpDir, 'resend'); + mkdirSync(configDir, { recursive: true }); + const credPath = join(configDir, 'credentials.json'); + writeFileSync( + credPath, + JSON.stringify({ + active_profile: 'only', + storage: 'secure_storage', + profiles: { only: {} }, + }), + ); + + const mockBackend = { + get: vi.fn().mockResolvedValue('re_only_key'), + set: vi.fn().mockResolvedValue(undefined), + delete: vi.fn().mockResolvedValue(true), + 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 { chmodSync } = await import('node:fs'); + const { removeApiKeyAsync } = await import('../../src/lib/config'); + + chmodSync(configDir, 0o555); + + try { + await expect(removeApiKeyAsync('only')).rejects.toThrow(); + expect(mockBackend.set).toHaveBeenCalledWith( + 'resend-cli', + 'only', + 're_only_key', + ); + } finally { + chmodSync(configDir, 0o755); + } + }); }); describe('removeAllApiKeysAsync', () => { @@ -311,7 +428,7 @@ describe('removeAllApiKeysAsync', () => { vi.restoreAllMocks(); }); - test('deletes all profiles from keychain when secure', async () => { + it('deletes all profiles from keychain when secure', async () => { const configDir = join(tmpDir, 'resend'); mkdirSync(configDir, { recursive: true }); writeFileSync( @@ -326,7 +443,7 @@ describe('removeAllApiKeysAsync', () => { const mockBackend = { get: vi.fn(), set: vi.fn(), - delete: vi.fn().mockResolvedValue(undefined), + delete: vi.fn().mockResolvedValue(true), isAvailable: vi.fn().mockResolvedValue(true), name: 'mock-backend', isSecure: true, @@ -347,7 +464,7 @@ describe('removeAllApiKeysAsync', () => { expect(mockBackend.delete).toHaveBeenCalledWith('resend-cli', 'prod'); }); - test('skips keychain deletion when not secure', async () => { + it('skips keychain deletion when not secure', async () => { const configDir = join(tmpDir, 'resend'); mkdirSync(configDir, { recursive: true }); writeFileSync( @@ -379,6 +496,46 @@ describe('removeAllApiKeysAsync', () => { await removeAllApiKeysAsync(); expect(mockBackend.delete).not.toHaveBeenCalled(); }); + + it('throws and preserves credentials file when any backend delete fails', async () => { + const configDir = join(tmpDir, 'resend'); + mkdirSync(configDir, { recursive: true }); + const credPath = join(configDir, 'credentials.json'); + writeFileSync( + credPath, + JSON.stringify({ + active_profile: 'default', + storage: 'secure_storage', + profiles: { default: {}, staging: {}, prod: {} }, + }), + ); + + const mockBackend = { + get: vi.fn(), + set: vi.fn(), + delete: vi + .fn() + .mockResolvedValueOnce(true) + .mockResolvedValueOnce(false) + .mockResolvedValueOnce(true), + 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 { removeAllApiKeysAsync } = await import('../../src/lib/config'); + await expect(removeAllApiKeysAsync()).rejects.toThrow( + 'Failed to remove credentials', + ); + expect(existsSync(credPath)).toBe(true); + }); }); describe('renameProfileAsync', () => { @@ -401,7 +558,7 @@ describe('renameProfileAsync', () => { vi.restoreAllMocks(); }); - test('renames in keychain when secure', async () => { + it('renames in keychain when secure', async () => { const configDir = join(tmpDir, 'resend'); mkdirSync(configDir, { recursive: true }); writeFileSync( @@ -416,7 +573,7 @@ describe('renameProfileAsync', () => { const mockBackend = { get: vi.fn().mockResolvedValue('re_secret_key'), set: vi.fn().mockResolvedValue(undefined), - delete: vi.fn().mockResolvedValue(undefined), + delete: vi.fn().mockResolvedValue(true), isAvailable: vi.fn().mockResolvedValue(true), name: 'mock-backend', isSecure: true, @@ -446,7 +603,7 @@ describe('renameProfileAsync', () => { expect(creds?.active_profile).toBe('new-name'); }); - test('skips keychain when not secure', async () => { + it('skips keychain when not secure', async () => { const configDir = join(tmpDir, 'resend'); mkdirSync(configDir, { recursive: true }); writeFileSync( @@ -485,4 +642,97 @@ describe('renameProfileAsync', () => { expect(creds?.profiles['new-name']).toEqual({ api_key: 're_file_key' }); expect(creds?.profiles['old-name']).toBeUndefined(); }); + + it('throws when backend.delete of old name returns false', async () => { + const configDir = join(tmpDir, 'resend'); + mkdirSync(configDir, { recursive: true }); + writeFileSync( + join(configDir, 'credentials.json'), + JSON.stringify({ + active_profile: 'old-name', + storage: 'secure_storage', + profiles: { 'old-name': {} }, + }), + ); + + const mockBackend = { + get: vi.fn().mockResolvedValue('re_secret_key'), + set: vi.fn().mockResolvedValue(undefined), + delete: vi.fn().mockResolvedValue(false), + 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 { renameProfileAsync, readCredentials } = await import( + '../../src/lib/config' + ); + await expect(renameProfileAsync('old-name', 'new-name')).rejects.toThrow( + 'Failed to remove old credential', + ); + + expect(mockBackend.delete).toHaveBeenCalledWith('resend-cli', 'old-name'); + expect(mockBackend.delete).toHaveBeenCalledWith('resend-cli', 'new-name'); + + const creds = readCredentials(); + expect(creds?.profiles['old-name']).toBeDefined(); + expect(creds?.profiles['new-name']).toBeUndefined(); + }); + + it('rolls back backend when file rename fails', async () => { + const configDir = join(tmpDir, 'resend'); + mkdirSync(configDir, { recursive: true }); + const credPath = join(configDir, 'credentials.json'); + writeFileSync( + credPath, + JSON.stringify({ + active_profile: 'old-name', + storage: 'secure_storage', + profiles: { 'old-name': {} }, + }), + ); + + const mockBackend = { + get: vi.fn().mockResolvedValue('re_secret_key'), + set: vi.fn().mockResolvedValue(undefined), + delete: vi.fn().mockResolvedValue(true), + 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 { chmodSync } = await import('node:fs'); + const { renameProfileAsync } = await import('../../src/lib/config'); + + chmodSync(credPath, 0o444); + + try { + await expect( + renameProfileAsync('old-name', 'new-name'), + ).rejects.toThrow(); + + expect(mockBackend.set).toHaveBeenCalledWith( + 'resend-cli', + 'old-name', + 're_secret_key', + ); + expect(mockBackend.delete).toHaveBeenCalledWith('resend-cli', 'new-name'); + } finally { + chmodSync(credPath, 0o644); + } + }); });