deleteCredential erases native failures into false, silently leaving credentials in the store (getSecret likewise; #136 covers only getPassword)

[acoliver]

Summary

deleteCredential() / deletePassword() collapse every backend error into false via .is_ok(). Because false is also the legitimate "there was no such credential" result, callers cannot distinguish "nothing to delete" from "the delete failed and the secret is still in the OS keyring".

The same erasure applies to getSecret() via .ok().

PR #136 fixes exactly this class of bug, but only for getPassword(). The delete and secret paths are untouched, so the most security-relevant instance remains.

Why this one matters more than the read path

A read that silently returns undefined degrades into a cache miss or a re-auth prompt — annoying, but fail-safe.

A delete that silently returns false is fail-open: the calling application believes a credential has been revoked and reports success to its user, while the secret remains readable in the Keychain / Credential Manager / secret-service. Callers implementing "log out", "rotate token", or "forget this account" have no way to detect the failure, and no amount of care on the JavaScript side can recover information that was already discarded in Rust.

This is not hypothetical for us: we consume @napi-rs/keyring@1.3.0 as the default backend of a secret store, and we recently hardened our delete() to classify and rethrow keyring rejections — only to find that the native binding never rejects here, so the hardening cannot fire.

Affected sites (v1.3.0, unchanged on main @ 3e7bcc4)

#	Site	Current	Erases
1	Entry::delete_credential	.is_ok() -> bool	all delete errors
2	Entry::delete_password	alias of 1	all delete errors
3	EntryTask::compute (DeleteCredential)	.is_ok() -> Option<bool>	all delete errors
4	AsyncEntry::delete_password	alias of 3	all delete errors
5	Entry::get_secret	.ok() -> Option<Vec<u8>>	all read errors
6	SecretTask::compute	.ok() -> Option<Vec<u8>>	all read errors

For comparison, PR #136 covers only entry.rs#L156-L158 and async_entry.rs#L253.

As in #136, the information is available and is discarded only at this wrapper layer: keyring-core returns a full Result, and the platform stores map errSecItemNotFound to NoEntry while mapping access failures to NoStorageAccess and other failures to PlatformFailure.

The doc comments already describe the intended behaviour

Every one of these functions carries a doc comment that the signature makes unreachable:

/// Delete the underlying credential for this entry.
///
/// Returns a [NoEntry](Error::NoEntry) error if there isn't one.
///
/// Can return an [Ambiguous](Error::Ambiguous) error
/// if there is more than one platform credential
/// that matches this entry.
pub fn delete_credential(&self) -> bool {

Ambiguous is worth calling out: it means a third-party app wrote a colliding credential and the store cannot tell which one to delete. That is precisely the case where silently answering false is most damaging, and the doc comment promises it will surface.

Reproduction

Any backend error works; the easiest deterministic one is the same error class asserted in #136's own new test (NoStorageAccess):

1. Store a credential.
2. Put the store into a state where deletion fails but the item still exists — for example lock the login keychain on macOS, or deny the process access to the item's ACL.
3. Call await new AsyncEntry(service, account).deleteCredential().

Expected: rejection (or at minimum something distinguishable from "absent").
Actual: resolves false, identical to a delete of a never-existing entry. Re-reading the entry afterwards still returns the secret.

Proposed fix

Mirror #136's into_optional_password helper for the delete path, so NoEntry remains the benign case and everything else propagates:

pub(crate) fn into_delete_result(result: keyring_core::Result<()>) -> Result<bool> {
  match result {
    Ok(()) => Ok(true),
    Err(keyring_core::Error::NoEntry) => Ok(false),
    Err(error) => Err(anyhow::Error::from(error).into()),
  }
}

and the analogous into_optional_secret for sites 5 and 6.

This preserves the existing boolean return type and keeps the common "deleted" / "wasn't there" outcomes exactly as they are today. It is still a behavioural break for anyone currently relying on a failed delete resolving false, so it likely belongs in the same release as #136 — a single release that makes error propagation consistent across read and delete seems much better for consumers than two staggered breaks.

Happy to open a PR for this if you would like, either standalone or rebased on top of #136 so the helpers live together.

Secondary findings

Lower severity, same theme, listed for completeness rather than as part of the ask:

• AsyncEntry::delete_password is typed Promise<unknown>. It lacks the #[napi(ts_return_type = "Promise<boolean>")] attribute that delete_credential carries (async_entry.rs#L224-L227), so the generated index.d.ts declares deletePassword(signal?): Promise<unknown> right next to deleteCredential(signal?): Promise<boolean>. This is the alias the bundled keytar.js shim calls, so the keytar-compat surface gets both the erased error and an unusable type.
• Windows with_target discards attribute-preservation failures. entry.rs#L104-L113 and async_entry.rs#L108-L117 use if let Ok(_) = set_secret(&[]) plus update_attributes(&attrs).ok(), so a failure to persist the username attribute is invisible and surfaces later as a credential that cannot be found by username.
• Linux store selection falls back silently. linux_credential_builder.rs#L17-L20 drops the secret-service error with Err(_) and falls back to keyutils. Since the kernel keyutils store is typically session-scoped rather than persistent, a user whose secret-service is unavailable silently gets non-persistent credential storage with no diagnostic. Even keeping the fallback, logging or exposing which store was selected would help.
• macOS find_credentials silently drops unreadable entries. The filter_map at entry.rs#L340-L370 omits any account whose get_generic_password fails, so an inaccessible credential is indistinguishable from a nonexistent one in the returned array.

Thanks for maintaining this — the binding is otherwise a very clean keytar replacement, which is exactly why the silent-delete case caught us off guard.

[acoliver]

Opened #138 with a fix covering all six sites listed above, plus the deletePassword typing note from the secondary findings. The two Windows with_target items, the Linux keyutils fallback and the macOS find_credentials filtering are not addressed there and remain open questions on this issue.

[acoliver]

Correction to my own report, and it makes the delete case worse than I described.

In the "Secondary findings" above I said a downstream report of a platform provider discarding the delete result "could not be substantiated". That was wrong. I was looking only at this repository. The discard is real, it is just one and two layers further down, and the consequence is more serious than the false erasure this issue was opened about.

On macOS the delete status is destroyed three times over:

1. security-framework-3.7.0/src/os/macos/passwords.rs:79-85

   /// Delete this item from its keychain
   #[inline]
   pub fn delete(self) {
       unsafe {
           SecKeychainItemDelete(self.as_concrete_TypeRef());
       }
   }
   

   Returns (). The OSStatus from SecKeychainItemDelete is discarded.

2. apple-native-keyring-store-1.0.1/src/keychain.rs:87-93

   fn delete_credential(&self) -> Result<()> {
       let (_, item) =
           find_generic_password(Some(&[self.get_keychain()?]), &self.service, &self.account)
               .map_err(decode_error)?;
       item.delete();
       Ok(())
   }
   

   Ok(()) unconditionally, whatever the OS did.

3. This repository: .is_ok() -> true.

So on macOS deleteCredential() resolves true when the OS refused the delete. Not false — true. A caller that correctly checks the return value is still told the credential was removed while it is still in the keychain.

Note the ordering in step 2: find_generic_password(...)? runs before the delete, so a genuinely missing credential returns Err(NoEntry) early and still yields false. The discard applies specifically to the case where the item was found and the deletion then failed — the exact case that matters.

What this means for #138

#138 does not fix macOS delete, and cannot. It changes .is_ok() into proper error propagation, which is correct and still worth merging — it fixes Linux and Windows delete, where the provider does propagate a real error that the binding was throwing away. But on macOS there is no error left to propagate by the time the binding sees it; keyring_core::Result is already Ok.

I am not proposing to widen #138. Fixing this properly belongs upstream in apple-native-keyring-store (and arguably in security-framework, whose macOS delete(self) swallows an OSStatus that SecKeychainItem's own item.rs:534 variant returns as Result<()>). I will raise it there and link back here.

Flagging it on this issue so the delete work is not considered closed by #138 alone, and to correct my earlier statement.

[acoliver]

Filed the two lower-layer bugs referenced in my correction above, so the full chain is tracked:

1. SecKeychainItem::delete() discards the OSStatus, so a failed keychain deletion is indistinguishable from success kornelski/rust-security-framework#256 — SecKeychainItem::delete() returns () and drops the OSStatus. The FFI declares SecKeychainItemDelete(...) -> OSStatus, and the sibling set_password() in the same impl block checks its status via cvt(...)?, so this looks like an oversight rather than a decision.
2. delete_credential() returns Ok(()) even when the keychain deletion failed open-source-cooperative/apple-native-keyring-store#19 — delete_credential() does item.delete(); Ok(()), converting the (already-lost) failure into a definite success.

To be explicit about what this means for #138: it remains correct and worth merging, and it fixes delete on Linux and Windows, where the provider does return a real error that .is_ok() currently discards. It cannot fix macOS, because by the time the binding sees the value it is already Ok(()) — there is nothing left to propagate. That needs (2), which in turn is cleanest after (1), though (2) may be fixable independently by switching to security_framework's already-checked SecItemDelete path.

No action needed here. Flagging it so macOS delete is not assumed fixed once #138 lands.

[acoliver]

Fixes now proposed for both lower layers, so the whole chain has a PR:

1. Report SecKeychainItemDelete failures via try_delete() kornelski/rust-security-framework#258 — adds try_delete() returning Result<()> and deprecates delete(). Includes a regression test that fails when the status is discarded.
2. Report keychain deletion failures instead of discarding them open-source-cooperative/apple-native-keyring-store#21 — propagates that status through decode_error. Draft, because it cannot build until (1) is released; there is no other status-returning way to delete from a specific SecKeychain.

Once those land, .is_ok() in this repo becomes the only remaining erasure on macOS, and #138 then fixes it for real rather than converting an already-lost error.