diff --git a/.env.example b/.env.example index 685dab0..658df63 100644 --- a/.env.example +++ b/.env.example @@ -41,6 +41,13 @@ SQUARE_ACCESS_TOKEN= SQUARE_LOCATION_ID= SQUARE_TERMINAL_DEVICE_ID= SQUARE_ENVIRONMENT=mock +# SQUARE_ALLOW_REAL_API — dev-build safety valve. In a `//go:build dev` build the +# backend HARD-FAILS (refuses to construct the client) when SQUARE_ENVIRONMENT +# is 'production', because a leftover/typo'd production env + real key in a dev +# shell would create real charges. Set SQUARE_ALLOW_REAL_API=1 ONLY to +# deliberately route a dev build to the real production API. Never set in a +# deployed production build. +SQUARE_ALLOW_REAL_API= # 2FA (PSD2 SCA stand-in) for online card payments. Enforcement is FAIL-CLOSED: # ON unless REQUIRE_2FA explicitly disables it (false/0/off/no, case-insensitive) # OR SQUARE_ENVIRONMENT explicitly equals one of mock/dev/development/test. @@ -53,6 +60,9 @@ SQUARE_ENVIRONMENT=mock # environments an operator must relay the logged code to the user out-of-band; # the API never returns the code while enforcement is ON. REQUIRE_2FA=true +# TWO_FACTOR_PEPPER — server-side pepper for HMAC-hashing 2FA codes (optional +# pre-launch; if unset, codes are hashed without pepper and a warning is logged) +TWO_FACTOR_PEPPER= # Webhook config MUST exactly match the Square Dashboard webhook subscription # (URL + signature key). If SQUARE_WEBHOOK_NOTIFICATION_URL is left unset it # defaults to http://localhost:8080/webhooks/square, which is fail-closed (503 @@ -92,6 +102,11 @@ NO_COLOR= # Frontend VITE_BACKEND_URL=http://localhost:8080 +# Backend CORS allowlist — comma-separated list of allowed frontend origins +# (read by the backend CORS middleware, see backend/main.go). Falls back to +# http://localhost:5173 when unset. +FRONTEND_ORIGIN=http://localhost:5173 + # Local S3 (Rustfs) — requires GO_TESTING=1 or dev build tag # These are dev-only overrides used by the dev S3 implementation RUSTFS_ENDPOINT=http://rustfs:9000 diff --git a/README.md b/README.md index 4409d54..379aa46 100644 --- a/README.md +++ b/README.md @@ -58,6 +58,10 @@ docker compose up --build -d `VITE_SQUARE_ENVIRONMENT=mock` (default in `frontend/.env`) makes the frontend render its built-in mock card form, pairing with the backend's `SQUARE_ENVIRONMENT=mock` for a token-only local walkthrough. Set it to `sandbox` or `production` only once real Square credentials are configured, never `mock` in a deployed build. +`FRONTEND_ORIGIN` (backend `.env`) is a comma-separated CORS allowlist for the API. `corsAllowedOrigins()` in `backend/main.go` splits on commas, trims, drops blanks, and falls back to `http://localhost:5173` when the var is unset or empty. Matching is exact-match only (`originAllowed()`), never reflected: `Access-Control-Allow-Origin` and `Vary: Origin` are set only when the request `Origin` is in the allowlist. + +`SQUARE_ALLOW_REAL_API` is a dev-build safety valve: a `//go:build dev` build **HARD-FAILS** (panics) when `SQUARE_ENVIRONMENT=production` unless this is set to `1`, so a typo'd or leftover production value in a dev shell cannot create real charges. Sandbox is allowed in a dev build (with a loud banner). Never set it in a deployed production build. + | Service | URL | |---------|-----| | Frontend | http://localhost | @@ -72,6 +76,8 @@ docker compose up --build -d `REQUIRE_2FA` gates saved-card online payments (PSD2 SCA stand-in) and is **fail-closed**: enforcement is ON by default for any `SQUARE_ENVIRONMENT` except an explicit `mock`/`dev`/`development`/`test` value — empty or unknown values are treated as production-enforced. Disable it with `REQUIRE_2FA=false` or an explicit mock env. The 6-digit code is delivered via the server log (`[2FA]` prefix; the operator relays it) until email/SMS lands. +`TWO_FACTOR_PEPPER` (backend `.env`, optional pre-launch) is a server-side pepper for HMAC-hashing 2FA codes: with it set, codes are hashed as HMAC-SHA256 keyed by the pepper; if unset, they fall back to the legacy unsalted SHA-256 digest and a one-time warning is logged. Set it in production so a leaked digest cannot be brute-forced offline. + ### Local dev (tmux) ```bash @@ -89,7 +95,7 @@ Default logins (password: `password`): ```bash cd backend && go build -o bin/backend ./main.go cd frontend && npm ci && npm run build -cd backend && go test -tags "test,dev" -count=1 -parallel 8 ./... # 2,137 tests compiled under the test,dev tags (~2min) +cd backend && go test -tags "test,dev" -count=1 -parallel 8 ./... # 2,169 tests compiled under the test,dev tags (~2min) cd backend && go test -tags "test,dev" -count=1 -race -timeout 480s ./... # race detector (all packages, ~4min) # NOTE: -count=N>1 is unreliable for handlers/payments and handlers/webhooks — # those suites share package-global state (Square mock ledger, in-memory webhook diff --git a/backend/handlers/bookings/bookings.go b/backend/handlers/bookings/bookings.go index f35a09c..dab7699 100644 --- a/backend/handlers/bookings/bookings.go +++ b/backend/handlers/bookings/bookings.go @@ -2381,6 +2381,21 @@ func CreateBookingHandler(w http.ResponseWriter, r *http.Request) { } } + // booking.TotalAmount is still 0 here: the INSERT..RETURNING row predates + // the recalc_booking_duration_and_total trigger (fired by the + // booking_services INSERT above). Re-read the trigger-maintained values so + // the response and deposit fields use the real total — with 0, DepositPaid + // computes TRUE on an unpaid booking, and the deposit is never charged. + var bookingTotal float64 + if err := db.Conn.QueryRow(r.Context(), ` + SELECT total_amount, total_duration_minutes FROM bookings WHERE id = $1 + `, booking.ID).Scan(&bookingTotal, &booking.DurationMinutes); err != nil { + log.Printf("Failed to re-read booking total after creation: %v", err) + } + booking.TotalAmount = bookingTotal + booking.AmountPaid = 0 + booking.AmountDue = bookingTotal + // Populate deposit display fields on the creation response. // No payments exist yet so pre-start paid is 0 and DepositPaid will be false. populateDepositFields(&booking, depositRequiredSnapshot, 0) diff --git a/backend/handlers/bookings/bookings_test.go b/backend/handlers/bookings/bookings_test.go index 73ce104..b980df4 100644 --- a/backend/handlers/bookings/bookings_test.go +++ b/backend/handlers/bookings/bookings_test.go @@ -4232,6 +4232,65 @@ func TestBookings_Create_DepositRequired_OneActiveBookingLimit(t *testing.T) { } } +// TestBookings_Create_DepositPaidFalseOnUnpaidBooking is a regression test for +// the P0 deposit-never-charged bug: CreateBookingHandler returned the booking +// from INSERT..RETURNING, which predates the recalc trigger, so TotalAmount +// serialized as 0 and DepositPaid computed TRUE on an unpaid booking. The +// frontend gate then trusted deposit_paid:true and never charged the deposit. +// The create response must report the real trigger-maintained total and +// deposit_paid=false. +func TestBookings_Create_DepositPaidFalseOnUnpaidBooking(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) + } + defer fixtures.DeleteUser(tx, userID) + + if _, err = tx.Exec(ctx, "UPDATE users SET deposits_required = 3 WHERE id = $1", userID); err != nil { + t.Fatalf("failed to set deposits_required: %v", err) + } + + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create test service: %v", err) + } + defer fixtures.DeleteService(tx, serviceID) + + var price float64 + if err := tx.QueryRow(ctx, "SELECT price FROM services WHERE id = $1", serviceID).Scan(&price); err != nil { + t.Fatalf("failed to read service price: %v", err) + } + + token := jwt.GenerateUserToken(userID) + start := clock.Now().Add(72 * time.Hour).Truncate(time.Second) + start = time.Date(start.Year(), start.Month(), start.Day(), 10, 0, 0, 0, start.Location()) + + w := makeRequest(http.HandlerFunc(CreateBookingHandler), "POST", "/api/bookings", + CreateBookingRequest{StartTime: start, ServiceIDs: []string{serviceID}}, token, ctx) + + if w.Code != http.StatusCreated { + t.Fatalf("expected status 201, got %d. body: %s", w.Code, w.Body.String()) + } + + var booking Booking + if err := parseResponseBody(w, &booking); err != nil { + t.Fatalf("failed to parse create response: %v", err) + } + + if booking.TotalAmount != price { + t.Errorf("create response TotalAmount = %v, want service price %v (must not be 0)", booking.TotalAmount, price) + } + if booking.DepositRequired && booking.DepositAmount <= 0 { + t.Errorf("create response DepositAmount = %v, want > 0 for a deposit-required booking", booking.DepositAmount) + } + if booking.DepositPaid { + t.Error("create response DepositPaid = true for a freshly created UNPAID booking — this defeats the frontend deposit gate and the deposit is never charged") + } +} + // TestBookings_Get_DepositFieldsReturned verifies that GET /api/bookings returns // the deposit-related fields (deposit_required, deposit_amount, deposit_paid, deposit_deadline). func TestBookings_Get_DepositFieldsReturned(t *testing.T) { diff --git a/backend/handlers/payments/charge_helpers.go b/backend/handlers/payments/charge_helpers.go index 72bd9df..aa61afe 100644 --- a/backend/handlers/payments/charge_helpers.go +++ b/backend/handlers/payments/charge_helpers.go @@ -7,6 +7,7 @@ import ( "net/http" "crussell/db" + "crussell/internal/square" "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgxpool" @@ -55,6 +56,20 @@ func resolveChargeSource(ctx context.Context, w http.ResponseWriter, svc *Paymen savedRowID, saveErr := svc.SaveCardForUser(ctx, userID, sqCustomerID, cardOnFile.CardID, cardOnFile.Brand, cardOnFile.Last4, cardOnFile.ExpMonth, cardOnFile.ExpYear, cardOnFile.Fingerprint) if saveErr != nil { log.Printf("Failed to save card: %v", saveErr) + // The Square card was JUST created by this call (CreateCardOnFile + // above) but the local DB save failed, so the card-on-file is + // orphaned at Square — no user_saved_cards row references it, yet + // it is a live, chargeable card. Best-effort cleanup: disable it + // so it cannot be charged without a DB row. This is deliberately + // NOT the payment-failure path below — that path intentionally + // keeps the card so the pending record's retry re-creates it via + // the deterministic sha256 idempotency key. Here the save never + // landed, so there is no retry to preserve. A cleanup failure must + // never fail the charge: log the redacted card id so the orphan is + // auditable for manual cleanup. + if delErr := SquareClient.DeleteCardOnFile(ctx, cardOnFile.CardID); delErr != nil { + log.Printf("WARN: created Square card %s not disabled after local save failed — orphan card-on-file requires manual cleanup: %v", square.TokenPrefix(cardOnFile.CardID), delErr) + } } else { savedCardID = &savedRowID } @@ -65,7 +80,8 @@ func resolveChargeSource(ctx context.Context, w http.ResponseWriter, svc *Paymen sourceID = *newCardToken } if savedCardID == nil && saveCard { - log.Printf("Card was not saved despite save_card=true for user %s", userID) + // sourceID is cardOnFile.CardID on this branch (the ccof card). + log.Printf("Card was not saved despite save_card=true for user %s (card %s)", userID, square.TokenPrefix(sourceID)) } return sourceID, savedCardID, squareCustomerID, true } diff --git a/backend/handlers/payments/charge_helpers_test.go b/backend/handlers/payments/charge_helpers_test.go new file mode 100644 index 0000000..80e2faf --- /dev/null +++ b/backend/handlers/payments/charge_helpers_test.go @@ -0,0 +1,134 @@ +//go:build test && dev + +package payments + +import ( + "context" + "errors" + "net/http/httptest" + "strings" + "sync" + "testing" + + "crussell/db" + "crussell/internal/square" + "crussell/testutils/fixtures" + + "github.com/stretchr/testify/require" +) + +// orphanCardClient wraps the dev Square client to record every DeleteCardOnFile +// call. Used to assert the save-card orphan cleanup in resolveChargeSource +// WITHOUT needing to reach the real Square API. failDelete simulates a Square +// disable failure so tests can verify the charge is not failed by cleanup. +type orphanCardClient struct { + square.SquareClient + mu sync.Mutex + deleted []string + failDelete bool +} + +func (c *orphanCardClient) DeleteCardOnFile(ctx context.Context, cardID string) error { + c.mu.Lock() + c.deleted = append(c.deleted, cardID) + c.mu.Unlock() + if c.failDelete { + return errors.New("square: network error disabling card at Square") + } + return nil +} + +func (c *orphanCardClient) deletedIDs() []string { + c.mu.Lock() + defer c.mu.Unlock() + return append([]string(nil), c.deleted...) +} + +// TestResolveChargeSource_SaveCard_HappyPath guards the save-card branch: the +// Square card is created, persisted locally via SaveCardForUser, and NO +// DeleteCardOnFile cleanup is triggered (a saved card must never be deleted). +func TestResolveChargeSource_SaveCard_HappyPath(t *testing.T) { + ctx := context.Background() + userID, err := fixtures.CreateTestUser(db.Conn) + require.NoError(t, err) + defer func() { + InvalidateSquareCustomerCache(userID) + _, _ = db.Conn.Exec(ctx, `DELETE FROM user_saved_cards WHERE user_id = $1`, userID) + _, _ = db.Conn.Exec(ctx, `DELETE FROM users WHERE id = $1`, userID) + }() + + origClient := SquareClient + rec := &orphanCardClient{SquareClient: square.NewDevClient()} + SquareClient = rec + defer func() { SquareClient = origClient }() + + token := "cnon:test-save-happy" + sourceID, savedCardID, sqCustID, ok := resolveChargeSource(ctx, httptest.NewRecorder(), NewPaymentService(), userID, &token, nil, true, "") + require.True(t, ok, "save-card source resolution must succeed on the happy path") + require.True(t, strings.HasPrefix(sourceID, "ccof:"), "source must be the created card-on-file, got %q", sourceID) + require.NotNil(t, savedCardID, "a successful SaveCardForUser must return the local row id") + require.NotEmpty(t, sqCustID, "the provisioned Square customer id must be returned") + + var rows int + require.NoError(t, db.Conn.QueryRow(ctx, `SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1 AND square_card_id = $2`, userID, sourceID).Scan(&rows)) + require.Equal(t, 1, rows, "the card must be persisted as a user_saved_cards row") + + require.Empty(t, rec.deletedIDs(), "a successfully saved card must never be disabled at Square") +} + +// TestResolveChargeSource_SaveCard_OrphanCleanedUp verifies the orphan-card +// fix: when CreateCardOnFile succeeds but the local DB save fails, the +// just-created Square card is disabled (DeleteCardOnFile) so no card-on-file +// is left at Square without a DB row. The charge must still resolve ok=true. +func TestResolveChargeSource_SaveCard_OrphanCleanedUp(t *testing.T) { + ctx := context.Background() + // A non-existent user: EnsureSquareCustomer is bypassed via the cache, and + // SaveCardForUser's INSERT fails on the users(id) FK — a clean injection of + // the DB-save failure without touching other test state. + userID := "c_orphan_00" + squareCustomerCache.Store(userID, "cus_orphan") + defer InvalidateSquareCustomerCache(userID) + + origClient := SquareClient + rec := &orphanCardClient{SquareClient: square.NewDevClient()} + SquareClient = rec + defer func() { SquareClient = origClient }() + + token := "cnon:test-orphan-cleanup" + sourceID, savedCardID, sqCustID, ok := resolveChargeSource(ctx, httptest.NewRecorder(), NewPaymentService(), userID, &token, nil, true, "") + require.True(t, ok, "a local save failure must NOT fail the charge") + require.Nil(t, savedCardID, "no local saved-card row must exist after the failed save") + require.Equal(t, "cus_orphan", sqCustID) + require.True(t, strings.HasPrefix(sourceID, "ccof:"), "source must still be the created card-on-file, got %q", sourceID) + + deletes := rec.deletedIDs() + require.Len(t, deletes, 1, "the just-created Square card must be disabled exactly once") + require.Equal(t, sourceID, deletes[0], "the disabled card must be the one this call just created") + + var rows int + require.NoError(t, db.Conn.QueryRow(ctx, `SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1`, userID).Scan(&rows)) + require.Zero(t, rows, "no orphaned saved-card row may exist") +} + +// TestResolveChargeSource_SaveCard_CleanupFailureStillCharges verifies the +// best-effort contract: when the DB save fails AND the Square disable also +// fails, the charge must still resolve ok=true (the orphan is only logged for +// manual cleanup, never allowed to fail the request). +func TestResolveChargeSource_SaveCard_CleanupFailureStillCharges(t *testing.T) { + ctx := context.Background() + userID := "c_orphan_01" + squareCustomerCache.Store(userID, "cus_orphan") + defer InvalidateSquareCustomerCache(userID) + + origClient := SquareClient + rec := &orphanCardClient{SquareClient: square.NewDevClient(), failDelete: true} + SquareClient = rec + defer func() { SquareClient = origClient }() + + token := "cnon:test-orphan-cleanup-fail" + sourceID, savedCardID, _, ok := resolveChargeSource(ctx, httptest.NewRecorder(), NewPaymentService(), userID, &token, nil, true, "") + require.True(t, ok, "a failed Square disable must never fail the charge") + require.Nil(t, savedCardID) + require.True(t, strings.HasPrefix(sourceID, "ccof:"), "source must be the created card-on-file, got %q", sourceID) + require.Equal(t, []string{sourceID}, rec.deletedIDs(), "the disable must be attempted even when it will fail") +} diff --git a/backend/handlers/payments/errors.go b/backend/handlers/payments/errors.go index ba6af95..0f02a21 100644 --- a/backend/handlers/payments/errors.go +++ b/backend/handlers/payments/errors.go @@ -50,5 +50,10 @@ func chargeFailureStatus(err error) int { if status >= 400 && status < 500 { return http.StatusPaymentRequired } + // Anything else (1xx/2xx/3xx — impossible in practice, but defensive) is + // AMBIGUOUS: the money state at Square is unknown, so the failure must be + // retryable. The default is deliberately 503, never 402 — a definitive + // decline classification on an ambiguous outcome would suppress the + // same-key retry that resumes the pending record. return http.StatusServiceUnavailable } diff --git a/backend/handlers/payments/errors_test.go b/backend/handlers/payments/errors_test.go index 2c471ad..214b040 100644 --- a/backend/handlers/payments/errors_test.go +++ b/backend/handlers/payments/errors_test.go @@ -118,6 +118,47 @@ func TestChargeFailureStatus_RetryableCarveOuts(t *testing.T) { } } +// TestChargeFailureStatus_DefaultAndEdgeStatuses pins the full status-space +// classification, including the ambiguous DEFAULT branch (1xx/2xx/3xx): the +// default MUST be 503 (ambiguous → retryable) — never 402, which labels a +// definitive decline and suppresses the same-key retry that resumes the pending +// record. 409 (Square IDEMPOTENCY_KEY_REUSED — a key reused with a different +// request body) is a definitive client error and must stay 402, not fall into +// the ambiguous bucket. +func TestChargeFailureStatus_DefaultAndEdgeStatuses(t *testing.T) { + tests := []struct { + name string + status int + want int + }{ + {"0 (plain/transport error) → 503", 0, http.StatusServiceUnavailable}, + {"1xx → 503 (ambiguous default)", http.StatusContinue, http.StatusServiceUnavailable}, + {"3xx → 503 (ambiguous default)", http.StatusMultipleChoices, http.StatusServiceUnavailable}, + {"400 → 402 (definitive)", http.StatusBadRequest, http.StatusPaymentRequired}, + {"401 → 402 (definitive)", http.StatusUnauthorized, http.StatusPaymentRequired}, + {"403 → 402 (definitive)", http.StatusForbidden, http.StatusPaymentRequired}, + {"408 → 503 (retryable)", http.StatusRequestTimeout, http.StatusServiceUnavailable}, + {"409 → 402 (idempotency-key conflict, definitive)", http.StatusConflict, http.StatusPaymentRequired}, + {"425 → 503 (retryable)", http.StatusTooEarly, http.StatusServiceUnavailable}, + {"429 → 503 (retryable)", http.StatusTooManyRequests, http.StatusServiceUnavailable}, + {"500 → 503", http.StatusInternalServerError, http.StatusServiceUnavailable}, + {"503 → 503", http.StatusServiceUnavailable, http.StatusServiceUnavailable}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var err error + if tt.status == 0 { + err = errors.New("mock: payment declined (simulated failure)") + } else { + err = structuredSquareAPIError(t, tt.status) + } + if got := chargeFailureStatus(err); got != tt.want { + t.Errorf("chargeFailureStatus(status=%d) = %d, want %d", tt.status, got, tt.want) + } + }) + } +} + // TestCreateBookingPayment_AmbiguousSquareFailure_Returns503 verifies the // charge-failure classification end to end: the dev mock's simulated failure // is a PLAIN error (no structured Square status), so the handler now returns diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index c5327ad..d5c6504 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -719,7 +719,7 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { paymentResult, err := SquareClient.CreatePayment(r.Context(), paymentReq) if err != nil { - log.Printf("Failed to process saved-card payment: %v", err) + log.Printf("Failed to process saved-card payment: %v (error_code=%q)", err, square.ErrorCode(err)) http.Error(w, "Payment failed", chargeFailureStatus(err)) return } @@ -1423,23 +1423,15 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { return } - // M1: generate a deterministic idempotency key server-side if the client - // doesn't provide one. The key is based on booking_id + payment_type + - // amount + card_id (or "new" for new cards), ensuring retries of the same - // logical charge use the same key while distinct charges get different keys. - if req.IdempotencyKey == "" { - cardPart := "new" - if req.CardID != nil && *req.CardID != "" { - cardPart = *req.CardID - } - req.IdempotencyKey = fmt.Sprintf("pay-%s-%s-%d-%s", bookingID, req.PaymentType, req.Amount, cardPart) - if len(req.IdempotencyKey) > 45 { - // Hash long keys to fit Square's 45-char limit - hash := sha256.Sum256([]byte(req.IdempotencyKey)) - req.IdempotencyKey = fmt.Sprintf("pay-%x", hash[:16]) - } - } - + // M1: when the client sends NO idempotency key, a DETERMINISTIC fallback + // is derived (booking_id + payment_type + amount + card_id) so a + // lost-response no-key retry reuses the same key instead of minting a + // second charge. The derivation deliberately runs INSIDE the transaction + // under the per-booking advisory lock (below): the helper advances a + // sequence past "spent" key slots, and that scan must not race a + // concurrent same-booking charge. See deriveBookingPaymentIdempotencyKey + // for how the fallback distinguishes "same live operation retried" (dedup) + // from "new operation that happens to have equal amount" (new charge). if req.PaymentType == "partial" { remainingCents, err := service.GetBookingRemainingBalanceCents(r.Context(), bookingID) if err != nil { @@ -1507,6 +1499,27 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { } }() + // M1: derive the deterministic no-client-key fallback INSIDE the + // transaction under the advisory lock so the spent-slot scan below races + // no concurrent charge (two equal partials must get distinct keys even + // when they arrive back-to-back). The scan itself does the idempotency + // re-validation: a completed row that has been refunded never blocks a new + // equal-amount charge, while an un-refunded completed row keeps its key so + // the dedup lookup below returns it (double-charge protection). + if req.IdempotencyKey == "" { + cardPart := "new" + if req.CardID != nil && *req.CardID != "" { + cardPart = *req.CardID + } + key, keyErr := deriveBookingPaymentIdempotencyKey(r.Context(), tx, bookingID, req.PaymentType, req.Amount, cardPart) + if keyErr != nil { + log.Printf("Failed to derive deterministic idempotency key for booking %s: %v", bookingID, keyErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + req.IdempotencyKey = key + } + var status string if err := tx.QueryRow(r.Context(), `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&status); err != nil { if errors.Is(err, pgx.ErrNoRows) { @@ -1604,7 +1617,23 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { reusePendingRecord := false switch { case err == nil && existingStatus.String == "completed": - // Idempotent dedup — return the already-completed payment. + // Idempotent dedup — return the already-completed payment. First + // RE-VALIDATE the matched row's state: a refunded completed payment's + // money is no longer live, so returning it as "success" would silently + // swallow a new equal-amount charge (the booking shows paid with no + // money collected). The no-client-key deterministic path already + // rotates the key past refunded rows (deriveBookingPaymentIdempotencyKey), + // so this guard primarily covers client-keyed retries and is + // defense-in-depth for the deterministic path. + if refunded, rErr := paymentHasLiveRefund(r.Context(), tx, existingID.String); rErr != nil { + log.Printf("Failed to re-validate dedup hit %s against refunds: %v", existingID.String, rErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } else if refunded { + log.Printf("Payment retry rejected: payment %s (key %q) was refunded — refusing to report a refunded payment as success", existingID.String, req.IdempotencyKey) + http.Error(w, "This payment has been refunded and can no longer be replayed", http.StatusConflict) + return + } if err := json.NewEncoder(w).Encode(PaymentResponse{ ID: existingID.String, BookingID: existingBookingID.String, @@ -1790,7 +1819,7 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { paymentResult, err := SquareClient.CreatePayment(r.Context(), paymentReq) if err != nil { - log.Printf("Failed to create payment: %v", err) + log.Printf("Failed to create payment: %v (error_code=%q)", err, square.ErrorCode(err)) http.Error(w, "Payment failed", chargeFailureStatus(err)) return } @@ -3661,7 +3690,7 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { paymentResult, err := SquareClient.CreatePayment(r.Context(), paymentReq) if err != nil { - log.Printf("Failed to create tip payment: %v", err) + log.Printf("Failed to create tip payment: %v (error_code=%q)", err, square.ErrorCode(err)) // Payment record intentionally left as 'pending' for manual retry. http.Error(w, "Payment failed", chargeFailureStatus(err)) return @@ -4004,6 +4033,90 @@ func uniqueChargeKey(prefix string) string { return prefix + rand.Text() } +// deriveBookingPaymentIdempotencyKey returns the deterministic fallback +// idempotency key for a no-client-key booking payment: +// "pay----", sha256-truncated when the +// verbatim form exceeds Square's 45-char limit (the hash stays deterministic, +// so a same-key retry still dedups). +// +// The key must distinguish "same live operation retried" (dedup) from "new +// operation that happens to have equal amount" (new charge). The candidate is +// the base key (seq 0) and then the base key with a "-" suffix (seq ≥ 1) +// until a slot without a COMPLETED payment is found; what makes a completed +// slot "spent" depends on the type: +// +// - 'partial' (repeatable type): a completed row ALWAYS advances the +// sequence — two genuine equal-amount partial payments are distinct +// operations and must diverge onto distinct keys (the dedup lookup would +// otherwise return the first as success and silently swallow the second). +// - non-repeatable types (deposit/full/balance): a completed row advances +// only when its money is no longer live (it has a completed/pending +// refund). A refunded payment must not be returned as "success" for a new +// equal-amount charge — the booking would show paid with no money +// collected (refund-then-repay). An UN-refunded completed row is the SAME +// live operation retried, so its key is reused and the dedup lookup +// returns it (paying the same 50% deposit twice on an un-refunded booking +// MUST still dedup — the double-charge protection). +// +// A PENDING row never occupies a slot (the scan only matches 'completed'), so +// a lost-response retry of an in-flight charge re-derives the same key and +// reuses the pending record. seq 0 is the historical un-sequenced key, so +// legacy deterministic-key rows are still matched. +// +// Must be called inside the transaction holding the per-booking advisory lock +// so the spent-slot scan races no concurrent charge (mirrors the tip flow's +// no-client-key fallback, which counts completed tips under the tip lock). +func deriveBookingPaymentIdempotencyKey(ctx context.Context, q db.Querier, bookingID, paymentType string, amount int64, cardPart string) (string, error) { + baseKey := fmt.Sprintf("pay-%s-%s-%d-%s", bookingID, paymentType, amount, cardPart) + for seq := 0; ; seq++ { + candidate := baseKey + if seq > 0 { + candidate = fmt.Sprintf("%s-%d", baseKey, seq) + } + if len(candidate) > 45 { + hash := sha256.Sum256([]byte(candidate)) + candidate = fmt.Sprintf("pay-%x", hash[:16]) + } + var completedID string + err := q.QueryRow(ctx, ` + SELECT id FROM payments + WHERE booking_id = $1 AND idempotency_key = $2 AND status = 'completed' + `, bookingID, candidate).Scan(&completedID) + if errors.Is(err, pgx.ErrNoRows) { + return candidate, nil + } + if err != nil { + return "", err + } + refunded, rErr := paymentHasLiveRefund(ctx, q, completedID) + if rErr != nil { + return "", rErr + } + if paymentType == "partial" || refunded { + continue + } + return candidate, nil + } +} + +// paymentHasLiveRefund reports whether the payment has a refund in a state +// meaning its money is no longer fully live: a completed refund (money +// returned) or a pending refund (money in flight). Failed refunds never moved +// money and are excluded. Used to re-validate a dedup hit — a refunded payment +// must never be returned as "success" for a new equal-amount charge. +func paymentHasLiveRefund(ctx context.Context, q db.Querier, paymentID string) (bool, error) { + var exists bool + err := q.QueryRow(ctx, ` + SELECT EXISTS( + SELECT 1 FROM refunds WHERE payment_id = $1 AND status IN ('completed', 'pending') + ) + `, paymentID).Scan(&exists) + if err != nil { + return false, err + } + return exists, nil +} + // randomHexSuffix returns n random bytes hex-encoded (2n hex chars) from // crypto/rand, used to disambiguate idempotency fallback keys that would // otherwise collide on deterministic inputs (e.g. the no-client-key refund diff --git a/backend/handlers/payments/payments_test.go b/backend/handlers/payments/payments_test.go index 48c0868..7500064 100644 --- a/backend/handlers/payments/payments_test.go +++ b/backend/handlers/payments/payments_test.go @@ -1583,6 +1583,162 @@ func TestCreateBookingPayment_MultiplePartialAllowed(t *testing.T) { } } +// TestBookingPayment_NoClientKey_RefundThenRepaySameAmount_CreatesNewCharge is +// the money-safety regression for the deterministic no-client-key fallback key +// (finding a-i): pay £50, refund it, then pay £50 again with NO client +// idempotency key. The second payment MUST be a NEW charge — the dedup lookup +// must not return the refunded (but still status='completed') payment as +// success, which would silently swallow the second payment while the booking +// shows paid with no money collected. The deterministic derivation rotates the +// key past the refunded payment's spent slot instead. +func TestBookingPayment_NoClientKey_RefundThenRepaySameAmount_CreatesNewCharge(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + // A past booking keeps the payment unsplit (single record) and 'partial' + // is the repeatable payment type, so the same-type duplicate guard does + // not interfere with the re-pay. A SECOND £50 service is linked so the + // booking total is £100: the first £50 payment then does not auto-complete + // the booking, and a £50 re-pay after the refund stays within the + // remaining balance (refunds re-open booking capacity). + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + secondServiceID, err := fixtures.CreateTestService(tx) + require.NoError(t, err) + _, err = tx.Exec(ctx, `INSERT INTO booking_services (booking_id, service_id) VALUES ($1, $2)`, bookingID, secondServiceID) + require.NoError(t, err) + userToken := jwt.GenerateUserToken(userID) + + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + adminToken := jwt.GenerateTestToken(adminID, "admin") + + handler := CreateBookingPayment + cardToken := "cnon:refund-repay-card" + // NO IdempotencyKey — exercises the deterministic booking+type+amount+card fallback. + req := CreateBookingPaymentRequest{ + Amount: 5000, + PaymentType: "partial", + NewCardToken: &cardToken, + } + + // 1. Pay £50 — completes. + w1 := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w1.Code, "first payment: %s", w1.Body.String()) + var resp1 PaymentResponse + require.NoError(t, parsePaymentResponseBody(w1, &resp1)) + + // 2. Refund the full £50 via the admin refund handler (the dev mock + // completes the Square refund synchronously). + refundReq := RefundRequest{Amount: 5000, Reason: "customer request", IdempotencyKey: "refund-repay-" + bookingID} + wRefund := makePaymentRequest(RefundPayment, "POST", "/api/admin/payments/"+resp1.ID+"/refund", refundReq, adminToken, ctx) + require.Equal(t, http.StatusOK, wRefund.Code, "refund: %s", wRefund.Body.String()) + + // 3. Pay £50 again — same no-key derivation. Must be a NEW charge, not a + // dedup to the refunded first payment. + w2 := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w2.Code, "repay: %s", w2.Body.String()) + var resp2 PaymentResponse + require.NoError(t, parsePaymentResponseBody(w2, &resp2)) + require.NotEqual(t, resp1.ID, resp2.ID, "refund-then-repay must create a NEW payment, not return the refunded payment as success") + + // Exactly two completed real payments with distinct idempotency keys and + // distinct Square charges. + var payCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND status = 'completed' AND payment_method NOT IN ('discount', 'on_the_house')`, bookingID).Scan(&payCount)) + require.Equal(t, 2, payCount, "refund-then-repay must record two distinct payments") + + var keys, sqIDs []string + rows, err := tx.Query(ctx, `SELECT idempotency_key, square_payment_id FROM payments WHERE booking_id = $1 AND status = 'completed' ORDER BY created_at ASC`, bookingID) + require.NoError(t, err) + for rows.Next() { + var k, s string + require.NoError(t, rows.Scan(&k, &s)) + keys = append(keys, k) + sqIDs = append(sqIDs, s) + } + rows.Close() + require.Equal(t, 2, len(keys)) + require.NotEqual(t, keys[0], keys[1], "the two payments must use distinct idempotency keys") + require.NotEqual(t, sqIDs[0], sqIDs[1], "the second payment must be a new Square charge, not a dedup of the refunded one") +} + +// TestBookingPayment_NoClientKey_TwoEqualPartials_DoNotCollapse is the +// money-safety regression for the deterministic no-client-key fallback key +// (finding a-ii): two genuine equal-amount partial payments on the same card +// must both be recorded. The deterministic key derivation includes a sequence +// that advances past the first completed equal partial, so the second derives +// a DISTINCT key instead of silently collapsing onto the first. +func TestBookingPayment_NoClientKey_TwoEqualPartials_DoNotCollapse(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + handler := CreateBookingPayment + cardToken := "cnon:equal-partial-card" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "partial", + NewCardToken: &cardToken, + } + + for i := 0; i < 2; i++ { + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "partial %d: %s", i, w.Body.String()) + } + + var keys []string + rows, err := tx.Query(ctx, `SELECT idempotency_key FROM payments WHERE booking_id = $1 AND status = 'completed' AND payment_method NOT IN ('discount', 'on_the_house') ORDER BY created_at ASC`, bookingID) + require.NoError(t, err) + for rows.Next() { + var k string + require.NoError(t, rows.Scan(&k)) + keys = append(keys, k) + } + rows.Close() + require.Equal(t, 2, len(keys), "two genuine equal-amount partials must both be recorded") + require.NotEqual(t, keys[0], keys[1], "equal-amount partials must not collapse onto one idempotency key") +} + +// TestBookingPayment_NoClientKey_SameDepositTwice_StillDedups pins the +// double-charge protection that MUST survive the sequence fix: paying the same +// 50% deposit twice on an UN-refunded booking with no client key must dedup to +// the existing completed payment — never a second Square charge. The +// deterministic derivation advances its sequence only past refunded/partial +// slots; an un-refunded 'deposit' slot keeps its key so the dedup lookup +// returns the original payment. +func TestBookingPayment_NoClientKey_SameDepositTwice_StillDedups(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + handler := CreateBookingPayment + cardToken := "cnon:same-deposit-card" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + } + + w1 := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w1.Code, "first deposit: %s", w1.Body.String()) + var resp1 PaymentResponse + require.NoError(t, parsePaymentResponseBody(w1, &resp1)) + + w2 := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w2.Code, "second deposit: %s", w2.Body.String()) + var resp2 PaymentResponse + require.NoError(t, parsePaymentResponseBody(w2, &resp2)) + require.Equal(t, resp1.ID, resp2.ID, "the same 50%% deposit charged twice on an un-refunded booking must DEDUP, not double-charge") + + var count int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND status = 'completed' AND payment_method NOT IN ('discount', 'on_the_house')`, bookingID).Scan(&count)) + require.Equal(t, 1, count, "only ONE deposit payment may be recorded") +} + // ============================================================ // User Booking Payment Tests — deposit, full, partial, balance // ============================================================ @@ -1809,10 +1965,14 @@ func TestBookingPayment_FullPayment_SplitsIntoDepositAndBalance(t *testing.T) { t.Fatalf("expected status 200, got %d. body: %s", w.Code, w.Body.String()) } - // Should be exactly 2 payment records. + // Should be exactly 2 payment records. Order by payment_type (the enum + // defines 'deposit' before 'balance', so the deposit row is first) plus + // created_at as a tiebreaker — ORDER BY amount was nondeterministic here + // because both split amounts are equal (£25.00), which flaked ~1-in-10 + // runs asserting records[0] is the deposit. rows, err := tx.Query(ctx, `SELECT payment_type, amount, square_payment_id - FROM payments WHERE booking_id = $1 ORDER BY amount DESC`, bookingID) + FROM payments WHERE booking_id = $1 ORDER BY payment_type, created_at ASC`, bookingID) if err != nil { t.Fatalf("failed to query payments: %v", err) } @@ -2252,6 +2412,77 @@ func TestGetBookingPaymentInfo_ExcludesDiscountPayments(t *testing.T) { } } +// TestBuildSplitRecords_DepositCarve_PenceExact pins the 50% deposit carve to +// exact pence outcomes (money-safety audit): the carve rounds ONCE at the +// pence boundary and the parts always partition the charged amount exactly, so +// the recorded pence can never exceed what Square actually charged. The +// tip-percentage math (tip% × subtotal) lives in the frontend — the backend +// receives the charged total — so this test pins the backend's deposit carve, +// the only percentage-derived math in the charge-splitting path. +func TestBuildSplitRecords_DepositCarve_PenceExact(t *testing.T) { + cases := []struct { + name string + total float64 // booking total, pounds + paid float64 // already paid, pounds + chargedPence int64 // the charge, int64 pence as Square reports it + wantDeposit int64 // expected deposit carve, pence + wantBalance int64 // expected balance portion, pence + wantTip int64 // expected tip portion, pence + }{ + {"£50 charge on £50 booking → 25.00 + 25.00", 50, 0, 5000, 2500, 2500, 0}, + {"£60 charge on £50 booking → deposit + balance + tip overflow", 50, 0, 6000, 2500, 2500, 1000}, + {"£25 charge on £100 booking (under the 50% cap) → all deposit", 100, 0, 2500, 2500, 0, 0}, + {"£60 charge on £100 booking → 50.00 deposit + 10.00 balance", 100, 0, 6000, 5000, 1000, 0}, + {"£100 charge on £100 booking → 50.00 + 50.00", 100, 0, 10000, 5000, 5000, 0}, + {"£50 charge on £100 booking with £30 already paid → deposit fills to cap", 100, 30, 5000, 2000, 3000, 0}, + {"£12.34 charge on £25.00 booking → all deposit", 25, 0, 1234, 1234, 0, 0}, + {"£25 charge on £25.50 booking → 12.75 deposit + 12.25 balance (half-penny carve)", 25.50, 0, 2500, 1275, 1225, 0}, + {"£45.67 charge on £50 booking → 25.00 + 20.67", 50, 0, 4567, 2500, 2067, 0}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + chargedPounds := float64(tc.chargedPence) / 100.0 + record := makeTestRecord("pence-booking", "full", chargedPounds) + info := &BookingPaymentInfo{ + StartTime: clock.Now().Add(48 * time.Hour), + TotalAmount: tc.total, + TotalPaid: tc.paid, + } + records := buildSplitRecords(record, "full", info, chargedPounds) + + var depositPence, balancePence, tipPence, partitionPence int64 + for _, r := range records { + pence := int64(math.Round(r.Amount * 100)) + partitionPence += pence + switch r.PaymentType { + case "deposit": + depositPence = pence + case "tip": + tipPence = pence + default: + // The booking-portion remainder after the deposit carve. + // Its TYPE is dynamic ('balance' when the payment reaches + // the booking total, 'partial' when it does not); the + // pence are what this table pins. + balancePence = pence + } + } + if depositPence != tc.wantDeposit { + t.Errorf("deposit carve = %d pence, want %d", depositPence, tc.wantDeposit) + } + if balancePence != tc.wantBalance { + t.Errorf("balance portion = %d pence, want %d", balancePence, tc.wantBalance) + } + if tipPence != tc.wantTip { + t.Errorf("tip portion = %d pence, want %d", tipPence, tc.wantTip) + } + if partitionPence != tc.chargedPence { + t.Errorf("split records partition to %d pence, want the charged %d pence", partitionPence, tc.chargedPence) + } + }) + } +} + // --------------------------------------------------------------------------- // Handler-level atomicity — verify the full handler succeeds with split. // --------------------------------------------------------------------------- diff --git a/backend/handlers/payments/refunds.go b/backend/handlers/payments/refunds.go index 7f98991..2c60037 100644 --- a/backend/handlers/payments/refunds.go +++ b/backend/handlers/payments/refunds.go @@ -102,10 +102,11 @@ func CalculateRefundForCancellation( // lockCancellationPayments serializes a cancellation refund against the manual // RefundPayment handler and the sweep. Both hold -// `pg_advisory_lock(hashtext('crussell:refund:' || payment_id))` (session-level) -// on the payment ids they touch; a cancellation that computes residuals -// without the same locks can over-refund against a manual refund in flight (the -// manual guard read precedes the cancellation's commit). Locks are acquired in +// `pg_advisory_xact_lock(hashtext('crussell:refund:' || payment_id))` +// (transaction-level, acquired via acquireAdvisoryXactLockBlocking) on the +// payment ids they touch; a cancellation that computes residuals without the +// same locks can over-refund against a manual refund in flight (the manual +// guard read precedes the cancellation's commit). Locks are acquired in // ascending payment_id order (matching processChargeGroup) to avoid deadlocks. // EVERY payment row the cancellation may refund is locked — giftcard and cash // rows included, not just card methods — so two concurrent refunds of the same diff --git a/backend/handlers/payments/sweep.go b/backend/handlers/payments/sweep.go index 7bccca5..36bac29 100644 --- a/backend/handlers/payments/sweep.go +++ b/backend/handlers/payments/sweep.go @@ -518,8 +518,12 @@ func clawbackTillSaleFunding(ctx context.Context, r staleRow) bool { // charge and its replay, the replayed wire body differs and a RETAINED key // returns IDEMPOTENCY_KEY_REUSED — stranding every retained-key row pending // (safe) until the 24h blind-fail. This is a single-location deployment; the -// contract is enforced by ops (same env for the sweeper and the API), NOT by a -// runtime equality check — deliberately comment-only. +// contract is enforced by ops (same env for the sweeper and the API). The +// environment and location are read through square.SquareEnvironment() and +// square.SquareLocationID() — the SAME code path the charge-time HTTP client +// uses (newHTTPClient resolves its base URL and location from those helpers) — +// so the sweep can never drift to a second, independent env read; the +// reconcile below logs the resolved values at each replay as a tripwire. func reconcileStalePaymentByKey(ctx context.Context, table string, r staleRow) (staleReconcileResult, string) { snapshot := r.SquareRequestSnapshot // fallbackBody is true when the row has NO stored square_request_snapshot @@ -552,9 +556,14 @@ func reconcileStalePaymentByKey(ctx context.Context, table string, r staleRow) ( snapshot = fallback } // The replay repeats the stored request snapshot verbatim so Square's - // idempotency dedup returns the original payment for a retained key — which - // requires the sweep to share SQUARE_ENVIRONMENT/SQUARE_LOCATION_ID with the - // charge process (see the ENV CONTRACT above). + // idempotency dedup returns the original payment for a retained key. The + // sweep resolves its Square environment and location through the same + // helpers the charge-time HTTP client uses (square.SquareEnvironment / + // square.SquareLocationID), so the replay and the charge can never read + // two different env sources (see the ENV CONTRACT above). The values are + // logged on each replay as a tripwire for env/location drift between the + // sweeper and the API. + log.Printf("[SWEEP] replay-by-key reconcile for %s row %s: SQUARE_ENVIRONMENT=%q SQUARE_LOCATION_ID=%q (must match the charge-time env/location for identical-body idempotency)", table, r.ID, square.SquareEnvironment(), square.SquareLocationID()) pr, err := SquareClient.ReplayPaymentByKey(ctx, snapshot) if err != nil { if errors.Is(err, square.ErrReplayKeyNotRetained) { diff --git a/backend/handlers/user/twofa.go b/backend/handlers/user/twofa.go index 16490d7..66cb323 100644 --- a/backend/handlers/user/twofa.go +++ b/backend/handlers/user/twofa.go @@ -1,17 +1,21 @@ package user import ( + "crypto/hmac" "crypto/rand" "crypto/sha256" "crypto/subtle" "database/sql" "encoding/hex" "encoding/json" + "errors" "fmt" "log" "math/big" "net/http" + "os" "sync" + "sync/atomic" "time" "crussell/clock" @@ -40,16 +44,71 @@ func generateTwoFACode() (string, error) { return fmt.Sprintf("%06d", n.Int64()), nil } -// hashTwoFACode returns the SHA-256 hex digest of a verification code. The DB -// stores only the digest; the plaintext code is delivered by logging it with a -// [2FA] prefix (see deliverTwoFACode). The digest is unsalted SHA-256 — -// peppering it via HMAC-SHA256 with a server-side 2FA_PEPPER secret is a future -// hardening step once such a secret is provisioned. +// twoFAPepperEnv is the environment variable carrying the server-side pepper +// that keys the HMAC of stored 2FA codes (documented in .env.example). When it +// is absent the code falls back to the legacy plain SHA-256 digest with a +// one-time warning — see hashTwoFACode. +const twoFAPepperEnv = "TWO_FACTOR_PEPPER" + +// twoFAPepperWarnOnce guards the one-time warning when TWO_FACTOR_PEPPER is +// unset, so a misconfigured deployment is loudly flagged once rather than on +// every code operation. +var twoFAPepperWarnOnce sync.Once + +// twoFAPepper returns the configured HMAC pepper, or "" when unset. Read per +// call (the rest of the backend reads env vars per call too) so a value +// provisioned at runtime is picked up; only the warning is gated on sync.Once. +func twoFAPepper() string { + pepper := os.Getenv(twoFAPepperEnv) + if pepper == "" { + twoFAPepperWarnOnce.Do(func() { + log.Printf("WARNING: TWO_FACTOR_PEPPER unset — 2FA codes hashed without an HMAC pepper (falling back to unsalted SHA-256); set TWO_FACTOR_PEPPER in production so a leaked digest cannot be brute-forced offline") + }) + } + return pepper +} + +// hashTwoFACode returns the hex digest of a verification code as stored in the +// DB. With TWO_FACTOR_PEPPER set the digest is HMAC-SHA256 keyed by the pepper, +// so a leaked digest cannot be brute-forced offline (the key stays server-side). +// When the pepper is unset it falls back to the legacy unsalted SHA-256 digest +// and logs a one-time warning. The plaintext code is never stored — only +// delivered via the [2FA] log line (see deliverTwoFACode). func hashTwoFACode(code string) string { + if pepper := twoFAPepper(); pepper != "" { + mac := hmac.New(sha256.New, []byte(pepper)) + mac.Write([]byte(code)) + return hex.EncodeToString(mac.Sum(nil)) + } sum := sha256.Sum256([]byte(code)) return hex.EncodeToString(sum[:]) } +// legacyHashTwoFACode returns the pre-pepper plain SHA-256 digest, used to +// verify rows written before TWO_FACTOR_PEPPER was provisioned during the +// migration window (see verifyTwoFACodeHash). +func legacyHashTwoFACode(code string) string { + sum := sha256.Sum256([]byte(code)) + return hex.EncodeToString(sum[:]) +} + +// verifyTwoFACodeHash reports whether reqCode matches a stored pending-code +// digest, always in constant time (subtle.ConstantTimeCompare). The first +// comparison uses the current pepper'd digest; when that fails the stored hash +// may be a legacy pre-pepper plain SHA-256 (rows written before TWO_FACTOR_PEPPER +// was provisioned), so the legacy digest is tried too. When a legacy row +// matches, legacy is true and the caller should re-hash with the pepper on the +// next successful verify, retiring the plain digest. +func verifyTwoFACodeHash(reqCode, storedHash string) (match, legacy bool) { + if subtle.ConstantTimeCompare([]byte(hashTwoFACode(reqCode)), []byte(storedHash)) == 1 { + return true, false + } + if subtle.ConstantTimeCompare([]byte(legacyHashTwoFACode(reqCode)), []byte(storedHash)) == 1 { + return true, true + } + return false, false +} + // twoFAMaxAttempts is the number of consecutive failed verify attempts allowed // before the pending code is invalidated and a new one must be requested. const twoFAMaxAttempts = 5 @@ -62,15 +121,29 @@ const twoFAAttemptWindow = 10 * time.Minute // user IDs cannot grow it without bound. Counters are purely in-memory (the DB // schema is locked — there is no attempt column), so they reset on process // restart; the 10-minute pending-code expiry bounds the practical impact. -const twoFAMaxTrackedAttempts = 10_000 +// Declared as a var so the eviction policy is unit-testable at a small cap. +var twoFAMaxTrackedAttempts = 10_000 // twoFAAttemptState tracks consecutive failed verify attempts for one user. The // per-user mutex serializes the whole verify critical section so concurrent -// attempts from the same user cannot race the limit check. +// attempts from the same user cannot race the limit check. count is atomic so +// the map eviction path can read it without taking the per-user mutex (lock +// ordering forbids mapMu→st.mu: checkTwoFACode holds st.mu then takes mapMu). +// lastMintAt is the disable-flow mint cooldown stamp (see twoFAMintCooldown). type twoFAAttemptState struct { - mu sync.Mutex - count int - lastAt time.Time + mu sync.Mutex + count atomic.Int32 + lastAt time.Time + lastMintAt time.Time +} + +// lockedOut reports whether the state is inside its lockout window: the attempt +// counter has reached the cap and the window has not yet elapsed. Such a record +// is the rate limit's source of truth for its user and must never be evicted +// while in-window — evicting it would silently reset the counter and grant a +// fresh guessing budget. +func (st *twoFAAttemptState) lockedOut(now time.Time) bool { + return st.count.Load() >= twoFAMaxAttempts && now.Sub(st.lastAt) <= twoFAAttemptWindow } var ( @@ -79,8 +152,13 @@ var ( ) // twoFAAttemptStateFor returns the per-user attempt state, creating it if -// needed. The map is bounded: stale entries are evicted opportunistically and, -// when at capacity, the least-recently-active entry is dropped. +// needed. The map is bounded: stale (window-expired) entries are evicted +// opportunistically and, when at capacity, the least-recently-active +// non-locked-out entry is dropped. A record still inside its lockout window is +// NEVER evicted — evicting it would reset the victim's attempt counter and +// bypass the rate limit under a hostile flood of new keys. When the map is +// full of in-window locked-out records (a pathological flood), a transient, +// untracked state is returned instead of growing the map past the cap. func twoFAAttemptStateFor(userID string) *twoFAAttemptState { twoFAAttemptMapMu.Lock() defer twoFAAttemptMapMu.Unlock() @@ -91,9 +169,15 @@ func twoFAAttemptStateFor(userID string) *twoFAAttemptState { var oldestAt time.Time for id, st := range twoFAAttemptMap { if now.Sub(st.lastAt) > twoFAAttemptWindow { + // Idle/expired — its counter has already lapsed; safe to evict. delete(twoFAAttemptMap, id) continue } + if st.lockedOut(now) { + // Inside its lockout window — the rate limit's source of truth + // for this user. Never evict (finding-e fix). + continue + } if oldestID == "" || st.lastAt.Before(oldestAt) { oldestID, oldestAt = id, st.lastAt } @@ -101,6 +185,13 @@ func twoFAAttemptStateFor(userID string) *twoFAAttemptState { if len(twoFAAttemptMap) >= twoFAMaxTrackedAttempts && oldestID != "" { delete(twoFAAttemptMap, oldestID) } + if len(twoFAAttemptMap) >= twoFAMaxTrackedAttempts { + // Every entry is a locked-out in-window record. Do not evict one + // (that would reset its rate limit) and do not grow past the cap: + // return a transient, untracked state so THIS request still + // proceeds under a fresh budget. + return &twoFAAttemptState{lastAt: now} + } } st := twoFAAttemptMap[userID] @@ -111,12 +202,21 @@ func twoFAAttemptStateFor(userID string) *twoFAAttemptState { return st } -// twoFAResetAttempts clears a user's attempt counter. Called on successful -// verify and when a fresh code is generated via setup. +// twoFAResetAttempts resets a user's attempt counter in place (count only) +// WITHOUT deleting the entry, preserving lastMintAt so the disable-flow mint +// cooldown survives a fresh-code delivery. Called on successful verify and when +// a fresh code is generated via setup or disable. lastAt is deliberately not +// touched here: it is re-stamped by checkTwoFACode on real activity, and +// writing it under mapMu would race with checkTwoFACode's st.mu-guarded write +// (the setup path holds no st.mu). The lock ordering is st.mu→mapMu at call +// sites, never the reverse (twoFAAttemptStateFor takes mapMu only and never +// takes st.mu). func twoFAResetAttempts(userID string) { twoFAAttemptMapMu.Lock() - delete(twoFAAttemptMap, userID) - twoFAAttemptMapMu.Unlock() + defer twoFAAttemptMapMu.Unlock() + if st := twoFAAttemptMap[userID]; st != nil { + st.count.Store(0) + } } // deliverTwoFACode generates a fresh verification code, persists only its @@ -295,10 +395,10 @@ const ( // here and still reported as a lockout. func checkTwoFACode(r *http.Request, userID string, st *twoFAAttemptState, reqCode string) (twoFACodeCheckResult, error) { if now := clock.Now(); now.Sub(st.lastAt) > twoFAAttemptWindow { - st.count = 0 + st.count.Store(0) st.lastAt = now } - if st.count >= twoFAMaxAttempts { + if st.count.Load() >= twoFAMaxAttempts { return twoFACodeLockedOut, nil } @@ -316,11 +416,14 @@ func checkTwoFACode(r *http.Request, userID string, st *twoFAAttemptState, reqCo return twoFACodeMissingOrExpired, nil } // Constant-time compare (subtle) so a wrong code's match position cannot be - // inferred from response timing. Both digests are fixed-length hex. - if subtle.ConstantTimeCompare([]byte(hashTwoFACode(reqCode)), []byte(pendingHash.String)) != 1 { - st.count++ + // inferred from response timing. Both digests are fixed-length hex. Legacy + // pre-pepper rows (plain SHA-256, hashed before TWO_FACTOR_PEPPER existed) + // still verify during the transition window. + match, legacy := verifyTwoFACodeHash(reqCode, pendingHash.String) + if !match { + st.count.Add(1) st.lastAt = clock.Now() - if st.count >= twoFAMaxAttempts { + if st.count.Load() >= twoFAMaxAttempts { // Lockout reached: destroy the pending code so a stolen digest // cannot be replayed against a fresh guessing loop. if _, err := db.Conn.Exec(r.Context(), ` @@ -336,9 +439,22 @@ func checkTwoFACode(r *http.Request, userID string, st *twoFAAttemptState, reqCo return twoFACodeIncorrect, nil } - // Success: clear the attempt counter before the caller performs its action. - st.count = 0 + // Success: a legacy (pre-pepper) hash that verified is re-hashed with the + // pepper so the plain digest is retired on the next successful verify. + if legacy { + if _, err := db.Conn.Exec(r.Context(), ` + UPDATE users + SET two_factor_pending_code_hash = $2 + WHERE id = $1 + `, userID, hashTwoFACode(reqCode)); err != nil { + log.Printf("failed to upgrade legacy 2FA pending code hash for user %s: %v", userID, err) + } + } + // Success: clear the attempt counter (and any disable-flow mint cooldown) + // before the caller performs its action. + st.count.Store(0) st.lastAt = clock.Now() + st.lastMintAt = time.Time{} twoFAResetAttempts(userID) return twoFACodeOK, nil } @@ -425,6 +541,19 @@ func writeTwoFAEnabled(w http.ResponseWriter) { } } +// twoFAMintCooldown bounds how often a fresh 2FA code may be minted for one +// user during the disable flow. Without it, a password-only attacker could loop +// disable → fresh code (which resets the 5-attempt counter) → 5 wrong guesses → +// fresh code again, for ~100 guesses/min unbounded. The cooldown caps guessing +// at 5 per window (~5/min) while still letting a legitimate code-lost user +// recover after a short wait. +const twoFAMintCooldown = 1 * time.Minute + +// errTwoFAMintThrottled is returned by ensurePendingTwoFACode when the user's +// last disable-flow mint is inside twoFAMintCooldown, so the caller returns 429 +// instead of minting another fresh code. +var errTwoFAMintThrottled = errors.New("2FA code mint throttled") + type TwoFADisableRequest struct { Code string `json:"code"` } @@ -437,9 +566,10 @@ type TwoFADisableRequest struct { // attacker must not be able to disable the protection. A fresh code is generated // and delivered via the [2FA] log channel when no valid pending code exists, and // the submitted code is checked under the shared 5-attempt lockout (wrong code → -// 400, lockout → 429); only a correct code clears the flag. In unenforced (dev) -// environments the loose behavior is kept: no code required, so local dev is not -// blocked. +// 400, lockout → 429); only a correct code clears the flag. Fresh-code mints are +// throttled per-user (twoFAMintCooldown) so the loop above cannot reset the +// lockout faster than once per cooldown. In unenforced (dev) environments the +// loose behavior is kept: no code required, so local dev is not blocked. func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { userID, ok := mw.GetUserID(r.Context()) if !ok { @@ -471,24 +601,20 @@ func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { defer st.mu.Unlock() // Reuse a valid pending code when one exists; otherwise generate + deliver - // a fresh one via the same [2FA] log channel as setup. - freshDelivered, err := ensurePendingTwoFACode(r, userID) - if err != nil { + // a fresh one via the same [2FA] log channel as setup. A fresh code gets its + // own independent 5-attempt budget (the mint resets the counter), so the + // per-user mint cooldown is what stops the unlimited-guess loop — an + // attacker can mint at most one fresh code per twoFAMintCooldown. + if err := ensurePendingTwoFACode(r, userID, st); err != nil { + if errors.Is(err, errTwoFAMintThrottled) { + http.Error(w, "Too many attempts. Wait before requesting a new code.", http.StatusTooManyRequests) + return + } log.Printf("failed to prepare 2FA code for disable for user %s: %v", userID, err) http.Error(w, "server error", http.StatusInternalServerError) return } - // The fresh delivery reset the shared attempt map entry (twoFAResetAttempts - // deletes it), but the held st still carries the pre-delivery count. Reset - // it only when a fresh code was actually delivered, so a locked-out user can - // use the code just minted in THIS request — while the reuse path keeps - // accumulating wrong attempts toward the 5-attempt lockout. - if freshDelivered { - st.count = 0 - st.lastAt = clock.Now() - } - result, err := checkTwoFACode(r, userID, st, req.Code) if err != nil { log.Printf("failed to check 2FA pending code for user %s: %v", userID, err) @@ -517,13 +643,19 @@ func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { } // ensurePendingTwoFACode guarantees the user has a valid (unexpired) pending -// code to verify against, generating + delivering a fresh one via the same [2FA] -// log channel as setup when the stored code is missing or expired. The boolean -// reports whether a fresh code was delivered (false = an existing valid code -// was reused), which the caller uses to decide whether to reset the held -// attempt counter. A fresh code also resets any prior lockout, matching setup's -// recovery behavior. The caller must hold the user's attempt-state mutex. -func ensurePendingTwoFACode(r *http.Request, userID string) (bool, error) { +// code to verify against, generating + delivering a fresh one via the same +// [2FA] log channel as setup when the stored code is missing or expired. The +// caller must hold the user's attempt-state mutex. +// +// A fresh code gets its own independent 5-attempt budget (deliverTwoFACode +// resets the counter via twoFAResetAttempts), so the per-user mint cooldown is +// what prevents a password-only attacker from looping mint → burn 5 guesses → +// mint forever: only one fresh code per twoFAMintCooldown per user. A locked-out +// user can still use the code minted in THIS request; a user who exhausts it +// must wait out the cooldown for the next mint — the documented disable-flow +// residual. A failed delivery does not start the cooldown (the stamp is written +// only after the UPDATE persisted). +func ensurePendingTwoFACode(r *http.Request, userID string, st *twoFAAttemptState) error { var pendingHash sql.NullString var pendingExpires sql.NullTime err := db.Conn.QueryRow(r.Context(), ` @@ -532,13 +664,20 @@ func ensurePendingTwoFACode(r *http.Request, userID string) (bool, error) { WHERE id = $1 `, userID).Scan(&pendingHash, &pendingExpires) if err != nil { - return false, err + return err } if pendingHash.Valid && pendingExpires.Valid && pendingExpires.Time.After(clock.Now()) { - return false, nil + return nil } - _, err = deliverTwoFACode(r, userID, "", "disable 2FA") - return true, err + now := clock.Now() + if !st.lastMintAt.IsZero() && now.Sub(st.lastMintAt) < twoFAMintCooldown { + return errTwoFAMintThrottled + } + if _, err := deliverTwoFACode(r, userID, "", "disable 2FA"); err != nil { + return err + } + st.lastMintAt = now + return nil } // disableTwoFA clears two_factor_enabled and the method + pending code fields. diff --git a/backend/handlers/user/twofa_test.go b/backend/handlers/user/twofa_test.go index 287be61..2608ced 100644 --- a/backend/handlers/user/twofa_test.go +++ b/backend/handlers/user/twofa_test.go @@ -12,16 +12,20 @@ package user import ( "bytes" "context" + "crypto/hmac" "crypto/sha256" "database/sql" "encoding/hex" "encoding/json" + "fmt" "log" "net/http" "net/http/httptest" "os" "regexp" + "strings" "testing" + "time" "crussell/clock" "crussell/db" @@ -136,6 +140,9 @@ func TestTwoFASetup_InvalidMethod(t *testing.T) { func TestTwoFASetup_Valid_StoresHash(t *testing.T) { twofaEnvUnenforced(t) + // Pin the pepper off so the stored hash assertion below is deterministic + // regardless of the ambient test environment. + t.Setenv("TWO_FACTOR_PEPPER", "") ctx, tx := testutils.SetupTestTx(t) userID, err := fixtures.CreateTestUser(tx) require.NoError(t, err) @@ -151,7 +158,8 @@ func TestTwoFASetup_Valid_StoresHash(t *testing.T) { require.Equal(t, "Code sent", resp.Message) require.Len(t, resp.Code, 6, "unenforced env must return the dev-convenience code") - // The DB must hold the SHA-256 digest of exactly the returned code. + // The DB must hold the digest of exactly the returned code. With the pepper + // pinned off, that is the plain SHA-256 (the legacy fallback). var pendingHash, method sql.NullString var expires sql.NullTime require.NoError(t, tx.QueryRow(ctx, ` @@ -606,3 +614,292 @@ func TestTwoFAVerify_WrongCodesAnyLengthRejected(t *testing.T) { w := performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "123456"}, userID2) require.Equal(t, http.StatusOK, w.Code, w.Body.String()) } + +// ============================================================================= +// Pepper hashing (finding c) +// ============================================================================= + +// TestTwoFAPepper_HashUsesHMAC verifies that with TWO_FACTOR_PEPPER set the +// stored digest is HMAC-SHA256 keyed by the pepper, NOT the legacy unsalted +// SHA-256 — so a leaked digest cannot be brute-forced offline. +func TestTwoFAPepper_HashUsesHMAC(t *testing.T) { + t.Setenv("TWO_FACTOR_PEPPER", "test-pepper-secret") + const code = "123456" + got := hashTwoFACode(code) + + mac := hmac.New(sha256.New, []byte("test-pepper-secret")) + mac.Write([]byte(code)) + want := hex.EncodeToString(mac.Sum(nil)) + require.Equal(t, want, got, "stored hash must be HMAC-SHA256 keyed by TWO_FACTOR_PEPPER") + require.NotEqual(t, legacyHashTwoFACode(code), got, "pepper'd hash must differ from the legacy plain SHA-256") +} + +// TestTwoFAPepper_UnsetFallback_PlainSHA256 verifies the graceful no-pepper +// fallback keeps the legacy unsalted SHA-256 digest when TWO_FACTOR_PEPPER is +// unset. +func TestTwoFAPepper_UnsetFallback_PlainSHA256(t *testing.T) { + t.Setenv("TWO_FACTOR_PEPPER", "") + const code = "654321" + got := hashTwoFACode(code) + sum := sha256.Sum256([]byte(code)) + require.Equal(t, hex.EncodeToString(sum[:]), got, "unset pepper must fall back to legacy plain SHA-256") + require.Equal(t, legacyHashTwoFACode(code), got) +} + +// TestTwoFAPepper_LegacyHashDetected verifies verifyTwoFACodeHash accepts both +// the pepper'd and the legacy plain forms (the transition window) and flags +// legacy rows for upgrade. +func TestTwoFAPepper_LegacyHashDetected(t *testing.T) { + t.Setenv("TWO_FACTOR_PEPPER", "test-pepper-secret") + const code = "123456" + match, legacy := verifyTwoFACodeHash(code, hashTwoFACode(code)) + require.True(t, match) + require.False(t, legacy, "pepper'd stored hash must not be flagged for upgrade") + match, legacy = verifyTwoFACodeHash(code, legacyHashTwoFACode(code)) + require.True(t, match) + require.True(t, legacy, "legacy stored hash must verify and flag the upgrade") + match, legacy = verifyTwoFACodeHash("999999", legacyHashTwoFACode(code)) + require.False(t, match) + require.False(t, legacy) +} + +// TestTwoFAPepper_LegacyHashUpgrade_OnSuccessfulVerify verifies that a legacy +// pre-pepper row still verifies during the migration window AND that the stored +// hash is upgraded to the pepper'd form on the next successful verify (the +// plain digest is retired). +func TestTwoFAPepper_LegacyHashUpgrade_OnSuccessfulVerify(t *testing.T) { + t.Setenv("TWO_FACTOR_PEPPER", "test-pepper-secret") + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + // Seed a legacy row exactly as the pre-pepper code wrote it: plain SHA-256. + _, err = tx.Exec(ctx, `UPDATE users + SET two_factor_method = 'email', + two_factor_pending_code_hash = $2, + two_factor_pending_code_expires = $3 + WHERE id = $1`, userID, legacyHashTwoFACode("123456"), clock.Now().Add(twoFAPendingExpiry)) + require.NoError(t, err) + + // checkTwoFACode (the shared verify path) must accept the legacy hash. + st := &twoFAAttemptState{lastAt: clock.Now()} + req := httptest.NewRequest(http.MethodPost, "/api/user/2fa/verify", nil).WithContext(ctx) + result, err := checkTwoFACode(req, userID, st, "123456") + require.NoError(t, err) + require.Equal(t, twoFACodeOK, result) + + // The stored hash must now be the pepper'd form. + var stored sql.NullString + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&stored)) + require.True(t, stored.Valid, "checkTwoFACode alone must not clear the pending hash") + require.Equal(t, hashTwoFACode("123456"), stored.String, "legacy hash must be upgraded to the pepper'd form on successful verify") +} + +// TestTwoFAVerify_LegacyHash_StillVerifies pins the end-to-end migration +// window: an enforced env with the pepper set must still accept a user whose +// pending code was hashed the old (pre-pepper) way. +func TestTwoFAVerify_LegacyHash_StillVerifies(t *testing.T) { + twofaEnvEnforced(t) + t.Setenv("TWO_FACTOR_PEPPER", "test-pepper-secret") + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + _, err = tx.Exec(ctx, `UPDATE users + SET two_factor_method = 'email', + two_factor_pending_code_hash = $2, + two_factor_pending_code_expires = $3 + WHERE id = $1`, userID, legacyHashTwoFACode("123456"), clock.Now().Add(twoFAPendingExpiry)) + require.NoError(t, err) + + w := performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "123456"}, userID) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + var enabled bool + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_enabled FROM users WHERE id = $1", userID).Scan(&enabled)) + require.True(t, enabled) +} + +// TestTwoFAVerify_LegacyHash_WrongCodeRejected verifies the legacy path still +// enforces the correct code: a wrong code against a legacy-hashed row is +// rejected and 2FA stays off. +func TestTwoFAVerify_LegacyHash_WrongCodeRejected(t *testing.T) { + twofaEnvEnforced(t) + t.Setenv("TWO_FACTOR_PEPPER", "test-pepper-secret") + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + _, err = tx.Exec(ctx, `UPDATE users + SET two_factor_method = 'email', + two_factor_pending_code_hash = $2, + two_factor_pending_code_expires = $3 + WHERE id = $1`, userID, legacyHashTwoFACode("123456"), clock.Now().Add(twoFAPendingExpiry)) + require.NoError(t, err) + + w := performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "999999"}, userID) + require.Equal(t, http.StatusBadRequest, w.Code, w.Body.String()) + + var enabled bool + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_enabled FROM users WHERE id = $1", userID).Scan(&enabled)) + require.False(t, enabled) +} + +// ============================================================================= +// Disable-flow mint throttle (finding d) +// ============================================================================= + +// TestTwoFADisable_MintThrottled_BoundsGuessing verifies the unlimited-guess +// loop is closed: a password-only attacker who burns the 5-attempt budget on a +// freshly minted code cannot mint ANOTHER fresh code (which would reset the +// counter) inside the per-user mint cooldown. Exactly one fresh code is minted +// across the whole loop and the throttled request returns 429. +func TestTwoFADisable_MintThrottled_BoundsGuessing(t *testing.T) { + twofaEnvEnforced(t) + var buf bytes.Buffer + log.SetOutput(&buf) + t.Cleanup(func() { log.SetOutput(os.Stderr) }) + + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + _, err = tx.Exec(ctx, `UPDATE users SET two_factor_enabled = true, two_factor_method = 'email' WHERE id = $1`, userID) + require.NoError(t, err) + + // Request 1 mints a fresh code (the first mint is allowed) and rejects the + // wrong submission; requests 2-5 reuse that code, reaching the lockout. + for i := 0; i < 4; i++ { + w := performUser2FARequest(t, DisableTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/disable", TwoFADisableRequest{Code: "000000"}, userID) + require.Equal(t, http.StatusBadRequest, w.Code, "attempt %d", i+1) + } + w := performUser2FARequest(t, DisableTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/disable", TwoFADisableRequest{Code: "000000"}, userID) + require.Equal(t, http.StatusTooManyRequests, w.Code, w.Body.String()) + + // Request 6 is inside the cooldown: no fresh code may be minted, so the loop + // stops with 429 instead of minting an unlimited series of fresh codes. + w = performUser2FARequest(t, DisableTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/disable", TwoFADisableRequest{Code: "000000"}, userID) + require.Equal(t, http.StatusTooManyRequests, w.Code, w.Body.String()) + require.Contains(t, w.Body.String(), "Wait before requesting a new code.") + + // Exactly ONE fresh code was minted across all six requests — the loop can + // no longer reset the attempt budget. + mints := strings.Count(buf.String(), "disable 2FA") + require.Equal(t, 1, mints, "expected exactly 1 fresh-code mint; log:\n%s", buf.String()) +} + +// TestTwoFADisable_MintThrottle_ExpiresAllowsRecovery verifies the documented +// residual is not a permanent lockout: once the mint cooldown elapses, a +// legitimate code-lost user can mint and verify a fresh code again. +func TestTwoFADisable_MintThrottle_ExpiresAllowsRecovery(t *testing.T) { + twofaEnvEnforced(t) + var buf bytes.Buffer + log.SetOutput(&buf) + t.Cleanup(func() { log.SetOutput(os.Stderr) }) + + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + _, err = tx.Exec(ctx, `UPDATE users SET two_factor_enabled = true, two_factor_method = 'email' WHERE id = $1`, userID) + require.NoError(t, err) + + // Burn the budget: 4 wrong 400s, the 5th locks out (429). + for i := 0; i < 4; i++ { + w := performUser2FARequest(t, DisableTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/disable", TwoFADisableRequest{Code: "000000"}, userID) + require.Equal(t, http.StatusBadRequest, w.Code, "attempt %d", i+1) + } + w := performUser2FARequest(t, DisableTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/disable", TwoFADisableRequest{Code: "000000"}, userID) + require.Equal(t, http.StatusTooManyRequests, w.Code, w.Body.String()) + w = performUser2FARequest(t, DisableTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/disable", TwoFADisableRequest{Code: "000000"}, userID) + require.Equal(t, http.StatusTooManyRequests, w.Code, "mint must be throttled inside the cooldown") + + // Simulate the cooldown elapsing (the test cannot wait a real minute). + st := twoFAAttemptStateFor(userID) + st.lastMintAt = clock.Now().Add(-twoFAMintCooldown - time.Second) + + // A fresh disable request now mints a new code via the [2FA] log channel and + // rejects the wrong submission with 400 — recovery is possible again. + buf.Reset() + w = performUser2FARequest(t, DisableTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/disable", TwoFADisableRequest{Code: "000000"}, userID) + require.Equal(t, http.StatusBadRequest, w.Code, w.Body.String()) + require.Regexp(t, regexp.MustCompile(`\[2FA\].*\d{6}`), buf.String(), "cooldown expiry must allow a fresh mint") +} + +// ============================================================================= +// Attempt-map eviction (finding e) +// ============================================================================= + +// TestTwoFAAttemptMap_InLockoutRecordNotEvicted verifies the eviction fix: a +// record still inside its lockout window is NEVER evicted by LRU pressure — a +// hostile flood of new keys cannot reset the victim's attempt counter. Only an +// idle/expired record is dropped to make room. +func TestTwoFAAttemptMap_InLockoutRecordNotEvicted(t *testing.T) { + twoFAAttemptMapMu.Lock() + origMap := twoFAAttemptMap + origCap := twoFAMaxTrackedAttempts + twoFAAttemptMap = make(map[string]*twoFAAttemptState) + twoFAMaxTrackedAttempts = 4 + twoFAAttemptMapMu.Unlock() + t.Cleanup(func() { + twoFAAttemptMapMu.Lock() + twoFAAttemptMap = origMap + twoFAMaxTrackedAttempts = origCap + twoFAAttemptMapMu.Unlock() + }) + + now := clock.Now() + for _, id := range []string{"idle_a", "idle_b", "idle_c"} { + twoFAAttemptMap[id] = &twoFAAttemptState{lastAt: now.Add(-time.Minute)} + } + victim := &twoFAAttemptState{lastAt: now.Add(-time.Second)} + victim.count.Store(5) + twoFAAttemptMap["victim"] = victim + + // A new user hits the cap: the eviction must drop an idle record, never the + // in-lockout victim. + st := twoFAAttemptStateFor("new_user") + require.NotNil(t, st) + if _, ok := twoFAAttemptMap["victim"]; !ok { + t.Error("in-lockout record must never be evicted by LRU pressure") + } + if got := twoFAAttemptMap["victim"].count.Load(); got != 5 { + t.Errorf("victim attempt count must survive eviction pressure, got %d", got) + } + if len(twoFAAttemptMap) > 4 { + t.Errorf("map must stay within the cap, got %d entries", len(twoFAAttemptMap)) + } + if _, ok := twoFAAttemptMap["new_user"]; !ok { + t.Error("new user must be tracked in the map") + } +} + +// TestTwoFAAttemptMap_FullOfLockedOut_ReturnsTransient verifies the pathological +// case: when every entry is a locked-out in-window record (a flood), the map +// does NOT evict one and does NOT grow past the cap — the new user gets a +// transient, untracked state for this request instead. +func TestTwoFAAttemptMap_FullOfLockedOut_ReturnsTransient(t *testing.T) { + twoFAAttemptMapMu.Lock() + origMap := twoFAAttemptMap + origCap := twoFAMaxTrackedAttempts + twoFAAttemptMap = make(map[string]*twoFAAttemptState) + twoFAMaxTrackedAttempts = 3 + twoFAAttemptMapMu.Unlock() + t.Cleanup(func() { + twoFAAttemptMapMu.Lock() + twoFAAttemptMap = origMap + twoFAMaxTrackedAttempts = origCap + twoFAAttemptMapMu.Unlock() + }) + + now := clock.Now() + for i := 0; i < 3; i++ { + st := &twoFAAttemptState{lastAt: now.Add(-time.Second)} + st.count.Store(5) + twoFAAttemptMap[fmt.Sprintf("locked_%d", i)] = st + } + + st := twoFAAttemptStateFor("new_user") + require.NotNil(t, st) + if _, ok := twoFAAttemptMap["new_user"]; ok { + t.Error("expected the transient state NOT to be stored when the map is full of in-lockout records") + } + if len(twoFAAttemptMap) != 3 { + t.Errorf("expected all 3 locked-out records to survive, got %d", len(twoFAAttemptMap)) + } +} diff --git a/backend/handlers/webhooks/square.go b/backend/handlers/webhooks/square.go index 820afb4..fa4923b 100644 --- a/backend/handlers/webhooks/square.go +++ b/backend/handlers/webhooks/square.go @@ -13,7 +13,9 @@ import ( "log" "net/http" "os" + "strings" "sync" + "time" "crussell/db" "crussell/handlers/payments" @@ -82,6 +84,56 @@ func (d *squareWebhookDedup) register(id string) bool { // (square_webhook_events) is the unbounded, restart-safe source of truth. var squareWebhookEventsSeen = newSquareWebhookDedup(500) +// webhookDBTimeout bounds the DB work performed while dispatching a webhook. +// The work runs on a Background-derived context — so a client disconnect cannot +// cancel it (the at-least-once delivery contract must survive) — but is +// timeout-bound so a hung DB call cannot hold a pgx pool connection forever; +// repeated hangs would otherwise exhaust the pool. 30s is the same +// post-request DB budget used elsewhere in the backend (handlers/user). +const webhookDBTimeout = 30 * time.Second + +// webhookDBContext returns a timeout-bound, Background-derived context for +// webhook dispatch DB work. +func webhookDBContext() (context.Context, context.CancelFunc) { + return context.WithTimeout(context.Background(), webhookDBTimeout) +} + +// squareEnvironmentMismatch reports whether the webhook's square-environment +// header conflicts with the deployment's configured SQUARE_ENVIRONMENT. +// +// The check is enforced ONLY when the configured environment is a known real +// Square environment (production/sandbox): those are the deployments where a +// mis-pointed subscription (e.g. a sandbox subscription posting to the +// production URL + signing key) would process events against the wrong state. +// In dev/mock deployments — or an empty/unknown SQUARE_ENVIRONMENT, which the +// rest of the backend treats as fail-closed production but is not a specific +// real environment to compare against — the header is informational and a +// mismatch is not rejectable, mirroring IsExplicitDevOrMockEnv +// (handlers/payments/twofa.go) so the interpretation cannot diverge. +// +// An absent header is allowed through: real Square deliveries always send it, +// so a missing header in an enforced deployment is a non-Square client (which +// already failed signature verification) or a local mock/dev poster — both +// safely handled downstream. Rejection is 403 (non-retryable for Square): +// a square-environment mismatch is a PERMANENT configuration error that a +// retry could never resolve, so a 5xx would make Square retry forever; a 4xx +// stops the retry loop and forces the operator to fix the subscription or the +// environment setting. +func squareEnvironmentMismatch(headerEnv string) bool { + headerEnv = strings.ToLower(strings.TrimSpace(headerEnv)) + if headerEnv == "" { + return false + } + switch configured := strings.ToLower(strings.TrimSpace(os.Getenv("SQUARE_ENVIRONMENT"))); configured { + case "production", "sandbox": + return headerEnv != configured + default: + // Empty/unknown/dev/mock configured environment — no specific real + // environment to enforce against. + return false + } +} + // HandleSquareWebhook verifies and dispatches Square webhook events. // // Fail-closed chain: 503 when the signing key is unset, 403 on a missing/bad @@ -130,6 +182,19 @@ func HandleSquareWebhook(w http.ResponseWriter, r *http.Request) { return } + // Fail-closed environment check: a sandbox subscription mis-pointed at the + // production URL + key would otherwise process sandbox events against + // production state. 403 (non-retryable for Square) is correct because a + // square-environment mismatch is a permanent config error — a 5xx would + // make Square retry a condition no retry can fix. See + // squareEnvironmentMismatch for the exact enforcement conditions. + if squareEnvironmentMismatch(r.Header.Get("square-environment")) { + log.Printf("[SQUARE-WEBHOOK] Rejecting event: square-environment header %q does not match configured SQUARE_ENVIRONMENT %q (403)", + r.Header.Get("square-environment"), os.Getenv("SQUARE_ENVIRONMENT")) + http.Error(w, "square environment mismatch", http.StatusForbidden) + return + } + var event SquareWebhookEvent if err := json.Unmarshal(body, &event); err != nil { log.Printf("Failed to parse webhook event: %v", err) @@ -181,7 +246,7 @@ func HandleSquareWebhook(w http.ResponseWriter, r *http.Request) { var dispatchErr error switch event.Type { case "payment.updated", "payment.created": - dispatchErr = handlePaymentUpdated(r.Context(), event.Data) + dispatchErr = handlePaymentUpdated(event.Data) case "refund.updated", "refund.created": dispatchErr = handleRefundUpdated(event.Data) case "dispute.created": @@ -205,7 +270,15 @@ func HandleSquareWebhook(w http.ResponseWriter, r *http.Request) { // error: without a persisted row we cannot prove the event was handled, so // reject and let Square retry (the retry re-dispatches idempotently and // retries the insert). event_id is not PII, so logging it is safe. - tag, err := db.Conn.Exec(r.Context(), + // Commit the dedup row AFTER successful dispatch. Fail closed on a write + // error: without a persisted row we cannot prove the event was handled, so + // reject and let Square retry (the retry re-dispatches idempotently and + // retries the insert). event_id is not PII, so logging it is safe. A + // bounded Background context keeps this post-dispatch write alive across a + // client disconnect without letting a hung insert hold the pool forever. + dedupCtx, dedupCancel := webhookDBContext() + defer dedupCancel() + tag, err := db.Conn.Exec(dedupCtx, "INSERT INTO square_webhook_events (event_id) VALUES ($1) ON CONFLICT (event_id) DO NOTHING", event.EventID) if err != nil { log.Printf("[SQUARE-WEBHOOK] Failed to record event_id %s (dedup write failed): %v", event.EventID, err) @@ -355,14 +428,16 @@ func squareDisputeStateToLocal(state string) string { // 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) { +// a deposit + balance split); the most recent is used. The caller supplies a +// bounded context (webhookDBContext) so this post-dispatch DB work survives a +// client disconnect without holding a pool connection forever. +func findPaymentBySquareID(ctx context.Context, squarePaymentID string) (paymentID, bookingID string, ok bool) { if squarePaymentID == "" { return "", "", false } var pid string var bid *string - err := db.Conn.QueryRow(context.Background(), ` + err := db.Conn.QueryRow(ctx, ` SELECT id, booking_id FROM payments WHERE square_payment_id = $1 ORDER BY created_at DESC, id DESC @@ -379,11 +454,12 @@ func findPaymentBySquareID(squarePaymentID string) (paymentID, bookingID string, // 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) { +// exists but the webhook payload carries no resolvable Square payment id. The +// caller supplies a bounded context (webhookDBContext). +func findPaymentByDisputeID(ctx context.Context, squareDisputeID string) (paymentID, bookingID string) { var pid string var bid *string - err := db.Conn.QueryRow(context.Background(), ` + err := db.Conn.QueryRow(ctx, ` SELECT d.payment_id, p.booking_id FROM disputes d JOIN payments p ON p.id = d.payment_id @@ -423,10 +499,11 @@ func disputeNotificationID(squareDisputeID string) string { // of the same dispute is a no-op (ON CONFLICT (id) DO NOTHING). The // booking-scoped NOT EXISTS guard does NOT apply to this path: it would // collapse every untracked dispute onto one unacknowledged NULL-booking row. -func insertCriticalPaymentNotification(bookingID, disputeID string) { +// The caller supplies a bounded context (webhookDBContext). +func insertCriticalPaymentNotification(ctx context.Context, bookingID, disputeID string) { if disputeID != "" { id := disputeNotificationID(disputeID) - tag, err := db.Conn.Exec(context.Background(), ` + tag, err := db.Conn.Exec(ctx, ` INSERT INTO admin_notifications (id, reason, booking_id, created_at) VALUES ($1, 'critical_payment_log'::admin_notification_reason, NULL, NOW()) ON CONFLICT (id) DO NOTHING @@ -444,7 +521,7 @@ func insertCriticalPaymentNotification(bookingID, disputeID string) { if bookingID != "" { bid = bookingID } - tag, err := db.Conn.Exec(context.Background(), ` + tag, err := db.Conn.Exec(ctx, ` INSERT INTO admin_notifications (reason, booking_id, created_at) SELECT 'critical_payment_log'::admin_notification_reason, $1, NOW() WHERE NOT EXISTS ( @@ -466,11 +543,12 @@ func insertCriticalPaymentNotification(bookingID, disputeID string) { // markPaymentFailed flips a payment to 'failed' after a lost dispute — the // money was charged back, so the row must not read as collected. 'refunded' // rows are left alone (the money was returned by refund, not charged back). -func markPaymentFailed(paymentID string) error { +// The caller supplies a bounded context (webhookDBContext). +func markPaymentFailed(ctx context.Context, paymentID string) error { if paymentID == "" { return nil } - _, err := db.Conn.Exec(context.Background(), + _, err := db.Conn.Exec(ctx, "UPDATE payments SET status = 'failed', updated_at = NOW() WHERE id = $1 AND status IN ('pending', 'completed')", paymentID) if err != nil { @@ -485,8 +563,13 @@ func markPaymentFailed(paymentID string) error { // 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. A non-nil error means dispatch failed and the -// caller must NOT commit the dedup row (Square retries). -func handlePaymentUpdated(ctx context.Context, data json.RawMessage) error { +// caller must NOT commit the dedup row (Square retries). All DB work runs on a +// bounded Background context (webhookDBContext): a client disconnect must not +// cancel the state mutation, and a hung DB call must not hold the pool forever. +func handlePaymentUpdated(data json.RawMessage) error { + ctx, cancel := webhookDBContext() + defer cancel() + var env squareWebhookData if err := json.Unmarshal(data, &env); err != nil { log.Printf("[SQUARE-WEBHOOK] payment.updated received (payload length=%d)", len(data)) @@ -511,7 +594,7 @@ func handlePaymentUpdated(ctx context.Context, data json.RawMessage) error { // 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(), + tag, err := db.Conn.Exec(ctx, `UPDATE payments SET status = $1, updated_at = NOW() WHERE square_payment_id = $2 AND status = 'pending'`, localStatus, payment.ID) if err != nil { @@ -530,7 +613,7 @@ func handlePaymentUpdated(ctx context.Context, data json.RawMessage) error { if localStatus == "failed" { return clawbackFailedTillSales(ctx, payment.ID) } - tsTag, err := db.Conn.Exec(context.Background(), + tsTag, err := db.Conn.Exec(ctx, `UPDATE till_sales SET status = $1, updated_at = NOW() WHERE square_payment_id = $2 AND status = 'pending'`, localStatus, payment.ID) if err != nil { @@ -553,7 +636,7 @@ func handlePaymentUpdated(ctx context.Context, data json.RawMessage) error { // sale's funding unreverted — the caller rejects the webhook so Square retries // the clawback (the sweep is the eventual backstop). func clawbackFailedTillSales(ctx context.Context, squarePaymentID string) error { - rows, err := db.Conn.Query(context.Background(), ` + rows, err := db.Conn.Query(ctx, ` SELECT ts.id, ts.item_type, ts.item_id, ts.total_amount, gc.redeemed_by, (ts.created_at = gc.created_at) AS is_create FROM till_sales ts @@ -593,7 +676,7 @@ func clawbackOneTillSale(ctx context.Context, saleID, itemType string, itemID sq if itemType != "gift_card" || !itemID.Valid || itemID.String == "" || isCreate == nil { // No gift card to claw back — mark the sale failed without touching // any card (mirrors the sweep's non-gift-card branch). - tag, err := db.Conn.Exec(context.Background(), ` + tag, err := db.Conn.Exec(ctx, ` UPDATE till_sales SET status = 'failed', updated_at = NOW() WHERE id = $1 AND status = 'pending' `, saleID) @@ -639,8 +722,12 @@ func revertTillSaleGiftCardFunding(ctx context.Context, action, giftCardID strin // handleRefundUpdated reconciles a Square Refund state change against the local // refunds row. Idempotent (status-guarded UPDATE + event_id dedup). A non-nil -// error means dispatch failed and the caller must NOT commit the dedup row. +// error means dispatch failed and the caller must NOT commit the dedup row. DB +// work runs on a bounded Background context (webhookDBContext). func handleRefundUpdated(data json.RawMessage) error { + ctx, cancel := webhookDBContext() + defer cancel() + var env squareWebhookData if err := json.Unmarshal(data, &env); err != nil { log.Printf("[SQUARE-WEBHOOK] refund.updated received (payload length=%d)", len(data)) @@ -672,7 +759,7 @@ func handleRefundUpdated(data json.RawMessage) error { 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) + tag, err := db.Conn.Exec(ctx, upd, refund.ID) if err != nil { log.Printf("[SQUARE-WEBHOOK] Failed to update refund %s to status %s: %v", refund.ID, localStatus, err) return err @@ -709,8 +796,12 @@ func truncateDisputeReason(reason string) string { // sweep fallback for disputes and a chargeback the app cannot see is a silent // money-loss path the owner must always be told about. Idempotent via // ON CONFLICT (square_dispute_id) DO NOTHING plus the event_id dedup. A non-nil -// error means dispatch failed (no dedup row committed — Square retries). +// error means dispatch failed (no dedup row committed — Square retries). DB +// work runs on a bounded Background context (webhookDBContext). func handleDisputeCreated(data json.RawMessage) error { + ctx, cancel := webhookDBContext() + defer cancel() + var env squareWebhookData if err := json.Unmarshal(data, &env); err != nil { log.Printf("[SQUARE-WEBHOOK] dispute.created received (payload length=%d)", len(data)) @@ -725,7 +816,7 @@ func handleDisputeCreated(data json.RawMessage) error { if dispute.DisputedPayment != nil { squarePaymentID = dispute.DisputedPayment.PaymentID } - paymentID, bookingID, paymentFound := findPaymentBySquareID(squarePaymentID) + paymentID, bookingID, paymentFound := findPaymentBySquareID(ctx, squarePaymentID) if !paymentFound { // Untracked chargeback: no local payments row for this Square charge // (Dashboard-initiated, mismatched Square payment id, or a deleted/erased @@ -737,11 +828,11 @@ func handleDisputeCreated(data json.RawMessage) error { // Still return nil so the dedup row commits and Square's retry is // acknowledged. log.Printf("[SQUARE-WEBHOOK] CRITICAL: dispute %s created for square payment %q with NO local payment row — chargeback cannot be reconciled in-app — admin notified (booking_id NULL)", dispute.ID, squarePaymentID) - insertCriticalPaymentNotification("", dispute.ID) + insertCriticalPaymentNotification(ctx, "", dispute.ID) return nil } amount := squareMoneyToAmount(dispute.AmountMoney) - tag, err := db.Conn.Exec(context.Background(), ` + tag, err := db.Conn.Exec(ctx, ` 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 @@ -751,7 +842,7 @@ func handleDisputeCreated(data json.RawMessage) error { return err } _ = tag - insertCriticalPaymentNotification(bookingID, "") + insertCriticalPaymentNotification(ctx, bookingID, "") log.Printf("[SQUARE-WEBHOOK] CRITICAL: dispute %s created (amount %s, reason %q) for square payment %s — admin notified", dispute.ID, amount, dispute.Reason, squarePaymentID) return nil } @@ -760,8 +851,12 @@ func handleDisputeCreated(data json.RawMessage) error { // 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. A non-nil -// error means dispatch failed (no dedup row committed — Square retries). +// error means dispatch failed (no dedup row committed — Square retries). DB +// work runs on a bounded Background context (webhookDBContext). func handleDisputeStateUpdated(data json.RawMessage) error { + ctx, cancel := webhookDBContext() + defer cancel() + var env squareWebhookData if err := json.Unmarshal(data, &env); err != nil { log.Printf("[SQUARE-WEBHOOK] dispute.state.updated received (payload length=%d)", len(data)) @@ -779,17 +874,17 @@ func handleDisputeStateUpdated(data json.RawMessage) error { if dispute.DisputedPayment != nil { squarePaymentID = dispute.DisputedPayment.PaymentID } - paymentID, bookingID, paymentFound := findPaymentBySquareID(squarePaymentID) + paymentID, bookingID, paymentFound := findPaymentBySquareID(ctx, squarePaymentID) if !paymentFound { // Row may already exist from dispute.created — recover its payment. - paymentID, bookingID = findPaymentByDisputeID(dispute.ID) + paymentID, bookingID = findPaymentByDisputeID(ctx, 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 nil } } - _, err := db.Conn.Exec(context.Background(), ` + _, err := db.Conn.Exec(ctx, ` 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 @@ -803,10 +898,10 @@ func handleDisputeStateUpdated(data json.RawMessage) error { switch localStatus { case "lost": - if err := markPaymentFailed(paymentID); err != nil { + if err := markPaymentFailed(ctx, paymentID); err != nil { return err } - insertCriticalPaymentNotification(bookingID, "") + insertCriticalPaymentNotification(ctx, 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) diff --git a/backend/handlers/webhooks/webhooks_test.go b/backend/handlers/webhooks/webhooks_test.go index 3ff531b..d018498 100644 --- a/backend/handlers/webhooks/webhooks_test.go +++ b/backend/handlers/webhooks/webhooks_test.go @@ -16,6 +16,7 @@ import ( "os" "strings" "testing" + "time" "unicode/utf8" "crussell/db" @@ -149,6 +150,23 @@ func makeWebhookRequest(body []byte, signature string, ctx context.Context) *htt return w } +// makeWebhookRequestWithEnv is makeWebhookRequest plus an explicit +// square-environment header (Square sends one on every real delivery). +func makeWebhookRequestWithEnv(body []byte, signature, env string, ctx context.Context) *httptest.ResponseRecorder { + w := httptest.NewRecorder() + req := httptest.NewRequest("POST", "/webhooks/square", bytes.NewReader(body)) + req = req.WithContext(ctx) + req.Header.Set("Content-Type", "application/json") + if signature != "" { + req.Header.Set("x-square-hmacsha256-signature", signature) + } + if env != "" { + req.Header.Set("square-environment", env) + } + HandleSquareWebhook(w, req) + return w +} + // webhookTestEnv sets a signing key and returns a valid signature for the body // (the fail-closed handler requires a verifiable signature on every request). func webhookTestEnv(t *testing.T, body []byte) (signature string) { @@ -621,3 +639,119 @@ func TestHandleSquareWebhook_DedupNilConn_FailsClosed(t *testing.T) { t.Fatalf("expected 503 when DB is not wired, got %d. body: %s", w.Code, w.Body.String()) } } + +// ============================================================================= +// square-environment header validation (finding a) +// ============================================================================= + +// TestHandleSquareWebhook_EnvMismatch_Rejected verifies fail-closed behavior: a +// correctly-signed event carrying a square-environment header that contradicts +// the configured SQUARE_ENVIRONMENT (e.g. a sandbox subscription mis-pointed at +// the production URL + key) is rejected with 403. 403 is correct here because +// the mismatch is a permanent configuration error — Square treats 4xx as +// non-retryable, so the retry loop stops instead of hammering a condition no +// retry can fix. No dispatch and no dedup row. +func TestHandleSquareWebhook_EnvMismatch_Rejected(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", "production") + event := SquareWebhookEvent{ + Type: "payment.updated", + EventID: "evt_env_mismatch_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{"id":"payment_env_mismatch_1"}`), + } + body, _ := json.Marshal(event) + sig := webhookTestEnv(t, body) + + w := makeWebhookRequestWithEnv(body, sig, "sandbox", context.Background()) + if w.Code != http.StatusForbidden { + t.Fatalf("expected 403 on environment mismatch, got %d. body: %s", w.Code, w.Body.String()) + } + if n := countWebhookEvents(t, event.EventID); n != 0 { + t.Errorf("expected no dedup row for a rejected event, got %d", n) + } +} + +// TestHandleSquareWebhook_EnvMatch_Accepted verifies a matching environment +// header passes the check and dispatches normally. +func TestHandleSquareWebhook_EnvMatch_Accepted(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", "production") + event := SquareWebhookEvent{ + Type: "payment.updated", + EventID: "evt_env_match_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{"id":"payment_env_match_1"}`), + } + body, _ := json.Marshal(event) + sig := webhookTestEnv(t, body) + + w := makeWebhookRequestWithEnv(body, sig, "production", context.Background()) + if w.Code != http.StatusOK { + t.Fatalf("expected 200 on matching environment, got %d. body: %s", w.Code, w.Body.String()) + } + if n := countWebhookEvents(t, event.EventID); n != 1 { + t.Errorf("expected 1 dedup row after successful dispatch, got %d", n) + } +} + +// TestHandleSquareWebhook_EnvHeaderAbsent_Allowed verifies an absent header is +// allowed through (local mock/dev posting), even in an enforced deployment — +// the signature check remains the authentication gate. +func TestHandleSquareWebhook_EnvHeaderAbsent_Allowed(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", "production") + event := SquareWebhookEvent{ + Type: "payment.updated", + EventID: "evt_env_absent_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{"id":"payment_env_absent_1"}`), + } + body, _ := json.Marshal(event) + sig := webhookTestEnv(t, body) + + w := makeWebhookRequestWithEnv(body, sig, "", context.Background()) + if w.Code != http.StatusOK { + t.Fatalf("expected 200 when the environment header is absent, got %d. body: %s", w.Code, w.Body.String()) + } +} + +// TestHandleSquareWebhook_EnvMismatch_DevNotEnforced verifies the header check +// is NOT enforced when the configured SQUARE_ENVIRONMENT is a dev/mock value +// (the same interpretation IsExplicitDevOrMockEnv uses elsewhere), so a header +// carrying "sandbox" against an explicit "mock" config still dispatches. +func TestHandleSquareWebhook_EnvMismatch_DevNotEnforced(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", "mock") + event := SquareWebhookEvent{ + Type: "payment.updated", + EventID: "evt_env_dev_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{"id":"payment_env_dev_1"}`), + } + body, _ := json.Marshal(event) + sig := webhookTestEnv(t, body) + + w := makeWebhookRequestWithEnv(body, sig, "sandbox", context.Background()) + if w.Code != http.StatusOK { + t.Fatalf("expected 200 in dev mode (check not enforced), got %d. body: %s", w.Code, w.Body.String()) + } +} + +// ============================================================================= +// Bounded post-dispatch DB contexts (finding b) +// ============================================================================= + +// TestWebhookDBContext_HasTimeout verifies webhookDBContext returns a +// Background-derived context with a deadline, so a hung DB call cannot hold a +// pgx pool connection forever while still surviving a client disconnect. +func TestWebhookDBContext_HasTimeout(t *testing.T) { + ctx, cancel := webhookDBContext() + defer cancel() + deadline, ok := ctx.Deadline() + if !ok { + t.Fatal("expected webhookDBContext to carry a deadline") + } + if remaining := time.Until(deadline); remaining <= 0 || remaining > webhookDBTimeout { + t.Errorf("expected remaining budget within (0, %v], got %v", webhookDBTimeout, remaining) + } + if got := ctx.Err(); got != nil { + t.Errorf("expected a fresh context to be active, got %v", got) + } +} diff --git a/backend/internal/square/square_dev.go b/backend/internal/square/square_dev.go index bab4a4b..d2122af 100644 --- a/backend/internal/square/square_dev.go +++ b/backend/internal/square/square_dev.go @@ -19,6 +19,28 @@ package square // test as evidence of how prod treats a retained key after a restart. If a // test needs retained-key behaviour, it must re-seed the payment under the key // into the same mock instance (see TestSweepStalePendingPayments_KeyedLostResponse_CompletedRescued). +// +// FAULT-INJECTION TOGGLES. The mock exposes opt-in toggles (ShouldFail, +// FailRefundCode, ForceCheckoutState, ForceRefundPending, FailCreateCheckout, +// FailAfterCommit, SimulateCardTokenUsed) that let dev/tests drive Square +// failure modes that are otherwise only reachable against the real API. +// FailAfterCommit simulates the exact "charged but response lost → same-key +// retry" prod scenario: CreatePayment COMMITS the charge (retaining the key +// and source in the ledgers exactly like a successful charge) and THEN returns +// a 5xx-style error to the caller. A subsequent CreatePayment with the SAME +// key + SAME source dedups to the committed payment, proving no double charge. +// SimulateCardTokenUsed simulates Square's CARD_TOKEN_USED rejection of a card +// token (cnon: nonce) reused after a previous save. +// +// REAL-API SAFETY GUARD. A `//go:build dev` build must never silently route to +// the real PRODUCTION Square API on an env-string match alone — a typo'd or +// leftover SQUARE_ENVIRONMENT=production in a dev shell would otherwise create +// REAL charges from test bookings. NewDevClient therefore HARD-FAILS (panics +// with errDevRealAPIRequiresOverride) when SQUARE_ENVIRONMENT=production +// unless the explicit override SQUARE_ALLOW_REAL_API=1 is set, and logs a loud +// banner before routing a dev build to the SANDBOX. The non-dev build +// (square.go, `//go:build !dev`) is untouched: NewProdClient always uses the +// real client path selected by the normal non-dev wiring. import ( "context" @@ -81,6 +103,26 @@ type MockClient struct { // post-insert CreateCheckout-failure path (marking the provisional // terminal_checkouts row failed) can be exercised in dev/tests. FailCreateCheckout bool + // FailAfterCommit simulates the exact "charged but response lost → same-key + // retry" prod scenario: CreatePayment COMMITS the charge internally + // (retaining the key + source in paymentByKey/paymentSource exactly like a + // successful charge) and THEN returns a 5xx-style error to the caller. A + // subsequent CreatePayment with the SAME key + SAME source dedups to the + // committed payment — never a second charge — exercising the retry path + // devs hit in prod when Square processes a charge but the response is lost. + FailAfterCommit bool + // SimulateCardTokenUsed makes CreateCardOnFile enforce Square's + // CARD_TOKEN_USED rejection: a card token (cnon: nonce) already used to + // create a card on this mock instance is rejected with the same structured + // 400 CARD_TOKEN_USED error real Square returns. Off by default — dev/test + // flows reuse plain "cnon:test-card"-style tokens across requests, so + // enforcement is enabled only in tests that exercise the reused-token + // rejection. UsedCardTokens() reports the tokens consumed so far. + SimulateCardTokenUsed bool + // usedCardTokens records card tokens consumed by CreateCardOnFile while + // SimulateCardTokenUsed is enabled (Square consumes a cnon: nonce on card + // creation, so reusing it is rejected with CARD_TOKEN_USED). + usedCardTokens map[string]bool } type devProdClient struct{} @@ -130,24 +172,47 @@ func NewClient() SquareClient { return NewDevClient() } +// errDevRealAPIRequiresOverride is the hard-fail error NewDevClient panics +// with when a dev build is asked to route to the real PRODUCTION Square API +// without the explicit SQUARE_ALLOW_REAL_API=1 override. A dev build must +// never silently charge real money on an env-string match alone. +var errDevRealAPIRequiresOverride = errors.New("square: dev build refuses SQUARE_ENVIRONMENT=production without SQUARE_ALLOW_REAL_API=1 (would route to the REAL Square API)") + func NewDevClient() SquareClient { - env := os.Getenv("SQUARE_ENVIRONMENT") - if env == "sandbox" || env == "production" { - log.Printf("[SQUARE-PROD] SQUARE_ENVIRONMENT=%s — making real API calls to %s", env, realBaseURL(env)) + env := SquareEnvironment() + switch env { + case "production": + // A `//go:build dev` build routing to the real production API is an + // explicit safety boundary, not a string-match convenience. Without + // the override, a typo'd or leftover SQUARE_ENVIRONMENT=production in + // a dev shell would make test bookings create REAL charges and payouts. + // Fail fast so the misconfiguration is impossible to miss. + if os.Getenv("SQUARE_ALLOW_REAL_API") != "1" { + log.Printf("[SQUARE-PROD] REFUSING to construct the real production Square client in a dev build: SQUARE_ENVIRONMENT=production without SQUARE_ALLOW_REAL_API=1 — set SQUARE_ALLOW_REAL_API=1 to override, or SQUARE_ENVIRONMENT=sandbox/mock for safe dev traffic") + panic(errDevRealAPIRequiresOverride) + } + log.Printf("[SQUARE-PROD] SQUARE_ENVIRONMENT=production WITH SQUARE_ALLOW_REAL_API=1 — dev build making REAL API calls to %s (explicit override, real money)", realBaseURL(env)) return &devProdClient{} - } - log.Println("[SQUARE-MOCK] Using in-memory mock client") - return &MockClient{ - cards: make(map[string]map[string]*CardOnFile), - cardByToken: make(map[string]*CardOnFile), - checkouts: make(map[string]*CheckoutResult), - payments: make(map[string]*PaymentResult), - paymentByKey: make(map[string]*PaymentResult), - paymentSource: make(map[string]string), - refunds: make(map[string]*RefundResult), - refundByKey: make(map[string]*RefundResult), - customers: make(map[string]*CustomerResult), - completed: make(map[string]*PaymentResult), + case "sandbox": + // Sandbox never moves real money, so a dev build may route there — but + // loudly, so no-one mistakes a sandbox for the mock. + log.Printf("[SQUARE-PROD] *** DEV BUILD ROUTING TO SQUARE SANDBOX %s — test credentials only, NO real charges — this is NOT the mock client ***", realBaseURL(env)) + return &devProdClient{} + default: + log.Println("[SQUARE-MOCK] Using in-memory mock client") + return &MockClient{ + cards: make(map[string]map[string]*CardOnFile), + cardByToken: make(map[string]*CardOnFile), + checkouts: make(map[string]*CheckoutResult), + payments: make(map[string]*PaymentResult), + paymentByKey: make(map[string]*PaymentResult), + paymentSource: make(map[string]string), + refunds: make(map[string]*RefundResult), + refundByKey: make(map[string]*RefundResult), + customers: make(map[string]*CustomerResult), + completed: make(map[string]*PaymentResult), + usedCardTokens: make(map[string]bool), + } } } @@ -205,6 +270,20 @@ func (m *MockClient) CreatePayment(ctx context.Context, req CreatePaymentReq) (* err: errors.New("square: customer_id required for card-on-file source"), } } + // Square's idempotency-key limit for POST /v2/payments is 45 characters + // (64 only for /v2/terminals/checkouts). Real Square rejects an oversized + // key with a 400 VALUE_TOO_LONG; the mock mirrors the rejection with the + // same structured error so dev parity catches over-length keys (the real + // client always derives ≤45-char keys, so this only fires on a caller bug). + if len(req.IdempotencyKey) > 45 { + return nil, &squareAPIError{ + Code: "VALUE_TOO_LONG", + Detail: "idempotency_key must be 45 characters or fewer", + Category: "INVALID_REQUEST_ERROR", + StatusCode: http.StatusBadRequest, + err: fmt.Errorf("square: idempotency_key %s is %d chars, exceeds Square's 45-char limit", tokenPrefix(req.IdempotencyKey), len(req.IdempotencyKey)), + } + } // Do NOT log the full source token — it is a single-use nonce (cnon:) or a // card reference (ccof:) that could be replayed. Log only its prefix and // length for debugging (S-2). @@ -309,6 +388,15 @@ func (m *MockClient) CreatePayment(ctx context.Context, req CreatePaymentReq) (* m.paymentSource[req.IdempotencyKey] = req.SourceID } log.Printf("[SQUARE-MOCK] Payment created: id=%s, status=%s, amount=%d, fees=%d", paymentID, status, amount, fees) + if m.FailAfterCommit { + // The charge is already committed above (payment + key + source are in + // the ledgers exactly like a successful charge) — now simulate the lost + // response: the caller sees a 5xx-style error while Square holds the + // payment under the key. A same-key + same-source retry dedups to the + // committed payment instead of charging twice, exactly like prod. + log.Printf("[SQUARE-MOCK] FailAfterCommit: payment %s committed under key=%s but returning simulated 503 (response lost)", paymentID, req.IdempotencyKey) + return nil, fmt.Errorf("square: charge %s committed but response lost (simulated HTTP 503) — retry with the same idempotency key to receive the committed payment", paymentID) + } return result, nil } @@ -609,6 +697,19 @@ func (m *MockClient) RefundKeyCount() int { return len(m.refundByKey) } +// UsedCardTokens returns the card tokens consumed by CreateCardOnFile while +// SimulateCardTokenUsed is enabled. Test accessor for asserting that a reused +// token is rejected with CARD_TOKEN_USED after a previous save. +func (m *MockClient) UsedCardTokens() []string { + m.mu.RLock() + defer m.mu.RUnlock() + out := make([]string, 0, len(m.usedCardTokens)) + for tok := range m.usedCardTokens { + out = append(out, tok) + } + return out +} + func (m *MockClient) CreateCardOnFile(ctx context.Context, userID, cardToken, customerID string) (*CardOnFile, error) { log.Printf("[SQUARE-MOCK] CreateCardOnFile: user=%s", userID) @@ -637,6 +738,19 @@ func (m *MockClient) CreateCardOnFile(ctx context.Context, userID, cardToken, cu m.mu.Lock() defer m.mu.Unlock() + if m.SimulateCardTokenUsed && m.usedCardTokens[cardToken] { + // Real Square consumes a cnon: nonce on card creation — reusing it to + // create another card is rejected with CARD_TOKEN_USED. The mock + // mirrors that structured 400 rejection (opt-in, see the struct doc). + return nil, &squareAPIError{ + Code: "CARD_TOKEN_USED", + Detail: "The card token has already been used.", + Category: "INVALID_REQUEST_ERROR", + StatusCode: http.StatusBadRequest, + err: fmt.Errorf("square: card token %s has already been used", tokenPrefix(cardToken)), + } + } + if m.cards[userID] == nil { m.cards[userID] = make(map[string]*CardOnFile) } @@ -665,6 +779,9 @@ func (m *MockClient) CreateCardOnFile(ctx context.Context, userID, cardToken, cu } m.cards[userID][cardID] = card m.cardByToken[card.CardID] = card + if m.SimulateCardTokenUsed { + m.usedCardTokens[cardToken] = true + } log.Printf("[SQUARE-MOCK] Card created: id=%s, brand=%s, last4=%s", cardID, card.Brand, card.Last4) return card, nil } diff --git a/backend/internal/square/square_dev_test.go b/backend/internal/square/square_dev_test.go index 70a63f9..67d8fb1 100644 --- a/backend/internal/square/square_dev_test.go +++ b/backend/internal/square/square_dev_test.go @@ -10,6 +10,7 @@ import ( "fmt" "log" "net/http" + "net/http/httptest" "os" "strings" "sync" @@ -1479,3 +1480,249 @@ func TestDevClient_ListPaymentRefunds_ConcurrentReads(t *testing.T) { require.NoError(t, err) assert.Len(t, results, 8) } + +// TestDevClient_ProductionEnvWithoutOverride_HardFails locks the dev-safety +// boundary: a `//go:build dev` build must NEVER silently route to the real +// PRODUCTION Square API on an env-string match alone (a typo'd/leftover +// SQUARE_ENVIRONMENT=production in a dev shell would create REAL charges from +// test bookings). NewDevClient hard-fails unless the explicit +// SQUARE_ALLOW_REAL_API=1 override is set; with the override it proceeds. +func TestDevClient_ProductionEnvWithoutOverride_HardFails(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", "production") + t.Setenv("SQUARE_ALLOW_REAL_API", "") + require.PanicsWithError(t, errDevRealAPIRequiresOverride.Error(), func() { + NewDevClient() + }, "a dev build must refuse SQUARE_ENVIRONMENT=production without SQUARE_ALLOW_REAL_API=1") + + // The explicit override is the opt-in that lets a dev build route to the + // real production API. + t.Setenv("SQUARE_ALLOW_REAL_API", "1") + client := NewDevClient() + require.IsType(t, &devProdClient{}, client, "SQUARE_ALLOW_REAL_API=1 must allow the dev build to route to the real production API") +} + +// TestDevClient_SandboxEnv_RoutesToRealClient locks the sandbox routing +// banner: a dev build may route to the Square SANDBOX (no real money), but +// only with a loud banner so the sandbox is never mistaken for the mock. +func TestDevClient_SandboxEnv_RoutesToRealClient(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", "sandbox") + t.Setenv("SQUARE_ALLOW_REAL_API", "") + + var buf bytes.Buffer + log.SetOutput(&buf) + defer log.SetOutput(os.Stderr) + + client := NewDevClient() + require.IsType(t, &devProdClient{}, client, "a dev build may route to the Square sandbox (no real money)") + logs := buf.String() + assert.Contains(t, logs, "SANDBOX", "routing a dev build to the sandbox must log a loud banner") + assert.Contains(t, logs, squareSandboxURL, "the banner must name the sandbox endpoint, not a mock") +} + +// TestDevClient_CreatePayment_FailAfterCommit locks the FailAfterCommit +// fault-injection: CreatePayment COMMITS the charge (retaining key + source in +// the ledgers exactly like a successful charge) and THEN returns a 5xx-style +// error — the "charged but response lost" prod scenario. A same-key + +// same-source retry must dedup to the committed payment, never issue a second +// charge. +func TestDevClient_CreatePayment_FailAfterCommit_ErrorThenSameKeyRetryDedups(t *testing.T) { + client := NewDevClient().(*MockClient) + ctx := context.Background() + + req := CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "cnon:test-card", + IdempotencyKey: "fail-after-commit-key", + ReferenceID: "booking-lost-response", + } + + client.FailAfterCommit = true + got, err := client.CreatePayment(ctx, req) + require.Error(t, err, "FailAfterCommit must return an error to the caller (the response was lost)") + assert.Nil(t, got) + assert.Contains(t, err.Error(), "503", "the lost-response error must read as a 5xx for ambiguous classification") + + // The charge was committed: the key + source are retained exactly like a + // successful charge, and the payment resolves by SquarePayID. + client.mu.RLock() + committed := client.paymentByKey["fail-after-commit-key"] + storedSource := client.paymentSource["fail-after-commit-key"] + client.mu.RUnlock() + require.NotNil(t, committed, "FailAfterCommit must COMMIT the charge under the idempotency key") + assert.Equal(t, "cnon:test-card", storedSource, "FailAfterCommit must retain the source under the key") + byID, err := client.GetPayment(ctx, committed.SquarePayID) + require.NoError(t, err) + assert.Equal(t, committed.ID, byID.ID, "the committed payment must be resolvable by SquarePayID") + + // A same-key + same-source retry dedups to the committed payment — the + // exact prod 503-retry semantics (no double charge). + client.FailAfterCommit = false + retry, err := client.CreatePayment(ctx, req) + require.NoError(t, err) + assert.Equal(t, committed.ID, retry.ID, "same-key retry must return the committed payment, not a second charge") + client.mu.RLock() + payCount := len(client.payments) + client.mu.RUnlock() + assert.Equal(t, 1, payCount, "FailAfterCommit + same-key retry must store exactly ONE charge") +} + +// TestDevClient_CreatePayment_RejectsOversizedIdempotencyKey locks the mock's +// 45-char idempotency-key cap: real Square rejects an over-length key for +// POST /v2/payments with a 400 VALUE_TOO_LONG, and the mock must mirror that +// structured rejection so dev parity catches a caller bug. +func TestDevClient_CreatePayment_RejectsOversizedIdempotencyKey(t *testing.T) { + client := NewDevClient().(*MockClient) + ctx := context.Background() + + longKey := strings.Repeat("k", 46) + result, err := client.CreatePayment(ctx, CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "cnon:test-card", + IdempotencyKey: longKey, + }) + require.Error(t, err, "an idempotency key over Square's 45-char limit must be rejected") + assert.Nil(t, result) + assert.Equal(t, "VALUE_TOO_LONG", ErrorCode(err)) + assert.Equal(t, http.StatusBadRequest, ErrorStatusCode(err)) + + // A 45-char key is the boundary and must be accepted. + ok, err := client.CreatePayment(ctx, CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "cnon:test-card", + IdempotencyKey: strings.Repeat("k", 45), + }) + require.NoError(t, err) + assert.Equal(t, "COMPLETED", ok.Status) +} + +// TestDevClient_CreateCardOnFile_SimulateCardTokenUsed locks the CARD_TOKEN_USED +// simulation: when SimulateCardTokenUsed is enabled, a card token (cnon: nonce) +// reused after a previous save is rejected with Square's structured 400 +// CARD_TOKEN_USED error. Off by default (dev/test flows reuse plain test +// tokens across requests), so the toggle must not reject reuse when disabled. +func TestDevClient_CreateCardOnFile_SimulateCardTokenUsed(t *testing.T) { + client := NewDevClient().(*MockClient) + client.SimulateCardTokenUsed = true + ctx := context.Background() + + card, err := client.CreateCardOnFile(ctx, "user-token-used", "cnon:single-use-nonce", "cus_test123") + require.NoError(t, err) + assert.NotEmpty(t, card.ID) + + // Reusing the same token → Square's CARD_TOKEN_USED rejection. + _, err = client.CreateCardOnFile(ctx, "user-token-used-2", "cnon:single-use-nonce", "cus_test123") + require.Error(t, err) + assert.Equal(t, "CARD_TOKEN_USED", ErrorCode(err)) + assert.Equal(t, http.StatusBadRequest, ErrorStatusCode(err)) + assert.ElementsMatch(t, []string{"cnon:single-use-nonce"}, client.UsedCardTokens()) + + // A fresh token still works. + fresh, err := client.CreateCardOnFile(ctx, "user-token-used-2", "cnon:fresh-nonce", "cus_test123") + require.NoError(t, err) + assert.NotEmpty(t, fresh.ID) + + // With the toggle OFF (default), reusing a token is allowed — dev/test + // flows reuse plain "cnon:test-card"-style tokens across requests. + client.SimulateCardTokenUsed = false + _, err = client.CreateCardOnFile(ctx, "user-token-reuse", "cnon:reused-token", "cus_test123") + require.NoError(t, err) + _, err = client.CreateCardOnFile(ctx, "user-token-reuse-2", "cnon:reused-token", "cus_test123") + require.NoError(t, err, "with SimulateCardTokenUsed off, token reuse must be allowed") +} + +// TestIdempotencyKeyLength_Parity_MockAndRealClientAgree asserts the mock and +// the real HTTP client AGREE on the over-length idempotency key rejection: +// both surface the same structured code (VALUE_TOO_LONG) and HTTP status (400). +func TestIdempotencyKeyLength_Parity_MockAndRealClientAgree(t *testing.T) { + ctx := context.Background() + longKey := strings.Repeat("k", 46) + + // Real client: Square's 400 VALUE_TOO_LONG response surfaces as a + // structured squareAPIError (the doJSON error-parsing path). + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"errors":[{"category":"INVALID_REQUEST_ERROR","code":"VALUE_TOO_LONG","detail":"idempotency_key too long"}]}`)) + })) + defer srv.Close() + hc := &httpClient{baseURL: srv.URL, token: "t", http: srv.Client()} + _, realErr := createPaymentHTTPWithClient(ctx, CreatePaymentReq{ + Amount: 5000, Currency: "GBP", SourceID: "cnon:test-card", IdempotencyKey: longKey, + }, hc) + require.Error(t, realErr) + + // Mock: rejects the same key client-side with the identical structured + // error (code + status), so dev parity holds. + mock := NewDevClient().(*MockClient) + _, mockErr := mock.CreatePayment(ctx, CreatePaymentReq{ + Amount: 5000, Currency: "GBP", SourceID: "cnon:test-card", IdempotencyKey: longKey, + }) + require.Error(t, mockErr) + + assert.Equal(t, "VALUE_TOO_LONG", ErrorCode(realErr)) + assert.Equal(t, ErrorCode(realErr), ErrorCode(mockErr), "mock and real client must agree on the error code for an over-length key") + assert.Equal(t, ErrorStatusCode(realErr), ErrorStatusCode(mockErr), "mock and real client must agree on the error status for an over-length key") + assert.Equal(t, http.StatusBadRequest, ErrorStatusCode(mockErr)) +} + +// TestCardTokenUsed_Parity_MockAndRealClientAgree asserts the mock and the +// real HTTP client AGREE on the reused-card-token rejection: both surface the +// same structured code (CARD_TOKEN_USED) and HTTP status (400). +func TestCardTokenUsed_Parity_MockAndRealClientAgree(t *testing.T) { + ctx := context.Background() + token := "cnon:reused-nonce" + + // Real client: Square's 400 CARD_TOKEN_USED response surfaces as a + // structured squareAPIError. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"errors":[{"category":"INVALID_REQUEST_ERROR","code":"CARD_TOKEN_USED","detail":"The card token has already been used."}]}`)) + })) + defer srv.Close() + hc := &httpClient{baseURL: srv.URL, token: "t", http: srv.Client()} + _, realErr := createCardOnFileHTTPWithClient(ctx, "user_1", token, "cus_1", hc) + require.Error(t, realErr) + + // Mock: with the simulation enabled, reusing a consumed token surfaces the + // identical structured error. + mock := NewDevClient().(*MockClient) + mock.SimulateCardTokenUsed = true + _, err := mock.CreateCardOnFile(ctx, "user_1", token, "cus_1") + require.NoError(t, err) + _, mockErr := mock.CreateCardOnFile(ctx, "user_2", token, "cus_1") + require.Error(t, mockErr) + + assert.Equal(t, "CARD_TOKEN_USED", ErrorCode(realErr)) + assert.Equal(t, ErrorCode(realErr), ErrorCode(mockErr), "mock and real client must agree on the error code for a reused card token") + assert.Equal(t, ErrorStatusCode(realErr), ErrorStatusCode(mockErr), "mock and real client must agree on the error status for a reused card token") + assert.Equal(t, http.StatusBadRequest, ErrorStatusCode(mockErr)) +} + +// TestEnvResolution_HelperMatchesHTTPClient locks the shared env-resolution +// contract (finding 4): the sweep and the charge path read SQUARE_ENVIRONMENT / +// SQUARE_LOCATION_ID through the SAME helpers the HTTP client uses, so the two +// deployables can never drift to independent env reads. +func TestEnvResolution_HelperMatchesHTTPClient(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", "sandbox") + t.Setenv("SQUARE_LOCATION_ID", "L_TEST_ENV") + + assert.Equal(t, "sandbox", SquareEnvironment()) + assert.Equal(t, "L_TEST_ENV", SquareLocationID()) + + // newHTTPClient derives base URL + location from the SAME helpers. + hc := newHTTPClient() + assert.Equal(t, squareSandboxURL, hc.baseURL, "sandbox env must resolve the sandbox base URL") + assert.Equal(t, "L_TEST_ENV", hc.locationID, "the HTTP client must read the location through SquareLocationID") + + // Production resolves the production base URL; anything else resolves the + // sandbox base URL — never the production URL. + t.Setenv("SQUARE_ENVIRONMENT", "production") + assert.Equal(t, squareProductionURL, newHTTPClient().baseURL, "production env must resolve the production base URL") + + t.Setenv("SQUARE_ENVIRONMENT", "mock") + assert.Equal(t, squareSandboxURL, newHTTPClient().baseURL, "any non-production env resolves the sandbox base URL (never the production URL)") +} diff --git a/backend/internal/square/square_http_client.go b/backend/internal/square/square_http_client.go index 01bd6cf..c8c269d 100644 --- a/backend/internal/square/square_http_client.go +++ b/backend/internal/square/square_http_client.go @@ -60,8 +60,28 @@ type httpClient struct { http *http.Client } +// SquareEnvironment returns the resolved SQUARE_ENVIRONMENT value. It is the +// SINGLE code path by which this package reads which Square environment it +// talks to: newHTTPClient derives its base URL from it and the dev build's +// NewDevClient routes on it, so a dev mock vs real API decision is never a +// second, drifting env read. The payments sweep reads the same value through +// this helper so the sweep and the charge process share one environment source +// (the sweep env contract). +func SquareEnvironment() string { + return os.Getenv("SQUARE_ENVIRONMENT") +} + +// SquareLocationID returns the SQUARE_LOCATION_ID value newHTTPClient embeds +// in payment requests. Exported so the payments sweep resolves the location +// through the same code path as the charge process: a location drift between a +// charge and its replay would change the replay body and break Square's +// identical-body idempotency dedup (the sweep env contract). +func SquareLocationID() string { + return os.Getenv("SQUARE_LOCATION_ID") +} + func newHTTPClient() *httpClient { - env := os.Getenv("SQUARE_ENVIRONMENT") + env := SquareEnvironment() baseURL := squareSandboxURL if env == "production" { baseURL = squareProductionURL @@ -69,7 +89,7 @@ func newHTTPClient() *httpClient { return &httpClient{ baseURL: baseURL, token: os.Getenv("SQUARE_ACCESS_TOKEN"), - locationID: os.Getenv("SQUARE_LOCATION_ID"), + locationID: SquareLocationID(), deviceID: os.Getenv("SQUARE_TERMINAL_DEVICE_ID"), http: &http.Client{Timeout: defaultHTTPTimeout}, } @@ -797,11 +817,14 @@ func listRefundsHTTPWithClient(ctx context.Context, paymentID string, beginTime } path = base + "&cursor=" + url.QueryEscape(resp.Cursor) } - // 20 pages fetched and a cursor is still present — return what we - // collected rather than discarding partial results (the previous - // infinite-loop guard dropped everything and returned an error). - log.Printf("[SQUARE] list refunds exceeded 20 pages (infinite-loop guard) — returning partial results: %d refunds for %s", len(results), paymentID) - return results, nil + // 20 pages fetched and a cursor is still present — the infinite-loop + // guard. Returning the partial results would be silently wrong for the + // money-sensitive reconcile caller: a refund sitting in the truncated tail + // would look like "no COMPLETED refund exists", letting the sweep mark the + // rows failed and over-refund. Error instead — reconcileRefundAtSquare + // treats any error as "leave the rows pending, retry later", so no money + // decision is made on partial data. + return nil, fmt.Errorf("square: list refunds exceeded 20 pages (infinite-loop guard) — refusing partial results for payment %s", paymentID) } func createCardOnFileHTTP(ctx context.Context, userID, cardToken, customerID string) (*CardOnFile, error) { @@ -874,10 +897,14 @@ func getCardsOnFileHTTPWithClient(ctx context.Context, userID string, hc *httpCl path = "/v2/cards?reference_id=" + url.QueryEscape(userID) + "&cursor=" + url.QueryEscape(resp.Cursor) } if truncated { - // 20 pages fetched and a cursor is still present — return what we - // collected rather than discarding partial results (mirrors the - // listRefunds 20-page guard's behavior). - log.Printf("[SQUARE] list cards for %s exceeded 20 pages (infinite-loop guard) — returning partial results: %d cards", userID, len(cards)) + // 20 pages fetched and a cursor is still present — the infinite-loop + // guard. Unlike listRefunds (where partial data can drive an over-refund + // decision and therefore ERRORS), cards are deliberately returned as + // partial: GetCardsOnFile has no money-sensitive caller, and erroring + // would break a "show my cards" feature for a user with >500 saved + // cards. The correctness gap (oldest card silently missing) is accepted + // and surfaced loudly in the log so it is not a silent truncation. + log.Printf("[SQUARE] list cards for %s exceeded 20 pages (infinite-loop guard) — TRUNCATED: returning partial results: %d of 500+ cards", userID, len(cards)) } if cards == nil { cards = []CardOnFile{} diff --git a/backend/internal/square/square_http_client_test.go b/backend/internal/square/square_http_client_test.go index 3e79e56..27dde1c 100644 --- a/backend/internal/square/square_http_client_test.go +++ b/backend/internal/square/square_http_client_test.go @@ -736,10 +736,11 @@ func TestListRefundsHTTP_Pagination(t *testing.T) { } }) - t.Run("page_guard_returns_partial_results", func(t *testing.T) { - // The 20-page guard must not discard what was already collected: it - // logs a truncation warning and returns the partial results instead - // of failing the reconcile with an error. + t.Run("page_guard_errors_instead_of_partial", func(t *testing.T) { + // The 20-page guard must ERROR rather than return partial results: a + // refund in the truncated tail would otherwise look like "no COMPLETED + // refund exists", letting the reconcile mark rows failed and over-refund. + // The reconcile caller treats any error as "leave rows pending, retry". calls := 0 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { calls++ @@ -750,14 +751,17 @@ func TestListRefundsHTTP_Pagination(t *testing.T) { hc := &httpClient{baseURL: srv.URL, token: "t", http: srv.Client()} refunds, err := listRefundsHTTPWithClient(context.Background(), "pay_partial", time.Now(), hc) - if err != nil { - t.Fatalf("expected partial results (nil error), got %v", err) + if err == nil { + t.Fatalf("expected a truncation error after 20 pages, got %d partial refunds with nil error", len(refunds)) + } + if !strings.Contains(err.Error(), "exceeded 20 pages") { + t.Errorf("expected error to name the 20-page guard, got %v", err) } if calls != 20 { t.Errorf("expected exactly 20 HTTP calls before guard, got %d", calls) } - if len(refunds) != 20 { - t.Errorf("expected 20 refunds collected across pages (one per page), got %d", len(refunds)) + if len(refunds) != 0 { + t.Errorf("expected no partial results on truncation error, got %d", len(refunds)) } }) } @@ -909,6 +913,33 @@ func TestGetCardsOnFileHTTP_ReferenceIDFilter(t *testing.T) { } } +// TestGetCardsOnFileHTTP_PageGuard_ReturnsPartial verifies the 20-page guard +// keeps returning partial results (nil error) for card listing, unlike +// listRefunds which errors: GetCardsOnFile has no money-sensitive caller, and +// erroring would break a "show my cards" feature for a user with >500 saved +// cards. The truncation is surfaced in the log, not by an error. +func TestGetCardsOnFileHTTP_PageGuard_ReturnsPartial(t *testing.T) { + calls := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"cards":[{"id":"ccof_t","card_brand":"VISA","last_4":"4242","exp_month":12,"exp_year":2030,"fingerprint":"fp_t","reference_id":"user_big","enabled":true,"version":1,"created_at":"2026-07-31T00:00:00Z"}],"cursor":"next"}`)) + })) + defer srv.Close() + + hc := &httpClient{baseURL: srv.URL, token: "t", http: srv.Client()} + cards, err := getCardsOnFileHTTPWithClient(context.Background(), "user_big", hc) + if err != nil { + t.Fatalf("expected partial cards with nil error, got %v", err) + } + if calls != 20 { + t.Errorf("expected exactly 20 HTTP calls before guard, got %d", calls) + } + if len(cards) != 20 { + t.Errorf("expected 20 cards collected across pages (one per page), got %d", len(cards)) + } +} + // TestCreateCheckoutHTTP_TipSettings verifies AllowTipping is emitted as // checkout.device_options.tip_settings.allow_tipping (Square's wire shape for // enabling terminal tips) and omitted entirely when not set. diff --git a/backend/internal/square/types.go b/backend/internal/square/types.go index b55bc87..3414818 100644 --- a/backend/internal/square/types.go +++ b/backend/internal/square/types.go @@ -40,6 +40,13 @@ type CreatePaymentReq struct { IdempotencyKey string ReferenceID string // booking ID or other reference Note string + // Autocomplete and TipMoney are valid Square wire fields that are + // intentionally NOT populated by any current handler: online payments are + // completed immediately (Autocomplete nil = Square default true, no + // approve-then-capture) and tips are handled locally as separate tip + // payments rather than split inside Square's CreatePayment (TipMoney nil). + // They are wired through the client into the request body for completeness + // and future use — do not remove them. Autocomplete *bool // nil (default) = true — complete immediately; false = approve only TipMoney *int64 // optional tip amount in pence CustomerID string // Square customer ID for card-on-file payments diff --git a/frontend/src/lib/components/booking/BookingFlow.svelte b/frontend/src/lib/components/booking/BookingFlow.svelte index bbb3019..42f0926 100644 --- a/frontend/src/lib/components/booking/BookingFlow.svelte +++ b/frontend/src/lib/components/booking/BookingFlow.svelte @@ -322,7 +322,14 @@ if (twoFactorBlocksSavedCards && selectedPaymentMethod) { selectedPaymentMethod = ''; } - await submitAndProceed(); + // Create the booking only if one does not already exist. A retry after + // a failed deposit charge (or a lost response) must NOT re-create a + // booking — the existing confirmedBooking is the one to charge, and + // the backend enforces one-active-booking for deposit-required users, + // so a second submission would 409 and orphan an unpaid booking. + if (!confirmedBooking) { + await submitAndProceed(); + } if (!confirmedBooking) { toast.error('Booking was not created. Please try again.'); return; @@ -2158,7 +2165,7 @@ {#if currentStep === finalStep} - {#if confirmedBooking} + {#if confirmedBooking && (depositPaid || !depositRequired || confirmedBooking.deposit_paid)} {@const isRequested = confirmedBooking.notes && confirmedBooking.notes.length > 0} {@const bookingDate = parseWallClockDate(confirmedBooking.start_time)} {@const dateStr = bookingDate.toLocaleDateString('en-GB', { diff --git a/frontend/src/routes/cancellation-policy/+page.svelte b/frontend/src/routes/cancellation-policy/+page.svelte index 5c82184..b840735 100644 --- a/frontend/src/routes/cancellation-policy/+page.svelte +++ b/frontend/src/routes/cancellation-policy/+page.svelte @@ -245,6 +245,20 @@ standard 14-day statutory cancellation "cooling-off" period under the Consumer Contracts Regulations 2013 does not apply to online bookings scheduled for a specific date or time.

+

+ If you believe your statutory consumer rights have not been met, you can get free, + impartial advice from + consumeradvice.scot + (advice.scot). If that does not resolve the issue, you can escalate your complaint to your + local Trading Standards office. Claims up to £5,000 can also be pursued through the + Scottish courts' Simple Procedure. +

We recognize that genuine emergencies, sudden severe illness, or bereavement can occur. Our management team retains complete administrative system access to waive cancellation fees, diff --git a/frontend/src/routes/terms/+page.svelte b/frontend/src/routes/terms/+page.svelte index ff5f2ae..aec5d01 100644 --- a/frontend/src/routes/terms/+page.svelte +++ b/frontend/src/routes/terms/+page.svelte @@ -227,6 +227,17 @@ GDPR Storage Limitation: 2 years of inactivity for accounts with no balance. +

+ If you have a consumer dispute that you cannot resolve with us, you can get free, + impartial advice from + consumeradvice.scot. +

Questions about these Terms? Please use our official