diff --git a/crates/acp_thread/src/acp_thread.rs b/crates/acp_thread/src/acp_thread.rs index 2faaf54f19e196..8d5e71c292ed1e 100644 --- a/crates/acp_thread/src/acp_thread.rs +++ b/crates/acp_thread/src/acp_thread.rs @@ -3392,7 +3392,7 @@ impl AcpThread { let terminal_task = cx.spawn({ let terminal_id = terminal_id.clone(); async move |_this, cx| { - let env = env.await; + let mut env = env.await; let shell = project .update(cx, |project, cx| { project @@ -3404,8 +3404,14 @@ impl AcpThread { ShellBuilder::new(&Shell::Program(shell), is_windows) .redirect_stdin_to_dev_null() .build(Some(command.clone()), &args); + // Spawn the network proxy (if the wrap requests network) before + // generating the sandbox policy, since the policy must pin the + // child to the proxy's loopback port. This also injects the + // child's proxy env vars. + let (proxy_handle, network_policy) = + setup_network_proxy(sandbox_wrap.as_ref(), &mut env, cx)?; let (task_command, task_args, sandbox_config) = - apply_sandbox_wrap(task_command, task_args, sandbox_wrap)?; + apply_sandbox_wrap(task_command, task_args, sandbox_wrap, network_policy)?; let terminal = project .update(cx, |project, cx| { project.create_terminal_task( @@ -3430,6 +3436,7 @@ impl AcpThread { terminal, language_registry, sandbox_config, + proxy_handle, cx, ) })) @@ -3512,6 +3519,7 @@ impl AcpThread { // External terminal providers manage their own sandboxing // (if any). We don't wrap their commands. None, + None, cx, ) }); diff --git a/crates/acp_thread/src/terminal.rs b/crates/acp_thread/src/terminal.rs index 849a2869d2287e..be1fc6e9555204 100644 --- a/crates/acp_thread/src/terminal.rs +++ b/crates/acp_thread/src/terminal.rs @@ -1,8 +1,9 @@ use agent_client_protocol::schema as acp; use anyhow::Result; +use collections::HashMap; use futures::{FutureExt as _, future::Shared}; use gpui::{App, AppContext, AsyncApp, Context, Entity, Task}; -use http_proxy::Allowlist; +use http_proxy::{Allowlist, ProxyConfig, ProxyEvent, ProxyHandle, UpstreamProxy}; use language::LanguageRegistry; use markdown::Markdown; use project::Project; @@ -44,10 +45,24 @@ pub struct SandboxWrap { pub extra_write_paths: Vec, /// Outbound network the command may reach, as a hostname allowlist. An /// empty allowlist (the default) means no network. A non-empty allowlist - /// (or [`Allowlist::any`]) relaxes the sandbox's network policy. + /// (or [`Allowlist::any`]) causes an in-process HTTP/HTTPS proxy to be + /// spawned that enforces the policy, with the sandbox confined to its + /// loopback port. pub network: Allowlist, /// Allow unrestricted filesystem writes (ignores all writable paths). pub allow_fs_write: bool, + /// Whether the project (and therefore this terminal) is local. The + /// enforcing proxy binds a loopback port on this host, so it can only + /// confine local commands; a remote terminal can't reach it. + pub is_local: bool, +} + +impl SandboxWrap { + /// Whether this wrap requests any outbound network access, and therefore + /// needs the in-process proxy to be spawned. + fn wants_network(&self) -> bool { + !self.network.is_deny_all() + } } /// Opaque RAII handle the sandbox implementation hands back to keep its @@ -56,12 +71,27 @@ pub struct SandboxWrap { /// whose only job is to drop with the entity. pub type SandboxConfigHandle = Box; +/// The outbound-network policy resolved for a sandboxed command. +pub(crate) enum NetworkPolicy { + /// The command requested no outbound network. + Denied, + /// Egress is confined to the in-process proxy on this loopback port. + Proxied(u16), + /// Network was requested but couldn't be confined to the proxy (a + /// non-local project, or a host with no sandbox integration). The agent + /// layer widens such requests to "arbitrary network access" before + /// prompting (see `run_terminal_tool`), so the user approved exactly this. + Unrestricted, +} + /// Apply a [`SandboxWrap`] to a `(program, args)` pair, substituting the /// platform's sandbox-launcher invocation in place of the original. The /// returned `SandboxConfigHandle` (when `Some`) must be kept alive for the /// duration of the spawned command — dropping it deletes any on-disk /// config the launcher reads at startup. /// +/// `network_policy` is the decision resolved by [`setup_network_proxy`]; +/// /// On non-macOS hosts this is a no-op: the inputs pass through unchanged /// and the returned handle is `None`. (We don't yet have a sandbox /// integration for other platforms.) @@ -69,6 +99,7 @@ pub(crate) fn apply_sandbox_wrap( program: String, args: Vec, sandbox_wrap: Option, + network_policy: NetworkPolicy, ) -> anyhow::Result<(String, Vec, Option)> { let Some(sandbox_wrap) = sandbox_wrap else { return Ok((program, args, None)); @@ -84,10 +115,10 @@ pub(crate) fn apply_sandbox_wrap( .chain(sandbox_wrap.extra_write_paths.iter()) .map(|p| p.as_path()) .collect(); - let network = if sandbox_wrap.network.is_deny_all() { - NetworkAccess::None - } else { - NetworkAccess::All + let network = match network_policy { + NetworkPolicy::Proxied(port) => NetworkAccess::LocalhostPort(port), + NetworkPolicy::Unrestricted => NetworkAccess::All, + NetworkPolicy::Denied => NetworkAccess::None, }; let permissions = sandbox::macos_seatbelt::SandboxPermissions { network, @@ -105,11 +136,171 @@ pub(crate) fn apply_sandbox_wrap( { // No sandbox integration available; ignore the wrap request and // let the command run with the agent's ambient permissions. - let _ = sandbox_wrap; + if let NetworkPolicy::Proxied(port) = network_policy { + log::debug!( + "[sandbox/network] ignoring proxy port {port} because this platform has no sandbox integration" + ); + } + let _sandbox_wrap = sandbox_wrap; Ok((program, args, None)) } } +/// Spawn the in-process network proxy for a sandboxed command, if one is +/// needed, and wire the child's environment to route through it. +/// +/// Returns the proxy handle (which must outlive the command) alongside the +/// resolved [`NetworkPolicy`] the sandbox should enforce. The handle is `Some` +/// only when a proxy was actually spawned — a local macOS project that +/// requested network. In every other case it is `None`, and the policy is +/// [`NetworkPolicy::Denied`] (no network requested) or +/// [`NetworkPolicy::Unrestricted`] (network requested but unenforceable: a +/// non-local project, where the loopback proxy can't serve a remote terminal, +/// or a non-macOS host with no sandbox to confine the child). +pub(crate) fn setup_network_proxy( + sandbox_wrap: Option<&SandboxWrap>, + env: &mut HashMap, + cx: &mut AsyncApp, +) -> Result<(Option, NetworkPolicy)> { + let Some(sandbox_wrap) = sandbox_wrap else { + return Ok((None, NetworkPolicy::Denied)); + }; + if !sandbox_wrap.wants_network() { + return Ok((None, NetworkPolicy::Denied)); + } + // The proxy only buys us anything when a Seatbelt sandbox confines the + // child to its loopback port, and only works for local projects. When we + // can't enforce it, fall back to unrestricted egress (already approved as + // such by the agent layer). + if !cfg!(target_os = "macos") || !sandbox_wrap.is_local { + return Ok((None, NetworkPolicy::Unrestricted)); + } + + // Chain through the user's real upstream proxy if the command's environment + // names one. A malformed value shouldn't break the terminal, so log and skip. + let upstream = match upstream_proxy_from_child_env(env) { + Ok(upstream) => upstream, + Err(error) => { + log::warn!("[sandbox/network] ignoring upstream proxy env: {error:#}"); + None + } + }; + + let (events_tx, events_rx) = futures::channel::mpsc::unbounded(); + let handle = ProxyHandle::spawn(ProxyConfig { + allowlist: sandbox_wrap.network.clone(), + upstream, + events: events_tx, + })?; + let port = handle.port(); + + apply_proxy_env(env, port); + spawn_proxy_event_logger(events_rx, cx); + + Ok((Some(handle), NetworkPolicy::Proxied(port))) +} + +fn upstream_proxy_from_child_env(env: &HashMap) -> Result> { + let url = first_nonempty_env_value( + env, + &[ + "HTTPS_PROXY", + "https_proxy", + "ALL_PROXY", + "all_proxy", + "HTTP_PROXY", + "http_proxy", + ], + ); + let no_proxy = first_nonempty_env_value(env, &["NO_PROXY", "no_proxy"]); + UpstreamProxy::parse(url, no_proxy) +} + +fn first_nonempty_env_value<'a>( + env: &'a HashMap, + names: &[&str], +) -> Option<&'a str> { + for name in names { + if let Some(value) = env.get(*name) + && !value.trim().is_empty() + { + return Some(value.as_str()); + } + } + None +} + +/// Point the child's proxy env vars at the in-process proxy and strip any +/// inherited `NO_PROXY`. +/// +/// Both upper- and lower-case forms are set because some clients (notably +/// curl on macOS) only honor the lowercase variant. `NO_PROXY` is blanked +/// out so all egress goes through our proxy unconditionally: an inherited +/// `NO_PROXY` matching an allowlisted host would make the client attempt a +/// direct connection, which the Seatbelt rule blocks — surfacing as a +/// confusing "connection refused" instead of a clean policy decision. +fn apply_proxy_env(env: &mut HashMap, port: u16) { + let url = format!("http://127.0.0.1:{port}"); + for key in [ + "HTTPS_PROXY", + "https_proxy", + "HTTP_PROXY", + "http_proxy", + "ALL_PROXY", + "all_proxy", + ] { + env.insert(key.to_string(), url.clone()); + } + for key in ["NO_PROXY", "no_proxy"] { + env.insert(key.to_string(), String::new()); + } +} + +/// Drain the proxy's event channel, logging each event. v1 surfacing only; +/// future integrations (UI, telemetry) can replace or fan out this consumer. +fn spawn_proxy_event_logger( + mut events: futures::channel::mpsc::UnboundedReceiver, + cx: &mut AsyncApp, +) { + cx.background_spawn(async move { + use futures::StreamExt as _; + while let Some(event) = events.next().await { + log_proxy_event(&event); + } + }) + .detach(); +} + +fn log_proxy_event(event: &ProxyEvent) { + match event { + ProxyEvent::Ready { .. } => {} + ProxyEvent::RequestAttempt { + host, + port, + method, + outcome, + } => { + log::debug!( + "[sandbox/network] {} {host}:{port} → {outcome:?}", + method.as_str() + ); + } + ProxyEvent::RequestCompleted { + host, + port, + method, + bytes_to_remote, + bytes_from_remote, + duration_ms, + } => { + log::debug!( + "[sandbox/network] completed {} {host}:{port} sent={bytes_to_remote} recv={bytes_from_remote} duration={duration_ms}ms", + method.as_str(), + ); + } + } +} + pub struct Terminal { id: acp::TerminalId, command: Entity, @@ -123,10 +314,11 @@ pub struct Terminal { /// (e.g., clicking the Stop button). This is set before kill() is called /// so that code awaiting wait_for_exit() can check it deterministically. user_stopped: Arc, - /// RAII handle kept alive for the duration of the sandboxed command. - /// `None` when the command isn't sandboxed (the common case for - /// terminals not created by the agent). + /// Seatbelt config kept alive until the sandboxed command exits. + /// `None` when the command isn't sandboxed or after it finishes. _sandbox_config: Option, + /// In-process network proxy kept alive until the sandboxed command exits. + _network_proxy: Option, } pub struct TerminalOutput { @@ -146,12 +338,14 @@ impl Terminal { terminal: Entity, language_registry: Arc, sandbox_config: Option, + network_proxy: Option, cx: &mut Context, ) -> Self { let command_task = terminal.read(cx).wait_for_completed_task(cx); Self { id, _sandbox_config: sandbox_config, + _network_proxy: network_proxy, command: cx.new(|cx| { Markdown::new( format!("```\n{}\n```", command_label).into(), @@ -181,6 +375,14 @@ impl Terminal { original_content_len, content_line_count, }); + // Dropping the proxy handle joins its listener thread + // (after a loopback wakeup connect); do that off the + // foreground thread so a slow/wedged shutdown can't + // stall the UI. + if let Some(proxy) = this._network_proxy.take() { + cx.background_spawn(async move { drop(proxy) }).detach(); + } + this._sandbox_config = None; cx.notify(); }) .ok(); @@ -352,3 +554,64 @@ pub async fn create_terminal_entity( }) .await } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn upstream_proxy_from_child_env_uses_from_env_precedence() { + let mut env = HashMap::default(); + env.insert("HTTPS_PROXY".to_string(), " ".to_string()); + env.insert("https_proxy".to_string(), "http://lower:1111".to_string()); + env.insert("ALL_PROXY".to_string(), "http://all:2222".to_string()); + env.insert("HTTP_PROXY".to_string(), "http://http:3333".to_string()); + env.insert("NO_PROXY".to_string(), "".to_string()); + env.insert("no_proxy".to_string(), "internal.example".to_string()); + + let upstream = upstream_proxy_from_child_env(&env) + .expect("proxy env should parse") + .expect("proxy env should configure an upstream"); + + assert_eq!(upstream.host, "lower"); + assert_eq!(upstream.port, 1111); + assert!(upstream.bypasses("internal.example", 443)); + assert!(!upstream.bypasses("zed.dev", 443)); + } + + #[test] + fn apply_proxy_env_points_all_proxy_vars_at_proxy_and_blanks_no_proxy() { + let mut env = HashMap::default(); + env.insert("HTTPS_PROXY".to_string(), "http://corp:3128".to_string()); + env.insert("NO_PROXY".to_string(), "internal.example".to_string()); + env.insert("PATH".to_string(), "/usr/bin".to_string()); + + apply_proxy_env(&mut env, 54321); + + for key in [ + "HTTPS_PROXY", + "https_proxy", + "HTTP_PROXY", + "http_proxy", + "ALL_PROXY", + "all_proxy", + ] { + assert_eq!( + env.get(key).map(String::as_str), + Some("http://127.0.0.1:54321"), + "{key} should point at the in-process proxy" + ); + } + // An inherited NO_PROXY would make clients attempt direct + // connections that the Seatbelt rule blocks; it must be blanked. + for key in ["NO_PROXY", "no_proxy"] { + assert_eq!( + env.get(key).map(String::as_str), + Some(""), + "{key} should be blanked" + ); + } + // Unrelated variables pass through. + assert_eq!(env.get("PATH").map(String::as_str), Some("/usr/bin")); + } +} diff --git a/crates/agent/src/tools/terminal_tool.rs b/crates/agent/src/tools/terminal_tool.rs index f4c8db93af0bab..acf167be2fd040 100644 --- a/crates/agent/src/tools/terminal_tool.rs +++ b/crates/agent/src/tools/terminal_tool.rs @@ -415,6 +415,7 @@ async fn run_terminal_tool( extra_write_paths: effective.write_paths, network: network_request_to_allowlist(&effective.network), allow_fs_write: effective.allow_fs_write_all, + is_local: is_local_project, }) } else { None