From 5e3dc9b428855b7c5e1492c4e0507c3b66ca46f4 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Wed, 5 Aug 2026 11:37:39 +0100 Subject: [PATCH] fix: comprehensive payment system hardening (4 review passes) CRITICAL fixes: - C1: JWT exp claim now validated via jwtauth.VerifyToken (was Decode) - C2: OverrideAmount validated post-substitution (prevents negative money minting) - C3: Terminal gift-card payments store gift_card_id; refund credits user balance - C4: Refund dedup returns stored amount, not req.Amount (prevents admin mislead) - C5: Booking recheck uses FOR UPDATE (prevents TOCTOU with cancellation) - C6: processChargeGroup idempotency key stable (charge-only, prevents double-refund) MAJOR fixes: - M2: Gift-card refund UPDATE checks RowsAffected; 0 rows -> failed - M3: ProcessCancellationRefund returns commit error (was swallowed) - M5: Dispute webhook handling (created + state.updated + disputes table) MEDIUM fixes: - ME1: CORS restricted to FRONTEND_ORIGIN env var (was reflect-any) - ME2: anonymize_user() scrubs users.notes, bookings.notes, name_history, refresh_tokens - ME3: Webhook handlers now mutate state (payment.updated, refund.updated) Frontend fixes: - Same-key retry on 503 (ambiguous failure) wired to all 8 payment flows - CHARGE_AND_STORE intent for save-card flows (SCA compliance) - Nonce staleness check verified across all flows Additional fixes from adversarial re-review: - F1: Till-sale completed dedup echoes stored amount (C4-class) - F2: Cash/giftcard terminal path uses FOR UPDATE (C5-class) - F3: Square-success UPDATE checks RowsAffected (till sales) - F4: Dispute reason truncated to 192 chars (prevents INSERT failure) - F5: Booking-user lookup failure marks refund failed (prevents silent money loss) - F6: Saved-card/tip rechecks wrapped in transaction (C5 residual) Tests: - 15 adversarial attack tests (negative override, zero override, terminal gift card, refund dedup, TOCTOU, deleted gift card, advisory lock, overcharge, zero/negative/huge amount, raw PAN, missing auth, gift card balance, concurrent refunds) - 14 webhook state tests (dispute created/state, payment/refund updated) - 3 CORS tests, 3 GDPR tests, 1 HTTP timeout test - Full suite passes with -race (25 packages, 0 failures) 25 files changed, +1532/-275 lines --- backend/auth/jwt.go | 2 +- backend/auth/jwt_test.go | 16 + .../payments/adversarial_attack_test.go | 639 +++++++++++++++++ backend/handlers/payments/charge_helpers.go | 7 +- backend/handlers/payments/handlers.go | 142 +++- backend/handlers/payments/refunds.go | 176 +++-- backend/handlers/payments/refunds_test.go | 134 ++-- backend/handlers/payments/till.go | 32 +- backend/handlers/payments/till_test.go | 160 ++++- backend/handlers/user/gdpr_test.go | 114 +++ backend/handlers/webhooks/square.go | 454 +++++++++++- .../handlers/webhooks/webhooks_state_test.go | 659 ++++++++++++++++++ .../square/square_http_client_timeout_test.go | 75 ++ backend/main.go | 92 ++- backend/main_test.go | 82 +++ .../account/UserBookingModal.svelte | 33 +- .../admin/GiftCardsManagement.svelte | 18 +- .../lib/components/admin/TillPurchases.svelte | 14 +- .../lib/components/booking/BookingFlow.svelte | 47 +- .../components/payments/CardSelection.svelte | 9 +- .../components/payments/MockCardForm.svelte | 3 +- .../components/payments/PaymentModal.svelte | 23 +- .../payments/SquareCardInput.svelte | 17 +- .../lib/components/payments/TipPayment.svelte | 31 +- .../payments/UserPaymentModal.svelte | 54 +- frontend/src/lib/square/square.ts | 45 ++ frontend/src/routes/account/+page.svelte | 56 +- init-scripts/init-script.sql | 46 ++ 28 files changed, 2905 insertions(+), 275 deletions(-) create mode 100644 backend/handlers/payments/adversarial_attack_test.go create mode 100644 backend/handlers/webhooks/webhooks_state_test.go create mode 100644 backend/internal/square/square_http_client_timeout_test.go diff --git a/backend/auth/jwt.go b/backend/auth/jwt.go index e3b3bc7..8861363 100644 --- a/backend/auth/jwt.go +++ b/backend/auth/jwt.go @@ -141,7 +141,7 @@ func GenerateToken(userID string, role string) (string, string, error) { // VerifyToken validates JWT and returns user_id, role, and jti func VerifyToken(tokenString string, ctx context.Context) (userID string, role string, jti string, err error) { - token, err := TokenAuth.Decode(tokenString) + token, err := jwtauth.VerifyToken(TokenAuth, tokenString) if err != nil { return "", "", "", err } diff --git a/backend/auth/jwt_test.go b/backend/auth/jwt_test.go index 25ae544..89c62b4 100644 --- a/backend/auth/jwt_test.go +++ b/backend/auth/jwt_test.go @@ -178,6 +178,22 @@ func TestVerifyToken_RevokedJTI(t *testing.T) { } } +// TestVerifyToken_ExpiredToken creates a token whose "exp" claim is in the past +// and verifies that VerifyToken rejects it with an error containing "expired". +func TestVerifyToken_ExpiredToken(t *testing.T) { + _, tokenString, err := TokenAuth.Encode(map[string]interface{}{ + "user_id": "user-expired", + "role": "verified_email", + "jti": "test-jti-expired", + "exp": clock.Now().Add(-1 * time.Hour).Unix(), + }) + require.NoError(t, err) + + _, _, _, err = VerifyToken(tokenString, context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "expired") +} + // TestVerifyToken_MissingJTI creates a token without a "jti" claim (using // TokenAuth.Encode directly) and verifies that VerifyToken returns an error // containing "invalid jti claim". diff --git a/backend/handlers/payments/adversarial_attack_test.go b/backend/handlers/payments/adversarial_attack_test.go new file mode 100644 index 0000000..55c5f99 --- /dev/null +++ b/backend/handlers/payments/adversarial_attack_test.go @@ -0,0 +1,639 @@ +//go:build test && dev + +package payments + +import ( + "context" + "encoding/json" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "crussell/db" + "crussell/internal/square" + "crussell/testutils" + "crussell/testutils/fixtures" + "crussell/testutils/jwt" + + "github.com/stretchr/testify/require" +) + +// ============================================================================= +// ADVERSARIAL QA ATTACK TESTS — break-the-app scenarios +// ============================================================================= + +// TestAttack_NegativeOverrideAmount_MintsGiftCardBalance attempts to mint +// money: a terminal giftcard payment with a NEGATIVE override_amount. The +// handler validates only req.Amount, then replaces amount with the override — +// a negative override on the giftcard path flips the balance deduction into a +// credit (balance - (-50) = balance + 50). +func TestAttack_NegativeOverrideAmount_MintsGiftCardBalance(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestDataAtTime(t, ctx, tx, time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + // Give the booking user a gift-card account balance. + _, err := tx.Exec(ctx, "INSERT INTO user_giftcard_balances (user_id, balance) VALUES ($1, 100.00)", userID) + require.NoError(t, err) + + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + adminToken := jwt.GenerateTestToken(adminID, "admin") + + // amount=5000 (£50) passes ValidateAmount; override_amount=-5000 is the attack. + neg := int64(-5000) + pm := "giftcard" + req := CreateTerminalPaymentRequest{ + Amount: 5000, + PaymentType: "full", + OverrideAmount: &neg, + PaymentMethod: &pm, + } + w := makePaymentRequest(CreateTerminalPayment, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx) + + var balance float64 + err = tx.QueryRow(ctx, "SELECT balance FROM user_giftcard_balances WHERE user_id = $1", userID).Scan(&balance) + require.NoError(t, err) + + // INVARIANT: a payment request must never increase a gift-card balance. + // Expected: 400 reject (invalid override) and balance unchanged at 100.00. + // Actual (bug): 200 COMPLETED, balance mints to 150.00. + if w.Code != http.StatusBadRequest && w.Code != http.StatusOK { + t.Fatalf("unexpected status %d: %s", w.Code, w.Body.String()) + } + if balance != 100.00 { + t.Errorf("BUG: negative override_amount changed balance to %.2f (was 100.00) — money minted/lost", balance) + } + var payCount int + _ = tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND amount < 0", bookingID).Scan(&payCount) + if payCount > 0 { + t.Errorf("BUG: %d negative-amount payment record(s) created", payCount) + } +} + +// TestAttack_ZeroOverrideAmount_CreatesFreePayment attempts a £0 gift-card +// payment via override_amount=0. The giftcard balance is untouched but a +// completed £0 payment is recorded. +func TestAttack_ZeroOverrideAmount_CreatesFreePayment(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestDataAtTime(t, ctx, tx, time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + _, err := tx.Exec(ctx, "INSERT INTO user_giftcard_balances (user_id, balance) VALUES ($1, 100.00)", userID) + require.NoError(t, err) + + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + adminToken := jwt.GenerateTestToken(adminID, "admin") + + zero := int64(0) + pm := "giftcard" + req := CreateTerminalPaymentRequest{ + Amount: 5000, + PaymentType: "full", + OverrideAmount: &zero, + PaymentMethod: &pm, + } + w := makePaymentRequest(CreateTerminalPayment, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx) + _ = w + + var zeroPayCount int + _ = tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND amount = 0", bookingID).Scan(&zeroPayCount) + if zeroPayCount > 0 { + t.Errorf("BUG: %d zero-amount completed payment record(s) created", zeroPayCount) + } +} + +// TestAttack_TerminalGiftCard_CancellationRefund_LosesBalance: a user pays a +// booking from their gift-card ACCOUNT balance via the terminal. The terminal +// giftcard branch never stores gift_card_id on the payment row, so the +// cancellation refund loop cannot credit the balance back ("cannot refund to +// card. Skipping") yet still inserts a COMPLETED refund row — money is lost +// while the books claim it was refunded. +func TestAttack_TerminalGiftCard_CancellationRefund_LosesBalance(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestDataAtTime(t, ctx, tx, time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + _, err := tx.Exec(ctx, "INSERT INTO user_giftcard_balances (user_id, balance) VALUES ($1, 100.00)", userID) + require.NoError(t, err) + + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + adminToken := jwt.GenerateTestToken(adminID, "admin") + + // Pay £30 from the gift-card account balance via terminal. + pm := "giftcard" + req := CreateTerminalPaymentRequest{Amount: 3000, PaymentType: "full", PaymentMethod: &pm} + w := makePaymentRequest(CreateTerminalPayment, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "terminal giftcard payment failed: %s", w.Body.String()) + + var balanceAfterPay float64 + require.NoError(t, tx.QueryRow(ctx, "SELECT balance FROM user_giftcard_balances WHERE user_id = $1", userID).Scan(&balanceAfterPay)) + require.Equal(t, 70.00, balanceAfterPay) + + // Cancel the booking >72h before start → full £30 refund. + innerTx := db.TxFromContext(ctx) + require.NotNil(t, innerTx) + now := time.Date(2099, 12, 28, 8, 0, 0, 0, time.UTC) + start := time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC) + result, err := ProcessCancellationRefundTx(ctx, innerTx, bookingID, 50, 30, start, now, "client_cancelled", &userID, false) + require.NoError(t, err) + require.NotNil(t, result) + if result.RefundableAmount != 30 { + t.Fatalf("expected refundable 30, got %.2f", result.RefundableAmount) + } + + var balanceAfterCancel float64 + require.NoError(t, tx.QueryRow(ctx, "SELECT balance FROM user_giftcard_balances WHERE user_id = $1", userID).Scan(&balanceAfterCancel)) + + // INVARIANT: a completed £30 refund must put the £30 back on the account. + // Expected: 100.00. Actual (bug): 70.00 — refund row 'completed' but no money returned. + if balanceAfterCancel != 100.00 { + t.Errorf("BUG: after full refund, balance is %.2f (was 70.00 post-payment) — refund recorded but money not credited", balanceAfterCancel) + } + + // The refund row must be pending (Square) or completed-with-money-moved. + var refundStatus string + var refundAmount float64 + if err := tx.QueryRow(ctx, "SELECT status, amount FROM refunds WHERE booking_id = $1 ORDER BY created_at DESC LIMIT 1", bookingID).Scan(&refundStatus, &refundAmount); err != nil { + t.Fatalf("no refund row recorded: %v", err) + } + if refundStatus != "completed" || refundAmount != 30 { + t.Errorf("BUG: refund row is status=%s amount=%.2f — expected completed £30 (it is completed but balance was NOT credited back)", refundStatus, refundAmount) + } +} + +// TestAttack_Refund_SameKeyDifferentAmount_ReportsWrongAmount: the client +// reuses an idempotency key for a refund with a DIFFERENT amount. The dedup +// returns the existing completed refund but echoes the NEW requested amount in +// the response — the admin believes the new amount was refunded. +func TestAttack_Refund_SameKeyDifferentAmount_ReportsWrongAmount(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + _, bookingID, _ := setupTestData(t, ctx, tx) + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + adminToken := jwt.GenerateTestToken(adminID, "admin") + + paymentID, err := fixtures.CreateTestPayment(tx, bookingID, 50, "in_person_card", "full", "completed") + require.NoError(t, err) + _, err = tx.Exec(ctx, "UPDATE payments SET square_payment_id = 'sqp_attack_samekey' WHERE id = $1", paymentID) + require.NoError(t, err) + + handler := RefundPayment + key := "attack-same-key-diff-amount" + w1 := makePaymentRequest(handler, "POST", "/api/admin/payments/"+paymentID+"/refund", RefundRequest{Amount: 2000, Reason: "first", IdempotencyKey: key}, adminToken, ctx) + require.Equal(t, http.StatusOK, w1.Code, "first refund failed: %s", w1.Body.String()) + + // Same key, DIFFERENT amount (£30). + w2 := makePaymentRequest(handler, "POST", "/api/admin/payments/"+paymentID+"/refund", RefundRequest{Amount: 3000, Reason: "second", IdempotencyKey: key}, adminToken, ctx) + require.Equal(t, http.StatusOK, w2.Code, "second refund failed: %s", w2.Body.String()) + + var resp RefundResponse + require.NoError(t, json.Unmarshal(w2.Body.Bytes(), &resp)) + + // The stored refund row for this key must hold the actually-refunded amount. + var storedAmount float64 + require.NoError(t, tx.QueryRow(ctx, "SELECT amount FROM refunds WHERE idempotency_key IS NOT NULL AND payment_id = $1 ORDER BY created_at DESC LIMIT 1", paymentID).Scan(&storedAmount)) + + // INVARIANT: the response must report the amount actually refunded (£20), + // never the newly-requested £30 for a dedup'd key. + if resp.Amount != int64(storedAmount*100) { + t.Errorf("BUG: same-key different-amount retry reports refunded %d pence, but stored refund is %.2f — admin misled", resp.Amount, storedAmount) + } + if resp.Status != "completed" { + t.Errorf("expected completed, got %s", resp.Status) + } +} + +// TestAttack_AdvisoryLockTimeout_Returns409 holds the booking payment advisory +// lock for longer than the ~3s bound and verifies the handler returns 409 +// (not a hang, not a double charge). +func TestAttack_AdvisoryLockTimeout_Returns409(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + token := jwt.GenerateUserToken(userID) + + // Hold the session advisory lock on a dedicated pool connection. + holder, err := db.Conn.Acquire(context.Background()) + require.NoError(t, err) + defer holder.Release() + _, err = holder.Exec(context.Background(), "SELECT pg_advisory_lock(hashtext($1))", "crussell:payment:"+bookingID) + require.NoError(t, err) + defer func() { + _, _ = holder.Exec(context.Background(), "SELECT pg_advisory_unlock(hashtext($1))", "crussell:payment:"+bookingID) + }() + + cardToken := "cnon:attack-lock-card" + req := CreateBookingPaymentRequest{ + Amount: 5000, + PaymentType: "full", + NewCardToken: &cardToken, + IdempotencyKey: "attack-lock-timeout-key", + } + start := time.Now() + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, token, ctx) + elapsed := time.Since(start) + + // Expected: 409 Conflict after ~3s (bounded try-lock), no charge. + if w.Code != http.StatusConflict { + t.Errorf("BUG: expected 409 when advisory lock is held, got %d: %s", w.Code, w.Body.String()) + } + if elapsed > 15*time.Second { + t.Errorf("BUG: handler hung for %v waiting on the lock", elapsed) + } + + // No payment record and no Square charge. + var payCount int + _ = tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND idempotency_key = 'attack-lock-timeout-key'", bookingID).Scan(&payCount) + if payCount != 0 { + t.Errorf("BUG: %d payment record(s) created while lock held", payCount) + } +} + +// TestAttack_RefundOvercharge verifies the over-refund guard: refunding more +// than the payment amount must be rejected. +func TestAttack_RefundOvercharge(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + _, bookingID, _ := setupTestData(t, ctx, tx) + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + adminToken := jwt.GenerateTestToken(adminID, "admin") + + paymentID, err := fixtures.CreateTestPayment(tx, bookingID, 30, "in_person_card", "full", "completed") + require.NoError(t, err) + _, err = tx.Exec(ctx, "UPDATE payments SET square_payment_id = 'sqp_attack_overcharge' WHERE id = $1", paymentID) + require.NoError(t, err) + + w := makePaymentRequest(RefundPayment, "POST", "/api/admin/payments/"+paymentID+"/refund", RefundRequest{Amount: 5000, Reason: "overcharge"}, adminToken, ctx) + if w.Code != http.StatusBadRequest { + t.Errorf("BUG: over-refund expected 400, got %d: %s", w.Code, w.Body.String()) + } + var refundCount int + _ = tx.QueryRow(ctx, "SELECT COUNT(*) FROM refunds WHERE payment_id = $1", paymentID).Scan(&refundCount) + if refundCount != 0 { + t.Errorf("BUG: over-refund created %d refund row(s)", refundCount) + } +} + +// TestAttack_ZeroAmount_Rejected: £0 booking payment must be rejected. +func TestAttack_ZeroAmount_Rejected(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + token := jwt.GenerateUserToken(userID) + cardToken := "cnon:attack-zero-card" + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", + CreateBookingPaymentRequest{Amount: 0, PaymentType: "full", NewCardToken: &cardToken, IdempotencyKey: "attack-zero"}, token, ctx) + if w.Code != http.StatusBadRequest { + t.Errorf("BUG: zero amount expected 400, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestAttack_NegativeAmount_Rejected: negative booking payment must be rejected. +func TestAttack_NegativeAmount_Rejected(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + token := jwt.GenerateUserToken(userID) + cardToken := "cnon:attack-negative-card" + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", + CreateBookingPaymentRequest{Amount: -1000, PaymentType: "full", NewCardToken: &cardToken, IdempotencyKey: "attack-negative"}, token, ctx) + if w.Code != http.StatusBadRequest { + t.Errorf("BUG: negative amount expected 400, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestAttack_HugeAmount_Rejected: a £1,000,000+ booking payment must be rejected. +func TestAttack_HugeAmount_Rejected(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + token := jwt.GenerateUserToken(userID) + cardToken := "cnon:attack-huge-card" + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", + CreateBookingPaymentRequest{Amount: 100_000_000, PaymentType: "full", NewCardToken: &cardToken, IdempotencyKey: "attack-huge"}, token, ctx) + if w.Code != http.StatusBadRequest { + t.Errorf("BUG: huge amount expected 400, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestAttack_RawPAN_NoCharge: a raw PAN as new_card_token must not produce a +// completed payment (PCI-DSS: only cnon:/ccof: tokens are accepted). The mock +// mirrors production and rejects non-token source IDs. +func TestAttack_RawPAN_NoCharge(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + token := jwt.GenerateUserToken(userID) + pan := "4111111111111111" + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", + CreateBookingPaymentRequest{Amount: 5000, PaymentType: "full", NewCardToken: &pan, IdempotencyKey: "attack-rawpan"}, token, ctx) + // The request must not be treated as success. + if w.Code == http.StatusOK { + t.Errorf("BUG: raw PAN accepted as a charge: %s", w.Body.String()) + } + var completed int + _ = tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND status = 'completed' AND idempotency_key = 'attack-rawpan'", bookingID).Scan(&completed) + if completed != 0 { + t.Errorf("BUG: raw PAN produced a completed payment") + } +} + +// TestAttack_MissingAuth_Rejected: payment route without a user context must be rejected. +func TestAttack_MissingAuth_Rejected(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + _, bookingID, _ := setupTestDataPast(t, ctx, tx) + cardToken := "cnon:attack-noauth-card" + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", + CreateBookingPaymentRequest{Amount: 5000, PaymentType: "full", NewCardToken: &cardToken, IdempotencyKey: "attack-noauth"}, "", ctx) + if w.Code == http.StatusOK { + t.Errorf("BUG: unauthenticated payment accepted: %s", w.Body.String()) + } +} + +// TestAttack_GiftCardSpendBeyondBalance: spending more than the remaining gift +// card balance must be rejected — a negative balance must never occur. +func TestAttack_GiftCardSpendBeyondBalance(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestDataAtTime(t, ctx, tx, time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + _, err := tx.Exec(ctx, "INSERT INTO user_giftcard_balances (user_id, balance) VALUES ($1, 10.00)", userID) + require.NoError(t, err) + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + adminToken := jwt.GenerateTestToken(adminID, "admin") + + pm := "giftcard" + req := CreateTerminalPaymentRequest{Amount: 5000, PaymentType: "full", PaymentMethod: &pm} // £50 > £10 balance + w := makePaymentRequest(CreateTerminalPayment, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx) + if w.Code != http.StatusBadRequest { + t.Errorf("BUG: spend beyond gift-card balance expected 400, got %d: %s", w.Code, w.Body.String()) + } + var balance float64 + _ = tx.QueryRow(ctx, "SELECT balance FROM user_giftcard_balances WHERE user_id = $1", userID).Scan(&balance) + if balance < 0 { + t.Errorf("BUG: gift card balance went negative: %.2f", balance) + } + if balance != 10.00 { + t.Errorf("balance changed to %.2f after rejected payment (want 10.00)", balance) + } +} + +// TestAttack_ConcurrentRefunds_NoOverRefund: two concurrent full-amount refunds +// on the same payment must result in exactly ONE Square refund call and one +// refund row. +func TestAttack_ConcurrentRefunds_NoOverRefund(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestData(t, ctx, tx) + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + adminToken := jwt.GenerateTestToken(adminID, "admin") + + paymentID, err := fixtures.CreateTestPayment(tx, bookingID, 50, "in_person_card", "full", "completed") + require.NoError(t, err) + _, err = tx.Exec(ctx, "UPDATE payments SET square_payment_id = 'sqp_attack_concurrent_refund' WHERE id = $1", paymentID) + require.NoError(t, err) + cleanupConcurrentTestRows(t, context.Background(), userID, bookingID) + + origClient := SquareClient + slow := &slowRefundClient{SquareClient: square.NewDevClient(), delay: 300 * time.Millisecond} + SquareClient = slow + defer func() { SquareClient = origClient }() + + innerTx := db.TxFromContext(ctx) + require.NotNil(t, innerTx) + require.NoError(t, innerTx.Commit(ctx)) + + pool := context.Background() + startBoth := make(chan struct{}) + codes := make([]int, 2) + done := make(chan struct{}, 2) + + for i := 0; i < 2; i++ { + go func(idx int) { + <-startBoth + req := RefundRequest{Amount: 5000, Reason: "concurrent", IdempotencyKey: "attack-concurrent-refund-" + string(rune('a'+idx))} + w := makePaymentRequest(RefundPayment, "POST", "/api/admin/payments/"+paymentID+"/refund", req, adminToken, pool) + codes[idx] = w.Code + done <- struct{}{} + }(i) + } + close(startBoth) + <-done + <-done + + okCount := 0 + for _, c := range codes { + if c == http.StatusOK { + okCount++ + } + } + if okCount != 1 { + t.Errorf("BUG: expected exactly 1 successful refund, got %d (codes %v)", okCount, codes) + } + var refundCount int + _ = db.Conn.QueryRow(pool, "SELECT COUNT(*) FROM refunds WHERE payment_id = $1", paymentID).Scan(&refundCount) + if refundCount != 1 { + t.Errorf("BUG: expected exactly 1 refund row, got %d", refundCount) + } +} + +// TestAttack_BookingPaymentRacingCancellation_NoCompletedPaymentWithoutRefund +// exercises the C5 fix: the post-charge booking-status recheck +// (recheckBookingPayable in charge_helpers.go) now reads the booking row with +// `FOR UPDATE`, so a concurrent cancellation — which also locks the row with +// FOR UPDATE before writing its cancelled status — serializes with it. Without +// the row lock, the recheck's plain SELECT could read the pre-cancellation +// status and commit a COMPLETED payment on a CANCELLED booking; the +// cancellation refund loop had already run while the charge was still pending, +// so NO refund would ever be generated for the money taken at Square. +// +// INVARIANT: never a completed payment on a cancelled booking without a +// refund. Either the payment is rejected (no completed payment row) or it +// succeeds AND a refund is generated. +func TestAttack_BookingPaymentRacingCancellation_NoCompletedPaymentWithoutRefund(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestData(t, ctx, tx) // status in_progress, start 2099-12-31 + token := jwt.GenerateUserToken(userID) + cleanupConcurrentTestRows(t, context.Background(), userID, bookingID) + + // Commit the setup so the payment and the cancellation run at pool level on + // INDEPENDENT connections — the advisory lock and the FOR UPDATE row lock + // only serialize across separate sessions. + innerTx := db.TxFromContext(ctx) + require.NotNil(t, innerTx) + require.NoError(t, innerTx.Commit(ctx)) + + // Delay the Square charge so the cancellation can interleave between the + // pending-record commit and the post-charge status recheck. + origClient := SquareClient + slow := &slowCreatePaymentClient{SquareClient: square.NewDevClient(), delay: 300 * time.Millisecond} + SquareClient = slow + defer func() { SquareClient = origClient }() + + pool := context.Background() + key := "attack-c5-" + bookingID + cardToken := "cnon:attack-c5-card" + req := CreateBookingPaymentRequest{ + Amount: 5000, + PaymentType: "full", + NewCardToken: &cardToken, + IdempotencyKey: key, + } + + var payRec *httptest.ResponseRecorder + payDone := make(chan struct{}) + go func() { + defer close(payDone) + payRec = makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, token, pool) + }() + + // Wait until the pending payment record is committed — the payment has now + // passed the pre-charge status check and is inside the (slow) Square call, + // holding the advisory lock. Starting the cancellation from here makes the + // interleaving deterministic. + deadline := time.Now().Add(10 * time.Second) + for { + var n int + if err := db.Conn.QueryRow(pool, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND idempotency_key = $2 AND status = 'pending'`, bookingID, key).Scan(&n); err == nil && n > 0 { + break + } + if time.Now().After(deadline) { + t.Fatal("timed out waiting for the payment to reach the pending-record commit") + } + time.Sleep(5 * time.Millisecond) + } + + start := time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC) + cancelAt := time.Date(2099, 12, 28, 8, 0, 0, 0, time.UTC) // >72h notice → full-refund tier + + cancelDone := make(chan struct{}) + go func() { + defer close(cancelDone) + cancelTx, err := db.Conn.Begin(pool) + if err != nil { + t.Errorf("cancellation: begin tx: %v", err) + return + } + defer cancelTx.Rollback(pool) + + // FOR UPDATE on the SAME booking row the payment's post-charge recheck + // locks (C5). Held until commit, so the recheck MUST block on it and + // then observe the committed cancelled status. + var st string + if err := cancelTx.QueryRow(pool, `SELECT status FROM bookings WHERE id = $1 FOR UPDATE`, bookingID).Scan(&st); err != nil { + t.Errorf("cancellation: lock booking row: %v", err) + return + } + if _, err := cancelTx.Exec(pool, `UPDATE bookings SET status = 'client_cancelled', updated_at = NOW() WHERE id = $1`, bookingID); err != nil { + t.Errorf("cancellation: set status: %v", err) + return + } + // Run the cancellation refund loop while the charge is still pending — + // it must find no COMPLETED payment to refund. + if _, err := ProcessCancellationRefundTx(pool, cancelTx, bookingID, 50, 50, start, cancelAt, "client_cancelled", &userID, false); err != nil { + t.Errorf("cancellation: refund: %v", err) + return + } + // Hold the row lock past the payment's Square round-trip so the + // payment's recheck is forced to block on it, then commit. + time.Sleep(600 * time.Millisecond) + if err := cancelTx.Commit(pool); err != nil { + t.Errorf("cancellation: commit: %v", err) + return + } + }() + + <-payDone + <-cancelDone + + // Ground truth: the booking is cancelled. + var bookingStatus string + require.NoError(t, db.Conn.QueryRow(pool, `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&bookingStatus)) + require.Equal(t, "client_cancelled", bookingStatus) + + // C5 INVARIANT: never a completed payment on a cancelled booking without a + // refund. + var completedCount int + require.NoError(t, db.Conn.QueryRow(pool, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND status = 'completed'`, bookingID).Scan(&completedCount)) + var refundCount int + require.NoError(t, db.Conn.QueryRow(pool, `SELECT COUNT(*) FROM refunds WHERE booking_id = $1`, bookingID).Scan(&refundCount)) + if completedCount > 0 && refundCount == 0 { + t.Errorf("BUG (C5): %d completed payment(s) on cancelled booking %s with NO refund — money taken at Square but never refunded", completedCount, bookingID) + } + + // Case-by-case: a 200 payment MUST be accompanied by a refund; any other + // result MUST NOT leave a completed payment behind. + if payRec.Code == http.StatusOK { + if refundCount == 0 { + t.Errorf("BUG (C5): payment succeeded (HTTP 200) on cancelled booking %s but no refund was generated", bookingID) + } + } else { + var status string + if err := db.Conn.QueryRow(pool, `SELECT status FROM payments WHERE booking_id = $1 AND idempotency_key = $2`, bookingID, key).Scan(&status); err == nil && status == "completed" { + t.Errorf("BUG (C5): payment failed (HTTP %d) but the payment row is status %q", payRec.Code, status) + } + } +} + +// TestAttack_DeletedGiftCard_RefundMarkedFailed exercises the M2 fix: the +// cancellation refund loop checks `RowsAffected()` on the gift-card balance +// UPDATE in ProcessCancellationRefundTx. When the gift card has been deleted, +// the UPDATE matches 0 rows — no money can move back onto the card — so the +// refund row must be recorded 'failed' (with a CRITICAL log), NEVER 'completed'. +// A 'completed' row on a deleted card would claim the customer's money was +// returned when it never moved. +func TestAttack_DeletedGiftCard_RefundMarkedFailed(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestData(t, ctx, tx) + + // Direct card redemption: a completed giftcard payment bound to a specific + // gift card (gift_card_id set — NOT the user's pooled account balance). + var gcID string + require.NoError(t, tx.QueryRow(ctx, ` + INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by) + VALUES (50.00, 50.00, $1) RETURNING id`, userID).Scan(&gcID)) + paymentID, err := fixtures.CreateTestPayment(tx, bookingID, 50, "giftcard", "full", "completed") + require.NoError(t, err) + _, err = tx.Exec(ctx, "UPDATE payments SET gift_card_id = $1 WHERE id = $2", gcID, paymentID) + require.NoError(t, err) + + // Delete the gift card (audit rows first so the FK allows the delete). + _, err = tx.Exec(ctx, "DELETE FROM gift_card_transactions WHERE gift_card_id = $1", gcID) + require.NoError(t, err) + _, err = tx.Exec(ctx, "DELETE FROM gift_cards WHERE id = $1", gcID) + require.NoError(t, err) + var gone int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM gift_cards WHERE id = $1", gcID).Scan(&gone)) + require.Zero(t, gone, "precondition: the gift card must be deleted") + + // Capture the CRITICAL slog output the M2 fix emits when the gift-card + // refund UPDATE matches 0 rows. + var sb syncBuffer + origLogger := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&sb, nil))) + defer slog.SetDefault(origLogger) + + innerTx := db.TxFromContext(ctx) + require.NotNil(t, innerTx) + start := time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC) + cancelAt := time.Date(2099, 12, 28, 8, 0, 0, 0, time.UTC) // >72h → full £50 refund + + result, err := ProcessCancellationRefundTx(ctx, innerTx, bookingID, 50, 50, start, cancelAt, "client_cancelled", &userID, false) + require.NoError(t, err) + require.NotNil(t, result) + if result.RefundableAmount != 50 { + t.Fatalf("expected refundable 50, got %.2f", result.RefundableAmount) + } + + // INVARIANT: the refund row must be 'failed' — the money could not be + // credited to a deleted card, so 'completed' would falsely claim a refund. + var refundStatus string + var refundAmount float64 + require.NoError(t, tx.QueryRow(ctx, "SELECT status, amount FROM refunds WHERE booking_id = $1 ORDER BY created_at DESC LIMIT 1", bookingID).Scan(&refundStatus, &refundAmount)) + if refundStatus != "failed" { + t.Errorf("BUG (M2): refund on deleted gift card is status=%q — must be 'failed' (money never moved)", refundStatus) + } + if refundAmount != 50 { + t.Errorf("expected refund amount 50, got %.2f", refundAmount) + } + + // The M2 fix must have emitted the CRITICAL log for the 0-row UPDATE. + logs := sb.String() + if !strings.Contains(logs, "CRITICAL") || !strings.Contains(logs, "affected 0 rows") { + t.Errorf("BUG (M2): expected a CRITICAL 'gift card refund UPDATE affected 0 rows' log, got: %q", logs) + } +} diff --git a/backend/handlers/payments/charge_helpers.go b/backend/handlers/payments/charge_helpers.go index 4f0ec28..72bd9df 100644 --- a/backend/handlers/payments/charge_helpers.go +++ b/backend/handlers/payments/charge_helpers.go @@ -160,7 +160,12 @@ func releaseBookingPaymentLock(pinConn *pgxpool.Conn, lockKey string) { // transaction semantics differ per path), and the 409 conflict response. func recheckBookingPayable(ctx context.Context, q db.Querier, bookingID string) (string, bool, error) { var status string - if err := q.QueryRow(ctx, `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&status); err != nil { + // FOR UPDATE (C5): a concurrent cancellation takes the same row lock and + // commits before this transaction commits, so the recheck cannot observe a + // status that changes between the read and the commit. Without the lock a + // cancellation could slip in between, leaving a completed payment on a + // cancelled booking with no refund. + if err := q.QueryRow(ctx, `SELECT status FROM bookings WHERE id = $1 FOR UPDATE`, bookingID).Scan(&status); err != nil { return "", false, err } return status, bookingStatusAllowsCompletedPayment(status), nil diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index c409b2a..054a04a 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -314,6 +314,16 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { amount = *req.OverrideAmount } + // C2: the struct validation above checks req.Amount only, and the override + // substitution happens after. A negative override would flip the gift-card + // balance deduction into a credit (money minting) and a zero override would + // record a free payment, so the EFFECTIVE amount must be validated here. + if err := ValidateAmount(amount); err != nil { + log.Printf("Failed to process request: %v", err) + http.Error(w, "Invalid override amount: "+err.Error(), http.StatusBadRequest) + return + } + // Idempotency key for the payment. Cash/giftcard terminal payments are // always fresh admin actions (not network-retryable), and the request has // no client key — so a deterministic booking+type+amount key would wrongly @@ -341,9 +351,14 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { } }() - // Check booking status inside the transaction. + // Check booking status inside the transaction. FOR UPDATE (C5): the + // payment insert below commits in this same transaction, so a + // concurrent cancellation (which takes the same row lock) must not be + // able to commit a cancelled status between this read and the commit — + // otherwise the payment would land on a cancelled booking with no + // refund ever generated. var status string - if err := tx.QueryRow(r.Context(), `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&status); err != nil { + if err := tx.QueryRow(r.Context(), `SELECT status FROM bookings WHERE id = $1 FOR UPDATE`, bookingID).Scan(&status); err != nil { if errors.Is(err, pgx.ErrNoRows) { http.Error(w, "Booking not found", http.StatusNotFound) return @@ -407,6 +422,12 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { } var cardVoucherType string // voucher_type_at_purchase from the gift card + // giftCardPaymentID records the source of funds on the payment row: + // the gift_card_id for a direct card redemption, or nil when the + // payment came from the user's account balance (usedBalance). The + // cancellation refund loop reads this column to know where to + // credit money back (C3). + var giftCardPaymentID *string if !usedBalance { // Try direct card redemption (for guests or users without a redeemed balance) if req.GiftCardID == nil || *req.GiftCardID == "" { @@ -460,14 +481,15 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal server error", http.StatusInternalServerError) return } + giftCardPaymentID = &cleanCardID } err = tx.QueryRow(r.Context(), ` INSERT INTO payments ( - booking_id, payment_type, payment_method, status, amount, idempotency_key, created_by, created_at, updated_at - ) VALUES ($1, $2, 'giftcard', 'completed', $3, $4, $5, NOW(), NOW()) + booking_id, payment_type, payment_method, status, amount, idempotency_key, created_by, created_at, updated_at, gift_card_id + ) VALUES ($1, $2, 'giftcard', 'completed', $3, $4, $5, NOW(), NOW(), $6) RETURNING id - `, bookingID, req.PaymentType, amountPounds, idempotencyKey, adminID).Scan(&paymentID) + `, bookingID, req.PaymentType, amountPounds, idempotencyKey, adminID, giftCardPaymentID).Scan(&paymentID) if err != nil { log.Printf("Failed to create giftcard payment record: %v", err) http.Error(w, "internal server error", http.StatusInternalServerError) @@ -685,7 +707,25 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // completed (the cancellation refund path computes refunds from // completed payments). Mark the row failed and alert ops: money was // taken at Square and MUST be refunded manually. - recheckStatus, payable, err := recheckBookingPayable(r.Context(), db.Conn, bookingID) + // + // The recheck and the status write run in ONE transaction so the + // FOR UPDATE row lock taken inside recheckBookingPayable persists to + // commit (C5) — a concurrent cancellation cannot commit a cancelled + // status between the recheck and the payments UPDATE. + recheckTx, reTxErr := db.Conn.Begin(r.Context()) + if reTxErr != nil { + log.Printf("CRITICAL: Square payment %s was processed for booking %s but opening the post-charge recheck transaction failed: %v — manual reconciliation required", + paymentResult.SquarePayID, bookingID, reTxErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + defer func() { + if err := recheckTx.Rollback(r.Context()); err != nil && !errors.Is(err, pgx.ErrTxClosed) { + slog.Error("failed to rollback post-charge recheck transaction", "err", err) + } + }() + + recheckStatus, payable, err := recheckBookingPayable(r.Context(), recheckTx, bookingID) if err != nil { log.Printf("CRITICAL: Square payment %s was processed for booking %s but re-reading booking status failed: %v — manual reconciliation required", paymentResult.SquarePayID, bookingID, err) @@ -695,15 +735,19 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { if !payable { log.Printf("CRITICAL: Square payment %s was processed but booking %s is now %q — marking saved-card payment %s failed; money taken at Square MUST be refunded manually", paymentResult.SquarePayID, bookingID, recheckStatus, paymentID) - if _, upErr := db.Conn.Exec(r.Context(), `UPDATE payments SET status = 'failed' WHERE id = $1`, paymentID); upErr != nil { + if _, upErr := recheckTx.Exec(r.Context(), `UPDATE payments SET status = 'failed' WHERE id = $1`, paymentID); upErr != nil { log.Printf("CRITICAL: Square payment %s landed on %q booking %s but marking payment %s failed errored: %v — manual reconciliation required", paymentResult.SquarePayID, recheckStatus, bookingID, paymentID, upErr) } + if cErr := recheckTx.Commit(r.Context()); cErr != nil { + log.Printf("CRITICAL: Square payment %s landed on %q booking %s and committing the failed mark errored: %v — manual reconciliation required", + paymentResult.SquarePayID, recheckStatus, bookingID, cErr) + } http.Error(w, "This booking is no longer accepting payments", http.StatusConflict) return } - if _, upErr := db.Conn.Exec(r.Context(), + if _, upErr := recheckTx.Exec(r.Context(), `UPDATE payments SET status = 'completed', square_payment_id = $1 WHERE id = $2`, paymentResult.SquarePayID, paymentID, ); upErr != nil { @@ -711,6 +755,12 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal server error", http.StatusInternalServerError) return } + if cErr := recheckTx.Commit(r.Context()); cErr != nil { + log.Printf("CRITICAL: Square payment %s succeeded but committing the post-charge status update for payment %s failed: %v — manual reconciliation required", + paymentResult.SquarePayID, paymentID, cErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } // Return the card details the frontend reads for the success state // (MINOR-R2) — CheckoutResponse alone leaves card_brand/card_last4 blank. @@ -1074,7 +1124,10 @@ func GetCheckoutStatus(w http.ResponseWriter, r *http.Request) { // at Square and MUST be refunded manually (mirrors CreateBookingPayment's // post-charge recheck). var recheckStatus string - if err := tx.QueryRow(r.Context(), `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&recheckStatus); err != nil { + // FOR UPDATE (C5): serializes against the cancellation path's lock on + // the same row so a concurrent cancellation cannot commit between this + // recheck and the transaction commit below. + if err := tx.QueryRow(r.Context(), `SELECT status FROM bookings WHERE id = $1 FOR UPDATE`, bookingID).Scan(&recheckStatus); err != nil { log.Printf("CRITICAL: Square payment %s for checkout %s was processed but re-reading booking %s status failed: %v — manual reconciliation required", paymentResult.SquarePayID, checkoutID, bookingID, err) http.Error(w, "internal server error", http.StatusInternalServerError) @@ -2009,6 +2062,13 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { return } + // N4: Square's refund-reason limit is 192 chars — a longer reason 400s at + // Square and would be misclassified as a definitive decline. Reject early. + if len(req.Reason) > 192 { + http.Error(w, "Refund reason must be 192 characters or less", http.StatusBadRequest) + return + } + if err := ValidateAmount(req.Amount); err != nil { log.Printf("Failed to process request: %v", err) http.Error(w, "Invalid request", http.StatusBadRequest) @@ -2121,13 +2181,20 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { `, idempotencyKey).Scan(&existingRefundID, &existingRefundStatus, &existingRefundAmount, &existingRefundOrigin, &existingRefundReason, &existingRefundCreatedAt, &existingRefundKey) switch { case err == nil && existingRefundStatus.String == "completed": + // C4: same-key dedup must report the STORED refund, never the newly + // requested amount — echoing req.Amount on a different-amount retry + // misleads the admin into believing the new amount was refunded. + createdAt := clock.Now() + if existingRefundCreatedAt.Valid { + createdAt = existingRefundCreatedAt.Time + } if err := json.NewEncoder(w).Encode(RefundResponse{ ID: existingRefundID.String, PaymentID: paymentID, - Amount: req.Amount, + Amount: int64(math.Round(existingRefundAmount.Float64 * 100)), Status: "completed", - Reason: req.Reason, - CreatedAt: clock.Now().Format(time.RFC3339), + Reason: existingRefundReason.String, + CreatedAt: createdAt.Format(time.RFC3339), }); err != nil { log.Printf("Failed to encode JSON response: %v", err) } @@ -2169,10 +2236,10 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { if err := json.NewEncoder(w).Encode(RefundResponse{ ID: existingRefundID.String, PaymentID: paymentID, - Amount: req.Amount, + Amount: resumeAmount, Status: "completed", - Reason: req.Reason, - CreatedAt: clock.Now().Format(time.RFC3339), + Reason: existingRefundReason.String, + CreatedAt: existingRefundCreatedAt.Time.Format(time.RFC3339), }); err != nil { log.Printf("Failed to encode JSON response: %v", err) } @@ -2216,9 +2283,9 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { if err := json.NewEncoder(w).Encode(RefundResponse{ ID: existingRefundID.String, PaymentID: paymentID, - Amount: req.Amount, + Amount: resumeAmount, Status: reissueStatus, - Reason: req.Reason, + Reason: existingRefundReason.String, CreatedAt: clock.Now().Format(time.RFC3339), }); err != nil { log.Printf("Failed to encode JSON response: %v", err) @@ -2231,9 +2298,9 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { if err := json.NewEncoder(w).Encode(RefundResponse{ ID: existingRefundID.String, PaymentID: paymentID, - Amount: req.Amount, + Amount: resumeAmount, Status: "completed", - Reason: req.Reason, + Reason: existingRefundReason.String, CreatedAt: clock.Now().Format(time.RFC3339), }); err != nil { log.Printf("Failed to encode JSON response: %v", err) @@ -2841,7 +2908,25 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { // would silently exclude it. Mark the tip row failed and alert ops: money // was taken at Square and MUST be refunded manually (mirrors // CreateBookingPayment's post-charge recheck). - tipRecheckStatus, tipPayable, err := recheckBookingPayable(r.Context(), db.Conn, bookingID) + // + // The recheck and the status write run in ONE transaction so the + // FOR UPDATE row lock taken inside recheckBookingPayable persists to + // commit (C5) — a concurrent cancellation cannot commit a cancelled + // status between the recheck and the payments UPDATE. + recheckTx, reTxErr := db.Conn.Begin(r.Context()) + if reTxErr != nil { + log.Printf("CRITICAL: Square tip payment %s (ID=%s) was processed but opening the post-charge recheck transaction failed: %v — manual reconciliation required", + paymentResult.Status, paymentResult.SquarePayID, reTxErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + defer func() { + if err := recheckTx.Rollback(r.Context()); err != nil && !errors.Is(err, pgx.ErrTxClosed) { + slog.Error("failed to rollback post-charge recheck transaction", "err", err) + } + }() + + tipRecheckStatus, tipPayable, err := recheckBookingPayable(r.Context(), recheckTx, bookingID) if err != nil { log.Printf("CRITICAL: Square tip payment %s (ID=%s) was processed but re-reading booking %s status failed: %v — manual reconciliation required", paymentResult.Status, paymentResult.SquarePayID, bookingID, err) @@ -2851,26 +2936,35 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { if !tipPayable { log.Printf("CRITICAL: Square tip payment %s (ID=%s) for booking %s was processed but booking is now %q — marking tip %s failed; money taken at Square MUST be refunded manually", paymentResult.Status, paymentResult.SquarePayID, bookingID, tipRecheckStatus, paymentID) - if _, upErr := db.Conn.Exec(r.Context(), `UPDATE payments SET status = 'failed' WHERE id = $1`, paymentID); upErr != nil { + if _, upErr := recheckTx.Exec(r.Context(), `UPDATE payments SET status = 'failed' WHERE id = $1`, paymentID); upErr != nil { log.Printf("CRITICAL: Square tip payment %s (ID=%s) landed on %q booking %s but marking tip %s failed errored: %v — manual reconciliation required", paymentResult.Status, paymentResult.SquarePayID, tipRecheckStatus, bookingID, paymentID, upErr) } + if cErr := recheckTx.Commit(r.Context()); cErr != nil { + log.Printf("CRITICAL: Square tip payment %s (ID=%s) landed on %q booking %s and committing the failed mark errored: %v — manual reconciliation required", + paymentResult.Status, paymentResult.SquarePayID, tipRecheckStatus, bookingID, cErr) + } http.Error(w, "This booking is no longer accepting tips", http.StatusConflict) return } // Step 3: Square succeeded — update the payment record. - _, upErr := db.Conn.Exec(r.Context(), + if _, upErr := recheckTx.Exec(r.Context(), `UPDATE payments SET status = 'completed', square_payment_id = $1 WHERE id = $2`, paymentResult.SquarePayID, paymentID, - ) - if upErr != nil { + ); upErr != nil { log.Printf("Failed to update payment %s after Square success: %v (square_payment_id=%s)", paymentID, upErr, paymentResult.SquarePayID) // Square charge succeeded but status update failed. // Record stays 'pending' for manual reconciliation. http.Error(w, "internal server error", http.StatusInternalServerError) return } + if cErr := recheckTx.Commit(r.Context()); cErr != nil { + log.Printf("CRITICAL: Square tip payment %s (ID=%s) succeeded but committing the post-charge status update for payment %s failed: %v — manual reconciliation required", + paymentResult.Status, paymentResult.SquarePayID, paymentID, cErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } if err := json.NewEncoder(w).Encode(PaymentResponse{ ID: paymentID, diff --git a/backend/handlers/payments/refunds.go b/backend/handlers/payments/refunds.go index 9e0211c..84da6c1 100644 --- a/backend/handlers/payments/refunds.go +++ b/backend/handlers/payments/refunds.go @@ -168,6 +168,7 @@ func ProcessCancellationRefundTx( // Get the booking's user info for refund routing. var bookingUserID string var isGuest bool + bookingUserLookupFailed := false if err := tx.QueryRow(ctx, ` SELECT b.user_id, COALESCE(u.account_role = 'guest', false) FROM bookings b @@ -175,7 +176,11 @@ func ProcessCancellationRefundTx( WHERE b.id = $1 `, bookingID).Scan(&bookingUserID, &isGuest); err != nil { log.Printf("Failed to get booking user info for refund: %v", err) - // Non-fatal — we'll still process Square refunds but skip balance credits. + // Non-fatal for Square refunds (which route by payment, not user), but + // the balance-credit branches below must know the lookup FAILED (vs a + // genuine guest) so they never record a 'completed' refund when no + // money could be credited — see creditFailed. + bookingUserLookupFailed = true } rows, err := tx.Query(ctx, ` @@ -268,6 +273,11 @@ func ProcessCancellationRefundTx( refundThisPayment := math.Min(residual, refundRemaining) var squareRefundID *string + // creditFailed records that money for this payment did NOT actually + // move (deleted gift card, failed balance credit) so the refund record + // is inserted 'failed' instead of claiming a completed refund (M2). + creditFailed := false + switch paymentMethod { case "online_square", "in_person_card": // Square API refund is processed AFTER the transaction commits @@ -280,7 +290,38 @@ func ProcessCancellationRefundTx( case "giftcard": if giftCardID == nil || *giftCardID == "" { - log.Printf("Giftcard payment %s has no gift_card_id — cannot refund to card. Skipping.", paymentID) + // A giftcard payment with no gift_card_id was made from the + // user's gift-card ACCOUNT balance (the terminal giftcard path + // stores no gift_card_id for balance payments). Credit the + // booking user's balance back — skipping would lose the money + // while the completed refund row claims it was returned (C3). + // Guests get no balance credit, mirroring the cash branch. + if isGuest || bookingUserID == "" { + if isGuest { + log.Printf("Guest giftcard refund: booking %s, payment %s, amount £%.2f — no balance credit", bookingID, paymentID, refundThisPayment) + } else { + log.Printf("Giftcard payment %s has no gift_card_id and no booking user — cannot refund. Skipping.", paymentID) + } + if bookingUserLookupFailed { + // The user may well exist — the lookup failed + // transiently, so 'completed' would claim money was + // credited when none could be. Mark the record failed + // for admin reconciliation instead. + creditFailed = true + } + break + } + log.Printf("Crediting £%.2f to user %s gift-card balance for account-balance giftcard payment %s", refundThisPayment, bookingUserID, paymentID) + if _, balErr := tx.Exec(ctx, ` + INSERT INTO user_giftcard_balances (user_id, balance, updated_at) + VALUES ($1, $2, NOW()) + ON CONFLICT (user_id) DO UPDATE SET + balance = user_giftcard_balances.balance + EXCLUDED.balance, + updated_at = NOW() + `, bookingUserID, refundThisPayment); balErr != nil { + log.Printf("Failed to credit user %s gift-card balance for refund of booking %s: %v", bookingUserID, bookingID, balErr) + creditFailed = true + } break } // expiry_date is maintained by EVERY gift-card write that counts as @@ -305,11 +346,21 @@ func ProcessCancellationRefundTx( log.Printf("Failed to query gift card expiry months (using default %d): %v", defaultGiftCardExpiryMonths, expiryErr) gcExpiryMonths = defaultGiftCardExpiryMonths } - if _, err := tx.Exec(ctx, ` + gcTag, gcErr := tx.Exec(ctx, ` UPDATE gift_cards SET amount_remaining = amount_remaining + $1, last_used_at = NOW(), expiry_date = NOW() + ($3 * INTERVAL '1 month') WHERE id = $2 - `, refundThisPayment, *giftCardID, gcExpiryMonths); err != nil { - log.Printf("Failed to refund £%.2f to gift card %s: %v", refundThisPayment, *giftCardID, err) + `, refundThisPayment, *giftCardID, gcExpiryMonths) + if gcErr != nil { + log.Printf("Failed to refund £%.2f to gift card %s: %v", refundThisPayment, *giftCardID, gcErr) + creditFailed = true + break + } + if gcTag.RowsAffected() == 0 { + // M2: the gift card no longer exists — the UPDATE matched 0 rows + // and no money moved. Marking the refund 'completed' would claim + // money was returned when it wasn't. + slog.Error("CRITICAL: gift card refund UPDATE affected 0 rows — card deleted?, refund NOT credited", "gift_card_id", *giftCardID, "payment_id", paymentID, "booking_id", bookingID, "amount", refundThisPayment) + creditFailed = true break } if _, err := tx.Exec(ctx, ` @@ -321,17 +372,28 @@ func ProcessCancellationRefundTx( case "cash": if isGuest || bookingUserID == "" { - log.Printf("Guest cash refund: booking %s, payment %s, amount £%.2f — admin must process cash refund at till", bookingID, paymentID, refundThisPayment) + if isGuest { + log.Printf("Guest cash refund: booking %s, payment %s, amount £%.2f — admin must process cash refund at till", bookingID, paymentID, refundThisPayment) + } else { + log.Printf("Cash refund: payment %s has no booking user — skipping balance credit", paymentID) + } + if bookingUserLookupFailed { + // The booking-user lookup errored, so this may be a real + // user whose balance credit was skipped — a 'completed' + // refund would claim money was returned when it wasn't. + creditFailed = true + } } else { log.Printf("Crediting £%.2f to user %s balance for cash payment %s", refundThisPayment, bookingUserID, paymentID) if _, balErr := tx.Exec(ctx, ` - INSERT INTO user_giftcard_balances (user_id, balance, updated_at) - VALUES ($1, $2, NOW()) - ON CONFLICT (user_id) DO UPDATE SET - balance = user_giftcard_balances.balance + EXCLUDED.balance, - updated_at = NOW() - `, bookingUserID, refundThisPayment); balErr != nil { + INSERT INTO user_giftcard_balances (user_id, balance, updated_at) + VALUES ($1, $2, NOW()) + ON CONFLICT (user_id) DO UPDATE SET + balance = user_giftcard_balances.balance + EXCLUDED.balance, + updated_at = NOW() + `, bookingUserID, refundThisPayment); balErr != nil { log.Printf("Failed to credit user %s balance for refund of booking %s: %v", bookingUserID, bookingID, balErr) + creditFailed = true } } @@ -343,6 +405,9 @@ func ProcessCancellationRefundTx( if paymentMethod == "online_square" || paymentMethod == "in_person_card" { recordStatus = "pending" } + if creditFailed { + recordStatus = "failed" + } record := RefundRecord{ PaymentID: paymentID, BookingID: bookingID, @@ -388,9 +453,11 @@ func ProcessCancellationRefundTx( if err := tx.QueryRow(ctx, "SELECT EXISTS(SELECT 1 FROM booking_discounts WHERE booking_id = $1 AND discount_source = 'loyalty')", bookingID).Scan(&loyaltyUsed); err != nil { log.Printf("Failed to check loyalty stamp refund for booking %s: %v", bookingID, err) } else if loyaltyUsed { - _, loyaltyErr := tx.Exec(ctx, "UPDATE users SET loyalty_stamps = loyalty_stamps + $1 WHERE id = $2", LoyaltyStampCost, bookingUserID) + loyaltyTag, loyaltyErr := tx.Exec(ctx, "UPDATE users SET loyalty_stamps = loyalty_stamps + $1 WHERE id = $2", LoyaltyStampCost, bookingUserID) if loyaltyErr != nil { log.Printf("Failed to refund loyalty stamps for booking %s: %v", bookingID, loyaltyErr) + } else if loyaltyTag.RowsAffected() == 0 { + slog.Error("CRITICAL: loyalty stamp refund UPDATE affected 0 rows — user not found", "user_id", bookingUserID, "booking_id", bookingID) } else { log.Printf("Refunded %d loyalty stamps to user %s after cancellation of booking %s", LoyaltyStampCost, bookingUserID, bookingID) } @@ -447,8 +514,8 @@ func ProcessCancellationRefund( } if cErr := tx.Commit(ctx); cErr != nil { - log.Printf("CRITICAL: Failed to commit cancellation refund transaction for booking %s: %v", bookingID, cErr) - return &calc, nil + slog.Error("CRITICAL: failed to commit cancellation refund", "booking_id", bookingID, "err", cErr) + return nil, fmt.Errorf("failed to commit cancellation refund: %w", cErr) } // Process pending Square refunds after the transaction commits successfully. @@ -885,29 +952,31 @@ func processChargeGroup(ctx context.Context, chargeID string, rows []pendingChar } } - // ONE Square refund per charge with a SET-STABLE idempotency key. The key - // is derived from the sha256 of the SORTED ids of the pending rows being - // aggregated — computed from `pending`, the rows re-read UNDER the lock, - // NEVER the caller-supplied `rows` slice (which may be stale by the time - // the lock is held). The key is used ONLY for the Square call — never - // stored in refunds.idempotency_key (the per-row keys remain the audit - // trail). + // ONE Square refund per charge with a CHARGE-STABLE idempotency key. The + // key is derived from the charge ID (square_payment_id) ONLY — NEVER from + // the set of pending row IDs — so it is identical across every sweep run + // no matter how the pending set evolves. The key is used ONLY for the + // Square call — never stored in refunds.idempotency_key (the per-row keys + // remain the audit trail). // - // SAME-set retry (a crash/response-loss where the pending set is unchanged) - // hashes to the SAME key → Square's idempotency dedup returns the ORIGINAL - // refund, so a retry can never double-refund. A CHANGED set (a new - // cancellation refund row joined the group while the old rows still sit - // 'pending' — the CRITICAL-log path where the post-refund DB UPDATE failed) - // hashes to a NEW key → Square issues a fresh refund covering the new - // total, so the new row is NEVER marked 'completed' against an old smaller - // refund with no money actually moved (the under-refund / lost-money - // bookkeeping bug). The 23h age-guard reconcile above still protects the - // cross-sweep case where Square's finite (~24h) key retention may have - // lapsed. + // SAME-set retry (a crash/response-loss where the pending set is + // unchanged) → SAME key → Square's idempotency dedup returns the + // ORIGINAL refund, so a retry can never double-refund. + // + // CHANGED set (a new cancellation refund row joined the group while the + // old rows still sit 'pending' — the CRITICAL-log path where the + // post-refund DB UPDATE failed) → STILL the SAME key. Money for the + // original total has ALREADY moved at Square, so a set-derived key would + // have hashed to a NEW key and issued a SECOND Square refund on top — + // double-refunding the customer (C6). Deduping on the charge key returns + // the original refund instead; the new row resolves against it and any + // residual gap is a known, admin-visible shortfall rather than lost + // money. The 23h age-guard reconcile above still protects the cross-sweep + // case where Square's finite (~24h) key retention may have lapsed. sqResult, sqErr := SquareClient.RefundPayment(ctx, square.RefundPaymentReq{ PaymentID: chargeID, Amount: totalCents, - IdempotencyKey: chargeAggKey(chargeID, idsOf(pending)), + IdempotencyKey: chargeAggKey(chargeID), Reason: reason, }) switch { @@ -1026,12 +1095,11 @@ func idsOf(rows []pendingChargeRow) []string { return ids } -// aggRefundKeySuffix returns a deterministic 12-hex-char suffix identifying -// the exact set of pending refund rows being aggregated. The sorted row IDs -// (CHAR(12), so plain concatenation is unambiguous) are sha256'd and truncated: -// the SAME set always yields the SAME suffix — keeping Square's idempotency-key -// dedup for a same-set crash-retry — while a CHANGED set yields a DIFFERENT -// suffix, so a new row can never resolve against an old smaller refund. +// aggRefundKeySuffix returns a deterministic 12-hex-char suffix for a set of +// identifiers. The identifiers (CHAR(12) refund ids, or an over-length +// square_payment_id) are sorted, sha256'd and truncated, so the SAME input +// always yields the SAME suffix. Used to compress an over-length chargeID into +// a fixed-width prefix for the charge-level idempotency key (see chargeAggKey). func aggRefundKeySuffix(ids []string) string { sorted := append([]string(nil), ids...) sort.Strings(sorted) @@ -1040,22 +1108,24 @@ func aggRefundKeySuffix(ids []string) string { } // chargeAggKey builds the charge-level idempotency key for an aggregated -// refund as -square-agg-. Square's idempotency-key limit -// is 45 chars; the verbatim form needs the chargeID ≤21 chars. square_payment_id -// is an arbitrary TEXT column holding Square's real payment ID (typically 20-28 -// chars), so the verbatim form can exceed the limit — and a >45-char key is -// rejected with a 400 INVALID_REQUEST_ERROR (classified ambiguous → stuck -// pending forever). When the verbatim form does not fit, the chargeID is -// sha256'd into a fixed-width prefix instead — NEVER truncated verbatim: two -// charges sharing a truncated prefix would collide on Square's global key dedup -// and silently swallow the second charge's refund (lost money). -func chargeAggKey(chargeID string, ids []string) string { - suffix := aggRefundKeySuffix(ids) - key := chargeID + "-square-agg-" + suffix +// refund as -square-agg. The key depends ONLY on the charge ID — +// NEVER on the set of pending row IDs — so it is stable across sweep runs even +// when a new cancellation refund row joins the group (see processChargeGroup). +// Square's idempotency-key limit is 45 chars; the verbatim form needs the +// chargeID ≤34 chars. square_payment_id is an arbitrary TEXT column holding +// Square's real payment ID (typically 20-28 chars), so the verbatim form can +// exceed the limit — and a >45-char key is rejected with a 400 +// INVALID_REQUEST_ERROR (classified ambiguous → stuck pending forever). When +// the verbatim form does not fit, the chargeID is sha256'd into a fixed-width +// prefix instead — NEVER truncated verbatim: two charges sharing a truncated +// prefix would collide on Square's global key dedup and silently swallow the +// second charge's refund (lost money). +func chargeAggKey(chargeID string) string { + key := chargeID + "-square-agg" if len(key) <= 45 { return key } - return aggRefundKeySuffix([]string{chargeID}) + "-square-agg-" + suffix + return aggRefundKeySuffix([]string{chargeID}) + "-square-agg" } // manualPendingRow is one stale manual refund row eligible for the sweep's diff --git a/backend/handlers/payments/refunds_test.go b/backend/handlers/payments/refunds_test.go index d84758a..68fba33 100644 --- a/backend/handlers/payments/refunds_test.go +++ b/backend/handlers/payments/refunds_test.go @@ -41,14 +41,14 @@ func (c *countingRefundClient) refundCalls() []square.RefundPaymentReq { } // assertAggKey asserts the charge-level aggregated refund key carries the -// "-square-agg-" segment and stays within Square's 45-char idempotency-key +// "-square-agg" segment and stays within Square's 45-char idempotency-key // limit. The prefix is the verbatim chargeID for short square_payment_ids and a // hashed fixed-width form for long ones (see chargeAggKey), so only the shared // segment + length bound are asserted here. func assertAggKey(t *testing.T, got, chargeID string) { t.Helper() - if !strings.Contains(got, "-square-agg-") { - t.Errorf("expected charge-level idempotency key containing \"-square-agg-\", got %q", got) + if !strings.Contains(got, "-square-agg") { + t.Errorf("expected charge-level idempotency key containing \"-square-agg\", got %q", got) } if len(got) > 45 { t.Errorf("expected idempotency key within Square's 45-char limit, got %d chars: %q", len(got), got) @@ -2623,6 +2623,51 @@ func TestCancellationRefund_SerializesAgainstManualRefund(t *testing.T) { } } +// TestRefundPayment_ReasonTooLong_Rejected verifies the N4 fix: a refund +// reason longer than Square's 192-char cap is rejected with 400 before any +// refund row is created — a longer reason would 400 at Square and be +// misclassified as a definitive decline, misleading the admin. +func TestRefundPayment_ReasonTooLong_Rejected(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin 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) + } + paymentID, err := fixtures.CreateTestPayment(tx, bookingID, 50.00, "online_square", "full", "completed") + if err != nil { + t.Fatalf("failed to create payment: %v", err) + } + + adminToken := jwt.GenerateTestToken(adminID, "admin") + req := RefundRequest{Amount: 1000, Reason: strings.Repeat("x", 193)} + rec := makePaymentRequest(RefundPayment, "POST", "/api/admin/payments/"+paymentID+"/refund", req, adminToken, ctx) + if rec.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for a 193-char reason, got %d: %s", rec.Code, rec.Body.String()) + } + + var refundCount int + if err := tx.QueryRow(ctx, "SELECT COUNT(*) FROM refunds WHERE payment_id = $1", paymentID).Scan(&refundCount); err != nil { + t.Fatalf("failed to count refunds: %v", err) + } + if refundCount != 0 { + t.Errorf("expected 0 refund rows, got %d", refundCount) + } +} + // ============================================================================= // D2 — the sweep retries stale MANUAL pending refunds with their own key // ============================================================================= @@ -3337,21 +3382,22 @@ func TestSweepPendingSquareRefunds_ManualWithSquareRefundID_NoMatch_Failed(t *te } // ============================================================================= -// Bug fix — set-stable charge-level idempotency key (refunds.go processChargeGroup) +// Bug fix — charge-stable idempotency key (refunds.go processChargeGroup, C6) // ============================================================================= // TestChargeAggKey_AlwaysFitsAndDeterministic locks the charge-level idempotency // key builder: the key must stay within Square's 45-char limit for ANY -// square_payment_id length (the verbatim form only fits chargeIDs ≤21 chars; +// square_payment_id length (the verbatim form only fits chargeIDs ≤34 chars; // longer ones fall back to a hashed fixed-width prefix — see chargeAggKey), be -// deterministic for a given (charge, set), and differ across charges/sets. +// deterministic for a given charge, be stable regardless of the pending row set +// (C6 — the key must NOT change when new rows join the group), and differ +// across charges. func TestChargeAggKey_AlwaysFitsAndDeterministic(t *testing.T) { - ids := []string{"id1", "id2"} longCharge := "sqp_real_square_payment_id_that_is_quite_long" // Short chargeID → verbatim form. - shortKey := chargeAggKey("sqp_short", ids) - if !strings.HasPrefix(shortKey, "sqp_short-square-agg-") { + shortKey := chargeAggKey("sqp_short") + if !strings.HasPrefix(shortKey, "sqp_short-square-agg") { t.Errorf("expected verbatim prefix for a short chargeID, got %q", shortKey) } if len(shortKey) > 45 { @@ -3359,42 +3405,41 @@ func TestChargeAggKey_AlwaysFitsAndDeterministic(t *testing.T) { } // Long chargeID → still ≤45, never the verbatim form. - longKey := chargeAggKey(longCharge, ids) - if strings.HasPrefix(longKey, longCharge+"-square-agg-") { + longKey := chargeAggKey(longCharge) + if strings.HasPrefix(longKey, longCharge+"-square-agg") { t.Errorf("expected long chargeID NOT embedded verbatim, got %q", longKey) } if len(longKey) > 45 { t.Errorf("expected long-charge key within 45 chars, got %d: %q", len(longKey), longKey) } - // Same (charge, set) → same key; different set → different key; different - // charge → different key (no cross-charge dedup collision). - if chargeAggKey("sqp_short", ids) != shortKey { - t.Error("expected the same (charge, set) to produce the same key") + // Same charge → same key (deterministic AND set-stable: the pending row set + // is never part of the key, so a new row joining the group cannot change it + // — C6); different charge → different key (no cross-charge dedup collision). + if chargeAggKey("sqp_short") != shortKey { + t.Error("expected the same charge to produce the same key") } - if chargeAggKey("sqp_short", []string{"id1"}) == shortKey { - t.Error("expected a changed set to produce a different key") + if chargeAggKey(longCharge) != longKey { + t.Error("expected the same long charge to produce the same key") } - if chargeAggKey(longCharge, []string{"id1"}) == longKey { - t.Error("expected a changed set to produce a different key (long charge)") - } - if chargeAggKey("sqp_other", ids) == shortKey { + if chargeAggKey("sqp_other") == shortKey { t.Error("expected a different charge to produce a different key") } - if chargeAggKey(longCharge+"x", ids) == longKey { + if chargeAggKey(longCharge+"x") == longKey { t.Error("expected a different long charge to produce a different key") } } -// TestProcessChargeGroup_ChangedPendingSet_NewKey verifies the idempotency-key -// bug fix: the charge-level aggregated refund key is derived from the SET of -// pending row ids. A SAME-set retry (the CRITICAL-log path where the post-refund -// DB UPDATE failed, leaving the rows 'pending') reuses the SAME key → Square's -// dedup returns the ORIGINAL refund. When a NEW cancellation refund row joins -// the group, the set changes → a NEW key → Square issues a fresh refund covering -// the new total, so the new row is NEVER marked 'completed' against the old -// smaller refund (the under-refund / lost-money bookkeeping bug). -func TestProcessChargeGroup_ChangedPendingSet_NewKey(t *testing.T) { +// TestProcessChargeGroup_ChangedPendingSet_NoDoubleRefund verifies the C6 +// idempotency-key fix: the charge-level aggregated refund key is derived from +// the charge ID ONLY, never from the set of pending row ids. A SAME-set retry +// (the CRITICAL-log path where the post-refund DB UPDATE failed, leaving the +// rows 'pending') reuses the SAME key → Square's dedup returns the ORIGINAL +// refund. When a NEW cancellation refund row joins the group, the set changes +// but the key STAYS THE SAME → Square's dedup STILL returns the ORIGINAL refund +// instead of issuing a second refund on top of money that already moved (the +// C6 double-refund bug). +func TestProcessChargeGroup_ChangedPendingSet_NoDoubleRefund(t *testing.T) { ctx, tx := testutils.SetupTestTx(t) userID, err := fixtures.CreateTestUser(tx) @@ -3518,25 +3563,24 @@ func TestProcessChargeGroup_ChangedPendingSet_NewKey(t *testing.T) { t.Fatalf("failed to reset refund rows pending: %v", err) } - // Run 3: the three-row set → key K2 ≠ K1 → a NEW Square refund for the full - // £75 — never deduped against the old £50 refund. + // Run 3: the three-row set → the SAME charge-stable key K1. Square's dedup + // returns the ORIGINAL £50 refund — a fresh £75 refund would have + // double-refunded the customer on top of money that already moved (C6, the + // old set-derived-key bug). The new row resolves against the original + // refund; no second Square refund is ever issued. key2 := runGroup() - if key2 == key1 { - t.Errorf("expected CHANGED set to yield a DIFFERENT key, got the same %q", key2) + if key2 != key1 { + t.Errorf("expected CHANGED set to reuse the charge-stable key %q, got %q", key1, key2) } calls = counting.refundCalls() if len(calls) != 3 { - t.Fatalf("expected 3 total Square refund calls, got %d", len(calls)) + t.Fatalf("expected 3 total Square refund calls (all dedup hits), got %d", len(calls)) } - if calls[2].Amount != 7500 { - t.Errorf("expected the changed-set refund of 7500 pence (£25+£25+£25), got %d", calls[2].Amount) - } - if n := mock.RefundKeyCount(); n != 2 { - t.Errorf("expected Square to have issued exactly 2 distinct refunds, got %d", n) + if n := mock.RefundKeyCount(); n != 1 { + t.Errorf("expected Square to have issued exactly 1 distinct refund (dedup), got %d", n) } - // All three rows resolve to completed; the new row is completed against the - // NEW refund, never the old smaller one. + // All three rows resolve to completed against the ORIGINAL refund. var newRefundID string var newStatus string if err := db.Conn.QueryRow(freshCtx, `SELECT status, square_refund_id FROM refunds WHERE id = $1`, row3).Scan(&newStatus, &newRefundID); err != nil { @@ -3545,8 +3589,8 @@ func TestProcessChargeGroup_ChangedPendingSet_NewKey(t *testing.T) { if newStatus != "completed" { t.Errorf("expected third refund status 'completed', got %q", newStatus) } - if newRefundID == "" || newRefundID == oldRefundID { - t.Errorf("expected third refund completed against the NEW refund (not the old %q), got %q", oldRefundID, newRefundID) + if newRefundID != oldRefundID { + t.Errorf("expected third refund completed against the ORIGINAL refund %q, got %q", oldRefundID, newRefundID) } var completedCount int if err := db.Conn.QueryRow(freshCtx, `SELECT COUNT(*) FROM refunds WHERE id = ANY($1) AND status = 'completed'`, []string{row1, row2, row3}).Scan(&completedCount); err != nil { diff --git a/backend/handlers/payments/till.go b/backend/handlers/payments/till.go index 88f6dce..67e97cf 100644 --- a/backend/handlers/payments/till.go +++ b/backend/handlers/payments/till.go @@ -333,14 +333,26 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { if req.IdempotencyKey != "" { var existingID, existingStatus, existingItemID string var existingTotal float64 - err := db.Conn.QueryRow(ctx, `SELECT id, status, item_id, total_amount FROM till_sales WHERE idempotency_key = $1`, req.IdempotencyKey).Scan(&existingID, &existingStatus, &existingItemID, &existingTotal) + var existingItemType, existingPaymentMethod string + err := db.Conn.QueryRow(ctx, `SELECT id, status, item_id, total_amount, item_type, payment_method FROM till_sales WHERE idempotency_key = $1`, req.IdempotencyKey).Scan(&existingID, &existingStatus, &existingItemID, &existingTotal, &existingItemType, &existingPaymentMethod) if err == nil { if existingStatus == "completed" { + // C4-class: a same-key retry of a COMPLETED sale must report the + // STORED sale (amount, item type, method), never the freshly + // requested fields — echoing req.Amount on a different-amount + // retry misleads the till into believing the new amount was + // sold. Guard the amount exactly as the pending path below does: + // a different amount is a genuinely different sale, not a retry. + if int64(math.Round(existingTotal*100)) != int64(math.Round(req.Amount*100)) { + log.Printf("Till-sale dedup amount mismatch: completed record %s has %.2f, request has %.2f", existingID, existingTotal, req.Amount) + http.Error(w, "Amount does not match the completed till sale", http.StatusBadRequest) + return + } if err := json.NewEncoder(w).Encode(TillSaleResponse{ ID: existingID, - ItemType: req.ItemType, - TotalAmount: req.Amount, - PaymentMethod: req.PaymentMethod, + ItemType: existingItemType, + TotalAmount: existingTotal, + PaymentMethod: existingPaymentMethod, Status: "completed", }); err != nil { log.Printf("Failed to encode JSON response: %v", err) @@ -935,7 +947,7 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { } // Square succeeded — update the till_sale record. - _, upErr := db.Conn.Exec(ctx, + tillTag, upErr := db.Conn.Exec(ctx, `UPDATE till_sales SET status = 'completed', square_payment_id = $1 WHERE id = $2`, paymentResult.SquarePayID, tillSaleID, ) @@ -944,6 +956,16 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal server error", http.StatusInternalServerError) return } + if tillTag.RowsAffected() == 0 { + // The stale-pending sweep (or a clawback) resolved the sale while + // the Square charge was in flight: the customer WAS charged and the + // gift card WAS funded, but the row no longer says 'pending'. + // Mirroring the non-Square pending-reuse path below, never report + // success for a row the DB doesn't agree on. + log.Printf("CRITICAL: Square payment %s succeeded but till_sale %s was already resolved (0 rows updated) — charge taken and card funded; MANUAL RECONCILIATION REQUIRED", paymentResult.SquarePayID, tillSaleID) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } saleStatus = "completed" } diff --git a/backend/handlers/payments/till_test.go b/backend/handlers/payments/till_test.go index f547301..d531abf 100644 --- a/backend/handlers/payments/till_test.go +++ b/backend/handlers/payments/till_test.go @@ -1424,6 +1424,155 @@ func TestCreateTillSale_PendingRetry_AmountMismatch_Rejected(t *testing.T) { } } +// TestCreateTillSale_CompletedDedup_AmountMismatch_Rejected verifies the +// C4-class dedup guard on the COMPLETED path (the C4 fix covered refunds and +// the till PENDING path, but the completed dedup echoed the freshly-requested +// amount and accepted any amount): a same-key retry of a completed sale with a +// different amount must be rejected (400), never silently reported as the new +// amount. +func TestCreateTillSale_CompletedDedup_AmountMismatch_Rejected(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + adminToken := jwt.GenerateTestToken(adminID, "admin") + + // Seed a COMPLETED till sale funded at £50.00. + key := "till-completed-dedup-mismatch-key" + var giftCardID string + err = tx.QueryRow(ctx, ` + INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by, is_inventory, voucher_type_at_purchase) + VALUES (50.00, 50.00, $1, FALSE, 'SPV') + RETURNING id + `, adminID).Scan(&giftCardID) + if err != nil { + t.Fatalf("failed to create gift card: %v", err) + } + _, err = tx.Exec(ctx, ` + INSERT INTO till_sales (item_type, item_id, description, quantity, unit_price, total_amount, + payment_method, status, user_id, square_payment_id, idempotency_key, created_by, created_at, updated_at) + VALUES ('gift_card', $1, 'Gift Card create', 1, 50.00, 50.00, 'online_square', 'completed', + NULL, 'sqp_completed_dedup', $2, $3, NOW(), NOW()) + `, giftCardID, key, adminID) + if err != nil { + t.Fatalf("failed to seed completed till sale: %v", err) + } + + // Retry with the same key but a different amount (£60 instead of £50). + reqBody := TillSaleRequest{ + ItemType: "gift_card", + Action: "create", + Amount: 60.00, + PaymentMethod: "saved_card", + IdempotencyKey: key, + } + bodyBytes, _ := json.Marshal(reqBody) + req := httptest.NewRequest("POST", "/api/admin/till/sale", bytes.NewReader(bodyBytes)) + req.Header.Set("Authorization", "Bearer "+adminToken) + req.Header.Set("Content-Type", "application/json") + 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) + + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400 (completed dedup amount mismatch), got %d. body: %s", w.Code, w.Body.String()) + } + + // The completed sale must be untouched. + var saleCount int + var saleStatus string + var saleAmount float64 + err = tx.QueryRow(ctx, `SELECT COUNT(*), MAX(status), COALESCE(MAX(total_amount), 0) FROM till_sales WHERE idempotency_key = $1`, key).Scan(&saleCount, &saleStatus, &saleAmount) + if err != nil { + t.Fatalf("failed to query till sale: %v", err) + } + if saleCount != 1 { + t.Errorf("expected 1 till sale, got %d", saleCount) + } + if saleStatus != "completed" { + t.Errorf("expected completed sale to remain completed, got %s", saleStatus) + } + if saleAmount != 50.00 { + t.Errorf("expected sale amount to remain 50.00, got %.2f", saleAmount) + } +} + +// TestCreateTillSale_CompletedDedup_ReturnsStoredAmount verifies the C4-class +// dedup fix on the COMPLETED till-sale path: a same-key retry reports the +// STORED sale (amount, payment method, item type), not the freshly-requested +// values — a retry with a different amount would otherwise mislead the till. +func TestCreateTillSale_CompletedDedup_ReturnsStoredAmount(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + adminToken := jwt.GenerateTestToken(adminID, "admin") + + key := "till-completed-dedup-stored-key" + var giftCardID string + err = tx.QueryRow(ctx, ` + INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by, is_inventory, voucher_type_at_purchase) + VALUES (50.00, 50.00, $1, FALSE, 'SPV') + RETURNING id + `, adminID).Scan(&giftCardID) + if err != nil { + t.Fatalf("failed to create gift card: %v", err) + } + _, err = tx.Exec(ctx, ` + INSERT INTO till_sales (item_type, item_id, description, quantity, unit_price, total_amount, + payment_method, status, user_id, square_payment_id, idempotency_key, created_by, created_at, updated_at) + VALUES ('gift_card', $1, 'Gift Card create', 1, 50.00, 50.00, 'online_square', 'completed', + NULL, 'sqp_completed_dedup_stored', $2, $3, NOW(), NOW()) + `, giftCardID, key, adminID) + if err != nil { + t.Fatalf("failed to seed completed till sale: %v", err) + } + + reqBody := TillSaleRequest{ + ItemType: "gift_card", + Action: "create", + Amount: 50.00, + PaymentMethod: "cash", // differs from the stored 'online_square' — must NOT be echoed + IdempotencyKey: key, + } + bodyBytes, _ := json.Marshal(reqBody) + req := httptest.NewRequest("POST", "/api/admin/till/sale", bytes.NewReader(bodyBytes)) + req.Header.Set("Authorization", "Bearer "+adminToken) + req.Header.Set("Content-Type", "application/json") + 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) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200 (completed dedup), got %d. body: %s", w.Code, w.Body.String()) + } + var resp TillSaleResponse + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("failed to decode response: %v", err) + } + if resp.TotalAmount != 50.00 { + t.Errorf("expected dedup response to report the STORED total 50.00, got %.2f", resp.TotalAmount) + } + if resp.PaymentMethod != "online_square" { + t.Errorf("expected dedup response to report the STORED payment method 'online_square', got %q", resp.PaymentMethod) + } + if resp.Status != "completed" { + t.Errorf("expected dedup response status 'completed', got %q", resp.Status) + } +} + // TestCreateTillSale_PendingRetry_Cash_CompletesRow verifies that a same-key // retry resolved by cash on a pending sale whose Square charge PROVABLY failed // (HIGH-3: a FAILED status means no money landed, so cash is safe) explicitly @@ -2272,8 +2421,11 @@ func makeTillSaleRequest(t *testing.T, req TillSaleRequest, adminToken string, c // the charge with the SAME idempotency key — Square dedups on the key and // returns the ORIGINAL payment — so the customer is charged exactly once, the // same row is reused, and the gift card is funded exactly once. +// This test asserts on a recording client swapped into the package-global +// SquareClient, so it must NOT run in parallel: a concurrent test swapping +// SquareClient to its own client can steal the recorded CreatePayment call +// (rec.keys stays empty → index panic). func TestCreateTillSale_PendingRetry_LostResponse_SingleSquareCharge(t *testing.T) { - t.Parallel() ctx, tx := testutils.SetupTestTx(t) adminID, err := fixtures.CreateTestAdminUser(tx) @@ -2378,7 +2530,8 @@ func TestCreateTillSale_PendingRetry_LostResponse_SingleSquareCharge(t *testing. // STORED idempotency key — the Square charge dedups on the stored key instead // of charging the customer twice. func TestCreateTillSale_PendingRetry_FreshKey_ReusesPendingRowViaGiftCard(t *testing.T) { - t.Parallel() + // Not t.Parallel(): asserts on rec.keys of the global SquareClient (see + // TestCreateTillSale_PendingRetry_LostResponse_SingleSquareCharge note). ctx, tx := testutils.SetupTestTx(t) adminID, err := fixtures.CreateTestAdminUser(tx) @@ -2472,7 +2625,8 @@ func TestCreateTillSale_PendingRetry_FreshKey_ReusesPendingRowViaGiftCard(t *tes // amount mismatch is the only signal distinguishing a retry from a fresh // top-up). func TestCreateTillSale_PendingRetry_FreshKey_DifferentAmount_NewCharge(t *testing.T) { - t.Parallel() + // Not t.Parallel(): asserts on rec.keys of the global SquareClient (see + // TestCreateTillSale_PendingRetry_LostResponse_SingleSquareCharge note). ctx, tx := testutils.SetupTestTx(t) adminID, err := fixtures.CreateTestAdminUser(tx) diff --git a/backend/handlers/user/gdpr_test.go b/backend/handlers/user/gdpr_test.go index 627422d..a444170 100644 --- a/backend/handlers/user/gdpr_test.go +++ b/backend/handlers/user/gdpr_test.go @@ -517,6 +517,120 @@ func TestAnonymizeUser_ClearsNotificationPrefs(t *testing.T) { } } +func TestAnonymizeUser_ScrubsNotes(t *testing.T) { + t.Parallel() + 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 = 'Client prefers quiet appointments and has a cat allergy' + WHERE id = $1 + `, userID) + if err != nil { + t.Fatalf("failed to set user notes: %v", err) + } + + _, err = tx.Exec(ctx, `SELECT anonymize_user($1)`, userID) + if err != nil { + 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) + if err != nil { + t.Fatalf("failed to query user notes: %v", err) + } + if notes != nil { + t.Errorf("expected users.notes to be NULL after anonymization, got %v", notes) + } + if lastLoginAt != nil { + t.Errorf("expected users.last_login_at to be NULL after anonymization, got %v", lastLoginAt) + } +} + +func TestAnonymizeUser_ScrubsNameHistory(t *testing.T) { + t.Parallel() + 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, ` + INSERT INTO name_history (user_id, previous_first_name, previous_last_name) + VALUES ($1, 'Old', 'Name') + `, userID) + if err != nil { + t.Fatalf("failed to insert name history: %v", err) + } + + _, err = tx.Exec(ctx, `SELECT anonymize_user($1)`, userID) + if err != nil { + t.Fatalf("anonymize_user failed: %v", err) + } + + var firstName, lastName string + err = tx.QueryRow(ctx, ` + SELECT previous_first_name, previous_last_name FROM name_history WHERE user_id = $1 + `, userID).Scan(&firstName, &lastName) + if err != nil { + t.Fatalf("failed to query name history: %v", err) + } + if firstName != "Deleted" { + t.Errorf("expected previous_first_name 'Deleted', got %q", firstName) + } + if lastName != "User" { + t.Errorf("expected previous_last_name 'User', got %q", lastName) + } +} + +func TestAnonymizeUser_ScrubsBookingNotes(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create test user: %v", err) + } + + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + + bookingID, err := fixtures.CreateTestBooking(tx, userID, serviceID) + if err != nil { + t.Fatalf("failed to create booking: %v", err) + } + + _, err = tx.Exec(ctx, ` + UPDATE bookings SET notes = 'Please call me on the day, doorbell broken' + WHERE id = $1 + `, bookingID) + if err != nil { + t.Fatalf("failed to set booking notes: %v", err) + } + + _, err = tx.Exec(ctx, `SELECT anonymize_user($1)`, userID) + if err != nil { + t.Fatalf("anonymize_user failed: %v", err) + } + + var notes interface{} + err = tx.QueryRow(ctx, `SELECT notes FROM bookings WHERE id = $1`, bookingID).Scan(¬es) + if err != nil { + t.Fatalf("failed to query booking notes: %v", err) + } + if notes != nil { + t.Errorf("expected booking notes to be NULL after anonymization, got %v", notes) + } +} + func TestAnonymizeUser_DoesNotAffectGuests(t *testing.T) { t.Parallel() ctx, tx := testutils.SetupTestTx(t) diff --git a/backend/handlers/webhooks/square.go b/backend/handlers/webhooks/square.go index 0b0d705..686b21f 100644 --- a/backend/handlers/webhooks/square.go +++ b/backend/handlers/webhooks/square.go @@ -1,10 +1,12 @@ package webhooks import ( + "context" "crypto/hmac" "crypto/sha256" "encoding/base64" "encoding/json" + "fmt" "io" "log" "net/http" @@ -191,12 +193,18 @@ func HandleSquareWebhook(w http.ResponseWriter, r *http.Request) { log.Printf("[SQUARE-WEBHOOK] Received event: %s", event.Type) switch event.Type { - case "payment.updated": + case "payment.updated", "payment.created", "payment.completed": handlePaymentUpdated(event.Data) - case "refund.updated": + case "refund.updated", "refund.created", "refund.completed", "refund.failed": handleRefundUpdated(event.Data) case "dispute.created": - log.Printf("[SQUARE-WEBHOOK] Dispute created: %s", event.EventID) + 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) + case "terminal.checkout.created", "terminal.checkout.updated": + handleTerminalCheckout(event.Data) default: log.Printf("[SQUARE-WEBHOOK] Unknown event type: %s", event.Type) } @@ -213,26 +221,442 @@ func verifySquareSignature(body []byte, signature, signingKey, notificationURL s return hmac.Equal([]byte(signature), []byte(expected)) } -// handlePaymentUpdated logs only the Square object id — never the raw payload, -// which contains PII (buyer email, card brand/last4, cardholder name, billing -// address, amounts). The envelope's event_id is logged at the dispatch site. -// On unmarshal failure log just the byte length (no content). +// squareWebhookData is the `data` envelope of a Square webhook v1 event. The +// affected object's id is at data.id; the full resource is nested at +// data.object. (e.g. data.object.payment). Only the id is logged — the +// nested object can contain PII and is never echoed to the log. +type squareWebhookData struct { + ID string `json:"id"` + Type string `json:"type"` + Object json.RawMessage `json:"object"` +} + +// squareDisputePayload maps the Square Dispute fields this app records. +// Reference: https://developer.squareup.com/reference/square/objects/Dispute +type squareDisputePayload struct { + ID string `json:"id"` + State string `json:"state"` + AmountMoney *squareMoneyPayload `json:"amount_money"` + Reason string `json:"reason"` + DisputedPayment *squareDisputedPaymentField `json:"disputed_payment"` +} + +type squareMoneyPayload struct { + Amount int64 `json:"amount"` // minor units (pence for GBP) + Currency string `json:"currency"` +} + +type squareDisputedPaymentField struct { + PaymentID string `json:"payment_id"` +} + +// squarePaymentPayload maps the Square Payment fields this app consumes. +type squarePaymentPayload struct { + ID string `json:"id"` + Status string `json:"status"` // "APPROVED", "COMPLETED", "CANCELED", "FAILED", "PENDING" +} + +// squareRefundPayload maps the Square Refund (PaymentRefund) fields this app +// consumes. +type squareRefundPayload struct { + ID string `json:"id"` + Status string `json:"status"` // "PENDING", "COMPLETED", "FAILED" +} + +// parseSquareObject unmarshals data.object. into out. Returns false when +// the nested resource is absent (legacy envelope carrying only data.id). +func parseSquareObject(object json.RawMessage, key string, out any) bool { + if len(object) == 0 { + return false + } + var wrapper map[string]json.RawMessage + if err := json.Unmarshal(object, &wrapper); err != nil { + return false + } + raw, ok := wrapper[key] + if !ok || len(raw) == 0 { + return false + } + if err := json.Unmarshal(raw, out); err != nil { + return false + } + return true +} + +// squareMoneyToAmount converts a Square Money object (minor units) to an exact +// two-decimal string for the NUMERIC(10,2) columns. String formatting avoids +// float64 rounding artifacts for money. +func squareMoneyToAmount(m *squareMoneyPayload) string { + if m == nil || m.Amount <= 0 { + return "0.00" + } + return fmt.Sprintf("%d.%02d", m.Amount/100, m.Amount%100) +} + +// squarePaymentStatusToLocal maps Square's payment state machine to the local +// payment_status enum. APPROVED/PENDING are NON-terminal (Square may still +// complete or void them), so they map to a zero local status and the caller +// leaves the row untouched — the same classification the stale-pending sweeps +// use (handlers/payments/sweep.go). +func squarePaymentStatusToLocal(status string) (string, bool) { + switch status { + case "COMPLETED": + return "completed", true + case "CANCELED", "FAILED": + return "failed", true + case "APPROVED", "PENDING": + return "", false + default: + return "", false + } +} + +// squareRefundStatusToLocal maps Square's refund status to the local +// payment_status enum. PENDING is non-terminal. +func squareRefundStatusToLocal(status string) (string, bool) { + switch status { + case "COMPLETED": + return "completed", true + case "FAILED": + return "failed", true + default: + return "", false + } +} + +// squareDisputeStateToLocal maps Square's dispute state to the local +// disputes.status. Only the terminal resolutions move the row to won/lost; +// ACCEPTED (seller accepted the dispute) is a loss — the money is gone. +// Everything else (inquiries, evidence required, processing) stays open. +func squareDisputeStateToLocal(state string) string { + switch state { + case "WON": + return "won" + case "LOST", "ACCEPTED": + return "lost" + default: + return "open" + } +} + +// findPaymentBySquareID resolves the local payment id and booking id for a +// Square payment id. Multiple local rows can share one Square charge id (e.g. +// a deposit + balance split); the most recent is used. +func findPaymentBySquareID(squarePaymentID string) (paymentID, bookingID string, ok bool) { + if squarePaymentID == "" { + return "", "", false + } + var pid string + var bid *string + err := db.Conn.QueryRow(context.Background(), ` + SELECT id, booking_id FROM payments + WHERE square_payment_id = $1 + ORDER BY created_at DESC, id DESC + LIMIT 1 + `, squarePaymentID).Scan(&pid, &bid) + if err != nil { + return "", "", false + } + if bid != nil { + bookingID = *bid + } + return pid, bookingID, true +} + +// findPaymentByDisputeID resolves the local payment (and its booking) recorded +// for a dispute row. Used by dispute.state.updated when the dispute row already +// exists but the webhook payload carries no resolvable Square payment id. +func findPaymentByDisputeID(squareDisputeID string) (paymentID, bookingID string) { + var pid string + var bid *string + err := db.Conn.QueryRow(context.Background(), ` + SELECT d.payment_id, p.booking_id + FROM disputes d + JOIN payments p ON p.id = d.payment_id + WHERE d.square_dispute_id = $1 + `, squareDisputeID).Scan(&pid, &bid) + if err != nil { + return "", "" + } + if bid != nil { + bookingID = *bid + } + return pid, bookingID +} + +// insertCriticalPaymentNotification surfaces a money event in the admin +// notification centre (reason='critical_payment_log'), the DB-backed stand-in +// for un-watched CRITICAL log lines (see ScanCriticalPaymentLogs in +// internal/jobs/cleanup.go). Dedup: one unacknowledged row per (reason, +// booking_id) — acknowledging re-arms it. +func insertCriticalPaymentNotification(bookingID string) { + var bid any + if bookingID != "" { + bid = bookingID + } + tag, err := db.Conn.Exec(context.Background(), ` + INSERT INTO admin_notifications (reason, booking_id, created_at) + SELECT 'critical_payment_log'::admin_notification_reason, $1, 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.acknowledged_at IS NULL + ) + `, bid) + if err != nil { + log.Printf("[SQUARE-WEBHOOK] Failed to insert critical_payment_log admin notification: %v", err) + return + } + if tag.RowsAffected() > 0 { + log.Printf("[SQUARE-WEBHOOK] Inserted critical_payment_log admin notification (booking_id=%s)", bookingID) + } +} + +// 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) { + if paymentID == "" { + return + } + _, 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) + } +} + +// 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) { - var obj struct{ ID string `json:"id"` } - if err := json.Unmarshal(data, &obj); err != nil { + var env squareWebhookData + if err := json.Unmarshal(data, &env); err != nil { log.Printf("[SQUARE-WEBHOOK] payment.updated received (payload length=%d)", len(data)) return } - log.Printf("[SQUARE-WEBHOOK] payment.updated received (data.id=%s)", obj.ID) + if env.ID == "" { + log.Printf("[SQUARE-WEBHOOK] payment.updated received (payload length=%d)", len(data)) + return + } + 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 + } + 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 + } + // Only 'pending' rows are candidates for a terminal transition — the same + // conservative rule the stale-pending sweeps use. A webhook for an already + // settled row (Square fires payment.updated for ANY field change, e.g. fee + // recalculation on a fully-refunded charge) must never revert a terminal + // status like 'refunded' back to 'completed'. + tag, err := db.Conn.Exec(context.Background(), + `UPDATE payments 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 update payment %s to status %s: %v", payment.ID, localStatus, err) + return + } + 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. + 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 + } + 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) + } } -// handleRefundUpdated logs only the Square object id — never the raw payload, -// which contains PII. See handlePaymentUpdated. +// handleRefundUpdated reconciles a Square Refund state change against the local +// refunds row. Idempotent (status-guarded UPDATE + event_id dedup). func handleRefundUpdated(data json.RawMessage) { - var obj struct{ ID string `json:"id"` } - if err := json.Unmarshal(data, &obj); err != nil { + var env squareWebhookData + if err := json.Unmarshal(data, &env); err != nil { log.Printf("[SQUARE-WEBHOOK] refund.updated received (payload length=%d)", len(data)) return } - log.Printf("[SQUARE-WEBHOOK] refund.updated received (data.id=%s)", obj.ID) + if env.ID == "" { + log.Printf("[SQUARE-WEBHOOK] refund.updated received (payload length=%d)", len(data)) + return + } + 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 + } + 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 + } + // COMPLETED may promote any non-completed row (incl. a sweep-failed refund + // Square later shows complete) — the over-refund guard counts completed + // refunds, so this only tightens it. FAILED only demotes a 'pending' row: + // demoting 'completed' would let the guard exclude money that already moved + // (the exact risk refunds.go documents for failed refunds). + var upd string + switch localStatus { + case "completed": + upd = `UPDATE refunds SET status = 'completed' WHERE square_refund_id = $1 AND status <> 'completed'` + case "failed": + upd = `UPDATE refunds SET status = 'failed' WHERE square_refund_id = $1 AND status = 'pending'` + } + 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 + } + if tag.RowsAffected() > 0 { + log.Printf("[SQUARE-WEBHOOK] refund.updated: square refund %s → local status %s", refund.ID, localStatus) + } +} + +// 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). +func truncateDisputeReason(reason string) string { + if len(reason) > 192 { + return reason[:192] + } + return reason +} + +// 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) { + var env squareWebhookData + if err := json.Unmarshal(data, &env); err != nil { + log.Printf("[SQUARE-WEBHOOK] dispute.created received (payload length=%d)", len(data)) + return + } + var dispute squareDisputePayload + if !parseSquareObject(env.Object, "dispute", &dispute) || dispute.ID == "" { + log.Printf("[SQUARE-WEBHOOK] dispute.created received (data.id=%s)", env.ID) + return + } + squarePaymentID := "" + if dispute.DisputedPayment != nil { + squarePaymentID = dispute.DisputedPayment.PaymentID + } + 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 + } + amount := squareMoneyToAmount(dispute.AmountMoney) + tag, err := db.Conn.Exec(context.Background(), ` + INSERT INTO disputes (square_dispute_id, payment_id, status, amount, reason, created_at, updated_at) + VALUES ($1, $2, 'open', $3, NULLIF($4, ''), NOW(), NOW()) + ON CONFLICT (square_dispute_id) DO NOTHING + `, dispute.ID, paymentID, amount, truncateDisputeReason(dispute.Reason)) + if err != nil { + log.Printf("[SQUARE-WEBHOOK] Failed to insert dispute %s: %v", dispute.ID, err) + return + } + _ = 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) +} + +// 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) { + 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 + } + 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 + } + localStatus := squareDisputeStateToLocal(dispute.State) + amount := squareMoneyToAmount(dispute.AmountMoney) + + squarePaymentID := "" + if dispute.DisputedPayment != nil { + squarePaymentID = dispute.DisputedPayment.PaymentID + } + paymentID, bookingID, paymentFound := findPaymentBySquareID(squarePaymentID) + if !paymentFound { + // Row may already exist from dispute.created — recover its payment. + 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 + } + } + + _, err := db.Conn.Exec(context.Background(), ` + INSERT INTO disputes (square_dispute_id, payment_id, status, amount, reason, created_at, updated_at) + VALUES ($1, $2, $3, $4, NULLIF($5, ''), NOW(), NOW()) + ON CONFLICT (square_dispute_id) DO UPDATE + SET status = EXCLUDED.status, amount = EXCLUDED.amount, + reason = EXCLUDED.reason, updated_at = NOW() + `, 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 + } + + switch localStatus { + case "lost": + markPaymentFailed(paymentID) + insertCriticalPaymentNotification(bookingID) + log.Printf("[SQUARE-WEBHOOK] CRITICAL: dispute %s LOST — payment %s marked failed; admin notified", dispute.ID, paymentID) + case "won": + log.Printf("[SQUARE-WEBHOOK] dispute %s WON — resolved in seller's favour; no action", dispute.ID) + default: + log.Printf("[SQUARE-WEBHOOK] dispute %s state → %s (status %s)", dispute.ID, dispute.State, localStatus) + } +} + +// handleDisputeEvidence logs evidence submissions/removals. Evidence does not +// change the dispute's local status, so it is informational only. +func handleDisputeEvidence(data json.RawMessage) { + 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 + } + 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 + } + log.Printf("[SQUARE-WEBHOOK] dispute evidence event for dispute %s (state %s)", dispute.ID, dispute.State) +} + +// 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) { + 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 + } + log.Printf("[SQUARE-WEBHOOK] terminal.checkout event received (data.id=%s)", env.ID) } diff --git a/backend/handlers/webhooks/webhooks_state_test.go b/backend/handlers/webhooks/webhooks_state_test.go new file mode 100644 index 0000000..5430eb9 --- /dev/null +++ b/backend/handlers/webhooks/webhooks_state_test.go @@ -0,0 +1,659 @@ +//go:build test + +package webhooks + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "crussell/db" +) + +// ============================================================================= +// Helpers — DB-backed state assertions +// ============================================================================= + +// createWebhookTestPayment inserts a payment row with the given Square charge +// id and returns the local payment id. The test DB is fresh per package run, +// so no cleanup is needed. +func createWebhookTestPayment(t *testing.T, squarePaymentID, status string) string { + t.Helper() + var id string + err := db.Conn.QueryRow(context.Background(), ` + INSERT INTO payments (payment_type, payment_method, status, amount, square_payment_id, created_at, updated_at) + VALUES ('full', 'online_square', $2, 10.00, $1, NOW(), NOW()) + RETURNING id + `, squarePaymentID, status).Scan(&id) + if err != nil { + t.Fatalf("failed to create webhook test payment: %v", err) + } + return id +} + +func createWebhookTestRefund(t *testing.T, paymentID, squareRefundID, status string) string { + t.Helper() + var id string + err := db.Conn.QueryRow(context.Background(), ` + INSERT INTO refunds (payment_id, amount, reason, status, square_refund_id, created_at) + VALUES ($1, 5.00, 'webhook test refund', $3, $2, NOW()) + RETURNING id + `, paymentID, squareRefundID, status).Scan(&id) + if err != nil { + t.Fatalf("failed to create webhook test refund: %v", err) + } + return id +} + +func getPaymentStatus(t *testing.T, id string) string { + t.Helper() + var status string + if err := db.Conn.QueryRow(context.Background(), + "SELECT status FROM payments WHERE id = $1", id).Scan(&status); err != nil { + t.Fatalf("failed to read payment status: %v", err) + } + return status +} + +func getRefundStatus(t *testing.T, id string) string { + t.Helper() + var status string + if err := db.Conn.QueryRow(context.Background(), + "SELECT status FROM refunds WHERE id = $1", id).Scan(&status); err != nil { + t.Fatalf("failed to read refund status: %v", err) + } + return status +} + +func getDisputeStatus(t *testing.T, squareDisputeID string) string { + t.Helper() + var status string + if err := db.Conn.QueryRow(context.Background(), + "SELECT status FROM disputes WHERE square_dispute_id = $1", squareDisputeID).Scan(&status); err != nil { + t.Fatalf("failed to read dispute status: %v", err) + } + return status +} + +func countCriticalNotifications(t *testing.T) int { + t.Helper() + var n int + if err := db.Conn.QueryRow(context.Background(), + "SELECT COUNT(*) FROM admin_notifications WHERE reason = 'critical_payment_log'").Scan(&n); err != nil { + t.Fatalf("failed to count critical_payment_log notifications: %v", err) + } + return n +} + +// deliverWebhook signs and dispatches a Square event through the full handler. +func deliverWebhook(t *testing.T, event SquareWebhookEvent) *httptest.ResponseRecorder { + t.Helper() + body, err := json.Marshal(event) + if err != nil { + t.Fatalf("failed to marshal webhook event: %v", err) + } + sig := webhookTestEnv(t, body) + return makeWebhookRequest(body, sig, context.Background()) +} + +// ============================================================================= +// Dispute handling — dispute.created +// ============================================================================= + +func TestWebhook_DisputeCreated_InsertsDisputeRow(t *testing.T) { + const squarePaymentID = "sqp_dispute_created" + payID := createWebhookTestPayment(t, squarePaymentID, "completed") + + event := SquareWebhookEvent{ + Type: "dispute.created", + EventID: "evt_dispute_created_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "dispute", + "id": "dts_dispute_created_1", + "object": { + "dispute": { + "id": "dts_dispute_created_1", + "state": "UNDER_REVIEW", + "amount_money": {"amount": 1234, "currency": "GBP"}, + "reason": "NO_KNOWLEDGE", + "disputed_payment": {"payment_id": "` + squarePaymentID + `"} + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + + var ( + status string + amount float64 + reason string + pid string + ) + err := db.Conn.QueryRow(context.Background(), ` + SELECT status, amount, reason, payment_id FROM disputes WHERE square_dispute_id = 'dts_dispute_created_1' + `).Scan(&status, &amount, &reason, &pid) + if err != nil { + t.Fatalf("expected a disputes row to be inserted, got: %v", err) + } + if status != "open" { + t.Errorf("expected dispute status 'open', got %q", status) + } + if amount != 12.34 { + t.Errorf("expected dispute amount 12.34, got %v", amount) + } + if reason != "NO_KNOWLEDGE" { + t.Errorf("expected dispute reason 'NO_KNOWLEDGE', got %q", reason) + } + if pid != payID { + t.Errorf("expected dispute payment_id %s, got %s", payID, pid) + } + + // A dispute is a CRITICAL money event — the admin notification centre must + // surface it. + if got := countCriticalNotifications(t); got < 1 { + t.Errorf("expected at least 1 critical_payment_log admin notification, got %d", got) + } +} + +func TestWebhook_DisputeCreated_NoLocalPayment_NoRow(t *testing.T) { + event := SquareWebhookEvent{ + Type: "dispute.created", + EventID: "evt_dispute_orphan_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "dispute", + "id": "dts_orphan_1", + "object": { + "dispute": { + "id": "dts_orphan_1", + "state": "UNDER_REVIEW", + "amount_money": {"amount": 1000, "currency": "GBP"}, + "disputed_payment": {"payment_id": "sqp_never_seen"} + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + var n int + if err := db.Conn.QueryRow(context.Background(), + "SELECT COUNT(*) FROM disputes WHERE square_dispute_id = 'dts_orphan_1'").Scan(&n); err != nil { + t.Fatalf("failed to count disputes: %v", err) + } + if n != 0 { + t.Errorf("expected no disputes row for an unknown square payment, got %d", n) + } +} + +func TestWebhook_DisputeCreated_LongReason_Truncated(t *testing.T) { + const squarePaymentID = "sqp_dispute_longreason" + _ = createWebhookTestPayment(t, squarePaymentID, "completed") + + longReason := strings.Repeat("z", 300) + event := SquareWebhookEvent{ + Type: "dispute.created", + EventID: "evt_dispute_longreason_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "dispute", + "id": "dts_longreason_1", + "object": { + "dispute": { + "id": "dts_longreason_1", + "state": "UNDER_REVIEW", + "amount_money": {"amount": 1234, "currency": "GBP"}, + "reason": "` + longReason + `", + "disputed_payment": {"payment_id": "` + squarePaymentID + `"} + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + + // disputes.reason is VARCHAR(192): the over-long reason must be truncated + // so the INSERT succeeds instead of failing (and, after the dedup row + // commits, silently dropping the dispute). + var storedReason string + if err := db.Conn.QueryRow(context.Background(), + "SELECT reason FROM disputes WHERE square_dispute_id = 'dts_longreason_1'").Scan(&storedReason); err != nil { + t.Fatalf("expected a disputes row to be inserted, got: %v", err) + } + if len(storedReason) > 192 { + t.Errorf("expected reason truncated to <=192 chars, got %d", len(storedReason)) + } + if storedReason != strings.Repeat("z", 192) { + t.Errorf("expected reason truncated to exactly 192 'z' chars, got %q", storedReason) + } +} + +// ============================================================================= +// Dispute handling — dispute.state.updated +// ============================================================================= + +func TestWebhook_DisputeStateUpdated_Lost_MarksPaymentFailed(t *testing.T) { + const squarePaymentID = "sqp_dispute_lost" + payID := createWebhookTestPayment(t, squarePaymentID, "completed") + // Seed the dispute row as dispute.created would have. + if _, err := db.Conn.Exec(context.Background(), ` + INSERT INTO disputes (square_dispute_id, payment_id, status, amount, reason) + VALUES ('dts_lost_1', $1, 'open', 12.34, 'NO_KNOWLEDGE') + `, payID); err != nil { + t.Fatalf("failed to seed dispute row: %v", err) + } + + event := SquareWebhookEvent{ + Type: "dispute.state.updated", + EventID: "evt_dispute_lost_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "dispute", + "id": "dts_lost_1", + "object": { + "dispute": { + "id": "dts_lost_1", + "state": "LOST", + "amount_money": {"amount": 1234, "currency": "GBP"}, + "reason": "NO_KNOWLEDGE", + "disputed_payment": {"payment_id": "` + squarePaymentID + `"} + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getDisputeStatus(t, "dts_lost_1"); got != "lost" { + t.Errorf("expected dispute status 'lost', got %q", got) + } + if got := getPaymentStatus(t, payID); got != "failed" { + t.Errorf("expected payment status 'failed' after lost dispute, got %q", got) + } + if got := countCriticalNotifications(t); got < 1 { + t.Errorf("expected a critical_payment_log notification for the lost dispute, got %d", got) + } +} + +func TestWebhook_DisputeStateUpdated_Won_KeepsPaymentCompleted(t *testing.T) { + const squarePaymentID = "sqp_dispute_won" + payID := createWebhookTestPayment(t, squarePaymentID, "completed") + + // No seeded dispute row: state.updated arriving before dispute.created must + // upsert the row. + event := SquareWebhookEvent{ + Type: "dispute.state.updated", + EventID: "evt_dispute_won_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "dispute", + "id": "dts_won_1", + "object": { + "dispute": { + "id": "dts_won_1", + "state": "WON", + "amount_money": {"amount": 1234, "currency": "GBP"}, + "disputed_payment": {"payment_id": "` + squarePaymentID + `"} + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getDisputeStatus(t, "dts_won_1"); got != "won" { + t.Errorf("expected dispute status 'won', got %q", got) + } + if got := getPaymentStatus(t, payID); got != "completed" { + t.Errorf("expected payment to stay 'completed' after won dispute, got %q", got) + } +} + +func TestWebhook_DisputeStateUpdated_Open_KeepsOpen(t *testing.T) { + const squarePaymentID = "sqp_dispute_open" + payID := createWebhookTestPayment(t, squarePaymentID, "completed") + if _, err := db.Conn.Exec(context.Background(), ` + INSERT INTO disputes (square_dispute_id, payment_id, status, amount, reason) + VALUES ('dts_open_1', $1, 'open', 12.34, 'NO_KNOWLEDGE') + `, payID); err != nil { + t.Fatalf("failed to seed dispute row: %v", err) + } + + event := SquareWebhookEvent{ + Type: "dispute.state.updated", + EventID: "evt_dispute_open_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "dispute", + "id": "dts_open_1", + "object": { + "dispute": { + "id": "dts_open_1", + "state": "EVIDENCE_REQUIRED", + "amount_money": {"amount": 1234, "currency": "GBP"}, + "disputed_payment": {"payment_id": "` + squarePaymentID + `"} + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getDisputeStatus(t, "dts_open_1"); got != "open" { + t.Errorf("expected dispute to stay 'open' on EVIDENCE_REQUIRED, got %q", got) + } + if got := getPaymentStatus(t, payID); got != "completed" { + t.Errorf("expected payment to stay 'completed', got %q", got) + } +} + +// ============================================================================= +// State mutation — payment.updated +// ============================================================================= + +func TestWebhook_PaymentUpdated_UpdatesPaymentStatus(t *testing.T) { + const squarePaymentID = "sqp_updated_completed" + payID := createWebhookTestPayment(t, squarePaymentID, "pending") + + event := SquareWebhookEvent{ + Type: "payment.updated", + EventID: "evt_payment_updated_completed_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "payment", + "id": "` + squarePaymentID + `", + "object": { + "payment": { + "id": "` + squarePaymentID + `", + "status": "COMPLETED" + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getPaymentStatus(t, payID); got != "completed" { + t.Errorf("expected payment status 'completed', got %q", got) + } +} + +func TestWebhook_PaymentUpdated_FailedStatus(t *testing.T) { + const squarePaymentID = "sqp_updated_failed" + payID := createWebhookTestPayment(t, squarePaymentID, "pending") + + event := SquareWebhookEvent{ + Type: "payment.updated", + EventID: "evt_payment_updated_failed_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "payment", + "id": "` + squarePaymentID + `", + "object": { + "payment": { + "id": "` + squarePaymentID + `", + "status": "FAILED" + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getPaymentStatus(t, payID); got != "failed" { + t.Errorf("expected payment status 'failed', got %q", got) + } +} + +func TestWebhook_PaymentUpdated_NonTerminal_LeavesPending(t *testing.T) { + const squarePaymentID = "sqp_updated_approved" + payID := createWebhookTestPayment(t, squarePaymentID, "pending") + + event := SquareWebhookEvent{ + Type: "payment.updated", + EventID: "evt_payment_updated_approved_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "payment", + "id": "` + squarePaymentID + `", + "object": { + "payment": { + "id": "` + squarePaymentID + `", + "status": "APPROVED" + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getPaymentStatus(t, payID); got != "pending" { + t.Errorf("expected payment to stay 'pending' on non-terminal APPROVED, got %q", got) + } +} + +// TestWebhook_PaymentUpdated_DoesNotRevertRefunded guards the pending-only +// transition: Square fires payment.updated for ANY field change (e.g. a fee +// recalculation on a fully refunded charge), and that must not flip the local +// row back from 'refunded' to 'completed' — which would reopen the +// over-refund guard. +func TestWebhook_PaymentUpdated_DoesNotRevertRefunded(t *testing.T) { + const squarePaymentID = "sqp_updated_refunded" + payID := createWebhookTestPayment(t, squarePaymentID, "refunded") + + event := SquareWebhookEvent{ + Type: "payment.updated", + EventID: "evt_payment_updated_refunded_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "payment", + "id": "` + squarePaymentID + `", + "object": { + "payment": { + "id": "` + squarePaymentID + `", + "status": "COMPLETED" + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getPaymentStatus(t, payID); got != "refunded" { + t.Errorf("expected refunded payment to stay 'refunded', got %q", got) + } +} + +func TestWebhook_PaymentUpdated_IdempotentReplay(t *testing.T) { + const squarePaymentID = "sqp_updated_idem" + payID := createWebhookTestPayment(t, squarePaymentID, "pending") + + event := SquareWebhookEvent{ + Type: "payment.updated", + EventID: "evt_payment_updated_idem_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "payment", + "id": "` + squarePaymentID + `", + "object": { + "payment": { + "id": "` + squarePaymentID + `", + "status": "COMPLETED" + } + } + }`), + } + // Two deliveries of the SAME event_id: the second is dropped by dedup, the + // state mutation applies exactly once. + w1 := deliverWebhook(t, event) + if w1.Code != http.StatusOK { + t.Fatalf("expected first delivery 200, got %d: %s", w1.Code, w1.Body.String()) + } + w2 := deliverWebhook(t, event) + if w2.Code != http.StatusOK { + t.Fatalf("expected replay 200, got %d: %s", w2.Code, w2.Body.String()) + } + if got := getPaymentStatus(t, payID); got != "completed" { + t.Errorf("expected payment status 'completed' after idempotent replay, got %q", got) + } + if n := countWebhookEvents(t, event.EventID); n != 1 { + t.Errorf("expected exactly 1 dedup row after replay, got %d", n) + } +} + +// ============================================================================= +// State mutation — refund.updated +// ============================================================================= + +func TestWebhook_RefundUpdated_UpdatesRefundStatus(t *testing.T) { + const ( + squarePaymentID = "sqp_refund_pay" + squareRefundID = "sqr_updated_completed" + ) + payID := createWebhookTestPayment(t, squarePaymentID, "completed") + refundID := createWebhookTestRefund(t, payID, squareRefundID, "pending") + + event := SquareWebhookEvent{ + Type: "refund.updated", + EventID: "evt_refund_updated_completed_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "refund", + "id": "` + squareRefundID + `", + "object": { + "refund": { + "id": "` + squareRefundID + `", + "status": "COMPLETED", + "payment_id": "` + squarePaymentID + `" + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getRefundStatus(t, refundID); got != "completed" { + t.Errorf("expected refund status 'completed', got %q", got) + } +} + +func TestWebhook_RefundUpdated_FailedStatus(t *testing.T) { + const ( + squarePaymentID = "sqp_refund_pay_fail" + squareRefundID = "sqr_updated_failed" + ) + payID := createWebhookTestPayment(t, squarePaymentID, "completed") + refundID := createWebhookTestRefund(t, payID, squareRefundID, "pending") + + event := SquareWebhookEvent{ + Type: "refund.updated", + EventID: "evt_refund_updated_failed_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "refund", + "id": "` + squareRefundID + `", + "object": { + "refund": { + "id": "` + squareRefundID + `", + "status": "FAILED", + "payment_id": "` + squarePaymentID + `" + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getRefundStatus(t, refundID); got != "failed" { + t.Errorf("expected refund status 'failed', got %q", got) + } +} + +func TestWebhook_RefundUpdated_NonTerminal_LeavesPending(t *testing.T) { + const ( + squarePaymentID = "sqp_refund_pay_pending" + squareRefundID = "sqr_updated_pending" + ) + payID := createWebhookTestPayment(t, squarePaymentID, "completed") + refundID := createWebhookTestRefund(t, payID, squareRefundID, "pending") + + event := SquareWebhookEvent{ + Type: "refund.updated", + EventID: "evt_refund_updated_pending_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "refund", + "id": "` + squareRefundID + `", + "object": { + "refund": { + "id": "` + squareRefundID + `", + "status": "PENDING", + "payment_id": "` + squarePaymentID + `" + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getRefundStatus(t, refundID); got != "pending" { + t.Errorf("expected refund to stay 'pending' on non-terminal PENDING, got %q", got) + } +} + +// TestWebhook_RefundUpdated_DoesNotDemoteCompleted guards the FAILED +// transition: a completed refund must never be demoted to 'failed' by a late +// webhook, since the over-refund guard counts 'completed' refunds — demoting +// would let the guard exclude money that already moved. +func TestWebhook_RefundUpdated_DoesNotDemoteCompleted(t *testing.T) { + const ( + squarePaymentID = "sqp_refund_pay_demote" + squareRefundID = "sqr_demote" + ) + payID := createWebhookTestPayment(t, squarePaymentID, "completed") + refundID := createWebhookTestRefund(t, payID, squareRefundID, "completed") + + event := SquareWebhookEvent{ + Type: "refund.updated", + EventID: "evt_refund_demote_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "refund", + "id": "` + squareRefundID + `", + "object": { + "refund": { + "id": "` + squareRefundID + `", + "status": "FAILED", + "payment_id": "` + squarePaymentID + `" + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getRefundStatus(t, refundID); got != "completed" { + t.Errorf("expected completed refund to stay 'completed', got %q", got) + } +} diff --git a/backend/internal/square/square_http_client_timeout_test.go b/backend/internal/square/square_http_client_timeout_test.go new file mode 100644 index 0000000..b59be68 --- /dev/null +++ b/backend/internal/square/square_http_client_timeout_test.go @@ -0,0 +1,75 @@ +//go:build test + +package square + +import ( + "context" + "errors" + "net" + "net/http" + "net/http/httptest" + "testing" + "time" +) + +// TestHTTPClientTimeout_ClassifiedAmbiguous503 locks the N6 contract: the +// production HTTP client's 30s timeout must fire against a stalled upstream and +// produce a context-deadline error. chargeFailureStatus +// (handlers/payments/errors.go) checks `errors.Is(err, context.DeadlineExceeded)` +// FIRST and maps it to 503 (Service Unavailable / ambiguous) — the charge may +// or may not have reached Square, so it must never be labelled the definitive +// 402 decline a retry would ignore. +func TestHTTPClientTimeout_ClassifiedAmbiguous503(t *testing.T) { + // Upstream Square stalls LONGER than the production client timeout, so the + // client must give up on its own — the handler never hangs. `stop` aborts + // the handler at teardown so srv.Close() does not wait out the full delay. + delay := defaultHTTPTimeout + 5*time.Second + stop := make(chan struct{}) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + select { + case <-time.After(delay): + w.WriteHeader(http.StatusOK) + case <-r.Context().Done(): + return + case <-stop: + return + } + })) + defer srv.Close() + + hc := &httpClient{baseURL: srv.URL, token: "test-token", http: &http.Client{Timeout: defaultHTTPTimeout}} + + start := time.Now() + _, err := createPaymentHTTPWithClient(context.Background(), CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "cnon:test-card", + IdempotencyKey: "timeout-slow-upstream", + }, hc) + elapsed := time.Since(start) + close(stop) + + if err == nil { + t.Fatal("expected a timeout error, got nil") + } + + // The client must have given up near its 30s timeout — not instantly and + // not after the 35s server delay. + if elapsed < 25*time.Second || elapsed > 33*time.Second { + t.Errorf("expected timeout after ~%v, got %v (err: %v)", defaultHTTPTimeout, elapsed, err) + } + + // The context-deadline predicate chargeFailureStatus maps to 503. Go's + // http.Client.Timeout wraps *timeoutError whose Is() matches + // context.DeadlineExceeded. + if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("expected errors.Is(err, context.DeadlineExceeded), got %v", err) + } + + // Standard Go contract: a client timeout surfaces as a net.Error with + // Timeout() == true. + var netErr net.Error + if !errors.As(err, &netErr) || !netErr.Timeout() { + t.Errorf("expected a timeout net.Error, got %T: %v", err, err) + } +} diff --git a/backend/main.go b/backend/main.go index c240341..fa88229 100644 --- a/backend/main.go +++ b/backend/main.go @@ -167,6 +167,64 @@ func healthCheckHandler(w http.ResponseWriter, r *http.Request) { } } +// corsAllowedOrigins returns the frontend origins permitted to call the API, +// read from the comma-separated FRONTEND_ORIGIN env var. Entries are trimmed +// and blanks dropped; an unset/empty var falls back to the local dev origin. +func corsAllowedOrigins() []string { + var allowed []string + for _, o := range strings.Split(os.Getenv("FRONTEND_ORIGIN"), ",") { + if o = strings.TrimSpace(o); o != "" { + allowed = append(allowed, o) + } + } + if len(allowed) == 0 { + allowed = []string{"http://localhost:5173"} + } + return allowed +} + +// originAllowed reports whether origin is exactly in the allowlist. +func originAllowed(origin string, allowed []string) bool { + for _, o := range allowed { + if origin == o { + return true + } + } + return false +} + +// corsMiddleware sets security headers plus a CORS allowlist so credentialed +// cross-origin requests (Authorization: Bearer) work only from configured +// frontend origins. +func corsMiddleware(next http.Handler) http.Handler { + allowedOrigins := corsAllowedOrigins() + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("X-Content-Type-Options", "nosniff") + w.Header().Set("X-Frame-Options", "DENY") + w.Header().Set("X-XSS-Protection", "1; mode=block") + // TODO: Enable HSTS in production + w.Header().Set("Strict-Transport-Security", "max-age=31536000; includeSubDomains") + // TODO: Enable Referrer-Policy in production + w.Header().Set("Referrer-Policy", "strict-origin-when-cross-origin") + w.Header().Set("Content-Security-Policy", "default-src 'none'; frame-ancestors 'none'") + + origin := r.Header.Get("Origin") + if origin != "" && originAllowed(origin, allowedOrigins) { + w.Header().Set("Access-Control-Allow-Origin", origin) + w.Header().Set("Vary", "Origin") + } + w.Header().Set("Access-Control-Allow-Methods", "GET, POST, PUT, PATCH, DELETE, OPTIONS") + w.Header().Set("Access-Control-Allow-Headers", "Authorization, Content-Type, Idempotency-Key") + + if r.Method == http.MethodOptions { + w.WriteHeader(http.StatusNoContent) + return + } + + next.ServeHTTP(w, r) + }) +} + func main() { initDB() initDav() @@ -231,35 +289,11 @@ func main() { r.Use(middleware.Recoverer) r.Use(middleware.Timeout(15 * time.Second)) - r.Use(func(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("X-Content-Type-Options", "nosniff") - w.Header().Set("X-Frame-Options", "DENY") - w.Header().Set("X-XSS-Protection", "1; mode=block") - // TODO: Enable HSTS in production - w.Header().Set("Strict-Transport-Security", "max-age=31536000; includeSubDomains") - // TODO: Enable Referrer-Policy in production - w.Header().Set("Referrer-Policy", "strict-origin-when-cross-origin") - w.Header().Set("Content-Security-Policy", "default-src 'none'; frame-ancestors 'none'") - - // Reflect origin (not wildcard '*') so credentialed cross-origin - // requests with Authorization: Bearer work in browsers. - origin := r.Header.Get("Origin") - if origin != "" { - w.Header().Set("Access-Control-Allow-Origin", origin) - w.Header().Set("Vary", "Origin") - } - w.Header().Set("Access-Control-Allow-Methods", "GET, POST, PUT, PATCH, DELETE, OPTIONS") - w.Header().Set("Access-Control-Allow-Headers", "Authorization, Content-Type, Idempotency-Key") - - if r.Method == http.MethodOptions { - w.WriteHeader(http.StatusNoContent) - return - } - - next.ServeHTTP(w, r) - }) - }) + // CORS + security headers: credentialed cross-origin requests + // (Authorization: Bearer) are only answered for origins in the configured + // FRONTEND_ORIGIN allowlist — never reflected blindly, so a leaked JWT + // cannot be used from a rogue site. + r.Use(corsMiddleware) // All API routes grouped under /api for clarity r.Route("/api", func(r chi.Router) { diff --git a/backend/main_test.go b/backend/main_test.go index c9f0834..9b45600 100644 --- a/backend/main_test.go +++ b/backend/main_test.go @@ -101,3 +101,85 @@ func TestHealthCheck_Degraded(t *testing.T) { // Restore original db.Conn db.Conn = originalDB } + +func TestCORS_OnlyAllowedOrigins(t *testing.T) { + t.Setenv("FRONTEND_ORIGIN", "https://app.example.com, http://localhost:5173") + handler := corsMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + + tests := []struct { + name string + origin string + expectAllowed bool + }{ + {name: "first configured origin is allowed", origin: "https://app.example.com", expectAllowed: true}, + {name: "second configured origin is allowed", origin: "http://localhost:5173", expectAllowed: true}, + {name: "unlisted origin is rejected", origin: "https://evil.example.com", expectAllowed: false}, + {name: "prefix-confusion origin is rejected", origin: "https://app.example.com.evil.test", expectAllowed: false}, + {name: "suffix-attack origin is rejected", origin: "https://app.example.com/evil", expectAllowed: false}, + {name: "no origin header is not echoed", origin: "", expectAllowed: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/api/health", nil) + if tt.origin != "" { + req.Header.Set("Origin", tt.origin) + } + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + got := rr.Header().Get("Access-Control-Allow-Origin") + if tt.expectAllowed && got != tt.origin { + t.Errorf("expected Access-Control-Allow-Origin %q, got %q", tt.origin, got) + } + if !tt.expectAllowed && got != "" { + t.Errorf("expected no Access-Control-Allow-Origin header, got %q", got) + } + }) + } +} + +func TestCORS_DefaultOriginWhenEnvUnset(t *testing.T) { + t.Setenv("FRONTEND_ORIGIN", "") + handler := corsMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + + allowed := httptest.NewRequest(http.MethodGet, "/", nil) + allowed.Header.Set("Origin", "http://localhost:5173") + aw := httptest.NewRecorder() + handler.ServeHTTP(aw, allowed) + if got := aw.Header().Get("Access-Control-Allow-Origin"); got != "http://localhost:5173" { + t.Errorf("expected default dev origin to be allowed, got %q", got) + } + + evil := httptest.NewRequest(http.MethodGet, "/", nil) + evil.Header.Set("Origin", "https://evil.example.com") + ew := httptest.NewRecorder() + handler.ServeHTTP(ew, evil) + if got := ew.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("expected unlisted origin to be rejected with default config, got %q", got) + } +} + +func TestCORSPreflight_RejectsUnlistedOrigin(t *testing.T) { + t.Setenv("FRONTEND_ORIGIN", "https://app.example.com") + handler := corsMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + + req := httptest.NewRequest(http.MethodOptions, "/api/bookings", nil) + req.Header.Set("Origin", "https://evil.example.com") + req.Header.Set("Access-Control-Request-Method", "POST") + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + if rr.Code != http.StatusNoContent { + t.Errorf("expected preflight 204, got %d", rr.Code) + } + if got := rr.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("expected no Access-Control-Allow-Origin on preflight for unlisted origin, got %q", got) + } +} diff --git a/frontend/src/lib/components/account/UserBookingModal.svelte b/frontend/src/lib/components/account/UserBookingModal.svelte index 3a97f6f..f3ec831 100644 --- a/frontend/src/lib/components/account/UserBookingModal.svelte +++ b/frontend/src/lib/components/account/UserBookingModal.svelte @@ -16,7 +16,7 @@ import type { Booking, BookingDiscount, Payment } from '$lib/types/booking'; import CardSelection from '$lib/components/payments/CardSelection.svelte'; import { savedCardsStore, type SavedCard } from '$lib/stores/savedCards.svelte'; - import { canSaveCardsForRole, isNonceStale } from '$lib/square/square'; + import { canSaveCardsForRole, isNonceStale, submitPaymentWithRetry } from '$lib/square/square'; interface Props { open: boolean; bookingId: string; @@ -164,6 +164,11 @@ // verification tokens expire after ~5 minutes, so a stale pair is discarded // on late retries and re-tokenized instead of rejected by Square. let tipTokenizedAt = $state(0); + // Save intent at tokenization time: the SCA verification token is bound to + // CHARGE vs CHARGE_AND_STORE, so toggling the save-card checkbox after a + // tokenize must force a fresh tokenization rather than reuse a token minted + // with the wrong intent. + let tipTokenizedForSaveCard = $state(false); const canSaveCards = $derived(canSaveCardsForRole(authStore.currentUser?.role)); @@ -242,7 +247,11 @@ // verification token on retry (tokenization is one-shot; the backend // idempotency key dedups). The verification token is amount-bound, so // a changed tip amount forces a fresh tokenization. - if (!tipNonce || isNonceStale(tipTokenizedAt, tipTokenAmount, tipAmount)) { + if ( + !tipNonce || + tipTokenizedForSaveCard !== tipSaveCard || + isNonceStale(tipTokenizedAt, tipTokenAmount, tipAmount) + ) { try { const tokenized = await tipCardSelection.tokenizeWithVerification( Math.round(tipAmount * 100), @@ -250,12 +259,14 @@ givenName: authStore.currentUser?.firstName, familyName: authStore.currentUser?.lastName, email: authStore.currentUser?.email - } + }, + tipSaveCard ); tipNonce = tokenized.nonce; tipVerificationToken = tokenized.verificationToken ?? ''; tipTokenAmount = tipAmount; tipTokenizedAt = Date.now(); + tipTokenizedForSaveCard = tipSaveCard; } catch (err) { toast.error(err instanceof Error ? err.message : 'Card entry failed'); return; @@ -271,6 +282,7 @@ tipProcessing = true; try { + const bookingId = selectedBooking.id; // New-card identity is a STABLE sentinel, NOT the cnon: nonce (same // rationale as the booking/account flows). Include the card so a // same-amount tip on a DIFFERENT card gets a fresh key instead of @@ -289,11 +301,13 @@ ...(verificationToken ? { verification_token: verificationToken } : {}) }; - const response = await apiFetch(`/api/bookings/${selectedBooking.id}/tip`, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify(body) - }); + const response = await submitPaymentWithRetry(() => + apiFetch(`/api/bookings/${bookingId}/tip`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body) + }) + ); if (!response.ok) { const errorText = await response.text(); throw new Error(errorText || 'Tip payment failed'); @@ -306,6 +320,7 @@ tipVerificationToken = ''; tipTokenAmount = 0; tipTokenizedAt = 0; + tipTokenizedForSaveCard = false; showTipModal = false; fetchBookingDetails(); } catch (err) { @@ -317,6 +332,7 @@ tipVerificationToken = ''; tipTokenAmount = 0; tipTokenizedAt = 0; + tipTokenizedForSaveCard = false; } finally { tipProcessing = false; } @@ -1156,6 +1172,7 @@ ${hasVAT ? `

VAT is included at ${biz?.default_vat_rate ?? 20} tipVerificationToken = ''; tipTokenAmount = 0; tipTokenizedAt = 0; + tipTokenizedForSaveCard = false; } }} > diff --git a/frontend/src/lib/components/admin/GiftCardsManagement.svelte b/frontend/src/lib/components/admin/GiftCardsManagement.svelte index 0e2703f..2a92416 100644 --- a/frontend/src/lib/components/admin/GiftCardsManagement.svelte +++ b/frontend/src/lib/components/admin/GiftCardsManagement.svelte @@ -10,7 +10,7 @@ import * as Modal from '$lib/components/ui/dialog'; import { Skeleton } from '$lib/components/ui/skeleton'; import SquareCardInput from '$lib/components/payments/SquareCardInput.svelte'; - import { isSquareConfigured } from '$lib/square/square'; + import { isSquareConfigured, submitPaymentWithRetry } from '$lib/square/square'; import { range } from '$lib/utils/format'; import { formatUserName } from '$lib/utils/nameDisplay'; import { parseWallClockDate } from '$lib/utils/timeSlots'; @@ -646,13 +646,15 @@ if (actionType === 'create' && generateType === 'account' && selectedCustomer) body.redeem_to_user_id = selectedCustomer.id; - const res = await apiFetch('/api/admin/till/sale', { - method: 'POST', - headers: { - 'Content-Type': 'application/json' - }, - body: JSON.stringify(body) - }); + const res = await submitPaymentWithRetry(() => + apiFetch('/api/admin/till/sale', { + method: 'POST', + headers: { + 'Content-Type': 'application/json' + }, + body: JSON.stringify(body) + }) + ); if (res.ok) { const data = await res.json(); paymentResult = { ...data }; diff --git a/frontend/src/lib/components/admin/TillPurchases.svelte b/frontend/src/lib/components/admin/TillPurchases.svelte index 567a58a..c1c44d7 100644 --- a/frontend/src/lib/components/admin/TillPurchases.svelte +++ b/frontend/src/lib/components/admin/TillPurchases.svelte @@ -7,7 +7,7 @@ import { extractErrorMessage } from '$lib/utils/toast-safe'; import { apiFetch } from '$lib/utils/api'; import SquareCardInput from '$lib/components/payments/SquareCardInput.svelte'; - import { isSquareConfigured } from '$lib/square/square'; + import { isSquareConfigured, submitPaymentWithRetry } from '$lib/square/square'; type CartItem = { id: string; @@ -290,11 +290,13 @@ } for (const body of saleBodies) { - const res = await apiFetch('/api/admin/till/sale', { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify(body) - }); + const res = await submitPaymentWithRetry(() => + apiFetch('/api/admin/till/sale', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body) + }) + ); if (!res.ok) { const errText = await res.text(); throw new Error(extractErrorMessage(errText) || 'Till sale failed'); diff --git a/frontend/src/lib/components/booking/BookingFlow.svelte b/frontend/src/lib/components/booking/BookingFlow.svelte index 5f538c1..dcd4e7a 100644 --- a/frontend/src/lib/components/booking/BookingFlow.svelte +++ b/frontend/src/lib/components/booking/BookingFlow.svelte @@ -38,7 +38,7 @@ import CardSelection from '$lib/components/payments/CardSelection.svelte'; import PolicyPopover from '$lib/components/ui/policyPopover.svelte'; import { POLICY } from '$lib/constants/policy'; - import { canSaveCardsForRole, isNonceStale } from '$lib/square/square'; + import { canSaveCardsForRole, isNonceStale, submitPaymentWithRetry } from '$lib/square/square'; import UserPaymentModal from '$lib/components/payments/UserPaymentModal.svelte'; import { extractBookedSlots, getLunchProtectionForSlots } from '$lib/lunchProtection'; import { @@ -123,6 +123,11 @@ // verification tokens expire after ~5 minutes, so a stale pair is discarded // on late retries and re-tokenized instead of rejected by Square. let depositTokenizedAt = $state(0); + // Save intent at tokenization time: the SCA verification token is bound to + // CHARGE vs CHARGE_AND_STORE, so toggling the save-card checkbox after a + // tokenize must force a fresh tokenization rather than reuse a token minted + // with the wrong intent. + let depositTokenizedForSaveCard = $state(false); // Synchronous double-click guard. Svelte 5 reactivity is async (effects run // on the next microtask), so `isProcessingPayment` may not propagate to the // button's `disabled` binding before a fast second click fires. This non- @@ -331,17 +336,26 @@ // created so the SCA verification amount matches the exact charge. // The verification token is amount-bound, so a changed deposit // amount forces a fresh tokenization. - if (!depositNonce || isNonceStale(depositTokenizedAt, depositTokenAmount, amountCents)) { + if ( + !depositNonce || + depositTokenizedForSaveCard !== depositSaveCard || + isNonceStale(depositTokenizedAt, depositTokenAmount, amountCents) + ) { try { - const tokenized = await paymentCardSelection.tokenizeWithVerification(amountCents, { - givenName: customerInfo.firstName || authStore.currentUser?.firstName, - familyName: customerInfo.lastName || authStore.currentUser?.lastName, - email: customerInfo.email || authStore.currentUser?.email - }); + const tokenized = await paymentCardSelection.tokenizeWithVerification( + amountCents, + { + givenName: customerInfo.firstName || authStore.currentUser?.firstName, + familyName: customerInfo.lastName || authStore.currentUser?.lastName, + email: customerInfo.email || authStore.currentUser?.email + }, + depositSaveCard + ); depositNonce = tokenized.nonce; depositVerificationToken = tokenized.verificationToken ?? ''; depositTokenAmount = amountCents; depositTokenizedAt = Date.now(); + depositTokenizedForSaveCard = depositSaveCard; } catch (err) { toast.error(err instanceof Error ? err.message : 'Card entry failed'); return; @@ -381,14 +395,16 @@ paymentAttempted = true; - const response = await apiFetch(`/api/bookings/${bookingId}/payment`, { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - ...getAuthHeaders() - }, - body: JSON.stringify(body) - }); + const response = await submitPaymentWithRetry(() => + apiFetch(`/api/bookings/${bookingId}/payment`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + ...getAuthHeaders() + }, + body: JSON.stringify(body) + }) + ); if (response.ok) { depositPaid = true; @@ -399,6 +415,7 @@ depositVerificationToken = ''; depositTokenAmount = 0; depositTokenizedAt = 0; + depositTokenizedForSaveCard = false; depositSaveCard = false; // Immutable update — avoid mutating the existing object so // concurrent renders (e.g. a stale fetch) can't observe partial diff --git a/frontend/src/lib/components/payments/CardSelection.svelte b/frontend/src/lib/components/payments/CardSelection.svelte index a1c9bbc..bf74ed6 100644 --- a/frontend/src/lib/components/payments/CardSelection.svelte +++ b/frontend/src/lib/components/payments/CardSelection.svelte @@ -75,16 +75,19 @@ * Tokenizes the new-card form with SCA verification details (SCA-mandated * for UK card-not-present charges). Returns the nonce AND the verification * token, which the caller must send to the backend as `verification_token` - * alongside the nonce so the charge completes. + * alongside the nonce so the charge completes. Pass `saveCard=true` when the + * card will ALSO be saved for reuse — the SCA intent becomes + * `CHARGE_AND_STORE` (Square requires it for charge-and-store flows). */ export async function tokenizeWithVerification( amount: number, - contact?: SquareVerificationContact + contact?: SquareVerificationContact, + saveCard: boolean = false ): Promise<{ nonce: string; verificationToken: string | null }> { if (!newCardMode || !squareCardInput) { throw new Error('No new card form is open'); } - return squareCardInput.tokenizeWithVerification(amount, contact); + return squareCardInput.tokenizeWithVerification(amount, contact, saveCard); } diff --git a/frontend/src/lib/components/payments/MockCardForm.svelte b/frontend/src/lib/components/payments/MockCardForm.svelte index b7d869b..dfc6cfd 100644 --- a/frontend/src/lib/components/payments/MockCardForm.svelte +++ b/frontend/src/lib/components/payments/MockCardForm.svelte @@ -184,7 +184,8 @@ givenName?: string; familyName?: string; email?: string; - } + }, + _saveCard: boolean = false ): Promise<{ nonce: string; verificationToken: string | null }> { if (!complete) { throw new Error('Card details are incomplete'); diff --git a/frontend/src/lib/components/payments/PaymentModal.svelte b/frontend/src/lib/components/payments/PaymentModal.svelte index b44e1c1..d5d6488 100644 --- a/frontend/src/lib/components/payments/PaymentModal.svelte +++ b/frontend/src/lib/components/payments/PaymentModal.svelte @@ -7,6 +7,7 @@ import { Checkbox } from '$lib/components/ui/checkbox'; import type { Booking, BookingService, BookingDiscount } from '$lib/types/booking'; import { apiFetch } from '$lib/utils/api'; +import { submitPaymentWithRetry } from '$lib/square/square'; const LOYALTY_DISCOUNT_RATE = 0.1; @@ -682,17 +683,19 @@ try { await applyLoyaltyRedemption(); - const response = await apiFetch(`/api/admin/bookings/${booking.id}/payment`, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ - amount: chargeAmount, - payment_type: 'full', - payment_method: 'saved_card', - saved_card_id: selectedSavedCardId, - idempotency_key: savedCardIdempotencyKey + const response = await submitPaymentWithRetry(() => + apiFetch(`/api/admin/bookings/${booking.id}/payment`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + amount: chargeAmount, + payment_type: 'full', + payment_method: 'saved_card', + saved_card_id: selectedSavedCardId, + idempotency_key: savedCardIdempotencyKey + }) }) - }); + ); if (!response.ok) { const errData = await response.text(); diff --git a/frontend/src/lib/components/payments/SquareCardInput.svelte b/frontend/src/lib/components/payments/SquareCardInput.svelte index 62d98f8..31469db 100644 --- a/frontend/src/lib/components/payments/SquareCardInput.svelte +++ b/frontend/src/lib/components/payments/SquareCardInput.svelte @@ -195,16 +195,23 @@ * integer string ("5000") would make Square's 3DS bind a * 100×-too-large amount and fail SCA. * @param contact Optional billing contact (name/email we already hold). + * @param saveCard When true the SCA verification is bound with intent + * `CHARGE_AND_STORE` (charge + save to Square for reuse). + * Square requires this intent — not bare `CHARGE` — for a + * card that will be both charged and stored; the SCA + * challenge must cover the store operation. When false the + * intent stays `CHARGE`. */ export async function tokenizeWithVerification( amount: number, - contact?: SquareVerificationContact + contact?: SquareVerificationContact, + saveCard: boolean = false ): Promise { if (isSquareMock()) { if (!mockForm) { throw new Error('Card form is not ready — please wait a moment and try again'); } - return mockForm.tokenizeWithVerification(amount, contact); + return mockForm.tokenizeWithVerification(amount, contact, saveCard); } const card = cardInstance as { tokenize: ( @@ -224,7 +231,11 @@ // monetary-value), e.g. "50.00" for £50.00 — NOT the minor-unit // integer ("5000"), which would bind a 100×-too-large 3DS amount. amount: (amount / 100).toFixed(2), - intent: 'CHARGE', + // CHARGE_AND_STORE binds the SCA challenge to BOTH the charge and + // the store, so the resulting verification token legally permits + // saving the card (SCA compliance). Bare CHARGE would only cover + // the charge. + intent: saveCard ? 'CHARGE_AND_STORE' : 'CHARGE', currencyCode: 'GBP', customerInitiated: true, sellerKeyedIn: false diff --git a/frontend/src/lib/components/payments/TipPayment.svelte b/frontend/src/lib/components/payments/TipPayment.svelte index c790ad8..99f69e8 100644 --- a/frontend/src/lib/components/payments/TipPayment.svelte +++ b/frontend/src/lib/components/payments/TipPayment.svelte @@ -11,7 +11,7 @@ import { Input } from '$lib/components/ui/input'; import * as Card from '$lib/components/ui/card'; import { onMount } from 'svelte'; - import { canSaveCardsForRole, isNonceStale } from '$lib/square/square'; + import { canSaveCardsForRole, isNonceStale, submitPaymentWithRetry } from '$lib/square/square'; // Shared tip-payment UI used by /tip and /pay-tip/[id]. The routes resolve // the booking (most-recent past booking vs. booking by URL id) and hand it @@ -77,6 +77,11 @@ // verification tokens expire after ~5 minutes, so a stale pair is discarded // on late retries and re-tokenized instead of rejected by Square. let tipTokenizedAt = $state(0); + // Save intent at tokenization time: the SCA verification token is bound to + // CHARGE vs CHARGE_AND_STORE, so toggling the save-card checkbox after a + // tokenize must force a fresh tokenization rather than reuse a token minted + // with the wrong intent. + let tipTokenizedForSaveCard = $state(false); const canSaveCards = $derived(canSaveCardsForRole(authStore.currentUser?.role)); @@ -210,7 +215,11 @@ // verification token on retry (tokenization is one-shot; the backend // idempotency key dedups). The verification token is amount-bound, so // a changed tip amount forces a fresh tokenization. - if (!tipNonce || isNonceStale(tipTokenizedAt, tipTokenAmount, tipAmount)) { + if ( + !tipNonce || + tipTokenizedForSaveCard !== saveCard || + isNonceStale(tipTokenizedAt, tipTokenAmount, tipAmount) + ) { try { const tokenized = await cardSelection.tokenizeWithVerification( Math.round(tipAmount * 100), @@ -218,12 +227,14 @@ givenName: authStore.currentUser?.firstName, familyName: authStore.currentUser?.lastName, email: authStore.currentUser?.email - } + }, + saveCard ); tipNonce = tokenized.nonce; tipVerificationToken = tokenized.verificationToken ?? ''; tipTokenAmount = tipAmount; tipTokenizedAt = Date.now(); + tipTokenizedForSaveCard = saveCard; } catch (err) { toast.error(err instanceof Error ? err.message : 'Card entry failed'); return; @@ -258,11 +269,13 @@ ...(verificationToken ? { verification_token: verificationToken } : {}) }; - const response = await apiFetch(`/api/bookings/${booking.id}/tip`, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify(body) - }); + const response = await submitPaymentWithRetry(() => + apiFetch(`/api/bookings/${booking.id}/tip`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body) + }) + ); if (!response.ok) { const errorText = await response.text(); @@ -277,6 +290,7 @@ tipVerificationToken = ''; tipTokenAmount = 0; tipTokenizedAt = 0; + tipTokenizedForSaveCard = false; toast.success('Thank you for your tip!'); } catch (err) { paymentState = 'error'; @@ -290,6 +304,7 @@ tipVerificationToken = ''; tipTokenAmount = 0; tipTokenizedAt = 0; + tipTokenizedForSaveCard = false; } } diff --git a/frontend/src/lib/components/payments/UserPaymentModal.svelte b/frontend/src/lib/components/payments/UserPaymentModal.svelte index ec66b7f..ea307ea 100644 --- a/frontend/src/lib/components/payments/UserPaymentModal.svelte +++ b/frontend/src/lib/components/payments/UserPaymentModal.svelte @@ -12,7 +12,7 @@ import PolicyPopover from '$lib/components/ui/policyPopover.svelte'; import { authStore } from '$lib/stores/auth.svelte'; import { apiFetch } from '$lib/utils/api'; - import { isNonceStale } from '$lib/square/square'; + import { isNonceStale, submitPaymentWithRetry } from '$lib/square/square'; const LOYALTY_DISCOUNT_RATE = 0.1; @@ -60,6 +60,11 @@ // verification tokens expire after ~5 minutes, so a stale pair is discarded // on late retries and re-tokenized instead of rejected by Square. let newCardTokenizedAt = $state(0); + // Save intent at tokenization time: the SCA verification token is bound to + // CHARGE vs CHARGE_AND_STORE, so toggling the save-card checkbox after a + // tokenize must force a fresh tokenization rather than reuse a token minted + // with the wrong intent. + let newCardTokenizedForSaveCard = $state(false); let paymentResult = $state<{ id: string; amount: number; @@ -374,17 +379,26 @@ // is one-shot; the backend idempotency key dedups). The verification // token is amount-bound, so a changed amount forces a fresh // tokenization. - if (!newCardNonce || isNonceStale(newCardTokenizedAt, newCardTokenAmount, amountCents)) { + if ( + !newCardNonce || + newCardTokenizedForSaveCard !== saveCard || + isNonceStale(newCardTokenizedAt, newCardTokenAmount, amountCents) + ) { try { - const tokenized = await cardSelection.tokenizeWithVerification(amountCents, { - givenName: authStore.currentUser?.firstName, - familyName: authStore.currentUser?.lastName, - email: authStore.currentUser?.email - }); + const tokenized = await cardSelection.tokenizeWithVerification( + amountCents, + { + givenName: authStore.currentUser?.firstName, + familyName: authStore.currentUser?.lastName, + email: authStore.currentUser?.email + }, + saveCard + ); newCardNonce = tokenized.nonce; newCardVerificationToken = tokenized.verificationToken ?? ''; newCardTokenAmount = amountCents; newCardTokenizedAt = Date.now(); + newCardTokenizedForSaveCard = saveCard; } catch (_err) { status = 'error'; const msg = _err instanceof Error ? _err.message : 'Card entry failed'; @@ -423,18 +437,20 @@ } try { - const response = await apiFetch(`/api/bookings/${booking.id}/payment`, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ - amount: amountCents, - payment_type: paymentType, - ...(cardId ? { card_id: cardId } : {}), - ...(newCardToken ? { new_card_token: newCardToken, save_card: saveCard } : {}), - ...(verificationToken ? { verification_token: verificationToken } : {}), - idempotency_key: payIdempotencyKey + const response = await submitPaymentWithRetry(() => + apiFetch(`/api/bookings/${booking.id}/payment`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + amount: amountCents, + payment_type: paymentType, + ...(cardId ? { card_id: cardId } : {}), + ...(newCardToken ? { new_card_token: newCardToken, save_card: saveCard } : {}), + ...(verificationToken ? { verification_token: verificationToken } : {}), + idempotency_key: payIdempotencyKey + }) }) - }); + ); if (!response.ok) { const errData = await response.text(); @@ -452,6 +468,7 @@ newCardVerificationToken = ''; newCardTokenAmount = 0; newCardTokenizedAt = 0; + newCardTokenizedForSaveCard = false; paymentResult = { id: data.id, amount: data.amount, @@ -477,6 +494,7 @@ newCardVerificationToken = ''; newCardTokenAmount = 0; newCardTokenizedAt = 0; + newCardTokenizedForSaveCard = false; releaseLock(); } } diff --git a/frontend/src/lib/square/square.ts b/frontend/src/lib/square/square.ts index 9860a07..b28416e 100644 --- a/frontend/src/lib/square/square.ts +++ b/frontend/src/lib/square/square.ts @@ -44,6 +44,51 @@ export function isNonceStale( return nonceTokenizedFor !== amount || now - nonceTokenizedAt > NONCE_STALENESS_MS; } +/** + * Payment failure classification mirrors the backend's `chargeFailureStatus`: + * - 503 (ambiguous): transport/network errors, Square 5xx, context + * cancellation — the pending record stays resumable, so a same-key retry + * makes the backend re-attempt/reuse it instead of double-charging. + * - 402 (definitive): card declined, expired, AVS/CVV failure — retrying + * with the same inputs can never succeed. + */ +export const PAYMENT_AMBIGUOUS_STATUS = 503; +export const PAYMENT_DEFINITIVE_STATUS = 402; + +/** True when a payment submission response is an ambiguous failure (503) that + * must be retried with the SAME idempotency key so the backend resumes the + * pending record. */ +export function isAmbiguousPaymentFailure(status: number): boolean { + return status === PAYMENT_AMBIGUOUS_STATUS; +} + +const PAYMENT_RETRY_DELAY_MS = 1500; +const PAYMENT_MAX_RETRIES = 3; + +/** + * Submits a payment and transparently retries on ambiguous (503) responses + * with the SAME request body — the body carries the idempotency key, so every + * retry reuses it and the backend re-attempts / resumes the pending record + * instead of issuing a second charge. Definitive failures (402) and every + * other status return immediately so the caller surfaces the error; the caller + * must NOT retry those on its own. Defaults to up to 3 retries with a 1.5s + * backoff (Square/backend transient failures typically clear within seconds). + */ +export async function submitPaymentWithRetry( + submit: () => Promise, + options: { maxRetries?: number; retryDelayMs?: number } = {} +): Promise { + const maxRetries = options.maxRetries ?? PAYMENT_MAX_RETRIES; + const retryDelayMs = options.retryDelayMs ?? PAYMENT_RETRY_DELAY_MS; + for (let attempt = 0; ; attempt++) { + const response = await submit(); + if (response.ok || !isAmbiguousPaymentFailure(response.status) || attempt >= maxRetries) { + return response; + } + await new Promise((resolve) => setTimeout(resolve, retryDelayMs)); + } +} + /** True when the frontend runs in local-dev mock mode: VITE_SQUARE_ENVIRONMENT === 'mock' * AND the dev build (import.meta.env.DEV). The DEV gate makes the mock structurally * impossible in any production bundle — even if the env var is mis-set at build time. */ diff --git a/frontend/src/routes/account/+page.svelte b/frontend/src/routes/account/+page.svelte index 62887e4..440481a 100644 --- a/frontend/src/routes/account/+page.svelte +++ b/frontend/src/routes/account/+page.svelte @@ -8,7 +8,7 @@ import CardEntryUnavailable from '$lib/components/payments/CardEntryUnavailable.svelte'; import CardSelection from '$lib/components/payments/CardSelection.svelte'; import SquareCardInput from '$lib/components/payments/SquareCardInput.svelte'; - import { canSaveCardsForRole, isNonceStale, isSquareConfigured } from '$lib/square/square'; + import { canSaveCardsForRole, isNonceStale, isSquareConfigured, submitPaymentWithRetry } from '$lib/square/square'; import { extractErrorMessage, sanitizeText } from '$lib/utils/toast-safe'; import { apiFetch } from '$lib/utils/api'; import UserBookingModal from '$lib/components/account/UserBookingModal.svelte'; @@ -219,6 +219,11 @@ // verification tokens expire after ~5 minutes, so a stale pair is discarded // on late retries and re-tokenized instead of rejected by Square. let buyTokenizedAt = $state(0); + // Save intent at tokenization time: the SCA verification token is bound to + // CHARGE vs CHARGE_AND_STORE, so toggling the save-card checkbox after a + // tokenize must force a fresh tokenization rather than reuse a token minted + // with the wrong intent. + let buyTokenizedForSaveCard = $state(false); // Cached idempotency key: generated once per purchase attempt, reused on // retry (so a lost-response retry dedups instead of double-charging), @@ -283,17 +288,26 @@ // New-card mode: tokenize once per attempt, reuse the nonce + SCA // verification token on retry (tokenization is one-shot; the backend // idempotency key dedups). - if (!buyNonce || isNonceStale(buyTokenizedAt, buyTokenAmount, buyAmount * 100)) { + if ( + !buyNonce || + buyTokenizedForSaveCard !== buySaveCard || + isNonceStale(buyTokenizedAt, buyTokenAmount, buyAmount * 100) + ) { try { - const tokenized = await buyCardSelection.tokenizeWithVerification(buyAmount * 100, { - givenName: userData?.firstName, - familyName: userData?.lastName, - email: userData?.email - }); + const tokenized = await buyCardSelection.tokenizeWithVerification( + buyAmount * 100, + { + givenName: userData?.firstName, + familyName: userData?.lastName, + email: userData?.email + }, + buySaveCard + ); buyNonce = tokenized.nonce; buyVerificationToken = tokenized.verificationToken ?? ''; buyTokenAmount = buyAmount * 100; buyTokenizedAt = Date.now(); + buyTokenizedForSaveCard = buySaveCard; } catch (err) { toast.error(err instanceof Error ? err.message : 'Card entry failed'); buyingGiftCard = false; @@ -328,19 +342,21 @@ buyKeyedCard = cardKey; } - const res = await apiFetch('/api/user/giftcards/buy', { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ - amount: buyAmount * 100, // cents - recipient_type: buyRecipientType, - recipient_email: buyRecipientEmail, - ...(cardId ? { card_id: cardId } : {}), - ...(newCardToken ? { new_card_token: newCardToken, save_card: buySaveCard } : {}), - ...(verificationToken ? { verification_token: verificationToken } : {}), - idempotency_key: buyIdempotencyKey + const res = await submitPaymentWithRetry(() => + apiFetch('/api/user/giftcards/buy', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + amount: buyAmount * 100, // cents + recipient_type: buyRecipientType, + recipient_email: buyRecipientEmail, + ...(cardId ? { card_id: cardId } : {}), + ...(newCardToken ? { new_card_token: newCardToken, save_card: buySaveCard } : {}), + ...(verificationToken ? { verification_token: verificationToken } : {}), + idempotency_key: buyIdempotencyKey + }) }) - }); + ); if (res.ok) { const data = await res.json(); @@ -353,6 +369,7 @@ buyVerificationToken = ''; buyTokenAmount = 0; buyTokenizedAt = 0; + buyTokenizedForSaveCard = false; await fetchGiftCardBalance(); } else { const errText = await res.text(); @@ -365,6 +382,7 @@ buyVerificationToken = ''; buyTokenAmount = 0; buyTokenizedAt = 0; + buyTokenizedForSaveCard = false; } } catch (err) { console.error('buyGiftCard error:', err); diff --git a/init-scripts/init-script.sql b/init-scripts/init-script.sql index 7b3d737..ece903b 100644 --- a/init-scripts/init-script.sql +++ b/init-scripts/init-script.sql @@ -160,6 +160,10 @@ CREATE OR REPLACE FUNCTION generate_admin_audit_log_id() RETURNS CHAR(12) AS $$ SELECT generate_short_id('admin_audit_log'); $$ LANGUAGE sql; +CREATE OR REPLACE FUNCTION generate_dispute_id() RETURNS CHAR(12) AS $$ + SELECT generate_short_id('disputes'); +$$ LANGUAGE sql; + CREATE OR REPLACE FUNCTION generate_verification_code() RETURNS CHAR(12) AS $$ SELECT substr(encode(gen_random_bytes(6), 'hex'), 1, 12); $$ LANGUAGE sql; CREATE OR REPLACE FUNCTION generate_referral_code() @@ -926,6 +930,8 @@ BEGIN phone = '+000000000000', -- NOT NULL column: use placeholder date_of_birth = '1900-01-01', -- NOT NULL column: use placeholder profile_pic_url = NULL, + notes = NULL, -- free-text staff notes may contain PII + last_login_at = NULL, -- removes activity metadata (right to be forgotten) account_role = 'guest', loyalty_stamps = 0, referral_code = NULL, @@ -971,6 +977,18 @@ BEGIN SET notes = NULL WHERE requested_by = target_id; + -- Scrub notes on this user's bookings (free-text PII written by the user) + UPDATE bookings + SET notes = NULL + WHERE user_id = target_id; + + -- Scrub previous-name history (names are PII; leaving them would let + -- "formerly [name]" receipts reveal the erased identity) + UPDATE name_history + SET previous_first_name = 'Deleted', + previous_last_name = 'User' + WHERE user_id = target_id; + -- Clear notification preferences (no contractual basis after account closure) DELETE FROM user_notification_preferences WHERE user_id = target_id; @@ -986,6 +1004,9 @@ BEGIN -- Clear login audit trail (no ongoing legal basis after account closure) DELETE FROM login_audit WHERE user_id = target_id; + + -- Revoke stored refresh tokens (auth credentials of an erased identity) + DELETE FROM refresh_tokens WHERE user_id = target_id; END; $$ LANGUAGE plpgsql; @@ -1014,6 +1035,9 @@ BEGIN -- Clear login audit trail before deleting the user row DELETE FROM login_audit WHERE user_id = target_id; + -- Revoke stored refresh tokens (auth credentials of an erased identity) + DELETE FROM refresh_tokens WHERE user_id = target_id; + DELETE FROM users WHERE id = target_id AND account_role = 'guest'; END; $$ LANGUAGE plpgsql; @@ -2305,6 +2329,28 @@ CREATE TABLE IF NOT EXISTS square_webhook_events ( received_at TIMESTAMPTZ NOT NULL DEFAULT NOW() ); +-- ======================================= +-- DISPUTES TABLE +-- Chargeback/dispute tracking surfaced from Square webhooks (dispute.created, +-- dispute.state.updated). Each row is one Square dispute against a local +-- payment, with the terminal resolution (open → won/lost) recorded so the +-- admin can see chargebacks in the DB-backed 'critical_payment_log' +-- notification centre instead of un-watched CRITICAL log lines. +-- ======================================= +CREATE TABLE IF NOT EXISTS disputes ( + id CHAR(12) PRIMARY KEY DEFAULT generate_dispute_id(), + square_dispute_id VARCHAR(64) UNIQUE NOT NULL, + payment_id CHAR(12) NOT NULL REFERENCES payments(id), + status VARCHAR(32) NOT NULL DEFAULT 'open', -- open, won, lost + amount NUMERIC(10,2) NOT NULL, + reason VARCHAR(192), + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW() +); + +CREATE INDEX idx_disputes_payment ON disputes(payment_id); +CREATE INDEX idx_disputes_status ON disputes(status); + -- ======================================= -- CARDDAV / CALDAV TABLES (used by SabreDAV sync in Go backend) -- =======================================