From 0ef1880e55b60e51209ad05bc8fb879c799a7298 Mon Sep 17 00:00:00 2001 From: bot-backend Date: Sat, 5 Sep 2026 22:51:22 +0500 Subject: [PATCH 1/2] =?UTF-8?q?fix(rbac):=20=D0=BD=D0=B5=20=D0=BE=D1=82?= =?UTF-8?q?=D0=B2=D0=B5=D1=87=D0=B0=D1=82=D1=8C=20401=20=D0=B7=D0=B0=20?= =?UTF-8?q?=D0=BD=D0=B5=D1=81=D1=83=D1=89=D0=B5=D1=81=D1=82=D0=B2=D1=83?= =?UTF-8?q?=D1=8E=D1=89=D0=B8=D0=B9=20=D0=BF=D1=83=D1=82=D1=8C=20(#3324)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rbac_guard — HTTP-middleware, он отрабатывает до роутинга и потому отвечал 401 с rbac-текстом даже на пути, которых в приложении нет. Аноним получал бесплатный оракул периметра: мусор под «интересным» префиксом давал 401, а такой же мусор под публичным префиксом — 404 роутера, то есть выключенная ручка была отличима от несуществующей. Guard теперь пропускает запрос дальше, если ни один маршрут роутера не матчится (Match.NONE у всех) — 404 отдаёт тот же роутер, что и на любой другой мусор. Существующие маршруты не затронуты: Match.PARTIAL (путь есть, метод другой) по-прежнему идёт в guard, реальный закрытый маршрут анониму даёт 401, публичный — работает без идентичности. --- tradein-mvp/backend/app/core/rbac.py | 32 ++++++++++++++++++++++ tradein-mvp/backend/tests/test_auth_api.py | 13 +++++++-- tradein-mvp/backend/tests/test_rbac.py | 31 +++++++++++++++++++++ 3 files changed, 73 insertions(+), 3 deletions(-) diff --git a/tradein-mvp/backend/app/core/rbac.py b/tradein-mvp/backend/app/core/rbac.py index 62701225..67b2d736 100644 --- a/tradein-mvp/backend/app/core/rbac.py +++ b/tradein-mvp/backend/app/core/rbac.py @@ -35,6 +35,7 @@ from typing import Any from fastapi import Request from fastapi.responses import JSONResponse, Response +from starlette.routing import Match from app.core.auth import get_role, is_path_allowed from app.core.config import settings @@ -181,6 +182,32 @@ def _db_role_path_allowed(role: str, path: str) -> bool: return any(_db_glob_match(p, path) for p in paths) +def _path_is_routed(request: Request) -> bool: + """Есть ли у пути хоть один маршрут в роутере приложения. + + #3324: guard — HTTP-middleware, он отрабатывает ДО роутинга, поэтому раньше + отвечал 401 и на пути, которых в приложении нет вовсе. Анониму этого хватало, + чтобы бесплатно разведать периметр: мусор под «интересным» префиксом + (``/api/public/whatever``) давал 401 с rbac-текстом, а мусор под публичным + префиксом — 404 роутера. Выключенная/закрытая ручка отличалась от + несуществующей. Несуществующий путь обязан отвечать одинаково независимо от + префикса, поэтому такие запросы пропускаются дальше — 404 отдаёт роутер, тот + же самый, что и на любой другой мусор. + + Ослабления нет: ``Match.NONE`` по ВСЕМ маршрутам значит, что выполнять + нечего — хендлера, до которого можно было бы дотянуться, не существует. + ``Match.PARTIAL`` (путь есть, метод другой) считается маршрутом и идёт в + guard как раньше — там путь реально существует, скрывать нечего. + + Неизвестное приложение (``scope["app"]`` не выставлен) — ведём себя как + раньше, то есть отдаём запрос в guard. + """ + router = getattr(request.scope.get("app"), "router", None) + if router is None: + return True + return any(route.matches(request.scope)[0] != Match.NONE for route in router.routes) + + def _propagate_authenticated_user(request: Request, username: str) -> None: """Инжектит ``X-Authenticated-User`` в ASGI scope — ПЕРЕЗАПИСЫВАЯ, а не только добавляя при отсутствии, — чтобы ``RateLimitMiddleware``/ @@ -239,6 +266,11 @@ async def rbac_guard( if path in _PUBLIC_PATHS or path.startswith(_PUBLIC_PATH_PREFIXES): return await call_next(request) + # #3324: путь, которого нет в роутере, отвечает как любой несуществующий + # путь (404 роутера) — иначе 401 работает оракулом существования ручки. + if not _path_is_routed(request): + return await call_next(request) + username: str | None = None role: str | None = None from_session = False diff --git a/tradein-mvp/backend/tests/test_auth_api.py b/tradein-mvp/backend/tests/test_auth_api.py index 9436d1f7..adbb6496 100644 --- a/tradein-mvp/backend/tests/test_auth_api.py +++ b/tradein-mvp/backend/tests/test_auth_api.py @@ -248,6 +248,13 @@ def _build_test_app(store: _Store) -> FastAPI: async def tradein_cache_stats() -> dict: return {"ok": True} + # По той же причине ручка настоящая: с #3324 guard не отвечает 401/403 за + # несуществующий путь (несуществующее обязано быть неотличимо от + # несуществующего), поэтому admin-гейт проверяется на реальном роуте. + @app.get("/api/v1/admin/dummy") + async def admin_dummy() -> dict: + return {"ok": True} + def _override_get_db(): # generator dependency — matches app.core.db.get_db shape yield _FakeDB(store) @@ -1306,9 +1313,9 @@ def test_session_user_can_reach_tradein_but_not_admin(client: TestClient, store: assert ok.status_code == 200 denied = client.get("/api/v1/admin/dummy") - # rbac_guard's admin-gate matches the path regex BEFORE routing even happens - # (route isn't registered on this test app) — role=employee != admin -> 403, - # never a 404 (a bare "any non-2xx" assertion would mask a rbac_guard typo). + # Роут зарегистрирован (см. _build_test_app), поэтому 403 приходит именно от + # admin-гейта rbac_guard'а: role=employee != admin. Не 404 — иначе «any + # non-2xx» маскировал бы опечатку в guard'е; и не 200 — иначе гейт не сработал. assert denied.status_code == 403 diff --git a/tradein-mvp/backend/tests/test_rbac.py b/tradein-mvp/backend/tests/test_rbac.py index dc3ecc0b..6447cded 100644 --- a/tradein-mvp/backend/tests/test_rbac.py +++ b/tradein-mvp/backend/tests/test_rbac.py @@ -384,6 +384,37 @@ def test_rbac_guard_no_header_on_non_admin_path_returns_401(client: TestClient) assert "no authenticated user" in resp.json()["detail"].lower() +def test_rbac_guard_unrouted_path_answers_the_same_everywhere(client: TestClient) -> None: + """#3324: аноним не должен по ответу отличать несуществующий путь от закрытого. + + Сравниваются два ОТВЕТА между собой, а не с константой: важно именно + неразличимость. До фикса мусор под «интересным» префиксом получал 401 с + rbac-текстом, а такой же мусор рядом — 404 роутера, и по этой разнице + периметр разведывался бесплатно. + """ + for interesting, boring in ( + # Пара, на которой оракул был виден в чистом виде: под публичным + # префиксом guard молчал (404 роутера), а рядом отвечал 401. + ("/api/v1/trade-in/zzz", "/api/v1/trade-in/r/zzz"), + ("/api/public/zzz", "/api/zzz"), + ("/trade-in/api/public/zzz", "/trade-in/api/zzz"), + ("/api/v1/admin/zzz", "/zzz"), + ): + first = client.get(interesting) + second = client.get(boring) + assert (first.status_code, first.json()) == (second.status_code, second.json()), ( + f"{interesting} → {first.status_code} {first.json()}, " + f"{boring} → {second.status_code} {second.json()}" + ) + assert first.status_code == 404 + + # Защита не ослаблена: РЕАЛЬНЫЙ закрытый маршрут анониму по-прежнему 401. + assert client.get("/api/v1/trade-in/support/unread").status_code == 401 + assert client.get("/api/v1/me").status_code == 401 + # Реальный публичный маршрут по-прежнему работает без идентичности. + assert client.get("/api/v1/trade-in/support/anon/unread").status_code == 200 + + # --------------------------------------------------------------------------- # 2026-07-31: внутренние разделы («Доля в продаже» / «Кэш») закрыты от клиентов # --------------------------------------------------------------------------- -- 2.45.3 From 8c4be70193447ea430358cf5401a6084f466bce8 Mon Sep 17 00:00:00 2001 From: bot-backend Date: Sat, 5 Sep 2026 23:12:17 +0500 Subject: [PATCH 2/2] =?UTF-8?q?fix(rbac):=20=D1=87=D0=B5=D1=81=D1=82=D0=BD?= =?UTF-8?q?=D1=8B=D0=B9=20=D0=B4=D0=BE=D0=BA=D1=81=D1=82=D1=80=D0=B8=D0=BD?= =?UTF-8?q?=D0=B3=20=D0=BF=D1=80=D0=BE=20=D1=80=D0=B0=D0=B7=D0=BC=D0=B5?= =?UTF-8?q?=D0=BD=20=D0=BE=D1=80=D0=B0=D0=BA=D1=83=D0=BB=D0=BE=D0=B2=20+?= =?UTF-8?q?=20=D1=82=D1=80=D0=B5=D0=B9=D0=BB=D0=B8=D0=BD=D0=B3-=D1=81?= =?UTF-8?q?=D0=BB=D1=8D=D1=88=20=D0=B1=D0=BE=D0=BB=D1=8C=D1=88=D0=B5=20?= =?UTF-8?q?=D0=BD=D0=B5=20=D0=B2=D1=8B=D0=B4=D0=B0=D1=91=D1=82=20=D0=BC?= =?UTF-8?q?=D0=B0=D1=80=D1=88=D1=80=D1=83=D1=82?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ревью #3352: комментарий у _path_is_routed утверждал «ослабления нет» — неверно. Раньше 401 был ПРЕФИКСНЫМ оракулом (таблицу маршрутов по нему не перечислить), теперь 401/404 — оракул СУЩЕСТВОВАНИЯ маршрута, включая имена admin-ручек. Докстринг переписан честно, с проверенным по caddy/sites/apps.caddy фактом: блок handle /trade-in/api/* стоит выше import users.caddy.snippet, внешнего basic_auth у trade-in нет — значит перебор имён выполним и снаружи. Второй канал того же оракула закрыт: /api/v1/me/ не матчил ни один маршрут, guard пропускал, а роутер отвечал 307 на существующий путь. FastAPI получил redirect_slashes=False (проверено: ни одного route с трейлинг-слэшем, ни одного такого вызова во фронте; deny-правила уже на глоб-форме). Тесты: PARTIAL-кейс (POST на GET-путь анониму → 401, не 404/405), трейлинг-слэш на РЕАЛЬНОМ app.main (тест на копии был бы тавтологией), admin-гейт сверяется по тексту 'admin only' — scope-ветка отвечает тем же 403, но 'forbidden for role'. --- tradein-mvp/backend/app/core/rbac.py | 38 +++++++++++++++++++--- tradein-mvp/backend/app/main.py | 10 ++++++ tradein-mvp/backend/tests/test_auth_api.py | 4 +++ tradein-mvp/backend/tests/test_rbac.py | 23 +++++++++++++ 4 files changed, 71 insertions(+), 4 deletions(-) diff --git a/tradein-mvp/backend/app/core/rbac.py b/tradein-mvp/backend/app/core/rbac.py index 67b2d736..f760ec54 100644 --- a/tradein-mvp/backend/app/core/rbac.py +++ b/tradein-mvp/backend/app/core/rbac.py @@ -194,10 +194,40 @@ def _path_is_routed(request: Request) -> bool: префикса, поэтому такие запросы пропускаются дальше — 404 отдаёт роутер, тот же самый, что и на любой другой мусор. - Ослабления нет: ``Match.NONE`` по ВСЕМ маршрутам значит, что выполнять - нечего — хендлера, до которого можно было бы дотянуться, не существует. - ``Match.PARTIAL`` (путь есть, метод другой) считается маршрутом и идёт в - guard как раньше — там путь реально существует, скрывать нечего. + ЧТО ИМЕННО РАЗМЕНЯНО (это НЕ «ослабления нет»). Раньше аноним получал 401 на + ЛЮБОЙ non-public путь — то есть оракул был ПРЕФИКСНЫЙ: он говорил «префикс + закрыт», но перечислить по нему таблицу маршрутов было нельзя. Теперь 401 = + «такой маршрут есть», 404 = «нет», и это уже оракул СУЩЕСТВОВАНИЯ маршрута: + перебором аноним восстанавливает список всех ручек приложения, включая имена + под ``/api/v1/admin/*``. Доступа это не даёт (закрытая ручка по-прежнему + отвечает 401/403), но карту периметра — даёт. + + Почему размен принят. Точечно: 404 на несуществующее — норма HTTP, а + подобранное ИМЯ ручки без креденшелов бесполезно; исчезает же реальный + признак «этот префикс что-то охраняет». Это защита в глубину, и её глубина + здесь честно меньше, чем была. + + ⚠️ Периметр admin-путей снаружи НЕ срезан — проверено по конфигу, а не по + предположению: ``caddy/sites/apps.caddy`` → блок ``handle /trade-in/api/*`` + делает ``uri strip_prefix /trade-in`` + ``reverse_proxy tradein-backend:8000`` + и стоит ЦЕЛИКОМ ВЫШЕ ``import caddy/users.caddy.snippet`` (basic_auth), т.е. + у trade-in своя авторизация и внешнего barrier'а нет. Значит внешний + ``https://gendsgn.ru/trade-in/api/v1/admin/...`` доходит до этого guard'а + анонимно, и перебор имён admin-ручек выполним снаружи, не только изнутри + docker-сети. Хочется убрать — резать надо в Caddy (отдельный issue), guard + этого не сделает: он про роли, а не про сетевой периметр. + + ``Match.NONE`` по ВСЕМ маршрутам значит, что выполнять нечего — хендлера, до + которого можно было бы дотянуться, не существует. ``Match.PARTIAL`` (путь + есть, метод другой) считается маршрутом и идёт в guard как раньше — там путь + реально существует, скрывать нечего. + + Трейлинг-слэш был вторым каналом того же оракула в обход guard'а: + ``/api/v1/me/`` не матчит ни один маршрут (``Match.NONE``) → guard пропускает + → Starlette-роутер отвечал 307 на ``/api/v1/me``, то есть «маршрут есть» + сообщал редирект, а не 401. Закрыто в ``app/main.py``: + ``FastAPI(redirect_slashes=False)`` — теперь такой путь даёт тот же 404, что + и любой другой мусор. Неизвестное приложение (``scope["app"]`` не выставлен) — ведём себя как раньше, то есть отдаём запрос в guard. diff --git a/tradein-mvp/backend/app/main.py b/tradein-mvp/backend/app/main.py index 74c2d2ad..e391025e 100644 --- a/tradein-mvp/backend/app/main.py +++ b/tradein-mvp/backend/app/main.py @@ -218,6 +218,16 @@ app = FastAPI( description="Оценка вторичного жилья (выкупная стоимость) — копия trade-in feature из gendesign", # noqa: E501 version="0.1.0", lifespan=lifespan, + # #3324: трейлинг-слэш обходил rbac_guard как канал разведки периметра. + # `/api/v1/me/` не матчит ни один маршрут → guard пропускает (см. + # rbac._path_is_routed) → роутер отвечал 307 на `/api/v1/me`, т.е. «маршрут + # существует» сообщал редирект вместо 401. Выключение проверено на предмет + # поломок: ни один route не объявлен с трейлинг-слэшем (нет `@router.get("/")` + # и пустых путей), ни один из 173 вызовов `api/v1` во фронте + # (tradein-mvp/frontend/src) не заканчивается слэшем, mount/StaticFiles нет. + # Deny-правила на слэш тоже не зависят от редиректа — они переведены на + # глоб-форму специально ради этого (см. auth_session.DB_ROLE_PATHS). + redirect_slashes=False, ) diff --git a/tradein-mvp/backend/tests/test_auth_api.py b/tradein-mvp/backend/tests/test_auth_api.py index adbb6496..b8be6b73 100644 --- a/tradein-mvp/backend/tests/test_auth_api.py +++ b/tradein-mvp/backend/tests/test_auth_api.py @@ -1316,7 +1316,11 @@ def test_session_user_can_reach_tradein_but_not_admin(client: TestClient, store: # Роут зарегистрирован (см. _build_test_app), поэтому 403 приходит именно от # admin-гейта rbac_guard'а: role=employee != admin. Не 404 — иначе «any # non-2xx» маскировал бы опечатку в guard'е; и не 200 — иначе гейт не сработал. + # Один статус этого не доказывает: scope-check (deny-список employee) отвечает + # тем же 403, поэтому сверяем ТЕКСТ — 'admin only' пишет только admin-гейт, + # scope-ветка пишет 'forbidden for role' (app/core/rbac.py). assert denied.status_code == 403 + assert denied.json()["detail"] == "admin only", denied.text # --------------------------------------------------------------------------- diff --git a/tradein-mvp/backend/tests/test_rbac.py b/tradein-mvp/backend/tests/test_rbac.py index 6447cded..27e16daa 100644 --- a/tradein-mvp/backend/tests/test_rbac.py +++ b/tradein-mvp/backend/tests/test_rbac.py @@ -411,10 +411,33 @@ def test_rbac_guard_unrouted_path_answers_the_same_everywhere(client: TestClient # Защита не ослаблена: РЕАЛЬНЫЙ закрытый маршрут анониму по-прежнему 401. assert client.get("/api/v1/trade-in/support/unread").status_code == 401 assert client.get("/api/v1/me").status_code == 401 + # Match.PARTIAL — путь есть, метод чужой (/api/v1/me зарегистрирован как GET). + # Такой запрос обязан идти в guard (401), а НЕ проваливаться в роутер за 405: + # иначе «метод не тот» стало бы ещё одним способом обойти проверку личности. + assert client.post("/api/v1/me").status_code == 401 # Реальный публичный маршрут по-прежнему работает без идентичности. assert client.get("/api/v1/trade-in/support/anon/unread").status_code == 200 +def test_real_app_does_not_redirect_trailing_slash() -> None: + """#3324: `/api/v1/me/` анониму → 404, а не 307 на существующий путь. + + Трейлинг-слэш был вторым каналом оракула существования: путь со слэшем не + матчит ни один маршрут (Match.NONE) → guard пропускает → Starlette отвечал + 307 на `/api/v1/me`, т.е. сообщал «маршрут есть» в обход 401. + + Проверяется РЕАЛЬНОЕ приложение (app.main), а не локальный _build_test_app: + флаг `redirect_slashes=False` живёт именно там, и тест на своей копии app + был бы тавтологией — он подтверждал бы настройку фикстуры, а не прода. + Импорт локальный: модуль этого файла намеренно не тянет app.main (см. шапку). + """ + from app.main import app as real_app + + # TestClient без `with` не запускает lifespan — в БД никто не ходит. + resp = TestClient(real_app).get("/api/v1/me/", follow_redirects=False) + assert resp.status_code == 404, f"{resp.status_code} {resp.headers.get('location')}" + + # --------------------------------------------------------------------------- # 2026-07-31: внутренние разделы («Доля в продаже» / «Кэш») закрыты от клиентов # --------------------------------------------------------------------------- -- 2.45.3