diff --git a/src/app/add_invoice.rs b/src/app/add_invoice.rs index 6cac52f1..e1db0b2d 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::*; @@ -51,7 +51,12 @@ pub async fn add_invoice_action( return Err(MostroCantDo(CantDoReason::InvalidPeer)); } // 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 => { diff --git a/src/app/order.rs b/src/app/order.rs index 73b3782c..beb4d36f 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,19 @@ 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. Creating an order only records the payment + // request — the result here is discarded — and the address still has + // to resolve when the payout is actually made, which is where an + // unreachable host is worth acting on. Checking it here would put an + // HTTP request to a sender-chosen host on the event loop for every + // order created, delaying every other message behind it. + 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/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 ─────────────────────────