Conversation
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
There was a problem hiding this comment.
Verdict: ⛔️ Request Changes
Review: diff 25 files.
Counterpart synonymdev/bitkit-ios#774 diverges — see the parity finding.
Findings:
8 inline (2 blocking)
Audit:
Awaits QA.
Coverage:
QA: journeys and manual tests await all reviewers to approve, author can run it now via comment: @ovi-reviewer test
Reviewed by gpt-6-sol-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)
| Logger.warn("Ignoring Pubky Ring auth error callback with missing or invalid nonce", context = TAG) | ||
| PubkyRingAuthCallbackHandlingResult.Ignored | ||
| } | ||
| _publicKey.update { publicKey.ensurePubkyPrefix() } |
There was a problem hiding this comment.
If a user adopts a Ring pubky with no profile and closes Bitkit before saving, the next launch restores the session but opens the empty Profile screen. Retry only reloads the missing profile, so they have to disconnect and pick the key again. Could we persist the pending profile setup state here so the existing resume flow takes them back to Create Profile?
There was a problem hiding this comment.
Fixed in 6dca12c: adoptRingIdentity now sets the pending profile setup flag when the picked pubky has no profile, and clears it when it has one, the same as iOS, so the existing resume flow reopens Create Profile after a relaunch. Saving, signing out or losing the Ring source clears it. Tests added in PubkyRepoTest.kt.
| modifier = Modifier.testTag("PubkyChoiceImport") | ||
| ) | ||
|
|
||
| else -> uiState.identities.forEachIndexed { index, identity -> |
There was a problem hiding this comment.
The Ring identities are all rendered in a fixed, non-scrollable Column. When there are more cards than fit on screen, the later pubkys cannot be reached or selected. Could we make this list scrollable so every identity remains accessible?
There was a problem hiding this comment.
Fixed in 42bdc1b: the Pubky Choice list now scrolls, so every Ring pubky can be reached.
jvsena42
left a comment
There was a problem hiding this comment.
Two gated findings and one copy regression, all inline, all checked by a second pass. Nothing blocking: every profile route is behind PaykitRouteGuard and the flag defaults off.
Checked and clean:
- Provider surface. Exported but double-gated: a signature-level
readPermissionplus a per-callBinder.getCallingUid()check that requires every package of the uid to match signatures, so shell and unknown uids fail closed.insert/update/deletethrow,callandopenFileare the defaults, andgetTypereturns only MIME. - The credential row is emitted only when the requested pubky matches the key derived from the stored secret.
- Squatting. The package manager refuses a second app declaring the same authority or the same permission under another signer, and the client pins the Ring package and checks signatures before any query.
- What crosses. Ring's secret is read just in time, never written to
PUBKY_SECRET_KEY, never in a backup payload (sessionSecretis gone fromPubkySessionBackupV1), and only redacted pubkys reach the logs. - Identity binding.
readCredentialrequires a single row, a matching row pubky and a matching derived key; adoption re-derives and requires equality;managedSecretKeyForcompares against the active public key before signing. - Lifecycle.
Keychain.wipe()clears the reference too, sign-out and SDK teardown delete it, and Ring being uninstalled fails the listing rather than emptying it. - Migration from the v2.5.0 relay flow: those sessions still import until expiry, and legacy
externalSessionbackups decode and restore to no identity.
Two caveats worth knowing, not findings: debug and e2e builds of both apps share the committed debug keystore, so a locally built app can read a dev-flavor wallet's pubky secret; and the seed-derived pubky secret cannot be rotated, so a leak on Ring's side is permanent for that wallet.
|
|
||
| throw PubkyAuthAttemptInactive() | ||
| } | ||
| keychain.upsertString( |
There was a problem hiding this comment.
Back during adoption orphans the reference, and a later signup link then throws away the seed-derived secret. MEDIUM, gated behind the Paykit opt-in.
The reference is upserted here, before the network signIn, and the only cleanup is the .onFailure branch. runSuspendCatching rethrows CancellationException, so on cancel there is no Result and that branch never runs. Back is live while the adoption spins — the screen only swaps the list for a spinner, there is no BackHandler, and popBackStack destroys the entry-scoped ViewModel and cancels its scope. Process death mid-adoption leaves the same orphan, and a restart keeps it: checkAdoptedSourcePresent returns early because Ring still lists the pubky.
Later a pubkyauth:// signup link finds hasIdentity() false and proceeds. approveSignupAuth never clears the reference (unlike createLocalIdentitySession), so persistSessionAccess sees an adopted pubky and deletes PUBKY_SECRET_KEY. After that managedSecretKeyFor returns null, so every auth request is refused with "Pubky Identity Required", snapshotSessionBackupState returns null so the identity is never backed up, and the next cold start hands the SDK Ring's credential for a different pubky. If the user later deletes that Ring key, they are told an identity that never came from Ring is "no longer available in Pubky Ring".
Fix: delete SHARED_PUBKY_SOURCE before registerIdentity in approveSignupAuth, mirroring createLocalIdentitySession — that alone closes the consequence. Optionally also clear the reference under NonCancellable on cancellation here, or run the adoption on appScope.
iOS is not exposed to this: synonymdev/bitkit-ios#774 runs adoption in an unstructured Task and clears the reference on cancellation.
There was a problem hiding this comment.
Fixed in 0dc4e77: approveSignupAuth in PubkyRepo.kt now deletes SHARED_PUBKY_SOURCE before registering, like createLocalIdentitySession, with tests in PubkyRepoTest.kt. I left adoption cancellable: clearing the reference on cancel would let an abandoned pick that fails later delete the reference of a retried pick.
| } | ||
|
|
||
| private suspend fun activeSecretKeyHex(): String? { | ||
| val publicKey = _publicKey.value ?: return keychain.loadString(Keychain.Key.PUBKY_SECRET_KEY.name) |
There was a problem hiding this comment.
An adopted identity whose stored session is rejected is never re-signed in, although the credential is right there. LOW-MEDIUM, gated.
initialize() reads only PAYKIT_SESSION and PUBKY_SECRET_KEY, and for an adopted identity the latter is always null by design. So when importSession fails, resolveSignedInSession gets a null secret and returns RestorationFailed without ever calling sharedPubkyClient.readCredential(adoptedPubky) — which managedSecretKeyFor already knows how to do. The session is kept, so every cold start repeats the "session expired" toast with no identity, while hasIdentity() stays true and a signup link is refused as already signed in. activeSecretKeyHex() short-circuits on the null _publicKey, so refreshSessionIfPossible cannot help either.
The exit is re-picking the key on Pubky Choice, which the toast does point at, so this is recoverable rather than a lockout. It is mainly a parity gap: synonymdev/bitkit-ios#774 passes the adopted secret into its re-sign-in and recovers.
Fix: fall back to the Ring credential for storedSecretKeyHex in initialize(), and use the same fallback in activeSecretKeyHex() when _publicKey is null.
There was a problem hiding this comment.
Fixed in 420fef9: when the saved session is rejected and there's no own key, resolveSessionInitialization in PubkyRepo.kt now re-signs in with the Pubky Ring credential. It only signs in, and keeps the session as before when Pubky Ring can't be read; tests in PubkyRepoTest.kt. I left activeSecretKeyHex() as is, since managedSecretKeyFor already reads the Ring key once _publicKey is set.
| if (isSignup && rejectPubkySignupForExistingIdentity()) return | ||
|
|
||
| if (!isSignup && pubkyRepo.publicKey.value == null) { | ||
| if (!isSignup && (pubkyRepo.publicKey.value == null || !pubkyRepo.hasSecretKey())) { |
There was a problem hiding this comment.
The !hasSecretKey() case was folded into the publicKey == null branch when profile__auth_approval_ring_only was removed, so an adopted identity whose key is momentarily unreadable — Ring's provider unavailable, or the orphan state above — now reads "Pubky Identity Required / Create a Pubky identity in your profile" although the profile is right there. Worth keeping two branches, with a "key unavailable, open Pubky Ring" message for the second.
There was a problem hiding this comment.
Fixed in 94940c1: handlePubkyAuth in AppViewModel.kt has two branches again. With a public key but no readable key it now shows "Use Pubky Ring" with "Bitkit can't read the key for this pubky from Pubky Ring. Open Pubky Ring to approve this request." iOS had the same gap and gets the same check in 55abbe3a (#774).
jvsena42
left a comment
There was a problem hiding this comment.
Delta since aa6db6384 (0dc4e7701, 420fef976, 94940c13c): no findings. All three are addressed.
approveSignupAuthdeletesSHARED_PUBKY_SOURCEbeforeregisterIdentity, so an orphaned reference can no longer makepersistSessionAccessdrop the seed-derived secret.- The re-sign-in falls back to
adoptedSecretKeyHex(), which reads the Ring credential through the same trusted client, so an adopted identity with a rejected session recovers instead of looping. The failure to read is logged and returns null rather than throwing. - The auth toast is split again: a missing identity and an unreadable key now say different things, and the new copy points at Pubky Ring.
Small leftover, not worth a change on its own: activeSecretKeyHex() still returns only PUBKY_SECRET_KEY when _publicKey is null, so refreshSessionIfPossible() cannot help an adopted identity before initialize() has run. The startup path is the one that mattered and it is fixed.
There was a problem hiding this comment.
Advice: ✅ Approve
Reaudit: diff 10 files.
No new findings; the rest is in the review.
Coverage:
QA: waits for the other reviewers' approval, or @ovi-reviewer test
Reviewed by gpt-6-sol-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner) · wrong <why> (owner)
addressed - reaudit confirmed
There was a problem hiding this comment.
Advice: ✅ Approve
Reaudit: diff 2 files.
No new findings; the rest is in the review.
Coverage:
QA: waits for the other reviewers' approval, or @ovi-reviewer test
Reviewed by gpt-6-sol-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner) · wrong <why> (owner)
This PR:
Description
Release gate
Out of Scope
Reviewer Notes
externalSession, written since v2.3.0 behind the same toggle) still decode, but restore without a Pubky identity. They only held a homeserver session, never the key: the key stays in Pubky Ring and can be picked again on Pubky Choice with a Pubky Ring build that includes feat: share pubky keys with Bitkit pubky/pubky-ring#377. Covered byrestoreSessionBackupState should restore no identity for legacy external session backupsinPubkyRepoTest.kt.app.pubkyring, and only when its certificate matches. Debug and e2e builds of both apps share the committed debug keystore, so any locally built app can read a dev-flavor wallet's pubky key; release builds aren't affected.Design
Pubky Choice follows the "Profile use existing" and "Profile Create" mockups; the matching Figma frames aren't confirmed, so no link is added.
Preview
QA Notes
Journeys
N/A — no journeys yet; see Manual Tests. Most flows need Pubky Ring running alongside Bitkit, which the Capabilities table doesn't list; the few that don't are marked "no journey yet".
Manual Tests
pubkyauthgrant is approved with the Ring key read just in time — Pubky Ring alongside Bitkit not in Capabilitiespubkyauthrequest → the "Use Pubky Ring" toast shows instead of "Pubky Identity Required" — Pubky Ring alongside Bitkit not in Capabilitiesregression:wrong certificate → re-sign the Ring APK with a throwaway keystore and install it → neither signature permission is granted, Ring shows no Bitkit card, and Bitkit lists no Ring pubkys — Pubky Ring alongside Bitkit not in Capabilitiesregression:install order → install Bitkit first, then Ring → both signature permissions show as granted and Bitkit lists the Ring pubky, so the grant is retro-applied — Pubky Ring alongside Bitkit not in Capabilitiespubkyauthrequest with the Bitkit pubky → Ring signs the grant with the secret read from Bitkit's provider and reports success — Pubky Ring alongside Bitkit not in Capabilitiesregression:pubkyauthdeep link → Bitkit approval sheet → requester, capabilities and active identity render and Authorize succeeds, with no relay UI left anywhere in the flow — no journey yetAutomated Checks
SharedPubkyContractTest.kt— a shared secret must be valid hex that derives the requested pubkySharedPubkyProviderTest.kt— an untrusted caller is rejected, and the credential row is served only for the stored pubkySharedPubkyClientTest.kt— a trusted Pubky Ring is listed, and a missing Ring or one with another certificate fails without its provider being queriedPubkyRepoTest.kt— adoption, the credential mismatch rejection, sign-up only when the pubky has no record, the pending profile setup, the launch and foreground re-checks, the null backup snapshot for a picked pubky, a signup link clearing a leftover Ring reference, and re-signing in with the Ring key when a saved session is rejectedPaykitSdkServiceTest.kt— a picked pubky's secret is read just in time from the Pubky Ring providerPubkyChoiceViewModelTest.kt— Ring pubkys are listed with their profile, and picking one continues to Create Profile, Contact Import or Pay Contacts, with an error toast when adoption or the follows lookup failsCreateProfileViewModelTest.kt— a failed lookup blocks saving for a signed-in or stored pubky, while a new pubky can still be createdAppViewModelSendFlowTest.kt— apubkyauthrequest shows the "Use Pubky Ring" toast when a picked pubky's key can't be readPubkyRingAuthCallbackTest.kt— the relay callback it covered no longer existsPubkyRepoTest.kt— relay auth start, completion, cancel and callback nonce cases, and the external-session backup cases, because the relay flow and that session kind no longer existPubkyChoiceViewModelTest.kt— the Ring launch and relay approval cases, because Pubky Choice no longer opens Pubky RingAppViewModelSendFlowTest.kt— the Pubky Ring callback deep link case, because the callback no longer exists