fix: preserve non-missing password read errors - #136
Conversation
Treat only NoEntry as an absent password while propagating other keyring errors through the synchronous and asynchronous bindings.
df0f7a5 to
bfa9820
Compare
acoliver
left a comment
acoliver
left a comment
There was a problem hiding this comment.
Thanks for this — the diagnosis is exactly right, and into_optional_password is the right shape for the fix: keep NoEntry as the benign None, propagate everything else. We hit this same class of bug from the consumer side and would very much like to see it land.
One thing worth flagging before it merges: the same .ok() / .is_ok() erasure exists at four other N-API boundaries that this PR does not touch, and the delete path is arguably the more severe instance. Filed as #137 with full detail, summarised here since it affects how this PR is best released.
Not covered by this PR (line numbers at v1.3.0):
| Site | Current | Result |
|---|---|---|
Entry::delete_credential |
.is_ok() |
failure -> false |
EntryTask::compute (DeleteCredential) |
.is_ok() |
failure -> false |
Entry::get_secret |
.ok() |
failure -> undefined |
SecretTask::compute |
.ok() |
failure -> undefined |
The delete case is worse than the read case this PR fixes. A read that erases to undefined fails safe — the caller re-prompts. A delete that erases to false fails open: false is indistinguishable from "there was no such credential", so an application implementing log-out or token rotation reports success to its user while the secret remains readable in the keychain.
Why this is a release-coordination question rather than just a follow-up: both changes are behavioural breaks for anyone currently treating a resolved value as "no error". Shipping the read fix in one release and the delete fix in another forces consumers through two rounds of the same migration, and in between leaves the API in a state where getPassword throws on backend failure but deleteCredential still silently swallows it — which is harder to reason about than either the current behaviour or the fully-fixed behaviour. It would be worth deciding now whether to extend this PR or to land both together.
Two small notes on the diff itself:
-
into_optional_passwordis defined inentry.rsand imported intoasync_entry.rs. If the delete and secret variants are added, three cross-modulepub(crate)helpers may be worth pulling into a small shared module (src/result.rsor similar) soentry.rsis not implicitly the home for conversions used by both. -
The helper is generic over the payload in everything but its name —
into_optional_passwordapplies verbatim toget_secret'sResult<Vec<u8>>. Making itinto_optional<T>(result: keyring_core::Result<T>) -> Result<Option<T>>would cover sites 5 and 6 with no additional logic, leaving only the delete variant (which mapsOk(())totrue) as a separate function.
I am happy to open a follow-up PR for the delete and secret paths against this branch, or to send a patch extending it here — whichever the maintainer prefers. Either way, this PR is a clear improvement over the status quo and I would rather see it merged than stalled.
|
Following up on my earlier comment: I opened #138 with the remaining sites, and I want to be straightforward that it ended up broader than the "delete and secret paths" I said I would send. It also touches Two reasons it grew, neither of them a criticism of this PR:
The If you would rather land this PR first, I am happy to rebase #138 to drop the |
|
I'm ok with anything, as we need to get these fixes to prod ASAP, since many of our end users are currently experiencing issues caused by these bugs. I would rather avoid forking this repo to get the fixes in. |
|
@Brooooooklyn, kindly pinging you if you would have time to look at these PRs |
|
@skvark I also submitted upstream. Along with PRs fwiw open-source-cooperative/apple-native-keyring-store#19 |
|
I'm on vacation. I'll deal with it as soon as I get back to the computer. |
…alse/absent (#138) ## Summary This PR preserves `NoEntry` as the benign absence case while allowing every other provider error to reach JavaScript: - `getSecret()` returns absence only for `keyring_core::Error::NoEntry`; other failures throw or reject. - `deleteCredential()` and `deletePassword()` return or resolve `true` after a successful delete and `false` only for `NoEntry`; other failures throw or reject. - Sync and async paths share the same result-mapping helpers. - The async `deletePassword()` declaration is corrected from `Promise<unknown>` to `Promise<boolean>`. Fixes #137. ## Why The previous `.ok()` and `.is_ok()` conversions discarded provider errors at the N-API boundary. A locked or inaccessible credential store could look like a missing secret. A failed delete could look like a credential that was already absent, leaving callers unable to determine whether the secret remained stored. `keyring-core` already distinguishes `NoEntry` from `NoStorageAccess`, `Ambiguous`, encoding failures, and other provider errors. This change preserves that distinction through the JavaScript API. ## Behavior | Operation | Provider result | JavaScript result | | --- | --- | --- | | `getSecret()` | value | value | | `getSecret()` | `NoEntry` | absent result | | `getSecret()` | any other error | throw or reject | | delete methods | `Ok(())` | `true` | | delete methods | `NoEntry` | `false` | | delete methods | any other error | throw or reject | The same mapping applies to `Entry` and `AsyncEntry`, including the `deletePassword()` aliases. ## Relationship to #136 PR #136 is now merged and owns the `getPassword()` error-propagation behavior. This branch is rebased onto that work. The generic optional-result helper preserves the merged behavior while serving both password and secret reads. This PR does not duplicate #136's JavaScript failure tests. ## macOS provider status The current Cargo resolution selects `apple-native-keyring-store` 1.0.2 through the existing compatible manifest range. Version 1.0.2 changed macOS deletion to use the checked `ItemSearchOptions::delete()` path, so a refused delete reaches this binding as an error. No dependency declaration or lockfile change is included here. See [apple-native-keyring-store#22](open-source-cooperative/apple-native-keyring-store#22). ## Compatibility Existing success and absence results remain unchanged: - Existing secrets return their value. - Missing secrets return the existing absent result. - Successful deletes return `true`. - Deletes of missing credentials return `false`. Provider failures that previously appeared as absence or `false` now throw or reject. The generated declarations retain the existing common return types. The async `deletePassword()` alias now accurately declares `Promise<boolean>` instead of `Promise<unknown>`. ## Tests Rust unit tests cover the mapping boundary for: - successful values and deletes - `NoEntry` - `NoStorageAccess` - `Ambiguous` - binary secret payloads Native JavaScript integration tests use the real platform store and cover: - sync and async `deleteCredential()` returning `true` for an existing credential and `false` after it is absent - sync and async `deletePassword()` alias parity - sync and async missing-secret reads No mocks are used. Local verification on macOS arm64: ```text cargo fmt -- --check pass cargo clippy --all-targets pass, no warnings cargo test pass, 9 tests node .yarn/releases/yarn-4.18.0.cjs build pass node .yarn/releases/yarn-4.18.0.cjs lint pass, no warnings or errors node .yarn/releases/yarn-4.18.0.cjs test pass, 14 tests cargo tree -i apple-native-keyring-store --locked resolves 1.0.2 git diff --check upstream/main...HEAD pass ``` ## Commit structure The first commit adds a macOS-scoped linker setting needed for `cargo test`. N-API symbols are supplied by Node when the addon loads, but the standalone Rust test binary otherwise fails to link them. The release native build and JavaScript integration suite pass with this setting. The second commit contains the result mapping, generated declaration, documentation, and behavioral tests.
|
@Brooooooklyn looks like the PRs are now sorted out. Could you make a new release? Thanks! |
3 participants
Summary
keyring_core::Error::NoEntryWhy
The wrapped libraries intentionally distinguish a missing credential from an
inaccessible or failing credential store.
keyring-corereturnsResult<String>, while the Apple backend mapserrSecItemNotFoundtoNoEntry, access failures toNoStorageAccess, and other Security.frameworkfailures to
PlatformFailure.Calling
.ok()erases all those errors and makes every failure appear toJavaScript as
null/undefined. This change preserves the optional result forNoEntrywhile allowing other failures to throw or reject.This also matches node-keytar compatibility: missing credentials resolve as
null, while native failures reject.GitHits helped find and verify this bug.1
Validation
Footnotes
GitHits helped find and verify this bug. Starting from a GitHits user report, GitHits' version-pinned package and source search traced the exact dependency chain from GitHits' resolved
@napi-rs/keyring@1.3.0Darwin binaries (lockfile) to.ok()at both N-API read boundaries (sync, async). Following those calls into the exact wrapped crates showed thatkeyring-core@1.0.0preserves the fullResult<String>contract (source) and the Apple backend preserves OS-status distinctions (mapping), isolating the information loss to this wrapper. ↩