diff --git a/backend/auth/jwt.go b/backend/auth/jwt.go index ac2e19e..2eb8fa3 100644 --- a/backend/auth/jwt.go +++ b/backend/auth/jwt.go @@ -7,6 +7,8 @@ import ( "fmt" "log" "log/slog" + "strings" + "sync" "time" "crussell/clock" @@ -37,7 +39,17 @@ const refreshTokenLifetimeDays = int64(RefreshTokenLifetime / (24 * time.Hour)) // grace window gets the generic error but does NOT kill the rotation family and // does NOT raise the refresh_token_reuse alert — only a replay after the window // has elapsed is treated as theft (see VerifyRefreshToken). -const refreshTokenReuseGrace = 30 * time.Second +// +// LOW-2: 60s (was 30s) because a legitimately-rotated token can be replayed +// from the SAME device well after the old 30s window when the user's second +// tab refreshes on a slower return (e.g. the tab was suspended by the OS and +// wakes up >30s after the first tab rotated). The widened grace costs a stolen +// token up to an extra 30s of freshness before reuse detection fires — an +// acceptable trade-off for not killing a legitimate session. The ideal fix +// (only kill when the replay's IP/User-Agent differs from the rotation's) +// would need the rotation origin persisted per family, which the locked schema +// cannot express today; the widened window is the safe minimum. +const refreshTokenReuseGrace = 60 * time.Second // refreshTokenReuseGraceSecs is the grace window in whole seconds for the SQL // make_interval(secs => ...) comparison in VerifyRefreshToken's reuse branch. @@ -123,6 +135,100 @@ func IsJTIRevoked(ctx context.Context, jti string) bool { return exists } +// familyAliveCacheTTL bounds how long a family-alive verdict stays cached. +// VerifyToken already runs one DB query per request for the JTI revocation +// check; the family-alive check would add a second. Caching confirmed verdicts +// for 30s turns that second query into an in-memory lookup for the common case +// (MEDIUM-1), cutting auth-path DB amplification in half. The TTL is short so a +// killed family is re-observed quickly, and the cache is explicitly invalidated +// on every family kill (VerifyRefreshToken reuse branch, LogoutHandler) so +// bound access tokens die immediately when theft is detected (HIGH 1). +const familyAliveCacheTTL = 30 * time.Second + +// familyAliveCacheMaxEntries bounds the in-memory map so a flood of distinct +// family ids cannot grow it without bound. +const familyAliveCacheMaxEntries = 10_000 + +// familyAliveCacheEntry is one cached family-alive verdict. Only DB-CONFIRMED +// results are ever stored — a failed query fails open and is never cached, so a +// transient outage cannot freeze a stale rejection or admission into the cache. +type familyAliveCacheEntry struct { + alive bool + expires time.Time +} + +// familyAliveCache is a mutex-guarded, bounded cache of family-alive verdicts +// keyed by "|" (the family belongs to one user, but the +// composite key keeps the verdict aligned with the SQL conjunct). +var familyAliveCache struct { + mu sync.Mutex + m map[string]familyAliveCacheEntry +} + +func init() { + familyAliveCache.m = make(map[string]familyAliveCacheEntry) +} + +// familyAliveLookup returns a cached verdict for a family key and whether it is +// still fresh, evicting expired entries opportunistically. +func familyAliveLookup(key string) (alive bool, ok bool) { + familyAliveCache.mu.Lock() + defer familyAliveCache.mu.Unlock() + e, ok := familyAliveCache.m[key] + if !ok { + return false, false + } + if clock.Now().After(e.expires) { + delete(familyAliveCache.m, key) + return false, false + } + return e.alive, true +} + +// familyAliveStore records a DB-confirmed verdict, evicting expired entries +// and then the oldest live entry when the cache is at capacity. +func familyAliveStore(key string, alive bool) { + familyAliveCache.mu.Lock() + defer familyAliveCache.mu.Unlock() + now := clock.Now() + e := familyAliveCacheEntry{alive: alive, expires: now.Add(familyAliveCacheTTL)} + if len(familyAliveCache.m) >= familyAliveCacheMaxEntries { + for k, ce := range familyAliveCache.m { + if now.After(ce.expires) { + delete(familyAliveCache.m, k) + } + } + } + if len(familyAliveCache.m) >= familyAliveCacheMaxEntries { + var oldestKey string + var oldestAt time.Time + for k, ce := range familyAliveCache.m { + if oldestKey == "" || ce.expires.Before(oldestAt) { + oldestKey, oldestAt = k, ce.expires + } + } + delete(familyAliveCache.m, oldestKey) + } + familyAliveCache.m[key] = e +} + +// InvalidateFamilyAlive drops every cached verdict for a family so the next +// VerifyToken re-queries the DB. Called whenever a rotation family is deleted +// (refresh-token reuse kill, logout) so bound access tokens die on their next +// verification instead of riding the cache TTL (HIGH 1). +func InvalidateFamilyAlive(familyID string) { + if familyID == "" { + return + } + familyAliveCache.mu.Lock() + defer familyAliveCache.mu.Unlock() + for k := range familyAliveCache.m { + if strings.HasPrefix(k, familyID+"|") { + 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 { @@ -247,10 +353,14 @@ func VerifyToken(tokenString string, ctx context.Context) (userID string, role s // verifyFamilyAlive rejects access tokens bound to a rotation family that no // longer exists in refresh_tokens. A token WITHOUT a family_id claim is unbound -// (minted via GenerateToken — tests/legacy callers) and passes. Fails closed on -// a live-DB query error: when the family cannot be confirmed alive the safe -// default for money endpoints is to refuse. Mirrors IsJTIRevoked's nil-db -// availability default (startup/unit tests). +// (minted via GenerateToken — tests/legacy callers) and passes. DB-confirmed +// verdicts are cached for familyAliveCacheTTL (MEDIUM-1) so VerifyToken does +// not run a second query per request; the cache is invalidated on family kills +// so a killed family's access tokens die on their next verification (HIGH 1). +// On a live-DB query error the check FAILS OPEN with a WARN log — matching +// IsJTIRevoked — because genuine theft is already handled by the family kill in +// VerifyRefreshToken's reuse branch, and a transient DB error must not turn +// into a total 401 outage for every authenticated request. func verifyFamilyAlive(ctx context.Context, token jwtClaimGetter, userID string) error { var familyVal any if err := token.Get(accessTokenFamilyClaim, &familyVal); err != nil { @@ -263,20 +373,51 @@ func verifyFamilyAlive(ctx context.Context, token jwtClaimGetter, userID string) if db.Conn == nil { return nil } + key := familyID + "|" + userID + if alive, cached := familyAliveLookup(key); cached { + if !alive { + return fmt.Errorf("token revoked") + } + return nil + } var exists bool err := db.Conn.QueryRow(ctx, `SELECT EXISTS( SELECT 1 FROM refresh_tokens WHERE family_id = $1 AND user_id = $2 )`, familyID, userID).Scan(&exists) if err != nil { - return fmt.Errorf("token revoked") + slog.Warn("family-alive check failed — failing open (access token admitted)", "family_id", familyID, "err", err) + return nil } + familyAliveStore(key, exists) if !exists { return fmt.Errorf("token revoked") } return nil } +// FamilyIDFromToken returns the access token's family_id claim, or "" when the +// token carries none (unbound — test/legacy minting via GenerateToken). It only +// DECODES the token without re-verifying the signature; callers must only use +// it on a token that already passed VerifyToken (e.g. RequireAuth middleware). +// Used by LogoutHandler to scope refresh-token revocation to the presented +// session's rotation family (LOW-1). +func FamilyIDFromToken(tokenString string) string { + if TokenAuth == nil || tokenString == "" { + return "" + } + decoded, err := TokenAuth.Decode(tokenString) + if err != nil { + return "" + } + var fv any + if err := decoded.Get(accessTokenFamilyClaim, &fv); err != nil { + return "" + } + familyID, _ := fv.(string) + return familyID +} + // jwtClaimGetter is the minimal subset of jwt.Token needed to read a claim // (the token returned by jwtauth.VerifyToken). Kept as an interface so the // lestrrat-go/jwx dependency stays out of this file's imports. @@ -436,6 +577,11 @@ func VerifyRefreshToken(ctx context.Context, tokenString string) (userID string, // (i) Revoke the ENTIRE family — the reused token and every descendant. if _, err := tx.Exec(ctx, `DELETE FROM refresh_tokens WHERE family_id = $1`, reusedFamilyID); err != nil { slog.Error("CRITICAL: refresh token reuse detected but family revocation failed", "userID", reusedUserID, "familyID", reusedFamilyID, "err", err) + } else { + // The family is gone — drop its cached verdict so bound access + // tokens die on their next verification instead of riding the + // family-alive cache TTL (HIGH 1). + InvalidateFamilyAlive(reusedFamilyID) } // (ii) Surface the theft in the admin notification centre. The NOT // EXISTS guard keeps ONE alert per reused family until an admin diff --git a/backend/auth/jwt_test.go b/backend/auth/jwt_test.go index 498494c..a583bd7 100644 --- a/backend/auth/jwt_test.go +++ b/backend/auth/jwt_test.go @@ -519,13 +519,13 @@ func TestVerifyRefreshToken_ReuseRevokesFamilyAndAlerts(t *testing.T) { t.Fatalf("expected 2 refresh tokens in family, got %d", famCount) } - // MEDIUM 4 grace window: backdate the original's used_at past the 30s - // reuse grace so the replay below is genuine theft. WITHOUT this, a replay + // Reuse grace window: backdate the original's used_at past the 60s reuse + // grace so the replay below is genuine theft. WITHOUT this, a replay // moments after rotation is a benign two-tab concurrent refresh and the // family must NOT be killed. if _, err := tx.Exec(ctx, ` UPDATE refresh_tokens - SET used_at = NOW() - make_interval(secs => 60) + SET used_at = NOW() - make_interval(secs => 120) WHERE token_hash = encode(sha256($1::bytea), 'hex') `, original); err != nil { t.Fatalf("failed to backdate used_at for reuse test: %v", err) @@ -572,8 +572,8 @@ func TestVerifyRefreshToken_ReuseRevokesFamilyAndAlerts(t *testing.T) { } } -// TestVerifyRefreshToken_ReplayWithinGrace_IsBenign verifies the MEDIUM 4 -// hardening: a used-token replay WITHIN the 30s grace window (two tabs sharing +// TestVerifyRefreshToken_ReplayWithinGrace_IsBenign verifies the reuse +// hardening: a used-token replay WITHIN the 60s grace window (two tabs sharing // one localStorage refresh token both refreshing on load) is a benign // concurrent refresh — the generic error is returned, but the rotation family // survives and no refresh_token_reuse alert is raised. @@ -679,7 +679,7 @@ func TestAccessTokenKilledWithRotationFamily(t *testing.T) { // detected, whole family DELETEd. if _, err := tx.Exec(ctx, ` UPDATE refresh_tokens - SET used_at = NOW() - make_interval(secs => 60) + SET used_at = NOW() - make_interval(secs => 120) WHERE token_hash = encode(sha256($1::bytea), 'hex') `, original); err != nil { t.Fatalf("failed to backdate used_at: %v", err) diff --git a/backend/handlers/auth/auth_test.go b/backend/handlers/auth/auth_test.go index 8d058bc..c06ea35 100644 --- a/backend/handlers/auth/auth_test.go +++ b/backend/handlers/auth/auth_test.go @@ -1414,6 +1414,110 @@ func TestLogoutHandler_InvalidToken(t *testing.T) { } } +// TestLogoutHandler_RevokesPresentedFamilyOnly pins the LOW-1 fix: logout must +// revoke the refresh tokens of the PRESENTED access token's rotation family +// (family_id claim), NOT every refresh token the user holds on other devices — +// a stolen access token must not be able to wipe all sessions. A second, +// unrelated rotation family for the same user survives the logout. +func TestLogoutHandler_RevokesPresentedFamilyOnly(t *testing.T) { + t.Parallel() + ctx, tx := resetTestData(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create test user: %v", err) + } + + // Two independent rotation families for the same user: A (the one being + // logged out) and B (a session on another device that must survive). + _, familyA, err := auth.GenerateRefreshToken(ctx, userID, "verified_email") + if err != nil { + t.Fatalf("failed to generate refresh token A: %v", err) + } + _, familyB, err := auth.GenerateRefreshToken(ctx, userID, "verified_email") + if err != nil { + t.Fatalf("failed to generate refresh token B: %v", err) + } + if familyA == familyB { + t.Fatal("expected two distinct rotation families") + } + + // The presented access token is bound to family A (as LoginHandler and + // RefreshTokenHandler mint it). + token, jti, err := auth.GenerateTokenForFamily(userID, "verified_email", familyA) + if err != nil { + t.Fatalf("failed to generate family-bound access token: %v", err) + } + + req := httptest.NewRequest("POST", "/api/logout", nil) + req.Header.Set("Authorization", "Bearer "+token) + reqCtx := context.WithValue(ctx, mw.JTIKey, jti) + reqCtx = context.WithValue(reqCtx, mw.UserIDKey, userID) + req = req.WithContext(reqCtx) + w := httptest.NewRecorder() + LogoutHandler(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("logout failed: %d. body: %s", w.Code, w.Body.String()) + } + + countFamily := func(familyID string) int { + var n int + if err := tx.QueryRow(ctx, `SELECT COUNT(*) FROM refresh_tokens WHERE family_id = $1`, familyID).Scan(&n); err != nil { + t.Fatalf("failed to count family rows: %v", err) + } + return n + } + if got := countFamily(familyA); got != 0 { + t.Errorf("expected family A (presented token) to be revoked after logout, got %d rows", got) + } + if got := countFamily(familyB); got != 1 { + t.Errorf("expected family B (other device session) to survive logout, got %d rows", got) + } +} + +// TestLogoutHandler_UnboundToken_RevokesUserWide pins the LOW-1 fallback: an +// access token WITHOUT a family_id claim (test/legacy minting via +// GenerateToken) cannot be scoped, so logout falls back to the historical +// user-wide refresh-token delete. +func TestLogoutHandler_UnboundToken_RevokesUserWide(t *testing.T) { + t.Parallel() + ctx, tx := resetTestData(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create test user: %v", err) + } + if _, _, err := auth.GenerateRefreshToken(ctx, userID, "verified_email"); err != nil { + t.Fatalf("failed to generate refresh token: %v", err) + } + + token, jti, err := auth.GenerateToken(userID, "verified_email") + if err != nil { + t.Fatalf("failed to generate unbound access token: %v", err) + } + + req := httptest.NewRequest("POST", "/api/logout", nil) + req.Header.Set("Authorization", "Bearer "+token) + reqCtx := context.WithValue(ctx, mw.JTIKey, jti) + reqCtx = context.WithValue(reqCtx, mw.UserIDKey, userID) + req = req.WithContext(reqCtx) + w := httptest.NewRecorder() + LogoutHandler(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("logout failed: %d. body: %s", w.Code, w.Body.String()) + } + + var n int + if err := tx.QueryRow(ctx, `SELECT COUNT(*) FROM refresh_tokens WHERE user_id = $1`, userID).Scan(&n); err != nil { + t.Fatalf("failed to count refresh tokens: %v", err) + } + if n != 0 { + t.Errorf("expected all refresh tokens revoked for an unbound token, got %d rows", n) + } +} + // ============================================================================= // Refresh Token JTI Tests // ============================================================================= diff --git a/backend/handlers/auth/local.go b/backend/handlers/auth/local.go index 47a9a02..78fd98f 100644 --- a/backend/handlers/auth/local.go +++ b/backend/handlers/auth/local.go @@ -576,11 +576,28 @@ func LogoutHandler(w http.ResponseWriter, r *http.Request) { return } - // B5: logging out must also kill every outstanding refresh token for this - // user, or a previously-issued (possibly stolen) refresh token would keep - // the session alive past logout. The 90-day credential is deleted from - // refresh_tokens, so no refresh request after logout can succeed. - if userID != "" { + // B5: logging out must also kill the outstanding refresh credential for + // THIS session, or a previously-issued (possibly stolen) refresh token + // would keep the session alive past logout. + // + // LOW-1: the revocation is scoped to the PRESENTED access token's rotation + // family (family_id claim) instead of every refresh token the user holds. + // A stolen access token can no longer wipe every session the user keeps on + // other devices — only this token's own lineage dies, which is all B5 + // needs (the presented refresh token lives in that family). An unbound + // token (no family_id claim — test/legacy minting via GenerateToken) + // falls back to the user-wide delete, preserving the old behaviour for + // those tokens. + familyID := auth.FamilyIDFromToken(strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ")) + if familyID != "" { + if _, err := db.Conn.Exec(r.Context(), `DELETE FROM refresh_tokens WHERE family_id = $1`, familyID); err != nil { + slog.Error("logout: failed to revoke refresh token family", "familyID", familyID, "err", err) + } + // Drop the family-alive cache verdict so this family's access tokens + // (the logged-out one and any in-flight duplicates) are re-checked + // against the now-empty refresh_tokens on their next request. + auth.InvalidateFamilyAlive(familyID) + } else if userID != "" { if _, err := db.Conn.Exec(r.Context(), `DELETE FROM refresh_tokens WHERE user_id = $1`, userID); err != nil { slog.Error("logout: failed to revoke refresh tokens", "userID", userID, "err", err) } diff --git a/backend/handlers/payments/completion.go b/backend/handlers/payments/completion.go index f8ee041..3990c96 100644 --- a/backend/handlers/payments/completion.go +++ b/backend/handlers/payments/completion.go @@ -80,6 +80,16 @@ func ApplyBookingCompletionSideEffects(ctx context.Context, tx pgx.Tx, bookingID var newStampCount int if bookingTotal > 0 && !loyaltyAppliedOnThisBooking { + // Loop B MEDIUM (stamp farming via refund + re-charge): the stamp must + // be awarded at most ONCE per booking, no matter how many times the + // booking is re-completed. A refund never moves the booking out of + // 'in_progress', so a re-payment re-completes it — without this guard + // each in_progress→completed transition would re-award a stamp with no + // net merchant cash flow. The bookings.loyalty_stamp_awarded_at marker + // blocks a booking that already earned its stamp; the marker is written + // (same tx) only when the award actually landed, so a daily-cap-blocked + // completion does not permanently forfeit the booking's stamp. The + // existing "no OTHER completed booking within a day" cap is kept. if err := tx.QueryRow(ctx, ` UPDATE users SET loyalty_stamps = loyalty_stamps + 1 @@ -91,6 +101,10 @@ func ApplyBookingCompletionSideEffects(ctx context.Context, tx pgx.Tx, bookingID AND b.updated_at >= CURRENT_DATE - INTERVAL '1 day' AND b.id != $2 ) + AND NOT EXISTS ( + SELECT 1 FROM bookings b + WHERE b.id = $2 AND b.loyalty_stamp_awarded_at IS NOT NULL + ) RETURNING loyalty_stamps `, userID, bookingID).Scan(&newStampCount); err != nil { if !errors.Is(err, pgx.ErrNoRows) { @@ -98,6 +112,14 @@ func ApplyBookingCompletionSideEffects(ctx context.Context, tx pgx.Tx, bookingID } } } + if newStampCount > 0 { + if _, err := tx.Exec(ctx, ` + UPDATE bookings SET loyalty_stamp_awarded_at = NOW() + WHERE id = $1 AND loyalty_stamp_awarded_at IS NULL + `, bookingID); err != nil { + log.Printf("Failed to mark loyalty stamp awarded for booking %s: %v", bookingID, err) + } + } // Create pending redemption when stamps reach LoyaltyStampCost if newStampCount == LoyaltyStampCost { diff --git a/backend/handlers/payments/giftcards.go b/backend/handlers/payments/giftcards.go index 567c55a..5ba0176 100644 --- a/backend/handlers/payments/giftcards.go +++ b/backend/handlers/payments/giftcards.go @@ -18,6 +18,7 @@ import ( "crussell/clock" "crussell/db" "crussell/internal/square" + "crussell/internal/twofa" "crussell/internal/validators" "crussell/mw" @@ -1468,8 +1469,12 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { // and SAVING a new card during this purchase (SaveCard), mirroring // CreateBookingPayment/CreateTipPayment. A one-off new-card (nonce) charge // that is not saved is 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. if (req.CardID != nil && *req.CardID != "") || req.SaveCard { - if !requireTwoFactorForCardAccess(w, r, paymentService, userID, req.VerificationCode) { + if !requireTwoFactorForCardAccess(w, r, paymentService, userID, req.VerificationCode, false) { return } } @@ -1720,6 +1725,19 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { return } + // MEDIUM-2: a saved-card gift-card purchase reached its terminal SUCCESS + // state — consume the verified 2FA code now, inside the transaction that + // records the completed charge (the gate verified without consuming, so a + // failed/ambiguous Square charge did not burn the code and a same-key + // retry could re-verify the SAME code). + if (req.CardID != nil && *req.CardID != "") || req.SaveCard { + if consErr := twofa.ConsumePendingCode(ctx, issueTx, userID); consErr != nil { + log.Printf("CRITICAL: Square payment succeeded (ID=%s) but consuming the 2FA code for user %s failed: %v — manual reconciliation required", paymentResult.SquarePayID, userID, consErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + } + var cardID string expiryMonths, err := GetGiftCardExpiryMonths(ctx, issueTx) diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index abc3cfe..d5f9675 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -5,6 +5,7 @@ import ( "crussell/clock" "crussell/db" "crussell/internal/square" + "crussell/internal/twofa" "crussell/internal/validators" "crussell/mw" "crypto/rand" @@ -25,6 +26,45 @@ import ( "github.com/jackc/pgx/v5" ) +// insertAdminAuditCharge records an admin-initiated saved-card charge in +// admin_audit_log (MEDIUM-3a). Mirrors the balance_check audit in +// giftcards.go:1239-1243 — 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 or a completed charge. +func insertAdminAuditCharge(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) + } +} + type CreateTerminalPaymentRequest struct { Amount int64 `json:"amount" validate:"required,gt=0"` PaymentType string `json:"payment_type" validate:"required"` @@ -365,6 +405,23 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { return } + // A3 (mirror of CreateBookingPayment at handlers.go:1596): tips have a + // dedicated endpoint (POST /api/bookings/{id}/tip, CreateTipPayment) which + // enforces the M4 "tips only after the service starts" gate, and the + // tip-enabled terminal overflow carve (B3, tip_enabled) records explicit + // gratuity as its own payment_type='tip' row. A bare payment_type='tip' + // here would record the ENTIRE charge as a tip — and every "is paid" + // computation excludes tip rows (paid_total, GetBookingPaymentInfo, + // bookingIsFullyPaid, GetBookingRefundableAmountPence) — so the booking + // would never be credited and a later legitimate charge would double-collect. + // Reject it BEFORE any charge-path branch (cash/giftcard, saved_card, + // terminal checkout) so all four sub-paths are closed at once. + if req.PaymentType == "tip" { + log.Printf("Payment rejected: booking %s payment_type 'tip' is not allowed via /payment — tips use the dedicated /tip endpoint", bookingID) + http.Error(w, "Tips can only be added via the dedicated tip endpoint after the booking has started", http.StatusBadRequest) + return + } + service := NewPaymentService() amount := req.Amount @@ -430,47 +487,111 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { return } - // B3: clamp the recorded amount to the booking's remaining obligation. - // The booking row FOR UPDATE lock above serializes concurrent cash/ - // giftcard payments on this booking, so this read races no same-method - // payment. A fully-paid booking is rejected below (nothing left to - // record). - effectiveAmount, remaining, clamped, cErr := clampTerminalChargeToRemainingBalance(r.Context(), bookingID, amount) - if cErr != nil { - log.Printf("Failed to compute remaining balance for terminal %s payment on booking %s: %v", *req.PaymentMethod, bookingID, cErr) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } - if clamped && effectiveAmount <= 0 { - // B3: the clamp zeroed the amount because the booking is fully paid - // (remaining <= 0). Reject rather than record a phantom £0 payment — - // an overpayment is handled manually at the counter, not minted - // into the ledger. - log.Printf("Terminal %s payment on booking %s rejected: booking already fully paid (remaining %d pence, requested %d pence)", *req.PaymentMethod, bookingID, remaining, amount) - http.Error(w, "Booking is already fully paid", http.StatusBadRequest) - return - } - if clamped { - log.Printf("Terminal %s payment on booking %s clamped from %d to %d pence (remaining obligation) — the frontend PaymentModal sent an amount that ignored prior payments; the customer is charged the remaining obligation only", *req.PaymentMethod, bookingID, amount, effectiveAmount) - amount = effectiveAmount - } - - amountPounds := float64(amount) / 100.0 - var paymentID string - - if *req.PaymentMethod == "cash" { - err = tx.QueryRow(r.Context(), ` - INSERT INTO payments ( - booking_id, payment_type, payment_method, status, amount, idempotency_key, created_by, created_at, updated_at - ) VALUES ($1, $2, 'cash', 'completed', $3, $4, $5, NOW(), NOW()) - RETURNING id - `, bookingID, req.PaymentType, amountPounds, idempotencyKey, adminID).Scan(&paymentID) - if err != nil { - log.Printf("Failed to create cash payment record: %v", err) + // B3: clamp the recorded amount to the booking's remaining obligation + // unless the customer explicitly requested a tip (tip_enabled) — mirror + // the card-terminal tip bound below: the booking portion can never + // exceed what is owed, and the tip portion can never exceed + // maxTerminalTipPence. The booking row FOR UPDATE lock above serializes + // concurrent cash/giftcard payments on this booking, so this read races + // no same-method payment. A fully-paid no-tip booking is rejected below + // (nothing left to record). + var remaining int64 + if req.TipEnabled { + remainingPence, remErr := service.GetBookingRemainingBalancePence(r.Context(), bookingID) + if remErr != nil { + log.Printf("Failed to compute remaining balance for tip-enabled terminal %s payment on booking %s: %v", *req.PaymentMethod, bookingID, remErr) http.Error(w, "internal server error", http.StatusInternalServerError) return } - ApplyVATToBookingPayment(r.Context(), tx, paymentID) + remaining = remainingPence + maxChargePence := remainingPence + maxTerminalTipPence + if amount > maxChargePence { + log.Printf("Terminal %s payment for booking %s clamped from %d to %d pence (remaining obligation %d + max tip bound £%.2f) — the requested total exceeded the booking remainder plus the tip cap", *req.PaymentMethod, bookingID, amount, maxChargePence, remainingPence, float64(maxTerminalTipPence)/100.0) + amount = maxChargePence + } + } else { + effectiveAmount, remaining, clamped, cErr := clampTerminalChargeToRemainingBalance(r.Context(), bookingID, amount) + if cErr != nil { + log.Printf("Failed to compute remaining balance for terminal %s payment on booking %s: %v", *req.PaymentMethod, bookingID, cErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + if clamped && effectiveAmount <= 0 { + // B3: the clamp zeroed the amount because the booking is fully paid + // (remaining <= 0). Reject rather than record a phantom £0 payment — + // an overpayment is handled manually at the counter, not minted + // into the ledger. + log.Printf("Terminal %s payment on booking %s rejected: booking already fully paid (remaining %d pence, requested %d pence)", *req.PaymentMethod, bookingID, remaining, amount) + http.Error(w, "Booking is already fully paid", http.StatusBadRequest) + return + } + if clamped { + log.Printf("Terminal %s payment on booking %s clamped from %d to %d pence (remaining obligation) — the frontend PaymentModal sent an amount that ignored prior payments; the customer is charged the remaining obligation only", *req.PaymentMethod, bookingID, amount, effectiveAmount) + amount = effectiveAmount + } + } + + // M4 (mirror of the card-terminal carve at sweep.go): when the customer + // explicitly requested a tip, any part of the charged amount beyond the + // remaining booking value is gratuity and must be recorded as its own + // payment_type='tip' row — never absorbed into the booking payment + // (which would over-credit the booking) nor rejected. The booking + // portion keeps the requested payment type, exactly as the non-tip + // cash/giftcard flow records it. + tipPortion := int64(0) + bookingPortion := amount + if req.TipEnabled && remaining < amount { + tipPortion = amount - remaining + bookingPortion = remaining + } + + amountPounds := float64(amount) / 100.0 + bookingPortionPounds := float64(bookingPortion) / 100.0 + tipPounds := float64(tipPortion) / 100.0 + var paymentID string + + if *req.PaymentMethod == "cash" { + if bookingPortionPounds > 0.004 { + err = tx.QueryRow(r.Context(), ` + INSERT INTO payments ( + booking_id, payment_type, payment_method, status, amount, idempotency_key, created_by, created_at, updated_at + ) VALUES ($1, $2, 'cash', 'completed', $3, $4, $5, NOW(), NOW()) + RETURNING id + `, bookingID, req.PaymentType, bookingPortionPounds, idempotencyKey, adminID).Scan(&paymentID) + if err != nil { + log.Printf("Failed to create cash payment record: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + ApplyVATToBookingPayment(r.Context(), tx, paymentID) + } + // The tip carve is recorded as its own 'tip' row so the booking + // portion is the only money that counts toward the obligation. + // When the charge is tip-only (fully-paid booking), the tip row is + // the ONLY record and its id is returned as the checkout id, + // mirroring the card-terminal carve (primary := records[0]). + if tipPounds > 0.004 { + tipKey := splitIdempotencyKey(idempotencyKey, "-split-tip") + tipID, tipErr := service.CreatePaymentRecordTx(r.Context(), tx, PaymentRecord{ + BookingID: bookingID, + PaymentType: "tip", + PaymentMethod: "cash", + Status: "completed", + Amount: tipPounds, + IdempotencyKey: &tipKey, + CreatedBy: &adminID, + CreatedAt: clock.Now(), + UpdatedAt: clock.Now(), + }, nil) + if tipErr != nil { + log.Printf("Failed to create cash tip payment record: %v", tipErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + if paymentID == "" { + paymentID = tipID + } + } } else { // giftcard var customerID sql.NullString err = tx.QueryRow(r.Context(), "SELECT user_id FROM bookings WHERE id = $1", bookingID).Scan(&customerID) @@ -567,29 +688,63 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { giftCardPaymentID = &cleanCardID } - err = tx.QueryRow(r.Context(), ` - INSERT INTO payments ( - booking_id, payment_type, payment_method, status, amount, idempotency_key, created_by, created_at, updated_at, gift_card_id - ) VALUES ($1, $2, 'giftcard', 'completed', $3, $4, $5, NOW(), NOW(), $6) - RETURNING id - `, bookingID, req.PaymentType, amountPounds, idempotencyKey, adminID, giftCardPaymentID).Scan(&paymentID) - if err != nil { - log.Printf("Failed to create giftcard payment record: %v", err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return + // The gift-card source funds the full charged amount (booking + // portion + tip). The primary row records the booking portion; the + // tip carve is recorded as its own 'tip' row sourced from the same + // gift card (C3 source-of-funds tracking), so the booking portion + // is the only money that counts toward the obligation. + bookingPayID := "" + if bookingPortionPounds > 0.004 { + err = tx.QueryRow(r.Context(), ` + INSERT INTO payments ( + booking_id, payment_type, payment_method, status, amount, idempotency_key, created_by, created_at, updated_at, gift_card_id + ) VALUES ($1, $2, 'giftcard', 'completed', $3, $4, $5, NOW(), NOW(), $6) + RETURNING id + `, bookingID, req.PaymentType, bookingPortionPounds, idempotencyKey, adminID, giftCardPaymentID).Scan(&bookingPayID) + if err != nil { + log.Printf("Failed to create giftcard payment record: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + paymentID = bookingPayID + } + if tipPounds > 0.004 { + tipKey := splitIdempotencyKey(idempotencyKey, "-split-tip") + tipID, tipErr := service.CreatePaymentRecordTx(r.Context(), tx, PaymentRecord{ + BookingID: bookingID, + PaymentType: "tip", + PaymentMethod: "giftcard", + Status: "completed", + Amount: tipPounds, + IdempotencyKey: &tipKey, + CreatedBy: &adminID, + CreatedAt: clock.Now(), + UpdatedAt: clock.Now(), + }, giftCardPaymentID) + if tipErr != nil { + log.Printf("Failed to create giftcard tip payment record: %v", tipErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + if paymentID == "" { + paymentID = tipID + } } // Apply VAT at redemption only if the gift card was purchased as MPV // (VAT deferred to redemption). For SPV, VAT was already paid at sale. // For account balance payments (usedBalance=true), VAT was already paid - // when the original card was purchased. - if usedBalance { - // VAT already paid at purchase time — nothing to do here. - } else if cardVoucherType == "MPV" { - vatCfg, vatErr := GetVATConfig(r.Context(), tx) - if vatErr == nil && vatCfg.IsVATRegistered { - if _, vatExecErr := tx.Exec(r.Context(), "SELECT apply_vat_to_payment($1, $2)", paymentID, vatCfg.DefaultVATRate); vatExecErr != nil { - log.Printf("Failed to apply VAT to giftcard payment %s: %v", paymentID, vatExecErr) + // when the original card was purchased. Applied to the booking-portion + // payment only — a tip record is never VAT-applicable. + if bookingPayID != "" { + if usedBalance { + // VAT already paid at purchase time — nothing to do here. + } else if cardVoucherType == "MPV" { + vatCfg, vatErr := GetVATConfig(r.Context(), tx) + if vatErr == nil && vatCfg.IsVATRegistered { + if _, vatExecErr := tx.Exec(r.Context(), "SELECT apply_vat_to_payment($1, $2)", bookingPayID, vatCfg.DefaultVATRate); vatExecErr != nil { + log.Printf("Failed to apply VAT to giftcard payment %s: %v", bookingPayID, vatExecErr) + } } } } @@ -794,7 +949,11 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // existing result WITHOUT demanding a fresh code — no new money moves, // so no new authorization is needed. Pending-reuse retries and fresh // charges still pass through the gate. - if bookingUserID.Valid && !requireTwoFactorForCardAccess(w, r, service, bookingUserID.String, req.VerificationCode) { + // 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. + if bookingUserID.Valid && !requireTwoFactorForCardAccess(w, r, service, bookingUserID.String, req.VerificationCode, false) { return } @@ -993,6 +1152,19 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal server error", http.StatusInternalServerError) return } + // MEDIUM-2: the charge reached its terminal SUCCESS state — consume the + // verified 2FA code now, INSIDE the transaction that records the + // completed charge (the gate verified without consuming so a failed + // charge would not burn the code). A failure here fails the whole + // transaction (the row stays pending and the sweep reconciles), which is + // the same known failure mode as any other post-charge tx error. + if bookingUserID.Valid { + if consErr := twofa.ConsumePendingCode(r.Context(), recheckTx, bookingUserID.String); consErr != nil { + log.Printf("CRITICAL: Square payment %s succeeded but consuming the 2FA code for user %s failed: %v — manual reconciliation required", paymentResult.SquarePayID, bookingUserID.String, consErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + } // B14: apply VAT to the saved-card terminal charge, inside the same // transaction as the completed flip (like the booking path at 2021-2028 // and the cash path at 397). Without this the saved-card branch never @@ -1008,6 +1180,20 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { return } + // MEDIUM-3a: record the admin-initiated saved-card charge in + // admin_audit_log (mirroring giftcards.go's balance_check audit). Runs + // best-effort AFTER the money transaction commits so an audit-write + // failure can never roll back a completed charge. + if bookingUserID.Valid { + insertAdminAuditCharge(r.Context(), adminID, bookingUserID.String, "saved_card_charge", map[string]any{ + "booking_id": bookingID, + "payment_id": paymentID, + "amount": float64(amount) / 100.0, + "card_last4": paymentResult.CardLast4, + "square_payment_id": paymentResult.SquarePayID, + }) + } + // F6: a fully-paid saved-card charge completes the booking exactly like // the terminal path (recordTerminalPaymentTx → completeFullyPaidBooking, // sweep.go:1632). Runs in its OWN transaction after the status commit @@ -1563,11 +1749,6 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { service := NewPaymentService() - // 2FA gating (C5): persisting a card requires 2FA when the feature is enforced. - if req.SaveCard && !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode) { - return - } - // Resolve buyer email for Square receipt delivery (failure is non-fatal). var bookingBuyerEmail string if err := db.Conn.QueryRow(r.Context(), `SELECT email FROM users WHERE id = $1`, userID).Scan(&bookingBuyerEmail); err != nil { @@ -1872,6 +2053,18 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { log.Printf("Failed to check idempotency: %v", err) } + // 2FA gating (C5): persisting a card requires 2FA when the feature is + // enforced. This runs AFTER the idempotency dedup's completed + // short-circuit (Loop B MEDIUM): a same-key lost-response retry returns the + // already-completed payment above without re-entering the gate, so its + // single-use code (already consumed by the original attempt) is never + // re-rejected as "expired". Pending-reuse and fresh paths still gate — a + // new charge may move at Square. The gate also runs before + // resolveChargeSource below, so an un-2FA'd request never persists a card. + if req.SaveCard && !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode, true) { + return + } + // After the idempotency check (which handles same-key retries), verify // that no completed payment of the same non-partial type already exists. // buildSplitRecords converts 'full' and 'deposit' input types into a @@ -2005,9 +2198,13 @@ 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. + // 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. if req.CardID != nil && *req.CardID != "" { - if !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode) { + if !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode, false) { return } } @@ -2268,6 +2465,21 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { return } + // 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. + 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", + paymentResult.Status, paymentResult.SquarePayID, userID, consErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + } + // Insert the additional split records. They carry the derived -split-N // idempotency keys, which are new rows; if the split produced only one // record, there is nothing more to insert. @@ -2825,8 +3037,10 @@ func CreatePaymentMethod(w http.ResponseWriter, r *http.Request) { // requires 2FA when the feature is enforced — the same gate the booking // and tip flows apply to req.SaveCard. Persisting a stored credential is // exactly what the PSD2 SCA stand-in protects, so the dedicated save-card - // endpoint must not be the un-gated side door. - if !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode) { + // endpoint must not be the un-gated side door. consume=true: saving a card + // is a terminal operation with no downstream charge to attach consumption + // to (MEDIUM-2). + if !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode, true) { return } @@ -2962,6 +3176,21 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { return } + // A tip payment is gratuity, not booking money: every refund computation + // excludes tip rows (GetBookingRefundableAmountPence, refunds.go, and the + // AdminRefundBooking query at handlers.go:3597). Refunding a tip here would + // pay the gratuity back while GetBookingRemainingBalancePence's refunded + // total re-opens booking charge capacity (a tip refund counts as "returned + // money") — a fully-paid booking would accept a second legitimate charge, + // double-collecting the balance. Tips are deliberately not refundable via + // this handler. Tip split rows share the charge's square_payment_id, so the + // Square reference guard below cannot catch them — this explicit check must + // run before it. + if payment.PaymentType == "tip" { + http.Error(w, "Cannot refund a tip payment", http.StatusBadRequest) + return + } + // Money-safety guard (M7): a payments row with NO booking is a gift-card // purchase (BuyGiftCard inserts without a booking — the same discriminator // the sweep uses in sweep.go). Refunding such a payment at Square returns @@ -3910,7 +4139,11 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { service := NewPaymentService() // 2FA gating (C5): persisting a card requires 2FA when the feature is enforced. - if req.SaveCard && !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode) { + // consume=true: saving a card is a terminal operation (the card row is + // created right here), so the verified code is single-use immediately — + // unlike the saved-card CHARGE gate below, which defers consumption to the + // charge's terminal success (MEDIUM-2). + if req.SaveCard && !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode, true) { return } @@ -4008,13 +4241,6 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { var sourceID string var savedCardID *string var savedCardCustomerID string - // 2FA gating (C5): charging a SAVED card requires 2FA when the feature is - // enforced. New-card (nonce) charges are not gated. - if req.CardID != nil && *req.CardID != "" { - if !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode) { - return - } - } sourceID, savedCardID, savedCardCustomerID, sourceOK := resolveChargeSource(r.Context(), w, service, userID, req.NewCardToken, req.CardID, req.SaveCard, "Card not found") if !sourceOK { return @@ -4144,6 +4370,19 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { log.Printf("Failed to check tip idempotency: %v", err) } + // 2FA gating (C5): charging a SAVED card requires 2FA when the feature is + // enforced. New-card (nonce) charges are not gated. This runs AFTER the + // idempotency dedup's completed short-circuit (Loop B MEDIUM): a same-key + // lost-response retry returns the already-completed payment above without + // re-entering the gate, so its single-use code (already consumed by the + // original attempt) is never re-rejected as "expired". Pending-reuse and + // fresh paths still gate — a new charge may move at Square. + if req.CardID != nil && *req.CardID != "" { + if !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode, false) { + return + } + } + if !reusePendingRecord { record := PaymentRecord{ BookingID: bookingID, @@ -4314,6 +4553,18 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal server error", http.StatusInternalServerError) return } + // MEDIUM-2: a saved-card tip charge reached its terminal SUCCESS state — + // consume the verified 2FA code now, inside the transaction that records + // the completed charge (the gate verified without consuming, so a failed + // charge did not burn the code and a same-key retry could reuse it). + if req.CardID != nil && *req.CardID != "" { + if consErr := twofa.ConsumePendingCode(r.Context(), recheckTx, userID); consErr != nil { + log.Printf("CRITICAL: Square tip payment %s (ID=%s) was processed but consuming the 2FA code for user %s failed: %v — manual reconciliation required", + paymentResult.Status, paymentResult.SquarePayID, userID, consErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + } if cErr := recheckTx.Commit(r.Context()); cErr != nil { log.Printf("CRITICAL: Square tip payment %s (ID=%s) succeeded but committing the post-charge status update for payment %s failed: %v — manual reconciliation required", paymentResult.Status, paymentResult.SquarePayID, paymentID, cErr) diff --git a/backend/handlers/payments/service.go b/backend/handlers/payments/service.go index 67469f4..3d219a0 100644 --- a/backend/handlers/payments/service.go +++ b/backend/handlers/payments/service.go @@ -483,6 +483,10 @@ func (s *PaymentService) GetBookingPaymentInfo(ctx context.Context, bookingID st FROM refunds r JOIN payments p ON r.payment_id = p.id WHERE p.booking_id = $1 AND r.status IN ('completed', 'pending') + -- Tips are excluded from TotalPaid above, so a tip refund must + -- equally be excluded here — otherwise a refunded tip would + -- subtract from the paid total and re-open booking capacity. + AND p.payment_type <> 'tip' GROUP BY p.booking_id ) rr ON b.id = rr.booking_id WHERE b.id = $1 @@ -521,6 +525,10 @@ func (s *PaymentService) GetBookingRemainingBalancePence(ctx context.Context, bo FROM refunds r JOIN payments p ON r.payment_id = p.id WHERE p.booking_id = $1 AND r.status = 'completed' + -- A tip refund returns gratuity, not booking money — it must not + -- re-open booking charge capacity (mirror of the paid_total tip + -- exclusion above). + AND p.payment_type <> 'tip' ) -- Money-safety (M-cap): refunds return money, so they re-open booking -- capacity — remaining = total - paid + refunded. LEAST clamps the cap diff --git a/backend/handlers/payments/sweep.go b/backend/handlers/payments/sweep.go index 4478461..cc8579e 100644 --- a/backend/handlers/payments/sweep.go +++ b/backend/handlers/payments/sweep.go @@ -691,11 +691,12 @@ const replayLegitimateRetryWindow = 22 * time.Hour // which makes a retained-key dedup return the ORIGINAL charge with created < // r.CreatedAt. Without this tolerance the sweep would declare that original // payment a "new charge" and auto-refund a charge the customer legitimately -// authorized. Anything at or before row.CreatedAt is therefore treated as -// AMBIGUOUS (never auto-refunded, never rescued) — see -// reconcileStalePaymentByKey. A payment created AFTER row.CreatedAt + -// replayLegitimateRetryWindow remains the provable expired-key duplicate. -const replayRescueLowerBoundSkew = time.Minute +// authorized. A payment created within the skew BEFORE the row is therefore the +// ORIGINAL and is rescued (Loop B MEDIUM — a >1min-ahead DB clock previously +// stranded such a charge pending until the 24h blind-fail); only a payment +// created more than the skew before the row, or after row.CreatedAt + +// replayLegitimateRetryWindow, is ambiguous / a provable expired-key duplicate. +const replayRescueLowerBoundSkew = 5 * time.Minute // replayMatchesRowAmount reports whether the replayed payment charged the same // amount the pending row records — the amount the sweep's replay body repeats @@ -728,7 +729,14 @@ func replayWithinLegitimateWindow(r staleRow, pr *square.PaymentResult) bool { if !ok || r.CreatedAt.IsZero() { return false } - return !created.Before(r.CreatedAt) && !created.After(r.CreatedAt.Add(replayLegitimateRetryWindow)) + // The lower bound is replayRescueLowerBoundSkew BEFORE the row: a DB clock + // running ahead of Square's (independent NTP drift, VM pause/resume) can + // make a retained-key dedup return the ORIGINAL charge with created_at + // slightly before the pending row. Such a payment cannot be a provably-new + // expired-key replay (those land ~22h AFTER the row), so within the skew + // tolerance it is the legitimate original and must be rescued — not left + // pending to strand the customer's authorized charge (Loop B MEDIUM). + return !created.Before(r.CreatedAt.Add(-replayRescueLowerBoundSkew)) && !created.After(r.CreatedAt.Add(replayLegitimateRetryWindow)) } // isSavedCardSource reports whether a Square source id is a card-on-file @@ -769,10 +777,10 @@ func parseReplayedCreatedAt(pr *square.PaymentResult) (time.Time, bool) { // replayed payment that cannot be proven to be the original (or a retry // within the legitimate window) is never rescued (the row stays pending, a // CRITICAL log is raised and an admin notification inserted), so a hidden -// second charge can never masquerade as the original one. The caller further -// refuses to auto-refund a payment created BEFORE the row (replayRescueLowerBoundSkew -// tolerance — a DB clock ahead of Square's can make the retained-key original -// look slightly older): such a payment is not provably a new charge. +// second charge can never masquerade as the original one. The lower bound of +// the legitimate window is replayRescueLowerBoundSkew BEFORE the row: a DB +// clock ahead of Square's can make the retained-key original look slightly +// older, and such a payment is the legitimate original, not a new charge. // // The check runs ONLY against real Square timestamps: it is gated off in an // explicit dev/mock env because the dev mock returns payments whose CreatedAt @@ -973,20 +981,15 @@ func reconcileStalePaymentByKey(ctx context.Context, table string, r staleRow) ( } // B1 (MEDIUM 2): a replayed COMPLETED payment created BEFORE the // pending row is NOT provably a new expired-key replay — those land - // ~22h AFTER the row. A retained-key dedup returns the ORIGINAL - // charge, and a DB clock running AHEAD of Square's (rows are - // inserted pending-first; independent NTP drift, VM pause/resume) - // can make that original's created_at lag the row's by up to - // replayRescueLowerBoundSkew. Auto-refunding it would reverse a - // legitimate payment the customer authorized, so any before-row - // created_at is ambiguous: the row is left PENDING with a CRITICAL - // notification for manual reconciliation. + // ~22h AFTER the row. Only genuinely-before payments reach this + // line: a payment within replayRescueLowerBoundSkew of the row is + // already treated as the ORIGINAL retained-key charge by + // replayWithinLegitimateWindow and rescued. This one is far enough + // before the row that no clock skew can explain it — the row is + // left PENDING with a CRITICAL notification for manual + // reconciliation, never auto-refunded. if created.Before(r.CreatedAt) { - skew := "well before" - if !created.Before(r.CreatedAt.Add(-replayRescueLowerBoundSkew)) { - skew = "slightly before (within the clock-skew tolerance)" - } - return leavePendingCritical(ctx, r, "stale pending %s reconcile by key: the replayed COMPLETED payment %s was created %s the pending row %s — cannot prove it is a NEW expired-key replay (a DB clock ahead of Square's can make a retained-key original look slightly older) — leaving row %s PENDING — MANUAL RECONCILIATION REQUIRED: verify at Square whether this is a second charge before refunding", table, pr.ID, skew, r.ID, r.ID) + return leavePendingCritical(ctx, r, "stale pending %s reconcile by key: the replayed COMPLETED payment %s was created %s before the pending row %s (beyond the %s clock-skew tolerance) — cannot prove it is a NEW expired-key replay nor the ORIGINAL charge — leaving row %s PENDING — MANUAL RECONCILIATION REQUIRED: verify at Square whether this is a second charge before refunding", table, pr.ID, created.Sub(r.CreatedAt).Round(time.Minute), r.ID, replayRescueLowerBoundSkew, r.ID) } lag := created.Sub(r.CreatedAt).Round(time.Minute).String() // B1: the replayed COMPLETED payment is a REAL charge the customer diff --git a/backend/handlers/payments/sweep_test.go b/backend/handlers/payments/sweep_test.go index 073a01c..1439fe7 100644 --- a/backend/handlers/payments/sweep_test.go +++ b/backend/handlers/payments/sweep_test.go @@ -1263,15 +1263,16 @@ func TestSweepStalePendingPayments_KeyedReplayOriginalPayment_Rescues(t *testing // TestSweepStalePendingPayments_KeyedReplayCreatedBeforeRow_LeavesPending locks // the B1 lower-bound clock-skew tolerance: a replayed COMPLETED payment created -// slightly BEFORE the pending row must NOT be auto-refunded as a "new charge". -// Rows are inserted pending-first (row.CreatedAt precedes Square's created_at by +// BEFORE the pending row must NOT be auto-refunded as a "new charge". Rows are +// inserted pending-first (row.CreatedAt precedes Square's created_at by // ~0.5-1s), and a DB clock running AHEAD of Square's (independent NTP drift, VM // pause/resume) makes a retained-key dedup return the ORIGINAL charge with -// created < row.CreatedAt. Such a payment cannot be proven to be a new -// expired-key replay (those land ~22h after the row), so auto-refunding it would -// reverse a legitimate charge — the row is left PENDING with a CRITICAL -// notification instead. Sequential (flips SQUARE_ENVIRONMENT), like the sibling -// B1/A1 tests. +// created < row.CreatedAt. A payment within replayRescueLowerBoundSkew of the +// row is that clock-skewed ORIGINAL and is rescued (locked by +// TestSweepStalePendingPayments_KeyedReplaySlightlyBeforeRow_Rescues); this +// fixture sits BEYOND the 5-minute tolerance, where no clock skew can explain +// it — the row is left PENDING with a CRITICAL notification instead. Sequential +// (flips SQUARE_ENVIRONMENT), like the sibling B1/A1 tests. func TestSweepStalePendingPayments_KeyedReplayCreatedBeforeRow_LeavesPending(t *testing.T) { ctx, tx := testutils.SetupTestTx(t) @@ -1298,15 +1299,15 @@ func TestSweepStalePendingPayments_KeyedReplayCreatedBeforeRow_LeavesPending(t * t.Fatalf("failed to age the stale payment: %v", err) } - // The replayed COMPLETED payment is the ORIGINAL charge under a retained - // key whose created_at lags the DB row's by 2s — the DB clock running ahead - // of Square's. Seeding from the row's own timestamp (minus 2s) keeps the - // lag deterministic. + // The replayed COMPLETED payment is created 10 minutes BEFORE the row — + // beyond the 5-minute clock-skew tolerance, so it cannot be a clock-skewed + // retained-key original and must remain ambiguous. Seeding from the row's + // own timestamp keeps the lag deterministic. 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) } - skewedCreated := rowCreatedAt.Add(-2 * time.Second) + skewedCreated := rowCreatedAt.Add(-10 * time.Minute) origClient := SquareClient mock := square.NewDevClient() @@ -1370,6 +1371,107 @@ func TestSweepStalePendingPayments_KeyedReplayCreatedBeforeRow_LeavesPending(t * } } +// TestSweepStalePendingPayments_KeyedReplaySlightlyBeforeRow_Rescues locks the +// Loop B MEDIUM lower-bound fix: a replayed COMPLETED payment created slightly +// BEFORE the pending row — within the 5-minute replayRescueLowerBoundSkew — is +// the clock-skewed ORIGINAL charge under a retained key (a DB clock ahead of +// Square's makes a retained-key dedup return the original with created < +// row.CreatedAt), NOT a provably-new duplicate. It must be RESCUED to +// 'completed'; stranding it pending (the pre-fix behavior for any before-row +// payment) would leave the customer's legitimately-authorized charge to resolve +// only via the 24h blind-fail as a WARN'd failed payment. Sequential (flips +// SQUARE_ENVIRONMENT), like the sibling B1/A1 tests. +func TestSweepStalePendingPayments_KeyedReplaySlightlyBeforeRow_Rescues(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + if err != nil { + t.Fatalf("failed to create booking: %v", err) + } + + staleID, err := fixtures.CreateTestPayment(tx, bookingID, 2000.00, "online_square", "full", "pending") + if err != nil { + t.Fatalf("failed to create stale pending payment: %v", err) + } + const key = "key-retained-within-skew" + 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) + } + + // The replayed COMPLETED payment is the ORIGINAL charge under a retained + // key whose created_at lags the DB row's by 2 minutes — inside the 5-minute + // clock-skew tolerance. Seeding from the row's own timestamp keeps the lag + // deterministic. + 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) + } + skewedCreated := rowCreatedAt.Add(-2 * time.Minute) + + origClient := SquareClient + mock := square.NewDevClient() + t.Setenv("SQUARE_ENVIRONMENT", "production") + SquareClient = &staleReplayClient{SquareClient: mock, result: &square.PaymentResult{ + Status: "COMPLETED", + ID: "pay_original_within_skew", + SquarePayID: "pay_original_within_skew", + CreatedAt: skewedCreated.Format(time.RFC3339Nano), + }} + defer func() { SquareClient = origClient }() + + pgxTx := db.TxFromContext(ctx) + if pgxTx == nil { + t.Fatal("no transaction in context") + } + if err := pgxTx.Commit(ctx); err != nil { + t.Fatalf("failed to commit test tx: %v", err) + } + + t.Cleanup(func() { + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM refunds WHERE payment_id = $1`, staleID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM admin_notifications WHERE reason = 'critical_payment_log' AND user_id = $1`, userID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM payments WHERE id = $1`, staleID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM users WHERE id = $1`, userID) + }) + + freshCtx := context.Background() + if _, err := SweepStalePendingPayments(freshCtx); err != nil { + t.Fatalf("sweep failed: %v", err) + } + + // Within-tolerance before-row: the clock-skewed ORIGINAL is rescued, never + // auto-refunded and never left pending. + var status, sqPayID string + if err := db.Conn.QueryRow(freshCtx, "SELECT status, COALESCE(square_payment_id, '') FROM payments WHERE id = $1", staleID).Scan(&status, &sqPayID); err != nil { + t.Fatalf("failed to query payment: %v", err) + } + if status != "completed" { + t.Errorf("expected a within-tolerance before-row replay rescued to 'completed', got %q", status) + } + if sqPayID != "pay_original_within_skew" { + t.Errorf("expected square_payment_id %s written back on the rescue, got %q", "pay_original_within_skew", sqPayID) + } + + var refundCount int + if err := db.Conn.QueryRow(freshCtx, `SELECT COUNT(*) FROM refunds WHERE payment_id = $1`, staleID).Scan(&refundCount); err != nil { + t.Fatalf("failed to count refunds: %v", err) + } + if refundCount != 0 { + t.Errorf("expected NO auto-refund of a within-tolerance before-row original, got %d refund rows", refundCount) + } +} + // TestSweepStalePendingPayments_KeyedReplayRetryAt215h_Rescues locks the B2 // 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 diff --git a/backend/handlers/payments/till.go b/backend/handlers/payments/till.go index 4302734..b886c8e 100644 --- a/backend/handlers/payments/till.go +++ b/backend/handlers/payments/till.go @@ -17,6 +17,7 @@ import ( "crussell/db" "crussell/internal/square" + "crussell/internal/twofa" "crussell/internal/validators" "crussell/mw" @@ -888,6 +889,11 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { } } + // cardUserID is the owner of the charged saved card (till sales are not + // user-scoped). Resolved in the saved_card case below; hoisted here because + // the post-charge 2FA consumption + audit after the Square call need it. + var cardUserID sql.NullString + switch req.PaymentMethod { case "cash": saleStatus = "completed" @@ -909,13 +915,14 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { // The till path is not user-scoped, so fetch the card's owner along with // the charge details — the owner is needed to lazily provision a Square - // customer if the row predates P14 (R6). - var cardUserID sql.NullString + // customer if the row predates P14 (R6). cardUserID is declared at + // function scope (before the switch) because the post-charge 2FA + // consumption + audit need it after the Square call. err = tx.QueryRow(ctx, ` - SELECT user_id, COALESCE(square_card_id, ''), COALESCE(square_customer_id, '') - FROM user_saved_cards - WHERE id = $1 AND deleted_at IS NULL - `, *req.UserSavedCardID).Scan(&cardUserID, &savedCardSqCardID, &savedCardCustomerID) + SELECT user_id, COALESCE(square_card_id, ''), COALESCE(square_customer_id, '') + FROM user_saved_cards + WHERE id = $1 AND deleted_at IS NULL + `, *req.UserSavedCardID).Scan(&cardUserID, &savedCardSqCardID, &savedCardCustomerID) if err != nil { log.Printf("Failed to get saved card details: %v", err) http.Error(w, "Card not found", http.StatusNotFound) @@ -947,8 +954,12 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { } // 2FA gating (C5): charging a customer's saved card requires 2FA when - // the feature is enforced. - if cardUserID.Valid && !requireTwoFactorForCardAccess(w, r, service, cardUserID.String, req.VerificationCode) { + // the feature is enforced. consume=false (MEDIUM-2): the code is + // verified here but only NULLed once the charge reaches its terminal + // success state 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. + if cardUserID.Valid && !requireTwoFactorForCardAccess(w, r, service, cardUserID.String, req.VerificationCode, false) { return } @@ -1274,6 +1285,28 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { return } + // MEDIUM-2: a saved-card till charge reached its terminal SUCCESS state + // — consume the verified 2FA code now (the gate verified without + // consuming, so a failed/ambiguous charge did not burn the code and a + // same-key retry could reuse it). Best-effort after the completion + // write: a consume failure cannot undo the completed sale, it only + // leaves the code valid until its 10-minute expiry. + if req.PaymentMethod == "saved_card" && cardUserID.Valid { + if consErr := twofa.ConsumePendingCode(ctx, db.Conn, cardUserID.String); consErr != nil { + log.Printf("CRITICAL: Square payment %s succeeded but consuming the 2FA code for user %s failed: %v — MANUAL RECONCILIATION REQUIRED", paymentResult.SquarePayID, cardUserID.String, consErr) + } + // MEDIUM-3a: record the admin-initiated saved-card till charge in + // admin_audit_log (mirroring giftcards.go's balance_check audit). + insertAdminAuditCharge(ctx, adminID, cardUserID.String, "till_saved_card_charge", map[string]any{ + "till_sale_id": tillSaleID, + "item_type": req.ItemType, + "gift_card_id": giftCardID, + "amount": req.Amount, + "card_last4": paymentResult.CardLast4, + "square_payment_id": paymentResult.SquarePayID, + }) + } + saleStatus = "completed" } diff --git a/backend/handlers/payments/twofa.go b/backend/handlers/payments/twofa.go index 5db93ad..59aba22 100644 --- a/backend/handlers/payments/twofa.go +++ b/backend/handlers/payments/twofa.go @@ -75,14 +75,18 @@ func (s *PaymentService) UserTwoFactorEnabled(ctx context.Context, userID string // stored pending 2FA code. It is a thin delegation shim over // twofa.VerifyForUser — the single source of truth for the verification core // (per-user brute-force lockout, constant-time compare, legacy pre-pepper -// hash fallback, code lifetime). consume=true is passed so a verified code is -// SINGLE-USE: the gate NULLs the pending code on success, so one code -// authorizes exactly one saved-card charge (not unlimited charges for its -// 10-minute lifetime). It returns nil on a valid code, or a classified -// twofa.ErrIncorrect / twofa.ErrLockedOut / twofa.ErrMissingOrExpired (or a -// wrapped DB error) for the caller to map to the correct HTTP status. -func verifyPendingTwoFactorCode(ctx context.Context, userID, code string) error { - return twofa.VerifyForUser(ctx, userID, code, true) +// hash fallback, code lifetime). consume=true makes a verified code SINGLE-USE +// immediately (the pending code is NULLed on success); consume=false verifies +// WITHOUT consuming (MEDIUM-2 — the saved-card CHARGE gates pass false and +// defer consumption to the completed-charge transaction via +// twofa.ConsumePendingCode, so a failed Square charge does not burn the code; +// the save-card SAVE gate passes true because saving a card is a terminal +// operation with no downstream charge to attach consumption to). It returns nil +// on a valid code, or a classified twofa.ErrIncorrect / twofa.ErrLockedOut / +// twofa.ErrMissingOrExpired (or a wrapped DB error) for the caller to map to +// the correct HTTP status. +func verifyPendingTwoFactorCode(ctx context.Context, userID, code string, consume bool) error { + return twofa.VerifyForUser(ctx, userID, code, consume) } // requireTwoFactorForCardAccess gates the saved-card online payment paths @@ -100,6 +104,13 @@ func verifyPendingTwoFactorCode(ctx context.Context, userID, code string) error // operator relays (delivery is the user package's build-dependent [2FA] log / // email-SMS channel). // +// consume controls whether a verified code is NULLed immediately (consume=true +// — the save-card SAVE gate) or left intact for the caller to consume when its +// operation reaches a terminal success state (consume=false — the saved-card +// CHARGE gates; see verifyPendingTwoFactorCode / twofa.ConsumePendingCode, +// MEDIUM-2). In every case the 5-attempt lockout and the +// code-destroy-on-lockout semantics are unchanged (twofa.Check). +// // The code check is delegated to crussell/internal/twofa via // verifyPendingTwoFactorCode, so this gate participates in the SAME per-user // brute-force lockout (5 failed attempts invalidate the pending code) as the @@ -109,7 +120,7 @@ func verifyPendingTwoFactorCode(ctx context.Context, userID, code string) error // // On any denial an error JSON is written (parseable by the frontend via // extractErrorMessage) and false is returned — the caller must abort the charge. -func requireTwoFactorForCardAccess(w http.ResponseWriter, r *http.Request, service *PaymentService, userID, verificationCode string) bool { +func requireTwoFactorForCardAccess(w http.ResponseWriter, r *http.Request, service *PaymentService, userID, verificationCode string, consume bool) bool { if !twoFactorEnforced() { return true } @@ -136,7 +147,7 @@ func requireTwoFactorForCardAccess(w http.ResponseWriter, r *http.Request, servi mw.RespondError(w, http.StatusForbidden, "A two-factor verification code is required to use this saved card. Ask the customer for their current code.") return false } - switch err := verifyPendingTwoFactorCode(r.Context(), userID, verificationCode); { + switch err := verifyPendingTwoFactorCode(r.Context(), userID, verificationCode, consume); { case err == nil: return true case errors.Is(err, twofa.ErrIncorrect): diff --git a/backend/handlers/payments/twofa_test.go b/backend/handlers/payments/twofa_test.go index 86aa3e1..d67252e 100644 --- a/backend/handlers/payments/twofa_test.go +++ b/backend/handlers/payments/twofa_test.go @@ -108,7 +108,7 @@ func TestRequireTwoFactorForCardAccess_NotEnforced(t *testing.T) { t.Setenv("SQUARE_ENVIRONMENT", "mock") req := httptest.NewRequest(http.MethodPost, "/", nil) w := httptest.NewRecorder() - require.True(t, requireTwoFactorForCardAccess(w, req, nil, "000000000001", "")) + require.True(t, requireTwoFactorForCardAccess(w, req, nil, "000000000001", "", false)) require.Equal(t, http.StatusOK, w.Code, "no response must be written when not enforced") } @@ -121,7 +121,7 @@ func TestRequireTwoFactorForCardAccess_Enforced(t *testing.T) { require.NoError(t, err) req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() - ok := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "123456") + ok := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "123456", false) require.False(t, ok) require.Equal(t, http.StatusForbidden, w.Code) var body map[string]string @@ -137,7 +137,7 @@ func TestRequireTwoFactorForCardAccess_Enforced(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() // B10: the enabled setup flag alone must NOT unlock the gate. - ok := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "") + ok := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "", false) require.False(t, ok) require.Equal(t, http.StatusForbidden, w.Code) }) @@ -148,7 +148,7 @@ func TestRequireTwoFactorForCardAccess_Enforced(t *testing.T) { seedTwoFAPendingCode(t, tx, userID, "424242") req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() - require.True(t, requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "424242")) + require.True(t, requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "424242", false)) require.Equal(t, http.StatusOK, w.Code) }) @@ -158,7 +158,7 @@ func TestRequireTwoFactorForCardAccess_Enforced(t *testing.T) { seedTwoFAPendingCode(t, tx, userID, "424242") req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() - ok := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "000000") + ok := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "000000", false) require.False(t, ok) require.Equal(t, http.StatusBadRequest, w.Code) var body map[string]string @@ -169,7 +169,7 @@ func TestRequireTwoFactorForCardAccess_Enforced(t *testing.T) { t.Run("unknown_user_writes_403_json", func(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() - require.False(t, requireTwoFactorForCardAccess(w, req, NewPaymentService(), "000000000000", "123456")) + require.False(t, requireTwoFactorForCardAccess(w, req, NewPaymentService(), "000000000000", "123456", false)) require.Equal(t, http.StatusForbidden, w.Code) var body map[string]string require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body), "403 body must be mw.RespondError JSON") @@ -177,12 +177,14 @@ func TestRequireTwoFactorForCardAccess_Enforced(t *testing.T) { }) } -// TestRequireTwoFactorForCardAccess_CodeIsSingleUse pins the finding-1 fix: a -// code verified through the gate is CONSUMED (the pending code is NULLed), so -// the same code cannot authorize a second saved-card charge within its -// 10-minute lifetime. The second attempt with the same code is denied with the -// documented "expired — request a new one" 400. -func TestRequireTwoFactorForCardAccess_CodeIsSingleUse(t *testing.T) { +// TestRequireTwoFactorForCardAccess_VerifyDoesNotConsume pins the MEDIUM-2 +// contract: the charge gate verifies the code WITHOUT consuming it (consume +// happens later, at the charge's terminal SUCCESS state via +// twofa.ConsumePendingCode), so a failed/ambiguous Square charge does NOT burn +// the operator-relayed code — a same-key retry can re-verify the SAME code. +// Only an explicit ConsumePendingCode (the completed-charge path) NULLs it, +// after which the code is dead ("expired"). +func TestRequireTwoFactorForCardAccess_VerifyDoesNotConsume(t *testing.T) { helperEnvEnforce2FA(t) ctx, tx := testutils.SetupTestTx(t) userID, err := fixtures.CreateTestUser(tx) @@ -191,17 +193,27 @@ func TestRequireTwoFactorForCardAccess_CodeIsSingleUse(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() - require.True(t, requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "424242"), "first use of the code must pass the gate") + require.True(t, requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "424242", false), "first gate pass must succeed") require.Equal(t, http.StatusOK, w.Code) - // The verified code must now be consumed (NULLed) in the DB. + // The code must still be present — the gate verified WITHOUT consuming. var pendingHash sql.NullString require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&pendingHash)) - require.False(t, pendingHash.Valid, "a verified gate code must be consumed (NULLed)") + require.True(t, pendingHash.Valid, "the gate must NOT consume the code (MEDIUM-2)") - // A second charge attempt with the same code must be denied as expired. + // A same-key retry (e.g. after a failed Square charge) re-verifies the SAME code. w = httptest.NewRecorder() - require.False(t, requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "424242"), "a consumed code must not pass the gate twice") + require.True(t, requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "424242", false), "a not-yet-consumed code must pass the gate again on retry") + require.Equal(t, http.StatusOK, w.Code) + + // Consumption happens at the charge's terminal SUCCESS state. + require.NoError(t, twofa.ConsumePendingCode(ctx, tx, userID)) + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&pendingHash)) + require.False(t, pendingHash.Valid, "ConsumePendingCode must NULL the pending code") + + // A further attempt with the consumed code is denied as expired. + w = httptest.NewRecorder() + require.False(t, requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "424242", false), "a consumed code must not pass the gate") require.Equal(t, http.StatusBadRequest, w.Code) var body map[string]string require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) @@ -427,3 +439,83 @@ func TestTwoFactorEnforced_CreateTillSale_SavedCard_With2FA_Succeeds(t *testing. require.Contains(t, []int{http.StatusOK, http.StatusCreated}, w.Code, w.Body.String()) } + +// TestTwoFactorEnforced_CreateBookingPayment_SaveCard_Retry_ReturnsCompleted +// pins the Loop B MEDIUM gate-ordering fix: the save-card 2FA gate runs AFTER +// the idempotency dedup's completed short-circuit. A same-key lost-response +// retry re-sends the SAME single-use verification code that the original +// attempt already consumed; if the gate ran first it would 400 "Verification +// code expired". With the gate below the dedup, the retry returns the +// already-completed payment instead of re-entering the gate. +func TestTwoFactorEnforced_CreateBookingPayment_SaveCard_Retry_ReturnsCompleted(t *testing.T) { + helperEnvEnforce2FA(t) + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + seedTwoFAPendingCode(t, tx, userID, "778899") + + cardToken := "cnon:2fa-save-card-retry" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + SaveCard: true, + IdempotencyKey: "2fa-save-card-retry", + VerificationCode: "778899", + } + + handler := withNonGuest(CreateBookingPayment) + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + // Same-key retry re-sends the identical request, whose code is now + // consumed. The completed-dedup must return the payment before the gate. + w2 := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w2.Code, "same-key retry must dedup to the completed payment, not re-run the gate: %s", w2.Body.String()) + + var payCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&payCount)) + require.Equal(t, 1, payCount, "the retry must not create a second payment") +} + +// TestTwoFactorEnforced_CreateTipPayment_SavedCard_Retry_ReturnsCompleted pins +// the Loop B MEDIUM gate-ordering fix on the tip endpoint: the saved-card +// charge 2FA gate runs AFTER the tip idempotency dedup's completed +// short-circuit. A same-key lost-response retry re-sends the SAME single-use +// verification code the original attempt consumed; with the gate first it would +// 400 "expired", with the gate below the dedup the retry returns the completed +// tip instead. +func TestTwoFactorEnforced_CreateTipPayment_SavedCard_Retry_ReturnsCompleted(t *testing.T) { + helperEnvEnforce2FA(t) + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + seedTwoFAPendingCode(t, tx, userID, "667788") + + _, err := fixtures.CreateTestPayment(tx, bookingID, 5000.00, "online_square", "full", "completed") + require.NoError(t, err) + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:mock_card_tip_retry", "VISA", "4321") + require.NoError(t, err) + + req := CreateTipPaymentRequest{ + Amount: 500, + CardID: &cardID, + IdempotencyKey: "2fa-tip-saved-card-retry", + VerificationCode: "667788", + } + + handler := withNonGuest(CreateTipPayment) + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + // Same-key retry re-sends the identical request, whose code is now + // consumed. The completed-dedup must return the tip before the gate. + w2 := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) + require.Equal(t, http.StatusOK, w2.Code, "same-key retry must dedup to the completed tip, not re-run the gate: %s", w2.Body.String()) + + var tipCount int + require.NoError(t, tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND payment_type = 'tip'", bookingID).Scan(&tipCount)) + require.Equal(t, 1, tipCount, "the retry must not create a second tip") +} diff --git a/backend/handlers/user/twofa.go b/backend/handlers/user/twofa.go index 829e64e..587136a 100644 --- a/backend/handlers/user/twofa.go +++ b/backend/handlers/user/twofa.go @@ -340,9 +340,11 @@ func checkTwoFACode(r *http.Request, userID string, st *twoFAAttemptState, reqCo // Coordination contract for the payments agent (B6/B10): handlers/payments // cannot import handlers/user — handlers/user imports handlers/payments // (TwoFactorEnforced, SquareClient), so a payments→user import is a cycle. The -// payments gate must call twofa.VerifyForUser(ctx, userID, code) from +// payments gate must call twofa.VerifyForUser(ctx, userID, code, consume) from // crussell/internal/twofa (the shared home of this verification core) instead -// of importing this package. +// of importing this package. Since MEDIUM-2 the payments saved-card CHARGE +// gates pass consume=false and NULL the code at the charge's terminal success +// via twofa.ConsumePendingCode; the save-card SAVE gates pass consume=true. func VerifyTwoFACodeForUser(ctx context.Context, userID, code string) error { st := twoFAAttemptStateFor(userID) st.Mu.Lock() diff --git a/backend/handlers/user/twofa_dev.go b/backend/handlers/user/twofa_dev.go index 38f71df..2609dbc 100644 --- a/backend/handlers/user/twofa_dev.go +++ b/backend/handlers/user/twofa_dev.go @@ -49,8 +49,15 @@ func twoFAEnsureIssueAllowed() error { return nil } // the user out-of-band until email/SMS lands. Production builds log it ONLY // when the operator explicitly opts in via TWO_FACTOR_ALLOW_LOG_DELIVERY=true // (see twofa_prod.go); otherwise they refuse issuance up front. +// +// MEDIUM-3b: the user id and the plaintext code are written to SEPARATE log +// lines so a log line cannot trivially pair a code with its owner. The two +// lines are still correlated by proximity, but a single-line grep or a log +// redaction rule that masks a "code" pattern no longer discloses the identity +// in the same record. func twoFADeliverCode(userID, label, code string) { - log.Printf("[2FA] verification code for user %s (%s): %s", userID, label, code) + log.Printf("[2FA] code delivery requested (user=%s, purpose=%s)", userID, label) + log.Printf("[2FA] code: %s", code) } // twoFADeliveryAvailable reports whether a 2FA code delivery channel exists in diff --git a/backend/handlers/user/twofa_prod.go b/backend/handlers/user/twofa_prod.go index 0524bfa..94484b0 100644 --- a/backend/handlers/user/twofa_prod.go +++ b/backend/handlers/user/twofa_prod.go @@ -94,10 +94,12 @@ func twoFAEnsureIssueAllowed() error { // unreachable. With the flag set, the code is written to the [2FA] log line // and an operator relays it to the user out-of-band, exactly like the // documented dev flow — the operator has accepted the risk of log-based -// delivery. +// delivery. MEDIUM-3b: the user id and the plaintext code go to SEPARATE log +// lines so a single record cannot trivially pair a code with its owner. func twoFADeliverCode(userID, label, code string) { if os.Getenv(twoFAAllowLogDeliveryEnv) == "true" { - log.Printf("[2FA] verification code for user %s (%s): %s", userID, label, code) + log.Printf("[2FA] code delivery requested (user=%s, purpose=%s)", userID, label) + log.Printf("[2FA] code: %s", code) } // Otherwise: deliberate no-op — never log the plaintext code by default. } diff --git a/backend/handlers/webhooks/square.go b/backend/handlers/webhooks/square.go index ac09df5..41040a0 100644 --- a/backend/handlers/webhooks/square.go +++ b/backend/handlers/webhooks/square.go @@ -153,17 +153,17 @@ func webhookDBContext() (context.Context, context.CancelFunc) { // squareEnvironmentMismatch reports whether the webhook's square-environment // header conflicts with the deployment's configured SQUARE_ENVIRONMENT. // -// The check is enforced ONLY when the configured environment is a known real -// Square environment (production/sandbox): those are the deployments where a -// mis-pointed subscription (e.g. a sandbox subscription posting to the -// production URL + signing key) would process events against the wrong state. -// In dev/mock deployments — or an empty/unknown SQUARE_ENVIRONMENT, which the -// rest of the backend treats as fail-closed production but is not a specific -// real environment to compare against — the header is informational and a -// mismatch is not rejectable. The dev/mock determination delegates to the -// shared payments.IsExplicitDevOrMockEnv (handlers/payments/twofa.go) rather -// than re-implementing the env-value list, so this check and the 2FA gate can -// never diverge on what counts as the dev/mock stack. +// The check is enforced for every non-dev/mock deployment: configured +// production and sandbox are compared directly, and an EMPTY or UNKNOWN +// configured SQUARE_ENVIRONMENT is treated as PRODUCTION (LOW-4) — matching how +// the rest of the backend treats empty/unknown env fail-closed (main.go:214 +// and payments twofa.go:40-42) — so a sandbox subscription mis-pointed at an +// unconfigured production URL cannot process sandbox events against production +// state. In dev/mock deployments the header is informational and a mismatch is +// not rejectable; the dev/mock determination delegates to the shared +// payments.IsExplicitDevOrMockEnv (handlers/payments/twofa.go) rather than +// re-implementing the env-value list, so this check and the 2FA gate can never +// diverge on what counts as the dev/mock stack. // // An absent header is allowed through: real Square deliveries always send it, // so a missing header in an enforced deployment is a non-Square client (which @@ -189,10 +189,11 @@ func squareEnvironmentMismatch(headerEnv string) bool { return false } configured := strings.ToLower(strings.TrimSpace(os.Getenv("SQUARE_ENVIRONMENT"))) - if configured != "production" && configured != "sandbox" { - // Empty/unknown configured environment — no specific real environment - // to enforce against; the header is informational only. - return false + if configured != "sandbox" { + // Empty/unknown configured environment is treated as PRODUCTION for the + // env check (LOW-4), matching the fail-closed production default the + // rest of the backend applies to empty/unknown SQUARE_ENVIRONMENT. + configured = "production" } return headerEnv != configured } diff --git a/backend/handlers/webhooks/webhooks_test.go b/backend/handlers/webhooks/webhooks_test.go index 5351b55..2602d17 100644 --- a/backend/handlers/webhooks/webhooks_test.go +++ b/backend/handlers/webhooks/webhooks_test.go @@ -998,6 +998,45 @@ func TestHandleSquareWebhook_EnvMismatch_DevNotEnforced(t *testing.T) { } } +// TestSquareEnvironmentMismatch_EmptyConfigTreatedAsProduction pins the LOW-4 +// fix: an EMPTY/UNKNOWN configured SQUARE_ENVIRONMENT is treated as +// PRODUCTION for the webhook env check (matching main.go:214 and payments +// twofa.go:40-42's fail-closed default), so a sandbox subscription mis-pointed +// at an unconfigured production URL is rejected instead of silently accepted. +func TestSquareEnvironmentMismatch_EmptyConfigTreatedAsProduction(t *testing.T) { + t.Setenv("SQUARE_ENVIRONMENT", "") + + // Empty/unknown configured env behaves exactly like "production": a + // sandbox header is a mismatch, a production header is a match. + if !squareEnvironmentMismatch("sandbox") { + t.Error("empty SQUARE_ENVIRONMENT must be treated as production (sandbox header = mismatch)") + } + if squareEnvironmentMismatch("production") { + t.Error("empty SQUARE_ENVIRONMENT treated as production must match a production header") + } + if squareEnvironmentMismatch("PRODUCTION") { + t.Error("header comparison must stay case-insensitive") + } + if squareEnvironmentMismatch("") { + t.Error("an absent header must stay allowed") + } + + // An explicit sandbox config still enforces sandbox semantics. + t.Setenv("SQUARE_ENVIRONMENT", "sandbox") + if squareEnvironmentMismatch("sandbox") { + t.Error("sandbox config must match a sandbox header") + } + if !squareEnvironmentMismatch("production") { + t.Error("sandbox config must reject a production header") + } + + // An explicit dev/mock config never enforces the header check. + t.Setenv("SQUARE_ENVIRONMENT", "mock") + if squareEnvironmentMismatch("sandbox") { + t.Error("dev/mock deployments must not enforce the header check") + } +} + // ============================================================================= // Bounded post-dispatch DB contexts (finding b) // ============================================================================= diff --git a/backend/internal/twofa/twofa.go b/backend/internal/twofa/twofa.go index b156dde..638e656 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, true) // consume = true +// err := twofa.VerifyForUser(ctx, userID, code, false) // consume = false // if err != nil { // switch { // case errors.Is(err, twofa.ErrIncorrect): @@ -25,13 +25,19 @@ // } // } // -// A correct code is SINGLE-USE on the payments gate: the gate passes -// consume=true, so the stored pending-code digest and its expiry are NULLed in -// the same critical section as the successful check. One code therefore -// authorizes exactly one saved-card charge, never unlimited charges for its -// 10-minute lifetime. The interactive setup/disable flows pass consume=false — -// they clear the pending fields themselves on success (enableTwoFA / -// disableTwoFA), so the code must stay valid through their whole handshake. +// 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 +// terminal operation with no downstream charge to attach consumption to. // // The failed-attempt counter is keyed per user and resets ONLY on a successful // verify (or after the 10-minute attempt window elapses) — never on a fresh @@ -278,13 +284,16 @@ const ( // check. A correct code resets the attempt counter and returns OK. An incorrect // code increments the counter and, on the 5th consecutive failure, invalidates // the pending code (lockout). A missing or expired pending code returns -// MissingOrExpired. consume makes a correct code single-use: the stored digest -// and its expiry are NULLed immediately, so one code cannot authorize a second -// operation within its lifetime (the payments saved-card gate passes true; the -// interactive setup/disable flows pass false and clear the pending fields -// themselves on success). 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. +// 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. 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) @@ -373,6 +382,32 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string, return OK, nil } +// ConsumePendingCode NULLs the user's pending 2FA code digest and expiry. The +// payments saved-card charge gate verifies WITHOUT consuming (MEDIUM-2) and the +// handlers call this when the charge reaches a TERMINAL SUCCESS state — inside +// the transaction that records the completed charge when one exists — so the +// code is consumed atomically with the charge OUTCOME, not the gate. A failed +// or ambiguous Square charge leaves the code intact and the same-key retry can +// re-verify the SAME code. Idempotent: consuming an already-NULL pending code +// is a no-op, so a code still authorizes exactly one completed charge and can +// never authorize a second after success. Accepts a db.Querier so the write can +// ride the caller's transaction (pgx.Tx) or the pool proxy. +func ConsumePendingCode(ctx context.Context, q db.Querier, userID string) error { + if userID == "" { + return nil + } + _, err := q.Exec(ctx, ` + UPDATE users + SET two_factor_pending_code_hash = NULL, + two_factor_pending_code_expires = NULL + WHERE id = $1 + `, userID) + if err != nil { + return fmt.Errorf("2FA consume pending code: %w", err) + } + return nil +} + // Classifying errors returned by VerifyForUser. var ( // ErrIncorrect reports a code that does not match the user's pending code. @@ -390,11 +425,13 @@ var ( // It returns nil on a correct code, or one of ErrIncorrect / ErrLockedOut / // ErrMissingOrExpired (or a DB error, wrapped). This is the entry point for // 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: -// the pending-code digest and its expiry are NULLed in the same critical -// section as the successful check (see Check), so one code authorizes exactly -// one gate pass. The interactive setup/disable flows pass false — they clear -// the pending fields themselves on success (enableTwoFA / disableTwoFA). +// 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). 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 47098f8..61c9c03 100644 --- a/backend/internal/twofa/twofa_test.go +++ b/backend/internal/twofa/twofa_test.go @@ -75,6 +75,31 @@ func TestVerifyForUser_LockoutAndMissing(t *testing.T) { require.ErrorIs(t, VerifyForUser(ctx, userID2, "123456", true), ErrMissingOrExpired) } +// TestConsumePendingCode pins the MEDIUM-2 contract: ConsumePendingCode NULLs +// the stored pending-code digest and expiry (idempotently), and is the ONLY +// place a verified-but-unconsumed code dies on the saved-card charge path. +func TestConsumePendingCode(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + seedPending(t, ctx, tx, userID, "123456") + + // A code verified WITHOUT consuming stays valid (the saved-card charge gate + // path, MEDIUM-2) — re-verification must keep working until consumption. + require.NoError(t, VerifyForUser(ctx, userID, "123456", false), "verify-without-consume must pass") + require.NoError(t, VerifyForUser(ctx, userID, "123456", false), "an unconsumed code must still verify on a same-key retry") + + require.NoError(t, ConsumePendingCode(ctx, tx, userID), "explicit consumption at charge success must succeed") + require.ErrorIs(t, VerifyForUser(ctx, userID, "123456", false), ErrMissingOrExpired, "a consumed code must no longer verify") + + // Consumption is idempotent — a second call (e.g. a retried completed + // charge) is a no-op, never an error. + require.NoError(t, ConsumePendingCode(ctx, tx, userID), "consuming an already-consumed code must be a no-op") + + // An unknown user is a no-op too. + require.NoError(t, ConsumePendingCode(ctx, tx, "000000000000")) +} + // TestVerifyForUser_AttemptStateMapPersists exercises the shared per-user // attempt map directly (the state the payments gate shares with the interactive // endpoints): the map is bounded and a locked-out record is never evicted. diff --git a/backend/main.go b/backend/main.go index 2e25732..43ee2c0 100644 --- a/backend/main.go +++ b/backend/main.go @@ -701,10 +701,18 @@ func main() { r.With(mw.RequireAuth, mw.RequireVerified, limitBody(uploadBodyLimit)).Post("/user/profile-picture", user.UploadProfilePictureHandler) - // Admin-only (no rate limit - trusted users with authenticated sessions) + // Admin-only. The group now carries a per-IP limiter (300/min) to bound + // the per-request DB amplification of the auth path (VerifyToken runs a + // JTI-revocation query + a family-alive query per request — MEDIUM-1): + // an unthrottled admin surface lets a single compromised admin session + // hammer the DB. 300/min is far above any legitimate admin UI usage and + // keys per-IP (RateLimitByUser would be a no-op — there is exactly one + // admin account, so per-user == global). The trusted-session model is + // unchanged; this only bounds amplification. r.Group(func(r chi.Router) { r.Use(mw.RequireAuth) r.Use(mw.RequireAdmin) + r.Use(mw.RateLimit(300, time.Minute)) r.Use(limitBody(defaultBodyLimit)) r.Route("/admin/services", func(r chi.Router) { diff --git a/frontend/src/lib/utils/uuid.ts b/frontend/src/lib/utils/uuid.ts index eba1b46..1093370 100644 --- a/frontend/src/lib/utils/uuid.ts +++ b/frontend/src/lib/utils/uuid.ts @@ -1,27 +1,17 @@ /** - * Generate a UUID v4 string. + * Generate a UUID v4 string using `crypto.randomUUID()`. * - * Uses `window.crypto.getRandomValues()` when available (secure context), - * falls back to `Math.random()` for non-secure contexts (e.g. plain HTTP). - * - * Safe for all environments — does NOT rely on `crypto.randomUUID()` - * which requires a secure context (HTTPS). + * The app is HTTPS-only (secure context), so `crypto.randomUUID()` is always + * available in the browser and in Node 19+ (vitest/dev server). Idempotency + * keys derived here gate Square dedup — they must be unpredictable, so this + * HARD-FAILS (throws) rather than silently falling back to `Math.random()` in + * a non-secure context (LOW-5). No fallback is ever used. */ export function generateUUID(): string { - const array = new Uint8Array(16); - if (typeof window !== 'undefined' && window.crypto?.getRandomValues) { - window.crypto.getRandomValues(array); - } else { - for (let i = 0; i < 16; i++) array[i] = Math.floor(Math.random() * 256); + if (typeof globalThis.crypto?.randomUUID !== 'function') { + throw new Error( + 'generateUUID requires crypto.randomUUID() (secure context). The app is HTTPS-only; refusing to fall back to Math.random().' + ); } - // UUID v4 marker and variant - array[6] = (array[6] & 0x0f) | 0x40; - array[8] = (array[8] & 0x3f) | 0x80; - return [...array] - .map((b, i) => { - const hex = b.toString(16).padStart(2, '0'); - if (i === 4 || i === 6 || i === 8 || i === 10) return '-' + hex; - return hex; - }) - .join(''); + return globalThis.crypto.randomUUID(); } diff --git a/init-scripts/init-script.sql b/init-scripts/init-script.sql index 42ec698..1ac3532 100644 --- a/init-scripts/init-script.sql +++ b/init-scripts/init-script.sql @@ -388,7 +388,15 @@ CREATE TABLE bookings ( updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), created_by CHAR(12), idempotency_key VARCHAR(64) UNIQUE, - out_of_hours BOOLEAN NOT NULL DEFAULT FALSE + out_of_hours BOOLEAN NOT NULL DEFAULT FALSE, + -- Once-only loyalty-stamp marker (Loop B MEDIUM — stamp farming via refund + -- + re-charge): the stamp is awarded at most ONCE per booking, no matter + -- how many times the booking is re-completed. A refund never moves the + -- booking out of 'in_progress', so a re-payment re-completes it; without + -- this marker each in_progress→completed transition would mint another + -- stamp with no net merchant cash flow. Set by + -- ApplyBookingCompletionSideEffects only when a stamp is actually awarded. + loyalty_stamp_awarded_at TIMESTAMPTZ ); CREATE INDEX idx_bookings_userid ON bookings(user_id);