diff --git a/backend/auth/jwt.go b/backend/auth/jwt.go index 8861363..0d6d481 100644 --- a/backend/auth/jwt.go +++ b/backend/auth/jwt.go @@ -19,11 +19,16 @@ import ( var TokenAuth *jwtauth.JWTAuth -// AuthResponse is the response structure for login/refresh endpoints +// AuthResponse is the response structure for login/refresh endpoints. +// RefreshToken is a 90-day opaque, DB-hashed, single-use credential; the +// client stores it and presents it (Bearer) to POST /api/refresh-token in +// exchange for a fresh access token + a rotated refresh token. It is returned +// in JSON so the SPA can persist it — omitting it would make the refresh flow +// unusable — but it is never logged and never returned by any other endpoint. type AuthResponse struct { Token string `json:"token"` JTI string `json:"jti"` - RefreshToken string `json:"-"` + RefreshToken string `json:"refreshToken,omitempty"` } // generateJTI generates a UUID v4 string using crypto/rand diff --git a/backend/handlers/auth/auth_test.go b/backend/handlers/auth/auth_test.go index 69a75ce..a4cfb3d 100644 --- a/backend/handlers/auth/auth_test.go +++ b/backend/handlers/auth/auth_test.go @@ -35,7 +35,6 @@ import ( "crussell/mw" "crussell/testutils" "crussell/testutils/fixtures" - "crussell/testutils/jwt" "github.com/go-chi/chi/v5" "github.com/jackc/pgx/v5/pgxpool" @@ -429,8 +428,8 @@ func TestLogin_InvalidCredentials_NonExistentEmail(t *testing.T) { // Refresh Token Handler Tests // ============================================================================= -// TestRefreshToken_Success tests that a valid JWT token can be refreshed -// to obtain a new token with extended expiry. +// TestRefreshToken_Success tests that a valid refresh token can be exchanged +// for a new access token + a rotated refresh token (B5). func TestRefreshToken_Success(t *testing.T) { t.Parallel() ctx, tx := resetTestData(t) @@ -444,19 +443,18 @@ func TestRefreshToken_Success(t *testing.T) { } defer fixtures.DeleteUser(tx, userID) - // Generate a valid token - token := jwt.GenerateTestToken(userID, "verified_email") + // B5: the endpoint REQUIRES the opaque refresh token in the Authorization + // header — the access token alone can no longer self-renew. + refreshToken, err := auth.GenerateRefreshToken(ctx, userID, "verified_email") + if err != nil { + t.Fatalf("failed to generate refresh token: %v", err) + } req := httptest.NewRequest("POST", "/api/refresh-token", nil) - req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set("Authorization", "Bearer "+refreshToken) + req = req.WithContext(ctx) w := httptest.NewRecorder() - // Use the middleware keys to set up context (matching what mw.RequireAuth does) - reqCtx := ctx - reqCtx = context.WithValue(reqCtx, mw.UserIDKey, userID) - reqCtx = context.WithValue(reqCtx, mw.UserRoleKey, "verified_email") - req = req.WithContext(reqCtx) - handler.ServeHTTP(w, req) if w.Code != http.StatusOK { @@ -464,7 +462,9 @@ func TestRefreshToken_Success(t *testing.T) { } var resp struct { - Token string `json:"token"` + Token string `json:"token"` + JTI string `json:"jti"` + RefreshToken string `json:"refreshToken"` } if err := testutils.ParseResponseBody(w, &resp); err != nil { t.Errorf("failed to parse response: %v", err) @@ -473,10 +473,16 @@ func TestRefreshToken_Success(t *testing.T) { if resp.Token == "" { t.Error("expected new token in response, got empty string") } + if resp.JTI == "" { + t.Error("expected new jti in response, got empty string") + } + if resp.RefreshToken == "" { + t.Error("expected rotated refresh token in response, got empty string") + } } // TestRefreshToken_Unauthorized_NoToken verifies that attempting to refresh -// a token without providing one results in HTTP 401 Unauthorized. +// without providing a refresh token results in HTTP 401 Unauthorized. func TestRefreshToken_Unauthorized_NoToken(t *testing.T) { t.Parallel() _, _ = resetTestData(t) @@ -488,10 +494,57 @@ func TestRefreshToken_Unauthorized_NoToken(t *testing.T) { handler.ServeHTTP(w, req) - // Without proper auth middleware, userID/role won't be in context - // The handler tries to query DB with empty userID, which should fail - if w.Code != http.StatusUnauthorized && w.Code != http.StatusInternalServerError { - t.Errorf("expected status 401 or 500, got %d. body: %s", w.Code, w.Body.String()) + if w.Code != http.StatusUnauthorized { + t.Errorf("expected status 401, got %d. body: %s", w.Code, w.Body.String()) + } +} + +// TestRefreshToken_Unauthorized_InvalidRefreshToken verifies that a bogus or +// unknown refresh token is rejected with 401. +func TestRefreshToken_Unauthorized_InvalidRefreshToken(t *testing.T) { + t.Parallel() + _, _ = resetTestData(t) + + handler := http.HandlerFunc(RefreshTokenHandler) + + req := httptest.NewRequest("POST", "/api/refresh-token", nil) + req.Header.Set("Authorization", "Bearer not-a-real-refresh-token") + w := httptest.NewRecorder() + + handler.ServeHTTP(w, req) + + if w.Code != http.StatusUnauthorized { + t.Errorf("expected status 401, got %d. body: %s", w.Code, w.Body.String()) + } +} + +// TestRefreshToken_AccessTokenRejected verifies the B5 fix: a stolen ACCESS +// token must NOT self-renew. Presenting it to /api/refresh-token is rejected +// because only a valid opaque refresh token can mint a new session. +func TestRefreshToken_AccessTokenRejected(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) + } + defer fixtures.DeleteUser(tx, userID) + + accessToken, _, err := auth.GenerateToken(userID, "verified_email") + if err != nil { + t.Fatalf("failed to generate access token: %v", err) + } + + req := httptest.NewRequest("POST", "/api/refresh-token", nil) + req.Header.Set("Authorization", "Bearer "+accessToken) + req = req.WithContext(ctx) + w := httptest.NewRecorder() + + RefreshTokenHandler(w, req) + + if w.Code != http.StatusUnauthorized { + t.Errorf("expected 401 when an access token is presented to refresh-token, got %d. body: %s", w.Code, w.Body.String()) } } @@ -1365,9 +1418,10 @@ func TestLogoutHandler_InvalidToken(t *testing.T) { // Refresh Token JTI Tests // ============================================================================= -// TestRefreshToken_RevokesOldJTI verifies that refreshing a token revokes the -// old JTI and issues a new one. The old token becomes invalid after refresh. -func TestRefreshToken_RevokesOldJTI(t *testing.T) { +// TestRefreshToken_RotatesRefreshToken verifies that a successful refresh +// CONSUMES the presented refresh token (rotation): replaying the same token +// afterwards is rejected 401, and the newly issued access token verifies. +func TestRefreshToken_RotatesRefreshToken(t *testing.T) { t.Parallel() ctx, tx := resetTestData(t) @@ -1378,52 +1432,40 @@ func TestRefreshToken_RevokesOldJTI(t *testing.T) { } defer fixtures.DeleteUser(tx, userID) - // Generate initial token and JTI - oldToken, oldJTI, err := auth.GenerateToken(userID, "verified_email") + refreshToken, err := auth.GenerateRefreshToken(ctx, userID, "verified_email") if err != nil { - t.Fatalf("failed to generate old token: %v", err) + t.Fatalf("failed to generate refresh token: %v", err) } - // Verify old JTI is not yet revoked - if auth.IsJTIRevoked(ctx, oldJTI) { - t.Fatal("old JTI should not be revoked before refresh") - } - - // Call refresh handler with old JTI in context + // First refresh succeeds and rotates the token. req := httptest.NewRequest("POST", "/api/refresh-token", nil) - req.Header.Set("Authorization", "Bearer "+oldToken) - reqCtx := ctx - reqCtx = context.WithValue(reqCtx, mw.UserIDKey, userID) - reqCtx = context.WithValue(reqCtx, mw.UserRoleKey, "verified_email") - reqCtx = context.WithValue(reqCtx, mw.JTIKey, oldJTI) - req = req.WithContext(reqCtx) + req.Header.Set("Authorization", "Bearer "+refreshToken) + req = req.WithContext(ctx) w := httptest.NewRecorder() - RefreshTokenHandler(w, req) - if w.Code != http.StatusOK { t.Fatalf("refresh failed: %d. body: %s", w.Code, w.Body.String()) } - // Verify old JTI was revoked - if !auth.IsJTIRevoked(ctx, oldJTI) { - t.Error("expected old JTI to be revoked after refresh") + // The used refresh token was consumed: replaying it fails 401. + req2 := httptest.NewRequest("POST", "/api/refresh-token", nil) + req2.Header.Set("Authorization", "Bearer "+refreshToken) + req2 = req2.WithContext(ctx) + w2 := httptest.NewRecorder() + RefreshTokenHandler(w2, req2) + if w2.Code != http.StatusUnauthorized { + t.Errorf("expected 401 for replayed (rotated) refresh token, got %d", w2.Code) } - // Verify old token is rejected by middleware - router := chi.NewRouter() - router.With(mw.RequireAuth).Get("/api/protected", func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - }) - - req2 := httptest.NewRequest("GET", "/api/protected", nil) - req2 = req2.WithContext(ctx) - req2.Header.Set("Authorization", "Bearer "+oldToken) - w2 := httptest.NewRecorder() - router.ServeHTTP(w2, req2) - - if w2.Code != http.StatusUnauthorized { - t.Errorf("expected 401 for revoked token after refresh, got %d", w2.Code) + // The freshly issued access token is a real, verifiable JWT. + var resp struct { + Token string `json:"token"` + } + if err := testutils.ParseResponseBody(w, &resp); err != nil { + t.Fatalf("failed to parse response: %v", err) + } + if _, _, _, err := auth.VerifyToken(resp.Token, ctx); err != nil { + t.Errorf("refreshed access token must verify: %v", err) } } @@ -1800,9 +1842,11 @@ func TestJTI_Revocation_PostgreSQL(t *testing.T) { // Login Response Field Tests (new from security pass) // ============================================================================= -// TestLogin_ResponseOmitsRefreshToken verifies the login response -// does not include refreshToken (G117 — 90-day credential excluded from JSON). -func TestLogin_ResponseOmitsRefreshToken(t *testing.T) { +// TestLogin_ResponseIncludesRefreshToken verifies the login response carries +// the opaque refresh token (B5 contract): the SPA must be able to persist it +// and present it to POST /api/refresh-token. The refresh token is hashed at +// rest in refresh_tokens and single-use (rotated on every refresh). +func TestLogin_ResponseIncludesRefreshToken(t *testing.T) { ctx, tx := resetTestData(t) handler := http.HandlerFunc(LoginHandler) @@ -1824,10 +1868,6 @@ func TestLogin_ResponseOmitsRefreshToken(t *testing.T) { t.Fatalf("expected 200, got %d. body: %s", w.Code, w.Body.String()) } - if strings.Contains(w.Body.String(), "refreshToken") { - t.Error("login response should NOT contain refreshToken (G117)") - } - var resp struct { Token string `json:"token"` JTI string `json:"jti"` @@ -1843,14 +1883,34 @@ func TestLogin_ResponseOmitsRefreshToken(t *testing.T) { if resp.JTI == "" { t.Error("expected non-empty jti") } - if resp.RefreshToken != "" { - t.Error("refreshToken should be excluded from JSON response (G117)") + if resp.RefreshToken == "" { + t.Error("expected non-empty refreshToken (B5)") + } + + // The issued refresh token must be stored (hashed) in refresh_tokens. + var dbCount int + if err := tx.QueryRow(ctx, "SELECT COUNT(*) FROM refresh_tokens WHERE user_id = $1", userID).Scan(&dbCount); err != nil { + t.Fatalf("failed to query refresh_tokens: %v", err) + } + if dbCount != 1 { + t.Errorf("expected 1 refresh_token row after login, got %d", dbCount) + } + + // And it must actually be usable: exchange it for a new access token. + req := httptest.NewRequest("POST", "/api/refresh-token", nil) + req.Header.Set("Authorization", "Bearer "+resp.RefreshToken) + req = req.WithContext(ctx) + rr := httptest.NewRecorder() + RefreshTokenHandler(rr, req) + if rr.Code != http.StatusOK { + t.Errorf("the login-issued refresh token must refresh successfully, got %d. body: %s", rr.Code, rr.Body.String()) } } -// TestRefreshToken_RevokesOldJTI_DBBacked verifies refresh still revokes old -// JTI and the revocation is persisted in the revoked_jtis table. -func TestRefreshToken_RevokesOldJTI_DBBacked(t *testing.T) { +// TestRefreshToken_RotatesRefreshToken_DBBacked verifies refresh-token +// rotation is DB-backed: after a successful refresh the used token's row is +// deleted and exactly one (new) refresh token row remains for the user. +func TestRefreshToken_RotatesRefreshToken_DBBacked(t *testing.T) { t.Parallel() ctx, tx := resetTestData(t) @@ -1860,42 +1920,40 @@ func TestRefreshToken_RevokesOldJTI_DBBacked(t *testing.T) { } defer fixtures.DeleteUser(tx, userID) - oldToken, oldJTI, err := auth.GenerateToken(userID, "verified_email") + refreshToken, err := auth.GenerateRefreshToken(ctx, userID, "verified_email") if err != nil { - t.Fatalf("failed to generate old token: %v", err) + t.Fatalf("failed to generate refresh token: %v", err) } - if auth.IsJTIRevoked(ctx, oldJTI) { - t.Fatal("old JTI should not be revoked before refresh") + var before int + if err := tx.QueryRow(ctx, "SELECT COUNT(*) FROM refresh_tokens WHERE user_id = $1", userID).Scan(&before); err != nil { + t.Fatalf("failed to count refresh tokens: %v", err) + } + if before != 1 { + t.Fatalf("expected 1 refresh token before refresh, got %d", before) } req := httptest.NewRequest("POST", "/api/refresh-token", nil) - req.Header.Set("Authorization", "Bearer "+oldToken) - reqCtx := ctx - reqCtx = context.WithValue(reqCtx, mw.UserIDKey, userID) - reqCtx = context.WithValue(reqCtx, mw.UserRoleKey, "verified_email") - reqCtx = context.WithValue(reqCtx, mw.JTIKey, oldJTI) - req = req.WithContext(reqCtx) + req.Header.Set("Authorization", "Bearer "+refreshToken) + req = req.WithContext(ctx) w := httptest.NewRecorder() - RefreshTokenHandler(w, req) - if w.Code != http.StatusOK { t.Fatalf("refresh failed: %d. body: %s", w.Code, w.Body.String()) } - if !auth.IsJTIRevoked(ctx, oldJTI) { - t.Error("expected old JTI to be revoked after refresh (DB-backed)") + // The used token was deleted (rotated) and a fresh one inserted — the row + // count is unchanged at 1, but the consumed token no longer verifies. + var after int + if err := tx.QueryRow(ctx, "SELECT COUNT(*) FROM refresh_tokens WHERE user_id = $1", userID).Scan(&after); err != nil { + t.Fatalf("failed to count refresh tokens after refresh: %v", err) + } + if after != 1 { + t.Errorf("expected exactly 1 refresh token after rotation, got %d", after) } - var dbCount int - err = tx.QueryRow(ctx, - "SELECT COUNT(*) FROM revoked_jtis WHERE jti = $1 AND expires_at > NOW()", oldJTI).Scan(&dbCount) - if err != nil { - t.Fatalf("failed to query revoked_jtis: %v", err) - } - if dbCount != 1 { - t.Errorf("expected 1 revoked_jtis row, got %d", dbCount) + if _, _, err := auth.VerifyRefreshToken(ctx, refreshToken); err == nil { + t.Error("the consumed refresh token must no longer verify (rotated)") } } diff --git a/backend/handlers/auth/local.go b/backend/handlers/auth/local.go index 1cb276f..d3e6b17 100644 --- a/backend/handlers/auth/local.go +++ b/backend/handlers/auth/local.go @@ -458,23 +458,57 @@ func LoginHandler(w http.ResponseWriter, r *http.Request) { return } + // Issue an opaque refresh token (B5): the 90-day credential is stored + // hashed in refresh_tokens and rotated on every use. A stolen ACCESS token + // can no longer self-renew — only a valid, unexpired, unrevoked refresh + // token can mint a new pair. The refresh token is returned in the body so + // the SPA can persist it and present it to POST /api/refresh-token. + refreshToken, err := auth.GenerateRefreshToken(r.Context(), userID, role) + if err != nil { + log.Printf("failed to issue refresh token for user %s: %v", userID, err) + http.Error(w, "could not generate refresh token", http.StatusInternalServerError) + return + } + if err := json.NewEncoder(w).Encode(auth.AuthResponse{ - Token: tokenString, - JTI: jti, + Token: tokenString, + JTI: jti, + RefreshToken: refreshToken, }); err != nil { log.Printf("Failed to encode JSON response: %v", err) } } -// POST /api/refresh-token (requires auth middleware) +// POST /api/refresh-token +// Requires a valid refresh token in the Authorization header (Bearer). The +// opaque refresh token is validated against the DB (hashed), consumed +// (rotated), and exchanged for a fresh access token + a NEW refresh token. +// +// B5 (security): the handler deliberately does NOT accept the access token. +// VerifyRefreshToken rotates (DELETEs) the presented refresh token, so a stolen +// access token can never self-renew — it expires in 1 hour and only a valid, +// unexpired, unrevoked refresh token can mint a new pair. A replayed refresh +// token (used twice) returns 401, detecting theft via rotation. func RefreshTokenHandler(w http.ResponseWriter, r *http.Request) { - userID, _ := mw.GetUserID(r.Context()) - role, _ := mw.GetUserRole(r.Context()) - oldJTI, _ := mw.GetJTI(r.Context()) + authHeader := r.Header.Get("Authorization") + if authHeader == "" || !strings.HasPrefix(authHeader, "Bearer ") { + mw.RespondJSON(w, http.StatusUnauthorized, map[string]string{"error": "missing or invalid authorization header"}) + return + } + refreshToken := strings.TrimPrefix(authHeader, "Bearer ") + + // VerifyRefreshToken consumes (rotates) the refresh token: the used token + // is deleted from refresh_tokens, so a stolen/leaked refresh token cannot + // be replayed and an access token alone can never mint a new session. + userID, role, err := auth.VerifyRefreshToken(r.Context(), refreshToken) + if err != nil { + mw.RespondJSON(w, http.StatusUnauthorized, map[string]string{"error": "invalid or expired refresh token"}) + return + } // Verify user still exists and role hasn't changed var currentRole string - err := db.Conn.QueryRow(r.Context(), ` + err = db.Conn.QueryRow(r.Context(), ` SELECT account_role FROM users WHERE id = $1 `, userID).Scan(¤tRole) @@ -488,24 +522,23 @@ func RefreshTokenHandler(w http.ResponseWriter, r *http.Request) { return } - // Revoke the old token's JTI before issuing a new one (rotation) - if oldJTI != "" { - // Best-effort revocation: log the error but continue with the refresh - if err := auth.RevokeJTI(r.Context(), oldJTI, clock.Now().Add(90*24*time.Hour)); err != nil { - slog.Error("refresh: failed to revoke old JTI", "oldJTI", oldJTI, "err", err) - } - } - - // Generate new token + // Issue a fresh access token + refresh token pair. newToken, jti, err := auth.GenerateToken(userID, currentRole) if err != nil { mw.RespondError(w, http.StatusInternalServerError, "could not generate token") return } + newRefreshToken, err := auth.GenerateRefreshToken(r.Context(), userID, currentRole) + if err != nil { + log.Printf("failed to issue rotated refresh token for user %s: %v", userID, err) + mw.RespondError(w, http.StatusInternalServerError, "could not generate refresh token") + return + } if err := json.NewEncoder(w).Encode(auth.AuthResponse{ - Token: newToken, - JTI: jti, + Token: newToken, + JTI: jti, + RefreshToken: newRefreshToken, }); err != nil { log.Printf("Failed to encode JSON response: %v", err) } @@ -518,6 +551,7 @@ func LogoutHandler(w http.ResponseWriter, r *http.Request) { http.Error(w, "invalid token", http.StatusUnauthorized) return } + userID, _ := mw.GetUserID(r.Context()) // Revoke the JTI — match the access token lifetime (1 hour) if err := auth.RevokeJTI(r.Context(), jti, clock.Now().Add(1*time.Hour)); err != nil { @@ -526,6 +560,16 @@ 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 != "" { + 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) + } + } + if err := json.NewEncoder(w).Encode(map[string]bool{"success": true}); err != nil { log.Printf("Failed to encode JSON response: %v", err) } diff --git a/backend/handlers/bookings/admin_reserve.go b/backend/handlers/bookings/admin_reserve.go index a1a53a3..dd83d75 100644 --- a/backend/handlers/bookings/admin_reserve.go +++ b/backend/handlers/bookings/admin_reserve.go @@ -12,7 +12,6 @@ import ( "fmt" "log" "log/slog" - "net" "net/http" "time" @@ -148,14 +147,12 @@ func AdminReserveSlotHandler(w http.ResponseWriter, r *http.Request) { // used by CheckTimeBlockerOverlap. The in-transaction DELETE is kept // as a safety net for the insert-phase. // Also clean up anonymous reservations matching this admin's IP - // (edge case: admin previously reserved without authentication). - ip := r.Header.Get("CF-Connecting-IP") - if ip == "" { - ip, _, _ = net.SplitHostPort(r.RemoteAddr) - if ip == "" { - ip = r.RemoteAddr - } - } + // (edge case: admin previously reserved without authentication). The IP + // goes through the SAME gated resolution the rate limiter uses + // (mw.ClientIP): CF-Connecting-IP is honored ONLY when + // TRUST_PROXY_HEADERS=true, so an origin-exposed backend can never be + // forced to key the ipHash on a client-controlled header (B7). + ip := mw.ClientIP(r) ipHash := fmt.Sprintf("%x", sha256.Sum256([]byte(ip)))[:8] if _, delErr := db.Conn.Exec(r.Context(), ` DELETE FROM time_blockers diff --git a/backend/handlers/bookings/overlap_test.go b/backend/handlers/bookings/overlap_test.go index a17b476..33272c5 100644 --- a/backend/handlers/bookings/overlap_test.go +++ b/backend/handlers/bookings/overlap_test.go @@ -2165,6 +2165,11 @@ func TestAdminReserveSlot_CleansUpAnonReservation(t *testing.T) { if token != "" { req.Header.Set("Authorization", "Bearer "+token) } + // B7: mw.ClientIP honors CF-Connecting-IP ONLY when TRUST_PROXY_HEADERS + // is true (unset here, so the header is ignored); the derived IP falls + // back to RemoteAddr. Set RemoteAddr so the handler's ipHash keys on + // the intended test IP, matching the origin-exposed deployment model. + req.RemoteAddr = ip + ":1234" req.Header.Set("CF-Connecting-IP", ip) baseCtx := req.Context() if len(requestCtx) > 0 { diff --git a/backend/handlers/bookings/reserve.go b/backend/handlers/bookings/reserve.go index 1771c36..841e478 100644 --- a/backend/handlers/bookings/reserve.go +++ b/backend/handlers/bookings/reserve.go @@ -17,7 +17,6 @@ import ( "crussell/handlers/scheduling" "crussell/internal/validators" "crussell/mw" - "net" "github.com/jackc/pgx/v5" ) @@ -65,14 +64,13 @@ func ReserveSlotHandler(w http.ResponseWriter, r *http.Request) { return } - // b. Extract client IP: check CF-Connecting-IP → X-Real-IP → X-Forwarded-For → RemoteAddr - ip := r.Header.Get("CF-Connecting-IP") - if ip == "" { - ip, _, _ = net.SplitHostPort(r.RemoteAddr) - if ip == "" { - ip = r.RemoteAddr - } - } + // b. Extract client IP through the SAME gated resolution the rate limiter + // uses (mw.ClientIP): CF-Connecting-IP is honored ONLY when + // TRUST_PROXY_HEADERS=true, so an origin-exposed backend can never be + // forced to key the anonymous-reservation ipHash on a client-controlled + // header (B7). An attacker who could rotate the header would otherwise mint + // a fresh anon bucket per request and evade the per-IP reservation cleanup. + ip := mw.ClientIP(r) // c. Detect auth: try context first (set by OptionalAuth middleware). // The inline Bearer fallback below is a safety net for the 22+ test diff --git a/backend/handlers/payments/discount_preview_test.go b/backend/handlers/payments/discount_preview_test.go index 44b4796..bec8ab1 100644 --- a/backend/handlers/payments/discount_preview_test.go +++ b/backend/handlers/payments/discount_preview_test.go @@ -524,7 +524,7 @@ func TestDiscountPreview_PaymentLock(t *testing.T) { t.Fatalf("failed to create first payment: %v", err) } - applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID) + applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID, nil) var discountCount int tx.QueryRow(ctx, @@ -544,7 +544,7 @@ func TestDiscountPreview_PaymentLock(t *testing.T) { t.Fatalf("failed to create second payment: %v", err) } - applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID) + applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID, nil) tx.QueryRow(ctx, "SELECT COUNT(*) FROM booking_discounts WHERE booking_id = $1", bookingID).Scan(&discountCount) diff --git a/backend/handlers/payments/errors_test.go b/backend/handlers/payments/errors_test.go index 2b7c445..cfc1f2b 100644 --- a/backend/handlers/payments/errors_test.go +++ b/backend/handlers/payments/errors_test.go @@ -538,7 +538,8 @@ func TestTwoFactorEnforced_CreatePaymentMethod_Blocked_403(t *testing.T) { } // TestTwoFactorEnforced_CreatePaymentMethod_With2FA_Succeeds verifies the gate -// lets a user WHO HAS enabled 2FA save a card through the add-card endpoint. +// lets a user WHO HAS enabled 2FA (and provides a matching one-time code) save +// a card through the add-card endpoint. func TestTwoFactorEnforced_CreatePaymentMethod_With2FA_Succeeds(t *testing.T) { t.Setenv("REQUIRE_2FA", "true") t.Setenv("SQUARE_ENVIRONMENT", "production") @@ -548,9 +549,7 @@ func TestTwoFactorEnforced_CreatePaymentMethod_With2FA_Succeeds(t *testing.T) { if err != nil { t.Fatalf("failed to create test user: %v", err) } - if _, err := tx.Exec(ctx, `UPDATE users SET two_factor_enabled = true WHERE id = $1`, userID); err != nil { - t.Fatalf("failed to enable 2FA: %v", err) - } + seedTwoFAPendingCode(t, tx, userID, "778899") t.Cleanup(func() { InvalidateSquareCustomerCache(userID) _, _ = db.Conn.Exec(context.Background(), `DELETE FROM user_saved_cards WHERE user_id = $1`, userID) @@ -558,9 +557,9 @@ func TestTwoFactorEnforced_CreatePaymentMethod_With2FA_Succeeds(t *testing.T) { token := jwt.GenerateUserToken(userID) handler := CreatePaymentMethod - w := makePaymentRequest(handler, "POST", "/api/user/payment-methods", CreatePaymentMethodRequest{CardToken: "cnon:2fa-ok"}, token, ctx) + w := makePaymentRequest(handler, "POST", "/api/user/payment-methods", CreatePaymentMethodRequest{CardToken: "cnon:2fa-ok", VerificationCode: "778899"}, token, ctx) if w.Code != http.StatusOK { - t.Fatalf("expected 200 when 2FA is enabled, got %d: %s", w.Code, w.Body.String()) + t.Fatalf("expected 200 when 2FA is enabled and the code matches, got %d: %s", w.Code, w.Body.String()) } var cardCount int @@ -617,12 +616,14 @@ func TestTwoFactorEnforced_BuyGiftCard_SaveCard_Blocked_403(t *testing.T) { // M7 — ConfirmOverflowTip (handlers.go CreateBookingPayment overflow gate) // ============================================================================= -// TestBookingPayment_Overflow_PostStart_Succeeds covers M7(c): once the booking -// has STARTED, an overpayment without confirm_overflow_tip succeeds (gratuity -// for service rendered is legitimate). The pre-start rejection (400 -// overflow_tip_confirmation_required) and the confirmed pre-start tip path are -// covered in m4_tip_refund_redesign_test.go. -func TestBookingPayment_Overflow_PostStart_Succeeds(t *testing.T) { +// TestBookingPayment_Overflow_PostStart_RequiresConfirmation locks B12: even +// on a booking that has STARTED, an overpayment that would become a tip is +// rejected with 400 overflow_tip_confirmation_required unless the client sets +// confirm_overflow_tip — an accidental overpayment (stale amount_due + +// discount preview) must never silently become gratuity. The pre-start +// rejection and the confirmed paths are covered in +// m4_tip_refund_redesign_test.go. +func TestBookingPayment_Overflow_PostStart_RequiresConfirmation(t *testing.T) { t.Parallel() ctx, tx := testutils.SetupTestTx(t) @@ -641,30 +642,36 @@ func TestBookingPayment_Overflow_PostStart_Succeeds(t *testing.T) { handler := CreateBookingPayment w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) - if w.Code != http.StatusOK { - t.Fatalf("expected 200 for a post-start overflow without confirmation, got %d: %s", w.Code, w.Body.String()) + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for a post-start overflow without confirmation, got %d: %s", w.Code, w.Body.String()) } - if strings.Contains(w.Body.String(), "overflow_tip_confirmation_required") { - t.Fatalf("a post-start overpayment must not require the overflow confirmation, body: %s", w.Body.String()) + if !strings.Contains(w.Body.String(), "overflow_tip_confirmation_required") { + t.Fatalf("expected the post-start overflow to require confirmation, body: %s", w.Body.String()) } + // No payment may be recorded for the rejected overflow. var payCount int if err := tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND status = 'completed'`, bookingID).Scan(&payCount); err != nil { t.Fatalf("failed to count payments: %v", err) } - if payCount != 2 { - t.Errorf("expected 2 completed payments after the post-start overflow (booking portion + F3 tip carve), got %d", payCount) + if payCount != 0 { + t.Errorf("expected 0 completed payments after the rejected post-start overflow, got %d", payCount) } - // F3: the £10 overflow beyond the £50 booking is carved as its own - // payment_type='tip' record (gratuity), mirroring buildTerminalSplitRecords. + // The same overflow WITH confirmation proceeds and carves the £10 excess + // as a tip record (gratuity). + req.ConfirmOverflowTip = true + w2 := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + if w2.Code != http.StatusOK { + t.Fatalf("expected 200 for a confirmed post-start overflow, got %d: %s", w2.Code, w2.Body.String()) + } var tipCount int var tipAmount float64 if err := tx.QueryRow(ctx, `SELECT COUNT(*), COALESCE(SUM(amount), 0) FROM payments WHERE booking_id = $1 AND status = 'completed' AND payment_type = 'tip'`, bookingID).Scan(&tipCount, &tipAmount); err != nil { t.Fatalf("failed to query tip records: %v", err) } if tipCount != 1 { - t.Errorf("expected exactly 1 tip record for the post-start overflow, got %d", tipCount) + t.Errorf("expected exactly 1 tip record for the confirmed post-start overflow, got %d", tipCount) } if tipAmount < 9.995 || tipAmount > 10.005 { t.Errorf("expected the tip to equal the £10 overflow, got %.2f", tipAmount) diff --git a/backend/handlers/payments/giftcards.go b/backend/handlers/payments/giftcards.go index be919ca..567c55a 100644 --- a/backend/handlers/payments/giftcards.go +++ b/backend/handlers/payments/giftcards.go @@ -12,6 +12,7 @@ import ( "net/http" "strconv" "strings" + "sync" "time" "crussell/clock" @@ -114,6 +115,10 @@ type BuyGiftCardRequest struct { // distinct purchases get different keys. Max=45 matches Square's limit. IdempotencyKey string `json:"idempotency_key,omitempty" validate:"omitempty,max=45"` VerificationToken *string `json:"verification_token,omitempty"` + // VerificationCode is the customer's current 2FA one-time code (B10): an + // enforced environment charges/persists a saved card only when this matches + // the customer's pending code. + VerificationCode string `json:"verification_code,omitempty"` } type RedeemGiftCardRequest struct { @@ -906,6 +911,100 @@ func TransferGiftCard(w http.ResponseWriter, r *http.Request) { // --- User Handlers --- +// --- Gift-card redeem failure rate limiting (B16) --- +// +// RedeemGiftCard converts a 12-hex gift-card code into account balance. The +// code space is small enough that an attacker can brute-force plausible codes +// through repeated redeem attempts; the route-level per-user limiter +// (mw.RateLimitByUser in main.go) throttles by ACCOUNT, but a distributed probe +// (many accounts, each trying codes) would still burn a DB lookup per attempt. +// This in-memory per-CODE counter adds a second layer: after +// giftCardRedeemFailMax consecutive "invalid code" failures for the same +// normalized code within giftCardRedeemFailWindow, further redeem attempts for +// that code are rejected 429 BEFORE any advisory lock or DB work. +// +// The counter is keyed on the normalized code, is reset whenever a redeem +// attempt resolves the code to a REAL card (any found row breaks the streak of +// invalid-code failures), and entries expire after the window so a one-off +// typo'd code is never locked out forever. In-memory only (no schema change — +// matching the A5 refund-failure counter in refunds.go, manualReconcileFailures), +// best-effort: a process restart clears it, and it is NOT a replacement for the +// account-level limiter. +const ( + // giftCardRedeemFailMax is how many consecutive invalid-code redeem + // failures against the same card code trigger the 429 lockout. + giftCardRedeemFailMax = 5 + // giftCardRedeemFailWindow is how long a code's failure streak is + // remembered. A card locked by repeated failures stays locked until the + // window elapses, then a fresh attempt starts a new streak. + giftCardRedeemFailWindow = 15 * time.Minute + // giftCardRedeemFailMaxEntries caps the in-memory map so the counter can + // never grow unbounded (one entry per code being probed). + giftCardRedeemFailMaxEntries = 10_000 +) + +type giftCardRedeemFailState struct { + count int + windowEnd time.Time +} + +var ( + giftCardRedeemFailMu sync.Mutex + giftCardRedeemFails = make(map[string]giftCardRedeemFailState) +) + +// giftCardRedeemLocked reports whether a code is currently in the 429 lockout +// (>= giftCardRedeemFailMax consecutive invalid-code failures inside the +// window). An expired entry is treated as not locked — the window has passed. +func giftCardRedeemLocked(code string) bool { + giftCardRedeemFailMu.Lock() + defer giftCardRedeemFailMu.Unlock() + st, ok := giftCardRedeemFails[code] + if !ok || clock.Now().After(st.windowEnd) { + return false + } + return st.count >= giftCardRedeemFailMax +} + +// giftCardRedeemFail records one more consecutive invalid-code failure for a +// code. A fresh streak (new code, or the previous streak expired) starts at 1 +// with a fresh window. Bounded: expired entries are purged on insert and, if +// the map is still full, one entry is evicted so the map never exceeds +// giftCardRedeemFailMaxEntries. +func giftCardRedeemFail(code string) { + giftCardRedeemFailMu.Lock() + defer giftCardRedeemFailMu.Unlock() + now := clock.Now() + if st, ok := giftCardRedeemFails[code]; ok && !now.After(st.windowEnd) { + st.count++ + giftCardRedeemFails[code] = st + return + } + if len(giftCardRedeemFails) >= giftCardRedeemFailMaxEntries { + for c, st := range giftCardRedeemFails { + if now.After(st.windowEnd) { + delete(giftCardRedeemFails, c) + } + } + if len(giftCardRedeemFails) >= giftCardRedeemFailMaxEntries { + for c := range giftCardRedeemFails { + delete(giftCardRedeemFails, c) + break + } + } + } + giftCardRedeemFails[code] = giftCardRedeemFailState{count: 1, windowEnd: now.Add(giftCardRedeemFailWindow)} +} + +// giftCardRedeemReset clears a code's invalid-code-failure streak — called +// whenever a redeem attempt resolves the code to a real gift card, because a +// successful resolution breaks the consecutive-failure run. +func giftCardRedeemReset(code string) { + giftCardRedeemFailMu.Lock() + delete(giftCardRedeemFails, code) + giftCardRedeemFailMu.Unlock() +} + func RedeemGiftCard(w http.ResponseWriter, r *http.Request) { ctx := r.Context() userID, ok := ctx.Value(mw.UserIDKey).(string) @@ -926,6 +1025,16 @@ func RedeemGiftCard(w http.ResponseWriter, r *http.Request) { return } + // B16: a code in the per-card lockout (>= giftCardRedeemFailMax + // consecutive invalid-code failures inside the window) is rejected 429 + // BEFORE the advisory lock or any DB work — brute-force probing of the + // 12-hex code space never reaches the database. + if giftCardRedeemLocked(code) { + log.Printf("Gift card redeem rate-limited: code %s has failed too many consecutive redeem attempts", code) + http.Error(w, "Too many failed redeem attempts for this gift card — try again later", http.StatusTooManyRequests) + return + } + // Serialize against a concurrent cancellation of this same card: the // gift-card cancel flow (CancelGiftCard) holds this session advisory lock // across its eligibility check AND its funding reversal, so redeeming the @@ -974,6 +1083,10 @@ func RedeemGiftCard(w http.ResponseWriter, r *http.Request) { `, code).Scan(&amountRemaining, &redeemedBy) if err != nil { if errors.Is(err, pgx.ErrNoRows) { + // B16: an "invalid code" failure — count it against the per-card + // lockout so repeated brute-force attempts on the same code get 429 + // after giftCardRedeemFailMax consecutive misses. + giftCardRedeemFail(code) http.Error(w, "Invalid or expired gift card code", http.StatusNotFound) return } @@ -981,6 +1094,9 @@ func RedeemGiftCard(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal server error", http.StatusInternalServerError) return } + // B16: the code resolved to a real gift card — a found row breaks the + // consecutive invalid-code-failure streak, so clear the per-card counter. + giftCardRedeemReset(code) if redeemedBy.Valid { http.Error(w, "This gift card has already been redeemed", http.StatusBadRequest) @@ -1353,7 +1469,7 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { // CreateBookingPayment/CreateTipPayment. A one-off new-card (nonce) charge // that is not saved is not gated. if (req.CardID != nil && *req.CardID != "") || req.SaveCard { - if !requireTwoFactorForCardAccess(w, r, paymentService, userID) { + if !requireTwoFactorForCardAccess(w, r, paymentService, userID, req.VerificationCode) { return } } diff --git a/backend/handlers/payments/giftcards_test.go b/backend/handlers/payments/giftcards_test.go index 834c225..a928a14 100644 --- a/backend/handlers/payments/giftcards_test.go +++ b/backend/handlers/payments/giftcards_test.go @@ -12,6 +12,7 @@ import ( "testing" "time" + "crussell/clock" "crussell/db" "crussell/mw" "crussell/testutils" @@ -267,7 +268,7 @@ func TestBuyGiftCard_Self(t *testing.T) { // Charge a mock payment token reqBody, _ := json.Marshal(map[string]interface{}{ - "amount": 2000, // £20.00 in cents + "amount": 2000, // £20.00 in pence "recipient_type": "self", "new_card_token": "cnon:card-nonce-ok", "idempotency_key": "idempotency-key-buy-gc-self", @@ -369,7 +370,7 @@ func TestBuyGiftCard_Friend(t *testing.T) { token := jwt.GenerateTestToken(userID, "verified_email") reqBody, _ := json.Marshal(map[string]interface{}{ - "amount": 5000, // £50.00 in cents + "amount": 5000, // £50.00 in pence "recipient_type": "friend", "new_card_token": "cnon:card-nonce-ok", "idempotency_key": "idempotency-key-buy-gc-friend", @@ -447,7 +448,7 @@ func TestAdminRecordPayment_CashAndGiftCard(t *testing.T) { // 1. Pay £30 with CASH reqBody, _ := json.Marshal(map[string]interface{}{ - "amount": 3000, // £30.00 in cents + "amount": 3000, // £30.00 in pence "payment_type": "full", "payment_method": "cash", }) @@ -479,7 +480,7 @@ func TestAdminRecordPayment_CashAndGiftCard(t *testing.T) { // 2. Pay £40 with PHYSICAL GIFT CARD (guest checkout simulation) reqBody2, _ := json.Marshal(map[string]interface{}{ - "amount": 4000, // £40.00 in cents + "amount": 4000, // £40.00 in pence "payment_type": "full", "payment_method": "giftcard", "gift_card_id": cardID, @@ -499,14 +500,17 @@ func TestAdminRecordPayment_CashAndGiftCard(t *testing.T) { t.Errorf("expected status 200, got %d. Body: %s", w2.Code, w2.Body.String()) } - // Verify gift card balance deducted from card directly + // Verify gift card balance deducted from card directly. B3: after the £30 + // cash payment the remaining obligation is £20, so the £40 gift-card + // payment is clamped to £20 (the PaymentModal amount that ignored prior + // payments must never be recorded verbatim) — the card keeps £80. var remaining float64 err = tx.QueryRow(ctx, "SELECT amount_remaining FROM gift_cards WHERE id = $1", cardID).Scan(&remaining) if err != nil { t.Fatalf("failed to query card: %v", err) } - if remaining != 60.00 { - t.Errorf("expected gift card balance to be 60.00, got %.2f", remaining) + if remaining != 80.00 { + t.Errorf("expected gift card balance to be 80.00 (only the £20 remaining obligation deducted), got %.2f", remaining) } // Verify gift card payment record created @@ -525,7 +529,7 @@ func TestAdminRecordPayment_CashAndGiftCard(t *testing.T) { // Now pay £25 using user account balance reqBody3, _ := json.Marshal(map[string]interface{}{ - "amount": 2500, // £25.00 in cents + "amount": 2500, // £25.00 in pence "payment_type": "full", "payment_method": "giftcard", }) @@ -1835,6 +1839,169 @@ func TestRedeemGiftCard_ZeroBalance(t *testing.T) { } } +// ============================================================================= +// RedeemGiftCard — B16 per-card failure rate limiting +// ============================================================================= + +// redeemCodeRequest dispatches a redeem request for the given code against +// RedeemGiftCard, reusing the caller's test transaction. +func redeemCodeRequest(t *testing.T, token string, tx pgx.Tx, code string) *httptest.ResponseRecorder { + t.Helper() + reqBody, _ := json.Marshal(map[string]interface{}{"code": code}) + req := httptest.NewRequest("POST", "/api/user/giftcards/redeem", bytes.NewBuffer(reqBody)) + req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(db.ContextWithTx(req.Context(), tx)) + w := httptest.NewRecorder() + r := chi.NewRouter() + r.Use(mw.RequireAuth) + r.Post("/api/user/giftcards/redeem", RedeemGiftCard) + r.ServeHTTP(w, req) + return w +} + +// TestGiftCardRedeemFailCounter_Semantics unit-tests the B16 in-memory per-card +// failure counter: 5 consecutive failures lock a code, a reset clears it, and a +// fresh streak starts after the window expires. +func TestGiftCardRedeemFailCounter_Semantics(t *testing.T) { + const code = "b16bad000004" + giftCardRedeemReset(code) + + if giftCardRedeemLocked(code) { + t.Fatal("expected a fresh code to start unlocked") + } + giftCardRedeemFail(code) + if giftCardRedeemLocked(code) { + t.Fatal("expected a single failure not to lock the code") + } + for i := 0; i < giftCardRedeemFailMax-1; i++ { + giftCardRedeemFail(code) + } + if !giftCardRedeemLocked(code) { + t.Fatal("expected the code to be locked after giftCardRedeemFailMax consecutive failures") + } + + giftCardRedeemReset(code) + if giftCardRedeemLocked(code) { + t.Fatal("expected reset to clear the lock") + } + + // Window expiry: re-lock, then push the entry's window into the past. + for i := 0; i < giftCardRedeemFailMax; i++ { + giftCardRedeemFail(code) + } + if !giftCardRedeemLocked(code) { + t.Fatal("expected the code to be locked again for the expiry check") + } + giftCardRedeemFailMu.Lock() + if st, ok := giftCardRedeemFails[code]; ok { + st.windowEnd = clock.Now().Add(-time.Minute) + giftCardRedeemFails[code] = st + } + giftCardRedeemFailMu.Unlock() + if giftCardRedeemLocked(code) { + t.Fatal("expected the code to be unlocked after the window expired") + } + giftCardRedeemFail(code) // fresh streak starts at 1 + if giftCardRedeemLocked(code) { + t.Fatal("expected a fresh streak to need giftCardRedeemFailMax failures again") + } + giftCardRedeemReset(code) +} + +// TestRedeemGiftCard_InvalidCode_RateLimit429 locks the B16 HTTP behaviour: 5 +// consecutive invalid-code failures for the same code return 404, and the 6th +// attempt is rejected 429 before any DB work. +func TestRedeemGiftCard_InvalidCode_RateLimit429(t *testing.T) { + _, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + token := jwt.GenerateTestToken(userID, "verified_email") + + const badCode = "b16bad000001" + giftCardRedeemReset(badCode) + for i := 0; i < giftCardRedeemFailMax; i++ { + if w := redeemCodeRequest(t, token, tx.(pgx.Tx), badCode); w.Code != http.StatusNotFound { + t.Fatalf("attempt %d: expected 404 for an unknown code, got %d. body: %s", i+1, w.Code, w.Body.String()) + } + } + if w := redeemCodeRequest(t, token, tx.(pgx.Tx), badCode); w.Code != http.StatusTooManyRequests { + t.Errorf("expected 429 after %d consecutive invalid-code failures, got %d. body: %s", giftCardRedeemFailMax, w.Code, w.Body.String()) + } + giftCardRedeemReset(badCode) +} + +// TestRedeemGiftCard_RateLimit_LockedBlocksEvenExistingCard proves the 429 +// check runs BEFORE the DB lookup: once a code is locked, even a real card +// created under that code is rejected until the window expires. +func TestRedeemGiftCard_RateLimit_LockedBlocksEvenExistingCard(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + token := jwt.GenerateTestToken(userID, "verified_email") + + const code = "b16bad000002" + giftCardRedeemReset(code) + for i := 0; i < giftCardRedeemFailMax; i++ { + if w := redeemCodeRequest(t, token, tx.(pgx.Tx), code); w.Code != http.StatusNotFound { + t.Fatalf("attempt %d: expected 404, got %d", i+1, w.Code) + } + } + // The code is now locked; a real card under it must still be blocked. + if _, err := tx.Exec(ctx, ` + INSERT INTO gift_cards (id, total_funds_added, amount_remaining) + VALUES ($1, 50.00, 50.00) + `, code); err != nil { + t.Fatalf("failed to create gift card: %v", err) + } + if w := redeemCodeRequest(t, token, tx.(pgx.Tx), code); w.Code != http.StatusTooManyRequests { + t.Errorf("expected 429 for a locked code even when a real card exists, got %d. body: %s", w.Code, w.Body.String()) + } + giftCardRedeemReset(code) +} + +// TestRedeemGiftCard_RateLimit_ResetOnFoundRow verifies a redeem attempt that +// resolves the code to a REAL card clears the code's invalid-code streak (the +// consecutive-failure definition breaks when the code is found). +func TestRedeemGiftCard_RateLimit_ResetOnFoundRow(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + token := jwt.GenerateTestToken(userID, "verified_email") + + const code = "b16bad000003" + giftCardRedeemReset(code) + for i := 0; i < giftCardRedeemFailMax-1; i++ { + if w := redeemCodeRequest(t, token, tx.(pgx.Tx), code); w.Code != http.StatusNotFound { + t.Fatalf("attempt %d: expected 404, got %d", i+1, w.Code) + } + } + if giftCardRedeemLocked(code) { + t.Fatal("expected the code not to be locked after 4 failures") + } + // Now the card exists under the code and is redeemed — the found row must + // clear the streak. + if _, err := tx.Exec(ctx, ` + INSERT INTO gift_cards (id, total_funds_added, amount_remaining) + VALUES ($1, 50.00, 50.00) + `, code); err != nil { + t.Fatalf("failed to create gift card: %v", err) + } + if w := redeemCodeRequest(t, token, tx.(pgx.Tx), code); w.Code != http.StatusOK { + t.Fatalf("expected a successful redeem of the real card, got %d. body: %s", w.Code, w.Body.String()) + } + if giftCardRedeemLocked(code) { + t.Error("expected the found-row redeem to reset the invalid-code streak") + } + giftCardRedeemReset(code) +} + // ============================================================================= // TopUpGiftCard — Additional edge cases // ============================================================================= diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index 52181bb..85d68fb 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -36,6 +36,10 @@ type CreateTerminalPaymentRequest struct { // directly, bypassing the terminal. The frontend sends this for the admin // "Charge Saved Card" action. UserSavedCardID *string `json:"saved_card_id,omitempty"` + // verification_code: the customer's current 2FA one-time code (B10). An + // enforced environment charges a saved card only when this matches the + // customer's pending code; the operator relays it from the [2FA] log/email. + VerificationCode string `json:"verification_code,omitempty"` // idempotency_key: optional client-generated per-attempt UUID for saved-card // charges. The frontend generates one per distinct charge and reuses it // across retries of the SAME charge, so two DISTINCT identical charges on @@ -55,12 +59,16 @@ type CreateBookingPaymentRequest struct { SaveCard bool `json:"save_card"` IdempotencyKey string `json:"idempotency_key,omitempty" validate:"omitempty,max=45"` VerificationToken *string `json:"verification_token,omitempty"` + // VerificationCode is the customer's current 2FA one-time code (B10): an + // enforced environment charges a saved card only when this matches the + // customer's pending code. + VerificationCode string `json:"verification_code,omitempty"` // ConfirmOverflowTip acknowledges that an overpayment beyond the booking's // remaining balance will be recorded as a tip (M7). Tips cannot be paid in // advance, so a pre-start overpayment is rejected with 400 // overflow_tip_confirmation_required unless the client sets this flag; the - // frontend prompts and resends with it. Post-start overpayments are always - // accepted (gratuity for service rendered). + // frontend prompts and resends with it. B12: post-start overpayments + // require the flag too. ConfirmOverflowTip bool `json:"confirm_overflow_tip"` } @@ -82,6 +90,10 @@ type CreateTipPaymentRequest struct { SaveCard bool `json:"save_card"` IdempotencyKey string `json:"idempotency_key,omitempty" validate:"omitempty,max=45"` VerificationToken *string `json:"verification_token,omitempty"` + // VerificationCode is the customer's current 2FA one-time code (B10): an + // enforced environment charges a saved card only when this matches the + // customer's pending code. + VerificationCode string `json:"verification_code,omitempty"` } type CheckoutResponse struct { @@ -270,6 +282,33 @@ func calculateDiscountPreview(ctx context.Context, bookingID string, userID stri return resp } +// clampTerminalChargeToRemainingBalance caps a requested terminal charge at the +// booking's remaining obligation (B3). The admin "Take Payment" PaymentModal +// sends subtotal - discounts - campaignDiscountPence, which ignores PRIOR +// payments; recording that verbatim would overcharge the customer (or carve +// the excess into an unintended tip). The clamp keeps the recorded/charged +// money within the actual obligation and returns whether the amount was +// reduced. Callers MUST have serialized the attempt (advisory lock or the +// booking FOR UPDATE row lock) so the remaining-balance read races no +// concurrent same-booking payment. The frontend must handle the discrepancy +// between the amount it displayed and the clamped amount that was charged. +// +// A fully-paid booking (remaining <= 0) is NOT clamped: a deliberate +// overpayment the admin records is real money received and must stay on the +// ledger (the app's documented "overpayment handled manually at the counter" +// semantics — e.g. two identical cash receipts). The clamp protects the common +// B3 case where prior payments left a POSITIVE remaining obligation. +func clampTerminalChargeToRemainingBalance(ctx context.Context, bookingID string, amount int64) (effective, remaining int64, clamped bool, err error) { + remaining, err = NewPaymentService().GetBookingRemainingBalancePence(ctx, bookingID) + if err != nil { + return amount, 0, false, err + } + if amount > remaining && remaining > 0 { + return remaining, remaining, true, nil + } + return amount, remaining, false, nil +} + func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // Defense-in-depth admin check (S-1) — the route is mounted under // mw.RequireAdmin; this keeps terminal charges admin-only regardless. @@ -379,6 +418,22 @@ 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 records the requested amount verbatim + // (the helper returns clamped=false for remaining <= 0). + effectiveAmount, _, 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 { + 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 @@ -570,7 +625,7 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // 2FA gating (C5): charging the customer's SAVED card requires 2FA when // the feature is enforced. Gate on the card's owner — the booking's // user, not the admin. New-card/terminal paths are not gated. - if bookingUserID.Valid && !requireTwoFactorForCardAccess(w, r, service, bookingUserID.String) { + if bookingUserID.Valid && !requireTwoFactorForCardAccess(w, r, service, bookingUserID.String, req.VerificationCode) { return } @@ -595,6 +650,21 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { } defer releaseBookingPaymentLock(pinConn, "crussell:payment:"+bookingID) + // B3: clamp the amount to the booking's remaining obligation. The + // advisory lock above serializes all same-booking payment attempts, so + // this read races no concurrent charge. A fully-paid booking records + // the requested amount verbatim (clamped=false for remaining <= 0). + effectiveAmount, _, clamped, cErr := clampTerminalChargeToRemainingBalance(r.Context(), bookingID, amount) + if cErr != nil { + log.Printf("Failed to compute remaining balance for saved-card payment on booking %s: %v", bookingID, cErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + if clamped { + log.Printf("Saved-card 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", bookingID, amount, effectiveAmount) + amount = effectiveAmount + } + // Idempotency key — two tiers: // 1. Client-supplied per-attempt UUID (preferred): the frontend // generates one per DISTINCT charge and reuses it across retries of @@ -674,6 +744,15 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { return } + // B13: the pre-charge discount SET for the post-charge apply-time + // re-check. The online booking path (CreateBookingPayment) keeps the set + // computed BEFORE the charge so applyEligibleCampaignsAtPayment can + // detect a campaign exhausted by a concurrent redemption between the + // frontend's preview and the apply-time re-check; the saved-card path + // snapshots it here, under the same advisory lock, before the Square + // charge. + var preChargeDiscounts []EligibleDiscount + // Pending-first: insert a pending payment record, commit, then charge. tx, err := db.Conn.Begin(r.Context()) if err != nil { @@ -720,6 +799,14 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { log.Printf("Failed to update square_source_id on reused saved-card payment %s: %v", paymentID, srcErr) } } + // B13: snapshot the pre-charge discount set (read-only, under the + // advisory lock) so the post-charge re-check can surface a campaign + // exhausted by a concurrent redemption (see the declaration above). + var bookingTotal float64 + if err := tx.QueryRow(r.Context(), `SELECT total_amount FROM bookings WHERE id = $1`, bookingID).Scan(&bookingTotal); err != nil { + log.Printf("Failed to load booking total for discount computation: %v", err) + } + preChargeDiscounts = ComputeEligibleDiscounts(r.Context(), tx, bookingID, bookingUserID.String, bookingTotal) if err := tx.Commit(r.Context()); err != nil { log.Printf("Failed to commit pending saved-card payment: %v", err) http.Error(w, "internal server error", http.StatusInternalServerError) @@ -816,6 +903,34 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { return } + // B13: apply eligible campaign discounts at charge time (mirroring the + // online booking path at CreateBookingPayment). This runs INSIDE the + // same transaction as the completed flip, BEFORE the flip, so the + // capDiscountToRemainingObligation headroom still counts the in-flight + // charge as the pending row (F1 — an over-credit can never be minted) + // and ComputeEligibleDiscounts' 2+-payments guard sees the same + // completed-payment count the online path sees. The apply is idempotent + // (ComputeEligibleDiscounts excludes already-recorded sources). A + // campaign exhausted by a concurrent redemption between the frontend's + // preview and this apply-time re-check surfaces the same + // campaignExhaustedAtApplyError → campaign_fully_redeemed path the + // booking path returns, instead of silently skipping the discount and + // leaving the booking underpaid. The completion side-effects + // (completeFullyPaidBooking → ApplyBookingCompletionSideEffects) skip + // re-application via their already-recorded guards. + campaignLostPence := int64(0) + var campaignLostID string + if applyErr := applyEligibleCampaignsAtPayment(r.Context(), recheckTx, bookingID, bookingUserID.String, preChargeDiscounts); applyErr != nil { + var exErr *campaignExhaustedAtApplyError + if errors.As(applyErr, &exErr) { + campaignLostPence = exErr.lostPence + campaignLostID = exErr.campaignID + log.Printf("B13: campaign %s exhausted between preview and apply for booking %s — lost discount %d pence; payment will complete and the difference will be returned to the customer", campaignLostID, bookingID, campaignLostPence) + } else { + log.Printf("Failed to apply eligible campaigns for booking %s: %v", bookingID, applyErr) + } + } + if _, upErr := recheckTx.Exec(r.Context(), `UPDATE payments SET status = 'completed', square_payment_id = $1 WHERE id = $2`, paymentResult.SquarePayID, paymentID, @@ -824,6 +939,14 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { 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 + // called apply_vat_to_payment and the row kept is_vat_applicable=FALSE + // with no vat_rate/vat_amount/net_amount — a real VAT-reporting loss for + // a VAT-registered business. ApplyVATToBookingPayment reads config and + // skips discount/on_the_house/tip rows defensively. + ApplyVATToBookingPayment(r.Context(), recheckTx, paymentID) if cErr := recheckTx.Commit(r.Context()); cErr != nil { log.Printf("CRITICAL: Square payment %s succeeded but committing the post-charge status update for payment %s failed: %v — manual reconciliation required", paymentResult.SquarePayID, paymentID, cErr) @@ -834,13 +957,31 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // 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 - // above, so the completion side-effects (loyalty, campaign discounts, - // deposits_required) are atomic and a booking paid in full by a - // saved-card charge leaves the admin's Current Appointment view. The - // eligible-discount application happens inside the completion - // side-effects, guarded by the same over-credit cap as every other path. + // above, so the completion side-effects (loyalty, deposits_required) are + // atomic and a booking paid in full by a saved-card charge leaves the + // admin's Current Appointment view. Campaign discounts were already + // applied at charge time above (B13); the completion side-effects skip + // re-application via their already-recorded guards. completeFullyPaidBooking(r.Context(), bookingID) + // B13: a campaign the frontend showed as eligible at preview was + // exhausted by a concurrent redemption before this charge applied it. + // The charge already succeeded at Square and the payment is committed — + // mirror the online booking path: honour the promised discount (a + // gift-card balance credit when the full price was charged) and return + // campaign_fully_redeemed so the frontend does not show the discount as + // applied. The booking-completion flow above ran regardless, exactly + // like the online path's in-transaction completion. + if campaignLostPence > 0 { + credited := refundLostCampaignAsBalanceCredit(r.Context(), bookingID, bookingUserID.String, campaignLostPence) + log.Printf("B13: campaign %s fully redeemed before payment %s applied it — lost discount %d pence (%s), returning 400 campaign_fully_redeemed to the frontend", campaignLostID, paymentID, campaignLostPence, credited) + mw.RespondJSON(w, http.StatusBadRequest, map[string]string{ + "error": "The discount campaign has been fully redeemed. The full amount applies.", + "code": "campaign_fully_redeemed", + }) + return + } + // Return the card details the frontend reads for the success state // (MINOR-R2) — CheckoutResponse alone leaves card_brand/card_last4 blank. if err := json.NewEncoder(w).Encode(map[string]any{ @@ -904,10 +1045,35 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // PRIMARY KEY; a row carrying one is provably pre-Square (no checkout was // ever created for it). provisionalID := "tmp-" + idempotencyKey + + // B3: clamp the terminal checkout amount to the booking's remaining + // obligation UNLESS the customer explicitly requested a tip (tip_enabled). + // An accidental overpayment must never be presented to the card reader as a + // charge that the record path would later carve into an unintended tip. The + // advisory lock above serializes this read against concurrent same-booking + // payments. A fully-paid booking keeps the requested amount verbatim + // (clamped=false for remaining <= 0). + checkoutAmount := amount + if !req.TipEnabled { + effectiveAmount, _, clamped, cErr := clampTerminalChargeToRemainingBalance(r.Context(), bookingID, amount) + if cErr != nil { + log.Printf("Failed to compute remaining balance for terminal checkout on booking %s: %v", bookingID, cErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + if clamped { + log.Printf("Terminal checkout for booking %s clamped from %d to %d pence (remaining obligation) — the frontend PaymentModal sent an amount that ignored prior payments; the card reader will present the remaining obligation only", bookingID, amount, effectiveAmount) + checkoutAmount = effectiveAmount + } + } + // tip_enabled is persisted on the checkout row so recordTerminalPaymentTx + // knows whether an overflow beyond the remaining value was an EXPLICIT tip + // (split into a tip record) or an accidental overpayment (kept on the + // booking record, refundable). if _, err := db.Conn.Exec(r.Context(), ` - INSERT INTO terminal_checkouts (checkout_id, booking_id, payment_type, status, amount) - VALUES ($1, $2, $3, 'PENDING', $4) - `, provisionalID, bookingID, req.PaymentType, float64(amount)/100.0); err != nil { + INSERT INTO terminal_checkouts (checkout_id, booking_id, payment_type, status, amount, tip_enabled) + VALUES ($1, $2, $3, 'PENDING', $4, $5) + `, provisionalID, bookingID, req.PaymentType, float64(checkoutAmount)/100.0, req.TipEnabled); err != nil { log.Printf("Failed to record provisional terminal checkout for booking %s: %v", bookingID, err) http.Error(w, "internal server error", http.StatusInternalServerError) return @@ -929,7 +1095,7 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { } checkoutReq := square.CreateCheckoutReq{ - Amount: amount, + Amount: checkoutAmount, Currency: "GBP", IdempotencyKey: idempotencyKey, ReferenceID: bookingID, @@ -1321,7 +1487,7 @@ 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) { + if req.SaveCard && !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode) { return } @@ -1667,8 +1833,12 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { if err := tx.QueryRow(r.Context(), `SELECT total_amount FROM bookings WHERE id = $1`, bookingID).Scan(&bookingTotal); err != nil { log.Printf("Failed to load booking total for discount computation: %v", err) } + // B13: keep the pre-charge discount SET (not just the sum) so + // applyEligibleCampaignsAtPayment can detect a campaign exhausted by a + // concurrent redemption between this computation and the apply-time re-run. + preChargeDiscounts := ComputeEligibleDiscounts(r.Context(), tx, bookingID, userID, bookingTotal) var eligibleDiscountPence int64 - for _, d := range ComputeEligibleDiscounts(r.Context(), tx, bookingID, userID, bookingTotal) { + for _, d := range preChargeDiscounts { eligibleDiscountPence += int64(math.Round(d.Amount * 100)) } @@ -1696,14 +1866,16 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { } discountedRemainingPence := remainingPence + eligibleDiscountPence if req.Amount > discountedRemainingPence { - var bookingStartTime time.Time - if sErr := db.Conn.QueryRow(r.Context(), `SELECT start_time FROM bookings WHERE id = $1`, bookingID).Scan(&bookingStartTime); sErr != nil { - log.Printf("Failed to get booking start time: %v", sErr) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } - if !req.ConfirmOverflowTip && !bookingStartTime.Before(clock.Now()) { - log.Printf("Overflow requires confirmation: amount %d exceeds discounted remaining %d for booking %s (not started, not confirmed)", req.Amount, discountedRemainingPence, bookingID) + // B12: an overflow that would become a tip ALWAYS requires the + // customer's explicit confirmation (confirm_overflow_tip) — both + // pre-start AND post-start. Previously only a pre-start overflow + // required the flag and a post-start overflow became a tip + // silently; the frontend's stale amount_due + discount preview + // could then mint an unintended tip. When the flag is absent the + // request is rejected with overflow_tip_confirmation_required so + // the frontend can prompt, regardless of booking state. + if !req.ConfirmOverflowTip { + log.Printf("Overflow requires confirmation: amount %d exceeds discounted remaining %d for booking %s", req.Amount, discountedRemainingPence, bookingID) mw.RespondJSON(w, http.StatusBadRequest, map[string]string{ "error": "The extra amount will be recorded as a tip. Confirm to continue.", "code": "overflow_tip_confirmation_required", @@ -1758,7 +1930,7 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // 2FA gating (C5): charging a SAVED card requires 2FA when the feature is // enforced. New-card (nonce) charges are not gated. if req.CardID != nil && *req.CardID != "" { - if !requireTwoFactorForCardAccess(w, r, service, userID) { + if !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode) { return } } @@ -1945,7 +2117,22 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // discounted total is applied and bookingIsFullyPaid (which counts // discount rows) completes the booking. The call is idempotent: discounts // already recorded for the booking are skipped by the duplicate check. - applyEligibleCampaignsAtPayment(r.Context(), tx2, bookingID, userID) + // B13: the apply-time re-check can discover that a campaign the customer + // was promised at preview was exhausted by a concurrent redemption — the + // lost discount must not be silently swallowed (see the error handling + // after the commit below). + campaignLostPence := int64(0) + var campaignLostID string + if applyErr := applyEligibleCampaignsAtPayment(r.Context(), tx2, bookingID, userID, preChargeDiscounts); applyErr != nil { + var exErr *campaignExhaustedAtApplyError + if errors.As(applyErr, &exErr) { + campaignLostPence = exErr.lostPence + campaignLostID = exErr.campaignID + log.Printf("B13: campaign %s exhausted between preview and apply for booking %s — lost discount %d pence; payment will complete and the difference will be returned to the customer", campaignLostID, bookingID, campaignLostPence) + } else { + log.Printf("Failed to apply eligible campaigns for booking %s: %v", bookingID, applyErr) + } + } // Build payment records — may split a single Square charge into // a deposit portion (up to 50% of booking total) plus a balance @@ -2077,6 +2264,28 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { return } + // B13: a campaign was exhausted between the preview and the apply-time + // re-check. The charge already succeeded at Square and the payment record + // is committed, so the customer's promised discount must not silently + // vanish. If the real money now covers the full booking obligation (the + // full price was charged), return the lost discount value to the customer + // as a gift-card account balance credit — the merchant honours the + // discount it quoted. If the booking is NOT fully covered (the customer + // was charged the discounted amount), no credit is due: the shortfall stays + // on the booking and the 400 below tells the frontend the campaign ended so + // it can prompt for the difference. In both cases the 400 + // (campaign_fully_redeemed) prevents the frontend from showing the discount + // as applied. + if campaignLostPence > 0 { + credited := refundLostCampaignAsBalanceCredit(r.Context(), bookingID, userID, campaignLostPence) + log.Printf("B13: campaign %s fully redeemed before payment %s applied it — lost discount %d pence (%s), returning 400 campaign_fully_redeemed to the frontend", campaignLostID, paymentID, campaignLostPence, credited) + mw.RespondJSON(w, http.StatusBadRequest, map[string]string{ + "error": "The discount campaign has been fully redeemed. The full amount applies.", + "code": "campaign_fully_redeemed", + }) + return + } + if err := json.NewEncoder(w).Encode(PaymentResponse{ ID: paymentID, BookingID: bookingID, @@ -2092,6 +2301,56 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { } } +// campaignExhaustedAtApplyError reports that a discount campaign the customer +// was shown as eligible at preview time was exhausted (times_redeemed reached +// max_redemptions) by the time the payment applied it (B13). lostPence is the +// discount the customer was promised but can no longer receive. +type campaignExhaustedAtApplyError struct { + campaignID string + lostPence int64 +} + +func (e *campaignExhaustedAtApplyError) Error() string { + return fmt.Sprintf("discount campaign %s was fully redeemed before the payment applied it (lost %d pence)", e.campaignID, e.lostPence) +} + +// refundLostCampaignAsBalanceCredit honours a discount the customer was +// promised but a concurrently-exhausted campaign could not apply (B13): when +// the booking's real-money ledger already covers the full obligation (the full +// price was charged at Square), the lost discount value is credited to the +// user's gift-card account balance so the merchant keeps the price it quoted. +// When the booking is NOT fully covered (the customer was charged the +// discounted amount), no credit is due — the shortfall stays on the booking. +// Returns a human-readable outcome for the caller's log line. +func refundLostCampaignAsBalanceCredit(ctx context.Context, bookingID, userID string, lostPence int64) string { + var totalPence, realPaidPence int64 + err := db.Conn.QueryRow(ctx, ` + SELECT COALESCE(ROUND((SELECT total_amount FROM bookings WHERE id = $1) * 100), 0), + COALESCE(ROUND((SELECT SUM(amount) FROM payments WHERE booking_id = $1 AND status = 'completed' + AND payment_type != 'tip' AND payment_method NOT IN ('discount', 'on_the_house')) * 100), 0) + `, bookingID).Scan(&totalPence, &realPaidPence) + if err != nil { + log.Printf("B13: failed to read booking ledger for campaign-loss credit on booking %s: %v", bookingID, err) + return "no credit (ledger unreadable)" + } + if realPaidPence < totalPence { + return "no credit (booking not fully paid by real money)" + } + creditPounds := float64(lostPence) / 100.0 + if _, err := db.Conn.Exec(ctx, ` + INSERT INTO user_giftcard_balances (user_id, balance, updated_at) + VALUES ($1, $2, NOW()) + ON CONFLICT (user_id) DO UPDATE SET + balance = user_giftcard_balances.balance + EXCLUDED.balance, + updated_at = NOW() + `, userID, creditPounds); err != nil { + log.Printf("CRITICAL: B13 campaign-loss credit of £%.2f to user %s (booking %s) failed: %v — MANUAL RECONCILIATION REQUIRED", creditPounds, userID, bookingID, err) + insertCriticalPaymentNotification(ctx, &bookingID, &userID) + return fmt.Sprintf("credit of £%.2f FAILED (manual reconciliation required)", creditPounds) + } + return fmt.Sprintf("credited £%.2f to gift-card account balance", creditPounds) +} + // applyEligibleCampaignsAtPayment checks and applies any eligible discount // campaigns to the booking. Uses the provided transaction so that discount // writes are atomic with the caller's payment transaction — if the payment @@ -2099,13 +2358,33 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // Skips if the booking already has 2+ completed non-discount payments — this // prevents applying new discounts after a customer has already paid, which // would create a credit balance or require a refund. -func applyEligibleCampaignsAtPayment(ctx context.Context, q db.Querier, bookingID string, userID string) { +// +// expected is the set of discounts the caller computed BEFORE the charge (under +// the same booking advisory lock). If any of those campaigns has since been +// exhausted by a CONCURRENT redemption on another booking (times_redeemed hit +// max_redemptions between the preview computation and the apply-time re-check — +// the max_redemptions race), the customer would be charged full price with no +// discount row and the booking would silently not complete. In that case a +// *campaignExhaustedAtApplyError is returned so the handler can surface a clear +// "campaign fully redeemed" error and return the promised discount value. +func applyEligibleCampaignsAtPayment(ctx context.Context, q db.Querier, bookingID, userID string, expected []EligibleDiscount) error { + for _, d := range expected { + if d.Source != "campaign" { + continue + } + var exhausted bool + err := q.QueryRow(ctx, `SELECT COALESCE(times_redeemed >= max_redemptions, FALSE) FROM discount_campaigns WHERE id = $1`, d.SourceID).Scan(&exhausted) + if err == nil && exhausted { + return &campaignExhaustedAtApplyError{campaignID: d.SourceID, lostPence: int64(math.Round(d.Amount * 100))} + } + } + var bookingTotal float64 if err := q.QueryRow(ctx, ` SELECT total_amount FROM bookings WHERE id = $1 `, bookingID).Scan(&bookingTotal); err != nil { log.Printf("Failed to calculate booking total for campaign check: %v", err) - return + return nil } for _, d := range ComputeEligibleDiscounts(ctx, q, bookingID, userID, bookingTotal) { @@ -2127,6 +2406,7 @@ func applyEligibleCampaignsAtPayment(ctx context.Context, q db.Querier, bookingI d.Amount = capped ApplyEligibleDiscount(ctx, q, bookingID, userID, bookingTotal, d) } + return nil } // buildSplitRecords determines whether to split a single Square charge into @@ -2421,6 +2701,10 @@ func DeletePaymentMethod(w http.ResponseWriter, r *http.Request) { type CreatePaymentMethodRequest struct { CardToken string `json:"card_token" validate:"required"` + // VerificationCode is the customer's current 2FA one-time code (B10): an + // enforced environment persists a card only when this matches the + // customer's pending code. + VerificationCode string `json:"verification_code,omitempty"` } func CreatePaymentMethod(w http.ResponseWriter, r *http.Request) { @@ -2454,7 +2738,7 @@ func CreatePaymentMethod(w http.ResponseWriter, r *http.Request) { // 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) { + if !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode) { return } @@ -3538,7 +3822,7 @@ 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) { + if req.SaveCard && !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode) { return } @@ -3639,7 +3923,7 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { // 2FA gating (C5): charging a SAVED card requires 2FA when the feature is // enforced. New-card (nonce) charges are not gated. if req.CardID != nil && *req.CardID != "" { - if !requireTwoFactorForCardAccess(w, r, service, userID) { + if !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode) { return } } diff --git a/backend/handlers/payments/loop_b_fixes_test.go b/backend/handlers/payments/loop_b_fixes_test.go new file mode 100644 index 0000000..301be85 --- /dev/null +++ b/backend/handlers/payments/loop_b_fixes_test.go @@ -0,0 +1,461 @@ +//go:build test && dev + +package payments + +import ( + "context" + "encoding/json" + "net/http" + "testing" + "time" + + "crussell/clock" + "crussell/db" + "crussell/internal/square" + "crussell/testutils" + "crussell/testutils/fixtures" + "crussell/testutils/jwt" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// ============================================================================= +// B3 — admin overcharge double-loss: server-side clamp of terminal charges to +// the booking's remaining obligation + tip only when explicitly requested. +// ============================================================================= + +// seedPriorPayment records a completed real payment on a booking so the +// remaining obligation is total - paid. +func seedPriorPayment(t *testing.T, ctx context.Context, q db.Querier, bookingID string, amountPounds float64) { + t.Helper() + _, err := q.Exec(ctx, ` + INSERT INTO payments (booking_id, payment_type, payment_method, status, amount, created_at, updated_at) + VALUES ($1, 'full', 'cash', 'completed', $2, NOW(), NOW()) + `, bookingID, amountPounds) + require.NoError(t, err) +} + +// TestTerminalCash_ClampsToRemainingObligation locks B3(a) for the cash branch: +// a PaymentModal sending £45 (subtotal - discounts - campaignPreview) on a +// booking with £30 already paid must be recorded at the £20 remaining +// obligation, never the verbatim £45 (which would overcharge the customer). +func TestTerminalCash_ClampsToRemainingObligation(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + _, bookingID, _ := setupTestData(t, ctx, tx) + seedPriorPayment(t, ctx, tx, bookingID, 30.00) + adminToken := jwt.GenerateAdminToken() + + handler := CreateTerminalPayment + req := CreateTerminalPaymentRequest{ + Amount: 4500, // £45 = subtotal - campaign preview, IGNORING the £30 already paid + PaymentType: "full", + PaymentMethod: strPtr("cash"), + } + w := makePaymentRequest(handler, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + + var paid float64 + require.NoError(t, tx.QueryRow(ctx, `SELECT COALESCE(SUM(amount), 0) FROM payments WHERE booking_id = $1 AND status = 'completed' AND payment_method NOT IN ('discount', 'on_the_house') AND payment_type != 'tip'`, bookingID).Scan(&paid)) + assert.InDelta(t, 50.00, paid, 0.001, "£30 prior + £20 clamped = £50 obligation, never £75") + + var resp CheckoutResponse + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp), "cash response must carry the payment id") + var clamped float64 + require.NoError(t, tx.QueryRow(ctx, `SELECT amount FROM payments WHERE id = $1`, resp.CheckoutID).Scan(&clamped)) + assert.InDelta(t, 20.00, clamped, 0.001, "the cash payment must be clamped to the £20 remaining obligation") +} + +// TestTerminalCash_FullyPaid_RecordsVerbatim locks the fully-paid edge of +// B3(a): when the booking has no remaining obligation, a cash payment is +// recorded VERBATIM (a deliberate admin overpayment is real money received and +// must stay on the ledger — the app's "overpayment handled at the counter" +// semantics). The clamp only protects the common B3 case where prior payments +// left a positive remaining obligation. +func TestTerminalCash_FullyPaid_RecordsVerbatim(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + _, bookingID, _ := setupTestData(t, ctx, tx) + seedPriorPayment(t, ctx, tx, bookingID, 50.00) + adminToken := jwt.GenerateAdminToken() + + handler := CreateTerminalPayment + req := CreateTerminalPaymentRequest{ + Amount: 4500, + PaymentType: "full", + PaymentMethod: strPtr("cash"), + } + w := makePaymentRequest(handler, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "a fully-paid booking must still record the admin's deliberate overpayment, body: %s", w.Body.String()) + + var resp CheckoutResponse + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp), "cash response must carry the payment id") + var lastCash float64 + require.NoError(t, tx.QueryRow(ctx, `SELECT amount FROM payments WHERE id = $1`, resp.CheckoutID).Scan(&lastCash)) + assert.InDelta(t, 45.00, lastCash, 0.001, "the fully-paid cash receipt must be recorded verbatim") +} + +// TestTerminalSavedCard_ClampsToRemainingObligation locks B3(a) for the +// saved-card branch of CreateTerminalPayment: the pending record and the +// Square charge use the clamped remaining obligation, not the verbatim +// PaymentModal amount that ignored prior payments. +func TestTerminalSavedCard_ClampsToRemainingObligation(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + seedPriorPayment(t, ctx, tx, bookingID, 30.00) + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:mock_card_123", "VISA", "4242") + require.NoError(t, err) + adminToken := jwt.GenerateAdminToken() + + origClient := SquareClient + SquareClient = square.NewDevClient() + defer func() { SquareClient = origClient }() + + handler := CreateTerminalPayment + req := CreateTerminalPaymentRequest{ + Amount: 4500, + PaymentType: "full", + PaymentMethod: strPtr("saved_card"), + UserSavedCardID: &cardID, + IdempotencyKey: "sc-b3-clamp-" + bookingID, + } + w := makePaymentRequest(handler, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + + var paid float64 + require.NoError(t, tx.QueryRow(ctx, `SELECT COALESCE(SUM(amount), 0) FROM payments WHERE booking_id = $1 AND status = 'completed' AND payment_method NOT IN ('discount', 'on_the_house') AND payment_type != 'tip'`, bookingID).Scan(&paid)) + assert.InDelta(t, 50.00, paid, 0.001, "£30 prior + £20 clamped = £50 obligation, never £75") + + var charged float64 + require.NoError(t, tx.QueryRow(ctx, `SELECT amount FROM payments WHERE booking_id = $1 AND payment_method = 'online_square' AND status = 'completed'`, bookingID).Scan(&charged)) + assert.InDelta(t, 20.00, charged, 0.001, "the saved-card charge must be clamped to the £20 remaining obligation") +} + +// TestTerminalCheckout_NoTip_ClampsToRemaining locks B3(b): a card-reader +// checkout for more than the remaining obligation is clamped down to the +// remaining value UNLESS the customer explicitly requested a tip. The +// recorded terminal_checkouts row amount must reflect the clamp. +func TestTerminalCheckout_NoTip_ClampsToRemaining(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + _, bookingID, _ := setupTestData(t, ctx, tx) + seedPriorPayment(t, ctx, tx, bookingID, 30.00) + adminToken := jwt.GenerateAdminToken() + + origClient := SquareClient + SquareClient = square.NewDevClient() + defer func() { SquareClient = origClient }() + + handler := CreateTerminalPayment + req := CreateTerminalPaymentRequest{ + Amount: 4500, // over the £20 remaining, no tip requested + PaymentType: "full", + TipEnabled: false, + IdempotencyKey: "chk-b3-clamp-" + bookingID, + } + w := makePaymentRequest(handler, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + + var checkoutAmount float64 + var tipEnabled bool + require.NoError(t, tx.QueryRow(ctx, `SELECT amount, tip_enabled FROM terminal_checkouts WHERE booking_id = $1 AND status = 'PENDING'`, bookingID).Scan(&checkoutAmount, &tipEnabled)) + assert.InDelta(t, 20.00, checkoutAmount, 0.001, "a no-tip checkout must present only the £20 remaining obligation") + assert.False(t, tipEnabled, "tip_enabled must be persisted as false") +} + +// TestTerminalCheckout_TipEnabled_NotClamped locks the tip side of B3(b): when +// the customer explicitly requested a tip, the checkout amount (booking +// portion + tip) is NOT clamped — the overflow is gratuity. +func TestTerminalCheckout_TipEnabled_NotClamped(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + _, bookingID, _ := setupTestData(t, ctx, tx) + seedPriorPayment(t, ctx, tx, bookingID, 30.00) + adminToken := jwt.GenerateAdminToken() + + origClient := SquareClient + SquareClient = square.NewDevClient() + defer func() { SquareClient = origClient }() + + handler := CreateTerminalPayment + req := CreateTerminalPaymentRequest{ + Amount: 4500, // £20 booking portion + £25 explicit tip + PaymentType: "full", + TipEnabled: true, + IdempotencyKey: "chk-b3-tip-" + bookingID, + } + w := makePaymentRequest(handler, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + + var checkoutAmount float64 + var tipEnabled bool + require.NoError(t, tx.QueryRow(ctx, `SELECT amount, tip_enabled FROM terminal_checkouts WHERE booking_id = $1 AND status = 'PENDING'`, bookingID).Scan(&checkoutAmount, &tipEnabled)) + assert.InDelta(t, 45.00, checkoutAmount, 0.001, "an explicit tip must not be clamped away") + assert.True(t, tipEnabled, "tip_enabled must be persisted as true") +} + +// ============================================================================= +// B14 — terminal saved-card charges must apply VAT (booking/cash paths do). +// ============================================================================= + +// TestTerminalSavedCard_AppliesVAT locks B14: a saved-card charge through the +// terminal handler must apply apply_vat_to_payment after the completed flip, +// exactly like the booking/cash paths — otherwise a VAT-registered business +// silently loses the VAT fields on every saved-card terminal charge. +func TestTerminalSavedCard_AppliesVAT(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + _, err := tx.Exec(ctx, `UPDATE business_settings SET is_vat_registered = TRUE, default_vat_rate = 20.00`) + require.NoError(t, err) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:mock_card_123", "VISA", "4242") + require.NoError(t, err) + adminToken := jwt.GenerateAdminToken() + + origClient := SquareClient + SquareClient = square.NewDevClient() + defer func() { SquareClient = origClient }() + + handler := CreateTerminalPayment + req := CreateTerminalPaymentRequest{ + Amount: 4500, + PaymentType: "full", + PaymentMethod: strPtr("saved_card"), + UserSavedCardID: &cardID, + IdempotencyKey: "sc-b14-vat-" + bookingID, + } + w := makePaymentRequest(handler, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + + var resp map[string]any + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + paymentID, ok := resp["payment_id"].(string) + require.True(t, ok, "response must carry payment_id") + + var isVATApplicable bool + var vatAmount, netAmount *float64 + require.NoError(t, tx.QueryRow(ctx, `SELECT is_vat_applicable, vat_amount, net_amount FROM payments WHERE id = $1`, paymentID).Scan(&isVATApplicable, &vatAmount, &netAmount)) + require.True(t, isVATApplicable, "a saved-card terminal charge must be VAT-applicable") + require.NotNil(t, vatAmount) + assert.InDelta(t, 7.50, *vatAmount, 0.001, "£45 at 20%% VAT = £7.50") + require.NotNil(t, netAmount) + assert.InDelta(t, 37.50, *netAmount, 0.001, "net = £37.50") +} + +// ============================================================================= +// B13 — max_redemptions race: the apply-time re-check must surface a +// campaign exhausted by a concurrent redemption instead of silently charging +// full price. +// ============================================================================= + +// TestApplyEligibleCampaignsAtPayment_CampaignExhausted_ReturnsError locks the +// B13 error path directly: a campaign that was eligible at preview time but +// exhausted (times_redeemed reached max_redemptions) by a concurrent +// redemption before the apply-time re-check must surface a +// campaignExhaustedAtApplyError with the promised discount value. +func TestApplyEligibleCampaignsAtPayment_CampaignExhausted_ReturnsError(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + var bookingTotal float64 + require.NoError(t, tx.QueryRow(ctx, `SELECT total_amount FROM bookings WHERE id = $1`, bookingID).Scan(&bookingTotal)) + + now := clock.Now() + var campaignID string + require.NoError(t, tx.QueryRow(ctx, ` + INSERT INTO discount_campaigns (name, campaign_type, discount_percent, status, start_date, end_date, max_redemptions, times_redeemed) + VALUES ($1, 'time_based', 10, 'active', $2, $3, 2, 0) + RETURNING id + `, "B13 Summer Sale", now.Add(-24*time.Hour), now.Add(24*time.Hour)).Scan(&campaignID)) + + // Preview-time computation: the campaign is eligible (£5 on a £50 booking). + expected := ComputeEligibleDiscounts(ctx, tx, bookingID, userID, bookingTotal) + require.Len(t, expected, 1, "the campaign must be eligible at preview time") + require.Equal(t, campaignID, expected[0].SourceID) + + // A CONCURRENT redemption on another booking exhausts the campaign. + _, err := tx.Exec(ctx, `UPDATE discount_campaigns SET times_redeemed = 2 WHERE id = $1`, campaignID) + require.NoError(t, err) + + err = applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID, expected) + require.Error(t, err, "an exhausted-at-apply campaign must surface an error, not silently charge full price") + var exErr *campaignExhaustedAtApplyError + require.ErrorAs(t, err, &exErr) + require.Equal(t, campaignID, exErr.campaignID) + require.Equal(t, int64(500), exErr.lostPence, "the lost discount is £5 on the £50 booking") + + // No discount row may have been created. + var discountCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM booking_discounts WHERE booking_id = $1`, bookingID).Scan(&discountCount)) + require.Zero(t, discountCount) +} + +// TestApplyEligibleCampaignsAtPayment_CampaignAvailable_NoError locks the B13 +// control: when the campaign is still available at apply time, the discount is +// applied and no error is returned. +func TestApplyEligibleCampaignsAtPayment_CampaignAvailable_NoError(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + var bookingTotal float64 + require.NoError(t, tx.QueryRow(ctx, `SELECT total_amount FROM bookings WHERE id = $1`, bookingID).Scan(&bookingTotal)) + + now := clock.Now() + var campaignID string + require.NoError(t, tx.QueryRow(ctx, ` + INSERT INTO discount_campaigns (name, campaign_type, discount_percent, status, start_date, end_date, max_redemptions, times_redeemed) + VALUES ($1, 'time_based', 10, 'active', $2, $3, 10, 0) + RETURNING id + `, "B13 Still Active", now.Add(-24*time.Hour), now.Add(24*time.Hour)).Scan(&campaignID)) + + expected := ComputeEligibleDiscounts(ctx, tx, bookingID, userID, bookingTotal) + require.Len(t, expected, 1) + + err := applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID, expected) + require.NoError(t, err, "an available campaign must apply cleanly") + + var discountCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM booking_discounts WHERE booking_id = $1`, bookingID).Scan(&discountCount)) + require.Equal(t, 1, discountCount, "the available campaign discount must be applied") +} + +// TestTerminalSavedCard_AppliesCampaignAtChargeTime locks the B13 fix for the +// saved-card terminal path: an eligible campaign must be applied AT CHARGE TIME +// (inside the completed-flip transaction), not deferred to the completion +// side-effects — those only run when bookingIsFullyPaid, by which point +// capDiscountToRemainingObligation sees zero headroom and the discount would be +// lost. Charging the discounted amount (£45 on a £50 booking) mints the £5 +// discount row so real money + discount == total and the booking completes. +func TestTerminalSavedCard_AppliesCampaignAtChargeTime(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:mock_card_123", "VISA", "4242") + require.NoError(t, err) + adminToken := jwt.GenerateAdminToken() + + now := clock.Now() + var campaignID string + require.NoError(t, tx.QueryRow(ctx, ` + INSERT INTO discount_campaigns (name, campaign_type, discount_percent, status, start_date, end_date, max_redemptions, times_redeemed) + VALUES ($1, 'time_based', 10, 'active', $2, $3, 10, 0) + RETURNING id + `, "Terminal Saved-Card Sale", now.Add(-24*time.Hour), now.Add(24*time.Hour)).Scan(&campaignID)) + + origClient := SquareClient + SquareClient = square.NewDevClient() + defer func() { SquareClient = origClient }() + + handler := CreateTerminalPayment + req := CreateTerminalPaymentRequest{ + Amount: 4500, // the discounted amount the frontend preview showed + PaymentType: "full", + PaymentMethod: strPtr("saved_card"), + UserSavedCardID: &cardID, + IdempotencyKey: "sc-b13-apply-" + bookingID, + } + w := makePaymentRequest(handler, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + + var discountCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM booking_discounts WHERE booking_id = $1`, bookingID).Scan(&discountCount)) + assert.Equal(t, 1, discountCount, "the eligible campaign must be applied at saved-card charge time") + var discountAmount float64 + require.NoError(t, tx.QueryRow(ctx, `SELECT discount_amount FROM booking_discounts WHERE booking_id = $1`, bookingID).Scan(&discountAmount)) + assert.InDelta(t, 5.00, discountAmount, 0.001, "the £50 booking at 10%% = £5 discount") + + var discountPay float64 + require.NoError(t, tx.QueryRow(ctx, `SELECT COALESCE(SUM(amount), 0) FROM payments WHERE booking_id = $1 AND payment_method = 'discount' AND status = 'completed'`, bookingID).Scan(&discountPay)) + assert.InDelta(t, 5.00, discountPay, 0.001, "the discount payment record must exist") + + var status string + require.NoError(t, tx.QueryRow(ctx, `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&status)) + assert.Equal(t, "completed", status, "real money + discount row must complete the booking") + + var redeemed int + require.NoError(t, tx.QueryRow(ctx, `SELECT times_redeemed FROM discount_campaigns WHERE id = $1`, campaignID).Scan(&redeemed)) + assert.Equal(t, 1, redeemed, "the campaign redemption counter must be incremented exactly once") +} + +// exhaustCampaignOnChargeClient simulates the B13 max_redemptions race: it +// exhausts the campaign (times_redeemed = max_redemptions) at the moment the +// Square charge is made — i.e. BETWEEN the pre-charge eligibility snapshot and +// the apply-time re-check inside the saved-card terminal path. +type exhaustCampaignOnChargeClient struct { + square.SquareClient + campaignID string +} + +func (c *exhaustCampaignOnChargeClient) CreatePayment(ctx context.Context, req square.CreatePaymentReq) (*square.PaymentResult, error) { + _, _ = db.Conn.Exec(ctx, `UPDATE discount_campaigns SET times_redeemed = max_redemptions WHERE id = $1`, c.campaignID) + return c.SquareClient.CreatePayment(ctx, req) +} + +// TestTerminalSavedCard_CampaignExhaustedAtApply_ReturnsCampaignFullyRedeemed +// locks the B13 saved-card terminal path: a campaign exhausted by a concurrent +// redemption between the frontend's preview and the apply-time re-check must +// surface the same campaign_fully_redeemed 400 the online booking path returns, +// instead of silently skipping the discount and leaving the booking underpaid. +// The charge still completes at Square and the payment is recorded; the +// frontend learns the campaign ended so it can prompt for the difference. +func TestTerminalSavedCard_CampaignExhaustedAtApply_ReturnsCampaignFullyRedeemed(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:mock_card_123", "VISA", "4242") + require.NoError(t, err) + adminToken := jwt.GenerateAdminToken() + + now := clock.Now() + var campaignID string + require.NoError(t, tx.QueryRow(ctx, ` + INSERT INTO discount_campaigns (name, campaign_type, discount_percent, status, start_date, end_date, max_redemptions, times_redeemed) + VALUES ($1, 'time_based', 10, 'active', $2, $3, 2, 0) + RETURNING id + `, "B13 Terminal Race", now.Add(-24*time.Hour), now.Add(24*time.Hour)).Scan(&campaignID)) + + origClient := SquareClient + SquareClient = &exhaustCampaignOnChargeClient{SquareClient: square.NewDevClient(), campaignID: campaignID} + defer func() { SquareClient = origClient }() + + handler := CreateTerminalPayment + req := CreateTerminalPaymentRequest{ + Amount: 4500, + PaymentType: "full", + PaymentMethod: strPtr("saved_card"), + UserSavedCardID: &cardID, + IdempotencyKey: "sc-b13-exhaust-" + bookingID, + } + w := makePaymentRequest(handler, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx) + require.Equal(t, http.StatusBadRequest, w.Code, "an exhausted-at-apply campaign must surface 400 campaign_fully_redeemed, body: %s", w.Body.String()) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + assert.Equal(t, "campaign_fully_redeemed", body["code"]) + + var discountCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM booking_discounts WHERE booking_id = $1`, bookingID).Scan(&discountCount)) + assert.Zero(t, discountCount, "an exhausted campaign must not mint a discount row") + + // The charge still succeeded at Square and the payment was recorded as + // completed (mirroring the online path: the payment is committed, then the + // 400 is returned so the frontend can prompt for the difference). + var payCount int + var payStatus string + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND payment_method = 'online_square'`, bookingID).Scan(&payCount)) + assert.Equal(t, 1, payCount) + require.NoError(t, tx.QueryRow(ctx, `SELECT status FROM payments WHERE booking_id = $1 AND payment_method = 'online_square' LIMIT 1`, bookingID).Scan(&payStatus)) + assert.Equal(t, "completed", payStatus) +} diff --git a/backend/handlers/payments/loyalty_test.go b/backend/handlers/payments/loyalty_test.go index 30d3da5..5a72895 100644 --- a/backend/handlers/payments/loyalty_test.go +++ b/backend/handlers/payments/loyalty_test.go @@ -261,7 +261,7 @@ func TestCampaignAutoApply_TimeBased(t *testing.T) { } // Call applyEligibleCampaignsAtPayment - applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID) + applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID, nil) // Verify booking_discounts was created var discountCount int @@ -313,7 +313,7 @@ func TestCampaignAutoApply_UserMilestone(t *testing.T) { t.Fatalf("failed to create payment: %v", err) } - applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID) + applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID, nil) var discountCount int tx.QueryRow(ctx, @@ -356,7 +356,7 @@ func TestCampaignAutoApply_GlobalMilestoneSkippedOnline(t *testing.T) { t.Fatalf("failed to create payment: %v", err) } - applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID) + applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID, nil) // Verify NO discount was applied (global milestone skipped for online payment) var discountCount int @@ -402,7 +402,7 @@ func TestCampaignAutoApply_GlobalMilestoneAppliedInPerson(t *testing.T) { tx.Exec(ctx, "UPDATE bookings SET status = 'confirmed', updated_at = NOW() WHERE id = $1", bookingID) - applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID) + applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID, nil) var discountCount int tx.QueryRow(ctx, @@ -447,7 +447,7 @@ func TestCampaignAutoApply_DoubleApplyGuard(t *testing.T) { VALUES ($1, 'deposit', 'online_square', 2500, 'completed', NOW(), NOW()) `, bookingID) - applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID) + applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID, nil) // Verify still only 1 discount var discountCount int @@ -497,7 +497,7 @@ func TestCampaignAutoApply_ReferralDiscount(t *testing.T) { t.Fatalf("failed to create payment: %v", err) } - applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID) + applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID, nil) // Verify referral discount was applied var discountCount int @@ -564,7 +564,7 @@ func TestCampaignAutoApply_ReferralDiscount_DoubleApplyGuard(t *testing.T) { t.Fatalf("failed to insert existing booking discount: %v", err) } - applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID) + applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID, nil) // Verify no second referral discount was applied var discountCount int @@ -609,7 +609,7 @@ func TestCampaignAutoApply_ReferralDiscount_AlreadyUsed(t *testing.T) { t.Fatalf("failed to insert used referral discount: %v", err) } - applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID) + applyEligibleCampaignsAtPayment(ctx, tx, bookingID, userID, nil) var discountCount int tx.QueryRow(ctx, diff --git a/backend/handlers/payments/m4_tip_refund_redesign_test.go b/backend/handlers/payments/m4_tip_refund_redesign_test.go index 01d6af1..1c4480a 100644 --- a/backend/handlers/payments/m4_tip_refund_redesign_test.go +++ b/backend/handlers/payments/m4_tip_refund_redesign_test.go @@ -462,7 +462,7 @@ func TestAdminRefundBooking_ExcludesTipsFromRefundable(t *testing.T) { assert.Equal(t, 0, refundCount, "tip money must not be refundable") } -func TestGetBookingRefundableAmountCents_ExcludesTips(t *testing.T) { +func TestGetBookingRefundableAmountPence_ExcludesTips(t *testing.T) { t.Parallel() ctx, tx := testutils.SetupTestTx(t) diff --git a/backend/handlers/payments/p14_payment_fixes_test.go b/backend/handlers/payments/p14_payment_fixes_test.go index 5475899..6700f28 100644 --- a/backend/handlers/payments/p14_payment_fixes_test.go +++ b/backend/handlers/payments/p14_payment_fixes_test.go @@ -377,7 +377,7 @@ func TestCreateTillSale_AmbiguousFailure_LeavesCardFundedPending(t *testing.T) { // Remaining balance excludes tips // --------------------------------------------------------------------------- -func TestGetBookingRemainingBalanceCents_ExcludesTips(t *testing.T) { +func TestGetBookingRemainingBalancePence_ExcludesTips(t *testing.T) { ctx, tx := testutils.SetupTestTx(t) userID, err := fixtures.CreateTestUser(tx) require.NoError(t, err) diff --git a/backend/handlers/payments/payments_round8_test.go b/backend/handlers/payments/payments_round8_test.go index bea0a4c..a5bb6de 100644 --- a/backend/handlers/payments/payments_round8_test.go +++ b/backend/handlers/payments/payments_round8_test.go @@ -135,6 +135,12 @@ func TestRound8_SweepUntrackedTerminal_OverBalance_TipSplit_VAT_CompletesBooking const tmpID = "tmp-round8-tip-split" seedStaleProvisionalTerminalCheckout(t, ctx, tx, bookingID, tmpID) + // B3: the £5 overflow is only carved into a tip record when the customer + // EXPLICITLY requested a tip (tip_enabled) — mark it so the split is + // exercised here. + if _, err := tx.Exec(ctx, `UPDATE terminal_checkouts SET tip_enabled = TRUE WHERE checkout_id = $1`, tmpID); err != nil { + t.Fatalf("failed to mark the checkout tip-enabled: %v", err) + } origClient := SquareClient const sqPayID = "sqp_round8_tip_split" diff --git a/backend/handlers/payments/payments_round9_test.go b/backend/handlers/payments/payments_round9_test.go index 6093769..59c9c9f 100644 --- a/backend/handlers/payments/payments_round9_test.go +++ b/backend/handlers/payments/payments_round9_test.go @@ -1167,14 +1167,17 @@ func TestRound9_CreateTerminalPayment_OverCap_Rejected_NoRow(t *testing.T) { assert.Equal(t, 0, payCount, "no payment row for an over-cap terminal payment") // Boundary: exactly £10,000 (1,000,000 pence) is ALLOWED — the cap is - // exclusive. + // exclusive. B3: on a £50 booking the recorded amount is clamped to the + // £50 remaining obligation (the frontend-sent amount that ignored prior + // payments must never be recorded verbatim), but the request itself is + // accepted. wb := round9TerminalPayment(t, ctx, tx.(pgx.Tx), bookingID, adminToken, map[string]interface{}{"amount": 1000000, "payment_type": "full", "payment_method": "cash"}) require.Equal(t, http.StatusOK, wb.Code, "boundary cash body: %s", wb.Body.String()) var boundaryCount int require.NoError(t, tx.QueryRow(ctx, - `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND amount = 10000.00`, bookingID).Scan(&boundaryCount)) - assert.Equal(t, 1, boundaryCount, "exactly £10,000 is inside the cap and must be recorded") + `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND amount = 50.00`, bookingID).Scan(&boundaryCount)) + assert.Equal(t, 1, boundaryCount, "the £10,000 boundary request must be accepted and clamped to the £50 remaining obligation") } // stringPtr is a small helper for optional string request fields. diff --git a/backend/handlers/payments/payments_test.go b/backend/handlers/payments/payments_test.go index 0d3ff40..762cd0a 100644 --- a/backend/handlers/payments/payments_test.go +++ b/backend/handlers/payments/payments_test.go @@ -3295,11 +3295,11 @@ func TestGetBookingRemainingBalancePence(t *testing.T) { } } -// TestGetBookingRemainingBalanceCents_RefundsReopenCapacity verifies the M-cap +// TestGetBookingRemainingBalancePence_RefundsReopenCapacity verifies the M-cap // is refund-aware: a completed refund returns money, so it re-opens booking // capacity — remaining = total - paid + refunded — while the cap never exceeds // the booking total. -func TestGetBookingRemainingBalanceCents_RefundsReopenCapacity(t *testing.T) { +func TestGetBookingRemainingBalancePence_RefundsReopenCapacity(t *testing.T) { t.Parallel() ctx, tx := testutils.SetupTestTx(t) @@ -3912,7 +3912,7 @@ func TestGetBookingPaymentInfo_Found(t *testing.T) { // Service layer: GetBookingRemainingBalancePence // ============================================================================= -func TestGetBookingRemainingBalanceCents_NotFound(t *testing.T) { +func TestGetBookingRemainingBalancePence_NotFound(t *testing.T) { ctx, tx := testutils.SetupTestTx(t) _ = tx svc := NewPaymentService() @@ -3922,16 +3922,16 @@ func TestGetBookingRemainingBalanceCents_NotFound(t *testing.T) { } } -func TestGetBookingRemainingBalanceCents_FullBalance(t *testing.T) { +func TestGetBookingRemainingBalancePence_FullBalance(t *testing.T) { ctx, tx := testutils.SetupTestTx(t) _, bookingID, _ := setupTestData(t, ctx, tx) svc := NewPaymentService() - cents, err := svc.GetBookingRemainingBalancePence(ctx, bookingID) + pence, err := svc.GetBookingRemainingBalancePence(ctx, bookingID) if err != nil { t.Errorf("unexpected error: %v", err) } - if cents <= 0 { - t.Errorf("expected positive remaining balance for unpaid booking, got %d", cents) + if pence <= 0 { + t.Errorf("expected positive remaining balance for unpaid booking, got %d", pence) } } diff --git a/backend/handlers/payments/refunds.go b/backend/handlers/payments/refunds.go index ce7e6c1..ac4bd17 100644 --- a/backend/handlers/payments/refunds.go +++ b/backend/handlers/payments/refunds.go @@ -1683,7 +1683,11 @@ func processManualPaymentGroup(ctx context.Context, paymentID string, rows []man case rcErr != nil: // Reconcile failed — unknown whether Square refunded. Leave // this row pending for the next sweep; NEVER mark failed on an - // unknown state. + // unknown state. B18: track consecutive reconcile failures and + // surface the hard-failing reconcile in the admin notification + // centre (mirrors resolveManualRefundAtCap) so the row does not + // oscillate here silently forever. + trackReconcileFailureReArm(ctx, []string{pr.ID}) log.Printf("Reconcile failed for aged manual refund %s (%v) — leaving pending for the next sweep", pr.ID, rcErr) case sqRefundID != nil: if _, upErr := db.Conn.Exec(ctx, ` diff --git a/backend/handlers/payments/sweep.go b/backend/handlers/payments/sweep.go index 5f59506..2e744e4 100644 --- a/backend/handlers/payments/sweep.go +++ b/backend/handlers/payments/sweep.go @@ -651,14 +651,20 @@ func clawbackTillSaleFunding(ctx context.Context, r staleRow) bool { // the row's creation and the sweep's 22h keyed cutoff (stalePendingKeyedAge), // so any COMPLETED payment created within [row.CreatedAt, row.CreatedAt + // replayLegitimateRetryWindow] can be that retry charge and must be rescued. -// A payment created LATER than 21h after the row (i.e. within ~1-3h of the -// sweep's own replay, which runs at row age 22h+) is the classic expired-key -// replay-induced charge — the sweep just created it by replaying the still -// valid saved-card source under a key Square no longer retains — and rescuing -// it would hide the duplicate charge behind the original row (finding A1). -// 21h is a clear margin: a legitimate retry cannot occur after the sweep has -// already picked the row up at the 22h cutoff. -const replayLegitimateRetryWindow = 21 * time.Hour +// +// B2: the window is exactly stalePendingKeyedAge (22h), never shorter. The +// sweep's own replay only runs once a row is at least 22h old, so a payment +// created within 22h of the row can ONLY be a legitimate same-key retry (a +// 21h window left a dead zone at 21-22h where a legitimate retry was refused +// and stranded the row pending). A payment created LATER than 22h after the +// row is the classic expired-key replay-induced charge — the sweep just +// created it by replaying the still valid saved-card source under a key Square +// no longer retains — and rescuing it would hide the duplicate charge behind +// the original row (finding A1). The boundary is inclusive: a payment created +// EXACTLY 22h after the row is still within the legitimate window (the sweep +// picks the row up at age >= 22h, so a retry landing at the very boundary was +// created no later than the moment the sweep could first have replayed). +const replayLegitimateRetryWindow = 22 * time.Hour // replayMatchesRowAmount reports whether the replayed payment charged the same // amount the pending row records — the amount the sweep's replay body repeats @@ -919,11 +925,37 @@ func reconcileStalePaymentByKey(ctx context.Context, table string, r staleRow) ( // ops so both charges can be reconciled at Square and the duplicate // refunded. if newCharge, created, createdOK := replayRevealsNewCharge(r, pr); newCharge { - lag := "unknown" - if createdOK { - lag = created.Sub(r.CreatedAt).Round(time.Minute).String() + // B1 caveat: auto-refund ONLY a PROVABLY-created-later duplicate. + // An UNPARSEABLE replayed CreatedAt does not prove the payment is a + // duplicate — it could be the ORIGINAL charge a retained key + // returned, whose created_at was lost/corrupt. Auto-refunding that + // would wrongly reverse a legitimately authorized charge, so the + // pre-B1 outcome is restored for this case: leave the row PENDING + // with a CRITICAL notification for a human to reconcile at Square + // (the refund is withheld until a human confirms the second charge + // exists — never auto-issued against a possibly-original payment). + if !createdOK { + return leavePendingCritical(ctx, r, "stale pending %s reconcile by key: the replayed COMPLETED payment %s has an UNPARSEABLE created_at — cannot prove it is a NEW expired-key replay rather than the ORIGINAL charge under a retained key — leaving row %s PENDING — MANUAL RECONCILIATION REQUIRED: verify at Square whether this is a second charge before refunding", table, pr.ID, r.ID) + } + lag := created.Sub(r.CreatedAt).Round(time.Minute).String() + // B1: the replayed COMPLETED payment is a REAL charge the customer + // never authorized (an expired-key replay landed it on the still + // valid saved-card source, provably created after the pending row). + // Auto-refund C2 at Square instead of leaving the row pending for a + // human: the amount is r.AmountPence + // (the replay repeats the row's amount), the key is fresh, and the + // reason marks the sweep. When the refund lands, the row is marked + // failed (the original charge was never found — the caller's + // definitively-failed branch does that; a till sale's gift-card + // funding is clawed back there too, since the duplicate has been + // refunded). Only a refund FAILURE keeps the CRITICAL + // manual-reconciliation path. + if refundErr := refundSweepDuplicateCharge(ctx, table, r, pr); refundErr == nil { + log.Printf("stale pending %s row %s: the replayed COMPLETED payment %s was a NEW charge under an expired idempotency key (lag %s) — auto-refunded the duplicate at Square and marking the row failed (the original charge was never found)", table, r.ID, pr.ID, lag) + return staleReconcileDefinitivelyFailed, "" + } else { + return leavePendingCritical(ctx, r, "stale pending %s reconcile by key: the replayed COMPLETED payment %s was created after the pending row %s (lag %s) — a NEW charge under an expired idempotency key; auto-refund FAILED (%v) — leaving the row PENDING — MANUAL RECONCILIATION REQUIRED: check Square for both charges and refund the duplicate", table, pr.ID, r.ID, lag, refundErr) } - return leavePendingCritical(ctx, r, "stale pending %s reconcile by key: the replayed COMPLETED payment %s was created after the pending row %s (lag %s) — a NEW charge under an expired idempotency key (likely a second charge against a still-valid saved card), NOT the original charge — leaving the row PENDING without rescue — MANUAL RECONCILIATION REQUIRED: check Square for both charges and refund the duplicate", table, pr.ID, r.ID, lag) } return staleReconcileCompleted, pr.ID case "CANCELED", "FAILED": @@ -938,6 +970,73 @@ func reconcileStalePaymentByKey(ctx context.Context, table string, r staleRow) ( } } +// refundSweepDuplicateCharge auto-refunds a replayed COMPLETED payment that the +// sweep proved to be a NEW charge under an expired idempotency key (B1) — money +// the customer never authorized. The refund reuses the existing Square refund +// path (SquareClient.RefundPayment) with: +// +// - amount r.AmountPence — the row's charge amount, which the replay body +// repeats, so the duplicate charged exactly this; +// - a deterministic idempotency key derived from the replayed payment's id, +// so a re-run refunding the SAME duplicate dedups at Square instead of +// issuing a second refund, while a different duplicate on a later replay +// gets a fresh key and is refunded too; +// - reason "duplicate charge — sweep replay" for the audit trail. +// +// On success a refunds row is recorded for payments-table rows (a till_sale +// row has no payments row to attach the refund to — the Square refund plus the +// deduped critical-payment admin notification cover the audit trail there) and +// an admin notification is inserted so an operator sees the auto-refund. The +// caller then marks the row definitively failed (the ORIGINAL charge was never +// found). Any failure leaves the money state at Square untouched and returns +// the error so the caller keeps the CRITICAL manual-reconciliation path. +func refundSweepDuplicateCharge(ctx context.Context, table string, r staleRow, pr *square.PaymentResult) error { + if pr == nil || pr.ID == "" { + return errors.New("replayed payment has no Square payment id to refund") + } + if r.AmountPence <= 0 { + return fmt.Errorf("refusing to auto-refund a non-positive amount %d pence for row %s", r.AmountPence, r.ID) + } + // Deterministic per-duplicate key: Square dedups same-key refunds, so a + // re-run replaying the same C2 never double-refunds. "sweepdup-" + Square + // payment id stays well under Square's 45-char idempotency-key limit. + refundKey := "sweepdup-" + pr.ID + if len(refundKey) > maxIdempotencyKeyLength { + refundKey = truncateIdempotencyKey("sweepdup", pr.ID) + } + reason := "duplicate charge — sweep replay" + res, refundErr := SquareClient.RefundPayment(ctx, square.RefundPaymentReq{ + PaymentID: pr.ID, + Amount: r.AmountPence, + IdempotencyKey: refundKey, + Reason: reason, + }) + if refundErr != nil { + return fmt.Errorf("auto-refund of replay-induced duplicate charge %s failed: %w", pr.ID, refundErr) + } + + status := "completed" + if res.Status == "PENDING" { + status = "pending" + } else if res.Status == "FAILED" || res.Status == "REJECTED" { + // Square definitively rejected the refund — the duplicate charge stands. + return fmt.Errorf("auto-refund of replay-induced duplicate charge %s was %s at Square", pr.ID, res.Status) + } + if table == "payments" { + amountPounds := float64(r.AmountPence) / 100.0 + if _, insErr := db.Conn.Exec(ctx, ` + INSERT INTO refunds (payment_id, booking_id, amount, square_refund_id, status, origin, reason, idempotency_key, created_by, created_at) + VALUES ($1, $2, $3, $4, $5, 'manual', $6, $7, $8, NOW()) + `, r.ID, r.BookingID, amountPounds, res.ID, status, reason, refundKey, r.CreatedBy); insErr != nil { + log.Printf("CRITICAL: auto-refunded duplicate charge %s at Square (refund %s) but recording the refunds row for pending %s failed: %v — MANUAL RECONCILIATION REQUIRED", pr.ID, res.ID, r.ID, insErr) + } + } + + insertCriticalPaymentNotification(ctx, r.BookingID, r.CreatedBy) + log.Printf("Auto-refunded replay-induced duplicate charge %s (%d pence) for pending row %s — refund %s", pr.ID, r.AmountPence, r.ID, res.ID) + return nil +} + // leaveGiftCardPurchasePending keeps a gift-card-purchase payment row (payments // table, booking_id NULL) pending after Square confirms the charge COMPLETED, // instead of rescuing it to 'completed'. Completing the row would permanently @@ -1616,11 +1715,16 @@ func recordTerminalPaymentTx(ctx context.Context, tx pgx.Tx, checkoutID, booking } // The payment type the admin charged is recorded on the checkout row by - // CreateTerminalPayment; fall back to 'full' for legacy rows. + // CreateTerminalPayment; fall back to 'full' for legacy rows. tip_enabled + // records whether the customer EXPLICITLY requested a tip (the frontend's + // tip_enabled flag): an overflow beyond the remaining booking value must + // only be recorded as a tip record when the customer asked for one (B3) — + // an accidental overpayment must never be relabelled gratuity. var checkoutPaymentType string + var checkoutTipEnabled bool if err := tx.QueryRow(ctx, ` - SELECT payment_type FROM terminal_checkouts WHERE checkout_id = $1 - `, checkoutID).Scan(&checkoutPaymentType); err != nil { + SELECT payment_type, tip_enabled FROM terminal_checkouts WHERE checkout_id = $1 + `, checkoutID).Scan(&checkoutPaymentType, &checkoutTipEnabled); err != nil { if !errors.Is(err, pgx.ErrNoRows) { slog.Error("Failed to read payment type for checkout", "checkout_id", checkoutID, "err", err) } @@ -1643,6 +1747,12 @@ func recordTerminalPaymentTx(ctx context.Context, tx pgx.Tx, checkoutID, booking // M4 split: a terminal charge above the remaining booking value is a tip — // record it as its own record so only the booking portion is refundable. + // B3: only when the customer EXPLICITLY requested a tip (checkout + // tip_enabled). Without an explicit tip request the overflow is an + // accidental overpayment, not gratuity: it stays on the booking record at + // the full charged amount (the excess is refundable, never a tip). The + // checkout-creation side clamps the amount to the remaining value when no + // tip is requested, so this branch only fires for legacy/edge checkouts. var records []PaymentRecord bookingInfo, bErr := service.GetBookingPaymentInfo(ctx, bookingID) if bErr == nil && bookingInfo != nil { @@ -1651,7 +1761,7 @@ func recordTerminalPaymentTx(ctx context.Context, tx pgx.Tx, checkoutID, booking bookingPortion := math.Min(charged, remainingBookingValue) bookingPortion = math.Round(bookingPortion*100) / 100 tipAmount := math.Round((charged-bookingPortion)*100) / 100 - if tipAmount > 0.004 { + if checkoutTipEnabled && tipAmount > 0.004 { records = buildTerminalSplitRecords(record, bookingInfo, bookingPortion, tipAmount) } } @@ -1675,6 +1785,34 @@ func recordTerminalPaymentTx(ctx context.Context, tx pgx.Tx, checkoutID, booking ApplyVATToBookingPayment(ctx, tx, pid) } + // B13: apply eligible campaign discounts at record time, inside the same + // transaction as the payment insert. The card-machine checkout completes + // ASYNCHRONOUSLY (the GetCheckoutStatus poll or the stale-checkout sweep), + // so there is no HTTP response to surface an exhausted-at-apply campaign + // the way the synchronous saved-card/booking paths do — a campaign that + // exhausts between the frontend's preview and this record stays a silent + // skip (the customer was already charged the discounted amount the frontend + // showed; the merchant absorbs the shortfall — documented limitation of the + // async flow). An AVAILABLE campaign MUST be applied here rather than + // deferred to the completion side-effects: completeFullyPaidBooking only + // runs ApplyBookingCompletionSideEffects when bookingIsFullyPaid (real + // money covers the total), at which point capDiscountToRemainingObligation + // sees zero headroom — so a discounted-amount terminal charge would never + // get its discount row and never complete. Applying here (with the same + // capDiscountToRemainingObligation guard as every other path) mints the + // discount row so bookingIsFullyPaid sees it; the completion side-effects + // skip re-application via their already-recorded guards. The apply runs + // AFTER the payment record insert so the headroom counts the charge as real + // money (never over-credits), and after the M4 tip split so tip records + // never affect the discount computation. + var bookingUserID string + if err := tx.QueryRow(ctx, `SELECT COALESCE(user_id, '') FROM bookings WHERE id = $1`, bookingID).Scan(&bookingUserID); err != nil { + slog.Error("Failed to load booking user for terminal campaign apply", "booking_id", bookingID, "err", err) + } + if applyErr := applyEligibleCampaignsAtPayment(ctx, tx, bookingID, bookingUserID, nil); applyErr != nil { + slog.Error("Failed to apply eligible campaigns for terminal checkout", "booking_id", bookingID, "err", applyErr) + } + // Release the in-flight guard: this checkout is now recorded. if _, err := tx.Exec(ctx, ` UPDATE terminal_checkouts SET status = 'COMPLETED', updated_at = NOW() diff --git a/backend/handlers/payments/sweep_test.go b/backend/handlers/payments/sweep_test.go index a6b8255..32d6d90 100644 --- a/backend/handlers/payments/sweep_test.go +++ b/backend/handlers/payments/sweep_test.go @@ -708,16 +708,18 @@ func TestSweepStalePendingPayments_KeyedSourceMismatch_LeavesPending(t *testing. } } -// TestSweepStalePendingPayments_KeyedReplayNewCharge_LeavesPending locks the A1 -// money-safety cross-check: a replayed COMPLETED payment created long AFTER the -// pending row is a NEW charge Square made with an EXPIRED idempotency key -// against the still-valid ccof source (the ~24h key retention is unverified), -// NOT the original charge a retained key returns. Rescuing the row with the new -// payment id would hide the second charge behind the original — the row must -// stay PENDING, a CRITICAL notification must be raised, and no square_payment_id -// may be written. The cross-check runs only in a non-dev/mock env, so this test -// flips SQUARE_ENVIRONMENT to production (sequential, like the 2FA tests). -func TestSweepStalePendingPayments_KeyedReplayNewCharge_LeavesPending(t *testing.T) { +// TestSweepStalePendingPayments_KeyedReplayNewCharge_AutoRefunded locks the B1 +// money-safety behaviour for the A1 cross-check: a replayed COMPLETED payment +// created long AFTER the pending row is a NEW charge Square made with an +// EXPIRED idempotency key against the still-valid ccof source (the ~24h key +// retention is unverified) — money the customer never authorized. Instead of +// only leaving the row pending for a human (the pre-B1 outcome), the sweep now +// AUTO-REFUNDS the duplicate at Square with a fresh key, records the refunds +// row, raises the critical notification and marks the row FAILED (the original +// charge was never found). The cross-check runs only in a non-dev/mock env, so +// this test flips SQUARE_ENVIRONMENT to production (sequential, like the 2FA +// tests). +func TestSweepStalePendingPayments_KeyedReplayNewCharge_AutoRefunded(t *testing.T) { ctx, tx := testutils.SetupTestTx(t) userID, err := fixtures.CreateTestUser(tx) @@ -744,6 +746,7 @@ func TestSweepStalePendingPayments_KeyedReplayNewCharge_LeavesPending(t *testing // production snapshot-decryption gate. created_by carries the payer so the // admin notification is attributable and assertable. const key = "key-expired-replay-new-charge" + const dupPayID = "pay_expired_key_new_charge" if _, err := tx.Exec(ctx, "UPDATE payments SET created_at = NOW() - INTERVAL '23 hours', idempotency_key = $1, square_source_id = 'ccof:test-saved-card', created_by = $2 WHERE id = $3", key, userID, staleID); err != nil { t.Fatalf("failed to age the stale payment: %v", err) } @@ -757,12 +760,13 @@ func TestSweepStalePendingPayments_KeyedReplayNewCharge_LeavesPending(t *testing origClient := SquareClient mock := square.NewDevClient() t.Setenv("SQUARE_ENVIRONMENT", "production") - SquareClient = &staleReplayClient{SquareClient: mock, result: &square.PaymentResult{ + counting := &countingRefundClient{SquareClient: &staleReplayClient{SquareClient: mock, result: &square.PaymentResult{ Status: "COMPLETED", - ID: "pay_expired_key_new_charge", - SquarePayID: "pay_expired_key_new_charge", + ID: dupPayID, + SquarePayID: dupPayID, CreatedAt: clock.Now().Format(time.RFC3339), - }} + }}} + SquareClient = counting defer func() { SquareClient = origClient }() pgxTx := db.TxFromContext(ctx) @@ -774,6 +778,7 @@ func TestSweepStalePendingPayments_KeyedReplayNewCharge_LeavesPending(t *testing } 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) @@ -791,18 +796,142 @@ func TestSweepStalePendingPayments_KeyedReplayNewCharge_LeavesPending(t *testing if err := db.Conn.QueryRow(freshCtx, "SELECT status, square_payment_id FROM payments WHERE id = $1", staleID).Scan(&status, &sqPayID); err != nil { t.Fatalf("failed to query payment: %v", err) } - if status != "pending" { - t.Errorf("expected the new-charge replay to leave the row pending (never rescue with the second charge's id), got %q", status) + if status != "failed" { + t.Errorf("expected the auto-refunded new-charge replay to mark the row failed, got %q", status) } if sqPayID != nil { t.Errorf("expected NO square_payment_id written on a new-charge replay, got %q", *sqPayID) } + + // Exactly one auto-refund must have been issued for the duplicate charge, + // for the row's amount, with the sweep reason. + calls := counting.refundCalls() + if len(calls) != 1 { + t.Fatalf("expected exactly one auto-refund of the duplicate charge, got %d", len(calls)) + } + if calls[0].PaymentID != dupPayID { + t.Errorf("expected the refund to target the duplicate charge %s, got %s", dupPayID, calls[0].PaymentID) + } + if calls[0].Amount != 200000 { + t.Errorf("expected the refund amount to be the row's 200000 pence, got %d", calls[0].Amount) + } + if calls[0].Reason != "duplicate charge — sweep replay" { + t.Errorf("expected the sweep duplicate reason, got %q", calls[0].Reason) + } + + // The refunds row records the auto-refund against the pending payment row. + var refundCount int + if err := db.Conn.QueryRow(freshCtx, `SELECT COUNT(*) FROM refunds WHERE payment_id = $1 AND status = 'completed'`, staleID).Scan(&refundCount); err != nil { + t.Fatalf("failed to count refunds: %v", err) + } + if refundCount != 1 { + t.Errorf("expected one completed refunds row for the auto-refund, got %d", refundCount) + } + var notifCount int if err := db.Conn.QueryRow(freshCtx, `SELECT COUNT(*) FROM admin_notifications WHERE reason = 'critical_payment_log' AND user_id = $1`, userID).Scan(¬ifCount); err != nil { t.Fatalf("failed to count admin notifications: %v", err) } if notifCount < 1 { - t.Errorf("expected a critical-payment admin notification for the suspected second charge, got %d", notifCount) + t.Errorf("expected a critical-payment admin notification recording the auto-refund, got %d", notifCount) + } +} + +// TestSweepStalePendingPayments_KeyedReplayUnparseableCreatedAt_LeavesPending +// locks the B1 caveat: a replayed COMPLETED payment whose created_at CANNOT be +// parsed must NOT be auto-refunded. An unparseable created_at does not prove +// the payment is a duplicate — it could be the ORIGINAL charge a retained key +// returned, whose created_at was lost/corrupt — so the sweep restores the +// pre-B1 outcome: leave the row PENDING with a CRITICAL notification and issue +// NO refund (never auto-reverse a possibly-legitimate authorized charge). +// Sequential (flips SQUARE_ENVIRONMENT), like the sibling B1/A1 tests. +func TestSweepStalePendingPayments_KeyedReplayUnparseableCreatedAt_LeavesPending(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + if err != nil { + t.Fatalf("failed to create booking: %v", err) + } + + staleID, err := fixtures.CreateTestPayment(tx, bookingID, 2000.00, "online_square", "full", "pending") + if err != nil { + t.Fatalf("failed to create stale pending payment: %v", err) + } + const key = "key-expired-replay-unparseable-createdat" + 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) + } + + origClient := SquareClient + mock := square.NewDevClient() + t.Setenv("SQUARE_ENVIRONMENT", "production") + counting := &countingRefundClient{SquareClient: &staleReplayClient{SquareClient: mock, result: &square.PaymentResult{ + Status: "COMPLETED", + ID: "pay_expired_key_unparseable_createdat", + SquarePayID: "pay_expired_key_unparseable_createdat", + CreatedAt: "not-a-real-timestamp", + }}} + SquareClient = counting + 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) + } + + var status string + if err := db.Conn.QueryRow(freshCtx, "SELECT status FROM payments WHERE id = $1", staleID).Scan(&status); err != nil { + t.Fatalf("failed to query payment: %v", err) + } + if status != "pending" { + t.Errorf("expected an unparseable-created_at replay to leave the row pending (pre-B1), got %q", status) + } + + // NO auto-refund may have been issued — the payment is not provably a duplicate. + if calls := counting.refundCalls(); len(calls) != 0 { + t.Errorf("expected NO auto-refund for an unparseable-created_at replay, got %d refund call(s)", len(calls)) + } + 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 refunds row for an unparseable-created_at replay, got %d", refundCount) + } + + // The leave-pending-CRITICAL path still raises the admin notification. + var notifCount int + if err := db.Conn.QueryRow(freshCtx, `SELECT COUNT(*) FROM admin_notifications WHERE reason = 'critical_payment_log' AND user_id = $1`, userID).Scan(¬ifCount); err != nil { + t.Fatalf("failed to count admin notifications: %v", err) + } + if notifCount < 1 { + t.Errorf("expected a critical-payment admin notification for the unparseable-created_at replay, got %d", notifCount) } } @@ -892,6 +1021,91 @@ func TestSweepStalePendingPayments_KeyedReplayOriginalPayment_Rescues(t *testing } } +// 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 +// under a legitimately replayed key and MUST be rescued. The pre-B2 21h window +// refused it and stranded the row pending. 22h is the boundary: only a payment +// created AFTER row.CreatedAt+22h can be the sweep's own expired-key replay. +func TestSweepStalePendingPayments_KeyedReplayRetryAt215h_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-retry-215h" + 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 a legitimate same-key retry created + // 21.5h after the row — inside the 22h legitimate window, so it is the real + // charge and must be rescued, not refused as an expired-key duplicate. + var rowCreatedAt time.Time + if err := tx.QueryRow(ctx, "SELECT created_at FROM payments WHERE id = $1", staleID).Scan(&rowCreatedAt); err != nil { + t.Fatalf("failed to read aged payment created_at: %v", err) + } + retryCreated := rowCreatedAt.Add(21*time.Hour + 30*time.Minute) + + origClient := SquareClient + mock := square.NewDevClient() + t.Setenv("SQUARE_ENVIRONMENT", "production") + SquareClient = &staleReplayClient{SquareClient: mock, result: &square.PaymentResult{ + Status: "COMPLETED", + ID: "pay_retry_at_215h", + SquarePayID: "pay_retry_at_215h", + CreatedAt: retryCreated.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 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) + } + + 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 the 21.5h same-key retry rescued to 'completed', got %q", status) + } + if sqPayID != "pay_retry_at_215h" { + t.Errorf("expected square_payment_id %s written back on the rescue, got %q", "pay_retry_at_215h", sqPayID) + } +} + // TestSweepStalePendingPayments_KeyedCCOFRejected_LeavesPendingNoClawback locks // the A1 ccof-blind-fail rule: a replay rejection (ErrReplayKeyNotRetained) // against a SAVED-CARD (ccof:) source is NOT proof the original charge never diff --git a/backend/handlers/payments/till.go b/backend/handlers/payments/till.go index 49065f5..4302734 100644 --- a/backend/handlers/payments/till.go +++ b/backend/handlers/payments/till.go @@ -43,6 +43,10 @@ type TillSaleRequest struct { CardToken string `json:"card_token,omitempty"` RedeemToUserID *string `json:"redeem_to_user_id,omitempty"` VerificationToken *string `json:"verification_token,omitempty"` + // VerificationCode is the card owner's current 2FA one-time code (B10): an + // enforced environment charges a saved card only when this matches the + // customer's pending code. + VerificationCode string `json:"verification_code,omitempty"` } type TillSaleResponse struct { @@ -944,7 +948,7 @@ 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) { + if cardUserID.Valid && !requireTwoFactorForCardAccess(w, r, service, cardUserID.String, req.VerificationCode) { return } diff --git a/backend/handlers/payments/twofa.go b/backend/handlers/payments/twofa.go index 7363e4b..514e328 100644 --- a/backend/handlers/payments/twofa.go +++ b/backend/handlers/payments/twofa.go @@ -9,6 +9,7 @@ import ( "strings" "crussell/db" + "crussell/internal/twofa" "crussell/mw" "github.com/jackc/pgx/v5" @@ -60,17 +61,52 @@ func (s *PaymentService) UserTwoFactorEnabled(ctx context.Context, userID string return enabled, nil } +// Single source of truth for 2FA verification: crussell/internal/twofa owns +// the code hashing (HMAC-SHA256 keyed by TWO_FACTOR_PEPPER, legacy SHA-256 +// fallback), the constant-time compare, the code-lifetime check, and the +// per-user brute-force lockout. The user package's interactive endpoints +// (setup/verify/disable) and this saved-card gate all share it; nothing is +// re-implemented locally here. Two agents once shipped a drift-risk duplicate +// of the hash+verify in this file (hashTwoFAVerificationCode + +// verifyPendingTwoFactorCode) — that copy is gone, and any future change to +// the hashing or lockout rules must land in internal/twofa only. + +// verifyPendingTwoFactorCode verifies the submitted code against the user's +// 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). 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) +} + // requireTwoFactorForCardAccess gates the saved-card online payment paths // (PSD2 SCA stand-in until real SCA infra lands). It returns true when the // request may proceed: // // - 2FA is not enforced (dev/mock), OR -// - the user has completed 2FA setup (two_factor_enabled). +// - the user has completed 2FA setup (two_factor_enabled) AND the request +// carries a verification_code matching the user's stored pending code. // -// When 2FA is enforced and the user has not enabled it, a 403 JSON error is -// written (parseable by the frontend via extractErrorMessage) and false is -// returned — the caller must abort the charge. -func requireTwoFactorForCardAccess(w http.ResponseWriter, r *http.Request, service *PaymentService, userID string) bool { +// B10: the setup flag alone must NOT unlock saved-card charges — an enforced +// environment requires an actual one-time code challenge at charge time, so +// merely enabling 2FA (a setup flag) can never unlock saved-card access with +// no challenge. The code is the customer's current pending 2FA code, which an +// operator relays (delivery is the user package's build-dependent [2FA] log / +// email-SMS channel). +// +// 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 +// user package's setup/verify/disable flows. Classified errors map to the HTTP +// statuses the frontend expects: incorrect → 400, locked out → 429, missing or +// expired → 400, DB failure → 500. +// +// 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 { if !twoFactorEnforced() { return true } @@ -87,9 +123,31 @@ func requireTwoFactorForCardAccess(w http.ResponseWriter, r *http.Request, servi mw.RespondError(w, http.StatusInternalServerError, "failed to check two-factor status") return false } - if enabled { + if !enabled { + mw.RespondError(w, http.StatusForbidden, "Two-factor authentication is required to use online card payments. Enable it in your account settings.") + return false + } + // B10: an enforced charge of a saved card needs a live one-time code, not + // just the enabled setup flag. + if verificationCode == "" { + 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); { + case err == nil: return true + case errors.Is(err, twofa.ErrIncorrect): + mw.RespondError(w, http.StatusBadRequest, "Invalid verification code") + return false + case errors.Is(err, twofa.ErrLockedOut): + mw.RespondError(w, http.StatusTooManyRequests, "Too many attempts") + return false + case errors.Is(err, twofa.ErrMissingOrExpired): + mw.RespondError(w, http.StatusBadRequest, "Verification code expired — request a new one") + return false + default: + log.Printf("failed to check two-factor verification code for user %s: %v", userID, err) + mw.RespondError(w, http.StatusInternalServerError, "failed to check two-factor verification code") + return false } - mw.RespondError(w, http.StatusForbidden, "Two-factor authentication is required to use online card payments. Enable it in your account settings.") - return false } diff --git a/backend/handlers/payments/twofa_test.go b/backend/handlers/payments/twofa_test.go index e1a9b84..d125772 100644 --- a/backend/handlers/payments/twofa_test.go +++ b/backend/handlers/payments/twofa_test.go @@ -12,12 +12,14 @@ package payments import ( "bytes" + "context" "encoding/json" "net/http" "net/http/httptest" "testing" "crussell/db" + "crussell/internal/twofa" "crussell/mw" "crussell/testutils" "crussell/testutils/fixtures" @@ -34,6 +36,21 @@ func helperEnvEnforce2FA(t *testing.T) { t.Setenv("SQUARE_ENVIRONMENT", "production") } +// seedTwoFAPendingCode stores a pending 2FA code hash + expiry for a user, the +// state the user package's deliverTwoFACode writes. The hash uses the shared +// twofa.Hash — the same single source of truth the gate verifies with. +func seedTwoFAPendingCode(t *testing.T, q db.Querier, userID, code string) { + t.Helper() + _, err := q.Exec(context.Background(), ` + UPDATE users SET + two_factor_enabled = true, + two_factor_pending_code_hash = $2, + two_factor_pending_code_expires = NOW() + INTERVAL '10 minutes' + WHERE id = $1 + `, userID, twofa.Hash(code)) + require.NoError(t, err) +} + func TestTwoFactorEnforced(t *testing.T) { tests := []struct { name string @@ -90,7 +107,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", "")) require.Equal(t, http.StatusOK, w.Code, "no response must be written when not enforced") } @@ -103,7 +120,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) + ok := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "123456") require.False(t, ok) require.Equal(t, http.StatusForbidden, w.Code) var body map[string]string @@ -111,21 +128,47 @@ func TestRequireTwoFactorForCardAccess_Enforced(t *testing.T) { require.NotEmpty(t, body["error"]) }) - t.Run("user_enabled_allows", func(t *testing.T) { + t.Run("user_enabled_but_no_code_writes_403_json", func(t *testing.T) { userID, err := fixtures.CreateTestUser(tx) require.NoError(t, err) _, err = tx.Exec(ctx, "UPDATE users SET two_factor_enabled = true WHERE id = $1", userID) require.NoError(t, err) req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() - require.True(t, requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID)) + // B10: the enabled setup flag alone must NOT unlock the gate. + ok := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "") + require.False(t, ok) + require.Equal(t, http.StatusForbidden, w.Code) + }) + + t.Run("user_enabled_with_valid_code_allows", func(t *testing.T) { + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + 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.Equal(t, http.StatusOK, w.Code) }) + t.Run("user_enabled_with_wrong_code_writes_400_json", func(t *testing.T) { + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + seedTwoFAPendingCode(t, tx, userID, "424242") + req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) + w := httptest.NewRecorder() + ok := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "000000") + require.False(t, ok) + require.Equal(t, http.StatusBadRequest, w.Code) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "Invalid verification code", body["error"]) + }) + t.Run("unknown_user_writes_403_json", func(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() - require.False(t, requireTwoFactorForCardAccess(w, req, NewPaymentService(), "000000000000")) + require.False(t, requireTwoFactorForCardAccess(w, req, NewPaymentService(), "000000000000", "123456")) 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") @@ -201,16 +244,16 @@ func TestTwoFactorEnforced_CreateBookingPayment_SaveCard_With2FA_Succeeds(t *tes userID, bookingID, _ := setupTestData(t, ctx, tx) userToken := jwt.GenerateUserToken(userID) - _, err := tx.Exec(ctx, "UPDATE users SET two_factor_enabled = true WHERE id = $1", userID) - require.NoError(t, err) + seedTwoFAPendingCode(t, tx, userID, "112233") cardToken := "cnon:test-card-nonce" req := CreateBookingPaymentRequest{ - Amount: 2500, - PaymentType: "deposit", - NewCardToken: &cardToken, - SaveCard: true, - IdempotencyKey: "2fa-save-card-ok", + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + SaveCard: true, + IdempotencyKey: "2fa-save-card-ok", + VerificationCode: "112233", } w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) @@ -227,16 +270,16 @@ func TestTwoFactorEnforced_CreateBookingPayment_SavedCard_With2FA_Succeeds(t *te userID, bookingID, _ := setupTestData(t, ctx, tx) userToken := jwt.GenerateUserToken(userID) - _, err := tx.Exec(ctx, "UPDATE users SET two_factor_enabled = true WHERE id = $1", userID) - require.NoError(t, err) + seedTwoFAPendingCode(t, tx, userID, "334455") cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:mock_card_123", "VISA", "4242") require.NoError(t, err) req := CreateBookingPaymentRequest{ - Amount: 5000, - PaymentType: "full", - CardID: &cardID, - IdempotencyKey: "2fa-saved-card-ok", + Amount: 5000, + PaymentType: "full", + CardID: &cardID, + IdempotencyKey: "2fa-saved-card-ok", + VerificationCode: "334455", } w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) @@ -315,27 +358,27 @@ func TestTwoFactorEnforced_CreateTillSale_SavedCard_Blocked(t *testing.T) { func TestTwoFactorEnforced_CreateTillSale_SavedCard_With2FA_Succeeds(t *testing.T) { helperEnvEnforce2FA(t) - ctx, tx := testutils.SetupTestTx(t) + _, tx := testutils.SetupTestTx(t) adminID, err := fixtures.CreateTestAdminUser(tx) require.NoError(t, err) userID, err := fixtures.CreateTestUser(tx) require.NoError(t, err) adminToken := jwt.GenerateTestToken(adminID, "admin") - _, err = tx.Exec(ctx, "UPDATE users SET two_factor_enabled = true WHERE id = $1", userID) - require.NoError(t, err) + seedTwoFAPendingCode(t, tx, userID, "556677") cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sq_test_card_id", "VISA", "1234") require.NoError(t, err) reqBody := TillSaleRequest{ - ItemType: "gift_card", - Action: "create", - Amount: 50.00, - PaymentMethod: "saved_card", - UserSavedCardID: &cardID, - UserID: &userID, - IdempotencyKey: "2fa-till-saved-ok", + ItemType: "gift_card", + Action: "create", + Amount: 50.00, + PaymentMethod: "saved_card", + UserSavedCardID: &cardID, + UserID: &userID, + IdempotencyKey: "2fa-till-saved-ok", + VerificationCode: "556677", } bodyBytes, _ := json.Marshal(reqBody) diff --git a/backend/handlers/services/services.go b/backend/handlers/services/services.go index b6649e6..e477ef4 100644 --- a/backend/handlers/services/services.go +++ b/backend/handlers/services/services.go @@ -486,14 +486,35 @@ func ServicesHandler(w http.ResponseWriter, r *http.Request) { } // ServicesEligibleForUserHandler returns services with eligibility calculated for a specific user -// Used by admin booking flows when booking on behalf of a user +// Used by admin booking flows when booking on behalf of a user, and by a user +// fetching their own eligibility. +// +// B4 (security): the response exposes DOB-derived age and health-data-ish patch +// test status, so the endpoint REQUIRES authentication AND the caller must +// either own the requested user_id or be an admin. The user-agnostic +// /api/services variant stays public; this per-user variant must not be +// queryable for an arbitrary user_id unauthenticated. func ServicesEligibleForUserHandler(w http.ResponseWriter, r *http.Request) { + callerID, ok := mw.GetUserID(r.Context()) + if !ok || callerID == "" { + http.Error(w, "unauthorized", http.StatusUnauthorized) + return + } + role, _ := mw.GetUserRole(r.Context()) + userID := chi.URLParam(r, "user_id") if userID == "" || !validators.IsValidID(userID) { http.Error(w, "user not found", http.StatusNotFound) return } + // Ownership check: only the user themselves or an admin may fetch + // DOB/age + patch-test eligibility for a user. Deny otherwise (B4). + if role != "admin" && userID != callerID { + http.Error(w, "forbidden", http.StatusForbidden) + return + } + // Get user's date of birth var dob time.Time err := db.Conn.QueryRow(r.Context(), `SELECT date_of_birth FROM users WHERE id = $1`, userID).Scan(&dob) diff --git a/backend/handlers/services/services_test.go b/backend/handlers/services/services_test.go index 3642a4a..97714c7 100644 --- a/backend/handlers/services/services_test.go +++ b/backend/handlers/services/services_test.go @@ -126,6 +126,81 @@ func findLastSegment(path, prefix string) int { return -1 } +// eligibleForUserCtx injects the authenticated-owner context that the +// B4-fixed ServicesEligibleForUserHandler now requires: the caller must own +// the requested user_id (or be an admin), so tests fetch eligibility as the +// owner themselves. +func eligibleForUserCtx(ctx context.Context, userID string) context.Context { + ctx = context.WithValue(ctx, mw.UserIDKey, userID) + ctx = context.WithValue(ctx, mw.UserRoleKey, "verified_email") + return ctx +} + +// TestServices_EligibleForUser_UnauthenticatedDenied verifies the B4 fix: an +// unauthenticated request for an arbitrary user_id is rejected 401 before any +// DOB/age or patch-test data is exposed. +func TestServices_EligibleForUser_UnauthenticatedDenied(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := createUserWithDOB(ctx, tx, "2000-01-01") + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + + handler := http.HandlerFunc(ServicesEligibleForUserHandler) + req := httptest.NewRequest("GET", "/api/services/eligible-for/"+userID, nil) + w := makeRequestWithContext(handler, req, ctx) + + if w.Code != http.StatusUnauthorized { + t.Errorf("expected 401 for unauthenticated eligible-for request, got %d. body: %s", w.Code, w.Body.String()) + } +} + +// TestServices_EligibleForUser_OtherUserForbidden verifies the B4 ownership +// check: an authenticated non-admin caller requesting ANOTHER user's +// eligibility is rejected 403. +func TestServices_EligibleForUser_OtherUserForbidden(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := createUserWithDOB(ctx, tx, "2000-01-01") + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + + handler := http.HandlerFunc(ServicesEligibleForUserHandler) + req := httptest.NewRequest("GET", "/api/services/eligible-for/"+userID, nil) + w := makeRequestWithContext(handler, req, eligibleForUserCtx(ctx, "someone-else")) + + if w.Code != http.StatusForbidden { + t.Errorf("expected 403 for cross-user eligible-for request, got %d. body: %s", w.Code, w.Body.String()) + } +} + +// TestServices_EligibleForUser_AdminAllowed verifies the B4 admin carve-out: an +// admin may fetch eligibility for any user (the admin booking flows). +func TestServices_EligibleForUser_AdminAllowed(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := createUserWithDOB(ctx, tx, "2000-01-01") + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + _, err = tx.Exec(ctx, ` + INSERT INTO services (name, description, price, duration_minutes, is_active, minimum_age_required) + VALUES ('Admin View Service', 'Visible to admins', 25.00, 30, true, 0) + `) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + + handler := http.HandlerFunc(ServicesEligibleForUserHandler) + req := httptest.NewRequest("GET", "/api/services/eligible-for/"+userID, nil) + adminCtx := context.WithValue(ctx, mw.UserIDKey, "admin001") + adminCtx = context.WithValue(adminCtx, mw.UserRoleKey, "admin") + w := makeRequestWithContext(handler, req, adminCtx) + + if w.Code != http.StatusOK { + t.Errorf("expected 200 for admin eligible-for request, got %d. body: %s", w.Code, w.Body.String()) + } +} + // TestServices_ListAll verifies that listing all services returns only active services, // filtering out inactive services from the response. func TestServices_ListAll(t *testing.T) { @@ -199,7 +274,7 @@ func TestServices_EligibleForUser_AgeFilter(t *testing.T) { handler := http.HandlerFunc(ServicesEligibleForUserHandler) req := httptest.NewRequest("GET", "/api/services/eligible-for/"+userID, nil) - w := makeRequestWithContext(handler, req, ctx) + w := makeRequestWithContext(handler, req, eligibleForUserCtx(ctx, userID)) if w.Code != http.StatusOK { t.Errorf("expected status 200, got %d. body: %s", w.Code, w.Body.String()) @@ -283,7 +358,7 @@ func TestServices_EligibleForUser_PatchTest(t *testing.T) { handler := http.HandlerFunc(ServicesEligibleForUserHandler) req := httptest.NewRequest("GET", "/api/services/eligible-for/"+userID, nil) - w := makeRequestWithContext(handler, req, ctx) + w := makeRequestWithContext(handler, req, eligibleForUserCtx(ctx, userID)) if w.Code != http.StatusOK { t.Errorf("expected status 200, got %d. body: %s", w.Code, w.Body.String()) @@ -593,7 +668,7 @@ func TestServices_EligibleForUser_PatchTest_Required_NoRecord(t *testing.T) { handler := http.HandlerFunc(ServicesEligibleForUserHandler) req := httptest.NewRequest("GET", "/api/services/eligible-for/"+userID, nil) - w := makeRequestWithContext(handler, req, ctx) + w := makeRequestWithContext(handler, req, eligibleForUserCtx(ctx, userID)) if w.Code != http.StatusOK { t.Errorf("expected status 200, got %d. body: %s", w.Code, w.Body.String()) @@ -656,7 +731,7 @@ func TestServices_EligibleForUser_PatchTest_Required_NoticePeriod(t *testing.T) handler := http.HandlerFunc(ServicesEligibleForUserHandler) req := httptest.NewRequest("GET", "/api/services/eligible-for/"+userID, nil) - w := makeRequestWithContext(handler, req, ctx) + w := makeRequestWithContext(handler, req, eligibleForUserCtx(ctx, userID)) if w.Code != http.StatusOK { t.Errorf("expected status 200, got %d. body: %s", w.Code, w.Body.String()) @@ -719,7 +794,7 @@ func TestServices_EligibleForUser_PatchTest_Expired(t *testing.T) { handler := http.HandlerFunc(ServicesEligibleForUserHandler) req := httptest.NewRequest("GET", "/api/services/eligible-for/"+userID, nil) - w := makeRequestWithContext(handler, req, ctx) + w := makeRequestWithContext(handler, req, eligibleForUserCtx(ctx, userID)) if w.Code != http.StatusOK { t.Errorf("expected status 200, got %d. body: %s", w.Code, w.Body.String()) diff --git a/backend/handlers/user/profile.go b/backend/handlers/user/profile.go index 479598f..8d0f91a 100644 --- a/backend/handlers/user/profile.go +++ b/backend/handlers/user/profile.go @@ -775,15 +775,42 @@ func ChangePasswordHandler(w http.ResponseWriter, r *http.Request) { return } + // B9: actually revoke this session's access token AND every refresh token, + // inside the same transaction as the password update. A password change + // must invalidate all existing sessions — otherwise a stolen token (access + // or refresh) would survive the credential rotation. + // + // (a) Insert the current access token's JTI into revoked_jtis so it can + // never be reused. expires_at outlives the 1-hour token lifetime, so the + // revocation record cannot lapse while the token is still valid. + jti, _ := mw.GetJTI(r.Context()) + if jti != "" { + if _, err := tx.Exec(r.Context(), ` + INSERT INTO revoked_jtis (jti, expires_at) VALUES ($1, $2) + ON CONFLICT (jti) DO NOTHING + `, jti, clock.Now().Add(24*time.Hour)); err != nil { + log.Printf("Failed to revoke JTI for user %s: %v", userID, err) + http.Error(w, "failed to update password", http.StatusInternalServerError) + return + } + } + // (b) Delete all refresh tokens for the user so no rotated/stolen 90-day + // credential can mint a new access token after the password change. + if _, err := tx.Exec(r.Context(), `DELETE FROM refresh_tokens WHERE user_id = $1`, userID); err != nil { + log.Printf("Failed to revoke refresh tokens for user %s: %v", userID, err) + http.Error(w, "failed to update password", http.StatusInternalServerError) + return + } + if err := tx.Commit(r.Context()); err != nil { log.Printf("Failed to commit transaction: %v", err) http.Error(w, "Internal server error", http.StatusInternalServerError) return } - // Revoke all existing tokens by invalidating the current JTI for this user - // This forces the user to re-authenticate after changing their password - log.Printf("Password changed for user %s - existing sessions should re-authenticate", userID) + // B9: the current JTI is now in revoked_jtis and every refresh token for + // the user has been deleted — existing sessions must re-authenticate. + log.Printf("Password changed for user %s - current JTI revoked and all refresh tokens deleted; existing sessions must re-authenticate", userID) w.WriteHeader(http.StatusOK) } diff --git a/backend/handlers/user/profile_test.go b/backend/handlers/user/profile_test.go index bb10e68..9f97380 100644 --- a/backend/handlers/user/profile_test.go +++ b/backend/handlers/user/profile_test.go @@ -29,6 +29,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "crussell/auth" "crussell/internal/s3" "crussell/mw" "crussell/testutils" @@ -148,6 +149,75 @@ func TestPasswordChange_Success(t *testing.T) { } } +// TestPasswordChange_RevokesTokens verifies the B9 fix: changing the password +// actually revokes the current access token's JTI (into revoked_jtis) and +// deletes every refresh token for the user, inside the change transaction. +func TestPasswordChange_RevokesTokens(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create test user: %v", err) + } + + // A real access token (with a real JTI) + a refresh token row. + tokenString, jti, err := auth.GenerateToken(userID, "verified_email") + if err != nil { + t.Fatalf("failed to generate token: %v", err) + } + refreshToken, err := auth.GenerateRefreshToken(ctx, userID, "verified_email") + if err != nil { + t.Fatalf("failed to generate refresh token: %v", err) + } + + var rtCountBefore int + if err := tx.QueryRow(ctx, "SELECT COUNT(*) FROM refresh_tokens WHERE user_id = $1", userID).Scan(&rtCountBefore); err != nil { + t.Fatalf("failed to count refresh tokens: %v", err) + } + if rtCountBefore != 1 { + t.Fatalf("expected 1 refresh token before password change, got %d", rtCountBefore) + } + + changeReq := ChangePasswordRequest{ + CurrentPassword: "testpassword123", + NewPassword: "newpassword456", + } + body, _ := json.Marshal(changeReq) + + req := httptest.NewRequest(http.MethodPut, "/api/user/change-password", bytes.NewReader(body)) + reqCtx := context.WithValue(ctx, mw.UserIDKey, userID) + reqCtx = context.WithValue(reqCtx, mw.JTIKey, jti) + req = req.WithContext(reqCtx) + req.Header.Set("Authorization", "Bearer "+tokenString) + req.Header.Set("Content-Type", "application/json") + + rr := httptest.NewRecorder() + ChangePasswordHandler(rr, req) + + if rr.Code != http.StatusOK { + t.Fatalf("expected status 200, got %d. body: %s", rr.Code, rr.Body.String()) + } + + // (a) The current JTI is revoked. + if !auth.IsJTIRevoked(ctx, jti) { + t.Error("expected the current JTI to be revoked after a password change (B9)") + } + + // (b) Every refresh token for the user was deleted. + var rtCountAfter int + if err := tx.QueryRow(ctx, "SELECT COUNT(*) FROM refresh_tokens WHERE user_id = $1", userID).Scan(&rtCountAfter); err != nil { + t.Fatalf("failed to count refresh tokens after password change: %v", err) + } + if rtCountAfter != 0 { + t.Errorf("expected 0 refresh tokens after password change, got %d (B9)", rtCountAfter) + } + + // The consumed refresh token no longer verifies. + if _, _, err := auth.VerifyRefreshToken(ctx, refreshToken); err == nil { + t.Error("refresh token must be invalid after a password change (B9)") + } +} + // TestPasswordChange_WrongOld verifies that providing an incorrect current password returns 401 Unauthorized. func TestPasswordChange_WrongOld(t *testing.T) { t.Parallel() diff --git a/backend/handlers/user/twofa.go b/backend/handlers/user/twofa.go index 522fd8b..a44241d 100644 --- a/backend/handlers/user/twofa.go +++ b/backend/handlers/user/twofa.go @@ -1,25 +1,21 @@ package user import ( - "crypto/hmac" + "context" "crypto/rand" - "crypto/sha256" - "crypto/subtle" "database/sql" - "encoding/hex" "encoding/json" "errors" "fmt" "log" "math/big" "net/http" - "sync" - "sync/atomic" "time" "crussell/clock" "crussell/db" "crussell/handlers/payments" + "crussell/internal/twofa" "crussell/internal/validators" "crussell/mw" @@ -67,188 +63,57 @@ var errTwoFADeliveryUnavailable = errors.New("2FA requires an email or SMS deliv // twoFAEnsureIssueAllowed guards code issuance; both are build-dependent. // Dev/test builds keep the documented loose-fake fallback (twofa_dev.go); // production builds refuse to issue codes without the pepper (twofa_prod.go), -// matching how main.go fails closed on a missing JWT_SECRET_KEY. hashTwoFACode -// calls twoFAPepper. +// matching how main.go fails closed on a missing JWT_SECRET_KEY. The pepper +// reader is registered into crussell/internal/twofa via twofa.SetPepperProvider +// by the build-tagged files (twofa_dev.go / twofa_prod.go). -// hashTwoFACode returns the hex digest of a verification code as stored in the -// DB. With TWO_FACTOR_PEPPER set the digest is HMAC-SHA256 keyed by the pepper, -// so a leaked digest cannot be brute-forced offline (the key stays server-side). -// When the pepper is unset it falls back to the legacy unsalted SHA-256 digest: -// dev/test builds also log a one-time warning, while production builds can -// never persist such a digest because issuance fails closed without the pepper -// (twoFAEnsureIssueAllowed) — the fallback survives only for the legacy-row -// migration window and the dev/test loose-fake flow. The plaintext code is -// never stored; delivery is build-dependent (see deliverTwoFACode). -func hashTwoFACode(code string) string { - if pepper := twoFAPepper(); pepper != "" { - mac := hmac.New(sha256.New, []byte(pepper)) - mac.Write([]byte(code)) - return hex.EncodeToString(mac.Sum(nil)) - } - sum := sha256.Sum256([]byte(code)) - return hex.EncodeToString(sum[:]) -} - -// legacyHashTwoFACode returns the pre-pepper plain SHA-256 digest, used to -// verify rows written before TWO_FACTOR_PEPPER was provisioned during the -// migration window (see verifyTwoFACodeHash). -func legacyHashTwoFACode(code string) string { - sum := sha256.Sum256([]byte(code)) - return hex.EncodeToString(sum[:]) -} - -// verifyTwoFACodeHash reports whether reqCode matches a stored pending-code -// digest, always in constant time (subtle.ConstantTimeCompare). The first -// comparison uses the current pepper'd digest; when that fails the stored hash -// may be a legacy pre-pepper plain SHA-256 (rows written before TWO_FACTOR_PEPPER -// was provisioned), so the legacy digest is tried too. When a legacy row -// matches, legacy is true and the caller should re-hash with the pepper on the -// next successful verify, retiring the plain digest. -func verifyTwoFACodeHash(reqCode, storedHash string) (match, legacy bool) { - if subtle.ConstantTimeCompare([]byte(hashTwoFACode(reqCode)), []byte(storedHash)) == 1 { - return true, false - } - if subtle.ConstantTimeCompare([]byte(legacyHashTwoFACode(reqCode)), []byte(storedHash)) == 1 { - return true, true - } - return false, false -} +// The verification core (per-user attempt-state map, brute-force lockout, +// constant-time code check) lives in crussell/internal/twofa — a package that +// imports neither handlers/user nor handlers/payments, so the payments +// card-access gate can verify a real 2FA challenge without an import cycle +// (B11c). The identifiers below are thin aliases/wrappers so the HTTP handlers +// and the existing tests keep their original names. // twoFAMaxAttempts is the number of consecutive failed verify attempts allowed // before the pending code is invalidated and a new one must be requested. -const twoFAMaxAttempts = 5 +const twoFAMaxAttempts = twofa.MaxAttempts // twoFAAttemptWindow bounds how long a per-user attempt counter lives before // resetting, and doubles as the stale-entry eviction horizon for the map. -const twoFAAttemptWindow = 10 * time.Minute +const twoFAAttemptWindow = twofa.AttemptWindow -// twoFAMaxTrackedAttempts caps the in-memory attempt map so a flood of distinct -// user IDs cannot grow it without bound. Counters are purely in-memory (the DB -// schema is locked — there is no attempt column), so they reset on process -// restart; the 10-minute pending-code expiry bounds the practical impact. -// Declared as a var so the eviction policy is unit-testable at a small cap. -var twoFAMaxTrackedAttempts = 10_000 +// hashTwoFACode returns the hex digest of a verification code as stored in the +// DB (pepper-driven HMAC-SHA256, or the legacy plain SHA-256 when the pepper is +// unset). Delegates to the shared implementation. +func hashTwoFACode(code string) string { return twofa.Hash(code) } -// twoFAAttemptState tracks consecutive failed verify attempts for one user. The -// per-user mutex serializes the whole verify critical section so concurrent -// attempts from the same user cannot race the limit check. count and lastAt are -// atomic so the map eviction path can read them without taking the per-user -// mutex (lock ordering forbids mapMu→st.mu: checkTwoFACode holds st.mu then -// takes mapMu). lastAt is stored as nanoseconds since the Unix epoch so the -// eviction scan and lockedOut read it race-free even on 32-bit platforms — a -// plain time.Time read/write pair there could tear the 8-byte timestamp and -// reset or extend the lockout window. -// lastMintAt is the disable-flow mint cooldown stamp (see twoFAMintCooldown); -// it is only ever touched under st.mu. -type twoFAAttemptState struct { - mu sync.Mutex - count atomic.Int32 - lastAt atomic.Int64 - lastMintAt time.Time +// legacyHashTwoFACode returns the pre-pepper plain SHA-256 digest. +func legacyHashTwoFACode(code string) string { return twofa.LegacyHash(code) } + +// verifyTwoFACodeHash reports whether reqCode matches a stored pending-code +// digest, always in constant time. Delegates to the shared implementation. +func verifyTwoFACodeHash(reqCode, storedHash string) (match, legacy bool) { + return twofa.VerifyHash(reqCode, storedHash) } -// lastActive returns the state's last-activity timestamp (nanoseconds since the -// Unix epoch, UTC). Reads are atomic so the map eviction scan can call it while -// holding only mapMu. -func (st *twoFAAttemptState) lastActive() time.Time { - return time.Unix(0, st.lastAt.Load()).UTC() -} +// twoFAAttemptState aliases the shared per-user attempt state. +type twoFAAttemptState = twofa.AttemptState -// setLastActive records a last-activity timestamp. Writes happen under st.mu -// (checkTwoFACode) while the eviction scan reads under mapMu only — the atomic -// store makes both race-free. -func (st *twoFAAttemptState) setLastActive(t time.Time) { - st.lastAt.Store(t.UnixNano()) -} - -// lockedOut reports whether the state is inside its lockout window: the attempt -// counter has reached the cap and the window has not yet elapsed. Such a record -// is the rate limit's source of truth for its user and must never be evicted -// while in-window — evicting it would silently reset the counter and grant a -// fresh guessing budget. -func (st *twoFAAttemptState) lockedOut(now time.Time) bool { - return st.count.Load() >= twoFAMaxAttempts && now.Sub(st.lastActive()) <= twoFAAttemptWindow -} - -var ( - twoFAAttemptMapMu sync.Mutex - twoFAAttemptMap = make(map[string]*twoFAAttemptState) -) - -// twoFAAttemptStateFor returns the per-user attempt state, creating it if -// needed. The map is bounded: stale (window-expired) entries are evicted -// opportunistically and, when at capacity, the least-recently-active -// non-locked-out entry is dropped. A record still inside its lockout window is -// NEVER evicted — evicting it would reset the victim's attempt counter and -// bypass the rate limit under a hostile flood of new keys. When the map is -// full of in-window locked-out records (a pathological flood), a transient, -// untracked state is returned instead of growing the map past the cap. +// twoFAAttemptStateFor returns the shared per-user attempt state (creating it +// if needed), under the bounded never-evict-in-lockout map. func twoFAAttemptStateFor(userID string) *twoFAAttemptState { - twoFAAttemptMapMu.Lock() - defer twoFAAttemptMapMu.Unlock() - now := clock.Now() - - if len(twoFAAttemptMap) >= twoFAMaxTrackedAttempts { - var oldestID string - var oldestAt time.Time - for id, st := range twoFAAttemptMap { - if now.Sub(st.lastActive()) > twoFAAttemptWindow { - // Idle/expired — its counter has already lapsed; safe to evict. - delete(twoFAAttemptMap, id) - continue - } - if st.lockedOut(now) { - // Inside its lockout window — the rate limit's source of truth - // for this user. Never evict (finding-e fix). - continue - } - if at := st.lastActive(); oldestID == "" || at.Before(oldestAt) { - oldestID, oldestAt = id, at - } - } - if len(twoFAAttemptMap) >= twoFAMaxTrackedAttempts && oldestID != "" { - delete(twoFAAttemptMap, oldestID) - } - if len(twoFAAttemptMap) >= twoFAMaxTrackedAttempts { - // Every entry is a locked-out in-window record. Do not evict one - // (that would reset its rate limit) and do not grow past the cap: - // return a transient, untracked state so THIS request still - // proceeds under a fresh budget. - st := &twoFAAttemptState{} - st.setLastActive(now) - return st - } - } - - st := twoFAAttemptMap[userID] - if st == nil { - st = &twoFAAttemptState{} - st.setLastActive(now) - twoFAAttemptMap[userID] = st - } - return st + return twofa.StateFor(userID) } -// twoFAResetAttempts resets a user's attempt counter in place (count only) -// WITHOUT deleting the entry, preserving lastMintAt so the disable-flow mint -// cooldown survives a fresh-code delivery. Called on successful verify and when -// a fresh code is generated via setup or disable. lastAt is deliberately not -// touched here: it is re-stamped by checkTwoFACode on real activity, and -// writing it under mapMu would race with checkTwoFACode's st.mu-guarded write -// (the setup path holds no st.mu). The lock ordering is st.mu→mapMu at call -// sites, never the reverse (twoFAAttemptStateFor takes mapMu only and never -// takes st.mu). +// twoFAResetAttempts zeroes the shared per-user attempt counter in place. +// Called on successful verify only — a fresh code mint must NOT reset it (B11b). func twoFAResetAttempts(userID string) { - twoFAAttemptMapMu.Lock() - defer twoFAAttemptMapMu.Unlock() - if st := twoFAAttemptMap[userID]; st != nil { - st.count.Store(0) - } + twofa.ResetAttempts(userID) } -// deliverTwoFACode generates a fresh verification code, persists only its +// deliverTwoFACode generates a fresh verification code and persists only its // digest plus the pending expiry (updating two_factor_method when method is -// non-empty), and resets any prior lockout. +// non-empty). // // Delivery is build-dependent (twofa_dev.go / twofa_prod.go): dev/test builds // write the plaintext code to the server log — the documented loose-fake @@ -261,6 +126,10 @@ func twoFAResetAttempts(userID string) { // code that could never reach the user. The API response still only returns the // code when 2FA is unenforced (dev convenience). purpose labels the delivery // (e.g. "setup", "disable 2FA"). +// +// A fresh code does NOT reset the per-user failed-attempt counter (B11b): only +// a successful verify does. Resetting on re-mint would let a password-only +// attacker loop mint → burn 5 guesses → mint forever within a code's lifetime. func deliverTwoFACode(r *http.Request, userID, method, purpose string) (string, error) { if err := twoFAEnsureIssueAllowed(); err != nil { return "", err @@ -290,9 +159,6 @@ func deliverTwoFACode(r *http.Request, userID, method, purpose string) (string, return "", err } - // A fresh code invalidates any prior lockout state. - twoFAResetAttempts(userID) - label := method if label == "" { label = purpose @@ -386,13 +252,30 @@ func SetupTwoFAHandler(w http.ResponseWriter, r *http.Request) { return } + // The per-user mutex serializes the mint with the disable flow's + // checkTwoFACode critical section so concurrent requests from the same user + // cannot race the mint cooldown or the lockout counter. + st := twoFAAttemptStateFor(userID) + st.Mu.Lock() + defer st.Mu.Unlock() + + // Mint cooldown (B11a): the same twoFAMintCooldown guard the disable flow + // applies via ensurePendingTwoFACode now bounds setup re-mints too. A fresh + // setup code no longer resets the failed-attempt counter (B11b), so without + // this a setup-spam loop could mint fresh codes (each invalidating the + // prior lockout state) and keep a guessing budget alive indefinitely. + now := clock.Now() + if !st.LastMintAt.IsZero() && now.Sub(st.LastMintAt) < twoFAMintCooldown { + http.Error(w, "Too many attempts. Wait before requesting a new code.", http.StatusTooManyRequests) + return + } + // Deliver a fresh code via the shared setup mechanism: generate, persist - // only the hash + expiry, reset any prior lockout, and deliver it - // build-dependently (the [2FA] log channel in dev/test; in production only - // when the operator explicitly opted into log delivery — see - // deliverTwoFACode). A production build with no delivery channel fails here - // with a clear, actionable error instead of issuing a code that would never - // reach the user. + // only the hash + expiry, and deliver it build-dependently (the [2FA] log + // channel in dev/test; in production only when the operator explicitly + // opted into log delivery — see deliverTwoFACode). A production build with + // no delivery channel fails here with a clear, actionable error instead of + // issuing a code that would never reach the user. code, err := deliverTwoFACode(r, userID, req.Method, "setup") if err != nil { if errors.Is(err, errTwoFADeliveryUnavailable) { @@ -406,6 +289,7 @@ func SetupTwoFAHandler(w http.ResponseWriter, r *http.Request) { http.Error(w, "server error", http.StatusInternalServerError) return } + st.LastMintAt = now resp := map[string]any{"message": "Code sent"} if !twoFARequired() { @@ -423,90 +307,59 @@ type TwoFAVerifyRequest struct { } // twoFACodeCheckResult classifies checkTwoFACode's outcome so callers can map -// it to the correct HTTP status. -type twoFACodeCheckResult int +// it to the correct HTTP status. Aliased to the shared twofa.Result. +type twoFACodeCheckResult = twofa.Result const ( - twoFACodeOK twoFACodeCheckResult = iota - twoFACodeIncorrect - twoFACodeLockedOut - twoFACodeMissingOrExpired + twoFACodeOK = twofa.OK + twoFACodeIncorrect = twofa.Incorrect + twoFACodeLockedOut = twofa.LockedOut + twoFACodeMissingOrExpired = twofa.MissingOrExpired ) // checkTwoFACode verifies the submitted code against the user's stored pending -// code under the per-user brute-force lockout, shared by VerifyTwoFAHandler and -// DisableTwoFAHandler. The caller must hold st.mu (from twoFAAttemptStateFor) -// so concurrent attempts from the same user cannot race the limit check. A -// correct code resets the attempt counter and returns twoFACodeOK. An incorrect -// code increments the counter and, on the 5th consecutive failure, invalidates -// the pending code (lockout). A missing or expired pending code returns -// twoFACodeMissingOrExpired. 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. +// code under the per-user brute-force lockout, shared by VerifyTwoFAHandler, +// DisableTwoFAHandler and VerifyTwoFACodeForUser. The caller must hold st.Mu +// (from twoFAAttemptStateFor) so concurrent attempts from the same user cannot +// race the limit check. Delegates to the shared implementation in +// crussell/internal/twofa. func checkTwoFACode(r *http.Request, userID string, st *twoFAAttemptState, reqCode string) (twoFACodeCheckResult, error) { - if now := clock.Now(); now.Sub(st.lastActive()) > twoFAAttemptWindow { - st.count.Store(0) - st.setLastActive(now) - } - if st.count.Load() >= twoFAMaxAttempts { - return twoFACodeLockedOut, nil - } + res, err := twofa.Check(r.Context(), userID, st, reqCode) + return twoFACodeCheckResult(res), err +} - var pendingHash sql.NullString - var pendingExpires sql.NullTime - err := db.Conn.QueryRow(r.Context(), ` - SELECT two_factor_pending_code_hash, two_factor_pending_code_expires - FROM users - WHERE id = $1 - `, userID).Scan(&pendingHash, &pendingExpires) +// VerifyTwoFACodeForUser verifies a 2FA code for a user under the same +// per-user brute-force lockout as the interactive endpoints, outside the HTTP +// handler layer. It returns nil on a correct code, or one of the exported +// twofa.ErrIncorrect / twofa.ErrLockedOut / twofa.ErrMissingOrExpired (or a DB +// error, wrapped). +// +// 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 +// crussell/internal/twofa (the shared home of this verification core) instead +// of importing this package. +func VerifyTwoFACodeForUser(ctx context.Context, userID, code string) error { + st := twoFAAttemptStateFor(userID) + st.Mu.Lock() + defer st.Mu.Unlock() + + result, err := twofa.Check(ctx, userID, st, code) if err != nil { - return twoFACodeLockedOut, err + return err } - if !pendingHash.Valid || !pendingExpires.Valid || !pendingExpires.Time.After(clock.Now()) { - return twoFACodeMissingOrExpired, nil + switch result { + case twoFACodeOK: + return nil + case twoFACodeIncorrect: + return twofa.ErrIncorrect + case twoFACodeLockedOut: + return twofa.ErrLockedOut + case twoFACodeMissingOrExpired: + return twofa.ErrMissingOrExpired } - // Constant-time compare (subtle) so a wrong code's match position cannot be - // inferred from response timing. Both digests are fixed-length hex. Legacy - // pre-pepper rows (plain SHA-256, hashed before TWO_FACTOR_PEPPER existed) - // still verify during the transition window. - match, legacy := verifyTwoFACodeHash(reqCode, pendingHash.String) - if !match { - st.count.Add(1) - st.setLastActive(clock.Now()) - if st.count.Load() >= twoFAMaxAttempts { - // Lockout reached: destroy the pending code so a stolen digest - // cannot be replayed against a fresh guessing loop. - if _, err := db.Conn.Exec(r.Context(), ` - UPDATE users - SET two_factor_pending_code_hash = NULL, - two_factor_pending_code_expires = NULL - WHERE id = $1 - `, userID); err != nil { - log.Printf("failed to invalidate 2FA pending code for user %s: %v", userID, err) - } - return twoFACodeLockedOut, nil - } - return twoFACodeIncorrect, nil - } - - // Success: a legacy (pre-pepper) hash that verified is re-hashed with the - // pepper so the plain digest is retired on the next successful verify. - if legacy { - if _, err := db.Conn.Exec(r.Context(), ` - UPDATE users - SET two_factor_pending_code_hash = $2 - WHERE id = $1 - `, userID, hashTwoFACode(reqCode)); err != nil { - log.Printf("failed to upgrade legacy 2FA pending code hash for user %s: %v", userID, err) - } - } - // Success: clear the attempt counter (and any disable-flow mint cooldown) - // before the caller performs its action. - st.count.Store(0) - st.setLastActive(clock.Now()) - st.lastMintAt = time.Time{} - twoFAResetAttempts(userID) - return twoFACodeOK, nil + return nil } // POST /api/user/2fa/verify @@ -543,8 +396,8 @@ func VerifyTwoFAHandler(w http.ResponseWriter, r *http.Request) { // the limit; after 5 consecutive failures the pending code is invalidated // and further attempts get 429 until a new code is requested via setup. st := twoFAAttemptStateFor(userID) - st.mu.Lock() - defer st.mu.Unlock() + st.Mu.Lock() + defer st.Mu.Unlock() result, err := checkTwoFACode(r, userID, st, req.Code) if err != nil { @@ -650,8 +503,8 @@ func SendDisableCodeHandler(w http.ResponseWriter, r *http.Request) { // checkTwoFACode critical section so concurrent requests from the same user // cannot race the cooldown or lockout counters. st := twoFAAttemptStateFor(userID) - st.mu.Lock() - defer st.mu.Unlock() + st.Mu.Lock() + defer st.Mu.Unlock() if err := ensurePendingTwoFACode(r, userID, st); err != nil { if errors.Is(err, errTwoFAMintThrottled) { @@ -715,14 +568,16 @@ func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { // (fresh-code generation + code check) so concurrent requests cannot race // the lockout counter. st := twoFAAttemptStateFor(userID) - st.mu.Lock() - defer st.mu.Unlock() + st.Mu.Lock() + defer st.Mu.Unlock() // Reuse a valid pending code when one exists; otherwise generate + deliver - // a fresh one via the same build-dependent channel as setup. A fresh code gets its - // own independent 5-attempt budget (the mint resets the counter), so the - // per-user mint cooldown is what stops the unlimited-guess loop — an - // attacker can mint at most one fresh code per twoFAMintCooldown. + // a fresh one via the same build-dependent channel as setup. Minting a fresh + // code does NOT reset the failed-attempt counter (B11b): the counter resets + // only on a successful verify or when the 10-minute attempt window elapses. + // The per-user mint cooldown still bounds how often a fresh code can be + // minted — at most one per twoFAMintCooldown — but it cannot grant a fresh + // guessing budget. if err := ensurePendingTwoFACode(r, userID, st); err != nil { if errors.Is(err, errTwoFAMintThrottled) { http.Error(w, "Too many attempts. Wait before requesting a new code.", http.StatusTooManyRequests) @@ -773,14 +628,16 @@ func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { // stored code is missing or expired. The caller must hold the user's // attempt-state mutex. // -// A fresh code gets its own independent 5-attempt budget (deliverTwoFACode -// resets the counter via twoFAResetAttempts), so the per-user mint cooldown is -// what prevents a password-only attacker from looping mint → burn 5 guesses → -// mint forever: only one fresh code per twoFAMintCooldown per user. A locked-out -// user can still use the code minted in THIS request; a user who exhausts it -// must wait out the cooldown for the next mint — the documented disable-flow -// residual. A failed delivery does not start the cooldown (the stamp is written -// only after the UPDATE persisted). +// Minting a fresh code does NOT reset the failed-attempt counter (B11b): the +// counter resets only on a successful verify or when the 10-minute attempt +// window elapses (see twoFAResetAttempts — called on successful verify only). +// The per-user mint cooldown still bounds how often a fresh code can be minted +// (at most one per twoFAMintCooldown per user), but it cannot grant a fresh +// guessing budget. A locked-out user therefore stays locked out until the +// attempt window elapses; the documented disable-flow residual is that a user +// who exhausts the budget must wait out the window, not the mint cooldown. A +// failed delivery does not start the cooldown (the stamp is written only after +// the UPDATE persisted). func ensurePendingTwoFACode(r *http.Request, userID string, st *twoFAAttemptState) error { var pendingHash sql.NullString var pendingExpires sql.NullTime @@ -796,13 +653,13 @@ func ensurePendingTwoFACode(r *http.Request, userID string, st *twoFAAttemptStat return nil } now := clock.Now() - if !st.lastMintAt.IsZero() && now.Sub(st.lastMintAt) < twoFAMintCooldown { + if !st.LastMintAt.IsZero() && now.Sub(st.LastMintAt) < twoFAMintCooldown { return errTwoFAMintThrottled } if _, err := deliverTwoFACode(r, userID, "", "disable 2FA"); err != nil { return err } - st.lastMintAt = now + st.LastMintAt = now return nil } @@ -822,11 +679,10 @@ func disableTwoFA(r *http.Request, userID string) error { // twoFADeleteAttempts removes a user's attempt-map entry entirely, unlike // twoFAResetAttempts which only zeroes the count in place. The admin 2FA // removal flow uses it so any lingering lockout/counter/mint-cooldown state is -// dropped wholesale and a re-setup starts from a clean slate. +// dropped wholesale and a re-setup starts from a clean slate. Delegates to the +// shared implementation. func twoFADeleteAttempts(userID string) { - twoFAAttemptMapMu.Lock() - defer twoFAAttemptMapMu.Unlock() - delete(twoFAAttemptMap, userID) + twofa.DeleteAttempts(userID) } // removeUser2FA clears all 2FA state for a user: the enabled flag, method, diff --git a/backend/handlers/user/twofa_dev.go b/backend/handlers/user/twofa_dev.go index 1615ef4..38f71df 100644 --- a/backend/handlers/user/twofa_dev.go +++ b/backend/handlers/user/twofa_dev.go @@ -10,6 +10,7 @@ package user // never log the code and fail closed without the pepper — see twofa_prod.go. import ( + "crussell/internal/twofa" "log" "os" "sync" @@ -21,18 +22,20 @@ import ( // issuance instead. var twoFAPepperWarnOnce sync.Once -// twoFAPepper returns the configured HMAC pepper, or "" when unset, logging the -// documented one-time warning. Read per call (the rest of the backend reads env -// vars per call too) so a value provisioned at runtime is picked up; only the -// warning is gated on sync.Once. -func twoFAPepper() string { - pepper := os.Getenv(twoFAPepperEnv) - if pepper == "" { - twoFAPepperWarnOnce.Do(func() { - log.Printf("WARNING: TWO_FACTOR_PEPPER unset — 2FA codes hashed without an HMAC pepper (falling back to unsalted SHA-256); set TWO_FACTOR_PEPPER in production so a leaked digest cannot be brute-forced offline") - }) - } - return pepper +// init registers the dev/test pepper reader into the shared verification core +// (crussell/internal/twofa): the documented loose-fake fallback — the legacy +// unsalted SHA-256 digest with a one-time warning when TWO_FACTOR_PEPPER is +// unset. Production builds fail closed instead (twofa_prod.go). +func init() { + twofa.SetPepperProvider(func() string { + pepper := os.Getenv(twoFAPepperEnv) + if pepper == "" { + twoFAPepperWarnOnce.Do(func() { + log.Printf("WARNING: TWO_FACTOR_PEPPER unset — 2FA codes hashed without an HMAC pepper (falling back to unsalted SHA-256); set TWO_FACTOR_PEPPER in production so a leaked digest cannot be brute-forced offline") + }) + } + return pepper + }) } // twoFAEnsureIssueAllowed always permits code issuance in dev/test builds: the diff --git a/backend/handlers/user/twofa_prod.go b/backend/handlers/user/twofa_prod.go index 7b78925..0524bfa 100644 --- a/backend/handlers/user/twofa_prod.go +++ b/backend/handlers/user/twofa_prod.go @@ -24,6 +24,7 @@ package user // operator explicitly opted into log delivery and accepted its risk. import ( + "crussell/internal/twofa" "errors" "log" "os" @@ -48,12 +49,12 @@ const twoFAAllowLogDeliveryEnv = "TWO_FACTOR_ALLOW_LOG_DELIVERY" // brute-force offline. var errTwoFAPepperRequired = errors.New("TWO_FACTOR_PEPPER is not set; refusing to issue a 2FA code (an unsalted digest would be offline-brute-forceable)") -// twoFAPepper returns the configured HMAC pepper, or "" when unset. Production -// builds do NOT warn or fall back to the legacy digest: code issuance fails -// closed via twoFAEnsureIssueAllowed, so no pending code is ever persisted as -// an unsalted SHA-256 digest. -func twoFAPepper() string { - return os.Getenv(twoFAPepperEnv) +// init registers the production pepper reader into the shared verification +// core (crussell/internal/twofa): raw env read, no fallback — code issuance +// fails closed via twoFAEnsureIssueAllowed, so no pending code is ever +// persisted as an unsalted SHA-256 digest. +func init() { + twofa.SetPepperProvider(func() string { return os.Getenv(twoFAPepperEnv) }) } // twoFADeliveryAvailable reports whether a 2FA code delivery channel exists in diff --git a/backend/handlers/user/twofa_test.go b/backend/handlers/user/twofa_test.go index 3ee895f..9fa1342 100644 --- a/backend/handlers/user/twofa_test.go +++ b/backend/handlers/user/twofa_test.go @@ -30,6 +30,7 @@ import ( "crussell/clock" "crussell/db" + "crussell/internal/twofa" "crussell/mw" "crussell/testutils" "crussell/testutils/fixtures" @@ -363,12 +364,13 @@ func TestTwoFADisable_LockoutAfterFiveFailedAttempts(t *testing.T) { require.True(t, enabled, "locked-out user must still have 2FA enabled") } -// TestTwoFA_VerifyAndDisable_SharedLockoutResetsOnFreshCode pins the shared -// per-user lockout across verify and disable: 5 wrong VERIFY attempts 429 and -// destroy the pending code; a subsequent DISABLE with a wrong code returns 400 -// (not 429) because the disable flow delivers a FRESH code which resets the -// shared counter — and only that fresh code (not the old one) succeeds. -func TestTwoFA_VerifyAndDisable_SharedLockoutResetsOnFreshCode(t *testing.T) { +// TestTwoFA_VerifyAndDisable_SharedLockoutPersistsAcrossMints pins the shared +// per-user lockout across verify and disable under B11b: 5 wrong VERIFY +// attempts 429 and destroy the pending code; the disable flow mints a FRESH +// code, but the persistent failed-attempt counter is NOT reset by the mint — a +// stale OR the freshly-minted code both stay 429 until the attempt window +// elapses. +func TestTwoFA_VerifyAndDisable_SharedLockoutPersistsAcrossMints(t *testing.T) { twofaEnvEnforced(t) ctx, tx := testutils.SetupTestTx(t) userID, err := fixtures.CreateTestUser(tx) @@ -385,25 +387,32 @@ func TestTwoFA_VerifyAndDisable_SharedLockoutResetsOnFreshCode(t *testing.T) { w := performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "999999"}, userID) require.Equal(t, http.StatusTooManyRequests, w.Code, w.Body.String()) - // The lockout is shared: the OLD code is gone, so disabling with it fails. var buf bytes.Buffer log.SetOutput(&buf) t.Cleanup(func() { log.SetOutput(os.Stderr) }) - // First disable delivers a fresh code (resetting the shared counter) and - // rejects the stale submission with 400, not 429. + // B11b: the lockout survives the fresh-code mint — the stale submission + // stays throttled (429), it is NOT re-budgeted (400). w = performUser2FARequest(t, DisableTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/disable", TwoFADisableRequest{Code: "123456"}, userID) - require.Equal(t, http.StatusBadRequest, w.Code, "stale code must be rejected after lockout") + require.Equal(t, http.StatusTooManyRequests, w.Code, "stale code must stay locked out after verify lockout; body: %s", w.Body.String()) - // The freshly delivered code succeeds in the SAME request that generated it. + // The freshly minted code is ALSO throttled: the persistent counter caps + // total failed attempts per code lifetime, not per code instance. freshCode := extractCodeFromLog(t, buf.String()) require.NotEmpty(t, freshCode, "disable must deliver a fresh code after verify lockout") w = performUser2FARequest(t, DisableTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/disable", TwoFADisableRequest{Code: freshCode}, userID) + require.Equal(t, http.StatusTooManyRequests, w.Code, "fresh code must not reset the shared lockout; body: %s", w.Body.String()) + + // Once the attempt window elapses (the code lifetime), the valid pending + // code verifies and disables 2FA. + st := twoFAAttemptStateFor(userID) + st.SetLastActive(clock.Now().Add(-twoFAAttemptWindow - time.Second)) + w = performUser2FARequest(t, DisableTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/disable", TwoFADisableRequest{Code: freshCode}, userID) require.Equal(t, http.StatusOK, w.Code, w.Body.String()) var enabled bool require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_enabled FROM users WHERE id = $1", userID).Scan(&enabled)) - require.False(t, enabled, "fresh code must disable 2FA after the shared lockout") + require.False(t, enabled, "fresh code must disable 2FA after the shared lockout window elapses") } // TestTwoFADisable_Unenforced_NoCodeRequired verifies the dev bypass: in an @@ -529,6 +538,32 @@ func TestTwoFASetup_CodeAlwaysLoggedAsDeliveryChannel(t *testing.T) { require.Regexp(t, regexp.MustCompile(`\[2FA\].*\d{6}`), buf.String(), "unenforced setup must log the plaintext code") } +// TestTwoFASetup_MintThrottled verifies the B11a fix: the setup path applies +// the same per-user mint cooldown as the disable flow, so a setup-spam loop +// cannot mint fresh codes faster than once per twoFAMintCooldown and keep a +// guessing budget alive. +func TestTwoFASetup_MintThrottled(t *testing.T) { + twofaEnvEnforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + // First setup is allowed and mints a code. + w := performUser2FARequest(t, SetupTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/setup", TwoFASetupRequest{Method: "email"}, userID) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + // A second setup inside the cooldown is throttled with 429. + w = performUser2FARequest(t, SetupTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/setup", TwoFASetupRequest{Method: "sms"}, userID) + require.Equal(t, http.StatusTooManyRequests, w.Code, w.Body.String()) + require.Contains(t, w.Body.String(), "Wait before requesting a new code.") + + // Elapsing the cooldown allows a fresh mint (the test cannot wait a minute). + st := twoFAAttemptStateFor(userID) + st.LastMintAt = clock.Now().Add(-twoFAMintCooldown - time.Second) + w = performUser2FARequest(t, SetupTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/setup", TwoFASetupRequest{Method: "email"}, userID) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) +} + func TestTwoFAVerify_LockoutAfterFiveFailedAttempts(t *testing.T) { twofaEnvEnforced(t) ctx, tx := testutils.SetupTestTx(t) @@ -561,7 +596,7 @@ func TestTwoFAVerify_LockoutAfterFiveFailedAttempts(t *testing.T) { require.Equal(t, http.StatusTooManyRequests, w.Code, "post-lockout attempts must keep returning 429") } -func TestTwoFAVerify_NewCodeViaSetupResetsLockout(t *testing.T) { +func TestTwoFAVerify_NewCodeViaSetup_DoesNotResetLockout(t *testing.T) { twofaEnvEnforced(t) ctx, tx := testutils.SetupTestTx(t) userID, err := fixtures.CreateTestUser(tx) @@ -576,15 +611,24 @@ func TestTwoFAVerify_NewCodeViaSetupResetsLockout(t *testing.T) { w := performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "999999"}, userID) require.Equal(t, http.StatusTooManyRequests, w.Code, w.Body.String()) - // Requesting a new code via setup resets the attempt counter, so - // verification is possible again. + // B11b: a fresh code via setup does NOT reset the persistent failed-attempt + // counter (it resets only on a SUCCESSFUL verify). The mint is allowed + // (first mint, no cooldown) but the lockout survives it. w = performUser2FARequest(t, SetupTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/setup", TwoFASetupRequest{Method: "email"}, userID) require.Equal(t, http.StatusOK, w.Code, w.Body.String()) - // The setup-generated code is unknown (enforced), so seed a fresh known - // code and confirm the reset allows verification. + // The locked-out user stays locked out even with a fresh code pending. seedPendingTwoFA(t, ctx, tx, userID, "654321") w = performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "654321"}, userID) + require.Equal(t, http.StatusTooManyRequests, w.Code, "counter must NOT reset on re-mint; body: %s", w.Body.String()) + + // Only a SUCCESSFUL verify resets the counter. Elapse the attempt window + // (the code lifetime) so the stale counter is dropped, then a fresh verify + // succeeds and clears it. + st := twoFAAttemptStateFor(userID) + st.SetLastActive(clock.Now().Add(-twoFAAttemptWindow - time.Second)) + seedPendingTwoFA(t, ctx, tx, userID, "111111") + w = performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "111111"}, userID) require.Equal(t, http.StatusOK, w.Code, w.Body.String()) } @@ -682,8 +726,8 @@ func TestTwoFAPepper_LegacyHashUpgrade_OnSuccessfulVerify(t *testing.T) { require.NoError(t, err) // checkTwoFACode (the shared verify path) must accept the legacy hash. - st := &twoFAAttemptState{} - st.setLastActive(clock.Now()) + st := &twofa.AttemptState{} + st.SetLastActive(clock.Now()) req := httptest.NewRequest(http.MethodPost, "/api/user/2fa/verify", nil).WithContext(ctx) result, err := checkTwoFACode(req, userID, st, "123456") require.NoError(t, err) @@ -787,8 +831,11 @@ func TestTwoFADisable_MintThrottled_BoundsGuessing(t *testing.T) { } // TestTwoFADisable_MintThrottle_ExpiresAllowsRecovery verifies the documented -// residual is not a permanent lockout: once the mint cooldown elapses, a -// legitimate code-lost user can mint and verify a fresh code again. +// residual is not a permanent lockout: once the mint cooldown AND the attempt +// window (the code lifetime) elapse, a legitimate code-lost user can mint and +// verify a fresh code again. Under B11b the failed-attempt counter persists +// across mints, so elapsing only the mint cooldown is NOT enough — the user +// must also wait out the 10-minute attempt window. func TestTwoFADisable_MintThrottle_ExpiresAllowsRecovery(t *testing.T) { twofaEnvEnforced(t) var buf bytes.Buffer @@ -811,16 +858,30 @@ func TestTwoFADisable_MintThrottle_ExpiresAllowsRecovery(t *testing.T) { w = performUser2FARequest(t, DisableTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/disable", TwoFADisableRequest{Code: "000000"}, userID) require.Equal(t, http.StatusTooManyRequests, w.Code, "mint must be throttled inside the cooldown") - // Simulate the cooldown elapsing (the test cannot wait a real minute). + // Elapsing ONLY the mint cooldown still leaves the user locked out: the + // persistent failed-attempt counter (B11b) survives the fresh mint (the + // mint itself writes the new [2FA] log line). st := twoFAAttemptStateFor(userID) - st.lastMintAt = clock.Now().Add(-twoFAMintCooldown - time.Second) - - // A fresh disable request now mints a new code via the [2FA] log channel and - // rejects the wrong submission with 400 — recovery is possible again. + st.LastMintAt = clock.Now().Add(-twoFAMintCooldown - time.Second) buf.Reset() w = performUser2FARequest(t, DisableTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/disable", TwoFADisableRequest{Code: "000000"}, userID) + require.Equal(t, http.StatusTooManyRequests, w.Code, "persistent counter must keep the user locked until the attempt window elapses") + require.Regexp(t, regexp.MustCompile(`\[2FA\].*\d{6}`), buf.String(), "mint cooldown elapse must allow a fresh mint") + + // Elapse the attempt window too (simulate a real user waiting it out): the + // valid pending code is reused and the lockout lapses — the wrong code is + // rejected with 400 (a fresh 5-attempt budget), not 429. + st.SetLastActive(clock.Now().Add(-twoFAAttemptWindow - time.Second)) + w = performUser2FARequest(t, DisableTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/disable", TwoFADisableRequest{Code: "000000"}, userID) require.Equal(t, http.StatusBadRequest, w.Code, w.Body.String()) - require.Regexp(t, regexp.MustCompile(`\[2FA\].*\d{6}`), buf.String(), "cooldown expiry must allow a fresh mint") + + // After the successful disable (2FA now off), the counter is cleared — a + // subsequent re-enable + verify starts from a fresh budget. + _, err = tx.Exec(ctx, `UPDATE users SET two_factor_enabled = true, two_factor_method = 'email' WHERE id = $1`, userID) + require.NoError(t, err) + seedPendingTwoFA(t, ctx, tx, userID, "123456") + w = performUser2FARequest(t, DisableTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/disable", TwoFADisableRequest{Code: "123456"}, userID) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) } // ============================================================================= @@ -832,44 +893,44 @@ func TestTwoFADisable_MintThrottle_ExpiresAllowsRecovery(t *testing.T) { // hostile flood of new keys cannot reset the victim's attempt counter. Only an // idle/expired record is dropped to make room. func TestTwoFAAttemptMap_InLockoutRecordNotEvicted(t *testing.T) { - twoFAAttemptMapMu.Lock() - origMap := twoFAAttemptMap - origCap := twoFAMaxTrackedAttempts - twoFAAttemptMap = make(map[string]*twoFAAttemptState) - twoFAMaxTrackedAttempts = 4 - twoFAAttemptMapMu.Unlock() + twofa.MapMu.Lock() + origMap := twofa.Map + origCap := twofa.MaxTrackedAttempts + twofa.Map = make(map[string]*twoFAAttemptState) + twofa.MaxTrackedAttempts = 4 + twofa.MapMu.Unlock() t.Cleanup(func() { - twoFAAttemptMapMu.Lock() - twoFAAttemptMap = origMap - twoFAMaxTrackedAttempts = origCap - twoFAAttemptMapMu.Unlock() + twofa.MapMu.Lock() + twofa.Map = origMap + twofa.MaxTrackedAttempts = origCap + twofa.MapMu.Unlock() }) now := clock.Now() for _, id := range []string{"idle_a", "idle_b", "idle_c"} { - st := &twoFAAttemptState{} - st.setLastActive(now.Add(-time.Minute)) - twoFAAttemptMap[id] = st + st := &twofa.AttemptState{} + st.SetLastActive(now.Add(-time.Minute)) + twofa.Map[id] = st } - victim := &twoFAAttemptState{} - victim.setLastActive(now.Add(-time.Second)) - victim.count.Store(5) - twoFAAttemptMap["victim"] = victim + victim := &twofa.AttemptState{} + victim.SetLastActive(now.Add(-time.Second)) + victim.Count.Store(5) + twofa.Map["victim"] = victim // A new user hits the cap: the eviction must drop an idle record, never the // in-lockout victim. st := twoFAAttemptStateFor("new_user") require.NotNil(t, st) - if _, ok := twoFAAttemptMap["victim"]; !ok { + if _, ok := twofa.Map["victim"]; !ok { t.Error("in-lockout record must never be evicted by LRU pressure") } - if got := twoFAAttemptMap["victim"].count.Load(); got != 5 { + if got := twofa.Map["victim"].Count.Load(); got != 5 { t.Errorf("victim attempt count must survive eviction pressure, got %d", got) } - if len(twoFAAttemptMap) > 4 { - t.Errorf("map must stay within the cap, got %d entries", len(twoFAAttemptMap)) + if len(twofa.Map) > 4 { + t.Errorf("map must stay within the cap, got %d entries", len(twofa.Map)) } - if _, ok := twoFAAttemptMap["new_user"]; !ok { + if _, ok := twofa.Map["new_user"]; !ok { t.Error("new user must be tracked in the map") } } @@ -879,34 +940,34 @@ func TestTwoFAAttemptMap_InLockoutRecordNotEvicted(t *testing.T) { // does NOT evict one and does NOT grow past the cap — the new user gets a // transient, untracked state for this request instead. func TestTwoFAAttemptMap_FullOfLockedOut_ReturnsTransient(t *testing.T) { - twoFAAttemptMapMu.Lock() - origMap := twoFAAttemptMap - origCap := twoFAMaxTrackedAttempts - twoFAAttemptMap = make(map[string]*twoFAAttemptState) - twoFAMaxTrackedAttempts = 3 - twoFAAttemptMapMu.Unlock() + twofa.MapMu.Lock() + origMap := twofa.Map + origCap := twofa.MaxTrackedAttempts + twofa.Map = make(map[string]*twoFAAttemptState) + twofa.MaxTrackedAttempts = 3 + twofa.MapMu.Unlock() t.Cleanup(func() { - twoFAAttemptMapMu.Lock() - twoFAAttemptMap = origMap - twoFAMaxTrackedAttempts = origCap - twoFAAttemptMapMu.Unlock() + twofa.MapMu.Lock() + twofa.Map = origMap + twofa.MaxTrackedAttempts = origCap + twofa.MapMu.Unlock() }) now := clock.Now() for i := 0; i < 3; i++ { - st := &twoFAAttemptState{} - st.setLastActive(now.Add(-time.Second)) - st.count.Store(5) - twoFAAttemptMap[fmt.Sprintf("locked_%d", i)] = st + st := &twofa.AttemptState{} + st.SetLastActive(now.Add(-time.Second)) + st.Count.Store(5) + twofa.Map[fmt.Sprintf("locked_%d", i)] = st } st := twoFAAttemptStateFor("new_user") require.NotNil(t, st) - if _, ok := twoFAAttemptMap["new_user"]; ok { + if _, ok := twofa.Map["new_user"]; ok { t.Error("expected the transient state NOT to be stored when the map is full of in-lockout records") } - if len(twoFAAttemptMap) != 3 { - t.Errorf("expected all 3 locked-out records to survive, got %d", len(twoFAAttemptMap)) + if len(twofa.Map) != 3 { + t.Errorf("expected all 3 locked-out records to survive, got %d", len(twofa.Map)) } } @@ -923,17 +984,17 @@ func TestTwoFAAttemptMap_FullOfLockedOut_ReturnsTransient(t *testing.T) { // completes (no deadlock), pre-pinned locked-out records survive the eviction // pressure, and the map never grows past the cap. func TestTwoFAAttemptMap_ConcurrentVerifyAndEviction(t *testing.T) { - twoFAAttemptMapMu.Lock() - origMap := twoFAAttemptMap - origCap := twoFAMaxTrackedAttempts - twoFAAttemptMap = make(map[string]*twoFAAttemptState) - twoFAMaxTrackedAttempts = 128 - twoFAAttemptMapMu.Unlock() + twofa.MapMu.Lock() + origMap := twofa.Map + origCap := twofa.MaxTrackedAttempts + twofa.Map = make(map[string]*twoFAAttemptState) + twofa.MaxTrackedAttempts = 128 + twofa.MapMu.Unlock() t.Cleanup(func() { - twoFAAttemptMapMu.Lock() - twoFAAttemptMap = origMap - twoFAMaxTrackedAttempts = origCap - twoFAAttemptMapMu.Unlock() + twofa.MapMu.Lock() + twofa.Map = origMap + twofa.MaxTrackedAttempts = origCap + twofa.MapMu.Unlock() }) const verifyWorkers = 6 @@ -944,16 +1005,16 @@ func TestTwoFAAttemptMap_ConcurrentVerifyAndEviction(t *testing.T) { // lockout records are never evicted under concurrent pressure. now := clock.Now() victims := make(map[string]*twoFAAttemptState, verifyWorkers) - twoFAAttemptMapMu.Lock() + twofa.MapMu.Lock() for i := 0; i < verifyWorkers; i++ { - st := &twoFAAttemptState{} - st.setLastActive(now.Add(-time.Second)) - st.count.Store(twoFAMaxAttempts) + st := &twofa.AttemptState{} + st.SetLastActive(now.Add(-time.Second)) + st.Count.Store(twoFAMaxAttempts) id := fmt.Sprintf("victim_%d", i) - twoFAAttemptMap[id] = st + twofa.Map[id] = st victims[id] = st } - twoFAAttemptMapMu.Unlock() + twofa.MapMu.Unlock() var wg sync.WaitGroup @@ -966,15 +1027,15 @@ func TestTwoFAAttemptMap_ConcurrentVerifyAndEviction(t *testing.T) { defer wg.Done() for iter := 0; iter < iters; iter++ { st := twoFAAttemptStateFor(fmt.Sprintf("verify_%d_%d", w, iter)) - st.mu.Lock() - if now := clock.Now(); now.Sub(st.lastActive()) > twoFAAttemptWindow { - st.count.Store(0) - st.setLastActive(now) + st.Mu.Lock() + if now := clock.Now(); now.Sub(st.LastActive()) > twoFAAttemptWindow { + st.Count.Store(0) + st.SetLastActive(now) } - _ = st.lockedOut(clock.Now()) - st.count.Add(1) - st.setLastActive(clock.Now()) - st.mu.Unlock() + _ = st.LockedOut(clock.Now()) + st.Count.Add(1) + st.SetLastActive(clock.Now()) + st.Mu.Unlock() } }(w) } @@ -992,18 +1053,18 @@ func TestTwoFAAttemptMap_ConcurrentVerifyAndEviction(t *testing.T) { } wg.Wait() - twoFAAttemptMapMu.Lock() - defer twoFAAttemptMapMu.Unlock() + twofa.MapMu.Lock() + defer twofa.MapMu.Unlock() for id, st := range victims { - if _, ok := twoFAAttemptMap[id]; !ok { + if _, ok := twofa.Map[id]; !ok { t.Errorf("in-window lockout record %s was evicted under concurrent pressure", id) } - if !st.lockedOut(clock.Now()) { + if !st.LockedOut(clock.Now()) { t.Errorf("victim %s must still report locked out", id) } } - if len(twoFAAttemptMap) > twoFAMaxTrackedAttempts { - t.Errorf("map grew past the cap: %d > %d", len(twoFAAttemptMap), twoFAMaxTrackedAttempts) + if len(twofa.Map) > twofa.MaxTrackedAttempts { + t.Errorf("map grew past the cap: %d > %d", len(twofa.Map), twofa.MaxTrackedAttempts) } } @@ -1014,17 +1075,17 @@ func TestTwoFAAttemptMap_ConcurrentVerifyAndEviction(t *testing.T) { // goroutines hammer the map eviction scan through twoFAAttemptStateFor. The // test completes only if no goroutine deadlocks on mapMu/st.mu. func TestTwoFA_ConcurrentCheckTwoFACode_NoDeadlock(t *testing.T) { - twoFAAttemptMapMu.Lock() - origMap := twoFAAttemptMap - origCap := twoFAMaxTrackedAttempts - twoFAAttemptMap = make(map[string]*twoFAAttemptState) - twoFAMaxTrackedAttempts = 64 - twoFAAttemptMapMu.Unlock() + twofa.MapMu.Lock() + origMap := twofa.Map + origCap := twofa.MaxTrackedAttempts + twofa.Map = make(map[string]*twoFAAttemptState) + twofa.MaxTrackedAttempts = 64 + twofa.MapMu.Unlock() t.Cleanup(func() { - twoFAAttemptMapMu.Lock() - twoFAAttemptMap = origMap - twoFAMaxTrackedAttempts = origCap - twoFAAttemptMapMu.Unlock() + twofa.MapMu.Lock() + twofa.Map = origMap + twofa.MaxTrackedAttempts = origCap + twofa.MapMu.Unlock() }) const workers = 6 @@ -1053,9 +1114,9 @@ func TestTwoFA_ConcurrentCheckTwoFACode_NoDeadlock(t *testing.T) { seedPendingTwoFA(t, tctx, tx, userID, "123456") st := twoFAAttemptStateFor(userID) for attempt := 1; attempt <= twoFAMaxAttempts; attempt++ { - st.mu.Lock() + st.Mu.Lock() res, err := checkTwoFACode(httptest.NewRequest(http.MethodPost, "/api/user/2fa/verify", nil).WithContext(tctx), userID, st, "999999") - st.mu.Unlock() + st.Mu.Unlock() if err != nil { errCh <- fmt.Errorf("worker %d iter %d check: %w", w, iter, err) return @@ -1069,7 +1130,7 @@ func TestTwoFA_ConcurrentCheckTwoFACode_NoDeadlock(t *testing.T) { return } } - if !st.lockedOut(clock.Now()) { + if !st.LockedOut(clock.Now()) { errCh <- fmt.Errorf("worker %d iter %d: must be locked out after %d wrong codes", w, iter, twoFAMaxAttempts) return } @@ -1189,3 +1250,38 @@ func TestTwoFADisableCode_UnenforcedStillMints(t *testing.T) { require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&pendingHash)) require.True(t, pendingHash.Valid, "unenforced env must still mint a pending code") } + +// TestTwoFACodeVerifyForUser exercises the exported helper that backs the +// B6/B10 payments gate (the saved-card charge must present a real 2FA +// challenge): correct code → nil, wrong code → twofa.ErrIncorrect, exhausting +// the budget → twofa.ErrLockedOut, and no pending code → twofa.ErrMissingOrExpired. +func TestTwoFACodeVerifyForUser(t *testing.T) { + twofaEnvEnforced(t) + t.Setenv("TWO_FACTOR_PEPPER", "") + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + seedPendingTwoFA(t, ctx, tx, userID, "123456") + + require.NoError(t, VerifyTwoFACodeForUser(ctx, userID, "123456"), "correct code must verify") + + // A correct verify clears the counter, so a second correct code also works. + require.NoError(t, VerifyTwoFACodeForUser(ctx, userID, "123456")) + + // Wrong code → ErrIncorrect. + err = VerifyTwoFACodeForUser(ctx, userID, "999999") + require.ErrorIs(t, err, twofa.ErrIncorrect) + + // Four more wrong codes reach the 5-attempt cap → ErrLockedOut. + for i := 0; i < 4; i++ { + _ = VerifyTwoFACodeForUser(ctx, userID, "999999") + } + err = VerifyTwoFACodeForUser(ctx, userID, "999999") + require.ErrorIs(t, err, twofa.ErrLockedOut, "after 5 consecutive failures the helper must lock out") + + // A user with no pending code → ErrMissingOrExpired. + userID2, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + err = VerifyTwoFACodeForUser(ctx, userID2, "123456") + require.ErrorIs(t, err, twofa.ErrMissingOrExpired) +} diff --git a/backend/handlers/webhooks/square.go b/backend/handlers/webhooks/square.go index 28eeac8..ac09df5 100644 --- a/backend/handlers/webhooks/square.go +++ b/backend/handlers/webhooks/square.go @@ -485,6 +485,14 @@ type squareDisputedPaymentField struct { type squarePaymentPayload struct { ID string `json:"id"` Status string `json:"status"` // "APPROVED", "COMPLETED", "CANCELED", "FAILED", "PENDING" + // ReferenceID / IdempotencyKey / AmountMoney feed the B1-support orphan + // detection (detectOrphanedReplayCharge): Square's Payment object carries + // them, and a sweep-minted duplicate charge preserves the origin row's + // idempotency key / reference_id / amount (the sweep replays the stored + // request verbatim). + ReferenceID string `json:"reference_id"` + IdempotencyKey string `json:"idempotency_key"` + AmountMoney *squareMoneyPayload `json:"amount_money"` } // squareRefundPayload maps the Square Refund (PaymentRefund) fields this app @@ -768,6 +776,175 @@ func markPaymentFailed(ctx context.Context, paymentID string) error { return nil } +// squarePaymentKnown reports whether ANY local payments row carries the Square +// payment id, whatever its status. The pending-only UPDATE in +// handlePaymentUpdated matches zero rows both when no row exists at all and +// when the row already settled (e.g. a completed webhook replay of an +// already-'completed' charge). This existence check distinguishes those two: +// a row existing means the event is a plain no-op replay of a known charge, +// while no row at all is the signature of an ORPHANED sweep-minted duplicate. +// Unlike findPaymentBySquareID (which swallows errors), a DB failure here +// propagates so the caller rejects the event and Square retries. +func squarePaymentKnown(ctx context.Context, squarePaymentID string) (bool, error) { + var one int + err := db.Conn.QueryRow(ctx, + `SELECT 1 FROM payments WHERE square_payment_id = $1 LIMIT 1`, squarePaymentID).Scan(&one) + if errors.Is(err, pgx.ErrNoRows) { + return false, nil + } + if err != nil { + return false, err + } + return true, nil +} + +// findPendingByOrphanKeys locates the pending ORIGIN row of a likely +// sweep-minted duplicate charge: the pending row that shares the replayed +// charge's idempotency key (primary — exact, because Square returns the key on +// the Payment object and payments.idempotency_key is UNIQUE) or, failing that, +// the pending row whose booking/gift-card reference AND amount match the +// payload's reference_id/amount_money (the sweep replays the stored snapshot +// verbatim, preserving both). Only 'pending' rows WITHOUT a square_payment_id +// are candidates — that is exactly the population the keyed stale-pending sweep +// replays (sweep.go), so a match is the sweep-minted duplicate's origin. +func findPendingByOrphanKeys(ctx context.Context, payment squarePaymentPayload) (paymentID, bookingID string, found bool, err error) { + if payment.IdempotencyKey != "" { + var pid string + var bid *string + err := db.Conn.QueryRow(ctx, ` + SELECT id, booking_id FROM payments + WHERE status = 'pending' AND idempotency_key = $1 AND square_payment_id IS NULL + ORDER BY created_at DESC, id DESC + LIMIT 1 + `, payment.IdempotencyKey).Scan(&pid, &bid) + if err == nil { + if bid != nil { + bookingID = *bid + } + return pid, bookingID, true, nil + } + if !errors.Is(err, pgx.ErrNoRows) { + return "", "", false, err + } + } + // reference_id fallback: booking charges carry the booking id as + // reference_id; a gift-card-linked payment carries the card code. The + // amount guard stops a coincidental reference match (a pending row for the + // same booking at a different amount) from being mistaken for the origin. + if payment.ReferenceID != "" && payment.AmountMoney != nil && payment.AmountMoney.Amount > 0 { + amount := float64(payment.AmountMoney.Amount) / 100.0 + var pid string + var bid *string + err := db.Conn.QueryRow(ctx, ` + SELECT id, booking_id FROM payments + WHERE status = 'pending' AND square_payment_id IS NULL + AND ABS(amount - $2) < 0.005 + AND (booking_id = $1 OR gift_card_id = $1) + ORDER BY created_at DESC, id DESC + LIMIT 1 + `, payment.ReferenceID, amount).Scan(&pid, &bid) + if err == nil { + if bid != nil { + bookingID = *bid + } + return pid, bookingID, true, nil + } + if !errors.Is(err, pgx.ErrNoRows) { + return "", "", false, err + } + } + return "", "", false, nil +} + +// orphanReplayChargeNotificationID derives the deterministic admin_notifications +// id for an orphaned sweep-minted duplicate charge's critical_payment_log +// notification: 'O' + 11 lowercase hex chars of a SHA-256 over +// 'orphan-replay-'. Mirrors disputeNotificationID ('D') and +// unknownEventNotificationID ('U'): same prefix + hex(sha256(input))[:11] +// scheme with a distinct uppercase prefix, so the three id spaces can never +// collide and the uppercase prefix guarantees no collision with a DB-generated +// id (generate_*_id emits 12 lowercase hex chars). The id is stable per ORIGIN +// payment row, so ON CONFLICT (id) DO NOTHING keeps re-deliveries and +// re-notifications of the same orphan charge to one row. +func orphanReplayChargeNotificationID(paymentID string) string { + sum := sha256.Sum256([]byte("orphan-replay-" + paymentID)) + return "O" + hex.EncodeToString(sum[:])[:11] +} + +// insertOrphanReplayChargeNotification surfaces an orphaned sweep-minted +// duplicate charge in the admin notification centre (reason +// 'critical_payment_log'), deduped by the deterministic per-origin-row id so +// repeated deliveries of the same orphan charge's event never add a second +// row. booking_id is set when the origin payment row has one, giving the owner +// a booking to act from. Best-effort: an insert failure is logged, never a +// dispatch error. +func insertOrphanReplayChargeNotification(ctx context.Context, paymentID, bookingID string) { + if paymentID == "" { + return + } + id := orphanReplayChargeNotificationID(paymentID) + var bid any + if bookingID != "" { + bid = bookingID + } + tag, err := db.Conn.Exec(ctx, ` + INSERT INTO admin_notifications (id, reason, booking_id, created_at) + VALUES ($1, 'critical_payment_log'::admin_notification_reason, $2, NOW()) + ON CONFLICT (id) DO NOTHING + `, id, bid) + if err != nil { + log.Printf("[SQUARE-WEBHOOK] Failed to insert orphaned-replay admin notification: %v", err) + return + } + if tag.RowsAffected() > 0 { + log.Printf("[SQUARE-WEBHOOK] Inserted orphaned-replay-charge admin notification (origin payment=%s, booking=%q)", paymentID, bookingID) + } +} + +// detectOrphanedReplayCharge handles a COMPLETED payment.updated event whose +// square_payment_id matches NO local payments row — the signature of an +// ORPHANED SWEEP-MINTED duplicate charge (B1-support). When the stale-pending +// sweep replays a pending row's stored idempotency key against Square and the +// key has expired, Square creates a NEW charge under that same key; the new +// charge's payment.updated event arrives here with no local row of its own, +// while the pending ORIGIN row (the one the sweep replayed) still exists and +// shares the replayed idempotency key — and, for booking/gift-card charges, +// the same reference_id and amount. +// +// Action (the webhook half of B1): mark the origin row 'failed' — it is NOT +// the charge that completed, so rescuing it to 'completed' would hide the +// duplicate behind the original and rescuing by square_payment_id is +// impossible (the orphan has no row) — and surface a deduped 'orphaned replay +// charge detected' admin notification. The AUTO-REFUND of the orphan charge is +// the SWEEP's job (sweep.go, the money agent's B1 fix): this path NEVER issues +// a refund and must never be turned into one; it detects + notifies + settles +// the origin row so the sweep does not blind-fail or double-rescue it later. +func detectOrphanedReplayCharge(ctx context.Context, payment squarePaymentPayload) error { + originID, bookingID, found, err := findPendingByOrphanKeys(ctx, payment) + if err != nil { + log.Printf("[SQUARE-WEBHOOK] Orphan-replay origin lookup failed for square payment %s: %v", payment.ID, err) + return err + } + if !found { + log.Printf("[SQUARE-WEBHOOK] payment.updated: COMPLETED square payment %s matches no local row and no pending origin row by idempotency key/reference_id — acknowledging (not a sweep-minted duplicate)", payment.ID) + return nil + } + tag, err := db.Conn.Exec(ctx, + `UPDATE payments SET status = 'failed', updated_at = NOW() WHERE id = $1 AND status = 'pending'`, + originID) + if err != nil { + log.Printf("[SQUARE-WEBHOOK] Failed to mark orphaned-replay origin payment %s failed: %v", originID, err) + return err + } + if tag.RowsAffected() > 0 { + log.Printf("[SQUARE-WEBHOOK] CRITICAL: COMPLETED square payment %s (orphaned sweep-minted duplicate) — origin pending payment %s marked failed; admin notified", payment.ID, originID) + } else { + log.Printf("[SQUARE-WEBHOOK] CRITICAL: COMPLETED square payment %s (orphaned sweep-minted duplicate) — origin payment %s already resolved; admin notified", payment.ID, originID) + } + insertOrphanReplayChargeNotification(ctx, originID, bookingID) + return nil +} + // handlePaymentUpdated reconciles a Square Payment state change against the // local payments row (real-time counterpart to the stale-pending sweep). The // Square id is logged, never the payload (PII). Idempotent: the UPDATE is a @@ -818,6 +995,26 @@ func handlePaymentUpdated(data json.RawMessage) error { } if tag.RowsAffected() > 0 { log.Printf("[SQUARE-WEBHOOK] payment.updated: square payment %s → local status %s", payment.ID, localStatus) + } else if localStatus == "completed" { + // B1-support: a COMPLETED payment.updated that matched NO pending + // 'payments' row may be an ORPHANED SWEEP-MINTED duplicate — the + // stale-pending sweep replayed a stored idempotency key against an + // expired key, Square landed a NEW charge whose id matches nothing + // locally, and this is that new charge's completion event. If NO local + // row carries this square_payment_id at all (not even a settled one), + // hunt for the pending origin row and settle it so the sweep never + // blind-fails or double-rescues it. A settled row existing means this + // is a plain no-op replay of a known charge and is left untouched. + known, kErr := squarePaymentKnown(ctx, payment.ID) + if kErr != nil { + log.Printf("[SQUARE-WEBHOOK] Failed to check whether square payment %s is known: %v", payment.ID, kErr) + return kErr + } + if !known { + if dErr := detectOrphanedReplayCharge(ctx, payment); dErr != nil { + return dErr + } + } } // A Square charge can also map to a till_sales row (online gift-card // purchase, retail at the till) — reconcile those too. Same pending-only diff --git a/backend/handlers/webhooks/webhooks_state_test.go b/backend/handlers/webhooks/webhooks_state_test.go index 9f9b5a6..df122bf 100644 --- a/backend/handlers/webhooks/webhooks_state_test.go +++ b/backend/handlers/webhooks/webhooks_state_test.go @@ -886,6 +886,216 @@ func TestWebhook_PaymentUpdated_IdempotentReplay(t *testing.T) { } } +// ============================================================================= +// B1-support — orphaned sweep-minted duplicate charge (payment.updated with no +// local row that resolves to a pending origin row) +// ============================================================================= + +// createWebhookTestPendingOrigin inserts a pending payments row that carries an +// idempotency key but NO square_payment_id — exactly the population the keyed +// stale-pending sweep replays — and returns its local id. +func createWebhookTestPendingOrigin(t *testing.T, idempotencyKey string) string { + t.Helper() + var id string + err := db.Conn.QueryRow(context.Background(), ` + INSERT INTO payments (payment_type, payment_method, status, amount, idempotency_key, created_at, updated_at) + VALUES ('full', 'online_square', 'pending', 10.00, $1, NOW(), NOW()) + RETURNING id + `, idempotencyKey).Scan(&id) + if err != nil { + t.Fatalf("failed to create pending origin payment: %v", err) + } + return id +} + +func countOrphanReplayNotifications(t *testing.T, originID string) int { + t.Helper() + var n int + if err := db.Conn.QueryRow(context.Background(), + `SELECT COUNT(*) FROM admin_notifications WHERE id = $1 AND reason = 'critical_payment_log'`, + orphanReplayChargeNotificationID(originID)).Scan(&n); err != nil { + t.Fatalf("failed to count orphan-replay notifications: %v", err) + } + return n +} + +// TestWebhook_PaymentUpdated_OrphanedReplay_MarksOriginFailed locks the +// B1-support webhook half: a COMPLETED payment.updated for a charge whose +// square_payment_id matches no local row — the sweep-minted duplicate — finds +// its pending origin row by the replayed idempotency key, marks it failed (not +// rescued), and raises exactly one deduped admin notification. +func TestWebhook_PaymentUpdated_OrphanedReplay_MarksOriginFailed(t *testing.T) { + const ( + orphanSquareID = "sqp_orphan_c2" + idemKey = "b1-orphan-key-001" + ) + originID := createWebhookTestPendingOrigin(t, idemKey) + + event := SquareWebhookEvent{ + Type: "payment.updated", + EventID: "evt_orphan_c2_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "payment", + "id": "` + orphanSquareID + `", + "object": { + "payment": { + "id": "` + orphanSquareID + `", + "status": "COMPLETED", + "idempotency_key": "` + idemKey + `", + "amount_money": {"amount": 1000, "currency": "GBP"} + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + // The origin row must be settled to 'failed' — NOT rescued to 'completed' + // (which would hide the duplicate behind the original charge). + if got := getPaymentStatus(t, originID); got != "failed" { + t.Errorf("expected origin pending payment 'failed', got %q", got) + } + if n := countOrphanReplayNotifications(t, originID); n != 1 { + t.Errorf("expected exactly 1 orphan-replay notification, got %d", n) + } + + // Re-delivery under a FRESH event_id (bypassing the handler's event_id + // dedup) must not add a second notification — the deterministic per-origin + // id dedups (and, by extension, the sweep's auto-refund cannot be + // double-triggered by this path). + event.EventID = "evt_orphan_c2_2" + w2 := deliverWebhook(t, event) + if w2.Code != http.StatusOK { + t.Fatalf("expected 200 on re-delivery, got %d: %s", w2.Code, w2.Body.String()) + } + if n := countOrphanReplayNotifications(t, originID); n != 1 { + t.Errorf("expected notification count to stay 1 after re-delivery, got %d", n) + } +} + +// TestWebhook_PaymentUpdated_OrphanedReplay_NoOrigin_Noop verifies the orphan +// detection is a no-op when no pending origin row matches: the event is +// acknowledged 200 without touching any row or raising a notification. +func TestWebhook_PaymentUpdated_OrphanedReplay_NoOrigin_Noop(t *testing.T) { + const orphanSquareID = "sqp_orphan_noorigin" + before := countCriticalNotifications(t) + + event := SquareWebhookEvent{ + Type: "payment.updated", + EventID: "evt_orphan_noorigin_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "payment", + "id": "` + orphanSquareID + `", + "object": { + "payment": { + "id": "` + orphanSquareID + `", + "status": "COMPLETED", + "idempotency_key": "b1-orphan-key-never-used", + "amount_money": {"amount": 1000, "currency": "GBP"} + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if n := countCriticalNotifications(t) - before; n != 0 { + t.Errorf("expected no new critical notification with no origin match, got %d", n) + } + if got := countWebhookEvents(t, event.EventID); got != 1 { + t.Errorf("expected 1 dedup row, got %d", got) + } +} + +// TestWebhook_PaymentUpdated_OrphanedReplay_ReferenceFallback locks the +// reference_id fallback of the origin lookup: a COMPLETED orphan event whose +// payload carries no idempotency key still finds its pending origin row via the +// replayed reference_id + matching amount. +func TestWebhook_PaymentUpdated_OrphanedReplay_ReferenceFallback(t *testing.T) { + const ( + orphanSquareID = "sqp_orphan_refc2" + refID = "b16bad0000aa" + ) + var originID string + if err := db.Conn.QueryRow(context.Background(), ` + INSERT INTO payments (payment_type, payment_method, status, amount, gift_card_id, created_at, updated_at) + VALUES ('full', 'online_square', 'pending', 10.00, $1, NOW(), NOW()) + RETURNING id + `, refID).Scan(&originID); err != nil { + t.Fatalf("failed to create reference-origin payment: %v", err) + } + + event := SquareWebhookEvent{ + Type: "payment.updated", + EventID: "evt_orphan_refc2_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "payment", + "id": "` + orphanSquareID + `", + "object": { + "payment": { + "id": "` + orphanSquareID + `", + "status": "COMPLETED", + "reference_id": "` + refID + `", + "amount_money": {"amount": 1000, "currency": "GBP"} + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getPaymentStatus(t, originID); got != "failed" { + t.Errorf("expected reference-matched origin payment 'failed', got %q", got) + } + if n := countOrphanReplayNotifications(t, originID); n != 1 { + t.Errorf("expected exactly 1 orphan-replay notification, got %d", n) + } +} + +// TestWebhook_PaymentUpdated_SettledRow_NoOrphanDetection verifies the orphan +// detection NEVER fires for a charge that already has a local row in a settled +// status (a plain payment.updated replay of a known charge): the row is left +// untouched and no notification is raised. +func TestWebhook_PaymentUpdated_SettledRow_NoOrphanDetection(t *testing.T) { + const squarePaymentID = "sqp_settled_replay" + payID := createWebhookTestPayment(t, squarePaymentID, "completed") + before := countCriticalNotifications(t) + + event := SquareWebhookEvent{ + Type: "payment.updated", + EventID: "evt_settled_replay_1", + CreatedAt: "2025-01-01T00:00:00Z", + Data: json.RawMessage(`{ + "type": "payment", + "id": "` + squarePaymentID + `", + "object": { + "payment": { + "id": "` + squarePaymentID + `", + "status": "COMPLETED", + "idempotency_key": "b1-settled-key", + "amount_money": {"amount": 1000, "currency": "GBP"} + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getPaymentStatus(t, payID); got != "completed" { + t.Errorf("expected settled payment to stay 'completed', got %q", got) + } + if n := countCriticalNotifications(t) - before; n != 0 { + t.Errorf("expected no new critical notification for a known settled row, got %d", n) + } +} + // ============================================================================= // State mutation — refund.updated // ============================================================================= diff --git a/backend/internal/twofa/testmain_test.go b/backend/internal/twofa/testmain_test.go new file mode 100644 index 0000000..ca7e8d0 --- /dev/null +++ b/backend/internal/twofa/testmain_test.go @@ -0,0 +1,20 @@ +//go:build test + +package twofa + +import ( + "os" + "testing" + + "crussell/db" + "crussell/testutils/testdb" +) + +func TestMain(m *testing.M) { + pool := testdb.CreateTestDatabase("crussell_test_internal_twofa") + db.Conn = db.NewPoolProxy(pool) + testdb.SeedBaseline(pool) + code := m.Run() + testdb.DestroyTestDatabase(pool, "crussell_test_internal_twofa") + os.Exit(code) +} diff --git a/backend/internal/twofa/twofa.go b/backend/internal/twofa/twofa.go new file mode 100644 index 0000000..72f5731 --- /dev/null +++ b/backend/internal/twofa/twofa.go @@ -0,0 +1,369 @@ +// Package twofa owns the shared 2FA verification-code machinery: the +// per-user brute-force attempt map, the constant-time code check, and the +// exported verification entry point. +// +// Why this package exists (B11c coordination contract): handlers/user imports +// handlers/payments (TwoFactorEnforced, SquareClient), so handlers/payments +// CANNOT import handlers/user — Go would reject the cycle. The saved-card +// charge gate (B6/B10, owned by the payments agent) needs to verify a real 2FA +// challenge with the same brute-force lockout as the interactive endpoints, so +// the verification core lives here, importing neither. +// +// Contract for the payments gate: +// +// err := twofa.VerifyForUser(ctx, userID, code) +// if err != nil { +// switch { +// case errors.Is(err, twofa.ErrIncorrect): +// // 400 +// case errors.Is(err, twofa.ErrLockedOut): +// // 429 +// case errors.Is(err, twofa.ErrMissingOrExpired): +// // 400 — user must request a fresh code +// default: +// // 500 (DB failure) +// } +// } +// +// 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 +// code mint, so minting a new code cannot grant a fresh guessing budget (B11b). +package twofa + +import ( + "context" + "crypto/hmac" + "crypto/sha256" + "crypto/subtle" + "database/sql" + "encoding/hex" + "errors" + "fmt" + "log" + "sync" + "sync/atomic" + "time" + + "crussell/clock" + "crussell/db" +) + +// MaxAttempts is the number of consecutive failed verify attempts allowed +// before the pending code is invalidated and a new one must be requested. +const MaxAttempts = 5 + +// AttemptWindow bounds how long a per-user attempt counter lives before +// resetting, and doubles as the stale-entry eviction horizon for the map. +const AttemptWindow = 10 * time.Minute + +// MaxTrackedAttempts caps the in-memory attempt map so a flood of distinct +// user IDs cannot grow it without bound. Counters are purely in-memory (the DB +// schema is locked — there is no attempt column), so they reset on process +// restart; the 10-minute pending-code expiry bounds the practical impact. +// Declared as a var so the eviction policy is unit-testable at a small cap. +var MaxTrackedAttempts = 10_000 + +// AttemptState tracks consecutive failed verify attempts for one user. The +// per-user mutex serializes the whole verify critical section so concurrent +// attempts from the same user cannot race the limit check. Count and LastAt are +// atomic so the map eviction path can read them without taking the per-user +// mutex (lock ordering forbids MapMu→st.Mu: Check holds st.Mu then takes +// MapMu). LastAt is stored as nanoseconds since the Unix epoch so the eviction +// scan and LockedOut read it race-free even on 32-bit platforms — a plain +// time.Time read/write pair there could tear the 8-byte timestamp and reset or +// extend the lockout window. +// LastMintAt is the mint-cooldown stamp (see twoFAMintCooldown in the user +// package); it is only ever touched under Mu. +type AttemptState struct { + Mu sync.Mutex + Count atomic.Int32 + LastAt atomic.Int64 + LastMintAt time.Time +} + +// LastActive returns the state's last-activity timestamp (nanoseconds since +// the Unix epoch, UTC). Reads are atomic so the map eviction scan can call it +// while holding only MapMu. +func (st *AttemptState) LastActive() time.Time { + return time.Unix(0, st.LastAt.Load()).UTC() +} + +// SetLastActive records a last-activity timestamp. Writes happen under Mu +// (Check) while the eviction scan reads under MapMu only — the atomic store +// makes both race-free. +func (st *AttemptState) SetLastActive(t time.Time) { + st.LastAt.Store(t.UnixNano()) +} + +// LockedOut reports whether the state is inside its lockout window: the attempt +// counter has reached the cap and the window has not yet elapsed. Such a record +// is the rate limit's source of truth for its user and must never be evicted +// while in-window — evicting it would silently reset the counter and grant a +// fresh guessing budget. +func (st *AttemptState) LockedOut(now time.Time) bool { + return st.Count.Load() >= MaxAttempts && now.Sub(st.LastActive()) <= AttemptWindow +} + +var ( + MapMu sync.Mutex + Map = make(map[string]*AttemptState) +) + +// StateFor returns the per-user attempt state, creating it if needed. The map +// is bounded: stale (window-expired) entries are evicted opportunistically and, +// when at capacity, the least-recently-active non-locked-out entry is dropped. +// A record still inside its lockout window is NEVER evicted — evicting it would +// reset the victim's attempt counter and bypass the rate limit under a hostile +// flood of new keys. When the map is full of in-window locked-out records (a +// pathological flood), a transient, untracked state is returned instead of +// growing the map past the cap. +func StateFor(userID string) *AttemptState { + MapMu.Lock() + defer MapMu.Unlock() + now := clock.Now() + + if len(Map) >= MaxTrackedAttempts { + var oldestID string + var oldestAt time.Time + for id, st := range Map { + if now.Sub(st.LastActive()) > AttemptWindow { + // Idle/expired — its counter has already lapsed; safe to evict. + delete(Map, id) + continue + } + if st.LockedOut(now) { + // Inside its lockout window — the rate limit's source of truth + // for this user. Never evict (finding-e fix). + continue + } + if at := st.LastActive(); oldestID == "" || at.Before(oldestAt) { + oldestID, oldestAt = id, at + } + } + if len(Map) >= MaxTrackedAttempts && oldestID != "" { + delete(Map, oldestID) + } + if len(Map) >= MaxTrackedAttempts { + // Every entry is a locked-out in-window record. Do not evict one + // (that would reset its rate limit) and do not grow past the cap: + // return a transient, untracked state so THIS request still + // proceeds under a fresh budget. + st := &AttemptState{} + st.SetLastActive(now) + return st + } + } + + st := Map[userID] + if st == nil { + st = &AttemptState{} + st.SetLastActive(now) + Map[userID] = st + } + return st +} + +// ResetAttempts resets a user's attempt counter in place (count only) WITHOUT +// deleting the entry, preserving LastMintAt so the mint cooldown survives a +// fresh-code delivery. Called on successful verify only — a fresh code mint +// MUST NOT reset the counter, or a password-only attacker could loop +// mint → burn 5 guesses → mint forever (B11b). LastAt is deliberately not +// touched here: it is re-stamped by Check on real activity, and writing it +// under MapMu would race with Check's Mu-guarded write. Lock ordering is +// Mu→MapMu at call sites, never the reverse (StateFor takes MapMu only and +// never takes Mu). +func ResetAttempts(userID string) { + MapMu.Lock() + defer MapMu.Unlock() + if st := Map[userID]; st != nil { + st.Count.Store(0) + } +} + +// DeleteAttempts removes a user's attempt-map entry entirely, unlike +// ResetAttempts which only zeroes the count in place. The admin 2FA removal +// flow uses it so any lingering lockout/counter/mint-cooldown state is dropped +// wholesale and a re-setup starts from a clean slate. +func DeleteAttempts(userID string) { + MapMu.Lock() + defer MapMu.Unlock() + delete(Map, userID) +} + +// PepperProvider supplies the server-side HMAC pepper (TWO_FACTOR_PEPPER). The +// build-dependent behavior — dev/test fallback to the legacy unsalted digest +// with a one-time warning vs production fail-closed — is registered by the +// handlers/user build-tagged files via SetPepperProvider. +var PepperProvider = func() string { return "" } + +// SetPepperProvider registers the build-specific pepper reader. +func SetPepperProvider(f func() string) { PepperProvider = f } + +// Hash returns the hex digest of a verification code as stored in the DB. With +// TWO_FACTOR_PEPPER set the digest is HMAC-SHA256 keyed by the pepper, so a +// leaked digest cannot be brute-forced offline (the key stays server-side). +// When the pepper is unset it falls back to the legacy unsalted SHA-256 digest: +// dev/test builds also log a one-time warning, while production builds can +// never persist such a digest because issuance fails closed without the pepper +// — the fallback survives only for the legacy-row migration window and the +// dev/test loose-fake flow. The plaintext code is never stored. +func Hash(code string) string { + if p := PepperProvider(); p != "" { + mac := hmac.New(sha256.New, []byte(p)) + mac.Write([]byte(code)) + return hex.EncodeToString(mac.Sum(nil)) + } + sum := sha256.Sum256([]byte(code)) + return hex.EncodeToString(sum[:]) +} + +// LegacyHash returns the pre-pepper plain SHA-256 digest, used to verify rows +// written before TWO_FACTOR_PEPPER was provisioned during the migration window +// (see VerifyHash). +func LegacyHash(code string) string { + sum := sha256.Sum256([]byte(code)) + return hex.EncodeToString(sum[:]) +} + +// VerifyHash reports whether reqCode matches a stored pending-code digest, +// always in constant time (subtle.ConstantTimeCompare). The first comparison +// uses the current pepper'd digest; when that fails the stored hash may be a +// legacy pre-pepper plain SHA-256 (rows written before TWO_FACTOR_PEPPER was +// provisioned), so the legacy digest is tried too. When a legacy row matches, +// legacy is true and the caller should re-hash with the pepper on the next +// successful verify, retiring the plain digest. +func VerifyHash(reqCode, storedHash string) (match, legacy bool) { + if subtle.ConstantTimeCompare([]byte(Hash(reqCode)), []byte(storedHash)) == 1 { + return true, false + } + if subtle.ConstantTimeCompare([]byte(LegacyHash(reqCode)), []byte(storedHash)) == 1 { + return true, true + } + return false, false +} + +// Result classifies Check's outcome so callers can map it to the correct HTTP +// status (or error, in VerifyForUser's case). +type Result int + +const ( + OK Result = iota + Incorrect + LockedOut + MissingOrExpired +) + +// Check verifies the submitted code against the user's stored pending code +// under the per-user brute-force lockout. The caller must hold st.Mu (from +// StateFor) so concurrent attempts from the same user cannot race the limit +// 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. 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) (Result, error) { + if now := clock.Now(); now.Sub(st.LastActive()) > AttemptWindow { + st.Count.Store(0) + st.SetLastActive(now) + } + if st.Count.Load() >= MaxAttempts { + return LockedOut, nil + } + + var pendingHash sql.NullString + var pendingExpires sql.NullTime + err := db.Conn.QueryRow(ctx, ` + SELECT two_factor_pending_code_hash, two_factor_pending_code_expires + FROM users + WHERE id = $1 + `, userID).Scan(&pendingHash, &pendingExpires) + if err != nil { + return LockedOut, err + } + if !pendingHash.Valid || !pendingExpires.Valid || !pendingExpires.Time.After(clock.Now()) { + return MissingOrExpired, nil + } + // Constant-time compare (subtle) so a wrong code's match position cannot be + // inferred from response timing. Both digests are fixed-length hex. Legacy + // pre-pepper rows (plain SHA-256, hashed before TWO_FACTOR_PEPPER existed) + // still verify during the transition window. + match, legacy := VerifyHash(reqCode, pendingHash.String) + if !match { + st.Count.Add(1) + st.SetLastActive(clock.Now()) + if st.Count.Load() >= MaxAttempts { + // Lockout reached: destroy the pending code so a stolen digest + // cannot be replayed against a fresh guessing loop. + if _, err := db.Conn.Exec(ctx, ` + UPDATE users + SET two_factor_pending_code_hash = NULL, + two_factor_pending_code_expires = NULL + WHERE id = $1 + `, userID); err != nil { + log.Printf("failed to invalidate 2FA pending code for user %s: %v", userID, err) + } + return LockedOut, nil + } + return Incorrect, nil + } + + // Success: a legacy (pre-pepper) hash that verified is re-hashed with the + // pepper so the plain digest is retired on the next successful verify. + if legacy { + if _, err := db.Conn.Exec(ctx, ` + UPDATE users + SET two_factor_pending_code_hash = $2 + WHERE id = $1 + `, userID, Hash(reqCode)); err != nil { + log.Printf("failed to upgrade legacy 2FA pending code hash for user %s: %v", userID, err) + } + } + // Success: clear the attempt counter (and any mint cooldown) before the + // caller performs its action. + st.Count.Store(0) + st.SetLastActive(clock.Now()) + st.LastMintAt = time.Time{} + ResetAttempts(userID) + return OK, nil +} + +// Classifying errors returned by VerifyForUser. +var ( + // ErrIncorrect reports a code that does not match the user's pending code. + ErrIncorrect = errors.New("2FA code is incorrect") + // ErrLockedOut reports that the user has exhausted the failed-attempt + // budget; further attempts must wait for the attempt window to elapse. + ErrLockedOut = errors.New("2FA code locked out: too many failed attempts") + // ErrMissingOrExpired reports that no valid pending code exists for the + // user; a fresh code must be requested first. + ErrMissingOrExpired = errors.New("2FA code is missing or has expired") +) + +// VerifyForUser verifies a 2FA code for a user outside the HTTP handler layer, +// under the same per-user brute-force lockout as the interactive endpoints. +// 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. +func VerifyForUser(ctx context.Context, userID, code string) error { + st := StateFor(userID) + st.Mu.Lock() + defer st.Mu.Unlock() + + result, err := Check(ctx, userID, st, code) + if err != nil { + return fmt.Errorf("2FA verify: %w", err) + } + switch result { + case OK: + return nil + case Incorrect: + return ErrIncorrect + case LockedOut: + return ErrLockedOut + case MissingOrExpired: + return ErrMissingOrExpired + } + return nil +} diff --git a/backend/internal/twofa/twofa_test.go b/backend/internal/twofa/twofa_test.go new file mode 100644 index 0000000..20515d1 --- /dev/null +++ b/backend/internal/twofa/twofa_test.go @@ -0,0 +1,91 @@ +//go:build test + +package twofa + +// Tests for the shared 2FA verification core (the package the payments +// card-access gate — B6/B10 — imports for real-challenge verification). +// The pepper provider is never registered here (handlers/user's build-tagged +// files register it), so Hash falls back to the legacy plain SHA-256 digest — +// which is exactly what the seeded pending-code hashes use. + +import ( + "context" + "testing" + "time" + + "crussell/clock" + "crussell/db" + "crussell/testutils" + "crussell/testutils/fixtures" + + "github.com/stretchr/testify/require" +) + +func seedPending(t *testing.T, ctx context.Context, tx db.Querier, userID, code string) { + t.Helper() + _, err := tx.Exec(ctx, `UPDATE users + SET two_factor_method = 'email', + two_factor_pending_code_hash = $2, + two_factor_pending_code_expires = $3 + WHERE id = $1`, userID, Hash(code), clock.Now().Add(10*time.Minute)) + require.NoError(t, err) +} + +func TestVerifyForUser_CorrectAndWrongCode(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + seedPending(t, ctx, tx, userID, "123456") + + require.NoError(t, VerifyForUser(ctx, userID, "123456"), "correct code must verify") + require.ErrorIs(t, VerifyForUser(ctx, userID, "999999"), ErrIncorrect) +} + +func TestVerifyForUser_LockoutAndMissing(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + seedPending(t, ctx, tx, userID, "123456") + + // Wrong code #1 → ErrIncorrect; four more reach the 5-attempt cap. + require.ErrorIs(t, VerifyForUser(ctx, userID, "999999"), ErrIncorrect) + for i := 0; i < 4; i++ { + _ = VerifyForUser(ctx, userID, "999999") + } + require.ErrorIs(t, VerifyForUser(ctx, userID, "999999"), ErrLockedOut) + + // A fresh user with no pending code → ErrMissingOrExpired. + userID2, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + require.ErrorIs(t, VerifyForUser(ctx, userID2, "123456"), ErrMissingOrExpired) +} + +// 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. +func TestVerifyForUser_AttemptStateMapPersists(t *testing.T) { + t.Cleanup(func() { + MapMu.Lock() + Map = make(map[string]*AttemptState) + MaxTrackedAttempts = 10_000 + MapMu.Unlock() + }) + + MapMu.Lock() + Map = make(map[string]*AttemptState) + MaxTrackedAttempts = 2 + MapMu.Unlock() + + // Fill the map with locked-out records; a new key must NOT evict one. + now := clock.Now() + for _, id := range []string{"victim_a", "victim_b"} { + st := &AttemptState{} + st.SetLastActive(now) + st.Count.Store(MaxAttempts) + Map[id] = st + } + _ = StateFor("new_user") // transient, untracked (map full of lockouts) + MapMu.Lock() + defer MapMu.Unlock() + require.Len(t, Map, 2, "locked-out records must survive the cap pressure") +} diff --git a/backend/main.go b/backend/main.go index 8202271..ae36e7a 100644 --- a/backend/main.go +++ b/backend/main.go @@ -92,8 +92,8 @@ var weakJWTSecretValues = []string{ "super-secret", } -// isWeakJWTSecret reports whether a JWT_SECRET_KEY is a known placeholder or -// shorter than the minimum safe length. +// isWeakJWTSecret reports whether a JWT_SECRET_KEY is a known placeholder, +// shorter than the minimum safe length, or lacks sufficient entropy. func isWeakJWTSecret(secret string) bool { trimmed := strings.TrimSpace(strings.ToLower(secret)) if len(trimmed) < minJWTSecretBytes { @@ -104,7 +104,19 @@ func isWeakJWTSecret(secret string) bool { return true } } - return false + // Entropy gate (B15): at least 8 distinct bytes. All-same-char and + // tiny-alphabet secrets ("aaaa...", "0000...", "abcdabcdabcd...") pass the + // length and blacklist checks but have trivial key space — HS256 keys need + // meaningful entropy, not just length. A strong random secret (even pure + // hex, which can use at most 16 distinct chars) clears the 8-byte bar. + seen := make(map[byte]struct{}, 8) + for i := 0; i < len(trimmed); i++ { + seen[trimmed[i]] = struct{}{} + if len(seen) >= 8 { + return false + } + } + return len(seen) < 8 } func limitBody(limit int64) func(http.Handler) http.Handler { @@ -213,6 +225,7 @@ func initSquare() { checkSnapshotEncKey() checkProxyRateLimitConfig() + checkWebhookSignatureKey() } // checkSnapshotEncKey validates SNAPSHOT_ENC_KEY at startup in non-mock @@ -265,6 +278,30 @@ func checkProxyRateLimitConfig() { log.Printf("WARNING: TRUST_PROXY_HEADERS is unset/false with SQUARE_ENVIRONMENT=%q (not a dev/mock value) — behind a trusted proxy (e.g. the nginx in compose.yml) every per-IP rate-limit key uses the proxy's RemoteAddr, collapsing all rate limiters to ONE global budget that any single client can exhaust for everyone. Set TRUST_PROXY_HEADERS=true when a trusted proxy sits in front and overwrites X-Real-IP/CF-Connecting-IP; keep it false only when the backend is origin-exposed.", os.Getenv("SQUARE_ENVIRONMENT")) } +// checkWebhookSignatureKey validates SQUARE_WEBHOOK_SIGNATURE_KEY at startup in +// non-mock deployments. Square webhooks are HMAC-signed and the handler rejects +// unsigned/mis-signed events fail-closed (503 without the key, 403 on a bad +// signature), so an empty key silently disables the only integration path that +// reconciles payments and refunds from Square. Mirroring the fail-fast +// JWT_SECRET_KEY check (main.go init): when an operator HAS configured a +// notification URL (they clearly intend to receive webhooks) but left the +// signing key empty, startup FAILS — discovering mid-run that every event is +// rejected would strand payment reconciliation. When the URL is also unset +// (webhooks not in use — the documented optional setup) a loud warning is +// logged instead, so the fail-fast never breaks webhook-less deployments. +func checkWebhookSignatureKey() { + if payments.IsExplicitDevOrMockEnv() { + return + } + if os.Getenv("SQUARE_WEBHOOK_SIGNATURE_KEY") != "" { + return + } + if os.Getenv("SQUARE_WEBHOOK_NOTIFICATION_URL") != "" { + log.Fatalf("FATAL: SQUARE_WEBHOOK_SIGNATURE_KEY environment variable not set with SQUARE_ENVIRONMENT=%q (non-mock) while SQUARE_WEBHOOK_NOTIFICATION_URL IS set — Square webhook events (payment/refund reconciliation) would be rejected fail-closed at runtime. Generate the signing key in the Square Dashboard webhook subscription and set it in .env.", os.Getenv("SQUARE_ENVIRONMENT")) + } + log.Printf("CRITICAL: SQUARE_WEBHOOK_SIGNATURE_KEY is not set with SQUARE_ENVIRONMENT=%q (non-mock) and SQUARE_WEBHOOK_NOTIFICATION_URL is unset — Square webhooks are not configured; any webhook event Square sends will be rejected (503, fail-closed). Set both in .env if you rely on webhook payment/refund reconciliation.", os.Getenv("SQUARE_ENVIRONMENT")) +} + func healthCheckHandler(w http.ResponseWriter, r *http.Request) { status := "ok" services := map[string]string{ @@ -468,9 +505,16 @@ func main() { r.Use(mw.OptionalAuth) r.Get("/services", services.ServicesHandler) r.Get("/services/popular", services.PopularServicesHandler) - r.Get("/services/eligible-for/{user_id}", services.ServicesEligibleForUserHandler) }) + // Per-user eligibility (B4): requires authentication, and the handler + // itself enforces owner-or-admin. The user-agnostic /api/services + // stays public; the per-user variant exposes DOB-derived age + patch + // test status, so an arbitrary user_id must never be queryable + // unauthenticated (the frontend calls it only with the current user's + // ID, or via the admin booking flows). + r.With(mw.RateLimit(120, time.Minute), mw.RequireAuth).Get("/services/eligible-for/{user_id}", services.ServicesEligibleForUserHandler) + // Registration: 10/min to prevent spam + progressive per-IP backoff r.With(mw.ProgressiveRateLimit, mw.RateLimit(10, time.Minute), limitBody(defaultBodyLimit)).Post("/register", authHandlers.RegisterHandler) @@ -553,14 +597,19 @@ func main() { // Public email check (used by BookingFlow for proactive registered-email detection) r.With(mw.RateLimit(60, time.Minute)).Get("/check-email", user.CheckEmailHandler) + // Refresh-token exchange (B5): the client presents the opaque refresh + // token in the Authorization header (Bearer). This MUST be outside the + // RequireAuth group — the refresh token is NOT a JWT, so RequireAuth + // would reject it. The handler itself validates + rotates the refresh + // token and mints a fresh access-token/refresh-token pair. + r.With(mw.RateLimit(10, time.Minute)).Post("/refresh-token", authHandlers.RefreshTokenHandler) + // Authenticated users r.Group(func(r chi.Router) { r.Use(mw.RequireAuth) r.Use(mw.RateLimit(120, time.Minute)) r.Use(limitBody(defaultBodyLimit)) - r.Post("/refresh-token", authHandlers.RefreshTokenHandler) - r.Get("/user/profile", user.GetProfileHandler) r.Put("/user/profile", user.UpdateProfileHandler) r.Put("/user/change-password", user.ChangePasswordHandler) @@ -574,19 +623,19 @@ func main() { // RequireNonGuest: any logged-in user who could save cards must be // able to reach these, not just verified accounts. r.With(mw.RequireNonGuest).Get("/user/2fa/status", user.GetTwoFAStatusHandler) - // The code-issuing/verifying endpoints get a dedicated per-user+IP + // The code-issuing/verifying endpoints get a dedicated per-user // limiter (10/min) on top of the group's generic 120/min limiter: // the 6-digit codes live in a 1M space, so a single user must not be // able to hammer setup/verify/disable faster than the per-user - // 5-attempt lockout can trip. The key combines the authenticated - // userID with the client IP: even behind a proxy that does not set - // TRUST_PROXY_HEADERS=true (so every request's RemoteAddr is the - // proxy's IP), the budget stays per-account — one account holder can - // never exhaust a shared GLOBAL bucket that 429s the entire 2FA - // surface (setup/verify/disable, and thus saved-card payments) for - // everyone. One shared limiter for all four so the whole 2FA surface - // counts against a single per-user budget. - twoFALimiter := mw.RateLimitByUserAndIP(10, time.Minute) + // 5-attempt lockout can trip. The key is the authenticated userID + // ALONE (RateLimitByUser) — NOT user+IP (B8): with the IP in the + // key, a client that can rotate its source IP (or that sits behind + // a proxy echoing a client-supplied CF-Connecting-IP when + // TRUST_PROXY_HEADERS=true) mints a fresh bucket per IP for the + // same account, collapsing the per-account budget. One shared + // limiter for all four so the whole 2FA surface counts against a + // single per-user budget. + twoFALimiter := mw.RateLimitByUser(10, time.Minute) r.With(mw.RequireNonGuest, twoFALimiter).Post("/user/2fa/setup", user.SetupTwoFAHandler) r.With(mw.RequireNonGuest, twoFALimiter).Post("/user/2fa/verify", user.VerifyTwoFAHandler) r.With(mw.RequireNonGuest, twoFALimiter).Post("/user/2fa/disable", user.DisableTwoFAHandler) @@ -626,7 +675,12 @@ func main() { Get("/bookings/{id}/discount-preview", payments.GetDiscountPreviewHandler) // User gift card routes - r.With(mw.RequireNonGuest).Post("/user/giftcards/redeem", payments.RedeemGiftCard) + // Dedicated redeem limiter (B16): a gift-card code lives in the + // 12-hex space, so redemption is brute-forceable. The redeem route + // gets a per-user 10/min budget (RateLimitByUser — one bucket per + // account regardless of IP rotation) on top of the group's generic + // 120/min limiter. + r.With(mw.RequireNonGuest, mw.RateLimitByUser(10, time.Minute)).Post("/user/giftcards/redeem", payments.RedeemGiftCard) r.Get("/user/giftcards/balance", payments.GetGiftCardBalance) r.With(mw.RequireNonGuest).Post("/user/giftcards/buy", payments.BuyGiftCard) // 14-day cooling-off right to cancel online gift-card purchases diff --git a/backend/main_test.go b/backend/main_test.go index df19022..1b9feba 100644 --- a/backend/main_test.go +++ b/backend/main_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" "crussell/db" @@ -262,3 +263,30 @@ func TestIsWeakJWTSecret_CaseAndWhitespaceInsensitive(t *testing.T) { t.Errorf("expected a whitespace-padded strong secret to be accepted") } } + +// ============================================================ +// isWeakJWTSecret entropy gate (B15) +// ============================================================ + +// TestIsWeakJWTSecret_EntropyGate verifies the B15 fix: secrets that clear the +// length and blacklist checks but lack entropy are rejected. An all-same-char +// and an all-zero 32+ char secret each have a 1-byte alphabet — trivially +// brute-forceable despite meeting the length requirement — while a strong +// random secret with many distinct bytes is accepted. +func TestIsWeakJWTSecret_EntropyGate(t *testing.T) { + weak := []string{ + strings.Repeat("a", 32), // all-same-char: 1 distinct byte + strings.Repeat("0", 32), // all-zero: 1 distinct byte + strings.Repeat("ab", 16), // 2 distinct bytes + strings.Repeat("abcd", 8), // 4 distinct bytes + "abcdefghijklmnopqrstuvwxyz123456", // many distinct, strong — accepted + } + for i := 0; i < len(weak)-1; i++ { + if !isWeakJWTSecret(weak[i]) { + t.Errorf("expected low-entropy secret %q to be rejected", weak[i]) + } + } + if isWeakJWTSecret(weak[len(weak)-1]) { + t.Errorf("expected high-entropy secret %q to be accepted", weak[len(weak)-1]) + } +} diff --git a/backend/mw/ratelimit.go b/backend/mw/ratelimit.go index ca0ec40..5090978 100644 --- a/backend/mw/ratelimit.go +++ b/backend/mw/ratelimit.go @@ -5,11 +5,8 @@ package mw import ( "crussell/clock" "fmt" - "net" "net/http" "time" - - "github.com/go-chi/chi/v5/middleware" ) func NewRateLimiter(limit int, window time.Duration) *RateLimiter { @@ -140,17 +137,17 @@ func RateLimit(limit int, window time.Duration) func(http.Handler) http.Handler // key combines the authenticated userID (mw.UserIDKey, injected by RequireAuth) // with the derived client IP, so a per-IP budget can never collapse into a // single GLOBAL bucket when the backend sits behind a proxy that does not set -// TRUST_PROXY_HEADERS=true: without that flag clientIP() keys every request on +// TRUST_PROXY_HEADERS=true: without that flag ClientIP keys every request on // RemoteAddr = the proxy's IP, so one account holder could otherwise exhaust // the shared budget and permanently 429 the whole surface for everyone. With // the userID in the key each account gets its own independent budget per IP. // When no userID is present (unauthenticated path) the key falls back to -// clientIP alone, matching RateLimit's behaviour. +// ClientIP alone, matching RateLimit's behaviour. func RateLimitByUserAndIP(limit int, window time.Duration) func(http.Handler) http.Handler { limiter := NewRateLimiter(limit, window) return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - key := clientIP(r) + key := ClientIP(r) if userID, ok := GetUserID(r.Context()); ok && userID != "" { key = userID + "|" + key } @@ -165,30 +162,37 @@ func RateLimitByUserAndIP(limit int, window time.Duration) func(http.Handler) ht } } -// clientIP derives the per-client rate-limit key. Priority: -// 1. CF-Connecting-IP header — honored ONLY when TRUST_PROXY_HEADERS=true -// (see trustProxyHeaders). A trusted edge (Cloudflare, or nginx whose -// real_ip module validated it against the set_real_ip_from ranges) has -// already overwritten it with the real client IP, so it is unspoofable -// there. Ignored by default because an origin-exposed backend must never -// trust a client-controlled value. -// 2. middleware.GetClientIP(r.Context()) — the X-Real-IP value nginx sets -// from $remote_addr, captured by middleware.ClientIPFromHeader("X-Real-IP") -// in main.go. That middleware is registered only when -// TRUST_PROXY_HEADERS=true, so it too is trusted solely behind a proxy. -// 3. net.SplitHostPort(r.RemoteAddr) / r.RemoteAddr fallback — the actual -// TCP peer; the only key source usable when the backend is origin-exposed. -func clientIP(r *http.Request) string { - if trustProxyHeaders { - if ip := r.Header.Get("CF-Connecting-IP"); ip != "" { - return ip - } +// RateLimitByUser limits requests per authenticated user ID ALONE, dropping the +// IP component entirely. This is the B8 safeguard for the 2FA surface (and the +// B16 gift-card redeem budget): when the IP is part of the key, a client that +// can rotate its source IP — or that sits behind a proxy which echoes a +// client-supplied CF-Connecting-IP when TRUST_PROXY_HEADERS is misconfigured +// true — mints a fresh bucket per IP for the SAME account, collapsing the +// per-account budget. Keying on the userID alone guarantees exactly one budget +// per account regardless of IP rotation or proxy configuration. When no userID +// is present (unauthenticated path) the key falls back to ClientIP so the +// surface still has a default budget. +func RateLimitByUser(limit int, window time.Duration) func(http.Handler) http.Handler { + limiter := NewRateLimiter(limit, window) + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + key, ok := GetUserID(r.Context()) + if !ok || key == "" { + key = ClientIP(r) + } + + if !limiter.Allow(key) { + RespondJSON(w, http.StatusTooManyRequests, map[string]string{"error": "Rate limit exceeded"}) + return + } + + next.ServeHTTP(w, r) + }) } - if ip := middleware.GetClientIP(r.Context()); ip != "" { - return ip - } - if ip, _, err := net.SplitHostPort(r.RemoteAddr); err == nil && ip != "" { - return ip - } - return r.RemoteAddr } + +// clientIP derives the per-client rate-limit key. It is a thin alias of the +// exported ClientIP (which lives in ratelimit_shared.go so it is available in +// every build configuration), kept for backward compatibility with existing +// callers. +func clientIP(r *http.Request) string { return ClientIP(r) } diff --git a/backend/mw/ratelimit_dev.go b/backend/mw/ratelimit_dev.go index 147f832..4eb8a2d 100644 --- a/backend/mw/ratelimit_dev.go +++ b/backend/mw/ratelimit_dev.go @@ -46,3 +46,13 @@ func RateLimitByUserAndIP(limit int, window time.Duration) func(http.Handler) ht }) } } + +// RateLimitByUser is the dev-build no-op twin of the production user-keyed +// limiter in ratelimit.go (see there for the B8 rationale). +func RateLimitByUser(limit int, window time.Duration) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + next.ServeHTTP(w, r) + }) + } +} diff --git a/backend/mw/ratelimit_shared.go b/backend/mw/ratelimit_shared.go index 2428895..00d5055 100644 --- a/backend/mw/ratelimit_shared.go +++ b/backend/mw/ratelimit_shared.go @@ -6,10 +6,14 @@ package mw import ( "context" "crussell/clock" + "net" + "net/http" "os" "strconv" "sync" "time" + + "github.com/go-chi/chi/v5/middleware" ) // trustProxyHeaders gates clientIP()'s use of the proxy-set client-IP headers: @@ -40,10 +44,41 @@ var trustProxyHeaders = func() bool { // CF-Connecting-IP) are honored by the rate limiter. main.go uses it to gate // middleware.ClientIPFromHeader("X-Real-IP") on the same flag, so an // origin-exposed backend never registers a middleware that would let a client -// forge its own rate-limit key. Both the header trust in clientIP() and the +// forge its own rate-limit key. Both the header trust in ClientIP and the // middleware registration read this single source of truth. func TrustProxyHeaders() bool { return trustProxyHeaders } +// ClientIP derives the per-client IP for security-sensitive handlers that need +// a client-address key (reservation ipHash, per-IP audit trails) using the +// SAME gated resolution as the rate limiter. Priority: +// +// 1. CF-Connecting-IP header — honored ONLY when TRUST_PROXY_HEADERS=true +// (see trustProxyHeaders). A trusted edge (Cloudflare, or nginx whose +// real_ip module validated it against the set_real_ip_from ranges) has +// already overwritten it with the real client IP, so it is unspoofable +// there. Ignored by default because an origin-exposed backend must never +// trust a client-controlled value (B7). +// 2. middleware.GetClientIP(r.Context()) — the X-Real-IP value nginx sets +// from $remote_addr, captured by middleware.ClientIPFromHeader("X-Real-IP") +// in main.go. That middleware is registered only when +// TRUST_PROXY_HEADERS=true, so it too is trusted solely behind a proxy. +// 3. net.SplitHostPort(r.RemoteAddr) / r.RemoteAddr fallback — the actual +// TCP peer; the only key source usable when the backend is origin-exposed. +func ClientIP(r *http.Request) string { + if trustProxyHeaders { + if ip := r.Header.Get("CF-Connecting-IP"); ip != "" { + return ip + } + } + if ip := middleware.GetClientIP(r.Context()); ip != "" { + return ip + } + if ip, _, err := net.SplitHostPort(r.RemoteAddr); err == nil && ip != "" { + return ip + } + return r.RemoteAddr +} + // RateLimiter implements a simple in-memory rate limiter type RateLimiter struct { requests map[string][]time.Time diff --git a/backend/mw/ratelimit_test.go b/backend/mw/ratelimit_test.go index 27b072c..c26af9f 100644 --- a/backend/mw/ratelimit_test.go +++ b/backend/mw/ratelimit_test.go @@ -489,6 +489,74 @@ func TestRateLimitByUserAndIP_UnauthenticatedFallsBackToIP(t *testing.T) { } } +// ============================================================ +// RateLimitByUser — user-keyed middleware (B8: the 2FA + gift-card +// redeem budgets must survive IP rotation / header spoofing) +// ============================================================ + +// newRateLimitByUserTestHandler builds a RateLimitByUser-wrapped handler that +// records how many times the inner handler was reached. +func newRateLimitByUserTestHandler(limit int, window time.Duration) (http.Handler, *int) { + calls := 0 + handler := RateLimitByUser(limit, window)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + w.WriteHeader(http.StatusOK) + })) + return handler, &calls +} + +// TestRateLimitByUser_OneBudgetPerUserRegardlessOfIP verifies the B8 fix: the +// 2FA/gift-card-redeem budget keys on the userID ALONE, so an attacker who +// rotates the client IP (or spoofs CF-Connecting-IP behind a misconfigured +// TRUST_PROXY_HEADERS=true proxy) cannot mint a fresh bucket per IP for the +// same account. One user exhausting their budget is limited even from a brand +// new IP, while a DIFFERENT user keeps an independent budget. +func TestRateLimitByUser_OneBudgetPerUserRegardlessOfIP(t *testing.T) { + handler, calls := newRateLimitByUserTestHandler(2, time.Minute) + + // User A exhausts its 2/min budget from IP1... + for i := 0; i < 2; i++ { + if w := serveRateLimitUserRequest(t, handler, "user-a", "198.51.100.1:1234"); w.Code != http.StatusOK { + t.Fatalf("A request %d: expected 200, got %d", i+1, w.Code) + } + } + // ...and is STILL limited from IP2 — the IP component is not part of the + // key, so rotating it cannot mint a fresh bucket (the B8 bypass). + if w := serveRateLimitUserRequest(t, handler, "user-a", "198.51.100.2:1234"); w.Code != http.StatusTooManyRequests { + t.Errorf("expected user A to stay limited after rotating IP, got %d", w.Code) + } + + // A different user keeps its own full allowance even from the SAME IPs. + for i := 0; i < 2; i++ { + if w := serveRateLimitUserRequest(t, handler, "user-b", "198.51.100.1:1234"); w.Code != http.StatusOK { + t.Fatalf("B request %d: expected 200 (independent user bucket), got %d", i+1, w.Code) + } + } + if w := serveRateLimitUserRequest(t, handler, "user-b", "198.51.100.2:1234"); w.Code != http.StatusTooManyRequests { + t.Errorf("expected user B to be limited only after ITS OWN burst, got %d", w.Code) + } + if *calls != 4 { + t.Errorf("expected exactly 4 handler calls (2 per user), got %d", *calls) + } +} + +// TestRateLimitByUser_UnauthenticatedFallsBackToIP verifies the fallback: with +// no userID in context the key is the client IP alone, so the middleware stays +// safe on unauthenticated paths. +func TestRateLimitByUser_UnauthenticatedFallsBackToIP(t *testing.T) { + handler, _ := newRateLimitByUserTestHandler(1, time.Minute) + + if w := serveRateLimitUserRequest(t, handler, "", "198.51.100.30:1234"); w.Code != http.StatusOK { + t.Fatalf("expected 200 for the first request, got %d", w.Code) + } + if w := serveRateLimitUserRequest(t, handler, "", "198.51.100.30:1234"); w.Code != http.StatusTooManyRequests { + t.Errorf("expected a second unauthenticated request from the same IP to be limited, got %d", w.Code) + } + if w := serveRateLimitUserRequest(t, handler, "", "198.51.100.31:1234"); w.Code != http.StatusOK { + t.Errorf("expected a different IP to keep its own bucket, got %d", w.Code) + } +} + // ============================================================ // ProgressiveRateLimiter.Check — dual-window progressive delay // algorithm (batch-1 fix regression) diff --git a/frontend/src/lib/components/admin/TillPurchases.svelte b/frontend/src/lib/components/admin/TillPurchases.svelte index 2791a95..cdb67bb 100644 --- a/frontend/src/lib/components/admin/TillPurchases.svelte +++ b/frontend/src/lib/components/admin/TillPurchases.svelte @@ -7,9 +7,13 @@ import { extractErrorMessage } from '$lib/utils/toast-safe'; import { apiFetch } from '$lib/utils/api'; import SquareCardInput from '$lib/components/payments/SquareCardInput.svelte'; - import { isSquareConfigured, submitPaymentWithRetry } from '$lib/square/square'; + import TwoFactorCodeInput from '$lib/components/payments/TwoFactorCodeInput.svelte'; + import { + isSquareConfigured, + isTwoFactorVerificationGateFailure, + submitPaymentWithRetry + } from '$lib/square/square'; import { authStore } from '$lib/stores/auth.svelte'; - import { resolve } from '$app/paths'; type CartItem = { id: string; @@ -103,19 +107,31 @@ }) ); - // PSD2 SCA stand-in: 2FA required but not enabled blocks charging a - // customer's saved card online. Cash, card machine, and online (new-card - // nonce) payments are unaffected. - const twoFactorBlocksSavedCards = $derived( - !!authStore.currentUser?.twoFactorRequired && !authStore.currentUser?.twoFactorEnabled + // B6/B10: charging a customer's saved card via the till requires the + // customer's current 2FA verification code when the backend enforces the + // gate. The backend keys on the CARD OWNER (not the admin), so the input is + // surfaced whenever the gate is enforced — the operator relays the + // customer's code. Cash, card machine, and online (new-card nonce) payments + // are unaffected. + const savedCardChargeRequires2FACode = $derived(authStore.savedCardChargeRequires2FACode); + + // B6/B10: verification code for the customer's saved-card till charge, + // collected on the saved-card payment screen. Kept populated across retries + // so an invalid/expired code can be corrected without re-typing it. The + // admin always supplies the CUSTOMER's code — the admin's own 2FA flag is + // irrelevant to the backend gate. + let twoFactorCode = $state(''); + // Set true when a charge 403s for a missing code — reveals the input even + // if the session user's flag is unset. + let reveal2FACodeInput = $state(false); + const show2FACodeInput = $derived( + reveal2FACodeInput || (savedCardChargeRequires2FACode && paymentMethod === 'saved_card') ); + const missing2FACode = $derived(show2FACodeInput && twoFactorCode.trim() === ''); // The saved-card option is hidden outright unless a customer is selected - // AND has at least one currently-valid card on file AND 2FA gating is not - // active. - const showSavedCardOption = $derived( - selectedCustomer !== null && validCards.length > 0 && !twoFactorBlocksSavedCards - ); + // AND has at least one currently-valid card on file. + const showSavedCardOption = $derived(selectedCustomer !== null && validCards.length > 0); const availablePaymentMethods = $derived( PAYMENT_METHODS.filter((m) => m.key !== 'saved_card' || showSavedCardOption) @@ -285,10 +301,6 @@ ); return; } - if (paymentMethod === 'saved_card' && twoFactorBlocksSavedCards) { - toast.error('Two-factor authentication is required to use online card payments'); - return; - } if (paymentMethod === 'saved_card' && (!selectedCustomer || !selectedSavedCardId)) { toast.error('Select a customer and a saved card before charging'); return; @@ -296,6 +308,7 @@ isProcessingPaymentSync = true; processing = true; paymentError = null; + let responseStatus = 0; try { // One sale per cart line × quantity — each till sale funds its own // gift card (the backend only accepts item_type 'gift_card'). @@ -312,6 +325,9 @@ if (paymentMethod === 'saved_card') { body.user_id = selectedCustomer?.id; body.user_saved_card_id = selectedSavedCardId; + // B6/B10: the backend requires the CARD OWNER's current 2FA + // verification code when the gate is enforced. + if (show2FACodeInput) body.verification_code = twoFactorCode; } else if (paymentMethod === 'online_square') { if (!onlineSquareCardInput) { throw new Error('Card form is not ready — please wait a moment and try again'); @@ -338,6 +354,7 @@ }) ); if (!res.ok) { + responseStatus = res.status; const errText = await res.text(); throw new Error(extractErrorMessage(errText) || 'Till sale failed'); } @@ -350,8 +367,14 @@ toast.success('Sale complete'); cart = []; idempotencyKeys.clear(); + twoFactorCode = ''; + reveal2FACodeInput = false; } catch (err) { const msg = err instanceof Error ? err.message : 'Sale failed'; + // B6/B10: a 2FA verification-gate rejection (missing/invalid/expired + // code, brute-force lockout) is recoverable — keep the code populated + // and reveal the input so the sale can be retried with a fresh code. + if (isTwoFactorVerificationGateFailure(responseStatus, msg)) reveal2FACodeInput = true; paymentError = msg; toast.error(msg); } finally { @@ -666,13 +689,6 @@ - {#if twoFactorBlocksSavedCards} -
- Two-factor authentication is required to use online card payments. - Enable it in your account settings. -
- {/if} - {#if paymentMethod === 'online_square'}
{#if isSquareConfigured()} @@ -755,6 +771,11 @@

{/if} + + + {/if} @@ -777,6 +798,7 @@ loading={processing} disabled={!canCharge || processing || + missing2FACode || (paymentMethod === 'online_square' && !onlineSquareCardReady) || (paymentMethod === 'saved_card' && !selectedSavedCardId)} > diff --git a/frontend/src/lib/components/booking/BookingFlow.svelte b/frontend/src/lib/components/booking/BookingFlow.svelte index d5d71aa..851aaa5 100644 --- a/frontend/src/lib/components/booking/BookingFlow.svelte +++ b/frontend/src/lib/components/booking/BookingFlow.svelte @@ -36,12 +36,14 @@ import TimeSlotPicker from '$lib/components/booking/TimeSlotPicker.svelte'; import ServiceSelector from '$lib/components/booking/ServiceSelector.svelte'; import CardSelection from '$lib/components/payments/CardSelection.svelte'; + import TwoFactorCodeInput from '$lib/components/payments/TwoFactorCodeInput.svelte'; import PolicyPopover from '$lib/components/ui/policyPopover.svelte'; import { POLICY } from '$lib/constants/policy'; import { canSaveCardsForRole, isNonceStale, isOverflowTipConfirmationRequired, + isTwoFactorVerificationGateFailure, submitPaymentWithRetry } from '$lib/square/square'; import UserPaymentModal from '$lib/components/payments/UserPaymentModal.svelte'; @@ -144,10 +146,30 @@ const canSaveCards = $derived(canSaveCardsForRole(authStore.currentUser?.role)); - // PSD2 SCA stand-in: 2FA required but not enabled blocks saved-card use - // and saving new cards for reuse. The new-card (nonce) path has its own - // SCA via Square tokenizeWithVerification. - const twoFactorBlocksSavedCards = $derived(authStore.twoFactorBlocksSavedCards); + // B6/B10: saved-card deposits (and saving a new card for reuse) require the + // customer's current 2FA verification code whenever the backend enforces the + // gate. The input is surfaced at the charge step; the new-card (nonce) path + // keeps its own SCA via Square tokenizeWithVerification. + const savedCardChargeRequires2FACode = $derived(authStore.savedCardChargeRequires2FACode); + const twoFactorEnabled = $derived(!!authStore.currentUser?.twoFactorEnabled); + + // B6/B10: verification code for a saved-card deposit / new-card save. The + // backend requires the card owner's CURRENT one-time code when 2FA is + // enforced (delivered via the server log / email-SMS channel and relayed by + // the operator). Kept populated across retries so an invalid/expired code can + // be corrected without re-typing it. + let depositTwoFactorCode = $state(''); + // Set true when a charge 403s for a missing code: the backend keys on the + // CARD OWNER, so even a session user whose own flag is unset must be able to + // enter the code. + let reveal2FACodeInput = $state(false); + const show2FACodeInput = $derived( + reveal2FACodeInput || + (savedCardChargeRequires2FACode && (selectedPaymentMethod !== '' || depositSaveCard)) + ); + const missing2FACode = $derived( + show2FACodeInput && twoFactorEnabled && depositTwoFactorCode.trim() === '' + ); const depositCardFormValid = $derived(paymentCardSelectionValid); @@ -319,12 +341,6 @@ isProcessingPayment = true; paymentAttempted = false; try { - // PSD2 SCA stand-in: never charge a saved card while 2FA is required - // but not enabled — clear any stale selection so the new-card - // (nonce) path is used instead. - if (twoFactorBlocksSavedCards && selectedPaymentMethod) { - selectedPaymentMethod = ''; - } // Create the booking only if one does not already exist. A retry after // a failed deposit charge (or a lost response) must NOT re-create a // booking — the existing confirmedBooking is the one to charge, and @@ -349,8 +365,10 @@ let newCardToken: string | undefined; let verificationToken: string | undefined; - if (selectedPaymentMethod && !twoFactorBlocksSavedCards) { - // saved card — nothing to tokenize + if (selectedPaymentMethod) { + // saved card — nothing to tokenize; B6/B10 requires the customer's + // current 2FA verification code (collected in the charge form) + // when the backend enforces the gate. } else if (paymentCardSelection) { // New-card mode: tokenize once per attempt, reuse the nonce + SCA // verification token on retry (tokenization is one-shot; the @@ -412,7 +430,8 @@ idempotency_key: depositIdempotencyKey, ...(selectedPaymentMethod ? { card_id: selectedPaymentMethod } : {}), ...(newCardToken ? { new_card_token: newCardToken, save_card: depositSaveCard } : {}), - ...(verificationToken ? { verification_token: verificationToken } : {}) + ...(verificationToken ? { verification_token: verificationToken } : {}), + ...(show2FACodeInput ? { verification_code: depositTwoFactorCode } : {}) }; paymentAttempted = true; @@ -483,6 +502,8 @@ depositTokenizedAt = 0; depositTokenizedForSaveCard = false; depositSaveCard = false; + depositTwoFactorCode = ''; + reveal2FACodeInput = false; overflowConfirm = null; // Immutable update — avoid mutating the existing object so // concurrent renders (e.g. a stale fetch) can't observe partial @@ -499,6 +520,12 @@ } const text = await response.text(); + // B6/B10: a 2FA verification-gate rejection (missing/invalid/expired + // code, brute-force lockout) is recoverable — keep the code populated + // and reveal the input so the deposit can be retried with a fresh code. + if (isTwoFactorVerificationGateFailure(response.status, extractErrorMessage(text))) { + reveal2FACodeInput = true; + } // Pre-start overpayment guard on stale booking data: park the rejected // request (body + amount) and surface the Confirm/Cancel prompt instead // of a dead-end 400. The cached nonce + SCA verification token + @@ -2634,33 +2661,44 @@ /> {/if} - {:else} -
- (paymentCardSelectionValid = v)} - /> -
- {/if} - -
- - + {:else} +
+ (paymentCardSelectionValid = v)} + />
+ {/if} + + +
+ +
+ +
+ + +

Secure payment powered by Square diff --git a/frontend/src/lib/components/payments/CardSelection.svelte b/frontend/src/lib/components/payments/CardSelection.svelte index b9e1817..b34721f 100644 --- a/frontend/src/lib/components/payments/CardSelection.svelte +++ b/frontend/src/lib/components/payments/CardSelection.svelte @@ -41,40 +41,24 @@ // would collide on the same checkbox id. Pure SPA, so no SSR concern. const consentId = `save-card-consent-${crypto.randomUUID()}`; - // PSD2 SCA stand-in: when 2FA is required but not yet enabled, saved-card - // selection and save-for-later are blocked. The new-card (nonce) path has - // its own SCA via Square tokenizeWithVerification, so only the saved-card - // list and the save toggle are gated here. - const twoFactorBlocksSavedCards = $derived(authStore.twoFactorBlocksSavedCards); + // B6/B10: saved-card charges require the customer's current 2FA verification + // code. This no longer BLOCKS saved-card selection — the code is collected + // at the charge step (the parent charge forms show the input). The new-card + // (nonce) path keeps its own SCA via Square tokenizeWithVerification. + const savedCardChargeRequires2FACode = $derived(authStore.savedCardChargeRequires2FACode); + const twoFactorEnabled = $derived(!!authStore.currentUser?.twoFactorEnabled); // Auto-select the default saved card when cards first load. Guarded by // !showNewCardForm so the "Use a new card" click (selectedCardId = '') is // NOT immediately overridden back to the default card — which would - // silently charge the wrong card on submit. Also skipped while 2FA gating - // is active so a saved card is never selected by default. + // silently charge the wrong card on submit. $effect(() => { - if ( - cards.length > 0 && - !selectedCardId && - !showNewCardForm && - !twoFactorBlocksSavedCards - ) { + if (cards.length > 0 && !selectedCardId && !showNewCardForm) { const defaultCard = cards.find((c) => c.is_default) ?? cards[0]; selectedCardId = defaultCard.id; } }); - // While 2FA gating is active, keep the shared component self-consistent: - // never allow a saved card to stay selected or the save-card checkbox to - // remain checked (the parents' submit paths also guard, this is belt-and- - // braces for pre-selected state from a previous session). - $effect(() => { - if (twoFactorBlocksSavedCards && (selectedCardId !== '' || saveCard)) { - selectedCardId = ''; - saveCard = false; - } - }); - // When no saved cards exist the new-card form shows by default (no toggle). const newCardMode = $derived(showNewCardForm || cards.length === 0); @@ -116,43 +100,51 @@ } +{#if savedCardChargeRequires2FACode} + {#if twoFactorEnabled} +

+

+ A verification code is required to use a saved card — you'll be asked for it at checkout. +

+
+ {:else} +
+

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

+
+ {/if} +{/if} + {#if cards.length > 0}
- {#if twoFactorBlocksSavedCards} -
-

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

-
- {:else} - {#each cards as card (card.id)} - - {/each} - {/if} +
+ {#if selectedCardId === card.id && !showNewCardForm} + Selected + {/if} + + {/each}
-{:else if twoFactorBlocksSavedCards} -
-

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

-
{/if} {#if newCardMode} @@ -194,7 +179,7 @@ {/if} - {#if canSaveCards && squareCardReady && !twoFactorBlocksSavedCards} + {#if canSaveCards && squareCardReady && !(savedCardChargeRequires2FACode && !twoFactorEnabled)}