From 09e5af196ed8a054d297eefb782f85195bfa7b50 Mon Sep 17 00:00:00 2001 From: Martin Ye Date: Tue, 16 Jun 2026 12:56:58 -0700 Subject: [PATCH 1/4] Add agent sandbox permissions settings page --- Cargo.lock | 1 + crates/agent/src/thread.rs | 18 +- .../src/conversation_view/thread_view.rs | 5 +- crates/settings_ui/Cargo.toml | 1 + crates/settings_ui/src/page_data.rs | 43 +- crates/settings_ui/src/pages.rs | 2 + .../settings_ui/src/pages/sandbox_settings.rs | 497 ++++++++++++++++++ crates/settings_ui/src/settings_ui.rs | 6 + crates/zed_actions/src/lib.rs | 7 +- 9 files changed, 534 insertions(+), 46 deletions(-) create mode 100644 crates/settings_ui/src/pages/sandbox_settings.rs diff --git a/Cargo.lock b/Cargo.lock index 6098df115fcd77..42cf79582d65bd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -16640,6 +16640,7 @@ dependencies = [ "gpui", "heck 0.5.0", "http_client", + "http_proxy", "itertools 0.14.0", "language", "language_model", diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index 07359eeb012941..1ae719c2001f7c 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -5419,7 +5419,6 @@ impl ToolCallEventStream { Ok(()) } Some(acp_thread::SandboxPermission::AllowAlways) => { - sandbox_grants.borrow_mut().record(request); Self::persist_sandbox_always_permission(request, fs, cx); Ok(()) } @@ -7019,7 +7018,9 @@ mod tests { } #[gpui::test] - async fn test_authorize_sandbox_allow_always_records_current_grant(cx: &mut TestAppContext) { + async fn test_authorize_sandbox_allow_always_does_not_cache_thread_grant( + cx: &mut TestAppContext, + ) { crate::tests::init_test(cx); let (event_stream, mut receiver) = ToolCallEventStream::test(); @@ -7095,18 +7096,15 @@ mod tests { assert!(send_result.is_ok()); authorize.await.unwrap(); + // "Allow always" persists to settings only 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"), - ] + assert!( + effective.write_paths.is_empty(), + "allow always should not record an in-memory thread grant: {:?}", + effective.write_paths ); } diff --git a/crates/agent_ui/src/conversation_view/thread_view.rs b/crates/agent_ui/src/conversation_view/thread_view.rs index d01c3f0305188f..03fedd135576ea 100644 --- a/crates/agent_ui/src/conversation_view/thread_view.rs +++ b/crates/agent_ui/src/conversation_view/thread_view.rs @@ -7315,12 +7315,11 @@ impl ThreadView { ) .icon_size(IconSize::XSmall) .icon_color(Color::Muted) - .tooltip(Tooltip::text("Open the unsandboxed execution setting")) + .tooltip(Tooltip::text("Open the sandbox permission settings")) .on_click(|_event, window, cx| { window.dispatch_action( Box::new(zed_actions::OpenSettingsAt { - path: zed_actions::AGENT_ALLOW_UNSANDBOXED_SETTINGS_PATH - .to_string(), + path: zed_actions::AGENT_SANDBOX_SETTINGS_PATH.to_string(), target: None, }), cx, diff --git a/crates/settings_ui/Cargo.toml b/crates/settings_ui/Cargo.toml index bb2f9c1f2b9a8d..b3a145c659887c 100644 --- a/crates/settings_ui/Cargo.toml +++ b/crates/settings_ui/Cargo.toml @@ -41,6 +41,7 @@ fuzzy.workspace = true gpui.workspace = true heck.workspace = true http_client.workspace = true +http_proxy.workspace = true itertools.workspace = true language.workspace = true language_model.workspace = true diff --git a/crates/settings_ui/src/page_data.rs b/crates/settings_ui/src/page_data.rs index 6f6b20f37fe63a..ab71cbecedbdd7 100644 --- a/crates/settings_ui/src/page_data.rs +++ b/crates/settings_ui/src/page_data.rs @@ -14,8 +14,8 @@ use crate::{ SettingsPage, SettingsPageItem, SubPageLink, USER, active_language, all_language_names, pages::{ open_audio_test_window, render_edit_prediction_setup_page, render_external_agents_page, - render_llm_providers_page, render_mcp_servers_page, render_skills_setup_page, - render_tool_permissions_setup_page, + render_llm_providers_page, render_mcp_servers_page, render_sandbox_settings_page, + render_skills_setup_page, render_tool_permissions_setup_page, }, }; @@ -7892,6 +7892,18 @@ fn ai_page(cx: &App) -> SettingsPage { files: USER | PROJECT, render: render_skills_setup_page, }), + SettingsPageItem::SubPageLink(SubPageLink { + title: "Sandbox".into(), + r#type: Default::default(), + json_path: Some(zed_actions::AGENT_SANDBOX_SETTINGS_PATH), + description: Some( + "Review and change the elevated terminal sandbox permissions that are always allowed without prompting." + .into(), + ), + in_json: true, + files: USER, + render: render_sandbox_settings_page, + }), SettingsPageItem::SubPageLink(SubPageLink { title: "Tool Permissions".into(), r#type: Default::default(), @@ -7930,33 +7942,6 @@ fn ai_page(cx: &App) -> SettingsPage { } items.extend([ - SettingsPageItem::SettingItem(SettingItem { - title: "Allow Unsandboxed Terminal Commands", - description: "When enabled, agent terminal commands run without the OS sandbox instead of prompting when the sandbox can't be created.", - field: Box::new(SettingField { - organization_override: None, - json_path: Some(zed_actions::AGENT_ALLOW_UNSANDBOXED_SETTINGS_PATH), - pick: |settings_content| { - settings_content - .agent - .as_ref()? - .sandbox_permissions - .as_ref()? - .allow_unsandboxed - .as_ref() - }, - write: |settings_content, value, _| { - settings_content - .agent - .get_or_insert_default() - .sandbox_permissions - .get_or_insert_default() - .allow_unsandboxed = value; - }, - }), - metadata: None, - files: USER, - }), SettingsPageItem::SettingItem(SettingItem { title: "Single File Review", description: "When enabled, agent edits will also be displayed in single-file buffers for review.", diff --git a/crates/settings_ui/src/pages.rs b/crates/settings_ui/src/pages.rs index 83943a8087c78c..ecb3eda8007544 100644 --- a/crates/settings_ui/src/pages.rs +++ b/crates/settings_ui/src/pages.rs @@ -5,6 +5,7 @@ mod external_agents_page; mod feature_flags; mod llm_providers_page; mod mcp_servers_page; +mod sandbox_settings; mod skill_creator; mod skills_setup; mod tool_permissions_setup; @@ -18,6 +19,7 @@ pub(crate) use external_agents_page::{CustomAgentForm, render_external_agents_pa pub(crate) use feature_flags::render_feature_flags_page; pub(crate) use llm_providers_page::render_llm_providers_page; pub(crate) use mcp_servers_page::{McpServerForm, render_mcp_servers_page}; +pub(crate) use sandbox_settings::render_sandbox_settings_page; pub use skill_creator::SkillCreatorOpenMode; pub(crate) use skill_creator::{ SkillCreatorEvent, SkillCreatorPage, render_skill_creator_page, skill_url_from_clipboard, diff --git a/crates/settings_ui/src/pages/sandbox_settings.rs b/crates/settings_ui/src/pages/sandbox_settings.rs new file mode 100644 index 00000000000000..cf0000d1c23c71 --- /dev/null +++ b/crates/settings_ui/src/pages/sandbox_settings.rs @@ -0,0 +1,497 @@ +use std::path::PathBuf; + +use agent_settings::AgentSettings; +use gpui::{ReadGlobal as _, ScrollHandle, prelude::*}; +use http_proxy::HostPattern; +use settings::{Settings as _, SettingsStore}; +use ui::{Banner, Divider, Severity, SwitchField, ToggleState, Tooltip, prelude::*}; +use util::ResultExt as _; + +use crate::SettingsWindow; +use crate::components::{SettingsInputField, SettingsSectionHeader}; + +const SANDBOX_DISCLAIMER: &str = "Customize how the sandbox for the agents tool should behave."; + +const DOMAINS_DESCRIPTION: &str = "Each entry is an exact domain (github.com) or a leading-*. subdomain wildcard (*.npmjs.org). IP addresses and local domains are not allowed."; + +const WRITE_PATHS_DESCRIPTION: &str = + "Each entry must be an absolute path and grants write access to the whole subtree."; + +pub(crate) fn render_sandbox_settings_page( + settings_window: &SettingsWindow, + scroll_handle: &ScrollHandle, + _window: &mut Window, + cx: &mut Context, +) -> AnyElement { + // Sandbox permissions are a user-level setting; they aren't configurable + // per-project, so always operate against the global value here. + let permissions = AgentSettings::get_global(cx).sandbox_permissions.clone(); + let validation_error = settings_window.sandbox_host_validation_error.clone(); + + // Read the list values from the raw user settings content rather than the + // compiled `AgentSettings`. The compiled `write_paths` are lexically + // normalized (see `compile_sandbox_permissions`), so editing or removing a + // row by the normalized value would fail to match the literal entry stored + // in settings.json and silently leave the permission in place. + let (network_hosts, write_paths) = raw_sandbox_lists(cx); + + let host_rows: Vec = network_hosts + .into_iter() + .enumerate() + .map(|(index, host)| render_host_row(index, host, cx)) + .collect(); + let add_host_input = render_add_host_input(cx); + + let path_rows: Vec = write_paths + .into_iter() + .enumerate() + .map(|(index, path)| render_path_row(index, path, cx)) + .collect(); + let add_path_input = render_add_path_input(cx); + + let empty_border = cx.theme().colors().border_variant; + + v_flex() + .id("sandbox-settings-page") + .size_full() + .pt_2p5() + .px_8() + .pb_16() + .gap_4() + .overflow_y_scroll() + .track_scroll(scroll_handle) + .child( + Banner::new().child( + Label::new(SANDBOX_DISCLAIMER) + .size(LabelSize::Small) + .color(Color::Muted) + .mt_0p5(), + ), + ) + .when_some(validation_error, |this, error| { + this.child( + Banner::new() + .severity(Severity::Warning) + .child(Label::new(error).size(LabelSize::Small)) + .action_slot( + Button::new("dismiss-sandbox-host-error", "Dismiss") + .style(ButtonStyle::Tinted(ui::TintColor::Warning)) + .on_click(cx.listener(|this, _, _, cx| { + this.sandbox_host_validation_error = None; + cx.notify(); + })), + ), + ) + }) + .child( + v_flex() + .gap_3() + .child(SettingsSectionHeader::new("Network").no_padding(true)) + .child( + SwitchField::new( + "sandbox-allow-all-hosts", + Some("Allow All Domains"), + Some( + "Let sandboxed commands reach any domain over the network without prompting." + .into(), + ), + permissions.allow_all_hosts, + move |state, _window, cx| { + set_allow_all_hosts(*state == ToggleState::Selected, cx); + }, + ) + .tab_index(0), + ) + .child(render_list_section( + "Allowed Domains", + DOMAINS_DESCRIPTION, + host_rows, + add_host_input, + empty_border, + )), + ) + .child(Divider::horizontal()) + .child( + v_flex() + .gap_3() + .child(SettingsSectionHeader::new("Filesystem").no_padding(true)) + .child( + SwitchField::new( + "sandbox-allow-fs-write-all", + Some("Allow All Filesystem Writes"), + Some( + "Let sandboxed commands write anywhere on the filesystem without prompting." + .into(), + ), + permissions.allow_fs_write_all, + move |state, _window, cx| { + set_allow_fs_write_all(*state == ToggleState::Selected, cx); + }, + ) + .tab_index(0), + ) + .child(render_list_section( + "Writable Paths", + WRITE_PATHS_DESCRIPTION, + path_rows, + add_path_input, + empty_border, + )), + ) + .child(Divider::horizontal()) + .child( + v_flex() + .gap_3() + .child(SettingsSectionHeader::new("Sandbox").no_padding(true)) + .child( + SwitchField::new( + "sandbox-allow-unsandboxed", + Some("Allow Unsandboxed Terminal Commands"), + Some( + "Run terminal commands without the OS sandbox." + .into(), + ), + permissions.allow_unsandboxed, + move |state, _window, cx| { + set_allow_unsandboxed(*state == ToggleState::Selected, cx); + }, + ) + .tab_index(0), + ), + ) + .into_any_element() +} + +fn render_list_section( + title: &'static str, + description: &'static str, + rows: Vec, + add_input: AnyElement, + empty_border: gpui::Hsla, +) -> impl IntoElement { + let is_empty = rows.is_empty(); + + v_flex() + .gap_0p5() + .child(Label::new(title)) + .child( + Label::new(description) + .size(LabelSize::Small) + .color(Color::Muted), + ) + .child( + v_flex() + .mt_2() + .w_full() + .gap_1p5() + .when(is_empty, |this| { + this.child(render_empty_state(empty_border)) + }) + .when(!is_empty, |this| { + this.child(v_flex().gap_1p5().children(rows)) + }) + .child(add_input), + ) +} + +fn render_empty_state(border_color: gpui::Hsla) -> AnyElement { + h_flex() + .p_2() + .rounded_md() + .border_1() + .border_dashed() + .border_color(border_color) + .child( + Label::new("Nothing configured") + .size(LabelSize::Small) + .color(Color::Disabled), + ) + .into_any_element() +} + +fn render_host_row(index: usize, host: String, cx: &mut Context) -> AnyElement { + let host_for_delete = host.clone(); + let host_for_update = host.clone(); + let settings_window = cx.entity().downgrade(); + + SettingsInputField::new() + .with_id(format!("sandbox-host-{}", index)) + .with_initial_text(host) + .tab_index(0) + .with_buffer_font() + .color(Color::Default) + .action_slot( + IconButton::new(format!("sandbox-host-delete-{}", index), IconName::Trash) + .icon_size(IconSize::Small) + .icon_color(Color::Muted) + .tooltip(Tooltip::text("Remove Domain")) + .on_click(cx.listener(move |_, _, _, cx| { + remove_network_host(host_for_delete.clone(), cx); + })), + ) + .on_confirm(move |new_host, _window, cx| { + let Some(new_host) = new_host else { + return; + }; + let new_host = new_host.trim().to_string(); + if new_host.is_empty() || new_host == host_for_update { + return; + } + let result = canonicalize_host(&new_host); + settings_window + .update(cx, |this, cx| { + match result { + Ok(canonical) => { + this.sandbox_host_validation_error = None; + update_network_host(host_for_update.clone(), canonical, cx); + } + Err(error) => { + this.sandbox_host_validation_error = Some(error); + } + } + cx.notify(); + }) + .log_err(); + }) + .into_any_element() +} + +fn render_add_host_input(cx: &mut Context) -> AnyElement { + let settings_window = cx.entity().downgrade(); + + SettingsInputField::new() + .with_id("sandbox-host-new") + .with_placeholder("Add domain (e.g. github.com or *.npmjs.org)…") + .tab_index(0) + .with_buffer_font() + .display_clear_button() + .display_confirm_button() + .clear_on_confirm() + .on_confirm(move |host, _window, cx| { + let Some(host) = host else { + return; + }; + let host = host.trim().to_string(); + if host.is_empty() { + return; + } + let result = canonicalize_host(&host); + settings_window + .update(cx, |this, cx| { + match result { + Ok(canonical) => { + this.sandbox_host_validation_error = None; + add_network_host(canonical, cx); + } + Err(error) => { + this.sandbox_host_validation_error = Some(error); + } + } + cx.notify(); + }) + .log_err(); + }) + .into_any_element() +} + +fn render_path_row(index: usize, path: PathBuf, cx: &mut Context) -> AnyElement { + let path_for_delete = path.clone(); + let path_for_update = path.clone(); + let settings_window = cx.entity().downgrade(); + + SettingsInputField::new() + .with_id(format!("sandbox-path-{}", index)) + .with_initial_text(path.to_string_lossy().into_owned()) + .tab_index(0) + .with_buffer_font() + .color(Color::Default) + .action_slot( + IconButton::new(format!("sandbox-path-delete-{}", index), IconName::Trash) + .icon_size(IconSize::Small) + .icon_color(Color::Muted) + .tooltip(Tooltip::text("Remove Path")) + .on_click(cx.listener(move |_, _, _, cx| { + remove_write_path(path_for_delete.clone(), cx); + })), + ) + .on_confirm(move |new_path, _window, cx| { + let Some(new_path) = new_path else { + return; + }; + let new_path = new_path.trim(); + if new_path.is_empty() { + return; + } + let new_path = PathBuf::from(new_path); + if new_path == path_for_update { + return; + } + update_write_path(path_for_update.clone(), new_path, cx); + settings_window.update(cx, |_, cx| cx.notify()).log_err(); + }) + .into_any_element() +} + +fn render_add_path_input(cx: &mut Context) -> AnyElement { + let settings_window = cx.entity().downgrade(); + + SettingsInputField::new() + .with_id("sandbox-path-new") + .with_placeholder("Add an absolute path (e.g. /path/to/directory)…") + .tab_index(0) + .with_buffer_font() + .display_clear_button() + .display_confirm_button() + .clear_on_confirm() + .on_confirm(move |path, _window, cx| { + let Some(path) = path else { + return; + }; + let path = path.trim(); + if path.is_empty() { + return; + } + add_write_path(PathBuf::from(path), cx); + settings_window.update(cx, |_, cx| cx.notify()).log_err(); + }) + .into_any_element() +} + +/// The literal host and write-path lists as stored in user settings.json. These +/// are the exact strings/paths that edits and removals must match against. +fn raw_sandbox_lists(cx: &App) -> (Vec, Vec) { + let store = SettingsStore::global(cx); + let permissions = store + .raw_user_settings() + .and_then(|user| user.content.agent.as_ref()) + .and_then(|agent| agent.sandbox_permissions.as_ref()); + + let network_hosts = permissions + .and_then(|permissions| permissions.network_hosts.as_ref()) + .map(|hosts| hosts.0.clone()) + .unwrap_or_default(); + let write_paths = permissions + .and_then(|permissions| permissions.write_paths.as_ref()) + .map(|paths| paths.0.clone()) + .unwrap_or_default(); + + (network_hosts, write_paths) +} + +/// Validate and canonicalize a user-provided domain, returning either the +/// canonical form to persist or a domain-friendly error to surface. +fn canonicalize_host(host: &str) -> Result { + use http_proxy::HostPatternError; + + HostPattern::parse(host) + .map(|pattern| pattern.to_string()) + .map_err(|error| match error { + HostPatternError::Empty => "Domain cannot be empty.".to_string(), + HostPatternError::IpLiteral(_) => { + "IP addresses and local domains aren't allowed; enter a domain like github.com." + .to_string() + } + HostPatternError::InvalidWildcard(_) => { + "Wildcards are only allowed as a leading label, e.g. *.github.com.".to_string() + } + HostPatternError::Invalid { .. } => { + "Not a valid domain. Use a domain like github.com or *.npmjs.org.".to_string() + } + }) +} + +fn update_sandbox_permissions( + cx: &mut App, + update: impl 'static + Send + FnOnce(&mut settings::SandboxPermissionsContent), +) { + SettingsStore::global(cx).update_settings_file(::global(cx), move |settings, _| { + update( + settings + .agent + .get_or_insert_default() + .sandbox_permissions + .get_or_insert_default(), + ); + }); +} + +fn set_allow_all_hosts(value: bool, cx: &mut App) { + update_sandbox_permissions(cx, move |permissions| { + permissions.allow_all_hosts = Some(value); + }); +} + +fn set_allow_fs_write_all(value: bool, cx: &mut App) { + update_sandbox_permissions(cx, move |permissions| { + permissions.allow_fs_write_all = Some(value); + }); +} + +fn set_allow_unsandboxed(value: bool, cx: &mut App) { + update_sandbox_permissions(cx, move |permissions| { + permissions.allow_unsandboxed = Some(value); + }); +} + +fn add_network_host(host: String, cx: &mut App) { + update_sandbox_permissions(cx, move |permissions| { + let hosts = &mut permissions.network_hosts.get_or_insert_default().0; + if !hosts.contains(&host) { + hosts.push(host); + } + }); +} + +fn update_network_host(old_host: String, new_host: String, cx: &mut App) { + update_sandbox_permissions(cx, move |permissions| { + let hosts = &mut permissions.network_hosts.get_or_insert_default().0; + if hosts.contains(&new_host) { + return; + } + if let Some(entry) = hosts.iter_mut().find(|host| **host == old_host) { + *entry = new_host; + } + }); +} + +fn remove_network_host(host: String, cx: &mut App) { + update_sandbox_permissions(cx, move |permissions| { + if let Some(hosts) = permissions.network_hosts.as_mut() { + hosts.0.retain(|entry| *entry != host); + } + }); +} + +fn add_write_path(path: PathBuf, cx: &mut App) { + // Normalize away `.`/`..` so the stored entry matches the form the runtime + // uses for coverage checks (see `compile_sandbox_permissions`) and the form + // persisted by the in-thread "Allow always" grant. + let Ok(path) = util::paths::normalize_lexically(&path) else { + return; + }; + update_sandbox_permissions(cx, move |permissions| { + let paths = &mut permissions.write_paths.get_or_insert_default().0; + // Store minimal subtrees so a parent path subsumes its descendants. + util::paths::insert_subtree(paths, path); + }); +} + +fn update_write_path(old_path: PathBuf, new_path: PathBuf, cx: &mut App) { + let Ok(new_path) = util::paths::normalize_lexically(&new_path) else { + return; + }; + update_sandbox_permissions(cx, move |permissions| { + if let Some(paths) = permissions.write_paths.as_mut() { + paths.0.retain(|entry| *entry != old_path); + util::paths::insert_subtree(&mut paths.0, new_path); + } + }); +} + +fn remove_write_path(path: PathBuf, cx: &mut App) { + update_sandbox_permissions(cx, move |permissions| { + if let Some(paths) = permissions.write_paths.as_mut() { + paths.0.retain(|entry| *entry != path); + } + }); +} diff --git a/crates/settings_ui/src/settings_ui.rs b/crates/settings_ui/src/settings_ui.rs index a1b69c9a7beefe..ebf90ac1c22456 100644 --- a/crates/settings_ui/src/settings_ui.rs +++ b/crates/settings_ui/src/settings_ui.rs @@ -888,6 +888,7 @@ pub struct SettingsWindow { shown_errors: HashSet, pub(crate) hidden_deleted_skill_directory_paths: HashSet, pub(crate) regex_validation_error: Option, + pub(crate) sandbox_host_validation_error: Option, last_copied_link_path: Option<&'static str>, /// Cached configuration views per provider, created lazily. Holds the /// provider's chosen presentation ([`Inline`] or [`SubPage`]). @@ -1906,6 +1907,7 @@ impl SettingsWindow { shown_errors: HashSet::default(), hidden_deleted_skill_directory_paths: HashSet::default(), regex_validation_error: None, + sandbox_host_validation_error: None, list_state, last_copied_link_path: None, provider_configuration_views: HashMap::default(), @@ -4065,6 +4067,7 @@ impl SettingsWindow { window: &mut Window, cx: &mut Context, ) { + self.sandbox_host_validation_error = None; self.sub_page_stack .push(SubPage::new(sub_page_link, section_header)); self.content_focus_handle.focus_handle(cx).focus(window, cx); @@ -4269,6 +4272,7 @@ impl SettingsWindow { pub(crate) fn pop_sub_page(&mut self, window: &mut Window, cx: &mut Context) { self.regex_validation_error = None; + self.sandbox_host_validation_error = None; if let Some(popped) = self.sub_page_stack.pop() && popped.link.r#type == SubPageType::SkillCreator { @@ -5082,6 +5086,7 @@ pub mod test { shown_errors: HashSet::default(), hidden_deleted_skill_directory_paths: HashSet::default(), regex_validation_error: None, + sandbox_host_validation_error: None, last_copied_link_path: None, provider_configuration_views: HashMap::default(), configuring_provider: None, @@ -5217,6 +5222,7 @@ pub mod test { shown_errors: HashSet::default(), hidden_deleted_skill_directory_paths: HashSet::default(), regex_validation_error: None, + sandbox_host_validation_error: None, last_copied_link_path: None, provider_configuration_views: HashMap::default(), configuring_provider: None, diff --git a/crates/zed_actions/src/lib.rs b/crates/zed_actions/src/lib.rs index 376e6835fc4156..c16bf5ef22fc4a 100644 --- a/crates/zed_actions/src/lib.rs +++ b/crates/zed_actions/src/lib.rs @@ -152,10 +152,9 @@ pub struct OpenSettingsAt { /// `OpenSettingsAt` path of the agent skills page in the settings UI. pub const AGENT_SKILLS_SETTINGS_PATH: &str = "agent.skills"; -/// `OpenSettingsAt` path of the "allow unsandboxed terminal commands" setting -/// in the settings UI. -pub const AGENT_ALLOW_UNSANDBOXED_SETTINGS_PATH: &str = - "agent.sandbox_permissions.allow_unsandboxed"; +/// `OpenSettingsAt` path of the agent sandbox permissions page in the settings +/// UI. +pub const AGENT_SANDBOX_SETTINGS_PATH: &str = "agent.sandbox_permissions"; #[derive(PartialEq, Clone, Debug, Deserialize, JsonSchema)] #[serde(rename_all = "snake_case")] From 412c10f8349e138bf6494838285c7a4d362a3ee1 Mon Sep 17 00:00:00 2001 From: Martin Ye Date: Tue, 16 Jun 2026 13:51:54 -0700 Subject: [PATCH 2/4] add test --- crates/agent/src/thread.rs | 22 +++++ crates/agent/src/tools/terminal_tool.rs | 125 ++++++++++++++++++++++++ 2 files changed, 147 insertions(+) diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index 1ae719c2001f7c..bf9c0f3bc17f47 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -5031,6 +5031,28 @@ impl ToolCallEventStream { (stream, receiver) } + /// Like [`Self::test`], but the returned stream shares the provided + /// thread-scoped sandbox grants. This mirrors how a real [`Thread`] builds a + /// distinct event stream per tool call while sharing one set of grants, so + /// tests can exercise sequences of tool calls within the same conversation. + #[cfg(any(test, feature = "test-support"))] + pub(crate) fn test_with_grants( + sandbox_grants: Rc>, + ) -> (Self, ToolCallEventStreamReceiver) { + let (events_tx, events_rx) = mpsc::unbounded::>(); + let (_cancellation_tx, cancellation_rx) = watch::channel(false); + + let stream = ToolCallEventStream::new( + "test_id".into(), + ThreadEventStream(events_tx), + None, + cancellation_rx, + sandbox_grants, + ); + + (stream, ToolCallEventStreamReceiver(events_rx)) + } + #[cfg(any(test, feature = "test-support"))] pub fn test_with_cancellation() -> (Self, ToolCallEventStreamReceiver, watch::Sender) { let (events_tx, events_rx) = mpsc::unbounded::>(); diff --git a/crates/agent/src/tools/terminal_tool.rs b/crates/agent/src/tools/terminal_tool.rs index 4e1c8f488309eb..89ab71516f08f2 100644 --- a/crates/agent/src/tools/terminal_tool.rs +++ b/crates/agent/src/tools/terminal_tool.rs @@ -2928,6 +2928,131 @@ mod tests { assert_eq!(environment.terminal_creation_count(), 0); } + /// Regression test: choosing "Allow always" on a sandbox prompt must persist + /// the grant to settings *only* — it must not also cache an in-memory thread + /// grant. Otherwise removing the entry from settings.json wouldn't revoke it + /// within the same conversation, and a later identical command would run + /// without prompting again (the bug this guards against). + #[cfg(target_os = "macos")] + #[gpui::test] + async fn test_allow_always_grant_is_revocable_via_settings(cx: &mut gpui::TestAppContext) { + use feature_flags::FeatureFlagAppExt as _; + + crate::tests::init_test(cx); + // Auto-allow the terminal tool itself so only the *sandbox* escalation + // prompts, and start with no persisted sandbox grants (mirroring a + // settings.json that doesn't grant the path — e.g. after the user + // removed it). + 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); + settings.sandbox_permissions = agent_settings::SandboxPermissions::default(); + 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; + + // Both tool calls belong to the same conversation, so they share one set + // of in-memory thread sandbox grants, exactly like a real `Thread`. + let sandbox_grants = std::rc::Rc::new(std::cell::RefCell::new( + crate::sandboxing::ThreadSandboxGrants::default(), + )); + + let input = serde_json::json!({ + "command": "touch build/output", + "cd": "root", + "fs_write_paths": ["build"], + "reason": "needs to write build artifacts", + }); + + // ---- First call: the user picks "Allow always". + 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.clone(), + environment.clone(), + )); + let (event_stream, mut receiver) = + crate::ToolCallEventStream::test_with_grants(sandbox_grants.clone()); + let resolved: SandboxedTerminalToolInput = serde_json::from_value(input.clone()).unwrap(); + let task = cx.update(|cx| tool.run(crate::ToolInput::resolved(resolved), event_stream, cx)); + + let authorization = receiver.expect_authorization().await; + authorization + .response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow_always"), + acp::PermissionOptionKind::AllowAlways, + )) + .expect("authorization response should send"); + task.await.expect("granted command should run"); + assert_eq!(environment.terminal_creation_count(), 1); + + // The grant must NOT have been cached in the shared thread grants: with + // empty persistent settings, the thread grants should cover nothing. + let cached = sandbox_grants.borrow().effective_with_persistent( + &crate::sandboxing::SandboxRequest::default(), + &agent_settings::SandboxPermissions::default(), + ); + assert!( + cached.write_paths.is_empty(), + "\"Allow always\" must not cache an in-memory thread grant: {:?}", + cached.write_paths + ); + + // ---- Second call: the same request, with the path absent from + // settings, must prompt again instead of being silently allowed. + let environment2 = 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 tool2 = std::sync::Arc::new(SandboxedTerminalTool::new( + project.clone(), + environment2.clone(), + )); + let (event_stream2, mut receiver2) = + crate::ToolCallEventStream::test_with_grants(sandbox_grants.clone()); + let resolved2: SandboxedTerminalToolInput = serde_json::from_value(input).unwrap(); + let task2 = + cx.update(|cx| tool2.run(crate::ToolInput::resolved(resolved2), event_stream2, cx)); + + let authorization2 = receiver2.expect_authorization().await; + let details = + acp_thread::sandbox_authorization_details_from_meta(&authorization2.tool_call.meta) + .expect("the identical request should prompt for sandbox authorization again"); + assert!( + details + .write_paths + .iter() + .any(|path| path.ends_with("build")), + "re-prompt should request the same write path: {:?}", + details.write_paths + ); + + authorization2 + .response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("deny"), + acp::PermissionOptionKind::RejectOnce, + )) + .expect("authorization response should send"); + let result = task2 + .await + .expect("denied sandbox request returns model-readable output"); + assert!(result.contains("user denied the requested sandbox permissions")); + assert_eq!(environment2.terminal_creation_count(), 0); + } + fn host_request(list: &[&str]) -> NetworkRequest { NetworkRequest::Hosts( list.iter() From 3d2e28cdda5754caa05dccce3148f702e6e0faab Mon Sep 17 00:00:00 2001 From: Martin Ye Date: Tue, 16 Jun 2026 13:58:21 -0700 Subject: [PATCH 3/4] fix --- crates/agent/src/thread.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index bf9c0f3bc17f47..507e6572196682 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -5035,7 +5035,7 @@ impl ToolCallEventStream { /// thread-scoped sandbox grants. This mirrors how a real [`Thread`] builds a /// distinct event stream per tool call while sharing one set of grants, so /// tests can exercise sequences of tool calls within the same conversation. - #[cfg(any(test, feature = "test-support"))] + #[cfg(test)] pub(crate) fn test_with_grants( sandbox_grants: Rc>, ) -> (Self, ToolCallEventStreamReceiver) { From 71aafa2608eb64ee138ff5e64432be21f7f5b17f Mon Sep 17 00:00:00 2001 From: Martin Ye Date: Tue, 16 Jun 2026 14:05:31 -0700 Subject: [PATCH 4/4] clippy fix --- crates/agent/src/thread.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index 507e6572196682..34585d27fbbc77 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -5035,7 +5035,9 @@ impl ToolCallEventStream { /// thread-scoped sandbox grants. This mirrors how a real [`Thread`] builds a /// distinct event stream per tool call while sharing one set of grants, so /// tests can exercise sequences of tool calls within the same conversation. - #[cfg(test)] + // Only the macOS-gated terminal sandbox regression test uses this, so match + // its `cfg` to avoid a dead-code error on other platforms. + #[cfg(all(test, target_os = "macos"))] pub(crate) fn test_with_grants( sandbox_grants: Rc>, ) -> (Self, ToolCallEventStreamReceiver) {