diff --git a/backend/api/conferences.py b/backend/api/conferences.py index 954c378..99288c2 100644 --- a/backend/api/conferences.py +++ b/backend/api/conferences.py @@ -9,7 +9,12 @@ from sqlalchemy.ext.asyncio import AsyncSession from api.deps import get_current_user from core.db import get_session -from core.rate_limit import enforce_rate_limit +from core.rate_limit import ( + RATE_LIMIT_MISS_MAX_REQUESTS, + RATE_LIMIT_SOFT_MAX_REQUESTS, + client_ip, + enforce_rate_limit, +) from models.user import User from schemas.conferences import ( ConferenceCreateIn, @@ -103,10 +108,18 @@ async def resolve_conference( п.4, уточнение резолва): вход в неё невозможен в любом случае (410 у join/guest-join), а признак закрытости неактуален для мёртвой конференции. """ - await enforce_rate_limit(f"resolve:{_client_ip(request)}") + ip = client_ip(request) + # Мягкий потолок против флуда: успешные резолвы легитимны и массовы — + # вся конференция открывает ссылку в одну минуту. + await enforce_rate_limit(f"resolve:{ip}", max_requests=RATE_LIMIT_SOFT_MAX_REQUESTS) service = ConferenceService(session) conference = await service.resolve(q) if conference is None: + # Жёсткий счётчик — только на промахи: перебор номера конференции + # выглядит именно так (см. core/rate_limit.py и ADR-001, п.4). + await enforce_rate_limit( + f"resolve_miss:{ip}", max_requests=RATE_LIMIT_MISS_MAX_REQUESTS + ) raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="not_found") if conference.status == "ended": return ResolveOut(id=conference.id, title=conference.title, status=conference.status) @@ -154,11 +167,15 @@ async def guest_join_conference( session: Annotated[AsyncSession, Depends(get_session)], ) -> JoinOut: """Войти гостем: представиться (имя обязательно, email факультативен) — без auth, rate limit.""" - await enforce_rate_limit(f"guest_join:{_client_ip(request)}") + ip = client_ip(request) + # Мягкий потолок: успешный гостевой вход — обычное дело для всей + # конференции сразу, ограничивать его числом «10 в минуту» нельзя. + await enforce_rate_limit(f"guest_join:{ip}", max_requests=RATE_LIMIT_SOFT_MAX_REQUESTS) service = ConferenceService(session) try: return await service.join_as_guest(conference_id, data=data) except ConferenceNotFoundError as exc: + await _count_guest_join_miss(ip) raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail="conference_not_found" ) from exc @@ -169,6 +186,9 @@ async def guest_join_conference( status_code=status.HTTP_403_FORBIDDEN, detail="password_required" ) from exc except InvalidPasswordError as exc: + # Подбор пароля закрытой конференции — тот же класс атаки, что и + # перебор номера, поэтому считается жёстким счётчиком. + await _count_guest_join_miss(ip) raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, detail="invalid_password" ) from exc @@ -284,6 +304,11 @@ def _require_utc(value: datetime) -> datetime: return value.astimezone(UTC) -def _client_ip(request: Request) -> str: - """IP-адрес клиента для rate limit (без auth — ключ по IP, а не по пользователю).""" - return request.client.host if request.client else "unknown" +async def _count_guest_join_miss(ip: str) -> None: + """Учесть неудачную попытку гостевого входа в жёстком счётчике. + + Вынесено отдельно, потому что вызывается из двух веток обработки ошибок + (несуществующая конференция и неверный пароль) и обязано бросать 429 + ровно так же, как обычный `enforce_rate_limit`. + """ + await enforce_rate_limit(f"guest_join_miss:{ip}", max_requests=RATE_LIMIT_MISS_MAX_REQUESTS) diff --git a/backend/core/rate_limit.py b/backend/core/rate_limit.py index e1b1d00..1fb8eea 100644 --- a/backend/core/rate_limit.py +++ b/backend/core/rate_limit.py @@ -3,15 +3,59 @@ Используется резолвом конференций и гостевым входом (`api/conferences.py`) — эндпоинтами без аутентификации, уязвимыми к перебору номера/ссылки конференции (см. ADR-001, п.4 — оценка энтропии и рекомендуемый лимит 10 запросов/мин на IP). + +## Два счётчика вместо одного (0.0.18) + +Прежняя схема считала ВСЕ запросы подряд с лимитом 10/мин. На нагрузочном +тесте 31.07.2026 это остановило вход целой конференции: люди открывали ссылку +одновременно, одиннадцатый получал 429, а фронтенд показывал «Не удалось найти +конференцию» — при том что конференция существовала и была активна. + +Смысл лимита по ADR-001 — защита от ПЕРЕБОРА номера конференции. Перебор — это +поток промахов; легитимный участник открывает существующую ссылку и получает +успех. Поэтому: + +- `RATE_LIMIT_MISS_MAX_REQUESTS` — жёсткий счётчик промахов (конференция не + найдена, неверный пароль). Именно он защищает от перебора, и он остался + прежним — 10/мин; +- `RATE_LIMIT_SOFT_MAX_REQUESTS` — мягкий потолок на общее число обращений с + одного адреса. Нужен только против тупого флуда; рассчитан так, чтобы сотня + человек из офиса за общим NAT спокойно зашла в одну конференцию. """ -from fastapi import HTTPException, status +from fastapi import HTTPException, Request, status from core.redis import redis_client RATE_LIMIT_MAX_REQUESTS = 10 RATE_LIMIT_WINDOW_SECONDS = 60 +# Промахи: перебор номера/ссылки или подбор пароля конференции. +RATE_LIMIT_MISS_MAX_REQUESTS = 10 +# Общий поток с одного IP. Офис за общим NAT — это ОДИН адрес, поэтому потолок +# заведомо выше правдоподобного числа участников одной конференции. +RATE_LIMIT_SOFT_MAX_REQUESTS = 300 + + +def client_ip(request: Request) -> str: + """IP клиента для rate limit — с учётом того, что backend стоит за nginx. + + `request.client.host` — это TCP-peer, то есть контейнер nginx, один и тот же + для всех пользователей. С ним лимит превращался в общий на весь инстанс: + на проде в Redis лежал единственный ключ `rate_limit:resolve:172.18.0.13`, + и десяти запросов в минуту хватало, чтобы заблокировать вход всем сразу. + + Берём `X-Real-IP`, а НЕ первый элемент `X-Forwarded-For`: nginx заполняет + его через `$proxy_add_x_forwarded_for`, то есть ДОПИСЫВАЕТ к присланному + клиентом. Первый элемент там подделывается одним заголовком, и лимит + обходился бы тривиально. `X-Real-IP` nginx всегда перезаписывает своим + `$remote_addr` (см. deploy/nginx/nginx.conf.template). + """ + real_ip = request.headers.get("x-real-ip") + if real_ip: + return real_ip.strip() + return request.client.host if request.client else "unknown" + async def enforce_rate_limit( key: str, diff --git a/backend/tests/test_conferences_api.py b/backend/tests/test_conferences_api.py index a6a6c9c..2cc1a1b 100644 --- a/backend/tests/test_conferences_api.py +++ b/backend/tests/test_conferences_api.py @@ -573,17 +573,75 @@ async def test_resolve_unknown_returns_uniform_404(client: httpx.AsyncClient) -> assert response.json()["detail"] == "not_found" -async def test_resolve_is_rate_limited_after_10_requests_per_minute( - client: httpx.AsyncClient, -) -> None: +def _ip_headers() -> dict[str, str]: + """Уникальный `X-Real-IP` на каждый тест. + + Счётчики rate limit живут в Redis 60 секунд и общие для всего инстанса, + поэтому без изоляции тесты влияли бы друг на друга через остаточные ключи. + Заодно это проверяет, что заголовок вообще читается: раньше ключ строился + по `request.client.host`, то есть по адресу nginx, одинаковому для всех. + """ + return {"X-Real-IP": f"198.51.100.{uuid.uuid4().int % 250 + 1}-{uuid.uuid4().hex[:8]}"} + + +async def test_resolve_misses_are_rate_limited(client: httpx.AsyncClient) -> None: + """Перебор номера конференции упирается в жёсткий лимит промахов (ADR-001, п.4).""" + headers = _ip_headers() for _ in range(10): - response = await client.get("/api/v1/conferences/resolve", params={"q": "irrelevant-query"}) + response = await client.get( + "/api/v1/conferences/resolve", params={"q": "irrelevant-query"}, headers=headers + ) assert response.status_code == 404 - limited = await client.get("/api/v1/conferences/resolve", params={"q": "irrelevant-query"}) + limited = await client.get( + "/api/v1/conferences/resolve", params={"q": "irrelevant-query"}, headers=headers + ) assert limited.status_code == 429 +async def test_successful_resolves_are_not_limited_by_miss_counter( + client: httpx.AsyncClient, db_session: AsyncSession +) -> None: + """Вся конференция может открыть ссылку одновременно (регресс теста 31.07.2026). + + Прежняя схема считала любые запросы с лимитом 10/мин, и одиннадцатый + участник получал 429 — фронтенд показывал «Не удалось найти конференцию» + для существующей и активной конференции. + """ + conference = await _make_conference(db_session) + await db_session.commit() + headers = _ip_headers() + + for _ in range(50): + response = await client.get( + "/api/v1/conferences/resolve", params={"q": conference.slug}, headers=headers + ) + assert response.status_code == 200, response.text + + +async def test_rate_limit_is_per_client_ip(client: httpx.AsyncClient) -> None: + """Счётчик привязан к адресу клиента, а не к адресу nginx. + + Исчерпав лимит промахов с одного адреса, с другого по-прежнему можно + работать. До исправления ключ был общим на весь инстанс. + """ + first, second = _ip_headers(), _ip_headers() + for _ in range(11): + await client.get( + "/api/v1/conferences/resolve", params={"q": "no-such-conference"}, headers=first + ) + + exhausted = await client.get( + "/api/v1/conferences/resolve", params={"q": "no-such-conference"}, headers=first + ) + assert exhausted.status_code == 429 + + other = await client.get( + "/api/v1/conferences/resolve", params={"q": "no-such-conference"}, headers=second + ) + assert other.status_code == 404, "лимит одного клиента не должен задевать другого" + + # --- Вход зарегистрированным пользователем --------------------------------------- @@ -835,21 +893,69 @@ async def test_guest_join_ended_conference_returns_410( assert response.json()["detail"] == "conference_ended" -async def test_guest_join_is_rate_limited_after_10_requests_per_minute( +async def test_guest_join_allows_a_whole_conference_to_enter( client: httpx.AsyncClient, db_session: AsyncSession ) -> None: + """Успешные гостевые входы не упираются в лимит промахов. + + На нагрузочном тесте 31.07.2026 конференцию из семи десятков человек не + пускало внутрь именно это ограничение — счётчик не различал легитимный + массовый вход и перебор. + """ conference = await _make_conference(db_session) await db_session.commit() + headers = _ip_headers() + + for i in range(30): + response = await client.post( + f"/api/v1/conferences/{conference.id}/guest-join", + json={"display_name": f"Guest {i}"}, + headers=headers, + ) + assert response.status_code == 200, response.text + + +async def test_guest_join_misses_are_rate_limited(client: httpx.AsyncClient) -> None: + """Перебор идентификатора конференции по-прежнему упирается в лимит.""" + headers = _ip_headers() + missing_id = uuid.uuid4() + + for _ in range(10): + response = await client.post( + f"/api/v1/conferences/{missing_id}/guest-join", + json={"display_name": "Bruteforce"}, + headers=headers, + ) + assert response.status_code == 404 + + limited = await client.post( + f"/api/v1/conferences/{missing_id}/guest-join", + json={"display_name": "Bruteforce"}, + headers=headers, + ) + assert limited.status_code == 429 + + +async def test_guest_join_wrong_password_is_rate_limited( + client: httpx.AsyncClient, db_session: AsyncSession +) -> None: + """Подбор пароля закрытой конференции считается тем же жёстким счётчиком.""" + conference = await _make_conference(db_session, is_closed=True, password="right-password") + await db_session.commit() + headers = _ip_headers() for _ in range(10): response = await client.post( f"/api/v1/conferences/{conference.id}/guest-join", - json={"display_name": "Repeat Guest"}, + json={"display_name": "Guesser", "password": "wrong"}, + headers=headers, ) - assert response.status_code == 200 + assert response.status_code == 403 limited = await client.post( - f"/api/v1/conferences/{conference.id}/guest-join", json={"display_name": "Repeat Guest"} + f"/api/v1/conferences/{conference.id}/guest-join", + json={"display_name": "Guesser", "password": "wrong"}, + headers=headers, ) assert limited.status_code == 429