diff --git a/tradein-mvp/backend/app/api/v1/trade_in.py b/tradein-mvp/backend/app/api/v1/trade_in.py index 88b8387f..8e814002 100644 --- a/tradein-mvp/backend/app/api/v1/trade_in.py +++ b/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( diff --git a/tradein-mvp/backend/tests/test_estimate_idor.py b/tradein-mvp/backend/tests/test_estimate_idor.py index 0e51c2d0..26f316ac 100644 --- a/tradein-mvp/backend/tests/test_estimate_idor.py +++ b/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