From df9dd52996eac0df6f9134c6326b164f494088df Mon Sep 17 00:00:00 2001 From: bot-backend Date: Sun, 30 Aug 2026 00:11:56 +0500 Subject: [PATCH] =?UTF-8?q?fix(mera/public):=20Infinity/NaN=20=D0=B2=D0=BE?= =?UTF-8?q?=20=D0=B2=D1=85=D0=BE=D0=B4=D0=B5=20=E2=80=94=20422,=20=D0=B8?= =?UTF-8?q?=20=D0=B1=D1=8E=D0=B4=D0=B6=D0=B5=D1=82=20=D1=81=D1=87=D0=B8?= =?UTF-8?q?=D1=82=D0=B0=D0=B5=D1=82=20=D1=82=D0=B0=D0=BA=D0=B8=D0=B5=20?= =?UTF-8?q?=D0=B7=D0=B0=D0=BF=D1=80=D0=BE=D1=81=D1=8B?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Аудит живого сайта 30.08.2026: POST /api/public/mera/coverage с {"lat":56.8,"lon":1e400,...} отвечал 500, и двенадцать таких запросов подряд дали двенадцать пятисоток и ни одного 429. Две независимые поломки в одном месте, обе воспроизведены локально до правки. 1. 500 вместо 422. json.loads принимает Infinity/-Infinity/NaN, а 1e400 даёт inf переполнением. Pydantic отбивает такое поле по границам и кладёт значение в input ошибки, а ответ об ошибке сериализуется json.dumps(allow_nan=False) и падает уже после входа в ответ. Ломается не поле, а сборка ответа об ошибке — одна на всё приложение, поэтому и обработчик один (app/core/http_errors.py), а не валидатор на lon. 2. Лимитер мимо. _enforce стоял первой строкой тела хендлера, а FastAPI валидирует тело позже зависимостей, но раньше тела — до проверки просто не доходило. Та же поправка места, что уже сделана сегодня у _require_public_estimate_enabled: перенос в dependencies. Сделано для всех ручек файла, не только coverage. У /estimate и /estimate/read флаг остаётся первой зависимостью — 429 на выключенной ручке подтверждал бы её существование. Тесты двусторонние: снятие обработчика роняет 4 проверки 422, возврат лимитера в тело роняет проверку бюджета (проверено). --- tradein-mvp/backend/app/api/public/mera.py | 81 ++++++++++++++----- tradein-mvp/backend/app/core/http_errors.py | 59 ++++++++++++++ tradein-mvp/backend/app/main.py | 5 ++ .../backend/tests/test_public_mera_api.py | 66 +++++++++++++++ 4 files changed, 190 insertions(+), 21 deletions(-) create mode 100644 tradein-mvp/backend/app/core/http_errors.py diff --git a/tradein-mvp/backend/app/api/public/mera.py b/tradein-mvp/backend/app/api/public/mera.py index 7b8c4d85..db853872 100644 --- a/tradein-mvp/backend/app/api/public/mera.py +++ b/tradein-mvp/backend/app/api/public/mera.py @@ -70,6 +70,7 @@ import asyncio import hashlib import logging import secrets +from collections.abc import Callable from datetime import datetime from typing import Annotated, Literal @@ -185,6 +186,33 @@ def _enforce(limiter: SlidingWindowLimiter, request: Request, what: str) -> None limiter.record(ip) +def _budget(limiter: SlidingWindowLimiter, what: str) -> Callable[[Request], None]: + """Per-IP бюджет КАК ЗАВИСИМОСТЬ, а не первой строкой тела. + + Ровно та же поправка места, что уже сделана у + `_require_public_estimate_enabled` (см. его докстринг): FastAPI решает + зависимости РАНЬШЕ, чем валидирует тело, поэтому проверка в теле не + срабатывает на запросах, которые падают на разборе тела — до неё просто не + доходит. + + Для флага это стоило утечки схемы, для лимитера — неограниченного потока + отказов: 12 запросов подряд с некорректным телом на `/coverage` дали + двенадцать ответов и ни одного 429 (замер 30.08.2026). Пока такой вход + ронял сериализацию 422 (чинится в app/core/http_errors.py), это был поток + 500 с одного адреса; после починки — поток 422, но всё так же мимо бюджета. + + ЧЕГО ЭТА ЗАВИСИМОСТЬ НЕ ЛОВИТ: тело, которое не разбирается как JSON + вообще — там `RequestValidationError` летит до решения зависимостей. Такой + запрос стоит один `json.loads` и остаётся под общим `RateLimitMiddleware` + (300/60с на IP); заводить ради него middleware поверх middleware смысла нет. + """ + + def _dep(request: Request) -> None: + _enforce(limiter, request, what) + + return _dep + + class PublicSuggestInput(BaseModel): """Вход публичного автокомплита. @@ -231,9 +259,12 @@ def _query_with_city(query: str, city_hint: str | None) -> str: return f"{city_hint}, {query}" -@router.post("/suggest", response_model=SuggestResponse) +@router.post( + "/suggest", + response_model=SuggestResponse, + dependencies=[Depends(_budget(_suggest_limiter, "suggest"))], +) async def public_suggest( - request: Request, payload: PublicSuggestInput, db: Annotated[Session, Depends(get_db)], ) -> SuggestResponse: @@ -260,8 +291,6 @@ async def public_suggest( десятка в публичном UI не показывается, а каждый лишний кандидат может стоить внешнего вызова. """ - _enforce(_suggest_limiter, request, "suggest") - # Суточный потолок — ПОСЛЕ per-IP: сначала отсекаем одиночного абузера его # собственным лимитом, и только оставшееся считаем в общий бюджет. daily_retry = _daily_suggest_limiter.retry_after(_GLOBAL_KEY) @@ -302,9 +331,12 @@ async def public_suggest( _suggest_slots.release() -@router.post("/coverage", response_model=CoverageProbeResponse) +@router.post( + "/coverage", + response_model=CoverageProbeResponse, + dependencies=[Depends(_budget(_coverage_limiter, "coverage"))], +) def public_coverage( - request: Request, payload: CoverageProbeInput, db: Annotated[Session, Depends(get_db)], ) -> CoverageProbeResponse: @@ -318,7 +350,6 @@ def public_coverage( Ответ не содержит ни одной цены (см. `CoverageProbeResponse`) — бесплатный шаг доказывает наличие данных, цену продаёт платный. """ - _enforce(_coverage_limiter, request, "coverage") return coverage_probe(payload=payload, db=db) @@ -351,9 +382,12 @@ _STATS_SQL = text(""" """) -@router.get("/stats", response_model=dict[str, LandingStat]) +@router.get( + "/stats", + response_model=dict[str, LandingStat], + dependencies=[Depends(_budget(_stats_limiter, "stats"))], +) def public_stats( - request: Request, db: Annotated[Session, Depends(get_db)], ) -> dict[str, LandingStat]: """Витринные метрики лэндинга — готовый ночной срез (issue: числа по проду). @@ -374,8 +408,6 @@ def public_stats( `value` — числовое value_num, если оно есть; иначе value_text (для метрик, у которых значение не число). Оба NULL — отдаём null, а не выдуманный ноль. """ - _enforce(_stats_limiter, request, "stats") - rows = db.execute(_STATS_SQL).fetchall() return { row.metric: LandingStat( @@ -499,9 +531,12 @@ _SHOWCASE_SQL = text( ) -@router.get("/showcase", response_model=ShowcaseResponse) +@router.get( + "/showcase", + response_model=ShowcaseResponse, + dependencies=[Depends(_budget(_showcase_limiter, "showcase"))], +) def public_showcase( - request: Request, db: Annotated[Session, Depends(get_db)], ) -> ShowcaseResponse: """Витрина лэндинга: реальные ДКП-сделки против прогноза МЕРЫ. @@ -518,7 +553,6 @@ def public_showcase( годных строк не поместилось и по какому правилу отсеяно остальное. Числа считает пересчёт; без них витрина не имеет права подписаться честно. """ - _enforce(_showcase_limiter, request, "showcase") run = db.execute(_SHOWCASE_RUN_SQL).mappings().first() if run is None: return ShowcaseResponse(computed_at=None, deals=[], stats=None) @@ -693,7 +727,13 @@ def _coverage_for( @router.post( "/estimate", response_model=PublicEstimateResult, - dependencies=[Depends(_require_public_estimate_enabled)], + # Порядок несущий: флаг ПЕРВЫМ. Лимитер впереди него отвечал бы 429 на + # выключенной ручке, а несуществующий путь даёт 401 — то есть 429 снова + # подтверждал бы существование ручки, ровно то, что чинил флаг-гейт. + dependencies=[ + Depends(_require_public_estimate_enabled), + Depends(_budget(_estimate_limiter, "estimate")), + ], ) async def public_estimate( request: Request, @@ -714,8 +754,6 @@ async def public_estimate( контур (сосед, `/api/v1/trade-in/r/{token}`) открывает его после оплаты по своему токену. Наружу здесь уезжает только `PublicEstimateResult`. """ - _enforce(_estimate_limiter, request, "estimate") - daily_retry = _daily_estimate_limiter.retry_after(_ESTIMATE_GLOBAL_KEY) if daily_retry is not None: logger.error( @@ -773,10 +811,13 @@ async def public_estimate( @router.post( "/estimate/read", response_model=PublicEstimateResult, - dependencies=[Depends(_require_public_estimate_enabled)], + # Флаг первым — по той же причине, что у `/estimate`. + dependencies=[ + Depends(_require_public_estimate_enabled), + Depends(_budget(_estimate_read_limiter, "estimate-read")), + ], ) def public_estimate_read( - request: Request, payload: PublicEstimateTokenInput, db: Annotated[Session, Depends(get_db)], ) -> PublicEstimateResult: @@ -791,8 +832,6 @@ def public_estimate_read( Просрочка и «нет такого токена» отвечают ОДИНАКОВО (404): различать их значит подтверждать существование расчёта тому, кто угадал токен. """ - _enforce(_estimate_read_limiter, request, "estimate-read") - row = db.execute( text( """ diff --git a/tradein-mvp/backend/app/core/http_errors.py b/tradein-mvp/backend/app/core/http_errors.py new file mode 100644 index 00000000..ef2aba49 --- /dev/null +++ b/tradein-mvp/backend/app/core/http_errors.py @@ -0,0 +1,59 @@ +"""Ответ об ошибке валидации, который собирается при ЛЮБОМ входе. + +ЧТО СЛОМАЛОСЬ +------------- +Аудит живого сайта 30.08.2026: публичная проба покрытия отвечала 500 на входе, +который обязан отсеиваться валидацией:: + + POST /trade-in/api/public/mera/coverage + {"lat":56.8,"lon":1e400,"rooms":2,"area_m2":50} → 500 + +Разбор. `json.loads` принимает то, чего нет в стандарте JSON: литералы +`Infinity`, `-Infinity`, `NaN`, а `1e400` даёт `inf` переполнением. Pydantic +такое поле честно отбивает по границам (`lon: le=180`) и кладёт значение в +`input` ошибки. Дальше штатный обработчик FastAPI отдаёт перечень ошибок через +`JSONResponse`, а тот сериализует `json.dumps(..., allow_nan=False)` — и падает +уже ПОСЛЕ входа в ответ. Наружу это 500, то есть отказ сервера там, где +корректный ответ — 422. + +ПОЧЕМУ ОДИН ОБРАБОТЧИК, А НЕ ВАЛИДАТОР НА ПОЛЕ +---------------------------------------------- +Чинить по одному полю значит починить `lon` и оставить `lat`, `area_m2`, +`limit` и каждое число каждой будущей схемы. Ломается не поле: ломается +сборка ОТВЕТА об ошибке, одна на всё приложение. Здесь она и чинится. + +Отдельным модулем (а не строкой в `app/main.py`) ровно затем, чтобы тест мог +поставить ТОТ ЖЕ обработчик на своё маленькое приложение, не втягивая весь +граф роутеров: иначе тестовое приложение отвечало бы иначе, чем прод, и +проверка «422, а не 500» была бы зелёной по построению. +""" + +from __future__ import annotations + +import math + +from fastapi import FastAPI, Request +from fastapi.encoders import jsonable_encoder +from fastapi.exceptions import RequestValidationError +from fastapi.responses import JSONResponse + + +def _json_safe_float(value: float) -> float | str: + """inf/nan → строка. JSON их не умеет, а на ВХОД они приходят законно.""" + return value if math.isfinite(value) else str(value) + + +async def validation_error_handler(request: Request, exc: RequestValidationError) -> JSONResponse: + """Тот же стандартный `{"detail": [...]}`, но нефинитное число в `input` + едет строкой ("inf"/"nan") вместо того, чтобы ронять ответ.""" + return JSONResponse( + status_code=422, + content=jsonable_encoder( + {"detail": exc.errors()}, + custom_encoder={float: _json_safe_float}, + ), + ) + + +def install_validation_error_handler(app: FastAPI) -> None: + app.add_exception_handler(RequestValidationError, validation_error_handler) diff --git a/tradein-mvp/backend/app/main.py b/tradein-mvp/backend/app/main.py index 2b2f25b2..74c2d2ad 100644 --- a/tradein-mvp/backend/app/main.py +++ b/tradein-mvp/backend/app/main.py @@ -43,6 +43,7 @@ from app.core.auth_db import get_auth_engine from app.core.config import settings from app.core.db import SessionLocal from app.core.fdw import ensure_fdw_user_mapping +from app.core.http_errors import install_validation_error_handler from app.core.ratelimit import RateLimitMiddleware from app.core.rbac import rbac_guard from app.core.request_audit import RequestAuditMiddleware @@ -219,6 +220,10 @@ app = FastAPI( lifespan=lifespan, ) + +# 422 вместо 500 на Infinity/NaN во входе — разбор в app/core/http_errors.py. +install_validation_error_handler(app) + # 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. diff --git a/tradein-mvp/backend/tests/test_public_mera_api.py b/tradein-mvp/backend/tests/test_public_mera_api.py index 33a8ead9..474450ac 100644 --- a/tradein-mvp/backend/tests/test_public_mera_api.py +++ b/tradein-mvp/backend/tests/test_public_mera_api.py @@ -47,6 +47,7 @@ from fastapi.testclient import TestClient # noqa: E402 from app.api.public import mera as public_mera # noqa: E402 from app.api.v1.geocode import SuggestResponse # noqa: E402 from app.core.db import get_db # noqa: E402 +from app.core.http_errors import install_validation_error_handler # noqa: E402 from app.core.rbac import _PUBLIC_PATHS, rbac_guard # noqa: E402 from app.schemas.trade_in import CoverageProbeResponse # noqa: E402 @@ -94,6 +95,10 @@ def client() -> TestClient: """ app = FastAPI() app.middleware("http")(rbac_guard) + # Тот же обработчик 422, что вешает app/main.py. Без него тестовое + # приложение отвечало бы на нефинитные числа иначе, чем прод, и проверка + # «422, а не 500» была бы зелёной по построению. + install_validation_error_handler(app) app.include_router(public_mera.router, prefix=PREFIX) @app.get("/api/v1/trade-in/coverage") @@ -635,3 +640,64 @@ def test_suggest_passes_city_prefixed_query_downstream(client: TestClient) -> No assert captured["q"] == "Серов, Ленина 1" # Сам хинт продолжаем передавать: от него зависит гейт кадастрового тира. assert captured["city_hint"] == "Серов" + + +# ── 9. Невалидный вход: 422 и всё тот же бюджет ────────────────────────────── +# +# Аудит живого сайта 30.08.2026: POST /coverage с `"lon":1e400` отвечал 500, и +# двенадцать таких запросов подряд дали двенадцать пятисоток и ни одного 429. +# Две разные поломки в одном месте, поэтому и проверок здесь две. + +_JSON = {"content-type": "application/json"} + +# `json.loads` принимает нестандартные литералы Infinity/NaN, а `1e400` — это +# переполнение float. Ни одно из этих чисел не сериализуется обратно в JSON, +# поэтому они и роняли ответ об ошибке. Поля берём разные намеренно: чинить +# должно не поле, а сериализацию перечня ошибок. +_NON_FINITE_BODIES = [ + b'{"lat":56.838,"lon":1e400,"rooms":2,"area_m2":54.0}', + b'{"lat":56.838,"lon":NaN,"rooms":2,"area_m2":54.0}', + b'{"lat":Infinity,"lon":60.597,"rooms":2,"area_m2":54.0}', + b'{"lat":56.838,"lon":60.597,"rooms":2,"area_m2":-Infinity}', +] + + +@pytest.mark.parametrize("body", _NON_FINITE_BODIES) +def test_non_finite_number_is_422_not_500(client: TestClient, body: bytes) -> None: + """Infinity/NaN во входе — это невалидный вход, а не отказ сервера. + + Красный вид этого теста без починки — не «assert 500 != 422», а + необработанный ValueError из `json.dumps(..., allow_nan=False)`: он летит + сквозь TestClient. Оба исхода одинаково красные и оба про одно: ответ об + ошибке не собрался. + """ + resp = client.post(f"{PREFIX}/coverage", content=body, headers=_JSON) + + assert resp.status_code == 422, resp.text + # Форма ответа остаётся стандартной, иначе фронт разбирает её иначе. + assert isinstance(resp.json()["detail"], list) + + +@pytest.mark.parametrize("route", ["/coverage", "/suggest"]) +def test_invalid_body_still_spends_the_per_ip_budget(client: TestClient, route: str) -> None: + """Бюджет обязан срабатывать РАНЬШЕ разбора тела. + + Иначе он не защищает ровно от того, что на разборе тела и падает: клиент + льёт неограниченный поток отказов с одного адреса. + + Двусторонность: верните `_enforce(...)` первой строкой тела хендлера — и + все ответы станут 422, ни одного 429, тест покраснеет. + """ + limit = public_mera._COVERAGE_LIMIT if route == "/coverage" else public_mera._SUGGEST_LIMIT + codes = [ + client.post( + f"{PREFIX}{route}", content=b'{"lat":56.838,"lon":1e400}', headers=_JSON + ).status_code + for _ in range(limit + 3) + ] + + assert 429 in codes, f"бюджет не сработал: {codes}" + # Ничего третьего быть не должно — ни 500, ни внезапной 200 на мусоре. + assert set(codes) <= {422, 429}, codes + # Отказ начинается ровно после исчерпания окна, а не «когда-нибудь». + assert codes.index(429) == limit, codes