Some checks failed
CI Trade-In / changes (pull_request) Successful in 9s
CI / changes (pull_request) Failing after 8s
CI / backend-tests (pull_request) Has been skipped
CI / frontend-tests (pull_request) Has been skipped
CI / openapi-codegen-check (pull_request) Has been skipped
CI Trade-In / browser-tests (pull_request) Successful in 46s
CI Trade-In / frontend-checks (pull_request) Successful in 1m3s
CI Trade-In / backend-tests (pull_request) Successful in 4m42s
Ревью четырьмя независимыми линзами (периметр, семантика Caddy, политика ПДн
против кода, фронт) + по два проверяющих на каждую находку. Ниже — то, что
пережило проверку и воспроизведено на живом коде, а не выведено из чтения.
## Caddy: открытый редирект и потерянные ссылки
Захват хвоста регекспом (`^/trade-in/mera-public/(.+)$` → `redir /{re…1}`) —
открытый редирект. Захват берётся из РАСКОДИРОВАННОГО пути, поэтому
`/trade-in/mera-public/%5Cevil.example/pay` даёт цель `/\evil.example/pay`, а
браузеры трактуют `/\` как `//` — Location уводит на чужой хост. Готовая
фишинговая заготовка с домена, который напечатан внутри оферты и уходит
модератору эквайера. Заменено поимённым списком путей: такой адрес просто не
матчится.
Адреса со слэшем на конце (`/oferta/`, и длинные `…/oferta/`) отдавали 404 —
ровно те ссылки, ради сохранности которых редирект и делался. Добавлена
нормализация, цепочка замкнута (проверено: 2 перехода → 200).
Query-строка терялась: размещённые ссылки с UTM приходили бы в аналитику как
прямой заход. `uri strip_prefix` + `{uri}` переносит её. Обёртка `route`
обязательна — без неё `redir` выполняется раньше `uri` и Location равен
исходному адресу (бесконечный цикл, поймано на стенде).
`/v3` — черновое превью с маркетинговыми плейсхолдерами — было открыто на
боевом домене молча. Теперь названо вслух и запинено тестом.
## Гейты, которых не было
`caddy validate` не звал НИ ОДИН workflow, а deploy применяет конфиг не через
`reload` (тот отказался бы принять битый), а через `up -d --force-recreate` —
опечатка уводит контейнер в crash-loop и роняет ВСЕ домены. Добавлен гейт в
ci.yml, тем же образом caddy:2, что и на проде.
Проверка «роут ↔ Caddy» была односторонней и пропускала обратную ошибку —
путь, открытый наружу, о котором приложение не знает. Так и уехал `/v3`.
Теперь двусторонняя, плюс проверка, что для каждой страницы есть 301.
## Бюджет внешнего геокодера
Per-IP окна ограничивают одного клиента, но не сумму: 40/мин с адреса — это
57 600 в сутки при бесплатном тире DaData в 10 000, ОБЩЕМ с закрытым контуром.
Подтверждено на проде: достаточно упомянуть не-екатеринбургский город, чтобы
локальный тир отключился и запрос гарантированно ушёл во внешний сервис. То
есть один скрипт оставлял без подсказок платящих пилотов.
Per-IP снижен до 20/мин, добавлен общий суточный потолок 2000 и потолок
одновременных подсказок (4): кадастровый тир уходит в FDW-скан чужой базы,
держит соединение около секунды, а пул общий с B2B — полтора десятка
параллельных публичных запросов клали бы закрытый контур.
## «Адрес нигде не сохраняется» — теперь правда целиком
Две утечки, обе воспроизведены:
1. ЖУРНАЛЫ. Геокодер печатает введённую строку открытым текстом на каждый
вызов, прод пишет stdout в persistent journald — адрес ложился на диск
рядом с IP того же запроса в access-логе Caddy. Закрыто фильтром логов на
время публичного запроса (contextvar, переживает await и to_thread).
Закрытый контур логи сохраняет: они нужны для разбора жалоб пилотов.
2. МОНИТОРИНГ. sentry_sdk кладёт в событие ПОЛНОЕ тело запроса — а тело
публичной ручки это ровно `{"q": "<адрес>"}`; `send_default_pii=False` тут
не гейт, он про куки. Плюс брэдкрамб httpx несёт адрес в query геокодера.
Закрыто `scrub_public_address`.
Текст п. 5.4 политики расширен до «ни в журналы веб-сервера, ни в технические
журналы, ни в мониторинг» — ровно то, что теперь обеспечено кодом.
## Фронт
- Отмена запроса подсказок откладывалась внутрь следующего debounce-такта и
не наступала вовсе, если человек переставал печатать: ответ по старой строке
долетал и ложился в список. Контроллер создаётся сразу, отменяется в cleanup.
- Список схлопывался на каждое нажатие — клик по намеченному пункту
промахивался. Старая выдача висит, пока не пришла новая.
- «Комнат» с лэндинга — свободный текст: «студия» не совпадала ни с одним
option, селект показывал пустоту, parseInt давал NaN, на сервер уходил
rooms: null → 422 с текстом «сломалось на нашей стороне». Нормализация
вынесена чистой функцией и покрыта тестами.
- У пробы покрытия не было ни таймаута, ни отмены: оборванное соединение
оставляло кнопку в «Смотрим данные…» навсегда. 15 с + понятный текст.
- Ошибка подсказок глушилась в пустой список — тупик без объяснения.
- Комбобокс: Tab проваливался в кнопки подсказок, список не закрывался по
уходу фокуса и перекрывал поля, Escape оставлял висячий aria-activedescendant.
## Проверено
Локальный стенд (реальный site-блок Caddy + заглушка): 18 маршрутов, включая
`%5C`, `//`, `%2F` — все три теперь 404. vitest 55 passed, backend 17 passed по
публичному API, tsc, lint, build, isolation guard 41 файл, caddy validate.
Мутации: снять редакцию логов → падает тест журналов; не вырезать тело запроса
→ падает тест мониторинга; убрать /estimate из Caddy → падает тест маршрутов.
250 lines
16 KiB
Python
250 lines
16 KiB
Python
"""Публичный API МЕРЫ (B2C, meraocenka.ru) — анонимный, ровно две ручки.
|
||
|
||
ЗАЧЕМ ОТДЕЛЬНЫЙ ПРЕФИКС, А НЕ ОТКРЫТИЕ КУСКА /api/v1/*
|
||
-------------------------------------------------------
|
||
На `meraocenka.ru` действует allowlist-by-default: Caddy проксирует поимённо
|
||
перечисленные пути, всё остальное — 404 (см. корневой Caddyfile, site-блок
|
||
meraocenka.ru; регресс — scripts/smoke-mera-perimeter.sh). Чтобы открыть там
|
||
API, нужно было выбрать одно из двух:
|
||
|
||
(а) пробросить `/trade-in/api/v1/trade-in/coverage` и `.../geocode/suggest`
|
||
поимённо — периметр остаётся узким, но одна опечатка в matcher'е
|
||
(`/trade-in/api/*` вместо точного пути) открывает наружу ВЕСЬ v1: ~20
|
||
ручек, включая PDF расчётов, фотографии объектов, историю и админку;
|
||
(б) завести отдельный префикс, под которым по определению не может лежать
|
||
ничего закрытого, и пробрасывать его целиком.
|
||
|
||
Выбрано (б). Разница не в удобстве, а в цене ошибки: при (а) безопасность
|
||
периметра держится на аккуратности матчера, при (б) — на структуре кода.
|
||
Добавить сюда ручку с приватными данными нужно СПЕЦИАЛЬНО (положить файл в
|
||
`app/api/public/`), случайно — нельзя.
|
||
|
||
Тот же принцип, что уже применён на фронте: публичный лэндинг вынесен в
|
||
`app/mera-public/` с guard-скриптом на граф импортов, а не помечен флагом
|
||
внутри общего дерева.
|
||
|
||
АНОНИМНОСТЬ
|
||
-----------
|
||
`rbac_guard` (app/core/rbac.py) требует `X-Authenticated-User` для любого
|
||
non-public пути. Обе ручки перечислены в `_PUBLIC_PATHS` ТОЧНЫМИ строками —
|
||
не префиксом: множество там — frozenset с проверкой `path in ...`, и
|
||
добавление префиксной ветки ради двух путей расширило бы механизм, которым
|
||
пользуется весь бэкенд, ради одной фичи.
|
||
|
||
ЧТО ЭТИ РУЧКИ НЕ ДЕЛАЮТ
|
||
-----------------------
|
||
Ни одна из них не пишет в БД строк с адресом пользователя: `/coverage` —
|
||
чистое чтение (один SELECT), `/suggest` — прокси автокомплита. Это не
|
||
случайность, а условие, при котором публичная форма может работать ДО того,
|
||
как появится контур согласия 152-ФЗ (issue #2895: сегодня адрес физлица
|
||
попадает в `trade_in_estimates` раньше любого согласия, а пути удаления
|
||
данных в бэкенде нет). Платный расчёт, который писать будет, открывается
|
||
отдельно и только вместе с этим контуром.
|
||
|
||
БЮДЖЕТЫ
|
||
-------
|
||
Общий `RateLimitMiddleware` (300/60с на IP) здесь недостаточен: `/suggest`
|
||
через DaData-тир геокодера — платный внешний вызов, то есть абуз стоит денег,
|
||
а не только CPU. Поэтому у каждой ручки свой, заведомо более узкий per-IP
|
||
бюджет поверх общего — тот же приём, что у анонимного чата поддержки
|
||
(app/api/v1/support.py, `_anon_ip_limiter`).
|
||
|
||
Лимитеры in-process: при нескольких репликах бэкенда бюджет умножится на их
|
||
число. Сейчас реплика одна (docker-compose, tradein-backend), что и делает
|
||
допущение верным; при масштабировании — выносить в Redis (issue заводить
|
||
тогда же, не раньше: преждевременный вынос добавит зависимость без выигрыша).
|
||
"""
|
||
|
||
from __future__ import annotations
|
||
|
||
import asyncio
|
||
import logging
|
||
from typing import Annotated
|
||
|
||
from fastapi import APIRouter, Depends, HTTPException, Request
|
||
from pydantic import BaseModel, Field
|
||
from sqlalchemy.orm import Session
|
||
|
||
from app.api.v1.geocode import SuggestResponse, suggest_addresses
|
||
from app.api.v1.trade_in import coverage_probe
|
||
from app.core.db import get_db
|
||
from app.core.public_request import install_address_log_redaction, public_request_scope
|
||
from app.core.ratelimit import SlidingWindowLimiter, _client_ip
|
||
from app.schemas.trade_in import CoverageProbeInput, CoverageProbeResponse
|
||
|
||
logger = logging.getLogger(__name__)
|
||
|
||
# Публичная форма обещает, что введённый адрес нигде не сохраняется. По базам
|
||
# это так, по журналам не было — геокодер печатал запрос открытым текстом, а
|
||
# прод пишет stdout в persistent journald. Ставим редакцию логов в момент
|
||
# импорта модуля (его импортирует app/main.py) — то есть ровно тогда, когда
|
||
# публичные ручки вообще появляются в приложении. Разбор — в
|
||
# app/core/public_request.py.
|
||
install_address_log_redaction()
|
||
|
||
router = APIRouter()
|
||
|
||
# Бюджеты подобраны от живого сценария, а не «на глаз»: человек набирает адрес
|
||
# с debounce'ом — это единицы запросов на один адрес, поэтому 40/мин хватает
|
||
# на несколько попыток подряд и режет перебор словарём. Проба покрытия — шаг
|
||
# осознанный (нажатие кнопки), 15/мин с запасом покрывает «поправил площадь,
|
||
# нажал ещё раз».
|
||
_SUGGEST_LIMIT = 20
|
||
_COVERAGE_LIMIT = 15
|
||
_WINDOW_S = 60.0
|
||
|
||
_suggest_limiter = SlidingWindowLimiter(limit=_SUGGEST_LIMIT, window_s=_WINDOW_S)
|
||
_coverage_limiter = SlidingWindowLimiter(limit=_COVERAGE_LIMIT, window_s=_WINDOW_S)
|
||
|
||
# ── Общий суточный потолок публичных подсказок ──────────────────────────────
|
||
#
|
||
# Per-IP окна одного клиента ограничивают, но не ограничивают СУММУ. Считаем:
|
||
# 20 запросов/мин с одного адреса — это 28 800 в сутки, а весь бесплатный тир
|
||
# DaData у проекта — 10 000 в сутки И ОН ОБЩИЙ с закрытым контуром. То есть без
|
||
# этого потолка один настойчивый клиент (или один скрипт) за несколько часов
|
||
# выедает квоту, и подсказки перестают работать у ПЛАТЯЩИХ пилотов, а не только
|
||
# у него. Найдено состязательным ревью и подтверждено на проде: достаточно
|
||
# упомянуть в запросе не-екатеринбургский город, чтобы локальный кадастровый
|
||
# тир отключился и запрос гарантированно ушёл во внешний сервис.
|
||
#
|
||
# 2000/сутки — заведомо меньше десятой доли тира: публичная форма не должна
|
||
# уметь навредить закрытому контуру в принципе. Порог достижим только абузом
|
||
# (живой посетитель тратит единицы запросов на адрес), поэтому исчерпание —
|
||
# сигнал, а не штатный режим: логируем ошибкой.
|
||
_DAILY_SUGGEST_BUDGET = 2000
|
||
_daily_suggest_limiter = SlidingWindowLimiter(limit=_DAILY_SUGGEST_BUDGET, window_s=86_400.0)
|
||
_GLOBAL_KEY = "public-suggest"
|
||
|
||
# ── Потолок одновременных подсказок ─────────────────────────────────────────
|
||
#
|
||
# Кадастровый тир геокодера уходит в FDW-скан ЧУЖОЙ базы (gendesign) и на
|
||
# коротком вводе занимает около секунды, всё это время удерживая соединение из
|
||
# пула. Пул общий с закрытым контуром и невелик (дефолт SQLAlchemy 5+10), так
|
||
# что полтора десятка одновременных публичных подсказок способны положить
|
||
# B2B-запросы в том же процессе — при том, что per-IP лимиты каждого из них
|
||
# формально соблюдены.
|
||
#
|
||
# Ждём слот недолго и отвечаем 429, а не копим очередь: очередь под нагрузкой
|
||
# превращается в те же занятые соединения плюс растущий таймаут у клиента.
|
||
_SUGGEST_CONCURRENCY = 4
|
||
_SUGGEST_SLOT_WAIT_S = 2.0
|
||
_suggest_slots = asyncio.Semaphore(_SUGGEST_CONCURRENCY)
|
||
|
||
|
||
def _enforce(limiter: SlidingWindowLimiter, request: Request, what: str) -> None:
|
||
"""429 при превышении per-IP бюджета. Попытку регистрируем ДО работы ручки.
|
||
|
||
В отличие от отправки сообщения в поддержку (там `record()` только на
|
||
успех, чтобы неудача не съедала бюджет), здесь считаем каждую попытку:
|
||
внешний вызов геокодера тратится и на запросе, который вернёт пусто, —
|
||
иначе перебор мусорными строками не стоил бы атакующему ничего.
|
||
"""
|
||
ip = _client_ip(request)
|
||
retry_after = limiter.retry_after(ip)
|
||
if retry_after is not None:
|
||
logger.info("public mera %s rate-limited for %s", what, ip)
|
||
raise HTTPException(
|
||
status_code=429,
|
||
detail="Слишком много запросов. Попробуйте через минуту.",
|
||
headers={"Retry-After": str(int(retry_after) + 1)},
|
||
)
|
||
limiter.record(ip)
|
||
|
||
|
||
class PublicSuggestInput(BaseModel):
|
||
"""Вход публичного автокомплита.
|
||
|
||
Телом, а не query-параметрами — см. `public_suggest`.
|
||
"""
|
||
|
||
q: str = Field(min_length=2, max_length=200)
|
||
limit: int = Field(default=8, ge=1, le=10)
|
||
city_hint: str | None = Field(default=None, max_length=100)
|
||
|
||
|
||
@router.post("/suggest", response_model=SuggestResponse)
|
||
async def public_suggest(
|
||
request: Request,
|
||
payload: PublicSuggestInput,
|
||
db: Annotated[Session, Depends(get_db)],
|
||
) -> SuggestResponse:
|
||
"""Автокомплит адреса для публичной формы (Свердловская область).
|
||
|
||
ПОЧЕМУ POST У ЧИТАЮЩЕЙ РУЧКИ. Каноничнее был бы GET с `?q=`. Но на
|
||
публичном домене включён access-лог (`/var/log/caddy/meraocenka.ru.log`), а
|
||
он пишет URI целиком — то есть адрес квартиры лёг бы в файл рядом с IP
|
||
посетителя. Мы публично обещаем на этой же странице, что введённый адрес
|
||
нигде не сохраняем; лог — это сохранение. Тело запроса в лог не попадает,
|
||
поэтому обещание остаётся правдой без правки конфигурации логирования (её
|
||
легко потерять при следующем рефакторинге Caddyfile — а тип запроса
|
||
потерять нельзя, сломается сразу и заметно).
|
||
|
||
Тот же довод, что у черновика с лэндинга: он едет через sessionStorage, а
|
||
не через query-параметры (frontend `estimate-draft.ts`).
|
||
|
||
Делегирует В ТУ ЖЕ функцию, что обслуживает B2B-экран
|
||
(`app.api.v1.geocode.suggest_addresses`), а не повторяет её логику:
|
||
публичная форма обязана резолвить адрес ровно так же, как платный расчёт,
|
||
иначе аноним выберет дом, которого потом «не окажется».
|
||
|
||
Отличие от v1 ровно одно — потолок `limit` 10 вместо 15: выдача сверх
|
||
десятка в публичном UI не показывается, а каждый лишний кандидат может
|
||
стоить внешнего вызова.
|
||
"""
|
||
_enforce(_suggest_limiter, request, "suggest")
|
||
|
||
# Суточный потолок — ПОСЛЕ per-IP: сначала отсекаем одиночного абузера его
|
||
# собственным лимитом, и только оставшееся считаем в общий бюджет.
|
||
daily_retry = _daily_suggest_limiter.retry_after(_GLOBAL_KEY)
|
||
if daily_retry is not None:
|
||
logger.error(
|
||
"публичные подсказки исчерпали суточный бюджет (%d) — квота геокодера "
|
||
"защищена, но форма на лэндинге сейчас без автокомплита",
|
||
_DAILY_SUGGEST_BUDGET,
|
||
)
|
||
raise HTTPException(
|
||
status_code=429,
|
||
detail="Подсказки адреса временно недоступны. Введите адрес полностью.",
|
||
headers={"Retry-After": str(int(daily_retry) + 1)},
|
||
)
|
||
_daily_suggest_limiter.record(_GLOBAL_KEY)
|
||
|
||
try:
|
||
await asyncio.wait_for(_suggest_slots.acquire(), timeout=_SUGGEST_SLOT_WAIT_S)
|
||
except TimeoutError:
|
||
raise HTTPException(
|
||
status_code=429,
|
||
detail="Сервис сейчас занят. Попробуйте ещё раз через несколько секунд.",
|
||
headers={"Retry-After": "5"},
|
||
) from None
|
||
|
||
try:
|
||
# Пометка публичного запроса нужна ровно здесь: внутри `suggest_addresses`
|
||
# геокодер логирует введённую строку, а публичная форма обещает, что
|
||
# адрес не попадает в журналы.
|
||
with public_request_scope():
|
||
return await suggest_addresses(
|
||
q=payload.q, limit=payload.limit, db=db, city_hint=payload.city_hint
|
||
)
|
||
finally:
|
||
_suggest_slots.release()
|
||
|
||
|
||
@router.post("/coverage", response_model=CoverageProbeResponse)
|
||
def public_coverage(
|
||
request: Request,
|
||
payload: CoverageProbeInput,
|
||
db: Annotated[Session, Depends(get_db)],
|
||
) -> CoverageProbeResponse:
|
||
"""Бесплатная проба покрытия (issue #2894) для публичной формы.
|
||
|
||
Делегирует в `app.api.v1.trade_in.coverage_probe` — ту же функцию, что
|
||
вызывает закрытый контур. Копии SQL здесь нет намеренно: разбор #2894
|
||
показал, что стоит когорте пробы разойтись с когортой платного расчёта —
|
||
проба честно отвечает «есть данные» там, где расчёт увидит ноль.
|
||
|
||
Ответ не содержит ни одной цены (см. `CoverageProbeResponse`) — бесплатный
|
||
шаг доказывает наличие данных, цену продаёт платный.
|
||
"""
|
||
_enforce(_coverage_limiter, request, "coverage")
|
||
return coverage_probe(payload=payload, db=db)
|