Improve sandbox write-path handling

crates/acp_thread/src/acp_thread.rs

@@ -79,6 +79,41 @@ pub const SUBAGENT_SESSION_INFO_META_KEY: &str = "subagent_session_info";
 
 pub const SANDBOX_AUTHORIZATION_META_KEY: &str = "sandbox_authorization";
 
+/// Stable `PermissionOption` ids for the sandbox-escalation approval prompt.
+///
+/// These are shared across the option construction (in the agent), the outcome
+/// dispatch, and the UI so the distinct grant lifetimes stay in sync. Note
+/// that `AllowThread` and `AllowAlways` both use
+/// `PermissionOptionKind::AllowAlways`; the id is what distinguishes them.
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub enum SandboxPermission {
+    AllowOnce,
+    AllowThread,
+    AllowAlways,
+    Deny,
+}
+
+impl SandboxPermission {
+    pub fn as_id(self) -> &'static str {
+        match self {
+            Self::AllowOnce => "allow",
+            Self::AllowThread => "allow_thread",
+            Self::AllowAlways => "allow_always",
+            Self::Deny => "deny",
+        }
+    }
+
+    pub fn from_id(id: &str) -> Option<Self> {
+        match id {
+            "allow" => Some(Self::AllowOnce),
+            "allow_thread" => Some(Self::AllowThread),
+            "allow_always" => Some(Self::AllowAlways),
+            "deny" => Some(Self::Deny),
+            _ => None,
+        }
+    }
+}
+
 #[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)]
 pub struct SandboxAuthorizationDetails {
     #[serde(default)]

crates/agent/src/sandboxing.rs

@@ -98,10 +98,13 @@ impl ThreadSandboxGrants {
             return true;
         }
         request.write_paths.iter().all(|requested| {
-            self.write_paths
-                .iter()
-                .chain(persistent.write_paths.iter())
-                .any(|granted| requested.starts_with(granted))
+            util::paths::path_within_subtree(
+                requested,
+                self.write_paths
+                    .iter()
+                    .chain(persistent.write_paths.iter())
+                    .map(PathBuf::as_path),
+            )
         })
     }
 
@@ -112,7 +115,7 @@ impl ThreadSandboxGrants {
         self.allow_fs_write_all |= request.allow_fs_write_all;
         self.unsandboxed |= request.unsandboxed;
         for path in &request.write_paths {
-            add_write_path(&mut self.write_paths, path);
+            util::paths::insert_subtree(&mut self.write_paths, path.clone());
         }
     }
 
@@ -131,11 +134,8 @@ impl ThreadSandboxGrants {
         persistent: &SandboxPermissions,
     ) -> SandboxRequest {
         let mut write_paths = persistent.write_paths.clone();
-        for path in &self.write_paths {
-            add_write_path(&mut write_paths, path);
-        }
-        for path in &request.write_paths {
-            add_write_path(&mut write_paths, path);
+        for path in self.write_paths.iter().chain(request.write_paths.iter()) {
+            util::paths::insert_subtree(&mut write_paths, path.clone());
         }
         SandboxRequest {
             network: persistent.allow_network || self.network || request.network,
@@ -148,17 +148,6 @@ impl ThreadSandboxGrants {
     }
 }
 
-/// Insert `path` into a set of write-grant subtrees, keeping it minimal:
-/// a no-op if already covered by a broader grant, otherwise added with any
-/// now-subsumed child grants pruned.
-fn add_write_path(write_paths: &mut Vec<PathBuf>, path: &std::path::Path) {
-    if write_paths.iter().any(|granted| path.starts_with(granted)) {
-        return;
-    }
-    write_paths.retain(|granted| !granted.starts_with(path));
-    write_paths.push(path.to_path_buf());
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;

crates/agent/src/thread.rs

@@ -1399,6 +1399,12 @@ impl Thread {
                 }
             });
 
+        // Recorded tool calls use the model-facing name, so a terminal call is
+        // always keyed as `terminal` and resolves to the non-sandboxed
+        // `TerminalTool` here, even if it originally ran under
+        // `SandboxedTerminalTool`. That's safe because both variants share the
+        // same `replay` behavior; replay only reconstructs UI state and never
+        // re-runs the command or re-applies sandbox policy.
         let tool = self.tools.get(tool_use.name.as_ref()).cloned().or_else(|| {
             self.context_server_registry
                 .read(cx)
@@ -4660,22 +4666,22 @@ impl ToolCallEventStream {
         };
         let options = acp_thread::PermissionOptions::Flat(vec![
             acp::PermissionOption::new(
-                acp::PermissionOptionId::new("allow"),
+                acp::PermissionOptionId::new(acp_thread::SandboxPermission::AllowOnce.as_id()),
                 "Allow once",
                 acp::PermissionOptionKind::AllowOnce,
             ),
             acp::PermissionOption::new(
-                acp::PermissionOptionId::new("allow_thread"),
+                acp::PermissionOptionId::new(acp_thread::SandboxPermission::AllowThread.as_id()),
                 "Allow for this thread",
                 acp::PermissionOptionKind::AllowAlways,
             ),
             acp::PermissionOption::new(
-                acp::PermissionOptionId::new("allow_always"),
+                acp::PermissionOptionId::new(acp_thread::SandboxPermission::AllowAlways.as_id()),
                 "Allow always",
                 acp::PermissionOptionKind::AllowAlways,
             ),
             acp::PermissionOption::new(
-                acp::PermissionOptionId::new("deny"),
+                acp::PermissionOptionId::new(acp_thread::SandboxPermission::Deny.as_id()),
                 "Deny",
                 acp::PermissionOptionKind::RejectOnce,
             ),
@@ -4778,19 +4784,22 @@ impl ToolCallEventStream {
             "unexpected params for sandbox permission"
         );
 
-        match outcome.option_id.0.as_ref() {
-            "allow" => Ok(()),
-            "allow_thread" => {
+        match acp_thread::SandboxPermission::from_id(outcome.option_id.0.as_ref()) {
+            Some(acp_thread::SandboxPermission::AllowOnce) => Ok(()),
+            Some(acp_thread::SandboxPermission::AllowThread) => {
                 sandbox_grants.borrow_mut().record(request);
                 Ok(())
             }
-            "allow_always" => {
+            Some(acp_thread::SandboxPermission::AllowAlways) => {
                 sandbox_grants.borrow_mut().record(request);
                 Self::persist_sandbox_always_permission(request, fs, cx);
                 Ok(())
             }
-            "deny" => Err(anyhow!("Permission to run tool denied by user")),
-            other => {
+            Some(acp_thread::SandboxPermission::Deny) => {
+                Err(anyhow!("Permission to run tool denied by user"))
+            }
+            None => {
+                let other = outcome.option_id.0.as_ref();
                 debug_assert!(false, "unexpected sandbox permission option_id: {other}");
                 Err(anyhow!("Permission to run tool denied by user"))
             }
@@ -4803,6 +4812,9 @@ impl ToolCallEventStream {
         cx: &AsyncApp,
     ) {
         let Some(fs) = fs else {
+            log::error!(
+                "Cannot persist \"allow always\" sandbox permission: no filesystem available"
+            );
             return;
         };
 

crates/agent/src/tools/terminal_tool.rs

@@ -489,16 +489,22 @@ fn resolve_write_paths(
 
 /// Pure path-joining step of [`resolve_write_paths`], split out so it can be
 /// unit-tested without a `Project`/`App`.
+///
+/// Each path is lexically normalized (resolving `.`/`..`) so that later
+/// subtree-containment checks and the user-facing approval prompt operate on
+/// the same path the sandbox will ultimately enforce. Relative paths with no
+/// base, and paths that traverse above the filesystem root, are dropped.
 fn join_write_paths(raw_paths: &[String], base: Option<&Path>) -> Vec<PathBuf> {
     raw_paths
         .iter()
         .filter_map(|raw| {
             let path = Path::new(raw);
-            if path.is_absolute() {
-                Some(path.to_path_buf())
+            let absolute = if path.is_absolute() {
+                path.to_path_buf()
             } else {
-                base.map(|base| base.join(path))
-            }
+                base?.join(path)
+            };
+            util::paths::normalize_lexically(&absolute).ok()
         })
         .collect()
 }
@@ -2300,6 +2306,40 @@ mod tests {
         assert_eq!(joined, vec![PathBuf::from(abs)]);
     }
 
+    #[test]
+    fn test_join_write_paths_normalizes_parent_traversal() {
+        let base = PathBuf::from(if cfg!(windows) {
+            "C:\\project"
+        } else {
+            "/project"
+        });
+        // `..` is resolved lexically so containment checks and the approval
+        // prompt see the real target rather than a traversal that the sandbox
+        // would canonicalize differently.
+        let joined = join_write_paths(
+            &[
+                "build/../../escape".to_string(),
+                if cfg!(windows) {
+                    "C:\\abs\\a\\..\\b".to_string()
+                } else {
+                    "/abs/a/../b".to_string()
+                },
+            ],
+            Some(base.as_path()),
+        );
+        let expected_escape = if cfg!(windows) {
+            PathBuf::from("C:\\escape")
+        } else {
+            PathBuf::from("/escape")
+        };
+        let expected_abs = if cfg!(windows) {
+            PathBuf::from("C:\\abs\\b")
+        } else {
+            PathBuf::from("/abs/b")
+        };
+        assert_eq!(joined, vec![expected_escape, expected_abs]);
+    }
+
     #[test]
     fn test_sandbox_approval_title_unsandboxed() {
         let mut request = sandbox_request(true, true, &["/tmp/build"]);

crates/agent_settings/src/agent_settings.rs

@@ -339,6 +339,13 @@ impl Default for AgentProfileId {
     }
 }
 
+/// Persistent "allow always" sandbox grants for agent-run terminal commands.
+///
+/// Coverage decisions for these grants are made in
+/// `agent::sandboxing::ThreadSandboxGrants::covers_with_persistent`, which
+/// combines them with the in-memory per-thread grants. `write_paths` are
+/// stored as minimal, lexically-normalized subtrees (see
+/// [`compile_sandbox_permissions`]).
 #[derive(Clone, Debug, Default, PartialEq, Eq)]
 pub struct SandboxPermissions {
     pub allow_network: bool,
@@ -347,34 +354,6 @@ pub struct SandboxPermissions {
     pub write_paths: Vec<PathBuf>,
 }
 
-impl SandboxPermissions {
-    pub fn covers(
-        &self,
-        network: bool,
-        allow_fs_write_all: bool,
-        unsandboxed: bool,
-        write_paths: &[PathBuf],
-    ) -> bool {
-        if unsandboxed {
-            return self.allow_unsandboxed;
-        }
-        if network && !self.allow_network {
-            return false;
-        }
-        if allow_fs_write_all && !self.allow_fs_write_all {
-            return false;
-        }
-        if self.allow_fs_write_all {
-            return true;
-        }
-        write_paths.iter().all(|requested| {
-            self.write_paths
-                .iter()
-                .any(|granted| requested.starts_with(granted))
-        })
-    }
-}
-
 #[derive(Clone, Debug, Default)]
 pub struct ToolPermissions {
     /// Global default permission when no tool-specific rules or patterns match.
@@ -728,7 +707,11 @@ fn compile_sandbox_permissions(
 
     let mut write_paths = Vec::new();
     for path in content.write_paths.map(|paths| paths.0).unwrap_or_default() {
-        add_sandbox_write_path(&mut write_paths, &path);
+        // Normalize away `..`/`.` before storing, since coverage checks are
+        // purely lexical; drop paths that escape the filesystem root.
+        if let Ok(normalized) = util::paths::normalize_lexically(&path) {
+            util::paths::insert_subtree(&mut write_paths, normalized);
+        }
     }
 
     SandboxPermissions {
@@ -739,14 +722,6 @@ fn compile_sandbox_permissions(
     }
 }
 
-fn add_sandbox_write_path(write_paths: &mut Vec<PathBuf>, path: &Path) {
-    if write_paths.iter().any(|granted| path.starts_with(granted)) {
-        return;
-    }
-    write_paths.retain(|granted| !granted.starts_with(path));
-    write_paths.push(path.to_path_buf());
-}
-
 fn compile_tool_permissions(content: Option<settings::ToolPermissionsContent>) -> ToolPermissions {
     let Some(content) = content else {
         return ToolPermissions::default();
@@ -925,10 +900,6 @@ mod tests {
     fn test_sandbox_permissions_empty() {
         let permissions = compile_sandbox_permissions(None);
         assert_eq!(permissions, SandboxPermissions::default());
-        assert!(!permissions.covers(true, false, false, &[]));
-        assert!(!permissions.covers(false, true, false, &[]));
-        assert!(!permissions.covers(false, false, true, &[]));
-        assert!(!permissions.covers(false, false, false, &[PathBuf::from("/tmp/build")]));
     }
 
     #[test]
@@ -953,20 +924,23 @@ mod tests {
             permissions.write_paths,
             vec![PathBuf::from("/tmp/build"), PathBuf::from("/var/log")]
         );
-        assert!(permissions.covers(true, false, true, &[PathBuf::from("/tmp/build/cache")]))
     }
 
     #[test]
-    fn test_sandbox_permissions_all_write_covers_paths() {
+    fn test_sandbox_permissions_normalizes_and_prunes_parent_traversal() {
         let json = json!({
-            "allow_fs_write_all": true,
+            "write_paths": [
+                "/tmp/build/../build/cache",
+                "/tmp/build",
+            ]
         });
 
         let content: settings::SandboxPermissionsContent = serde_json::from_value(json).unwrap();
         let permissions = compile_sandbox_permissions(Some(content));
 
-        assert!(permissions.covers(false, true, false, &[]));
-        assert!(permissions.covers(false, false, false, &[PathBuf::from("/anywhere")]))
+        // `/tmp/build/../build/cache` normalizes to `/tmp/build/cache`, which is
+        // then pruned as a redundant child of `/tmp/build`.
+        assert_eq!(permissions.write_paths, vec![PathBuf::from("/tmp/build")]);
     }
 
     #[test]

crates/agent_ui/src/conversation_view.rs

@@ -459,9 +459,9 @@ fn permission_option_for_action(
 ) -> Option<&acp::PermissionOption> {
     if kind == acp::PermissionOptionKind::AllowAlways
         && let PermissionOptions::Flat(options) = options
-        && let Some(option) = options
-            .iter()
-            .find(|option| option.option_id.0.as_ref() == "allow_always")
+        && let Some(option) = options.iter().find(|option| {
+            option.option_id.0.as_ref() == acp_thread::SandboxPermission::AllowAlways.as_id()
+        })
     {
         return Some(option);
     }

crates/agent_ui/src/conversation_view/thread_view.rs

@@ -7960,7 +7960,9 @@ impl ThreadView {
                                 Icon::new(IconName::CheckDouble)
                                     .size(IconSize::XSmall)
                                     .color(Color::Success),
-                                if option.option_id.0.as_ref() == "allow_thread" {
+                                if option.option_id.0.as_ref()
+                                    == acp_thread::SandboxPermission::AllowThread.as_id()
+                                {
                                     None
                                 } else {
                                     Some(&AllowAlways as &dyn Action)