From 5c559ed0e3e3286f481ba1d5faee2d922377c1b8 Mon Sep 17 00:00:00 2001 From: bot-backend Date: Sat, 30 May 2026 19:21:31 +0300 Subject: [PATCH] =?UTF-8?q?fix(tradein):=20SSRF=20host-allowlist=20=D0=BD?= =?UTF-8?q?=D0=B0=20admin=20scrape=20+=20=D1=83=D0=B4=D0=B0=D0=BB=D1=91?= =?UTF-8?q?=D0=BD=20dead=20ADMIN=5FTOKEN=20(Refs=20#756)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit admin scrape-эндпоинты фетчили произвольный *_url без host-проверки (SSRF defense-in-depth поверх RBAC). _assert_allowed_url(url): абсолютный URL с не-allowlist хостом → 400; относительный путь → ok (хост подставляется источником). Guard в 5 хендлерах (avito_house/avito_detail/yandex_detail/ cian_detail/cian_newbuilding). scrape_allowed_hosts в config (сверено со scrapers/*.py — добавлены ekb.cian.ru, ekaterinburg.n1.ru). ADMIN_TOKEN (0 чтений в коде) убран из docker-compose.prod.yml + DEPLOY.md. main.py RBAC не тронут. 11 тестов pass, ruff clean. --- tradein-mvp/DEPLOY.md | 3 +- tradein-mvp/backend/app/api/v1/admin.py | 17 +++ tradein-mvp/backend/app/core/config.py | 16 +++ tradein-mvp/backend/tests/test_admin_ssrf.py | 106 +++++++++++++++++++ tradein-mvp/docker-compose.prod.yml | 2 - 5 files changed, 140 insertions(+), 4 deletions(-) create mode 100644 tradein-mvp/backend/tests/test_admin_ssrf.py diff --git a/tradein-mvp/DEPLOY.md b/tradein-mvp/DEPLOY.md index f17cd93c..8883a20f 100644 --- a/tradein-mvp/DEPLOY.md +++ b/tradein-mvp/DEPLOY.md @@ -57,7 +57,7 @@ import /opt/gendesign/tradein-mvp/deploy/Caddyfile.tradein-fragment shell-скриптом deploy через `source .env.runtime` перед `compose up`. 2. `/opt/gendesign/tradein-mvp/backend/.env.runtime` — переменные внутри контейнера `tradein-backend` (читаются через `env_file:` в compose). Сюда - попадают `YANDEX_GEOCODER_API_KEY`, `ADMIN_TOKEN`, `COOKIE_ENCRYPTION_KEY` — + попадают `YANDEX_GEOCODER_API_KEY`, `COOKIE_ENCRYPTION_KEY` — всё, что нужно scripts/backfill_house_coords.py и application code внутри контейнера. @@ -81,7 +81,6 @@ COOKIE_ENCRYPTION_KEY=<64-char hex> # Может быть симлинком на ../.env.runtime если переменные совпадают: # ln -s ../.env.runtime /opt/gendesign/tradein-mvp/backend/.env.runtime YANDEX_GEOCODER_API_KEY= -ADMIN_TOKEN= COOKIE_ENCRYPTION_KEY=<64-char hex> GENDESIGN_FDW_PASSWORD= GLITCHTIP_DSN= diff --git a/tradein-mvp/backend/app/api/v1/admin.py b/tradein-mvp/backend/app/api/v1/admin.py index 825528dd..e70dd661 100644 --- a/tradein-mvp/backend/app/api/v1/admin.py +++ b/tradein-mvp/backend/app/api/v1/admin.py @@ -9,6 +9,7 @@ import json import logging import time from typing import Annotated, Any, Literal +from urllib.parse import urlparse from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException, Query from pydantic import BaseModel, Field @@ -46,6 +47,17 @@ logger = logging.getLogger(__name__) router = APIRouter() +def _assert_allowed_url(url: str) -> None: + """SSRF guard: отклоняем абсолютные URL с хостом не из allowlist (#756). + + Относительные пути (urlparse().netloc пустой) пропускаем — хост + будет подставлен фиксированным AVITO_BASE / BASE_URL в самом scraper'е. + """ + parsed = urlparse(url) + if parsed.netloc and parsed.netloc not in settings.scrape_allowed_hosts: + raise HTTPException(status_code=400, detail="host not allowed") + + _ALL_SOURCES = ["avito", "cian", "yandex", "n1"] @@ -453,6 +465,7 @@ async def scrape_avito_house( Flow: fetch_house_catalog → save_house_catalog_enrichment. Returns counters {'house_id', 'reviews', 'sellers', 'listings_linked', 'placement_history'}. """ + _assert_allowed_url(house_url) try: enrichment = await fetch_house_catalog(house_url) except Exception as e: @@ -481,6 +494,7 @@ async def scrape_avito_detail( Flow: fetch_detail → save_detail_enrichment (UPDATE listings WHERE source='avito'). Returns {'ok', 'item_id', 'updated'}. """ + _assert_allowed_url(item_url) try: enrichment = await fetch_detail(item_url) except Exception as e: @@ -929,6 +943,7 @@ async def scrape_yandex_detail( Returns a snapshot of the extracted DetailEnrichment (no DB write - read-only debug; main pipeline writes via estimator on /estimate flow). """ + _assert_allowed_url(offer_url) async with YandexDetailScraper() as scraper: result = await scraper.fetch_detail(offer_url) if result is None: @@ -1047,6 +1062,7 @@ async def scrape_cian_detail( If `listing_id` provided → save enrichment (offer_price_history + listings updates). Without it → debug-only (no DB write). """ + _assert_allowed_url(offer_url) from app.services.scrapers.cian_detail import fetch_detail, save_detail_enrichment enrichment = await fetch_detail(offer_url) @@ -1090,6 +1106,7 @@ async def scrape_cian_newbuilding( If `house_id` provided → persist (houses + houses_price_dynamics + management_companies). Without it → debug-only (no DB write). """ + _assert_allowed_url(zhk_url) from app.services.scrapers.cian_newbuilding import ( fetch_newbuilding, save_newbuilding_enrichment, diff --git a/tradein-mvp/backend/app/core/config.py b/tradein-mvp/backend/app/core/config.py index 77055354..d9327655 100644 --- a/tradein-mvp/backend/app/core/config.py +++ b/tradein-mvp/backend/app/core/config.py @@ -108,5 +108,21 @@ class Settings(BaseSettings): # Конфигурируется через env ESTIMATE_QUOTA_LIMIT. Default 15. estimate_quota_limit: int = 15 + # SSRF-защита для admin scrape endpoints (#756). + # Список хостов которым разрешено передавать абсолютные URL в параметрах *_url. + # Относительные пути (без netloc) проходят без проверки — хост подставляется + # фиксированным в самом scraper'е. Задаётся через env SCRAPE_ALLOWED_HOSTS + # (comma-separated). По умолчанию — все хосты используемые scrapers/*.py. + scrape_allowed_hosts: set[str] = { + "www.avito.ru", + "avito.ru", + "www.cian.ru", + "cian.ru", + "ekb.cian.ru", + "realty.yandex.ru", + "ekaterinburg.n1.ru", + "n1.ru", + } + settings = Settings() diff --git a/tradein-mvp/backend/tests/test_admin_ssrf.py b/tradein-mvp/backend/tests/test_admin_ssrf.py new file mode 100644 index 00000000..4de4089c --- /dev/null +++ b/tradein-mvp/backend/tests/test_admin_ssrf.py @@ -0,0 +1,106 @@ +"""Unit tests for SSRF allowlist guard (_assert_allowed_url) — issue #756. + +Approach: unit-tests on the helper directly (no DB/scraper fixtures needed). +The guard is a pure synchronous function; TestClient integration is not required +because the guard raises before any async scraper is called. +""" + +from __future__ import annotations + +import os +import sys +from unittest.mock import MagicMock + +os.environ.setdefault("DATABASE_URL", "postgresql+psycopg://test:test@localhost:5432/test") + +_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 HTTPException # noqa: E402 + +from app.api.v1.admin import _assert_allowed_url # noqa: E402 +from app.core.config import settings # noqa: E402 + +# --------------------------------------------------------------------------- +# Absolute URL — host NOT in allowlist → 400 +# --------------------------------------------------------------------------- + + +def test_absolute_ssrf_metadata_endpoint_blocked() -> None: + """http://169.254.169.254/... (AWS metadata) должен быть отклонён с 400.""" + with pytest.raises(HTTPException) as exc_info: + _assert_allowed_url("http://169.254.169.254/latest/meta-data/") + assert exc_info.value.status_code == 400 + assert "host not allowed" in exc_info.value.detail + + +def test_absolute_localhost_blocked() -> None: + with pytest.raises(HTTPException) as exc_info: + _assert_allowed_url("http://localhost/internal/admin") + assert exc_info.value.status_code == 400 + + +def test_absolute_arbitrary_host_blocked() -> None: + with pytest.raises(HTTPException) as exc_info: + _assert_allowed_url("https://evil.example.com/scrape") + assert exc_info.value.status_code == 400 + + +# --------------------------------------------------------------------------- +# Absolute URL — host IN allowlist → passes +# --------------------------------------------------------------------------- + + +def test_absolute_avito_allowed() -> None: + _assert_allowed_url("https://www.avito.ru/catalog/houses/some-slug/12345") + + +def test_absolute_cian_allowed() -> None: + _assert_allowed_url("https://ekb.cian.ru/sale/flat/327830237/") + + +def test_absolute_yandex_realty_allowed() -> None: + _assert_allowed_url("https://realty.yandex.ru/ekaterinburg/kupit/kvartira/123/") + + +def test_absolute_n1_allowed() -> None: + _assert_allowed_url("https://ekaterinburg.n1.ru/kupit/kvartiry/vtorichka/") + + +# --------------------------------------------------------------------------- +# Relative path — no netloc → always passes +# --------------------------------------------------------------------------- + + +def test_relative_path_passes_the_guard() -> None: + """Путь без схемы/хоста (авито-стиль /ekaterinburg/...) разрешается.""" + _assert_allowed_url("/ekaterinburg/kvartiry/prodam-123") + + +def test_relative_path_house_catalog_passes() -> None: + _assert_allowed_url("/catalog/houses/ekb-slug/99999") + + +def test_empty_string_passes() -> None: + """Пустая строка — нет netloc → guard не блокирует (upstream validation задача scraper'а).""" + _assert_allowed_url("") + + +# --------------------------------------------------------------------------- +# Allowlist integrity: verify expected hosts present +# --------------------------------------------------------------------------- + + +def test_allowlist_contains_required_scraper_hosts() -> None: + required = { + "www.avito.ru", + "ekb.cian.ru", + "www.cian.ru", + "realty.yandex.ru", + "ekaterinburg.n1.ru", + } + missing = required - settings.scrape_allowed_hosts + assert not missing, f"Missing hosts in scrape_allowed_hosts: {missing}" diff --git a/tradein-mvp/docker-compose.prod.yml b/tradein-mvp/docker-compose.prod.yml index 5a6e1905..17f3c23a 100644 --- a/tradein-mvp/docker-compose.prod.yml +++ b/tradein-mvp/docker-compose.prod.yml @@ -58,8 +58,6 @@ services: # Yandex Geocoder API key — читается из backend/.env.runtime (env_file). # Раньше дублировалось здесь как ${YANDEX_GEOCODER_API_KEY:-} — убрано # т.к. environment: overrides env_file и при пустой host-env стирает значение. - # Защита /api/v1/admin/* — задаётся в .env.runtime. Пусто = открыто (dev). - ADMIN_TOKEN: "${ADMIN_TOKEN:-}" # GlitchTip DSN — мониторинг ошибок (#396). Пусто = выключено. GLITCHTIP_DSN: "${GLITCHTIP_DSN:-}" GENDESIGN_FDW_PASSWORD: "${GENDESIGN_FDW_PASSWORD:-}"