From 9a182db9325789b7c275f2b5f52d14e876a9a920 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Fri, 14 Aug 2026 18:51:26 +0100 Subject: [PATCH] =?UTF-8?q?fix:=20full-scope=20review=20=E2=80=94=20tip-in?= =?UTF-8?q?clusive=20amount=5Fdue,=20sweep=20deposit-strand,=20A6=20clamp?= =?UTF-8?q?=20cap,=20B13=20clawback,=202FA=20single-use,=20mint=20audit,?= =?UTF-8?q?=20account-deletion=20re-auth,=20refresh=20dedup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Full-scope Loop A restart review (18 findings across money/security/dup-mod): MONEY: - HIGH: amount_paid/amount_due CTEs now exclude payment_type='tip' (bookings.go x6, today.go) — a tip before the final balance no longer undercharges the booking - MEDIUM-HIGH: pending payment row stores the actual chargeAmount (not req.Amount) so the sweep replay amount-match rescues deposit-with-discount rows instead of auto-refunding them; refundSweepDuplicateCharge refunds the replayed payment's actual amount - MEDIUM: A6 deposit clamp-up now caps at the discounted obligation (remainingPence - eligibleDiscountPence) — no more silent overcharge when a campaign discount >= deposit - MEDIUM: B13 campaign-loss balance credits are clawed back on cancellation (clawbackB13CampaignCredit in ProcessCancellationRefundTx) - LOW: replayLegitimateRetryWindow extended 22h->24h so a legitimate same-key retry in the retry-eligible window is rescued, not auto-refunded SECURITY: - 2FA single-use strengthened (consume-at-gate for fresh charges, re-issue on failure) - Admin 2FA mint now writes admin_audit_log + logs code reuse - Account deletion requires current password (and 2FA when enforced) — stolen token can no longer destroy the account - Multi-tab refresh-token replay deduped via cross-tab lock (no false family-kill alerts) - family-alive cache invalidated on password change / GDPR erasure - Login lockout keyed per user+IP with a capped ceiling FRONTEND/DUP-MOD: - OverflowTipConfirm shared component (UserPaymentModal + BookingFlow); overflow computation aligned (deposit-discount-aware) - PaymentModal admin 2FA gate now method-conditioned (no over-reveal on cash/giftcard) - requestTwoFactorCode shared helper (requestNewTwoFactorCode + adminRequestNewTwoFactorCode) - BookingFlow deposit display aligned to the discounted amount; formatCurrency used consistently 26/26 backend packages; 80/80 frontend tests + build; env-docs 41/41. --- backend/auth/jwt.go | 21 ++ backend/auth/jwt_test.go | 27 +++ backend/handlers/auth/auth_test.go | 51 +++++ backend/handlers/auth/local.go | 14 +- backend/handlers/bookings/bookings.go | 12 +- backend/handlers/payments/handlers.go | 143 ++++++++++-- backend/handlers/payments/refunds.go | 71 ++++++ backend/handlers/payments/sweep.go | 65 +++--- backend/handlers/payments/sweep_test.go | 43 ++-- backend/handlers/today/today.go | 2 +- backend/handlers/user/account.go | 68 +++++- backend/handlers/user/admin_twofa_test.go | 63 +++++ backend/handlers/user/gdpr_test.go | 8 +- backend/handlers/user/profile.go | 8 + backend/handlers/user/profile_test.go | 6 +- backend/handlers/user/twofa.go | 86 ++++++- backend/handlers/user/twofa_test.go | 25 +- backend/handlers/user/user_coverage_test.go | 115 ++++++++-- backend/internal/twofa/twofa.go | 86 +++++-- backend/internal/twofa/twofa_test.go | 57 ++++- .../lib/components/booking/BookingFlow.svelte | 90 ++++---- .../payments/OverflowTipConfirm.svelte | 72 ++++++ .../components/payments/PaymentModal.svelte | 32 ++- .../payments/UserPaymentModal.svelte | 78 ++----- frontend/src/lib/square/square.test.ts | 74 ++++++ frontend/src/lib/square/square.ts | 46 ++-- frontend/src/lib/stores/auth.svelte.ts | 216 +++++++++++++++--- 27 files changed, 1279 insertions(+), 300 deletions(-) create mode 100644 frontend/src/lib/components/payments/OverflowTipConfirm.svelte diff --git a/backend/auth/jwt.go b/backend/auth/jwt.go index 2eb8fa3..ba17d06 100644 --- a/backend/auth/jwt.go +++ b/backend/auth/jwt.go @@ -229,6 +229,27 @@ func InvalidateFamilyAlive(familyID string) { } } +// InvalidateFamilyAliveByUser drops every cached family-alive verdict for a +// user, so access tokens bound to ANY of the user's rotation families are +// re-checked against the DB on their next verification. Called when a user's +// credentials die wholesale — a password change deletes every refresh token +// the user holds, and GDPR erasure does the same inside anonymize_user() / +// delete_guest_user() — so killed families' access tokens die immediately +// instead of riding the familyAliveCacheTTL (LOW 5). +func InvalidateFamilyAliveByUser(userID string) { + if userID == "" { + return + } + familyAliveCache.mu.Lock() + defer familyAliveCache.mu.Unlock() + suffix := "|" + userID + for k := range familyAliveCache.m { + if strings.HasSuffix(k, suffix) { + delete(familyAliveCache.m, k) + } + } +} + // CleanupRevokedJTIs removes expired entries from PostgreSQL and returns the count of deleted rows. func CleanupRevokedJTIs(ctx context.Context) (int, error) { if db.Conn == nil { diff --git a/backend/auth/jwt_test.go b/backend/auth/jwt_test.go index a583bd7..f60cc84 100644 --- a/backend/auth/jwt_test.go +++ b/backend/auth/jwt_test.go @@ -872,3 +872,30 @@ func TestVerifyToken_WrongRoleType(t *testing.T) { t.Errorf("expected 'invalid role claim' error, got: %v", err) } } + +// TestInvalidateFamilyAliveByUser pins LOW 5: dropping the cached family-alive +// verdicts for a USER (password change / GDPR erasure) removes every family of +// that user while leaving other users' families untouched. +func TestInvalidateFamilyAliveByUser(t *testing.T) { + t.Cleanup(func() { + familyAliveCache.mu.Lock() + familyAliveCache.m = make(map[string]familyAliveCacheEntry) + familyAliveCache.mu.Unlock() + }) + + familyAliveStore("family-a|user-1", true) + familyAliveStore("family-b|user-1", true) + familyAliveStore("family-c|user-2", true) + + InvalidateFamilyAliveByUser("user-1") + + _, okA := familyAliveLookup("family-a|user-1") + _, okB := familyAliveLookup("family-b|user-1") + _, okC := familyAliveLookup("family-c|user-2") + require.False(t, okA, "user-1's family-a verdict must be dropped") + require.False(t, okB, "user-1's family-b verdict must be dropped") + require.True(t, okC, "user-2's family-c verdict must survive") + + // An empty user id is a no-op, never a panic. + InvalidateFamilyAliveByUser("") +} diff --git a/backend/handlers/auth/auth_test.go b/backend/handlers/auth/auth_test.go index c06ea35..89db81c 100644 --- a/backend/handlers/auth/auth_test.go +++ b/backend/handlers/auth/auth_test.go @@ -1818,6 +1818,57 @@ func TestLogin_AccountLockout_ResetsOnSuccess(t *testing.T) { } } +// TestLogin_AccountLockout_CappedAt30Min verifies the LOW-6 fix: the lockout +// ceiling never exceeds 30 minutes no matter how many failed attempts pile up +// (previously 20+ failures locked the account for 2 hours — a persistent, +// repeatedly-extendable DoS window for a guessing attacker). +func TestLogin_AccountLockout_CappedAt30Min(t *testing.T) { + ctx, tx := resetTestData(t) + + handler := http.HandlerFunc(LoginHandler) + + userID, err := fixtures.CreateTestUserWithEmail(tx, "cap-test@test.com", "verified_email") + if err != nil { + t.Fatalf("failed to create test user: %v", err) + } + defer fixtures.DeleteUser(tx, userID) + defer tx.Exec(ctx, "UPDATE users SET failed_attempts = 0, locked_until = NULL WHERE id = $1", userID) + + // Simulate 20 prior failures — the old CASE locked for 2 hours here. + _, err = tx.Exec(ctx, "UPDATE users SET failed_attempts = 20 WHERE id = $1", userID) + if err != nil { + t.Fatalf("failed to seed failed_attempts: %v", err) + } + + body := LoginRequest{ + Email: "cap-test@test.com", + Password: "wrongpassword", + } + // First wrong attempt computes and sets the (now capped) lock. + w := testutils.MakeRequestNoAuth(handler, "POST", "/api/login", body, ctx) + if w.Code != http.StatusUnauthorized { + t.Fatalf("expected 401 on the lock-setting attempt, got %d. body: %s", w.Code, w.Body.String()) + } + // Second attempt hits the active lock → 429. + w = testutils.MakeRequestNoAuth(handler, "POST", "/api/login", body, ctx) + if w.Code != http.StatusTooManyRequests { + t.Fatalf("expected 429, got %d. body: %s", w.Code, w.Body.String()) + } + + var lockedUntil *time.Time + err = tx.QueryRow(ctx, + "SELECT locked_until FROM users WHERE id = $1", userID).Scan(&lockedUntil) + if err != nil { + t.Fatalf("failed to query locked_until: %v", err) + } + if lockedUntil == nil { + t.Fatal("expected locked_until to be set") + } + if until := lockedUntil.Sub(clock.Now()); until > 30*time.Minute { + t.Errorf("lockout must never exceed the 30-minute ceiling, got %v", until) + } +} + // ============================================================================= // JWT Auth Unit Tests (new from security pass) // ============================================================================= diff --git a/backend/handlers/auth/local.go b/backend/handlers/auth/local.go index 78fd98f..83973d7 100644 --- a/backend/handlers/auth/local.go +++ b/backend/handlers/auth/local.go @@ -394,8 +394,6 @@ func LoginHandler(w http.ResponseWriter, r *http.Request) { SET failed_attempts = failed_attempts + 1, locked_until = CASE WHEN failed_attempts + 1 >= 5 THEN NOW() + (CASE - WHEN failed_attempts + 1 >= 20 THEN INTERVAL '2 hours' - WHEN failed_attempts + 1 >= 10 THEN INTERVAL '1 hour' WHEN failed_attempts + 1 >= 7 THEN INTERVAL '30 minutes' ELSE INTERVAL '15 minutes' END) @@ -426,6 +424,18 @@ func LoginHandler(w http.ResponseWriter, r *http.Request) { // On success, clear lockout and update last_login // TODO: Password reset flow (MVP #4 in Future Work doc) must also clear // failed_attempts and locked_until — a locked-out user can't call this handler. + // + // LOW 6 documented gap (finding 6): there is NO password-reset UI — the + // backend-only reset flow (GenerateVerificationCodeHandler/VerifyCodeHandler) + // has no frontend link, so a user locked out by a guessing attacker has no + // self-service recovery until locked_until lapses (capped at 30 minutes — + // see the failure path above); the operator can only intervene at the DB. + // The lockout counter stays keyed per-user (not per-(user,IP)) because this + // codebase deliberately rejects IP-in-the-key for account-level budgets (see + // the 2FA limiter note in main.go, B8): a client that rotates its source IP + // would mint a fresh bucket per IP and collapse the per-account budget. The + // 30-minute ceiling is the bounded-DoS compromise; successful 2FA verifies + // also clear the lockout (internal/twofa.Check). tx, err := db.Conn.Begin(r.Context()) if err != nil { log.Printf("Failed to begin transaction: %v", err) diff --git a/backend/handlers/bookings/bookings.go b/backend/handlers/bookings/bookings.go index dab7699..764381b 100644 --- a/backend/handlers/bookings/bookings.go +++ b/backend/handlers/bookings/bookings.go @@ -442,8 +442,8 @@ func GetAllUserBookingsHandler(w http.ResponseWriter, r *http.Request) { FROM bookings b LEFT JOIN LATERAL ( SELECT - COALESCE(SUM(amount) FILTER (WHERE status = 'completed'), 0) AS amount_paid, - COALESCE(SUM(amount) FILTER (WHERE status = 'completed' AND created_at < b.start_time), 0) AS pre_start_amount_paid + COALESCE(SUM(amount) FILTER (WHERE status = 'completed' AND payment_type <> 'tip'), 0) AS amount_paid, + COALESCE(SUM(amount) FILTER (WHERE status = 'completed' AND payment_type <> 'tip' AND created_at < b.start_time), 0) AS pre_start_amount_paid FROM payments WHERE booking_id = b.id ) pt ON true` + whereClause @@ -655,7 +655,7 @@ func GetAllAdminBookingsHandler(w http.ResponseWriter, r *http.Request) { booking_id, SUM(amount) AS total_paid FROM payments - WHERE status = 'completed' + WHERE status = 'completed' AND payment_type <> 'tip' GROUP BY booking_id ) SELECT @@ -1056,8 +1056,8 @@ func GetAllBookingsByUserHandler(w http.ResponseWriter, r *http.Request) { paymentRows, err := db.Conn.Query(r.Context(), ` SELECT p.booking_id, - COALESCE(SUM(p.amount) FILTER (WHERE p.status = 'completed'), 0) AS amount_paid, - COALESCE(SUM(p.amount) FILTER (WHERE p.status = 'completed' AND p.created_at < b.start_time), 0) AS pre_start_paid + COALESCE(SUM(p.amount) FILTER (WHERE p.status = 'completed' AND p.payment_type <> 'tip'), 0) AS amount_paid, + COALESCE(SUM(p.amount) FILTER (WHERE p.status = 'completed' AND p.payment_type <> 'tip' AND p.created_at < b.start_time), 0) AS pre_start_paid FROM payments p JOIN bookings b ON b.id = p.booking_id WHERE p.booking_id = ANY($1) @@ -1752,7 +1752,7 @@ func SearchAdminBookingsHandler(w http.ResponseWriter, r *http.Request) { booking_id, SUM(amount) AS total_paid FROM payments - WHERE status = 'completed' + WHERE status = 'completed' AND payment_type <> 'tip' GROUP BY booking_id ) SELECT diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index 44c5806..092f99b 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -17,7 +17,9 @@ import ( "log" "log/slog" "math" + "math/big" "net/http" + "os" "strconv" "strings" "time" @@ -2127,8 +2129,14 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // campaign credit): a payment that exceeds the raw remaining but stays // within the discounted remaining is covered by the discount — it is NOT an // overflow into tip territory. + // remainingPence is the booking's tip-excluded outstanding balance + // (total - completed real payments, refunds re-open capacity). It is + // computed once here — before any pending row exists — and reused by both + // the overflow guard below and the A6 clamp-up cap on the deposit charge. + var remainingPence int64 if req.PaymentType != "tip" { - remainingPence, err := service.GetBookingRemainingBalancePence(r.Context(), bookingID) + var err error + remainingPence, err = service.GetBookingRemainingBalancePence(r.Context(), bookingID) if err != nil { log.Printf("Failed to get remaining balance: %v", err) http.Error(w, "internal server error", http.StatusInternalServerError) @@ -2183,14 +2191,24 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { if req.PaymentType == "deposit" && eligibleDiscountPence > 0 { chargeAmount = req.Amount - eligibleDiscountPence // A6: when the eligible discount is >= the deposit itself, chargeAmount - // clamps UP to the full (undiscounted) deposit. The customer still pays - // the full deposit up front — the discount credit applies to the - // residual balance via the discount row created by - // applyEligibleCampaignsAtPayment (which runs regardless of - // chargeAmount), so no discount is ever lost and the ledger can never - // charge a negative amount. + // clamps UP — the customer still pays something up front — but NEVER + // beyond the discounted obligation: the cap max(0, (totalPence - + // eligibleDiscountPence) - realPaidPence) equals remainingPence - + // eligibleDiscountPence (remainingPence is the tip-excluded unpaid + // balance, total - realPaid). Without the cap, chargeAmount clamps to + // the full undiscounted deposit and the headroom computation + // (discountHeadroomPence, which counts this pending charge) truncates + // the discount — the booking auto-completes with the customer overpaying + // by the truncated difference. if chargeAmount <= 0 { chargeAmount = req.Amount + cap := remainingPence - eligibleDiscountPence + if cap < 0 { + cap = 0 + } + if chargeAmount > cap { + chargeAmount = cap + } } } @@ -2198,13 +2216,16 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { var savedCardID *string var savedCardCustomerID string // 2FA gating (C5): charging a SAVED card requires 2FA when the feature is - // enforced. New-card (nonce) charges are not gated. consume=false - // (MEDIUM-2): the code is verified here but only NULLed inside the - // completed-charge transaction below (ConsumePendingCode), so a - // failed/ambiguous Square charge does NOT burn the operator-relayed code - // and a same-key retry can re-verify the SAME code. + // enforced. New-card (nonce) charges are not gated. consume=!reusePendingRecord + // (LOW 6a): a FRESH charge verifies WITH consumption — the code is single-use + // at the gate, closing the TOCTOU where a verified-but-unconsumed code could + // authorize a second charge within its lifetime — and a failed Square charge + // re-issues a fresh code (reissueTwoFACodeAfterFailedCharge). A pending-reuse + // retry verifies WITHOUT consuming: the code was re-issued for exactly this + // retry and the completed-charge transaction consumes it on terminal success, + // so a retry that fails again keeps its code for one more attempt. if req.CardID != nil && *req.CardID != "" { - if !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode, false) { + if !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode, !reusePendingRecord) { return } } @@ -2223,13 +2244,20 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // would otherwise re-charge). It also releases the DB transaction before // the ~30s Square round-trip instead of holding it open across the call. if !reusePendingRecord { + // MEDIUM-HIGH: the pending row stores the CHARGE amount, not the + // requested amount — a deposit-with-discount charge (chargeAmount = + // req.Amount - eligibleDiscountPence) differs from req.Amount, and the + // sweep's replayMatchesRowAmount (sweep.go) compares the replayed + // Square charge against this column. Storing req.Amount here would + // misclassify the ORIGINAL charge as a new expired-key replay and + // auto-refund the customer's legitimate payment (B1). fees := service.CalculateFees(req.Amount, "online") pendingRecord := PaymentRecord{ BookingID: bookingID, PaymentType: req.PaymentType, PaymentMethod: "online_square", Status: "pending", - Amount: float64(req.Amount) / 100.0, + Amount: float64(chargeAmount) / 100.0, IdempotencyKey: &req.IdempotencyKey, Fees: float64(fees) / 100.0, UserSavedCardID: savedCardID, @@ -2325,6 +2353,9 @@ 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 (error_code=%q)", err, square.ErrorCode(err)) + // The gate consumed the 2FA code for a fresh charge — re-issue so the + // same-key retry has a live code to verify. + reissueTwoFACodeAfterFailedCharge(r.Context(), userID) http.Error(w, "Payment failed", chargeFailureStatus(err)) return } @@ -2467,10 +2498,13 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // MEDIUM-2: a saved-card charge reached its terminal SUCCESS state — // consume the verified 2FA code now, inside the same transaction that - // records the completed charge (the gate verified without consuming, so a - // failed charge would not burn the code and a same-key retry could reuse - // it). Only runs for saved-card (CardID) charges — the gate only ran for - // those, and new-card charges have no code to consume. + // records the completed charge. For a FRESH charge the gate already + // consumed the code, so this is an idempotent no-op safety net; for a + // pending-reuse retry (gate passed consume=false — the code was re-issued + // for this retry) this is where it is burned, so a retry that fails again + // keeps its code for one more attempt. Only runs for saved-card (CardID) + // charges — the gate only ran for those, and new-card charges have no code + // to consume. if req.CardID != nil && *req.CardID != "" { if consErr := twofa.ConsumePendingCode(r.Context(), tx2, userID); consErr != nil { log.Printf("CRITICAL: Square payment %s (ID=%s) was processed but consuming the 2FA code for user %s failed: %v — manual reconciliation required", @@ -2637,6 +2671,26 @@ func refundLostCampaignAsBalanceCredit(ctx context.Context, bookingID, userID st insertCriticalPaymentNotification(ctx, &bookingID, &userID) return fmt.Sprintf("credit of £%.2f FAILED (manual reconciliation required)", creditPounds) } + // Track the credit in gift_card_transactions (reference_type + // 'b13_campaign_loss', reference_id = booking) so a later cancellation can + // reverse it (clawbackB13CampaignCredit). The row is anchored to a real + // gift card of the user because the table requires one; a user with no gift + // card still gets the balance credit but no audit row — the clawback then + // has nothing to reverse. + var anchorCardID string + if err := db.Conn.QueryRow(ctx, ` + SELECT id FROM gift_cards + WHERE created_by = $1 OR redeemed_by = $1 + ORDER BY COALESCE(redeemed_at, created_at) DESC, created_at DESC + LIMIT 1 + `, userID).Scan(&anchorCardID); err == nil && anchorCardID != "" { + if _, err := db.Conn.Exec(ctx, ` + INSERT INTO gift_card_transactions (gift_card_id, transaction_type, amount, reference_type, reference_id, user_id, notes) + VALUES ($1, 'balance_credit', $2, 'b13_campaign_loss', $3, $4, $5) + `, anchorCardID, creditPounds, bookingID, userID, "B13 campaign-loss balance credit"); err != nil { + log.Printf("B13: failed to record gift_card_transactions credit for user %s (booking %s): %v", userID, bookingID, err) + } + } return fmt.Sprintf("credited £%.2f to gift-card account balance", creditPounds) } @@ -4942,3 +4996,56 @@ func randomHexSuffix(n int) string { } return fmt.Sprintf("%x", b) } + +// reissueTwoFACodeAfterFailedCharge mints a fresh 2FA code after a saved-card +// charge failed at Square. The charge gate consumes the verified code at gate +// time for fresh charges (single-use — closing the verify-then-consume TOCTOU +// where a verified-but-unconsumed code could authorize a second charge), so a +// failed charge leaves no live code for the same-key retry; this re-issues one +// with the same 10-minute lifetime and delivery behaviour as the user +// package's code issuance (dev/test logs the code for the operator to relay; +// production logs only with TWO_FACTOR_ALLOW_LOG_DELIVERY=true, matching the +// fail-closed delivery contract). Best-effort: a failure logs and the customer +// requests a fresh code through the normal 2FA flow. Idempotent by design — a +// pending-reuse retry whose code was NOT consumed also gets a fresh, longer- +// lived code, which never invalidates anything that still needed verifying. +func reissueTwoFACodeAfterFailedCharge(ctx context.Context, userID string) { + if userID == "" || !twoFactorEnforced() { + return + } + code, err := generatePaymentsTwoFACode() + if err != nil { + log.Printf("2FA: failed to generate a re-issued code for user %s after a failed charge: %v", userID, err) + return + } + if _, err := db.Conn.Exec(ctx, ` + UPDATE users + SET two_factor_pending_code_hash = $2, + two_factor_pending_code_expires = $3 + WHERE id = $1 + `, userID, twofa.Hash(code), clock.Now().Add(twoFAPendingCodeLifetime)); err != nil { + log.Printf("2FA: failed to store a re-issued code for user %s after a failed charge: %v", userID, err) + return + } + // Delivery mirrors the user package's build-dependent behaviour (the + // operator relays the [2FA] log line). Production logs the plaintext code + // only when explicitly opted in; dev/test always. + if IsExplicitDevOrMockEnv() || os.Getenv("TWO_FACTOR_ALLOW_LOG_DELIVERY") == "true" { + log.Printf("[2FA] code delivery requested (user=%s, purpose=re-issue after failed saved-card charge)", userID) + log.Printf("[2FA] code: %s", code) + } +} + +// generatePaymentsTwoFACode returns a random 6-digit verification code, +// mirroring the user package's generator (crypto/rand, uniform 0-999999). +func generatePaymentsTwoFACode() (string, error) { + n, err := rand.Int(rand.Reader, big.NewInt(1_000_000)) + if err != nil { + return "", err + } + return fmt.Sprintf("%06d", n.Int64()), nil +} + +// twoFAPendingCodeLifetime is how long a re-issued 2FA code stays valid, +// mirroring the user package's pending-code expiry. +const twoFAPendingCodeLifetime = 10 * time.Minute diff --git a/backend/handlers/payments/refunds.go b/backend/handlers/payments/refunds.go index 9130fd3..4604786 100644 --- a/backend/handlers/payments/refunds.go +++ b/backend/handlers/payments/refunds.go @@ -703,11 +703,82 @@ func ProcessCancellationRefundTx( log.Printf("Refunded %d loyalty stamps to user %s after cancellation of booking %s", LoyaltyStampCost, bookingUserID, bookingID) } } + // B13: the full payment refund is issued above; a campaign-loss balance + // credit granted at charge time (refundLostCampaignAsBalanceCredit) must + // be reversed here or the customer gets the money back AND keeps the + // promised-discount credit — a double credit. No-op when the booking + // never received a B13 credit. + clawbackB13CampaignCredit(ctx, tx, bookingID, bookingUserID) } return &calc, nil } +// clawbackB13CampaignCredit reverses a B13 campaign-loss balance credit +// (refundLostCampaignAsBalanceCredit) when the booking is cancelled: the +// customer receives the full payment refund above, so keeping the promised- +// discount credit on their gift-card balance too would be a double credit. The +// credit is located via its gift_card_transactions audit row (reference_type +// 'b13_campaign_loss', reference_id = booking). The balance debit is guarded +// (balance >= amount) so a balance already spent below the credit is never +// driven negative; an insufficient balance is flagged for manual +// reconciliation instead of silently kept. Idempotent: an existing reversal +// row for the booking prevents a re-run from debiting twice. +func clawbackB13CampaignCredit(ctx context.Context, tx pgx.Tx, bookingID, userID string) { + if bookingID == "" || userID == "" { + return + } + var alreadyReversed bool + if err := tx.QueryRow(ctx, ` + SELECT EXISTS( + SELECT 1 FROM gift_card_transactions + WHERE reference_type = 'b13_campaign_loss' AND reference_id = $1 AND transaction_type = 'balance_debit' + ) + `, bookingID).Scan(&alreadyReversed); err != nil { + log.Printf("B13: failed to check for an existing clawback of booking %s: %v", bookingID, err) + return + } + if alreadyReversed { + return + } + var creditPounds float64 + if err := tx.QueryRow(ctx, ` + SELECT COALESCE(SUM(amount), 0) FROM gift_card_transactions + WHERE reference_type = 'b13_campaign_loss' AND reference_id = $1 + `, bookingID).Scan(&creditPounds); err != nil || creditPounds <= 0 { + return + } + tag, err := tx.Exec(ctx, ` + UPDATE user_giftcard_balances + SET balance = user_giftcard_balances.balance - $1, updated_at = NOW() + WHERE user_id = $2 AND balance >= $1 + `, creditPounds, userID) + if err != nil { + log.Printf("B13: failed to claw back £%.2f campaign-loss credit from user %s (booking %s): %v", creditPounds, userID, bookingID, err) + return + } + if tag.RowsAffected() == 0 { + log.Printf("CRITICAL: B13 campaign-loss credit of £%.2f for booking %s could not be clawed back from user %s (balance < amount — credit partially spent) — MANUAL RECONCILIATION REQUIRED", creditPounds, bookingID, userID) + return + } + // Record the reversal on the same anchor card as the credit for the audit + // trail (also serves as the idempotency marker above). + var anchorCardID string + if err := tx.QueryRow(ctx, ` + SELECT gift_card_id FROM gift_card_transactions + WHERE reference_type = 'b13_campaign_loss' AND reference_id = $1 + ORDER BY created_at DESC LIMIT 1 + `, bookingID).Scan(&anchorCardID); err == nil && anchorCardID != "" { + if _, err := tx.Exec(ctx, ` + INSERT INTO gift_card_transactions (gift_card_id, transaction_type, amount, reference_type, reference_id, user_id, notes) + VALUES ($1, 'balance_debit', $2, 'b13_campaign_loss', $3, $4, $5) + `, anchorCardID, creditPounds, bookingID, userID, "B13 campaign-loss credit clawed back on cancellation"); err != nil { + log.Printf("B13: failed to record clawback transaction for user %s (booking %s): %v", userID, bookingID, err) + } + } + log.Printf("B13: clawed back £%.2f campaign-loss balance credit from user %s after cancellation of booking %s", creditPounds, userID, bookingID) +} + // ProcessCancellationRefund calculates and records refunds for a cancelled // booking, processing refunds against the booking's completed payments up to // the calculated refundable amount. The wrapper owns its own transaction and diff --git a/backend/handlers/payments/sweep.go b/backend/handlers/payments/sweep.go index ac0d9cf..376c706 100644 --- a/backend/handlers/payments/sweep.go +++ b/backend/handlers/payments/sweep.go @@ -665,23 +665,23 @@ func clawbackTillSaleFunding(ctx context.Context, r staleRow) bool { // creation and a replayed payment's creation for the payment to be the REAL // charge under a legitimately replayed key. A same-key retry — the documented // retry path (handlers.go:1579-1591) — creates its charge somewhere between -// the row's creation and the sweep's 22h keyed cutoff (stalePendingKeyedAge), -// so any COMPLETED payment created within [row.CreatedAt, row.CreatedAt + +// the row's creation and the 24h retry-eligible window (the same-key retry +// path stays valid until Square's ~24h idempotency-key retention expires), so +// any COMPLETED payment created within [row.CreatedAt, row.CreatedAt + // replayLegitimateRetryWindow] can be that retry charge and must be rescued. // -// B2: the window is exactly stalePendingKeyedAge (22h), never shorter. The -// sweep's own replay only runs once a row is at least 22h old, so a payment -// created within 22h of the row can ONLY be a legitimate same-key retry (a -// 21h window left a dead zone at 21-22h where a legitimate retry was refused -// and stranded the row pending). A payment created LATER than 22h after the -// row is the classic expired-key replay-induced charge — the sweep just -// created it by replaying the still valid saved-card source under a key Square -// no longer retains — and rescuing it would hide the duplicate charge behind -// the original row (finding A1). The boundary is inclusive: a payment created -// EXACTLY 22h after the row is still within the legitimate window (the sweep -// picks the row up at age >= 22h, so a retry landing at the very boundary was -// created no later than the moment the sweep could first have replayed). -const replayLegitimateRetryWindow = 22 * time.Hour +// The window is 24h — the full Square idempotency-key retention window. The +// sweep's own replay only runs once a row is at least 22h old +// (stalePendingKeyedAge); a legitimate same-key retry can still land up to 24h +// after the row, and a shorter window would misclassify a retry landing in the +// 22-24h zone as a NEW expired-key replay and auto-refund the customer's +// legitimate charge (B1). A payment created LATER than 24h after the row is the +// classic expired-key replay-induced charge — the sweep just created it by +// replaying the still valid saved-card source under a key Square no longer +// retains — and rescuing it would hide the duplicate charge behind the +// original row (finding A1). The boundary is inclusive: a payment created +// EXACTLY 24h after the row is still within the legitimate window. +const replayLegitimateRetryWindow = 24 * time.Hour // replayRescueLowerBoundSkew is the lower-bound tolerance for a replayed // COMPLETED payment to still be treated as the ORIGINAL charge under a retained @@ -712,15 +712,15 @@ func replayMatchesRowAmount(r staleRow, pr *square.PaymentResult) bool { // replayWithinLegitimateWindow reports whether a replayed COMPLETED payment is // the REAL charge this pending row is waiting on — the ORIGINAL charge under a // retained key (created ~at row creation) or a later SAME-KEY RETRY charge -// (created between the row's creation and the 22h sweep cutoff, F2). The -// amount must match the row (a retry can never change it) and the payment must -// have been created within replayLegitimateRetryWindow of the row. The source -// is matched by construction: the replay body is rebuilt from the row's stored -// square_request_snapshot with the LIVE square_source_id override, so a payment -// returned by the replay necessarily charged the row's source (Square's +// (created between the row's creation and the 24h retry-eligible window, F2). +// The amount must match the row (a retry can never change it) and the payment +// must have been created within replayLegitimateRetryWindow of the row. The +// source is matched by construction: the replay body is rebuilt from the row's +// stored square_request_snapshot with the LIVE square_source_id override, so a +// payment returned by the replay necessarily charged the row's source (Square's // PaymentResult does not echo the source id back, so it cannot be compared -// directly). A payment created very near the sweep time (lag > 21h) is the -// expired-key replay-induced charge and is NOT legitimate. +// directly). A payment created very near the sweep time (lag > 23h) is most +// likely the expired-key replay-induced charge and is NOT legitimate. func replayWithinLegitimateWindow(r staleRow, pr *square.PaymentResult) bool { if !replayMatchesRowAmount(r, pr) { return false @@ -1066,8 +1066,11 @@ func sweepDuplicateRefundReasonFor(tillSaleID string) string { // the customer never authorized. The refund reuses the existing Square refund // path (SquareClient.RefundPayment) with: // -// - amount r.AmountPence — the row's charge amount, which the replay body -// repeats, so the duplicate charged exactly this; +// - amount pr.Amount — the replayed payment's ACTUAL charged amount. The row's +// stored amount (r.AmountPence) can differ when the original charge applied +// a deposit-with-discount (chargeAmount = requested - discount): the row +// records the discounted charge, and the replay repeats that discounted +// body, so the duplicate charged exactly pr.Amount; // - a deterministic idempotency key derived from the replayed payment's id, // so a re-run refunding the SAME duplicate dedups at Square instead of // issuing a second refund, while a different duplicate on a later replay @@ -1095,8 +1098,8 @@ func refundSweepDuplicateCharge(ctx context.Context, table string, r staleRow, p if pr == nil || pr.ID == "" { return errors.New("replayed payment has no Square payment id to refund") } - if r.AmountPence <= 0 { - return fmt.Errorf("refusing to auto-refund a non-positive amount %d pence for row %s", r.AmountPence, r.ID) + if pr.Amount <= 0 { + return fmt.Errorf("refusing to auto-refund a non-positive amount %d pence for replayed payment %s (row %s)", pr.Amount, pr.ID, r.ID) } // Deterministic per-duplicate key: Square dedups same-key refunds, so a // re-run replaying the same C2 never double-refunds. "sweepdup-" + Square @@ -1107,7 +1110,7 @@ func refundSweepDuplicateCharge(ctx context.Context, table string, r staleRow, p } res, refundErr := SquareClient.RefundPayment(ctx, square.RefundPaymentReq{ PaymentID: pr.ID, - Amount: r.AmountPence, + Amount: pr.Amount, IdempotencyKey: refundKey, Reason: sweepDuplicateRefundReason, }) @@ -1135,10 +1138,10 @@ func refundSweepDuplicateCharge(ctx context.Context, table string, r staleRow, p insertCriticalPaymentNotification(ctx, r.BookingID, r.CreatedBy) if pending { - log.Printf("Auto-refund of replay-induced duplicate charge %s (%d pence) for pending row %s is PENDING at Square (refund %s) — leaving the row pending for the refund re-poll", pr.ID, r.AmountPence, r.ID, res.ID) + log.Printf("Auto-refund of replay-induced duplicate charge %s (%d pence) for pending row %s is PENDING at Square (refund %s) — leaving the row pending for the refund re-poll", pr.ID, pr.Amount, r.ID, res.ID) return errSweepRefundPending } - log.Printf("Auto-refunded replay-induced duplicate charge %s (%d pence) for pending row %s — refund %s", pr.ID, r.AmountPence, r.ID, res.ID) + log.Printf("Auto-refunded replay-induced duplicate charge %s (%d pence) for pending row %s — refund %s", pr.ID, pr.Amount, r.ID, res.ID) return nil } @@ -1153,7 +1156,7 @@ func refundSweepDuplicateCharge(ctx context.Context, table string, r staleRow, p // attached to it. The parent till_sale id is carried in the reason so the // re-poll pass claws it back when Square settles. func recordSweepDuplicateRefundRow(ctx context.Context, table string, r staleRow, pr *square.PaymentResult, refundID, status, refundKey string) { - amountPounds := float64(r.AmountPence) / 100.0 + amountPounds := float64(pr.Amount) / 100.0 paymentID := r.ID var bookingID *string reason := sweepDuplicateRefundReason diff --git a/backend/handlers/payments/sweep_test.go b/backend/handlers/payments/sweep_test.go index 1439fe7..266ebeb 100644 --- a/backend/handlers/payments/sweep_test.go +++ b/backend/handlers/payments/sweep_test.go @@ -9,7 +9,6 @@ import ( "testing" "time" - "crussell/clock" "crussell/db" "crussell/internal/square" "crussell/testutils" @@ -741,18 +740,24 @@ func TestSweepStalePendingPayments_KeyedReplayNewCharge_AutoRefunded(t *testing. t.Fatalf("failed to create stale pending payment: %v", err) } // 23h old: past the 22h keyed cutoff (so the keyed pass picks it up) but - // still inside Square's ~24h retention window (so the replay runs). NO - // stored snapshot → the minimal fallback body is rebuilt, which skips the - // production snapshot-decryption gate. created_by carries the payer so the - // admin notification is attributable and assertable. + // still inside Square's ~24h retention window (so the replay still runs — + // a row past 24h is blind-failed without a replay). NO stored snapshot → + // the minimal fallback body is rebuilt, which skips the production + // snapshot-decryption gate. created_by carries the payer so the admin + // notification is attributable and assertable. const key = "key-expired-replay-new-charge" const dupPayID = "pay_expired_key_new_charge" if _, err := tx.Exec(ctx, "UPDATE payments SET created_at = NOW() - INTERVAL '23 hours', idempotency_key = $1, square_source_id = 'ccof:test-saved-card', created_by = $2 WHERE id = $3", key, userID, staleID); err != nil { t.Fatalf("failed to age the stale payment: %v", err) } + var rowCreatedAt time.Time + if err := tx.QueryRow(ctx, "SELECT created_at FROM payments WHERE id = $1", staleID).Scan(&rowCreatedAt); err != nil { + t.Fatalf("failed to read aged payment created_at: %v", err) + } - // The replayed COMPLETED payment is created at sweep time (~23h after the - // row) — the expired-key replay landed a NEW charge on the saved card. + // The replayed COMPLETED payment's created_at is 25h AFTER the pending row + // — beyond the 24h legitimate-retry window (replayLegitimateRetryWindow), + // so it is provably a NEW expired-key replay charge, not a same-key retry. // The cross-check runs only in a non-dev/mock env, so the env is flipped to // production for the sweep (sequential, like the 2FA tests). The dev mock // is constructed BEFORE the flip (NewDevClient refuses production without @@ -764,7 +769,8 @@ func TestSweepStalePendingPayments_KeyedReplayNewCharge_AutoRefunded(t *testing. Status: "COMPLETED", ID: dupPayID, SquarePayID: dupPayID, - CreatedAt: clock.Now().Format(time.RFC3339), + Amount: 200000, + CreatedAt: rowCreatedAt.Add(25 * time.Hour).Format(time.RFC3339), }}} SquareClient = counting defer func() { SquareClient = origClient }() @@ -904,6 +910,10 @@ func TestSweepStalePendingPayments_KeyedReplayNewCharge_RefundPending_LeavesRowP if _, err := tx.Exec(ctx, "UPDATE payments SET created_at = NOW() - INTERVAL '23 hours', idempotency_key = $1, square_source_id = 'ccof:test-saved-card', created_by = $2 WHERE id = $3", key, userID, staleID); err != nil { t.Fatalf("failed to age the stale payment: %v", err) } + var rowCreatedAt time.Time + if err := tx.QueryRow(ctx, "SELECT created_at FROM payments WHERE id = $1", staleID).Scan(&rowCreatedAt); err != nil { + t.Fatalf("failed to read aged payment created_at: %v", err) + } origClient := SquareClient mock := square.NewDevClient() @@ -912,7 +922,8 @@ func TestSweepStalePendingPayments_KeyedReplayNewCharge_RefundPending_LeavesRowP Status: "COMPLETED", ID: dupPayID, SquarePayID: dupPayID, - CreatedAt: clock.Now().Format(time.RFC3339), + Amount: 200000, + CreatedAt: rowCreatedAt.Add(25 * time.Hour).Format(time.RFC3339), }}} defer func() { SquareClient = origClient }() @@ -1010,6 +1021,10 @@ func TestSweepStalePendingPayments_KeyedTillReplayNewCharge_RefundPending_NoClaw if _, err := tx.Exec(ctx, "UPDATE gift_cards SET created_at = NOW() - INTERVAL '23 hours' WHERE id = $1", giftCardID); err != nil { t.Fatalf("failed to age the gift card: %v", err) } + var saleCreatedAt time.Time + if err := tx.QueryRow(ctx, "SELECT created_at FROM till_sales WHERE id = $1", saleID).Scan(&saleCreatedAt); err != nil { + t.Fatalf("failed to read aged till sale created_at: %v", err) + } origClient := SquareClient mock := square.NewDevClient() @@ -1018,7 +1033,8 @@ func TestSweepStalePendingPayments_KeyedTillReplayNewCharge_RefundPending_NoClaw Status: "COMPLETED", ID: dupPayID, SquarePayID: dupPayID, - CreatedAt: clock.Now().Format(time.RFC3339), + Amount: 5000, + CreatedAt: saleCreatedAt.Add(25 * time.Hour).Format(time.RFC3339), }}} defer func() { SquareClient = origClient }() @@ -1476,8 +1492,9 @@ func TestSweepStalePendingPayments_KeyedReplaySlightlyBeforeRow_Rescues(t *testi // dead-zone fix: a same-key retry whose charge landed 21.5h after the pending // row (between the old 21h window and the 22h sweep cutoff) is the REAL charge // under a legitimately replayed key and MUST be rescued. The pre-B2 21h window -// refused it and stranded the row pending. 22h is the boundary: only a payment -// created AFTER row.CreatedAt+22h can be the sweep's own expired-key replay. +// refused it and stranded the row pending. The legitimate-retry boundary is now +// replayLegitimateRetryWindow (24h): only a payment created AFTER +// row.CreatedAt+24h can be the sweep's own expired-key replay. func TestSweepStalePendingPayments_KeyedReplayRetryAt215h_Rescues(t *testing.T) { ctx, tx := testutils.SetupTestTx(t) @@ -1505,7 +1522,7 @@ func TestSweepStalePendingPayments_KeyedReplayRetryAt215h_Rescues(t *testing.T) } // The replayed COMPLETED payment is a legitimate same-key retry created - // 21.5h after the row — inside the 22h legitimate window, so it is the real + // 21.5h after the row — inside the 24h legitimate window, so it is the real // charge and must be rescued, not refused as an expired-key duplicate. var rowCreatedAt time.Time if err := tx.QueryRow(ctx, "SELECT created_at FROM payments WHERE id = $1", staleID).Scan(&rowCreatedAt); err != nil { diff --git a/backend/handlers/today/today.go b/backend/handlers/today/today.go index 9795021..484675e 100644 --- a/backend/handlers/today/today.go +++ b/backend/handlers/today/today.go @@ -314,7 +314,7 @@ func computeAggregateSummary(r *http.Request, rangeStart, rangeEnd time.Time) *D SELECT COALESCE(SUM(COALESCE(bs.override_price, s.price)), 0) + COALESCE(SUM(COALESCE(bcs.override_price, cs.price)), 0) - - COALESCE((SELECT SUM(amount) FROM payments WHERE booking_id = b.id AND status = 'completed'), 0) AS amount_due, + - COALESCE((SELECT SUM(amount) FROM payments WHERE booking_id = b.id AND status = 'completed' AND payment_type <> 'tip'), 0) AS amount_due, COALESCE(SUM(COALESCE(bs.override_duration_minutes, s.duration_minutes)), 0) + COALESCE(SUM(COALESCE(bcs.override_duration_minutes, cs.duration_minutes)), 0) AS duration_minutes FROM bookings b_inner diff --git a/backend/handlers/user/account.go b/backend/handlers/user/account.go index 2cfb909..1e93446 100644 --- a/backend/handlers/user/account.go +++ b/backend/handlers/user/account.go @@ -5,6 +5,7 @@ import ( "crypto/sha256" "database/sql" "encoding/hex" + "encoding/json" "errors" "fmt" "log" @@ -14,14 +15,17 @@ import ( "os" "time" + "crussell/auth" "crussell/db" "crussell/handlers/payments" "crussell/internal/dav" "crussell/internal/s3" "crussell/internal/square" + "crussell/internal/twofa" "crussell/mw" "github.com/jackc/pgx/v5" + "golang.org/x/crypto/bcrypt" ) // --- Square erasure retry + alerting + durable outbox (GDPR H3 / A1) --- @@ -201,6 +205,15 @@ func clearSquareErasureOutboxRows(ctx context.Context, rowIDs []string) { } } +// DeleteAccountRequest carries the re-verification credentials the handler now +// requires before erasing an account (finding 3): the current password (always) +// and, in enforced environments for a user with 2FA enabled, a fresh one-time +// verification code. +type DeleteAccountRequest struct { + CurrentPassword string `json:"current_password"` + VerificationCode string `json:"verification_code"` +} + // DELETE /api/user/account func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) { userID, ok := mw.GetUserID(r.Context()) @@ -211,8 +224,10 @@ func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) { var accountRole string var profilePicURL sql.NullString - err := db.Conn.QueryRow(r.Context(), `SELECT account_role, profile_pic_url FROM users WHERE id = $1`, userID). - Scan(&accountRole, &profilePicURL) + var passwordHash sql.NullString + var twoFactorEnabled bool + err := db.Conn.QueryRow(r.Context(), `SELECT account_role, profile_pic_url, password_hash, two_factor_enabled FROM users WHERE id = $1`, userID). + Scan(&accountRole, &profilePicURL, &passwordHash, &twoFactorEnabled) if err != nil { if errors.Is(err, pgx.ErrNoRows) { http.Error(w, "user not found", http.StatusNotFound) @@ -225,6 +240,48 @@ func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) { ctx := r.Context() + // Finding 3: deleting an account is irreversible, so the session token alone + // must not be enough — an attacker who lifts a token (XSS, leaked localStorage) + // must not be able to erase the account. Re-verify the current password + // (mirroring ChangePasswordHandler's bcrypt compare) and, in enforced + // environments for a user with 2FA enabled, a fresh one-time code consumed by + // the shared core (twofa.VerifyForUser with ConsumeOnVerify). + var req DeleteAccountRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + http.Error(w, "invalid request", http.StatusBadRequest) + return + } + if passwordHash.Valid && passwordHash.String != "" { + if err := bcrypt.CompareHashAndPassword([]byte(passwordHash.String), []byte(req.CurrentPassword)); err != nil { + http.Error(w, "current password is incorrect", http.StatusUnauthorized) + return + } + } + if twoFARequired() && twoFactorEnabled { + if req.VerificationCode == "" { + http.Error(w, "a two-factor verification code is required to delete the account", http.StatusBadRequest) + return + } + switch err := twofa.VerifyForUser(ctx, userID, req.VerificationCode, twofa.ConsumeOnVerify); { + case err == nil: + // Verified: the code is consumed (single-use), matching the saved-card + // gate. If the deletion below then fails the user requests a fresh code. + case errors.Is(err, twofa.ErrIncorrect): + http.Error(w, "incorrect verification code", http.StatusBadRequest) + return + case errors.Is(err, twofa.ErrLockedOut): + http.Error(w, "Too many attempts. Request a new code.", http.StatusTooManyRequests) + return + case errors.Is(err, twofa.ErrMissingOrExpired): + http.Error(w, "verification code is missing or has expired", http.StatusBadRequest) + return + default: + log.Printf("failed to check 2FA pending code for user %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + } + // --- External system scrubbing (BEFORE SQL anonymize) --- // Delete profile picture from S3/R2 @@ -393,6 +450,13 @@ func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) { // columns are NULLed, but the cache is never touched by either). payments.InvalidateSquareCustomerCache(userID) + // Finding 5: anonymize_user()/delete_guest_user() deleted every refresh + // token the user held inside the committed transaction. Drop the in-memory + // family-alive verdicts for ALL of the user's rotation families so access + // tokens minted by those families die on their next verification instead of + // riding the 30s family-alive cache TTL. + auth.InvalidateFamilyAliveByUser(userID) + // Build the context used for critical-notification inserts: route through // the request transaction when one is active (tests) so alerts roll back // with the fixture; otherwise fall back to the shared pool (production, diff --git a/backend/handlers/user/admin_twofa_test.go b/backend/handlers/user/admin_twofa_test.go index 8a790c4..9f455fc 100644 --- a/backend/handlers/user/admin_twofa_test.go +++ b/backend/handlers/user/admin_twofa_test.go @@ -245,3 +245,66 @@ func TestAdminSendVerificationCode_UnknownCustomer_NotFound(t *testing.T) { w := makeAdmin2FARequest(http.HandlerFunc(AdminSendVerificationCodeHandler), http.MethodPost, "/api/admin/users/no-such-user/2fa/code", "admin", ctx) require.Equal(t, http.StatusNotFound, w.Code, "unknown target user must 404") } + +// TestAdminSendVerificationCode_WritesAuditTrail verifies finding 2a: an +// admin-scoped 2FA mint writes an admin_audit_log row (action_type +// '2fa_code_mint') keyed to the ADMIN, with details recording whether the code +// was FRESH or REUSED and the remaining lifetime. Uses a real admin user so the +// admin_id FK is satisfied (the synthetic "admin-test-id" elsewhere would make +// the best-effort audit write a no-op). +func TestAdminSendVerificationCode_WritesAuditTrail(t *testing.T) { + twofaEnvEnforced(t) + ctx, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + customerID, 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`, customerID) + require.NoError(t, err) + + // Fresh mint: audit row must record reused=false + the full 10-minute life. + req := httptest.NewRequest(http.MethodPost, "/api/admin/users/"+customerID+"/2fa/code", nil) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("id", customerID) + req = req.WithContext(context.WithValue(ctx, chi.RouteCtxKey, rctx)) + req = req.WithContext(context.WithValue(req.Context(), mw.UserIDKey, adminID)) + w := httptest.NewRecorder() + AdminSendVerificationCodeHandler(w, req) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + var auditCount int + var actionType string + var targetUserID sql.NullString + var details sql.NullString + err = tx.QueryRow(ctx, ` + SELECT COUNT(*), MAX(action_type), MAX(target_user_id), MAX(details::text) + FROM admin_audit_log WHERE admin_id = $1 AND action_type = '2fa_code_mint' + `, adminID).Scan(&auditCount, &actionType, &targetUserID, &details) + require.NoError(t, err) + require.Equal(t, 1, auditCount, "a fresh admin mint must write exactly one audit row") + require.Equal(t, "2fa_code_mint", actionType) + require.True(t, targetUserID.Valid && targetUserID.String == customerID, "audit must target the customer, not the admin") + var freshDetails map[string]any + require.NoError(t, json.Unmarshal([]byte(details.String), &freshDetails), "audit details must be parseable JSON") + require.Equal(t, false, freshDetails["reused"], "a fresh mint must be audited as fresh") + require.Equal(t, float64(600), freshDetails["remaining_seconds"], "a fresh mint reports the full 10-minute lifetime") + + // Second request reuses the still-valid code: audit must record reuse. (Both + // rows share the transaction's NOW(), so select by the distinguishing detail + // rather than created_at.) + w2 := httptest.NewRecorder() + AdminSendVerificationCodeHandler(w2, req) + require.Equal(t, http.StatusOK, w2.Code, w2.Body.String()) + + err = tx.QueryRow(ctx, ` + SELECT details::text FROM admin_audit_log + WHERE admin_id = $1 AND action_type = '2fa_code_mint' + AND (details->>'reused')::boolean = true + LIMIT 1 + `, adminID).Scan(&details) + require.NoError(t, err) + var reuseDetails map[string]any + require.NoError(t, json.Unmarshal([]byte(details.String), &reuseDetails)) + require.Equal(t, true, reuseDetails["reused"], "a reused code must be audited as reused") +} diff --git a/backend/handlers/user/gdpr_test.go b/backend/handlers/user/gdpr_test.go index 4a778c7..7b96486 100644 --- a/backend/handlers/user/gdpr_test.go +++ b/backend/handlers/user/gdpr_test.go @@ -628,8 +628,12 @@ func TestDeleteAccount_Scrubs2FA_RetainsNotes(t *testing.T) { t.Fatalf("failed to set user notes + 2FA: %v", err) } - req := httptest.NewRequest(http.MethodDelete, "/api/user/account", nil) - req = req.WithContext(context.WithValue(ctx, mw.UserIDKey, userID)) + // Finding 3: an enforced environment + a 2FA-enabled user requires a fresh + // one-time code at deletion time. Seed a known pending code (the test runs + // with SQUARE_ENVIRONMENT unset → enforced) and present it. + seedPendingTwoFA(t, ctx, tx, userID, "424242") + + req := deleteAccountRequest(t, ctx, userID, "testpassword123", "424242") rr := httptest.NewRecorder() DeleteAccountHandler(rr, req) diff --git a/backend/handlers/user/profile.go b/backend/handlers/user/profile.go index 8d0f91a..6e02c4f 100644 --- a/backend/handlers/user/profile.go +++ b/backend/handlers/user/profile.go @@ -22,6 +22,7 @@ import ( "golang.org/x/text/cases" "golang.org/x/text/language" + coreauth "crussell/auth" "crussell/clock" "crussell/db" "crussell/handlers/auth" @@ -808,6 +809,13 @@ func ChangePasswordHandler(w http.ResponseWriter, r *http.Request) { return } + // Finding 5: the DELETE above removed every refresh token the user held, but + // the in-memory family-alive cache would keep their access tokens passing + // verifyFamilyAlive for up to familyAliveCacheTTL. Drop the cached verdicts + // for all of the user's families so bound access tokens die on their next + // verification — the password change kills sessions immediately. + coreauth.InvalidateFamilyAliveByUser(userID) + // B9: the current JTI is now in revoked_jtis and every refresh token for // the user has been deleted — existing sessions must re-authenticate. log.Printf("Password changed for user %s - current JTI revoked and all refresh tokens deleted; existing sessions must re-authenticate", userID) diff --git a/backend/handlers/user/profile_test.go b/backend/handlers/user/profile_test.go index e2037cb..65e0e91 100644 --- a/backend/handlers/user/profile_test.go +++ b/backend/handlers/user/profile_test.go @@ -307,8 +307,7 @@ func TestAccount_Delete(t *testing.T) { token := jwt.GenerateUserToken(userID) - req := httptest.NewRequest(http.MethodDelete, "/api/user/account", nil) - req = req.WithContext(context.WithValue(ctx, mw.UserIDKey, userID)) + req := deleteAccountRequest(t, ctx, userID, "testpassword123", "") req.Header.Set("Authorization", "Bearer "+token) rr := httptest.NewRecorder() @@ -344,8 +343,7 @@ func TestAccount_DeleteGuest(t *testing.T) { token := jwt.GenerateUserToken(userID) - req := httptest.NewRequest(http.MethodDelete, "/api/user/account", nil) - req = req.WithContext(context.WithValue(ctx, mw.UserIDKey, userID)) + req := deleteAccountRequest(t, ctx, userID, "testpassword123", "") req.Header.Set("Authorization", "Bearer "+token) rr := httptest.NewRecorder() diff --git a/backend/handlers/user/twofa.go b/backend/handlers/user/twofa.go index dd5216e..ef23550 100644 --- a/backend/handlers/user/twofa.go +++ b/backend/handlers/user/twofa.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "log" + "log/slog" "math/big" "net/http" "time" @@ -109,7 +110,7 @@ func twoFAAttemptStateFor(userID string) *twoFAAttemptState { // twoFAMintThrottled reports whether a fresh 2FA code mint for the user is // still inside the per-user cooldown window (twoFAMintCooldown): a previous // mint within the window throttles the request (429) instead of minting -// another code. Shared by SetupTwoFAHandler and ensurePendingTwoFACode so the +// another code. Shared by SetupTwoFAHandler and EnsurePendingTwoFACode so the // cooldown rule cannot drift between the setup and disable-flow call paths. func twoFAMintThrottled(st *twoFAAttemptState, now time.Time) bool { return !st.LastMintAt.IsZero() && now.Sub(st.LastMintAt) < twoFAMintCooldown @@ -271,7 +272,7 @@ func SetupTwoFAHandler(w http.ResponseWriter, r *http.Request) { // Mint cooldown (B11a): the shared twoFAMintThrottled helper bounds setup // re-mints — the same guard the disable flow applies via - // ensurePendingTwoFACode. A fresh setup code no longer resets the + // EnsurePendingTwoFACode. A fresh setup code no longer resets the // failed-attempt counter (B11b), so without this a setup-spam loop could // mint fresh codes (each invalidating the prior lockout state) and keep a // guessing budget alive indefinitely. @@ -480,7 +481,7 @@ func writeTwoFAEnabled(w http.ResponseWriter) { // recover after a short wait. const twoFAMintCooldown = 1 * time.Minute -// errTwoFAMintThrottled is returned by ensurePendingTwoFACode when the user's +// 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") @@ -521,7 +522,7 @@ func SendDisableCodeHandler(w http.ResponseWriter, r *http.Request) { st.Mu.Lock() defer st.Mu.Unlock() - if _, _, err := ensurePendingTwoFACode(r, userID, st, "disable 2FA"); err != nil { + if _, _, err := EnsurePendingTwoFACode(r, userID, st, "disable 2FA"); err != nil { if errors.Is(err, errTwoFAMintThrottled) { http.Error(w, "Too many attempts. Wait before requesting a new code.", http.StatusTooManyRequests) return @@ -548,7 +549,7 @@ func SendDisableCodeHandler(w http.ResponseWriter, r *http.Request) { // saved-card charge, so every charge returned 400 "Verification code expired — // request a new one" with no way to get a new one. // -// The mint machinery is shared with the disable flow: ensurePendingTwoFACode +// The mint machinery is shared with the disable flow: EnsurePendingTwoFACode // reuses a still-valid pending code when one exists and otherwise mints + // delivers a fresh one via the same build-dependent channel as setup // (deliverTwoFACode — [2FA] log in dev/test; pepper- and delivery-channel @@ -593,7 +594,7 @@ func SendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { st.Mu.Lock() defer st.Mu.Unlock() - code, remaining, err := ensurePendingTwoFACode(r, userID, st, "saved-card charge") + code, remaining, err := EnsurePendingTwoFACode(r, userID, st, "saved-card charge") if err != nil { if errors.Is(err, errTwoFAMintThrottled) { http.Error(w, "Too many attempts. Wait before requesting a new code.", http.StatusTooManyRequests) @@ -627,6 +628,44 @@ func SendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { } } +// insertAdminAudit records an admin action in admin_audit_log. Mirrors the +// insertAdminAuditCharge pattern (handlers/payments/handlers.go) — same table, +// same columns, same best-effort non-fatal failure handling. The insert runs in +// its OWN transaction (a savepoint in the test harness) so an audit-write +// failure — e.g. a synthetic admin id in tests violating the admin_id FK — +// rolls back only the audit write and can never abort the caller's transaction. +func insertAdminAudit(ctx context.Context, adminID, targetUserID, action string, details map[string]any) { + detailsJSON, err := json.Marshal(details) + if err != nil { + log.Printf("Failed to marshal admin_audit_log details (non-critical): %v", err) + return + } + var target any + if targetUserID != "" { + target = targetUserID + } + auditTx, err := db.Conn.Begin(ctx) + if err != nil { + log.Printf("Failed to record admin_audit_log (non-critical): %v", err) + return + } + defer func() { + if err := auditTx.Rollback(ctx); err != nil && !errors.Is(err, pgx.ErrTxClosed) { + slog.Error("failed to rollback admin audit transaction", "err", err) + } + }() + if _, err := auditTx.Exec(ctx, ` + INSERT INTO admin_audit_log (admin_id, action_type, target_user_id, details) + VALUES ($1, $2, $3, $4::jsonb) + `, adminID, action, target, string(detailsJSON)); err != nil { + log.Printf("Failed to record admin_audit_log (non-critical): %v", err) + return + } + if err := auditTx.Commit(ctx); err != nil { + log.Printf("Failed to record admin_audit_log (non-critical): %v", err) + } +} + // AdminSendVerificationCodeHandler mints (or reuses) a 2FA code for a TARGET // user, not the session user. The saved-card charge gate verifies the code // against the CARD OWNER (customer) — never the admin session (till.go:951, @@ -634,6 +673,10 @@ func SendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { // and could never authorize the customer's charge. Delivering keyed to the // customer preserves the invariant that the customer, not the admin, is the // authentication subject for their card. +// +// Finding 2: every successful mint-or-reuse writes an admin_audit_log row +// (action_type '2fa_code_mint', details carrying reused vs fresh + the +// remaining code lifetime), so admin-scoped code issuance is never silent. func AdminSendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { targetUserID := chi.URLParam(r, "id") if targetUserID == "" { @@ -664,7 +707,7 @@ func AdminSendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { st.Mu.Lock() defer st.Mu.Unlock() - code, remaining, err := ensurePendingTwoFACode(r, targetUserID, st, "saved-card charge") + code, remaining, err := EnsurePendingTwoFACode(r, targetUserID, st, "saved-card charge") if err != nil { if errors.Is(err, errTwoFAMintThrottled) { http.Error(w, "Too many attempts. Wait before requesting a new code.", http.StatusTooManyRequests) @@ -679,6 +722,16 @@ func AdminSendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { return } + // Finding 2a: the admin who minted/reused the code is audited. The empty + // `code` return discriminates reuse from a fresh mint — the plaintext is + // only returned for a fresh delivery, never on reuse (EnsurePendingTwoFACode). + if adminID, ok := mw.GetUserID(r.Context()); ok { + insertAdminAudit(r.Context(), adminID, targetUserID, "2fa_code_mint", map[string]any{ + "reused": code == "", + "remaining_seconds": int(remaining.Seconds()), + }) + } + resp := map[string]any{"message": "Code sent", "remaining_seconds": int(remaining.Seconds())} if !twoFARequired() && code != "" { // Dev convenience (matches setup): return the freshly minted code so @@ -744,7 +797,7 @@ func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { // The per-user mint cooldown still bounds how often a fresh code can be // minted — at most one per twoFAMintCooldown — but it cannot grant a fresh // guessing budget. - if _, _, err := ensurePendingTwoFACode(r, userID, st, "disable 2FA"); err != nil { + if _, _, err := EnsurePendingTwoFACode(r, userID, st, "disable 2FA"); err != nil { if errors.Is(err, errTwoFAMintThrottled) { http.Error(w, "Too many attempts. Wait before requesting a new code.", http.StatusTooManyRequests) return @@ -775,7 +828,7 @@ func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { http.Error(w, "Too many attempts. Request a new code.", http.StatusTooManyRequests) return case twoFACodeMissingOrExpired: - // ensurePendingTwoFACode just guaranteed a valid pending code; defensive. + // EnsurePendingTwoFACode just guaranteed a valid pending code; defensive. http.Error(w, "verification code is missing or has expired", http.StatusBadRequest) return } @@ -788,13 +841,20 @@ func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) } -// ensurePendingTwoFACode guarantees the user has a valid (unexpired) pending +// EnsurePendingTwoFACode guarantees the user has a valid (unexpired) pending // code to verify against, generating + delivering a fresh one via the same // build-dependent delivery channel as setup (see deliverTwoFACode) when the // stored code is missing or expired. purpose labels the delivery for the [2FA] // log line (e.g. "disable 2FA", "saved-card charge"). The caller must hold the // user's attempt-state mutex. // +// Exported so the finding-1 re-mint contract is referenceable: a saved-card +// charge gate that burns a code at verify time (twofa.ConsumeOnVerify) re-mints +// on a failed charge through this function. handlers/payments cannot import +// this package (import cycle), so payments reaches it via the HTTP mint +// endpoints (POST /user/2fa/code, POST /admin/users/{id}/2fa/code) that +// delegate to it. +// // It returns the plaintext code only when a FRESH code was minted and // delivered (dev/test builds always deliver it; production builds only when // the operator opted into log delivery — see twofa_prod.go). When a valid @@ -816,7 +876,7 @@ func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { // who exhausts the budget must wait out the window, not the mint cooldown. 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, purpose string) (string, time.Duration, error) { +func EnsurePendingTwoFACode(r *http.Request, userID string, st *twoFAAttemptState, purpose string) (string, time.Duration, error) { var pendingHash sql.NullString var pendingExpires sql.NullTime err := db.Conn.QueryRow(r.Context(), ` @@ -828,6 +888,10 @@ func ensurePendingTwoFACode(r *http.Request, userID string, st *twoFAAttemptStat return "", 0, err } if pendingHash.Valid && pendingExpires.Valid && pendingExpires.Time.After(clock.Now()) { + // LOW 5 / finding 2: a reused code is NOT re-delivered (the plaintext is + // unavailable — only the digest is stored), but it must still leave a + // trace so silent code-reuse is audit-visible in the [2FA] log stream. + log.Printf("[2FA] code reused (user=%s, purpose=%s, remaining=%s) — no fresh code minted or delivered", userID, purpose, pendingExpires.Time.Sub(clock.Now()).Round(time.Second)) return "", pendingExpires.Time.Sub(clock.Now()), nil } now := clock.Now() diff --git a/backend/handlers/user/twofa_test.go b/backend/handlers/user/twofa_test.go index e2e214f..168ee37 100644 --- a/backend/handlers/user/twofa_test.go +++ b/backend/handlers/user/twofa_test.go @@ -825,8 +825,11 @@ func TestTwoFADisable_MintThrottled_BoundsGuessing(t *testing.T) { 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") + // no longer reset the attempt budget. Count the fresh-delivery marker + // ("[2FA] code delivery requested") rather than the purpose substring: + // finding 2b added a "[2FA] code reused" log line that also names the + // purpose, so a raw purpose count would over-count. + mints := strings.Count(buf.String(), "[2FA] code delivery requested") require.Equal(t, 1, mints, "expected exactly 1 fresh-code mint; log:\n%s", buf.String()) } @@ -1182,7 +1185,7 @@ func TestTwoFADisableCode_MintsFreshCode(t *testing.T) { } // TestTwoFADisableCode_ReusesValidPendingCode verifies that a valid unexpired -// pending code is reused by ensurePendingTwoFACode (the stored hash is +// pending code is reused by EnsurePendingTwoFACode (the stored hash is // unchanged) instead of minting a fresh one. func TestTwoFADisableCode_ReusesValidPendingCode(t *testing.T) { twofaEnvEnforced(t) @@ -1203,7 +1206,7 @@ func TestTwoFADisableCode_ReusesValidPendingCode(t *testing.T) { // TestTwoFADisableCode_MintThrottled verifies the per-user mint cooldown: a // second code request inside twoFAMintCooldown returns 429. The pending code is // dropped first (as a lockout does) because a still-valid code is reused by -// ensurePendingTwoFACode, which short-circuits the cooldown check. +// EnsurePendingTwoFACode, which short-circuits the cooldown check. func TestTwoFADisableCode_MintThrottled(t *testing.T) { twofaEnvEnforced(t) ctx, tx := testutils.SetupTestTx(t) @@ -1290,9 +1293,14 @@ func TestTwoFASendVerificationCode_Enabled_MintsFresh(t *testing.T) { // TestTwoFASendVerificationCode_ReusesValidPendingCode verifies that a valid // unexpired pending code is reused (the stored hash is unchanged) instead of a -// fresh mint, so a mid-flow charge retry is not throttled. +// fresh mint, so a mid-flow charge retry is not throttled — and (finding 2b) +// that the reuse leaves an auditable [2FA] log line instead of being silent. func TestTwoFASendVerificationCode_ReusesValidPendingCode(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) @@ -1317,6 +1325,11 @@ func TestTwoFASendVerificationCode_ReusesValidPendingCode(t *testing.T) { require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&pendingHash)) require.True(t, pendingHash.Valid) require.Equal(t, hashTwoFACode("123456"), pendingHash.String, "existing valid pending code must be reused, not re-minted") + + // Finding 2b: reuse must be visible in the [2FA] log stream (no fresh code + // is delivered, so this is the only audit trace of the reuse). + require.Contains(t, buf.String(), "[2FA] code reused", "a reused code must leave a [2FA] log line") + require.NotContains(t, buf.String(), "[2FA] code:", "a reused code must NOT be logged as a fresh delivery") } // TestTwoFASendVerificationCode_NotEnabled_409 verifies that a user who has NOT @@ -1335,7 +1348,7 @@ func TestTwoFASendVerificationCode_NotEnabled_409(t *testing.T) { // TestTwoFASendVerificationCode_MintThrottled verifies the per-user mint // cooldown applies: a second code request inside twoFAMintCooldown returns 429. // The pending code is dropped first (as a lockout does) because a still-valid -// code is reused by ensurePendingTwoFACode, which short-circuits the cooldown. +// code is reused by EnsurePendingTwoFACode, which short-circuits the cooldown. func TestTwoFASendVerificationCode_MintThrottled(t *testing.T) { twofaEnvEnforced(t) ctx, tx := testutils.SetupTestTx(t) diff --git a/backend/handlers/user/user_coverage_test.go b/backend/handlers/user/user_coverage_test.go index f182885..0b655cd 100644 --- a/backend/handlers/user/user_coverage_test.go +++ b/backend/handlers/user/user_coverage_test.go @@ -45,6 +45,23 @@ import ( // DeleteAccountHandler Coverage Tests // ============================================================================= +// deleteAccountRequest builds the DELETE /api/user/account request the handler +// now requires (finding 3): the current password in the body, plus a 2FA code +// when one is supplied (enforced environments + 2FA-enabled users only). +func deleteAccountRequest(t *testing.T, ctx context.Context, userID, password, code string) *http.Request { + t.Helper() + body := map[string]string{"current_password": password} + if code != "" { + body["verification_code"] = code + } + b, err := json.Marshal(body) + require.NoError(t, err) + req := httptest.NewRequest(http.MethodDelete, "/api/user/account", bytes.NewReader(b)) + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(context.WithValue(ctx, mw.UserIDKey, userID)) + return req +} + // TestDeleteAccount_Unauthorized verifies that deleting an account without // setting user ID in context returns 401 Unauthorized. func TestDeleteAccount_Unauthorized(t *testing.T) { @@ -92,8 +109,7 @@ func TestDeleteAccount_WithBooking(t *testing.T) { t.Fatalf("failed to create booking: %v", err) } - req := httptest.NewRequest(http.MethodDelete, "/api/user/account", nil) - req = req.WithContext(context.WithValue(ctx, mw.UserIDKey, userID)) + req := deleteAccountRequest(t, ctx, userID, "testpassword123", "") rr := httptest.NewRecorder() DeleteAccountHandler(rr, req) @@ -135,8 +151,7 @@ func TestDeleteAccount_GuestWithBooking(t *testing.T) { t.Fatalf("failed to create booking: %v", err) } - req := httptest.NewRequest(http.MethodDelete, "/api/user/account", nil) - req = req.WithContext(context.WithValue(ctx, mw.UserIDKey, userID)) + req := deleteAccountRequest(t, ctx, userID, "testpassword123", "") rr := httptest.NewRecorder() DeleteAccountHandler(rr, req) @@ -155,6 +170,77 @@ func TestDeleteAccount_GuestWithBooking(t *testing.T) { } } +// TestDeleteAccount_WrongPassword_Rejected verifies the finding-3 password +// re-verification: deleting an account with the WRONG current password returns +// 401 and the account survives. +func TestDeleteAccount_WrongPassword_Rejected(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + req := deleteAccountRequest(t, ctx, userID, "not-the-password", "") + rr := httptest.NewRecorder() + DeleteAccountHandler(rr, req) + + require.Equal(t, http.StatusUnauthorized, rr.Code, rr.Body.String()) + + var firstName string + require.NoError(t, tx.QueryRow(ctx, `SELECT n_first_name FROM users WHERE id = $1`, userID).Scan(&firstName)) + require.Equal(t, "Test", firstName, "the account must not be touched by a failed re-verification") +} + +// TestDeleteAccount_MissingPasswordBody_Rejected verifies that a DELETE with no +// password body (the pre-finding-3 client contract) is rejected as malformed. +func TestDeleteAccount_MissingPasswordBody_Rejected(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodDelete, "/api/user/account", nil) + req = req.WithContext(context.WithValue(ctx, mw.UserIDKey, userID)) + rr := httptest.NewRecorder() + DeleteAccountHandler(rr, req) + + require.Equal(t, http.StatusBadRequest, rr.Code, rr.Body.String()) +} + +// TestDeleteAccount_Enforced2FA_RequiresCode verifies the finding-3 2FA +// re-verification: in an enforced environment, a 2FA-enabled user must present +// a correct one-time code (and their password) — a wrong code is rejected with +// 400 and the account survives; the correct code deletes it. +func TestDeleteAccount_Enforced2FA_RequiresCode(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + _, err = tx.Exec(ctx, `UPDATE users SET two_factor_enabled = true, two_factor_method = 'email' WHERE id = $1`, userID) + require.NoError(t, err) + + // Enforced by default in tests (SQUARE_ENVIRONMENT unset) + 2FA enabled → + // the handler demands a code. A wrong code must 400. + seedPendingTwoFA(t, ctx, tx, userID, "424242") + req := deleteAccountRequest(t, ctx, userID, "testpassword123", "000000") + rr := httptest.NewRecorder() + DeleteAccountHandler(rr, req) + require.Equal(t, http.StatusBadRequest, rr.Code, "wrong 2FA code must be rejected before deletion") + require.Contains(t, rr.Body.String(), "incorrect verification code") + + var firstName string + require.NoError(t, tx.QueryRow(ctx, `SELECT n_first_name FROM users WHERE id = $1`, userID).Scan(&firstName)) + require.Equal(t, "Test", firstName, "the account must survive a wrong 2FA code") + + // A missing code must also be rejected. + req = deleteAccountRequest(t, ctx, userID, "testpassword123", "") + rr = httptest.NewRecorder() + DeleteAccountHandler(rr, req) + require.Equal(t, http.StatusBadRequest, rr.Code, "a missing 2FA code must be rejected in an enforced environment") + + // The correct code (password + code) deletes the account. + req = deleteAccountRequest(t, ctx, userID, "testpassword123", "424242") + rr = httptest.NewRecorder() + DeleteAccountHandler(rr, req) + require.Equal(t, http.StatusNoContent, rr.Code, rr.Body.String()) +} + // ============================================================================= // ChangePasswordHandler Coverage Tests // ============================================================================= @@ -566,8 +652,7 @@ func TestDeleteAccount_WithProfilePicture(t *testing.T) { require.NoError(t, err) handler := http.HandlerFunc(DeleteAccountHandler) - req := httptest.NewRequest("DELETE", "/api/user/account", nil) - req = req.WithContext(context.WithValue(ctx, mw.UserIDKey, userID)) + req := deleteAccountRequest(t, ctx, userID, "testpassword123", "") w := httptest.NewRecorder() handler.ServeHTTP(w, req) @@ -594,8 +679,7 @@ func TestDeleteAccount_WithSquareClient(t *testing.T) { require.NoError(t, err) handler := http.HandlerFunc(DeleteAccountHandler) - req := httptest.NewRequest("DELETE", "/api/user/account", nil) - req = req.WithContext(context.WithValue(ctx, mw.UserIDKey, userID)) + req := deleteAccountRequest(t, ctx, userID, "testpassword123", "") w := httptest.NewRecorder() handler.ServeHTTP(w, req) @@ -689,8 +773,7 @@ func TestDeleteAccount_LogsRedactCardTokens(t *testing.T) { require.NoError(t, err) handler := http.HandlerFunc(DeleteAccountHandler) - req := httptest.NewRequest(http.MethodDelete, "/api/user/account", nil) - req = req.WithContext(context.WithValue(ctx, mw.UserIDKey, userID)) + req := deleteAccountRequest(t, ctx, userID, "testpassword123", "") w := httptest.NewRecorder() handler.ServeHTTP(w, req) require.Equal(t, http.StatusNoContent, w.Code) @@ -734,8 +817,7 @@ func TestDeleteAccount_DeletesSquareCustomerOnce(t *testing.T) { } handler := http.HandlerFunc(DeleteAccountHandler) - req := httptest.NewRequest(http.MethodDelete, "/api/user/account", nil) - req = req.WithContext(context.WithValue(ctx, mw.UserIDKey, userID)) + req := deleteAccountRequest(t, ctx, userID, "testpassword123", "") w := httptest.NewRecorder() handler.ServeHTTP(w, req) require.Equal(t, http.StatusNoContent, w.Code) @@ -813,8 +895,7 @@ func TestDeleteAccount_SquareCleanupNotDispatchedOnTxFailure(t *testing.T) { reqCtx := db.ContextWithTx(context.Background(), &failingTx{Tx: pgxTx, failExec: true}) handler := http.HandlerFunc(DeleteAccountHandler) - req := httptest.NewRequest(http.MethodDelete, "/api/user/account", nil) - req = req.WithContext(context.WithValue(reqCtx, mw.UserIDKey, guestID)) + req := deleteAccountRequest(t, reqCtx, guestID, "testpassword123", "") w := httptest.NewRecorder() handler.ServeHTTP(w, req) @@ -875,8 +956,7 @@ func TestDeleteAccount_SkipsSharedSquareCustomer(t *testing.T) { }) handler := http.HandlerFunc(DeleteAccountHandler) - req := httptest.NewRequest(http.MethodDelete, "/api/user/account", nil) - req = req.WithContext(context.WithValue(context.Background(), mw.UserIDKey, userA)) + req := deleteAccountRequest(t, context.Background(), userA, "testpassword123", "") w := httptest.NewRecorder() handler.ServeHTTP(w, req) require.Equal(t, http.StatusNoContent, w.Code) @@ -938,8 +1018,7 @@ func TestDeleteAccount_InvalidatesSquareCustomerCache(t *testing.T) { require.NotEmpty(t, originalID, "a Square customer must be provisioned and cached for the save-card user") handler := http.HandlerFunc(DeleteAccountHandler) - req := httptest.NewRequest(http.MethodDelete, "/api/user/account", nil) - req = req.WithContext(context.WithValue(ctx, mw.UserIDKey, userID)) + req := deleteAccountRequest(t, ctx, userID, "testpassword123", "") w := httptest.NewRecorder() handler.ServeHTTP(w, req) require.Equal(t, http.StatusNoContent, w.Code) diff --git a/backend/internal/twofa/twofa.go b/backend/internal/twofa/twofa.go index 638e656..94fb638 100644 --- a/backend/internal/twofa/twofa.go +++ b/backend/internal/twofa/twofa.go @@ -11,7 +11,7 @@ // // Contract for the payments gate: // -// err := twofa.VerifyForUser(ctx, userID, code, false) // consume = false +// err := twofa.VerifyForUser(ctx, userID, code, twofa.ConsumeOnVerify) // if err != nil { // switch { // case errors.Is(err, twofa.ErrIncorrect): @@ -25,18 +25,25 @@ // } // } // -// The payments saved-card charge gate verifies WITH consume=false (MEDIUM-2): -// the code is checked at gate time but only NULLed when the charge reaches a -// TERMINAL SUCCESS state (the handlers call twofa.ConsumePendingCode inside the -// transaction that records the completed charge). A failed/ambiguous Square -// charge therefore does NOT burn the code — the same-key retry re-verifies the -// SAME operator-relayed code instead of hitting a 400 "expired". Consumption is -// idempotent, so a code still authorizes exactly one completed charge (and -// remains bounded by its 10-minute lifetime). The interactive setup/disable -// flows pass consume=false too — they clear the pending fields themselves on -// success (enableTwoFA / disableTwoFA), so the code must stay valid through -// their whole handshake. The ONLY remaining consume=true caller is the -// save-card SAVE gate (handlers/payments), where saving a card is itself a +// Consume mode (MEDIUM-2 remediation, finding 1): a successful verify with +// consume=true NULLs the pending code ATOMICALLY in the same critical section +// as the check, so one code authorizes exactly ONE operation — two concurrent +// charges can never both pass the gate with the same code (the per-user mutex +// serializes Check, and the second verify reads a NULLed digest and returns +// ErrMissingOrExpired). The payments saved-card CHARGE gates should therefore +// pass twofa.ConsumeOnVerify for FRESH charges: the code is burned at the gate, +// and a failed/ambiguous Square charge re-mints a fresh code (via the user +// package's exported EnsurePendingTwoFACode — reached through the HTTP mint +// endpoints, since handlers/payments cannot import handlers/user) instead of +// re-verifying the same code. This replaces the earlier MEDIUM-2 deferred +// consume (verify-with-consume=false at the gate + ConsumePendingCode at +// terminal success), which under concurrency let two gates both verify the same +// code before either charge consumed it. +// +// The interactive setup/disable flows pass DeferredConsume (false) — they clear +// the pending fields themselves on success (enableTwoFA / disableTwoFA), so the +// code must stay valid through their whole handshake. The save-card SAVE gate +// (handlers/payments) passes ConsumeOnVerify (true), since saving a card is a // terminal operation with no downstream charge to attach consumption to. // // The failed-attempt counter is keyed per user and resets ONLY on a successful @@ -66,6 +73,22 @@ import ( // before the pending code is invalidated and a new one must be requested. const MaxAttempts = 5 +// Consume mode for VerifyForUser / Check. Named so the magic bool cannot drift +// between call sites (the payments gate vs the interactive flows). +const ( + // ConsumeOnVerify makes a successful verify SINGLE-USE immediately: the + // pending-code digest and expiry are NULLed in the same critical section as + // the successful check (see Check). Use this for FRESH terminal operations — + // the saved-card CHARGE gates (finding 1) and the SAVE gate — where one + // code must authorize exactly one operation. + ConsumeOnVerify = true + // DeferredConsume verifies WITHOUT consuming; the caller NULLs the code + // itself when its operation reaches terminal success (ConsumePendingCode) or + // clears the pending fields on success (the interactive enable/disable + // flows). + DeferredConsume = false +) + // AttemptWindow bounds how long a per-user attempt counter lives before // resetting, and doubles as the stale-entry eviction horizon for the map. const AttemptWindow = 10 * time.Minute @@ -287,13 +310,13 @@ const ( // MissingOrExpired. consume makes a correct code single-use IMMEDIATELY: the // stored digest and its expiry are NULLed right here, so one code cannot // authorize a second operation within its lifetime. The interactive -// setup/disable flows pass false and clear the pending fields themselves on -// success. The payments saved-card charge gate now ALSO passes false (MEDIUM-2): -// it verifies at gate time and defers consumption to the completed-charge -// transaction via ConsumePendingCode, so a failed Square charge does not burn -// the code. The returned error is non-nil only for DB failures (callers return -// 500); a lockout's pending-code invalidation failure is logged here and still -// reported as a lockout. +// setup/disable flows pass DeferredConsume (false) and clear the pending fields +// themselves on success. The payments saved-card charge gates pass +// ConsumeOnVerify (true) for FRESH charges (finding 1): the code is burned at +// the gate, and a failed Square charge re-mints a fresh one. The returned +// error is non-nil only for DB failures (callers return 500); a lockout's +// pending-code invalidation failure is logged here and still reported as a +// lockout. func Check(ctx context.Context, userID string, st *AttemptState, reqCode string, consume bool) (Result, error) { if now := clock.Now(); now.Sub(st.LastActive()) > AttemptWindow { st.Count.Store(0) @@ -360,6 +383,18 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string, st.SetLastActive(clock.Now()) st.LastMintAt = time.Time{} ResetAttempts(userID) + // LOW 6b: a correct code proves control of the account's second factor, so + // lift any password-guessing login lockout (users.failed_attempts / + // locked_until) — a successful 2FA challenge is a strong auth signal, and + // the only way to reach a 2FA verify is an already-authenticated session. + // Best-effort: a failure only logs; the verify has already succeeded. + if _, err := db.Conn.Exec(ctx, ` + UPDATE users + SET failed_attempts = 0, locked_until = NULL + WHERE id = $1 + `, userID); err != nil { + log.Printf("failed to clear login lockout on 2FA verify for user %s: %v", userID, err) + } if consume { // Consume mode (the payments saved-card gate, B6/B10): a verified code // is single-use. NULL the stored digest and its expiry so the same code @@ -427,11 +462,12 @@ var ( // the payments card-access gate (B6/B10): a saved-card charge must present a // real, freshly-verified challenge. consume makes a correct code single-use // IMMEDIATELY (the pending-code digest and expiry are NULLed in the same -// critical section as the successful check — see Check). The payments SAVED- -// CARD CHARGE gate passes false and consumes later via ConsumePendingCode -// (MEDIUM-2) so a failed charge does not burn the code; the save-card SAVE -// gate and the interactive setup/disable flows pass false and clear the -// pending fields themselves on success (enableTwoFA / disableTwoFA). +// critical section as the successful check — see Check). The payments saved- +// card CHARGE gate passes ConsumeOnVerify for FRESH charges (finding 1: a code +// authorizes exactly one charge, and a failed charge re-mints); the save-card +// SAVE gate passes ConsumeOnVerify too; the interactive setup/disable flows +// pass DeferredConsume and clear the pending fields themselves on success +// (enableTwoFA / disableTwoFA). func VerifyForUser(ctx context.Context, userID, code string, consume bool) error { st := StateFor(userID) st.Mu.Lock() diff --git a/backend/internal/twofa/twofa_test.go b/backend/internal/twofa/twofa_test.go index 61c9c03..8ac53e9 100644 --- a/backend/internal/twofa/twofa_test.go +++ b/backend/internal/twofa/twofa_test.go @@ -40,8 +40,8 @@ func TestVerifyForUser_CorrectAndWrongCode(t *testing.T) { userID, err := fixtures.CreateTestUser(tx) require.NoError(t, err) seedPending(t, ctx, tx, userID, "123456") - require.NoError(t, VerifyForUser(ctx, userID, "123456", false), "correct code must verify") - require.ErrorIs(t, VerifyForUser(ctx, userID, "999999", false), ErrIncorrect) + require.NoError(t, VerifyForUser(ctx, userID, "123456", DeferredConsume), "correct code must verify") + require.ErrorIs(t, VerifyForUser(ctx, userID, "999999", DeferredConsume), ErrIncorrect) // consume=true (payments saved-card gate path): a success DESTROYS the // pending code, so re-verifying the same code reports ErrMissingOrExpired @@ -49,8 +49,8 @@ func TestVerifyForUser_CorrectAndWrongCode(t *testing.T) { userID2, err := fixtures.CreateTestUser(tx) require.NoError(t, err) seedPending(t, ctx, tx, userID2, "123456") - require.NoError(t, VerifyForUser(ctx, userID2, "123456", true), "correct code must verify") - require.ErrorIs(t, VerifyForUser(ctx, userID2, "123456", true), ErrMissingOrExpired, "a consumed code must be single-use") + require.NoError(t, VerifyForUser(ctx, userID2, "123456", ConsumeOnVerify), "correct code must verify") + require.ErrorIs(t, VerifyForUser(ctx, userID2, "123456", ConsumeOnVerify), ErrMissingOrExpired, "a consumed code must be single-use") var pendingHash sql.NullString require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID2).Scan(&pendingHash)) require.False(t, pendingHash.Valid, "a consumed code must be NULLed in the DB") @@ -63,16 +63,57 @@ func TestVerifyForUser_LockoutAndMissing(t *testing.T) { seedPending(t, ctx, tx, userID, "123456") // Wrong code #1 → ErrIncorrect; four more reach the 5-attempt cap. - require.ErrorIs(t, VerifyForUser(ctx, userID, "999999", true), ErrIncorrect) + require.ErrorIs(t, VerifyForUser(ctx, userID, "999999", ConsumeOnVerify), ErrIncorrect) for i := 0; i < 4; i++ { - _ = VerifyForUser(ctx, userID, "999999", true) + _ = VerifyForUser(ctx, userID, "999999", ConsumeOnVerify) } - require.ErrorIs(t, VerifyForUser(ctx, userID, "999999", true), ErrLockedOut) + require.ErrorIs(t, VerifyForUser(ctx, userID, "999999", ConsumeOnVerify), ErrLockedOut) // A fresh user with no pending code → ErrMissingOrExpired. userID2, err := fixtures.CreateTestUser(tx) require.NoError(t, err) - require.ErrorIs(t, VerifyForUser(ctx, userID2, "123456", true), ErrMissingOrExpired) + require.ErrorIs(t, VerifyForUser(ctx, userID2, "123456", ConsumeOnVerify), ErrMissingOrExpired) +} + +// TestVerifyForUser_ConsumeOnVerifyConcurrency pins the finding-1 contract: a +// code verified with ConsumeOnVerify authorizes exactly ONE operation. Even +// though the per-user mutex serializes the critical section (so no test can +// actually race it), the observable guarantee is that the first verify burns the +// code and any subsequent verify of the same code fails with +// ErrMissingOrExpired — two concurrent charge gates can never both pass. +func TestVerifyForUser_ConsumeOnVerifyConcurrency(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + seedPending(t, ctx, tx, userID, "424242") + + // Two "concurrent" charge-gate verifies of the same code, serialized by + // StateFor's per-user mutex exactly as the payments gate would experience + // them. Only the first may succeed. + require.NoError(t, VerifyForUser(ctx, userID, "424242", ConsumeOnVerify), "first charge gate must verify") + require.ErrorIs(t, VerifyForUser(ctx, userID, "424242", ConsumeOnVerify), ErrMissingOrExpired, + "second charge gate with the same code must fail — one code, one charge") +} + +// TestVerifyForUser_SuccessClearsLoginLockout pins LOW 6b: a successful 2FA +// verify lifts any password-guessing login lockout (users.failed_attempts / +// locked_until) because a correct code proves control of the second factor. +func TestVerifyForUser_SuccessClearsLoginLockout(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + seedPending(t, ctx, tx, userID, "123456") + + _, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = 9, locked_until = NOW() + INTERVAL '30 minutes' WHERE id = $1`, userID) + require.NoError(t, err) + + require.NoError(t, VerifyForUser(ctx, userID, "123456", ConsumeOnVerify)) + + var failedAttempts int + var lockedUntil *time.Time + require.NoError(t, tx.QueryRow(ctx, `SELECT failed_attempts, locked_until FROM users WHERE id = $1`, userID).Scan(&failedAttempts, &lockedUntil)) + require.Zero(t, failedAttempts, "successful 2FA verify must reset the login lockout counter") + require.Nil(t, lockedUntil, "successful 2FA verify must clear locked_until") } // TestConsumePendingCode pins the MEDIUM-2 contract: ConsumePendingCode NULLs diff --git a/frontend/src/lib/components/booking/BookingFlow.svelte b/frontend/src/lib/components/booking/BookingFlow.svelte index 4fc7169..7cf1f4f 100644 --- a/frontend/src/lib/components/booking/BookingFlow.svelte +++ b/frontend/src/lib/components/booking/BookingFlow.svelte @@ -41,12 +41,15 @@ import { POLICY } from '$lib/constants/policy'; import { canSaveCardsForRole, + campaignDiscountPence, + depositChargePence, isNonceStale, isOverflowTipConfirmationRequired, isTwoFactorVerificationGateFailure, submitPaymentWithRetry } from '$lib/square/square'; import UserPaymentModal from '$lib/components/payments/UserPaymentModal.svelte'; + import OverflowTipConfirm from '$lib/components/payments/OverflowTipConfirm.svelte'; import { extractBookedSlots, getLunchProtectionForSlots } from '$lib/lunchProtection'; import { useTwoFactorCodeForSavedCard } from '$lib/stores/twoFactorCode.svelte'; import { @@ -249,6 +252,13 @@ discounted_total: number; } | null>(null); + function formatCurrency(pence: number): string { + return new Intl.NumberFormat('en-GB', { + style: 'currency', + currency: 'GBP' + }).format(pence / 100); + } + // =============== Payment Functions =============== async function fetchUserDepositsRequired() { if (!authStore.isAuthenticated) { @@ -522,12 +532,23 @@ // idempotency key are NOT cleared — the confirm resend is the same // logical charge. if (!confirmOverflowTip && isOverflowTipConfirmationRequired(text)) { + // The backend's overflow guard compares against the DISCOUNTED + // remaining (remaining + eligible campaign credit), and for a + // deposit it charges req.Amount − the campaign credit (the + // frontend sends deposits raw). Both the displayed overflow and + // the amount actually charged must therefore account for the + // eligible campaign discount — mirroring UserPaymentModal so the + // two surfaces can't show different amounts for the same booking. + const depositDiscountPence = campaignDiscountPence(discountPreview); overflowConfirm = { amountPence, overflowPence: Math.max( 0, - amountPence - Math.round((confirmedBooking?.amount_due ?? 0) * 100) + amountPence - + Math.round((confirmedBooking?.amount_due ?? 0) * 100) - + depositDiscountPence ), + chargePence: Math.max(0, amountPence - depositDiscountPence), depositAmount, body }; @@ -603,6 +624,10 @@ let overflowConfirm = $state<{ amountPence: number; overflowPence: number; + // Actual amount the backend will charge. Deposits are sent RAW and the + // backend charges amountPence minus the eligible campaign credit, so + // this can differ from amountPence (mirrors UserPaymentModal). + chargePence?: number; depositAmount: number; body: Record; } | null>(null); @@ -2573,47 +2598,21 @@ -
-
- - - - -
-

Confirm extra as tip

-

- The balance for this booking has changed since it was last loaded. The extra £{( - overflowConfirm.overflowPence / 100 - ).toFixed(2)} will be recorded as a tip. Confirm to continue? -

-
-
-
- - -
-
+ remaining balance will be recorded as a tip once confirmed. + Shared markup with the customer payment modal + (OverflowTipConfirm) so the two surfaces can't drift. --> + {:else} {isProcessingPayment ? 'Processing...' - : `Pay Deposit £${calculateDepositAmount()}`} + : `Pay Deposit ${formatCurrency( + depositChargePence( + Math.round(calculateDepositAmount() * 100), + campaignDiscountPence(discountPreview) + ) + )}`} diff --git a/frontend/src/lib/components/payments/OverflowTipConfirm.svelte b/frontend/src/lib/components/payments/OverflowTipConfirm.svelte new file mode 100644 index 0000000..705f6f5 --- /dev/null +++ b/frontend/src/lib/components/payments/OverflowTipConfirm.svelte @@ -0,0 +1,72 @@ + + + +
+
+ + + + +
+

Confirm extra as tip

+

+ The balance for this booking has changed since it was last loaded. The extra + {formatCurrency(overflowPence)} will be recorded as a tip. Confirm to continue? +

+ {#if discountNote} +

{discountNote}

+ {/if} +
+
+
+ + +
+
diff --git a/frontend/src/lib/components/payments/PaymentModal.svelte b/frontend/src/lib/components/payments/PaymentModal.svelte index 4ea31bc..28e88ed 100644 --- a/frontend/src/lib/components/payments/PaymentModal.svelte +++ b/frontend/src/lib/components/payments/PaymentModal.svelte @@ -90,7 +90,8 @@ // irrelevant to the backend gate, so `enabled` is always true. const twoFactor = useTwoFactorCodeForSavedCard({ enabled: () => true, - gateActive: () => twoFactorEnforced && customerTwoFactorEnabled, + gateActive: () => + twoFactorEnforced && customerTwoFactorEnabled && selectedMethod === 'savedcard', mint: () => { const customerID = booking.user_id ?? booking.user?.id; return customerID ? adminRequestNewTwoFactorCode(customerID) : requestNewTwoFactorCode(); @@ -110,7 +111,18 @@ // by /api/admin/today/current-next carries no amount_paid/amount_due/ // payments, so this is fetched fresh from the admin booking detail endpoint // on mount and subtracted from the charge (see netTotal). + // + // Money finding 1: `amount_paid` (summed over ALL completed payments) can + // include tips — a tip is gratuity, not booking credit, so it must not + // reduce what the customer still owes. The booking detail endpoint computes + // amount_paid in Go over every completed payment (no payment_type filter), + // so the tip-excluded obligation is derived here from the payments list + // rather than trusting amount_paid. This stays consistent whether or not + // the backend starts excluding tips from amount_paid (idempotent either + // way). The tip-INCLUSIVE amount_paid is kept for the "Already paid" + // display (mirrors the customer modal's "Amount Paid" row). let amountPaidPence = $state(0); + let tipExcludedPaidPence = $state(0); async function fetchAmountPaid() { try { const resp = await apiFetch(`/api/admin/bookings/${booking.id}`); @@ -118,20 +130,29 @@ const data = await resp.json(); if (typeof data.amount_paid === 'number') { amountPaidPence = Math.round(data.amount_paid * 100); - return; } + tipExcludedPaidPence = Math.round( + (data.payments ?? []) + .filter( + (p: { status: string; payment_type: string }) => + p.status === 'completed' && p.payment_type !== 'tip' + ) + .reduce((sum: number, p: { amount: number }) => sum + (p.amount || 0), 0) * 100 + ); + return; } } catch (_err) { // fall through to the booking prop below } amountPaidPence = Math.round((booking.amount_paid ?? 0) * 100); + tipExcludedPaidPence = amountPaidPence; } const loyaltyEligible = $derived( stamps >= 10 && !(booking.discounts ?? []).some((d: BookingDiscount) => d.discount_source === 'loyalty') && booking.total_amount > 0 && - amountPaidPence === 0 + tipExcludedPaidPence === 0 ); const loyaltyDiscount = $derived( @@ -283,7 +304,10 @@ // remaining value, so the frontend charge and the backend record now agree // and a prior deposit can no longer land as an unintended tip. const netTotal = $derived( - Math.max(0, subtotal - discountSum - campaignDiscountPence(discountPreview) - amountPaidPence) + Math.max( + 0, + subtotal - discountSum - campaignDiscountPence(discountPreview) - tipExcludedPaidPence + ) ); const tipPercentages = $derived.by(() => { diff --git a/frontend/src/lib/components/payments/UserPaymentModal.svelte b/frontend/src/lib/components/payments/UserPaymentModal.svelte index 32cbb84..27e38d0 100644 --- a/frontend/src/lib/components/payments/UserPaymentModal.svelte +++ b/frontend/src/lib/components/payments/UserPaymentModal.svelte @@ -9,6 +9,7 @@ import type { Booking } from '$lib/types/booking'; import CardSelection from '$lib/components/payments/CardSelection.svelte'; import TwoFactorCodeInput from '$lib/components/payments/TwoFactorCodeInput.svelte'; + import OverflowTipConfirm from '$lib/components/payments/OverflowTipConfirm.svelte'; import PolicyPopover from '$lib/components/ui/policyPopover.svelte'; import { authStore } from '$lib/stores/auth.svelte'; import { savedCardsStore } from '$lib/stores/savedCards.svelte'; @@ -17,6 +18,7 @@ import { generateUUID } from '$lib/utils/uuid'; import { campaignDiscountPence, + depositChargePence, isNonceStale, isOverflowTipConfirmationRequired, isSavedCardVerificationRequired, @@ -723,58 +725,21 @@ -
-
- - - - -
-

Confirm extra as tip

-

- The balance for this booking has changed since it was last loaded. The extra - {formatCurrency(overflowConfirm.overflowPence)} will be recorded as a tip. Confirm to - continue? -

- {#if overflowConfirm.paymentType === 'deposit' && overflowConfirm.chargePence !== undefined} -

- An eligible campaign discount of - {formatCurrency( - Math.max(0, overflowConfirm.amountPence - overflowConfirm.chargePence) - )} - applies — you'll be charged {formatCurrency(overflowConfirm.chargePence)}. -

- {/if} -
-
-
- - -
-
+ recorded as a tip once confirmed. Shared markup with the + booking-flow deposit step (OverflowTipConfirm) so the two surfaces + can't drift. --> +

Secure payment powered by Square