fix(kotlin-sdk): android host-app integration fixes (key security policy, unmanaged-identity reads, typed signing error)#4060
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe Kotlin SDK adds selectable identity-key security policies, legacy key migration, typed missing-signing-key errors, pending identity-key repair tracking, and managed-identity NotFound translation for Dashpay flows. ChangesIdentity key security
Platform wallet error translation
Pending identity-key repair
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
⛔ Blockers found — Sonnet deferred (commit 4b073c3) |
There was a problem hiding this comment.
Code Review
Source: reviewers = claude general opus (failed); claude security-auditor opus (failed); codex general gpt-5.5; codex security-auditor gpt-5.5; verifier = codex gpt-5.5; specialists = security-auditor; policy gate = review_policy.enforce_backport_prereq_policy.
Full-PR verification found two in-scope blocking correctness issues in the Kotlin SDK identity-key changes. No CodeRabbit findings were present, and there are no out-of-scope observations to preserve.
Findings
-
[BLOCKING]
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt:224-226
Default key policy deletes the existing identity-key wrapping key.The default
AUTH_GATEDpath still usesKEYS_ALIAS, but this code treats an existing AESSecretKeyat that alias as a stale/wrong-type entry and deletes it before generating the RSA keypair. Existing installs have DataStore private-key blobs encrypted with that AES key; once the alias is deleted,retrievePrivateKeyuses the new RSA private key against the old AES-GCM blob and cannot recover it. The PR documents the default as preserving historical behavior, so upgrade must either use a new RSA alias with AES fallback/migration or re-encrypt existing blobs before deleting the old alias. -
[BLOCKING]
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:284-289
Successful key repair leavespendingIdentityKeysstale.repairIdentityKeywrites the repaired private key directly throughidentityKeyDeriver.deriveAndStore, butpendingIdentityKeysis owned byPlatformWalletPersistenceHandlerand is only cleared by the privateclearPendingIdentityKeypath during a later successfulonPersistIdentityKeyUpsert. A host watching the newpendingIdentityKeysflow will continue to see a repaired key as pending unless another persistence upsert happens for the same key, which is not guaranteed by the repair action. Route repair through the persistence handler or expose a success path that clears the pending entry whenderiveAndStorereturns a storage identifier.
Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt:224-226: Default key policy deletes the existing identity-key wrapping key
The default AUTH_GATED path still uses KEYS_ALIAS, but this code treats an existing AES SecretKey at that alias as a stale/wrong-type entry and deletes it before generating the RSA keypair. Existing installs have DataStore private-key blobs encrypted with that AES key; once the alias is deleted, retrievePrivateKey uses the new RSA private key against the old AES-GCM blob and cannot recover it. The PR documents the default as preserving historical behavior, so upgrade must either use a new RSA alias with AES fallback/migration or re-encrypt existing blobs before deleting the old alias.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:284-289: Successful key repair leaves pendingIdentityKeys stale
repairIdentityKey writes the repaired private key directly through identityKeyDeriver.deriveAndStore, but pendingIdentityKeys is owned by PlatformWalletPersistenceHandler and is only cleared by the private clearPendingIdentityKey path during a later successful onPersistIdentityKeyUpsert. A host watching the new pendingIdentityKeys flow will continue to see a repaired key as pending unless another persistence upsert happens for the same key, which is not guaranteed by the repair action. Route repair through the persistence handler or expose a success path that clears the pending entry when deriveAndStore returns a storage identifier.
| // Absent, or a stale symmetric entry from an earlier build (which | ||
| // gated encrypt too and broke unprompted writes) — drop and recreate. | ||
| runCatching { keyStore.deleteEntry(KEYS_ALIAS) } | ||
| runCatching { keyStore.deleteEntry(alias) } |
There was a problem hiding this comment.
🔴 Blocking: Default key policy deletes the existing identity-key wrapping key
The default AUTH_GATED path still uses KEYS_ALIAS, but this code treats an existing AES SecretKey at that alias as a stale/wrong-type entry and deletes it before generating the RSA keypair. Existing installs have DataStore private-key blobs encrypted with that AES key; once the alias is deleted, retrievePrivateKey uses the new RSA private key against the old AES-GCM blob and cannot recover it. The PR documents the default as preserving historical behavior, so upgrade must either use a new RSA alias with AES fallback/migration or re-encrypt existing blobs before deleting the old alias.
source: ['codex']
There was a problem hiding this comment.
Resolved in d3aa786 — Default key policy deletes the existing identity-key wrapping key no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-Forward Prior Findings
Both prior blocking findings are FIXED: the legacy AES alias is retained, and successful repairs clear their pending-key entries.
New Findings In Latest Delta
The latest delta introduces one blocking RSA-alias migration regression and one concurrent StateFlow lost-update risk. The cumulative review also found a blocking error-translation defect in the preceding Dashpay commit.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 1): reviewers codex/general=gpt-5.6-sol(completed); sonnet5/general=claude-sonnet-5(completed); codex/security-auditor=gpt-5.6-sol(completed); sonnet5/security-auditor=claude-sonnet-5(completed); verifier=verifier-codex-fallback-4060-1783736084=gpt-5.6-sol fallback; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🔴 2 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt:143-159: RSA keys stored under the previous alias are stranded
The PR base already stored empty-IV RSA/OAEP blobs under `KeystoreManager.KEYS_ALIAS`. The latest delta moves AUTH_GATED operations to `KEYS_ALIAS_AUTH_GATED`, but only non-empty-IV AES blobs use the legacy fallback. Existing empty-IV blobs therefore reach line 159 and are decrypted with a newly generated key at the new alias instead of the RSA private key that remains at `KEYS_ALIAS`, making previously stored identity keys unusable after upgrade. `isPrivateKeyDecryptable` also reports these blobs as healthy based only on their shape. Detect whether `KEYS_ALIAS` contains the former RSA keypair, decrypt matching blobs with it, and migrate successful decryptions to the selected policy alias.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.kt:560-566: Invalid wallet handles are translated into unmanaged identities
This catches every platform-wallet `NotFound` result and converts it to a zero identity handle. However, `platform_wallet_get_managed_identity` applies the same blanket Option-to-NotFound conversion both when `PLATFORM_WALLET_STORAGE.with_item` cannot find `wallet_handle` and when the nested managed-identity lookup misses. A `Dashpay` instance can retain its captured handle after its owning `ManagedPlatformWallet` is closed, so calls through that stale instance return null, empty collections, or false instead of propagating the native wallet failure. This contradicts the commit's guarantee that non-identity-miss errors remain untouched. Give the native lookup distinct errors for an invalid wallet and a missing managed identity, then translate only the latter.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:2135-2142: Concurrent pending-key updates can overwrite each other
`recordPendingIdentityKey` and `clearPendingIdentityKey` perform non-atomic read-modify-write operations on `MutableStateFlow.value`. The persistence callback runs synchronously on its Rust caller thread, while the newly added `markIdentityKeyRepaired` can be invoked from an arbitrary host thread through `repairIdentityKey`. If a repair clears key A while a persistence callback records key B, both can read the same map and one write can discard the other. Losing the record leaves a watch-only key without the queryable pending state this PR adds; losing the clear leaves a repaired key stale. Use `MutableStateFlow.update` or another atomic synchronization mechanism for both mutations.
| if (keystore.isLegacyKeysBlob(blob)) { | ||
| // Legacy AES-GCM blob: recover with the retained legacy key (may | ||
| // throw UserNotAuthenticatedException — the legacy key was | ||
| // auth-gated — which the signer handles exactly as the RSA path), | ||
| // or null if that key is already gone (unrecoverable). | ||
| val plain = keystore.decryptLegacyKeysBlob(blob) ?: return null | ||
| // Opportunistically migrate to the RSA scheme. A rewrite failure | ||
| // must not lose the value we just recovered, so it stays best-effort | ||
| // and the read still returns the plaintext (migration retries next | ||
| // read). | ||
| runCatching { | ||
| val migrated = keystore.encrypt(plain, alias = keystore.keysAlias) | ||
| store.edit { it[privateKeyKey(pubkeyHex)] = encode(migrated) } | ||
| } | ||
| return plain | ||
| } | ||
| return keystore.decrypt(blob, alias = keystore.keysAlias) |
There was a problem hiding this comment.
🔴 Blocking: RSA keys stored under the previous alias are stranded
The PR base already stored empty-IV RSA/OAEP blobs under KeystoreManager.KEYS_ALIAS. The latest delta moves AUTH_GATED operations to KEYS_ALIAS_AUTH_GATED, but only non-empty-IV AES blobs use the legacy fallback. Existing empty-IV blobs therefore reach line 159 and are decrypted with a newly generated key at the new alias instead of the RSA private key that remains at KEYS_ALIAS, making previously stored identity keys unusable after upgrade. isPrivateKeyDecryptable also reports these blobs as healthy based only on their shape. Detect whether KEYS_ALIAS contains the former RSA keypair, decrypt matching blobs with it, and migrate successful decryptions to the selected policy alias.
source: ['codex-general', 'codex-security-auditor']
There was a problem hiding this comment.
Resolved in f8dd367 — RSA keys stored under the previous alias are stranded no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| internal inline fun translateManagedIdentityNotFoundToZero(getHandle: () -> Long): Long = | ||
| try { | ||
| getHandle() | ||
| } catch (e: DashSDKException) { | ||
| val notFound = DashSdkError.PLATFORM_WALLET_CODE_OFFSET + | ||
| DashSdkError.PLATFORM_WALLET_NOT_FOUND_CODE | ||
| if (e.code == notFound) 0L else throw e |
There was a problem hiding this comment.
🔴 Blocking: Invalid wallet handles are translated into unmanaged identities
This catches every platform-wallet NotFound result and converts it to a zero identity handle. However, platform_wallet_get_managed_identity applies the same blanket Option-to-NotFound conversion both when PLATFORM_WALLET_STORAGE.with_item cannot find wallet_handle and when the nested managed-identity lookup misses. A Dashpay instance can retain its captured handle after its owning ManagedPlatformWallet is closed, so calls through that stale instance return null, empty collections, or false instead of propagating the native wallet failure. This contradicts the commit's guarantee that non-identity-miss errors remain untouched. Give the native lookup distinct errors for an invalid wallet and a missing managed identity, then translate only the latter.
source: ['codex-general']
There was a problem hiding this comment.
Resolved in f8dd367 — Invalid wallet handles are translated into unmanaged identities no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| private fun recordPendingIdentityKey(entry: PendingIdentityKey) { | ||
| _pendingIdentityKeys.value = _pendingIdentityKeys.value + (entry.publicKeyHex to entry) | ||
| } | ||
|
|
||
| private fun clearPendingIdentityKey(publicKeyHex: String) { | ||
| if (publicKeyHex in _pendingIdentityKeys.value) { | ||
| _pendingIdentityKeys.value = _pendingIdentityKeys.value - publicKeyHex | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Concurrent pending-key updates can overwrite each other
recordPendingIdentityKey and clearPendingIdentityKey perform non-atomic read-modify-write operations on MutableStateFlow.value. The persistence callback runs synchronously on its Rust caller thread, while the newly added markIdentityKeyRepaired can be invoked from an arbitrary host thread through repairIdentityKey. If a repair clears key A while a persistence callback records key B, both can read the same map and one write can discard the other. Losing the record leaves a watch-only key without the queryable pending state this PR adds; losing the clear leaves a repaired key stale. Use MutableStateFlow.update or another atomic synchronization mechanism for both mutations.
source: ['sonnet5-security-auditor']
There was a problem hiding this comment.
Resolved in f8dd367 — Concurrent pending-key updates can overwrite each other no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
|
Both confirmed and fixed. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt (1)
806-873: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDefer pending-state updates until the Room write runs
clearPendingIdentityKey/recordPendingIdentityKeyhappen beforestage(walletId) { ... }, so any rollback or transaction failure leavespendingIdentityKeysout of sync with the row it describes. That can clear a failed key too early or leave a stale pending entry after a write that never commits. Move the map update into the staged write so it shares the row’s transactional lifetime.🤖 Prompt for 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. In `@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt` around lines 806 - 873, The pending-state mutations in the private-key persistence flow around deriver.deriveAndStore, clearPendingIdentityKey, and recordPendingIdentityKey currently occur before the staged Room write. Defer both updates into the stage(walletId) write callback, applying the clear only after a successful derive and the pending record for a failed derive within the same staged operation, so rollback or transaction failure leaves pendingIdentityKeys consistent with the persisted row.
🧹 Nitpick comments (1)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.kt (1)
430-446: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe KDoc for
withManagedIdentityis orphaned.The existing KDoc (lines 424–429) describes
withManagedIdentitybut is now separated from it by the newmanagedIdentityHandleOrZerofunction and its own KDoc. In Kotlin, only the KDoc immediately before a declaration is associated with it, sowithManagedIdentityloses its documentation.♻️ Proposed fix: move the orphaned KDoc to its correct position
/** - * Open the managed-identity handle for [identityId], run [block], - * and destroy the handle before returning (the [contacts] / - * [acceptIncomingRequest] discipline). Returns null when the - * identity isn't managed by this wallet. - */ - /** * Snapshot the managed identity for [identityId], or 0 when the wallet * does not manage it. The native side reports an unmanaged identity as * a platform-wallet NotFound error rather than a zero handle, so the * "not managed" outcome is translated here — every local-read caller * treats it as an absence (null / empty / false), never an exception. */ private fun managedIdentityHandleOrZero(identityId: ByteArray): Long = translateManagedIdentityNotFoundToZero { TokensNative.getManagedIdentity(walletHandle, identityId) } + /** + * Open the managed-identity handle for [identityId], run [block], + * and destroy the handle before returning (the [contacts] / + * [acceptIncomingRequest] discipline). Returns null when the + * identity isn't managed by this wallet. + */ private inline fun <T> withManagedIdentity( identityId: ByteArray, block: (Long) -> T, ): T? {🤖 Prompt for 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. In `@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.kt` around lines 430 - 446, Move the existing KDoc describing withManagedIdentity so it is immediately before the withManagedIdentity declaration, after managedIdentityHandleOrZero and its KDoc. Keep the managedIdentityHandleOrZero documentation attached to that function and preserve both documentation contents.
🤖 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.
Outside diff comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- Around line 806-873: The pending-state mutations in the private-key
persistence flow around deriver.deriveAndStore, clearPendingIdentityKey, and
recordPendingIdentityKey currently occur before the staged Room write. Defer
both updates into the stage(walletId) write callback, applying the clear only
after a successful derive and the pending record for a failed derive within the
same staged operation, so rollback or transaction failure leaves
pendingIdentityKeys consistent with the persisted row.
---
Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.kt`:
- Around line 430-446: Move the existing KDoc describing withManagedIdentity so
it is immediately before the withManagedIdentity declaration, after
managedIdentityHandleOrZero and its KDoc. Keep the managedIdentityHandleOrZero
documentation attached to that function and preserve both documentation
contents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c43cab53-9e7f-44a1-9a3b-ca57d7cb7970
📒 Files selected for processing (12)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicy.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreSigner.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicyTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/tokens/ManagedIdentityNotFoundTranslationTest.kt
…d wallet handle; atomic pending-key updates Addresses the latest review on dashpay#4060. WalletStorage/KeystoreManager (BLOCKING): the alias split routed only non-empty-IV AES blobs to the legacy fallback, so empty-IV RSA blobs written by the pre-alias-split scheme under KEYS_ALIAS were decrypted against a NEWLY generated key at the policy alias and stranded. retrievePrivateKey now recovers them with the retained former RSA keypair at KEYS_ALIAS (an upgrade fast path when the policy alias is unprovisioned, else a wrong-key catch-fallback) and migrates the value forward; UserNotAuthenticatedException still propagates so the signer can prompt. isPrivateKeyDecryptable now reflects real key presence (policy-alias OR former-RSA OR legacy-AES key), not blob shape alone. The relevant KeystoreManager methods are `open` so the routing can be unit-tested without AndroidKeyStore (no Robolectric provider), covering the full matrix: legacy-AES, legacy-RSA-at-KEYS_ALIAS, new-alias, stranded, mixed-window, and auth-propagation. dashpay.rs (BLOCKING): platform_wallet_get_managed_identity flattened both the invalid-wallet-handle (outer with_item miss) and identity-not-managed (inner miss) into NotFound, which the Kotlin Dashpay layer translates to an unmanaged (zero) handle — so a stale Dashpay instance masked a wallet failure as "unmanaged". The outer miss now returns ErrorInvalidHandle (untranslated, surfaces as PlatformWallet.InvalidHandle); only the inner miss stays NotFound. PlatformWalletPersistenceHandler (suggestion): recordPendingIdentityKey / clearPendingIdentityKey now use MutableStateFlow.update so a persistence-thread record and a host-thread repair-clear cannot drop each other's mutation. Also moves the orphaned withManagedIdentity KDoc onto its declaration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bfoss765
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-Forward Prior Findings
All three prior findings are FIXED at f8dd367, so none are carried forward.
New Findings In Latest Delta
The alias-recovery fast path introduces one blocking wrong-key routing bug, and the new native dual-error-code path lacks direct Rust boundary coverage. No additional cumulative finding survived scope validation.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 3): reviewers codex/general=gpt-5.6-sol (parseable output, wrapper exit 5); sonnet5/general=claude-sonnet-5 (parseable output, wrapper terminated after idle); verifier=verifier-sonnet5-4060-1783793326/claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt:170-174: RSA-blob alias inference from unrelated key presence throws uncaught BadPaddingException
This fast path assumes `!hasIdentityKeysKey(keysAlias) && hasLegacyRsaKeysKey()` proves the blob was encrypted by the retained pre-alias-split keypair. That is not true when an install has both un-migrated pre-split blobs and newer blobs written under another post-split policy alias. After an AUTH_GATED→DEVICE_BOUND switch, a `KEYS_ALIAS_AUTH_GATED` blob can enter this branch merely because an unrelated legacy RSA key still exists; `decryptLegacyRsaKeysBlob` then calls `doFinal` with the wrong private key and throws `BadPaddingException` uncaught. The identical presence check in `isPrivateKeyDecryptable` can also report the blob healthy, preventing the documented repair path. Catch `GeneralSecurityException` on this fast path and continue through the normal alias/repair handling, or persist an alias discriminator instead of inferring origin from key presence.
In `packages/rs-platform-wallet-ffi/src/dashpay.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay.rs:69-98: No Rust unit test exercises platform_wallet_get_managed_identity's new dual error codes
`platform_wallet_get_managed_identity` now deliberately returns `ErrorInvalidHandle` for an unknown wallet handle versus `NotFound` for a valid wallet lacking the managed identity—the exact distinction prior-2 required. The existing test module covers unknown-handle behavior in other functions, but no test calls this function directly. The Kotlin `ManagedIdentityNotFoundTranslationTest` uses synthetic exceptions and does not exercise the FFI boundary. Add a Rust test asserting both error-code outcomes.
| if (!keystore.hasIdentityKeysKey(keystore.keysAlias) && keystore.hasLegacyRsaKeysKey()) { | ||
| val recovered = keystore.decryptLegacyRsaKeysBlob(blob) ?: return null | ||
| migrateToPolicyAlias(pubkeyHex, recovered) | ||
| return recovered | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: RSA-blob alias inference from unrelated key presence throws uncaught BadPaddingException
This fast path assumes !hasIdentityKeysKey(keysAlias) && hasLegacyRsaKeysKey() proves the blob was encrypted by the retained pre-alias-split keypair. That is not true when an install has both un-migrated pre-split blobs and newer blobs written under another post-split policy alias. After an AUTH_GATED→DEVICE_BOUND switch, a KEYS_ALIAS_AUTH_GATED blob can enter this branch merely because an unrelated legacy RSA key still exists; decryptLegacyRsaKeysBlob then calls doFinal with the wrong private key and throws BadPaddingException uncaught. The identical presence check in isPrivateKeyDecryptable can also report the blob healthy, preventing the documented repair path. Catch GeneralSecurityException on this fast path and continue through the normal alias/repair handling, or persist an alias discriminator instead of inferring origin from key presence.
source: ['codex-general']
There was a problem hiding this comment.
Partially addressed in 374d023, but still valid in narrowed form: the uncaught BadPaddingException path is fixed; isPrivateKeyDecryptable still treats unrelated RSA-key presence as proof that the specific blob is recoverable. Carried forward in the current review at WalletStorage.kt:257-265.
Corrected after exact-head verification; this thread should remain open.
There was a problem hiding this comment.
Both halves are now closed: the uncaught BadPaddingException in retrievePrivateKey was fixed in 374d023, and the narrowed still-open half — isPrivateKeyDecryptable inferring recoverability from unrelated-key presence — is fixed in 1fcd60e (tracked on the escalated thread e17e265dc680). It now probes actual decryptability against the specific blob; see that thread for detail and the WalletStorageUpgradeMatrixTest coverage (12/12 green).
| let info = wm.get_wallet_info(&wallet.wallet_id())?; | ||
| info.identity_manager.managed_identity(&id).cloned() | ||
| }); | ||
| let inner = unwrap_option_or_return!(option); | ||
| // Two distinct "None" outcomes must NOT collapse into one `NotFound` | ||
| // (dashpay/platform#4060): the OUTER `with_item` miss means `wallet_handle` | ||
| // is absent from `PLATFORM_WALLET_STORAGE` — a closed/stale wallet, i.e. a | ||
| // real wallet failure — whereas the INNER miss means the (valid) wallet | ||
| // simply does not manage `id`. The Kotlin `Dashpay` layer translates only | ||
| // the inner "not managed" miss into a zero handle | ||
| // (`translateManagedIdentityNotFoundToZero`); if the invalid-handle case | ||
| // also arrived as `NotFound`, a stale `Dashpay` instance would silently | ||
| // read as "unmanaged" instead of surfacing the wallet failure. Emit | ||
| // `ErrorInvalidHandle` for the outer miss so it stays untranslated, and | ||
| // keep the inner miss as `NotFound`. | ||
| let Some(inner) = option else { | ||
| return PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorInvalidHandle, | ||
| format!("platform wallet handle {wallet_handle} not found"), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🟡 Suggestion: No Rust unit test exercises platform_wallet_get_managed_identity's new dual error codes
platform_wallet_get_managed_identity now deliberately returns ErrorInvalidHandle for an unknown wallet handle versus NotFound for a valid wallet lacking the managed identity—the exact distinction prior-2 required. The existing test module covers unknown-handle behavior in other functions, but no test calls this function directly. The Kotlin ManagedIdentityNotFoundTranslationTest uses synthetic exceptions and does not exercise the FFI boundary. Add a Rust test asserting both error-code outcomes.
source: ['sonnet5-general']
There was a problem hiding this comment.
Partially addressed in 374d023, but still valid in narrowed form: the new Rust test covers ErrorInvalidHandle, while the valid-wallet NotFound outcome remains untested at the native FFI boundary. Carried forward in the current review at dashpay.rs:1034-1058.
Corrected after exact-head verification; this thread should remain open.
There was a problem hiding this comment.
Substantially addressed in 82c682cf1d. Splitting the dual-error decision into a pure, generic classify_managed_identity_outcome made both codes directly unit-testable at the exact seam where they previously collapsed. classify_managed_identity_outcome_distinguishes_removed_wallet_from_unmanaged now asserts:
Some(Some(None))(valid wallet, identity not managed) →NotManaged→NotFound— the valid-walletNotFoundoutcome this suggestion asked for;Some(None)(wallet removed from the manager map) and outerNone→InvalidHandle→ErrorInvalidHandle;Some(Some(Some(_)))→Found.
The full FFI path for the outer-None arm remains covered by get_managed_identity_unknown_wallet_is_invalid_handle. A fully-seeded PlatformWallet fixture (SDK + all sub-wallets) still isn't buildable in this unit-test module, so the end-to-end Ok/native-storage path stays out of scope here — but the load-bearing dual-code discrimination is now tested directly in Rust, so a re-collapse fails a unit test rather than slipping past the synthetic Kotlin translation test.
|
Both blocking findings fixed. (a) Empty-IV RSA blobs written under the former |
… retrievePrivateKey Addresses the latest review on dashpay#4060. WalletStorage.retrievePrivateKey (BLOCKING, finding 41026612bc10): the empty-IV RSA upgrade fast path inferred blob origin from key *presence* (!hasIdentityKeysKey(policyAlias) && hasLegacyRsaKeysKey()) and then called decryptLegacyRsaKeysBlob with no crypto-failure handling. Presence is not proof of origin: after an AUTH_GATED->DEVICE_BOUND policy switch, a blob written under a sibling policy alias can enter this branch merely because an unrelated legacy RSA key still lingers at KEYS_ALIAS — decryptLegacyRsaKeysBlob then does a doFinal with the wrong private key and threw a BadPaddingException that escaped uncaught into KeystoreSigner (which catches only UserNotAuthenticatedException). Every candidate key is now TRIED and a wrong-key GeneralSecurityException is absorbed as "not this key" (per the KeystoreManager.decryptLegacyRsaKeysBlob contract) via the new tryFormerRsaRecovery helper, used by both the fast path and the steady-state catch fallback. A blob no present key can open is now unrecoverable -> null (key-health then offers a re-derive), mirroring the legacy-AES stranded path — never a bogus plaintext, never an uncaught crypto exception. UserNotAuthenticatedException still propagates so the signer can prompt and retry. Tests: WalletStorageUpgradeMatrixTest gains siblingAliasBlobUnderUnprovisioned- PolicyDoesNotThrow (former RSA key present, policy unprovisioned, sibling-alias blob -> null, not a thrown BadPaddingException — this failed before the fix); formerRsaBlobWithDeletedKeyIsStranded now asserts null (RSA-stranded made consistent with the AES-stranded null contract). Full :sdk suite green (131). dashpay.rs (suggestion d7c06333de19): add get_managed_identity_unknown_wallet_- is_invalid_handle asserting the outer with_item miss returns ErrorInvalidHandle (not NotFound), locking in the dual-error contract the invalid-handle fix relies on. The inner not-managed -> NotFound outcome needs a seeded-wallet fixture this unit-test module lacks; it stays covered by the Kotlin translation test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review round addressed in Dashpay.kt — invalid handles read as unmanaged (67d3d6c5b3ad): verified this is already resolved by WalletStorage.kt — RSA-blob alias inference throws (41026612bc10): good catch — the empty-IV RSA fast path was trusting key presence to prove blob origin, so a sibling-policy-alias blob (e.g. an AUTH_GATED blob read after a DEVICE_BOUND switch) with an unrelated former RSA key present hit Residual worth noting: |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-Forward Prior Findings
Both prior findings are STILL VALID, each now only partially addressed. Prior-1 originally covered two consequences of the same root cause (RSA-blob alias inferred from unrelated key presence): the uncaught BadPaddingException crash in retrievePrivateKey is fixed via the new tryFormerRsaRecovery helper, verified by siblingAliasBlobUnderUnprovisionedPolicyDoesNotThrow; but isPrivateKeyDecryptable (WalletStorage.kt:257-265), explicitly named in the same original finding, still infers recoverability from key presence rather than actual decryptability, so it can report a stranded sibling-alias blob as healthy and suppress the PR's own repair UI. Prior-2 (missing Rust FFI test for the dual error-code contract) is half-closed: a direct test now pins the ErrorInvalidHandle/outer-miss path, but the NotFound/inner-miss path is still untested at the real FFI boundary — the only existing coverage (ManagedIdentityNotFoundTranslationTest) throws a synthetic exception and never calls the native function.
New Findings In Latest Delta
No new defect was introduced by the latest delta itself (f8dd367..374d023), which only touches the RSA-recovery fast path and its tests. However, cumulative re-review of code this PR added (not touched by the latest delta) surfaced one new blocking gap: platform_wallet_get_managed_identity (dashpay.rs:77-99) collapses a removed/stale wallet-manager entry into the same NotFound code path as a valid wallet that simply doesn't manage the requested identity, undermining this PR's own stated goal of distinguishing those two cases.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 1): reviewers codex/general=gpt-5.6-sol(completed); sonnet5/general=claude-sonnet-5(completed); codex/security-auditor=gpt-5.6-sol(completed); sonnet5/security-auditor=claude-sonnet-5(completed); codex/ffi-engineer=gpt-5.6-sol(completed_parseable_output_with_nonzero_wrapper_exit); sonnet5/ffi-engineer=claude-sonnet-5(completed); verifier=verifier-sonnet5-4060-1783818311=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🔴 2 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt:257-265: isPrivateKeyDecryptable still infers recoverability from unrelated-key presence, not actual decryptability
This is the still-open half of the originally-posted finding: 'RSA-blob alias inference from unrelated key presence' had two consequences — a crash in `retrievePrivateKey` (now fixed) and a false-healthy verdict in `isPrivateKeyDecryptable` (not fixed). This method returns `keystore.isKeysBlobDecryptable(blob) && (keystore.hasIdentityKeysKey(keystore.keysAlias) || keystore.hasLegacyRsaKeysKey())`. Verified against KeystoreManager.kt: `isKeysBlobDecryptable` (line 138-139) only checks empty-IV + RSA key size (a structural check), and `hasIdentityKeysKey`/`hasLegacyRsaKeysKey` (lines 172-184) only check Keystore key *existence* — neither verifies the specific blob was written by that specific key. For the exact SIBLING_POLICY scenario the new `siblingAliasBlobUnderUnprovisionedPolicyDoesNotThrow` test now exercises for `retrievePrivateKey` (which correctly returns `null`), `isPrivateKeyDecryptable` on the same blob still returns `true`, because the unrelated former RSA key is present and the blob is structurally RSA-shaped. This feeds `WalletKeyHealthSheet.kt`'s `hasPrivateKey` computation for the key-health repair UI, so a genuinely stranded identity key is reported healthy after a `KeySecurityPolicy` switch, and the user is never offered the re-derive/repair path this PR's own KDoc says the check exists to drive. No test covers `isPrivateKeyDecryptable` under `Scheme.SIBLING_POLICY`.
In `packages/rs-platform-wallet-ffi/src/dashpay.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/dashpay.rs:77-99: A wallet removed from the manager but not yet handle-destroyed still collapses into 'unmanaged identity' NotFound
`platform_wallet_get_managed_identity`'s own doc comment (lines 82-92) states its purpose is to distinguish a stale/invalid `wallet_handle` (`ErrorInvalidHandle`) from a valid wallet that simply doesn't manage the identity (`NotFound`) — precisely so Kotlin's `translateManagedIdentityNotFoundToZero` (Dashpay.kt:563-570) doesn't silently swallow a real wallet failure. But the `?` on `wm.get_wallet_info(&wallet.wallet_id())` (line 79) causes the closure to return `None` early whenever the wallet's info has been removed from the shared `WalletManager` map — the exact same `None` the closure produces when the wallet legitimately doesn't manage `identity_id`. Both collapse into the same inner `Option::None` at line 93, and both become `NotFound` → translated to a zero handle by Kotlin, masquerading as 'identity not managed' instead of surfacing the wallet-removed failure. This state is reachable: `PlatformWalletManager.kt`'s `removeWallet` (lines 531-564) calls `WalletManagerNative.removeWallet` (step 2, which calls `platform_wallet_manager_remove_wallet` → `manager.remove_wallet`, clearing the shared `WalletManager`'s entry for that `wallet_id`) *before* closing/destroying the `ManagedPlatformWallet` wrapper (step 3, which is what actually invalidates the `PLATFORM_WALLET_STORAGE` handle `platform_wallet_get_managed_identity` checks). Any `Dashpay` instance holding that still-open `wallet_handle` during this window — or more generally any caller racing a concurrent wallet removal — gets a false-negative 'not managed' result instead of the wallet-failure signal this PR was written to preserve. Handle the missing-`wallet_info` case as its own distinct outcome (e.g. `ErrorInvalidHandle` or a dedicated code) rather than folding it into the same `None` slot as the inner managed-identity lookup.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay.rs:1034-1058: platform_wallet_get_managed_identity's NotFound (inner-miss) path still has no direct Rust FFI test
The new `get_managed_identity_unknown_wallet_is_invalid_handle` test (added this delta) exercises only the outer `with_item` miss (unknown `wallet_handle` → `ErrorInvalidHandle`) — real, verified progress on the original suggestion. The complementary and equally load-bearing inner outcome — a valid wallet handle that simply does not manage the requested `identity_id` → `NotFound` — still has no fixture at the Rust/FFI boundary; the test module's own doc comment (lines 1044-1048) explicitly defers this to the Kotlin `ManagedIdentityNotFoundTranslationTest`. Verified: that Kotlin test only throws a synthetic `DashSDKException(notFoundCode, ...)` directly (lines 24-32) — it never calls `platform_wallet_get_managed_identity`, never touches `PLATFORM_WALLET_STORAGE`/`MANAGED_IDENTITY_STORAGE`, and so cannot catch a regression in the real native `Option` handling (e.g. `unwrap_option_or_return!(inner)` at dashpay.rs:99) that would silently re-collapse the two error codes this whole PR was written to separate.
| suspend fun isPrivateKeyDecryptable(pubkeyHex: String): Boolean { | ||
| val encoded = store.data.first()[privateKeyKey(pubkeyHex)] ?: return false | ||
| return keystore.isKeysBlobDecryptable(decode(encoded)) | ||
| val blob = decode(encoded) | ||
| if (keystore.isLegacyKeysBlob(blob)) { | ||
| return keystore.hasLegacyKeysKey() | ||
| } | ||
| return keystore.isKeysBlobDecryptable(blob) && | ||
| (keystore.hasIdentityKeysKey(keystore.keysAlias) || keystore.hasLegacyRsaKeysKey()) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: isPrivateKeyDecryptable still infers recoverability from unrelated-key presence, not actual decryptability
This is the still-open half of the originally-posted finding: 'RSA-blob alias inference from unrelated key presence' had two consequences — a crash in retrievePrivateKey (now fixed) and a false-healthy verdict in isPrivateKeyDecryptable (not fixed). This method returns keystore.isKeysBlobDecryptable(blob) && (keystore.hasIdentityKeysKey(keystore.keysAlias) || keystore.hasLegacyRsaKeysKey()). Verified against KeystoreManager.kt: isKeysBlobDecryptable (line 138-139) only checks empty-IV + RSA key size (a structural check), and hasIdentityKeysKey/hasLegacyRsaKeysKey (lines 172-184) only check Keystore key existence — neither verifies the specific blob was written by that specific key. For the exact SIBLING_POLICY scenario the new siblingAliasBlobUnderUnprovisionedPolicyDoesNotThrow test now exercises for retrievePrivateKey (which correctly returns null), isPrivateKeyDecryptable on the same blob still returns true, because the unrelated former RSA key is present and the blob is structurally RSA-shaped. This feeds WalletKeyHealthSheet.kt's hasPrivateKey computation for the key-health repair UI, so a genuinely stranded identity key is reported healthy after a KeySecurityPolicy switch, and the user is never offered the re-derive/repair path this PR's own KDoc says the check exists to drive. No test covers isPrivateKeyDecryptable under Scheme.SIBLING_POLICY.
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in 1fcd60e — isPrivateKeyDecryptable still infers recoverability from unrelated-key presence, not actual decryptability no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Fixed and verified at branch head (commit 1fcd60e, already on fix/kotlin-sdk-android-integration); the auto-resolve on this thread was correct.
isPrivateKeyDecryptable no longer infers recoverability from key presence. It now PROBES the same candidate keys retrievePrivateKey uses (the policy alias if provisioned, then the former KEYS_ALIAS RSA keypair; legacy AES for GCM blobs) via a prompt-free probeOpensBlob, returning true only when a present key actually opens the blob. A wrong-key crypto failure (BadPadding/AEAD tag) or an absent key → false; a UserNotAuthenticatedException (auth-gated key, closed window) → true (present, recovers after auth — a health check must not report it strandable). Recovered plaintext is scrubbed immediately.
Tests (WalletStorageUpgradeMatrixTest, :sdk:testDebugUnitTest, 12/12 green):
siblingAliasBlobUnderUnprovisionedPolicyDoesNotThrownow also assertsisPrivateKeyDecryptable == false— the exact SIBLING_POLICY unrelated-RSA-present case (returnedtruebefore the fix).formerRsaBlobEmptyPolicyTakesFastPathAndMigrates/legacyAesBlobIsRecoveredAndMigratedcover the genuine pre-split legacy blob →true.authGatedPolicyKeyReportsDecryptableWithoutPrompting/authGatedFormerRsaKeyReportsDecryptablepin theUserNotAuth→ decryptable (no-prompt) semantics.
| let info = wm.get_wallet_info(&wallet.wallet_id())?; | ||
| info.identity_manager.managed_identity(&id).cloned() | ||
| }); | ||
| let inner = unwrap_option_or_return!(option); | ||
| // Two distinct "None" outcomes must NOT collapse into one `NotFound` | ||
| // (dashpay/platform#4060): the OUTER `with_item` miss means `wallet_handle` | ||
| // is absent from `PLATFORM_WALLET_STORAGE` — a closed/stale wallet, i.e. a | ||
| // real wallet failure — whereas the INNER miss means the (valid) wallet | ||
| // simply does not manage `id`. The Kotlin `Dashpay` layer translates only | ||
| // the inner "not managed" miss into a zero handle | ||
| // (`translateManagedIdentityNotFoundToZero`); if the invalid-handle case | ||
| // also arrived as `NotFound`, a stale `Dashpay` instance would silently | ||
| // read as "unmanaged" instead of surfacing the wallet failure. Emit | ||
| // `ErrorInvalidHandle` for the outer miss so it stays untranslated, and | ||
| // keep the inner miss as `NotFound`. | ||
| let Some(inner) = option else { | ||
| return PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorInvalidHandle, | ||
| format!("platform wallet handle {wallet_handle} not found"), | ||
| ); | ||
| }; | ||
| let managed = unwrap_option_or_return!(inner); |
There was a problem hiding this comment.
🔴 Blocking: A wallet removed from the manager but not yet handle-destroyed still collapses into 'unmanaged identity' NotFound
platform_wallet_get_managed_identity's own doc comment (lines 82-92) states its purpose is to distinguish a stale/invalid wallet_handle (ErrorInvalidHandle) from a valid wallet that simply doesn't manage the identity (NotFound) — precisely so Kotlin's translateManagedIdentityNotFoundToZero (Dashpay.kt:563-570) doesn't silently swallow a real wallet failure. But the ? on wm.get_wallet_info(&wallet.wallet_id()) (line 79) causes the closure to return None early whenever the wallet's info has been removed from the shared WalletManager map — the exact same None the closure produces when the wallet legitimately doesn't manage identity_id. Both collapse into the same inner Option::None at line 93, and both become NotFound → translated to a zero handle by Kotlin, masquerading as 'identity not managed' instead of surfacing the wallet-removed failure. This state is reachable: PlatformWalletManager.kt's removeWallet (lines 531-564) calls WalletManagerNative.removeWallet (step 2, which calls platform_wallet_manager_remove_wallet → manager.remove_wallet, clearing the shared WalletManager's entry for that wallet_id) before closing/destroying the ManagedPlatformWallet wrapper (step 3, which is what actually invalidates the PLATFORM_WALLET_STORAGE handle platform_wallet_get_managed_identity checks). Any Dashpay instance holding that still-open wallet_handle during this window — or more generally any caller racing a concurrent wallet removal — gets a false-negative 'not managed' result instead of the wallet-failure signal this PR was written to preserve. Handle the missing-wallet_info case as its own distinct outcome (e.g. ErrorInvalidHandle or a dedicated code) rather than folding it into the same None slot as the inner managed-identity lookup.
source: ['codex']
There was a problem hiding this comment.
Fixed in 82c682cf1d.
The ? on wm.get_wallet_info(&wallet.wallet_id()) inside the with_item closure folded the get_wallet_info miss into the same closure-None as the managed-identity miss, so a wallet removed from the shared WalletManager map but not yet handle-destroyed collapsed into NotFound → zeroed by Kotlin. The closure now uses .map() so get_wallet_info presence lives in its own Option layer, and a new pure classify_managed_identity_outcome maps the three layers to the contract:
- handle absent from
PLATFORM_WALLET_STORAGE(outerNone) →ErrorInvalidHandle - wallet removed from the manager map (
Some(None)) →ErrorInvalidHandle - valid wallet, identity not managed (
Some(Some(None))) →NotFound - found (
Some(Some(Some(mi)))) →Ok
New unit test classify_managed_identity_outcome_distinguishes_removed_wallet_from_unmanaged covers the removed-but-not-destroyed race and all three arms; full platform-wallet-ffi lib suite green (142 passed).
There was a problem hiding this comment.
Resolved in 82c682c — A wallet removed from the manager but not yet handle-destroyed still collapses into 'unmanaged identity' NotFound no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| /// An unknown `wallet_handle` surfaces `ErrorInvalidHandle` — the OUTER | ||
| /// `with_item` miss — NOT `NotFound` (dashpay/platform#4060). This is the | ||
| /// load-bearing half of the dual-error contract: the Kotlin `Dashpay` layer | ||
| /// translates only `NotFound` (a valid wallet that does not manage the id) | ||
| /// into a zero handle, so a stale/closed wallet MUST arrive as | ||
| /// `ErrorInvalidHandle` to avoid masquerading as an unmanaged identity. The | ||
| /// 32-byte `identity_id` is read before the wallet lookup (`read_identifier`), | ||
| /// so a real buffer is supplied; `out_managed_identity_handle` must be left | ||
| /// untouched on the miss. | ||
| /// | ||
| /// The complementary inner outcome (a valid wallet lacking the managed | ||
| /// identity → `NotFound`) needs a fully seeded wallet in | ||
| /// `PLATFORM_WALLET_STORAGE`, which this unit-test module has no fixture for; | ||
| /// that path is covered at the translation layer by the Kotlin | ||
| /// `ManagedIdentityNotFoundTranslationTest`. | ||
| #[test] | ||
| fn get_managed_identity_unknown_wallet_is_invalid_handle() { | ||
| let id = [0u8; 32]; | ||
| let mut out: Handle = 0; | ||
| let r = unsafe { | ||
| platform_wallet_get_managed_identity(0xDEAD_BEEF, id.as_ptr(), &mut out) | ||
| }; | ||
| assert_eq!(r.code, PlatformWalletFFIResultCode::ErrorInvalidHandle); | ||
| assert_eq!(out, 0, "out handle is untouched on an invalid-handle miss"); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: platform_wallet_get_managed_identity's NotFound (inner-miss) path still has no direct Rust FFI test
The new get_managed_identity_unknown_wallet_is_invalid_handle test (added this delta) exercises only the outer with_item miss (unknown wallet_handle → ErrorInvalidHandle) — real, verified progress on the original suggestion. The complementary and equally load-bearing inner outcome — a valid wallet handle that simply does not manage the requested identity_id → NotFound — still has no fixture at the Rust/FFI boundary; the test module's own doc comment (lines 1044-1048) explicitly defers this to the Kotlin ManagedIdentityNotFoundTranslationTest. Verified: that Kotlin test only throws a synthetic DashSDKException(notFoundCode, ...) directly (lines 24-32) — it never calls platform_wallet_get_managed_identity, never touches PLATFORM_WALLET_STORAGE/MANAGED_IDENTITY_STORAGE, and so cannot catch a regression in the real native Option handling (e.g. unwrap_option_or_return!(inner) at dashpay.rs:99) that would silently re-collapse the two error codes this whole PR was written to separate.
source: ['claude', 'codex']
There was a problem hiding this comment.
The inner-miss NotFound path now has direct Rust coverage as of 82c682cf1d. The dual-error decision was extracted into a pure classify_managed_identity_outcome, and classify_managed_identity_outcome_distinguishes_removed_wallet_from_unmanaged asserts Some(Some(None)) (valid wallet that doesn't manage the id) → NotManaged → NotFound, alongside the two InvalidHandle arms and Found. This closes the regression gap you flagged: a re-collapse of the two codes now fails a Rust unit test at the seam, not just the synthetic Kotlin ManagedIdentityNotFoundTranslationTest. (Same commit as d7c06333de19.)
There was a problem hiding this comment.
Still valid at 28b9811 — the new classifier unit test closes the Option-decision regression, but the exported valid-wallet/unmanaged-identity NotFound path still lacks the direct seeded-wallet FFI test originally requested.
There was a problem hiding this comment.
Resolved in this update — platform_wallet_get_managed_identity's NotFound (inner-miss) path still has no direct Rust FFI test no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…not key presence Addresses escalated blocker e17e265dc680 on dashpay#4060 — the still-open half of the RSA-alias-inference finding (the retrievePrivateKey crash was fixed last round; the false-healthy key-health verdict was not). isPrivateKeyDecryptable returned isKeysBlobDecryptable(blob) && (hasIdentityKeysKey || hasLegacyRsaKeysKey) — pure structural + key-*presence* checks that never verified the specific blob was written by a present key. For the SIBLING_POLICY scenario (a blob written under a sibling policy alias while an unrelated former RSA key lingers at KEYS_ALIAS) it reported the stranded key "healthy", so WalletKeyHealthSheet never offered the re-derive/repair path the check exists to drive. It now PROBES the same candidate keys retrievePrivateKey uses (policy alias if provisioned, then the former KEYS_ALIAS RSA keypair; legacy AES for GCM blobs) and returns true only when a present key actually opens the blob. The probe never prompts: KeystoreManager.decrypt / decryptLegacyRsaKeysBlob / decryptLegacyKeysBlob are bare Cipher ops (BiometricPrompt is driven only by KeystoreSigner/BiometricGate, never here), so an auth-gated key with a closed window throws UserNotAuthenticatedException rather than showing UI — which counts as DECRYPTABLE (present + would recover after auth; a health check must not report it strandable). Only a wrong-key crypto failure (BadPadding/AEAD) or an absent key is "not decryptable". Recovered plaintext is scrubbed immediately. Tests: siblingAlias… now also asserts isPrivateKeyDecryptable == false (the fix; true before). Adds authGatedPolicyKeyReportsDecryptableWithoutPrompting and authGatedFormerRsaKeyReportsDecryptable (UserNotAuth -> decryptable), with a throwAuthOnLegacyRsaDecrypt toggle on the fake. Full :sdk suite green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-Forward Prior Findings
Both reviewers' two prior blocking/suggestion findings against dashpay.rs are untouched by this delta (which only edits WalletStorage.kt and its test) and remain valid on direct re-read: a wallet removed from WalletManager but not yet handle-destroyed still collapses into 'unmanaged identity' NotFound (blocking), and the native NotFound inner-miss path still has no direct Rust FFI test (suggestion).
New Findings In Latest Delta
The delta correctly fixes prior-1 — isPrivateKeyDecryptable now probes real decryptability via probeOpensBlob instead of inferring health from unrelated-key presence, with solid new regression coverage for the sibling-alias and auth-gated-window cases. However, verification surfaced a new gap in that same fix: probeOpensBlob treats UserNotAuthenticatedException as proof of decryptability regardless of whether the locked alias is actually the one that wrote the ciphertext. Per KeySecurityPolicy.kt's own documented switching semantics, a policy switch back to a previously-provisioned, now-locked alias while the blob was actually written under the sibling alias is an expected/intended stranding scenario meant to be caught by this exact health check — and it silently reports healthy instead, undermining the PR's stated purpose in the one case it didn't test.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 2): reviewers codex/general=gpt-5.6-sol(completed_parseable_output_with_nonzero_wrapper_exit); sonnet5/general=claude-sonnet-5(completed); verifier=verifier-sonnet5-4060-1783827369=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🔴 1 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt:275-303: A locked, provisioned alias is treated as proof it owns the ciphertext, defeating the sibling-alias health check this PR just fixed
`probeOpensBlob` (lines 290-303) catches `UserNotAuthenticatedException` and returns `true` unconditionally. That exception fires at `cipher.init()` before AndroidKeyStore ever reaches the ciphertext, so it proves only that the *alias* is auth-gated and locked — not that it produced *this* blob. `KeySecurityPolicy.kt` (lines 37-45) documents that switching policies leaves the old alias's blob 'undecryptable' and expects it to 'surface through the key-health / re-derive path' — i.e. this exact function. Concretely: DEVICE_BOUND writes/migrates a blob under `KEYS_ALIAS_DEVICE_BOUND`; the host later switches back to AUTH_GATED, whose alias (`KEYS_ALIAS_AUTH_GATED`) is still provisioned from an earlier period and its auth window happens to be closed. `isPrivateKeyDecryptable` then hits `hasIdentityKeysKey(keysAlias) && probeOpensBlob{decrypt(blob, keysAlias)}` (lines 275-276), gets `UserNotAuthenticatedException`, and reports `true` — even though this alias can never open that blob (a real `BadPaddingException` would surface once actually unlocked, exactly the failure `WalletKeyHealthSheet`'s repair path exists to catch). The two new tests (`authGatedPolicyKeyReportsDecryptableWithoutPrompting`, `authGatedFormerRsaKeyReportsDecryptable`) only cover a locked alias that legitimately wrote the blob — the combined 'provisioned + locked + wrong alias' case remains untested and mishandled.
| (keystore.hasIdentityKeysKey(keystore.keysAlias) && | ||
| probeOpensBlob { keystore.decrypt(blob, keystore.keysAlias) }) || | ||
| (keystore.hasLegacyRsaKeysKey() && | ||
| probeOpensBlob { keystore.decryptLegacyRsaKeysBlob(blob) }) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * True iff [decrypt] recovers [blob] with a PRESENT key (plaintext scrubbed | ||
| * immediately), or the key is auth-gated with a closed window | ||
| * (`UserNotAuthenticatedException` — present and would recover after auth, so | ||
| * decryptable). A wrong-key crypto failure or an absent key (`null`) is false. | ||
| * Prompt-free by construction — see [isPrivateKeyDecryptable]. Used only by the | ||
| * non-prompting key-health probe, never on a signing path. | ||
| */ | ||
| private fun probeOpensBlob(decrypt: () -> ByteArray?): Boolean = | ||
| try { | ||
| val plain = decrypt() | ||
| if (plain != null) { | ||
| plain.fill(0) | ||
| true | ||
| } else { | ||
| false | ||
| } | ||
| } catch (e: UserNotAuthenticatedException) { | ||
| true | ||
| } catch (e: GeneralSecurityException) { | ||
| false | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: A locked, provisioned alias is treated as proof it owns the ciphertext, defeating the sibling-alias health check this PR just fixed
probeOpensBlob (lines 290-303) catches UserNotAuthenticatedException and returns true unconditionally. That exception fires at cipher.init() before AndroidKeyStore ever reaches the ciphertext, so it proves only that the alias is auth-gated and locked — not that it produced this blob. KeySecurityPolicy.kt (lines 37-45) documents that switching policies leaves the old alias's blob 'undecryptable' and expects it to 'surface through the key-health / re-derive path' — i.e. this exact function. Concretely: DEVICE_BOUND writes/migrates a blob under KEYS_ALIAS_DEVICE_BOUND; the host later switches back to AUTH_GATED, whose alias (KEYS_ALIAS_AUTH_GATED) is still provisioned from an earlier period and its auth window happens to be closed. isPrivateKeyDecryptable then hits hasIdentityKeysKey(keysAlias) && probeOpensBlob{decrypt(blob, keysAlias)} (lines 275-276), gets UserNotAuthenticatedException, and reports true — even though this alias can never open that blob (a real BadPaddingException would surface once actually unlocked, exactly the failure WalletKeyHealthSheet's repair path exists to catch). The two new tests (authGatedPolicyKeyReportsDecryptableWithoutPrompting, authGatedFormerRsaKeyReportsDecryptable) only cover a locked alias that legitimately wrote the blob — the combined 'provisioned + locked + wrong alias' case remains untested and mishandled.
source: ['codex']
There was a problem hiding this comment.
Resolved in 527004a — A locked, provisioned alias is treated as proof it owns the ciphertext, defeating the sibling-alias health check this PR just fixed no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…ed alias via prompt-free sibling Two dashpay#4060 fixes on the Keystore path. 1) No-secure-lock-screen key generation (NEW on-device defect, Android 14/S21): every Keystore key set setUnlockedDeviceRequired(true) (AES master + both RSA wrapping pairs) and the AUTH_GATED pair also set setUserAuthenticationRequired(true). Both parameters require a secure lock screen, so with NO lock configured KeyMint rejects generate_key (ProviderException "Keystore key generation failed" / KeyStoreException system error, internal Keystore code 4 / KeyMint 10309) and wallet creation hard-crashes — reproduced by disabling the lock screen after a successful bind. Product ruling: the app MUST work without a screen lock. generateWithLockScreenDegradation now drops the lock-bound params when the device has no secure lock screen: proactively via a KeyguardManager .isDeviceSecure probe (wired from WalletStorage, which holds the Context), and as a safety net by catching the no-secure-lock-screen generate_key rejection and retrying once without them. The AUTH_GATED alias degrades to a non-gated key on a lockless device (there is no authenticator to gate on). The downgrade is logged at WARN; the StrongBox->TEE fallback is preserved. Existing keys are never regenerated (callers check presence first), so already-provisioned lock-bound keys are untouched. Parameter selection (lockBoundKeyParamsSupported) and failure classification (isNoSecureLockScreenKeyGenFailure) are pure, JVM-unit-tested functions (KeystoreKeyGenPolicyTest). 2) isPrivateKeyDecryptable locked-wrong-alias false-healthy (finding b80a15c93339): probeOpensBlob treated a locked auth-gated alias's UserNotAuthenticatedException (thrown at cipher.init, before the ciphertext) as "decryptable", so a sibling-written blob under a provisioned-but-locked AUTH_GATED policy was reported healthy — defeating the very sibling-alias health check this PR added. The health check now first lets the prompt-free, non-gated DEVICE_BOUND sibling DISPROVE ownership: an RSA-OAEP ciphertext opens under exactly one keypair, so if that sibling opens the blob the current policy alias does not own it, and since retrievePrivateKey never falls back to the sibling the blob is genuinely strandable -> false (drives the re-derive/repair path). The irreducible symmetric residual (a locked auth-gated FORMER RSA key at KEYS_ALIAS) is documented; it self-heals via retrieve's fallback->null on the first real unlock. Tests mirror the sibling-alias scenarios (WalletStorageUpgradeMatrixTest). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-Forward Prior Findings
Both prior dashpay.rs findings are untouched by this delta (which only touches KeystoreManager.kt/WalletStorage.kt and their tests) and remain valid on direct re-read: a wallet removed from WalletManager but not yet handle-destroyed still collapses into 'unmanaged identity' NotFound (blocking, confirmed reachable via PlatformWalletManager.removeWallet unregistering before closing the wrapper), and the native NotFound inner-miss path still has no direct Rust FFI test (suggestion).
New Findings In Latest Delta
The delta correctly fixes prior-1 — WalletStorage.isPrivateKeyDecryptable now calls the new KeystoreManager.opensUnderNonGatedDeviceBoundSibling prompt-free probe to disprove a locked AUTH_GATED alias's ownership of a DEVICE_BOUND-sibling-written blob before falling through to the ambiguous UserNotAuthenticatedException-as-decryptable catch-all, with solid new regression tests for both the disproof and the legitimate-owner non-regression case. Verification surfaced three residual concerns in the same area, all in scope: (1) the new sibling probe's own 'prompt-free, never gates' assumption is unsound when the device screen is currently locked, since setUnlockedDeviceRequired still applies to DEVICE_BOUND and throws the same UserNotAuthenticatedException subtype that gets swallowed and misread as 'not this key,' silently reintroducing the exact false-positive this delta fixes in that narrower window — untested by any new fixture; (2) the new lockless-device key-generation degradation permanently drops setUserAuthenticationRequired on the AUTH_GATED alias for the life of the key, so a device that later gains a secure lock screen never actually regains the documented 'signing requires auth' guarantee for keys created before that point; (3) the failure classifier used to trigger that degradation (isNoSecureLockScreenKeyGenFailure) matches any nested KeyStoreException once wrapped by a 'key generation failed' ProviderException, which is broader than the specific no-secure-lock-screen KeyMint signature and could downgrade AUTH_GATED on an unrelated Keystore error on a genuinely secure device.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 1): reviewers codex/general=gpt-5.6-sol(completed); sonnet5/general=claude-sonnet-5(completed); codex/security-auditor=gpt-5.6-sol(completed); sonnet5/security-auditor=claude-sonnet-5(completed); codex/ffi-engineer=gpt-5.6-sol(completed); sonnet5/ffi-engineer=claude-sonnet-5(completed); verifier=verifier-sonnet5-4060-1783862960=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🟡 3 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt:262-271: opensUnderNonGatedDeviceBoundSibling's 'prompt-free / never gates' assumption breaks down when the device screen is currently locked, reintroducing the false-positive this delta fixes
`opensUnderNonGatedDeviceBoundSibling` (KeystoreManager.kt:262-271) is documented as 'probed PROMPT-FREE (that alias never gates)' and used by `WalletStorage.isPrivateKeyDecryptable` to disprove a locked AUTH_GATED alias's ownership of a sibling-written blob. But `KeySecurityPolicy.kt:31` itself documents that DEVICE_BOUND keys 'remain bound to this device's Keystore (`setUnlockedDeviceRequired` still applies)' — confirmed in the RSA keygen spec (KeystoreManager.kt:399-400), which applies `setUnlockedDeviceRequired` to BOTH policy aliases whenever `lockBound=true` at generation time. Per Android's contract, that flag throws `UserNotAuthenticatedException` on decrypt whenever the device is *currently locked*, independent of `setUserAuthenticationRequired` — and `UserNotAuthenticatedException` is a `GeneralSecurityException` subclass, so it's swallowed indistinguishably from a genuine wrong-key failure by the bare `catch (e: GeneralSecurityException) { false }` at line 268-270. Concretely: if `isPrivateKeyDecryptable` runs while the screen happens to be locked, the sibling probe throws and returns `false` (fails to disprove), and the code falls through to probing the current AUTH_GATED alias (WalletStorage.kt:297-299) — which is *also* gated by `setUnlockedDeviceRequired` and throws the same exception, which `probeOpensBlob` (WalletStorage.kt:322-323) explicitly treats as `true` ('decryptable'). Neither new regression test (`provisionedLockedWrongAliasBlobDisprovedByPromptFreeSibling`, `lockedPolicyOwnerNotDisprovedWhenSiblingCannotOpen`) models the sibling probe itself throwing — `FakeKeystoreManager.opensUnderNonGatedDeviceBoundSibling` (WalletStorageUpgradeMatrixTest.kt:317-318) only ever returns a boolean, confirmed by direct read.
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt:435-494: A key generated on a lockless device permanently loses AUTH_GATED's documented authentication requirement, even after the user later adds a secure lock screen
`generateWithLockScreenDegradation` drops `setUserAuthenticationRequired` (in addition to `setUnlockedDeviceRequired`) for the AUTH_GATED alias on a lockless device — an explicitly documented, intentional trade-off for this PR's stated goal (lines 392-398: 'the AUTH_GATED alias then degrades to a non-gated key... there is no lock-screen authenticator to gate on anyway'). The gap is what happens next: `ensureKeysKeyPair`'s own doc comment (lines 455-457) confirms 'existing keys are never regenerated,' so if the user later enrolls a PIN/biometric, the already-provisioned AUTH_GATED key stays permanently non-auth-gated for the life of the install. `KeySecurityPolicy.kt` (lines 19-25) documents AUTH_GATED's public contract as 'decrypt (sign) requires user authentication' unconditionally, with no caveat about this degrade-and-never-recover path, and `KeystoreSigner` only invokes `BiometricGate` after catching `UserNotAuthenticatedException` (KeystoreSigner.kt:217-223) — a weakened key never throws it, so signing silently proceeds without the documented auth prompt. This doesn't introduce a *newly* exploitable capability at generation time (there's no authenticator to gate on when there's no lock screen), but it does mean the security policy silently and permanently diverges from its documented contract once the device state changes, with no re-binding or key-health signal to catch it (the key still successfully decrypts, so `isPrivateKeyDecryptable` never flags it for repair).
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt:581-607: isNoSecureLockScreenKeyGenFailure's classifier is broader than the specific no-secure-lock-screen KeyMint signature
Once the outer exception is a `ProviderException` whose message contains 'key generation failed' (setting `sawKeyGenFailure = true`), ANY nested exception whose class name ends in `KeyStoreException` is classified as proof of the no-secure-lock-screen condition — its own error code/message is never checked in that branch. Confirmed by the test `classifiesWrappedKeystoreGenerationFailure` (KeystoreKeyGenPolicyTest.kt:41-52), which only pins the one observed on-device shape (KeyMint 10309) but doesn't constrain the classifier to it. Since `withStrongBoxFallback` already isolates `StrongBoxUnavailableException` separately, this path is reached only for less-common `ProviderException`s during lock-bound key generation — but an unrelated hardware/OEM Keystore generation failure wrapped the same way, on a device that IS secure, would still trigger the same silent downgrade to a non-auth-gated AUTH_GATED key described in the finding above. Consider requiring the inner exception to also mention `generate_key` or a lock-screen-specific signal, rather than accepting any `KeyStoreException` once the outer message matches.
| open fun opensUnderNonGatedDeviceBoundSibling(blob: EncryptedBlob): Boolean { | ||
| if (keySecurityPolicy == KeySecurityPolicy.DEVICE_BOUND) return false | ||
| if (!hasIdentityKeysKey(KEYS_ALIAS_DEVICE_BOUND)) return false | ||
| return try { | ||
| decrypt(blob, KEYS_ALIAS_DEVICE_BOUND).fill(0) | ||
| true | ||
| } catch (e: GeneralSecurityException) { | ||
| false | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: opensUnderNonGatedDeviceBoundSibling's 'prompt-free / never gates' assumption breaks down when the device screen is currently locked, reintroducing the false-positive this delta fixes
opensUnderNonGatedDeviceBoundSibling (KeystoreManager.kt:262-271) is documented as 'probed PROMPT-FREE (that alias never gates)' and used by WalletStorage.isPrivateKeyDecryptable to disprove a locked AUTH_GATED alias's ownership of a sibling-written blob. But KeySecurityPolicy.kt:31 itself documents that DEVICE_BOUND keys 'remain bound to this device's Keystore (setUnlockedDeviceRequired still applies)' — confirmed in the RSA keygen spec (KeystoreManager.kt:399-400), which applies setUnlockedDeviceRequired to BOTH policy aliases whenever lockBound=true at generation time. Per Android's contract, that flag throws UserNotAuthenticatedException on decrypt whenever the device is currently locked, independent of setUserAuthenticationRequired — and UserNotAuthenticatedException is a GeneralSecurityException subclass, so it's swallowed indistinguishably from a genuine wrong-key failure by the bare catch (e: GeneralSecurityException) { false } at line 268-270. Concretely: if isPrivateKeyDecryptable runs while the screen happens to be locked, the sibling probe throws and returns false (fails to disprove), and the code falls through to probing the current AUTH_GATED alias (WalletStorage.kt:297-299) — which is also gated by setUnlockedDeviceRequired and throws the same exception, which probeOpensBlob (WalletStorage.kt:322-323) explicitly treats as true ('decryptable'). Neither new regression test (provisionedLockedWrongAliasBlobDisprovedByPromptFreeSibling, lockedPolicyOwnerNotDisprovedWhenSiblingCannotOpen) models the sibling probe itself throwing — FakeKeystoreManager.opensUnderNonGatedDeviceBoundSibling (WalletStorageUpgradeMatrixTest.kt:317-318) only ever returns a boolean, confirmed by direct read.
source: ['claude']
There was a problem hiding this comment.
Good catch on the wording — clarified in 28b98114de (doc only).
You're right that "never gates" overstated it: a lock-bound DEVICE_BOUND key still carries setUnlockedDeviceRequired, so a probe run while the device is currently locked throws UserNotAuthenticatedException and the catch (GeneralSecurityException) returns false. "Prompt-free" was only ever about the absence of a biometric prompt (no setUserAuthenticationRequired).
On behavior, this is the conservative direction rather than a false-positive reintroduction: when locked, the sibling probe returns false ("cannot disprove") → isPrivateKeyDecryptable reports the blob decryptable instead of falsely offering repair, and the disproof defers to the next unlock (the same residual already documented for the locked FORMER-RSA case). The key-health sheet runs in-app on an unlocked device, where the probe is live. The KDoc now spells this out explicitly rather than changing the (already-safe) control flow.
There was a problem hiding this comment.
Intentionally deferred at 28b9811 — the code behavior is unchanged; the expanded KDoc now explicitly documents the currently-locked sibling-probe residual as an accepted conservative trade-off that defers disproof until unlock.
There was a problem hiding this comment.
Resolved in this update — opensUnderNonGatedDeviceBoundSibling's 'prompt-free / never gates' assumption breaks down when the device screen is currently locked, reintroducing the false-positive this delta fixes no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| /** | ||
| * Run [generate] — which builds+initializes the spec for `(strongBox, | ||
| * lockBound)` and produces the key — degrading gracefully when the device | ||
| * has no secure lock screen. | ||
| * | ||
| * THE APP MUST WORK WITHOUT A SCREEN LOCK (product decision, | ||
| * dashpay/platform#4060). The lock-screen-bound Keystore parameters | ||
| * (`setUnlockedDeviceRequired`, and for the auth-gated alias | ||
| * `setUserAuthenticationRequired`) require a secure lock screen; without one | ||
| * KeyMint fails `generate_key` (observed on-device: `ProviderException` | ||
| * "Keystore key generation failed" wrapping `KeyStoreException` system error, | ||
| * KeyMint 10309). Strategy: | ||
| * 1. If [deviceSecureProbe] reports NO secure lock screen, build the key | ||
| * WITHOUT the lock-bound params up front and log the downgrade. | ||
| * 2. Otherwise attempt with the lock-bound params; if generation still | ||
| * fails with the no-secure-lock-screen signature (a race — the lock was | ||
| * removed after the probe — or an OEM that rejects it despite a probe | ||
| * saying secure), retry once WITHOUT them and log the downgrade. | ||
| * Each attempt keeps the existing StrongBox→TEE fallback. | ||
| * | ||
| * Existing keys are never regenerated (callers check presence first), so this | ||
| * only affects FIRST-USE creation on a lockless device; a key already | ||
| * provisioned with the lock-bound params is untouched. | ||
| */ | ||
| private fun <T> generateWithLockScreenDegradation( | ||
| alias: String, | ||
| generate: (strongBox: Boolean, lockBound: Boolean) -> T, | ||
| ): T { | ||
| fun withStrongBoxFallback(lockBound: Boolean): T = | ||
| try { | ||
| generate(true, lockBound) | ||
| } catch (_: StrongBoxUnavailableException) { | ||
| generate(false, lockBound) | ||
| } | ||
|
|
||
| val lockBoundSupported = lockBoundKeyParamsSupported(deviceSecureProbe()) | ||
| if (!lockBoundSupported) { | ||
| Log.w( | ||
| TAG, | ||
| "No secure lock screen (KeyguardManager.isDeviceSecure=false); generating " + | ||
| "'$alias' WITHOUT lock-screen binding so the wallet works without a " + | ||
| "screen lock (dashpay/platform#4060).", | ||
| ) | ||
| return withStrongBoxFallback(lockBound = false) | ||
| } | ||
| return try { | ||
| withStrongBoxFallback(lockBound = true) | ||
| } catch (e: ProviderException) { | ||
| if (!isNoSecureLockScreenKeyGenFailure(e)) throw e | ||
| Log.w( | ||
| TAG, | ||
| "Key generation for '$alias' was rejected for requiring a secure lock " + | ||
| "screen even though the device reported secure (lock removed mid-flight, " + | ||
| "or an OEM quirk); retrying WITHOUT lock-screen binding " + | ||
| "(dashpay/platform#4060).", | ||
| e, | ||
| ) | ||
| withStrongBoxFallback(lockBound = false) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: A key generated on a lockless device permanently loses AUTH_GATED's documented authentication requirement, even after the user later adds a secure lock screen
generateWithLockScreenDegradation drops setUserAuthenticationRequired (in addition to setUnlockedDeviceRequired) for the AUTH_GATED alias on a lockless device — an explicitly documented, intentional trade-off for this PR's stated goal (lines 392-398: 'the AUTH_GATED alias then degrades to a non-gated key... there is no lock-screen authenticator to gate on anyway'). The gap is what happens next: ensureKeysKeyPair's own doc comment (lines 455-457) confirms 'existing keys are never regenerated,' so if the user later enrolls a PIN/biometric, the already-provisioned AUTH_GATED key stays permanently non-auth-gated for the life of the install. KeySecurityPolicy.kt (lines 19-25) documents AUTH_GATED's public contract as 'decrypt (sign) requires user authentication' unconditionally, with no caveat about this degrade-and-never-recover path, and KeystoreSigner only invokes BiometricGate after catching UserNotAuthenticatedException (KeystoreSigner.kt:217-223) — a weakened key never throws it, so signing silently proceeds without the documented auth prompt. This doesn't introduce a newly exploitable capability at generation time (there's no authenticator to gate on when there's no lock screen), but it does mean the security policy silently and permanently diverges from its documented contract once the device state changes, with no re-binding or key-health signal to catch it (the key still successfully decrypts, so isPrivateKeyDecryptable never flags it for repair).
source: ['codex']
There was a problem hiding this comment.
Intentional — leaving the code as-is.
The degrade-on-lockless-device path is the explicit product decision for this PR: the app MUST work without a secure lock screen (#4060), and there is no authenticator to gate on when no lock screen exists, so setUserAuthenticationRequired is dropped alongside setUnlockedDeviceRequired. You've correctly identified the follow-on: because existing keys are never regenerated, a key first provisioned on a lockless device stays non-auth-gated even after the user later enrolls a PIN/biometric.
We're accepting that as a bounded trade-off for now — it introduces no newly-exploitable capability at generation time (there was no authenticator to gate on) — and tracking "re-bind / upgrade the AUTH_GATED alias once a lock screen is added" as a follow-up rather than blocking this PR. A key-health/upgrade signal that detects the lock-screen-added transition and offers a re-derive is the natural home for it. Thanks for the precise write-up; it's the right scoping for that follow-up.
There was a problem hiding this comment.
Still valid at 28b9811 — an AUTH_GATED key generated on a lockless device is still reused permanently and does not regain authentication gating after later secure-lock enrollment.
There was a problem hiding this comment.
Resolved in this update — A key generated on a lockless device permanently loses AUTH_GATED's documented authentication requirement, even after the user later adds a secure lock screen no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| internal fun isNoSecureLockScreenKeyGenFailure(t: Throwable): Boolean { | ||
| var cur: Throwable? = t | ||
| var sawKeyGenFailure = false | ||
| while (cur != null) { | ||
| val name = cur::class.java.name | ||
| val msg = cur.message.orEmpty() | ||
| if (name.endsWith("ProviderException") && | ||
| msg.contains("key generation failed", ignoreCase = true) | ||
| ) { | ||
| sawKeyGenFailure = true | ||
| } | ||
| if (name.endsWith("KeyStoreException")) { | ||
| // A Keystore-side generation system error reached via a | ||
| // key-gen ProviderException, or one that names generate_key / | ||
| // a lock-screen requirement directly. | ||
| if (sawKeyGenFailure || | ||
| msg.contains("generate_key", ignoreCase = true) || | ||
| msg.contains("lock screen", ignoreCase = true) | ||
| ) { | ||
| return true | ||
| } | ||
| } | ||
| cur = cur.cause | ||
| } | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 Suggestion: isNoSecureLockScreenKeyGenFailure's classifier is broader than the specific no-secure-lock-screen KeyMint signature
Once the outer exception is a ProviderException whose message contains 'key generation failed' (setting sawKeyGenFailure = true), ANY nested exception whose class name ends in KeyStoreException is classified as proof of the no-secure-lock-screen condition — its own error code/message is never checked in that branch. Confirmed by the test classifiesWrappedKeystoreGenerationFailure (KeystoreKeyGenPolicyTest.kt:41-52), which only pins the one observed on-device shape (KeyMint 10309) but doesn't constrain the classifier to it. Since withStrongBoxFallback already isolates StrongBoxUnavailableException separately, this path is reached only for less-common ProviderExceptions during lock-bound key generation — but an unrelated hardware/OEM Keystore generation failure wrapped the same way, on a device that IS secure, would still trigger the same silent downgrade to a non-auth-gated AUTH_GATED key described in the finding above. Consider requiring the inner exception to also mention generate_key or a lock-screen-specific signal, rather than accepting any KeyStoreException once the outer message matches.
source: ['codex']
There was a problem hiding this comment.
Leaving the classifier breadth as-is, deliberately.
The broad match is load-bearing for the on-device defect this fixes. The path is only reached when (1) StrongBox is already isolated, (2) the device probe reported secure, and (3) generation with the lock-bound params threw a ProviderException("...key generation failed"). On the observed shape (KeyMint 10309 / internal Keystore code 4 after the lock screen was removed), the nested KeyStoreException does not reliably carry a generate_key or lock screen string — which is exactly why the type + outer-message match is the signal and the two string checks are only additional accept conditions. Requiring the inner exception to also name generate_key/lock-screen would risk rejecting the real 10309 recovery across OEM message variations, regressing the on-device crash this PR was written to fix — which I can't re-verify without the device.
The blast radius is bounded: existing keys are never regenerated, so this only affects first-use creation, and the fallback is a working non-gated key rather than a hard crash. I've noted the residual you flagged — a genuinely unrelated keygen failure on a secure device being swept into the downgrade — as an accepted trade-off vs. crashing; if a reliable KeyMint-code-specific signal proves stable across OEMs it can tighten this later.
There was a problem hiding this comment.
Still valid at 28b9811 — isNoSecureLockScreenKeyGenFailure remains unchanged and still accepts any nested KeyStoreException once the outer generic key-generation message matches.
There was a problem hiding this comment.
Resolved in this update — isNoSecureLockScreenKeyGenFailure's classifier is broader than the specific no-secure-lock-screen KeyMint signature no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
|
Round-2:
Pushed |
… ErrorInvalidHandle Addresses blocker 03ff842bd7d7 on dashpay#4060 (and substantially answers suggestion d7c06333de19). platform_wallet_get_managed_identity used `?` on `wm.get_wallet_info(&wallet.wallet_id())` inside the `with_item` closure. That folded a get_wallet_info miss into the same closure-`None` the closure produces when the wallet legitimately does not manage the identity, so a wallet REMOVED from the shared WalletManager map but whose handle is not yet destroyed (the window PlatformWalletManager.removeWallet opens: it clears the manager entry in step 2 before destroying the ManagedPlatformWallet wrapper in step 3) collapsed into NotFound. Kotlin's translateManagedIdentityNotFoundToZero then swallowed that real wallet failure as a zero-balance unmanaged identity, contradicting the function's own doc contract (a stale/invalid handle must surface ErrorInvalidHandle). The closure now uses `.map()` so get_wallet_info presence lives in its own Option layer, and a new pure, generic `classify_managed_identity_outcome` maps the three-layer outcome to the FFI contract: - handle absent from PLATFORM_WALLET_STORAGE (outer None) -> ErrorInvalidHandle - wallet removed from the manager map (Some(None)) -> ErrorInvalidHandle - valid wallet, identity not managed (Some(Some(None))) -> NotFound - found (Some(Some(Some(mi)))) -> Ok The classifier is split out so both error arms are unit-testable without a fully-seeded PlatformWallet fixture (which the FFI unit-test module cannot build). Adds classify_managed_identity_outcome_distinguishes_removed_wallet_from_unmanaged covering the removed-but-not-destroyed race (-> InvalidHandle), the identity-not-managed miss (-> NotFound, the d7c06333de19 ask), and the found case. Full platform-wallet-ffi lib suite green (142 passed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…mpt-free" scope Addresses suggestion 3408c9d1d301 on dashpay#4060. The KDoc said the DEVICE_BOUND sibling alias "never gates", which overstated it: a lock-bound DEVICE_BOUND key still carries setUnlockedDeviceRequired, so a decrypt probe run while the device is CURRENTLY LOCKED throws UserNotAuthenticatedException (a GeneralSecurityException) and the catch returns false. "Prompt-free" was only ever about the absence of a biometric prompt (no setUserAuthenticationRequired). Clarified that the currently-locked case returns false ("cannot disprove") is the conservative direction — the caller reports the blob decryptable rather than falsely offering repair, deferring the disproof to the next unlock (the same residual already documented for the locked FORMER-RSA path) — and that the key-health sheet runs in-app on an unlocked device where the probe is live. Doc only; no behavior change. :sdk compiles clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-Forward Prior Findings
Prior-1 (blocking) is FIXED: platform_wallet_get_managed_identity now preserves the WalletManager lookup layer and maps a removed-but-not-destroyed wallet to ErrorInvalidHandle. Prior-2 remains STILL VALID at lower confidence: the new classifier unit test closes the original decision-logic regression, but the exported valid-wallet/unmanaged-identity NotFound path still lacks the direct seeded-wallet FFI test originally requested. Prior-3 is INTENTIONALLY DEFERRED: this delta documents the currently-locked sibling-probe residual as an accepted conservative trade-off without changing behavior. Prior-4 and prior-5 remain STILL VALID: lockless AUTH_GATED keys never regain authentication after later lock enrollment, and the no-lock-screen classifier still accepts overly broad wrapped Keystore failures.
New Findings In Latest Delta
No new defect was found in either commit. The Rust classifier fix is correct, and cargo test -p platform-wallet-ffi --lib dashpay:: passed 6/6 tests, including the new regression test; the Kotlin change is documentation-only.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_opus_sample, bucket 0): reviewers codex/general=gpt-5.6-sol(completed after runtime auth recovery); sonnet5/general=claude-sonnet-5(completed); opus/general=claude-opus-4-8(completed); codex/security-auditor=gpt-5.6-sol(completed after runtime auth recovery); sonnet5/security-auditor=claude-sonnet-5(completed); opus/security-auditor=claude-opus-4-8(completed); codex/ffi-engineer=gpt-5.6-sol(completed after runtime auth recovery); sonnet5/ffi-engineer=claude-sonnet-5(completed); opus/ffi-engineer=claude-opus-4-8(completed); verifier=verifier-sonnet5-4060-1783886996=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🟡 1 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt:593-618: isNoSecureLockScreenKeyGenFailure's classifier accepts any nested KeyStoreException once the outer message matches
Carried forward from prior-5 — STILL VALID, confirmed unchanged by direct read at current HEAD. Once a `ProviderException` whose message contains 'key generation failed' sets `sawKeyGenFailure = true` (lines 599-603), ANY nested exception whose class name ends in `KeyStoreException` is accepted as proof of the no-secure-lock-screen condition (604-613) without checking its own error code or message. An unrelated OEM/hardware/parameter Keystore failure wrapped the same way, on a device that IS secure, could still trigger `generateWithLockScreenDegradation`'s retry with `lockBound=false`, silently downgrading a fresh AUTH_GATED alias to non-auth-gated even though `deviceSecureProbe()` reported secure. Consider requiring the inner exception to also mention `generate_key` or a lock-screen-specific signal before accepting it as proof.
| internal fun isNoSecureLockScreenKeyGenFailure(t: Throwable): Boolean { | ||
| var cur: Throwable? = t | ||
| var sawKeyGenFailure = false | ||
| while (cur != null) { | ||
| val name = cur::class.java.name | ||
| val msg = cur.message.orEmpty() | ||
| if (name.endsWith("ProviderException") && | ||
| msg.contains("key generation failed", ignoreCase = true) | ||
| ) { | ||
| sawKeyGenFailure = true | ||
| } | ||
| if (name.endsWith("KeyStoreException")) { | ||
| // A Keystore-side generation system error reached via a | ||
| // key-gen ProviderException, or one that names generate_key / | ||
| // a lock-screen requirement directly. | ||
| if (sawKeyGenFailure || | ||
| msg.contains("generate_key", ignoreCase = true) || | ||
| msg.contains("lock screen", ignoreCase = true) | ||
| ) { | ||
| return true | ||
| } | ||
| } | ||
| cur = cur.cause | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: isNoSecureLockScreenKeyGenFailure's classifier accepts any nested KeyStoreException once the outer message matches
Carried forward from prior-5 — STILL VALID, confirmed unchanged by direct read at current HEAD. Once a ProviderException whose message contains 'key generation failed' sets sawKeyGenFailure = true (lines 599-603), ANY nested exception whose class name ends in KeyStoreException is accepted as proof of the no-secure-lock-screen condition (604-613) without checking its own error code or message. An unrelated OEM/hardware/parameter Keystore failure wrapped the same way, on a device that IS secure, could still trigger generateWithLockScreenDegradation's retry with lockBound=false, silently downgrading a fresh AUTH_GATED alias to non-auth-gated even though deviceSecureProbe() reported secure. Consider requiring the inner exception to also mention generate_key or a lock-screen-specific signal before accepting it as proof.
source: ['codex-general', 'codex-security-auditor', 'sonnet5-general', 'sonnet5-security-auditor', 'opus-general', 'opus-security-auditor']
There was a problem hiding this comment.
Resolved in this update — isNoSecureLockScreenKeyGenFailure's classifier accepts any nested KeyStoreException once the outer message matches no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…le pending-key state Fixes dashpay#4053. Identity private keys were always wrapped under the auth-gated Keystore alias (KEYS_ALIAS, setUserAuthenticationRequired, ~30s post-unlock validity) with no way for a host app that gates wallet access behind its own auth model (e.g. an app-side PIN, as the Dash Android wallet does) to opt out — sign-time decrypts outside the window fail, and there was no way to even see that a key had been persisted watch-only. - New KeySecurityPolicy enum: AUTH_GATED (default — behavior unchanged, same alias, same auth window) and DEVICE_BOUND (a separate non-auth-gated but still hardware-backed / StrongBox-preferring, non-exportable RSA wrapping alias). Distinct aliases because Keystore auth parameters are fixed at key generation. - KeystoreManager takes the policy at construction and exposes the selected keysAlias; the RSA helpers are parameterized per alias and the user-auth gate is applied only to the AUTH_GATED alias. - WalletStorage gains a policy-taking convenience constructor and routes identity-key encrypt/decrypt through the keystore's keysAlias. - PlatformWalletPersistenceHandler no longer skips a failed identity-key derive/store silently: it logs at ERROR (the key is persisted watch-only and cannot sign) and records a queryable PendingIdentityKey entry on a StateFlow, cleared automatically when a later persist round stores the key. PlatformWalletManager exposes the flow as pendingIdentityKeys so hosts can surface a repair path (repairIdentityKey). Defaults are unchanged everywhere: constructing KeystoreManager / WalletStorage without a policy keeps the exact historical AUTH_GATED behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ng keys Fixes dashpay#4052. A signing failure caused by a missing stored private key ("no private key stored for <hex>") surfaced as the opaque DashSdkError.PlatformWallet.Generic, indistinguishable from any other wallet failure — hosts could not route the user to key repair. The error text originates in Kotlin (KeystoreSigner's completeSign error string), travels through Rust as free text, and returns under the catch-all platform-wallet codes (ErrorUnknown via the blanket PlatformWalletError conversion, or ErrorWalletOperation), so the typed mapping happens on the Kotlin boundary where it is reliable: - New DashSdkError.PlatformWallet.SigningKeyUnavailable with a MESSAGE_MARKER constant; KeystoreSigner now builds its "missing key" completion error from the same constant, so the emitter and the matcher cannot drift. - fromPlatformWalletNative recognizes the marker on the catch-all codes only — the dedicated retry-semantics codes (16/18/19/20) are never overridden. Also pins PlatformWalletFFIResultCode::NotFound (98) as PLATFORM_WALLET_NOT_FOUND_CODE for the dashpay#4051 translation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ties Fixes dashpay#4051. Dashpay.syncState (and payments/contacts/acceptIncomingRequest) threw DashSdkError.PlatformWallet.Generic("requested …ManagedIdentity not found") when called for an identity the wallet does not manage — e.g. a contact's identity — even though every signature already models the absence (nullable String, empty Contacts, false). The native platform_wallet_get_managed_identity reports the miss through the FFI's blanket Option → result conversion (PlatformWalletFFIResultCode::NotFound, 98) instead of returning a zero handle, so the fix translates that specific error to a zero handle on the Kotlin boundary (translateManagedIdentityNotFoundToZero) and routes all managed-identity snapshots through it: syncState/payments now return null, contacts returns empty lists, acceptIncomingRequest returns false. Any other native error is rethrown untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…y; clear pending on repair Two blocking correctness fixes from review. 1) KeystoreManager: the default AUTH_GATED policy generated the RSA wrapping keypair at KEYS_ALIAS — the same alias the pre-RSA scheme used for an auth-gated AES-256-GCM key — and ensureKeysKeyPair deleted that AES key before creating the RSA pair, stranding existing installs' AES-GCM identity- key blobs (data loss). Give AUTH_GATED its own RSA alias (KEYS_ALIAS_AUTH_GATED) and keep KEYS_ALIAS as a read-only legacy alias that is never deleted or regenerated. WalletStorage.retrievePrivateKey now detects a legacy AES-GCM blob, decrypts it with the retained legacy key, and transparently re-encrypts (migrates) it under the RSA alias; a fresh install has no legacy blobs and always takes the RSA path. isPrivateKeyDecryptable reports a legacy blob as recoverable while its key survives, so key-health no longer offers a needless re-derive for migratable keys. 2) PlatformWalletManager.repairIdentityKey wrote the re-derived key straight through the deriver, bypassing onPersistIdentityKeyUpsert (the only path that clears pendingIdentityKeys), so a repaired key lingered as pending. On a successful repair it now clears the entry via a new internal PlatformWalletPersistenceHandler.markIdentityKeyRepaired hook. Tests: alias split + blob-type discrimination (RSA vs legacy AES) in KeySecurityPolicyTest; markIdentityKeyRepaired clears the pending entry in PlatformWalletPersistenceHandlerTest. (The Keystore crypto round trip itself can't run on the JVM — AndroidKeyStore has no Robolectric provider — so the alias/blob decision logic is what's unit-pinned.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d wallet handle; atomic pending-key updates Addresses the latest review on dashpay#4060. WalletStorage/KeystoreManager (BLOCKING): the alias split routed only non-empty-IV AES blobs to the legacy fallback, so empty-IV RSA blobs written by the pre-alias-split scheme under KEYS_ALIAS were decrypted against a NEWLY generated key at the policy alias and stranded. retrievePrivateKey now recovers them with the retained former RSA keypair at KEYS_ALIAS (an upgrade fast path when the policy alias is unprovisioned, else a wrong-key catch-fallback) and migrates the value forward; UserNotAuthenticatedException still propagates so the signer can prompt. isPrivateKeyDecryptable now reflects real key presence (policy-alias OR former-RSA OR legacy-AES key), not blob shape alone. The relevant KeystoreManager methods are `open` so the routing can be unit-tested without AndroidKeyStore (no Robolectric provider), covering the full matrix: legacy-AES, legacy-RSA-at-KEYS_ALIAS, new-alias, stranded, mixed-window, and auth-propagation. dashpay.rs (BLOCKING): platform_wallet_get_managed_identity flattened both the invalid-wallet-handle (outer with_item miss) and identity-not-managed (inner miss) into NotFound, which the Kotlin Dashpay layer translates to an unmanaged (zero) handle — so a stale Dashpay instance masked a wallet failure as "unmanaged". The outer miss now returns ErrorInvalidHandle (untranslated, surfaces as PlatformWallet.InvalidHandle); only the inner miss stays NotFound. PlatformWalletPersistenceHandler (suggestion): recordPendingIdentityKey / clearPendingIdentityKey now use MutableStateFlow.update so a persistence-thread record and a host-thread repair-clear cannot drop each other's mutation. Also moves the orphaned withManagedIdentity KDoc onto its declaration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… retrievePrivateKey Addresses the latest review on dashpay#4060. WalletStorage.retrievePrivateKey (BLOCKING, finding 41026612bc10): the empty-IV RSA upgrade fast path inferred blob origin from key *presence* (!hasIdentityKeysKey(policyAlias) && hasLegacyRsaKeysKey()) and then called decryptLegacyRsaKeysBlob with no crypto-failure handling. Presence is not proof of origin: after an AUTH_GATED->DEVICE_BOUND policy switch, a blob written under a sibling policy alias can enter this branch merely because an unrelated legacy RSA key still lingers at KEYS_ALIAS — decryptLegacyRsaKeysBlob then does a doFinal with the wrong private key and threw a BadPaddingException that escaped uncaught into KeystoreSigner (which catches only UserNotAuthenticatedException). Every candidate key is now TRIED and a wrong-key GeneralSecurityException is absorbed as "not this key" (per the KeystoreManager.decryptLegacyRsaKeysBlob contract) via the new tryFormerRsaRecovery helper, used by both the fast path and the steady-state catch fallback. A blob no present key can open is now unrecoverable -> null (key-health then offers a re-derive), mirroring the legacy-AES stranded path — never a bogus plaintext, never an uncaught crypto exception. UserNotAuthenticatedException still propagates so the signer can prompt and retry. Tests: WalletStorageUpgradeMatrixTest gains siblingAliasBlobUnderUnprovisioned- PolicyDoesNotThrow (former RSA key present, policy unprovisioned, sibling-alias blob -> null, not a thrown BadPaddingException — this failed before the fix); formerRsaBlobWithDeletedKeyIsStranded now asserts null (RSA-stranded made consistent with the AES-stranded null contract). Full :sdk suite green (131). dashpay.rs (suggestion d7c06333de19): add get_managed_identity_unknown_wallet_- is_invalid_handle asserting the outer with_item miss returns ErrorInvalidHandle (not NotFound), locking in the dual-error contract the invalid-handle fix relies on. The inner not-managed -> NotFound outcome needs a seeded-wallet fixture this unit-test module lacks; it stays covered by the Kotlin translation test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…not key presence Addresses escalated blocker e17e265dc680 on dashpay#4060 — the still-open half of the RSA-alias-inference finding (the retrievePrivateKey crash was fixed last round; the false-healthy key-health verdict was not). isPrivateKeyDecryptable returned isKeysBlobDecryptable(blob) && (hasIdentityKeysKey || hasLegacyRsaKeysKey) — pure structural + key-*presence* checks that never verified the specific blob was written by a present key. For the SIBLING_POLICY scenario (a blob written under a sibling policy alias while an unrelated former RSA key lingers at KEYS_ALIAS) it reported the stranded key "healthy", so WalletKeyHealthSheet never offered the re-derive/repair path the check exists to drive. It now PROBES the same candidate keys retrievePrivateKey uses (policy alias if provisioned, then the former KEYS_ALIAS RSA keypair; legacy AES for GCM blobs) and returns true only when a present key actually opens the blob. The probe never prompts: KeystoreManager.decrypt / decryptLegacyRsaKeysBlob / decryptLegacyKeysBlob are bare Cipher ops (BiometricPrompt is driven only by KeystoreSigner/BiometricGate, never here), so an auth-gated key with a closed window throws UserNotAuthenticatedException rather than showing UI — which counts as DECRYPTABLE (present + would recover after auth; a health check must not report it strandable). Only a wrong-key crypto failure (BadPadding/AEAD) or an absent key is "not decryptable". Recovered plaintext is scrubbed immediately. Tests: siblingAlias… now also asserts isPrivateKeyDecryptable == false (the fix; true before). Adds authGatedPolicyKeyReportsDecryptableWithoutPrompting and authGatedFormerRsaKeyReportsDecryptable (UserNotAuth -> decryptable), with a throwAuthOnLegacyRsaDecrypt toggle on the fake. Full :sdk suite green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ed alias via prompt-free sibling Two dashpay#4060 fixes on the Keystore path. 1) No-secure-lock-screen key generation (NEW on-device defect, Android 14/S21): every Keystore key set setUnlockedDeviceRequired(true) (AES master + both RSA wrapping pairs) and the AUTH_GATED pair also set setUserAuthenticationRequired(true). Both parameters require a secure lock screen, so with NO lock configured KeyMint rejects generate_key (ProviderException "Keystore key generation failed" / KeyStoreException system error, internal Keystore code 4 / KeyMint 10309) and wallet creation hard-crashes — reproduced by disabling the lock screen after a successful bind. Product ruling: the app MUST work without a screen lock. generateWithLockScreenDegradation now drops the lock-bound params when the device has no secure lock screen: proactively via a KeyguardManager .isDeviceSecure probe (wired from WalletStorage, which holds the Context), and as a safety net by catching the no-secure-lock-screen generate_key rejection and retrying once without them. The AUTH_GATED alias degrades to a non-gated key on a lockless device (there is no authenticator to gate on). The downgrade is logged at WARN; the StrongBox->TEE fallback is preserved. Existing keys are never regenerated (callers check presence first), so already-provisioned lock-bound keys are untouched. Parameter selection (lockBoundKeyParamsSupported) and failure classification (isNoSecureLockScreenKeyGenFailure) are pure, JVM-unit-tested functions (KeystoreKeyGenPolicyTest). 2) isPrivateKeyDecryptable locked-wrong-alias false-healthy (finding b80a15c93339): probeOpensBlob treated a locked auth-gated alias's UserNotAuthenticatedException (thrown at cipher.init, before the ciphertext) as "decryptable", so a sibling-written blob under a provisioned-but-locked AUTH_GATED policy was reported healthy — defeating the very sibling-alias health check this PR added. The health check now first lets the prompt-free, non-gated DEVICE_BOUND sibling DISPROVE ownership: an RSA-OAEP ciphertext opens under exactly one keypair, so if that sibling opens the blob the current policy alias does not own it, and since retrievePrivateKey never falls back to the sibling the blob is genuinely strandable -> false (drives the re-derive/repair path). The irreducible symmetric residual (a locked auth-gated FORMER RSA key at KEYS_ALIAS) is documented; it self-heals via retrieve's fallback->null on the first real unlock. Tests mirror the sibling-alias scenarios (WalletStorageUpgradeMatrixTest). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… ErrorInvalidHandle Addresses blocker 03ff842bd7d7 on dashpay#4060 (and substantially answers suggestion d7c06333de19). platform_wallet_get_managed_identity used `?` on `wm.get_wallet_info(&wallet.wallet_id())` inside the `with_item` closure. That folded a get_wallet_info miss into the same closure-`None` the closure produces when the wallet legitimately does not manage the identity, so a wallet REMOVED from the shared WalletManager map but whose handle is not yet destroyed (the window PlatformWalletManager.removeWallet opens: it clears the manager entry in step 2 before destroying the ManagedPlatformWallet wrapper in step 3) collapsed into NotFound. Kotlin's translateManagedIdentityNotFoundToZero then swallowed that real wallet failure as a zero-balance unmanaged identity, contradicting the function's own doc contract (a stale/invalid handle must surface ErrorInvalidHandle). The closure now uses `.map()` so get_wallet_info presence lives in its own Option layer, and a new pure, generic `classify_managed_identity_outcome` maps the three-layer outcome to the FFI contract: - handle absent from PLATFORM_WALLET_STORAGE (outer None) -> ErrorInvalidHandle - wallet removed from the manager map (Some(None)) -> ErrorInvalidHandle - valid wallet, identity not managed (Some(Some(None))) -> NotFound - found (Some(Some(Some(mi)))) -> Ok The classifier is split out so both error arms are unit-testable without a fully-seeded PlatformWallet fixture (which the FFI unit-test module cannot build). Adds classify_managed_identity_outcome_distinguishes_removed_wallet_from_unmanaged covering the removed-but-not-destroyed race (-> InvalidHandle), the identity-not-managed miss (-> NotFound, the d7c06333de19 ask), and the found case. Full platform-wallet-ffi lib suite green (142 passed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…mpt-free" scope Addresses suggestion 3408c9d1d301 on dashpay#4060. The KDoc said the DEVICE_BOUND sibling alias "never gates", which overstated it: a lock-bound DEVICE_BOUND key still carries setUnlockedDeviceRequired, so a decrypt probe run while the device is CURRENTLY LOCKED throws UserNotAuthenticatedException (a GeneralSecurityException) and the catch returns false. "Prompt-free" was only ever about the absence of a biometric prompt (no setUserAuthenticationRequired). Clarified that the currently-locked case returns false ("cannot disprove") is the conservative direction — the caller reports the blob decryptable rather than falsely offering repair, deferring the disproof to the next unlock (the same residual already documented for the locked FORMER-RSA path) — and that the key-health sheet runs in-app on an unlocked device where the probe is live. Doc only; no behavior change. :sdk compiles clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bfoss765
force-pushed
the
fix/kotlin-sdk-android-integration
branch
from
28b9811 to
1a4fd4f
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Preliminary Codex-only verification confirmed exact HEAD 1a4fd4f, reviewed the full PR range, and reconciled the rebased series with range-diff. Carried-forward prior findings: prior-1, prior-2, and prior-3 remain STILL VALID as suggestions. New findings in the latest delta/current PR: three blockers remain—a private-key migration can race wallet deletion, pending-key state escapes changeset rollback, and a rebased unit test deterministically contradicts the current code-98 mapping.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 3 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt:310-314: Legacy-key migration can recreate a private key after wallet deletion
retrievePrivateKey reads and decrypts a legacy blob without holding privateKeyMutex, and migrateToPolicyAlias later rewrites it with an unconditional DataStore edit. A concurrent removeWallet can acquire withPrivateKeyExclusion after the read, delete the alias and owner index, and cascade its Room rows; the delayed migration then recreates privkey.<pubkeyHex> after the wipe succeeds. The recreated ciphertext has no owner-index or database reference, violating removeWallet's documented guarantee that no identity-key ciphertext survives. Serialize the read/decrypt/rewrite with deletion, or make the rewrite conditional on the stored value still matching the blob originally read.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:1077-1109: Pending-key state escapes changeset rollback
The pendingIdentityKeys StateFlow is changed immediately even though the corresponding PublicKeyEntity mutation remains staged until onChangesetEnd. If derivation fails and a later callback aborts the round, the watch-only row is discarded but a false pending entry survives. Conversely, when an earlier watch-only row is pending, a successful retry clears the entry immediately; if that round then rolls back or its Room transaction fails, alias cleanup deletes the newly derived key while the old watch-only row remains without its repair signal. Buffer pending-map deltas with ChangesetBuffer and publish them only after the Room transaction commits; rollback must preserve the pre-round map.
In `packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt:156-174: Rebased test contradicts the current NotFound mapping
DashSdkError.fromPlatformWalletNative currently maps platform-wallet codes 7, 8, and 98 to DashSdkError.NotFound, and platformWalletCodesMapToPlatformWalletSubtree already asserts that behavior. This test instead expects the same code 98 result to be PlatformWallet.Generic, so its assertion fails and the following cast would also fail. The range-diff shows the rebase introduced the typed code-98 mapping without updating this older test.
In `packages/rs-platform-wallet-ffi/src/dashpay.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay.rs:1328-1332: The valid-wallet unmanaged-identity path lacks a direct FFI test
The source explicitly acknowledges that no seeded fixture calls platform_wallet_get_managed_identity with a valid wallet that does not manage the requested identity. The classifier test uses synthetic nested Option values, the exported-function test covers only an unknown handle, and ManagedIdentityNotFoundTranslationTest throws a synthetic code-1098 exception without traversing JNI or Rust storage. Because this PR relies on the production valid-wallet/unmanaged-identity path to return null or empty results, add an integration fixture that exercises PLATFORM_WALLET_STORAGE, WalletManager::get_wallet_info, the public FFI match arm, and the NotFound result code together.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt:482-490: A lockless AUTH_GATED key never regains authentication gating
First use on a lockless device generates the AUTH_GATED alias with lockBound=false, omitting setUserAuthenticationRequired. ensureKeysKeyPair subsequently reuses every existing RSA pair without inspecting its effective policy or regenerating it. If the user later enrolls a PIN or biometric, the alias therefore remains permanently non-gated, so decrypt and signing operations never raise UserNotAuthenticatedException or invoke BiometricGate despite AUTH_GATED's documented contract. Record the effective generation policy and provide a safe rotation and rewrapping path after secure-lock enrollment.
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt:593-612: Generic wrapped Keystore failures can trigger a security downgrade
Once an outer ProviderException contains the generic phrase "key generation failed", the classifier accepts any nested class ending in KeyStoreException without checking that exception's error code or message. Android Keystore can use this wrapper for unrelated hardware, parameter, OEM, or transient failures. If the retry without lock-bound parameters succeeds, a device that reported itself secure silently receives an AUTH_GATED key without its authentication requirement. Require the nested Keystore failure itself to identify generate_key or a secure-lock-screen requirement.
| private suspend fun migrateToPolicyAlias(pubkeyHex: String, plain: ByteArray) { | ||
| runCatching { | ||
| val migrated = keystore.encrypt(plain, alias = keystore.keysAlias) | ||
| store.edit { it[privateKeyKey(pubkeyHex)] = encode(migrated) } | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Legacy-key migration can recreate a private key after wallet deletion
retrievePrivateKey reads and decrypts a legacy blob without holding privateKeyMutex, and migrateToPolicyAlias later rewrites it with an unconditional DataStore edit. A concurrent removeWallet can acquire withPrivateKeyExclusion after the read, delete the alias and owner index, and cascade its Room rows; the delayed migration then recreates privkey. after the wipe succeeds. The recreated ciphertext has no owner-index or database reference, violating removeWallet's documented guarantee that no identity-key ciphertext survives. Serialize the read/decrypt/rewrite with deletion, or make the rewrite conditional on the stored value still matching the blob originally read.
source: ['codex-general']
| val id = outcome.getOrNull() | ||
| if (id != null) { | ||
| // Stored — clear any earlier failure for this pubkey. | ||
| clearPendingIdentityKey(publicKeyData.toHex()) | ||
| } else { | ||
| // NOT silent (dashpay/platform#4053): the key is being | ||
| // persisted watch-only, so every signature with it will | ||
| // fail until it is re-derived. Log loudly and record a | ||
| // queryable pending entry (see [pendingIdentityKeys]). | ||
| val reason = outcome.exceptionOrNull()?.let { t -> | ||
| t.message ?: t.javaClass.simpleName | ||
| } ?: "deriver returned no storage identifier" | ||
| Log.e( | ||
| TAG, | ||
| "identity private-key derive/store FAILED — key " + | ||
| "${publicKeyData.toHex()} (identity ${identityId.toBase58String()}, " + | ||
| "keyId $keyId, slot $identityIndex/$keyIndex) is persisted " + | ||
| "WATCH-ONLY and cannot sign until re-derived " + | ||
| "(see PlatformWalletPersistenceHandler.pendingIdentityKeys): $reason", | ||
| outcome.exceptionOrNull(), | ||
| ) | ||
| recordPendingIdentityKey( | ||
| PendingIdentityKey( | ||
| walletIdHex = keyOwnerWalletId.toHex(), | ||
| identityIdBase58 = identityId.toBase58String(), | ||
| keyId = keyId, | ||
| publicKeyHex = publicKeyData.toHex(), | ||
| identityIndex = identityIndex, | ||
| keyIndex = keyIndex, | ||
| reason = reason, | ||
| failedAtMs = System.currentTimeMillis(), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🔴 Blocking: Pending-key state escapes changeset rollback
The pendingIdentityKeys StateFlow is changed immediately even though the corresponding PublicKeyEntity mutation remains staged until onChangesetEnd. If derivation fails and a later callback aborts the round, the watch-only row is discarded but a false pending entry survives. Conversely, when an earlier watch-only row is pending, a successful retry clears the entry immediately; if that round then rolls back or its Room transaction fails, alias cleanup deletes the newly derived key while the old watch-only row remains without its repair signal. Buffer pending-map deltas with ChangesetBuffer and publish them only after the Room transaction commits; rollback must preserve the pre-round map.
source: ['codex-general', 'codex-ffi-engineer']
| @Test | ||
| fun platformWalletNotFoundCodeMapsToGeneric() { | ||
| // PlatformWalletFFIResultCode::NotFound (98) — the code the Option → | ||
| // result conversion emits for "requested <thing> not found". Stays | ||
| // Generic in the hierarchy; Dashpay's managed-identity reads | ||
| // translate it to null before it ever escapes (#4051). | ||
| val mapped = DashSdkError.fromNative( | ||
| DashSDKException( | ||
| DashSdkError.PLATFORM_WALLET_CODE_OFFSET + | ||
| DashSdkError.PLATFORM_WALLET_NOT_FOUND_CODE, | ||
| "requested platform_wallet::identity::ManagedIdentity not found", | ||
| ), | ||
| ) | ||
| assertTrue(mapped is DashSdkError.PlatformWallet.Generic) | ||
| assertEquals( | ||
| DashSdkError.PLATFORM_WALLET_NOT_FOUND_CODE, | ||
| (mapped as DashSdkError.PlatformWallet.Generic).nativeCode, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Rebased test contradicts the current NotFound mapping
DashSdkError.fromPlatformWalletNative currently maps platform-wallet codes 7, 8, and 98 to DashSdkError.NotFound, and platformWalletCodesMapToPlatformWalletSubtree already asserts that behavior. This test instead expects the same code 98 result to be PlatformWallet.Generic, so its assertion fails and the following cast would also fail. The range-diff shows the rebase introduced the typed code-98 mapping without updating this older test.
| @Test | |
| fun platformWalletNotFoundCodeMapsToGeneric() { | |
| // PlatformWalletFFIResultCode::NotFound (98) — the code the Option → | |
| // result conversion emits for "requested <thing> not found". Stays | |
| // Generic in the hierarchy; Dashpay's managed-identity reads | |
| // translate it to null before it ever escapes (#4051). | |
| val mapped = DashSdkError.fromNative( | |
| DashSDKException( | |
| DashSdkError.PLATFORM_WALLET_CODE_OFFSET + | |
| DashSdkError.PLATFORM_WALLET_NOT_FOUND_CODE, | |
| "requested platform_wallet::identity::ManagedIdentity not found", | |
| ), | |
| ) | |
| assertTrue(mapped is DashSdkError.PlatformWallet.Generic) | |
| assertEquals( | |
| DashSdkError.PLATFORM_WALLET_NOT_FOUND_CODE, | |
| (mapped as DashSdkError.PlatformWallet.Generic).nativeCode, | |
| ) | |
| } | |
| @Test | |
| fun platformWalletNotFoundCodeMapsToNotFound() { | |
| val mapped = DashSdkError.fromNative( | |
| DashSDKException( | |
| DashSdkError.PLATFORM_WALLET_CODE_OFFSET + | |
| DashSdkError.PLATFORM_WALLET_NOT_FOUND_CODE, | |
| "requested platform_wallet::identity::ManagedIdentity not found", | |
| ), | |
| ) | |
| assertTrue(mapped is DashSdkError.NotFound) | |
| } |
source: ['codex-general']
There was a problem hiding this comment.
Resolved in 4b073c3 — Rebased test contradicts the current NotFound mapping no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| /// The complementary inner outcome (a valid wallet lacking the managed | ||
| /// identity → `NotFound`) needs a fully seeded wallet in | ||
| /// `PLATFORM_WALLET_STORAGE`, which this unit-test module has no fixture for; | ||
| /// that path is covered at the translation layer by the Kotlin | ||
| /// `ManagedIdentityNotFoundTranslationTest`. |
There was a problem hiding this comment.
🟡 Suggestion: The valid-wallet unmanaged-identity path lacks a direct FFI test
The source explicitly acknowledges that no seeded fixture calls platform_wallet_get_managed_identity with a valid wallet that does not manage the requested identity. The classifier test uses synthetic nested Option values, the exported-function test covers only an unknown handle, and ManagedIdentityNotFoundTranslationTest throws a synthetic code-1098 exception without traversing JNI or Rust storage. Because this PR relies on the production valid-wallet/unmanaged-identity path to return null or empty results, add an integration fixture that exercises PLATFORM_WALLET_STORAGE, WalletManager::get_wallet_info, the public FFI match arm, and the NotFound result code together.
source: ['codex-general', 'codex-ffi-engineer']
| val lockBoundSupported = lockBoundKeyParamsSupported(deviceSecureProbe()) | ||
| if (!lockBoundSupported) { | ||
| Log.w( | ||
| TAG, | ||
| "No secure lock screen (KeyguardManager.isDeviceSecure=false); generating " + | ||
| "'$alias' WITHOUT lock-screen binding so the wallet works without a " + | ||
| "screen lock (dashpay/platform#4060).", | ||
| ) | ||
| return withStrongBoxFallback(lockBound = false) |
There was a problem hiding this comment.
🟡 Suggestion: A lockless AUTH_GATED key never regains authentication gating
First use on a lockless device generates the AUTH_GATED alias with lockBound=false, omitting setUserAuthenticationRequired. ensureKeysKeyPair subsequently reuses every existing RSA pair without inspecting its effective policy or regenerating it. If the user later enrolls a PIN or biometric, the alias therefore remains permanently non-gated, so decrypt and signing operations never raise UserNotAuthenticatedException or invoke BiometricGate despite AUTH_GATED's documented contract. Record the effective generation policy and provide a safe rotation and rewrapping path after secure-lock enrollment.
source: ['codex-general', 'codex-security-auditor', 'codex-ffi-engineer']
| internal fun isNoSecureLockScreenKeyGenFailure(t: Throwable): Boolean { | ||
| var cur: Throwable? = t | ||
| var sawKeyGenFailure = false | ||
| while (cur != null) { | ||
| val name = cur::class.java.name | ||
| val msg = cur.message.orEmpty() | ||
| if (name.endsWith("ProviderException") && | ||
| msg.contains("key generation failed", ignoreCase = true) | ||
| ) { | ||
| sawKeyGenFailure = true | ||
| } | ||
| if (name.endsWith("KeyStoreException")) { | ||
| // A Keystore-side generation system error reached via a | ||
| // key-gen ProviderException, or one that names generate_key / | ||
| // a lock-screen requirement directly. | ||
| if (sawKeyGenFailure || | ||
| msg.contains("generate_key", ignoreCase = true) || | ||
| msg.contains("lock screen", ignoreCase = true) | ||
| ) { | ||
| return true |
There was a problem hiding this comment.
🟡 Suggestion: Generic wrapped Keystore failures can trigger a security downgrade
Once an outer ProviderException contains the generic phrase "key generation failed", the classifier accepts any nested class ending in KeyStoreException without checking that exception's error code or message. Android Keystore can use this wrapper for unrelated hardware, parameter, OEM, or transient failures. If the retry without lock-bound parameters succeeds, a device that reported itself secure silently receives an AUTH_GATED key without its authentication requirement. Require the nested Keystore failure itself to identify generate_key or a secure-lock-screen requirement.
| internal fun isNoSecureLockScreenKeyGenFailure(t: Throwable): Boolean { | |
| var cur: Throwable? = t | |
| var sawKeyGenFailure = false | |
| while (cur != null) { | |
| val name = cur::class.java.name | |
| val msg = cur.message.orEmpty() | |
| if (name.endsWith("ProviderException") && | |
| msg.contains("key generation failed", ignoreCase = true) | |
| ) { | |
| sawKeyGenFailure = true | |
| } | |
| if (name.endsWith("KeyStoreException")) { | |
| // A Keystore-side generation system error reached via a | |
| // key-gen ProviderException, or one that names generate_key / | |
| // a lock-screen requirement directly. | |
| if (sawKeyGenFailure || | |
| msg.contains("generate_key", ignoreCase = true) || | |
| msg.contains("lock screen", ignoreCase = true) | |
| ) { | |
| return true | |
| internal fun isNoSecureLockScreenKeyGenFailure(t: Throwable): Boolean { | |
| var cur: Throwable? = t | |
| while (cur != null) { | |
| val name = cur::class.java.name | |
| val msg = cur.message.orEmpty() | |
| if (name.endsWith("KeyStoreException") && | |
| (msg.contains("generate_key", ignoreCase = true) || | |
| msg.contains("lock screen", ignoreCase = true)) | |
| ) { | |
| return true | |
| } | |
| cur = cur.cause | |
| } | |
| return false | |
| } |
source: ['codex-general', 'codex-security-auditor', 'codex-ffi-engineer']
bfoss765
changed the title
…Generic The dashpay#4051 unmanaged-identity work added a platformWalletNotFoundCodeMapsToGeneric test, the PLATFORM_WALLET_NOT_FOUND_CODE constant, and the translateManagedIdentityNotFoundToZero helper (whose doc states an untranslated code-98 miss surfaces as PlatformWallet.Generic) — but never updated the actual mapping, so fromPlatformWalletNative still lumped 98 in with 7/8 into the typed top-level NotFound and the new test failed. Code 98 (the blanket Option -> result miss) now falls through to PlatformWallet.Generic carrying nativeCode 98, matching Swift parity (Swift maps result code 98 to PlatformWalletError.notFound, i.e. it stays in the wallet-error family rather than crossing into a top-level category). Local reads recognize the miss at the raw code via translateManagedIdentityNotFoundToZero and return an absence; 7 (ErrorIdentityNotFound) and 8 (ErrorContactNotFound) still map to the typed top-level NotFound. Reconciled the two stale base parity tests (platformWalletCodesMapToPlatformWalletSubtree, platformWalletNotFoundConvertsAtThePublicBoundary) that still asserted 98 -> NotFound. Full :sdk:testDebugUnitTest suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Preliminary Codex-only verification confirmed exact HEAD 4b073c3 and reviewed the cumulative PR plus the latest two-file delta. Carried-forward prior findings: prior-1, prior-2, prior-4, prior-5, and prior-6 remain STILL VALID; prior-3 is FIXED. New findings in the latest delta: globally reclassifying shared platform-wallet code 98 as PlatformWallet.Generic breaks existing public APIs that promise typed NotFound, adding a third blocking issue.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed),gpt-5.6-sol— security-auditor (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking
5 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt:282-289: Code 98 no longer maps to the public NotFound type
Code 98 is the shared PlatformWalletFFIResultCode::NotFound value used by the blanket Option-to-result conversion and by unrelated native APIs; it is not specific to managed-identity lookup. Removing it from the NotFound arm globally changes established public behavior. For example, platform_wallet_manager_spv_rescan_filters returns code 98 with "Wallet not found", while PlatformWalletManager.rescanSpvFilters explicitly promises DashSdkError.NotFound for an unknown wallet; the current mapping now returns PlatformWallet.Generic instead. The Dashpay fix does not require this global change because translateManagedIdentityNotFoundToZero intercepts raw code 1098 before mapNativeErrors runs. Preserve the typed code-98 mapping and keep the managed-identity absence translation local to Dashpay.
| 7, // ErrorIdentityNotFound | ||
| 8, // ErrorContactNotFound | ||
| 98, // NotFound (Option returned as an error) | ||
| -> NotFound(message, cause) | ||
| // 98 (PlatformWalletFFIResultCode::NotFound, the blanket Option → | ||
| // result miss) intentionally falls through to PlatformWallet.Generic | ||
| // carrying nativeCode 98: local reads recognize it at the raw code | ||
| // via translateManagedIdentityNotFoundToZero (#4051) and return an | ||
| // absence, so it must NOT collapse into the typed top-level NotFound. |
There was a problem hiding this comment.
🔴 Blocking: Code 98 no longer maps to the public NotFound type
Code 98 is the shared PlatformWalletFFIResultCode::NotFound value used by the blanket Option-to-result conversion and by unrelated native APIs; it is not specific to managed-identity lookup. Removing it from the NotFound arm globally changes established public behavior. For example, platform_wallet_manager_spv_rescan_filters returns code 98 with "Wallet not found", while PlatformWalletManager.rescanSpvFilters explicitly promises DashSdkError.NotFound for an unknown wallet; the current mapping now returns PlatformWallet.Generic instead. The Dashpay fix does not require this global change because translateManagedIdentityNotFoundToZero intercepts raw code 1098 before mapNativeErrors runs. Preserve the typed code-98 mapping and keep the managed-identity absence translation local to Dashpay.
| 7, // ErrorIdentityNotFound | |
| 8, // ErrorContactNotFound | |
| 98, // NotFound (Option returned as an error) | |
| -> NotFound(message, cause) | |
| // 98 (PlatformWalletFFIResultCode::NotFound, the blanket Option → | |
| // result miss) intentionally falls through to PlatformWallet.Generic | |
| // carrying nativeCode 98: local reads recognize it at the raw code | |
| // via translateManagedIdentityNotFoundToZero (#4051) and return an | |
| // absence, so it must NOT collapse into the typed top-level NotFound. | |
| 7, // ErrorIdentityNotFound | |
| 8, // ErrorContactNotFound | |
| 98, // NotFound (Option returned as an error) | |
| -> NotFound(message, cause) |
source: ['codex']
2 participants
Kotlin-only fixes for the three SDK issues blocking the Dash Android wallet's Platform migration (dashpay/dash-wallet#1507). No native/Rust changes; the prebuilt library is untouched. Defaults are unchanged everywhere.
1. Configurable identity-key security policy + queryable pending-key state (fixes #4053)
Identity private keys were always wrapped under the auth-gated Keystore alias (
KeystoreManager.KEYS_ALIAS,setUserAuthenticationRequired(true), ~30 s post-unlock validity) with no configurability — a host app that gates wallet access behind its own auth model (the Dash wallet gates by PIN app-side) fails every sign outside the window, and a failed derive/store during persistence was silently swallowed.KeySecurityPolicyenum:AUTH_GATED(default — exact historical behavior, same alias, same auth window) andDEVICE_BOUND(a separate non-auth-gated but still hardware-backed / StrongBox-preferring, non-exportable RSA wrapping alias,org.dashfoundation.wallet.keys.devicebound). Distinct aliases because Keystore auth parameters are fixed at key generation; the KDoc documents the pick-once-per-install stability requirement.KeystoreManager(keySecurityPolicy)selects the alias (keysAlias); the RSA helpers are parameterized per alias, and the user-auth gate is only applied when generating theAUTH_GATEDalias.WalletStorage(context, keySecurityPolicy)convenience constructor; identity-key encrypt/decrypt routes throughkeystore.keysAlias.PlatformWalletPersistenceHandlerno longer skips a failed identity-key derive/store silently: it logs at ERROR (the key is persisted watch-only and cannot sign) and records a queryablePendingIdentityKeyon aStateFlow, cleared when a later persist round stores the key. Exposed asPlatformWalletManager.pendingIdentityKeys, repairable via the existingrepairIdentityKey.2.
Dashpayreads return null/empty for unmanaged identities (fixes #4051)Dashpay.syncState(andpayments/contacts/acceptIncomingRequest) threwDashSdkError.PlatformWallet.Generic("requested …ManagedIdentity not found")for identities the wallet does not manage, even though every signature already models absence. The nativeplatform_wallet_get_managed_identityreports the miss via the FFI's blanketOption → resultconversion (PlatformWalletFFIResultCode::NotFound, 98) instead of a zero handle, so the Kotlin boundary now translates that specific error to the existing zero-handle path:syncState/paymentsreturn null,contactsreturns empty lists,acceptIncomingRequestreturns false. Other native errors are rethrown untouched.3. Typed
SigningKeyUnavailableerror (fixes #4052)Signing failures caused by a missing stored private key ("no private key stored for ") surfaced as the opaque
DashSdkError$PlatformWallet$Generic. The message originates in Kotlin (KeystoreSigner.completeSign), travels through Rust as free text, and returns under the catch-all platform-wallet codes — so the typed mapping happens on the Kotlin boundary where it is reliable: a newDashSdkError.PlatformWallet.SigningKeyUnavailablewith a sharedMESSAGE_MARKERconstant thatKeystoreSignerbuilds its error from (emitter and matcher cannot drift).fromPlatformWalletNativematches the marker on the catch-all codes only; the dedicated retry-semantics codes (16/18/19/20) are never overridden.Testing
:sdk:testDebugUnitTest— BUILD SUCCESSFUL. New coverage:KeySecurityPolicyTest(alias/policy plumbing, defaults), pending-identity-key tests inPlatformWalletPersistenceHandlerTest(failure recorded, null-identifier recorded, later success clears),ManagedIdentityNotFoundTranslationTest(NotFound → zero handle, other errors rethrown), andDashSdkErrorTestadditions (marker mapping on codes 6/99, retry-semantics codes never overridden, NotFound code pin).Refs dashpay/dash-wallet#1507.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests