Skip to main content

max / makenotwork

server: drive Payments axis to A+ (ultra-fuzz Run #1 --deep Phase 1) Make the webhook-dedup invariant structural rather than review-time discipline: try_claim_fan_plus_credit now returns a FanPlusCreditClaim witness (private ctor, SubscriptionGate pattern) and issue_fan_plus_credit_code requires it, so minting/emailing a Fan+ renewal credit without first winning the idempotency slot is uncompilable. Document the invariant at the dedup site. Delete the dead existing_user_id auto-attach param from complete_guest_transaction (only caller passed None; latent foot-gun NOTE). Add adversarial coverage for db::idempotency (was testing=C, zero tests): scope isolation, first-writer-wins, concurrent single-row race, 24h cleanup boundary, empty key. Module made pub for the integration test crate. Guard synckit_billing apply_billing_update against an inverted Stripe period (start <= end) alongside the existing non-positive-end guard. Fix a pre-existing clippy bool_assert_comparison nit in auth.rs tests. Gate: idempotency 6, payments/license/subs workflows 108, lib units 64 green; clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-22 21:50 UTC
Commit: 221931376b16898a3a5404206b7317647ccd2b93
Parent: 54589bd
10 files changed, +310 insertions, -30 deletions
M server/src/auth.rs +1 -1
@@ -668,7 +668,7 @@ mod tests {
668 668 // A corrupt / non-Argon2 stored hash must fail login cleanly (Ok(false)),
669 669 // not 500 — avoids an availability bug and an account oracle (SEC, Run #23).
670 670 for bad in ["", "not-a-phc-string", "$argon2id$garbage", "$2y$10$abcdefghijklmnopqrstuv"] {
671 - assert_eq!(verify_password("any", bad).unwrap(), false, "hash {bad:?} should be a non-match");
671 + assert!(!verify_password("any", bad).unwrap(), "hash {bad:?} should be a non-match");
672 672 }
673 673 }
674 674
M server/src/db/mod.rs +1 -1
@@ -64,7 +64,7 @@ pub(crate) mod imports;
64 64 pub(crate) mod media_files;
65 65 pub(crate) mod tips;
66 66 pub(crate) mod project_members;
67 - pub(crate) mod idempotency;
67 + pub mod idempotency; // pub so the integration test crate can exercise it directly
68 68 pub(crate) mod pending_refunds;
69 69 pub mod webhook_events;
70 70 pub(crate) mod scheduler_jobs;
M server/src/db/promo_codes.rs +54 -6
@@ -432,24 +432,42 @@ pub async fn create_platform_promo_code(
432 432 Ok(promo_code)
433 433 }
434 434
435 + /// Unforgeable proof that the once-per-renewal Fan+ credit slot for a
436 + /// `(stripe_sub_id, period_end)` was won by *this* webhook delivery.
437 + ///
438 + /// Returned only by [`try_claim_fan_plus_credit`], and required by reference by
439 + /// [`issue_fan_plus_credit_code`] — the only path that mints the credit. The
440 + /// private field makes it unconstructable outside this module (the
441 + /// [`super::subscriptions::SubscriptionGate`] pattern), so "mint or email a Fan+
442 + /// credit without first winning the idempotency slot" cannot be written. This
443 + /// turns the webhook-dedup invariant — a non-idempotent, money-moving side-effect
444 + /// must sit behind its own atomic claim — from review-time discipline into a
445 + /// compile-time guarantee.
446 + #[must_use]
447 + pub struct FanPlusCreditClaim {
448 + _seal: (),
449 + }
450 +
435 451 /// Claim the once-per-renewal slot for a Fan+ monthly credit.
436 452 ///
437 - /// Returns `true` if this `(stripe_sub_id, period_end)` was not yet claimed (row
438 - /// inserted — the caller should mint and email the credit), `false` if a prior
439 - /// delivery of the same renewal already claimed it (the caller must do nothing).
453 + /// Returns `Some(`[`FanPlusCreditClaim`]`)` if this `(stripe_sub_id, period_end)`
454 + /// was not yet claimed (row inserted — the caller holds the witness needed to
455 + /// mint and email the credit), `None` if a prior delivery of the same renewal
456 + /// already claimed it (the caller must do nothing).
440 457 ///
441 458 /// This is the DB-level idempotency guard that closes the duplicate-webhook
442 459 /// double-credit race: `invoice.payment_succeeded` dedup at the webhook layer is
443 460 /// a check-then-act read that two concurrent deliveries both pass, so the
444 461 /// money-moving side-effect must serialize on its own atomic write. The
445 462 /// `ON CONFLICT DO NOTHING` against the `(stripe_sub_id, period_end)` primary key
446 - /// makes exactly one of N concurrent deliveries win.
463 + /// makes exactly one of N concurrent deliveries win — and only that one gets a
464 + /// witness.
447 465 #[tracing::instrument(skip_all)]
448 466 pub async fn try_claim_fan_plus_credit(
449 467 pool: &PgPool,
450 468 stripe_sub_id: &str,
451 469 period_end: i64,
452 - ) -> Result<bool> {
470 + ) -> Result<Option<FanPlusCreditClaim>> {
453 471 let result = sqlx::query(
454 472 "INSERT INTO fan_plus_credit_issuance (stripe_sub_id, period_end) \
455 473 VALUES ($1, $2) ON CONFLICT DO NOTHING",
@@ -459,7 +477,37 @@ pub async fn try_claim_fan_plus_credit(
459 477 .execute(pool)
460 478 .await?;
461 479
462 - Ok(result.rows_affected() == 1)
480 + Ok((result.rows_affected() == 1).then_some(FanPlusCreditClaim { _seal: () }))
481 + }
482 +
483 + /// Mint the $5 single-use, platform-wide credit code for a won Fan+ renewal.
484 + ///
485 + /// Requires a [`FanPlusCreditClaim`] by reference: the only way to obtain one is
486 + /// to win [`try_claim_fan_plus_credit`], so this side-effect is structurally
487 + /// unreachable for a duplicate/redelivered webhook. The credit terms ($5 fixed,
488 + /// single use) are sealed here rather than spelled out at the call site, so every
489 + /// Fan+ credit is identical by construction.
490 + #[tracing::instrument(skip_all)]
491 + pub async fn issue_fan_plus_credit_code(
492 + _claim: &FanPlusCreditClaim,
493 + pool: &PgPool,
494 + creator_id: UserId,
495 + code: &str,
496 + expires_at: Option<chrono::DateTime<chrono::Utc>>,
497 + ) -> Result<DbPromoCode> {
498 + create_platform_promo_code(
499 + pool,
500 + creator_id,
501 + code,
502 + super::CodePurpose::Discount,
503 + Some(DiscountType::Fixed),
504 + Some(500), // $5 credit
505 + 0,
506 + None,
507 + Some(1), // single use
508 + expires_at,
509 + )
510 + .await
463 511 }
464 512
465 513 /// Look up a platform-wide promo code by user ID and code string (case-insensitive).
M server/src/db/synckit_billing.rs +6 -1
@@ -171,7 +171,12 @@ pub async fn apply_billing_update<'e>(
171 171 period: Option<(i64, i64)>,
172 172 ) -> Result<bool> {
173 173 let (period_start, period_end) = match period {
174 - Some((start, end)) if end > 0 => (
174 + // Require a positive, non-inverted window. A non-positive `end` is the
175 + // thin/zero-webhook guard (no 1970 period); `start <= end` additionally
176 + // rejects an inverted range, so a malformed Stripe period writes nothing
177 + // (the `COALESCE` keeps the existing values) rather than stamping an
178 + // end-before-start window onto a live app.
179 + Some((start, end)) if end > 0 && start <= end => (
175 180 DateTime::<Utc>::from_timestamp(start, 0),
176 181 DateTime::<Utc>::from_timestamp(end, 0),
177 182 ),
M server/src/db/transactions.rs +8 -8
@@ -71,18 +71,19 @@ pub async fn create_transaction<'e>(
71 71 Ok(tx)
72 72 }
73 73
74 - /// Complete a guest transaction: set guest_email, generate claim_token, and
75 - /// optionally auto-attach to an existing user if the email matches.
74 + /// Complete a guest transaction: mark it completed, record the guest email, and
75 + /// mint a `claim_token` so the buyer can later attach the purchase to an account.
76 76 ///
77 - /// Uses a transaction to prevent races between auto-attach and concurrent
78 - /// signup/email-verification for the same email.
77 + /// Guest purchases always land unclaimed (`buyer_id` NULL); attachment to a user
78 + /// happens out of band via [`attach_guest_purchases_by_email`] at signup/email
79 + /// verification. (A prior `existing_user_id` auto-attach parameter was always
80 + /// passed `None` and has been removed — Run #1 NOTE, dead foot-gun.)
79 81 #[tracing::instrument(skip_all)]
80 82 pub async fn complete_guest_transaction<'e>(
81 83 executor: impl sqlx::PgExecutor<'e>,
82 84 stripe_checkout_session_id: &str,
83 85 stripe_payment_intent_id: &str,
84 86 guest_email: &str,
85 - existing_user_id: Option<UserId>,
86 87 ) -> Result<Option<DbTransaction>> {
87 88 let claim_token = ClaimToken::new();
88 89
@@ -93,8 +94,8 @@ pub async fn complete_guest_transaction<'e>(
93 94 stripe_payment_intent_id = $2,
94 95 completed_at = NOW(),
95 96 guest_email = $3,
96 - claim_token = CASE WHEN $4::UUID IS NOT NULL THEN NULL ELSE $5 END,
97 - buyer_id = $4
97 + claim_token = $4,
98 + buyer_id = NULL
98 99 WHERE stripe_checkout_session_id = $1
99 100 AND status = 'pending'
100 101 RETURNING *
@@ -103,7 +104,6 @@ pub async fn complete_guest_transaction<'e>(
103 104 .bind(stripe_checkout_session_id)
104 105 .bind(stripe_payment_intent_id)
105 106 .bind(guest_email)
106 - .bind(existing_user_id)
107 107 .bind(claim_token)
108 108 .fetch_optional(executor)
109 109 .await?;
M server/src/routes/stripe/webhook/billing.rs +8 -12
@@ -81,12 +81,13 @@ pub(super) async fn handle_invoice_payment_succeeded(
81 81 // that two concurrent deliveries of the same `invoice.payment_succeeded`
82 82 // both pass, so the credit — a money-moving side-effect — serializes on
83 83 // its own atomic write here. `try_claim_fan_plus_credit` inserts a
84 - // `(stripe_sub_id, period_end)` row ON CONFLICT DO NOTHING; only the
85 - // delivery that wins the insert mints and emails the code. A redelivery
86 - // (or duplicate concurrent delivery) finds the slot taken and skips,
87 - // so a renewal issues at most one $5 credit.
84 + // `(stripe_sub_id, period_end)` row ON CONFLICT DO NOTHING and hands the
85 + // winner a `FanPlusCreditClaim` witness; `issue_fan_plus_credit_code`
86 + // requires that witness, so the mint+email path is unreachable without
87 + // it. A redelivery (or duplicate concurrent delivery) gets `None` and
88 + // skips, so a renewal issues at most one $5 credit.
88 89 if is_renewal
89 - && db::promo_codes::try_claim_fan_plus_credit(
90 + && let Some(claim) = db::promo_codes::try_claim_fan_plus_credit(
90 91 &state.db, &stripe_sub_id, invoice.period_end,
91 92 )
92 93 .await
@@ -102,16 +103,11 @@ pub(super) async fn handle_invoice_payment_succeeded(
102 103 // if one ever lands, the INSERT errors out as DB error 23505 and
103 104 // surfaces to the operator log — no silent overwrite.
104 105 let code = helpers::generate_key_code();
105 - match db::promo_codes::create_platform_promo_code(
106 + match db::promo_codes::issue_fan_plus_credit_code(
107 + &claim,
106 108 &state.db,
107 109 fan_sub.user_id,
108 110 code.as_str(),
109 - db::CodePurpose::Discount,
110 - Some(db::DiscountType::Fixed),
111 - Some(500), // $5 credit
112 - 0,
113 - None,
114 - Some(1), // single use
115 111 period_end,
116 112 ).await {
117 113 Ok(pc) => {
M server/src/routes/stripe/webhook/checkout.rs -1
@@ -631,7 +631,6 @@ pub(super) async fn handle_guest_checkout_completed(
631 631 &session_id,
632 632 &payment_intent_id,
633 633 &guest_email,
634 - None,
635 634 ).await? {
636 635 Some(tx) => {
637 636 tracing::info!(
M server/src/routes/stripe/webhook/mod.rs +9
@@ -51,6 +51,15 @@ pub(in crate::routes::stripe) async fn webhook(
51 51 // (handlers are idempotent). The old "mark before processing" ordering could
52 52 // strand an event whose process died after the mark committed but before its
53 53 // work or retry row landed.
54 + //
55 + // INVARIANT (load-bearing): this dedup is check-then-act, so two concurrent
56 + // deliveries of the same event both reach `process_webhook_event`. Safety
57 + // therefore rests entirely on every handler's side-effects being atomic and
58 + // idempotent — a status-guarded UPDATE, an ON CONFLICT write, or a money
59 + // mint that sits behind its own claim witness (see `FanPlusCreditClaim` in
60 + // `db::promo_codes`). A *non-idempotent* side-effect (sending an email,
61 + // calling an external API) MUST be gated behind such a claim, never run
62 + // before its atomic write — otherwise a redelivery double-fires it.
54 63 match db::webhook_events::is_event_processed(&state.db, &event.id).await {
55 64 Ok(true) => {
56 65 tracing::info!(event_id = %event.id, "duplicate webhook event, skipping");
A server/tests/workflows/idempotency.rs +222
@@ -0,0 +1,222 @@
1 + //! Adversarial coverage for `db::idempotency` (the POST-retry cache).
2 + //!
3 + //! Run #1 graded `db/idempotency.rs` Testing=C (zero tests). These probe the
4 + //! invariants the module is supposed to hold: (key, user, method, path) scoping,
5 + //! first-writer-wins under ON CONFLICT, and the 24-hour cleanup boundary.
6 +
7 + use crate::harness::db::TestDb;
8 + use makenotwork::db::{idempotency, UserId};
9 + use sqlx::PgPool;
10 +
11 + /// Seed a minimal verified user and return its id (the cache FKs `users(id)`).
12 + async fn seed_user(pool: &PgPool, username: &str) -> UserId {
13 + let hash = makenotwork::auth::hash_password("password123").expect("hash");
14 + sqlx::query_scalar::<_, UserId>(
15 + "INSERT INTO users (username, email, password_hash, email_verified)
16 + VALUES ($1, $2, $3, true) RETURNING id",
17 + )
18 + .bind(username)
19 + .bind(format!("{username}@test.com"))
20 + .bind(&hash)
21 + .fetch_one(pool)
22 + .await
23 + .expect("seed user")
24 + }
25 +
26 + #[tokio::test]
27 + async fn store_then_get_roundtrip() {
28 + let db = TestDb::new().await;
29 + let user = seed_user(&db.pool, "idem_roundtrip").await;
30 +
31 + assert!(
32 + idempotency::get_cached_response(&db.pool, "k1", user, "POST", "/checkout")
33 + .await
34 + .expect("get miss")
35 + .is_none(),
36 + "cold key must miss"
37 + );
38 +
39 + idempotency::store_response(&db.pool, "k1", user, "POST", "/checkout", 201, "{\"ok\":true}")
40 + .await
41 + .expect("store");
42 +
43 + let hit = idempotency::get_cached_response(&db.pool, "k1", user, "POST", "/checkout")
44 + .await
45 + .expect("get hit")
46 + .expect("must hit after store");
47 + assert_eq!(hit.status_code, 201);
48 + assert_eq!(hit.response_body, "{\"ok\":true}");
49 + }
50 +
51 + #[tokio::test]
52 + async fn scope_isolates_key_across_user_method_path() {
53 + let db = TestDb::new().await;
54 + let alice = seed_user(&db.pool, "idem_alice").await;
55 + let bob = seed_user(&db.pool, "idem_bob").await;
56 +
57 + // Same key string, four distinct scopes — none may leak into another.
58 + idempotency::store_response(&db.pool, "shared", alice, "POST", "/a", 200, "alice-a")
59 + .await
60 + .unwrap();
61 + idempotency::store_response(&db.pool, "shared", alice, "POST", "/b", 200, "alice-b")
62 + .await
63 + .unwrap();
64 + idempotency::store_response(&db.pool, "shared", alice, "DELETE", "/a", 200, "alice-del-a")
65 + .await
66 + .unwrap();
67 + idempotency::store_response(&db.pool, "shared", bob, "POST", "/a", 200, "bob-a")
68 + .await
69 + .unwrap();
70 +
71 + let cases = [
72 + (alice, "POST", "/a", "alice-a"),
73 + (alice, "POST", "/b", "alice-b"),
74 + (alice, "DELETE", "/a", "alice-del-a"),
75 + (bob, "POST", "/a", "bob-a"),
76 + ];
77 + for (user, method, path, want) in cases {
78 + let got = idempotency::get_cached_response(&db.pool, "shared", user, method, path)
79 + .await
80 + .unwrap()
81 + .expect("each scope is stored independently");
82 + assert_eq!(got.response_body, want, "scope {method} {path} leaked");
83 + }
84 +
85 + // A scope nobody wrote (bob, DELETE, /a) must still miss.
86 + assert!(
87 + idempotency::get_cached_response(&db.pool, "shared", bob, "DELETE", "/a")
88 + .await
89 + .unwrap()
90 + .is_none()
91 + );
92 + }
93 +
94 + #[tokio::test]
95 + async fn second_store_is_a_no_op_first_writer_wins() {
96 + let db = TestDb::new().await;
97 + let user = seed_user(&db.pool, "idem_firstwriter").await;
98 +
99 + idempotency::store_response(&db.pool, "k", user, "POST", "/x", 201, "first")
100 + .await
101 + .unwrap();
102 + // ON CONFLICT DO NOTHING: a second store on the same scope must not clobber.
103 + idempotency::store_response(&db.pool, "k", user, "POST", "/x", 500, "second")
104 + .await
105 + .unwrap();
106 +
107 + let got = idempotency::get_cached_response(&db.pool, "k", user, "POST", "/x")
108 + .await
109 + .unwrap()
110 + .unwrap();
111 + assert_eq!(got.status_code, 201, "first writer's status must stand");
112 + assert_eq!(got.response_body, "first", "first writer's body must stand");
113 + }
114 +
115 + #[tokio::test]
116 + async fn concurrent_stores_keep_exactly_one_row() {
117 + let db = TestDb::new().await;
118 + let user = seed_user(&db.pool, "idem_concurrent").await;
119 +
120 + // 16 concurrent deliveries of the "same" retry, each with a distinct body.
121 + let mut handles = Vec::new();
122 + for i in 0..16 {
123 + let pool = db.pool.clone();
124 + handles.push(tokio::spawn(async move {
125 + idempotency::store_response(
126 + &pool,
127 + "race",
128 + user,
129 + "POST",
130 + "/checkout",
131 + 201,
132 + &format!("body-{i}"),
133 + )
134 + .await
135 + }));
136 + }
137 + for h in handles {
138 + h.await.expect("join").expect("store ok");
139 + }
140 +
141 + let count: i64 = sqlx::query_scalar(
142 + "SELECT COUNT(*) FROM idempotency_keys WHERE key = 'race' AND user_id = $1",
143 + )
144 + .bind(user)
145 + .fetch_one(&db.pool)
146 + .await
147 + .unwrap();
148 + assert_eq!(count, 1, "exactly one row survives the race");
149 +
150 + // And the cached read is stable (some single winner's body).
151 + let got = idempotency::get_cached_response(&db.pool, "race", user, "POST", "/checkout")
152 + .await
153 + .unwrap()
154 + .unwrap();
155 + assert!(got.response_body.starts_with("body-"));
156 + }
157 +
158 + #[tokio::test]
159 + async fn cleanup_expired_respects_24h_boundary() {
160 + let db = TestDb::new().await;
161 + let user = seed_user(&db.pool, "idem_cleanup").await;
162 +
163 + idempotency::store_response(&db.pool, "fresh", user, "POST", "/x", 200, "fresh")
164 + .await
165 + .unwrap();
166 + idempotency::store_response(&db.pool, "stale", user, "POST", "/x", 200, "stale")
167 + .await
168 + .unwrap();
169 + // Backdate the stale row just past the 24h window.
170 + sqlx::query(
171 + "UPDATE idempotency_keys SET created_at = NOW() - INTERVAL '25 hours' \
172 + WHERE key = 'stale' AND user_id = $1",
173 + )
174 + .bind(user)
175 + .execute(&db.pool)
176 + .await
177 + .unwrap();
178 +
179 + let deleted = idempotency::cleanup_expired(&db.pool).await.unwrap();
180 + assert_eq!(deleted, 1, "only the >24h row is purged");
181 +
182 + assert!(
183 + idempotency::get_cached_response(&db.pool, "fresh", user, "POST", "/x")
184 + .await
185 + .unwrap()
186 + .is_some(),
187 + "fresh row survives cleanup"
188 + );
189 + assert!(
190 + idempotency::get_cached_response(&db.pool, "stale", user, "POST", "/x")
191 + .await
192 + .unwrap()
193 + .is_none(),
194 + "stale row is gone"
195 + );
196 + }
197 +
198 + #[tokio::test]
199 + async fn empty_key_is_stored_and_scoped_independently() {
200 + let db = TestDb::new().await;
201 + let user = seed_user(&db.pool, "idem_emptykey").await;
202 +
203 + // An empty key is a benign, distinct key value — it must round-trip and not
204 + // collide with a non-empty key in the same scope.
205 + idempotency::store_response(&db.pool, "", user, "POST", "/x", 202, "empty")
206 + .await
207 + .unwrap();
208 + idempotency::store_response(&db.pool, "k", user, "POST", "/x", 200, "nonempty")
209 + .await
210 + .unwrap();
211 +
212 + let empty = idempotency::get_cached_response(&db.pool, "", user, "POST", "/x")
213 + .await
214 + .unwrap()
215 + .unwrap();
216 + assert_eq!(empty.response_body, "empty");
217 + let nonempty = idempotency::get_cached_response(&db.pool, "k", user, "POST", "/x")
218 + .await
219 + .unwrap()
220 + .unwrap();
221 + assert_eq!(nonempty.response_body, "nonempty");
222 + }
M server/tests/workflows/mod.rs +1
@@ -91,3 +91,4 @@ mod rate_limiting;
91 91 mod lifecycle;
92 92 mod cart;
93 93 mod bundles;
94 + mod idempotency;