From e9b0f0f2a7fa18db5ec60d0fb26c91bc7db81087 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Wed, 12 Aug 2026 15:59:26 +0100 Subject: [PATCH] =?UTF-8?q?fix:=20review-loop=20hardening=20=E2=80=94=20id?= =?UTF-8?q?entical-body=20replay,=202FA=20gates,=20webhook=20at-least-once?= =?UTF-8?q?,=20GDPR=20scrub?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the comprehensive payment-system review. Fixes the issues the review found in the initial integration, plus the rough edges it introduced. Money-safety: - Replay-by-key now replays the FULL original request verbatim from a stored square_request_snapshot, so a retained idempotency key returns the original payment instead of IDEMPOTENCY_KEY_REUSED (previously the row sat pending forever). IDEMPOTENCY_KEY_REUSED remains ambiguous (never proof of no charge). - Dev mock mirrors real Square for unknown-key replays: ccof: saved-card sources are charged and rescued; spent cnon: nonces surface ErrReplayKeyNotRetained. (Fixes dev/prod parity divergence.) - Webhook dedup row committed AFTER dispatch (at-least-once); FAILED till sales claw back gift-card funding; event-type strings match Square's real catalog. - Expired-gift-card cancellation refunds set creditFailed (never a phantom 'completed' refund); cancellation refunds lock all payment rows ascending. - Sweep never rescue-completes a gift-card purchase without delivering the card. - Tip no-client-key fallback is a deterministic count-based key under the booking advisory lock (retry-safe, distinct tips don't collapse). - M-cap subtracts completed refunds, clamped to [0, total]. 2FA (PSD2 SCA stand-in) for online saved-card payments: - Full feature: status/setup/verify/disable endpoints, gating helper wired into all 7 saved-card charge paths (incl. BuyGiftCard + admin saved-card), account admin-tab settings UI, frontend gating across all payment surfaces. - Enforcement is FAIL-CLOSED: on unless REQUIRE_2FA=false or an explicit mock/dev SQUARE_ENVIRONMENT; startup warning when off in a non-dev env. - Verify is brute-force hardened (5-attempt lockout, timing-safe compare); plaintext codes only logged when enforcement is off (dev). - GDPR: anonymize_user also scrubs 2FA columns and staff notes. Infra/docs: - nginx: /api/ response cache removed (cross-user disclosure); port 80 redirects to HTTPS (localhost/RFC1918 exempt, end-anchored regexes); HSTS; separate webhook rate-limit zone. - Schema: users 2FA columns; payments/till_sales square_source_id + square_request_snapshot. - Legal docs: gift-card cooling-off, international-transfers section, tips policy; Gap Backlog P3 webhooks marked done; stale counts/wording corrected. - Flaky test race fixed (t.Parallel + global mock mutation); suite 26/26 packages green, 2,142 tests, svelte-check clean. --- .env.example | 7 + README.md | 2 +- backend/handlers/payments/giftcards.go | 76 ++- backend/handlers/payments/giftcards_test.go | 123 +++++ backend/handlers/payments/handlers.go | 197 +++++++- backend/handlers/payments/payments_test.go | 157 ++++++ backend/handlers/payments/refunds.go | 21 +- backend/handlers/payments/refunds_test.go | 12 + backend/handlers/payments/service.go | 38 +- backend/handlers/payments/sweep.go | 357 ++++++++++++-- backend/handlers/payments/sweep_test.go | 173 ++++++- backend/handlers/payments/testmain_test.go | 6 + backend/handlers/payments/till.go | 48 +- backend/handlers/payments/twofa.go | 94 ++++ backend/handlers/payments/twofa_test.go | 340 +++++++++++++ backend/handlers/user/account.go | 27 ++ backend/handlers/user/gdpr_test.go | 112 ++++- backend/handlers/user/profile.go | 19 +- backend/handlers/user/twofa.go | 397 +++++++++++++++ backend/handlers/user/twofa_test.go | 452 ++++++++++++++++++ backend/handlers/webhooks/square.go | 393 +++++++++++---- .../webhooks/webhooks_dispatch_error_test.go | 83 ++++ .../handlers/webhooks/webhooks_state_test.go | 152 ++++++ backend/handlers/webhooks/webhooks_test.go | 9 +- backend/internal/square/square.go | 4 +- backend/internal/square/square_dev.go | 99 +++- backend/internal/square/square_dev_test.go | 161 +++++++ backend/internal/square/square_http_client.go | 112 +++-- .../square/square_http_client_test.go | 126 +++++ backend/internal/square/types.go | 35 +- backend/main.go | 15 + .../lib/components/admin/TillPurchases.svelte | 27 +- .../lib/components/booking/BookingFlow.svelte | 15 +- .../components/payments/CardSelection.svelte | 97 ++-- .../components/payments/PaymentModal.svelte | 35 +- .../lib/components/payments/TipPayment.svelte | 20 +- .../payments/UserPaymentModal.svelte | 11 +- frontend/src/lib/stores/auth.svelte.ts | 11 +- frontend/src/lib/types/booking.ts | 2 +- frontend/src/routes/account/+page.svelte | 173 +++++++ init-scripts/init-script.sql | 29 ++ nginx/conf.d/default.conf | 186 ++++++- obsidian/Crussell/Admin Manual.md | 2 +- .../Crussell/Future Work - Gap Backlog.md | 2 +- .../Crussell/Gift Card Terms & Conditions.md | 66 ++- obsidian/Crussell/Overview.md | 4 +- obsidian/Crussell/Privacy Policy.md | 26 +- obsidian/Crussell/Technical Manual.md | 28 +- .../Terms & Conditions - Overall App.md | 46 +- .../Testing Architecture & DB Management.md | 9 +- 50 files changed, 4223 insertions(+), 413 deletions(-) create mode 100644 backend/handlers/payments/twofa.go create mode 100644 backend/handlers/payments/twofa_test.go create mode 100644 backend/handlers/user/twofa.go create mode 100644 backend/handlers/user/twofa_test.go create mode 100644 backend/handlers/webhooks/webhooks_dispatch_error_test.go diff --git a/.env.example b/.env.example index b0de4f9..2f0b57b 100644 --- a/.env.example +++ b/.env.example @@ -41,6 +41,13 @@ SQUARE_ACCESS_TOKEN= SQUARE_LOCATION_ID= SQUARE_TERMINAL_DEVICE_ID= SQUARE_ENVIRONMENT=mock +# 2FA (PSD2 SCA stand-in) for online card payments. Enforcement is FAIL-CLOSED: +# ON unless REQUIRE_2FA=false OR SQUARE_ENVIRONMENT explicitly equals one of +# mock/dev/development/test. Empty or unknown SQUARE_ENVIRONMENT values are +# treated as production-enforced (a mistyped env var can never silently disarm +# the gate; the backend logs a startup warning in that case). Set +# REQUIRE_2FA=false only in controlled environments. +REQUIRE_2FA=true # Webhook config MUST exactly match the Square Dashboard webhook subscription # (URL + signature key). If SQUARE_WEBHOOK_NOTIFICATION_URL is left unset it # defaults to http://localhost:8080/webhooks/square, which is fail-closed (503 diff --git a/README.md b/README.md index 9d34f82..d5b9381 100644 --- a/README.md +++ b/README.md @@ -85,7 +85,7 @@ Default logins (password: `password`): ```bash cd backend && go build -o bin/backend ./main.go cd frontend && npm ci && npm run build -cd backend && go test -tags "test,dev" -count=1 -parallel 8 ./... # 1,934 tests passed (4 skipped, ~2min) +cd backend && go test -tags "test,dev" -count=1 -parallel 8 ./... # 2,133 tests passed (4 skipped, ~2min) cd backend && go test -tags "test,dev" -count=1 -race -timeout 480s ./... # race detector (all packages, ~4min) cd backend && go test -tags "test,dev" -count=10 -parallel 8 ./... # thorough verification (~2-3min) ``` diff --git a/backend/handlers/payments/giftcards.go b/backend/handlers/payments/giftcards.go index c2b31b4..a7c793a 100644 --- a/backend/handlers/payments/giftcards.go +++ b/backend/handlers/payments/giftcards.go @@ -101,12 +101,12 @@ type TransferGiftCardRequest struct { } type BuyGiftCardRequest struct { - Amount int64 `json:"amount"` - RecipientType string `json:"recipient_type"` - RecipientEmail string `json:"recipient_email,omitempty"` - CardID *string `json:"card_id,omitempty"` - NewCardToken *string `json:"new_card_token,omitempty"` - SaveCard bool `json:"save_card"` + Amount int64 `json:"amount"` + RecipientType string `json:"recipient_type"` + RecipientEmail string `json:"recipient_email,omitempty"` + CardID *string `json:"card_id,omitempty"` + NewCardToken *string `json:"new_card_token,omitempty"` + SaveCard bool `json:"save_card"` // IdempotencyKey is optional (M1): 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 @@ -991,7 +991,14 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { if req.CardID != nil && *req.CardID != "" { cardPart = *req.CardID } - req.IdempotencyKey = fmt.Sprintf("gc-%s-%d-%s-%s", userID, req.Amount, req.RecipientType, cardPart) + // 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)) @@ -1087,6 +1094,14 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { } } + // 2FA gating (C5): charging a SAVED card requires 2FA when the feature is + // enforced. New-card (nonce) charges are not gated. + if req.CardID != nil && *req.CardID != "" { + if !requireTwoFactorForCardAccess(w, r, paymentService, userID) { + return + } + } + var sourceID string var savedCardID *string var savedCardCustomerID string @@ -1120,6 +1135,12 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { // Reusing the pending record from a failed prior attempt — do not // insert a duplicate (idempotency_key is UNIQUE). Proceed straight to // the Square call, which dedups on the same key. + // Refresh square_source_id: this attempt may charge a DIFFERENT token + // than the failed attempt (one-time cnon: nonces are spent), and the + // sweep replays the charge from the stored source. + if _, srcErr := tx.Exec(ctx, `UPDATE payments SET square_source_id = $1 WHERE id = $2`, sourceID, reusePendingID); srcErr != nil { + log.Printf("Failed to update square_source_id on reused gift-card payment %s: %v", reusePendingID, srcErr) + } buyPaymentID = reusePendingID } else { fees := paymentService.CalculateFees(req.Amount, "online") @@ -1137,10 +1158,10 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { CreatedBy: &userID, } err = tx.QueryRow(ctx, ` - INSERT INTO payments (payment_type, payment_method, status, amount, square_payment_id, idempotency_key, fees, user_saved_card_id, created_by, created_at, updated_at) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) + INSERT INTO payments (payment_type, payment_method, status, amount, square_payment_id, idempotency_key, fees, user_saved_card_id, created_by, created_at, updated_at, square_source_id) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12) RETURNING id - `, record.PaymentType, record.PaymentMethod, record.Status, record.Amount, record.SquarePaymentID, record.IdempotencyKey, record.Fees, record.UserSavedCardID, record.CreatedBy, record.CreatedAt, record.UpdatedAt).Scan(&buyPaymentID) + `, record.PaymentType, record.PaymentMethod, record.Status, record.Amount, record.SquarePaymentID, record.IdempotencyKey, record.Fees, record.UserSavedCardID, record.CreatedBy, record.CreatedAt, record.UpdatedAt, sourceID).Scan(&buyPaymentID) if err != nil { log.Printf("Failed to insert payment record: %v", err) http.Error(w, "internal server error", http.StatusInternalServerError) @@ -1188,9 +1209,9 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { } paymentReq := square.CreatePaymentReq{ - Amount: req.Amount, - Currency: "GBP", - SourceID: sourceID, + Amount: req.Amount, + Currency: "GBP", + SourceID: sourceID, // CustomerID carries the saved-card row's Square customer id on ccof: // charges (save-card path); a cnon: nonce charge (one-off) needs none // (R6). @@ -1201,6 +1222,16 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { VerificationToken: verificationToken, } + // M1: store the verbatim request JSON so the sweep can replay the charge + // with an IDENTICAL body under the same key — Square compares the whole + // request on key reuse, and a reconstructed body returns + // IDEMPOTENCY_KEY_REUSED, leaving the row pending forever. + if snap, mErr := json.Marshal(paymentReq); mErr != nil { + log.Printf("Failed to marshal square_request_snapshot for gift-card payment %s: %v", buyPaymentID, mErr) + } else if _, sErr := db.Conn.Exec(ctx, `UPDATE payments SET square_request_snapshot = $1 WHERE id = $2`, string(snap), buyPaymentID); sErr != nil { + log.Printf("Failed to store square_request_snapshot for gift-card payment %s: %v", buyPaymentID, sErr) + } + paymentResult, err := SquareClient.CreatePayment(ctx, paymentReq) if err != nil { log.Printf("Failed to process gift card purchase payment: %v", err) @@ -1325,8 +1356,10 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { } // TODO: send gift card code via email to recipient once SMTP is wired. // Do NOT log the spendable gift-card code — it is a credential (12-digit - // code anyone can redeem). Log only the value and recipient for audit. - log.Printf("Gift card purchased for friend — value: £%.2f, intended for: %s (code stored in DB, not logged)", amountPounds, recipient) + // code anyone can redeem). Log only the value and redacted recipient for + // audit — the full email is third-party PII with no retention coverage + // and the raw free-text field is a log-injection vector. + log.Printf("Gift card purchased for friend — value: £%.2f, intended for: %s (code stored in DB, not logged)", amountPounds, redactEmail(recipient)) _, err = issueTx.Exec(ctx, ` INSERT INTO gift_card_transactions (gift_card_id, transaction_type, amount, reference_type, reference_id, user_id, notes) @@ -1357,6 +1390,19 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { // --- Helpers --- +// redactEmail masks an email for logs (PII — third-party addresses are not +// covered by retention/anonymization, and the raw free-text field is a +// log-injection vector). Mirrors the dev mock's redaction: first two chars of +// the local part plus the domain, e.g. "ja***@example.com"; malformed +// addresses fall back to "[redacted]". The full email stays in the DB row. +func redactEmail(email string) string { + at := strings.Index(email, "@") + if at < 2 || at+1 >= len(email) { + return "[redacted]" + } + return email[:2] + "***@" + email[at+1:] +} + type ExpiredBalance struct { ID string `json:"id"` AccountID *string `json:"account_id,omitempty"` diff --git a/backend/handlers/payments/giftcards_test.go b/backend/handlers/payments/giftcards_test.go index 7d2eef7..834c225 100644 --- a/backend/handlers/payments/giftcards_test.go +++ b/backend/handlers/payments/giftcards_test.go @@ -2358,3 +2358,126 @@ func TestTransferGiftCard_JSONDecodeError(t *testing.T) { t.Errorf("expected status 400, got %d. body: %s", w.Code, w.Body.String()) } } + +// TestBuyGiftCard_NoClientKey_TwoPurchases_DoNotCollapse verifies the fallback +// idempotency-key fix: two identical purchases WITHOUT a client-supplied key +// must be treated as DISTINCT purchases — each issues its own gift card. The +// old deterministic fallback key (user+amount+recipient+card) collapsed the +// second purchase into the first, silently returning the first card's code. +func TestBuyGiftCard_NoClientKey_TwoPurchases_DoNotCollapse(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + + token := jwt.GenerateTestToken(userID, "verified_email") + + buy := func() (int, string) { + t.Helper() + reqBody, _ := json.Marshal(map[string]interface{}{ + "amount": 2000, + "recipient_type": "self", + "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.(pgx.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() + } + + if code, body := buy(); code != http.StatusCreated { + t.Fatalf("first purchase: expected 201, got %d: %s", code, body) + } + if code, body := buy(); code != http.StatusCreated { + t.Fatalf("second purchase: expected 201, got %d: %s", code, body) + } + + // Two DISTINCT fallback keys — never one shared dedup key. + var distinctKeys int + err = tx.QueryRow(ctx, "SELECT COUNT(DISTINCT idempotency_key) FROM payments WHERE created_by = $1", userID).Scan(&distinctKeys) + if err != nil { + t.Fatalf("failed to query distinct keys: %v", err) + } + if distinctKeys != 2 { + t.Errorf("expected 2 DISTINCT fallback idempotency keys for two no-key purchases, got %d", distinctKeys) + } + + // Two payment records and two issued gift cards — the second purchase must + // NOT have been deduped into the first. + var payCount int + err = tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE created_by = $1", userID).Scan(&payCount) + if err != nil { + t.Fatalf("failed to query payments: %v", err) + } + if payCount != 2 { + t.Errorf("expected 2 payment records for 2 distinct no-key purchases, got %d", payCount) + } + + var cardCount int + err = tx.QueryRow(ctx, "SELECT COUNT(*) FROM gift_cards WHERE created_by = $1", userID).Scan(&cardCount) + if err != nil { + t.Fatalf("failed to query gift cards: %v", err) + } + if cardCount != 2 { + t.Errorf("expected 2 gift cards issued for 2 distinct purchases, got %d", cardCount) + } +} + +// TestBuyGiftCard_PendingRow_StoresSquareSourceID verifies that BuyGiftCard's +// pending payment row stores the EXACT source_id sent to Square's CreatePayment +// (payments.square_source_id), so the sweep can replay the charge with an +// identical request body under the same idempotency key. +func TestBuyGiftCard_PendingRow_StoresSquareSourceID(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + + token := jwt.GenerateTestToken(userID, "verified_email") + + idempotencyKey := "buy-gc-source-id-test" + reqBody, _ := json.Marshal(map[string]interface{}{ + "amount": 2000, + "recipient_type": "self", + "new_card_token": "cnon:card-nonce-ok", + "idempotency_key": idempotencyKey, + }) + 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.(pgx.Tx))) + + w := httptest.NewRecorder() + r := chi.NewRouter() + r.Use(mw.RequireAuth) + r.Post("/api/user/giftcards/buy", BuyGiftCard) + r.ServeHTTP(w, req) + + if w.Code != http.StatusCreated { + t.Fatalf("expected 201, got %d: %s", w.Code, w.Body.String()) + } + + // The row must carry the exact one-off nonce sent as CreatePayment's + // SourceID (new-card path with save_card=false). + var sourceID string + err = tx.QueryRow(ctx, "SELECT COALESCE(square_source_id, '') FROM payments WHERE idempotency_key = $1", idempotencyKey).Scan(&sourceID) + if err != nil { + t.Fatalf("failed to query square_source_id: %v", err) + } + if sourceID != "cnon:card-nonce-ok" { + t.Errorf("expected square_source_id %q (the exact CreatePayment SourceID), got %q", "cnon:card-nonce-ok", sourceID) + } +} diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index 18e3545..c5327ad 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -560,6 +560,13 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal server error", http.StatusInternalServerError) return } + // 2FA gating (C5): charging the customer's SAVED card requires 2FA when + // the feature is enforced. Gate on the card's owner — the booking's + // user, not the admin. New-card/terminal paths are not gated. + if bookingUserID.Valid && !requireTwoFactorForCardAccess(w, r, service, bookingUserID.String) { + return + } + // Resolve the saved-card Square source for the booking's user (the // card's owner, not the admin) — shared new-card-vs-saved-card // resolution, see resolveChargeSource for the R6 rationale. @@ -657,20 +664,27 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { Amount: float64(amount) / 100.0, IdempotencyKey: &scKey, UserSavedCardID: req.UserSavedCardID, + SquareSourceID: &sourceID, CreatedAt: clock.Now(), UpdatedAt: clock.Now(), CreatedBy: &adminID, } if err := tx.QueryRow(r.Context(), ` - INSERT INTO payments (booking_id, payment_type, payment_method, status, amount, idempotency_key, user_saved_card_id, created_by, created_at, updated_at) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) + INSERT INTO payments (booking_id, payment_type, payment_method, status, amount, idempotency_key, user_saved_card_id, square_source_id, created_by, created_at, updated_at) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) RETURNING id - `, record.BookingID, record.PaymentType, record.PaymentMethod, record.Status, record.Amount, record.IdempotencyKey, record.UserSavedCardID, record.CreatedBy, record.CreatedAt, record.UpdatedAt).Scan(&paymentID); err != nil { + `, record.BookingID, record.PaymentType, record.PaymentMethod, record.Status, record.Amount, record.IdempotencyKey, record.UserSavedCardID, record.SquareSourceID, record.CreatedBy, record.CreatedAt, record.UpdatedAt).Scan(&paymentID); err != nil { log.Printf("Failed to insert pending saved-card payment: %v", err) _ = tx.Rollback(r.Context()) http.Error(w, "internal server error", http.StatusInternalServerError) return } + } else { + // Refresh square_source_id on a reused pending row — the sweep + // replays the charge from the stored source. + if _, srcErr := tx.Exec(r.Context(), `UPDATE payments SET square_source_id = $1 WHERE id = $2`, sourceID, paymentID); srcErr != nil { + log.Printf("Failed to update square_source_id on reused saved-card payment %s: %v", paymentID, srcErr) + } } if err := tx.Commit(r.Context()); err != nil { log.Printf("Failed to commit pending saved-card payment: %v", err) @@ -683,7 +697,7 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { _ = db.Conn.QueryRow(r.Context(), `SELECT email FROM users WHERE id = $1`, bookingUserID.String).Scan(&buyerEmail) } - paymentResult, err := SquareClient.CreatePayment(r.Context(), square.CreatePaymentReq{ + paymentReq := square.CreatePaymentReq{ Amount: amount, Currency: "GBP", SourceID: sourceID, @@ -692,7 +706,18 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { ReferenceID: bookingID, Note: req.PaymentType, BuyerEmail: buyerEmail, - }) + } + // M1: store the verbatim request JSON so the sweep can replay the charge + // with an IDENTICAL body under the same key — Square compares the whole + // request on key reuse, and a reconstructed body returns + // IDEMPOTENCY_KEY_REUSED, leaving the row pending forever. + if snap, mErr := json.Marshal(paymentReq); mErr != nil { + log.Printf("Failed to marshal square_request_snapshot for saved-card payment %s: %v", paymentID, mErr) + } else if _, sErr := db.Conn.Exec(r.Context(), `UPDATE payments SET square_request_snapshot = $1 WHERE id = $2`, string(snap), paymentID); sErr != nil { + log.Printf("Failed to store square_request_snapshot for saved-card payment %s: %v", paymentID, sErr) + } + + paymentResult, err := SquareClient.CreatePayment(r.Context(), paymentReq) if err != nil { log.Printf("Failed to process saved-card payment: %v", err) http.Error(w, "Payment failed", chargeFailureStatus(err)) @@ -927,20 +952,52 @@ func activeTerminalCheckoutID(ctx context.Context, bookingID string) string { } // A provisional (pre-Square) row carries a synthetic "tmp-" checkout_id (or - // an empty one for legacy rows) — no checkout was ever created at Square - // for it, so it is PROVABLY not live (R3). A hard crash between the - // terminal_checkouts insert and the Square CreateCheckout call is the only - // way one exists. Mark it failed so a fresh checkout can be created instead - // of wedging the booking behind a non-existent Square checkout. + // an empty one for legacy rows). 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. + // Resolving it to failed unconditionally would let a lost-response retry + // create a SECOND live checkout (C2) while C1 can still complete at the + // terminal into an untracked charge. Query Square first to disambiguate. if checkoutID == "" || strings.HasPrefix(checkoutID, "tmp-") { - log.Printf("Provisional (pre-Square) terminal checkout row %q for booking %s resolved as failed — no live checkout at Square", checkoutID, bookingID) if checkoutID != "" { - if _, upErr := db.Conn.Exec(ctx, ` - UPDATE terminal_checkouts SET status = 'failed', updated_at = NOW() WHERE checkout_id = $1 - `, checkoutID); upErr != nil { - log.Printf("Failed to mark provisional terminal checkout %s failed: %v", checkoutID, upErr) + 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) + } + return "" + 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) + return checkoutID + case isTerminalCheckoutError(err): + // NOT_FOUND (no checkout was ever created — the crash happened + // before the Square call) or CANCELED — safe to resolve failed + // and create a fresh checkout. + log.Printf("Provisional (pre-Square) terminal checkout row %q for booking %s resolved as failed — no live checkout at Square", checkoutID, bookingID) + if _, upErr := db.Conn.Exec(ctx, ` + UPDATE terminal_checkouts SET status = 'failed', updated_at = NOW() WHERE checkout_id = $1 + `, checkoutID); upErr != nil { + log.Printf("Failed to mark provisional terminal checkout %s failed: %v", checkoutID, upErr) + } + return "" + default: + // Ambiguous error — the checkout's money state at Square is + // unknown. Keep it in flight rather than spawning a second live + // checkout. + return checkoutID } } + log.Printf("Legacy empty checkout_id row for booking %s resolved as failed — no live checkout at Square", bookingID) return "" } @@ -1329,6 +1386,13 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { return } + service := NewPaymentService() + + // 2FA gating (C5): persisting a card requires 2FA when the feature is enforced. + if req.SaveCard && !requireTwoFactorForCardAccess(w, r, service, userID) { + return + } + // Resolve buyer email for Square receipt delivery (failure is non-fatal). var bookingBuyerEmail string if err := db.Conn.QueryRow(r.Context(), `SELECT email FROM users WHERE id = $1`, userID).Scan(&bookingBuyerEmail); err != nil { @@ -1376,8 +1440,6 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { } } - service := NewPaymentService() - if req.PaymentType == "partial" { remainingCents, err := service.GetBookingRemainingBalanceCents(r.Context(), bookingID) if err != nil { @@ -1628,6 +1690,13 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { var sourceID string var savedCardID *string var savedCardCustomerID string + // 2FA gating (C5): charging a SAVED card requires 2FA when the feature is + // enforced. New-card (nonce) charges are not gated. + if req.CardID != nil && *req.CardID != "" { + if !requireTwoFactorForCardAccess(w, r, service, userID) { + return + } + } // Resolve the new-card-vs-saved-card Square source (shared with // CreateTipPayment, BuyGiftCard, and the saved-card branch of // CreateTerminalPayment — see resolveChargeSource for the R6 rationale). @@ -1653,6 +1722,7 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { IdempotencyKey: &req.IdempotencyKey, Fees: float64(fees) / 100.0, UserSavedCardID: savedCardID, + SquareSourceID: &sourceID, CreatedAt: clock.Now(), UpdatedAt: clock.Now(), CreatedBy: &userID, @@ -1666,6 +1736,14 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // Apply VAT to the pending record inside the same transaction — same // pattern as CreateTipPayment. ApplyVATToBookingPayment(r.Context(), tx, paymentID) + } else { + // Refresh square_source_id on a reused pending row: this attempt may + // charge a different token than the failed attempt (one-time cnon: + // nonces are spent), and the sweep replays the charge from the stored + // source. + if _, srcErr := tx.Exec(r.Context(), `UPDATE payments SET square_source_id = $1 WHERE id = $2`, sourceID, paymentID); srcErr != nil { + log.Printf("Failed to update square_source_id on reused payment %s: %v", paymentID, srcErr) + } } // Always commit the transaction. In the reuse path no rows were written, @@ -1700,6 +1778,16 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { VerificationToken: verificationToken, } + // M1: store the verbatim request JSON so the sweep can replay the charge + // with an IDENTICAL body under the same key — Square compares the whole + // request on key reuse, and a reconstructed body returns + // IDEMPOTENCY_KEY_REUSED, leaving the row pending forever. + if snap, mErr := json.Marshal(paymentReq); mErr != nil { + log.Printf("Failed to marshal square_request_snapshot for payment %s: %v", paymentID, mErr) + } else if _, sErr := db.Conn.Exec(r.Context(), `UPDATE payments SET square_request_snapshot = $1 WHERE id = $2`, string(snap), paymentID); sErr != nil { + log.Printf("Failed to store square_request_snapshot for payment %s: %v", paymentID, sErr) + } + paymentResult, err := SquareClient.CreatePayment(r.Context(), paymentReq) if err != nil { log.Printf("Failed to create payment: %v", err) @@ -3260,6 +3348,13 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { return } + service := NewPaymentService() + + // 2FA gating (C5): persisting a card requires 2FA when the feature is enforced. + if req.SaveCard && !requireTwoFactorForCardAccess(w, r, service, userID) { + return + } + if err := ValidateAmount(req.Amount); err != nil { log.Printf("Failed to process request: %v", err) http.Error(w, "Invalid request", http.StatusBadRequest) @@ -3278,8 +3373,6 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { return } - service := NewPaymentService() - bookingUserID, err := service.GetBookingUserID(r.Context(), bookingID) if err != nil { if errors.Is(err, pgx.ErrNoRows) { @@ -3344,18 +3437,25 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { // Idempotency key: prefer the client-supplied UUID (one per attempt, so // two legitimate identical tips on the same booking don't collapse into - // one). Fall back to a unique key when absent — must NOT be derived from - // request fields alone (bookingID + amount would dedupe distinct tips). + // one). When the client sends NO key, a DETERMINISTIC fallback is derived + // inside the transaction below (the count query must see the committed + // rows) — never a random key: a random fallback meant a lost-response + // no-key retry minted a fresh key, a fresh pending row, and a SECOND + // Square charge (H2). idempotencyKey := req.IdempotencyKey - if idempotencyKey == "" { - idempotencyKey = uniqueChargeKey("tip-") - } // Resolve the card source ID — same pattern as CreateBookingPayment (see // resolveChargeSource for the R6 rationale). var sourceID string var savedCardID *string var savedCardCustomerID string + // 2FA gating (C5): charging a SAVED card requires 2FA when the feature is + // enforced. New-card (nonce) charges are not gated. + if req.CardID != nil && *req.CardID != "" { + if !requireTwoFactorForCardAccess(w, r, service, userID) { + return + } + } sourceID, savedCardID, savedCardCustomerID, sourceOK := resolveChargeSource(r.Context(), w, service, userID, req.NewCardToken, req.CardID, req.SaveCard, "Card not found") if !sourceOK { return @@ -3386,6 +3486,36 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { } }() + // No-client-key fallback: derive a deterministic key INSIDE the tx so the + // count query races no other tip attempt (the tip advisory lock serializes + // per booking). Money-safety: n counts COMPLETED tips only, so a + // lost-response retry of a charge whose pending row exists derives the SAME + // n → the same key → the dedup lookup below reuses the pending row instead + // of minting a second Square charge, while two genuinely distinct identical + // tips get n=1, n=2 and never collapse onto one key. + if idempotencyKey == "" { + var completedTips int + if err := tx.QueryRow(r.Context(), ` + SELECT COUNT(*) FROM payments + WHERE booking_id = $1 AND payment_type = 'tip' AND status = 'completed' + `, bookingID).Scan(&completedTips); err != nil { + log.Printf("Failed to count completed tips for booking %s: %v", bookingID, err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + cardPart := "new" + if req.CardID != nil && *req.CardID != "" { + cardPart = *req.CardID + } + idempotencyKey = fmt.Sprintf("tip-%s-%d-%s-%d", bookingID, req.Amount, cardPart, completedTips+1) + if len(idempotencyKey) > 45 { + // Hash long keys to fit Square's 45-char limit — the hash stays + // deterministic, so a retry still derives the same key. + hash := sha256.Sum256([]byte(idempotencyKey)) + idempotencyKey = fmt.Sprintf("tip-%x", hash[:16]) + } + } + // Check idempotency inside the transaction. // Only short-circuit when the existing record is 'completed'. A 'pending' // record means the previous Square call failed — returning it as 200 would @@ -3458,6 +3588,7 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { IdempotencyKey: &idempotencyKey, Fees: 0, UserSavedCardID: savedCardID, + SquareSourceID: &sourceID, CreatedAt: clock.Now(), UpdatedAt: clock.Now(), CreatedBy: &userID, @@ -3470,6 +3601,14 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { return } ApplyVATToBookingPayment(r.Context(), tx, paymentID) + } else { + // Refresh square_source_id on a reused pending row: this attempt may + // charge a different token than the failed attempt (one-time cnon: + // nonces are spent), and the sweep replays the charge from the stored + // source. + if _, srcErr := tx.Exec(r.Context(), `UPDATE payments SET square_source_id = $1 WHERE id = $2`, sourceID, paymentID); srcErr != nil { + log.Printf("Failed to update square_source_id on reused tip payment %s: %v", paymentID, srcErr) + } } // Always commit the transaction. In the reuse path no rows were written, @@ -3510,6 +3649,16 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { VerificationToken: verificationToken, } + // M1: store the verbatim request JSON so the sweep can replay the charge + // with an IDENTICAL body under the same key — Square compares the whole + // request on key reuse, and a reconstructed body returns + // IDEMPOTENCY_KEY_REUSED, leaving the row pending forever. + if snap, mErr := json.Marshal(paymentReq); mErr != nil { + log.Printf("Failed to marshal square_request_snapshot for tip payment %s: %v", paymentID, mErr) + } else if _, sErr := db.Conn.Exec(r.Context(), `UPDATE payments SET square_request_snapshot = $1 WHERE id = $2`, string(snap), paymentID); sErr != nil { + log.Printf("Failed to store square_request_snapshot for tip payment %s: %v", paymentID, sErr) + } + paymentResult, err := SquareClient.CreatePayment(r.Context(), paymentReq) if err != nil { log.Printf("Failed to create tip payment: %v", err) diff --git a/backend/handlers/payments/payments_test.go b/backend/handlers/payments/payments_test.go index dc49849..9ea6b0f 100644 --- a/backend/handlers/payments/payments_test.go +++ b/backend/handlers/payments/payments_test.go @@ -2809,6 +2809,100 @@ func TestTipPayment_TransactionFailure_SkipsSquare(t *testing.T) { } } +// TestTipPayment_NoClientKey_LostResponseRetryReusesPendingRow verifies the H2 +// fix: a no-client-key tip whose Square call failed ambiguously leaves a +// pending row; a retry (still no client key) must derive the SAME deterministic +// key from the booking+amount+card+sequence and reuse that pending row — never +// mint a fresh random key, a fresh pending row, and a second Square charge. +func TestTipPayment_NoClientKey_LostResponseRetryReusesPendingRow(t *testing.T) { + // NOT t.Parallel: it swaps the package-global SquareClient (a ShouldFail + // mock) mid-test, and a concurrent parallel charge test would observe the + // swapped client and fail with a spurious 503. + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + _, err := fixtures.CreateTestPayment(tx, bookingID, 5000.00, "online_square", "full", "completed") + require.NoError(t, err) + + origClient := SquareClient + mc := square.NewDevClient().(*square.MockClient) + mc.ShouldFail = true + SquareClient = mc + defer func() { SquareClient = origClient }() + + cardToken := "cnon:no-key-tip-retry" + req := CreateTipPaymentRequest{ + Amount: 1000, + NewCardToken: &cardToken, + } + + handler := CreateTipPayment + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) + require.Equal(t, http.StatusServiceUnavailable, w.Code, "first attempt must fail ambiguously: %s", w.Body.String()) + + // The pending row must exist with a deterministic key. + var pendingKey string + var pendingCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT idempotency_key, COUNT(*) OVER() FROM payments WHERE booking_id = $1 AND payment_type = 'tip'`, bookingID).Scan(&pendingKey, &pendingCount)) + require.Equal(t, 1, pendingCount) + require.NotEmpty(t, pendingKey) + require.Contains(t, pendingKey, bookingID, "the no-client-key fallback must derive a deterministic key from the booking, not a random one") + + // Retry with Square healthy: no client key, same amount/card. + mc.ShouldFail = false + w = makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "retry must succeed: %s", w.Body.String()) + + // Exactly ONE tip payment row, completed, with the SAME key. + var count int + var status string + var storedKey string + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*), MAX(status), MAX(idempotency_key) FROM payments WHERE booking_id = $1 AND payment_type = 'tip'`, bookingID).Scan(&count, &status, &storedKey)) + require.Equal(t, 1, count, "a no-client-key retry must reuse the pending row, not insert a duplicate") + require.Equal(t, "completed", status) + require.Equal(t, pendingKey, storedKey, "the retry must derive the same deterministic key") +} + +// TestTipPayment_NoClientKey_DistinctIdenticalTipsDoNotCollapse verifies the +// H2 fix's other half: two genuinely distinct identical no-client-key tips on +// the same booking get n=1 and n=2, so they never collapse onto one +// idempotency key (which would silently swallow the second tip). +func TestTipPayment_NoClientKey_DistinctIdenticalTipsDoNotCollapse(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + _, err := fixtures.CreateTestPayment(tx, bookingID, 5000.00, "online_square", "full", "completed") + require.NoError(t, err) + + handler := CreateTipPayment + cardToken := "cnon:no-key-tip-dup" + req := CreateTipPaymentRequest{ + Amount: 1000, + NewCardToken: &cardToken, + } + for i := 0; i < 2; i++ { + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "tip %d: %s", i, w.Body.String()) + } + + var keys []string + rows, err := tx.Query(ctx, `SELECT idempotency_key FROM payments WHERE booking_id = $1 AND payment_type = 'tip' ORDER BY created_at`, bookingID) + require.NoError(t, err) + for rows.Next() { + var k string + require.NoError(t, rows.Scan(&k)) + keys = append(keys, k) + } + rows.Close() + require.Equal(t, 2, len(keys), "two distinct identical tips must both be recorded") + require.NotEqual(t, keys[0], keys[1], "distinct identical tips must not collapse onto one idempotency key") +} + func TestGetUserPaymentMethods_NoCards(t *testing.T) { t.Parallel() ctx, tx := testutils.SetupTestTx(t) @@ -2970,6 +3064,69 @@ func TestGetBookingRemainingBalanceCents(t *testing.T) { } } +// TestGetBookingRemainingBalanceCents_RefundsReopenCapacity verifies the M-cap +// is refund-aware: a completed refund returns money, so it re-opens booking +// capacity — remaining = total - paid + refunded — while the cap never exceeds +// the booking total. +func TestGetBookingRemainingBalanceCents_RefundsReopenCapacity(t *testing.T) { + t.Parallel() + 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.CreateTestBooking(tx, userID, serviceID) + require.NoError(t, err) + + var bookingTotal int64 + require.NoError(t, tx.QueryRow(ctx, `SELECT ROUND(total_amount * 100)::bigint FROM bookings WHERE id = $1`, bookingID).Scan(&bookingTotal)) + + service := NewPaymentService() + + // Pay the full booking amount. + _, err = service.CreatePaymentRecord(ctx, PaymentRecord{ + BookingID: bookingID, + PaymentType: "full", + PaymentMethod: "online_square", + Status: "completed", + Amount: float64(bookingTotal) / 100.0, + }, nil) + require.NoError(t, err) + + remaining, err := service.GetBookingRemainingBalanceCents(ctx, bookingID) + require.NoError(t, err) + require.Equal(t, int64(0), remaining, "a fully-paid booking must have 0 remaining") + + // Refund half the booking value — capacity must re-open by that amount. + var payRowID string + require.NoError(t, tx.QueryRow(ctx, `SELECT id FROM payments WHERE booking_id = $1 AND payment_type = 'full' ORDER BY created_at DESC LIMIT 1`, bookingID).Scan(&payRowID)) + + refundAmount := bookingTotal / 2 + _, err = tx.Exec(ctx, ` + INSERT INTO refunds (payment_id, booking_id, amount, status, reason, origin) + VALUES ($1, $2, $3, 'completed', 'test refund', 'manual') + `, payRowID, bookingID, float64(refundAmount)/100.0) + require.NoError(t, err) + + remaining, err = service.GetBookingRemainingBalanceCents(ctx, bookingID) + require.NoError(t, err) + require.Equal(t, refundAmount, remaining, "a completed refund must re-open the remaining balance by its amount") + + // The cap must never exceed the booking total even if refunds exceed payments. + _, err = tx.Exec(ctx, ` + INSERT INTO refunds (payment_id, booking_id, amount, status, reason, origin) + VALUES ($1, $2, $3, 'completed', 'over-refund test', 'manual') + `, payRowID, bookingID, bookingTotal) + require.NoError(t, err) + + remaining, err = service.GetBookingRemainingBalanceCents(ctx, bookingID) + require.NoError(t, err) + require.Equal(t, bookingTotal, remaining, "the remaining balance must never exceed the booking total") +} + func TestCreatePaymentMethod_HappyPath(t *testing.T) { t.Parallel() ctx, tx := testutils.SetupTestTx(t) diff --git a/backend/handlers/payments/refunds.go b/backend/handlers/payments/refunds.go index ac4abef..7f98991 100644 --- a/backend/handlers/payments/refunds.go +++ b/backend/handlers/payments/refunds.go @@ -103,17 +103,20 @@ func CalculateRefundForCancellation( // lockCancellationPayments serializes a cancellation refund against the manual // RefundPayment handler and the sweep. Both hold // `pg_advisory_lock(hashtext('crussell:refund:' || payment_id))` (session-level) -// on the card payment ids they touch; a cancellation that computes residuals +// on the payment ids they touch; a cancellation that computes residuals // without the same locks can over-refund against a manual refund in flight (the // manual guard read precedes the cancellation's commit). Locks are acquired in -// ascending payment_id order (matching processChargeGroup) to avoid deadlocks, -// and only for card methods the manual handler can touch. +// ascending payment_id order (matching processChargeGroup) to avoid deadlocks. +// EVERY payment row the cancellation may refund is locked — giftcard and cash +// rows included, not just card methods — so two concurrent refunds of the same +// giftcard/cash payment serialize on the residual computation and can never +// double-credit (H3). func lockCancellationPayments(ctx context.Context, tx pgx.Tx, payments []paymentRow) error { var ids []string for _, p := range payments { - if p.PaymentMethod == "online_square" || p.PaymentMethod == "in_person_card" { - ids = append(ids, p.ID) - } + // discount/on_the_house rows are already excluded by the caller's + // query, so every remaining row is one this loop may refund. + ids = append(ids, p.ID) } if len(ids) == 0 { return nil @@ -338,6 +341,12 @@ func ProcessCancellationRefundTx( log.Printf("Failed to check gift card %s expiry: %v — proceeding with refund", *giftCardID, err) } else if expired { log.Printf("Gift card %s has expired — money retained by salon, no refund due for booking %s", *giftCardID, bookingID) + // Money-safety (C4): the UPDATE gift_cards credit above is + // SKIPPED for expired cards (money retained by the salon), so + // creditFailed must be set here — otherwise the shared tail + // below inserts the refund record as 'completed' claiming money + // was returned when it never moved. + creditFailed = true break } // Refunding to the card is a "use" per the rolling-expiry terms — diff --git a/backend/handlers/payments/refunds_test.go b/backend/handlers/payments/refunds_test.go index 68fba33..25851bc 100644 --- a/backend/handlers/payments/refunds_test.go +++ b/backend/handlers/payments/refunds_test.go @@ -531,6 +531,18 @@ func TestProcessCancellationRefund_ExpiredGiftCard_Retained(t *testing.T) { if txCount != 0 { t.Errorf("expected NO refund-to-expired-card transaction, got %d", txCount) } + + // C4 money-safety: the refund record must be 'failed' — never 'completed'. + // The old bug fell through the switch and recorded a completed refund even + // though the UPDATE gift_cards credit was skipped (no money moved). + var refundStatus string + if err := tx.QueryRow(ctx, + "SELECT status FROM refunds WHERE booking_id = $1", bookingID).Scan(&refundStatus); err != nil { + t.Fatalf("failed to query refund status: %v", err) + } + if refundStatus != "failed" { + t.Errorf("expected expired-card refund record status 'failed' (no money credited), got %q", refundStatus) + } } func TestProcessCancellationRefund_CashCreditsUserBalance(t *testing.T) { diff --git a/backend/handlers/payments/service.go b/backend/handlers/payments/service.go index 052906f..522cc0f 100644 --- a/backend/handlers/payments/service.go +++ b/backend/handlers/payments/service.go @@ -57,12 +57,17 @@ type PaymentRecord struct { NetAmount *float64 UserSavedCardID *string SquarePaymentID *string - IdempotencyKey *string - Fees float64 - CreatedAt time.Time - UpdatedAt time.Time - CreatedBy *string - GiftCardID *string + // SquareSourceID is the exact source_id (cnon: nonce or ccof: card id) sent + // in the CreatePayment call, stored on the pending row so the sweep can + // replay the charge with an IDENTICAL request body under the same + // idempotency key. + SquareSourceID *string + IdempotencyKey *string + Fees float64 + CreatedAt time.Time + UpdatedAt time.Time + CreatedBy *string + GiftCardID *string } type RefundRecord struct { @@ -129,8 +134,8 @@ func (s *PaymentService) insertPaymentRecord(ctx context.Context, record Payment booking_id, payment_type, payment_method, vendor_code, invoice_number, status, amount, is_vat_applicable, vat_rate, vat_amount, net_amount, user_saved_card_id, square_payment_id, idempotency_key, fees, created_at, updated_at, created_by, - gift_card_id - ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19) + gift_card_id, square_source_id + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20) RETURNING id `, bookingID, @@ -152,6 +157,7 @@ func (s *PaymentService) insertPaymentRecord(ctx context.Context, record Payment record.UpdatedAt, record.CreatedBy, giftCardID, + record.SquareSourceID, ).Scan(&id) if err != nil { @@ -509,9 +515,21 @@ func (s *PaymentService) GetBookingRemainingBalanceCents(ctx context.Context, bo -- A tip is money paid beyond the booking total — it does not -- reduce the balance owed, so it must not count as "paid". AND payment_type <> 'tip' + ), + refunded_total AS ( + SELECT COALESCE(SUM(r.amount), 0) AS refunded_pounds + FROM refunds r + JOIN payments p ON r.payment_id = p.id + WHERE p.booking_id = $1 AND r.status = 'completed' ) - SELECT GREATEST(0, ROUND((bt.total_pounds - pt.paid_pounds) * 100))::bigint - FROM booking_total bt, paid_total pt + -- Money-safety (M-cap): refunds return money, so they re-open booking + -- capacity — remaining = total - paid + refunded. LEAST clamps the cap + -- at the booking total so the M-cap can never allow a charge beyond the + -- booking's full value even in the pathological case where completed + -- refunds exceed payments, and GREATEST floors at 0 so a fully-paid + -- (or over-paid) booking can never be charged again. + SELECT GREATEST(0, ROUND(LEAST(bt.total_pounds - pt.paid_pounds + rt.refunded_pounds, bt.total_pounds) * 100))::bigint + FROM booking_total bt, paid_total pt, refunded_total rt `, bookingID).Scan(&remainingCents) if err != nil { return 0, err diff --git a/backend/handlers/payments/sweep.go b/backend/handlers/payments/sweep.go index cfecd90..853cc89 100644 --- a/backend/handlers/payments/sweep.go +++ b/backend/handlers/payments/sweep.go @@ -1,12 +1,15 @@ package payments import ( + "bytes" "context" "database/sql" + "encoding/json" "errors" "fmt" "log" "math" + "strconv" "strings" "time" @@ -140,14 +143,40 @@ type staleRow struct { // the lost-response case: the sweep replays the key at Square to learn the // true charge outcome before declaring failure. IdempotencyKey string + // SquareSourceID is the exact source_id sent in the original CreatePayment + // call, stored on the row so the replay-by-key can rebuild an IDENTICAL + // request body (same key + source + amount). Replaying a different body + // would return IDEMPOTENCY_KEY_REUSED, which proves nothing about whether + // the charge landed. + SquareSourceID string + // SquareRequestSnapshot is the verbatim original CreatePayment request JSON + // stored on the row at charge time (square_request_snapshot) — the FULL + // body the replay-by-key must repeat (source, key, amount, customer_id, + // reference_id, note, buyer_email_address, verification_token, ...). + // Square's idempotency dedup compares the whole request, so a replay built + // from partial row data returns IDEMPOTENCY_KEY_REUSED for a RETAINED key + // and the row stays pending forever (safe but never auto-rescued). Nil for + // legacy rows — the sweep then rebuilds the minimal body from the stored + // key + source + amount. + SquareRequestSnapshot []byte // AmountPence is the row's charge amount in pence — the amount the original // CreatePayment used. The replay-by-key must repeat it so Square's // idempotency dedup returns the original payment. AmountPence int64 // CreatedAt is the pending row's creation time; rows already past Square's // idempotency-key retention window cannot be replayed trustworthily. - CreatedAt time.Time - ItemID string // till_sales.item_id — the gift card ("" when NULL / non-gift-card) + CreatedAt time.Time + // BookingID is the payments row's booking_id ("" when the row has none). + // A payments row with NO booking is a gift-card purchase (BuyGiftCard + // inserts without a booking): rescuing it to 'completed' on a Square + // COMPLETED reconcile would permanently block the same-key retry that + // delivers the card, so such rows are kept pending instead (C6). + BookingID *string + // CreatedBy is the payments row's created_by user id (gift-card purchases + // always carry the purchaser), used to attribute the critical-payment admin + // notification. + CreatedBy *string + ItemID string // till_sales.item_id — the gift card ("" when NULL / non-gift-card) RedeemToUserID *string // gift_cards.redeemed_by — user credited by a create-with-redeem IsCreate bool // true when this sale created the gift card (timestamps equal) HasGiftCard bool // false when the LEFT JOIN found no gift_cards row (gc.id IS NULL) @@ -177,6 +206,14 @@ func sweepStaleRows(ctx context.Context, table string, cutoff time.Time) (resolv if r.SquarePaymentID != "" { switch reconcileStalePaymentAtSquare(ctx, table, r.SquarePaymentID) { case staleReconcileCompleted: + if table == "payments" && r.BookingID == nil { + // Gift-card purchase row: the charge landed at Square but + // the gift card was never delivered (C6). Completing the row + // permanently blocks the same-key retry that delivers the + // card — leave it pending and alert. + leaveGiftCardPurchasePending(ctx, r) + continue + } if rescueStaleRowCompleted(ctx, table, r.ID) { resolved++ completed++ @@ -257,8 +294,15 @@ func sweepKeyedStaleRows(ctx context.Context, table string, cutoff time.Time) (r log.Printf("Stale pending %s row %s has a stored idempotency key but is already past Square's key retention window — marked failed without a replay reconcile (may have been charged with a lost response)", table, r.ID) continue } - switch res, sqPayID := reconcileStalePaymentByKey(ctx, table, r.IdempotencyKey, r.AmountPence); res { + switch res, sqPayID := reconcileStalePaymentByKey(ctx, table, r); res { case staleReconcileCompleted: + if table == "payments" && r.BookingID == nil { + // Gift-card purchase row (C6): the charge landed at Square but + // the card was never delivered. Completing the row blocks the + // same-key retry that delivers the card — leave it pending. + leaveGiftCardPurchasePending(ctx, r) + continue + } if rescueKeyedStaleRowCompleted(ctx, table, r.ID, sqPayID) { resolved++ completed++ @@ -306,6 +350,7 @@ func fetchStaleRows(ctx context.Context, table string, cutoff time.Time, keyedOn if table == "till_sales" { rows, err = db.Conn.Query(ctx, ` SELECT ts.id, COALESCE(ts.square_payment_id, ''), COALESCE(ts.idempotency_key, ''), + COALESCE(ts.square_source_id, ''), COALESCE(ts.square_request_snapshot, ''), ts.created_at, ts.item_id, gc.redeemed_by, (ts.created_at = gc.created_at) AS is_create, (gc.id IS NOT NULL) AS has_gift_card, ts.total_amount @@ -316,7 +361,8 @@ func fetchStaleRows(ctx context.Context, table string, cutoff time.Time, keyedOn } else { rows, err = db.Conn.Query(ctx, ` SELECT id, COALESCE(square_payment_id, ''), COALESCE(idempotency_key, ''), - created_at, amount + COALESCE(square_source_id, ''), COALESCE(square_request_snapshot, ''), + created_at, amount, booking_id, created_by FROM `+table+` WHERE status = 'pending' AND created_at < $1`+keyedPredicate+` `, cutoff) @@ -343,13 +389,14 @@ func fetchStaleRows(ctx context.Context, table string, cutoff time.Time, keyedOn func scanStaleRow(table string, rows pgx.Rows) (staleRow, error) { var r staleRow if table == "till_sales" { - var itemID, redeemedBy sql.NullString + var itemID, redeemedBy, snapshot sql.NullString var isCreate *bool var hasGiftCard bool - if err := rows.Scan(&r.ID, &r.SquarePaymentID, &r.IdempotencyKey, &r.CreatedAt, + if err := rows.Scan(&r.ID, &r.SquarePaymentID, &r.IdempotencyKey, &r.SquareSourceID, &snapshot, &r.CreatedAt, &itemID, &redeemedBy, &isCreate, &hasGiftCard, &r.TotalAmount); err != nil { return r, err } + r.SquareRequestSnapshot = []byte(snapshot.String) r.ItemID = itemID.String if redeemedBy.Valid && redeemedBy.String != "" { r.RedeemToUserID = &redeemedBy.String @@ -360,10 +407,20 @@ func scanStaleRow(table string, rows pgx.Rows) (staleRow, error) { return r, nil } var amount float64 - if err := rows.Scan(&r.ID, &r.SquarePaymentID, &r.IdempotencyKey, &r.CreatedAt, &amount); err != nil { + var bookingID, createdBy, snapshot sql.NullString + if err := rows.Scan(&r.ID, &r.SquarePaymentID, &r.IdempotencyKey, &r.SquareSourceID, &snapshot, &r.CreatedAt, &amount, &bookingID, &createdBy); err != nil { return r, err } + r.SquareRequestSnapshot = []byte(snapshot.String) r.AmountPence = int64(math.Round(amount * 100)) + if bookingID.Valid && bookingID.String != "" { + b := bookingID.String + r.BookingID = &b + } + if createdBy.Valid && createdBy.String != "" { + c := createdBy.String + r.CreatedBy = &c + } return r, nil } @@ -438,22 +495,51 @@ func clawbackTillSaleFunding(ctx context.Context, r staleRow) bool { // reconcileStalePaymentByKey asks Square for the authoritative status of the // charge made under a stale pending row's idempotency key and returns the -// tri-state result. The replay returns the ORIGINAL payment for a retained key -// (Square's documented idempotency behavior — never a second charge); a -// COMPLETED payment rescues the row to 'completed' with its real payment id. A -// definitive rejection (ErrReplayKeyNotRetained — Square has no payment under -// the key, so the charge never happened) or a FAILED/CANCELED payment proves -// the charge never completed and fails the row. Any OTHER error (transport / -// 5xx / ambiguous) leaves the row pending — the charge may still have -// completed at Square. The second return value is the Square payment id of the -// completed payment ("" otherwise), written back on a rescue. -func reconcileStalePaymentByKey(ctx context.Context, table, idempotencyKey string, amountPence int64) (staleReconcileResult, string) { - pr, err := SquareClient.ReplayPaymentByKey(ctx, idempotencyKey, amountPence) +// tri-state result. The replay sends an IDENTICAL body to the original charge: +// the stored square_request_snapshot (the FULL request — source_id, key, +// amount and every field the original carried). Replaying a partial body would +// return IDEMPOTENCY_KEY_REUSED for a RETAINED key and strand the row pending +// forever. Square's idempotency guarantee returns the ORIGINAL payment for a +// retained key (never a second charge); a COMPLETED payment rescues the row to +// 'completed' with its real payment id. A definitive 4xx rejection +// (ErrReplayKeyNotRetained — Square attempted a real charge with the +// expired/used source and refused it, so the charge never happened) or a +// FAILED/CANCELED payment proves the charge never completed and fails the row. +// IDEMPOTENCY_KEY_REUSED is NEVER proof of no charge: with an identical body it +// can only mean the stored source differs from the original (a data bug), so +// the row is left pending with a CRITICAL log. Any OTHER error (transport / +// 5xx / ambiguous) leaves the row pending — the charge may still have completed +// at Square. The second return value is the Square payment id of the completed +// payment ("" otherwise), written back on a rescue. +func reconcileStalePaymentByKey(ctx context.Context, table string, r staleRow) (staleReconcileResult, string) { + snapshot := r.SquareRequestSnapshot + if len(bytes.TrimSpace(snapshot)) == 0 { + // Legacy row without a stored request snapshot — rebuild the minimal + // identical body (key + source + amount) exactly as the pre-snapshot + // replay did. Such rows can still be reconciled as long as the stored + // source/amount match the original charge. + fallback, mErr := json.Marshal(square.CreatePaymentReq{ + Amount: r.AmountPence, + Currency: "GBP", + SourceID: r.SquareSourceID, + IdempotencyKey: r.IdempotencyKey, + }) + if mErr != nil { + log.Printf("Stale pending %s reconcile by key: failed to rebuild replay body for row %s (%v) — leaving pending", table, r.ID, mErr) + return staleReconcileLeavePending, "" + } + snapshot = fallback + } + pr, err := SquareClient.ReplayPaymentByKey(ctx, snapshot) if err != nil { if errors.Is(err, square.ErrReplayKeyNotRetained) { - log.Printf("Stale pending %s reconcile by key: Square has no payment under the stored idempotency key (replay probe rejected) — marking failed; the charge provably never happened", table) + log.Printf("Stale pending %s reconcile by key: Square has no payment under the stored idempotency key (identical-body replay rejected) — marking failed; the charge provably never happened", table) return staleReconcileDefinitivelyFailed, "" } + if square.ErrorCode(err) == "IDEMPOTENCY_KEY_REUSED" { + log.Printf("CRITICAL: stale pending %s reconcile by key hit IDEMPOTENCY_KEY_REUSED — the stored square_source_id differs from the original charge's source (data bug); this is NOT proof the charge never happened — leaving pending — MANUAL RECONCILIATION REQUIRED", table) + return staleReconcileLeavePending, "" + } log.Printf("Stale pending %s reconcile by idempotency key hit an ambiguous error (%v) — leaving pending for a later sweep run", table, err) return staleReconcileLeavePending, "" } @@ -479,6 +565,44 @@ func reconcileStalePaymentByKey(ctx context.Context, table, idempotencyKey strin } } +// leaveGiftCardPurchasePending keeps a gift-card-purchase payment row (payments +// table, booking_id NULL) pending after Square confirms the charge COMPLETED, +// instead of rescuing it to 'completed'. Completing the row would permanently +// block the same-key retry (BuyGiftCard) that reuses the pending record to +// deliver the card — the customer would stay charged with no gift card (C6). +// The row is left pending with a CRITICAL log + admin notification so the retry +// can still deliver the card and an admin is alerted to reconcile manually. +func leaveGiftCardPurchasePending(ctx context.Context, r staleRow) { + log.Printf("CRITICAL: gift card purchase payment %s is COMPLETED at Square but the gift card was never issued (issue transaction failed, retry abandoned) — leaving the payment PENDING so a same-key retry can still deliver the card — MANUAL RECONCILIATION REQUIRED", r.ID) + insertCriticalPaymentNotification(ctx, nil, r.CreatedBy) +} + +// insertCriticalPaymentNotification surfaces an unresolved money event in the +// admin notification centre (reason 'critical_payment_log' — the DB-backed +// stand-in for the un-watched CRITICAL payment logs). bookingID is set when the +// issue ties to a booking (untracked terminal charges); userID is set when it +// ties to a user (gift-card purchases). The NOT EXISTS guard keeps ONE +// notification per issue instead of one per sweep run. +func insertCriticalPaymentNotification(ctx context.Context, bookingID, userID *string) { + tag, err := db.Conn.Exec(ctx, ` + INSERT INTO admin_notifications (reason, booking_id, user_id, created_at) + SELECT 'critical_payment_log', $1, $2, NOW() + WHERE NOT EXISTS ( + SELECT 1 FROM admin_notifications an + WHERE an.reason = 'critical_payment_log' + AND an.booking_id IS NOT DISTINCT FROM $1 + AND an.user_id IS NOT DISTINCT FROM $2 + ) + `, bookingID, userID) + if err != nil { + log.Printf("Failed to insert admin_notifications for critical payment issue (booking=%v user=%v): %v", bookingID, userID, err) + return + } + if int(tag.RowsAffected()) > 0 { + log.Printf("Inserted critical-payment admin notification (booking=%v user=%v)", bookingID, userID) + } +} + // staleReconcileResult is the tri-state outcome of reconciling one stale // pending row against Square. Only a definitively-resolved outcome touches the // row: an ambiguous answer (transport error / 5xx) leaves it pending so a @@ -624,7 +748,7 @@ func SweepStaleTerminalCheckouts(ctx context.Context) (int, error) { // terminal_checkouts table. A stale PENDING/IN_PROGRESS row means the // checkout is still live at Square (or was left after a crash / lost poll). rows, err := db.Conn.Query(ctx, ` - SELECT 'terminal_checkout', checkout_id, checkout_id + SELECT 'terminal_checkout', checkout_id, checkout_id, booking_id FROM terminal_checkouts WHERE status IN ('PENDING', 'IN_PROGRESS') AND created_at < $1 @@ -634,7 +758,7 @@ func SweepStaleTerminalCheckouts(ctx context.Context) (int, error) { } for rows.Next() { var r staleTerminalCheckoutRow - if err := rows.Scan(&r.Kind, &r.RowID, &r.CheckoutID); err != nil { + if err := rows.Scan(&r.Kind, &r.RowID, &r.CheckoutID, &r.BookingID); err != nil { log.Printf("Failed to scan stale terminal checkout row: %v", err) continue } @@ -644,7 +768,7 @@ func SweepStaleTerminalCheckouts(ctx context.Context) (int, error) { // Till-sale card-machine checkouts are tracked on the till_sales row. rows, err = db.Conn.Query(ctx, ` - SELECT 'till_sale', id, square_checkout_id + SELECT 'till_sale', id, square_checkout_id, '' FROM till_sales WHERE status = 'pending' AND square_checkout_id IS NOT NULL AND created_at < $1 @@ -654,7 +778,7 @@ func SweepStaleTerminalCheckouts(ctx context.Context) (int, error) { } for rows.Next() { var r staleTerminalCheckoutRow - if err := rows.Scan(&r.Kind, &r.RowID, &r.CheckoutID); err != nil { + if err := rows.Scan(&r.Kind, &r.RowID, &r.CheckoutID, &r.BookingID); err != nil { log.Printf("Failed to scan stale terminal checkout row: %v", err) continue } @@ -697,21 +821,16 @@ func SweepStaleTerminalCheckouts(ctx context.Context) (int, error) { switch { case rErr == nil && recheck.Status == "COMPLETED": // The customer completed the payment during the cancel window. - // The poll handler records it — mark the row COMPLETED (or - // leave the till sale pending) instead of failed. + // Record it if it was never polled/recorded — a COMPLETED + // checkout must not stay an untracked charge (H4). if r.Kind == "terminal_checkout" { - if tag, upErr := db.Conn.Exec(ctx, ` - UPDATE terminal_checkouts SET status = 'COMPLETED', updated_at = NOW() - WHERE checkout_id = $1 AND status IN ('PENDING', 'IN_PROGRESS') - `, r.RowID); upErr != nil { - log.Printf("Failed to mark terminal checkout %s completed after cancel re-check: %v", r.RowID, upErr) - } else if int(tag.RowsAffected()) > 0 { + if recordUntrackedTerminalPayment(ctx, r.CheckoutID, r.BookingID, recheck) { resolved++ } } else { log.Printf("Terminal checkout %s completed during sweep cancel — leaving sale %s pending (poll handler records it)", r.CheckoutID, r.RowID) } - log.Printf("Cancelled stale terminal checkout %s (%s %s, pending >%s) but re-check shows COMPLETED — recorded as completed, payment handled by the poll handler", r.CheckoutID, r.Kind, r.RowID, staleTerminalCheckoutAge) + log.Printf("Cancelled stale terminal checkout %s (%s %s, pending >%s) but re-check shows COMPLETED — payment recorded by the sweep", r.CheckoutID, r.Kind, r.RowID, staleTerminalCheckoutAge) case isTerminalCheckoutError(rErr) || errors.Is(rErr, square.ErrCheckoutPending): // The cancel landed (CANCELED / cancel-requested / expired / // still-reporting-pending-but-now-cancelled) — it can never @@ -734,14 +853,11 @@ func SweepStaleTerminalCheckouts(ctx context.Context) (int, error) { } case gErr == nil && pr.Status == "COMPLETED": if r.Kind == "terminal_checkout" { - // The payment is recorded by the poll handler; release the - // in-flight guard so a fresh charge can be created. - if tag, upErr := db.Conn.Exec(ctx, ` - UPDATE terminal_checkouts SET status = 'COMPLETED', updated_at = NOW() - WHERE checkout_id = $1 AND status IN ('PENDING', 'IN_PROGRESS') - `, r.RowID); upErr != nil { - log.Printf("Failed to mark terminal checkout %s completed: %v", r.RowID, upErr) - } else if int(tag.RowsAffected()) > 0 { + // The checkout completed at Square but was never polled/recorded + // (the frontend never called GetCheckoutStatus). Record the + // payment rows now — otherwise the charge stays invisible to + // refunds and TotalPaid (H4). + if recordUntrackedTerminalPayment(ctx, r.CheckoutID, r.BookingID, pr) { resolved++ } } else { @@ -784,11 +900,14 @@ func SweepStaleTerminalCheckouts(ctx context.Context) (int, error) { // staleTerminalCheckoutRow is one live-checkout row the sweep reads from // either table so it can resolve the checkout at Square before touching the // row. Kind is "terminal_checkout" (booking, terminal_checkouts table) or -// "till_sale" (till_sales.square_checkout_id). +// "till_sale" (till_sales.square_checkout_id). BookingID is the booking the +// terminal_checkouts row belongs to ("" for till sales) — needed to record an +// untracked COMPLETED terminal charge (H4). type staleTerminalCheckoutRow struct { Kind string RowID string CheckoutID string + BookingID string } // markTerminalCheckoutRowFailed moves one tracked row to the terminal 'failed' @@ -813,6 +932,162 @@ func markTerminalCheckoutRowFailed(ctx context.Context, r staleTerminalCheckoutR return int(tag.RowsAffected()) > 0 } +// recordUntrackedTerminalPayment records the payments row(s) for a terminal +// checkout that COMPLETED at Square but was never polled/recorded, then marks +// the terminal_checkouts row COMPLETED. The stale sweep otherwise leaves a real +// charge with NO payments row — invisible to refunds and TotalPaid (H4). It +// reuses the exact insert convention of GetCheckoutStatus: the same advisory +// 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). +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) + return false + } + service := NewPaymentService() + + // Serialize with the poll handler (GetCheckoutStatus): both record the same + // Square payment, so the advisory lock + dedup SELECT prevent a double + // insert (the idempotency_key UNIQUE constraint is the backstop). + pinConn, err := db.Conn.Acquire(ctx) + if err != nil { + log.Printf("Failed to acquire connection for terminal-completion lock: %v", err) + return false + } + defer pinConn.Release() + lockOK, err := acquireAdvisoryLock(ctx, pinConn, "crussell:terminal:"+pr.SquarePayID) + if err != nil { + log.Printf("Failed to acquire terminal-completion serialization lock for %s: %v", pr.SquarePayID, err) + return false + } + if !lockOK { + log.Printf("Terminal-completion serialization lock for %s not acquired within bound — a poll is already recording this checkout", pr.SquarePayID) + return false + } + defer func() { + if _, err := pinConn.Exec(context.Background(), `SELECT pg_advisory_unlock(hashtext('crussell:terminal:' || $1))`, pr.SquarePayID); err != nil { + log.Printf("Failed to release terminal-completion serialization lock for %s: %v", pr.SquarePayID, err) + } + }() + + tx, err := db.Conn.Begin(ctx) + if err != nil { + log.Printf("Failed to begin terminal-completion transaction: %v", err) + return false + } + defer func() { + if err := tx.Rollback(ctx); err != nil && !errors.Is(err, pgx.ErrTxClosed) { + log.Printf("Failed to rollback terminal-completion transaction: %v", err) + } + }() + + // Dedup by Square payment ID: a concurrent poll (or a prior sweep run) + // already recorded this charge — just release the in-flight guard. + var existingID string + if err := tx.QueryRow(ctx, ` + SELECT id FROM payments + WHERE booking_id = $1 AND square_payment_id = $2 + `, bookingID, pr.SquarePayID).Scan(&existingID); err == nil { + if _, upErr := tx.Exec(ctx, ` + UPDATE terminal_checkouts SET status = 'COMPLETED', updated_at = NOW() + WHERE checkout_id = $1 AND status IN ('PENDING', 'IN_PROGRESS') + `, checkoutID); upErr != nil { + log.Printf("Failed to mark terminal checkout %s completed: %v", checkoutID, upErr) + return false + } + if cErr := tx.Commit(ctx); cErr != nil { + log.Printf("Failed to commit terminal-completion transaction: %v", cErr) + return false + } + return true + } else if !errors.Is(err, pgx.ErrNoRows) { + log.Printf("Failed to check for existing terminal payment: %v", err) + return false + } + + // The payment type the admin charged is recorded on the checkout row by + // CreateTerminalPayment; fall back to 'full' for legacy rows. + var checkoutPaymentType string + if err := tx.QueryRow(ctx, ` + SELECT payment_type FROM terminal_checkouts WHERE checkout_id = $1 + `, checkoutID).Scan(&checkoutPaymentType); err != nil { + if !errors.Is(err, pgx.ErrNoRows) { + log.Printf("Failed to read payment type for checkout %s: %v", checkoutID, err) + } + checkoutPaymentType = "full" + } + + idempotencyKey := bookingID + "-terminal-" + strconv.FormatInt(pr.Amount, 10) + "-" + pr.SquarePayID + record := PaymentRecord{ + BookingID: bookingID, + PaymentType: checkoutPaymentType, + PaymentMethod: "in_person_card", + Status: "completed", + Amount: float64(pr.Amount) / 100.0, + SquarePaymentID: &pr.SquarePayID, + IdempotencyKey: &idempotencyKey, + Fees: float64(pr.Fees) / 100.0, + CreatedAt: clock.Now(), + UpdatedAt: clock.Now(), + } + + // M4 split: a terminal charge above the remaining booking value is a tip — + // record it as its own record so only the booking portion is refundable. + var records []PaymentRecord + bookingInfo, bErr := service.GetBookingPaymentInfo(ctx, bookingID) + if bErr == nil && bookingInfo != nil { + charged := float64(pr.Amount) / 100.0 + remainingBookingValue := math.Max(0, bookingInfo.TotalAmount-bookingInfo.TotalPaid) + bookingPortion := math.Min(charged, remainingBookingValue) + bookingPortion = math.Round(bookingPortion*100) / 100 + tipAmount := math.Round((charged-bookingPortion)*100) / 100 + if tipAmount > 0.004 { + records = buildTerminalSplitRecords(record, bookingInfo, bookingPortion, tipAmount) + } + } + if len(records) == 0 { + records = []PaymentRecord{record} + } + + primary := records[0] + paymentID, err := service.CreatePaymentRecordTx(ctx, tx, primary, nil) + if err != nil { + log.Printf("Failed to create payment record for untracked terminal charge %s: %v", pr.SquarePayID, err) + return false + } + ApplyVATToBookingPayment(ctx, tx, paymentID) + for _, rec := range records[1:] { + pid, cErr := service.CreatePaymentRecordTx(ctx, tx, rec, nil) + if cErr != nil { + log.Printf("Failed to create terminal tip split record: %v", cErr) + return false + } + ApplyVATToBookingPayment(ctx, tx, pid) + } + + // Release the in-flight guard: this checkout is now recorded. + if _, err := tx.Exec(ctx, ` + UPDATE terminal_checkouts SET status = 'COMPLETED', updated_at = NOW() + WHERE checkout_id = $1 AND status IN ('PENDING', 'IN_PROGRESS') + `, checkoutID); err != nil { + log.Printf("Failed to mark terminal checkout %s completed: %v", checkoutID, err) + return false + } + if err := tx.Commit(ctx); err != nil { + log.Printf("Failed to commit terminal-completion transaction: %v", err) + return false + } + + // The booking may now be fully paid — complete it like the poll handler does. + completeFullyPaidBooking(ctx, bookingID) + log.Printf("CRITICAL: recorded untracked terminal charge %s (booking %s) from stale checkout %s — payment row %s created (never polled by the frontend)", pr.SquarePayID, bookingID, checkoutID, paymentID) + return true +} + // isTerminalCheckoutError reports whether a GetCheckout error proves the // checkout can never complete. Square's HTTP client returns ErrCheckoutPending // for a still-live checkout and surfaces a definitively CANCELED status as a diff --git a/backend/handlers/payments/sweep_test.go b/backend/handlers/payments/sweep_test.go index 2205ba5..6c2a244 100644 --- a/backend/handlers/payments/sweep_test.go +++ b/backend/handlers/payments/sweep_test.go @@ -249,14 +249,14 @@ type staleReplayClient struct { err error } -func (c *staleReplayClient) ReplayPaymentByKey(ctx context.Context, idempotencyKey string, amount int64) (*square.PaymentResult, error) { +func (c *staleReplayClient) ReplayPaymentByKey(ctx context.Context, snapshotJSON []byte) (*square.PaymentResult, error) { if c.err != nil { return nil, c.err } if c.result != nil { return c.result, nil } - return c.SquareClient.ReplayPaymentByKey(ctx, idempotencyKey, amount) + return c.SquareClient.ReplayPaymentByKey(ctx, snapshotJSON) } // TestSweepStalePendingPayments_KeyedLostResponse_CompletedRescued locks the @@ -289,7 +289,7 @@ func TestSweepStalePendingPayments_KeyedLostResponse_CompletedRescued(t *testing // still inside Square's 24h idempotency-key retention window (so the replay // returns the original payment instead of being blind-failed). const key = "key-lost-response-completed" - if _, err := tx.Exec(ctx, "UPDATE payments SET created_at = NOW() - INTERVAL '23 hours', idempotency_key = $1 WHERE id = $2", key, staleID); err != nil { + 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) } @@ -495,8 +495,8 @@ func TestSweepStalePendingPayments_KeyedTillLostResponse_CompletedRescued(t *tes var saleID string err = tx.QueryRow(ctx, ` - INSERT INTO till_sales (item_type, description, quantity, unit_price, total_amount, payment_method, status, idempotency_key, created_by, created_at, updated_at) - VALUES ('gift_card', 'Gift Card create', 1, 50.00, 50.00, 'online_square', 'pending', $1, $2, NOW() - INTERVAL '23 hours', NOW()) + INSERT INTO till_sales (item_type, description, quantity, unit_price, total_amount, payment_method, status, idempotency_key, square_source_id, created_by, created_at, updated_at) + VALUES ('gift_card', 'Gift Card create', 1, 50.00, 50.00, 'online_square', 'pending', $1, 'cnon:test-card', $2, NOW() - INTERVAL '23 hours', NOW()) RETURNING id `, "key-lost-till-completed", adminID).Scan(&saleID) if err != nil { @@ -533,6 +533,164 @@ func TestSweepStalePendingPayments_KeyedTillLostResponse_CompletedRescued(t *tes } } +// TestSweepStalePendingPayments_KeyedGiftCardPurchase_Completed_LeavesPending +// locks C6: a gift-card purchase payment row (payments table, NO booking) whose +// charge COMPLETED at Square must NOT be rescued to 'completed' — completing it +// would permanently block the same-key retry that delivers the card (customer +// charged, no card). The row stays pending, a critical-payment admin +// notification is inserted, and a same-key retry remains possible. +func TestSweepStalePendingPayments_KeyedGiftCardPurchase_Completed_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) + } + + const key = "key-gc-purchase-completed" + var payID string + err = tx.QueryRow(ctx, ` + INSERT INTO payments (payment_type, payment_method, status, amount, idempotency_key, square_source_id, created_by, created_at, updated_at) + VALUES ('full', 'online_square', 'pending', 50.00, $1, 'cnon:test-card', $2, NOW() - INTERVAL '23 hours', NOW()) + RETURNING id + `, key, userID).Scan(&payID) + if err != nil { + t.Fatalf("failed to seed gift-card purchase payment: %v", err) + } + + origClient := SquareClient + mock := square.NewDevClient().(*square.MockClient) + pay, err := mock.CreatePayment(context.Background(), square.CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "cnon:test-card", + IdempotencyKey: key, + }) + if err != nil { + t.Fatalf("failed to seed completed Square payment: %v", err) + } + SquareClient = mock + 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) + } + + t.Cleanup(func() { + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM admin_notifications WHERE reason = 'critical_payment_log' AND user_id = $1`, userID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM payments WHERE id = $1`, payID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM users WHERE id = $1`, userID) + }) + + freshCtx := context.Background() + if _, err := SweepStalePendingPayments(freshCtx); err != nil { + t.Fatalf("sweep failed: %v", err) + } + + var status, sqPayID string + if err := db.Conn.QueryRow(freshCtx, "SELECT status, COALESCE(square_payment_id, '') FROM payments WHERE id = $1", payID).Scan(&status, &sqPayID); err != nil { + t.Fatalf("failed to query payment: %v", err) + } + if status != "pending" { + t.Errorf("expected the gift-card purchase row left 'pending' (a same-key retry must still deliver the card), got %q", status) + } + if sqPayID != "" { + t.Errorf("expected no square_payment_id written on the gift-card purchase row, got %q", sqPayID) + } + // The charge landed at Square (the mock still holds the payment). + if _, err := mock.GetPayment(freshCtx, pay.SquarePayID); err != nil { + t.Errorf("expected the Square payment to still exist (customer was charged): %v", err) + } + var notifCount int + if err := db.Conn.QueryRow(freshCtx, `SELECT COUNT(*) FROM admin_notifications WHERE reason = 'critical_payment_log' AND user_id = $1`, userID).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 unresolved gift-card purchase, got %d", notifCount) + } +} + +// TestSweepStalePendingPayments_KeyedSourceMismatch_LeavesPending locks C1: a +// replay that hits IDEMPOTENCY_KEY_REUSED (the stored square_source_id differs +// from the original charge's source — a data bug) must NEVER fail the row. The +// original charge may well have landed at Square, so the row is left pending +// for manual reconciliation instead of being marked failed (which would claw +// back funding / block a same-key retry). +func TestSweepStalePendingPayments_KeyedSourceMismatch_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) + } + + staleID, err := fixtures.CreateTestPayment(tx, bookingID, 2000.00, "online_square", "full", "pending") + if err != nil { + t.Fatalf("failed to create stale pending payment: %v", err) + } + const key = "key-source-mismatch" + // The stored source differs from what the charge actually used at Square — + // the data-bug condition the identical-body replay surfaces as + // IDEMPOTENCY_KEY_REUSED. + if _, err := tx.Exec(ctx, "UPDATE payments SET created_at = NOW() - INTERVAL '23 hours', idempotency_key = $1, square_source_id = 'cnon:wrong-source' WHERE id = $2", key, staleID); err != nil { + t.Fatalf("failed to age the stale payment: %v", err) + } + + origClient := SquareClient + mock := square.NewDevClient().(*square.MockClient) + if _, err := mock.CreatePayment(context.Background(), square.CreatePaymentReq{ + Amount: 200000, + Currency: "GBP", + SourceID: "cnon:original-source", + IdempotencyKey: key, + }); err != nil { + t.Fatalf("failed to seed completed Square payment: %v", err) + } + SquareClient = mock + 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) + } + + 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) + }) + + freshCtx := context.Background() + if _, err := SweepStalePendingPayments(freshCtx); err != nil { + t.Fatalf("sweep failed: %v", err) + } + + var status string + if err := db.Conn.QueryRow(freshCtx, "SELECT status FROM payments WHERE id = $1", staleID).Scan(&status); err != nil { + t.Fatalf("failed to query payment: %v", err) + } + if status != "pending" { + t.Errorf("expected IDEMPOTENCY_KEY_REUSED to leave the row pending (never proof of no charge), got %q", status) + } +} + // TestSweepStalePendingPayments_KeyedTillLostResponse_ProvenFailed_Clawbacks // locks the keyed clawback: a stale pending till sale with a stored idempotency // key whose charge Square PROVES never happened (no payment under the key) is @@ -858,7 +1016,7 @@ func (c *completingDuringCancelClient) GetCheckout(ctx context.Context, checkout if c.calls == 1 { return nil, square.ErrCheckoutPending } - return &square.PaymentResult{Status: "COMPLETED", SquarePayID: "sqp_completed_during_cancel"}, nil + return &square.PaymentResult{Status: "COMPLETED", SquarePayID: "sqp_completed_during_cancel", Amount: 5000}, nil } return c.SquareClient.GetCheckout(ctx, checkoutID) } @@ -888,6 +1046,9 @@ func TestSweepStaleTerminalCheckouts_CompletedDuringCancel_MarkedCompleted(t *te } t.Cleanup(func() { + // The sweep records the untracked COMPLETED charge as a payments row + // (H4) — clean it up before the booking so the FK delete order holds. + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM payments WHERE square_payment_id = 'sqp_completed_during_cancel'`) _, _ = db.Conn.Exec(context.Background(), `DELETE FROM terminal_checkouts WHERE checkout_id = $1`, checkoutID) _, _ = db.Conn.Exec(context.Background(), `DELETE FROM bookings WHERE id = $1`, bookingID) _, _ = db.Conn.Exec(context.Background(), `DELETE FROM services WHERE id = $1`, serviceID) diff --git a/backend/handlers/payments/testmain_test.go b/backend/handlers/payments/testmain_test.go index 4f23455..d109fdc 100644 --- a/backend/handlers/payments/testmain_test.go +++ b/backend/handlers/payments/testmain_test.go @@ -18,6 +18,12 @@ func TestMain(m *testing.M) { jwt.Init() square.Client = square.NewDevClient() SquareClient = square.Client + // Default the package to an explicit dev/mock Square env so generic payment + // tests run with the 2FA gate OFF. twoFactorEnforced() is fail-closed: an + // empty SQUARE_ENVIRONMENT now means ENFORCED, so a dev default must be set + // explicitly. Tests that assert enforcement flip these via t.Setenv. + os.Setenv("REQUIRE_2FA", "") + os.Setenv("SQUARE_ENVIRONMENT", "mock") 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 67e97cf..1a5d13e 100644 --- a/backend/handlers/payments/till.go +++ b/backend/handlers/payments/till.go @@ -603,6 +603,10 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { var needsSquarePayment bool var savedCardSqCardID string var savedCardCustomerID string + // tillSquareSourceID records the exact source_id sent to CreatePayment + // (the ccof: card id or the cnon: nonce) so the pending row stores it for + // the sweep's identical-body replay. + var tillSquareSourceID string // Pending-retry for card_machine: the original Square checkout may still be // live at the terminal. If the pending till_sales row already recorded a @@ -741,10 +745,17 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { } } + // 2FA gating (C5): charging a customer's saved card requires 2FA when + // the feature is enforced. + if cardUserID.Valid && !requireTwoFactorForCardAccess(w, r, service, cardUserID.String) { + return + } + if req.IdempotencyKey == "" { req.IdempotencyKey = uniqueChargeKey("till-") } + tillSquareSourceID = savedCardSqCardID saleStatus = "pending" needsSquarePayment = true case "card_machine": @@ -786,6 +797,7 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { req.IdempotencyKey = uniqueChargeKey("till-") } + tillSquareSourceID = req.CardToken saleStatus = "pending" needsSquarePayment = true case "on_the_house": @@ -801,15 +813,24 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { var tillSaleID string if existingPendingID != "" { // Reusing the pending sale row from a failed prior attempt — the sale - // was already inserted, so skip the insert and reuse its ID. + // was already inserted, so skip the insert and reuse its ID. Refresh + // the stored source: this attempt may charge a different token than the + // failed attempt (one-time cnon: nonces are spent), and the sweep + // replays the charge from the stored source. tillSaleID = existingPendingID + if tillSquareSourceID != "" { + if _, srcErr := tx.Exec(ctx, `UPDATE till_sales SET square_source_id = $1 WHERE id = $2`, tillSquareSourceID, tillSaleID); srcErr != nil { + log.Printf("Failed to update square_source_id on reused till sale %s: %v", tillSaleID, srcErr) + } + } } else { 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, - square_payment_id, square_checkout_id, idempotency_key, notes, created_by, created_at, updated_at - ) VALUES ($1, $2, $3, 1, $4, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, NOW(), NOW()) + square_payment_id, square_checkout_id, idempotency_key, notes, created_by, created_at, updated_at, + square_source_id + ) VALUES ($1, $2, $3, 1, $4, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, NOW(), NOW(), $14) RETURNING id `, req.ItemType, @@ -825,6 +846,7 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { req.IdempotencyKey, "Admin till sale: "+req.Action+" gift card", adminID, + tillSquareSourceID, ).Scan(&tillSaleID) if err != nil { log.Printf("Failed to insert till sale: %v", err) @@ -895,6 +917,16 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { Note: "Gift Card " + req.Action, BuyerEmail: buyerEmail, } + // M1: store the verbatim request JSON so the sweep can replay the + // charge with an IDENTICAL body under the same key — Square + // compares the whole request on key reuse, and a reconstructed body + // returns IDEMPOTENCY_KEY_REUSED, leaving the row pending forever. + if snap, mErr := json.Marshal(paymentReq); mErr != nil { + log.Printf("Failed to marshal square_request_snapshot for till sale %s: %v", tillSaleID, mErr) + } else if _, sErr := db.Conn.Exec(ctx, `UPDATE till_sales SET square_request_snapshot = $1 WHERE id = $2`, string(snap), tillSaleID); sErr != nil { + log.Printf("Failed to store square_request_snapshot for till sale %s: %v", tillSaleID, sErr) + } + paymentResult, squareErr = SquareClient.CreatePayment(ctx, paymentReq) } else if req.PaymentMethod == "online_square" { // PCI-DSS: raw PANs are never accepted. The admin till must supply a @@ -920,6 +952,16 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { BuyerEmail: buyerEmail, VerificationToken: verificationToken, } + // M1: store the verbatim request JSON so the sweep can replay the + // charge with an IDENTICAL body under the same key — Square + // compares the whole request on key reuse, and a reconstructed body + // returns IDEMPOTENCY_KEY_REUSED, leaving the row pending forever. + if snap, mErr := json.Marshal(paymentReq); mErr != nil { + log.Printf("Failed to marshal square_request_snapshot for till sale %s: %v", tillSaleID, mErr) + } else if _, sErr := db.Conn.Exec(ctx, `UPDATE till_sales SET square_request_snapshot = $1 WHERE id = $2`, string(snap), tillSaleID); sErr != nil { + log.Printf("Failed to store square_request_snapshot for till sale %s: %v", tillSaleID, sErr) + } + paymentResult, squareErr = SquareClient.CreatePayment(ctx, paymentReq) } diff --git a/backend/handlers/payments/twofa.go b/backend/handlers/payments/twofa.go new file mode 100644 index 0000000..b2a8d62 --- /dev/null +++ b/backend/handlers/payments/twofa.go @@ -0,0 +1,94 @@ +package payments + +import ( + "context" + "errors" + "log" + "net/http" + "os" + + "crussell/db" + "crussell/mw" + + "github.com/jackc/pgx/v5" +) + +// twoFactorEnforced reports whether 2FA is required for online card payments. +// It is fail-closed: enforcement is ON unless 2FA has been explicitly disabled +// (REQUIRE_2FA=false) or SQUARE_ENVIRONMENT explicitly selects the dev/mock +// stack (mock/dev/development/test). Empty or unknown SQUARE_ENVIRONMENT values +// are treated as production-enforced, so a mistyped env var can never silently +// disarm the gate — main.go logs a startup warning for that misconfiguration. +func twoFactorEnforced() bool { + if os.Getenv("REQUIRE_2FA") == "false" { + return false + } + return !IsExplicitDevOrMockEnv() +} + +// IsExplicitDevOrMockEnv reports whether SQUARE_ENVIRONMENT explicitly selects +// the dev/mock Square stack. Only these exact values are treated as dev; an +// empty or unknown value is NOT dev (fail-closed), because in production an +// unset/mistyped env var must never bypass the 2FA gate. +func IsExplicitDevOrMockEnv() bool { + switch os.Getenv("SQUARE_ENVIRONMENT") { + case "mock", "dev", "development", "test": + return true + default: + return false + } +} + +// TwoFactorEnforced is the exported form of twoFactorEnforced, so the user +// package (settings endpoints) and the profile handler can report whether 2FA +// is currently required without re-implementing the env logic. +func (s *PaymentService) TwoFactorEnforced() bool { + return twoFactorEnforced() +} + +// UserTwoFactorEnabled reports whether the user has completed 2FA setup +// (users.two_factor_enabled). It is the source of truth for the card-access +// gate: an enforced environment blocks online card access for users who have +// not enabled 2FA. +func (s *PaymentService) UserTwoFactorEnabled(ctx context.Context, userID string) (bool, error) { + var enabled bool + err := db.Conn.QueryRow(ctx, `SELECT two_factor_enabled FROM users WHERE id = $1`, userID).Scan(&enabled) + if err != nil { + return false, err + } + return enabled, nil +} + +// requireTwoFactorForCardAccess gates the saved-card online payment paths +// (PSD2 SCA stand-in until real SCA infra lands). It returns true when the +// request may proceed: +// +// - 2FA is not enforced (dev/mock), OR +// - the user has completed 2FA setup (two_factor_enabled). +// +// When 2FA is enforced and the user has not enabled it, a 403 JSON error is +// written (parseable by the frontend via extractErrorMessage) and false is +// returned — the caller must abort the charge. +func requireTwoFactorForCardAccess(w http.ResponseWriter, r *http.Request, service *PaymentService, userID string) bool { + if !twoFactorEnforced() { + return true + } + if service == nil { + service = &PaymentService{} + } + enabled, err := service.UserTwoFactorEnabled(r.Context(), userID) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + mw.RespondError(w, http.StatusForbidden, "Two-factor authentication is required to use online card payments. Enable it in your account settings.") + return false + } + log.Printf("failed to check two-factor status for user %s: %v", userID, err) + mw.RespondError(w, http.StatusInternalServerError, "failed to check two-factor status") + return false + } + if enabled { + return true + } + mw.RespondError(w, http.StatusForbidden, "Two-factor authentication is required to use online card payments. Enable it in your account settings.") + return false +} diff --git a/backend/handlers/payments/twofa_test.go b/backend/handlers/payments/twofa_test.go new file mode 100644 index 0000000..666a1ea --- /dev/null +++ b/backend/handlers/payments/twofa_test.go @@ -0,0 +1,340 @@ +//go:build test && dev + +package payments + +// Tests for the PSD2 SCA stand-in gate (twofa.go): the twoFactorEnforced() env +// matrix, requireTwoFactorForCardAccess() gating, and the end-to-end +// enforcement of the saved-card payment paths in CreateBookingPayment / +// CreateTillSale. Tests that flip REQUIRE_2FA/SQUARE_ENVIRONMENT via t.Setenv +// must stay sequential (no t.Parallel): os.Getenv is process-global and +// t.Setenv panics under t.Parallel. Sequential tests run before this package's +// parallel batch, so the enforced env never leaks into parallel tests. + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "crussell/db" + "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" +) + +func helperEnvEnforce2FA(t *testing.T) { + t.Helper() + t.Setenv("REQUIRE_2FA", "true") + t.Setenv("SQUARE_ENVIRONMENT", "production") +} + +func TestTwoFactorEnforced(t *testing.T) { + tests := []struct { + name string + require2FA string + squareEnv string + wantEnforced bool + }{ + // Fail-closed default: empty/unknown SQUARE_ENVIRONMENT is treated as + // production-enforced, so a mistyped env var can never silently disarm + // the gate. + {"empty_env_fail_closed_enforced", "", "", true}, + {"unknown_env_fail_closed_enforced", "", "staging", true}, + {"require2fa_false_disables_prod", "false", "production", false}, + {"require2fa_false_disables_sandbox", "false", "sandbox", false}, + {"require2fa_false_disables_unknown_env", "false", "staging", false}, + {"production_enforced", "", "production", true}, + {"sandbox_enforced", "", "sandbox", true}, + {"require2fa_true_prod_enforced", "true", "production", true}, + {"mock_never_enforced", "", "mock", false}, + {"dev_never_enforced", "", "dev", false}, + {"development_never_enforced", "", "development", false}, + {"test_never_enforced", "", "test", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("REQUIRE_2FA", tt.require2FA) + t.Setenv("SQUARE_ENVIRONMENT", tt.squareEnv) + require.Equal(t, tt.wantEnforced, twoFactorEnforced()) + require.Equal(t, tt.wantEnforced, NewPaymentService().TwoFactorEnforced(), "exported wrapper must match twoFactorEnforced") + }) + } +} + +// TestRequireTwoFactorForCardAccess_NotEnforced verifies the dev/mock path +// allows every request without touching the DB (no user rows are consulted). +// Uses an explicit mock env: empty SQUARE_ENVIRONMENT now defaults to ENFORCED +// (fail-closed). +func TestRequireTwoFactorForCardAccess_NotEnforced(t *testing.T) { + t.Setenv("REQUIRE_2FA", "") + t.Setenv("SQUARE_ENVIRONMENT", "mock") + req := httptest.NewRequest(http.MethodPost, "/", nil) + w := httptest.NewRecorder() + require.True(t, requireTwoFactorForCardAccess(w, req, nil, "000000000001")) + require.Equal(t, http.StatusOK, w.Code, "no response must be written when not enforced") +} + +func TestRequireTwoFactorForCardAccess_Enforced(t *testing.T) { + helperEnvEnforce2FA(t) + ctx, tx := testutils.SetupTestTx(t) + + t.Run("user_not_enabled_writes_403_json", func(t *testing.T) { + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) + w := httptest.NewRecorder() + ok := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID) + require.False(t, ok) + require.Equal(t, http.StatusForbidden, w.Code) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body), "403 body must be mw.RespondError JSON") + require.NotEmpty(t, body["error"]) + }) + + t.Run("user_enabled_allows", func(t *testing.T) { + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + _, err = tx.Exec(ctx, "UPDATE users SET two_factor_enabled = true WHERE id = $1", userID) + require.NoError(t, err) + req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) + w := httptest.NewRecorder() + require.True(t, requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID)) + require.Equal(t, http.StatusOK, w.Code) + }) + + t.Run("unknown_user_writes_403_json", func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) + w := httptest.NewRecorder() + require.False(t, requireTwoFactorForCardAccess(w, req, NewPaymentService(), "000000000000")) + require.Equal(t, http.StatusForbidden, w.Code) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body), "403 body must be mw.RespondError JSON") + require.NotEmpty(t, body["error"]) + }) +} + +// TestTwoFactorEnforced_CreateBookingPayment_SaveCard_Blocked verifies the +// end-to-end gate on the save-card path: enforced + user without 2FA → 403 with +// no payment row and no saved card (Square never called). +func TestTwoFactorEnforced_CreateBookingPayment_SaveCard_Blocked(t *testing.T) { + helperEnvEnforce2FA(t) + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + cardToken := "cnon:test-card-nonce" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + SaveCard: true, + IdempotencyKey: "2fa-save-card-blocked", + } + + w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusForbidden, w.Code, w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Contains(t, body["error"], "Two-factor") + + var payCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&payCount)) + require.Zero(t, payCount, "blocked 2FA request must not create a payment row") + var cardCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1", userID).Scan(&cardCount)) + require.Zero(t, cardCount, "blocked 2FA request must not persist a card") +} + +// TestTwoFactorEnforced_CreateBookingPayment_SavedCard_Blocked verifies the +// gate on charging an existing saved card. +func TestTwoFactorEnforced_CreateBookingPayment_SavedCard_Blocked(t *testing.T) { + helperEnvEnforce2FA(t) + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:mock_card_123", "VISA", "4242") + require.NoError(t, err) + + req := CreateBookingPaymentRequest{ + Amount: 5000, + PaymentType: "full", + CardID: &cardID, + IdempotencyKey: "2fa-saved-card-blocked", + } + + w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusForbidden, w.Code, w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Contains(t, body["error"], "Two-factor") + + var payCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&payCount)) + require.Zero(t, payCount, "blocked saved-card charge must not create a payment row") +} + +func TestTwoFactorEnforced_CreateBookingPayment_SaveCard_With2FA_Succeeds(t *testing.T) { + helperEnvEnforce2FA(t) + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + _, err := tx.Exec(ctx, "UPDATE users SET two_factor_enabled = true WHERE id = $1", userID) + require.NoError(t, err) + + cardToken := "cnon:test-card-nonce" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + SaveCard: true, + IdempotencyKey: "2fa-save-card-ok", + } + + w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + var payCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&payCount)) + require.Equal(t, 1, payCount) +} + +func TestTwoFactorEnforced_CreateBookingPayment_SavedCard_With2FA_Succeeds(t *testing.T) { + helperEnvEnforce2FA(t) + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + _, err := tx.Exec(ctx, "UPDATE users SET two_factor_enabled = true WHERE id = $1", userID) + require.NoError(t, err) + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:mock_card_123", "VISA", "4242") + require.NoError(t, err) + + req := CreateBookingPaymentRequest{ + Amount: 5000, + PaymentType: "full", + CardID: &cardID, + IdempotencyKey: "2fa-saved-card-ok", + } + + w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) +} + +// TestTwoFactorEnforced_NewCardCharge_NotGated verifies the gate applies ONLY +// to saved-card paths: a new-card (nonce) charge is allowed without 2FA even +// when enforced. +func TestTwoFactorEnforced_NewCardCharge_NotGated(t *testing.T) { + helperEnvEnforce2FA(t) + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + cardToken := "cnon:test-card-nonce" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + IdempotencyKey: "2fa-new-card-not-gated", + } + + w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) +} + +// TestTwoFactorEnforced_CreateTillSale_SavedCard_Blocked verifies the till's +// saved-card charge path: an admin charging a customer's saved card while the +// card's owner has no 2FA is blocked with 403 and no till_sale is created. +func TestTwoFactorEnforced_CreateTillSale_SavedCard_Blocked(t *testing.T) { + helperEnvEnforce2FA(t) + ctx, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + adminToken := jwt.GenerateTestToken(adminID, "admin") + + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sq_test_card_id", "VISA", "1234") + require.NoError(t, err) + + reqBody := TillSaleRequest{ + ItemType: "gift_card", + Action: "create", + Amount: 50.00, + PaymentMethod: "saved_card", + UserSavedCardID: &cardID, + UserID: &userID, + IdempotencyKey: "2fa-till-saved-blocked", + } + + 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") + req = req.WithContext(db.ContextWithTx(req.Context(), tx.(pgx.Tx))) + + w := httptest.NewRecorder() + r := chi.NewRouter() + r.Use(mw.RequireAuth) + r.Post("/api/admin/till/sale", CreateTillSale) + r.ServeHTTP(w, req) + + require.Equal(t, http.StatusForbidden, w.Code, w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Contains(t, body["error"], "Two-factor") + + var saleCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM till_sales").Scan(&saleCount)) + require.Zero(t, saleCount, "blocked till saved-card sale must not create a till_sale row") +} + +func TestTwoFactorEnforced_CreateTillSale_SavedCard_With2FA_Succeeds(t *testing.T) { + helperEnvEnforce2FA(t) + ctx, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + adminToken := jwt.GenerateTestToken(adminID, "admin") + _, err = tx.Exec(ctx, "UPDATE users SET two_factor_enabled = true WHERE id = $1", userID) + require.NoError(t, err) + + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sq_test_card_id", "VISA", "1234") + require.NoError(t, err) + + reqBody := TillSaleRequest{ + ItemType: "gift_card", + Action: "create", + Amount: 50.00, + PaymentMethod: "saved_card", + UserSavedCardID: &cardID, + UserID: &userID, + IdempotencyKey: "2fa-till-saved-ok", + } + + 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") + req = req.WithContext(db.ContextWithTx(req.Context(), tx.(pgx.Tx))) + + w := httptest.NewRecorder() + r := chi.NewRouter() + r.Use(mw.RequireAuth) + r.Post("/api/admin/till/sale", CreateTillSale) + r.ServeHTTP(w, req) + + require.Contains(t, []int{http.StatusOK, http.StatusCreated}, w.Code, w.Body.String()) +} diff --git a/backend/handlers/user/account.go b/backend/handlers/user/account.go index 5a89b84..9823261 100644 --- a/backend/handlers/user/account.go +++ b/backend/handlers/user/account.go @@ -21,6 +21,24 @@ import ( "github.com/jackc/pgx/v5" ) +// scrubAnonymizedUser2FA nulls the 2FA columns and staff notes that the SQL +// anonymize_user() function does not scrub: it predates the 2FA columns and +// intentionally preserves notes. A deleted user's live 2FA credential and any +// PII in staff notes must not survive erasure, so run this inside the same +// transaction as anonymize_user() to keep erasure atomic. +func scrubAnonymizedUser2FA(ctx context.Context, q db.Querier, userID string) error { + _, err := q.Exec(ctx, ` + UPDATE users + SET two_factor_enabled = FALSE, + two_factor_method = NULL, + two_factor_pending_code_hash = NULL, + two_factor_pending_code_expires = NULL, + notes = NULL + WHERE id = $1 + `, userID) + return err +} + // DELETE /api/user/account func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) { userID, ok := mw.GetUserID(r.Context()) @@ -158,6 +176,15 @@ func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) { return } + // GDPR erasure gap: anonymize_user() leaves the 2FA columns and staff + // notes on the row. Scrub them here, in the same transaction, so erasure + // is atomic with the anonymization. + if err := scrubAnonymizedUser2FA(ctx, tx, userID); err != nil { + log.Printf("Failed to scrub 2FA fields for user %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + if err := tx.Commit(ctx); err != nil { log.Printf("Failed to commit transaction for user anonymization: %v", err) http.Error(w, "server error", http.StatusInternalServerError) diff --git a/backend/handlers/user/gdpr_test.go b/backend/handlers/user/gdpr_test.go index cf8d31d..aaa2bf1 100644 --- a/backend/handlers/user/gdpr_test.go +++ b/backend/handlers/user/gdpr_test.go @@ -517,7 +517,11 @@ func TestAnonymizeUser_ClearsNotificationPrefs(t *testing.T) { } } -func TestAnonymizeUser_PreservesNotes(t *testing.T) { +// TestAnonymizeUser_Scrubs2FAAndNotes verifies the GDPR erasure gap closure: +// anonymize_user() alone leaves the 2FA columns and staff notes on the row, so +// the Go-side scrub (run in the same transaction by DeleteAccountHandler) must +// null them. +func TestAnonymizeUser_Scrubs2FAAndNotes(t *testing.T) { t.Parallel() ctx, tx := testutils.SetupTestTx(t) @@ -527,11 +531,16 @@ func TestAnonymizeUser_PreservesNotes(t *testing.T) { } _, err = tx.Exec(ctx, ` - UPDATE users SET notes = 'Client prefers quiet appointments and has a cat allergy' + UPDATE users SET + notes = 'Client prefers quiet appointments and has a cat allergy', + two_factor_enabled = TRUE, + two_factor_method = 'email', + two_factor_pending_code_hash = 'abc123', + two_factor_pending_code_expires = NOW() + INTERVAL '10 minutes' WHERE id = $1 `, userID) if err != nil { - t.Fatalf("failed to set user notes: %v", err) + t.Fatalf("failed to set user notes + 2FA: %v", err) } _, err = tx.Exec(ctx, `SELECT anonymize_user($1)`, userID) @@ -539,17 +548,98 @@ func TestAnonymizeUser_PreservesNotes(t *testing.T) { t.Fatalf("anonymize_user failed: %v", err) } - var notes, lastLoginAt interface{} - err = tx.QueryRow(ctx, `SELECT notes, last_login_at FROM users WHERE id = $1`, userID).Scan(¬es, &lastLoginAt) + // Mirror DeleteAccountHandler: scrub after anonymize_user in the same tx. + if err := scrubAnonymizedUser2FA(ctx, tx, userID); err != nil { + t.Fatalf("scrubAnonymizedUser2FA failed: %v", err) + } + + var notes interface{} + var enabled bool + var method, pendingHash, pendingExpires interface{} + err = tx.QueryRow(ctx, ` + SELECT notes, two_factor_enabled, two_factor_method, + two_factor_pending_code_hash, two_factor_pending_code_expires + FROM users WHERE id = $1 + `, userID).Scan(¬es, &enabled, &method, &pendingHash, &pendingExpires) if err != nil { - t.Fatalf("failed to query user notes: %v", err) + t.Fatalf("failed to query user after anonymization: %v", err) } - // Notes should be preserved (contain business-critical info like allergies) - if notes == nil { - t.Errorf("expected users.notes to be preserved after anonymization, got NULL") + if notes != nil { + t.Errorf("expected users.notes to be NULL after erasure, got %v", notes) } - if lastLoginAt != nil { - t.Errorf("expected users.last_login_at to be NULL after anonymization, got %v", lastLoginAt) + if enabled { + t.Error("expected two_factor_enabled to be FALSE after erasure") + } + if method != nil { + t.Errorf("expected two_factor_method to be NULL after erasure, got %v", method) + } + if pendingHash != nil { + t.Errorf("expected two_factor_pending_code_hash to be NULL after erasure, got %v", pendingHash) + } + if pendingExpires != nil { + t.Errorf("expected two_factor_pending_code_expires to be NULL after erasure, got %v", pendingExpires) + } +} + +// TestDeleteAccount_Scrubs2FAAndNotes runs the full DeleteAccountHandler for a +// user with 2FA enabled and staff notes, asserting the handler's transaction +// scrubs both. Kept sequential (no t.Parallel) because the handler reads the +// process-global payments.SquareClient. +func TestDeleteAccount_Scrubs2FAAndNotes(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create test user: %v", err) + } + + _, err = tx.Exec(ctx, ` + UPDATE users SET + notes = 'Staff note with PII', + two_factor_enabled = TRUE, + two_factor_method = 'sms', + two_factor_pending_code_hash = 'deadbeef', + two_factor_pending_code_expires = NOW() + INTERVAL '10 minutes' + WHERE id = $1 + `, userID) + if err != nil { + t.Fatalf("failed to set user notes + 2FA: %v", err) + } + + req := httptest.NewRequest(http.MethodDelete, "/api/user/account", nil) + req = req.WithContext(context.WithValue(ctx, mw.UserIDKey, userID)) + rr := httptest.NewRecorder() + DeleteAccountHandler(rr, req) + + if rr.Code != http.StatusNoContent { + t.Fatalf("expected 204, got %d. body: %s", rr.Code, rr.Body.String()) + } + + var notes interface{} + var enabled bool + var method, pendingHash, pendingExpires interface{} + err = tx.QueryRow(ctx, ` + SELECT notes, two_factor_enabled, two_factor_method, + two_factor_pending_code_hash, two_factor_pending_code_expires + FROM users WHERE id = $1 + `, userID).Scan(¬es, &enabled, &method, &pendingHash, &pendingExpires) + if err != nil { + t.Fatalf("failed to query user after deletion: %v", err) + } + if notes != nil { + t.Errorf("expected users.notes to be NULL after deletion, got %v", notes) + } + if enabled { + t.Error("expected two_factor_enabled to be FALSE after deletion") + } + if method != nil { + t.Errorf("expected two_factor_method to be NULL after deletion, got %v", method) + } + if pendingHash != nil { + t.Errorf("expected two_factor_pending_code_hash to be NULL after deletion, got %v", pendingHash) + } + if pendingExpires != nil { + t.Errorf("expected two_factor_pending_code_expires to be NULL after deletion, got %v", pendingExpires) } } diff --git a/backend/handlers/user/profile.go b/backend/handlers/user/profile.go index c7eb004..498c79c 100644 --- a/backend/handlers/user/profile.go +++ b/backend/handlers/user/profile.go @@ -25,6 +25,7 @@ import ( "crussell/clock" "crussell/db" "crussell/handlers/auth" + "crussell/handlers/payments" "crussell/internal/images" "crussell/internal/s3" "crussell/internal/validators" @@ -65,6 +66,13 @@ type UserProfile struct { DepositsRequired int `json:"deposits_required"` PreviousFirstName *string `json:"previousFirstName,omitempty"` PreviousLastName *string `json:"previousLastName,omitempty"` + + // Two-factor authentication state. TwoFactorRequired reflects whether the + // deployment enforces 2FA (REQUIRE_2FA + SQUARE_ENVIRONMENT), so the + // frontend can gate the online-card-payment UI. + TwoFactorEnabled bool `json:"twoFactorEnabled"` + TwoFactorRequired bool `json:"twoFactorRequired"` + TwoFactorMethod *string `json:"twoFactorMethod"` } type UpdateProfileRequest struct { @@ -141,11 +149,13 @@ func GetProfileHandler(w http.ResponseWriter, r *http.Request) { } var user UserProfile + var twoFactorMethod sql.NullString err := db.Conn.QueryRow(r.Context(), ` SELECT id, email, n_first_name, n_last_name, phone, date_of_birth::text, account_role, loyalty_stamps, referral_code, profile_pic_url, deposits_required, + two_factor_enabled, two_factor_method, (SELECT COUNT(*) FROM user_referrals WHERE referrer_id = users.id AND claimed_booking_id IS NOT NULL) AS referral_code_uses, (SELECT COALESCE(SUM(bd.discount_amount), 0) FROM booking_discounts bd WHERE bd.user_id = users.id AND bd.discount_source = 'referral') AS referral_savings FROM users @@ -153,7 +163,9 @@ func GetProfileHandler(w http.ResponseWriter, r *http.Request) { `, userID).Scan( &user.ID, &user.Email, &user.FirstName, &user.LastName, &user.Phone, &user.DateOfBirth, &user.Role, - &user.LoyaltyStamps, &user.ReferralCode, &user.ProfilePicURL, &user.DepositsRequired, &user.ReferralCodeUses, &user.ReferralSavings, + &user.LoyaltyStamps, &user.ReferralCode, &user.ProfilePicURL, &user.DepositsRequired, + &user.TwoFactorEnabled, &twoFactorMethod, + &user.ReferralCodeUses, &user.ReferralSavings, ) if err != nil { @@ -161,6 +173,11 @@ func GetProfileHandler(w http.ResponseWriter, r *http.Request) { return } + user.TwoFactorRequired = payments.NewPaymentService().TwoFactorEnforced() + if twoFactorMethod.Valid { + user.TwoFactorMethod = &twoFactorMethod.String + } + // Check if user has an unconsumed previous name (booking_id IS NULL means the name // change hasn't been "seen" via a completed booking yet). var prevFirstName, prevLastName sql.NullString diff --git a/backend/handlers/user/twofa.go b/backend/handlers/user/twofa.go new file mode 100644 index 0000000..b2f5ab3 --- /dev/null +++ b/backend/handlers/user/twofa.go @@ -0,0 +1,397 @@ +package user + +import ( + "crypto/rand" + "crypto/sha256" + "crypto/subtle" + "database/sql" + "encoding/hex" + "encoding/json" + "fmt" + "log" + "math/big" + "net/http" + "sync" + "time" + + "crussell/clock" + "crussell/db" + "crussell/handlers/payments" + "crussell/mw" +) + +// twoFARequired reports whether 2FA enforcement is active in this deployment. +// The user endpoints and the profile handler expose this to the frontend so it +// can gate the settings UI. +func twoFARequired() bool { + return payments.NewPaymentService().TwoFactorEnforced() +} + +// twoFAPendingExpiry is how long a generated verification code stays valid. +// Loose fake: real email/SMS infrastructure will own this lifetime once it lands. +const twoFAPendingExpiry = 10 * time.Minute + +// generateTwoFACode returns a random 6-digit verification code. +func generateTwoFACode() (string, error) { + n, err := rand.Int(rand.Reader, big.NewInt(1_000_000)) + if err != nil { + return "", err + } + return fmt.Sprintf("%06d", n.Int64()), nil +} + +// hashTwoFACode returns the SHA-256 hex digest of a verification code. The DB +// stores only the digest; the plaintext code is only ever logged in unenforced +// (dev) environments (see SetupTwoFAHandler). The digest is unsalted SHA-256 — +// peppering it via HMAC-SHA256 with a server-side 2FA_PEPPER secret is a future +// hardening step once such a secret is provisioned. +func hashTwoFACode(code string) string { + sum := sha256.Sum256([]byte(code)) + return hex.EncodeToString(sum[:]) +} + +// twoFAMaxAttempts is the number of consecutive failed verify attempts allowed +// before the pending code is invalidated and a new one must be requested. +const twoFAMaxAttempts = 5 + +// twoFAAttemptWindow bounds how long a per-user attempt counter lives before +// resetting, and doubles as the stale-entry eviction horizon for the map. +const twoFAAttemptWindow = 10 * time.Minute + +// twoFAMaxTrackedAttempts caps the in-memory attempt map so a flood of distinct +// user IDs cannot grow it without bound. Counters are purely in-memory (the DB +// schema is locked — there is no attempt column), so they reset on process +// restart; the 10-minute pending-code expiry bounds the practical impact. +const 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. +type twoFAAttemptState struct { + mu sync.Mutex + count int + lastAt time.Time +} + +var ( + twoFAAttemptMapMu sync.Mutex + twoFAAttemptMap = make(map[string]*twoFAAttemptState) +) + +// twoFAAttemptStateFor returns the per-user attempt state, creating it if +// needed. The map is bounded: stale entries are evicted opportunistically and, +// when at capacity, the least-recently-active entry is dropped. +func twoFAAttemptStateFor(userID string) *twoFAAttemptState { + twoFAAttemptMapMu.Lock() + defer twoFAAttemptMapMu.Unlock() + now := clock.Now() + + if len(twoFAAttemptMap) >= twoFAMaxTrackedAttempts { + var oldestID string + var oldestAt time.Time + for id, st := range twoFAAttemptMap { + if now.Sub(st.lastAt) > twoFAAttemptWindow { + delete(twoFAAttemptMap, id) + continue + } + if oldestID == "" || st.lastAt.Before(oldestAt) { + oldestID, oldestAt = id, st.lastAt + } + } + if len(twoFAAttemptMap) >= twoFAMaxTrackedAttempts && oldestID != "" { + delete(twoFAAttemptMap, oldestID) + } + } + + st := twoFAAttemptMap[userID] + if st == nil { + st = &twoFAAttemptState{lastAt: now} + twoFAAttemptMap[userID] = st + } + return st +} + +// twoFAResetAttempts clears a user's attempt counter. Called on successful +// verify and when a fresh code is generated via setup. +func twoFAResetAttempts(userID string) { + twoFAAttemptMapMu.Lock() + delete(twoFAAttemptMap, userID) + twoFAAttemptMapMu.Unlock() +} + +type TwoFAStatusResponse struct { + Enabled bool `json:"enabled"` + Method *string `json:"method"` + Required bool `json:"required"` +} + +// GET /api/user/2fa/status +func GetTwoFAStatusHandler(w http.ResponseWriter, r *http.Request) { + userID, ok := mw.GetUserID(r.Context()) + if !ok { + http.Error(w, "unauthorized", http.StatusUnauthorized) + return + } + + var enabled bool + var method sql.NullString + err := db.Conn.QueryRow(r.Context(), ` + SELECT two_factor_enabled, two_factor_method + FROM users + WHERE id = $1 + `, userID).Scan(&enabled, &method) + if err != nil { + log.Printf("failed to fetch 2FA status for user %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + + resp := TwoFAStatusResponse{Enabled: enabled, Required: twoFARequired()} + if method.Valid { + resp.Method = &method.String + } + if err := json.NewEncoder(w).Encode(resp); err != nil { + log.Printf("failed to encode 2FA status response: %v", err) + } +} + +type TwoFASetupRequest struct { + Method string `json:"method"` +} + +// POST /api/user/2fa/setup +// Generates a verification code and stores only its SHA-256 hash plus a +// 10-minute expiry in the pending columns. The code itself is delivered by +// logging it with a [2FA] prefix — a loose fake for the not-yet-wired email/SMS +// transport. When 2FA is not enforced (dev), the code is also returned in the +// response so the flow is testable without reading backend logs. +func SetupTwoFAHandler(w http.ResponseWriter, r *http.Request) { + userID, ok := mw.GetUserID(r.Context()) + if !ok { + http.Error(w, "unauthorized", http.StatusUnauthorized) + return + } + + var req TwoFASetupRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + http.Error(w, "invalid request", http.StatusBadRequest) + return + } + if req.Method != "email" && req.Method != "sms" { + http.Error(w, "method must be 'email' or 'sms'", http.StatusBadRequest) + return + } + + var enabled bool + err := db.Conn.QueryRow(r.Context(), `SELECT two_factor_enabled FROM users WHERE id = $1`, userID).Scan(&enabled) + if err != nil { + log.Printf("failed to check 2FA state for user %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + if enabled { + http.Error(w, "Two-factor authentication is already enabled", http.StatusConflict) + return + } + + code, err := generateTwoFACode() + if err != nil { + log.Printf("failed to generate 2FA code for user %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + + expires := clock.Now().Add(twoFAPendingExpiry) + _, err = db.Conn.Exec(r.Context(), ` + UPDATE users + SET two_factor_method = $2, + two_factor_pending_code_hash = $3, + two_factor_pending_code_expires = $4 + WHERE id = $1 + `, userID, req.Method, hashTwoFACode(code), expires) + if err != nil { + log.Printf("failed to store 2FA pending code for user %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + + // A fresh code invalidates any prior lockout state. + twoFAResetAttempts(userID) + + if !twoFARequired() { + // Dev-only convenience: unenforced environments log the plaintext code + // (loose fake delivery). NEVER log it when enforced — a production + // misconfiguration must not leak verification codes to stdout. + log.Printf("[2FA] verification code for user %s (%s): %s", userID, req.Method, code) + } else { + log.Printf("[2FA] 2FA code generated for user %s (delivery channel: %s — NOT SENT, fake delivery)", userID, req.Method) + } + + resp := map[string]any{"message": "Code sent"} + if !twoFARequired() { + // Dev convenience: unenforced environments return the code so the + // fake-delivery flow is usable without grepping the backend log. + resp["code"] = code + } + if err := json.NewEncoder(w).Encode(resp); err != nil { + log.Printf("failed to encode 2FA setup response: %v", err) + } +} + +type TwoFAVerifyRequest struct { + Code string `json:"code"` +} + +// POST /api/user/2fa/verify +// Confirms the pending code (SHA-256, timing-safe, not expired) and flips +// two_factor_enabled on. When 2FA is not enforced (dev) any code — including an +// empty one — verifies, so local testing never depends on reading the logged +// code. +func VerifyTwoFAHandler(w http.ResponseWriter, r *http.Request) { + userID, ok := mw.GetUserID(r.Context()) + if !ok { + http.Error(w, "unauthorized", http.StatusUnauthorized) + return + } + + var req TwoFAVerifyRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + http.Error(w, "invalid request", http.StatusBadRequest) + return + } + + if !twoFARequired() { + // Dev bypass: no code verification in unenforced environments. + if err := enableTwoFA(r, userID); err != nil { + log.Printf("failed to enable 2FA for user %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + writeTwoFAEnabled(w) + return + } + + // Enforced path — brute-force resistant. The attempt counter is per-user and + // in-memory (no schema change); after 5 consecutive failures the pending + // code is invalidated and further attempts get 429 until a new code is + // requested via setup. The per-user mutex serializes the critical section so + // concurrent attempts cannot race the limit. + st := twoFAAttemptStateFor(userID) + st.mu.Lock() + defer st.mu.Unlock() + + if now := clock.Now(); now.Sub(st.lastAt) > twoFAAttemptWindow { + st.count = 0 + st.lastAt = now + } + if st.count >= twoFAMaxAttempts { + http.Error(w, "Too many attempts. Request a new code.", http.StatusTooManyRequests) + return + } + + var pendingHash sql.NullString + var pendingExpires sql.NullTime + err := db.Conn.QueryRow(r.Context(), ` + SELECT two_factor_pending_code_hash, two_factor_pending_code_expires + FROM users + WHERE id = $1 + `, userID).Scan(&pendingHash, &pendingExpires) + if err != nil { + log.Printf("failed to fetch 2FA pending code for user %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + if !pendingHash.Valid || !pendingExpires.Valid || !pendingExpires.Time.After(clock.Now()) { + http.Error(w, "verification code is missing or has expired", http.StatusBadRequest) + return + } + // Constant-time compare (subtle) so a wrong code's match position cannot be + // inferred from response timing. Both digests are fixed-length hex. + if subtle.ConstantTimeCompare([]byte(hashTwoFACode(req.Code)), []byte(pendingHash.String)) != 1 { + st.count++ + st.lastAt = clock.Now() + if st.count >= twoFAMaxAttempts { + // Lockout reached: destroy the pending code so a stolen digest + // cannot be replayed against a fresh guessing loop. + if _, err := db.Conn.Exec(r.Context(), ` + UPDATE users + SET two_factor_pending_code_hash = NULL, + two_factor_pending_code_expires = NULL + WHERE id = $1 + `, userID); err != nil { + log.Printf("failed to invalidate 2FA pending code for user %s: %v", userID, err) + } + http.Error(w, "Too many attempts. Request a new code.", http.StatusTooManyRequests) + return + } + http.Error(w, "incorrect verification code", http.StatusBadRequest) + return + } + + // Success: clear the attempt counter before enabling 2FA. + st.count = 0 + st.lastAt = clock.Now() + twoFAResetAttempts(userID) + + if err := enableTwoFA(r, userID); err != nil { + log.Printf("failed to enable 2FA for user %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + writeTwoFAEnabled(w) +} + +// enableTwoFA persists two_factor_enabled=true and clears the pending code +// fields (the method was set during setup). +func enableTwoFA(r *http.Request, userID string) error { + _, err := db.Conn.Exec(r.Context(), ` + UPDATE users + SET two_factor_enabled = true, + two_factor_pending_code_hash = NULL, + two_factor_pending_code_expires = NULL + WHERE id = $1 + `, userID) + return err +} + +func writeTwoFAEnabled(w http.ResponseWriter) { + if err := json.NewEncoder(w).Encode(map[string]bool{"enabled": true}); err != nil { + log.Printf("failed to encode 2FA verify response: %v", err) + } +} + +type TwoFADisableRequest struct { + Code string `json:"code"` +} + +// POST /api/user/2fa/disable +// Turns 2FA off and clears method + pending fields for the authenticated user. +// The code field is accepted but ignored — a documented loose-fake simplification +// until the real SCA flow requires re-authentication to disable. +func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { + userID, ok := mw.GetUserID(r.Context()) + if !ok { + http.Error(w, "unauthorized", http.StatusUnauthorized) + return + } + + // Body is optional; decode leniently so an empty body disables cleanly. + var req TwoFADisableRequest + _ = json.NewDecoder(r.Body).Decode(&req) + + _, err := db.Conn.Exec(r.Context(), ` + UPDATE users + SET two_factor_enabled = false, + two_factor_method = NULL, + two_factor_pending_code_hash = NULL, + two_factor_pending_code_expires = NULL + WHERE id = $1 + `, userID) + if err != nil { + log.Printf("failed to disable 2FA for user %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + + w.WriteHeader(http.StatusOK) +} diff --git a/backend/handlers/user/twofa_test.go b/backend/handlers/user/twofa_test.go new file mode 100644 index 0000000..14df938 --- /dev/null +++ b/backend/handlers/user/twofa_test.go @@ -0,0 +1,452 @@ +//go:build test + +package user + +// Tests for the loose-fake 2FA endpoints (GET /api/user/2fa/status, +// POST /api/user/2fa/setup|verify|disable). Every test that flips +// REQUIRE_2FA/SQUARE_ENVIRONMENT via t.Setenv must stay sequential (no +// t.Parallel): os.Getenv is process-global and t.Setenv panics under +// t.Parallel. Sequential tests in this package run before the parallel batch, +// so the enforced env never leaks into parallel tests. + +import ( + "bytes" + "context" + "crypto/sha256" + "database/sql" + "encoding/hex" + "encoding/json" + "log" + "net/http" + "net/http/httptest" + "os" + "regexp" + "testing" + + "crussell/clock" + "crussell/db" + "crussell/mw" + "crussell/testutils" + "crussell/testutils/fixtures" + + "github.com/stretchr/testify/require" +) + +func twofaEnvEnforced(t *testing.T) { + t.Helper() + t.Setenv("REQUIRE_2FA", "true") + t.Setenv("SQUARE_ENVIRONMENT", "production") +} + +func twofaEnvUnenforced(t *testing.T) { + t.Helper() + // Explicit mock env: empty SQUARE_ENVIRONMENT now defaults to ENFORCED + // (fail-closed), so an unenforced test must opt in via an explicit dev value. + t.Setenv("REQUIRE_2FA", "") + t.Setenv("SQUARE_ENVIRONMENT", "mock") +} + +// performUser2FARequest invokes a handler with the authenticated-user context +// injected directly (the profile_test.go pattern). An empty userID simulates an +// unauthenticated request (no mw.UserIDKey in context). +func performUser2FARequest(t *testing.T, handler http.HandlerFunc, ctx context.Context, method, path string, body any, userID string) *httptest.ResponseRecorder { + t.Helper() + var req *http.Request + if body != nil { + b, err := json.Marshal(body) + require.NoError(t, err) + req = httptest.NewRequest(method, path, bytes.NewReader(b)) + req.Header.Set("Content-Type", "application/json") + } else { + req = httptest.NewRequest(method, path, nil) + } + if userID != "" { + req = req.WithContext(context.WithValue(ctx, mw.UserIDKey, userID)) + } else { + req = req.WithContext(ctx) + } + w := httptest.NewRecorder() + handler(w, req) + return w +} + +// seedPendingTwoFA writes a known verification code's SHA-256 hash plus a fresh +// expiry into the user's pending columns, so enforced-mode verify tests don't +// depend on reading the logged code. +func seedPendingTwoFA(t *testing.T, ctx context.Context, q db.Querier, userID, code string) { + t.Helper() + _, err := q.Exec(ctx, `UPDATE users + SET two_factor_method = 'email', + two_factor_pending_code_hash = $2, + two_factor_pending_code_expires = $3 + WHERE id = $1`, userID, hashTwoFACode(code), clock.Now().Add(twoFAPendingExpiry)) + require.NoError(t, err) +} + +func TestTwoFAStatus_NotEnabled(t *testing.T) { + twofaEnvUnenforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + w := performUser2FARequest(t, GetTwoFAStatusHandler, ctx, http.MethodGet, "/api/user/2fa/status", nil, userID) + require.Equal(t, http.StatusOK, w.Code) + + var resp TwoFAStatusResponse + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + require.False(t, resp.Enabled, "fresh user must report 2FA disabled") + require.False(t, resp.Required, "unenforced env must report required=false") + require.Nil(t, resp.Method) +} + +func TestTwoFAStatus_Required(t *testing.T) { + twofaEnvEnforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + w := performUser2FARequest(t, GetTwoFAStatusHandler, ctx, http.MethodGet, "/api/user/2fa/status", nil, userID) + require.Equal(t, http.StatusOK, w.Code) + + var resp TwoFAStatusResponse + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + require.False(t, resp.Enabled) + require.True(t, resp.Required, "enforced env must report required=true") +} + +func TestTwoFASetup_InvalidMethod(t *testing.T) { + twofaEnvUnenforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + w := performUser2FARequest(t, SetupTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/setup", TwoFASetupRequest{Method: "carrier-pigeon"}, userID) + require.Equal(t, http.StatusBadRequest, w.Code) +} + +func TestTwoFASetup_Valid_StoresHash(t *testing.T) { + twofaEnvUnenforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + w := performUser2FARequest(t, SetupTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/setup", TwoFASetupRequest{Method: "email"}, userID) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + var resp struct { + Message string `json:"message"` + Code string `json:"code"` + } + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + require.Equal(t, "Code sent", resp.Message) + require.Len(t, resp.Code, 6, "unenforced env must return the dev-convenience code") + + // The DB must hold the SHA-256 digest of exactly the returned code. + var pendingHash, method sql.NullString + var expires sql.NullTime + require.NoError(t, tx.QueryRow(ctx, ` + SELECT two_factor_pending_code_hash, two_factor_method, two_factor_pending_code_expires + FROM users WHERE id = $1`, userID).Scan(&pendingHash, &method, &expires)) + require.True(t, pendingHash.Valid, "setup must write a pending code hash") + require.Equal(t, "email", method.String) + require.True(t, expires.Valid && expires.Time.After(clock.Now()), "pending code must have a future expiry") + sum := sha256.Sum256([]byte(resp.Code)) + require.Equal(t, hex.EncodeToString(sum[:]), pendingHash.String, "stored hash must be the SHA-256 of the returned code") +} + +func TestTwoFASetup_AlreadyEnabled_Conflict(t *testing.T) { + twofaEnvUnenforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + _, err = tx.Exec(ctx, "UPDATE users SET two_factor_enabled = true WHERE id = $1", userID) + require.NoError(t, err) + + w := performUser2FARequest(t, SetupTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/setup", TwoFASetupRequest{Method: "sms"}, userID) + require.Equal(t, http.StatusConflict, w.Code) +} + +func TestTwoFAVerify_WrongCode(t *testing.T) { + twofaEnvEnforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + seedPendingTwoFA(t, ctx, tx, userID, "123456") + + w := performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "999999"}, userID) + require.Equal(t, http.StatusBadRequest, w.Code) + + var enabled bool + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_enabled FROM users WHERE id = $1", userID).Scan(&enabled)) + require.False(t, enabled, "wrong code must not enable 2FA") +} + +func TestTwoFAVerify_CorrectCode(t *testing.T) { + twofaEnvEnforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + seedPendingTwoFA(t, ctx, tx, userID, "123456") + + w := performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "123456"}, userID) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + var resp map[string]bool + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + require.True(t, resp["enabled"]) + + var enabled bool + var pendingHash sql.NullString + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_enabled, two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&enabled, &pendingHash)) + require.True(t, enabled, "correct code must enable 2FA") + require.False(t, pendingHash.Valid, "pending code must be cleared after verification") +} + +func TestTwoFAVerify_Expired(t *testing.T) { + twofaEnvEnforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + // Seed the correct code but backdate the expiry so the handler's + // pendingExpires.After(clock.Now()) check fails. + _, err = tx.Exec(ctx, `UPDATE users + SET two_factor_method = 'email', + two_factor_pending_code_hash = $2, + two_factor_pending_code_expires = NOW() - INTERVAL '1 minute' + WHERE id = $1`, userID, hashTwoFACode("123456")) + require.NoError(t, err) + + w := performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "123456"}, userID) + require.Equal(t, http.StatusBadRequest, w.Code) + + var enabled bool + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_enabled FROM users WHERE id = $1", userID).Scan(&enabled)) + require.False(t, enabled, "expired code must not enable 2FA") +} + +func TestTwoFAVerify_Unenforced_AnyCodeSucceeds(t *testing.T) { + twofaEnvUnenforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + // Dev bypass: in an unenforced env even an empty code with no pending row + // verifies. + w := performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: ""}, userID) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + var enabled bool + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_enabled FROM users WHERE id = $1", userID).Scan(&enabled)) + require.True(t, enabled) +} + +func TestTwoFADisable(t *testing.T) { + twofaEnvEnforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + _, err = tx.Exec(ctx, `UPDATE users SET two_factor_enabled = true, two_factor_method = 'email' WHERE id = $1`, userID) + require.NoError(t, err) + + w := performUser2FARequest(t, DisableTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/disable", nil, userID) + require.Equal(t, http.StatusOK, w.Code) + + var enabled bool + var method sql.NullString + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_enabled, two_factor_method FROM users WHERE id = $1", userID).Scan(&enabled, &method)) + require.False(t, enabled, "disable must clear two_factor_enabled") + require.False(t, method.Valid, "disable must clear the method") +} + +func TestTwoFA_Unauthenticated(t *testing.T) { + tests := []struct { + name string + method string + path string + body any + }{ + {"status", http.MethodGet, "/api/user/2fa/status", nil}, + {"setup", http.MethodPost, "/api/user/2fa/setup", TwoFASetupRequest{Method: "email"}}, + {"verify", http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "123456"}}, + {"disable", http.MethodPost, "/api/user/2fa/disable", nil}, + } + handlers := map[string]http.HandlerFunc{ + "status": GetTwoFAStatusHandler, + "setup": SetupTwoFAHandler, + "verify": VerifyTwoFAHandler, + "disable": DisableTwoFAHandler, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + w := performUser2FARequest(t, handlers[tt.name], context.Background(), tt.method, tt.path, tt.body, "") + require.Equal(t, http.StatusUnauthorized, w.Code) + }) + } +} + +func TestProfile_Get_IncludesTwoFAState(t *testing.T) { + twofaEnvEnforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + _, err = tx.Exec(ctx, `UPDATE users SET two_factor_enabled = true, two_factor_method = 'email' WHERE id = $1`, userID) + require.NoError(t, err) + + w := performUser2FARequest(t, GetProfileHandler, ctx, http.MethodGet, "/api/user/profile", nil, userID) + require.Equal(t, http.StatusOK, w.Code) + + var profile UserProfile + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &profile)) + require.True(t, profile.TwoFactorEnabled) + require.True(t, profile.TwoFactorRequired, "profile must expose the enforced flag") + require.NotNil(t, profile.TwoFactorMethod) + require.Equal(t, "email", *profile.TwoFactorMethod) +} + +func TestTwoFA_FailClosedDefaultEnforced(t *testing.T) { + // Empty SQUARE_ENVIRONMENT (a misconfigured prod deploy) must default to + // ENFORCED, never silently disable the gate. + t.Setenv("REQUIRE_2FA", "") + t.Setenv("SQUARE_ENVIRONMENT", "") + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + w := performUser2FARequest(t, GetTwoFAStatusHandler, ctx, http.MethodGet, "/api/user/2fa/status", nil, userID) + require.Equal(t, http.StatusOK, w.Code) + var resp TwoFAStatusResponse + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + require.True(t, resp.Required, "empty SQUARE_ENVIRONMENT must be treated as enforced (fail-closed)") +} + +func TestTwoFASetup_Enforced_NoCodeInResponse(t *testing.T) { + twofaEnvEnforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + w := performUser2FARequest(t, SetupTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/setup", TwoFASetupRequest{Method: "email"}, userID) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + var resp map[string]any + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + _, hasCode := resp["code"] + require.False(t, hasCode, "enforced setup must NOT return the code in the response") +} + +func TestTwoFASetup_CodeLoggedOnlyWhenUnenforced(t *testing.T) { + // Capture the standard logger so we can assert on what setup logs. + var buf bytes.Buffer + log.SetOutput(&buf) + t.Cleanup(func() { log.SetOutput(os.Stderr) }) + + // Enforced: the [2FA] log line must carry the delivery note but never the + // plaintext code — a production misconfig must not leak codes to stdout. + twofaEnvEnforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + w := performUser2FARequest(t, SetupTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/setup", TwoFASetupRequest{Method: "email"}, userID) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + out := buf.String() + require.Contains(t, out, "NOT SENT, fake delivery") + require.NotContains(t, out, "verification code for user", "enforced setup must NOT log the plaintext code") + + // Unenforced (dev): the plaintext code IS logged for the loose-fake flow. + buf.Reset() + twofaEnvUnenforced(t) + ctx2, tx2 := testutils.SetupTestTx(t) + userID2, err := fixtures.CreateTestUser(tx2) + require.NoError(t, err) + w = performUser2FARequest(t, SetupTwoFAHandler, ctx2, http.MethodPost, "/api/user/2fa/setup", TwoFASetupRequest{Method: "sms"}, userID2) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + require.Regexp(t, regexp.MustCompile(`\[2FA\].*\d{6}`), buf.String(), "unenforced setup must log the plaintext code") +} + +func TestTwoFAVerify_LockoutAfterFiveFailedAttempts(t *testing.T) { + twofaEnvEnforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + seedPendingTwoFA(t, ctx, tx, userID, "123456") + + // Attempts 1-4: plain 400. + for i := 0; i < 4; i++ { + w := performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "999999"}, userID) + require.Equal(t, http.StatusBadRequest, w.Code, "attempt %d", i+1) + } + + // Attempt 5: lockout — 429 and the pending code is invalidated. + w := performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "999999"}, userID) + require.Equal(t, http.StatusTooManyRequests, w.Code, w.Body.String()) + require.Contains(t, w.Body.String(), "Too many attempts. Request a new code.") + + var pendingHash sql.NullString + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&pendingHash)) + require.False(t, pendingHash.Valid, "lockout must invalidate the pending code") + + var enabled bool + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_enabled FROM users WHERE id = $1", userID).Scan(&enabled)) + require.False(t, enabled, "locked-out user must not be enabled") + + // Attempt 6: still 429 (even with the correct code) until a new code is + // requested via setup. + w = performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "123456"}, userID) + require.Equal(t, http.StatusTooManyRequests, w.Code, "post-lockout attempts must keep returning 429") +} + +func TestTwoFAVerify_NewCodeViaSetupResetsLockout(t *testing.T) { + twofaEnvEnforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + seedPendingTwoFA(t, ctx, tx, userID, "123456") + + // Reach lockout: 4 plain 400s, then the 5th failure locks out. + for i := 0; i < 4; i++ { + w := performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "999999"}, userID) + require.Equal(t, http.StatusBadRequest, w.Code, "attempt %d", i+1) + } + w := performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "999999"}, userID) + require.Equal(t, http.StatusTooManyRequests, w.Code, w.Body.String()) + + // Requesting a new code via setup resets the attempt counter, so + // verification is possible again. + w = performUser2FARequest(t, SetupTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/setup", TwoFASetupRequest{Method: "email"}, userID) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + // The setup-generated code is unknown (enforced), so seed a fresh known + // code and confirm the reset allows verification. + seedPendingTwoFA(t, ctx, tx, userID, "654321") + w = performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "654321"}, userID) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) +} + +func TestTwoFAVerify_WrongCodesAnyLengthRejected(t *testing.T) { + // Exercises the constant-time compare path: wrong codes of any length and + // shape fail identically (400) without enabling, while the correct code + // still succeeds — no length-based early exit leaks match information. + twofaEnvEnforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + seedPendingTwoFA(t, ctx, tx, userID, "123456") + + for _, code := range []string{"12345", "1234567", "abcdef", ""} { + w := performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: code}, userID) + require.Equal(t, http.StatusBadRequest, w.Code, "wrong code %q must be rejected", code) + } + + var enabled bool + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_enabled FROM users WHERE id = $1", userID).Scan(&enabled)) + require.False(t, enabled) + + // Four wrong attempts were consumed above; one more would lock out. Use a + // fresh user to prove the correct code still verifies. + userID2, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + seedPendingTwoFA(t, ctx, tx, userID2, "123456") + w := performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "123456"}, userID2) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) +} diff --git a/backend/handlers/webhooks/square.go b/backend/handlers/webhooks/square.go index 686b21f..9a70ce9 100644 --- a/backend/handlers/webhooks/square.go +++ b/backend/handlers/webhooks/square.go @@ -4,8 +4,10 @@ import ( "context" "crypto/hmac" "crypto/sha256" + "database/sql" "encoding/base64" "encoding/json" + "errors" "fmt" "io" "log" @@ -14,6 +16,8 @@ import ( "sync" "crussell/db" + + "github.com/jackc/pgx/v5" ) type SquareWebhookEvent struct { @@ -144,70 +148,79 @@ func HandleSquareWebhook(w http.ResponseWriter, r *http.Request) { return } - // Dedup BEFORE dispatch: a correctly signed replay of a handled event must - // not re-enter the handlers (which will mutate state once wired). The - // in-memory fast-path drops recent replays without a DB round-trip; the - // square_webhook_events INSERT ... ON CONFLICT DO NOTHING is the source of - // truth — 0 rows affected means the event was already handled (persisted - // from before a restart, or a concurrent duplicate) and dispatch is - // skipped. Returns 200 to acknowledge delivery without processing. - // - // ORDERING NOTE: the dedup row is committed before dispatch. If the process - // crashes between the insert and dispatch, the event is dropped (Square's - // retry is 200-skipped). This is acceptable while dispatch is log-only; - // when handlers mutate state, switch to dispatch-then-record or make - // dispatch idempotent. - if event.EventID != "" { - if squareWebhookEventsSeen.has(event.EventID) { - log.Printf("[SQUARE-WEBHOOK] Duplicate event_id %s; skipping (already processed)", event.EventID) - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("ok")) - return - } - if db.Conn == nil { - log.Printf("[SQUARE-WEBHOOK] DB unavailable — rejecting event_id %s (fail-closed)", event.EventID) - http.Error(w, "webhook processing unavailable", http.StatusServiceUnavailable) - return - } - tag, err := db.Conn.Exec(r.Context(), - "INSERT INTO square_webhook_events (event_id) VALUES ($1) ON CONFLICT (event_id) DO NOTHING", event.EventID) - if err != nil { - // Fail closed: without a successful dedup write we cannot prove this - // event hasn't been handled before, so reject and let Square retry - // later. event_id is not PII, so logging it is safe. - log.Printf("[SQUARE-WEBHOOK] Failed to record event_id %s (dedup write failed): %v", event.EventID, err) - http.Error(w, "webhook processing unavailable", http.StatusServiceUnavailable) - return - } - // Record in the fast-path cache only after the DB write succeeds, so a - // failed write never leaves a stale entry that would drop a retry. - squareWebhookEventsSeen.register(event.EventID) - if tag.RowsAffected() == 0 { - log.Printf("[SQUARE-WEBHOOK] Duplicate event_id %s; skipping (already processed)", event.EventID) - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("ok")) - return - } + // Fast-path dedup: a correctly signed replay of a recently handled event is + // dropped in memory without a DB round-trip. The persistent source of truth + // is the square_webhook_events row committed AFTER dispatch below, so this + // cache never hides an event whose dedup row is not yet persisted — a crash + // before that commit simply replays the event, which the idempotent handlers + // absorb. + if squareWebhookEventsSeen.has(event.EventID) { + log.Printf("[SQUARE-WEBHOOK] Duplicate event_id %s; skipping (already processed)", event.EventID) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("ok")) + return + } + if db.Conn == nil { + log.Printf("[SQUARE-WEBHOOK] DB unavailable — rejecting event_id %s (fail-closed)", event.EventID) + http.Error(w, "webhook processing unavailable", http.StatusServiceUnavailable) + return } log.Printf("[SQUARE-WEBHOOK] Received event: %s", event.Type) + // Dispatch FIRST, then commit the dedup row. The handlers mutate state, so a + // dedup row committed before dispatch would permanently drop the event on a + // crash between the insert and the dispatch (Square's retry would be + // 200-skipped, and dispute.created has no sweep fallback). Committing after + // a successful dispatch keeps delivery at-least-once: on any dispatch error + // NO dedup row is written and we return 5xx so Square retries. The handlers + // are idempotent (status='pending'-guarded UPDATEs keyed on the Square id), + // so a retry — or two retries dispatching concurrently — applies each state + // change at most once and is otherwise a no-op. + var dispatchErr error switch event.Type { - case "payment.updated", "payment.created", "payment.completed": - handlePaymentUpdated(event.Data) - case "refund.updated", "refund.created", "refund.completed", "refund.failed": - handleRefundUpdated(event.Data) + case "payment.updated", "payment.created": + dispatchErr = handlePaymentUpdated(event.Data) + case "refund.updated", "refund.created": + dispatchErr = handleRefundUpdated(event.Data) case "dispute.created": - handleDisputeCreated(event.Data) + dispatchErr = handleDisputeCreated(event.Data) case "dispute.state.updated": - handleDisputeStateUpdated(event.Data) - case "dispute.evidence.submitted", "dispute.evidence.created", "dispute.evidence.removed", "dispute.evidence.deleted": - handleDisputeEvidence(event.Data) + dispatchErr = handleDisputeStateUpdated(event.Data) + case "dispute.evidence.created", "dispute.evidence.deleted": + dispatchErr = handleDisputeEvidence(event.Data) case "terminal.checkout.created", "terminal.checkout.updated": - handleTerminalCheckout(event.Data) + dispatchErr = handleTerminalCheckout(event.Data) default: log.Printf("[SQUARE-WEBHOOK] Unknown event type: %s", event.Type) } + if dispatchErr != nil { + log.Printf("[SQUARE-WEBHOOK] Event %s (%s) dispatch failed: %v — NOT recording dedup row; Square will retry", event.Type, event.EventID, dispatchErr) + http.Error(w, "webhook processing failed", http.StatusServiceUnavailable) + return + } + + // Commit the dedup row AFTER successful dispatch. Fail closed on a write + // error: without a persisted row we cannot prove the event was handled, so + // reject and let Square retry (the retry re-dispatches idempotently and + // retries the insert). event_id is not PII, so logging it is safe. + tag, err := db.Conn.Exec(r.Context(), + "INSERT INTO square_webhook_events (event_id) VALUES ($1) ON CONFLICT (event_id) DO NOTHING", event.EventID) + if err != nil { + log.Printf("[SQUARE-WEBHOOK] Failed to record event_id %s (dedup write failed): %v", event.EventID, err) + http.Error(w, "webhook processing unavailable", http.StatusServiceUnavailable) + return + } + // Record in the fast-path cache only after the DB write succeeds, so a + // failed write never leaves a stale entry that would drop a retry. + squareWebhookEventsSeen.register(event.EventID) + if tag.RowsAffected() == 0 { + // A concurrent duplicate delivery already committed this event's row. + log.Printf("[SQUARE-WEBHOOK] Duplicate event_id %s; acknowledging (already processed)", event.EventID) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("ok")) + return + } w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte("ok")) @@ -416,42 +429,45 @@ func insertCriticalPaymentNotification(bookingID string) { // markPaymentFailed flips a payment to 'failed' after a lost dispute — the // money was charged back, so the row must not read as collected. 'refunded' // rows are left alone (the money was returned by refund, not charged back). -func markPaymentFailed(paymentID string) { +func markPaymentFailed(paymentID string) error { if paymentID == "" { - return + return nil } _, err := db.Conn.Exec(context.Background(), "UPDATE payments SET status = 'failed', updated_at = NOW() WHERE id = $1 AND status IN ('pending', 'completed')", paymentID) if err != nil { log.Printf("[SQUARE-WEBHOOK] Failed to mark payment %s failed after lost dispute: %v", paymentID, err) + return err } + return nil } // handlePaymentUpdated reconciles a Square Payment state change against the // local payments row (real-time counterpart to the stale-pending sweep). The // Square id is logged, never the payload (PII). Idempotent: the UPDATE is a // no-op when the local status already matches, and event_id dedup prevents -// re-entry at the handler level. -func handlePaymentUpdated(data json.RawMessage) { +// re-entry at the handler level. A non-nil error means dispatch failed and the +// caller must NOT commit the dedup row (Square retries). +func handlePaymentUpdated(data json.RawMessage) error { var env squareWebhookData if err := json.Unmarshal(data, &env); err != nil { log.Printf("[SQUARE-WEBHOOK] payment.updated received (payload length=%d)", len(data)) - return + return nil } if env.ID == "" { log.Printf("[SQUARE-WEBHOOK] payment.updated received (payload length=%d)", len(data)) - return + return nil } var payment squarePaymentPayload if !parseSquareObject(env.Object, "payment", &payment) || payment.ID == "" || payment.Status == "" { log.Printf("[SQUARE-WEBHOOK] payment.updated received (data.id=%s)", env.ID) - return + return nil } localStatus, terminal := squarePaymentStatusToLocal(payment.Status) if !terminal { log.Printf("[SQUARE-WEBHOOK] payment.updated: square payment %s status %q is non-terminal — no local state change", payment.ID, payment.Status) - return + return nil } // Only 'pending' rows are candidates for a terminal transition — the same // conservative rule the stale-pending sweeps use. A webhook for an already @@ -463,47 +479,231 @@ func handlePaymentUpdated(data json.RawMessage) { localStatus, payment.ID) if err != nil { log.Printf("[SQUARE-WEBHOOK] Failed to update payment %s to status %s: %v", payment.ID, localStatus, err) - return + return err } if tag.RowsAffected() > 0 { log.Printf("[SQUARE-WEBHOOK] payment.updated: square payment %s → local status %s", payment.ID, localStatus) } // A Square charge can also map to a till_sales row (online gift-card // purchase, retail at the till) — reconcile those too. Same pending-only - // guard: never revert a terminal till-sale status. + // guard: never revert a terminal till-sale status. A definitively failed + // charge (Square FAILED/CANCELED) claws back the gift-card funding those + // pending sales added, exactly like the stale-pending sweep + // (handlers/payments/sweep.go); an ambiguous status never reaches here. + if localStatus == "failed" { + return clawbackFailedTillSales(payment.ID) + } tsTag, err := db.Conn.Exec(context.Background(), `UPDATE till_sales SET status = $1, updated_at = NOW() WHERE square_payment_id = $2 AND status = 'pending'`, localStatus, payment.ID) if err != nil { log.Printf("[SQUARE-WEBHOOK] Failed to reconcile till_sales for square payment %s: %v", payment.ID, err) - return + return err } if tsTag.RowsAffected() > 0 { log.Printf("[SQUARE-WEBHOOK] payment.updated: reconciled %d till_sale(s) for square payment %s → status %s", tsTag.RowsAffected(), payment.ID, localStatus) } + return nil +} + +// errTillSaleNotPending mirrors the sweep's claim-first guard: the gating +// UPDATE matched zero rows, so the sale is no longer 'pending' and its gift +// card must be left untouched (a sale already resolved by a concurrent +// completion/clawback is not ours to revert). +var errTillSaleNotPending = errors.New("till sale is not pending") + +// clawbackFailedTillSales reverts the gift-card funding of every still-pending +// till sale funded by a Square charge that is DEFINITIVELY failed (Square +// FAILED/CANCELED — never ambiguous). It mirrors the stale-pending sweep's +// clawbackTillSaleFunding + revertGiftCardFunding (handlers/payments/sweep.go, +// till.go): each sale is claimed with a status='pending' guard so an +// already-resolved row is skipped without error, and the failed mark + funding +// revert commit atomically. A non-nil error means a DB failure left a pending +// sale's funding unreverted — the caller rejects the webhook so Square retries +// the clawback (the sweep is the eventual backstop). +func clawbackFailedTillSales(squarePaymentID string) error { + rows, err := db.Conn.Query(context.Background(), ` + SELECT ts.id, ts.item_type, ts.item_id, ts.total_amount, gc.redeemed_by, + (ts.created_at = gc.created_at) AS is_create + FROM till_sales ts + LEFT JOIN gift_cards gc ON gc.id = ts.item_id + WHERE ts.square_payment_id = $1 AND ts.status = 'pending' + `, squarePaymentID) + if err != nil { + log.Printf("[SQUARE-WEBHOOK] Failed to read pending till_sales for funding clawback (square payment %s): %v", squarePaymentID, err) + return err + } + defer rows.Close() + for rows.Next() { + var ( + saleID string + itemType string + itemID sql.NullString + totalAmount float64 + redeemedBy sql.NullString + isCreate *bool + ) + if err := rows.Scan(&saleID, &itemType, &itemID, &totalAmount, &redeemedBy, &isCreate); err != nil { + log.Printf("[SQUARE-WEBHOOK] Failed to scan pending till_sale for funding clawback (square payment %s): %v", squarePaymentID, err) + return err + } + if err := clawbackOneTillSale(saleID, itemType, itemID, totalAmount, redeemedBy, isCreate); err != nil { + return err + } + } + return rows.Err() +} + +// clawbackOneTillSale resolves one pending till sale of a definitively failed +// charge. A gift-card sale has its funding reverted atomically with the failed +// mark; a sale with no gift card (future retail product / orphaned item) is +// only marked failed. An already-resolved sale is skipped, not an error. +func clawbackOneTillSale(saleID, itemType string, itemID sql.NullString, totalAmount float64, redeemedBy sql.NullString, isCreate *bool) error { + if itemType != "gift_card" || !itemID.Valid || itemID.String == "" || isCreate == nil { + // No gift card to claw back — mark the sale failed without touching + // any card (mirrors the sweep's non-gift-card branch). + tag, err := db.Conn.Exec(context.Background(), ` + UPDATE till_sales SET status = 'failed', updated_at = NOW() + WHERE id = $1 AND status = 'pending' + `, saleID) + if err != nil { + log.Printf("[SQUARE-WEBHOOK] Failed to mark till sale %s failed: %v", saleID, err) + return err + } + if tag.RowsAffected() > 0 { + log.Printf("[SQUARE-WEBHOOK] payment.updated: marked till sale %s failed (no gift card to claw back)", saleID) + } + return nil + } + action := "topup" + if *isCreate { + action = "create" + } + var redeem *string + if redeemedBy.Valid && redeemedBy.String != "" { + redeem = &redeemedBy.String + } + if err := revertTillSaleGiftCardFunding(action, itemID.String, totalAmount, redeem, saleID); err != nil { + if errors.Is(err, errTillSaleNotPending) { + log.Printf("[SQUARE-WEBHOOK] Till sale %s was already resolved (not pending) — skipping funding clawback", saleID) + return nil + } + log.Printf("CRITICAL: [SQUARE-WEBHOOK] failed to claw back gift card %s funding for failed till sale %s: %v — MANUAL RECONCILIATION REQUIRED: gift card may still be funded", itemID.String, saleID, err) + return err + } + return nil +} + +// revertTillSaleGiftCardFunding undoes the gift-card funding of a till sale +// whose charge definitively failed, in the SAME transaction as the failed mark +// (claim-first): a created card is deleted (with its purchase transaction) and +// any immediate redeem-to-account credit reversed; a topped-up card has the +// amount subtracted back out and its top-up transaction removed. SQL mirrors +// handlers/payments/till.go revertGiftCardFunding. +func revertTillSaleGiftCardFunding(action, giftCardID string, amount float64, redeemToUserID *string, tillSaleID string) error { + tx, err := db.Conn.Begin(context.Background()) + if err != nil { + return fmt.Errorf("failed to begin clawback transaction: %w", err) + } + defer func() { + if err := tx.Rollback(context.Background()); err != nil && !errors.Is(err, pgx.ErrTxClosed) { + log.Printf("[SQUARE-WEBHOOK] failed to rollback gift-card clawback transaction: %v", err) + } + }() + + // Claim the sale first: the row lock serializes against a concurrent + // completion UPDATE; a zero-row claim means the funding is not ours. + tag, err := tx.Exec(context.Background(), ` + UPDATE till_sales SET status = 'failed', updated_at = NOW() + WHERE id = $1 AND status = 'pending'`, tillSaleID) + if err != nil { + return fmt.Errorf("failed to claim till sale for clawback: %w", err) + } + if tag.RowsAffected() == 0 { + return errTillSaleNotPending + } + + if action == "create" { + // A newly created card's transactions are scoped to THIS sale's + // funding (reference_type='till_sale' AND reference_id=sale id) — never + // a wholesale delete, which would destroy the value of a different + // idempotency-keyed top-up sale that funded the same card before this + // create resolved. Then remove the card itself. + if _, err := tx.Exec(context.Background(), `DELETE FROM gift_card_transactions WHERE gift_card_id = $1 AND reference_type = 'till_sale' AND reference_id = $2`, giftCardID, tillSaleID); err != nil { + return fmt.Errorf("failed to delete gift card transaction: %w", err) + } + if _, err := tx.Exec(context.Background(), `DELETE FROM gift_cards WHERE id = $1`, giftCardID); err != nil { + return fmt.Errorf("failed to delete gift card: %w", err) + } + // If the card was immediately redeemed to a user balance in this + // request, reverse that credit (guarded so it can never go negative). + if redeemToUserID != nil && *redeemToUserID != "" { + bTag, bErr := tx.Exec(context.Background(), ` + UPDATE user_giftcard_balances + SET balance = user_giftcard_balances.balance - $1, updated_at = NOW() + WHERE user_id = $2 AND balance >= $1 + `, amount, *redeemToUserID) + if bErr != nil { + return fmt.Errorf("failed to reverse redeemed gift card balance: %w", bErr) + } + if bTag.RowsAffected() == 0 { + log.Printf("CRITICAL: [SQUARE-WEBHOOK] ... MANUAL RECONCILIATION REQUIRED: create-with-redeem clawback for gift card %s could not fully reverse the £%.2f balance credited to user %s (balance < amount)", giftCardID, amount, *redeemToUserID) + } + } + } else { + // Top-up: subtract the amount back out of the card. The guard keeps + // amount_remaining from ever going negative in the pathological case + // where some of the top-up was already spent before the charge failed. + tag, err := tx.Exec(context.Background(), ` + UPDATE gift_cards + SET total_funds_added = total_funds_added - $1, + amount_remaining = amount_remaining - $1 + WHERE id = $2 AND amount_remaining >= $1 + `, amount, giftCardID) + if err != nil { + return fmt.Errorf("failed to reverse gift card top-up: %w", err) + } + if tag.RowsAffected() == 0 { + log.Printf("CRITICAL: [SQUARE-WEBHOOK] ... MANUAL RECONCILIATION REQUIRED: top-up %v on gift card %s could not be fully reversed (amount_remaining < top-up)", amount, giftCardID) + } + // Remove only this request's top-up transaction (reference_id = till + // sale) so prior sales' accounting on the same card is untouched. + if _, err := tx.Exec(context.Background(), ` + DELETE FROM gift_card_transactions + WHERE gift_card_id = $1 AND reference_type = 'till_sale' AND reference_id = $2 + `, giftCardID, tillSaleID); err != nil { + return fmt.Errorf("failed to delete gift card top-up transaction: %w", err) + } + } + + if err := tx.Commit(context.Background()); err != nil { + return fmt.Errorf("failed to commit clawback transaction: %w", err) + } + return nil } // handleRefundUpdated reconciles a Square Refund state change against the local -// refunds row. Idempotent (status-guarded UPDATE + event_id dedup). -func handleRefundUpdated(data json.RawMessage) { +// refunds row. Idempotent (status-guarded UPDATE + event_id dedup). A non-nil +// error means dispatch failed and the caller must NOT commit the dedup row. +func handleRefundUpdated(data json.RawMessage) error { var env squareWebhookData if err := json.Unmarshal(data, &env); err != nil { log.Printf("[SQUARE-WEBHOOK] refund.updated received (payload length=%d)", len(data)) - return + return nil } if env.ID == "" { log.Printf("[SQUARE-WEBHOOK] refund.updated received (payload length=%d)", len(data)) - return + return nil } var refund squareRefundPayload if !parseSquareObject(env.Object, "refund", &refund) || refund.ID == "" || refund.Status == "" { log.Printf("[SQUARE-WEBHOOK] refund.updated received (data.id=%s)", env.ID) - return + return nil } localStatus, terminal := squareRefundStatusToLocal(refund.Status) if !terminal { log.Printf("[SQUARE-WEBHOOK] refund.updated: square refund %s status %q is non-terminal — no local state change", refund.ID, refund.Status) - return + return nil } // COMPLETED may promote any non-completed row (incl. a sweep-failed refund // Square later shows complete) — the over-refund guard counts completed @@ -520,17 +720,18 @@ func handleRefundUpdated(data json.RawMessage) { tag, err := db.Conn.Exec(context.Background(), upd, refund.ID) if err != nil { log.Printf("[SQUARE-WEBHOOK] Failed to update refund %s to status %s: %v", refund.ID, localStatus, err) - return + return err } if tag.RowsAffected() > 0 { log.Printf("[SQUARE-WEBHOOK] refund.updated: square refund %s → local status %s", refund.ID, localStatus) } + return nil } // truncateDisputeReason caps a Square dispute reason at the disputes.reason -// VARCHAR(192) column width. An over-long reason would fail the INSERT — and -// because the event_id dedup row commits BEFORE dispatch, a failed insert -// silently drops the dispute (money-at-risk with no record). +// VARCHAR(192) column width. An over-long reason would fail the disputes +// INSERT; the handler treats that as a dispatch error (no dedup row, 5xx), so +// Square would retry forever — truncating lets the event succeed instead. func truncateDisputeReason(reason string) string { if len(reason) > 192 { return reason[:192] @@ -541,17 +742,18 @@ func truncateDisputeReason(reason string) string { // handleDisputeCreated records a newly opened dispute: inserts the disputes row // and surfaces a critical_payment_log admin notification so the owner sees the // chargeback in-app. Idempotent via ON CONFLICT (square_dispute_id) DO NOTHING -// plus the event_id dedup. -func handleDisputeCreated(data json.RawMessage) { +// plus the event_id dedup. A non-nil error means dispatch failed (no dedup row +// committed — Square retries). +func handleDisputeCreated(data json.RawMessage) error { var env squareWebhookData if err := json.Unmarshal(data, &env); err != nil { log.Printf("[SQUARE-WEBHOOK] dispute.created received (payload length=%d)", len(data)) - return + return nil } var dispute squareDisputePayload if !parseSquareObject(env.Object, "dispute", &dispute) || dispute.ID == "" { log.Printf("[SQUARE-WEBHOOK] dispute.created received (data.id=%s)", env.ID) - return + return nil } squarePaymentID := "" if dispute.DisputedPayment != nil { @@ -560,7 +762,7 @@ func handleDisputeCreated(data json.RawMessage) { paymentID, bookingID, paymentFound := findPaymentBySquareID(squarePaymentID) if !paymentFound { log.Printf("[SQUARE-WEBHOOK] dispute.created: no local payment for square payment %q — dispute %s not recorded", squarePaymentID, dispute.ID) - return + return nil } amount := squareMoneyToAmount(dispute.AmountMoney) tag, err := db.Conn.Exec(context.Background(), ` @@ -570,27 +772,29 @@ func handleDisputeCreated(data json.RawMessage) { `, dispute.ID, paymentID, amount, truncateDisputeReason(dispute.Reason)) if err != nil { log.Printf("[SQUARE-WEBHOOK] Failed to insert dispute %s: %v", dispute.ID, err) - return + return err } _ = tag insertCriticalPaymentNotification(bookingID) log.Printf("[SQUARE-WEBHOOK] CRITICAL: dispute %s created (amount %s, reason %q) for square payment %s — admin notified", dispute.ID, amount, dispute.Reason, squarePaymentID) + return nil } // handleDisputeStateUpdated applies a Square dispute state change to the local // disputes row (upsert — a state.updated may arrive before the created event), // and on a terminal loss marks the payment failed + raises CRITICAL. Won is -// logged only. Idempotent: the upsert converges to the same row. -func handleDisputeStateUpdated(data json.RawMessage) { +// logged only. Idempotent: the upsert converges to the same row. A non-nil +// error means dispatch failed (no dedup row committed — Square retries). +func handleDisputeStateUpdated(data json.RawMessage) error { var env squareWebhookData if err := json.Unmarshal(data, &env); err != nil { log.Printf("[SQUARE-WEBHOOK] dispute.state.updated received (payload length=%d)", len(data)) - return + return nil } var dispute squareDisputePayload if !parseSquareObject(env.Object, "dispute", &dispute) || dispute.ID == "" { log.Printf("[SQUARE-WEBHOOK] dispute.state.updated received (data.id=%s)", env.ID) - return + return nil } localStatus := squareDisputeStateToLocal(dispute.State) amount := squareMoneyToAmount(dispute.AmountMoney) @@ -605,7 +809,7 @@ func handleDisputeStateUpdated(data json.RawMessage) { paymentID, bookingID = findPaymentByDisputeID(dispute.ID) if paymentID == "" { log.Printf("[SQUARE-WEBHOOK] dispute.state.updated: no local payment for dispute %s (square payment %q) — cannot record state %s", dispute.ID, squarePaymentID, dispute.State) - return + return nil } } @@ -618,12 +822,14 @@ func handleDisputeStateUpdated(data json.RawMessage) { `, dispute.ID, paymentID, localStatus, amount, truncateDisputeReason(dispute.Reason)) if err != nil { log.Printf("[SQUARE-WEBHOOK] Failed to update dispute %s to state %s: %v", dispute.ID, dispute.State, err) - return + return err } switch localStatus { case "lost": - markPaymentFailed(paymentID) + if err := markPaymentFailed(paymentID); err != nil { + return err + } insertCriticalPaymentNotification(bookingID) log.Printf("[SQUARE-WEBHOOK] CRITICAL: dispute %s LOST — payment %s marked failed; admin notified", dispute.ID, paymentID) case "won": @@ -631,32 +837,35 @@ func handleDisputeStateUpdated(data json.RawMessage) { default: log.Printf("[SQUARE-WEBHOOK] dispute %s state → %s (status %s)", dispute.ID, dispute.State, localStatus) } + return nil } // handleDisputeEvidence logs evidence submissions/removals. Evidence does not // change the dispute's local status, so it is informational only. -func handleDisputeEvidence(data json.RawMessage) { +func handleDisputeEvidence(data json.RawMessage) error { var env squareWebhookData if err := json.Unmarshal(data, &env); err != nil { log.Printf("[SQUARE-WEBHOOK] dispute evidence event received (payload length=%d)", len(data)) - return + return nil } var dispute squareDisputePayload if !parseSquareObject(env.Object, "dispute", &dispute) || dispute.ID == "" { log.Printf("[SQUARE-WEBHOOK] dispute evidence event received (data.id=%s)", env.ID) - return + return nil } log.Printf("[SQUARE-WEBHOOK] dispute evidence event for dispute %s (state %s)", dispute.ID, dispute.State) + return nil } // handleTerminalCheckout logs terminal checkout lifecycle events. Terminal // checkout state is owned by the poll/sweep handlers (handlers/payments/), // which fetch the authoritative status from Square — no state mutation here. -func handleTerminalCheckout(data json.RawMessage) { +func handleTerminalCheckout(data json.RawMessage) error { var env squareWebhookData if err := json.Unmarshal(data, &env); err != nil { log.Printf("[SQUARE-WEBHOOK] terminal.checkout event received (payload length=%d)", len(data)) - return + return nil } log.Printf("[SQUARE-WEBHOOK] terminal.checkout event received (data.id=%s)", env.ID) + return nil } diff --git a/backend/handlers/webhooks/webhooks_dispatch_error_test.go b/backend/handlers/webhooks/webhooks_dispatch_error_test.go new file mode 100644 index 0000000..5eef0fc --- /dev/null +++ b/backend/handlers/webhooks/webhooks_dispatch_error_test.go @@ -0,0 +1,83 @@ +//go:build test + +package webhooks + +// Regression test for the at-least-once webhook delivery contract: a dispatch +// error must NOT commit the dedup row, so Square's retry is not swallowed. + +import ( + "encoding/json" + "net/http" + "testing" +) + +// TestHandleSquareWebhook_DispatchError_NoDedup_RetryReDispatches verifies the +// handler's dispatch-first/commit-dedup-after order: when the dispatch handler +// errors (here the disputes INSERT fails because the amount overflows +// NUMERIC(10,2)), the handler returns 503 and records NO dedup row — so a +// replayed delivery (Square's retry) is re-dispatched, not 200-skipped. Before +// the at-least-once ordering this invariant silently dropped events on any +// transient dispatch failure. +func TestHandleSquareWebhook_DispatchError_NoDedup_RetryReDispatches(t *testing.T) { + const squarePaymentID = "sqp_dispatch_err" + _ = createWebhookTestPayment(t, squarePaymentID, "completed") + + // amount_money.amount = 9,999,999,999,999,999 pence → £99,999,999,999,999.99, + // far beyond disputes.amount NUMERIC(10,2) — the INSERT fails. + overflowEvent := SquareWebhookEvent{ + Type: "dispute.created", + EventID: "evt_dispatch_err_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "dispute", + "id": "dts_dispatch_err_1", + "object": { + "dispute": { + "id": "dts_dispatch_err_1", + "state": "UNDER_REVIEW", + "amount_money": {"amount": 9999999999999999, "currency": "GBP"}, + "reason": "OVERFLOW", + "disputed_payment": {"payment_id": "` + squarePaymentID + `"} + } + } + }`), + } + + w1 := deliverWebhook(t, overflowEvent) + if w1.Code != http.StatusServiceUnavailable { + t.Fatalf("expected 503 on dispatch error, got %d: %s", w1.Code, w1.Body.String()) + } + if n := countWebhookEvents(t, overflowEvent.EventID); n != 0 { + t.Fatalf("expected NO dedup row after a dispatch error (Square must retry), got %d", n) + } + + // Square retries with a well-formed payload under the SAME event_id. + retryEvent := SquareWebhookEvent{ + Type: "dispute.created", + EventID: "evt_dispatch_err_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "dispute", + "id": "dts_dispatch_err_1", + "object": { + "dispute": { + "id": "dts_dispatch_err_1", + "state": "UNDER_REVIEW", + "amount_money": {"amount": 1234, "currency": "GBP"}, + "reason": "NO_KNOWLEDGE", + "disputed_payment": {"payment_id": "` + squarePaymentID + `"} + } + } + }`), + } + w2 := deliverWebhook(t, retryEvent) + if w2.Code != http.StatusOK { + t.Fatalf("expected retry 200, got %d: %s", w2.Code, w2.Body.String()) + } + if got := getDisputeStatus(t, "dts_dispatch_err_1"); got != "open" { + t.Errorf("expected retried dispute to be recorded with status 'open', got %q", got) + } + if n := countWebhookEvents(t, retryEvent.EventID); n != 1 { + t.Errorf("expected exactly 1 dedup row after the successful retry, got %d", n) + } +} diff --git a/backend/handlers/webhooks/webhooks_state_test.go b/backend/handlers/webhooks/webhooks_state_test.go index 5430eb9..6975626 100644 --- a/backend/handlers/webhooks/webhooks_state_test.go +++ b/backend/handlers/webhooks/webhooks_state_test.go @@ -11,6 +11,7 @@ import ( "testing" "crussell/db" + "crussell/testutils/fixtures" ) // ============================================================================= @@ -99,6 +100,157 @@ func deliverWebhook(t *testing.T, event SquareWebhookEvent) *httptest.ResponseRe return makeWebhookRequest(body, sig, context.Background()) } +// createWebhookTestGiftCardAndSale seeds a pending gift-card till sale tied to +// a Square payment id and returns the sale id and gift card id. When +// cardCreatedAt == saleCreatedAt the sale created the card (is_create → action +// 'create'); otherwise the card pre-exists (action 'topup'). cardAmount is the +// card's starting total_funds_added/amount_remaining. +func createWebhookTestGiftCardAndSale(t *testing.T, squarePaymentID, cardCreatedAt, saleCreatedAt string, cardAmount float64) (saleID, giftCardID string) { + t.Helper() + adminID, err := fixtures.CreateTestAdminUser(db.Conn) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + if err := db.Conn.QueryRow(context.Background(), ` + INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by, is_inventory, voucher_type_at_purchase, created_at) + VALUES ($1, $1, $2, FALSE, 'SPV', $3::timestamptz) + RETURNING id + `, cardAmount, adminID, cardCreatedAt).Scan(&giftCardID); err != nil { + t.Fatalf("failed to create gift card: %v", err) + } + 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 ('gift_card', $1, 'webhook clawback test', 1, 40.00, 40.00, 'online_square', 'pending', + $2, $3, $4::timestamptz, NOW()) + RETURNING id + `, giftCardID, squarePaymentID, adminID, saleCreatedAt).Scan(&saleID); err != nil { + t.Fatalf("failed to create pending till sale: %v", err) + } + return saleID, giftCardID +} + +// deliverPaymentUpdatedFailed dispatches a payment.updated webhook carrying a +// definitively FAILED Square status for the given Square payment id. +func deliverPaymentUpdatedFailed(t *testing.T, squarePaymentID string) *httptest.ResponseRecorder { + t.Helper() + event := SquareWebhookEvent{ + Type: "payment.updated", + EventID: "evt_" + squarePaymentID, + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "payment", + "id": "` + squarePaymentID + `", + "object": { + "payment": { + "id": "` + squarePaymentID + `", + "status": "FAILED" + } + } + }`), + } + return deliverWebhook(t, event) +} + +func getTillSaleStatus(t *testing.T, id string) string { + t.Helper() + var status string + if err := db.Conn.QueryRow(context.Background(), + "SELECT status FROM till_sales WHERE id = $1", id).Scan(&status); err != nil { + t.Fatalf("failed to read till_sales status: %v", err) + } + return status +} + +func getGiftCardFunding(t *testing.T, id string) (totalFundsAdded, amountRemaining float64) { + t.Helper() + if err := db.Conn.QueryRow(context.Background(), + "SELECT total_funds_added, amount_remaining FROM gift_cards WHERE id = $1", id).Scan(&totalFundsAdded, &amountRemaining); err != nil { + t.Fatalf("failed to read gift card funding: %v", err) + } + return totalFundsAdded, amountRemaining +} + +// ============================================================================= +// Till-sale gift-card clawback — payment.updated FAILED/CANCELED +// ============================================================================= + +// TestWebhook_PaymentUpdated_Failed_ClawsBackCreatedCard verifies that a +// definitively-failed Square charge (FAILED) claws back the gift-card funding +// of a pending till sale that CREATED the card: the card and its purchase +// transaction are deleted and the sale is marked failed, exactly as the sweep +// does. +func TestWebhook_PaymentUpdated_Failed_ClawsBackCreatedCard(t *testing.T) { + const squarePaymentID = "sqp_clawback_create" + const cardCreatedAt = "2025-01-01T00:00:00Z" + saleID, giftCardID := createWebhookTestGiftCardAndSale(t, squarePaymentID, cardCreatedAt, cardCreatedAt, 40.00) + + 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) + } + if exists := giftCardExists(t, giftCardID); exists { + t.Error("expected created gift card to be deleted by the clawback") + } +} + +// TestWebhook_PaymentUpdated_Failed_ClawsBackTopup verifies the top-up +// clawback for a pre-existing card: the sale's funding is subtracted back out +// of the card and the sale is marked failed. +func TestWebhook_PaymentUpdated_Failed_ClawsBackTopup(t *testing.T) { + const squarePaymentID = "sqp_clawback_topup" + saleID, giftCardID := createWebhookTestGiftCardAndSale(t, squarePaymentID, "2025-01-01T00:00:00Z", "2025-01-02T00:00:00Z", 60.00) + + 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) + } + total, remaining := getGiftCardFunding(t, giftCardID) + if total != 20.00 || remaining != 20.00 { + t.Errorf("expected top-up clawback to leave £20.00 on the card, got total=%v remaining=%v", total, remaining) + } +} + +// TestWebhook_PaymentUpdated_Failed_AlreadyResolved_Skipped verifies the +// clawback skips without error when the till sale is already resolved (not +// pending): the webhook still acknowledges 200 and leaves the terminal state +// untouched. +func TestWebhook_PaymentUpdated_Failed_AlreadyResolved_Skipped(t *testing.T) { + const squarePaymentID = "sqp_clawback_resolved" + saleID, giftCardID := createWebhookTestGiftCardAndSale(t, squarePaymentID, "2025-01-01T00:00:00Z", "2025-01-01T00:00:00Z", 40.00) + if _, err := db.Conn.Exec(context.Background(), + "UPDATE till_sales SET status = 'completed', updated_at = NOW() WHERE id = $1", saleID); err != nil { + t.Fatalf("failed to resolve till sale: %v", err) + } + + 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 != "completed" { + t.Errorf("expected resolved till sale to stay 'completed', got %q", got) + } + if total, remaining := getGiftCardFunding(t, giftCardID); total != 40.00 || remaining != 40.00 { + t.Errorf("expected gift card untouched when the sale is already resolved, got total=%v remaining=%v", total, remaining) + } +} + +func giftCardExists(t *testing.T, id string) bool { + t.Helper() + var n int + if err := db.Conn.QueryRow(context.Background(), + "SELECT COUNT(*) FROM gift_cards WHERE id = $1", id).Scan(&n); err != nil { + t.Fatalf("failed to count gift cards: %v", err) + } + return n > 0 +} + // ============================================================================= // Dispute handling — dispute.created // ============================================================================= diff --git a/backend/handlers/webhooks/webhooks_test.go b/backend/handlers/webhooks/webhooks_test.go index 92163e8..4baead4 100644 --- a/backend/handlers/webhooks/webhooks_test.go +++ b/backend/handlers/webhooks/webhooks_test.go @@ -499,7 +499,10 @@ func TestHandleSquareWebhook_DedupDispatchOnce(t *testing.T) { // TestHandleSquareWebhook_DedupPersistsAcrossRestart simulates a restart: the // event was handled by a previous process whose in-memory cache is gone, but -// the dedup row survived in the DB — the replayed delivery must be skipped. +// the dedup row survived in the DB. The dedup row now commits AFTER dispatch +// (at-least-once delivery), so a replayed delivery is re-dispatched — the +// idempotent status-guarded handlers are a no-op the second time — and is +// acknowledged 200 without duplicating the persisted dedup row. func TestHandleSquareWebhook_DedupPersistsAcrossRestart(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", @@ -527,8 +530,8 @@ func TestHandleSquareWebhook_DedupPersistsAcrossRestart(t *testing.T) { if w.Body.String() != "ok" { t.Errorf("expected body 'ok', got %q", w.Body.String()) } - if out := buf.String(); strings.Contains(out, "Received event: payment.updated") { - t.Errorf("expected replayed event to skip dispatch, got:\n%s", out) + if out := buf.String(); !strings.Contains(out, "Received event: payment.updated") { + t.Errorf("expected replayed event to be re-dispatched (at-least-once), got:\n%s", out) } if n := countWebhookEvents(t, event.EventID); n != 1 { t.Errorf("expected still exactly 1 dedup row, got %d", n) diff --git a/backend/internal/square/square.go b/backend/internal/square/square.go index 95f5769..3ddc280 100644 --- a/backend/internal/square/square.go +++ b/backend/internal/square/square.go @@ -35,8 +35,8 @@ func (p *ProdClient) GetPayment(ctx context.Context, paymentID string) (*Payment return getPaymentHTTP(ctx, paymentID) } -func (p *ProdClient) ReplayPaymentByKey(ctx context.Context, idempotencyKey string, amount int64) (*PaymentResult, error) { - return replayPaymentByKeyHTTP(ctx, idempotencyKey, amount) +func (p *ProdClient) ReplayPaymentByKey(ctx context.Context, snapshotJSON []byte) (*PaymentResult, error) { + return replayPaymentByKeyHTTP(ctx, snapshotJSON) } func (p *ProdClient) CreateCustomer(ctx context.Context, name, email string) (*CustomerResult, error) { diff --git a/backend/internal/square/square_dev.go b/backend/internal/square/square_dev.go index e230880..5b0cdfd 100644 --- a/backend/internal/square/square_dev.go +++ b/backend/internal/square/square_dev.go @@ -6,6 +6,7 @@ import ( "context" "crussell/clock" "crypto/sha256" + "encoding/json" "errors" "fmt" "log" @@ -29,9 +30,11 @@ func mockSleep(d time.Duration) { type MockClient struct { mu sync.RWMutex cards map[string]map[string]*CardOnFile + cardByToken map[string]*CardOnFile // ccof: token (CardOnFile.CardID) → the saved card, for replay-by-key rescue checkouts map[string]*CheckoutResult payments map[string]*PaymentResult paymentByKey map[string]*PaymentResult + paymentSource map[string]string // idempotency key → the source_id the original CreatePayment used refunds map[string]*RefundResult refundByKey map[string]*RefundResult customers map[string]*CustomerResult @@ -76,8 +79,8 @@ func (d *devProdClient) GetCheckout(ctx context.Context, checkoutID string) (*Pa func (d *devProdClient) GetPayment(ctx context.Context, paymentID string) (*PaymentResult, error) { return getPaymentHTTP(ctx, paymentID) } -func (d *devProdClient) ReplayPaymentByKey(ctx context.Context, idempotencyKey string, amount int64) (*PaymentResult, error) { - return replayPaymentByKeyHTTP(ctx, idempotencyKey, amount) +func (d *devProdClient) ReplayPaymentByKey(ctx context.Context, snapshotJSON []byte) (*PaymentResult, error) { + return replayPaymentByKeyHTTP(ctx, snapshotJSON) } func (d *devProdClient) CreateCustomer(ctx context.Context, name, email string) (*CustomerResult, error) { return createCustomerHTTP(ctx, name, email) @@ -117,14 +120,16 @@ func NewDevClient() SquareClient { } log.Println("[SQUARE-MOCK] Using in-memory mock client") return &MockClient{ - cards: make(map[string]map[string]*CardOnFile), - checkouts: make(map[string]*CheckoutResult), - payments: make(map[string]*PaymentResult), - paymentByKey: make(map[string]*PaymentResult), - refunds: make(map[string]*RefundResult), - refundByKey: make(map[string]*RefundResult), - customers: make(map[string]*CustomerResult), - completed: make(map[string]*PaymentResult), + cards: make(map[string]map[string]*CardOnFile), + cardByToken: make(map[string]*CardOnFile), + checkouts: make(map[string]*CheckoutResult), + payments: make(map[string]*PaymentResult), + paymentByKey: make(map[string]*PaymentResult), + paymentSource: make(map[string]string), + refunds: make(map[string]*RefundResult), + refundByKey: make(map[string]*RefundResult), + customers: make(map[string]*CustomerResult), + completed: make(map[string]*PaymentResult), } } @@ -256,6 +261,7 @@ func (m *MockClient) CreatePayment(ctx context.Context, req CreatePaymentReq) (* m.payments[result.SquarePayID] = result if req.IdempotencyKey != "" { m.paymentByKey[req.IdempotencyKey] = result + m.paymentSource[req.IdempotencyKey] = req.SourceID } log.Printf("[SQUARE-MOCK] Payment created: id=%s, status=%s, amount=%d, fees=%d", paymentID, status, amount, fees) return result, nil @@ -404,22 +410,72 @@ func (m *MockClient) GetPayment(ctx context.Context, paymentID string) (*Payment return payment, nil } -// ReplayPaymentByKey mirrors the real client's replay-by-key reconcile -// (POST /v2/payments with the same idempotency key): the dedup map returns the -// ORIGINAL payment for a retained key — never a second charge — and an unknown -// key is rejected with ErrReplayKeyNotRetained, exactly as the real client -// rejects the synthetic probe source token it sends for an unknown key. -func (m *MockClient) ReplayPaymentByKey(ctx context.Context, idempotencyKey string, amount int64) (*PaymentResult, error) { - log.Printf("[SQUARE-MOCK] ReplayPaymentByKey: key=%s", idempotencyKey) +// ReplayPaymentByKey mirrors the real client's IDENTICAL-body replay-by-key +// reconcile (POST /v2/payments with the full stored request snapshot): a +// retained key with the matching stored source returns the ORIGINAL payment +// (never a second charge); a retained key with a DIFFERENT source returns a +// structured 400 IDEMPOTENCY_KEY_REUSED — exactly what Square returns when an +// idempotency key is reused with a different request body (the stored source +// must never differ from the original, so the sweep treats it as ambiguous); +// an unknown key makes Square attempt a real charge with the stored source: a +// still-valid ccof: saved-card token CHARGES successfully (returning a new +// COMPLETED payment the sweep rescues), while a spent/expired cnon: nonce is +// rejected with a 4xx — surfaced as ErrReplayKeyNotRetained. +func (m *MockClient) ReplayPaymentByKey(ctx context.Context, snapshotJSON []byte) (*PaymentResult, error) { + var req CreatePaymentReq + if err := json.Unmarshal(snapshotJSON, &req); err != nil { + return nil, fmt.Errorf("square: replay-by-key cannot parse stored request snapshot: %w", err) + } + log.Printf("[SQUARE-MOCK] ReplayPaymentByKey: key=%s, source=%s", req.IdempotencyKey, tokenPrefix(req.SourceID)) m.mu.RLock() - defer m.mu.RUnlock() + existing, ok := m.paymentByKey[req.IdempotencyKey] + storedSource := m.paymentSource[req.IdempotencyKey] + m.mu.RUnlock() - if existing, ok := m.paymentByKey[idempotencyKey]; ok { - log.Printf("[SQUARE-MOCK] ReplayPaymentByKey dedup hit: key=%s → id=%s", idempotencyKey, existing.ID) + if ok { + if storedSource != "" && storedSource != req.SourceID { + // Same key, different body — Square's documented IDEMPOTENCY_KEY_REUSED + // rejection. A data bug (the stored source differs from the original + // charge), NOT proof the charge never happened. + return nil, &squareAPIError{ + Code: "IDEMPOTENCY_KEY_REUSED", + Detail: "idempotency key was reused with a different request body", + StatusCode: http.StatusBadRequest, + err: fmt.Errorf("square: idempotency key %s reused with a different source_id", req.IdempotencyKey), + } + } + log.Printf("[SQUARE-MOCK] ReplayPaymentByKey dedup hit: key=%s → id=%s", req.IdempotencyKey, existing.ID) return existing, nil } - return nil, fmt.Errorf("%w: Square has no payment under idempotency key", ErrReplayKeyNotRetained) + + // Unknown key — mirror real Square: it attempts a real charge with the + // stored source. A still-valid ccof: saved-card token charges successfully + // (the sweep then rescues the row); a spent/expired cnon: nonce (or any + // unchargeable source) is rejected with a definitive 4xx. + if strings.HasPrefix(req.SourceID, "ccof:") { + m.mu.RLock() + _, cardOK := m.cardByToken[req.SourceID] + m.mu.RUnlock() + if !cardOK { + // The saved card is not in the mock ledger — mirror real Square + // rejecting a deleted/disabled card with a definitive 4xx. + return nil, fmt.Errorf("%w: Square has no saved card %s to charge", ErrReplayKeyNotRetained, tokenPrefix(req.SourceID)) + } + if req.Currency == "" { + req.Currency = gbpCurrency + } + pr, err := m.CreatePayment(ctx, req) + if err != nil { + if replayErrorProvesNoCharge(err) { + return nil, fmt.Errorf("%w: %v", ErrReplayKeyNotRetained, err) + } + return nil, err + } + log.Printf("[SQUARE-MOCK] ReplayPaymentByKey charged saved card for unknown key: key=%s → id=%s", req.IdempotencyKey, pr.ID) + return pr, nil + } + return nil, fmt.Errorf("%w: Square has no payment under idempotency key (HTTP 400: source rejected)", ErrReplayKeyNotRetained) } func (m *MockClient) RefundPayment(ctx context.Context, req RefundPaymentReq) (*RefundResult, error) { @@ -568,6 +624,7 @@ func (m *MockClient) CreateCardOnFile(ctx context.Context, userID, cardToken, cu CreatedAt: now.Format(time.RFC3339), } m.cards[userID][cardID] = card + m.cardByToken[card.CardID] = card log.Printf("[SQUARE-MOCK] Card created: id=%s, brand=%s, last4=%s", cardID, card.Brand, card.Last4) return card, nil } diff --git a/backend/internal/square/square_dev_test.go b/backend/internal/square/square_dev_test.go index d5d7f5d..497d122 100644 --- a/backend/internal/square/square_dev_test.go +++ b/backend/internal/square/square_dev_test.go @@ -5,6 +5,7 @@ package square import ( "bytes" "context" + "encoding/json" "errors" "fmt" "log" @@ -19,6 +20,15 @@ import ( "github.com/stretchr/testify/require" ) +// replaySnapshotReq marshals a CreatePaymentReq into the stored +// square_request_snapshot shape (domain JSON) that ReplayPaymentByKey parses. +func replaySnapshotReq(t *testing.T, req CreatePaymentReq) []byte { + t.Helper() + snap, err := json.Marshal(req) + require.NoError(t, err) + return snap +} + func TestDevClient_CreatePayment_ReturnsCompleted(t *testing.T) { client := NewDevClient().(*MockClient) @@ -672,6 +682,157 @@ func TestDevClient_GetCardsOnFile_Empty(t *testing.T) { assert.Empty(t, cards) } +func TestDevClient_ReplayPaymentByKey_MatchingSource_ReturnsOriginal(t *testing.T) { + // Identical-body replay contract: a retained key with the MATCHING stored + // source returns the ORIGINAL payment (Square's idempotency guarantee) — + // never a second charge and never IDEMPOTENCY_KEY_REUSED. + client := NewDevClient().(*MockClient) + ctx := context.Background() + + orig, err := client.CreatePayment(ctx, CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "cnon:test-card", + IdempotencyKey: "replay-match-key", + }) + require.NoError(t, err) + + got, err := client.ReplayPaymentByKey(ctx, replaySnapshotReq(t, CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "cnon:test-card", + IdempotencyKey: "replay-match-key", + })) + require.NoError(t, err) + assert.Equal(t, orig.ID, got.ID, "identical-body replay must return the original payment") +} + +func TestDevClient_ReplayPaymentByKey_SourceMismatch_ReturnsKeyReused(t *testing.T) { + // Identical-body replay contract: reusing a retained key with a DIFFERENT + // source is Square's documented IDEMPOTENCY_KEY_REUSED rejection — a data + // bug, NOT proof the charge never happened. The mock must carry the + // structured code so ErrorCode(err) can read it (the sweep treats it as + // ambiguous). + client := NewDevClient().(*MockClient) + ctx := context.Background() + + _, err := client.CreatePayment(ctx, CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "cnon:test-card", + IdempotencyKey: "replay-mismatch-key", + }) + require.NoError(t, err) + + _, err = client.ReplayPaymentByKey(ctx, replaySnapshotReq(t, CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "cnon:different", + IdempotencyKey: "replay-mismatch-key", + })) + require.Error(t, err) + assert.Equal(t, "IDEMPOTENCY_KEY_REUSED", ErrorCode(err), "source-mismatch replay must carry IDEMPOTENCY_KEY_REUSED") + assert.Equal(t, http.StatusBadRequest, ErrorStatusCode(err)) + assert.False(t, errors.Is(err, ErrReplayKeyNotRetained), "IDEMPOTENCY_KEY_REUSED is NOT proof the charge never happened") +} + +func TestDevClient_ReplayPaymentByKey_UnknownKey_NotRetained(t *testing.T) { + // Identical-body replay contract: an unknown key makes Square attempt a + // real charge with the (expired/used) cnon: nonce, which is rejected with a + // 4xx — surfaced as ErrReplayKeyNotRetained (proof the charge never + // happened). + client := NewDevClient().(*MockClient) + ctx := context.Background() + + _, err := client.ReplayPaymentByKey(ctx, replaySnapshotReq(t, CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "cnon:test-card", + IdempotencyKey: "key-never-seen", + })) + require.Error(t, err) + assert.True(t, errors.Is(err, ErrReplayKeyNotRetained), "unknown-key cnon replay must surface ErrReplayKeyNotRetained, got %v", err) +} + +func TestDevClient_ReplayPaymentByKey_UnknownKey_CcofSavedCard_ChargesAndRescues(t *testing.T) { + // B3 dev/prod parity: an unknown key with a STILL-VALID ccof: saved-card + // token makes real Square attempt a REAL charge that succeeds — the sweep + // must RESCUE such rows, never fail them. The mock mirrors this by looking + // up the saved card and creating a new COMPLETED payment under the key. + client := NewDevClient().(*MockClient) + ctx := context.Background() + + card, err := client.CreateCardOnFile(ctx, "user-replay-rescue", "cnon:test-token", "cus_replay123") + require.NoError(t, err) + + snapshot := replaySnapshotReq(t, CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: card.CardID, + CustomerID: "cus_replay123", + IdempotencyKey: "key-never-seen-ccof", + }) + got, err := client.ReplayPaymentByKey(ctx, snapshot) + require.NoError(t, err) + assert.Equal(t, "COMPLETED", got.Status, "a still-valid ccof: source must charge successfully on an unknown key") + assert.Equal(t, int64(5000), got.Amount) + assert.Equal(t, "ON_FILE", got.EntryMethod) + + // The charge must be recorded under the key so a later identical replay + // returns the SAME payment (Square's dedup) instead of charging twice. + got2, err := client.ReplayPaymentByKey(ctx, snapshot) + require.NoError(t, err) + assert.Equal(t, got.ID, got2.ID, "a replayed ccof: charge under the same key must dedup, never charge twice") +} + +func TestDevClient_ReplayPaymentByKey_UnknownKey_UnregisteredCcof_NotRetained(t *testing.T) { + // A ccof: token that is NOT in the saved-card ledger mirrors real Square + // rejecting a deleted/disabled card with a definitive 4xx — the charge + // never happened. + client := NewDevClient().(*MockClient) + ctx := context.Background() + + _, err := client.ReplayPaymentByKey(ctx, replaySnapshotReq(t, CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "ccof:never-registered", + CustomerID: "cus_replay123", + IdempotencyKey: "key-never-seen-ccof-deleted", + })) + require.Error(t, err) + assert.True(t, errors.Is(err, ErrReplayKeyNotRetained), "an unregistered ccof: token must surface ErrReplayKeyNotRetained, got %v", err) +} + +func TestDevClient_ReplayPaymentByKey_DedupSourceTracked(t *testing.T) { + // The mock must record the source used by each CreatePayment so a later + // identical-body replay can verify the source matches (the "works in dev == + // works in prod" guarantee for the reconcile sweep). + client := NewDevClient().(*MockClient) + ctx := context.Background() + + _, err := client.CreatePayment(ctx, CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "cnon:dedup-src", + IdempotencyKey: "replay-dedup-key", + }) + require.NoError(t, err) + + client.mu.RLock() + stored := client.paymentSource["replay-dedup-key"] + client.mu.RUnlock() + assert.Equal(t, "cnon:dedup-src", stored, "the source of each keyed payment must be stored for replay parity") + + got, err := client.ReplayPaymentByKey(ctx, replaySnapshotReq(t, CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "cnon:dedup-src", + IdempotencyKey: "replay-dedup-key", + })) + require.NoError(t, err) + assert.NotEmpty(t, got.ID) +} + func TestDevClient_GetCheckout_StillPending(t *testing.T) { client := NewDevClient().(*MockClient) client.HoldCheckouts = true diff --git a/backend/internal/square/square_http_client.go b/backend/internal/square/square_http_client.go index 4109cb1..01bd6cf 100644 --- a/backend/internal/square/square_http_client.go +++ b/backend/internal/square/square_http_client.go @@ -41,21 +41,17 @@ const ( // Handlers log these errors verbatim, so echoing more than a snippet risks // leaking PII that Square may have mirrored from the request. maxErrorBody = 500 - - // probePaymentSourceID is the synthetic Square source token carried by the - // sweep's replay-by-key reconcile (ReplayPaymentByKey). It uses the cnon: - // prefix so it passes this client's PCI token validation (isTokenLike), but - // it is NOT a real Square-issued nonce and can never be processed into a - // charge. When the replayed idempotency key is unknown at Square, Square - // therefore definitively rejects the request instead of creating a new - // payment — the replay can never charge a customer. - probePaymentSourceID = "cnon:sqr-reconcile-probe" ) // --------------------------------------------------------------------------- // HTTP client — shared by ProdClient (!dev) and devProdClient (dev). // --------------------------------------------------------------------------- +// gbpCurrency is the currency sent in every Square money amount. UK-only app, +// so GBP is the only currency ever used; the named constant keeps the wire +// bodies (including the identical-body replay) consistent. +const gbpCurrency = "GBP" + type httpClient struct { baseURL string token string @@ -458,6 +454,19 @@ func createPaymentHTTPWithClient(ctx context.Context, req CreatePaymentReq, hc * if !isTokenLike(req.SourceID) { return nil, fmt.Errorf("square: invalid card token %s — use a card nonce (cnon:xxx) or card ID (ccof:xxx)", tokenPrefix(req.SourceID)) } + var resp sqCreatePaymentResponse + if err := hc.doJSON(ctx, http.MethodPost, "/v2/payments", buildCreatePaymentBody(req, hc), &resp); err != nil { + return nil, err + } + return paymentFromSquare(&resp.Payment), nil +} + +// buildCreatePaymentBody converts a CreatePaymentReq into the exact POST +// /v2/payments wire body. Shared by createPaymentHTTPWithClient (the original +// charge) and replayPaymentByKeyHTTPWithClient (the identical-body replay), so +// a charge replayed from the stored snapshot produces BYTE-IDENTICAL JSON to +// the original — Square's idempotency dedup compares the full request body. +func buildCreatePaymentBody(req CreatePaymentReq, hc *httpClient) sqCreatePaymentRequest { body := sqCreatePaymentRequest{ SourceID: req.SourceID, IdempotencyKey: req.IdempotencyKey, @@ -473,11 +482,7 @@ func createPaymentHTTPWithClient(ctx context.Context, req CreatePaymentReq, hc * if req.TipMoney != nil { body.TipMoney = &sqMoney{Amount: *req.TipMoney, Currency: req.Currency} } - var resp sqCreatePaymentResponse - if err := hc.doJSON(ctx, http.MethodPost, "/v2/payments", body, &resp); err != nil { - return nil, err - } - return paymentFromSquare(&resp.Payment), nil + return body } func createCheckoutHTTP(ctx context.Context, req CreateCheckoutReq) (*CheckoutResult, error) { @@ -572,25 +577,38 @@ func getPaymentHTTPWithClient(ctx context.Context, paymentID string, hc *httpCli return paymentFromSquare(&resp.Payment), nil } -func replayPaymentByKeyHTTP(ctx context.Context, idempotencyKey string, amount int64) (*PaymentResult, error) { - return replayPaymentByKeyHTTPWithClient(ctx, idempotencyKey, amount, newHTTPClient()) +func replayPaymentByKeyHTTP(ctx context.Context, snapshotJSON []byte) (*PaymentResult, error) { + return replayPaymentByKeyHTTPWithClient(ctx, snapshotJSON, newHTTPClient()) } -// replayPaymentByKeyHTTPWithClient re-issues POST /v2/payments with the same -// idempotency key and amount. Square's documented idempotency behavior returns -// the ORIGINAL payment object when the key is reused — never a second charge. -// The body's source_id is probePaymentSourceID, a synthetic token that cannot -// be processed into a charge, so a key Square does not retain makes Square -// reject the request instead of creating a new payment; that rejection is -// surfaced as ErrReplayKeyNotRetained (proof the charge never happened). -func replayPaymentByKeyHTTPWithClient(ctx context.Context, idempotencyKey string, amount int64, hc *httpClient) (*PaymentResult, error) { - body := sqCreatePaymentRequest{ - SourceID: probePaymentSourceID, - IdempotencyKey: idempotencyKey, - AmountMoney: sqMoney{Amount: amount, Currency: "GBP"}, +// replayPaymentByKeyHTTPWithClient re-issues POST /v2/payments with an +// IDENTICAL body to the original charge: the square_request_snapshot stored on +// the pending row is the verbatim CreatePaymentReq JSON captured at charge +// time, and buildCreatePaymentBody reproduces the exact wire request the +// original charge sent (source_id, key, amount, customer_id, reference_id, +// note, buyer_email_address, verification_token, tip, location). Square's +// idempotency guarantee returns the ORIGINAL payment for a retained key (never +// a second charge); a key Square no longer retains makes Square attempt a real +// charge with the (expired/used) source, which Square rejects with a definitive +// 4xx — surfaced as ErrReplayKeyNotRetained (proof the charge never happened). +// A replay body missing fields the original charge carried would return +// IDEMPOTENCY_KEY_REUSED for a RETAINED key and strand the row pending forever, +// so the snapshot is never reconstructed from partial row data. +func replayPaymentByKeyHTTPWithClient(ctx context.Context, snapshotJSON []byte, hc *httpClient) (*PaymentResult, error) { + var req CreatePaymentReq + if err := json.Unmarshal(snapshotJSON, &req); err != nil { + // An unparsable snapshot must never look like proof of no charge — the + // sweep leaves such rows pending for manual reconciliation. + return nil, fmt.Errorf("square: replay-by-key cannot parse stored request snapshot: %w", err) + } + if req.SourceID == "" || req.IdempotencyKey == "" { + return nil, fmt.Errorf("square: replay-by-key snapshot missing source_id/idempotency_key") + } + if req.Currency == "" { + req.Currency = gbpCurrency } var resp sqCreatePaymentResponse - if err := hc.doJSON(ctx, http.MethodPost, "/v2/payments", body, &resp); err != nil { + if err := hc.doJSON(ctx, http.MethodPost, "/v2/payments", buildCreatePaymentBody(req, hc), &resp); err != nil { if replayErrorProvesNoCharge(err) { return nil, fmt.Errorf("%w: %v", ErrReplayKeyNotRetained, err) } @@ -600,32 +618,30 @@ func replayPaymentByKeyHTTPWithClient(ctx context.Context, idempotencyKey string } // replayErrorProvesNoCharge reports whether a ReplayPaymentByKey error -// definitively proves Square has no payment under the key. A retained key -// makes Square return the original payment (HTTP 2xx); every other DEFINITIVE -// business rejection must therefore be Square attempting to process the -// synthetic probe source for an unknown key — which can never succeed, so the -// charge never happened. Auth (401/403 — affects every Square call, must not -// fail rows) and rate-limit (429 — transient) are deliberately NOT proof; a -// 5xx / transport error is ambiguous by definition. +// definitively proves Square has no payment under the key. The replay carries +// the ORIGINAL source_id (identical-body retry), so a retained key makes Square +// return the original payment (HTTP 2xx); any definitive 4xx business rejection +// must therefore be Square attempting a REAL charge with the expired/used +// source — which can never succeed, so the charge never happened under that +// key. IDEMPOTENCY_KEY_REUSED is the exception: it can only occur when the +// stored source differs from the original charge's source (a data bug), so it +// proves NOTHING about whether the original charge landed — it is AMBIGUOUS, +// never proof of no charge. Auth (401/403 — affects every Square call, must +// not fail rows), rate-limit (429 — transient), 5xx and transport errors are +// ambiguous by definition. func replayErrorProvesNoCharge(err error) bool { if err == nil { return false } + if ErrorCode(err) == "IDEMPOTENCY_KEY_REUSED" { + return false + } switch ErrorStatusCode(err) { case http.StatusUnauthorized, http.StatusForbidden, http.StatusTooManyRequests: return false } - if status := ErrorStatusCode(err); status >= 400 && status < 500 { - return true - } - // Errors without a structured HTTP status: a structured Square error code - // is a definitive business response; the message match covers the dev mock's - // plain rejection wording. - if ErrorCode(err) != "" { - return true - } - msg := strings.ToUpper(err.Error()) - return strings.Contains(msg, "INVALID_REQUEST") || strings.Contains(msg, "SOURCE_ID") + status := ErrorStatusCode(err) + return status >= 400 && status < 500 } // squareAPIError wraps a formatted Square API error while exposing the @@ -740,7 +756,7 @@ func refundPaymentHTTPWithClient(ctx context.Context, req RefundPaymentReq, hc * body := sqRefundPaymentRequest{ PaymentID: req.PaymentID, IdempotencyKey: req.IdempotencyKey, - AmountMoney: sqMoney{Amount: req.Amount, Currency: "GBP"}, + AmountMoney: sqMoney{Amount: req.Amount, Currency: gbpCurrency}, Reason: req.Reason, } var resp sqRefundPaymentResponse diff --git a/backend/internal/square/square_http_client_test.go b/backend/internal/square/square_http_client_test.go index d52f9db..3e79e56 100644 --- a/backend/internal/square/square_http_client_test.go +++ b/backend/internal/square/square_http_client_test.go @@ -3,11 +3,13 @@ package square import ( + "bytes" "context" "crypto/sha256" "encoding/json" "errors" "fmt" + "io" "net/http" "net/http/httptest" "strings" @@ -16,6 +18,130 @@ import ( "unicode/utf8" ) +// TestReplayPaymentByKeyHTTP_IdenticalBody verifies the replay-by-key sends the +// FULL ORIGINAL request body (identical-body replay): a snapshot carrying +// customer_id, reference_id, note and buyer_email_address — the fields the +// original charge sends that a key+source+amount reconstruction would DROP — +// must reach Square verbatim. Square's idempotency dedup compares the whole +// request, so a partial replay body returns IDEMPOTENCY_KEY_REUSED for a +// retained key and the row stays pending forever. +func TestReplayPaymentByKeyHTTP_IdenticalBody(t *testing.T) { + var capturedRaw []byte + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + t.Errorf("expected POST, got %s", r.Method) + } + if r.URL.Path != "/v2/payments" { + t.Errorf("expected /v2/payments, got %s", r.URL.Path) + } + var err error + capturedRaw, err = io.ReadAll(r.Body) + if err != nil { + t.Errorf("failed to read request body: %v", err) + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"payment":{"id":"pay_orig","status":"COMPLETED","total_money":{"amount":5000,"currency":"GBP"},"source_type":"CARD","location_id":"loc","created_at":"2026-07-31T00:00:00Z","updated_at":"2026-07-31T00:00:00Z"}}`)) + })) + defer srv.Close() + + hc := &httpClient{baseURL: srv.URL, token: "t", http: srv.Client()} + req := CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "cnon:original-source", + IdempotencyKey: "ik-replay", + ReferenceID: "booking-123", + Note: "deposit", + CustomerID: "cus_123", + VerificationToken: "verify-token-abc", + BuyerEmail: "buyer@example.com", + } + snapshot, err := json.Marshal(req) + if err != nil { + t.Fatalf("failed to build snapshot: %v", err) + } + res, err := replayPaymentByKeyHTTPWithClient(context.Background(), snapshot, hc) + if err != nil { + t.Fatalf("replayPaymentByKeyHTTP failed: %v", err) + } + // The wire body must be BYTE-IDENTICAL to the original charge's body + // (both go through buildCreatePaymentBody from the same CreatePaymentReq). + expectedWire, err := json.Marshal(buildCreatePaymentBody(req, hc)) + if err != nil { + t.Fatalf("failed to marshal expected wire body: %v", err) + } + if !bytes.Equal(capturedRaw, expectedWire) { + t.Errorf("replay body is not byte-identical to the original charge body:\n got %s\n want %s", capturedRaw, expectedWire) + } + var captured map[string]any + if err := json.Unmarshal(capturedRaw, &captured); err != nil { + t.Fatalf("failed to decode request body: %v", err) + } + if captured["source_id"] != "cnon:original-source" { + t.Errorf("expected the ORIGINAL source_id in the replay body, got %v", captured["source_id"]) + } + if captured["idempotency_key"] != "ik-replay" { + t.Errorf("expected idempotency_key ik-replay, got %v", captured["idempotency_key"]) + } + // The extra fields the original charge sent must survive the replay — + // dropping them would make Square return IDEMPOTENCY_KEY_REUSED. + if captured["customer_id"] != "cus_123" { + t.Errorf("expected customer_id cus_123 in the replay body, got %v", captured["customer_id"]) + } + if captured["reference_id"] != "booking-123" { + t.Errorf("expected reference_id booking-123 in the replay body, got %v", captured["reference_id"]) + } + if captured["note"] != "deposit" { + t.Errorf("expected note deposit in the replay body, got %v", captured["note"]) + } + if captured["buyer_email_address"] != "buyer@example.com" { + t.Errorf("expected buyer_email_address buyer@example.com in the replay body, got %v", captured["buyer_email_address"]) + } + if captured["verification_token"] != "verify-token-abc" { + t.Errorf("expected verification_token verify-token-abc in the replay body, got %v", captured["verification_token"]) + } + amt, ok := captured["amount_money"].(map[string]any) + if !ok || amt["amount"] != float64(5000) || amt["currency"] != "GBP" { + t.Errorf("expected amount_money {5000 GBP} (identical to the original charge), got %v", captured["amount_money"]) + } + if res.ID != "pay_orig" { + t.Errorf("expected the original payment returned, got %+v", res) + } +} + +// TestReplayErrorProvesNoCharge_Classification locks the identical-body replay +// error classification: a definitive 4xx (minus 401/403/429) proves the charge +// never happened; IDEMPOTENCY_KEY_REUSED is AMBIGUOUS (a data bug, never proof +// of no charge); 401/403/429/5xx/transport are ambiguous. +func TestReplayErrorProvesNoCharge_Classification(t *testing.T) { + badRequest := func(code string) error { + return &squareAPIError{Code: code, StatusCode: http.StatusBadRequest, err: errors.New("square: boom")} + } + cases := []struct { + name string + err error + want bool + }{ + {name: "card_declined_4xx_proves_no_charge", err: badRequest("CARD_DECLINED"), want: true}, + {name: "invalid_request_4xx_proves_no_charge", err: badRequest("INVALID_REQUEST_ERROR"), want: true}, + {name: "plain_400_proves_no_charge", err: &squareAPIError{StatusCode: http.StatusBadRequest, err: errors.New("square: HTTP 400")}, want: true}, + {name: "idempotency_key_reused_is_ambiguous", err: badRequest("IDEMPOTENCY_KEY_REUSED"), want: false}, + {name: "unauthorized_is_ambiguous", err: &squareAPIError{StatusCode: http.StatusUnauthorized, err: errors.New("square: 401")}, want: false}, + {name: "forbidden_is_ambiguous", err: &squareAPIError{StatusCode: http.StatusForbidden, err: errors.New("square: 403")}, want: false}, + {name: "rate_limited_is_ambiguous", err: &squareAPIError{StatusCode: http.StatusTooManyRequests, err: errors.New("square: 429")}, want: false}, + {name: "server_error_is_ambiguous", err: &squareAPIError{StatusCode: http.StatusInternalServerError, err: errors.New("square: 500")}, want: false}, + {name: "transport_error_is_ambiguous", err: errors.New("network error: connection reset"), want: false}, + {name: "nil_is_ambiguous", err: nil, want: false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := replayErrorProvesNoCharge(tc.err); got != tc.want { + t.Errorf("replayErrorProvesNoCharge(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} + func TestPaymentFromSquare_ElseBranch_SurfacesBrandWithoutCardID(t *testing.T) { p := &sqPayment{ ID: "pay_1", diff --git a/backend/internal/square/types.go b/backend/internal/square/types.go index 8cb6fdb..b55bc87 100644 --- a/backend/internal/square/types.go +++ b/backend/internal/square/types.go @@ -22,14 +22,14 @@ var ErrRefundDeclined = errors.New("square: refund declined") var ErrRefundAlreadyProcessed = errors.New("square: refund already processed") // ErrReplayKeyNotRetained is returned by ReplayPaymentByKey when Square proves -// it holds NO payment under the idempotency key — either the original -// CreatePayment never reached Square (connect/DNS failure before the request -// was processed) or the key's 24h retention window has closed. The replay -// carries a synthetic probe source token that can never process a real charge, -// so Square's rejection of the probe is definitive: the charge never happened. -// Callers treat this error as proof the payment was never made — never as an -// ambiguous "maybe charged" state. -var ErrReplayKeyNotRetained = errors.New("square: no payment under idempotency key (replay probe rejected)") +// it holds NO payment under the idempotency key. The replay re-issues POST +// /v2/payments with an IDENTICAL body to the original charge (the stored +// square_request_snapshot), so a key Square no longer retains makes Square +// attempt a REAL charge with the (expired/used) source — which Square rejects +// with a definitive 4xx. That rejection is proof the charge never happened +// under the key. Callers treat this error as proof the payment was never made — +// never as an ambiguous "maybe charged" state. +var ErrReplayKeyNotRetained = errors.New("square: no payment under idempotency key (identical-body replay rejected)") // CreatePaymentReq maps to Square's CreatePayment endpoint (POST /v2/payments). // Square API reference: https://developer.squareup.com/reference/square/payments-api/create-payment @@ -191,15 +191,18 @@ type SquareClient interface { GetPayment(ctx context.Context, paymentID string) (*PaymentResult, error) // ReplayPaymentByKey asks Square whether a payment exists under an - // idempotency key by re-issuing POST /v2/payments with the same key and - // amount. Square's idempotency guarantee returns the ORIGINAL payment for a - // retained key and NEVER issues a second charge. The request carries a - // synthetic probe source token that cannot process a real charge, so when - // the key is unknown/expired Square definitively rejects the request - // (ErrReplayKeyNotRetained) instead of creating a new payment — the replay - // can never charge a customer. Used by the stale-pending sweep to rescue + // idempotency key by re-issuing POST /v2/payments with the FULL ORIGINAL + // request body — the square_request_snapshot stored on the pending row at + // charge time (same source_id, key, amount and every other field the + // original charge carried). Square's idempotency guarantee returns the + // ORIGINAL payment for a retained key and NEVER issues a second charge. A + // key Square does not retain makes Square attempt a real charge with the + // (expired/used) source, which is rejected with a definitive 4xx — surfaced + // as ErrReplayKeyNotRetained (proof the charge never happened). A reused key + // with a DIFFERENT source returns IDEMPOTENCY_KEY_REUSED — a data bug that is + // NOT proof of no charge. Used by the stale-pending sweep to rescue // lost-response charges whose square_payment_id was never persisted. - ReplayPaymentByKey(ctx context.Context, idempotencyKey string, amount int64) (*PaymentResult, error) + ReplayPaymentByKey(ctx context.Context, snapshotJSON []byte) (*PaymentResult, error) // CreateCustomer provisions a Square customer (customer provisioning for // card-on-file payments). Square dedups on the deterministic diff --git a/backend/main.go b/backend/main.go index 8736dc3..8d9d5e3 100644 --- a/backend/main.go +++ b/backend/main.go @@ -124,6 +124,14 @@ func initSquare() { } else { fmt.Println("Square client initialized (dev mock)") } + // 2FA enforcement is fail-closed (see payments.twoFactorEnforced): it is + // OFF only when REQUIRE_2FA=false or SQUARE_ENVIRONMENT explicitly selects + // the dev/mock stack. Warn loudly when a non-dev env (empty/unknown — a + // likely misconfiguration) leaves the gate disabled, so saved-card charges + // can never silently ship without the PSD2 SCA stand-in. + if os.Getenv("REQUIRE_2FA") == "false" && !payments.IsExplicitDevOrMockEnv() { + log.Printf("WARNING: 2FA enforcement is OFF (REQUIRE_2FA=false) with SQUARE_ENVIRONMENT=%q (not an explicit mock/dev value). Online saved-card payments will NOT require 2FA.", os.Getenv("SQUARE_ENVIRONMENT")) + } } func healthCheckHandler(w http.ResponseWriter, r *http.Request) { @@ -403,6 +411,13 @@ func main() { r.Put("/user/change-password", user.ChangePasswordHandler) r.Get("/user/notification-preferences", user.GetNotificationPreferencesHandler) r.Put("/user/notification-preferences", user.UpdateNotificationPreferencesHandler) + // 2FA settings (loose-fake PSD2 SCA gate for online card payments). + // RequireNonGuest: any logged-in user who could save cards must be + // able to reach these, not just verified accounts. + r.With(mw.RequireNonGuest).Get("/user/2fa/status", user.GetTwoFAStatusHandler) + r.With(mw.RequireNonGuest).Post("/user/2fa/setup", user.SetupTwoFAHandler) + r.With(mw.RequireNonGuest).Post("/user/2fa/verify", user.VerifyTwoFAHandler) + r.With(mw.RequireNonGuest).Post("/user/2fa/disable", user.DisableTwoFAHandler) r.Delete("/user/account", user.DeleteAccountHandler) r.Get("/user/gdpr-export", user.GetGDPRExportHandler) r.Get("/user/loyalty", user.GetLoyaltyHandler) diff --git a/frontend/src/lib/components/admin/TillPurchases.svelte b/frontend/src/lib/components/admin/TillPurchases.svelte index c1c44d7..a35c807 100644 --- a/frontend/src/lib/components/admin/TillPurchases.svelte +++ b/frontend/src/lib/components/admin/TillPurchases.svelte @@ -8,6 +8,8 @@ import { apiFetch } from '$lib/utils/api'; import SquareCardInput from '$lib/components/payments/SquareCardInput.svelte'; import { isSquareConfigured, submitPaymentWithRetry } from '$lib/square/square'; + import { authStore } from '$lib/stores/auth.svelte'; + import { resolve } from '$app/paths'; type CartItem = { id: string; @@ -96,9 +98,19 @@ }) ); + // PSD2 SCA stand-in: 2FA required but not enabled blocks charging a + // customer's saved card online. Cash, card machine, and online (new-card + // nonce) payments are unaffected. + const twoFactorBlocksSavedCards = $derived( + !!authStore.currentUser?.twoFactorRequired && !authStore.currentUser?.twoFactorEnabled + ); + // The saved-card option is hidden outright unless a customer is selected - // AND has at least one currently-valid card on file. - const showSavedCardOption = $derived(selectedCustomer !== null && validCards.length > 0); + // AND has at least one currently-valid card on file AND 2FA gating is not + // active. + const showSavedCardOption = $derived( + selectedCustomer !== null && validCards.length > 0 && !twoFactorBlocksSavedCards + ); const availablePaymentMethods = $derived( PAYMENT_METHODS.filter((m) => m.key !== 'saved_card' || showSavedCardOption) @@ -249,6 +261,10 @@ ); return; } + if (paymentMethod === 'saved_card' && twoFactorBlocksSavedCards) { + toast.error('Two-factor authentication is required to use online card payments'); + return; + } if (paymentMethod === 'saved_card' && (!selectedCustomer || !selectedSavedCardId)) { toast.error('Select a customer and a saved card before charging'); return; @@ -614,6 +630,13 @@ + {#if twoFactorBlocksSavedCards} +
+ Two-factor authentication is required to use online card payments. + Enable it in your account settings. +
+ {/if} + {#if paymentMethod === 'online_square'}
{#if isSquareConfigured()} diff --git a/frontend/src/lib/components/booking/BookingFlow.svelte b/frontend/src/lib/components/booking/BookingFlow.svelte index dcd4e7a..bbb3019 100644 --- a/frontend/src/lib/components/booking/BookingFlow.svelte +++ b/frontend/src/lib/components/booking/BookingFlow.svelte @@ -139,6 +139,13 @@ const canSaveCards = $derived(canSaveCardsForRole(authStore.currentUser?.role)); + // PSD2 SCA stand-in: 2FA required but not enabled blocks saved-card use + // and saving new cards for reuse. The new-card (nonce) path has its own + // SCA via Square tokenizeWithVerification. + const twoFactorBlocksSavedCards = $derived( + !!authStore.currentUser?.twoFactorRequired && !authStore.currentUser?.twoFactorEnabled + ); + const depositCardFormValid = $derived(paymentCardSelectionValid); // VAT registration status from public business info (via shared store) @@ -309,6 +316,12 @@ isProcessingPayment = true; paymentAttempted = false; try { + // PSD2 SCA stand-in: never charge a saved card while 2FA is required + // but not enabled — clear any stale selection so the new-card + // (nonce) path is used instead. + if (twoFactorBlocksSavedCards && selectedPaymentMethod) { + selectedPaymentMethod = ''; + } await submitAndProceed(); if (!confirmedBooking) { toast.error('Booking was not created. Please try again.'); @@ -327,7 +340,7 @@ let newCardToken: string | undefined; let verificationToken: string | undefined; - if (selectedPaymentMethod) { + if (selectedPaymentMethod && !twoFactorBlocksSavedCards) { // saved card — nothing to tokenize } else if (paymentCardSelection) { // New-card mode: tokenize once per attempt, reuse the nonce + SCA diff --git a/frontend/src/lib/components/payments/CardSelection.svelte b/frontend/src/lib/components/payments/CardSelection.svelte index bf74ed6..52f255a 100644 --- a/frontend/src/lib/components/payments/CardSelection.svelte +++ b/frontend/src/lib/components/payments/CardSelection.svelte @@ -5,6 +5,8 @@ import type { SquareVerificationContact } from './SquareCardInput.svelte'; import PolicyPopover from '$lib/components/ui/policyPopover.svelte'; import { isSquareConfigured } from '$lib/square/square'; + import { authStore } from '$lib/stores/auth.svelte'; + import { resolve } from '$app/paths'; export interface SelectableCard { id: string; @@ -39,17 +41,42 @@ // would collide on the same checkbox id. Pure SPA, so no SSR concern. const consentId = `save-card-consent-${crypto.randomUUID()}`; + // PSD2 SCA stand-in: when 2FA is required but not yet enabled, saved-card + // selection and save-for-later are blocked. The new-card (nonce) path has + // its own SCA via Square tokenizeWithVerification, so only the saved-card + // list and the save toggle are gated here. + const twoFactorBlocksSavedCards = $derived( + !!authStore.currentUser?.twoFactorRequired && !authStore.currentUser?.twoFactorEnabled + ); + // Auto-select the default saved card when cards first load. Guarded by // !showNewCardForm so the "Use a new card" click (selectedCardId = '') is // NOT immediately overridden back to the default card — which would - // silently charge the wrong card on submit. + // silently charge the wrong card on submit. Also skipped while 2FA gating + // is active so a saved card is never selected by default. $effect(() => { - if (cards.length > 0 && !selectedCardId && !showNewCardForm) { + if ( + cards.length > 0 && + !selectedCardId && + !showNewCardForm && + !twoFactorBlocksSavedCards + ) { const defaultCard = cards.find((c) => c.is_default) ?? cards[0]; selectedCardId = defaultCard.id; } }); + // While 2FA gating is active, keep the shared component self-consistent: + // never allow a saved card to stay selected or the save-card checkbox to + // remain checked (the parents' submit paths also guard, this is belt-and- + // braces for pre-selected state from a previous session). + $effect(() => { + if (twoFactorBlocksSavedCards && (selectedCardId !== '' || saveCard)) { + selectedCardId = ''; + saveCard = false; + } + }); + // When no saved cards exist the new-card form shows by default (no toggle). const newCardMode = $derived(showNewCardForm || cards.length === 0); @@ -93,32 +120,41 @@ {#if cards.length > 0}
- {#each cards as card (card.id)} -
+ {:else} + {#each cards as card (card.id)} + - {/each} + {#if selectedCardId === card.id && !showNewCardForm} + Selected + {/if} + + {/each} + {/if}
+{:else if twoFactorBlocksSavedCards} +
+

+ Two-factor authentication is required to use online card payments. + Enable it in your account settings. +

+
{/if} {#if newCardMode} @@ -153,7 +196,7 @@ {/if} - {#if canSaveCards && squareCardReady} + {#if canSaveCards && squareCardReady && !twoFactorBlocksSavedCards}