Isolate MCP children from the parent environment and bound transport risk

Author: alexylon

CHANGELOG.md

@@ -6,6 +6,16 @@ All notable changes to Sofos are documented in this file.
 
 ### Security
 
+- **MCP server children no longer inherit the sofos environment.** Stdio MCP servers used to start with every variable the parent shell exported, so `ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, `MORPH_API_KEY`, ssh agent sockets, and AWS credentials all reached the child. The launcher now clears the environment first and forwards a small allowlist (`PATH`, `HOME` / `USERPROFILE`, `TMPDIR` / `TEMP` / `TMP`, `LANG`, every `LC_*` locale variable, plus Windows essentials like `SYSTEMROOT` and `PATHEXT`); anything else needs to be listed under the server's `env` config field. Existing servers that already declare their required vars in config are unaffected.
+- **MCP HTTP transport no longer follows redirects.** A `302` from a hostile or misconfigured MCP host could otherwise forward a configured `Authorization: Bearer ...` header to a different origin. A redirect status now returns a clear error explaining the refusal; reconfigure the server to its final URL instead.
+- **MCP HTTP response bodies are capped at 32 MB.** A server that streamed multi-GB JSON for a `tools/list` reply could previously stall a turn and exhaust memory under the 120-second timeout. Sofos now rejects oversized responses up front (when `Content-Length` is announced) or aborts mid-stream when the running total crosses the cap.
+- **MCP JSON-RPC responses are matched against the originating request id.** A server that emits an unsolicited frame or replies out of order is rejected with a clear "id mismatch" error instead of returning the wrong result to the caller. The check accepts both numeric and string-shaped echoes of sofos's outgoing numeric id, matching the spec.
+- **Slow MCP child shutdown no longer pauses the tokio executor.** The stdio launcher and the timeout-recovery path bound the kill+wait to about 200 ms with non-blocking polls; a server stuck on uninterruptible IO is left to the OS instead of holding up the runtime.
+
+### Changed
+
+- **MCP stdio servers spawn on a background worker.** Process creation used to happen on the tokio executor thread, which could pause the UI on a slow filesystem or NFS mount; it now runs on the blocking-task pool, so the rest of the session stays responsive while a server starts up.
+
 - **A global deny rule survives a local allow with the same name.** Adding `Bash(rm)` to `.sofos/config.local.toml`'s allow list used to silently strip the matching `Bash(rm)` from `~/.sofos/config.toml`'s deny list; both entries now coexist after merge, so the per-command verdict reflects every configured rule instead of dropping the global guarantee.
 - **`PATH=`, `LD_PRELOAD=`, `LD_LIBRARY_PATH=`, `DYLD_*=`, `NODE_PATH=`, and `PYTHONPATH=` prefixes now route the bash call to a confirmation prompt.** A command like `PATH=. cargo build` used to auto-allow as `cargo`; sofos now asks the user before running anything that swaps the binary the shell will execute, even when the base command is on the allow list or when blanket `Bash` allow is active. Built-in forbidden bases (`rm`, `chmod`, `sudo`, …) still take precedence and stay denied.
 - **A session-scoped Bash path grant now applies only to the file the user named.** Allowing `cat /home/me/.ssh/config` once used to permit every other file under `/home/me/.ssh` for the rest of the session; the grant is now scoped to the specific file, so a follow-up `cat /home/me/.ssh/id_rsa` re-prompts. Grants saved to config (yes-and-remember) still cover the whole `parent/**` directory because the user explicitly opts in to that wider scope.

src/mcp/protocol.rs

@@ -23,6 +23,19 @@ impl From<u64> for Id {
     }
 }
 
+impl Id {
+    /// True when this id matches a request id sofos sent. Sofos always
+    /// sends numeric ids, but the spec lets the server echo them as
+    /// strings (`"1"` instead of `1`), so the comparison accepts both
+    /// shapes for the same logical value.
+    pub fn matches_outgoing(&self, outgoing: u64) -> bool {
+        match self {
+            Id::Number(n) => *n == outgoing,
+            Id::String(s) => s == &outgoing.to_string(),
+        }
+    }
+}
+
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct JsonRpcRequest {
     pub jsonrpc: String,

src/mcp/transport/http.rs

@@ -5,6 +5,7 @@ use crate::mcp::client::{
 };
 use crate::mcp::config::McpServerConfig;
 use crate::mcp::protocol::*;
+use futures::StreamExt;
 use serde_json::Value;
 use std::collections::HashMap;
 use std::sync::Arc;
@@ -17,6 +18,11 @@ use std::time::Duration;
 /// when the user just wants a quick "server unreachable" signal.
 const HTTP_CONNECT_TIMEOUT: Duration = Duration::from_secs(10);
 
+/// Cap on a single MCP HTTP response body. A hostile or buggy server
+/// can otherwise stream multi-GB JSON for a `tools/list` reply and
+/// OOM the host long before the request timeout fires.
+const MCP_HTTP_BODY_CAP: usize = 32 * 1024 * 1024;
+
 pub struct HttpClient {
     server_name: String,
     url: String,
@@ -40,9 +46,16 @@ impl HttpClient {
         // connect timeout is shorter than the overall ceiling so an
         // unreachable host fails fast instead of holding the full
         // request budget on DNS / TCP / TLS.
+        //
+        // Redirects are disabled outright. The default reqwest policy
+        // would follow up to 10 hops and forward custom headers like
+        // `Authorization: Bearer ...` across origins, leaking the
+        // bearer token to whatever host the server pointed at. A 3xx
+        // here surfaces as an explicit error instead.
         let client = reqwest::Client::builder()
             .timeout(MCP_REQUEST_TIMEOUT)
             .connect_timeout(HTTP_CONNECT_TIMEOUT)
+            .redirect(reqwest::redirect::Policy::none())
             .build()
             .map_err(|e| SofosError::McpError(format!("Failed to build MCP HTTP client: {}", e)))?;
 
@@ -119,12 +132,71 @@ impl HttpClient {
             ))
         })?;
 
-        let response_json: JsonRpcResponse = response.json().await.map_err(|e| {
+        let status = response.status();
+        if status.is_redirection() {
+            return Err(SofosError::McpError(format!(
+                "MCP server '{}' returned redirect status {}; refused to follow \
+                 to keep the configured bearer token from leaking cross-origin",
+                self.server_name, status
+            )));
+        }
+
+        if let Some(announced) = response.content_length() {
+            if announced as usize > MCP_HTTP_BODY_CAP {
+                return Err(SofosError::McpError(format!(
+                    "MCP server '{}' announced a {} byte response, exceeds {} MB cap",
+                    self.server_name,
+                    announced,
+                    MCP_HTTP_BODY_CAP / (1024 * 1024)
+                )));
+            }
+        }
+
+        let mut buffered: Vec<u8> = Vec::with_capacity(8 * 1024);
+        let mut stream = response.bytes_stream();
+        while let Some(chunk) = stream.next().await {
+            let chunk = chunk.map_err(|e| {
+                SofosError::McpError(format!(
+                    "Failed to read response body from MCP server '{}': {}",
+                    self.server_name, e
+                ))
+            })?;
+            if buffered.len().saturating_add(chunk.len()) > MCP_HTTP_BODY_CAP {
+                return Err(SofosError::McpError(format!(
+                    "MCP server '{}' exceeded the {} MB response cap mid-stream",
+                    self.server_name,
+                    MCP_HTTP_BODY_CAP / (1024 * 1024)
+                )));
+            }
+            buffered.extend_from_slice(&chunk);
+        }
+
+        let raw: Value = serde_json::from_slice(&buffered).map_err(|e| {
             SofosError::McpError(format!(
                 "Failed to parse response from MCP server '{}': {}",
                 self.server_name, e
             ))
         })?;
+        if raw.get("method").is_some() {
+            return Err(SofosError::McpError(format!(
+                "MCP server '{}' sent a server-initiated message; sofos does not \
+                 implement the server-to-client side of the spec",
+                self.server_name
+            )));
+        }
+
+        let response_json: JsonRpcResponse = serde_json::from_value(raw).map_err(|e| {
+            SofosError::McpError(format!(
+                "Failed to parse response envelope from MCP server '{}': {}",
+                self.server_name, e
+            ))
+        })?;
+        if !response_json.id.matches_outgoing(id) {
+            return Err(SofosError::McpError(format!(
+                "MCP server '{}' returned response with id {:?}, expected {}",
+                self.server_name, response_json.id, id
+            )));
+        }
 
         if let Some(error) = response_json.error {
             return Err(SofosError::McpError(format!(