From 639b6b5eb0a1a61364ff0a6513c84eb4fbf7f4cf Mon Sep 17 00:00:00 2001 From: acoliver Date: Wed, 5 Aug 2026 17:22:58 -0300 Subject: [PATCH 1/4] Stop the Keychain prompt storm and add an OS keyring opt-out (Fixes #2928) SecureStore could not tell "this credential does not exist" from "macOS refused / the user cancelled", so a denied Keychain prompt looked like a missing credential, the existing classifier never fired, and every subsequent read re-prompted with nothing able to stop it. Three changes, all in TypeScript: Error fidelity. classifyError moves to a new dependency-leaf module (classify-error.ts) so the adapter boundary can use it without an import cycle, and it now recognises genuine user cancellation as DENIED instead of letting it fall through to a silently degradable UNAVAILABLE. The match is deliberately narrow -- errSecUserCanceled / errSecCanceled and word-boundary "user cancel(l)ed" -- because @napi-rs/keyring accepts an AbortSignal on every method and abort/timeout text must not latch. Syscall/errno errors (EACCES, EPERM) never latch either: their messages read as "permission denied" without the keyring having refused anything. One session-level transition. keyring-session-state.ts holds the process-wide latch. It is enforced at the single real chokepoint -- createGuardedAdapter() in default-keyring-adapter.ts -- through which SecureStore, machine-secret and MCP token storage all obtain their adapter. The guard checks the session before each native call and routes every thrown native error through the shared noteKeyringError before rethrowing it unchanged, so the first DENIED/LOCKED latches the keyring off, emits exactly one stderr warning, and yields zero further native entry -- even for an adapter a consumer cached before the latch, and even when that consumer swallows the error. RUNTIME_REPLACED stays terminal and is never absorbed. An explicit opt-out. LLXPRT_DISABLE_OS_KEYRING=1 and the security.disableOsKeyring setting make the factory return null before @napi-rs/keyring is imported, so zero Keychain operations occur including for llxprt-code-machine-secret. The setting is pushed into storage at the top of finalizeConfig, ahead of profile auth wiring, because that path performs a real SecureStore read. While the keyring is disabled the fallback write path resolves the machine secret read-only and refuses to mint a replacement when a v:2 envelope exists, since a keychain-resident secret may be present but unreachable and replacing it would permanently orphan those envelopes. Minting is still allowed when there is nothing to orphan. The v:2 read path fails closed with an actionable error rather than generating a new root of trust. Read-path fidelity is deliberately not addressed here. @napi-rs/keyring discards the OSStatus in PasswordTask::compute via Rust .ok(), and 1.3.0 is already the latest release, so recovering it requires changing the native binding. Tracked in #3067. --- packages/cli/src/config/postConfigRuntime.ts | 13 + .../settings-schema/schema-extensions.ts | 10 + packages/storage/bunfig.toml | 2 +- packages/storage/src/index.ts | 4 + .../src/secure-store/classify-error.ts | 104 ++ .../secure-store/default-keyring-adapter.ts | 138 ++- .../src/secure-store/keyring-session-state.ts | 232 +++++ .../secure-store/provider-key-storage.test.ts | 14 +- .../secure-store.fallback.test.ts | 11 +- .../storage/src/secure-store/secure-store.ts | 173 +++- .../secure-store.fallback-hardening.bun.ts | 7 +- .../secure-store.keyring-session.bun.ts | 961 ++++++++++++++++++ .../storage/test-setup-bun-session-reset.ts | 35 + project-plans/issue2928/PLAN.md | 246 +++++ scripts/bun-test-manifest-data-storage.ts | 1 + 15 files changed, 1868 insertions(+), 83 deletions(-) create mode 100644 packages/storage/src/secure-store/classify-error.ts create mode 100644 packages/storage/src/secure-store/keyring-session-state.ts create mode 100644 packages/storage/test-bun/secure-store.keyring-session.bun.ts create mode 100644 packages/storage/test-setup-bun-session-reset.ts create mode 100644 project-plans/issue2928/PLAN.md 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/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 51fb6acea2..6f05933fdd 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 { ProviderKeyStorage, KEY_NAME_REGEX, 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 7b53be4da5..028756557e 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 { SecureStoreError } from './secure-store-errors.js'; import type { KeyringAdapter } from './secure-store.js'; @@ -155,20 +159,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, @@ -176,14 +219,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, @@ -194,25 +249,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; } @@ -244,6 +317,21 @@ 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 4cf4c2d5e2..15ae117a65 100644 --- a/packages/storage/src/secure-store/secure-store.ts +++ b/packages/storage/src/secure-store/secure-store.ts @@ -40,6 +40,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'; @@ -49,6 +53,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) ──────────────────── // @@ -64,7 +72,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 ─────────────────────────────────────────────────────── @@ -96,44 +104,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) { @@ -158,10 +133,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 @@ -186,6 +157,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; @@ -215,6 +187,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(), @@ -228,9 +202,88 @@ export class SecureStore { filePath: this.machineSecretFilePath, }); + /** + * 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 + */ + /** + * 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; + } + + 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 { @@ -315,6 +368,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) { @@ -351,9 +413,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() }; @@ -406,12 +469,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( () => @@ -460,8 +525,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 @@ -496,7 +561,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( () => @@ -560,7 +625,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 @@ -682,7 +747,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( () => @@ -723,7 +788,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 @@ -875,12 +940,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..f0e603012c --- /dev/null +++ b/packages/storage/test-bun/secure-store.keyring-session.bun.ts @@ -0,0 +1,961 @@ +/** + * @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 | undefined) => void; + beforeEach: () => void; + afterEach: () => void; +} { + let saved: string | undefined; + return { + set: (value: string | undefined) => { + if (value === undefined) delete process.env[OPT_OUT_ENV]; + else process.env[OPT_OUT_ENV] = value; + }, + 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'); + + 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); + + 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 settings setter produces the same behavior as the env var, and the env var wins when both are present', async () => { + // Setting-only: behaves like the env var. + setOsKeyringDisabledBySetting(true); + expect(isOsKeyringSessionDisabled()).toBe(true); + const adapter = await createDefaultKeyringAdapter(); + expect(adapter).toBeNull(); + setOsKeyringDisabledBySetting(false); + + // Env wins when both are present (env=1, setting=false → disabled). + env.set('1'); + setOsKeyringDisabledBySetting(false); + expect(isOsKeyringSessionDisabled()).toBe(true); + const adapter2 = await createDefaultKeyringAdapter(); + expect(adapter2).toBeNull(); + }); + + // 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..e783f4223b --- /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 settings setter produces the same behavior as the env var, and the env var wins + when both are present. +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/scripts/bun-test-manifest-data-storage.ts b/scripts/bun-test-manifest-data-storage.ts index 9396a8172e..aa7bc4eff2 100644 --- a/scripts/bun-test-manifest-data-storage.ts +++ b/scripts/bun-test-manifest-data-storage.ts @@ -19,6 +19,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', From fd4410bd4d4e97fd2773517f624aa121153b3c79 Mon Sep 17 00:00:00 2001 From: acoliver Date: Wed, 5 Aug 2026 18:09:27 -0300 Subject: [PATCH 2/4] Regenerate settings docs and correct opt-out precedence wording CI caught two consequences of adding security.disableOsKeyring: - scripts shard: generate-settings-doc's check mode failed because docs/cli/configuration.md and schemas/settings.schema.json were not regenerated. Ran npm run docs:settings; both gain only the new boolean. - cli shard: settings.test.ts asserts the fully-defaulted security block with toStrictEqual, so it needed the new key. Also addresses two review comments on the PR: - The "env var wins when both are present" wording was wrong. isOsKeyringSessionDisabled() ORs the latch, the setting and the env var, so there is no precedence -- each independently disables the keyring. Corrected the comments and rewrote the test to assert what actually holds: either path alone disables, and with neither set the session is enabled. - Wrapped the process.stderr.write spy in try/finally so a failing assertion cannot leave it attached and pollute later tests. --- docs/cli/configuration.md | 5 ++ packages/cli/src/config/settings.test.ts | 1 + .../src/secure-store/keyring-session-state.ts | 19 ++++--- .../secure-store.keyring-session.bun.ts | 57 +++++++++++-------- project-plans/issue2928/PLAN.md | 4 +- schemas/settings.schema.json | 7 +++ 6 files changed, 59 insertions(+), 34 deletions(-) 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/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/src/secure-store/keyring-session-state.ts b/packages/storage/src/secure-store/keyring-session-state.ts index 5923590dd2..74dc043089 100644 --- a/packages/storage/src/secure-store/keyring-session-state.ts +++ b/packages/storage/src/secure-store/keyring-session-state.ts @@ -64,10 +64,10 @@ let warned = false; /** * Opt-out state propagated from CLI settings - * (`security.disableOsKeyring`). The env var is read directly (below) so it - * always wins without a setter call. Storage is a low-level package and must - * not read CLI settings, so the CLI pushes this boolean in during settings - * load (R3.2). + * (`security.disableOsKeyring`). The env var is a separate, independent + * opt-out read directly (below), so it works without a setter call. Storage is + * a low-level package and must not read CLI settings, so the CLI pushes this + * boolean in during settings load (R3.2). */ let disabledBySetting = false; @@ -92,9 +92,14 @@ export function isOsKeyringDisabledBySetting(): boolean { /** * Pushes the settings-derived opt-out into process-wide state. Called by the - * CLI during settings load. The env var wins when both are present because it - * is read directly in {@link isOsKeyringSessionDisabled} and in the adapter - * factory, independent of this flag. + * CLI during settings load. + * + * The env var and this setting are INDEPENDENT opt-out paths, ORed together in + * {@link isOsKeyringSessionDisabled} — neither takes precedence over the other, + * and either alone disables the keyring. The env var is read directly there and + * in the adapter factory, so it works with zero CLI involvement and without + * this setter ever being called. + * * @plan PLAN-20260805-ISSUE2928 * @requirement R3.2 */ diff --git a/packages/storage/test-bun/secure-store.keyring-session.bun.ts b/packages/storage/test-bun/secure-store.keyring-session.bun.ts index f0e603012c..2b4b09c8dd 100644 --- a/packages/storage/test-bun/secure-store.keyring-session.bun.ts +++ b/packages/storage/test-bun/secure-store.keyring-session.bun.ts @@ -418,25 +418,30 @@ describe('R2 — process-wide latch', () => { const dir = temp.fallbackDir(); const stderrSpy = vi.spyOn(process.stderr, 'write'); - 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}`); - } + // 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, + }); - 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); + for (let i = 0; i < 3; i++) { + await store.set(`k${i}`, `v${i}`); + } - stderrSpy.mockRestore(); + 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 @@ -747,20 +752,22 @@ describe('R3 — explicit opt-out', () => { }); // Case 15 - it('the settings setter produces the same behavior as the env var, and the env var wins when both are present', async () => { - // Setting-only: behaves like the env var. + 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); - const adapter = await createDefaultKeyringAdapter(); - expect(adapter).toBeNull(); + expect(await createDefaultKeyringAdapter()).toBeNull(); setOsKeyringDisabledBySetting(false); - // Env wins when both are present (env=1, setting=false → disabled). + // 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'); - setOsKeyringDisabledBySetting(false); expect(isOsKeyringSessionDisabled()).toBe(true); - const adapter2 = await createDefaultKeyringAdapter(); - expect(adapter2).toBeNull(); + expect(await createDefaultKeyringAdapter()).toBeNull(); + + // Neither set: the session is enabled again. + env.set(undefined); + expect(isOsKeyringSessionDisabled()).toBe(false); }); // Case 16 (R3 migration, post-R3.4-removal): a v:2 envelope written while a diff --git a/project-plans/issue2928/PLAN.md b/project-plans/issue2928/PLAN.md index e783f4223b..43a9ffcd83 100644 --- a/project-plans/issue2928/PLAN.md +++ b/project-plans/issue2928/PLAN.md @@ -228,8 +228,8 @@ calls, so they cannot pass merely by hitting the same denied adapter again). 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 settings setter produces the same behavior as the env var, and the env var wins - when both are present. +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. 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.", From 24ec383652f5ef103d6d634753a4ac199f8c8f67 Mon Sep 17 00:00:00 2001 From: acoliver Date: Wed, 5 Aug 2026 18:52:41 -0300 Subject: [PATCH 3/4] Split the opt-out env test helper into set and clear env.set(undefined) read ambiguously enough that review took it for vitest's helper and flagged it as possibly assigning the literal string "undefined". It was a local helper that already deleted the key, but the ambiguity is worth removing: set() now accepts only a string, and an explicit clear() performs the delete, so passing undefined is no longer expressible. --- .../test-bun/secure-store.keyring-session.bun.ts | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/packages/storage/test-bun/secure-store.keyring-session.bun.ts b/packages/storage/test-bun/secure-store.keyring-session.bun.ts index 2b4b09c8dd..8312f6fb8e 100644 --- a/packages/storage/test-bun/secure-store.keyring-session.bun.ts +++ b/packages/storage/test-bun/secure-store.keyring-session.bun.ts @@ -110,15 +110,21 @@ function useResettableSessionState(): { /** Saves, sets, and restores the opt-out env var around each test. */ function useOptOutEnv(): { - set: (value: string | undefined) => void; + set: (value: string) => void; + clear: () => void; beforeEach: () => void; afterEach: () => void; } { let saved: string | undefined; return { - set: (value: string | undefined) => { - if (value === undefined) delete process.env[OPT_OUT_ENV]; - else process.env[OPT_OUT_ENV] = value; + 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]; @@ -766,7 +772,7 @@ describe('R3 — explicit opt-out', () => { expect(await createDefaultKeyringAdapter()).toBeNull(); // Neither set: the session is enabled again. - env.set(undefined); + env.clear(); expect(isOsKeyringSessionDisabled()).toBe(false); }); From ffb022fdf6e5b1a6cb7555462e437edd1f381314 Mon Sep 17 00:00:00 2001 From: acoliver Date: Wed, 5 Aug 2026 20:11:16 -0300 Subject: [PATCH 4/4] Declare the session-reset preload in the manifest, not just bunfig The Bun-only reset hooks were wired into packages/storage/bunfig.toml, but scripts/run_bun_tests.ts builds explicit --preload arguments from the manifest entry's preloads and never reads that file. In manifest-driven runs -- which is what npm test and CI use -- the session reset was silently dropped, so the process-wide keyring latch could leak from one test file into the next. The suites masked it because each one already resets the latch in its own beforeEach. The storage manifest entry now declares both preloads. Also moves the read-path JSDoc back onto loadMachineSecretForRead. It was orphaned above loadMachineSecretForWrite when the R3.4 machine-secret mirror was dropped, leaving two stacked blocks on one method and none on the other. --- .../storage/src/secure-store/secure-store.ts | 20 +++++++++---------- scripts/bun-test-manifest-data-storage.ts | 10 +++++++++- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/packages/storage/src/secure-store/secure-store.ts b/packages/storage/src/secure-store/secure-store.ts index 9963306533..77a80c2e70 100644 --- a/packages/storage/src/secure-store/secure-store.ts +++ b/packages/storage/src/secure-store/secure-store.ts @@ -203,16 +203,6 @@ export class SecureStore { filePath: this.machineSecretFilePath, }); - /** - * 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 - */ /** * Machine-secret resolution for the fallback WRITE path. * @@ -264,6 +254,16 @@ export class SecureStore { 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(); diff --git a/scripts/bun-test-manifest-data-storage.ts b/scripts/bun-test-manifest-data-storage.ts index 678823c67c..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',