fix(api): stop concurrent requests minting several Ghost Keys from one donation

[sanity]

Problem

sign_certificate guards against reusing a PaymentIntent with a certificate_signed flag in Stripe metadata, but the guard is a read-check-write across three separate Stripe API calls with no atomicity between them:

if pi.metadata.get("certificate_signed").is_some() { return Err(AlreadySigned) }  // read
PaymentIntent::update(&client, &pi.id, /* certificate_signed = true */).await?;   // write
// ... sign ...

Two requests carrying the same payment_intent_id can both retrieve the PaymentIntent before either writes, both observe an unset flag, and both go on to sign.

This is worse than an ordinary double-submit. Ghost Keys are sold on the claim that an identity costs real money, so Sybil attacks get expensive. An attacker who donates $1 once and then fires N concurrent /sign-cert requests with N different blinded keys gets N identities for $1, and the scarcity property the whole scheme rests on collapses. The page I just merged in #93 states that property to donors in as many words.

Found while reviewing #93; flagged there as out of scope for that PR.

Approach

A per-PaymentIntent async lock (payment_claim), taken before the PaymentIntent is retrieved and held past the metadata write, so the check and the write cannot interleave.

Scoped honestly: this closes the window within the process, which is the entire exposure today since the API is a single axum process with no orchestration. If it is ever run as more than one instance behind a load balancer the claim has to move to shared storage. The Stripe flag remains the durable record either way, so it still blocks a retry arriving after the first finished, and still survives a restart. That limitation is written into the module docs rather than left implicit.

Two details worth review attention:

• The map is cleaned up on drop. The lock is taken before the PaymentIntent is known to be real, so without reclaiming entries an unauthenticated caller could grow the map without bound by posting garbage ids. Entries are removed when the last guard for a key drops, bounding the map by in-flight requests rather than by PaymentIntents ever seen.
• The Arc::strong_count(lock) == 2 check in Drop means "the map plus this guard, so nobody is waiting". A queued waiter has already cloned the Arc, making it 3, and the entry stays so the waiter keeps contending on the same mutex.

Two adjacent donor-facing fixes the lock enables

Both were reachable before and both leave a donor charged with no key and no way to retry. Neither is safe to do without the lock, because each briefly widens the window the lock now closes:

1. A malformed request no longer burns the donation. BlindedMessage::from_base64 on caller-supplied input ran after the mark was written, so a bad request permanently consumed a real payment. Parsing moved before the mark.
2. A failed signing attempt releases the mark. If sign_with_notary_key or get_notary fails (for example the amount has no notary keypair, the exact hazard documented on the tier list in feat(ghostkey): fix donation funnel conversion and suppress Stripe Link #93), the mark is cleared so the donor can retry. Safe only because the claim is still held, so nothing can slip into the window while the flag is briefly clear.

Testing

payment_claim unit tests, plus source-scrape pins in handle_sign_cert following the pattern used elsewhere in the Freenet codebase.

Mutation-tested, so these are not vacuous. Making claim() hand out a fresh unshared mutex per call (i.e. reintroducing the bug):

test concurrent_claims_on_one_payment_intent_do_not_overlap ... FAILED
test entry_survives_while_a_waiter_is_queued ... FAILED
test claims_on_different_payment_intents_run_concurrently ... ok
test released_claims_are_reclaimed ... ok

with the intended message: "two requests were inside the claim for one PaymentIntent at once, so both could sign and one donation would mint two Ghost Keys". Restored, all 47 pass.

The pins exist because the dangerous regressions here are invisible to behavioural tests: moving the claim after the flag read, or writing let _ = claim(..) instead of let _claim = claim(..), both leave the happy path returning a perfectly valid certificate while the exclusion silently does nothing.

cargo fmt --check clean, cargo build, cargo test -p ghostkey-api 47 passed.

Not addressed

Two pre-existing clippy warnings (unnecessary_get_then_check in this file, type_complexity in tor.rs) are left alone to keep a security-path diff tight. CI gates fmt/build/test, not clippy.

[AI-assisted - Claude]

[sanity]

Review

Four lenses, run sequentially in-session (not as independent blind subagents), each committed to in writing before the next. Full tier: this is a payment path. One finding, fixed in 57db256.

Lens 1 — concurrency correctness (the failure mode specific to this change)

The mechanism itself checked out on every point I could find to attack:

Property	Verdict
std Mutex held across an .await	✅ No. claim() scopes the map lock in a block that ends before lock_owned().await, so a slow claim on one PaymentIntent cannot block claims on others (or deadlock the runtime).
Arc::strong_count(lock) == 2 in Drop	✅ Correct. Struct fields drop after Drop::drop returns, so _guard still holds its Arc during the body: 2 means map + this guard. A queued waiter has already cloned in claim(), making it 3, and the entry correctly stays.
Entry removed before the tokio mutex is released	✅ Safe. ClaimGuard drops only when sign_certificate returns, so the protected region is already over. A fresh claim taking a new mutex cannot overlap it.
Mutex poisoning	✅ unwrap_or_else(|e| e.into_inner()) recovers rather than propagating. Refusing to clean up on a poisoned map would leak.
Guard is Send across awaits	✅ Proven by compilation; axum requires Send futures.

Stripe semantics for the new release path, verified against the docs rather than assumed: setting a metadata key to an empty string deletes the key (it does not store ""), and metadata updates merge rather than replace. Both matter here. If Stripe stored an empty string, pi.metadata.get("certificate_signed").is_some() would still be true and release_certificate_mark would be a silent no-op that looked like it worked. And because updates merge, the donation_type key set at creation survives both the mark and the release.

Lens 2 — adversarial

Attempts to break it that came up empty: submitting distinct blinded keys sequentially against one PaymentIntent (second is correctly rejected), exploiting the window while the mark is briefly released (the claim is still held, and a released mark is the correct state since nothing was issued), and holding the lock to block a legitimate donor (needs the victim's PaymentIntent id, and only affects that one donation).

One thing worth recording: the donation routes have no rate limiting. get_routes() wires /sign-certificate, /create-donation and friends with only CorsLayer; the rate limiter is applied to the invite routes only. Pre-existing and not introduced here, and it isn't a key-minting risk because a real succeeded PaymentIntent is still required. It does mean unauthenticated callers can drive Stripe retrieve calls. Worth a separate issue rather than widening this diff.

Lens 3 — test integrity

The finding. Two tests asserted on the size of the global CLAIM_LOCKS map (tracked_keys()), but the harness runs tests in parallel threads within one process and that map is process-global. released_claims_are_reclaimed compared a before/after count that another test could move underneath it, and entry_survives_while_a_waiter_is_queued asserted >= 1, which another test's live entry could satisfy vacuously. Both were green, and both were coupled to whatever else happened to be running.

Replaced tracked_keys() with is_tracked(key) and made every assertion key-specific, so the tests no longer interfere with each other by construction. entry_survives_while_a_waiter_is_queued also now asserts the entry is gone once the last guard drops, which it never checked before.

Re-ran the mutation after changing the assertions, since edited assertions have to be re-proven non-vacuous:

test concurrent_claims_on_one_payment_intent_do_not_overlap ... FAILED
test entry_survives_while_a_waiter_is_queued ... FAILED

Restored, 47 pass, and 5 consecutive runs of the payment_claim tests are stable.

Lens 4 — scope

What this does not do, stated so it isn't mistaken for more: the guard is per-process. It closes the entire exposure as deployed today (single axum process, no orchestration), and it does not span multiple instances behind a load balancer. The Stripe flag remains the durable record, so it still blocks a retry arriving after the first finished and still survives a restart. This is in the module docs, not just here.

Verification

cargo fmt --check clean, cargo build, cargo test -p ghostkey-api 47 passed, 1 ignored.

[AI-assisted - Claude]