Bound registrations with a per-IP quota instead of failure backoff (B-58)
The registration throttle called record_failure on every attempt, successful ones included. Five legitimate signups from one shared or NAT address locked the sixth real user out for up to 600s, doubling from there — while an attacker sidestepped the limiter entirely through B-54. Failure backoff is the wrong instrument here: nothing about creating an account is a failed guess at a secret, so the only people it reliably punished were the honest ones. RollingQuota says what was actually meant: 5 accounts per IP per hour, in a rolling window. The caller over it waits exactly until the oldest of the five ages out — an accurate Retry-After, and waiting never makes the next wait longer. It is recorded only once an account exists, so attempts that create nothing (a taken username, a validation error) leave the quota untouched, and checked before the Argon2 hash, so an IP out of quota costs nothing to refuse. Bounded like the failure limiter (B-56): the keys are caller-chosen, so the dict gets both a sweep and a hard cap, evicting keys with room left in their quota before full ones. Also fixes the inline comment that cited B-31 (the resubscribe finding) where it meant B-33. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
+84
-1
@@ -110,6 +110,84 @@ class RateLimiter:
|
||||
self._buckets.pop(key, None)
|
||||
|
||||
|
||||
class RollingQuota:
|
||||
"""How many times a key may do something in a rolling window — as opposed to
|
||||
RateLimiter above, which punishes *failures* with a growing delay.
|
||||
|
||||
B-58: registration was throttled with the failure limiter, and recorded a
|
||||
"failure" on every attempt, successful ones included. Five legitimate signups
|
||||
from one shared or NAT address locked the sixth real user out for up to 600s,
|
||||
with the backoff doubling from there — while an attacker sidestepped the whole
|
||||
thing through B-54. The intent (bound how many accounts one source can create)
|
||||
is right; failure backoff is the wrong instrument for it, since nothing here is
|
||||
a failed guess at a secret. A quota says exactly what is meant: this many
|
||||
accounts per source per window, and the answer to the one over it is "not yet",
|
||||
with an accurate wait rather than a punishment that grows.
|
||||
"""
|
||||
|
||||
def __init__(
|
||||
self,
|
||||
limit: int,
|
||||
window_seconds: float,
|
||||
max_keys: int = _MAX_BUCKETS,
|
||||
sweep_interval_seconds: float = _SWEEP_INTERVAL_SECONDS,
|
||||
) -> None:
|
||||
self._limit = limit
|
||||
self._window_seconds = window_seconds
|
||||
self._max_keys = max_keys
|
||||
self._sweep_interval_seconds = sweep_interval_seconds
|
||||
self._events: dict[str, list[float]] = {}
|
||||
self._last_sweep_at = time.monotonic()
|
||||
|
||||
def _live_events(self, key: str, now: float) -> list[float]:
|
||||
"""The key's events still inside the window, pruned in place."""
|
||||
events = self._events.get(key)
|
||||
if events is None:
|
||||
return []
|
||||
cutoff = now - self._window_seconds
|
||||
while events and events[0] <= cutoff:
|
||||
events.pop(0)
|
||||
if not events:
|
||||
del self._events[key]
|
||||
return events
|
||||
|
||||
def _prune(self, now: float) -> None:
|
||||
# Same bound as RateLimiter (B-56): the keys are caller-chosen, so the dict
|
||||
# needs both a sweep and a hard cap. Eviction order is likewise "closest to
|
||||
# leaving the window first" — dropping a key with room left in its quota
|
||||
# changes nothing, dropping a full one hands out free accounts.
|
||||
for key in list(self._events):
|
||||
self._live_events(key, now)
|
||||
self._last_sweep_at = now
|
||||
|
||||
excess = len(self._events) - self._max_keys
|
||||
if excess > 0:
|
||||
by_oldest = sorted(self._events.items(), key=lambda item: item[1][-1])
|
||||
for key, _ in by_oldest[:excess]:
|
||||
del self._events[key]
|
||||
|
||||
def retry_after(self, key: str) -> float:
|
||||
"""Seconds until this key may act again — 0 while it is under quota."""
|
||||
now = time.monotonic()
|
||||
events = self._live_events(key, now)
|
||||
if len(events) < self._limit:
|
||||
return 0.0
|
||||
return events[0] + self._window_seconds - now
|
||||
|
||||
def record(self, key: str) -> None:
|
||||
"""Counts one *completed* action. Attempts that create nothing (a taken
|
||||
username, a validation error) deliberately don't consume the quota — the
|
||||
limit is on accounts that exist, not on requests."""
|
||||
now = time.monotonic()
|
||||
if now - self._last_sweep_at >= self._sweep_interval_seconds or len(self._events) > self._max_keys:
|
||||
self._prune(now)
|
||||
self._events.setdefault(key, []).append(now)
|
||||
|
||||
|
||||
_REGISTRATIONS_PER_IP = 5
|
||||
_REGISTRATION_WINDOW_SECONDS = 3600.0
|
||||
|
||||
|
||||
class AuthRateLimiters:
|
||||
"""The three throttles B-33 needs, bundled so they can live on `app.state`
|
||||
(like `UserLocks`, see app/tx/locks.py) rather than as module globals.
|
||||
@@ -124,4 +202,9 @@ class AuthRateLimiters:
|
||||
def __init__(self) -> None:
|
||||
self.login = RateLimiter(threshold=5, base_delay=2.0, max_delay=300.0)
|
||||
self.login_ip = RateLimiter(threshold=20, base_delay=2.0, max_delay=300.0)
|
||||
self.register_ip = RateLimiter(threshold=5, base_delay=5.0, max_delay=600.0)
|
||||
# B-58: a quota, not failure backoff — 5 accounts per IP per hour. The one
|
||||
# over it waits only until the oldest of the five ages out, and a busy NAT
|
||||
# is slowed rather than locked out for progressively longer.
|
||||
self.register_ip = RollingQuota(
|
||||
limit=_REGISTRATIONS_PER_IP, window_seconds=_REGISTRATION_WINDOW_SECONDS
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user