diff --git a/tradein-mvp/backend/app/api/v1/auth.py b/tradein-mvp/backend/app/api/v1/auth.py index bd3b3ec8..96b4cd90 100644 --- a/tradein-mvp/backend/app/api/v1/auth.py +++ b/tradein-mvp/backend/app/api/v1/auth.py @@ -45,7 +45,7 @@ import secrets from typing import Annotated from fastapi import APIRouter, Depends, HTTPException, Request, Response -from pydantic import BaseModel +from pydantic import BaseModel, Field from sqlalchemy.orm import Session from app.core.config import settings @@ -83,6 +83,13 @@ _LOGIN_LIMITER = SlidingWindowLimiter( # Потолок: появятся воркеры (`--workers N`) — потолок делится на N, и его надо # переносить в Redis (`app.services.cache` уже держит там пул). Тот же ceiling # у соседнего `_LOGIN_LIMITER`; перезапуск процесса обнуляет оба. +# +# ⚠️ `limit` здесь НЕ ПОРОГ и ничего не режет: мы зовём только `record()`, а он +# на лимит не смотрит — считает и отдаёт число попыток в окне. Настоящий порог +# живёт в `_throttle_delay_s`, которая читает настройку на каждом вызове (и +# потому подхватывает monkeypatch в тестах). Значение продублировано сюда ровно +# для того, чтобы `retry_after()` на этом объекте — если его однажды позовут — +# отвечал по тому же числу, а не по случайному. _USERNAME_FAIL_LIMITER = SlidingWindowLimiter( limit=settings.login_username_fail_threshold, window_s=settings.login_username_fail_window_s, @@ -109,7 +116,15 @@ _ACCESS_EXPIRED_MESSAGE = "Пробный доступ закончился" class LoginRequest(BaseModel): - username: str + # max_length=64 — ровно верхняя граница CHECK'а реестра + # (`users_username_ascii_ck`, data/sql/auth/001), так что живое имя отсечь + # нельзя. Ограничение нужно не валидации ради: сырое имя становится ключом + # ОБОИХ лимитеров, а их `defaultdict` подчищается только при >10000 ключей и + # только от пустых корзин — при окне в час корзины непустые, освобождать + # нечего. Без границы длины килобайтные имена растили бы память ключами. + # Паттерн/минимум длины НЕ дублируем: в режиме `identity_store="tradein"` + # CHECK'а нет и живут не-ASCII имена (см. тест на кириллицу). + username: str = Field(max_length=64) password: str @@ -132,15 +147,23 @@ def _throttle_delay_s(fails_in_window: int) -> float: первые перебранные попытки почти незаметны, а сотни — упираются в потолок. Потолок обязателен: без него задержка становится той же блокировкой, только растянутой во времени. + + Показатель степени зажат (`min(..., 16)`) — это не косметика. `min()` считает + ОБА аргумента до сравнения, поэтому наивный `float(2 ** (excess - 1))` при + excess>=1025 падает с `OverflowError: int too large to convert to float` — + то есть ровно под целевой нагрузкой (1045 неудач по имени за час = 0.3 rps) + защита начинала отдавать 500 мгновенно и без аудита, вместо 401 с задержкой. + 2**16 = 65536с заведомо больше любого разумного потолка, так что зажим + видимого поведения не меняет, а арифметику делает безусловно конечной. """ excess = fails_in_window - settings.login_username_fail_threshold if excess <= 0: return 0.0 - return min(settings.login_username_throttle_max_delay_s, float(2 ** (excess - 1))) + return min(settings.login_username_throttle_max_delay_s, 2.0 ** min(excess - 1, 16)) async def _reject_invalid_credentials( - username: str, ip: str, user_agent: str | None + db: Session, username: str, ip: str, user_agent: str | None ) -> HTTPException: """Единый хвост ЛЮБОГО отказа по кредам: счётчик → аудит → задержка → 401. @@ -157,6 +180,15 @@ async def _reject_invalid_credentials( Возвращает `HTTPException`, а не бросает: `raise await …` не собирается, а `raise (await …)` читается хуже, чем `raise` над возвращённым значением. + + *db* нужен ровно затем, чтобы ОТДАТЬ соединение перед сном. `get_identity_db` + в дефолтном режиме (`identity_store="tradein"`, он же прод) отдаёт ту же + сессию, что `get_db` — движок с QueuePool на 5+10 соединений. После SELECT в + `get_user_by_username` сессия держит соединение в открытой транзакции, и сон + внутри её области жизни превращал бы каждую спящую попытку в занятое + соединение: ~15 одновременных неудач выбирают пул целиком, и тогда ЛЮБОЙ + эндпоинт ждёт checkout 30с и падает. Отказ в обслуживании против всех сразу — + хуже той блокировки учётки, ради отказа от которой всё это писалось. """ fails = _USERNAME_FAIL_LIMITER.record(username) delay_s = _throttle_delay_s(fails) @@ -182,6 +214,10 @@ async def _reject_invalid_credentials( delay_s, ip, ) + # Соединение — в пул ДО сна (см. docstring). Сессия дальше не нужна: + # вызывающий немедленно делает raise, а повторный close() в самой + # зависимости идемпотентен. + db.close() # await, не time.sleep: событийный цикл в это время обслуживает всех # остальных — тормозим перебор, а не сервис. await asyncio.sleep(delay_s) @@ -221,7 +257,7 @@ async def login( # Пароль проверен ВЫШЕ и безусловно — только теперь смотрим на состояние # доступа. Порядок несущий, а не стилистический: см. модульный docstring. if user is None or not password_ok: - raise await _reject_invalid_credentials(body.username, ip, user_agent) + raise await _reject_invalid_credentials(db, body.username, ip, user_agent) access_state = user["access_state"] if access_state is AccessState.TRIAL_EXPIRED: @@ -247,7 +283,7 @@ async def login( # disabled (и любое нераспознанное состояние — to_access_state fail-closed) # → ТОТ ЖЕ generic 401, то же событие и та же задержка, что при неверном # пароле: заблокированный аккаунт неотличим от несуществующего. - raise await _reject_invalid_credentials(body.username, ip, user_agent) + raise await _reject_invalid_credentials(db, body.username, ip, user_agent) token = create_session(db, user_id=user["user_id"], ip=ip, user_agent=user_agent) diff --git a/tradein-mvp/backend/tests/test_auth_api.py b/tradein-mvp/backend/tests/test_auth_api.py index 48e9c406..52341829 100644 --- a/tradein-mvp/backend/tests/test_auth_api.py +++ b/tradein-mvp/backend/tests/test_auth_api.py @@ -445,6 +445,13 @@ def test_throttle_delay_grows_and_caps(monkeypatch: pytest.MonkeyPatch) -> None: assert auth_router._throttle_delay_s(6) == 4.0 assert auth_router._throttle_delay_s(7) == 4.0 # потолок assert auth_router._throttle_delay_s(1000) == 4.0 + # Счётчик ничем не ограничен сверху (`record()` только добавляет метку), а + # `min()` вычисляет ОБА аргумента. Без зажатого показателя степени + # `float(2 ** (excess - 1))` при ~1045 неудачах падает с OverflowError, и + # защита начинает отдавать 500 без задержки и без аудита — ровно под той + # нагрузкой, ради которой писалась. 1000 выше проходило впритык под обрывом. + assert auth_router._throttle_delay_s(5_000) == 4.0 + assert auth_router._throttle_delay_s(10**6) == 4.0 def test_distributed_bruteforce_one_username_many_ips_hits_global_ceiling( @@ -500,6 +507,43 @@ def test_throttle_actually_delays_the_response( assert elapsed >= 1.0 +def test_db_connection_released_before_sleeping( + client: TestClient, store: _Store, monkeypatch: pytest.MonkeyPatch +) -> None: + """Соединение с БД возвращается в пул ДО сна, а не удерживается всю задержку. + + На проде `get_identity_db` в дефолтном режиме отдаёт ту же сессию, что + `get_db` (движок с QueuePool 5+10, pool_timeout=30), а `get_user_by_username` + оставляет её в открытой транзакции. Сон внутри этой области жизни держал бы + соединение занятым все 8с: ~15 одновременно спящих неудач выбирают пул + целиком, и дальше ЛЮБОЙ эндпоинт ждёт checkout 30с и падает — отказ в + обслуживании против всех, ради ухода от которого замедление и выбиралось + вместо блокировки. + + Проверяем порядком, а не мокой пула: если `close()` случился до сна, между + ним и концом ответа лежит вся задержка; если бы сессию закрывала только + зависимость (то есть после сна) — зазор был бы околонулевым. + """ + store.add_user("holder", hash_password("Secret123!"), role="employee") + _throttle_settings(monkeypatch, threshold=0, max_delay_s=1.0) + + closes: list[float] = [] + real_close = _FakeDB.close + + def _spy_close(self: _FakeDB) -> None: + closes.append(time.monotonic()) + real_close(self) + + monkeypatch.setattr(_FakeDB, "close", _spy_close) + + resp = client.post("/api/v1/auth/login", json={"username": "holder", "password": "wrong"}) + finished = time.monotonic() + + assert resp.status_code == 401 + assert closes, "сессия не закрывалась вовсе" + assert finished - closes[0] >= 1.0 + + def test_typo_does_not_throttle_and_correct_password_still_works( client: TestClient, store: _Store, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -523,6 +567,39 @@ def test_typo_does_not_throttle_and_correct_password_still_works( assert config.settings.session_cookie_name in ok.cookies +def test_counter_decays_when_window_passes(monkeypatch: pytest.MonkeyPatch) -> None: + """Вторая половина DoD 2: наказание не накапливается вечно. + + Окно скользящее, старые неудачи выпадают сами — снимать ничего вручную не + нужно. Проверяем на самом счётчике, а не через HTTP: один вызов login стоит + полного bcrypt (~0.25с), так что игрушечное окно истекало бы прямо посреди + цикла запросов и тест мерил бы скорость хеширования, а не спад счётчика. + """ + _throttle_settings(monkeypatch, threshold=1, max_delay_s=4.0) + limiter = auth_router._USERNAME_FAIL_LIMITER + monkeypatch.setattr(limiter, "_window_s", 0.2) + + assert [limiter.record("frank") for _ in range(3)] == [1, 2, 3] + assert auth_router._throttle_delay_s(3) > 0 + + time.sleep(0.25) # окно прошло — прошлые неудачи больше не считаются + + assert limiter.record("frank") == 1 + assert auth_router._throttle_delay_s(1) == 0.0 + + +def test_username_length_is_bounded(client: TestClient) -> None: + """Сырое имя становится ключом обоих лимитеров, а их словарь чистится только + при >10000 ключей и только от пустых корзин — при окне в час чистить нечего. + Границу длины держим на 64 (верх CHECK'а реестра), чтобы килобайтные имена + не растили память ключами.""" + resp = client.post("/api/v1/auth/login", json={"username": "x" * 65, "password": "p"}) + assert resp.status_code == 422 + # 64 — всё ещё валидная длина, отвечаем обычным generic-отказом. + ok_len = client.post("/api/v1/auth/login", json={"username": "x" * 64, "password": "p"}) + assert ok_len.status_code == 401 + + def test_throttle_identical_for_existing_and_unknown_username( client: TestClient, store: _Store, monkeypatch: pytest.MonkeyPatch ) -> None: