Conversation
ec8ee8c to
41ddb6b
Compare
The first wipe step ran through the best-effort `step` helper, which logged and swallowed both the thrown read-back check and the returned Result. The shared mirror therefore stayed readable by Ring through node shutdown and the network-bound remote cleanup whenever the guard failed. Call disableSharedIdentityExport() directly and propagate its failure so the wipe stops before anything else is torn down.
Session teardown attempted all four credential deletes independently, so a failed PUBKY_SECRET_KEY delete could still be followed by a successful quarantine-marker delete. The suspect secret would then be restored on the next start and re-exported to Ring. Only clear the marker once the secret it guards is actually gone.
ad7ce1a to
68c9ee8
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68c9ee8a1f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0700ee2367
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Disconnecting an unavailable Pubky Ring identity dropped the session and cleared the sharing flags with cleanup marked done, so endpoints and the receiver marker published under that identity stayed live and the reconciliation loop had nothing left to retry. Remove them while the borrowed session still works, mirroring sign out, and keep the cleanup pending when the removal fails.
…te its secret Local state teardown deleted the managed secret and its quarantine marker independently, so a failed PUBKY_SECRET_KEY delete during sign out or a wallet wipe could still be followed by a successful marker delete. The surviving suspect secret would then be treated as a managed local identity on the next start, signed in with and re-exported to Ring. Only release the marker once the secret it guards is gone, matching the guarded credential teardown path.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Reviewed at b890fefab as a key-material and auth change. No HIGH, no MEDIUM. One LOW inline. A second LOW is a follow-up on the codex cleanup thread, posted as a reply there. Paykit/Pubky is dev-flag gated, so both affect opted-in users on released builds and neither blocks.
Checked and clean:
- Provider (Bitkit → Ring).
- Read and write access require the
signature-levelREAD_SHARED_PUBKYpermission, andgrantUriPermissionsis off. enforceCaller()requirescallingPackage == app.pubkyring, requires that package to belong to the calling UID, and requirescheckSignatures == SIGNATURE_MATCH.insert,updateanddeletethrow.- The credential row is returned only when the path pubky equals the locally derived pubky.
- Each variant has its own authority, so a dev or tnet build, or a same-package clone, cannot read the release key.
- Read and write access require the
- Discovery (Ring → Bitkit).
verifyRingProviderchecks package, authority, permission and signature before each query.readCredentialrejectscount != 1and a pubky mismatch.signInWithExternalCredentialre-derives the pubkey from the secret and compares it with the selected row, so the connected identity cannot differ from the one shown.
- Key material.
- The borrowed secret is never persisted:
shouldStoreLocalSecret = false,managedSecretForSessionPersistencereturns null, and the live access is stripped oflocalSecretKey. snapshotSessionBackupStatereturns null while a ref exists, and the ref is not in the metadata backup.- No secret or credential URI reaches a
Loggercall.
- The borrowed secret is never persisted:
- Upgrade from rc31 and rc51.
- Owned identities re-import with the same secret, and export is enabled after validation.
- Legacy Ring-authorized sessions import as external, with export disabled.
externalIdentityRefdefaults to null andignoreUnknownKeysis set, so both upgrade and downgrade decode.
- Wipe order.
disableSharedIdentityExport().getOrThrow()runs beforestopNode(), andclearLocalStatedisables export before deleting keys. - Concurrency. None of the
identityLifecycleMutexcallers acquires it while already holding it. Adoption rollback runs underNonCancellable. Choice effects useSharedFlow. - Gating note.
SharedPubkyProvideris exported in every variant, whateverisPaykitEnabledis set to. It serves a credential only when the export flag is1and an owned secret exists. So a user who opted in and later turned Paykit off still has their key readable by a same-signature Ring. That looks intended, but it is worth one line in the PR body.
b890fef to
41ebbd7
Compare
# Conflicts: # app/src/main/java/to/bitkit/ui/components/Button.kt
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 613bc78c24
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Delta since b890fefab (ab5362507, 25cbd4018, 3adc559ae, merge 613bc78c2): no HIGH, no MEDIUM.
createIdentityguard: fixed. A restored legacy Ring session is now rejected withPubkyAlreadySignedInError(:511), and the pending-setup branch resumes only when the stored secret derives the active key (:484-494). The Choice → Profile redirect is still gone, but that now costs a toast instead of an identity.- Private endpoints on Ring loss: partially fixed. It works when the network is up. The offline case still leaves endpoints published; follow-up on the existing thread.
- Lock ordering.
PrivatePaykitReponever takesidentityLifecycleMutex, so calling it under the lock can't invert. TheProviderbreaks the DI cycle. - Cross-identity. Every session-creating path runs
closeAndClear()before setting_publicKey. Backup restore applies METADATA before WALLET, so a carried-over marker cannot erase freshly restored private state. - Transient vs definitive (
ab5362507). A null cursor,SecurityExceptionor a dead provider keeps the ref and session. Only an uninstalled, untrusted or unsupported source, or a missing identity, disconnects. - Upgrade.
privatePaykitStateCleanupPendingdefaults to false, and unknown keys are ignored. - Merge. The only conflict was
Button.kt, which isn't PR-owned and was resolved to master.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26e67c319d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Delta since 613bc78c2 (d1e3ed253, 26e67c319): no findings.
26e67c319. Once the session is deleted, teardown finishes underNonCancellable, and the ref reset now happens together with the session deletion. Adoption rollback is still unconditional. Lock order isidentityLifecycleMutex→PrivatePaykitRepo→PaykitSdkService, with no reverse edge.d1e3ed253. OnlyclearUnavailableExternalIdentityLockedraises the quarantine marker, and restore now clears it only afterPUBKY_SECRET_KEYhas been deleted. No managed secret is left readable next to a borrowed ref. On LocalSeed restore the secret is re-derived from the seed, so nothing unrecoverable is deleted.- Still open: the offline Ring-loss case on the cleanup thread.
clearExternalSessionAccess()still runs even when the private endpoint removal failed, and the pending retry only clears local state. That is the same gap codex raised as "Retry endpoint removal before discarding cleanup state".
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Delta since 26e67c319 (f19a2c785, f37c8cd6d): two MEDIUMs, both gated behind Paykit and affecting only opted-in users on released builds, so this is not blocking. Posted as a reply on the retention thread (PubkyRepo.kt:1729), because both are side effects of the retention that thread asked for: the Ring session gets into the metadata backup during the retained window, and a rejected retained session blocks every identity entry point.
Checked and clean:
f19a2c785. A missing quarantine marker returns null without touching the keystore, and only a decrypt or datastore failure throws. A first launch after upgrade from a shipped tag therefore takes the null path. The provider andresolveStoredSessionInitializationfail closed on a read error.- Teardown order.
closeAndClearruns beforeclearExternalSessionAccess, and the marker is cleared only after local cleanup succeeds. Everything after session deletion isNonCancellable. A cancel during removal leaves the ref null, the marker set and the session retained, and the next retry converges. - Lock order is unchanged:
identityLifecycleMutex→PrivatePaykitRepo→PaykitSdkService, with no reverse edge. - Parity. iOS clears the session before deleting the reference, so it never has the ref-gone/session-kept state. Neither hole exists in synonymdev/bitkit-ios#643.
This branch has not been deployed
3 participants
Builds on merged #1084 and #1097.
Description
The Bitkit private key remains canonical. Ambiguous private state is quarantined rather than silently reclassified or deleted.
Release gate
Merge/release after Ring #342. Confirm Play-installed Ring and Bitkit signing certificates and pass final artifact interoperability.
Design
Bitkit handoff: Profile choice and contact import.
Validation
Current head
f37c8cd6dcontains master905640577; every PR commit is signed.git diff --checkpassed.EmptyNavDeepLinkDetectorcrashes on unchangedScreenDeepLinkRuntime.ktbefore producing findings. No rules, suppressions, or baselines were changed.Local mainnet manifest generation remains blocked because the available
google-services.jsonhas noto.bitkitclient.Manual Tests
Tests used fresh disposable zero-balance emulator wallets and staging identities.
/pub/pubky.app/:rw; requester verifies exact key/capabilities and no private key is copied.Automated coverage
SharedPubkyContractTest,SharedPubkyDiscoveryTest,SharedPubkyManifestTest,SharedPubkyProviderTest,PubkyStoreSerializerTest,PubkyRepoTest,PrivatePaykitRepoTest,PaykitSdkServiceTest,PubkyChoiceViewModelTest,PubkyRingAuthCallbackTest, andAppViewModelSendFlowTest.