From b243c54950db81944626e32bb54e77d2b84f95ca Mon Sep 17 00:00:00 2001 From: acoliver Date: Tue, 4 Aug 2026 20:23:46 -0300 Subject: [PATCH 1/2] Verify keyring deletes actually removed the credential (Refs #3011) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @napi-rs/keyring erases backend errors into the boolean returned by deleteCredential(), so SecureStore cannot tell a failed delete from "there was nothing to delete" and a secret can survive a delete that was reported as successful. The erasure is worse on macOS than the issue described. The delete status is discarded at three layers: security-framework's macOS SecKeychainItem::delete() returns unit and drops the OSStatus, apple-native-keyring-store calls item.delete() then returns Ok(()) unconditionally, and the binding maps that to true. So on macOS a refused delete resolves TRUE, not false. Checking the boolean is not enough on any platform, and on macOS it is actively misleading. Read the credential back after every delete and reject if it is still there. The native boolean is returned unchanged only once absence has been confirmed, so true still means "a credential was deleted" and false still means "there was nothing to delete". The thrown message is fixed and interpolation-free on purpose: SecureStore.classifyError() re-derives the code from message text and ignores SecureStoreError.code, and validateKey() permits a key literally named "not found" — interpolating one would classify the failure as NOT_FOUND and get it swallowed downstream. Service and account go to a debug log instead; the read-back value is never logged or thrown. This closes the gap at the layer the adapter owns. It is not yet observable through SecureStore.delete(), whose bare catch is rewritten by the separate in-flight PR #3010, and it cannot detect a failure that also makes the read-back fail (a fully locked store, or an Ambiguous collision, both of which the binding erases to null). Refs rather than Fixes for that reason. --- .../secure-store/default-keyring-adapter.ts | 42 ++- .../keyring-delete-verification.ts | 59 ++++ .../keyring-delete-verification.bun.ts | 302 ++++++++++++++++++ project-plans/issue3011/plan.md | 285 +++++++++++++++++ scripts/bun-test-manifest-data-storage.ts | 1 + 5 files changed, 688 insertions(+), 1 deletion(-) create mode 100644 packages/storage/src/secure-store/keyring-delete-verification.ts create mode 100644 packages/storage/test-bun/keyring-delete-verification.bun.ts create mode 100644 project-plans/issue3011/plan.md diff --git a/packages/storage/src/secure-store/default-keyring-adapter.ts b/packages/storage/src/secure-store/default-keyring-adapter.ts index 60c3a90138..ce24da68a7 100644 --- a/packages/storage/src/secure-store/default-keyring-adapter.ts +++ b/packages/storage/src/secure-store/default-keyring-adapter.ts @@ -28,6 +28,8 @@ 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 { verifyKeyringDelete } from './keyring-delete-verification.js'; +import { SecureStoreError } from './secure-store-errors.js'; import type { KeyringAdapter } from './secure-store.js'; export type FindCredentialsFunction = ( @@ -242,7 +244,45 @@ export async function createDefaultKeyringAdapter(): Promise { const entry = new kr.AsyncEntry(service, account); - return entry.deleteCredential(); + const deleted = await entry.deleteCredential(); + // Always probe, regardless of the native boolean. On macOS the delete + // status is destroyed below the binding (security-framework discards + // the OSStatus, apple-native-keyring-store returns Ok unconditionally, + // and the binding maps that to true), so a true result is no guarantee + // the credential is gone. Only the read-back can confirm absence. + const outcome = await verifyKeyringDelete(() => entry.getPassword()); + if (outcome === 'still-present') { + // Diagnostics only — service/account names, never the read-back value. + _keyringLogger.debug( + () => + `[keyring] credential remains after deletion: service='${service}' account='${account}'`, + ); + // Where this rejection currently surfaces: + // - MCP KeychainTokenStorage.deleteCredentials() calls this + // adapter directly and propagates it. Observable today. + // - SecureStore.deleteLocked() still wraps this call in a bare + // `catch {}` and discards it. NOT observable through + // SecureStore.delete() yet; the in-flight PR for issue #1985 + // replaces that catch with classification and a rethrow of + // anything that is not NOT_FOUND. secure-store.ts is owned by + // that PR, so it is deliberately not modified here. + // + // The message is fixed and interpolation-free for when that lands: + // classifyError() re-derives the code from message text and ignores + // SecureStoreError.code, so the message must avoid every trigger + // substring ("not found", "locked", "denied", "permission", + // "timeout", "timed out"). validateKey() permits a key literally + // named "not found" — interpolating one would re-classify this as + // NOT_FOUND and get it swallowed, silently defeating the throw. + throw new SecureStoreError( + 'Credential remains after keyring deletion', + 'UNAVAILABLE', + 'Retry the delete; if it persists, inspect the OS keyring entry for this service and account and remove it manually.', + ); + } + // Absent: return the original native boolean unchanged. true means "a + // credential was deleted"; false means "there was nothing to delete". + return deleted; }, }; const withFindCreds = withFindCredentials(adapter, findCredentialsFn); diff --git a/packages/storage/src/secure-store/keyring-delete-verification.ts b/packages/storage/src/secure-store/keyring-delete-verification.ts new file mode 100644 index 0000000000..494c2ec715 --- /dev/null +++ b/packages/storage/src/secure-store/keyring-delete-verification.ts @@ -0,0 +1,59 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Keyring delete verification. + * + * Reads back the credential after a delete to check whether it is actually + * gone. This is necessary on every platform because `@napi-rs/keyring`'s + * `deleteCredential()` erases backend errors into a boolean: on Linux/Windows + * failures collapse to `false`, and on macOS the OSStatus is discarded at + * three layers below the binding so failures collapse to `true`. Absence is + * decided by `=== null` only — the empty string is a present credential. + * + * A rejecting read-back propagates, because absence cannot be confirmed and + * claiming the secret is gone would be the exact failure this module exists + * to detect. + * + * Known limitation: this detects a credential that remains *readable* after a + * delete (the macOS silent-failure case, ACL denials, partial backend + * failure). It does NOT detect a fully locked store (the read erases to + * `null`) or `Ambiguous` collisions (the binding erases the ambiguous read + * error to `null`, so the probe reports absent for a credential that still + * exists). Completeness requires the upstream fix; see + * project-plans/issue3011/plan.md for the full matrix. + * + * @plan PLAN-20260804-ISSUE3011 + * @requirement R1 + */ + +/** + * Discriminated outcome of a post-delete read-back probe. + * + * - `absent` — read-back is `null`; the credential is gone. + * - `still-present` — read-back is a value (including the empty string); the + * delete did not take effect. + * + * @plan PLAN-20260804-ISSUE3011 + * @requirement R1 + */ +export type KeyringDeleteOutcome = 'absent' | 'still-present'; + +/** + * Reads back the credential after a delete and classifies whether it is gone. + * + * Takes a thunk rather than a `KeyringAdapter` because the adapter is still + * under construction inside the factory when this runs. + * + * @plan PLAN-20260804-ISSUE3011 + * @requirement R1 + */ +export async function verifyKeyringDelete( + readBack: () => Promise, +): Promise { + const value = await readBack(); + return value === null ? 'absent' : 'still-present'; +} diff --git a/packages/storage/test-bun/keyring-delete-verification.bun.ts b/packages/storage/test-bun/keyring-delete-verification.bun.ts new file mode 100644 index 0000000000..b8e82aaf4d --- /dev/null +++ b/packages/storage/test-bun/keyring-delete-verification.bun.ts @@ -0,0 +1,302 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Behavioral tests for keyring delete verification (issue #3011). + * + * `@napi-rs/keyring`'s `deleteCredential()` erases backend errors into a + * boolean: on Linux/Windows failures collapse to `false`, and on macOS the + * OSStatus is discarded below the binding so failures collapse to `true`. A + * read-back probe after every delete checks whether the credential is actually + * gone. These tests assert the observable contract of that probe and of the + * adapter that uses it — not internal call wiring. + * + * @plan PLAN-20260804-ISSUE3011 + * @requirement R1 + */ + +import { beforeEach, afterEach, describe, expect, it, mock } from 'bun:test'; +import type { KeyringAdapter } from '../src/secure-store/secure-store.js'; +import { + verifyKeyringDelete, + type KeyringDeleteOutcome, +} from '../src/secure-store/keyring-delete-verification.js'; +import { createDefaultKeyringAdapter } from '../src/secure-store/default-keyring-adapter.js'; +import { SecureStoreError } from '../src/secure-store/secure-store-errors.js'; +import { + resetRuntimeIdentityForTesting, + forceRuntimeReplacedForTesting, +} from '../src/secure-store/runtime-identity.js'; +import { resetRuntimeReplacedWarningForTesting } from '../src/secure-store/runtime-replaced-errors.js'; + +// ─── Fake @napi-rs/keyring (boundary double) ──────────────────────────────── +// +// The factory dynamic-imports @napi-rs/keyring; bun's mock.module intercepts +// that import for this isolated process (see scripts/run_bun_tests.ts: one +// process per file). The native return value and whether the entry is +// actually removed are independently controllable, so the macOS silent-failure +// case (native true + credential survives) can be staged. + +interface FakeKeyringController { + readonly entries: Map; + /** What deleteCredential() returns — the native boolean. */ + deleteResult: boolean; + /** + * Whether deleteCredential() actually removes the entry from the store. + * false simulates a refused delete (macOS: the OS rejected it but the + * binding still reported true). + */ + actuallyRemoves: boolean; + /** When set, getPassword() rejects with this error (probe-rejection case). */ + probeError: Error | null; +} + +function createFreshController(): FakeKeyringController { + return { + entries: new Map(), + deleteResult: false, + actuallyRemoves: true, + probeError: null, + }; +} + +function compositeKey(service: string, account: string): string { + return `${service}\u0000${account}`; +} + +let controller: FakeKeyringController = createFreshController(); + +mock.module('@napi-rs/keyring', () => ({ + AsyncEntry: class { + constructor( + private readonly service: string, + private readonly account: string, + ) {} + + async getPassword(): Promise { + if (controller.probeError !== null) { + throw controller.probeError; + } + return ( + controller.entries.get(compositeKey(this.service, this.account)) ?? null + ); + } + + async deleteCredential(): Promise { + if (controller.actuallyRemoves) { + controller.entries.delete(compositeKey(this.service, this.account)); + } + return controller.deleteResult; + } + }, +})); + +// ─── verifyKeyringDelete (cases 1-4) ───────────────────────────────── + +/** + * @plan PLAN-20260804-ISSUE3011 + * @requirement R1 + */ +describe('verifyKeyringDelete', () => { + it('classifies a null read-back as absent (case 1)', async () => { + const outcome: KeyringDeleteOutcome = await verifyKeyringDelete( + async () => null, + ); + expect(outcome).toBe('absent'); + }); + + it('classifies a non-null read-back as still-present (case 2)', async () => { + const outcome: KeyringDeleteOutcome = await verifyKeyringDelete( + async () => 'a-real-secret', + ); + expect(outcome).toBe('still-present'); + }); + + it('classifies an empty-string read-back as still-present, not absent (case 3)', async () => { + // An empty credential is still a credential; a falsy check would silently + // treat it as absent, defeating the verification. + const outcome: KeyringDeleteOutcome = await verifyKeyringDelete( + async () => '', + ); + expect(outcome).toBe('still-present'); + }); + + it('propagates a read-back rejection instead of degrading to absent (case 4)', async () => { + const readBack = async (): Promise => { + throw new Error('probe failed'); + }; + await expect(verifyKeyringDelete(readBack)).rejects.toThrow('probe failed'); + }); +}); + +// ─── createDefaultKeyringAdapter deletePassword (cases 5-11) ──────────────── + +async function loadAdapter(): Promise { + const adapter = await createDefaultKeyringAdapter(); + if (adapter === null) { + throw new Error( + 'createDefaultKeyringAdapter returned null — fake @napi-rs/keyring mock did not load', + ); + } + return adapter; +} + +/** + * @plan PLAN-20260804-ISSUE3011 + * @requirement R1 + */ +describe('createDefaultKeyringAdapter deletePassword — delete verification (issue #3011)', () => { + beforeEach(() => { + resetRuntimeIdentityForTesting(); + resetRuntimeReplacedWarningForTesting(); + controller = createFreshController(); + }); + afterEach(() => { + resetRuntimeIdentityForTesting(); + resetRuntimeReplacedWarningForTesting(); + }); + + // The trigger substrings classifyError() scans the message for; the fixed + // message must contain none of them. + const TRIGGERS = [ + 'not found', + 'locked', + 'denied', + 'permission', + 'timeout', + 'timed out', + ]; + + it('returns true when native delete succeeds and the credential is gone (case 5)', async () => { + controller.deleteResult = true; + controller.actuallyRemoves = true; + const adapter = await loadAdapter(); + + const result = await adapter.deletePassword('svc', 'acct'); + + expect(result).toBe(true); + }); + + it('returns false when native delete reports false and the credential is absent (case 6)', async () => { + controller.deleteResult = false; + controller.actuallyRemoves = true; + const adapter = await loadAdapter(); + + const result = await adapter.deletePassword('svc', 'acct'); + + expect(result).toBe(false); + }); + + it('rejects when native delete reports false but the credential survives (case 7)', async () => { + const secret = 'super-secret-value-xyz'; + controller.deleteResult = false; + controller.actuallyRemoves = false; + controller.entries.set(compositeKey('svc', 'acct'), secret); + const adapter = await loadAdapter(); + + let caught: unknown = null; + try { + await adapter.deletePassword('svc', 'acct'); + } catch (error) { + caught = error; + } + + expect(caught).toBeInstanceOf(SecureStoreError); + if (caught instanceof SecureStoreError) { + expect(caught.code).toBe('UNAVAILABLE'); + expect(caught.message).toBe('Credential remains after keyring deletion'); + // Never the secret value. + expect(caught.message).not.toContain(secret); + // Never a classifyError trigger substring. + const lower = caught.message.toLowerCase(); + for (const trigger of TRIGGERS) { + expect(lower).not.toContain(trigger); + } + } + }); + + it('rejects when native delete reports true but the credential survives — the macOS silent-failure case (case 8)', async () => { + // This is the single most important test in the file: on macOS the native + // delete reports true even when the OS refused, so without a read-back the + // failure is invisible. The old `if (deleted) return true` fast path would + // pass this credential through as deleted. + const secret = 'survives-despite-true'; + controller.deleteResult = true; + controller.actuallyRemoves = false; + controller.entries.set(compositeKey('svc', 'acct'), secret); + const adapter = await loadAdapter(); + + let caught: unknown = null; + try { + await adapter.deletePassword('svc', 'acct'); + } catch (error) { + caught = error; + } + + expect(caught).toBeInstanceOf(SecureStoreError); + if (caught instanceof SecureStoreError) { + expect(caught.code).toBe('UNAVAILABLE'); + expect(caught.message).toBe('Credential remains after keyring deletion'); + expect(caught.message).not.toContain(secret); + const lower = caught.message.toLowerCase(); + for (const trigger of TRIGGERS) { + expect(lower).not.toContain(trigger); + } + } + }); + + it('propagates a read-back probe rejection out of deletePassword (case 9)', async () => { + controller.probeError = new Error('probe unreadable'); + const adapter = await loadAdapter(); + + await expect(adapter.deletePassword('svc', 'acct')).rejects.toThrow( + 'probe unreadable', + ); + }); + + it('uses the fixed message even when service/account contain classifyError triggers (case 10)', async () => { + // validateKey() allows a key literally named "not found". The message must + // not interpolate it, or classifyError() would re-classify the error as + // NOT_FOUND and deleteLocked would swallow it. + controller.deleteResult = false; + controller.actuallyRemoves = false; + controller.entries.set( + compositeKey('svc not found', 'acct not found'), + 'value', + ); + const adapter = await loadAdapter(); + + let caught: unknown = null; + try { + await adapter.deletePassword('svc not found', 'acct not found'); + } catch (error) { + caught = error; + } + + expect(caught).toBeInstanceOf(SecureStoreError); + if (caught instanceof SecureStoreError) { + expect(caught.message).toBe('Credential remains after keyring deletion'); + // The trigger-bearing service/account names must not leak into the message. + expect(caught.message).not.toContain('not found'); + expect(caught.message).not.toContain('svc not found'); + expect(caught.message).not.toContain('acct not found'); + const lower = caught.message.toLowerCase(); + for (const trigger of TRIGGERS) { + expect(lower).not.toContain(trigger); + } + } + }); + + it('fires the runtime-replaced guard before any native call (case 11)', async () => { + const adapter = await loadAdapter(); + // Force the terminal state on an already-cached adapter. + forceRuntimeReplacedForTesting(); + + await expect(adapter.deletePassword('svc', 'acct')).rejects.toBeInstanceOf( + SecureStoreError, + ); + }); +}); diff --git a/project-plans/issue3011/plan.md b/project-plans/issue3011/plan.md new file mode 100644 index 0000000000..19f40c1dcf --- /dev/null +++ b/project-plans/issue3011/plan.md @@ -0,0 +1,285 @@ +# Issue #3011 — Detect native delete failures erased into `false` + +## Problem + +`packages/storage/src/secure-store/default-keyring-adapter.ts` returns the +result of `@napi-rs/keyring`'s `AsyncEntry.deleteCredential()` straight through: + + deletePassword: async (service, account) => { + const entry = new kr.AsyncEntry(service, account); + return entry.deleteCredential(); + }, + +The native binding computes that boolean as `self.inner.delete_credential().is_ok()`, +so **every** backend error is collapsed into `false` — the same value it returns +when there was simply no credential to delete. A genuinely failed delete is +therefore indistinguishable from a no-op, and `SecureStore.delete()` reports +success while the secret survives in the OS keyring. + +Confirmed upstream at `Brooooooklyn/keyring-node` (issue 137, PR 138). The same +erasure affects `getPassword`/`getSecret` via `.ok()`. + +### Correction to the issue text (verified) + +Issue #3011 says "at least one platform provider discards the result of the +underlying delete entirely". This **is** substantiated — on macOS the delete +status is destroyed below the binding at three layers: + +1. `security-framework-3.7.0/src/os/macos/passwords.rs:81` — + `pub fn delete(self) { unsafe { SecKeychainItemDelete(self.as_concrete_TypeRef()); } }` + returns `()`, discarding the `OSStatus` entirely. +2. `apple-native-keyring-store-1.0.1/src/keychain.rs:87-93` — + `fn delete_credential(&self) -> Result<()> { ... item.delete(); Ok(()) }` + — unconditionally `Ok`. +3. `@napi-rs/keyring` — `.is_ok()` → `true`. + +So on macOS `deleteCredential()` resolves **`true`** even when the OS refused +the delete. This is distinct from the generic `.is_ok()` erasure on other +platforms (Linux/Windows), where a failed delete collapses to `false`. + +**Consequence:** upstream binding PR `Brooooooklyn/keyring-node#138` cannot fix +macOS delete, because the error is destroyed *below* the binding (in the Rust +`apple-native-keyring-store` and `security-framework` crates). Propagating the +`OSStatus` from the binding would require the intermediate crates to stop +discarding it first. + +## Dependency on PR #3010 (READ THIS FIRST) + +`SecureStore.deleteLocked()` on `main` still swallows adapter rejections: + + try { + deletedFromKeyring = await adapter.deletePassword(this.serviceName, key); + } catch { + // Keyring delete failed + } + +PR #3010 (branch `issue1985`, CI green, **not yet merged**) replaces that bare +`catch` with classification and a rethrow of any non-`NOT_FOUND` failure. + +Consequence: the adapter-level fix in this plan is **correct but not +end-to-end observable until #3010 merges**. Until then a thrown delete failure +is still swallowed one layer up. The two changes are textually disjoint — +#3010 does not touch `default-keyring-adapter.ts` and this plan does not touch +`secure-store.ts` — so they will not conflict. + +**Do not "fix" `deleteLocked` as part of this work.** That code is owned by +#3010; duplicating it would create a conflict. + +## Scope + +In scope, and only this: + +- `packages/storage/src/secure-store/default-keyring-adapter.ts` — the + `deletePassword` implementation. +- A new sibling verification module (see below). +- Tests for both. +- Manifest registration for any new bun test file. + +Out of scope: `secure-store.ts`, `classifyError`, the fallback file path, +`getPassword`/`getSecret` erasure (upstream's problem; we cannot detect a read +failure locally because the erased value *is* the answer we would probe with), +and vendoring/patching/replacing the native dependency. + +## Design + +### New module: `keyring-delete-verification.ts` + +Mirror the existing `keyring-write-verification.ts` convention — a small, +dependency-light, directly unit-testable module rather than logic buried in the +adapter factory closure. + + export type KeyringDeleteOutcome = 'absent' | 'still-present'; + + export async function verifyKeyringDelete( + readBack: () => Promise, + ): Promise; + +Taking a `readBack` thunk rather than a `KeyringAdapter` matters: the adapter is +still being constructed inside the factory when this runs, and a thunk is +trivially testable without any native binding. + +### Adapter behaviour + +The probe runs after **every** delete, regardless of the native boolean. The +macOS three-layer chain (see above) means a `true` result is no guarantee the +credential is gone, so the `if (deleted) return true` fast path is unsafe and +must not be used. + + const deleted = await entry.deleteCredential(); + const outcome = await verifyKeyringDelete(() => entry.getPassword()); + if (outcome === 'still-present') throw ; + return deleted; + +Resulting contract for `deletePassword`: + +| Result | Meaning | +| --- | --- | +| `true` | the native delete reported success **and** the read-back confirmed the credential is gone | +| `false` | the native delete reported nothing-to-delete **and** the read-back confirmed the credential is gone | +| throws | the credential is still present after the delete (the read-back found it) | + +The native `true`/`false` distinction is preserved (not collapsed): it is +returned unchanged when the probe confirms absence. `true` still means "a +credential was deleted"; `false` still means "there was nothing to delete". +The probe is the safety net layered on top, not a replacement for the native +signal. + +### Fail-fast on an unreadable probe + +If the read-back itself rejects, **let it propagate** — do not catch and do not +degrade to `false`. We cannot confirm the secret is gone, so claiming it is +would be the exact failure this issue exists to prevent. This also matches the +project's stated preference for failing fast over defensive hedging. + +Note this branch is nearly unreachable with `@napi-rs/keyring@1.3.0` (its +`getPassword` erases errors to `null` too) but becomes live once upstream +propagates errors, so it is written now rather than retrofitted. + +### Error shape + +Throw `SecureStoreError` from `./secure-store-errors.js` (a dependency-leaf +module — importing it creates no cycle). + +Code: `UNAVAILABLE`. This is deliberate and needs to be understood: +`SecureStore.classifyError()` re-derives the code from the **message text** and +ignores an existing `SecureStoreError.code`, so the message governs behaviour +downstream. The message is a **fixed, interpolation-free string**: + + Credential remains after keyring deletion + +The message must avoid the substrings `not found`, `locked`, `denied`, +`permission`, `timeout`, `timed out`. Under PR #3010 only `NOT_FOUND` is +swallowed by `deleteLocked`, but any specific classification changes caller +behaviour, so all triggers are avoided. The message is deliberately +postcondition-phrased ("remains after") rather than causation-phrased: the +delete and the probe are two separate native calls, so a concurrent writer +between them could also produce this state; the message must not assert this +process definitively caused it. + +The message must **never** be interpolated with service/account/key names. +`validateKey()` only rejects `/`, ``, `\0`, `.`, and `..` — so a key legally +named `not found` would be interpolated into the message and re-classified as +`NOT_FOUND`, which `deleteLocked` swallows, silently defeating this throw. +Service and account context are emitted to the module's `_keyringLogger` at +debug level instead, so diagnostics are not lost. + +**The message must never include the read-back value or any secret material.** +Service and account/key names only (and only in the debug log), consistent with +existing debug logging. + +## Known limitation (document, do not try to solve) + +Read-back can only detect a failed delete when the credential remains +*readable*. If the backend is failing such that both the delete and the read +return their erased sentinels — a fully locked store, where `getPassword` +yields `null` — the probe sees `null` and reports `absent`, and the failure goes +undetected. + +This mitigation closes the "delete refused but item still readable" case (the +macOS silent-failure case where native `true` is returned but the OS refused, +ACL denials on Linux/Windows, partial backend failure) and not the "store +entirely inaccessible" case. It does **not** reliably detect `Ambiguous` +collisions: the read-back performs the same ambiguous lookup, and the binding +erases that read error to `null`, so the probe can report "absent" for a +credential that still exists. `Ambiguous` is therefore excluded from the list +of detected cases. It is a strict improvement, not a complete fix; completeness +requires the upstream change. State this plainly in the PR — do not overclaim. + +### Per-platform erasure matrix + +| Platform | Delete succeeded | Delete failed (OS refused) | Nothing to delete | +| --- | --- | --- | --- | +| **macOS** | `true` | **`true`** (three-layer discard: `security-framework` → `apple-native-keyring-store` → binding `is_ok`) | `false` | +| **Linux/Windows** | `true` | `false` (binding `is_ok` → `false`) | `false` | + +macOS reports `false` for "nothing to delete" because +`apple-native-keyring-store` calls `find_generic_password(...)?` *before* +deleting, so a missing credential returns `Err(NoEntry)` early and never +reaches the `item.delete(); Ok(())` discard. The discard only applies once the +item has been found — which is precisely the "delete failed" column. + +On macOS the native boolean is **meaningless** for a failed delete (it always +reads `true`), so the read-back probe is the only local signal that can catch a +refused delete. On Linux/Windows the boolean at least collapses failures to +`false`, but that is indistinguishable from "nothing to delete", so the probe +adds value there too. + +### Dependency decision + +- **No good older version to pin to.** The macOS three-layer discard predates + the current `@napi-rs/keyring` release; older versions exhibit the same or + worse behaviour. +- **Replacement/vendoring is disproportionate** while upstream PR + `Brooooooklyn/keyring-node#138` is open. Replacing the native dependency or + vendoring a patched fork would be a large, maintenance-heavy change for a + mitigation that the local read-back already provides. +- **The local read-back mitigation is worthwhile now.** It catches the most + dangerous case (macOS silent failure) and the Linux/Windows ACL-denial case + without any dependency change. +- **The Apple provider defect needs separate upstream tracking.** Because the + error is destroyed below the binding (in `apple-native-keyring-store` and + `security-framework`), even a merged PR #138 cannot fix macOS delete; the + intermediate Rust crates must stop discarding the `OSStatus` first. + +## Test plan (test-first, behavioral, per dev-docs/RULES.md) + +No mock theater: assert observable behaviour of the contract above, not call +counts or internal wiring. A fake `readBack` thunk is a legitimate boundary +double, not a mock of the unit under test. + +`verifyKeyringDelete`: + +1. read-back returns `null` -> `'absent'` +2. read-back returns a value -> `'still-present'` +3. read-back returns empty string -> `'still-present'` (an empty credential is + still a credential; must not be treated as absent via a falsy check) +4. read-back rejects -> the rejection propagates + +Adapter-level, against a fake keyring module injected in place of +`@napi-rs/keyring` (the factory dynamic-imports it, so tests must drive it the +way the existing suite does). The fake **independently controls** the native +return value and whether the entry is actually removed, so the macOS +silent-failure case (native `true` + credential survives) can be staged: + +5. native `true` + entry actually removed -> resolves `true` +6. native `false` + entry actually removed -> resolves `false` +7. native `false` + entry survives -> **rejects**, error is a `SecureStoreError`, + fixed message contains neither the secret value nor a `classifyError` trigger +8. native `true` + entry survives -> **rejects** (the macOS silent-failure case; + the most important test in the file — the old `if (deleted) return true` fast + path would let this pass as deleted) +9. read-back probe rejects -> the rejection propagates out of `deletePassword` +10. service/account containing `not found` -> the fixed message is used (no + interpolation), and the message contains no trigger substring +11. the runtime-replaced guard still fires before any native call (assert error + identity only — no call-count assertions) + +No `getPasswordCallCount` / `deleteCallCount` assertions or counters: those are +mock-interaction assertions forbidden by `dev-docs/RULES.md`. The probe-rejection +case (9) is the behavioural way to prove the probe is part of the contract. +Narrow thrown errors with `instanceof`, never `as` type assertions. + +Regression guard: existing `deletePassword` behaviour for the two common +outcomes (`true` on success, `false` on genuine absence) is unchanged. + +## Verification + + npm run test + npm run lint + npm run lint:eslint-guard + npm run typecheck + npm run format + npm run build + bun scripts/start.ts --profile-load stepfun-37 "write me a haiku and nothing else" + +New bun test files must be registered in +`scripts/bun-test-manifest-data-storage.ts` or they will not run in CI. + +## Constraints + +- No new `.js` files and no Vitest/Node tests — TypeScript and `bun:test` only. +- Never add `eslint-disable`, `@ts-ignore`, `@ts-expect-error`, `@ts-nocheck`, + severity downgrades, or complexity-threshold increases. Fix the underlying + issue instead. This is mechanically enforced by + `scripts/check-eslint-guard.js`. +- Do not touch `.llxprt/`. diff --git a/scripts/bun-test-manifest-data-storage.ts b/scripts/bun-test-manifest-data-storage.ts index 94e7647579..e8e64cf548 100644 --- a/scripts/bun-test-manifest-data-storage.ts +++ b/scripts/bun-test-manifest-data-storage.ts @@ -11,6 +11,7 @@ export const STORAGE_MANIFEST_ENTRY: BunTestWorkspaceEntry = { preload: 'test-setup-storage-isolation.ts', files: [ 'test-bun/credential-write-lock.bun.ts', + 'test-bun/keyring-delete-verification.bun.ts', 'test-bun/keyring-write-verification.bun.ts', 'test-bun/machine-secret.bun.ts', 'test-bun/machine-secret.concurrent-write.bun.ts', From 90c10d74507d820674bf2188f96b8b9eafd1eb8b Mon Sep 17 00:00:00 2001 From: acoliver Date: Tue, 4 Aug 2026 20:59:30 -0300 Subject: [PATCH 2/2] Strengthen runtime-replaced guard test to prove no native call occurs The case only asserted that deletePassword() rejects, which also passes if deleteCredential() or getPassword() ran before the guard threw. Prove it behaviourally rather than with call counters: seed a credential and stage the fake so a native delete would remove it and a native read would reject with a distinctive probe error. Asserting the RUNTIME_REPLACED identity and the surviving credential shows neither native call happened. Verified by removing the delete guard: the case then fails. --- .../keyring-delete-verification.bun.ts | 27 +++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/packages/storage/test-bun/keyring-delete-verification.bun.ts b/packages/storage/test-bun/keyring-delete-verification.bun.ts index b8e82aaf4d..ddb8eae0ff 100644 --- a/packages/storage/test-bun/keyring-delete-verification.bun.ts +++ b/packages/storage/test-bun/keyring-delete-verification.bun.ts @@ -291,12 +291,35 @@ describe('createDefaultKeyringAdapter deletePassword — delete verification (is }); it('fires the runtime-replaced guard before any native call (case 11)', async () => { + // Stage the fake so that either native call, if it ran, would leave + // observable evidence: deleteCredential() would remove the seeded entry, + // and getPassword() would reject with a distinctive probe error. Asserting + // on that evidence proves the guard ran first without inspecting call + // counts, which would be testing wiring rather than behaviour. + controller.entries.set(compositeKey('svc', 'acct'), 'seeded-secret'); + controller.deleteResult = true; + controller.actuallyRemoves = true; + controller.probeError = new Error('probe must never run'); const adapter = await loadAdapter(); // Force the terminal state on an already-cached adapter. forceRuntimeReplacedForTesting(); - await expect(adapter.deletePassword('svc', 'acct')).rejects.toBeInstanceOf( - SecureStoreError, + let caught: unknown = null; + try { + await adapter.deletePassword('svc', 'acct'); + } catch (error) { + caught = error; + } + + // The runtime-replaced error, not the probe error — so getPassword() + // never ran. + expect(caught).toBeInstanceOf(SecureStoreError); + if (caught instanceof SecureStoreError) { + expect(caught.code).toBe('RUNTIME_REPLACED'); + } + // The credential survives — so deleteCredential() never ran. + expect(controller.entries.get(compositeKey('svc', 'acct'))).toBe( + 'seeded-secret', ); }); });