From 876875fdafe386904185d636cb4607de4325a797 Mon Sep 17 00:00:00 2001 From: Petr Date: Mon, 24 Aug 2026 07:32:44 +0200 Subject: [PATCH] feat(serve): enforce the permission firewall on every REST route (#655) `PermissionEngine` was built only in the Typer callback, so a persisted `permissions set --mode deny` policy -- and both `--deny-writes` and `--deny-destructive` -- protected the CLI process and nothing else. `kbagent serve` exposed all 236 routes, `DELETE /storage/buckets` included, behind a single all-or-nothing bearer token. #677 built the enforcement machinery but wired it to three `/auth/*` routes; this adds the missing half, coverage. One app-level dependency looks the matched route up by `(method, path template)` in a central table and calls the same `check_or_raise` the CLI uses, so a denial answers HTTP 403 with `error_code: PERMISSION_DENIED` -- the same code the CLI exits on. - `server/route_permissions.py`: 226 route -> operation entries plus 9 exempt bootstrap paths. Central rather than 226 per-route decorators so a security reviewer reads one screen, and so an unclassified route is refused instead of silently exempted. - The per-route `require_permission(...)` form still wins: a route declaring it inline is skipped by the table lookup, which is what keeps #677's `/auth/*` routes and test probe routes working. - New `GET /permissions/show` reports the EFFECTIVE policy (persisted block merged with the daemon's `--deny-*` flags). Read-only by design: a bearer token must not be able to widen the policy constraining it. - Two new serve-only operations, both `write` because both spawn a local CLI process on the host: `ai.chat`, `workspace.sql-improve`. - `permissions.py`'s `http.*` comment claimed serve-side enforcement that did not exist; it is true now and says so precisely. Tests: 31 new in `tests/test_server_route_permissions.py`. The completeness pair asserts the table matches the live app in both directions, so a route added without an entry fails CI rather than meeting its 403 in production. Verb/risk agreement is checked too (every DELETE is destructive-or-admin; the read-classified POSTs are an explicit allowlist). Mutation-checked: disabling the dependency fails 5. Fixes #655 --- CLAUDE.md | 12 + docs/web-server-endpoints.md | 10 +- docs/web-server.md | 78 ++- .../kbagent/references/commands-reference.md | 1 + .../skills/kbagent/references/gotchas.md | 48 +- src/keboola_agent_cli/commands/context.py | 5 +- src/keboola_agent_cli/permissions.py | 37 +- src/keboola_agent_cli/server/app.py | 24 +- src/keboola_agent_cli/server/dependencies.py | 16 + .../server/route_permissions.py | 467 ++++++++++++++++++ .../server/routers/permissions.py | 63 +++ tests/test_server_route_permissions.py | 379 ++++++++++++++ 12 files changed, 1109 insertions(+), 31 deletions(-) create mode 100644 src/keboola_agent_cli/server/route_permissions.py create mode 100644 src/keboola_agent_cli/server/routers/permissions.py create mode 100644 tests/test_server_route_permissions.py diff --git a/CLAUDE.md b/CLAUDE.md index 2e31c8c6..f9a67d4a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -672,6 +672,18 @@ kbagent token refresh --project NAME --token-id ID [--yes] # like cli:read, cli:write, cli:destructive). `tool:*` patterns are INERT since 0.85.0 (the MCP # passthrough is gone): they load but match nothing, so a mode=deny policy whose only allowance was # tool:read now denies everything. The agent guards rails against mistakes; not a sandbox. +# (since vNEXT, #655) The policy also firewalls the WHOLE `kbagent serve` REST surface: every +# route is classified in server/route_permissions.py and checked by one app-level dependency +# against the SERVED config dir's policy -> HTTP 403 error_code PERMISSION_DENIED, the same +# code the CLI exits on. On 0.90.1 and older only /auth/* was checked and every other route +# executed unchecked. An unclassified route is REFUSED, not exempted (a test keeps the table +# and the live app in sync both ways); bootstrap paths (/health/ping, /health/auth-info, +# /ui-config, /docs, /redoc, /openapi.json, SPA shell) are never checked. Coarser than the +# CLI where a path param collapses leaves: POST /semantic-layer/items/{kind} maps to the +# parent key `semantic-layer.add`, so a leaf-only policy pattern is CLI-only. +# `GET /permissions/show` (serve, since vNEXT) reports the EFFECTIVE policy (persisted block +# merged with the daemon's --deny-* flags); read-only by design -- no REST route can widen +# the policy that constrains it, so `permissions set|reset` stay terminal actions on the host. kbagent permissions list [--category read|write|destructive|admin] kbagent permissions show kbagent permissions set --mode allow|deny [--allow PATTERN ...] [--deny PATTERN ...] diff --git a/docs/web-server-endpoints.md b/docs/web-server-endpoints.md index cceeb3ec..6de74a10 100644 --- a/docs/web-server-endpoints.md +++ b/docs/web-server-endpoints.md @@ -9,7 +9,7 @@ auth, and the concepts behind these routes live in [`web-server.md`](web-server.md); a running server serves the same spec interactively at `/docs` (Swagger) and `/openapi.json`. -**231 operations** across **202 paths** and **30 routers**. +**232 operations** across **203 paths** and **31 routers**. Paths are shown as the server registers them. Reaching them through the Node BFF (or single-process `--ui` mode) prefixes every path with `/api`. @@ -458,6 +458,14 @@ Developer Portal app discovery -- list a vendor's apps, get one app's full entry ## System +### `permissions` (1 operation) + +Read the session firewall policy this server enforces on every route (issue #655). Read-only: `permissions set|reset` stay terminal actions on the host, so a bearer token can never widen the policy that constrains it. Mirrors `kbagent permissions show`. + +| Method | Path | Summary | +|---|---|---| +| `GET` | `/permissions/show` | Show the active permission policy | + ### `health` (6 operations) Liveness ping, auth-info bootstrap, version, changelog, and doctor checks. `/health/ping` is the only public endpoint -- everything else requires Bearer auth. diff --git a/docs/web-server.md b/docs/web-server.md index 9c7076f9..e9303de2 100644 --- a/docs/web-server.md +++ b/docs/web-server.md @@ -100,14 +100,13 @@ the web UI's chat. `agents` mirrors `kbagent agent *` (both sides read the same `agents.json`); what is serve-only there is the cron loop, so a scheduled task fires only while the server runs. `auth` mirrors only the read/audit half of `kbagent auth` *(since v0.90.1)* — `login` / -`login-password` / `logout` deliberately have no endpoint — and it is so far -the only router that enforces the permission policy; see "`/auth/*` — three -read/audit endpoints, three deliberate gaps" below. +`login-password` / `logout` deliberately have no endpoint. Going the other way, several CLI surfaces are deliberately CLI-only: `sync` -(filesystem-local by design), `permissions`, and `init`. The mirrors still -considered missing are tracked in #657, and the fact that `permissions` -constrains only `/auth/*` and not the other ~30 routers is #655. +(filesystem-local by design), `init`, and the *write* half of `permissions` +(`set` / `reset`). The mirrors still considered missing are tracked in #657. +`GET /permissions/show` *(since vNEXT)* is the read half — see "The session +firewall applies to every route" below. Auto-generated OpenAPI spec at `/openapi.json`, Swagger UI at `/docs`. @@ -485,12 +484,11 @@ None of the three response shapes (`ProjectCandidatesResult`, `RegisterProjectsResult`, `AuthStatusResult`) ever carries a token value, including the `kbc-session://` sentinel. -`/auth/*` is also the **first router to enforce the permission policy**: every -route above declares `Depends(require_permission(...))`, so a denied operation -answers **HTTP 403** with `error_code: PERMISSION_DENIED` — the same code the -CLI exits on. The other ~30 routers do not check the engine yet; see the -gotchas entry on this before assuming a deny policy firewalls the whole REST -surface. +`/auth/*` was the **first router to enforce the permission policy** (0.90.1): +every route above declares `Depends(require_permission(...))` at its own call +site. Since vNEXT that is no longer special — *every* route on the server is +enforced (see "The session firewall applies to every route" below) — and the +three inline declarations survive only as the per-route override form. The policy in force is the **persisted `permissions` block of the config dir `serve` resolves**, plus whichever session flags the `kbagent` invocation @@ -572,6 +570,62 @@ for a human), then use `POST /auth/register-projects` — or `auth register-projects` on the CLI — to register the resulting session's projects for `serve` to use. +### The session firewall applies to every route + +*(since vNEXT — issue #655)* + +`PermissionEngine` used to be built only in the Typer callback, so a persisted +`permissions set --mode deny` policy — and both `--deny-writes` and +`--deny-destructive` — protected the CLI process and nothing else. `kbagent +serve` exposed every route, `DELETE /storage/buckets` included, behind a single +all-or-nothing bearer token. 0.90.1 closed that for `/auth/*` only; vNEXT closes +it for the whole surface. + +**How it works.** One app-level dependency (`server/route_permissions.py`) runs +on every request, looks the matched route up by `(method, path template)` in +`ROUTE_OPERATIONS`, and calls the same `PermissionEngine.check_or_raise` the CLI +uses. A denial answers **HTTP 403** with `error_code: PERMISSION_DENIED` and the +same message the CLI prints, so a client can branch on one value across both +surfaces. + +Three properties worth knowing: + +- **A route with no classification is refused, not exempted.** Failing open + would make the newest, least-reviewed part of the surface the one outside the + firewall. A test (`tests/test_server_route_permissions.py`) asserts the table + matches the live app in both directions, so the refusal path should never be + reached in a released build — if you hit it, a route was added without a table + entry. +- **No policy configured changes nothing.** With a clean `config.json` and no + session flags, `is_allowed` returns True for everything and the surface + behaves exactly as it did before. +- **Bootstrap paths are never checked** (`UNGUARDED_PATHS`: `/health/ping`, + `/health/auth-info`, `/ui-config`, `/docs`, `/redoc`, `/openapi.json`, and the + SPA shell). A locked-down server must still be able to say who it is; + otherwise a client cannot tell a policy refusal from a dead process. + +**Granularity caveat.** A few routes are coarser than the CLI operation they +mirror. `POST /semantic-layer/items/{kind}` covers `metric`/`dataset`/… in one +route, so it maps to the collapsed parent key `semantic-layer.add`, not +`semantic-layer.add.metric`. A policy naming only a leaf key is enforced on the +CLI but not over REST — name the parent, or a `cli:*` category, to cover both. + +**Discovering the policy.** `GET /permissions/show` returns the *effective* +policy the server enforces — the persisted block already merged with the +`--deny-*` flags the daemon was launched with, which a REST caller can neither +see nor change: + +```json +{"active": true, "policy": {"mode": "allow", "allow": [], "deny": ["cli:destructive"]}} +``` + +It stays reachable under any policy (`permissions.*` operations are always +allowed, by the same anti-lockout rule the CLI has) but still requires the +bearer token. There is deliberately **no write counterpart**: letting a bearer +token widen the policy that constrains it would make the firewall +self-defeating, so `permissions set` / `reset` stay terminal actions on the +host. + ### Manage tokens are per-request Operations that need a Keboola Manage API token (`org setup`, diff --git a/plugins/kbagent/skills/kbagent/references/commands-reference.md b/plugins/kbagent/skills/kbagent/references/commands-reference.md index a7c9dfaf..db7391d5 100644 --- a/plugins/kbagent/skills/kbagent/references/commands-reference.md +++ b/plugins/kbagent/skills/kbagent/references/commands-reference.md @@ -92,6 +92,7 @@ All seven commands authenticate via `KBC_MANAGE_API_TOKEN` (Manage API), not the ## Permissions (session firewall commands) The `permissions` subcommands persist a write/destructive policy to config.json (the `--deny-*` flags above are the one-shot form). The engine guards against agent mistakes; it is not a sandbox. +Since vNEXT (#655) the persisted policy also firewalls the whole `kbagent serve` REST surface -- every route is checked against the SERVED config dir's policy and a denial answers HTTP 403 `PERMISSION_DENIED`. On 0.90.1 and older only `/auth/*` was checked. `GET /permissions/show` (serve-only, since vNEXT) reports the EFFECTIVE policy; there is deliberately no REST route that CHANGES it. - `permissions list [--category read|write|destructive|admin]` -- list all operations with their risk category and current allowed/denied status - `permissions show` -- show the current active permission policy - `permissions set --mode allow|deny [--allow PATTERN ...] [--deny PATTERN ...]` -- set the permission policy (firewall rules); patterns like `cli:read`, `cli:write`, `cli:destructive`. `tool:*` patterns are INERT since v0.85.0 (the MCP passthrough is gone) -- they load but match nothing, so a `--mode deny` policy whose only allowance was `tool:read` now denies everything diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index 79993bab..004c2dc7 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -4506,17 +4506,37 @@ shapes. keep) must be in its allow list, or the server will not start either. Pass `--config-dir` to `serve` itself: the root-level `kbagent --config-dir ... serve` sets the dir for the CLI invocation, not for the server process. -- **A deny policy does NOT firewall the whole REST surface.** - `permissions set --mode deny --deny cli:write` blocks `POST - /auth/register-projects` (HTTP 403, `error_code: PERMISSION_DENIED`) but - does nothing to `POST /storage/tables/{project}` (create a table) or any - other write route on any other router -- those still execute unchecked. -- `GET /auth/projects` backs a registry key (`auth.projects`) with no CLI leaf - command -- `auth register-projects`'s interactive picker is its terminal - equivalent. It is exempted from `scripts/check_command_sync.py`'s dead-key - check via `SERVE_ONLY_OPERATIONS` in `permissions.py`; see CONTRIBUTING.md's - command-sync gate section for the general rule when adding another - serve-only operation. -- Treat this as a gap being closed incrementally, not the design end state: - today a deny policy gates only `/auth/*`, not the ~30 other routers a - session token can otherwise reach. +- **A deny policy firewalls the WHOLE REST surface (since vNEXT, #655).** + Every route is classified in `server/route_permissions.py` and checked by + one app-level dependency against the served config dir's policy, so + `permissions set --mode deny --deny cli:write` now blocks `POST + /storage/tables/{project}` exactly like it blocks `POST + /auth/register-projects` -- HTTP 403, `error_code: PERMISSION_DENIED`, same + code the CLI exits on. **On 0.90.1 and older only `/auth/*` was checked**; + on those versions every other write/destructive route executed unchecked, so + do not rely on a deny policy to contain a `serve` you did not upgrade. + Bootstrap paths (`/health/ping`, `/health/auth-info`, `/ui-config`, `/docs`, + `/redoc`, `/openapi.json`, SPA shell) are never checked, by design. +- **`GET /permissions/show` (since vNEXT)** reports the EFFECTIVE policy -- + the persisted block already merged with the `--deny-*` flags the daemon was + launched with. Read-only: there is no REST way to change the policy, so an + agent cannot widen the firewall that constrains it. Reachable under any + policy (`permissions.*` is always allowed), but still needs the bearer token. +- **REST classification is coarser than the CLI for `semantic-layer` sub-apps.** + `POST /semantic-layer/items/{kind}` maps to the collapsed parent key + `semantic-layer.add`, not `semantic-layer.add.metric` (`kind` is a path + param). A policy naming only the leaf key is enforced on the CLI but not + over REST -- name the parent or a `cli:*` category to cover both. +- Three registry keys back a serve-only surface with no CLI leaf command: + `auth.projects` (the terminal equivalent is `auth register-projects`'s + interactive picker) plus `ai.chat` and `workspace.sql-improve` (since vNEXT -- + the dashboard's Local AI tile and the SQL editor's "improve this query" + helper; both are `write`, because both spawn a local CLI process on the host + exactly like `agent prompt-improve`). All three are exempted from + `scripts/check_command_sync.py`'s dead-key check via `SERVE_ONLY_OPERATIONS` + in `permissions.py`; see CONTRIBUTING.md's command-sync gate section before + adding another. +- A route with no entry in `ROUTE_OPERATIONS` is **refused** (403), not + exempted. A released build cannot reach that path -- a test asserts the + table matches the live app in both directions -- so seeing it means a route + was added without classifying it. diff --git a/src/keboola_agent_cli/commands/context.py b/src/keboola_agent_cli/commands/context.py index 4d6d4d49..4470aeb4 100644 --- a/src/keboola_agent_cli/commands/context.py +++ b/src/keboola_agent_cli/commands/context.py @@ -1917,7 +1917,10 @@ List all operations with risk categories and current allowed/denied status. kbagent permissions show - Show current active permission policy. + Show current active permission policy. The persisted policy also applies + to every route of `kbagent serve` (denial = HTTP 403 PERMISSION_DENIED), + against the config dir the SERVER resolved; `GET /permissions/show` over + serve reports that effective policy. No REST route can change it. kbagent permissions set --mode allow|deny [--allow PATTERN ...] [--deny PATTERN ...] Set firewall-style permission policy. Patterns: exact (branch.delete), diff --git a/src/keboola_agent_cli/permissions.py b/src/keboola_agent_cli/permissions.py index 844cb901..ac21696e 100644 --- a/src/keboola_agent_cli/permissions.py +++ b/src/keboola_agent_cli/permissions.py @@ -321,9 +321,19 @@ "semantic-layer.reference-data.get": "read", "semantic-layer.reference-data.set": "write", "semantic-layer.reference-data.delete": "destructive", + # Serve-only AI helpers (since vNEXT). Neither touches Keboola, but both + # spawn a local `claude` / `codex` / `gemini` process on the host, exactly + # like `agent prompt-improve` -- classified `write` for the same reason, so + # one --deny-writes keeps an agent from spawning subprocesses through any + # of the three. Exempted from the dead-key check via SERVE_ONLY_OPERATIONS. + "ai.chat": "write", + "workspace.sql-improve": "write", # Raw HTTP client against `kbagent serve` (used by AI subprocesses). # Categorised by the underlying HTTP method: GET = read, mutating verbs - # = write. The serve's own routes enforce their own permissions on top. + # = write. Since vNEXT (issue #655) the serve's own routes DO enforce the + # served config dir's policy on top, so a denied operation is refused at + # both ends; before that, this second layer did not exist and the claim + # this comment used to make was false. "http.get": "read", "http.post": "write", "http.patch": "write", @@ -393,7 +403,16 @@ # they have no CLI leaf command, so the command-sync gate would otherwise report # them as dead keys -- `scripts/check_command_sync.py` subtracts this set before # its "key matching no live command" check. -SERVE_ONLY_OPERATIONS: frozenset[str] = frozenset({"auth.projects"}) +SERVE_ONLY_OPERATIONS: frozenset[str] = frozenset( + { + "auth.projects", + # Both back a web-UI affordance with no terminal equivalent: the + # dashboard's Local AI chat tile and the workspace SQL editor's + # "improve this query" helper. + "ai.chat", + "workspace.sql-improve", + } +) # The operation namespace that disappeared with the MCP passthrough, and the @@ -528,6 +547,20 @@ def active(self) -> bool: """Whether a permission policy is configured.""" return self._policy is not None + @property + def policy(self) -> PermissionPolicy | None: + """The EFFECTIVE policy this engine evaluates, or None when unrestricted. + + On the serve side the engine is built by ``create_app`` from the served + directory's persisted policy already merged with ``--deny-writes`` / + ``--deny-destructive`` (:func:`apply_firewall_flags`), so this is what + ``GET /permissions/show`` must report: a caller needs the rules that + actually apply, not the persisted half of them. + + Read-only by design -- a policy is chosen once, at engine construction. + """ + return self._policy + def is_allowed(self, operation: str) -> bool: """Check if an operation is allowed by the active policy. diff --git a/src/keboola_agent_cli/server/app.py b/src/keboola_agent_cli/server/app.py index 0396ff1e..113eed9a 100644 --- a/src/keboola_agent_cli/server/app.py +++ b/src/keboola_agent_cli/server/app.py @@ -19,7 +19,7 @@ import secrets from contextlib import asynccontextmanager -from fastapi import FastAPI +from fastapi import Depends, FastAPI from fastapi.middleware.cors import CORSMiddleware from fastapi.openapi.utils import get_openapi from fastapi.responses import JSONResponse @@ -32,6 +32,7 @@ from .agents_store import AgentStore from .auth import PUBLIC_PATHS, AuthSettings, install_auth from .dependencies import ServiceRegistry, install_permission_engine, install_registry +from .route_permissions import enforce_route_permission from .routers import ( agents, ai_chat, @@ -64,6 +65,9 @@ transformation, workspaces, ) +from .routers import ( + permissions as permissions_router, +) logger = logging.getLogger(__name__) @@ -357,6 +361,17 @@ ), }, # ---- System ---- + { + "name": "permissions", + "description": ( + "**System.** " + "Read the session firewall policy this server enforces on every " + "route (issue #655). Read-only: `permissions set|reset` stay " + "terminal actions on the host, so a bearer token can never widen " + "the policy that constrains it. " + "Mirrors `kbagent permissions show`." + ), + }, { "name": "health", "description": ( @@ -674,6 +689,12 @@ async def _lifespan(app_: FastAPI): app = FastAPI( lifespan=_lifespan, # type: ignore[arg-type] + # The session firewall, applied to EVERY route the app declares + # (issue #655). App-level rather than per-router so a new router + # cannot be added outside it; the route -> operation classification + # lives in `route_permissions.ROUTE_OPERATIONS`, and an unclassified + # route is refused rather than silently exempted. + dependencies=[Depends(enforce_route_permission)], title="kbagent serve", description=APP_DESCRIPTION, version=__version__, @@ -780,6 +801,7 @@ async def _generic_handler(_request, exc: Exception): return _format_error(str(exc) or repr(exc), ErrorCode.INTERNAL_ERROR, http_status=500) app.include_router(health.router) + app.include_router(permissions_router.router) app.include_router(auth.router) app.include_router(projects.router) app.include_router(members.router) diff --git a/src/keboola_agent_cli/server/dependencies.py b/src/keboola_agent_cli/server/dependencies.py index 9fee92ee..31e2930b 100644 --- a/src/keboola_agent_cli/server/dependencies.py +++ b/src/keboola_agent_cli/server/dependencies.py @@ -227,6 +227,14 @@ def get_permission_engine(request: Request) -> PermissionEngine: return engine # type: ignore[no-any-return] +# Attribute stamped on the callable :func:`require_permission` returns, so the +# app-level dependency in ``route_permissions`` can recognise a route that +# already guards itself and skip its own (table-driven) lookup. A plain +# ``hasattr`` check on some ad-hoc name would be guesswork; a named constant +# makes the handshake between the two modules greppable. +PERMISSION_DEPENDENCY_MARKER = "kbagent_permission_operation" + + def require_permission(operation: str) -> Callable[[PermissionEngine], None]: """Build a FastAPI dependency enforcing the permission policy for ``operation``. @@ -245,6 +253,13 @@ def require_permission(operation: str) -> Callable[[PermissionEngine], None]: The engine comes from :func:`get_permission_engine` (app state), never from the registry, so overriding ``get_registry`` in a test cannot disable the check. + + Since 0.90.2 (issue #655) this is the per-route OVERRIDE form, not the + default one: every route is classified centrally in + ``server/route_permissions.py`` and checked by one app-level dependency. + Use this when a route needs an operation the table cannot express -- and + when you do, the app-level dependency stands down for that route + (:data:`PERMISSION_DEPENDENCY_MARKER` is how it recognises one). """ def _check_permission( @@ -252,6 +267,7 @@ def _check_permission( ) -> None: engine.check_or_raise(operation) + setattr(_check_permission, PERMISSION_DEPENDENCY_MARKER, operation) return _check_permission diff --git a/src/keboola_agent_cli/server/route_permissions.py b/src/keboola_agent_cli/server/route_permissions.py new file mode 100644 index 00000000..651749af --- /dev/null +++ b/src/keboola_agent_cli/server/route_permissions.py @@ -0,0 +1,467 @@ +"""Route-to-operation map that puts the REST surface behind the session firewall. + +Issue #655: ``PermissionEngine`` used to be built only in the Typer callback, +so a persisted ``permissions set --mode deny`` policy -- and both +``--deny-writes`` / ``--deny-destructive`` -- protected the CLI process and +nothing else. ``kbagent serve`` exposed every route, ``DELETE +/storage/buckets`` included, behind one all-or-nothing bearer token. + +#677 built the enforcement machinery (an engine on ``app.state``, a +``PermissionDeniedError`` -> HTTP 403 handler, and the +:func:`~keboola_agent_cli.server.dependencies.require_permission` dependency) +but wired it to three ``/auth/*`` routes only. This module supplies the +missing half: coverage. + +Why one central table instead of 231 per-route declarations +----------------------------------------------------------- +FastAPI puts the matched route object into ``request.scope["route"]`` *before* +dependencies run, so a single app-level dependency can look the request up by +``(method, route.path)``. That buys two things a scattered declaration cannot: + +1. **One auditable screen.** A reviewer answering "what can a caller still do + under ``--deny-destructive``?" reads one file, not thirty routers. +2. **Fail-closed by construction.** A route with no entry is *denied*, not + silently allowed -- the failure mode of a forgotten annotation is a loud + 403, never an open door. ``tests/test_server_route_permissions.py`` keeps + that path unreachable in practice by asserting the table covers the live + app exactly, in both directions. + +The per-route form still works and still wins: a route declaring +``Depends(require_permission(...))`` inline is enforced by that dependency and +skipped here (see :func:`resolve_route_operation`). That is what keeps the +``/auth/*`` routes from #677 -- and test-only probe routes registered after +``create_app`` -- working unchanged. + +Granularity caveat +------------------ +A few CLI operations are finer-grained than the route that mirrors them. +``POST /semantic-layer/items/{kind}`` covers ``metric``/``dataset``/... in one +route, so it maps to the collapsed parent key ``semantic-layer.add`` rather +than ``semantic-layer.add.metric``. A policy naming only the leaf key is +therefore enforced on the CLI but not over REST; name the parent (or +``cli:write``) to cover both. Documented in ``docs/web-server.md``. +""" + +from __future__ import annotations + +from fastapi import Depends, Request + +from ..permissions import OPERATION_REGISTRY, PermissionEngine +from .dependencies import get_permission_engine + +# Paths served without a permission check. +# +# All of them are bootstrap surface: they carry no project data and no side +# effects, and denying them would leave a caller unable to discover *why* it +# is being denied. ``/health/ping``, ``/health/auth-info``, ``/docs``, +# ``/redoc`` and ``/openapi.json`` are already unauthenticated +# (``server/auth.py``'s ``PUBLIC_PATHS``); ``/ui-config`` and the SPA shell +# routes still require the bearer token, they just carry no policy decision. +# +# FastAPI's own ``/docs``, ``/redoc`` and ``/openapi.json`` are plain Starlette +# routes registered inside ``FastAPI.__init__``, so the app-level dependency +# never attaches to them at all. They are listed anyway: the exemption should +# read as a decision, not as an accident of registration order. +UNGUARDED_PATHS: frozenset[str] = frozenset( + { + "/health/ping", + "/health/auth-info", + "/ui-config", + "/openapi.json", + "/docs", + "/docs/oauth2-redirect", + "/redoc", + # `kbagent serve --ui` SPA shell (see server/__init__.py::_install_ui). + "/", + "/index.html", + } +) + +# (HTTP method, route path template) -> OPERATION_REGISTRY key. +# +# The path is ``APIRoute.path`` verbatim -- including ``:path`` converters -- +# so an entry can be copy-pasted from, and grepped against, the router +# decorator it mirrors. +ROUTE_OPERATIONS: dict[tuple[str, str], str] = { + # ── health / meta ──────────────────────────────────────────────── + ("GET", "/version"): "version", + ("GET", "/changelog"): "changelog", + ("GET", "/doctor"): "doctor", + ("GET", "/permissions/show"): "permissions.show", + # ── projects ───────────────────────────────────────────────────── + ("GET", "/projects"): "project.list", + ("POST", "/projects"): "project.add", + # Serve-only bulk form of `project remove`; same blast radius, so the + # same admin-class key rather than a weaker one of its own. + ("POST", "/projects/bulk-delete"): "project.remove", + ("DELETE", "/projects/{alias}"): "project.remove", + ("PATCH", "/projects/{alias}"): "project.edit", + ("GET", "/projects/status"): "project.status", + ("GET", "/projects/current"): "project.current", + ("POST", "/projects/use/{alias}"): "project.use", + ("GET", "/projects/{alias}/info"): "project.info", + ("GET", "/projects/{alias}/description"): "project.description-get", + ("PUT", "/projects/{alias}/description"): "project.description-set", + # ── members ────────────────────────────────────────────────────── + ("GET", "/members/{project}"): "project.member-list", + ("GET", "/members/{project}/invitations"): "project.invitation-list", + ("POST", "/members/{project}/invite"): "project.invite", + ("POST", "/members/{project}/invitations/cancel"): "project.invitation-cancel", + ("POST", "/members/{project}/remove"): "project.member-remove", + ("POST", "/members/{project}/set-role"): "project.member-set-role", + # ── feature flags (super-admin manage token) ───────────────────── + ("GET", "/feature/{project}/list"): "feature.list", + ("GET", "/feature/{project}/project-show"): "feature.project-show", + ("POST", "/feature/{project}/project-add"): "feature.project-add", + ("POST", "/feature/{project}/project-remove"): "feature.project-remove", + ("GET", "/feature/{project}/user-show"): "feature.user-show", + ("POST", "/feature/{project}/user-add"): "feature.user-add", + ("POST", "/feature/{project}/user-remove"): "feature.user-remove", + # ── billing ────────────────────────────────────────────────────── + ("GET", "/billing/credits"): "billing.credits", + # ── configurations ─────────────────────────────────────────────── + ("GET", "/configs"): "config.list", + ("GET", "/configs/search"): "config.search", + ("GET", "/configs/examples/{component_id}"): "config.examples", + ("GET", "/configs/trash/{project}"): "config.trash-list", + ("GET", "/configs/{project}/{component_id}/{config_id}"): "config.detail", + ("PATCH", "/configs/{project}/{component_id}/{config_id}"): "config.update", + ("DELETE", "/configs/{project}/{component_id}/{config_id}"): "config.delete", + ("POST", "/configs/{project}/{component_id}/{config_id}/restore"): "config.restore", + ("POST", "/configs/{project}/{component_id}"): "config.new", + ("POST", "/configs/{project}/{component_id}/{config_id}/clone"): "config.clone", + ( + "POST", + "/configs/{project}/{component_id}/{config_id}/set-default-bucket", + ): "config.set-default-bucket", + ("POST", "/configs/{project}/{component_id}/{config_id}/rename"): "config.rename", + ("GET", "/configs/{project}/{component_id}/{config_id}/metadata"): "config.metadata-list", + ("GET", "/configs/{project}/{component_id}/{config_id}/metadata/{key}"): "config.get-metadata", + ("PUT", "/configs/{project}/{component_id}/{config_id}/metadata/{key}"): "config.set-metadata", + ( + "DELETE", + "/configs/{project}/{component_id}/{config_id}/metadata/{metadata_id}", + ): "config.delete-metadata", + ("POST", "/configs/{project}/{component_id}/{config_id}/folder"): "config.set-folder", + ("POST", "/configs/{project}/{component_id}/{config_id}/rows"): "config.row-create", + ("PATCH", "/configs/{project}/{component_id}/{config_id}/rows/{row_id}"): "config.row-update", + ("DELETE", "/configs/{project}/{component_id}/{config_id}/rows/{row_id}"): "config.row-delete", + ("GET", "/configs/{project}/{component_id}/{config_id}/oauth-url"): "config.oauth-url", + ("GET", "/configs/{project}/{component_id}/{config_id}/state"): "config.state-get", + ("PUT", "/configs/{project}/{component_id}/{config_id}/state"): "config.state-set", + ("GET", "/configs/{project}/{component_id}/{config_id}/variables"): "config.variables-get", + ("PUT", "/configs/{project}/{component_id}/{config_id}/variables"): "config.variables-set", + ( + "DELETE", + "/configs/{project}/{component_id}/{config_id}/variables", + ): "config.variables-clear", + # ── components ─────────────────────────────────────────────────── + ("GET", "/components"): "component.list", + ("GET", "/components/{component_id}"): "component.detail", + # Scaffolding writes a new configuration (`config new --push`). + ("POST", "/components/{component_id}/scaffold"): "config.new", + ("POST", "/components/{component_id}/actions/{action}"): "component.sync-action", + # ── storage: buckets ───────────────────────────────────────────── + ("GET", "/storage/buckets"): "storage.buckets", + ("GET", "/storage/buckets/{project}/{bucket_id:path}"): "storage.bucket-detail", + ("POST", "/storage/buckets/{project}"): "storage.create-bucket", + ("DELETE", "/storage/buckets/{project}"): "storage.delete-bucket", + ("POST", "/storage/buckets/{project}/{bucket_id:path}/describe"): "storage.describe-bucket", + # ── storage: tables ────────────────────────────────────────────── + ("GET", "/storage/tables"): "storage.tables", + ("GET", "/storage/table-detail/{project}/{table_id:path}"): "storage.table-detail", + # Both read table DATA; `download-table` is the CLI operation that does + # the same thing, and the preview is just a row-capped variant of it. + ("GET", "/storage/table-preview/{project}/{table_id:path}"): "storage.download-table", + ("GET", "/storage/table-download/{project}/{table_id:path}"): "storage.download-table", + ("POST", "/storage/tables/{project}"): "storage.create-table", + ("POST", "/storage/tables/{project}/upload"): "storage.upload-table", + ("DELETE", "/storage/tables/{project}"): "storage.delete-table", + ("POST", "/storage/tables/{project}/truncate"): "storage.truncate-table", + ("POST", "/storage/tables/{project}/{table_id:path}/swap"): "storage.swap-tables", + ("POST", "/storage/tables/{project}/{table_id:path}/pull"): "storage.clone-table", + ("POST", "/storage/tables/{project}/{table_id:path}/describe"): "storage.describe-table", + # ── storage: columns ───────────────────────────────────────────── + ("POST", "/storage/columns/{project}/{table_id:path}"): "storage.add-column", + ("DELETE", "/storage/columns/{project}/{table_id:path}"): "storage.delete-column", + ("POST", "/storage/columns/{project}/{table_id:path}/describe"): "storage.describe-column", + ("POST", "/storage/columns/{project}/describe-migrate"): "storage.describe-migrate", + # ── storage: snapshots ─────────────────────────────────────────── + ("POST", "/storage/tables/{project}/{table_id:path}/snapshots"): "storage.snapshot-create", + ("GET", "/storage/snapshots/{project}/{table_id:path}"): "storage.snapshots", + ("GET", "/storage/snapshot-detail/{project}/{snapshot_id}"): "storage.snapshot-detail", + ("DELETE", "/storage/snapshots/{project}"): "storage.snapshot-delete", + ("POST", "/storage/table-from-snapshot/{project}"): "storage.table-from-snapshot", + # ── storage: files ─────────────────────────────────────────────── + ("GET", "/storage/files"): "storage.files", + ("POST", "/storage/files/upload"): "storage.file-upload", + ("GET", "/storage/files/{project}/{file_id}"): "storage.file-detail", + ("GET", "/storage/files/{project}/{file_id}/download"): "storage.file-download", + ("DELETE", "/storage/files/{project}"): "storage.file-delete", + ("POST", "/storage/files/{project}/{file_id}/tag"): "storage.file-tag", + ("POST", "/storage/files/{project}/load-to-table"): "storage.load-file", + # ── data streams ───────────────────────────────────────────────── + ("GET", "/stream/{project}/list"): "stream.list", + ("GET", "/stream/{project}/detail"): "stream.detail", + ("POST", "/stream/{project}/create-source"): "stream.create-source", + ("POST", "/stream/{project}/delete"): "stream.delete", + # ── scoped storage tokens ──────────────────────────────────────── + ("GET", "/token/list"): "token.list", + ("GET", "/token/{project}/list"): "token.list", + ("POST", "/token/{project}/create"): "token.create", + ("POST", "/token/{project}/delete"): "token.delete", + ("POST", "/token/{project}/refresh"): "token.refresh", + # ── jobs ───────────────────────────────────────────────────────── + ("GET", "/jobs"): "job.list", + ("GET", "/jobs/{project}/{job_id}"): "job.detail", + # SSE tail of one job -- the streaming form of `job detail`. + ("GET", "/jobs/{project}/{job_id}/stream"): "job.detail", + ("POST", "/jobs/{project}/run"): "job.run", + ("POST", "/jobs/{project}/terminate"): "job.terminate", + # ── branches ───────────────────────────────────────────────────── + ("GET", "/branches"): "branch.list", + ("POST", "/branches/{project}"): "branch.create", + ("POST", "/branches/{project}/use"): "branch.use", + ("POST", "/branches/{project}/reset"): "branch.reset", + ("DELETE", "/branches/{project}/{branch_id}"): "branch.delete", + # `branch merge` is itself only a URL producer (the merge happens in the + # web UI), so the GET mirrors it exactly -- including its `write` class. + ("GET", "/branches/{project}/merge-url"): "branch.merge", + ("GET", "/branches/{project}/metadata"): "branch.metadata-list", + ("GET", "/branches/{project}/metadata/{key}"): "branch.metadata-get", + ("PUT", "/branches/{project}/metadata/{key}"): "branch.metadata-set", + ("DELETE", "/branches/{project}/metadata/{metadata_id}"): "branch.metadata-delete", + # ── workspaces ─────────────────────────────────────────────────── + ("GET", "/workspaces"): "workspace.list", + ("POST", "/workspaces/{project}"): "workspace.create", + ("GET", "/workspaces/{project}/{workspace_id}"): "workspace.detail", + ("DELETE", "/workspaces/{project}/{workspace_id}"): "workspace.delete", + ("POST", "/workspaces/{project}/{workspace_id}/password"): "workspace.password", + ("POST", "/workspaces/{project}/{workspace_id}/load"): "workspace.load", + ("POST", "/workspaces/{project}/{workspace_id}/query"): "workspace.query", + ("POST", "/workspaces/{project}/from-transformation"): "workspace.from-transformation", + ("POST", "/workspaces/gc"): "workspace.gc", + ("POST", "/workspaces/sql/improve/stream"): "workspace.sql-improve", + # ── flows ──────────────────────────────────────────────────────── + ("GET", "/flows"): "flow.list", + ("GET", "/flows/examples"): "flow.examples", + ("POST", "/flows/validate"): "flow.validate", + ("GET", "/flows/{project}/schema"): "flow.schema", + ("GET", "/flows/{project}/{config_id}"): "flow.detail", + ("POST", "/flows/{project}"): "flow.new", + ("PATCH", "/flows/{project}/{config_id}"): "flow.update", + ("DELETE", "/flows/{project}/{config_id}"): "flow.delete", + ("GET", "/flows/{project}/{config_id}/schedules"): "schedule.list", + ("POST", "/flows/{project}/{config_id}/schedule"): "flow.schedule", + ("DELETE", "/flows/{project}/{config_id}/schedule"): "flow.schedule-remove", + # ── schedules / notifications ──────────────────────────────────── + ("GET", "/schedules"): "schedule.list", + ("GET", "/schedules/{project}/{schedule_id}"): "schedule.detail", + ("GET", "/schedules/find/query"): "schedule.find", + ("GET", "/notifications"): "notification.list", + ("GET", "/notifications/{project}/{subscription_id}"): "notification.detail", + # ── lineage (all read-only; `build` is the only cache writer) ───── + ("POST", "/lineage/build"): "lineage.build", + ("GET", "/lineage/info"): "lineage.info", + ("POST", "/lineage/show"): "lineage.show", + ("GET", "/lineage/edges"): "lineage.show", + ("GET", "/lineage/browser"): "lineage.show", + ("GET", "/lineage/data"): "lineage.show", + ("GET", "/lineage/walk"): "lineage.show", + ("GET", "/lineage/mermaid"): "lineage.show", + # ── sharing ────────────────────────────────────────────────────── + ("GET", "/sharing"): "sharing.list", + ("GET", "/sharing/edges"): "sharing.edges", + ("POST", "/sharing/{project}/share"): "sharing.share", + ("POST", "/sharing/{project}/unshare/{bucket_id:path}"): "sharing.unshare", + ("POST", "/sharing/{project}/link"): "sharing.link", + ("POST", "/sharing/{project}/unlink/{bucket_id:path}"): "sharing.unlink", + # ── data apps ──────────────────────────────────────────────────── + ("GET", "/data-apps"): "data-app.list", + ("GET", "/data-apps/{project}/{app_id}"): "data-app.detail", + ("POST", "/data-apps/{project}"): "data-app.create", + ("POST", "/data-apps/{project}/{app_id}/deploy"): "data-app.deploy", + ("POST", "/data-apps/{project}/{app_id}/start"): "data-app.start", + ("POST", "/data-apps/{project}/{app_id}/stop"): "data-app.stop", + ("DELETE", "/data-apps/{project}/{app_id}"): "data-app.delete", + ("GET", "/data-apps/{project}/{app_id}/password"): "data-app.password", + ("GET", "/data-apps/{project}/{app_id}/logs"): "data-app.logs", + ("GET", "/data-apps/{project}/{app_id}/runs"): "data-app.runs", + ("GET", "/data-apps/{project}/{app_id}/secrets"): "data-app.secrets-list", + ("GET", "/data-apps/{project}/{app_id}/secrets/{key:path}"): "data-app.secrets-get", + ("PUT", "/data-apps/{project}/{app_id}/secrets"): "data-app.secrets-set", + ("POST", "/data-apps/{project}/{app_id}/secrets/remove"): "data-app.secrets-remove", + ("POST", "/data-apps/validate-repo"): "data-app.validate-repo", + ("GET", "/data-apps/{project}/{app_id}/git-repo"): "data-app.git-repo", + ("GET", "/data-apps/{project}/{app_id}/git-repo/credentials"): "data-app.git-credentials", + ( + "POST", + "/data-apps/{project}/{app_id}/git-repo/credentials", + ): "data-app.git-credentials-create", + # ── developer portal ───────────────────────────────────────────── + ("GET", "/dev-portal/apps"): "dev-portal.list", + ("GET", "/dev-portal/apps/{app}"): "dev-portal.get", + # ── Kai / local AI ─────────────────────────────────────────────── + ("GET", "/kai/ping"): "kai.ping", + ("GET", "/kai/preflight"): "kai.preflight", + ("POST", "/kai/ask"): "kai.ask", + ("POST", "/kai/chat"): "kai.chat", + ("GET", "/kai/history"): "kai.history", + ("GET", "/kai/chat/{chat_id}"): "kai.chat-detail", + ("POST", "/ai/chat/stream"): "ai.chat", + # ── encrypt / search / docs ────────────────────────────────────── + ("POST", "/encrypt/values"): "encrypt.values", + ("GET", "/search"): "search", + ("POST", "/documentation/query"): "docs.query", + # ── semantic layer ─────────────────────────────────────────────── + ("GET", "/semantic-layer/models"): "semantic-layer.model.list", + ("POST", "/semantic-layer/models"): "semantic-layer.model.create", + ("DELETE", "/semantic-layer/models/{model}"): "semantic-layer.model.delete", + ("GET", "/semantic-layer/show"): "semantic-layer.show", + ("GET", "/semantic-layer/validate"): "semantic-layer.validate", + ("GET", "/semantic-layer/search-context"): "semantic-layer.search-context", + ("GET", "/semantic-layer/get-context"): "semantic-layer.get-context", + ("GET", "/semantic-layer/schema"): "semantic-layer.schema", + ("GET", "/semantic-layer/export"): "semantic-layer.export", + ("POST", "/semantic-layer/diff"): "semantic-layer.diff", + # Collapsed parent keys -- `kind` is a path param, see "Granularity + # caveat" in the module docstring. + ("POST", "/semantic-layer/items/{kind}"): "semantic-layer.add", + ("PUT", "/semantic-layer/items/{kind}/{name}"): "semantic-layer.edit", + ("DELETE", "/semantic-layer/items/{kind}/{name}"): "semantic-layer.remove", + ("POST", "/semantic-layer/import"): "semantic-layer.import", + ("POST", "/semantic-layer/promote"): "semantic-layer.promote", + ("POST", "/semantic-layer/build"): "semantic-layer.build", + ("POST", "/semantic-layer/token/encrypt"): "semantic-layer.token", + ("GET", "/semantic-layer/reference-data"): "semantic-layer.reference-data.list", + ("GET", "/semantic-layer/reference-data/{record_id}"): "semantic-layer.reference-data.get", + ("PUT", "/semantic-layer/reference-data"): "semantic-layer.reference-data.set", + ( + "DELETE", + "/semantic-layer/reference-data/{record_id}", + ): "semantic-layer.reference-data.delete", + # ── transformations ────────────────────────────────────────────── + ("POST", "/transformations/{project}"): "transformation.create", + ("GET", "/transformations/{project}/{config_id}"): "transformation.show", + ("PATCH", "/transformations/{project}/{config_id}"): "transformation.edit", + # ── organization ───────────────────────────────────────────────── + ("POST", "/org/setup"): "org.setup", + # `--refresh` is a flag on the same command, not a command of its own. + ("POST", "/org/refresh"): "org.setup", + # ── scheduled agent tasks ──────────────────────────────────────── + ("GET", "/agents"): "agent.list", + ("POST", "/agents"): "agent.create", + ("GET", "/agents/cron/preview"): "agent.cron-preview", + ("POST", "/agents/test"): "agent.test", + ("POST", "/agents/test/stream"): "agent.test", + ("POST", "/agents/prompt/improve"): "agent.prompt-improve", + ("POST", "/agents/prompt/improve/stream"): "agent.prompt-improve", + ("GET", "/agents/{task_id}"): "agent.show", + ("PATCH", "/agents/{task_id}"): "agent.update", + ("DELETE", "/agents/{task_id}"): "agent.delete", + ("POST", "/agents/{task_id}/run"): "agent.run", + ("POST", "/agents/{task_id}/run/stream"): "agent.run", + ("GET", "/agents/{task_id}/runs"): "agent.runs", + ("GET", "/agents/{task_id}/runs/{run_id}"): "agent.run-detail", + ("GET", "/agents/{task_id}/runs/{run_id}/events"): "agent.run-events", +} + + +def resolve_route_operation(method: str, path: str) -> str | None: + """Return the operation key guarding ``method path``, or None when exempt. + + ``path`` is the route TEMPLATE (``APIRoute.path``), never the concrete + request URL -- looking policy up by a concrete URL would make the decision + depend on user-supplied identifiers. + """ + if path in UNGUARDED_PATHS: + return None + return ROUTE_OPERATIONS.get((method.upper(), path)) + + +def enforce_route_permission( + request: Request, + engine: PermissionEngine = Depends(get_permission_engine), +) -> None: + """App-level dependency: check the active policy for the matched route. + + Registered once on the ``FastAPI`` instance, so it runs for every route the + app declares -- there is no per-router opt-in to forget. FastAPI resolves + the route before dependencies run, so ``request.scope["route"]`` is the + matched :class:`~fastapi.routing.APIRoute` and its ``.path`` is the + template the table is keyed on. + + Three outcomes: + + * exempt path (:data:`UNGUARDED_PATHS`) -> allowed, no policy consulted; + * mapped route -> ``engine.check_or_raise(operation)``, i.e. HTTP 403 with + ``error_code: PERMISSION_DENIED`` when the policy says no; + * unmapped route -> **denied**. See :func:`_deny_unmapped`. + + With no policy configured (the default) ``is_allowed`` returns True for + everything, so a mapped route behaves exactly as it did before this + dependency existed. Only an unmapped route changes behavior -- and the + completeness test makes that state unreachable in a released build. + """ + route = request.scope.get("route") + path = getattr(route, "path", None) + if path is None: + # No matched APIRoute (a Mount, or a 404 that never resolved). Nothing + # to authorize; Starlette will answer for it. + return + + if path in UNGUARDED_PATHS: + return + + operation = ROUTE_OPERATIONS.get((request.method.upper(), path)) + if operation is None: + if _declares_inline_permission(route): + # The route carries its own `require_permission(...)`, which has + # already run (route dependencies are resolved after app-level + # ones). Checking again here would need an operation the table + # does not have; the inline guard is the authority. + return + _deny_unmapped(request.method.upper(), path) + return + + engine.check_or_raise(operation) + + +def _declares_inline_permission(route: object) -> bool: + """Whether ``route`` declares a ``require_permission(...)`` dependency itself.""" + from .dependencies import PERMISSION_DEPENDENCY_MARKER + + for dependant in getattr(route, "dependencies", ()) or (): + call = getattr(dependant, "dependency", None) + if getattr(call, PERMISSION_DEPENDENCY_MARKER, None) is not None: + return True + return False + + +def _deny_unmapped(method: str, path: str) -> None: + """Refuse a route no entry in :data:`ROUTE_OPERATIONS` classifies. + + Failing open here would mean a route added without a table entry is exempt + from every policy -- silently, and exactly for the newest and least + reviewed part of the surface. Failing closed makes the omission a loud + 403 the first time anyone calls the route, and + ``tests/test_server_route_permissions.py`` turns it into a failing test + long before that. + """ + from ..errors import PermissionDeniedError + + raise PermissionDeniedError( + f"{method} {path} has no permission classification, so its risk cannot be " + "evaluated and it is refused. Add an entry to ROUTE_OPERATIONS in " + "server/route_permissions.py (or list the path in UNGUARDED_PATHS)." + ) + + +def unknown_operations() -> list[str]: + """Table values that are not :data:`OPERATION_REGISTRY` keys. + + A typo'd operation would be classified ``write`` by the engine's own + fail-closed default and never match an exact-name policy pattern -- wrong + in a way nothing else notices. The completeness test asserts this is empty. + """ + return sorted({op for op in ROUTE_OPERATIONS.values() if op not in OPERATION_REGISTRY}) diff --git a/src/keboola_agent_cli/server/routers/permissions.py b/src/keboola_agent_cli/server/routers/permissions.py new file mode 100644 index 00000000..f0af6eee --- /dev/null +++ b/src/keboola_agent_cli/server/routers/permissions.py @@ -0,0 +1,63 @@ +"""Read-only discovery of the firewall policy the REST surface enforces. + +Issue #655 asked for this alongside enforcement, and the two belong together: +once a route can answer 403 PERMISSION_DENIED, a client needs a way to learn +the rules *before* it starts making calls it is not allowed to make. Without +it the only discovery channel is trial and error against destructive routes. + +There is deliberately no write counterpart. ``permissions set`` / ``reset`` +edit ``config.json`` on the host, and letting a bearer token widen the very +policy that constrains it would make the firewall self-defeating -- a policy +change stays a terminal action on the machine running the server. +""" + +from __future__ import annotations + +from typing import Any + +from fastapi import APIRouter, Depends + +from ...permissions import ( + INERT_PATTERN_HINT, + INERT_SINCE_VERSION, + PermissionEngine, + find_inert_patterns, +) +from ..dependencies import get_permission_engine + +router = APIRouter(prefix="/permissions", tags=["permissions"]) + + +@router.get("/show", summary="Show the active permission policy") +def show(engine: PermissionEngine = Depends(get_permission_engine)) -> dict[str, Any]: + """Report the policy every route on this server is checked against. + + Mirrors `kbagent permissions show --json`, with one deliberate difference: + the CLI reports the persisted policy and the session ``--deny-*`` flags as + two separate layers, while this returns the single EFFECTIVE policy the + server enforces -- ``create_app`` merges the two at startup and a REST + caller can neither see nor change the flags the daemon was launched with. + + ``active`` is False when no policy is configured at all; ``policy`` is then + null and every operation is allowed. + """ + policy = engine.policy + payload: dict[str, Any] = { + "active": policy is not None, + "policy": ( + None + if policy is None + else {"mode": policy.mode, "allow": list(policy.allow), "deny": list(policy.deny)} + ), + } + + # Additive keys, present only when the policy carries rules that can no + # longer match anything -- same wording the CLI and `doctor` use, so a + # client cannot learn a third phrasing of one problem. + inert = find_inert_patterns(policy) + if inert: + payload["inert_patterns"] = inert + payload["inert_since_version"] = INERT_SINCE_VERSION + payload["inert_hint"] = INERT_PATTERN_HINT + + return payload diff --git a/tests/test_server_route_permissions.py b/tests/test_server_route_permissions.py new file mode 100644 index 00000000..1c8f8980 --- /dev/null +++ b/tests/test_server_route_permissions.py @@ -0,0 +1,379 @@ +"""Tests for the app-wide route firewall on ``kbagent serve`` (issue #655). + +``tests/test_server_permissions.py`` pins the SEAM #677 built -- the engine on +``app.state``, ``require_permission``, and the 403 envelope. This file pins the +COVERAGE #655 asked for: every route on the app is classified, the +classification is checked on every request, and an unclassified route is +refused rather than silently exempted. + +The completeness tests are the load-bearing ones. They are what makes the +runtime fail-closed branch unreachable in a released build: a route added +without a table entry fails here long before anyone meets its 403. +""" + +from __future__ import annotations + +import importlib.util +from pathlib import Path +from typing import Any, ClassVar + +import pytest + +if importlib.util.find_spec("fastapi") is None: # pragma: no cover + pytest.skip( + "FastAPI not installed; run `uv pip install -e '.[server]'`", allow_module_level=True + ) + +from fastapi.testclient import TestClient + +from keboola_agent_cli.config_store import ConfigStore +from keboola_agent_cli.models import PermissionPolicy +from keboola_agent_cli.permissions import OPERATION_REGISTRY, SERVE_ONLY_OPERATIONS +from keboola_agent_cli.server import create_app +from keboola_agent_cli.server.dependencies import PERMISSION_DEPENDENCY_MARKER +from keboola_agent_cli.server.route_permissions import ( + ROUTE_OPERATIONS, + UNGUARDED_PATHS, + resolve_route_operation, + unknown_operations, +) + +TOKEN = "route-perm-token" +AUTH = {"Authorization": f"Bearer {TOKEN}"} + + +def _persist_policy(config_dir: Path, policy: PermissionPolicy) -> None: + store = ConfigStore(config_dir=config_dir) + config = store.load() + config.permissions = policy + store.save(config) + + +def _app(tmp_path: Path, **kwargs: Any) -> Any: + return create_app(config_dir=str(tmp_path), auth_token=TOKEN, **kwargs) + + +def _client(tmp_path: Path, **kwargs: Any) -> TestClient: + return TestClient(_app(tmp_path, **kwargs)) + + +def _iter_api_routes(app: Any) -> list[Any]: + """Every real ``APIRoute`` on ``app``, expanding lazy router includes. + + FastAPI 0.137 stopped flattening ``include_router`` eagerly: ``app.routes`` + holds one ``_IncludedRouter`` proxy per include (35 of them here) instead of + the 236 routes they stand for, and nothing materialises them -- not + ``app.openapi()``, not ``TestClient`` startup. Request handling is + unaffected (``request.scope["route"]`` is still the real ``APIRoute``, with + its full path template), but a test that walks ``app.routes`` naively would + audit four routes, find nothing wrong, and pass. A coverage test that can + pass by seeing nothing is worse than no coverage test, hence the recursion. + """ + stack, seen, routes = list(app.routes), set(), [] + while stack: + route = stack.pop() + if id(route) in seen: + continue + seen.add(id(route)) + inner = getattr(route, "original_router", None) + if inner is not None: + stack.extend(inner.routes) + continue + if getattr(route, "methods", None): + routes.append(route) + return routes + + +def _live_routes(app: Any) -> set[tuple[str, str]]: + """Every (method, path template) the app answers, minus HEAD/OPTIONS.""" + return { + (method, route.path) + for route in _iter_api_routes(app) + for method in route.methods + if method not in ("HEAD", "OPTIONS") + } + + +def _declares_inline_guard(app: Any, method: str, path: str) -> bool: + for route in _iter_api_routes(app): + if route.path != path or method not in route.methods: + continue + for dependant in getattr(route, "dependencies", ()) or (): + call = getattr(dependant, "dependency", None) + if getattr(call, PERMISSION_DEPENDENCY_MARKER, None) is not None: + return True + return False + + +class TestTableCoversTheLiveApp: + """The three-way partition: mapped, exempt, or inline-guarded. No fourth case.""" + + def test_every_route_is_classified(self, tmp_path: Path) -> None: + app = _app(tmp_path) + unclassified = sorted( + f"{method} {path}" + for method, path in _live_routes(app) + if (method, path) not in ROUTE_OPERATIONS + and path not in UNGUARDED_PATHS + and not _declares_inline_guard(app, method, path) + ) + assert unclassified == [], ( + "These routes have no permission classification and would be REFUSED at " + "runtime. Add an entry to ROUTE_OPERATIONS in server/route_permissions.py, " + "or list the path in UNGUARDED_PATHS if it is bootstrap surface: " + f"{unclassified}" + ) + + def test_no_stale_entries(self, tmp_path: Path) -> None: + """A table entry for a route that no longer exists is dead weight. + + Worse than dead: it reads as coverage during a security review while + classifying nothing at all. + """ + live = _live_routes(_app(tmp_path)) + stale = sorted( + f"{method} {path}" for method, path in ROUTE_OPERATIONS if (method, path) not in live + ) + assert stale == [], f"ROUTE_OPERATIONS entries matching no live route: {stale}" + + def test_every_operation_is_a_registry_key(self) -> None: + """A typo'd operation silently defaults to 'write' and matches no exact pattern.""" + assert unknown_operations() == [] + + def test_auth_routes_are_covered_by_their_inline_guards(self, tmp_path: Path) -> None: + """#677's three routes stay enforced without a table entry.""" + app = _app(tmp_path) + for method, path in [ + ("GET", "/auth/projects"), + ("GET", "/auth/status"), + ("POST", "/auth/register-projects"), + ]: + assert (method, path) not in ROUTE_OPERATIONS + assert _declares_inline_guard(app, method, path), f"{method} {path} lost its guard" + + +class TestResolveRouteOperation: + def test_exempt_path_resolves_to_none(self) -> None: + assert resolve_route_operation("GET", "/health/ping") is None + + def test_mapped_route_resolves(self) -> None: + assert resolve_route_operation( + "DELETE", "/configs/{project}/{component_id}/{config_id}" + ) == ("config.delete") + + def test_method_is_part_of_the_key(self) -> None: + """Same path, different verb, different risk -- the classic mapping bug.""" + path = "/configs/{project}/{component_id}/{config_id}" + assert resolve_route_operation("GET", path) == "config.detail" + assert resolve_route_operation("DELETE", path) == "config.delete" + + def test_lowercase_method_is_accepted(self) -> None: + assert resolve_route_operation("get", "/projects") == "project.list" + + def test_unmapped_route_resolves_to_none(self) -> None: + assert resolve_route_operation("GET", "/nope/not/a/route") is None + + +class TestEnforcementOnRealRoutes: + """The behaviour #655 reported missing, on routes it named.""" + + def test_destructive_route_is_denied(self, tmp_path: Path) -> None: + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=["cli:destructive"])) + response = _client(tmp_path).request( + "DELETE", "/storage/buckets/demo", headers=AUTH, json={} + ) + assert response.status_code == 403 + body = response.json() + assert body["error"]["code"] == "PERMISSION_DENIED" + assert "storage.delete-bucket" in body["error"]["message"] + + def test_read_route_still_allowed_under_a_destructive_deny(self, tmp_path: Path) -> None: + """The firewall must stay surgical -- denying deletes cannot break listing.""" + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=["cli:destructive"])) + response = _client(tmp_path).get("/projects", headers=AUTH) + assert response.status_code == 200 + + def test_deny_writes_flag_blocks_a_write_route(self, tmp_path: Path) -> None: + client = _client(tmp_path, deny_writes=True) + response = client.post("/jobs/demo/run", headers=AUTH, json={}) + assert response.status_code == 403 + assert response.json()["error"]["code"] == "PERMISSION_DENIED" + + def test_deny_writes_flag_leaves_reads_alone(self, tmp_path: Path) -> None: + client = _client(tmp_path, deny_writes=True) + assert client.get("/version", headers=AUTH).status_code == 200 + + def test_token_delete_is_denied_by_an_exact_pattern(self, tmp_path: Path) -> None: + """Exact-name patterns work over REST, not just category ones.""" + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=["token.delete"])) + client = _client(tmp_path) + assert client.post("/token/demo/delete", headers=AUTH, json={}).status_code == 403 + + def test_default_install_denies_nothing(self, tmp_path: Path) -> None: + """No policy configured -> the surface behaves exactly as before #655.""" + client = _client(tmp_path) + assert client.get("/projects", headers=AUTH).status_code == 200 + assert client.get("/version", headers=AUTH).status_code == 200 + + def test_mode_deny_still_serves_bootstrap_paths(self, tmp_path: Path) -> None: + """A locked-down server must still be able to say who it is. + + Denying `/health/ping` would leave a client unable to distinguish a + policy refusal from a dead process. + """ + _persist_policy(tmp_path, PermissionPolicy(mode="deny", allow=[])) + client = _client(tmp_path) + assert client.get("/health/ping").status_code == 200 + assert client.get("/health/auth-info", headers=AUTH).status_code == 200 + + def test_mode_deny_blocks_an_unlisted_read(self, tmp_path: Path) -> None: + _persist_policy(tmp_path, PermissionPolicy(mode="deny", allow=["project.list"])) + client = _client(tmp_path) + assert client.get("/projects", headers=AUTH).status_code == 200 + assert client.get("/storage/buckets", headers=AUTH).status_code == 403 + + +class TestUnmappedRouteFailsClosed: + def test_route_added_without_a_table_entry_is_refused(self, tmp_path: Path) -> None: + app = _app(tmp_path) + + @app.get("/_unclassified") + def _unclassified() -> dict[str, str]: # pragma: no cover - never reached + return {"status": "ok"} + + response = TestClient(app).get("/_unclassified", headers=AUTH) + assert response.status_code == 403 + assert response.json()["error"]["code"] == "PERMISSION_DENIED" + assert "ROUTE_OPERATIONS" in response.json()["error"]["message"] + + def test_inline_guarded_route_without_a_table_entry_is_allowed(self, tmp_path: Path) -> None: + """The per-route override still works -- that is what keeps /auth/* alive.""" + from fastapi import Depends + + from keboola_agent_cli.server.dependencies import require_permission + + app = _app(tmp_path) + + @app.get("/_inline", dependencies=[Depends(require_permission("config.list"))]) + def _inline() -> dict[str, str]: + return {"status": "ok"} + + client = TestClient(app) + assert client.get("/_inline", headers=AUTH).status_code == 200 + + def test_inline_guarded_route_is_still_subject_to_its_own_policy(self, tmp_path: Path) -> None: + from fastapi import Depends + + from keboola_agent_cli.server.dependencies import require_permission + + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=["config.list"])) + app = _app(tmp_path) + + @app.get("/_inline", dependencies=[Depends(require_permission("config.list"))]) + def _inline() -> dict[str, str]: # pragma: no cover - never reached + return {"status": "ok"} + + assert TestClient(app).get("/_inline", headers=AUTH).status_code == 403 + + +class TestPermissionsShowEndpoint: + def test_reports_no_policy_when_clean(self, tmp_path: Path) -> None: + body = _client(tmp_path).get("/permissions/show", headers=AUTH).json() + assert body == {"active": False, "policy": None} + + def test_reports_the_persisted_policy(self, tmp_path: Path) -> None: + _persist_policy(tmp_path, PermissionPolicy(mode="deny", allow=["cli:read"], deny=[])) + body = _client(tmp_path).get("/permissions/show", headers=AUTH).json() + assert body["active"] is True + assert body["policy"] == {"mode": "deny", "allow": ["cli:read"], "deny": []} + + def test_reports_the_effective_policy_including_session_flags(self, tmp_path: Path) -> None: + """The merged view is the point: a REST caller cannot see the daemon's flags.""" + body = ( + _client(tmp_path, deny_destructive=True).get("/permissions/show", headers=AUTH).json() + ) + assert body["active"] is True + assert body["policy"]["deny"] == ["cli:destructive"] + + def test_surfaces_inert_patterns(self, tmp_path: Path) -> None: + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=["tool:write"])) + body = _client(tmp_path).get("/permissions/show", headers=AUTH).json() + assert body["inert_patterns"] == ["tool:write"] + assert body["inert_since_version"] == "0.85.0" + + def test_is_reachable_under_a_total_deny(self, tmp_path: Path) -> None: + """Discovery must survive the policy it describes, or it is useless.""" + _persist_policy(tmp_path, PermissionPolicy(mode="deny", allow=[])) + response = _client(tmp_path).get("/permissions/show", headers=AUTH) + assert response.status_code == 200 + assert response.json()["active"] is True + + def test_still_requires_the_bearer_token(self, tmp_path: Path) -> None: + assert _client(tmp_path).get("/permissions/show").status_code == 401 + + +class TestServeOnlyOperationsAreRegistered: + """New serve-only keys must be real registry entries AND declared serve-only.""" + + @pytest.mark.parametrize("operation", ["ai.chat", "workspace.sql-improve"]) + def test_operation_is_registered_and_declared_serve_only(self, operation: str) -> None: + assert operation in OPERATION_REGISTRY + assert operation in SERVE_ONLY_OPERATIONS + + +class TestVerbAndRiskClassAgree: + """Cheap structural checks that catch the mapping mistakes worth catching. + + Neither is a law of nature -- HTTP verbs and risk classes are different + vocabularies -- so both carry an explicit allowlist. The point is that a + disagreement has to be *chosen*: a new DELETE quietly classified ``read`` + would be a firewall hole nothing else notices. + """ + + # POST because the request needs a request BODY, not because it mutates. + _NON_MUTATING_POSTS: ClassVar[set[str]] = { + "POST /data-apps/validate-repo", + "POST /documentation/query", + "POST /flows/validate", + "POST /kai/ask", + "POST /lineage/build", + "POST /lineage/show", + "POST /semantic-layer/diff", + "POST /workspaces/{project}/{workspace_id}/password", + } + # `branch merge` only ever produces a URL (the merge happens in the web + # UI), so mirroring its `write` class on a GET is CLI parity, not a slip. + _NON_READ_GETS: ClassVar[set[str]] = {"GET /branches/{project}/merge-url"} + + def test_every_delete_route_is_destructive_or_admin(self) -> None: + offenders = sorted( + f"{method} {path} -> {op} ({OPERATION_REGISTRY[op]})" + for (method, path), op in ROUTE_OPERATIONS.items() + if method == "DELETE" and OPERATION_REGISTRY[op] not in ("destructive", "admin") + ) + assert offenders == [], ( + "A DELETE route classified below `destructive` slips through " + f"--deny-destructive: {offenders}" + ) + + def test_mutating_verbs_are_not_classified_read(self) -> None: + offenders = sorted( + f"{method} {path}" + for (method, path), op in ROUTE_OPERATIONS.items() + if method in ("POST", "PUT", "PATCH") and OPERATION_REGISTRY[op] == "read" + ) + assert set(offenders) <= self._NON_MUTATING_POSTS, ( + "New mutating route classified `read` -- it would survive " + f"--deny-writes: {sorted(set(offenders) - self._NON_MUTATING_POSTS)}" + ) + + def test_get_routes_are_reads(self) -> None: + offenders = sorted( + f"{method} {path}" + for (method, path), op in ROUTE_OPERATIONS.items() + if method == "GET" and OPERATION_REGISTRY[op] != "read" + ) + assert set(offenders) <= self._NON_READ_GETS, ( + "A GET classified above `read` is blocked by --deny-writes; make sure " + f"that is intended: {sorted(set(offenders) - self._NON_READ_GETS)}" + )