Harden tool authorization: sensitive settings, deferred ops, copy/move patterns

crates/agent/src/pattern_extraction.rs

@@ -1,6 +1,12 @@
 use crate::shell_parser::extract_commands;
+use std::path::{Path, PathBuf};
 use url::Url;
 
+/// Normalize path separators to forward slashes for consistent cross-platform patterns.
+fn normalize_separators(path_str: &str) -> String {
+    path_str.replace('\\', "/")
+}
+
 /// Extracts the command name from a shell command using the shell parser.
 ///
 /// This parses the command properly to extract just the command name (first word),
@@ -41,23 +47,64 @@ pub fn extract_terminal_pattern_display(command: &str) -> Option<String> {
 }
 
 pub fn extract_path_pattern(path: &str) -> Option<String> {
-    let parent = std::path::Path::new(path).parent()?;
-    let parent_str = parent.to_str()?;
+    let parent = Path::new(path).parent()?;
+    let parent_str = normalize_separators(parent.to_str()?);
     if parent_str.is_empty() || parent_str == "/" {
         return None;
     }
-    Some(format!("^{}/", regex::escape(parent_str)))
+    Some(format!("^{}/", regex::escape(&parent_str)))
 }
 
 pub fn extract_path_pattern_display(path: &str) -> Option<String> {
-    let parent = std::path::Path::new(path).parent()?;
-    let parent_str = parent.to_str()?;
+    let parent = Path::new(path).parent()?;
+    let parent_str = normalize_separators(parent.to_str()?);
     if parent_str.is_empty() || parent_str == "/" {
         return None;
     }
     Some(format!("{}/", parent_str))
 }
 
+fn common_parent_dir(path_a: &str, path_b: &str) -> Option<PathBuf> {
+    let parent_a = Path::new(path_a).parent()?;
+    let parent_b = Path::new(path_b).parent()?;
+
+    let components_a: Vec<_> = parent_a.components().collect();
+    let components_b: Vec<_> = parent_b.components().collect();
+
+    let common_count = components_a
+        .iter()
+        .zip(components_b.iter())
+        .take_while(|(a, b)| a == b)
+        .count();
+
+    if common_count == 0 {
+        return None;
+    }
+
+    let common: PathBuf = components_a[..common_count].iter().collect();
+    Some(common)
+}
+
+pub fn extract_copy_move_pattern(input: &str) -> Option<String> {
+    let (source, dest) = input.split_once('\n')?;
+    let common = common_parent_dir(source, dest)?;
+    let common_str = normalize_separators(common.to_str()?);
+    if common_str.is_empty() || common_str == "/" {
+        return None;
+    }
+    Some(format!("^{}/", regex::escape(&common_str)))
+}
+
+pub fn extract_copy_move_pattern_display(input: &str) -> Option<String> {
+    let (source, dest) = input.split_once('\n')?;
+    let common = common_parent_dir(source, dest)?;
+    let common_str = normalize_separators(common.to_str()?);
+    if common_str.is_empty() || common_str == "/" {
+        return None;
+    }
+    Some(format!("{}/", common_str))
+}
+
 pub fn extract_url_pattern(url: &str) -> Option<String> {
     let parsed = Url::parse(url).ok()?;
     let domain = parsed.host_str()?;
@@ -170,4 +217,56 @@ mod tests {
             Some("^https?://test\\.example\\.com".to_string())
         );
     }
+
+    #[test]
+    fn test_extract_copy_move_pattern_same_directory() {
+        assert_eq!(
+            extract_copy_move_pattern(
+                "/Users/alice/project/src/old.rs\n/Users/alice/project/src/new.rs"
+            ),
+            Some("^/Users/alice/project/src/".to_string())
+        );
+    }
+
+    #[test]
+    fn test_extract_copy_move_pattern_sibling_directories() {
+        assert_eq!(
+            extract_copy_move_pattern(
+                "/Users/alice/project/src/old.rs\n/Users/alice/project/dst/new.rs"
+            ),
+            Some("^/Users/alice/project/".to_string())
+        );
+    }
+
+    #[test]
+    fn test_extract_copy_move_pattern_no_common_prefix() {
+        assert_eq!(
+            extract_copy_move_pattern("/home/file.txt\n/tmp/file.txt"),
+            None
+        );
+    }
+
+    #[test]
+    fn test_extract_copy_move_pattern_relative_paths() {
+        assert_eq!(
+            extract_copy_move_pattern("src/old.rs\nsrc/new.rs"),
+            Some("^src/".to_string())
+        );
+    }
+
+    #[test]
+    fn test_extract_copy_move_pattern_display() {
+        assert_eq!(
+            extract_copy_move_pattern_display(
+                "/Users/alice/project/src/old.rs\n/Users/alice/project/dst/new.rs"
+            ),
+            Some("/Users/alice/project/".to_string())
+        );
+    }
+
+    #[test]
+    fn test_extract_copy_move_pattern_no_arrow() {
+        assert_eq!(extract_copy_move_pattern("just/a/path.rs"), None);
+        assert_eq!(extract_copy_move_pattern_display("just/a/path.rs"), None);
+    }
 }

crates/agent/src/thread.rs

@@ -675,9 +675,16 @@ impl ToolPermissionContext {
                 extract_terminal_pattern(input_value),
                 extract_terminal_pattern_display(input_value),
             )
+        } else if tool_name == CopyPathTool::NAME || tool_name == MovePathTool::NAME {
+            // input_value is "source\ndestination"; extract a pattern from the
+            // common parent directory of both paths so that "always allow" covers
+            // future checks against both the source and the destination.
+            (
+                extract_copy_move_pattern(input_value),
+                extract_copy_move_pattern_display(input_value),
+            )
         } else if tool_name == EditFileTool::NAME
             || tool_name == DeletePathTool::NAME
-            || tool_name == MovePathTool::NAME
             || tool_name == CreateDirectoryTool::NAME
             || tool_name == SaveFileTool::NAME
         {

crates/agent/src/tools/copy_path_tool.rs

@@ -1,15 +1,17 @@
-use crate::{
-    AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_from_settings,
+use super::edit_file_tool::{
+    SensitiveSettingsKind, is_sensitive_settings_path, sensitive_settings_kind,
 };
+use crate::{AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_for_path};
 use agent_client_protocol::ToolKind;
 use agent_settings::AgentSettings;
 use anyhow::{Context as _, Result, anyhow};
 use futures::FutureExt as _;
-use gpui::{App, AppContext, Entity, Task};
+use gpui::{App, Entity, Task};
 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;
 
@@ -83,57 +85,71 @@ impl AgentTool for CopyPathTool {
     ) -> Task<Result<Self::Output>> {
         let settings = AgentSettings::get_global(cx);
 
-        let source_decision =
-            decide_permission_from_settings(Self::NAME, &input.source_path, settings);
+        let source_decision = decide_permission_for_path(Self::NAME, &input.source_path, settings);
         if let ToolPermissionDecision::Deny(reason) = source_decision {
             return Task::ready(Err(anyhow!("{}", reason)));
         }
 
         let dest_decision =
-            decide_permission_from_settings(Self::NAME, &input.destination_path, settings);
+            decide_permission_for_path(Self::NAME, &input.destination_path, settings);
         if let ToolPermissionDecision::Deny(reason) = dest_decision {
             return Task::ready(Err(anyhow!("{}", reason)));
         }
 
         let needs_confirmation = matches!(source_decision, ToolPermissionDecision::Confirm)
-            || matches!(dest_decision, ToolPermissionDecision::Confirm);
+            || matches!(dest_decision, ToolPermissionDecision::Confirm)
+            || (!settings.always_allow_tool_actions
+                && matches!(source_decision, ToolPermissionDecision::Allow)
+                && is_sensitive_settings_path(Path::new(&input.source_path)))
+            || (!settings.always_allow_tool_actions
+                && matches!(dest_decision, ToolPermissionDecision::Allow)
+                && is_sensitive_settings_path(Path::new(&input.destination_path)));
 
         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_value: input.source_path.clone(),
+                input_value: format!("{}\n{}", input.source_path, input.destination_path),
             };
-            Some(event_stream.authorize(format!("Copy {src} to {dest}"), context, cx))
+            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,
+            };
+            Some(event_stream.authorize(title, context, cx))
         } else {
             None
         };
 
-        let copy_task = self.project.update(cx, |project, cx| {
-            match project
-                .find_project_path(&input.source_path, cx)
-                .and_then(|project_path| project.entry_for_path(&project_path, cx))
-            {
-                Some(entity) => match project.find_project_path(&input.destination_path, cx) {
-                    Some(project_path) => project.copy_entry(entity.id, project_path, cx),
-                    None => Task::ready(Err(anyhow!(
-                        "Destination path {} was outside the project.",
-                        input.destination_path
-                    ))),
-                },
-                None => Task::ready(Err(anyhow!(
-                    "Source path {} was not found in the project.",
-                    input.source_path
-                ))),
-            }
-        });
-
-        cx.background_spawn(async move {
+        let project = self.project.clone();
+        cx.spawn(async move |cx| {
             if let Some(authorize) = authorize {
                 authorize.await?;
             }
 
+            let copy_task = project.update(cx, |project, cx| {
+                match project
+                    .find_project_path(&input.source_path, cx)
+                    .and_then(|project_path| project.entry_for_path(&project_path, cx))
+                {
+                    Some(entity) => match project.find_project_path(&input.destination_path, cx) {
+                        Some(project_path) => Ok(project.copy_entry(entity.id, project_path, cx)),
+                        None => Err(anyhow!(
+                            "Destination path {} was outside the project.",
+                            input.destination_path
+                        )),
+                    },
+                    None => Err(anyhow!(
+                        "Source path {} was not found in the project.",
+                        input.source_path
+                    )),
+                }
+            })?;
+
             let result = futures::select! {
                 result = copy_task.fuse() => result,
                 _ = event_stream.cancelled_by_user().fuse() => {

crates/agent/src/tools/create_directory_tool.rs

@@ -1,3 +1,4 @@
+use super::edit_file_tool::{SensitiveSettingsKind, sensitive_settings_kind};
 use agent_client_protocol::ToolKind;
 use agent_settings::AgentSettings;
 use anyhow::{Context as _, Result, anyhow};
@@ -7,12 +8,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_from_settings,
-};
+use crate::{AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_for_path};
 
 /// Creates a new directory at the specified path within the project. Returns confirmation that the directory was created.
 ///
@@ -71,43 +71,51 @@ impl AgentTool for CreateDirectoryTool {
         cx: &mut App,
     ) -> Task<Result<Self::Output>> {
         let settings = AgentSettings::get_global(cx);
-        let decision = decide_permission_from_settings(Self::NAME, &input.path, settings);
+        let mut decision = decide_permission_for_path(Self::NAME, &input.path, settings);
+        let sensitive_kind = sensitive_settings_kind(Path::new(&input.path));
+
+        if matches!(decision, ToolPermissionDecision::Allow)
+            && !settings.always_allow_tool_actions
+            && sensitive_kind.is_some()
+        {
+            decision = ToolPermissionDecision::Confirm;
+        }
 
         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_value: input.path.clone(),
                 };
-                Some(event_stream.authorize(
-                    format!("Create directory {}", MarkdownInlineCode(&input.path)),
-                    context,
-                    cx,
-                ))
+                Some(event_stream.authorize(title, context, cx))
             }
         };
 
-        let project_path = match self.project.read(cx).find_project_path(&input.path, cx) {
-            Some(project_path) => project_path,
-            None => {
-                return Task::ready(Err(anyhow!("Path to create was outside the project")));
-            }
-        };
         let destination_path: Arc<str> = input.path.as_str().into();
 
-        let create_entry = self.project.update(cx, |project, cx| {
-            project.create_entry(project_path.clone(), true, cx)
-        });
-
-        cx.spawn(async move |_cx| {
+        let project = self.project.clone();
+        cx.spawn(async move |cx| {
             if let Some(authorize) = authorize {
                 authorize.await?;
             }
 
+            let create_entry = project.update(cx, |project, cx| {
+                match project.find_project_path(&input.path, cx) {
+                    Some(project_path) => Ok(project.create_entry(project_path, true, cx)),
+                    None => Err(anyhow!("Path to create was outside the project")),
+                }
+            })?;
+
             futures::select! {
                 result = create_entry.fuse() => {
                     result.with_context(|| format!("Creating directory {destination_path}"))?;