diff --git a/backend/app/api/routes/jobs.py b/backend/app/api/routes/jobs.py index ea3517fc..d8543768 100644 --- a/backend/app/api/routes/jobs.py +++ b/backend/app/api/routes/jobs.py @@ -207,6 +207,7 @@ async def cancel_job( async def pause_job( job_id: UUID, session: SessionDep, + _auth: PrintAuthDep, ) -> ProblemDetail: """Return 501 — pause is not yet implemented.""" # Verify the job exists so we return 404 rather than 501 for unknown jobs @@ -241,6 +242,7 @@ async def pause_job( async def resume_job( job_id: UUID, session: SessionDep, + _auth: PrintAuthDep, ) -> ProblemDetail: """Return 501 — resume is not yet implemented.""" # Verify the job exists so we return 404 rather than 501 for unknown jobs @@ -276,6 +278,7 @@ async def resume_job( async def retry_job( job_id: UUID, session: SessionDep, + _auth: PrintAuthDep, ) -> JobRead: """Clone a terminal job into a fresh QUEUED job.""" job = await _get_job_or_404(session, job_id) diff --git a/backend/app/api/routes/printers.py b/backend/app/api/routes/printers.py index 833e7d7c..ec99356a 100644 --- a/backend/app/api/routes/printers.py +++ b/backend/app/api/routes/printers.py @@ -143,9 +143,12 @@ async def list_printers( async def get_printer( printer_id: UUID, session: SessionDep, + _auth: ReadAuthDep, ) -> PrinterRead: """Return full printer metadata for a single printer.""" printer = await _get_printer_or_404(session, printer_id) + if _auth is not None: + check_printer_access(_auth, printer_id) state = await printer_state_repo.get(session, printer_id) return PrinterRead( id=printer.id, diff --git a/backend/tests/integration/api/test_auth_wiring.py b/backend/tests/integration/api/test_auth_wiring.py index 6040942b..5e09b486 100644 --- a/backend/tests/integration/api/test_auth_wiring.py +++ b/backend/tests/integration/api/test_auth_wiring.py @@ -6,6 +6,13 @@ Phase 1k.1a (Task 25): Removed TemplateLoader and /api/templates tests. Templates are deleted in Phase 1k.1a. + +2026-08-14 security audit: added coverage for GET /api/printers/{id} and +POST /api/jobs/{id}/retry, which shipped with NO auth dependency at all +while their sibling endpoints in the same router file all required one +(see tests/unit/api/test_route_auth_coverage_guardrail.py for the +structural guardrail that now catches this class of gap for every route, +not just these two). """ from __future__ import annotations @@ -119,3 +126,118 @@ async def test_pangolin_sso_header_grants_read(api_client_with_seed): headers={"X-Pangolin-User": "testuser@example.com"}, ) assert resp.status_code == 200, f"SSO should grant read: {resp.status_code}" + + +# --------------------------------------------------------------------------- +# 2026-08-14 security audit regressions: +# GET /api/printers/{id} and POST /api/jobs/{id}/retry had NO auth +# dependency at all — any caller that could reach the backend got full +# printer connection metadata / could trigger a real reprint with zero +# credentials. See app/api/routes/printers.py::get_printer and +# app/api/routes/jobs.py::retry_job. +# --------------------------------------------------------------------------- + + +async def _seed_printer(factory): + from app.models.printer import Printer + + async with factory() as s: + printer = Printer( + name="audit-seed-printer", + slug="audit-seed-printer", + model="PT-P750W", + backend="ptouch", + connection={"host": "192.0.2.10", "port": 9100}, + ) + s.add(printer) + await s.commit() + await s.refresh(printer) + return printer + + +async def _seed_failed_job(factory, printer_id): + from app.models.job import Job, JobState + + async with factory() as s: + job = Job( + printer_id=printer_id, + template_key="audit-seed-template", + state=JobState.FAILED.value, + payload={"text": "audit"}, + ) + s.add(job) + await s.commit() + await s.refresh(job) + return job + + +@pytest.mark.asyncio +async def test_get_printer_detail_without_auth_returns_401(api_client_with_seed): + """GET /api/printers/{id} must reject unauthenticated requests. + + Regression test: this endpoint previously had no auth dependency at + all, unlike GET /api/printers (list) and every other single-printer + endpoint (status/tape/queue/pause/resume) in the same file. + """ + import app.db.engine as _engine_module + + printer = await _seed_printer(_engine_module.async_session) + + resp = await api_client_with_seed.get(f"/api/printers/{printer.id}") + assert resp.status_code == 401, f"Expected 401, got {resp.status_code}: {resp.text}" + + +@pytest.mark.asyncio +async def test_get_printer_detail_with_read_key_returns_200(api_client_with_seed): + """GET /api/printers/{id} succeeds for a caller with a valid read-scope key.""" + import app.db.engine as _engine_module + + factory = _engine_module.async_session + printer = await _seed_printer(factory) + read_key = await _make_read_key(factory) + + resp = await api_client_with_seed.get( + f"/api/printers/{printer.id}", + headers={"X-Label-Hub-Key": read_key}, + ) + assert resp.status_code == 200, f"Expected 200, got {resp.status_code}: {resp.text}" + assert resp.json()["id"] == str(printer.id) + + +@pytest.mark.asyncio +async def test_retry_job_without_auth_returns_401(api_client_with_seed): + """POST /api/jobs/{id}/retry must reject unauthenticated requests. + + Regression test: this endpoint previously had no auth dependency at + all, unlike list_jobs/get_job (read scope) and cancel_job (print scope) + in the same file — despite retry_job creating a new QUEUED job that the + worker actually dispatches to the physical printer. + """ + import app.db.engine as _engine_module + + factory = _engine_module.async_session + printer = await _seed_printer(factory) + job = await _seed_failed_job(factory, printer.id) + + resp = await api_client_with_seed.post(f"/api/jobs/{job.id}/retry") + assert resp.status_code == 401, f"Expected 401, got {resp.status_code}: {resp.text}" + + +@pytest.mark.asyncio +async def test_retry_job_with_print_key_returns_201(api_client_with_seed): + """POST /api/jobs/{id}/retry succeeds for a caller with a valid print-scope key.""" + import app.db.engine as _engine_module + + factory = _engine_module.async_session + printer = await _seed_printer(factory) + job = await _seed_failed_job(factory, printer.id) + print_key = await _make_print_key(factory) + + resp = await api_client_with_seed.post( + f"/api/jobs/{job.id}/retry", + headers={"X-Label-Hub-Key": print_key}, + ) + assert resp.status_code == 201, f"Expected 201, got {resp.status_code}: {resp.text}" + body = resp.json() + assert body["id"] != str(job.id) + assert body["state"] == "queued" diff --git a/backend/tests/integration/api/test_printer_acl.py b/backend/tests/integration/api/test_printer_acl.py index 0e4f6c1a..6a8bff50 100644 --- a/backend/tests/integration/api/test_printer_acl.py +++ b/backend/tests/integration/api/test_printer_acl.py @@ -117,3 +117,54 @@ async def test_key_restricted_to_printer_a_allowed_on_printer_a(api_client_with_ assert resp.status_code in (204, 404), ( f"Expected 204 or 404, got {resp.status_code}: {resp.text}" ) + + +@pytest.mark.asyncio +async def test_get_printer_detail_restricted_to_printer_a_blocked_on_printer_b( + api_client_with_seed, +): + """2026-08-14 follow-up: GET /api/printers/{id} (single-printer detail) + now requires auth but was missing the per-printer ACL check that every + sibling endpoint (get_printer_status/pause_printer/resume_printer/ + clear_printer_queue) enforces via check_printer_access(). A read-key + scoped to printer A must not be able to read printer B's metadata + (including its connection dict — host/port/SNMP config). + + Analogous to test_key_restricted_to_printer_a_blocked_on_printer_b above, + but seeds its own printer B directly instead of relying on + api_client_with_seed's (frequently empty, skip-on-empty) default seed + data — this test must actually run, not skip, to be a real regression + guard. + """ + import app.db.engine as _engine_module + from app.models.printer import Printer + + factory = _engine_module.async_session + + async with factory() as s: + printer_b = Printer( + name="acl-test-printer-b", + slug="acl-test-printer-b", + model="PT-P750W", + backend="ptouch", + connection={"host": "192.0.2.20", "port": 9100}, + ) + s.add(printer_b) + await s.commit() + await s.refresh(printer_b) + + printer_a_id = str(uuid4()) # some other printer the key IS allowed for + + plaintext = await _insert_restricted_key( + factory, + allowed_printer_ids=[printer_a_id], + scopes=["read"], + ) + + resp = await api_client_with_seed.get( + f"/api/printers/{printer_b.id}", + headers={"X-Label-Hub-Key": plaintext}, + ) + assert resp.status_code == 403, ( + f"Expected 403 for restricted key on wrong printer, got {resp.status_code}: {resp.text}" + ) diff --git a/backend/tests/unit/api/test_jobs_routes.py b/backend/tests/unit/api/test_jobs_routes.py index 14980ce5..b6c8247e 100644 --- a/backend/tests/unit/api/test_jobs_routes.py +++ b/backend/tests/unit/api/test_jobs_routes.py @@ -545,7 +545,8 @@ async def test_retry_job_direct_returns_new_queued_job(session) -> None: printer = await _make_printer(session) original = await _make_job(session, printer.id, state=JobState.FAILED.value) - result = await retry_job(job_id=original.id, session=session) + fake_auth = AuthContext(source="api-key", scope="admin", api_key_id=_uuid4(), ip="127.0.0.1") + result = await retry_job(job_id=original.id, session=session, _auth=fake_auth) assert str(result.id) != str(original.id) assert result.state == JobState.QUEUED.value diff --git a/backend/tests/unit/api/test_route_auth_coverage_guardrail.py b/backend/tests/unit/api/test_route_auth_coverage_guardrail.py new file mode 100644 index 00000000..fdbeb0eb --- /dev/null +++ b/backend/tests/unit/api/test_route_auth_coverage_guardrail.py @@ -0,0 +1,172 @@ +"""Structural guardrail: every route must carry a recognised auth dependency +— or be explicitly and deliberately allowlisted here. + +Why this exists (2026-08-14 security audit) +--------------------------------------------- +Two REST endpoints shipped with NO auth dependency at all, while every +sibling endpoint in the same router file required one: + +- ``GET /api/printers/{printer_id}`` (``printers.py::get_printer``) — + ``list_printers``, ``get_printer_status``, ``get_printer_tape``, + ``get_printer_queue``, ``pause_printer``, ``resume_printer`` and + ``clear_printer_queue`` all required ``ReadAuthDep``/``PrintAuthDep``; the + single-printer detail GET did not. It leaked full printer metadata + (including the raw ``connection`` dict — host/port/SNMP config) to any + caller that could reach the backend, with zero credentials. +- ``POST /api/jobs/{job_id}/retry`` (``jobs.py::retry_job``) — clones a + terminal job into a fresh ``QUEUED`` job that the worker actually + dispatches to the physical printer. ``list_jobs``, ``get_job`` and + ``cancel_job`` in the same file all required ``read``/``print`` scope; + ``retry_job`` (and, less severely, the ``pause``/``resume`` 501 stubs) + did not — an unauthenticated caller could trigger real prints. + +Both gaps were silent omissions (no docstring claims either is intentionally +public), unlike ``GET /api/events`` which documents "Auth: none beyond the +Pangolin proxy SSO at the network layer" right in its own module docstring, +or the ``qr.py`` landing pages which document "These routes intentionally +live outside the ``/api/`` prefix ... printed as QR payloads on physical +labels". Deliberate public-by-design routes are allowlisted below with a +pointer to where that decision is documented; anything else must resolve a +``require_read``/``require_print``/``require_admin``/``require_webhook_key`` +dependency. + +This test enumerates every ``APIRoute`` the real app (``app.main.create_app``) +actually mounts and fails the moment a NEW route is added without a +recognised auth dependency and without a conscious addition to the +allowlist — the guardrail fires while developing, not hopefully during +review (see ``.claude/rules/api-response-dto-boundary.md`` for the sibling +rule this generalises: enforcement belongs in the suite, upfront). +""" + +from __future__ import annotations + +from typing import Any + +import app.models # noqa: F401 — registers all SQLModel tables with metadata +from fastapi.routing import APIRoute + +# --------------------------------------------------------------------------- +# Allowlist: routes that are deliberately reachable without an app-level +# auth dependency, and WHY. Every entry here is a conscious decision, not a +# gap — add to this list only alongside a docstring/comment at the route +# itself explaining the reasoning (matching the events.py / qr.py pattern). +# --------------------------------------------------------------------------- + +_PUBLIC_BY_DESIGN: dict[tuple[str, str], str] = { + ("GET", "/healthz"): "liveness probe — zero deps, no PII, own docstring", + # /openapi.json, /docs, /docs/oauth2-redirect, /redoc are framework-added + # Starlette Route objects (not APIRoute) — _iter_api_routes never visits + # them, so they don't need (and can't take) an allowlist entry here. + # /readiness is intentionally NOT allowlisted: it declares its own + # Depends(require_read) and is correctly picked up as authenticated. + ("GET", "/api/events"): "documented: 'Auth: none beyond the Pangolin proxy SSO " + "at the network layer' (events.py module docstring)", + ("GET", "/api/lookup/{app}/{entity_id}"): "read-only integration-entity lookup " + "backing the public QR landing pages (lookup.py); same trust class as qr.py", + ("GET", "/loc/{entity_id}"): "QR landing page — documented public-by-design " + "(qr.py: 'printed as QR payloads on physical labels')", + ("GET", "/asset/{entity_id}"): "QR landing page — see qr.py module docstring", + ("GET", "/spool/{entity_id}"): "QR landing page — see qr.py module docstring", + ("GET", "/product/{entity_id}"): "QR landing page — see qr.py module docstring", +} + +# Dependency callables that count as "this route is authenticated". Matched +# by qualname substring so both the named singletons (require_read etc., +# scope_deps.py) and the webhook key dependency are recognised regardless of +# how they're wired (function param vs. router-level `dependencies=[...]`). +_RECOGNISED_AUTH_MARKERS = ("require_scope", "require_webhook_key") + + +def _iter_api_routes(routes: list[Any]) -> list[APIRoute]: + """Recursively flatten FastAPI's route tree into concrete APIRoute objects. + + Included routers show up as opaque wrapper objects (not the routes + themselves) rather than being flattened into ``app.routes`` directly — + each one exposes the original ``APIRouter`` via ``.original_router``. + """ + out: list[APIRoute] = [] + for route in routes: + original_router = getattr(route, "original_router", None) + if original_router is not None: + out.extend(_iter_api_routes(original_router.routes)) + elif isinstance(route, APIRoute): + out.append(route) + return out + + +def _collect_dependency_qualnames(dependant: Any) -> set[str]: + names: set[str] = set() + for sub in dependant.dependencies: + fn = sub.call + names.add(getattr(fn, "__qualname__", str(fn))) + names |= _collect_dependency_qualnames(sub) + return names + + +def _build_real_app() -> Any: + """Return the actual FastAPI instance app.main mounts routes on. + + ``create_app()`` returns a ``_LifespanManager`` ASGI wrapper (so httpx's + ASGITransport, which never sends a lifespan scope, still triggers + startup) — the real ``FastAPI`` instance is its ``._app`` attribute. + Building routes does not touch the DB or run the lifespan, so this is + safe to call with no environment/DB setup beyond what module import + already requires. + """ + from app.main import create_app + + manager = create_app() + return manager._app + + +def test_every_route_has_recognised_auth_or_is_explicitly_allowlisted() -> None: + """Fail the build the moment a route ships with no recognised auth dependency. + + This is the guardrail: it re-discovers the exact class of bug this file's + module docstring describes (GET /api/printers/{id}, POST + /api/jobs/{id}/retry) and keeps rediscovering it for every future route. + """ + app = _build_real_app() + routes = _iter_api_routes(app.routes) + assert routes, "expected at least one APIRoute — route discovery is broken" + + unguarded: list[str] = [] + for route in routes: + for method in sorted(route.methods or []): + if method == "HEAD": + continue # HEAD mirrors GET's auth — no separate handler + key = (method, route.path) + if key in _PUBLIC_BY_DESIGN: + continue + dep_names = _collect_dependency_qualnames(route.dependant) + if any(marker in name for name in dep_names for marker in _RECOGNISED_AUTH_MARKERS): + continue + unguarded.append(f"{method} {route.path}") + + assert not unguarded, ( + "Route(s) with NO recognised auth dependency and NOT in the " + "_PUBLIC_BY_DESIGN allowlist:\n " + + "\n ".join(unguarded) + + "\n\nEither add Depends(require_read/require_print/require_admin) " + "(or require_webhook_key) to the route, or — only if this route is " + "genuinely meant to be reachable without credentials — add it to " + "_PUBLIC_BY_DESIGN in this file together with a comment at the " + "route itself explaining why (see events.py / qr.py for the " + "pattern)." + ) + + +def test_allowlist_entries_still_exist_as_routes() -> None: + """Catch stale allowlist entries (renamed/removed routes) going forward. + + A shrinking allowlist that silently drops entries is as dangerous as a + growing one that silently gains them — this keeps the allowlist honest. + """ + app = _build_real_app() + routes = _iter_api_routes(app.routes) + live_keys = {(method, route.path) for route in routes for method in route.methods or []} + + stale = [key for key in _PUBLIC_BY_DESIGN if key not in live_keys] + assert not stale, ( + f"Allowlist entries that no longer match any live route (stale — remove or fix): {stale}" + )