Throttle login and registration with exponential backoff (B-33)
POST /auth/login had no rate limiting, no lockout, no delay — a patient distributed attack could brute-force a password against an enumerable username list on a custodial wallet, where a guessed password means withdrawing someone's funds. Add per-username and per-IP throttling with exponential backoff (app/auth/rate_limit.py), keyed on app.state like UserLocks rather than a module global so each app instance gets isolated throttle state. Unknown-user and wrong-password already shared one response path, so no enumeration oracle there. Registration is throttled per-IP too, which also bounds how many accounts one IP can spin up (B-31). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -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)
|
||||
+64
-1
@@ -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)
|
||||
|
||||
@@ -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': '正在登录…',
|
||||
|
||||
@@ -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"
|
||||
Reference in New Issue
Block a user