diff --git a/BUGS.md b/BUGS.md index 0a470ef..1a9d1d6 100644 --- a/BUGS.md +++ b/BUGS.md @@ -40,17 +40,6 @@ remains the last prerequisite for running unattended. ## High — security -### B-56 — `RateLimiter._buckets` is never pruned - -`app/auth/rate_limit.py:37`. - -The dict grows without bound, keyed by attacker-chosen strings (arbitrary -usernames, and — via B-54 — arbitrary IPs). `decay_seconds` ages a bucket's -*counter* but never removes the entry. - -Fix: evict entries whose last failure is older than `decay_seconds` (opportunistically -on `record_failure`, or on a periodic sweep), and cap the dict size. - ### B-57 — username matching is case-sensitive while the login throttle key is not `app/auth/routes.py:126` (`body.username.lower()`) vs `:132` diff --git a/CLAUDE.md b/CLAUDE.md index 891ea64..9549b70 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,7 +8,7 @@ The user communicates in Italian in chat — reply to them in Italian. Everythin ## Project status -All 10 stages of the original build order are code-complete and unit-tested — 281 tests, all under `tests/unit/` (`tests/integration/` is an empty package). Beyond them: Docker + Caddy deployment, admin dashboard (`/admin`), static test UI (`/`), pending-inclusive balance display, an SSE push channel layered over the original polling, self-service password change + admin password reset, and the reconciliation/corroboration machinery below. +All 10 stages of the original build order are code-complete and unit-tested — 287 tests, all under `tests/unit/` (`tests/integration/` is an empty package). Beyond them: Docker + Caddy deployment, admin dashboard (`/admin`), static test UI (`/`), pending-inclusive balance display, an SSE push channel layered over the original polling, self-service password change + admin password reset, and the reconciliation/corroboration machinery below. Verified on mainnet with real money: registration + address derivation, deposit crediting (1-conf), a real 10 PLM bet (broadcast → confirmed → change credited back), and one full round cycle (close → draw on a real block hash → 70/30 payout with sat math checked against the broadcast tx → confirmation → close → next round auto-opened). **Withdrawal and the RBF bump path have never been exercised against a live broadcast** — unit-tested only. @@ -33,7 +33,7 @@ PYTHONPATH=. python scripts/decrypt_master_key.py # ops recovery: decrypt+pr PYTHONPATH=. python scripts/encrypt_master_key.py # ops bootstrap: import an externally-generated xprv (--overwrite to replace) PYTHONPATH=. python scripts/electrum_smoke_test.py # manual check: connect, handshake, subscribe to headers, print the tip -python -m pytest # all 281 tests +python -m pytest # all 287 tests python -m pytest tests/unit/test_hd.py # one file python -m pytest tests/unit/test_hd.py::test_derivation_is_deterministic # one test ``` diff --git a/app/auth/rate_limit.py b/app/auth/rate_limit.py index becd568..375a314 100644 --- a/app/auth/rate_limit.py +++ b/app/auth/rate_limit.py @@ -1,6 +1,15 @@ import time from dataclasses import dataclass +# B-56: bounds on _buckets, which is keyed by strings the caller chooses. +# 50k entries is a few MB at ~100 bytes each — far more than any real deployment's +# active attacker set, and small enough that filling it isn't a memory attack. +_MAX_BUCKETS = 50_000 +# How often record_failure sweeps out spent entries. Cheap (one pass over a dict +# that the sweep itself keeps small) and off the request's critical path in the +# normal case, since a successful login records no failure at all. +_SWEEP_INTERVAL_SECONDS = 60.0 + @dataclass class _Bucket: @@ -29,22 +38,65 @@ class RateLimiter: base_delay: float = 2.0, max_delay: float = 300.0, decay_seconds: float = 900.0, + max_buckets: int = _MAX_BUCKETS, + sweep_interval_seconds: float = _SWEEP_INTERVAL_SECONDS, ) -> None: self._threshold = threshold self._base_delay = base_delay self._max_delay = max_delay self._decay_seconds = decay_seconds + self._max_buckets = max_buckets + self._sweep_interval_seconds = sweep_interval_seconds self._buckets: dict[str, _Bucket] = {} + self._last_sweep_at = time.monotonic() + + def _is_spent(self, bucket: _Bucket, now: float) -> bool: + """Nothing left to remember: the lockout has expired *and* the failure count + would decay to zero on the next failure anyway. Dropping such a bucket is + indistinguishable from keeping it — which is what makes eviction safe.""" + return bucket.locked_until <= now and now - bucket.last_failure_at > self._decay_seconds + + def _prune(self, now: float) -> None: + """B-56: the dict was keyed by attacker-chosen strings (any username, and via + B-54 any IP) and only ever grew — `decay_seconds` aged a bucket's counter but + never removed the entry, so hammering login with random usernames was an + unbounded memory leak. + + Spent buckets go first, and they carry no information, so that alone keeps + the dict at the size of the genuinely active attack surface. The hard cap + below is the backstop for a burst faster than the sweep interval: it evicts + the entries closest to expiry, i.e. the ones whose loss buys an attacker the + least — never the freshest lockouts, which are the ones actually holding an + attack back.""" + for key in [k for k, b in self._buckets.items() if self._is_spent(b, now)]: + del self._buckets[key] + self._last_sweep_at = now + + excess = len(self._buckets) - self._max_buckets + if excess > 0: + by_expiry = sorted( + self._buckets.items(), key=lambda item: (item[1].locked_until, item[1].last_failure_at) + ) + for key, _ in by_expiry[:excess]: + del self._buckets[key] def retry_after(self, key: str) -> float: bucket = self._buckets.get(key) if bucket is None: return 0.0 - remaining = bucket.locked_until - time.monotonic() + now = time.monotonic() + if self._is_spent(bucket, now): + # Self-cleaning read path: a key that's merely being probed never + # accumulates an entry that outlives its own usefulness. + del self._buckets[key] + return 0.0 + remaining = bucket.locked_until - now return remaining if remaining > 0 else 0.0 def record_failure(self, key: str) -> None: now = time.monotonic() + if now - self._last_sweep_at >= self._sweep_interval_seconds or len(self._buckets) > self._max_buckets: + self._prune(now) bucket = self._buckets.setdefault(key, _Bucket()) if bucket.failures and now - bucket.last_failure_at > self._decay_seconds: bucket.failures = 0 diff --git a/tests/unit/test_rate_limit.py b/tests/unit/test_rate_limit.py new file mode 100644 index 0000000..a7f00d4 --- /dev/null +++ b/tests/unit/test_rate_limit.py @@ -0,0 +1,91 @@ +"""B-56: RateLimiter._buckets is keyed by strings the caller chooses — any +username, and (before B-54) any IP — and used to only ever grow. decay_seconds +aged a bucket's *counter* but never removed the entry, so hammering login with +random usernames was an unbounded memory leak. These tests pin both halves of +the bound: spent entries are swept, and the dict has a hard cap. + +The throttling behaviour itself is exercised end-to-end in test_auth.py; what +matters here is that pruning never hands an attacker a free pass. +""" + +import time + +from app.auth.rate_limit import RateLimiter + + +def _limiter(**kwargs) -> RateLimiter: + # sweep_interval_seconds=0 makes every record_failure sweep, so the tests are + # deterministic instead of depending on wall-clock timing. + kwargs.setdefault("sweep_interval_seconds", 0.0) + return RateLimiter(**kwargs) + + +def test_spent_buckets_are_swept_on_the_next_failure(): + limiter = _limiter(decay_seconds=0.05) + for i in range(50): + limiter.record_failure(f"user:{i}") + assert len(limiter._buckets) == 50 + + time.sleep(0.06) # every bucket is now past decay_seconds and unlocked + limiter.record_failure("user:fresh") + + assert list(limiter._buckets) == ["user:fresh"] + + +def test_a_bucket_still_locking_someone_out_is_never_swept(): + """The whole point of the entry: evicting it would reset the backoff and let the + attacker start over from a free attempt.""" + limiter = _limiter(threshold=1, base_delay=300.0, decay_seconds=0.05) + limiter.record_failure("user:victim") + assert limiter.retry_after("user:victim") > 0 + + time.sleep(0.06) # past decay_seconds, but the lockout is still running + limiter.record_failure("user:someone-else") + + assert "user:victim" in limiter._buckets + assert limiter.retry_after("user:victim") > 0 + + +def test_the_dict_is_capped_even_within_one_sweep_interval(): + """The cap is the backstop for a burst faster than the sweep interval, where + nothing has had time to expire yet.""" + limiter = _limiter(max_buckets=10, sweep_interval_seconds=3600.0, decay_seconds=3600.0) + for i in range(200): + limiter.record_failure(f"ip:{i}") + + assert len(limiter._buckets) <= 11 # the cap, plus the entry recorded after the last prune + + +def test_the_cap_evicts_the_entries_closest_to_expiry_first(): + """What gets dropped under pressure must buy an attacker the least. The deepest + lockout — the one built up over the most failures, and so the one actually + holding an attack back — has to be the last thing evicted, not collateral of a + flood of one-failure keys.""" + limiter = _limiter(max_buckets=3, threshold=1, base_delay=1.0, max_delay=600.0) + for _ in range(10): + limiter.record_failure("ip:persistent") # backoff doubles: locked for ~512s + for i in range(20): + limiter.record_failure(f"ip:filler{i}") # one failure each: locked for ~1s + + assert "ip:persistent" in limiter._buckets + assert limiter.retry_after("ip:persistent") > 100 + + +def test_retry_after_drops_a_spent_bucket_it_looks_at(): + limiter = _limiter(decay_seconds=0.05) + limiter.record_failure("user:probe") + + time.sleep(0.06) + assert limiter.retry_after("user:probe") == 0.0 + assert limiter._buckets == {} + + +def test_pruning_does_not_reset_a_live_failure_count(): + """A bucket below the lockout threshold still carries state worth keeping: the + next failure must count as the second, not the first.""" + limiter = _limiter(threshold=2, base_delay=300.0, decay_seconds=3600.0) + limiter.record_failure("user:a") + limiter.record_failure("user:b") # triggers a sweep + + limiter.record_failure("user:a") + assert limiter.retry_after("user:a") > 0