fix(tradein/security): утечка ключа прокси, аудит действий админа, отличимость неудачного входа, IDOR в заявке #2536

Merged

lekss361 merged 2 commits from fix/tradein-audit-security into main

2026-07-26 22:42:16 +00:00

lekss361 commented

2026-07-26 20:59:08 +00:00

Owner

Четыре правки по аудиту.

1. Ключ прокси утекал в ответ API и в трекер ошибок

admin.py:2397 — ручка смены IP возвращала клиенту str(exc), а внутри был полный URL смены IP с ключом в строке запроса. Тот же секрет попадал в breadcrumb трекера.

Клиенту теперь нейтральное reason="changeip request failed", детали только в лог.
В sentry_scrub.py добавлен _redact_url_secrets_inplace — вычищает распространённые имена секретных параметров запроса (api_key, proxy_key, token, secret, password, access_token, auth). Встроен внутрь scrub_pii_event, то есть композицией, а не заменой — это разные классы секретов с разными механизмами обнаружения. Реализован in-place, чтобы сохранить identity объекта события, которую проверяют существующие тесты.

Контекст: в проекте это уже третий случай секрета в URL. Штатный sanitize_url из SDK режет только user:pass@ и параметры запроса, но не путь — для токена Telegram пришлось делать отдельный redact-regex.

2. Действия администратора не оставляли следа

Загрузка кук, авто-логин, правка прокси и настроек скраперов не писались в аудит — установить, кто именно это сделал, было нельзя.

Правка сделана в middleware, а не в admin.py: одним изменением покрыты все ~80 админских ручек. Пишется admin_action с атрибуцией (пользователь, адрес, путь, метод, статус) и без тела запроса — там куки и пароли. Дашборд audit.py читает события без фильтра по типу, поэтому новые появятся там автоматически.

3. Неудачный вход был неотличим от успешного

request_audit.py:32 — код ответа не сохранялся. Теперь читается, и событие расходится на login и login_failed.

Проверено, что middleware аудита внешний по отношению к защитному слою (порядок регистрации в main.py даёт обратную вложенность), поэтому к моменту чтения статуса 401 и 403 от защиты уже проставлены.

Про шум сканеров, который я отдельно заметил в трекере (31 запись из 50 за сутки — долбёжка в /wp-login.php, /.git/config): отдельная фильтрация не нужна и добавлена не была. Этот шум структурно не может попасть в события: Caddy отсекает его basic-auth'ом до проксирования, а код срабатывает только при наличии заголовка пользователя. Задокументировано в файле.

Известный компромисс, прописанный в коде: корзина дедупликации событий входа (одно на пользователя+адрес+устройство в день) не разделена на успешные и неуспешные — это требует правки user_events.py, вне границ задачи. Если в один день будет сначала отказ, а потом реальный вход с того же устройства, второй не запишется. Объём событий не изменился, просто единственное событие дня теперь корректно отражает свой исход.

4. Заявку можно было привязать к чужой оценке

lead.py:80 — POST /lead принимал estimate_id и не проверял владельца. Переиспользован тот же guard «владелец или админ», что и в GET /estimate/{id}, без правок в самом trade_in.py.

Test plan

Новые тесты: секрет не в ответе на ошибку смены IP; admin_action с атрибуцией и без тела; login против login_failed; чужой estimate_id в заявке → 404, обход админом, 401/403
Обновлены 2 существующих теста под изменившуюся семантику
uv run pytest -q — 2635 passed, 8 skipped
uv run ruff check с проектным конфигом — чисто
Реальное поведение трекера не проверялось — нет доступа к боевому инстансу; вывод про интеграцию httpx сделан по исходникам SDK

Четыре правки по аудиту. ## 1. Ключ прокси утекал в ответ API и в трекер ошибок `admin.py:2397` — ручка смены IP возвращала клиенту `str(exc)`, а внутри был полный URL смены IP с ключом в строке запроса. Тот же секрет попадал в breadcrumb трекера. - Клиенту теперь нейтральное `reason="changeip request failed"`, детали только в лог. - В `sentry_scrub.py` добавлен `_redact_url_secrets_inplace` — вычищает распространённые имена секретных параметров запроса (`api_key`, `proxy_key`, `token`, `secret`, `password`, `access_token`, `auth`). Встроен **внутрь** `scrub_pii_event`, то есть композицией, а не заменой — это разные классы секретов с разными механизмами обнаружения. Реализован in-place, чтобы сохранить identity объекта события, которую проверяют существующие тесты. Контекст: в проекте это уже третий случай секрета в URL. Штатный `sanitize_url` из SDK режет только `user:pass@` и параметры запроса, но не путь — для токена Telegram пришлось делать отдельный redact-regex. ## 2. Действия администратора не оставляли следа Загрузка кук, авто-логин, правка прокси и настроек скраперов не писались в аудит — установить, кто именно это сделал, было нельзя. Правка сделана в middleware, а не в `admin.py`: одним изменением покрыты все ~80 админских ручек. Пишется `admin_action` с атрибуцией (пользователь, адрес, путь, метод, статус) и **без тела запроса** — там куки и пароли. Дашборд `audit.py` читает события без фильтра по типу, поэтому новые появятся там автоматически. ## 3. Неудачный вход был неотличим от успешного `request_audit.py:32` — код ответа не сохранялся. Теперь читается, и событие расходится на `login` и `login_failed`. Проверено, что middleware аудита **внешний** по отношению к защитному слою (порядок регистрации в `main.py` даёт обратную вложенность), поэтому к моменту чтения статуса 401 и 403 от защиты уже проставлены. Про шум сканеров, который я отдельно заметил в трекере (31 запись из 50 за сутки — долбёжка в `/wp-login.php`, `/.git/config`): отдельная фильтрация не нужна и добавлена не была. Этот шум структурно не может попасть в события: Caddy отсекает его basic-auth'ом **до** проксирования, а код срабатывает только при наличии заголовка пользователя. Задокументировано в файле. **Известный компромисс, прописанный в коде:** корзина дедупликации событий входа (одно на пользователя+адрес+устройство в день) не разделена на успешные и неуспешные — это требует правки `user_events.py`, вне границ задачи. Если в один день будет сначала отказ, а потом реальный вход с того же устройства, второй не запишется. Объём событий не изменился, просто единственное событие дня теперь корректно отражает свой исход. ## 4. Заявку можно было привязать к чужой оценке `lead.py:80` — `POST /lead` принимал `estimate_id` и не проверял владельца. Переиспользован тот же guard «владелец или админ», что и в `GET /estimate/{id}`, без правок в самом `trade_in.py`. ## Test plan - [x] Новые тесты: секрет не в ответе на ошибку смены IP; `admin_action` с атрибуцией и без тела; `login` против `login_failed`; чужой `estimate_id` в заявке → 404, обход админом, 401/403 - [x] Обновлены 2 существующих теста под изменившуюся семантику - [x] `uv run pytest -q` — 2635 passed, 8 skipped - [x] `uv run ruff check` с проектным конфигом — чисто - [ ] Реальное поведение трекера не проверялось — нет доступа к боевому инстансу; вывод про интеграцию httpx сделан по исходникам SDK

lekss361 added 2 commits 2026-07-26 20:59:09 +00:00

fix(tradein/security): audit findings — proxy-key leak, admin audit, login status, lead IDOR 2cfd112c53

Четыре независимых security-audit находки:

1. rotate_proxy_ip (admin.py) отдавал str(httpx exc) клиенту и в Sentry —
mobileproxy changeip-URL несёт API-ключ в query-string. Ответ теперь
нейтральный ("changeip request failed"); sentry_scrub.py получил
composable full-text редактор секрет-подобных query-параметров
(?token=/?proxy_key=/?api_key=/... — не завязан на конкретного провайдера),
встроенный в scrub_pii_event (in-place, сохраняет event identity) — main.py
не тронут, редактор подключается автоматически через существующую ссылку.

2. Меняющие состояние /api/v1/admin/* ручки (куки, авто-логin, прокси,
настройки скрапера, bulk) были полностью исключены из user_events —
установить, кто их вызвал, было нельзя. RequestAuditMiddleware теперь
пишет `admin_action` для POST/PUT/PATCH/DELETE на /admin/* с атрибуцией
(username/ip/path/method/status), БЕЗ тела запроса (там секреты). GET
дашборды по-прежнему не логируются (design как раньше — не шумят).

3. login-событие писалось безусловно, без учёта response.status_code —
отражённая RBAC-попытка (протухший внутренний секрет / неизвестная роль /
scope-блок) была неотличима от настоящего входа. Теперь event_type
расходится на login/login_failed по фактическому статусу ответа; payload
несёт status_code. Дедуп-бакет (once/user+ip+ua+day) оставлен как есть —
разбивка на success/fail потребовала бы правки user_events.py (вне
scope); задокументировано как известный trade-off.
Побочный вопрос (шум basic_auth 401 от сканеров) не требует доп. фильтра
здесь: Caddy гейтит basic_auth ДО проксирования — трафик без валидного
X-Authenticated-User в этот код вообще не попадает.

4. POST /lead принимал estimate_id без проверки владельца — можно было
привязать заявку к чужой оценке. Переиспользован owner-or-admin guard
_assert_estimate_access из trade_in.py (тот же подход, что #690).

Тесты на каждый пункт (secret не в ответе/событии, admin_action с
атрибуцией и без body, login vs login_failed, чужой estimate_id -> 404) +
regression на существующие сьюты. Полный `pytest -q --deselect
tests/test_search_api.py::test_search_cache_hit`: 2635 passed, 8 skipped.

Merge remote-tracking branch 'forgejo/main' into fix/tradein-audit-security

CI Trade-In / backend-tests (pull_request) Successful in 5m0s

Details

CI Trade-In / changes (pull_request) Successful in 14s

Details

CI / changes (pull_request) Successful in 13s

Details

CI Trade-In / frontend-checks (pull_request) Has been skipped

Details

CI / backend-tests (pull_request) Has been skipped

Details

CI / openapi-codegen-check (pull_request) Has been skipped