feat: open pubky auth links - #722
Conversation
This comment has been minimized.
This comment has been minimized.
d8110c0 to
48b2191
Compare
48b2191 to
c108dee
Compare
There was a problem hiding this comment.
Reviewed the link as an attacker-controlled trust boundary. Validation is strict — exact scheme/host/path, no userinfo/port/fragment, exactly one x-bitkit-claim=watch-only-account-v1, caps must equal the fixed watch-only set, duplicate relay/secret rejected — and I could not escalate through it. The wrapper forces requiresBitkitClaim, so ordinaryApproval (the only path that forwards URL-supplied capabilities) is unreachable from the bitkit:// wrapper; the companion path passes the hardcoded watchOnlyAccountCapabilities constant. Raw pubkyauth:// requests are a separate matter: handleScannedData strips a lightning:/lnurl*: prefix before classification, so a lightning:pubkyauth://signin?...&caps=<anything> link does reach ordinaryApproval with URL-supplied caps — behind the consent sheet, the Paykit flag, and local auth where enabled. The claim payload is version|accountIndex|addressType|78-byte xpub — public material, no spending key. Secrets are not logged, and retained-link handling is single-slot and PIN-gated.
One medium, about provenance rather than authorisation. One thing I could not settle offline: whether Pubky Ring actually registers pubkyring:// — if not, Ring sign-in reports "not installed".
Minor gap while you are here: duplicate caps parameters are not rejected the way duplicate relay/secret are. Not exploitable (the companion path never reads URL caps, so a divergence yields rejection), but inconsistent with the other duplicate checks.
4f7a770 to
151170c
Compare
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1dzLxvzqbhAMSTyXamczc
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1dzLxvzqbhAMSTyXamczc
There was a problem hiding this comment.
Reviewed this specifically as a deeplink-gating problem, because I just confirmed a HIGH-severity hole of exactly this shape on the Android side (synonymdev/bitkit-android#1224 — a pubkyauth://direct_signup?hs=<attacker> link binds the wallet's seed-derived identity to an attacker's homeserver with no sheet and no PIN). iOS does not have that class of bug here. Recording the trace since it's the kind of thing worth having on file.
Every URL entry point, and its gating:
| Entry point | Effect | Gate |
|---|---|---|
.onOpenURL (AppScene.swift:149) |
retainDeepLink(url) — store only |
none needed, no side effects |
Cold start scene(_:willConnectTo:) (SceneDelegate.swift:40, :51-54) |
DeepLinkRouter.forward → store |
store only |
Warm scene(_:openURLContexts:) (SceneDelegate.swift:69-73) |
store | store only |
AppDelegate.application(_:open:) (BitkitApp.swift:43-50) |
store (not invoked in a scene lifecycle) | store only |
AppScene.onAppear router drain (:287-289) |
store | store only |
Sole consumer — MainNavView .task(id:) / .onChange (:326-333 → :651-712) |
handleScannedData / ring callback |
MainNavView mounts only when walletExists, not initializing/restoring/migrating, and isPinVerified || !pinEnabled (AppScene.swift:326-429) |
Same gate as the QR scanner, so no mismatch — which is precisely where Android went wrong. Backgrounding sets isPinVerified = false (AppScene.swift:743), unmounting MainNavView and cancelling its .task.
Why the Android primitive isn't reachable: the Android hole was a direct_signup link binding the identity with no sheet and no PIN. iOS always presents the approval sheet, which is the part that matters.
The reachable link surface is wider than the bitkit:// wrapper, though. Info.plist registers lightning/LIGHTNING, and handleScannedData strips those prefixes before classification (AppViewModel.swift:492), so lightning:pubkyauth://direct_signup?hs=<z32> and lightning:pubkyauth://signin?...&caps=<anything> are both OS-deliverable and both skip requiresBitkitClaim (PubkyAuthRequest.swift:126). The claim contract — exactly one x-bitkit-claim=watch-only-account-v1, caps equal to the fixed watch-only set, host hardcoded to signin_grant — applies only to bitkit://pubky-auth/setup. So an hs= rebinding param and a generic capability grant are link-reachable; what stops them is the consent sheet, not the parser.
On that sheet the attacker-supplied homeserver z32 is rendered (PubkyAuthApprovalSheet.swift:262-274) and the requested capabilities are itemised (:253-255). Claim-carrying wrapper requests take two taps (watch-only consent, then authorize); non-claim requests open directly in .authorize (:84), so one tap. Local auth is required whenever PIN or biometrics is enabled, and skipped via PubkyApprovalLocalAuthMode.none when the user has neither (:22-24, :408-409). Everything is behind PaykitFeatureFlags.isUIEnabled (:553), default false. Ring callbacks are dropped unless the nonce matches the active attempt (PubkyProfileManager.swift:607-613).
Also checked and ruled out: .task(id:) cancelling an in-flight handler (bitkit-core's uniffiRustCallAsync has no cancellation handler and handleScannedData has no checkpoints, so the sheet still presents); duplicate delivery via onOpenURL + scene delegate (deduped by .onChange equality); routePendingDeepLinkIfReady clearing pendingDeepLinkURL synchronously before the first await (AppViewModel.swift:124-131), so concurrent callers can't double-handle. relay is displayed as origin only and its query is stripped from logs (MainNavView.swift:723-742).
One caveat on my own work: I read pubky 0.9.3 from the cargo registry to confirm Signer::approve_auth only posts an encrypted token to the relay and never calls signup; the lock pins 0.8.0, which I did not have locally.
Optional, non-blocking, and deliberately not touching the retain/replay design you three already settled: a retained URL has no expiry. A link tapped while locked or pre-onboarding is replayed whenever MainNavView next mounts — potentially days later, e.g. a payment link opening the Send sheet right after wallet creation. Consent taps still apply so it is not a bypass, but a staleness check in routePendingDeepLinkIfReady would tighten it.
An uncertain broadcast may still confirm, so discarding the signed transaction on the way out must not delete the proof prepared for it. Leaving the pending row behind is recoverable; deleting it is not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1dzLxvzqbhAMSTyXamczc
fix: unlock send sheet after failed broadcast
42affae to
4d0a0b6
Compare
jvsena42
left a comment
There was a problem hiding this comment.
One LOW inline — a documentation claim rather than a code defect. Not blocking.
Scope note, so nobody reviews #729 twice: the four hardware-wallet files (HwFundingSigner.swift, HwSendSignView.swift, HwFundingSignerTests.swift, changelog.d/next/729.fixed.md) plus the .broadcastConnectivity toast strings are byte-identical to origin/master — git diff --quiet pr722 origin/master passes for all four. They're #729, merged to master at 12:27Z, showing up here only because the base branch codex/pubky-ring-signup hasn't synced master. The whole block vanishes once #724 merges master. Nothing about what the device signs changed.
Similarly, bcc07259a re-carries #724's signup parsing under this branch; I assessed it only for interaction with this PR's link delivery, not as new work.
Delta since my last review: the branch was rebased, then formatting-only 4d0a0b659 and the #724 re-stack. My relay-origin thread is addressed and survives the rebase unchanged. No over-correction, and nothing in the retention/gating design moved.
Checked and clean:
- Request pinning (TOCTOU). The sheet reads only
config.request(an immutablelet) and passesconfig.request/config.request.rawUrltoperformAuthorization. A second link arriving mid-approval goes throughshowSheet, which callshideSheet()first — the.sheet(item:)tears the view and its@Statedown, dismissing theAuthCheckcover, and re-presents after 0.7s starting fresh at.watchOnlyConsent. An approval already past PIN completes on the captured request, never the new one. The signup path additionally guards onrawUrlequality. This is the approve-A-grant-B case and it's genuinely closed. - Lock-screen bypass. All five entry points only store. The sole consumer is
MainNavView, mounted only whenwalletExists, not initializing/restoring, andisPinVerified || !pinEnabled; backgrounding resetsisPinVerified, which unmounts it and the sheet.routePendingDeepLinkIfReadyclears the slot before its firstawait, so the.task(id:)+.onChangedouble-trigger can't double-handle. - Wrapper trust boundary.
isBitkitSetupHandoffrequires exact scheme/host/path, no userinfo/port/fragment, non-empty query not starting with?.normalizedProtocolURLhardcodes hostsignin_grantand passes raw query bytes through.parserejects duplicate relay/secret, then forces the claim check — exactly onex-bitkit-claim=watch-only-account-v1andcapsset-equal to the fixed constant.parseSignupthrows if a wrapper carried a claim, and a wrapper can't be a signup URL anyway. Companion approval passes the hardcodedwatchOnlyAccountCapabilities, neverrequest.capabilities. - Rendered attacker text.
relayOriginis rebuilt asscheme://host[:port]fromURLComponents— no path, query or userinfo — so it can't contain<to break the<accent>markup.sanitizedDeeplinkDescriptionstrips query and userinfo, and for opaquelightning:URLs logs only the scheme, sosecretnever reaches logs. - URL-scheme hijack. No security-relevant inbound secret rides on a claimable scheme. The one outbound URL carrying a secret is the Ring handoff, where Bitkit is the requester — a hijacker of
pubkyring://could only act as the signer, the same exposurepubkyauth://had before this PR. Ring callbacks are nonce-pinned. - Lifecycle. The
.task(id:)restarting when LDK reaches.runningcan't lose a pubky link:handleScannedDatareachesshowSheetwith noawaitbefore it, and BitkitCore's uniffi async has no cancellation handler on the decode path.
One thing I could not settle, stated as unverified rather than cleared: my local paykit-rs checkout is older than the pinned rc51, and pubky_session.rs delegates host handling to pubky::DeepLink::from_str, so while PubkyAuthRequestTests proves rc51 parses signin_grant, I couldn't confirm the Rust approval path accepts it — the E2E journey deliberately cancels before approval. Worth one manual approve on rc51 to close.
Cross-repo: the alias-gating concern raised on synonymdev/bitkit-android#1224 is structural here rather than a bug — iOS registers URL schemes statically in Info.plist and can't runtime-gate them the way Android's manifest aliases can.
I've corrected two statements in my own earlier review bodies on this PR that the trace below showed were wrong.
| @@ -0,0 +1,14 @@ | |||
| # Pubky auth | |||
|
|
|||
| This suite covers the uniquely targetable `bitkit://pubky-auth/setup` OS handoff into Bitkit. The wrapper carries the Paykit grant-auth requester fields and normalizes to `pubkyauth://signin_grant`; raw Pubky auth and signup requests remain supported through QR scanning and clipboard paste. | |||
There was a problem hiding this comment.
"raw Pubky auth and signup requests remain supported through QR scanning and clipboard paste" is incomplete — they're also reachable as OS links, and the same wording in the PR description ("Accepts only the uniquely targetable bitkit://pubky-auth/setup wrapper") reads as a security property the code doesn't implement.
Info.plist registers lightning/LIGHTNING, and handleScannedData strips that prefix before classification (AppViewModel.swift:492). I verified the transform directly rather than assuming: lightning:pubkyauth://direct_signup?hs=abc123 parses as scheme lightning with the absoluteString round-tripping byte-identical, and after removingLightningSchemes() becomes scheme pubkyauth, host direct_signup, query intact. Case-insensitive, so LIGHTNING: works too.
So both of these are OS-deliverable and skip requiresBitkitClaim (PubkyAuthRequest.swift:126, false whenever the scheme isn't bitkit):
lightning:pubkyauth://direct_signup?hs=<attacker-z32>-> signup consent sheet -> on approval,registerIdentity(…, homeserverZ32: <attacker>)lightning:pubkyauth://signin?relay=…&secret=…&caps=/pub/anything/:rw-> opens directly in.authorize->ordinaryApproval(authUrl, request.capabilities, …)with the URL's caps
This is not a hole you opened. The signin variant is on master; the signup variant is already reachable on the base branch (#724 head), which has the same onOpenURL -> handleScannedData -> removingLightningSchemes -> isProtocolURL -> signup-branch chain. This PR only changed delivery to retain-and-replay. And the consent boundary holds throughout: the attacker's homeserver z32 is rendered on the sheet, capabilities are itemised, an explicit Authorize tap is required, local auth applies whenever PIN or biometrics is enabled, and the whole surface is behind PaykitFeatureFlags.isUIEnabled (default false, Dev Settings only). That is why this is LOW and why it isn't the Android no-sheet bug.
What's wrong is only the description of the contract. Narrowest fix is two sentences: say here, and in the PR description, that the wrapper is the only link form receiving claim validation, and that lightning:/lnurl*:-prefixed raw pubkyauth:// requests are accepted from links exactly as the scanner and paste paths accept them.
Separately, and genuinely as a question rather than a request: should a raw pubky request arrive via a tapped link at all? If the team's threat model is "no pubky request without the claim contract from a link", the smallest change is one guard at the top of MainNavView.handleDeepLink — if url.scheme != "bitkit" and PubkyAuthRequest.isProtocolURL(url.absoluteString.removingLightningSchemes()), toast and return. No need to expose isBitkitSetupHandoff (the wrapper's scheme is bitkit, which the strip never touches) and no need to touch requiresLightningNode. That's a product call, not a defect.
There was a problem hiding this comment.
Corrected in c2de3f0. The journey README and PR description now state that Bitkit claim validation applies only to the bitkit://pubky-auth/setup wrapper, while lightning:/lnurl*:-prefixed raw Pubky auth and signup links follow the scanner and paste path. I left the separate policy question unchanged for a team product decision.
There was a problem hiding this comment.
Confirmed fixed in c2de3f056. The README now scopes claim validation to the wrapper and names the lightning:/lnurl*: link form explicitly, and PR description bullets 1 and 2 match. Nothing over-corrected — the QR/paste sentence is preserved rather than replaced.
Leaving the open question from my comment as a question: whether raw pubky requests should arrive via a tapped link at all is still a product call, and nothing here forecloses it.
Closes #715
This PR extends the #697 → #724 signup/auth stack with Bitkit-targeted Pubky marketplace setup-link delivery and explicit watch-only account authorization.
Description
bitkit://pubky-auth/setupwrapper and normalizes that wrapper to Paykit rc51'spubkyauth://signin_grantrequest while preserving its raw query.lightning:/lnurl*:-prefixed rawpubkyauth://auth and signup requests from OS links through the same parsing path used by scanner and clipboard paste.watch-only-account-v1claim with the exact server/private read-write capability set and rejects missing, duplicate, unsupported, or malformed authorization parameters before account material can be authorized.pubkyring://signinscheme only for Bitkit's outbound Ring handoff.Linked Issues/Tasks
Stack Note
#697 and #724 are currently seven
mastercommits behind, while #722's preserved history already contains those commits. Until the parent branches sync withmaster, GitHub's stacked diff also shows those inherited changes. The #722-specific behavior is the marketplace wrapper, lifecycle routing, watch-only validation, relay display, Ring handoff, and journey coverage described above.Preview
QA Notes
Manual Tests
bitkit://gift-…: Gift Loading opens and owns node failure presentation.These external-handoff scenarios remain open; the automated validation below does not mark them complete.
Automated Checks
PubkyAuthRequestTests,PubkyAuthURLSchemeTests,SceneDelegateTests,PubkyProfileManagerTests,PubkyAuthApprovalSheetTests,PendingProfileSetupResumeTests, andShopPaymentRequestTestsonbcc07259.bcc07259; the app launched and produced a semantic UI snapshot.git diff --check: passed onbcc07259.c2de3f05, per implementation-only direction.