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")]); + } }