From 0aac73e557f0aeb87c1bba23643bc97130fc3a13 Mon Sep 17 00:00:00 2001 From: Davide Grilli Date: Mon, 3 Aug 2026 22:32:20 +0200 Subject: [PATCH] Bound registrations with a per-IP quota instead of failure backoff (B-58) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- BUGS.md | 14 ------ CLAUDE.md | 6 +-- app/auth/rate_limit.py | 85 ++++++++++++++++++++++++++++++++++- app/auth/routes.py | 14 +++--- tests/unit/test_auth.py | 38 +++++++++++++++- tests/unit/test_rate_limit.py | 51 ++++++++++++++++++++- 6 files changed, 183 insertions(+), 25 deletions(-) diff --git a/BUGS.md b/BUGS.md index d326a91..597c9bd 100644 --- a/BUGS.md +++ b/BUGS.md @@ -40,20 +40,6 @@ remains the last prerequisite for running unattended. ## High — security -### B-58 — the registration throttle counts successes as failures and is IP-only - -`app/auth/routes.py:66-71`. - -`record_failure` is called on every registration attempt, successful ones -included. Five legitimate signups from one shared/NAT address lock the sixth real -user out with exponential backoff up to 600 s — while an attacker skips the limiter -entirely through B-54. The intent (bounding accounts per source) is reasonable; -the current shape punishes only honest users. The inline comment also cites B-31 -(the resubscribe finding) where it means B-33. - -Fix: keep an accounts-per-IP quota if that is the goal, but express it as a quota -rather than as failure backoff, and fix the B-nn reference. - ### B-59 — deposit crediting is not corroborated, unlike external-spend detection `app/deposits/service.py:31-45`, `app/electrum/listener.py:401-433`. diff --git a/CLAUDE.md b/CLAUDE.md index f03836e..8d0f0f9 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 — 290 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 — 296 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 290 tests +python -m pytest # all 296 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 ``` @@ -245,7 +245,7 @@ Accepted **by design** — distinct from the audit findings above (all fixed), w - **Withdrawal and RBF bump are unit-tested but never live-broadcast** (failure and rollback branches included — still not a real network). - **No user-facing history.** `GET /users/me/last-round-result` covers exactly one case (the reveal backstop above). Admin has `/admin/rounds`, `/admin/pending-transactions`, `/admin/audit-log`; a user has no equivalent — a failed withdrawal leaves a `failed` row they can never see, which argues for closing this. - **Admin auth is one shared bearer token** (`ADMIN_TOKEN`) with no per-admin identity: `audit_log` records *what* changed (config edits as `config_updated`, with before/after) but never *who* did it. It gates the user list, privkey export, password resets and history, so a leak is high-blast-radius. -- **No rate limiting anywhere** (register, bet, withdrawal, admin, SSE). For login this is a blocker, not a gap — tracked as B-33. +- **No rate limiting on bet, withdrawal, admin or SSE.** Login has a per-username + per-IP failure throttle and registration a per-IP quota of 5 accounts/hour (`app/auth/rate_limit.py`: `RateLimiter` for failed guesses at a secret, `RollingQuota` for "how many of these may one source create" — B-33, B-58); everything else is unlimited. - **`/guida` and `/report-bug` are placeholders** (`app/static/guida.html`, `report-bug.html`) — links work, content is "coming soon". - **No integration tests against a live Electrum connection.** `tests/integration/` is empty; live verification has all been manual (`scripts/electrum_smoke_test.py`, ad hoc scripts, real mainnet txs). - **Single-process assumptions**: the SSE broadcaster and the per-user locks are in-process only. A multi-worker deployment needs a shared channel and a DB/Redis lock. The round-uniqueness invariant is *not* in this category — it's a DB index. diff --git a/app/auth/rate_limit.py b/app/auth/rate_limit.py index 375a314..ff69a5d 100644 --- a/app/auth/rate_limit.py +++ b/app/auth/rate_limit.py @@ -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 + ) diff --git a/app/auth/routes.py b/app/auth/routes.py index 7a68ba0..9664261 100644 --- a/app/auth/routes.py +++ b/app/auth/routes.py @@ -30,10 +30,11 @@ def _rate_limiters(request: Request) -> AuthRateLimiters: # still catches that). The IP limiter's threshold is deliberately higher # than the username one: a single account should lock out fast, but a # shared/NAT IP hosting several genuine users shouldn't be punished for one - # of them mistyping a password a few times. Registration gets its own, - # coarser limiter, IP-only — no username exists yet to key on — mainly to - # bound how many accounts one IP can spin up (B-31), not to protect a - # secret. Lives on app.state (see AuthRateLimiters) rather than a module + # of them mistyping a password a few times. Registration gets its own + # instrument entirely: a per-IP *quota* on accounts created (B-58), since + # bounding how many accounts one source can spin up is not the same problem + # as slowing down guesses at a secret, and failure backoff only punished the + # honest signups. Lives on app.state (see AuthRateLimiters) rather than a module # global so each app instance gets its own, isolated throttle state. if not hasattr(request.app.state, "auth_rate_limiters"): request.app.state.auth_rate_limiters = AuthRateLimiters() @@ -70,10 +71,12 @@ async def register( ) -> TokenResponse: limiters = _rate_limiters(request) ip_key = f"ip:{_client_ip(request)}" + # B-58: checked before the Argon2 hash below, so an IP that's out of quota + # costs nothing to turn away. Recorded only once an account actually exists — + # see the successful path below. retry_after = limiters.register_ip.retry_after(ip_key) if retry_after > 0: raise _rate_limited_error(retry_after) - limiters.register_ip.record_failure(ip_key) # B-57: case-insensitive, matching the unique index on lower(username) — and # matching the throttle key below, which has always been lowercased. @@ -110,6 +113,7 @@ async def register( ) from exc continue await session.refresh(user) + limiters.register_ip.record(ip_key) # B-58: one account created, one slot used request.app.state.electrum_listener.address_for_new_user(user.id, user.address) return TokenResponse( access_token=create_access_token(user.id, user.token_version), address=user.address diff --git a/tests/unit/test_auth.py b/tests/unit/test_auth.py index 7bb2204..695673f 100644 --- a/tests/unit/test_auth.py +++ b/tests/unit/test_auth.py @@ -112,7 +112,9 @@ async def test_successful_login_resets_the_username_bucket(client): assert resp.status_code == 200 -async def test_registration_is_rate_limited_per_ip(client): +async def test_registration_is_quota_limited_per_ip(client): # B-58 + """Five accounts per IP per hour. The sixth is told to wait, not punished with a + backoff that doubles from there.""" for i in range(5): resp = await client.post( "/auth/register", json={"username": f"user{i}", "password": "a-strong-password"} @@ -168,3 +170,37 @@ async def test_the_database_itself_rejects_a_case_variant(client): ) with pytest.raises(IntegrityError): await session.commit() + + +async def test_failed_registrations_do_not_consume_the_quota(client): # B-58 + """The limit is on accounts that exist, not on requests: record_failure used to + fire on every attempt, so five signups — successful ones included — locked the + sixth real user out for up to 600s from a shared or NAT address. Attempts that + create nothing must leave the quota untouched.""" + await _register(client, username="taken") + + for _ in range(10): + resp = await client.post( + "/auth/register", json={"username": "taken", "password": "a-strong-password"} + ) + assert resp.status_code == 409 # username_taken, no account created + + # Four slots left out of five, all still usable. + for i in range(4): + resp = await client.post( + "/auth/register", json={"username": f"genuine{i}", "password": "a-strong-password"} + ) + assert resp.status_code == 201 + + +async def test_the_quota_reports_how_long_to_wait(client): # B-58 + for i in range(5): + await _register(client, username=f"quotauser{i}", password="a-strong-password") + + resp = await client.post( + "/auth/register", json={"username": "one-too-many", "password": "a-strong-password"} + ) + + assert resp.status_code == 429 + retry_after = resp.json()["detail"]["params"]["retry_after_seconds"] + assert 0 < retry_after <= 3601 # bounded by the window, not by a growing penalty diff --git a/tests/unit/test_rate_limit.py b/tests/unit/test_rate_limit.py index a7f00d4..c9c575a 100644 --- a/tests/unit/test_rate_limit.py +++ b/tests/unit/test_rate_limit.py @@ -10,7 +10,7 @@ matters here is that pruning never hands an attacker a free pass. import time -from app.auth.rate_limit import RateLimiter +from app.auth.rate_limit import RateLimiter, RollingQuota def _limiter(**kwargs) -> RateLimiter: @@ -89,3 +89,52 @@ def test_pruning_does_not_reset_a_live_failure_count(): limiter.record_failure("user:a") assert limiter.retry_after("user:a") > 0 + + +# --- B-58: registration is a quota, not failure backoff ------------------------- + + +def test_quota_allows_up_to_the_limit_then_asks_for_a_wait(): + quota = RollingQuota(limit=3, window_seconds=60.0) + for _ in range(3): + assert quota.retry_after("ip:1") == 0.0 + quota.record("ip:1") + + wait = quota.retry_after("ip:1") + assert 0 < wait <= 60.0 + + +def test_quota_is_per_key(): + quota = RollingQuota(limit=1, window_seconds=60.0) + quota.record("ip:1") + + assert quota.retry_after("ip:1") > 0 + assert quota.retry_after("ip:2") == 0.0 + + +def test_quota_frees_a_slot_once_the_oldest_event_leaves_the_window(): + """The point of a rolling window over failure backoff: the caller waits exactly + until there's room again, and waiting doesn't make the next wait longer.""" + quota = RollingQuota(limit=2, window_seconds=0.05) + quota.record("ip:1") + quota.record("ip:1") + assert quota.retry_after("ip:1") > 0 + + time.sleep(0.06) + assert quota.retry_after("ip:1") == 0.0 + + +def test_quota_prunes_spent_keys_and_caps_its_dict(): + """Same bound as the failure limiter (B-56): caller-chosen keys, so both a sweep + and a hard cap.""" + quota = RollingQuota(limit=1, window_seconds=0.05, sweep_interval_seconds=0.0) + for i in range(50): + quota.record(f"ip:{i}") + time.sleep(0.06) + quota.record("ip:fresh") + assert list(quota._events) == ["ip:fresh"] + + capped = RollingQuota(limit=1, window_seconds=3600.0, max_keys=10, sweep_interval_seconds=0.0) + for i in range(200): + capped.record(f"ip:{i}") + assert len(capped._events) <= 11