Skip to content

feat: support Pubky signup - #1224

Open
ben-kaufman wants to merge 25 commits into
masterfrom
codex/pubky-ring-signup
Open

feat: support Pubky signup#1224
ben-kaufman wants to merge 25 commits into
masterfrom
codex/pubky-ring-signup

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Description

  • Accept app-authorized pubkyring://signup and auth-bearing pubkyauth://signup requests, plus direct pubkyauth://direct_signup and parameter-only legacy pubkyauth://signup, through the normal scanner and deep-link flow.
  • Register the wallet-derived Pubky identity with the requested Homeserver and optional signup token.
  • Require explicit approval and the existing PIN/biometric check for every signup format; show the requested homeserver before registration.
  • Restore locally owned identities by signing in on their existing homeserver, keeping credentials retryable after failures instead of registering at Homegate.
  • Activate the returned Paykit session and continue into the existing profile setup flow.
  • Reuse the approval sheet’s loading state while signup completes, report Already signed in when a local identity exists, and keep payment-only scanner state intact when rejecting Pubky requests.

This PR is stacked on #1200 and uses its Paykit rc51 authorization model. Ordinary Pubky App sign-in must use that grant-auth model; compatibility with the older sign-in request is intentionally outside this signup PR.

Preview

Not included; this reuses the existing scanner, authorization approval sheet, loading treatment, and profile setup UI.

QA Notes

  1. With no Pubky identity, scan a signup QR from staging Pubky App; approve it, confirm the website continues, and complete profile setup.
  2. With no Pubky identity, scan either direct signup format; confirm the homeserver is visible, Cancel performs no registration, and Authorize requires local authentication before profile setup opens.
  3. Scan a signup request while signed in and confirm the Already signed in state.
  4. Confirm payment-only scanners reject Pubky requests without clearing the current payment flow.
  5. With the wallet locked, open a signup deep link; confirm nothing registers or opens until unlocking, then the approval sheet appears.
  6. After signup on a non-default homeserver, make session recovery fail, restore connectivity, and retry Create/Restore Profile. Confirm it keeps the same homeserver. Repeat with a failed profile save and confirm retry never calls signup.

Validation:

  • Full testDevDebugUnitTest: 2,346 tests passed on the restacked branch.
  • compileDevDebugKotlin and detekt passed.
  • Regression coverage includes locked signup deep links, consent/local-auth ordering, real SDK activation and partial-persistence cleanup, stale pending state, and one-time profile navigation.
  • PubkyRepoTest.kt: 97 tests passed, including existing-key recovery, secure-storage errors, sign-in/profile failure retries, cancellation, and no-key signup. Detekt reports no changed-file findings.

@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from d02c926 to 09dff52 Compare September 2, 2026 22:23
@greptile-apps

greptile-apps Bot commented Sep 2, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds Pubky Ring signup URL parsing and scanner routing, registers the wallet-derived identity with the requested Homeserver, activates Paykit, and resumes profile setup through durable local state.

  • Adds parsing and validation for pubkyring://signup requests.
  • Adds registration, requester authorization, session activation, and profile-setup continuation.
  • Preserves payment scanner state when Pubky requests are rejected and reports existing local identities.

Confidence Score: 3/5

This PR should not merge until signup activation can recover from the second network operation failing and pending profile setup can be exited without an immediate navigation loop.

The new signup sequence can complete remote registration and authorization while leaving Bitkit without a local session, and the successful path's durable pending marker makes the CreateProfile back action ineffective.

Files Needing Attention: app/src/main/java/to/bitkit/services/PaykitSdkService.kt, app/src/main/java/to/bitkit/repositories/PubkyRepo.kt, app/src/main/java/to/bitkit/ui/ContentView.kt

Important Files Changed

Filename Overview
app/src/main/java/to/bitkit/models/PubkyAuthRequest.kt Adds strict Ring signup parsing, query validation, and conversion into the existing Pubky authorization request model.
app/src/main/java/to/bitkit/repositories/PubkyRepo.kt Coordinates signup registration, authorization, activation, and pending profile state, but the multi-step flow can strand remotely completed signup without a local session.
app/src/main/java/to/bitkit/services/PaykitSdkService.kt Adds registration without activation, discarding an activatable signup result and requiring a failure-prone second sign-in.
app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt Routes Ring signup through the normal scanner while explicitly rejecting Pubky requests in payment-only contexts.
app/src/main/java/to/bitkit/ui/ContentView.kt Resumes pending profile setup automatically, but conflicts with the unchanged dismissible CreateProfile back action.
app/src/main/java/to/bitkit/ui/screens/profile/PubkyAuthApprovalViewModel.kt Dispatches Ring signup approval, handles existing identities, and dismisses the approval sheet before profile setup.

Sequence Diagram

sequenceDiagram
    participant Scanner
    participant Approval as Approval UI
    participant Repo as PubkyRepo
    participant Server as Homeserver
    participant App as Requesting app
    participant Paykit
    Scanner->>Approval: Pubky Ring signup request
    Approval->>Repo: Approve signup
    Repo->>Server: Register derived identity
    Repo->>App: Approve authorization
    Repo->>Paykit: Sign in and activate session
    Paykit-->>Repo: Active local session
    Repo-->>Approval: Dismiss approval
    Repo->>Repo: Mark profile setup pending
    Repo-->>Scanner: Navigate to profile setup
Loading

Reviews (1): Last reviewed commit: d02c926 | Re-trigger Greptile

Comment thread app/src/main/java/to/bitkit/services/PaykitSdkService.kt
Comment thread app/src/main/java/to/bitkit/ui/ContentView.kt
@ben-kaufman ben-kaufman changed the title feat: support Pubky Ring signup feat: support Pubky signup Sep 2, 2026
@piotr-iohk

piotr-iohk commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Regtest device QA, home Scan, QR from staging.pubky.app.

Staging e2e doesn’t finish — is that expected?
Signup QR does create a Bitkit identity and Create Profile (ring2 / pubky9m…). Staging stays on the Ring / QR step after Authorize. Sign-in QR from the same flow is rejected (only Pubky grant auth URLs are supported). So this isn’t e2e with the Pubky staging app — Bitkit gets a local profile, the website never continues. If approveRingAuth is supposed to complete that session, it’s a bug. If the site only listens for Ring, say so; Ben’s “QR → sheet → profile” case is then only the Bitkit half.

No spinner after scan.
Scanner closes, home looks unchanged for ~18s while register/activate runs. Feels like the scan did nothing, worse when staging also doesn’t move. “Deriving your keys…” on Create Profile is too late. Need a loader from scan until the profile screen.

Already signed in toast works and the scanner closes (iOS leaves it up — noted on #724).

bitkit_logs_2026-09-03_09-44-43-android.zip

Screen_Recording_20260903_113712_Bitkit.Regtest-android.mp4

@ben-kaufman

Copy link
Copy Markdown
Contributor Author

@piotr-iohk Thanks for the device QA and logs. You found a real signup interop bug: Bitkit treated every pubkyauth://signup request as direct signup, even when Pubky App included relay, secret, and caps. I pushed a fix so auth-bearing signup requests now register the identity and then complete the relay authorization; parameter-only/direct signup still skips app authorization. Direct signup also shows progress while registration and activation run.

The ordinary sign-in QR rejection is separate. These PRs use Paykit rc50’s app-scoped grant auth model, while staging Pubky App is still generating the older auth request format. Pubky App needs to update its sign-in flow to the new grant model for ordinary sign-in to work with Bitkit. Could you please recheck the staging signup path on this head?

@piotr-iohk

piotr-iohk commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Retested the rebased head on a physical Samsung S22 using the regtest build and a signup QR from staging.pubky.app.

Signup now completes end to end: authorization progress is visible, Bitkit creates the Pubky identity and opens profile setup, and the staging website continues successfully. The logs confirm the authorization completed and the local identity/session were created.

Ordinary sign-in still returns Authorization failed because staging currently generates the older non-grant authorization request. As clarified, that is outside the scope of this signup PR.

Non-blocking UI parity note: the approval screen differs between platforms. iOS (left) always shows the placeholder profile card, while Android (right) omits it because no profile exists yet. It would be good to align the intended design, but I don’t think this should block the signup fix.

Screenshot 2026-09-03 at 16 50 53

bitkit_logs_2026-09-03_14-48-43.zip

Screen_Recording_20260903_164721_Bitkit.Regtest.mp4

piotr-iohk
piotr-iohk previously approved these changes Sep 3, 2026
@ovitrif

ovitrif commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

needs conflict resolution @ben-kaufman

@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from 504ae04 to 94bc950 Compare September 3, 2026 19:03
ovitrif

This comment was marked as resolved.

@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from ea18746 to f331ad8 Compare September 3, 2026 22:16
@ben-kaufman
ben-kaufman requested a review from ovitrif September 3, 2026 22:18
@ben-kaufman
ben-kaufman force-pushed the codex/paykit-rc50-auth branch from 21d5a06 to a681dfc Compare September 4, 2026 12:24
@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from f331ad8 to ef49aa1 Compare September 4, 2026 13:19

Copy link
Copy Markdown
Contributor Author

I checked the failing pubky_paykit CI shard. It repeatedly fails the profile/contact-edit scenario with concatenated old and new text (ALICE WALLET AALICE WALLET A, Wallet A original notesEdited from wallet B), rather than failing Ring signup. The shared E2E helper calls clearValue() then setValue() without checking the clear took effect; the app's edit handlers replace the name/bio state directly and do not append it. This points to the Android input-clearing/automation path, but I have not reproduced the cause locally yet, so I am not calling this CI failure resolved or a proven harmless flake. The previously discussed signup-session preservation fix and its regression coverage are present; I resolved that answered review thread.

@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from ef49aa1 to ff24be9 Compare September 4, 2026 22:15

Copy link
Copy Markdown
Contributor Author

Restacked onto the updated #1200 head (580b042fd) and pushed as ff24be924. All five signup commits are signed; range-diff confirms their patches are unchanged. The only resulting tree changes are the parent PR's SDK-version/changelog update. The previously reported profile-edit E2E issue is not being marked fixed by this restack; the new head still needs CI verification.

ovitrif

This comment was marked as resolved.

ovitrif

This comment was marked as resolved.

@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from ff24be9 to f54f1f3 Compare September 6, 2026 15:04
@ovitrif ovitrif added this to the 2.6.0 milestone Sep 9, 2026
ovitrif

This comment was marked as resolved.

ovi-reviewer[bot]

This comment was marked as resolved.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

15aca5075 closes it. You took the second option — scoping the Authorizing refusal to a live inFlightAuthorization rather than settling the state on the terminal paths — and it works for both branches at once, since the guard keys on the in-flight record instead of on which branch was taken.

Trace: after either terminal path returns, the finally at :161 CASes inFlightAuthorization back to null. Re-opening the same URL then hits load:55 (in-flight restore skipped) → resetForLoad:63 → state is Authorizing but inFlightAuthorization.get()?.authUrl == authUrl is null == authUrl, so isAuthorizing is false → CAS to {authUrl, Loading} at :278parseAuthUrlAuthorize with real buttons. No SuccessContent flash either, since neither path touches _uiState.

No new door opened: resetForLoad(A) is unreachable while A is genuinely in flight because load:55-62 returns early on the same condition, and a second approveSignupAuth is independently blocked by the untouched inFlightAuthorization.compareAndSet(null, …) at :149 plus initializeMutex and the hasIdentity() re-check in PubkyRepo.approveSignupAuth:1006-1013. A UI-state reset can't bypass those.

terminal signup outcomes allow the same URL to reload for consent (PubkyAuthApprovalViewModelTest.kt:240-270) is a real regression test — revert the :270-271/:274 hunk and it fails on both the assertEquals(Authorize, …) at :261 and the parseAuthUrl verification at :262, and it loops both Result.success and PubkyAlreadySignedInError.

One sibling case inline.

if (
currentState.authUrl == authUrl &&
currentState.state in setOf(ApprovalState.Authenticating, ApprovalState.Authorizing)
(currentState.state == ApprovalState.Authenticating || isAuthorizing)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low, dev/QA-facing — the Authorizing half is fixed, but Authenticating can strand a URL the same way, via a dismissal the VM never hears about.

With PIN enabled: Authorize → requestAuthorize:104-111 sets AuthenticatingAuthCheckView overlays inside the sheet's Box (PubkyAuthApprovalSheet.kt:80-98, :149-164). If the user then taps the scrim, swipes down, or presses system back, none of PubkyAuthApprovalSheet.kt, AuthCheckView.kt or BiometricsView.kt declares a BackHandler or DisposableEffect, so the event goes straight to SheetHost.kt:181-195onDismissContentView.kt:499 appViewModel.hideSheet(), bypassing the VM entirely. cancelLocalAuth is only wired to AuthCheckView.onBack / BiometricsView.onFailure (:160, :175), neither of which fires on host-driven dismissal.

So the VM stays {authUrl, Authenticating} with no pending prompt, and unlike the Authorizing branch there is no in-flight record to invalidate it — this guard returns false on the flag alone. Re-scanning the same static direct-signup URL renders AuthorizingContent: spinner, no PIN pad, no buttons, for the life of the Activity. Same symptom you just fixed, reached from the sibling state. Biometrics is mostly immune, since the system prompt's cancel drives onFailurecancelLocalAuth.

The new test at :217-222 pins the Authenticating refusal as intended but doesn't cover host-driven dismissal, so it passes either way.

Smallest fix keeps the guard and releases the state when the overlay goes away — in PubkyAuthorizationLocalAuth:

DisposableEffect(Unit) { onDispose { pendingAuthUrl?.let(viewModel::cancelLocalAuth) } }

Alternatively collapse AuthenticatingAuthorize from the sheet host's onDismiss.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dismissing the approval sheet now clears its pending authentication state. Reopening the same URL returns to consent and requires local authentication again.

@ben-kaufman

Copy link
Copy Markdown
Contributor Author

Moved toast initialization before the startup collectors, so an immediate session-recovery error can show its toast without crashing the app.

ovi-reviewer[bot]

This comment was marked as resolved.

@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from c3d33f1 to d828fc9 Compare September 9, 2026 19:45
@ben-kaufman

Copy link
Copy Markdown
Contributor Author

For QA 1, Bitkit waits for the relay approval call to finish before opening Create Profile. That does not confirm the companion received the response. The screenshot shows 127.0.0.1:3001, while the report describes staging and production. Could you confirm the companion commit, homeserver and relay configuration, and capture redacted browser console/network errors for a fresh signup request kept open in the same tab?

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

d828fc945 closes it, and the approach is better than either option I offered.

Rather than hooking PubkyAuthorizationLocalAuth or the sheet host's onDismiss, you put a DisposableEffect(viewModel, authUrl) on the sheet root (PubkyAuthApprovalSheet.kt:79-81) calling cancelLocalAuth(authUrl). That catches every dismissal route — back (SheetHost.kt:181-188), scrim (:190-195) and swipe (confirmValueChange:128 permits Hidden since dismissEnabled is true for PubkyAuth; ContentView.kt:472 only restricts Sheet.Send) — because they all converge on hideSheet()_currentSheet = null → the composable leaves composition. It also covers two cases I hadn't raised: Activity recreation (rotating during the PIN pad used to strand Authenticating with no pad) and sheet replacement by a TimedSheet or incoming-payment sheet.

Nothing new opened. cancelLocalAuth guards on Authenticating only (:139), so it's a deliberate no-op during Authorizing — correct, that state is genuinely in flight, and its terminal cases were already handled by 15aca5075's inFlightAuthorization predicate. There's no window between local-auth success and Authorizing either: AuthCheckView.onSuccess:158-162 and BiometricsView.onSuccess:173-177 call confirmAuthorize synchronously in the same callback. Key-change ordering is safe since Compose dispatches forgets before remembers, and the cancel is URL-guarded anyway.

The toastManager move to :423 is right for the same commit's NPE: Kotlin runs initializers in textual order, and init:493 collects an already-true sessionRestorationFailedToastEventBus.sendtoast()toastManager.enqueue, all inline on Main.immediate inside the constructor, before the old :4508 declaration had run. AppViewModelSendFlowTest.kt:271-296 is a real guard for it — on revert enqueue is never reached and the verify fails.

One coverage note, not a defect: PubkyAuthApprovalViewModelTest.kt:226-228 passes on head~1 too, since after cancelLocalAuth the state is Authorize and load on Authorize already re-parsed. The Compose fix itself has nothing driving it — PubkyAuthApprovalSheetTest.kt only covers resolvePubkyApprovalLocalAuthMode.

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one blocker in the Ring signup failure path.

Replies

ben-kaufman: Moved toast initialization before the startup collectors, so an immediate session-recovery error can show its toast without crashing the app. (comment)

I verified the construction order and its focused regression on the current code; the startup failure path is covered and this is fixed.

ben-kaufman: For QA 1, Bitkit waits for the relay approval call to finish before opening Create Profile. That does not confirm the companion received the response. (comment)

The previous run did not record the companion commit, relay configuration, or browser diagnostics, so that result does not isolate a Bitkit defect. I have marked it for a fresh device run with those details captured.

Coverage

Total: 58% (whole PR, 23 files)

  • Journeys: 83% - Five of six signup journeys have regression coverage; Ring signup lacks bounded recovery when relay delivery never returns.
  • Unit tests: 92% - Focused tests cover signup parsing, routing, ordering, rollback, state, and navigation; none covers a non-returning Ring relay retry.
  • QA: 0% - Manual Tests not run

Reviewed by Codex (gpt-5.6-sol-high) via gh-pr-review-loop skill

homeserverZ32 = requireNotNull(request.homeserverPublicKey),
signupCode = request.signupToken,
)
request.authorizationUrl?.let { pubkyService.approveRingAuth(it, secretKeyHex) }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that this awaits the Ring relay with no timeout, while the pinned bitkit-core/pubky client has no global HTTP request timeout. If the relay accepts a connection but stops responding, this coroutine stays in Authorizing, retains inFlightAuthorization, and holds initializeMutex, so the user cannot retry signup without restarting the app. Could we bound this call with a Kotlin Duration, restore the sheet to Authorize on timeout, and add a test that proves a retry can proceed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bounded the Ring relay call to 30.seconds. A timeout returns the sheet to consent, and a manual retry requests local authentication again.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean — no new findings. Every real issue on this surface is already in the thread history, either fixed, accepted, or still open as ovi-reviewer's relay-timeout thread 3976005716, which I'm not re-raising.

Key material. Secret is seed-derived (PubkyRepo.deriveKeys:538deriveLocalSecretKeyFromWalletSeed:1370), passed in-process to bootstrap().signUp (PaykitSdkService.kt:292) and bitkitcore approvePubkyAuth (PubkyService.kt:143), and persisted only via persistSessionAccess (:909, :913) into the AndroidKeyStore-backed Keychain — identical to the pre-existing signIn. No new recovery blob: snapshotSessionBackupState emits LocalSeed kind with no secret. Logging is closed: scanLogId routes through sanitizedQrLogValue() (redacted#<sha256[0:8]>), deeplinks through sanitizedDeeplinkLogValue() (drops query and fragment), and every new parse error message in PubkyAuthRequest.kt is value-free (:249, :253).

Authorization / TOCTOU. requestAuthorize CASes on state.authUrl == authUrl && state == Authorize (PubkyAuthApprovalViewModel.kt:106), confirmAuthorize CASes inFlightAuthorization (:149), and authorize() re-parses the same immutable URL with pure functions (:167) — so the homeserverPublicKey rendered at PubkyAuthApprovalSheet.kt:411-418 is the value handed to registerIdentity (PubkyRepo.kt:1016). approveSignupAuth runs under initializeMutex (:1006) with hasIdentity() checked both before and after deriveKeys() (:1010, :1013), so a Ring-delegated session, a RestorationFailed identity, or a concurrent initialize() all block a second identity. One production caller, reachable only via Authorize → local auth → confirmAuthorize.

Deeplink surface. MainActivityPubkySignup is enabled="false" / exported="true" (AndroidManifest.xml:184-185), with no flavour manifest overriding it, flipped solely by PubkyAuthHandlerRegistrar.kt:67 on isPaykitUiEnabled && !hasIdentity. Any app can fire the intent while it's enabled, but the most it reaches is the consent sheet: processDeeplink requires Paykit on and a wallet (AppViewModel.kt:5172), launchScan defers while !_isAuthenticated (:2076), and flushDeferredScan refuses until authenticated (:2203).

Lifecycle / partial failure. registerIdentity persists nothing, so an approveRingAuth failure after it leaves only a remote account that rc51's 409 path recovers on retry. activateRegisteredIdentity failure runs clearRegisteredIdentityActivationLocked under NonCancellable (:314, :935), clearing both keychain entries plus SDK state; a failed pending-flag write runs forgetSessionAccess + clearLocalState. Wipe is complete: WipeWalletUseCase.kt:62wipeLocalState, then settingsStore.reset() at :69 clears the whole local_settings store, so the pending flag can't survive into a new wallet.

Cancellation. The VM is hiltViewModel() in the sheet host (ContentView.kt:593-597), a sibling of RootNavHost, so it's Activity-scoped and sheet dismissal cannot cancel approveSignupAuth. Every rollback is NonCancellable. No new runCatching around suspend calls. A dropped Dismiss effect is harmless because CreateProfile navigation is driven by the persisted flag.

Cross-identity. activateBootstrapResult deletes PAYKIT_SDK_STATE when the public key changes (:926-928); sharesPrivatePaykitEndpoints is reset before registration; the receiver noise key is seed-derived and so identical for the same wallet by construction.

Standing limitation, unchanged: PubkyAuthErrorMessage.kt:29 still falls back to message for unknown Rust errors. That's a pre-existing sink shared with ordinary auth, and my local bitkit-core checkout predates the pubky module, so I still can't read those strings — same limitation I stated before, not a new finding.

Parity with synonymdev/bitkit-ios#724, three things worth cross-feeding:

  1. iOS is safer on the relay hang. PubkyProfileManager.approveSignupWithTimeout races the relay POST against a 30 s Task.sleep and explicitly refuses to activate a late response. Android awaits approveRingAuth unbounded while holding initializeMutex — that's ovi-reviewer's open thread, and the iOS implementation is a ready-made shape for the fix.
  2. Correcting something I said on the iOS side: I'd previously called iOS free of any deeplink vector. That's wrong, and it cuts against the "Android's surface is strictly larger" framing. iOS registers lightning/lnurl* in Info.plist, and handleScannedData strips those prefixes at AppViewModel.swift:480 before testing PubkyAuthRequest.isProtocolURL at :536 — so lightning:pubkyauth://signup?hs=… opened from a web page reaches the same approval sheet. It's gated at :540 and ends at a consent screen, so it's LOW there, but the two platforms have the same shape of exposure, not different ones. Detail is on that PR.
  3. Android is safer on mid-flight teardown. Android's approval VM is Activity-scoped so activation completes even if the sheet is replaced; on iOS performAuthorization runs in a view-owned task, so a programmatic sheet replacement could cancel it between approveAuth and activateIdentity.

Gating: dev/QA-facing today.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix confirmed at e3670e99 — ovi-reviewer's thread 3976005716 can be closed. One new LOW inline, which is pre-existing and only surfaced because chasing this fix finally let me read the bitkit-core internals I'd twice flagged as unverified.

The bound works, and a late relay response cannot activate anything. PubkyService.kt:155-157 wraps the exact relay await:

withTimeoutOrNull(timeout) { approvePubkyAuth(authUrl, secretKeyHex) } ?: throw PubkyRingAuthTimeoutError()

approvePubkyAuth has exactly one caller (PubkyRepo.kt:1021), so the non-signup approval paths are byte-identical.

The part the new tests don't prove — they mock the FFI with delay — is whether the cancel reaches the socket. It does: bitkitcore.android.kt:18102-18122 runs uniffiRustCallAsync in withContext(Dispatchers.IO), suspends in suspendCancellableCoroutine, and registers invokeOnCancellation { cancelFunc(rustFuture) }. On the Rust side (v0.5.14 src/modules/pubky/auth.rs:122-136) approve_pubky_auth awaits signer.approve_auth inline with no tokio::spawn and never touches the AUTH_FLOW mutex, so cancel/free drops the reqwest POST mid-flight rather than waiting on a hung socket.

Mutex release traced: PubkyRingAuthTimeoutError is an AppError, not a CancellationExceptionServiceQueue.kt:31 rewraps → escapes withContext(ioDispatcher) at PubkyRepo.kt:1008runSuspendCatching at :1007 returns failureinitializeMutex.withLock at :1006 unlocks in its finally.

Late response: there is no callback path. activateRegisteredIdentity (:1024), _publicKey (:1034) and setPubkyProfileSetupPending(true) (:1038) are all sequenced after :1021 returns normally; after the throw, control never reaches them. A response arriving after the bound resumes an already-cancelled continuation, which kotlinx discards. So no keychain write, no in-memory identity, no pending flag — the iOS guarantee holds by construction here, not just by convention.

On the catching pattern: neither runCatching nor the guarded TimeoutCancellationException shape is used, and neither is needed. withTimeoutOrNull converts only its own timeout into null and rethrows a foreign CancellationException, so structured-concurrency cancellation is preserved and the surfaced error is a plain retriable AppError. That's cleaner than the BlocktankRepo.refreshCjitEntries precedent, and PubkyServiceTest's "relay cancellation propagates" case pins that a real cancellation isn't laundered into a timeout. No rollback is needed on this path because nothing local is written between registerIdentity (:1016, an in-memory bootstrap().signUp result) and the throw; the two NonCancellable rollbacks at :1028/:1042 are unchanged.

RING_AUTH_TIMEOUT = 30.seconds is a kotlin.time.Duration, KDoc'd, overridable, and matches iOS. approvePubkyAuth returns Unit, so the elvis can't misfire on a legitimate null. New string is alphabetical (strings.xml:626, between _missing_claim and _title).


Closing my standing limitation. I've said twice on this PR that I couldn't judge PubkyAuthErrorMessage.kt's raw-message fallback because my local bitkit-core checkout predated the pubky module. I fetched v0.5.14 and read it, so that's now settled — and the answer is that it does leak attacker-controlled text, though no key material. Detail inline. It's pre-existing and gated, so it doesn't affect this PR's verdict; I'm raising it as its own thread rather than on 3976005716 because the same fallback serves the pre-existing paykit approveAuth path too.

Observation, not a finding: registerIdentity (:1016) is still unbounded from Bitkit's side, but it goes through paykit's client, which has its own PubkyClientConfig.requestTimeoutSecs — a different client from the one ovi's thread was about, and the same shape as the pre-existing createIdentity → signUp. Not asking for a second bound.

}
current = current.cause
}
return message

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low, gated, and pre-existing — this commit only moved the when into pubkyAuthMessageResource(), it didn't introduce the fallback. Raising it now because I can finally substantiate it: I'd previously said I couldn't read the bitkit-core error strings, so this was left unverified. I fetched v0.5.14 and read them.

else -> null at :33 means any PubkyException from bitkit-core falls through to return message here, and that message carries the relay's response body verbatim.

The chain: approve_pubky_auth maps every failure to PubkyError::AuthFailed { reason: e.to_string() } (v0.5.14 src/modules/pubky/auth.rs:133-135) → Kotlin PubkyException.AuthFailed.message = "reason=$reason" (bitkitcore.common.kt:4341-4346) → copied by AppError(cause) (Errors.kt:12) → lands here. bitkit-core v0.5.14 pins pubky = "0.6.0", whose signer/auth.rs:38-75 and errors.rs:123-139 produce, among others:

  • "Server responded with an error: <status> - <response body>" — the relay's body verbatim
  • "HTTP transport error: error sending request for url (<relay>/<base64url(hash(secret))>)" — relay URL plus channel id

Never the secret, never the token (encrypted, request body only), never key material. So this is not a key leak.

What it is: attacker-controlled text rendered inside Bitkit's own chrome. Concretely — QR pubkyring://signup?hs=<valid z32>&relay=https://attacker/inbox/&secret=<32B>&caps=…, user consents (homeserver and trust warning are shown, which is the accepted limitation), relay answers 400 with body Your Bitkit wallet is compromised, move funds to bc1q…. The user sees an error toast titled "Authorization Failed" whose body is reason=Request failed: Server responded with an error: 400 Bad Request - Your Bitkit wallet is compromised…, subject to the toast's line cap. The same text lands in QA log bundles.

Minimal fix, in the function this commit already touched — add a catch-all so remote text never reaches the UI:

is PubkyException -> R.string.profile__auth_error_generic

or return null at :18 and let the caller supply generic copy. Either way the timeout path you just added is unaffected: it resolves to a resource at :22 before reaching the fallback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unknown authorization errors now show the localized error title instead of raw exception text. Specific messages, including relay timeout, remain unchanged.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix confirmed at 1c43a454. PubkyAuthErrorMessage.kt:18 is now return null instead of return message, so no bitkit-core error text reaches the UI or the logs.

I checked the callers rather than assuming null is safe there — all four pass the result straight into toast(description = …), and toast's signature is description: String? = null (AppViewModel.kt:4515), so a null renders the title alone ("Authorization Failed"). That's the generic copy the finding asked for, with no caller doing an ?: error.message fallback that would defeat it. Call sites: PubkyAuthApprovalViewModel.kt:71 and :313, WatchOnlyAccountsViewModel.kt:79, AppViewModel.kt:5227.

The two new tests are on point — unmapped errors do not expose remote text in toast descriptions and wrapped known errors retain their localized descriptions. The second is the one that matters for not over-correcting: mapped errors still resolve through pubkyAuthMessageResource() and keep their localized strings, including the PubkyRingAuthTimeoutError case added in the previous commit.

That closes the last thing I had open on this PR. Clean from my side.

jvsena42

This comment was marked as low quality.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes: signing in to a third-party Pubky app with a Bitkit-managed identity dead-ends on this branch.

Screen_recording_20260910_115851.mp4

Repro

Dev/regtest build of this branch on an emulator, signing in to Loopky with the identity Bitkit created via the new signup flow. Bitkit takes the deeplink, then dismisses the approval sheet with a title-only error toast. Reproduced twice in one session (deeplink at 14:57, QR scan of the same URL at 14:55).

14:57:23.922 DEBUG [AppViewModel.kt:5053] Received deeplink 'pubkyauth://signin' - AppViewModel
14:57:37.378 ERROR [PubkyAuthApprovalViewModel.kt:67] Failed to parse auth request
  [AppError='code=protocol_error, context=only Pubky grant auth URLs are supported'] - PubkyAuthApprovalVM

The URL, from the sender's logcat (secret elided):

pubkyauth://signin?caps=%2Fpub%2Floopky%2F%3Arw%2C%2Fpub%2Fpubky.app%2F%3Arw
  &relay=https%3A%2F%2Fhttprelay.pubky.app%2Finbox&secret=…
  &x-success=loopky%3A%2F%2Flogin-callback&x-cancel=…&x-error=…&x-source=Loopky

Why it fails — Bitkit's side, and it already ships the fix

pubkyauth://signin?caps=&relay=&secret= is the cookie-flow auth URL that every released Pubky Ring understands (Ring bundles react-native-pubky 0.13.0 = pubky 0.9.x, whose parser knows signin/signup/direct_signup/session). Apps that want to work with shipped Ring have to mint that form; the pubky 0.10+ grant URL (pubkyauth://signin_grant?…) makes Ring answer "Unrecognized format". So the sender is not malformed — this is a pubky-generation mismatch, and Bitkit is on the far side of it.

PubkyRepo.parseAuthUrl (PubkyRepo.kt:996) routes everything that isn't a signup URL to paykit.parsePubkyAuthUrl. paykit 0.1.0-rc51 is built on pubky 0.11.0, whose parser accepts grant URLs only — strings on libpaykit.so has exactly the message we hit next to expected a Pubky grant auth URL.

But Bitkit also links a parser that does understand this URL: bitkitcore.parsePubkyAuthUrl (bitkit-core 0.5.14pubky 0.6.0; its PubkyAuthKind covers signin, signup, secret_export), imported at PubkyService.kt:18 as parseLegacyPubkyAuthUrl and used by this PR in validateSignupRequest — with bitkitcore.approvePubkyAuth already wired up in approveRingAuth. So this PR teaches Bitkit to accept the legacy signup intent, while its sibling signin still goes to the grant-only parser and dies.

Scope note, so this isn't misread: the sign-in fall-through is pre-existing on master (parseAuthUrl there has no signup branch and the same single-parser path). I'm raising it here because this PR is where the legacy-URL entry points are being defined and it already has both primitives in hand — happy for it to move to a follow-up if you'd rather keep this one tight.

Asks

  1. Fall back to the legacy parser for non-grant pubkyauth:// URLs. In PubkyRepo.parseAuthUrl, when paykit.parsePubkyAuthUrl rejects a URL as non-grant, retry through parseLegacyPubkyAuthUrl rather than surfacing a failure. Approval then has to go through bitkitcore.approvePubkyAuth (as approveRingAuth already does), not PubkyRepo.approveAuth — that one lands in approvalBootstrap (PaykitSdkService.kt:1000-1006), which re-parses with the same grant-only parser and validates a clientId these URLs don't carry.

  2. Don't advertise handling for URLs Bitkit rejects. MainActivityPubkyAuth claims the entire pubkyauth scheme with no host (AndroidManifest.xml:172-177, pre-existing). On my device Pubky Ring is not installed, and pubkyauth://signin resolved straight to to.bitkit.dev/to.bitkit.ui.MainActivityPubkyAuth — Loopky's "is a Pubky authenticator present?" probe is literally pubkyauth://signin, so Bitkit answers yes and then can't serve what it invited. Either land (1) or restrict the filter to the hosts Bitkit can actually process (signin_grant, signup_grant), the way the new signup alias is scoped.

  3. The toast has no description. localizedPubkyAuthMessage maps only PubkyAuthRequestError, WatchOnlyAccountError and the Ring timeout, so a paykit protocol error falls through to null and the user gets a bare "authorization failed" title with no reason. Worth a generic fallback string.

@ben-kaufman

Copy link
Copy Markdown
Contributor Author

@jvsena42 We decided not to support legacy Pubky sign-in, so we will not add the legacy parser or approval fallback. Bitkit now advertises only the supported grant and signup links. Unknown authorization errors also show a localized description, with raw exception text kept hidden.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manifest restriction reviewed at a9c5c5c9. The restriction itself is correct and complete at the OS level — one LOW inline about a route that goes around it, which is the Android twin of what you fixed on synonymdev/bitkit-ios#724.

One correction to my own framing before anything else: the two added lines are on the authorization alias .ui.MainActivityPubkyAuth, not the signup alias. The signup alias is byte-identical to 1c43a454. So the predicate governing the changed filter is isPaykitUiEnabled && hasIdentity && hasSecretKey, not !hasIdentity.

Matched-URI set, before vs after. Android merges all <data> attributes within one filter as a cross-product, and a filter carrying any android:host requires a non-null host — so:

before after
MainActivityPubkyAuth pubkyauth: × any host including absent — pubkyauth://signin, pubkyauth:///?…, pubkyauth://session, pubkyauth://secret_export, … pubkyauth × host ∈ {signin_grant, signup_grant} only

That is a real narrowing, and I couldn't find an OS-level way around it: pubkyring:// isn't on this alias; absent-host forms (pubkyauth:///?…, opaque pubkyauth:signin_grant?…) are excluded rather than over-matched because AuthorityEntry.match returns NO_MATCH_DATA on a null host; scheme matching is case-sensitive so PUBKYAUTH:// fails closed; and pubkyauth://signin_grant.evil / pubkyauth://evil#@signin_grant don't resolve. Host matching is case-insensitive, so pubkyauth://SIGNIN_GRANT resolves — but paykit's parser then adjudicates, worst case an error toast.

Legitimate flow still works, which I checked rather than assumed, since an over-tight filter would be worse than the hole: paykit rc51's intent table carries signin_grant, signup_grant, direct_signup, signin, signup, so both advertised hosts parse. Bitkit's own outbound pubkyauth:///?relay=… is handed in-process to bitkitcore and never fired as an intent, so the narrower filter doesn't touch it, and the signup shapes live on the untouched signup alias.

Runtime gate holds: still enabled="false" / exported="true", and PubkyAuthHandlerRegistrar.kt:81 remains the only setComponentEnabledSetting writer in app/src/main. Nothing in this commit widens exported or touches the registrar.

The tests earn their keepPubkyAuthManifestTest now asserts the negatives, not just the positives: pubkyauth://?caps=rw, pubkyauth://signin, pubkyauth://grant, pubkyauth://session, pubkyauth://secret_export must resolve to nothing. Every one of those resolved to the auth alias before this commit, so the test genuinely fails without the manifest change.

And as I said, I checked the error-copy line myself: return nullreturn context.getString(R.string.common__error_body) still keeps bitkit-core text out of the UI, it just gives the toast a body instead of a title alone. Fine.

contactPaymentContext: ContactPaymentContext?,
allowPubkyAuth: Boolean,
) = withContext(bgDispatcher) {
val input = result.removeLightningSchemes()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low, gated — but it routes around the restriction you just added, so worth folding in here rather than later.

This line strips the lightning/lnurl prefix before the Pubky test at :2673, so a wrapped Pubky URL is delivered through MainActivity's always-on lightning filter and never consults either Pubky alias — meaning both the enabled="false" gate and the new signin_grant/signup_grant host restriction are bypassed at delivery time.

Trace for lightning:pubkyauth://signin?caps=…&relay=…&secret=… opened from any app or web page:

  1. Delivered to MainActivity (manifest :120, lightning scheme, always enabled).
  2. processDeeplink:5171 tests isProtocolUrl(uri.toString()) on the raw string → scheme is lightning → false → falls through to :5184 launchScan(...), which takes the default allowPubkyAuth = isMainScanner (true outside the Send sheet).
  3. handleScan:2626 strips → :2673 isProtocolUrl(input) now true → :2675 flag check → handlePubkyAuth → identity/secret-key checks → Sheet.PubkyAuth("pubkyauth://signin?…").

So the legacy signin shape you deliberately stopped advertising can still be presented for approval remotely. Same for lnurl:pubkyauth://… and LIGHTNING:… (the patterns at :5301-5302 are IGNORE_CASE).

Not a consent or lock bypass — the sheet still requires Authorize plus PIN/biometrics, and the in-app identity checks mirror the alias predicate. What it defeats is the delivery-time restriction, which is precisely what this commit set out to add.

Minimal fix, mirroring what landed on synonymdev/bitkit-ios#724 — decide Pubky-ness on the raw input rather than the stripped one. Either test result instead of input at :2673, or guard right here:

if (PubkyAuthRequest.isProtocolUrl(input) && input != result) {
    // a Pubky URL never legitimately arrives wrapped in lightning:/lnurl:
    return@withContext
}

A wrapped URL then falls through to coreService.decode, which has no handleDecodedScan branch for Scanner.PubkyAuth, so it lands on the existing warning toast — no new copy needed.

Worth a case in AppViewModelSendFlowTest: handleDeeplinkIntent("lightning:pubkyauth://signin_grant?…") must not produce Sheet.PubkyAuth. The manifest test can't cover this one, since the vector lives on MainActivity's filter rather than the aliases.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapped Pubky URLs now use the existing scan error path before payment state is reset. They no longer reach authorization through lightning or LNURL links.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants