fix(tradein/auth): предотвратить подмену X-Authenticated-User при session-auth (#2552)

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

@@ -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 = (
+    hash_to_check = (
-        user is not None
+        user["password_hash"]
-        and user["is_active"]
+        if user is not None and user["password_hash"] is not None
-        and user["password_hash"] is not None
+        else _DUMMY_PASSWORD_HASH
-        and verify_password(body.password, user["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(

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 (если его там ещё нет),
+    """Инжектит ``X-Authenticated-User`` в ASGI scope — ПЕРЕЗАПИСЫВАЯ, а не
-    чтобы ``RateLimitMiddleware``/``RequestAuditMiddleware`` (оба читают сырой
+    только добавляя при отсутствии, — чтобы ``RateLimitMiddleware``/
-    заголовок напрямую, #2213/#2550) и downstream route-хендлеры (читающие
+    ``RequestAuditMiddleware`` (оба читают сырой заголовок напрямую,
-    его через FastAPI ``Header()``) видели сессионного DB-юзера так же, как
+    #2213/#2550) и downstream route-хендлеры (читающие его через FastAPI
-    Caddy trusted-header юзера — без правок в каждом из этих мест по
+    ``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", []),
+        (k, v) for k, v in request.scope.get("headers", []) if k != b"x-authenticated-user"
-        (b"x-authenticated-user", username.encode("latin-1")),
+    ] + [(b"x-authenticated-user", username.encode("latin-1", "replace"))]
-    ]
 
 
 async def rbac_guard(

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