Skip to content

feat: open pubky auth links - #722

Merged
ovitrif merged 47 commits into
codex/pubky-ring-signupfrom
codex/715-pubkyauth-links
Sep 11, 2026
Merged

feat: open pubky auth links#722
ovitrif merged 47 commits into
codex/pubky-ring-signupfrom
codex/715-pubkyauth-links

Conversation

@ovitrif

@ovitrif ovitrif commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

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

  • Applies Bitkit claim validation only to the uniquely targetable bitkit://pubky-auth/setup wrapper and normalizes that wrapper to Paykit rc51's pubkyauth://signin_grant request while preserving its raw query.
  • Rejects 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.
  • Retains setup requests, Ring callbacks, BIP21, BOLT11, and gift links through startup, restoration, PIN, scene, and app lifecycle gates until their destination UI can handle them.
  • Preserves feat: support Pubky signup #724's signup parsing, identity checks, scanner dismissal, loading state, and profile-setup continuation as the source of truth.
  • Requires wrapper requests to carry the supported watch-only-account-v1 claim with the exact server/private read-write capability set and rejects missing, duplicate, unsupported, or malformed authorization parameters before account material can be authorized.
  • Displays the parsed authorization relay origin on watch-only consent and authorization without representing it as a verified service identity.
  • Uses the separate pubkyring://signin scheme only for Bitkit's outbound Ring handoff.

Linked Issues/Tasks

Stack Note

#722 targets #724, which targets #697. Head 93dcfa42 merges 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

  • 1. Terminated or PIN-locked wallet with Paykit UI and a Bitkit-generated Pubky identity → open a valid setup link → finish startup/unlock: watch-only consent shows the authorization relay origin.
  • 2. Watch-only consent → Cancel: returns without authorizing or exporting account material.
  • 3. Offline wallet → open 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

  • Previous focused clean-simulator tests: 135 passed across PubkyAuthRequestTests, PubkyAuthURLSchemeTests, SceneDelegateTests, PubkyProfileManagerTests, PubkyAuthApprovalSheetTests, PendingProfileSetupResumeTests, and ShopPaymentRequestTests on bcc07259.
  • Previous E2E Debug simulator build-and-run: passed on bcc07259; the app launched and produced a semantic UI snapshot.
  • Previous SwiftFormat, translation validation, and git diff --check: passed on bcc07259.
  • Previous unit-test workflow: passed on 644dd9f8 after isolating the SamRock URL protocol.
  • Current head 93dcfa42: signed merge commit; current feat: support Pubky signup #724 head 96fd5058 is an ancestor; no unresolved conflicts; git diff --check passed.
  • Tests, builds, formatters, translation validation, simulator/device QA, CI reruns, and CI watching were not run for this conflict refresh.

@greptile-apps

This comment has been minimized.

Comment thread Bitkit/Info.plist Outdated
ovitrif

This comment was marked as off-topic.

Comment thread Bitkit/AppScene.swift Outdated
Comment thread Bitkit/Models/PubkyAuthRequest.swift
@ovitrif
ovitrif force-pushed the codex/715-pubkyauth-links branch from d8110c0 to 48b2191 Compare September 2, 2026 21:43
@ovitrif
ovitrif requested a review from ben-kaufman September 2, 2026 21:45
Comment thread Bitkit/ViewModels/AppViewModel.swift Outdated
@ovitrif ovitrif changed the title feat: open Pubky auth links feat: open pubky auth links Sep 3, 2026
@ovitrif
ovitrif force-pushed the codex/715-pubkyauth-links branch from 48b2191 to c108dee Compare September 3, 2026 21:51
@ovitrif
ovitrif requested a review from ben-kaufman September 3, 2026 21:52
Comment thread Bitkit/ViewModels/AppViewModel.swift Outdated
@ovitrif
ovitrif requested a review from ben-kaufman September 3, 2026 22:45
Comment thread Bitkit/ViewModels/AppViewModel.swift Outdated

@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.

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.

Comment thread Bitkit/ViewModels/AppViewModel.swift Outdated
@ovitrif
ovitrif changed the base branch from master to codex/pubky-ring-signup September 8, 2026 17:05

@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.

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/mastergit 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 immutable let) and passes config.request / config.request.rawUrl to performAuthorization. A second link arriving mid-approval goes through showSheet, which calls hideSheet() first — the .sheet(item:) tears the view and its @State down, dismissing the AuthCheck cover, 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 on rawUrl equality. 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 when walletExists, not initializing/restoring, and isPinVerified || !pinEnabled; backgrounding resets isPinVerified, which unmounts it and the sheet. routePendingDeepLinkIfReady clears the slot before its first await, so the .task(id:) + .onChange double-trigger can't double-handle.
  • Wrapper trust boundary. isBitkitSetupHandoff requires exact scheme/host/path, no userinfo/port/fragment, non-empty query not starting with ?. normalizedProtocolURL hardcodes host signin_grant and passes raw query bytes through. parse rejects duplicate relay/secret, then forces the claim check — exactly one x-bitkit-claim=watch-only-account-v1 and caps set-equal to the fixed constant. parseSignup throws if a wrapper carried a claim, and a wrapper can't be a signup URL anyway. Companion approval passes the hardcoded watchOnlyAccountCapabilities, never request.capabilities.
  • Rendered attacker text. relayOrigin is rebuilt as scheme://host[:port] from URLComponents — no path, query or userinfo — so it can't contain < to break the <accent> markup. sanitizedDeeplinkDescription strips query and userinfo, and for opaque lightning: URLs logs only the scheme, so secret never 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 exposure pubkyauth:// had before this PR. Ring callbacks are nonce-pinned.
  • Lifecycle. The .task(id:) restarting when LDK reaches .running can't lose a pubky link: handleScannedData reaches showSheet with no await before 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.

Comment thread journeys/pubky-auth/README.md Outdated
@ovitrif
ovitrif requested a review from jvsena42 September 8, 2026 19:07
@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch 2 times, most recently from 7f26e99 to a3ebbc7 Compare September 9, 2026 09:33

@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.

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.

@jvsena42

Copy link
Copy Markdown
Member

Filed the finishPayment re-queue as #737 against master, so it doesn't ride on this PR.

I re-verified the whole chain at master 5ea3cf3d before filing — same line numbers, and git diff --quiet origin/master 30cc049a confirms both PaykitPaymentRequestService.swift and PaykitPaymentProofService.swift are byte-identical, so nothing here is yours to fix.

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 PaykitPaymentRequestServiceTests.swift:1554-1556 pins exactly that. The defect is narrower than "it re-queues": it can't distinguish that case from "paid, proof already deleted", because both guard sources read local proofs that submit() removed at PaykitPaymentProofService.swift:621.

@jvsena42

Copy link
Copy Markdown
Member

@ovitrif conflics

@jvsena42

Copy link
Copy Markdown
Member

@ovitrif conflicts — mergeable=CONFLICTING at head e6500863.

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 30cc049a) is now two heads stale.

@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.

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.

@ovitrif
ovitrif merged commit 902ff39 into codex/pubky-ring-signup Sep 11, 2026
9 checks passed
@ovitrif
ovitrif deleted the codex/715-pubkyauth-links branch September 11, 2026 16:44
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.

feat: open Pubky auth links from iOS

3 participants