Fix Windows paths, harden tilde expansion, cap MCP images

alexylon · alexylon · commit fd2cb0ba32f0 · 2026-04-21T20:12:45.000+03:00
diff --git a/src/tools/bashexec.rs b/src/tools/bashexec.rs
@@ -1,6 +1,8 @@
 use crate::error::{Result, SofosError};
 use crate::tools::permissions::{CommandPermission, PermissionManager};
-use crate::tools::utils::{MAX_TOOL_OUTPUT_TOKENS, TruncationKind, truncate_for_context};
+use crate::tools::utils::{
+    MAX_TOOL_OUTPUT_TOKENS, TruncationKind, is_absolute_path, truncate_for_context,
+};
 use std::collections::HashSet;
 use std::path::PathBuf;
 use std::process::Command;
@@ -334,11 +336,18 @@ impl BashExecutor {
                 cleaned
             };
 
-            if path_candidate.starts_with('/') {
-                self.check_bash_external_path(path_candidate, permission_manager)?;
-            } else if path_candidate.starts_with("~/") || path_candidate == "~" {
+            // Check tilde before absolute so `~` / `~/foo` get expanded
+            // first. `is_absolute_path` catches Unix (`/foo`) and
+            // Windows (`C:\foo`, `\\server\share`) shapes on every
+            // platform — `Path::is_absolute` alone would miss Unix
+            // paths on Windows, letting a bash command referencing
+            // `/etc/passwd` bypass the external-path prompt when the
+            // binary runs there.
+            if path_candidate.starts_with("~/") || path_candidate == "~" {
                 let expanded = PermissionManager::expand_tilde_pub(path_candidate);
                 self.check_bash_external_path(&expanded, permission_manager)?;
+            } else if is_absolute_path(path_candidate) {
+                self.check_bash_external_path(path_candidate, permission_manager)?;
             }
         }
 
diff --git a/src/tools/codesearch.rs b/src/tools/codesearch.rs
@@ -179,9 +179,15 @@ impl CodeSearchTool {
         }
     }
 
-    /// List available file types supported by ripgrep
-    pub fn _list_file_types() -> Result<String> {
-        let output = Command::new("rg")
+    /// List available file types supported by ripgrep.
+    ///
+    /// Uses the resolved `rg_path` from the constructor so it honours
+    /// `SOFOS_RG_PATH` and the Homebrew / Linux fallback list. The
+    /// earlier implementation hard-coded `Command::new("rg")` which
+    /// silently fell back to PATH and broke wherever ripgrep wasn't
+    /// on PATH but had been found via the fallbacks.
+    pub fn _list_file_types(&self) -> Result<String> {
+        let output = Command::new(&self.rg_path)
             .arg("--type-list")
             .output()
             .map_err(|e| SofosError::ToolExecution(format!("Failed to list file types: {}", e)))?;
diff --git a/src/tools/filesystem.rs b/src/tools/filesystem.rs
@@ -1,5 +1,6 @@
 use crate::error::{Result, SofosError};
 use crate::error_ext::ResultExt;
+use crate::tools::utils::is_absolute_path;
 use std::fs;
 use std::path::{Path, PathBuf};
 
@@ -104,7 +105,12 @@ impl FileSystemTool {
     /// Validate and resolve a path relative to the workspace
     /// Returns an error if the path attempts to escape the workspace
     fn validate_path(&self, path: &str) -> Result<PathBuf> {
-        if path.starts_with('/') {
+        // `is_absolute_path` catches both Unix (`/foo`) and Windows
+        // (`C:\foo`, UNC `\\server\share`) shapes. Using
+        // `Path::is_absolute` directly would miss Unix-style paths
+        // when running on Windows — a regression the helper
+        // specifically guards against.
+        if is_absolute_path(path) {
             return Err(SofosError::PathViolation(
                 "Absolute paths are not allowed".to_string(),
             ));
diff --git a/src/tools/image.rs b/src/tools/image.rs
@@ -1,6 +1,7 @@
 use crate::error::{Result, SofosError};
 use crate::error_ext::ResultExt;
 use crate::tools::permissions::{CommandPermission, PermissionManager};
+use crate::tools::utils::is_absolute_or_tilde;
 use base64::{Engine, engine::general_purpose::STANDARD};
 use std::path::PathBuf;
 
@@ -119,7 +120,7 @@ impl ImageLoader {
     }
 
     pub fn load_local_image(&self, path: &str) -> Result<ImageSource> {
-        let full_path = if path.starts_with('/') || path.starts_with('~') {
+        let full_path = if is_absolute_or_tilde(path) {
             PathBuf::from(PermissionManager::expand_tilde_pub(path))
         } else {
             self.workspace.join(path)
diff --git a/src/tools/mod.rs b/src/tools/mod.rs
@@ -22,8 +22,9 @@ use tool_name::ToolName;
 
 use crate::tools::types::get_read_only_tools;
 use crate::tools::utils::{
-    MAX_DIFF_TOKENS, MAX_MCP_OUTPUT_TOKENS, MAX_PATH_LIST_TOKENS, MAX_TOOL_OUTPUT_TOKENS,
-    TruncationKind, confirm_destructive, truncate_for_context,
+    MAX_DIFF_TOKENS, MAX_MCP_IMAGE_BYTES, MAX_MCP_IMAGE_COUNT, MAX_MCP_OUTPUT_TOKENS,
+    MAX_PATH_LIST_TOKENS, MAX_TOOL_OUTPUT_TOKENS, TruncationKind, confirm_destructive,
+    is_absolute_or_tilde, truncate_for_context,
 };
 pub use types::{add_code_search_tool, get_all_tools, get_all_tools_with_morph};
 
@@ -111,6 +112,56 @@ pub struct ToolExecutor {
 const MORPH_STUB_ORIGINAL_MIN: usize = 500;
 const MORPH_STUB_FLOOR_BYTES: usize = 50;
 
+/// Apply both MCP-response caps (image count/bytes and text tokens) in
+/// the order the dispatcher needs. The drop note for images has to land
+/// AFTER text truncation so the model always sees it — otherwise a
+/// ~1 MB text response could push the note out past the truncation
+/// boundary and the model would silently lose the "images dropped"
+/// signal. The overage this adds to the text field is ~100 bytes,
+/// immaterial compared with the 10 MB API ceiling this cap is really
+/// protecting against. Factored out from the `execute` dispatcher so
+/// the ordering can be pinned in unit tests.
+fn cap_mcp_response(result: &mut McpToolResult) {
+    let dropped = cap_mcp_images(result);
+    result.text = truncate_for_context(
+        &result.text,
+        MAX_MCP_OUTPUT_TOKENS,
+        TruncationKind::McpOutput,
+    );
+    if dropped > 0 {
+        result.text.push_str(&format!(
+            "\n\n[{} image attachment(s) dropped: MCP image cap is {} images or ~{} MB total]",
+            dropped,
+            MAX_MCP_IMAGE_COUNT,
+            MAX_MCP_IMAGE_BYTES / (1024 * 1024)
+        ));
+    }
+}
+
+/// Drop image attachments from an MCP result until both the per-call
+/// count cap and the total-bytes cap are satisfied. Returns the number
+/// of images that were dropped. Walks the list in order and keeps each
+/// image that still fits under both caps — so a single oversized image
+/// in the middle of the response is skipped without blocking smaller
+/// images that come after it. Kept images retain their original order.
+fn cap_mcp_images(result: &mut McpToolResult) -> usize {
+    let original = result.images.len();
+    let mut kept = Vec::with_capacity(result.images.len().min(MAX_MCP_IMAGE_COUNT));
+    let mut total_bytes: usize = 0;
+    for img in std::mem::take(&mut result.images) {
+        let size = img.base64_data.len();
+        if kept.len() >= MAX_MCP_IMAGE_COUNT
+            || total_bytes.saturating_add(size) > MAX_MCP_IMAGE_BYTES
+        {
+            continue;
+        }
+        total_bytes += size;
+        kept.push(img);
+    }
+    result.images = kept;
+    original - result.images.len()
+}
+
 /// Ensure `path`'s parent directory exists, creating it (and any missing
 /// intermediates) if not. Used by move/copy when the destination is
 /// outside the workspace — inside-workspace writes go through
@@ -257,7 +308,7 @@ impl ToolExecutor {
     /// doesn't exist or canonicalize fails. Individual dispatchers can
     /// still customise the error via `.map_err(...)?`.
     fn resolve_existing(&self, caller_path: &str) -> Result<ResolvedPath> {
-        let full_path = if caller_path.starts_with('/') || caller_path.starts_with('~') {
+        let full_path = if is_absolute_or_tilde(caller_path) {
             std::path::PathBuf::from(permissions::PermissionManager::expand_tilde_pub(
                 caller_path,
             ))
@@ -283,7 +334,7 @@ impl ToolExecutor {
     /// lossy conversion if the parent is also missing (nested mkdir)
     /// or the path has no parent (filesystem root).
     fn resolve_for_write(&self, caller_path: &str) -> Result<ResolvedPath> {
-        let full_path = if caller_path.starts_with('/') || caller_path.starts_with('~') {
+        let full_path = if is_absolute_or_tilde(caller_path) {
             std::path::PathBuf::from(permissions::PermissionManager::expand_tilde_pub(
                 caller_path,
             ))
@@ -325,8 +376,9 @@ impl ToolExecutor {
 
         // Neither the target nor its parent exists. Relative paths are
         // still inside-workspace by convention; absolute / tilde paths
-        // are treated as outside and will hit the permission gate.
-        let is_inside_workspace = !caller_path.starts_with('/') && !caller_path.starts_with('~');
+        // (including Windows `C:\...`) are treated as outside and will
+        // hit the permission gate.
+        let is_inside_workspace = !is_absolute_or_tilde(caller_path);
         let canonical_str = full_path.to_string_lossy().to_string();
         Ok(ResolvedPath {
             canonical: full_path,
@@ -554,18 +606,7 @@ impl ToolExecutor {
         if let Some(mcp_manager) = &self.mcp_manager {
             if mcp_manager.is_mcp_tool(tool_name).await {
                 let mut result = mcp_manager.execute_tool(tool_name, input).await?;
-                // Cap the server-provided text before handing it to the
-                // model. The MCP server itself is a separate process, so
-                // this doesn't sandbox what it can read — but it keeps
-                // an overly-chatty or malicious server from reproducing
-                // the "string too long" HTTP 400 that oversized internal
-                // tool outputs used to trigger. Images pass through
-                // untouched.
-                result.text = truncate_for_context(
-                    &result.text,
-                    MAX_MCP_OUTPUT_TOKENS,
-                    TruncationKind::McpOutput,
-                );
+                cap_mcp_response(&mut result);
                 return Ok(ToolExecutionResult::Structured(result));
             }
         }
diff --git a/src/tools/permissions.rs b/src/tools/permissions.rs
@@ -1,5 +1,5 @@
 use crate::error::{Result, SofosError};
-use crate::tools::utils::{ConfirmationType, confirm_multi_choice};
+use crate::tools::utils::{ConfirmationType, confirm_multi_choice, is_absolute_path};
 use globset::{Glob, GlobSet, GlobSetBuilder};
 use serde::{Deserialize, Serialize};
 use std::collections::HashSet;
@@ -396,14 +396,20 @@ impl PermissionManager {
     }
 
     /// Extract path patterns from Bash() entries.
-    /// Only treats entries as path grants if the content starts with / or ~ AND contains a glob char.
-    /// Plain commands like Bash(npm test) are not path patterns.
+    /// Only treats entries as path grants if the content is absolute or
+    /// a tilde path AND contains a glob char. Plain commands like
+    /// Bash(npm test) are not path patterns. `is_absolute_path` handles
+    /// Unix (`/tmp/**`), Windows drive-letter (`C:\Users\**`), and UNC
+    /// paths on every platform — using `Path::is_absolute` alone would
+    /// mis-classify a Unix-style `Bash(/var/log/**)` config entry as a
+    /// command pattern when the binary runs on Windows.
     fn extract_bash_path_pattern(entry: &str) -> Option<&str> {
         let trimmed = entry.trim();
         if let Some(rest) = trimmed.strip_prefix("Bash(") {
             if let Some(end) = rest.rfind(')') {
                 let content = &rest[..end];
-                if (content.starts_with('/') || content.starts_with('~')) && content.contains('*') {
+                let looks_like_path = content.starts_with('~') || is_absolute_path(content);
+                if looks_like_path && content.contains('*') {
                     return Some(content);
                 }
             }
@@ -459,14 +465,49 @@ impl PermissionManager {
             .unwrap_or(without_prefix)
     }
 
+    /// Look up the user's home directory in a way that works on both
+    /// Unix (`$HOME`) and Windows (`%USERPROFILE%`). `std::env::home_dir`
+    /// was re-stabilised with a correct Windows implementation in Rust
+    /// 1.85, but reading the platform-native env var directly keeps us
+    /// compatible with older toolchains and makes the per-platform
+    /// choice explicit. Returns `None` when the env var is unset, which
+    /// is the same "fall through unexpanded" signal the caller uses.
+    fn home_dir() -> Option<PathBuf> {
+        #[cfg(windows)]
+        {
+            std::env::var_os("USERPROFILE").map(PathBuf::from)
+        }
+        #[cfg(not(windows))]
+        {
+            std::env::var_os("HOME").map(PathBuf::from)
+        }
+    }
+
+    /// Expand a leading `~` or `~/` to the user's home directory. Uses
+    /// `PathBuf::push` so the separator between the home directory and
+    /// the rest of the path is the platform's native one — the old
+    /// `format!("{}/{}", home, rest)` produced `C:\Users\alice/foo` on
+    /// Windows, which Windows accepts but looks wrong on inspection.
+    /// Paths not starting with `~` are returned unchanged.
+    ///
+    /// Strips leading separators from the remainder before pushing
+    /// because `PathBuf::push` *replaces* self when the argument is
+    /// absolute — so `expand_tilde("~//foo")` without the trim would
+    /// return `/foo` (escaped out of home) instead of the
+    /// bash-semantic `~/foo` = `home/foo`. Matters more on Windows
+    /// where a user-supplied `~/\\server\share\file` would be UNC-
+    /// absolute and would likewise replace the home prefix.
     fn expand_tilde(path: &str) -> String {
+        if path == "~" {
+            return Self::home_dir()
+                .map(|p| p.to_string_lossy().to_string())
+                .unwrap_or_else(|| path.to_string());
+        }
         if let Some(rest) = path.strip_prefix("~/") {
-            if let Ok(home) = std::env::var("HOME") {
-                return format!("{}/{}", home, rest);
-            }
-        } else if path == "~" {
-            if let Ok(home) = std::env::var("HOME") {
-                return home;
+            if let Some(mut home) = Self::home_dir() {
+                let rest = rest.trim_start_matches(['/', '\\']);
+                home.push(rest);
+                return home.to_string_lossy().to_string();
             }
         }
         path.to_string()
@@ -1186,6 +1227,67 @@ ask = []
         assert_eq!(not_tilde, "./file.txt");
     }
 
+    #[test]
+    fn test_tilde_expansion_trims_leading_separator_in_remainder() {
+        // `PathBuf::push` replaces self when the pushed argument is
+        // absolute. If the caller types `~//foo` (double slash after
+        // the tilde), the remainder after `~/` starts with `/`, which
+        // `push` would treat as absolute and escape the home
+        // directory entirely — `~//foo` would resolve to `/foo`,
+        // which is not what bash would do. The trim keeps the
+        // expansion rooted at the home directory.
+        let _lock = HOME_MUTEX.lock().unwrap();
+        let _temp_dir = TempDir::new().unwrap();
+
+        let original_home = std::env::var_os("HOME");
+        std::env::set_var("HOME", "/home/testuser");
+
+        let single = PermissionManager::expand_tilde_pub("~/foo");
+        let double = PermissionManager::expand_tilde_pub("~//foo");
+        let triple = PermissionManager::expand_tilde_pub("~///foo");
+
+        match original_home {
+            Some(home) => std::env::set_var("HOME", home),
+            None => std::env::remove_var("HOME"),
+        }
+
+        assert_eq!(single, "/home/testuser/foo");
+        assert_eq!(
+            double, "/home/testuser/foo",
+            "double-slash after tilde must not escape home"
+        );
+        assert_eq!(
+            triple, "/home/testuser/foo",
+            "any number of leading slashes must not escape home"
+        );
+    }
+
+    #[cfg(windows)]
+    #[test]
+    fn test_tilde_expansion_uses_userprofile_on_windows() {
+        // Regression: `expand_tilde` used to read `$HOME`, which isn't
+        // the canonical home-directory env var on Windows. A user
+        // typing `~/docs` got no expansion. The fix reads
+        // `%USERPROFILE%` on Windows and joins via `PathBuf::push` so
+        // the resulting separator is native (backslash on Windows).
+        let _lock = HOME_MUTEX.lock().unwrap();
+        let _temp_dir = TempDir::new().unwrap();
+
+        let original = std::env::var_os("USERPROFILE");
+        std::env::set_var("USERPROFILE", r"C:\Users\testuser");
+
+        let expanded = PermissionManager::expand_tilde_pub("~/docs/file.txt");
+        let expanded_dir = PermissionManager::expand_tilde_pub("~");
+
+        match original {
+            Some(home) => std::env::set_var("USERPROFILE", home),
+            None => std::env::remove_var("USERPROFILE"),
+        }
+
+        assert_eq!(expanded, r"C:\Users\testuser\docs\file.txt");
+        assert_eq!(expanded_dir, r"C:\Users\testuser");
+    }
+
     #[test]
     fn test_tilde_in_allow_rules() {
         let _lock = HOME_MUTEX.lock().unwrap();
diff --git a/src/tools/tests.rs b/src/tools/tests.rs
diff --git a/src/tools/utils.rs b/src/tools/utils.rs
