From 8b1a1d7bb28f3bd21e47cb1c8d018cd745623a09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20LIARD?= Date: Sun, 26 Apr 2026 17:56:38 +0200 Subject: [PATCH] refactor(registry): require SecretBackend on from_configs_with_models MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The signature of `ProviderRegistry::from_configs_with_models` previously took raw `&[ProviderConfig]` and trusted callers to invoke `storage::secrets::resolve_provider_secrets` first. That trust failed three times in three months — once per reload path: - PR #280: `preset::build_registry` (CLI `grob validate`) - PR #284: `server::config_guard`, `server::config_api`, `server::rpc::server_ns` (PUT /api/config, POST /api/config/reload, JSON-RPC server/reload) The bug repeated because the API made it optional to do the right thing. This refactor makes resolution mandatory at the type level: every caller must pass `&dyn SecretBackend`, and the function applies `resolve_provider_secrets` internally before constructing any provider. Diff summary: - 1 new required parameter (`secret_backend: &dyn SecretBackend`) - 5 callers simplified (build_backend → pass through, drop the local `resolve_provider_secrets` step that PRs #280/#284 added) - 1 test fixture switched to `EnvBackend` (no-op for literal keys) - 1 new regression test (`from_configs_routes_resolution_through_backend`) that asserts the backend's `get` is called exactly once for a `secret:`-prefixed provider — would have caught all three historical bugs at compile-AND-test time Full test suite passes: 1231 tests, 1231 passed. Closes the bug class entirely. Any future caller that builds a ProviderRegistry now compiles only if it supplies a backend, and any backend it passes will be exercised by the resolver. We can never again "forget to call resolve_provider_secrets". Co-Authored-By: Claude Opus 4.7 (1M context) --- src/preset/validation.rs | 7 +-- src/providers/registry.rs | 113 +++++++++++++++++++++++++++++++++++- src/server/config_api.rs | 13 ++--- src/server/config_guard.rs | 7 +-- src/server/init.rs | 7 +-- src/server/rpc/config_ns.rs | 3 + src/server/rpc/hit_ns.rs | 3 + src/server/rpc/pledge_ns.rs | 3 + src/server/rpc/server_ns.rs | 12 ++-- src/server/rpc/tools_ns.rs | 3 + 10 files changed, 138 insertions(+), 33 deletions(-) diff --git a/src/preset/validation.rs b/src/preset/validation.rs index 76784f48..9ad20b73 100644 --- a/src/preset/validation.rs +++ b/src/preset/validation.rs @@ -72,14 +72,11 @@ pub fn build_registry(config: &AppConfig) -> Result<(Arc, Toke ); let secret_backend = crate::storage::secrets::build_backend(&config.secrets, grob_store.clone()); - let resolved_providers = crate::storage::secrets::resolve_provider_secrets( - &config.providers, - secret_backend.as_ref(), - ); let registry = Arc::new( ProviderRegistry::from_configs_with_models( - &resolved_providers, + &config.providers, + secret_backend.as_ref(), Some(token_store.clone()), &config.models, &config.server.timeouts, diff --git a/src/providers/registry.rs b/src/providers/registry.rs index c8859ae9..d20545a4 100644 --- a/src/providers/registry.rs +++ b/src/providers/registry.rs @@ -311,13 +311,35 @@ impl ProviderRegistry { } } - /// Load providers from configuration with model mappings + /// Load providers from configuration with model mappings. + /// + /// Resolves `secret:` and `$ENV_VAR` placeholders in each + /// provider's `api_key` through the supplied [`SecretBackend`] before + /// building the underlying client. This is the single entry point used + /// by `server::init`, the CLI `validate` command, and every hot-reload + /// path; making the backend a required parameter prevents the recurring + /// class of bug where a caller forgot the resolution step and the + /// literal placeholder ended up as the bearer token (PR #280, PR #284). + /// + /// Callers that have no secrets to resolve — typically tests using + /// literal keys — can pass [`storage::secrets::EnvBackend`], which is + /// stateless and a no-op for non-`secret:` / non-`$` strings. + /// + /// # Errors + /// + /// Returns a `ProviderError` when any provider's underlying client + /// cannot be built or when an OAuth-typed provider is missing its + /// `oauth_provider` reference. pub fn from_configs_with_models( configs: &[ProviderConfig], + secret_backend: &dyn crate::storage::secrets::SecretBackend, token_store: Option, models: &[ModelConfig], timeouts: &TimeoutConfig, ) -> Result { + let resolved = crate::storage::secrets::resolve_provider_secrets(configs, secret_backend); + let configs = &resolved; + let mut registry = Self::new(); let build_ctx = ProviderBuildContext { token_store, @@ -655,9 +677,13 @@ mod tests { }, ]; - // Actually test the method we implemented + // Test fixtures use literal API keys (no `secret:` / `$` prefix), + // so any backend is a no-op here. Use `EnvBackend` because it is + // stateless and avoids creating a temporary `GrobStore`. + let backend = crate::storage::secrets::EnvBackend; let registry = ProviderRegistry::from_configs_with_models( &providers, + &backend, None, // token_store &models, &TimeoutConfig::default(), @@ -669,4 +695,87 @@ mod tests { assert!(registry.list_models().contains(&"model-2".to_string())); assert_eq!(registry.list_providers().len(), 2); } + + #[test] + fn from_configs_routes_resolution_through_backend() { + // Regression guard for the class of bug fixed by PR #280, #284, and + // this refactor. Three reload paths previously bypassed the secret + // resolution step and shipped the literal `secret:openrouter` as + // the upstream bearer token. Now `from_configs_with_models` + // requires a `&dyn SecretBackend` and applies resolution + // internally — any future caller that compiles also resolves. + // + // The `LlmProvider` trait does not expose the resolved api_key for + // inspection (security: zeroize on drop, secrecy crate). We therefore + // verify the integration by counting how many times the backend's + // `get` is invoked: exactly once for our single `secret:`-prefixed + // provider. A future caller forgetting to call this function would + // surface as a zero-call counter even before reaching production. + use crate::providers::AuthType; + use crate::storage::secrets::SecretBackend; + use std::sync::atomic::{AtomicUsize, Ordering}; + + struct CountingBackend { + calls: AtomicUsize, + } + impl SecretBackend for CountingBackend { + fn get(&self, name: &str) -> Option { + self.calls.fetch_add(1, Ordering::SeqCst); + if name == "openrouter" { + Some(SecretString::new("sk-resolved-real-key".into())) + } else { + None + } + } + fn label(&self) -> &'static str { + "counting" + } + } + + let backend = CountingBackend { + calls: AtomicUsize::new(0), + }; + let providers = vec![ProviderConfig { + name: "openrouter".to_string(), + provider_type: "openrouter".to_string(), + auth_type: AuthType::ApiKey, + api_key: Some(SecretString::new("secret:openrouter".to_string())), + base_url: None, + models: vec![], + enabled: Some(true), + oauth_provider: None, + project_id: None, + location: None, + headers: None, + budget_usd: None, + region: None, + pass_through: Some(true), + tls_cert: None, + tls_key: None, + tls_ca: None, + pool: None, + circuit_breaker: None, + health_check: None, + }]; + + let registry = ProviderRegistry::from_configs_with_models( + &providers, + &backend, + None, + &[], + &TimeoutConfig::default(), + ) + .expect("registry build"); + + assert_eq!( + backend.calls.load(Ordering::SeqCst), + 1, + "secret: prefix must trigger exactly one backend lookup; \ + zero would mean the resolution step was skipped" + ); + assert!( + registry.provider("openrouter").is_some(), + "registry must contain the resolved provider" + ); + } } diff --git a/src/server/config_api.rs b/src/server/config_api.rs index 0264bc51..eb1cd5cd 100644 --- a/src/server/config_api.rs +++ b/src/server/config_api.rs @@ -223,18 +223,15 @@ pub(crate) async fn reload_config(State(state): State>) -> Respons // 2. Build new router (compiles regexes) let new_router = Router::new(new_config.clone()); - // 3. Build new provider registry. Resolve `secret:` and - // `$ENV_VAR` placeholders before passing the providers in, so a - // hot reload behaves the same as `grob start` and CLI `validate`. - // Same code path as `server::init` and `preset::build_registry`. + // 3. Build new provider registry (reuse existing token_store). + // `from_configs_with_models` resolves `secret:` and + // `$ENV_VAR` placeholders internally via the supplied backend, so + // a hot reload behaves the same as `grob start` and CLI `validate`. let secret_backend = crate::storage::secrets::build_backend(&new_config.secrets, state.grob_store.clone()); - let resolved_providers = crate::storage::secrets::resolve_provider_secrets( + let new_registry = match ProviderRegistry::from_configs_with_models( &new_config.providers, secret_backend.as_ref(), - ); - let new_registry = match ProviderRegistry::from_configs_with_models( - &resolved_providers, Some(state.token_store.clone()), &new_config.models, &new_config.server.timeouts, diff --git a/src/server/config_guard.rs b/src/server/config_guard.rs index 3d2cea69..b0a8132f 100644 --- a/src/server/config_guard.rs +++ b/src/server/config_guard.rs @@ -114,13 +114,10 @@ fn reload_state( let secret_backend = crate::storage::secrets::build_backend(&config.secrets, state.grob_store.clone()); - let resolved_providers = crate::storage::secrets::resolve_provider_secrets( - &config.providers, - secret_backend.as_ref(), - ); let new_registry = crate::providers::ProviderRegistry::from_configs_with_models( - &resolved_providers, + &config.providers, + secret_backend.as_ref(), Some(state.token_store.clone()), &config.models, &config.server.timeouts, diff --git a/src/server/init.rs b/src/server/init.rs index 000d7a22..8a652f04 100644 --- a/src/server/init.rs +++ b/src/server/init.rs @@ -50,14 +50,11 @@ pub(crate) async fn init_core_services( let secret_backend = crate::storage::secrets::build_backend(&config.secrets, grob_store.clone()); info!("🔑 Secret backend: {}", secret_backend.label()); - let resolved_providers = crate::storage::secrets::resolve_provider_secrets( - &config.providers, - secret_backend.as_ref(), - ); let provider_registry = Arc::new( ProviderRegistry::from_configs_with_models( - &resolved_providers, + &config.providers, + secret_backend.as_ref(), Some(token_store.clone()), &config.models, &config.server.timeouts, diff --git a/src/server/rpc/config_ns.rs b/src/server/rpc/config_ns.rs index af385810..d997646a 100644 --- a/src/server/rpc/config_ns.rs +++ b/src/server/rpc/config_ns.rs @@ -65,8 +65,11 @@ pub async fn set( // `config_guard::persist_and_reload`: persistence to disk is a non-goal // for #228 (in-memory mutation only). let new_router = Router::new(new_config.clone()); + let secret_backend = + crate::storage::secrets::build_backend(&new_config.secrets, state.grob_store.clone()); let new_registry = ProviderRegistry::from_configs_with_models( &new_config.providers, + secret_backend.as_ref(), Some(state.token_store.clone()), &new_config.models, &new_config.server.timeouts, diff --git a/src/server/rpc/hit_ns.rs b/src/server/rpc/hit_ns.rs index bd377e0c..6934ca50 100644 --- a/src/server/rpc/hit_ns.rs +++ b/src/server/rpc/hit_ns.rs @@ -144,8 +144,11 @@ fn swap_state( action: &str, ) -> Result<(), ErrorObjectOwned> { let new_router = Router::new(new_config.clone()); + let secret_backend = + crate::storage::secrets::build_backend(&new_config.secrets, state.grob_store.clone()); let new_registry = ProviderRegistry::from_configs_with_models( &new_config.providers, + secret_backend.as_ref(), Some(state.token_store.clone()), &new_config.models, &new_config.server.timeouts, diff --git a/src/server/rpc/pledge_ns.rs b/src/server/rpc/pledge_ns.rs index de95bbd6..2deacba1 100644 --- a/src/server/rpc/pledge_ns.rs +++ b/src/server/rpc/pledge_ns.rs @@ -157,8 +157,11 @@ fn swap_state( action: &str, ) -> Result<(), ErrorObjectOwned> { let new_router = Router::new(new_config.clone()); + let secret_backend = + crate::storage::secrets::build_backend(&new_config.secrets, state.grob_store.clone()); let new_registry = ProviderRegistry::from_configs_with_models( &new_config.providers, + secret_backend.as_ref(), Some(state.token_store.clone()), &new_config.models, &new_config.server.timeouts, diff --git a/src/server/rpc/server_ns.rs b/src/server/rpc/server_ns.rs index 811c0a13..dfe88266 100644 --- a/src/server/rpc/server_ns.rs +++ b/src/server/rpc/server_ns.rs @@ -60,18 +60,14 @@ pub async fn reload_config( let new_router = Router::new(new_config.clone()); - // Resolve `secret:` and `$ENV_VAR` placeholders so the JSON-RPC - // reload path sees the same authenticated registry as `grob start`, - // `validate`, and the HTTP `/api/config/reload` endpoint. + // `from_configs_with_models` resolves `secret:` and `$ENV_VAR` + // placeholders internally via the supplied backend. let secret_backend = crate::storage::secrets::build_backend(&new_config.secrets, state.grob_store.clone()); - let resolved_providers = crate::storage::secrets::resolve_provider_secrets( - &new_config.providers, - secret_backend.as_ref(), - ); let new_registry = ProviderRegistry::from_configs_with_models( - &resolved_providers, + &new_config.providers, + secret_backend.as_ref(), Some(state.token_store.clone()), &new_config.models, &new_config.server.timeouts, diff --git a/src/server/rpc/tools_ns.rs b/src/server/rpc/tools_ns.rs index 008742d8..49eee2e9 100644 --- a/src/server/rpc/tools_ns.rs +++ b/src/server/rpc/tools_ns.rs @@ -160,8 +160,11 @@ fn swap_state( action: &str, ) -> Result<(), ErrorObjectOwned> { let new_router = Router::new(new_config.clone()); + let secret_backend = + crate::storage::secrets::build_backend(&new_config.secrets, state.grob_store.clone()); let new_registry = ProviderRegistry::from_configs_with_models( &new_config.providers, + secret_backend.as_ref(), Some(state.token_store.clone()), &new_config.models, &new_config.server.timeouts,