From cfad5bd925991b21eba27f0b291a91d536769b10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20LIARD?= Date: Tue, 28 Apr 2026 22:30:30 +0200 Subject: [PATCH 1/4] chore(prek): drop redundant cargo-test and cargo-doctest from pre-push CI re-runs the full test suite (incl. doctests) on every PR via the .github/workflows/ci.yml tests job, so local pre-push duplication adds ~20 min per push without catching anything new. Pre-push hooks should be fast-fail; expensive checks belong on the CI server. Closes audit finding: silent productivity tax (pre-push duplication). --- prek.toml | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-) diff --git a/prek.toml b/prek.toml index 82c09caa..99598f46 100644 --- a/prek.toml +++ b/prek.toml @@ -54,25 +54,10 @@ args = ["-c", "lychee --offline --no-progress --include-fragments docs/**/*.md R types = ["markdown"] pass_filenames = false -# Tests — expensive, runs only on pre-push -[[repos.hooks]] -id = "cargo-test" -name = "cargo test" -language = "system" -entry = "cargo nextest run --profile ci" -types = ["rust"] -pass_filenames = false -stages = ["pre-push"] - -# Doc tests — not covered by nextest -[[repos.hooks]] -id = "cargo-doctest" -name = "cargo doc tests" -language = "system" -entry = "cargo test --doc" -types = ["rust"] -pass_filenames = false -stages = ["pre-push"] +# NOTE: cargo-test and cargo-doctest were removed from pre-push to keep +# the gate fast-fail. CI re-runs the full suite on every PR (see +# `.github/workflows/ci.yml` `tests` job), so duplicating ~20 min of work +# locally only delays the developer feedback loop without adding signal. # Cargo deny — license & advisory & dependency checks # Only runs when Cargo.toml/lock or Rust files change. From fafba5e58d3e00deb9d7bfdfb7505093e5357ce1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20LIARD?= Date: Tue, 28 Apr 2026 22:31:23 +0200 Subject: [PATCH 2/4] docs(config): clarify is_enabled semantics and typo-safety contract Documents the three-state intent (true/false/absent) of ProviderConfig.is_enabled and the dependency on deny_unknown_fields (added in the next commit) to reject typos like enbaled = false at parse time. Behaviour is unchanged; this is purely contractual clarity to support the silent-typo-killer audit. Closes audit finding: silent typo killer on provider config. --- src/cli/config/providers.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/cli/config/providers.rs b/src/cli/config/providers.rs index 87c9ab77..586f49ae 100644 --- a/src/cli/config/providers.rs +++ b/src/cli/config/providers.rs @@ -119,7 +119,16 @@ pub struct ProviderConfig { } impl ProviderConfig { - /// Returns `true` if the provider is enabled (defaults to `true`). + /// Returns `true` if the provider is enabled. + /// + /// Semantics: + /// - `enabled = true` → enabled. + /// - `enabled = false` → disabled. + /// - `enabled` absent → enabled (sensible default for newly added blocks). + /// + /// Typo safety: `#[serde(deny_unknown_fields)]` on [`ProviderConfig`] + /// rejects misspelled keys (e.g. `enbaled`) at parse time, so an absent + /// `enabled` field genuinely means "not specified" rather than "typo'd". pub fn is_enabled(&self) -> bool { self.enabled.unwrap_or(true) } From 93401b4a3777ee11e730c9673038aeaa4f63be44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20LIARD?= Date: Tue, 28 Apr 2026 22:35:00 +0200 Subject: [PATCH 3/4] fix(config): reject unknown TOML fields in core config structs Adds #[serde(deny_unknown_fields)] to AppConfig and the major sub-structs (ProviderConfig, ModelConfig, TierConfig, RouterConfig, ScoringConfig, CacheConfig, BudgetConfig, DlpConfig, SecurityConfig). Without this guard, a typo like enbaled = false in a [[providers]] block silently parses (the unknown key is dropped) and the provider remains enabled with the wrong intent. With the guard, parsing fails loudly and the operator gets an actionable error pointing at the offending key. Tested with the full nextest suite (1268 tests) plus all doctests: no fixture, preset or example carries a stale field, so this is a pure tightening with no migration cost. Closes audit finding: silent typo killer on TOML config. --- src/cli/config/budget.rs | 1 + src/cli/config/cache.rs | 1 + src/cli/config/providers.rs | 1 + src/cli/config/routing.rs | 3 +++ src/cli/config/security.rs | 1 + src/features/dlp/config.rs | 1 + src/models/config.rs | 1 + src/routing/classify/classify.rs | 1 + 8 files changed, 10 insertions(+) diff --git a/src/cli/config/budget.rs b/src/cli/config/budget.rs index 414a2f26..4eaa6e74 100644 --- a/src/cli/config/budget.rs +++ b/src/cli/config/budget.rs @@ -6,6 +6,7 @@ use crate::cli::BudgetUsd; /// Budget configuration #[derive(Debug, Clone, Deserialize, Serialize, Default)] +#[serde(deny_unknown_fields)] pub struct BudgetConfig { /// Global monthly hard cap in USD (0 = unlimited) #[serde(default)] diff --git a/src/cli/config/cache.rs b/src/cli/config/cache.rs index b2e42554..201ddf66 100644 --- a/src/cli/config/cache.rs +++ b/src/cli/config/cache.rs @@ -4,6 +4,7 @@ use serde::{Deserialize, Serialize}; /// LLM response cache configuration #[derive(Debug, Clone, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] pub struct CacheConfig { /// Enable response caching (only for temperature=0 requests) #[serde(default)] diff --git a/src/cli/config/providers.rs b/src/cli/config/providers.rs index 586f49ae..88c240f8 100644 --- a/src/cli/config/providers.rs +++ b/src/cli/config/providers.rs @@ -28,6 +28,7 @@ pub enum AuthType { /// Provider configuration from TOML. #[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] pub struct ProviderConfig { /// Unique provider name used in routing and logging. pub name: String, diff --git a/src/cli/config/routing.rs b/src/cli/config/routing.rs index d0992450..0538177f 100644 --- a/src/cli/config/routing.rs +++ b/src/cli/config/routing.rs @@ -9,6 +9,7 @@ use super::user::PresetConfig; /// Router configuration #[derive(Debug, Clone, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] pub struct RouterConfig { /// Default model for unclassified requests pub default: String, @@ -102,6 +103,7 @@ pub struct FanOutConfig { /// Model configuration with 1:N provider mappings #[derive(Debug, Clone, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] pub struct ModelConfig { /// External model name (used in API requests) pub name: String, @@ -203,6 +205,7 @@ pub struct TierMatchCondition { /// When the scoring heuristic classifies a request, the dispatch pipeline /// resolves providers from the matching tier instead of the default model mappings. #[derive(Debug, Clone, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] pub struct TierConfig { /// Tier name — must match a `ComplexityTier` variant (case-insensitive). pub name: String, diff --git a/src/cli/config/security.rs b/src/cli/config/security.rs index 27583a9c..c403952e 100644 --- a/src/cli/config/security.rs +++ b/src/cli/config/security.rs @@ -8,6 +8,7 @@ use super::default_true; /// Security configuration (wired into middleware stack) #[derive(Debug, Clone, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] pub struct SecurityConfig { /// Master switch for security middleware #[serde(default = "default_true")] diff --git a/src/features/dlp/config.rs b/src/features/dlp/config.rs index f9329651..dc81c20e 100644 --- a/src/features/dlp/config.rs +++ b/src/features/dlp/config.rs @@ -3,6 +3,7 @@ use serde::{Deserialize, Serialize}; /// Top-level DLP configuration, mapped from `[dlp]` in TOML. #[derive(Debug, Clone, Deserialize, Serialize, Default)] +#[serde(deny_unknown_fields)] pub struct DlpConfig { /// Enables the DLP pipeline globally. #[serde(default)] diff --git a/src/models/config.rs b/src/models/config.rs index 63fe5196..1c87c1b5 100644 --- a/src/models/config.rs +++ b/src/models/config.rs @@ -25,6 +25,7 @@ use crate::features::tap::TapConfig; /// Application configuration #[derive(Debug, Clone, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] pub struct AppConfig { /// Config schema version (for forward compatibility) #[serde(default, skip_serializing_if = "Option::is_none")] diff --git a/src/routing/classify/classify.rs b/src/routing/classify/classify.rs index ebe280eb..38b70b2a 100644 --- a/src/routing/classify/classify.rs +++ b/src/routing/classify/classify.rs @@ -84,6 +84,7 @@ impl Default for ScoringThresholds { /// Scoring configuration combining weights and thresholds. #[derive(Debug, Clone, Default, serde::Deserialize, serde::Serialize)] +#[serde(deny_unknown_fields)] pub struct ScoringConfig { /// Per-signal weights. pub weights: ScoringWeights, From 56675a67fb73de87d5ffd658b1a3e5f80988eb79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20LIARD?= Date: Tue, 28 Apr 2026 22:36:52 +0200 Subject: [PATCH 4/4] docs(config-guard): document deny-list rationale and log denied reloads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each entry in DENIED_SECTIONS / DENIED_KEYS now carries a short justification table covering why it can not be hot-reloaded — either because the data is sensitive (credentials, DLP rules) or because the consumer is constructed once at process start (TLS listener, secret backend, TEE attestation, FIPS gate). Adds tee, fips, server.tls and secrets.backend to the deny-list so the documented "static-init" rationale matches actual behaviour. Also emits an INFO log on every denied attempt telling the operator to restart instead of expecting the silent reload to apply. Adds two unit tests covering the new deny entries (tee/fips sections and server.tls / secrets.backend keys) and asserts that sibling keys in the same sections remain editable. Closes audit finding: hot-reload UX (silent ignore of denied edits). --- src/server/config_guard.rs | 80 +++++++++++++++++++++++++++++++++++--- 1 file changed, 74 insertions(+), 6 deletions(-) diff --git a/src/server/config_guard.rs b/src/server/config_guard.rs index b0a8132f..58993720 100644 --- a/src/server/config_guard.rs +++ b/src/server/config_guard.rs @@ -12,29 +12,76 @@ use std::sync::Arc; use tracing::info; /// Top-level TOML sections that are never writable via any config API. -const DENIED_SECTIONS: &[&str] = &["providers", "dlp"]; +/// +/// Each entry is denied because hot-reloading it cannot be done safely +/// at runtime — either the data is sensitive (and must travel through a +/// dedicated secret API), or the code path that consumes it is set up +/// once at process start and not re-initialised on `/api/config/reload`: +/// +/// | Section | Reason | +/// |-------------|-----------------------------------------------------------------------------------------| +/// | `providers` | Contains API keys; mutate via `grob connect` / secret backend, not the config API. | +/// | `dlp` | Security policy must not be weakened by an authenticated control-plane caller. | +/// | `tee` | TEE attestation runs at startup; flipping the mode mid-flight bypasses the gate. | +/// | `fips` | FIPS mode is checked once on init; toggling at runtime gives a false sense of compliance. | +/// +/// To change any of these the operator must edit `~/.grob/config.toml` +/// and restart the daemon. +const DENIED_SECTIONS: &[&str] = &["providers", "dlp", "tee", "fips"]; /// Per-section keys that are never writable via any config API. +/// +/// These are individual fields whose host section is otherwise editable, +/// but the field itself is either credential material or wired into a +/// non-reloadable subsystem: +/// +/// | Section.Key | Reason | +/// |--------------------|---------------------------------------------------------------------------------| +/// | `router.api_key` | Credential material — never round-trip through the config API. | +/// | `budget.api_key` | Same. | +/// | `cache.api_key` | Same. | +/// | `server.tls` | TLS listener is bound at startup; rebuilding it requires a daemon restart. | +/// | `secrets.backend` | The secret backend is constructed once and shared via `Arc`; swapping it at | +/// | | runtime would orphan in-flight readers and change credential resolution semantics. | const DENIED_KEYS: &[(&str, &str)] = &[ ("router", "api_key"), ("budget", "api_key"), ("cache", "api_key"), + ("server", "tls"), + ("secrets", "backend"), ]; /// Checks whether a (section, key) pair is blocked by the deny-list. /// -/// Returns `true` when the write must be rejected: -/// - The entire `providers` section (contains API keys). -/// - The entire `dlp` section (security must not be weakened). -/// - Any `api_key` field in any section. +/// Returns `true` when the write must be rejected. See [`DENIED_SECTIONS`] +/// and [`DENIED_KEYS`] for the rationale behind every entry. A denied +/// attempt is logged at INFO so the operator sees actionable guidance +/// (restart instead of expecting a silent reload to take effect). pub fn is_section_or_key_denied(section: &str, key: &str) -> bool { if DENIED_SECTIONS.contains(§ion) { + info!( + section = %section, + "config hot-reload: section is on the deny-list; restart the daemon to apply changes" + ); return true; } if key == "api_key" { + info!( + section = %section, + key = %key, + "config hot-reload: api_key fields cannot be set via the config API; use `grob connect` or the secret backend" + ); + return true; + } + if DENIED_KEYS.iter().any(|(s, k)| *s == section && *k == key) { + info!( + section = %section, + key = %key, + "config hot-reload: key is on the deny-list; restart the daemon to apply changes" + ); return true; } - DENIED_KEYS.iter().any(|(s, k)| *s == section && *k == key) + false } /// Validates a key update against the deny-list using [`ConfigSection`]. @@ -177,6 +224,27 @@ mod tests { assert!(!is_section_or_key_denied("cache", "ttl_secs")); } + #[test] + fn deny_static_init_sections() { + // tee and fips are checked once at startup; toggling them at runtime + // would bypass the gate without the operator realising. + assert!(is_section_or_key_denied("tee", "mode")); + assert!(is_section_or_key_denied("tee", "sealed_keys")); + assert!(is_section_or_key_denied("fips", "mode")); + assert!(is_section_or_key_denied("fips", "anything")); + } + + #[test] + fn deny_static_init_keys() { + // The TLS listener and secret backend are constructed once on + // process start; both require a daemon restart to swap. + assert!(is_section_or_key_denied("server", "tls")); + assert!(is_section_or_key_denied("secrets", "backend")); + // Sibling keys in the same sections must remain editable. + assert!(!is_section_or_key_denied("server", "host")); + assert!(!is_section_or_key_denied("server", "port")); + } + #[cfg(feature = "mcp")] mod mcp_compat { use super::*;