From 35f5c3426b9a498dcaf12c2c6e821ca055b425aa Mon Sep 17 00:00:00 2001 From: lekss361 Date: Sun, 26 Jul 2026 22:33:54 +0000 Subject: [PATCH 1/6] =?UTF-8?q?fix(tradein/scrapers):=20=D0=B4=D0=B5=D1=82?= =?UTF-8?q?=D0=B5=D0=BA=D1=86=D0=B8=D1=8F=20=D0=B4=D1=80=D0=B5=D0=B9=D1=84?= =?UTF-8?q?=D0=B0=20=D1=80=D0=B0=D0=B7=D0=BC=D0=B5=D1=82=D0=BA=D0=B8=20?= =?UTF-8?q?=D0=B2=D0=BC=D0=B5=D1=81=D1=82=D0=BE=20=D1=82=D0=B8=D1=85=D0=BE?= =?UTF-8?q?=D0=B9=20=D0=BF=D1=83=D1=81=D1=82=D0=BE=D1=82=D1=8B=20(#2535)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../backend/app/services/cian_session.py | 72 ++++-- ...avito_detail_publish_date_year_rollover.py | 94 ++++++++ .../tests/test_avito_sweep_dom_drift.py | 206 ++++++++++++++++++ .../test_cian_serp_offers_total_mismatch.py | 106 +++++++++ .../backend/tests/test_cian_session.py | 99 ++++++++- .../src/scraper_kit/providers/avito/detail.py | 16 +- .../src/scraper_kit/providers/avito/serp.py | 74 +++++++ .../src/scraper_kit/providers/cian/serp.py | 72 ++++-- 8 files changed, 695 insertions(+), 44 deletions(-) create mode 100644 tradein-mvp/backend/tests/test_avito_detail_publish_date_year_rollover.py create mode 100644 tradein-mvp/backend/tests/test_avito_sweep_dom_drift.py create mode 100644 tradein-mvp/backend/tests/test_cian_serp_offers_total_mismatch.py diff --git a/tradein-mvp/backend/app/services/cian_session.py b/tradein-mvp/backend/app/services/cian_session.py index 56d83a16..d6d68bf2 100644 --- a/tradein-mvp/backend/app/services/cian_session.py +++ b/tradein-mvp/backend/app/services/cian_session.py @@ -71,6 +71,24 @@ _MFE_AUTH = "header-frontend" # Callers that need to distinguish ban from valid auth should check state.get("_ban"). VERIFY_BAN_SENTINEL: dict[str, Any] = {"_ban": True} +# audit-scrapers finding 4: verify_session раньше сводило 5xx / сетевой сбой / +# смену вёрстки к тому же None, что и реальный логаут (401 / isAuthenticated=false) — +# вызывающие (_cian_pre_claim, admin upload/auto-login) реагировали "куки протухли, +# перезалей" там, где куки ни при чём (Cian недоступен ИЛИ scraper_kit.cian_state_parser +# больше не находит header-frontend initialState). Два отдельных сигнала ниже НЕ +# триггерят "cookies expired" алерт у вызывающих. + +# Cian источник недоступен прямо сейчас (5xx-ответ ИЛИ сетевой/транспортный сбой — +# timeout, DNS, connection reset). Cookies могут быть абсолютно валидны — просто +# нечем было их проверить. Retry позже, БЕЗ пометки session invalid. +VERIFY_SOURCE_UNAVAILABLE_SENTINEL: dict[str, Any] = {"_source_unavailable": True} + +# HTTP 200 получен, но ожидаемый auth-state (header-frontend/initialState с +# user.isAuthenticated) не найден/не распарсился — Cian изменил вёрстку/MFE-схему. +# Это engineering-проблема (extract_state/_MFE_AUTH нужно обновить), НЕ протухшие +# cookies — переставлять куки здесь бесполезно. +VERIFY_MARKUP_CHANGED_SENTINEL: dict[str, Any] = {"_markup_changed": True} + def _classify_verify_response( status_code: int, @@ -79,19 +97,25 @@ def _classify_verify_response( """Pure classifier — maps (status_code, html) to verify_session outcome. Returns: - VERIFY_BAN_SENTINEL — 403/TLS ban (cookies may be fine, server is blocking) - None — 401 or isAuthenticated=false (cookies genuinely expired) - state dict — authenticated successfully + VERIFY_BAN_SENTINEL — 403/TLS ban (cookies могут быть в порядке, + блокирует сервер) + VERIFY_SOURCE_UNAVAILABLE_SENTINEL — 5xx/иной non-200 без содержимого — + источник недоступен, НЕ cookies + VERIFY_MARKUP_CHANGED_SENTINEL — HTTP 200, но auth-state не найден/не + распарсился — вёрстка/схема изменилась + None — 401 ИЛИ isAuthenticated=false — cookies + ДЕЙСТВИТЕЛЬНО протухли/разлогинены + state dict — authenticated successfully """ if status_code == 403: return VERIFY_BAN_SENTINEL if status_code == 401: return None - if html is None: - return None + if status_code != 200 or html is None: + return VERIFY_SOURCE_UNAVAILABLE_SENTINEL state = extract_state(html, mfe=_MFE_AUTH, key="initialState") if state is None: - return None + return VERIFY_MARKUP_CHANGED_SENTINEL user = state.get("user", {}) or {} if not user.get("isAuthenticated"): return None @@ -104,11 +128,18 @@ async def verify_session(cookies: dict[str, str]) -> dict[str, Any] | None: Uses curl_cffi with impersonate='chrome120' (same as prod scrapers) to avoid TLS-fingerprint bans that httpx would trigger. - Returns: - state dict — authenticated (contains user.isAuthenticated + userId) - VERIFY_BAN_SENTINEL — HTTP 403 TLS/bot ban; cookies may still be valid — - callers should NOT trigger a cookie-refresh alert - None — HTTP 401 or isAuthenticated=false; cookies expired + Returns (проверяй через `is`, НЕ `==` — это sentinel-объекты): + state dict — authenticated (user.isAuthenticated + userId) + VERIFY_BAN_SENTINEL — HTTP 403 TLS/bot ban; cookies могут быть + валидны — НЕ триггерить cookie-refresh alert + VERIFY_SOURCE_UNAVAILABLE_SENTINEL — 5xx/network/timeout; источник недоступен, + НЕ триггерить cookie-refresh alert, retry позже + VERIFY_MARKUP_CHANGED_SENTINEL — HTTP 200 но auth-state не распарсился; + Cian изменил вёрстку — НЕ cookie-проблема, + нужен engineering-фикс extract_state/_MFE_AUTH + None — HTTP 401 или isAuthenticated=false; cookies + ДЕЙСТВИТЕЛЬНО протухли — здесь и только здесь + имеет смысл просить re-upload Никогда не логирует сырые значения cookies. """ @@ -134,6 +165,18 @@ async def verify_session(cookies: dict[str, str]) -> dict[str, Any] | None: logger.warning( "Cian cookies verify: HTTP 403 TLS/bot ban — cookies NOT marked expired" ) + elif result is VERIFY_SOURCE_UNAVAILABLE_SENTINEL: + logger.warning( + "Cian cookies verify: source unavailable (status=%d) — " + "cookies NOT marked expired, retry later", + status, + ) + elif result is VERIFY_MARKUP_CHANGED_SENTINEL: + logger.error( + "Cian cookies verify: HTTP 200 but auth-state not found/parseable " + "(mfe=%s) — markup/schema changed, cookies NOT marked expired", + _MFE_AUTH, + ) elif result is None: logger.warning("Cian cookies verify: expired/unauthenticated (status=%d)", status) else: @@ -142,8 +185,11 @@ async def verify_session(cookies: dict[str, str]) -> dict[str, Any] | None: return result except Exception as exc: - logger.warning("Cian cookies verify failed: %s", exc) - return None + # Сетевой/транспортный сбой (timeout, DNS, connection reset и т.п.) — источник + # недоступен, НЕ признак протухших cookies (finding 4). Раньше здесь везде + # возвращался None, конфлируя с реальным логаутом. + logger.warning("Cian cookies verify: transport/network error — %s", exc) + return VERIFY_SOURCE_UNAVAILABLE_SENTINEL def save_session( diff --git a/tradein-mvp/backend/tests/test_avito_detail_publish_date_year_rollover.py b/tradein-mvp/backend/tests/test_avito_detail_publish_date_year_rollover.py new file mode 100644 index 00000000..987cccf0 --- /dev/null +++ b/tradein-mvp/backend/tests/test_avito_detail_publish_date_year_rollover.py @@ -0,0 +1,94 @@ +"""Audit-scrapers finding 3: Avito detail publish_date year-boundary rollover. + +Avito не показывает год для дат текущего года («20 декабря в 15:30»). Раньше +`_extract_meta` всегда брал ТЕКУЩИЙ год момента парсинга — объявлению, опубликованному +в декабре и прочитанному в январе следующего года, ставился год парсинга (будущая +дата), завышая свежесть лота. Фикс: если получившаяся дата оказалась в будущем +относительно момента парсинга — откатываем на год назад. + +Refs: audit-scrapers 2026-07-26, finding 3 (low). +""" + +from __future__ import annotations + +import os +from datetime import date as real_date + +import pytest +from selectolax.parser import HTMLParser + +os.environ.setdefault("DATABASE_URL", "postgresql+psycopg://test:test@localhost:5432/test") + +from scraper_kit.providers.avito import detail as kit_detail + + +def _freeze_today(monkeypatch: pytest.MonkeyPatch, frozen: real_date) -> None: + """Подменяет `date` в scraper_kit.providers.avito.detail так, что date.today() + детерминированно возвращает `frozen` (date — immutable C-тип, .today нельзя + monkeypatch'нуть напрямую — подменяем ссылку на класс в модуле).""" + + class _FrozenDate(real_date): + @classmethod + def today(cls) -> real_date: # type: ignore[override] + return frozen + + monkeypatch.setattr(kit_detail, "date", _FrozenDate) + + +def _tree_with_publish_text(text: str) -> HTMLParser: + html = f'
{text}
' + return HTMLParser(html) + + +def test_december_publish_date_read_in_january_rolls_back_a_year( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Объявление '20 декабря' парсится 5 января СЛЕДУЮЩЕГО года: без фикса + дата была бы 2027-12-20 (в будущем относительно today=2027-01-05) — теперь + откатывается на 2026-12-20.""" + _freeze_today(monkeypatch, real_date(2027, 1, 5)) + tree = _tree_with_publish_text("№ 4291500000 · 20 декабря в 15:30") + + publish_date, _, _ = kit_detail._extract_meta(tree) + + assert publish_date == real_date(2026, 12, 20) + + +def test_same_year_past_publish_date_not_rolled_back(monkeypatch: pytest.MonkeyPatch) -> None: + """Control: дата в прошлом (не будущем) в том же году — год НЕ откатывается.""" + _freeze_today(monkeypatch, real_date(2027, 1, 5)) + tree = _tree_with_publish_text("№ 4291500001 · 3 января в 09:00") + + publish_date, _, _ = kit_detail._extract_meta(tree) + + assert publish_date == real_date(2027, 1, 3) + + +def test_publish_date_equal_to_today_not_rolled_back(monkeypatch: pytest.MonkeyPatch) -> None: + """Control: дата ровно = today (не строго будущее) — год НЕ откатывается.""" + _freeze_today(monkeypatch, real_date(2027, 1, 5)) + tree = _tree_with_publish_text("№ 4291500002 · 5 января в 12:00") + + publish_date, _, _ = kit_detail._extract_meta(tree) + + assert publish_date == real_date(2027, 1, 5) + + +def test_mid_year_publish_date_not_rolled_back(monkeypatch: pytest.MonkeyPatch) -> None: + """Обычный случай вдали от границы года — поведение не меняется.""" + _freeze_today(monkeypatch, real_date(2027, 6, 15)) + tree = _tree_with_publish_text("№ 4291500003 · 20 марта в 10:00") + + publish_date, _, _ = kit_detail._extract_meta(tree) + + assert publish_date == real_date(2027, 3, 20) + + +def test_no_publish_date_in_text_returns_none(monkeypatch: pytest.MonkeyPatch) -> None: + """Regression guard: отсутствие даты в тексте по-прежнему даёт None (не падает).""" + _freeze_today(monkeypatch, real_date(2027, 1, 5)) + tree = _tree_with_publish_text("№ 4291500004") + + publish_date, _, _ = kit_detail._extract_meta(tree) + + assert publish_date is None diff --git a/tradein-mvp/backend/tests/test_avito_sweep_dom_drift.py b/tradein-mvp/backend/tests/test_avito_sweep_dom_drift.py new file mode 100644 index 00000000..f28ed174 --- /dev/null +++ b/tradein-mvp/backend/tests/test_avito_sweep_dom_drift.py @@ -0,0 +1,206 @@ +"""Audit-scrapers finding 1: Avito citywide/byrooms/exhaustive sweep DOM-drift detection. + +Раньше 0 карточек на page=1 (обход всего города / категории комнатности / ценового +бакета exhaustive-сбора) молча трактовалось как «объявлений действительно нет» — +неотличимо от content-block/captcha или дрейфа DOM-маркера карточки (`data-marker= +"item-*"`). Фикс переиспользует существующий механизм `AvitoContentBlockedError` +(см. `fetch_around`, #754/#779) + новый `_is_unexpected_empty_page()` — независимый +сигнал `_extract_total_count` (счётчик `page-title/count` либо no-results маркер): + + - page=1, 0 карточек, НЕТ no-results маркера/счётчика → аномалия → raise. + - page=1, 0 карточек, ЕСТЬ no-results маркер (total=0) → валидная пустая выборка. + - page>1, 0 карточек → всегда graceful end-of-pagination (не regressed). + - exhaustive leaf-бакет: probe независимо утверждал total>0, но после пагинации + всех страниц собрано 0 карточек → аномалия → raise (даже без per-page проверки + внутри _paginate_leaf_bucket, т.к. там нет break-on-empty цикла). + +Refs: audit-scrapers 2026-07-26, finding 1 (medium). +""" + +from __future__ import annotations + +import os +from unittest.mock import AsyncMock, patch + +import pytest + +os.environ.setdefault("DATABASE_URL", "postgresql+psycopg://test:test@localhost:5432/test") + +from scraper_kit.avito_exceptions import AvitoContentBlockedError +from scraper_kit.base import ScrapedLot +from scraper_kit.providers.avito.serp import ROOM_SLUGS, AvitoScraper + +from app.services.scraper_adapters import RealScraperConfig + +# HTML "успешно получен, разумного размера", но БЕЗ data-marker="item-*" карточек +# И без no-results маркера/счётчика — неотличимо от content-block/DOM-drift. +_NO_MARKER_HTML = "" + ("x" * 500) + "" + +# Валидная пустая выборка: no-results маркер присутствует (_AVITO_NO_RESULTS_MARKERS). +_NO_RESULTS_HTML = ( + "По вашему запросу ничего не найдено. Попробуйте изменить фильтры." + + ("y" * 200) + + "" +) + +# Firewall/captcha-страница (переиспользуем существующий fixture-паттерн из #754) — +# используется только для проверки, что page>1 остаётся graceful независимо от +# содержимого (проверка применяется ТОЛЬКО к page==1). +_BLOCKPAGE_HTML = "

Доступ ограничен

" + + +def _make_lot(source_id: str) -> ScrapedLot: + return ScrapedLot( + source="avito", + source_url=f"https://www.avito.ru/ekaterinburg/kvartiry/{source_id}", + source_id=source_id, + price_rub=6_000_000, + ) + + +# ── fetch_city_wide (_paginate_sweep) ──────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_citywide_page1_zero_cards_no_marker_raises() -> None: + s = AvitoScraper(RealScraperConfig()) + with patch.object(s, "_fetch_serp_html", AsyncMock(return_value=_NO_MARKER_HTML)): + with pytest.raises(AvitoContentBlockedError): + await s.fetch_city_wide(pages=5, delay_override_sec=0) + + +@pytest.mark.asyncio +async def test_citywide_page1_zero_cards_with_no_results_marker_is_valid_empty() -> None: + s = AvitoScraper(RealScraperConfig()) + with patch.object(s, "_fetch_serp_html", AsyncMock(return_value=_NO_RESULTS_HTML)): + result = await s.fetch_city_wide(pages=5, delay_override_sec=0) + assert result == [] + + +@pytest.mark.asyncio +async def test_citywide_page_gt1_zero_cards_stays_graceful() -> None: + """page=1 реально возвращает карточки (mock _parse_html) — page=2 пустой + firewall-текст без карточек НЕ должен поднимать исключение (только page==1).""" + s = AvitoScraper(RealScraperConfig()) + call_n = 0 + + async def _fetch(url: str, page: int) -> str: + return "page1" if page == 1 else _BLOCKPAGE_HTML + + def _parse(html: str, source_url_base: str) -> list[ScrapedLot]: + nonlocal call_n + call_n += 1 + return [_make_lot("A"), _make_lot("B")] if call_n == 1 else [] + + with patch.object(s, "_fetch_serp_html", AsyncMock(side_effect=_fetch)): + with patch.object(s, "_parse_html", side_effect=_parse): + with patch.object(s, "sleep_between_requests", AsyncMock(return_value=None)): + result = await s.fetch_city_wide(pages=5, delay_override_sec=0) + + assert len(result) == 2 + assert call_n == 2 # page1(2 lots) + page2(0 lots) → stop, no raise + + +# ── fetch_by_rooms ──────────────────────────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_byrooms_category_page1_zero_cards_no_marker_raises() -> None: + s = AvitoScraper(RealScraperConfig()) + with patch.object(s, "_fetch_serp_html", AsyncMock(return_value=_NO_MARKER_HTML)): + with pytest.raises(AvitoContentBlockedError): + await s.fetch_by_rooms(pages=5, delay_override_sec=0, room_slugs=ROOM_SLUGS[:1]) + + +@pytest.mark.asyncio +async def test_byrooms_category_page1_zero_cards_with_marker_is_valid_empty() -> None: + s = AvitoScraper(RealScraperConfig()) + with patch.object(s, "_fetch_serp_html", AsyncMock(return_value=_NO_RESULTS_HTML)): + result = await s.fetch_by_rooms(pages=5, delay_override_sec=0, room_slugs=ROOM_SLUGS[:1]) + assert result == [] + + +# ── _paginate_leaf_bucket (exhaustive/fetch_all_secondary) ─────────────────── + + +@pytest.mark.asyncio +async def test_leaf_bucket_expected_total_positive_but_zero_parsed_raises() -> None: + """Probe независимо утверждал total=5 (bucket не может быть легитимно пустым), + но парсинг всех страниц дал 0 карточек — DOM-drift, не пустой бакет.""" + s = AvitoScraper(RealScraperConfig()) + seen: dict[str, ScrapedLot] = {} + + with patch.object(s, "_parse_html", return_value=[]): + with pytest.raises(AvitoContentBlockedError): + await s._paginate_leaf_bucket( + room_slug="studii-ASgBAgICAUSSA8YQ", + room_label="studio", + lo=0, + hi=3_000_000, + html="probe-page-1", + max_pages=1, + seen=seen, + price_cap_per_bucket=1400, + max_pages_per_bucket=100, + concurrency=5, + secondary_only=True, + on_bucket=None, + skip_buckets=None, + expected_total=5, + ) + assert seen == {} + + +@pytest.mark.asyncio +async def test_leaf_bucket_expected_total_none_zero_parsed_no_raise() -> None: + """Probe провалился (expected_total=None, best-effort пагинация) — 0 карточек + здесь НЕ аномалия (мы не знаем, есть ли реально данные в бакете).""" + s = AvitoScraper(RealScraperConfig()) + seen: dict[str, ScrapedLot] = {} + + with patch.object(s, "_parse_html", return_value=[]): + # Не должно поднимать исключение. + await s._paginate_leaf_bucket( + room_slug="studii-ASgBAgICAUSSA8YQ", + room_label="studio", + lo=0, + hi=3_000_000, + html=None, + max_pages=1, + seen=seen, + price_cap_per_bucket=1400, + max_pages_per_bucket=100, + concurrency=5, + secondary_only=True, + on_bucket=None, + skip_buckets=None, + expected_total=None, + ) + assert seen == {} + + +@pytest.mark.asyncio +async def test_leaf_bucket_expected_total_matches_collected_no_raise() -> None: + """Нормальный путь: probe total=2, парсинг реально даёт 2 карточки — не аномалия.""" + s = AvitoScraper(RealScraperConfig()) + seen: dict[str, ScrapedLot] = {} + lots = [_make_lot("L1"), _make_lot("L2")] + + with patch.object(s, "_parse_html", return_value=lots): + await s._paginate_leaf_bucket( + room_slug="studii-ASgBAgICAUSSA8YQ", + room_label="studio", + lo=0, + hi=3_000_000, + html="probe-page-1", + max_pages=1, + seen=seen, + price_cap_per_bucket=1400, + max_pages_per_bucket=100, + concurrency=5, + secondary_only=True, + on_bucket=None, + skip_buckets=None, + expected_total=2, + ) + assert set(seen.keys()) == {"L1", "L2"} diff --git a/tradein-mvp/backend/tests/test_cian_serp_offers_total_mismatch.py b/tradein-mvp/backend/tests/test_cian_serp_offers_total_mismatch.py new file mode 100644 index 00000000..5e0fcc04 --- /dev/null +++ b/tradein-mvp/backend/tests/test_cian_serp_offers_total_mismatch.py @@ -0,0 +1,106 @@ +"""Audit-scrapers finding 2: Cian totalOffers vs results.offers length mismatch. + +`_parse_serp_html` извлекает `totalOffers` и `results.offers` из ОДНОГО Redux +state-блоба (одна SSR-выдача). Раньше `results.offers` пустой при `totalOffers>0` +логировался WARNING'ом и тихо возвращался `[]` — не считался schema-regression, не +попадал в мониторинг (`_report_schema_regression`/Glitchtip). + +Порог: 0 vs >0 — единственный позиционно-независимый сигнал, который можно +проверить без номера страницы внутри `_parse_serp_html` (эта функция не знает, +какая это страница пагинации — дробный порог типа "< 50% от totalOffers" ложно +сработал бы на легитимной последней частичной странице exhaustive-пагинации, +которую эта функция не различает). totalOffers=0 (реально пустой поиск) НЕ +считается регрессией. + +Refs: audit-scrapers 2026-07-26, finding 2 (low). +""" + +from __future__ import annotations + +import os +from unittest.mock import MagicMock, patch + +os.environ.setdefault("DATABASE_URL", "postgresql+psycopg://test:test@localhost:5432/test") + +from scraper_kit.providers.cian.serp import CianScraper + +from app.services.scraper_adapters import RealScraperConfig + + +def _scraper() -> CianScraper: + return CianScraper(RealScraperConfig()) + + +def test_total_offers_positive_but_offers_empty_reports_regression() -> None: + """totalOffers=5, results.offers=[] — internal contradiction, must report.""" + s = _scraper() + state = {"results": {"totalOffers": 5, "offers": []}} + with patch("scraper_kit.providers.cian.serp.extract_state", return_value=state): + with patch.object(s, "_report_schema_regression") as mock_report: + lots = s._parse_serp_html("irrelevant") + + assert lots == [] + mock_report.assert_called_once() + (msg,), _ = mock_report.call_args + assert "totalOffers=5" in msg + + +def test_total_offers_zero_and_offers_empty_is_valid_empty_search() -> None: + """totalOffers=0, offers=[] — легитимная пустая выборка, НЕ регрессия.""" + s = _scraper() + state = {"results": {"totalOffers": 0, "offers": []}} + with patch("scraper_kit.providers.cian.serp.extract_state", return_value=state): + with patch.object(s, "_report_schema_regression") as mock_report: + lots = s._parse_serp_html("irrelevant") + + assert lots == [] + mock_report.assert_not_called() + + +def test_total_offers_none_and_offers_empty_is_not_reported_as_regression() -> None: + """totalOffers отсутствует/None в state — недостаточно сигнала для regression-репорта + (могла быть частично битая state-структура без явного totalOffers>0 контр-сигнала).""" + s = _scraper() + state = {"results": {"offers": []}} + with patch("scraper_kit.providers.cian.serp.extract_state", return_value=state): + with patch.object(s, "_report_schema_regression") as mock_report: + lots = s._parse_serp_html("irrelevant") + + assert lots == [] + mock_report.assert_not_called() + + +def test_offers_present_normal_path_unaffected() -> None: + """totalOffers=1, offers содержит 1 запись без cianId/id — не проходит + _offer_to_lot, но это уже существующая (0/N offer-level) охрана, не finding 2.""" + s = _scraper() + state = {"results": {"totalOffers": 1, "offers": [{"noId": True}]}} + with patch("scraper_kit.providers.cian.serp.extract_state", return_value=state): + with patch.object(s, "_report_schema_regression") as mock_report: + lots = s._parse_serp_html("irrelevant") + + # offers_data непустой → finding 2 guard не участвует; существующая offer-level + # охрана (raw_count>0 and saved_count==0) должна отработать вместо неё. + assert lots == [] + mock_report.assert_called_once() + (msg,), _ = mock_report.call_args + assert "_offer_to_lot" in msg + + +def test_state_none_extraction_failed_no_regression_report() -> None: + """extract_state вернул None (captcha/структура целиком не найдена) — уже + существующая ветка, НЕ должна триггерить finding-2 regression report.""" + s = _scraper() + with patch("scraper_kit.providers.cian.serp.extract_state", return_value=None): + with patch.object(s, "_report_schema_regression") as mock_report: + lots = s._parse_serp_html("irrelevant") + + assert lots == [] + mock_report.assert_not_called() + + +def test_report_schema_regression_swallows_missing_glitchtip_dsn() -> None: + """_report_schema_regression не должен падать, если glitchtip_dsn не настроен.""" + s = _scraper() + s._config = MagicMock(glitchtip_dsn=None) + s._report_schema_regression("test message") # не должно бросить исключение diff --git a/tradein-mvp/backend/tests/test_cian_session.py b/tradein-mvp/backend/tests/test_cian_session.py index 2518ea25..ee4e50e6 100644 --- a/tradein-mvp/backend/tests/test_cian_session.py +++ b/tradein-mvp/backend/tests/test_cian_session.py @@ -10,6 +10,8 @@ import pytest from app.services.cian_session import ( CIAN_REQUIRED_COOKIES, VERIFY_BAN_SENTINEL, + VERIFY_MARKUP_CHANGED_SENTINEL, + VERIFY_SOURCE_UNAVAILABLE_SENTINEL, _classify_verify_response, load_session, mark_session_invalid, @@ -211,14 +213,40 @@ def test_classify_200_authenticated_returns_state(monkeypatch: pytest.MonkeyPatc assert result == expected -def test_classify_200_state_missing_returns_none(monkeypatch: pytest.MonkeyPatch) -> None: - """200 but extract_state returns None → None.""" +def test_classify_200_state_missing_returns_markup_changed_sentinel( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """audit-scrapers finding 4: HTTP 200 но extract_state не нашёл auth-state + (Cian сменил вёрстку/MFE-схему header-frontend) → VERIFY_MARKUP_CHANGED_SENTINEL, + НЕ None. Раньше это конфлировалось с "cookies expired" (реальный логаут).""" monkeypatch.setattr( "app.services.cian_session.extract_state", lambda html, mfe, key: None, ) result = _classify_verify_response(200, "") - assert result is None + assert result is VERIFY_MARKUP_CHANGED_SENTINEL + assert result is not None # НЕ должно триггерить cookie-refresh alert + + +def test_classify_5xx_returns_source_unavailable_sentinel() -> None: + """audit-scrapers finding 4: HTTP 500 (источник недоступен) → + VERIFY_SOURCE_UNAVAILABLE_SENTINEL, НЕ None (cookies тут ни при чём).""" + result = _classify_verify_response(500, None) + assert result is VERIFY_SOURCE_UNAVAILABLE_SENTINEL + assert result is not None + + +def test_classify_502_returns_source_unavailable_sentinel() -> None: + """Любой non-200/403/401 статус (напр. 502 bad gateway) — источник недоступен.""" + result = _classify_verify_response(502, None) + assert result is VERIFY_SOURCE_UNAVAILABLE_SENTINEL + + +def test_classify_status_200_html_none_returns_source_unavailable_sentinel() -> None: + """Defensive: status=200 но html=None (не должно случаться в проде, но + classifier не должен молча вернуть None='expired') → source-unavailable.""" + result = _classify_verify_response(200, None) + assert result is VERIFY_SOURCE_UNAVAILABLE_SENTINEL def test_classify_403_is_distinct_from_401() -> None: @@ -230,6 +258,28 @@ def test_classify_403_is_distinct_from_401() -> None: assert expired is None +def test_classify_all_four_outcomes_are_mutually_distinct() -> None: + """audit-scrapers finding 4: expired (401) / ban (403) / source-unavailable (5xx) + / markup-changed (200+extract_state=None) — четыре РАЗНЫХ сигнала, ни один не + коллапсирует в другой. Только expired (None) должен триггерить re-login alert.""" + expired = _classify_verify_response(401, None) + ban = _classify_verify_response(403, None) + source_down = _classify_verify_response(500, None) + + with pytest.MonkeyPatch.context() as mp: + mp.setattr("app.services.cian_session.extract_state", lambda html, mfe, key: None) + markup_changed = _classify_verify_response(200, "") + + outcomes = [expired, ban, source_down, markup_changed] + # None встречается ровно один раз (только expired) — остальные три truthy sentinel'а + # и все различны между собой (identity, не equality — это разные dict-объекты). + assert outcomes.count(None) == 1 + assert expired is None + non_none = [o for o in outcomes if o is not None] + assert len(non_none) == 3 + assert len({id(o) for o in non_none}) == 3 + + # --------------------------------------------------------------------------- # verify_session (async) — integration with curl_cffi mock # --------------------------------------------------------------------------- @@ -317,10 +367,12 @@ async def test_verify_session_not_authenticated_returns_none( @pytest.mark.asyncio -async def test_verify_session_state_missing_returns_none( +async def test_verify_session_state_missing_returns_markup_changed_sentinel( monkeypatch: pytest.MonkeyPatch, ) -> None: - """200 + extract_state returns None → None.""" + """audit-scrapers finding 4: 200 + extract_state returns None (markup changed) + → VERIFY_MARKUP_CHANGED_SENTINEL, НЕ None. Раньше ложно триггерило "cookies + expired, please re-upload" для реальной причины "Cian сменил вёрстку".""" monkeypatch.setattr( "app.services.cian_session.extract_state", lambda html, mfe, key: None, @@ -334,7 +386,42 @@ async def test_verify_session_state_missing_returns_none( with patch("app.services.cian_session.AsyncSession", return_value=mock_session): result = await verify_session({"DMIR_AUTH": "x"}) - assert result is None + assert result is VERIFY_MARKUP_CHANGED_SENTINEL + assert result is not None + + +@pytest.mark.asyncio +async def test_verify_session_5xx_returns_source_unavailable_sentinel() -> None: + """audit-scrapers finding 4: HTTP 500 → VERIFY_SOURCE_UNAVAILABLE_SENTINEL, + НЕ None. Источник временно недоступен — cookies тут ни при чём, вызывающий + не должен помечать сессию invalid / просить re-upload.""" + mock_session = AsyncMock() + mock_session.__aenter__ = AsyncMock(return_value=mock_session) + mock_session.__aexit__ = AsyncMock(return_value=None) + mock_session.get = AsyncMock(return_value=_make_cffi_resp(500)) + + with patch("app.services.cian_session.AsyncSession", return_value=mock_session): + result = await verify_session({"DMIR_AUTH": "x"}) + + assert result is VERIFY_SOURCE_UNAVAILABLE_SENTINEL + assert result is not None + + +@pytest.mark.asyncio +async def test_verify_session_network_error_returns_source_unavailable_sentinel() -> None: + """audit-scrapers finding 4: сетевой/транспортный сбой (timeout, connection + reset и т.п.) → VERIFY_SOURCE_UNAVAILABLE_SENTINEL, НЕ None. Раньше generic + except возвращал None — конфлировал сетевой сбой с протухшими cookies.""" + mock_session = AsyncMock() + mock_session.__aenter__ = AsyncMock(return_value=mock_session) + mock_session.__aexit__ = AsyncMock(return_value=None) + mock_session.get = AsyncMock(side_effect=ConnectionError("connection reset by peer")) + + with patch("app.services.cian_session.AsyncSession", return_value=mock_session): + result = await verify_session({"DMIR_AUTH": "x"}) + + assert result is VERIFY_SOURCE_UNAVAILABLE_SENTINEL + assert result is not None @pytest.mark.asyncio diff --git a/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/avito/detail.py b/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/avito/detail.py index 915c5502..0b6c90e7 100644 --- a/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/avito/detail.py +++ b/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/avito/detail.py @@ -994,12 +994,18 @@ def _extract_meta(tree: HTMLParser) -> tuple[date | None, int | None, int | None day = int(m_date.group(1)) month_word = m_date.group(2).lower() month = RUS_MONTHS.get(month_word) - # Год — текущий (Avito не показывает год для свежих объявлений) - import datetime - - current_year = datetime.date.today().year if month: - publish_date = date(current_year, month, day) + # Avito не показывает год для свежих объявлений — берём текущий. + # audit-scrapers finding 3: если объявление опубликовано в декабре, + # а страница парсится в январе СЛЕДУЮЩЕГО года, "текущий год" даёт + # дату в будущем (завышает свежесть лота). Если получившаяся дата + # оказалась в будущем относительно момента парсинга — откатываем + # на год назад (это дата из прошлого года). + today = date.today() + candidate = date(today.year, month, day) + if candidate > today: + candidate = date(today.year - 1, month, day) + publish_date = candidate except (ValueError, KeyError): pass diff --git a/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/avito/serp.py b/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/avito/serp.py index 9b6e7329..510f0119 100644 --- a/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/avito/serp.py +++ b/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/avito/serp.py @@ -953,6 +953,29 @@ class AvitoScraper(BaseScraper): return 0 return None + def _is_unexpected_empty_page(self, html: str) -> bool: + """Отличить «в выборке реально 0 объявлений» от DOM-дрейфа/content-block. + + Вызывается ТОЛЬКО когда `_parse_html` уже вернул 0 карточек на page=1 + (обход всего города/категории/бакета) — само по себе это неотличимо от + «объявлений действительно нет» (#audit-scrapers finding 1). + + `_extract_total_count(html)` даёт независимый от DOM-карточек сигнал + (счётчик `page-title/count` либо no-results-маркер): + - total_hint == 0 → валидный no-results-маркер найден — НЕ аномалия. + - total_hint > 0 → счётчик утверждает, что результаты есть, но карточки + (`data-marker="item-*"`) не распознаны — DOM-маркер + карточки разошёлся со счётчиком (drift). + - total_hint is None → ни счётчика, ни no-results-маркера — тоже + подозрительно (captcha/firewall без ожидаемой + структуры страницы). + + Returns: + True — 0 карточек считается аномалией (нужно поднять + ``AvitoContentBlockedError``); False — валидная пустая выборка. + """ + return self._extract_total_count(html) != 0 + async def _fetch_rooms_page_html( self, room_slug: str, @@ -1282,6 +1305,7 @@ class AvitoScraper(BaseScraper): secondary_only=secondary_only, on_bucket=on_bucket, skip_buckets=skip_buckets, + expected_total=total, ) await walk_price_range( @@ -1309,6 +1333,7 @@ class AvitoScraper(BaseScraper): secondary_only: bool, on_bucket: Callable[..., Any] | None, skip_buckets: set[str] | None, + expected_total: int | None = None, ) -> None: """Параллельная пагинация одного leaf-бакета + фильтр + дедуп + on_bucket. @@ -1320,6 +1345,14 @@ class AvitoScraper(BaseScraper): bucket_key = "room_label:lo:hi" (закрытый) либо "room_label:lo:open" (открытый). skip_buckets: если bucket_key в skip_buckets — пагинация и on_bucket пропускаются. AvitoBlockedError/AvitoRateLimitedError из page-фетчей пробрасываются наверх. + + expected_total: total из probe (``_extract_total_count``), известный ДО вызова + (см. finding 1 audit-scrapers). Если задан и > 0, а после пагинации всех + ``max_pages`` страниц собрано 0 карточек — это противоречие (тот же probe-html + независимо утверждал total>0), т.е. DOM-маркер карточки разошёлся со счётчиком + (drift), а не легитимно пустой бакет (тот даёт expected_total=0 и сюда даже не + доходит — вызывающий _leaf не паджинирует пустые бакеты). None — probe провалился + (best-effort пагинация, отсутствие данных ожидаемо, проверка пропускается). """ _lo_param = lo if lo > 0 else None _hi_param = hi # None → _build_rooms_url не ставит pmax @@ -1378,6 +1411,23 @@ class AvitoScraper(BaseScraper): collected_this_bucket = len(bucket_lots) + # ── Guard: probe утверждал total>0, но парсинг всех страниц дал 0 карточек ── + # (finding 1 audit-scrapers). Тот же probe-html независимо подтвердил, что + # результаты есть (_extract_total_count) — 0 карточек здесь не может быть + # легитимной пустой выдачей, значит DOM-маркер карточки разошёлся со счётчиком. + if expected_total is not None and expected_total > 0 and collected_this_bucket == 0: + logger.error( + "avito: bucket %s probe expected_total=%d but 0 cards parsed across " + "%d page(s) — content-block/DOM-drift suspected", + bucket_key, + expected_total, + max_pages, + ) + raise AvitoContentBlockedError( + f"Avito bucket {bucket_key}: probe total={expected_total} but 0 cards " + "parsed — content-block/DOM-drift suspected" + ) + # ── Фильтр новостроек (secondary_only) ──────────────────────────────── dropped_nb = 0 if secondary_only: @@ -1596,6 +1646,18 @@ class AvitoScraper(BaseScraper): lots = self._parse_html(html, source_url_base=url) if not lots: + if page == 1 and self._is_unexpected_empty_page(html): + logger.error( + "avito %s SERP page=1 returned HTTP 200 but 0 cards " + "(no no-results marker) — likely content-block/captcha or " + "DOM-marker drift url=%s", + label, + url, + ) + raise AvitoContentBlockedError( + f"Avito {label} sweep: HTTP 200 with 0 cards on page=1 — " + "content-block/DOM-drift suspected" + ) logger.info("avito %s page=%d: 0 lots — end of pagination", label, page) break @@ -1706,6 +1768,18 @@ class AvitoScraper(BaseScraper): lots = self._parse_html(html, source_url_base=url) if not lots: + if page == 1 and self._is_unexpected_empty_page(html): + logger.error( + "avito byrooms category=%s page=1 returned HTTP 200 but 0 cards " + "(no no-results marker) — likely content-block/captcha or " + "DOM-marker drift url=%s", + name, + url, + ) + raise AvitoContentBlockedError( + f"Avito byrooms category={name}: HTTP 200 with 0 cards on " + "page=1 — content-block/DOM-drift suspected" + ) logger.info( "avito byrooms category=%s page=%d: 0 lots — end of category", name, diff --git a/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/cian/serp.py b/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/cian/serp.py index 7b03fca5..d35e46e1 100644 --- a/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/cian/serp.py +++ b/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/cian/serp.py @@ -639,6 +639,25 @@ class CianScraper(BaseScraper): if inspect.isawaitable(res_cb): await res_cb + def _report_schema_regression(self, message: str) -> None: + """Отправить сигнал schema-regression в Glitchtip (если настроен). + + Общий механизм для silent-failure guard'ов `_parse_serp_html` (offer-level + parse-failure и totalOffers/results.offers mismatch, audit-scrapers finding 2) + — переиспользуется, чтобы обе проверки одинаково попадали в мониторинг, а не + только в текстовый лог. + """ + try: + if self._config.glitchtip_dsn: + # Ленивый импорт: sentry_sdk — app-side error-reporting, не dep + # scraper_kit. Только когда glitchtip настроен И случилась + # schema-regression. + import sentry_sdk + + sentry_sdk.capture_message(message, level="error") + except Exception: + logger.debug("sentry_sdk report failed (not installed/initialised)", exc_info=True) + def _parse_serp_html(self, html: str) -> list[ScrapedLot]: """Извлечь offers из Cian Redux state. @@ -655,18 +674,41 @@ class CianScraper(BaseScraper): ) return [] - offers_data: list[dict[str, Any]] = state.get("results", {}).get("offers", []) + results = state.get("results", {}) + offers_data: list[dict[str, Any]] = results.get("offers", []) + total_offers = results.get("totalOffers") + if not offers_data: - logger.warning( - "cian state found but results.offers пуст (totalOffers=%s)", - state.get("results", {}).get("totalOffers", "?"), - ) + # audit-scrapers finding 2: totalOffers и results.offers приходят из ОДНОГО + # state-блоба (одна SSR-выдача) — если totalOffers>0, а offers пуст, это + # внутреннее противоречие payload'а, а не легитимная пагинация (probe/leaf + # запрашивают только max_pages = ceil(totalOffers/28), посчитанные из ТОГО ЖЕ + # totalOffers, так что «сходили за последнюю страницу» здесь не объясняет 0). + # Порог: 0 vs >0 — единственный позиционно-независимый сигнал, который можно + # проверить без номера страницы; дробный порог (напр. «< 50% от expected») + # ложно сработал бы на легитимной последней частичной странице пагинации, + # которую эта функция не различает. + if isinstance(total_offers, int) and total_offers > 0: + logger.error( + "cian SERP: totalOffers=%d но results.offers пуст — schema " + "regression suspected (counter/offers mismatch, not empty search)", + total_offers, + ) + self._report_schema_regression( + f"cian SERP: totalOffers={total_offers} but results.offers is " + "empty — possible schema regression (counter/offers mismatch)" + ) + else: + logger.warning( + "cian state found but results.offers пуст (totalOffers=%s)", + total_offers if total_offers is not None else "?", + ) return [] logger.info( "cian SERP state ok: %d offers (totalOffers=%s)", len(offers_data), - state.get("results", {}).get("totalOffers", "?"), + total_offers if total_offers is not None else "?", ) lots: list[ScrapedLot] = [] @@ -684,20 +726,10 @@ class CianScraper(BaseScraper): "cian SERP: 0/%d offers прошли _offer_to_lot — возможна schema regression", raw_count, ) - try: - if self._config.glitchtip_dsn: - # Ленивый импорт: sentry_sdk — app-side error-reporting, не dep - # scraper_kit. Только когда glitchtip настроен И случилась - # schema-regression. ImportError глотается общим except ниже. - import sentry_sdk - - sentry_sdk.capture_message( - f"cian SERP: {raw_count}/{raw_count} offers failed _offer_to_lot" - " — possible schema regression", - level="error", - ) - except Exception: - pass # sentry_sdk not installed/initialised in dev + self._report_schema_regression( + f"cian SERP: {raw_count}/{raw_count} offers failed _offer_to_lot" + " — possible schema regression" + ) return lots From 76016fd469caabc6a2d4a9fdc6120fe25abf2b4f Mon Sep 17 00:00:00 2001 From: lekss361 Date: Sun, 26 Jul 2026 22:42:15 +0000 Subject: [PATCH 2/6] =?UTF-8?q?fix(tradein/security):=20=D1=83=D1=82=D0=B5?= =?UTF-8?q?=D1=87=D0=BA=D0=B0=20=D0=BA=D0=BB=D1=8E=D1=87=D0=B0=20=D0=BF?= =?UTF-8?q?=D1=80=D0=BE=D0=BA=D1=81=D0=B8,=20=D0=B0=D1=83=D0=B4=D0=B8?= =?UTF-8?q?=D1=82=20=D0=B4=D0=B5=D0=B9=D1=81=D1=82=D0=B2=D0=B8=D0=B9=20?= =?UTF-8?q?=D0=B0=D0=B4=D0=BC=D0=B8=D0=BD=D0=B0,=20=D0=BE=D1=82=D0=BB?= =?UTF-8?q?=D0=B8=D1=87=D0=B8=D0=BC=D0=BE=D1=81=D1=82=D1=8C=20=D0=BD=D0=B5?= =?UTF-8?q?=D1=83=D0=B4=D0=B0=D1=87=D0=BD=D0=BE=D0=B3=D0=BE=20=D0=B2=D1=85?= =?UTF-8?q?=D0=BE=D0=B4=D0=B0,=20IDOR=20=D0=B2=20=D0=B7=D0=B0=D1=8F=D0=B2?= =?UTF-8?q?=D0=BA=D0=B5=20(#2536)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tradein-mvp/backend/app/api/v1/admin.py | 9 +- tradein-mvp/backend/app/api/v1/lead.py | 24 ++- tradein-mvp/backend/app/core/request_audit.py | 87 ++++++++-- .../backend/app/observability/sentry_scrub.py | 70 +++++++- .../backend/tests/test_request_audit.py | 151 +++++++++++++++++- .../backend/tests/test_scraper_admin_apis.py | 39 ++++- .../backend/tests/test_trade_in_lead.py | 102 +++++++++++- 7 files changed, 458 insertions(+), 24 deletions(-) diff --git a/tradein-mvp/backend/app/api/v1/admin.py b/tradein-mvp/backend/app/api/v1/admin.py index 28d793ca..e7df8544 100644 --- a/tradein-mvp/backend/app/api/v1/admin.py +++ b/tradein-mvp/backend/app/api/v1/admin.py @@ -2394,9 +2394,14 @@ async def rotate_proxy_ip( data = resp.json() except Exception: data = {} - except Exception as exc: + except Exception: + # НЕ отдавать str(exc) клиенту (аудит-фикс, #security-audit): httpx-исключения + # несут полный request URL, а rotate_url — mobileproxy changeip-ссылка с API- + # ключом провайдера в query-string (?...&proxy_key=...). str(exc) с этим URL в + # HTTP-ответе — прямая утечка секрета вызывающему клиенту. Причина сбоя остаётся + # в логах (exc_info=True) для диагностики; наружу — только нейтральный reason. logger.warning("rotate-ip: changeip failed source=%s", source, exc_info=True) - return RotateIpResponse(ok=False, reason=f"changeip error: {exc}") + return RotateIpResponse(ok=False, reason="changeip request failed") # changeip отдаёт новый IP в одном из полей (формат провайдер-зависимый). new_ip = None diff --git a/tradein-mvp/backend/app/api/v1/lead.py b/tradein-mvp/backend/app/api/v1/lead.py index 5ab93bf7..a5f1d106 100644 --- a/tradein-mvp/backend/app/api/v1/lead.py +++ b/tradein-mvp/backend/app/api/v1/lead.py @@ -5,6 +5,16 @@ POST /api/v1/trade-in/lead — контактная заявка с резуль trade_in_leads. Notification (Telegram/email) — вне scope: нет существующей SMTP/Telegram интеграции в коде (подтверждено при разборе issue), только persist + log; `notified_at` в таблице зарезервирован под будущую доставку. + +IDOR-фикс (security-audit): `estimate_id` раньше только проверялся на +СУЩЕСТВОВАНИЕ (`SELECT 1 ... WHERE id = ...`), без проверки владельца — любой +аутентифицированный пилот мог привязать свою заявку к чужой оценке (утечка через +последующий просмотр лида: чужой адрес/телефон/оценка в заявке, которую видит не +её владелец). Гвард переиспользует `_assert_estimate_access` из +`app.api.v1.trade_in` — тот же owner-or-admin подход, что и `GET /estimate/{id}` +(#690, `tests/test_estimate_idor.py`): 401 без `X-Authenticated-User`, 403 — +неизвестная роль, 404 — оценка не найдена ИЛИ принадлежит не этому пользователю +(существование чужой оценки не подтверждаем). """ from __future__ import annotations @@ -14,11 +24,12 @@ import re from typing import Annotated, Any, Literal from uuid import UUID -from fastapi import APIRouter, Depends, HTTPException, Request +from fastapi import APIRouter, Depends, Header, HTTPException, Request from pydantic import BaseModel, Field, field_validator from sqlalchemy import text from sqlalchemy.orm import Session +from app.api.v1.trade_in import _assert_estimate_access from app.core.db import get_db logger = logging.getLogger(__name__) @@ -75,15 +86,20 @@ async def create_trade_in_lead( payload: TradeInLeadInput, request: Request, db: Annotated[Session, Depends(get_db)], + x_authenticated_user: Annotated[str | None, Header(alias="X-Authenticated-User")] = None, ) -> dict[str, Any]: """Сохраняет лид (телефон + согласие) в trade_in_leads.""" if payload.estimate_id is not None: - exists = db.execute( - text("SELECT 1 FROM trade_in_estimates WHERE id = CAST(:id AS uuid)"), + estimate_row = db.execute( + text("SELECT created_by FROM trade_in_estimates WHERE id = CAST(:id AS uuid)"), {"id": str(payload.estimate_id)}, ).fetchone() - if exists is None: + if estimate_row is None: raise HTTPException(status_code=404, detail="estimate not found") + # IDOR guard (security-audit, зеркалит #690): нельзя привязать лид к + # чужой оценке. 404 и на "не найдено", и на "чужая" — не подтверждаем + # существование чужого estimate_id. + _assert_estimate_access(estimate_row.created_by, x_authenticated_user) user_agent = request.headers.get("user-agent") # 152-ФЗ audit trail: реальный клиентский IP из X-Forwarded-For (его ставит diff --git a/tradein-mvp/backend/app/core/request_audit.py b/tradein-mvp/backend/app/core/request_audit.py index a217cbfd..7eafefbc 100644 --- a/tradein-mvp/backend/app/core/request_audit.py +++ b/tradein-mvp/backend/app/core/request_audit.py @@ -1,10 +1,27 @@ -"""RequestAuditMiddleware — пишет `api_request` (+ дедуплицированный `login`) -события в `user_events` для каждого аутентифицированного `/api/*` запроса. +"""RequestAuditMiddleware — пишет `api_request` / `admin_action` (+ дедуплицированный +`login`/`login_failed`) события в `user_events` для каждого аутентифицированного +`/api/*` запроса. Foundation для Feature 2 (login/IP audit) и базы Feature 3 (behavior analytics). Логирование выполняется ПОСЛЕ `call_next` (не задерживает и не ветвит реальный ответ клиенту) и через fire-and-forget `schedule_event` — сбой аудита никогда не влияет на HTTP-ответ. + +Порядок middleware-стека (см. `app/main.py`: `rbac_guard` — `@app.middleware("http")`, +объявлен ДО `app.add_middleware(RequestAuditMiddleware)`) делает `RequestAudit` +ВНЕШНИМ по отношению к `rbac_guard` (Starlette строит стек в обратном порядке +регистрации — последний `add_middleware` оборачивает предыдущие). Поэтому к моменту, +когда код ниже читает `response.status_code`, в нём уже отражён исход rbac_guard +(401/403 short-circuit) ИЛИ реального хендлера — статус несёт реальный смысл +"успех/отказ", а не только "запрос дошёл до хендлера". + +Заведомо неаутентифицированный трафик (сканеры, долбящиеся в /wp-login.php и т.п. +без валидного basic_auth) сюда вообще не попадает: Caddy гейтит basic_auth ПЕРЕД +проксированием, так что `X-Authenticated-User` в таких запросах нет — условие +`if username and ...` ниже их уже отсекает. Поэтому шум сканеров не нужно +дополнительно фильтровать в этом файле — тот класс проблемы («сигнал тонет в шуме +сканера») здесь структурно невозможен: событие может появиться только для +запроса, прошедшего Caddy basic_auth. """ from __future__ import annotations @@ -25,6 +42,13 @@ logger = logging.getLogger(__name__) # циклическую зависимость. _PUBLIC_PATHS = frozenset({"/health", "/docs", "/redoc", "/openapi.json"}) +# Методы, меняющие состояние — для /api/v1/admin/* именно они должны попадать в +# аудит с атрибуцией (кто именно загрузил куки / включил авто-логин / поправил +# прокси / изменил настройки скрапера / выполнил bulk-операцию). GET/HEAD/OPTIONS +# на /admin/* остаются вне аудита (см. комментарий ниже — это просмотр дашбордов, +# не действие). +_MUTATING_METHODS = frozenset({"POST", "PUT", "PATCH", "DELETE"}) + class RequestAuditMiddleware(BaseHTTPMiddleware): """Логирует активность аутентифицированных пользователей в `user_events`.""" @@ -38,32 +62,67 @@ class RequestAuditMiddleware(BaseHTTPMiddleware): if username and path.startswith("/api/") and path not in _PUBLIC_PATHS: ip = _client_ip(request) ua = request.headers.get("user-agent") + method = request.method + success = response.status_code < 400 + is_admin_path = path.startswith("/api/v1/admin/") # Общий behavior/activity-поток — каждый authenticated API-запрос. - # /api/v1/admin/* исключаем: это ops-действия (просмотр самих - # дашбордов аудита/аналитики), а не поведение пилота — иначе - # запросы дашборда зашумляют top_paths и счётчики активности. - # login ниже логируем всегда (вход админа с IP — валидный аудит). - if not path.startswith("/api/v1/admin/"): + # /api/v1/admin/* исключаем из `api_request`: это ops-действия + # (просмотр дашбордов аудита/аналитики), а не поведение пилота — + # иначе запросы дашборда зашумляют top_paths и счётчики активности. + if not is_admin_path: schedule_event( event_type="api_request", username=username, ip=ip, user_agent=ua, path=path, - method=request.method, + method=method, + payload={"status_code": response.status_code}, ) - - # Дедуплицированный login/IP-audit сигнал — максимум раз в день - # на (юзер, IP, устройство). - if should_log_login(username, ip, ua): + elif method in _MUTATING_METHODS: + # Admin-аудит (security-audit fix): раньше ЛЮБОЙ запрос под + # /api/v1/admin/* (включая меняющие состояние — загрузка кук, + # авто-логин, правка прокси, настройки скраперов, bulk-операции) + # полностью исключался из `user_events` тем же условием, что и + # шумные GET-дашборды — установить, КТО совершил действие, было + # невозможно. Пишем факт действия + атрибуцию (username/ip/path/ + # method/статус) — БЕЗ тела запроса (там куки/пароли/секреты + # правки прокси), это НЕ payload-лог, а событие "что произошло". schedule_event( - event_type="login", + event_type="admin_action", username=username, ip=ip, user_agent=ua, path=path, - method=request.method, + method=method, + payload={"status_code": response.status_code, "success": success}, + ) + + # Дедуплицированный login/IP-audit сигнал — максимум раз в день + # на (юзер, IP, устройство). Security-audit fix: раньше событие + # всегда писалось как `login` независимо от исхода запроса — + # отражённая RBAC-попытка (валидный Caddy basic_auth, но + # 401/403 от rbac_guard: протухший X-Internal-Auth-Secret, + # неизвестная роль, scope-блок) была неотличима от настоящего + # входа. Теперь тип события расходится по `response.status_code`: + # `login` — успех, `login_failed` — otказ. Дедуп-бакет (once per + # user+ip+ua+day) НЕ разбит отдельно на success/fail (это + # потребовало бы менять `should_log_login` в user_events.py — + # вне scope этого фикса): если в рамках одного дня с этого же + # устройства сначала случился отказ, а затем реальный успешный + # вход, второе событие в тот же день не запишется — тот же + # компромисс дедупа, что был и раньше, разница только в том, что + # теперь ЕДИНСТВЕННОЕ событие дня корректно отражает, чем оно было. + if should_log_login(username, ip, ua): + schedule_event( + event_type="login" if success else "login_failed", + username=username, + ip=ip, + user_agent=ua, + path=path, + method=method, + payload={"status_code": response.status_code}, ) except Exception: logger.warning("RequestAuditMiddleware: failed to record event", exc_info=True) diff --git a/tradein-mvp/backend/app/observability/sentry_scrub.py b/tradein-mvp/backend/app/observability/sentry_scrub.py index 197e86be..9d68d457 100644 --- a/tradein-mvp/backend/app/observability/sentry_scrub.py +++ b/tradein-mvp/backend/app/observability/sentry_scrub.py @@ -47,6 +47,31 @@ _TG_BOT_TOKEN_REPLACEMENT = "/bot[REDACTED]" # `id:value` в логах, напр. `chat_id:12345`). _TG_BOT_TOKEN_BARE_RE = re.compile(r"\b\d{6,12}:[A-Za-z0-9_-]{30,}\b") +# Query-string секреты в исходящих URL сторонних API (аудит-фикс, #security-audit): +# mobileproxy changeip-ссылка (`AVITO_PROXY_ROTATE_URL` и др., admin.py +# rotate_proxy_ip) несёт провайдерский API-ключ в query (`?...&proxy_key=...`). +# Два независимых пути утечки в GlitchTip, зеркалящих TG-токен выше: +# 1. `HttpxIntegration.send()` парсит URL через `parse_url(str(request.url), +# sanitize=False)` (ЯВНЫЙ opt-out из sentry_sdk `sanitize_url`, который иначе +# сам вырезал бы query-параметры) и кладёт полный URL в span `data["url"]` — +# сейчас неактивно (`traces_sample_rate=0.0` в app/main.py/scheduler_main.py → +# span не сэмплится/не уходит), но молча перестанет спасать, если трейсинг +# когда-нибудь включат. +# 2. `include_local_variables=True` (sentry_sdk default в app/main.py — в отличие +# от tgbot_main.py, где явно False) кладёт stack-frame locals (`rotate_url`, +# `exc` в rotate_proxy_ip) в traceback открытым текстом. +# Как и TG-токен — full-text regex по КАЖДОЙ строке event (не ключ-based): секрет +# может всплыть где угодно (frame locals, breadcrumb, exception message). НЕ +# завязано на конкретного провайдера — покрывает любой query-параметр из +# общеупотребимого набора секретных имён (api_key/proxy_key/token/secret/password/ +# access_token/auth), т.к. cian/yandex у нас имеют СВОИ rotate-URL (потенциально +# другой провайдер, другое имя параметра). +_URL_SECRET_QUERY_RE = re.compile( + r"(?i)([?&](?:api[_-]?key|proxy[_-]?key|token|secret|password|pwd|" + r"access[_-]?token|auth)=)[^&\s\"'<>]+" +) +_URL_SECRET_QUERY_REPLACEMENT = r"\g<1>" + _REDACTED + def _scrub(obj: Any) -> None: """Рекурсивно заменить значения PII-ключей в dict на [REDACTED] (in-place).""" @@ -61,8 +86,50 @@ def _scrub(obj: Any) -> None: _scrub(item) +def _redact_url_secrets_inplace(obj: Any) -> None: + """Рекурсивно (IN-PLACE, как `_scrub`) заменяет значения секрет-подобных + query-параметров (`?token=...`, `?proxy_key=...` и т.п.) на [REDACTED] в + КАЖДОЙ строке event — не ключ-based: секрет утекает через httpx span + `url`/`query` data и через текст исключений (`str(exc)` httpx содержит полный + request URL), а не только через известные PII-поля формы. Мутирует dict/list + на месте (НЕ пересоздаёт структуру, в отличие от `_redact_strings`) — + сохраняет identity верхнеуровневого `event`, на что опирается контракт + `scrub_pii_event`/`before_send` и существующие тесты (`out is event`). + """ + if isinstance(obj, dict): + for key, value in obj.items(): + if isinstance(value, str): + redacted = _URL_SECRET_QUERY_RE.sub(_URL_SECRET_QUERY_REPLACEMENT, value) + if redacted != value: + obj[key] = redacted + else: + _redact_url_secrets_inplace(value) + elif isinstance(obj, list): + for i, value in enumerate(obj): + if isinstance(value, str): + redacted = _URL_SECRET_QUERY_RE.sub(_URL_SECRET_QUERY_REPLACEMENT, value) + if redacted != value: + obj[i] = redacted + else: + _redact_url_secrets_inplace(value) + # tuple намеренно не обрабатываем: sentry_sdk event — это JSON-совместимая + # структура (dict/list/str/int/...), tuple там не встречается, а даже если бы + # встретился — он immutable, in-place правка невозможна (см. `_scrub`, тот же + # выбор для dict/list). + + def scrub_pii_event(event: Event, _hint: dict[str, Any]) -> Event | None: - """Redact consumer-PII из error event перед отправкой. Возвращает event (не None).""" + """Redact consumer-PII + URL query-string секретов из error event перед отправкой. + + Композиция (обе — in-place, сохраняют identity `event`): (1) ключ-based + dict-scrub consumer-PII полей формы (как раньше), (2) full-text regex-проход + по ВСЕМУ event, вырезающий значения секрет-подобных query-параметров в любой + строке (proxy/API-ключи в исходящих URL сторонних сервисов, напр. mobileproxy + changeip — #security-audit). Второй шаг не завязан на конкретные ключи полей — + ловит секрет в frame locals, breadcrumb, exception message и т.д., где он может + оказаться независимо от include_local_variables/traces_sample_rate. Возвращает + event (не None). + """ if not isinstance(event, dict): return event request = event.get("request") @@ -70,6 +137,7 @@ def scrub_pii_event(event: Event, _hint: dict[str, Any]) -> Event | None: _scrub(request.get("data")) _scrub(event.get("extra")) _scrub(event.get("contexts")) + _redact_url_secrets_inplace(event) return event diff --git a/tradein-mvp/backend/tests/test_request_audit.py b/tradein-mvp/backend/tests/test_request_audit.py index 62bbf525..06bbd2d5 100644 --- a/tradein-mvp/backend/tests/test_request_audit.py +++ b/tradein-mvp/backend/tests/test_request_audit.py @@ -15,7 +15,7 @@ os.environ.setdefault("DATABASE_URL", "postgresql+psycopg://test:test@localhost: from unittest.mock import patch import pytest -from fastapi import FastAPI +from fastapi import FastAPI, Response from fastapi.testclient import TestClient from app.core.request_audit import RequestAuditMiddleware @@ -138,3 +138,152 @@ def test_admin_path_excluded_from_api_request_but_login_kept() -> None: assert resp.status_code == 200 event_types = [c.kwargs["event_type"] for c in mock_schedule.call_args_list] assert event_types == ["login"] + + +# ── Admin audit (security-audit fix): mutating /admin/* -> admin_action ──────── + + +def test_admin_mutating_post_schedules_admin_action_with_attribution() -> None: + """POST на /api/v1/admin/* (напр. правка прокси / настройки скрапера) должен + писать `admin_action` с атрибуцией (кто), а НЕ игнорироваться целиком, как + раньше (security-audit: не было возможности установить, кто это сделал).""" + app = FastAPI() + app.add_middleware(RequestAuditMiddleware) + + @app.post("/api/v1/admin/scraper/avito/rotate-ip") + def rotate_ip() -> dict[str, bool]: + return {"ok": True} + + with ( + patch("app.core.request_audit.schedule_event") as mock_schedule, + patch("app.core.request_audit.should_log_login", return_value=False), + ): + resp = TestClient(app).post( + "/api/v1/admin/scraper/avito/rotate-ip", + headers={"X-Authenticated-User": "admin"}, + ) + + assert resp.status_code == 200 + assert mock_schedule.call_count == 1 + kwargs = mock_schedule.call_args.kwargs + assert kwargs["event_type"] == "admin_action" + assert kwargs["username"] == "admin" + assert kwargs["path"] == "/api/v1/admin/scraper/avito/rotate-ip" + assert kwargs["method"] == "POST" + assert kwargs["payload"] == {"status_code": 200, "success": True} + + +def test_admin_get_does_not_schedule_admin_action() -> None: + """GET на /admin/* (просмотр дашборда) НЕ должен писать admin_action — только + мутирующие методы считаются "действием".""" + app = FastAPI() + app.add_middleware(RequestAuditMiddleware) + + @app.get("/api/v1/admin/scraper/health") + def health() -> dict[str, bool]: + return {"ok": True} + + with ( + patch("app.core.request_audit.schedule_event") as mock_schedule, + patch("app.core.request_audit.should_log_login", return_value=False), + ): + TestClient(app).get( + "/api/v1/admin/scraper/health", headers={"X-Authenticated-User": "admin"} + ) + + mock_schedule.assert_not_called() + + +def test_admin_action_payload_excludes_request_body() -> None: + """security-audit: тело запроса (куки/пароли/секреты правки прокси) НЕ должно + попадать в audit-payload — только факт действия + атрибуция.""" + app = FastAPI() + app.add_middleware(RequestAuditMiddleware) + + @app.post("/api/v1/admin/scraper/cookies") + def upload_cookies() -> dict[str, bool]: + # Хендлер намеренно НЕ объявляет body-параметр — middleware проверяет + # только headers/path/method/status, JSON-тело запроса ниже (куки) в + # audit-payload попасть не может структурно, не только "по договорённости". + return {"ok": True} + + with ( + patch("app.core.request_audit.schedule_event") as mock_schedule, + patch("app.core.request_audit.should_log_login", return_value=False), + ): + TestClient(app).post( + "/api/v1/admin/scraper/cookies", + headers={"X-Authenticated-User": "admin"}, + json={"cookies": "super-secret-session-cookie"}, + ) + + kwargs = mock_schedule.call_args.kwargs + assert kwargs["event_type"] == "admin_action" + assert "super-secret-session-cookie" not in repr(kwargs) + + +def test_admin_mutating_failure_status_recorded_in_payload() -> None: + """admin_action на неуспешный ответ (напр. 500 от нижестоящего сервиса) должен + нести success=False + реальный status_code — не маскироваться под успех.""" + app = FastAPI() + app.add_middleware(RequestAuditMiddleware) + + @app.post("/api/v1/admin/scraper/pacing") + def pacing() -> Response: + return Response(status_code=502) + + with ( + patch("app.core.request_audit.schedule_event") as mock_schedule, + patch("app.core.request_audit.should_log_login", return_value=False), + ): + TestClient(app).post( + "/api/v1/admin/scraper/pacing", headers={"X-Authenticated-User": "admin"} + ) + + kwargs = mock_schedule.call_args.kwargs + assert kwargs["event_type"] == "admin_action" + assert kwargs["payload"] == {"status_code": 502, "success": False} + + +# ── login vs login_failed (security-audit fix) ────────────────────────────────── + + +def test_login_event_type_when_request_succeeds(client: TestClient) -> None: + """Ответ < 400 -> event_type='login' (успешный вход/активность), payload несёт + status_code.""" + with ( + patch("app.core.request_audit.schedule_event") as mock_schedule, + patch("app.core.request_audit.should_log_login", return_value=True), + ): + client.get("/api/v1/ping", headers={"X-Authenticated-User": "alice"}) + + login_calls = [c for c in mock_schedule.call_args_list if c.kwargs["event_type"] == "login"] + assert len(login_calls) == 1 + assert login_calls[0].kwargs["payload"] == {"status_code": 200} + + +def test_login_failed_event_type_when_rbac_rejects_request() -> None: + """Ответ >= 400 (напр. RBAC-отказ downstream: неизвестная роль / протухший + внутренний секрет) -> event_type='login_failed', а НЕ 'login' — раньше эти + два случая были неразличимы в журнале (security-audit).""" + app = FastAPI() + app.add_middleware(RequestAuditMiddleware) + + @app.get("/api/v1/ping") + def ping() -> Response: + return Response(status_code=403, content="forbidden") + + with ( + patch("app.core.request_audit.schedule_event") as mock_schedule, + patch("app.core.request_audit.should_log_login", return_value=True), + ): + TestClient(app).get("/api/v1/ping", headers={"X-Authenticated-User": "alice"}) + + login_calls = [ + c + for c in mock_schedule.call_args_list + if c.kwargs["event_type"] in ("login", "login_failed") + ] + assert len(login_calls) == 1 + assert login_calls[0].kwargs["event_type"] == "login_failed" + assert login_calls[0].kwargs["payload"] == {"status_code": 403} diff --git a/tradein-mvp/backend/tests/test_scraper_admin_apis.py b/tradein-mvp/backend/tests/test_scraper_admin_apis.py index 4d1a4390..ecfd4578 100644 --- a/tradein-mvp/backend/tests/test_scraper_admin_apis.py +++ b/tradein-mvp/backend/tests/test_scraper_admin_apis.py @@ -305,7 +305,44 @@ def test_rotate_ip_changeip_error(client: TestClient) -> None: assert r.status_code == 200 body = r.json() assert body["ok"] is False - assert "changeip error" in body["reason"] + # security-audit: нейтральный reason, БЕЗ текста исходного исключения + # (str(exc) httpx мог нести rotate_url с proxy-ключом в query — см. тест ниже). + assert body["reason"] == "changeip request failed" + + +def test_rotate_ip_changeip_error_does_not_leak_proxy_key(client: TestClient) -> None: + """security-audit: секретный API-ключ провайдера в rotate_url НЕ должен попасть + в HTTP-ответ клиенту через текст httpx-исключения (раньше + `reason=f"changeip error: {exc}"` отдавал str(exc) с полным URL, включая + query-параметр ключа, наружу).""" + from app.api.v1 import admin as admin_module + + secret_url = "http://ch/changeip?proxy_key=TOP-SECRET-KEY-1234" + + class _BoomClient: + def __init__(self, *a: Any, **k: Any) -> None: + pass + + async def __aenter__(self) -> _BoomClient: + return self + + async def __aexit__(self, *a: Any) -> None: + return None + + async def get(self, *a: Any, **k: Any) -> Any: + raise RuntimeError(f"All connection attempts failed for {secret_url}&format=json") + + with ( + patch.object(admin_module.httpx, "AsyncClient", _BoomClient), + patch.object(admin_module.settings, "avito_proxy_rotate_url", secret_url), + ): + r = client.post("/api/v1/admin/scraper/avito/rotate-ip") + + assert r.status_code == 200 + assert "TOP-SECRET-KEY-1234" not in r.text + body = r.json() + assert body["ok"] is False + assert "TOP-SECRET-KEY-1234" not in (body["reason"] or "") # ── API 4: GET /scraper/pacing ─────────────────────────────────────────────── diff --git a/tradein-mvp/backend/tests/test_trade_in_lead.py b/tradein-mvp/backend/tests/test_trade_in_lead.py index 8f2f9859..8afd994f 100644 --- a/tradein-mvp/backend/tests/test_trade_in_lead.py +++ b/tradein-mvp/backend/tests/test_trade_in_lead.py @@ -7,6 +7,9 @@ - телефон без цифр / слишком мало цифр -> 422 (digit-guard, #2376 hardening) - source="landing" (мёртвая воронка) -> 422 (литерал убран из схемы) - estimate_id, которого нет в trade_in_estimates -> 404 + - IDOR guard (security-audit, зеркалит #690/test_estimate_idor.py): estimate_id + чужого пользователя -> 404; admin может привязать любой; нет + X-Authenticated-User -> 401; неизвестная роль -> 403 """ from __future__ import annotations @@ -16,6 +19,7 @@ import os os.environ.setdefault("DATABASE_URL", "postgresql+psycopg://test:test@localhost:5432/test") from datetime import UTC, datetime +from types import SimpleNamespace from typing import Any from unittest.mock import MagicMock from uuid import uuid4 @@ -45,6 +49,16 @@ def client(db: MagicMock) -> TestClient: return TestClient(app) +@pytest.fixture(autouse=True) +def _restore_get_role(): + """Restore app.core.auth.get_role after each test (mirror test_estimate_idor.py).""" + from app.core import auth as auth_mod + + original = auth_mod.get_role + yield + auth_mod.get_role = original + + def _insert_result(lead_id: str) -> MagicMock: result = MagicMock() result.mappings.return_value.one.return_value = { @@ -135,9 +149,14 @@ def test_lead_with_unknown_estimate_id_404(client: TestClient, db: MagicMock) -> def test_lead_with_known_estimate_id_200(client: TestClient, db: MagicMock) -> None: + """Owner привязывает лид к своей же оценке -> 200.""" + from app.core import auth as auth_mod + + auth_mod.get_role = lambda _u: "pilot" # type: ignore[assignment] + lead_id = str(uuid4()) exists_result = MagicMock() - exists_result.fetchone.return_value = (1,) + exists_result.fetchone.return_value = SimpleNamespace(created_by="kopylov") db.execute.side_effect = [exists_result, _insert_result(lead_id)] estimate_id = str(uuid4()) @@ -148,6 +167,7 @@ def test_lead_with_known_estimate_id_200(client: TestClient, db: MagicMock) -> N "consent": True, "estimate_id": estimate_id, }, + headers={"X-Authenticated-User": "kopylov"}, ) assert r.status_code == 200, r.text params = db.execute.call_args.args[1] @@ -155,6 +175,86 @@ def test_lead_with_known_estimate_id_200(client: TestClient, db: MagicMock) -> N assert params["source"] == "result" +# ── IDOR guard (security-audit): estimate_id ownership ───────────────────────── + + +def test_lead_estimate_id_owned_by_other_user_gets_404(client: TestClient, db: MagicMock) -> None: + """Чужой estimate_id -> 404 (существование не подтверждаем), лид НЕ создаётся.""" + from app.core import auth as auth_mod + + auth_mod.get_role = lambda _u: "pilot" # type: ignore[assignment] + + exists_result = MagicMock() + exists_result.fetchone.return_value = SimpleNamespace(created_by="victim") + db.execute.return_value = exists_result + + r = client.post( + "/api/v1/trade-in/lead", + json={"phone": "+79123456789", "consent": True, "estimate_id": str(uuid4())}, + headers={"X-Authenticated-User": "attacker"}, + ) + assert r.status_code == 404, r.text + assert not db.commit.called + + +def test_lead_estimate_id_admin_can_attach_any_200(client: TestClient, db: MagicMock) -> None: + """Admin может привязать лид к чужой оценке (owner-or-admin, зеркалит #690).""" + from app.core import auth as auth_mod + + auth_mod.get_role = lambda _u: "admin" # type: ignore[assignment] + + lead_id = str(uuid4()) + exists_result = MagicMock() + exists_result.fetchone.return_value = SimpleNamespace(created_by="someone_else") + db.execute.side_effect = [exists_result, _insert_result(lead_id)] + + r = client.post( + "/api/v1/trade-in/lead", + json={"phone": "+79123456789", "consent": True, "estimate_id": str(uuid4())}, + headers={"X-Authenticated-User": "admin"}, + ) + assert r.status_code == 200, r.text + + +def test_lead_estimate_id_requires_authenticated_user_401( + client: TestClient, db: MagicMock +) -> None: + """estimate_id задан, но нет X-Authenticated-User -> 401 (defense-in-depth: в + проде rbac_guard уже требует заголовок раньше, см. app/main.py).""" + exists_result = MagicMock() + exists_result.fetchone.return_value = SimpleNamespace(created_by="kopylov") + db.execute.return_value = exists_result + + r = client.post( + "/api/v1/trade-in/lead", + json={"phone": "+79123456789", "consent": True, "estimate_id": str(uuid4())}, + ) + assert r.status_code == 401, r.text + assert not db.commit.called + + +def test_lead_estimate_id_unknown_role_403(client: TestClient, db: MagicMock) -> None: + """Аутентифицирован через Caddy, но роль отсутствует в roles.yaml -> 403.""" + from app.core import auth as auth_mod + + def _raise_keyerror(_u: str): + raise KeyError(_u) + + auth_mod.get_role = _raise_keyerror # type: ignore[assignment] + + exists_result = MagicMock() + exists_result.fetchone.return_value = SimpleNamespace(created_by="kopylov") + db.execute.return_value = exists_result + + r = client.post( + "/api/v1/trade-in/lead", + json={"phone": "+79123456789", "consent": True, "estimate_id": str(uuid4())}, + headers={"X-Authenticated-User": "ghost"}, + ) + assert r.status_code == 403, r.text + assert not db.commit.called + + def test_lead_digit_free_phone_422(client: TestClient, db: MagicMock) -> None: # "(()) -- .." проходит regex-маску (только +/скобки/дефисы/точки/пробелы), # но содержит 0 цифр -> должно отклоняться digit-валидатором. From a450aed71b292e88b95d06b527abf4b63b63be47 Mon Sep 17 00:00:00 2001 From: lekss361 Date: Sun, 26 Jul 2026 22:50:39 +0000 Subject: [PATCH 3/6] =?UTF-8?q?fix(tradein/sql):=20=D1=86=D0=B5=D0=BD?= =?UTF-8?q?=D0=B0=20=D0=B2=20=D0=BF=D0=BE=D0=B4=D0=BF=D0=B8=D1=81=D0=B8=20?= =?UTF-8?q?=D0=B4=D0=B5=D0=B4=D1=83=D0=BF=D0=BB=D0=B8=D0=BA=D0=B0=D1=86?= =?UTF-8?q?=D0=B8=D0=B8=20=C2=AB=D0=B4=D0=BE=D0=BB=D0=B8=20=D0=BA=D0=B2?= =?UTF-8?q?=D0=B0=D1=80=D1=82=D0=B8=D1=80=20=D0=B4=D0=BE=D0=BC=D0=B0=20?= =?UTF-8?q?=D0=B2=20=D0=BF=D1=80=D0=BE=D0=B4=D0=B0=D0=B6=D0=B5=C2=BB=20(#2?= =?UTF-8?q?539)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../190_sale_share_price_bucket_signature.sql | 231 ++++++++++++++++++ 1 file changed, 231 insertions(+) create mode 100644 tradein-mvp/backend/data/sql/190_sale_share_price_bucket_signature.sql diff --git a/tradein-mvp/backend/data/sql/190_sale_share_price_bucket_signature.sql b/tradein-mvp/backend/data/sql/190_sale_share_price_bucket_signature.sql new file mode 100644 index 00000000..72a85b5d --- /dev/null +++ b/tradein-mvp/backend/data/sql/190_sale_share_price_bucket_signature.sql @@ -0,0 +1,231 @@ +-- 190_sale_share_price_bucket_signature.sql +-- +-- CONTEXT: аудит МЕРЫ. Числитель v_building_sale_share (мигр. 148) дедупит +-- листинги по сигнатуре (rooms, round(area_m2), floor) — убирает кросс- +-- площадочные дубли одной физической квартиры (avito+cian+domclick). Но в +-- типовом секционном доме 4 РАЗНЫЕ квартиры на одном этаже в разных +-- подъездах имеют ТУ ЖЕ тройку признаков (подъезда в данных нет) → ложно +-- схлопываются в одну. +-- +-- Прод-замер (снят вручную, до этой миграции; не переснят в рамках неё — +-- нет доступа к БД из этой сессии, см. ниже): +-- · без дедупа (активные вторичные, house_id_fk/rooms/area_m2/floor/ +-- price_rub все NOT NULL): 15 424 записи; +-- · текущая сигнатура (rooms, round(area_m2), floor): 11 324 «квартиры» +-- (−4 100 против raw — почти весь эффект дедупа, но и false-merge тоже); +-- · та же сигнатура + price_bucket round(price_rub/100000): 12 497 +-- (+1 173 против текущей, +10.4%) — возвращает часть false-merge'ов. +-- Внутри 3 220 групп, схлопнутых текущей сигнатурой: +-- · 1 252 группы (1 756 записей) имеют РАЗНЫЕ цены — почти наверняка +-- разные квартиры, не кросс-пост; +-- · 233 группы (249 записей) пришли с ОДНОЙ площадки — одна площадка +-- редко публикует одну и ту же квартиру дважды, тоже почти наверняка +-- разные квартиры (см. "residual risk" ниже — этот класс НЕ решается +-- одним лишь price_bucket, если у них к тому же совпала цена). +-- +-- РЕШЕНИЕ ВЛАДЕЛЬЦА ПРОДУКТА: схлопывать записи, только если они совпадают +-- ЕЩЁ И по цене (round(price_rub/100000) — тот же бакет, что уже +-- используется в backend/app/services/estimator.py::_DEDUP_PRICE_BUCKET_RUB +-- для кросс-source физ-дедупа аналогов; ~±0.5% допуска при 21М, ~±2% при +-- 2.5М — см. app/core/config.py:265). Разные квартиры в одном доме +-- почти никогда не стоят ровно одинаково, кросс-пост одного лота — стоит. +-- +-- ЧТО НЕ ВОШЛО (source-distinctness) и почему: +-- Продуктовое решение также просило требовать "с разных площадок". Честно +-- выразить это внутри count(DISTINCT ...) НЕЛЬЗЯ без перестройки CTE +-- listing_agg в двухуровневую агрегацию (сначала GROUP BY house_id + +-- расширенная сигнатура + count(DISTINCT source) per группа, потом per-house +-- SUM(CASE WHEN distinct_sources>=2 THEN 1 ELSE listing_count END)) — это +-- затронуло бы ВСЕ 6 агрегатов CTE (active_secondary, listings_45d, +-- median_price_rub, median_price_per_m2, avg_days_on_market, +-- listings_med_floors), которые сейчас делят один плоский FILTER-паттерн, +-- накопленный за 6 миграций (148-153). Риск регрессии от такой перестройки +-- в одной миграции выше, чем ценность второго guard'а поверх уже сильно +-- сузившего false-merge price_bucket. Берём только price-часть. +-- +-- RESIDUAL RISK (направление ошибки после этой миграции): +-- 1) НЕ решено — 233 группы/249 записей с ОДНОЙ площадкой: если у них +-- внутри группы цена ТОЖЕ совпадает (не проверено, нет прод-доступа +-- в этой сессии), они останутся ложно схлопнуты (недосчёт числителя, +-- sale_share_pct ЗАНИЖЕН для этих домов) — тот же вид ошибки, что и +-- раньше, но у существенно меньшего подмножества. +-- 2) НОВЫЙ вид ошибки, которого не было: настоящий кросс-пост одного +-- физлота, где цена УСПЕЛА измениться между скрейпами разных площадок +-- (снизили цену на avito, domclick ещё не досканирован) — теперь НЕ +-- схлопнется (разные price_bucket) → числитель ЗАВЫШЕН для этих домов. +-- Раньше такая пара схлопывалась верно (без price в ключе). Прямого +-- прод-замера размера этого класса нет. +-- Итого: миграция МЕНЯЕТ баланс ошибки с «сильный недосчёт от false-merge +-- по этажу/подъезду» на «слабый недосчёт по одноплощадочным совпадениям + +-- небольшой new-пересчёт по кросс-постам с ценовым дрейфом» — чище, но не +-- идеально в обе стороны. +-- +-- price_rub NULL/0 handling: listings.price_rub объявлена `bigint NOT NULL` +-- (002_core_tables.sql), но код уже трактует её defensively как потенциально +-- отсутствующую (146/148: `l.price_rub IS NOT NULL` в median FILTER) — то же +-- делаем здесь. price_bucket-компонент = NULL, когда price_rub IS NULL ИЛИ +-- <= 0 (0/отрицательное — sentinel нераспарсенной цены, не реальная цена). +-- Партиально-NULL кортеж (rooms/area/floor есть, price_bucket NULL) +-- count(DISTINCT ROW(...)) трактует как СВОЙ отдельный кортеж (см. NULL- +-- handling мигр. 148) — т.е. листинг без подтверждённой цены НЕ схлопывается +-- ни с чем, считается один. Консервативно (не создаёт ложных совпадений по +-- цене) и совпадает с философией estimator.py::_lot_dedup_components +-- (`if not price: composite = None` → лот не участвует в физ-дедупе). +-- +-- WHAT: в CTE listing_agg расширяем сигнатуру DISTINCT В ОБОИХ числителях +-- (active_secondary, listings_45d) с (rooms, round(area_m2), floor) до +-- (rooms, round(area_m2), floor, price_bucket), где price_bucket = CASE +-- WHEN l.price_rub IS NULL OR l.price_rub <= 0 THEN NULL +-- ELSE round(l.price_rub / 100000.0) END. +-- Остальные 4 агрегата CTE (median_price_rub, median_price_per_m2, +-- avg_days_on_market, listings_med_floors) — НЕ дедуп-based (считают по +-- сырым листингам, прошедшим FILTER), не трогаем. Весь top-level SELECT / +-- WHERE / плаузибилити-гейт (мигр. 145/153) / appended-колонки +-- (zhkh_flat_count, flat_count_source) / гео(≤300м, мигр.150) / floors-guard +-- (±3, мигр.152) — БАЙТ-В-БАЙТ как в мигр. 153. +-- +-- DEPENDENCIES: 143 (view + houses.gar_*), 144 (canon match → gar_flat_count), +-- 145 (плаузибилити-гейт знаменателя), 146 (listings_45d + sale_share_pct_45d +-- + zhkh в COALESCE), 147 (canon strip geo-prefixes), 148 (дедуп +-- кросс-площадочных дублей — база сигнатуры, которую здесь расширяем), 149 +-- (ЖКХ-приоритет знаменателя), 150 (гео-фильтр ≤300м в CTE), 151 (bare-street +-- aliases — view не трогала), 152 (floors-guard ±3), 153 (плаузибилити по +-- листинговой медианной этажности). Базируется на текущем (153) определении +-- view — меняем ТОЛЬКО DISTINCT-выражение в active_secondary/listings_45d. +-- +-- SAFETY / IDEMPOTENCY: CREATE OR REPLACE VIEW ONLY (структура top-level +-- колонок не меняется — те же позиции/типы/имена, что в 153) + COMMENT. +-- Никакого DDL над таблицами. Повторный прогон — no-op (REPLACE на +-- идентичное определение). Деплой-раннер гонит файл через +-- psql -v ON_ERROR_STOP=on БЕЗ --single-transaction → транзакцию открывает +-- САМ файл (BEGIN/COMMIT ниже), как 146/148/149/150/152/153. +-- +-- CONSUMERS (грепнуто по backend+frontend, не тронуты этой миграцией): +-- backend/app/services/buildings_query.py — SELECT * колонок view (список, +-- summary, гистограмма) — тот же набор колонок, не ломается; +-- backend/app/schemas/buildings.py, backend/app/api/v1/buildings.py — +-- Pydantic-схема поверх тех же колонок, не ломается; +-- backend/tests/test_buildings_api.py — тестирует ТОЛЬКО текст SQL-билдеров +-- (строку "FROM v_building_sale_share" и т.п.), не внутренний DISTINCT view +-- → не ломается этой миграцией; +-- ⚠ backend/app/services/buildings_query.py::build_listings_query — ОТДЕЛЬНЫЙ +-- SQL (не читает view), реализует ТУ ЖЕ (rooms, round(area_m2), floor) +-- сигнатуру САМОСТОЯТЕЛЬНО (DISTINCT ON) для панели листингов одного дома. +-- После этой миграции сигнатуры /buildings/sale-share (список, через view, +-- теперь +price_bucket) и /buildings/{id}/listings (панель, старая 3-тройка) +-- РАСХОДЯТСЯ — на детальной панели дома возможен чуть меньший count уникальных +-- квартир, чем active_secondary в списке. НЕ трогаем buildings_query.py в +-- этой миграции (вне границ задачи) — фиксируем расхождение как known +-- follow-up для отдельной задачи. +-- +-- NB по нумерации: последний занятый = 188 (187/188 заняты веб-чатом); +-- следующий свободный sequential = 189 (проверено `ls tradein-mvp/backend/ +-- data/sql | grep '^18'` — 187, 188 заняты, 189 свободен; дубля basename нет). +-- +-- Deploy order: после 188_tg_support_chat_id_scope.sql. + +BEGIN; + +CREATE OR REPLACE VIEW v_building_sale_share AS + WITH listing_agg AS ( + SELECT l.house_id_fk AS house_id, + count(DISTINCT (l.rooms, round(l.area_m2), l.floor, + CASE WHEN l.price_rub IS NULL OR l.price_rub <= 0 THEN NULL + ELSE round(l.price_rub / 100000.0) END)) + FILTER (WHERE l.is_active AND l.listing_segment = 'vtorichka'::text AND (l.geom IS NULL OR hg.geom IS NULL OR ST_DistanceSphere(l.geom, hg.geom) <= 300) AND (l.total_floors IS NULL OR COALESCE(hg.zhkh_floors, hg.total_floors) IS NULL OR abs(l.total_floors - COALESCE(hg.zhkh_floors, hg.total_floors)) <= 3)) AS active_secondary, + count(DISTINCT (l.rooms, round(l.area_m2), l.floor, + CASE WHEN l.price_rub IS NULL OR l.price_rub <= 0 THEN NULL + ELSE round(l.price_rub / 100000.0) END)) FILTER ( + WHERE l.listing_segment = 'vtorichka'::text + AND l.last_seen_at >= (now() - interval '45 days') + AND (l.geom IS NULL OR hg.geom IS NULL OR ST_DistanceSphere(l.geom, hg.geom) <= 300) + AND (l.total_floors IS NULL OR COALESCE(hg.zhkh_floors, hg.total_floors) IS NULL OR abs(l.total_floors - COALESCE(hg.zhkh_floors, hg.total_floors)) <= 3) + ) AS listings_45d, + percentile_cont(0.5::double precision) WITHIN GROUP (ORDER BY (l.price_rub::double precision)) + FILTER (WHERE l.is_active AND l.listing_segment = 'vtorichka'::text AND l.price_rub IS NOT NULL AND (l.geom IS NULL OR hg.geom IS NULL OR ST_DistanceSphere(l.geom, hg.geom) <= 300) AND (l.total_floors IS NULL OR COALESCE(hg.zhkh_floors, hg.total_floors) IS NULL OR abs(l.total_floors - COALESCE(hg.zhkh_floors, hg.total_floors)) <= 3)) AS median_price_rub, + percentile_cont(0.5::double precision) WITHIN GROUP (ORDER BY (l.price_per_m2::double precision)) + FILTER (WHERE l.is_active AND l.listing_segment = 'vtorichka'::text AND l.price_per_m2 IS NOT NULL AND (l.geom IS NULL OR hg.geom IS NULL OR ST_DistanceSphere(l.geom, hg.geom) <= 300) AND (l.total_floors IS NULL OR COALESCE(hg.zhkh_floors, hg.total_floors) IS NULL OR abs(l.total_floors - COALESCE(hg.zhkh_floors, hg.total_floors)) <= 3)) AS median_price_per_m2, + avg(l.days_on_market) + FILTER (WHERE l.is_active AND l.listing_segment = 'vtorichka'::text AND l.days_on_market IS NOT NULL AND (l.geom IS NULL OR hg.geom IS NULL OR ST_DistanceSphere(l.geom, hg.geom) <= 300) AND (l.total_floors IS NULL OR COALESCE(hg.zhkh_floors, hg.total_floors) IS NULL OR abs(l.total_floors - COALESCE(hg.zhkh_floors, hg.total_floors)) <= 3)) AS avg_days_on_market, + percentile_cont(0.5) WITHIN GROUP (ORDER BY l.total_floors) + FILTER (WHERE l.is_active AND l.listing_segment = 'vtorichka'::text AND (l.geom IS NULL OR hg.geom IS NULL OR ST_DistanceSphere(l.geom, hg.geom) <= 300) AND (l.total_floors IS NULL OR COALESCE(hg.zhkh_floors, hg.total_floors) IS NULL OR abs(l.total_floors - COALESCE(hg.zhkh_floors, hg.total_floors)) <= 3)) AS listings_med_floors + FROM listings l + JOIN houses hg ON hg.id = l.house_id_fk + WHERE l.house_id_fk IS NOT NULL + GROUP BY l.house_id_fk + ) + SELECT h.id AS house_id, + h.short_address, + h.full_address, + h.address, + h.lat, + h.lon, + h.year_built, + h.house_type, + h.total_floors, + h.series_name, + h.is_emergency, + COALESCE(h.zhkh_flat_count, h.gar_flat_count, NULLIF(h.total_units, 0), NULLIF(h.flat_count, 0)) AS flat_count_effective, + h.gar_flat_count, + h.gar_match_method, + la.active_secondary, + la.median_price_rub, + la.median_price_per_m2, + la.avg_days_on_market, + CASE + WHEN COALESCE(h.zhkh_flat_count, h.gar_flat_count, NULLIF(h.total_units, 0), NULLIF(h.flat_count, 0)) + >= GREATEST(COALESCE(h.total_floors, 0), COALESCE(la.listings_med_floors, 0)::int, 8) + AND la.active_secondary <= COALESCE(h.zhkh_flat_count, h.gar_flat_count, NULLIF(h.total_units, 0), NULLIF(h.flat_count, 0)) + THEN round(100.0 * la.active_secondary::numeric + / COALESCE(h.zhkh_flat_count, h.gar_flat_count, NULLIF(h.total_units, 0), NULLIF(h.flat_count, 0))::numeric, 1) + ELSE NULL::numeric + END AS sale_share_pct, + la.listings_45d, + CASE + WHEN COALESCE(h.zhkh_flat_count, h.gar_flat_count, NULLIF(h.total_units, 0), NULLIF(h.flat_count, 0)) + >= GREATEST(COALESCE(h.total_floors, 0), COALESCE(la.listings_med_floors, 0)::int, 8) + AND la.listings_45d <= COALESCE(h.zhkh_flat_count, h.gar_flat_count, NULLIF(h.total_units, 0), NULLIF(h.flat_count, 0)) + THEN round(100.0 * la.listings_45d::numeric + / COALESCE(h.zhkh_flat_count, h.gar_flat_count, NULLIF(h.total_units, 0), NULLIF(h.flat_count, 0))::numeric, 1) + ELSE NULL::numeric + END AS sale_share_pct_45d, + h.zhkh_flat_count, + CASE + WHEN h.zhkh_flat_count IS NOT NULL THEN 'zhkh' + WHEN h.gar_flat_count IS NOT NULL THEN 'gar' + WHEN NULLIF(h.total_units, 0) IS NOT NULL THEN 'total_units' + WHEN NULLIF(h.flat_count, 0) IS NOT NULL THEN 'flat_count' + ELSE NULL::text + END AS flat_count_source + FROM houses h + JOIN listing_agg la ON la.house_id = h.id + WHERE h.geom IS NOT NULL AND (la.active_secondary > 0 OR la.listings_45d > 0); + +COMMENT ON VIEW v_building_sale_share IS + 'Per-building rollup вторички для «доли квартир дома в продаже» (мигр. 143; знаменатель — ' + 'ГАР canon-match мигр. 144; 2-й источник ЖКХ + окно 45д мигр. 146; дедуп кросс-площадочных ' + 'дублей мигр. 148 + price_bucket мигр. 189; ЖКХ-приоритет знаменателя мигр. 149; гео-фильтр ' + 'числителя ≤300м мигр. 150). flat_count_effective = ' + 'COALESCE(zhkh_flat_count, gar_flat_count, NULLIF(total_units,0), NULLIF(flat_count,0)) — ' + 'ЖКХ ПРИОРИТЕТ (ГИС ЖКХ точнее ГАР, который дико недосчитывает квартиры в МКД; мигр. 149). ' + 'Колонки zhkh_flat_count (сырой ЖКХ-счёт) + flat_count_source (zhkh|gar|total_units|flat_count|' + 'NULL — какой источник реально дал знаменатель) добавлены для прозрачности. Оба числителя ' + 'считают УНИКАЛЬНЫЕ КВАРТИРЫ по сигнатуре count(DISTINCT (rooms, round(area_m2), floor, ' + 'price_bucket)), где price_bucket = round(price_rub/100000) ИЛИ NULL при price_rub NULL/<=0 ' + '(мигр. 189: одна тройка rooms/area/floor не отличает соседние квартиры на одном этаже в разных ' + 'подъездах — совпадение ЕЩЁ И по цене резко снижает false-merge; NULL-цена не схлопывается ни с ' + 'чем, считается отдельно — та же партиально-NULL философия, что и в мигр. 148 для rooms/area/' + 'floor, и что в estimator.py::_lot_dedup_components для физ-дедупа аналогов). Требование ' + '«разных площадок» из продуктового решения НЕ выражено в SQL (потребовало бы двухуровневой ' + 'агрегации across всех 6 FILTER-агрегатов CTE) — residual risk: однисточниковые группы с ' + 'совпавшей ценой остаются ложно схлопнуты; кросс-посты с ценовым дрейфом между скрейпами ' + 'перестают схлопываться (см. комментарий мигр. 189 в файле). active_secondary = FILTER ' + '(is_active AND vtorichka); listings_45d = FILTER (vtorichka AND last_seen_at>=now()-45d). ' + 'sale_share_pct = active_secondary/denom; sale_share_pct_45d = listings_45d/denom. Оба под ' + 'плаузибилити-гейтом (denom>=GREATEST(total_floors, листинговая-медианная-этажность, 8) AND ' + 'числитель<=denom; мигр. 145 + 153), иначе NULL. Фильтр: geom NOT NULL AND (active_secondary>0 ' + 'OR listings_45d>0) — churn-only дома тоже видны. active_secondary/listings_45d/медианы цены и ' + 'срока считают ТОЛЬКО листинги ≤300м от geom своего дома (мигр. 150) с floors-guard ±3 (мигр. ' + '152). Листинги/дома без geom — кепим. Знаменатель НЕ изменён мигр. 189.'; + +COMMIT; From bcb903cfa2d8a9b536de2af3a4aa26f63d658f7c Mon Sep 17 00:00:00 2001 From: lekss361 Date: Sun, 26 Jul 2026 22:57:41 +0000 Subject: [PATCH 4/6] =?UTF-8?q?fix(tradein/devops):=20=D0=BF=D1=80=D0=BE?= =?UTF-8?q?=D0=B2=D0=B5=D1=80=D0=BA=D0=B0=20=D0=B2=D1=81=D0=B5=D0=B3=D0=BE?= =?UTF-8?q?=20=D1=81=D1=82=D0=B5=D0=BA=D0=B0=20=D0=BF=D0=BE=D1=81=D0=BB?= =?UTF-8?q?=D0=B5=20=D0=B4=D0=B5=D0=BF=D0=BB=D0=BE=D1=8F=20+=20=D0=B7?= =?UTF-8?q?=D0=B0=D0=BF=D0=B0=D1=81=20=D0=BD=D0=B0=20=D0=BE=D1=81=D1=82?= =?UTF-8?q?=D0=B0=D0=BD=D0=BE=D0=B2=D0=BA=D1=83=20(#2540)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .forgejo/workflows/deploy-tradein.yml | 91 +++++++++++++++++++ .../backend/data/sql/_manifest_applied.txt | 28 ++++++ tradein-mvp/docker-compose.prod.yml | 28 ++++++ 3 files changed, 147 insertions(+) diff --git a/.forgejo/workflows/deploy-tradein.yml b/.forgejo/workflows/deploy-tradein.yml index 4ee694eb..d2425ca4 100644 --- a/.forgejo/workflows/deploy-tradein.yml +++ b/.forgejo/workflows/deploy-tradein.yml @@ -592,6 +592,97 @@ jobs: fi echo "→ backend healthy на /health." + # Frontend health check — раньше проверялся ТОЛЬКО backend: сломанный + # фронт (500/белый экран после build, или контейнер упавший на старте) + # помечался успешным деплоем, отката не происходило (см. заголовок + # секции выше). Проверяем изнутри backend-контейнера — он в одной + # tradein-net сети с frontend, и curl там уже есть (в отличие от + # node:alpine рантайм-образа frontend, где нет ни curl, ни wget — + # добавлять их туда ради healthcheck не стали, backend достаточно). + # Путь ОБЯЗАН включать /trade-in: basePath запечён в prod-образ на + # build (NEXT_PUBLIC_BASE_PATH=/trade-in, см. build-frontend job) — + # голый "/" внутри Next вернёт 404, а не что-то живое. "/trade-in/" + # редиректит (307) на /trade-in/v2 — curl -f не считает 3xx ошибкой, + # так что это чистая liveness-проверка (процесс жив и роутит), + # без привязки к тому, что именно сейчас показывает витрина. + frontend_healthy="" + for i in $(seq 1 30); do + if docker compose -p gendesign-tradein -f /opt/gendesign/tradein-mvp/docker-compose.prod.yml \ + exec -T backend curl -fsS http://frontend:3000/trade-in/ >/dev/null 2>&1; then + frontend_healthy="yes"; break + fi + sleep 1 + done + if [ -z "$frontend_healthy" ]; then + echo "ERROR: frontend не ответил на /trade-in/ за 30s — деплой FAILED" + exit 1 + fi + echo "→ frontend healthy на /trade-in/." + + # Browser health check — /health в browser/server.py всегда 200, пока + # жив сам aiohttp-процесс (см. health_handler: "compose НЕ имеет + # healthcheck на browser, только depends_on: service_started" — до + # этой правки browser вообще не проверялся никаким деплой-шагом). + # Это liveness процесса, НЕ readiness camoufox-инстансов конкретных + # источников (те поднимаются лениво на первый /fetch) — но упавший + # при старте контейнер (например, битый образ) здесь ловится сразу, + # а не молча остаётся мёртвым до первого реального /fetch scraper'ом. + browser_healthy="" + for i in $(seq 1 30); do + if docker compose -p gendesign-tradein -f /opt/gendesign/tradein-mvp/docker-compose.prod.yml \ + exec -T backend curl -fsS http://browser:3000/health >/dev/null 2>&1; then + browser_healthy="yes"; break + fi + sleep 1 + done + if [ -z "$browser_healthy" ]; then + echo "ERROR: browser не ответил на /health за 30s — деплой FAILED" + exit 1 + fi + echo "→ browser healthy на /health." + + # tgbot/scraper — те же backend-образ и Dockerfile, но bare python- + # процессы БЕЗ ASGI/HTTP-сервера (см. комментарии в tgbot_main.py / + # scheduler_main.py: "здесь нет ASGI-приложения"), поэтому HTTP- + # healthcheck для них невозможен в принципе. Liveness проверяем по + # состоянию контейнера через docker inspect: упавший на старте + # процесс (например, ImportError в новом коде) restart-policy + # unless-stopped уводит в бесконечный crash-loop — раньше это НИКАК + # не блокировало деплой (маркер писался, даже если tgbot/scraper + # были мертвы). Двойная проверка (running → пауза → снова running) + # снижает шанс поймать контейнер ровно в момент between-restarts + # промежуточного "running" внутри crash-loop. + # tgbot пересоздаётся на КАЖДОМ деплое (безусловно в $SERVICES); + # scraper — только когда SCRAPER_CHANGED (см. блок выше) — поэтому + # проверяем только то, что реально входит в текущий $SERVICES. + for svc in tgbot scraper; do + case " $SERVICES " in + *" $svc "*) ;; + *) continue ;; + esac + container_ok="" + state="unknown" + for i in $(seq 1 15); do + state=$(docker inspect -f '{{.State.Status}}' "tradein-$svc" 2>/dev/null || echo "unknown") + if [ "$state" = "running" ]; then + container_ok="yes"; break + fi + sleep 1 + done + if [ -n "$container_ok" ]; then + sleep 3 + state=$(docker inspect -f '{{.State.Status}}' "tradein-$svc" 2>/dev/null || echo "unknown") + if [ "$state" != "running" ]; then + container_ok="" + fi + fi + if [ -z "$container_ok" ]; then + echo "ERROR: tradein-$svc не в стабильном состоянии running (state='$state') — деплой FAILED" + exit 1 + fi + echo "→ tradein-$svc running." + done + # Cleanup старых образов for repo in ghcr.io/lekss361/gendesign-tradein-backend \ ghcr.io/lekss361/gendesign-tradein-frontend; do diff --git a/tradein-mvp/backend/data/sql/_manifest_applied.txt b/tradein-mvp/backend/data/sql/_manifest_applied.txt index 0d8889dc..7d8d5a0f 100644 --- a/tradein-mvp/backend/data/sql/_manifest_applied.txt +++ b/tradein-mvp/backend/data/sql/_manifest_applied.txt @@ -176,3 +176,31 @@ 169_osm_poi_ekb_local.sql 170_scrape_schedules_seed_osm_poi_ekb_refresh.sql 172_trade_in_leads.sql +173_scrape_proxies_add_domclick_affinity.sql +174_domclick_session_cookies.sql +175_scrape_schedules_seed_domclick_detail_backfill.sql +176_domrf_kapremont.sql +177_deals_city_region.sql +178_deal_city_price_bands.sql +179_scrape_schedules_seed_oblast_city_sweeps.sql +180_seed_sber_freshness_monitor.sql +181_clamp_bad_listing_dates.sql +182_trade_in_leads_consent_proof.sql +183_reenable_deactivate_stale_domklik.sql +184_user_events.sql +185_account_quota_overrides.sql +186_tg_support.sql +# +# 187_web_support_chat.sql / 188_tg_support_chat_id_scope.sql — НАМЕРЕННО НЕ +# добавлены (2026-07-27, devops-аудит). Прецедент из ЭТОГО же репо: +# commit 5eadae1e (fix(tradein/support): address deep-review ... L5) добавил +# и тут же убрал "187_web_support_chat.sql" из этого файла с формулировкой +# "keeping an unmerged migration name out of it preserves the option to +# rename before merge without tripping the "can't rename applied +# migrations" test". Обе миграции — часть веб-чата поддержки (#2532/#2533), +# который на момент этой правки ещё активно дорабатывается в параллельной +# сессии/окне (тот же фиче-набор, соседняя задача). Дописывать их сюда сейчас +# повторило бы именно ту ошибку, которую L5 исправил: заморозить имя файла +# ДО того как он гарантированно осел на проде в финальном виде. Когда фича +# стабилизируется и подтверждено, что 187/188 применены (_schema_migrations +# на проде) — дописать одной строкой в отдельном PR. diff --git a/tradein-mvp/docker-compose.prod.yml b/tradein-mvp/docker-compose.prod.yml index 72dc4c8b..4d346c26 100644 --- a/tradein-mvp/docker-compose.prod.yml +++ b/tradein-mvp/docker-compose.prod.yml @@ -44,6 +44,20 @@ services: # (грубо ~2.5g на каждую доп. параллельную страницу). mem_limit: 2560m memswap_limit: 3g + # stop_grace_period: browser/server.py — bare aiohttp web.run_app(), которое + # само ловит SIGTERM (aiohttp.web.GracefulExit) и даёт себе внутренний + # shutdown_timeout=60s (aiohttp default, здесь не переопределён) на закрытие + # in-flight соединений ПЕРЕД тем как _on_cleanup закроет camoufox-инстансы. + # Без stop_grace_period Docker бы SIGKILL'ил через дефолтные 10s — это убивало + # бы headless-страницу (комментарий выше: /fetch карточка ~15-27s, из + # scraper stop_grace_period #1951) на середине навигации/скрейпа задолго до + # того как aiohttp вообще успеет начать свой собственный graceful-путь. + # 90s = 60s aiohttp shutdown_timeout + ~30s запас на закрытие Firefox- + # инстансов в _on_cleanup (дороже обычного process.kill — camoufox — полноценный + # Firefox-профиль). Не 120s как у scraper/tgbot: у browser нет + # многочасовых unit'ов (единица работы — одна страница, секунды-десятки + # секунд), 120s был бы избыточным запасом без code-level обоснования. + stop_grace_period: 90s logging: *default-logging env_file: - path: ./backend/.env.runtime @@ -97,6 +111,20 @@ services: # наложение export + бэкфилл при 640m было бы впритык) mem_limit: 768m memswap_limit: 768m + # stop_grace_period: uvicorn command ниже не задаёт --timeout-graceful-shutdown, + # т.е. используется uvicorn-дефолт None (безлимитно ждёт in-flight запросы на + # SIGTERM — verified в uvicorn docs, Server.shutdown() без timeout зависает до + # завершения задач). Единственный реальный backstop — Docker'овский + # stop_grace_period; дефолтные 10s SIGKILL'или бы синхронный PDF-экспорт + # (/estimate/{id}/pdf — sync-def route, значит выполняется в Starlette + # threadpool: WeasyPrint write_pdf() + url_fetcher timeout=10s на встроенные + # SVG/шрифты, см. app/services/exporters/trade_in_pdf.py) прямо посреди + # рендера. 60s — щедрый запас над этим (fetcher максимум 10s + рендер + # исторически секунды, не минуты); не 120s как у scraper/tgbot — там код + # сам ограничивает свой drain через _DRAIN_TIMEOUT_S=100s (cooperative + # shutdown handler), здесь такого code-level таймера нет и заводить его + # ради одного PDF-эндпоинта — за рамками этого fix'а. + stop_grace_period: 60s logging: *default-logging # Prod: uvicorn БЕЗ --reload (Dockerfile CMD несёт --reload только для dev hot-reload, # где app/ bind-mount'ится). В prod --reload = лишний WatchFiles-наблюдатель + риск From ca4641134643ef14ab75e3d3ab6ed53728dbabf8 Mon Sep 17 00:00:00 2001 From: lekss361 Date: Sun, 26 Jul 2026 23:04:43 +0000 Subject: [PATCH 5/6] =?UTF-8?q?fix(tradein/tests):=20=D1=82=D0=B5=D1=81?= =?UTF-8?q?=D1=82=D1=8B=20=D0=B0=D0=B2=D1=82=D0=BE=D1=80=D0=B8=D0=B7=D0=B0?= =?UTF-8?q?=D1=86=D0=B8=D0=B8=20=D0=BF=D1=80=D0=BE=D0=B2=D0=B5=D1=80=D1=8F?= =?UTF-8?q?=D1=8E=D1=82=20=D0=BD=D0=B0=D1=81=D1=82=D0=BE=D1=8F=D1=89=D0=B8?= =?UTF-8?q?=D0=B9=20guard=20+=20=D1=80=D0=B5=D0=B0=D0=BB=D1=8C=D0=BD=D1=8B?= =?UTF-8?q?=D0=B9=20=D1=80=D0=B5=D0=BD=D0=B4=D0=B5=D1=80=20PDF=20(#2541)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tradein-mvp/backend/app/core/rbac.py | 136 ++++++++++++ tradein-mvp/backend/app/main.py | 110 +-------- tradein-mvp/backend/tests/conftest.py | 18 ++ .../tests/test_internal_auth_secret.py | 63 +----- .../backend/tests/test_pdf_real_render.py | 209 ++++++++++++++++++ tradein-mvp/backend/tests/test_rbac.py | 72 ++---- 6 files changed, 396 insertions(+), 212 deletions(-) create mode 100644 tradein-mvp/backend/app/core/rbac.py create mode 100644 tradein-mvp/backend/tests/conftest.py create mode 100644 tradein-mvp/backend/tests/test_pdf_real_render.py diff --git a/tradein-mvp/backend/app/core/rbac.py b/tradein-mvp/backend/app/core/rbac.py new file mode 100644 index 00000000..0596de39 --- /dev/null +++ b/tradein-mvp/backend/app/core/rbac.py @@ -0,0 +1,136 @@ +"""RBAC guard middleware — extracted from ``app/main.py``. + +Historically ``rbac_guard`` lived inline in ``app/main.py`` and the test suite +(``tests/test_rbac.py``, ``tests/test_internal_auth_secret.py``) kept a +hand-maintained *copy* of it, labelled "MIRROR of app.main — keep in sync +manually". The copy drifted: it was missing the #2213 +``X-Internal-Auth-Secret`` defense-in-depth check that the real guard has, +so a regression in that check would NOT have failed CI. + +This module holds the real guard with no DB/lifespan/scheduler side effects +(only ``app.core.auth`` + ``app.core.config``, both side-effect-free at +import time beyond requiring ``DATABASE_URL`` in the environment for +``Settings()``). ``app/main.py`` and the test apps both import THIS module, +so tests exercise the exact production code path instead of a copy that can +silently fall out of sync. +""" + +from __future__ import annotations + +import logging +import re +import secrets +from collections.abc import Awaitable, Callable + +from fastapi import Request +from fastapi.responses import JSONResponse, Response + +from app.core.auth import get_role, is_path_allowed +from app.core.config import settings + +logger = logging.getLogger(__name__) + +# RBAC: defense-in-depth поверх Caddy basic_auth + X-Authenticated-User +# (см. app/core/auth.py + auth/roles.yaml). Правила: +# 1) Любой non-public path требует X-Authenticated-User — иначе 401. +# 2) Юзер должен быть в roles.yaml — иначе 403 («неизвестный юзер ничего +# не видит» — decided 2026-05-25). +# 3) /api/v1/admin/* (= внешний /trade-in/api/v1/admin/* после Caddy +# `uri strip_prefix /trade-in`) — только role=admin, иначе 403. +# Public paths без auth (/health, /docs, /openapi.json) пропускаем — +# X-Authenticated-User там не приходит из Caddy. +_ADMIN_API_RE = re.compile(r"^/api/v1/admin/") +_PUBLIC_PATHS = frozenset({"/health", "/docs", "/redoc", "/openapi.json"}) +# #R2-H3: Caddy срезает внешний префикс /trade-in (uri strip_prefix) перед +# tradein-backend, а globs в roles.yaml — ВНЕШНИЕ (/trade-in/api/v1/**). Для +# scope-проверки восстанавливаем внешний путь. +_EXTERNAL_PREFIX = "/trade-in" +# Bootstrap-пути, доступные ЛЮБОМУ известному юзеру независимо от роли: /me отдаёт +# роль (expired → trial-экран), /brand/* — брендинг login/trial-экрана. Без них +# expired (roles.yaml paths:[] deny:/**) не получил бы роль и не увидел trial-экран. +_RBAC_BOOTSTRAP_EXEMPT = ("/api/v1/me", "/api/v1/brand") + + +async def rbac_guard( + request: Request, + call_next: Callable[[Request], Awaitable[Response]], +) -> Response: + path = request.url.path + if path in _PUBLIC_PATHS: + return await call_next(request) + + username = request.headers.get("X-Authenticated-User") + if not username: + return JSONResponse( + status_code=401, + content={"detail": "no authenticated user (Caddy basic_auth required)"}, + ) + + # #2213 defense-in-depth: если общий секрет задан — запрос с X-Authenticated-User + # ОБЯЗАН нести валидный X-Internal-Auth-Secret (его добавляет Caddy из env). + # Иначе это подделка заголовка мимо Caddy (напр. изнутри gendesign_shared) → 401. + # Constant-time compare против timing-атак. Пусто = защита не активна (fail-open). + secret = settings.tradein_internal_auth_secret + if secret: + provided = request.headers.get("X-Internal-Auth-Secret", "") + if not secrets.compare_digest(provided, secret): + logger.warning( + "RBAC: X-Authenticated-User=%r без валидного X-Internal-Auth-Secret " + "на %s — возможная подделка заголовка мимо Caddy", + username, + path, + ) + return JSONResponse( + status_code=401, + content={"detail": "invalid or missing internal auth secret"}, + ) + + try: + role = get_role(username) + except KeyError: + logger.warning("RBAC: unknown user %r tried %s", username, path) + return JSONResponse( + status_code=403, + content={"detail": "user not in roles config"}, + ) + + if _ADMIN_API_RE.match(path) and role != "admin": + logger.info("RBAC: blocked %s (role=%s) from %s", username, role, path) + return JSONResponse( + status_code=403, + content={"detail": "admin only"}, + ) + + # #R2-H3: энфорсим roles.yaml scope (paths/deny) для ВСЕХ non-admin путей, а не + # только /admin/*. Иначе revoked (role=expired, paths:[] deny:/**) или узко- + # скоупленный аккаунт достаёт non-admin API (напр. POST /api/v1/search — + # экспорт листингов), который roles.yaml ему запрещает. Bootstrap-пути (/me, + # /brand) исключены выше по списку. roles.yaml globs внешние → восстанавливаем + # внешний путь (Caddy срезал /trade-in). На сбой парса — fail-open + громкий + # лог: не лочим платящего pilot из-за конфиг-бага (admin-гейт выше остаётся). + if not path.startswith(_RBAC_BOOTSTRAP_EXEMPT): + external_path = _EXTERNAL_PREFIX + path + try: + allowed = is_path_allowed(role, external_path) + except Exception: + logger.exception( + "RBAC scope-check raised for %s %s (ext=%s) — fail-open", + username, + path, + external_path, + ) + allowed = True + if not allowed: + logger.info( + "RBAC: scope-blocked %s (role=%s) from %s (ext=%s)", + username, + role, + path, + external_path, + ) + return JSONResponse( + status_code=403, + content={"detail": "forbidden for role"}, + ) + + return await call_next(request) diff --git a/tradein-mvp/backend/app/main.py b/tradein-mvp/backend/app/main.py index 7ccf4b12..65fb5098 100644 --- a/tradein-mvp/backend/app/main.py +++ b/tradein-mvp/backend/app/main.py @@ -8,15 +8,12 @@ from __future__ import annotations import logging import os -import re -import secrets -from collections.abc import AsyncGenerator, Awaitable, Callable +from collections.abc import AsyncGenerator from contextlib import asynccontextmanager import sentry_sdk -from fastapi import FastAPI, Request +from fastapi import FastAPI from fastapi.middleware.cors import CORSMiddleware -from fastapi.responses import JSONResponse, Response from sentry_sdk.integrations.fastapi import FastApiIntegration from sentry_sdk.integrations.httpx import HttpxIntegration from sentry_sdk.integrations.logging import LoggingIntegration @@ -35,11 +32,11 @@ from app.api.v1 import ( support, trade_in, ) -from app.core.auth import get_role, is_path_allowed from app.core.config import settings from app.core.db import SessionLocal from app.core.fdw import ensure_fdw_user_mapping from app.core.ratelimit import RateLimitMiddleware +from app.core.rbac import rbac_guard from app.core.request_audit import RequestAuditMiddleware from app.observability.sentry_scrub import scrub_pii_event @@ -138,104 +135,9 @@ app = FastAPI( # не видит» — decided 2026-05-25). # 3) /api/v1/admin/* (= внешний /trade-in/api/v1/admin/* после Caddy # `uri strip_prefix /trade-in`) — только role=admin, иначе 403. -# Public paths без auth (/health, /docs, /openapi.json) пропускаем — -# X-Authenticated-User там не приходит из Caddy. -_ADMIN_API_RE = re.compile(r"^/api/v1/admin/") -_PUBLIC_PATHS = frozenset({"/health", "/docs", "/redoc", "/openapi.json"}) -# #R2-H3: Caddy срезает внешний префикс /trade-in (uri strip_prefix) перед -# tradein-backend, а globs в roles.yaml — ВНЕШНИЕ (/trade-in/api/v1/**). Для -# scope-проверки восстанавливаем внешний путь. -_EXTERNAL_PREFIX = "/trade-in" -# Bootstrap-пути, доступные ЛЮБОМУ известному юзеру независимо от роли: /me отдаёт -# роль (expired → trial-экран), /brand/* — брендинг login/trial-экрана. Без них -# expired (roles.yaml paths:[] deny:/**) не получил бы роль и не увидел trial-экран. -_RBAC_BOOTSTRAP_EXEMPT = ("/api/v1/me", "/api/v1/brand") - - -@app.middleware("http") -async def rbac_guard( - request: Request, - call_next: Callable[[Request], Awaitable[Response]], -) -> Response: - path = request.url.path - if path in _PUBLIC_PATHS: - return await call_next(request) - - username = request.headers.get("X-Authenticated-User") - if not username: - return JSONResponse( - status_code=401, - content={"detail": "no authenticated user (Caddy basic_auth required)"}, - ) - - # #2213 defense-in-depth: если общий секрет задан — запрос с X-Authenticated-User - # ОБЯЗАН нести валидный X-Internal-Auth-Secret (его добавляет Caddy из env). - # Иначе это подделка заголовка мимо Caddy (напр. изнутри gendesign_shared) → 401. - # Constant-time compare против timing-атак. Пусто = защита не активна (fail-open). - secret = settings.tradein_internal_auth_secret - if secret: - provided = request.headers.get("X-Internal-Auth-Secret", "") - if not secrets.compare_digest(provided, secret): - logger.warning( - "RBAC: X-Authenticated-User=%r без валидного X-Internal-Auth-Secret " - "на %s — возможная подделка заголовка мимо Caddy", - username, - path, - ) - return JSONResponse( - status_code=401, - content={"detail": "invalid or missing internal auth secret"}, - ) - - try: - role = get_role(username) - except KeyError: - logger.warning("RBAC: unknown user %r tried %s", username, path) - return JSONResponse( - status_code=403, - content={"detail": "user not in roles config"}, - ) - - if _ADMIN_API_RE.match(path) and role != "admin": - logger.info("RBAC: blocked %s (role=%s) from %s", username, role, path) - return JSONResponse( - status_code=403, - content={"detail": "admin only"}, - ) - - # #R2-H3: энфорсим roles.yaml scope (paths/deny) для ВСЕХ non-admin путей, а не - # только /admin/*. Иначе revoked (role=expired, paths:[] deny:/**) или узко- - # скоупленный аккаунт достаёт non-admin API (напр. POST /api/v1/search — - # экспорт листингов), который roles.yaml ему запрещает. Bootstrap-пути (/me, - # /brand) исключены выше по списку. roles.yaml globs внешние → восстанавливаем - # внешний путь (Caddy срезал /trade-in). На сбой парса — fail-open + громкий - # лог: не лочим платящего pilot из-за конфиг-бага (admin-гейт выше остаётся). - if not path.startswith(_RBAC_BOOTSTRAP_EXEMPT): - external_path = _EXTERNAL_PREFIX + path - try: - allowed = is_path_allowed(role, external_path) - except Exception: - logger.exception( - "RBAC scope-check raised for %s %s (ext=%s) — fail-open", - username, - path, - external_path, - ) - allowed = True - if not allowed: - logger.info( - "RBAC: scope-blocked %s (role=%s) from %s (ext=%s)", - username, - role, - path, - external_path, - ) - return JSONResponse( - status_code=403, - content={"detail": "forbidden for role"}, - ) - - return await call_next(request) +# Guard body живёт в app/core/rbac.py (без DB/lifespan side effects), чтобы +# тесты могли импортировать РЕАЛЬНЫЙ guard вместо hand-maintained копии. +app.middleware("http")(rbac_guard) app.add_middleware( diff --git a/tradein-mvp/backend/tests/conftest.py b/tradein-mvp/backend/tests/conftest.py new file mode 100644 index 00000000..c6660a94 --- /dev/null +++ b/tradein-mvp/backend/tests/conftest.py @@ -0,0 +1,18 @@ +"""Repo-wide test config for tradein-mvp/backend. + +Currently only registers custom pytest markers so they don't emit +PytestUnknownMarkWarning when used (`--strict-markers` is not enabled in +pyproject.toml, so an unregistered marker would only warn, not fail — this +just keeps output clean and documents intent in one place). +""" + +from __future__ import annotations + + +def pytest_configure(config) -> None: + config.addinivalue_line( + "markers", + "pdf_render: real (non-mocked) WeasyPrint render — needs native " + "Pango/cairo/GObject libs, self-skips where unavailable (see " + "tests/test_pdf_real_render.py docstring for how to run it for real).", + ) diff --git a/tradein-mvp/backend/tests/test_internal_auth_secret.py b/tradein-mvp/backend/tests/test_internal_auth_secret.py index bf94d7d8..0c301d54 100644 --- a/tradein-mvp/backend/tests/test_internal_auth_secret.py +++ b/tradein-mvp/backend/tests/test_internal_auth_secret.py @@ -2,13 +2,16 @@ Backend доверяет X-Authenticated-User (его ставит Caddy). На общей docker-сети gendesign_shared любой контейнер мог бы отправить поддельный -`X-Authenticated-User: admin` напрямую на tradein-backend:8000. Общий секрет -X-Internal-Auth-Secret закрывает дыру: если TRADEIN_INTERNAL_AUTH_SECRET задан, -каждый запрос с X-Authenticated-User обязан нести валидный секрет (constant-time), +`X-Authenticated-User: admin` напрямую на tradein-backend:8000, минуя Caddy. +Общий секрет X-Internal-Auth-Secret закрывает дыру: если TRADEIN_INTERNAL_AUTH_SECRET +задан, каждый запрос с X-Authenticated-User обязан нести валидный секрет (constant-time), иначе 401. Пусто = fail-open (backward-compat до провижининга). -MIRROR of rbac_guard из app/main.py — включая #2213 secret-gate. Держим копию -здесь (как test_rbac.py), чтобы не тянуть тяжёлый app.main (lifespan/DB/scheduler). +Использует РЕАЛЬНЫЙ rbac_guard (app/core/rbac.py) — тот же, что регистрирует +app/main.py в проде. Раньше здесь была hand-maintained копия ("MIRROR of +rbac_guard из app/main.py"); соседний test_rbac.py держал СВОЮ отдельную +копию, которая успела отстать (потеряла именно этот secret-gate) — регрессия +в реальном guard'е могла бы пройти CI незамеченной. См. app/core/rbac.py. """ from __future__ import annotations @@ -17,20 +20,13 @@ import os os.environ.setdefault("DATABASE_URL", "postgresql+psycopg://test:test@localhost:5432/test") -import re -import secrets -from collections.abc import Awaitable, Callable - import pytest -from fastapi import FastAPI, Request -from fastapi.responses import JSONResponse, Response +from fastapi import FastAPI from fastapi.testclient import TestClient from app.core import auth as auth_mod from app.core import config - -_ADMIN_API_RE = re.compile(r"^/api/v1/admin/") -_PUBLIC_PATHS = frozenset({"/health", "/docs", "/redoc", "/openapi.json"}) +from app.core.rbac import rbac_guard @pytest.fixture(autouse=True) @@ -39,44 +35,9 @@ def _reset_auth_cache() -> None: def _build_test_app() -> FastAPI: - """Копия rbac_guard из app/main.py (с #2213 secret-gate).""" + """Test app используя РЕАЛЬНЫЙ rbac_guard (с #2213 secret-gate).""" app = FastAPI() - - @app.middleware("http") - async def rbac_guard( - request: Request, - call_next: Callable[[Request], Awaitable[Response]], - ) -> Response: - path = request.url.path - if path in _PUBLIC_PATHS: - return await call_next(request) - - username = request.headers.get("X-Authenticated-User") - if not username: - return JSONResponse( - status_code=401, - content={"detail": "no authenticated user (Caddy basic_auth required)"}, - ) - - secret = config.settings.tradein_internal_auth_secret - if secret: - provided = request.headers.get("X-Internal-Auth-Secret", "") - if not secrets.compare_digest(provided, secret): - return JSONResponse( - status_code=401, - content={"detail": "invalid or missing internal auth secret"}, - ) - - try: - role = auth_mod.get_role(username) - except KeyError: - return JSONResponse( - status_code=403, - content={"detail": "user not in roles config"}, - ) - if _ADMIN_API_RE.match(path) and role != "admin": - return JSONResponse(status_code=403, content={"detail": "admin only"}) - return await call_next(request) + app.middleware("http")(rbac_guard) @app.get("/api/v1/ping") async def ping() -> dict: diff --git a/tradein-mvp/backend/tests/test_pdf_real_render.py b/tradein-mvp/backend/tests/test_pdf_real_render.py new file mode 100644 index 00000000..9ed70cf5 --- /dev/null +++ b/tradein-mvp/backend/tests/test_pdf_real_render.py @@ -0,0 +1,209 @@ +"""Real (non-mocked) WeasyPrint render invariants for the Trade-In PDF report. + +tests/test_pdf_security.py exercises only the HTML-builder layer with +WeasyPrint stubbed out (``sys.modules['weasyprint'] = MagicMock()``) — by +design, so those tests run everywhere without needing WeasyPrint's native +GTK/Pango/cairo libs. That design has a blind spot: it can NEVER catch a real +WeasyPrint pagination regression — e.g. the bug fixed in commit 42a50cf8 +("running @page header/footer → ровно 4 страницы (без пустых)"), where the +persistent running header/footer produced an extra TRAILING BLANK 5th page +instead of the documented "Структура отчёта — 4 страницы" invariant (see +app/services/exporters/trade_in_pdf.py module docstring). A mocked +WeasyPrint can never lay out/paginate anything, so it structurally cannot see +that class of bug — only a real render can. + +Requires WeasyPrint's native dependencies (Pango/cairo/GObject). These are +NOT installed in this dev sandbox (Windows, no libgobject-2.0-0) and NOT +installed on the bare ``ubuntu-latest`` runner used by +.forgejo/workflows/ci-tradein.yml (no apt-get step there — see that file). +This whole module self-skips via ``pytest.skip(..., allow_module_level=True)`` +the moment the real `import weasyprint` fails for ANY reason (missing native +lib, etc.), so it is a harmless no-op everywhere it can't actually run. + +How to run it for real: + - Inside the prod/dev docker image — the `runner` stage of + tradein-mvp/backend/Dockerfile installs libcairo2 + libpango-1.0-0 + + libpangoft2-1.0-0 + fonts-dejavu-core, so WeasyPrint's native deps are + present there: + docker exec tradein-backend python -m pytest -q -m pdf_render \ + tests/test_pdf_real_render.py + - Locally on a Linux box with WeasyPrint's system deps installed (see + https://doc.courtbouillon.org/weasyprint/stable/first_steps.html): + uv run pytest -q -m pdf_render tests/test_pdf_real_render.py + - Marked ``@pytest.mark.pdf_render`` (registered in tests/conftest.py) so it + can be explicitly selected/excluded once a CI runner with the native libs + exists; today ci-tradein.yml's `ubuntu-latest` runner doesn't have them, + so this module simply self-skips there — nothing to deselect. +""" + +from __future__ import annotations + +import os +import sys + +os.environ.setdefault("DATABASE_URL", "postgresql+psycopg://test:test@localhost:5432/test") + +# test_pdf_security.py deliberately does `sys.modules['weasyprint'] = MagicMock()` +# for its own unit tests. sys.modules is a process-global cache: if that module +# was imported before this one in the same pytest run, `import weasyprint` below +# would silently return the Mock instead of the real package. Purge any +# existing stub before attempting the real import — independent of collection +# order across test modules. +for _name in [n for n in sys.modules if n == "weasyprint" or n.startswith("weasyprint.")]: + del sys.modules[_name] + +import pytest # noqa: E402 + +try: + import weasyprint +except Exception as exc: # pragma: no cover - env-dependent (native GTK/Pango/cairo libs) + pytest.skip( + f"WeasyPrint native deps unavailable, skipping real-render tests: {exc}", + allow_module_level=True, + ) + +pytestmark = pytest.mark.pdf_render + +import unittest.mock as _mock # noqa: E402 +from datetime import UTC, datetime, timedelta # noqa: E402 +from uuid import uuid4 # noqa: E402 + +from app.schemas.trade_in import AggregatedEstimate # noqa: E402 +from app.services.brand import Brand # noqa: E402 +from app.services.exporters import trade_in_pdf as mod # noqa: E402 + +_GENERIC = Brand( + slug="generic", + name="Trade-In", + logo_url=None, + primary_color="#1d4ed8", + accent_color="#f59e0b", + footer_text=None, + pdf_disclaimer=None, +) + +_SNAPSHOT = { + "address": "Екатеринбург, ул. Ленина, 1", + "area_m2": 50.0, + "rooms": 2, + "floor": 3, + "total_floors": 9, + "year_built": 2010, + "house_type": "panel", + "repair_state": "standard", + "has_balcony": True, +} + + +def _estimate(**overrides) -> AggregatedEstimate: + base = dict( + estimate_id=uuid4(), + median_price_rub=10_000_000, + range_low_rub=9_000_000, + range_high_rub=11_000_000, + median_price_per_m2=200_000, + confidence="high", + n_analogs=15, + period_months=24, + analogs=[], + actual_deals=[], + expires_at=datetime.now(UTC) + timedelta(days=30), + ) + base.update(overrides) + return AggregatedEstimate(**base) + + +def _zero_estimate(**overrides) -> AggregatedEstimate: + """Оценка с median=0 — insufficient_data=True (одностраничный empty-state).""" + base = dict( + estimate_id=uuid4(), + median_price_rub=0, + range_low_rub=0, + range_high_rub=0, + median_price_per_m2=0, + confidence="low", + n_analogs=0, + period_months=24, + analogs=[], + actual_deals=[], + expires_at=datetime.now(UTC) + timedelta(days=30), + ) + base.update(overrides) + return AggregatedEstimate(**base) + + +def _render_real_document(estimate, snapshot, brand): + """Call the REAL generate_trade_in_pdf (no mocking of WeasyPrint), capturing + the actual weasyprint.HTML/CSS instances + html string it constructs via + thin capturing subclasses — so we can additionally call .render() on the + exact same HTML instance to get a weasyprint.Document (whose .pages is the + public, documented page-count API), without duplicating trade_in_pdf.py's + own html_str/css_str assembly logic here (that would drift out of sync + with the real function, defeating the point of this test). + + Returns (document, pdf_bytes, html_string). + """ + html_instances: list[weasyprint.HTML] = [] + css_instances: list[weasyprint.CSS] = [] + html_strings: list[str] = [] + + class _CapturingHTML(weasyprint.HTML): + def __init__(self, *a, **kw): + html_strings.append(kw.get("string") if "string" in kw else (a[0] if a else None)) + super().__init__(*a, **kw) + html_instances.append(self) + + class _CapturingCSS(weasyprint.CSS): + def __init__(self, *a, **kw): + super().__init__(*a, **kw) + css_instances.append(self) + + with ( + _mock.patch.object(weasyprint, "HTML", _CapturingHTML), + _mock.patch.object(weasyprint, "CSS", _CapturingCSS), + ): + pdf_bytes = mod.generate_trade_in_pdf(estimate, snapshot, brand=brand) + + assert html_instances, "HTML() was never constructed by generate_trade_in_pdf" + assert css_instances, "CSS() was never constructed by generate_trade_in_pdf" + document = html_instances[-1].render(stylesheets=[css_instances[-1]]) + return document, pdf_bytes, html_strings[-1] + + +def test_real_pdf_has_exactly_4_pages_no_trailing_blank() -> None: + """Regression for commit 42a50cf8: the running @page header/footer must + NOT produce a 5th trailing blank page. trade_in_pdf.py's own module + docstring documents "Структура отчёта — 4 страницы" as the fixed + cover/listings/deals/offer composition — that count IS the authoritative + "no empty pages" invariant for this report (any blank page shows up as an + extra page beyond these 4 named sections).""" + est = _estimate(n_analogs=12, sources_used=["avito"]) + document, pdf_bytes, _html_str = _render_real_document(est, _SNAPSHOT, _GENERIC) + assert len(document.pages) == 4, ( + f"expected exactly 4 pages (cover/listings/deals/offer), got " + f"{len(document.pages)} — likely a trailing/leading blank-page regression" + ) + assert pdf_bytes.startswith(b"%PDF-"), "write_pdf must produce a real PDF" + + +def test_real_pdf_insufficient_data_is_single_page() -> None: + """insufficient_data=True → one-page empty-state, not the 4-page report.""" + est = _zero_estimate() + document, _pdf_bytes, html_str = _render_real_document(est, _SNAPSHOT, _GENERIC) + assert len(document.pages) == 1 + assert "Недостаточно данных" in html_str + + +def test_real_pdf_key_blocks_present_exactly_once() -> None: + """Key section headings for each of the 4 pages are present in the actual + composed HTML that WeasyPrint rendered (integration-level check — the + mocked tests in test_pdf_security.py already verify each builder function + in isolation; this catches a composition bug where wiring them together + in generate_trade_in_pdf silently drops or duplicates a section).""" + est = _estimate(n_analogs=12, sources_used=["avito"]) + document, _pdf_bytes, html_str = _render_real_document(est, _SNAPSHOT, _GENERIC) + assert len(document.pages) == 4 + markers = ["РЫНОК КВАРТИР", "ФОРМИРОВАНИЕ ВЫКУПНОЙ СТОИМОСТИ"] + for marker in markers: + found = html_str.count(marker) + assert found == 1, f"expected exactly one {marker!r} block, found {found}" diff --git a/tradein-mvp/backend/tests/test_rbac.py b/tradein-mvp/backend/tests/test_rbac.py index 74c393e3..39c50e51 100644 --- a/tradein-mvp/backend/tests/test_rbac.py +++ b/tradein-mvp/backend/tests/test_rbac.py @@ -1,7 +1,5 @@ """RBAC unit + integration tests for tradein backend. -MIRROR of backend/tests/test_rbac.py — kept in sync manually. - Coverage: - get_role / get_user_scope happy + error paths - is_path_allowed glob semantics (admin everywhere, pilot blocked from /admin/**) @@ -14,22 +12,29 @@ Coverage: Тесты используют изолированный FastAPI app (см. _build_test_app), чтобы не тянуть тяжёлые модули из app.main (lifespan task + DB session + scheduler). -RBAC middleware и /me router импортируются напрямую — это даёт нам ровно -тот же поведение что в проде. +Guard больше НЕ мокается/копируется вручную — импортируется тот же +``app.core.rbac.rbac_guard``, что регистрирует app/main.py в проде. Раньше +здесь была hand-maintained "MIRROR of app.main" копия, которая незаметно +отстала (не имела #2213 X-Internal-Auth-Secret check) — правки реального +guard'а тесты бы не заметили. См. app/core/rbac.py. """ from __future__ import annotations -import re -from collections.abc import Awaitable, Callable +import os + +# Settings (app.core.config, импортируемый через app.core.rbac) требует +# DATABASE_URL на конструирование — stub перед любым app-импортом (тот же +# паттерн что и в остальных tests/*.py). +os.environ.setdefault("DATABASE_URL", "postgresql+psycopg://test:test@localhost:5432/test") import pytest -from fastapi import FastAPI, Request -from fastapi.responses import JSONResponse, Response +from fastapi import FastAPI from fastapi.testclient import TestClient from app.api.v1 import me as me_router from app.core import auth as auth_mod +from app.core.rbac import rbac_guard @pytest.fixture(autouse=True) @@ -38,60 +43,13 @@ def _reset_auth_cache() -> None: # --------------------------------------------------------------------------- -# Test app — копия rbac_guard из app/main.py. +# Test app — использует РЕАЛЬНЫЙ rbac_guard (app/core/rbac.py), а не копию. # --------------------------------------------------------------------------- -_ADMIN_API_RE = re.compile(r"^/api/v1/admin/") -_PUBLIC_PATHS = frozenset({"/health", "/docs", "/redoc", "/openapi.json"}) -# MIRROR of app.main (#R2-H3): keep in sync with the real rbac_guard. -_EXTERNAL_PREFIX = "/trade-in" -_RBAC_BOOTSTRAP_EXEMPT = ("/api/v1/me", "/api/v1/brand") - - def _build_test_app() -> FastAPI: app = FastAPI() - - @app.middleware("http") - async def rbac_guard( - request: Request, - call_next: Callable[[Request], Awaitable[Response]], - ) -> Response: - path = request.url.path - if path in _PUBLIC_PATHS: - return await call_next(request) - - username = request.headers.get("X-Authenticated-User") - if not username: - return JSONResponse( - status_code=401, - content={"detail": "no authenticated user (Caddy basic_auth required)"}, - ) - try: - role = auth_mod.get_role(username) - except KeyError: - return JSONResponse( - status_code=403, - content={"detail": "user not in roles config"}, - ) - if _ADMIN_API_RE.match(path) and role != "admin": - return JSONResponse( - status_code=403, - content={"detail": "admin only"}, - ) - # #R2-H3 mirror: enforce roles.yaml scope on non-bootstrap paths. - if not path.startswith(_RBAC_BOOTSTRAP_EXEMPT): - external_path = _EXTERNAL_PREFIX + path - try: - allowed = auth_mod.is_path_allowed(role, external_path) - except Exception: - allowed = True - if not allowed: - return JSONResponse( - status_code=403, - content={"detail": "forbidden for role"}, - ) - return await call_next(request) + app.middleware("http")(rbac_guard) app.include_router(me_router.router, prefix="/api/v1", tags=["me"]) From 036ff84eaa8452c1a1437bdff08b7c3644de192e Mon Sep 17 00:00:00 2001 From: lekss361 Date: Sun, 26 Jul 2026 23:16:57 +0000 Subject: [PATCH 6/6] =?UTF-8?q?fix(tradein/tgbot):=20=D0=BE=D0=B3=D1=80?= =?UTF-8?q?=D0=B0=D0=BD=D0=B8=D1=87=D0=B5=D0=BD=D0=B8=D0=B5=20=D1=87=D0=B0?= =?UTF-8?q?=D1=81=D1=82=D0=BE=D1=82=D1=8B=20=D0=BD=D0=B0=20=D0=BE=D1=82?= =?UTF-8?q?=D0=BF=D1=80=D0=B0=D0=B2=D0=B8=D1=82=D0=B5=D0=BB=D1=8F=20=D0=B2?= =?UTF-8?q?=20=D0=BC=D0=BE=D1=81=D1=82=D0=B5=20=D0=BF=D0=BE=D0=B4=D0=B4?= =?UTF-8?q?=D0=B5=D1=80=D0=B6=D0=BA=D0=B8=20(#2543)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../backend/app/services/tgbot/bridge.py | 63 ++++++++++ .../tests/services/tgbot/test_bridge.py | 113 ++++++++++++++++++ 2 files changed, 176 insertions(+) diff --git a/tradein-mvp/backend/app/services/tgbot/bridge.py b/tradein-mvp/backend/app/services/tgbot/bridge.py index 6cbc8f3a..fc49aa68 100644 --- a/tradein-mvp/backend/app/services/tgbot/bridge.py +++ b/tradein-mvp/backend/app/services/tgbot/bridge.py @@ -36,6 +36,21 @@ (entrypoint), не здесь. E) /start клиенту → короткое приветствие МЕРЫ, без зеркалирования в топик (команда — не содержательное обращение, не должна засорять топик). + F) Флуд-лимит на отправителя (низкий приоритет, per-chat_id): воркер + long-polling однопоточный и обрабатывает апдейты СТРОГО последовательно, а + Telegram ограничивает саму support-группу ~20 сообщениями/минуту — ОДНИМ + бюджетом на ВСЕХ клиентов разом (зеркала + шапки + ответы оператора). + Превышение — 429 с ожиданием 30-60с, на которые воркер не может обработать + НИ ОДНОГО следующего апдейта — один флудящий клиент подвешивает доставку + всем остальным. `_flood_limiter` (тот же `SlidingWindowLimiter`, что и + веб-чат поддержки, ключ — TELEGRAM chat_id) режет per-sender поток заметно + ниже группового лимита; сообщения сверх бюджета НЕ зеркалируются (иначе + сам факт мирроринга уже съедает групповой бюджет, который мы и защищаем) и + НЕ пишутся в tg_support_messages (нечего маршрутизировать без + topic_message_id). Клиент получает уведомление, что сообщение НЕ + доставлено (молчать нельзя — иначе клиент решит, что оператор его получил), + но не чаще ОДНОГО РАЗА за то же окно (`_flood_notify_limiter`, limit=1) — + иначе само уведомление стало бы вторым источником флуда. Персистентность вынесена за `BridgeStorage`-протокол — маршрутизирующая логика (`process_update` и приватные `_handle_*`) не завязана на реальную БД, тестируется @@ -58,6 +73,7 @@ from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.orm import Session from app.core.config import settings +from app.core.ratelimit import SlidingWindowLimiter from app.core.shutdown import shutdown_requested from app.services.tgbot import web_support_storage from app.services.tgbot.client import TelegramApiError, TelegramClient @@ -87,6 +103,29 @@ SERVICE_UNAVAILABLE_TEXT = ( # tg_support_messages.kind): "text | photo | document | video | voice | other". _KNOWN_KINDS = ("text", "photo", "document", "video", "voice") +# (низкий приоритет, флуд-защита) — см. пункт F) в докстринге модуля. Порог +# НАМЕРЕННО заметно ниже группового лимита Telegram (~20 msg/min): бюджет +# делится с шапками-идентификациями и ответами оператора, и с другими +# одновременными клиентами — щедрый лимит одного отправителя всё равно упёрся +# бы в общий групповой 429. Тот же примитив, что и веб-чат поддержки +# (app/api/v1/support.py `_send_limiter`), ключ здесь — TELEGRAM chat_id +# отправителя (не username — у Telegram-клиента username может отсутствовать). +_FLOOD_LIMIT = 5 +_FLOOD_WINDOW_S = 60.0 +_flood_limiter = SlidingWindowLimiter(limit=_FLOOD_LIMIT, window_s=_FLOOD_WINDOW_S) + +# Уведомление о флуде — не чаще ОДНОГО раза за то же окно, иначе само +# уведомление стало бы вторым источником флуда. Отдельный лимитер с limit=1 на +# то же окно: `check()` возвращает None (и фиксирует попытку) ровно один раз за +# окно, дальше молчит до его истечения — без отдельной структуры "когда в +# последний раз уведомляли". +_flood_notify_limiter = SlidingWindowLimiter(limit=1, window_s=_FLOOD_WINDOW_S) + +FLOOD_LIMITED_TEXT = ( + "Сообщение не доставлено — вы отправляете сообщения слишком часто. " + "Пожалуйста, подождите немного и напишите ещё раз." +) + # #tgsupport-web review M2: реплай оператора медиа-типом (в т.ч. фото С ПОДПИСЬЮ) # на веб-зеркало НЕ доставляется частично — веб-чат текстовый MVP, оператор # получает это уведомление в топике вместо тихого игнора (иначе уверен, что ответил). @@ -407,6 +446,30 @@ async def _handle_private_message( logger.warning("tgbot bridge: приватное сообщение без message_id — игнор") return + # F) Флуд-лимит на отправителя — peek БЕЗ расхода бюджета (тот же паттерн, + # что `_send_limiter` в app/api/v1/support.py: под лимитом ниже сразу + # `.record()`-им попытку). Над лимитом — НЕ зеркалируем (иначе сам мирроринг + # уже съедает групповой Telegram-бюджет, который лимит и защищает) и НЕ + # пишем в tg_support_messages (без topic_message_id маршрутизировать ответ + # всё равно нечего). + flood_key = str(chat_id) # SlidingWindowLimiter — ключ str (см. app/core/ratelimit.py) + if _flood_limiter.retry_after(flood_key) is not None: + logger.warning( + "tgbot bridge: chat_id=%d превысил флуд-лимит (%d msg/%.0fs) — " + "сообщение НЕ зеркалируется в топик (защита группового Telegram-лимита)", + chat_id, + _FLOOD_LIMIT, + _FLOOD_WINDOW_S, + ) + # Уведомляем клиента, что сообщение НЕ доставлено (молчать нельзя — + # иначе клиент решит, что оператор его получил), но не чаще одного раза + # за окно — `_flood_notify_limiter.check()` возвращает None (и сам + # фиксирует попытку) ровно один раз за окно. + if _flood_notify_limiter.check(flood_key) is None: + await client.send_message(chat_id=chat_id, text=FLOOD_LIMITED_TEXT) + return + _flood_limiter.record(flood_key) + # Шапка — только на первое сообщение клиента за окно, иначе топик засоряется. if not storage.had_recent_inbound(chat_id, window_seconds=_HEADER_THROTTLE_WINDOW_S): header = _format_topic_header(chat_id, username, first_name, last_name) diff --git a/tradein-mvp/backend/tests/services/tgbot/test_bridge.py b/tradein-mvp/backend/tests/services/tgbot/test_bridge.py index fdc82334..601f4f32 100644 --- a/tradein-mvp/backend/tests/services/tgbot/test_bridge.py +++ b/tradein-mvp/backend/tests/services/tgbot/test_bridge.py @@ -243,6 +243,25 @@ def _support_chat_settings(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setattr(bridge.settings, "telegram_support_topic_id", SUPPORT_TOPIC_ID) +@pytest.fixture(autouse=True) +def _reset_flood_limiters(monkeypatch: pytest.MonkeyPatch) -> None: + """F) `_flood_limiter`/`_flood_notify_limiter` — module-level singletons (тот же + паттерн, что `_send_limiter` в app/api/v1/support.py); большинство тестов в + этом файле шлют сообщения от одного и того же chat_id=555, поэтому без сброса + накопленные хиты одного теста бы протекали в следующий и ломали его + предположения (тест флуда должен видеть ЧИСТЫЙ бюджет).""" + monkeypatch.setattr( + bridge, + "_flood_limiter", + bridge.SlidingWindowLimiter(limit=bridge._FLOOD_LIMIT, window_s=bridge._FLOOD_WINDOW_S), + ) + monkeypatch.setattr( + bridge, + "_flood_notify_limiter", + bridge.SlidingWindowLimiter(limit=1, window_s=bridge._FLOOD_WINDOW_S), + ) + + @pytest.fixture(autouse=True) def _stop_patches(): """Останавливает httpx.AsyncClient monkeypatch после каждого теста (unittest.mock.patch.start() @@ -427,6 +446,100 @@ async def test_private_message_notifies_client_when_support_chat_unset( assert storage.get_offset() == 14 +# ── F) флуд-лимит на отправителя (низкий приоритет) ───────────────────────── + + +async def test_private_message_flood_limit_blocks_excess_and_notifies_once() -> None: + """Больше `_FLOOD_LIMIT` сообщений от ОДНОГО chat_id за окно — зеркалирование + сверх лимита отключается (никакого copyMessage, никакой записи в + tg_support_messages — маршрутизировать ответ всё равно нечего без + topic_message_id). Клиент получает уведомление о недоставке РОВНО один раз + за окно, а не на каждое следующее превышение — иначе само уведомление стало + бы вторым источником флуда.""" + calls: list[tuple[str, dict[str, Any]]] = [] + client = _make_client({"copyMessage": {"message_id": 900}}, calls) + storage = FakeBridgeStorage() + + update_id = 100 + for i in range(bridge._FLOOD_LIMIT): + update = {"update_id": update_id, "message": _private_message(message_id=i + 1)} + await bridge.process_update(update, client, storage) + update_id += 1 + + # Ровно _FLOOD_LIMIT сообщений прошли мирроринг: первое — шапка + зеркало, + # остальные — только зеркало. + mirrored_calls = [m for m, _ in calls if m == "copyMessage"] + assert len(mirrored_calls) == bridge._FLOOD_LIMIT + assert len(storage.messages) == bridge._FLOOD_LIMIT + + calls.clear() + over_limit_update = { + "update_id": update_id, + "message": _private_message(message_id=bridge._FLOOD_LIMIT + 1), + } + await bridge.process_update(over_limit_update, client, storage) + update_id += 1 + + # Сверх лимита — НЕ зеркалируется, НЕ пишется в лог переписки, клиент + # получает уведомление о недоставке (не тихий игнор — клиент не должен + # решить, что оператор получил сообщение). + assert len(calls) == 1 + method, payload = calls[0] + assert method == "sendMessage" + assert payload["chat_id"] == 555 + assert payload["text"] == bridge.FLOOD_LIMITED_TEXT + assert len(storage.messages) == bridge._FLOOD_LIMIT + + calls.clear() + second_over_limit_update = { + "update_id": update_id, + "message": _private_message(message_id=bridge._FLOOD_LIMIT + 2), + } + await bridge.process_update(second_over_limit_update, client, storage) + + # Повторное превышение в ТОМ ЖЕ окне — уведомление подавлено (не второй + # источник флуда), никаких Telegram-вызовов вообще. + assert calls == [] + assert len(storage.messages) == bridge._FLOOD_LIMIT + + +async def test_private_message_flood_limit_does_not_block_other_client() -> None: + """Флуд-лимит — per-chat_id: клиент А исчерпал свой бюджет, но клиент Б + (другой chat_id) продолжает получать зеркалирование как обычно — один + флудящий клиент не блокирует доставку сообщений остальным (сама суть + задачи — воркер однопоточный, но лимит не даёт флудеру монополизировать + его через Telegram 429).""" + calls: list[tuple[str, dict[str, Any]]] = [] + client = _make_client({"copyMessage": {"message_id": 901}}, calls) + storage = FakeBridgeStorage() + + flooding_chat_id = 555 + update_id = 300 + for i in range(bridge._FLOOD_LIMIT + 2): + update = { + "update_id": update_id, + "message": _private_message(chat_id=flooding_chat_id, message_id=i + 1), + } + await bridge.process_update(update, client, storage) + update_id += 1 + + calls.clear() + + other_chat_id = 777001 + other_update = { + "update_id": update_id, + "message": _private_message(chat_id=other_chat_id, message_id=1, username="another_client"), + } + await bridge.process_update(other_update, client, storage) + + methods = [m for m, _ in calls] + # Другой клиент получает шапку (первое обращение) + зеркало как обычно — + # флуд первого клиента на него не влияет. + assert methods == ["sendMessage", "copyMessage"] + mirror_call = calls[1][1] + assert mirror_call["from_chat_id"] == other_chat_id + + # ── B) реплай оператора → user ──────────────────────────────────────────────