From 95cbf49f97ebf229ae4ad567b9bd94836a425285 Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Mon, 28 Sep 2026 20:26:34 -0500 Subject: [PATCH 1/4] fix(api): stop contended PaymentIntent claims leaking their map entry ClaimGuard::drop decided whether to remove a PaymentIntent's entry from CLAIM_LOCKS by checking Arc::strong_count(lock) == 2. tokio's OwnedMutexGuard releases the mutex before it drops its own Arc, so after the holder's Drop::drop saw a queued waiter (count 3) and kept the entry, there was a gap in which the waiter could acquire on another worker, finish, and run its own ClaimGuard::drop while the holder's Arc was still live. It also saw count 3 and kept the entry. Nobody removed it, so every contended PaymentIntent could leak one entry permanently, breaking the map's documented bound against unauthenticated garbage ids. Exclusion was not affected. This is what made entry_survives_while_a_waiter_is_queued fail intermittently in CI ("entry outlived the last guard for this key"). Track claimants explicitly instead. claim() increments a per-entry count under the map lock before awaiting and returns a Registration that decrements it (and removes the entry at zero) on drop. The registration is a plain local until the lock is acquired, so a claim future dropped while still queued (axum drops handler futures on client disconnect) withdraws too. ClaimGuard's fields are ordered so the mutex is released before the registration is withdrawn, so a newcomer can never create a fresh mutex for a key whose old one is still held. Adds tests for a waiter cancelled while queued and for one cancelled after the holder released but before it was polled again. Claude-Session: https://claude.ai/code/session_01C7ph5zmqbaZdYnnuuNjGnv --- rust/api/src/payment_claim.rs | 178 +++++++++++++++++++++++++++------- 1 file changed, 141 insertions(+), 37 deletions(-) diff --git a/rust/api/src/payment_claim.rs b/rust/api/src/payment_claim.rs index 51ec6bcd..4f090e62 100644 --- a/rust/api/src/payment_claim.rs +++ b/rust/api/src/payment_claim.rs @@ -27,61 +27,106 @@ 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. + 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 { + // Field order is load-bearing: fields drop in declaration order, so the + // mutex is released before the registration is withdrawn. The other way + // round, withdrawing the last registration would remove the entry while + // this mutex was still locked, and a newcomer could create a fresh mutex + // for the same PaymentIntent and take it before this one was released. + _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, } } @@ -93,14 +138,12 @@ 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) } #[cfg(test)] mod tests { + use std::future::Future; use std::sync::atomic::{AtomicUsize, Ordering}; use super::*; @@ -220,4 +263,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" + ); + } } From 6a7db7807a3a206cd1009f4fcb902bbbac5778b0 Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Mon, 28 Sep 2026 20:38:28 -0500 Subject: [PATCH 2/4] test(api): wait for the queued waiter deterministically in payment_claim entry_survives_while_a_waiter_is_queued slept 50ms and assumed the waiter had queued by then. It now waits, via a test-only claimant-count helper, until the waiter is actually registered behind the held claim, with a 10s bound that fails the test instead of hanging, so the test proves what its name says. Also: - Registration::drop logs an error next to its debug_assert, so a release build reports a missing entry instead of silently ignoring it. - The ClaimGuard field-order comment no longer calls the order load-bearing. With the other order a newcomer could get a fresh mutex while the old holder is only running its destructor, after its critical section is over, so this order is the tidy one rather than a safety requirement. Claude-Session: https://claude.ai/code/session_01C7ph5zmqbaZdYnnuuNjGnv --- rust/api/src/payment_claim.rs | 40 ++++++++++++++++++++++++++++------- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/rust/api/src/payment_claim.rs b/rust/api/src/payment_claim.rs index 4f090e62..d4c2b283 100644 --- a/rust/api/src/payment_claim.rs +++ b/rust/api/src/payment_claim.rs @@ -81,7 +81,13 @@ impl Drop for Registration { 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. + // 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; }; @@ -94,11 +100,12 @@ impl Drop for Registration { /// Exclusive claim on one PaymentIntent, held for as long as the guard lives. pub(crate) struct ClaimGuard { - // Field order is load-bearing: fields drop in declaration order, so the - // mutex is released before the registration is withdrawn. The other way - // round, withdrawing the last registration would remove the entry while - // this mutex was still locked, and a newcomer could create a fresh mutex - // for the same PaymentIntent and take it before this one was released. + // 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, } @@ -141,6 +148,15 @@ fn is_tracked(payment_intent_id: &str) -> bool { 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; @@ -247,8 +263,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 \ From 8341348894922d99a9d45ad4a17935407045a50b Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Mon, 28 Sep 2026 20:40:09 -0500 Subject: [PATCH 3/4] fix(api): accept only well-formed PaymentIntent ids Input hardening for the payment endpoints. - Add payment_claim::is_payment_intent_id: true iff the id is "pi_" followed by one or more ASCII alphanumerics. - sign_certificate rejects a malformed id first, before any other work or Stripe call, with a new CertificateError::InvalidPaymentIntentId that sign_certificate_route maps to 400 "Invalid payment reference". - sign_certificate requires the retrieved PaymentIntent to be the one requested before it is marked as spent. - /update-donation and /check-payment-status apply the same format check before any Stripe call and return their existing 400 error. Tests: unit tests for is_payment_intent_id; hermetic tests that each endpoint returns 400 for malformed ids without Stripe configured; and source-order pins that the format check precedes the claim and the Stripe retrieval, and that the id match precedes the mark. Claude-Session: https://claude.ai/code/session_01C7ph5zmqbaZdYnnuuNjGnv --- rust/api/src/errors.rs | 4 ++ rust/api/src/handle_sign_cert.rs | 83 ++++++++++++++++++++++++++++++++ rust/api/src/payment_claim.rs | 33 +++++++++++++ rust/api/src/routes.rs | 67 ++++++++++++++++++++++++++ 4 files changed, 187 insertions(+) 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 d4c2b283..b8c4b8a6 100644 --- a/rust/api/src/payment_claim.rs +++ b/rust/api/src/payment_claim.rs @@ -137,6 +137,16 @@ pub(crate) async fn claim(payment_intent_id: &str) -> ClaimGuard { } } +/// Whether `id` is a well-formed PaymentIntent id: `pi_` followed by one or +/// more ASCII alphanumerics. +/// +/// 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.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: @@ -164,6 +174,29 @@ mod tests { 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"); + } + } + /// 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. diff --git a/rust/api/src/routes.rs b/rust/api/src/routes.rs index 8c107143..a2e67162 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(), @@ -310,6 +317,11 @@ 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::InvalidCurrency); + } + let secret_key = std::env::var("STRIPE_SECRET_KEY").map_err(DonationError::EnvError)?; let client = Client::new(&secret_key); @@ -351,6 +363,11 @@ 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::InvalidCurrency); + } + let secret_key = std::env::var("STRIPE_SECRET_KEY").map_err(DonationError::EnvError)?; let client = Client::new(&secret_key); @@ -1111,3 +1128,53 @@ 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 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 status = match result { + Ok(_) => panic!("{id:?} was accepted by update-donation"), + Err(e) => e.into_response().status(), + }; + assert_eq!(status, StatusCode::BAD_REQUEST, "{id:?}"); + } + } + + #[tokio::test] + async fn check_payment_status_refuses_malformed_ids() { + for id in MALFORMED { + let status = match check_payment_status_route(Path(id.to_string())).await { + Ok(_) => panic!("{id:?} was accepted by check-payment-status"), + Err(e) => e.into_response().status(), + }; + assert_eq!(status, StatusCode::BAD_REQUEST, "{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"); + } +} From 780e022a5911e255abfbb20c1cf8f91f17ad97be Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Mon, 28 Sep 2026 20:50:16 -0500 Subject: [PATCH 4/4] fix(api): clearer error for malformed payment references; cap id length /update-donation and /check-payment-status answered a malformed PaymentIntent id with DonationError::InvalidCurrency ("Invalid currency"). Replace that variant, which had no other use, with DonationError::InvalidPaymentIntentId, a 400 "Invalid payment reference", matching /sign-certificate. It covers both the format check and the existing PaymentIntentId parse failure in those two routes. The hermetic route tests now assert the message as well as the status. is_payment_intent_id also rejects ids longer than 255 bytes, Stripe's documented maximum id length, with a unit test that a 255-byte id is accepted and a 256-byte one rejected. Claude-Session: https://claude.ai/code/session_01C7ph5zmqbaZdYnnuuNjGnv --- rust/api/src/payment_claim.rs | 28 +++++++++++++++++-- rust/api/src/routes.rs | 52 +++++++++++++++++++++++++++-------- 2 files changed, 65 insertions(+), 15 deletions(-) diff --git a/rust/api/src/payment_claim.rs b/rust/api/src/payment_claim.rs index b8c4b8a6..dd62741e 100644 --- a/rust/api/src/payment_claim.rs +++ b/rust/api/src/payment_claim.rs @@ -137,14 +137,19 @@ pub(crate) async fn claim(payment_intent_id: &str) -> ClaimGuard { } } +/// 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. +/// 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.strip_prefix("pi_") - .is_some_and(|rest| !rest.is_empty() && rest.bytes().all(|b| b.is_ascii_alphanumeric())) + 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. @@ -197,6 +202,23 @@ mod tests { } } + #[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. diff --git a/rust/api/src/routes.rs b/rust/api/src/routes.rs index a2e67162..7ece1fac 100644 --- a/rust/api/src/routes.rs +++ b/rust/api/src/routes.rs @@ -183,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), @@ -192,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") @@ -319,14 +322,14 @@ async fn update_donation( // Accept only well-formed PaymentIntent ids, before any Stripe call. if !is_payment_intent_id(&request.payment_intent_id) { - return Err(DonationError::InvalidCurrency); + 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() @@ -365,14 +368,14 @@ async fn check_payment_status_route( // Accept only well-formed PaymentIntent ids, before any Stripe call. if !is_payment_intent_id(&payment_intent_id) { - return Err(DonationError::InvalidCurrency); + 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 @@ -1135,6 +1138,17 @@ mod payment_intent_id_route_tests { 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] @@ -1145,22 +1159,36 @@ mod payment_intent_id_route_tests { amount: 100, })) .await; - let status = match result { + let error = match result { Ok(_) => panic!("{id:?} was accepted by update-donation"), - Err(e) => e.into_response().status(), + Err(e) => rendered(e).await, }; - assert_eq!(status, StatusCode::BAD_REQUEST, "{id:?}"); + 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 status = match check_payment_status_route(Path(id.to_string())).await { + let error = match check_payment_status_route(Path(id.to_string())).await { Ok(_) => panic!("{id:?} was accepted by check-payment-status"), - Err(e) => e.into_response().status(), + Err(e) => rendered(e).await, }; - assert_eq!(status, StatusCode::BAD_REQUEST, "{id:?}"); + assert_eq!( + error, + ( + StatusCode::BAD_REQUEST, + "Invalid payment reference".to_string() + ), + "{id:?}" + ); } }