refactor(api): typed allowlist + enumeration test against route drift#4162
Conversation
houko
left a comment
houko
left a comment
There was a problem hiding this comment.
Two issues found on review — no mechanical auto-fixes applied since both require author judgement.
-
Security regression (
middleware.rs:496):prefix_get("/api/mcp/servers/")inPUBLIC_ROUTES_GET_ONLYmakes every GET request under/api/mcp/servers/*unconditionally public. Theis_mcp_oauth_callbackguard intended to narrow this is dead code becausealways_public_get_onlyfires first. Old behaviour only exposed the specific…/auth/callbacksuffix. -
Silent auth-semantics change (
tests/auth_public_allowlist.rs:231):/api/configmoved fromDashboardRead(public by default) toAuthed. The PR description says no semantics changed — that's incorrect and should be updated regardless of which classification is ultimately correct.
Everything else looks clean: the typed-constant approach is solid, the alphabetical ordering is consistent, the three catalog-verification tests are a genuine improvement over ad-hoc matches!() drift.
Generated by Claude Code
| // i18n locale bundles — static, fetched before auth flow | ||
| PublicRoute::prefix_get("/locales/"), | ||
| // MCP OAuth callback: /api/mcp/servers/{name}/auth/callback | ||
| PublicRoute::prefix_get("/api/mcp/servers/"), |
There was a problem hiding this comment.
Security regression: broad prefix makes all GET /api/mcp/servers/* paths unconditionally public.
matches_route returns true for any GET request whose path starts with /api/mcp/servers/. That value feeds directly into always_public_get_only, which is ORed into always_public before is_mcp_oauth_callback is consulted:
let always_public =
always_public_method_free || always_public_get_only || is_mcp_oauth_callback;Because always_public_get_only is already true for the whole prefix, is_mcp_oauth_callback is dead code — it can never flip always_public from false to true for any /api/mcp/servers/* path. The doc-comment on the constant says "The auth middleware applies that extra guard", but it does not — the prefix match wins first.
Old behaviour: only is_get && starts_with("/api/mcp/servers/") && ends_with("/auth/callback") was public.
New behaviour: every GET under /api/mcp/servers/ is public (e.g. GET /api/mcp/servers/prod/tools, GET /api/mcp/servers/prod/config, any future route).
Fix: remove prefix_get("/api/mcp/servers/") from PUBLIC_ROUTES_GET_ONLY and let is_mcp_oauth_callback remain the sole gate for that path family. If you want the test helper to enumerate MCP OAuth paths, add a dedicated PublicRoute variant that carries the suffix requirement, or simply document is_mcp_oauth_callback separately in the test.
Generated by Claude Code
| re("/api/workflows", Expect::DashboardRead), | ||
| // Auth-required endpoints (must 401 without Bearer token) | ||
| re("/api/health/detail", Expect::Authed), | ||
| re("/api/config", Expect::Authed), |
There was a problem hiding this comment.
Auth-semantic change contradicts the PR description.
The PR's "What is NOT changed" section states "No route's auth semantics changed", but /api/config was in dashboard_read_exact in the old middleware (public when require_auth_for_reads is off, which is the default). Classifying it as Authed here means any default-config deployment that relied on the dashboard loading /api/config without a token will start seeing 401s after this lands.
You noted the uncertainty yourself in "Routes I'm uncertain about". The classification here makes the right call if /api/config really does return sensitive fields — but please:
- Update the PR description to accurately reflect this semantic change.
- Confirm the intent: if
/api/configshould be fully auth-required even in no-require_auth_for_readsmode, keepAuthedand note it explicitly. If it should follow the same pattern as other read endpoints, add it back toPUBLIC_ROUTES_DASHBOARD_READS.
Generated by Claude Code
…e drift Replace the inline string arrays in middleware::auth() with three typed pub const slices (PUBLIC_ROUTES_ALWAYS, PUBLIC_ROUTES_GET_ONLY, PUBLIC_ROUTES_DASHBOARD_READS) and a PublicRoute / PublicMethod / PublicMatch set of types. is_public() is rewritten to walk these consts via a shared matches_route() helper. Auth semantics are unchanged for every route; this is a pure refactor of the allow-list representation. Add crates/librefang-api/tests/auth_public_allowlist.rs with four tests: - always_public_paths_are_in_catalog: catalog consistency check (no HTTP) - dashboard_read_paths_are_in_catalog: same for dashboard-reads group - authed_paths_are_not_in_any_public_catalog: guards against accidental promotion of auth-required routes into the public lists - always_public_routes_are_reachable_without_token: HTTP check via tower::ServiceExt::oneshot - authed_routes_require_token: HTTP check — Authed paths must 401 - dashboard_read_routes_reachable_without_auth_when_no_key_configured: HTTP check — DashboardRead paths pass through in no-auth dev mode Routes I'm uncertain about: - /api/config (GET): currently Authed in test table (not in any public list in middleware), but /api/config/schema is AlwaysPublic. If the full config endpoint should be dashboard-readable, add it to PUBLIC_ROUTES_DASHBOARD_READS and the test table. Fixes #3712
CRITICAL #1: Remove `prefix_get("/api/mcp/servers/")` from PUBLIC_ROUTES_GET_ONLY. The prefix was added to support the OAuth callback but its broadness exposed GET /api/mcp/servers/{name} (returns server config including env vars) and /auth/status (returns McpAuthState carrying OAuthTokens) without authentication. The OAuth callback path /api/mcp/servers/*/auth/callback remains public via the dedicated is_mcp_oauth_callback guard (prefix + suffix check) that was already present in auth() — no functional change to the callback path. CRITICAL #2: Add `exact_get("/api/config")` back to PUBLIC_ROUTES_DASHBOARD_READS. The old allowlist had it in dashboard_read_exact; it was dropped when the allowlist was refactored. The handler already redacts secrets, so public read was intentional. The dashboard SPA calls GET /api/config via getFullConfig() on every render. HIGH: Rewrite auth_public_allowlist.rs file header to accurately describe the test as a static documentation test with a hardcoded hand-maintained table, not a live router drift gate. The previous comment claimed adding a server.rs route without updating the table would fail CI — that was false. MEDIUM: Update PUBLIC_ROUTES_ALWAYS doc comment to accurately describe its grouping-by-semantic-role ordering rather than claiming all three slices are sorted alphabetically. Also: add regression test mcp_servers_prefix_does_not_leak_protected_paths that asserts /api/mcp/servers/{name} and /auth/status return 401 while /auth/callback stays non-401. Update is_in_always_public() helper to mirror the is_mcp_oauth_callback guard so catalog-consistency checks still pass for callback paths. Remove stale comments referencing the removed prefix entry.
8b00b49 to
1d7369d
Compare
1 participant
Summary
Typed public-route allowlist replacing the stringly-typed
matchblock inmiddleware.rs, plus a catalog consistency test.Follow-up commit (8b00b49) addresses reviewer-found security regressions:
prefix_get(\"/api/mcp/servers/\")fromPUBLIC_ROUTES_GET_ONLY. The broad prefix was exposingGET /api/mcp/servers/{name}(server config including env vars) andGET /api/mcp/servers/{name}/auth/status(McpAuthState carrying OAuthTokens) without authentication. The MCP OAuth callback (/api/mcp/servers/*/auth/callback) remains public via the existingis_mcp_oauth_callbackguard inauth()which correctly enforces both the prefix and the/auth/callbacksuffix.exact_get(\"/api/config\")toPUBLIC_ROUTES_DASHBOARD_READS. The old allowlist had it indashboard_read_exact; it was dropped during the refactor. The handler already redacts secrets and the dashboard SPA callsGET /api/config(viagetFullConfig()) on every render.auth_public_allowlist.rsfile header to accurately describe the test as a static documentation test with a hand-maintained table — not a live router drift gate. The previous comment falsely claimed that adding a route toserver.rswithout updating the table would fail CI. Proper router-enumeration drift gating is tracked as follow-up work per Auth public-allowlist hardcoded in middleware.rs while routes live in server.rs — drift hazard #3712 acceptance criteria.PUBLIC_ROUTES_ALWAYSdoc comment to accurately describe its grouping-by-semantic-role ordering (not alphabetical).mcp_servers_prefix_does_not_leak_protected_pathsthat asserts/{name}and/auth/statusreturn 401 while/auth/callbackstays non-401.Test plan
cargo test -p librefang-api --test auth_public_allowlist— all existing + new regression test passGET /api/mcp/servers/{name}returns 401GET /api/mcp/servers/{name}/auth/statusreturns 401GET /api/mcp/servers/{name}/auth/callbackreturns non-401 (OAuth redirect still works)GET /api/configreturns non-401 whenrequire_auth_for_readsis off (dashboard renders)