From 1a59990cc56a5a7d474dd681636e439d7d5944a9 Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Thu, 5 Mar 2026 10:39:11 -0600 Subject: [PATCH 1/6] fix: make credential and config stores keychain-primary with file fallback Both stores now follow the same pattern: write to system keychain first, only write to file when keychain is unavailable, never proactively delete existing file backups. config-store: removed deleteFile() calls from saveConfig() and getConfig() migration path that caused environment configs to vanish after addon rebuilds or keychain access failures. credential-store: stopped always writing to file alongside keychain. File is now only written as fallback when keychain write fails. --- src/lib/config-store.spec.ts | 148 ++++++++++++++++++++++++++++++- src/lib/config-store.ts | 8 +- src/lib/credential-store.spec.ts | 21 +++-- src/lib/credential-store.ts | 2 +- 4 files changed, 166 insertions(+), 13 deletions(-) diff --git a/src/lib/config-store.spec.ts b/src/lib/config-store.spec.ts index 734dac86..f7f434e6 100644 --- a/src/lib/config-store.spec.ts +++ b/src/lib/config-store.spec.ts @@ -1,5 +1,14 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import { existsSync, readFileSync, unlinkSync, mkdtempSync, rmdirSync, statSync, writeFileSync } from 'node:fs'; +import { + existsSync, + readFileSync, + unlinkSync, + mkdtempSync, + rmdirSync, + statSync, + writeFileSync, + mkdirSync, +} from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; @@ -13,6 +22,44 @@ let testDir: string; let workosDir: string; let configFile: string; +// Mock keyring storage +const mockKeyring = new Map(); +let keyringAvailable = true; + +vi.mock('@napi-rs/keyring', () => ({ + Entry: class MockEntry { + private key: string; + + constructor( + service: string, + private 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(); @@ -36,7 +83,11 @@ describe('config-store', () => { testDir = mkdtempSync(join(tmpdir(), 'config-store-test-')); workosDir = join(testDir, '.workos'); configFile = join(workosDir, 'config.json'); - // Force file-based storage for tests + + // Reset state + mockKeyring.clear(); + keyringAvailable = true; + // Force file-based storage for most tests setInsecureConfigStorage(true); }); @@ -199,4 +250,97 @@ describe('config-store', () => { expect(env?.endpoint).toBe('http://localhost:8001'); }); }); + + describe('keyring storage (default)', () => { + beforeEach(() => { + setInsecureConfigStorage(false); + }); + + it('saves config to keyring', () => { + saveConfig(sampleConfig); + expect(mockKeyring.has('workos-cli:config')).toBe(true); + }); + + it('retrieves config from keyring', () => { + saveConfig(sampleConfig); + const config = getConfig(); + expect(config?.activeEnvironment).toBe('production'); + expect(config?.environments.production.apiKey).toBe('sk_test_abc123'); + }); + + it('does not write file when keyring succeeds', () => { + saveConfig(sampleConfig); + expect(mockKeyring.has('workos-cli:config')).toBe(true); + expect(existsSync(configFile)).toBe(false); + }); + + it('does not delete existing file on keyring success', () => { + // Create a pre-existing file (from a prior fallback write) + mkdirSync(workosDir, { recursive: true }); + writeFileSync(configFile, JSON.stringify(sampleConfig)); + + saveConfig({ ...sampleConfig, activeEnvironment: 'staging' }); + + expect(mockKeyring.has('workos-cli:config')).toBe(true); + expect(existsSync(configFile)).toBe(true); + }); + + it('clears from both keyring and file', () => { + saveConfig(sampleConfig); + mkdirSync(workosDir, { recursive: true }); + writeFileSync(configFile, JSON.stringify(sampleConfig)); + + clearConfig(); + + expect(mockKeyring.has('workos-cli:config')).toBe(false); + expect(existsSync(configFile)).toBe(false); + }); + }); + + describe('file fallback (keyring unavailable)', () => { + beforeEach(() => { + setInsecureConfigStorage(false); + keyringAvailable = false; + }); + + it('falls back to file when keyring unavailable', () => { + saveConfig(sampleConfig); + expect(existsSync(configFile)).toBe(true); + expect(mockKeyring.has('workos-cli:config')).toBe(false); + }); + + it('reads from file when keyring unavailable', () => { + saveConfig(sampleConfig); + const config = getConfig(); + expect(config?.activeEnvironment).toBe('production'); + }); + }); + + describe('migration (file to keyring)', () => { + beforeEach(() => { + setInsecureConfigStorage(false); + }); + + it('migrates file config to keyring on read but keeps file', () => { + mkdirSync(workosDir, { recursive: true }); + writeFileSync(configFile, JSON.stringify(sampleConfig)); + + const config = getConfig(); + + expect(config?.activeEnvironment).toBe('production'); + expect(mockKeyring.has('workos-cli:config')).toBe(true); + expect(existsSync(configFile)).toBe(true); + }); + + it('keeps file if keyring unavailable during migration', () => { + mkdirSync(workosDir, { recursive: true }); + writeFileSync(configFile, JSON.stringify(sampleConfig)); + + keyringAvailable = false; + const config = getConfig(); + + expect(config?.activeEnvironment).toBe('production'); + expect(existsSync(configFile)).toBe(true); + }); + }); }); diff --git a/src/lib/config-store.ts b/src/lib/config-store.ts index f3589082..df2128d1 100644 --- a/src/lib/config-store.ts +++ b/src/lib/config-store.ts @@ -134,8 +134,8 @@ export function getConfig(): CliConfig | null { const fileConfig = readFromFile(); if (fileConfig) { - // Migrate file config to keyring if possible - if (writeToKeyring(fileConfig)) deleteFile(); + // Migrate file config to keyring if possible (keep file as backup) + writeToKeyring(fileConfig); return fileConfig; } @@ -145,9 +145,7 @@ export function getConfig(): CliConfig | null { export function saveConfig(config: CliConfig): void { if (forceInsecureStorage) return writeToFile(config); - if (writeToKeyring(config)) { - deleteFile(); - } else { + if (!writeToKeyring(config)) { showFallbackWarning(); writeToFile(config); } diff --git a/src/lib/credential-store.spec.ts b/src/lib/credential-store.spec.ts index 141201a5..cff20764 100644 --- a/src/lib/credential-store.spec.ts +++ b/src/lib/credential-store.spec.ts @@ -124,10 +124,22 @@ describe('credential-store', () => { expect(creds?.userId).toBe(validCreds.userId); }); - it('keeps file as durable fallback alongside keyring', () => { + it('does not write file when keyring succeeds', () => { saveCredentials(validCreds); - // Both keyring and file should have credentials + // Keyring has credentials, file should NOT be written + expect(mockKeyring.has('workos-cli:credentials')).toBe(true); + expect(existsSync(credentialsFile)).toBe(false); + }); + + it('does not delete existing file on keyring success', () => { + // Create a pre-existing file (from a prior fallback write) + mkdirSync(installerDir, { recursive: true }); + writeFileSync(credentialsFile, JSON.stringify(validCreds)); + + // Save with keyring available — file should remain untouched + saveCredentials({ ...validCreds, accessToken: 'new_token' }); + expect(mockKeyring.has('workos-cli:credentials')).toBe(true); expect(existsSync(credentialsFile)).toBe(true); }); @@ -236,11 +248,10 @@ describe('credential-store', () => { }); it('hasCredentials only checks file when flag is set', () => { - // Save with default mode (writes to both keyring + file) + // Save with insecure mode (writes to file) + setInsecureStorage(true); saveCredentials(validCreds); - // Enable insecure storage — should still find the file - setInsecureStorage(true); expect(hasCredentials()).toBe(true); }); }); diff --git a/src/lib/credential-store.ts b/src/lib/credential-store.ts index 100ce16e..38d20cd0 100644 --- a/src/lib/credential-store.ts +++ b/src/lib/credential-store.ts @@ -155,9 +155,9 @@ export function getCredentials(): Credentials | null { export function saveCredentials(creds: Credentials): void { if (forceInsecureStorage) return writeToFile(creds); - writeToFile(creds); if (!writeToKeyring(creds)) { showFallbackWarning(); + writeToFile(creds); } } From 98095ab3f2d89ee51889a83194b57da1ca661291 Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Thu, 5 Mar 2026 10:43:15 -0600 Subject: [PATCH 2/6] feat: add cross-process file locking for token refresh Prevents race condition where two concurrent CLI processes both attempt to refresh the same token, causing one to get invalid_grant when the server rotates the refresh token. New file-lock utility uses atomic file creation (O_EXCL) with stale lock detection (30s timeout). New refreshAccessTokenSafe() wraps the existing refresh with lock-acquire-then-recheck pattern: after acquiring the lock, re-reads credentials in case another process already refreshed. Falls back to unlocked refresh if lock acquisition fails (degraded but functional). All callers (ensure-auth, background-refresh, credential-proxy) now use the safe variant. --- src/lib/background-refresh.ts | 4 +- src/lib/credential-proxy.ts | 4 +- src/lib/ensure-auth.spec.ts | 4 +- src/lib/ensure-auth.ts | 4 +- src/lib/file-lock.spec.ts | 131 ++++++++++++++++++++++++++++++++ src/lib/file-lock.ts | 121 +++++++++++++++++++++++++++++ src/lib/token-refresh-client.ts | 43 ++++++++++- 7 files changed, 302 insertions(+), 9 deletions(-) create mode 100644 src/lib/file-lock.spec.ts create mode 100644 src/lib/file-lock.ts diff --git a/src/lib/background-refresh.ts b/src/lib/background-refresh.ts index b4c24356..53760197 100644 --- a/src/lib/background-refresh.ts +++ b/src/lib/background-refresh.ts @@ -6,7 +6,7 @@ import { logInfo, logError, logWarn } from '../utils/debug.js'; import { getCredentials, updateTokens } from './credentials.js'; -import { refreshAccessToken, tokenNeedsRefresh } from './token-refresh-client.js'; +import { refreshAccessTokenSafe, tokenNeedsRefresh } from './token-refresh-client.js'; import { analytics } from '../utils/analytics.js'; export interface BackgroundRefreshOptions { @@ -75,7 +75,7 @@ export function startBackgroundRefresh(options: BackgroundRefreshOptions): Backg }); const startTime = Date.now(); - const result = await refreshAccessToken(authkitDomain, clientId); + const result = await refreshAccessTokenSafe(authkitDomain, clientId); if (result.success && result.accessToken && result.expiresAt) { // Update credentials file atomically diff --git a/src/lib/credential-proxy.ts b/src/lib/credential-proxy.ts index 63638bf8..e5dc6883 100644 --- a/src/lib/credential-proxy.ts +++ b/src/lib/credential-proxy.ts @@ -9,7 +9,7 @@ import { URL } from 'node:url'; import { logInfo, logError, logWarn } from '../utils/debug.js'; import { getCredentials, updateTokens, type Credentials } from './credentials.js'; import { analytics } from '../utils/analytics.js'; -import { refreshAccessToken } from './token-refresh-client.js'; +import { refreshAccessTokenSafe } from './token-refresh-client.js'; export interface RefreshConfig { /** AuthKit domain for refresh endpoint */ @@ -68,7 +68,7 @@ async function doRefresh(): Promise { trigger: 'lazy', }); - const result = await refreshAccessToken(authkitDomain, clientId); + const result = await refreshAccessTokenSafe(authkitDomain, clientId); if (result.success && result.accessToken && result.expiresAt) { // Update credentials file atomically diff --git a/src/lib/ensure-auth.spec.ts b/src/lib/ensure-auth.spec.ts index 76a5da24..12b91685 100644 --- a/src/lib/ensure-auth.spec.ts +++ b/src/lib/ensure-auth.spec.ts @@ -67,10 +67,10 @@ vi.mock('../commands/login.js', () => ({ runLogin: () => mockRunLogin(), })); -// Mock refreshAccessToken +// Mock refreshAccessTokenSafe const mockRefreshAccessToken = vi.fn(); vi.mock('./token-refresh-client.js', () => ({ - refreshAccessToken: (...args: unknown[]) => mockRefreshAccessToken(...args), + refreshAccessTokenSafe: (...args: unknown[]) => mockRefreshAccessToken(...args), })); // Import after mocks are set up diff --git a/src/lib/ensure-auth.ts b/src/lib/ensure-auth.ts index f54ced4a..d6d5bfe6 100644 --- a/src/lib/ensure-auth.ts +++ b/src/lib/ensure-auth.ts @@ -3,7 +3,7 @@ */ import { getCredentials, updateTokens, hasCredentials, isTokenExpired, clearCredentials } from './credentials.js'; -import { refreshAccessToken } from './token-refresh-client.js'; +import { refreshAccessTokenSafe } from './token-refresh-client.js'; import { getCliAuthClientId, getAuthkitDomain } from './settings.js'; import { runLogin } from '../commands/login.js'; import { logInfo } from '../utils/debug.js'; @@ -76,7 +76,7 @@ export async function ensureAuthenticated(): Promise { const authkitDomain = getAuthkitDomain(); if (clientId && authkitDomain) { - const refreshResult = await refreshAccessToken(authkitDomain, clientId); + const refreshResult = await refreshAccessTokenSafe(authkitDomain, clientId); if (refreshResult.success && refreshResult.accessToken && refreshResult.expiresAt) { updateTokens(refreshResult.accessToken, refreshResult.expiresAt, refreshResult.refreshToken); diff --git a/src/lib/file-lock.spec.ts b/src/lib/file-lock.spec.ts new file mode 100644 index 00000000..7fc10260 --- /dev/null +++ b/src/lib/file-lock.spec.ts @@ -0,0 +1,131 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { existsSync, unlinkSync, mkdtempSync, rmSync, writeFileSync, mkdirSync, readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +// Mock debug utilities +vi.mock('../utils/debug.js', () => ({ + logInfo: vi.fn(), + logWarn: vi.fn(), +})); + +const { acquireLock } = await import('./file-lock.js'); + +describe('file-lock', () => { + let testDir: string; + let lockPath: string; + + beforeEach(() => { + testDir = mkdtempSync(join(tmpdir(), 'file-lock-test-')); + lockPath = join(testDir, 'test.lock'); + }); + + afterEach(() => { + if (existsSync(lockPath)) unlinkSync(lockPath); + if (existsSync(testDir)) rmSync(testDir, { recursive: true }); + }); + + describe('acquireLock', () => { + it('creates lock file and releases it', async () => { + const lock = await acquireLock({ lockPath }); + + expect(existsSync(lockPath)).toBe(true); + + // Verify lock file contains pid and timestamp + const content = JSON.parse(readFileSync(lockPath, 'utf-8')); + expect(content.pid).toBe(process.pid); + expect(content.timestamp).toBeGreaterThan(0); + + lock.release(); + expect(existsSync(lockPath)).toBe(false); + }); + + it('creates parent directory if missing', async () => { + const nestedLock = join(testDir, 'nested', 'dir', 'test.lock'); + + const lock = await acquireLock({ lockPath: nestedLock }); + expect(existsSync(nestedLock)).toBe(true); + + lock.release(); + }); + + it('second acquire waits until first releases', async () => { + const lock1 = await acquireLock({ lockPath, timeoutMs: 2000 }); + + // Start second acquire in background + let lock2Acquired = false; + const lock2Promise = acquireLock({ lockPath, timeoutMs: 2000, retryIntervalMs: 50 }).then((lock) => { + lock2Acquired = true; + return lock; + }); + + // Give it a tick to try acquiring + await new Promise((resolve) => setTimeout(resolve, 150)); + expect(lock2Acquired).toBe(false); + + // Release first lock + lock1.release(); + + // Second should now acquire + const lock2 = await lock2Promise; + expect(lock2Acquired).toBe(true); + + lock2.release(); + }); + + it('times out when lock is held', async () => { + const lock1 = await acquireLock({ lockPath }); + + await expect(acquireLock({ lockPath, timeoutMs: 200, retryIntervalMs: 50 })).rejects.toThrow( + /Lock acquisition timed out/, + ); + + lock1.release(); + }); + + it('reclaims stale lock', async () => { + // Create a stale lock file manually + mkdirSync(testDir, { recursive: true }); + writeFileSync( + lockPath, + JSON.stringify({ + pid: 99999, + timestamp: Date.now() - 60_000, // 60s ago + }), + ); + + // Should reclaim and acquire + const lock = await acquireLock({ lockPath, staleMs: 30_000 }); + expect(existsSync(lockPath)).toBe(true); + + // Verify it's our lock now + const content = JSON.parse(readFileSync(lockPath, 'utf-8')); + expect(content.pid).toBe(process.pid); + + lock.release(); + }); + + it('does not reclaim non-stale lock', async () => { + // Create a fresh lock file + mkdirSync(testDir, { recursive: true }); + writeFileSync( + lockPath, + JSON.stringify({ + pid: 99999, + timestamp: Date.now(), // just now + }), + ); + + await expect(acquireLock({ lockPath, timeoutMs: 200, staleMs: 30_000, retryIntervalMs: 50 })).rejects.toThrow( + /Lock acquisition timed out/, + ); + }); + + it('release is idempotent', async () => { + const lock = await acquireLock({ lockPath }); + + lock.release(); + expect(() => lock.release()).not.toThrow(); + }); + }); +}); diff --git a/src/lib/file-lock.ts b/src/lib/file-lock.ts new file mode 100644 index 00000000..6c71cf36 --- /dev/null +++ b/src/lib/file-lock.ts @@ -0,0 +1,121 @@ +/** + * File-based advisory lock for cross-process coordination. + * Uses atomic file creation (O_EXCL) to prevent races. + */ + +import fs from 'node:fs'; +import path from 'node:path'; +import { logInfo, logWarn } from '../utils/debug.js'; + +export interface LockOptions { + /** Lock file path */ + lockPath: string; + /** Max time to hold lock before it's considered stale (default: 30000ms) */ + staleMs?: number; + /** How long to wait for lock acquisition (default: 10000ms) */ + timeoutMs?: number; + /** Polling interval while waiting (default: 100ms) */ + retryIntervalMs?: number; +} + +export interface LockHandle { + /** Release the lock */ + release: () => void; +} + +interface LockFileContent { + pid: number; + timestamp: number; +} + +function writeLockFile(lockPath: string): void { + const dir = path.dirname(lockPath); + if (!fs.existsSync(dir)) { + fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); + } + + const content: LockFileContent = { + pid: process.pid, + timestamp: Date.now(), + }; + + const fd = fs.openSync(lockPath, 'wx'); + try { + fs.writeSync(fd, JSON.stringify(content)); + } finally { + fs.closeSync(fd); + } +} + +function readLockFile(lockPath: string): LockFileContent | null { + try { + const content = fs.readFileSync(lockPath, 'utf-8'); + return JSON.parse(content); + } catch { + return null; + } +} + +function isLockStale(lock: LockFileContent, staleMs: number): boolean { + return Date.now() - lock.timestamp > staleMs; +} + +function reclaimStaleLock(lockPath: string): boolean { + try { + fs.unlinkSync(lockPath); + return true; + } catch { + return false; + } +} + +/** + * Acquire an advisory file lock. + * Creates lock file atomically with O_EXCL. + * Detects and reclaims stale locks older than staleMs. + * Throws if lock cannot be acquired within timeoutMs. + */ +export async function acquireLock(options: LockOptions): Promise { + const { lockPath, staleMs = 30_000, timeoutMs = 10_000, retryIntervalMs = 100 } = options; + + const deadline = Date.now() + timeoutMs; + + while (true) { + try { + writeLockFile(lockPath); + logInfo(`[file-lock] Acquired: ${lockPath}`); + + return { + release: () => { + try { + fs.unlinkSync(lockPath); + logInfo(`[file-lock] Released: ${lockPath}`); + } catch { + // Lock file may already be gone (stale reclaim by another process) + } + }, + }; + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'EEXIST') { + throw err; + } + + // Lock file exists — check if stale + const existing = readLockFile(lockPath); + if (existing && isLockStale(existing, staleMs)) { + logWarn(`[file-lock] Reclaiming stale lock (pid=${existing.pid}, age=${Date.now() - existing.timestamp}ms)`); + if (reclaimStaleLock(lockPath)) { + continue; // Retry immediately after reclaim + } + } + + // Check timeout + if (Date.now() >= deadline) { + throw new Error(`Lock acquisition timed out after ${timeoutMs}ms: ${lockPath}`); + } + + // Wait and retry + await new Promise((resolve) => setTimeout(resolve, retryIntervalMs)); + } + } +} diff --git a/src/lib/token-refresh-client.ts b/src/lib/token-refresh-client.ts index ad7fb1c7..8914bf86 100644 --- a/src/lib/token-refresh-client.ts +++ b/src/lib/token-refresh-client.ts @@ -2,8 +2,11 @@ * Token refresh client for WorkOS OAuth */ +import path from 'node:path'; +import os from 'node:os'; import { logInfo, logError } from '../utils/debug.js'; -import { getCredentials } from './credentials.js'; +import { getCredentials, isTokenExpired } from './credentials.js'; +import { acquireLock } from './file-lock.js'; export interface RefreshResult { success: boolean; @@ -121,3 +124,41 @@ export async function refreshAccessToken(authkitDomain: string, clientId: string export function tokenNeedsRefresh(expiresAt: number, thresholdMs: number = 2 * 60 * 1000): boolean { return Date.now() + thresholdMs >= expiresAt; } + +function getRefreshLockPath(): string { + return path.join(os.homedir(), '.workos', 'refresh.lock'); +} + +/** + * Refresh access token with cross-process coordination. + * Acquires file lock before refreshing to prevent races. + * If another process refreshed while we waited, returns the fresh creds. + */ +export async function refreshAccessTokenSafe(authkitDomain: string, clientId: string): Promise { + let lock; + try { + lock = await acquireLock({ lockPath: getRefreshLockPath() }); + } catch (err) { + // Lock acquisition failed — fall back to unlocked refresh (racy but functional) + logError(`[token-refresh] Lock acquisition failed, proceeding without lock: ${(err as Error).message}`); + return refreshAccessToken(authkitDomain, clientId); + } + + try { + // Re-read creds — another process may have refreshed while we waited for the lock + const creds = getCredentials(); + if (creds && !isTokenExpired(creds)) { + logInfo('[token-refresh] Credentials already fresh after acquiring lock (another process refreshed)'); + return { + success: true, + accessToken: creds.accessToken, + expiresAt: creds.expiresAt, + }; + } + + // Still expired — we're the one who refreshes + return await refreshAccessToken(authkitDomain, clientId); + } finally { + lock.release(); + } +} From 4723be15695d227da305fe3bc5ce92686bf62732 Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Thu, 5 Mar 2026 12:44:57 -0600 Subject: [PATCH 3/6] Revert "feat: add cross-process file locking for token refresh" This reverts commit 98095ab3f2d89ee51889a83194b57da1ca661291. --- src/lib/background-refresh.ts | 4 +- src/lib/credential-proxy.ts | 4 +- src/lib/ensure-auth.spec.ts | 4 +- src/lib/ensure-auth.ts | 4 +- src/lib/file-lock.spec.ts | 131 -------------------------------- src/lib/file-lock.ts | 121 ----------------------------- src/lib/token-refresh-client.ts | 43 +---------- 7 files changed, 9 insertions(+), 302 deletions(-) delete mode 100644 src/lib/file-lock.spec.ts delete mode 100644 src/lib/file-lock.ts diff --git a/src/lib/background-refresh.ts b/src/lib/background-refresh.ts index 53760197..b4c24356 100644 --- a/src/lib/background-refresh.ts +++ b/src/lib/background-refresh.ts @@ -6,7 +6,7 @@ import { logInfo, logError, logWarn } from '../utils/debug.js'; import { getCredentials, updateTokens } from './credentials.js'; -import { refreshAccessTokenSafe, tokenNeedsRefresh } from './token-refresh-client.js'; +import { refreshAccessToken, tokenNeedsRefresh } from './token-refresh-client.js'; import { analytics } from '../utils/analytics.js'; export interface BackgroundRefreshOptions { @@ -75,7 +75,7 @@ export function startBackgroundRefresh(options: BackgroundRefreshOptions): Backg }); const startTime = Date.now(); - const result = await refreshAccessTokenSafe(authkitDomain, clientId); + const result = await refreshAccessToken(authkitDomain, clientId); if (result.success && result.accessToken && result.expiresAt) { // Update credentials file atomically diff --git a/src/lib/credential-proxy.ts b/src/lib/credential-proxy.ts index e5dc6883..63638bf8 100644 --- a/src/lib/credential-proxy.ts +++ b/src/lib/credential-proxy.ts @@ -9,7 +9,7 @@ import { URL } from 'node:url'; import { logInfo, logError, logWarn } from '../utils/debug.js'; import { getCredentials, updateTokens, type Credentials } from './credentials.js'; import { analytics } from '../utils/analytics.js'; -import { refreshAccessTokenSafe } from './token-refresh-client.js'; +import { refreshAccessToken } from './token-refresh-client.js'; export interface RefreshConfig { /** AuthKit domain for refresh endpoint */ @@ -68,7 +68,7 @@ async function doRefresh(): Promise { trigger: 'lazy', }); - const result = await refreshAccessTokenSafe(authkitDomain, clientId); + const result = await refreshAccessToken(authkitDomain, clientId); if (result.success && result.accessToken && result.expiresAt) { // Update credentials file atomically diff --git a/src/lib/ensure-auth.spec.ts b/src/lib/ensure-auth.spec.ts index 12b91685..76a5da24 100644 --- a/src/lib/ensure-auth.spec.ts +++ b/src/lib/ensure-auth.spec.ts @@ -67,10 +67,10 @@ vi.mock('../commands/login.js', () => ({ runLogin: () => mockRunLogin(), })); -// Mock refreshAccessTokenSafe +// Mock refreshAccessToken const mockRefreshAccessToken = vi.fn(); vi.mock('./token-refresh-client.js', () => ({ - refreshAccessTokenSafe: (...args: unknown[]) => mockRefreshAccessToken(...args), + refreshAccessToken: (...args: unknown[]) => mockRefreshAccessToken(...args), })); // Import after mocks are set up diff --git a/src/lib/ensure-auth.ts b/src/lib/ensure-auth.ts index d6d5bfe6..f54ced4a 100644 --- a/src/lib/ensure-auth.ts +++ b/src/lib/ensure-auth.ts @@ -3,7 +3,7 @@ */ import { getCredentials, updateTokens, hasCredentials, isTokenExpired, clearCredentials } from './credentials.js'; -import { refreshAccessTokenSafe } from './token-refresh-client.js'; +import { refreshAccessToken } from './token-refresh-client.js'; import { getCliAuthClientId, getAuthkitDomain } from './settings.js'; import { runLogin } from '../commands/login.js'; import { logInfo } from '../utils/debug.js'; @@ -76,7 +76,7 @@ export async function ensureAuthenticated(): Promise { const authkitDomain = getAuthkitDomain(); if (clientId && authkitDomain) { - const refreshResult = await refreshAccessTokenSafe(authkitDomain, clientId); + const refreshResult = await refreshAccessToken(authkitDomain, clientId); if (refreshResult.success && refreshResult.accessToken && refreshResult.expiresAt) { updateTokens(refreshResult.accessToken, refreshResult.expiresAt, refreshResult.refreshToken); diff --git a/src/lib/file-lock.spec.ts b/src/lib/file-lock.spec.ts deleted file mode 100644 index 7fc10260..00000000 --- a/src/lib/file-lock.spec.ts +++ /dev/null @@ -1,131 +0,0 @@ -import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import { existsSync, unlinkSync, mkdtempSync, rmSync, writeFileSync, mkdirSync, readFileSync } from 'node:fs'; -import { join } from 'node:path'; -import { tmpdir } from 'node:os'; - -// Mock debug utilities -vi.mock('../utils/debug.js', () => ({ - logInfo: vi.fn(), - logWarn: vi.fn(), -})); - -const { acquireLock } = await import('./file-lock.js'); - -describe('file-lock', () => { - let testDir: string; - let lockPath: string; - - beforeEach(() => { - testDir = mkdtempSync(join(tmpdir(), 'file-lock-test-')); - lockPath = join(testDir, 'test.lock'); - }); - - afterEach(() => { - if (existsSync(lockPath)) unlinkSync(lockPath); - if (existsSync(testDir)) rmSync(testDir, { recursive: true }); - }); - - describe('acquireLock', () => { - it('creates lock file and releases it', async () => { - const lock = await acquireLock({ lockPath }); - - expect(existsSync(lockPath)).toBe(true); - - // Verify lock file contains pid and timestamp - const content = JSON.parse(readFileSync(lockPath, 'utf-8')); - expect(content.pid).toBe(process.pid); - expect(content.timestamp).toBeGreaterThan(0); - - lock.release(); - expect(existsSync(lockPath)).toBe(false); - }); - - it('creates parent directory if missing', async () => { - const nestedLock = join(testDir, 'nested', 'dir', 'test.lock'); - - const lock = await acquireLock({ lockPath: nestedLock }); - expect(existsSync(nestedLock)).toBe(true); - - lock.release(); - }); - - it('second acquire waits until first releases', async () => { - const lock1 = await acquireLock({ lockPath, timeoutMs: 2000 }); - - // Start second acquire in background - let lock2Acquired = false; - const lock2Promise = acquireLock({ lockPath, timeoutMs: 2000, retryIntervalMs: 50 }).then((lock) => { - lock2Acquired = true; - return lock; - }); - - // Give it a tick to try acquiring - await new Promise((resolve) => setTimeout(resolve, 150)); - expect(lock2Acquired).toBe(false); - - // Release first lock - lock1.release(); - - // Second should now acquire - const lock2 = await lock2Promise; - expect(lock2Acquired).toBe(true); - - lock2.release(); - }); - - it('times out when lock is held', async () => { - const lock1 = await acquireLock({ lockPath }); - - await expect(acquireLock({ lockPath, timeoutMs: 200, retryIntervalMs: 50 })).rejects.toThrow( - /Lock acquisition timed out/, - ); - - lock1.release(); - }); - - it('reclaims stale lock', async () => { - // Create a stale lock file manually - mkdirSync(testDir, { recursive: true }); - writeFileSync( - lockPath, - JSON.stringify({ - pid: 99999, - timestamp: Date.now() - 60_000, // 60s ago - }), - ); - - // Should reclaim and acquire - const lock = await acquireLock({ lockPath, staleMs: 30_000 }); - expect(existsSync(lockPath)).toBe(true); - - // Verify it's our lock now - const content = JSON.parse(readFileSync(lockPath, 'utf-8')); - expect(content.pid).toBe(process.pid); - - lock.release(); - }); - - it('does not reclaim non-stale lock', async () => { - // Create a fresh lock file - mkdirSync(testDir, { recursive: true }); - writeFileSync( - lockPath, - JSON.stringify({ - pid: 99999, - timestamp: Date.now(), // just now - }), - ); - - await expect(acquireLock({ lockPath, timeoutMs: 200, staleMs: 30_000, retryIntervalMs: 50 })).rejects.toThrow( - /Lock acquisition timed out/, - ); - }); - - it('release is idempotent', async () => { - const lock = await acquireLock({ lockPath }); - - lock.release(); - expect(() => lock.release()).not.toThrow(); - }); - }); -}); diff --git a/src/lib/file-lock.ts b/src/lib/file-lock.ts deleted file mode 100644 index 6c71cf36..00000000 --- a/src/lib/file-lock.ts +++ /dev/null @@ -1,121 +0,0 @@ -/** - * File-based advisory lock for cross-process coordination. - * Uses atomic file creation (O_EXCL) to prevent races. - */ - -import fs from 'node:fs'; -import path from 'node:path'; -import { logInfo, logWarn } from '../utils/debug.js'; - -export interface LockOptions { - /** Lock file path */ - lockPath: string; - /** Max time to hold lock before it's considered stale (default: 30000ms) */ - staleMs?: number; - /** How long to wait for lock acquisition (default: 10000ms) */ - timeoutMs?: number; - /** Polling interval while waiting (default: 100ms) */ - retryIntervalMs?: number; -} - -export interface LockHandle { - /** Release the lock */ - release: () => void; -} - -interface LockFileContent { - pid: number; - timestamp: number; -} - -function writeLockFile(lockPath: string): void { - const dir = path.dirname(lockPath); - if (!fs.existsSync(dir)) { - fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); - } - - const content: LockFileContent = { - pid: process.pid, - timestamp: Date.now(), - }; - - const fd = fs.openSync(lockPath, 'wx'); - try { - fs.writeSync(fd, JSON.stringify(content)); - } finally { - fs.closeSync(fd); - } -} - -function readLockFile(lockPath: string): LockFileContent | null { - try { - const content = fs.readFileSync(lockPath, 'utf-8'); - return JSON.parse(content); - } catch { - return null; - } -} - -function isLockStale(lock: LockFileContent, staleMs: number): boolean { - return Date.now() - lock.timestamp > staleMs; -} - -function reclaimStaleLock(lockPath: string): boolean { - try { - fs.unlinkSync(lockPath); - return true; - } catch { - return false; - } -} - -/** - * Acquire an advisory file lock. - * Creates lock file atomically with O_EXCL. - * Detects and reclaims stale locks older than staleMs. - * Throws if lock cannot be acquired within timeoutMs. - */ -export async function acquireLock(options: LockOptions): Promise { - const { lockPath, staleMs = 30_000, timeoutMs = 10_000, retryIntervalMs = 100 } = options; - - const deadline = Date.now() + timeoutMs; - - while (true) { - try { - writeLockFile(lockPath); - logInfo(`[file-lock] Acquired: ${lockPath}`); - - return { - release: () => { - try { - fs.unlinkSync(lockPath); - logInfo(`[file-lock] Released: ${lockPath}`); - } catch { - // Lock file may already be gone (stale reclaim by another process) - } - }, - }; - } catch (err) { - if ((err as NodeJS.ErrnoException).code !== 'EEXIST') { - throw err; - } - - // Lock file exists — check if stale - const existing = readLockFile(lockPath); - if (existing && isLockStale(existing, staleMs)) { - logWarn(`[file-lock] Reclaiming stale lock (pid=${existing.pid}, age=${Date.now() - existing.timestamp}ms)`); - if (reclaimStaleLock(lockPath)) { - continue; // Retry immediately after reclaim - } - } - - // Check timeout - if (Date.now() >= deadline) { - throw new Error(`Lock acquisition timed out after ${timeoutMs}ms: ${lockPath}`); - } - - // Wait and retry - await new Promise((resolve) => setTimeout(resolve, retryIntervalMs)); - } - } -} diff --git a/src/lib/token-refresh-client.ts b/src/lib/token-refresh-client.ts index 8914bf86..ad7fb1c7 100644 --- a/src/lib/token-refresh-client.ts +++ b/src/lib/token-refresh-client.ts @@ -2,11 +2,8 @@ * Token refresh client for WorkOS OAuth */ -import path from 'node:path'; -import os from 'node:os'; import { logInfo, logError } from '../utils/debug.js'; -import { getCredentials, isTokenExpired } from './credentials.js'; -import { acquireLock } from './file-lock.js'; +import { getCredentials } from './credentials.js'; export interface RefreshResult { success: boolean; @@ -124,41 +121,3 @@ export async function refreshAccessToken(authkitDomain: string, clientId: string export function tokenNeedsRefresh(expiresAt: number, thresholdMs: number = 2 * 60 * 1000): boolean { return Date.now() + thresholdMs >= expiresAt; } - -function getRefreshLockPath(): string { - return path.join(os.homedir(), '.workos', 'refresh.lock'); -} - -/** - * Refresh access token with cross-process coordination. - * Acquires file lock before refreshing to prevent races. - * If another process refreshed while we waited, returns the fresh creds. - */ -export async function refreshAccessTokenSafe(authkitDomain: string, clientId: string): Promise { - let lock; - try { - lock = await acquireLock({ lockPath: getRefreshLockPath() }); - } catch (err) { - // Lock acquisition failed — fall back to unlocked refresh (racy but functional) - logError(`[token-refresh] Lock acquisition failed, proceeding without lock: ${(err as Error).message}`); - return refreshAccessToken(authkitDomain, clientId); - } - - try { - // Re-read creds — another process may have refreshed while we waited for the lock - const creds = getCredentials(); - if (creds && !isTokenExpired(creds)) { - logInfo('[token-refresh] Credentials already fresh after acquiring lock (another process refreshed)'); - return { - success: true, - accessToken: creds.accessToken, - expiresAt: creds.expiresAt, - }; - } - - // Still expired — we're the one who refreshes - return await refreshAccessToken(authkitDomain, clientId); - } finally { - lock.release(); - } -} From 84f2498a8eb3f89f5963c9e535251368b48b01aa Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Thu, 5 Mar 2026 12:57:49 -0600 Subject: [PATCH 4/6] fix: don't clear credentials on transient refresh errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only clear credentials on invalid_grant (refresh token permanently dead). Network and server errors are transient — preserve credentials so the user can retry without being forced to re-login. Previously, any refresh failure wiped the keychain, destroying a potentially valid refresh token over a temporary server hiccup. --- src/lib/ensure-auth.spec.ts | 7 +++---- src/lib/ensure-auth.ts | 3 +-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/lib/ensure-auth.spec.ts b/src/lib/ensure-auth.spec.ts index 76a5da24..38f7a9b0 100644 --- a/src/lib/ensure-auth.spec.ts +++ b/src/lib/ensure-auth.spec.ts @@ -304,7 +304,7 @@ describe('ensure-auth', () => { expect(mockRunLogin).toHaveBeenCalledOnce(); }); - it('clears credentials when refresh fails with network error', async () => { + it('preserves credentials when refresh fails with network error', async () => { saveCredentials(expiredAccessCreds); mockRefreshAccessToken.mockResolvedValue({ @@ -313,13 +313,12 @@ describe('ensure-auth', () => { error: 'Network error', }); - // Don't save new creds in login — verify old ones were cleared mockRunLogin.mockImplementation(() => {}); await ensureAuthenticated(); - // Old stale credentials should be gone - expect(hasCredentials()).toBe(false); + // Credentials should be preserved — transient errors shouldn't nuke the session + expect(hasCredentials()).toBe(true); }); it('clears credentials when no refresh token available', async () => { diff --git a/src/lib/ensure-auth.ts b/src/lib/ensure-auth.ts index f54ced4a..e36aee9e 100644 --- a/src/lib/ensure-auth.ts +++ b/src/lib/ensure-auth.ts @@ -98,8 +98,7 @@ export async function ensureAuthenticated(): Promise { return result; } - // Network or server error - clear stale creds and try login as fallback - clearCredentials(); + // Network or server error - keep credentials intact for retry if (isNonInteractiveEnvironment()) { exitWithAuthRequired( `Authentication refresh failed (${refreshResult.errorType}). Run \`workos login\` in an interactive terminal.`, From 28993f9492dbebb8a2a2a7d1497096ae4738d6c9 Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Thu, 5 Mar 2026 14:33:45 -0600 Subject: [PATCH 5/6] fix: eliminate redundant keyring reads and migration retries - ensure-auth: collapse hasCredentials() + getCredentials() two-step into single getCredentials() call, eliminating double keyring read and fixing semantic mismatch (hasCredentials returns true for corrupt files, getCredentials returns null) - credential-store/config-store: add migrationAttempted flag to prevent repeated failed keyring writes on every read when keyring is unavailable (e.g., headless Linux with no keyring daemon) --- src/lib/config-store.ts | 9 +++++++-- src/lib/credential-store.ts | 8 +++++++- src/lib/ensure-auth.ts | 28 ++++++++-------------------- 3 files changed, 22 insertions(+), 23 deletions(-) diff --git a/src/lib/config-store.ts b/src/lib/config-store.ts index df2128d1..4c35f881 100644 --- a/src/lib/config-store.ts +++ b/src/lib/config-store.ts @@ -33,9 +33,11 @@ const ACCOUNT_NAME = 'config'; let fallbackWarningShown = false; let forceInsecureStorage = false; +let migrationAttempted = false; export function setInsecureConfigStorage(value: boolean): void { forceInsecureStorage = value; + migrationAttempted = false; } function getConfigDir(): string { @@ -134,8 +136,10 @@ export function getConfig(): CliConfig | null { const fileConfig = readFromFile(); if (fileConfig) { - // Migrate file config to keyring if possible (keep file as backup) - writeToKeyring(fileConfig); + if (!migrationAttempted) { + migrationAttempted = true; + writeToKeyring(fileConfig); + } return fileConfig; } @@ -154,6 +158,7 @@ export function saveConfig(config: CliConfig): void { export function clearConfig(): void { deleteFromKeyring(); deleteFile(); + migrationAttempted = false; } export function getActiveEnvironment(): EnvironmentConfig | null { diff --git a/src/lib/credential-store.ts b/src/lib/credential-store.ts index 38d20cd0..169c4484 100644 --- a/src/lib/credential-store.ts +++ b/src/lib/credential-store.ts @@ -32,9 +32,11 @@ const ACCOUNT_NAME = 'credentials'; let fallbackWarningShown = false; let forceInsecureStorage = false; +let migrationAttempted = false; export function setInsecureStorage(value: boolean): void { forceInsecureStorage = value; + migrationAttempted = false; } function getCredentialsDir(): string { @@ -145,7 +147,10 @@ export function getCredentials(): Credentials | null { const fileCreds = readFromFile(); if (fileCreds) { - writeToKeyring(fileCreds); + if (!migrationAttempted) { + migrationAttempted = true; + writeToKeyring(fileCreds); + } return fileCreds; } @@ -164,6 +169,7 @@ export function saveCredentials(creds: Credentials): void { export function clearCredentials(): void { deleteFromKeyring(); deleteFile(); + migrationAttempted = false; } export function updateTokens(accessToken: string, expiresAt: number, refreshToken?: string): void { diff --git a/src/lib/ensure-auth.ts b/src/lib/ensure-auth.ts index e36aee9e..8e0be91d 100644 --- a/src/lib/ensure-auth.ts +++ b/src/lib/ensure-auth.ts @@ -2,7 +2,7 @@ * Startup auth guard - ensures valid authentication before command execution. */ -import { getCredentials, updateTokens, hasCredentials, isTokenExpired, clearCredentials } from './credentials.js'; +import { getCredentials, updateTokens, isTokenExpired, clearCredentials } from './credentials.js'; import { refreshAccessToken } from './token-refresh-client.js'; import { getCliAuthClientId, getAuthkitDomain } from './settings.js'; import { runLogin } from '../commands/login.js'; @@ -36,29 +36,17 @@ export async function ensureAuthenticated(): Promise { tokenRefreshed: false, }; - // Case 1: No credentials at all - if (!hasCredentials()) { - if (isNonInteractiveEnvironment()) { - exitWithAuthRequired(); - } - logInfo('[ensure-auth] No credentials found, triggering login'); - await runLogin(); - result.loginTriggered = true; - result.authenticated = hasCredentials(); - return result; - } - + // Case 1: No credentials or invalid credentials const creds = getCredentials(); if (!creds) { - // Credentials file exists but is invalid/empty — clear stale data - clearCredentials(); + clearCredentials(); // Clean up any corrupt/empty files if (isNonInteractiveEnvironment()) { exitWithAuthRequired(); } - logInfo('[ensure-auth] Invalid credentials file, triggering login'); + logInfo('[ensure-auth] No valid credentials found, triggering login'); await runLogin(); result.loginTriggered = true; - result.authenticated = hasCredentials(); + result.authenticated = getCredentials() !== null; return result; } @@ -94,7 +82,7 @@ export async function ensureAuthenticated(): Promise { logInfo('[ensure-auth] Refresh token expired, triggering login'); await runLogin(); result.loginTriggered = true; - result.authenticated = hasCredentials(); + result.authenticated = getCredentials() !== null; return result; } @@ -107,7 +95,7 @@ export async function ensureAuthenticated(): Promise { logInfo(`[ensure-auth] Refresh failed (${refreshResult.errorType}), triggering login`); await runLogin(); result.loginTriggered = true; - result.authenticated = hasCredentials(); + result.authenticated = getCredentials() !== null; return result; } } @@ -120,6 +108,6 @@ export async function ensureAuthenticated(): Promise { logInfo('[ensure-auth] No refresh token, triggering login'); await runLogin(); result.loginTriggered = true; - result.authenticated = hasCredentials(); + result.authenticated = getCredentials() !== null; return result; } From bf9bcdb6f583a79217a1623c1468147de890ca61 Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Thu, 5 Mar 2026 14:51:00 -0600 Subject: [PATCH 6/6] fix: move session refresh message to debug log The "(Session refreshed)" message is implementation detail noise for users. Moved to debug log, keeping only "Already logged in as ..." in user-facing output. --- src/commands/login.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/commands/login.ts b/src/commands/login.ts index 404852b1..5b73276b 100644 --- a/src/commands/login.ts +++ b/src/commands/login.ts @@ -3,6 +3,7 @@ import clack from '../utils/clack.js'; import { saveCredentials, getCredentials, getAccessToken, isTokenExpired, updateTokens } from '../lib/credentials.js'; import { getCliAuthClientId, getAuthkitDomain } from '../lib/settings.js'; import { refreshAccessToken } from '../lib/token-refresh-client.js'; +import { logInfo } from '../utils/debug.js'; /** * Parse JWT payload @@ -88,8 +89,8 @@ export async function runLogin(): Promise { const result = await refreshAccessToken(authkitDomain, clientId); if (result.accessToken && result.expiresAt) { updateTokens(result.accessToken, result.expiresAt, result.refreshToken); + logInfo('[login] Session refreshed via refresh token'); clack.log.info(`Already logged in as ${existingCreds.email ?? 'unknown'}`); - clack.log.info('(Session refreshed)'); clack.log.info('Run `workos logout` to log out'); return; }