Guard admin endpoints behind auth by default

[ChristianPavilonis]

Summary

• Fix security vulnerability where /admin/keys/rotate and /admin/keys/deactivate were publicly accessible when no handler regex covered /admin/* paths
• Settings::from_toml_and_env() now returns a hard error when no handler covers admin endpoints, catching misconfiguration at build time
• enforce_basic_auth remains a single, simple mechanism — admin protection is enforced by requiring handler coverage in the configuration

Changes

File	Change
crates/common/src/auth.rs	Simplify enforce_basic_auth — remove is_admin_path() and runtime admin guard; admin paths use standard handler-based auth
crates/common/src/settings.rs	Add build-time validation: from_toml_and_env() errors when uncovered_admin_endpoints() finds unprotected admin paths; update doc comments and tests
crates/common/build.rs	Remove cargo:warning loop (redundant — from_toml_and_env now errors on uncovered admin endpoints)
crates/common/src/test_support.rs	Add ^/admin handler to base test settings
trusted-server.toml	Add ^/admin handler to dev config

Closes

Closes #400

Test plan

• cargo test --workspace (472 tests pass)
• cargo clippy --all-targets --all-features -- -D warnings
• cargo fmt --all -- --check

Checklist

• Changes follow CLAUDE.md conventions
• No unwrap() in production code — use expect("should ...")
• Uses tracing macros (not println!)
• New code has tests
• No secrets or credentials committed

[aram356]

Summary

This PR correctly identifies and fixes the security gap where admin endpoints could be accessed without authentication when no handler is configured. The fail-closed approach is sound. However, the implementation adds complexity that may not be necessary given the existing handler-based auth system.

♻️ refactor

• Simplify: use config validation instead of runtime guard: The handler-based auth already enforces credentials correctly. Instead of adding is_admin_path() and special-case logic in enforce_basic_auth, make Settings::validate() return an error when no handler covers admin endpoints. This keeps enforce_basic_auth unchanged, eliminates the dual-mechanism (is_admin_path prefix check vs ADMIN_ENDPOINTS list), and catches misconfiguration at build time as an error rather than silently returning 401s at runtime. The existing uncovered_admin_endpoints() method is the right building block — call it from validate() instead of build.rs.

[aram356]

To clarify the refactor suggestion above — the required config is just a standard [[handlers]] entry in trusted-server.toml:

[[handlers]]
path = "^/admin"
username = "admin"
password = "a-strong-password"

With the validation approach, Settings::validate() would error at build time if this entry is missing, ensuring admin endpoints are always covered by the existing handler-based auth — no runtime special-casing needed.

[ChristianPavilonis]

Addressed the review feedback — refactored from runtime guard to build-time config validation:

• Removed is_admin_path() and the special-case admin branch from enforce_basic_auth — it's back to a single, simple mechanism
• Added a hard error in Settings::from_toml_and_env() when uncovered_admin_endpoints() returns any uncovered paths — build fails immediately with a clear message
• Removed the cargo:warning loop in build.rs (redundant now that from_toml_and_env errors)
• Added ^/admin handler to trusted-server.toml and test fixtures
• Added from_toml_and_env_rejects_config_without_admin_handler test

All 472 tests pass, clippy clean, fmt clean.

[aram356]

Summary

Build-time config validation for admin endpoints is the right approach — misconfiguration becomes a compile error. The implementation is clean and well-tested. Two items need addressing before merge.

Blocking

🔧 wrench

• from_toml() bypasses admin validation: from_toml() is pub and skips the uncovered_admin_endpoints() check entirely. Defense-in-depth: add the same check to from_toml() so no code path can construct a Settings without admin coverage. Cost is one trivial Vec allocation. See inline comment on settings.rs:377.
• ADMIN_ENDPOINTS can drift from router: The hardcoded list has no compile-time or test-time link to the actual route table in main.rs:97-98. A new admin route added without updating this constant ships unprotected. See inline comment on settings.rs:404.

Non-blocking

🌱 seedling

• Placeholder changeme password: No validation rejects weak handler passwords, unlike the synthetic secret key check. (trusted-server.toml:9)

♻️ refactor

• Nested temp_env::with_var: 7-level nesting can be flattened with temp_env::with_vars. (crates/common/src/settings.rs:841)

🤔 thinking

• uncovered_admin_endpoints visibility: pub is broader than needed — pub(crate) would suffice. (crates/common/src/settings.rs:412)

⛏ nitpick

• Comment says "fail" but means "panic": .expect() panics, not returns an error. (crates/common/build.rs:31)

👍 praise

• Build-time validation approach: Shifting auth enforcement to config validation at compile time is architecturally sound. The uncovered_admin_endpoints method and its test coverage (full/partial/no coverage) are thorough.

CI Status

• cargo fmt: PASS
• cargo test: PASS
• vitest: PASS
• format-typescript: PASS
• format-docs: PASS
• CodeQL: PASS

[aram356]

🔧 wrench — This check only runs in from_toml_and_env(), but from_toml() (line 331) is pub and skips it entirely.

The safety argument is that build.rs already validated, so the embedded TOML is guaranteed safe. But from_toml() is a public API — nothing prevents a future caller from constructing Settings with TOML that lacks an admin handler.

Adding the same uncovered_admin_endpoints() check inside from_toml() costs one trivial Vec allocation per server startup and provides defense-in-depth:

pub fn from_toml(toml_str: &str) -> Result<Self, Report<TrustedServerError>> {
    let settings: Self =
        toml::from_str(toml_str).change_context(TrustedServerError::Configuration {
            message: "Failed to deserialize TOML configuration".to_string(),
        })?;

    let uncovered = settings.uncovered_admin_endpoints();
    if !uncovered.is_empty() {
        return Err(Report::new(TrustedServerError::Configuration {
            message: format!(
                "No handler covers admin endpoint(s): {}",
                uncovered.join(", ")
            ),
        }));
    }

    Ok(settings)
}

[aram356]

🔧 wrench — ADMIN_ENDPOINTS can drift from the actual route table.

This hardcoded list must be manually kept in sync with the route match arms in crates/fastly/src/main.rs:97-98. No compiler error or test catches drift — if someone adds POST /admin/keys/list to main.rs but forgets to update this constant, the new endpoint ships unprotected.

Options (pick one):

1. Extract admin routes into a shared constant in crates/common that both the router and this validation reference.
2. Add a test in crates/fastly that asserts the routes and ADMIN_ENDPOINTS stay in sync.
3. At minimum, add a comment at the route definitions in main.rs:97-98 pointing back to this constant.

[aram356]

🌱 seedling — Placeholder changeme password ships in config.

The synthetic secret key has a runtime rejection for placeholder values (settings_data.rs:37), but handler passwords have no equivalent check. A deployment with password = "changeme" passes all validation.

Consider a validate_password custom validator for admin handlers in a follow-up, or at minimum document that operators must override via env vars in production.

[aram356]

♻️ refactor — 7-level temp_env::with_var nesting.

Use temp_env::with_vars (plural) to flatten to a single level:

temp_env::with_vars(
    [
        (origin_key, Some("https://origin.test-publisher.com")),
        (path_key_0, Some("^/env-handler")),
        (username_key_0, Some("env-user")),
        (password_key_0, Some("env-pass")),
        (path_key_1, Some("^/admin")),
        (username_key_1, Some("admin")),
        (password_key_1, Some("admin-pass")),
    ],
    || {
        let settings = Settings::from_toml_and_env(&toml_str)
            .expect("Settings should load from env");
        assert_eq!(settings.handlers.len(), 2);
        let handler = &settings.handlers[0];
        assert_eq!(handler.path, "^/env-handler");
        assert_eq!(handler.username, "env-user");
        assert_eq!(handler.password, "env-pass");
    },
);

[aram356]

🤔 thinking — uncovered_admin_endpoints is pub but only called internally by from_toml_and_env() and tests. pub(crate) would keep the API surface tighter and signal this is an internal mechanism, not a stable public API.

[aram356]

⛏ nitpick — "This will fail if..." — since .expect() is called on line 33, the build panics. Consider: "Panics if admin endpoints are not covered by a handler."

[aram356]

👍 praise — Shifting from runtime guards to build-time config validation is the right architectural call. Misconfiguration becomes a compile error rather than a silent runtime gap. The uncovered_admin_endpoints method is clean, testable, and well-documented, and the test coverage of full/partial/no coverage is thorough.

[ChristianPavilonis]

Review feedback addressed

All comments from the second review have been addressed in 0ffad1b:

Blocking

• from_toml() bypasses admin validation — Fixed. from_toml() now runs the same uncovered_admin_endpoints() check as from_toml_and_env(). Updated all test TOMLs (in google_tag_manager.rs, prebid.rs) to include a [[handlers]] entry covering /admin. Tests that deliberately exercise uncovered_admin_endpoints() on invalid configs use toml::from_str directly to bypass validation.

• ADMIN_ENDPOINTS can drift from router — Added admin_endpoints_match_fastly_router sync test in crates/common/src/settings.rs. It uses include_str! to read crates/fastly/src/main.rs at compile time and asserts both directions: every ADMIN_ENDPOINTS entry exists in the router, and every /admin/ route in the router exists in ADMIN_ENDPOINTS. Also added a cross-reference comment at the router match arms.

Non-blocking

• Nested temp_env::with_var — Flattened 7-level nesting to a single temp_env::with_vars call.
• uncovered_admin_endpoints visibility — Changed both ADMIN_ENDPOINTS and uncovered_admin_endpoints() to pub(crate).
• Comment says "fail" but means "panic" — Fixed in build.rs.

All 474 tests pass, clippy clean, fmt clean.