fix(web): count login attempts per address and while they are checked

The login limiter counted failures per (address, username) only after the check finished, so attempts sent together all passed before the first failure was recorded, and one address could own every record of the bounded table and push out the one holding its own failures. An admitted attempt now holds a slot until it reports back, and an address is blocked as a whole after five times the per-username limit spread over any names, which also caps the records one peer can own.
2026-11-02 11:15:03 +03:00 · 2026-10-31 09:04:23 +00:00
parent 661a169045
commit 8e8541a5d8
2 changed files with 130 additions and 33 deletions
@@ -14,6 +14,9 @@ const (
 	// caller-supplied username, so an unauthenticated attacker rotating
 	// usernames would otherwise grow the map without bound.
 	loginLimitMaxRecords = 10000
+	// Failures one address may spread over all usernames, as a multiple of the per-username
+	// limit, before the address is blocked as a whole: bounds the records one peer can own.
+	loginLimitIPFactor = 5
 )

 var defaultLoginLimiter = newLoginLimiter(loginLimitMaxFailures, loginLimitWindow, loginLimitCooldown)
@@ -25,10 +28,14 @@ type loginLimiter struct {
 	window      time.Duration
 	cooldown    time.Duration
 	attempts    map[string]*loginLimitRecord
+	ipAttempts  map[string]*loginLimitRecord
 }

+// loginLimitRecord counts failures and admitted attempts still being checked: a burst
+// sent at once would otherwise all pass before bcrypt reports the first failure.
 type loginLimitRecord struct {
 	failures     []time.Time
+	inFlight     []time.Time
 	blockedUntil time.Time
 }

@@ -39,27 +46,47 @@ func newLoginLimiter(maxFailures int, window, cooldown time.Duration) *loginLimi
 		window:      window,
 		cooldown:    cooldown,
 		attempts:    make(map[string]*loginLimitRecord),
+		ipAttempts:  make(map[string]*loginLimitRecord),
 	}
 }

+// allow admits an attempt and reserves a slot for it in both the (ip, username) and the
+// address-wide record; the caller reports the outcome through registerFailure or registerSuccess.
 func (l *loginLimiter) allow(ip, username string) (time.Time, bool) {
 	l.mu.Lock()
 	defer l.mu.Unlock()

-	key := loginLimitKey(ip, username)
-	record := l.attempts[key]
-	if record == nil {
-		return time.Time{}, true
-	}
 	now := l.now()
+	ipKey := strings.TrimSpace(ip)
+	if blockedUntil, ok := l.admit(l.ipAttempts, ipKey, l.maxFailures*loginLimitIPFactor, now); !ok {
+		return blockedUntil, false
+	}
+	if blockedUntil, ok := l.admit(l.attempts, loginLimitKey(ip, username), l.maxFailures, now); !ok {
+		l.ipAttempts[ipKey].dropReservation()
+		return blockedUntil, false
+	}
+	return time.Time{}, true
+}
+
+// admit refuses a blocked key, or one whose failures and pending attempts already fill the
+// budget (starting its cooldown), and otherwise reserves a slot. Callers hold l.mu.
+func (l *loginLimiter) admit(records map[string]*loginLimitRecord, key string, limit int, now time.Time) (time.Time, bool) {
+	record := records[key]
+	if record == nil {
+		l.evictForRoom(records, now)
+		record = &loginLimitRecord{}
+		records[key] = record
+	}
 	if now.Before(record.blockedUntil) {
 		return record.blockedUntil, false
 	}
 	record.blockedUntil = time.Time{}
-	record.failures = pruneLoginFailures(record.failures, now.Add(-l.window))
-	if len(record.failures) == 0 {
-		delete(l.attempts, key)
+	record.prune(now.Add(-l.window))
+	if len(record.failures)+len(record.inFlight) >= limit {
+		record.block(now.Add(l.cooldown))
+		return record.blockedUntil, false
 	}
+	record.inFlight = append(record.inFlight, now)
 	return time.Time{}, true
 }

@@ -68,18 +95,26 @@ func (l *loginLimiter) registerFailure(ip, username string) (time.Time, bool) {
 	defer l.mu.Unlock()

 	now := l.now()
-	key := loginLimitKey(ip, username)
-	record := l.attempts[key]
-	if record == nil {
-		l.evictForRoom(now)
-		record = &loginLimitRecord{}
-		l.attempts[key] = record
+	ipBlockedUntil, ipBlocked := l.fail(l.ipAttempts, strings.TrimSpace(ip), l.maxFailures*loginLimitIPFactor, now)
+	blockedUntil, blocked := l.fail(l.attempts, loginLimitKey(ip, username), l.maxFailures, now)
+	if blocked {
+		return blockedUntil, true
 	}
-	record.failures = pruneLoginFailures(record.failures, now.Add(-l.window))
+	return ipBlockedUntil, ipBlocked
+}
+
+func (l *loginLimiter) fail(records map[string]*loginLimitRecord, key string, limit int, now time.Time) (time.Time, bool) {
+	record := records[key]
+	if record == nil {
+		l.evictForRoom(records, now)
+		record = &loginLimitRecord{}
+		records[key] = record
+	}
+	record.prune(now.Add(-l.window))
+	record.dropReservation()
 	record.failures = append(record.failures, now)
-	if len(record.failures) >= l.maxFailures {
-		record.failures = nil
-		record.blockedUntil = now.Add(l.cooldown)
+	if len(record.failures) >= limit {
+		record.block(now.Add(l.cooldown))
 		return record.blockedUntil, true
 	}
 	return time.Time{}, false
@@ -89,39 +124,59 @@ func (l *loginLimiter) registerSuccess(ip, username string) {
 	l.mu.Lock()
 	defer l.mu.Unlock()
 	delete(l.attempts, loginLimitKey(ip, username))
+	if record := l.ipAttempts[strings.TrimSpace(ip)]; record != nil {
+		record.dropReservation()
+	}
 }

-// evictForRoom keeps the attempts map bounded before inserting a new record.
+func (r *loginLimitRecord) prune(cutoff time.Time) {
+	r.failures = pruneLoginFailures(r.failures, cutoff)
+	r.inFlight = pruneLoginFailures(r.inFlight, cutoff)
+}
+
+func (r *loginLimitRecord) block(until time.Time) {
+	r.failures = nil
+	r.inFlight = nil
+	r.blockedUntil = until
+}
+
+func (r *loginLimitRecord) dropReservation() {
+	if r != nil && len(r.inFlight) > 0 {
+		r.inFlight = r.inFlight[1:]
+	}
+}
+
+// evictForRoom keeps a records map bounded before inserting a new record.
 // It first reclaims records that are no longer blocked and whose failures have
 // aged out of the window; if the map is still at the ceiling (a genuine
-// broad flood), it drops one arbitrary record so memory can never grow past the
-// cap. Callers hold l.mu.
-func (l *loginLimiter) evictForRoom(now time.Time) {
-	if len(l.attempts) < loginLimitMaxRecords {
+// broad flood), it drops one record that is neither blocked nor awaiting an
+// outcome, then any, so memory can never grow past the cap. Callers hold l.mu.
+func (l *loginLimiter) evictForRoom(records map[string]*loginLimitRecord, now time.Time) {
+	if len(records) < loginLimitMaxRecords {
 		return
 	}
 	cutoff := now.Add(-l.window)
-	for key, record := range l.attempts {
+	for key, record := range records {
 		if now.Before(record.blockedUntil) {
 			continue
 		}
-		record.failures = pruneLoginFailures(record.failures, cutoff)
-		if len(record.failures) == 0 {
-			delete(l.attempts, key)
+		record.prune(cutoff)
+		if len(record.failures) == 0 && len(record.inFlight) == 0 {
+			delete(records, key)
 		}
 	}
-	if len(l.attempts) < loginLimitMaxRecords {
+	if len(records) < loginLimitMaxRecords {
 		return
 	}
-	for key, record := range l.attempts {
-		if now.Before(record.blockedUntil) {
+	for key, record := range records {
+		if now.Before(record.blockedUntil) || len(record.inFlight) > 0 {
 			continue
 		}
-		delete(l.attempts, key)
+		delete(records, key)
 		return
 	}
-	for key := range l.attempts {
-		delete(l.attempts, key)
+	for key := range records {
+		delete(records, key)
 		return
 	}
 }
@@ -118,3 +118,45 @@ func TestLoginLimiterSeparatesIPAndUsername(t *testing.T) {
 		t.Fatal("different username should not be blocked")
 	}
 }
+
+// Without a per-address bound, one address can fill the table with its own blocked
+// records and evict the record holding its admin failures, resetting the count.
+func TestLoginLimiterBlocksAnAddressSpreadingFailuresOverUsernames(t *testing.T) {
+	now := time.Date(2026, 5, 6, 12, 0, 0, 0, time.UTC)
+	limiter := newLoginLimiter(5, 5*time.Minute, 15*time.Minute)
+	limiter.now = func() time.Time { return now }
+
+	for i := range 5 * loginLimitIPFactor {
+		if _, ok := limiter.allow("192.0.2.10", "decoy-"+strconv.Itoa(i)); !ok {
+			t.Fatalf("attempt %d refused before the address limit", i+1)
+		}
+		limiter.registerFailure("192.0.2.10", "decoy-"+strconv.Itoa(i))
+	}
+	if _, ok := limiter.allow("192.0.2.10", "admin"); ok {
+		t.Fatal("an address past its failure budget may still try the admin account")
+	}
+	if _, ok := limiter.allow("192.0.2.11", "admin"); !ok {
+		t.Fatal("another address was blocked too")
+	}
+}
+
+// The count must cover attempts admitted and still being checked: bcrypt is slow, and
+// a burst sent at once would otherwise all pass before the first failure is recorded.
+func TestLoginLimiterCountsAttemptsStillBeingChecked(t *testing.T) {
+	now := time.Date(2026, 5, 6, 12, 0, 0, 0, time.UTC)
+	limiter := newLoginLimiter(5, 5*time.Minute, 15*time.Minute)
+	limiter.now = func() time.Time { return now }
+
+	for i := range 5 {
+		if _, ok := limiter.allow("192.0.2.10", "admin"); !ok {
+			t.Fatalf("attempt %d of a burst refused", i+1)
+		}
+	}
+	if _, ok := limiter.allow("192.0.2.10", "admin"); ok {
+		t.Fatal("a sixth concurrent attempt was admitted")
+	}
+	now = now.Add(20 * time.Minute)
+	if _, ok := limiter.allow("192.0.2.10", "admin"); !ok {
+		t.Fatal("attempts that never reported back still block after the window and cooldown")
+	}
+}