From a606f308f55632f1876b40bcfc14b874c81ba4da Mon Sep 17 00:00:00 2001 From: Argenis Date: Fri, 13 Mar 2026 16:15:30 -0400 Subject: [PATCH] fix(security): respect allowed_roots in tool-level path pre-checks (#3434) When workspace_only=true and allowed_roots is configured, several tools (file_read, content_search, glob_search) rejected absolute paths before the allowed_roots allowlist was consulted. Additionally, tilde paths (~/...) passed is_path_allowed but were then incorrectly joined with workspace_dir as literal relative paths. Changes: - Add SecurityPolicy::resolve_tool_path() to properly expand tilde paths and handle absolute vs relative path resolution for tools - Add SecurityPolicy::is_under_allowed_root() for tool pre-checks to consult the allowed_roots allowlist before rejecting absolute paths - Update file_read to use resolve_tool_path instead of workspace_dir.join - Update content_search and glob_search absolute-path pre-checks to allow paths under allowed_roots - Add tests covering workspace_only + allowed_roots scenarios Closes #3082 --- src/security/policy.rs | 87 +++++++++++++++++++++++++++++++++++++ src/tools/content_search.rs | 9 ++-- src/tools/file_read.rs | 48 +++++++++++++++++++- src/tools/glob_search.rs | 17 +++++--- 4 files changed, 152 insertions(+), 9 deletions(-) diff --git a/src/security/policy.rs b/src/security/policy.rs index 79b0d21a4..04cd68a6c 100644 --- a/src/security/policy.rs +++ b/src/security/policy.rs @@ -1061,6 +1061,35 @@ impl SecurityPolicy { self.tracker.count() >= self.max_actions_per_hour as usize } + /// Resolve a user-provided path for tool use. + /// + /// Expands `~` prefixes and resolves relative paths against the workspace + /// directory. This should be called **after** `is_path_allowed` to obtain + /// the filesystem path that the tool actually operates on. + pub fn resolve_tool_path(&self, path: &str) -> PathBuf { + let expanded = expand_user_path(path); + if expanded.is_absolute() { + expanded + } else { + self.workspace_dir.join(expanded) + } + } + + /// Check whether the given raw path (before canonicalization) falls under + /// an `allowed_roots` entry. Tilde expansion is applied to the path + /// before comparison. This is useful for tool-level pre-checks that want + /// to allow absolute paths that are explicitly permitted by policy. + pub fn is_under_allowed_root(&self, path: &str) -> bool { + let expanded = expand_user_path(path); + if !expanded.is_absolute() { + return false; + } + self.allowed_roots.iter().any(|root| { + let canonical = root.canonicalize().unwrap_or_else(|_| root.clone()); + expanded.starts_with(&canonical) || expanded.starts_with(root) + }) + } + /// Build from config sections pub fn from_config( autonomy_config: &crate::config::AutonomyConfig, @@ -2385,4 +2414,62 @@ mod tests { "URL-encoded parent dir traversal must be blocked" ); } + + #[test] + fn resolve_tool_path_expands_tilde() { + let p = SecurityPolicy { + workspace_dir: PathBuf::from("/workspace"), + ..SecurityPolicy::default() + }; + let resolved = p.resolve_tool_path("~/Documents/file.txt"); + // Should expand ~ to home dir, not join with workspace + assert!(resolved.is_absolute()); + assert!(!resolved.starts_with("/workspace")); + assert!(resolved.to_string_lossy().ends_with("Documents/file.txt")); + } + + #[test] + fn resolve_tool_path_keeps_absolute() { + let p = SecurityPolicy { + workspace_dir: PathBuf::from("/workspace"), + ..SecurityPolicy::default() + }; + let resolved = p.resolve_tool_path("/some/absolute/path"); + assert_eq!(resolved, PathBuf::from("/some/absolute/path")); + } + + #[test] + fn resolve_tool_path_joins_relative() { + let p = SecurityPolicy { + workspace_dir: PathBuf::from("/workspace"), + ..SecurityPolicy::default() + }; + let resolved = p.resolve_tool_path("relative/path.txt"); + assert_eq!(resolved, PathBuf::from("/workspace/relative/path.txt")); + } + + #[test] + fn is_under_allowed_root_matches_allowed_roots() { + let p = SecurityPolicy { + workspace_dir: PathBuf::from("/workspace"), + workspace_only: true, + allowed_roots: vec![PathBuf::from("/projects"), PathBuf::from("/data")], + ..SecurityPolicy::default() + }; + assert!(p.is_under_allowed_root("/projects/myapp/src/main.rs")); + assert!(p.is_under_allowed_root("/data/file.csv")); + assert!(!p.is_under_allowed_root("/etc/passwd")); + assert!(!p.is_under_allowed_root("relative/path")); + } + + #[test] + fn is_under_allowed_root_returns_false_for_empty_roots() { + let p = SecurityPolicy { + workspace_dir: PathBuf::from("/workspace"), + workspace_only: true, + allowed_roots: vec![], + ..SecurityPolicy::default() + }; + assert!(!p.is_under_allowed_root("/any/path")); + } } diff --git a/src/tools/content_search.rs b/src/tools/content_search.rs index 08a8ad428..87476f62d 100644 --- a/src/tools/content_search.rs +++ b/src/tools/content_search.rs @@ -171,7 +171,10 @@ impl Tool for ContentSearchTool { } // --- Path security checks --- - if std::path::Path::new(search_path).is_absolute() { + // Reject absolute paths unless they fall under an explicit allowed root. + if std::path::Path::new(search_path).is_absolute() + && !self.security.is_under_allowed_root(search_path) + { return Ok(ToolResult { success: false, output: String::new(), @@ -207,8 +210,7 @@ impl Tool for ContentSearchTool { } // --- Resolve search directory --- - let workspace = &self.security.workspace_dir; - let resolved_path = workspace.join(search_path); + let resolved_path = self.security.resolve_tool_path(search_path); let resolved_canon = match std::fs::canonicalize(&resolved_path) { Ok(p) => p, @@ -314,6 +316,7 @@ impl Tool for ContentSearchTool { let raw_stdout = String::from_utf8_lossy(&output.stdout); // --- Parse and format output --- + let workspace = &self.security.workspace_dir; let workspace_canon = std::fs::canonicalize(workspace).unwrap_or_else(|_| workspace.clone()); diff --git a/src/tools/file_read.rs b/src/tools/file_read.rs index 3d7c03e0e..8528ccfd9 100644 --- a/src/tools/file_read.rs +++ b/src/tools/file_read.rs @@ -82,7 +82,7 @@ impl Tool for FileReadTool { }); } - let full_path = self.security.workspace_dir.join(path); + let full_path = self.security.resolve_tool_path(path); // Resolve path before reading to block symlink escapes. let resolved_path = match tokio::fs::canonicalize(&full_path).await { @@ -1034,4 +1034,50 @@ mod tests { let _ = tokio::fs::remove_dir_all(&dir).await; } + + #[tokio::test] + async fn file_read_allowed_root_with_workspace_only() { + let root = std::env::temp_dir().join("zeroclaw_test_file_read_allowed_root"); + let workspace = root.join("workspace"); + let allowed = root.join("allowed_dir"); + + let _ = tokio::fs::remove_dir_all(&root).await; + tokio::fs::create_dir_all(&workspace).await.unwrap(); + tokio::fs::create_dir_all(&allowed).await.unwrap(); + tokio::fs::write(allowed.join("data.txt"), "allowed content") + .await + .unwrap(); + + let security = Arc::new(SecurityPolicy { + autonomy: AutonomyLevel::Supervised, + workspace_dir: workspace.clone(), + workspace_only: true, + allowed_roots: vec![allowed.clone()], + ..SecurityPolicy::default() + }); + let tool = FileReadTool::new(security); + + // Absolute path under allowed_root should succeed + let abs_path = allowed.join("data.txt").to_string_lossy().to_string(); + let result = tool.execute(json!({"path": &abs_path})).await.unwrap(); + + assert!( + result.success, + "file_read with allowed_root path should succeed, error: {:?}", + result.error + ); + assert!(result.output.contains("allowed content")); + + // Path outside both workspace and allowed_roots should still fail + let outside = root.join("outside"); + tokio::fs::create_dir_all(&outside).await.unwrap(); + tokio::fs::write(outside.join("secret.txt"), "secret") + .await + .unwrap(); + let outside_path = outside.join("secret.txt").to_string_lossy().to_string(); + let result = tool.execute(json!({"path": &outside_path})).await.unwrap(); + assert!(!result.success); + + let _ = tokio::fs::remove_dir_all(&root).await; + } } diff --git a/src/tools/glob_search.rs b/src/tools/glob_search.rs index 179f3ccc1..015c18592 100644 --- a/src/tools/glob_search.rs +++ b/src/tools/glob_search.rs @@ -57,8 +57,10 @@ impl Tool for GlobSearchTool { }); } - // Security: reject absolute paths - if pattern.starts_with('/') || pattern.starts_with('\\') { + // Security: reject absolute paths unless under an explicit allowed root. + if (pattern.starts_with('/') || pattern.starts_with('\\')) + && !self.security.is_under_allowed_root(pattern) + { return Ok(ToolResult { success: false, output: String::new(), @@ -84,9 +86,13 @@ impl Tool for GlobSearchTool { }); } - // Build full pattern anchored to workspace - let workspace = &self.security.workspace_dir; - let full_pattern = workspace.join(pattern).to_string_lossy().to_string(); + // Build full pattern: use resolve_tool_path to handle tilde expansion + // and absolute paths correctly. + let full_pattern = self + .security + .resolve_tool_path(pattern) + .to_string_lossy() + .to_string(); let entries = match glob::glob(&full_pattern) { Ok(paths) => paths, @@ -99,6 +105,7 @@ impl Tool for GlobSearchTool { } }; + let workspace = &self.security.workspace_dir; let workspace_canon = match std::fs::canonicalize(workspace) { Ok(p) => p, Err(e) => {