From 1b0f1b7957d3c814e06ec45232c91ae4bf8f7c84 Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Sat, 30 May 2026 03:35:39 +0200 Subject: [PATCH] fix(skills): enforce fail-closed semantic scan provider validation (#4706, #4709) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When semantic_scan is enabled but semantic_scan_provider is empty or refers to an unknown provider, plugin add now returns an explicit CommandError instead of silently proceeding with the primary provider. - Trim provider name before empty check to prevent whitespace bypass - Add registry lookup before resolve_background_provider; unknown provider names are rejected rather than silently falling back - resolve_background_provider itself is unchanged — other callers retain their intentional fallback semantics Closes #4709 Closes #4706 --- .../zeph-core/src/agent/agent_access_impl.rs | 133 +++++++++++++++++- crates/zeph-core/src/agent/builder.rs | 12 ++ 2 files changed, 142 insertions(+), 3 deletions(-) diff --git a/crates/zeph-core/src/agent/agent_access_impl.rs b/crates/zeph-core/src/agent/agent_access_impl.rs index 04b069326..030edf8c4 100644 --- a/crates/zeph-core/src/agent/agent_access_impl.rs +++ b/crates/zeph-core/src/agent/agent_access_impl.rs @@ -1059,12 +1059,37 @@ impl AgentAccess for Agent { .collect(); // Resolve scanner once, before the async block captures `self`. + // Fail-closed: if semantic_scan is enabled but no provider is configured, refuse + // to proceed rather than silently falling back to the primary provider (#4706, #4709). let semantic_scan_enabled = self.services.skill.semantic_scan; let maybe_scanner: Option = if semantic_scan_enabled { - let provider = self.resolve_background_provider( - self.services.skill.semantic_scan_provider.as_str(), - ); + let provider_name = self.services.skill.semantic_scan_provider.as_str(); + if provider_name.trim().is_empty() { + return Box::pin(async move { + Err(CommandError::new( + "semantic_scan is enabled but semantic_scan_provider is not set; \ + refusing plugin add to maintain fail-closed security posture", + )) + }); + } + let provider_known = self + .runtime + .providers + .provider_pool + .iter() + .any(|e| e.effective_name().eq_ignore_ascii_case(provider_name)); + if !provider_known { + let name = provider_name.to_owned(); + return Box::pin(async move { + Err(CommandError::new(format!( + "semantic_scan is enabled but semantic_scan_provider '{name}' \ + is not configured in [[llm.providers]]; \ + refusing plugin add to maintain fail-closed security posture", + ))) + }); + } + let provider = self.resolve_background_provider(provider_name); Some(zeph_skills::semantic_scanner::SkillSemanticScanner::new( provider, )) @@ -1840,4 +1865,106 @@ mod tests { "timeout must fire on a never-resolving future" ); } + + // R-4706/R-4709: when semantic_scan is enabled but semantic_scan_provider is empty, + // `plugin add` must return a CommandError immediately (fail-closed). Before this fix + // the code fell through to resolve_background_provider which silently used the primary + // provider, bypassing the intent that an unconfigured scanner means "do not proceed". + #[tokio::test] + async fn plugin_add_semantic_scan_enabled_empty_provider_returns_error() { + let mut agent = Agent::new( + mock_provider(vec![]), + MockChannel::new(vec![]), + create_test_registry(), + None, + 5, + MockToolExecutor::no_tools(), + ) + .with_semantic_scan(true, ""); + + let result = agent.handle_plugins("add some-plugin").await; + assert!( + result.is_err(), + "expected CommandError for missing semantic_scan_provider, got: {result:?}" + ); + let msg = result.unwrap_err().to_string(); + assert!( + msg.contains("semantic_scan_provider"), + "error message must mention semantic_scan_provider, got: {msg}" + ); + } + + // R-4706/R-4709: when semantic_scan is disabled, plugin subcommands must proceed + // normally regardless of whether semantic_scan_provider is set. + #[tokio::test] + async fn plugin_list_semantic_scan_disabled_succeeds() { + let mut agent = Agent::new( + mock_provider(vec![]), + MockChannel::new(vec![]), + create_test_registry(), + None, + 5, + MockToolExecutor::no_tools(), + ) + .with_semantic_scan(false, ""); + + // "list" does not trigger scan logic; it should succeed without error. + let result = agent.handle_plugins("list").await; + assert!( + result.is_ok(), + "plugin list must succeed when semantic_scan is disabled, got: {result:?}" + ); + } + + // R-4706/R-4709: "plugin add" with semantic_scan disabled must reach the install path + // rather than return a scan-related error. The install itself may fail (no real plugin + // source), but it must NOT fail with the fail-closed error message. + #[tokio::test] + async fn plugin_add_semantic_scan_disabled_no_scan_error() { + let mut agent = Agent::new( + mock_provider(vec![]), + MockChannel::new(vec![]), + create_test_registry(), + None, + 5, + MockToolExecutor::no_tools(), + ) + .with_semantic_scan(false, ""); + + let result = agent.handle_plugins("add some-plugin").await; + // The call may succeed or fail for unrelated reasons (no real plugin source), + // but must NOT fail with the fail-closed error about semantic_scan_provider. + if let Err(ref e) = result { + assert!( + !e.to_string().contains("semantic_scan_provider"), + "must not fail with scan error when semantic_scan is disabled, got: {e}" + ); + } + } + + // R-4706/R-4709: unknown provider name must also fail-closed rather than silently + // falling back to the primary provider via resolve_background_provider. + #[tokio::test] + async fn plugin_add_semantic_scan_unknown_provider_returns_error() { + let mut agent = Agent::new( + mock_provider(vec![]), + MockChannel::new(vec![]), + create_test_registry(), + None, + 5, + MockToolExecutor::no_tools(), + ) + .with_semantic_scan(true, "nonexistent_provider"); + + let result = agent.handle_plugins("add some-plugin").await; + assert!( + result.is_err(), + "expected CommandError for unknown semantic_scan_provider, got: {result:?}" + ); + let msg = result.unwrap_err().to_string(); + assert!( + msg.contains("semantic_scan_provider"), + "error message must mention semantic_scan_provider, got: {msg}" + ); + } } diff --git a/crates/zeph-core/src/agent/builder.rs b/crates/zeph-core/src/agent/builder.rs index 93343cbf9..9ca24c377 100644 --- a/crates/zeph-core/src/agent/builder.rs +++ b/crates/zeph-core/src/agent/builder.rs @@ -380,6 +380,18 @@ impl Agent { self } + /// Enable Stage-2 LLM semantic compliance scan for `plugin add` and set its provider. + /// + /// When `enabled` is `true` and `provider_name` is empty the agent will refuse `plugin add` + /// with a `CommandError` (fail-closed). Passing a non-empty `provider_name` with `enabled = + /// false` is a no-op — the scanner is only instantiated when both conditions hold. + #[must_use] + pub fn with_semantic_scan(mut self, enabled: bool, provider_name: impl Into) -> Self { + self.services.skill.semantic_scan = enabled; + self.services.skill.semantic_scan_provider = provider_name.into(); + self + } + /// Override the embedding model name used for skill matching. #[must_use] pub fn with_embedding_model(mut self, model: String) -> Self {