serve: enforce the permission firewall on every REST route (#655) - #682
Draft
padak wants to merge 1 commit into
Draft
serve: enforce the permission firewall on every REST route (#655)#682padak wants to merge 1 commit into
padak wants to merge 1 commit into
Conversation
`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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #655.
The gap
PermissionEnginewas built only in the Typer callback (cli.py), so a persistedpermissions set --mode denypolicy — and both--deny-writesand--deny-destructive— protected the CLI process and nothing else.kbagent serveexposed all 236 routes,DELETE /storage/bucketsandPOST /token/{p}/deleteincluded, behind a single all-or-nothing bearer token.#677 built the enforcement machinery — an engine on
app.state, aPermissionDeniedError→ HTTP 403 handler, and therequire_permissiondependency — and wired it to three/auth/*routes. This PR adds the missing half: coverage.How
One app-level dependency (
FastAPI(dependencies=[...]), so no router can be added outside it) runs on every request, looks the matched route up by(method, path template)in a central table, and calls the samePermissionEngine.check_or_raisethe CLI uses. FastAPI puts the matched route intorequest.scope["route"]before dependencies resolve, so the template —/storage/buckets/{project}, not the concrete URL — is what the policy is keyed on. Verified at runtime, not assumed.Why one table instead of 226 per-route decorators:
--deny-destructive?" is answered by reading one file, not thirty routers.The per-route
require_permission(...)form still wins where it is declared: such a route is skipped by the table lookup (recognised via aPERMISSION_DEPENDENCY_MARKERattribute). That is what keeps #677's three/auth/*routes — and the test-only probe routes intests/test_server_permissions.py, registered aftercreate_app— working unchanged.With no policy configured, nothing changes.
is_allowedreturns True for everything, so the default install behaves exactly as before.Also in scope
GET /permissions/show(the LOW ask in serve: missing mirrors — describe-batch (inline payload), unload-table; job run lacks idempotency_key #657, and serve: PermissionEngine firewall is not enforced on REST routes #655's option-2 discoverability point). Reports the effective policy — the persisted block already merged with the--deny-*flags the daemon was launched with, which a REST caller can neither see nor change. Reachable under any policy (the same anti-lockout rule the CLI has), still requires the bearer token. Deliberately no write counterpart: letting a bearer token widen the policy that constrains it would make the firewall self-defeating, sopermissions set/resetstay terminal actions on the host.ai.chatandworkspace.sql-improve, bothwrite: neither touches Keboola, but both spawn a localclaude/codex/geminiprocess on the host, exactly like the already-writeagent.prompt-improve. Added toSERVE_ONLY_OPERATIONSso the command-sync gate does not read them as dead keys.permissions.py'shttp.*block claimed "The serve's own routes enforce their own permissions on top." It is true now, and the comment says precisely since when.Classification notes
DELETEroute maps to adestructive- oradmin-class operation; a test asserts it.readoperations (/flows/validate,/lineage/show,/kai/ask, …) — POST because the request needs a body, not because it mutates. Explicit allowlist in the test, so a new mutating route classifiedreadfails.GET /branches/{project}/merge-urlmaps to thewrite-classbranch.merge. That is CLI parity, not a slip:kbagent branch mergeonly ever produces a URL too.POST /semantic-layer/items/{kind}coversmetric/dataset/… in one route, so it maps to the collapsed parent keysemantic-layer.add, notsemantic-layer.add.metric. A policy naming only a leaf key is enforced on the CLI but not over REST — name the parent or acli:*category to cover both./health/ping,/health/auth-info,/ui-config,/docs,/redoc,/openapi.json,/docs/oauth2-redirect, and the SPA shell/+/index.html). A locked-down server must still be able to say who it is, or a client cannot tell a policy refusal from a dead process.Testing
make checkgreen: 6148 passed, 12 skipped.tyclean (the one remaining diagnostic is the pre-existingscripts/hatch_build.pyunresolved import). Lint, format, command-sync, version-gates, sentinel-guards, error-codes and the endpoint-reference gate all pass;docs/web-server-endpoints.mdregenerated and committed.31 new tests in
tests/test_server_route_permissions.py(new file, so no conflict with #681's edits totest_server_permissions.py):OPERATION_REGISTRYkey. This is what keeps the runtime fail-closed branch unreachable in a released build.cli:destructivedeniesDELETE /storage/buckets/{project}whileGET /projectsstays 200;--deny-writesdeniesPOST /jobs/{p}/run; an exacttoken.deletepattern works;mode=denyblocks an unlisted read but still serves/health/*.ROUTE_OPERATIONS./permissions/show— clean, persisted, effective-with-flags, inert patterns, reachable under total deny, still 401 without the token.Mutation-checked: commenting out the app-level dependency fails exactly 5 of them, so they are load-bearing rather than decorative.
A trap worth flagging for review
FastAPI 0.137 stopped flattening
include_routereagerly —app.routesholds 35 lazy_IncludedRouterproxies instead of the 236 routes they stand for, and nothing materialises them (notapp.openapi(), notTestClientstartup). Request handling is unaffected, but a completeness test that walkedapp.routesnaively would audit four routes, find nothing wrong, and pass._iter_api_routesrecurses throughoriginal_router; the docstring says why, because this is exactly the false-pass shape a coverage test must not have.Merge-order notes
src/keboola_agent_cli/permissions.py. Both PRs rewrite the same three-linehttp.*comment (it fixes the false claim; this makes the claim true). Expect a small conflict there — resolve in favour of whichever lands second, keeping this PR's wording if that is this one.ROUTE_OPERATIONSentries. The completeness test names each missing route, so the failure is self-explaining rather than mysterious.No version bump, no changelog entry (per CONTRIBUTING: those belong to the release PR). New behaviour is gated with the literal
(since vNEXT)placeholder on every doc surface;context.pycarries no version tag, matching the precedent set in #681.Doc surfaces updated (convention #17)
docs/web-server.md(new "The session firewall applies to every route" section, plus two stale paragraphs that asserted the gap),docs/web-server-endpoints.md(regenerated),CLAUDE.md,plugins/kbagent/skills/kbagent/references/gotchas.md(the "A deny policy does NOT firewall the whole REST surface" entry was live and is now inverted, with the 0.90.1-and-older behaviour kept for readers on those versions),commands-reference.md, andcommands/context.py'sAGENT_CONTEXT. No CLI command added, renamed, or removed, sokeboola-expert.mdandSKILL.mdneed no change.