Repository navigation
Deliver NUT-18 payments over the configured transport - #2530
Conversation
|
@cdk-bot review |
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- NUT-18 HTTP delivery misreports success when receiver replies with an empty or non-JSON body (medium) - see inline comment
Unanchored locations included in summary:- crates/cdk/src/wallet/payment_request.rs:455
- NUT-18 HTTP delivery no longer follows HTTP redirects (medium) - see inline comment
- NUT-18 delivery now rides a transport that may have TLS certificate verification disabled, exposing ecash proofs to MITM on attacker-supplied receiver URLs (low) - see inline comment
Unanchored locations included in summary:- crates/cdk/src/wallet/payment_request.rs:455
8badcb3 to
09aff6e
Compare
|
@cdk-bot review |
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- NUT-18 TLS-verification refusal is bypassable when the connector is built via
with_transport/with_shared_transportwith a pre-configured transport (medium) - Wallets whose mint connector is built via the publicHttpClient::with_transport/with_shared_transport(ornew) constructors around a transport pre-configured through the publicTransport::with_proxy(proxy, _, accept_invalid_certs=true)(supported by the reqwest backend only; bitreq/wasm reject the flag at build time) end up withtls_verification_disabled == falseeven though the transport accepts any certificate.post_payment_request_payloadthen delivers spendable NUT-18 proofs over HTTPS to a receiver URL chosen by the payment request on an unverified connection, defeating the refusal the PR adds; a network MITM can capture the bearer proofs and redeem them before the receiver. Pre-PR delivery used a fresh, certificate-verifying client, so this exposure is new. All in-repo construction paths keep the flag accurate; the gap is external embedders using the pre-built-transport constructors.
Unanchored locations included in summary:- crates/cdk/src/wallet/mint_connector/http_client.rs:200
- crates/cdk/src/wallet/mint_connector/http_client.rs:257
- crates/cdk-http-client/src/transport/mod.rs:104
- NUT-18 HTTP POST delivery now hard-fails for wallets built with a custom mint connector (low) - Wallets built with a custom
MintConnectorimplementation (public extension points:WalletBuilder::client/shared_client,WalletConfig.mint_connector,Wallet::set_client) inherit the new trait defaultErr(PaymentRequestDeliveryUnsupported)forpost_payment_request_payload, with no direct-request fallback. For that embedder class, NUT-18 payment requests that only advertise an HttpPost transport — which worked on base for any connector — now fail 100% of the time at runtime (no compile-time signal). The failure surfaces inPreparedPaymentRequest::confirm()afterconfirm_sendhas already swapped the proofs into a pending send, so the payer must callrevoke_sendwith the returned operation id to reclaim funds, even though the delivery capability was deterministically absent before any funds moved. Behavior is documented and intentional per the PR's own docs and test, so this is disclosed as a sign-off/migration/fail-fast note rather than an unintended bug.
Unanchored locations included in summary:- crates/cdk/src/wallet/payment_request.rs:455
- Rewritten custom-transport example follows redirects, silently reintroducing the "redirected POST reported as delivered" failure the PR fixes (low) - The canonical custom-transport example (
crates/cdk/examples/mint-token-bolt12-with-custom-http.rs, rewritten by this PR to the newTransport::http_postshape and wired into the wallet via.client(...)) configures its ureq 3.4.0 agent without a redirect policy, so it follows up to 10 redirects by default and converts a POST answered 301/302/303 into a bodyless GET. Since this PR routes NUT-18 HttpPost delivery through the connector transport, a wallet built by copying the example reportsPaymentRequestPayloaddelivery as successful whenever a receiver answers 301/302 (e.g., trailing-slash normalization or http→https upgrade) and the redirect target returns 2xx to the GET: the proofs never reach the receiver, no error prompts the payer, and the swapped proofs sit in the pending/reserved state until someone notices and callsrevoke_send. A hostile receiver can also use this to fake successful delivery. The failure is confined to wallets using this example's transport pattern (or any other redirect-following custom transport); the library's built-inAsync/TorAsynctransports are unaffected because the PR pins them withno_redirects().
Unanchored locations included in summary:- crates/cdk/examples/mint-token-bolt12-with-custom-http.rs:32
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2530 +/- ##
==========================================
+ Coverage 79.88% 80.01% +0.12%
==========================================
Files 387 387
Lines 106600 107093 +493
==========================================
+ Hits 85158 85689 +531
+ Misses 21442 21404 -38 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
09aff6e to
56043c4
Compare
|
@cdk-bot review |
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- RateLimitedTransport doesn't override tls_verification_disabled, so its default false bypasses the new unverified-TLS refusal for NUT-18 proof delivery and LNURL fetches (medium) - A wallet client built as HttpClient::with_transport(mint, RateLimitedTransport::with_manager(unverified_transport, limiter), ...) — the composition the exported RateLimitedHttpClient/RateLimitedTorHttpClient aliases exist for — reports TLS verification enabled while all HTTPS requests run unverified. NUT-18 proof delivery (bearer e-cash) to https receivers and LNURL invoice fetches then pass ensure_verified_tls, exposing proofs/invoices to a MITM who terminates TLS with any certificate.
Additional locations included in summary:- crates/cdk/src/wallet/mint_connector/http_client.rs:179
- crates/cdk/src/wallet/mint_connector/http_client.rs:528
Unanchored locations included in summary: - crates/cdk-common/src/rate_limit.rs:772
|
@cdk-bot review |
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- NUT-18 unverified-TLS refusal fires only at confirm time, after proofs are reserved and swap fees paid (medium) - For a wallet whose transport has TLS certificate verification disabled (WalletRepository::danger_accept_invalid_certs + proxy, HttpClient::with_proxy(..., true), or an embedder-supplied transport reporting tls_verification_disabled() — all paths this PR propagates), any HTTP-POST-only NUT-18 request with an https target deterministically fails inside confirm() after confirm_send has already run the mint swap: swap input fees are consumed and the token is left as a pending send that must be discovered and reclaimed with revoke_send (which rides another swap). The unambiguous refusal is wrapped as PaymentRequestDeliveryFailed, whose doc tells the payer to treat it as an ambiguous outcome, and retrying can never succeed. No fund theft — proofs are reclaimable — but this is exactly the fee-loss-plus-stuck-pending-send outcome the PR's prepare-time capability check (commit 528f9ce) was added to prevent, and a regression vs. base where delivery used a fresh verifying client and succeeded.
Additional locations included in summary:- crates/cdk/src/wallet/mint_connector/http_client.rs:519
- crates/cdk/src/wallet/mint_connector/mod.rs:102
bf95c28 to
f42271c
Compare
|
@cdk-bot review |
f42271c to
c943dfc
Compare
|
@cdk-bot review |
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- NUT-18 pre-check lets parsable non-http(s) receiver targets through, so the swap still pays the input fee first (low) - For an HTTP-only NUT-18 request whose target parses as a URL but uses a scheme the HTTP transport cannot speak (e.g. ftp://, ws://, mailto:), the new pre-check passes, the swap at confirm() pays the mint input fee, and only then does delivery fail — leaving a pending send that must be revoked. This is exactly the outcome the PR's commits and the new trait docs promise to prevent, and the non-http scheme is knowable up front for a transport whose only verbs are http_get/http_post/http_post_form.
Additional locations included in summary:- crates/cdk/src/wallet/payment_request.rs:328
- On wasm32 a redirecting NUT-18 receiver is misreported as a fetch failure, so the new Transport redirect contract (and PaymentRequestDeliveryRedirected) is unreachable (low) - On wasm32, when a NUT-18 receiver answers a delivery with a 3xx, the payer gets Error::HttpError(None, "Fetch failed: …") wrapped in PaymentRequestDeliveryFailed instead of Error::PaymentRequestDeliveryRedirected and its guidance that the receiver's final URL must be obtained from the receiver. Funds safety is identical (both classifications are non-definitive; the pending-send/revoke guidance applies either way), so this is a diagnostics/contract gap limited to wasm builds, not a funds risk.
Additional locations included in summary:- crates/cdk/src/wallet/mint_connector/http_client.rs:529
Unanchored locations included in summary: - crates/cdk-http-client/src/backends/wasm_backend.rs:159
- crates/cdk-http-client/src/backends/wasm_backend.rs:187
- crates/cdk/src/wallet/mint_connector/http_client.rs:529
1fc6eb0 to
8b93bed
Compare
|
@cdk-bot review |
|
Code review ACK 8b93bed On ambiguous delivery failure, |
|
@cdk-bot review |
It is unrelated, if it is an issue it should be addresed in a follow-up |
8b93bed to
bb69aed
Compare
|
@cdk-bot review |
bb69aed to
ed3beec
Compare
BREAKING CHANGE: HttpTransport GET and POST methods now return raw responses. Update custom implementations to the new signatures and use json_or_status_error when typed responses are needed.
Check payment-method eligibility and balance before retaining a delivery refusal during wallet selection. Underfunded wallets no longer mask InsufficientFunds with an unrelated connector error. Cover empty and underfunded unsupported wallets alongside an underfunded deliverable wallet in a regression test.
ed3beec to
73ef05d
Compare
|
Squashed, rebased and added on commit to keep the low balance error. |
Description
Fixes #2525
HTTP delivery built a fresh default client, so a wallet configured with --proxy or Tor still reached the recipient over ordinary network egress. The recipient learned the payer's address even though every mint request went through the configured transport.
Delivery now rides the wallet's mint connector, alongside the other non-mint calls it already makes. A connector without delivery support fails explicitly rather than falling back to a direct request.
The receiver's response is now parsed as JSON, so a success answered with an empty body is reported as a failure. NUT-18 does not define a response body. Delivery also stops following redirects and is paced by the client rate limiter, both inherited from the connector transport.
Notes to the reviewers
Suggested CHANGELOG Updates
CHANGED
ADDED
REMOVED
FIXED
Checklist
just quick-checkbefore committingcrates/cdk-ffi)