fix: review round — B1 clock-skew tolerance + re-poll escalation, refresh-token access-token revocation, shared 2FA composable, per-package-DB test alignment

Three fresh reviews (money/security/dup-mod) cross-validated findings: - MEDIUM: B1 'new charge' discrimination adds a lower-bound tolerance (replayRescueLowerBoundSkew) so a retained-key replay of the ORIGINAL charge (DB clock ahead of Square) is never auto-refunded; ambiguous margins leave PENDING + CRITICAL - MEDIUM: B1 re-poll escalates after stalePendingB1RefundAge (48h) — FAILED/REJECTED refunds go terminal (fail parent, claw back till-sale funding, CRITICAL notification); no more unbounded re-polling / stranded parents without webhooks - DRIFT-REAL: processManualPaymentGroup now checks PENDING/FAILED/REJECTED on the synchronous refund response (mirrors processChargeGroup/manual handler) — no more premature 'completed' - HIGH: refresh-token family kill now also invalidates the attacker's freshly-minted ACCESS token — access tokens carry a family_id claim and VerifyToken rejects tokens whose family was deleted (GenerateTokenForFamily + family-alive check); 30s grace window for concurrent two-tab refresh (no false theft alert) - LOW: 2FA mint endpoint returns remaining_seconds; in-memory 2FA counters documented; 90-day refresh expiry single-sourced (RefreshTokenLifetime + make_interval) - Dup/mod: NEW shared useTwoFactorCodeForSavedCard Svelte composable replaces 6 surface copies of the 2FA gate logic (Request-a-new-code added to BookingFlow + TillPurchases); account page adopts generateUUID - Test architecture: removed t.Parallel() from 8 global-SquareClient-swapping tests per Testing Architecture doc line 89 (B1 flaky-test lesson) — fixes within-package race - SQL alias pence rename (total_cents/paid_cents -> total_pence/paid_pence) 26/26 backend packages; 72/72 frontend tests + build; env-docs 41/41.
2026-08-22 00:34:50 +01:00
parent a8bf24ee23
commit a6a4683b74
24 changed files with 1338 additions and 384 deletions
@@ -19,6 +19,37 @@ import (

 var TokenAuth *jwtauth.JWTAuth

+// RefreshTokenLifetime is how long an issued refresh token stays valid before
+// it expires (90 days). Minted rows get expires_at = NOW() + RefreshTokenLifetime
+// (see GenerateRefreshToken / GenerateRefreshTokenInFamily), and the cleanup
+// jobs that expire refresh-token families and retain revoked tokens
+// (handlers/scheduling CleanupExpiredRefreshTokens, jobs SweepSquareWebhookEvents)
+// reference the SAME constant so the SQL window can never drift from the mint.
+const RefreshTokenLifetime = 90 * 24 * time.Hour
+
+// refreshTokenLifetimeDays is RefreshTokenLifetime expressed in whole days, fed
+// to the SQL make_interval(days => ...) calls in the mint queries.
+const refreshTokenLifetimeDays = int64(RefreshTokenLifetime / (24 * time.Hour))
+
+// refreshTokenReuseGrace is how long after a rotation a used-token replay is
+// treated as a BENIGN concurrent refresh (two tabs sharing one refresh token in
+// localStorage both refreshing on load) instead of theft. A replay inside the
+// grace window gets the generic error but does NOT kill the rotation family and
+// does NOT raise the refresh_token_reuse alert — only a replay after the window
+// has elapsed is treated as theft (see VerifyRefreshToken).
+const refreshTokenReuseGrace = 30 * time.Second
+
+// refreshTokenReuseGraceSecs is the grace window in whole seconds for the SQL
+// make_interval(secs => ...) comparison in VerifyRefreshToken's reuse branch.
+const refreshTokenReuseGraceSecs = int64(refreshTokenReuseGrace / time.Second)
+
+// accessTokenFamilyClaim is the JWT claim that binds an access token to the
+// refresh-token rotation family it was minted alongside. VerifyToken rejects an
+// access token whose family_id no longer exists in refresh_tokens, so when
+// reuse detection DELETEs a family every access token minted by that lineage
+// dies immediately instead of remaining valid for its 1-hour TTL (HIGH 1).
+const accessTokenFamilyClaim = "family_id"
+
 // 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
@@ -127,20 +158,39 @@ func InitJWT(secret string) {
 	TokenAuth = jwtauth.New("HS256", []byte(secret), nil)
 }

-// GenerateToken creates a JWT with user_id, role, and a unique jti claim
-// Returns the token string, the JTI, and any error
+// GenerateToken creates a JWT with user_id, role, and a unique jti claim.
+// Returns the token string, the JTI, and any error. The token carries NO
+// family_id claim, so it is exempt from the family-alive check in VerifyToken —
+// kept for callers minting transient/test tokens. Production login and refresh
+// paths use GenerateTokenForFamily so a killed rotation family cannot keep an
+// access token alive (HIGH 1).
 func GenerateToken(userID string, role string) (string, string, error) {
+	return GenerateTokenForFamily(userID, role, "")
+}
+
+// GenerateTokenForFamily mints an access token carrying a family_id claim that
+// binds it to a refresh-token rotation family. VerifyToken rejects a token whose
+// family_id no longer exists in refresh_tokens, so when reuse detection DELETEs
+// the whole family (VerifyRefreshToken), every access token minted by that
+// lineage dies immediately instead of remaining valid for its 1-hour TTL.
+// familyID == "" mints an unbound token (same as GenerateToken).
+func GenerateTokenForFamily(userID string, role string, familyID string) (string, string, error) {
 	jti, err := generateJTI()
 	if err != nil {
 		return "", "", err
 	}

-	_, tokenString, err := TokenAuth.Encode(map[string]any{
+	claims := map[string]any{
 		"user_id": userID,
 		"role":    role,
 		"jti":     jti,
 		"exp":     clock.Now().Add(1 * time.Hour).Unix(), // 1 hour
-	})
+	}
+	if familyID != "" {
+		claims[accessTokenFamilyClaim] = familyID
+	}
+
+	_, tokenString, err := TokenAuth.Encode(claims)
 	return tokenString, jti, err
 }

@@ -182,9 +232,58 @@ func VerifyToken(tokenString string, ctx context.Context) (userID string, role s
 		return "", "", "", fmt.Errorf("token revoked")
 	}

+	// HIGH 1: reject an access token bound (family_id claim) to a rotation
+	// family that reuse detection has killed. A token minted at rotation carries
+	// family_id; when that family no longer exists in refresh_tokens the token is
+	// dead even though its JTI was never revoked — closing the up-to-1-hour
+	// window where a stolen refresh token's freshly-minted access token could
+	// still hit money endpoints (gift-card buy, saved-card booking payment, tip).
+	if err := verifyFamilyAlive(ctx, token, userID); err != nil {
+		return "", "", "", err
+	}
+
 	return userID, role, jti, nil
 }

+// verifyFamilyAlive rejects access tokens bound to a rotation family that no
+// longer exists in refresh_tokens. A token WITHOUT a family_id claim is unbound
+// (minted via GenerateToken — tests/legacy callers) and passes. Fails closed on
+// a live-DB query error: when the family cannot be confirmed alive the safe
+// default for money endpoints is to refuse. Mirrors IsJTIRevoked's nil-db
+// availability default (startup/unit tests).
+func verifyFamilyAlive(ctx context.Context, token jwtClaimGetter, userID string) error {
+	var familyVal any
+	if err := token.Get(accessTokenFamilyClaim, &familyVal); err != nil {
+		return nil
+	}
+	familyID, ok := familyVal.(string)
+	if !ok || familyID == "" {
+		return nil
+	}
+	if db.Conn == nil {
+		return nil
+	}
+	var exists bool
+	err := db.Conn.QueryRow(ctx,
+		`SELECT EXISTS(
+			SELECT 1 FROM refresh_tokens WHERE family_id = $1 AND user_id = $2
+		)`, familyID, userID).Scan(&exists)
+	if err != nil {
+		return fmt.Errorf("token revoked")
+	}
+	if !exists {
+		return fmt.Errorf("token revoked")
+	}
+	return nil
+}
+
+// jwtClaimGetter is the minimal subset of jwt.Token needed to read a claim
+// (the token returned by jwtauth.VerifyToken). Kept as an interface so the
+// lestrrat-go/jwx dependency stays out of this file's imports.
+type jwtClaimGetter interface {
+	Get(string, interface{}) error
+}
+
 // generateRefreshTokenString creates a cryptographically random opaque refresh token
 func generateRefreshTokenString() (string, error) {
 	b := make([]byte, 32)
@@ -194,23 +293,25 @@ func generateRefreshTokenString() (string, error) {
 	return fmt.Sprintf("%x", b), nil
 }

-// GenerateRefreshToken creates a refresh token stored in the database
-// Returns the opaque token string to return to the client
-func GenerateRefreshToken(ctx context.Context, userID string, role string) (string, error) {
+// GenerateRefreshToken creates a refresh token stored in the database. Returns
+// the opaque token string to return to the client plus the id of the rotation
+// family the new row was created in (so the caller can bind the matching access
+// token to it via GenerateTokenForFamily — see HIGH 1).
+func GenerateRefreshToken(ctx context.Context, userID string, role string) (string, string, error) {
 	token, err := generateRefreshTokenString()
 	if err != nil {
-		return "", err
+		return "", "", err
 	}

-	// Store hashed version in DB with 90-day expiry
+	// Store hashed version in DB with the shared RefreshTokenLifetime expiry
 	query := `
-		INSERT INTO refresh_tokens (user_id, token_hash, role, expires_at)
-		VALUES ($1, encode(sha256($2::bytea), 'hex'), $3, NOW() + INTERVAL '90 days')
-		RETURNING id`
+		INSERT INTO refresh_tokens (user_id, token_hash, role, family_id, expires_at)
+		VALUES ($1, encode(sha256($2::bytea), 'hex'), $3, gen_random_uuid(), NOW() + make_interval(days => $4))
+		RETURNING id, family_id`

 	tx, err := db.Conn.Begin(ctx)
 	if err != nil {
-		return "", fmt.Errorf("failed to begin transaction: %w", err)
+		return "", "", fmt.Errorf("failed to begin transaction: %w", err)
 	}
 	defer func() {
 		if err := tx.Rollback(ctx); err != nil && !errors.Is(err, pgx.ErrTxClosed) {
@@ -219,16 +320,17 @@ func GenerateRefreshToken(ctx context.Context, userID string, role string) (stri
 	}()

 	var tokenID int64
-	err = tx.QueryRow(ctx, query, userID, token, role).Scan(&tokenID)
+	var familyID string
+	err = tx.QueryRow(ctx, query, userID, token, role, refreshTokenLifetimeDays).Scan(&tokenID, &familyID)
 	if err != nil {
-		return "", fmt.Errorf("failed to store refresh token: %w", err)
+		return "", "", fmt.Errorf("failed to store refresh token: %w", err)
 	}

 	if err := tx.Commit(ctx); err != nil {
-		return "", fmt.Errorf("failed to commit transaction: %w", err)
+		return "", "", fmt.Errorf("failed to commit transaction: %w", err)
 	}

-	return token, nil
+	return token, familyID, nil
 }

 // GenerateRefreshTokenInFamily creates a refresh token in the SAME rotation
@@ -242,10 +344,10 @@ func GenerateRefreshTokenInFamily(ctx context.Context, userID string, role strin
 		return "", err
 	}

-	// Store hashed version in DB with 90-day expiry, in the given family
+	// Store hashed version in DB with the shared RefreshTokenLifetime expiry, in the given family
 	query := `
 		INSERT INTO refresh_tokens (user_id, token_hash, role, family_id, expires_at)
-		VALUES ($1, encode(sha256($2::bytea), 'hex'), $3, $4, NOW() + INTERVAL '90 days')
+		VALUES ($1, encode(sha256($2::bytea), 'hex'), $3, $4, NOW() + make_interval(days => $5))
 		RETURNING id`

 	tx, err := db.Conn.Begin(ctx)
@@ -259,7 +361,7 @@ func GenerateRefreshTokenInFamily(ctx context.Context, userID string, role strin
 	}()

 	var tokenID int64
-	err = tx.QueryRow(ctx, query, userID, token, role, familyID).Scan(&tokenID)
+	err = tx.QueryRow(ctx, query, userID, token, role, familyID, refreshTokenLifetimeDays).Scan(&tokenID)
 	if err != nil {
 		return "", fmt.Errorf("failed to store refresh token: %w", err)
 	}
@@ -274,10 +376,15 @@ func GenerateRefreshTokenInFamily(ctx context.Context, userID string, role strin
 // VerifyRefreshToken checks a refresh token and returns user details if valid.
 // The token is consumed (marked used) upon successful verification — rotation —
 // and its family_id is returned so the caller can mint the descendant in the
-// SAME family. If an ALREADY-ROTATED token is presented again (a replay: the
-// attacker rotated it, then the victim replayed it), the entire rotation family
-// is revoked (the descendant minted at rotation dies too) and a critical admin
-// notification (reason 'refresh_token_reuse') is raised. The caller always gets
+// SAME family. If an ALREADY-ROTATED token is presented again after the
+// refreshTokenReuseGrace window (a replay: the attacker rotated it, then the
+// victim replayed it), the entire rotation family is revoked (the descendant
+// minted at rotation dies too, and so does every access token bound to the
+// family — HIGH 1) and a critical admin notification (reason
+// 'refresh_token_reuse') is raised. A replay WITHIN the grace window is a
+// benign concurrent refresh (two tabs sharing one localStorage refresh token
+// refreshing on load): it gets the generic error but kills NOTHING and raises
+// NO alert, so the legitimately-rotated session survives. The caller always gets
 // the generic "invalid or expired refresh token" error so reuse is never leaked.
 func VerifyRefreshToken(ctx context.Context, tokenString string) (userID string, role string, familyID string, err error) {
 	query := `
@@ -312,14 +419,18 @@ func VerifyRefreshToken(ctx context.Context, tokenString string) (userID string,
 	}

 	// The rotation UPDATE matched nothing: the token is expired, revoked, or
-	// never issued — OR it was already used (replayed). A used token is theft:
-	// the descendant minted at rotation would otherwise stay valid for 90 days.
+	// never issued — OR it was already used (replayed). A used token replayed
+	// AFTER the grace window is theft: the descendant minted at rotation would
+	// otherwise stay valid for 90 days. A used token replayed WITHIN the grace
+	// window is a benign concurrent refresh (two tabs, one shared refresh token)
+	// and falls through to the generic error below — no family kill, no alert.
 	var reusedUserID, reusedFamilyID string
 	reuseErr := tx.QueryRow(ctx, `
 		SELECT user_id, family_id FROM refresh_tokens
 		WHERE token_hash = encode(sha256($1::bytea), 'hex')
 		  AND used_at IS NOT NULL
-	`, tokenString).Scan(&reusedUserID, &reusedFamilyID)
+		  AND used_at < NOW() - make_interval(secs => $2)
+	`, tokenString, refreshTokenReuseGraceSecs).Scan(&reusedUserID, &reusedFamilyID)

 	if reuseErr == nil {
 		// (i) Revoke the ENTIRE family — the reused token and every descendant.
@@ -378,7 +378,7 @@ func TestGenerateRefreshToken_Success(t *testing.T) {
 		t.Fatalf("failed to create test user: %v", err)
 	}

-	token, err := GenerateRefreshToken(ctx, userID, "verified_email")
+	token, _, err := GenerateRefreshToken(ctx, userID, "verified_email")
 	if err != nil {
 		t.Fatalf("GenerateRefreshToken() failed: %v", err)
 	}
@@ -418,7 +418,7 @@ func TestVerifyRefreshToken_Success(t *testing.T) {
 		t.Fatalf("failed to create test user: %v", err)
 	}

-	token, err := GenerateRefreshToken(ctx, userID, "verified_email")
+	token, _, err := GenerateRefreshToken(ctx, userID, "verified_email")
 	if err != nil {
 		t.Fatalf("GenerateRefreshToken() failed: %v", err)
 	}
@@ -455,7 +455,7 @@ func TestVerifyRefreshToken_Rotation(t *testing.T) {
 		t.Fatalf("failed to create test user: %v", err)
 	}

-	token, err := GenerateRefreshToken(ctx, userID, "verified_email")
+	token, _, err := GenerateRefreshToken(ctx, userID, "verified_email")
 	if err != nil {
 		t.Fatalf("GenerateRefreshToken() failed: %v", err)
 	}
@@ -491,7 +491,7 @@ func TestVerifyRefreshToken_ReuseRevokesFamilyAndAlerts(t *testing.T) {
 	}

 	// 1. Generate a refresh token (new family) and rotate it once.
-	original, err := GenerateRefreshToken(ctx, userID, "verified_email")
+	original, _, err := GenerateRefreshToken(ctx, userID, "verified_email")
 	if err != nil {
 		t.Fatalf("GenerateRefreshToken() failed: %v", err)
 	}
@@ -519,6 +519,18 @@ func TestVerifyRefreshToken_ReuseRevokesFamilyAndAlerts(t *testing.T) {
 		t.Fatalf("expected 2 refresh tokens in family, got %d", famCount)
 	}

+	// MEDIUM 4 grace window: backdate the original's used_at past the 30s
+	// reuse grace so the replay below is genuine theft. WITHOUT this, a replay
+	// moments after rotation is a benign two-tab concurrent refresh and the
+	// family must NOT be killed.
+	if _, err := tx.Exec(ctx, `
+		UPDATE refresh_tokens
+		SET used_at = NOW() - make_interval(secs => 60)
+		WHERE token_hash = encode(sha256($1::bytea), 'hex')
+	`, original); err != nil {
+		t.Fatalf("failed to backdate used_at for reuse test: %v", err)
+	}
+
 	// 3. Replay the ORIGINAL token — theft.
 	_, _, _, err = VerifyRefreshToken(ctx, original)
 	if err == nil {
@@ -560,6 +572,141 @@ func TestVerifyRefreshToken_ReuseRevokesFamilyAndAlerts(t *testing.T) {
 	}
 }

+// TestVerifyRefreshToken_ReplayWithinGrace_IsBenign verifies the MEDIUM 4
+// hardening: a used-token replay WITHIN the 30s grace window (two tabs sharing
+// one localStorage refresh token both refreshing on load) is a benign
+// concurrent refresh — the generic error is returned, but the rotation family
+// survives and no refresh_token_reuse alert is raised.
+func TestVerifyRefreshToken_ReplayWithinGrace_IsBenign(t *testing.T) {
+	ctx, tx := testtx.SetupTestTx(t)
+
+	userID, err := fixtures.CreateTestUser(tx)
+	if err != nil {
+		t.Fatalf("failed to create test user: %v", err)
+	}
+
+	original, _, err := GenerateRefreshToken(ctx, userID, "verified_email")
+	if err != nil {
+		t.Fatalf("GenerateRefreshToken() failed: %v", err)
+	}
+
+	_, _, familyID, err := VerifyRefreshToken(ctx, original)
+	if err != nil {
+		t.Fatalf("first verification should succeed, got: %v", err)
+	}
+	if familyID == "" {
+		t.Fatal("expected non-empty family_id from rotation")
+	}
+
+	// Mint the descendant in the same family (as RefreshTokenHandler does).
+	if _, err := GenerateRefreshTokenInFamily(ctx, userID, "verified_email", familyID); err != nil {
+		t.Fatalf("GenerateRefreshTokenInFamily() failed: %v", err)
+	}
+
+	// Replay the original IMMEDIATELY — inside the grace window → benign.
+	_, _, _, err = VerifyRefreshToken(ctx, original)
+	if err == nil {
+		t.Fatal("expected error for within-grace replay, got nil")
+	}
+	if !strings.Contains(err.Error(), "invalid or expired") {
+		t.Errorf("expected 'invalid or expired' error, got: %v", err)
+	}
+
+	// The family survives: used original + descendant both still present.
+	var famCount int
+	if err := tx.QueryRow(ctx,
+		`SELECT COUNT(*) FROM refresh_tokens WHERE family_id = $1`, familyID).Scan(&famCount); err != nil {
+		t.Fatalf("failed to count family rows: %v", err)
+	}
+	if famCount != 2 {
+		t.Errorf("expected 2 refresh tokens in family after within-grace replay, got %d", famCount)
+	}
+
+	// No theft alert.
+	var alertCount int
+	if err := tx.QueryRow(ctx,
+		`SELECT COUNT(*) FROM admin_notifications WHERE reason = 'refresh_token_reuse' AND user_id = $1`,
+		userID).Scan(&alertCount); err != nil {
+		t.Fatalf("failed to query admin_notifications: %v", err)
+	}
+	if alertCount != 0 {
+		t.Errorf("expected 0 'refresh_token_reuse' alerts for a within-grace replay, got %d", alertCount)
+	}
+}
+
+// TestAccessTokenKilledWithRotationFamily verifies the HIGH 1 fix: an access
+// token minted at rotation is bound (family_id claim) to the rotation family,
+// so when reuse detection DELETEs the family the access token — which the
+// attacker was handed at rotation — stops verifying immediately instead of
+// staying valid for its 1-hour TTL.
+func TestAccessTokenKilledWithRotationFamily(t *testing.T) {
+	ctx, tx := testtx.SetupTestTx(t)
+
+	userID, err := fixtures.CreateTestUser(tx)
+	if err != nil {
+		t.Fatalf("failed to create test user: %v", err)
+	}
+
+	original, _, err := GenerateRefreshToken(ctx, userID, "verified_email")
+	if err != nil {
+		t.Fatalf("GenerateRefreshToken() failed: %v", err)
+	}
+
+	_, _, familyID, err := VerifyRefreshToken(ctx, original)
+	if err != nil {
+		t.Fatalf("first verification should succeed, got: %v", err)
+	}
+
+	// The rotation response: a descendant refresh token in the same family and
+	// an access token minted in that SAME family (as RefreshTokenHandler does).
+	if _, err := GenerateRefreshTokenInFamily(ctx, userID, "verified_email", familyID); err != nil {
+		t.Fatalf("GenerateRefreshTokenInFamily() failed: %v", err)
+	}
+	attackerToken, attackerJTI, err := GenerateTokenForFamily(userID, "verified_email", familyID)
+	if err != nil {
+		t.Fatalf("GenerateTokenForFamily() failed: %v", err)
+	}
+	if attackerJTI == "" {
+		t.Fatal("expected non-empty JTI")
+	}
+
+	// While its family is alive the access token verifies (JTI not revoked).
+	if _, _, _, err := VerifyToken(attackerToken, ctx); err != nil {
+		t.Fatalf("access token must verify while its rotation family is alive: %v", err)
+	}
+
+	// Backdate used_at past the grace window, then replay the original — theft
+	// detected, whole family DELETEd.
+	if _, err := tx.Exec(ctx, `
+		UPDATE refresh_tokens
+		SET used_at = NOW() - make_interval(secs => 60)
+		WHERE token_hash = encode(sha256($1::bytea), 'hex')
+	`, original); err != nil {
+		t.Fatalf("failed to backdate used_at: %v", err)
+	}
+	_, _, _, err = VerifyRefreshToken(ctx, original)
+	if err == nil {
+		t.Fatal("expected theft detection on replayed token, got nil")
+	}
+
+	var famCount int
+	if err := tx.QueryRow(ctx,
+		`SELECT COUNT(*) FROM refresh_tokens WHERE family_id = $1`, familyID).Scan(&famCount); err != nil {
+		t.Fatalf("failed to count family rows: %v", err)
+	}
+	if famCount != 0 {
+		t.Fatalf("expected 0 refresh tokens in family after reuse, got %d", famCount)
+	}
+
+	// The attacker's access token is now dead even though its JTI was never
+	// revoked — the family-alive check rejects it (HIGH 1).
+	if _, _, _, err := VerifyToken(attackerToken, ctx); err == nil {
+		t.Fatal("access token must be rejected once its rotation family is killed")
+	} else if !strings.Contains(err.Error(), "token revoked") {
+		t.Fatalf("expected 'token revoked' error, got: %v", err)
+	}
+}
+
 // TestVerifyRefreshToken_InvalidToken calls VerifyRefreshToken with a fake
 // token string and expects it to fail with "invalid or expired".
 func TestVerifyRefreshToken_InvalidToken(t *testing.T) {