feat(api): add require_auth_for_reads to lock down dashboard reads#2398
Merged
Conversation
The dashboard public-read allowlist in the auth middleware hard-codes /api/agents, /api/config, /api/budget, /api/sessions, /api/approvals, /api/hands, /api/skills, /api/workflows and more as GET-public so the SPA can render before the user enters credentials. With the default api_listen = "0.0.0.0:4545", any host on the LAN can enumerate agents, read configuration (minus redacted api_key), observe spend, and list pending approvals without a token — a hard mismatch with the "Bearer token authentication" line in SECURITY.md. Introduce KernelConfig.require_auth_for_reads (default false, so existing deployments keep rendering unchanged). When it is true AND api_key is configured, the middleware collapses the allowlist to the static-asset / OAuth / health subset and forces every dashboard read through bearer authentication. Unauthenticated health probes, OAuth callback, and dashboard shell HTML remain reachable. Refactor the public-path match into matches!()-based groups so clippy (nonminimal_bool) stays quiet and the two tiers are obviously separated in code review. Regression coverage: - require_auth_for_reads=true blocks unauthenticated GET /api/agents - require_auth_for_reads=true still allows the correct bearer - /api/health stays public with the flag on - require_auth_for_reads=false preserves the legacy public GET
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd9f12d86f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| | "/api/versions" | ||
| | "/api/health" | ||
| | "/api/health/detail" | ||
| | "/api/status" |
There was a problem hiding this comment.
With require_auth_for_reads = true and a non-empty api_key, GET /api/status still bypasses auth because it is listed under always_public_method_free, so the new hardening flag does not actually stop unauthenticated metadata enumeration on this route. The status handler returns sensitive operational details (including agent list, api_listen, and home_dir in routes/config.rs), so a remote caller can still retrieve internal state even after enabling the flag.
Useful? React with 👍 / 👎.
Three real issues found while reviewing the original commit on this branch: 1. **`/api/status` still reachable.** The status handler at routes/config.rs:48 returns the full agents listing (id, name, state, model provider, profile) plus `home_dir`, `api_listen`, session count and memory usage — exactly the enumeration surface `require_auth_for_reads` exists to close. The original commit kept it in `always_public_method_free`, so flipping the flag did not actually stop `/api/status` from leaking. Moved it into `dashboard_read_exact` (still GET-only) so the flag locks it down. 2. **Flag silently no-ops when only user_api_keys or dashboard credentials are configured.** The gate was `require_auth_for_reads && api_key_present` where `api_key_present = !api_key.trim().is_empty()`. Production deployments commonly configure per-user keys or dashboard user/pass without a standalone api_key — in those cases, flipping the flag did nothing. Replaced with the full "any auth configured" predicate: `api_key || !user_api_keys.is_empty() || dashboard_auth_enabled`, which mirrors the existing auth-bypass check a few lines below. 3. **No operator feedback when flag is on but no auth exists.** If `require_auth_for_reads = true` is set without any auth configured, the flag is (correctly) a no-op at the middleware layer — but the operator gets no signal. Added a startup `warn!` in `build_router` so the misconfig is visible in logs at boot. Tests added: - `test_require_auth_for_reads_blocks_api_status` — locks in the fix for #1. - `test_require_auth_for_reads_engages_with_user_api_keys_only` — locks in the fix for #2 (both the 401 on missing creds and the 200 on a valid per-user key). - `test_require_auth_for_reads_is_noop_without_any_auth` — pins the middleware contract for #3 (the warning handles UX, middleware stays permissive when no auth backends exist). Verified: `cargo test -p librefang-api --lib middleware` — 20 passed (7 new for this feature) and `cargo clippy -p librefang-api -p librefang-types --all-targets -- -D warnings` clean.
Follow-up finding from self-review. `/api/health/detail`'s own handler doc comment at routes/config.rs:317 says "requires auth", but the middleware allowlist had it in the always-public set. The handler returns operational data that should not be reachable from a cold probe: - `panic_count` / `restart_count` from the supervisor - `agent_count` - `embedding_provider` / `embedding_model` / `extraction_model` (infra leak) - `config_warnings` — the full output of `KernelConfig::validate()`, which can tell a remote attacker exactly what's wrong with the deployment - event-bus `dropped_events` count Moved to `dashboard_read_exact` so it gets locked down when the flag is on. `/api/health` stays public because its payload is genuinely minimal (`status`, `version`, two-item `checks` array) and load balancers / orchestrators need it for probing. Test added: `test_require_auth_for_reads_blocks_api_health_detail` — pins both contracts (/api/health stays public, /api/health/detail becomes auth-required) in a single test. Note: this is a partial fix. With the flag OFF, `/api/health/detail` stays public to preserve backwards compatibility, which means the handler's own "requires auth" doc comment is still being violated in the default configuration. Making it always-require-auth is a separate behavioural change that belongs in its own PR. Tests: `cargo test -p librefang-api --lib middleware` — 21 passed (8 for require_auth_for_reads, up from 7).
Two more findings from self-review of the always-public set: 1. **`/api/auth/dashboard-check` echoed the configured dashboard username to anonymous callers.** The SPA uses this endpoint before the user has logged in (to pick the right login form), so the route is legitimately unauthenticated — but returning `"username": "<admin>"` handed an anonymous remote caller one half of the credential pair, enabling targeted credential stuffing against `/auth/dashboard-login`. The `mode` field is sufficient for the SPA to pick the right login form; the user already knows their own username. Now always returns an empty string. 2. **`/api/version` echoed the machine hostname.** Version endpoints conventionally expose build info, but the hostname is a per-machine identifier that lets a remote probe correlate a daemon to a specific deployment target. Dropped from the payload. Operators who need the hostname should read it from the daemon's shell environment. No tests referenced either field, `cargo test -p librefang-api --lib` passes with 262 tests (no behaviour change for the dashboard SPA beyond needing the user to type their username at login time, which is how every other dashboard already works). Residual pre-existing gap noted for follow-up: `/api/health/detail`'s own doc comment says it "requires auth" but the middleware kept it public until this PR's flag was added. With the flag off it's still public, honouring backwards compatibility for existing operator probes. Making it unconditionally auth-required is a separate behavioural change.
houko
added a commit
that referenced
this pull request
Apr 14, 2026
…ed callers
SECURITY.md promises "Health redaction: Public endpoint returns
minimal info; full diagnostics require auth", but the `/api/status`
and `/api/health/detail` handlers returned everything unconditionally,
and the auth middleware pinned them as always-public. With the default
`api_listen = "0.0.0.0:4545"` an unauthenticated LAN caller could read:
* every agent's UUID, name, running state, provider, and model
(`/api/status` embeds the full `agents` array)
* `api_listen`, `home_dir`, `log_level`, `network_enabled`
* process RSS, uptime, session/active counts
* panic_count, restart_count, memory/embedding internals
(`/api/health/detail`)
This is an information-disclosure gap on its own, and it hands an
attacker a full target inventory before they've even picked a CVE.
Redact both handlers in place rather than changing the auth middleware
allowlist (which is already under review in PR #2398). They now check
the caller's `Authorization: Bearer …` header against the live
`api_key_lock` using a constant-time compare; unauthenticated callers
get the same minimal shape as `/api/health` so probes, load balancers,
and orchestrators keep working. If `api_key` is empty (local dev
mode) the previous full payload is preserved so `librefang doctor`
and localhost dashboards are unaffected.
3 tasks
The handler at routes/config.rs:317 documents itself as "Full health diagnostics (requires auth)" and `/api/health`'s doc explicitly says "Use GET /api/health/detail for full diagnostics (requires auth)". The middleware was the only thing still treating it as public — a pre-existing mismatch that this PR's first pass only partially closed by flag-gating. Remove it from both `always_public` and `dashboard_read_exact`. With the endpoint in neither public list, the middleware's default auth-required path handles it, so `/api/health/detail` now requires a bearer token regardless of `require_auth_for_reads`. The handler contract and the middleware contract finally agree. Breaking change for operators: if anyone was probing `/api/health/detail` without auth, they'll start getting 401. `/api/health` (minimal liveness) stays public for load balancers and orchestrators, so the standard deployment probe path is unaffected. Monitoring setups that want the detailed view should configure a bearer token — that was the original design intent. Test replaced: `test_api_health_detail_always_requires_auth` now pins both directions — `/api/health` stays public with the flag off, and `/api/health/detail` is 401 regardless of whether the flag is on or off. Tests: `cargo test -p librefang-api --lib middleware` — 21 passed.
…for-reads # Conflicts: # crates/librefang-api/src/middleware.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3b8bf6fbe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+381
to
+382
| let dashboard_read_public = | ||
| (is_get && (dashboard_read_exact || dashboard_read_prefix)) || path == "/api/logs/stream"; // SSE stream, read-only |
There was a problem hiding this comment.
/api/status
This change makes the legacy dashboard-read allowlist GET-only via is_get, so with api_key configured and require_auth_for_reads = false, HEAD /api/status now falls through to auth and returns 401. Before this commit /api/status was method-agnostic in the public set, and axum serves HEAD for GET handlers, so existing health/monitor probes that use HEAD against /api/status will regress even though the new flag is off.
Useful? React with 👍 / 👎.
houko
added a commit
that referenced
this pull request
Apr 14, 2026
#2398 added `require_auth_for_reads` as an opt-in flag that collapses the dashboard public-read allowlist when set to `true`. In practice that means an operator who set `api_key = "…"` but forgot the separate `require_auth_for_reads = true` was still leaking agent IDs, config, budget, sessions, and pending approvals to any host on the LAN — because the default was `false` and the flag name wasn't obvious. The dashboard SPA already handles 401 via its existing login flow (`setOnUnauthorized` / `getStoredApiKey` / bearer injection), so the safe default is "close the allowlist whenever auth is configured", not "closed only when two separate knobs agree". Switch `KernelConfig.require_auth_for_reads` from `bool` to `Option<bool>`: - `None` (default) → derive from whether any authentication is configured: non-empty `api_key`, per-user API keys, or dashboard credentials. Setting an `api_key` now auto-collapses the reads allowlist, matching operator expectations. - `Some(true)` → force closed even if auth looks misconfigured. Useful for catching an accidental `api_key = ""` redeploy. - `Some(false)` → explicit escape hatch for deployments fronted by an external auth proxy that want the in-tree dashboard to keep rendering before the reverse proxy attaches its own credentials. When the auto-enable path kicks in, log a one-line `info!` so operators see that reads are now locked. When the operator forces `require_auth_for_reads = true` but has no auth configured at all, keep the existing `warn!` so a misconfiguration still surfaces. All existing public, OAuth, and health endpoints stay reachable regardless of the resolved value — that list is hard-coded in `middleware.rs::is_always_public` and unchanged. Dashboard impact: none. The SPA in `crates/librefang-api/dashboard` already injects `Authorization: Bearer` from `localStorage` on every request and intercepts 401 to trigger the login screen, so flipping the default on for `api_key`-having deployments does not break rendering — an unauthenticated visitor lands on the login form, types their key, and resumes. Migration: operators who explicitly *want* open reads with an `api_key` set (fronted by reverse-proxy auth, say) must now add `require_auth_for_reads = false` to `config.toml`. Previously they didn't need any flag at all.
4 tasks
houko
added a commit
that referenced
this pull request
Apr 14, 2026
* fix(api): derive require_auth_for_reads from api_key when unset #2398 added `require_auth_for_reads` as an opt-in flag that collapses the dashboard public-read allowlist when set to `true`. In practice that means an operator who set `api_key = "…"` but forgot the separate `require_auth_for_reads = true` was still leaking agent IDs, config, budget, sessions, and pending approvals to any host on the LAN — because the default was `false` and the flag name wasn't obvious. The dashboard SPA already handles 401 via its existing login flow (`setOnUnauthorized` / `getStoredApiKey` / bearer injection), so the safe default is "close the allowlist whenever auth is configured", not "closed only when two separate knobs agree". Switch `KernelConfig.require_auth_for_reads` from `bool` to `Option<bool>`: - `None` (default) → derive from whether any authentication is configured: non-empty `api_key`, per-user API keys, or dashboard credentials. Setting an `api_key` now auto-collapses the reads allowlist, matching operator expectations. - `Some(true)` → force closed even if auth looks misconfigured. Useful for catching an accidental `api_key = ""` redeploy. - `Some(false)` → explicit escape hatch for deployments fronted by an external auth proxy that want the in-tree dashboard to keep rendering before the reverse proxy attaches its own credentials. When the auto-enable path kicks in, log a one-line `info!` so operators see that reads are now locked. When the operator forces `require_auth_for_reads = true` but has no auth configured at all, keep the existing `warn!` so a misconfiguration still surfaces. All existing public, OAuth, and health endpoints stay reachable regardless of the resolved value — that list is hard-coded in `middleware.rs::is_always_public` and unchanged. Dashboard impact: none. The SPA in `crates/librefang-api/dashboard` already injects `Authorization: Bearer` from `localStorage` on every request and intercepts 401 to trigger the login screen, so flipping the default on for `api_key`-having deployments does not break rendering — an unauthenticated visitor lands on the login form, types their key, and resumes. Migration: operators who explicitly *want* open reads with an `api_key` set (fronted by reverse-proxy auth, say) must now add `require_auth_for_reads = false` to `config.toml`. Previously they didn't need any flag at all. * fix(api): align require_auth_for_reads behavior with documentation The original doc claimed Some(true) would force the reads allowlist closed even when no auth was configured, but the middleware deliberately short- circuits in that case (middleware.rs:423-426) and falls through to the unauthenticated local-dev bypass at lines 438-443. That bypass is intentional — it keeps the flag from silently no-oping when operators use per-user keys or dashboard credentials instead of a global api_key, which is explained in the comment above the check. Rather than ripping out the documented bypass, fix the doc to match reality: Some(true) logs a boot-time warning when no auth is configured, but only actually enforces once some form of auth is present. Drop the stray 'auto' TOML literal from the doc too — Option<bool> never accepted a string value. Also simplify the redundant info-log guard in server.rs (the outer match already narrows to the None branch) and extract the derivation to a small pure function with unit tests covering all four input combinations (None + auth, None + no auth, Some(false), Some(true)). * docs(changelog): flag require_auth_for_reads default change as breaking --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This was referenced Apr 14, 2026
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
1 participant
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.
Summary
Complement
This is PR-A of two complementary changes. A follow-up PR will update the dashboard SPA to log in before pulling read endpoints so operators can flip the flag without breaking the UI.
Reproduction before fix
After setting `require_auth_for_reads = true`:
Test plan