From c703140295aec56ae6fe4a0f8b33e4901c5c813b Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Tue, 18 Aug 2026 19:57:12 -0500 Subject: [PATCH 1/3] fix(auth): stop repeated macOS keychain password prompts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes: 1. In-process credential cache: a single CLI run reads credentials from many call sites (auth, telemetry, token refresh); each uncached keyring read is a separate keychain ACL check, i.e. one password dialog per read on an untrusted binary. First read is now cached; saves keep the cache coherent. 2. macOS keychain access now shells out to /usr/bin/security instead of the native binding. Keychain item ACLs pin trust to the requesting binary's code signature, and the Bun-compiled release binary is ad-hoc signed — a new signature every release — so 'Always Allow' never persisted across versions. /usr/bin/security is Apple-signed and stable; items it creates read back silently. Existing items are re-minted (delete + add) on next save, so migration costs at most one final prompt. Interim measure until releases are Developer ID signed, at which point darwin should revert to @napi-rs/keyring to regain app-level isolation (any user process can invoke /usr/bin/security, so this trades app isolation for prompt-free UX — comparable in practice to the existing file fallback). --- src/lib/credential-store.spec.ts | 85 +++++++++++++++++++++++++++++- src/lib/credential-store.ts | 59 +++++++++++++++++++-- src/lib/credentials.spec.ts | 3 ++ src/lib/darwin-keychain.ts | 69 ++++++++++++++++++++++++ src/test/keyring-isolation.spec.ts | 5 ++ src/test/setup.ts | 29 ++++++++++ 6 files changed, 244 insertions(+), 6 deletions(-) create mode 100644 src/lib/darwin-keychain.ts diff --git a/src/lib/credential-store.spec.ts b/src/lib/credential-store.spec.ts index dac61d03..574a4675 100644 --- a/src/lib/credential-store.spec.ts +++ b/src/lib/credential-store.spec.ts @@ -19,6 +19,9 @@ const mockKeyring = new Map(); // Track whether keyring is "available" for this test let keyringAvailable = true; +// Count backend reads so cache behavior is assertable +let keyringReads = 0; + // Mock @napi-rs/keyring BEFORE importing credential-store vi.mock('@napi-rs/keyring', () => ({ Entry: class MockEntry { @@ -32,6 +35,45 @@ vi.mock('@napi-rs/keyring', () => ({ } getPassword(): string | null { + keyringReads++; + if (!keyringAvailable && this.account !== '__probe__') { + throw new Error('Keyring not available'); + } + return mockKeyring.get(this.key) ?? null; + } + + setPassword(password: string): void { + if (!keyringAvailable) { + throw new Error('Keyring not available'); + } + mockKeyring.set(this.key, password); + } + + deletePassword(): void { + if (!keyringAvailable && mockKeyring.has(this.key)) { + throw new Error('Keyring not available'); + } + mockKeyring.delete(this.key); + } + }, +})); + +// On darwin, credential-store routes through DarwinSecurityEntry instead of +// the native Entry. Back it with the SAME map + availability flag so every +// test behaves identically on all platforms. +vi.mock('./darwin-keychain.js', () => ({ + DarwinSecurityEntry: class MockDarwinSecurityEntry { + private key: string; + + constructor( + service: string, + private account: string, + ) { + this.key = `${service}:${account}`; + } + + getPassword(): string | null { + keyringReads++; if (!keyringAvailable && this.account !== '__probe__') { throw new Error('Keyring not available'); } @@ -85,9 +127,10 @@ describe('credential-store', () => { installerDir = join(testDir, '.workos'); credentialsFile = join(installerDir, 'credentials.json'); - // Reset state + // Reset state (setInsecureStorage also resets the in-process cache) mockKeyring.clear(); keyringAvailable = true; + keyringReads = 0; setInsecureStorage(false); }); @@ -311,6 +354,46 @@ describe('credential-store', () => { }); }); + describe('in-process cache', () => { + it('reads the keyring at most once per process for repeated getCredentials calls', () => { + saveCredentials(validCreds); + keyringReads = 0; + + getCredentials(); + getCredentials(); + hasCredentials(); + getCredentials(); + + expect(keyringReads).toBeLessThanOrEqual(1); + }); + + it('caches the logged-out state too', () => { + getCredentials(); + keyringReads = 0; + + expect(getCredentials()).toBeNull(); + expect(keyringReads).toBe(0); + }); + + it('saveCredentials updates the cache without a fresh read', () => { + saveCredentials(validCreds); + const updated = { ...validCreds, accessToken: 'token456' }; + saveCredentials(updated); + + keyringReads = 0; + expect(getCredentials()?.accessToken).toBe('token456'); + expect(keyringReads).toBe(0); + }); + + it('clearCredentials invalidates the cache', () => { + saveCredentials(validCreds); + expect(getCredentials()).not.toBeNull(); + + clearCredentials(); + expect(getCredentials()).toBeNull(); + }); + }); + describe('getCredentialsPath', () => { it('returns path to credentials file', () => { const path = getCredentialsPath(); diff --git a/src/lib/credential-store.ts b/src/lib/credential-store.ts index ed32d849..6e294870 100644 --- a/src/lib/credential-store.ts +++ b/src/lib/credential-store.ts @@ -7,6 +7,7 @@ */ import { Entry } from '@napi-rs/keyring'; +import { DarwinSecurityEntry } from './darwin-keychain.js'; import fs from 'node:fs'; import path from 'node:path'; import os from 'node:os'; @@ -53,9 +54,20 @@ let fallbackWarningShown = false; let forceInsecureStorage = false; let migrationAttempted = false; +/** + * In-process cache: a single CLI run reads credentials from many call sites + * (auth, telemetry, token refresh, ...). Each uncached keyring read is a + * separate keychain ACL check — on macOS with an untrusted binary that means + * one password dialog PER READ. Cache the first result; saves keep it + * coherent (all refresh paths write through saveCredentials in-process). + * undefined = not loaded yet. + */ +let cachedCreds: Credentials | null | undefined; + export function setInsecureStorage(value: boolean): void { forceInsecureStorage = value; migrationAttempted = false; + cachedCreds = undefined; } function getCredentialsDir(): string { @@ -128,7 +140,20 @@ function deleteFile(): void { } } -function getKeyringEntry(): Entry { +interface KeyringEntry { + getPassword(): string | null; + setPassword(password: string): void; + deletePassword(): void; +} + +function getKeyringEntry(): KeyringEntry { + // On macOS, go through /usr/bin/security (stable Apple-signed binary) + // instead of the native binding: the ad-hoc-signed CLI binary changes + // signature every release, so native keychain access prompts per version. + // See darwin-keychain.ts; revert once releases are Developer ID signed. + if (process.platform === 'darwin') { + return new DarwinSecurityEntry(SERVICE_NAME, ACCOUNT_NAME); + } return new Entry(SERVICE_NAME, ACCOUNT_NAME); } @@ -207,17 +232,33 @@ export function hasCredentials(): boolean { // read as logged-out here too, so this never disagrees with getCredentials(). // (readFrom* both run isValidCredentials; avoids getCredentials()'s keyring // migration side effect.) + if (cachedCreds !== undefined) return cachedCreds !== null; if (forceInsecureStorage) { return readFromFile() !== null; } - return readFromKeyring() !== null || readFromFile() !== null; + const keyringCreds = readFromKeyring(); + if (keyringCreds) { + // Safe to cache: getCredentials() would return this without migrating. + // A file-only hit is NOT cached so its migration still runs there. + cachedCreds = keyringCreds; + return true; + } + return readFromFile() !== null; } export function getCredentials(): Credentials | null { - if (forceInsecureStorage) return readFromFile(); + if (cachedCreds !== undefined) return cachedCreds; + + if (forceInsecureStorage) { + cachedCreds = readFromFile(); + return cachedCreds; + } const keyringCreds = readFromKeyring(); - if (keyringCreds) return keyringCreds; + if (keyringCreds) { + cachedCreds = keyringCreds; + return keyringCreds; + } const fileCreds = readFromFile(); if (fileCreds) { @@ -225,25 +266,33 @@ export function getCredentials(): Credentials | null { migrationAttempted = true; writeToKeyring(fileCreds); } + cachedCreds = fileCreds; return fileCreds; } + cachedCreds = null; return null; } export function saveCredentials(creds: Credentials): void { - if (forceInsecureStorage) return writeToFile(creds); + if (forceInsecureStorage) { + writeToFile(creds); + cachedCreds = creds; + return; + } if (!writeToKeyring(creds)) { showFallbackWarning(); writeToFile(creds); } + cachedCreds = creds; } export function clearCredentials(): void { deleteFromKeyring(); deleteFile(); migrationAttempted = false; + cachedCreds = undefined; } export function updateTokens(accessToken: string, expiresAt: number, refreshToken?: string): void { diff --git a/src/lib/credentials.spec.ts b/src/lib/credentials.spec.ts index 35d8ef86..8ee4ee20 100644 --- a/src/lib/credentials.spec.ts +++ b/src/lib/credentials.spec.ts @@ -142,6 +142,9 @@ describe('credentials', () => { saveCredentials(validCreds); // Corrupt the file writeFileSync(credentialsFile, 'not valid json'); + // External corruption is only observable from a fresh process — + // credential reads are cached in-process. Reset to simulate one. + setInsecureStorage(true); expect(getCredentials()).toBeNull(); }); }); diff --git a/src/lib/darwin-keychain.ts b/src/lib/darwin-keychain.ts new file mode 100644 index 00000000..f2a6e5a2 --- /dev/null +++ b/src/lib/darwin-keychain.ts @@ -0,0 +1,69 @@ +/** + * macOS credential storage via /usr/bin/security instead of the native + * keyring binding. + * + * Why: keychain item ACLs pin trust to the requesting binary's code + * signature. The shipped CLI is a Bun-compiled, ad-hoc-signed binary whose + * signature changes every release, so reads through the native binding + * trigger a password prompt per version (and per read). /usr/bin/security is + * Apple-signed and stable, so items it creates read back silently forever. + * + * Accepted trade-off until releases are Developer ID signed: any user-level + * process can read the item silently through the same tool, so app-level + * isolation is lost — comparable to the existing file fallback's protection. + * ponytail: interim until Developer ID signing lands, then revert darwin to + * @napi-rs/keyring to regain app-level isolation. + */ +import { spawnSync } from 'node:child_process'; + +const SECURITY = '/usr/bin/security'; +const NOT_FOUND_EXIT = 44; // errSecItemNotFound + +/** Same surface as @napi-rs/keyring's Entry, backed by /usr/bin/security. */ +export class DarwinSecurityEntry { + constructor( + private readonly service: string, + private readonly account: string, + ) {} + + getPassword(): string | null { + const r = spawnSync(SECURITY, ['find-generic-password', '-s', this.service, '-a', this.account, '-w'], { + encoding: 'utf-8', + }); + if (r.status === NOT_FOUND_EXIT) return null; + if (r.status !== 0) { + throw new Error(`security find-generic-password exited ${r.status}: ${(r.stderr ?? '').trim()}`); + } + const raw = r.stdout.replace(/\n$/, ''); + // Items written by this class hold base64; items left behind by the + // native binding hold raw JSON (read of those may prompt once — their + // ACL still pins the old binary — until the next save re-mints the item). + if (raw.startsWith('{')) return raw; + return Buffer.from(raw, 'base64').toString('utf-8'); + } + + setPassword(password: string): void { + // Delete-then-add rather than update in place: updating keeps the old + // item's ACL (pinned to a previous binary). A fresh item is owned by + // /usr/bin/security and reads back without prompting. + this.deletePassword(); + const b64 = Buffer.from(password, 'utf-8').toString('base64'); + // -i reads commands from stdin so the secret never appears in argv, + // where it would be visible to `ps`. + const r = spawnSync(SECURITY, ['-i'], { + input: `add-generic-password -a "${this.account}" -s "${this.service}" -w "${b64}"\n`, + encoding: 'utf-8', + }); + if (r.status !== 0) { + throw new Error(`security add-generic-password exited ${r.status}: ${(r.stderr ?? '').trim()}`); + } + } + + deletePassword(): void { + const r = spawnSync(SECURITY, ['delete-generic-password', '-s', this.service, '-a', this.account], { + encoding: 'utf-8', + }); + if (r.status === 0 || r.status === NOT_FOUND_EXIT) return; + throw new Error(`security delete-generic-password exited ${r.status}: ${(r.stderr ?? '').trim()}`); + } +} diff --git a/src/test/keyring-isolation.spec.ts b/src/test/keyring-isolation.spec.ts index a141c6d7..8651207c 100644 --- a/src/test/keyring-isolation.spec.ts +++ b/src/test/keyring-isolation.spec.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from 'vitest'; import * as keyring from '@napi-rs/keyring'; +import * as darwinKeychain from '../lib/darwin-keychain.js'; /** * Guardrail: the test suite must NEVER touch the real OS keychain. @@ -15,4 +16,8 @@ describe('keyring test isolation', () => { it('replaces the real OS keychain with an in-memory mock during tests', () => { expect((keyring as Record).__IS_TEST_MOCK__).toBe(true); }); + + it('replaces the darwin /usr/bin/security backend with an in-memory mock during tests', () => { + expect((darwinKeychain as Record).__IS_TEST_MOCK__).toBe(true); + }); }); diff --git a/src/test/setup.ts b/src/test/setup.ts index 4d61a675..baffcb81 100644 --- a/src/test/setup.ts +++ b/src/test/setup.ts @@ -15,6 +15,35 @@ import { vi } from 'vitest'; * is asserted by src/test/keyring-isolation.spec.ts so removing this setup * fails CI instead of silently re-arming the footgun. */ +// Same guardrail for the darwin backend: on macOS credential-store routes +// through /usr/bin/security (see src/lib/darwin-keychain.ts), which would +// touch the real `workos-cli` keychain item just like the native binding. +vi.mock('../lib/darwin-keychain.js', () => { + const store = new Map(); + return { + __IS_TEST_MOCK__: true, + DarwinSecurityEntry: class MockDarwinSecurityEntry { + private key: string; + + constructor(service: string, account: string) { + this.key = `${service}:${account}`; + } + + getPassword(): string | null { + return store.get(this.key) ?? null; + } + + setPassword(password: string): void { + store.set(this.key, password); + } + + deletePassword(): void { + store.delete(this.key); + } + }, + }; +}); + vi.mock('@napi-rs/keyring', () => { const store = new Map(); return { From 94384505213cc90fb7b5e6d17110a9487ed4fedf Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Tue, 18 Aug 2026 20:20:50 -0500 Subject: [PATCH 2/3] fix(auth): apply keychain backend + cache to config-store too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The config keychain item (workos-cli/config) had the same two problems as credentials: native-binding reads pinned to the per-release ad-hoc signature, and no in-process cache despite getConfig() running several times per command. This was observable: with the credential fix in place, a rebuilt binary still prompted once — for the config item. Also hardens DarwinSecurityEntry: service/account are validated against a safe charset since they are interpolated into a `security -i` command line (review feedback). --- src/lib/config-store.spec.ts | 37 ++++++++++++++++++++++++++++ src/lib/config-store.ts | 47 +++++++++++++++++++++++++++++++++--- src/lib/darwin-keychain.ts | 11 ++++++++- 3 files changed, 90 insertions(+), 5 deletions(-) diff --git a/src/lib/config-store.spec.ts b/src/lib/config-store.spec.ts index ced52314..70abbd48 100644 --- a/src/lib/config-store.spec.ts +++ b/src/lib/config-store.spec.ts @@ -60,6 +60,40 @@ vi.mock('@napi-rs/keyring', () => ({ }, })); +// On darwin, config-store routes through DarwinSecurityEntry instead of the +// native Entry. Back it with the SAME map + availability flag so every test +// behaves identically on all platforms. +vi.mock('./darwin-keychain.js', () => ({ + DarwinSecurityEntry: class MockDarwinSecurityEntry { + private key: string; + + constructor(service: string, account: string) { + this.key = `${service}:${account}`; + } + + getPassword(): string | null { + if (!keyringAvailable) { + throw new Error('Keyring not available'); + } + return mockKeyring.get(this.key) ?? null; + } + + setPassword(password: string): void { + if (!keyringAvailable) { + throw new Error('Keyring not available'); + } + mockKeyring.set(this.key, password); + } + + deletePassword(): void { + if (!keyringAvailable && mockKeyring.has(this.key)) { + throw new Error('Keyring not available'); + } + mockKeyring.delete(this.key); + } + }, +})); + // Mock os.homedir BEFORE importing config-store module vi.mock('node:os', async (importOriginal) => { const original = await importOriginal(); @@ -176,6 +210,9 @@ describe('config-store', () => { it('returns null for corrupted file', () => { saveConfig(sampleConfig); writeFileSync(configFile, 'not valid json'); + // External corruption is only observable from a fresh process — + // config reads are cached in-process. Reset to simulate one. + setInsecureConfigStorage(true); expect(getConfig()).toBeNull(); }); }); diff --git a/src/lib/config-store.ts b/src/lib/config-store.ts index 1e1fc99f..aaf13ce1 100644 --- a/src/lib/config-store.ts +++ b/src/lib/config-store.ts @@ -10,6 +10,7 @@ */ import { Entry } from '@napi-rs/keyring'; +import { DarwinSecurityEntry } from './darwin-keychain.js'; import fs from 'node:fs'; import path from 'node:path'; import os from 'node:os'; @@ -58,9 +59,18 @@ let fallbackWarningShown = false; let forceInsecureStorage = false; let migrationAttempted = false; +/** + * In-process cache — same rationale as credential-store: getConfig() is + * called several times per run, and each uncached keyring read is a separate + * keychain ACL check (one password dialog per read on an untrusted binary). + * undefined = not loaded yet. + */ +let cachedConfig: CliConfig | null | undefined; + export function setInsecureConfigStorage(value: boolean): void { forceInsecureStorage = value; migrationAttempted = false; + cachedConfig = undefined; } function getConfigDir(): string { @@ -128,7 +138,19 @@ function deleteFile(): void { } } -function getKeyringEntry(): Entry { +interface KeyringEntry { + getPassword(): string | null; + setPassword(password: string): void; + deletePassword(): void; +} + +function getKeyringEntry(): KeyringEntry { + // Same backend selection as credential-store: on macOS go through + // /usr/bin/security so keychain trust survives across ad-hoc-signed + // releases. See darwin-keychain.ts. + if (process.platform === 'darwin') { + return new DarwinSecurityEntry(SERVICE_NAME, ACCOUNT_NAME); + } return new Entry(SERVICE_NAME, ACCOUNT_NAME); } @@ -193,10 +215,18 @@ function showFallbackWarning(): void { } export function getConfig(): CliConfig | null { - if (forceInsecureStorage) return readFromFile(); + if (cachedConfig !== undefined) return cachedConfig; + + if (forceInsecureStorage) { + cachedConfig = readFromFile(); + return cachedConfig; + } const keyringConfig = readFromKeyring(); - if (keyringConfig) return keyringConfig; + if (keyringConfig) { + cachedConfig = keyringConfig; + return keyringConfig; + } const fileConfig = readFromFile(); if (fileConfig) { @@ -204,18 +234,25 @@ export function getConfig(): CliConfig | null { migrationAttempted = true; writeToKeyring(fileConfig); } + cachedConfig = fileConfig; return fileConfig; } + cachedConfig = null; return null; } export function saveConfig(config: CliConfig): void { - if (forceInsecureStorage) return writeToFile(config); + if (forceInsecureStorage) { + writeToFile(config); + cachedConfig = config; + return; + } if (!writeToKeyring(config)) { showFallbackWarning(); writeToFile(config); + cachedConfig = config; return; } @@ -225,12 +262,14 @@ export function saveConfig(config: CliConfig): void { logWarn('Keyring write succeeded but read-back failed — falling back to file'); writeToFile(config); } + cachedConfig = config; } export function clearConfig(): void { deleteFromKeyring(); deleteFile(); migrationAttempted = false; + cachedConfig = undefined; } export function getActiveEnvironment(): EnvironmentConfig | null { diff --git a/src/lib/darwin-keychain.ts b/src/lib/darwin-keychain.ts index f2a6e5a2..34bddc4e 100644 --- a/src/lib/darwin-keychain.ts +++ b/src/lib/darwin-keychain.ts @@ -24,7 +24,16 @@ export class DarwinSecurityEntry { constructor( private readonly service: string, private readonly account: string, - ) {} + ) { + // service/account are interpolated into a `security -i` command line + // (double-quoted). Current callers pass module constants; reject anything + // that could break the interactive parser if a future caller doesn't. + for (const v of [service, account]) { + if (!/^[\w.-]+$/.test(v)) { + throw new Error(`invalid keychain service/account: ${JSON.stringify(v)}`); + } + } + } getPassword(): string | null { const r = spawnSync(SECURITY, ['find-generic-password', '-s', this.service, '-a', this.account, '-w'], { From afb1c7f3775cd7b11375fb38fe4e0810c1400924 Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Tue, 18 Aug 2026 20:27:36 -0500 Subject: [PATCH 3/3] refactor(auth): extract shared KeyringStore for credential and config stores credential-store and config-store were ~90% duplicated (backend selection, in-process cache, file fallback, one-shot migration, warnings). Both are now thin wrappers around one KeyringStore parameterized by the parts that actually differ: account/file names, validation (credentials must read as logged-out when malformed), and save read-back verification (config only). Public APIs unchanged; net -180 lines. --- src/lib/config-store.ts | 243 +++--------------------------- src/lib/credential-store.ts | 267 +++------------------------------ src/lib/keyring-store.ts | 284 ++++++++++++++++++++++++++++++++++++ 3 files changed, 325 insertions(+), 469 deletions(-) create mode 100644 src/lib/keyring-store.ts diff --git a/src/lib/config-store.ts b/src/lib/config-store.ts index aaf13ce1..298ed627 100644 --- a/src/lib/config-store.ts +++ b/src/lib/config-store.ts @@ -1,21 +1,13 @@ /** - * CLI config storage abstraction with keyring support and file fallback. + * CLI config storage: keychain-backed with file fallback, via KeyringStore + * (backend selection, in-process cache, migration — see keyring-store.ts). * * Stores environment configurations (names, API keys, endpoints) separately * from OAuth credentials. Uses a second keyring entry under the same service. - * - * Storage priority: - * 1. If insecure storage forced: use file only - * 2. Try keyring, fall back to file with warning if unavailable */ -import { Entry } from '@napi-rs/keyring'; -import { DarwinSecurityEntry } from './darwin-keychain.js'; import fs from 'node:fs'; -import path from 'node:path'; -import os from 'node:os'; -import { logWarn } from '../utils/debug.js'; -import { observeHostFailure } from './host-probe.js'; +import { KeyringStore } from './keyring-store.js'; interface BaseEnvironmentConfig { name: string; @@ -52,224 +44,28 @@ export interface CliConfig { environments: Record; } -const SERVICE_NAME = 'workos-cli'; -const ACCOUNT_NAME = 'config'; - -let fallbackWarningShown = false; -let forceInsecureStorage = false; -let migrationAttempted = false; - -/** - * In-process cache — same rationale as credential-store: getConfig() is - * called several times per run, and each uncached keyring read is a separate - * keychain ACL check (one password dialog per read on an untrusted binary). - * undefined = not loaded yet. - */ -let cachedConfig: CliConfig | null | undefined; +const store = new KeyringStore({ + serviceName: 'workos-cli', + accountName: 'config', + fileName: 'config.json', + label: 'config', + verifySaveReadBack: true, +}); export function setInsecureConfigStorage(value: boolean): void { - forceInsecureStorage = value; - migrationAttempted = false; - cachedConfig = undefined; -} - -function getConfigDir(): string { - return path.join(os.homedir(), '.workos'); -} - -function getConfigFilePath(): string { - return path.join(getConfigDir(), 'config.json'); -} - -function fileExists(): boolean { - return fs.existsSync(getConfigFilePath()); -} - -function readFromFile(): CliConfig | null { - if (!fileExists()) return null; - const filePath = getConfigFilePath(); - try { - const content = fs.readFileSync(filePath, 'utf-8'); - return JSON.parse(content); - } catch (error) { - observeHostFailure('home-fs', error, { - operation: 'read', - target: filePath, - label: 'config fallback file', - }); - logWarn('Failed to read config file:', error); - return null; - } -} - -function writeToFile(config: CliConfig): void { - const dir = getConfigDir(); - const filePath = getConfigFilePath(); - try { - if (!fs.existsSync(dir)) { - fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); - } - fs.writeFileSync(filePath, JSON.stringify(config, null, 2), { - mode: 0o600, - }); - } catch (error) { - observeHostFailure('home-fs', error, { - operation: 'write', - target: filePath, - label: 'config fallback file', - }); - throw error; - } -} - -function deleteFile(): void { - const filePath = getConfigFilePath(); - if (fileExists()) { - try { - fs.unlinkSync(filePath); - } catch (error) { - observeHostFailure('home-fs', error, { - operation: 'delete', - target: filePath, - label: 'config fallback file', - }); - throw error; - } - } -} - -interface KeyringEntry { - getPassword(): string | null; - setPassword(password: string): void; - deletePassword(): void; -} - -function getKeyringEntry(): KeyringEntry { - // Same backend selection as credential-store: on macOS go through - // /usr/bin/security so keychain trust survives across ad-hoc-signed - // releases. See darwin-keychain.ts. - if (process.platform === 'darwin') { - return new DarwinSecurityEntry(SERVICE_NAME, ACCOUNT_NAME); - } - return new Entry(SERVICE_NAME, ACCOUNT_NAME); -} - -function readFromKeyring(): CliConfig | null { - try { - const entry = getKeyringEntry(); - const data = entry.getPassword(); - if (!data) return null; - return JSON.parse(data); - } catch (error) { - logWarn('Failed to read config from keyring:', error); - observeHostFailure('keychain', error, { - operation: 'read', - target: `${SERVICE_NAME}/${ACCOUNT_NAME}`, - label: 'config keychain entry', - }); - return null; - } -} - -function writeToKeyring(config: CliConfig): boolean { - try { - const entry = getKeyringEntry(); - entry.setPassword(JSON.stringify(config)); - return true; - } catch (error) { - logWarn('Failed to write config to keyring:', error); - observeHostFailure('keychain', error, { - operation: 'write', - target: `${SERVICE_NAME}/${ACCOUNT_NAME}`, - label: 'config keychain entry', - }); - return false; - } -} - -function deleteFromKeyring(): void { - try { - const entry = getKeyringEntry(); - entry.deletePassword(); - } catch (error) { - const msg = error instanceof Error ? error.message : String(error); - if (!msg.includes('not found') && !msg.includes('No such')) { - logWarn('Failed to delete config from keyring:', error); - observeHostFailure('keychain', error, { - operation: 'delete', - target: `${SERVICE_NAME}/${ACCOUNT_NAME}`, - label: 'config keychain entry', - }); - } - } -} - -function showFallbackWarning(): void { - if (fallbackWarningShown || forceInsecureStorage) return; - fallbackWarningShown = true; - logWarn( - 'Unable to store config in system keyring. Using file storage.', - 'Config saved to ~/.workos/config.json', - 'Use --insecure-storage to suppress this warning.', - ); + store.setInsecure(value); } export function getConfig(): CliConfig | null { - if (cachedConfig !== undefined) return cachedConfig; - - if (forceInsecureStorage) { - cachedConfig = readFromFile(); - return cachedConfig; - } - - const keyringConfig = readFromKeyring(); - if (keyringConfig) { - cachedConfig = keyringConfig; - return keyringConfig; - } - - const fileConfig = readFromFile(); - if (fileConfig) { - if (!migrationAttempted) { - migrationAttempted = true; - writeToKeyring(fileConfig); - } - cachedConfig = fileConfig; - return fileConfig; - } - - cachedConfig = null; - return null; + return store.get(); } export function saveConfig(config: CliConfig): void { - if (forceInsecureStorage) { - writeToFile(config); - cachedConfig = config; - return; - } - - if (!writeToKeyring(config)) { - showFallbackWarning(); - writeToFile(config); - cachedConfig = config; - return; - } - - // Verify the keyring write is readable (guards against silent keyring failures - // where setPassword succeeds but getPassword returns null in the same process) - if (!readFromKeyring()) { - logWarn('Keyring write succeeded but read-back failed — falling back to file'); - writeToFile(config); - } - cachedConfig = config; + store.save(config); } export function clearConfig(): void { - deleteFromKeyring(); - deleteFile(); - migrationAttempted = false; - cachedConfig = undefined; + store.clear(); } export function getActiveEnvironment(): EnvironmentConfig | null { @@ -300,7 +96,7 @@ export function freshEnvKey(config: CliConfig, base: string): string { } export function getConfigPath(): string { - return getConfigFilePath(); + return store.filePath; } /** @@ -308,8 +104,8 @@ export function getConfigPath(): string { */ export function diagnoseConfig(): string[] { const lines: string[] = []; - const filePath = getConfigFilePath(); - const filePresent = fileExists(); + const filePath = store.filePath; + const filePresent = fs.existsSync(filePath); lines.push(`file: ${filePath} (exists=${filePresent})`); @@ -325,8 +121,7 @@ export function diagnoseConfig(): string[] { } try { - const entry = getKeyringEntry(); - const data = entry.getPassword(); + const data = store.readKeyringRaw(); if (data) { const parsed = JSON.parse(data) as Partial; const envCount = parsed.environments ? Object.keys(parsed.environments).length : 0; @@ -338,7 +133,7 @@ export function diagnoseConfig(): string[] { lines.push(`keyring: error — ${e instanceof Error ? e.message : String(e)}`); } - lines.push(`insecureStorage=${forceInsecureStorage}`); + lines.push(`insecureStorage=${store.insecure}`); return lines; } diff --git a/src/lib/credential-store.ts b/src/lib/credential-store.ts index 6e294870..422fb5a9 100644 --- a/src/lib/credential-store.ts +++ b/src/lib/credential-store.ts @@ -1,18 +1,14 @@ /** - * Credential storage abstraction with keyring support and file fallback. + * Credential storage: keychain-backed with file fallback, via KeyringStore + * (backend selection, in-process cache, migration — see keyring-store.ts). * * Storage priority: * 1. If --insecure-storage: use file only * 2. Try keyring, fall back to file with warning if unavailable */ -import { Entry } from '@napi-rs/keyring'; -import { DarwinSecurityEntry } from './darwin-keychain.js'; import fs from 'node:fs'; -import path from 'node:path'; -import os from 'node:os'; -import { logWarn } from '../utils/debug.js'; -import { observeHostFailure } from './host-probe.js'; +import { KeyringStore } from './keyring-store.js'; export interface StagingCache { clientId: string; @@ -47,252 +43,32 @@ function isValidCredentials(value: unknown): value is Credentials { ); } -const SERVICE_NAME = 'workos-cli'; -const ACCOUNT_NAME = 'credentials'; - -let fallbackWarningShown = false; -let forceInsecureStorage = false; -let migrationAttempted = false; - -/** - * In-process cache: a single CLI run reads credentials from many call sites - * (auth, telemetry, token refresh, ...). Each uncached keyring read is a - * separate keychain ACL check — on macOS with an untrusted binary that means - * one password dialog PER READ. Cache the first result; saves keep it - * coherent (all refresh paths write through saveCredentials in-process). - * undefined = not loaded yet. - */ -let cachedCreds: Credentials | null | undefined; +const store = new KeyringStore({ + serviceName: 'workos-cli', + accountName: 'credentials', + fileName: 'credentials.json', + label: 'credentials', + validate: (parsed) => (isValidCredentials(parsed) ? parsed : null), +}); export function setInsecureStorage(value: boolean): void { - forceInsecureStorage = value; - migrationAttempted = false; - cachedCreds = undefined; -} - -function getCredentialsDir(): string { - return path.join(os.homedir(), '.workos'); -} - -function getCredentialsPath(): string { - return path.join(getCredentialsDir(), 'credentials.json'); -} - -function fileExists(): boolean { - return fs.existsSync(getCredentialsPath()); -} - -function readFromFile(): Credentials | null { - if (!fileExists()) return null; - const filePath = getCredentialsPath(); - try { - const content = fs.readFileSync(filePath, 'utf-8'); - const parsed: unknown = JSON.parse(content); - if (!isValidCredentials(parsed)) { - logWarn('[credential-store] file: stored credentials are missing required fields; treating as logged out'); - return null; - } - return parsed; - } catch (error) { - observeHostFailure('home-fs', error, { - operation: 'read', - target: filePath, - label: 'credential fallback file', - }); - logWarn('Failed to read credentials file:', error); - return null; - } -} - -function writeToFile(creds: Credentials): void { - const dir = getCredentialsDir(); - const filePath = getCredentialsPath(); - try { - if (!fs.existsSync(dir)) { - fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); - } - fs.writeFileSync(filePath, JSON.stringify(creds, null, 2), { - mode: 0o600, - }); - } catch (error) { - observeHostFailure('home-fs', error, { - operation: 'write', - target: filePath, - label: 'credential fallback file', - }); - throw error; - } -} - -function deleteFile(): void { - const filePath = getCredentialsPath(); - if (fileExists()) { - try { - fs.unlinkSync(filePath); - } catch (error) { - observeHostFailure('home-fs', error, { - operation: 'delete', - target: filePath, - label: 'credential fallback file', - }); - throw error; - } - } -} - -interface KeyringEntry { - getPassword(): string | null; - setPassword(password: string): void; - deletePassword(): void; -} - -function getKeyringEntry(): KeyringEntry { - // On macOS, go through /usr/bin/security (stable Apple-signed binary) - // instead of the native binding: the ad-hoc-signed CLI binary changes - // signature every release, so native keychain access prompts per version. - // See darwin-keychain.ts; revert once releases are Developer ID signed. - if (process.platform === 'darwin') { - return new DarwinSecurityEntry(SERVICE_NAME, ACCOUNT_NAME); - } - return new Entry(SERVICE_NAME, ACCOUNT_NAME); -} - -function readFromKeyring(): Credentials | null { - try { - const entry = getKeyringEntry(); - const data = entry.getPassword(); - if (!data) { - logWarn('[credential-store] keyring: entry exists but data is null/empty'); - return null; - } - const parsed: unknown = JSON.parse(data); - if (!isValidCredentials(parsed)) { - logWarn('[credential-store] keyring: stored credentials are missing required fields; treating as logged out'); - return null; - } - return parsed; - } catch (error) { - const msg = error instanceof Error ? error.message : String(error); - logWarn(`[credential-store] keyring read failed: ${msg}`); - observeHostFailure('keychain', error, { - operation: 'read', - target: `${SERVICE_NAME}/${ACCOUNT_NAME}`, - label: 'credential keychain entry', - }); - return null; - } -} - -function writeToKeyring(creds: Credentials): boolean { - try { - const entry = getKeyringEntry(); - entry.setPassword(JSON.stringify(creds)); - return true; - } catch (error) { - const msg = error instanceof Error ? error.message : String(error); - logWarn(`[credential-store] keyring write failed: ${msg}`); - observeHostFailure('keychain', error, { - operation: 'write', - target: `${SERVICE_NAME}/${ACCOUNT_NAME}`, - label: 'credential keychain entry', - }); - return false; - } -} - -function deleteFromKeyring(): void { - try { - const entry = getKeyringEntry(); - entry.deletePassword(); - } catch (error) { - const msg = error instanceof Error ? error.message : String(error); - if (!msg.includes('not found') && !msg.includes('No such')) { - logWarn('Failed to delete from keyring:', error); - observeHostFailure('keychain', error, { - operation: 'delete', - target: `${SERVICE_NAME}/${ACCOUNT_NAME}`, - label: 'credential keychain entry', - }); - } - } -} - -function showFallbackWarning(): void { - if (fallbackWarningShown || forceInsecureStorage) return; - fallbackWarningShown = true; - logWarn( - 'Unable to store credentials in system keyring. Using file storage.', - 'Credentials saved to ~/.workos/credentials.json', - 'Use --insecure-storage to suppress this warning.', - ); + store.setInsecure(value); } export function hasCredentials(): boolean { - // Validate rather than just probing for a file/entry: a malformed blob must - // read as logged-out here too, so this never disagrees with getCredentials(). - // (readFrom* both run isValidCredentials; avoids getCredentials()'s keyring - // migration side effect.) - if (cachedCreds !== undefined) return cachedCreds !== null; - if (forceInsecureStorage) { - return readFromFile() !== null; - } - const keyringCreds = readFromKeyring(); - if (keyringCreds) { - // Safe to cache: getCredentials() would return this without migrating. - // A file-only hit is NOT cached so its migration still runs there. - cachedCreds = keyringCreds; - return true; - } - return readFromFile() !== null; + return store.has(); } export function getCredentials(): Credentials | null { - if (cachedCreds !== undefined) return cachedCreds; - - if (forceInsecureStorage) { - cachedCreds = readFromFile(); - return cachedCreds; - } - - const keyringCreds = readFromKeyring(); - if (keyringCreds) { - cachedCreds = keyringCreds; - return keyringCreds; - } - - const fileCreds = readFromFile(); - if (fileCreds) { - if (!migrationAttempted) { - migrationAttempted = true; - writeToKeyring(fileCreds); - } - cachedCreds = fileCreds; - return fileCreds; - } - - cachedCreds = null; - return null; + return store.get(); } export function saveCredentials(creds: Credentials): void { - if (forceInsecureStorage) { - writeToFile(creds); - cachedCreds = creds; - return; - } - - if (!writeToKeyring(creds)) { - showFallbackWarning(); - writeToFile(creds); - } - cachedCreds = creds; + store.save(creds); } export function clearCredentials(): void { - deleteFromKeyring(); - deleteFile(); - migrationAttempted = false; - cachedCreds = undefined; + store.clear(); } export function updateTokens(accessToken: string, expiresAt: number, refreshToken?: string): void { @@ -316,8 +92,8 @@ export function updateTokens(accessToken: string, expiresAt: number, refreshToke */ export function diagnoseCredentials(): string[] { const lines: string[] = []; - const filePath = getCredentialsPath(); - const filePresent = fileExists(); + const filePath = store.filePath; + const filePresent = fs.existsSync(filePath); lines.push(`file: ${filePath} (exists=${filePresent})`); @@ -335,8 +111,7 @@ export function diagnoseCredentials(): string[] { } try { - const entry = getKeyringEntry(); - const data = entry.getPassword(); + const data = store.readKeyringRaw(); if (data) { const parsed = JSON.parse(data) as Partial; const expired = parsed.expiresAt ? Date.now() >= parsed.expiresAt : 'unknown'; @@ -350,8 +125,10 @@ export function diagnoseCredentials(): string[] { lines.push(`keyring: error — ${e instanceof Error ? e.message : String(e)}`); } - lines.push(`insecureStorage=${forceInsecureStorage}`); + lines.push(`insecureStorage=${store.insecure}`); return lines; } -export { getCredentialsPath }; +export function getCredentialsPath(): string { + return store.filePath; +} diff --git a/src/lib/keyring-store.ts b/src/lib/keyring-store.ts new file mode 100644 index 00000000..041b1140 --- /dev/null +++ b/src/lib/keyring-store.ts @@ -0,0 +1,284 @@ +/** + * Shared machinery for keychain-backed stores with file fallback + * (credential-store and config-store). + * + * Behavior per store instance: + * - Backend: @napi-rs/keyring everywhere except macOS, which routes through + * /usr/bin/security so keychain trust survives across ad-hoc-signed + * releases (see darwin-keychain.ts). + * - In-process cache: values are read from many call sites per run; each + * uncached keyring read is a separate keychain ACL check — one password + * dialog per read on an untrusted binary. First read is cached; saves keep + * it coherent. + * - File fallback with a one-time warning when the keyring is unavailable, + * and one-shot migration of file contents back into the keyring. + */ + +import { Entry } from '@napi-rs/keyring'; +import { DarwinSecurityEntry } from './darwin-keychain.js'; +import fs from 'node:fs'; +import path from 'node:path'; +import os from 'node:os'; +import { logWarn } from '../utils/debug.js'; +import { observeHostFailure } from './host-probe.js'; + +interface KeyringEntryLike { + getPassword(): string | null; + setPassword(password: string): void; + deletePassword(): void; +} + +export interface KeyringStoreOptions { + serviceName: string; + accountName: string; + /** Fallback file name under ~/.workos, e.g. 'credentials.json'. */ + fileName: string; + /** Human noun for log messages and host-probe labels, e.g. 'credentials'. */ + label: string; + /** + * Validate/narrow a parsed blob; return null to treat as absent (a + * malformed blob must read as logged-out/unset, not crash callers). + * Default: accept as-is. + */ + validate?: (parsed: unknown) => T | null; + /** + * After a keyring write, read it back and fall back to file if unreadable + * (guards against keyrings where setPassword succeeds but getPassword + * returns null in the same process). + */ + verifySaveReadBack?: boolean; +} + +export class KeyringStore { + private cached: T | null | undefined; + private forceInsecure = false; + private migrationAttempted = false; + private fallbackWarningShown = false; + + constructor(private readonly opts: KeyringStoreOptions) {} + + get filePath(): string { + return path.join(os.homedir(), '.workos', this.opts.fileName); + } + + setInsecure(value: boolean): void { + this.forceInsecure = value; + this.migrationAttempted = false; + this.cached = undefined; + } + + get insecure(): boolean { + return this.forceInsecure; + } + + /** Cached read: keyring first, then file (with one-shot keyring migration). */ + get(): T | null { + if (this.cached !== undefined) return this.cached; + + if (this.forceInsecure) { + this.cached = this.readFromFile(); + return this.cached; + } + + const keyringValue = this.readFromKeyring(); + if (keyringValue) { + this.cached = keyringValue; + return keyringValue; + } + + const fileValue = this.readFromFile(); + if (fileValue) { + if (!this.migrationAttempted) { + this.migrationAttempted = true; + this.writeToKeyring(fileValue); + } + this.cached = fileValue; + return fileValue; + } + + this.cached = null; + return null; + } + + /** + * Presence check without get()'s migration side effect. A keyring hit is + * cached (get() would return it unchanged); a file-only hit is NOT cached + * so get()'s migration still runs. + */ + has(): boolean { + if (this.cached !== undefined) return this.cached !== null; + if (this.forceInsecure) return this.readFromFile() !== null; + const keyringValue = this.readFromKeyring(); + if (keyringValue) { + this.cached = keyringValue; + return true; + } + return this.readFromFile() !== null; + } + + save(value: T): void { + if (this.forceInsecure) { + this.writeToFile(value); + this.cached = value; + return; + } + + if (!this.writeToKeyring(value)) { + this.showFallbackWarning(); + this.writeToFile(value); + this.cached = value; + return; + } + + if (this.opts.verifySaveReadBack && !this.readFromKeyring()) { + logWarn('Keyring write succeeded but read-back failed — falling back to file'); + this.writeToFile(value); + } + this.cached = value; + } + + clear(): void { + this.deleteFromKeyring(); + this.deleteFile(); + this.migrationAttempted = false; + this.cached = undefined; + } + + /** Direct, uncached keyring read for diagnostics. Throws on backend errors. */ + readKeyringRaw(): string | null { + return this.getKeyringEntry().getPassword(); + } + + private validate(parsed: unknown): T | null { + return this.opts.validate ? this.opts.validate(parsed) : (parsed as T); + } + + private getKeyringEntry(): KeyringEntryLike { + if (process.platform === 'darwin') { + return new DarwinSecurityEntry(this.opts.serviceName, this.opts.accountName); + } + return new Entry(this.opts.serviceName, this.opts.accountName); + } + + private readFromKeyring(): T | null { + const { serviceName, accountName, label } = this.opts; + try { + const data = this.getKeyringEntry().getPassword(); + if (!data) return null; + const parsed: unknown = JSON.parse(data); + const valid = this.validate(parsed); + if (!valid) { + logWarn(`[keyring-store] keyring: stored ${label} failed validation; treating as absent`); + } + return valid; + } catch (error) { + const msg = error instanceof Error ? error.message : String(error); + logWarn(`[keyring-store] ${label} keyring read failed: ${msg}`); + observeHostFailure('keychain', error, { + operation: 'read', + target: `${serviceName}/${accountName}`, + label: `${label} keychain entry`, + }); + return null; + } + } + + private writeToKeyring(value: T): boolean { + const { serviceName, accountName, label } = this.opts; + try { + this.getKeyringEntry().setPassword(JSON.stringify(value)); + return true; + } catch (error) { + const msg = error instanceof Error ? error.message : String(error); + logWarn(`[keyring-store] ${label} keyring write failed: ${msg}`); + observeHostFailure('keychain', error, { + operation: 'write', + target: `${serviceName}/${accountName}`, + label: `${label} keychain entry`, + }); + return false; + } + } + + private deleteFromKeyring(): void { + const { serviceName, accountName, label } = this.opts; + try { + this.getKeyringEntry().deletePassword(); + } catch (error) { + const msg = error instanceof Error ? error.message : String(error); + if (!msg.includes('not found') && !msg.includes('No such')) { + logWarn(`Failed to delete ${label} from keyring:`, error); + observeHostFailure('keychain', error, { + operation: 'delete', + target: `${serviceName}/${accountName}`, + label: `${label} keychain entry`, + }); + } + } + } + + private readFromFile(): T | null { + const filePath = this.filePath; + if (!fs.existsSync(filePath)) return null; + try { + const content = fs.readFileSync(filePath, 'utf-8'); + const parsed: unknown = JSON.parse(content); + const valid = this.validate(parsed); + if (!valid) { + logWarn(`[keyring-store] file: stored ${this.opts.label} failed validation; treating as absent`); + } + return valid; + } catch (error) { + observeHostFailure('home-fs', error, { + operation: 'read', + target: filePath, + label: `${this.opts.label} fallback file`, + }); + logWarn(`Failed to read ${this.opts.label} file:`, error); + return null; + } + } + + private writeToFile(value: T): void { + const filePath = this.filePath; + try { + const dir = path.dirname(filePath); + if (!fs.existsSync(dir)) { + fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); + } + fs.writeFileSync(filePath, JSON.stringify(value, null, 2), { mode: 0o600 }); + } catch (error) { + observeHostFailure('home-fs', error, { + operation: 'write', + target: filePath, + label: `${this.opts.label} fallback file`, + }); + throw error; + } + } + + private deleteFile(): void { + const filePath = this.filePath; + if (!fs.existsSync(filePath)) return; + try { + fs.unlinkSync(filePath); + } catch (error) { + observeHostFailure('home-fs', error, { + operation: 'delete', + target: filePath, + label: `${this.opts.label} fallback file`, + }); + throw error; + } + } + + private showFallbackWarning(): void { + if (this.fallbackWarningShown || this.forceInsecure) return; + this.fallbackWarningShown = true; + logWarn( + `Unable to store ${this.opts.label} in system keyring. Using file storage.`, + `Saved to ~/.workos/${this.opts.fileName}`, + 'Use --insecure-storage to suppress this warning.', + ); + } +}