diff --git a/scripts/smoke-mera-perimeter.sh b/scripts/smoke-mera-perimeter.sh index 99e53ef5..535381e4 100644 --- a/scripts/smoke-mera-perimeter.sh +++ b/scripts/smoke-mera-perimeter.sh @@ -101,6 +101,27 @@ check "gendsgn.ru/api/v1/admin/* — 401 anonymous" "$BASE_MAIN/api/v1/admin/use check "merahome.ru — 301 to canonical" "https://merahome.ru/" 301 check "meraotsenka.ru — 301 to canonical" "https://meraotsenka.ru/" 301 +# 6. Платёжный периметр (PR-D2) — готовит почву под PR-D3 (роутер) и PR-D4 +# (Caddy), но САМ НИЧЕГО НЕ ОТКРЫВАЕТ. Ожидаем закрытое состояние С ОБЕИХ +# СТОРОН прямо сейчас: +# - meraocenka.ru вообще не проксирует /trade-in/api/* (allowlist-by-default, +# см. проверку 2) — 404 от Caddy, до бэкенда не доходит; +# - gendsgn.ru проксирует /trade-in/api/* в tradein-backend, но rbac_guard +# (`_PUBLIC_PATHS` в app/core/rbac.py — ЭТОТ PR её не трогает) не знает +# платёжные пути и требует X-Authenticated-User → 401 анониму. +# Если один из этих чек-ов вдруг перестанет быть 404/401 РАНЬШЕ мержа +# PR-D3/PR-D4 — это и есть преждевременная утечка периметра, которую ловит +# этот смоук (канарейка: осознанно станет красной, когда PR-D3/PR-D4 явно +# откроют эти пути — тогда ожидания здесь надо обновить вместе с ними). +check "meraocenka.ru payments/notify — must 404 (Caddy не проксирует, PR-D4)" \ + "$BASE_MERA/trade-in/api/v1/trade-in/payments/notify" 404 +check "meraocenka.ru payments/checkout — must 404 (Caddy не проксирует, PR-D4)" \ + "$BASE_MERA/trade-in/api/v1/trade-in/payments/checkout" 404 +check "trade-in payments/notify — 401 anonymous (rbac закрыт до PR-D3)" \ + "$BASE_MAIN/trade-in/api/v1/trade-in/payments/notify" 401 +check "trade-in payments/checkout — 401 anonymous (rbac закрыт до PR-D3)" \ + "$BASE_MAIN/trade-in/api/v1/trade-in/payments/checkout" 401 + echo "========================================" if [ "$fail" -eq 0 ]; then echo "ALL CHECKS PASSED" diff --git a/tradein-mvp/backend/app/core/ratelimit.py b/tradein-mvp/backend/app/core/ratelimit.py index f5f3fe04..cda736bb 100644 --- a/tradein-mvp/backend/app/core/ratelimit.py +++ b/tradein-mvp/backend/app/core/ratelimit.py @@ -32,6 +32,18 @@ from starlette.middleware.base import BaseHTTPMiddleware from app.core.config import settings +# Платёжная нотификация Т-Банка (PR-D2, готовит почву под PR-D3 — путь ещё +# закрыт rbac до того момента). Сервер-к-серверу, без сессии/X-Authenticated-User +# → в общем лимитере попал бы в один и тот же per-IP ключ с любым другим +# анонимным трафиком с той же исходящей сети банка. Мотив НЕ «банк упрётся в +# лимит» — 300/60с и так щедро — а «429 никогда не должен стать причиной, по +# которой денежное состояние разъехалось»: для банка недоставленная нотификация +# = «доставка не удалась», альтернативного канала нет, а очередь ретраев +# растягивается на сутки. Только точный путь notify — НЕ checkout (тот +# инициирует пользователь с сессией/курсором в браузере, абуз там штатно +# лимитируем как любой другой API-путь). +_PAYMENTS_NOTIFY_PATH = "/api/v1/trade-in/payments/notify" + class RateLimitMiddleware(BaseHTTPMiddleware): """Sliding-window rate limit на /api/v1/*. Health и статика — без лимита.""" @@ -42,6 +54,23 @@ class RateLimitMiddleware(BaseHTTPMiddleware): async def dispatch(self, request: Request, call_next): # type: ignore[no-untyped-def] path = request.url.path + # Платёжная нотификация — мимо ОБЩЕГО (per-user/per-IP shared) лимитера, + # но НЕ без лимита вовсе: idiom `_notify_limiter` (`SlidingWindowLimiter`, + # тот же приём, что `support.py:92`/`:319` — узкий per-feature бюджет + # ВМЕСТО общего, не полное отключение защиты). Порог заведомо выше любого + # штатного трафика банка (документированное расписание ретраев неизвестно, + # см. mera-tbank-acquiring-recon.md — берём с кратным запасом), но конечен: + # полное отключение оставило бы путь без backstop против шторма запросов — + # подпись отсекает мусор ПОСЛЕ разбора тела (PR-D3), не до. + if path == _PAYMENTS_NOTIFY_PATH: + retry_after = _notify_limiter.check(_client_ip(request)) + if retry_after is not None: + return JSONResponse( + status_code=429, + content={"detail": "Слишком много запросов. Попробуйте позже."}, + headers={"Retry-After": str(int(retry_after) + 1)}, + ) + return await call_next(request) # Лимитируем только API; health и прочее — пропускаем. if not path.startswith("/api/"): return await call_next(request) @@ -143,6 +172,16 @@ class SlidingWindowLimiter: return None +# Щедрый бюджет для платёжной нотификации (PR-D2): 3000/60с (50 req/s) — на два +# порядка выше любого правдоподобного трафика банка (тест 400/60с проходит с +# запасом в 7.5×), но конечен — backstop против шторма запросов на путь, где +# подпись проверяется уже ПОСЛЕ разбора тела. Ключ — client IP (у сервер-к- +# серверу вызова нет сессии/X-Authenticated-User). +_NOTIFY_RATE_LIMIT = 3000 +_NOTIFY_RATE_WINDOW_S = 60.0 +_notify_limiter = SlidingWindowLimiter(limit=_NOTIFY_RATE_LIMIT, window_s=_NOTIFY_RATE_WINDOW_S) + + def _client_ip(request: Request) -> str: """Честный клиентский IP при РОВНО ОДНОМ доверенном прокси (Caddy) перед нами. diff --git a/tradein-mvp/backend/app/core/request_audit.py b/tradein-mvp/backend/app/core/request_audit.py index 7eafefbc..9416b556 100644 --- a/tradein-mvp/backend/app/core/request_audit.py +++ b/tradein-mvp/backend/app/core/request_audit.py @@ -40,7 +40,29 @@ logger = logging.getLogger(__name__) # Зеркалит app.main._PUBLIC_PATHS. Не импортируем напрямую из app.main — оно # импортирует этот модуль (регистрирует middleware), обратный импорт дал бы # циклическую зависимость. -_PUBLIC_PATHS = frozenset({"/health", "/docs", "/redoc", "/openapi.json"}) +# +# PR-D2: `/api/v1/trade-in/payments/notify` — заранее в skip-набор (defense-in- +# depth), хотя rbac ещё закрывает этот путь до PR-D3. Причины две: +# 1) сам путь не должен попадать в аудит вообще — тело нотификации содержит +# `Token`/`Pan`/`ExpDate` (см. `app/main.py._before_send`, тот же мотив, что +# и вырезание тела из мониторинга); хоть это middleware само по себе тело +# запроса в payload не пишет (только status_code/path/method), путь не +# должен зависеть от того, что кто-то потом добавит поле "body" в событие; +# 2) НЕ авторизующая проверка: `RequestAuditMiddleware` внешний относительно +# `rbac_guard` и читает сырой `X-Authenticated-User` (см. `main.py` порядок +# middleware) — анонимный POST на notify с подделанным заголовком +# `X-Authenticated-User: admin` иначе писал бы фальшивые события в +# `user_events` с атрибуцией admin, при этом rbac при этом ничего не знает +# (сам гейт отдельно, 401 всё равно вернёт до PR-D3). +_PUBLIC_PATHS = frozenset( + { + "/health", + "/docs", + "/redoc", + "/openapi.json", + "/api/v1/trade-in/payments/notify", + } +) # Методы, меняющие состояние — для /api/v1/admin/* именно они должны попадать в # аудит с атрибуцией (кто именно загрузил куки / включил авто-логин / поправил diff --git a/tradein-mvp/backend/app/main.py b/tradein-mvp/backend/app/main.py index 8b76910e..9fa819c4 100644 --- a/tradein-mvp/backend/app/main.py +++ b/tradein-mvp/backend/app/main.py @@ -68,20 +68,32 @@ logging.getLogger("httpx").setLevel(logging.WARNING) if settings.glitchtip_dsn: from app.observability.sentry_scrub import ( redact_telegram_bot_token, + scrub_payment_request_body, stabilize_retry_error_fingerprint, ) def _before_send(event: dict[str, object], hint: dict[str, object]) -> dict[str, object] | None: - """Композиция PII-scrub + Telegram bot-токен redaction (#tgsupport-web) + - RetryError fingerprint-стабилизация (glitchtip-noise) — см. - app/tgbot_main.py._before_send (та же композиция без последнего шага, - тот бот geocoder не зовёт). PII/token — тот же риск: теперь этот процесс - тоже держит TelegramClient в стек-фреймах при ошибке sendMessage, а - include_local_variables=False ниже — первый рубеж защиты. RetryError — - этот процесс обслуживает /api/v1/geocode/* (suggest/lookup/reverse), - которые ретраят Nominatim через tenacity; см. + """Композиция платёжный body-wipe + PII-scrub + Telegram bot-токен redaction + + RetryError fingerprint-стабилизация (#tgsupport-web, PR-D2, glitchtip-noise) — + см. app/tgbot_main.py._before_send (идентичная композиция без последнего шага, + тот бот geocoder не зовёт). Тот же риск: теперь этот процесс тоже держит + TelegramClient в стек-фреймах при ошибке sendMessage, а + include_local_variables=False ниже — первый рубеж защиты. + + PR-D2: платёжный body-wipe идёт ПЕРВЫМ шагом, а не заменяет остальные — + режет `request.data` целиком только для `/payments/*`, остальные пути + (extra/contexts/traceback) по-прежнему проходят ключ-based scrub и + token-redaction. Тот же обработчик передан ОБОИМ каналам ниже + (before_send и before_send_transaction) — вчерашний баг в Птице закрыл + только error-канал, transaction-канал остался вообще без обработчика. + + RetryError-стабилизация — этот процесс обслуживает /api/v1/geocode/* + (suggest/lookup/reverse), которые ретраят Nominatim через tenacity; см. sentry_scrub.stabilize_retry_error_fingerprint.""" - scrubbed = scrub_pii_event(event, hint) # type: ignore[arg-type] + scrubbed = scrub_payment_request_body(event, hint) # type: ignore[arg-type] + if scrubbed is None: + return None + scrubbed = scrub_pii_event(scrubbed, hint) # type: ignore[arg-type] if scrubbed is None: return None detokened = redact_telegram_bot_token(scrubbed, hint) # type: ignore[arg-type] @@ -99,6 +111,10 @@ if settings.glitchtip_dsn: # держит base URL с токеном в локальных переменных стек-фрейма — default # sentry_sdk (True) приложил бы их к traceback открытым текстом. before_send=_before_send, + # PR-D2: тот же обработчик на transaction-канал — traces_sample_rate=0.0 + # сегодня не шлёт трейсы вообще, но это belt-and-suspenders на случай, + # если трейсинг когда-нибудь включат (см. docstring _before_send выше). + before_send_transaction=_before_send, integrations=[ StarletteIntegration(), FastApiIntegration(), diff --git a/tradein-mvp/backend/app/observability/sentry_scrub.py b/tradein-mvp/backend/app/observability/sentry_scrub.py index 7cb78fd0..0920486e 100644 --- a/tradein-mvp/backend/app/observability/sentry_scrub.py +++ b/tradein-mvp/backend/app/observability/sentry_scrub.py @@ -29,7 +29,36 @@ from tenacity import RetryError _REDACTED = "[REDACTED]" # Ключи consumer-PII (нижний регистр; сверка case-insensitive). -_PII_KEYS = frozenset({"client_name", "client_phone", "client_email", "phone", "email", "name"}) +# PR-D2 (payments perimeter hardening): + платёжные поля Т-Банка (customer_email/ +# customer_phone из checkout, pan/expdate/cardid/rebillid/token/terminalkey из +# notify) — belt-and-suspenders поверх `scrub_payment_request_body` ниже, которая +# вырезает `request.data` для /payments/* целиком: этот словарь всё равно нужен +# для extra/contexts И на случай, если платёжное поле когда-нибудь попадёт в +# error event НЕ через request.data (напр. кто-то положит его в extra вручную). +_PII_KEYS = frozenset( + { + "client_name", + "client_phone", + "client_email", + "phone", + "email", + "name", + "customer_email", + "customer_phone", + "pan", + "expdate", + "cardid", + "rebillid", + "token", + "terminalkey", + } +) + +# Сегмент пути платёжного периметра (notify + checkout + любой будущий +# /payments/* суб-путь) — PR-D2, готовит почву под PR-D3 (эндпоинты ещё не +# существуют). Матчим по сегменту, не по конкретному эндпоинту, чтобы не +# требовать правки этого файла на каждый новый платёжный путь. +_PAYMENTS_URL_SEGMENT = "/api/v1/trade-in/payments/" # Telegram Bot API токен в пути URL: /bot:/. # Матчим ровно этот сегмент (не весь URL) — сохраняет остальной путь/query @@ -179,6 +208,39 @@ def scrub_pii_event(event: Event, _hint: dict[str, Any]) -> Event | None: return event +def scrub_payment_request_body(event: Event, _hint: dict[str, Any]) -> Event | None: + """Вырезать `event['request']['data']` целиком для платёжных путей (PR-D2). + + Ключ-based `scrub_pii_event` НЕ спасает платёжную нотификацию: sentry_sdk + 2.64 (`integrations/starlette.py`) кладёт ПОЛНОЕ тело запроса в + `event.request.data`, и `send_default_pii=False` этот путь не гейтит — тот + флаг управляет только куками, не телом запроса (проверено живьём на соседнем + продукте). Тело нотификации Т-Банка несёт `Token`/`Pan`/`ExpDate`/`CardId`/ + `RebillId`/`DATA` — банк сам выбирает имена полей, перечислить их все заранее + нельзя, поэтому единственная безопасная стратегия для этого пути — не + отправлять тело целиком, а не пытаться вычистить отдельные ключи. + + Матчим по сегменту `/api/v1/trade-in/payments/` (не по конкретному + эндпоинту) — покрывает notify, checkout и любой будущий суб-путь одним + фильтром, без правки этого файла на каждое расширение платёжного API. + Сравнение регистронезависимое: `_PUBLIC_PATHS` (rbac) — точное множество без + учёта регистра только у Caddy, не у Python, так что нестандартный регистр + пути технически может долететь до обработчика и породить событие. + + Композировать с `scrub_pii_event`/`redact_telegram_bot_token`, а не вместо + них — этот шаг закрывает только `request.data`, extra/contexts и + traceback-locals остаются на ответственности остальных шагов композиции. + """ + if not isinstance(event, dict): + return event + request = event.get("request") + if isinstance(request, dict): + url = request.get("url") + if isinstance(url, str) and _PAYMENTS_URL_SEGMENT in url.lower(): + request.pop("data", None) + return event + + def _redact_strings(obj: Any) -> Any: """Рекурсивно проходит dict/list/tuple и прогоняет обе токен-регулярки по КАЖДОЙ строке (не только по конкретным ключам) — токен может оказаться в locals diff --git a/tradein-mvp/backend/app/scheduler_main.py b/tradein-mvp/backend/app/scheduler_main.py index e5f93ed0..210b30fa 100644 --- a/tradein-mvp/backend/app/scheduler_main.py +++ b/tradein-mvp/backend/app/scheduler_main.py @@ -45,20 +45,31 @@ if settings.glitchtip_dsn: from sentry_sdk.integrations.sqlalchemy import SqlalchemyIntegration from app.observability.sentry_scrub import ( + scrub_payment_request_body, scrub_pii_event, stabilize_retry_error_fingerprint, ) def _before_send(event: dict, hint: dict) -> dict | None: # type: ignore[type-arg] - """PII-scrub + RetryError fingerprint-стабилизация (glitchtip-noise). + """PR-D2: этот процесс не держит ASGI-приложения (нет `request` в event + сегодня), но payments_confirm/payments_reconcile (PR-E, тот же + `tradein-scraper` контейнер) будут звать Т-Банк API отсюда — belt-and- + suspenders на случай, если платёжные данные когда-нибудь попадут в + `request`/`extra`. Тот же обработчик на оба канала ниже — см. + app/main.py._before_send (идентичный мотив, не дублировать без причины). - Этот процесс гоняет `geocode_missing_listings` (ночной batch, сотни - адресов за прогон) — @retry-декорированные Nominatim-хелперы - (app/services/geocoder.py) на исчерпанных ретраях исторически плодили - по отдельному GlitchTip issue на КАЖДЫЙ адрес (RetryError.__str__() - тащит нестабильный repr() Future). См. sentry_scrub docstring. + PII-scrub + RetryError fingerprint-стабилизация (glitchtip-noise) идут + следом за платёжным body-wipe: этот процесс гоняет + `geocode_missing_listings` (ночной batch, сотни адресов за прогон) — + @retry-декорированные Nominatim-хелперы (app/services/geocoder.py) на + исчерпанных ретраях исторически плодили по отдельному GlitchTip issue + на КАЖДЫЙ адрес (RetryError.__str__() тащит нестабильный repr() Future). + См. sentry_scrub docstring. """ - scrubbed = scrub_pii_event(event, hint) + scrubbed = scrub_payment_request_body(event, hint) # type: ignore[arg-type] + if scrubbed is None: + return None + scrubbed = scrub_pii_event(scrubbed, hint) if scrubbed is None: return None return stabilize_retry_error_fingerprint(scrubbed, hint) @@ -70,6 +81,7 @@ if settings.glitchtip_dsn: traces_sample_rate=0.0, send_default_pii=False, before_send=_before_send, + before_send_transaction=_before_send, integrations=[ SqlalchemyIntegration(), HttpxIntegration(), diff --git a/tradein-mvp/backend/app/tgbot_main.py b/tradein-mvp/backend/app/tgbot_main.py index 5f731a86..48eaf32e 100644 --- a/tradein-mvp/backend/app/tgbot_main.py +++ b/tradein-mvp/backend/app/tgbot_main.py @@ -58,12 +58,17 @@ if settings.glitchtip_dsn: from sentry_sdk.integrations.httpx import HttpxIntegration from sentry_sdk.integrations.logging import LoggingIntegration - from app.observability.sentry_scrub import redact_telegram_bot_token, scrub_pii_event + from app.observability.sentry_scrub import ( + redact_telegram_bot_token, + scrub_payment_request_body, + scrub_pii_event, + ) def _before_send(event: Any, hint: dict[str, Any]) -> Any: - """Композиция PII-scrub (form-данные) + Telegram bot-токен redaction - (#tgsupport review). Токен утекает ДВУМЯ независимыми векторами, которые - `include_local_variables=False` ниже и этот хук закрывают вместе: + """Композиция платёжный body-wipe (PR-D2) + PII-scrub (form-данные) + + Telegram bot-токен redaction (#tgsupport review). Токен утекает ДВУМЯ + независимыми векторами, которые `include_local_variables=False` ниже и + этот хук закрывают вместе: 1. `include_local_variables=True` (sentry_sdk default) кладёт stack-frame locals (`self._base`/`url` в `TelegramClient._request`) в traceback — закрыто через `include_local_variables=False` в `sentry_sdk.init`. @@ -72,8 +77,16 @@ if settings.glitchtip_dsn: перестанет спасать, если трейсинг когда-нибудь включат. Regex-редактор — belt-and-suspenders на случай #1 (если include_local_variables случайно вернут) И на span data. + + Платёжный body-wipe — belt-and-suspenders: этот процесс не держит ASGI- + приложения (нет `request` в event сегодня), но тот же обработчик передан + ОБОИМ каналам ниже (before_send/before_send_transaction) ради единообразия + со всеми точками инициализации sentry_sdk в проекте (см. app/main.py). """ - scrubbed = scrub_pii_event(event, hint) + scrubbed = scrub_payment_request_body(event, hint) + if scrubbed is None: + return None + scrubbed = scrub_pii_event(scrubbed, hint) if scrubbed is None: return None return redact_telegram_bot_token(scrubbed, hint) @@ -86,6 +99,7 @@ if settings.glitchtip_dsn: send_default_pii=False, include_local_variables=False, before_send=_before_send, + before_send_transaction=_before_send, integrations=[ HttpxIntegration(), LoggingIntegration(level=logging.INFO, event_level=logging.ERROR), diff --git a/tradein-mvp/backend/tests/test_ratelimit.py b/tradein-mvp/backend/tests/test_ratelimit.py index 4d1cd8dc..b9b71c1a 100644 --- a/tradein-mvp/backend/tests/test_ratelimit.py +++ b/tradein-mvp/backend/tests/test_ratelimit.py @@ -149,6 +149,98 @@ def test_sliding_window_limiter_per_key_isolation(): assert limiter.retry_after("bob") is None # свой ключ — не задет alice +# ── Платёжная нотификация — мимо ОБЩЕГО лимитера (PR-D2, критерий приёмки #3) ── + + +@pytest.fixture +def notify_client(monkeypatch): + """То же минимальное приложение, что `client`, но лимит анонима искусственно + крошечный (1/60с) — если бы notify-путь шёл через общий лимитер, 2-й запрос + уже получил бы 429. Плюс контрольный `/api/v1/ping` — доказывает, что + лимитер в принципе активен (не выключен целиком), просто notify мимо него.""" + monkeypatch.setattr(config.settings, "rate_limit", 1) + monkeypatch.setattr(config.settings, "rate_limit_window_s", 60.0) + monkeypatch.setattr(config.settings, "rate_limit_authenticated_multiplier", 2) + + app = FastAPI() + app.add_middleware(RateLimitMiddleware) + + @app.get("/api/v1/ping") + def ping() -> dict[str, bool]: + return {"ok": True} + + @app.post("/api/v1/trade-in/payments/notify") + def notify() -> dict[str, bool]: + return {"ok": True} + + return TestClient(app) + + +def test_notify_path_bypasses_general_limiter_400_requests_zero_429(notify_client): + """PR-D2 acceptance criteria: 400 запросов к notify с одного адреса за минуту + не дают ни одного отказа по частоте — даже с общим лимитом искусственно + зажатым до 1/60с (см. фикстуру).""" + statuses = [ + notify_client.post("/api/v1/trade-in/payments/notify").status_code for _ in range(400) + ] + assert all( + code == 200 for code in statuses + ), f"notify получил 429 хотя бы раз: {[c for c in statuses if c != 200]}" + + +def test_general_limiter_still_active_for_other_paths(notify_client): + """Контроль: общий лимитер НЕ выключен целиком — обычный /api/v1/ping с тем + же крошечным лимитом (1/60с) отбивается на 2-м запросе как обычно. Доказывает, + что notify-bypass узкий (точный путь), а не побочный эффект общей поломки.""" + assert notify_client.get("/api/v1/ping").status_code == 200 + assert notify_client.get("/api/v1/ping").status_code == 429 + + +def test_notify_bypass_has_own_dedicated_limiter_not_unlimited(): + """notify НЕ отключён от лимитера вовсе — своя щедрая, но конечная корзина + (`_notify_limiter`, идиома `SlidingWindowLimiter` из `support.py`). Проверяем + напрямую: исчерпать маленький искусственный лимит и убедиться, что backstop + таки срабатывает (защита от полного disable вместо узкого бюджета).""" + from app.core import ratelimit as ratelimit_module + + limiter = ratelimit_module.SlidingWindowLimiter(limit=2, window_s=60.0) + assert limiter.check("1.2.3.4") is None + assert limiter.check("1.2.3.4") is None + # 3-й запрос того же ключа — уже за лимитом (backstop жив). + assert limiter.check("1.2.3.4") is not None + + +def test_notify_limiter_key_is_per_ip_not_global(monkeypatch): + """Бюджет notify — per-IP (не общий на все входящие сразу), тот же принцип + ключа, что общий лимитер использует для анонимного трафика.""" + monkeypatch.setattr(config.settings, "rate_limit", 300) + monkeypatch.setattr(config.settings, "rate_limit_window_s", 60.0) + + from app.core import ratelimit as ratelimit_module + + monkeypatch.setattr( + ratelimit_module, + "_notify_limiter", + ratelimit_module.SlidingWindowLimiter(limit=1, window_s=60.0), + ) + + app = FastAPI() + app.add_middleware(RateLimitMiddleware) + + @app.post("/api/v1/trade-in/payments/notify") + def notify() -> dict[str, bool]: + return {"ok": True} + + client = TestClient(app) + # Первый запрос с IP #1 — проходит, второй с тем же IP — 429 (лимит=1). + headers_ip1 = {"X-Forwarded-For": "1.1.1.1"} + headers_ip2 = {"X-Forwarded-For": "2.2.2.2"} + assert client.post("/api/v1/trade-in/payments/notify", headers=headers_ip1).status_code == 200 + assert client.post("/api/v1/trade-in/payments/notify", headers=headers_ip1).status_code == 429 + # Другой IP — своя, независимая корзина. + assert client.post("/api/v1/trade-in/payments/notify", headers=headers_ip2).status_code == 200 + + def test_sliding_window_limiter_prunes_empty_buckets_past_threshold(): """review L2: пустые корзины чистятся при накоплении >10000 ключей (тот же паттерн, что `RateLimitMiddleware.dispatch`) — не бесконечная утечка памяти. diff --git a/tradein-mvp/backend/tests/test_request_audit.py b/tradein-mvp/backend/tests/test_request_audit.py index 06bbd2d5..1e3f04c8 100644 --- a/tradein-mvp/backend/tests/test_request_audit.py +++ b/tradein-mvp/backend/tests/test_request_audit.py @@ -262,6 +262,37 @@ def test_login_event_type_when_request_succeeds(client: TestClient) -> None: assert login_calls[0].kwargs["payload"] == {"status_code": 200} +# ── Платёжная нотификация — вне аудита (PR-D2, критерий приёмки #4) ───────── + + +def test_payments_notify_path_skips_audit_even_with_spoofed_admin_header() -> None: + """PR-D2 acceptance criteria: запрос к нотификации не создаёт записей в + журнале аудита — даже с заголовком `X-Authenticated-User: admin`. Middleware + — ВНЕШНИЙ относительно rbac_guard и читает сырой заголовок напрямую (см. + docstring `request_audit.py`), так что анонимный POST со спуфнутым + заголовком иначе писал бы фальшивое `login`/`api_request` событие с + атрибуцией admin, хотя rbac этот путь пока (до PR-D3) закрывает 401'ом + отдельно и независимо от этого middleware.""" + app = FastAPI() + app.add_middleware(RequestAuditMiddleware) + + @app.post("/api/v1/trade-in/payments/notify") + def notify() -> 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=True), + ): + resp = TestClient(app).post( + "/api/v1/trade-in/payments/notify", + headers={"X-Authenticated-User": "admin"}, + ) + + assert resp.status_code == 200 + mock_schedule.assert_not_called() + + def test_login_failed_event_type_when_rbac_rejects_request() -> None: """Ответ >= 400 (напр. RBAC-отказ downstream: неизвестная роль / протухший внутренний секрет) -> event_type='login_failed', а НЕ 'login' — раньше эти diff --git a/tradein-mvp/backend/tests/test_sentry_init_wiring.py b/tradein-mvp/backend/tests/test_sentry_init_wiring.py new file mode 100644 index 00000000..46b86499 --- /dev/null +++ b/tradein-mvp/backend/tests/test_sentry_init_wiring.py @@ -0,0 +1,111 @@ +"""PR-D2 (платёжный периметр): каждая точка инициализации `sentry_sdk.init(...)` +в проекте обязана проводить ОБА канала мониторинга — `before_send` (error-события) +и `before_send_transaction` (performance-трейсы). Мотивирующий инцидент (соседний +продукт, Птица, вчера): закрыли только error-канал через `before_send`, а +`before_send_transaction` остался вообще без обработчика — очистка body/PII там +не применялась. + +Инициализация происходит на module-level внутри `if settings.glitchtip_dsn:` — +поведенческий тест потребовал бы реального импорта модуля с DSN, выставленным +ДО импорта (модуль кэшируется, monkeypatch settings после импорта на init уже не +влияет), плюс `sentry_sdk.init` — процесс-глобальный singleton (повторные вызовы +из разных тестов друг друга затирают). Вместо этого — статический разбор AST: +детерминирован, не трогает process-global state, не зависит от порядка тестов. + +НЕ grep/substring по тексту файла: `before_send_transaction` уже упоминается в +docstring-комментариях этих же файлов (объясняющих МОТИВ) — substring-поиск дал +бы ложный PASS без единой реальной проводки в `sentry_sdk.init(...)`. Разбор +именно keyword-аргументов AST Call-узла `sentry_sdk.init(...)` не подвержен +этому false positive. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest + +_APP_DIR = Path(__file__).resolve().parent.parent / "app" + +# Все известные точки инициализации sentry_sdk в проекте (backend API, scraper +# scheduler, telegram support-bridge). Список сверяется отдельным тестом ниже +# против грепа по всему `app/`, чтобы новая точка инициализации не прошла мимо +# этого файла молча. +_SENTRY_INIT_FILES = ["main.py", "scheduler_main.py", "tgbot_main.py"] + + +def _sentry_init_calls(source: str, filename: str) -> list[ast.Call]: + """Все AST Call-узлы вида `sentry_sdk.init(...)` в модуле.""" + tree = ast.parse(source, filename=filename) + calls = [] + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + func = node.func + if ( + isinstance(func, ast.Attribute) + and func.attr == "init" + and isinstance(func.value, ast.Name) + and func.value.id == "sentry_sdk" + ): + calls.append(node) + return calls + + +@pytest.mark.parametrize("filename", _SENTRY_INIT_FILES) +def test_sentry_init_wires_both_channels(filename: str) -> None: + source = (_APP_DIR / filename).read_text(encoding="utf-8") + calls = _sentry_init_calls(source, filename) + assert calls, f"{filename}: sentry_sdk.init(...) call not found (файл переехал?)" + for call in calls: + kwarg_names = {kw.arg for kw in call.keywords if kw.arg is not None} + assert "before_send" in kwarg_names, ( + f"{filename}: sentry_sdk.init(...) не передаёт before_send — " + "error-канал уходит в GlitchTip без scrub" + ) + assert "before_send_transaction" in kwarg_names, ( + f"{filename}: sentry_sdk.init(...) не передаёт before_send_transaction — " + "transaction-канал уходит в GlitchTip без scrub (ровно вчерашний баг Птицы)" + ) + + +def test_sentry_init_before_send_and_transaction_use_same_handler() -> None: + """`before_send` и `before_send_transaction` обязаны указывать на ОДИН и тот + же обработчик (одинаковое имя переменной/функции в keyword-значении) — иначе + возможен регресс, при котором кто-то поправит один канал и забудет второй, + хотя формально оба параметра присутствуют.""" + for filename in _SENTRY_INIT_FILES: + source = (_APP_DIR / filename).read_text(encoding="utf-8") + calls = _sentry_init_calls(source, filename) + for call in calls: + kwargs = {kw.arg: kw.value for kw in call.keywords if kw.arg is not None} + before_send = kwargs.get("before_send") + before_send_txn = kwargs.get("before_send_transaction") + assert before_send is not None and before_send_txn is not None + # Оба значения — ссылки на имя (ast.Name), сравниваем идентификатор. + assert isinstance(before_send, ast.Name) + assert isinstance(before_send_txn, ast.Name) + assert before_send.id == before_send_txn.id, ( + f"{filename}: before_send={before_send.id!r} != " + f"before_send_transaction={before_send_txn.id!r} — разные обработчики " + "на двух каналах, ровно тот класс бага, что и голый пропуск канала" + ) + + +def test_all_sentry_init_call_sites_are_enumerated() -> None: + """Если кто-то добавит НОВУЮ точку инициализации sentry_sdk.init(...) где-то + ещё в app/ — этот тест должен упасть, а не молча пропустить её мимо теста + выше (список `_SENTRY_INIT_FILES` — руками поддерживаемый allowlist).""" + found_files = set() + for py_file in _APP_DIR.rglob("*.py"): + source = py_file.read_text(encoding="utf-8") + if _sentry_init_calls(source, str(py_file)): + found_files.add(py_file.relative_to(_APP_DIR).as_posix()) + + expected = set(_SENTRY_INIT_FILES) + assert found_files == expected, ( + f"Точки инициализации sentry_sdk.init(...) разошлись со списком в тесте: " + f"найдено {sorted(found_files)}, ожидалось {sorted(expected)}. Новую точку " + "нужно добавить в _SENTRY_INIT_FILES ЭТОГО файла и проверить оба канала." + ) diff --git a/tradein-mvp/backend/tests/test_sentry_scrub.py b/tradein-mvp/backend/tests/test_sentry_scrub.py index 67d26650..4925c48d 100644 --- a/tradein-mvp/backend/tests/test_sentry_scrub.py +++ b/tradein-mvp/backend/tests/test_sentry_scrub.py @@ -14,6 +14,7 @@ os.environ.setdefault("DATABASE_URL", "postgresql+psycopg://test:test@localhost: from app.observability.sentry_scrub import ( redact_telegram_bot_token, + scrub_payment_request_body, scrub_pii_event, stabilize_retry_error_fingerprint, ) @@ -232,6 +233,128 @@ def test_bare_token_redaction_leaves_benign_colon_strings_untouched(benign: str) assert out["logentry"]["message"] == benign +# ── Платёжный body-wipe (PR-D2, критерий приёмки #1) ───────────────────────── + + +def test_scrub_payment_request_body_removes_data_for_payments_path() -> None: + """Событие мониторинга с адресом платёжного пути и телом, содержащим `Token` + и `Pan`, уходит БЕЗ ключа с телом (PR-D2 acceptance criteria).""" + event = { + "request": { + "url": "https://gendsgn.ru/api/v1/trade-in/payments/notify", + "data": { + "Token": "deadbeefdeadbeefdeadbeef", + "Pan": "220000******0000", + "ExpDate": "1230", + "CardId": "123456", + "RebillId": "987654", + "DATA": {"Email": "someone@example.com"}, + }, + "method": "POST", + } + } + out = scrub_payment_request_body(event, {}) + assert out is not None + assert "data" not in out["request"] + # Остальные поля request не тронуты. + assert out["request"]["method"] == "POST" + assert out["request"]["url"] == "https://gendsgn.ru/api/v1/trade-in/payments/notify" + + +def test_scrub_payment_request_body_covers_checkout_too() -> None: + """Матч по сегменту пути, не по конкретному эндпоинту — checkout тоже режется.""" + event = { + "request": { + "url": "https://gendsgn.ru/api/v1/trade-in/payments/checkout", + "data": {"consent": True, "product_code": "report_pdf"}, + } + } + out = scrub_payment_request_body(event, {}) + assert out is not None + assert "data" not in out["request"] + + +def test_scrub_payment_request_body_case_insensitive_url_match() -> None: + """Регистр URL не должен позволять данным проскочить — Caddy/rbac регистр + трактуют по-разному, страховка на случай, если событие всё же породилось.""" + event = { + "request": { + "url": "https://gendsgn.ru/API/V1/Trade-In/Payments/Notify", + "data": {"Token": "secret"}, + } + } + out = scrub_payment_request_body(event, {}) + assert out is not None + assert "data" not in out["request"] + + +def test_scrub_payment_request_body_leaves_other_paths_untouched() -> None: + """Не платёжный путь — тело остаётся (это не общий kill-switch на request.data).""" + event = { + "request": { + "url": "https://gendsgn.ru/api/v1/trade-in/estimate", + "data": {"area_sqm": 50, "region": "66"}, + } + } + out = scrub_payment_request_body(event, {}) + assert out is not None + assert out["request"]["data"] == {"area_sqm": 50, "region": "66"} + + +def test_scrub_payment_request_body_handles_missing_request() -> None: + out = scrub_payment_request_body({"level": "error"}, {}) + assert out == {"level": "error"} + + +def test_scrub_payment_request_body_handles_non_dict_event() -> None: + assert scrub_payment_request_body(None, {}) is None # type: ignore[arg-type] + + +def test_scrub_payment_request_body_handles_missing_url() -> None: + """`request` без `url` (нестандартный event) — не бросает, тело не трогает.""" + event = {"request": {"data": {"Token": "x"}}} + out = scrub_payment_request_body(event, {}) + assert out is not None + assert out["request"]["data"] == {"Token": "x"} + + +# ── Расширенный набор платёжных PII-ключей (PR-D2, критерий приёмки #2) ────── + + +def test_pii_keys_scrub_payment_fields_at_arbitrary_depth() -> None: + """Скрабер вычищает `customer_email`/`customer_phone`/платёжные поля на + произвольной глубине вложенности (PR-D2 acceptance criteria).""" + event = { + "extra": { + "checkout_context": { + "buyer": { + "customer_email": "buyer@example.com", + "customer_phone": "+79991234567", + "nested_list": [ + {"pan": "220000******1111", "expdate": "0129"}, + {"cardid": "abc123", "rebillid": "xyz789"}, + ], + }, + "token": "sensitive-token-value", + "terminalkey": "TinkoffBankTest", + "order_id": "ord_123", + } + } + } + out = scrub_pii_event(event, {}) + ctx = out["extra"]["checkout_context"] + assert ctx["buyer"]["customer_email"] == "[REDACTED]" + assert ctx["buyer"]["customer_phone"] == "[REDACTED]" + assert ctx["buyer"]["nested_list"][0]["pan"] == "[REDACTED]" + assert ctx["buyer"]["nested_list"][0]["expdate"] == "[REDACTED]" + assert ctx["buyer"]["nested_list"][1]["cardid"] == "[REDACTED]" + assert ctx["buyer"]["nested_list"][1]["rebillid"] == "[REDACTED]" + assert ctx["token"] == "[REDACTED]" + assert ctx["terminalkey"] == "[REDACTED]" + # non-PII поле остаётся. + assert ctx["order_id"] == "ord_123" + + def test_composed_before_send_scrubs_pii_and_token_together() -> None: """Композиция, реально используемая в `app.tgbot_main._before_send`: PII-scrub (ключ-based) И token-redaction (regex full-text) применяются оба, не заменяя @@ -262,6 +385,37 @@ def test_composed_before_send_scrubs_pii_and_token_together() -> None: assert "8663867262:AAExampleSecretPartAbCdEf123" not in frame_url +def test_composed_before_send_payment_wipe_pii_and_token_together() -> None: + """Полная композиция `app.main._before_send` (PR-D2): body-wipe для платёжного + пути → PII-scrub → token-redaction, в этом порядке, все три применяются.""" + event = { + "request": { + "url": "https://gendsgn.ru/api/v1/trade-in/payments/notify", + "data": {"Token": "deadbeef", "Pan": "220000******0000"}, + }, + "extra": {"client_phone": "+79991234567"}, + "exception": { + "values": [{"stacktrace": {"frames": [{"vars": {"url": _LEAKED_TOKEN_URL}}]}}] + }, + } + + def composed_before_send(evt, hint): + scrubbed = scrub_payment_request_body(evt, hint) + if scrubbed is None: + return None + scrubbed = scrub_pii_event(scrubbed, hint) + if scrubbed is None: + return None + return redact_telegram_bot_token(scrubbed, hint) + + out = composed_before_send(event, {}) + assert out is not None + assert "data" not in out["request"] + assert out["extra"]["client_phone"] == "[REDACTED]" + frame_url = out["exception"]["values"][0]["stacktrace"]["frames"][0]["vars"]["url"] + assert "8663867262:AAExampleSecretPartAbCdEf123" not in frame_url + + # ── RetryError fingerprint stabilization (glitchtip-noise, #) ─ # # tenacity.RetryError.__str__() тащит repr() последнего Future — memory address