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.
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.
7f26e99 to
a3ebbc7
Compare
jvsena42
left a comment
There was a problem hiding this comment.
No HIGH, no MEDIUM on this PR. One reply on the existing AppViewModel.swift:194 thread, and one real bug that turns out to belong on master rather than here.
Attribution first, because it changes what this PR is. gh pr diff is against codex/pubky-ring-signup, which lags master. 69 of the 99 files are byte-identical to origin/master at head — I verified per file with git diff --quiet origin/master 30cc049a -- <path> — including most of the files that look alarming in the diffstat: Keychain.swift, Errors.swift, PaykitPaymentProofService.swift, PaykitPaymentRequestService.swift, PrivatePaykitService+Payments.swift, FixedWidthInteger+Saturating.swift, ReceiveLiquidityDecision.swift, all 14 non-en locales and the six UI components. They arrived via 30cc049a merge: refresh master. The actually-authored surface is ~20 files: deeplink entry points, PubkyAuthRequest, AppViewModel routing, MainNavView, PubkyAuthApprovalSheet, SheetViewModel, a cancelPaymentRequest wrapper, ringHandoffURL, en strings, tests, journeys.
Where the gate sits relative to the URL entry point — the question that decides whether "gated" is a real defence here. At head Info.plist does not register pubkyauth or pubkyring inbound (greptile's summary refers to a superseded revision); inbound is bitkit, bitcoin, lightning, lnurl* (:10-18), and pubkyring is outbound-query only (:38-41). Any app or web page can open bitkit://… or lightning:…, but every entry point only stores the URL (AppScene.swift:157, :316, :345; SceneDelegate.swift:40, :51-54, :69-73; BitkitApp.swift:107-114). The sole consumer is MainNavView (:381-388), mounted only when walletExists, not initializing or restoring, and isPinVerified || !pinEnabled (AppScene.swift:448). Before the Paykit flag, attacker input reaches only Foundation URLComponents, removingLightningSchemes, SamRockSetupRequest.isProtocolURL, Bip21Utils.isDuplicatedBip21, and — for non-pubky URLs — the pre-existing BitkitCore decode(invoice:) scanner path. For pubky URLs the gate is PaykitFeatureFlags.isUIEnabled at AppViewModel.swift:594, which sits before PubkyAuthRequest.parse (the Rust parser) and before any sheet. So the Foundation-level parse is ungated but fails closed: isBitkitSetupHandoff (PubkyAuthRequest.swift:310-327) demands exact scheme/host/path with no userinfo, port or fragment and a non-empty query; unknown host or path → isProtocolURL false → falls through to decode → generic error toast.
Lock bypass — checked specifically, none found. All five entry points store only; backgrounding resets isPinVerified (AppScene.swift:772) → unmount → sheet torn down. Pubky and signup requests always land on a sheet with an explicit Authorize plus local auth when PIN or biometrics is on (PubkyAuthApprovalSheet.swift:401-415). Nothing pubky-related fires without a tap.
Other things I ruled out: the Ring callback can't be cancelled into an inconsistent state (activeAuthAttemptID is in-memory, so a cold-start callback is .ignored); TOCTOU on the sheet is closed (it reads only config.request, a let, and passes config.request.rawUrl); the wrapper parser passes raw query bytes through unre-serialised, rejects duplicate relay/secret, and does claim exact-match plus capability set-equality; rendered attacker text is confined to relayOrigin rebuilt as scheme://host[:port] and a lineLimit(1) clientID, with logs stripping query and userinfo (MainNavView.swift:830-848). bitkit:// is claimable by another app, but the only secret it carries is the marketplace relay secret, so a hijacker could only approve as itself; the outbound pubkyring://signin handoff is nonce-pinned. On the inherited files: Keychain.swift adds only paykitSubscriptionState and preserves kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly and non-synchronizable throughout — no folded live predicate of the #697 kind. Saturating arithmetic is correct at unsigned boundaries and every UInt64 call site saturates conservatively (fees → .max fail the budget check; budgets/balances → 0 reject); the signed helper is wrong in the negative direction but both signed call sites add non-negative operands. Localization: zero printf specifiers in en (all {placeholder} tokens) and placeholder-name sets match across all 14 locales for the 76 touched keys, so no format-specifier crash class.
One real bug, and it isn't yours. finishPayment can re-queue a just-paid one-time request from stale history: PaykitPaymentRequestService.swift:1256-1258 searches historyRequests for .accepted, but perform() :1503-1505 wrote that local .accepted entry itself and nothing updates it until the next completed refresh. The protected-id guard at :1261-1265 is backed purely by local proofs, which submit() already deleted at PaykitPaymentProofService.swift:621, so it passes and :1269 appends the paid request back into pendingRequests.
A second payment can't fire immediately — re-presentation goes through beginContactPayment with afterPrivatePaymentListVersion: consumedVersion, so until the payee publishes a strictly newer list it defers with .waitingForUpdatedPaymentList. But given payee republish plus a fresh invoice plus the user confirming a second Send sheet, it is a genuine second payment on a new invoice with no LDK duplicate-hash protection; the second proof then wedges locally because the SDK's require_state(&[Accepted, ActiveRecurring]) rejects it.
PaykitPaymentRequestService.swift and PaykitPaymentProofService.swift are both byte-identical to master, so this is inherited, not introduced here, and it's gated behind PaykitFeatureFlags.isUIEnabled. It shows in this diff only because the base lags master. Raising it here for the record — it wants a standalone issue against master, not a change in this PR. Narrowest fix when someone picks it up: gate the re-queue on the SDK record state rather than the locally mutated history entry — only append when service.synchronize().incoming still contains request.id, or when a local proof exists with neither proofData nor paymentStarted, which is the "closed the sheet without paying" case PaykitPaymentRequestServiceTests.swift:1553-1556 actually covers.
Coverage boundary, so you know what I didn't look at. Beyond the vs-master diff I did not examine: BlocktankViewModel, NavigationViewModel, WalletViewModel, TagManager, ContactDetailView, CreatePaymentRequestView, PaymentRequestsView, SubscriptionsView, the four Transfer views, the five Receive views, the four Send views, the six UI components and the subscription-clock asset, Docs/receive-liquidity.md, three changelog fragments, journeys/README.md, journeys/amount-limits/*, journeys/hardware-wallet/*, and all test files except test-name listings of PubkyAuthRequestTests, PubkyAuthURLSchemeTests and SceneDelegateTests. All are master-identical or unrelated to the deeplink, funds and key-material surfaces.
Figma: no figma.com URL in the PR body, in any comment, or in the diff. This PR touches several components and adds an illustration asset — non-blocking, but if there's a frame for them, link it and I'll diff the implementation against it next pass.
Parity with synonymdev/bitkit-android#1224: Android puts the pubkyauth and pubkyring://signup intent-filters on activity-aliases shipping android:enabled="false", toggled at runtime by PubkyAuthHandlerRegistrar, so the OS never delivers the link while gated — validation then happens on raw pubkyauth://. iOS registers bitkit statically (always delivered, cannot be runtime-gated), accepts only the bitkit://pubky-auth/setup wrapper with claim validation plus lightning:-prefixed raw forms, and gates at handling time. Net: Android gates at registration, iOS gates at handling but validates more strictly. Both terminate on a consent sheet.
|
Filed the I re-verified the whole chain at master One correction to what I wrote above, worth having in the issue rather than lost in a review body: the re-queue itself is intentional — it's what keeps a request visible when the user opens the Send sheet and closes it without paying, and |
|
@ovitrif conflics |
|
@ovitrif conflicts — No one had flagged it here yet. This one has been conflicting since at least this morning, so it's been skipped in my last two review passes rather than reviewed. Worth prioritising given what it carries: it's the deeplink-surface PR, and my last full review of it (at |
jvsena42
left a comment
There was a problem hiding this comment.
644dd9f8 is a clean isolation fix — no findings.
It drops the process-global URLProtocol.registerClass(SamRockURLProtocol.self) and its unregisterClass in tearDown, leaving the session-scoped configuration.protocolClasses routing at :297. The global registration was the leak: it affects URLSession.shared and default sessions process-wide, and with the static handler reset to nil in tearDown, any other test class hitting a default session afterwards would have been routed into SamRockURLProtocol with no handler.
The one way this could have weakened coverage is if a test relied on the global registration — code under test using URLSession.shared would then silently hit the real network. It doesn't: all four tests that set a handler build the service with SamRockService(urlSession: samRockURLSession()) (:245, :278, :285), and SamRockService.init(urlSession:) (SamRockService.swift:264) only falls back to .shared when nothing is injected. So every SamRock request still goes through the mock.
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 raw Pubky auth and signup requests delivered as OS links before authorization; raw requests remain available to scanner and clipboard-paste flows.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
#722 targets #724, which targets #697. Head
93dcfa42merges the current #724 branch (96fd5058) while preserving the approved #722 history. Until the parent branches merge, GitHub's stacked diff also shows their 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 prior 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.644dd9f8after isolating the SamRock URL protocol.93dcfa42: signed merge commit; current feat: support Pubky signup #724 head96fd5058is an ancestor; no unresolved conflicts;git diff --checkpassed.