fix(tradein): extend IDOR guard to all estimate_id routes (#690 review)

Ревью PR #702: #690 закрыл только GET /estimate/{id} + /pdf, но IDOR того же
класса оставался в 10 derived-роутах. Критично — photo-эндпоинты отдают сырые
байты фото интерьера квартиры, enumerable по UUID оценки.

Добавлен _assert_estimate_access_by_id() (тянет created_by по estimate_id →
_assert_estimate_access). Guard навешан на: GET/POST /photos,
/photos/{photo_id} (write+read PII), /houses, /placement-history,
/house-analytics, /cian-price-changes, /sell-time-sensitivity, /imv-benchmark.
upload_photo: existence SELECT 1 → SELECT created_by (без лишнего запроса).

Тесты: photo owner/other/admin/no-header (200/404/200/401), list_photos,
upload write-side 404, derived-роут houses+imv, + закрыты gap'ы ревью
(legacy NULL created_by → 404, unknown-user → 403 на /pdf). 27 passed.

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

@@ -74,6 +74,23 @@ def _assert_estimate_access(created_by: str | None, x_authenticated_user: str |
         raise HTTPException(status_code=404, detail="estimate not found")
 
 
+def _assert_estimate_access_by_id(
+    db: Session, estimate_id: UUID, x_authenticated_user: str | None
+) -> None:
+    """IDOR guard для derived-роутов, не читающих саму оценку (#690).
+
+    Тянет created_by оценки и применяет _assert_estimate_access. 404 если оценки
+    нет (как и owner-mismatch — существование не подтверждаем).
+    """
+    row = db.execute(
+        text("SELECT created_by FROM trade_in_estimates WHERE id = CAST(:id AS uuid)"),
+        {"id": str(estimate_id)},
+    ).fetchone()
+    if row is None:
+        raise HTTPException(status_code=404, detail="estimate not found")
+    _assert_estimate_access(row.created_by, x_authenticated_user)
+
+
 @router.post("/estimate", response_model=AggregatedEstimate)
 async def estimate(
     payload: TradeInEstimateInput,
@@ -339,14 +356,16 @@ async def upload_photo(
     estimate_id: UUID,
     db: Annotated[Session, Depends(get_db)],
     file: Annotated[UploadFile, File()],
+    x_authenticated_user: Annotated[str | None, Header(alias="X-Authenticated-User")] = None,
 ) -> PhotoMeta:
     """Загрузить фото квартиры к оценке (#394). Хранение в estimate_photos (bytea)."""
     estimate = db.execute(
-        text("SELECT 1 FROM trade_in_estimates WHERE id = CAST(:id AS uuid)"),
+        text("SELECT created_by FROM trade_in_estimates WHERE id = CAST(:id AS uuid)"),
         {"id": str(estimate_id)},
     ).fetchone()
     if estimate is None:
         raise HTTPException(status_code=404, detail="estimate not found")
+    _assert_estimate_access(estimate.created_by, x_authenticated_user)
 
     ctype = (file.content_type or "").lower()
     if ctype not in _ALLOWED_IMAGE_TYPES:
@@ -413,8 +432,10 @@ async def upload_photo(
 def list_photos(
     estimate_id: UUID,
     db: Annotated[Session, Depends(get_db)],
+    x_authenticated_user: Annotated[str | None, Header(alias="X-Authenticated-User")] = None,
 ) -> list[PhotoMeta]:
     """Список фото оценки — метаданные, без содержимого (#394)."""
+    _assert_estimate_access_by_id(db, estimate_id, x_authenticated_user)
     rows = (
         db.execute(
             text(
@@ -438,8 +459,10 @@ def get_photo(
     estimate_id: UUID,
     photo_id: UUID,
     db: Annotated[Session, Depends(get_db)],
+    x_authenticated_user: Annotated[str | None, Header(alias="X-Authenticated-User")] = None,
 ) -> Response:
     """Отдать содержимое фото — image bytes (#394)."""
+    _assert_estimate_access_by_id(db, estimate_id, x_authenticated_user)
     row = db.execute(
         text(
             """
@@ -566,6 +589,7 @@ _HOUSE_SELECT_COLS = """
 def get_estimate_houses(
     estimate_id: UUID,
     db: Annotated[Session, Depends(get_db)],
+    x_authenticated_user: Annotated[str | None, Header(alias="X-Authenticated-User")] = None,
 ) -> list[HouseInfoForEstimate]:
     """House(s) информация для estimate.
 
@@ -576,6 +600,7 @@ def get_estimate_houses(
     Возвращаем прямой матч + nearby (dedup by id), up to ~6 домов.
     Пустой список если нет matches.
     """
+    _assert_estimate_access_by_id(db, estimate_id, x_authenticated_user)
     target = db.execute(
         text(
             """
@@ -653,12 +678,14 @@ def get_estimate_houses(
 def get_estimate_placement_history(
     estimate_id: UUID,
     db: Annotated[Session, Depends(get_db)],
+    x_authenticated_user: Annotated[str | None, Header(alias="X-Authenticated-User")] = None,
 ) -> list[PlacementHistoryEntry]:
     """Историческая продажная активность по дому(ам) target estimate.
 
     Возвращает rows из house_placement_history для всех houses связанных с
     target адресом. Сортировано по last_price_date DESC.
     """
+    _assert_estimate_access_by_id(db, estimate_id, x_authenticated_user)
     target = db.execute(
         text(
             """
@@ -735,12 +762,14 @@ def get_estimate_placement_history(
 def get_estimate_house_analytics(
     estimate_id: UUID,
     db: Annotated[Session, Depends(get_db)],
+    x_authenticated_user: Annotated[str | None, Header(alias="X-Authenticated-User")] = None,
 ) -> HouseAnalyticsResponse:
     """House-level analytics from house_placement_history backfill.
 
     Resolves target house(s) — если в самом доме <8 hist rows — расширяем поиск до 300м.
     Возвращает: price-history by year (median ₽/м²), recent sold (12mo), KPI.
     """
+    _assert_estimate_access_by_id(db, estimate_id, x_authenticated_user)
     target = db.execute(
         text("SELECT lat, lon, address FROM trade_in_estimates WHERE id = CAST(:id AS uuid)"),
         {"id": str(estimate_id)},
@@ -924,6 +953,7 @@ def get_estimate_house_analytics(
 def get_estimate_cian_price_changes(
     estimate_id: UUID,
     db: Annotated[Session, Depends(get_db)],
+    x_authenticated_user: Annotated[str | None, Header(alias="X-Authenticated-User")] = None,
 ) -> list[CianPriceChangeStats]:
     """История изменений цены для Cian-аналогов из estimate.
 
@@ -936,6 +966,7 @@ def get_estimate_cian_price_changes(
     Возвращает только аналоги с хотя бы одним изменением цены.
     Пустой список если нет cian-аналогов или нет истории.
     """
+    _assert_estimate_access_by_id(db, estimate_id, x_authenticated_user)
     rows = (
         db.execute(
             text(
@@ -1013,6 +1044,7 @@ def get_estimate_cian_price_changes(
 def get_estimate_sell_time_sensitivity(
     estimate_id: UUID,
     db: Annotated[Session, Depends(get_db)],
+    x_authenticated_user: Annotated[str | None, Header(alias="X-Authenticated-User")] = None,
 ) -> SellTimeSensitivityResponse:
     """Срок продажи в зависимости от цены к медиане дома/района.
 
@@ -1020,6 +1052,7 @@ def get_estimate_sell_time_sensitivity(
     Filter last_price > start_price * 0.7 — отбрасываем подозрительно
     заниженные лоты (выбросы, ошибки парсинга).
     """
+    _assert_estimate_access_by_id(db, estimate_id, x_authenticated_user)
     # 1. Resolve house_ids (same logic as house-analytics)
     target = db.execute(
         text("SELECT lat, lon, address FROM trade_in_estimates WHERE id = CAST(:id AS uuid)"),
@@ -1195,6 +1228,7 @@ def get_estimate_sell_time_sensitivity(
 def get_estimate_imv_benchmark(
     estimate_id: UUID,
     db: Annotated[Session, Depends(get_db)],
+    x_authenticated_user: Annotated[str | None, Header(alias="X-Authenticated-User")] = None,
 ) -> IMVBenchmarkResponse:
     """Avito IMV benchmark для estimate (для UI badge «наша 6.4М · Avito 6.29М»).
 
@@ -1202,6 +1236,7 @@ def get_estimate_imv_benchmark(
       1. avito_imv_evaluations WHERE estimate_id = :id (если linked в estimator)
       2. Если не linked — fallback: most recent IMV для same address (TTL 24h)
     """
+    _assert_estimate_access_by_id(db, estimate_id, x_authenticated_user)
     # Сначала пытаемся найти directly linked
     row = db.execute(
         text(

tradein-mvp/backend/tests/test_estimate_idor.py

@@ -254,3 +254,223 @@ def test_pdf_requires_authenticated_user(trade_in_app: FastAPI) -> None:
     client = _client_with(trade_in_app, db_mock, role="pilot")
     resp = client.get(f"/api/v1/trade-in/estimate/{_ESTIMATE_ID}/pdf")
     assert resp.status_code == 401
+
+
+def test_pdf_unknown_user_gets_403(trade_in_app: FastAPI) -> None:
+    """/pdf: authenticated via Caddy but missing from roles.yaml → 403 (reviewer gap)."""
+    db_mock = _make_db_mock(_make_estimate_row(created_by="kopylov"))
+    client = _client_with(trade_in_app, db_mock, role=None)
+    resp = client.get(
+        f"/api/v1/trade-in/estimate/{_ESTIMATE_ID}/pdf",
+        headers={"X-Authenticated-User": "ghost"},
+    )
+    assert resp.status_code == 403
+
+
+# ── Reviewer-named gap: legacy NULL created_by ───────────────────────────────
+
+
+def test_get_estimate_legacy_null_owner_non_admin_gets_404(trade_in_app: FastAPI) -> None:
+    """Legacy estimate with NULL created_by: non-admin pilot must NOT read it → 404."""
+    db_mock = _make_db_mock(_make_estimate_row(created_by=None))
+    client = _client_with(trade_in_app, db_mock, role="pilot")
+    resp = client.get(
+        f"/api/v1/trade-in/estimate/{_ESTIMATE_ID}",
+        headers={"X-Authenticated-User": "kopylov"},
+    )
+    assert resp.status_code == 404
+
+
+# ── Photo endpoints (#690 — raw apartment-interior bytes are enumerable PII) ──
+
+
+def _make_photo_row() -> SimpleNamespace:
+    """A minimal estimate_photos row for get_photo (content + content_type)."""
+    return SimpleNamespace(content=b"\x89PNG-fake", content_type="image/png")
+
+
+def _make_db_mock_seq(*rows: SimpleNamespace | None) -> MagicMock:
+    """DB session mock whose successive .execute(...).fetchone() yield *rows* in order.
+
+    Derived/photo routes run the guard SELECT (created_by) first, then their own
+    queries; configure side_effect so the guard row comes first.
+    """
+    db = MagicMock()
+
+    def _execute(*_a, **_k):
+        result = MagicMock()
+        result.fetchone.return_value = next(_iter)
+        return result
+
+    _iter = iter(rows)
+    db.execute.side_effect = _execute
+    return db
+
+
+def test_get_photo_owner_can_read(trade_in_app: FastAPI) -> None:
+    """Owner pilot reads photo bytes → 200."""
+    db_mock = _make_db_mock_seq(
+        SimpleNamespace(created_by="kopylov"),  # guard query
+        _make_photo_row(),  # route query
+    )
+    client = _client_with(trade_in_app, db_mock, role="pilot")
+    resp = client.get(
+        f"/api/v1/trade-in/estimate/{_ESTIMATE_ID}/photos/{_ESTIMATE_ID}",
+        headers={"X-Authenticated-User": "kopylov"},
+    )
+    assert resp.status_code == 200
+    assert resp.content == b"\x89PNG-fake"
+
+
+def test_get_photo_other_pilot_gets_404(trade_in_app: FastAPI) -> None:
+    """Non-owner pilot must NOT read someone else's photo → 404 (guard raises first)."""
+    db_mock = _make_db_mock_seq(SimpleNamespace(created_by="victim"))
+    client = _client_with(trade_in_app, db_mock, role="pilot")
+    resp = client.get(
+        f"/api/v1/trade-in/estimate/{_ESTIMATE_ID}/photos/{_ESTIMATE_ID}",
+        headers={"X-Authenticated-User": "attacker"},
+    )
+    assert resp.status_code == 404
+
+
+def test_get_photo_admin_can_read_any(trade_in_app: FastAPI) -> None:
+    """Admin reads any photo → 200."""
+    db_mock = _make_db_mock_seq(
+        SimpleNamespace(created_by="someone_else"),
+        _make_photo_row(),
+    )
+    client = _client_with(trade_in_app, db_mock, role="admin")
+    resp = client.get(
+        f"/api/v1/trade-in/estimate/{_ESTIMATE_ID}/photos/{_ESTIMATE_ID}",
+        headers={"X-Authenticated-User": "admin"},
+    )
+    assert resp.status_code == 200
+
+
+def test_get_photo_requires_authenticated_user(trade_in_app: FastAPI) -> None:
+    """get_photo: no X-Authenticated-User header → 401."""
+    db_mock = _make_db_mock_seq(SimpleNamespace(created_by="kopylov"))
+    client = _client_with(trade_in_app, db_mock, role="pilot")
+    resp = client.get(
+        f"/api/v1/trade-in/estimate/{_ESTIMATE_ID}/photos/{_ESTIMATE_ID}",
+    )
+    assert resp.status_code == 401
+
+
+def test_list_photos_owner_can_read(trade_in_app: FastAPI) -> None:
+    """Owner pilot lists photos → 200 (empty list ok)."""
+    db = MagicMock()
+    guard_result = MagicMock()
+    guard_result.fetchone.return_value = SimpleNamespace(created_by="kopylov")
+    list_result = MagicMock()
+    list_result.mappings.return_value.all.return_value = []
+    db.execute.side_effect = [guard_result, list_result]
+    client = _client_with(trade_in_app, db, role="pilot")
+    resp = client.get(
+        f"/api/v1/trade-in/estimate/{_ESTIMATE_ID}/photos",
+        headers={"X-Authenticated-User": "kopylov"},
+    )
+    assert resp.status_code == 200
+    assert resp.json() == []
+
+
+def test_list_photos_other_pilot_gets_404(trade_in_app: FastAPI) -> None:
+    """Non-owner pilot must NOT list someone else's photos → 404."""
+    db_mock = _make_db_mock_seq(SimpleNamespace(created_by="victim"))
+    client = _client_with(trade_in_app, db_mock, role="pilot")
+    resp = client.get(
+        f"/api/v1/trade-in/estimate/{_ESTIMATE_ID}/photos",
+        headers={"X-Authenticated-User": "attacker"},
+    )
+    assert resp.status_code == 404
+
+
+def test_upload_photo_other_pilot_gets_404(trade_in_app: FastAPI) -> None:
+    """Write-side IDOR: non-owner pilot must NOT upload to someone else's estimate → 404.
+
+    Guard runs on the existence SELECT (created_by) before any write — so a single
+    fetchone returning created_by='admin' + a non-owner header is enough.
+    """
+    db_mock = _make_db_mock_seq(SimpleNamespace(created_by="admin"))
+    client = _client_with(trade_in_app, db_mock, role="pilot")
+    resp = client.post(
+        f"/api/v1/trade-in/estimate/{_ESTIMATE_ID}/photos",
+        headers={"X-Authenticated-User": "attacker"},
+        files={"file": ("x.png", b"\x89PNG-fake", "image/png")},
+    )
+    assert resp.status_code == 404
+
+
+def test_upload_photo_owner_can_upload(trade_in_app: FastAPI, monkeypatch) -> None:
+    """Owner pilot uploads → 200. Stub sanitize_image + the INSERT mapping row."""
+    from app.api.v1 import trade_in as trade_in_module
+
+    monkeypatch.setattr(
+        trade_in_module, "sanitize_image", lambda _c: (b"\x89PNG-clean", "image/png")
+    )
+
+    from datetime import UTC, datetime
+
+    db = MagicMock()
+    guard_result = MagicMock()
+    guard_result.fetchone.return_value = SimpleNamespace(created_by="kopylov")
+    count_result = MagicMock()
+    count_result.scalar_one.return_value = 0
+    insert_result = MagicMock()
+    insert_result.mappings.return_value.fetchone.return_value = {
+        "id": _ESTIMATE_ID,
+        "filename": "x.png",
+        "content_type": "image/png",
+        "size_bytes": 9,
+        "uploaded_at": datetime.now(tz=UTC),
+    }
+    db.execute.side_effect = [guard_result, count_result, insert_result]
+    client = _client_with(trade_in_app, db, role="pilot")
+    resp = client.post(
+        f"/api/v1/trade-in/estimate/{_ESTIMATE_ID}/photos",
+        headers={"X-Authenticated-User": "kopylov"},
+        files={"file": ("x.png", b"\x89PNG-fake", "image/png")},
+    )
+    assert resp.status_code == 200
+
+
+# ── Derived analytics routes (representative: /houses, /imv-benchmark) ────────
+
+
+def test_get_estimate_houses_other_pilot_gets_404(trade_in_app: FastAPI) -> None:
+    """Derived route: non-owner pilot blocked by guard → 404 (before any house query)."""
+    db_mock = _make_db_mock_seq(SimpleNamespace(created_by="victim"))
+    client = _client_with(trade_in_app, db_mock, role="pilot")
+    resp = client.get(
+        f"/api/v1/trade-in/estimate/{_ESTIMATE_ID}/houses",
+        headers={"X-Authenticated-User": "attacker"},
+    )
+    assert resp.status_code == 404
+
+
+def test_get_estimate_houses_owner_can_read(trade_in_app: FastAPI) -> None:
+    """Derived route: owner pilot passes guard → 200. Stub target with no address/geo."""
+    db = MagicMock()
+    guard_result = MagicMock()
+    guard_result.fetchone.return_value = SimpleNamespace(created_by="kopylov")
+    target_result = MagicMock()
+    target_result.fetchone.return_value = SimpleNamespace(lat=None, lon=None, address=None)
+    db.execute.side_effect = [guard_result, target_result]
+    client = _client_with(trade_in_app, db, role="pilot")
+    resp = client.get(
+        f"/api/v1/trade-in/estimate/{_ESTIMATE_ID}/houses",
+        headers={"X-Authenticated-User": "kopylov"},
+    )
+    assert resp.status_code == 200
+    assert resp.json() == []
+
+
+def test_get_estimate_imv_benchmark_other_pilot_gets_404(trade_in_app: FastAPI) -> None:
+    """imv-benchmark: non-owner pilot blocked by guard → 404."""
+    db_mock = _make_db_mock_seq(SimpleNamespace(created_by="victim"))
+    client = _client_with(trade_in_app, db_mock, role="pilot")
+    resp = client.get(
+        f"/api/v1/trade-in/estimate/{_ESTIMATE_ID}/imv-benchmark",
+        headers={"X-Authenticated-User": "attacker"},
+    )
+    assert resp.status_code == 404