From 4ecc3d689c0594604a972ad136c158d4f0b0ce43 Mon Sep 17 00:00:00 2001 From: bot-backend Date: Thu, 30 Jul 2026 20:14:26 +0300 Subject: [PATCH] =?UTF-8?q?fix(tradein/auth):=20=D0=BF=D1=80=D0=B5=D0=B4?= =?UTF-8?q?=D0=BE=D1=82=D0=B2=D1=80=D0=B0=D1=82=D0=B8=D1=82=D1=8C=20=D0=BF?= =?UTF-8?q?=D0=BE=D0=B4=D0=BC=D0=B5=D0=BD=D1=83=20X-Authenticated-User=20?= =?UTF-8?q?=D0=BF=D1=80=D0=B8=20session-auth=20(#2552)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CRITICAL: _propagate_authenticated_user делала skip-if-present вместо перезаписи — клиент-контролируемый X-Authenticated-User (Caddy шлёт его на КАЖДЫЙ прод-запрос) выигрывал у резолвленной сессии для всего downstream-трафика, читающего заголовок напрямую (_assert_estimate_access*, account_quota, /trade-in/history, support.py) — в обоих auth_mode (dual и db_only). Теперь заголовок безусловно перезаписывается сессионным username (ASGI header-имена всегда lowercase bytes). Medium: .encode("latin-1") без errors="replace" крашил бы 500-кой каждый запрос кириллического username. Login timing-oracle — verify_password короткозамыкалась на unknown-username/NULL-hash (~1мс vs ~100-300мс bcrypt) → теперь всегда сверяется против dummy-хеша при отсутствующем юзере/хеше. Login rate-limit key length-prefixed — username с ':' (или IPv6 IP) больше не может схлопнуть чужой бюджет. Новые тесты подтверждают регрессию: прогнаны на старом коде (до фикса) через временный откат rbac.py — все три (spoof dual-mode, spoof db_only, кириллица) падали с 'victim' == 'alice' / UnicodeEncodeError; после фикса — зелёные. test_rbac.py/test_internal_auth_secret.py без изменений. --- tradein-mvp/backend/app/api/v1/auth.py | 38 +++++-- tradein-mvp/backend/app/core/rbac.py | 36 +++++-- tradein-mvp/backend/tests/test_auth_api.py | 118 ++++++++++++++++++++- 3 files changed, 170 insertions(+), 22 deletions(-) diff --git a/tradein-mvp/backend/app/api/v1/auth.py b/tradein-mvp/backend/app/api/v1/auth.py index 3c03987d..9933bc2e 100644 --- a/tradein-mvp/backend/app/api/v1/auth.py +++ b/tradein-mvp/backend/app/api/v1/auth.py @@ -9,9 +9,20 @@ Security: - Неверные creds (неизвестный username / неактивен / password_hash NULL / неверный пароль) → ОДИНАКОВЫЙ 401 с generic сообщением — не раскрываем, существует ли username (user-enumeration защита). + - #2552 post-review Medium 2: `verify_password` ВСЕГДА вызывается ровно + один раз — для несуществующего username / NULL password_hash сверяем + против статичного dummy-хеша (`_DUMMY_PASSWORD_HASH`, сгенерирован один + раз на импорте модуля), результат игнорируется. Без этого короткое + замыкание (`user is None → сразу 401`) давало наблюдаемую разницу во + времени ответа (~1мс без bcrypt vs ~100-300мс с ним) — классический + timing-oracle для user-enumeration, даже при одинаковом detail-сообщении. - Rate-limit по (username, IP) — ЖЁСТЧЕ общего `RateLimitMiddleware` (`/api/*`), т.к. login — типичная brute-force поверхность. Использует - `SlidingWindowLimiter` (тот же примитив, что и общий rate-limit). + `SlidingWindowLimiter` (тот же примитив, что и общий rate-limit). Ключ + length-prefixed (`len(username):username:ip`) — без этого произвольный + username с `:` внутри мог бы схлопнуть бюджет с другой (username, ip) + парой (IPv6-адреса тоже содержат `:`, так что просто эскейпить разделитель + в username недостаточно — паразитная граница возможна с обеих сторон). - Raw-пароль НИКОГДА не логируется и не попадает в user_events payload — только username/ip/user_agent/path/method (см. schedule_event ниже). """ @@ -19,6 +30,7 @@ Security: from __future__ import annotations import logging +import secrets from typing import Annotated from fastapi import APIRouter, Depends, HTTPException, Request, Response @@ -27,7 +39,7 @@ from sqlalchemy.orm import Session from app.core.config import settings from app.core.db import get_db -from app.core.password import verify_password +from app.core.password import hash_password, verify_password from app.core.ratelimit import SlidingWindowLimiter, _client_ip from app.services.auth_session import create_session, get_user_by_username, revoke_session from app.services.user_events import schedule_event @@ -46,6 +58,13 @@ _LOGIN_LIMITER = SlidingWindowLimiter( window_s=settings.login_rate_limit_window_s, ) +# Timing-oracle защита (см. module docstring): bcrypt-хеш случайного пароля, +# сгенерированный ОДИН РАЗ на импорте модуля — используется вместо +# password_hash, когда юзер не найден/деактивирован/без пароля, чтобы +# `verify_password` (доминирующая по времени операция, ~100-300мс) всегда +# отрабатывала полный bcrypt-компар, независимо от того, существует ли аккаунт. +_DUMMY_PASSWORD_HASH = hash_password(secrets.token_urlsafe(16)) + _INVALID_CREDENTIALS_DETAIL = "неверный логин или пароль" @@ -67,7 +86,7 @@ async def login( ) -> LoginResponse: ip = _client_ip(request) user_agent = request.headers.get("user-agent") - rate_key = f"{body.username}:{ip}" + rate_key = f"{len(body.username)}:{body.username}:{ip}" retry_after = _LOGIN_LIMITER.check(rate_key) if retry_after is not None: @@ -78,12 +97,15 @@ async def login( ) user = get_user_by_username(db, body.username) - credentials_ok = ( - user is not None - and user["is_active"] - and user["password_hash"] is not None - and verify_password(body.password, user["password_hash"]) + hash_to_check = ( + user["password_hash"] + if user is not None and user["password_hash"] is not None + else _DUMMY_PASSWORD_HASH ) + # ВСЕГДА вызывается — dummy-хеш при отсутствующем юзере/NULL password_hash + # держит время ответа одинаковым независимо от существования аккаунта. + password_ok = verify_password(body.password, hash_to_check) + credentials_ok = user is not None and user["is_active"] and password_ok if not credentials_ok: schedule_event( diff --git a/tradein-mvp/backend/app/core/rbac.py b/tradein-mvp/backend/app/core/rbac.py index 2f0aed52..ffaf1a04 100644 --- a/tradein-mvp/backend/app/core/rbac.py +++ b/tradein-mvp/backend/app/core/rbac.py @@ -95,12 +95,25 @@ def _db_role_path_allowed(role: str, path: str) -> bool: def _propagate_authenticated_user(request: Request, username: str) -> None: - """Инжектит ``X-Authenticated-User`` в ASGI scope (если его там ещё нет), - чтобы ``RateLimitMiddleware``/``RequestAuditMiddleware`` (оба читают сырой - заголовок напрямую, #2213/#2550) и downstream route-хендлеры (читающие - его через FastAPI ``Header()``) видели сессионного DB-юзера так же, как - Caddy trusted-header юзера — без правок в каждом из этих мест по - отдельности (минимально инвазивный способ). + """Инжектит ``X-Authenticated-User`` в ASGI scope — ПЕРЕЗАПИСЫВАЯ, а не + только добавляя при отсутствии, — чтобы ``RateLimitMiddleware``/ + ``RequestAuditMiddleware`` (оба читают сырой заголовок напрямую, + #2213/#2550) и downstream route-хендлеры (читающие его через FastAPI + ``Header()``) видели РЕЗОЛВЛЕННОГО ИЗ СЕССИИ юзера — без правок в каждом + из этих мест по отдельности (минимально инвазивный способ). + + #2552 post-review fix (CRITICAL): раньше это была skip-if-present + мутация (``if request.headers.get(...): return``) — сессия резолвилась + ПЕРВОЙ (см. rbac_guard), но клиент-контролируемый ``X-Authenticated-User`` + (который Caddy шлёт на КАЖДЫЙ прод-запрос) выигрывал у неё для ВСЕГО + downstream-трафика: атакующий с валидной cookie юзера ``alice`` мог + подделать заголовок ``X-Authenticated-User: victim`` и получить доступ к + данным victim в ~15 роутах, читающих заголовок напрямую + (``_assert_estimate_access*``, ``account_quota``, ``/trade-in/history``, + ``support.py``) — работало в ОБОИХ auth_mode (dual и db_only), т.к. эти + хендлеры не знают про rbac_guard'овский ``from_session`` флаг, только про + сырой заголовок. Session-identity ДОЛЖНА быть источником истины, если + сессия резолвлена — полная перезапись, не skip. Механизм: ``request.scope`` — ОДИН и тот же dict-объект, прокинутый по ссылке через весь ASGI call chain (Starlette не копирует scope между @@ -112,6 +125,10 @@ def _propagate_authenticated_user(request: Request, username: str) -> None: предыдущие) и читает ``request.headers`` уже ПОСЛЕ ``call_next()`` отработал весь внутренний стек, включая эту мутацию. + ASGI header-имена — всегда lowercase bytes (см. ASGI spec), поэтому + фильтр по ``b"x-authenticated-user"`` ловит заголовок независимо от + регистра, в котором его прислал клиент (Starlette уже нормализует). + Известное ограничение: ``RateLimitMiddleware`` тоже внешний относительно rbac_guard, но читает заголовок ДО вызова call_next() (до того, как этот guard успевает отработать) — для ЭТОГО конкретного запроса сессионный @@ -122,12 +139,9 @@ def _propagate_authenticated_user(request: Request, username: str) -> None: точного per-user квотинга для DB-юзеров — переносить резолв сессии выше RateLimit в app/main.py отдельным issue. """ - if request.headers.get("X-Authenticated-User"): - return request.scope["headers"] = [ - *request.scope.get("headers", []), - (b"x-authenticated-user", username.encode("latin-1")), - ] + (k, v) for k, v in request.scope.get("headers", []) if k != b"x-authenticated-user" + ] + [(b"x-authenticated-user", username.encode("latin-1", "replace"))] async def rbac_guard( diff --git a/tradein-mvp/backend/tests/test_auth_api.py b/tradein-mvp/backend/tests/test_auth_api.py index 0eb828e2..8b55fb13 100644 --- a/tradein-mvp/backend/tests/test_auth_api.py +++ b/tradein-mvp/backend/tests/test_auth_api.py @@ -18,12 +18,12 @@ from __future__ import annotations import os from datetime import UTC, datetime, timedelta from types import SimpleNamespace -from typing import Any +from typing import Annotated, Any os.environ.setdefault("DATABASE_URL", "postgresql+psycopg://test:test@localhost:5432/test") import pytest -from fastapi import FastAPI +from fastapi import FastAPI, Header from fastapi.testclient import TestClient from app.api.v1 import auth as auth_router @@ -182,6 +182,15 @@ def _build_test_app(store: _Store) -> FastAPI: async def tradein_dummy() -> dict: return {"ok": True} + @app.get("/api/v1/trade-in/whoami") + async def tradein_whoami( + x_authenticated_user: Annotated[str | None, Header(alias="X-Authenticated-User")] = None, + ) -> dict: + """Echoes the X-Authenticated-User header exactly as a downstream handler + (`_assert_estimate_access*`, `account_quota`, etc.) would see it — used to + assert session-identity wins over a client-forged header (#2552 spoof fix).""" + return {"user": x_authenticated_user} + def _override_get_db(): # generator dependency — matches app.core.db.get_db shape yield _FakeDB(store) @@ -271,6 +280,47 @@ def test_login_null_password_hash_401(client: TestClient, store: _Store) -> None assert resp.status_code == 401 +def test_login_always_calls_verify_password_timing_oracle_guard( + client: TestClient, store: _Store, monkeypatch: pytest.MonkeyPatch +) -> None: + """#2552 post-review Medium 2: `verify_password` должен выполняться ровно + один раз на КАЖДУЮ попытку логина — включая неизвестный username и NULL + password_hash — иначе короткое замыкание даёт наблюдаемый timing-oracle + для user-enumeration. Тест не измеряет тайминг (флейки в CI), а проверяет + сам факт + аргумент вызова через monkeypatch-счётчик.""" + store.add_user("alice", hash_password("Secret123!"), role="employee") + store.add_user("nullhash", None, role="employee") + + calls: list[str] = [] + real_verify = auth_router.verify_password + + def _counting_verify(plain: str, hashed: str) -> bool: + calls.append(hashed) + return real_verify(plain, hashed) + + monkeypatch.setattr(auth_router, "verify_password", _counting_verify) + + resp_unknown = client.post("/api/v1/auth/login", json={"username": "ghost", "password": "x"}) + assert resp_unknown.status_code == 401 + + resp_null_hash = client.post( + "/api/v1/auth/login", json={"username": "nullhash", "password": "x"} + ) + assert resp_null_hash.status_code == 401 + + resp_wrong_pw = client.post( + "/api/v1/auth/login", json={"username": "alice", "password": "wrong"} + ) + assert resp_wrong_pw.status_code == 401 + + assert len(calls) == 3 + # Unknown user / NULL hash — сверяется против dummy-хеша, не против NULL. + assert calls[0] == auth_router._DUMMY_PASSWORD_HASH + assert calls[1] == auth_router._DUMMY_PASSWORD_HASH + # Реальный юзер с реальным hash — НЕ dummy. + assert calls[2] != auth_router._DUMMY_PASSWORD_HASH + + def test_login_rate_limit_429(client: TestClient, store: _Store) -> None: store.add_user("dave", hash_password("Secret123!"), role="employee") limit = config.settings.login_rate_limit @@ -421,4 +471,66 @@ def test_session_user_can_reach_tradein_but_not_admin(client: TestClient, store: assert ok.status_code == 200 denied = client.get("/api/v1/admin/dummy") - assert denied.status_code in (401, 403, 404) + # rbac_guard's admin-gate matches the path regex BEFORE routing even happens + # (route isn't registered on this test app) — role=employee != admin -> 403, + # never a 404 (a bare "any non-2xx" assertion would mask a rbac_guard typo). + assert denied.status_code == 403 + + +# --------------------------------------------------------------------------- +# #2552 post-review CRITICAL fix: session identity must win over a spoofed +# client-sent X-Authenticated-User header (was a skip-if-present bug — the +# forged header used to override the session for every downstream reader of +# the raw header: _assert_estimate_access*, account_quota, /trade-in/history, +# support.py — in BOTH auth_mode=dual and db_only). +# --------------------------------------------------------------------------- + + +def test_session_identity_wins_over_spoofed_header_dual_mode( + client: TestClient, store: _Store +) -> None: + store.add_user("alice", hash_password("Secret123!"), role="employee") + store.add_user("victim", hash_password("Secret123!"), role="employee") + client.post("/api/v1/auth/login", json={"username": "alice", "password": "Secret123!"}) + + resp = client.get( + "/api/v1/trade-in/whoami", + headers={"X-Authenticated-User": "victim"}, + ) + assert resp.status_code == 200 + assert resp.json()["user"] == "alice" + + +def test_session_identity_wins_over_spoofed_header_db_only_mode( + client: TestClient, store: _Store, monkeypatch: pytest.MonkeyPatch +) -> None: + store.add_user("alice", hash_password("Secret123!"), role="employee") + store.add_user("victim", hash_password("Secret123!"), role="employee") + client.post("/api/v1/auth/login", json={"username": "alice", "password": "Secret123!"}) + + monkeypatch.setattr(config.settings, "auth_mode", "db_only") + + resp = client.get( + "/api/v1/trade-in/whoami", + headers={"X-Authenticated-User": "victim"}, + ) + assert resp.status_code == 200 + assert resp.json()["user"] == "alice" + + +def test_cyrillic_username_session_propagation_does_not_500( + client: TestClient, store: _Store +) -> None: + """#2552 post-review Medium 1: `.encode("latin-1")` без errors="replace" на + кириллическом username крашил бы КАЖДЫЙ запрос такого юзера с 500.""" + store.add_user("алиса", hash_password("Secret123!"), role="employee") + login_resp = client.post( + "/api/v1/auth/login", json={"username": "алиса", "password": "Secret123!"} + ) + assert login_resp.status_code == 200, login_resp.text + + resp = client.get("/api/v1/trade-in/whoami") + assert resp.status_code == 200, resp.text + # latin-1 "replace" гарантированно не крашит — точное значение (что именно + # получится из non-latin1 байт) не является контрактом, важно отсутствие 500. + assert resp.json()["user"] is not None