Fix #17: detect locked keychain and preserve credentials on access failure #3

+238 -58

8 changed files

Diff round #0

expand all

src

commands

issue.ts

lib

api-client.ts

session.ts

utils

auth-helpers.ts

tests

commands

issue.test.ts

lib

api-client.test.ts

issues-api.test.ts

utils

auth-helpers.test.ts

+8 -29

src/commands/issue.ts

··· 14 14 updateIssue, 15 15 } from '../lib/issues-api.js'; 16 16 import { buildRepoAtUri } from '../utils/at-uri.js'; 17 - import { requireAuth } from '../utils/auth-helpers.js'; 17 + import { ensureAuthenticated, requireAuth } from '../utils/auth-helpers.js'; 18 18 import { readBodyInput } from '../utils/body-input.js'; 19 19 import { formatDate, formatIssueState, outputJson } from '../utils/formatting.js'; 20 20 import { validateIssueBody, validateIssueTitle } from '../utils/validation.js'; ··· 101 101 try { 102 102 // 1. Validate auth 103 103 const client = createApiClient(); 104 - if (!(await client.resumeSession())) { 105 - console.error('✗ Not authenticated. Run "tangled auth login" first.'); 106 - process.exit(1); 107 - } 104 + await ensureAuthenticated(client); 108 105 109 106 // 2. Get repo context 110 107 const context = await getCurrentRepoContext(); ··· 192 189 193 190 // 2. Validate auth 194 191 const client = createApiClient(); 195 - if (!(await client.resumeSession())) { 196 - console.error('✗ Not authenticated. Run "tangled auth login" first.'); 197 - process.exit(1); 198 - } 192 + await ensureAuthenticated(client); 199 193 200 194 // 3. Get repo context 201 195 const context = await getCurrentRepoContext(); ··· 268 262 try { 269 263 // 1. Validate auth 270 264 const client = createApiClient(); 271 - if (!(await client.resumeSession())) { 272 - console.error('✗ Not authenticated. Run "tangled auth login" first.'); 273 - process.exit(1); 274 - } 265 + await ensureAuthenticated(client); 275 266 276 267 // 2. Get repo context 277 268 const context = await getCurrentRepoContext(); ··· 313 304 try { 314 305 // 1. Validate auth 315 306 const client = createApiClient(); 316 - if (!(await client.resumeSession())) { 317 - console.error('✗ Not authenticated. Run "tangled auth login" first.'); 318 - process.exit(1); 319 - } 307 + await ensureAuthenticated(client); 320 308 321 309 // 2. Get repo context 322 310 const context = await getCurrentRepoContext(); ··· 358 346 .action(async (issueId: string, options: { force?: boolean }) => { 359 347 // 1. Validate auth 360 348 const client = createApiClient(); 361 - if (!(await client.resumeSession())) { 362 - console.error('✗ Not authenticated. Run "tangled auth login" first.'); 363 - process.exit(1); 364 - } 349 + await ensureAuthenticated(client); 365 350 366 351 // 2. Get repo context 367 352 const context = await getCurrentRepoContext(); ··· 450 435 try { 451 436 // 1. Validate auth 452 437 const client = createApiClient(); 453 - if (!(await client.resumeSession())) { 454 - console.error('✗ Not authenticated. Run "tangled auth login" first.'); 455 - process.exit(1); 456 - } 438 + await ensureAuthenticated(client); 457 439 458 440 // 2. Get repo context 459 441 const context = await getCurrentRepoContext(); ··· 530 512 try { 531 513 // 1. Validate auth 532 514 const client = createApiClient(); 533 - if (!(await client.resumeSession())) { 534 - console.error('✗ Not authenticated. Run "tangled auth login" first.'); 535 - process.exit(1); 536 - } 515 + await ensureAuthenticated(client); 537 516 538 517 // 2. Get repo context 539 518 const context = await getCurrentRepoContext();

+6 -1

src/lib/api-client.ts

··· 1 1 import { AtpAgent } from '@atproto/api'; 2 2 import type { AtpSessionData } from '@atproto/api'; 3 3 import { 4 + KeychainAccessError, 4 5 clearCurrentSessionMetadata, 5 6 deleteSession, 6 7 getCurrentSessionMetadata, ··· 106 107 107 108 return true; 108 109 } catch (error) { 109 - // If resume fails, clear invalid session 110 + if (error instanceof KeychainAccessError) { 111 + // Don't clear credentials — keychain may just be temporarily locked 112 + throw error; 113 + } 114 + // Session data invalid or agent resume failed — clear stale state 110 115 await clearCurrentSessionMetadata(); 111 116 return false; 112 117 }

+20 -7

src/lib/session.ts

··· 3 3 4 4 const SERVICE_NAME = 'tangled-cli'; 5 5 6 + export class KeychainAccessError extends Error { 7 + constructor(message: string) { 8 + super(message); 9 + this.name = 'KeychainAccessError'; 10 + } 11 + } 12 + 6 13 export interface SessionMetadata { 7 14 handle: string; 8 15 did: string; ··· 44 51 } 45 52 return JSON.parse(serialized) as AtpSessionData; 46 53 } catch (error) { 47 - throw new Error( 48 - `Failed to load session from keychain: ${error instanceof Error ? error.message : 'Unknown error'}` 54 + throw new KeychainAccessError( 55 + `Cannot access keychain: ${error instanceof Error ? error.message : 'Unknown error'}` 49 56 ); 50 57 } 51 58 } ··· 79 86 * Get metadata about current active session 80 87 */ 81 88 export async function getCurrentSessionMetadata(): Promise<SessionMetadata | null> { 82 - const entry = new AsyncEntry(SERVICE_NAME, 'current-session-metadata'); 83 - const serialized = await entry.getPassword(); 84 - if (!serialized) { 85 - return null; 89 + try { 90 + const entry = new AsyncEntry(SERVICE_NAME, 'current-session-metadata'); 91 + const serialized = await entry.getPassword(); 92 + if (!serialized) { 93 + return null; 94 + } 95 + return JSON.parse(serialized) as SessionMetadata; 96 + } catch (error) { 97 + throw new KeychainAccessError( 98 + `Cannot access keychain: ${error instanceof Error ? error.message : 'Unknown error'}` 99 + ); 86 100 } 87 - return JSON.parse(serialized) as SessionMetadata; 88 101 } 89 102 90 103 /**

+45 -1

src/utils/auth-helpers.ts

··· 1 + import { execSync } from 'node:child_process'; 1 2 import type { TangledApiClient } from '../lib/api-client.js'; 3 + import { KeychainAccessError } from '../lib/session.js'; 2 4 3 5 /** 4 6 * Validate that the client is authenticated and has an active session ··· 9 11 did: string; 10 12 handle: string; 11 13 }> { 12 - if (!(await client.isAuthenticated())) { 14 + if (!client.isAuthenticated()) { 13 15 throw new Error('Must be authenticated. Run "tangled auth login" first.'); 14 16 } 15 17 ··· 20 22 21 23 return session; 22 24 } 25 + 26 + function tryUnlockKeychain(): boolean { 27 + if (process.platform !== 'darwin') return false; 28 + try { 29 + execSync('security unlock-keychain', { stdio: 'inherit' }); 30 + return true; 31 + } catch { 32 + return false; 33 + } 34 + } 35 + 36 + /** 37 + * Resume session and ensure the client is authenticated. 38 + * On macOS, if the keychain is locked, attempts to unlock it interactively 39 + * via `security unlock-keychain` before falling back to an error message. 40 + * Exits the process with a clear error message if authentication fails. 41 + */ 42 + export async function ensureAuthenticated(client: TangledApiClient): Promise<void> { 43 + try { 44 + const authenticated = await client.resumeSession(); 45 + if (!authenticated) { 46 + console.error('✗ Not authenticated. Run "tangled auth login" first.'); 47 + process.exit(1); 48 + } 49 + } catch (error) { 50 + if (error instanceof KeychainAccessError) { 51 + const unlocked = tryUnlockKeychain(); 52 + if (unlocked) { 53 + try { 54 + const retried = await client.resumeSession(); 55 + if (retried) return; 56 + } catch { 57 + // fall through to error message 58 + } 59 + } 60 + console.error('✗ Cannot access keychain. Please unlock your Mac keychain and try again.'); 61 + console.error(' You can unlock it manually with: security unlock-keychain'); 62 + process.exit(1); 63 + } 64 + throw error; 65 + } 66 + }

+28 -7

tests/commands/issue.test.ts

··· 157 157 158 158 describe('authentication required', () => { 159 159 it('should fail when not authenticated', async () => { 160 - vi.mocked(mockClient.resumeSession).mockResolvedValue(false); 160 + vi.mocked(authHelpers.ensureAuthenticated).mockImplementationOnce(async () => { 161 + console.error('✗ Not authenticated. Run "tangled auth login" first.'); 162 + process.exit(1); 163 + }); 161 164 162 165 const command = createIssueCommand(); 163 166 ··· 409 412 410 413 describe('authentication required', () => { 411 414 it('should fail when not authenticated', async () => { 412 - vi.mocked(mockClient.resumeSession).mockResolvedValue(false); 415 + vi.mocked(authHelpers.ensureAuthenticated).mockImplementationOnce(async () => { 416 + console.error('✗ Not authenticated. Run "tangled auth login" first.'); 417 + process.exit(1); 418 + }); 413 419 414 420 const command = createIssueCommand(); 415 421 ··· 656 662 }); 657 663 658 664 it('should fail when not authenticated', async () => { 659 - vi.mocked(mockClient.resumeSession).mockResolvedValue(false); 665 + vi.mocked(authHelpers.ensureAuthenticated).mockImplementationOnce(async () => { 666 + console.error('✗ Not authenticated. Run "tangled auth login" first.'); 667 + process.exit(1); 668 + }); 660 669 661 670 const command = createIssueCommand(); 662 671 await expect(command.parseAsync(['node', 'test', 'view', '1'])).rejects.toThrow( ··· 836 845 }); 837 846 838 847 it('should fail when not authenticated', async () => { 839 - vi.mocked(mockClient.resumeSession).mockResolvedValue(false); 848 + vi.mocked(authHelpers.ensureAuthenticated).mockImplementationOnce(async () => { 849 + console.error('✗ Not authenticated. Run "tangled auth login" first.'); 850 + process.exit(1); 851 + }); 840 852 841 853 const command = createIssueCommand(); 842 854 await expect( ··· 961 973 }); 962 974 963 975 it('should fail when not authenticated', async () => { 964 - vi.mocked(mockClient.resumeSession).mockResolvedValue(false); 976 + vi.mocked(authHelpers.ensureAuthenticated).mockImplementationOnce(async () => { 977 + console.error('✗ Not authenticated. Run "tangled auth login" first.'); 978 + process.exit(1); 979 + }); 965 980 966 981 const command = createIssueCommand(); 967 982 await expect(command.parseAsync(['node', 'test', 'close', '1'])).rejects.toThrow( ··· 1030 1045 }); 1031 1046 1032 1047 it('should fail when not authenticated', async () => { 1033 - vi.mocked(mockClient.resumeSession).mockResolvedValue(false); 1048 + vi.mocked(authHelpers.ensureAuthenticated).mockImplementationOnce(async () => { 1049 + console.error('✗ Not authenticated. Run "tangled auth login" first.'); 1050 + process.exit(1); 1051 + }); 1034 1052 1035 1053 const command = createIssueCommand(); 1036 1054 await expect(command.parseAsync(['node', 'test', 'reopen', '1'])).rejects.toThrow( ··· 1137 1155 }); 1138 1156 1139 1157 it('should fail when not authenticated', async () => { 1140 - vi.mocked(mockClient.resumeSession).mockResolvedValue(false); 1158 + vi.mocked(authHelpers.ensureAuthenticated).mockImplementationOnce(async () => { 1159 + console.error('✗ Not authenticated. Run "tangled auth login" first.'); 1160 + process.exit(1); 1161 + }); 1141 1162 1142 1163 const command = createIssueCommand(); 1143 1164 await expect(command.parseAsync(['node', 'test', 'delete', '1', '--force'])).rejects.toThrow(

+23 -9

tests/lib/api-client.test.ts

··· 1 1 import type { AtpSessionData } from '@atproto/api'; 2 2 import { beforeEach, describe, expect, it, vi } from 'vitest'; 3 3 import { TangledApiClient } from '../../src/lib/api-client.js'; 4 + import { KeychainAccessError } from '../../src/lib/session.js'; 4 5 import * as sessionModule from '../../src/lib/session.js'; 5 6 import { mockSessionData, mockSessionMetadata } from '../helpers/mock-data.js'; 6 7 ··· 30 31 }; 31 32 }); 32 33 33 - // Mock session management 34 - vi.mock('../../src/lib/session.js', () => ({ 35 - saveSession: vi.fn(), 36 - loadSession: vi.fn(), 37 - deleteSession: vi.fn(), 38 - saveCurrentSessionMetadata: vi.fn(), 39 - getCurrentSessionMetadata: vi.fn(), 40 - clearCurrentSessionMetadata: vi.fn(), 41 - })); 34 + // Mock session management (use importOriginal to preserve KeychainAccessError class) 35 + vi.mock('../../src/lib/session.js', async (importOriginal) => { 36 + const actual = await importOriginal<typeof import('../../src/lib/session.js')>(); 37 + return { 38 + ...actual, 39 + saveSession: vi.fn(), 40 + loadSession: vi.fn(), 41 + deleteSession: vi.fn(), 42 + saveCurrentSessionMetadata: vi.fn(), 43 + getCurrentSessionMetadata: vi.fn(), 44 + clearCurrentSessionMetadata: vi.fn(), 45 + }; 46 + }); 42 47 43 48 describe('TangledApiClient', () => { 44 49 let client: TangledApiClient; ··· 150 155 expect(resumed).toBe(false); 151 156 expect(vi.mocked(sessionModule.clearCurrentSessionMetadata)).toHaveBeenCalled(); 152 157 }); 158 + 159 + it('should rethrow KeychainAccessError without clearing metadata', async () => { 160 + vi.mocked(sessionModule.getCurrentSessionMetadata).mockRejectedValueOnce( 161 + new KeychainAccessError('Cannot access keychain: locked') 162 + ); 163 + 164 + await expect(client.resumeSession()).rejects.toThrow(KeychainAccessError); 165 + expect(vi.mocked(sessionModule.clearCurrentSessionMetadata)).not.toHaveBeenCalled(); 166 + }); 153 167 }); 154 168 155 169 describe('isAuthenticated', () => {

+1 -1

tests/lib/issues-api.test.ts

··· 28 28 }; 29 29 30 30 return { 31 - isAuthenticated: vi.fn(async () => authenticated), 31 + isAuthenticated: vi.fn(() => authenticated), 32 32 getSession: vi.fn(() => 33 33 authenticated ? { did: 'did:plc:test123', handle: 'test.bsky.social' } : null 34 34 ),

+107 -3

tests/utils/auth-helpers.test.ts

··· 1 - import { describe, expect, it, vi } from 'vitest'; 1 + import { execSync } from 'node:child_process'; 2 + import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; 2 3 import type { TangledApiClient } from '../../src/lib/api-client.js'; 3 - import { requireAuth } from '../../src/utils/auth-helpers.js'; 4 + import { KeychainAccessError } from '../../src/lib/session.js'; 5 + import { ensureAuthenticated, requireAuth } from '../../src/utils/auth-helpers.js'; 6 + 7 + vi.mock('node:child_process', () => ({ 8 + execSync: vi.fn(), 9 + })); 4 10 5 11 // Mock API client factory 6 12 const createMockClient = ( ··· 8 14 session: { did: string; handle: string } | null 9 15 ): TangledApiClient => { 10 16 return { 11 - isAuthenticated: vi.fn(async () => authenticated), 17 + isAuthenticated: vi.fn(() => authenticated), 12 18 getSession: vi.fn(() => session), 13 19 } as unknown as TangledApiClient; 14 20 }; ··· 37 43 await expect(requireAuth(mockClient)).rejects.toThrow('No active session found'); 38 44 }); 39 45 }); 46 + 47 + describe('ensureAuthenticated', () => { 48 + // biome-ignore lint/suspicious/noExplicitAny: spy instance types vary by platform signature 49 + let mockExit: any; 50 + // biome-ignore lint/suspicious/noExplicitAny: spy instance types vary by platform signature 51 + let mockConsoleError: any; 52 + 53 + beforeEach(() => { 54 + mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { 55 + throw new Error('process.exit called'); 56 + }); 57 + mockConsoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); 58 + vi.mocked(execSync).mockReset(); 59 + }); 60 + 61 + afterEach(() => { 62 + mockExit.mockRestore(); 63 + mockConsoleError.mockRestore(); 64 + }); 65 + 66 + it('should return normally when resumeSession succeeds', async () => { 67 + const mockClient = { 68 + resumeSession: vi.fn().mockResolvedValue(true), 69 + } as unknown as TangledApiClient; 70 + 71 + await expect(ensureAuthenticated(mockClient)).resolves.toBeUndefined(); 72 + expect(mockExit).not.toHaveBeenCalled(); 73 + }); 74 + 75 + it('should exit with error when not authenticated', async () => { 76 + const mockClient = { 77 + resumeSession: vi.fn().mockResolvedValue(false), 78 + } as unknown as TangledApiClient; 79 + 80 + await expect(ensureAuthenticated(mockClient)).rejects.toThrow('process.exit called'); 81 + expect(mockConsoleError).toHaveBeenCalledWith( 82 + '✗ Not authenticated. Run "tangled auth login" first.' 83 + ); 84 + expect(mockExit).toHaveBeenCalledWith(1); 85 + }); 86 + 87 + it('should unlock keychain and retry when KeychainAccessError is thrown', async () => { 88 + const mockClient = { 89 + resumeSession: vi 90 + .fn() 91 + .mockRejectedValueOnce(new KeychainAccessError('locked')) 92 + .mockResolvedValueOnce(true), 93 + } as unknown as TangledApiClient; 94 + 95 + vi.mocked(execSync).mockReturnValue(Buffer.from('')); 96 + 97 + await expect(ensureAuthenticated(mockClient)).resolves.toBeUndefined(); 98 + expect(execSync).toHaveBeenCalledWith('security unlock-keychain', { stdio: 'inherit' }); 99 + expect(mockExit).not.toHaveBeenCalled(); 100 + }); 101 + 102 + it('should exit with keychain error when unlock fails', async () => { 103 + const mockClient = { 104 + resumeSession: vi.fn().mockRejectedValue(new KeychainAccessError('locked')), 105 + } as unknown as TangledApiClient; 106 + 107 + vi.mocked(execSync).mockImplementation(() => { 108 + throw new Error('unlock failed'); 109 + }); 110 + 111 + await expect(ensureAuthenticated(mockClient)).rejects.toThrow('process.exit called'); 112 + expect(mockConsoleError).toHaveBeenCalledWith( 113 + '✗ Cannot access keychain. Please unlock your Mac keychain and try again.' 114 + ); 115 + expect(mockExit).toHaveBeenCalledWith(1); 116 + }); 117 + 118 + it('should exit with keychain error when unlock succeeds but retry fails', async () => { 119 + const mockClient = { 120 + resumeSession: vi 121 + .fn() 122 + .mockRejectedValueOnce(new KeychainAccessError('locked')) 123 + .mockRejectedValueOnce(new KeychainAccessError('still locked')), 124 + } as unknown as TangledApiClient; 125 + 126 + vi.mocked(execSync).mockReturnValue(Buffer.from('')); 127 + 128 + await expect(ensureAuthenticated(mockClient)).rejects.toThrow('process.exit called'); 129 + expect(mockConsoleError).toHaveBeenCalledWith( 130 + '✗ Cannot access keychain. Please unlock your Mac keychain and try again.' 131 + ); 132 + expect(mockExit).toHaveBeenCalledWith(1); 133 + }); 134 + 135 + it('should rethrow unexpected errors', async () => { 136 + const mockClient = { 137 + resumeSession: vi.fn().mockRejectedValue(new Error('unexpected network error')), 138 + } as unknown as TangledApiClient; 139 + 140 + await expect(ensureAuthenticated(mockClient)).rejects.toThrow('unexpected network error'); 141 + expect(mockExit).not.toHaveBeenCalled(); 142 + }); 143 + });

History

4 rounds 1 comment

markbennett.ca submitted #3 5w

diff interdiff

5 commits

expand

3f52b8eb

Fix #17: detect locked keychain and preserve credentials on access failure

e88c6edf

Move session metadata from keychain to plain file to prevent credential loss

29bfafa9

chore: allow git fetch in Claude permissions

ba9505a3

Formatting

9741e20a

Update pipeline cloning

expand 1 comment

markbennett.ca 5w

Alright, I haven't seen the error for a while now. Will try this and create a new issue if it comes up again!

pull request successfully merged

markbennett.ca submitted #2 5w

diff interdiff

4 commits

expand

fda30bbc

Fix #17: detect locked keychain and preserve credentials on access failure

96add696

Move session metadata from keychain to plain file to prevent credential loss

269745a4

chore: allow git fetch in Claude permissions

afadda07

Formatting

expand 0 comments

markbennett.ca submitted #1 5w

diff interdiff

2 commits

expand

fda30bbc

Fix #17: detect locked keychain and preserve credentials on access failure

96add696

Move session metadata from keychain to plain file to prevent credential loss

expand 0 comments

markbennett.ca submitted #0 5w

1 commit

expand

fda30bbc

Fix #17: detect locked keychain and preserve credentials on access failure

expand 0 comments