From 893a6629a88c465990746f63588d2e5dd047ac1a Mon Sep 17 00:00:00 2001 From: Nora Date: Tue, 7 Jul 2026 12:31:11 -0600 Subject: [PATCH] fix(sp37-followup): close auth-coverage gaps and drop dead matrix entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- backend/src/cyclone/api_routers/claim_acks.py | 2 +- backend/src/cyclone/auth/permissions.py | 25 ++-- .../tests/test_routes_have_auth_coverage.py | 107 ++++++++++++++++++ 3 files changed, 120 insertions(+), 14 deletions(-) create mode 100644 backend/tests/test_routes_have_auth_coverage.py diff --git a/backend/src/cyclone/api_routers/claim_acks.py b/backend/src/cyclone/api_routers/claim_acks.py index 553a969..f6a2985 100644 --- a/backend/src/cyclone/api_routers/claim_acks.py +++ b/backend/src/cyclone/api_routers/claim_acks.py @@ -292,7 +292,7 @@ def list_ack_orphans_endpoint( """List acks with no resolvable Claim row of their own kind. Used by the Inbox "Ack orphans" lane for the operator's manual - reconciliation flow. Mirrors ``/api/inbox/remit-orphans``. + reconciliation flow. Filters by kind: ``999``, ``277ca``, ``ta1``. """ if kind is not None: items = store.find_ack_orphans(kind) diff --git a/backend/src/cyclone/auth/permissions.py b/backend/src/cyclone/auth/permissions.py index 45a7201..da83741 100644 --- a/backend/src/cyclone/auth/permissions.py +++ b/backend/src/cyclone/auth/permissions.py @@ -38,11 +38,11 @@ PERMISSIONS: dict[tuple[str, str], set[Role]] = { ("GET", "/api/remittances"): ALL_ROLES, ("GET", "/api/providers"): ALL_ROLES, ("GET", "/api/batches"): ALL_ROLES, - ("GET", "/api/dashboard/summary"): ALL_ROLES, + ("GET", "/api/dashboard/kpis"): ALL_ROLES, # dashboard summary cards (renamed from /summary) ("GET", "/api/activity"): ALL_ROLES, ("GET", "/api/inbox/lanes"): ALL_ROLES, ("GET", "/api/inbox/export.csv"): ALL_ROLES, - ("GET", "/api/reconcile"): ALL_ROLES, + ("GET", "/api/inbox/ack-orphans"): ALL_ROLES, # Inbox "Ack orphans" lane — wired in src/hooks/useAckOrphans.ts ("GET", "/api/reconciliation"): ALL_ROLES, ("GET", "/api/acks"): ALL_ROLES, ("GET", "/api/ta1-acks"): ALL_ROLES, @@ -50,7 +50,6 @@ PERMISSIONS: dict[tuple[str, str], set[Role]] = { ("GET", "/api/batch-diff"): ALL_ROLES, ("GET", "/api/config"): ALL_ROLES, ("GET", "/api/payers"): ALL_ROLES, - ("GET", "/api/audit-log"): ADMIN_ONLY, # Clearhouse (SFTP creds + dzinesco identity) — admin only. ("GET", "/api/clearhouse"): ADMIN_ONLY, @@ -60,12 +59,12 @@ PERMISSIONS: dict[tuple[str, str], set[Role]] = { # Admin ops (audit log, backup, scheduler, db rotate, reload-config). ("GET", "/api/admin/audit-log"): ADMIN_ONLY, ("GET", "/api/admin/audit-log/verify"): ADMIN_ONLY, - ("GET", "/api/admin/backup"): ADMIN_ONLY, - ("POST", "/api/admin/backup"): ADMIN_ONLY, - ("GET", "/api/admin/backup/scheduler"): ADMIN_ONLY, - ("POST", "/api/admin/backup/scheduler"): ADMIN_ONLY, - ("GET", "/api/admin/scheduler"): ADMIN_ONLY, - ("POST", "/api/admin/scheduler"): ADMIN_ONLY, + ("GET", "/api/admin/backup"): ADMIN_ONLY, # prefix; covers /list, /status, /{id}/restore/*, /{id}/verify + ("POST", "/api/admin/backup"): ADMIN_ONLY, # prefix; covers /create, /prune, /{id}/restore/* + ("GET", "/api/admin/backup/scheduler"): ADMIN_ONLY, # prefix; covers /start, /stop, /tick + ("POST", "/api/admin/backup/scheduler"): ADMIN_ONLY, # prefix; covers /start, /stop, /tick + ("GET", "/api/admin/scheduler"): ADMIN_ONLY, # prefix; covers /status, /processed-files + ("POST", "/api/admin/scheduler"): ADMIN_ONLY, # prefix; covers /start, /stop, /tick, /pull-inbound ("POST", "/api/admin/db/rotate-key"): ADMIN_ONLY, ("POST", "/api/admin/reload-config"): ADMIN_ONLY, ("GET", "/api/admin/validate-provider"): ADMIN_ONLY, @@ -80,15 +79,15 @@ PERMISSIONS: dict[tuple[str, str], set[Role]] = { ("POST", "/api/inbox/candidates"): WRITE_ROLES, ("POST", "/api/inbox/rejected"): WRITE_ROLES, ("POST", "/api/inbox/payer-rejected"): WRITE_ROLES, - ("POST", "/api/reconcile"): WRITE_ROLES, ("POST", "/api/reconciliation"): WRITE_ROLES, ("POST", "/api/acks"): WRITE_ROLES, + # Unlink a wrong claim-ack match — inverse of POST /api/inbox/candidates/{remit_id}/match. + # Prefix (not the placeholder path) because the matcher treats {kind} + # as a literal substring; only ``/api/acks`` actually matches real requests. + ("DELETE", "/api/acks"): WRITE_ROLES, ("POST", "/api/batches"): WRITE_ROLES, # /export-837 regenerates X12 from DB rows ("POST", "/api/eligibility"): WRITE_ROLES, ("POST", "/api/submit-batch"): WRITE_ROLES, # SP37: canonical outbound path (mirrors CLI) - - # CSV export — read-only. - ("GET", "/api/export.csv"): ALL_ROLES, } diff --git a/backend/tests/test_routes_have_auth_coverage.py b/backend/tests/test_routes_have_auth_coverage.py new file mode 100644 index 0000000..b4adbd4 --- /dev/null +++ b/backend/tests/test_routes_have_auth_coverage.py @@ -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" + ) \ No newline at end of file