fix(conferences): rate limit блокировал вход всей конференции сразу
Два бага в одном месте, оба вскрылись на нагрузочном тесте 31.07.2026. 1. Ключ лимита строился по `request.client.host`. Backend стоит за nginx, поэтому это адрес КОНТЕЙНЕРА NGINX, одинаковый для всех пользователей. Проверено на проде: в Redis лежал единственный ключ `rate_limit:resolve:172.18.0.13`. То есть лимит «10 запросов в минуту» действовал на весь инстанс разом, а не на клиента. 2. Считались все запросы подряд, включая успешные. Одиннадцатый человек, открывший ссылку на конференцию в течение минуты, получал 429 — и видел «Не удалось найти конференцию» для существующей и активной конференции. Люди попадали внутрь с пятой-десятой попытки, попадая в новое окно. Что изменилось: - адрес клиента берётся из `X-Real-IP` (nginx его уже передаёт). Именно `X-Real-IP`, а не первый элемент `X-Forwarded-For`: последний заполняется через `$proxy_add_x_forwarded_for`, то есть дописывается к присланному клиентом, и лимит обходился бы одним заголовком; - жёсткий счётчик (10/мин, как было) теперь считает только ПРОМАХИ: конференция не найдена или пароль неверен. Именно так выглядит перебор номера, от которого лимит и защищает по ADR-001, п.4; - на общий поток с адреса оставлен мягкий потолок 300/мин — против тупого флуда. Офис за общим NAT это один адрес, поэтому потолок заведомо выше правдоподобного числа участников одной конференции. Тесты: успешные резолвы и гостевые входы не упираются в лимит (50 и 30 подряд); перебор номера, несуществующий идентификатор и подбор пароля по-прежнему упираются; лимит одного клиента не задевает другого.
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user