feat(mcp): security hardening — command allowlist and env var validation

CHANGELOG.md

@@ -6,6 +6,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 
 ## [Unreleased]
 
+### Added
+- MCP server command allowlist validation — only permitted commands (npx, uvx, node, python3, python, docker, deno, bun) can spawn child processes; configurable via `mcp.allowed_commands`
+- MCP env var blocklist — blocks 21 dangerous variables (LD_PRELOAD, DYLD_*, NODE_OPTIONS, PYTHONPATH, JAVA_TOOL_OPTIONS, etc.) and BASH_FUNC_* prefix from MCP server processes
+- Path separator rejection in MCP command validation to prevent symlink-based bypasses
+
 ### Changed
 - Shell command detection rewritten from substring matching to tokenizer-based pipeline with escape normalization, eliminating bypass vectors via backslash insertion, hex/octal escapes, quote splitting, and pipe chains
 - Shell sandbox path validation now uses `std::path::absolute()` as fallback when `canonicalize()` fails on non-existent paths

crates/zeph-core/src/bootstrap.rs

@@ -849,7 +849,7 @@ pub fn create_mcp_manager(config: &Config) -> zeph_mcp::McpManager {
             }
         })
         .collect();
-    zeph_mcp::McpManager::new(entries)
+    zeph_mcp::McpManager::new(entries, config.mcp.allowed_commands.clone())
 }
 
 pub async fn create_mcp_registry(

crates/zeph-mcp/src/client.rs

@@ -42,8 +42,12 @@ impl McpClient {
         command: &str,
         args: &[String],
         env: &std::collections::HashMap<String, String>,
+        allowed_commands: &[String],
         timeout: Duration,
     ) -> Result<Self, McpError> {
+        crate::security::validate_command(command, allowed_commands)?;
+        crate::security::validate_env(env)?;
+
         let mut cmd = Command::new(command);
         cmd.args(args);
         for (k, v) in env {

crates/zeph-mcp/src/error.rs

@@ -46,6 +46,12 @@ pub enum McpError {
 
     #[error("embedding error: {0}")]
     Embedding(String),
+
+    #[error("MCP command '{command}' not allowed")]
+    CommandNotAllowed { command: String },
+
+    #[error("env var '{var_name}' is blocked for MCP server processes")]
+    EnvVarBlocked { var_name: String },
 }
 
 #[cfg(test)]

crates/zeph-mcp/src/executor.rs

@@ -203,23 +203,23 @@ mod tests {
 
     #[test]
     fn executor_construction() {
-        let mgr = Arc::new(McpManager::new(vec![]));
+        let mgr = Arc::new(McpManager::new(vec![], vec![]));
         let executor = McpToolExecutor::new(mgr);
         let dbg = format!("{executor:?}");
         assert!(dbg.contains("McpToolExecutor"));
     }
 
     #[tokio::test]
     async fn execute_no_blocks_returns_none() {
-        let mgr = Arc::new(McpManager::new(vec![]));
+        let mgr = Arc::new(McpManager::new(vec![], vec![]));
         let executor = McpToolExecutor::new(mgr);
         let result = executor.execute("no mcp blocks here").await.unwrap();
         assert!(result.is_none());
     }
 
     #[tokio::test]
     async fn execute_invalid_json_block_returns_error() {
-        let mgr = Arc::new(McpManager::new(vec![]));
+        let mgr = Arc::new(McpManager::new(vec![], vec![]));
         let executor = McpToolExecutor::new(mgr);
         let text = "```mcp\nnot json\n```";
         let result = executor.execute(text).await;
@@ -228,7 +228,7 @@ mod tests {
 
     #[tokio::test]
     async fn execute_valid_block_server_not_connected() {
-        let mgr = Arc::new(McpManager::new(vec![]));
+        let mgr = Arc::new(McpManager::new(vec![], vec![]));
         let executor = McpToolExecutor::new(mgr);
         let text = "```mcp\n{\"server\":\"missing\",\"tool\":\"t\"}\n```";
         let result = executor.execute(text).await;

crates/zeph-mcp/src/lib.rs

@@ -6,6 +6,7 @@ pub mod executor;
 pub mod manager;
 pub mod prompt;
 pub mod registry;
+pub mod security;
 pub mod tool;
 
 pub use error::McpError;

crates/zeph-mcp/src/manager.rs

@@ -33,6 +33,7 @@ pub struct ServerEntry {
 
 pub struct McpManager {
     configs: Vec<ServerEntry>,
+    allowed_commands: Vec<String>,
     clients: Arc<RwLock<HashMap<String, McpClient>>>,
 }
 
@@ -46,9 +47,10 @@ impl std::fmt::Debug for McpManager {
 
 impl McpManager {
     #[must_use]
-    pub fn new(configs: Vec<ServerEntry>) -> Self {
+    pub fn new(configs: Vec<ServerEntry>, allowed_commands: Vec<String>) -> Self {
         Self {
             configs,
+            allowed_commands,
             clients: Arc::new(RwLock::new(HashMap::new())),
         }
     }
@@ -58,9 +60,11 @@ impl McpManager {
     pub async fn connect_all(&self) -> Vec<McpTool> {
         let mut join_set = JoinSet::new();
 
+        let allowed = self.allowed_commands.clone();
         for config in self.configs.clone() {
+            let allowed = allowed.clone();
             join_set.spawn(async move {
-                let result = connect_entry(&config).await;
+                let result = connect_entry(&config, &allowed).await;
                 (config.id, result)
             });
         }
@@ -131,7 +135,7 @@ impl McpManager {
             }
         }
 
-        let client = connect_entry(entry).await?;
+        let client = connect_entry(entry, &self.allowed_commands).await?;
         let tools = match client.list_tools().await {
             Ok(tools) => tools,
             Err(e) => {
@@ -208,10 +212,21 @@ impl McpManager {
     }
 }
 
-async fn connect_entry(entry: &ServerEntry) -> Result<McpClient, McpError> {
+async fn connect_entry(
+    entry: &ServerEntry,
+    allowed_commands: &[String],
+) -> Result<McpClient, McpError> {
     match &entry.transport {
         McpTransport::Stdio { command, args, env } => {
-            McpClient::connect(&entry.id, command, args, env, entry.timeout).await
+            McpClient::connect(
+                &entry.id,
+                command,
+                args,
+                env,
+                allowed_commands,
+                entry.timeout,
+            )
+            .await
         }
         McpTransport::Http { url } => McpClient::connect_url(&entry.id, url, entry.timeout).await,
     }
@@ -235,13 +250,13 @@ mod tests {
 
     #[tokio::test]
     async fn list_servers_empty() {
-        let mgr = McpManager::new(vec![]);
+        let mgr = McpManager::new(vec![], vec![]);
         assert!(mgr.list_servers().await.is_empty());
     }
 
     #[tokio::test]
     async fn remove_server_not_found_returns_error() {
-        let mgr = McpManager::new(vec![]);
+        let mgr = McpManager::new(vec![], vec![]);
         let err = mgr.remove_server("nonexistent").await.unwrap_err();
         assert!(
             matches!(err, McpError::ServerNotFound { ref server_id } if server_id == "nonexistent")
@@ -250,24 +265,24 @@ mod tests {
     }
 
     #[tokio::test]
-    async fn add_server_nonexistent_binary_returns_connection_error() {
-        let mgr = McpManager::new(vec![]);
+    async fn add_server_nonexistent_binary_returns_command_not_allowed() {
+        let mgr = McpManager::new(vec![], vec![]);
         let entry = make_entry("test-server");
         let err = mgr.add_server(&entry).await.unwrap_err();
-        assert!(matches!(err, McpError::Connection { .. }));
+        assert!(matches!(err, McpError::CommandNotAllowed { .. }));
     }
 
     #[tokio::test]
     async fn connect_all_skips_failing_servers() {
-        let mgr = McpManager::new(vec![make_entry("a"), make_entry("b")]);
+        let mgr = McpManager::new(vec![make_entry("a"), make_entry("b")], vec![]);
         let tools = mgr.connect_all().await;
         assert!(tools.is_empty());
         assert!(mgr.list_servers().await.is_empty());
     }
 
     #[tokio::test]
     async fn call_tool_server_not_found() {
-        let mgr = McpManager::new(vec![]);
+        let mgr = McpManager::new(vec![], vec![]);
         let err = mgr
             .call_tool("missing", "some_tool", serde_json::json!({}))
             .await
@@ -294,15 +309,18 @@ mod tests {
 
     #[test]
     fn manager_debug() {
-        let mgr = McpManager::new(vec![make_entry("a"), make_entry("b")]);
+        let mgr = McpManager::new(vec![make_entry("a"), make_entry("b")], vec![]);
         let dbg = format!("{mgr:?}");
         assert!(dbg.contains("server_count"));
         assert!(dbg.contains("2"));
     }
 
     #[tokio::test]
     async fn list_servers_returns_sorted() {
-        let mgr = McpManager::new(vec![make_entry("z"), make_entry("a"), make_entry("m")]);
+        let mgr = McpManager::new(
+            vec![make_entry("z"), make_entry("a"), make_entry("m")],
+            vec![],
+        );
         // No servers connected (all fail), so list is empty
         mgr.connect_all().await;
         let ids = mgr.list_servers().await;
@@ -318,21 +336,21 @@ mod tests {
 
     #[tokio::test]
     async fn remove_server_preserves_other_entries() {
-        let mgr = McpManager::new(vec![]);
+        let mgr = McpManager::new(vec![], vec![]);
         // With no connected servers, remove always returns ServerNotFound
         assert!(mgr.remove_server("a").await.is_err());
         assert!(mgr.remove_server("b").await.is_err());
         assert!(mgr.list_servers().await.is_empty());
     }
 
     #[tokio::test]
-    async fn add_server_connection_error_preserves_message() {
-        let mgr = McpManager::new(vec![]);
+    async fn add_server_command_not_allowed_preserves_message() {
+        let mgr = McpManager::new(vec![], vec![]);
         let entry = make_entry("my-server");
         let err = mgr.add_server(&entry).await.unwrap_err();
         let msg = err.to_string();
-        assert!(msg.contains("my-server"));
-        assert!(msg.contains("connection failed"));
+        assert!(msg.contains("nonexistent-mcp-binary"));
+        assert!(msg.contains("not allowed"));
     }
 
     #[test]
@@ -402,7 +420,7 @@ mod tests {
 
     #[tokio::test]
     async fn add_server_http_nonexistent_returns_connection_error() {
-        let mgr = McpManager::new(vec![]);
+        let mgr = McpManager::new(vec![], vec![]);
         let entry = make_http_entry("http-test");
         let err = mgr.add_server(&entry).await.unwrap_err();
         assert!(matches!(
@@ -413,14 +431,17 @@ mod tests {
 
     #[test]
     fn manager_new_stores_configs() {
-        let mgr = McpManager::new(vec![make_entry("a"), make_entry("b"), make_entry("c")]);
+        let mgr = McpManager::new(
+            vec![make_entry("a"), make_entry("b"), make_entry("c")],
+            vec![],
+        );
         let dbg = format!("{mgr:?}");
         assert!(dbg.contains("3"));
     }
 
     #[tokio::test]
     async fn call_tool_different_missing_servers() {
-        let mgr = McpManager::new(vec![]);
+        let mgr = McpManager::new(vec![], vec![]);
         for id in &["server-a", "server-b", "server-c"] {
             let err = mgr
                 .call_tool(id, "tool", serde_json::json!({}))
@@ -436,7 +457,7 @@ mod tests {
 
     #[tokio::test]
     async fn connect_all_with_http_entries_skips_failing() {
-        let mgr = McpManager::new(vec![make_http_entry("x"), make_http_entry("y")]);
+        let mgr = McpManager::new(vec![make_http_entry("x"), make_http_entry("y")], vec![]);
         let tools = mgr.connect_all().await;
         assert!(tools.is_empty());
         assert!(mgr.list_servers().await.is_empty());