Confirm settle with provider API before issuing; add test-injection seam
The settle-webhook honored payment on the webhook body's claim alone. Zaprite webhooks carry no signature, so a forged order.change/status=PAID POST with a buyer-visible order id minted a signed license without payment. handle_inner now re-fetches provider.get_invoice_status and requires Settled before persisting "settled" or taking any settle-derived action (issuance, tier-change, subscription renewal — the guard precedes all of them). On a provider-API error it acks 200 without issuing, so a transient outage can't trigger a webhook retry storm; the reconcile loop re-confirms and issues later. Adds the always-compiled AppState::provider_override seam (None in prod), honored by provider_from_row at every resolution site, so integration tests drive the real resolver with a MockPaymentProvider. Greens the two paid_purchase_* tests, deletes the dead payment_provider_preference_round_trip, and adds forged-settle + provider-unreachable regression tests. api 47/47. Not addressed: a literal paid-amount/currency check (needs a trait change).
This commit is contained in:
@@ -108,6 +108,15 @@ pub struct AppState {
|
||||
/// `Arc<dyn ...>` so call sites get cheap clones; swapped under a
|
||||
/// write lock when the operator runs Connect / Disconnect.
|
||||
pub payment: Arc<RwLock<Option<Arc<dyn crate::payment::PaymentProvider>>>>,
|
||||
/// Test-only injection seam. When `Some`, the merchant-profile
|
||||
/// resolver (`resolve_provider_for_profile_rail`, `payment_provider_by_id`)
|
||||
/// returns THIS provider instead of constructing a real BTCPay/Zaprite
|
||||
/// client from the DB row via `payment::build_provider`. The DB still
|
||||
/// drives profile/rail/row resolution, so that logic is exercised for
|
||||
/// real — only the network-talking impl is swapped. Always `None` in
|
||||
/// production (`main.rs`); set by integration tests so they can drive
|
||||
/// the real purchase/settle path with a `MockPaymentProvider`.
|
||||
pub provider_override: Option<Arc<dyn crate::payment::PaymentProvider>>,
|
||||
pub config: Arc<Config>,
|
||||
/// Keysat-licenses-Keysat tier. Read at boot, swapped when the
|
||||
/// operator activates a fresh license via the admin endpoint.
|
||||
@@ -199,7 +208,20 @@ impl AppState {
|
||||
.ok_or_else(|| {
|
||||
AppError::NotFound(format!("payment provider {provider_id}"))
|
||||
})?;
|
||||
crate::payment::build_provider(&row, self.config.btcpay_public_url.as_deref())
|
||||
self.provider_from_row(&row)
|
||||
}
|
||||
|
||||
/// Instantiate a `PaymentProvider` from a resolved DB row, honoring the
|
||||
/// test-only `provider_override` seam. In production `provider_override`
|
||||
/// is always `None`, so this just delegates to `payment::build_provider`.
|
||||
fn provider_from_row(
|
||||
&self,
|
||||
row: &crate::db::repo::PaymentProviderRow,
|
||||
) -> AppResult<Arc<dyn crate::payment::PaymentProvider>> {
|
||||
if let Some(p) = &self.provider_override {
|
||||
return Ok(p.clone());
|
||||
}
|
||||
crate::payment::build_provider(row, self.config.btcpay_public_url.as_deref())
|
||||
.map_err(AppError::Internal)
|
||||
}
|
||||
|
||||
@@ -241,9 +263,7 @@ impl AppState {
|
||||
pref.payment_provider_id
|
||||
))
|
||||
})?;
|
||||
let provider =
|
||||
crate::payment::build_provider(&row, self.config.btcpay_public_url.as_deref())
|
||||
.map_err(AppError::Internal)?;
|
||||
let provider = self.provider_from_row(&row)?;
|
||||
return Ok((row, provider));
|
||||
}
|
||||
|
||||
@@ -271,9 +291,7 @@ impl AppState {
|
||||
))),
|
||||
[only] => {
|
||||
let row = (*only).clone();
|
||||
let provider =
|
||||
crate::payment::build_provider(&row, self.config.btcpay_public_url.as_deref())
|
||||
.map_err(AppError::Internal)?;
|
||||
let provider = self.provider_from_row(&row)?;
|
||||
Ok((row, provider))
|
||||
}
|
||||
[first, ..] => {
|
||||
@@ -287,9 +305,7 @@ impl AppState {
|
||||
this warning."
|
||||
);
|
||||
let row = (*first).clone();
|
||||
let provider =
|
||||
crate::payment::build_provider(&row, self.config.btcpay_public_url.as_deref())
|
||||
.map_err(AppError::Internal)?;
|
||||
let provider = self.provider_from_row(&row)?;
|
||||
Ok((row, provider))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -117,6 +117,50 @@ async fn handle_inner(
|
||||
"webhook event applied"
|
||||
);
|
||||
|
||||
// Anti-forgery: never settle on the webhook body's claim alone. Re-fetch
|
||||
// the authoritative status from the provider's own API and require it to
|
||||
// actually be Settled before we mark the invoice paid or take ANY
|
||||
// settle-derived action. This guard runs ahead of every downstream effect
|
||||
// — status persistence, tier-change application, subscription renewal, and
|
||||
// license issuance — so confirming once here gates all of them.
|
||||
// This is load-bearing for providers without webhook signatures: Zaprite
|
||||
// webhooks carry no HMAC, so a forged `order.change`/`status=PAID` POST
|
||||
// with a buyer-visible order id would otherwise mint a free license. The
|
||||
// re-fetch also defeats replay of a stale settled body against an invoice
|
||||
// that has since expired/refunded (the provider reports the current state,
|
||||
// not the replayed one). BTCPay is HMAC-verified upstream and is settled
|
||||
// already, so this is cheap belt-and-suspenders there. On a provider
|
||||
// error we fail closed — the reconcile loop re-confirms on its next tick.
|
||||
if new_status == "settled" {
|
||||
match provider.get_invoice_status(&provider_invoice_id).await {
|
||||
Ok(crate::payment::ProviderInvoiceStatus::Settled) => {}
|
||||
Ok(other) => {
|
||||
tracing::warn!(
|
||||
provider = provider.kind().as_str(),
|
||||
provider_invoice_id = %provider_invoice_id,
|
||||
provider_status = ?other,
|
||||
"settle webhook NOT confirmed by provider API; refusing to settle/issue"
|
||||
);
|
||||
return Ok(StatusCode::OK);
|
||||
}
|
||||
Err(e) => {
|
||||
// Ack 200 rather than erroring: a non-2xx makes BTCPay/Zaprite
|
||||
// re-deliver aggressively, so a transient provider-API outage
|
||||
// would turn every in-flight webhook into a retry storm. We
|
||||
// simply don't issue now — the reconcile loop re-fetches the
|
||||
// status on its next tick and issues then, so issuance is still
|
||||
// "fail closed" without depending on this delivery.
|
||||
tracing::warn!(
|
||||
provider = provider.kind().as_str(),
|
||||
provider_invoice_id = %provider_invoice_id,
|
||||
error = format!("{e:#}"),
|
||||
"could not reach provider to confirm settle; not issuing now, deferring to reconciler"
|
||||
);
|
||||
return Ok(StatusCode::OK);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Persist status.
|
||||
repo::update_invoice_status(&state.db, &provider_invoice_id, new_status).await?;
|
||||
|
||||
|
||||
@@ -128,6 +128,7 @@ async fn main() -> anyhow::Result<()> {
|
||||
db: pool,
|
||||
keypair: Arc::new(keypair),
|
||||
payment: Arc::new(tokio::sync::RwLock::new(provider)),
|
||||
provider_override: None,
|
||||
config: Arc::new(cfg.clone()),
|
||||
self_tier,
|
||||
rates: keysat::rates::RateCache::new(),
|
||||
|
||||
Reference in New Issue
Block a user