fix(sp37-followup): close auth-coverage gaps and drop dead matrix entries
Audit on 2026-07-07 found three production bugs and six stale matrix
entries:
Three registered routes were DENIED in production (fail-closed):
- DELETE /api/acks/{kind}/{ack_id}/match-claim/{claim_id}
(Inbox unlink-a-wrong-match action; wired from the frontend)
- GET /api/dashboard/kpis (dashboard summary cards)
- GET /api/inbox/ack-orphans (Inbox 'Ack orphans' lane; already
wired into src/hooks/useAckOrphans.ts + AckOrphansLane.tsx)
All three had working frontend callers + passing tests (TestClient
bypasses matrix_gate), so operators hitting those endpoints got 403s
in production while CI stayed green. Fixed by adding the three
missing matrix entries with the role sets their siblings use
(WRITE_ROLES for the unlink action; ALL_ROLES for both read lanes).
Six dead matrix entries from refactor drift removed:
- GET /api/dashboard/summary (renamed to /kpis)
- GET /api/reconcile (renamed to /reconciliation)
- POST /api/reconcile (renamed to /reconciliation)
- GET /api/audit-log (moved to /api/admin/audit-log)
- GET /api/admin/backup/scheduler (refactored to /start, /stop, /tick)
- GET /api/export.csv (renamed to /api/inbox/export.csv)
Plus a one-line doc fix at api_routers/claim_acks.py:295 — the
endpoint docstring claimed it 'mirrors /api/inbox/remit-orphans'
but that sibling route was never built.
Guard test: tests/test_routes_have_auth_coverage.py enumerates every
APIRoute via the live FastAPI app (recurse into include_router mounts)
and asserts each (method, path) has a non-None allowed_roles result.
Parametrised so each missing route surfaces as its own test report
entry with the exact uncovered path. 84 routes × 1 parametrised case
= 84 individual coverage checks; if a future contributor adds a new
route without a matrix entry, this test fires immediately.
This commit is contained in:
@@ -0,0 +1,107 @@
|
||||
"""Audit guard: every registered route MUST have a matching PERMISSIONS entry.
|
||||
|
||||
This is the inverse of ``test_auth_permissions_matrix.py``: that one pins
|
||||
the fail-closed invariant for a specific dead prefix; this one pins the
|
||||
opposite — for every route that actually exists, the matrix MUST grant
|
||||
some role (otherwise operators hitting the route would get 403s in
|
||||
production but the tests would still pass because ``TestClient`` bypasses
|
||||
the ``matrix_gate`` middleware).
|
||||
|
||||
Found via a one-shot audit on 2026-07-07, three production bugs surfaced:
|
||||
|
||||
- DELETE /api/acks/{kind}/{ack_id}/match-claim/{claim_id}
|
||||
(used by the Inbox "Matched claims" operator flow to unlink a wrong
|
||||
match)
|
||||
- GET /api/dashboard/kpis (dashboard summary cards)
|
||||
- GET /api/inbox/ack-orphans (Inbox "Ack orphans" lane — already
|
||||
wired into the frontend via ``src/hooks/useAckOrphans.ts`` and
|
||||
``src/components/inbox/AckOrphansLane.tsx``)
|
||||
|
||||
All three had registered routes + tests + (for #3) frontend callers,
|
||||
yet ``allowed_roles(method, path)`` returned ``None`` because no matrix
|
||||
entry covered the path. Fail-closed denied every role, so the operators
|
||||
saw 403s in production but every CI test passed.
|
||||
|
||||
This test enumerates the real FastAPI route table (after all
|
||||
``include_router`` calls) and asserts each route has coverage. If a
|
||||
contributor adds a new route without a matching matrix entry, this test
|
||||
fires.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
from fastapi.routing import APIRoute
|
||||
|
||||
from cyclone.api import app
|
||||
from cyclone.auth.permissions import allowed_roles
|
||||
|
||||
|
||||
# Endpoints mounted by FastAPI itself (docs, openapi schema) — not part
|
||||
# of the application surface. Skipped from the coverage assertion because
|
||||
# they aren't protected by ``matrix_gate``.
|
||||
_INTERNAL_PATHS = frozenset(("/docs", "/openapi.json", "/redoc", "/docs/oauth2-redirect"))
|
||||
|
||||
|
||||
def _collect_routes(fastapi_app):
|
||||
"""Recursively collect (method, full_path) for every registered route.
|
||||
|
||||
FastAPI mounts sub-routers via ``_IncludedRouter`` wrappers; their
|
||||
``original_router`` exposes the routes mounted at the include point.
|
||||
We recurse into those so every endpoint appears exactly once with its
|
||||
full path (e.g. ``/api/auth/me``), no prefix doubling.
|
||||
"""
|
||||
out: list[tuple[str, str]] = []
|
||||
for r in fastapi_app.routes:
|
||||
if isinstance(r, APIRoute):
|
||||
for m in r.methods or set():
|
||||
if m in {"GET", "POST", "PATCH", "DELETE", "PUT"}:
|
||||
out.append((m, r.path))
|
||||
elif hasattr(r, "original_router"):
|
||||
out.extend(_collect_routes(r.original_router))
|
||||
return out
|
||||
|
||||
|
||||
ALL_ROUTES = _collect_routes(app)
|
||||
|
||||
|
||||
def test_internal_paths_not_in_collection():
|
||||
"""Sanity: docs/openapi/redoc are NOT in the collected routes because
|
||||
we skip them via the APIRoute isinstance check (they're mounted as
|
||||
Starlette Routes, not APIRoutes)."""
|
||||
paths = {p for _, p in ALL_ROUTES}
|
||||
assert not any(p in _INTERNAL_PATHS for p in paths), (
|
||||
"FastAPI internal docs paths leaked into route collection — "
|
||||
"fix _collect_routes() to filter on APIRoute, not method set"
|
||||
)
|
||||
|
||||
|
||||
def test_every_route_has_matrix_coverage():
|
||||
"""Every registered (method, path) MUST match at least one PERMISSIONS entry.
|
||||
|
||||
Fail-closed means missing entries silently deny the route in
|
||||
production. The frontend may have a working caller + the test suite
|
||||
may be green (TestClient bypasses auth) but operators will see 403s.
|
||||
|
||||
To fix an uncovered route, add a ``("METHOD", "/api/foo"): ROLES``
|
||||
entry to ``cyclone.auth.permissions.PERMISSIONS``. Prefix entries
|
||||
(e.g. ``/api/inbox``) match child paths, so adding a prefix is
|
||||
usually the lightest-touch fix.
|
||||
"""
|
||||
uncovered = [
|
||||
(m, p) for m, p in ALL_ROUTES if allowed_roles(m, p) is None
|
||||
]
|
||||
assert not uncovered, (
|
||||
"These routes are registered but have NO matching PERMISSIONS "
|
||||
"entry — they will be denied (403) in production:\n "
|
||||
+ "\n ".join(f"{m} {p}" for m, p in sorted(uncovered))
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("verb,path", sorted(ALL_ROUTES))
|
||||
def test_each_route_individually_has_coverage(verb, path):
|
||||
"""Per-route parametrised coverage — failure output points at the
|
||||
exact missing route. Complements the aggregate test by surfacing
|
||||
each gap as its own test report entry."""
|
||||
assert allowed_roles(verb, path) is not None, (
|
||||
f"{verb} {path} has no PERMISSIONS entry — fail-closed will deny it"
|
||||
)
|
||||
Reference in New Issue
Block a user