fix(tradein): scope GET /history to caller, close cross-pilot data leak (#656) (#670)

Co-authored-by: bot-backend <bot-backend@gendsgn.local>
Co-committed-by: bot-backend <bot-backend@gendsgn.local>

tradein-mvp/backend/app/api/v1/trade_in.py

@@ -61,7 +61,7 @@ async def estimate(
     """
     account_quota.check_and_raise(db, x_authenticated_user)
     from app.services.estimator import estimate_quality
-    result = await estimate_quality(payload, db)
+    result = await estimate_quality(payload, db, created_by=x_authenticated_user)
     account_quota.increment(db, x_authenticated_user)
     return result
 
@@ -397,19 +397,57 @@ def get_photo(
 def estimate_history(
     db: Annotated[Session, Depends(get_db)],
     limit: int = 50,
+    account: str | None = None,
+    x_authenticated_user: Annotated[str | None, Header(alias="X-Authenticated-User")] = None,
 ) -> list[dict[str, object]]:
-    """История оценок (#399) — последние N записей trade_in_estimates."""
+    """История оценок (#399) — последние N записей trade_in_estimates.
+
+    Скоупинг (#656 — закрывает cross-pilot data-leak): non-admin видит ТОЛЬКО
+    свои оценки (created_by = X-Authenticated-User); legacy NULL-строки без
+    владельца не попадают. Admin видит все строки, либо фильтрует по ?account=<user>.
+    401 если заголовок отсутствует (mirror /me — Caddy basic_auth обязателен).
+    """
+    if not x_authenticated_user:
+        raise HTTPException(
+            status_code=401,
+            detail="no authenticated user (Caddy basic_auth required)",
+        )
+
+    from app.core.auth import get_role
+
+    try:
+        role = get_role(x_authenticated_user)
+    except KeyError:
+        logger.warning(
+            "user %r authenticated via Caddy but missing from roles.yaml",
+            x_authenticated_user,
+        )
+        raise HTTPException(status_code=403, detail="user not in roles config") from None
+
+    params: dict[str, object] = {"limit": min(max(limit, 1), 200)}
+    where = ""
+    if role == "admin":
+        # Admin: все строки, либо фильтр по конкретному аккаунту.
+        if account:
+            where = "WHERE created_by = :account"
+            params["account"] = account
+    else:
+        # Non-admin: жёстко скоупим на свои строки; ?account игнорируется.
+        where = "WHERE created_by = :owner"
+        params["owner"] = x_authenticated_user
+
     rows = db.execute(
         text(
-            """
+            f"""
             SELECT id, address, rooms, area_m2, median_price,
                    confidence, n_analogs, created_at
             FROM trade_in_estimates
+            {where}
             ORDER BY created_at DESC
             LIMIT :limit
             """
         ),
-        {"limit": min(max(limit, 1), 200)},
+        params,
     ).mappings().all()
     return [dict(r) for r in rows]
 

tradein-mvp/backend/app/services/estimator.py

@@ -659,7 +659,9 @@ def _save_yandex_history_items(
 
 
 # ── Public ───────────────────────────────────────────────────────────────────
-async def estimate_quality(payload: TradeInEstimateInput, db: Session) -> AggregatedEstimate:
+async def estimate_quality(
+    payload: TradeInEstimateInput, db: Session, created_by: str | None = None
+) -> AggregatedEstimate:
     """Главная функция — оценка квартиры по реальным данным.
 
     PR M / #564 Phase 3: rosreestr_deals **included** в actual_deals output.
@@ -680,7 +682,7 @@ async def estimate_quality(payload: TradeInEstimateInput, db: Session) -> Aggreg
     if geo is None:
         # Без координат не можем искать через PostGIS. Возвращаем low confidence.
         logger.warning("geocode failed for %s — returning low-confidence estimate", payload.address)
-        return _empty_estimate(payload, db, reason="address_not_geocoded")
+        return _empty_estimate(payload, db, reason="address_not_geocoded", created_by=created_by)
 
     # 1b. DaData enrichment (PR Q1) — on-demand cleanup для target адреса.
     # Best-effort: graceful None при отсутствии credentials / quota / fail.
@@ -1025,6 +1027,7 @@ async def estimate_quality(payload: TradeInEstimateInput, db: Session) -> Aggreg
                 expected_sold_price, expected_sold_range_low,
                 expected_sold_range_high, expected_sold_per_m2,
                 asking_to_sold_ratio, ratio_basis,
+                created_by,
                 expires_at
             ) VALUES (
                 CAST(:id AS uuid),
@@ -1044,6 +1047,7 @@ async def estimate_quality(payload: TradeInEstimateInput, db: Session) -> Aggreg
                 :expected_sold_price, :expected_sold_range_low,
                 :expected_sold_range_high, :expected_sold_per_m2,
                 :asking_to_sold_ratio, :ratio_basis,
+                :created_by,
                 :expires_at
             )
             """
@@ -1092,6 +1096,7 @@ async def estimate_quality(payload: TradeInEstimateInput, db: Session) -> Aggreg
             "expected_sold_per_m2": expected_sold_per_m2,
             "asking_to_sold_ratio": asking_to_sold_ratio,
             "ratio_basis": ratio_basis,
+            "created_by": created_by,
             "expires_at": expires_at,
         },
     )
@@ -2026,7 +2031,7 @@ def _deal_to_analog(row: dict[str, Any]) -> AnalogLot:
 
 
 def _empty_estimate(
-    payload: TradeInEstimateInput, db: Session, *, reason: str
+    payload: TradeInEstimateInput, db: Session, *, reason: str, created_by: str | None = None
 ) -> AggregatedEstimate:
     """Fallback когда нет данных для оценки.
 
@@ -2049,6 +2054,7 @@ def _empty_estimate(
                 confidence, confidence_explanation, n_analogs,
                 analogs, actual_deals,
                 sources_used,
+                created_by,
                 expires_at
             ) VALUES (
                 CAST(:id AS uuid), :address,
@@ -2059,6 +2065,7 @@ def _empty_estimate(
                 'low', :explanation, 0,
                 '[]'::jsonb, '[]'::jsonb,
                 '[]'::jsonb,
+                :created_by,
                 :expires_at
             )
             """
@@ -2079,6 +2086,7 @@ def _empty_estimate(
             "client_name": payload.client_name,
             "client_phone": payload.client_phone,
             "explanation": reason,
+            "created_by": created_by,
             "expires_at": expires_at,
         },
     )

tradein-mvp/backend/data/sql/083_trade_in_estimates_created_by.sql

@@ -0,0 +1,34 @@
+-- 083_trade_in_estimates_created_by.sql
+-- #656 [P1, security/data-leak] — добавляем владельца оценки для скоупинга GET /history.
+--
+-- ПРОБЛЕМА: GET /history SELECT'ил последние оценки ВСЕХ пользователей (без фильтра)
+--   → cross-pilot data leak (один пилот видел адреса/клиентов другого). trade_in_estimates
+--   не имела колонки владельца. Caddy basic_auth прокидывает X-Authenticated-User в каждый
+--   аутентифицированный запрос; RBAC middleware уже 401'ит неизвестных юзеров, так что
+--   заголовок гарантированно присутствует для реальных вызовов.
+--
+-- ФИКС: created_by text (username из X-Authenticated-User). Estimator пишет его на INSERT.
+--   GET /history скоупит: non-admin → WHERE created_by = :user; admin → все строки
+--   (или ?account=<user> фильтр). Legacy-строки остаются created_by=NULL — они без
+--   владельца, поэтому non-admin их НЕ видит (корректно: нет owner = не показываем).
+--
+-- Индекс (created_by, created_at DESC) обслуживает основной паттерн запроса /history
+--   (ORDER BY created_at DESC LIMIT N, опционально WHERE created_by = :user).
+-- Idempotent: ADD COLUMN IF NOT EXISTS / CREATE INDEX IF NOT EXISTS.
+-- Apply after: 082_scrape_schedules_seed_ratio_refresh.sql
+
+BEGIN;
+
+ALTER TABLE trade_in_estimates
+    ADD COLUMN IF NOT EXISTS created_by text;
+
+CREATE INDEX IF NOT EXISTS idx_trade_in_estimates_created_by_created_at
+    ON trade_in_estimates (created_by, created_at DESC);
+
+COMMENT ON COLUMN trade_in_estimates.created_by IS
+    'Username (X-Authenticated-User из Caddy basic_auth), создавший оценку. '
+    'Используется для скоупинга GET /history (#656): non-admin видит только свои '
+    'строки. NULL для legacy-строк, созданных до миграции 083 — они без владельца '
+    'и в non-admin историю не попадают.';
+
+COMMIT;

tradein-mvp/backend/tests/test_history_scope.py

@@ -0,0 +1,155 @@
+"""Tests for GET /api/v1/trade-in/history scoping (#656).
+
+Закрывает cross-pilot data-leak: non-admin видит только свои оценки
+(created_by = X-Authenticated-User), admin видит все либо фильтрует по ?account.
+
+Проверяем САМ скоупинг через перехват SQL/params, переданных в db.execute —
+реальная БД не нужна (DB + get_role мокируются).
+"""
+
+from __future__ import annotations
+
+import os
+import sys
+from unittest.mock import MagicMock
+
+# psycopg v3 driver required; stub DATABASE_URL before any app import
+os.environ.setdefault("DATABASE_URL", "postgresql+psycopg://test:test@localhost:5432/test")
+
+# WeasyPrint requires GTK — not present in CI/Windows. Stub before any app import.
+_wp_mock = MagicMock()
+sys.modules.setdefault("weasyprint", _wp_mock)
+sys.modules.setdefault("weasyprint.CSS", _wp_mock)
+sys.modules.setdefault("weasyprint.HTML", _wp_mock)
+
+import pytest  # noqa: E402
+from fastapi import FastAPI  # noqa: E402
+from fastapi.testclient import TestClient  # noqa: E402
+
+
+@pytest.fixture()
+def trade_in_app() -> FastAPI:
+    """Minimal FastAPI app mounting only the trade-in router."""
+    from app.api.v1 import trade_in as trade_in_module
+
+    application = FastAPI()
+    application.include_router(trade_in_module.router, prefix="/api/v1/trade-in")
+    return application
+
+
+def _make_db_mock(rows: list[dict]) -> MagicMock:
+    """DB session mock returning *rows* from .mappings().all()."""
+    db = MagicMock()
+    mapping_result = MagicMock()
+    mapping_result.all.return_value = rows
+    execute_result = MagicMock()
+    execute_result.mappings.return_value = mapping_result
+    db.execute.return_value = execute_result
+    return db
+
+
+def _captured_sql_and_params(db_mock: MagicMock) -> tuple[str, dict]:
+    """Extract the SQL text + params dict from the single db.execute call."""
+    args, _ = db_mock.execute.call_args
+    sql = str(args[0])
+    params = args[1]
+    return sql, params
+
+
+def _client_with(app: FastAPI, db_mock: MagicMock, role: str) -> TestClient:
+    """Override get_db with *db_mock* and patch get_role to return *role*."""
+    from app.api.v1 import trade_in as trade_in_module
+    from app.core.db import get_db
+
+    def _override_db():
+        yield db_mock
+
+    app.dependency_overrides[get_db] = _override_db
+    # estimate_history imports get_role from app.core.auth at call time → patch source.
+    trade_in_module_auth = sys.modules["app.core.auth"]
+    trade_in_module_auth.get_role = lambda _u: role  # type: ignore[assignment]
+    _ = trade_in_module  # keep router import side-effect explicit
+    return TestClient(app)
+
+
+def test_history_requires_authenticated_user(trade_in_app: FastAPI) -> None:
+    """No X-Authenticated-User header → 401 (mirror /me)."""
+    db_mock = _make_db_mock([])
+    from app.core.db import get_db
+
+    def _override_db():
+        yield db_mock
+
+    trade_in_app.dependency_overrides[get_db] = _override_db
+    client = TestClient(trade_in_app)
+    resp = client.get("/api/v1/trade-in/history")
+    assert resp.status_code == 401
+    # DB must not be touched when unauthenticated.
+    db_mock.execute.assert_not_called()
+
+
+def test_non_admin_sees_only_own_rows(trade_in_app: FastAPI) -> None:
+    """Pilot user → WHERE created_by = :owner with owner == header value."""
+    db_mock = _make_db_mock([{"id": "1", "address": "A", "rooms": 2,
+                             "area_m2": 50, "median_price": 5_000_000,
+                             "confidence": "medium", "n_analogs": 7,
+                             "created_at": "2026-05-29T00:00:00"}])
+    client = _client_with(trade_in_app, db_mock, role="pilot")
+    resp = client.get(
+        "/api/v1/trade-in/history",
+        headers={"X-Authenticated-User": "kopylov"},
+    )
+    assert resp.status_code == 200
+    sql, params = _captured_sql_and_params(db_mock)
+    assert "created_by = :owner" in sql
+    assert params["owner"] == "kopylov"
+    assert "account" not in params
+
+
+def test_non_admin_account_param_is_ignored(trade_in_app: FastAPI) -> None:
+    """Pilot passing ?account=victim must still be scoped to themselves."""
+    db_mock = _make_db_mock([])
+    client = _client_with(trade_in_app, db_mock, role="pilot")
+    resp = client.get(
+        "/api/v1/trade-in/history",
+        params={"account": "victim"},
+        headers={"X-Authenticated-User": "kopylov"},
+    )
+    assert resp.status_code == 200
+    sql, params = _captured_sql_and_params(db_mock)
+    assert "created_by = :owner" in sql
+    assert params["owner"] == "kopylov"
+    # The attacker-supplied account must NOT reach the query.
+    assert "account" not in params
+    assert "victim" not in params.values()
+
+
+def test_admin_sees_all_rows(trade_in_app: FastAPI) -> None:
+    """Admin without ?account → no WHERE filter (all rows)."""
+    db_mock = _make_db_mock([])
+    client = _client_with(trade_in_app, db_mock, role="admin")
+    resp = client.get(
+        "/api/v1/trade-in/history",
+        headers={"X-Authenticated-User": "admin"},
+    )
+    assert resp.status_code == 200
+    sql, params = _captured_sql_and_params(db_mock)
+    assert "WHERE" not in sql.upper()
+    assert "owner" not in params
+    assert "account" not in params
+
+
+def test_admin_filters_by_account(trade_in_app: FastAPI) -> None:
+    """Admin with ?account=kopylov → WHERE created_by = :account."""
+    db_mock = _make_db_mock([])
+    client = _client_with(trade_in_app, db_mock, role="admin")
+    resp = client.get(
+        "/api/v1/trade-in/history",
+        params={"account": "kopylov"},
+        headers={"X-Authenticated-User": "admin"},
+    )
+    assert resp.status_code == 200
+    sql, params = _captured_sql_and_params(db_mock)
+    assert "created_by = :account" in sql
+    assert params["account"] == "kopylov"
+    assert "owner" not in params