diff --git a/rust/api/src/errors.rs b/rust/api/src/errors.rs index 3b80c608..39ccc5b4 100644 --- a/rust/api/src/errors.rs +++ b/rust/api/src/errors.rs @@ -7,6 +7,9 @@ pub enum CertificateError { PaymentNotSuccessful, PaymentMethodMissing, CertificateAlreadySigned, + /// The request's PaymentIntent id is not well-formed, or the PaymentIntent + /// Stripe returned is not the one requested. + InvalidPaymentIntentId, Base64Error(base64::DecodeError), KeyError(String), ParseIdError(stripe::ParseIdError), @@ -21,6 +24,7 @@ impl std::fmt::Display for CertificateError { CertificateError::PaymentNotSuccessful => write!(f, "Payment not successful"), CertificateError::PaymentMethodMissing => write!(f, "Payment method is missing"), CertificateError::CertificateAlreadySigned => write!(f, "Certificate already signed"), + CertificateError::InvalidPaymentIntentId => write!(f, "Invalid payment reference"), CertificateError::Base64Error(e) => write!(f, "Base64 decoding error: {}", e), CertificateError::KeyError(e) => write!(f, "Key error: {}", e), CertificateError::ParseIdError(e) => write!(f, "Parse ID error: {}", e), diff --git a/rust/api/src/handle_sign_cert.rs b/rust/api/src/handle_sign_cert.rs index 653d9ea4..a9d61717 100644 --- a/rust/api/src/handle_sign_cert.rs +++ b/rust/api/src/handle_sign_cert.rs @@ -42,6 +42,13 @@ pub async fn sign_certificate( log::debug!("Current working directory: {:?}", std::env::current_dir()); log::debug!("HOME environment variable: {:?}", std::env::var("HOME")); + // Accept only well-formed PaymentIntent ids. Checked first, before any + // other work is done for the request. + if !crate::payment_claim::is_payment_intent_id(&request.payment_intent_id) { + log::warn!("Rejected sign-certificate request with a malformed PaymentIntent id"); + return Err(CertificateError::InvalidPaymentIntentId); + } + let stripe_secret_key = std::env::var("STRIPE_SECRET_KEY").map_err(|e| { log::error!("Environment variable STRIPE_SECRET_KEY not found: {}", e); log::error!( @@ -78,6 +85,15 @@ pub async fn sign_certificate( log::info!("Retrieved PaymentIntent: {:?}", pi); log::info!("PaymentIntent status: {:?}", pi.status); + // Require the retrieved PaymentIntent to be the one requested. + if pi.id.as_str() != request.payment_intent_id { + log::error!( + "Retrieved PaymentIntent {} does not match the requested id", + pi.id + ); + return Err(CertificateError::InvalidPaymentIntentId); + } + match pi.status { PaymentIntentStatus::Succeeded => { // Proceed with certificate signing @@ -263,6 +279,73 @@ mod tests { ); } + /// The id format check has to run before the claim and before the first + /// Stripe call, not somewhere later in the function. + #[test] + fn payment_intent_id_is_checked_before_the_claim_and_stripe() { + let source = production_source(); + + let check_at = source + .find(&squeeze( + "payment_claim::is_payment_intent_id(&request.payment_intent_id)", + )) + .expect("sign_certificate no longer checks the PaymentIntent id format"); + let claim_at = source + .find(&squeeze("payment_claim::claim(&request.payment_intent_id)")) + .expect("sign_certificate no longer claims the PaymentIntent at all"); + let retrieve_at = source + .find(&squeeze("PaymentIntent::retrieve(")) + .expect("the PaymentIntent retrieval has moved or been renamed"); + + assert!( + check_at < claim_at, + "the PaymentIntent id format must be checked before the claim is taken" + ); + assert!( + check_at < retrieve_at, + "the PaymentIntent id format must be checked before calling Stripe" + ); + } + + /// The retrieved PaymentIntent must be compared with the requested id + /// before the PaymentIntent is marked as spent. + #[test] + fn retrieved_payment_intent_is_matched_before_it_is_marked() { + let source = production_source(); + + let match_at = source + .find(&squeeze("if pi.id.as_str() != request.payment_intent_id {")) + .expect("sign_certificate no longer checks the retrieved PaymentIntent id"); + let mark_at = source + .find(&squeeze( + r#"metadata.insert("certificate_signed".to_string(), "true".to_string());"#, + )) + .expect("the certificate_signed mark has moved or been renamed"); + + assert!( + match_at < mark_at, + "the retrieved PaymentIntent must be matched against the requested \ + id before it is marked as spent" + ); + } + + /// Malformed ids are refused without needing Stripe at all: the check runs + /// before STRIPE_SECRET_KEY is even read, so this stays hermetic. + #[tokio::test] + async fn malformed_payment_intent_ids_are_refused() { + for id in ["", "pi_", "pi_abc-def", "pi_abc def", "cus_123"] { + let result = super::sign_certificate(super::SignCertificateRequest { + payment_intent_id: id.to_string(), + blinded_ghost_key_base64: String::new(), + }) + .await; + assert!( + matches!(result, Err(super::CertificateError::InvalidPaymentIntentId)), + "{id:?} was not refused as an invalid payment reference: {result:?}" + ); + } + } + /// A signing failure after the mark is set must clear it, or the donor is /// charged and permanently unable to retry. #[test] diff --git a/rust/api/src/payment_claim.rs b/rust/api/src/payment_claim.rs index 51ec6bcd..dd62741e 100644 --- a/rust/api/src/payment_claim.rs +++ b/rust/api/src/payment_claim.rs @@ -27,64 +27,131 @@ use std::sync::{Arc, LazyLock, Mutex}; use tokio::sync::{Mutex as AsyncMutex, OwnedMutexGuard}; +/// One PaymentIntent's lock plus the number of requests that currently hold +/// or are waiting for it. +struct ClaimEntry { + lock: Arc>, + /// Requests registered against this entry: the holder plus every queued + /// waiter. Only ever read or written under the `CLAIM_LOCKS` lock. + claimants: usize, +} + /// Live locks, keyed by PaymentIntent id. /// -/// Entries are removed when the last guard for a key is dropped (see -/// `ClaimGuard::drop`), so the map is bounded by the number of in-flight -/// requests rather than by the number of PaymentIntents ever seen. That -/// bound is the point: the lock is taken before the PaymentIntent is known to -/// exist, so without cleanup an unauthenticated caller could grow this map -/// without limit by posting garbage ids. -static CLAIM_LOCKS: LazyLock>>>> = +/// Entries are removed when their last claimant goes away, whether it held +/// the lock or was cancelled while still waiting for it (see `Registration`), +/// so the map is bounded by the number of in-flight requests rather than by +/// the number of PaymentIntents ever seen. That bound is the point: the lock +/// is taken before the PaymentIntent is known to exist, so without cleanup an +/// unauthenticated caller could grow this map without limit by posting +/// garbage ids. +/// +/// The claimant count is kept explicitly rather than inferred from +/// `Arc::strong_count` on the lock. tokio's `OwnedMutexGuard` releases the +/// mutex before it drops its own `Arc`, so in that gap a waiter on another +/// thread can acquire, finish and look at the count while the previous +/// holder's reference is still live. Each side then sees someone else still +/// using the entry, nobody removes it, and it leaks for good. +static CLAIM_LOCKS: LazyLock>> = LazyLock::new(|| Mutex::new(HashMap::new())); -/// Exclusive claim on one PaymentIntent, held for as long as the guard lives. -pub(crate) struct ClaimGuard { +/// Locks the claim map, recovering from poison. +/// +/// A poisoned map lock only means some other thread panicked while holding +/// it. Every critical section below leaves the map structurally sound at each +/// step, and refusing to clean up would leak, so recover rather than +/// propagate. +fn claim_locks() -> std::sync::MutexGuard<'static, HashMap> { + CLAIM_LOCKS.lock().unwrap_or_else(|e| e.into_inner()) +} + +/// One request's registration as a claimant of a PaymentIntent's entry. +/// +/// Created in `claim` in the same critical section that increments the +/// count, before anything is awaited, and withdrawn on drop. Because it is an +/// ordinary local until the lock is acquired, a request whose `claim` future +/// is dropped while still queued (axum drops the handler future when the +/// client disconnects) still withdraws, and so cannot leak the entry. +struct Registration { payment_intent_id: String, - // Dropped after `Drop::drop` runs, which is what makes the strong_count - // arithmetic there work out. - _guard: OwnedMutexGuard<()>, } -impl Drop for ClaimGuard { +impl Drop for Registration { fn drop(&mut self) { - // A poisoned map lock only means some other thread panicked while - // holding it; the map itself is still structurally sound, and refusing - // to clean up would leak. Recover rather than propagate. - let mut map = CLAIM_LOCKS.lock().unwrap_or_else(|e| e.into_inner()); - - if let Some(lock) = map.get(&self.payment_intent_id) { - // Two references means the map and this guard, so nobody else is - // holding or waiting and the entry can go. Three or more means a - // waiter has already cloned the Arc and must keep contending on - // this same mutex, so leave it in place. - if Arc::strong_count(lock) == 2 { - map.remove(&self.payment_intent_id); - } + let mut map = claim_locks(); + let Some(entry) = map.get_mut(&self.payment_intent_id) else { + // Unreachable: an entry is only removed when its count reaches + // zero, and this registration is still counted. Logged as well as + // asserted so a release build reports the broken invariant rather + // than silently carrying on. + log::error!( + "payment claim entry missing for a live registration ({})", + self.payment_intent_id + ); + debug_assert!(false, "claim entry missing for a live registration"); + return; + }; + entry.claimants -= 1; + if entry.claimants == 0 { + map.remove(&self.payment_intent_id); } } } +/// Exclusive claim on one PaymentIntent, held for as long as the guard lives. +pub(crate) struct ClaimGuard { + // Fields drop in declaration order, so the mutex is released before the + // registration is withdrawn, and an entry only disappears once its mutex + // is free. The other order would not break exclusion, since by the time a + // guard is dropped its holder's critical section is over, but a newcomer + // could then get a fresh mutex for the same PaymentIntent while the old + // holder was still running its destructor. This order is the tidy one. + _guard: OwnedMutexGuard<()>, + _registration: Registration, +} + /// Wait until no other in-process request is signing against this /// PaymentIntent, then take the claim. pub(crate) async fn claim(payment_intent_id: &str) -> ClaimGuard { - let lock = { - let mut map = CLAIM_LOCKS.lock().unwrap_or_else(|e| e.into_inner()); - map.entry(payment_intent_id.to_string()) - .or_insert_with(|| Arc::new(AsyncMutex::new(()))) - .clone() + let payment_intent_id = payment_intent_id.to_string(); + let (lock, registration) = { + let mut map = claim_locks(); + let entry = map + .entry(payment_intent_id.clone()) + .or_insert_with(|| ClaimEntry { + lock: Arc::new(AsyncMutex::new(())), + claimants: 0, + }); + entry.claimants += 1; + (Arc::clone(&entry.lock), Registration { payment_intent_id }) }; // Awaited with the map lock released, so a slow claim on one PaymentIntent - // never blocks claims on others. + // never blocks claims on others. If this future is dropped here, + // `registration` is dropped with it and the count comes back down. let guard = lock.lock_owned().await; ClaimGuard { - payment_intent_id: payment_intent_id.to_string(), _guard: guard, + _registration: registration, } } +/// Stripe's documented maximum length for an object id, in bytes. +const MAX_PAYMENT_INTENT_ID_LEN: usize = 255; + +/// Whether `id` is a well-formed PaymentIntent id: `pi_` followed by one or +/// more ASCII alphanumerics, and no longer than Stripe's 255-byte id limit. +/// +/// Callers check this before taking a claim or making any Stripe call, so +/// that only well-formed PaymentIntent ids are ever accepted. +pub(crate) fn is_payment_intent_id(id: &str) -> bool { + id.len() <= MAX_PAYMENT_INTENT_ID_LEN + && id + .strip_prefix("pi_") + .is_some_and(|rest| !rest.is_empty() && rest.bytes().all(|b| b.is_ascii_alphanumeric())) +} + /// Whether a specific PaymentIntent currently has a live entry. /// /// Tests assert on individual keys rather than on the size of the map: @@ -93,18 +160,65 @@ pub(crate) async fn claim(payment_intent_id: &str) -> ClaimGuard { /// what every other test in this module happens to be doing at that instant. #[cfg(test)] fn is_tracked(payment_intent_id: &str) -> bool { - CLAIM_LOCKS - .lock() - .unwrap_or_else(|e| e.into_inner()) - .contains_key(payment_intent_id) + claim_locks().contains_key(payment_intent_id) +} + +/// How many requests are registered against a PaymentIntent: its holder plus +/// every queued waiter, or 0 if it has no entry. +#[cfg(test)] +fn claimants(payment_intent_id: &str) -> usize { + claim_locks() + .get(payment_intent_id) + .map_or(0, |entry| entry.claimants) } #[cfg(test)] mod tests { + use std::future::Future; use std::sync::atomic::{AtomicUsize, Ordering}; use super::*; + #[test] + fn well_formed_payment_intent_ids_are_accepted() { + for id in ["pi_1", "pi_3PqRsTuVwXyZ0123456789ab", "pi_ABCdef123"] { + assert!(is_payment_intent_id(id), "{id:?} should be accepted"); + } + } + + #[test] + fn malformed_payment_intent_ids_are_rejected() { + for id in [ + "", + "pi_", + "pi_abc-def", + "pi_abc def", + "pi_abc_def", + "cus_123", + "PI_123", + "pi123", + ] { + assert!(!is_payment_intent_id(id), "{id:?} should be rejected"); + } + } + + #[test] + fn payment_intent_ids_are_capped_at_255_bytes() { + let longest = format!("pi_{}", "a".repeat(252)); + assert_eq!(longest.len(), 255); + assert!( + is_payment_intent_id(&longest), + "a 255-byte id should be accepted" + ); + + let too_long = format!("pi_{}", "a".repeat(253)); + assert_eq!(too_long.len(), 256); + assert!( + !is_payment_intent_id(&too_long), + "a 256-byte id should be rejected" + ); + } + /// The property the whole module exists for: two concurrent claims on one /// PaymentIntent never overlap. Without the lock both tasks observe /// `inside == 0`, both proceed, and the peak is 2. @@ -204,8 +318,16 @@ mod tests { is_tracked("pi_handoff") }); - // Give the waiter time to queue behind the held claim. - tokio::time::sleep(std::time::Duration::from_millis(50)).await; + // Wait until the waiter has actually registered behind the held claim + // rather than sleeping and hoping it has. The bound only exists to + // turn a lost waiter into a failure instead of a hang. + tokio::time::timeout(std::time::Duration::from_secs(10), async { + while claimants("pi_handoff") < 2 { + tokio::task::yield_now().await; + } + }) + .await + .expect("the waiter never registered behind the held claim"); assert!( is_tracked("pi_handoff"), "entry vanished while a waiter was queued, so the waiter is now \ @@ -220,4 +342,65 @@ mod tests { "entry outlived the last guard for this key" ); } + + /// axum drops a handler's future when the client disconnects, which can + /// happen while the request is still queued behind another claim. The + /// abandoned waiter never gets a guard, so it has to withdraw on its own + /// or its key is never reclaimed. + #[tokio::test] + async fn cancelled_waiter_does_not_leak_its_key() { + let held = claim("pi_cancelled_waiter").await; + + // The first poll registers the waiter and then parks it behind + // `held`; the timeout then drops the still-queued future. + let abandoned = tokio::time::timeout( + std::time::Duration::from_millis(20), + claim("pi_cancelled_waiter"), + ) + .await; + assert!( + abandoned.is_err(), + "the waiter acquired a claim that was still held" + ); + assert!( + is_tracked("pi_cancelled_waiter"), + "entry vanished while its holder was still live" + ); + + drop(held); + assert!( + !is_tracked("pi_cancelled_waiter"), + "a waiter cancelled while queued stranded its entry after the \ + holder released, so abandoned requests grow the map without bound" + ); + + // The key is still usable afterwards. + drop(claim("pi_cancelled_waiter").await); + assert!(!is_tracked("pi_cancelled_waiter")); + } + + /// The same, for a waiter cancelled after the holder has already gone and + /// before it was ever polled again: it must still be the one to clean up. + #[tokio::test] + async fn waiter_cancelled_after_holder_released_does_not_leak() { + let held = claim("pi_cancelled_late").await; + + let mut waiter = Box::pin(claim("pi_cancelled_late")); + // Poll once so it registers and queues, then release the holder + // without ever polling the waiter again. + let first_poll = + std::future::poll_fn(|cx| std::task::Poll::Ready(waiter.as_mut().poll(cx))).await; + assert!(first_poll.is_pending()); + drop(held); + assert!( + is_tracked("pi_cancelled_late"), + "entry vanished while a waiter was still registered" + ); + + drop(waiter); + assert!( + !is_tracked("pi_cancelled_late"), + "a waiter cancelled after the holder released stranded its entry" + ); + } } diff --git a/rust/api/src/routes.rs b/rust/api/src/routes.rs index 8c107143..7ece1fac 100644 --- a/rust/api/src/routes.rs +++ b/rust/api/src/routes.rs @@ -24,6 +24,7 @@ use crate::handle_sign_cert::{ }; use crate::invite; use crate::invite_pow::{PowChallenge, PowChallengeResponse, PowError, PowManager}; +use crate::payment_claim::is_payment_intent_id; use crate::rate_limit::{ AggregateBucket, RateLimiter, DEFAULT_GLOBAL_INVITES_PER_HOUR, GLOBAL_WINDOW_MINUTES, MAX_INVITES_PER_WINDOW, @@ -151,6 +152,12 @@ async fn sign_certificate_route( status: StatusCode::BAD_REQUEST.as_u16(), }))) }, + CertificateError::InvalidPaymentIntentId => { + Err((StatusCode::BAD_REQUEST, Json(ErrorResponse { + error: "Invalid payment reference".to_string(), + status: StatusCode::BAD_REQUEST.as_u16(), + }))) + }, CertificateError::CertificateAlreadySigned => { Err((StatusCode::CONFLICT, Json(ErrorResponse { error: "Certificate has already been signed for this payment.".to_string(), @@ -176,7 +183,8 @@ async fn sign_certificate_route( #[derive(Debug)] pub enum DonationError { - InvalidCurrency, + /// The request's PaymentIntent id is not well-formed. + InvalidPaymentIntentId, StripeError(stripe::StripeError), EnvError(std::env::VarError), OtherError(String), @@ -185,7 +193,9 @@ pub enum DonationError { impl IntoResponse for DonationError { fn into_response(self) -> axum::response::Response { let (status, error_message) = match self { - DonationError::InvalidCurrency => (StatusCode::BAD_REQUEST, "Invalid currency"), + DonationError::InvalidPaymentIntentId => { + (StatusCode::BAD_REQUEST, "Invalid payment reference") + } DonationError::StripeError(e) => { error!("Stripe error: {:?}", e); (StatusCode::INTERNAL_SERVER_ERROR, "Stripe error occurred") @@ -310,11 +320,16 @@ async fn update_donation( ) -> Result, DonationError> { info!("Received update-donation request: {:?}", request); + // Accept only well-formed PaymentIntent ids, before any Stripe call. + if !is_payment_intent_id(&request.payment_intent_id) { + return Err(DonationError::InvalidPaymentIntentId); + } + let secret_key = std::env::var("STRIPE_SECRET_KEY").map_err(DonationError::EnvError)?; let client = Client::new(&secret_key); let payment_intent_id = PaymentIntentId::from_str(&request.payment_intent_id) - .map_err(|_| DonationError::InvalidCurrency)?; + .map_err(|_| DonationError::InvalidPaymentIntentId)?; let params = stripe::UpdatePaymentIntent { amount: Some(request.amount), ..Default::default() @@ -351,11 +366,16 @@ async fn check_payment_status_route( payment_intent_id ); + // Accept only well-formed PaymentIntent ids, before any Stripe call. + if !is_payment_intent_id(&payment_intent_id) { + return Err(DonationError::InvalidPaymentIntentId); + } + let secret_key = std::env::var("STRIPE_SECRET_KEY").map_err(DonationError::EnvError)?; let client = Client::new(&secret_key); let payment_intent_id = PaymentIntentId::from_str(&payment_intent_id) - .map_err(|_| DonationError::InvalidCurrency)?; + .map_err(|_| DonationError::InvalidPaymentIntentId)?; let intent = stripe::PaymentIntent::retrieve(&client, &payment_intent_id, &[]) .await @@ -1111,3 +1131,78 @@ mod invite_handler_tests { assert_eq!(state.global_bucket.current(), 1); } } + +#[cfg(test)] +mod payment_intent_id_route_tests { + use super::*; + + const MALFORMED: [&str; 5] = ["", "pi_", "pi_abc-def", "pi_abc def", "cus_123"]; + + /// The status and `error` message a DonationError is sent to the client as. + async fn rendered(e: DonationError) -> (StatusCode, String) { + let response = e.into_response(); + let status = response.status(); + let bytes = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap(); + let body: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); + (status, body["error"].as_str().unwrap().to_string()) + } + + /// The format check runs before STRIPE_SECRET_KEY is read, so a malformed + /// id is a 400 here without Stripe being configured or contacted. + #[tokio::test] + async fn update_donation_refuses_malformed_ids() { + for id in MALFORMED { + let result = update_donation(Json(UpdateDonationRequest { + payment_intent_id: id.to_string(), + amount: 100, + })) + .await; + let error = match result { + Ok(_) => panic!("{id:?} was accepted by update-donation"), + Err(e) => rendered(e).await, + }; + assert_eq!( + error, + ( + StatusCode::BAD_REQUEST, + "Invalid payment reference".to_string() + ), + "{id:?}" + ); + } + } + + #[tokio::test] + async fn check_payment_status_refuses_malformed_ids() { + for id in MALFORMED { + let error = match check_payment_status_route(Path(id.to_string())).await { + Ok(_) => panic!("{id:?} was accepted by check-payment-status"), + Err(e) => rendered(e).await, + }; + assert_eq!( + error, + ( + StatusCode::BAD_REQUEST, + "Invalid payment reference".to_string() + ), + "{id:?}" + ); + } + } + + #[tokio::test] + async fn sign_certificate_route_maps_malformed_ids_to_400() { + let request: SignCertificateRequest = serde_json::from_value(serde_json::json!({ + "payment_intent_id": "pi_abc-def", + "blinded_ghost_key_base64": "", + })) + .unwrap(); + let (status, Json(body)) = sign_certificate_route(Json(request)) + .await + .expect_err("a malformed id was accepted"); + assert_eq!(status, StatusCode::BAD_REQUEST); + assert_eq!(body.error, "Invalid payment reference"); + } +}