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..ddb8eae0ff --- /dev/null +++ b/packages/storage/test-bun/keyring-delete-verification.bun.ts @@ -0,0 +1,325 @@ +/** + * @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 () => { + // 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(); + + 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', + ); + }); +}); 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',