fix: review-loop B — adversarial findings (sweep auto-refund, admin clamp, 2FA real challenge, opaque refresh tokens, gated client IP, GBP pence)

Loop B aggressive adversarial round (3 attack agents) + fix + secondary + verification: - CRITICAL: sweep replay auto-refunds provably-created-later duplicate charges (gated on parseable CreatedAt); 22h legitimate-retry window == 22h sweep cutoff (no dead zone) - HIGH: admin Take Payment clamps to remaining obligation (cash/giftcard/saved-card/terminal); no unintended tip from overflow; campaign credit against remaining - HIGH: /api/services/eligible-for/{id} requires auth + owner-or-admin (DOB/age + patch-test health-data leak closed) - HIGH: opaque refresh-token rotation (login/refresh return {token, jti, refreshToken}; refresh REQUIRES opaque token; single-use rotation; logout revokes; access token rejected at refresh) - HIGH: saved-card charges require a REAL 2FA verification code (B6/B10) — backend gate on all 8 charge paths + shared TwoFactorCodeInput frontend component on all 7 surfaces; 2FA gate is no longer setup-flag-only - MEDIUM: ungated CF-Connecting-IP in reserve/admin_reserve gated via exported mw.ClientIP; 2FA limiter keyed on userID alone (no header-rotation bypass); ChangePassword actually revokes JTI + refresh tokens; 2FA setup mint cooldown + persistent failed-attempt counter; campaign redemption race surfaces campaign_fully_redeemed - Terminal saved-card VAT applied (was under-collected); age-guard reconcile failures notify; isWeakJWTSecret entropy gate; gift-card redeem per-card counter + per-user limiter; webhook signature key startup validation - NEW internal/twofa package (single source of truth breaking the payments<->user import cycle); consolidation of duplicate 2FA hash/verify - Frontend: refresh-token storage + rotation, TwoFactorCodeInput component, amountPaidPence in admin modal, B5/B6/B10 contract wiring; 70 frontend tests - Tests: loop_b_fixes_test.go, internal/twofa tests, updated auth/services/profile/twofa/mw tests All 26 backend packages pass (incl. internal/twofa); frontend 70/70 + build clean; env-docs 41/41.
2026-08-22 00:34:50 +01:00
parent faceb9809c
commit fe88f2084d
55 changed files with 4180 additions and 971 deletions
@@ -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) }
@@ -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)
+		})
+	}
+}
@@ -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
@@ -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)