From 6d9ed1fc35e97c53e1136a30ba0cb6cf6914634e Mon Sep 17 00:00:00 2001 From: MartinYe1234 <52641447+MartinYe1234@users.noreply.github.com> Date: Mon, 1 Jun 2026 18:07:05 -0700 Subject: [PATCH] Add granular per-path write permissions for the agent terminal sandbox (#57972) Adds granular, per-path write permissions to the agent's sandboxed `terminal` tool. Previously a command could only request unrestricted filesystem writes (`allow_fs_write: true`) as an all-or-nothing escape hatch. Now the model can request write access to specific paths via `fs_write_paths`, and each granted directory covers its whole subtree. The broad escape hatch is still available as `allow_fs_write_all: true` for cases where the paths can't be enumerated up front. Approval prompts now let the user grant a write request either just for that command or for the rest of the conversation. Conversation grants are tracked in memory (never persisted) using pure subtree containment, so later commands writing under an already-granted path run without prompting again. Closes AI-335 Release Notes: - Improved the agent's terminal sandbox so commands can request write access to specific paths instead of all-or-nothing filesystem access, with the option to grant access for a single command or the rest of the conversation. --------- Co-authored-by: Anant Goel Co-authored-by: Richard Feldman --- crates/acp_thread/src/acp_thread.rs | 38 + crates/acp_thread/src/terminal.rs | 9 +- crates/agent/src/sandboxing.rs | 336 +++++++++ crates/agent/src/templates.rs | 7 +- crates/agent/src/templates/system_prompt.hbs | 13 +- crates/agent/src/thread.rs | 348 ++++++++- crates/agent/src/tool_permissions.rs | 1 + crates/agent/src/tools/terminal_tool.rs | 708 +++++++++++++++--- crates/agent_settings/src/agent_settings.rs | 116 ++- crates/agent_ui/src/agent_ui.rs | 1 + crates/agent_ui/src/conversation_view.rs | 54 +- .../src/conversation_view/thread_view.rs | 108 ++- crates/settings_content/src/agent.rs | 107 +++ 13 files changed, 1727 insertions(+), 119 deletions(-) diff --git a/crates/acp_thread/src/acp_thread.rs b/crates/acp_thread/src/acp_thread.rs index f1025494d6..6a8f509f75 100644 --- a/crates/acp_thread/src/acp_thread.rs +++ b/crates/acp_thread/src/acp_thread.rs @@ -77,6 +77,35 @@ pub fn meta_with_tool_name(tool_name: &str) -> acp::Meta { /// Key used in ACP ToolCall meta to store the session id and message indexes pub const SUBAGENT_SESSION_INFO_META_KEY: &str = "subagent_session_info"; +pub const SANDBOX_AUTHORIZATION_META_KEY: &str = "sandbox_authorization"; + +#[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)] +pub struct SandboxAuthorizationDetails { + #[serde(default)] + pub network: bool, + #[serde(default)] + pub allow_fs_write_all: bool, + #[serde(default)] + pub unsandboxed: bool, + #[serde(default)] + pub write_paths: Vec, +} + +pub fn meta_with_sandbox_authorization(details: SandboxAuthorizationDetails) -> acp::Meta { + acp::Meta::from_iter([( + SANDBOX_AUTHORIZATION_META_KEY.into(), + serde_json::to_value(details).unwrap_or_default(), + )]) +} + +pub fn sandbox_authorization_details_from_meta( + meta: &Option, +) -> Option { + meta.as_ref() + .and_then(|m| m.get(SANDBOX_AUTHORIZATION_META_KEY)) + .and_then(|v| serde_json::from_value(v.clone()).ok()) +} + #[derive(Clone, Debug, Deserialize, Serialize)] pub struct SubagentSessionInfo { /// The session id of the subagent sessiont that was spawned @@ -268,6 +297,7 @@ pub struct ToolCall { pub raw_output: Option, pub tool_name: Option, pub subagent_session_info: Option, + pub sandbox_authorization_details: Option, } impl ToolCall { @@ -309,6 +339,8 @@ impl ToolCall { let tool_name = tool_name_from_meta(&tool_call.meta); let subagent_session_info = subagent_session_info_from_meta(&tool_call.meta); + let sandbox_authorization_details = + sandbox_authorization_details_from_meta(&tool_call.meta); let label = if tool_call.kind == acp::ToolKind::Execute { cx.new(|cx| Markdown::new_text(title.into(), cx)) @@ -329,6 +361,7 @@ impl ToolCall { raw_output: tool_call.raw_output, tool_name, subagent_session_info, + sandbox_authorization_details, }; Ok(result) } @@ -364,6 +397,10 @@ impl ToolCall { if let Some(subagent_session_info) = subagent_session_info_from_meta(&meta) { self.subagent_session_info = Some(subagent_session_info); } + if let Some(sandbox_authorization_details) = sandbox_authorization_details_from_meta(&meta) + { + self.sandbox_authorization_details = Some(sandbox_authorization_details); + } if let Some(title) = title { if self.kind == acp::ToolKind::Execute { @@ -1956,6 +1993,7 @@ impl AcpThread { raw_output: None, tool_name: None, subagent_session_info: None, + sandbox_authorization_details: None, }; self.push_entry(AgentThreadEntry::ToolCall(failed_tool_call), cx); return Ok(()); diff --git a/crates/acp_thread/src/terminal.rs b/crates/acp_thread/src/terminal.rs index 32496b8f44..ffe3108a7f 100644 --- a/crates/acp_thread/src/terminal.rs +++ b/crates/acp_thread/src/terminal.rs @@ -35,9 +35,15 @@ pub struct SandboxWrap { /// here — *not* the command's working directory, which is model- /// controlled and would let the model widen its own writable scope. pub writable_paths: Vec, + /// Additional write subtrees the user explicitly approved for this + /// command (per-path write grants). Kept separate from `writable_paths` + /// to make the trust boundary explicit: these originate from + /// model-requested paths that passed a user-approval prompt. They are + /// merged with `writable_paths` when generating the sandbox policy. + pub extra_write_paths: Vec, /// Allow outbound network access for this command. pub allow_network: bool, - /// Allow unrestricted filesystem writes (ignores `writable_paths`). + /// Allow unrestricted filesystem writes (ignores all writable paths). pub allow_fs_write: bool, } @@ -70,6 +76,7 @@ pub(crate) fn apply_sandbox_wrap( let writable: Vec<&std::path::Path> = sandbox_wrap .writable_paths .iter() + .chain(sandbox_wrap.extra_write_paths.iter()) .map(|p| p.as_path()) .collect(); let permissions = sandbox::macos_seatbelt::SandboxPermissions { diff --git a/crates/agent/src/sandboxing.rs b/crates/agent/src/sandboxing.rs index a0a6a17377..d36813c4bb 100644 --- a/crates/agent/src/sandboxing.rs +++ b/crates/agent/src/sandboxing.rs @@ -15,11 +15,347 @@ //! Naming note: this module is about agent terminal sandboxing specifically. //! Other agent operations (e.g. file edits) are gated separately. +use agent_settings::SandboxPermissions; use feature_flags::{FeatureFlagAppExt as _, SandboxingFeatureFlag}; use gpui::App; +use std::path::PathBuf; /// Whether agent-run terminal commands should be wrapped in an OS-level /// sandbox for this process. See module docs for the policy. pub(crate) fn sandboxing_enabled(cx: &App) -> bool { cfg!(target_os = "macos") && cx.has_flag::() } + +/// A request for elevated sandbox permissions for a single terminal command. +/// +/// Built from the model-controlled `terminal` tool input after the user has +/// authorized the baseline command. All paths here have already been resolved +/// to absolute, canonicalized paths by the caller — never raw, model-provided +/// strings, and never the model-controlled working directory. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub(crate) struct SandboxRequest { + /// Allow outbound network access for this command. + pub network: bool, + /// Allow unrestricted filesystem writes (the broad escape hatch). + pub allow_fs_write_all: bool, + /// Run the command fully outside the sandbox. + pub unsandboxed: bool, + /// Concrete paths the command needs to write to. Each grants its whole + /// subtree. These are never globs — write access is always a concrete path subtree + pub write_paths: Vec, +} + +impl SandboxRequest { + /// Whether this request asks for anything beyond the default sandbox + /// scope, and therefore needs user approval. + pub fn needs_escalation(&self) -> bool { + self.network || self.allow_fs_write_all || self.unsandboxed || !self.write_paths.is_empty() + } +} + +/// In-memory record of the sandbox permissions the user approved "for the +/// rest of the thread". +/// +/// Lives on the `Thread` and is shared (via `Rc>`) with each tool +/// call's event stream so a later command requesting an already-granted +/// permission can skip the approval prompt. Persistent "allow always" grants +/// are stored separately in [`SandboxPermissions`]. +#[derive(Default)] +pub(crate) struct ThreadSandboxGrants { + network: bool, + allow_fs_write_all: bool, + unsandboxed: bool, + /// Canonicalized paths granted write access for the thread. Each covers its + /// whole subtree; redundant children are pruned on insert. + write_paths: Vec, +} + +impl ThreadSandboxGrants { + /// Whether the union of thread grants and persistent "allow always" grants + /// covers everything `request` asks for, so the command can run without + /// prompting again. + /// + /// Write coverage is pure subtree containment: every requested path must + /// sit under some granted path. This is fully deterministic and never + /// widens scope, because grants are concrete paths rather than globs. + pub fn covers_with_persistent( + &self, + request: &SandboxRequest, + persistent: &SandboxPermissions, + ) -> bool { + if request.unsandboxed { + return self.unsandboxed || persistent.allow_unsandboxed; + } + if request.network && !(self.network || persistent.allow_network) { + return false; + } + if request.allow_fs_write_all && !(self.allow_fs_write_all || persistent.allow_fs_write_all) + { + return false; + } + // A full-access write grant covers any concrete write request. + if self.allow_fs_write_all || persistent.allow_fs_write_all { + return true; + } + request.write_paths.iter().all(|requested| { + self.write_paths + .iter() + .chain(persistent.write_paths.iter()) + .any(|granted| requested.starts_with(granted)) + }) + } + + /// Record everything in `request` as granted for the rest of the thread, + /// pruning paths that become redundant. + pub fn record(&mut self, request: &SandboxRequest) { + self.network |= request.network; + 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); + } + } + + /// Compute the effective sandbox permissions to enforce for a command: the + /// union of persistent "allow always" grants, thread grants, and this + /// specific command's request. + /// + /// This is what makes standing grants "stick": every sandboxed command + /// applies the accumulated grants, so the model can write to a previously + /// approved path without re-requesting it. Passing the current `request` in + /// also covers "allow once" grants, which are enforced for this command + /// without being recorded for the thread. + pub fn effective_with_persistent( + &self, + request: &SandboxRequest, + 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); + } + SandboxRequest { + network: persistent.allow_network || self.network || request.network, + allow_fs_write_all: persistent.allow_fs_write_all + || self.allow_fs_write_all + || request.allow_fs_write_all, + unsandboxed: request.unsandboxed, + write_paths, + } + } +} + +/// 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, 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::*; + + fn request(network: bool, all: bool, paths: &[&str]) -> SandboxRequest { + SandboxRequest { + network, + allow_fs_write_all: all, + unsandboxed: false, + write_paths: paths.iter().map(PathBuf::from).collect(), + } + } + + fn unsandboxed_request() -> SandboxRequest { + SandboxRequest { + network: false, + allow_fs_write_all: false, + unsandboxed: true, + write_paths: Vec::new(), + } + } + + fn covers(grants: &ThreadSandboxGrants, request: &SandboxRequest) -> bool { + grants.covers_with_persistent(request, &SandboxPermissions::default()) + } + + fn effective(grants: &ThreadSandboxGrants, request: &SandboxRequest) -> SandboxRequest { + grants.effective_with_persistent(request, &SandboxPermissions::default()) + } + + #[test] + fn empty_grants_cover_nothing() { + let grants = ThreadSandboxGrants::default(); + assert!(!covers(&grants, &request(true, false, &[]))); + assert!(!covers(&grants, &request(false, true, &[]))); + assert!(!covers(&grants, &unsandboxed_request())); + assert!(!covers(&grants, &request(false, false, &["/tmp/build"]))); + } + + #[test] + fn subtree_containment_covers_children() { + let mut grants = ThreadSandboxGrants::default(); + grants.record(&request(false, false, &["/tmp/build"])); + + // Exact match and any descendant are covered. + assert!(covers(&grants, &request(false, false, &["/tmp/build"]))); + assert!(covers( + &grants, + &request(false, false, &["/tmp/build/cache"]) + )); + // A sibling / parent is not. + assert!(!covers(&grants, &request(false, false, &["/tmp/other"]))); + assert!(!covers(&grants, &request(false, false, &["/tmp"]))); + } + + #[test] + fn record_prunes_redundant_children() { + let mut grants = ThreadSandboxGrants::default(); + grants.record(&request(false, false, &["/tmp/build/cache"])); + grants.record(&request(false, false, &["/tmp/build"])); + assert_eq!(grants.write_paths, vec![PathBuf::from("/tmp/build")]); + } + + #[test] + fn record_keeps_existing_broader_grant() { + let mut grants = ThreadSandboxGrants::default(); + grants.record(&request(false, false, &["/tmp/build"])); + grants.record(&request(false, false, &["/tmp/build/cache"])); + assert_eq!(grants.write_paths, vec![PathBuf::from("/tmp/build")]); + } + + #[test] + fn all_access_covers_any_concrete_write() { + let mut grants = ThreadSandboxGrants::default(); + grants.record(&request(false, true, &[])); + assert!(covers( + &grants, + &request(false, false, &["/anywhere/at/all"]) + )); + // But not network, which wasn't granted. + assert!(!covers(&grants, &request(true, false, &[]))); + } + + #[test] + fn network_grant_tracked_independently() { + let mut grants = ThreadSandboxGrants::default(); + grants.record(&request(true, false, &[])); + assert!(covers(&grants, &request(true, false, &[]))); + assert!(!covers(&grants, &request(true, false, &["/tmp/build"]))); + } + + #[test] + fn unsandboxed_grant_tracked_independently() { + let mut grants = ThreadSandboxGrants::default(); + grants.record(&unsandboxed_request()); + assert!(covers(&grants, &unsandboxed_request())); + assert!(!covers(&grants, &request(true, false, &[]))); + assert!(!covers(&grants, &request(false, true, &[]))); + } + + #[test] + fn persistent_grants_combine_with_thread_grants() { + let mut grants = ThreadSandboxGrants::default(); + grants.record(&request(true, false, &[])); + let persistent = SandboxPermissions { + allow_network: false, + allow_fs_write_all: false, + allow_unsandboxed: false, + write_paths: vec![PathBuf::from("/tmp/build")], + }; + + assert!( + grants + .covers_with_persistent(&request(true, false, &["/tmp/build/cache"]), &persistent) + ); + assert!( + !grants.covers_with_persistent(&request(true, false, &["/tmp/other"]), &persistent) + ); + } + + #[test] + fn persistent_all_access_covers_concrete_writes() { + let grants = ThreadSandboxGrants::default(); + let persistent = SandboxPermissions { + allow_network: false, + allow_fs_write_all: true, + allow_unsandboxed: false, + write_paths: Vec::new(), + }; + + assert!(grants.covers_with_persistent(&request(false, false, &["/anywhere"]), &persistent)); + assert!(grants.covers_with_persistent(&request(false, true, &[]), &persistent)); + assert!(!grants.covers_with_persistent(&request(true, false, &[]), &persistent)); + } + + #[test] + fn persistent_unsandboxed_covers_unsandboxed_requests_only() { + let grants = ThreadSandboxGrants::default(); + let persistent = SandboxPermissions { + allow_network: false, + allow_fs_write_all: false, + allow_unsandboxed: true, + write_paths: Vec::new(), + }; + + assert!(grants.covers_with_persistent(&unsandboxed_request(), &persistent)); + assert!(!grants.covers_with_persistent(&request(true, false, &[]), &persistent)); + assert!(!grants.covers_with_persistent(&request(false, true, &[]), &persistent)); + } + + #[test] + fn effective_applies_thread_grants_to_empty_request() { + // The core fix: a command that requests nothing still gets the + // thread's granted write paths in its enforced policy. + let mut grants = ThreadSandboxGrants::default(); + grants.record(&request(false, false, &["/tmp/build"])); + + let effective = effective(&grants, &request(false, false, &[])); + assert_eq!(effective.write_paths, vec![PathBuf::from("/tmp/build")]); + } + + #[test] + fn effective_unions_grants_with_once_request() { + // An "allow once" path (passed via `request`, never recorded) is + // enforced for this command alongside the standing grants. + let mut grants = ThreadSandboxGrants::default(); + grants.record(&request(true, false, &["/tmp/build"])); + + let effective = effective(&grants, &request(false, false, &["/tmp/once"])); + assert!(effective.network); + assert_eq!( + effective.write_paths, + vec![PathBuf::from("/tmp/build"), PathBuf::from("/tmp/once")] + ); + } + + #[test] + fn effective_applies_persistent_grants_to_empty_request() { + let grants = ThreadSandboxGrants::default(); + let persistent = SandboxPermissions { + allow_network: true, + allow_fs_write_all: false, + allow_unsandboxed: false, + write_paths: vec![PathBuf::from("/tmp/always")], + }; + + let effective = grants.effective_with_persistent(&request(false, false, &[]), &persistent); + assert!(effective.network); + assert_eq!(effective.write_paths, vec![PathBuf::from("/tmp/always")]); + } + + #[test] + fn effective_dedupes_request_already_covered_by_grant() { + let mut grants = ThreadSandboxGrants::default(); + grants.record(&request(false, false, &["/tmp/build"])); + + let effective = effective(&grants, &request(false, false, &["/tmp/build/cache"])); + assert_eq!(effective.write_paths, vec![PathBuf::from("/tmp/build")]); + } +} diff --git a/crates/agent/src/templates.rs b/crates/agent/src/templates.rs index 0d3e617d01..a946c404dc 100644 --- a/crates/agent/src/templates.rs +++ b/crates/agent/src/templates.rs @@ -44,7 +44,7 @@ pub struct SystemPromptTemplate<'a> { /// platform equivalent), if present and non-empty. pub user_agents_md: Option, /// Whether agent-run terminal commands are wrapped in an OS-level - /// sandbox for this conversation. When `true`, the rendered prompt + /// sandbox for this thread. When `true`, the rendered prompt /// describes the sandbox's read/write/network rules and the /// per-command flags the model can request to relax them. When /// `false`, the prompt omits the sandbox section entirely. @@ -193,9 +193,10 @@ mod tests { assert!(rendered.contains("`/tmp/alpha`")); assert!(rendered.contains("`/tmp/beta`")); assert!(rendered.contains("allow_network: true")); - assert!(rendered.contains("allow_fs_write: true")); + assert!(rendered.contains("fs_write_paths")); + assert!(rendered.contains("allow_fs_write_all: true")); assert!(rendered.contains("unsandboxed: true")); - assert!(rendered.contains("remain in effect for the entire duration")); + assert!(rendered.contains("for the rest of the thread")); } #[test] diff --git a/crates/agent/src/templates/system_prompt.hbs b/crates/agent/src/templates/system_prompt.hbs index 7326937ab0..38d96632f2 100644 --- a/crates/agent/src/templates/system_prompt.hbs +++ b/crates/agent/src/templates/system_prompt.hbs @@ -193,16 +193,23 @@ The current project contains the following root directories: The `terminal` tool runs commands inside a sandbox with these permissions: - Reads: any path on the filesystem is readable. -- Writes: a per-thread temporary directory exposed via `$TMPDIR`, `$TMP`, and `$TEMP` is writable and persists across `terminal` calls in this conversation{{#if worktrees}}, along with these project directories: +- Writes: a per-thread temporary directory exposed via `$TMPDIR`, `$TMP`, and `$TEMP` is writable and persists across `terminal` calls in this thread{{#if worktrees}}, along with these project directories: {{#each worktrees}} - `{{abs_path}}` {{/each}} Writes anywhere else on the filesystem are blocked.{{else}}. No project directories are currently writable.{{/if}} - Network: outbound network access is blocked. -You can request elevated permissions on individual `terminal` calls by setting `allow_network: true`, `allow_fs_write: true`, or `unsandboxed: true`. The user will be prompted to approve before the command runs. +You can request elevated permissions on individual `terminal` calls: -These sandbox settings are guaranteed to remain in effect for the entire duration of this conversation. If they ever change, you will be told. +- `allow_network: true` — allow outbound network access. +- `fs_write_paths: ["/abs/or/worktree-relative/path", ...]` — allow writes to specific paths (each directory grants its whole subtree). Prefer this whenever you know which paths the command needs to write. +- `allow_fs_write_all: true` — allow unrestricted filesystem writes. Only use this when the specific paths can't be enumerated up front. +- `unsandboxed: true` — run the command with no sandbox at all. Use only when none of the above suffice. + +The user will be prompted to approve before the command runs, and can grant a sandbox request for that command, for the rest of the thread, or always. Once a write path is granted for the thread or always, later commands in this thread writing under that path won't prompt again. + +These sandbox settings are guaranteed to remain in effect for the entire duration of this thread. If they ever change, you will be told. {{/if}} {{#if model_name}} diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index d2111b56e4..1134e1f50d 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -3,9 +3,9 @@ use crate::{ CreateThreadTool, DbLanguageModel, DbThread, DeletePathTool, DiagnosticsTool, EditFileTool, FetchTool, FindPathTool, FindReferencesTool, GetCodeActionsTool, GoToDefinitionTool, GrepTool, ListAgentsAndModelsTool, ListDirectoryTool, MovePathTool, ProjectSnapshot, ReadFileTool, - RenameTool, SpawnAgentTool, SystemPromptTemplate, Template, Templates, TerminalTool, - ToolPermissionDecision, UpdatePlanTool, UpdateTitleTool, WebSearchTool, WriteFileTool, - decide_permission_from_settings, + RenameTool, SandboxedTerminalTool, SpawnAgentTool, SystemPromptTemplate, Template, Templates, + TerminalTool, ToolPermissionDecision, UpdatePlanTool, UpdateTitleTool, WebSearchTool, + WriteFileTool, decide_permission_from_settings, }; use acp_thread::{MentionUri, UserMessageId}; use action_log::ActionLog; @@ -15,6 +15,7 @@ use feature_flags::{ UpdatePlanToolFeatureFlag, UpdateTitleToolFeatureFlag, }; +use crate::sandboxing::{SandboxRequest, ThreadSandboxGrants, sandboxing_enabled}; use agent_client_protocol::schema as acp; use agent_settings::{ AgentProfileId, AgentSettings, SUMMARIZE_THREAD_DETAILED_PROMPT, SUMMARIZE_THREAD_PROMPT, @@ -52,6 +53,7 @@ use serde::{Deserialize, Serialize}; use settings::{ LanguageModelSelection, Settings, SettingsStore, ToolPermissionMode, update_settings_file, }; +use std::cell::RefCell; use std::fmt::Write; use std::{ collections::BTreeMap, @@ -1123,6 +1125,11 @@ pub struct Thread { running_subagents: Vec>, inherits_parent_model_settings: bool, sandboxed_terminal_temp_dir: Option, + /// Sandbox permissions the user approved "for the rest of the thread". + /// Shared with each tool call's event stream so repeated requests for + /// already-granted permissions skip the approval prompt. + /// Never persisted — lives and dies with this thread. + sandbox_grants: Rc>, } impl Thread { @@ -1250,6 +1257,7 @@ impl Thread { running_subagents: Vec::new(), inherits_parent_model_settings: true, sandboxed_terminal_temp_dir: None, + sandbox_grants: Rc::new(RefCell::new(ThreadSandboxGrants::default())), } } @@ -1444,6 +1452,7 @@ impl Thread { stream.clone(), Some(self.project.read(cx).fs().clone()), cancellation_rx, + self.sandbox_grants.clone(), ); tool.replay(tool_use.input.clone(), output, tool_event_stream, cx) .log_err(); @@ -1594,6 +1603,7 @@ impl Thread { running_subagents: Vec::new(), inherits_parent_model_settings: true, sandboxed_terminal_temp_dir: db_thread.sandboxed_terminal_temp_dir, + sandbox_grants: Rc::new(RefCell::new(ThreadSandboxGrants::default())), } } @@ -1843,7 +1853,13 @@ impl Thread { self.action_log.clone(), update_agent_location, )); + // Register terminal tool variants; `enabled_tools` exposes the one + // matching the current sandbox state to the model as `terminal`. self.add_tool(TerminalTool::new(self.project.clone(), environment.clone())); + self.add_tool(SandboxedTerminalTool::new( + self.project.clone(), + environment.clone(), + )); self.add_tool(WebSearchTool); self.add_tool(DiagnosticsTool::new(self.project.clone())); @@ -2705,6 +2721,7 @@ impl Thread { event_stream.clone(), Some(fs), cancellation_rx, + self.sandbox_grants.clone(), ); tool_event_stream.update_fields( acp::ToolCallUpdateFields::new().status(acp::ToolCallStatus::InProgress), @@ -3178,14 +3195,35 @@ impl Thread { } } + // Terminal variants are configured by users under the canonical + // `terminal` name. Expose the one matching the current sandbox state + // to the model under that name. + let use_sandboxed_terminal = sandboxing_enabled(cx); + let mut tools = self .tools .iter() .filter_map(|(tool_name, tool)| { + let terminal_variant = matches!( + tool_name.as_ref(), + TerminalTool::NAME | SandboxedTerminalTool::NAME + ); + let profile_tool_name = if terminal_variant { + TerminalTool::NAME + } else { + tool_name.as_ref() + }; + if tool.supports_provider(&model.provider_id()) - && profile.is_tool_enabled(tool_name) + && profile.is_tool_enabled(profile_tool_name) { - Some((truncate(tool_name), tool.clone())) + match (tool_name.as_ref(), use_sandboxed_terminal) { + (TerminalTool::NAME, false) | (SandboxedTerminalTool::NAME, true) => { + Some((SharedString::from(TerminalTool::NAME), tool.clone())) + } + (TerminalTool::NAME | SandboxedTerminalTool::NAME, _) => None, + _ => Some((truncate(tool_name), tool.clone())), + } } else { None } @@ -4101,6 +4139,8 @@ pub struct ToolCallEventStream { stream: ThreadEventStream, fs: Option>, cancellation_rx: watch::Receiver, + /// Shared, thread-scoped sandbox grants (see [`Thread::sandbox_grants`]). + sandbox_grants: Rc>, } impl ToolCallEventStream { @@ -4120,6 +4160,7 @@ impl ToolCallEventStream { ThreadEventStream(events_tx), None, cancellation_rx, + Rc::new(RefCell::new(ThreadSandboxGrants::default())), ); ( @@ -4140,12 +4181,14 @@ impl ToolCallEventStream { stream: ThreadEventStream, fs: Option>, cancellation_rx: watch::Receiver, + sandbox_grants: Rc>, ) -> Self { Self { tool_use_id, stream, fs, cancellation_rx, + sandbox_grants, } } @@ -4328,6 +4371,216 @@ impl ToolCallEventStream { self.run_authorization_loop(title, options, Some(context), None, cx) } + /// Gate a sandbox *escalation* (network access, per-path writes, or full + /// filesystem write access) on user approval. + /// + /// Offers the user three grant lifetimes — "once", "for the rest of this + /// thread", and "always". Thread grants live in the shared, in-memory + /// [`ThreadSandboxGrants`]. Always grants are persisted in agent settings + /// and are also observed while a prompt is pending, matching the + /// settings-driven authorization flow for regular tools. + pub(crate) fn authorize_sandbox( + &self, + title: impl Into, + request: SandboxRequest, + cx: &mut App, + ) -> Task> { + if Self::sandbox_request_covered_by_grants(&request, &self.sandbox_grants, cx) { + return Task::ready(Ok(())); + } + + let title = title.into(); + let sandbox_authorization_details = acp_thread::SandboxAuthorizationDetails { + network: request.network, + allow_fs_write_all: request.allow_fs_write_all, + unsandboxed: request.unsandboxed, + write_paths: request.write_paths.clone(), + }; + let options = acp_thread::PermissionOptions::Flat(vec![ + acp::PermissionOption::new( + acp::PermissionOptionId::new("allow"), + "Allow once", + acp::PermissionOptionKind::AllowOnce, + ), + acp::PermissionOption::new( + acp::PermissionOptionId::new("allow_thread"), + "Allow for this thread", + acp::PermissionOptionKind::AllowAlways, + ), + acp::PermissionOption::new( + acp::PermissionOptionId::new("allow_always"), + "Allow always", + acp::PermissionOptionKind::AllowAlways, + ), + acp::PermissionOption::new( + acp::PermissionOptionId::new("deny"), + "Deny", + acp::PermissionOptionKind::RejectOnce, + ), + ]); + + let fs = self.fs.clone(); + let stream = self.stream.clone(); + let tool_use_id = self.tool_use_id.clone(); + let sandbox_grants = self.sandbox_grants.clone(); + cx.spawn(async move |cx| { + let (response_tx, mut response_rx) = oneshot::channel(); + if let Err(error) = stream + .0 + .unbounded_send(Ok(ThreadEvent::ToolCallAuthorization( + ToolCallAuthorization { + tool_call: acp::ToolCallUpdate::new( + tool_use_id.to_string(), + acp::ToolCallUpdateFields::new().title(title), + ) + .meta(acp_thread::meta_with_sandbox_authorization( + sandbox_authorization_details, + )), + options, + response: response_tx, + context: None, + kind: acp_thread::AuthorizationKind::PermissionGrant, + }, + ))) + { + log::error!("Failed to send sandbox authorization: {error}"); + return Err(anyhow!("Failed to send sandbox authorization: {error}")); + } + + let (mut settings_tx, mut settings_rx) = watch::channel(()); + let _settings_subscription = cx.update(|cx| { + cx.observe_global::(move |_cx| { + settings_tx.send(()).ok(); + }) + }); + + loop { + let settings_changed = async { + if settings_rx.changed().await.is_err() { + std::future::pending::<()>().await; + } + }; + futures::select_biased! { + outcome = (&mut response_rx).fuse() => { + let outcome = outcome + .map_err(|_| anyhow!("authorization channel closed"))?; + return Self::handle_sandbox_permission_outcome( + &outcome, + &request, + sandbox_grants.clone(), + fs.clone(), + cx, + ); + } + _ = settings_changed.fuse() => { + if cx.update(|cx| Self::sandbox_request_covered_by_grants( + &request, + &sandbox_grants, + cx, + )) { + drop(response_rx); + stream.update_tool_call_fields( + &tool_use_id, + acp::ToolCallUpdateFields::new() + .status(acp::ToolCallStatus::InProgress), + None, + ); + return Ok(()); + } + } + } + } + }) + } + + fn sandbox_request_covered_by_grants( + request: &SandboxRequest, + sandbox_grants: &Rc>, + cx: &App, + ) -> bool { + let settings = AgentSettings::get_global(cx); + sandbox_grants + .borrow() + .covers_with_persistent(request, &settings.sandbox_permissions) + } + + fn handle_sandbox_permission_outcome( + outcome: &acp_thread::SelectedPermissionOutcome, + request: &SandboxRequest, + sandbox_grants: Rc>, + fs: Option>, + cx: &AsyncApp, + ) -> Result<()> { + debug_assert!( + outcome.params.is_none(), + "unexpected params for sandbox permission" + ); + + match outcome.option_id.0.as_ref() { + "allow" => Ok(()), + "allow_thread" => { + sandbox_grants.borrow_mut().record(request); + Ok(()) + } + "allow_always" => { + 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 => { + debug_assert!(false, "unexpected sandbox permission option_id: {other}"); + Err(anyhow!("Permission to run tool denied by user")) + } + } + } + + fn persist_sandbox_always_permission( + request: &SandboxRequest, + fs: Option>, + cx: &AsyncApp, + ) { + let Some(fs) = fs else { + return; + }; + + let request = request.clone(); + cx.update(|cx| { + update_settings_file(fs, cx, move |settings, _| { + let agent = settings.agent.get_or_insert_default(); + if request.network { + agent.allow_sandbox_network(); + } + if request.allow_fs_write_all { + agent.allow_sandbox_fs_write_all(); + } + if request.unsandboxed { + agent.allow_sandbox_unsandboxed(); + } + for path in request.write_paths { + agent.add_sandbox_write_path(path); + } + }); + }); + } + + /// The sandbox permissions to actually enforce for a command: the union + /// of this command's `request`, everything granted "for the rest of the + /// conversation", and persistent "allow always" sandbox grants. + /// + /// Callers must apply this to the enforced sandbox policy (rather than + /// the raw `request`) so standing grants keep working for later commands + /// that write to a previously approved path without re-requesting it. + pub(crate) fn effective_sandbox_request( + &self, + request: &SandboxRequest, + persistent: &agent_settings::SandboxPermissions, + ) -> SandboxRequest { + self.sandbox_grants + .borrow() + .effective_with_persistent(request, persistent) + } + /// Prompts the user to choose between an explicit set of actions and /// returns the chosen `option_id`. /// @@ -5085,6 +5338,91 @@ mod tests { } } + #[gpui::test] + async fn test_authorize_sandbox_allow_always_records_current_grant(cx: &mut TestAppContext) { + crate::tests::init_test(cx); + + let (event_stream, mut receiver) = ToolCallEventStream::test(); + let request = SandboxRequest { + network: false, + allow_fs_write_all: false, + unsandboxed: false, + write_paths: vec![ + PathBuf::from("/tmp/build"), + PathBuf::from("/tmp/cache"), + PathBuf::from("/tmp/logs"), + PathBuf::from("/tmp/secret"), + ], + }; + + let authorize = cx.update(|cx| { + event_stream.authorize_sandbox("Allow write access?", request.clone(), cx) + }); + let authorization = receiver.expect_authorization().await; + let details = + acp_thread::sandbox_authorization_details_from_meta(&authorization.tool_call.meta) + .expect("sandbox authorization should include request details"); + assert_eq!(details.network, request.network); + assert_eq!(details.allow_fs_write_all, request.allow_fs_write_all); + assert_eq!(details.unsandboxed, request.unsandboxed); + assert_eq!(details.write_paths, request.write_paths); + assert!(authorization.tool_call.fields.content.is_none()); + + let acp_thread::PermissionOptions::Flat(options) = &authorization.options else { + panic!("expected flat sandbox permission options"); + }; + let options = options + .iter() + .map(|option| { + ( + option.option_id.0.as_ref(), + option.name.as_ref(), + option.kind, + ) + }) + .collect::>(); + assert_eq!( + options, + vec![ + ("allow", "Allow once", acp::PermissionOptionKind::AllowOnce), + ( + "allow_thread", + "Allow for this thread", + acp::PermissionOptionKind::AllowAlways, + ), + ( + "allow_always", + "Allow always", + acp::PermissionOptionKind::AllowAlways, + ), + ("deny", "Deny", acp::PermissionOptionKind::RejectOnce), + ] + ); + + let send_result = authorization + .response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow_always"), + acp::PermissionOptionKind::AllowAlways, + )); + assert!(send_result.is_ok()); + authorize.await.unwrap(); + + let effective = event_stream.effective_sandbox_request( + &SandboxRequest::default(), + &agent_settings::SandboxPermissions::default(), + ); + assert_eq!( + effective.write_paths, + vec![ + PathBuf::from("/tmp/build"), + PathBuf::from("/tmp/cache"), + PathBuf::from("/tmp/logs"), + PathBuf::from("/tmp/secret"), + ] + ); + } + #[gpui::test] async fn test_replay_tool_call_replays_image_content(cx: &mut TestAppContext) { let (thread, _event_stream) = setup_thread_for_test(cx).await; diff --git a/crates/agent/src/tool_permissions.rs b/crates/agent/src/tool_permissions.rs index 64b88cd573..20dc68f3bd 100644 --- a/crates/agent/src/tool_permissions.rs +++ b/crates/agent/src/tool_permissions.rs @@ -597,6 +597,7 @@ mod tests { use_modifier_to_send: true, message_editor_min_lines: 1, tool_permissions, + sandbox_permissions: Default::default(), show_turn_stats: false, show_merge_conflict_indicator: true, sidebar_side: Default::default(), diff --git a/crates/agent/src/tools/terminal_tool.rs b/crates/agent/src/tools/terminal_tool.rs index 3b591d4449..2644be6b0e 100644 --- a/crates/agent/src/tools/terminal_tool.rs +++ b/crates/agent/src/tools/terminal_tool.rs @@ -5,7 +5,6 @@ use gpui::{App, AsyncApp, Entity, SharedString, Task}; use project::Project; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -#[cfg(test)] use settings::Settings; use std::{ path::{Path, PathBuf}, @@ -44,7 +43,7 @@ const COMMAND_OUTPUT_LIMIT: u64 = 16 * 1024; /// - For other commands that may open a pager or editor, set `PAGER=cat` and/or `EDITOR=true` similarly. #[derive(Clone, Debug, Default, Serialize, Deserialize, JsonSchema)] pub struct TerminalToolInput { - /// The one-liner command to execute. Do not include shell substitutions or interpolations such as `$VAR`, `${VAR}`, `$(...)`, backticks, `$((...))`, `<(...)`, or `>(...)`; resolve those values first or ask the user. + /// The one-liner command to execute. Do not include shell substitutions or interpolations such as `$VAR`, `${VAR}`, `$(...)`, backticks, `$((...))`, `<(...)`, or `>(...)`; resolve those values first or ask the user for the literal value to use. /// /// REMINDER: read-only git commands (`git log`, `git diff`, `git show`, `git blame`) MUST include `--no-pager` (e.g. `git --no-pager log`). Git commands that may open an editor (`git rebase`, `git commit`, `git merge`, `git tag`) MUST be prefixed with `GIT_EDITOR=true ` (e.g. `GIT_EDITOR=true git rebase origin/main`). Otherwise the terminal will hang. pub command: String, @@ -58,37 +57,137 @@ pub struct TerminalToolInput { /// Return only the last N lines of terminal output to the model after the command finishes. Do not pipe output to `tail`; use this parameter instead so the user can still see live output. Avoid requesting too many lines, or the response may waste tokens or exceed the context window. #[serde(default)] pub tail_lines: Option, - /// Request network access for this command. +} + +/// Executes a shell one-liner and returns the combined output. +/// +/// This tool spawns a process using the user's shell, reads from stdout and stderr (preserving the order of writes), and returns a string with the combined output result. +/// +/// The output results will be shown to the user already, only list it again if necessary, avoid being redundant. +/// +/// Make sure you use the `cd` parameter to navigate to one of the root directories of the project. NEVER do it as part of the `command` itself, otherwise it will error. +/// +/// Do not generate terminal commands that use shell substitutions or interpolations such as `$VAR`, `${VAR}`, `$(...)`, backticks, `$((...))`, `<(...)`, or `>(...)`. Resolve those values first or ask the user for the literal value to use. +/// +/// Do not pipe output to `head`, `tail`, or similar output-filtering commands just to reduce what you receive. Instead, use `head_lines` and/or `tail_lines`; this keeps the terminal output visible to the user in real time while limiting only the final output sent back to you. When both are specified, the first `head_lines` lines are returned, then a blank line, then the last `tail_lines` lines. Avoid requesting too many lines, or the response may waste tokens or exceed the context window. +/// +/// Do not use this tool for commands that run indefinitely, such as servers (like `npm run start`, `npm run dev`, `python -m http.server`, etc) or file watchers that don't terminate on their own. +/// +/// For potentially long-running commands, prefer specifying `timeout_ms` to bound runtime and prevent indefinite hangs. +/// +/// Remember that each invocation of this tool will spawn a new shell process, so you can't rely on any state from previous invocations. +/// +/// The terminal is an interactive pty, so any command that blocks waiting for input will hang the tool until it times out. To avoid this: +/// +/// - Always insert `--no-pager` immediately after `git` for any read-only git command, including `git log`, `git diff`, `git show`, `git blame`, and `git stash show`. Example: `git --no-pager log -n 5` (NOT `git log -n 5`). +/// - Always prepend `GIT_EDITOR=true ` to any git command that may invoke an editor, including `git rebase`, `git commit`, `git merge`, and `git tag`. Example: `GIT_EDITOR=true git rebase origin/main` (NOT `git rebase origin/main`). +/// - For other commands that may open a pager or editor, set `PAGER=cat` and/or `EDITOR=true` similarly. +#[derive(Clone, Debug, Default, Serialize, Deserialize, JsonSchema)] +pub struct SandboxedTerminalToolInput { + /// The one-liner command to execute. Do not include shell substitutions or interpolations such as `$VAR`, `${VAR}`, `$(...)`, backticks, `$((...))`, `<(...)`, or `>(...)`; resolve those values first or ask the user for the literal value to use. /// - /// Only meaningful when the system prompt's "Terminal sandbox" section - /// is present — ignored otherwise. By default sandboxed commands - /// cannot make outbound network connections; set this to `true` only - /// when the command needs network access. The user will be prompted - /// to approve before the command runs. + /// REMINDER: read-only git commands (`git log`, `git diff`, `git show`, `git blame`) MUST include `--no-pager` (e.g. `git --no-pager log`). Git commands that may open an editor (`git rebase`, `git commit`, `git merge`, `git tag`) MUST be prefixed with `GIT_EDITOR=true ` (e.g. `GIT_EDITOR=true git rebase origin/main`). Otherwise the terminal will hang. + pub command: String, + /// Working directory for the command. This must be one of the root directories of the project. + pub cd: String, + /// Optional maximum runtime (in milliseconds). If exceeded, the running terminal task is killed. + pub timeout_ms: Option, + /// Return only the first N lines of terminal output to the model after the command finishes. Do not pipe output to `head`; use this parameter instead so the user can still see live output. Avoid requesting too many lines, or the response may waste tokens or exceed the context window. + #[serde(default)] + pub head_lines: Option, + /// Return only the last N lines of terminal output to the model after the command finishes. Do not pipe output to `tail`; use this parameter instead so the user can still see live output. Avoid requesting too many lines, or the response may waste tokens or exceed the context window. + #[serde(default)] + pub tail_lines: Option, + /// Set to `true` only if the command needs outbound network access. + /// + /// Sandboxed commands cannot reach the network by default, so set this + /// when running commands that fetch or upload (installing dependencies, + /// cloning, pushing, downloading, etc.). Requesting it triggers a user + /// approval prompt, so only set it when you expect the command to need + /// network. #[serde(default)] pub allow_network: Option, - /// Request unrestricted filesystem-write access for this command. + /// Paths the command needs to write to outside the default-writable + /// locations. /// - /// Only meaningful when the system prompt's "Terminal sandbox" section - /// is present — ignored otherwise. By default sandboxed commands can - /// only write to the project worktree directories and a per-command - /// temporary directory; set this to `true` only when the command - /// needs to write elsewhere. The user will be prompted to approve - /// before the command runs. + /// Sandboxed commands can already write to the project worktree + /// directories and a per-command temporary directory, so only list paths + /// outside those. Provide absolute or worktree-relative paths; each + /// directory grants write access to its whole subtree. Prefer this over + /// `allow_fs_write_all` whenever you can enumerate the paths. Requesting + /// paths triggers a user approval prompt. #[serde(default)] - pub allow_fs_write: Option, - /// Request to run this command outside the sandbox entirely. + pub fs_write_paths: Vec, + /// Set to `true` only when the command needs to write outside the + /// default-writable locations but the specific paths cannot be + /// enumerated up front. /// - /// Only meaningful when the system prompt's "Terminal sandbox" section - /// is present — ignored otherwise. Prefer `allow_network: true` or - /// `allow_fs_write: true` when one of those is enough. Set this to - /// `true` ONLY when the command needs behavior that the sandbox can't - /// grant on a per-permission basis. The user will be prompted to - /// approve before the command runs without sandbox restrictions. + /// This is a broad escape hatch — prefer `fs_write_paths` whenever the + /// set of paths is known. Requesting it triggers a user approval prompt. + #[serde(default, alias = "allow_fs_write")] + pub allow_fs_write_all: Option, + /// Set to `true` only as a last resort, to run the command fully outside + /// the sandbox. + /// + /// First try the narrower options (`allow_network`, `fs_write_paths`, + /// `allow_fs_write_all`); use this only when the command needs behavior + /// the sandbox can't grant on a per-permission basis. Requesting it + /// triggers a user approval prompt. #[serde(default)] pub unsandboxed: Option, } +#[derive(Clone, Debug, Default)] +struct TerminalSandboxInput { + allow_network: Option, + fs_write_paths: Vec, + allow_fs_write_all: Option, + unsandboxed: Option, +} + +struct TerminalToolRequest { + command: String, + cd: String, + timeout_ms: Option, + selection: TerminalOutputSelection, + sandbox: Option, +} + +impl From for TerminalToolRequest { + fn from(input: TerminalToolInput) -> Self { + Self { + command: input.command, + cd: input.cd, + timeout_ms: input.timeout_ms, + selection: TerminalOutputSelection { + head_lines: input.head_lines, + tail_lines: input.tail_lines, + }, + sandbox: None, + } + } +} + +impl From for TerminalToolRequest { + fn from(input: SandboxedTerminalToolInput) -> Self { + Self { + command: input.command, + cd: input.cd, + timeout_ms: input.timeout_ms, + selection: TerminalOutputSelection { + head_lines: input.head_lines, + tail_lines: input.tail_lines, + }, + sandbox: Some(TerminalSandboxInput { + allow_network: input.allow_network, + fs_write_paths: input.fs_write_paths, + allow_fs_write_all: input.allow_fs_write_all, + unsandboxed: input.unsandboxed, + }), + } + } +} + pub struct TerminalTool { project: Entity, environment: Rc, @@ -103,6 +202,20 @@ impl TerminalTool { } } +pub struct SandboxedTerminalTool { + project: Entity, + environment: Rc, +} + +impl SandboxedTerminalTool { + pub fn new(project: Entity, environment: Rc) -> Self { + Self { + project, + environment, + } + } +} + impl AgentTool for TerminalTool { type Input = TerminalToolInput; type Output = String; @@ -132,7 +245,45 @@ impl AgentTool for TerminalTool { run_terminal_tool( self.project.clone(), self.environment.clone(), - input, + input.into(), + event_stream, + cx, + ) + .await + }) + } +} + +impl AgentTool for SandboxedTerminalTool { + type Input = SandboxedTerminalToolInput; + type Output = String; + + const NAME: &'static str = "sandboxed_terminal"; + + fn kind() -> acp::ToolKind { + acp::ToolKind::Execute + } + + fn initial_title( + &self, + input: Result, + _cx: &mut App, + ) -> SharedString { + terminal_initial_title(input.map(|input| input.command)) + } + + fn run( + self: Arc, + input: ToolInput, + event_stream: ToolCallEventStream, + cx: &mut App, + ) -> Task> { + cx.spawn(async move |cx| { + let input = input.recv().await.map_err(|e| e.to_string())?; + run_terminal_tool( + self.project.clone(), + self.environment.clone(), + input.into(), event_stream, cx, ) @@ -152,55 +303,73 @@ fn terminal_initial_title(input: Result) -> SharedStr async fn run_terminal_tool( project: Entity, environment: Rc, - input: TerminalToolInput, + input: TerminalToolRequest, event_stream: ToolCallEventStream, cx: &mut AsyncApp, ) -> Result { - let selection = TerminalOutputSelection { - head_lines: input.head_lines, - tail_lines: input.tail_lines, - }; + let selection = input.selection; + let sandbox_input = input.sandbox.clone().unwrap_or_default(); let (working_dir, authorize, sandboxing) = cx.update(|cx| { - let working_dir = working_dir(&input, &project, cx).map_err(|err| err.to_string())?; + let working_dir = working_dir(&input.cd, &project, cx).map_err(|err| err.to_string())?; let context = crate::ToolPermissionContext::new(TerminalTool::NAME, vec![input.command.clone()]); let authorize = event_stream.authorize(SharedString::new(input.command.clone()), context, cx); - let sandboxing = sandboxing_enabled(cx); + let sandboxing = input.sandbox.is_some() && sandboxing_enabled(cx); Result::<_, String>::Ok((working_dir, authorize, sandboxing)) })?; authorize.await.map_err(|e| e.to_string())?; - let want_network = sandboxing && input.allow_network == Some(true); - let want_fs_write = sandboxing && input.allow_fs_write == Some(true); - let want_unsandboxed = sandboxing && input.unsandboxed == Some(true); - let escalate = !want_unsandboxed && (want_network || want_fs_write); + let want_network = sandboxing && sandbox_input.allow_network == Some(true); + let want_fs_write_all = sandboxing && sandbox_input.allow_fs_write_all == Some(true); + let want_unsandboxed = sandboxing && sandbox_input.unsandboxed == Some(true); - if want_unsandboxed || escalate { - let title = sandbox_approval_title(want_network, want_fs_write, want_unsandboxed); - let approve = cx.update(|cx| { - let context = - crate::ToolPermissionContext::new(TerminalTool::NAME, vec![input.command.clone()]); - event_stream.authorize_always_prompt(title, context, cx) - }); + let write_paths: Vec = if sandboxing && !want_unsandboxed { + cx.update(|cx| { + resolve_write_paths( + &sandbox_input.fs_write_paths, + working_dir.as_deref(), + &project, + cx, + ) + }) + } else { + Vec::new() + }; + + let request = crate::sandboxing::SandboxRequest { + network: !want_unsandboxed && want_network, + allow_fs_write_all: !want_unsandboxed && want_fs_write_all, + unsandboxed: want_unsandboxed, + write_paths, + }; + + if request.needs_escalation() { + let title = sandbox_approval_title(&request); + let approve = cx.update(|cx| event_stream.authorize_sandbox(title, request.clone(), cx)); if let Err(error) = approve.await { - return Ok(if want_unsandboxed { - format!( + if want_unsandboxed { + return Ok(format!( "Command cancelled: user denied permission to run outside the sandbox ({error})." - ) - } else { - format!( - "Command cancelled: user denied the requested sandbox permissions ({error})." - ) - }); + )); + } + return Ok(format!( + "Command cancelled: user denied the requested sandbox permissions ({error})." + )); } } let extra_env = Vec::new(); let sandbox_wrap = if sandboxing && !want_unsandboxed { + let sandbox_permissions = cx.update(|cx| { + agent_settings::AgentSettings::get_global(cx) + .sandbox_permissions + .clone() + }); + let effective = event_stream.effective_sandbox_request(&request, &sandbox_permissions); let writable_paths: Vec = cx.update(|cx| { project .read(cx) @@ -210,8 +379,9 @@ async fn run_terminal_tool( }); Some(acp_thread::SandboxWrap { writable_paths, - allow_network: want_network, - allow_fs_write: want_fs_write, + extra_write_paths: effective.write_paths, + allow_network: effective.network, + allow_fs_write: effective.allow_fs_write_all, }) } else { None @@ -291,26 +461,81 @@ async fn run_terminal_tool( )) } -/// User-facing title for the sandbox-escalation approval prompt. +/// Resolve model-requested write paths into absolute paths. /// -/// `want_unsandboxed` wins over the per-permission flags because -/// `unsandboxed: true` bypasses the per-permission machinery entirely. -fn sandbox_approval_title( - want_network: bool, - want_fs_write: bool, - want_unsandboxed: bool, -) -> &'static str { - if want_unsandboxed { - "Allow this command to run outside the sandbox?" - } else { - match (want_network, want_fs_write) { - (true, true) => "Allow network access and arbitrary filesystem writes?", - (true, false) => "Allow network access?", - (false, true) => "Allow arbitrary filesystem writes?", - // Caller only invokes this when at least one flag is set, so - // this fallback is unreachable in practice. - (false, false) => "Allow this command to run?", - } +/// Relative paths are resolved against the command's working directory when +/// known, otherwise against the project's first worktree root. Paths that +/// can't be made absolute (relative paths with no base) are dropped. The +/// resulting paths are shown to the user for approval, so resolving against +/// model-controlled inputs is safe — nothing is granted without that prompt. +fn resolve_write_paths( + raw_paths: &[String], + working_dir: Option<&Path>, + project: &Entity, + cx: &App, +) -> Vec { + if raw_paths.is_empty() { + return Vec::new(); + } + let base = working_dir.map(Path::to_path_buf).or_else(|| { + project + .read(cx) + .worktrees(cx) + .next() + .map(|worktree| worktree.read(cx).abs_path().to_path_buf()) + }); + join_write_paths(raw_paths, base.as_deref()) +} + +/// Pure path-joining step of [`resolve_write_paths`], split out so it can be +/// unit-tested without a `Project`/`App`. +fn join_write_paths(raw_paths: &[String], base: Option<&Path>) -> Vec { + raw_paths + .iter() + .filter_map(|raw| { + let path = Path::new(raw); + if path.is_absolute() { + Some(path.to_path_buf()) + } else { + base.map(|base| base.join(path)) + } + }) + .collect() +} + +/// User-facing title for the sandbox-escalation approval prompt. Only called +/// when the request actually asks for something (see +/// [`crate::sandboxing::SandboxRequest::needs_escalation`]). +fn sandbox_approval_title(request: &crate::sandboxing::SandboxRequest) -> String { + if request.unsandboxed { + return "Allow this command to run outside the sandbox?".to_string(); + } + + let mut parts: Vec = Vec::new(); + if request.network { + parts.push("network access".to_string()); + } + if request.allow_fs_write_all { + parts.push("unrestricted filesystem writes".to_string()); + } else if !request.write_paths.is_empty() { + parts.push(format!( + "write access to {}", + write_path_summary(&request.write_paths) + )); + } + match parts.as_slice() { + [] => "Allow this command extra permissions?".to_string(), + [only] => format!("Allow {only}?"), + [first, second] => format!("Allow {first} and {second}?"), + _ => format!("Allow {}?", parts.join(", ")), + } +} + +fn write_path_summary(paths: &[PathBuf]) -> String { + match paths { + [] => "0 paths".to_string(), + [path] => path.display().to_string(), + paths => format!("{} paths", paths.len()), } } @@ -432,16 +657,10 @@ fn process_content( content } -fn working_dir( - input: &TerminalToolInput, - project: &Entity, - cx: &mut App, -) -> Result> { +fn working_dir(cd: &str, project: &Entity, cx: &mut App) -> Result> { let project = project.read(cx); - let cd = &input.cd; if cd == "." || cd.is_empty() { - // Accept "." or "" as meaning "the one worktree" if we only have one worktree. let mut worktrees = project.worktrees(cx); match worktrees.next() { @@ -458,7 +677,6 @@ fn working_dir( let input_path = Path::new(cd); if input_path.is_absolute() { - // Absolute paths are allowed, but only if they're in one of the project's worktrees. if project .worktrees(cx) .any(|worktree| input_path.starts_with(&worktree.read(cx).abs_path())) @@ -484,8 +702,8 @@ mod tests { .to_string(), cd: ".".to_string(), timeout_ms: None, - ..Default::default() - }; + ..Default::default() + }; let title = format_initial_title(Ok(input)); @@ -1359,7 +1577,6 @@ mod tests { timeout_ms: None, head_lines: Some(1), tail_lines: Some(1), - ..Default::default() }), event_stream, cx, @@ -2027,51 +2244,135 @@ mod tests { ); } + fn sandbox_request( + network: bool, + all: bool, + paths: &[&str], + ) -> crate::sandboxing::SandboxRequest { + crate::sandboxing::SandboxRequest { + network, + allow_fs_write_all: all, + unsandboxed: false, + write_paths: paths.iter().map(PathBuf::from).collect(), + } + } + #[test] - fn test_sandbox_approval_title_unsandboxed_wins() { - // `unsandboxed: true` skips the sandbox entirely, so the title should - // reflect that even when other flags are also set — they're moot. - assert_eq!( - sandbox_approval_title(true, true, true), - "Allow this command to run outside the sandbox?" + fn test_join_write_paths_resolves_relative_and_absolute() { + let base = PathBuf::from(if cfg!(windows) { + "C:\\project" + } else { + "/project" + }); + let abs = if cfg!(windows) { + "C:\\abs\\path" + } else { + "/abs/path" + }; + let joined = join_write_paths( + &[ + abs.to_string(), + "relative/dir".to_string(), + "file.txt".to_string(), + ], + Some(base.as_path()), ); assert_eq!( - sandbox_approval_title(false, false, true), + joined, + vec![ + PathBuf::from(abs), + base.join("relative/dir"), + base.join("file.txt"), + ] + ); + } + + #[test] + fn test_join_write_paths_drops_relative_without_base() { + // Absolute paths still pass through; relative ones are dropped when + // there's no base to resolve them against. + let abs = if cfg!(windows) { + "C:\\abs\\keep" + } else { + "/abs/keep" + }; + let joined = join_write_paths(&[abs.to_string(), "relative/drop".to_string()], None); + assert_eq!(joined, vec![PathBuf::from(abs)]); + } + + #[test] + fn test_sandbox_approval_title_unsandboxed() { + let mut request = sandbox_request(true, true, &["/tmp/build"]); + request.unsandboxed = true; + assert_eq!( + sandbox_approval_title(&request), "Allow this command to run outside the sandbox?" ); } #[test] - fn test_sandbox_approval_title_per_permission_flags() { + fn test_sandbox_approval_title_all_access_and_network() { assert_eq!( - sandbox_approval_title(true, true, false), - "Allow network access and arbitrary filesystem writes?" + sandbox_approval_title(&sandbox_request(true, true, &[])), + "Allow network access and unrestricted filesystem writes?" ); assert_eq!( - sandbox_approval_title(true, false, false), + sandbox_approval_title(&sandbox_request(true, false, &[])), "Allow network access?" ); assert_eq!( - sandbox_approval_title(false, true, false), - "Allow arbitrary filesystem writes?" + sandbox_approval_title(&sandbox_request(false, true, &[])), + "Allow unrestricted filesystem writes?" + ); + } + + #[test] + fn test_sandbox_approval_title_per_path_writes() { + assert_eq!( + sandbox_approval_title(&sandbox_request(false, false, &["/tmp/build"])), + "Allow write access to /tmp/build?" + ); + assert_eq!( + sandbox_approval_title(&sandbox_request(true, false, &["/tmp/build"])), + "Allow network access and write access to /tmp/build?" + ); + } + + #[test] + fn test_sandbox_approval_title_summarizes_multiple_paths_by_count() { + let title = + sandbox_approval_title(&sandbox_request(false, false, &["/a", "/b", "/c", "/d"])); + assert_eq!(title, "Allow write access to 4 paths?"); + } + + #[test] + fn test_all_access_takes_precedence_over_paths_in_title() { + // When all-access is requested, the specific paths are redundant and + // should not be listed. + assert_eq!( + sandbox_approval_title(&sandbox_request(false, true, &["/tmp/build"])), + "Allow unrestricted filesystem writes?" ); } #[test] fn test_input_schema_includes_sandbox_flags() { - // The model only sees these fields when the sandboxing prompt - // section is rendered, but they're always present in the schema so - // input validation doesn't reject them when sent. Guard against + // The sandboxed terminal tool advertises these fields so the model can + // request escalations when the sandbox is in effect. Guard against // accidentally renaming or removing them. - let schema = serde_json::to_string(&schemars::schema_for!(TerminalToolInput)) + let schema = serde_json::to_string(&schemars::schema_for!(SandboxedTerminalToolInput)) .expect("input schema should serialize"); assert!( schema.contains("allow_network"), "schema should advertise allow_network: {schema}" ); assert!( - schema.contains("allow_fs_write"), - "schema should advertise allow_fs_write: {schema}" + schema.contains("fs_write_paths"), + "schema should advertise fs_write_paths: {schema}" + ); + assert!( + schema.contains("allow_fs_write_all"), + "schema should advertise allow_fs_write_all: {schema}" ); assert!( schema.contains("unsandboxed"), @@ -2085,13 +2386,214 @@ mod tests { // calls. Make sure deserialization doesn't reject the minimal // payload and that the fields default to `None` (which the tool // interprets as "no escalation requested"). - let input: TerminalToolInput = serde_json::from_value(serde_json::json!({ + let input: SandboxedTerminalToolInput = serde_json::from_value(serde_json::json!({ "command": "echo hi", "cd": ".", })) .expect("minimal input should deserialize"); assert_eq!(input.allow_network, None); - assert_eq!(input.allow_fs_write, None); + assert!(input.fs_write_paths.is_empty()); + assert_eq!(input.allow_fs_write_all, None); assert_eq!(input.unsandboxed, None); } + + #[test] + fn test_legacy_allow_fs_write_aliases_to_allow_fs_write_all() { + let input: SandboxedTerminalToolInput = serde_json::from_value(serde_json::json!({ + "command": "echo hi", + "cd": ".", + "allow_fs_write": true, + })) + .expect("legacy allow_fs_write should deserialize"); + + assert_eq!(input.allow_fs_write_all, Some(true)); + } + + #[cfg(target_os = "macos")] + #[gpui::test] + async fn test_legacy_allow_fs_write_uses_sandbox_permission_options( + cx: &mut gpui::TestAppContext, + ) { + use feature_flags::FeatureFlagAppExt as _; + + crate::tests::init_test(cx); + cx.update(|cx| { + cx.update_flags(true, vec!["sandboxing".to_string()]); + let mut settings = agent_settings::AgentSettings::get_global(cx).clone(); + settings.tool_permissions.default = settings::ToolPermissionMode::Allow; + settings.tool_permissions.tools.remove(TerminalTool::NAME); + agent_settings::AgentSettings::override_global(settings, cx); + }); + + let fs = fs::FakeFs::new(cx.executor()); + fs.insert_tree("/root", serde_json::json!({})).await; + let project = project::Project::test(fs, ["/root".as_ref()], cx).await; + + let environment = std::rc::Rc::new(cx.update(|cx| { + crate::tests::FakeThreadEnvironment::default().with_terminal( + crate::tests::FakeTerminalHandle::new_with_immediate_exit(cx, 0), + ) + })); + #[allow(clippy::arc_with_non_send_sync)] + let tool = std::sync::Arc::new(SandboxedTerminalTool::new(project, environment.clone())); + let (event_stream, mut receiver) = crate::ToolCallEventStream::test(); + let input: SandboxedTerminalToolInput = serde_json::from_value(serde_json::json!({ + "command": "echo hi", + "cd": "root", + "allow_fs_write": true, + })) + .expect("legacy allow_fs_write should deserialize"); + + let task = cx.update(|cx| tool.run(crate::ToolInput::resolved(input), event_stream, cx)); + + let authorization = receiver.expect_authorization().await; + let details = + acp_thread::sandbox_authorization_details_from_meta(&authorization.tool_call.meta) + .expect("legacy allow_fs_write should request sandbox authorization details"); + assert!(!details.network); + assert!(details.allow_fs_write_all); + assert!(!details.unsandboxed); + assert!(details.write_paths.is_empty()); + + let acp_thread::PermissionOptions::Flat(options) = &authorization.options else { + panic!("expected flat sandbox permission options"); + }; + let options = options + .iter() + .map(|option| { + ( + option.option_id.0.as_ref(), + option.name.as_ref(), + option.kind, + ) + }) + .collect::>(); + assert_eq!( + options, + vec![ + ("allow", "Allow once", acp::PermissionOptionKind::AllowOnce), + ( + "allow_thread", + "Allow for this thread", + acp::PermissionOptionKind::AllowAlways, + ), + ( + "allow_always", + "Allow always", + acp::PermissionOptionKind::AllowAlways, + ), + ("deny", "Deny", acp::PermissionOptionKind::RejectOnce), + ] + ); + + authorization + .response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("deny"), + acp::PermissionOptionKind::RejectOnce, + )) + .expect("authorization response should send"); + + let result = task + .await + .expect("denied sandbox request returns model-readable output"); + assert!(result.contains("user denied the requested sandbox permissions")); + assert_eq!(environment.terminal_creation_count(), 0); + } + + #[cfg(target_os = "macos")] + #[gpui::test] + async fn test_unsandboxed_uses_sandbox_permission_options(cx: &mut gpui::TestAppContext) { + use feature_flags::FeatureFlagAppExt as _; + + crate::tests::init_test(cx); + cx.update(|cx| { + cx.update_flags(true, vec!["sandboxing".to_string()]); + let mut settings = agent_settings::AgentSettings::get_global(cx).clone(); + settings.tool_permissions.default = settings::ToolPermissionMode::Allow; + settings.tool_permissions.tools.remove(TerminalTool::NAME); + agent_settings::AgentSettings::override_global(settings, cx); + }); + + let fs = fs::FakeFs::new(cx.executor()); + fs.insert_tree("/root", serde_json::json!({})).await; + let project = project::Project::test(fs, ["/root".as_ref()], cx).await; + + let environment = std::rc::Rc::new(cx.update(|cx| { + crate::tests::FakeThreadEnvironment::default().with_terminal( + crate::tests::FakeTerminalHandle::new_with_immediate_exit(cx, 0), + ) + })); + #[allow(clippy::arc_with_non_send_sync)] + let tool = std::sync::Arc::new(SandboxedTerminalTool::new(project, environment.clone())); + let (event_stream, mut receiver) = crate::ToolCallEventStream::test(); + let input: SandboxedTerminalToolInput = serde_json::from_value(serde_json::json!({ + "command": "echo hi", + "cd": "root", + "allow_network": true, + "allow_fs_write_all": true, + "unsandboxed": true, + })) + .expect("unsandboxed input should deserialize"); + + let task = cx.update(|cx| tool.run(crate::ToolInput::resolved(input), event_stream, cx)); + + let authorization = receiver.expect_authorization().await; + assert_eq!( + authorization.tool_call.fields.title.as_deref(), + Some("Allow this command to run outside the sandbox?") + ); + let details = + acp_thread::sandbox_authorization_details_from_meta(&authorization.tool_call.meta) + .expect("unsandboxed should request sandbox authorization details"); + assert!(!details.network); + assert!(!details.allow_fs_write_all); + assert!(details.unsandboxed); + assert!(details.write_paths.is_empty()); + + let acp_thread::PermissionOptions::Flat(options) = &authorization.options else { + panic!("expected flat sandbox permission options"); + }; + let options = options + .iter() + .map(|option| { + ( + option.option_id.0.as_ref(), + option.name.as_ref(), + option.kind, + ) + }) + .collect::>(); + assert_eq!( + options, + vec![ + ("allow", "Allow once", acp::PermissionOptionKind::AllowOnce), + ( + "allow_thread", + "Allow for this thread", + acp::PermissionOptionKind::AllowAlways, + ), + ( + "allow_always", + "Allow always", + acp::PermissionOptionKind::AllowAlways, + ), + ("deny", "Deny", acp::PermissionOptionKind::RejectOnce), + ] + ); + + authorization + .response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("deny"), + acp::PermissionOptionKind::RejectOnce, + )) + .expect("authorization response should send"); + + let result = task + .await + .expect("denied sandbox request returns model-readable output"); + assert!(result.contains("user denied permission to run outside the sandbox")); + assert_eq!(environment.terminal_creation_count(), 0); + } } diff --git a/crates/agent_settings/src/agent_settings.rs b/crates/agent_settings/src/agent_settings.rs index c5ca601f1b..b25081080f 100644 --- a/crates/agent_settings/src/agent_settings.rs +++ b/crates/agent_settings/src/agent_settings.rs @@ -1,7 +1,7 @@ mod agent_profile; mod user_agents_md; -use std::path::{Component, Path}; +use std::path::{Component, Path, PathBuf}; use std::sync::{Arc, LazyLock}; use agent_client_protocol::schema as acp; @@ -171,6 +171,7 @@ pub struct AgentSettings { pub show_turn_stats: bool, pub show_merge_conflict_indicator: bool, pub tool_permissions: ToolPermissions, + pub sandbox_permissions: SandboxPermissions, } impl AgentSettings { @@ -337,6 +338,42 @@ impl Default for AgentProfileId { } } +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct SandboxPermissions { + pub allow_network: bool, + pub allow_fs_write_all: bool, + pub allow_unsandboxed: bool, + pub write_paths: Vec, +} + +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. @@ -676,10 +713,39 @@ impl Settings for AgentSettings { show_turn_stats: agent.show_turn_stats.unwrap(), show_merge_conflict_indicator: agent.show_merge_conflict_indicator.unwrap(), tool_permissions: compile_tool_permissions(agent.tool_permissions), + sandbox_permissions: compile_sandbox_permissions(agent.sandbox_permissions), } } } +fn compile_sandbox_permissions( + content: Option, +) -> SandboxPermissions { + let Some(content) = content else { + return SandboxPermissions::default(); + }; + + 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); + } + + SandboxPermissions { + allow_network: content.allow_network.unwrap_or(false), + allow_fs_write_all: content.allow_fs_write_all.unwrap_or(false), + allow_unsandboxed: content.allow_unsandboxed.unwrap_or(false), + write_paths, + } +} + +fn add_sandbox_write_path(write_paths: &mut Vec, 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) -> ToolPermissions { let Some(content) = content else { return ToolPermissions::default(); @@ -854,6 +920,54 @@ mod tests { assert_eq!(permissions.default, ToolPermissionMode::Confirm); } + #[test] + 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] + fn test_sandbox_permissions_parsing_and_pruning() { + let json = json!({ + "allow_network": true, + "allow_unsandboxed": true, + "write_paths": [ + "/tmp/build/cache", + "/tmp/build", + "/var/log" + ] + }); + + let content: settings::SandboxPermissionsContent = serde_json::from_value(json).unwrap(); + let permissions = compile_sandbox_permissions(Some(content)); + + assert!(permissions.allow_network); + assert!(!permissions.allow_fs_write_all); + assert!(permissions.allow_unsandboxed); + assert_eq!( + 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() { + let json = json!({ + "allow_fs_write_all": true, + }); + + 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")])) + } + #[test] fn test_tool_rules_default_returns_confirm() { let default_rules = ToolRules::default(); diff --git a/crates/agent_ui/src/agent_ui.rs b/crates/agent_ui/src/agent_ui.rs index 94390cfbbc..bc2a8f4147 100644 --- a/crates/agent_ui/src/agent_ui.rs +++ b/crates/agent_ui/src/agent_ui.rs @@ -943,6 +943,7 @@ mod tests { use_modifier_to_send: true, message_editor_min_lines: 1, tool_permissions: Default::default(), + sandbox_permissions: Default::default(), show_turn_stats: false, show_merge_conflict_indicator: true, sidebar_side: Default::default(), diff --git a/crates/agent_ui/src/conversation_view.rs b/crates/agent_ui/src/conversation_view.rs index 755c9629b1..242d4b3eae 100644 --- a/crates/agent_ui/src/conversation_view.rs +++ b/crates/agent_ui/src/conversation_view.rs @@ -389,7 +389,7 @@ impl Conversation { ) -> Option<()> { let (authorize_session_id, tool_call_id, options) = self.pending_tool_call(session_id, cx)?; - let option = options.first_option_of_kind(kind)?; + let option = permission_option_for_action(options, kind)?; self.authorize_tool_call( authorize_session_id, tool_call_id, @@ -453,6 +453,22 @@ pub(crate) struct RootThreadUpdated; impl EventEmitter for ConversationView {} +fn permission_option_for_action( + options: &PermissionOptions, + kind: acp::PermissionOptionKind, +) -> 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") + { + return Some(option); + } + + options.first_option_of_kind(kind) +} + fn resolve_outcome_from_selection( options: &PermissionOptions, selection: Option<&thread_view::PermissionSelection>, @@ -7517,6 +7533,42 @@ pub(crate) mod tests { ]) } + fn sandbox_permission_options() -> PermissionOptions { + PermissionOptions::Flat(vec![ + acp::PermissionOption::new( + acp::PermissionOptionId::new("allow"), + "Allow once", + acp::PermissionOptionKind::AllowOnce, + ), + acp::PermissionOption::new( + acp::PermissionOptionId::new("allow_thread"), + "Allow for this thread", + acp::PermissionOptionKind::AllowAlways, + ), + acp::PermissionOption::new( + acp::PermissionOptionId::new("allow_always"), + "Allow always", + acp::PermissionOptionKind::AllowAlways, + ), + acp::PermissionOption::new( + acp::PermissionOptionId::new("deny"), + "Deny", + acp::PermissionOptionKind::RejectOnce, + ), + ]) + } + + #[test] + fn permission_option_for_action_prefers_explicit_sandbox_allow_always() { + let options = sandbox_permission_options(); + + let option = + super::permission_option_for_action(&options, acp::PermissionOptionKind::AllowAlways) + .unwrap(); + + assert_eq!(option.option_id.0.as_ref(), "allow_always"); + } + #[test] fn resolve_outcome_from_selection_flat_allow_picks_allow_once() { let options = flat_allow_deny_options(); diff --git a/crates/agent_ui/src/conversation_view/thread_view.rs b/crates/agent_ui/src/conversation_view/thread_view.rs index 68cc7192df..c4ae1a2a95 100644 --- a/crates/agent_ui/src/conversation_view/thread_view.rs +++ b/crates/agent_ui/src/conversation_view/thread_view.rs @@ -7,7 +7,7 @@ use crate::{ use agent_client_protocol::schema as acp; use std::cell::RefCell; -use acp_thread::{ContentBlock, PlanEntry}; +use acp_thread::{ContentBlock, PlanEntry, SandboxAuthorizationDetails}; use agent::{SkillLoadingError, SkillLoadingErrorsUpdated}; use agent_settings::UserAgentsMd; use cloud_api_types::{SubmitAgentThreadFeedbackBody, SubmitAgentThreadFeedbackCommentsBody}; @@ -571,6 +571,7 @@ pub struct ThreadView { /// Used for showing/hiding tool call results, terminal output, etc. pub expanded_tool_calls: HashSet, pub expanded_tool_call_raw_inputs: HashSet, + collapsed_sandbox_authorization_details: HashSet, pub expanded_thinking_blocks: HashSet<(usize, usize)>, auto_expanded_thinking_block: Option<(usize, usize)>, user_toggled_thinking_blocks: HashSet<(usize, usize)>, @@ -885,6 +886,7 @@ impl ThreadView { thread_feedback: Default::default(), expanded_tool_calls: HashSet::default(), expanded_tool_call_raw_inputs: HashSet::default(), + collapsed_sandbox_authorization_details: HashSet::default(), expanded_thinking_blocks: HashSet::default(), auto_expanded_thinking_block: None, user_toggled_thinking_blocks: HashSet::default(), @@ -7027,6 +7029,17 @@ impl ThreadView { .into_any_element() }), ) + .when_some( + tool_call.sandbox_authorization_details.as_ref(), + |this, details| { + this.child(self.render_sandbox_authorization_details( + entry_ix, + &tool_call.id, + details, + cx, + )) + }, + ) .when(should_show_raw_input, |this| { let is_raw_input_expanded = self.expanded_tool_call_raw_inputs.contains(&tool_call.id); @@ -7392,6 +7405,93 @@ impl ThreadView { .children(tool_output_display) } + fn render_sandbox_authorization_details( + &self, + entry_ix: usize, + tool_call_id: &acp::ToolCallId, + details: &SandboxAuthorizationDetails, + cx: &Context, + ) -> AnyElement { + if details.write_paths.is_empty() { + return Empty.into_any_element(); + } + + let is_open = !self + .collapsed_sandbox_authorization_details + .contains(tool_call_id); + let paths = details + .write_paths + .iter() + .map(|path| path.display().to_string()) + .collect::>(); + + v_flex() + .p_2() + .gap_1() + .border_t_1() + .border_color(self.tool_card_border_color(cx)) + .child( + h_flex() + .id(("sandbox-authorization-details-header", entry_ix)) + .gap_1() + .pl_0p5() + .rounded_xs() + .hover(|style| style.bg(cx.theme().colors().element_hover)) + .child( + Disclosure::new(("sandbox-authorization-details", entry_ix), is_open) + .opened_icon(IconName::ChevronUp) + .closed_icon(IconName::ChevronDown), + ) + .child( + Label::new("Paths") + .size(LabelSize::XSmall) + .color(Color::Muted) + .buffer_font(cx), + ) + .on_click(cx.listener({ + let tool_call_id = tool_call_id.clone(); + move |this, _event, _window, cx| { + if this + .collapsed_sandbox_authorization_details + .remove(&tool_call_id) + { + cx.notify(); + return; + } + + this.collapsed_sandbox_authorization_details + .insert(tool_call_id.clone()); + cx.notify(); + } + })), + ) + .when(is_open, |this| { + this.child( + v_flex() + .gap_0p5() + .pl_5() + .children(paths.into_iter().enumerate().map(|(path_ix, path)| { + h_flex() + .gap_1() + .child(Label::new("•").size(LabelSize::Small).color(Color::Muted)) + .child( + div() + .id(format!( + "sandbox-authorization-path-{entry_ix}-{path_ix}" + )) + .w_full() + .max_w_full() + .overflow_x_scroll() + .child( + Label::new(path).buffer_font(cx).size(LabelSize::Small), + ), + ) + })), + ) + }) + .into_any_element() + } + fn render_permission_buttons( &self, session_id: acp::SessionId, @@ -7860,7 +7960,11 @@ impl ThreadView { Icon::new(IconName::CheckDouble) .size(IconSize::XSmall) .color(Color::Success), - Some(&AllowAlways as &dyn Action), + if option.option_id.0.as_ref() == "allow_thread" { + None + } else { + Some(&AllowAlways as &dyn Action) + }, ), acp::PermissionOptionKind::RejectOnce => ( Icon::new(IconName::Close) diff --git a/crates/settings_content/src/agent.rs b/crates/settings_content/src/agent.rs index 6697213602..bd859908b3 100644 --- a/crates/settings_content/src/agent.rs +++ b/crates/settings_content/src/agent.rs @@ -214,6 +214,11 @@ pub struct AgentSettingsContent { /// `always_confirm`) match against the tool's text input (command, path, /// URL, etc.). pub tool_permissions: Option, + + /// Persistent sandbox permission grants for agent-run terminal commands. + /// These are populated when choosing "Allow always" from a sandbox + /// escalation prompt. + pub sandbox_permissions: Option, } impl AgentSettingsContent { @@ -316,6 +321,39 @@ impl AgentSettingsContent { }); } } + + pub fn allow_sandbox_network(&mut self) { + self.sandbox_permissions + .get_or_insert_default() + .allow_network = Some(true); + } + + pub fn allow_sandbox_fs_write_all(&mut self) { + self.sandbox_permissions + .get_or_insert_default() + .allow_fs_write_all = Some(true); + } + + pub fn allow_sandbox_unsandboxed(&mut self) { + self.sandbox_permissions + .get_or_insert_default() + .allow_unsandboxed = Some(true); + } + + pub fn add_sandbox_write_path(&mut self, path: PathBuf) { + let write_paths = &mut self + .sandbox_permissions + .get_or_insert_default() + .write_paths + .get_or_insert_default() + .0; + + if write_paths.iter().any(|granted| path.starts_with(granted)) { + return; + } + write_paths.retain(|granted| !granted.starts_with(&path)); + write_paths.push(path); + } } #[with_fallible_options] @@ -570,6 +608,30 @@ pub enum CustomAgentServerSettings { }, } +#[with_fallible_options] +#[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize, JsonSchema, MergeFrom)] +pub struct SandboxPermissionsContent { + /// Whether sandboxed terminal commands may always use outbound network + /// access without prompting. + /// Default: false + pub allow_network: Option, + + /// Whether sandboxed terminal commands may always write anywhere on the + /// filesystem without prompting. + /// Default: false + pub allow_fs_write_all: Option, + + /// Whether terminal commands may always run outside the sandbox without + /// prompting when they request `unsandboxed: true`. + /// Default: false + pub allow_unsandboxed: Option, + + /// Directory subtrees that sandboxed terminal commands may always write + /// to without prompting. Paths written by Zed are absolute. + /// Default: [] + pub write_paths: Option>, +} + #[with_fallible_options] #[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize, JsonSchema, MergeFrom)] pub struct ToolPermissionsContent { @@ -835,4 +897,49 @@ mod tests { assert_eq!(always_deny.0.len(), 1); assert_eq!(always_deny.0[0].pattern, "^rm\\s"); } + + #[test] + fn test_allow_sandbox_permissions_create_structure() { + let mut settings = AgentSettingsContent::default(); + assert!(settings.sandbox_permissions.is_none()); + + settings.allow_sandbox_network(); + settings.allow_sandbox_fs_write_all(); + settings.allow_sandbox_unsandboxed(); + settings.add_sandbox_write_path(PathBuf::from("/tmp/build")); + + let sandbox_permissions = settings.sandbox_permissions.as_ref().unwrap(); + assert_eq!(sandbox_permissions.allow_network, Some(true)); + assert_eq!(sandbox_permissions.allow_fs_write_all, Some(true)); + assert_eq!(sandbox_permissions.allow_unsandboxed, Some(true)); + assert_eq!( + sandbox_permissions + .write_paths + .as_ref() + .unwrap() + .0 + .as_slice(), + &[PathBuf::from("/tmp/build")] + ); + } + + #[test] + fn test_add_sandbox_write_path_prunes_redundant_paths() { + let mut settings = AgentSettingsContent::default(); + + settings.add_sandbox_write_path(PathBuf::from("/tmp/build/cache")); + settings.add_sandbox_write_path(PathBuf::from("/tmp/build")); + settings.add_sandbox_write_path(PathBuf::from("/tmp/build/output")); + + let write_paths = settings + .sandbox_permissions + .as_ref() + .unwrap() + .write_paths + .as_ref() + .unwrap() + .0 + .as_slice(); + assert_eq!(write_paths, &[PathBuf::from("/tmp/build")]); + } }