Conversation
@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.
github-actions
Bot
added
the
maintainer:e2e:ok
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe secure-store keyring adapter now verifies credential removal after native deletion. A new helper classifies read-back results, while tests cover deletion outcomes, surviving credentials, probe errors, stable error messages, runtime guards, and manifest registration. ChangesSecure-store deletion verification
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
WalkthroughBefore this change, the default keyring adapter treated Release NotesBug Fixes
Tests
Documentation
Changes
Magnitude🎯 1 (S) Related
Pre-merge Checks
Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/storage/test-bun/keyring-delete-verification.bun.ts`:
- Around line 293-301: Update the case 11 test around adapter.deletePassword to
track the existing mocks for deleteCredential and getPassword, then assert both
call counts remain zero after the rejection. Preserve the SecureStoreError
assertion while verifying the runtime-replaced guard runs before either native
operation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: be040f2a-81d8-44e8-9b41-890b86c05573
⛔ Files ignored due to path filters (1)
project-plans/issue3011/plan.mdis excluded by!project-plans/**
📒 Files selected for processing (4)
packages/storage/src/secure-store/default-keyring-adapter.tspackages/storage/src/secure-store/keyring-delete-verification.tspackages/storage/test-bun/keyring-delete-verification.bun.tsscripts/bun-test-manifest-data-storage.ts
OpenCodeReview — PR #3040
|
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
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.
1 participant
TLDR
@napi-rs/keyringerases backend errors into the boolean returned bydeleteCredential(), so a failed delete is indistinguishable from "there was nothing to delete" and a secret can survive a delete that was reported as successful. This adds a read-back probe after every delete and rejects if the credential is still there.The erasure is worse than issue #3011 described. The issue assumed failures collapse to
false. On macOS they collapse totrue.Dive Deeper
The macOS path destroys the delete status three times over
security-frameworkos/macos/passwords.rs:79-85pub fn delete(self) { unsafe { SecKeychainItemDelete(...); } }OSStatusapple-native-keyring-store-1.0.1keychain.rs:87-93item.delete(); Ok(())Okunconditionally@napi-rs/keyring.is_ok()trueSo on macOS a refused delete resolves
true. A caller that correctly checks the return value is still told the credential was removed while it is still in the Keychain. Checking the boolean is not sufficient on any platform, and on macOS it is actively misleading.Note the ordering in layer 2:
find_generic_password(...)?runs before the delete, so a genuinely missing credential returnsErr(NoEntry)early and still yieldsfalse. The discard applies specifically when the item was found and the deletion then failed — the case that matters.Per-platform erasure
truetruefalsetruefalsefalseWhat this does
Read the credential back after every delete. If it is still present, reject. Only once absence is confirmed is the native boolean returned unchanged, so
truestill means "a credential was deleted" andfalsestill means "there was nothing to delete".There is deliberately no
if (deleted) return truefast path — that would skip verification on exactly the macOS case above.Why the error message is fixed and interpolation-free
SecureStore.classifyError()re-derives the error code from the message text and ignoresSecureStoreError.code.validateKey()only rejects/,\,\0,.and..— so a key literally namednot foundis legal. Interpolating service/account into the message would let such a key re-classify the failure asNOT_FOUND, which the delete path is designed to treat as benign, silently defeating the throw.The message is therefore the constant
Credential remains after keyring deletion, and service/account go to a debug log instead. The read-back value is never logged or thrown.The wording is also postcondition-phrased ("remains after") rather than causation-phrased ("delete did not take effect"), because the delete and the probe are two separate native calls and a concurrent writer between them could produce the same state.
Scope and honest limitations
Please read this section — the fix is real but bounded.
KeychainTokenStorage.deleteCredentials()calls this adapter directly and propagates the rejection.SecureStore.delete().deleteLocked()still wraps the call in a barecatch {}. The in-flight PR Harden secure-store fallback: surface delete() keyring failures and align has() with get() (Fixes #1985) #3010 (issue1985, CI green) replaces that with classification and a rethrow of anything that is notNOT_FOUND.secure-store.tsis owned by that PR, so it is deliberately untouched here — the two changes are textually disjoint and will not conflict.Ambiguouscollision, erases the read tonulltoo, so the probe reports absent. Detecting those requires the upstream fix.apple-native-keyring-store.This is why the PR says Refs, not Fixes — #3011 should stay open until #3010 lands and the upstream work resolves.
Dependency decision (asked for by the issue)
There is no earlier version to pin to that propagates these errors; vendoring or replacing the binding is disproportionate while a focused upstream PR is open; the local read-back mitigation is worth having now. Recorded in
project-plans/issue3011/plan.md.Reviewer Test Plan
11 cases. The important ones are 7 and 8 (credential survives a
false/truedelete -> rejects) and 10 (service/account namednot foundstill produce the fixed message).To confirm they are real regression tests rather than tests of existing behaviour, revert
deletePasswordtoreturn entry.deleteCredential();and re-run — case 8 fails withexpect(caught).toBeInstanceOf(SecureStoreError). Restoring makes it pass. I verified this both ways.The fake
@napi-rs/keyringcontrols the native boolean and whether the entry is actually removed independently, which is what allows the macOStrue-but-survives case to be modelled at all.Testing Matrix
Verified locally on macOS:
npm run test,lint,lint:eslint-guard,typecheck,format,build, and the stepfun-37 smoke test all pass. The change is platform-independent TypeScript; the storage suite is 33/33.Linked issues / bugs
Refs #3011. Related to #3010 (which makes this observable through
SecureStore.delete()).Upstream: keyring-node#137, keyring-node#138.
Summary by CodeRabbit