diff --git a/crates/agent/src/tests/mod.rs b/crates/agent/src/tests/mod.rs index 6434ba09a872a4674d53450606834a5b2923436b..07f3447903c05afb3eb8cc1308eaf951fe524d02 100644 --- a/crates/agent/src/tests/mod.rs +++ b/crates/agent/src/tests/mod.rs @@ -1115,6 +1115,27 @@ fn test_permission_options_without_pattern() { assert!(!labels.iter().any(|label| label.contains("commands"))); } +#[test] +fn test_permission_options_symlink_target_are_flat_once_only() { + let permission_options = + ToolPermissionContext::symlink_target(EditFileTool::NAME, vec!["/outside/file.txt".into()]) + .build_permission_options(); + + let PermissionOptions::Flat(options) = permission_options else { + panic!("Expected flat permission options for symlink target authorization"); + }; + + assert_eq!(options.len(), 2); + assert!(options.iter().any(|option| { + option.option_id.0.as_ref() == "allow" + && option.kind == acp::PermissionOptionKind::AllowOnce + })); + assert!(options.iter().any(|option| { + option.option_id.0.as_ref() == "deny" + && option.kind == acp::PermissionOptionKind::RejectOnce + })); +} + #[test] fn test_permission_option_ids_for_terminal() { let permission_options = ToolPermissionContext::new( diff --git a/crates/agent/src/tests/test_tools.rs b/crates/agent/src/tests/test_tools.rs index ed304a3bb6c4ea9b9eea75fdcfd245e265714080..dc48ae1719157e7db11c5d2812139d570bd0d166 100644 --- a/crates/agent/src/tests/test_tools.rs +++ b/crates/agent/src/tests/test_tools.rs @@ -128,10 +128,10 @@ impl AgentTool for ToolRequiringPermission { return Task::ready(Err(anyhow::anyhow!("{}", reason))); } ToolPermissionDecision::Confirm => { - let context = crate::ToolPermissionContext { - tool_name: "tool_requiring_permission".to_string(), - input_values: vec![String::new()], - }; + let context = crate::ToolPermissionContext::new( + "tool_requiring_permission", + vec![String::new()], + ); Some(event_stream.authorize("Authorize?", context, cx)) } }; diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index 0a14a19e739abc4be5ac40f5da2ee663c19fbece..5e16ec826d1ff56c7e185344a1431cc5d8cafe12 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -629,6 +629,13 @@ pub struct NewTerminal { pub struct ToolPermissionContext { pub tool_name: String, pub input_values: Vec, + pub scope: ToolPermissionScope, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ToolPermissionScope { + ToolInput, + SymlinkTarget, } impl ToolPermissionContext { @@ -636,6 +643,15 @@ impl ToolPermissionContext { Self { tool_name: tool_name.into(), input_values, + scope: ToolPermissionScope::ToolInput, + } + } + + pub fn symlink_target(tool_name: impl Into, target_paths: Vec) -> Self { + Self { + tool_name: tool_name.into(), + input_values: target_paths, + scope: ToolPermissionScope::SymlinkTarget, } } @@ -669,6 +685,20 @@ impl ToolPermissionContext { let tool_name = &self.tool_name; let input_values = &self.input_values; + if self.scope == ToolPermissionScope::SymlinkTarget { + return acp_thread::PermissionOptions::Flat(vec![ + acp::PermissionOption::new( + acp::PermissionOptionId::new("allow"), + "Yes", + acp::PermissionOptionKind::AllowOnce, + ), + acp::PermissionOption::new( + acp::PermissionOptionId::new("deny"), + "No", + acp::PermissionOptionKind::RejectOnce, + ), + ]); + } // Check if the user's shell supports POSIX-like command chaining. // See the doc comment above for the full explanation of why this is needed. diff --git a/crates/agent/src/tools.rs b/crates/agent/src/tools.rs index 000a394d910c6b6ae4b68b5377dbabb7c1da21f1..2ffc187e953f928db9c9ee3bad9efffd8e74cecc 100644 --- a/crates/agent/src/tools.rs +++ b/crates/agent/src/tools.rs @@ -17,6 +17,7 @@ mod save_file_tool; mod streaming_edit_file_tool; mod subagent_tool; mod terminal_tool; +mod tool_permissions; mod web_search_tool; use crate::AgentTool; @@ -41,6 +42,7 @@ pub use save_file_tool::*; pub use streaming_edit_file_tool::*; pub use subagent_tool::*; pub use terminal_tool::*; +pub use tool_permissions::*; pub use web_search_tool::*; macro_rules! tools { diff --git a/crates/agent/src/tools/copy_path_tool.rs b/crates/agent/src/tools/copy_path_tool.rs index a8af9515fd28dcd121d04900cef1f800442ff53f..22da12b2e2a33577e50e6d0804f5580ff4e2b56c 100644 --- a/crates/agent/src/tools/copy_path_tool.rs +++ b/crates/agent/src/tools/copy_path_tool.rs @@ -1,5 +1,6 @@ -use super::edit_file_tool::{ - SensitiveSettingsKind, is_sensitive_settings_path, sensitive_settings_kind, +use super::tool_permissions::{ + SensitiveSettingsKind, authorize_symlink_escapes, canonicalize_worktree_roots, + collect_symlink_escapes, sensitive_settings_kind, }; use crate::{AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_for_paths}; use agent_client_protocol::ToolKind; @@ -84,40 +85,67 @@ impl AgentTool for CopyPathTool { cx: &mut App, ) -> Task> { let settings = AgentSettings::get_global(cx); - let paths = vec![input.source_path.clone(), input.destination_path.clone()]; let decision = decide_permission_for_paths(Self::NAME, &paths, settings); if let ToolPermissionDecision::Deny(reason) = decision { return Task::ready(Err(anyhow!("{}", reason))); } - let needs_confirmation = matches!(decision, ToolPermissionDecision::Confirm) - || (matches!(decision, ToolPermissionDecision::Allow) - && (is_sensitive_settings_path(Path::new(&input.source_path)) - || is_sensitive_settings_path(Path::new(&input.destination_path)))); + let project = self.project.clone(); + cx.spawn(async move |cx| { + let fs = project.read_with(cx, |project, _cx| project.fs().clone()); + let canonical_roots = canonicalize_worktree_roots(&project, &fs, cx).await; - let authorize = if needs_confirmation { - let src = MarkdownInlineCode(&input.source_path); - let dest = MarkdownInlineCode(&input.destination_path); - let context = crate::ToolPermissionContext { - tool_name: Self::NAME.to_string(), - input_values: vec![input.source_path.clone(), input.destination_path.clone()], - }; - let title = format!("Copy {src} to {dest}"); - let sensitive_kind = sensitive_settings_kind(Path::new(&input.source_path)) - .or_else(|| sensitive_settings_kind(Path::new(&input.destination_path))); - let title = match sensitive_kind { - Some(SensitiveSettingsKind::Local) => format!("{title} (local settings)"), - Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), - None => title, + let symlink_escapes: Vec<(&str, std::path::PathBuf)> = + project.read_with(cx, |project, cx| { + collect_symlink_escapes( + project, + &input.source_path, + &input.destination_path, + &canonical_roots, + cx, + ) + }); + + let sensitive_kind = + sensitive_settings_kind(Path::new(&input.source_path), fs.as_ref()) + .await + .or( + sensitive_settings_kind(Path::new(&input.destination_path), fs.as_ref()) + .await, + ); + + let needs_confirmation = matches!(decision, ToolPermissionDecision::Confirm) + || (matches!(decision, ToolPermissionDecision::Allow) && sensitive_kind.is_some()); + + let authorize = if !symlink_escapes.is_empty() { + // Symlink escape authorization replaces (rather than supplements) + // the normal tool-permission prompt. The symlink prompt already + // requires explicit user approval with the canonical target shown, + // which is strictly more security-relevant than a generic confirm. + Some(cx.update(|cx| { + authorize_symlink_escapes(Self::NAME, &symlink_escapes, &event_stream, cx) + })) + } else if needs_confirmation { + Some(cx.update(|cx| { + let src = MarkdownInlineCode(&input.source_path); + let dest = MarkdownInlineCode(&input.destination_path); + let context = crate::ToolPermissionContext::new( + Self::NAME, + vec![input.source_path.clone(), input.destination_path.clone()], + ); + let title = format!("Copy {src} to {dest}"); + let title = match sensitive_kind { + Some(SensitiveSettingsKind::Local) => format!("{title} (local settings)"), + Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), + None => title, + }; + event_stream.authorize(title, context, cx) + })) + } else { + None }; - Some(event_stream.authorize(title, context, cx)) - } else { - None - }; - let project = self.project.clone(); - cx.spawn(async move |cx| { if let Some(authorize) = authorize { authorize.await?; } @@ -160,3 +188,258 @@ impl AgentTool for CopyPathTool { }) } } + +#[cfg(test)] +mod tests { + use super::*; + use agent_client_protocol as acp; + use fs::Fs as _; + use gpui::TestAppContext; + use project::{FakeFs, Project}; + use serde_json::json; + use settings::SettingsStore; + use std::path::PathBuf; + use util::path; + + fn init_test(cx: &mut TestAppContext) { + cx.update(|cx| { + let settings_store = SettingsStore::test(cx); + cx.set_global(settings_store); + }); + cx.update(|cx| { + let mut settings = AgentSettings::get_global(cx).clone(); + settings.tool_permissions.default = settings::ToolPermissionMode::Allow; + AgentSettings::override_global(settings, cx); + }); + } + + #[gpui::test] + async fn test_copy_path_symlink_escape_source_requests_authorization(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { "file.txt": "content" } + }, + "external": { + "secret.txt": "SECRET" + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link_to_external").as_ref(), + PathBuf::from("../external"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(CopyPathTool::new(project)); + + let input = CopyPathToolInput { + source_path: "project/link_to_external".into(), + destination_path: "project/external_copy".into(), + }; + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| tool.run(input, event_stream, cx)); + + let auth = event_rx.expect_authorization().await; + let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); + assert!( + title.contains("points outside the project") + || title.contains("symlinks outside project"), + "Authorization title should mention symlink escape, got: {title}", + ); + + auth.response + .send(acp::PermissionOptionId::new("allow")) + .unwrap(); + + let result = task.await; + assert!(result.is_ok(), "should succeed after approval: {result:?}"); + } + + #[gpui::test] + async fn test_copy_path_symlink_escape_denied(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { "file.txt": "content" } + }, + "external": { + "secret.txt": "SECRET" + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link_to_external").as_ref(), + PathBuf::from("../external"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(CopyPathTool::new(project)); + + let input = CopyPathToolInput { + source_path: "project/link_to_external".into(), + destination_path: "project/external_copy".into(), + }; + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| tool.run(input, event_stream, cx)); + + let auth = event_rx.expect_authorization().await; + drop(auth); + + let result = task.await; + assert!(result.is_err(), "should fail when denied"); + } + + #[gpui::test] + async fn test_copy_path_symlink_escape_confirm_requires_single_approval( + cx: &mut TestAppContext, + ) { + init_test(cx); + cx.update(|cx| { + let mut settings = AgentSettings::get_global(cx).clone(); + settings.tool_permissions.default = settings::ToolPermissionMode::Confirm; + AgentSettings::override_global(settings, cx); + }); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { "file.txt": "content" } + }, + "external": { + "secret.txt": "SECRET" + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link_to_external").as_ref(), + PathBuf::from("../external"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(CopyPathTool::new(project)); + + let input = CopyPathToolInput { + source_path: "project/link_to_external".into(), + destination_path: "project/external_copy".into(), + }; + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| tool.run(input, event_stream, cx)); + + let auth = event_rx.expect_authorization().await; + let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); + assert!( + title.contains("points outside the project") + || title.contains("symlinks outside project"), + "Authorization title should mention symlink escape, got: {title}", + ); + + auth.response + .send(acp::PermissionOptionId::new("allow")) + .unwrap(); + + assert!( + !matches!( + event_rx.try_next(), + Ok(Some(Ok(crate::ThreadEvent::ToolCallAuthorization(_)))) + ), + "Expected a single authorization prompt", + ); + + let result = task.await; + assert!( + result.is_ok(), + "Tool should succeed after one authorization: {result:?}" + ); + } + + #[gpui::test] + async fn test_copy_path_symlink_escape_honors_deny_policy(cx: &mut TestAppContext) { + init_test(cx); + cx.update(|cx| { + let mut settings = AgentSettings::get_global(cx).clone(); + settings.tool_permissions.tools.insert( + "copy_path".into(), + agent_settings::ToolRules { + default: Some(settings::ToolPermissionMode::Deny), + ..Default::default() + }, + ); + AgentSettings::override_global(settings, cx); + }); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { "file.txt": "content" } + }, + "external": { + "secret.txt": "SECRET" + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link_to_external").as_ref(), + PathBuf::from("../external"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(CopyPathTool::new(project)); + + let input = CopyPathToolInput { + source_path: "project/link_to_external".into(), + destination_path: "project/external_copy".into(), + }; + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let result = cx.update(|cx| tool.run(input, event_stream, cx)).await; + + assert!(result.is_err(), "Tool should fail when policy denies"); + assert!( + !matches!( + event_rx.try_next(), + Ok(Some(Ok(crate::ThreadEvent::ToolCallAuthorization(_)))) + ), + "Deny policy should not emit symlink authorization prompt", + ); + } +} diff --git a/crates/agent/src/tools/create_directory_tool.rs b/crates/agent/src/tools/create_directory_tool.rs index 43c3829bdd835cc08ec2ad2314c94c562b149505..d19df105fa9053e1ba3be048db799bcd737bcb43 100644 --- a/crates/agent/src/tools/create_directory_tool.rs +++ b/crates/agent/src/tools/create_directory_tool.rs @@ -1,4 +1,7 @@ -use super::edit_file_tool::{SensitiveSettingsKind, sensitive_settings_kind}; +use super::tool_permissions::{ + SensitiveSettingsKind, authorize_symlink_access, canonicalize_worktree_roots, + detect_symlink_escape, sensitive_settings_kind, +}; use agent_client_protocol::ToolKind; use agent_settings::AgentSettings; use anyhow::{Context as _, Result, anyhow}; @@ -8,11 +11,11 @@ use project::Project; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use settings::Settings; -use std::path::Path; use std::sync::Arc; use util::markdown::MarkdownInlineCode; use crate::{AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_for_path}; +use std::path::Path; /// Creates a new directory at the specified path within the project. Returns confirmation that the directory was created. /// @@ -71,37 +74,67 @@ impl AgentTool for CreateDirectoryTool { cx: &mut App, ) -> Task> { let settings = AgentSettings::get_global(cx); - let mut decision = decide_permission_for_path(Self::NAME, &input.path, settings); - let sensitive_kind = sensitive_settings_kind(Path::new(&input.path)); + let decision = decide_permission_for_path(Self::NAME, &input.path, settings); - if matches!(decision, ToolPermissionDecision::Allow) && sensitive_kind.is_some() { - decision = ToolPermissionDecision::Confirm; + if let ToolPermissionDecision::Deny(reason) = decision { + return Task::ready(Err(anyhow!("{}", reason))); } - let authorize = match decision { - ToolPermissionDecision::Allow => None, - ToolPermissionDecision::Deny(reason) => { - return Task::ready(Err(anyhow!("{}", reason))); - } - ToolPermissionDecision::Confirm => { - let title = format!("Create directory {}", MarkdownInlineCode(&input.path)); - let title = match &sensitive_kind { - Some(SensitiveSettingsKind::Local) => format!("{title} (local settings)"), - Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), - None => title, - }; - let context = crate::ToolPermissionContext { - tool_name: Self::NAME.to_string(), - input_values: vec![input.path.clone()], - }; - Some(event_stream.authorize(title, context, cx)) - } - }; - let destination_path: Arc = input.path.as_str().into(); let project = self.project.clone(); cx.spawn(async move |cx| { + let fs = project.read_with(cx, |project, _cx| project.fs().clone()); + let canonical_roots = canonicalize_worktree_roots(&project, &fs, cx).await; + + let symlink_escape_target = project.read_with(cx, |project, cx| { + detect_symlink_escape(project, &input.path, &canonical_roots, cx) + .map(|(_, target)| target) + }); + + let sensitive_kind = sensitive_settings_kind(Path::new(&input.path), fs.as_ref()).await; + + let decision = + if matches!(decision, ToolPermissionDecision::Allow) && sensitive_kind.is_some() { + ToolPermissionDecision::Confirm + } else { + decision + }; + + let authorize = if let Some(canonical_target) = symlink_escape_target { + // Symlink escape authorization replaces (rather than supplements) + // the normal tool-permission prompt. The symlink prompt already + // requires explicit user approval with the canonical target shown, + // which is strictly more security-relevant than a generic confirm. + Some(cx.update(|cx| { + authorize_symlink_access( + Self::NAME, + &input.path, + &canonical_target, + &event_stream, + cx, + ) + })) + } else { + match decision { + ToolPermissionDecision::Allow => None, + ToolPermissionDecision::Confirm => Some(cx.update(|cx| { + let title = format!("Create directory {}", MarkdownInlineCode(&input.path)); + let title = match &sensitive_kind { + Some(SensitiveSettingsKind::Local) => { + format!("{title} (local settings)") + } + Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), + None => title, + }; + let context = + crate::ToolPermissionContext::new(Self::NAME, vec![input.path.clone()]); + event_stream.authorize(title, context, cx) + })), + ToolPermissionDecision::Deny(_) => None, + } + }; + if let Some(authorize) = authorize { authorize.await?; } @@ -126,3 +159,279 @@ impl AgentTool for CreateDirectoryTool { }) } } + +#[cfg(test)] +mod tests { + use super::*; + use agent_client_protocol as acp; + use fs::Fs as _; + use gpui::TestAppContext; + use project::{FakeFs, Project}; + use serde_json::json; + use settings::SettingsStore; + use std::path::PathBuf; + use util::path; + + use crate::ToolCallEventStream; + + fn init_test(cx: &mut TestAppContext) { + cx.update(|cx| { + let settings_store = SettingsStore::test(cx); + cx.set_global(settings_store); + }); + cx.update(|cx| { + let mut settings = AgentSettings::get_global(cx).clone(); + settings.tool_permissions.default = settings::ToolPermissionMode::Allow; + AgentSettings::override_global(settings, cx); + }); + } + + #[gpui::test] + async fn test_create_directory_symlink_escape_requests_authorization(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { "main.rs": "fn main() {}" } + }, + "external": { + "data": { "file.txt": "content" } + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link_to_external").as_ref(), + PathBuf::from("../external"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(CreateDirectoryTool::new(project)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + tool.run( + CreateDirectoryToolInput { + path: "project/link_to_external".into(), + }, + event_stream, + cx, + ) + }); + + let auth = event_rx.expect_authorization().await; + let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); + assert!( + title.contains("points outside the project") || title.contains("symlink"), + "Authorization title should mention symlink escape, got: {title}", + ); + + auth.response + .send(acp::PermissionOptionId::new("allow")) + .unwrap(); + + let result = task.await; + assert!( + result.is_ok(), + "Tool should succeed after authorization: {result:?}" + ); + } + + #[gpui::test] + async fn test_create_directory_symlink_escape_denied(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { "main.rs": "fn main() {}" } + }, + "external": { + "data": { "file.txt": "content" } + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link_to_external").as_ref(), + PathBuf::from("../external"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(CreateDirectoryTool::new(project)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + tool.run( + CreateDirectoryToolInput { + path: "project/link_to_external".into(), + }, + event_stream, + cx, + ) + }); + + let auth = event_rx.expect_authorization().await; + + drop(auth); + + let result = task.await; + assert!( + result.is_err(), + "Tool should fail when authorization is denied" + ); + } + + #[gpui::test] + async fn test_create_directory_symlink_escape_confirm_requires_single_approval( + cx: &mut TestAppContext, + ) { + init_test(cx); + cx.update(|cx| { + let mut settings = AgentSettings::get_global(cx).clone(); + settings.tool_permissions.default = settings::ToolPermissionMode::Confirm; + AgentSettings::override_global(settings, cx); + }); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { "main.rs": "fn main() {}" } + }, + "external": { + "data": { "file.txt": "content" } + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link_to_external").as_ref(), + PathBuf::from("../external"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(CreateDirectoryTool::new(project)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + tool.run( + CreateDirectoryToolInput { + path: "project/link_to_external".into(), + }, + event_stream, + cx, + ) + }); + + let auth = event_rx.expect_authorization().await; + let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); + assert!( + title.contains("points outside the project") || title.contains("symlink"), + "Authorization title should mention symlink escape, got: {title}", + ); + + auth.response + .send(acp::PermissionOptionId::new("allow")) + .unwrap(); + + assert!( + !matches!( + event_rx.try_next(), + Ok(Some(Ok(crate::ThreadEvent::ToolCallAuthorization(_)))) + ), + "Expected a single authorization prompt", + ); + + let result = task.await; + assert!( + result.is_ok(), + "Tool should succeed after one authorization: {result:?}" + ); + } + + #[gpui::test] + async fn test_create_directory_symlink_escape_honors_deny_policy(cx: &mut TestAppContext) { + init_test(cx); + cx.update(|cx| { + let mut settings = AgentSettings::get_global(cx).clone(); + settings.tool_permissions.tools.insert( + "create_directory".into(), + agent_settings::ToolRules { + default: Some(settings::ToolPermissionMode::Deny), + ..Default::default() + }, + ); + AgentSettings::override_global(settings, cx); + }); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { "main.rs": "fn main() {}" } + }, + "external": { + "data": { "file.txt": "content" } + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link_to_external").as_ref(), + PathBuf::from("../external"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(CreateDirectoryTool::new(project)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let result = cx + .update(|cx| { + tool.run( + CreateDirectoryToolInput { + path: "project/link_to_external".into(), + }, + event_stream, + cx, + ) + }) + .await; + + assert!(result.is_err(), "Tool should fail when policy denies"); + assert!( + !matches!( + event_rx.try_next(), + Ok(Some(Ok(crate::ThreadEvent::ToolCallAuthorization(_)))) + ), + "Deny policy should not emit symlink authorization prompt", + ); + } +} diff --git a/crates/agent/src/tools/delete_path_tool.rs b/crates/agent/src/tools/delete_path_tool.rs index 41e060933fd4aa3ad57b719736b5ea9097663313..bcbb841fb7d0f1a416be17c04fac9542838ede1c 100644 --- a/crates/agent/src/tools/delete_path_tool.rs +++ b/crates/agent/src/tools/delete_path_tool.rs @@ -1,5 +1,6 @@ -use super::edit_file_tool::{ - SensitiveSettingsKind, is_sensitive_settings_path, sensitive_settings_kind, +use super::tool_permissions::{ + SensitiveSettingsKind, authorize_symlink_access, canonicalize_worktree_roots, + detect_symlink_escape, sensitive_settings_kind, }; use crate::{AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_for_path}; use action_log::ActionLog; @@ -78,37 +79,66 @@ impl AgentTool for DeletePathTool { let path = input.path; let settings = AgentSettings::get_global(cx); - let mut decision = decide_permission_for_path(Self::NAME, &path, settings); + let decision = decide_permission_for_path(Self::NAME, &path, settings); - if matches!(decision, ToolPermissionDecision::Allow) - && is_sensitive_settings_path(Path::new(&path)) - { - decision = ToolPermissionDecision::Confirm; + if let ToolPermissionDecision::Deny(reason) = decision { + return Task::ready(Err(anyhow!("{}", reason))); } - let authorize = match decision { - ToolPermissionDecision::Allow => None, - ToolPermissionDecision::Deny(reason) => { - return Task::ready(Err(anyhow!("{}", reason))); - } - ToolPermissionDecision::Confirm => { - let context = crate::ToolPermissionContext { - tool_name: Self::NAME.to_string(), - input_values: vec![path.clone()], - }; - let title = format!("Delete {}", MarkdownInlineCode(&path)); - let title = match sensitive_settings_kind(Path::new(&path)) { - Some(SensitiveSettingsKind::Local) => format!("{title} (local settings)"), - Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), - None => title, - }; - Some(event_stream.authorize(title, context, cx)) - } - }; - let project = self.project.clone(); let action_log = self.action_log.clone(); cx.spawn(async move |cx| { + let fs = project.read_with(cx, |project, _cx| project.fs().clone()); + let canonical_roots = canonicalize_worktree_roots(&project, &fs, cx).await; + + let symlink_escape_target = project.read_with(cx, |project, cx| { + detect_symlink_escape(project, &path, &canonical_roots, cx) + .map(|(_, target)| target) + }); + + let settings_kind = sensitive_settings_kind(Path::new(&path), fs.as_ref()).await; + + let decision = + if matches!(decision, ToolPermissionDecision::Allow) && settings_kind.is_some() { + ToolPermissionDecision::Confirm + } else { + decision + }; + + let authorize = if let Some(canonical_target) = symlink_escape_target { + // Symlink escape authorization replaces (rather than supplements) + // the normal tool-permission prompt. The symlink prompt already + // requires explicit user approval with the canonical target shown, + // which is strictly more security-relevant than a generic confirm. + Some(cx.update(|cx| { + authorize_symlink_access( + Self::NAME, + &path, + &canonical_target, + &event_stream, + cx, + ) + })) + } else { + match decision { + ToolPermissionDecision::Allow => None, + ToolPermissionDecision::Confirm => Some(cx.update(|cx| { + let context = + crate::ToolPermissionContext::new(Self::NAME, vec![path.clone()]); + let title = format!("Delete {}", MarkdownInlineCode(&path)); + let title = match settings_kind { + Some(SensitiveSettingsKind::Local) => { + format!("{title} (local settings)") + } + Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), + None => title, + }; + event_stream.authorize(title, context, cx) + })), + ToolPermissionDecision::Deny(_) => None, + } + }; + if let Some(authorize) = authorize { authorize.await?; } @@ -188,3 +218,293 @@ impl AgentTool for DeletePathTool { }) } } + +#[cfg(test)] +mod tests { + use super::*; + use agent_client_protocol as acp; + use fs::Fs as _; + use gpui::TestAppContext; + use project::{FakeFs, Project}; + use serde_json::json; + use settings::SettingsStore; + use std::path::PathBuf; + use util::path; + + use crate::ToolCallEventStream; + + fn init_test(cx: &mut TestAppContext) { + cx.update(|cx| { + let settings_store = SettingsStore::test(cx); + cx.set_global(settings_store); + }); + cx.update(|cx| { + let mut settings = AgentSettings::get_global(cx).clone(); + settings.tool_permissions.default = settings::ToolPermissionMode::Allow; + AgentSettings::override_global(settings, cx); + }); + } + + #[gpui::test] + async fn test_delete_path_symlink_escape_requests_authorization(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { "main.rs": "fn main() {}" } + }, + "external": { + "data": { "file.txt": "content" } + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link_to_external").as_ref(), + PathBuf::from("../external"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let action_log = cx.new(|_| ActionLog::new(project.clone())); + let tool = Arc::new(DeletePathTool::new(project, action_log)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + tool.run( + DeletePathToolInput { + path: "project/link_to_external".into(), + }, + event_stream, + cx, + ) + }); + + let auth = event_rx.expect_authorization().await; + let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); + assert!( + title.contains("points outside the project") || title.contains("symlink"), + "Authorization title should mention symlink escape, got: {title}", + ); + + auth.response + .send(acp::PermissionOptionId::new("allow")) + .unwrap(); + + let result = task.await; + // FakeFs cannot delete symlink entries (they are neither Dir nor File + // internally), so the deletion itself may fail. The important thing is + // that the authorization was requested and accepted — any error must + // come from the fs layer, not from a permission denial. + if let Err(err) = &result { + let msg = format!("{err:#}"); + assert!( + !msg.contains("denied") && !msg.contains("authorization"), + "Error should not be a permission denial, got: {msg}", + ); + } + } + + #[gpui::test] + async fn test_delete_path_symlink_escape_denied(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { "main.rs": "fn main() {}" } + }, + "external": { + "data": { "file.txt": "content" } + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link_to_external").as_ref(), + PathBuf::from("../external"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let action_log = cx.new(|_| ActionLog::new(project.clone())); + let tool = Arc::new(DeletePathTool::new(project, action_log)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + tool.run( + DeletePathToolInput { + path: "project/link_to_external".into(), + }, + event_stream, + cx, + ) + }); + + let auth = event_rx.expect_authorization().await; + + drop(auth); + + let result = task.await; + assert!( + result.is_err(), + "Tool should fail when authorization is denied" + ); + } + + #[gpui::test] + async fn test_delete_path_symlink_escape_confirm_requires_single_approval( + cx: &mut TestAppContext, + ) { + init_test(cx); + cx.update(|cx| { + let mut settings = AgentSettings::get_global(cx).clone(); + settings.tool_permissions.default = settings::ToolPermissionMode::Confirm; + AgentSettings::override_global(settings, cx); + }); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { "main.rs": "fn main() {}" } + }, + "external": { + "data": { "file.txt": "content" } + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link_to_external").as_ref(), + PathBuf::from("../external"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let action_log = cx.new(|_| ActionLog::new(project.clone())); + let tool = Arc::new(DeletePathTool::new(project, action_log)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + tool.run( + DeletePathToolInput { + path: "project/link_to_external".into(), + }, + event_stream, + cx, + ) + }); + + let auth = event_rx.expect_authorization().await; + let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); + assert!( + title.contains("points outside the project") || title.contains("symlink"), + "Authorization title should mention symlink escape, got: {title}", + ); + + auth.response + .send(acp::PermissionOptionId::new("allow")) + .unwrap(); + + assert!( + !matches!( + event_rx.try_next(), + Ok(Some(Ok(crate::ThreadEvent::ToolCallAuthorization(_)))) + ), + "Expected a single authorization prompt", + ); + + let result = task.await; + if let Err(err) = &result { + let message = format!("{err:#}"); + assert!( + !message.contains("denied") && !message.contains("authorization"), + "Error should not be a permission denial, got: {message}", + ); + } + } + + #[gpui::test] + async fn test_delete_path_symlink_escape_honors_deny_policy(cx: &mut TestAppContext) { + init_test(cx); + cx.update(|cx| { + let mut settings = AgentSettings::get_global(cx).clone(); + settings.tool_permissions.tools.insert( + "delete_path".into(), + agent_settings::ToolRules { + default: Some(settings::ToolPermissionMode::Deny), + ..Default::default() + }, + ); + AgentSettings::override_global(settings, cx); + }); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { "main.rs": "fn main() {}" } + }, + "external": { + "data": { "file.txt": "content" } + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link_to_external").as_ref(), + PathBuf::from("../external"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let action_log = cx.new(|_| ActionLog::new(project.clone())); + let tool = Arc::new(DeletePathTool::new(project, action_log)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let result = cx + .update(|cx| { + tool.run( + DeletePathToolInput { + path: "project/link_to_external".into(), + }, + event_stream, + cx, + ) + }) + .await; + + assert!(result.is_err(), "Tool should fail when policy denies"); + assert!( + !matches!( + event_rx.try_next(), + Ok(Some(Ok(crate::ThreadEvent::ToolCallAuthorization(_)))) + ), + "Deny policy should not emit symlink authorization prompt", + ); + } +} diff --git a/crates/agent/src/tools/edit_file_tool.rs b/crates/agent/src/tools/edit_file_tool.rs index ef3b3cc30a54fd6eb3c9e07c3bce4ea7b194ca47..697ab3022312f10d53f46df9c874554d2d16aa5e 100644 --- a/crates/agent/src/tools/edit_file_tool.rs +++ b/crates/agent/src/tools/edit_file_tool.rs @@ -1,8 +1,8 @@ use super::restore_file_from_disk_tool::RestoreFileFromDiskTool; use super::save_file_tool::SaveFileTool; +use super::tool_permissions::authorize_file_edit; use crate::{ - AgentTool, Templates, Thread, ToolCallEventStream, ToolPermissionDecision, - decide_permission_for_path, + AgentTool, Templates, Thread, ToolCallEventStream, edit_agent::{EditAgent, EditAgentOutput, EditAgentOutputEvent, EditFormat}, }; use acp_thread::Diff; @@ -16,14 +16,11 @@ use indoc::formatdoc; use language::language_settings::{self, FormatOnSave}; use language::{LanguageRegistry, ToPoint}; use language_model::LanguageModelToolResultContent; -use paths; use project::lsp_store::{FormatTrigger, LspFormatTarget}; use project::{Project, ProjectPath}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use settings::Settings; -use std::ffi::OsStr; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use std::sync::Arc; use ui::SharedString; use util::ResultExt; @@ -163,144 +160,6 @@ impl EditFileTool { } } -pub enum SensitiveSettingsKind { - Local, - Global, -} - -/// Canonicalize a path, stripping the Windows extended-length path prefix (`\\?\`) -/// that `std::fs::canonicalize` adds on Windows. This ensures that canonicalized -/// paths can be compared with non-canonicalized paths via `starts_with`. -fn safe_canonicalize(path: &Path) -> std::io::Result { - let canonical = std::fs::canonicalize(path)?; - #[cfg(target_os = "windows")] - { - let s = canonical.to_string_lossy(); - if let Some(stripped) = s.strip_prefix("\\\\?\\") { - return Ok(PathBuf::from(stripped)); - } - } - Ok(canonical) -} - -/// Returns the kind of sensitive settings location this path targets, if any: -/// either inside a `.zed/` local-settings directory or inside the global config dir. -pub fn sensitive_settings_kind(path: &Path) -> Option { - let local_settings_folder = paths::local_settings_folder_name(); - if path.components().any(|component| { - component.as_os_str() == <_ as AsRef>::as_ref(&local_settings_folder) - }) { - return Some(SensitiveSettingsKind::Local); - } - - // Walk up the path hierarchy until we find an ancestor that exists and can - // be canonicalized, then reconstruct the path from there. This handles - // cases where multiple levels of subdirectories don't exist yet (e.g. - // ~/.config/zed/new_subdir/evil.json). - let canonical_path = { - let mut current: Option<&Path> = Some(path); - let mut suffix_components = Vec::new(); - loop { - match current { - Some(ancestor) => match safe_canonicalize(ancestor) { - Ok(canonical) => { - let mut result = canonical; - for component in suffix_components.into_iter().rev() { - result.push(component); - } - break Some(result); - } - Err(_) => { - if let Some(file_name) = ancestor.file_name() { - suffix_components.push(file_name.to_os_string()); - } - current = ancestor.parent(); - } - }, - None => break None, - } - } - }; - if let Some(canonical_path) = canonical_path { - let config_dir = safe_canonicalize(paths::config_dir()) - .unwrap_or_else(|_| paths::config_dir().to_path_buf()); - if canonical_path.starts_with(&config_dir) { - return Some(SensitiveSettingsKind::Global); - } - } - - None -} - -pub fn is_sensitive_settings_path(path: &Path) -> bool { - sensitive_settings_kind(path).is_some() -} - -pub fn authorize_file_edit( - tool_name: &str, - path: &Path, - display_description: &str, - thread: &WeakEntity, - event_stream: &ToolCallEventStream, - cx: &mut App, -) -> Task> { - let path_str = path.to_string_lossy(); - let settings = agent_settings::AgentSettings::get_global(cx); - let decision = decide_permission_for_path(tool_name, &path_str, settings); - - if let ToolPermissionDecision::Deny(reason) = decision { - return Task::ready(Err(anyhow!("{}", reason))); - } - - let explicitly_allowed = matches!(decision, ToolPermissionDecision::Allow); - - if explicitly_allowed && !is_sensitive_settings_path(path) { - return Task::ready(Ok(())); - } - - match sensitive_settings_kind(path) { - Some(SensitiveSettingsKind::Local) => { - let context = crate::ToolPermissionContext { - tool_name: tool_name.to_string(), - input_values: vec![path_str.to_string()], - }; - return event_stream.authorize( - format!("{} (local settings)", display_description), - context, - cx, - ); - } - Some(SensitiveSettingsKind::Global) => { - let context = crate::ToolPermissionContext { - tool_name: tool_name.to_string(), - input_values: vec![path_str.to_string()], - }; - return event_stream.authorize( - format!("{} (settings)", display_description), - context, - cx, - ); - } - None => {} - } - - let Ok(project_path) = thread.read_with(cx, |thread, cx| { - thread.project().read(cx).find_project_path(path, cx) - }) else { - return Task::ready(Err(anyhow!("thread was dropped"))); - }; - - if project_path.is_some() { - Task::ready(Ok(())) - } else { - let context = crate::ToolPermissionContext { - tool_name: tool_name.to_string(), - input_values: vec![path_str.to_string()], - }; - event_stream.authorize(display_description, context, cx) - } -} - impl AgentTool for EditFileTool { type Input = EditFileToolInput; type Output = EditFileToolOutput; @@ -725,12 +584,14 @@ fn resolve_path( #[cfg(test)] mod tests { use super::*; + use crate::tools::tool_permissions::{SensitiveSettingsKind, sensitive_settings_kind}; use crate::{ContextServerRegistry, Templates}; - use fs::Fs; + use fs::Fs as _; use gpui::{TestAppContext, UpdateGlobal}; use language_model::fake_provider::FakeLanguageModel; use prompt_store::ProjectContext; use serde_json::json; + use settings::Settings; use settings::SettingsStore; use util::{path, rel_path::rel_path}; @@ -1391,6 +1252,302 @@ mod tests { assert_eq!(event.tool_call.fields.title, Some("test 5.4".into())); } + #[gpui::test] + async fn test_authorize_create_under_symlink_with_allow(cx: &mut TestAppContext) { + init_test(cx); + + let fs = project::FakeFs::new(cx.executor()); + fs.insert_tree("/root", json!({})).await; + fs.insert_tree("/outside", json!({})).await; + fs.insert_symlink("/root/link", PathBuf::from("/outside")) + .await; + + let project = Project::test(fs.clone(), [path!("/root").as_ref()], cx).await; + let context_server_registry = + cx.new(|cx| ContextServerRegistry::new(project.read(cx).context_server_store(), cx)); + let language_registry = project.read_with(cx, |project, _cx| project.languages().clone()); + let model = Arc::new(FakeLanguageModel::default()); + let thread = cx.new(|cx| { + Thread::new( + project.clone(), + cx.new(|_cx| ProjectContext::default()), + context_server_registry, + Templates::new(), + Some(model), + cx, + ) + }); + let tool = Arc::new(EditFileTool::new( + project, + thread.downgrade(), + language_registry, + Templates::new(), + )); + + cx.update(|cx| { + let mut settings = agent_settings::AgentSettings::get_global(cx).clone(); + settings.tool_permissions.default = settings::ToolPermissionMode::Allow; + agent_settings::AgentSettings::override_global(settings, cx); + }); + + let (stream_tx, mut stream_rx) = ToolCallEventStream::test(); + let authorize_task = cx.update(|cx| { + tool.authorize( + &EditFileToolInput { + display_description: "create through symlink".into(), + path: "link/new.txt".into(), + mode: EditFileMode::Create, + }, + &stream_tx, + cx, + ) + }); + + let event = stream_rx.expect_authorization().await; + assert!( + event + .tool_call + .fields + .title + .as_deref() + .is_some_and(|title| title.contains("points outside the project")), + "Expected symlink escape authorization for create under external symlink" + ); + + event + .response + .send(acp::PermissionOptionId::new("allow")) + .unwrap(); + authorize_task.await.unwrap(); + } + + #[gpui::test] + async fn test_edit_file_symlink_escape_requests_authorization(cx: &mut TestAppContext) { + init_test(cx); + + let fs = project::FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "src": { "main.rs": "fn main() {}" } + }), + ) + .await; + fs.insert_tree( + path!("/outside"), + json!({ + "config.txt": "old content" + }), + ) + .await; + fs.create_symlink( + path!("/root/link_to_external").as_ref(), + PathBuf::from("/outside"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let language_registry = project.read_with(cx, |project, _| project.languages().clone()); + let context_server_registry = + cx.new(|cx| ContextServerRegistry::new(project.read(cx).context_server_store(), cx)); + let model = Arc::new(FakeLanguageModel::default()); + let thread = cx.new(|cx| { + Thread::new( + project.clone(), + cx.new(|_cx| ProjectContext::default()), + context_server_registry, + Templates::new(), + Some(model), + cx, + ) + }); + let tool = Arc::new(EditFileTool::new( + project.clone(), + thread.downgrade(), + language_registry, + Templates::new(), + )); + + let (stream_tx, mut stream_rx) = ToolCallEventStream::test(); + let _authorize_task = cx.update(|cx| { + tool.authorize( + &EditFileToolInput { + display_description: "edit through symlink".into(), + path: PathBuf::from("link_to_external/config.txt"), + mode: EditFileMode::Edit, + }, + &stream_tx, + cx, + ) + }); + + let auth = stream_rx.expect_authorization().await; + let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); + assert!( + title.contains("points outside the project"), + "title should mention symlink escape, got: {title}" + ); + } + + #[gpui::test] + async fn test_edit_file_symlink_escape_denied(cx: &mut TestAppContext) { + init_test(cx); + + let fs = project::FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "src": { "main.rs": "fn main() {}" } + }), + ) + .await; + fs.insert_tree( + path!("/outside"), + json!({ + "config.txt": "old content" + }), + ) + .await; + fs.create_symlink( + path!("/root/link_to_external").as_ref(), + PathBuf::from("/outside"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let language_registry = project.read_with(cx, |project, _| project.languages().clone()); + let context_server_registry = + cx.new(|cx| ContextServerRegistry::new(project.read(cx).context_server_store(), cx)); + let model = Arc::new(FakeLanguageModel::default()); + let thread = cx.new(|cx| { + Thread::new( + project.clone(), + cx.new(|_cx| ProjectContext::default()), + context_server_registry, + Templates::new(), + Some(model), + cx, + ) + }); + let tool = Arc::new(EditFileTool::new( + project.clone(), + thread.downgrade(), + language_registry, + Templates::new(), + )); + + let (stream_tx, mut stream_rx) = ToolCallEventStream::test(); + let authorize_task = cx.update(|cx| { + tool.authorize( + &EditFileToolInput { + display_description: "edit through symlink".into(), + path: PathBuf::from("link_to_external/config.txt"), + mode: EditFileMode::Edit, + }, + &stream_tx, + cx, + ) + }); + + let auth = stream_rx.expect_authorization().await; + drop(auth); // deny by dropping + + let result = authorize_task.await; + assert!(result.is_err(), "should fail when denied"); + } + + #[gpui::test] + async fn test_edit_file_symlink_escape_honors_deny_policy(cx: &mut TestAppContext) { + init_test(cx); + cx.update(|cx| { + let mut settings = agent_settings::AgentSettings::get_global(cx).clone(); + settings.tool_permissions.tools.insert( + "edit_file".into(), + agent_settings::ToolRules { + default: Some(settings::ToolPermissionMode::Deny), + ..Default::default() + }, + ); + agent_settings::AgentSettings::override_global(settings, cx); + }); + + let fs = project::FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "src": { "main.rs": "fn main() {}" } + }), + ) + .await; + fs.insert_tree( + path!("/outside"), + json!({ + "config.txt": "old content" + }), + ) + .await; + fs.create_symlink( + path!("/root/link_to_external").as_ref(), + PathBuf::from("/outside"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let language_registry = project.read_with(cx, |project, _| project.languages().clone()); + let context_server_registry = + cx.new(|cx| ContextServerRegistry::new(project.read(cx).context_server_store(), cx)); + let model = Arc::new(FakeLanguageModel::default()); + let thread = cx.new(|cx| { + Thread::new( + project.clone(), + cx.new(|_cx| ProjectContext::default()), + context_server_registry, + Templates::new(), + Some(model), + cx, + ) + }); + let tool = Arc::new(EditFileTool::new( + project.clone(), + thread.downgrade(), + language_registry, + Templates::new(), + )); + + let (stream_tx, mut stream_rx) = ToolCallEventStream::test(); + let result = cx + .update(|cx| { + tool.authorize( + &EditFileToolInput { + display_description: "edit through symlink".into(), + path: PathBuf::from("link_to_external/config.txt"), + mode: EditFileMode::Edit, + }, + &stream_tx, + cx, + ) + }) + .await; + + assert!(result.is_err(), "Tool should fail when policy denies"); + assert!( + !matches!( + stream_rx.try_next(), + Ok(Some(Ok(crate::ThreadEvent::ToolCallAuthorization(_)))) + ), + "Deny policy should not emit symlink authorization prompt", + ); + } + #[gpui::test] async fn test_authorize_global_config(cx: &mut TestAppContext) { init_test(cx); @@ -2392,13 +2549,18 @@ mod tests { ); } - #[test] - fn test_sensitive_settings_kind_detects_nonexistent_subdirectory() { + #[gpui::test] + async fn test_sensitive_settings_kind_detects_nonexistent_subdirectory( + cx: &mut TestAppContext, + ) { + let fs = project::FakeFs::new(cx.executor()); let config_dir = paths::config_dir(); + fs.insert_tree(&*config_dir.to_string_lossy(), json!({})) + .await; let path = config_dir.join("nonexistent_subdir_xyz").join("evil.json"); assert!( matches!( - sensitive_settings_kind(&path), + sensitive_settings_kind(&path, fs.as_ref()).await, Some(SensitiveSettingsKind::Global) ), "Path in non-existent subdirectory of config dir should be detected as sensitive: {:?}", @@ -2406,13 +2568,18 @@ mod tests { ); } - #[test] - fn test_sensitive_settings_kind_detects_deeply_nested_nonexistent_subdirectory() { + #[gpui::test] + async fn test_sensitive_settings_kind_detects_deeply_nested_nonexistent_subdirectory( + cx: &mut TestAppContext, + ) { + let fs = project::FakeFs::new(cx.executor()); let config_dir = paths::config_dir(); + fs.insert_tree(&*config_dir.to_string_lossy(), json!({})) + .await; let path = config_dir.join("a").join("b").join("c").join("evil.json"); assert!( matches!( - sensitive_settings_kind(&path), + sensitive_settings_kind(&path, fs.as_ref()).await, Some(SensitiveSettingsKind::Global) ), "Path in deeply nested non-existent subdirectory of config dir should be detected as sensitive: {:?}", @@ -2420,11 +2587,14 @@ mod tests { ); } - #[test] - fn test_sensitive_settings_kind_returns_none_for_non_config_path() { + #[gpui::test] + async fn test_sensitive_settings_kind_returns_none_for_non_config_path( + cx: &mut TestAppContext, + ) { + let fs = project::FakeFs::new(cx.executor()); let path = PathBuf::from("/tmp/not_a_config_dir/some_file.json"); assert!( - sensitive_settings_kind(&path).is_none(), + sensitive_settings_kind(&path, fs.as_ref()).await.is_none(), "Path outside config dir should not be detected as sensitive: {:?}", path ); diff --git a/crates/agent/src/tools/fetch_tool.rs b/crates/agent/src/tools/fetch_tool.rs index 69263e3b765f75b2dfd1af94ff015afd36c4e279..5f65bcd6f1d12b6a7f3e7473f624c319068c80ce 100644 --- a/crates/agent/src/tools/fetch_tool.rs +++ b/crates/agent/src/tools/fetch_tool.rs @@ -155,10 +155,8 @@ impl AgentTool for FetchTool { return Task::ready(Err(anyhow::anyhow!("{}", reason))); } ToolPermissionDecision::Confirm => { - let context = crate::ToolPermissionContext { - tool_name: Self::NAME.to_string(), - input_values: vec![input.url.clone()], - }; + let context = + crate::ToolPermissionContext::new(Self::NAME, vec![input.url.clone()]); Some(event_stream.authorize( format!("Fetch {}", MarkdownInlineCode(&input.url)), context, diff --git a/crates/agent/src/tools/list_directory_tool.rs b/crates/agent/src/tools/list_directory_tool.rs index 4e642dba4f826d50ebcd0d04e7f99209d0be22c7..16cc61c4e5d8f3ea7155608dfb048f6f3f994d6c 100644 --- a/crates/agent/src/tools/list_directory_tool.rs +++ b/crates/agent/src/tools/list_directory_tool.rs @@ -1,6 +1,10 @@ +use super::tool_permissions::{ + ResolvedProjectPath, authorize_symlink_access, canonicalize_worktree_roots, + resolve_project_path, +}; use crate::{AgentTool, ToolCallEventStream}; use agent_client_protocol::ToolKind; -use anyhow::{Result, anyhow}; +use anyhow::{Context as _, Result, anyhow}; use gpui::{App, Entity, SharedString, Task}; use project::{Project, ProjectPath, WorktreeSettings}; use schemars::JsonSchema; @@ -45,6 +49,76 @@ impl ListDirectoryTool { pub fn new(project: Entity) -> Self { Self { project } } + + fn build_directory_output( + project: &Entity, + project_path: &ProjectPath, + input_path: &str, + cx: &App, + ) -> Result { + let worktree = project + .read(cx) + .worktree_for_id(project_path.worktree_id, cx) + .with_context(|| format!("{input_path} is not in a known worktree"))?; + + let global_settings = WorktreeSettings::get_global(cx); + let worktree_settings = WorktreeSettings::get(Some(project_path.into()), cx); + let worktree_snapshot = worktree.read(cx).snapshot(); + let worktree_root_name = worktree.read(cx).root_name(); + + let Some(entry) = worktree_snapshot.entry_for_path(&project_path.path) else { + return Err(anyhow!("Path not found: {}", input_path)); + }; + + if !entry.is_dir() { + return Err(anyhow!("{input_path} is not a directory.")); + } + + let mut folders = Vec::new(); + let mut files = Vec::new(); + + for entry in worktree_snapshot.child_entries(&project_path.path) { + // Skip private and excluded files and directories + if global_settings.is_path_private(&entry.path) + || global_settings.is_path_excluded(&entry.path) + { + continue; + } + + let project_path: ProjectPath = (worktree_snapshot.id(), entry.path.clone()).into(); + if worktree_settings.is_path_excluded(&project_path.path) + || worktree_settings.is_path_private(&project_path.path) + { + continue; + } + + let full_path = worktree_root_name + .join(&entry.path) + .display(worktree_snapshot.path_style()) + .into_owned(); + if entry.is_dir() { + folders.push(full_path); + } else { + files.push(full_path); + } + } + + let mut output = String::new(); + + if !folders.is_empty() { + writeln!(output, "# Folders:\n{}", folders.join("\n")).unwrap(); + } + + if !files.is_empty() { + writeln!(output, "\n# Files:\n{}", files.join("\n")).unwrap(); + } + + if output.is_empty() { + writeln!(output, "{input_path} is empty.").unwrap(); + } + + Ok(output) + } } impl AgentTool for ListDirectoryTool { @@ -73,7 +147,7 @@ impl AgentTool for ListDirectoryTool { fn run( self: Arc, input: Self::Input, - _event_stream: ToolCallEventStream, + event_stream: ToolCallEventStream, cx: &mut App, ) -> Task> { // Sometimes models will return these even though we tell it to give a path and not a glob. @@ -98,115 +172,104 @@ impl AgentTool for ListDirectoryTool { return Task::ready(Ok(output)); } - let Some(project_path) = self.project.read(cx).find_project_path(&input.path, cx) else { - return Task::ready(Err(anyhow!("Path {} not found in project", input.path))); - }; - let Some(worktree) = self - .project - .read(cx) - .worktree_for_id(project_path.worktree_id, cx) - else { - return Task::ready(Err(anyhow!("Worktree not found"))); - }; - - // Check if the directory whose contents we're listing is itself excluded or private - let global_settings = WorktreeSettings::get_global(cx); - if global_settings.is_path_excluded(&project_path.path) { - return Task::ready(Err(anyhow!( - "Cannot list directory because its path matches the user's global `file_scan_exclusions` setting: {}", - &input.path - ))); - } - - if global_settings.is_path_private(&project_path.path) { - return Task::ready(Err(anyhow!( - "Cannot list directory because its path matches the user's global `private_files` setting: {}", - &input.path - ))); - } - - let worktree_settings = WorktreeSettings::get(Some((&project_path).into()), cx); - if worktree_settings.is_path_excluded(&project_path.path) { - return Task::ready(Err(anyhow!( - "Cannot list directory because its path matches the user's worktree`file_scan_exclusions` setting: {}", - &input.path - ))); - } - - if worktree_settings.is_path_private(&project_path.path) { - return Task::ready(Err(anyhow!( - "Cannot list directory because its path matches the user's worktree `private_paths` setting: {}", - &input.path - ))); - } - - let worktree_snapshot = worktree.read(cx).snapshot(); - let worktree_root_name = worktree.read(cx).root_name(); - - let Some(entry) = worktree_snapshot.entry_for_path(&project_path.path) else { - return Task::ready(Err(anyhow!("Path not found: {}", input.path))); - }; + let project = self.project.clone(); + cx.spawn(async move |cx| { + let fs = project.read_with(cx, |project, _cx| project.fs().clone()); + let canonical_roots = canonicalize_worktree_roots(&project, &fs, cx).await; + + let (project_path, symlink_canonical_target) = + project.read_with(cx, |project, cx| -> anyhow::Result<_> { + let resolved = resolve_project_path(project, &input.path, &canonical_roots, cx)?; + Ok(match resolved { + ResolvedProjectPath::Safe(path) => (path, None), + ResolvedProjectPath::SymlinkEscape { + project_path, + canonical_target, + } => (project_path, Some(canonical_target)), + }) + })?; + + // Check settings exclusions synchronously + project.read_with(cx, |project, cx| { + let worktree = project + .worktree_for_id(project_path.worktree_id, cx) + .with_context(|| { + format!("{} is not in a known worktree", &input.path) + })?; + + let global_settings = WorktreeSettings::get_global(cx); + if global_settings.is_path_excluded(&project_path.path) { + anyhow::bail!( + "Cannot list directory because its path matches the user's global `file_scan_exclusions` setting: {}", + &input.path + ); + } - if !entry.is_dir() { - return Task::ready(Err(anyhow!("{} is not a directory.", input.path))); - } - let worktree_snapshot = worktree.read(cx).snapshot(); + if global_settings.is_path_private(&project_path.path) { + anyhow::bail!( + "Cannot list directory because its path matches the user's global `private_files` setting: {}", + &input.path + ); + } - let mut folders = Vec::new(); - let mut files = Vec::new(); + let worktree_settings = WorktreeSettings::get(Some((&project_path).into()), cx); + if worktree_settings.is_path_excluded(&project_path.path) { + anyhow::bail!( + "Cannot list directory because its path matches the user's worktree `file_scan_exclusions` setting: {}", + &input.path + ); + } - for entry in worktree_snapshot.child_entries(&project_path.path) { - // Skip private and excluded files and directories - if global_settings.is_path_private(&entry.path) - || global_settings.is_path_excluded(&entry.path) - { - continue; - } + if worktree_settings.is_path_private(&project_path.path) { + anyhow::bail!( + "Cannot list directory because its path matches the user's worktree `private_paths` setting: {}", + &input.path + ); + } - let project_path: ProjectPath = (worktree_snapshot.id(), entry.path.clone()).into(); - if worktree_settings.is_path_excluded(&project_path.path) - || worktree_settings.is_path_private(&project_path.path) - { - continue; - } + let worktree_snapshot = worktree.read(cx).snapshot(); + let Some(entry) = worktree_snapshot.entry_for_path(&project_path.path) else { + anyhow::bail!("Path not found: {}", input.path); + }; + if !entry.is_dir() { + anyhow::bail!("{} is not a directory.", input.path); + } - let full_path = worktree_root_name - .join(&entry.path) - .display(worktree_snapshot.path_style()) - .into_owned(); - if entry.is_dir() { - folders.push(full_path); - } else { - files.push(full_path); + anyhow::Ok(()) + })?; + + if let Some(canonical_target) = &symlink_canonical_target { + let authorize = cx.update(|cx| { + authorize_symlink_access( + Self::NAME, + &input.path, + canonical_target, + &event_stream, + cx, + ) + }); + authorize.await?; } - } - - let mut output = String::new(); - - if !folders.is_empty() { - writeln!(output, "# Folders:\n{}", folders.join("\n")).unwrap(); - } - - if !files.is_empty() { - writeln!(output, "\n# Files:\n{}", files.join("\n")).unwrap(); - } - if output.is_empty() { - writeln!(output, "{} is empty.", input.path).unwrap(); - } - - Task::ready(Ok(output)) + let list_path = input.path; + cx.update(|cx| { + Self::build_directory_output(&project, &project_path, &list_path, cx) + }) + }) } } #[cfg(test)] mod tests { use super::*; + use agent_client_protocol as acp; + use fs::Fs as _; use gpui::{TestAppContext, UpdateGlobal}; use indoc::indoc; use project::{FakeFs, Project}; use serde_json::json; use settings::SettingsStore; + use std::path::PathBuf; use util::path; fn platform_paths(path_str: &str) -> String { @@ -655,4 +718,305 @@ mod tests { .contains("Cannot list directory"), ); } + + #[gpui::test] + async fn test_list_directory_symlink_escape_requests_authorization(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { + "main.rs": "fn main() {}" + } + }, + "external": { + "secrets": { + "key.txt": "SECRET_KEY=abc123" + } + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link_to_external").as_ref(), + PathBuf::from("../external"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(ListDirectoryTool::new(project)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + tool.clone().run( + ListDirectoryToolInput { + path: "project/link_to_external".into(), + }, + event_stream, + cx, + ) + }); + + let auth = event_rx.expect_authorization().await; + let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); + assert!( + title.contains("points outside the project"), + "Authorization title should mention symlink escape, got: {title}", + ); + + auth.response + .send(acp::PermissionOptionId::new("allow")) + .unwrap(); + + let result = task.await; + assert!( + result.is_ok(), + "Tool should succeed after authorization: {result:?}" + ); + } + + #[gpui::test] + async fn test_list_directory_symlink_escape_denied(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { + "main.rs": "fn main() {}" + } + }, + "external": { + "secrets": {} + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link_to_external").as_ref(), + PathBuf::from("../external"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(ListDirectoryTool::new(project)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + tool.clone().run( + ListDirectoryToolInput { + path: "project/link_to_external".into(), + }, + event_stream, + cx, + ) + }); + + let auth = event_rx.expect_authorization().await; + + // Deny by dropping the response sender without sending + drop(auth); + + let result = task.await; + assert!( + result.is_err(), + "Tool should fail when authorization is denied" + ); + } + + #[gpui::test] + async fn test_list_directory_symlink_escape_private_path_no_authorization( + cx: &mut TestAppContext, + ) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { + "main.rs": "fn main() {}" + } + }, + "external": { + "secrets": {} + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link_to_external").as_ref(), + PathBuf::from("../external"), + ) + .await + .unwrap(); + + cx.update(|cx| { + SettingsStore::update_global(cx, |store, cx| { + store.update_user_settings(cx, |settings| { + settings.project.worktree.private_files = + Some(vec!["**/link_to_external".to_string()].into()); + }); + }); + }); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(ListDirectoryTool::new(project)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let result = cx + .update(|cx| { + tool.clone().run( + ListDirectoryToolInput { + path: "project/link_to_external".into(), + }, + event_stream, + cx, + ) + }) + .await; + + assert!( + result.is_err(), + "Expected list_directory to fail on private path" + ); + let error = result.unwrap_err().to_string(); + assert!( + error.contains("private"), + "Expected private path validation error, got: {error}" + ); + + let event = event_rx.try_next(); + assert!( + !matches!( + event, + Ok(Some(Ok(crate::thread::ThreadEvent::ToolCallAuthorization( + _ + )))) + ), + "No authorization should be requested when validation fails before listing", + ); + } + + #[gpui::test] + async fn test_list_directory_no_authorization_for_normal_paths(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/project"), + json!({ + "src": { + "main.rs": "fn main() {}" + } + }), + ) + .await; + + let project = Project::test(fs.clone(), [path!("/project").as_ref()], cx).await; + let tool = Arc::new(ListDirectoryTool::new(project)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let result = cx + .update(|cx| { + tool.clone().run( + ListDirectoryToolInput { + path: "project/src".into(), + }, + event_stream, + cx, + ) + }) + .await; + + assert!( + result.is_ok(), + "Normal path should succeed without authorization" + ); + + let event = event_rx.try_next(); + assert!( + !matches!( + event, + Ok(Some(Ok(crate::thread::ThreadEvent::ToolCallAuthorization( + _ + )))) + ), + "No authorization should be requested for normal paths", + ); + } + + #[gpui::test] + async fn test_list_directory_intra_project_symlink_no_authorization(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/project"), + json!({ + "real_dir": { + "file.txt": "content" + } + }), + ) + .await; + + fs.create_symlink( + path!("/project/link_dir").as_ref(), + PathBuf::from("real_dir"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(ListDirectoryTool::new(project)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let result = cx + .update(|cx| { + tool.clone().run( + ListDirectoryToolInput { + path: "project/link_dir".into(), + }, + event_stream, + cx, + ) + }) + .await; + + assert!( + result.is_ok(), + "Intra-project symlink should succeed without authorization: {result:?}", + ); + + let event = event_rx.try_next(); + assert!( + !matches!( + event, + Ok(Some(Ok(crate::thread::ThreadEvent::ToolCallAuthorization( + _ + )))) + ), + "No authorization should be requested for intra-project symlinks", + ); + } } diff --git a/crates/agent/src/tools/move_path_tool.rs b/crates/agent/src/tools/move_path_tool.rs index 25cbac9507eda1d23312ac220a43bd70f600e415..887cfd18d361b5d296017274b5600c5d3cb30716 100644 --- a/crates/agent/src/tools/move_path_tool.rs +++ b/crates/agent/src/tools/move_path_tool.rs @@ -1,5 +1,6 @@ -use super::edit_file_tool::{ - SensitiveSettingsKind, is_sensitive_settings_path, sensitive_settings_kind, +use super::tool_permissions::{ + SensitiveSettingsKind, authorize_symlink_escapes, canonicalize_worktree_roots, + collect_symlink_escapes, sensitive_settings_kind, }; use crate::{AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_for_paths}; use agent_client_protocol::ToolKind; @@ -97,40 +98,67 @@ impl AgentTool for MovePathTool { cx: &mut App, ) -> Task> { let settings = AgentSettings::get_global(cx); - let paths = vec![input.source_path.clone(), input.destination_path.clone()]; let decision = decide_permission_for_paths(Self::NAME, &paths, settings); if let ToolPermissionDecision::Deny(reason) = decision { return Task::ready(Err(anyhow!("{}", reason))); } - let needs_confirmation = matches!(decision, ToolPermissionDecision::Confirm) - || (matches!(decision, ToolPermissionDecision::Allow) - && (is_sensitive_settings_path(Path::new(&input.source_path)) - || is_sensitive_settings_path(Path::new(&input.destination_path)))); + let project = self.project.clone(); + cx.spawn(async move |cx| { + let fs = project.read_with(cx, |project, _cx| project.fs().clone()); + let canonical_roots = canonicalize_worktree_roots(&project, &fs, cx).await; - let authorize = if needs_confirmation { - let src = MarkdownInlineCode(&input.source_path); - let dest = MarkdownInlineCode(&input.destination_path); - let context = crate::ToolPermissionContext { - tool_name: Self::NAME.to_string(), - input_values: vec![input.source_path.clone(), input.destination_path.clone()], - }; - let title = format!("Move {src} to {dest}"); - let settings_kind = sensitive_settings_kind(Path::new(&input.source_path)) - .or_else(|| sensitive_settings_kind(Path::new(&input.destination_path))); - let title = match settings_kind { - Some(SensitiveSettingsKind::Local) => format!("{title} (local settings)"), - Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), - None => title, + let symlink_escapes: Vec<(&str, std::path::PathBuf)> = + project.read_with(cx, |project, cx| { + collect_symlink_escapes( + project, + &input.source_path, + &input.destination_path, + &canonical_roots, + cx, + ) + }); + + let sensitive_kind = + sensitive_settings_kind(Path::new(&input.source_path), fs.as_ref()) + .await + .or( + sensitive_settings_kind(Path::new(&input.destination_path), fs.as_ref()) + .await, + ); + + let needs_confirmation = matches!(decision, ToolPermissionDecision::Confirm) + || (matches!(decision, ToolPermissionDecision::Allow) && sensitive_kind.is_some()); + + let authorize = if !symlink_escapes.is_empty() { + // Symlink escape authorization replaces (rather than supplements) + // the normal tool-permission prompt. The symlink prompt already + // requires explicit user approval with the canonical target shown, + // which is strictly more security-relevant than a generic confirm. + Some(cx.update(|cx| { + authorize_symlink_escapes(Self::NAME, &symlink_escapes, &event_stream, cx) + })) + } else if needs_confirmation { + Some(cx.update(|cx| { + let src = MarkdownInlineCode(&input.source_path); + let dest = MarkdownInlineCode(&input.destination_path); + let context = crate::ToolPermissionContext::new( + Self::NAME, + vec![input.source_path.clone(), input.destination_path.clone()], + ); + let title = format!("Move {src} to {dest}"); + let title = match sensitive_kind { + Some(SensitiveSettingsKind::Local) => format!("{title} (local settings)"), + Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), + None => title, + }; + event_stream.authorize(title, context, cx) + })) + } else { + None }; - Some(event_stream.authorize(title, context, cx)) - } else { - None - }; - let project = self.project.clone(); - cx.spawn(async move |cx| { if let Some(authorize) = authorize { authorize.await?; } @@ -170,3 +198,258 @@ impl AgentTool for MovePathTool { }) } } + +#[cfg(test)] +mod tests { + use super::*; + use agent_client_protocol as acp; + use fs::Fs as _; + use gpui::TestAppContext; + use project::{FakeFs, Project}; + use serde_json::json; + use settings::SettingsStore; + use std::path::PathBuf; + use util::path; + + fn init_test(cx: &mut TestAppContext) { + cx.update(|cx| { + let settings_store = SettingsStore::test(cx); + cx.set_global(settings_store); + }); + cx.update(|cx| { + let mut settings = AgentSettings::get_global(cx).clone(); + settings.tool_permissions.default = settings::ToolPermissionMode::Allow; + AgentSettings::override_global(settings, cx); + }); + } + + #[gpui::test] + async fn test_move_path_symlink_escape_source_requests_authorization(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { "file.txt": "content" } + }, + "external": { + "secret.txt": "SECRET" + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link_to_external").as_ref(), + PathBuf::from("../external"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(MovePathTool::new(project)); + + let input = MovePathToolInput { + source_path: "project/link_to_external".into(), + destination_path: "project/external_moved".into(), + }; + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| tool.run(input, event_stream, cx)); + + let auth = event_rx.expect_authorization().await; + let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); + assert!( + title.contains("points outside the project") + || title.contains("symlinks outside project"), + "Authorization title should mention symlink escape, got: {title}", + ); + + auth.response + .send(acp::PermissionOptionId::new("allow")) + .unwrap(); + + let result = task.await; + assert!(result.is_ok(), "should succeed after approval: {result:?}"); + } + + #[gpui::test] + async fn test_move_path_symlink_escape_denied(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { "file.txt": "content" } + }, + "external": { + "secret.txt": "SECRET" + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link_to_external").as_ref(), + PathBuf::from("../external"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(MovePathTool::new(project)); + + let input = MovePathToolInput { + source_path: "project/link_to_external".into(), + destination_path: "project/external_moved".into(), + }; + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| tool.run(input, event_stream, cx)); + + let auth = event_rx.expect_authorization().await; + drop(auth); + + let result = task.await; + assert!(result.is_err(), "should fail when denied"); + } + + #[gpui::test] + async fn test_move_path_symlink_escape_confirm_requires_single_approval( + cx: &mut TestAppContext, + ) { + init_test(cx); + cx.update(|cx| { + let mut settings = AgentSettings::get_global(cx).clone(); + settings.tool_permissions.default = settings::ToolPermissionMode::Confirm; + AgentSettings::override_global(settings, cx); + }); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { "file.txt": "content" } + }, + "external": { + "secret.txt": "SECRET" + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link_to_external").as_ref(), + PathBuf::from("../external"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(MovePathTool::new(project)); + + let input = MovePathToolInput { + source_path: "project/link_to_external".into(), + destination_path: "project/external_moved".into(), + }; + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| tool.run(input, event_stream, cx)); + + let auth = event_rx.expect_authorization().await; + let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); + assert!( + title.contains("points outside the project") + || title.contains("symlinks outside project"), + "Authorization title should mention symlink escape, got: {title}", + ); + + auth.response + .send(acp::PermissionOptionId::new("allow")) + .unwrap(); + + assert!( + !matches!( + event_rx.try_next(), + Ok(Some(Ok(crate::ThreadEvent::ToolCallAuthorization(_)))) + ), + "Expected a single authorization prompt", + ); + + let result = task.await; + assert!( + result.is_ok(), + "Tool should succeed after one authorization: {result:?}" + ); + } + + #[gpui::test] + async fn test_move_path_symlink_escape_honors_deny_policy(cx: &mut TestAppContext) { + init_test(cx); + cx.update(|cx| { + let mut settings = AgentSettings::get_global(cx).clone(); + settings.tool_permissions.tools.insert( + "move_path".into(), + agent_settings::ToolRules { + default: Some(settings::ToolPermissionMode::Deny), + ..Default::default() + }, + ); + AgentSettings::override_global(settings, cx); + }); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { "file.txt": "content" } + }, + "external": { + "secret.txt": "SECRET" + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link_to_external").as_ref(), + PathBuf::from("../external"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(MovePathTool::new(project)); + + let input = MovePathToolInput { + source_path: "project/link_to_external".into(), + destination_path: "project/external_moved".into(), + }; + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let result = cx.update(|cx| tool.run(input, event_stream, cx)).await; + + assert!(result.is_err(), "Tool should fail when policy denies"); + assert!( + !matches!( + event_rx.try_next(), + Ok(Some(Ok(crate::ThreadEvent::ToolCallAuthorization(_)))) + ), + "Deny policy should not emit symlink authorization prompt", + ); + } +} diff --git a/crates/agent/src/tools/open_tool.rs b/crates/agent/src/tools/open_tool.rs index f401e56908d4f2a4702d71f6baeb766bcef1b60b..4028d3998b237b196f9c15756cfe7495d1cda3fa 100644 --- a/crates/agent/src/tools/open_tool.rs +++ b/crates/agent/src/tools/open_tool.rs @@ -1,8 +1,12 @@ +use super::tool_permissions::{ + ResolvedProjectPath, authorize_symlink_access, canonicalize_worktree_roots, + resolve_project_path, +}; use crate::AgentTool; use agent_client_protocol::ToolKind; use anyhow::{Context as _, Result}; use futures::FutureExt as _; -use gpui::{App, AppContext, Entity, SharedString, Task}; +use gpui::{App, AppContext as _, Entity, SharedString, Task}; use project::Project; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -64,13 +68,51 @@ impl AgentTool for OpenTool { ) -> Task> { // If path_or_url turns out to be a path in the project, make it absolute. let abs_path = to_absolute_path(&input.path_or_url, self.project.clone(), cx); - let context = crate::ToolPermissionContext { - tool_name: Self::NAME.to_string(), - input_values: vec![input.path_or_url.clone()], - }; - let authorize = - event_stream.authorize(self.initial_title(Ok(input.clone()), cx), context, cx); - cx.background_spawn(async move { + let initial_title = self.initial_title(Ok(input.clone()), cx); + + let project = self.project.clone(); + cx.spawn(async move |cx| { + let fs = project.read_with(cx, |project, _cx| project.fs().clone()); + let canonical_roots = canonicalize_worktree_roots(&project, &fs, cx).await; + + // Symlink escape authorization replaces (rather than supplements) + // the normal tool-permission prompt. The symlink prompt already + // requires explicit user approval with the canonical target shown, + // which is strictly more security-relevant than a generic confirm. + let symlink_escape = project.read_with(cx, |project, cx| { + match resolve_project_path( + project, + PathBuf::from(&input.path_or_url), + &canonical_roots, + cx, + ) { + Ok(ResolvedProjectPath::SymlinkEscape { + canonical_target, .. + }) => Some(canonical_target), + _ => None, + } + }); + + let authorize = if let Some(canonical_target) = symlink_escape { + cx.update(|cx| { + authorize_symlink_access( + Self::NAME, + &input.path_or_url, + &canonical_target, + &event_stream, + cx, + ) + }) + } else { + cx.update(|cx| { + let context = crate::ToolPermissionContext::new( + Self::NAME, + vec![input.path_or_url.clone()], + ); + event_stream.authorize(initial_title, context, cx) + }) + }; + futures::select! { result = authorize.fuse() => result?, _ = event_stream.cancelled_by_user().fuse() => { @@ -78,11 +120,15 @@ impl AgentTool for OpenTool { } } - match abs_path { - Some(path) => open::that(path), - None => open::that(&input.path_or_url), - } - .context("Failed to open URL or file path")?; + let path_or_url = input.path_or_url.clone(); + cx.background_spawn(async move { + match abs_path { + Some(path) => open::that(path), + None => open::that(path_or_url), + } + .context("Failed to open URL or file path") + }) + .await?; Ok(format!("Successfully opened {}", input.path_or_url)) }) diff --git a/crates/agent/src/tools/read_file_tool.rs b/crates/agent/src/tools/read_file_tool.rs index e0f1df0ca8662712a5e6967629740efd3cc89677..dcdb6dc914a7a3f135583e795671d2b03a2913bf 100644 --- a/crates/agent/src/tools/read_file_tool.rs +++ b/crates/agent/src/tools/read_file_tool.rs @@ -13,6 +13,10 @@ use settings::Settings; use std::sync::Arc; use util::markdown::MarkdownCodeBlock; +use super::tool_permissions::{ + ResolvedProjectPath, authorize_symlink_access, canonicalize_worktree_roots, + resolve_project_path, +}; use crate::{AgentTool, Thread, ToolCallEventStream, outline}; /// Reads the content of the given file in the project. @@ -110,57 +114,97 @@ impl AgentTool for ReadFileTool { event_stream: ToolCallEventStream, cx: &mut App, ) -> Task> { - let Some(project_path) = self.project.read(cx).find_project_path(&input.path, cx) else { - return Task::ready(Err(anyhow!("Path {} not found in project", &input.path))); - }; - let Some(abs_path) = self.project.read(cx).absolute_path(&project_path, cx) else { - return Task::ready(Err(anyhow!( - "Failed to convert {} to absolute path", - &input.path - ))); - }; - - // Error out if this path is either excluded or private in global settings - let global_settings = WorktreeSettings::get_global(cx); - if global_settings.is_path_excluded(&project_path.path) { - return Task::ready(Err(anyhow!( - "Cannot read file because its path matches the global `file_scan_exclusions` setting: {}", - &input.path - ))); - } + let project = self.project.clone(); + let thread = self.thread.clone(); + let action_log = self.action_log.clone(); + cx.spawn(async move |cx| { + let fs = project.read_with(cx, |project, _cx| project.fs().clone()); + let canonical_roots = canonicalize_worktree_roots(&project, &fs, cx).await; + + let (project_path, symlink_canonical_target) = + project.read_with(cx, |project, cx| { + let resolved = + resolve_project_path(project, &input.path, &canonical_roots, cx)?; + anyhow::Ok(match resolved { + ResolvedProjectPath::Safe(path) => (path, None), + ResolvedProjectPath::SymlinkEscape { + project_path, + canonical_target, + } => (project_path, Some(canonical_target)), + }) + })?; - if global_settings.is_path_private(&project_path.path) { - return Task::ready(Err(anyhow!( - "Cannot read file because its path matches the global `private_files` setting: {}", - &input.path - ))); - } + let abs_path = project + .read_with(cx, |project, cx| { + project.absolute_path(&project_path, cx) + }) + .ok_or_else(|| { + anyhow!("Failed to convert {} to absolute path", &input.path) + })?; + + // Check settings exclusions synchronously + project.read_with(cx, |_project, cx| { + let global_settings = WorktreeSettings::get_global(cx); + if global_settings.is_path_excluded(&project_path.path) { + anyhow::bail!( + "Cannot read file because its path matches the global `file_scan_exclusions` setting: {}", + &input.path + ); + } - // Error out if this path is either excluded or private in worktree settings - let worktree_settings = WorktreeSettings::get(Some((&project_path).into()), cx); - if worktree_settings.is_path_excluded(&project_path.path) { - return Task::ready(Err(anyhow!( - "Cannot read file because its path matches the worktree `file_scan_exclusions` setting: {}", - &input.path - ))); - } + if global_settings.is_path_private(&project_path.path) { + anyhow::bail!( + "Cannot read file because its path matches the global `private_files` setting: {}", + &input.path + ); + } - if worktree_settings.is_path_private(&project_path.path) { - return Task::ready(Err(anyhow!( - "Cannot read file because its path matches the worktree `private_files` setting: {}", - &input.path - ))); - } + let worktree_settings = WorktreeSettings::get(Some((&project_path).into()), cx); + if worktree_settings.is_path_excluded(&project_path.path) { + anyhow::bail!( + "Cannot read file because its path matches the worktree `file_scan_exclusions` setting: {}", + &input.path + ); + } - let file_path = input.path.clone(); + if worktree_settings.is_path_private(&project_path.path) { + anyhow::bail!( + "Cannot read file because its path matches the worktree `private_files` setting: {}", + &input.path + ); + } + + anyhow::Ok(()) + })?; + + if let Some(canonical_target) = &symlink_canonical_target { + let authorize = cx.update(|cx| { + authorize_symlink_access( + Self::NAME, + &input.path, + canonical_target, + &event_stream, + cx, + ) + }); + authorize.await?; + } + + let file_path = input.path.clone(); - event_stream.update_fields(ToolCallUpdateFields::new().locations(vec![ - acp::ToolCallLocation::new(&abs_path) - .line(input.start_line.map(|line| line.saturating_sub(1))), - ])); + cx.update(|_cx| { + event_stream.update_fields(ToolCallUpdateFields::new().locations(vec![ + acp::ToolCallLocation::new(&abs_path) + .line(input.start_line.map(|line| line.saturating_sub(1))), + ])); + }); + + let is_image = project.read_with(cx, |_project, cx| { + image_store::is_image_file(&project, &project_path, cx) + }); + + if is_image { - if image_store::is_image_file(&self.project, &project_path, cx) { - return cx.spawn(async move |cx| { let image_entity: Entity = cx .update(|cx| { self.project.update(cx, |project, cx| { @@ -183,18 +227,11 @@ impl AgentTool for ReadFileTool { ))), ])); - Ok(language_model_image.into()) - }); - } - - let project = self.project.clone(); - let action_log = self.action_log.clone(); + return Ok(language_model_image.into()); + } - cx.spawn(async move |cx| { - let open_buffer_task = cx.update(|cx| { - project.update(cx, |project, cx| { - project.open_buffer(project_path.clone(), cx) - }) + let open_buffer_task = project.update(cx, |project, cx| { + project.open_buffer(project_path.clone(), cx) }); let buffer = futures::select! { @@ -216,7 +253,7 @@ impl AgentTool for ReadFileTool { if let Some(mtime) = buffer.read_with(cx, |buffer, _| { buffer.file().and_then(|file| file.disk_state().mtime()) }) { - self.thread + thread .update(cx, |thread, _| { thread.file_read_times.insert(abs_path.to_path_buf(), mtime); }) @@ -292,6 +329,7 @@ impl AgentTool for ReadFileTool { cx, ); if let Ok(LanguageModelToolResultContent::Text(text)) = &result { + let text: &str = text; let markdown = MarkdownCodeBlock { tag: &input.path, text, @@ -312,12 +350,15 @@ impl AgentTool for ReadFileTool { mod test { use super::*; use crate::{ContextServerRegistry, Templates, Thread}; + use agent_client_protocol as acp; + use fs::Fs as _; use gpui::{AppContext, TestAppContext, UpdateGlobal as _}; use language_model::fake_provider::FakeLanguageModel; use project::{FakeFs, Project}; use prompt_store::ProjectContext; use serde_json::json; use settings::SettingsStore; + use std::path::PathBuf; use std::sync::Arc; use util::path; @@ -606,6 +647,16 @@ mod test { }); } + fn single_pixel_png() -> Vec { + vec![ + 0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A, 0x00, 0x00, 0x00, 0x0D, 0x49, 0x48, + 0x44, 0x52, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x01, 0x08, 0x06, 0x00, 0x00, + 0x00, 0x1F, 0x15, 0xC4, 0x89, 0x00, 0x00, 0x00, 0x0A, 0x49, 0x44, 0x41, 0x54, 0x78, + 0x9C, 0x63, 0x00, 0x01, 0x00, 0x00, 0x05, 0x00, 0x01, 0x0D, 0x0A, 0x2D, 0xB4, 0x00, + 0x00, 0x00, 0x00, 0x49, 0x45, 0x4E, 0x44, 0xAE, 0x42, 0x60, 0x82, + ] + } + #[gpui::test] async fn test_read_file_security(cx: &mut TestAppContext) { init_test(cx); @@ -813,6 +864,70 @@ mod test { ); } + #[gpui::test] + async fn test_read_image_symlink_requires_authorization(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree(path!("/root"), json!({})).await; + fs.insert_tree(path!("/outside"), json!({})).await; + fs.insert_file(path!("/outside/secret.png"), single_pixel_png()) + .await; + fs.insert_symlink( + path!("/root/secret.png"), + PathBuf::from("/outside/secret.png"), + ) + .await; + + let project = Project::test(fs.clone(), [path!("/root").as_ref()], cx).await; + let action_log = cx.new(|_| ActionLog::new(project.clone())); + let context_server_registry = + cx.new(|cx| ContextServerRegistry::new(project.read(cx).context_server_store(), cx)); + let model = Arc::new(FakeLanguageModel::default()); + let thread = cx.new(|cx| { + Thread::new( + project.clone(), + cx.new(|_cx| ProjectContext::default()), + context_server_registry, + Templates::new(), + Some(model), + cx, + ) + }); + let tool = Arc::new(ReadFileTool::new(thread.downgrade(), project, action_log)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let read_task = cx.update(|cx| { + tool.run( + ReadFileToolInput { + path: "root/secret.png".to_string(), + start_line: None, + end_line: None, + }, + event_stream, + cx, + ) + }); + + let authorization = event_rx.expect_authorization().await; + assert!( + authorization + .tool_call + .fields + .title + .as_deref() + .is_some_and(|title| title.contains("points outside the project")), + "Expected symlink escape authorization before reading the image" + ); + authorization + .response + .send(acp::PermissionOptionId::new("allow")) + .unwrap(); + + let result = read_task.await; + assert!(result.is_ok()); + } + #[gpui::test] async fn test_read_file_with_multiple_worktree_settings(cx: &mut TestAppContext) { init_test(cx); @@ -1046,4 +1161,245 @@ mod test { "Config.toml should be blocked by worktree1's private_files setting" ); } + + #[gpui::test] + async fn test_read_file_symlink_escape_requests_authorization(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { "main.rs": "fn main() {}" } + }, + "external": { + "secret.txt": "SECRET_KEY=abc123" + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/secret_link.txt").as_ref(), + PathBuf::from("../external/secret.txt"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let action_log = cx.new(|_| ActionLog::new(project.clone())); + let context_server_registry = + cx.new(|cx| ContextServerRegistry::new(project.read(cx).context_server_store(), cx)); + let model = Arc::new(FakeLanguageModel::default()); + let thread = cx.new(|cx| { + Thread::new( + project.clone(), + cx.new(|_cx| ProjectContext::default()), + context_server_registry, + Templates::new(), + Some(model), + cx, + ) + }); + let tool = Arc::new(ReadFileTool::new( + thread.downgrade(), + project.clone(), + action_log, + )); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + tool.clone().run( + ReadFileToolInput { + path: "project/secret_link.txt".to_string(), + start_line: None, + end_line: None, + }, + event_stream, + cx, + ) + }); + + let auth = event_rx.expect_authorization().await; + let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); + assert!( + title.contains("points outside the project"), + "title: {title}" + ); + + auth.response + .send(acp::PermissionOptionId::new("allow")) + .unwrap(); + + let result = task.await; + assert!(result.is_ok(), "should succeed after approval: {result:?}"); + } + + #[gpui::test] + async fn test_read_file_symlink_escape_denied(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { "main.rs": "fn main() {}" } + }, + "external": { + "secret.txt": "SECRET_KEY=abc123" + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/secret_link.txt").as_ref(), + PathBuf::from("../external/secret.txt"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let action_log = cx.new(|_| ActionLog::new(project.clone())); + let context_server_registry = + cx.new(|cx| ContextServerRegistry::new(project.read(cx).context_server_store(), cx)); + let model = Arc::new(FakeLanguageModel::default()); + let thread = cx.new(|cx| { + Thread::new( + project.clone(), + cx.new(|_cx| ProjectContext::default()), + context_server_registry, + Templates::new(), + Some(model), + cx, + ) + }); + let tool = Arc::new(ReadFileTool::new( + thread.downgrade(), + project.clone(), + action_log, + )); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + tool.clone().run( + ReadFileToolInput { + path: "project/secret_link.txt".to_string(), + start_line: None, + end_line: None, + }, + event_stream, + cx, + ) + }); + + let auth = event_rx.expect_authorization().await; + drop(auth); + + let result = task.await; + assert!( + result.is_err(), + "Tool should fail when authorization is denied" + ); + } + + #[gpui::test] + async fn test_read_file_symlink_escape_private_path_no_authorization(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { "main.rs": "fn main() {}" } + }, + "external": { + "secret.txt": "SECRET_KEY=abc123" + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/secret_link.txt").as_ref(), + PathBuf::from("../external/secret.txt"), + ) + .await + .unwrap(); + + cx.update(|cx| { + settings::SettingsStore::update_global(cx, |store, cx| { + store.update_user_settings(cx, |settings| { + settings.project.worktree.private_files = + Some(vec!["**/secret_link.txt".to_string()].into()); + }); + }); + }); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let action_log = cx.new(|_| ActionLog::new(project.clone())); + let context_server_registry = + cx.new(|cx| ContextServerRegistry::new(project.read(cx).context_server_store(), cx)); + let model = Arc::new(FakeLanguageModel::default()); + let thread = cx.new(|cx| { + Thread::new( + project.clone(), + cx.new(|_cx| ProjectContext::default()), + context_server_registry, + Templates::new(), + Some(model), + cx, + ) + }); + let tool = Arc::new(ReadFileTool::new( + thread.downgrade(), + project.clone(), + action_log, + )); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let result = cx + .update(|cx| { + tool.clone().run( + ReadFileToolInput { + path: "project/secret_link.txt".to_string(), + start_line: None, + end_line: None, + }, + event_stream, + cx, + ) + }) + .await; + + assert!( + result.is_err(), + "Expected read_file to fail on private path" + ); + let error = result.unwrap_err().to_string(); + assert!( + error.contains("private_files"), + "Expected private-files validation error, got: {error}" + ); + + let event = event_rx.try_next(); + assert!( + !matches!( + event, + Ok(Some(Ok(crate::thread::ThreadEvent::ToolCallAuthorization( + _ + )))) + ), + "No authorization should be requested when validation fails before read", + ); + } } diff --git a/crates/agent/src/tools/restore_file_from_disk_tool.rs b/crates/agent/src/tools/restore_file_from_disk_tool.rs index e16930046316a2aa9187587afc67493f1bd6908c..e07b7d52b0bdfcf41f4a07f56dda317573f5f716 100644 --- a/crates/agent/src/tools/restore_file_from_disk_tool.rs +++ b/crates/agent/src/tools/restore_file_from_disk_tool.rs @@ -1,5 +1,7 @@ -use super::edit_file_tool::{ - SensitiveSettingsKind, is_sensitive_settings_path, sensitive_settings_kind, +use super::tool_permissions::{ + ResolvedProjectPath, SensitiveSettingsKind, authorize_symlink_access, + canonicalize_worktree_roots, path_has_symlink_escape, resolve_project_path, + sensitive_settings_kind, }; use agent_client_protocol as acp; use agent_settings::AgentSettings; @@ -69,71 +71,94 @@ impl AgentTool for RestoreFileFromDiskTool { event_stream: ToolCallEventStream, cx: &mut App, ) -> Task> { - let settings = AgentSettings::get_global(cx); - let mut confirmation_paths: Vec = Vec::new(); + let settings = AgentSettings::get_global(cx).clone(); + // Check for any immediate deny before spawning async work. for path in &input.paths { let path_str = path.to_string_lossy(); - let decision = decide_permission_for_path(Self::NAME, &path_str, settings); - match decision { - ToolPermissionDecision::Allow => { - if is_sensitive_settings_path(Path::new(&*path_str)) { - confirmation_paths.push(path_str.to_string()); + let decision = decide_permission_for_path(Self::NAME, &path_str, &settings); + if let ToolPermissionDecision::Deny(reason) = decision { + return Task::ready(Err(anyhow::anyhow!("{}", reason))); + } + } + + let project = self.project.clone(); + let input_paths = input.paths; + + cx.spawn(async move |cx| { + let fs = project.read_with(cx, |project, _cx| project.fs().clone()); + let canonical_roots = canonicalize_worktree_roots(&project, &fs, cx).await; + + let mut confirmation_paths: Vec = Vec::new(); + + for path in &input_paths { + let path_str = path.to_string_lossy(); + let decision = decide_permission_for_path(Self::NAME, &path_str, &settings); + let symlink_escape = project.read_with(cx, |project, cx| { + path_has_symlink_escape(project, path, &canonical_roots, cx) + }); + + match decision { + ToolPermissionDecision::Allow => { + if !symlink_escape { + let is_sensitive = super::tool_permissions::is_sensitive_settings_path( + Path::new(&*path_str), + fs.as_ref(), + ) + .await; + if is_sensitive { + confirmation_paths.push(path_str.to_string()); + } + } + } + ToolPermissionDecision::Deny(reason) => { + return Err(anyhow::anyhow!("{}", reason)); + } + ToolPermissionDecision::Confirm => { + if !symlink_escape { + confirmation_paths.push(path_str.to_string()); + } } - } - ToolPermissionDecision::Deny(reason) => { - return Task::ready(Err(anyhow::anyhow!("{}", reason))); - } - ToolPermissionDecision::Confirm => { - confirmation_paths.push(path_str.to_string()); } } - } - let authorize = if !confirmation_paths.is_empty() { - let title = if confirmation_paths.len() == 1 { - format!( - "Restore {} from disk", - MarkdownInlineCode(&confirmation_paths[0]) - ) - } else { - let paths: Vec<_> = confirmation_paths - .iter() - .take(3) - .map(|p| p.as_str()) - .collect(); - if confirmation_paths.len() > 3 { + if !confirmation_paths.is_empty() { + let title = if confirmation_paths.len() == 1 { format!( - "Restore {}, and {} more from disk", - paths.join(", "), - confirmation_paths.len() - 3 + "Restore {} from disk", + MarkdownInlineCode(&confirmation_paths[0]) ) } else { - format!("Restore {} from disk", paths.join(", ")) - } - }; - let sensitive_kind = confirmation_paths - .iter() - .find_map(|p| sensitive_settings_kind(Path::new(p))); - let title = match sensitive_kind { - Some(SensitiveSettingsKind::Local) => format!("{title} (local settings)"), - Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), - None => title, - }; - let context = crate::ToolPermissionContext { - tool_name: Self::NAME.to_string(), - input_values: confirmation_paths, - }; - Some(event_stream.authorize(title, context, cx)) - } else { - None - }; - - let project = self.project.clone(); - let input_paths = input.paths; + let paths: Vec<_> = confirmation_paths + .iter() + .take(3) + .map(|p| p.as_str()) + .collect(); + if confirmation_paths.len() > 3 { + format!( + "Restore {}, and {} more from disk", + paths.join(", "), + confirmation_paths.len() - 3 + ) + } else { + format!("Restore {} from disk", paths.join(", ")) + } + }; - cx.spawn(async move |cx| { - if let Some(authorize) = authorize { + let mut settings_kind = None; + for p in &confirmation_paths { + if let Some(kind) = sensitive_settings_kind(Path::new(p), fs.as_ref()).await { + settings_kind = Some(kind); + break; + } + } + let title = match settings_kind { + Some(SensitiveSettingsKind::Local) => format!("{title} (local settings)"), + Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), + None => title, + }; + let context = crate::ToolPermissionContext::new(Self::NAME, confirmation_paths); + let authorize = cx.update(|cx| event_stream.authorize(title, context, cx)); authorize.await?; } let mut buffers_to_reload: FxHashSet> = FxHashSet::default(); @@ -146,11 +171,40 @@ impl AgentTool for RestoreFileFromDiskTool { let mut reload_errors: Vec = Vec::new(); for path in input_paths { - let Some(project_path) = - project.read_with(cx, |project, cx| project.find_project_path(&path, cx)) - else { - not_found_paths.push(path); - continue; + let project_path = match project.read_with(cx, |project, cx| { + resolve_project_path(project, &path, &canonical_roots, cx) + }) { + Ok(resolved) => { + let (project_path, symlink_canonical_target) = match resolved { + ResolvedProjectPath::Safe(path) => (path, None), + ResolvedProjectPath::SymlinkEscape { + project_path, + canonical_target, + } => (project_path, Some(canonical_target)), + }; + if let Some(canonical_target) = &symlink_canonical_target { + let path_str = path.to_string_lossy(); + let authorize_task = cx.update(|cx| { + authorize_symlink_access( + Self::NAME, + &path_str, + canonical_target, + &event_stream, + cx, + ) + }); + let result = authorize_task.await; + if let Err(err) = result { + reload_errors.push(format!("{}: {}", path.to_string_lossy(), err)); + continue; + } + } + project_path + } + Err(_) => { + not_found_paths.push(path); + continue; + } }; let open_buffer_task = @@ -246,7 +300,7 @@ impl AgentTool for RestoreFileFromDiskTool { #[cfg(test)] mod tests { use super::*; - use fs::Fs; + use fs::Fs as _; use gpui::TestAppContext; use language::LineEnding; use project::FakeFs; @@ -408,4 +462,197 @@ mod tests { let _ = LineEnding::Unix; // keep import used if the buffer edit API changes } + + #[gpui::test] + async fn test_restore_file_symlink_escape_requests_authorization(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": {} + }, + "external": { + "secret.txt": "secret content" + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link.txt").as_ref(), + PathBuf::from("../external/secret.txt"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(RestoreFileFromDiskTool::new(project)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + tool.clone().run( + RestoreFileFromDiskToolInput { + paths: vec![PathBuf::from("project/link.txt")], + }, + event_stream, + cx, + ) + }); + + cx.run_until_parked(); + + let auth = event_rx.expect_authorization().await; + let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); + assert!( + title.contains("points outside the project"), + "Expected symlink escape authorization, got: {title}", + ); + + auth.response + .send(acp::PermissionOptionId::new("allow")) + .unwrap(); + + let _result = task.await; + } + + #[gpui::test] + async fn test_restore_file_symlink_escape_honors_deny_policy(cx: &mut TestAppContext) { + init_test(cx); + cx.update(|cx| { + let mut settings = AgentSettings::get_global(cx).clone(); + settings.tool_permissions.tools.insert( + "restore_file_from_disk".into(), + agent_settings::ToolRules { + default: Some(settings::ToolPermissionMode::Deny), + ..Default::default() + }, + ); + AgentSettings::override_global(settings, cx); + }); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": {} + }, + "external": { + "secret.txt": "secret content" + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link.txt").as_ref(), + PathBuf::from("../external/secret.txt"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(RestoreFileFromDiskTool::new(project)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let result = cx + .update(|cx| { + tool.clone().run( + RestoreFileFromDiskToolInput { + paths: vec![PathBuf::from("project/link.txt")], + }, + event_stream, + cx, + ) + }) + .await; + + assert!(result.is_err(), "Tool should fail when policy denies"); + assert!( + !matches!( + event_rx.try_next(), + Ok(Some(Ok(crate::ThreadEvent::ToolCallAuthorization(_)))) + ), + "Deny policy should not emit symlink authorization prompt", + ); + } + + #[gpui::test] + async fn test_restore_file_symlink_escape_confirm_requires_single_approval( + cx: &mut TestAppContext, + ) { + init_test(cx); + cx.update(|cx| { + let mut settings = AgentSettings::get_global(cx).clone(); + settings.tool_permissions.default = settings::ToolPermissionMode::Confirm; + AgentSettings::override_global(settings, cx); + }); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": {} + }, + "external": { + "secret.txt": "secret content" + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link.txt").as_ref(), + PathBuf::from("../external/secret.txt"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(RestoreFileFromDiskTool::new(project)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + tool.clone().run( + RestoreFileFromDiskToolInput { + paths: vec![PathBuf::from("project/link.txt")], + }, + event_stream, + cx, + ) + }); + + cx.run_until_parked(); + + let auth = event_rx.expect_authorization().await; + let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); + assert!( + title.contains("points outside the project"), + "Expected symlink escape authorization, got: {title}", + ); + + auth.response + .send(acp::PermissionOptionId::new("allow")) + .unwrap(); + + assert!( + !matches!( + event_rx.try_next(), + Ok(Some(Ok(crate::ThreadEvent::ToolCallAuthorization(_)))) + ), + "Expected a single authorization prompt", + ); + + let _result = task.await; + } } diff --git a/crates/agent/src/tools/save_file_tool.rs b/crates/agent/src/tools/save_file_tool.rs index 842afa609bbfd946e4fb256d7ae11d4b6c383212..bb46e1018713baa44a187ae75aeda5f1a491f5c8 100644 --- a/crates/agent/src/tools/save_file_tool.rs +++ b/crates/agent/src/tools/save_file_tool.rs @@ -13,8 +13,10 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use util::markdown::MarkdownInlineCode; -use super::edit_file_tool::{ - SensitiveSettingsKind, is_sensitive_settings_path, sensitive_settings_kind, +use super::tool_permissions::{ + ResolvedProjectPath, SensitiveSettingsKind, authorize_symlink_access, + canonicalize_worktree_roots, path_has_symlink_escape, resolve_project_path, + sensitive_settings_kind, }; use crate::{AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_for_path}; @@ -66,68 +68,92 @@ impl AgentTool for SaveFileTool { event_stream: ToolCallEventStream, cx: &mut App, ) -> Task> { - let settings = AgentSettings::get_global(cx); - let mut confirmation_paths: Vec = Vec::new(); + let settings = AgentSettings::get_global(cx).clone(); + // Check for any immediate deny before spawning async work. for path in &input.paths { let path_str = path.to_string_lossy(); - let decision = decide_permission_for_path(Self::NAME, &path_str, settings); - match decision { - ToolPermissionDecision::Allow => { - if is_sensitive_settings_path(Path::new(&*path_str)) { - confirmation_paths.push(path_str.to_string()); - } - } - ToolPermissionDecision::Deny(reason) => { - return Task::ready(Err(anyhow::anyhow!("{}", reason))); - } - ToolPermissionDecision::Confirm => { - confirmation_paths.push(path_str.to_string()); - } + let decision = decide_permission_for_path(Self::NAME, &path_str, &settings); + if let ToolPermissionDecision::Deny(reason) = decision { + return Task::ready(Err(anyhow::anyhow!("{}", reason))); } } - let authorize = if !confirmation_paths.is_empty() { - let title = if confirmation_paths.len() == 1 { - format!("Save {}", MarkdownInlineCode(&confirmation_paths[0])) - } else { - let paths: Vec<_> = confirmation_paths - .iter() - .take(3) - .map(|p| p.as_str()) - .collect(); - if confirmation_paths.len() > 3 { - format!( - "Save {}, and {} more", - paths.join(", "), - confirmation_paths.len() - 3 - ) - } else { - format!("Save {}", paths.join(", ")) - } - }; - let sensitive_kind = confirmation_paths - .iter() - .find_map(|p| sensitive_settings_kind(Path::new(p))); - let title = match sensitive_kind { - Some(SensitiveSettingsKind::Local) => format!("{title} (local settings)"), - Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), - None => title, - }; - let context = crate::ToolPermissionContext { - tool_name: Self::NAME.to_string(), - input_values: confirmation_paths.clone(), - }; - Some(event_stream.authorize(title, context, cx)) - } else { - None - }; - let project = self.project.clone(); let input_paths = input.paths; cx.spawn(async move |cx| { - if let Some(authorize) = authorize { + let fs = project.read_with(cx, |project, _cx| project.fs().clone()); + let canonical_roots = canonicalize_worktree_roots(&project, &fs, cx).await; + + let mut confirmation_paths: Vec = Vec::new(); + + for path in &input_paths { + let path_str = path.to_string_lossy(); + let decision = decide_permission_for_path(Self::NAME, &path_str, &settings); + let symlink_escape = project.read_with(cx, |project, cx| { + path_has_symlink_escape(project, path, &canonical_roots, cx) + }); + + match decision { + ToolPermissionDecision::Allow => { + if !symlink_escape { + let is_sensitive = super::tool_permissions::is_sensitive_settings_path( + Path::new(&*path_str), + fs.as_ref(), + ) + .await; + if is_sensitive { + confirmation_paths.push(path_str.to_string()); + } + } + } + ToolPermissionDecision::Deny(reason) => { + return Err(anyhow::anyhow!("{}", reason)); + } + ToolPermissionDecision::Confirm => { + if !symlink_escape { + confirmation_paths.push(path_str.to_string()); + } + } + } + } + + if !confirmation_paths.is_empty() { + let title = if confirmation_paths.len() == 1 { + format!("Save {}", MarkdownInlineCode(&confirmation_paths[0])) + } else { + let paths: Vec<_> = confirmation_paths + .iter() + .take(3) + .map(|p| p.as_str()) + .collect(); + if confirmation_paths.len() > 3 { + format!( + "Save {}, and {} more", + paths.join(", "), + confirmation_paths.len() - 3 + ) + } else { + format!("Save {}", paths.join(", ")) + } + }; + + let mut settings_kind = None; + for p in &confirmation_paths { + if let Some(kind) = sensitive_settings_kind(Path::new(p), fs.as_ref()).await { + settings_kind = Some(kind); + break; + } + } + let title = match settings_kind { + Some(SensitiveSettingsKind::Local) => format!("{title} (local settings)"), + Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), + None => title, + }; + let context = + crate::ToolPermissionContext::new(Self::NAME, confirmation_paths.clone()); + let authorize = cx.update(|cx| event_stream.authorize(title, context, cx)); authorize.await?; } @@ -137,14 +163,44 @@ impl AgentTool for SaveFileTool { let mut clean_paths: Vec = Vec::new(); let mut not_found_paths: Vec = Vec::new(); let mut open_errors: Vec<(PathBuf, String)> = Vec::new(); + let mut authorization_errors: Vec<(PathBuf, String)> = Vec::new(); let mut save_errors: Vec<(String, String)> = Vec::new(); for path in input_paths { - let Some(project_path) = - project.read_with(cx, |project, cx| project.find_project_path(&path, cx)) - else { - not_found_paths.push(path); - continue; + let project_path = match project.read_with(cx, |project, cx| { + resolve_project_path(project, &path, &canonical_roots, cx) + }) { + Ok(resolved) => { + let (project_path, symlink_canonical_target) = match resolved { + ResolvedProjectPath::Safe(path) => (path, None), + ResolvedProjectPath::SymlinkEscape { + project_path, + canonical_target, + } => (project_path, Some(canonical_target)), + }; + if let Some(canonical_target) = &symlink_canonical_target { + let path_str = path.to_string_lossy(); + let authorize_task = cx.update(|cx| { + authorize_symlink_access( + Self::NAME, + &path_str, + canonical_target, + &event_stream, + cx, + ) + }); + let result = authorize_task.await; + if let Err(err) = result { + authorization_errors.push((path.clone(), err.to_string())); + continue; + } + } + project_path + } + Err(_) => { + not_found_paths.push(path); + continue; + } }; let open_buffer_task = @@ -221,6 +277,15 @@ impl AgentTool for SaveFileTool { lines.push(format!("- {}: {}", path.display(), error)); } } + if !authorization_errors.is_empty() { + lines.push(format!( + "Authorization failed ({}):", + authorization_errors.len() + )); + for (path, error) in &authorization_errors { + lines.push(format!("- {}: {}", path.display(), error)); + } + } if !save_errors.is_empty() { lines.push(format!("Save failed ({}):", save_errors.len())); for (path, error) in &save_errors { @@ -240,7 +305,7 @@ impl AgentTool for SaveFileTool { #[cfg(test)] mod tests { use super::*; - use fs::Fs; + use fs::Fs as _; use gpui::TestAppContext; use project::FakeFs; use serde_json::json; @@ -392,4 +457,282 @@ mod tests { "expected not-found path bullet, got:\n{output}" ); } + + #[gpui::test] + async fn test_save_file_symlink_escape_requests_authorization(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": {} + }, + "external": { + "secret.txt": "secret content" + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link.txt").as_ref(), + PathBuf::from("../external/secret.txt"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(SaveFileTool::new(project)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + tool.clone().run( + SaveFileToolInput { + paths: vec![PathBuf::from("project/link.txt")], + }, + event_stream, + cx, + ) + }); + + cx.run_until_parked(); + + let auth = event_rx.expect_authorization().await; + let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); + assert!( + title.contains("points outside the project"), + "Expected symlink escape authorization, got: {title}", + ); + + auth.response + .send(acp::PermissionOptionId::new("allow")) + .unwrap(); + + let _result = task.await; + } + + #[gpui::test] + async fn test_save_file_symlink_escape_honors_deny_policy(cx: &mut TestAppContext) { + init_test(cx); + cx.update(|cx| { + let mut settings = AgentSettings::get_global(cx).clone(); + settings.tool_permissions.tools.insert( + "save_file".into(), + agent_settings::ToolRules { + default: Some(settings::ToolPermissionMode::Deny), + ..Default::default() + }, + ); + AgentSettings::override_global(settings, cx); + }); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": {} + }, + "external": { + "secret.txt": "secret content" + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link.txt").as_ref(), + PathBuf::from("../external/secret.txt"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(SaveFileTool::new(project)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let result = cx + .update(|cx| { + tool.clone().run( + SaveFileToolInput { + paths: vec![PathBuf::from("project/link.txt")], + }, + event_stream, + cx, + ) + }) + .await; + + assert!(result.is_err(), "Tool should fail when policy denies"); + assert!( + !matches!( + event_rx.try_next(), + Ok(Some(Ok(crate::ThreadEvent::ToolCallAuthorization(_)))) + ), + "Deny policy should not emit symlink authorization prompt", + ); + } + + #[gpui::test] + async fn test_save_file_symlink_escape_confirm_requires_single_approval( + cx: &mut TestAppContext, + ) { + init_test(cx); + cx.update(|cx| { + let mut settings = AgentSettings::get_global(cx).clone(); + settings.tool_permissions.default = settings::ToolPermissionMode::Confirm; + AgentSettings::override_global(settings, cx); + }); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": {} + }, + "external": { + "secret.txt": "secret content" + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link.txt").as_ref(), + PathBuf::from("../external/secret.txt"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(SaveFileTool::new(project)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + tool.clone().run( + SaveFileToolInput { + paths: vec![PathBuf::from("project/link.txt")], + }, + event_stream, + cx, + ) + }); + + cx.run_until_parked(); + + let auth = event_rx.expect_authorization().await; + let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); + assert!( + title.contains("points outside the project"), + "Expected symlink escape authorization, got: {title}", + ); + + auth.response + .send(acp::PermissionOptionId::new("allow")) + .unwrap(); + + assert!( + !matches!( + event_rx.try_next(), + Ok(Some(Ok(crate::ThreadEvent::ToolCallAuthorization(_)))) + ), + "Expected a single authorization prompt", + ); + + let _result = task.await; + } + + #[gpui::test] + async fn test_save_file_symlink_denial_does_not_reduce_success_count(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "dirty.txt": "on disk value\n", + }, + "external": { + "secret.txt": "secret content" + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/project/link.txt").as_ref(), + PathBuf::from("../external/secret.txt"), + ) + .await + .unwrap(); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let dirty_project_path = project.read_with(cx, |project, cx| { + project + .find_project_path("project/dirty.txt", cx) + .expect("dirty.txt should exist in project") + }); + let dirty_buffer = project + .update(cx, |project, cx| { + project.open_buffer(dirty_project_path, cx) + }) + .await + .unwrap(); + dirty_buffer.update(cx, |buffer, cx| { + buffer.edit([(0..buffer.len(), "in memory value\n")], None, cx); + }); + assert!( + dirty_buffer.read_with(cx, |buffer, _| buffer.is_dirty()), + "dirty.txt should be dirty before save" + ); + + let tool = Arc::new(SaveFileTool::new(project)); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + tool.clone().run( + SaveFileToolInput { + paths: vec![ + PathBuf::from("project/dirty.txt"), + PathBuf::from("project/link.txt"), + ], + }, + event_stream, + cx, + ) + }); + + cx.run_until_parked(); + + let auth = event_rx.expect_authorization().await; + auth.response + .send(acp::PermissionOptionId::new("deny")) + .unwrap(); + + let output = task.await.unwrap(); + assert!( + output.contains("Saved 1 file(s)."), + "Expected successful save count to remain accurate, got:\n{output}", + ); + assert!( + output.contains("Authorization failed (1):"), + "Expected authorization failure section, got:\n{output}", + ); + assert!( + !output.contains("Save failed"), + "Authorization denials should not be counted as save failures, got:\n{output}", + ); + } } diff --git a/crates/agent/src/tools/streaming_edit_file_tool.rs b/crates/agent/src/tools/streaming_edit_file_tool.rs index b822191a7e78ba566f1551661e748b2027f2404d..561a94be1bd34145498f22147c844874fee316d5 100644 --- a/crates/agent/src/tools/streaming_edit_file_tool.rs +++ b/crates/agent/src/tools/streaming_edit_file_tool.rs @@ -180,7 +180,7 @@ impl StreamingEditFileTool { event_stream: &ToolCallEventStream, cx: &mut App, ) -> Task> { - super::edit_file_tool::authorize_file_edit( + super::tool_permissions::authorize_file_edit( EditFileTool::NAME, &input.path, &input.display_description, diff --git a/crates/agent/src/tools/terminal_tool.rs b/crates/agent/src/tools/terminal_tool.rs index 252545eaed10287119845142acfa0bc80165121f..ddb8ee1975a6d4a3fc9838a133482f65dc3b00b6 100644 --- a/crates/agent/src/tools/terminal_tool.rs +++ b/crates/agent/src/tools/terminal_tool.rs @@ -107,10 +107,8 @@ impl AgentTool for TerminalTool { return Task::ready(Err(anyhow::anyhow!("{}", reason))); } ToolPermissionDecision::Confirm => { - let context = crate::ToolPermissionContext { - tool_name: Self::NAME.to_string(), - input_values: vec![input.command.clone()], - }; + let context = + crate::ToolPermissionContext::new(Self::NAME, vec![input.command.clone()]); Some(event_stream.authorize(self.initial_title(Ok(input.clone()), cx), context, cx)) } }; diff --git a/crates/agent/src/tools/tool_permissions.rs b/crates/agent/src/tools/tool_permissions.rs new file mode 100644 index 0000000000000000000000000000000000000000..6fa6215a44f563551b0ba943e84bdb5419453621 --- /dev/null +++ b/crates/agent/src/tools/tool_permissions.rs @@ -0,0 +1,837 @@ +use crate::{ + Thread, ToolCallEventStream, ToolPermissionContext, ToolPermissionDecision, + decide_permission_for_path, +}; +use anyhow::{Result, anyhow}; +use fs::Fs; +use gpui::{App, Entity, Task, WeakEntity}; +use project::{Project, ProjectPath}; +use settings::Settings; +use std::ffi::OsStr; +use std::path::{Path, PathBuf}; +use std::sync::Arc; + +pub enum SensitiveSettingsKind { + Local, + Global, +} + +/// Result of resolving a path within the project with symlink safety checks. +/// +/// See [`resolve_project_path`]. +#[derive(Debug, Clone)] +pub enum ResolvedProjectPath { + /// The path resolves to a location safely within the project boundaries. + Safe(ProjectPath), + /// The path resolves through a symlink to a location outside the project. + /// Agent tools should prompt the user before proceeding with access. + SymlinkEscape { + /// The project-relative path (before symlink resolution). + project_path: ProjectPath, + /// The canonical (real) filesystem path the symlink points to. + canonical_target: PathBuf, + }, +} + +/// Asynchronously canonicalizes the absolute paths of all worktrees in a +/// project using the provided `Fs`. The returned paths can be passed to +/// [`resolve_project_path`] and related helpers so that they don't need to +/// perform blocking filesystem I/O themselves. +pub async fn canonicalize_worktree_roots( + project: &Entity, + fs: &Arc, + cx: &C, +) -> Vec { + let abs_paths: Vec> = project.read_with(cx, |project, cx| { + project + .worktrees(cx) + .map(|worktree| worktree.read(cx).abs_path()) + .collect() + }); + + let mut canonical_roots = Vec::with_capacity(abs_paths.len()); + for abs_path in &abs_paths { + match fs.canonicalize(abs_path).await { + Ok(canonical) => canonical_roots.push(canonical), + Err(_) => canonical_roots.push(abs_path.to_path_buf()), + } + } + canonical_roots +} + +/// Walks up ancestors of `path` to find the deepest one that exists on disk and +/// can be canonicalized, then reattaches the remaining suffix components. +/// +/// This is needed for paths where the leaf (or intermediate directories) don't +/// exist yet but an ancestor may be a symlink. For example, when creating +/// `.zed/settings.json` where `.zed` is a symlink to an external directory. +/// +/// Note: intermediate directories *can* be symlinks (not just leaf entries), +/// so we must walk the full ancestor chain. For example: +/// `ln -s /external/config /project/.zed` +/// makes `.zed` an intermediate symlink directory. +async fn canonicalize_with_ancestors(path: &Path, fs: &dyn Fs) -> Option { + let mut current: Option<&Path> = Some(path); + let mut suffix_components = Vec::new(); + loop { + match current { + Some(ancestor) => match fs.canonicalize(ancestor).await { + Ok(canonical) => { + let mut result = canonical; + for component in suffix_components.into_iter().rev() { + result.push(component); + } + return Some(result); + } + Err(_) => { + if let Some(file_name) = ancestor.file_name() { + suffix_components.push(file_name.to_os_string()); + } + current = ancestor.parent(); + } + }, + None => return None, + } + } +} + +fn is_within_any_worktree(canonical_path: &Path, canonical_worktree_roots: &[PathBuf]) -> bool { + canonical_worktree_roots + .iter() + .any(|root| canonical_path.starts_with(root)) +} + +/// Returns the kind of sensitive settings location this path targets, if any: +/// either inside a `.zed/` local-settings directory or inside the global config dir. +pub async fn sensitive_settings_kind(path: &Path, fs: &dyn Fs) -> Option { + let local_settings_folder = paths::local_settings_folder_name(); + if path.components().any(|component| { + component.as_os_str() == <_ as AsRef>::as_ref(&local_settings_folder) + }) { + return Some(SensitiveSettingsKind::Local); + } + + if let Some(canonical_path) = canonicalize_with_ancestors(path, fs).await { + let config_dir = fs + .canonicalize(paths::config_dir()) + .await + .unwrap_or_else(|_| paths::config_dir().to_path_buf()); + if canonical_path.starts_with(&config_dir) { + return Some(SensitiveSettingsKind::Global); + } + } + + None +} + +pub async fn is_sensitive_settings_path(path: &Path, fs: &dyn Fs) -> bool { + sensitive_settings_kind(path, fs).await.is_some() +} + +/// Resolves a path within the project, checking for symlink escapes. +/// +/// This is the primary entry point for agent tools that need to resolve a +/// user-provided path string into a validated `ProjectPath`. It combines +/// path lookup (`find_project_path`) with symlink safety verification. +/// +/// `canonical_worktree_roots` should be obtained from +/// [`canonicalize_worktree_roots`] before calling this function so that no +/// blocking I/O is needed here. +/// +/// # Returns +/// +/// - `Ok(ResolvedProjectPath::Safe(project_path))` — the path resolves to a +/// location within the project boundaries. +/// - `Ok(ResolvedProjectPath::SymlinkEscape { .. })` — the path resolves +/// through a symlink to a location outside the project. Agent tools should +/// prompt the user before proceeding. +/// - `Err(..)` — the path could not be found in the project or could not be +/// verified. The error message is suitable for returning to the model. +pub fn resolve_project_path( + project: &Project, + path: impl AsRef, + canonical_worktree_roots: &[PathBuf], + cx: &App, +) -> Result { + let path = path.as_ref(); + let project_path = project + .find_project_path(path, cx) + .ok_or_else(|| anyhow!("Path {} is not in the project", path.display()))?; + + let worktree = project + .worktree_for_id(project_path.worktree_id, cx) + .ok_or_else(|| anyhow!("Could not resolve path {}", path.display()))?; + let snapshot = worktree.read(cx); + + // Fast path: if the entry exists in the snapshot and is not marked + // external, we know it's safe (the background scanner already verified). + if let Some(entry) = snapshot.entry_for_path(&project_path.path) { + if !entry.is_external { + return Ok(ResolvedProjectPath::Safe(project_path)); + } + + // Entry is external (set by the worktree scanner when a symlink's + // canonical target is outside the worktree root). Return the + // canonical path if the entry has one, otherwise fall through to + // filesystem-level canonicalization. + if let Some(canonical) = &entry.canonical_path { + if is_within_any_worktree(canonical.as_ref(), canonical_worktree_roots) { + return Ok(ResolvedProjectPath::Safe(project_path)); + } + + return Ok(ResolvedProjectPath::SymlinkEscape { + project_path, + canonical_target: canonical.to_path_buf(), + }); + } + } + + // For missing/create-mode paths (or external descendants without their own + // canonical_path), resolve symlink safety through snapshot metadata rather + // than std::fs canonicalization. This keeps behavior correct for non-local + // worktrees and in-memory fs backends. + for ancestor in project_path.path.ancestors() { + let Some(ancestor_entry) = snapshot.entry_for_path(ancestor) else { + continue; + }; + + if !ancestor_entry.is_external { + return Ok(ResolvedProjectPath::Safe(project_path)); + } + + let Some(canonical_ancestor) = ancestor_entry.canonical_path.as_ref() else { + continue; + }; + + let suffix = project_path.path.strip_prefix(ancestor).map_err(|_| { + anyhow!( + "Path {} could not be resolved in the project", + path.display() + ) + })?; + + let canonical_target = if suffix.is_empty() { + canonical_ancestor.to_path_buf() + } else { + canonical_ancestor.join(suffix.as_std_path()) + }; + + if is_within_any_worktree(&canonical_target, canonical_worktree_roots) { + return Ok(ResolvedProjectPath::Safe(project_path)); + } + + return Ok(ResolvedProjectPath::SymlinkEscape { + project_path, + canonical_target, + }); + } + + Ok(ResolvedProjectPath::Safe(project_path)) +} + +/// Prompts the user for permission when a path resolves through a symlink to a +/// location outside the project. This check is an additional gate after +/// settings-based deny decisions: even if a tool is configured as "always allow," +/// a symlink escape still requires explicit user approval. +pub fn authorize_symlink_access( + tool_name: &str, + display_path: &str, + canonical_target: &Path, + event_stream: &ToolCallEventStream, + cx: &mut App, +) -> Task> { + let title = format!( + "`{}` points outside the project (symlink to `{}`)", + display_path, + canonical_target.display(), + ); + + let context = ToolPermissionContext::symlink_target( + tool_name, + vec![canonical_target.display().to_string()], + ); + + event_stream.authorize(title, context, cx) +} + +/// Creates a single authorization prompt for multiple symlink escapes. +/// Each escape is a `(display_path, canonical_target)` pair. +/// +/// Accepts `&[(&str, PathBuf)]` to match the natural return type of +/// [`detect_symlink_escape`], avoiding intermediate owned-to-borrowed +/// conversions at call sites. +pub fn authorize_symlink_escapes( + tool_name: &str, + escapes: &[(&str, PathBuf)], + event_stream: &ToolCallEventStream, + cx: &mut App, +) -> Task> { + debug_assert!(!escapes.is_empty()); + + if escapes.len() == 1 { + return authorize_symlink_access(tool_name, escapes[0].0, &escapes[0].1, event_stream, cx); + } + + let targets = escapes + .iter() + .map(|(path, target)| format!("`{}` → `{}`", path, target.display())) + .collect::>() + .join(" and "); + let title = format!("{} (symlinks outside project)", targets); + + let context = ToolPermissionContext::symlink_target( + tool_name, + escapes + .iter() + .map(|(_, target)| target.display().to_string()) + .collect(), + ); + + event_stream.authorize(title, context, cx) +} + +/// Checks whether a path escapes the project via symlink, without creating +/// an authorization task. Useful for pre-filtering paths before settings checks. +pub fn path_has_symlink_escape( + project: &Project, + path: impl AsRef, + canonical_worktree_roots: &[PathBuf], + cx: &App, +) -> bool { + matches!( + resolve_project_path(project, path, canonical_worktree_roots, cx), + Ok(ResolvedProjectPath::SymlinkEscape { .. }) + ) +} + +/// Collects symlink escape info for a path without creating an authorization task. +/// Returns `Some((display_path, canonical_target))` if the path escapes via symlink. +pub fn detect_symlink_escape<'a>( + project: &Project, + display_path: &'a str, + canonical_worktree_roots: &[PathBuf], + cx: &App, +) -> Option<(&'a str, PathBuf)> { + match resolve_project_path(project, display_path, canonical_worktree_roots, cx).ok()? { + ResolvedProjectPath::Safe(_) => None, + ResolvedProjectPath::SymlinkEscape { + canonical_target, .. + } => Some((display_path, canonical_target)), + } +} + +/// Collects symlink escape info for two paths (source and destination) and +/// returns any escapes found. This deduplicates the common pattern used by +/// tools that operate on two paths (copy, move). +/// +/// Returns a `Vec` of `(display_path, canonical_target)` pairs for paths +/// that escape the project via symlink. The returned vec borrows the display +/// paths from the input strings. +pub fn collect_symlink_escapes<'a>( + project: &Project, + source_path: &'a str, + destination_path: &'a str, + canonical_worktree_roots: &[PathBuf], + cx: &App, +) -> Vec<(&'a str, PathBuf)> { + let mut escapes = Vec::new(); + if let Some(escape) = detect_symlink_escape(project, source_path, canonical_worktree_roots, cx) + { + escapes.push(escape); + } + if let Some(escape) = + detect_symlink_escape(project, destination_path, canonical_worktree_roots, cx) + { + escapes.push(escape); + } + escapes +} + +/// Checks authorization for file edits, handling symlink escapes and +/// sensitive settings paths. +/// +/// # Authorization precedence +/// +/// When a symlink escape is detected, the symlink authorization prompt +/// *replaces* (rather than supplements) the normal tool-permission prompt. +/// This is intentional: the symlink prompt already requires explicit user +/// approval and displays the canonical target, which provides strictly more +/// security-relevant information than the generic tool confirmation. Requiring +/// two sequential prompts for the same operation would degrade UX without +/// meaningfully improving security, since the user must already approve the +/// more specific symlink-escape prompt. +pub fn authorize_file_edit( + tool_name: &str, + path: &Path, + display_description: &str, + thread: &WeakEntity, + event_stream: &ToolCallEventStream, + cx: &mut App, +) -> Task> { + let path_str = path.to_string_lossy(); + + let settings = agent_settings::AgentSettings::get_global(cx); + let decision = decide_permission_for_path(tool_name, &path_str, settings); + + if let ToolPermissionDecision::Deny(reason) = decision { + return Task::ready(Err(anyhow!("{}", reason))); + } + + let path_owned = path.to_path_buf(); + let display_description = display_description.to_string(); + let tool_name = tool_name.to_string(); + let thread = thread.clone(); + let event_stream = event_stream.clone(); + + // The local settings folder check is synchronous (pure path inspection), + // so we can handle this common case without spawning. + let local_settings_folder = paths::local_settings_folder_name(); + let is_local_settings = path.components().any(|component| { + component.as_os_str() == <_ as AsRef>::as_ref(&local_settings_folder) + }); + + cx.spawn(async move |cx| { + // Resolve the path and check for symlink escapes. + let (project_entity, fs) = thread.read_with(cx, |thread, cx| { + let project = thread.project().clone(); + let fs = project.read(cx).fs().clone(); + (project, fs) + })?; + + let canonical_roots = canonicalize_worktree_roots(&project_entity, &fs, cx).await; + + let resolved = project_entity.read_with(cx, |project, cx| { + resolve_project_path(project, &path_owned, &canonical_roots, cx) + }); + + if let Ok(ResolvedProjectPath::SymlinkEscape { + canonical_target, .. + }) = &resolved + { + let authorize = cx.update(|cx| { + authorize_symlink_access( + &tool_name, + &path_owned.to_string_lossy(), + canonical_target, + &event_stream, + cx, + ) + }); + return authorize.await; + } + + // Create-mode paths may not resolve yet, so also inspect the parent path + // for symlink escapes before applying settings-based allow decisions. + if resolved.is_err() { + if let Some(parent_path) = path_owned.parent() { + let parent_resolved = project_entity.read_with(cx, |project, cx| { + resolve_project_path(project, parent_path, &canonical_roots, cx) + }); + + if let Ok(ResolvedProjectPath::SymlinkEscape { + canonical_target, .. + }) = &parent_resolved + { + let authorize = cx.update(|cx| { + authorize_symlink_access( + &tool_name, + &path_owned.to_string_lossy(), + canonical_target, + &event_stream, + cx, + ) + }); + return authorize.await; + } + } + } + + let explicitly_allowed = matches!(decision, ToolPermissionDecision::Allow); + + // Check sensitive settings asynchronously. + let settings_kind = if is_local_settings { + Some(SensitiveSettingsKind::Local) + } else { + sensitive_settings_kind(&path_owned, fs.as_ref()).await + }; + + let is_sensitive = settings_kind.is_some(); + if explicitly_allowed && !is_sensitive { + return Ok(()); + } + + match settings_kind { + Some(SensitiveSettingsKind::Local) => { + let authorize = cx.update(|cx| { + let context = ToolPermissionContext::new( + &tool_name, + vec![path_owned.to_string_lossy().to_string()], + ); + event_stream.authorize( + format!("{} (local settings)", display_description), + context, + cx, + ) + }); + return authorize.await; + } + Some(SensitiveSettingsKind::Global) => { + let authorize = cx.update(|cx| { + let context = ToolPermissionContext::new( + &tool_name, + vec![path_owned.to_string_lossy().to_string()], + ); + event_stream.authorize( + format!("{} (settings)", display_description), + context, + cx, + ) + }); + return authorize.await; + } + None => {} + } + + match resolved { + Ok(_) => Ok(()), + Err(_) => { + let authorize = cx.update(|cx| { + let context = ToolPermissionContext::new( + &tool_name, + vec![path_owned.to_string_lossy().to_string()], + ); + event_stream.authorize(&display_description, context, cx) + }); + authorize.await + } + } + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use fs::Fs; + use gpui::TestAppContext; + use project::{FakeFs, Project}; + use serde_json::json; + use settings::SettingsStore; + use util::path; + + fn init_test(cx: &mut TestAppContext) { + cx.update(|cx| { + let settings_store = SettingsStore::test(cx); + cx.set_global(settings_store); + }); + } + + async fn worktree_roots( + project: &Entity, + fs: &Arc, + cx: &TestAppContext, + ) -> Vec { + let abs_paths: Vec> = project.read_with(cx, |project, cx| { + project + .worktrees(cx) + .map(|wt| wt.read(cx).abs_path()) + .collect() + }); + + let mut roots = Vec::with_capacity(abs_paths.len()); + for p in &abs_paths { + match fs.canonicalize(p).await { + Ok(c) => roots.push(c), + Err(_) => roots.push(p.to_path_buf()), + } + } + roots + } + + #[gpui::test] + async fn test_resolve_project_path_safe_for_normal_files(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root/project"), + json!({ + "src": { + "main.rs": "fn main() {}", + "lib.rs": "pub fn hello() {}" + }, + "README.md": "# Project" + }), + ) + .await; + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.run_until_parked(); + let fs_arc: Arc = fs; + let roots = worktree_roots(&project, &fs_arc, cx).await; + + cx.read(|cx| { + let project = project.read(cx); + + let resolved = resolve_project_path(project, "project/src/main.rs", &roots, cx) + .expect("should resolve normal file"); + assert!( + matches!(resolved, ResolvedProjectPath::Safe(_)), + "normal file should be Safe, got: {:?}", + resolved + ); + + let resolved = resolve_project_path(project, "project/README.md", &roots, cx) + .expect("should resolve readme"); + assert!( + matches!(resolved, ResolvedProjectPath::Safe(_)), + "readme should be Safe, got: {:?}", + resolved + ); + }); + } + + #[gpui::test] + async fn test_resolve_project_path_detects_symlink_escape(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": { + "src": { + "main.rs": "fn main() {}" + } + }, + "external": { + "secret.txt": "top secret" + } + }), + ) + .await; + + fs.create_symlink(path!("/root/project/link").as_ref(), "../external".into()) + .await + .expect("should create symlink"); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.run_until_parked(); + let fs_arc: Arc = fs; + let roots = worktree_roots(&project, &fs_arc, cx).await; + + cx.read(|cx| { + let project = project.read(cx); + + let resolved = resolve_project_path(project, "project/link", &roots, cx) + .expect("should resolve symlink path"); + match &resolved { + ResolvedProjectPath::SymlinkEscape { + canonical_target, .. + } => { + assert_eq!( + canonical_target, + Path::new(path!("/root/external")), + "canonical target should point to external directory" + ); + } + ResolvedProjectPath::Safe(_) => { + panic!("symlink escaping project should be detected as SymlinkEscape"); + } + } + }); + } + + #[gpui::test] + async fn test_resolve_project_path_allows_intra_project_symlinks(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root/project"), + json!({ + "real_dir": { + "file.txt": "hello" + } + }), + ) + .await; + + fs.create_symlink(path!("/root/project/link_dir").as_ref(), "real_dir".into()) + .await + .expect("should create symlink"); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.run_until_parked(); + let fs_arc: Arc = fs; + let roots = worktree_roots(&project, &fs_arc, cx).await; + + cx.read(|cx| { + let project = project.read(cx); + + let resolved = resolve_project_path(project, "project/link_dir", &roots, cx) + .expect("should resolve intra-project symlink"); + assert!( + matches!(resolved, ResolvedProjectPath::Safe(_)), + "intra-project symlink should be Safe, got: {:?}", + resolved + ); + }); + } + + #[gpui::test] + async fn test_resolve_project_path_missing_child_under_external_symlink( + cx: &mut TestAppContext, + ) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "project": {}, + "external": { + "existing.txt": "hello" + } + }), + ) + .await; + + fs.create_symlink(path!("/root/project/link").as_ref(), "../external".into()) + .await + .expect("should create symlink"); + + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.run_until_parked(); + let fs_arc: Arc = fs; + let roots = worktree_roots(&project, &fs_arc, cx).await; + + cx.read(|cx| { + let project = project.read(cx); + + let resolved = resolve_project_path(project, "project/link/new_dir", &roots, cx) + .expect("should resolve missing child path under symlink"); + match resolved { + ResolvedProjectPath::SymlinkEscape { + canonical_target, .. + } => { + assert_eq!( + canonical_target, + Path::new(path!("/root/external/new_dir")), + "missing child path should resolve to escaped canonical target", + ); + } + ResolvedProjectPath::Safe(_) => { + panic!("missing child under external symlink should be SymlinkEscape"); + } + } + }); + } + + #[gpui::test] + async fn test_resolve_project_path_allows_cross_worktree_symlinks(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "worktree_one": {}, + "worktree_two": { + "shared_dir": { + "file.txt": "hello" + } + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/worktree_one/link_to_worktree_two").as_ref(), + PathBuf::from("../worktree_two/shared_dir"), + ) + .await + .expect("should create symlink"); + + let project = Project::test( + fs.clone(), + [ + path!("/root/worktree_one").as_ref(), + path!("/root/worktree_two").as_ref(), + ], + cx, + ) + .await; + cx.run_until_parked(); + let fs_arc: Arc = fs; + let roots = worktree_roots(&project, &fs_arc, cx).await; + + cx.read(|cx| { + let project = project.read(cx); + + let resolved = + resolve_project_path(project, "worktree_one/link_to_worktree_two", &roots, cx) + .expect("should resolve cross-worktree symlink"); + assert!( + matches!(resolved, ResolvedProjectPath::Safe(_)), + "cross-worktree symlink should be Safe, got: {:?}", + resolved + ); + }); + } + + #[gpui::test] + async fn test_resolve_project_path_missing_child_under_cross_worktree_symlink( + cx: &mut TestAppContext, + ) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/root"), + json!({ + "worktree_one": {}, + "worktree_two": { + "shared_dir": {} + } + }), + ) + .await; + + fs.create_symlink( + path!("/root/worktree_one/link_to_worktree_two").as_ref(), + PathBuf::from("../worktree_two/shared_dir"), + ) + .await + .expect("should create symlink"); + + let project = Project::test( + fs.clone(), + [ + path!("/root/worktree_one").as_ref(), + path!("/root/worktree_two").as_ref(), + ], + cx, + ) + .await; + cx.run_until_parked(); + let fs_arc: Arc = fs; + let roots = worktree_roots(&project, &fs_arc, cx).await; + + cx.read(|cx| { + let project = project.read(cx); + + let resolved = resolve_project_path( + project, + "worktree_one/link_to_worktree_two/new_dir", + &roots, + cx, + ) + .expect("should resolve missing child under cross-worktree symlink"); + assert!( + matches!(resolved, ResolvedProjectPath::Safe(_)), + "missing child under cross-worktree symlink should be Safe, got: {:?}", + resolved + ); + }); + } +} diff --git a/crates/agent/src/tools/web_search_tool.rs b/crates/agent/src/tools/web_search_tool.rs index c01486c1816137fa0ccc53478a60825a08d469cb..2c92bc0b34b9dba252a8cd6884558622553eccb3 100644 --- a/crates/agent/src/tools/web_search_tool.rs +++ b/crates/agent/src/tools/web_search_tool.rs @@ -84,10 +84,8 @@ impl AgentTool for WebSearchTool { return Task::ready(Err(anyhow!("{}", reason))); } ToolPermissionDecision::Confirm => { - let context = crate::ToolPermissionContext { - tool_name: Self::NAME.to_string(), - input_values: vec![input.query.clone()], - }; + let context = + crate::ToolPermissionContext::new(Self::NAME, vec![input.query.clone()]); Some(event_stream.authorize( format!("Search the web for {}", MarkdownInlineCode(&input.query)), context,