diff --git a/src/app/add_invoice.rs b/src/app/add_invoice.rs index 6cac52f1..15380488 100644 --- a/src/app/add_invoice.rs +++ b/src/app/add_invoice.rs @@ -1,7 +1,7 @@ use crate::app::context::AppContext; use crate::util::{ enqueue_order_msg, get_order, notify_taker_reputation, show_hold_invoice, update_order_event, - validate_invoice, + validate_invoice, InvoiceCheck, }; use mostro_core::db::Crud; use mostro_core::prelude::*; @@ -50,18 +50,29 @@ pub async fn add_invoice_action( if buyer_pubkey != event.sender { return Err(MostroCantDo(CantDoReason::InvalidPeer)); } + // Reject on status before the payment request is looked at. The status + // check is a local comparison while validating the request reaches out to + // a host named by the sender, so an order that can't accept an invoice at + // all should never pay for that round-trip. + if !matches!( + ord_status, + Status::SettledHoldInvoice | Status::WaitingBuyerInvoice + ) { + return Err(MostroCantDo(CantDoReason::NotAllowedByStatus)); + } + // We save the invoice on db - order.buyer_invoice = validate_invoice(&msg, &order).await?; + // + // `Online` keeps the pre-existing behavior: the buyer is submitting the + // destination their payout will go to, so an unreachable one should be + // rejected now rather than at release. Bounded by + // `lnurl::LNURL_TOTAL_BUDGET`. + order.buyer_invoice = validate_invoice(&msg, &order, InvoiceCheck::Online).await?; + // Buyer can add invoice orders with WaitingBuyerInvoice status - match ord_status { - Status::SettledHoldInvoice => { - pay_new_invoice(&mut order, pool, &msg).await?; - return Ok(()); - } - Status::WaitingBuyerInvoice => {} - _ => { - return Err(MostroCantDo(CantDoReason::NotAllowedByStatus)); - } + if ord_status == Status::SettledHoldInvoice { + pay_new_invoice(&mut order, pool, &msg).await?; + return Ok(()); } // Notify taker reputation @@ -247,6 +258,62 @@ mod tests { )); } + /// A disallowed status must be rejected without the payment request ever + /// being resolved. The status check is a local string comparison; the + /// resolution is a round-trip to a host the sender chose, and the handler + /// runs on the shared event loop, so the cheap check has to come first. + #[tokio::test] + async fn add_invoice_action_rejects_disallowed_status_before_resolving_address() { + use lnurl::lnurl::LnUrl; + + let pool = setup_pool().await; + let ctx = build_ctx(pool.clone()); + let seller = Keys::generate().public_key(); + let buyer = Keys::generate().public_key(); + + let mut order = waiting_invoice_sell_order(seller, buyer); + order.status = Status::Active.to_string(); + let order = order.create(ctx.pool()).await.unwrap(); + + // A host that completes the handshake and then never answers: any + // attempt to resolve this address would burn the full LNURL budget. + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let port = listener.local_addr().unwrap().port(); + let listener_task = tokio::spawn(async move { + let mut held = Vec::new(); + while let Ok((stream, _)) = listener.accept().await { + held.push(stream); + } + }); + let lnurl = LnUrl { + url: format!("http://127.0.0.1:{port}/.well-known/lnurlp/blackhole"), + } + .encode(); + + let msg = Message::new_order( + Some(order.id), + Some(1), + None, + Action::AddInvoice, + Some(Payload::PaymentRequest(None, lnurl, None)), + ); + let event = buyer_event(buyer); + + let start = std::time::Instant::now(); + let result = add_invoice_action(&ctx, msg, &event, &Keys::generate()).await; + let elapsed = start.elapsed(); + listener_task.abort(); + + assert!( + matches!(result, Err(MostroCantDo(CantDoReason::NotAllowedByStatus))), + "a disallowed status must be rejected, got {result:?}" + ); + assert!( + elapsed < std::time::Duration::from_secs(1), + "status rejection must not wait on the payment request, took {elapsed:?}" + ); + } + #[tokio::test] async fn add_invoice_action_rejects_disallowed_status() { let pool = setup_pool().await; diff --git a/src/app/dev_fee.rs b/src/app/dev_fee.rs index 32a6f382..4006cfab 100644 --- a/src/app/dev_fee.rs +++ b/src/app/dev_fee.rs @@ -912,22 +912,16 @@ pub async fn resolve_dev_fee_invoice( // fee payment came from. let comment = dev_fee_comment(&order.id, &keys.public_key()); - let payment_request = tokio::time::timeout( - std::time::Duration::from_secs(15), - resolv_ln_address( - DEV_FEE_LIGHTNING_ADDRESS, - order.dev_fee as u64, - Some(comment.as_str()), - ), + // No timeout wrapper here: `resolv_ln_address` bounds itself with + // `lnurl::LNURL_TOTAL_BUDGET`, which covers both round-trips it makes. + // A second, longer deadline layered on top could never fire, and would + // only advertise a limit that isn't the real one. + let payment_request = resolv_ln_address( + DEV_FEE_LIGHTNING_ADDRESS, + order.dev_fee as u64, + Some(comment.as_str()), ) .await - .map_err(|_| { - error!( - "Dev fee LNURL resolution timeout for order {} ({} sats)", - order.id, order.dev_fee - ); - MostroInternalErr(ServiceError::LnAddressParseError) - })? .map_err(|e| { error!( "Dev fee LNURL resolution failed for order {} ({} sats): {:?}", diff --git a/src/app/order.rs b/src/app/order.rs index 73b3782c..f2ba5e7d 100644 --- a/src/app/order.rs +++ b/src/app/order.rs @@ -1,6 +1,6 @@ use crate::app::context::AppContext; use crate::db::update_user_trade_index; -use crate::util::{get_bitcoin_price, publish_order, validate_invoice}; +use crate::util::{get_bitcoin_price, publish_order, validate_invoice, InvoiceCheck}; use mostro_core::prelude::*; use nostr_sdk::prelude::*; use nostr_sdk::Keys; @@ -92,9 +92,6 @@ pub async fn order_action( let request_id = msg.get_inner_message_kind().request_id; if let Some(order) = msg.get_inner_message_kind().get_order() { - // Validate invoice - let _invoice = validate_invoice(&msg, &Order::from(order.clone())).await?; - // Check if fiat currency is accepted let mostro_settings = &ctx.settings().mostro; if let Err(cause) = order.check_fiat_currency(&mostro_settings.fiat_currencies_accepted) { @@ -132,6 +129,31 @@ pub async fn order_action( calculate_and_check_quote(ctx, order, fiat_amount).await?; } + // Validate the payment request last: every check above is local and + // cheap, so a malformed order is rejected without touching it. + // + // `Offline` deliberately skips resolving a lightning address or LNURL + // over the network. Note this is not a throwaway value — the address + // is persisted as `orders.buyer_invoice` (see `prepare_new_order`) and + // is what `do_payment` pays out to. Skipping the reachability probe + // here is safe *because* a failure to resolve at payout time is now + // handled as a payment failure: the buyer is notified and the retry + // job picks the order up. Checking it here instead would put an HTTP + // request to a sender-chosen host on the event loop for every order + // created, delaying every other message behind it, in exchange for a + // liveness answer that can go stale before it is ever used. + // + // The returned value is discarded because it is the same string that + // gets persisted, not a different one. For a `NewOrder` the payload is + // `Payload::Order`, and `Message::get_payment_request` — what + // `validate_invoice` reads — resolves that to `ord.buyer_invoice`, + // which is the exact field `prepare_new_order` copies into the stored + // order. (A `NewOrder` carrying any other payload never reaches here: + // `get_order` returns `None` and this whole block is skipped.) So the + // invoice that is checked and the invoice that is saved are one value. + let _invoice = + validate_invoice(&msg, &Order::from(order.clone()), InvoiceCheck::Offline).await?; + let trade_index = match msg.get_inner_message_kind().trade_index { Some(trade_index) => trade_index, None => { diff --git a/src/app/release.rs b/src/app/release.rs index 8c7bd722..baf839ea 100644 --- a/src/app/release.rs +++ b/src/app/release.rs @@ -16,7 +16,36 @@ use sqlx::{Pool, Sqlite}; use std::cmp::Ordering; use std::str::FromStr; use tokio::sync::mpsc::channel; -use tracing::info; +use tracing::{error, info}; + +/// Record a failed payout attempt so the order stays recoverable. +/// +/// Every path that gives up on paying the buyer after the hold invoice has +/// been settled has to come through here. `find_failed_payment` selects orders +/// on `failed_payment == true`, so an attempt that returns without setting it +/// leaves the order settled, the buyer unpaid and unnotified, and invisible to +/// the retry job — and a buyer sending a corrected invoice does not help, +/// since that path only resets `payment_attempts` and relies on that same job. +/// +/// The bookkeeping can itself fail: the `UPDATE` may error, or an order whose +/// amount or kind no longer validates bails out on the retries-exhausted +/// branch. That leaves exactly the unrecoverable state this function exists to +/// prevent, so it is logged at `error` rather than dropped — both callers of +/// [`do_payment`] discard its `Result`, which makes the log the only signal an +/// operator ever gets. +async fn record_payout_failure(ctx: &AppContext, order: &Order, request_id: Option) { + match check_failure_retries(ctx, order, request_id).await { + Ok(failed_payment) => info!( + "Order id {} has {} failed payments retries", + failed_payment.id, failed_payment.payment_attempts + ), + Err(e) => error!( + "Order id {}: could not record the payment failure ({:?}); the order may stay \ + settled and unpaid until it is recovered manually", + order.id, e + ), + } +} /// Check if order has failed payment retries pub async fn check_failure_retries( @@ -496,24 +525,56 @@ pub async fn do_payment( return Err(MostroInternalErr(ServiceError::InvoiceInvalidError)); } let payment_request = if let Ok(addr) = ln_addr { - resolv_ln_address(&addr.to_string(), amount, None) - .await - .map_err(|_| MostroInternalErr(ServiceError::LnAddressParseError))? + // Resolving a lightning address is a network round-trip to a host the + // buyer chose. When it yields no invoice — host unreachable, too slow, + // or answering without a `pr` — that is a payment failure like any + // other and has to go through the same bookkeeping. + // + // Returning early instead would leave `failed_payment = false`, and + // that flag is exactly the predicate `find_failed_payment` selects on + // (`db.rs`). The order would sit in `SettledHoldInvoice` with the + // seller's funds captured, the buyer unpaid and unnotified, and + // invisible to the retry job — and a buyer sending a corrected + // invoice wouldn't help either, since that path only resets + // `payment_attempts` and relies on the same job to act on it. + match resolv_ln_address(&addr.to_string(), amount, None).await { + Ok(pr) if !pr.is_empty() => pr, + outcome => { + match outcome { + Err(e) => info!( + "Order id {}: could not resolve payout address: {:?}", + order.id, e + ), + _ => info!("Order id {}: payout address returned no invoice", order.id), + } + record_payout_failure(ctx, &order, request_id).await; + return Err(MostroInternalErr(ServiceError::LnAddressParseError)); + } + } } else { payment_request }; - let mut ln_client_payment = LndConnector::new().await?; + // Reaching LND is part of paying, so a handshake that fails or times out + // is a failed payout attempt — not a reason to return ahead of the + // bookkeeping. `LndConnector::new` now bounds that handshake, which makes + // this path reachable on a node whose LND is wedged rather than down. + let mut ln_client_payment = match LndConnector::new().await { + Ok(client) => client, + Err(e) => { + info!( + "Order id {}: could not connect to LND for payout: {:?}", + order.id, e + ); + record_payout_failure(ctx, &order, request_id).await; + return Err(e); + } + }; let (tx, mut rx) = channel(100); let payment_task = ln_client_payment.send_payment(&payment_request, amount as i64, tx); if let Err(paymement_result) = payment_task.await { info!("Error during ln payment : {}", paymement_result); - if let Ok(failed_payment) = check_failure_retries(ctx, &order, request_id).await { - info!( - "Order id {} has {} failed payments retries", - failed_payment.id, failed_payment.payment_attempts - ); - } + record_payout_failure(ctx, &order, request_id).await; } // Get Mostro keys from context @@ -1653,24 +1714,96 @@ mod tests { )); } + /// A payout address that yields no invoice must be recorded as a payment + /// failure, not returned as a bare error. + /// + /// The distinction is the whole point: `find_failed_payment` retries on + /// `failed_payment == true`, so an order that fails resolution without + /// setting that flag is settled, unpaid and invisible to the retry job — + /// and the buyer never hears about it. + #[tokio::test] + async fn do_payment_records_failure_when_payout_address_yields_no_invoice() { + // Arrange: an order settled and awaiting payout, whose buyer invoice + // is a lightning address that cannot produce an invoice. Under + // cfg(test) the address resolves to a local well-known path that + // isn't served, so this never leaves the machine. + init_global_config(); + let pool = create_test_pool().await; + let ctx = build_ctx(&pool); + let seller = Keys::generate().public_key(); + let buyer = Keys::generate().public_key(); + let mut order = fiat_sent_sell_order(seller, buyer); + order.status = Status::SettledHoldInvoice.to_string(); + order.buyer_invoice = Some("nosuchpayee@localhost".to_string()); + order.amount = 1_000; + order.fee = 10; + order.payment_attempts = 0; + order.failed_payment = false; + let order = order.create(&pool).await.unwrap(); + + // Act + let result = do_payment(&ctx, order.clone(), None).await; + + // Assert: the call still fails, but it left the bookkeeping behind + // that makes the failure recoverable. + assert!(result.is_err(), "unresolvable payout address must fail"); + let (failed_payment, payment_attempts): (bool, i64) = + sqlx::query_as("SELECT failed_payment, payment_attempts FROM orders WHERE id = ?") + .bind(order.id) + .fetch_one(&pool) + .await + .unwrap(); + assert!( + failed_payment, + "failed_payment must be set so find_failed_payment can retry the order" + ); + assert_eq!( + payment_attempts, 1, + "the failed attempt must be counted, not silently dropped" + ); + } + + /// Failing to reach LND is a failed payout attempt like any other, so it + /// has to leave the same bookkeeping behind. Without it a settled order + /// whose LND handshake fails would never be retried, since + /// `find_failed_payment` only selects on `failed_payment == true`. #[tokio::test] async fn do_payment_fails_fast_when_lnd_is_unreachable() { // Arrange: with the global config set to test defaults, the LND cert // path is invalid, so LndConnector::new() returns an error without - // any network access. + // any network access. The buyer invoice is not a lightning address, + // so address resolution is skipped and LND is the first thing tried. init_global_config(); let pool = create_test_pool().await; let ctx = build_ctx(&pool); let seller = Keys::generate().public_key(); let buyer = Keys::generate().public_key(); let mut order = fiat_sent_sell_order(seller, buyer); + order.status = Status::SettledHoldInvoice.to_string(); order.buyer_invoice = Some("lnbc1notchecked".to_string()); + order.payment_attempts = 0; + order.failed_payment = false; + let order = order.create(&pool).await.unwrap(); // Act - let result = do_payment(&ctx, order, None).await; + let result = do_payment(&ctx, order.clone(), None).await; // Assert assert!(result.is_err()); + let (failed_payment, payment_attempts): (bool, i64) = + sqlx::query_as("SELECT failed_payment, payment_attempts FROM orders WHERE id = ?") + .bind(order.id) + .fetch_one(&pool) + .await + .unwrap(); + assert!( + failed_payment, + "an unreachable LND must still mark the payout as failed so it can be retried" + ); + assert_eq!( + payment_attempts, 1, + "the failed attempt must be counted, not silently dropped" + ); } #[tokio::test] diff --git a/src/app/take_sell.rs b/src/app/take_sell.rs index 66eebdba..49e4f908 100644 --- a/src/app/take_sell.rs +++ b/src/app/take_sell.rs @@ -5,6 +5,7 @@ use crate::db::{buyer_has_pending_order, update_user_trade_index}; use crate::util::{ enqueue_order_msg, get_dev_fee, get_fiat_amount_requested, get_market_amount_and_fee, get_order, set_waiting_invoice_status, show_hold_invoice, update_order_event, validate_invoice, + InvoiceCheck, }; use mostro_core::db::Crud; use mostro_core::prelude::*; @@ -171,7 +172,12 @@ pub async fn take_sell_action( // Validate invoice and get payment request if present // NOW dev_fee is set correctly for proper validation - let payment_request = validate_invoice(&msg, &order).await?; + // + // `Online` keeps the pre-existing behavior: this is the buyer committing + // to a payout destination, so an unreachable one should block the take + // rather than surface at release time. The round-trip is bounded by + // `lnurl::LNURL_TOTAL_BUDGET`. + let payment_request = validate_invoice(&msg, &order, InvoiceCheck::Online).await?; let trade_index = match msg.get_inner_message_kind().trade_index { Some(trade_index) => trade_index, diff --git a/src/lightning/invoice.rs b/src/lightning/invoice.rs index abd4664e..c9819bef 100644 --- a/src/lightning/invoice.rs +++ b/src/lightning/invoice.rs @@ -194,6 +194,43 @@ pub async fn is_valid_invoice( validate_bolt11_invoice(&payment_request, amount, fee).await } +/// Validates a payment request without performing any network I/O. +/// +/// Identical to [`is_valid_invoice`] for BOLT11 invoices — every BOLT11 check +/// is local, so none of them are skipped. The difference is lightning +/// addresses and LNURLs: those are accepted on syntax alone, with no +/// round-trip to the host they name. +/// +/// # Why a separate entry point +/// +/// Resolving a lightning address means an HTTP request to a host the sender +/// picked, and callers on the event loop pay that latency before anything +/// else can be processed. Where reachability isn't yet needed, checking it +/// early buys nothing: the address still has to resolve at payout time, and +/// that's where an unreachable host is actually actionable. Order creation +/// is the clear case — it stores the address and discards the validation +/// result — so it uses this variant and lets the network check happen when +/// the address is really used. +/// +/// Use [`is_valid_invoice`] where reachability is part of the decision being +/// made, such as accepting a buyer's payout destination on a take. +pub async fn is_valid_invoice_offline( + payment_request: String, + amount: Option, + fee: Option, +) -> Result<(), MostroError> { + // A parseable lightning address or LNURL is all this variant checks: + // both constructors fully validate the syntax they accept. + if LightningAddress::from_str(&payment_request).is_ok() + || LnUrl::from_str(&payment_request).is_ok() + { + return Ok(()); + } + + // Anything else must still clear full BOLT11 validation, which is local. + validate_bolt11_invoice(&payment_request, amount, fee).await +} + #[cfg(test)] mod tests { use super::*; @@ -417,6 +454,108 @@ mod tests { ); } + /// Bind a listener that completes the TCP handshake and then never + /// answers, on an OS-assigned port, and return an LNURL pointing at it. + /// + /// A lightning address can't be used here: under `cfg(test)` + /// `extract_lnurl` pins those to port 8080, which `start_test_server` + /// already owns. An LNURL carries its own URL, so it can target an + /// ephemeral port and stay independent of the other tests. + async fn start_blackhole_server() -> (String, tokio::task::JoinHandle<()>) { + let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); + let port = listener.local_addr().expect("local addr").port(); + let handle = tokio::spawn(async move { + // Hold every accepted socket open without writing a response. + let mut held = Vec::new(); + while let Ok((stream, _)) = listener.accept().await { + held.push(stream); + } + }); + let url = format!("http://127.0.0.1:{port}/.well-known/lnurlp/blackhole"); + (LnUrl { url }.encode(), handle) + } + + /// A host that accepts the connection and then stalls must not be able to + /// hold invoice validation open for longer than the LNURL budget. The + /// connect timeout is deliberately not exercised: the handshake succeeds, + /// so only the total budget can end this. + #[tokio::test] + async fn lnurl_validation_against_unresponsive_host_is_time_bounded() { + init_settings_test(); + let (lnurl, handle) = start_blackhole_server().await; + + let start = std::time::Instant::now(); + let result = is_valid_invoice(lnurl, None, None).await; + let elapsed = start.elapsed(); + handle.abort(); + + assert!(result.is_err(), "an unresponsive host must not validate"); + assert!( + elapsed < crate::lnurl::LNURL_TOTAL_BUDGET + std::time::Duration::from_secs(2), + "validation took {elapsed:?}, which exceeds the LNURL budget of {:?}", + crate::lnurl::LNURL_TOTAL_BUDGET + ); + } + + /// The offline variant is what order creation uses: a syntactically valid + /// LNURL passes without any HTTP round-trip, so an unresponsive host costs + /// nothing at all. + #[tokio::test] + async fn offline_validation_of_lnurl_does_no_network_io() { + init_settings_test(); + let (lnurl, handle) = start_blackhole_server().await; + + let start = std::time::Instant::now(); + let result = is_valid_invoice_offline(lnurl, None, None).await; + let elapsed = start.elapsed(); + handle.abort(); + + assert!( + result.is_ok(), + "a syntactically valid LNURL must pass offline validation: {result:?}" + ); + assert!( + elapsed < std::time::Duration::from_secs(1), + "offline validation must not contact the host, but took {elapsed:?}" + ); + } + + #[tokio::test] + async fn offline_validation_accepts_well_formed_lightning_address() { + init_settings_test(); + assert!( + is_valid_invoice_offline("MostroP2P@example.com".to_string(), None, None) + .await + .is_ok(), + "a well-formed lightning address must pass offline validation" + ); + } + + /// Dropping the network round-trip must not drop the local checks: a + /// payment request that is neither an address nor an LNURL still has to + /// clear full BOLT11 validation. + #[tokio::test] + async fn offline_validation_still_rejects_malformed_payment_request() { + init_settings_test(); + assert!( + is_valid_invoice_offline("not-a-payment-request".to_string(), None, None) + .await + .is_err(), + "garbage must still be rejected without network access" + ); + } + + #[tokio::test] + async fn offline_validation_still_enforces_bolt11_amount_checks() { + init_settings_test(); + let payment_request = build_test_invoice(Some(1_000_000), 86_400); + assert_eq!( + is_valid_invoice_offline(payment_request, Some(5_000), None).await, + Err(MostroInternalErr(ServiceError::InvoiceInvalidError)), + "BOLT11 amount mismatch must still be caught offline" + ); + } + #[tokio::test] async fn test_lnurl_validation_with_test_server() { init_settings_test(); diff --git a/src/lightning/mod.rs b/src/lightning/mod.rs index 56788678..93e979bf 100644 --- a/src/lightning/mod.rs +++ b/src/lightning/mod.rs @@ -15,7 +15,9 @@ use mostro_core::prelude::*; use nostr_sdk::nostr::hashes::hex::FromHex; use nostr_sdk::nostr::secp256k1::rand::{self, RngCore}; use std::cmp::Ordering; +use std::time::Duration; use tokio::sync::mpsc::Sender; +use tokio::time::timeout; use tracing::info; #[derive(Clone)] @@ -90,17 +92,39 @@ fn decode_hash32(field: &str, value: &str) -> Result, MostroError> { Ok(bytes) } +/// Cap on establishing a gRPC connection to LND. +/// +/// `fedimint_tonic_lnd::connect` has no deadline of its own, and several +/// handlers open a fresh connection while running on the single event-loop +/// task in `app::run` — so an unreachable or wedged LND would otherwise stall +/// message processing for everyone, with no upper bound. +/// +/// Bounding the *connection* is safe in a way that bounding the RPCs after it +/// is not: if this elapses, no request was issued and no invoice state was +/// touched, so failing is unambiguous. The RPCs themselves are deliberately +/// left unbounded here — a timeout on `settle_invoice` would report failure +/// for an operation LND may still have completed. +const LND_CONNECT_TIMEOUT: Duration = Duration::from_secs(10); + impl LndConnector { pub async fn new() -> Result { let ln_settings = Settings::get_ln(); // Connecting to LND requires only host, port, cert file, and macaroon file - let client = fedimint_tonic_lnd::connect( - ln_settings.lnd_grpc_host.clone(), - ln_settings.lnd_cert_file.clone(), - ln_settings.lnd_macaroon_file.clone(), + let client = timeout( + LND_CONNECT_TIMEOUT, + fedimint_tonic_lnd::connect( + ln_settings.lnd_grpc_host.clone(), + ln_settings.lnd_cert_file.clone(), + ln_settings.lnd_macaroon_file.clone(), + ), ) .await + .map_err(|_| { + MostroInternalErr(ServiceError::LnNodeError(format!( + "timed out connecting to LND after {LND_CONNECT_TIMEOUT:?}" + ))) + })? .map_err(|e| MostroInternalErr(ServiceError::LnNodeError(e.to_string())))?; // Safe unwrap here diff --git a/src/lnurl.rs b/src/lnurl.rs index c1c2175e..3064ecd0 100644 --- a/src/lnurl.rs +++ b/src/lnurl.rs @@ -3,11 +3,39 @@ use mostro_core::prelude::*; use once_cell::sync::Lazy; use reqwest::Client; use serde_json::Value; +use std::time::Duration; +use tokio::time::timeout; use tracing::{error, warn}; +/// Cap on a single HTTP round-trip to an LNURL endpoint. +const LNURL_REQUEST_TIMEOUT: Duration = Duration::from_secs(4); + +/// Cap on establishing the TCP/TLS connection. Separate from the request +/// timeout so an unroutable or filtered host fails fast instead of spending +/// the whole request budget on a handshake that will never complete. +const LNURL_CONNECT_TIMEOUT: Duration = Duration::from_secs(2); + +/// Redirect budget. Refusing redirects outright would break real endpoints +/// (`www` to apex, provider migrations), but the chain has to be finite. +const LNURL_MAX_REDIRECTS: usize = 3; + +/// Total wall-clock budget for one LNURL operation, covering every HTTP +/// round-trip it makes. +/// +/// [`HTTP_CLIENT`]'s own timeout is *per request*, and [`resolv_ln_address`] +/// issues two sequential requests where the second host is chosen by the +/// first response — so a per-request cap alone lets a remote endpoint cost +/// twice that. This constant is the one that actually bounds the operation. +/// +/// It matters because the callers run on the single event-loop task in +/// `app::run`: this is how long one message can hold up everyone else's. +pub const LNURL_TOTAL_BUDGET: Duration = Duration::from_secs(5); + pub static HTTP_CLIENT: Lazy = Lazy::new(|| { Client::builder() - .timeout(std::time::Duration::from_secs(10)) + .timeout(LNURL_REQUEST_TIMEOUT) + .connect_timeout(LNURL_CONNECT_TIMEOUT) + .redirect(reqwest::redirect::Policy::limited(LNURL_MAX_REDIRECTS)) .user_agent(concat!("mostro/", env!("CARGO_PKG_VERSION"))) .build() .expect("valid reqwest Client") @@ -53,7 +81,22 @@ async fn extract_lnurl(address: &str) -> Result { Ok(url) } +/// Check that `address` resolves to a live LNURL-pay endpoint. +/// +/// Bounded by [`LNURL_TOTAL_BUDGET`]: the address is remote input and the +/// host it names is under no obligation to answer, so a caller on the event +/// loop must not be able to wait on it indefinitely. A host that exhausts +/// the budget is reported as unreachable, which is what it is from here. pub async fn ln_exists(address: &str) -> Result<(), MostroError> { + timeout(LNURL_TOTAL_BUDGET, ln_exists_inner(address)) + .await + .map_err(|_| { + warn!("LNURL existence check exceeded the {LNURL_TOTAL_BUDGET:?} budget"); + MostroInternalErr(ServiceError::NoAPIResponse) + })? +} + +async fn ln_exists_inner(address: &str) -> Result<(), MostroError> { // Get the url from the str - could be a LNURL or a Lightning Address let url = extract_lnurl(address).await?; // Make the request to the LNURL @@ -150,10 +193,30 @@ fn build_callback_url( /// server rejected the request or doesn't support `payRequest` /// * `Err(MostroError)` - If the address/LNURL can't be resolved or the HTTP /// exchange fails +/// +/// Bounded by [`LNURL_TOTAL_BUDGET`] across *both* round-trips it makes: the +/// callback host comes from the first response, so without a total budget a +/// remote server could spend the per-request timeout twice. pub async fn resolv_ln_address( address: &str, amount: u64, comment: Option<&str>, +) -> Result { + timeout( + LNURL_TOTAL_BUDGET, + resolv_ln_address_inner(address, amount, comment), + ) + .await + .map_err(|_| { + warn!("LNURL resolution exceeded the {LNURL_TOTAL_BUDGET:?} budget"); + MostroInternalErr(ServiceError::NoAPIResponse) + })? +} + +async fn resolv_ln_address_inner( + address: &str, + amount: u64, + comment: Option<&str>, ) -> Result { // Get the url from the str - could be a LNURL or a Lightning Address let url = extract_lnurl(address).await?; diff --git a/src/util.rs b/src/util.rs index b590c04c..53bcc186 100644 --- a/src/util.rs +++ b/src/util.rs @@ -8,7 +8,7 @@ use crate::db::is_user_present; use crate::escrow::EscrowBackend; use crate::flow; use crate::lightning; -use crate::lightning::invoice::is_valid_invoice; +use crate::lightning::invoice::{is_valid_invoice, is_valid_invoice_offline}; use crate::messages; use crate::nip33::{create_platform_tag_values, new_order_event, new_rating_event, order_to_tags}; use crate::NOSTR_CLIENT; @@ -1426,7 +1426,33 @@ pub async fn get_user_orders_by_id( Ok(found_orders) } -pub async fn validate_invoice(msg: &Message, order: &Order) -> Result, MostroError> { +/// How thoroughly [`validate_invoice`] should check a payment request. +/// +/// The distinction only affects lightning addresses and LNURLs. BOLT11 +/// invoices are validated identically either way, since every BOLT11 check +/// is local. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum InvoiceCheck { + /// Syntax and local BOLT11 rules only — no network I/O. + /// + /// The right choice when the payment request is merely being recorded + /// and reachability isn't part of the decision. Callers run on the + /// event loop, so skipping the round-trip here keeps one sender's + /// choice of host from delaying every other user's messages. + Offline, + /// Additionally resolve lightning addresses and LNURLs over the network, + /// bounded by `lnurl::LNURL_TOTAL_BUDGET`. + /// + /// The right choice when an unreachable destination should block the + /// action outright. + Online, +} + +pub async fn validate_invoice( + msg: &Message, + order: &Order, + check: InvoiceCheck, +) -> Result, MostroError> { // init payment request to None let mut payment_request = None; // if payment request is present @@ -1435,15 +1461,15 @@ pub async fn validate_invoice(msg: &Message, order: &Order) -> Result is_valid_invoice_offline(pr.clone(), amount, fee).await, + InvoiceCheck::Online => is_valid_invoice(pr.clone(), amount, fee).await, + }; + // if invoice is valid - if is_valid_invoice( - pr.clone(), - Some(order.amount as u64), - Some(total_buyer_fees as u64), - ) - .await - .is_err() - { + if outcome.is_err() { return Err(MostroCantDo(CantDoReason::InvalidInvoice)); } // if invoice is valid return it @@ -2671,6 +2697,10 @@ mod tests { assert_eq!(dispute.order_id, order_id); } + /// Every case here uses a BOLT11 payment request, which is validated + /// entirely locally — so both check modes must agree on all of them. + /// Running the table twice is what pins that equivalence down: `Offline` + /// drops the network round-trip for addresses/LNURLs and nothing else. #[tokio::test] async fn validate_invoice_paths() { init_globals(); @@ -2678,35 +2708,48 @@ mod tests { order.amount = 1_100; order.fee = 100; - // No payment request in the message → nothing to validate. - let msg = Message::new_order(None, None, None, Action::AddInvoice, None); - assert_eq!(validate_invoice(&msg, &order).await.unwrap(), None); + for check in [InvoiceCheck::Offline, InvoiceCheck::Online] { + // No payment request in the message → nothing to validate. + let msg = Message::new_order(None, None, None, Action::AddInvoice, None); + assert_eq!( + validate_invoice(&msg, &order, check).await.unwrap(), + None, + "{check:?}: an absent payment request must validate to None" + ); - // Garbage payment request → invalid invoice. - let msg = Message::new_order( - None, - None, - None, - Action::AddInvoice, - Some(Payload::PaymentRequest( + // Garbage payment request → invalid invoice. + let msg = Message::new_order( None, - "notaninvoice".to_string(), None, - )), - ); - let err = validate_invoice(&msg, &order).await.unwrap_err(); - assert!(matches!(err, MostroCantDo(CantDoReason::InvalidInvoice))); + None, + Action::AddInvoice, + Some(Payload::PaymentRequest( + None, + "notaninvoice".to_string(), + None, + )), + ); + let err = validate_invoice(&msg, &order, check).await.unwrap_err(); + assert!( + matches!(err, MostroCantDo(CantDoReason::InvalidInvoice)), + "{check:?}: garbage must be rejected, got {err:?}" + ); - // Freshly-built invoice for amount - fee = 1000 sats validates. - let pr = build_test_invoice(1_000_000, 86_400); - let msg = Message::new_order( - None, - None, - None, - Action::AddInvoice, - Some(Payload::PaymentRequest(None, pr.clone(), None)), - ); - assert_eq!(validate_invoice(&msg, &order).await.unwrap(), Some(pr)); + // Freshly-built invoice for amount - fee = 1000 sats validates. + let pr = build_test_invoice(1_000_000, 86_400); + let msg = Message::new_order( + None, + None, + None, + Action::AddInvoice, + Some(Payload::PaymentRequest(None, pr.clone(), None)), + ); + assert_eq!( + validate_invoice(&msg, &order, check).await.unwrap(), + Some(pr), + "{check:?}: a matching BOLT11 invoice must validate" + ); + } } // ───────────────────────── taker reputation notification ─────────────────────────