From e3f928b24de5f015e6178d21ce342986554d8c09 Mon Sep 17 00:00:00 2001 From: lekss361 Date: Fri, 29 May 2026 18:41:54 +0300 Subject: [PATCH] fix(tradein): scope GET /history to caller, close cross-pilot data leak (#656) GET /history SELECTed latest estimates of ALL users -> cross-pilot leak. Adds trade_in_estimates.created_by (migration 083), populated from X-Authenticated-User on estimate creation. /history now: non-admin forced to own rows (legacy NULL rows excluded); admin sees all or ?account=; 401 if header missing, 403 if user not in roles.yaml. _empty_estimate rows also tagged. tests/test_history_scope.py (5 cases) + estimator regression green. Closes #656 --- tradein-mvp/backend/app/api/v1/trade_in.py | 46 +++++- tradein-mvp/backend/app/services/estimator.py | 14 +- .../sql/083_trade_in_estimates_created_by.sql | 34 ++++ .../backend/tests/test_history_scope.py | 155 ++++++++++++++++++ 4 files changed, 242 insertions(+), 7 deletions(-) create mode 100644 tradein-mvp/backend/data/sql/083_trade_in_estimates_created_by.sql create mode 100644 tradein-mvp/backend/tests/test_history_scope.py diff --git a/tradein-mvp/backend/app/api/v1/trade_in.py b/tradein-mvp/backend/app/api/v1/trade_in.py index dd9928f6..4a8fd865 100644 --- a/tradein-mvp/backend/app/api/v1/trade_in.py +++ b/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=. + 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] diff --git a/tradein-mvp/backend/app/services/estimator.py b/tradein-mvp/backend/app/services/estimator.py index 3fc96d3e..1f9fae3f 100644 --- a/tradein-mvp/backend/app/services/estimator.py +++ b/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, }, ) diff --git a/tradein-mvp/backend/data/sql/083_trade_in_estimates_created_by.sql b/tradein-mvp/backend/data/sql/083_trade_in_estimates_created_by.sql new file mode 100644 index 00000000..b21616b0 --- /dev/null +++ b/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= фильтр). 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; diff --git a/tradein-mvp/backend/tests/test_history_scope.py b/tradein-mvp/backend/tests/test_history_scope.py new file mode 100644 index 00000000..07469290 --- /dev/null +++ b/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