fix(serve): route-aware --ui auth (no /doctor /version /changelog bypass) (GHSA-ffpq, 0.60.4)

[padak]

Summary

Fixes M4 from the 2026-06-12 security audit (private advisory GHSA-ffpq-prmh-3gx2) — an auth bypass in kbagent serve --ui mode.

In single-process UI mode the bearer-auth middleware consults app.state.is_ui_public to decide which GETs are public SPA surface. That predicate was a hand-maintained prefix deny-list (_allow_static_through_auth in server/app.py) whose comment literally said "tight allow-list to avoid leaking auth bypass to API routes added in future" — but the implementation was a deny-list, and it had gone stale: it omitted the health-router routes /version, /changelog, and /doctor. So in --ui mode those executed unauthenticated. GET /doctor runs registry.doctor.run_checks() → exposes project aliases, ids, stack URLs, and the config-file path; reachable from the LAN under --host 0.0.0.0 --ui.

Fix

Replace the prefix list with a route-aware predicate. All routers are included before the UI is installed and before the StaticFiles mount is appended, so _allow_static_through_auth snapshots the app's actually-registered route patterns (isinstance(r, Route) — the Mount is correctly excluded). A GET is public only if it's the static shell (/, /index.html, /assets/*, favicons) or matches no registered route (a genuine client-side SPA path → public index.html shell). Any path that resolves to a real endpoint requires auth.

This is drift-proof: every current and future endpoint is protected automatically, with no list to keep in sync — directly removing the failure mode that produced the bug. Only --ui mode is affected; API-only serve never exposed these (it has no is_ui_public predicate).

Tests

New TestUiAuthRouteAwareBypass in test_serve_ui.py: parametrized over /doctor /version /changelog asserting each is 401 without auth in --ui mode (the exact bypass), plus a test that a non-API path stays public (not auth-walled). The pre-existing test_dev_portal_read_requires_auth_in_ui_mode continues to pass. Full suite green: 3978 passed, 132 skipped; lint/format/ty/changelog clean.

(While writing the test I noticed the html=True "SPA fallback for client-side routes" claim in the _install_ui comment is inaccurate — StaticFiles 404s unknown paths rather than serving index.html — but that's pre-existing and out of scope for this auth fix; flagging for a possible follow-up.)

Audit progress

Open advisories after this: M5 (serve-token blast radius), M6 (CORS), M7 (plaintext-on-encrypt warning), M8 (SSRF), M10 (version regex), plus the M1 residual.

---

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

[padak]

Review of #413 — fix(serve): route-aware --ui auth (no /doctor /version /changelog bypass) (GHSA-ffpq, 0.60.4)

Generated by kbagent-pr-reviewer subagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed via make check, not duplicated here.

Summary

This PR fixes auth-bypass advisory GHSA-ffpq-prmh-3gx2 in kbagent serve --ui. The bearer-auth middleware consults an is_ui_public predicate to decide which GETs are public SPA surface; the old implementation was a hand-maintained prefix list that had gone stale and omitted the top-level health-router routes /version, /changelog, /doctor, so they executed unauthenticated in --ui mode (/doctor in particular leaks project aliases/ids/stack URLs/config path, reachable from the LAN under --host 0.0.0.0 --ui). The fix replaces the prefix list with a route-aware predicate that snapshots the app's actually-registered Route patterns after all routers are included and before the StaticFiles Mount is appended: a GET is public only if it is the static shell (/, /index.html, /assets/*, favicons) or matches no registered route. Verdict: APPROVE. The fix is correct, drift-proof, verified end-to-end (bare path and /api/ alias), make check is green, and there are no blocking or non-blocking findings.

Verdict

• Verdict: APPROVE
• Blocking findings: 0
• Non-blocking findings: 0
• Nits: 1

Blocking findings

(none)

Non-blocking findings

(none)

Nits

• [NIT-1] src/keboola_agent_cli/server/app.py:705 — the _install_ui docstring/comment still claims StaticFiles html=True serves index.html for unknown SPA paths ("missing paths fall through to index.html for SPA client-side routing"). Verified empirically that StaticFiles(html=True) returns 404 for unknown paths (e.g. GET /some-client-side-view → 404, not the shell). The PR description already self-flags this as pre-existing and out of scope; recording it here so the follow-up is greppable. No security impact: the route-aware predicate correctly returns public (not 401) for non-endpoint paths, so nothing is auth-walled or leaked.

Verification log

What I actually ran, with exit codes and trimmed output.

• gh pr view 413 --json state,... → OPEN, fix(serve): prefix, +76/-52, 7 files. Conventional prefix matches change type ✓
• Working tree on fix/serve-ui-auth-route-aware, matches <branch> ✓; gh auth status authenticated to keboola/cli ✓
• PR scope correction: local main is stale at 0.60.1 (a73d94a), which is also the merge-base, so git diff main...HEAD shows 17 files. gh pr diff 413 (GitHub's diff against the up-to-date origin/main that already has 0.60.2/0.60.3) shows the true 7-file scope: pyproject/plugin.json/marketplace.json/uv.lock (version bump), changelog.py (one 0.60.4 entry), server/app.py (the fix), test_serve_ui.py (the test). The 10 extra files (agent_runner, sync_service, context.py, gotchas.md, ...) belong to already-merged #411/#412 and were NOT reviewed as part of #413. Branch commits confirm this: bddd275 is the only PR-413 commit; eb1ea15/67acdad are merged #412/#411.
• make check → 3978 passed, 8 skipped, 124 deselected in 97s; lint/format/ty/changelog/version/command-sync clean ✓
• uv run pytest tests/test_serve_ui.py -v → 18 passed (incl. new TestUiAuthRouteAwareBypass parametrized over /doctor /version /changelog + the non-API-path-stays-public test; pre-existing test_dev_portal_read_requires_auth_in_ui_mode and test_query_param_no_longer_accepted still pass) ✓
• Exact advisory reproduction: unauthenticated GET /doctor in --ui mode → 401 UNAUTHORIZED, no doctor diagnostics in body ✓. Also /version, /changelog → 401; with valid token → 200 ✓
• /api/ alias surface: GET /api/doctor, /api/version, /api/changelog (the path-rewrite branch) → 401 ✓ (bypass closed on both surfaces)
• Predicate probe (app.state.is_ui_public): real endpoints /doctor /version /changelog /health/auth-info /projects /dev-portal/apps /agents → False (protected); parametrized /stream/{project}/list, /stream/{project}/detail, /jobs/{project}/{id}/stream, /configs/{project}/detail → False (protected); SPA / /index.html /assets/* /favicon.ico + client routes → True (public) ✓
• /stream note: bare GET /stream returns public (True), but verified there is no bare /stream route — the stream router uses /stream/{project}/list|detail|..., all correctly protected. The PR description's incidental mention of /stream among bypassed routes is imprecise (no bare endpoint existed), not a gap.
• Regex anchoring: path_regex.pattern for /doctor is ^/doctor$; /doctorEVIL and /doctor/sub correctly do NOT match (no partial-match leak) ✓
• 3-layer / convention greps on the PR diff: no typer/click/httpx in wrong layer, no magic numbers, no raw error_code="...", no bare except:, no print() in prod, no token/secret in added non-test lines ✓
• Version consistency: pyproject 0.60.4 = plugin.json 0.60.4 = marketplace.json 0.60.4 = uv.lock 0.60.4; make changelog-check → "All 48 stable releases have changelog entries" ✓

Open questions for the author

(none)