feat: support Pubky signup - #724
Conversation
Greptile SummaryThis PR adds Pubky Ring signup URL parsing, wallet-derived identity registration, authorization approval, session activation, and resumable profile setup through the existing scanner and approval UI.
Confidence Score: 3/5The PR should not merge until Ring signup can recover from intermediate failures and users can leave pending profile setup without being immediately redirected back. The new flow can strand a remotely registered identity when approval or sign-in fails, and its navigation observer creates a repeatable Create Profile trap while setup remains pending. Files Needing Attention: Bitkit/Managers/PubkyProfileManager.swift, Bitkit/MainNavView.swift
|
| Filename | Overview |
|---|---|
| Bitkit/Managers/PubkyProfileManager.swift | Adds Ring signup and pending-profile state, but the signup sequence is not recoverable after an intermediate failure. |
| Bitkit/MainNavView.swift | Adds automatic profile-setup resumption, but route-driven reevaluation prevents users from leaving the setup screen. |
| Bitkit/Models/PubkyAuthRequest.swift | Adds strict parsing and validation for Ring signup parameters and reconstructs the corresponding authorization URL. |
| Bitkit/ViewModels/AppViewModel.swift | Routes Ring signup requests through the existing scanner while preserving restricted payment-flow state. |
| Bitkit/Views/Sheets/PubkyAuthApproval/PubkyAuthApprovalSheet.swift | Extends the approval sheet to execute signup and transition into profile setup, with partial-failure recovery delegated to the manager. |
| Bitkit/Services/PubkyService.swift | Adds low-level registration and Ring authorization operations used by the new signup sequence. |
Sequence Diagram
sequenceDiagram
participant R as Pubky Ring request
participant B as Bitkit scanner
participant H as Homeserver
participant A as Auth relay
participant P as Profile setup
R->>B: pubkyring://signup
B->>B: Parse and show approval
B->>H: Register wallet-derived identity
H-->>B: Registration complete
B->>A: Approve authorization
A-->>B: Authorization complete
B->>H: Sign in
H-->>B: Active session
B->>P: Navigate to Create Profile
Reviews (1): Last reviewed commit: "feat: support Pubky Ring signup" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
ben-kaufman
changed the title
|
Regtest device QA, home Scan, QR from staging.pubky.app. Staging e2e doesn’t finish — is that expected? No spinner after scan. Scanner stays up after already-signed-in / invalid auth. bitkit_logs_2026-09-03_09-45-08-ios.zip ScreenRecording_09-03-2026_11-38-24_ios-compressed.mp4 |
|
@piotr-iohk Thanks for the device QA and logs. You found a real signup interop bug: Bitkit treated every 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? |
91555ae to
436ee06
Compare
|
Retested the rebased head on a physical iPhone 13 using the regtest build and a signup QR from staging.pubky.app. Signup now completes end to end: Bitkit shows progress while processing the request, creates the Pubky identity, opens profile setup, and the staging website continues successfully. Ordinary sign-in still fails because staging currently generates the older non-grant authorization request. As clarified, that is outside the scope of this signup PR. The original signup interoperability and missing-progress issues are resolved for me. bitkit_logs_2026-09-03_14-36-23-ios.zip ScreenRecording_09-03-2026.16-31-36_1-ios.MP4 |
436ee06 to
4eedebb
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
692eccc to
0ae29a6
Compare
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
51100a3a closes it, and you bounded the leg rather than re-enabling dismissal — which was the right half to pick.
completeSignupAuthentication:428 now wraps the relay call in approveSignupWithTimeout(authorizationTimeout, operation: approveAuth) (default 30s, :420), built as an AsyncStream<Result<Void, Error>> with .bufferingOldest(1): operationTask yields the FFI outcome, timeoutTask yields URLError(.timedOut) at :453, :465 takes whichever lands first, and defer/onCancel cancel both and finish() the continuation (:459-461). A black-holed relay now surfaces as timedOut → the performAuthorization catch → toast → state = .authorize → sheet dismissable again. Every .authorizing leg terminates.
The AsyncStream shape is a better choice than the withThrowingTaskGroup I named — a task group would block on the un-cancellable FFI child before returning, so the timeout wouldn't actually release the sheet. Good catch.
The guard itself is untouched (canDismiss:63-65, showsBackButton:102-103, .interactiveDismissDisabled:114, onBack guard :466 all byte-identical to 8fce8ed6), so the double-approval race stays closed.
On a late success after the timeout: it's dropped cleanly — the yield lands on a finished continuation and activateIdentity is never called for the abandoned attempt. Nothing persists locally, since registerIdentity returns the bootstrap result without activateBootstrapResult, and isSignupInFlight is released by defer:424 so retry works. What can still happen is the relay authorization landing late, leaving the requesting app signed in while Bitkit reports a timeout; re-tapping Authorize on that Ring URL then fails on the consumed channel. That's inherent to racing an un-cancellable call — I asked for the race, so I'm recording it rather than filing it.
testSignupTimeoutAndCancellationIgnoreLateApprovalAndAllowRetry is a real guard: approveAuth parks on an unresumed continuation, so on revert both loop iterations hang and the fulfillment times out. It pins the error kind per branch (timedOut vs CancellationError, with .seconds(30) at :263 proving cancel isn't just the timeout), that nothing activates or persists after abandonment, that retry works, and that resuming the late approval afterwards neither activates nor clobbers publicKey.
One nit: the late FFI outcome is discarded at :445-447 with no log line, so a QA bundle won't show why a >30s relay eventually resolved. A Logger.warn in operationTask's catch would close that.
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA Notes
iPhone 17 Pro simulator, iOS 26.5 (QA_1) on host m1a; every Manual Tests item carried from 51100a3: the commits since change no behaviour (logging, comments, tests, or docs only).
-
✅ carried: Drove the REAL staging Pubky App as the companion, not a hand-built URL.
Test 1:
With no Pubky identity, scan a signup QR from staging Pubky App; approve it, confirm the website continues,…
-
✅ carried: Drove the direct signup format on the proven binary (nm on the installed Bitkit.debug.dylib: 92 approveSignupWithTimeout, 52 authorizationTimeout, 22 isSignupInFlight - this…
Test 2:
With no Pubky identity, scan either direct signup format; confirm the homeserver is visible, Cancel performs…
-
✅ carried: Precondition established in this round rather than assumed: item 2 left a real Pubky identity signed in (pubkyhmpz6kwq8adwdrsctzd1h8bczfbiktspzm41btk5jb8t15ke7cwy) and the home…
Test 3:
Scan a signup request while signed in and confirm the scanner closes with Already signed in.
-
✅ carried: RE-DRIVEN at this head.
Test 4:
Scan an invalid auth request and confirm the scanner closes with an error.
-
✅ carried: Drove the payment-only (send) scanner on the proven binary while a real Pubky identity was signed in (QA724B).
Test 5:
Confirm payment-only scanners reject Pubky requests without clearing the current payment flow.
-
✅ carried: SIGNED UP ON A NON-DEFAULT HOMESERVER: on a freshly erased device with a new wallet, scanned…
Test 6:
After signup on a non-default homeserver, make session recovery fail, restore connectivity, and retry…
Approve.
Coverage
Total: 33% (delta diff since 51100a3, 1 file)
- Journeys: 0% - This delta adds a log statement and changes no user journey, so none was exercised for it.
- Unit tests: 0% - A log line has no assertable behaviour and the delta adds no test, which is appropriate here.
- QA: 100% - 6 of 6 Manual Tests passed in the QA phase
Reviewed by Claude Code (claude-opus-5) via gh-pr-review-loop skill
There was a problem hiding this comment.
QA Notes
QA_1 iPhone 17 Pro simulator on iOS 26.5, built and installed from this head.
-
✅ passed: A live staging Pubky App signup QR authorized successfully, advanced the companion app, and completed matching profile setup in Bitkit.
Test 1:
With no Pubky identity, scan a signup QR from staging Pubky App; approve it, confirm the website continues,…1.mp4
-
✅ passed: Both direct-signup paths showed the requested homeserver; Cancel registered nothing, and Authorize required the enabled wallet PIN before profile setup.
Test 2:
With no Pubky identity, scan either direct signup format; confirm the homeserver is visible, Cancel performs…2.mp4
-
✅ passed: A signup request scanned while signed in closed the scanner with Already signed in and opened no approval or profile sheet.
Test 3:
Scan a signup request while signed in and confirm the scanner closes with Already signed in.3.mp4
-
✅ carried: The invalid-request scan still closes with Invalid auth request; none of its recorded implementation files changed in this delta.
Test 4:
Scan an invalid auth request and confirm the scanner closes with an error.
-
✅ carried: The payment scanner still rejects Pubky requests while preserving its current payment flow; none of its recorded implementation files changed in this delta.
Test 5:
Confirm payment-only scanners reject Pubky requests without clearing the current payment flow.
-
✅ passed: After app-scoped offline recovery and profile-save failures, restored connectivity recovered the same identity and homeserver, and repeated retries made no signup calls.
Test 6:
After signup on a non-default homeserver, make session recovery fail, restore connectivity, and retry…6.mp4
Approve.
Coverage
Total: 100% (delta diff since 0108e70, 1 file)
- Journeys: 100% - All six listed signup, rejection, and recovery journeys were exercised or validly carried.
- Unit tests: 100% - The existing signup timeout and cancellation test covers the task cancellation and retry control flow this guard preserves.
- QA: 100% - 6 of 6 Manual Tests passed in the QA phase
Reviewed by Codex (gpt-5.6-sol-high) via gh-pr-review-loop skill
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Two LOWs inline, both gated. No HIGH, no MEDIUM.
First, a correction to my own earlier review on this PR, which I've edited in place rather than leaving wrong. I claimed "no deeplink vector — a malicious web link cannot reach Bitkit." That's wrong. pubkyauth/pubkyring genuinely aren't registered, but lightning and lnurl* are (Info.plist:5-21), and handleScannedData strips those prefixes at AppViewModel.swift:480 before deciding the input is a Pubky request at :536. So a wrapped link does reach the approval sheet. Details in the first inline comment. The practical impact is contained by the flag, but the "iOS has no web-reachable route" framing I built on it doesn't hold, and I've cross-fed the correction to synonymdev/bitkit-android#1224 where I'd used it as a contrast.
What I checked and found sound:
- Fund draining — not touched.
ShopPaymentRequest.swift:27is a new enum case only.AppViewModel.swift:481-484releases the claimed contact context and throws before:513 resetSendState;:486-489rejects Pubky URLs in.paymentRequestsscope before decode;AppScene.swift:891-894catches it, marks presented, and continues — nobeginPaymentRequestresult is acted on and no sat-moving path is touched. - Key material.
deriveKeys()on a detached task;secretKeyHexflows only toregisterIdentity/approveRingAuth/activateBootstrapResult. Persisted solely byPubkyService.swift:1059-1075 persistSessionAccessviaKeychain.upsertwithkSecAttrAccessibleAfterFirstUnlockThisDeviceOnly(Keychain.swift:38/:94) in a network-scoped group (Env.swift:123-127, matching the four entitlement groups) — worth calling out given the cross-configuration keychain problem that bit #697.registerIdentity(:419-433) deliberately does not activate, so nothing hits the keychain before relay approval. - Authorization. All three formats resolve
isSignup→showSheet(.pubkyAuthApproval)(:891-910);performAuthorizationis reachable only fromonAuthorize()→resolvePubkyApprovalLocalAuthMode(PubkyAuthApprovalSheet.swift:377-391);config.requestis a value pinned at presentation,homeserveris extracted before anyawait(PubkyProfileManager.swift:385), and the identity-exists guard is re-checked afterderiveKeys()(:388,:393).clientIDforced"",bitkitClaimrejected on signup (PubkyAuthRequest.swift:154). - Trust boundaries.
hsis z32-validated (:140) before display; relay/secret/caps are re-serialized with strict unreserved encoding (:196-199) and re-validated byBitkitCore.parsePubkyAuthUrl(:138); duplicates rejected (:216);stis opaque and only sent to the homeserver the same QR named. No attacker string is used as a path, file name, or outbound URL beyond the protocol-inherent relay POST. - Cancellation / concurrency.
isSignupInFlightis a synchronous check-and-set on@MainActor(:422-424);bufferingOldest(1)makes first-result-wins deterministic (:440);onCancelanddeferboth finish the continuation; no suspension point betweenactivateIdentityreturning and the four state writes (:429-435). - Cross-identity. A Ring long-poll can't land on top of a completed signup —
PubkyRingAuthView.swift:124 .task(id: isWaitingForRing)cancelscompleteAuthenticationwhen the screen is left, and theCancellationErrorbranch discards any completed session (PubkyProfileManager.swift:711-721). Keychain buckets are network-scoped.
Cross-feed from synonymdev/bitkit-android#1224, worth a look here: 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; .interactiveDismissDisabled(!state.canDismiss) blocks user dismissal while .authorizing, but a programmatic sheet replacement (an incoming payment-request sheet, say) would cancel the view task between approveAuth and activateIdentity. Whether activation still persists then depends on cooperative cancellation inside the PaykitSdkService actor — I did not trace the actor, so I'm flagging it as unverified rather than asserting a bug. Conversely, iOS is the safer side on the relay hang: your 30 s approveSignupWithTimeout race is exactly the shape Android's open thread needs.
| return | ||
| } | ||
|
|
||
| if PubkyAuthRequest.isProtocolURL(uri) { |
There was a problem hiding this comment.
Low, gated — but it invalidates the "no deeplink vector" claim I made earlier on this PR, so recording it properly.
A lightning:-wrapped Pubky URL opened from a web page reaches the signup approval sheet without any scan.
Trace: Safari (or any app) opens lightning:pubkyauth://signup?hs=<z32>&st=x. iOS delivers it because the outer scheme is registered (Info.plist:5-21 lists lightning, LIGHTNING, lnurl, lnurlw, lnurlp, lnurlc). Then MainNavView.swift:366 .onOpenURL → not http(s), not a PubkyRingAuthCallback → :408 handleScannedData(url.absoluteString) at the default .unrestricted scope → AppViewModel.swift:480 removingLightningSchemes() strips the prefix (String+Utilities.swift:4-20, anchored and case-insensitive) leaving pubkyauth://signup?hs=… → :536 isProtocolURL true → :537 scope check passes → :540 flag check → :549 handlePubkyAuthApproval → :891 isSignup → :893 hasStoredIdentity() false → :905 showSheet(.pubkyAuthApproval).
Why it's LOW and not higher: the gate at :540 sits before handlePubkyAuthApproval, and from the sheet onward it's the same consent + PIN/biometric gate as the QR path, with the attacker-named homeserver displayed. Reaching it needs Dev Settings unlocked (hidden tap in SupportScreen.swift:174) plus paykitUiEnabled. The outcome is identical to the QR/clipboard case — the delta is purely delivery: a web link instead of a deliberate scan.
Also worth being precise about what this PR did and didn't introduce: at the base branch the same wrapper already reached the auth-approval sheet via bitkit-core's .pubkyAuth decode. This PR widens it to identity creation for users with no identity, and adds a second acceptance point at :536 that bypasses the bitkit-core decode and additionally accepts pubkyring://.
Minimal fix — decide "is this a Pubky request" on the unstripped input. A Pubky request never legitimately arrives wrapped: Pubky App and Ring emit bare pubkyauth:///pubkyring://, and the scanner and clipboard pass the raw string. Right after :480:
if PubkyAuthRequest.isProtocolURL(uri), !PubkyAuthRequest.isProtocolURL(<raw input>) {
throw ScanHandlingError.pubkyAuthRequest
}Throwing before decode(invoice:) matters: if you only guard the :536 branch, the wrapped string falls through to :557 decode and re-enters via :730 case let .pubkyAuth → :740 handlePubkyAuthApproval. Please don't fix this by adding scheme allowlists in onOpenURL or by touching removingLightningSchemes — that's the bitkit-core#70 workaround and the payment paths depend on it.
There was a problem hiding this comment.
Wrapped Pubky requests are now rejected before decoding or clearing payment state. Bare Pubky URLs and Lightning/LNURL payment normalization still work as before.
There was a problem hiding this comment.
The guard closes both forms I reported, but it's narrower than what it's defending against — two wrappers still reach the approval sheet.
The new check compares the app stripper's output against the raw input:
483 let rawUri = uri
484 let uri = uri.removingLightningSchemes()
489 if PubkyAuthRequest.isProtocolURL(uri), !PubkyAuthRequest.isProtocolURL(rawUri) {
490 throw ScanHandlingError.pubkyAuthRequestBut decode is bitkit-core, which does its own unwrapping. At the pinned revision (Package.resolved → 890502f2, v0.5.14), src/modules/scanner/implementation.rs:
58 let invoice_str = invoice_str.trim();
59 let invoice_str = invoice_str.strip_prefix("lightning:").unwrap_or(invoice_str);
64 if invoice_str.starts_with("bitkit://") {
65 let data = invoice_str.replace("bitkit://", "");
82 return Box::pin(Self::decode(data)).await;
128 } else if invoice_str.to_lowercase().starts_with("pubkyauth:") {
129 Ok(Scanner::PubkyAuth { data: invoice_str.to_string() })So anything the Rust unwraps but removingLightningSchemes doesn't slips past :489 with isProtocolURL false on both sides, then re-enters at :737 case let .pubkyAuth → :747 handlePubkyAuthApproval.
1. lnurl:lightning:pubkyauth://signup?hs=X (also lnurlp:/lnurlw:/lnurlc:lightning:, and lightning:lightning:).
removingLightningSchemes is a single ordered pass and "lightning:" is tested first:
4 "lightning:", "lnurl:", "lnurlw:", "lnurlc:", "lnurlp:"
15 for prefix in Self.lightningSchemePrefixes {
16 if let range = value.range(of: prefix, options: [.anchored, .caseInsensitive]) {
17 value.removeSubrange(range)For lnurl:lightning:… the lightning: test misses, lnurl: strips, and the loop never revisits lightning: — so the result is still lightning:pubkyauth://… and isProtocolURL is false. Rust then strips lightning: at :59 and returns .pubkyAuth.
The new test's prefix list at ShopPaymentRequestTests.swift:69 has lightning:lnurl: but not the reverse order, so it doesn't catch this.
2. bitkit://pubkyauth://signup?hs=X — URL(string:) accepts it, the stripper doesn't touch bitkit:, guard is false/false, and Rust's replace + recursion at :65/:82 lands on the pubkyauth: branch.
Both are reachable in a Release build from any web page, since bitkit, lightning and lnurl* are all registered in Info.plist:10-18. Same severity class as the original: the flag check at :738 is after receipt, stripping, resetSendState and decode, so with Paykit off it's a generic error toast, and with it on the sheet still needs explicit PIN/biometric approval. LOW, gated.
Suggested refinement — keep :489, and add a check on the decode output rather than trying to out-guess the stripper. At :737 case let .pubkyAuth, require PubkyAuthRequest.isProtocolURL(rawUri) and throw pubkyAuthRequest otherwise. That closes every wrapper regardless of which layer unwrapped it, and it needs no change to removingLightningSchemes or onOpenURL — both of which I still think should stay as they are.
Confirmed clean while I was in here: removingLightningSchemes is untouched, onOpenURL has no allowlist, and the redaction half of the fix landed properly — PubkyAuthApprovalSheet.swift:449-453 logs a fixed string on the signup branch, and I walked approveSignupAuth, completeSignupAuthentication, approveSignupWithTimeout and PubkyService.registerIdentity/activateRegisteredIdentity for other frames that might re-interpolate. None do.
There was a problem hiding this comment.
Added the raw-input check to the decoded Pubky auth branch, including nested Lightning/LNURL and bitkit:// wrappers. These decoder-only cases are rejected after decoding and the existing send-state reset, so my earlier claim that all wrappers were rejected before reset was too broad. Payment URL normalization is unchanged.
| continuation.yield(.success(())) | ||
| } catch { | ||
| if !(error is CancellationError) { | ||
| Logger.warn("Pubky signup relay approval failed", context: "PubkyProfileManager") |
There was a problem hiding this comment.
Low, gated — a refinement of the redaction decision on thread 3974028428, not a reversal of it.
Keeping this line a fixed string was the right call, but the redaction doesn't actually hold, because every error approveSignupAuth throws gets interpolated two frames up: PubkyAuthApprovalSheet.swift:449 is Logger.error("Failed to approve pubky auth: \(error)", context: "PubkyAuthApprovalSheet").
Path: approveAuth throws → :450 continuation.yield(.failure(error)) → :468 result.get() rethrows the same Error → approveSignupAuth → performAuthorization catch → :449 \(error). So the relay FFI error text you deliberately kept out of this line lands in the log anyway, one layer up, and a QA log bundle gets it.
To be clear on the stakes: this isn't key material. The relay URL's channel segment is a hash of the client secret and the token is encrypted — it's a consistency gap, not a leak.
Minimal fix: in the :438 catch, when config.request.isSignup, log a fixed string (or type(of: error)) instead of \(error). Leave the :450 toast alone — that's on-device only.
There was a problem hiding this comment.
Signup failures now use a fixed log message in the approval sheet too. The on-device error toast is unchanged.
|
@jvsena42 On sheet replacement, both the authorization button and PIN callback start unstructured |
The base branch was changed.
|
Needs conflict resolution |
|
Merged |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Both fixes reviewed at 16f31299. Redaction landed cleanly; the wrapped-scheme guard closes the two forms I reported but is narrower than bitkit-core's own unwrapping — refinement on the existing thread, still LOW and gated.
Attribution, because the diffstat is misleading. 143 files differ from e509cb34, but 133 are byte-identical to origin/master (the 5ea3cf3d merge bringing in #697/#685/#686/#711/#735 and a swiftformat pass). The actual new authorship since my last review is git diff e509cb34 13dbd5d6 --stat = 3 files, +37/−1. That was the whole review surface.
Fix (b) landed. PubkyAuthApprovalSheet.swift:449-453 logs the fixed string on the signup branch; the interpolating line survives only for non-signup. I walked the rest of the Ring-signup path for other frames that might re-interpolate — approveSignupAuth, completeSignupAuthentication, approveSignupWithTimeout, PubkyService.registerIdentity/activateRegisteredIdentity — none do. The :307 interpolation is on the legacy createIdentity path and predates the PR. Note the toast at :454 still shows error.localizedDescription on screen; that wasn't part of the finding (UI, not logs), flagging it only so it's a decision rather than an oversight.
Fix (a) is partial — details on the thread. Short version: the guard compares the app stripper's output to the raw input, but decode is bitkit-core, which unwraps lightning: and bitkit:// itself. I verified this at the exact pinned revision (Package.resolved → 890502f2 = v0.5.14) rather than from memory. Two wrappers still reach handlePubkyAuthApproval: lnurl:lightning:pubkyauth://… (because removingLightningSchemes is a single ordered pass that tests lightning: first and never revisits it) and bitkit://pubkyauth://…. Suggested refinement is to check the decode output at :737 instead of trying to out-guess the stripper — no change to removingLightningSchemes or onOpenURL, which I still think should stay as they are.
The #697 regression check came back clean, which was the thing I most wanted to rule out now that #697 has landed on master:
Env.keychainGroup(Env.swift:123-127) is still network-suffixed;git diff e509cb34 head -- Env.swiftis empty.cf0b8c03did remove the"Pubky grant client ID"prefix check fromshouldDeferStaleSession— but that predicate was already identical ate509cb34, so nothing changed on this branch.- More importantly the precedent scenario can't fire here:
clientID(:1207-1212) keys onEnv.network, andEnv.network(:131-142) is regtest for Debug and bitcoin for Release. Different network means a different keychain group and a different client ID; same network means an identical client ID. So there's no same-entry mismatch producible across a Debug-over-TestFlight install at this head — the thing that bit #697 is structurally absent here.
Merge fidelity: faithful. git show --cc 16f31299 shows one conflict hunk (AppViewModel.swift ~:949), and comparing against git merge-tree --write-tree 13dbd5d6 5ea3cf3d the head differs only there, where the resolution is the union of both sides (releaseContactPaymentContext from the PR, hasSendPaymentTarget from master). No line differs from both parents anywhere else.
Everything I cleared at e509cb34 — fund paths, key material, authorization/TOCTOU, trust boundaries, lifecycle, cancellation, cross-identity — is untouched by both the fix and the merge.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Fix confirmed at 38c01421 — both wrappers are closed, and you took the decode-output approach rather than trying to out-guess the stripper.
AppViewModel.swift:738-743 guards case let .pubkyAuth on PubkyAuthRequest.isProtocolURL(rawUri) and throws otherwise, which catches anything bitkit-core unwrapped regardless of which layer did it. Traced the four cases:
- bare
pubkyauth://signup?…→isProtocolURL(rawUri)true → passes. Legit path intact. lightning:pubkyauth://…→ still caught earlier at:489.lnurl:lightning:pubkyauth://…→:489skipped as before (the stripper's single ordered pass leaveslightning:in place), decode yields.pubkyAuth, new guard sees a raw URI with schemelnurl→ throws.bitkit://pubkyauth://…→:489skipped, Rust'sreplace+ recursion yields.pubkyAuth, new guard throws.
Good catch releasing the contact payment context in the guard — that matches what :485-488 does and avoids leaking a claimed context on the throw path.
I checked it doesn't over-correct, which was the main risk with a guard this broad. The legitimate bitkit://pubky-auth/... Ring callback never reaches here: MainNavView.swift:386 intercepts it via PubkyRingAuthCallback.parse and returns before handleScannedData, and that parser requires scheme == "bitkit" && host == "pubky-auth" (PubkyProfileManager.swift:43), which isProtocolURL would reject. Different entry point, unaffected.
The test list covers more orderings than I named — lnurl:lightning:, lnurlp:lightning:, lnurlw:lightning:, lnurlc:lightning:, lightning:lightning:, bitkit://, and bitkit://bitkit://lightning: — plus a case asserting the contact payment claim is released on rejection.
That closes everything I had open on this PR. Clean from my side.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Re-checked at 96fd5058. Master merge only, no new authored commit — the merge brought in #720, #732 and #734.
Both halves of the wrapped-scheme fix survived intact, which is what I wanted to confirm on a merge this size:
AppViewModel.swift:483let rawUri = uriand:489the raw-vs-stripped guard:738the decode-output guard oncase let .pubkyAuth, with the contact-context release beneath it
git show --cc 96fd5058 produces no combined-diff lines, so nothing was resolved by hand and there's no evil-merge surface to explain.
Nothing to re-review; my confirmation stands.
There was a problem hiding this comment.
Reaudit e509cb3..96fd505: a merge of master into the branch. Comparing the PR's net patch before and after the merge, 3 of its 17 files differ and the other 14 are byte-identical, so the conflict resolution is the whole of the reviewable change. AppViewModel.swift keeps master's hasSendPaymentTarget alongside this branch's resetSendState(preservingContactPaymentContext:), and adds a raw-input guard on the decoded .pubkyAuth branch so a bitkit://-wrapped request is rejected: removingLightningSchemes() does not strip that prefix, so the earlier guard cannot fire on it. PubkyAuthApprovalSheet.swift keeps the authorizing-state dismissal lock together with the signup-specific error logging that omits the raw error. ShopPaymentRequestTests.swift keeps both the payment-scope and the wrapped/decoded Pubky rejection suites. The Debug simulator build succeeds. No findings.
QA Notes
- ⏭️ skipped: tested on e509cb3, recording
- ⏭️ skipped: tested on e509cb3, recording
- ⏭️ skipped: tested on e509cb3, recording
- ⏭️ skipped: tested on 51100a3, screenshot
- ⏭️ skipped: tested on 51100a3, screenshot
- ⏭️ skipped: tested on e509cb3, recording
Approve.
Replies
ben-kaufman: @jvsena42 On sheet replacement, both the authorization button and PIN callback start unstructured
Taskinstances… (comment)
That reading matches the code at this head, and it does not need a change here. performAuthorization re-checks sheets.pubkyAuthApprovalSheetItem?.request.rawUrl == config.request.rawUrl before it calls sheets.hideSheet(), so an in-flight leg belonging to a replaced sheet cannot dismiss or activate over a newer request, and approveSignupWithTimeout bounds that leg at 30 seconds. The uncancelled Task therefore runs to completion against a stale request and its result is discarded rather than applied.
ben-kaufman: Merged
masterand resolved the conflict, preserving the signup and payment changes. (comment)
Confirmed. Comparing the PR's net patch before and after the merge, 3 of the 17 files differ and the other 14 are byte-identical. AppViewModel.swift keeps master's hasSendPaymentTarget alongside this branch's resetSendState(preservingContactPaymentContext:), PubkyAuthApprovalSheet.swift keeps the authorizing-state dismissal lock alongside the signup-specific error logging, and ShopPaymentRequestTests.swift keeps both the payment-scope and the wrapped Pubky rejection suites. The Debug simulator build succeeds.
Coverage
Total: 67% (delta diff since e509cb3, 3 files)
- Journeys: 0% - The delta is a merge-conflict resolution with no user-visible behaviour change, so no journey was added or exercised.
- Unit tests: 100% -
ShopPaymentRequestTests.swiftcovers both merged halves: payment-scope rejection and wrapped/decoded Pubky rejection including the contact-claim release. - QA: 100% - 6 of 6 Manual Tests passed in the QA phase
Reviewed by Claude Code (claude-opus-5-high) via gh-pr-review-loop skill
feat: open pubky auth links
4 participants
Description
pubkyring://signupand auth-bearingpubkyauth://signuprequests, plus directpubkyauth://direct_signupand parameter-only legacypubkyauth://signup, through the normal scanner and deep-link flow.This PR is stacked on #697 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.
Linked Issues/Tasks
Depends on #697.
Screenshot / Video
Not included; this reuses the existing scanner, authorization approval sheet, loading treatment, and profile setup UI.
QA Notes
Validation:
PubkyProfileManagerTests.swiftcovers stored-key sign-in, credential-read/sign-in/profile failures and retries, cancellation-error propagation, and unchanged no-key signup/cleanup. Existing consent, activation and scanner regressions also pass.