diff --git a/BUGS.md b/BUGS.md index 6003d14..63a0639 100644 --- a/BUGS.md +++ b/BUGS.md @@ -1,11 +1,11 @@ # Known bugs A second full-codebase audit on 2026-07-27 found **25 further issues** (4 critical, 6 high, -7 medium, 8 low), listed below as B-33 … B-49. B-25 through B-32 are fixed (see "Previously -fixed" below) — no Critical-severity finding remains open; the other 17 are High/Medium/Low. +7 medium, 8 low), listed below as B-33 … B-49. B-25 through B-33 are fixed (see "Previously +fixed" below) — no Critical-severity finding remains open; the other 16 are High/Medium/Low. The 139-test suite was green at the time of the audit, so none of these were caught by existing -coverage — every fix lands with a regression test (the eight fixes so far brought the suite -from 139 to 187). +coverage — every fix lands with a regression test (the nine fixes so far brought the suite +from 139 to 192). The recurring pattern across the open findings is worth stating once: the code is rigorous about the failure modes that have actually been hit, and silent about the ones that have not. @@ -20,22 +20,6 @@ single-process assumptions, no user-facing history, etc.), see "Known gaps / TOD ## High -### B-33 — No brute-force protection on a custodial wallet - -`POST /auth/login` (`auth/routes.py:83`) has no rate limiting, no lockout, no delay and no -CAPTCHA, and the password minimum is 8 characters. Argon2 slows a single attempt but not a -patient distributed attack against an enumerable username list — and `409 username_taken` on -registration is a perfect enumeration oracle. - -"No rate limiting" is listed as a generic known gap; on a system where guessing a password -means **withdrawing someone's funds**, it deserves to be tracked separately and treated as a -blocker. - -**Proposed fix.** Per-username *and* per-IP throttling with exponential backoff on failed -logins (`slowapi`, or a small DB-backed counter — but note the in-process caveat if workers -are ever scaled). Return an identical response for unknown-user and wrong-password. Rate-limit -registration too, which also bounds B-31's attacker-controlled user count. - ### B-34 — Password change and admin reset do not invalidate existing sessions Neither `/users/me/change-password` nor `/admin/users/{id}/reset-password` invalidates @@ -227,9 +211,10 @@ already does. - **B-30** — a lost scripthash subscription meant a user's deposits were never credited, with no periodic safety net - **B-31** — resubscribing on reconnect ran serially before anything else started, freezing the chain tip (and so an in-flight draw) for the whole sweep - **B-32** — an RBF bump's fee delta could fall below BIP125's relay-mandated minimum, so the node rejected it and the same tick retried identically forever; also had no ceiling on how high the fee rate could climb +- **B-33** — `POST /auth/login` had no rate limiting on a custodial wallet, so a patient distributed attack could brute-force a password against an enumerable username list; fixed with per-username *and* per-IP exponential backoff (`app/auth/rate_limit.py`), registration throttled per-IP too (also bounds B-31's attacker-controlled user count) See git history for the fix-by-fix breakdown (commits `f13f685`, `50a43ae`, `933760e`, and the -B-28/B-29/B-30/B-31/B-32 fixes). Suite grew from 139 to 187 tests over the eight. +B-28/B-29/B-30/B-31/B-32/B-33 fixes). Suite grew from 139 to 192 tests over the nine. A full-codebase audit on 2026-07-26 (commit `d4e0974`) found 24 bugs across every Python module under `app/`, both static frontends, and the Docker/Caddy deployment — 5 critical, diff --git a/app/auth/rate_limit.py b/app/auth/rate_limit.py new file mode 100644 index 0000000..becd568 --- /dev/null +++ b/app/auth/rate_limit.py @@ -0,0 +1,75 @@ +import time +from dataclasses import dataclass + + +@dataclass +class _Bucket: + failures: int = 0 + locked_until: float = 0.0 + last_failure_at: float = 0.0 + + +class RateLimiter: + """In-process failed-attempt throttle with exponential backoff, keyed by an + arbitrary string (username, IP...). Single-process-only, like UserLocks + (app/tx/locks.py) — an accepted MVP constraint; a multi-worker deployment + would need a shared store (Redis) instead (B-33). + + Brute-forcing a login here isn't a spammy client to be capped at N req/s — + it's an attempt to withdraw someone else's funds — so failures are + penalized with a delay that doubles each time past `threshold` free + attempts, rather than a flat rate cap. `decay_seconds` ages a bucket back + to zero once failures stop, so a shared/NAT IP isn't punished forever for + someone else's earlier mistakes. + """ + + def __init__( + self, + threshold: int = 5, + base_delay: float = 2.0, + max_delay: float = 300.0, + decay_seconds: float = 900.0, + ) -> None: + self._threshold = threshold + self._base_delay = base_delay + self._max_delay = max_delay + self._decay_seconds = decay_seconds + self._buckets: dict[str, _Bucket] = {} + + 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() + return remaining if remaining > 0 else 0.0 + + def record_failure(self, key: str) -> None: + now = time.monotonic() + bucket = self._buckets.setdefault(key, _Bucket()) + if bucket.failures and now - bucket.last_failure_at > self._decay_seconds: + bucket.failures = 0 + bucket.failures += 1 + bucket.last_failure_at = now + if bucket.failures >= self._threshold: + delay = min(self._max_delay, self._base_delay * 2 ** (bucket.failures - self._threshold)) + bucket.locked_until = now + delay + + def record_success(self, key: str) -> None: + self._buckets.pop(key, None) + + +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. + + A module global would persist for the lifetime of the process — fine in + production (one app instance), but wrong in the test suite, where every + test builds its own FastAPI app against a fresh in-memory DB and expects a + clean slate; a shared global would leak failure counts between unrelated + tests. Per-`app.state` state gets a fresh instance per app automatically. + """ + + 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) diff --git a/app/auth/routes.py b/app/auth/routes.py index a744346..3abfbf4 100644 --- a/app/auth/routes.py +++ b/app/auth/routes.py @@ -5,6 +5,7 @@ from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession from app.api.errors import http_error +from app.auth.rate_limit import AuthRateLimiters from app.auth.security import MIN_PASSWORD_LENGTH, create_access_token, hash_password, verify_password from app.db.models import User from app.db.session import get_session @@ -15,6 +16,43 @@ router = APIRouter(prefix="/auth", tags=["auth"]) _MAX_REGISTER_RETRIES = 5 +def _rate_limiters(request: Request) -> AuthRateLimiters: + # B-33: no rate limiting on login was a brute-forceable path to withdrawing + # someone else's funds. Keyed per-username *and* per-IP so an attacker can't + # dodge the throttle by spraying one password across many accounts, nor by + # routing one account's guesses through many IPs alone (the username key + # 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 + # 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() + return request.app.state.auth_rate_limiters + + +def _client_ip(request: Request) -> str: + # Caddy (see Caddyfile) reverse-proxies every request, so request.client.host + # is the proxy's address, not the caller's — fall back to it only if the + # header is somehow missing (e.g. hitting the app container directly). + forwarded = request.headers.get("x-forwarded-for") + if forwarded: + return forwarded.split(",")[0].strip() + return request.client.host if request.client else "unknown" + + +def _rate_limited_error(retry_after: float): + return http_error( + status.HTTP_429_TOO_MANY_REQUESTS, + "rate_limited", + "too many attempts, try again later", + retry_after_seconds=int(retry_after) + 1, + ) + + class RegisterRequest(BaseModel): """Registration used to accept an empty username and a one-character password, while /users/me/change-password demanded 8 characters — an odd place to be @@ -34,6 +72,13 @@ class TokenResponse(BaseModel): async def register( body: RegisterRequest, request: Request, session: AsyncSession = Depends(get_session) ) -> TokenResponse: + limiters = _rate_limiters(request) + ip_key = f"ip:{_client_ip(request)}" + 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) + existing = await session.scalar(select(User).where(User.username == body.username)) if existing is not None: raise http_error(status.HTTP_409_CONFLICT, "username_taken", "username already taken") @@ -81,8 +126,26 @@ class LoginRequest(BaseModel): @router.post("/login", response_model=TokenResponse) -async def login(body: LoginRequest, session: AsyncSession = Depends(get_session)) -> TokenResponse: +async def login( + body: LoginRequest, request: Request, session: AsyncSession = Depends(get_session) +) -> TokenResponse: + limiters = _rate_limiters(request) + username_key = f"user:{body.username.lower()}" + ip_key = f"ip:{_client_ip(request)}" + retry_after = max(limiters.login.retry_after(username_key), limiters.login_ip.retry_after(ip_key)) + if retry_after > 0: + raise _rate_limited_error(retry_after) + user = await session.scalar(select(User).where(User.username == body.username)) if user is None or not verify_password(body.password, user.password_hash): + # Same code path (and therefore the same response) whether the username + # doesn't exist or the password is wrong — no enumeration oracle here. + limiters.login.record_failure(username_key) + limiters.login_ip.record_failure(ip_key) raise http_error(status.HTTP_401_UNAUTHORIZED, "invalid_credentials", "invalid credentials") + + # Only the username bucket resets on success — the IP bucket is left to decay + # on its own, so one correct login can't be used to wipe out an IP's failure + # count while it's mid-attack against other accounts. + limiters.login.record_success(username_key) return TokenResponse(access_token=create_access_token(user.id), address=user.address) diff --git a/app/static/i18n.js b/app/static/i18n.js index d83645f..96bfa9f 100644 --- a/app/static/i18n.js +++ b/app/static/i18n.js @@ -137,6 +137,7 @@ const TRANSLATIONS = { 'error.withdrawal_to_own_address': 'That is your own deposit address — withdraw to an external wallet.', 'error.internal_error': 'Unexpected server error. Please try again shortly.', 'error.guide_unavailable': 'The guide is not available right now.', + 'error.rate_limited': 'Too many attempts, please try again in {retry_after_seconds} seconds.', 'loading.creating': 'Creating…', 'loading.loggingIn': 'Logging in…', @@ -274,6 +275,7 @@ const TRANSLATIONS = { 'error.withdrawal_to_own_address': 'Questo è il tuo indirizzo di deposito — preleva verso un wallet esterno.', 'error.internal_error': 'Errore inatteso del server. Riprova tra poco.', 'error.guide_unavailable': 'La guida non è disponibile in questo momento.', + 'error.rate_limited': 'Troppi tentativi, riprova tra {retry_after_seconds} secondi.', 'loading.creating': 'Creazione…', 'loading.loggingIn': 'Accesso…', @@ -411,6 +413,7 @@ const TRANSLATIONS = { 'error.withdrawal_to_own_address': 'Esa es tu propia dirección de depósito — retira a una cartera externa.', 'error.internal_error': 'Error inesperado del servidor. Inténtalo de nuevo en un momento.', 'error.guide_unavailable': 'La guía no está disponible en este momento.', + 'error.rate_limited': 'Demasiados intentos, inténtalo de nuevo en {retry_after_seconds} segundos.', 'loading.creating': 'Creando…', 'loading.loggingIn': 'Entrando…', @@ -548,6 +551,7 @@ const TRANSLATIONS = { 'error.withdrawal_to_own_address': "C'est votre propre adresse de dépôt — retirez vers un portefeuille externe.", 'error.internal_error': 'Erreur inattendue du serveur. Veuillez réessayer dans un instant.', 'error.guide_unavailable': "Le guide n'est pas disponible pour le moment.", + 'error.rate_limited': 'Trop de tentatives, réessayez dans {retry_after_seconds} secondes.', 'loading.creating': 'Création…', 'loading.loggingIn': 'Connexion…', @@ -685,6 +689,7 @@ const TRANSLATIONS = { 'error.withdrawal_to_own_address': 'Das ist deine eigene Einzahlungsadresse — zahle auf eine externe Wallet aus.', 'error.internal_error': 'Unerwarteter Serverfehler. Bitte versuche es in Kürze erneut.', 'error.guide_unavailable': 'Die Anleitung ist derzeit nicht verfügbar.', + 'error.rate_limited': 'Zu viele Versuche, bitte versuche es in {retry_after_seconds} Sekunden erneut.', 'loading.creating': 'Wird erstellt…', 'loading.loggingIn': 'Anmeldung…', @@ -822,6 +827,7 @@ const TRANSLATIONS = { 'error.withdrawal_to_own_address': 'Это ваш собственный адрес для депозита — выводите на внешний кошелёк.', 'error.internal_error': 'Непредвиденная ошибка сервера. Попробуйте ещё раз через минуту.', 'error.guide_unavailable': 'Руководство сейчас недоступно.', + 'error.rate_limited': 'Слишком много попыток, повторите через {retry_after_seconds} сек.', 'loading.creating': 'Создание…', 'loading.loggingIn': 'Вход…', @@ -959,6 +965,7 @@ const TRANSLATIONS = { 'error.withdrawal_to_own_address': '这是你自己的充值地址 — 请提现到外部钱包。', 'error.internal_error': '服务器发生意外错误，请稍后重试。', 'error.guide_unavailable': '指南当前不可用。', + 'error.rate_limited': '尝试次数过多，请在 {retry_after_seconds} 秒后重试。', 'loading.creating': '正在创建…', 'loading.loggingIn': '正在登录…', diff --git a/tests/unit/test_auth.py b/tests/unit/test_auth.py new file mode 100644 index 0000000..e1d33e1 --- /dev/null +++ b/tests/unit/test_auth.py @@ -0,0 +1,126 @@ +import pytest +from cryptography.fernet import Fernet +from httpx import ASGITransport, AsyncClient + +from app.config import settings + + +@pytest.fixture +async def client(monkeypatch, tmp_path): + monkeypatch.setattr(settings, "database_url", f"sqlite+aiosqlite:///{tmp_path}/test.db") + monkeypatch.setattr(settings, "jwt_secret", "test-jwt-secret") + monkeypatch.setattr(settings, "xprv_encryption_key", Fernet.generate_key().decode()) + monkeypatch.setattr(settings, "master_key_path", str(tmp_path / "master.xprv.enc")) + + import app.wallet.hd as hd + + hd._account_key = None + hd.generate_master_key() + + from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine + + from app.db import base as db_base + + import app.db.models # noqa: F401 + + db_base.engine = create_async_engine(settings.database_url) + db_base.AsyncSessionLocal = async_sessionmaker(db_base.engine, expire_on_commit=False) + + from app.db import session as db_session + + db_session.AsyncSessionLocal = db_base.AsyncSessionLocal + + async with db_base.engine.begin() as conn: + await conn.run_sync(db_base.Base.metadata.create_all) + + from fastapi import FastAPI + + from app.auth.routes import router as auth_router + from app.electrum.listener import ElectrumListener + + app = FastAPI() + app.include_router(auth_router) + app.state.electrum_listener = ElectrumListener(lambda endpoint: None, db_base.AsyncSessionLocal) + + transport = ASGITransport(app=app) + async with AsyncClient(transport=transport, base_url="http://test") as ac: + yield ac + + await db_base.engine.dispose() + + +async def _register(client, username="alice", password="original-password"): + resp = await client.post("/auth/register", json={"username": username, "password": password}) + assert resp.status_code == 201 + return resp.json()["access_token"] + + +async def test_login_locks_out_after_repeated_failures(client): + await _register(client) + + for _ in range(5): + resp = await client.post("/auth/login", json={"username": "alice", "password": "wrong"}) + assert resp.status_code == 401 + + resp = await client.post("/auth/login", json={"username": "alice", "password": "wrong"}) + assert resp.status_code == 429 + assert resp.json()["detail"]["code"] == "rate_limited" + + # Even the *correct* password is refused while locked out — the throttle + # protects against a lucky guess landing inside the backoff window too. + resp = await client.post("/auth/login", json={"username": "alice", "password": "original-password"}) + assert resp.status_code == 429 + + +async def test_unknown_username_and_wrong_password_share_a_bucket_and_response(client): + await _register(client, username="bob") + + for _ in range(5): + resp = await client.post("/auth/login", json={"username": "nobody", "password": "wrong"}) + assert resp.status_code == 401 + assert resp.json()["detail"]["code"] == "invalid_credentials" + + resp = await client.post("/auth/login", json={"username": "nobody", "password": "wrong"}) + assert resp.status_code == 429 + + +async def test_login_failures_against_one_account_do_not_lock_out_another(client): + await _register(client, username="alice") + await _register(client, username="carol", password="carols-password") + + for _ in range(6): + await client.post("/auth/login", json={"username": "alice", "password": "wrong"}) + + # Different username, but same IP (the test client always looks the same) — + # only the per-username bucket should be exhausted, not the whole IP, since + # the per-username threshold (5) is hit well before the shared IP bucket's. + resp = await client.post("/auth/login", json={"username": "carol", "password": "carols-password"}) + assert resp.status_code == 200 + + +async def test_successful_login_resets_the_username_bucket(client): + await _register(client) + + for _ in range(4): + resp = await client.post("/auth/login", json={"username": "alice", "password": "wrong"}) + assert resp.status_code == 401 + + resp = await client.post("/auth/login", json={"username": "alice", "password": "original-password"}) + assert resp.status_code == 200 + + resp = await client.post("/auth/login", json={"username": "alice", "password": "original-password"}) + assert resp.status_code == 200 + + +async def test_registration_is_rate_limited_per_ip(client): + for i in range(5): + resp = await client.post( + "/auth/register", json={"username": f"user{i}", "password": "a-strong-password"} + ) + assert resp.status_code == 201 + + resp = await client.post( + "/auth/register", json={"username": "user5", "password": "a-strong-password"} + ) + assert resp.status_code == 429 + assert resp.json()["detail"]["code"] == "rate_limited"