diff --git a/src/app/admin_cancel.rs b/src/app/admin_cancel.rs index 9ce21ef4..e875bcf2 100644 --- a/src/app/admin_cancel.rs +++ b/src/app/admin_cancel.rs @@ -42,9 +42,16 @@ use tracing::{error, info}; /// Returns `MostroError` if: /// - Solver is not assigned to the dispute /// - Order/dispute not found +/// - The dispute initiator flags are unset or ambiguous, or a counterparty +/// pubkey is missing/unparseable — both are checked before the refund, so +/// they leave the escrow and the order untouched (#805) /// - Lightning invoice cancellation fails /// - Database update fails /// - Nostr publish fails +/// +/// Failures *after* the refund are logged and swallowed rather than +/// returned (DM fan-out, bond resolution), so the handler always runs to +/// the end once the escrow has moved. Making that tail atomic is #810. pub async fn admin_cancel_action( ctx: &AppContext, msg: Message, @@ -115,6 +122,51 @@ pub async fn admin_cancel_action( let bond_resolution = bond::extract_bond_resolution(&msg); bond::validate_bond_resolution(pool, &order, &bond_resolution).await?; + // Resolve the dispute initiator *before* the escrow is touched (#805). + // This match rejects orders whose initiator flags are unset or ambiguous, + // and `cancel_hold_invoice` below is irreversible: running it first meant + // a rejected request could still refund the seller while leaving the order + // in `Dispute`, so the LN side and the DB disagreed with no way back. + // This initiator check now precedes the refund, as does the counterparty + // resolution below. What remains after the refund either can't reject the + // call (the DM fan-out is best effort) or is a DB/event write whose + // atomicity is tracked separately in #810. + let dispute_initiator = match (order.seller_dispute, order.buyer_dispute) { + (true, false) => "seller", + (false, true) => "buyer", + (seller_dispute, buyer_dispute) => { + // A bare `DisputeEventError` doesn't tell the operator why the + // cancel bounces. The pair is only reachable through a corrupted + // row — `dispute_action` gates on `Active`/`FiatSent`, so exactly + // one flag is set by the time an order reaches `Dispute`. + error!( + order_id = %order.id, + seller_dispute, + buyer_dispute, + "admin_cancel: ambiguous dispute initiator flags; refusing before the escrow is touched" + ); + return Err(MostroInternalErr(ServiceError::DisputeEventError)); + } + }; + + // Same reasoning as the initiator check above: resolving the + // counterparties is pure validation over the `order` snapshot, so it + // belongs before the irreversible refund rather than after it. Left + // where it was, an unparseable or missing pubkey aborted the handler + // once the escrow was already refunded and the order already + // `CanceledByAdmin` — past the point where a solver retry is possible + // (the `Dispute` guard above rejects it) and before the bond resolution + // below, stranding every `Locked` bond with no reconciler. + let (seller_pubkey, buyer_pubkey) = match (&order.seller_pubkey, &order.buyer_pubkey) { + (Some(seller), Some(buyer)) => ( + PublicKey::from_str(seller.as_str()) + .map_err(|_| MostroInternalErr(ServiceError::InvalidPubkey))?, + PublicKey::from_str(buyer.as_str()) + .map_err(|_| MostroInternalErr(ServiceError::InvalidPubkey))?, + ), + (None, _) | (_, None) => return Err(MostroInternalErr(ServiceError::InvalidPubkey)), + }; + if order.hash.is_some() { // We return funds to seller if let Some(hash) = order.hash.as_ref() { @@ -126,13 +178,6 @@ pub async fn admin_cancel_action( // we check if there is a dispute let dispute = find_dispute_by_order_id(pool, order.id).await; - // Get the creator of the dispute - let dispute_initiator = match (order.seller_dispute, order.buyer_dispute) { - (true, false) => "seller", - (false, true) => "buyer", - (_, _) => return Err(MostroInternalErr(ServiceError::DisputeEventError)), - }; - if let Ok(mut d) = dispute { let dispute_id = d.id; // we update the dispute @@ -194,27 +239,24 @@ pub async fn admin_cancel_action( let message = message .as_json() .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; - // Message to admin - send_dm(event.sender, my_keys, &message, None) - .await - .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; - - let (seller_pubkey, buyer_pubkey) = match (&order.seller_pubkey, &order.buyer_pubkey) { - (Some(seller), Some(buyer)) => ( - PublicKey::from_str(seller.as_str()) - .map_err(|_| MostroInternalErr(ServiceError::InvalidPubkey))?, - PublicKey::from_str(buyer.as_str()) - .map_err(|_| MostroInternalErr(ServiceError::InvalidPubkey))?, - ), - (None, _) => return Err(MostroInternalErr(ServiceError::InvalidPubkey)), - (_, None) => return Err(MostroInternalErr(ServiceError::InvalidPubkey)), - }; - send_dm(seller_pubkey, my_keys, &message, None) - .await - .map_err(|e| MostroInternalErr(ServiceError::NostrError(e.to_string())))?; - send_dm(buyer_pubkey, my_keys, &message, None) - .await - .map_err(|e| MostroInternalErr(ServiceError::NostrError(e.to_string())))?; + // The escrow is already refunded and the order already `CanceledByAdmin` + // by this point, so a relay hiccup on a notification must not abort the + // handler: an early return here skips the bond resolution below and + // strands every `Locked` bond with no retry path (a second admin cancel + // is rejected by the `Dispute` guard, and only range maker bonds have a + // reconciler). Delivery is best effort, mirroring `notify_bond_slashed`. + for (role, destination) in [ + ("admin", event.sender), + ("seller", seller_pubkey), + ("buyer", buyer_pubkey), + ] { + if let Err(e) = send_dm(destination, my_keys, &message, None).await { + error!( + order_id = %order.id, + "admin_cancel: failed to notify the {role} of the cancellation: {e}" + ); + } + } // Phase 2: apply the solver's `BondResolution` to the bond rows // (release-by-default when absent). The buyer/seller pubkeys on @@ -527,9 +569,11 @@ mod tests { )); } - /// A dispute order with no `hash` skips the LND cancel and, when - /// neither `seller_dispute` nor `buyer_dispute` is set, fails the - /// dispute-initiator resolution with `DisputeEventError`. + /// With neither `seller_dispute` nor `buyer_dispute` set, the + /// dispute-initiator resolution fails with `DisputeEventError`. The + /// order carries no `hash`, so this covers the resolution itself + /// independently of the escrow; the pre-refund ordering is pinned by + /// `dispute_without_initiator_flag_errors_before_refunding` below. #[tokio::test] async fn dispute_without_initiator_flag_errors() { let pool = setup_pool().await; @@ -561,6 +605,133 @@ mod tests { )); } + /// Regression for #805: an order carrying a hold-invoice `hash` whose + /// dispute-initiator flags are unset must fail validation *before* the + /// irreversible `cancel_hold_invoice`. Reaching the LND seam here would + /// surface as `LnNodeError` and would mean the seller was already + /// refunded on a request that goes on to be rejected. + /// + /// The discriminator is the error *type*, which depends on `dead_lnd` + /// making every RPC fail: if the LND seam is ever replaced by a mock + /// that returns `Ok`, the pre-fix ordering would also end in + /// `DisputeEventError` and this test would stop guarding the invariant. + /// Assert on a cancel-call spy instead if that day comes. + #[tokio::test] + async fn dispute_without_initiator_flag_errors_before_refunding() { + let pool = setup_pool().await; + let ctx = build_ctx(pool.clone()); + let mut ln = dead_lnd().await; + let admin = Keys::generate(); + let seller = Keys::generate().public_key(); + let buyer = Keys::generate().public_key(); + + let mut order = dispute_order(seller, buyer); + // Hold invoice present, but neither side is flagged as the dispute + // initiator, so `dispute_initiator` resolution must reject the call. + order.hash = Some("11".repeat(32)); + let order = order.create(ctx.pool()).await.unwrap(); + assign_solver(ctx.pool(), order.id, &admin.public_key()).await; + + let result = admin_cancel_action( + &ctx, + cancel_msg(order.id), + &admin_event(admin.public_key()), + &admin, + &mut ln, + ) + .await; + + assert!( + matches!( + result, + Err(MostroInternalErr(ServiceError::DisputeEventError)) + ), + "expected the initiator check to reject before the LND cancel, got {result:?}" + ); + + // The order must be left untouched so the solver can retry. + let stored = get_order(&cancel_msg(order.id), ctx.pool()).await.unwrap(); + assert_eq!(stored.status, Status::Dispute.to_string()); + } + + /// Both initiator flags set is as ambiguous as neither: the same arm + /// rejects it before the escrow is touched, so a corrupted row can't + /// pick an arbitrary side for the kind-38386 `initiator` tag. + #[tokio::test] + async fn dispute_with_both_initiator_flags_errors_before_refunding() { + let pool = setup_pool().await; + let ctx = build_ctx(pool.clone()); + let mut ln = dead_lnd().await; + let admin = Keys::generate(); + let seller = Keys::generate().public_key(); + let buyer = Keys::generate().public_key(); + + let mut order = dispute_order(seller, buyer); + order.seller_dispute = true; + order.buyer_dispute = true; + order.hash = Some("11".repeat(32)); + let order = order.create(ctx.pool()).await.unwrap(); + assign_solver(ctx.pool(), order.id, &admin.public_key()).await; + + let result = admin_cancel_action( + &ctx, + cancel_msg(order.id), + &admin_event(admin.public_key()), + &admin, + &mut ln, + ) + .await; + + assert!( + matches!( + result, + Err(MostroInternalErr(ServiceError::DisputeEventError)) + ), + "expected the initiator check to reject before the LND cancel, got {result:?}" + ); + } + + /// Counterparty resolution is validation too, so a missing pubkey must + /// be rejected before the refund (#805). Pre-fix this parsed only after + /// `cancel_hold_invoice`, the dispute row and the order status had all + /// been written — a rejection that had already moved the money and left + /// the bonds `Locked` past any retry. + #[tokio::test] + async fn missing_counterparty_pubkey_errors_before_refunding() { + let pool = setup_pool().await; + let ctx = build_ctx(pool.clone()); + let mut ln = dead_lnd().await; + let admin = Keys::generate(); + let seller = Keys::generate().public_key(); + let buyer = Keys::generate().public_key(); + + let mut order = dispute_order(seller, buyer); + order.seller_dispute = true; + order.buyer_pubkey = None; + order.hash = Some("11".repeat(32)); + let order = order.create(ctx.pool()).await.unwrap(); + assign_solver(ctx.pool(), order.id, &admin.public_key()).await; + + let result = admin_cancel_action( + &ctx, + cancel_msg(order.id), + &admin_event(admin.public_key()), + &admin, + &mut ln, + ) + .await; + + assert!( + matches!(result, Err(MostroInternalErr(ServiceError::InvalidPubkey))), + "expected the pubkey check to reject before the LND cancel, got {result:?}" + ); + + // Untouched: no refund, no status change, so the row is still + // recoverable once the pubkey is repaired. + let stored = Order::by_id(ctx.pool(), order.id).await.unwrap().unwrap(); + assert_eq!(stored.status, Status::Dispute.to_string()); + } + /// A dispute order carrying a hold-invoice `hash` returns funds to the /// seller via `cancel_hold_invoice`, which fails against the dead LND /// endpoint and surfaces as `LnNodeError`. @@ -598,9 +769,10 @@ mod tests { /// Full no-LND cancel path: a seller-initiated dispute with no hold /// invoice hash. The dispute row is moved to `SellerRefunded` and the - /// order to `CanceledByAdmin` before the DM fan-out. Those DB writes are - /// deterministic; the terminal `send_dm` depends on the process-global - /// Nostr client, so the top-level result is not asserted. + /// order to `CanceledByAdmin`, and the handler runs to completion even + /// though the DM fan-out fails here (no process-global Nostr client) — + /// notifications are best effort precisely so a relay failure can't skip + /// the bond resolution that follows them. #[tokio::test] async fn seller_dispute_without_hash_refunds_and_cancels() { let pool = setup_pool().await; @@ -619,7 +791,7 @@ mod tests { .unwrap() .id; - let _ = admin_cancel_action( + let result = admin_cancel_action( &ctx, cancel_msg(order.id), &admin_event(admin.public_key()), @@ -628,6 +800,11 @@ mod tests { ) .await; + assert!( + result.is_ok(), + "a failed DM must not abort the handler before bond resolution: {result:?}" + ); + let stored_order = Order::by_id(ctx.pool(), order.id).await.unwrap().unwrap(); assert_eq!(stored_order.status, Status::CanceledByAdmin.to_string()); let stored_dispute = Dispute::by_id(ctx.pool(), dispute_id) diff --git a/src/app/admin_settle.rs b/src/app/admin_settle.rs index 5dcacca5..b02240bf 100644 --- a/src/app/admin_settle.rs +++ b/src/app/admin_settle.rs @@ -91,6 +91,28 @@ pub async fn admin_settle_action( let bond_resolution = bond::extract_bond_resolution(&msg); bond::validate_bond_resolution(pool, &order, &bond_resolution).await?; + // Resolve the dispute initiator *before* the settle (#805, same class as + // the fix applied to `admin_cancel`). `settle_seller_hold_invoice` is + // irreversible: resolving the initiator afterwards meant a rejected + // request had already moved the escrow, and the early return then also + // skipped the `AdminSettled` fan-out and the bond resolution below. + let dispute_initiator = match (order.seller_dispute, order.buyer_dispute) { + (true, false) => "seller", + (false, true) => "buyer", + (seller_dispute, buyer_dispute) => { + // Only reachable through a corrupted row — `dispute_action` + // gates on `Active`/`FiatSent`, so exactly one flag is set by + // the time an order reaches `Dispute`. + error!( + order_id = %order.id, + seller_dispute, + buyer_dispute, + "admin_settle: ambiguous dispute initiator flags; refusing before the escrow is settled" + ); + return Err(MostroInternalErr(ServiceError::DisputeEventError)); + } + }; + // Settle seller hold invoice settle_seller_hold_invoice(event, ln_client, Action::AdminSettled, true, &order) .await @@ -130,13 +152,6 @@ pub async fn admin_settle_action( .await .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; - // Get the creator of the dispute - let dispute_initiator = match (order.seller_dispute, order.buyer_dispute) { - (true, false) => "seller", - (false, true) => "buyer", - (_, _) => return Err(MostroInternalErr(ServiceError::DisputeEventError)), - }; - // We create a tag to show status of the dispute let tags: Tags = Tags::from_list(vec![ Tag::custom( @@ -538,6 +553,50 @@ mod handler_tests { )); } + /// Same invariant as `admin_cancel` (#805): with the initiator flags + /// unset, the request must be rejected *before* the irreversible + /// `settle_seller_hold_invoice`. Pre-fix the initiator was resolved + /// after the settle, so this order reached the settle seam and returned + /// `LnNodeError` — the escrow moved on a request that was then rejected, + /// skipping the `AdminSettled` fan-out and the bond resolution. + #[tokio::test] + async fn dispute_without_initiator_flag_errors_before_settling() { + let pool = setup_pool().await; + let ctx = build_ctx(pool.clone()); + let mut ln = dead_lnd().await; + let admin = Keys::generate(); + let seller = Keys::generate().public_key(); + let buyer = Keys::generate().public_key(); + + // Neither side flagged as initiator; `preimage` is left as the + // dispute-order default so the settle seam is genuinely reachable. + let order = dispute_order(seller, buyer) + .create(ctx.pool()) + .await + .unwrap(); + assign_solver(ctx.pool(), order.id, &admin.public_key()).await; + + let result = admin_settle_action( + &ctx, + settle_msg(order.id), + &admin_event(admin.public_key()), + &admin, + &mut ln, + ) + .await; + + assert!( + matches!( + result, + Err(MostroInternalErr(ServiceError::DisputeEventError)) + ), + "expected the initiator check to reject before the settle, got {result:?}" + ); + + let stored = Order::by_id(ctx.pool(), order.id).await.unwrap().unwrap(); + assert_eq!(stored.status, Status::Dispute.to_string()); + } + /// A genuine dispute settle reaches `settle_seller_hold_invoice`, which /// short-circuits on the missing preimage before any LND call and is /// mapped to `LnNodeError`. The LND settle + `do_payment` tail beyond