diff --git a/backend/auth/jwt.go b/backend/auth/jwt.go index 458eb76..ac2e19e 100644 --- a/backend/auth/jwt.go +++ b/backend/auth/jwt.go @@ -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. diff --git a/backend/auth/jwt_test.go b/backend/auth/jwt_test.go index acdb824..498494c 100644 --- a/backend/auth/jwt_test.go +++ b/backend/auth/jwt_test.go @@ -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) { diff --git a/backend/handlers/auth/auth_test.go b/backend/handlers/auth/auth_test.go index bc1d02b..8d058bc 100644 --- a/backend/handlers/auth/auth_test.go +++ b/backend/handlers/auth/auth_test.go @@ -445,7 +445,7 @@ func TestRefreshToken_Success(t *testing.T) { // 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") + refreshToken, _, err := auth.GenerateRefreshToken(ctx, userID, "verified_email") if err != nil { t.Fatalf("failed to generate refresh token: %v", err) } @@ -1432,7 +1432,7 @@ func TestRefreshToken_RotatesRefreshToken(t *testing.T) { } defer fixtures.DeleteUser(tx, userID) - refreshToken, err := auth.GenerateRefreshToken(ctx, userID, "verified_email") + refreshToken, _, err := auth.GenerateRefreshToken(ctx, userID, "verified_email") if err != nil { t.Fatalf("failed to generate refresh token: %v", err) } @@ -1763,7 +1763,7 @@ func TestRefreshToken_Generation(t *testing.T) { } defer fixtures.DeleteUser(tx, userID) - refreshToken, err := auth.GenerateRefreshToken(ctx, userID, "verified_email") + refreshToken, _, err := auth.GenerateRefreshToken(ctx, userID, "verified_email") if err != nil { t.Fatalf("failed to generate refresh token: %v", err) } @@ -1920,7 +1920,7 @@ func TestRefreshToken_RotatesRefreshToken_DBBacked(t *testing.T) { } defer fixtures.DeleteUser(tx, userID) - refreshToken, err := auth.GenerateRefreshToken(ctx, userID, "verified_email") + refreshToken, _, err := auth.GenerateRefreshToken(ctx, userID, "verified_email") if err != nil { t.Fatalf("failed to generate refresh token: %v", err) } diff --git a/backend/handlers/auth/local.go b/backend/handlers/auth/local.go index a290733..47a9a02 100644 --- a/backend/handlers/auth/local.go +++ b/backend/handlers/auth/local.go @@ -451,25 +451,30 @@ func LoginHandler(w http.ResponseWriter, r *http.Request) { return } - // Generate JWT - tokenString, jti, err := auth.GenerateToken(userID, role) - if err != nil { - http.Error(w, "could not generate token", http.StatusInternalServerError) - 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) + // the SPA can persist it and present it to POST /api/refresh-token. The + // access token is bound to the new rotation family (HIGH 1) so that if the + // login refresh token is ever replayed the whole family — access token + // included — is killed. + refreshToken, familyID, 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 } + // Generate the access token AFTER the refresh token so it can be bound to + // the same rotation family. + tokenString, jti, err := auth.GenerateTokenForFamily(userID, role, familyID) + if err != nil { + log.Printf("failed to generate access token for user %s: %v", userID, err) + http.Error(w, "could not generate token", http.StatusInternalServerError) + return + } + if err := json.NewEncoder(w).Encode(auth.AuthResponse{ Token: tokenString, JTI: jti, @@ -528,11 +533,9 @@ func RefreshTokenHandler(w http.ResponseWriter, r *http.Request) { // Issue a fresh access token + refresh token pair. The rotated refresh // token is minted in the SAME family (familyID from VerifyRefreshToken) so // a replayed ancestor can revoke the whole lineage, descendants included. - newToken, jti, err := auth.GenerateToken(userID, currentRole) - if err != nil { - mw.RespondError(w, http.StatusInternalServerError, "could not generate token") - return - } + // The access token is bound to that same family (HIGH 1): when reuse + // detection kills the family, the freshly-minted access token handed to the + // attacker at rotation dies with it instead of staying valid for 1 hour. newRefreshToken, err := auth.GenerateRefreshTokenInFamily(r.Context(), userID, currentRole, familyID) if err != nil { log.Printf("failed to issue rotated refresh token for user %s: %v", userID, err) @@ -540,6 +543,14 @@ func RefreshTokenHandler(w http.ResponseWriter, r *http.Request) { return } + // Mint the access token AFTER the descendant refresh token so it can be + // bound to the same rotation family. + newToken, jti, err := auth.GenerateTokenForFamily(userID, currentRole, familyID) + if err != nil { + mw.RespondError(w, http.StatusInternalServerError, "could not generate token") + return + } + if err := json.NewEncoder(w).Encode(auth.AuthResponse{ Token: newToken, JTI: jti, diff --git a/backend/handlers/payments/completion.go b/backend/handlers/payments/completion.go index 88e3670..f8ee041 100644 --- a/backend/handlers/payments/completion.go +++ b/backend/handlers/payments/completion.go @@ -382,16 +382,16 @@ func bookingIsFullyPaid(ctx context.Context, q db.Querier, bookingID string) boo var fullyPaid bool if err := q.QueryRow(ctx, ` WITH booking_total AS ( - SELECT total_amount * 100 AS total_cents FROM bookings WHERE id = $1 + SELECT total_amount * 100 AS total_pence FROM bookings WHERE id = $1 ), paid_total AS ( - SELECT COALESCE(SUM(amount), 0) * 100 AS paid_cents + SELECT COALESCE(SUM(amount), 0) * 100 AS paid_pence FROM payments WHERE booking_id = $1 AND status = 'completed' AND payment_type != 'tip' AND payment_method NOT IN ('on_the_house') ) - SELECT pt.paid_cents >= bt.total_cents AND bt.total_cents > 0 + SELECT pt.paid_pence >= bt.total_pence AND bt.total_pence > 0 FROM booking_total bt, paid_total pt `, bookingID).Scan(&fullyPaid); err != nil { log.Printf("Failed to check full-payment threshold for booking %s: %v", bookingID, err) diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index 5467322..abc3cfe 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -2301,16 +2301,16 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { var depositMet bool if err := tx2.QueryRow(r.Context(), fmt.Sprintf(` WITH booking_total AS ( - SELECT total_amount * 100 AS total_cents FROM bookings WHERE id = $1 + SELECT total_amount * 100 AS total_pence FROM bookings WHERE id = $1 ), paid_total AS ( - SELECT COALESCE(SUM(amount), 0) * 100 AS paid_cents + SELECT COALESCE(SUM(amount), 0) * 100 AS paid_pence FROM payments WHERE booking_id = $1 AND status = 'completed' AND payment_type != 'tip' AND payment_method NOT IN ('discount', 'on_the_house') ) - SELECT pt.paid_cents >= ROUND(bt.total_cents * %f) + SELECT pt.paid_pence >= ROUND(bt.total_pence * %f) FROM booking_total bt, paid_total pt `, depositPromotionMinPct), bookingID).Scan(&depositMet); err != nil { log.Printf("Failed to check deposit threshold for booking %s: %v", bookingID, err) diff --git a/backend/handlers/payments/loop_b_fixes_test.go b/backend/handlers/payments/loop_b_fixes_test.go index e8817ef..0e49065 100644 --- a/backend/handlers/payments/loop_b_fixes_test.go +++ b/backend/handlers/payments/loop_b_fixes_test.go @@ -74,7 +74,6 @@ func TestTerminalCash_ClampsToRemainingObligation(t *testing.T) { // a customer who already paid in full (overpayment is handled manually at the // counter, not minted into the ledger). func TestTerminalCash_FullyPaid_RejectsOvercharge(t *testing.T) { - t.Parallel() ctx, tx := testutils.SetupTestTx(t) _, bookingID, _ := setupTestData(t, ctx, tx) @@ -100,7 +99,6 @@ func TestTerminalCash_FullyPaid_RejectsOvercharge(t *testing.T) { // 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) @@ -138,7 +136,6 @@ func TestTerminalSavedCard_ClampsToRemainingObligation(t *testing.T) { // 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) @@ -170,7 +167,6 @@ func TestTerminalCheckout_NoTip_ClampsToRemaining(t *testing.T) { // 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) @@ -207,7 +203,6 @@ func TestTerminalCheckout_TipEnabled_NotClamped(t *testing.T) { // 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`) @@ -335,7 +330,6 @@ func TestApplyEligibleCampaignsAtPayment_CampaignAvailable_NoError(t *testing.T) // 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) @@ -408,7 +402,6 @@ func (c *exhaustCampaignOnChargeClient) CreatePayment(ctx context.Context, req s // 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) diff --git a/backend/handlers/payments/m4_tip_refund_redesign_test.go b/backend/handlers/payments/m4_tip_refund_redesign_test.go index c25ff66..7d0be65 100644 --- a/backend/handlers/payments/m4_tip_refund_redesign_test.go +++ b/backend/handlers/payments/m4_tip_refund_redesign_test.go @@ -221,7 +221,6 @@ func TestProcessCancellationRefund_ExcludesTips(t *testing.T) { } func TestGetBookingPaymentInfo_ExcludesTips(t *testing.T) { - t.Parallel() ctx, tx := testutils.SetupTestTx(t) userID, err := fixtures.CreateTestUser(tx) diff --git a/backend/handlers/payments/refunds.go b/backend/handlers/payments/refunds.go index add69ff..9130fd3 100644 --- a/backend/handlers/payments/refunds.go +++ b/backend/handlers/payments/refunds.go @@ -44,6 +44,20 @@ import ( // 'failed' and surfaced for manual arrangement instead. const stalePendingRefundAge = 23 * time.Hour +// stalePendingB1RefundAge is the age guard for a B1 sweep auto-refund of a +// replay-induced duplicate charge (sweepPendingB1Refunds) that Square left +// PENDING. FAILED/REJECTED are terminal refund states, but the webhook +// FAILED-refund reconciliation that would normally resolve them is OPTIONAL +// (README) — with it unconfigured, hasInFlightSweepDuplicateRefund blocks the +// stale sweeps from resolving the parent forever, no alert fires and every +// sweep run re-polls Square indefinitely. After a B1 refund has been pending +// this long the re-poll pass treats FAILED/REJECTED as terminal itself (failing +// the refund, resolving the parent and alerting), and a still-pending refund +// older than this is escalated to a deduped CRITICAL admin notification and no +// longer re-polled. Never marks anything completed without a COMPLETED Square +// refund. +const stalePendingB1RefundAge = 48 * time.Hour + // maxManualRefundAttempts caps retry attempts for a refund stuck on a // decline/ambiguous outcome before it is resolved terminally. Both the manual // sweep (processManualPaymentGroup / resolveManualRefundAtCap) and the @@ -91,6 +105,21 @@ var ( manualReconcileFailures = make(map[string]int) ) +// b1EscalatedMu guards b1Escalated, the set of B1 sweep auto-refund rows the +// re-poll pass has escalated (pending past stalePendingB1RefundAge). Escalated +// rows are no longer re-polled: the FAILED/REJECTED-terminal branch already +// resolves them (the row leaves the pending query), and a still-pending +// escalated row keeps a deduped CRITICAL admin notification alive while the +// parent stays pending for manual reconciliation. In-memory (no schema change — +// the schema is single-source and pre-launch, no ALTERs), mirroring +// manualReconcileFailures: a process restart resets the set, which merely +// re-polls the row once and re-escalates it (the notification is deduped), never +// suppressing the alert. +var ( + b1EscalatedMu sync.Mutex + b1Escalated = make(map[string]bool) +) + // trackReconcileFailureReArm records one more consecutive cap-time reconcile // failure for each refund row and, once a row crosses // maxConsecutiveReconcileFailures, surfaces a deduped 'critical_payment_log' @@ -1587,14 +1616,16 @@ func sweepManualPendingSquareRefunds(ctx context.Context) (int, error) { // b1PendingRefund is one refunds row for a sweep auto-refund of a // replay-induced duplicate charge (B1) that Square left PENDING. type b1PendingRefund struct { - RefundID string - PaymentID string // refunds.payment_id — the parent payment row - Amount float64 - SquareRefundID string - IdempotencyKey string // deterministic "sweepdup-" + duplicate payment id (payments-table rows) - Reason string // carries the parent till_sale id for till_sale rows + RefundID string + PaymentID string // refunds.payment_id — the parent payment row + Amount float64 + SquareRefundID string + IdempotencyKey string // deterministic "sweepdup-" + duplicate payment id (payments-table rows) + Reason string // carries the parent till_sale id for till_sale rows SquarePaymentID string // synthetic till-sale payment row's square_payment_id ("" for payments-table rows) - CreatedBy string + CreatedBy string + CreatedAt time.Time // refunds.created_at — when the auto-refund was recorded (age for escalation) + BookingID string // parent payment's booking_id ("" when the row has none) } // sweepPendingB1Refunds re-polls refunds rows for sweep auto-refunds of @@ -1612,7 +1643,8 @@ type b1PendingRefund struct { func sweepPendingB1Refunds(ctx context.Context) (int, error) { rows, err := db.Conn.Query(ctx, ` SELECT r.id, r.payment_id, r.amount, r.square_refund_id, COALESCE(r.idempotency_key, ''), - r.reason, COALESCE(p.square_payment_id, ''), COALESCE(r.created_by, '') + r.reason, COALESCE(p.square_payment_id, ''), COALESCE(r.created_by, ''), + r.created_at, COALESCE(p.booking_id, '') FROM refunds r LEFT JOIN payments p ON p.id = r.payment_id WHERE r.status = 'pending' AND r.square_refund_id IS NOT NULL @@ -1627,7 +1659,7 @@ func sweepPendingB1Refunds(ctx context.Context) (int, error) { var pending []b1PendingRefund for rows.Next() { var pr b1PendingRefund - if err := rows.Scan(&pr.RefundID, &pr.PaymentID, &pr.Amount, &pr.SquareRefundID, &pr.IdempotencyKey, &pr.Reason, &pr.SquarePaymentID, &pr.CreatedBy); err != nil { + if err := rows.Scan(&pr.RefundID, &pr.PaymentID, &pr.Amount, &pr.SquareRefundID, &pr.IdempotencyKey, &pr.Reason, &pr.SquarePaymentID, &pr.CreatedBy, &pr.CreatedAt, &pr.BookingID); err != nil { log.Printf("Failed to scan B1 sweep auto-refund: %v", err) continue } @@ -1644,6 +1676,17 @@ func sweepPendingB1Refunds(ctx context.Context) (int, error) { processed := 0 for i := range pending { pr := &pending[i] + // A row already escalated (pending past stalePendingB1RefundAge) is no + // longer re-polled — it either resolved terminal (and left the pending + // query) or stays pending under the deduped CRITICAL notification for + // manual reconciliation. + b1EscalatedMu.Lock() + escalated := b1Escalated[pr.RefundID] + b1EscalatedMu.Unlock() + if escalated { + log.Printf("B1 refund %s was already escalated (pending over %s) — not re-polling; manual reconciliation holds the parent pending", pr.RefundID, stalePendingB1RefundAge) + continue + } // The Square payment id the refund targets. A till_sale's refund is // attached to the synthetic payments row (square_payment_id = the // duplicate charge); a payments-table refund is attached to the @@ -1669,19 +1712,33 @@ func sweepPendingB1Refunds(ctx context.Context) (int, error) { break } } + stale := clock.Now().Sub(pr.CreatedAt) > stalePendingB1RefundAge switch status { case "COMPLETED": if resolveB1RefundCompleted(ctx, pr) { processed++ } case "PENDING", "APPROVED": - log.Printf("B1 refund %s is still %q at Square — leaving the parent row pending", pr.RefundID, status) + if stale { + escalateStaleB1Refund(ctx, pr) + } else { + log.Printf("B1 refund %s is still %q at Square — leaving the parent row pending", pr.RefundID, status) + } case "": log.Printf("B1 refund %s (%s) was not found at Square via ListPaymentRefunds — leaving pending; manual reconciliation may be required", pr.RefundID, dupPayID) default: - // FAILED / REJECTED — the webhook FAILED-refund reconciliation - // (coordinated) owns this terminal state; never resolve it here. - log.Printf("B1 refund %s is %q at Square — leaving pending for the webhook FAILED-refund reconciliation", pr.RefundID, status) + // FAILED / REJECTED — terminal at Square. While the refund is young + // the webhook FAILED-refund reconciliation (OPTIONAL per README) + // owns it; once it has been pending past stalePendingB1RefundAge the + // sweep treats it as terminal itself so the parent can never be + // stranded by a missing webhook. + if stale { + if resolveB1RefundFailedTerminal(ctx, pr, status) { + processed++ + } + } else { + log.Printf("B1 refund %s is %q at Square — leaving pending for the webhook FAILED-refund reconciliation (or the %s age escalation)", pr.RefundID, status, stalePendingB1RefundAge) + } } } return processed, nil @@ -1699,13 +1756,51 @@ func resolveB1RefundCompleted(ctx context.Context, pr *b1PendingRefund) bool { `, pr.RefundID); err != nil { log.Printf("Failed to mark B1 refund %s completed after Square settle: %v", pr.RefundID, err) } + // A COMPLETED refund resolved the parent — clear any escalation flag so a + // restarted/re-polled row can be observed again if it ever re-enters. + b1EscalatedMu.Lock() + delete(b1Escalated, pr.RefundID) + b1EscalatedMu.Unlock() + return resolveB1ParentFailed(ctx, pr) +} + +// resolveB1RefundFailedTerminal resolves a B1 sweep auto-refund that Square +// reports FAILED/REJECTED once it has been pending longer than +// stalePendingB1RefundAge — the terminal treatment the webhook FAILED-refund +// reconciliation (OPTIONAL per README) would have applied when configured. The +// refund is marked failed, the parent resolved to failed (a till sale's funded +// gift card clawed back), and a deduped CRITICAL admin notification raised so +// the missing webhook can never strand the parent silently forever. Returns +// true when the parent was resolved. +func resolveB1RefundFailedTerminal(ctx context.Context, pr *b1PendingRefund, squareStatus string) bool { + if _, err := db.Conn.Exec(ctx, ` + UPDATE refunds SET status = 'failed' + WHERE id = $1 AND status = 'pending' + `, pr.RefundID); err != nil { + log.Printf("Failed to mark B1 refund %s failed after %q at Square: %v", pr.RefundID, squareStatus, err) + } + b1EscalatedMu.Lock() + b1Escalated[pr.RefundID] = true + b1EscalatedMu.Unlock() + insertB1EscalationNotification(ctx, pr) + resolved := resolveB1ParentFailed(ctx, pr) + log.Printf("B1 refund %s was %q at Square and pending over %s — marked the refund failed and resolved the parent; MANUAL RECONCILIATION REQUIRED: verify at Square whether the duplicate charge was refunded", pr.RefundID, squareStatus, stalePendingB1RefundAge) + return resolved +} + +// resolveB1ParentFailed resolves a B1 refund's parent row to failed after the +// refund settled definitively (COMPLETED — the duplicate was reversed — or +// FAILED/REJECTED past the age threshold). A payments-table refund's payment_id +// IS the pending parent row; a till_sale's id is encoded in the reason and its +// funded gift card is clawed back. Returns true when the parent was resolved. +func resolveB1ParentFailed(ctx context.Context, pr *b1PendingRefund) bool { if pr.Reason == sweepDuplicateRefundReason { // payments-table parent: the refund's payment_id IS the pending row. if failStaleRow(ctx, "payments", pr.PaymentID) { - log.Printf("B1 refund %s COMPLETED at Square — marked the pending payment %s failed (the duplicate charge was reversed)", pr.RefundID, pr.PaymentID) + log.Printf("B1 refund %s — marked the pending payment %s failed", pr.RefundID, pr.PaymentID) return true } - log.Printf("B1 refund %s COMPLETED at Square but payment %s was already resolved", pr.RefundID, pr.PaymentID) + log.Printf("B1 refund %s — payment %s was already resolved", pr.RefundID, pr.PaymentID) return false } // till_sale parent: the sale id is encoded in the reason. @@ -1713,28 +1808,62 @@ func resolveB1RefundCompleted(ctx context.Context, pr *b1PendingRefund) bool { tillSaleID := strings.TrimSuffix(pr.Reason[idx+len("(till_sale "):], ")") ts, ok := loadTillSaleStaleRow(ctx, tillSaleID) if !ok { - log.Printf("CRITICAL: B1 refund %s COMPLETED at Square but parent till sale %s could not be loaded — MANUAL RECONCILIATION REQUIRED: gift card may still be funded", pr.RefundID, tillSaleID) + log.Printf("CRITICAL: B1 refund %s — parent till sale %s could not be loaded — MANUAL RECONCILIATION REQUIRED: gift card may still be funded", pr.RefundID, tillSaleID) return false } if ts.HasGiftCard { if clawbackTillSaleFunding(ctx, ts) { - log.Printf("B1 refund %s COMPLETED at Square — clawed back till sale %s's funded gift card and marked it failed", pr.RefundID, tillSaleID) + log.Printf("B1 refund %s — clawed back till sale %s's funded gift card and marked it failed", pr.RefundID, tillSaleID) return true } - log.Printf("CRITICAL: B1 refund %s COMPLETED at Square but clawing back till sale %s's funding failed — MANUAL RECONCILIATION REQUIRED: gift card may still be funded", pr.RefundID, tillSaleID) + log.Printf("CRITICAL: B1 refund %s — clawing back till sale %s's funding failed — MANUAL RECONCILIATION REQUIRED: gift card may still be funded", pr.RefundID, tillSaleID) return false } if failStaleRow(ctx, "till_sales", tillSaleID) { - log.Printf("B1 refund %s COMPLETED at Square — marked till sale %s failed", pr.RefundID, tillSaleID) + log.Printf("B1 refund %s — marked till sale %s failed", pr.RefundID, tillSaleID) return true } - log.Printf("B1 refund %s COMPLETED at Square but till sale %s was already resolved", pr.RefundID, tillSaleID) + log.Printf("B1 refund %s — till sale %s was already resolved", pr.RefundID, tillSaleID) return false } - log.Printf("B1 refund %s COMPLETED at Square but the parent row could not be identified from reason %q — MANUAL RECONCILIATION REQUIRED", pr.RefundID, pr.Reason) + log.Printf("B1 refund %s — the parent row could not be identified from reason %q — MANUAL RECONCILIATION REQUIRED", pr.RefundID, pr.Reason) return false } +// escalateStaleB1Refund surfaces a B1 sweep auto-refund that has been PENDING +// (non-terminal) at Square for longer than stalePendingB1RefundAge: a deduped +// CRITICAL admin notification fires (the webhook FAILED-refund reconciliation +// is OPTIONAL — with it unconfigured nothing else alerts) and the row is no +// longer re-polled. The refund is left PENDING and the parent stays pending — +// marking failed on a non-terminal state could exclude money that may still +// move. +func escalateStaleB1Refund(ctx context.Context, pr *b1PendingRefund) { + b1EscalatedMu.Lock() + b1Escalated[pr.RefundID] = true + b1EscalatedMu.Unlock() + insertB1EscalationNotification(ctx, pr) + log.Printf("CRITICAL: B1 refund %s has been PENDING at Square for over %s — leaving the parent pending and STOPPING re-poll — MANUAL RECONCILIATION REQUIRED: verify the refund at Square and resolve the parent", pr.RefundID, stalePendingB1RefundAge) +} + +// insertB1EscalationNotification raises the deduped 'critical_payment_log' +// admin notification for an escalated B1 refund (insertCriticalPaymentNotification +// keeps ONE per booking/user until acknowledged). A payments-table refund is +// attributed to its parent payment's booking and the payer; a till-sale refund +// (synthetic payment row, no booking) is attributed to the payer only. +func insertB1EscalationNotification(ctx context.Context, pr *b1PendingRefund) { + var bookingID *string + if pr.BookingID != "" { + b := pr.BookingID + bookingID = &b + } + var userID *string + if pr.CreatedBy != "" { + u := pr.CreatedBy + userID = &u + } + insertCriticalPaymentNotification(ctx, bookingID, userID) +} + // loadTillSaleStaleRow reads a till_sale's gift-card context for the B1 // re-poll pass's clawback — the same fields fetchStaleRows/scanStaleRow // populate for the stale-pending sweep. @@ -2057,10 +2186,25 @@ func processManualPaymentGroup(ctx context.Context, paymentID string, rows []man } switch { case sqErr == nil: + // Resolve by Square's status: a synchronous refund response can be + // PENDING (money in flight, e.g. an async card network) — marking it + // completed while Square later fails it would permanently block that + // amount in the over-refund guard. Only a definitive COMPLETED + // resolves to completed; PENDING stays pending for the sweep to + // reconcile; FAILED/REJECTED is a real failure. Mirrors + // processChargeGroup and the RefundPayment handler (handlers.go). + sqStatus := "completed" + if sqResult.Status == "PENDING" { + sqStatus = "pending" + log.Printf("Square refund %s for manual refund %s is PENDING (in flight) — leaving the row pending for the sweep to resolve", sqResult.ID, pr.ID) + } else if sqResult.Status == "FAILED" || sqResult.Status == "REJECTED" { + sqStatus = "failed" + log.Printf("Square refund %s for manual refund %s FAILED — marking the row failed", sqResult.ID, pr.ID) + } if _, upErr := db.Conn.Exec(ctx, ` - UPDATE refunds SET status = 'completed', square_refund_id = $1 - WHERE id = $2 - `, sqResult.ID, pr.ID); upErr != nil { + UPDATE refunds SET status = $1, square_refund_id = $2 + WHERE id = $3 + `, sqStatus, sqResult.ID, pr.ID); upErr != nil { log.Printf("CRITICAL: Square refund committed (%s) but DB update for manual refund %s failed — manual reconciliation required: %v", sqResult.ID, pr.ID, upErr) } processed++ diff --git a/backend/handlers/payments/refunds_test.go b/backend/handlers/payments/refunds_test.go index 82509c2..b3603e0 100644 --- a/backend/handlers/payments/refunds_test.go +++ b/backend/handlers/payments/refunds_test.go @@ -4401,3 +4401,385 @@ func TestProcessChargeGroup_ReconcileError_AtCap_ReArmsAndNotifies(t *testing.T) maxConsecutiveReconcileFailures, critCount) } } + +// ============================================================================= +// MEDIUM 3 — B1 re-poll age escalation (webhook-optional strand fix) +// ============================================================================= + +// b1RePollStatusClient reports every refund Square holds for a payment as a +// SINGLE synthetic refund with a fixed id and status, so the B1 re-poll +// escalation branches are deterministic regardless of what the underlying mock +// stored. +type b1RePollStatusClient struct { + square.SquareClient + refundID string + status string +} + +func (c *b1RePollStatusClient) ListPaymentRefunds(ctx context.Context, paymentID string, beginTime time.Time) ([]square.RefundResult, error) { + return []square.RefundResult{{ + ID: c.refundID, + Status: c.status, + PaymentID: paymentID, + }}, nil +} + +// seedB1RefundAndPendingParent seeds a payments-table B1 sweep auto-refund row +// (status 'pending', square_refund_id set, the deterministic sweepdup key) on a +// still-pending parent payment, aged refundAgeHours back from now, and returns +// the parent payment id, the refund id and the booking id. +func seedB1RefundAndPendingParent(t *testing.T, ctx context.Context, tx db.Querier, userID, bookingID string, amount float64, squareRefundID, refundKey string, refundAgeHours int) (paymentID, refundID string) { + t.Helper() + pid, err := fixtures.CreateTestPayment(tx, bookingID, amount, "online_square", "full", "pending") + if err != nil { + t.Fatalf("failed to create pending parent payment: %v", err) + } + var rid string + err = tx.QueryRow(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, 'pending', 'manual', $5, $6, $7, NOW() - ($8 || ' hours')::interval) + RETURNING id + `, pid, bookingID, amount, squareRefundID, sweepDuplicateRefundReason, refundKey, userID, refundAgeHours).Scan(&rid) + if err != nil { + t.Fatalf("failed to insert B1 refund row: %v", err) + } + return pid, rid +} + +// TestSweepPendingB1Refunds_FailedPastAge_Terminal locks the MEDIUM 3 fix: a B1 +// sweep auto-refund Square reports FAILED once it has been pending longer than +// stalePendingB1RefundAge is terminal — the webhook FAILED-refund reconciliation +// is OPTIONAL (README), so with it unconfigured the sweep itself must fail the +// refund, resolve the parent payment and raise the CRITICAL admin notification +// instead of re-polling forever. +func TestSweepPendingB1Refunds_FailedPastAge_Terminal(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) + } + + const squareRefundID = "ref_b1_failed_terminal" + const refundKey = "sweepdup-pay_dup_terminal" + paymentID, refundID := seedB1RefundAndPendingParent(t, ctx, tx, userID, bookingID, 50.00, squareRefundID, refundKey, 49) + + 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 setup tx: %v", err) + } + + t.Cleanup(func() { + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM refunds WHERE id = $1`, refundID) + _, _ = 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`, paymentID) + _, _ = 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) + }) + + origClient := SquareClient + SquareClient = &b1RePollStatusClient{SquareClient: square.NewDevClient(), refundID: squareRefundID, status: "FAILED"} + defer func() { SquareClient = origClient }() + + freshCtx := context.Background() + if _, err := SweepPendingSquareRefunds(freshCtx); err != nil { + t.Fatalf("SweepPendingSquareRefunds failed: %v", err) + } + + var refundStatus string + if err := db.Conn.QueryRow(freshCtx, `SELECT status FROM refunds WHERE id = $1`, refundID).Scan(&refundStatus); err != nil { + t.Fatalf("failed to query refund: %v", err) + } + if refundStatus != "failed" { + t.Errorf("expected the stale FAILED B1 refund marked 'failed', got %q", refundStatus) + } + + // The parent payment must be resolved to failed — no longer stranded. + var paymentStatus string + if err := db.Conn.QueryRow(freshCtx, `SELECT status FROM payments WHERE id = $1`, paymentID).Scan(&paymentStatus); err != nil { + t.Fatalf("failed to query parent payment: %v", err) + } + if paymentStatus != "failed" { + t.Errorf("expected the parent payment resolved to 'failed', got %q", paymentStatus) + } + + 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 terminal FAILED B1 refund, got %d", notifCount) + } +} + +// TestSweepPendingB1Refunds_FailedYoung_StaysPending locks the pre-threshold +// behaviour: a B1 auto-refund Square reports FAILED while still younger than +// stalePendingB1RefundAge stays PENDING (the webhook FAILED-refund +// reconciliation owns it, and the age escalation has not yet applied) — the +// parent is NOT failed on a terminal Square status before the threshold. +func TestSweepPendingB1Refunds_FailedYoung_StaysPending(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) + } + + const squareRefundID = "ref_b1_failed_young" + const refundKey = "sweepdup-pay_dup_young" + paymentID, refundID := seedB1RefundAndPendingParent(t, ctx, tx, userID, bookingID, 50.00, squareRefundID, refundKey, 2) + + 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 setup tx: %v", err) + } + + t.Cleanup(func() { + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM refunds WHERE id = $1`, refundID) + _, _ = 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`, paymentID) + _, _ = 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) + }) + + origClient := SquareClient + SquareClient = &b1RePollStatusClient{SquareClient: square.NewDevClient(), refundID: squareRefundID, status: "FAILED"} + defer func() { SquareClient = origClient }() + + freshCtx := context.Background() + if _, err := SweepPendingSquareRefunds(freshCtx); err != nil { + t.Fatalf("SweepPendingSquareRefunds failed: %v", err) + } + + var refundStatus string + if err := db.Conn.QueryRow(freshCtx, `SELECT status FROM refunds WHERE id = $1`, refundID).Scan(&refundStatus); err != nil { + t.Fatalf("failed to query refund: %v", err) + } + if refundStatus != "pending" { + t.Errorf("expected the young FAILED B1 refund to stay 'pending' (webhook owns it until the age escalation), got %q", refundStatus) + } + var paymentStatus string + if err := db.Conn.QueryRow(freshCtx, `SELECT status FROM payments WHERE id = $1`, paymentID).Scan(&paymentStatus); err != nil { + t.Fatalf("failed to query parent payment: %v", err) + } + if paymentStatus != "pending" { + t.Errorf("expected the parent payment untouched while the refund is young, got %q", paymentStatus) + } +} + +// TestSweepPendingB1Refunds_PendingPastAge_EscalatedStopsRepoll locks the MEDIUM 3 +// age cap: a B1 auto-refund still PENDING past stalePendingB1RefundAge is +// escalated (a deduped CRITICAL admin notification fires) and no longer +// re-polled — a second run with Square now reporting COMPLETED must NOT resolve +// the parent, proving the escalation stopped the re-poll. The refund stays +// PENDING (never marked failed on a non-terminal state; never completed without +// a COMPLETED Square refund). +func TestSweepPendingB1Refunds_PendingPastAge_EscalatedStopsRepoll(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) + } + + const squareRefundID = "ref_b1_pending_esc" + const refundKey = "sweepdup-pay_dup_pending_esc" + paymentID, refundID := seedB1RefundAndPendingParent(t, ctx, tx, userID, bookingID, 50.00, squareRefundID, refundKey, 49) + + 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 setup tx: %v", err) + } + + t.Cleanup(func() { + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM refunds WHERE id = $1`, refundID) + _, _ = 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`, paymentID) + _, _ = 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) + }) + + origClient := SquareClient + defer func() { SquareClient = origClient }() + + freshCtx := context.Background() + // Run 1: Square still reports PENDING past the age threshold → escalation. + SquareClient = &b1RePollStatusClient{SquareClient: square.NewDevClient(), refundID: squareRefundID, status: "PENDING"} + if _, err := SweepPendingSquareRefunds(freshCtx); err != nil { + t.Fatalf("SweepPendingSquareRefunds run 1 failed: %v", err) + } + + var refundStatus, paymentStatus string + if err := db.Conn.QueryRow(freshCtx, `SELECT status FROM refunds WHERE id = $1`, refundID).Scan(&refundStatus); err != nil { + t.Fatalf("failed to query refund: %v", err) + } + if refundStatus != "pending" { + t.Errorf("expected the escalated refund to stay 'pending' (never failed on a non-terminal state), got %q", refundStatus) + } + if err := db.Conn.QueryRow(freshCtx, `SELECT status FROM payments WHERE id = $1`, paymentID).Scan(&paymentStatus); err != nil { + t.Fatalf("failed to query parent payment: %v", err) + } + if paymentStatus != "pending" { + t.Errorf("expected the parent payment to stay pending after escalation, got %q", paymentStatus) + } + 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 escalated refund, got %d", notifCount) + } + + // Run 2: Square would now report COMPLETED — but the row was escalated, so + // it is no longer re-polled and the parent must stay pending (a re-poll here + // would have resolved it). This proves the cap stopped the re-poll. + SquareClient = &b1RePollStatusClient{SquareClient: square.NewDevClient(), refundID: squareRefundID, status: "COMPLETED"} + if _, err := SweepPendingSquareRefunds(freshCtx); err != nil { + t.Fatalf("SweepPendingSquareRefunds run 2 failed: %v", err) + } + if err := db.Conn.QueryRow(freshCtx, `SELECT status FROM refunds WHERE id = $1`, refundID).Scan(&refundStatus); err != nil { + t.Fatalf("failed to re-query refund: %v", err) + } + if refundStatus != "pending" { + t.Errorf("expected the escalated refund to NOT be re-polled (status stays 'pending'), got %q", refundStatus) + } + if err := db.Conn.QueryRow(freshCtx, `SELECT status FROM payments WHERE id = $1`, paymentID).Scan(&paymentStatus); err != nil { + t.Fatalf("failed to re-query parent payment: %v", err) + } + if paymentStatus != "pending" { + t.Errorf("expected the parent payment untouched after the escalated row was skipped, got %q", paymentStatus) + } +} + +// ============================================================================= +// DRIFT-REAL — synchronous-PENDING manual refund must stay pending +// ============================================================================= + +// TestSweepManualRefund_SyncPendingResponse_LeavesPending locks the DRIFT-REAL +// fix: when the sweep re-issues a manual refund and Square returns PENDING +// synchronously (money in flight, e.g. an async card network), the refund row +// must be left 'pending' — never 'completed' (a later Square failure would +// permanently block the amount in the over-refund guard). Mirrors the +// processChargeGroup / RefundPayment handler status handling. +func TestSweepManualRefund_SyncPendingResponse_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) + } + + paymentID, err := fixtures.CreateTestPayment(tx, bookingID, 50.00, "online_square", "full", "completed") + if err != nil { + t.Fatalf("failed to create card payment: %v", err) + } + if _, err := tx.Exec(ctx, "UPDATE payments SET square_payment_id = 'sqp_manual_pending_sync' WHERE id = $1", paymentID); err != nil { + t.Fatalf("failed to set square_payment_id: %v", err) + } + + storedKey := paymentID + "-refund-5000" + var refundID string + err = tx.QueryRow(ctx, ` + INSERT INTO refunds (payment_id, booking_id, amount, status, reason, idempotency_key, origin, created_at) + VALUES ($1, $2, 50, 'pending', 'customer request', $3, 'manual', NOW()) + RETURNING id + `, paymentID, bookingID, storedKey).Scan(&refundID) + if err != nil { + t.Fatalf("failed to insert stale manual pending refund: %v", err) + } + + 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 booking_id = $1`, bookingID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM refunds WHERE id = $1`, refundID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM payments WHERE id = $1`, paymentID) + _, _ = 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) + }) + + origClient := SquareClient + // pendingRefundClient forces the synchronous RefundPayment response to + // PENDING (defined in sweep_test.go). + SquareClient = &pendingRefundClient{SquareClient: square.NewDevClient()} + defer func() { SquareClient = origClient }() + + freshCtx := context.Background() + // The sweep processes the whole shared test database — clear any pending + // rows left by earlier sequential tests so the re-issue below is the only + // one acting on this payment. + if _, err := db.Conn.Exec(freshCtx, `DELETE FROM refunds WHERE status = 'pending' AND id <> $1`, refundID); err != nil { + t.Fatalf("failed to clean leftover pending refunds: %v", err) + } + if _, err := SweepPendingSquareRefunds(freshCtx); err != nil { + t.Fatalf("SweepPendingSquareRefunds failed: %v", err) + } + + var status string + var squareRefundID *string + err = db.Conn.QueryRow(freshCtx, `SELECT status, square_refund_id FROM refunds WHERE id = $1`, refundID).Scan(&status, &squareRefundID) + if err != nil { + t.Fatalf("failed to query refund: %v", err) + } + if status != "pending" { + t.Errorf("expected a synchronous-PENDING Square refund to leave the row 'pending', got %q", status) + } + if squareRefundID == nil || *squareRefundID == "" { + t.Error("expected square_refund_id to be set (Square holds the refund)") + } +} diff --git a/backend/handlers/payments/sweep.go b/backend/handlers/payments/sweep.go index 9df2417..4478461 100644 --- a/backend/handlers/payments/sweep.go +++ b/backend/handlers/payments/sweep.go @@ -683,6 +683,20 @@ func clawbackTillSaleFunding(ctx context.Context, r staleRow) bool { // created no later than the moment the sweep could first have replayed). const replayLegitimateRetryWindow = 22 * time.Hour +// replayRescueLowerBoundSkew is the lower-bound tolerance for a replayed +// COMPLETED payment to still be treated as the ORIGINAL charge under a retained +// key rather than a provably-new duplicate. Rows are inserted pending-first, so +// natural order is row.CreatedAt < Square's created_at by ~0.5-1s — and the DB +// clock can run AHEAD of Square's (independent NTP drift, VM pause/resume), +// which makes a retained-key dedup return the ORIGINAL charge with created < +// r.CreatedAt. Without this tolerance the sweep would declare that original +// payment a "new charge" and auto-refund a charge the customer legitimately +// authorized. Anything at or before row.CreatedAt is therefore treated as +// AMBIGUOUS (never auto-refunded, never rescued) — see +// reconcileStalePaymentByKey. A payment created AFTER row.CreatedAt + +// replayLegitimateRetryWindow remains the provable expired-key duplicate. +const replayRescueLowerBoundSkew = time.Minute + // replayMatchesRowAmount reports whether the replayed payment charged the same // amount the pending row records — the amount the sweep's replay body repeats // and the amount any same-key retry MUST reuse (the retry path rejects a @@ -755,7 +769,10 @@ func parseReplayedCreatedAt(pr *square.PaymentResult) (time.Time, bool) { // replayed payment that cannot be proven to be the original (or a retry // within the legitimate window) is never rescued (the row stays pending, a // CRITICAL log is raised and an admin notification inserted), so a hidden -// second charge can never masquerade as the original one. +// second charge can never masquerade as the original one. The caller further +// refuses to auto-refund a payment created BEFORE the row (replayRescueLowerBoundSkew +// tolerance — a DB clock ahead of Square's can make the retained-key original +// look slightly older): such a payment is not provably a new charge. // // The check runs ONLY against real Square timestamps: it is gated off in an // explicit dev/mock env because the dev mock returns payments whose CreatedAt @@ -954,6 +971,23 @@ func reconcileStalePaymentByKey(ctx context.Context, table string, r staleRow) ( 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) } + // B1 (MEDIUM 2): a replayed COMPLETED payment created BEFORE the + // pending row is NOT provably a new expired-key replay — those land + // ~22h AFTER the row. A retained-key dedup returns the ORIGINAL + // charge, and a DB clock running AHEAD of Square's (rows are + // inserted pending-first; independent NTP drift, VM pause/resume) + // can make that original's created_at lag the row's by up to + // replayRescueLowerBoundSkew. Auto-refunding it would reverse a + // legitimate payment the customer authorized, so any before-row + // created_at is ambiguous: the row is left PENDING with a CRITICAL + // notification for manual reconciliation. + if created.Before(r.CreatedAt) { + skew := "well before" + if !created.Before(r.CreatedAt.Add(-replayRescueLowerBoundSkew)) { + skew = "slightly before (within the clock-skew tolerance)" + } + return leavePendingCritical(ctx, r, "stale pending %s reconcile by key: the replayed COMPLETED payment %s was created %s the pending row %s — cannot prove it is a NEW expired-key replay (a DB clock ahead of Square's can make a retained-key original look slightly older) — leaving row %s PENDING — MANUAL RECONCILIATION REQUIRED: verify at Square whether this is a second charge before refunding", table, pr.ID, skew, r.ID, r.ID) + } 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 diff --git a/backend/handlers/payments/sweep_test.go b/backend/handlers/payments/sweep_test.go index a5abef9..073a01c 100644 --- a/backend/handlers/payments/sweep_test.go +++ b/backend/handlers/payments/sweep_test.go @@ -1261,6 +1261,115 @@ func TestSweepStalePendingPayments_KeyedReplayOriginalPayment_Rescues(t *testing } } +// TestSweepStalePendingPayments_KeyedReplayCreatedBeforeRow_LeavesPending locks +// the B1 lower-bound clock-skew tolerance: a replayed COMPLETED payment created +// slightly BEFORE the pending row must NOT be auto-refunded as a "new charge". +// Rows are inserted pending-first (row.CreatedAt precedes Square's created_at by +// ~0.5-1s), and a DB clock running AHEAD of Square's (independent NTP drift, VM +// pause/resume) makes a retained-key dedup return the ORIGINAL charge with +// created < row.CreatedAt. Such a payment cannot be proven to be a new +// expired-key replay (those land ~22h after the row), so auto-refunding it would +// reverse a legitimate charge — the row is left PENDING with a CRITICAL +// notification instead. Sequential (flips SQUARE_ENVIRONMENT), like the sibling +// B1/A1 tests. +func TestSweepStalePendingPayments_KeyedReplayCreatedBeforeRow_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-retained-clock-skew" + if _, err := tx.Exec(ctx, "UPDATE payments SET created_at = NOW() - INTERVAL '23 hours', idempotency_key = $1, square_source_id = 'ccof:test-saved-card', created_by = $2 WHERE id = $3", key, userID, staleID); err != nil { + t.Fatalf("failed to age the stale payment: %v", err) + } + + // The replayed COMPLETED payment is the ORIGINAL charge under a retained + // key whose created_at lags the DB row's by 2s — the DB clock running ahead + // of Square's. Seeding from the row's own timestamp (minus 2s) keeps the + // lag deterministic. + var rowCreatedAt time.Time + if err := tx.QueryRow(ctx, "SELECT created_at FROM payments WHERE id = $1", staleID).Scan(&rowCreatedAt); err != nil { + t.Fatalf("failed to read aged payment created_at: %v", err) + } + skewedCreated := rowCreatedAt.Add(-2 * time.Second) + + origClient := SquareClient + mock := square.NewDevClient() + t.Setenv("SQUARE_ENVIRONMENT", "production") + SquareClient = &staleReplayClient{SquareClient: mock, result: &square.PaymentResult{ + Status: "COMPLETED", + ID: "pay_original_skewed", + SquarePayID: "pay_original_skewed", + CreatedAt: skewedCreated.Format(time.RFC3339Nano), + }} + defer func() { SquareClient = origClient }() + + pgxTx := db.TxFromContext(ctx) + if pgxTx == nil { + t.Fatal("no transaction in context") + } + if err := pgxTx.Commit(ctx); err != nil { + t.Fatalf("failed to commit test tx: %v", err) + } + + t.Cleanup(func() { + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM refunds WHERE payment_id = $1`, staleID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM admin_notifications WHERE reason = 'critical_payment_log' AND user_id = $1`, userID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM payments WHERE id = $1`, staleID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM users WHERE id = $1`, userID) + }) + + freshCtx := context.Background() + if _, err := SweepStalePendingPayments(freshCtx); err != nil { + t.Fatalf("sweep failed: %v", err) + } + + // Ambiguous (created before the row): the row must stay PENDING — never + // auto-refunded, never rescued. + 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 a before-row replay to leave the payment pending, got %q", status) + } + + // No auto-refund may have been issued for the ambiguous payment. + var refundCount int + if err := db.Conn.QueryRow(freshCtx, `SELECT COUNT(*) FROM refunds WHERE payment_id = $1`, staleID).Scan(&refundCount); err != nil { + t.Fatalf("failed to count refunds: %v", err) + } + if refundCount != 0 { + t.Errorf("expected NO auto-refund of an ambiguous before-row payment, got %d refunds rows", refundCount) + } + + // A critical-payment admin notification must surface the manual reconciliation. + 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 ambiguous replay, got %d", notifCount) + } +} + // TestSweepStalePendingPayments_KeyedReplayRetryAt215h_Rescues locks the B2 // dead-zone fix: a same-key retry whose charge landed 21.5h after the pending // row (between the old 21h window and the 22h sweep cutoff) is the REAL charge diff --git a/backend/handlers/scheduling/scheduled-cleanup.go b/backend/handlers/scheduling/scheduled-cleanup.go index f5bc51a..a91d19b 100644 --- a/backend/handlers/scheduling/scheduled-cleanup.go +++ b/backend/handlers/scheduling/scheduled-cleanup.go @@ -8,6 +8,7 @@ import ( "log/slog" "time" + "crussell/auth" "crussell/clock" "crussell/db" @@ -229,7 +230,7 @@ func CleanupExpiredVerificationCodes(ctx context.Context) (int, error) { } // CleanupExpiredRefreshTokens deletes expired refresh tokens and -// revoked tokens older than 90 days. +// revoked tokens older than the shared RefreshTokenLifetime window (auth). func CleanupExpiredRefreshTokens(ctx context.Context) (int, error) { tx, err := db.Conn.Begin(ctx) if err != nil { @@ -244,8 +245,8 @@ func CleanupExpiredRefreshTokens(ctx context.Context) (int, error) { result, err := tx.Exec(ctx, ` DELETE FROM refresh_tokens WHERE expires_at < NOW() - OR (revoked = TRUE AND created_at < NOW() - INTERVAL '90 days') - `) + OR (revoked = TRUE AND created_at < NOW() - make_interval(days => $1)) + `, int64(auth.RefreshTokenLifetime/(24*time.Hour))) if err != nil { return 0, fmt.Errorf("failed to cleanup refresh tokens: %w", err) } diff --git a/backend/handlers/user/profile_test.go b/backend/handlers/user/profile_test.go index 175e824..e2037cb 100644 --- a/backend/handlers/user/profile_test.go +++ b/backend/handlers/user/profile_test.go @@ -165,7 +165,7 @@ func TestPasswordChange_RevokesTokens(t *testing.T) { if err != nil { t.Fatalf("failed to generate token: %v", err) } - refreshToken, err := auth.GenerateRefreshToken(ctx, userID, "verified_email") + refreshToken, _, err := auth.GenerateRefreshToken(ctx, userID, "verified_email") if err != nil { t.Fatalf("failed to generate refresh token: %v", err) } diff --git a/backend/handlers/user/twofa.go b/backend/handlers/user/twofa.go index 177c26d..b7437f3 100644 --- a/backend/handlers/user/twofa.go +++ b/backend/handlers/user/twofa.go @@ -508,7 +508,7 @@ func SendDisableCodeHandler(w http.ResponseWriter, r *http.Request) { st.Mu.Lock() defer st.Mu.Unlock() - if _, err := ensurePendingTwoFACode(r, userID, st, "disable 2FA"); err != nil { + if _, _, err := ensurePendingTwoFACode(r, userID, st, "disable 2FA"); err != nil { if errors.Is(err, errTwoFAMintThrottled) { http.Error(w, "Too many attempts. Wait before requesting a new code.", http.StatusTooManyRequests) return @@ -542,10 +542,15 @@ func SendDisableCodeHandler(w http.ResponseWriter, r *http.Request) { // gated in production). Fresh-code mints are throttled per-user // (twoFAMintCooldown) and never reset the failed-attempt counter (B11b). // -// Contract: 200 {"message":"Code sent"} (+ a dev-only "code" field when 2FA is -// unenforced, matching setup); 409 when the user has not enabled 2FA; 429 on -// the mint cooldown; 503 when no delivery channel is configured (production -// without TWO_FACTOR_ALLOW_LOG_DELIVERY=true); 500 on DB failure. The route is +// Contract: 200 {"message":"Code sent","remaining_seconds":N} (+ a dev-only +// "code" field when 2FA is unenforced, matching setup) where N is how many +// seconds the effective pending code stays valid — the FULL twoFAPendingExpiry +// after a fresh mint, or the decremented lifetime when an existing valid code +// was reused (LOW 5). A 200 with a reused code must NOT be read as "a new code +// was sent": the frontend should use the already-delivered code and show the +// countdown. 409 when the user has not enabled 2FA; 429 on the mint cooldown; +// 503 when no delivery channel is configured (production without +// TWO_FACTOR_ALLOW_LOG_DELIVERY=true); 500 on DB failure. The route is // mounted with RequireAuth + RequireNonGuest + the shared per-user 2FA limiter // (plus the group's per-IP limiter), so an enabled user cannot hammer code // requests faster than the surface budget. @@ -575,7 +580,7 @@ func SendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { st.Mu.Lock() defer st.Mu.Unlock() - code, err := ensurePendingTwoFACode(r, userID, st, "saved-card charge") + code, remaining, err := ensurePendingTwoFACode(r, userID, st, "saved-card charge") if err != nil { if errors.Is(err, errTwoFAMintThrottled) { http.Error(w, "Too many attempts. Wait before requesting a new code.", http.StatusTooManyRequests) @@ -593,7 +598,11 @@ func SendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { return } - resp := map[string]any{"message": "Code sent"} + // remaining_seconds tells the client how much longer the (possibly reused) + // pending code stays valid, so a button that would otherwise toast "Code + // sent" while no NEW code was minted can instead show a countdown / keep + // the existing code (LOW 5). + resp := map[string]any{"message": "Code sent", "remaining_seconds": int(remaining.Seconds())} if !twoFARequired() && code != "" { // Dev convenience (matches setup): return the freshly minted code so // the request path is testable without grepping the backend log. The @@ -658,7 +667,7 @@ func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { // The per-user mint cooldown still bounds how often a fresh code can be // minted — at most one per twoFAMintCooldown — but it cannot grant a fresh // guessing budget. - if _, err := ensurePendingTwoFACode(r, userID, st, "disable 2FA"); err != nil { + if _, _, err := ensurePendingTwoFACode(r, userID, st, "disable 2FA"); err != nil { if errors.Is(err, errTwoFAMintThrottled) { http.Error(w, "Too many attempts. Wait before requesting a new code.", http.StatusTooManyRequests) return @@ -712,9 +721,13 @@ func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { // It returns the plaintext code only when a FRESH code was minted and // delivered (dev/test builds always deliver it; production builds only when // the operator opted into log delivery — see twofa_prod.go). When a valid -// pending code was reused, the return is empty: only the digest is stored, so -// the plaintext is unavailable. Callers must only expose the returned code in -// unenforced environments (matching SetupTwoFAHandler's dev convenience). +// pending code was reused, the returned code is empty: only the digest is +// stored, so the plaintext is unavailable. Callers must only expose the +// returned code in unenforced environments (matching SetupTwoFAHandler's dev +// convenience). The remaining lifetime of the effective pending code (the +// reused one, or the fresh mint's full twoFAPendingExpiry) is always returned +// so a caller can surface "code still valid for N seconds" instead of implying +// a fresh code was sent (LOW 5). // // 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 @@ -726,7 +739,7 @@ func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { // who exhausts the budget must wait out the window, not the mint cooldown. A // failed delivery does not start the cooldown (the stamp is written only after // the UPDATE persisted). -func ensurePendingTwoFACode(r *http.Request, userID string, st *twoFAAttemptState, purpose string) (string, error) { +func ensurePendingTwoFACode(r *http.Request, userID string, st *twoFAAttemptState, purpose string) (string, time.Duration, error) { var pendingHash sql.NullString var pendingExpires sql.NullTime err := db.Conn.QueryRow(r.Context(), ` @@ -735,21 +748,21 @@ func ensurePendingTwoFACode(r *http.Request, userID string, st *twoFAAttemptStat WHERE id = $1 `, userID).Scan(&pendingHash, &pendingExpires) if err != nil { - return "", err + return "", 0, err } if pendingHash.Valid && pendingExpires.Valid && pendingExpires.Time.After(clock.Now()) { - return "", nil + return "", pendingExpires.Time.Sub(clock.Now()), nil } now := clock.Now() if !st.LastMintAt.IsZero() && now.Sub(st.LastMintAt) < twoFAMintCooldown { - return "", errTwoFAMintThrottled + return "", 0, errTwoFAMintThrottled } code, err := deliverTwoFACode(r, userID, "", purpose) if err != nil { - return "", err + return "", 0, err } st.LastMintAt = now - return code, nil + return code, twoFAPendingExpiry, nil } // disableTwoFA clears two_factor_enabled and the method + pending code fields. diff --git a/backend/handlers/user/twofa_test.go b/backend/handlers/user/twofa_test.go index 47fa238..e2e214f 100644 --- a/backend/handlers/user/twofa_test.go +++ b/backend/handlers/user/twofa_test.go @@ -1275,6 +1275,7 @@ func TestTwoFASendVerificationCode_Enabled_MintsFresh(t *testing.T) { require.Equal(t, "Code sent", resp["message"]) _, hasCode := resp["code"] require.False(t, hasCode, "enforced env must NOT return the code in the response") + require.Equal(t, float64(600), resp["remaining_seconds"], "a fresh mint must report the full 10-minute lifetime (LOW 5)") var pendingHash sql.NullString var expires sql.NullTime @@ -1302,6 +1303,16 @@ func TestTwoFASendVerificationCode_ReusesValidPendingCode(t *testing.T) { w := performUser2FARequest(t, SendVerificationCodeHandler, ctx, http.MethodPost, "/api/user/2fa/code", nil, userID) require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + var resp map[string]any + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + require.Equal(t, "Code sent", resp["message"]) + // LOW 5: the reused pending code's remaining lifetime is reported so the + // client knows a NEW code was NOT minted and can count down the existing one. + rem, ok := resp["remaining_seconds"].(float64) + require.True(t, ok, "response must include remaining_seconds") + require.Greater(t, rem, 0.0, "reused code must still have lifetime remaining") + require.LessOrEqual(t, rem, 600.0, "reused code lifetime must not exceed the 10-minute window") + var pendingHash sql.NullString require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&pendingHash)) require.True(t, pendingHash.Valid) diff --git a/backend/internal/jobs/cleanup.go b/backend/internal/jobs/cleanup.go index 126b608..27b638d 100644 --- a/backend/internal/jobs/cleanup.go +++ b/backend/internal/jobs/cleanup.go @@ -267,11 +267,14 @@ func RegisterAll(s *Scheduler) { }) } -// SweepSquareWebhookEvents deletes square_webhook_events rows older than 90 -// days. Every accepted webhook event_id is stored permanently for restart-safe -// dedup (payment.updated fires on every payment update), so without retention -// the table would grow without bound. 90 days comfortably exceeds Square's -// webhook replay window while keeping the table bounded. +// SweepSquareWebhookEvents deletes square_webhook_events rows older than the +// shared auth.RefreshTokenLifetime window (90 days). Every accepted webhook +// event_id is stored permanently for restart-safe dedup (payment.updated fires +// on every payment update), so without retention the table would grow without +// bound. 90 days comfortably exceeds Square's webhook replay window while +// keeping the table bounded. The 90-day window deliberately reuses +// auth.RefreshTokenLifetime so the two coinciding retention windows cannot +// drift apart. func SweepSquareWebhookEvents(ctx context.Context) (int, error) { tx, err := db.Conn.Begin(ctx) if err != nil { @@ -285,8 +288,8 @@ func SweepSquareWebhookEvents(ctx context.Context) (int, error) { tag, err := tx.Exec(ctx, ` DELETE FROM square_webhook_events - WHERE received_at < NOW() - INTERVAL '90 days' - `) + WHERE received_at < NOW() - make_interval(days => $1) + `, int64(auth.RefreshTokenLifetime/(24*time.Hour))) if err != nil { return 0, fmt.Errorf("failed to sweep square webhook events: %w", err) } diff --git a/backend/internal/twofa/twofa.go b/backend/internal/twofa/twofa.go index 5c4d305..b156dde 100644 --- a/backend/internal/twofa/twofa.go +++ b/backend/internal/twofa/twofa.go @@ -65,9 +65,20 @@ const MaxAttempts = 5 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. +// user IDs cannot grow it without bound. +// +// ACCEPTED LIMITATION (LOW 6 — documentation only, no behavior change): every +// 2FA counter here — the per-user failed-attempt count, the lockout window, and +// the mint-cooldown stamp (AttemptState.LastMintAt, used by twoFAMintCooldown +// in handlers/user) — is purely in-memory and resets on process restart. The +// DB schema is locked (there is no attempt column), and the practical impact +// is bounded by the 10-minute pending-code expiry (AttemptWindow / +// twoFAPendingExpiry): at most one fresh 5-guess budget per 10-minute window. +// A MULTI-INSTANCE deployment would need a shared store (e.g. a DB column or +// Redis) for these counters, because today each instance keeps its own map — +// an attacker could distribute guesses across instances. Single-instance +// deployments (this app) are unaffected. +// // Declared as a var so the eviction policy is unit-testable at a small cap. var MaxTrackedAttempts = 10_000 diff --git a/frontend/src/lib/components/admin/TillPurchases.svelte b/frontend/src/lib/components/admin/TillPurchases.svelte index cdb67bb..892da7f 100644 --- a/frontend/src/lib/components/admin/TillPurchases.svelte +++ b/frontend/src/lib/components/admin/TillPurchases.svelte @@ -14,6 +14,7 @@ submitPaymentWithRetry } from '$lib/square/square'; import { authStore } from '$lib/stores/auth.svelte'; + import { useTwoFactorCodeForSavedCard } from '$lib/stores/twoFactorCode.svelte'; type CartItem = { id: string; @@ -112,22 +113,16 @@ // 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. + // are unaffected. Shared two-factor-code state (code, reveal, show/missing + // derivations, "Request a new code" handler) — see + // $lib/stores/twoFactorCode.svelte.ts. The admin always supplies the + // CUSTOMER's code — the admin's own 2FA flag is irrelevant to the backend + // gate, so `enabled` is always true. 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() === ''); + const twoFactor = useTwoFactorCodeForSavedCard({ + enabled: () => true, + gateActive: () => savedCardChargeRequires2FACode && paymentMethod === 'saved_card' + }); // The saved-card option is hidden outright unless a customer is selected // AND has at least one currently-valid card on file. @@ -327,7 +322,7 @@ 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; + if (twoFactor.showInput) body.verification_code = twoFactor.code; } else if (paymentMethod === 'online_square') { if (!onlineSquareCardInput) { throw new Error('Card form is not ready — please wait a moment and try again'); @@ -367,14 +362,14 @@ toast.success('Sale complete'); cart = []; idempotencyKeys.clear(); - twoFactorCode = ''; - reveal2FACodeInput = false; + twoFactor.setCode(''); + twoFactor.reveal = 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; + if (isTwoFactorVerificationGateFailure(responseStatus, msg)) twoFactor.reveal = true; paymentError = msg; toast.error(msg); } finally { @@ -775,7 +770,23 @@ - + + {#if twoFactor.showInput} + + {/if} {/if} @@ -798,7 +809,7 @@ loading={processing} disabled={!canCharge || processing || - missing2FACode || + twoFactor.missing || (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 851aaa5..4fc7169 100644 --- a/frontend/src/lib/components/booking/BookingFlow.svelte +++ b/frontend/src/lib/components/booking/BookingFlow.svelte @@ -48,6 +48,7 @@ } from '$lib/square/square'; import UserPaymentModal from '$lib/components/payments/UserPaymentModal.svelte'; import { extractBookedSlots, getLunchProtectionForSlots } from '$lib/lunchProtection'; + import { useTwoFactorCodeForSavedCard } from '$lib/stores/twoFactorCode.svelte'; import { formatLocalDateTime, getLondonTodayCalendarDate, @@ -149,27 +150,16 @@ // 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. + // keeps its own SCA via Square tokenizeWithVerification. Shared + // two-factor-code state (code, reveal, show/missing derivations, "Request a + // new code" handler) — see $lib/stores/twoFactorCode.svelte.ts. 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 twoFactor = useTwoFactorCodeForSavedCard({ + enabled: () => twoFactorEnabled, + gateActive: () => + savedCardChargeRequires2FACode && (selectedPaymentMethod !== '' || depositSaveCard) + }); const depositCardFormValid = $derived(paymentCardSelectionValid); @@ -431,7 +421,7 @@ ...(selectedPaymentMethod ? { card_id: selectedPaymentMethod } : {}), ...(newCardToken ? { new_card_token: newCardToken, save_card: depositSaveCard } : {}), ...(verificationToken ? { verification_token: verificationToken } : {}), - ...(show2FACodeInput ? { verification_code: depositTwoFactorCode } : {}) + ...(twoFactor.showInput ? { verification_code: twoFactor.code } : {}) }; paymentAttempted = true; @@ -502,8 +492,8 @@ depositTokenizedAt = 0; depositTokenizedForSaveCard = false; depositSaveCard = false; - depositTwoFactorCode = ''; - reveal2FACodeInput = false; + twoFactor.setCode(''); + twoFactor.reveal = false; overflowConfirm = null; // Immutable update — avoid mutating the existing object so // concurrent renders (e.g. a stale fetch) can't observe partial @@ -524,7 +514,7 @@ // 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; + twoFactor.reveal = true; } // Pre-start overpayment guard on stale booking data: park the rejected // request (body + amount) and surface the Confirm/Cancel prompt instead @@ -2661,44 +2651,56 @@ /> {/if} - {:else} -
- (paymentCardSelectionValid = v)} - /> -
- {/if} + {:else} +
+ (paymentCardSelectionValid = v)} + /> +
+ {/if} - -
- -
+
+ + {#if twoFactor.showInput && twoFactorEnabled} + + {/if} +
-
- - -
+
+ + +

Secure payment powered by Square diff --git a/frontend/src/lib/components/payments/TipPayment.svelte b/frontend/src/lib/components/payments/TipPayment.svelte index f9a8ca9..eb0cf60 100644 --- a/frontend/src/lib/components/payments/TipPayment.svelte +++ b/frontend/src/lib/components/payments/TipPayment.svelte @@ -7,6 +7,7 @@ import { parseWallClockDate } from '$lib/utils/timeSlots'; import CardSelection from '$lib/components/payments/CardSelection.svelte'; import { savedCardsStore, type SavedCard } from '$lib/stores/savedCards.svelte'; + import { useTwoFactorCodeForSavedCard } from '$lib/stores/twoFactorCode.svelte'; import { Button } from '$lib/components/ui/button'; import { Input } from '$lib/components/ui/input'; import * as Card from '$lib/components/ui/card'; @@ -17,7 +18,6 @@ isNonceStale, isSavedCardVerificationRequired, isTwoFactorVerificationGateFailure, - requestNewTwoFactorCode, sanitizeDecimalInput, SAVED_CARD_VERIFICATION_MESSAGE, submitPaymentWithRetry @@ -108,51 +108,15 @@ // B6/B10: saved-card tips (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. + // keeps its own SCA via Square tokenizeWithVerification. Shared + // two-factor-code state (code, reveal, show/missing derivations, "Request a + // new code" handler) — see $lib/stores/twoFactorCode.svelte.ts. const savedCardChargeRequires2FACode = $derived(authStore.savedCardChargeRequires2FACode); const twoFactorEnabled = $derived(!!authStore.currentUser?.twoFactorEnabled); - - // B6/B10: verification code for a saved-card tip / 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 twoFactorCode = $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 && (selectedCardId !== '' || saveCard)) - ); - const missing2FACode = $derived( - show2FACodeInput && twoFactorEnabled && twoFactorCode.trim() === '' - ); - - // POST /api/user/2fa/code mint state for the "Request a new code" button - // (session user = card owner, so a minted code authorizes their charge). - let requesting2FACode = $state(false); - async function handleRequestNew2FACode() { - if (requesting2FACode) return; - requesting2FACode = true; - try { - const result = await requestNewTwoFactorCode(); - if (result.ok) { - twoFactorCode = ''; - toast.success(result.message); - } else if (result.status === 429) { - toast.error(result.message || 'Too many requests. Wait before requesting a new code.'); - } else if (result.status === 503) { - toast.error( - result.message || 'Verification codes are unavailable right now. Try again later.' - ); - } else { - toast.error(result.message); - } - } finally { - requesting2FACode = false; - } - } + const twoFactor = useTwoFactorCodeForSavedCard({ + enabled: () => twoFactorEnabled, + gateActive: () => savedCardChargeRequires2FACode && (selectedCardId !== '' || saveCard) + }); const isCardValid = $derived(cardSelectionValid); @@ -335,7 +299,7 @@ ...(selectedCardId ? { card_id: selectedCardId } : {}), ...(newCardToken ? { new_card_token: newCardToken, save_card: saveCard } : {}), ...(verificationToken ? { verification_token: verificationToken } : {}), - ...(show2FACodeInput ? { verification_code: twoFactorCode } : {}) + ...(twoFactor.showInput ? { verification_code: twoFactor.code } : {}) }; const response = await submitPaymentWithRetry(() => @@ -361,8 +325,8 @@ tipTokenAmount = 0; tipTokenizedAt = 0; tipTokenizedForSaveCard = false; - twoFactorCode = ''; - reveal2FACodeInput = false; + twoFactor.setCode(''); + twoFactor.reveal = false; toast.success('Thank you for your tip!'); onSuccess?.(); } catch (err) { @@ -379,7 +343,7 @@ // code, brute-force lockout) is recoverable — keep the code populated // and reveal the input so the tip can be retried with a fresh code. if (isTwoFactorVerificationGateFailure(responseStatus, errorMessage)) { - reveal2FACodeInput = true; + twoFactor.reveal = true; } toast.error(errorMessage); // A definitive charge failure (e.g. declined card) consumes the nonce @@ -539,18 +503,18 @@ - {#if show2FACodeInput && twoFactorEnabled} + {#if twoFactor.showInput && twoFactorEnabled} @@ -569,7 +533,7 @@ @@ -1084,7 +1045,7 @@ onclick={() => (paymentType === 'deposit' ? handlePayDeposit() : handlePayFull())} class="w-full" loading={status === 'processing'} - disabled={payButtonDisabled || missing2FACode} + disabled={payButtonDisabled || twoFactor.missing} > {#if paymentType === 'deposit'} Pay Deposit ({formatCurrency( @@ -1165,7 +1126,7 @@ onclick={() => (paymentType === 'partial' ? handlePayPartial() : handlePayFull())} class="w-full" loading={status === 'processing'} - disabled={payButtonDisabled || missing2FACode} + disabled={payButtonDisabled || twoFactor.missing} > {#if paymentType === 'partial'} Pay {partialAmountValid diff --git a/frontend/src/lib/stores/twoFactorCode.svelte.ts b/frontend/src/lib/stores/twoFactorCode.svelte.ts new file mode 100644 index 0000000..111d7d9 --- /dev/null +++ b/frontend/src/lib/stores/twoFactorCode.svelte.ts @@ -0,0 +1,99 @@ +// src/lib/stores/twoFactorCode.svelte.ts +import { toast } from 'svelte-sonner'; +import { requestNewTwoFactorCode } from '$lib/square/square'; + +/** + * Shared 2FA verification-code state for the saved-card charge surfaces. + * + * B6/B10: the backend's requireTwoFactorForCardAccess gate requires the CARD + * OWNER's current one-time verification code on every saved-card charge in an + * enforced environment. This composable owns the whole verification-code UX — + * the code itself, the reveal flag (a charge that 403s for a missing code + * reveals the input even when the session profile's 2FA flag is stale), the + * show/missing derivations and the "Request a new code" handler — so the six + * payment surfaces (booking modal, tip, account gift-card, booking-flow + * deposit, admin till and admin payment modal) can't drift on any of them. + * + * Each surface supplies its own predicates: + * - `enabled()` — whether the session user's own 2FA is active (customer + * surfaces: `!!authStore.currentUser?.twoFactorEnabled`). + * Admin surfaces (till, admin payment modal) return true: + * the operator always supplies the CUSTOMER's code, so the + * session user's own flag is irrelevant to the gate. + * - `gateActive()` — whether the pending charge hits the 2FA gate: a saved + * card is selected, or a new card is being saved for reuse. + * The surface passes its exact gate expression so each + * surface's gate semantics are preserved verbatim. + */ +export function useTwoFactorCodeForSavedCard(options: { + enabled: () => boolean; + gateActive: () => boolean; +}) { + // Kept populated across retries so an invalid/expired code can be corrected + // without re-typing it. + let code = $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. Revealing the input makes the failure recoverable. + let reveal = $state(false); + // POST /api/user/2fa/code mint state for the "Request a new code" button + // (session user = card owner, so a minted code authorizes their charge). + let requesting = $state(false); + + // Show the code input whenever the pending charge hits the backend's 2FA + // gate: charging a saved card OR saving the new card for reuse. + const showInput = $derived(reveal || options.gateActive()); + const missing = $derived(showInput && options.enabled() && code.trim() === ''); + + async function requestNewCode() { + if (requesting) return; + requesting = true; + try { + const result = await requestNewTwoFactorCode(); + if (result.ok) { + code = ''; + toast.success(result.message); + } else if (result.status === 429) { + toast.error(result.message || 'Too many requests. Wait before requesting a new code.'); + } else if (result.status === 503) { + toast.error( + result.message || 'Verification codes are unavailable right now. Try again later.' + ); + } else { + toast.error(result.message); + } + } finally { + requesting = false; + } + } + + function setCode(value: string) { + code = value; + } + + return { + get code() { + return code; + }, + set code(value: string) { + code = value; + }, + setCode, + get reveal() { + return reveal; + }, + set reveal(value: boolean) { + reveal = value; + }, + get showInput() { + return showInput; + }, + get missing() { + return missing; + }, + get requesting() { + return requesting; + }, + requestNewCode + }; +} diff --git a/frontend/src/routes/account/+page.svelte b/frontend/src/routes/account/+page.svelte index a220d84..6b78681 100644 --- a/frontend/src/routes/account/+page.svelte +++ b/frontend/src/routes/account/+page.svelte @@ -15,10 +15,11 @@ isSavedCardVerificationRequired, isSquareConfigured, isTwoFactorVerificationGateFailure, - requestNewTwoFactorCode, SAVED_CARD_VERIFICATION_MESSAGE, submitPaymentWithRetry } from '$lib/square/square'; + import { useTwoFactorCodeForSavedCard } from '$lib/stores/twoFactorCode.svelte'; + import { generateUUID } from '$lib/utils/uuid'; import { extractErrorMessage, sanitizeText } from '$lib/utils/toast-safe'; import { apiFetch } from '$lib/utils/api'; import UserBookingModal from '$lib/components/account/UserBookingModal.svelte'; @@ -251,50 +252,15 @@ // B6/B10: gift-card buys charge a saved card (or save a new card for reuse) // whenever the backend enforces the 2FA gate — the CARD OWNER's current - // verification code must be carried on the charge. Mirrors the customer - // surface (UserPaymentModal). Kept populated across retries so an - // invalid/expired code can be corrected without re-typing it. - let buyTwoFactorCode = $state(''); - // Set true when a charge 403s for a missing code — reveals the input even - // if the session profile's 2FA flag is stale, making the failure - // recoverable. - let buyReveal2FACodeInput = $state(false); + // verification code must be carried on the charge. Shared two-factor-code + // state (code, reveal, show/missing derivations, "Request a new code" + // handler) — see $lib/stores/twoFactorCode.svelte.ts. const buyTwoFactorEnabled = $derived(!!authStore.currentUser?.twoFactorEnabled); const buySavedCardChargeRequires2FACode = $derived(authStore.savedCardChargeRequires2FACode); - // Show the code input whenever the pending charge hits the backend's 2FA - // gate: charging a saved card OR saving the new card for reuse. - const buyShow2FACodeInput = $derived( - buyReveal2FACodeInput || - (buySavedCardChargeRequires2FACode && (buySelectedCard !== '' || buySaveCard)) - ); - const buyMissing2FACode = $derived( - buyShow2FACodeInput && buyTwoFactorEnabled && buyTwoFactorCode.trim() === '' - ); - - // POST /api/user/2fa/code mint state for the "Request a new code" button - // (session user = card owner, so a minted code authorizes their charge). - let buyRequesting2FACode = $state(false); - async function handleBuyRequestNew2FACode() { - if (buyRequesting2FACode) return; - buyRequesting2FACode = true; - try { - const result = await requestNewTwoFactorCode(); - if (result.ok) { - buyTwoFactorCode = ''; - toast.success(result.message); - } else if (result.status === 429) { - toast.error(result.message || 'Too many requests. Wait before requesting a new code.'); - } else if (result.status === 503) { - toast.error( - result.message || 'Verification codes are unavailable right now. Try again later.' - ); - } else { - toast.error(result.message); - } - } finally { - buyRequesting2FACode = false; - } - } + const buyTwoFactor = useTwoFactorCodeForSavedCard({ + enabled: () => buyTwoFactorEnabled, + gateActive: () => buySavedCardChargeRequires2FACode && (buySelectedCard !== '' || buySaveCard) + }); // Client-side mirror of the £500/day online purchase cap. The backend is // authoritative — this counter only reflects confirmed purchases made in @@ -496,7 +462,7 @@ // (where the charge actually landed) would double-charge. const cardKey = cardId || 'new-card'; if (!buyIdempotencyKey || buyKeyedAmount !== buyAmount || buyKeyedCard !== cardKey) { - buyIdempotencyKey = generateIdempotencyKey(); + buyIdempotencyKey = generateUUID(); buyKeyedAmount = buyAmount; buyKeyedCard = cardKey; } @@ -512,7 +478,7 @@ ...(cardId ? { card_id: cardId } : {}), ...(newCardToken ? { new_card_token: newCardToken, save_card: buySaveCard } : {}), ...(verificationToken ? { verification_token: verificationToken } : {}), - ...(buyShow2FACodeInput ? { verification_code: buyTwoFactorCode } : {}), + ...(buyTwoFactor.showInput ? { verification_code: buyTwoFactor.code } : {}), idempotency_key: buyIdempotencyKey }) }) @@ -533,8 +499,8 @@ buyTokenAmount = 0; buyTokenizedAt = 0; buyTokenizedForSaveCard = false; - buyTwoFactorCode = ''; - buyReveal2FACodeInput = false; + buyTwoFactor.setCode(''); + buyTwoFactor.reveal = false; await fetchGiftCardBalance(); } else { // Capture the status BEFORE consuming the body — the saved-card @@ -553,7 +519,7 @@ ? SAVED_CARD_VERIFICATION_MESSAGE : extractErrorMessage(errText) || 'Failed to purchase gift card'; if (isTwoFactorVerificationGateFailure(status, buyErrMsg)) { - buyReveal2FACodeInput = true; + buyTwoFactor.reveal = true; } toast.error(buyErrMsg); // A definitive charge failure (e.g. declined card) consumes the @@ -670,24 +636,6 @@ }); } - function generateIdempotencyKey(): string { - const array = new Uint8Array(16); - if (typeof window !== 'undefined' && window.crypto) { - window.crypto.getRandomValues(array); - } else { - for (let i = 0; i < 16; i++) array[i] = Math.floor(Math.random() * 256); - } - array[6] = (array[6] & 0x0f) | 0x40; - array[8] = (array[8] & 0x3f) | 0x80; - return [...array] - .map((b, i) => { - const hex = b.toString(16).padStart(2, '0'); - if (i === 4 || i === 6 || i === 8 || i === 10) return '-' + hex; - return hex; - }) - .join(''); - } - async function fetchNotifPrefs() { try { const res = await apiFetch('/api/user/notification-preferences'); @@ -2633,18 +2581,18 @@ - {#if buyShow2FACodeInput && buyTwoFactorEnabled} + {#if buyTwoFactor.showInput && buyTwoFactorEnabled} @@ -2655,7 +2603,7 @@ onclick={buyGiftCard} disabled={buyingGiftCard || !isBuyCardValid || - buyMissing2FACode || + buyTwoFactor.missing || buyDailyTotal + buyAmount > DAILY_GIFT_CARD_BUY_LIMIT} class="mt-2 w-full" >