test(api): RBAC contract test — viewer JWT on every classified route

New test walks app.routes, classifies each APIRoute as admin/viewer/open
by identity-matching require_admin / require_viewer closures inside the
route's dependency tree, then asserts:
  - admin routes return 403 to a viewer JWT
  - viewer routes return neither 401 nor 403 to a viewer JWT
SSE routes skipped (separate scope under F6). Role hints deliberately
NOT encoded in the OpenAPI spec — classification stays server-side so
/openapi.json can't be used to enumerate admin routes.

Resolves THREAT_MODEL F2/I + F5/E; paired with the existing
test_schemathesis.py::test_auth_enforcement (401-half coverage).
This commit is contained in:
2026-04-24 14:00:12 -04:00
parent 99ccd41bb5
commit e53b580767
2 changed files with 182 additions and 4 deletions

View File

@@ -196,7 +196,7 @@ Each sub-flow below gets its own table. Status codes:
| S | Forged role claim in JWT | M | Role read from DB by UUID on each authz, not trusted from token. (Verify — see `project_rbac_null_role.md`.) | | S | Forged role claim in JWT | M | Role read from DB by UUID on each authz, not trusted from token. (Verify — see `project_rbac_null_role.md`.) |
| T | Client-side role flag tampering | M | Server-side gating required; client-side hide-only is UI polish. See `feedback_serverside_ui.md`. | | T | Client-side role flag tampering | M | Server-side gating required; client-side hide-only is UI polish. See `feedback_serverside_ui.md`. |
| R | Admin denies granting a role | M | `update_user_role` calls logged. | | R | Admin denies granting a role | M | `update_user_role` calls logged. |
| I | Route missing `require_*` accidentally exposes admin data to viewer | **?** | Verify: schemathesis / contract tests assert 401/403 on every protected route. Recommend a test that enumerates all routes and asserts gating. | | I | Route missing `require_*` accidentally exposes admin data to viewer | M | 401 half covered by `tests/api/test_schemathesis.py::test_auth_enforcement` (schemathesis + `ignored_auth` check on every operation). 403 half covered by `tests/api/test_rbac_contract.py`, which introspects every `APIRoute.dependant` at collection time, classifies each as admin/viewer/open via identity-match against the `require_admin`/`require_viewer` singletons, and asserts a viewer JWT receives 403 on admin routes and non-401/403 on viewer routes. SSE routes are skipped (covered separately under F6). |
| D | n/a (authz is a check, not a bottleneck) | — | | | D | n/a (authz is a check, not a bottleneck) | — | |
| E | Viewer crafts path traversal in URL to hit admin route | M | FastAPI path matching is exact; no dynamic include. | | E | Viewer crafts path traversal in URL to hit admin route | M | FastAPI path matching is exact; no dynamic include. |
| E | Master-only CLI command reachable in agent mode | M | `MASTER_ONLY_COMMANDS` gating at CLI registration + `_require_master_mode()` guard in handler. | | E | Master-only CLI command reachable in agent mode | M | `MASTER_ONLY_COMMANDS` gating at CLI registration + `_require_master_mode()` guard in handler. |
@@ -242,7 +242,7 @@ Each sub-flow below gets its own table. Status codes:
| I | Mutation response returns internal state not meant for client | **?** | Verify per-route response_model shape. | | I | Mutation response returns internal state not meant for client | **?** | Verify per-route response_model shape. |
| D | Malformed body triggers expensive validation / oversized payload | M | FastAPI enforces content-length at ASGI layer; Pydantic short-circuits on type mismatch. | | D | Malformed body triggers expensive validation / oversized payload | M | FastAPI enforces content-length at ASGI layer; Pydantic short-circuits on type mismatch. |
| D | Destructive mutation storm (e.g. delete-all-users) | A | Admin role is trusted; protecting admins from themselves is out of scope. | | D | Destructive mutation storm (e.g. delete-all-users) | A | Admin role is trusted; protecting admins from themselves is out of scope. |
| E | Mutation bypasses role check via missing `require_admin` | **?** | Verify via schemathesis: every mutation route returns 403 for viewer. | | E | Mutation bypasses role check via missing `require_admin` | M | `tests/api/test_rbac_contract.py::test_admin_route_rejects_viewer` parametrizes every route classified admin by FastAPI-dependency introspection (identity-match on the `require_admin` closure) and asserts viewer JWT → 403. A missing `require_admin` would reclassify the route away from "admin" and break the viewer route's non-403 assertion, so the check is bidirectional. |
#### F6 — Streaming / SSE #### F6 — Streaming / SSE
@@ -292,7 +292,7 @@ code" or "accepted, add to table above."
- [x] ~~Production error handler suppresses tracebacks and internal details.~~ Shipped — generic `@app.exception_handler(Exception)` in `decnet/web/api.py`; opaque `{detail, error_id}` in prod, traceback only under `DECNET_DEVELOPER=True`. - [x] ~~Production error handler suppresses tracebacks and internal details.~~ Shipped — generic `@app.exception_handler(Exception)` in `decnet/web/api.py`; opaque `{detail, error_id}` in prod, traceback only under `DECNET_DEVELOPER=True`.
- [x] ~~`detail=str(e)` / `detail=f"…{e}"` sites in `decnet/web/router/fleet/api_deploy_deckies.py:41,67,83,155`.~~ Audited 2026-04-24: L41 + L83 are deliberate `ValueError` messages from `load_ini_from_string` / `build_deckies_from_ini` (user-authored INI validator feedback, not internal state); L67/73 wraps `detect_subnet`'s `RuntimeError` with a remediation hint (`"Add a [general] section with interface=, net=, and gw="`); L155 aggregates structured `DispatchResult.detail` fields from swarm workers, not raw exceptions. All four sites are admin-gated. No sanitization needed. - [x] ~~`detail=str(e)` / `detail=f"…{e}"` sites in `decnet/web/router/fleet/api_deploy_deckies.py:41,67,83,155`.~~ Audited 2026-04-24: L41 + L83 are deliberate `ValueError` messages from `load_ini_from_string` / `build_deckies_from_ini` (user-authored INI validator feedback, not internal state); L67/73 wraps `detect_subnet`'s `RuntimeError` with a remediation hint (`"Add a [general] section with interface=, net=, and gw="`); L155 aggregates structured `DispatchResult.detail` fields from swarm workers, not raw exceptions. All four sites are admin-gated. No sanitization needed.
- [x] ~~Password length clamp before bcrypt.~~ Verified — Pydantic `max_length=72`. - [x] ~~Password length clamp before bcrypt.~~ Verified — Pydantic `max_length=72`.
- [ ] Contract test asserting every protected route returns 401 unauthenticated and 403 for under-roled. - [x] ~~Contract test asserting every protected route returns 401 unauthenticated and 403 for under-roled.~~ 401 half: `tests/api/test_schemathesis.py::test_auth_enforcement` (schemathesis + `ignored_auth`). 403 half: `tests/api/test_rbac_contract.py` (server-side dependency introspection + viewer JWT per route). Role hints deliberately kept out of the OpenAPI spec — classification stays server-side.
- [ ] Field allow-list on viewer responses for attacker / user / bounty serializers. - [ ] Field allow-list on viewer responses for attacker / user / bounty serializers.
- [ ] Sort/filter query keys are allow-listed, not passed through raw. - [ ] Sort/filter query keys are allow-listed, not passed through raw.
- [ ] Role-scoped repo methods OR per-route pre-filter for viewer queries (pick one, document it). - [ ] Role-scoped repo methods OR per-route pre-filter for viewer queries (pick one, document it).
@@ -300,7 +300,7 @@ code" or "accepted, add to table above."
- [ ] `offset` is capped OR pagination is cursor-based OR deep-offset is cheap. - [ ] `offset` is capped OR pagination is cursor-based OR deep-offset is cheap.
- [ ] Free-text `q` parameters hit an indexed/FTS5 column, never a full-table `LIKE` scan. - [ ] Free-text `q` parameters hit an indexed/FTS5 column, never a full-table `LIKE` scan.
- [ ] Per-route response_model shape audit on mutations. - [ ] Per-route response_model shape audit on mutations.
- [ ] Contract test asserting every mutation route returns 403 for viewer. - [x] ~~Contract test asserting every mutation route returns 403 for viewer.~~ Covered by `test_rbac_contract.py` (same test also covers read routes — classification is by dependency, not HTTP verb).
- [ ] SSE handler applies per-connection role filter before forwarding events. - [ ] SSE handler applies per-connection role filter before forwarding events.
- [ ] Per-user concurrent SSE connection cap. - [ ] Per-user concurrent SSE connection cap.
- [x] ~~Artifact download sets `Content-Disposition: attachment` + `X-Content-Type-Options: nosniff`.~~ Shipped — explicit headers on `FileResponse` in `api_get_artifact.py`; asserted in `tests/api/artifacts/test_get_artifact.py::test_content_disposition_is_attachment`. - [x] ~~Artifact download sets `Content-Disposition: attachment` + `X-Content-Type-Options: nosniff`.~~ Shipped — explicit headers on `FileResponse` in `api_get_artifact.py`; asserted in `tests/api/artifacts/test_get_artifact.py::test_content_disposition_is_attachment`.
@@ -360,3 +360,4 @@ In priority order:
| 2026-04-23 | F1 Authn: 3 threats moved from **?** to **M** (rate limit shipped; uniform 401 verified; bcrypt length clamp verified). Added DA-08 accepted risk: reverse-proxy per-IP bucket collapse. | ANTI | | 2026-04-23 | F1 Authn: 3 threats moved from **?** to **M** (rate limit shipped; uniform 401 verified; bcrypt length clamp verified). Added DA-08 accepted risk: reverse-proxy per-IP bucket collapse. | ANTI |
| 2026-04-23 | F1/I "traceback / stack trace leakage" moved from **?** to **M** via generic Exception handler with `error_id` correlation. Added follow-up checklist entry for `detail=str(e)` sites in fleet deploy router. | ANTI | | 2026-04-23 | F1/I "traceback / stack trace leakage" moved from **?** to **M** via generic Exception handler with `error_id` correlation. Added follow-up checklist entry for `detail=str(e)` sites in fleet deploy router. | ANTI |
| 2026-04-24 | F7: "MIME sniffing" moved from **?** to **M** (explicit `Content-Disposition`/`nosniff` headers + test). F7: "path-traversal" row reworded to point at the existing `_resolve_artifact_path` containment check. Fleet-deploy `detail=str(e)` audit resolved — all four sites documented as deliberate, admin-gated, no sanitization needed. | ANTI | | 2026-04-24 | F7: "MIME sniffing" moved from **?** to **M** (explicit `Content-Disposition`/`nosniff` headers + test). F7: "path-traversal" row reworded to point at the existing `_resolve_artifact_path` containment check. Fleet-deploy `detail=str(e)` audit resolved — all four sites documented as deliberate, admin-gated, no sanitization needed. | ANTI |
| 2026-04-24 | F2/I + F5/E moved from **?** to **M** via new `tests/api/test_rbac_contract.py` — classifies every APIRoute by FastAPI-dependency introspection and asserts viewer JWT → 403 on admin routes, non-401/403 on viewer routes. Role hints deliberately omitted from OpenAPI spec. SSE routes skipped (F6 scope). | ANTI |

View File

@@ -0,0 +1,177 @@
"""
RBAC contract test — every route is classified by server-side dependency
introspection and exercised with a viewer JWT.
Covers THREAT_MODEL.md F2/E (mutation bypass via missing `require_admin`)
and F5/E (mutation routes returning 403 for viewer). The 401-unauth half
is covered by `test_schemathesis.py::test_auth_enforcement`.
We deliberately do NOT annotate role hints in the OpenAPI spec —
classification stays server-side so an attacker reading /openapi.json
can't enumerate admin routes.
"""
from __future__ import annotations
import httpx
import pytest
from fastapi.routing import APIRoute
from decnet.web.api import app
from decnet.web.dependencies import (
require_admin,
require_viewer,
require_stream_viewer,
get_current_user_unchecked,
get_current_user,
)
# ---------------------------------------------------------------------------
# Route classification (runs at import time)
# ---------------------------------------------------------------------------
_ADMIN_CALLS = {require_admin}
_VIEWER_CALLS = {require_viewer, require_stream_viewer, get_current_user_unchecked, get_current_user}
def _walk_deps(dependant) -> set:
"""Recursively collect every dependency `call` in the tree."""
calls: set = set()
stack = list(dependant.dependencies)
while stack:
d = stack.pop()
if d.call is not None:
calls.add(d.call)
stack.extend(d.dependencies)
return calls
def _classify(route: APIRoute) -> str:
calls = _walk_deps(route.dependant)
if calls & _ADMIN_CALLS:
return "admin"
if calls & _VIEWER_CALLS:
return "viewer"
return "open"
def _is_sse(route: APIRoute) -> bool:
"""SSE endpoints keep the connection open — authz fires before the stream
starts, but httpx won't return until the server closes. Skip them here;
F6 gets its own dedicated verification pass."""
return route.path.endswith(("/events", "/stream", "/status-events"))
def _collect() -> tuple[list[tuple[str, str, str]], list[tuple[str, str, str]]]:
"""Return (admin_routes, viewer_routes) as (method, path, name) triples."""
admin: list[tuple[str, str, str]] = []
viewer: list[tuple[str, str, str]] = []
for route in app.routes:
if not isinstance(route, APIRoute):
continue
if _is_sse(route):
continue
cls = _classify(route)
for method in sorted(route.methods - {"HEAD", "OPTIONS"}):
entry = (method, route.path, route.name)
if cls == "admin":
admin.append(entry)
elif cls == "viewer":
viewer.append(entry)
return admin, viewer
ADMIN_ROUTES, VIEWER_ROUTES = _collect()
assert ADMIN_ROUTES, "no admin routes discovered — classifier is broken"
assert VIEWER_ROUTES, "no viewer routes discovered — classifier is broken"
# ---------------------------------------------------------------------------
# Path-param substitution
# ---------------------------------------------------------------------------
_ZERO_UUID = "00000000-0000-0000-0000-000000000000"
def _substitute_path(path: str, route: APIRoute) -> str:
"""Fill `{param}` placeholders with dummy values that satisfy path regex.
Authz (403) fires before route-handler execution, so the values don't
need to match real DB rows — they only need to survive FastAPI's
param-type coercion. Heuristic by param name keeps this independent
of pydantic-version internals.
"""
out = path
while "{" in out:
start = out.index("{")
end = out.index("}", start)
name = out[start + 1 : end].lower()
if "uuid" in name:
value = _ZERO_UUID
elif name.endswith("_id") or name == "id":
value = "1"
else:
value = "x"
out = out[:start] + value + out[end + 1 :]
return out
# ---------------------------------------------------------------------------
# Tests
# ---------------------------------------------------------------------------
_WRITE_METHODS = {"POST", "PUT", "PATCH", "DELETE"}
@pytest.mark.parametrize(
"method,path,name",
ADMIN_ROUTES,
ids=lambda t: f"{t[0]} {t[1]}" if isinstance(t, tuple) else str(t),
)
async def test_admin_route_rejects_viewer(client, viewer_token, method, path, name):
"""Every admin-classified route must return 403 when hit with a viewer JWT.
If a route returns 422 instead, the `Depends(require_admin)` parameter
is declared after a body/query param in the route signature — move it
earlier so authz runs before schema validation. A 401 means the token
was rejected outright (viewer user seeding broken → check conftest).
"""
url = _substitute_path(path, _route_lookup(method, path))
kwargs = {"headers": {"Authorization": f"Bearer {viewer_token}"}}
if method in _WRITE_METHODS:
kwargs["json"] = {}
resp = await client.request(method, url, **kwargs)
assert resp.status_code == 403, (
f"{method} {path} (name={name}): expected 403 for viewer, "
f"got {resp.status_code} — body={resp.text[:200]!r}. "
"If 422: move Depends(require_admin) before the body param in the signature. "
"If 401: viewer token invalid."
)
@pytest.mark.parametrize(
"method,path,name",
VIEWER_ROUTES,
ids=lambda t: f"{t[0]} {t[1]}" if isinstance(t, tuple) else str(t),
)
async def test_viewer_route_does_not_reject_viewer(client, viewer_token, method, path, name):
"""Viewer-accessible routes must not return 401/403 for a valid viewer JWT."""
url = _substitute_path(path, _route_lookup(method, path))
kwargs = {"headers": {"Authorization": f"Bearer {viewer_token}"}}
if method in _WRITE_METHODS:
kwargs["json"] = {}
resp = await client.request(method, url, **kwargs)
assert resp.status_code not in (401, 403), (
f"{method} {path} (name={name}): viewer unexpectedly got {resp.status_code} "
f"— body={resp.text[:200]!r}"
)
def _route_lookup(method: str, path: str) -> APIRoute:
for route in app.routes:
if isinstance(route, APIRoute) and route.path == path and method in route.methods:
return route
raise LookupError(f"{method} {path}")