diff --git a/docs/cli/configuration.md b/docs/cli/configuration.md index dc61dfbb5a..94069a3a43 100644 --- a/docs/cli/configuration.md +++ b/docs/cli/configuration.md @@ -673,6 +673,11 @@ In addition to a project settings file, a project's `.llxprt` directory can cont - **Default:** `false` - **Requires restart:** Yes +- **`security.disableOsKeyring`** (boolean): + - **Description:** Disable use of the OS keyring/keychain for credential storage and use the encrypted file fallback. Can also be set with the LLXPRT_DISABLE_OS_KEYRING=1 environment variable. + - **Default:** `false` + - **Requires restart:** Yes + - **`security.enablePermanentToolApproval`** (boolean): - **Description:** Enable the "Allow for all future sessions" option in tool confirmation dialogs. - **Default:** `false` diff --git a/packages/cli/src/config/postConfigRuntime.ts b/packages/cli/src/config/postConfigRuntime.ts index 3d853691f4..6eb2782862 100644 --- a/packages/cli/src/config/postConfigRuntime.ts +++ b/packages/cli/src/config/postConfigRuntime.ts @@ -14,6 +14,7 @@ import { type Config, type ImageOperationBackend, } from '@vybestack/llxprt-code-core'; +import { setOsKeyringDisabledBySetting } from '@vybestack/llxprt-code-storage'; import { DebugLogger } from '@vybestack/llxprt-code-telemetry'; import { ProfileManager } from '@vybestack/llxprt-code-settings'; import type { @@ -684,6 +685,18 @@ function finalizeMetadata(input: PostConfigInput): void { * Step 17: finalizeMetadata() — seed default disabled tools, store model params, store bootstrap args, log warnings */ export async function finalizeConfig(input: PostConfigInput): Promise { + // Propagate security.disableOsKeyring into the storage package's process-wide + // opt-out (issue #2928 R3.2) BEFORE any profile/auth application. Profile + // auth wiring (applyProfileToRuntime → createProviderKeyStorage().getKey()) + // performs a real SecureStore read during steps 12-13 below, so this MUST run + // first to suppress the OS keyring before that first read — otherwise a user + // who sets security.disableOsKeyring still gets a Keychain prompt at startup. + // The env var LLXPRT_DISABLE_OS_KEYRING=1 is independent and read directly in + // storage, so it keeps working with zero CLI involvement. + setOsKeyringDisabledBySetting( + input.profileSettingsWithTools.security?.disableOsKeyring === true, + ); + // Step 10-11: Set runtime context + re-register provider infra await setupRuntimeContext(input); diff --git a/packages/cli/src/config/settings-schema/schema-extensions.ts b/packages/cli/src/config/settings-schema/schema-extensions.ts index 46192f95f5..97ecd10271 100644 --- a/packages/cli/src/config/settings-schema/schema-extensions.ts +++ b/packages/cli/src/config/settings-schema/schema-extensions.ts @@ -19,6 +19,16 @@ export const EXTENSION_SETTINGS_SCHEMA = { description: 'Disable YOLO mode, even if enabled by a flag.', showInDialog: true, }, + disableOsKeyring: { + type: 'boolean', + label: 'Disable OS Keyring', + category: 'Security', + requiresRestart: true, + default: false, + description: + 'Disable use of the OS keyring/keychain for credential storage and use the encrypted file fallback. Can also be set with the LLXPRT_DISABLE_OS_KEYRING=1 environment variable.', + showInDialog: true, + }, enablePermanentToolApproval: { type: 'boolean', label: 'Allow Permanent Tool Approval', diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 8b0186ea40..d4886a8477 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -177,6 +177,7 @@ describe('Settings Loading and Merging', () => { }); expect(settings.merged.security).toStrictEqual({ disableYoloMode: false, + disableOsKeyring: false, folderTrust: { enabled: false }, auth: {}, blockGitExtensions: false, diff --git a/packages/storage/bunfig.toml b/packages/storage/bunfig.toml index e8eefc50ef..8f0d94954c 100644 --- a/packages/storage/bunfig.toml +++ b/packages/storage/bunfig.toml @@ -1,2 +1,2 @@ [test] -preload = ["../../test-setup/augment-bun-vi.ts", "./test-setup-storage-isolation.ts"] +preload = ["../../test-setup/augment-bun-vi.ts", "./test-setup-storage-isolation.ts", "./test-setup-bun-session-reset.ts"] diff --git a/packages/storage/src/index.ts b/packages/storage/src/index.ts index cc902fddb5..6ccdc1bce5 100644 --- a/packages/storage/src/index.ts +++ b/packages/storage/src/index.ts @@ -44,6 +44,10 @@ export { isSecureStoreError, isRuntimeReplacedError, } from './secure-store/secure-store-errors.js'; +// OS keyring opt-out setter for the CLI settings bridge (issue #2928 R3.2). +// Storage is a low-level package and must not read CLI settings; the CLI +// pushes the resolved setting in here. +export { setOsKeyringDisabledBySetting } from './secure-store/keyring-session-state.js'; export { isKeychainGrantPersistenceBroken, GRANT_NOT_PERSISTING_MESSAGE, diff --git a/packages/storage/src/secure-store/classify-error.ts b/packages/storage/src/secure-store/classify-error.ts new file mode 100644 index 0000000000..cc394f8288 --- /dev/null +++ b/packages/storage/src/secure-store/classify-error.ts @@ -0,0 +1,104 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Dependency-leaf error classifier for OS keyring errors (issue #2928). + * + * Extracted from secure-store.ts so the adapter boundary + * (default-keyring-adapter.ts → createGuardedAdapter) can classify and latch + * on keyring errors WITHOUT importing from secure-store.ts (which would create + * a cycle, since secure-store.ts imports default-keyring-adapter.ts). Imports + * only `SecureStoreErrorCode` / `isSecureStoreError` from secure-store-errors.ts + * (a true dependency leaf), so it introduces no cycles. + * + * @plan PLAN-20260805-ISSUE2928 + * @requirement R1.1, R2.5 + */ + +import { + isSecureStoreError, + type SecureStoreErrorCode, +} from './secure-store-errors.js'; + +function isErrorWithCode(value: unknown): value is { code: string } { + return ( + typeof value === 'object' && + value !== null && + 'code' in value && + typeof value.code === 'string' + ); +} + +/** + * Classifies an unknown thrown value into a SecureStoreErrorCode using message + * heuristics. A SecureStoreError already carries an authoritative code and is + * returned as-is. + * + * Ordering matters: the "access platform storage" check runs BEFORE the + * cancellation/denied checks so a headless "no Secret Service" machine is + * classified UNAVAILABLE (degradable) rather than DENIED (latching). + * + * The cancellation test is narrowly targeted at genuine + * USER cancellation only. A bare `msg.includes('cancel')` would also match + * abort/timeout text such as "request cancelled due to timeout" + * (@napi-rs/keyring accepts an AbortSignal on every method), which would + * irreversibly latch the keyring off for the whole process. Only the macOS + * status names and explicit user-cancellation phrasing match. + * + * @plan PLAN-20260805-ISSUE2928 + * @requirement R1.1 + */ +// Read-path limitation: @napi-rs/keyring (every published version through +// 1.3.0, the latest) does `Ok(self.inner.get_password().ok())` in +// PasswordTask::compute, discarding the OSStatus, so getPassword returns null +// for BOTH a denial and a genuine absence. The classifications below therefore +// only fire on the write, delete-verification and probe paths, which do +// propagate errors. Recovering read-path fidelity requires changing the native +// binding — tracked in issue #3067. +export function classifyError(error: unknown): SecureStoreErrorCode { + // A SecureStoreError already carries an authoritative classification. + // RUNTIME_REPLACED in particular matches none of the message heuristics + // below and would be downgraded to UNAVAILABLE, which the get()/has() + // fallback paths are allowed to swallow — absorbing a terminal error that + // the runtime-replaced invariant requires callers to rethrow. + if (isSecureStoreError(error)) { + return error.code; + } + const msg = + error instanceof Error + ? error.message.toLowerCase() + : String(error).toLowerCase(); + // "Couldn't access platform storage: PermissionDenied" is what the keyring + // crate reports when the machine has no Secret Service at all — a headless + // Linux box, container, ssh session or WSL. Despite the wording it means "no + // credential backend here", not "you lack permission to use one", so it has + // to be classified UNAVAILABLE and degrade to the encrypted file. Checked + // before the generic denied/permission test below, which would otherwise + // match on the substring and turn a routine no-keyring machine into a hard + // error. + if (msg.includes('access platform storage')) return 'UNAVAILABLE'; + // macOS errSecUserCanceled and explicit user-cancellation messages. Narrowly + // targeted: a bare "cancel" substring would also match abort/timeout + // messages like "request cancelled due to timeout", which would irreversibly + // latch the keyring off for the whole process. @napi-rs/keyring accepts an + // AbortSignal on every method, so abort-related text must NOT latch. Only + // genuine USER cancellation matches. `cancell?ed` covers both the US + // "canceled" and the British "cancelled" spellings. + if ( + msg.includes('errsecusercanceled') || + msg.includes('errseccanceled') || + /\buser cancell?ed\b/.test(msg) || + /cancell?ed by the user\b/.test(msg) + ) { + return 'DENIED'; + } + if (msg.includes('locked')) return 'LOCKED'; + if (msg.includes('denied') || msg.includes('permission')) return 'DENIED'; + if (msg.includes('timeout') || msg.includes('timed out')) return 'TIMEOUT'; + if (msg.includes('not found')) return 'NOT_FOUND'; + if (isErrorWithCode(error) && error.code === 'ENOENT') return 'NOT_FOUND'; + return 'UNAVAILABLE'; +} diff --git a/packages/storage/src/secure-store/default-keyring-adapter.ts b/packages/storage/src/secure-store/default-keyring-adapter.ts index 871596a306..6fa39282ac 100644 --- a/packages/storage/src/secure-store/default-keyring-adapter.ts +++ b/packages/storage/src/secure-store/default-keyring-adapter.ts @@ -28,6 +28,10 @@ import type { StorageLogger } from '../types/logger.js'; import { NullStorageLoggerImpl } from '../types/logger.js'; import { isRuntimeReplaced } from './runtime-identity.js'; import { assertRuntimeNotReplaced } from './runtime-replaced-errors.js'; +import { + isOsKeyringSessionDisabled, + noteKeyringError, +} from './keyring-session-state.js'; import { verifyKeyringDelete } from './keyring-delete-verification.js'; import { recordAuthorizedKeyringRead } from './keychain-grant-persistence.js'; import { SecureStoreError } from './secure-store-errors.js'; @@ -63,19 +67,11 @@ function isOsKeyringDisabledForTests(): boolean { return process.env[DISABLE_OS_KEYRING_ENV] === '1'; } -/** - * Production opt-out: when `LLXPRT_DISABLE_OS_KEYRING=1` the factory returns - * null and all credential traffic routes to the encrypted file fallback. This - * is the user-facing recovery lever for the discarded Keychain grant (issue - * #3020). Distinct from the test marker above, which exists for suite - * isolation: this one is shipped, documented, and leaves existing Keychain - * items untouched. - */ -const PROD_DISABLE_OS_KEYRING_ENV = 'LLXPRT_DISABLE_OS_KEYRING'; - -function isOsKeyringDisabled(): boolean { - return process.env[PROD_DISABLE_OS_KEYRING_ENV] === '1'; -} +// The production opt-out (`LLXPRT_DISABLE_OS_KEYRING=1`, the user-facing +// recovery lever for the discarded Keychain grant in issue #3020) now lives in +// keyring-session-state.ts, which owns that env var alongside the +// security.disableOsKeyring setting and the runtime latch. Reading it here too +// would give the same variable two sources of truth. function isErrorWithCode(value: unknown): value is { code: string } { return ( @@ -170,20 +166,59 @@ function resolveKeyringModule(namespace: unknown): KeyringModuleShape | null { } /** - * Wraps an adapter so that every method re-checks the replaced-runtime state - * immediately before entering native code. This guarantees R2 even for - * adapters cached before the transition — the guard fires on every call. + * Throws a SecureStoreError (UNAVAILABLE) when the OS keyring has been latched + * unusable or opted out for this session. Called inside the guarded adapter + * BEFORE entering native code, so zero OS keychain operations occur after the + * transition — including for an adapter a consumer cached before the latch + * (R2.3). + * + * @plan PLAN-20260805-ISSUE2928 + * @requirement R2.3 + */ +function assertKeyringSessionEnabled(): void { + if (isOsKeyringSessionDisabled()) { + throw new SecureStoreError( + 'The OS keyring is disabled for this session (denied/locked earlier, or opted out). No OS keychain operations are permitted.', + 'UNAVAILABLE', + 'Restart LLxprt after unlocking your keyring / granting access, or clear the OS keyring opt-out (security.disableOsKeyring / LLXPRT_DISABLE_OS_KEYRING), to retry.', + ); + } +} + +/** + * Wraps an adapter so that every method: + * 1. re-checks the terminal replaced-runtime state (RUNTIME_REPLACED); + * 2. re-checks the session latch/opt-out (R2.3 — zero native entry after the + * transition, even for adapters cached before the latch); + * 3. routes any thrown native error through {@link noteKeyringError} (the + * single classification + latch chokepoint, R2.1/R2.5) and rethrows it + * unchanged — never swallowed, never altered. + * + * This is the ONE real chokepoint: every consumer's adapter comes from + * {@link createDefaultKeyringAdapter}, so SecureStore, MCP KeychainTokenStorage + * and machine-secret all route through here and cannot bypass the latch. + * + * Exported so tests can wrap an injected counting/raw adapter with the exact + * guard used in production and assert the boundary behavior (zero native entry + * after a latch; errors latch even when the caller swallows them). * * @plan PLAN-20260801-ISSUE2926 + * @plan PLAN-20260805-ISSUE2928 * @requirement R2 */ -function createGuardedAdapter(inner: KeyringAdapter): KeyringAdapter { +export function createGuardedAdapter(inner: KeyringAdapter): KeyringAdapter { const guardedGet = async ( service: string, account: string, ): Promise => { assertRuntimeNotReplaced(); - return inner.getPassword(service, account); + assertKeyringSessionEnabled(); + try { + return await inner.getPassword(service, account); + } catch (error) { + noteKeyringError(error); + throw error; + } }; const guardedSet = async ( service: string, @@ -191,14 +226,26 @@ function createGuardedAdapter(inner: KeyringAdapter): KeyringAdapter { password: string, ): Promise => { assertRuntimeNotReplaced(); - await inner.setPassword(service, account, password); + assertKeyringSessionEnabled(); + try { + await inner.setPassword(service, account, password); + } catch (error) { + noteKeyringError(error); + throw error; + } }; const guardedDelete = async ( service: string, account: string, ): Promise => { assertRuntimeNotReplaced(); - return inner.deletePassword(service, account); + assertKeyringSessionEnabled(); + try { + return await inner.deletePassword(service, account); + } catch (error) { + noteKeyringError(error); + throw error; + } }; const adapter: KeyringAdapter = { getPassword: guardedGet, @@ -209,25 +256,43 @@ function createGuardedAdapter(inner: KeyringAdapter): KeyringAdapter { const innerFind = inner.findCredentials; adapter.findCredentials = async (service: string) => { assertRuntimeNotReplaced(); - return innerFind(service); + assertKeyringSessionEnabled(); + try { + return await innerFind(service); + } catch (error) { + noteKeyringError(error); + throw error; + } }; } return adapter; } -function withFindCredentials( +/** + * Degrades a guarded adapter's findCredentials to return [] on error. Applied + * OUTSIDE the guard so the guard (and therefore {@link noteKeyringError}) sees + * and classifies + latches the error first (R2.1), while SecureStore.list() + * still observes [] and never throws. Without this layer list()'s own + * try/catch would also prevent the throw; kept for observable parity with the + * prior behavior and any other findCredentials caller. + * + * @plan PLAN-20260805-ISSUE2928 + * @requirement R2.1 + */ +function degradeFindCredentialsToEmpty( adapter: KeyringAdapter, - findCredentialsFn: FindCredentialsFunction | undefined, ): KeyringAdapter { - if (findCredentialsFn !== undefined) { - adapter.findCredentials = async (service: string) => { - try { - return await findCredentialsFn(service); - } catch { - return []; - } - }; + if (adapter.findCredentials === undefined) { + return adapter; } + const inner = adapter.findCredentials; + adapter.findCredentials = async (service: string) => { + try { + return await inner(service); + } catch { + return []; + } + }; return adapter; } @@ -259,7 +324,23 @@ export async function createDefaultKeyringAdapter(): Promise { it('listKeys deduplicates across keyring and fallback', async () => { const mockKeyring = createMockKeyring(); - // Create a SecureStore where keyring will fail after initial set + // Create a SecureStore where keyring will fail after initial set. + // Uses a TIMEOUT error (transient, non-latching): since issue #2928 a + // LOCKED/DENIED classification latches the keyring unusable for the whole + // process, which would prevent the "restore keyring" phase below from + // re-reading the keyring. let shouldFailKeyring = false; const flakyKeyring: KeyringAdapter = { getPassword: async (service, account) => { - if (shouldFailKeyring) throw new Error('keyring locked'); + if (shouldFailKeyring) throw new Error('The operation timed out.'); return mockKeyring.getPassword(service, account); }, setPassword: async (service, account, password) => { - if (shouldFailKeyring) throw new Error('keyring locked'); + if (shouldFailKeyring) throw new Error('The operation timed out.'); return mockKeyring.setPassword(service, account, password); }, deletePassword: async (service, account) => { - if (shouldFailKeyring) throw new Error('keyring locked'); + if (shouldFailKeyring) throw new Error('The operation timed out.'); return mockKeyring.deletePassword(service, account); }, findCredentials: async (service) => { - if (shouldFailKeyring) throw new Error('keyring locked'); + if (shouldFailKeyring) throw new Error('The operation timed out.'); return mockKeyring.findCredentials!(service); }, }; diff --git a/packages/storage/src/secure-store/secure-store.fallback.test.ts b/packages/storage/src/secure-store/secure-store.fallback.test.ts index 17257da3fd..64c6c5b35d 100644 --- a/packages/storage/src/secure-store/secure-store.fallback.test.ts +++ b/packages/storage/src/secure-store/secure-store.fallback.test.ts @@ -267,17 +267,22 @@ describe('SecureStore — Probe Cache Invalidation', () => { it('consecutive failure counter resets on successful keyring operation', async () => { let shouldFail = false; const mockKeyring = createMockKeyring(); + // Issue #2928: a LOCKED/DENIED error now latches the OS keyring off for the + // whole process. This test exercises the transient consecutive-failure + + // recovery path, so it uses a non-latching TIMEOUT error rather than a + // "locked" one (which would latch and make recovery unreachable). + const transientMessage = 'The keyring operation timed out'; const adapter: KeyringAdapter = { getPassword: async (service, account) => { - if (shouldFail) throw new Error('Keyring temporarily locked'); + if (shouldFail) throw new Error(transientMessage); return mockKeyring.getPassword(service, account); }, setPassword: async (service, account, password) => { - if (shouldFail) throw new Error('Keyring temporarily locked'); + if (shouldFail) throw new Error(transientMessage); return mockKeyring.setPassword(service, account, password); }, deletePassword: async (service, account) => { - if (shouldFail) throw new Error('Keyring temporarily locked'); + if (shouldFail) throw new Error(transientMessage); return mockKeyring.deletePassword(service, account); }, }; diff --git a/packages/storage/src/secure-store/secure-store.ts b/packages/storage/src/secure-store/secure-store.ts index 0e6e119201..77a80c2e70 100644 --- a/packages/storage/src/secure-store/secure-store.ts +++ b/packages/storage/src/secure-store/secure-store.ts @@ -41,6 +41,10 @@ import { createDefaultKeyringAdapter, setKeyringLogger, } from './default-keyring-adapter.js'; +import { + isOsKeyringSessionDisabled, + noteKeyringError, +} from './keyring-session-state.js'; import { CredentialWriteLock } from './credential-write-lock.js'; export { createDefaultKeyringAdapter } from './default-keyring-adapter.js'; @@ -50,6 +54,10 @@ export { forceRuntimeReplacedForTesting, resetRuntimeIdentityForTesting, } from './runtime-identity.js'; +// Re-export the OS keyring session-state probes so tests can reach them +// alongside the runtime-replaced probes (PLAN-20260805-ISSUE2928 R2). +export { resetOsKeyringSessionForTesting } from './keyring-session-state.js'; +export { hasOsKeyringWarningBeenEmitted } from './keyring-session-state.js'; // ─── Error Type (re-exported from dependency-leaf module) ──────────────────── // @@ -65,7 +73,7 @@ export { } from './secure-store-errors.js'; export type { SecureStoreErrorCode } from './secure-store-errors.js'; -import { SecureStoreError, isSecureStoreError } from './secure-store-errors.js'; +import { SecureStoreError } from './secure-store-errors.js'; import type { SecureStoreErrorCode } from './secure-store-errors.js'; // ─── Adapter Interface ─────────────────────────────────────────────────────── @@ -97,44 +105,11 @@ export interface SecureStoreOptions { // ─── Helper Functions ──────────────────────────────────────────────────────── -function isErrorWithCode(value: unknown): value is { code: string } { - return ( - typeof value === 'object' && - value !== null && - 'code' in value && - typeof value.code === 'string' - ); -} - -function classifyError(error: unknown): SecureStoreErrorCode { - // A SecureStoreError already carries an authoritative classification. - // RUNTIME_REPLACED in particular matches none of the message heuristics - // below and would be downgraded to UNAVAILABLE, which the get()/has() - // fallback paths are allowed to swallow — absorbing a terminal error that - // the runtime-replaced invariant requires callers to rethrow. - if (isSecureStoreError(error)) { - return error.code; - } - const msg = - error instanceof Error - ? error.message.toLowerCase() - : String(error).toLowerCase(); - // "Couldn't access platform storage: PermissionDenied" is what the keyring - // crate reports when the machine has no Secret Service at all — a headless - // Linux box, container, ssh session or WSL. Despite the wording it means "no - // credential backend here", not "you lack permission to use one", so it has - // to be classified UNAVAILABLE and degrade to the encrypted file. Checked - // before the generic denied/permission test below, which would otherwise - // match on the substring and turn a routine no-keyring machine into a hard - // error. - if (msg.includes('access platform storage')) return 'UNAVAILABLE'; - if (msg.includes('locked')) return 'LOCKED'; - if (msg.includes('denied') || msg.includes('permission')) return 'DENIED'; - if (msg.includes('timeout') || msg.includes('timed out')) return 'TIMEOUT'; - if (msg.includes('not found')) return 'NOT_FOUND'; - if (isErrorWithCode(error) && error.code === 'ENOENT') return 'NOT_FOUND'; - return 'UNAVAILABLE'; -} +// classifyError lives in classify-error.ts (a dependency leaf) so the adapter +// boundary (default-keyring-adapter.ts → createGuardedAdapter → noteKeyringError) +// can classify and latch on keyring errors without importing from this module +// (which would create a cycle). The shared noteKeyringError in +// keyring-session-state.ts is imported below. function getRemediation(code: SecureStoreErrorCode): string { switch (code) { @@ -159,10 +134,6 @@ function getRemediation(code: SecureStoreErrorCode): string { } } -function isTransientError(error: unknown): boolean { - return classifyError(error) === 'TIMEOUT'; -} - /** * Keyring read classifications that may be safely degraded to the encrypted * fallback file by both `get()` and `has()`. Centralizing this set keeps the @@ -187,6 +158,7 @@ export class SecureStore { private readonly fallbackDir: string; private readonly logger: StorageLogger; private readonly machineSecretLoaderFn: () => Promise; + private readonly machineSecretLoaderInjected: boolean; private readonly machineSecretFilePath: string | undefined; private readonly lock: CredentialWriteLock; @@ -216,6 +188,8 @@ export class SecureStore { this.logger = options?.logger ?? new NullStorageLoggerImpl(); this.machineSecretLoaderFn = options?.machineSecretLoader ?? this.defaultMachineSecretLoader; + this.machineSecretLoaderInjected = + options?.machineSecretLoader !== undefined; this.machineSecretFilePath = options?.machineSecretPath; this.lock = new CredentialWriteLock({ lockDir: options?.lockDir ?? Storage.getCredentialLocksDir(), @@ -229,9 +203,88 @@ export class SecureStore { filePath: this.machineSecretFilePath, }); + /** + * Machine-secret resolution for the fallback WRITE path. + * + * While the OS keyring is disabled (latched or opted out) a keychain-resident + * machine secret may exist but be unreachable. Generating a replacement would + * silently orphan every existing v:2 envelope sealed under the real one, and + * the newly written envelopes would in turn become unreadable on the next + * healthy start when the keychain secret comes back. So while disabled we + * resolve read-only, and only permit minting a fresh secret when no v:2 + * envelope exists to orphan. + * + * @plan PLAN-20260805-ISSUE2928 + * @requirement R3.4 + */ + private async loadMachineSecretForWrite(): Promise { + if (this.machineSecretLoaderInjected || !isOsKeyringSessionDisabled()) { + return this.machineSecretLoaderFn(); + } + const existing = await this.loadMachineSecretForRead(); + if (existing !== null) { + return existing; + } + if (await this.hasAnyV2FallbackFile()) { + throw new SecureStoreError( + 'Refusing to generate a replacement machine secret while the OS keyring is disabled and v:2 fallback files exist: doing so would permanently orphan them.', + 'UNAVAILABLE', + 'Re-enable the OS keyring (unset LLXPRT_DISABLE_OS_KEYRING / security.disableOsKeyring) and re-save this key so the existing machine secret is used, or delete the orphaned .enc files and re-authenticate.', + ); + } + return this.machineSecretLoaderFn(); + } + + /** Whether any v:2 envelope exists in this store's fallback directory. */ + private async hasAnyV2FallbackFile(): Promise { + let files: string[]; + try { + files = await fs.readdir(this.fallbackDir); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return false; + throw error; + } + for (const file of files) { + if (!file.endsWith('.enc')) continue; + const version = await this.readExistingEnvelopeVersion( + path.join(this.fallbackDir, file), + ); + if (version === 2) return true; + } + return false; + } + + /** + * Read-only machine-secret resolution for the decrypt path (R3.5). Never + * mints a new secret as a side effect of a read: when the default loader is + * in use, resolve via getMachineSecret with generateIfMissing:false so a + * missing secret fails closed instead of orphaning existing v:2 envelopes. + * An injected loader is honored as-is (the injection contract governs it). + * + * @plan PLAN-20260805-ISSUE2928 + * @requirement R3.5 + */ + private async loadMachineSecretForRead(): Promise { + if (this.machineSecretLoaderInjected) { + return this.machineSecretLoaderFn(); + } + return getMachineSecret({ + filePath: this.machineSecretFilePath, + generateIfMissing: false, + }); + } + // ─── Keyring Loading ────────────────────────────────────────────────────── private async getKeyring(): Promise { + // R2.3: once the OS keyring is latched unusable (or opted out) for this + // process, return null IMMEDIATELY — before touching keyringLoadAttempted + // and before invoking the loader. This holds even for an instance that + // had already cached an adapter, so zero keyring operations occur after + // the transition. + if (isOsKeyringSessionDisabled()) { + return null; + } if (this.keyringLoadAttempted) return this.keyringInstance ?? null; this.keyringLoadAttempted = true; try { @@ -316,6 +369,15 @@ export class SecureStore { async isKeychainAvailable(): Promise { assertRuntimeNotReplaced(); + // FIX 5 (issue #2928): a latched/opted-out session must report unavailable + // BEFORE the TTL-cached probe result, so a cached `true` cannot survive the + // latch. Without this, a probe taken while the keyring was healthy would + // keep returning true for up to PROBE_TTL_MS after the keyring was latched + // off. + if (isOsKeyringSessionDisabled()) { + this.probeCache = null; + return false; + } if (this.probeCache !== null) { const elapsed = Date.now() - this.probeCache.timestamp; if (elapsed < this.PROBE_TTL_MS) { @@ -352,9 +414,10 @@ export class SecureStore { ); return probeOk; } catch (error) { + const code = noteKeyringError(error); const msg = error instanceof Error ? error.message : String(error); this.logger.debug(() => `[probe] keyring probe failed: ${msg}`); - if (isTransientError(error)) { + if (code === 'TIMEOUT') { this.probeCache = null; } else { this.probeCache = { available: false, timestamp: Date.now() }; @@ -423,12 +486,14 @@ export class SecureStore { const adapter = await this.getKeyring(); let keyringWriteSucceeded = false; let keyringWriteError: unknown = null; + let keyringWriteCode: SecureStoreErrorCode | null = null; if (adapter !== null) { try { await adapter.setPassword(this.serviceName, key, value); keyringWriteSucceeded = true; } catch (error) { keyringWriteError = error; + keyringWriteCode = noteKeyringError(error); this.recordKeyringFailure(); this.logger.debug( () => @@ -477,8 +542,8 @@ export class SecureStore { // Keyring unavailable or write failed. if (this.fallbackPolicy === 'deny') { - if (adapter !== null && keyringWriteError !== null) { - const classified = classifyError(keyringWriteError); + if (keyringWriteCode !== null) { + const classified = keyringWriteCode; const msg = keyringWriteError instanceof Error ? keyringWriteError.message @@ -513,7 +578,7 @@ export class SecureStore { this.logger.debug(() => `[get] key='${key}' → not found in keyring`); } catch (error) { this.recordKeyringFailure(); - const classified = classifyError(error); + const classified = noteKeyringError(error); const msg = error instanceof Error ? error.message : String(error); this.logger.debug( () => @@ -577,7 +642,7 @@ export class SecureStore { this.recordKeyringSuccess(); } catch (error) { keyringFailure = { - code: classifyError(error), + code: noteKeyringError(error), message: error instanceof Error ? error.message : String(error), }; // A thrown NOT_FOUND means the keyring responded correctly but had @@ -699,7 +764,7 @@ export class SecureStore { } } catch (error) { this.recordKeyringFailure(); - const classified = classifyError(error); + const classified = noteKeyringError(error); const msg = error instanceof Error ? error.message : String(error); this.logger.debug( () => @@ -740,7 +805,7 @@ export class SecureStore { const salt = crypto.randomBytes(SALT_LEN); - const machineSecret = await this.machineSecretLoaderFn(); + const machineSecret = await this.loadMachineSecretForWrite(); const useV2 = machineSecret !== null; // Never downgrade an existing v:2 file to v:1 when the machine secret is @@ -892,12 +957,14 @@ export class SecureStore { const encryptedData = ciphertext.subarray(44); let kdfInput: string; if (envelope.v === 2) { - const machineSecret = await this.machineSecretLoaderFn(); + const machineSecret = await this.loadMachineSecretForRead(); if (machineSecret === null) { throw new SecureStoreError( - 'v:2 fallback file requires a machine secret that is unavailable', + 'v:2 fallback file cannot be decrypted: no machine secret is available. ' + + 'The OS keyring may be disabled or unavailable and no machine-secret file exists on disk.', 'CORRUPT', - 'Re-save the key or re-authenticate. The machine secret may have changed or been removed.', + 'Re-enable the OS keyring (security.disableOsKeyring=false) and re-save the key, or ' + + 'restore the machine-secret file on this machine. See the issue #2928 migration notes.', ); } kdfInput = deriveV2KdfInput(this.serviceName, machineSecret); diff --git a/packages/storage/test-bun/secure-store.fallback-hardening.bun.ts b/packages/storage/test-bun/secure-store.fallback-hardening.bun.ts index ac843773f1..81481a33a9 100644 --- a/packages/storage/test-bun/secure-store.fallback-hardening.bun.ts +++ b/packages/storage/test-bun/secure-store.fallback-hardening.bun.ts @@ -551,9 +551,14 @@ describe('SecureStore fallback hardening — consecutive-failure tracking erodes }); it('invalidates the cached probe after 3 consecutive failing delete() calls with a non-NOT_FOUND error', async () => { + // UNAVAILABLE (non-latching, non-NOT_FOUND): since issue #2928 a LOCKED/ + // DENIED classification latches the keyring unusable for the process, + // which would short-circuit getKeyring() before the threshold could ever + // be reached. UNAVAILABLE exercises the consecutive-failure counter + // without latching. const { adapter, probeCount } = makeProbeableAdapter({ deleteThrowsFor: new Set(['fail-delete']), - deleteError: new Error('Keyring locked'), + deleteError: new Error('dbus connection refused'), }); const store = new SecureStore(SERVICE, { fallbackDir: env.dir(), diff --git a/packages/storage/test-bun/secure-store.keyring-session.bun.ts b/packages/storage/test-bun/secure-store.keyring-session.bun.ts new file mode 100644 index 0000000000..8312f6fb8e --- /dev/null +++ b/packages/storage/test-bun/secure-store.keyring-session.bun.ts @@ -0,0 +1,974 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Behavioral tests for issue #2928: distinguish Keychain denial from absence, + * degrade once, and allow an OS keyring opt-out. + * + * R1 — error fidelity: user-cancellation classifies as DENIED (not swallowed + * as UNAVAILABLE); NOT_FOUND/UNAVAILABLE still degrade. Classification narrows the + * cancellation test so abort/timeout text does NOT latch. + * R2 — one process-wide latch enforced at the adapter boundary: the first + * DENIED/LOCKED latches the keyring unusable for the process with exactly + * one stderr warning; TIMEOUT and UNAVAILABLE do not latch; + * RUNTIME_REPLACED is not absorbed and does not latch. + * R3 — explicit opt-out: LLXPRT_DISABLE_OS_KEYRING=1 and the settings + * equivalent disable the keyring; a v:2 envelope written with a + * file-resident machine secret round-trips through disabled mode. + * + * All assertions are on observable behaviour (returned values, thrown codes, + * file contents/modes). A counting adapter is used where "zero keyring + * operations" is itself the specification. + * + * @plan PLAN-20260805-ISSUE2928 + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'bun:test'; +import { promises as fs } from 'node:fs'; +import * as path from 'node:path'; +import * as os from 'node:os'; +import * as crypto from 'node:crypto'; +import { + SecureStore, + SecureStoreError, + createDefaultKeyringAdapter, + type KeyringAdapter, +} from '../src/secure-store/secure-store.js'; +import { createGuardedAdapter } from '../src/secure-store/default-keyring-adapter.js'; +import { + resetOsKeyringSessionForTesting, + hasOsKeyringWarningBeenEmitted, + setOsKeyringDisabledBySetting, + isOsKeyringSessionDisabled, + OS_KEYRING_UNUSABLE_MESSAGE, + OS_KEYRING_UNUSABLE_REMEDIATION, +} from '../src/secure-store/keyring-session-state.js'; +import { runtimeReplacedError } from '../src/secure-store/runtime-replaced-errors.js'; +import { + getMachineSecret, + resetMachineSecretCache, +} from '../src/secure-store/machine-secret.js'; + +const SERVICE = 'keyring-session-2928'; +const OPT_OUT_ENV = 'LLXPRT_DISABLE_OS_KEYRING'; + +// ─── Shared helpers (RULES.md: no copy-pasted setup) ──────────────────────── + +/** + * Wires a temp fallback dir + machine-secret path for the enclosing describe. + * Returns lazy accessors so each describe block gets isolated on-disk state + * with one line of setup. + */ +function useTempDirs(): { + fallbackDir: () => string; + machineSecretPath: () => string; + encExists: (key: string) => Promise; + machineSecretExists: () => Promise; +} { + let tempDir = ''; + beforeEach(async () => { + tempDir = await fs.mkdtemp( + path.join(os.tmpdir(), 'secure-store-keyring-session-'), + ); + resetMachineSecretCache(); + }); + afterEach(async () => { + resetMachineSecretCache(); + if (tempDir) { + await fs.rm(tempDir, { recursive: true, force: true }); + } + }); + return { + fallbackDir: () => path.join(tempDir, 'fallback'), + machineSecretPath: () => path.join(tempDir, 'machine_secret'), + encExists: async (key: string) => + fileExists(path.join(tempDir, 'fallback', `${key}.enc`)), + machineSecretExists: async () => + fileExists(path.join(tempDir, 'machine_secret')), + }; +} + +/** Resets the process-wide OS keyring latch + opt-out between tests. */ +function useResettableSessionState(): { + beforeEach: () => void; + afterEach: () => void; +} { + return { + beforeEach: () => { + resetOsKeyringSessionForTesting(); + setOsKeyringDisabledBySetting(false); + }, + afterEach: () => { + resetOsKeyringSessionForTesting(); + setOsKeyringDisabledBySetting(false); + }, + }; +} + +/** Saves, sets, and restores the opt-out env var around each test. */ +function useOptOutEnv(): { + set: (value: string) => void; + clear: () => void; + beforeEach: () => void; + afterEach: () => void; +} { + let saved: string | undefined; + return { + set: (value: string) => { + process.env[OPT_OUT_ENV] = value; + }, + // `delete`, not assignment: the production check reads + // process.env[VAR] === '1', but leaving the key present with a literal + // 'undefined' would still be observable to any existence check. + clear: () => { + delete process.env[OPT_OUT_ENV]; + }, + beforeEach: () => { + saved = process.env[OPT_OUT_ENV]; + delete process.env[OPT_OUT_ENV]; + }, + afterEach: () => { + if (saved === undefined) delete process.env[OPT_OUT_ENV]; + else process.env[OPT_OUT_ENV] = saved; + }, + }; +} + +async function fileExists(p: string): Promise { + try { + await fs.access(p); + return true; + } catch (error) { + if ( + typeof error === 'object' && + error !== null && + 'code' in error && + error.code === 'ENOENT' + ) { + return false; + } + throw error; + } +} + +/** Adapter whose getPassword throws the supplied error. */ +function adapterThrowingOnGet(error: Error): KeyringAdapter { + return { + getPassword: async () => { + throw error; + }, + setPassword: async () => {}, + deletePassword: async () => false, + }; +} + +/** Adapter whose setPassword throws the supplied error. */ +function adapterThrowingOnSet(error: Error): KeyringAdapter { + return { + getPassword: async () => null, + setPassword: async () => { + throw error; + }, + deletePassword: async () => false, + }; +} + +/** Adapter that counts every call — used only for zero-call assertions. */ +function createCountingAdapter(): { adapter: KeyringAdapter; calls: string[] } { + const calls: string[] = []; + const adapter: KeyringAdapter = { + getPassword: async () => { + calls.push('getPassword'); + return null; + }, + setPassword: async () => { + calls.push('setPassword'); + }, + deletePassword: async () => { + calls.push('deletePassword'); + return false; + }, + }; + return { adapter, calls }; +} + +/** + * Wraps an existing adapter so each native method records its invocation. Used + * to prove that under the opt-out flag the real factory short-circuits and no + * native method is ever reached (if it were, calls would be non-empty). + */ +function wrapWithCallCount( + inner: KeyringAdapter, + calls: string[], +): KeyringAdapter { + return { + getPassword: async (service: string, account: string) => { + calls.push('getPassword'); + return inner.getPassword(service, account); + }, + setPassword: async (service: string, account: string, password: string) => { + calls.push('setPassword'); + await inner.setPassword(service, account, password); + }, + deletePassword: async (service: string, account: string) => { + calls.push('deletePassword'); + return inner.deletePassword(service, account); + }, + }; +} + +/** A fixed 32-byte machine secret for deterministic v:2 envelopes. */ +function fixedMachineSecret(): Buffer { + return crypto.randomBytes(32); +} + +/** Narrows a thrown value to a SecureStoreError or fails the test loudly. */ +function asSecureStoreError(error: unknown): SecureStoreError { + if (error instanceof SecureStoreError) return error; + throw new Error(`Expected SecureStoreError, got: ${String(error)}`); +} + +// ─── R1: error fidelity ───────────────────────────────────────────────────── + +/** + * @plan PLAN-20260805-ISSUE2928 + * @requirement R1.1 + */ +describe('R1 — classifyError fidelity', () => { + const temp = useTempDirs(); + const session = useResettableSessionState(); + beforeEach(session.beforeEach); + afterEach(session.afterEach); + + // Case 1 + it('classifies "User canceled the operation." as DENIED on the read path (not swallowed)', async () => { + const store = new SecureStore(SERVICE, { + keyringLoader: async () => + adapterThrowingOnGet(new Error('User canceled the operation.')), + fallbackDir: temp.fallbackDir(), + fallbackPolicy: 'allow', + machineSecretLoader: async () => null, + }); + + let error: unknown = null; + try { + await store.get('k'); + } catch (caught) { + error = caught; + } + + const e = asSecureStoreError(error); + expect(e.code).toBe('DENIED'); + }); + + // Case 2 + it('classifies errSecUserCanceled-worded and British "Cancelled" variants as DENIED', async () => { + const messages = [ + 'errSecUserCanceled: The operation was canceled.', + 'The user cancelled the request.', + ]; + for (const message of messages) { + resetOsKeyringSessionForTesting(); + const store = new SecureStore(SERVICE, { + keyringLoader: async () => adapterThrowingOnGet(new Error(message)), + fallbackDir: temp.fallbackDir(), + fallbackPolicy: 'allow', + machineSecretLoader: async () => null, + }); + + let error: unknown = null; + try { + await store.get('k'); + } catch (caught) { + error = caught; + } + + const e = asSecureStoreError(error); + expect(e.code).toBe('DENIED'); + } + }); + + // Case 3 + it('a NOT_FOUND-worded read error still degrades to the fallback file (no regression)', async () => { + const dir = temp.fallbackDir(); + const seeder = new SecureStore(SERVICE, { + keyringLoader: async () => null, + fallbackDir: dir, + machineSecretLoader: async () => null, + }); + await seeder.set('seed-key', 'seed-value'); + + const store = new SecureStore(SERVICE, { + keyringLoader: async () => + adapterThrowingOnGet(new Error('The item was not found.')), + fallbackDir: dir, + fallbackPolicy: 'allow', + machineSecretLoader: async () => null, + }); + + const value = await store.get('seed-key'); + expect(value).toBe('seed-value'); + }); + + // Case 4 + it('an UNAVAILABLE ("access platform storage") read error still degrades (no regression)', async () => { + const dir = temp.fallbackDir(); + const seeder = new SecureStore(SERVICE, { + keyringLoader: async () => null, + fallbackDir: dir, + machineSecretLoader: async () => null, + }); + await seeder.set('seed-key', 'seed-value'); + + const store = new SecureStore(SERVICE, { + keyringLoader: async () => + adapterThrowingOnGet( + new Error("Couldn't access platform storage: PermissionDenied"), + ), + fallbackDir: dir, + fallbackPolicy: 'allow', + machineSecretLoader: async () => null, + }); + + const value = await store.get('seed-key'); + expect(value).toBe('seed-value'); + }); + + // Case 2a + it('"The request was cancelled due to timeout." classifies as TIMEOUT, NOT DENIED, and does NOT latch', async () => { + const dir = temp.fallbackDir(); + const store = new SecureStore(SERVICE, { + keyringLoader: async () => + adapterThrowingOnSet( + new Error('The request was cancelled due to timeout.'), + ), + fallbackDir: dir, + fallbackPolicy: 'allow', + machineSecretLoader: async () => null, + }); + + // A write that "times out" must fall back WITHOUT latching the session. + await store.set('k', 'v'); + expect(isOsKeyringSessionDisabled()).toBe(false); + + // And a subsequent op must still reach the adapter (proves no latch). + const { adapter, calls } = createCountingAdapter(); + const store2 = new SecureStore(SERVICE, { + keyringLoader: async () => adapter, + fallbackDir: dir, + machineSecretLoader: async () => null, + }); + await store2.get('k'); + expect(calls.length).toBeGreaterThan(0); + }); + + // Case 2b + it('"User canceled the operation." DOES latch (genuine user cancellation)', async () => { + const dir = temp.fallbackDir(); + const store = new SecureStore(SERVICE, { + keyringLoader: async () => + adapterThrowingOnSet(new Error('User canceled the operation.')), + fallbackDir: dir, + fallbackPolicy: 'allow', + machineSecretLoader: async () => null, + }); + + await store.set('k', 'v'); + expect(isOsKeyringSessionDisabled()).toBe(true); + }); +}); + +// ─── R2: one process-wide latch (enforced at the adapter boundary) ────────── + +/** + * @plan PLAN-20260805-ISSUE2928 + * @requirement R2.1–R2.6 + */ +describe('R2 — process-wide latch', () => { + const temp = useTempDirs(); + const session = useResettableSessionState(); + beforeEach(session.beforeEach); + afterEach(session.afterEach); + + // Case 5 + it('first DENIED from a write latches: a second get() on a NEW SecureStore performs zero adapter calls', async () => { + const dir = temp.fallbackDir(); + const store1 = new SecureStore(SERVICE, { + keyringLoader: async () => + adapterThrowingOnSet( + new Error('The user denied the keychain request.'), + ), + fallbackDir: dir, + fallbackPolicy: 'allow', + machineSecretLoader: async () => null, + }); + await store1.set('k', 'v'); + + expect(isOsKeyringSessionDisabled()).toBe(true); + + const { adapter, calls } = createCountingAdapter(); + const store2 = new SecureStore(SERVICE, { + keyringLoader: async () => adapter, + fallbackDir: dir, + machineSecretLoader: async () => null, + }); + await store2.get('k'); + expect(calls).toHaveLength(0); + }); + + // Case 6 + it('emits exactly one stderr warning across three failing operations', async () => { + const dir = temp.fallbackDir(); + const stderrSpy = vi.spyOn(process.stderr, 'write'); + + // try/finally: an assertion or async rejection before mockRestore() would + // otherwise leave the spy attached to process.stderr.write and pollute + // every later test in this Bun process. + try { + const store = new SecureStore(SERVICE, { + keyringLoader: async () => + adapterThrowingOnSet(new Error('Permission denied by user')), + fallbackDir: dir, + fallbackPolicy: 'allow', + machineSecretLoader: async () => null, + }); + + for (let i = 0; i < 3; i++) { + await store.set(`k${i}`, `v${i}`); + } + + expect(hasOsKeyringWarningBeenEmitted()).toBe(true); + const notice = stderrSpy.mock.calls.map((c) => String(c[0])).join(''); + const occurrences = notice.split(OS_KEYRING_UNUSABLE_MESSAGE).length - 1; + expect(occurrences).toBe(1); + expect(notice).toContain(OS_KEYRING_UNUSABLE_REMEDIATION); + } finally { + stderrSpy.mockRestore(); + } + }); + + // Case 7 + it('after latch with fallbackPolicy allow, a SECOND op on a fresh counting adapter performs ZERO calls and round-trips through the fallback', async () => { + const dir = temp.fallbackDir(); + const store = new SecureStore(SERVICE, { + keyringLoader: async () => + adapterThrowingOnSet(new Error('The keychain access was denied.')), + fallbackDir: dir, + fallbackPolicy: 'allow', + machineSecretLoader: async () => null, + }); + + // First write latches the session (DENIED) and falls back to the file. + await store.set('roundtrip', 'secret-value'); + expect(await temp.encExists('roundtrip')).toBe(true); + expect(isOsKeyringSessionDisabled()).toBe(true); + + // Second op: a FRESH counting adapter proves ZERO native entry post-latch. + const { adapter, calls } = createCountingAdapter(); + const writer2 = new SecureStore(SERVICE, { + keyringLoader: async () => adapter, + fallbackDir: dir, + fallbackPolicy: 'allow', + machineSecretLoader: async () => null, + }); + await writer2.set('roundtrip', 'updated-value'); + expect(calls).toHaveLength(0); + + // Round-trip still works via the encrypted fallback. + const reader = new SecureStore(SERVICE, { + keyringLoader: async () => null, + fallbackDir: dir, + machineSecretLoader: async () => null, + }); + const value = await reader.get('roundtrip'); + expect(value).toBe('updated-value'); + }); + + // Case 8 + it('after latch with fallbackPolicy deny, a SECOND op on a fresh counting adapter performs ZERO calls and throws UNAVAILABLE with remediation', async () => { + const dir = temp.fallbackDir(); + const primer = new SecureStore(SERVICE, { + keyringLoader: async () => + adapterThrowingOnSet(new Error('The user denied access.')), + fallbackDir: dir, + fallbackPolicy: 'deny', + machineSecretLoader: async () => null, + }); + // Prime the latch with the first (denied-policy) set, which throws. + await expect(primer.set('k', 'v')).rejects.toBeDefined(); + expect(isOsKeyringSessionDisabled()).toBe(true); + + // Second op: a FRESH counting adapter — cannot pass merely by hitting the + // same denied adapter again. Zero native entry post-latch. + const { adapter, calls } = createCountingAdapter(); + const store2 = new SecureStore(SERVICE, { + keyringLoader: async () => adapter, + fallbackDir: dir, + fallbackPolicy: 'deny', + machineSecretLoader: async () => null, + }); + + let error: unknown = null; + try { + await store2.set('k2', 'v2'); + } catch (caught) { + error = caught; + } + + expect(calls).toHaveLength(0); + const e = asSecureStoreError(error); + // Post-latch the keyring is unavailable; the deny policy surfaces + // UNAVAILABLE with a concrete remedy, not the original DENIED. + expect(e.code).toBe('UNAVAILABLE'); + expect(e.remediation.length).toBeGreaterThan(0); + }); + + // Case 9 + it('TIMEOUT does NOT latch: a subsequent operation still calls the adapter', async () => { + const dir = temp.fallbackDir(); + const store1 = new SecureStore(SERVICE, { + keyringLoader: async () => + adapterThrowingOnSet(new Error('The operation timed out.')), + fallbackDir: dir, + fallbackPolicy: 'allow', + machineSecretLoader: async () => null, + }); + await store1.set('k', 'v'); + + expect(isOsKeyringSessionDisabled()).toBe(false); + + const { adapter, calls } = createCountingAdapter(); + const store2 = new SecureStore(SERVICE, { + keyringLoader: async () => adapter, + fallbackDir: dir, + machineSecretLoader: async () => null, + }); + await store2.get('k'); + expect(calls.length).toBeGreaterThan(0); + }); + + // Case 10 + it('UNAVAILABLE does NOT latch: a subsequent operation still calls the adapter', async () => { + const dir = temp.fallbackDir(); + const store1 = new SecureStore(SERVICE, { + keyringLoader: async () => + adapterThrowingOnSet( + new Error("Couldn't access platform storage: NoService"), + ), + fallbackDir: dir, + fallbackPolicy: 'allow', + machineSecretLoader: async () => null, + }); + await store1.set('k', 'v'); + + expect(isOsKeyringSessionDisabled()).toBe(false); + + const { adapter, calls } = createCountingAdapter(); + const store2 = new SecureStore(SERVICE, { + keyringLoader: async () => adapter, + fallbackDir: dir, + machineSecretLoader: async () => null, + }); + await store2.get('k'); + expect(calls.length).toBeGreaterThan(0); + }); + + // Case 11 + it('RUNTIME_REPLACED still propagates, does NOT latch, and is NOT converted to the UNAVAILABLE session error', async () => { + const dir = temp.fallbackDir(); + const { adapter: healthy, calls } = createCountingAdapter(); + const store = new SecureStore(SERVICE, { + keyringLoader: async () => adapterThrowingOnGet(runtimeReplacedError()), + fallbackDir: dir, + fallbackPolicy: 'allow', + machineSecretLoader: async () => null, + }); + + let error: unknown = null; + try { + await store.get('k'); + } catch (caught) { + error = caught; + } + + const e = asSecureStoreError(error); + expect(e.code).toBe('RUNTIME_REPLACED'); + // Must NOT be downgraded to the session UNAVAILABLE error. + expect(e.code).not.toBe('UNAVAILABLE'); + // The latch must NOT have fired for a terminal RUNTIME_REPLACED error. + expect(isOsKeyringSessionDisabled()).toBe(false); + + // A NEW store with a counting adapter must still reach native code — + // proving RUNTIME_REPLACED did not latch the session. + const store2 = new SecureStore(SERVICE, { + keyringLoader: async () => healthy, + fallbackDir: dir, + machineSecretLoader: async () => null, + }); + await store2.get('k'); + expect(calls.length).toBeGreaterThan(0); + }); +}); + +// ─── R2: latch enforced at the adapter boundary ──────────────────────────── + +/** + * Proves the chokepoint (createGuardedAdapter, used by + * createDefaultKeyringAdapter) enforces the latch for every consumer that + * holds an adapter directly — the gap that SecureStore-only catch sites left. + * + * @plan PLAN-20260805-ISSUE2928 + * @requirement R2.1, R2.3, R2.5 + */ +describe('R2 — latch at the adapter boundary', () => { + const session = useResettableSessionState(); + beforeEach(session.beforeEach); + afterEach(session.afterEach); + + it('a guarded adapter held across a latch throws UNAVAILABLE BEFORE native entry (zero native calls)', async () => { + const { adapter: counting, calls } = createCountingAdapter(); + const guarded = createGuardedAdapter(counting); + + // First call succeeds (no latch yet) — proves the adapter is wired through. + await guarded.getPassword(SERVICE, 'a'); + expect(calls.length).toBeGreaterThan(0); + + // Latch the session (simulating a DENIED elsewhere in the process). + setOsKeyringDisabledBySetting(true); + + // The held adapter's SECOND call must throw BEFORE entering native code. + const callsBefore = calls.length; + let error: unknown = null; + try { + await guarded.getPassword(SERVICE, 'a'); + } catch (caught) { + error = caught; + } + expect(calls.length).toBe(callsBefore); // zero native entry post-latch + const e = asSecureStoreError(error); + expect(e.code).toBe('UNAVAILABLE'); + // The remediation names the concrete remedy (restart / clear opt-out). + expect(e.remediation.toLowerCase()).toContain('restart'); + }); + + it('an error raised through the guarded adapter latches the session even when the caller swallows it', async () => { + const guarded = createGuardedAdapter( + adapterThrowingOnGet(new Error('The user denied the keychain request.')), + ); + + expect(isOsKeyringSessionDisabled()).toBe(false); + // Swallow the error, machine-secret-style (readFromKeyring catches). + try { + await guarded.getPassword(SERVICE, 'a'); + } catch { + // swallowed + } + // The guard classified + latched via noteKeyringError regardless of the + // caller swallowing it — the gap that SecureStore-only catch sites left. + expect(isOsKeyringSessionDisabled()).toBe(true); + }); +}); + +// ─── R3: explicit opt-out ─────────────────────────────────────────────────── + +/** + * @plan PLAN-20260805-ISSUE2928 + * @requirement R3.1–R3.5 + */ +describe('R3 — explicit opt-out', () => { + const temp = useTempDirs(); + const session = useResettableSessionState(); + const env = useOptOutEnv(); + beforeEach(session.beforeEach); + afterEach(session.afterEach); + beforeEach(env.beforeEach); + afterEach(env.afterEach); + + // Case 12 + it('LLXPRT_DISABLE_OS_KEYRING=1 makes createDefaultKeyringAdapter() resolve null', async () => { + env.set('1'); + const adapter = await createDefaultKeyringAdapter(); + expect(adapter).toBeNull(); + }); + + // Case 13 + it('with the flag set, a SecureStore set/get/delete round-trip uses the fallback file and the counting loader is never invoked', async () => { + env.set('1'); + const dir = temp.fallbackDir(); + const { adapter, calls } = createCountingAdapter(); + const store = new SecureStore(SERVICE, { + keyringLoader: async () => adapter, + fallbackDir: dir, + machineSecretLoader: async () => null, + }); + + await store.set('optout-key', 'optout-value'); + expect(await temp.encExists('optout-key')).toBe(true); + + const got = await store.get('optout-key'); + expect(got).toBe('optout-value'); + + const deleted = await store.delete('optout-key'); + expect(deleted).toBe(true); + + expect(calls).toHaveLength(0); + }); + + // Case 14 + it('with the flag set, getMachineSecret() resolves from the file only with ZERO adapter/native calls', async () => { + env.set('1'); + const secretPath = temp.machineSecretPath(); + + // Counting keyringLoader: delegates to the real factory so the flag + // short-circuit is exercised, and wraps any returned adapter to count + // native method calls. Under the flag the factory returns null, so the + // wrapper records zero calls — proving no OS keychain access. If the flag + // short-circuit were deleted, the factory would return a real adapter and + // these calls would be non-empty. + const adapterCalls: string[] = []; + const countingKeyringLoader = async (): Promise => { + const real = await createDefaultKeyringAdapter(); + if (real === null) return null; + return wrapWithCallCount(real, adapterCalls); + }; + + const first = await getMachineSecret({ + filePath: secretPath, + keyringLoader: countingKeyringLoader, + }); + expect(first).not.toBeNull(); + expect(await temp.machineSecretExists()).toBe(true); + expect(adapterCalls).toHaveLength(0); + + const known = crypto.randomBytes(32).toString('base64'); + await fs.writeFile(secretPath, known, { mode: 0o600 }); + resetMachineSecretCache(); + const second = await getMachineSecret({ + filePath: secretPath, + generateIfMissing: false, + keyringLoader: countingKeyringLoader, + }); + expect(second).not.toBeNull(); + expect(Buffer.compare(Buffer.from(known, 'base64'), second!)).toBe(0); + // Read-only resolution must also invoke zero native calls. + expect(adapterCalls).toHaveLength(0); + }); + + // Case 15 + it('the env var and the setting are independent opt-out paths: either alone disables the keyring', async () => { + // Setting alone (env unset) disables. + setOsKeyringDisabledBySetting(true); + expect(isOsKeyringSessionDisabled()).toBe(true); + expect(await createDefaultKeyringAdapter()).toBeNull(); + setOsKeyringDisabledBySetting(false); + + // Env alone (setting explicitly false) disables — the env var is read + // directly, so it does not depend on the setter ever being called. + env.set('1'); + expect(isOsKeyringSessionDisabled()).toBe(true); + expect(await createDefaultKeyringAdapter()).toBeNull(); + + // Neither set: the session is enabled again. + env.clear(); + expect(isOsKeyringSessionDisabled()).toBe(false); + }); + + // Case 16 (R3 migration, post-R3.4-removal): a v:2 envelope written while a + // file-resident machine secret exists is still readable in disabled mode — a + // genuine round-trip through the opt-out, not a file-existence check. + it('a v:2 envelope written with a file-resident machine secret round-trips through disabled mode', async () => { + const dir = temp.fallbackDir(); + const secretPath = temp.machineSecretPath(); + const secret = fixedMachineSecret(); + + // Persist the machine secret to the file FIRST, so it is file-resident. + await fs.mkdir(path.dirname(secretPath), { recursive: true }); + await fs.writeFile(secretPath, secret.toString('base64'), { mode: 0o600 }); + + // Write a v:2 envelope using that same secret (injected loader). + const writer = new SecureStore(SERVICE, { + keyringLoader: async () => null, + fallbackDir: dir, + machineSecretPath: secretPath, + machineSecretLoader: async () => secret, + }); + await writer.set('migrate-key', 'migrate-value'); + expect(await temp.encExists('migrate-key')).toBe(true); + + // Switch to disabled mode and read back through the opt-out. The read path + // resolves the machine secret read-only from the file (keyring disabled). + env.set('1'); + resetMachineSecretCache(); + const reader = new SecureStore(SERVICE, { + keyringLoader: async () => null, + fallbackDir: dir, + machineSecretPath: secretPath, + // No injected machineSecretLoader: read path resolves read-only from file. + }); + const value = await reader.get('migrate-key'); + expect(value).toBe('migrate-value'); + }); + + // Case 17 + it('R3.5: in disabled mode with a v:2 envelope and no file secret, get() throws CORRUPT naming the concrete remedy and does NOT create a secret file', async () => { + const dir = temp.fallbackDir(); + const secretPath = temp.machineSecretPath(); + const secret = fixedMachineSecret(); + + // Phase 1 (healthy): write a v:2 envelope using an injected machine secret. + const writer = new SecureStore(SERVICE, { + keyringLoader: async () => null, + fallbackDir: dir, + machineSecretPath: secretPath, + machineSecretLoader: async () => secret, + }); + await writer.set('orphan-key', 'orphan-value'); + expect(await temp.encExists('orphan-key')).toBe(true); + + // Remove the machine-secret file so the read path cannot resolve it. + await fs.unlink(secretPath).catch(() => {}); + expect(await temp.machineSecretExists()).toBe(false); + + // Phase 2 (disabled): read with the default (read-only) machine secret + // loader — no generation, no new secret file. + env.set('1'); + resetMachineSecretCache(); + const reader = new SecureStore(SERVICE, { + keyringLoader: async () => null, + fallbackDir: dir, + machineSecretPath: secretPath, + // No injected machineSecretLoader: read path resolves read-only. + }); + + let error: unknown = null; + try { + await reader.get('orphan-key'); + } catch (caught) { + error = caught; + } + + const e = asSecureStoreError(error); + expect(e.code).toBe('CORRUPT'); + // Actionable: names a concrete remedy (re-enable / re-save / restore), not + // just the word "keyring". + const remedy = e.remediation.toLowerCase(); + expect( + remedy.includes('re-enable') || + remedy.includes('re-save') || + remedy.includes('restore'), + ).toBe(true); + + // No new machine-secret file was created. + expect(await temp.machineSecretExists()).toBe(false); + }); +}); + +// ─── Review remediation: latch-safety and durability boundaries ───────────── + +/** + * Boundaries surfaced by review that the earlier cases did not cover: + * the factory must honor the runtime latch (not only the env/setting flags), + * a filesystem errno error must not latch, and the fallback write must never + * mint a replacement machine secret that would orphan existing v:2 envelopes. + * + * @plan PLAN-20260805-ISSUE2928 + * @requirement R2.1, R2.3, R3.4 + */ +describe('latch safety and fallback durability', () => { + const temp = useTempDirs(); + const session = useResettableSessionState(); + beforeEach(session.beforeEach); + afterEach(session.afterEach); + + it('createDefaultKeyringAdapter returns null after a runtime latch, not a throwing adapter', async () => { + const guarded = createGuardedAdapter( + adapterThrowingOnGet(new Error('The user denied the keychain request.')), + ); + try { + await guarded.getPassword(SERVICE, 'a'); + } catch { + // swallowed — the guard has latched the session + } + expect(isOsKeyringSessionDisabled()).toBe(true); + + // A throwing adapter here would make machine-secret report 'unusable' and + // abort without ever trying its file fallback. + expect(await createDefaultKeyringAdapter()).toBeNull(); + }); + + it('a filesystem errno error does not latch the keyring even when its message reads as denied', async () => { + const eacces: NodeJS.ErrnoException = new Error( + "EACCES: permission denied, open '/tmp/cache'", + ); + eacces.code = 'EACCES'; + + const store = new SecureStore(SERVICE, { + keyringLoader: async () => adapterThrowingOnSet(eacces), + fallbackDir: temp.fallbackDir(), + fallbackPolicy: 'allow', + machineSecretLoader: async () => null, + }); + await store.set('k', 'v'); + + expect(isOsKeyringSessionDisabled()).toBe(false); + }); + + it('refuses to mint a replacement machine secret while disabled when a v:2 envelope exists', async () => { + const dir = temp.fallbackDir(); + const secretPath = temp.machineSecretPath(); + const secret = fixedMachineSecret(); + + // Seed a v:2 envelope sealed under a secret that lives only "in the keychain". + const writer = new SecureStore(SERVICE, { + keyringLoader: async () => null, + fallbackDir: dir, + machineSecretPath: secretPath, + machineSecretLoader: async () => secret, + }); + await writer.set('sealed', 'sealed-value'); + expect(await temp.encExists('sealed')).toBe(true); + expect(await temp.machineSecretExists()).toBe(false); + + // Now the keyring is opted out and the default loader would happily mint a + // brand-new secret, orphaning 'sealed'. It must refuse instead. + setOsKeyringDisabledBySetting(true); + resetMachineSecretCache(); + const store = new SecureStore(SERVICE, { + keyringLoader: async () => null, + fallbackDir: dir, + machineSecretPath: secretPath, + }); + + let error: unknown = null; + try { + await store.set('another', 'another-value'); + } catch (caught) { + error = caught; + } + + const e = asSecureStoreError(error); + expect(e.message.toLowerCase()).toContain('orphan'); + // Critically: no replacement secret was written. + expect(await temp.machineSecretExists()).toBe(false); + }); + + it('still mints a machine secret while disabled when there is no v:2 envelope to orphan', async () => { + const dir = temp.fallbackDir(); + const secretPath = temp.machineSecretPath(); + setOsKeyringDisabledBySetting(true); + resetMachineSecretCache(); + + const store = new SecureStore(SERVICE, { + keyringLoader: async () => null, + fallbackDir: dir, + machineSecretPath: secretPath, + }); + await store.set('fresh', 'fresh-value'); + + expect(await store.get('fresh')).toBe('fresh-value'); + expect(await temp.machineSecretExists()).toBe(true); + }); +}); diff --git a/packages/storage/test-setup-bun-session-reset.ts b/packages/storage/test-setup-bun-session-reset.ts new file mode 100644 index 0000000000..ba9c87fb78 --- /dev/null +++ b/packages/storage/test-setup-bun-session-reset.ts @@ -0,0 +1,35 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Bun-only preload that resets process-wide secure-store module state between + * tests. + * + * The OS keyring session latch (issue #2928) and the machine-secret cache are + * process-wide. A test that exercises a DENIED/LOCKED path latches the keyring + * for the rest of the Bun process, which would turn every later + * healthy-keyring assertion red. + * + * This lives in its own file rather than in test-setup-storage-isolation.ts + * because that file is loaded by BOTH bunfig.toml and vitest.config.ts, and + * importing `bun:test` there breaks Vitest collection with + * "Cannot find package 'bun:test'". + * + * @plan PLAN-20260805-ISSUE2928 + * @requirement R2 + */ + +import { beforeEach, afterEach } from 'bun:test'; +import { resetOsKeyringSessionForTesting } from './src/secure-store/keyring-session-state.js'; +import { resetMachineSecretCache } from './src/secure-store/machine-secret.js'; + +function resetSecureStoreProcessState(): void { + resetOsKeyringSessionForTesting(); + resetMachineSecretCache(); +} + +beforeEach(resetSecureStoreProcessState); +afterEach(resetSecureStoreProcessState); diff --git a/project-plans/issue2928/PLAN.md b/project-plans/issue2928/PLAN.md new file mode 100644 index 0000000000..43a9ffcd83 --- /dev/null +++ b/project-plans/issue2928/PLAN.md @@ -0,0 +1,246 @@ +# PLAN-20260805-ISSUE2928 — Distinguish Keychain denial from absence; degrade once; allow keyring opt-out + +Issue: https://github.com/vybestack/llxprt-code/issues/2928 + +## Problem + +`SecureStore` cannot tell "this credential does not exist" from "macOS refused / the user +cancelled". Every failure collapses into "not found", so the existing `classifyError` +(LOCKED / DENIED / TIMEOUT / NOT_FOUND / UNAVAILABLE) never fires on the read path, users +see a misleading "not authenticated" state, and nothing tells LLxprt to stop hammering a +Keychain that will prompt on every call. + +## Verified evidence + +Read of `Brooooooklyn/keyring-node` `main` (the source of `@napi-rs/keyring`): + +- `src/async_entry.rs`, `PasswordTask::compute` -> `Ok(self.inner.get_password().ok())` + The `.ok()` discards the `OSStatus`. +- `src/async_entry.rs`, `EntryTask::compute`, `TaskKind::DeleteCredential` -> + `Ok(Some(self.inner.delete_credential().is_ok()))` — same collapse. +- `src/entry.rs` (sync `Entry`) — identical `.ok()` / `.is_ok()`. +- `set_password` / `set_secret` DO propagate: `map_err(anyhow::Error::from)?`. + +Empirically confirmed on darwin against the installed binding: + + AsyncEntry('llxprt-nonexistent-test-svc-zzz','nobody').getPassword() -> null + .deleteCredential() -> false + +`@napi-rs/keyring@1.3.0` is installed **and is the latest published version** +(`npm view @napi-rs/keyring versions` ends at 1.3.0). **Upgrading cannot fix the read +path.** + +`findCredentials` is not a usable disambiguator: on macOS its `filter_map` calls +`get_generic_password` for *every* account under the service and drops failures with +`if let Ok(...)`, so using it to disambiguate one read would multiply prompts — the exact +storm this issue exists to stop. + +## Scope + +IN scope: + +- Error fidelity everywhere the binding actually surfaces an error (write path, delete + verification, and cancellation message classification). +- One process-wide state transition that latches the OS keyring unusable after the first + authorization failure, with exactly one user-visible warning, enforced at the adapter + boundary (`createGuardedAdapter`) so no consumer can bypass it. +- `LLXPRT_DISABLE_OS_KEYRING=1` plus a settings equivalent, honored at adapter + construction (and propagated before profile/auth application), with a fail-closed read + path (R3.5) that keeps v:2 envelopes readable when a file-resident machine secret + exists and fails with an actionable error otherwise. (The automatic on-disk machine- + secret mirror originally proposed as a migration invariant — R3.4 — was rejected; see + "Security trade-off".) + +OUT of scope (deferred, requires a dependency/toolchain decision — see "Deferred"): + +- Forking, vendoring, or replacing `@napi-rs/keyring` to recover the read-path `OSStatus`. +- The `set_generic_password` find-error fallthrough (issue body item 2) — that code lives + in the Apple backend inside the Rust dependency and is not reachable from TypeScript. + +## Requirements + +### R1 — Preserve error fidelity where the binding permits + +- **R1.1** Given a keyring operation fails with a genuine user-cancellation error, + `classifyError` returns `DENIED`, not `UNAVAILABLE`. Today such a message matches no + heuristic and falls through to `UNAVAILABLE`, which `isFallbackableKeyringReadError` + treats as degradable — so a cancelled prompt is silently swallowed. The match is + narrowly targeted (`errsecusercanceled` / `errseccanceled`, and word-boundary + "user cancel(l)ed" / "cancel(l)ed by the user") rather than a bare "cancel" + substring: `@napi-rs/keyring` accepts an `AbortSignal` on every method, so + abort/timeout text such as "request cancelled due to timeout" must NOT latch. + Additionally, `noteKeyringError` never latches on a Node syscall/errno error + (`EACCES`, `EPERM`, …), whose message can read as "permission denied" without the + OS keyring having refused anything. +- **R1.2** Given a keyring **write** fails with `DENIED` or `LOCKED`, the failure is not + silently converted into an encrypted-fallback write; the R2 transition fires first. +- **R1.3** The read-path limitation is documented in code and in a follow-up issue: a + denial on `getPassword` is indistinguishable from absence at the binding boundary. + +### R2 — One explicit session-level state transition, enforced at the adapter boundary + +The latch is enforced at the ONE real chokepoint — `createGuardedAdapter()` in +`default-keyring-adapter.ts`. Every consumer's adapter comes from that factory +(SecureStore, machine-secret, and MCP token storage), and the wrapper checks the +session state before each native call and routes every thrown native error +through the shared `noteKeyringError` (classify + latch) before rethrowing it +unchanged. This closes the gap left by SecureStore-only catch sites: consumers +that hold an adapter directly (machine-secret's `readFromKeyring`/ +`persistToKeyringLocked`, MCP's cached module, and SecureStore's own `list()` / +write-verification) can no longer bypass the latch. + +- **R2.1** The first `DENIED` or `LOCKED` classification observed from any keyring + operation latches the OS keyring unusable for the remainder of the process. + Because the guarded adapter routes every native error through `noteKeyringError`, + this holds even when the immediate caller swallows the error (machine-secret + style). `TIMEOUT` does not latch (transient). `UNAVAILABLE` does not latch (no + backend present means no prompts, and the adapter is already `null`). +- **R2.2** Exactly one user-visible warning is emitted per process, on stderr, so it + reaches the user regardless of the injected logger (same rationale as + `emitRuntimeReplacedWarning`). +- **R2.3** After the latch, zero further keyring operations are attempted. Two layers + enforce this: `SecureStore.getKeyring()` returns `null` before invoking the loader, + and — for adapters already cached/held by a consumer — `createGuardedAdapter()` + throws `UNAVAILABLE` before entering native code on every method. +- **R2.4** After the latch, `fallbackPolicy: 'allow'` routes to the encrypted-file + fallback; `fallbackPolicy: 'deny'` fails with actionable remediation. +- **R2.5** The transition is implemented as a single shared `noteKeyringError`/ + `isOsKeyringSessionDisabled` pair invoked at the adapter boundary — not try/catch + sprinkled through call sites. `classifyError` was extracted to a dependency-leaf + module (`classify-error.ts`) so the adapter can import it without a cycle. + Fail-fast over layered defensive guards, per project convention. +- **R2.6** The latch is distinct from `RUNTIME_REPLACED`, which remains terminal, does + NOT latch, and must still be rethrown by fallback layers (it is never converted to + the session `UNAVAILABLE` error). + +### R3 — Explicit opt-out + +- **R3.1** `LLXPRT_DISABLE_OS_KEYRING=1` makes `createDefaultKeyringAdapter()` return + `null` **before** importing `@napi-rs/keyring`, so zero Keychain operations occur — + including for `llxprt-code-machine-secret`. +- **R3.2** A settings equivalent, `security.disableOsKeyring`, is propagated into + `@vybestack/llxprt-code-storage` via an explicit setter invoked during CLI + configuration finalization **before any profile/auth application** (storage is a + low-level package and must not read CLI settings directly). Moving it ahead of the + profile auth wiring avoids a startup Keychain prompt for a user who sets the flag. + The env var stays independent (read directly in storage, zero CLI involvement). +- **R3.3** Every consumer honors it. All three obtain their adapter from the same factory: + `SecureStore` (`keyringLoaderFn`), `machine-secret.ts` (`loadKeyring`), and MCP + `keychain-token-storage.ts` (`defaultKeytarLoader`). The factory is the single chokepoint; + no per-consumer plumbing is required beyond verifying it. +- **R3.4** **REMOVED (machine-secret mirror rejected).** The original plan proposed + mirroring the resolved machine secret to a 0600 on-disk file whenever a v:2 fallback + envelope was written, so a later switch into disabled mode would not orphan the + envelope. That mirror is **not delivered**: it placed the keychain-resident root of + trust on disk, was fail-open on persistence failure, could install a mismatched/stale + secret, and raced other writers. Migration safety now rests on R3.6 and R3.5: users who + need to read a v:2 envelope in disabled mode must keep a file-resident machine secret + (e.g. written explicitly), and a missing secret fails closed with an actionable error. +- **R3.6** While the OS keyring is disabled (latched or opted out), the fallback WRITE + path MUST NOT mint a replacement machine secret when a v:2 envelope already exists. + A keychain-resident secret may be present but unreachable; generating a replacement + would permanently orphan every envelope sealed under the real one, and the newly + written envelopes would themselves become unreadable on the next healthy start. + `loadMachineSecretForWrite` therefore resolves read-only while disabled and refuses + with an actionable `UNAVAILABLE` error if any v:2 envelope would be orphaned. Minting + is still permitted when there is nothing to orphan, so a first-time opt-out user gets + a normal v:2 file-backed store rather than a silent v:1 downgrade. +- **R3.5** In disabled mode, a v:2 envelope that cannot be decrypted because no file + secret exists MUST NOT cause a new secret to be minted. It fails with an actionable + typed error naming the cause and the concrete remedy (re-enable the OS keyring, re-save + the key, or restore the machine-secret file). The read path already passes + `generateIfMissing: false`; this requirement is about the message being actionable. + +## Security trade-off: the machine-secret mirror was rejected (not delivered) + +The original R3.4 proposed mirroring the machine secret to a 0600 on-disk file whenever +a v:2 fallback envelope was written. That approach was **rejected and removed**: + +- It placed the keychain-resident root of trust on disk, eroding the offline-theft + property (a stolen data directory should yield no decryptable v:2 files). +- Persistence failure was fail-open: the envelope write still succeeded, silently + leaving a later disabled-mode read unable to decrypt. +- It could install a mismatched/stale secret (an in-memory secret from one source + written to a shared default path), and it raced other writers. + +As delivered, no machine secret is ever written to disk as a side effect of a fallback +write. Users retain today's offline-theft property: a healthy keyring writes nothing +machine-secret-related to disk, and a stolen data directory yields no decryptable v:2 +files. The cost is that a v:2 envelope written under a healthy keyring cannot be +decrypted in disabled mode unless the same machine secret is independently available +on disk — and that case fails closed with an actionable error (R3.5) rather than +silently or by minting a new root of trust. + +## Test plan (behavioral, bun:test, no mock theater) + +All new/changed tests are `bun:test` under `packages/storage/test-bun/` +(`secure-store.keyring-session.bun.ts`). Tests drive the public API (`SecureStore`, +`createDefaultKeyringAdapter`, `getMachineSecret`, `createGuardedAdapter`) with an +injected `KeyringAdapter` that throws realistic native error messages. A counting +adapter is used where "zero keyring operations" is itself the specification +(R2.3, R3.1) — there, a counting adapter is the only way to observe the behavior. Each +strengthened case is written so it would FAIL if the corresponding production logic were +deleted (e.g. the post-latch second-op cases use a FRESH counting adapter and assert zero +calls, so they cannot pass merely by hitting the same denied adapter again). + +### R1 — error fidelity + +1. `classifyError` via observable behavior: a `get()` whose adapter throws + `User canceled the operation.` throws a `SecureStoreError` with code `DENIED` instead + of returning `null`. +2. Same for `errSecUserCanceled`-worded and `Cancelled` (British) variants. +3. A `NOT_FOUND`-worded error still degrades to the fallback file (no regression). +4. `UNAVAILABLE` ("access platform storage") still degrades (no regression). +5. **R1.1:** `"The request was cancelled due to timeout."` classifies as `TIMEOUT` + (NOT `DENIED`), does NOT latch, and a subsequent op still reaches the adapter. +6. **R1.1:** `"User canceled the operation."` DOES latch (genuine user cancellation). + +### R2 — process-wide latch + +7. First `DENIED` from a write latches: a second `get()` on a NEW `SecureStore` instance + sharing the process performs zero adapter calls (counting adapter asserts 0). +8. Exactly one stderr warning across three failing operations. +9. After latch with `fallbackPolicy:'allow'`, a SECOND op on a fresh counting adapter + performs ZERO adapter calls, and a `set()`/`get()` round-trips through the fallback. +10. After latch with `fallbackPolicy:'deny'`, a SECOND op on a fresh counting adapter + performs ZERO adapter calls AND throws `UNAVAILABLE` (the post-latch code) with a + concrete remediation — not the original `DENIED`. +11. `TIMEOUT` does NOT latch: a subsequent operation still calls the adapter. +12. `UNAVAILABLE` does NOT latch. +13. `RUNTIME_REPLACED` still propagates as `RUNTIME_REPLACED` (not absorbed, not + converted to the session `UNAVAILABLE` error), does NOT latch, and a subsequent op on + a counting adapter still reaches native code. + +### R2 — latch at the adapter boundary + +14. A guarded adapter obtained from `createGuardedAdapter` and held across a latch throws + `UNAVAILABLE` BEFORE native entry on its second call (zero further native calls), with + a remediation naming the concrete remedy (restart). +15. An error raised through the guarded adapter latches the session even when the caller + swallows it (machine-secret style) — proving the chokepoint closes the SecureStore-only + gap. + +### R3 — explicit opt-out + +16. With `LLXPRT_DISABLE_OS_KEYRING=1`, `createDefaultKeyringAdapter()` resolves `null`. +17. With the flag set, a full `SecureStore` set/get/delete round-trip uses the fallback + file and the injected counting keyring loader is never invoked. +18. With the flag set, `getMachineSecret()` resolves from the file only; an injected + counting keyring loader (delegating to the real factory) records zero native/adapter + calls on both the generating and read-only paths. +19. The env var and the setting are independent opt-out paths ORed together: either one + alone disables the keyring, and with neither set the session is enabled. +20. **Migration (post-R3.4-removal):** a v:2 envelope written while a file-resident + machine secret exists is still readable in disabled mode — a genuine round-trip + through the opt-out, not a file-existence check. +21. **R3.5:** in disabled mode with a v:2 envelope and no file secret, `get()` throws a + `CORRUPT` error whose remediation names a concrete remedy (re-enable/re-save/restore), + and the machine-secret file is NOT created. + +## Deferred + +Read-path `OSStatus` fidelity requires forking/vendoring/replacing `@napi-rs/keyring` +(Rust toolchain plus prebuilt binaries for darwin-arm64/x64, linux-x64/arm64 gnu+musl, +win32-x64/arm64). That is a dependency + build + CI change and is tracked in issue #3067. +Once such a binding exists it plugs in behind `createDefaultKeyringAdapter()` without +touching any call site, because every consumer already routes through that factory. diff --git a/schemas/settings.schema.json b/schemas/settings.schema.json index 50b6e3826a..a99aa4f80b 100644 --- a/schemas/settings.schema.json +++ b/schemas/settings.schema.json @@ -927,6 +927,13 @@ "default": false, "type": "boolean" }, + "disableOsKeyring": { + "title": "Disable OS Keyring", + "description": "Disable use of the OS keyring/keychain for credential storage and use the encrypted file fallback. Can also be set with the LLXPRT_DISABLE_OS_KEYRING=1 environment variable.", + "markdownDescription": "Disable use of the OS keyring/keychain for credential storage and use the encrypted file fallback. Can also be set with the LLXPRT_DISABLE_OS_KEYRING=1 environment variable.\n\n- Category: `Security`\n- Requires restart: `yes`\n- Default: `false`", + "default": false, + "type": "boolean" + }, "enablePermanentToolApproval": { "title": "Allow Permanent Tool Approval", "description": "Enable the \"Allow for all future sessions\" option in tool confirmation dialogs.", diff --git a/scripts/bun-test-manifest-data-storage.ts b/scripts/bun-test-manifest-data-storage.ts index 624a540fdc..08c64f0753 100644 --- a/scripts/bun-test-manifest-data-storage.ts +++ b/scripts/bun-test-manifest-data-storage.ts @@ -8,7 +8,15 @@ import type { BunTestWorkspaceEntry } from './bun-test-manifest.ts'; export const STORAGE_MANIFEST_ENTRY: BunTestWorkspaceEntry = { workspace: 'storage', - preload: 'test-setup-storage-isolation.ts', + // Both preloads must be listed here: run_bun_tests.ts passes these as + // explicit --preload args and does NOT read packages/storage/bunfig.toml, so + // a preload declared only there would be silently dropped in manifest-driven + // runs (which is what `npm test` uses) and the process-wide keyring latch + // would leak between test files. + preload: [ + 'test-setup-storage-isolation.ts', + 'test-setup-bun-session-reset.ts', + ], files: [ 'test-bun/credential-write-lock.bun.ts', 'test-bun/keyring-delete-verification.bun.ts', @@ -21,6 +29,7 @@ export const STORAGE_MANIFEST_ENTRY: BunTestWorkspaceEntry = { 'test-bun/secure-store.fallback-hardening.bun.ts', 'test-bun/secure-store.concurrent-write.bun.ts', 'test-bun/secure-store.runtime-replaced.bun.ts', + 'test-bun/secure-store.keyring-session.bun.ts', 'test-bun/storage.bun.ts', 'src/secure-store/provider-key-storage.test.ts', 'src/secure-store/secure-store-integration.test.ts',