From 5cc5a7f6d254c8fbd4ac00e3c76cb583597a5078 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Wed, 12 Aug 2026 23:49:19 +0100 Subject: [PATCH] =?UTF-8?q?fix:=20review=20round=207=20=E2=80=94=20fresh-e?= =?UTF-8?q?yes=20audit=20fixes=20(6=20agents)=20+=20full=20test=20suites?= =?UTF-8?q?=20for=20every=20backend=20change?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fresh-eyes review round with 6 independent agents (money-safety, concurrency, Square wire parity, security, frontend flow, testing-gaps). Every finding was independently verified against the code before fixing. All backend changes now carry full test suites (10+ new tests, each verified to FAIL without its guard). All 20 packages green, race detector clean. Money-safety: - Gift-card purchase refunds no longer create money: manual refunds of a no-booking (gift-card purchase) payment are rejected with a clear message in the direct handler AND never re-issued by the sweep-resume path (processManualPaymentGroup skips them; reconcile-then-fail, no re-issue). - BuyGiftCard no-client-key fallback: derived deterministically under the advisory lock (pending-row reuse fixes lost-response double-charge; completed-row sequence advance preserves distinct-purchase collapse fix). - Terminal completion is never unrecorded: activeTerminalCheckoutID now calls recordUntrackedTerminalPayment when a provisional (tmp-) checkout is found COMPLETED at Square (previously only marked the row COMPLETED — a lost poll left the payment invisible and unrefundable). - Sweep: provisional tmp- checkout rows are resolved against Square first (COMPLETED → record; live → keep guard; NOT_FOUND/CANCELED → fail; ambiguous → leave pending) instead of blind-failing a possibly-live checkout. recordUntrackedTerminalPayment re-checks the booking status (FOR UPDATE) and refuses to record on a cancelled booking, inserting a critical_payment_log admin notification instead. Till-sale post-charge UPDATE now requires status='pending' (no resurrection of a clawed-back sale). Frontend (Svelte 5): - UserPaymentModal keeps CardSelection mounted through processing (bind:this ref + Square iframe survive the loyalty/tokenize awaits) — new-card payments work again. - BookingFlow clears the cached nonce/verification pair on any failure (retry re-tokenizes fresh; idempotency key retained for dedup); 409 'already paid' refetches the booking and reconciles depositPaid so the confirmation gate opens; Back button disabled during processing. - Synchronous double-submit guards on buyGiftCard/redeemGiftCard/submitTip. Square wire parity (mock vs real): - processing_fee sign unified (negated at paymentFromSquare; mock agrees). - SimulateSourceUsed (SOURCE_USED, 400) matches real CreateCard. - GetCardsOnFile excludes disabled cards (matches ListCards). - ForcePaymentStatus toggle + tests prove the charge path can't be status-blind. - CreateCheckout rejects empty device_id (env fallback SQUARE_TERMINAL_DEVICE_ID); completed terminal checkout's payment resolvable by id. Security: - 2FA attempt-map data race fixed: lastAt is atomic.Int64 (nanos) — eviction scan reads race-free; concurrent verify+evict tests under -race. - Backend refuses to start on weak/placeholder JWT_SECRET_KEY (<32 chars or known public placeholders) with openssl rand -hex 32 guidance. - Dockerfile no longer COPYs .env (secrets injected via compose env_file). - SabreDAV requires DAV_ADMIN_PASSWORD (no admin/admin default); compose fails at config time when missing. Testing gaps closed (each verified to FAIL without its guard): - refunded-dedup 409 (CreateBookingPayment), keyed sweep past-retention blind-fail, reconcile status-switch (CANCELED/FAILED/APPROVED/PENDING/unknown in both by-key and by-id paths), resolveChargeSource Square-failure branches, structured 500 / CARD_DECLINED / cancelled-context E2E (row stays pending), deriveBookingPaymentIdempotencyKey >45-char truncation, webhook findPaymentByDisputeID fallback, clawbackOneTillSale non-gift-card branch, dispute.evidence / terminal.checkout dispatch. Infra: - local-dev-2.sh fails loudly on port-5432 squatters / docker compose failures (previously died silently under ERR_EXIT with hidden output). - Test harness defaults SQUARE_TERMINAL_DEVICE_ID; money_safety_fixes_test.go gained the missing build tag. Verification: go test -tags test,dev -count=1 -parallel 8 ./... (20/20 ok), -race clean on 2FA + payments money paths, go build ./... + -tags dev, go vet clean, svelte-check 0 errors, env-docs gate OK (36 vars), docker compose config valid. --- .env.example | 13 +- backend/Dockerfile | 4 +- backend/handlers/payments/giftcards.go | 111 ++- backend/handlers/payments/handlers.go | 59 +- .../payments/money_safety_fixes_test.go | 388 ++++++++++ .../payments/payments_r6_review_test.go | 30 +- .../handlers/payments/payments_round7_test.go | 692 ++++++++++++++++++ backend/handlers/payments/refunds.go | 55 +- backend/handlers/payments/sweep.go | 124 +++- backend/handlers/payments/sweep_test.go | 458 ++++++++++++ backend/handlers/payments/testmain_test.go | 6 + backend/handlers/payments/till.go | 8 +- backend/handlers/payments/till_test.go | 183 +++++ backend/handlers/user/twofa.go | 52 +- backend/handlers/user/twofa_test.go | 199 ++++- .../handlers/webhooks/webhooks_round7_test.go | 397 ++++++++++ backend/internal/square/square_dev.go | 131 +++- backend/internal/square/square_dev_test.go | 230 +++++- backend/internal/square/square_http_client.go | 22 +- .../square/square_http_client_test.go | 97 +++ backend/main.go | 50 ++ compose.yml | 2 +- .../lib/components/booking/BookingFlow.svelte | 68 +- .../lib/components/payments/TipPayment.svelte | 20 +- .../payments/UserPaymentModal.svelte | 14 +- frontend/src/routes/account/+page.svelte | 82 ++- local-dev-2.sh | 28 +- sabredav/server.php | 13 +- 28 files changed, 3328 insertions(+), 208 deletions(-) create mode 100644 backend/handlers/payments/money_safety_fixes_test.go create mode 100644 backend/handlers/payments/payments_round7_test.go create mode 100644 backend/handlers/webhooks/webhooks_round7_test.go diff --git a/.env.example b/.env.example index 658df63..07b38d9 100644 --- a/.env.example +++ b/.env.example @@ -10,7 +10,11 @@ POSTGRES_DB=mydb POSTGRES_HOST=postgres POSTGRES_PORT=5432 -JWT_SECRET_KEY="a-very-secret-key-that-should-be-in-env" +# JWT_SECRET_KEY — REQUIRED, FAIL-CLOSED. The backend refuses to start with an +# empty, weak (<32 chars), or known-placeholder value, because a shared/public +# signing key lets anyone forge an admin JWT. Generate a strong random key: +# openssl rand -hex 32 +JWT_SECRET_KEY= # S3/R2 Configuration (for image storage) # Dev: Uses local Rustfs container (see compose.yml) @@ -93,7 +97,12 @@ GO_TESTING= # CardDAV (SabreDAV) — profile photo sync DAV_BASE_URL=http://localhost:8080 -DAV_ADMIN_PASSWORD=admin +# DAV_ADMIN_PASSWORD — REQUIRED, FAIL-CLOSED. sabredav/server.php refuses to +# start when unset or set to a known weak/default value ('admin' etc.) — this +# server exposes customer PII vCards, so no public default credential is ever +# acceptable. Generate a strong random value: +# openssl rand -hex 32 +DAV_ADMIN_PASSWORD= # Logging # Set to "true" to disable ANSI color escape sequences in log output diff --git a/backend/Dockerfile b/backend/Dockerfile index 3afeb76..a5a5e18 100644 --- a/backend/Dockerfile +++ b/backend/Dockerfile @@ -5,8 +5,8 @@ WORKDIR /app # Copy in your prebuilt Go binary (from local ./backend/bin/backend) COPY bin/backend ./backend -# Copy env file if you want to bake it in (or mount via volume/env_file in compose) -COPY .env ./ +# Secrets are injected at runtime via compose env_file / environment (see +# docker-compose.yml backend env_file: ./.env) — NEVER baked into the image. EXPOSE 8080 diff --git a/backend/handlers/payments/giftcards.go b/backend/handlers/payments/giftcards.go index a7c793a..f20b835 100644 --- a/backend/handlers/payments/giftcards.go +++ b/backend/handlers/payments/giftcards.go @@ -972,40 +972,31 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { return } - // M1: idempotency_key is optional. If not provided, a deterministic key is - // generated server-side based on user_id + amount + recipient_type + card_id. - // This ensures retries of the same logical purchase use the same key while - // distinct purchases get different keys. Max=45 matches Square's limit. + // M1: idempotency_key is optional. When omitted, a deterministic fallback + // key is derived server-side AFTER the advisory lock is acquired (see + // deriveGiftCardIdempotencyKey): the base key (user_id + amount + + // recipient_type + card_id) is reused while a PENDING payment row exists + // for the same logical purchase (a lost-response retry lands on the SAME + // key so Square dedups — the old fresh-random-suffix fallback generated a + // NEW key per retry, missed the pending row, and charged twice), and it + // advances deterministically past COMPLETED purchases so two genuinely + // distinct no-key purchases never collapse onto one dedup key. Clients who + // need full control still supply their own idempotency_key; that path is + // unchanged. + clientSuppliedKey := req.IdempotencyKey != "" + var noKeyCardPart string + if !clientSuppliedKey { + noKeyCardPart = "new" + if req.CardID != nil && *req.CardID != "" { + noKeyCardPart = *req.CardID + } + } if err := validators.Validate.Struct(&req); err != nil { log.Printf("Failed to process request: %v", err) http.Error(w, "Invalid request", http.StatusBadRequest) return } - // M1: generate a deterministic idempotency key server-side if the client - // doesn't provide one. The key is based on user_id + amount + recipient_type - // + card_id (or "new" for new cards), ensuring retries of the same logical - // purchase use the same key while distinct purchases get different keys. - if req.IdempotencyKey == "" { - cardPart := "new" - if req.CardID != nil && *req.CardID != "" { - cardPart = *req.CardID - } - // Fallback key: base + a fresh random suffix (mirrors the refunds.go - // randomHexSuffix pattern) so two identical no-client-key purchases can - // never collapse onto one dedup key — the old deterministic fallback - // silently returned the first card's code for the second purchase. - // Clients who need retry-dedup supply their own idempotency_key; that - // path is unchanged. - base := fmt.Sprintf("gc-%s-%d-%s-%s", userID, req.Amount, req.RecipientType, cardPart) - req.IdempotencyKey = base + "-" + randomHexSuffix(6) - if len(req.IdempotencyKey) > 45 { - // Hash long keys to fit Square's 45-char limit - hash := sha256.Sum256([]byte(req.IdempotencyKey)) - req.IdempotencyKey = fmt.Sprintf("gc-%x", hash[:16]) - } - } - // Product rule (security): only verified accounts may save cards. An // unverified/guest/affiliate user may still buy a gift card, but // save_card=true is rejected here — before any charge source resolution. @@ -1046,8 +1037,12 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { // Bounded try-lock (R6) so a contended lock never blocks the pool across // the Square round-trip. lockKey := req.IdempotencyKey - if lockKey == "" { - lockKey = userID + if !clientSuppliedKey { + // Identical logical no-key purchases must serialize on the SAME lock + // key — the deterministic base, not a per-request random suffix — so + // concurrent lost-response retries cannot both derive a fresh slot and + // both charge. + lockKey = fmt.Sprintf("gc-%s-%d-%s-%s", userID, req.Amount, req.RecipientType, noKeyCardPart) } pinConn, lockOK := acquireBookingPaymentLock(ctx, w, "crussell:giftcard:"+lockKey, "Purchase in progress, try again") if !lockOK { @@ -1055,6 +1050,19 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { } defer releaseBookingPaymentLock(pinConn, "crussell:giftcard:"+lockKey) + // Derive the deterministic fallback key UNDER the lock so the spent-slot + // scan races no concurrent purchase (mirrors deriveBookingPaymentIdempotencyKey + // in handlers.go, which derives under the per-booking lock). + if !clientSuppliedKey { + derivedKey, dErr := deriveGiftCardIdempotencyKey(ctx, db.Conn, userID, req.Amount, req.RecipientType, noKeyCardPart) + if dErr != nil { + log.Printf("Failed to derive gift-card idempotency key: %v", dErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + req.IdempotencyKey = derivedKey + } + // Idempotency: only short-circuit when the existing record is 'completed'. // A 'pending' record means the previous Square call failed — returning it // as 200 would show a success without ever charging. Re-attempt below with @@ -1388,6 +1396,49 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { } } +// deriveGiftCardIdempotencyKey returns the deterministic fallback idempotency +// key for a no-client-key gift-card purchase: +// "gc----", sha256-truncated when the +// verbatim form exceeds Square's 45-char limit (the hash stays deterministic). +// +// The key must distinguish "same live purchase retried" (dedup) from "new +// purchase that happens to be identical" (new charge). The candidate is the +// base key (seq 0) then the base key with a "-" suffix (seq >= 1) until a +// slot without a COMPLETED purchase is found. A COMPLETED purchase always +// advances the sequence — two genuine identical no-key purchases (e.g. two +// £20 self cards) are distinct operations and must diverge onto distinct keys +// (the old random-suffix fallback's collapse fix), while a PENDING row never +// occupies a slot: a lost-response retry re-derives the base key, the +// idempotency lookup below reuses the pending row, and Square's same-key dedup +// returns the original charge — ONE charge instead of the old double-charge. +// Must be called under the crussell:giftcard advisory lock so the spent-slot +// scan races no concurrent purchase (mirrors deriveBookingPaymentIdempotencyKey +// in handlers.go). +func deriveGiftCardIdempotencyKey(ctx context.Context, q db.Querier, userID string, amount int64, recipientType, cardPart string) (string, error) { + baseKey := fmt.Sprintf("gc-%s-%d-%s-%s", userID, amount, recipientType, cardPart) + for seq := 0; ; seq++ { + candidate := baseKey + if seq > 0 { + candidate = fmt.Sprintf("%s-%d", baseKey, seq) + } + if len(candidate) > 45 { + hash := sha256.Sum256([]byte(candidate)) + candidate = fmt.Sprintf("gc-%x", hash[:16]) + } + var completedID string + err := q.QueryRow(ctx, ` + SELECT id FROM payments + WHERE created_by = $1 AND idempotency_key = $2 AND status = 'completed' + `, userID, candidate).Scan(&completedID) + if errors.Is(err, pgx.ErrNoRows) { + return candidate, nil + } + if err != nil { + return "", err + } + } +} + // --- Helpers --- // redactEmail masks an email for logs (PII — third-party addresses are not diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index d5c6504..4d2ee93 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -790,10 +790,10 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // Return the card details the frontend reads for the success state // (MINOR-R2) — CheckoutResponse alone leaves card_brand/card_last4 blank. if err := json.NewEncoder(w).Encode(map[string]any{ - "payment_id": paymentID, - "status": "COMPLETED", - "card_brand": paymentResult.CardBrand, - "card_last4": paymentResult.CardLast4, + "payment_id": paymentID, + "status": "COMPLETED", + "card_brand": paymentResult.CardBrand, + "card_last4": paymentResult.CardLast4, }); err != nil { log.Printf("Failed to encode JSON response: %v", err) } @@ -964,17 +964,31 @@ func activeTerminalCheckoutID(ctx context.Context, bookingID string) string { result, err := SquareClient.GetCheckout(ctx, checkoutID) switch { case err == nil && result.Status == "COMPLETED": - // C1 actually completed at the terminal. Mark the row COMPLETED - // so the booking's in-flight guard releases; the payment is - // recorded by GetCheckoutStatus on the poll path (mirrors the - // real-checkout COMPLETED handling below). - log.Printf("Provisional terminal checkout %s for booking %s is COMPLETED at Square — marking COMPLETED", checkoutID, bookingID) - if _, upErr := db.Conn.Exec(ctx, ` - UPDATE terminal_checkouts SET status = 'COMPLETED', updated_at = NOW() WHERE checkout_id = $1 - `, checkoutID); upErr != nil { - log.Printf("Failed to mark provisional terminal checkout %s completed: %v", checkoutID, upErr) + // C1 actually completed at the terminal. The payment MUST be + // recorded now — the old code only marked the row COMPLETED and + // relied on GetCheckoutStatus (the poll path) to record it, but a + // checkout that is never polled (abandoned booking / lost poll) + // would leave the charge permanently untracked: + // SweepStaleTerminalCheckouts only re-examines PENDING/IN_PROGRESS + // rows, so a row already marked COMPLETED here is never revisited + // and the money stays unrecorded and unrefundable via the app. + // Mirror the sweep's recordUntrackedTerminalPayment: the same + // crussell:terminal: advisory lock (serializes + // against a concurrent poll), the same dedup by booking_id + + // square_payment_id, the same PaymentRecord shape, the same + // deposit/balance/tip split, and the same fully-paid completion. + recorded := recordUntrackedTerminalPayment(ctx, checkoutID, bookingID, result) + if recorded { + log.Printf("Provisional terminal checkout %s for booking %s is COMPLETED at Square — payment recorded", checkoutID, bookingID) + return "" } - return "" + // Recording failed (transient DB/lock contention) — keep the + // in-flight guard UP so a second live checkout is never created + // while the charge is unrecorded. The row stays PENDING, so the + // stale-terminal sweep re-runs recordUntrackedTerminalPayment on + // it; once recorded, this guard releases on the next attempt. + log.Printf("Provisional terminal checkout %s for booking %s is COMPLETED at Square but payment recording failed — keeping it in flight; no second checkout until the charge is recorded", checkoutID, bookingID) + return checkoutID case errors.Is(err, square.ErrCheckoutPending): // C1 is still live at Square — reuse it instead of creating C2. log.Printf("Provisional terminal checkout %s for booking %s is live at Square — reusing it", checkoutID, bookingID) @@ -2443,6 +2457,23 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { return } + // Money-safety guard (M7): a payments row with NO booking is a gift-card + // purchase (BuyGiftCard inserts without a booking — the same discriminator + // the sweep uses in sweep.go). Refunding such a payment at Square returns + // the cash while the issued gift card and its balance credit stay live: + // £N paid out with the £N card still spendable = money created from + // nothing. The reversal alternative (delete the card + debit the pooled + // balance) is unsafe: a self-purchase is auto-redeemed into the account + // balance which may already be partially spent, and gift_card_transactions + // rows reference the card. Reject with a clear message directing the admin + // to the gift-card section, BEFORE any Square call or pending-refund row + // (this also blocks the dedup/resume paths below, which re-issue at + // Square). + if payment.BookingID == "" { + http.Error(w, "Cannot refund a gift-card purchase via payment refund. Refund gift-card purchases by cancelling the card in the gift-card section.", http.StatusBadRequest) + return + } + if payment.SquarePaymentID == nil { http.Error(w, "Payment has no Square reference", http.StatusBadRequest) return diff --git a/backend/handlers/payments/money_safety_fixes_test.go b/backend/handlers/payments/money_safety_fixes_test.go new file mode 100644 index 0000000..5637be3 --- /dev/null +++ b/backend/handlers/payments/money_safety_fixes_test.go @@ -0,0 +1,388 @@ +//go:build test && dev + +package payments + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "crussell/db" + "crussell/internal/square" + "crussell/mw" + "crussell/testutils" + "crussell/testutils/fixtures" + "crussell/testutils/jwt" + + "github.com/go-chi/chi/v5" + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/require" +) + +// ============================================================================= +// M7 — a manual refund of a gift-card purchase must never create money +// ============================================================================= + +// TestRefundPayment_GiftCardPurchase_Rejected locks the direct-handler guard: +// a completed gift-card purchase payment (payments row with NO booking_id) +// must be rejected with a clear message BEFORE any Square call or refund row — +// refunding it at Square would return the cash while the issued card + balance +// credit stay live (money created from nothing). +func TestRefundPayment_GiftCardPurchase_Rejected(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + + // Seed a completed gift-card purchase payment: NO booking_id, Square + // payment id set — exactly what BuyGiftCard inserts. + var paymentID string + err = tx.QueryRow(ctx, ` + INSERT INTO payments (payment_type, payment_method, status, amount, square_payment_id, created_by, created_at, updated_at) + VALUES ('full', 'online_square', 'completed', 10.00, 'pay_gc_purchase_refund_1', $1, NOW(), NOW()) + RETURNING id + `, userID).Scan(&paymentID) + require.NoError(t, err) + + adminToken := jwt.GenerateTestToken(adminID, "admin") + req := RefundRequest{Amount: 1000, Reason: "customer changed their mind"} + rec := makePaymentRequest(RefundPayment, "POST", "/api/admin/payments/"+paymentID+"/refund", req, adminToken, ctx) + require.Equal(t, http.StatusBadRequest, rec.Code, "a gift-card purchase must be rejected before any Square call, body: %s", rec.Body.String()) + + if !strings.Contains(rec.Body.String(), "gift-card") { + t.Errorf("expected the rejection message to direct the admin to the gift-card section, got %q", rec.Body.String()) + } + + // No refund row may be created for the blocked refund. + var refundCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM refunds WHERE payment_id = $1", paymentID).Scan(&refundCount)) + require.Equal(t, 0, refundCount, "no refund row may exist for a rejected gift-card purchase refund") +} + +// TestProcessManualPaymentGroup_GiftCardPurchase_NotReIssued locks the sweep +// resume path: a pending manual refund for a gift-card purchase payment must +// NEVER be re-issued at Square (the sweep is a bypass of the handler guard). +// The row is reconciled only — a COMPLETED refund at Square resolves it to +// completed; a genuine no-match fails it + admin-notifies. +func TestProcessManualPaymentGroup_GiftCardPurchase_NotReIssued(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + + const sqPayID = "pay_gc_purchase_sweep_1" + var paymentID string + err = tx.QueryRow(ctx, ` + INSERT INTO payments (payment_type, payment_method, status, amount, square_payment_id, idempotency_key, created_by, created_at, updated_at) + VALUES ('full', 'online_square', 'completed', 10.00, $1, 'gc-purchase-sweep-key', $2, NOW(), NOW()) + RETURNING id + `, sqPayID, userID).Scan(&paymentID) + require.NoError(t, err) + + var refundID string + err = tx.QueryRow(ctx, ` + INSERT INTO refunds (payment_id, booking_id, amount, status, reason, idempotency_key, created_by, created_at, origin) + VALUES ($1, NULL, 10.00, 'pending', 'manual refund of gift-card purchase', 'refund-key-1', $2, NOW() - INTERVAL '1 minute', 'manual') + RETURNING id + `, paymentID, adminID).Scan(&refundID) + require.NoError(t, err) + + pgxTx := db.TxFromContext(ctx) + require.NotNil(t, pgxTx) + require.NoError(t, pgxTx.Commit(ctx)) + + freshCtx := context.Background() + t.Cleanup(func() { + _, _ = db.Conn.Exec(freshCtx, `DELETE FROM admin_notifications WHERE reason = 'refund_failed' AND booking_id IS NULL AND created_at > NOW() - INTERVAL '1 hour'`) + _, _ = db.Conn.Exec(freshCtx, `DELETE FROM refunds WHERE id = $1`, refundID) + _, _ = db.Conn.Exec(freshCtx, `DELETE FROM payments WHERE id = $1`, paymentID) + _, _ = db.Conn.Exec(freshCtx, `DELETE FROM users WHERE id = $1`, userID) + }) + + t.Run("no_completed_refund_at_square_fails_without_reissue", func(t *testing.T) { + origClient := SquareClient + mock := square.NewDevClient().(*square.MockClient) + counting := &countingRefundClient{SquareClient: mock} + SquareClient = counting + defer func() { SquareClient = origClient }() + + n, err := processManualPaymentGroup(freshCtx, paymentID, []manualPendingRow{{ID: refundID}}) + require.NoError(t, err) + require.Equal(t, 0, n, "a blocked gift-card-purchase refund must not count as money-moved") + + if calls := counting.refundCalls(); len(calls) != 0 { + t.Fatalf("expected NO Square refund re-issue for a gift-card purchase, got %d refund call(s)", len(calls)) + } + + var status string + require.NoError(t, db.Conn.QueryRow(freshCtx, `SELECT status FROM refunds WHERE id = $1`, refundID).Scan(&status)) + require.Equal(t, "failed", status, "a gift-card-purchase manual refund with no Square refund must be failed, never re-issued") + + var notifCount int + require.NoError(t, db.Conn.QueryRow(freshCtx, `SELECT COUNT(*) FROM admin_notifications WHERE reason = 'refund_failed' AND booking_id IS NULL`).Scan(¬ifCount)) + require.Equal(t, 1, notifCount, "the blocked refund must surface an admin notification") + }) + + t.Run("completed_refund_at_square_resolves_completed", func(t *testing.T) { + // Put the row back to pending (the previous sub-run failed it). + _, err := db.Conn.Exec(freshCtx, `UPDATE refunds SET status = 'pending', refund_attempts = 0 WHERE id = $1`, refundID) + require.NoError(t, err) + _, err = db.Conn.Exec(freshCtx, `DELETE FROM admin_notifications WHERE reason = 'refund_failed' AND booking_id IS NULL`) + require.NoError(t, err) + + origClient := SquareClient + mock := square.NewDevClient().(*square.MockClient) + // Seed a COMPLETED refund at Square directly on the mock (this is the + // historical attempt's money already having moved — NOT a re-issue). + _, err = mock.RefundPayment(freshCtx, square.RefundPaymentReq{ + PaymentID: sqPayID, + Amount: 1000, + IdempotencyKey: "seed-refund-gc-purchase", + Reason: "historical", + }) + require.NoError(t, err) + + counting := &countingRefundClient{SquareClient: mock} + SquareClient = counting + defer func() { SquareClient = origClient }() + + n, err := processManualPaymentGroup(freshCtx, paymentID, []manualPendingRow{{ID: refundID}}) + require.NoError(t, err) + require.Equal(t, 1, n, "the reconciled COMPLETED refund resolves the row") + + if calls := counting.refundCalls(); len(calls) != 0 { + t.Fatalf("expected NO Square refund re-issue even with a completed refund at Square, got %d refund call(s)", len(calls)) + } + + var status string + require.NoError(t, db.Conn.QueryRow(freshCtx, `SELECT status FROM refunds WHERE id = $1`, refundID).Scan(&status)) + require.Equal(t, "completed", status, "an exact COMPLETED refund at Square resolves the row to completed") + }) +} + +// ============================================================================= +// M7 — BuyGiftCard no-client-key fallback: retry dedup without collapse +// ============================================================================= + +// buyGiftCardNoKey issues a BuyGiftCard request without a client-supplied +// idempotency key and returns the response code + body. +func buyGiftCardNoKey(t *testing.T, ctx context.Context, tx pgx.Tx, token string, amount int, recipientType string) (int, string) { + t.Helper() + reqBody, _ := json.Marshal(map[string]interface{}{ + "amount": amount, + "recipient_type": recipientType, + "new_card_token": "cnon:card-nonce-ok", + }) + req := httptest.NewRequest("POST", "/api/user/giftcards/buy", bytes.NewBuffer(reqBody)) + req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(db.ContextWithTx(req.Context(), tx)) + w := httptest.NewRecorder() + r := chi.NewRouter() + r.Use(mw.RequireAuth) + r.Post("/api/user/giftcards/buy", BuyGiftCard) + r.ServeHTTP(w, req) + return w.Code, w.Body.String() +} + +// TestBuyGiftCard_NoClientKey_LostResponseRetry_SingleCharge locks the M7 fix: +// a no-client-key purchase whose Square response is lost (charge COMMITTED at +// Square, handler saw a 503, payment row left pending) must, when retried with +// the identical body, re-derive the SAME deterministic fallback key, reuse the +// pending row, and land ONE Square charge — the old fresh-random-suffix +// fallback generated a new key per retry and charged twice. +func TestBuyGiftCard_NoClientKey_LostResponseRetry_SingleCharge(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + token := jwt.GenerateTestToken(userID, "verified_email") + + origClient := SquareClient + mock := square.NewDevClient().(*square.MockClient) + mock.FailAfterCommit = true + counting := &countingPaymentClient{SquareClient: mock} + SquareClient = counting + defer func() { SquareClient = origClient }() + + // Attempt 1: Square commits the charge under the derived key but the + // response is lost — the payment row stays pending. + if code, body := buyGiftCardNoKey(t, ctx, tx.(pgx.Tx), token, 2000, "self"); code == http.StatusCreated { + t.Fatalf("expected the lost-response attempt to fail (charge committed, response lost), got 201: %s", body) + } + + mock.FailAfterCommit = false + + // Retry: deterministic key reuse + Square dedup → the retry succeeds. + if code, body := buyGiftCardNoKey(t, ctx, tx.(pgx.Tx), token, 2000, "self"); code != http.StatusCreated { + t.Fatalf("expected the retry to succeed, got %d: %s", code, body) + } + + // Exactly ONE payment row, ONE idempotency key, ONE issued card. + var payCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE created_by = $1", userID).Scan(&payCount)) + require.Equal(t, 1, payCount, "exactly one payment row for a lost-response retry") + + var keyCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(DISTINCT idempotency_key) FROM payments WHERE created_by = $1", userID).Scan(&keyCount)) + require.Equal(t, 1, keyCount, "both attempts must share ONE deterministic fallback key") + + var cardCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM gift_cards WHERE created_by = $1", userID).Scan(&cardCount)) + require.Equal(t, 1, cardCount, "exactly one gift card issued") + + // Both Square calls reused the SAME key; only ONE successful payment exists. + keys := counting.keys + if len(keys) != 2 { + t.Fatalf("expected 2 CreatePayment calls (attempt + retry), got %d", len(keys)) + } + if keys[0] != keys[1] { + t.Errorf("expected the retry to reuse the fallback key %q, got %q — a fresh key per retry is the double-charge bug", keys[0], keys[1]) + } + if len(counting.payments) != 1 { + t.Errorf("expected exactly ONE successful Square charge, got %d", len(counting.payments)) + } +} + +// TestBuyGiftCard_NoClientKey_DifferentAmounts_DistinctCharges locks the other +// half of the tradeoff: two genuinely DISTINCT no-key purchases (different +// amounts) must diverge onto distinct deterministic keys and issue two charges. +func TestBuyGiftCard_NoClientKey_DifferentAmounts_DistinctCharges(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + token := jwt.GenerateTestToken(userID, "verified_email") + + for _, amount := range []int{2000, 5000} { + if code, body := buyGiftCardNoKey(t, ctx, tx.(pgx.Tx), token, amount, "self"); code != http.StatusCreated { + t.Fatalf("expected 201 for £%d purchase, got %d: %s", amount/100, code, body) + } + } + + var keyCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(DISTINCT idempotency_key) FROM payments WHERE created_by = $1", userID).Scan(&keyCount)) + require.Equal(t, 2, keyCount, "two different-amount no-key purchases must get distinct keys") + + var payCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE created_by = $1", userID).Scan(&payCount)) + require.Equal(t, 2, payCount) + + var cardCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM gift_cards WHERE created_by = $1", userID).Scan(&cardCount)) + require.Equal(t, 2, cardCount) +} + +// TestBuyGiftCard_NoClientKey_DifferentRecipients_DistinctCharges locks the +// distinct-recipient branch of the tradeoff. +func TestBuyGiftCard_NoClientKey_DifferentRecipients_DistinctCharges(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + token := jwt.GenerateTestToken(userID, "verified_email") + + for _, recipient := range []string{"self", "friend"} { + if code, body := buyGiftCardNoKey(t, ctx, tx.(pgx.Tx), token, 2000, recipient); code != http.StatusCreated { + t.Fatalf("expected 201 for %s purchase, got %d: %s", recipient, code, body) + } + } + + var keyCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(DISTINCT idempotency_key) FROM payments WHERE created_by = $1", userID).Scan(&keyCount)) + require.Equal(t, 2, keyCount, "different-recipient no-key purchases must get distinct keys") + + var payCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE created_by = $1", userID).Scan(&payCount)) + require.Equal(t, 2, payCount) + + var cardCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM gift_cards WHERE created_by = $1", userID).Scan(&cardCount)) + require.Equal(t, 2, cardCount) +} + +// ============================================================================= +// H4 — a COMPLETED provisional terminal checkout must be recorded, not just +// released +// ============================================================================= + +// TestActiveTerminalCheckoutID_ProvisionalCompleted_RecordsPayment locks the +// H4 fix: when activeTerminalCheckoutID discovers a provisional (tmp-) +// checkout COMPLETED at Square, it must RECORD the payment (mirroring the +// sweep's recordUntrackedTerminalPayment) instead of only marking the row +// COMPLETED and relying on a poll that may never come — a never-polled +// checkout would otherwise leave the charge permanently unrecorded and +// unrefundable via the app. +func TestActiveTerminalCheckoutID_ProvisionalCompleted_RecordsPayment(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + serviceID, err := fixtures.CreateTestService(tx) + require.NoError(t, err) + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + require.NoError(t, err) + if _, err := tx.Exec(ctx, "UPDATE bookings SET status = 'in_progress' WHERE id = $1", bookingID); err != nil { + t.Fatalf("failed to set booking in_progress: %v", err) + } + + const tmpID = "tmp-completed-no-poll" + seedStaleProvisionalTerminalCheckout(t, ctx, tx, bookingID, tmpID) + + const sqPayID = "pay_provisional_completed_1" + origClient := SquareClient + SquareClient = &provisionalCheckoutClient{ + SquareClient: square.NewDevClient(), + checkoutID: tmpID, + result: &square.PaymentResult{ + Status: "COMPLETED", + SquarePayID: sqPayID, + Amount: 5000, + Fees: 88, + CardBrand: "VISA", + CardLast4: "4242", + ReferenceID: bookingID, + }, + } + defer func() { SquareClient = origClient }() + + pgxTx := db.TxFromContext(ctx) + require.NotNil(t, pgxTx) + require.NoError(t, pgxTx.Commit(ctx)) + + freshCtx := context.Background() + t.Cleanup(func() { + _, _ = db.Conn.Exec(freshCtx, `DELETE FROM payments WHERE square_payment_id = $1`, sqPayID) + _, _ = db.Conn.Exec(freshCtx, `DELETE FROM terminal_checkouts WHERE checkout_id = $1`, tmpID) + _, _ = db.Conn.Exec(freshCtx, `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(freshCtx, `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(freshCtx, `DELETE FROM users WHERE id = $1`, userID) + }) + + // A recorded COMPLETED provisional checkout must release the in-flight + // guard ("" returned), not wedge the booking. + got := activeTerminalCheckoutID(freshCtx, bookingID) + require.Equal(t, "", got, "a recorded COMPLETED provisional checkout must release the in-flight guard") + + // The charge must be recorded as a payment row. + var payCount int + require.NoError(t, db.Conn.QueryRow(freshCtx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND square_payment_id = $2`, bookingID, sqPayID).Scan(&payCount)) + require.Equal(t, 1, payCount, "the never-polled COMPLETED provisional charge must be recorded as a payment") + + var status string + require.NoError(t, db.Conn.QueryRow(freshCtx, `SELECT status FROM terminal_checkouts WHERE checkout_id = $1`, tmpID).Scan(&status)) + require.Equal(t, "COMPLETED", status, "the recorded checkout row must be marked COMPLETED") +} diff --git a/backend/handlers/payments/payments_r6_review_test.go b/backend/handlers/payments/payments_r6_review_test.go index 237b761..88a96f0 100644 --- a/backend/handlers/payments/payments_r6_review_test.go +++ b/backend/handlers/payments/payments_r6_review_test.go @@ -185,20 +185,16 @@ func TestActiveTerminalCheckoutID_ResolvesProvisionalRow(t *testing.T) { require.Equal(t, "failed", status, "the provisional row must be marked failed") } -// failingGetCheckoutClient errors on every GetCheckout — if the sweep wrongly -// resolves a provisional row against Square, the row is left pending (the error -// is not a terminal checkout state) and the test fails. -type failingGetCheckoutClient struct { - square.SquareClient -} - -func (c *failingGetCheckoutClient) GetCheckout(ctx context.Context, checkoutID string) (*square.PaymentResult, error) { - return nil, fmt.Errorf("GetCheckout was called for %s — provisional rows must resolve without a Square round-trip", checkoutID) -} - -// TestSweepStaleTerminalCheckouts_ResolvesProvisionalRowWithoutSquare proves -// the sweep resolves a stale provisional row to failed without calling Square. -func TestSweepStaleTerminalCheckouts_ResolvesProvisionalRowWithoutSquare(t *testing.T) { +// TestSweepStaleTerminalCheckouts_TmpProvisional_ResolvesAgainstSquare locks +// the H4 fix: a provisional "tmp-" terminal_checkouts row is resolved against +// Square FIRST, never blind-failed. A hard crash between the row insert and +// the provisional→real UPDATE can leave a LIVE checkout at Square (created +// under the idempotency key embedded in the tmp id) while the row still +// carries the synthetic id — so a tmp- row is NO LONGER provably not live. +// A tmp- id Square has never seen (NOT_FOUND — the crash happened before the +// Square call, or the id cannot be resolved) is the expected outcome and is +// safely resolved to failed. +func TestSweepStaleTerminalCheckouts_TmpProvisional_ResolvesAgainstSquare(t *testing.T) { ctx, tx := testutils.SetupTestTx(t) userID, bookingID, serviceID := setupTestData(t, ctx, tx) @@ -230,8 +226,12 @@ func TestSweepStaleTerminalCheckouts_ResolvesProvisionalRowWithoutSquare(t *test t.Fatalf("failed to clean leftover stale till sales: %v", err) } + // A fresh mock holds no checkout under the tmp id → GetCheckout returns + // the mock's plain "checkout not found" error (NOT_FOUND), the expected + // outcome for a tmp- id that never reached Square → the row is resolved + // to failed. origClient := SquareClient - SquareClient = &failingGetCheckoutClient{SquareClient: square.NewDevClient()} + SquareClient = square.NewDevClient() defer func() { SquareClient = origClient }() n, err := SweepStaleTerminalCheckouts(freshCtx) diff --git a/backend/handlers/payments/payments_round7_test.go b/backend/handlers/payments/payments_round7_test.go new file mode 100644 index 0000000..7ead907 --- /dev/null +++ b/backend/handlers/payments/payments_round7_test.go @@ -0,0 +1,692 @@ +//go:build test && dev + +package payments + +import ( + "context" + "crypto/sha256" + "fmt" + "net/http" + "net/http/httptest" + "reflect" + "strings" + "testing" + "time" + + "crussell/clock" + "crussell/db" + "crussell/internal/square" + "crussell/testutils" + "crussell/testutils/fixtures" + + "github.com/stretchr/testify/require" +) + +// ============================================================================= +// R7 — CreateBookingPayment refunded-dedup 409 guard (paymentHasLiveRefund) +// ============================================================================= + +// TestCreateBookingPayment_RefundedDedup_409 locks the money-safety guard on +// the idempotent dedup path: a client-supplied idempotency_key that matches a +// COMPLETED payment which has since been refunded must NOT be reported as +// success (that would silently swallow a new equal-amount charge — the booking +// shows paid with no money collected). The replay is rejected with 409 and no +// new payment row is created. +func TestCreateBookingPayment_RefundedDedup_409(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + _, bookingID, userToken := setupPaymentStatusTest(t, ctx, tx, "confirmed") + + const key = "client-uuid-refunded-dedup" + cardToken := "cnon:refunded-dedup-card" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + IdempotencyKey: key, + } + handler := CreateBookingPayment + + w1 := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w1.Code, "initial charge must succeed, body: %s", w1.Body.String()) + + // A £25 deposit on the £50 fixture booking leaves the booking confirmed + // (not fully paid), so the retry reaches the GENERAL dedup path (with the + // refund re-validation) rather than the completed-booking short-circuit. + var status string + require.NoError(t, tx.QueryRow(ctx, `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&status)) + require.Equal(t, "confirmed", status, "the replay must hit the general dedup path") + + var paymentID string + require.NoError(t, tx.QueryRow(ctx, `SELECT id FROM payments WHERE booking_id = $1 AND idempotency_key = $2 AND status = 'completed'`, bookingID, key).Scan(&paymentID)) + + // The admin refunds the payment — a live (completed) refund row now makes + // the payment's money no longer collectable. + _, err := tx.Exec(ctx, ` + INSERT INTO refunds (payment_id, booking_id, amount, status, reason, created_at) + VALUES ($1, $2, 25.00, 'completed', 'admin refund', NOW()) + `, paymentID, bookingID) + require.NoError(t, err) + + // Same key + same amount retry: MUST NOT return the refunded payment as + // success. + w2 := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusConflict, w2.Code, "a refunded-payment replay must 409, body: %s", w2.Body.String()) + require.Contains(t, w2.Body.String(), "refunded and can no longer be replayed") + + // No new payment row may be created by the rejected replay. + var keyCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND idempotency_key = $2`, bookingID, key).Scan(&keyCount)) + require.Equal(t, 1, keyCount, "the rejected replay must not create a second payment row") +} + +// ============================================================================= +// R7 — sweep keyed blind-fail for rows past Square's key-retention window +// ============================================================================= + +// TestSweepStalePendingPayments_KeyedPastRetention_BlindFails locks the +// payments-table blind-fail: a stale pending row with a stored idempotency_key +// but no square_payment_id that is ALREADY older than Square's ~24h key +// retention window when swept is marked 'failed' WITHOUT a replay reconcile +// (replaying an expired key would misread the probe rejection as "never +// charged"), and it counts toward the unverifiable WARN accounting. +func TestSweepStalePendingPayments_KeyedPastRetention_BlindFails(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + serviceID, err := fixtures.CreateTestService(tx) + require.NoError(t, err) + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + require.NoError(t, err) + + staleID, err := fixtures.CreateTestPayment(tx, bookingID, 2000.00, "online_square", "full", "pending") + require.NoError(t, err) + // 25h old: past stalePendingPaymentAge (24h), so replayExpired (now-24h) is + // already in the past even though the replay mock is FRESH — the row must be + // blind-failed, never replayed. + if _, err := tx.Exec(ctx, "UPDATE payments SET created_at = NOW() - INTERVAL '25 hours', idempotency_key = 'key-past-retention-pay', square_source_id = 'cnon:test-card' WHERE id = $1", staleID); err != nil { + t.Fatalf("failed to age the stale payment: %v", err) + } + + // Fresh mock: has no payment under the key, but the blind-fail must happen + // WITHOUT any replay because the retention window already closed. + origClient := SquareClient + SquareClient = square.NewDevClient() + defer func() { SquareClient = origClient }() + + pgxTx := db.TxFromContext(ctx) + require.NotNil(t, pgxTx) + require.NoError(t, pgxTx.Commit(ctx)) + + t.Cleanup(func() { + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM payments WHERE id = $1`, staleID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM users WHERE id = $1`, userID) + }) + + // The past-retention blind-fail must be counted in the unverifiable + // accounting (resolved, NOT completed). + resolved, completed, unverifiable, err := sweepKeyedStaleRows(context.Background(), "payments", clock.Now().Add(-stalePendingKeyedAge)) + require.NoError(t, err) + require.Equal(t, 1, resolved, "the past-retention keyed row must be resolved by the sweep") + require.Equal(t, 0, completed, "a past-retention keyed row can never be rescued to completed") + require.Equal(t, 1, unverifiable, "the past-retention blind-fail must count toward the unverifiable WARN accounting") + + var rowStatus string + require.NoError(t, db.Conn.QueryRow(context.Background(), `SELECT status FROM payments WHERE id = $1`, staleID).Scan(&rowStatus)) + require.Equal(t, "failed", rowStatus, "a keyed row past Square's retention window must be blind-failed") + + // No reconcile happened: the row must not carry a square_payment_id. + var sqPayID *string + require.NoError(t, db.Conn.QueryRow(context.Background(), `SELECT square_payment_id FROM payments WHERE id = $1`, staleID).Scan(&sqPayID)) + require.Nil(t, sqPayID, "a blind-failed row must never receive a square_payment_id") +} + +// TestSweepStalePendingPayments_KeyedTillPastRetention_BlindFail_NoClawback +// locks the same blind-fail for a KEYED till sale: the sale is marked 'failed' +// and its funded gift card is NOT clawed back — unlike the proven-failure path, +// the blind-fail's charge outcome is unknown, so the funding must stay put. +func TestSweepStalePendingPayments_KeyedTillPastRetention_BlindFail_NoClawback(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + pool := context.Background() + + // seedStaleTillSaleWithCard already ages both the sale and its created gift + // card to 25h (create) — past Square's 24h retention window. Add the stored + // idempotency key to put it on the keyed pass. + saleID, giftCardID := seedStaleTillSaleWithCard(t, ctx, tx, adminID, 50.00, "", true) + if _, err := tx.Exec(ctx, "UPDATE till_sales SET idempotency_key = 'key-past-retention-till' WHERE id = $1", saleID); err != nil { + t.Fatalf("failed to set the till sale idempotency key: %v", err) + } + + origClient := SquareClient + SquareClient = square.NewDevClient() + defer func() { SquareClient = origClient }() + + pgxTx := db.TxFromContext(ctx) + require.NotNil(t, pgxTx) + require.NoError(t, pgxTx.Commit(ctx)) + + if _, err := SweepStalePendingPayments(pool); err != nil { + t.Fatalf("sweep failed: %v", err) + } + + var status string + require.NoError(t, db.Conn.QueryRow(pool, `SELECT status FROM till_sales WHERE id = $1`, saleID).Scan(&status)) + require.Equal(t, "failed", status, "a keyed till sale past retention must be blind-failed") + + var cardCount int + require.NoError(t, db.Conn.QueryRow(pool, `SELECT COUNT(*) FROM gift_cards WHERE id = $1`, giftCardID).Scan(&cardCount)) + require.Equal(t, 1, cardCount, "the blind-failed till sale's funded gift card must NOT be clawed back (charge outcome unknown)") +} + +// ============================================================================= +// R7 — reconcile tri-state status outcomes (by key AND by square_payment_id) +// ============================================================================= + +// TestSweepStalePendingPayments_ReconcileByKey_TriState locks the status +// switch in reconcileStalePaymentByKey: after a successful keyed replay, +// CANCELED/FAILED are definitive failures (row marked failed), APPROVED/PENDING +// are non-terminal (row left pending), and any unknown status is failed. +func TestSweepStalePendingPayments_ReconcileByKey_TriState(t *testing.T) { + cases := []struct { + name string + status string + wantFinal string // "failed" or "pending" + }{ + {name: "canceled_marks_failed", status: "CANCELED", wantFinal: "failed"}, + {name: "failed_marks_failed", status: "FAILED", wantFinal: "failed"}, + {name: "approved_leaves_pending", status: "APPROVED", wantFinal: "pending"}, + {name: "pending_leaves_pending", status: "PENDING", wantFinal: "pending"}, + {name: "unknown_status_marks_failed", status: "WEIRD", wantFinal: "failed"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + serviceID, err := fixtures.CreateTestService(tx) + require.NoError(t, err) + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + require.NoError(t, err) + + staleID, err := fixtures.CreateTestPayment(tx, bookingID, 2000.00, "online_square", "full", "pending") + require.NoError(t, err) + key := "key-replay-tri-" + tc.name + // 23h old: past the 22h keyed cutoff (so the keyed pass picks it up) + // but still inside Square's 24h retention window (so the replay runs). + if _, err := tx.Exec(ctx, "UPDATE payments SET created_at = NOW() - INTERVAL '23 hours', idempotency_key = $1, square_source_id = 'cnon:test-card' WHERE id = $2", key, staleID); err != nil { + t.Fatalf("failed to age the stale payment: %v", err) + } + + origClient := SquareClient + SquareClient = &staleReplayClient{SquareClient: square.NewDevClient(), result: &square.PaymentResult{Status: tc.status}} + defer func() { SquareClient = origClient }() + + pgxTx := db.TxFromContext(ctx) + require.NotNil(t, pgxTx) + require.NoError(t, pgxTx.Commit(ctx)) + + t.Cleanup(func() { + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM payments WHERE id = $1`, staleID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM users WHERE id = $1`, userID) + }) + + if _, err := SweepStalePendingPayments(context.Background()); err != nil { + t.Fatalf("sweep failed: %v", err) + } + + var rowStatus string + require.NoError(t, db.Conn.QueryRow(context.Background(), `SELECT status FROM payments WHERE id = $1`, staleID).Scan(&rowStatus)) + require.Equal(t, tc.wantFinal, rowStatus, "replay-by-key status %q must leave the row %q", tc.status, tc.wantFinal) + }) + } +} + +// TestSweepStalePendingPayments_ReconcileByPaymentID_TriStateStatuses locks the +// same status switch in reconcileStalePaymentAtSquare (rows reconciled by their +// stored square_payment_id). +func TestSweepStalePendingPayments_ReconcileByPaymentID_TriStateStatuses(t *testing.T) { + cases := []struct { + name string + status string + wantFinal string + }{ + {name: "canceled_marks_failed", status: "CANCELED", wantFinal: "failed"}, + {name: "failed_marks_failed", status: "FAILED", wantFinal: "failed"}, + {name: "approved_leaves_pending", status: "APPROVED", wantFinal: "pending"}, + {name: "pending_leaves_pending", status: "PENDING", wantFinal: "pending"}, + {name: "unknown_status_marks_failed", status: "WEIRD", wantFinal: "failed"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + serviceID, err := fixtures.CreateTestService(tx) + require.NoError(t, err) + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + require.NoError(t, err) + + staleID, err := fixtures.CreateTestPayment(tx, bookingID, 2000.00, "online_square", "full", "pending") + require.NoError(t, err) + sqPayID := "sqp_reconcile_tri_" + tc.name + // 25h old: past the 24h pass-2 cutoff, with a square_payment_id so + // the reconcile runs by payment id (not by key). + if _, err := tx.Exec(ctx, "UPDATE payments SET created_at = NOW() - INTERVAL '25 hours', square_payment_id = $1 WHERE id = $2", sqPayID, staleID); err != nil { + t.Fatalf("failed to age the stale payment: %v", err) + } + + origClient := SquareClient + SquareClient = &staleGetPaymentClient{SquareClient: square.NewDevClient(), result: &square.PaymentResult{Status: tc.status, SquarePayID: sqPayID}} + defer func() { SquareClient = origClient }() + + pgxTx := db.TxFromContext(ctx) + require.NotNil(t, pgxTx) + require.NoError(t, pgxTx.Commit(ctx)) + + t.Cleanup(func() { + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM payments WHERE id = $1`, staleID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM users WHERE id = $1`, userID) + }) + + if _, err := SweepStalePendingPayments(context.Background()); err != nil { + t.Fatalf("sweep failed: %v", err) + } + + var rowStatus string + require.NoError(t, db.Conn.QueryRow(context.Background(), `SELECT status FROM payments WHERE id = $1`, staleID).Scan(&rowStatus)) + require.Equal(t, tc.wantFinal, rowStatus, "by-payment-id status %q must leave the row %q", tc.status, tc.wantFinal) + }) + } +} + +// ============================================================================= +// R7 — resolveChargeSource Square-failure branches (all 500 + ok=false) +// ============================================================================= + +// round7SquareFailureClient forces CreateCustomer / CreateCardOnFile to return +// errors so the resolveChargeSource error branches can be exercised +// deterministically (the embedding pattern mirrors staleGetPaymentClient / +// staleReplayClient). +type round7SquareFailureClient struct { + square.SquareClient + failCreateCustomer bool + failCreateCard bool +} + +func (c *round7SquareFailureClient) CreateCustomer(ctx context.Context, name, email string) (*square.CustomerResult, error) { + if c.failCreateCustomer { + return nil, fmt.Errorf("square: network error creating customer") + } + return c.SquareClient.CreateCustomer(ctx, name, email) +} + +func (c *round7SquareFailureClient) CreateCardOnFile(ctx context.Context, userID, cardToken, customerID string) (*square.CardOnFile, error) { + if c.failCreateCard { + return nil, fmt.Errorf("square: network error creating card-on-file") + } + return c.SquareClient.CreateCardOnFile(ctx, userID, cardToken, customerID) +} + +func TestResolveChargeSource_SquareFailureBranches_500NoOrphan(t *testing.T) { + ctx := context.Background() + + t.Run("ensure_square_customer_failure", func(t *testing.T) { + userID, err := fixtures.CreateTestUser(db.Conn) + require.NoError(t, err) + defer func() { + InvalidateSquareCustomerCache(userID) + _, _ = db.Conn.Exec(ctx, `DELETE FROM user_saved_cards WHERE user_id = $1`, userID) + _, _ = db.Conn.Exec(ctx, `DELETE FROM users WHERE id = $1`, userID) + }() + + origClient := SquareClient + SquareClient = &round7SquareFailureClient{SquareClient: square.NewDevClient(), failCreateCustomer: true} + defer func() { SquareClient = origClient }() + + token := "cnon:r7-customer-fail" + w := httptest.NewRecorder() + sourceID, savedCardID, sqCustID, ok := resolveChargeSource(ctx, w, NewPaymentService(), userID, &token, nil, true, "") + require.False(t, ok, "an EnsureSquareCustomer failure must fail source resolution") + require.Equal(t, http.StatusInternalServerError, w.Code) + require.Empty(t, sourceID) + require.Nil(t, savedCardID) + require.Empty(t, sqCustID) + + var rows int + require.NoError(t, db.Conn.QueryRow(ctx, `SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1`, userID).Scan(&rows)) + require.Zero(t, rows, "no orphan saved-card row may be created when customer provisioning fails") + }) + + t.Run("create_card_on_file_failure", func(t *testing.T) { + userID, err := fixtures.CreateTestUser(db.Conn) + require.NoError(t, err) + defer func() { + InvalidateSquareCustomerCache(userID) + _, _ = db.Conn.Exec(ctx, `DELETE FROM user_saved_cards WHERE user_id = $1`, userID) + _, _ = db.Conn.Exec(ctx, `DELETE FROM users WHERE id = $1`, userID) + }() + // Provisioning succeeds from the cache; the card-on-file creation fails. + squareCustomerCache.Store(userID, "cus_r7_createcard") + defer InvalidateSquareCustomerCache(userID) + + origClient := SquareClient + SquareClient = &round7SquareFailureClient{SquareClient: square.NewDevClient(), failCreateCard: true} + defer func() { SquareClient = origClient }() + + token := "cnon:r7-createcard-fail" + w := httptest.NewRecorder() + sourceID, savedCardID, sqCustID, ok := resolveChargeSource(ctx, w, NewPaymentService(), userID, &token, nil, true, "") + require.False(t, ok, "a CreateCardOnFile failure must fail source resolution") + require.Equal(t, http.StatusInternalServerError, w.Code) + require.Empty(t, sourceID) + require.Nil(t, savedCardID) + require.Empty(t, sqCustID) + + var rows int + require.NoError(t, db.Conn.QueryRow(ctx, `SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1`, userID).Scan(&rows)) + require.Zero(t, rows, "no orphan saved-card row may be created when the Square card creation fails") + }) + + t.Run("saved_card_lazy_provisioning_failure", func(t *testing.T) { + userID, err := fixtures.CreateTestUser(db.Conn) + require.NoError(t, err) + defer func() { + InvalidateSquareCustomerCache(userID) + _, _ = db.Conn.Exec(ctx, `DELETE FROM user_saved_cards WHERE user_id = $1`, userID) + _, _ = db.Conn.Exec(ctx, `DELETE FROM users WHERE id = $1`, userID) + }() + // A pre-P14 saved-card row with an EMPTY square_customer_id: charging it + // requires lazy provisioning, which fails here. + cardID, err := fixtures.CreateTestPaymentMethod(db.Conn, userID, "ccof:r7-legacy-card", "VISA", "4242") + require.NoError(t, err) + + origClient := SquareClient + SquareClient = &round7SquareFailureClient{SquareClient: square.NewDevClient(), failCreateCustomer: true} + defer func() { SquareClient = origClient }() + + w := httptest.NewRecorder() + sourceID, savedCardID, sqCustID, ok := resolveChargeSource(ctx, w, NewPaymentService(), userID, nil, &cardID, false, "Card not found") + require.False(t, ok, "a lazy-provisioning failure must fail source resolution") + require.Equal(t, http.StatusInternalServerError, w.Code) + require.Empty(t, sourceID) + require.Nil(t, savedCardID) + require.Empty(t, sqCustID) + + // The pre-existing saved card must remain, still un-provisioned. + var rows int + require.NoError(t, db.Conn.QueryRow(ctx, `SELECT COUNT(*) FROM user_saved_cards WHERE id = $1 AND user_id = $2`, cardID, userID).Scan(&rows)) + require.Equal(t, 1, rows, "the existing saved card must not be deleted by the failed provisioning") + var custID *string + require.NoError(t, db.Conn.QueryRow(ctx, `SELECT square_customer_id FROM user_saved_cards WHERE id = $1`, cardID).Scan(&custID)) + require.Nil(t, custID, "the failed provisioning must not persist a Square customer id") + }) + + t.Run("get_card_non_404_failure", func(t *testing.T) { + userID, err := fixtures.CreateTestUser(db.Conn) + require.NoError(t, err) + defer func() { + _, _ = db.Conn.Exec(ctx, `DELETE FROM users WHERE id = $1`, userID) + }() + + // A cancelled context makes the GetCardByID query fail with a non-404 + // error (context canceled), exercising the 500 branch (NOT the 404 + // no-rows branch). + cancelCtx, cancel := context.WithCancel(context.Background()) + cancel() + + cardID := "nonexistent-card-id" + w := httptest.NewRecorder() + sourceID, savedCardID, sqCustID, ok := resolveChargeSource(cancelCtx, w, NewPaymentService(), userID, nil, &cardID, false, "Card not found") + require.False(t, ok, "a non-404 GetCardByID failure must fail source resolution") + require.Equal(t, http.StatusInternalServerError, w.Code) + require.Empty(t, sourceID) + require.Nil(t, savedCardID) + require.Empty(t, sqCustID) + }) +} + +// ============================================================================= +// R7 — CreateBookingPayment end-to-end charge-failure classification +// ============================================================================= + +// round7ChargeClient forces CreatePayment to return a fixed error so the +// charge-failure status mapping can be exercised end to end through the +// handler (the pending record must stay pending on every failure). +type round7ChargeClient struct { + square.SquareClient + createErr error +} + +func (c *round7ChargeClient) CreatePayment(ctx context.Context, req square.CreatePaymentReq) (*square.PaymentResult, error) { + if c.createErr != nil { + return nil, c.createErr + } + return c.SquareClient.CreatePayment(ctx, req) +} + +// round7CancelAwareChargeClient blocks inside CreatePayment until the request +// context is cancelled and then returns ctx.Err() — simulating Square hanging +// until the client gives up mid-charge. +type round7CancelAwareChargeClient struct { + square.SquareClient + entered chan struct{} +} + +func (c *round7CancelAwareChargeClient) CreatePayment(ctx context.Context, req square.CreatePaymentReq) (*square.PaymentResult, error) { + close(c.entered) + <-ctx.Done() + return nil, ctx.Err() +} + +// structuredSquareErrorWithCode builds a structured *square.squareAPIError of +// the same concrete type the real client produces, re-stamped with the given +// HTTP status and Square error code. The type is not nameable outside +// internal/square, so the clone-through-reflection technique mirrors +// errors_test.go's structuredSquareAPIError (which rewrites only the status); +// here the code is also rewritten so a CARD_DECLINED decline can be produced. +func structuredSquareErrorWithCode(t *testing.T, status int, code string) error { + t.Helper() + mc := square.NewDevClient().(*square.MockClient) + _, err := mc.CreatePayment(context.Background(), square.CreatePaymentReq{ + Amount: 1000, + Currency: "GBP", + SourceID: "ccof:card_1", + }) + require.Error(t, err, "expected the mock to reject a ccof charge without a customer") + v := reflect.ValueOf(err) + require.Equal(t, reflect.Ptr, v.Kind(), "expected the structured error to be a pointer") + clone := reflect.New(v.Elem().Type()) + clone.Elem().Set(v.Elem()) + clone.Elem().FieldByName("StatusCode").SetInt(int64(status)) + clone.Elem().FieldByName("Code").SetString(code) + return clone.Interface().(error) +} + +func TestCreateBookingPayment_ChargeFailureStatuses_KeepPending(t *testing.T) { + t.Run("structured_500_returns_503_keeps_pending", func(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + _, bookingID, userToken := setupPaymentStatusTest(t, ctx, tx, "confirmed") + + origClient := SquareClient + SquareClient = &round7ChargeClient{SquareClient: square.NewDevClient(), createErr: structuredSquareAPIError(t, http.StatusInternalServerError)} + defer func() { SquareClient = origClient }() + + cardToken := "cnon:r7-structured-500" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + IdempotencyKey: "r7-500-" + bookingID, + } + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusServiceUnavailable, w.Code, "a structured Square 500 must classify as 503, body: %s", w.Body.String()) + + var status string + require.NoError(t, tx.QueryRow(ctx, `SELECT status FROM payments WHERE booking_id = $1 AND idempotency_key = $2`, bookingID, req.IdempotencyKey).Scan(&status)) + require.Equal(t, "pending", status, "an ambiguous charge failure must leave the row pending for a same-key retry") + }) + + t.Run("card_declined_returns_402_keeps_pending", func(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + _, bookingID, userToken := setupPaymentStatusTest(t, ctx, tx, "confirmed") + + origClient := SquareClient + SquareClient = &round7ChargeClient{SquareClient: square.NewDevClient(), createErr: structuredSquareErrorWithCode(t, http.StatusPaymentRequired, "CARD_DECLINED")} + defer func() { SquareClient = origClient }() + + cardToken := "cnon:r7-card-declined" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + IdempotencyKey: "r7-declined-" + bookingID, + } + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusPaymentRequired, w.Code, "a definitive decline (CARD_DECLINED 4xx) must classify as 402, body: %s", w.Body.String()) + + var status string + require.NoError(t, tx.QueryRow(ctx, `SELECT status FROM payments WHERE booking_id = $1 AND idempotency_key = $2`, bookingID, req.IdempotencyKey).Scan(&status)) + require.Equal(t, "pending", status, "a declined charge must leave the row pending") + }) + + t.Run("cancelled_context_returns_503_keeps_pending", func(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + _, bookingID, userToken := setupPaymentStatusTest(t, ctx, tx, "confirmed") + + entered := make(chan struct{}) + origClient := SquareClient + SquareClient = &round7CancelAwareChargeClient{SquareClient: square.NewDevClient(), entered: entered} + defer func() { SquareClient = origClient }() + + reqCtx, cancel := context.WithCancel(ctx) + defer cancel() + + cardToken := "cnon:r7-cancel-ctx" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + IdempotencyKey: "r7-cancel-" + bookingID, + } + done := make(chan *httptest.ResponseRecorder, 1) + go func() { + done <- makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, reqCtx) + }() + select { + case <-entered: + // The charge reached Square with the DB pending record already + // committed — cancel now. + case <-time.After(10 * time.Second): + t.Fatal("the charge never reached Square") + } + cancel() + w := <-done + require.Equal(t, http.StatusServiceUnavailable, w.Code, "a cancelled charge context must classify as 503, body: %s", w.Body.String()) + + var status string + require.NoError(t, tx.QueryRow(ctx, `SELECT status FROM payments WHERE booking_id = $1 AND idempotency_key = $2`, bookingID, req.IdempotencyKey).Scan(&status)) + require.Equal(t, "pending", status, "a cancelled-context charge must leave the row pending") + }) +} + +// ============================================================================= +// R7 — deriveBookingPaymentIdempotencyKey >45-char sha256 truncation +// ============================================================================= + +// TestDeriveBookingPaymentIdempotencyKey_LongInput_TruncatedDeterministic +// locks the >45-char truncation: a candidate key built from a very long +// bookingID + card must be hashed down to a deterministic ≤45-char key, and +// the truncation must be input-sensitive (the seq-0 truncated key is distinct +// from the seq-1 candidate's key — distinct inputs never collapse). +func TestDeriveBookingPaymentIdempotencyKey_LongInput_TruncatedDeterministic(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + // A bookingID far longer than Square's 45-char idempotency-key limit. + longBookingID := "pay-" + strings.Repeat("b", 60) + cardPart := "ccof:some-long-card-id" + base := fmt.Sprintf("pay-%s-%s-%d-%s", longBookingID, "deposit", 2500, cardPart) + + key1, err := deriveBookingPaymentIdempotencyKey(ctx, tx, longBookingID, "deposit", 2500, cardPart) + require.NoError(t, err) + if len(key1) > 45 { + t.Errorf("the derived key must stay within Square's 45-char limit, got %d chars: %q", len(key1), key1) + } + require.True(t, strings.HasPrefix(key1, "pay-"), "the truncated key must keep the pay- prefix, got %q", key1) + + // Deterministic: an identical re-derivation returns the same key. + key2, err := deriveBookingPaymentIdempotencyKey(ctx, tx, longBookingID, "deposit", 2500, cardPart) + require.NoError(t, err) + require.Equal(t, key1, key2, "the truncated key must be deterministic") + + // The truncation hashes the FULL candidate: verify the seq-0 key against + // the sha256[:16] derivation and confirm it differs from the seq-1 key. + h0 := sha256.Sum256([]byte(base)) + wantSeq0 := fmt.Sprintf("pay-%x", h0[:16]) + require.Equal(t, wantSeq0, key1, "the seq-0 truncated key must be sha256[:16] of the full candidate") + + h1 := sha256.Sum256([]byte(base + "-1")) + wantSeq1 := fmt.Sprintf("pay-%x", h1[:16]) + require.NotEqual(t, wantSeq1, key1, "the seq-0 truncated key must be distinct from the seq-1 truncated key") +} + +// TestDeriveBookingPaymentIdempotencyKey_Truncation_Seq1Distinct locks the +// sequence-advance path under truncation: when the seq-0 truncated key is +// occupied by a REFUNDED completed payment (so it can no longer be replayed), +// the derive advances to a seq-1 truncated key that is distinct from seq-0 — +// two different inputs must never collide in the truncated namespace. +func TestDeriveBookingPaymentIdempotencyKey_Truncation_Seq1Distinct(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + serviceID, err := fixtures.CreateTestService(tx) + require.NoError(t, err) + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + require.NoError(t, err) + // A real CHAR(12) bookingID plus a long card ID pushes the candidate over + // 45 chars while the booking_id stays insertable in the payments table. + cardPart := "ccof:super-long-card-on-file-id-00000001" + + key0, err := deriveBookingPaymentIdempotencyKey(ctx, tx, bookingID, "deposit", 2500, cardPart) + require.NoError(t, err) + if len(key0) > 45 { + t.Errorf("the seq-0 truncated key must stay within 45 chars, got %d: %q", len(key0), key0) + } + + // Occupy the seq-0 slot with a REFUNDED completed payment so the derive + // must advance to seq 1 (a refunded completed row never blocks a new + // equal-amount charge). + var paymentID string + require.NoError(t, tx.QueryRow(ctx, ` + INSERT INTO payments (booking_id, payment_type, payment_method, status, amount, idempotency_key, created_at, updated_at) + VALUES ($1, 'deposit', 'online_square', 'completed', 25.00, $2, NOW(), NOW()) + RETURNING id + `, bookingID, key0).Scan(&paymentID)) + _, err = tx.Exec(ctx, ` + INSERT INTO refunds (payment_id, booking_id, amount, status, reason, created_at) + VALUES ($1, $2, 25.00, 'completed', 'test refund', NOW()) + `, paymentID, bookingID) + require.NoError(t, err) + + key1, err := deriveBookingPaymentIdempotencyKey(ctx, tx, bookingID, "deposit", 2500, cardPart) + require.NoError(t, err) + require.NotEqual(t, key0, key1, "the seq-1 truncated key must be distinct from the seq-0 key") + if len(key1) > 45 { + t.Errorf("the seq-1 truncated key must stay within 45 chars, got %d: %q", len(key1), key1) + } +} diff --git a/backend/handlers/payments/refunds.go b/backend/handlers/payments/refunds.go index 2c60037..67e3072 100644 --- a/backend/handlers/payments/refunds.go +++ b/backend/handlers/payments/refunds.go @@ -3,6 +3,7 @@ package payments import ( "context" "crypto/sha256" + "database/sql" "errors" "fmt" "log" @@ -1147,8 +1148,11 @@ func chargeAggKey(chargeID string) string { // - square_refund_id NULL: the handler's ambiguous-error path — re-issued // with the row's OWN stored idempotency key. type manualPendingRow struct { - ID string - PaymentID string + ID string + PaymentID string + // BookingID is the payment's booking_id ("" when NULL = gift-card purchase; + // such manual refunds are never re-issued, see processManualPaymentGroup). + BookingID string Amount float64 IdempotencyKey string Reason string @@ -1168,7 +1172,7 @@ type manualPendingRow struct { // retry is idempotent). func sweepManualPendingSquareRefunds(ctx context.Context) (int, error) { rows, err := db.Conn.Query(ctx, ` - SELECT r.id, r.payment_id, r.amount, r.idempotency_key, r.reason, + SELECT r.id, r.payment_id, p.booking_id, r.amount, r.idempotency_key, r.reason, p.square_payment_id, r.square_refund_id, r.created_at FROM refunds r JOIN payments p ON p.id = r.payment_id @@ -1186,10 +1190,12 @@ func sweepManualPendingSquareRefunds(ctx context.Context) (int, error) { var pr manualPendingRow var key *string var sqRefundID *string - if err := rows.Scan(&pr.ID, &pr.PaymentID, &pr.Amount, &key, &pr.Reason, &pr.SquarePaymentID, &sqRefundID, &pr.CreatedAt); err != nil { + var bookingID sql.NullString + if err := rows.Scan(&pr.ID, &pr.PaymentID, &bookingID, &pr.Amount, &key, &pr.Reason, &pr.SquarePaymentID, &sqRefundID, &pr.CreatedAt); err != nil { log.Printf("Failed to scan manual pending refund: %v", err) continue } + pr.BookingID = bookingID.String if key != nil { pr.IdempotencyKey = *key } @@ -1301,7 +1307,7 @@ func processManualPaymentGroup(ctx context.Context, paymentID string, rows []man // Re-read under the lock — only rows still pending and under the attempt // cap are eligible (a concurrent manual refund may have resolved some). prRows, err := db.Conn.Query(ctx, ` - SELECT r.id, r.amount, r.idempotency_key, r.reason, r.created_at, + SELECT r.id, p.booking_id, r.amount, r.idempotency_key, r.reason, r.created_at, r.payment_id, p.square_payment_id, r.square_refund_id FROM refunds r JOIN payments p ON p.id = r.payment_id @@ -1316,10 +1322,12 @@ func processManualPaymentGroup(ctx context.Context, paymentID string, rows []man var pr manualPendingRow var key *string var sqRefundID *string - if err := prRows.Scan(&pr.ID, &pr.Amount, &key, &pr.Reason, &pr.CreatedAt, &pr.PaymentID, &pr.SquarePaymentID, &sqRefundID); err != nil { + var bookingID sql.NullString + if err := prRows.Scan(&pr.ID, &bookingID, &pr.Amount, &key, &pr.Reason, &pr.CreatedAt, &pr.PaymentID, &pr.SquarePaymentID, &sqRefundID); err != nil { log.Printf("Failed to scan manual pending refund under lock: %v", err) continue } + pr.BookingID = bookingID.String if key != nil { pr.IdempotencyKey = *key } @@ -1384,6 +1392,41 @@ func processManualPaymentGroup(ctx context.Context, paymentID string, rows []man pr := &pending[i] amountCents := int64(math.Round(pr.Amount * 100)) + // A payment with NO booking is a gift-card purchase (BuyGiftCard + // inserts without a booking) — the handler rejects these outright, so a + // pending row that predates that guard must never be re-issued here + // (the sweep is a bypass of the handler guard). Reconcile only, + // exactly like the square_refund_id branch: an exact COMPLETED refund + // resolves to completed (money already moved); a no-match means Square + // never refunded — mark failed + admin-notified so the amount unblocks + // the over-refund guard and the customer is handled via the gift-card + // section. + if pr.BookingID == "" { + sqRefundID, rcErr := reconcileRefundAtSquare(ctx, pr.SquarePaymentID, amountCents, pr.CreatedAt) + switch { + case rcErr != nil: + log.Printf("Reconcile failed for gift-card-purchase manual refund %s (%v) — leaving pending for the next sweep", pr.ID, rcErr) + case sqRefundID != nil: + if _, upErr := db.Conn.Exec(ctx, ` + UPDATE refunds SET status = 'completed', square_refund_id = $1 + WHERE id = $2 AND status = 'pending' + `, *sqRefundID, pr.ID); upErr != nil { + log.Printf("Failed to mark gift-card-purchase manual refund %s completed after Square reconcile: %v", pr.ID, upErr) + } + processed++ + default: + if _, upErr := db.Conn.Exec(ctx, ` + UPDATE refunds SET status = 'failed' + WHERE id = $1 AND status = 'pending' + `, pr.ID); upErr != nil { + log.Printf("Failed to mark gift-card-purchase manual refund %s failed: %v", pr.ID, upErr) + } + insertRefundFailedNotifications(ctx, []string{pr.ID}) + log.Printf("Gift-card-purchase manual refund %s (payment %s) blocked — payment has no booking; Square shows no COMPLETED refund — marked failed, customer must be refunded via the gift-card section", pr.ID, pr.PaymentID) + } + continue + } + // Row WITH a stored square_refund_id — the RefundPayment handler's // synchronous-PENDING response. Square already holds the refund, so a // re-issue would risk a SECOND refund (Square's key dedup does not diff --git a/backend/handlers/payments/sweep.go b/backend/handlers/payments/sweep.go index 36bac29..8e1dc07 100644 --- a/backend/handlers/payments/sweep.go +++ b/backend/handlers/payments/sweep.go @@ -835,13 +835,10 @@ func SweepStaleTerminalCheckouts(ctx context.Context) (int, error) { resolved := 0 for _, r := range pending { - // A provisional (pre-Square) terminal_checkouts row carries a synthetic - // "tmp-" checkout_id (or an empty one) — no checkout was ever created - // at Square for it, so it is PROVABLY not live (R3). Resolve it to - // failed directly without a Square round-trip; a hard crash between the - // row insert and the Square CreateCheckout call is the only way one - // exists. - if r.CheckoutID == "" || strings.HasPrefix(r.CheckoutID, "tmp-") { + // An EMPTY checkout_id is a legacy provisional (pre-Square) row for + // which no Square call was ever possible — resolving it to failed + // directly is safe (R3). + if r.CheckoutID == "" { if markTerminalCheckoutRowFailed(ctx, r) { resolved++ } @@ -849,6 +846,72 @@ func SweepStaleTerminalCheckouts(ctx context.Context) (int, error) { continue } + // A provisional row carries a synthetic "tmp-" checkout_id. It is no + // longer PROVABLY not live (H4): a hard crash between the + // terminal_checkouts insert and the provisional→real UPDATE leaves a + // LIVE checkout at Square (created under the idempotency key embedded + // in the tmp id) while the row still carries the synthetic id. + // Blind-failing would release the in-flight guard while that checkout + // can still complete at the terminal into an untracked charge. Query + // Square first and classify exactly like activeTerminalCheckoutID + // (handlers.go H4): COMPLETED → record the payment and release the + // guard; ErrCheckoutPending → the checkout is still live at Square, + // keep the guard; isTerminalCheckoutError (NOT_FOUND / CANCELED) → + // safe to fail (the crash happened before the Square call, or the + // checkout was cancelled); ambiguous → leave the row in flight. A tmp- + // id that never reached Square (or one Square cannot resolve) comes + // back NOT_FOUND — the expected outcome — and failing it is correct. + if strings.HasPrefix(r.CheckoutID, "tmp-") { + pr, gErr := SquareClient.GetCheckout(ctx, r.CheckoutID) + switch { + case gErr == nil && pr.Status == "COMPLETED": + // The checkout actually completed at the terminal. Record the + // payment if it was never polled/recorded — a COMPLETED charge + // must not stay an invisible untracked charge (H4). Booking + // checkouts record full payment rows; till-sale checkouts + // record the sale row. + if r.Kind == "terminal_checkout" { + if recordUntrackedTerminalPayment(ctx, r.CheckoutID, r.BookingID, pr) { + resolved++ + } + } else if recordUntrackedTillSalePayment(ctx, r.RowID, pr) { + resolved++ + } + log.Printf("Provisional terminal checkout %s (%s %s) is COMPLETED at Square — payment recorded by the sweep", r.CheckoutID, r.Kind, r.RowID) + case errors.Is(gErr, square.ErrCheckoutPending): + // The checkout is still live at Square — keep the in-flight + // guard so a second checkout cannot be created while it can + // still complete. + log.Printf("Provisional terminal checkout %s (%s %s) is still live at Square — leaving pending; the in-flight guard stays held", r.CheckoutID, r.Kind, r.RowID) + case isTerminalCheckoutError(gErr): + // NOT_FOUND (no checkout was ever created — the crash happened + // before the Square call) or CANCELED — safe to resolve failed. + switch { + case r.Kind == "till_sale" && isCheckoutDefinitivelyDead(gErr): + if clawBackTillSaleFunding(ctx, r.RowID) { + resolved++ + } + log.Printf("Provisional terminal checkout %s is definitively terminal (%v) — marked till sale %s failed, gift-card funding clawed back", r.CheckoutID, gErr, r.RowID) + case r.Kind == "till_sale": + if markTerminalCheckoutRowFailed(ctx, r) { + resolved++ + } + log.Printf("CRITICAL: provisional terminal checkout %s reports only CANCEL_REQUESTED (not provably dead) — till sale %s marked failed WITHOUT clawing back the funded gift card; verify at Square before re-issuing — MANUAL RECONCILIATION REQUIRED", r.CheckoutID, r.RowID) + default: + if markTerminalCheckoutRowFailed(ctx, r) { + resolved++ + } + log.Printf("Provisional (pre-Square) terminal checkout row %q (%s %s) resolved as failed — no live checkout at Square", r.CheckoutID, r.Kind, r.RowID) + } + default: + // Ambiguous error — the checkout's money state at Square is + // unknown. Leave the row in flight rather than releasing the + // guard and allowing a second checkout. + log.Printf("Provisional terminal checkout %s (%s %s) status unknown (%v) — leaving pending for a later sweep", r.CheckoutID, r.Kind, r.RowID, gErr) + } + continue + } + // Conservative status check: only cancel a checkout that is provably // still waiting at Square. A COMPLETED checkout must never be // cancelled, and an ambiguous status (network error) is left alone for @@ -991,9 +1054,15 @@ func markTerminalCheckoutRowFailed(ctx context.Context, r staleTerminalCheckoutR // lock key (serializes against a concurrent poll), the same dedup by // booking_id + square_payment_id, the same PaymentRecord shape and derived // idempotency key, and the same deposit/balance/tip split for a charge above -// the remaining booking value. Returns true when the checkout row was resolved -// (payment recorded or already recorded); false when recording failed (the -// row is left pending so the next sweep re-runs the whole reconcile). +// the remaining booking value. It also mirrors GetCheckoutStatus's booking +// re-check: a booking that moved out of a payable state (cancelled / lapsed / +// no-show) after the charge completed refuses to record the payment, marks the +// checkout failed, and inserts a critical-payment admin notification so the +// owner is told a charge landed on a cancelled booking and a manual refund is +// required. Returns true when the checkout row was resolved (payment recorded, +// already recorded, or refused on a cancelled booking); false when recording +// failed (the row is left pending so the next sweep re-runs the whole +// reconcile). func recordUntrackedTerminalPayment(ctx context.Context, checkoutID, bookingID string, pr *square.PaymentResult) bool { if pr == nil || pr.SquarePayID == "" { log.Printf("CRITICAL: terminal checkout %s is COMPLETED at Square but carries no Square payment ID — cannot record the payment — MANUAL RECONCILIATION REQUIRED", checkoutID) @@ -1060,6 +1129,41 @@ func recordUntrackedTerminalPayment(ctx context.Context, checkoutID, bookingID s return false } + // Re-check the booking status under the advisory lock (mirrors + // GetCheckoutStatus): a cancellation/eviction that committed between the + // terminal charge completing at Square and this sweep recording it must + // not produce a completed payment on a cancelled/lapsed/no-show booking — + // the cancellation refund path computes refunds from completed payments + // and would silently exclude this charge. Mark the checkout failed and + // alert ops: money was taken at Square and MUST be refunded manually. + var recheckStatus string + // FOR UPDATE (C5): serializes against the cancellation path's lock on the + // same row so a concurrent cancellation cannot commit between this recheck + // and the transaction commit below. + if err := tx.QueryRow(ctx, `SELECT status FROM bookings WHERE id = $1 FOR UPDATE`, bookingID).Scan(&recheckStatus); err != nil { + log.Printf("CRITICAL: Square payment %s for checkout %s was processed but re-reading booking %s status failed: %v — manual reconciliation required", + pr.SquarePayID, checkoutID, bookingID, err) + return false + } + if !bookingStatusAllowsCompletedPayment(recheckStatus) { + log.Printf("CRITICAL: Square payment %s for checkout %s was processed but booking %s is now %q — marking checkout failed; money taken at Square MUST be refunded manually", + pr.SquarePayID, checkoutID, bookingID, recheckStatus) + if _, upErr := tx.Exec(ctx, ` + UPDATE terminal_checkouts SET status = 'failed', updated_at = NOW() + WHERE checkout_id = $1 AND status IN ('PENDING', 'IN_PROGRESS') + `, checkoutID); upErr != nil { + log.Printf("CRITICAL: Square payment %s landed on %q booking %s but marking checkout %s failed errored: %v — manual reconciliation required", + pr.SquarePayID, recheckStatus, bookingID, checkoutID, upErr) + } + if cErr := tx.Commit(ctx); cErr != nil { + log.Printf("CRITICAL: Square payment %s landed on %q booking %s and committing the checkout-failed mark errored: %v — manual reconciliation required", + pr.SquarePayID, recheckStatus, bookingID, cErr) + return false + } + insertCriticalPaymentNotification(ctx, &bookingID, nil) + return true + } + // The payment type the admin charged is recorded on the checkout row by // CreateTerminalPayment; fall back to 'full' for legacy rows. var checkoutPaymentType string diff --git a/backend/handlers/payments/sweep_test.go b/backend/handlers/payments/sweep_test.go index 010d8d8..88baee0 100644 --- a/backend/handlers/payments/sweep_test.go +++ b/backend/handlers/payments/sweep_test.go @@ -1941,3 +1941,461 @@ func TestSweepStaleTerminalCheckouts_MockCancelRequested_CancelThenClawback(t *t t.Errorf("expected the clawed-back created gift card deleted, got %d cards", cardCount) } } + +// ============================================================================= +// SweepStaleTerminalCheckouts — provisional "tmp-" rows are resolved against +// Square first (H4), never blind-failed +// ============================================================================= + +// provisionalCheckoutClient forces GetCheckout for one target checkout id to a +// fixed result/error so the sweep's provisional "tmp-" row resolution +// (mirroring handlers.go activeTerminalCheckoutID's H4 classification) can be +// exercised deterministically, while delegating everything else to the mock. +type provisionalCheckoutClient struct { + square.SquareClient + checkoutID string + result *square.PaymentResult + err error +} + +func (c *provisionalCheckoutClient) GetCheckout(ctx context.Context, checkoutID string) (*square.PaymentResult, error) { + if checkoutID == c.checkoutID { + if c.err != nil { + return nil, c.err + } + return c.result, nil + } + return c.SquareClient.GetCheckout(ctx, checkoutID) +} + +// seedStaleProvisionalTerminalCheckout seeds a stale (2h old) PENDING +// terminal_checkouts row carrying a synthetic "tmp-" checkout_id for the +// caller's booking inside the setup transaction. The caller commits the setup +// tx; pool-level cleanup of the row is registered. +func seedStaleProvisionalTerminalCheckout(t *testing.T, ctx context.Context, q db.Querier, bookingID, tmpID string) { + t.Helper() + pool := context.Background() + if _, err := q.Exec(ctx, ` + INSERT INTO terminal_checkouts (checkout_id, booking_id, payment_type, status, amount, created_at) + VALUES ($1, $2, 'full', 'PENDING', 50.00, NOW() - INTERVAL '2 hours') + `, tmpID, bookingID); err != nil { + t.Fatalf("failed to seed stale provisional terminal checkout: %v", err) + } + t.Cleanup(func() { + _, _ = db.Conn.Exec(pool, `DELETE FROM terminal_checkouts WHERE checkout_id = $1`, tmpID) + }) +} + +// TestSweepStaleTerminalCheckouts_TmpProvisional_Completed_RecordsPayment locks +// the H4 fix: a provisional "tmp-" terminal_checkouts row whose checkout +// actually COMPLETED at Square (a hard crash between the row insert and the +// provisional→real UPDATE leaves the real checkout live while the row still +// carries the synthetic id) is resolved against Square and the untracked charge +// RECORDED — never blind-failed. Blind-failing would release the in-flight +// guard while the customer was still charged, leaving an invisible untracked +// payment (a second checkout could then be created for the same booking). +func TestSweepStaleTerminalCheckouts_TmpProvisional_Completed_RecordsPayment(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + if err != nil { + t.Fatalf("failed to create booking: %v", err) + } + // The booking must be in a payable state for the untracked charge to be + // recorded. + if _, err := tx.Exec(ctx, "UPDATE bookings SET status = 'in_progress' WHERE id = $1", bookingID); err != nil { + t.Fatalf("failed to set booking in_progress: %v", err) + } + + const tmpID = "tmp-crash-completed-key" + seedStaleProvisionalTerminalCheckout(t, ctx, tx, bookingID, tmpID) + + origClient := SquareClient + SquareClient = &provisionalCheckoutClient{ + SquareClient: square.NewDevClient(), + checkoutID: tmpID, + result: &square.PaymentResult{Status: "COMPLETED", SquarePayID: "sqp_tmp_completed", Amount: 5000, Fees: 88, CardBrand: "VISA", CardLast4: "4242"}, + } + defer func() { SquareClient = origClient }() + + pgxTx := db.TxFromContext(ctx) + if pgxTx == nil { + t.Fatal("no transaction in context") + } + if err := pgxTx.Commit(ctx); err != nil { + t.Fatalf("failed to commit test tx: %v", err) + } + + pool := context.Background() + t.Cleanup(func() { + _, _ = db.Conn.Exec(pool, `DELETE FROM payments WHERE square_payment_id = 'sqp_tmp_completed'`) + _, _ = db.Conn.Exec(pool, `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(pool, `DELETE FROM users WHERE id = $1`, userID) + }) + + // Drop any other stale terminal rows left by sequential tests so the count + // is deterministic. + if _, err := db.Conn.Exec(pool, `DELETE FROM terminal_checkouts WHERE status IN ('PENDING', 'IN_PROGRESS') AND checkout_id <> $1`, tmpID); err != nil { + t.Fatalf("failed to clean leftover stale terminal checkouts: %v", err) + } + if _, err := db.Conn.Exec(pool, `DELETE FROM till_sales WHERE status = 'pending' AND square_checkout_id IS NOT NULL`); err != nil { + t.Fatalf("failed to clean leftover stale till sales: %v", err) + } + + n, err := SweepStaleTerminalCheckouts(pool) + if err != nil { + t.Fatalf("sweep failed: %v", err) + } + if n != 1 { + t.Errorf("expected the COMPLETED provisional checkout resolved by the sweep, got %d resolutions", n) + } + + var status string + if err := db.Conn.QueryRow(pool, "SELECT status FROM terminal_checkouts WHERE checkout_id = $1", tmpID).Scan(&status); err != nil { + t.Fatalf("failed to query terminal checkout: %v", err) + } + if status != "COMPLETED" { + t.Errorf("expected a COMPLETED provisional checkout marked 'COMPLETED', got %q", status) + } + + // The untracked charge must have been recorded as a payment row (H4). + var payCount int + if err := db.Conn.QueryRow(pool, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND square_payment_id = 'sqp_tmp_completed'`, bookingID).Scan(&payCount); err != nil { + t.Fatalf("failed to count recorded payments: %v", err) + } + if payCount != 1 { + t.Errorf("expected the COMPLETED provisional charge recorded as a payment, got %d rows", payCount) + } +} + +// TestSweepStaleTerminalCheckouts_TmpProvisional_NotFound_Fails locks the H4 +// NOT_FOUND direction: a provisional "tmp-" row that Square has never seen (the +// crash happened before the Square call, so no checkout was ever created — or +// the tmp id cannot be resolved) resolves to the expected NOT_FOUND and is +// safely marked failed. +func TestSweepStaleTerminalCheckouts_TmpProvisional_NotFound_Fails(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + if err != nil { + t.Fatalf("failed to create booking: %v", err) + } + + const tmpID = "tmp-never-reached-square-key" + seedStaleProvisionalTerminalCheckout(t, ctx, tx, bookingID, tmpID) + + // A fresh mock holds no checkout under the tmp id → GetCheckout returns the + // mock's plain "checkout not found" error, which isTerminalCheckoutError + // classifies as terminal — the expected outcome for an unresolvable tmp id. + origClient := SquareClient + SquareClient = square.NewDevClient() + defer func() { SquareClient = origClient }() + + pgxTx := db.TxFromContext(ctx) + if pgxTx == nil { + t.Fatal("no transaction in context") + } + if err := pgxTx.Commit(ctx); err != nil { + t.Fatalf("failed to commit test tx: %v", err) + } + + pool := context.Background() + t.Cleanup(func() { + _, _ = db.Conn.Exec(pool, `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(pool, `DELETE FROM users WHERE id = $1`, userID) + }) + + if _, err := db.Conn.Exec(pool, `DELETE FROM terminal_checkouts WHERE status IN ('PENDING', 'IN_PROGRESS') AND checkout_id <> $1`, tmpID); err != nil { + t.Fatalf("failed to clean leftover stale terminal checkouts: %v", err) + } + if _, err := db.Conn.Exec(pool, `DELETE FROM till_sales WHERE status = 'pending' AND square_checkout_id IS NOT NULL`); err != nil { + t.Fatalf("failed to clean leftover stale till sales: %v", err) + } + + n, err := SweepStaleTerminalCheckouts(pool) + if err != nil { + t.Fatalf("sweep failed: %v", err) + } + if n != 1 { + t.Errorf("expected the not-found provisional checkout resolved to failed, got %d resolutions", n) + } + + var status string + if err := db.Conn.QueryRow(pool, "SELECT status FROM terminal_checkouts WHERE checkout_id = $1", tmpID).Scan(&status); err != nil { + t.Fatalf("failed to query terminal checkout: %v", err) + } + if status != "failed" { + t.Errorf("expected a not-found provisional checkout marked 'failed', got %q", status) + } +} + +// TestSweepStaleTerminalCheckouts_TmpProvisional_Ambiguous_LeavesPending locks +// the H4 ambiguous direction: a provisional "tmp-" row whose Square status is +// unknown (transport error) must NOT be blind-failed — the checkout may still +// be live or may have completed, so the row stays pending for a later run. +func TestSweepStaleTerminalCheckouts_TmpProvisional_Ambiguous_LeavesPending(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + if err != nil { + t.Fatalf("failed to create booking: %v", err) + } + + const tmpID = "tmp-ambiguous-key" + seedStaleProvisionalTerminalCheckout(t, ctx, tx, bookingID, tmpID) + + origClient := SquareClient + SquareClient = &provisionalCheckoutClient{ + SquareClient: square.NewDevClient(), + checkoutID: tmpID, + err: fmt.Errorf("network error: connection reset by peer"), + } + defer func() { SquareClient = origClient }() + + pgxTx := db.TxFromContext(ctx) + if pgxTx == nil { + t.Fatal("no transaction in context") + } + if err := pgxTx.Commit(ctx); err != nil { + t.Fatalf("failed to commit test tx: %v", err) + } + + pool := context.Background() + t.Cleanup(func() { + _, _ = db.Conn.Exec(pool, `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(pool, `DELETE FROM users WHERE id = $1`, userID) + }) + + if _, err := db.Conn.Exec(pool, `DELETE FROM terminal_checkouts WHERE status IN ('PENDING', 'IN_PROGRESS') AND checkout_id <> $1`, tmpID); err != nil { + t.Fatalf("failed to clean leftover stale terminal checkouts: %v", err) + } + if _, err := db.Conn.Exec(pool, `DELETE FROM till_sales WHERE status = 'pending' AND square_checkout_id IS NOT NULL`); err != nil { + t.Fatalf("failed to clean leftover stale till sales: %v", err) + } + + n, err := SweepStaleTerminalCheckouts(pool) + if err != nil { + t.Fatalf("sweep failed: %v", err) + } + if n != 0 { + t.Errorf("expected an ambiguous provisional checkout left unresolved, got %d resolutions", n) + } + + var status string + if err := db.Conn.QueryRow(pool, "SELECT status FROM terminal_checkouts WHERE checkout_id = $1", tmpID).Scan(&status); err != nil { + t.Fatalf("failed to query terminal checkout: %v", err) + } + if status != "PENDING" { + t.Errorf("expected an ambiguous provisional checkout left 'PENDING', got %q", status) + } +} + +// TestSweepStaleTerminalCheckouts_TmpProvisional_StillLive_LeavesPending locks +// the H4 ErrCheckoutPending direction: a provisional "tmp-" row whose checkout +// is still live at Square (PENDING / IN_PROGRESS) must keep the in-flight guard +// — the sweep must never resolve it failed (releasing the guard) while the +// customer can still complete the payment at the terminal into an untracked +// charge. +func TestSweepStaleTerminalCheckouts_TmpProvisional_StillLive_LeavesPending(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + if err != nil { + t.Fatalf("failed to create booking: %v", err) + } + + const tmpID = "tmp-still-live-key" + seedStaleProvisionalTerminalCheckout(t, ctx, tx, bookingID, tmpID) + + origClient := SquareClient + SquareClient = &provisionalCheckoutClient{ + SquareClient: square.NewDevClient(), + checkoutID: tmpID, + err: square.ErrCheckoutPending, + } + defer func() { SquareClient = origClient }() + + pgxTx := db.TxFromContext(ctx) + if pgxTx == nil { + t.Fatal("no transaction in context") + } + if err := pgxTx.Commit(ctx); err != nil { + t.Fatalf("failed to commit test tx: %v", err) + } + + pool := context.Background() + t.Cleanup(func() { + _, _ = db.Conn.Exec(pool, `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(pool, `DELETE FROM users WHERE id = $1`, userID) + }) + + if _, err := db.Conn.Exec(pool, `DELETE FROM terminal_checkouts WHERE status IN ('PENDING', 'IN_PROGRESS') AND checkout_id <> $1`, tmpID); err != nil { + t.Fatalf("failed to clean leftover stale terminal checkouts: %v", err) + } + if _, err := db.Conn.Exec(pool, `DELETE FROM till_sales WHERE status = 'pending' AND square_checkout_id IS NOT NULL`); err != nil { + t.Fatalf("failed to clean leftover stale till sales: %v", err) + } + + n, err := SweepStaleTerminalCheckouts(pool) + if err != nil { + t.Fatalf("sweep failed: %v", err) + } + if n != 0 { + t.Errorf("expected a still-live provisional checkout left unresolved, got %d resolutions", n) + } + + var status string + if err := db.Conn.QueryRow(pool, "SELECT status FROM terminal_checkouts WHERE checkout_id = $1", tmpID).Scan(&status); err != nil { + t.Fatalf("failed to query terminal checkout: %v", err) + } + if status != "PENDING" { + t.Errorf("expected a still-live provisional checkout left 'PENDING' (guard held), got %q", status) + } +} + +// TestSweepStaleTerminalCheckouts_CancelledBooking_NoPaymentRecorded locks the +// sweep's booking re-check (mirroring GetCheckoutStatus): a terminal checkout +// that COMPLETED at Square while the booking is we_cancelled must NOT be +// recorded as a completed payment — the cancellation refund path computes +// refunds from completed payments and would silently exclude this charge. +// Instead the checkout is marked failed, no payment row is created, and a +// critical-payment admin notification tells the owner a charge landed on a +// cancelled booking and a manual refund is required. +func TestSweepStaleTerminalCheckouts_CancelledBooking_NoPaymentRecorded(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + if err != nil { + t.Fatalf("failed to create booking: %v", err) + } + // The charge completes at Square AFTER the booking was cancelled. + if _, err := tx.Exec(ctx, "UPDATE bookings SET status = 'we_cancelled' WHERE id = $1", bookingID); err != nil { + t.Fatalf("failed to set booking we_cancelled: %v", err) + } + + const checkoutID = "chk_completed_on_cancelled_booking" + if _, err := tx.Exec(ctx, ` + INSERT INTO terminal_checkouts (checkout_id, booking_id, payment_type, status, amount, created_at) + VALUES ($1, $2, 'full', 'PENDING', 50.00, NOW() - INTERVAL '2 hours') + `, checkoutID, bookingID); err != nil { + t.Fatalf("failed to seed stale terminal checkout: %v", err) + } + + origClient := SquareClient + SquareClient = &provisionalCheckoutClient{ + SquareClient: square.NewDevClient(), + checkoutID: checkoutID, + result: &square.PaymentResult{Status: "COMPLETED", SquarePayID: "sqp_cancelled_booking", Amount: 5000, Fees: 88, CardBrand: "VISA", CardLast4: "4242"}, + } + defer func() { SquareClient = origClient }() + + pgxTx := db.TxFromContext(ctx) + if pgxTx == nil { + t.Fatal("no transaction in context") + } + if err := pgxTx.Commit(ctx); err != nil { + t.Fatalf("failed to commit test tx: %v", err) + } + + pool := context.Background() + t.Cleanup(func() { + _, _ = db.Conn.Exec(pool, `DELETE FROM admin_notifications WHERE reason = 'critical_payment_log' AND booking_id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM payments WHERE booking_id = $1 AND square_payment_id = 'sqp_cancelled_booking'`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM terminal_checkouts WHERE checkout_id = $1`, checkoutID) + _, _ = db.Conn.Exec(pool, `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(pool, `DELETE FROM users WHERE id = $1`, userID) + }) + + if _, err := db.Conn.Exec(pool, `DELETE FROM terminal_checkouts WHERE status IN ('PENDING', 'IN_PROGRESS') AND checkout_id <> $1`, checkoutID); err != nil { + t.Fatalf("failed to clean leftover stale terminal checkouts: %v", err) + } + if _, err := db.Conn.Exec(pool, `DELETE FROM till_sales WHERE status = 'pending' AND square_checkout_id IS NOT NULL`); err != nil { + t.Fatalf("failed to clean leftover stale till sales: %v", err) + } + + n, err := SweepStaleTerminalCheckouts(pool) + if err != nil { + t.Fatalf("sweep failed: %v", err) + } + if n != 1 { + t.Errorf("expected the COMPLETED checkout on a cancelled booking resolved (to failed) by the sweep, got %d resolutions", n) + } + + // The checkout must be marked failed (guard released, sweep does not retry + // forever), never COMPLETED and never left with a recorded payment. + var status string + if err := db.Conn.QueryRow(pool, "SELECT status FROM terminal_checkouts WHERE checkout_id = $1", checkoutID).Scan(&status); err != nil { + t.Fatalf("failed to query terminal checkout: %v", err) + } + if status != "failed" { + t.Errorf("expected the cancelled-booking checkout marked 'failed', got %q", status) + } + + // No payment row may exist for the charge. + var payCount int + if err := db.Conn.QueryRow(pool, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND square_payment_id = 'sqp_cancelled_booking'`, bookingID).Scan(&payCount); err != nil { + t.Fatalf("failed to count recorded payments: %v", err) + } + if payCount != 0 { + t.Errorf("expected NO payment recorded on the cancelled booking, got %d rows", payCount) + } + + // The owner must be told a charge landed on a cancelled booking. + var notifCount int + if err := db.Conn.QueryRow(pool, `SELECT COUNT(*) FROM admin_notifications WHERE reason = 'critical_payment_log' AND booking_id = $1 AND user_id IS NULL`, bookingID).Scan(¬ifCount); err != nil { + t.Fatalf("failed to count admin notifications: %v", err) + } + if notifCount != 1 { + t.Errorf("expected a critical-payment admin notification for the cancelled booking, got %d", notifCount) + } +} diff --git a/backend/handlers/payments/testmain_test.go b/backend/handlers/payments/testmain_test.go index d109fdc..b6d159e 100644 --- a/backend/handlers/payments/testmain_test.go +++ b/backend/handlers/payments/testmain_test.go @@ -24,6 +24,12 @@ func TestMain(m *testing.M) { // explicitly. Tests that assert enforcement flip these via t.Setenv. os.Setenv("REQUIRE_2FA", "") os.Setenv("SQUARE_ENVIRONMENT", "mock") + // The dev mock's CreateCheckout mirrors real Square's requirement that a + // terminal checkout carries device_options.device_id (it 400s when both the + // request DeviceID and this env fallback are empty). Default the terminal + // device id so terminal/till/sweep tests can create checkouts; tests that + // assert the rejection flip it via t.Setenv. + os.Setenv("SQUARE_TERMINAL_DEVICE_ID", "dev-terminal") testdb.SeedBaseline(pool) code := m.Run() testdb.DestroyTestDatabase(pool, "crussell_test_handlers_payments") diff --git a/backend/handlers/payments/till.go b/backend/handlers/payments/till.go index 70217ac..9f914a2 100644 --- a/backend/handlers/payments/till.go +++ b/backend/handlers/payments/till.go @@ -895,9 +895,13 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { return } - // Square succeeded — update the till_sale record. + // Square succeeded — update the till_sale record. The status='pending' + // guard (claim-first) prevents resurrecting a sale the stale-pending + // sweep / gift-card clawback resolved to 'failed' while the Square + // charge was in flight: money was taken and the card was already + // clawed back, so the sale must stay failed (0 rows → CRITICAL below). tillTag, upErr := db.Conn.Exec(ctx, - `UPDATE till_sales SET status = 'completed', square_payment_id = $1 WHERE id = $2`, + `UPDATE till_sales SET status = 'completed', square_payment_id = $1 WHERE id = $2 AND status = 'pending'`, paymentResult.SquarePayID, tillSaleID, ) if upErr != nil { diff --git a/backend/handlers/payments/till_test.go b/backend/handlers/payments/till_test.go index d531abf..c5237a6 100644 --- a/backend/handlers/payments/till_test.go +++ b/backend/handlers/payments/till_test.go @@ -8,8 +8,10 @@ import ( "encoding/json" "errors" "fmt" + "log" "net/http" "net/http/httptest" + "strings" "sync" "testing" "time" @@ -2712,3 +2714,184 @@ func TestCreateTillSale_PendingRetry_FreshKey_DifferentAmount_NewCharge(t *testi t.Errorf("expected the original pending row to be untouched, got %s", origStatus) } } + +// ============================================================================= +// CreateTillSale — the post-charge completion UPDATE is claim-first: a sale +// resolved to 'failed' by the stale-pending sweep / gift-card clawback while +// the Square charge is in flight must NOT be resurrected to 'completed'. +// ============================================================================= + +// lockedBuffer is a mutex-guarded bytes.Buffer so the standard logger's output +// can be captured and asserted on even when concurrent writers (mock +// auto-complete goroutines) log independently. +type lockedBuffer struct { + buf bytes.Buffer + mu sync.Mutex +} + +func (b *lockedBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.Write(p) +} + +func (b *lockedBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.String() +} + +// captureStdLog redirects the standard logger's output to a mutex-guarded +// buffer until the returned restore function is called. +func captureStdLog(t *testing.T) (*lockedBuffer, func()) { + t.Helper() + l := &lockedBuffer{} + orig := log.Writer() + log.SetOutput(l) + return l, func() { log.SetOutput(orig) } +} + +// tillSweepResolveClient returns a successful CreatePayment but flips the +// till_sales row to 'failed' first — simulating the stale-pending sweep / +// gift-card clawback resolving the sale mid-flight (status change, row still +// exists) while the Square charge is in flight. The retry's post-charge +// completion UPDATE must then hit 0 rows instead of resurrecting the sale. +type tillSweepResolveClient struct { + square.SquareClient + saleID string +} + +func (c *tillSweepResolveClient) CreatePayment(ctx context.Context, req square.CreatePaymentReq) (*square.PaymentResult, error) { + if _, err := db.Conn.Exec(context.Background(), `UPDATE till_sales SET status = 'failed', updated_at = NOW() WHERE id = $1`, c.saleID); err != nil { + log.Printf("failed to simulate sweep resolution for till sale %s: %v", c.saleID, err) + } + return &square.PaymentResult{ + Status: "COMPLETED", + SquarePayID: "sqp_sweep_resolved_midflight", + Amount: 5000, + Fees: 88, + CardBrand: "VISA", + CardLast4: "4242", + }, nil +} + +// TestCreateTillSale_SweepResolvedMidFlight_NoResurrect locks the claim-first +// guard: a pending till sale retried while the stale-pending sweep / gift-card +// clawback marks the sale 'failed' (the charge was proven never to complete, so +// the funded gift card is clawed back) must NOT be resurrected by the retry's +// post-charge completion UPDATE. The Square charge still succeeds (money moves +// at Square), so the handler detects RowsAffected()==0, logs CRITICAL and fails +// the response — the sale stays 'failed', the row is never flipped back to +// 'completed'. +func TestCreateTillSale_SweepResolvedMidFlight_NoResurrect(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + adminToken := jwt.GenerateTestToken(adminID, "admin") + + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sq_test_card_id", "VISA", "1234") + if err != nil { + t.Fatalf("failed to create saved card: %v", err) + } + + // Seed a PENDING till_sale (the retry target) with its funded gift card, + // exactly like TestCreateTillSale_PendingRetry_ReattemptsSquare. + key := "till-sweep-resolve-key" + var giftCardID string + err = tx.QueryRow(ctx, ` + INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by, is_inventory, voucher_type_at_purchase) + VALUES (50.00, 50.00, $1, FALSE, 'SPV') + RETURNING id + `, adminID).Scan(&giftCardID) + if err != nil { + t.Fatalf("failed to create gift card: %v", err) + } + var saleID string + err = tx.QueryRow(ctx, ` + INSERT INTO till_sales (item_type, item_id, description, quantity, unit_price, total_amount, + payment_method, status, user_id, user_saved_card_id, idempotency_key, created_by, created_at, updated_at) + VALUES ('gift_card', $1, 'Gift Card create', 1, 50.00, 50.00, 'online_square', 'pending', + $2, $3, $4, $5, NOW(), NOW()) + RETURNING id + `, giftCardID, userID, cardID, key, adminID).Scan(&saleID) + if err != nil { + t.Fatalf("failed to seed pending till sale: %v", err) + } + + pgxTx := db.TxFromContext(ctx) + if pgxTx == nil { + t.Fatal("no transaction in context") + } + if err := pgxTx.Commit(ctx); err != nil { + t.Fatalf("failed to commit setup tx: %v", err) + } + + pool := context.Background() + t.Cleanup(func() { + _, _ = db.Conn.Exec(pool, `DELETE FROM till_sales WHERE id = $1`, saleID) + _, _ = db.Conn.Exec(pool, `DELETE FROM gift_card_transactions WHERE gift_card_id = $1`, giftCardID) + _, _ = db.Conn.Exec(pool, `DELETE FROM gift_cards WHERE id = $1`, giftCardID) + _, _ = db.Conn.Exec(pool, `DELETE FROM user_saved_cards WHERE id = $1`, cardID) + _, _ = db.Conn.Exec(pool, `DELETE FROM users WHERE id IN ($1, $2)`, userID, adminID) + }) + + // The charge lands at Square while the sweep/clawback flips the sale to + // 'failed' mid-flight. + origClient := SquareClient + SquareClient = &tillSweepResolveClient{SquareClient: square.NewDevClient(), saleID: saleID} + defer func() { SquareClient = origClient }() + + logBuf, restore := captureStdLog(t) + defer restore() + + reqBody := TillSaleRequest{ + ItemType: "gift_card", + Action: "create", + Amount: 50.00, + PaymentMethod: "saved_card", + UserSavedCardID: &cardID, + UserID: &userID, + IdempotencyKey: key, + } + bodyBytes, _ := json.Marshal(reqBody) + req := httptest.NewRequest("POST", "/api/admin/till/sale", bytes.NewReader(bodyBytes)) + req.Header.Set("Authorization", "Bearer "+adminToken) + req.Header.Set("Content-Type", "application/json") + + w := httptest.NewRecorder() + r := chi.NewRouter() + r.Use(mw.RequireAuth) + r.Post("/api/admin/till/sale", CreateTillSale) + r.ServeHTTP(w, req) + + if w.Code != http.StatusInternalServerError { + t.Errorf("expected the sweep-resolved retry to fail the response (500), got %d. body: %s", w.Code, w.Body.String()) + } + + // The sale must NOT be resurrected to 'completed' by the post-charge + // UPDATE — the sweep already resolved it to 'failed'. + var status string + var sqPayID *string + if err := db.Conn.QueryRow(pool, `SELECT status, square_payment_id FROM till_sales WHERE id = $1`, saleID).Scan(&status, &sqPayID); err != nil { + t.Fatalf("failed to query till sale: %v", err) + } + if status != "failed" { + t.Errorf("expected the sweep-resolved sale to stay 'failed' (not resurrected), got %q", status) + } + if sqPayID != nil { + t.Errorf("expected no square_payment_id written on the sweep-resolved sale, got %q", *sqPayID) + } + + // The RowsAffected()==0 branch must have fired its CRITICAL reconciliation + // log (money taken at Square + card funded + row already resolved). + if got := logBuf.String(); !strings.Contains(got, "already resolved (0 rows updated)") { + t.Errorf("expected the CRITICAL 0-rows reconciliation log, got: %s", got) + } +} diff --git a/backend/handlers/user/twofa.go b/backend/handlers/user/twofa.go index 66cb323..9017e8b 100644 --- a/backend/handlers/user/twofa.go +++ b/backend/handlers/user/twofa.go @@ -126,24 +126,43 @@ var twoFAMaxTrackedAttempts = 10_000 // twoFAAttemptState tracks consecutive failed verify attempts for one user. The // per-user mutex serializes the whole verify critical section so concurrent -// attempts from the same user cannot race the limit check. count is atomic so -// the map eviction path can read it without taking the per-user mutex (lock -// ordering forbids mapMu→st.mu: checkTwoFACode holds st.mu then takes mapMu). -// lastMintAt is the disable-flow mint cooldown stamp (see twoFAMintCooldown). +// attempts from the same user cannot race the limit check. count and lastAt are +// atomic so the map eviction path can read them without taking the per-user +// mutex (lock ordering forbids mapMu→st.mu: checkTwoFACode holds st.mu then +// takes mapMu). lastAt is stored as nanoseconds since the Unix epoch so the +// eviction scan and lockedOut read it race-free even on 32-bit platforms — a +// plain time.Time read/write pair there could tear the 8-byte timestamp and +// reset or extend the lockout window. +// lastMintAt is the disable-flow mint cooldown stamp (see twoFAMintCooldown); +// it is only ever touched under st.mu. type twoFAAttemptState struct { mu sync.Mutex count atomic.Int32 - lastAt time.Time + lastAt atomic.Int64 lastMintAt time.Time } +// lastActive returns the state's last-activity timestamp (nanoseconds since the +// Unix epoch, UTC). Reads are atomic so the map eviction scan can call it while +// holding only mapMu. +func (st *twoFAAttemptState) lastActive() time.Time { + return time.Unix(0, st.lastAt.Load()).UTC() +} + +// setLastActive records a last-activity timestamp. Writes happen under st.mu +// (checkTwoFACode) while the eviction scan reads under mapMu only — the atomic +// store makes both race-free. +func (st *twoFAAttemptState) setLastActive(t time.Time) { + st.lastAt.Store(t.UnixNano()) +} + // lockedOut reports whether the state is inside its lockout window: the attempt // counter has reached the cap and the window has not yet elapsed. Such a record // is the rate limit's source of truth for its user and must never be evicted // while in-window — evicting it would silently reset the counter and grant a // fresh guessing budget. func (st *twoFAAttemptState) lockedOut(now time.Time) bool { - return st.count.Load() >= twoFAMaxAttempts && now.Sub(st.lastAt) <= twoFAAttemptWindow + return st.count.Load() >= twoFAMaxAttempts && now.Sub(st.lastActive()) <= twoFAAttemptWindow } var ( @@ -168,7 +187,7 @@ func twoFAAttemptStateFor(userID string) *twoFAAttemptState { var oldestID string var oldestAt time.Time for id, st := range twoFAAttemptMap { - if now.Sub(st.lastAt) > twoFAAttemptWindow { + if now.Sub(st.lastActive()) > twoFAAttemptWindow { // Idle/expired — its counter has already lapsed; safe to evict. delete(twoFAAttemptMap, id) continue @@ -178,8 +197,8 @@ func twoFAAttemptStateFor(userID string) *twoFAAttemptState { // for this user. Never evict (finding-e fix). continue } - if oldestID == "" || st.lastAt.Before(oldestAt) { - oldestID, oldestAt = id, st.lastAt + if at := st.lastActive(); oldestID == "" || at.Before(oldestAt) { + oldestID, oldestAt = id, at } } if len(twoFAAttemptMap) >= twoFAMaxTrackedAttempts && oldestID != "" { @@ -190,13 +209,16 @@ func twoFAAttemptStateFor(userID string) *twoFAAttemptState { // (that would reset its rate limit) and do not grow past the cap: // return a transient, untracked state so THIS request still // proceeds under a fresh budget. - return &twoFAAttemptState{lastAt: now} + st := &twoFAAttemptState{} + st.setLastActive(now) + return st } } st := twoFAAttemptMap[userID] if st == nil { - st = &twoFAAttemptState{lastAt: now} + st = &twoFAAttemptState{} + st.setLastActive(now) twoFAAttemptMap[userID] = st } return st @@ -394,9 +416,9 @@ const ( // (callers return 500); a lockout's pending-code invalidation failure is logged // here and still reported as a lockout. func checkTwoFACode(r *http.Request, userID string, st *twoFAAttemptState, reqCode string) (twoFACodeCheckResult, error) { - if now := clock.Now(); now.Sub(st.lastAt) > twoFAAttemptWindow { + if now := clock.Now(); now.Sub(st.lastActive()) > twoFAAttemptWindow { st.count.Store(0) - st.lastAt = now + st.setLastActive(now) } if st.count.Load() >= twoFAMaxAttempts { return twoFACodeLockedOut, nil @@ -422,7 +444,7 @@ func checkTwoFACode(r *http.Request, userID string, st *twoFAAttemptState, reqCo match, legacy := verifyTwoFACodeHash(reqCode, pendingHash.String) if !match { st.count.Add(1) - st.lastAt = clock.Now() + st.setLastActive(clock.Now()) if st.count.Load() >= twoFAMaxAttempts { // Lockout reached: destroy the pending code so a stolen digest // cannot be replayed against a fresh guessing loop. @@ -453,7 +475,7 @@ func checkTwoFACode(r *http.Request, userID string, st *twoFAAttemptState, reqCo // Success: clear the attempt counter (and any disable-flow mint cooldown) // before the caller performs its action. st.count.Store(0) - st.lastAt = clock.Now() + st.setLastActive(clock.Now()) st.lastMintAt = time.Time{} twoFAResetAttempts(userID) return twoFACodeOK, nil diff --git a/backend/handlers/user/twofa_test.go b/backend/handlers/user/twofa_test.go index 2608ced..146ab84 100644 --- a/backend/handlers/user/twofa_test.go +++ b/backend/handlers/user/twofa_test.go @@ -24,6 +24,7 @@ import ( "os" "regexp" "strings" + "sync" "testing" "time" @@ -681,7 +682,8 @@ func TestTwoFAPepper_LegacyHashUpgrade_OnSuccessfulVerify(t *testing.T) { require.NoError(t, err) // checkTwoFACode (the shared verify path) must accept the legacy hash. - st := &twoFAAttemptState{lastAt: clock.Now()} + st := &twoFAAttemptState{} + st.setLastActive(clock.Now()) req := httptest.NewRequest(http.MethodPost, "/api/user/2fa/verify", nil).WithContext(ctx) result, err := checkTwoFACode(req, userID, st, "123456") require.NoError(t, err) @@ -845,9 +847,12 @@ func TestTwoFAAttemptMap_InLockoutRecordNotEvicted(t *testing.T) { now := clock.Now() for _, id := range []string{"idle_a", "idle_b", "idle_c"} { - twoFAAttemptMap[id] = &twoFAAttemptState{lastAt: now.Add(-time.Minute)} + st := &twoFAAttemptState{} + st.setLastActive(now.Add(-time.Minute)) + twoFAAttemptMap[id] = st } - victim := &twoFAAttemptState{lastAt: now.Add(-time.Second)} + victim := &twoFAAttemptState{} + victim.setLastActive(now.Add(-time.Second)) victim.count.Store(5) twoFAAttemptMap["victim"] = victim @@ -889,7 +894,8 @@ func TestTwoFAAttemptMap_FullOfLockedOut_ReturnsTransient(t *testing.T) { now := clock.Now() for i := 0; i < 3; i++ { - st := &twoFAAttemptState{lastAt: now.Add(-time.Second)} + st := &twoFAAttemptState{} + st.setLastActive(now.Add(-time.Second)) st.count.Store(5) twoFAAttemptMap[fmt.Sprintf("locked_%d", i)] = st } @@ -903,3 +909,188 @@ func TestTwoFAAttemptMap_FullOfLockedOut_ReturnsTransient(t *testing.T) { t.Errorf("expected all 3 locked-out records to survive, got %d", len(twoFAAttemptMap)) } } + +// ============================================================================= +// Attempt-map concurrency (finding-f — lastAt data race) +// ============================================================================= + +// TestTwoFAAttemptMap_ConcurrentVerifyAndEviction is a -race smoke test for the +// attempt-map data race: checkTwoFACode's st.mu-guarded writes to count and +// lastAt run concurrently with twoFAAttemptStateFor's mapMu-only eviction scan +// reading them. lastAt is an atomic.Int64 (nanoseconds since the epoch), so the +// scan and lockedOut read it without st.mu — no mutex inversion (mapMu→st.mu is +// forbidden) and no torn 8-byte timestamp. Asserts every concurrent call +// completes (no deadlock), pre-pinned locked-out records survive the eviction +// pressure, and the map never grows past the cap. +func TestTwoFAAttemptMap_ConcurrentVerifyAndEviction(t *testing.T) { + twoFAAttemptMapMu.Lock() + origMap := twoFAAttemptMap + origCap := twoFAMaxTrackedAttempts + twoFAAttemptMap = make(map[string]*twoFAAttemptState) + twoFAMaxTrackedAttempts = 128 + twoFAAttemptMapMu.Unlock() + t.Cleanup(func() { + twoFAAttemptMapMu.Lock() + twoFAAttemptMap = origMap + twoFAMaxTrackedAttempts = origCap + twoFAAttemptMapMu.Unlock() + }) + + const verifyWorkers = 6 + const evictWorkers = 4 + const iters = 25 + + // Pre-pin locked-out victims so we can assert afterwards that in-window + // lockout records are never evicted under concurrent pressure. + now := clock.Now() + victims := make(map[string]*twoFAAttemptState, verifyWorkers) + twoFAAttemptMapMu.Lock() + for i := 0; i < verifyWorkers; i++ { + st := &twoFAAttemptState{} + st.setLastActive(now.Add(-time.Second)) + st.count.Store(twoFAMaxAttempts) + id := fmt.Sprintf("victim_%d", i) + twoFAAttemptMap[id] = st + victims[id] = st + } + twoFAAttemptMapMu.Unlock() + + var wg sync.WaitGroup + + // Verifiers mirror checkTwoFACode's critical section on shared states: take + // st.mu, reset an expired window, bump the counter, stamp lastAt, and read + // lockedOut — overlapping the eviction scan's lock-free atomic reads. + for w := 0; w < verifyWorkers; w++ { + wg.Add(1) + go func(w int) { + defer wg.Done() + for iter := 0; iter < iters; iter++ { + st := twoFAAttemptStateFor(fmt.Sprintf("verify_%d_%d", w, iter)) + st.mu.Lock() + if now := clock.Now(); now.Sub(st.lastActive()) > twoFAAttemptWindow { + st.count.Store(0) + st.setLastActive(now) + } + _ = st.lockedOut(clock.Now()) + st.count.Add(1) + st.setLastActive(clock.Now()) + st.mu.Unlock() + } + }(w) + } + + // Evictors drive twoFAAttemptStateFor's cap-driven eviction scan, which + // reads count + lastAt WITHOUT st.mu — the access pattern under test. + for w := 0; w < evictWorkers; w++ { + wg.Add(1) + go func(w int) { + defer wg.Done() + for iter := 0; iter < 2000; iter++ { + _ = twoFAAttemptStateFor(fmt.Sprintf("flood_%d_%d", w, iter)) + } + }(w) + } + wg.Wait() + + twoFAAttemptMapMu.Lock() + defer twoFAAttemptMapMu.Unlock() + for id, st := range victims { + if _, ok := twoFAAttemptMap[id]; !ok { + t.Errorf("in-window lockout record %s was evicted under concurrent pressure", id) + } + if !st.lockedOut(clock.Now()) { + t.Errorf("victim %s must still report locked out", id) + } + } + if len(twoFAAttemptMap) > twoFAMaxTrackedAttempts { + t.Errorf("map grew past the cap: %d > %d", len(twoFAAttemptMap), twoFAMaxTrackedAttempts) + } +} + +// TestTwoFA_ConcurrentCheckTwoFACode_NoDeadlock runs the REAL verify path +// concurrently: each goroutine mints its own transaction and user (pgx.Tx is +// not concurrency-safe, so per-goroutine tx avoids sharing one), burns the +// 5-attempt budget to lockout, and asserts lockedOut afterwards — while other +// goroutines hammer the map eviction scan through twoFAAttemptStateFor. The +// test completes only if no goroutine deadlocks on mapMu/st.mu. +func TestTwoFA_ConcurrentCheckTwoFACode_NoDeadlock(t *testing.T) { + twoFAAttemptMapMu.Lock() + origMap := twoFAAttemptMap + origCap := twoFAMaxTrackedAttempts + twoFAAttemptMap = make(map[string]*twoFAAttemptState) + twoFAMaxTrackedAttempts = 64 + twoFAAttemptMapMu.Unlock() + t.Cleanup(func() { + twoFAAttemptMapMu.Lock() + twoFAAttemptMap = origMap + twoFAMaxTrackedAttempts = origCap + twoFAAttemptMapMu.Unlock() + }) + + const workers = 6 + const iters = 15 + var wg sync.WaitGroup + errCh := make(chan error, workers) + + for w := 0; w < workers; w++ { + wg.Add(1) + go func(w int) { + defer wg.Done() + ctx := context.Background() + tx, err := db.Conn.Pool().Begin(ctx) + if err != nil { + errCh <- fmt.Errorf("worker %d begin: %w", w, err) + return + } + defer tx.Rollback(context.Background()) + tctx := db.ContextWithTx(ctx, tx) + for iter := 0; iter < iters; iter++ { + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + errCh <- fmt.Errorf("worker %d iter %d create user: %w", w, iter, err) + return + } + seedPendingTwoFA(t, tctx, tx, userID, "123456") + st := twoFAAttemptStateFor(userID) + for attempt := 1; attempt <= twoFAMaxAttempts; attempt++ { + st.mu.Lock() + res, err := checkTwoFACode(httptest.NewRequest(http.MethodPost, "/api/user/2fa/verify", nil).WithContext(tctx), userID, st, "999999") + st.mu.Unlock() + if err != nil { + errCh <- fmt.Errorf("worker %d iter %d check: %w", w, iter, err) + return + } + want := twoFACodeIncorrect + if attempt == twoFAMaxAttempts { + want = twoFACodeLockedOut + } + if res != want { + errCh <- fmt.Errorf("worker %d iter %d attempt %d: got %v, want %v", w, iter, attempt, res, want) + return + } + } + if !st.lockedOut(clock.Now()) { + errCh <- fmt.Errorf("worker %d iter %d: must be locked out after %d wrong codes", w, iter, twoFAMaxAttempts) + return + } + } + }(w) + } + + // Concurrent map pressure: twoFAAttemptStateFor reads count + lastAt under + // mapMu only, racing the workers' st.mu-guarded writes (the old data race). + for w := 0; w < 4; w++ { + wg.Add(1) + go func(w int) { + defer wg.Done() + for iter := 0; iter < 1000; iter++ { + _ = twoFAAttemptStateFor(fmt.Sprintf("flood_%d_%d", w, iter)) + } + }(w) + } + wg.Wait() + close(errCh) + for err := range errCh { + t.Error(err) + } +} diff --git a/backend/handlers/webhooks/webhooks_round7_test.go b/backend/handlers/webhooks/webhooks_round7_test.go new file mode 100644 index 0000000..d4e6e14 --- /dev/null +++ b/backend/handlers/webhooks/webhooks_round7_test.go @@ -0,0 +1,397 @@ +//go:build test + +package webhooks + +// Round 7 regression tests — webhook money paths found at 0% coverage: +// +// (a) handleDisputeStateUpdated's findPaymentByDisputeID fallback when a +// dispute.state.updated payload carries no resolvable Square payment id. +// (b) clawbackOneTillSale's non-gift-card branch (a definitively-failed charge +// marks the till sale failed WITHOUT reversing any gift-card funding). +// (c) handleDisputeEvidence / handleTerminalCheckout — informational dispatch +// paths that must complete 200 + log without mutating state. + +import ( + "bytes" + "context" + "encoding/json" + "log" + "net/http" + "strings" + "testing" + + "crussell/db" + "crussell/testutils/fixtures" +) + +// ============================================================================= +// Helpers +// ============================================================================= + +// createWebhookTestGiftCard inserts a funded gift card and returns its id. +func createWebhookTestGiftCard(t *testing.T, amount float64) string { + t.Helper() + adminID, err := fixtures.CreateTestAdminUser(db.Conn) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + var id string + if err := db.Conn.QueryRow(context.Background(), ` + INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by, is_inventory, voucher_type_at_purchase) + VALUES ($1, $1, $2, FALSE, 'SPV') + RETURNING id + `, amount, adminID).Scan(&id); err != nil { + t.Fatalf("failed to create gift card: %v", err) + } + return id +} + +// createWebhookTestTillSale inserts a pending till sale with the given item +// type and item id (nil for a NULL item_id) and returns the sale id. +func createWebhookTestTillSale(t *testing.T, squarePaymentID, itemType string, itemID any) string { + t.Helper() + adminID, err := fixtures.CreateTestAdminUser(db.Conn) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + var saleID string + if err := db.Conn.QueryRow(context.Background(), ` + INSERT INTO till_sales (item_type, item_id, description, quantity, unit_price, total_amount, + payment_method, status, square_payment_id, created_by, created_at, updated_at) + VALUES ($1, $2, 'webhook round7 test', 1, 40.00, 40.00, 'online_square', 'pending', + $3, $4, NOW(), NOW()) + RETURNING id + `, itemType, itemID, squarePaymentID, adminID).Scan(&saleID); err != nil { + t.Fatalf("failed to create pending till sale: %v", err) + } + return saleID +} + +// createWebhookTestBookingPayment creates a completed payment bound to a fresh +// booking and returns the local payment id and booking id. A booking-scoped +// payment makes the critical-notification assertions below attributable to one +// booking, so they cannot race with other tests' NULL-booking rows. +func createWebhookTestBookingPayment(t *testing.T) (payID, bookingID string) { + t.Helper() + userID, err := fixtures.CreateTestUser(db.Conn) + if err != nil { + t.Fatalf("failed to create test user: %v", err) + } + serviceID, err := fixtures.CreateTestService(db.Conn) + if err != nil { + t.Fatalf("failed to create test service: %v", err) + } + bookingID, err = fixtures.CreateTestBooking(db.Conn, userID, serviceID) + if err != nil { + t.Fatalf("failed to create test booking: %v", err) + } + payID, err = fixtures.CreateTestPayment(db.Conn, bookingID, 10.00, "online_square", "full", "completed") + if err != nil { + t.Fatalf("failed to create test payment: %v", err) + } + return payID, bookingID +} + +// countUnackedCriticalNotificationsForBooking returns the number of +// unacknowledged critical_payment_log notifications for a booking. +func countUnackedCriticalNotificationsForBooking(t *testing.T, bookingID string) int { + t.Helper() + var n int + if err := db.Conn.QueryRow(context.Background(), ` + SELECT COUNT(*) FROM admin_notifications + WHERE reason = 'critical_payment_log' AND booking_id = $1 AND acknowledged_at IS NULL + `, bookingID).Scan(&n); err != nil { + t.Fatalf("failed to count critical notifications for booking %s: %v", bookingID, err) + } + return n +} + +// ============================================================================= +// (a) handleDisputeStateUpdated — findPaymentByDisputeID fallback +// ============================================================================= + +// TestWebhook_DisputeStateUpdated_Lost_EmptyPaymentID_FallsBackToDisputeRow +// covers the findPaymentByDisputeID fallback: a dispute.state.updated event +// whose disputed_payment.payment_id is empty cannot resolve the payment via +// square_payment_id, so the handler recovers it from the seeded disputes row. +// A LOST state must still mark the payment failed and raise a CRITICAL +// notification — the fallback must not silently drop the chargeback. +func TestWebhook_DisputeStateUpdated_Lost_EmptyPaymentID_FallsBackToDisputeRow(t *testing.T) { + payID, bookingID := createWebhookTestBookingPayment(t) + if _, err := db.Conn.Exec(context.Background(), ` + INSERT INTO disputes (square_dispute_id, payment_id, status, amount, reason) + VALUES ('dts_fallback_1', $1, 'open', 12.34, 'NO_KNOWLEDGE') + `, payID); err != nil { + t.Fatalf("failed to seed dispute row: %v", err) + } + + event := SquareWebhookEvent{ + Type: "dispute.state.updated", + EventID: "evt_dispute_fallback_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "dispute", + "id": "dts_fallback_1", + "object": { + "dispute": { + "id": "dts_fallback_1", + "state": "LOST", + "amount_money": {"amount": 1234, "currency": "GBP"}, + "reason": "NO_KNOWLEDGE", + "disputed_payment": {"payment_id": ""} + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getDisputeStatus(t, "dts_fallback_1"); got != "lost" { + t.Errorf("expected dispute status 'lost', got %q", got) + } + if got := getPaymentStatus(t, payID); got != "failed" { + t.Errorf("expected payment 'failed' after lost dispute recovered via dispute row, got %q", got) + } + // The lost dispute is a CRITICAL money event — the admin notification + // centre must surface it for the payment's booking. + if n := countUnackedCriticalNotificationsForBooking(t, bookingID); n != 1 { + t.Errorf("expected 1 unacknowledged critical_payment_log notification for booking %s, got %d", bookingID, n) + } + if n := countWebhookEvents(t, event.EventID); n != 1 { + t.Errorf("expected 1 dedup row, got %d", n) + } +} + +// TestWebhook_DisputeStateUpdated_EmptyPaymentID_NoDisputeRow_NoMutation +// covers the fallback's dead end: when neither the payload's (empty) Square +// payment id nor the disputes table yields a payment, the handler returns +// success WITHOUT mutating state — payment untouched, no disputes row, no +// notification — and still commits the dedup row (Square's retry is +// acknowledged 200, not re-dispatched forever). +func TestWebhook_DisputeStateUpdated_EmptyPaymentID_NoDisputeRow_NoMutation(t *testing.T) { + payID, bookingID := createWebhookTestBookingPayment(t) + + event := SquareWebhookEvent{ + Type: "dispute.state.updated", + EventID: "evt_dispute_no_row_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "dispute", + "id": "dts_no_dispute_row_1", + "object": { + "dispute": { + "id": "dts_no_dispute_row_1", + "state": "LOST", + "amount_money": {"amount": 1234, "currency": "GBP"}, + "reason": "NO_KNOWLEDGE", + "disputed_payment": {"payment_id": ""} + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + var n int + if err := db.Conn.QueryRow(context.Background(), + "SELECT COUNT(*) FROM disputes WHERE square_dispute_id = 'dts_no_dispute_row_1'").Scan(&n); err != nil { + t.Fatalf("failed to count disputes: %v", err) + } + if n != 0 { + t.Errorf("expected no disputes row when the fallback finds no payment, got %d", n) + } + if got := getPaymentStatus(t, payID); got != "completed" { + t.Errorf("expected payment untouched ('completed') when the fallback finds no dispute, got %q", got) + } + // No critical notification: the handler returns before any insert. + if n := countUnackedCriticalNotificationsForBooking(t, bookingID); n != 0 { + t.Errorf("expected NO critical notification when the fallback finds no dispute, got %d", n) + } + if n := countWebhookEvents(t, event.EventID); n != 1 { + t.Errorf("expected 1 dedup row (the no-op dispatch still commits), got %d", n) + } +} + +// ============================================================================= +// (b) clawbackOneTillSale — non-gift-card branch +// ============================================================================= + +// TestWebhook_PaymentUpdated_Failed_NonGiftCardSale_MarksFailedNoClawback +// covers the non-gift-card branch of clawbackOneTillSale: a retail_product +// till sale (item_type != "gift_card") funded by a definitively-failed Square +// charge is marked failed WITHOUT reversing any gift-card funding — even when +// the sale's item_id happens to reference a real, funded gift card (the LEFT +// JOIN would find it; the item_type guard must short-circuit before any +// reversal). +func TestWebhook_PaymentUpdated_Failed_NonGiftCardSale_MarksFailedNoClawback(t *testing.T) { + const squarePaymentID = "sqp_clawback_retail" + giftCardID := createWebhookTestGiftCard(t, 60.00) + saleID := createWebhookTestTillSale(t, squarePaymentID, "retail_product", giftCardID) + + w := deliverPaymentUpdatedFailed(t, squarePaymentID) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getTillSaleStatus(t, saleID); got != "failed" { + t.Errorf("expected till sale 'failed', got %q", got) + } + // No gift-card balance was reversed: the card referenced by the sale's + // item_id keeps its full £60.00 funding. + total, remaining := getGiftCardFunding(t, giftCardID) + if total != 60.00 || remaining != 60.00 { + t.Errorf("expected gift card funding untouched (no clawback for a non-gift-card sale), got total=%v remaining=%v", total, remaining) + } +} + +// TestWebhook_PaymentUpdated_Failed_GiftCardSaleMissingCard_MarksFailedNoClawback +// covers the nil-isCreate variant of the same branch: a gift_card till sale +// whose item_id points at NO gift card (dangling id) makes the LEFT JOIN yield +// a NULL is_create — the branch must still mark the sale failed without +// attempting any reversal. +func TestWebhook_PaymentUpdated_Failed_GiftCardSaleMissingCard_MarksFailedNoClawback(t *testing.T) { + const squarePaymentID = "sqp_clawback_dangling" + saleID := createWebhookTestTillSale(t, squarePaymentID, "gift_card", "GCMISSING001") + + w := deliverPaymentUpdatedFailed(t, squarePaymentID) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getTillSaleStatus(t, saleID); got != "failed" { + t.Errorf("expected till sale 'failed', got %q", got) + } + // The missing card means there is nothing to claw back — and the handler + // must not error out: the webhook acknowledges 200 with a dedup row. + if n := countWebhookEvents(t, "evt_"+squarePaymentID); n != 1 { + t.Errorf("expected 1 dedup row, got %d", n) + } +} + +// ============================================================================= +// (c) handleDisputeEvidence / handleTerminalCheckout — informational dispatch +// ============================================================================= + +// TestWebhook_DisputeEvidence_InformationalOnly covers both +// dispute.evidence.created and dispute.evidence.deleted: the handler logs and +// acknowledges 200 without writing a disputes row or raising any notification. +func TestWebhook_DisputeEvidence_InformationalOnly(t *testing.T) { + cases := []struct { + eventType string + disputeID string + eventID string + }{ + {"dispute.evidence.created", "dts_evidence_created_1", "evt_evidence_created_1"}, + {"dispute.evidence.deleted", "dts_evidence_deleted_1", "evt_evidence_deleted_1"}, + } + for _, tc := range cases { + event := SquareWebhookEvent{ + Type: tc.eventType, + EventID: tc.eventID, + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "dispute", + "id": "` + tc.disputeID + `", + "object": { + "dispute": { + "id": "` + tc.disputeID + `", + "state": "EVIDENCE_REQUIRED" + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200 for %s, got %d: %s", tc.eventType, w.Code, w.Body.String()) + } + // No state mutation: no disputes row is written for an evidence event. + var n int + if err := db.Conn.QueryRow(context.Background(), + "SELECT COUNT(*) FROM disputes WHERE square_dispute_id = $1", tc.disputeID).Scan(&n); err != nil { + t.Fatalf("failed to count disputes: %v", err) + } + if n != 0 { + t.Errorf("expected no disputes row from %s, got %d", tc.eventType, n) + } + if got := countWebhookEvents(t, tc.eventID); got != 1 { + t.Errorf("expected 1 dedup row for %s, got %d", tc.eventID, got) + } + } + + // The handler logs the evidence event (informational only). + var buf bytes.Buffer + oldOutput := log.Writer() + log.SetOutput(&buf) + defer log.SetOutput(oldOutput) + event := SquareWebhookEvent{ + Type: "dispute.evidence.created", + EventID: "evt_evidence_log_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "dispute", + "id": "dts_evidence_log_1", + "object": {"dispute": {"id": "dts_evidence_log_1", "state": "UNDER_REVIEW"}} + }`), + } + if w := deliverWebhook(t, event); w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if out := buf.String(); !strings.Contains(out, "dispute evidence event for dispute") { + t.Errorf("expected an informational evidence log line, got:\n%s", out) + } +} + +// TestWebhook_TerminalCheckout_InformationalOnly covers both +// terminal.checkout.created and terminal.checkout.updated: the handler logs +// and acknowledges 200 without writing any terminal_checkouts row. +func TestWebhook_TerminalCheckout_InformationalOnly(t *testing.T) { + cases := []struct { + eventType string + checkoutID string + eventID string + }{ + {"terminal.checkout.created", "chk_round7_created_1", "evt_terminal_created_1"}, + {"terminal.checkout.updated", "chk_round7_updated_1", "evt_terminal_updated_1"}, + } + for _, tc := range cases { + event := SquareWebhookEvent{ + Type: tc.eventType, + EventID: tc.eventID, + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{"type": "terminal.checkout", "id": "` + tc.checkoutID + `"}`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200 for %s, got %d: %s", tc.eventType, w.Code, w.Body.String()) + } + // No state mutation: no terminal_checkouts row is written. + var n int + if err := db.Conn.QueryRow(context.Background(), + "SELECT COUNT(*) FROM terminal_checkouts WHERE checkout_id = $1", tc.checkoutID).Scan(&n); err != nil { + t.Fatalf("failed to count terminal checkouts: %v", err) + } + if n != 0 { + t.Errorf("expected no terminal_checkouts row from %s, got %d", tc.eventType, n) + } + if got := countWebhookEvents(t, tc.eventID); got != 1 { + t.Errorf("expected 1 dedup row for %s, got %d", tc.eventID, got) + } + } + + var buf bytes.Buffer + oldOutput := log.Writer() + log.SetOutput(&buf) + defer log.SetOutput(oldOutput) + event := SquareWebhookEvent{ + Type: "terminal.checkout.updated", + EventID: "evt_terminal_log_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{"type": "terminal.checkout", "id": "chk_round7_log_1"}`), + } + if w := deliverWebhook(t, event); w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if out := buf.String(); !strings.Contains(out, "terminal.checkout event received") { + t.Errorf("expected an informational terminal.checkout log line, got:\n%s", out) + } +} diff --git a/backend/internal/square/square_dev.go b/backend/internal/square/square_dev.go index d2122af..43627ad 100644 --- a/backend/internal/square/square_dev.go +++ b/backend/internal/square/square_dev.go @@ -22,15 +22,18 @@ package square // // FAULT-INJECTION TOGGLES. The mock exposes opt-in toggles (ShouldFail, // FailRefundCode, ForceCheckoutState, ForceRefundPending, FailCreateCheckout, -// FailAfterCommit, SimulateCardTokenUsed) that let dev/tests drive Square -// failure modes that are otherwise only reachable against the real API. -// FailAfterCommit simulates the exact "charged but response lost → same-key +// FailAfterCommit, SimulateSourceUsed, ForcePaymentStatus) that let dev/tests +// drive Square failure modes that are otherwise only reachable against the real +// API. FailAfterCommit simulates the exact "charged but response lost → same-key // retry" prod scenario: CreatePayment COMMITS the charge (retaining the key // and source in the ledgers exactly like a successful charge) and THEN returns // a 5xx-style error to the caller. A subsequent CreatePayment with the SAME // key + SAME source dedups to the committed payment, proving no double charge. -// SimulateCardTokenUsed simulates Square's CARD_TOKEN_USED rejection of a card -// token (cnon: nonce) reused after a previous save. +// SimulateSourceUsed simulates Square's SOURCE_USED rejection of a card source +// (cnon: nonce) reused after a previous save. ForcePaymentStatus forces +// CreatePayment's payment status while returning nil error — the "Square +// returned 200 with a non-terminal payment" prod scenario, so a status-blind +// handler (records 'completed' on nil error alone) is caught in dev. // // REAL-API SAFETY GUARD. A `//go:build dev` build must never silently route to // the real PRODUCTION Square API on an env-string match alone — a typo'd or @@ -111,18 +114,29 @@ type MockClient struct { // committed payment — never a second charge — exercising the retry path // devs hit in prod when Square processes a charge but the response is lost. FailAfterCommit bool - // SimulateCardTokenUsed makes CreateCardOnFile enforce Square's - // CARD_TOKEN_USED rejection: a card token (cnon: nonce) already used to - // create a card on this mock instance is rejected with the same structured - // 400 CARD_TOKEN_USED error real Square returns. Off by default — dev/test - // flows reuse plain "cnon:test-card"-style tokens across requests, so - // enforcement is enabled only in tests that exercise the reused-token - // rejection. UsedCardTokens() reports the tokens consumed so far. - SimulateCardTokenUsed bool - // usedCardTokens records card tokens consumed by CreateCardOnFile while - // SimulateCardTokenUsed is enabled (Square consumes a cnon: nonce on card - // creation, so reusing it is rejected with CARD_TOKEN_USED). - usedCardTokens map[string]bool + // SimulateSourceUsed makes CreateCardOnFile enforce Square's SOURCE_USED + // rejection: a card source (cnon: nonce) already used to create a card on + // this mock instance is rejected with the same structured 400 SOURCE_USED + // error real Square's CreateCard API returns (SOURCE_USED — NOT the + // CreatePayment code CARD_TOKEN_USED). Off by default — dev/test flows + // reuse plain "cnon:test-card"-style tokens across requests, so enforcement + // is enabled only in tests that exercise the reused-source rejection. + // UsedSources() reports the sources consumed so far. + SimulateSourceUsed bool + // usedSources records card sources consumed by CreateCardOnFile while + // SimulateSourceUsed is enabled (Square consumes a cnon: nonce on card + // creation, so reusing it is rejected with SOURCE_USED). + usedSources map[string]bool + // ForcePaymentStatus forces CreatePayment's payment status instead of the + // default "COMPLETED" (or "APPROVED" for autocomplete=false). When set, + // CreatePayment returns a payment carrying the forced status with nil + // error — the "Square returned 200 with a non-terminal payment" prod + // scenario. It proves a status-blind handler (one that records 'completed' + // on nil error alone) is a regression: the client surfaces Status + // faithfully (paymentFromSquare never errors on a non-terminal status — + // see square_http_client.go), so only the handler's own status check can + // catch a FAILED/CANCELED/PENDING/APPROVED payment. + ForcePaymentStatus string } type devProdClient struct{} @@ -211,7 +225,7 @@ func NewDevClient() SquareClient { refundByKey: make(map[string]*RefundResult), customers: make(map[string]*CustomerResult), completed: make(map[string]*PaymentResult), - usedCardTokens: make(map[string]bool), + usedSources: make(map[string]bool), } } } @@ -326,6 +340,13 @@ func (m *MockClient) CreatePayment(ctx context.Context, req CreatePaymentReq) (* if req.Autocomplete != nil && !*req.Autocomplete { status = "APPROVED" } + if m.ForcePaymentStatus != "" { + // Drive the "Square returned 200 with a non-terminal payment" prod + // scenario: the payment comes back with a non-default status and nil + // error, so a status-blind caller (records 'completed' on nil error + // alone) is exposed as a regression. + status = m.ForcePaymentStatus + } amount := req.Amount tipAmount := int64(0) @@ -343,6 +364,12 @@ func (m *MockClient) CreatePayment(ctx context.Context, req CreatePaymentReq) (* } paymentID := fmt.Sprintf("pay_mock_%d", now.UnixNano()) + // Sign-convention parity (finding A): Square reports processing_fee amounts + // as NEGATIVE on the wire, and paymentFromSquare negates them so + // PaymentResult.Fees is POSITIVE — the value handlers store as p.fees. The + // mock fabricates the same positive magnitude directly: online rate 1.4% + + // 25p (amount*14/1000+25). Mock and real client must agree on the sign; + // see TestProcessingFeeSign_Parity_MockAndRealClientAgree. fees := amount*14/1000 + 25 // online rate: 1.4% + 25p locationID := req.LocationID @@ -404,6 +431,26 @@ func (m *MockClient) CreateCheckout(ctx context.Context, req CreateCheckoutReq) if m.FailCreateCheckout { return nil, fmt.Errorf("mock: checkout creation failed (simulated failure)") } + // Real Square's TerminalCheckout API REQUIRES device_options.device_id: a + // checkout with an empty device id is rejected with a 400. The real client + // resolves the per-request device ID with an env fallback + // (SQUARE_TERMINAL_DEVICE_ID, square_http_client.go:516); the mock mirrors + // the SAME resolution and rejects when neither is set — so a missing + // terminal misconfiguration is caught in dev instead of silently + // "succeeding" where prod 400s. + deviceID := req.DeviceID + if deviceID == "" { + deviceID = os.Getenv("SQUARE_TERMINAL_DEVICE_ID") + } + if deviceID == "" { + return nil, &squareAPIError{ + Code: "INVALID_REQUEST_ERROR", + Detail: "device_options.device_id is required to create a terminal checkout", + Category: "INVALID_REQUEST_ERROR", + StatusCode: http.StatusBadRequest, + err: errors.New("square: device_options.device_id is required for a terminal checkout (set SQUARE_TERMINAL_DEVICE_ID or pass DeviceID)"), + } + } log.Printf("[SQUARE-MOCK] CreateCheckout: amount=%d, allowTipping=%v, reference=%s", req.Amount, req.AllowTipping, req.ReferenceID) now := clock.Now().UTC() @@ -488,6 +535,15 @@ func (m *MockClient) CreateCheckout(ctx context.Context, req CreateCheckoutReq) ReferenceID: req.ReferenceID, } m.completed[checkoutID] = paymentResult + // Real Square registers the terminal payment under its own ID: + // GET /v2/payments/{id} succeeds on a completed terminal + // checkout's payment in prod, but failed in dev because the + // payment was never added to m.payments (finding I). Mirror prod + // by registering it under both the ID and SquarePayID keys, exactly + // like CreatePayment, so the reconcile/sweep GetPayment path + // behaves identically. + m.payments[paymentID] = paymentResult + m.payments[paymentResult.SquarePayID] = paymentResult m.checkouts[checkoutID].Status = "COMPLETED" m.checkouts[checkoutID].UpdatedAt = payNow.Format(time.RFC3339) m.checkouts[checkoutID].PaymentIDs = []string{paymentID} @@ -697,15 +753,15 @@ func (m *MockClient) RefundKeyCount() int { return len(m.refundByKey) } -// UsedCardTokens returns the card tokens consumed by CreateCardOnFile while -// SimulateCardTokenUsed is enabled. Test accessor for asserting that a reused -// token is rejected with CARD_TOKEN_USED after a previous save. -func (m *MockClient) UsedCardTokens() []string { +// UsedSources returns the card sources consumed by CreateCardOnFile while +// SimulateSourceUsed is enabled. Test accessor for asserting that a reused +// source is rejected with SOURCE_USED after a previous save. +func (m *MockClient) UsedSources() []string { m.mu.RLock() defer m.mu.RUnlock() - out := make([]string, 0, len(m.usedCardTokens)) - for tok := range m.usedCardTokens { - out = append(out, tok) + out := make([]string, 0, len(m.usedSources)) + for src := range m.usedSources { + out = append(out, src) } return out } @@ -738,16 +794,18 @@ func (m *MockClient) CreateCardOnFile(ctx context.Context, userID, cardToken, cu m.mu.Lock() defer m.mu.Unlock() - if m.SimulateCardTokenUsed && m.usedCardTokens[cardToken] { + if m.SimulateSourceUsed && m.usedSources[cardToken] { // Real Square consumes a cnon: nonce on card creation — reusing it to - // create another card is rejected with CARD_TOKEN_USED. The mock - // mirrors that structured 400 rejection (opt-in, see the struct doc). + // create another card is rejected with SOURCE_USED (the CreateCard + // error; CARD_TOKEN_USED is a CreatePayment code and would be wrong + // here). The mock mirrors that structured 400 rejection (opt-in, see + // the struct doc). return nil, &squareAPIError{ - Code: "CARD_TOKEN_USED", - Detail: "The card token has already been used.", + Code: "SOURCE_USED", + Detail: "The provided source id was already used to create a card", Category: "INVALID_REQUEST_ERROR", StatusCode: http.StatusBadRequest, - err: fmt.Errorf("square: card token %s has already been used", tokenPrefix(cardToken)), + err: fmt.Errorf("square: card source %s has already been used to create a card", tokenPrefix(cardToken)), } } @@ -779,8 +837,8 @@ func (m *MockClient) CreateCardOnFile(ctx context.Context, userID, cardToken, cu } m.cards[userID][cardID] = card m.cardByToken[card.CardID] = card - if m.SimulateCardTokenUsed { - m.usedCardTokens[cardToken] = true + if m.SimulateSourceUsed { + m.usedSources[cardToken] = true } log.Printf("[SQUARE-MOCK] Card created: id=%s, brand=%s, last4=%s", cardID, card.Brand, card.Last4) return card, nil @@ -799,6 +857,13 @@ func (m *MockClient) GetCardsOnFile(ctx context.Context, userID string) ([]CardO var cards []CardOnFile for _, card := range userCards { + // Real Square's List Cards API EXCLUDES disabled cards by default + // (the client sends no include_disabled param) — a disabled/deleted + // card disappears from GetCardsOnFile. Mirror that so dev parity + // matches prod (finding C). + if !card.Enabled { + continue + } cards = append(cards, *card) } return cards, nil diff --git a/backend/internal/square/square_dev_test.go b/backend/internal/square/square_dev_test.go index 67d8fb1..51fc4c4 100644 --- a/backend/internal/square/square_dev_test.go +++ b/backend/internal/square/square_dev_test.go @@ -83,6 +83,7 @@ func TestDevClient_CreateCheckout_PendingThenCompleted(t *testing.T) { IdempotencyKey: "checkout-key-1", ReferenceID: "booking-456", AllowTipping: true, + DeviceID: "dvc_test", } result, err := client.CreateCheckout(ctx, req) @@ -126,6 +127,7 @@ func TestDevClient_CreateCheckout_DeadlineDurationFormat(t *testing.T) { Currency: "GBP", IdempotencyKey: "checkout-deadline", ReferenceID: "deadline-ref", + DeviceID: "dvc_test", }) require.NoError(t, err) assert.Equal(t, "PT5M", result.Deadline, "deadline_duration must be an RFC 3339 duration, not a timestamp") @@ -146,6 +148,7 @@ func TestDevClient_CreateCheckout_NoTip(t *testing.T) { IdempotencyKey: "checkout-key-notip", ReferenceID: "booking-789", AllowTipping: false, + DeviceID: "dvc_test", } result, err := client.CreateCheckout(ctx, req) @@ -267,11 +270,11 @@ func TestDevClient_CardOnFile_Delete(t *testing.T) { err = client.DeleteCardOnFile(ctx, card.ID) require.NoError(t, err) + // Square's List Cards API excludes disabled cards by default — a + // disabled/deleted card is no longer returned by GetCardsOnFile. cards, err := client.GetCardsOnFile(ctx, userID) require.NoError(t, err) - - require.Len(t, cards, 1) - assert.False(t, cards[0].Enabled) + assert.Empty(t, cards, "a disabled card must be excluded from GetCardsOnFile like Square's List Cards") } func TestDevClient_CardOnFile_DeleteNotFound(t *testing.T) { @@ -724,10 +727,11 @@ func TestDevClient_DeleteCardOnFile_SoftDelete(t *testing.T) { err = client.DeleteCardOnFile(ctx, card.ID) require.NoError(t, err) + // Square's List Cards API excludes disabled cards by default — the + // disabled card is no longer returned by GetCardsOnFile. cards, err := client.GetCardsOnFile(ctx, userID) require.NoError(t, err) - require.Len(t, cards, 1) - assert.False(t, cards[0].Enabled) + assert.Empty(t, cards, "a disabled card must be excluded from GetCardsOnFile like Square's List Cards") } func TestDevClient_GetCardsOnFile_Empty(t *testing.T) { @@ -900,6 +904,7 @@ func TestDevClient_GetCheckout_StillPending(t *testing.T) { Currency: "GBP", IdempotencyKey: "pending-checkout", ReferenceID: "pending-ref", + DeviceID: "dvc_test", }) require.NoError(t, err) assert.Equal(t, "PENDING", result.Status) @@ -919,6 +924,7 @@ func TestDevClient_CreateCheckout_HoldCheckouts(t *testing.T) { Currency: "GBP", IdempotencyKey: "hold-checkout", ReferenceID: "hold-ref", + DeviceID: "dvc_test", }) require.NoError(t, err) assert.Equal(t, "PENDING", result.Status) @@ -940,6 +946,7 @@ func TestDevClient_GetCheckout_ForceInProgress(t *testing.T) { Currency: "GBP", IdempotencyKey: "in-progress-checkout", ReferenceID: "in-progress-ref", + DeviceID: "dvc_test", }) require.NoError(t, err) assert.Equal(t, "IN_PROGRESS", result.Status) @@ -971,6 +978,7 @@ func TestDevClient_GetCheckout_ForceCancelRequested(t *testing.T) { Currency: "GBP", IdempotencyKey: "cancel-requested-checkout", ReferenceID: "cancel-requested-ref", + DeviceID: "dvc_test", }) require.NoError(t, err) assert.Equal(t, "CANCEL_REQUESTED", result.Status) @@ -1000,6 +1008,7 @@ func TestDevClient_GetCheckout_ForceCanceled(t *testing.T) { Currency: "GBP", IdempotencyKey: "canceled-checkout", ReferenceID: "canceled-ref", + DeviceID: "dvc_test", }) require.NoError(t, err) assert.Equal(t, "CANCELED", result.Status) @@ -1228,6 +1237,7 @@ func TestDevClient_CancelCheckout_CancelsPending(t *testing.T) { Currency: "GBP", IdempotencyKey: "cancel-checkout", ReferenceID: "cancel-ref", + DeviceID: "dvc_test", }) require.NoError(t, err) assert.Equal(t, "PENDING", result.Status) @@ -1261,6 +1271,7 @@ func TestDevClient_CancelCheckout_CompletedIsNoOp(t *testing.T) { Currency: "GBP", IdempotencyKey: "cancel-completed", ReferenceID: "cancel-comp-ref", + DeviceID: "dvc_test", }) require.NoError(t, err) @@ -1598,39 +1609,41 @@ func TestDevClient_CreatePayment_RejectsOversizedIdempotencyKey(t *testing.T) { assert.Equal(t, "COMPLETED", ok.Status) } -// TestDevClient_CreateCardOnFile_SimulateCardTokenUsed locks the CARD_TOKEN_USED -// simulation: when SimulateCardTokenUsed is enabled, a card token (cnon: nonce) +// TestDevClient_CreateCardOnFile_SimulateSourceUsed locks the SOURCE_USED +// simulation: when SimulateSourceUsed is enabled, a card source (cnon: nonce) // reused after a previous save is rejected with Square's structured 400 -// CARD_TOKEN_USED error. Off by default (dev/test flows reuse plain test -// tokens across requests), so the toggle must not reject reuse when disabled. -func TestDevClient_CreateCardOnFile_SimulateCardTokenUsed(t *testing.T) { +// SOURCE_USED error (the CreateCard error for a reused source — NOT the +// CreatePayment code CARD_TOKEN_USED). Off by default (dev/test flows reuse +// plain test tokens across requests), so the toggle must not reject reuse when +// disabled. +func TestDevClient_CreateCardOnFile_SimulateSourceUsed(t *testing.T) { client := NewDevClient().(*MockClient) - client.SimulateCardTokenUsed = true + client.SimulateSourceUsed = true ctx := context.Background() card, err := client.CreateCardOnFile(ctx, "user-token-used", "cnon:single-use-nonce", "cus_test123") require.NoError(t, err) assert.NotEmpty(t, card.ID) - // Reusing the same token → Square's CARD_TOKEN_USED rejection. + // Reusing the same source → Square's SOURCE_USED rejection. _, err = client.CreateCardOnFile(ctx, "user-token-used-2", "cnon:single-use-nonce", "cus_test123") require.Error(t, err) - assert.Equal(t, "CARD_TOKEN_USED", ErrorCode(err)) + assert.Equal(t, "SOURCE_USED", ErrorCode(err)) assert.Equal(t, http.StatusBadRequest, ErrorStatusCode(err)) - assert.ElementsMatch(t, []string{"cnon:single-use-nonce"}, client.UsedCardTokens()) + assert.ElementsMatch(t, []string{"cnon:single-use-nonce"}, client.UsedSources()) - // A fresh token still works. + // A fresh source still works. fresh, err := client.CreateCardOnFile(ctx, "user-token-used-2", "cnon:fresh-nonce", "cus_test123") require.NoError(t, err) assert.NotEmpty(t, fresh.ID) - // With the toggle OFF (default), reusing a token is allowed — dev/test + // With the toggle OFF (default), reusing a source is allowed — dev/test // flows reuse plain "cnon:test-card"-style tokens across requests. - client.SimulateCardTokenUsed = false + client.SimulateSourceUsed = false _, err = client.CreateCardOnFile(ctx, "user-token-reuse", "cnon:reused-token", "cus_test123") require.NoError(t, err) _, err = client.CreateCardOnFile(ctx, "user-token-reuse-2", "cnon:reused-token", "cus_test123") - require.NoError(t, err, "with SimulateCardTokenUsed off, token reuse must be allowed") + require.NoError(t, err, "with SimulateSourceUsed off, source reuse must be allowed") } // TestIdempotencyKeyLength_Parity_MockAndRealClientAgree asserts the mock and @@ -1668,37 +1681,38 @@ func TestIdempotencyKeyLength_Parity_MockAndRealClientAgree(t *testing.T) { assert.Equal(t, http.StatusBadRequest, ErrorStatusCode(mockErr)) } -// TestCardTokenUsed_Parity_MockAndRealClientAgree asserts the mock and the -// real HTTP client AGREE on the reused-card-token rejection: both surface the -// same structured code (CARD_TOKEN_USED) and HTTP status (400). -func TestCardTokenUsed_Parity_MockAndRealClientAgree(t *testing.T) { +// TestSourceUsed_Parity_MockAndRealClientAgree asserts the mock and the +// real HTTP client AGREE on the reused-card-source rejection: both surface the +// same structured code (SOURCE_USED — Square's CreateCard error, NOT the +// CreatePayment code CARD_TOKEN_USED) and HTTP status (400). +func TestSourceUsed_Parity_MockAndRealClientAgree(t *testing.T) { ctx := context.Background() - token := "cnon:reused-nonce" + source := "cnon:reused-nonce" - // Real client: Square's 400 CARD_TOKEN_USED response surfaces as a + // Real client: Square's 400 SOURCE_USED response surfaces as a // structured squareAPIError. srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusBadRequest) - _, _ = w.Write([]byte(`{"errors":[{"category":"INVALID_REQUEST_ERROR","code":"CARD_TOKEN_USED","detail":"The card token has already been used."}]}`)) + _, _ = w.Write([]byte(`{"errors":[{"category":"INVALID_REQUEST_ERROR","code":"SOURCE_USED","detail":"The provided source id was already used to create a card"}]}`)) })) defer srv.Close() hc := &httpClient{baseURL: srv.URL, token: "t", http: srv.Client()} - _, realErr := createCardOnFileHTTPWithClient(ctx, "user_1", token, "cus_1", hc) + _, realErr := createCardOnFileHTTPWithClient(ctx, "user_1", source, "cus_1", hc) require.Error(t, realErr) - // Mock: with the simulation enabled, reusing a consumed token surfaces the + // Mock: with the simulation enabled, reusing a consumed source surfaces the // identical structured error. mock := NewDevClient().(*MockClient) - mock.SimulateCardTokenUsed = true - _, err := mock.CreateCardOnFile(ctx, "user_1", token, "cus_1") + mock.SimulateSourceUsed = true + _, err := mock.CreateCardOnFile(ctx, "user_1", source, "cus_1") require.NoError(t, err) - _, mockErr := mock.CreateCardOnFile(ctx, "user_2", token, "cus_1") + _, mockErr := mock.CreateCardOnFile(ctx, "user_2", source, "cus_1") require.Error(t, mockErr) - assert.Equal(t, "CARD_TOKEN_USED", ErrorCode(realErr)) - assert.Equal(t, ErrorCode(realErr), ErrorCode(mockErr), "mock and real client must agree on the error code for a reused card token") - assert.Equal(t, ErrorStatusCode(realErr), ErrorStatusCode(mockErr), "mock and real client must agree on the error status for a reused card token") + assert.Equal(t, "SOURCE_USED", ErrorCode(realErr)) + assert.Equal(t, ErrorCode(realErr), ErrorCode(mockErr), "mock and real client must agree on the error code for a reused card source") + assert.Equal(t, ErrorStatusCode(realErr), ErrorStatusCode(mockErr), "mock and real client must agree on the error status for a reused card source") assert.Equal(t, http.StatusBadRequest, ErrorStatusCode(mockErr)) } @@ -1726,3 +1740,153 @@ func TestEnvResolution_HelperMatchesHTTPClient(t *testing.T) { t.Setenv("SQUARE_ENVIRONMENT", "mock") assert.Equal(t, squareSandboxURL, newHTTPClient().baseURL, "any non-production env resolves the sandbox base URL (never the production URL)") } + +// TestProcessingFeeSign_Parity_MockAndRealClientAgree locks the +// processing-fee sign convention: Square reports processing_fee amounts as +// NEGATIVE on the wire and paymentFromSquare negates them so PaymentResult.Fees +// is POSITIVE — the magnitude the handlers store as p.fees. The mock +// fabricates the same positive magnitude directly, so mock and real client must +// agree on the value (finding A). +func TestProcessingFeeSign_Parity_MockAndRealClientAgree(t *testing.T) { + ctx := context.Background() + amount := int64(5000) + wantFees := int64(5000*14/1000 + 25) // 1.4% + 25p on £50.00 = 95p + + // Real client: Square's wire processing_fee is negative; paymentFromSquare + // must surface the positive magnitude. + pr := paymentFromSquare(&sqPayment{ + ID: "pay_fee", + Status: "COMPLETED", + TotalMoney: sqMoney{Amount: amount, Currency: "GBP"}, + ProcessingFee: []sqFee{ + {AmountMoney: sqMoney{Amount: -wantFees, Currency: "GBP"}, Type: "INITIAL"}, + }, + }) + assert.Equal(t, wantFees, pr.Fees, "real client must negate Square's negative processing_fee into a positive PaymentResult.Fees") + + // Mock: fabricates the same positive fee. + mock := NewDevClient().(*MockClient) + got, err := mock.CreatePayment(ctx, CreatePaymentReq{ + Amount: amount, Currency: "GBP", SourceID: "cnon:test-card", IdempotencyKey: "fee-parity-key", + }) + require.NoError(t, err) + assert.Equal(t, wantFees, got.Fees, "mock and real client must agree on the positive fee magnitude") +} + +// TestDevClient_GetCardsOnFile_ExcludesDisabled locks the List Cards parity: +// real Square's List Cards API EXCLUDES disabled cards by default (the client +// sends no include_disabled param), so a card disabled via DeleteCardOnFile +// must disappear from GetCardsOnFile — exactly like prod (finding C). +func TestDevClient_GetCardsOnFile_ExcludesDisabled(t *testing.T) { + client := NewDevClient().(*MockClient) + ctx := context.Background() + userID := "user-disabled-exclusion" + + c1, err := client.CreateCardOnFile(ctx, userID, "cnon:enabled-card", "cus_test123") + require.NoError(t, err) + c2, err := client.CreateCardOnFile(ctx, userID, "cnon:to-be-disabled", "cus_test123") + require.NoError(t, err) + + require.NoError(t, client.DeleteCardOnFile(ctx, c2.ID)) + + cards, err := client.GetCardsOnFile(ctx, userID) + require.NoError(t, err) + require.Len(t, cards, 1, "only the enabled card must be listed") + assert.Equal(t, c1.ID, cards[0].ID) +} + +// TestDevClient_CreatePayment_ForcePaymentStatus drives the "Square returned +// 200 with a non-terminal payment" prod scenario: with ForcePaymentStatus set, +// CreatePayment returns a payment carrying a non-default status with NIL error +// (the client never errors on a status — see paymentFromSquare). A status-blind +// handler that records 'completed' on nil error alone would mis-record these; +// the toggle makes that regression exercisable in dev (finding D). +func TestDevClient_CreatePayment_ForcePaymentStatus(t *testing.T) { + client := NewDevClient().(*MockClient) + ctx := context.Background() + + for _, status := range []string{"FAILED", "CANCELED", "APPROVED", "PENDING"} { + t.Run(status, func(t *testing.T) { + client.ForcePaymentStatus = status + res, err := client.CreatePayment(ctx, CreatePaymentReq{ + Amount: 5000, Currency: "GBP", SourceID: "cnon:test-card", IdempotencyKey: "forced-status-" + status, + }) + require.NoError(t, err, "a forced status must still return nil error (the client is status-transparent)") + assert.Equal(t, status, res.Status, "the payment must carry the forced status") + + got, err := client.GetPayment(ctx, res.ID) + require.NoError(t, err) + assert.Equal(t, status, got.Status, "GetPayment must surface the same status") + }) + } +} + +// TestDevClient_CreateCheckout_RequiresDeviceID locks the TerminalCheckout +// device_id parity: real Square REQUIRES device_options.device_id (400 on +// empty). The mock resolves the per-request device ID with the same env +// fallback as the real client (SQUARE_TERMINAL_DEVICE_ID) and rejects when +// neither is set — a missing terminal misconfiguration is caught in dev +// (finding H). +func TestDevClient_CreateCheckout_RequiresDeviceID(t *testing.T) { + ctx := context.Background() + + t.Run("empty_device_id_is_rejected", func(t *testing.T) { + t.Setenv("SQUARE_TERMINAL_DEVICE_ID", "") + client := NewDevClient().(*MockClient) + res, err := client.CreateCheckout(ctx, CreateCheckoutReq{ + Amount: 5000, Currency: "GBP", IdempotencyKey: "chk-no-device", + }) + require.Error(t, err) + assert.Nil(t, res) + assert.Equal(t, "INVALID_REQUEST_ERROR", ErrorCode(err)) + assert.Equal(t, http.StatusBadRequest, ErrorStatusCode(err)) + }) + + t.Run("request_device_id_is_accepted", func(t *testing.T) { + client := NewDevClient().(*MockClient) + res, err := client.CreateCheckout(ctx, CreateCheckoutReq{ + Amount: 5000, Currency: "GBP", IdempotencyKey: "chk-req-device", DeviceID: "dvc_req", + }) + require.NoError(t, err) + assert.Equal(t, "PENDING", res.Status) + }) + + t.Run("env_device_id_fallback_is_accepted", func(t *testing.T) { + t.Setenv("SQUARE_TERMINAL_DEVICE_ID", "dvc_env") + client := NewDevClient().(*MockClient) + res, err := client.CreateCheckout(ctx, CreateCheckoutReq{ + Amount: 5000, Currency: "GBP", IdempotencyKey: "chk-env-device", + }) + require.NoError(t, err) + assert.Equal(t, "PENDING", res.Status) + }) +} + +// TestDevClient_CreateCheckout_CompletedPaymentResolvableByID locks the +// terminal-payment registration parity: a completed terminal checkout's payment +// must be resolvable via GetPayment (GET /v2/payments/{id} in prod), not just +// via GetCheckout. The mock previously never stored it in m.payments, so +// GetPayment failed in dev where prod succeeded (finding I). +func TestDevClient_CreateCheckout_CompletedPaymentResolvableByID(t *testing.T) { + client := NewDevClient().(*MockClient) + ctx := context.Background() + + checkout, err := client.CreateCheckout(ctx, CreateCheckoutReq{ + Amount: 7500, Currency: "GBP", IdempotencyKey: "chk-payment-resolvable", DeviceID: "dvc_test", + }) + require.NoError(t, err) + + var completed *PaymentResult + assert.Eventually(t, func() bool { + var getErr error + completed, getErr = client.GetCheckout(ctx, checkout.ID) + return getErr == nil && completed.Status == "COMPLETED" + }, 5*time.Second, 100*time.Millisecond, "expected checkout to complete") + + // The completed terminal payment must resolve by ID — the sweep's + // reconcile-by-id GetPayment path, which prod supports. + got, err := client.GetPayment(ctx, completed.ID) + require.NoError(t, err) + assert.Equal(t, completed.ID, got.ID) + assert.Equal(t, "COMPLETED", got.Status) +} diff --git a/backend/internal/square/square_http_client.go b/backend/internal/square/square_http_client.go index c8c269d..86a9f5c 100644 --- a/backend/internal/square/square_http_client.go +++ b/backend/internal/square/square_http_client.go @@ -996,6 +996,19 @@ func cancelCheckoutHTTPWithClient(ctx context.Context, checkoutID string, hc *ht // Conversion helpers — Square JSON → domain types. // --------------------------------------------------------------------------- +// paymentFromSquare maps Square's Payment object into the domain PaymentResult. +// Status is copied FAITHFULLY (COMPLETED/APPROVED/PENDING/FAILED/CANCELED are +// all surfaced verbatim) — the client deliberately does NOT turn a non-terminal +// status into an error. doJSON already parses 2xx bodies without dropping them, +// so a 200-with-FAILED payment reaches the caller with nil error and Status +// "FAILED". This is intentional: the payments sweep reconciles by id via +// GetPayment/ReplayPaymentByKey and SWITCHES on pr.Status +// (reconcileStalePaymentAtSquare marks a FAILED/CANCELED payment +// definitively-failed, leaves APPROVED/PENDING pending). If the client returned +// an error for a FAILED payment, the sweep would classify it as an ambiguous +// "leave pending" instead — strictly worse. Handlers must therefore check +// Status, not nil-error alone; the mock's ForcePaymentStatus toggle exists so a +// status-blind handler regression is exercisable in dev. func paymentFromSquare(sq *sqPayment) *PaymentResult { r := &PaymentResult{ ID: sq.ID, @@ -1015,8 +1028,15 @@ func paymentFromSquare(sq *sqPayment) *PaymentResult { if sq.TipMoney != nil { r.TipAmount = sq.TipMoney.Amount } + // Square reports processing_fee amounts as NEGATIVE (money withheld from the + // gross charge) or zero — never positive. PaymentResult.Fees is the POSITIVE + // magnitude the handlers store into p.fees (accounting sums a positive + // total_square_fees), so the raw negative Square amounts are negated at this + // boundary. The dev mock fabricates the same positive magnitude directly, so + // mock and real client agree on the sign convention (see + // TestProcessingFeeSign_Parity_MockAndRealClientAgree). for _, f := range sq.ProcessingFee { - r.Fees += f.AmountMoney.Amount + r.Fees += -f.AmountMoney.Amount } if sq.CardDetails != nil { cd := sq.CardDetails diff --git a/backend/internal/square/square_http_client_test.go b/backend/internal/square/square_http_client_test.go index 27dde1c..b0ec54c 100644 --- a/backend/internal/square/square_http_client_test.go +++ b/backend/internal/square/square_http_client_test.go @@ -1686,3 +1686,100 @@ func TestDeleteCustomerHTTP(t *testing.T) { } }) } + +// TestPaymentFromSquare_NegatesProcessingFees locks the processing-fee sign +// convention (finding A): Square reports processing_fee amounts as NEGATIVE on +// the wire, and paymentFromSquare must surface a POSITIVE PaymentResult.Fees +// (the magnitude handlers store as p.fees). A fee of -95 on the wire must +// become Fees == 95. +func TestPaymentFromSquare_NegatesProcessingFees(t *testing.T) { + p := &sqPayment{ + ID: "pay_fee", + Status: "COMPLETED", + TotalMoney: sqMoney{Amount: 5000, Currency: "GBP"}, + ProcessingFee: []sqFee{ + {AmountMoney: sqMoney{Amount: -95, Currency: "GBP"}, Type: "INITIAL"}, + {AmountMoney: sqMoney{Amount: -20, Currency: "GBP"}, Type: "SECONDARY"}, + }, + } + result := paymentFromSquare(p) + if result.Fees != 115 { + t.Errorf("expected Fees 115 (sum of negated Square fees), got %d", result.Fees) + } + + // A zero fee stays zero. + zero := paymentFromSquare(&sqPayment{ID: "pay_zero", Status: "COMPLETED", ProcessingFee: []sqFee{{AmountMoney: sqMoney{Amount: 0, Currency: "GBP"}}}}) + if zero.Fees != 0 { + t.Errorf("expected Fees 0 for a zero fee, got %d", zero.Fees) + } +} + +// TestPaymentFromSquare_StatusMapping verifies paymentFromSquare maps every +// documented Square payment status FAITHFULLY (finding D): APPROVED, PENDING, +// FAILED, CANCELED and COMPLETED all flow through into PaymentResult.Status. +// The client never downgrades or drops a non-terminal status. +func TestPaymentFromSquare_StatusMapping(t *testing.T) { + for _, status := range []string{"APPROVED", "PENDING", "FAILED", "CANCELED", "COMPLETED"} { + t.Run(status, func(t *testing.T) { + p := &sqPayment{ID: "pay_" + status, Status: status, TotalMoney: sqMoney{Amount: 5000, Currency: "GBP"}} + result := paymentFromSquare(p) + if result.Status != status { + t.Errorf("expected Status %q mapped verbatim, got %q", status, result.Status) + } + }) + } +} + +// TestDoJSON_200WithFailedPayment_NotDropped verifies doJSON does NOT silently +// drop a 200-with-FAILED payment (finding D): a 2xx body carrying a FAILED +// payment is parsed into a PaymentResult with Status "FAILED" and nil error — +// the client surfaces the status faithfully instead of erroring, so a +// status-blind handler (records 'completed' on nil error alone) is exposed. +func TestDoJSON_200WithFailedPayment_NotDropped(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"payment":{"id":"pay_failed_200","status":"FAILED","total_money":{"amount":5000,"currency":"GBP"},"location_id":"loc","created_at":"2026-07-31T00:00:00Z"}}`)) + })) + defer srv.Close() + + hc := &httpClient{baseURL: srv.URL, token: "t", http: srv.Client()} + res, err := createPaymentHTTPWithClient(context.Background(), CreatePaymentReq{ + Amount: 5000, Currency: "GBP", SourceID: "cnon:test-card", IdempotencyKey: "ik-failed-200", + }, hc) + if err != nil { + t.Fatalf("a 200-with-FAILED payment must NOT error (the client is status-transparent), got %v", err) + } + if res.Status != "FAILED" { + t.Errorf("expected Status FAILED surfaced faithfully, got %q", res.Status) + } + if res.ID != "pay_failed_200" { + t.Errorf("expected the failed payment returned, got %+v", res) + } +} + +// TestDoJSON_CardProcessingNotEnabled403 verifies a 403 CARD_PROCESSING_NOT_ENABLED +// response surfaces the structured Square error with StatusCode 403 (finding E) +// so the handlers agent can special-case it (errors.go, not owned here). +func TestDoJSON_CardProcessingNotEnabled403(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"errors":[{"category":"PAYMENT_METHOD_ERROR","code":"CARD_PROCESSING_NOT_ENABLED","detail":"Card processing is not enabled for this account."}]}`)) + })) + defer srv.Close() + + hc := &httpClient{baseURL: srv.URL, token: "t", http: srv.Client()} + err := hc.doJSON(context.Background(), http.MethodPost, "/v2/payments", nil, nil) + if err == nil { + t.Fatal("expected error for 403") + } + if got := ErrorStatusCode(err); got != http.StatusForbidden { + t.Errorf("expected ErrorStatusCode 403, got %d", got) + } + if got := ErrorCode(err); got != "CARD_PROCESSING_NOT_ENABLED" { + t.Errorf("expected ErrorCode CARD_PROCESSING_NOT_ENABLED, got %q", got) + } + if got := ErrorCategory(err); got != "PAYMENT_METHOD_ERROR" { + t.Errorf("expected ErrorCategory PAYMENT_METHOD_ERROR, got %q", got) + } +} diff --git a/backend/main.go b/backend/main.go index d1bc2db..719fa27 100644 --- a/backend/main.go +++ b/backend/main.go @@ -53,9 +53,59 @@ func init() { if jwtSecret == "" { log.Fatal("FATAL: JWT_SECRET_KEY environment variable not set. Application cannot start.") } + // Fail-closed: a weak, publicly-known, or placeholder JWT_SECRET_KEY must + // not start the server. Every deployment copying .env.example unchanged + // would otherwise share the SAME signing key, letting anyone forge an + // admin JWT (gift-card minting, refunds, saved-card access). + if isWeakJWTSecret(jwtSecret) { + log.Fatal("FATAL: JWT_SECRET_KEY is too weak: it must be at least 32 characters and not a known placeholder value (the current value is publicly documented). Generate a strong random key and set it, e.g. `openssl rand -hex 32`.") + } auth.InitJWT(jwtSecret) } +// minJWTSecretBytes is the minimum length enforced for JWT_SECRET_KEY. HS256 +// needs at least 32 bytes (256 bits) to be meaningful; shorter keys are +// trivially brute-forceable and every deployment sharing one is forgeable. +const minJWTSecretBytes = 32 + +// weakJWTSecretValues lists known placeholder/example values for +// JWT_SECRET_KEY that are public (documented in .env.example, READMEs, or +// attack tooling) and must never be accepted as a signing key. +var weakJWTSecretValues = []string{ + "a-very-secret-key-that-should-be-in-env", + "change-me", + "changeme", + "changethis", + "CHANGE_ME", + "secret", + "password", + "your-secret-key", + "your-secret", + "jwt-secret", + "jwt-secret-key", + "default-secret", + "my-secret", + "test-secret", + "test-secret-key", + "test-secret-key-for-testing-only", + "super-secret", +} + +// isWeakJWTSecret reports whether a JWT_SECRET_KEY is a known placeholder or +// shorter than the minimum safe length. +func isWeakJWTSecret(secret string) bool { + trimmed := strings.TrimSpace(strings.ToLower(secret)) + if len(trimmed) < minJWTSecretBytes { + return true + } + for _, weak := range weakJWTSecretValues { + if trimmed == weak { + return true + } + } + return false +} + func limitBody(limit int64) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/compose.yml b/compose.yml index cbe5524..af03397 100644 --- a/compose.yml +++ b/compose.yml @@ -41,7 +41,7 @@ services: POSTGRES_DB: ${POSTGRES_DB} POSTGRES_USER: ${POSTGRES_USER} POSTGRES_PASSWORD: ${POSTGRES_PASSWORD} - DAV_ADMIN_PASSWORD: ${DAV_ADMIN_PASSWORD:-admin} # Must be set in production + DAV_ADMIN_PASSWORD: ${DAV_ADMIN_PASSWORD:?DAV_ADMIN_PASSWORD must be set — generate a strong random value with `openssl rand -hex 32`} volumes: - ./sabredav:/var/www/dav depends_on: diff --git a/frontend/src/lib/components/booking/BookingFlow.svelte b/frontend/src/lib/components/booking/BookingFlow.svelte index 42f0926..22ed26d 100644 --- a/frontend/src/lib/components/booking/BookingFlow.svelte +++ b/frontend/src/lib/components/booking/BookingFlow.svelte @@ -450,12 +450,70 @@ toast.success('Payment successful!'); } else { const text = await response.text(); - toast.warning(text || 'Payment failed — you can pay again from your booking details.'); + // A 409 "already paid" (double-tab, or a lost-response retry that + // actually landed) must not leave the user wedged on the pay form + // with a stale deposit_paid=false — money was taken. Reconcile + // against the server's truth so the confirmation gate (depositPaid + // / confirmedBooking.deposit_paid) opens and the user reaches the + // confirmation screen. The body-text match is a belt-and-braces + // fallback for 4xx responses that still report the charge as + // already processed. + if (response.status === 409 || /already|paid|processed/.test(text.toLowerCase())) { + try { + const bookingResp = await apiFetch(`/api/bookings/${bookingId}`); + if (bookingResp.ok) { + const serverBooking = await bookingResp.json(); + // Immutable update — spread, never mutate (see audit note above). + confirmedBooking = { + ...confirmedBooking, + status: serverBooking.status ?? confirmedBooking.status, + deposit_paid: serverBooking.deposit_paid ?? confirmedBooking.deposit_paid, + deposit_amount: + serverBooking.deposit_amount ?? confirmedBooking.deposit_amount, + amount_paid: serverBooking.amount_paid ?? confirmedBooking.amount_paid, + amount_due: serverBooking.amount_due ?? confirmedBooking.amount_due, + payments: serverBooking.payments ?? confirmedBooking.payments, + total_amount: serverBooking.total_amount ?? confirmedBooking.total_amount + }; + depositPaid = confirmedBooking.deposit_paid; + toast.success('Payment successful!'); + } else { + toast.warning( + text || 'Payment failed — you can pay again from your booking details.' + ); + } + } catch { + toast.warning( + text || 'Payment failed — you can pay again from your booking details.' + ); + } + } else { + toast.warning(text || 'Payment failed — you can pay again from your booking details.'); + } + // A definitive charge failure (declined card, any 4xx) consumes the + // nonce + SCA verification token (Square nonces are single-use) — + // clear the cached pair so a retry re-tokenizes fresh instead of + // resubmitting a spent nonce for up to 240s. The idempotency key + // stays so a lost-response retry still dedups against the original + // charge (matches the TipPayment pattern). + depositNonce = ''; + depositVerificationToken = ''; + depositTokenAmount = 0; + depositTokenizedAt = 0; + depositTokenizedForSaveCard = false; } } catch { toast.error( 'An error occurred. Your booking may still be confirmed — check your appointments.' ); + // A thrown error (network / malformed response) means the nonce + SCA + // verification token are unreliable — clear them so a retry + // re-tokenizes fresh. The idempotency key stays for dedup. + depositNonce = ''; + depositVerificationToken = ''; + depositTokenAmount = 0; + depositTokenizedAt = 0; + depositTokenizedForSaveCard = false; } finally { isProcessingPayment = false; isProcessingPaymentSync = false; @@ -2441,7 +2499,13 @@ {/if}
- +
- - {#if (status === 'idle' || status === 'error') && authStore.isAuthenticated} + + {#if authStore.isAuthenticated} {#if paymentMethodsLoading}
Loading payment methods...
{:else} diff --git a/frontend/src/routes/account/+page.svelte b/frontend/src/routes/account/+page.svelte index df3653c..97e0dab 100644 --- a/frontend/src/routes/account/+page.svelte +++ b/frontend/src/routes/account/+page.svelte @@ -196,6 +196,14 @@ let giftCardCode = $state(''); let redeemingGiftCard = $state(false); let showRedeemConfirm = $state(false); + // Synchronous double-click guard for redeemGiftCard. The redeem POST is + // one-shot; a rapid second click would fire a duplicate redeem (the backend + // FOR UPDATE lock makes the second fail with "already been redeemed", + // showing an error toast right after a success). Svelte 5 reactivity is + // async, so the reactive `disabled` may not have propagated before a fast + // second click — this non-reactive flag is checked at entry before any + // await and cleared in finally. + let isRedeemingSync = false; // Buy Gift Card State let buyAmount = $state<10 | 20 | 50>(10); @@ -203,6 +211,12 @@ let buyRecipientEmail = $state(''); let buySelectedCard = $state(''); let buyingGiftCard = $state(false); + // Synchronous double-click guard for buyGiftCard. buyingGiftCard is only set + // to true AFTER tokenization, so during the tokenize await the reactive + // `disabled` on the Pay button is not yet active and a rapid second click + // would tokenize twice (minting a second nonce, wasting one). This non- + // reactive flag is checked at entry before any await and cleared in finally. + let isBuyingSync = false; let purchaseResultCode = $state(null); let buyCardSelection = $state(null); let buyCardSelectionValid = $state(false); @@ -251,40 +265,51 @@ } async function redeemGiftCard() { - if (giftCardCode.replace(/[^a-zA-Z0-9]/g, '').length !== 12) { - toast.error('Invalid gift card code format'); - return; - } - redeemingGiftCard = true; + if (isRedeemingSync) return; + isRedeemingSync = true; try { - const res = await apiFetch('/api/user/giftcards/redeem', { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ code: giftCardCode }) - }); - if (res.ok) { - const data = await res.json(); - toast.success(`Success! Redeemed ${formatCurrency(data.amount_redeemed)} to your balance.`); - giftCardCode = ''; - await fetchGiftCardBalance(); - } else { - const errText = await res.text(); - toast.error(extractErrorMessage(errText) || 'Failed to redeem gift card'); + if (giftCardCode.replace(/[^a-zA-Z0-9]/g, '').length !== 12) { + toast.error('Invalid gift card code format'); + return; + } + redeemingGiftCard = true; + try { + const res = await apiFetch('/api/user/giftcards/redeem', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ code: giftCardCode }) + }); + if (res.ok) { + const data = await res.json(); + toast.success( + `Success! Redeemed ${formatCurrency(data.amount_redeemed)} to your balance.` + ); + giftCardCode = ''; + await fetchGiftCardBalance(); + } else { + const errText = await res.text(); + toast.error(extractErrorMessage(errText) || 'Failed to redeem gift card'); + } + } catch (err) { + console.error('redeemGiftCard error:', err); + toast.error('Network error'); + } finally { + redeemingGiftCard = false; } - } catch (err) { - console.error('redeemGiftCard error:', err); - toast.error('Network error'); } finally { - redeemingGiftCard = false; + isRedeemingSync = false; } } async function buyGiftCard() { - let newCardToken: string | undefined; - let verificationToken: string | undefined; - if (buySelectedCard) { - // saved card — nothing to tokenize - } else if (buyCardSelection) { + if (isBuyingSync) return; + isBuyingSync = true; + try { + let newCardToken: string | undefined; + let verificationToken: string | undefined; + if (buySelectedCard) { + // saved card — nothing to tokenize + } else if (buyCardSelection) { // New-card mode: tokenize once per attempt, reuse the nonce + SCA // verification token on retry (tokenization is one-shot; the backend // idempotency key dedups). @@ -396,6 +421,9 @@ } finally { buyingGiftCard = false; } + } finally { + isBuyingSync = false; + } } function formatAndPreserveCursor( diff --git a/local-dev-2.sh b/local-dev-2.sh index 8e0d831..0f19a0c 100755 --- a/local-dev-2.sh +++ b/local-dev-2.sh @@ -44,8 +44,32 @@ fi # --- 3. Database Reset --- log_step "Resetting PostgreSQL..." -docker compose down -v postgres > /dev/null 2>&1 -docker compose up postgres -d > /dev/null 2>&1 +# Host port 5432 must be free: the container maps 5432:5432, so a squatter +# (e.g. a leftover manual postgres) makes the port bind fail. Under ERR_EXIT +# with hidden output that failure used to kill the script silently. +if command -v ss > /dev/null 2>&1 && ss -tln 2>/dev/null | grep -q ":5432 "; then + log_error "Port 5432 is already in use. The postgres container cannot bind it." + log_error "Something else is listening on 5432:" + ss -tlnp 2>/dev/null | grep ":5432 " + log_error "Stop the process holding 5432 (e.g. a manually-started postgres), then re-run." + exit 1 +fi +if ! docker compose down -v postgres > /tmp/opencode/pg-down.log 2>&1; then + log_error "Failed to stop old postgres container. See /tmp/opencode/pg-down.log" + exit 1 +fi +if ! docker compose up postgres -d > /tmp/opencode/pg-up.log 2>&1; then + log_error "Failed to start postgres container. See /tmp/opencode/pg-up.log" + exit 1 +fi +# ERR_EXIT cannot catch a container that is 'Up' but lost its host port mapping +# (docker can report success while the bind silently fails). Verify the map. +if ! docker port postgres 5432 > /dev/null 2>&1; then + log_error "postgres container started but is NOT mapped to host port 5432." + docker logs postgres 2>&1 | tail -5 + log_error "Free host port 5432 (see ss output above) and re-run." + exit 1 +fi log_success "PostgreSQL reset complete" # --- 3a. Wait for PostgreSQL to be ready --- diff --git a/sabredav/server.php b/sabredav/server.php index d05f7b0..78684ff 100644 --- a/sabredav/server.php +++ b/sabredav/server.php @@ -144,8 +144,17 @@ if ($stmt->fetchColumn() == 0) { "); } -// Create default user if not exists (username: admin, password from DAV_ADMIN_PASSWORD env) -$davPassword = getenv('DAV_ADMIN_PASSWORD') ?: 'admin'; +// Create default user if not exists (username: admin, password from DAV_ADMIN_PASSWORD env). +// DAV_ADMIN_PASSWORD is REQUIRED: this CardDAV/CalDAV server exposes customer +// PII (vCards), so a default or publicly-known admin credential is never +// acceptable — fail fast instead of starting with one. +$davPassword = getenv('DAV_ADMIN_PASSWORD'); +$weakDavPasswords = ['admin', 'password', 'changeme', 'change-me', 'changethis', 'secret', 'sabredav', 'test']; +if ($davPassword === false || $davPassword === '' || in_array(strtolower(trim($davPassword)), $weakDavPasswords, true)) { + error_log("FATAL: DAV_ADMIN_PASSWORD is not set or is a known weak/default value. Refusing to start: set a strong random DAV_ADMIN_PASSWORD (e.g. `openssl rand -hex 32`) in the environment and restart."); + http_response_code(500); + die("DAV_ADMIN_PASSWORD is not configured"); +} $stmt = $pdo->query("SELECT COUNT(*) FROM dav_users"); if ($stmt->fetchColumn() == 0) { $digest = md5('admin:SabreDAV:' . $davPassword);