From 6cecd71df26265c93ec8ff55dc45c599a292e86b Mon Sep 17 00:00:00 2001 From: acoliver Date: Tue, 4 Aug 2026 17:11:26 -0300 Subject: [PATCH 1/2] build: let cargo test link on macOS The crate is built as a cdylib whose napi_* symbols are supplied by the host Node process at load time. A `cargo test` binary has no such host, so linking fails with undefined napi_* symbols and unit tests cannot run at all. Deferring those symbols to load time lets the test harness link, without affecting the cdylib itself (napi_build already passes the same flag for the library target, and the release build is unchanged). --- .cargo/config.toml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.cargo/config.toml b/.cargo/config.toml index 1261fea..addf895 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -1,3 +1,9 @@ [target.aarch64-unknown-linux-musl] linker = "aarch64-linux-musl-gcc" rustflags = ["-C", "target-feature=-crt-static"] + +# `cargo test` builds this crate as a standalone test binary, which cannot +# resolve the `napi_*` symbols that the Node process provides at load time for +# the cdylib. Deferring those symbols to load time lets unit tests link. +[target.'cfg(target_os = "macos")'] +rustflags = ["-C", "link-arg=-Wl,-undefined,dynamic_lookup"] From 27753811f587954ba195d05cac243a910684dd41 Mon Sep 17 00:00:00 2001 From: acoliver Date: Fri, 21 Aug 2026 04:35:15 -0300 Subject: [PATCH 2/2] fix: propagate deletion and secret store errors sync and async Both getSecret and deleteCredential collapsed every backend failure through .ok()/.is_ok(), reusing the sentinel that also means 'absent'. A locked or inaccessible store made a delete look like a successful removal while the secret stayed readable, and a failed read looked like a cache miss. The two behavior halves now share one rule via a single helper pair: - into_optional maps Ok to Some and NoEntry to None, and propagates every other read error, so a null result means the credential is absent. - into_deleted maps Ok to true and NoEntry to false, and propagates every other delete error, so false always means 'there was nothing to delete'. Public JS signatures are unchanged: getPassword stays (something) or null, getSecret stays null instead of erasing failures, and deletePassword is a deleteCredential alias with the same boolean. Covered under cargo test. The unit suite already carries upstream's password error-preservation tests. --- __test__/index.spec.ts | 41 ++++++++++++++++++ index.d.ts | 36 ++++++++++++---- src/async_entry.rs | 27 ++++++++---- src/entry.rs | 74 +++++++++---------------------- src/lib.rs | 1 + src/result.rs | 98 ++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 205 insertions(+), 72 deletions(-) create mode 100644 src/result.rs diff --git a/__test__/index.spec.ts b/__test__/index.spec.ts index a19b7ca..99487c7 100644 --- a/__test__/index.spec.ts +++ b/__test__/index.spec.ts @@ -8,6 +8,8 @@ const testPassword = 'napi.rs' const testService = 'keyring-node-test-service' const testUser = 'test-user' const testSecret = new Uint8Array([0x68, 0x65, 0x6c, 0x6c, 0x6f]) // "hello" in bytes +const testDeleteUser = 'test-delete-user' +const testMissingSecretUser = 'test-missing-secret-user' test('Should create and get password back', (t) => { const entry = new Entry(testService, testUser) @@ -72,6 +74,45 @@ test('Should handle binary data correctly with setSecret/getSecret async', async await t.notThrowsAsync(() => entry.deleteCredential()) }) +test('deleteCredential should report whether a credential was removed', (t) => { + const entry = new Entry(testService, testDeleteUser) + t.notThrows(() => entry.setPassword(testPassword)) + t.true(entry.deleteCredential(), 'deleting an existing credential reports true') + t.false(entry.deleteCredential(), 'deleting a missing credential reports false') +}) + +test('deleteCredential should report whether a credential was removed async', async (t) => { + const entry = new AsyncEntry(testService, testDeleteUser) + await t.notThrowsAsync(() => entry.setPassword(testPassword)) + t.true(await entry.deleteCredential(), 'deleting an existing credential resolves true') + t.false(await entry.deleteCredential(), 'deleting a missing credential resolves false') +}) + +test('deletePassword should behave like deleteCredential', (t) => { + const entry = new Entry(testService, testDeleteUser) + t.notThrows(() => entry.setPassword(testPassword)) + t.true(entry.deletePassword()) + t.false(entry.deletePassword()) +}) + +test('deletePassword should behave like deleteCredential async', async (t) => { + const entry = new AsyncEntry(testService, testDeleteUser) + await t.notThrowsAsync(() => entry.setPassword(testPassword)) + t.true(await entry.deletePassword()) + t.false(await entry.deletePassword()) +}) + +test('Should return no secret value when the entry is missing async', async (t) => { + const entry = new AsyncEntry(testService, testMissingSecretUser) + // These resolve `null` at runtime while being declared as `undefined`. + t.is((await entry.getSecret()) ?? null, null) +}) + +test('Should return no secret value when the entry is missing', (t) => { + const entry = new Entry(testService, testMissingSecretUser) + t.is(entry.getSecret(), null) +}) + let testTarget: string | undefined const platform = os.platform() diff --git a/index.d.ts b/index.d.ts index 34a95da..efdaf87 100644 --- a/index.d.ts +++ b/index.d.ts @@ -48,9 +48,12 @@ export declare class AsyncEntry { /** * Retrieve the secret saved for this entry. * - * Returns a [NoEntry](Error::NoEntry) error if there isn't one. + * Returns no secret if there isn't one. * - * Can return an [Ambiguous](Error::Ambiguous) error + * Rejects if the credential store cannot be read, for example when it is + * locked or inaccessible. + * + * Can reject with an [Ambiguous](Error::Ambiguous) error * if there is more than one platform credential * that matches this entry. This can only happen * on some platforms, and then only if a third-party @@ -60,9 +63,15 @@ export declare class AsyncEntry { /** * Delete the underlying credential for this entry. * - * Returns a [NoEntry](Error::NoEntry) error if there isn't one. + * Resolves `true` if a credential was deleted, and `false` if there was no + * credential to delete. * - * Can return an [Ambiguous](Error::Ambiguous) error + * Rejects if the credential exists but could not be deleted, for example + * when the store is locked or inaccessible. A failed deletion is never + * reported as `false`, so a `false` result always means the credential is + * absent from the store. + * + * Can reject with an [Ambiguous](Error::Ambiguous) error * if there is more than one platform credential * that matches this entry. This can only happen * on some platforms, and then only if a third-party @@ -74,7 +83,7 @@ export declare class AsyncEntry { */ deleteCredential(signal?: AbortSignal | undefined | null): Promise /** Alias for `deleteCredential` */ - deletePassword(signal?: AbortSignal | undefined | null): Promise + deletePassword(signal?: AbortSignal | undefined | null): Promise } export declare class Entry { @@ -125,9 +134,12 @@ export declare class Entry { /** * Retrieve the secret saved for this entry. * - * Returns a [NoEntry](Error::NoEntry) error if there isn't one. + * Returns no secret if there isn't one. * - * Can return an [Ambiguous](Error::Ambiguous) error + * Throws if the credential store cannot be read, for example when it is + * locked or inaccessible. + * + * Can throw an [Ambiguous](Error::Ambiguous) error * if there is more than one platform credential * that matches this entry. This can only happen * on some platforms, and then only if a third-party @@ -137,9 +149,15 @@ export declare class Entry { /** * Delete the underlying credential for this entry. * - * Returns a [NoEntry](Error::NoEntry) error if there isn't one. + * Returns `true` if a credential was deleted, and `false` if there was no + * credential to delete. * - * Can return an [Ambiguous](Error::Ambiguous) error + * Throws if the credential exists but could not be deleted, for example + * when the store is locked or inaccessible. A failed deletion is never + * reported as `false`, so a `false` result always means the credential is + * absent from the store. + * + * Can throw an [Ambiguous](Error::Ambiguous) error * if there is more than one platform credential * that matches this entry. This can only happen * on some platforms, and then only if a third-party diff --git a/src/async_entry.rs b/src/async_entry.rs index 494d38e..b004d3d 100644 --- a/src/async_entry.rs +++ b/src/async_entry.rs @@ -3,9 +3,9 @@ use std::sync::Arc; use napi::bindgen_prelude::*; use napi_derive::napi; -use crate::entry::into_optional_password; #[cfg(target_os = "linux")] use crate::linux_credential_builder::LinuxCredentialBuilder; +use crate::result::{into_deleted, into_optional}; #[napi] pub struct AsyncEntry { @@ -181,9 +181,12 @@ impl AsyncEntry { #[napi(ts_return_type = "Promise")] /// Retrieve the secret saved for this entry. /// - /// Returns a [NoEntry](Error::NoEntry) error if there isn't one. + /// Returns no secret if there isn't one. /// - /// Can return an [Ambiguous](Error::Ambiguous) error + /// Rejects if the credential store cannot be read, for example when it is + /// locked or inaccessible. + /// + /// Can reject with an [Ambiguous](Error::Ambiguous) error /// if there is more than one platform credential /// that matches this entry. This can only happen /// on some platforms, and then only if a third-party @@ -200,9 +203,15 @@ impl AsyncEntry { #[napi(ts_return_type = "Promise")] /// Delete the underlying credential for this entry. /// - /// Returns a [NoEntry](Error::NoEntry) error if there isn't one. + /// Resolves `true` if a credential was deleted, and `false` if there was no + /// credential to delete. /// - /// Can return an [Ambiguous](Error::Ambiguous) error + /// Rejects if the credential exists but could not be deleted, for example + /// when the store is locked or inaccessible. A failed deletion is never + /// reported as `false`, so a `false` result always means the credential is + /// absent from the store. + /// + /// Can reject with an [Ambiguous](Error::Ambiguous) error /// if there is more than one platform credential /// that matches this entry. This can only happen /// on some platforms, and then only if a third-party @@ -221,7 +230,7 @@ impl AsyncEntry { ) } - #[napi] + #[napi(ts_return_type = "Promise")] /// Alias for `deleteCredential` pub fn delete_password(&self, signal: Option) -> AsyncTask { self.delete_credential(signal) @@ -251,7 +260,7 @@ impl Task for PasswordTask { type JsValue = Option; fn compute(&mut self) -> Result { - into_optional_password(self.inner.get_password()) + into_optional(self.inner.get_password()) } fn resolve(&mut self, _env: Env, output: Self::Output) -> Result { @@ -270,7 +279,7 @@ impl Task for SecretTask { type JsValue = Option>; fn compute(&mut self) -> Result { - Ok(self.inner.get_secret().ok()) + into_optional(self.inner.get_secret()) } fn resolve(&mut self, _env: Env, output: Self::Output) -> Result { @@ -286,7 +295,7 @@ impl Task for EntryTask { fn compute(&mut self) -> Result { match self.kind { - TaskKind::DeleteCredential => Ok(Some(self.inner.delete_credential().is_ok())), + TaskKind::DeleteCredential => into_deleted(self.inner.delete_credential()).map(Some), TaskKind::SetPassword(ref password) => { self .inner diff --git a/src/entry.rs b/src/entry.rs index 55c5343..5ba00c6 100644 --- a/src/entry.rs +++ b/src/entry.rs @@ -3,16 +3,7 @@ use napi_derive::napi; #[cfg(target_os = "linux")] use crate::linux_credential_builder::LinuxCredentialBuilder; - -pub(crate) fn into_optional_password( - result: keyring_core::Result, -) -> Result> { - match result { - Ok(password) => Ok(Some(password)), - Err(keyring_core::Error::NoEntry) => Ok(None), - Err(error) => Err(anyhow::Error::from(error).into()), - } -} +use crate::result::{into_deleted, into_optional}; #[napi] pub struct Entry { @@ -164,29 +155,38 @@ impl Entry { /// on some platforms, and then only if a third-party /// application wrote the ambiguous credential. pub fn get_password(&self) -> Result> { - into_optional_password(self.inner.get_password()) + into_optional(self.inner.get_password()) } #[napi] /// Retrieve the secret saved for this entry. /// - /// Returns a [NoEntry](Error::NoEntry) error if there isn't one. + /// Returns no secret if there isn't one. /// - /// Can return an [Ambiguous](Error::Ambiguous) error + /// Throws if the credential store cannot be read, for example when it is + /// locked or inaccessible. + /// + /// Can throw an [Ambiguous](Error::Ambiguous) error /// if there is more than one platform credential /// that matches this entry. This can only happen /// on some platforms, and then only if a third-party /// application wrote the ambiguous credential. - pub fn get_secret(&self) -> Option> { - self.inner.get_secret().ok() + pub fn get_secret(&self) -> Result>> { + into_optional(self.inner.get_secret()) } #[napi] /// Delete the underlying credential for this entry. /// - /// Returns a [NoEntry](Error::NoEntry) error if there isn't one. + /// Returns `true` if a credential was deleted, and `false` if there was no + /// credential to delete. /// - /// Can return an [Ambiguous](Error::Ambiguous) error + /// Throws if the credential exists but could not be deleted, for example + /// when the store is locked or inaccessible. A failed deletion is never + /// reported as `false`, so a `false` result always means the credential is + /// absent from the store. + /// + /// Can throw an [Ambiguous](Error::Ambiguous) error /// if there is more than one platform credential /// that matches this entry. This can only happen /// on some platforms, and then only if a third-party @@ -195,51 +195,17 @@ impl Entry { /// Note: This does _not_ affect the lifetime of the [Entry] /// structure, which is controlled by Rust. It only /// affects the underlying credential store. - pub fn delete_credential(&self) -> bool { - self - .inner - .delete_credential() - .map_err(anyhow::Error::from) - .is_ok() + pub fn delete_credential(&self) -> Result { + into_deleted(self.inner.delete_credential()) } #[napi] /// Alias for `deleteCredential` - pub fn delete_password(&self) -> bool { + pub fn delete_password(&self) -> Result { self.delete_credential() } } -#[cfg(test)] -mod tests { - use super::into_optional_password; - - #[test] - fn returns_password_when_found() { - assert_eq!( - into_optional_password(Ok("password".to_string())).unwrap(), - Some("password".to_string()) - ); - } - - #[test] - fn returns_none_when_password_is_missing() { - assert_eq!( - into_optional_password(Err(keyring_core::Error::NoEntry)).unwrap(), - None - ); - } - - #[test] - fn preserves_non_missing_errors() { - let result = into_optional_password(Err(keyring_core::Error::NoStorageAccess(Box::new( - std::io::Error::other("keychain is locked"), - )))); - - assert!(result.is_err()); - } -} - #[napi(object)] pub struct Credential { pub account: String, diff --git a/src/lib.rs b/src/lib.rs index 9e7038d..f72e2a7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -5,3 +5,4 @@ pub mod entry; #[cfg(target_os = "linux")] mod linux_credential_builder; +mod result; diff --git a/src/result.rs b/src/result.rs new file mode 100644 index 0000000..1db28c4 --- /dev/null +++ b/src/result.rs @@ -0,0 +1,98 @@ +use napi::bindgen_prelude::*; + +/// Convert a lookup result into an optional value. +/// +/// [`keyring_core::Error::NoEntry`] means the credential simply is not there, +/// which is not a failure: it becomes `None`. Every other error describes a +/// store that could not answer the question (locked, inaccessible, ambiguous, +/// malformed) and is propagated so the caller can tell "absent" apart from +/// "unavailable". +pub(crate) fn into_optional(result: keyring_core::Result) -> Result> { + match result { + Ok(value) => Ok(Some(value)), + Err(keyring_core::Error::NoEntry) => Ok(None), + Err(error) => Err(anyhow::Error::from(error).into()), + } +} + +/// Convert a deletion result into "was a credential removed?". +/// +/// [`keyring_core::Error::NoEntry`] means there was nothing to delete, so the +/// entry is already in the requested state and `false` is reported. Every +/// other error means the credential may still exist in the store, so it is +/// propagated rather than reported as `false`; otherwise a failed deletion +/// would be indistinguishable from a no-op and the caller would believe a +/// secret had been removed when it had not. +pub(crate) fn into_deleted(result: keyring_core::Result<()>) -> Result { + match result { + Ok(()) => Ok(true), + Err(keyring_core::Error::NoEntry) => Ok(false), + Err(error) => Err(anyhow::Error::from(error).into()), + } +} + +#[cfg(test)] +mod tests { + use super::{into_deleted, into_optional}; + + fn platform_error() -> keyring_core::Error { + keyring_core::Error::NoStorageAccess(Box::new(std::io::Error::other( + "credential store is locked", + ))) + } + + #[test] + fn into_optional_returns_value_when_found() { + assert_eq!( + into_optional(Ok("password".to_string())).unwrap(), + Some("password".to_string()) + ); + } + + #[test] + fn into_optional_returns_none_when_entry_is_missing() { + let result: Result, _> = into_optional(Err(keyring_core::Error::NoEntry)); + assert_eq!(result.unwrap(), None); + } + + #[test] + fn into_optional_preserves_non_missing_errors() { + let result: Result, _> = into_optional(Err(platform_error())); + assert!(result.is_err()); + } + + #[test] + fn into_optional_supports_secret_payloads() { + assert_eq!( + into_optional(Ok(vec![1u8, 2, 3])).unwrap(), + Some(vec![1u8, 2, 3]) + ); + } + + #[test] + fn into_optional_preserves_ambiguous_errors() { + let result: Result>, _> = + into_optional(Err(keyring_core::Error::Ambiguous(Vec::new()))); + assert!(result.is_err()); + } + + #[test] + fn into_deleted_reports_true_when_credential_removed() { + assert!(into_deleted(Ok(())).unwrap()); + } + + #[test] + fn into_deleted_reports_false_when_entry_is_missing() { + assert!(!into_deleted(Err(keyring_core::Error::NoEntry)).unwrap()); + } + + #[test] + fn into_deleted_preserves_non_missing_errors() { + assert!(into_deleted(Err(platform_error())).is_err()); + } + + #[test] + fn into_deleted_preserves_ambiguous_errors() { + assert!(into_deleted(Err(keyring_core::Error::Ambiguous(Vec::new()))).is_err()); + } +}