diff --git a/crates/chat-cli/src/cli/chat/tools/fs_read.rs b/crates/chat-cli/src/cli/chat/tools/fs_read.rs index 5c5c0abf2..0c92656bc 100644 --- a/crates/chat-cli/src/cli/chat/tools/fs_read.rs +++ b/crates/chat-cli/src/cli/chat/tools/fs_read.rs @@ -181,7 +181,8 @@ impl FsRead { FsReadOperation::Line(FsLine { path, .. }) | FsReadOperation::Directory(FsDirectory { path, .. }) | FsReadOperation::Search(FsSearch { path, .. }) => { - let Ok(path) = directories::canonicalizes_path(os, path) else { + // always use absolute file path for permission check + let Ok(path) = directories::canonicalizes_path_absolute(os, path) else { ask = true; continue; }; @@ -208,7 +209,8 @@ impl FsRead { let denied_match_set = paths .iter() .flat_map(|path| { - let Ok(path) = directories::canonicalizes_path(os, path) else { + // always use absolute file path for permission check + let Ok(path) = directories::canonicalizes_path_absolute(os, path) else { return vec![]; }; deny_set.matches(path.as_ref() as &str) @@ -1488,6 +1490,8 @@ mod tests { "operations": [ { "path": "/home/user/", "mode": "Directory" }, { "path": "/home/user/file.txt", "mode": "Line" }, + { "path": "/home/user/folder", "mode": "Directory" }, + { "path": "/home/user/folder/file.txt", "mode": "Line" }, ] })) .unwrap(); @@ -1537,4 +1541,52 @@ mod tests { let res = outside_tool.eval_perm(&os, &agent); assert!(matches!(res, PermissionEvalResult::Ask)); } + + #[tokio::test] + async fn test_fs_read_relative_to_cwd() { + use std::fs; + use tempfile::TempDir; + + let temp_dir = TempDir::new().unwrap(); + let temp_path = temp_dir.path(); + + // Create test files and directories + let test_file = temp_path.join("file.txt"); + let test_folder = temp_path.join("folder"); + let test_folder2 = temp_path.join("folder-2"); + let test_file2 = test_folder2.join("nested.txt"); + fs::write(&test_file, "test content").unwrap(); + fs::create_dir(&test_folder).unwrap(); + fs::create_dir(&test_folder2).unwrap(); + fs::write(&test_file2, "nested content").unwrap(); + + let os = Os::new().await.unwrap(); + let agent = Agent { + name: "test_agent".to_string(), + tools_settings: HashMap::new(), // No fs_read settings + ..Default::default() + }; + + // Save original CWD and change to temp directory + let original_cwd = std::env::current_dir().unwrap(); + std::env::set_current_dir(temp_path).unwrap(); + + // all of these files are under CWD through relative path. reading them should be allowed by default + let fs_read = serde_json::from_value::(serde_json::json!({ + "operations": [ + { "path": "./file.txt", "mode": "Line" }, + { "path": "./folder-2/nested.txt", "mode": "Line" }, + { "path": "./folder/../folder-2/nested.txt", "mode": "Line" }, + { "path": "./folder", "mode": "Directory" }, + { "path": "./folder-2/", "mode": "Directory" }, + { "path": "./folder/../folder-2", "mode": "Directory" }, + ] + })).unwrap(); + + let res = fs_read.eval_perm(&os, &agent); + assert!(matches!(res, PermissionEvalResult::Allow)); + + // Restore original CWD + std::env::set_current_dir(original_cwd).unwrap(); + } } diff --git a/crates/chat-cli/src/util/directories.rs b/crates/chat-cli/src/util/directories.rs index 89c6f3bc4..1ed519069 100644 --- a/crates/chat-cli/src/util/directories.rs +++ b/crates/chat-cli/src/util/directories.rs @@ -1,7 +1,6 @@ use std::env::VarError; use std::path::{ - PathBuf, - StripPrefixError, + Path, PathBuf, StripPrefixError }; use globset::{ @@ -181,6 +180,7 @@ pub fn chat_local_agent_dir(os: &Os) -> Result { } /// Canonicalizes path given by expanding the path given +/// Works for glob pattern pub fn canonicalizes_path(os: &Os, path_as_str: &str) -> Result { let context = |input: &str| Ok(os.env.get(input).ok()); let home_dir = || os.env.home().map(|p| p.to_string_lossy().to_string()); @@ -188,6 +188,21 @@ pub fn canonicalizes_path(os: &Os, path_as_str: &str) -> Result { Ok(shellexpand::full_with_context(path_as_str, home_dir, context)?.to_string()) } +/// Canonicalizes path and converts to absolute path. +/// This method requires that the file exist and doesn't support glob pattern. +pub fn canonicalizes_path_absolute(os: &Os, path_as_str: &str) -> Result { + let expanded = canonicalizes_path(os, path_as_str)?; + + if Path::new(&expanded).is_relative() { + // This handles relative path: "./folder" and "../file" and "./folder-1/../folder-2" and turn them into absolute path + // This also resolves symlinks + let absolute_path = std::fs::canonicalize(&expanded)?; + Ok(absolute_path.to_string_lossy().to_string()) + } else { + Ok(expanded) + } +} + /// Given a globset builder and a path, build globs for both the file and directory patterns /// This is needed because by default glob does not match children of a dir so we need both /// patterns to exist in a globset. @@ -469,4 +484,54 @@ mod tests { let result = canonicalizes_path(&test_os, "**/middle/**/path").unwrap(); assert_eq!(result, "**/middle/**/path"); } + + #[tokio::test] + async fn test_canonicalizes_path_absolute() { + use std::fs; + use tempfile::TempDir; + + let temp_dir = TempDir::new().unwrap(); + let temp_path = temp_dir.path(); + + // Create test files and directories + let test_file = temp_path.join("file"); + let test_folder = temp_path.join("folder"); + let test_folder2 = temp_path.join("folder-2"); + fs::write(&test_file, "test content").unwrap(); + fs::create_dir(&test_folder).unwrap(); + fs::create_dir(&test_folder2).unwrap(); + + let test_os = Os::new().await.unwrap(); + + // Save original CWD and change to temp directory + let original_cwd = std::env::current_dir().unwrap(); + std::env::set_current_dir(temp_path).unwrap(); + + // absolute path should remain unchanged + let result: String = canonicalizes_path_absolute(&test_os, &test_file.to_string_lossy()).unwrap(); + assert_eq!(result, test_file.to_string_lossy()); + + // "./" should resolve to temp directory + let result = canonicalizes_path_absolute(&test_os, "./").unwrap(); + assert_eq!(result, temp_path.canonicalize().unwrap().to_string_lossy().trim_end_matches('/')); + + // "./file" should resolve to file + let result = canonicalizes_path_absolute(&test_os, "./file").unwrap(); + assert_eq!(result, test_file.canonicalize().unwrap().to_string_lossy()); + + // "./folder/" should resolve to folder/ + let result = canonicalizes_path_absolute(&test_os, "./folder/").unwrap(); + assert_eq!(result, test_folder.canonicalize().unwrap().to_string_lossy()); + + // "./folder" should also resolve to folder/ + let result = canonicalizes_path_absolute(&test_os, "./folder").unwrap(); + assert_eq!(result, test_folder.canonicalize().unwrap().to_string_lossy()); + + // "./folder/../folder-2" should resolve to folder-2/ + let result = canonicalizes_path_absolute(&test_os, "./folder/../folder-2").unwrap(); + assert_eq!(result, test_folder2.canonicalize().unwrap().to_string_lossy()); + + // Restore original CWD + std::env::set_current_dir(original_cwd).unwrap(); + } }