From cb3af484cd96a9e94c95fba1b76b10f16f128366 Mon Sep 17 00:00:00 2001 From: NotBigGhost Date: Sun, 13 Sep 2026 13:14:14 +0300 Subject: [PATCH] =?UTF-8?q?CSRF:=20=D0=BF=D0=B5=D1=80=D0=B5=D0=B2=D1=8B?= =?UTF-8?q?=D0=B4=D0=B0=D0=B2=D0=B0=D1=82=D1=8C=20=D1=82=D0=BE=D0=BA=D0=B5?= =?UTF-8?q?=D0=BD,=20=D0=B5=D1=81=D0=BB=D0=B8=20=D1=81=D0=B5=D1=81=D1=81?= =?UTF-8?q?=D0=B8=D1=8F=20=D0=B5=D1=81=D1=82=D1=8C,=20=D0=B0=20cookie=20?= =?UTF-8?q?=D0=BD=D0=B5=D1=82?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit На проде любые изменения данных, включая выход из аккаунта, отвечали 403 CSRF_FAILED, хотя сайт оставался залогиненным. Cookie csrf_token одна на игрока и админку: вход в админку перезаписывал её со сроком 8 часов, а сессия игрока живёт 7 дней. Когда токен истекал, сервер выдавал новый только при входе, а войти и выйти мешала та же проверка. Из этого состояния было не выбраться, кроме как стереть cookie сайта руками. Теперь CSRFMiddleware перевыдаёт токен на любом ответе /api, если запрос несёт сессионную cookie без csrf_token, в том числе на самом отказе. SPA на загрузке делает GET /api/users/me, поэтому пользователю хватает перезагрузить страницу. Правится только стартовое сообщение ответа, тело идёт насквозь, и SSE-поток не буферизуется. Срок токена теперь не короче самой долгой сессии, так что вход в админку больше не укорачивает токен игрока. Проверка double-submit не ослаблена: запрос с cookie, но без заголовка или с чужим токеном по-прежнему получает 403, и cookie в этом случае не перевыдаётся. Перевыданный токен из кросс-сайтового ответа атакующему ничего не даёт: прочитать cookie может только JS того же origin. Тесты закрепляют восстановление на GET и на отказе, срок после входа в админку, прежнюю строгость проверки, отсутствие токена у анонимов и то, что middleware не склеивает чанки потока. На старом коде четыре из них падают. #50 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XfTsytzT6TojfmprRDKiV6 --- backend/app/core/security.py | 23 +++++- backend/app/main.py | 33 +++++++- backend/tests/test_csrf.py | 150 +++++++++++++++++++++++++++++++++++ 3 files changed, 198 insertions(+), 8 deletions(-) create mode 100644 backend/tests/test_csrf.py diff --git a/backend/app/core/security.py b/backend/app/core/security.py index 14f9036..5b118bf 100644 --- a/backend/app/core/security.py +++ b/backend/app/core/security.py @@ -65,12 +65,19 @@ def generate_csrf_token() -> str: return secrets.token_urlsafe(24) -def _set_csrf_cookie(response: Response, max_age: int) -> str: +def _csrf_max_age() -> int: + """Токен один на обе сессии, поэтому живёт не меньше самой долгой из них: иначе вход + в админку (8 ч) перезаписывал токен игрока (7 дней), и после его истечения все мутации + игрока падали с CSRF_FAILED. Без сессии токен бесполезен — лишний срок безопасен.""" + return max(settings.jwt_user_ttl_minutes, settings.jwt_admin_ttl_minutes) * 60 + + +def _set_csrf_cookie(response: Response) -> str: csrf = generate_csrf_token() response.set_cookie( key=CSRF_COOKIE, value=csrf, - max_age=max_age, + max_age=_csrf_max_age(), httponly=False, # должен читаться JS, чтобы продублировать в заголовок secure=settings.cookie_secure, samesite="lax", @@ -80,6 +87,14 @@ def _set_csrf_cookie(response: Response, max_age: int) -> str: return csrf +def fresh_csrf_set_cookie() -> tuple[bytes, bytes]: + """Готовый заголовок Set-Cookie со свежим токеном — для ASGI-middleware, где объекта + Response нет. Строится тем же _set_csrf_cookie, чтобы атрибуты не разъехались.""" + carrier = Response() + _set_csrf_cookie(carrier) + return next((k, v) for k, v in carrier.raw_headers if k == b"set-cookie") + + def set_user_session(response: Response, user_id: int, provider: str) -> None: ttl = settings.jwt_user_ttl_minutes token = create_token(user_id, AUDIENCE_USER, ttl, provider) @@ -93,7 +108,7 @@ def set_user_session(response: Response, user_id: int, provider: str) -> None: path=_USER_PATH, domain=settings.cookie_domain_value, ) - _set_csrf_cookie(response, ttl * 60) + _set_csrf_cookie(response) def set_admin_session(response: Response, admin_id: int) -> None: @@ -109,7 +124,7 @@ def set_admin_session(response: Response, admin_id: int) -> None: path=_ADMIN_PATH, domain=settings.cookie_domain_value, ) - _set_csrf_cookie(response, ttl * 60) + _set_csrf_cookie(response) def clear_user_session(response: Response) -> None: diff --git a/backend/app/main.py b/backend/app/main.py index 46bccc4..ad9fca6 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -34,12 +34,35 @@ _STATIC_DIR = Path(os.getenv("STATIC_DIR", str(Path(__file__).resolve().parent.p _UNSAFE_METHODS = {"POST", "PUT", "PATCH", "DELETE"} +def _with_fresh_csrf_cookie(send): # noqa: ANN001, ANN202 + """Дописывает свежий csrf_token в заголовки ответа. Трогает только + http.response.start: тело (в т.ч. SSE-поток) проходит насквозь, чанк за чанком.""" + + async def wrapped(message): # noqa: ANN001 + if message["type"] == "http.response.start": + headers = list(message.get("headers", [])) + already_set = any( + k.lower() == b"set-cookie" and v.startswith(security.CSRF_COOKIE.encode() + b"=") + for k, v in headers + ) + if not already_set: + headers.append(security.fresh_csrf_set_cookie()) + message = {**message, "headers": headers} + await send(message) + + return wrapped + + class CSRFMiddleware: """Double-submit CSRF на чистом ASGI: для аутентифицированных мутаций на /api требуем совпадения заголовка X-CSRF-Token и cookie csrf_token. + Если сессия есть, а csrf_token в запросе нет (cookie истекла или её стёрли), любой ответ + на /api — включая отказ ниже — перевыдаёт токен. Иначе состояние не лечилось: токен + выдаётся только при входе, а войти и выйти мешала эта же проверка. + Намеренно НЕ на BaseHTTPMiddleware: тот буферизует потоковые ответы и ломает SSE - (/api/events). Чистый ASGI пропускает стримы насквозь, вмешиваясь только при отказе CSRF. + (/api/events). Чистый ASGI пропускает стримы насквозь. """ def __init__(self, app) -> None: # noqa: ANN001 @@ -48,13 +71,15 @@ class CSRFMiddleware: async def __call__(self, scope, receive, send): # noqa: ANN001 if scope["type"] == "http": request = Request(scope) - if request.method in _UNSAFE_METHODS and request.url.path.startswith("/api"): + if request.url.path.startswith("/api"): has_session = ( security.USER_COOKIE in request.cookies or security.ADMIN_COOKIE in request.cookies ) - if has_session: - cookie_token = request.cookies.get(security.CSRF_COOKIE) + cookie_token = request.cookies.get(security.CSRF_COOKIE) + if has_session and not cookie_token: + send = _with_fresh_csrf_cookie(send) + if has_session and request.method in _UNSAFE_METHODS: header_token = request.headers.get(security.CSRF_HEADER) if not cookie_token or cookie_token != header_token: response = JSONResponse( diff --git a/backend/tests/test_csrf.py b/backend/tests/test_csrf.py new file mode 100644 index 0000000..28bcb09 --- /dev/null +++ b/backend/tests/test_csrf.py @@ -0,0 +1,150 @@ +"""CSRF double-submit: токен восстанавливается, если сессия пережила cookie csrf_token, +а сама проверка мутаций остаётся такой же строгой.""" +from __future__ import annotations + +import asyncio + +from fastapi.testclient import TestClient + +from tests.conftest import csrf_headers, login + + +def _set_cookie(resp, name: str) -> str | None: + """Заголовок Set-Cookie для cookie name (или None, если ответ её не ставит).""" + for header in resp.headers.get_list("set-cookie"): + if header.startswith(f"{name}="): + return header + return None + + +def _max_age(set_cookie: str) -> int: + for part in set_cookie.split(";"): + key, _, value = part.strip().partition("=") + if key.lower() == "max-age": + return int(value) + raise AssertionError(f"нет Max-Age: {set_cookie}") + + +def test_missing_token_reissued_on_safe_request(client: TestClient): + """Сессия жива, csrf_token истёк → первый же GET отдаёт новый токен, мутации проходят.""" + login(client, "Игрок") + client.cookies.delete("csrf_token") + + me = client.get("/api/users/me") + assert me.status_code == 200, me.text + assert _set_cookie(me, "csrf_token") is not None + assert client.cookies.get("csrf_token") + + r = client.patch( + "/api/users/me/profile", json={"favorite_faction_id": None}, headers=csrf_headers(client) + ) + assert r.status_code == 200, r.text + + +def test_rejected_mutation_reissues_token(client: TestClient): + """Отказ CSRF без cookie сам выдаёт токен: иначе не выйти и не перезайти.""" + login(client, "Игрок") + client.cookies.delete("csrf_token") + + r = client.post("/api/auth/logout") + assert r.status_code == 403 + assert r.json()["error"]["code"] == "CSRF_FAILED" + assert _set_cookie(r, "csrf_token") is not None + + r2 = client.post("/api/auth/logout", headers=csrf_headers(client)) + assert r2.status_code == 200, r2.text + + +def test_admin_login_does_not_shorten_token(client: TestClient, make_admin): + """Вход в админку перезаписывает общий csrf_token — срок не короче сессии игрока.""" + r_user = client.post("/api/auth/dev/login", json={"nickname": "Игрок"}) + session_age = _max_age(_set_cookie(r_user, "fs_session")) + + make_admin("boss", "secret123") + r_admin = client.post( + "/api/admin/auth/login", + json={"username": "boss", "password": "secret123"}, + headers=csrf_headers(client), + ) + assert r_admin.status_code == 200, r_admin.text + assert _max_age(_set_cookie(r_admin, "csrf_token")) >= session_age + + +def test_check_is_not_weakened(client: TestClient): + """Cookie есть, заголовка нет или он чужой — 403, и токен при этом не перевыдаётся.""" + login(client, "Игрок") + token = client.cookies.get("csrf_token") + body = {"favorite_faction_id": None} + + no_header = client.patch("/api/users/me/profile", json=body) + assert no_header.status_code == 403 + assert _set_cookie(no_header, "csrf_token") is None + + wrong = client.patch("/api/users/me/profile", json=body, headers={"X-CSRF-Token": "forged"}) + assert wrong.status_code == 403 + assert _set_cookie(wrong, "csrf_token") is None + + assert client.cookies.get("csrf_token") == token + + +def test_anonymous_gets_no_token(client: TestClient): + r = client.get("/api/auth/config") + assert r.status_code == 200 + assert _set_cookie(r, "csrf_token") is None + + +def _run_middleware(app_messages: list[dict], cookie: bytes) -> list[dict]: + """Прогоняет CSRFMiddleware над фейковым приложением и возвращает отправленное.""" + from app.main import CSRFMiddleware + + async def fake_app(scope, receive, send): # noqa: ANN001 + for message in app_messages: + await send(message) + + sent: list[dict] = [] + + async def send(message): # noqa: ANN001 + sent.append(message) + + async def receive(): # pragma: no cover — фейковому приложению тело запроса не нужно + return {"type": "http.request", "body": b"", "more_body": False} + + scope = { + "type": "http", + "method": "GET", + "path": "/api/events", + "raw_path": b"/api/events", + "root_path": "", + "scheme": "http", + "server": ("testserver", 80), + "query_string": b"", + "headers": [(b"cookie", cookie)], + } + asyncio.run(CSRFMiddleware(fake_app)(scope, receive, send)) + return sent + + +def test_reissue_keeps_stream_unbuffered(): + """Перевыдача трогает только стартовое сообщение: чанки SSE идут по одному, без склейки.""" + start = {"type": "http.response.start", "status": 200, "headers": [(b"content-type", b"text/event-stream")]} + chunks = [ + {"type": "http.response.body", "body": b": connected\n\n", "more_body": True}, + {"type": "http.response.body", "body": b": ping\n\n", "more_body": True}, + {"type": "http.response.body", "body": b"", "more_body": False}, + ] + sent = _run_middleware([start, *chunks], cookie=b"fs_session=abc") + + assert sent[1:] == chunks + cookies = [v for k, v in sent[0]["headers"] if k == b"set-cookie"] + assert len(cookies) == 1 and cookies[0].startswith(b"csrf_token=") + + +def test_reissue_does_not_duplicate_app_cookie(): + """Если приложение само ставит csrf_token (вход), второй Set-Cookie не дописывается.""" + own = (b"set-cookie", b"csrf_token=from-app; Path=/") + start = {"type": "http.response.start", "status": 200, "headers": [own]} + body = {"type": "http.response.body", "body": b"{}", "more_body": False} + sent = _run_middleware([start, body], cookie=b"fs_session=abc") + + cookies = [v for k, v in sent[0]["headers"] if k == b"set-cookie"] + assert cookies == [own[1]]