Conversation
Wires DashSync's `openInAppBrowserWithURL:` payment-processor delegate into the iOS app: after a successful send for a BIP21 URI that carried `callback=<https-url>`, the wallet opens the URL in an `SFSafariViewController` so merchants can complete their checkout flow. The Safari sheet is presented from the external presentation context (not the popped `ProvideAmountViewController`) with `.overFullScreen` modal style. This keeps the underlying view in the hierarchy on dismiss and avoids revealing a detached controller that would otherwise crash inside its `viewWillAppear`. Mirrors the pattern already used by `TxDetailViewController` for the block-explorer button. Also adds a defensive guard around `ProvideAmountViewController`'s `navigationController!` force-unwrap so this category of dismiss-after-pop crash cannot recur from any other code path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 37 minutes and 27 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@sag333ar |
bfoss765
changed the title
bfoss765
changed the title
|
⛔ Final review complete — 4 blocking finding(s) (commit c4a5b0d) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Sol-only technical fallback
At exact head c4a5b0d, the callback feature is not merge-ready: it references a DashSync API absent from both the recorded dependency commit and current DashSync master, loses callback metadata on scanned BIP70 requests, and races the payment-flow transitions. The callback is also neither transaction-bound nor authenticated as belonging to the confirmed payee, allowing the wrong or an attacker-controlled destination to open after payment.
Source: reviewer 1: gpt-5.6-sol (agent: sol-fallback-reviewer, role: general); reviewer 2: gpt-5.6-sol (agent: sol-fallback-reviewer, role: security-auditor); reviewer 3: gpt-5.6-sol (agent: sol-fallback-reviewer, role: ffi-engineer); final verifier: gpt-5.6-sol (agent: sol-verifier, role: final-verifier)
One or more required Phase-1 GLM Flash lanes remained technically unusable after the bounded exact-model retry. Their evidence was discarded as authoritative, and the complete selected role cohort was rerun fresh on exact gpt-5.6-sol before this fresh Sol verifier produced the final decision. No additional Phase-2 reviewer pass ran.
Review provenance
- Phase 1 GLM evidence: technically unusable after bounded retry; discarded from the decision
- GLM failure attempts:
codex-ffi-engineer-b91b409f64454a609c030cef1de031c4(failed),codex-ffi-engineer-1610beae09504ee58a188fc74a5caaab(failed),codex-general-87b0672bd2a34a60b821cb4a2a329100(failed),codex-general-cfb173673cac4d97901fda698d55ae7a(failed),codex-security-auditor-3a99d8a4ecad4a35a3247c9b8dd6ff9b(failed),codex-security-auditor-9b3869c916e44f98a3803b34f8341269(failed) - Sol-only fallback reasons:
launch_transport_or_nonzero_exit,launch_transport_or_nonzero_exit,launch_transport_or_nonzero_exit - Sol-only fallback reviewers:
gpt-5.6-sol— general (completed); agentsol-fallback-reviewer,gpt-5.6-sol— security-auditor (completed); agentsol-fallback-reviewer,gpt-5.6-sol— ffi-engineer (completed); agentsol-fallback-reviewer - Fresh verifier (Sol):
gpt-5.6-sol— final-verifier; agentsol-verifier - Additional Phase 2 pass: not run; the Sol-only fallback is final
🔴 4 blocking
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `DashWallet/Sources/UI/Payments/PaymentModels/DWPaymentProcessor.m`:
- [BLOCKING] DashWallet/Sources/UI/Payments/PaymentModels/DWPaymentProcessor.m:706: The required DashSync callback API is not available to a clean build
This line accesses `DSPaymentRequest.callback`, but the repository records DashSync commit `13fc5bea98a6c0f1b80c4e2c146ff5bf536baf76`, whose `DSPaymentRequest.h` declares `callbackScheme` but no `callback` property. Current DashSync master (`d3c1497a7b4323a9f21ce3f8f0c5c4e262cab4ca`) also lacks it; the property exists only on the still-separate DashSync PR #608 head (`aca6889b43961db91ffc8bcc603ebeba5b3f935e`). Because the Podfile uses an unpinned `../DashSync/` checkout and this PR updates neither that dependency nor `DashSyncCurrentCommit`, the exact app head cannot compile against its recorded or normal upstream dependency. Land and record the corresponding DashSync change, or otherwise make the dependency reproducible, before merging this app change.
- [BLOCKING] DashWallet/Sources/UI/Payments/PaymentModels/DWPaymentProcessor.m:705-708: Callback metadata is neither preserved nor bound to the completed payment
Reading the callback from mutable `self.paymentInput.request` creates two concrete failures. First, `DWQRScanModel.m` eagerly fetches a scanned URI's `r=` request and, on success, places only the resulting `protocolRequest` in the new `DWPaymentInput`; the original `DSPaymentRequest` carrying `callback` is discarded, so this expression is nil and the advertised scanned BIP70 flow never opens its callback. Second, `PaymentController.performPayment(with:)` resets and reuses the processor, including the Home controller used by incoming payment URLs. If payment A still has a publication or BIP70 relay completion pending when payment B replaces `paymentInput`, A's completion can read and launch B's callback even though B was not the completed transaction. Preserve callback metadata when converting scanned requests and snapshot it with the protocol request/transaction when starting each send; completion handlers must use that immutable snapshot rather than current processor state.
- [BLOCKING] DashWallet/Sources/UI/Payments/PaymentModels/DWPaymentProcessor.m:705-708: Untrusted callback is opened as a trusted post-payment continuation
The callback comes from an untrusted QR code or app URL, yet it is opened automatically after payment without displaying or approving its destination and without proving that it belongs to the confirmed payee. DashSync PR #608 only checks that the parsed URL's scheme is `https`; TLS authenticates the callback host, not its relationship to the payment. An attacker can therefore wrap a legitimate, PKI-authenticated BIP70 `r=` request in an outer BIP21 URI with an attacker-controlled callback: the confirmation authenticates the real merchant, but the post-payment browser opens the attacker's site in that merchant checkout context. Bind the callback to authenticated payment metadata, or show its host and require explicit approval before navigation.
In `DashWallet/Sources/UI/Payment Controller/PaymentController.swift`:
- [BLOCKING] DashWallet/Sources/UI/Payment Controller/PaymentController.swift:249-264: The fixed delay races the payment screen transitions
The 0.4-second timer begins immediately after `didSend` starts dismissing `ConfirmPaymentViewController`. That dismissal's completion then starts an animated navigation pop and asynchronously reports success; the standard tab flow subsequently dismisses the payments modal and presents `SuccessTxDetailViewController`. A fixed delay does not wait for this chain, and `stableAnchor.view.window != nil` remains true while the hierarchy is being dismissed. Safari can therefore be presented from a transitioning controller, be ignored by UIKit, or compete with the success controller. Chain callback presentation from the actual payment-container dismissal/navigation completion, or coordinate it through the active transition coordinator, instead of inferring completion from elapsed time.
| } | ||
|
|
||
| // BIP21 `callback=<https-url>` — already https-validated at parse time. | ||
| NSURL *inAppCallback = self.paymentInput.request.callback; |
There was a problem hiding this comment.
🔴 Blocking: The required DashSync callback API is not available to a clean build
This line accesses DSPaymentRequest.callback, but the repository records DashSync commit 13fc5bea98a6c0f1b80c4e2c146ff5bf536baf76, whose DSPaymentRequest.h declares callbackScheme but no callback property. Current DashSync master (d3c1497a7b4323a9f21ce3f8f0c5c4e262cab4ca) also lacks it; the property exists only on the still-separate DashSync PR #608 head (aca6889b43961db91ffc8bcc603ebeba5b3f935e). Because the Podfile uses an unpinned ../DashSync/ checkout and this PR updates neither that dependency nor DashSyncCurrentCommit, the exact app head cannot compile against its recorded or normal upstream dependency. Land and record the corresponding DashSync change, or otherwise make the dependency reproducible, before merging this app change.
source: ['claude']
| // BIP21 `callback=<https-url>` — already https-validated at parse time. | ||
| NSURL *inAppCallback = self.paymentInput.request.callback; | ||
| if (inAppCallback && [self.delegate respondsToSelector:@selector(paymentProcessor:openInAppBrowserWithURL:)]) { | ||
| [self.delegate paymentProcessor:self openInAppBrowserWithURL:inAppCallback]; |
There was a problem hiding this comment.
🔴 Blocking: Callback metadata is neither preserved nor bound to the completed payment
Reading the callback from mutable self.paymentInput.request creates two concrete failures. First, DWQRScanModel.m eagerly fetches a scanned URI's r= request and, on success, places only the resulting protocolRequest in the new DWPaymentInput; the original DSPaymentRequest carrying callback is discarded, so this expression is nil and the advertised scanned BIP70 flow never opens its callback. Second, PaymentController.performPayment(with:) resets and reuses the processor, including the Home controller used by incoming payment URLs. If payment A still has a publication or BIP70 relay completion pending when payment B replaces paymentInput, A's completion can read and launch B's callback even though B was not the completed transaction. Preserve callback metadata when converting scanned requests and snapshot it with the protocol request/transaction when starting each send; completion handlers must use that immutable snapshot rather than current processor state.
source: ['claude']
| // BIP21 `callback=<https-url>` — already https-validated at parse time. | ||
| NSURL *inAppCallback = self.paymentInput.request.callback; | ||
| if (inAppCallback && [self.delegate respondsToSelector:@selector(paymentProcessor:openInAppBrowserWithURL:)]) { | ||
| [self.delegate paymentProcessor:self openInAppBrowserWithURL:inAppCallback]; |
There was a problem hiding this comment.
🔴 Blocking: Untrusted callback is opened as a trusted post-payment continuation
The callback comes from an untrusted QR code or app URL, yet it is opened automatically after payment without displaying or approving its destination and without proving that it belongs to the confirmed payee. DashSync PR #608 only checks that the parsed URL's scheme is https; TLS authenticates the callback host, not its relationship to the payment. An attacker can therefore wrap a legitimate, PKI-authenticated BIP70 r= request in an outer BIP21 URI with an attacker-controlled callback: the confirmation authenticates the real merchant, but the post-payment browser opens the attacker's site in that merchant checkout context. Bind the callback to authenticated payment metadata, or show its host and require explicit approval before navigation.
source: ['claude']
3 participants
Feature implemented
Surfaces the BIP21
callback=<https-url>parameter (added inDashSync) to the iOS app's UI layer. When a user scans/pays a URI
that carried a
callback=, the wallet opens that URL in anSFSafariViewControllerimmediately after the send completes somerchants can continue their checkout flow (THORChain swaps,
hosted gift-card flows, etc.) without leaving the wallet app.
Depends on the corresponding DashSync change which:
callback=<https-url>from BIP21 URIs.openInAppBrowserWithURL:optional delegate methodon
DWPaymentProcessorDelegatethat's invoked fromhandleCallbackSchemeIfNeeded:after a successful broadcast.(Link to DashSync PR: )
Also hardens an unrelated force-unwrap in
ProvideAmountViewControllerthat was the actual crash siteduring development testing — described under "What was done?".
What was done?
PaymentController.swiftpaymentProcessor(_:openInAppBrowserWith:)delegateimplementation: after the confirm-screen dismiss + nav-pop
animations settle (~0.4 s), presents an
SFSafariViewControllerpointing at the callback URL.
SFSafariViewController.dw_controller(with:)factory for consistent tint with the rest of the app.
modalPresentationStyle = .overFullScreenandmodalPresentationCapturesStatusBarAppearance = true— matchesthe pattern already used by
TXDetailViewControllerfor its"View on Explorer" button.
(
presentationContextProvider), not fromprovideAmountViewController. The latter is popped from thenav stack on send-success, so presenting on it would reveal a
detached controller on dismiss and crash inside its
viewWillAppear. Using the stable anchor +.overFullScreenkeeps the underlying view in the hierarchy throughout.
DWPaymentProcessor.h/.mpaymentProcessor:openInAppBrowserWithURL:onDWPaymentProcessorDelegate.handleCallbackSchemeIfNeeded:address:tx:now also readsself.paymentInput.request.callback(the newNSURLproperty added in DashSync) and, if present and the delegate
responds, dispatches the callback URL to the host.
httpsschemeenforcement happens at parse time on the DashSync side), so this
layer trusts the URL.
ProvideAmountViewController.swiftnavigationController!force-unwrap inviewWillAppear(_:)with aguard letearly return.viewWillAppearcan fire after this VC has been popped fromthe navigation stack (e.g. when a modal presented from it is
dismissed), at which point
navigationControllerisnilandthe force-unwrap crashes. Defense-in-depth so this category of
dismiss-after-pop crash cannot recur from other code paths.
How Has This Been Tested?
Manual end-to-end testing on Dash mainnet using a TestFlight-style
local build (iOS 26.5, Xcode 16) against
Dash Core:23.1.2peers:
dash:<addr>?amount=0.001&op_return=74657374&callback=https%3A%2F%2Fgoogle.com.After the send completes (1 InstantSend lock),
SFSafariViewControlleropens to
https://google.comand loads cleanly. Tapping Donedismisses without crashing. Reproduced multiple times across
different recipient addresses.
callback=parameter — confirms no Safari sheetis presented (delegate method simply isn't invoked).
no longer triggers the
ProvideAmountViewController.viewWillAppearforce-unwrap path.sweeps) continue to work as before — the new delegate is
@optional, so behaviour for clients that don't implement itis unchanged.
No automated UI / unit-test coverage was added in this PR.
Breaking Changes
None. The new
paymentProcessor:openInAppBrowserWithURL:delegatemethod is declared
@optional, and thehandleCallbackSchemeIfNeeded:callsite usesrespondsToSelector:before invoking it. Existing consumers ofDWPaymentProcessorDelegatecontinue to compile and behaveidentically.
Checklist:
For repository code-owners and collaborators only