From dfcfd1d3e2e16ffad7a74d26e60f7280cd9fdb45 Mon Sep 17 00:00:00 2001
From: mrhid6 <mrhid6@gmail.com>
Date: Wed, 16 Sep 2026 08:37:07 +0000
Subject: [PATCH] fix(mfa): make ticket attempt count atomic via Redis INCR

---
 server/internal/auth/mfaticket.go      | 48 ++++++++++++++++++--------
 server/internal/auth/mfaticket_test.go | 14 ++++++++
 2 files changed, 47 insertions(+), 15 deletions(-)

diff --git a/server/internal/auth/mfaticket.go b/server/internal/auth/mfaticket.go
index 3fbe906..5f29822 100644
--- a/server/internal/auth/mfaticket.go
+++ b/server/internal/auth/mfaticket.go
@@ -57,6 +57,14 @@ func attemptsLeft(attempts int) int {
 	return maxTicketAttempts - attempts
 }
 
+// attemptsKey is the counter backing a ticket's brute-force cap. It is a
+// separate key rather than a field on the ticket JSON so INCR can make the
+// count atomic: two requests racing on the same ticket must each cost one
+// attempt, not both read the same count and both write count+1.
+func attemptsKey(id string) string {
+	return ticketPrefix + id + ":attempts"
+}
+
 func CreateTicket(ctx context.Context, t *Ticket) (string, error) {
 	id, err := randomHex(32)
 	if err != nil {
@@ -87,31 +95,41 @@ func LoadTicket(ctx context.Context, id string) (*Ticket, error) {
 	return &t, nil
 }
 
-// FailTicket records a wrong code and returns how many attempts remain. At zero
-// the ticket is destroyed rather than left to time out.
+// FailTicket records a wrong code and returns how many attempts remain. The
+// count is kept in its own INCR-backed key rather than the ticket JSON: a
+// read-modify-write on the JSON lets requests racing on the same ticket all
+// read the same count and all write count+1, which bypasses the cap instead
+// of costing one attempt each. At zero the ticket is destroyed rather than
+// left to time out.
 func FailTicket(ctx context.Context, id string) (int, error) {
-	t, err := LoadTicket(ctx, id)
+	// Confirms the ticket exists first, so a missing/expired/destroyed ticket
+	// still answers with the one indistinguishable ErrTicketExpired rather
+	// than incrementing a counter for an id nobody holds.
+	if _, err := LoadTicket(ctx, id); err != nil {
+		return 0, err
+	}
+	key := attemptsKey(id)
+	n, err := rdb.Incr(ctx, key).Result()
 	if err != nil {
 		return 0, err
 	}
-	t.Attempts++
-	if attemptsLeft(t.Attempts) == 0 {
+	if n == 1 {
+		// Only the creator of the counter sets its expiry, so a later
+		// increment never extends it past the ticket's own window.
+		if err := rdb.Expire(ctx, key, ticketTTL).Err(); err != nil {
+			return 0, err
+		}
+	}
+	left := attemptsLeft(int(n))
+	if left == 0 {
 		_ = DeleteTicket(ctx, id)
 		return 0, nil
 	}
-	data, err := json.Marshal(t)
-	if err != nil {
-		return 0, err
-	}
-	// KEEPTTL: a wrong code must not extend the five-minute window.
-	if err := rdb.Set(ctx, ticketPrefix+id, data, redis.KeepTTL).Err(); err != nil {
-		return 0, err
-	}
-	return attemptsLeft(t.Attempts), nil
+	return left, nil
 }
 
 func DeleteTicket(ctx context.Context, id string) error {
-	return rdb.Del(ctx, ticketPrefix+id).Err()
+	return rdb.Del(ctx, ticketPrefix+id, attemptsKey(id)).Err()
 }
 
 func SetPendingCookie(c *gin.Context, id string) {
diff --git a/server/internal/auth/mfaticket_test.go b/server/internal/auth/mfaticket_test.go
index f608737..757671b 100644
--- a/server/internal/auth/mfaticket_test.go
+++ b/server/internal/auth/mfaticket_test.go
@@ -19,6 +19,20 @@ func TestAttemptsLeftCountsDownAndHitsZero(t *testing.T) {
 	}
 }
 
+// attemptsKey must derive deterministically from the ticket id and stay
+// distinct from the ticket's own key, since FailTicket relies on INCR against
+// it being the sole writer of the attempt count.
+func TestAttemptsKeyIsDerivedFromTicketID(t *testing.T) {
+	got := attemptsKey("abc123")
+	want := "km:mfa:abc123:attempts"
+	if got != want {
+		t.Errorf("attemptsKey(%q) = %q, want %q", "abc123", got, want)
+	}
+	if attemptsKey("abc123") == ticketPrefix+"abc123" {
+		t.Error("attempts key must not collide with the ticket's own key")
+	}
+}
+
 // An enrol-only ticket exists because the instance requires MFA the user does
 // not have. It must not satisfy a verification endpoint, and a verification
 // ticket must not reach the enrolment endpoints - each would skip the other's