From afe081e75a792b0aa8be8747c29b02ca67cefb1d Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Tue, 28 Jul 2026 20:35:57 -0500 Subject: [PATCH 1/2] wip: payment claim lock --- rust/api/src/handle_sign_cert.rs | 159 ++++++++++++++++++++++-- rust/api/src/main.rs | 1 + rust/api/src/payment_claim.rs | 205 +++++++++++++++++++++++++++++++ 3 files changed, 358 insertions(+), 7 deletions(-) create mode 100644 rust/api/src/payment_claim.rs diff --git a/rust/api/src/handle_sign_cert.rs b/rust/api/src/handle_sign_cert.rs index 9699c390..653d9ea4 100644 --- a/rust/api/src/handle_sign_cert.rs +++ b/rust/api/src/handle_sign_cert.rs @@ -54,6 +54,15 @@ pub async fn sign_certificate( log::info!("STRIPE_SECRET_KEY found"); let client = Client::new(stripe_secret_key); + // Take an exclusive claim on this PaymentIntent and hold it for the rest of + // the function. The `certificate_signed` check below and the update that + // sets it are two separate Stripe calls with nothing atomic between them, + // so without this, concurrent requests carrying the same PaymentIntent all + // observe an unset flag and all go on to sign, minting several Ghost Keys + // from one donation. See the payment_claim module for why that specific + // failure matters more than an ordinary double-submit. + let _claim = crate::payment_claim::claim(&request.payment_intent_id).await; + // Verify payment intent let pi = PaymentIntent::retrieve( &client, @@ -89,6 +98,16 @@ pub async fn sign_certificate( return Err(CertificateError::CertificateAlreadySigned); } + // Parse the caller-supplied key BEFORE marking the PaymentIntent as spent. + // A malformed request is the caller's mistake and must not consume the + // donation; marking first would leave a donor charged with nothing to show + // for it and no way to retry. + let blinded_ghostkey = + BlindedMessage::from_base64(&request.blinded_ghost_key_base64).map_err(|e| { + log::error!("Error in from_base64: {:?}", e); + CertificateError::MiscError(e.to_string()) + })?; + // Mark the payment intent as used for certificate signing let mut metadata = HashMap::new(); metadata.insert("certificate_signed".to_string(), "true".to_string()); @@ -101,15 +120,34 @@ pub async fn sign_certificate( // Sign the certificate log::info!("Payment intent verified successfully"); - let blinded_ghostkey = - BlindedMessage::from_base64(&request.blinded_ghost_key_base64).map_err(|e| { - log::error!("Error in from_base64: {:?}", e); - CertificateError::MiscError(e.to_string()) - })?; - let amount_cents = pi.amount as u64; let amount_dollars = amount_cents / 100; - let blind_signature = sign_with_notary_key(&blinded_ghostkey, amount_dollars).map_err(|e| { + + match sign_marked_payment(&blinded_ghostkey, amount_dollars, amount_cents) { + Ok(response) => Ok(response), + Err(e) => { + // The PaymentIntent is marked spent but no certificate came out of + // it, so without this the donor is charged and permanently locked + // out of retrying. Releasing the mark is safe here specifically + // because `_claim` is still held: no concurrent request can slip + // into the window where the flag is briefly clear again. + release_certificate_mark(&client, &pi.id).await; + Err(e) + } + } +} + +/// Produce the signed certificate for a PaymentIntent that has already been +/// marked as spent. +/// +/// Split out so the caller can tell "signing failed" apart from the earlier +/// validation steps and undo the mark for exactly that case. +fn sign_marked_payment( + blinded_ghostkey: &BlindedMessage, + amount_dollars: u64, + amount_cents: u64, +) -> Result { + let blind_signature = sign_with_notary_key(blinded_ghostkey, amount_dollars).map_err(|e| { log::error!("Error in sign_with_notary_key: {:?}", e); e })?; @@ -131,3 +169,110 @@ pub async fn sign_certificate( amount: amount_cents, }) } + +/// Clear `certificate_signed` after a failed signing attempt, so the donation +/// can be retried. +/// +/// Stripe deletes a metadata key when it is set to an empty string. A failure +/// here is logged rather than propagated: the caller is already returning the +/// original signing error, which is the more useful one to surface, and the +/// donation is recoverable by hand from the log line. +async fn release_certificate_mark(client: &Client, pi_id: &stripe::PaymentIntentId) { + let mut metadata = HashMap::new(); + metadata.insert("certificate_signed".to_string(), String::new()); + let params = stripe::UpdatePaymentIntent { + metadata: Some(metadata), + ..Default::default() + }; + + if let Err(e) = PaymentIntent::update(client, pi_id, params).await { + log::error!( + "Signing failed for PaymentIntent {} AND clearing certificate_signed \ + failed: {:?}. This donation is now marked spent with no certificate \ + issued and needs to be cleared by hand before the donor can retry.", + pi_id, + e + ); + } else { + log::warn!( + "Signing failed for PaymentIntent {}; cleared certificate_signed so \ + the donor can retry.", + pi_id + ); + } +} + +#[cfg(test)] +mod tests { + /// Strip all whitespace so the pins below survive rustfmt re-wrapping the + /// lines they match. + fn squeeze(s: &str) -> String { + s.chars().filter(|c| !c.is_whitespace()).collect() + } + + /// Production source only. Without this cut the needles match their own + /// text in this test module and every pin passes vacuously. + fn production_source() -> String { + let source = include_str!("handle_sign_cert.rs"); + let production = source + .split_once("\nmod tests {") + .map(|(before, _)| before) + .expect("test module marker not found; the cut below is not working"); + squeeze(production) + } + + /// The claim has to be taken before the flag is read, not after. Taking it + /// afterwards leaves exactly the read-check-write window it exists to + /// close, and nothing else in the test suite would notice: the happy path + /// still returns a valid certificate. + #[test] + fn claim_is_taken_before_the_signed_flag_is_read() { + let source = production_source(); + + let claim_at = source + .find(&squeeze("payment_claim::claim(&request.payment_intent_id)")) + .expect("sign_certificate no longer claims the PaymentIntent at all"); + let check_at = source + .find(&squeeze(r#"pi.metadata.get("certificate_signed")"#)) + .expect("the certificate_signed check has moved or been renamed"); + + assert!( + claim_at < check_at, + "the PaymentIntent claim must be taken BEFORE certificate_signed is \ + read, otherwise concurrent requests can both observe an unset flag \ + and one donation mints several Ghost Keys" + ); + } + + /// `let _ = claim(..)` drops the guard immediately and `let _claim = ..` + /// holds it to end of scope. The two differ by one character and only the + /// second one actually excludes anything, so pin the binding shape. + #[test] + fn claim_guard_is_bound_and_not_dropped_immediately() { + let source = production_source(); + + assert!( + source.contains(&squeeze("let _claim = crate::payment_claim::claim(")), + "the claim guard must be bound to a named binding that lives to the \ + end of sign_certificate" + ); + assert!( + !source.contains(&squeeze("let _ = crate::payment_claim::claim(")), + "`let _ = claim(..)` drops the guard on the spot, so the claim is \ + released before the flag is even read and the race is fully open" + ); + } + + /// A signing failure after the mark is set must clear it, or the donor is + /// charged and permanently unable to retry. + #[test] + fn failed_signing_releases_the_mark() { + let source = production_source(); + + assert!( + source.contains(&squeeze("release_certificate_mark(&client, &pi.id)")), + "signing failures must clear certificate_signed, otherwise a \ + transient failure burns the donation" + ); + } +} diff --git a/rust/api/src/main.rs b/rust/api/src/main.rs index 4a298b4d..5fd36266 100644 --- a/rust/api/src/main.rs +++ b/rust/api/src/main.rs @@ -18,6 +18,7 @@ mod errors; mod handle_sign_cert; mod invite; mod invite_pow; +mod payment_claim; mod rate_limit; mod routes; mod tor; diff --git a/rust/api/src/payment_claim.rs b/rust/api/src/payment_claim.rs new file mode 100644 index 00000000..5fbfa577 --- /dev/null +++ b/rust/api/src/payment_claim.rs @@ -0,0 +1,205 @@ +//! Serializes certificate-signing attempts for a single PaymentIntent. +//! +//! The durable record that a PaymentIntent has already been spent on a Ghost +//! Key is its `certificate_signed` metadata flag in Stripe. Stripe offers no +//! compare-and-swap on metadata, so reading the flag, deciding, and then +//! setting it is three separate API calls with no atomicity between them. Two +//! requests carrying the same PaymentIntent could both observe an unset flag, +//! both set it, and both go on to sign, minting two Ghost Keys from one +//! donation. +//! +//! That matters more than an ordinary double-submit bug. Ghost Keys are sold +//! on the claim that an identity costs real money, so Sybil attacks get +//! expensive. An attacker who can mint N keys from one $1 donation by firing N +//! concurrent requests reduces that cost to nearly zero and the scarcity +//! property collapses. +//! +//! A per-PaymentIntent lock closes the window, which is the whole exposure +//! today: the API is a single axum process, so every request for a given +//! PaymentIntent contends on the same map. If it is ever run as more than one +//! instance behind a load balancer, this guard no longer spans them and the +//! claim has to move to shared storage. The Stripe flag remains the durable +//! record either way, so it still blocks a retry that arrives after the first +//! one finished, and still survives a restart. + +use std::collections::HashMap; +use std::sync::{Arc, LazyLock, Mutex}; + +use tokio::sync::{Mutex as AsyncMutex, OwnedMutexGuard}; + +/// 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>>>> = + LazyLock::new(|| Mutex::new(HashMap::new())); + +/// Exclusive claim on one PaymentIntent, held for as long as the guard lives. +pub(crate) struct ClaimGuard { + 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 { + 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); + } + } + } +} + +/// 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() + }; + + // Awaited with the map lock released, so a slow claim on one PaymentIntent + // never blocks claims on others. + let guard = lock.lock_owned().await; + + ClaimGuard { + payment_intent_id: payment_intent_id.to_string(), + _guard: guard, + } +} + +#[cfg(test)] +fn tracked_keys() -> usize { + CLAIM_LOCKS.lock().unwrap_or_else(|e| e.into_inner()).len() +} + +#[cfg(test)] +mod tests { + use std::sync::atomic::{AtomicUsize, Ordering}; + + use super::*; + + /// 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. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn concurrent_claims_on_one_payment_intent_do_not_overlap() { + let inside = Arc::new(AtomicUsize::new(0)); + let peak = Arc::new(AtomicUsize::new(0)); + + let mut tasks = Vec::new(); + for _ in 0..16 { + let inside = Arc::clone(&inside); + let peak = Arc::clone(&peak); + tasks.push(tokio::spawn(async move { + let _claim = claim("pi_contended").await; + + let now = inside.fetch_add(1, Ordering::SeqCst) + 1; + peak.fetch_max(now, Ordering::SeqCst); + // Long enough that overlapping tasks would reliably be caught + // in the window together. + tokio::time::sleep(std::time::Duration::from_millis(5)).await; + inside.fetch_sub(1, Ordering::SeqCst); + })); + } + for t in tasks { + t.await.unwrap(); + } + + assert_eq!( + peak.load(Ordering::SeqCst), + 1, + "two requests were inside the claim for one PaymentIntent at once, \ + so both could sign and one donation would mint two Ghost Keys" + ); + } + + /// The lock must be per-PaymentIntent, not global, or one slow donation + /// serializes everyone else's. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn claims_on_different_payment_intents_run_concurrently() { + let inside = Arc::new(AtomicUsize::new(0)); + let peak = Arc::new(AtomicUsize::new(0)); + + let mut tasks = Vec::new(); + for i in 0..8 { + let inside = Arc::clone(&inside); + let peak = Arc::clone(&peak); + tasks.push(tokio::spawn(async move { + let _claim = claim(&format!("pi_distinct_{i}")).await; + + let now = inside.fetch_add(1, Ordering::SeqCst) + 1; + peak.fetch_max(now, Ordering::SeqCst); + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + inside.fetch_sub(1, Ordering::SeqCst); + })); + } + for t in tasks { + t.await.unwrap(); + } + + assert!( + peak.load(Ordering::SeqCst) > 1, + "distinct PaymentIntents were serialized against each other" + ); + } + + /// The lock is taken before the PaymentIntent is known to be real, so + /// entries have to be reclaimed or unauthenticated garbage ids grow the + /// map without bound. + #[tokio::test] + async fn released_claims_are_reclaimed() { + let before = tracked_keys(); + + for i in 0..64 { + let _claim = claim(&format!("pi_garbage_{i}")).await; + } + + assert_eq!( + tracked_keys(), + before, + "claim map grew after every guard was dropped, so a caller posting \ + unknown PaymentIntent ids can exhaust memory" + ); + } + + /// A waiter must keep contending on the same mutex the holder is using; if + /// cleanup dropped the entry out from under it, the two would end up on + /// different mutexes and the exclusion would silently stop working. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn entry_survives_while_a_waiter_is_queued() { + let held = claim("pi_handoff").await; + + let waiter = tokio::spawn(async move { + let _claim = claim("pi_handoff").await; + tracked_keys() + }); + + // Give the waiter time to queue behind the held claim. + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + assert!( + tracked_keys() >= 1, + "entry vanished while a waiter was queued" + ); + + drop(held); + assert!(waiter.await.unwrap() >= 1); + } +} From 57db25656b9c1e07e069cd3702e380b227e14901 Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Wed, 29 Jul 2026 09:21:30 -0500 Subject: [PATCH 2/2] test: assert on specific claim keys, not global map size --- rust/api/src/payment_claim.rs | 46 ++++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/rust/api/src/payment_claim.rs b/rust/api/src/payment_claim.rs index 5fbfa577..51ec6bcd 100644 --- a/rust/api/src/payment_claim.rs +++ b/rust/api/src/payment_claim.rs @@ -85,9 +85,18 @@ pub(crate) async fn claim(payment_intent_id: &str) -> ClaimGuard { } } +/// Whether a specific PaymentIntent currently has a live entry. +/// +/// Tests assert on individual keys rather than on the size of the map: +/// `CLAIM_LOCKS` is process-global and the test harness runs tests in parallel +/// threads, so any assertion about the total count is really an assertion about +/// what every other test in this module happens to be doing at that instant. #[cfg(test)] -fn tracked_keys() -> usize { - CLAIM_LOCKS.lock().unwrap_or_else(|e| e.into_inner()).len() +fn is_tracked(payment_intent_id: &str) -> bool { + CLAIM_LOCKS + .lock() + .unwrap_or_else(|e| e.into_inner()) + .contains_key(payment_intent_id) } #[cfg(test)] @@ -166,17 +175,19 @@ mod tests { /// map without bound. #[tokio::test] async fn released_claims_are_reclaimed() { - let before = tracked_keys(); + let keys: Vec = (0..64).map(|i| format!("pi_garbage_{i}")).collect(); - for i in 0..64 { - let _claim = claim(&format!("pi_garbage_{i}")).await; + for key in &keys { + let _claim = claim(key).await; } - assert_eq!( - tracked_keys(), - before, - "claim map grew after every guard was dropped, so a caller posting \ - unknown PaymentIntent ids can exhaust memory" + let leaked: Vec<&String> = keys.iter().filter(|k| is_tracked(k)).collect(); + assert!( + leaked.is_empty(), + "{} claim entries survived their guards, so a caller posting unknown \ + PaymentIntent ids can exhaust memory: {:?}", + leaked.len(), + leaked ); } @@ -189,17 +200,24 @@ mod tests { let waiter = tokio::spawn(async move { let _claim = claim("pi_handoff").await; - tracked_keys() + // Still tracked while this second guard holds it. + is_tracked("pi_handoff") }); // Give the waiter time to queue behind the held claim. tokio::time::sleep(std::time::Duration::from_millis(50)).await; assert!( - tracked_keys() >= 1, - "entry vanished while a waiter was queued" + is_tracked("pi_handoff"), + "entry vanished while a waiter was queued, so the waiter is now \ + contending on a different mutex than the holder and the exclusion \ + has silently stopped working" ); drop(held); - assert!(waiter.await.unwrap() >= 1); + assert!(waiter.await.unwrap()); + assert!( + !is_tracked("pi_handoff"), + "entry outlived the last guard for this key" + ); } }