fix(serve): --ui auth survives fastapi 0.137 (router match, fail closed); lift cap - #444
Conversation
f29da23 to
3f5b957
Compare
The --ui auth predicate scanned app.routes as a flat list to decide which paths need auth. fastapi 0.137 nests included routers into a lazy tree (_IncludedRouter) instead of flattening, so the flat scan missed nested endpoints and served /doctor, /version, /changelog, /agents unauthenticated (GHSA-ffpq-prmh-3gx2). Ask the router's match protocol whether a GET resolves to a real endpoint instead -- the same resolution a real request uses, so it cannot miss a live endpoint. Fails closed: any error -> path treated as protected, never silently public.
3f5b957 to
4c38b16
Compare
serve --ui auth works under fastapi 0.137 (prior commit), so the temporary <0.137 cap is removed; uv.lock -> fastapi 0.137.2. Stays on the unreleased 0.63.4 instead of bumping -- the cap and its removal ship in one release; changelog note folded into the 0.63.4 entry.
4c38b16 to
a77b00f
Compare
padak
left a comment
There was a problem hiding this comment.
Review of #444 — fix(serve): --ui auth survives fastapi 0.137 (router match, fail closed); lift cap
Generated by
kbagent-pr-reviewersubagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed viamake check, not duplicated here.
Summary
This PR fixes a security regression introduced by fastapi 0.137: the --ui mode
auth predicate in server/app.py previously scanned app.routes for Route
instances to decide which paths require auth. fastapi 0.137 changed include_router
to store routers as lazy _IncludedRouter objects rather than flattening them,
causing the flat scan to find only 4 routes (the OpenAPI UI routes) instead of all
32 registered routes -- so /doctor, /version, /changelog, /agents, and every
other router-grouped endpoint was served unauthenticated in --ui mode. The fix
replaces the flat scan with Starlette's native route.matches() dispatch protocol,
which resolves correctly against the live router tree. The fix also fails closed
(any exception during matching treats the path as protected). The fastapi<0.137 cap
added in #443 is lifted. The verified behavior is correct: all 18 existing
test_serve_ui.py tests pass, make check is clean (4137 passed, 8 skipped),
and a live probe against fastapi 0.137.2 confirms the old predicate returned
is_public=True for all API paths while the new predicate returns is_public=False.
Verdict: APPROVE. No blocking findings. The implementation is correct, test coverage
is adequate for the changed behavior, and the change is appropriately scoped to
the server layer.
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 2
- Nits: 1
Blocking findings
(none)
Non-blocking findings
[NB-1] src/keboola_agent_cli/server/app.py:750 — _is_ui_public iterates all routes on every request; no test for fail-closed exception path
The new predicate loops over app.router.routes (32 entries in the current
router configuration) and calls .matches() on each at every request that
reaches the is_ui_public check (i.e. all non-PUBLIC_PATHS, non-/docs
GET requests in --ui mode). Measured at ~220 µs per call for worst-case SPA
paths that exhaust all routes. At current scale this is negligible, but
caching the result in a frozenset built once (the same pattern as
api_route_patterns in the old code) would eliminate the per-request scan.
Additionally, the except Exception: return False fail-closed path is
not covered by a test -- a unit test that patches one route's .matches()
to raise and asserts the path is treated as protected would pin the
fail-closed contract.
Fix (optional): build the frozenset of matched paths once at install time via a
one-time scan, or add a unit test for the exception path.
[NB-2] tests/test_serve_ui.py — TestUiAuthRouteAwareBypass does not include /agents in the parametrize list
The PR description names /agents as one of the four endpoints that were
served unauthenticated under the old flat-scan predicate. The
TestUiAuthRouteAwareBypass class parametrizes only ["/doctor", "/version", "/changelog"]. The /agents endpoint IS covered by
TestCookieAuth::test_cookie_authenticates_after_bootstrap (asserts
client.get("/agents").status_code == 401 before cookie), but the intent
of TestUiAuthRouteAwareBypass is to be the canonical regression test for
the GHSA-ffpq-prmh-3gx2 bypass -- adding "/agents" to the parametrize list
would make the coverage explicit rather than implicit.
Fix: add "/agents" to the @pytest.mark.parametrize("path", [...]) decorator
in TestUiAuthRouteAwareBypass::test_health_router_extras_require_auth_in_ui_mode.
Nits
[NIT-1]tests/test_serve_ui.py:1— fastapi 0.137 introduces a new
StarletteDeprecationWarning: Using httpx with starlette.testclient is deprecated; install httpx2 instead(1 warning emitted during the test run).
This is a framework-level signal that the test client dependency is
transitioning; not introduced by this PR but newly visible because the cap is
lifted. Tracking it in a follow-up issue or pinning a filterwarning in
conftest.py(pytest.mark.filterwarnings("ignore::DeprecationWarning", "starlette")) would keep the warning noise floor clean.
Verification log
gh pr view 444 --json title,body,files,state→ 4 files, +37/-29, state
OPEN,fix(serve):conventional prefix ✓git rev-parse --abbrev-ref HEADon worktree →fix/serve-ui-auth-fastapi-0137✓- Layer violation grep (typer/click in services, httpx in commands, formatter in
clients) → empty ✓ (no layer violations; change is server layer only) - New
@app.command(...)decorators incommands/**→ none (no CLI command added) - Plugin synchronization map check → no new CLI command surface; no documentation
drift surfaces apply (no new command, no version bump in this PR -- version stays
0.63.4 and rides the unreleased changelog entry from #443) make check(first run): FAILED --ModuleNotFoundError: No module named 'fastapi'
(worktree venv missing server extras). This is a local env setup issue, not a
PR defect.uv sync --extra server→ installed fastapi 0.137.2 ✓make check(second run): 4137 passed, 8 skipped, 125 deselected, 17 warnings
in 83.57s ✓uv run pytest tests/test_serve_ui.py -v→ 18 passed, 1 warning in 2.33s ✓- Live probe: old flat-scan predicate under fastapi 0.137.2 returns
is_public=True
for/doctor,/version,/changelog,/agents,/dev-portal/apps(4 Route
objects only from OpenAPI/docs routes, 0 of 25_IncludedRouterobjects matched).
Newroute.matches()predicate returnsis_public=Falsefor all five paths
(_IncludedRouter.matches()resolvesMatch.FULL). ✓ app.router.routesroute type count under 0.137.2:{'Route': 4, '_IncludedRouter': 25}
-- confirms the flat-scan regression and the fix scope ✓name="ui"StaticFiles mount verified atapp.py:715-- matches thegetattr(route, "name", None) == "ui"skip guard ✓- Conventional commit:
fix(serve):✓ (bug fix in theservecomponent, not a new feature) - No magic numbers, no bare
except:, no raw error_code strings, noprint()
in production code, no token in logged output ✓ - Security: no new httpx calls outside client layer, no token surfacing without
mask_token()✓ - Performance measured: ~220 µs/call worst-case (SPA path, exhausts all 32 routes);
~132 µs/call for API path (early exit on first_IncludedRoutermatch). Negligible
for the target use case (localhost dev tool), noted as NB-1 for awareness.
Open questions for the author
(none)
padak
left a comment
There was a problem hiding this comment.
Approved. ✅
The route-aware fail-closed fix is correct — a live probe confirmed the old flat route scan returned is_public=True for every API endpoint under fastapi 0.137 (only 4 Route objects from OpenAPI/docs were visible; 25 real endpoints lived inside _IncludedRouter objects the scan never traversed), while the new route.matches() approach restores the intended deny-by-default behavior. make check passes (4137 tests) after uv sync --extra server.
The two non-blocking findings are not merge blockers — leaving it to your judgment whether to fold them into this PR or defer to a follow-up:
- NB-1:
_is_ui_publiciterates all 32 routes per request (~220 µs worst case) and there's no unit test pinning the fail-closed exception path. Caching the match result at install time + a test for the exception branch would lock the contract. - NB-2:
/agentsis missing from theTestUiAuthRouteAwareBypassparametrize list (covered implicitly elsewhere, but explicit would be cleaner).
Your call on whether these are worth addressing now.
Why
Follow-up to #443, which capped
fastapi<0.137because 0.137 reopened GHSA-ffpq-prmh-3gx2. Theserve --uiauth predicate scannedapp.routesas a flat list; fastapi 0.137 (fastapi/fastapi#15745) madeinclude_routernest routers into a lazy tree (_IncludedRouter) instead of flattening them, so the flat scan missed nested endpoints and served/doctor,/version,/changelog,/agentsunauthenticated. The nesting is deliberate on fastapi's side, so the fix belongs on ours.What changed
server/app.py: the--uiauth predicate asks the router's match protocol whether a GET resolves to a real endpoint (skipping the SPA StaticFiles catch-all), instead of scanningapp.routes.matches()is the same resolution a real request uses, so it can't miss a live endpoint however fastapi structures routes. Also fails closed.pyproject.toml: lift thefastapi<0.137cap (latest 0.137.2 in the lock).Verification
make checkgreen on fastapi 0.137.2. Thetest_serve_ui.pyroute-aware tests assert/doctor,/version,/changelogand/dev-portal/*require auth in--uimode while genuine SPA paths stay public; confirmed they fail on the old flat-scan predicate under 0.137 (those endpoints served unauthenticated) and pass on this fix.Follow-up
Two non-blocking nits from #443's review are deferred to a separate PR: drop the obsolete
typer[all]extra inpyproject.toml, and use the group's owncontext_classinstead of standaloneclick.Contextinrepl.py.