diff --git a/crates/acp_thread/src/terminal.rs b/crates/acp_thread/src/terminal.rs index be1fc6e9555..95421953a9d 100644 --- a/crates/acp_thread/src/terminal.rs +++ b/crates/acp_thread/src/terminal.rs @@ -43,12 +43,8 @@ pub struct SandboxWrap { /// 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, - /// 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`]) 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, + /// Outbound network access explicitly approved for this command. + pub network: SandboxNetworkAccess, /// Allow unrestricted filesystem writes (ignores all writable paths). pub allow_fs_write: bool, /// Whether the project (and therefore this terminal) is local. The @@ -57,11 +53,25 @@ pub struct SandboxWrap { 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() +#[derive(Clone, Debug, Default)] +pub enum SandboxNetworkAccess { + /// Block all outbound network access. + #[default] + None, + /// Allow only hosts in this allowlist, enforced by routing HTTP/HTTPS + /// through an in-process proxy and confining the command to the proxy's + /// loopback port. + Restricted(Allowlist), + /// Allow unrestricted outbound network access. + All, +} + +impl SandboxNetworkAccess { + fn restricted_allowlist(&self) -> Option<&Allowlist> { + match self { + Self::Restricted(allowlist) => Some(allowlist), + Self::None | Self::All => None, + } } } @@ -77,10 +87,8 @@ pub(crate) enum NetworkPolicy { 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. + /// The command explicitly requested, and the user approved, unrestricted + /// outbound network access. Unrestricted, } @@ -90,7 +98,9 @@ pub(crate) enum NetworkPolicy { /// 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`]; +/// `network_policy` is the decision resolved by [`setup_network_proxy`]. +/// Unrestricted network access must be requested explicitly via +/// [`SandboxNetworkAccess::All`]. /// /// 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 @@ -146,17 +156,16 @@ pub(crate) fn apply_sandbox_wrap( } } -/// Spawn the in-process network proxy for a sandboxed command, if one is -/// needed, and wire the child's environment to route through it. +/// Spawn the in-process network proxy for a sandboxed command with restricted +/// network access, 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). +/// only when a proxy was actually spawned. Unrestricted network access skips +/// proxy setup and resolves to [`NetworkPolicy::Unrestricted`]. Restricted +/// network access requires a local macOS project so the sandbox can confine +/// egress to the proxy; otherwise this rejects the command instead of widening +/// it. pub(crate) fn setup_network_proxy( sandbox_wrap: Option<&SandboxWrap>, env: &mut HashMap, @@ -165,15 +174,19 @@ pub(crate) fn setup_network_proxy( let Some(sandbox_wrap) = sandbox_wrap else { return Ok((None, NetworkPolicy::Denied)); }; - if !sandbox_wrap.wants_network() { - return Ok((None, NetworkPolicy::Denied)); - } + let Some(allowlist) = sandbox_wrap.network.restricted_allowlist() else { + let policy = match &sandbox_wrap.network { + SandboxNetworkAccess::None => NetworkPolicy::Denied, + SandboxNetworkAccess::All => NetworkPolicy::Unrestricted, + SandboxNetworkAccess::Restricted(_) => unreachable!(), + }; + return Ok((None, policy)); + }; + // 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). + // child to its loopback port, and only works for local projects. if !cfg!(target_os = "macos") || !sandbox_wrap.is_local { - return Ok((None, NetworkPolicy::Unrestricted)); + anyhow::bail!("restricted network access requested, but no enforcing proxy is available"); } // Chain through the user's real upstream proxy if the command's environment @@ -188,7 +201,7 @@ pub(crate) fn setup_network_proxy( let (events_tx, events_rx) = futures::channel::mpsc::unbounded(); let handle = ProxyHandle::spawn(ProxyConfig { - allowlist: sandbox_wrap.network.clone(), + allowlist: allowlist.clone(), upstream, events: events_tx, })?; @@ -559,6 +572,19 @@ pub async fn create_terminal_entity( mod tests { use super::*; + #[test] + fn only_restricted_network_access_uses_proxy_allowlist() { + assert!(SandboxNetworkAccess::None.restricted_allowlist().is_none()); + assert!(SandboxNetworkAccess::All.restricted_allowlist().is_none()); + assert!( + SandboxNetworkAccess::Restricted(Allowlist::from_patterns([ + http_proxy::HostPattern::parse("example.com").unwrap() + ])) + .restricted_allowlist() + .is_some() + ); + } + #[test] fn upstream_proxy_from_child_env_uses_from_env_precedence() { let mut env = HashMap::default(); diff --git a/crates/agent/src/tools/terminal_tool.rs b/crates/agent/src/tools/terminal_tool.rs index acf167be2fd..08adc6795da 100644 --- a/crates/agent/src/tools/terminal_tool.rs +++ b/crates/agent/src/tools/terminal_tool.rs @@ -105,17 +105,17 @@ pub struct SandboxedTerminalToolInput { /// commands that fetch or upload (installing dependencies, cloning, /// pushing, downloading, etc.). Each entry must be a hostname or a /// leading-`*.` subdomain wildcard; IP literals and other wildcards are - /// rejected. Only HTTP and HTTPS are proxied — use `https://` URLs rather - /// than `git@`/`ssh://`. Requesting hosts triggers a user approval prompt, - /// so only list hosts you expect the command to need. + /// rejected. Host-specific access is enforced by an HTTP/HTTPS proxy — use + /// `https://` URLs rather than `git@`/`ssh://`. Requesting hosts triggers a + /// user approval prompt, so only list hosts you expect the command to need. #[serde(default)] pub allow_hosts: Vec, /// Set to `true` only if the command needs outbound network access to /// hosts you can't enumerate up front. /// - /// This is a broad escape hatch — prefer `allow_hosts` with specific - /// hostnames whenever possible, so the user knows what's being approved. - /// Requesting it triggers a user approval prompt. + /// This grants unrestricted outbound network access. Prefer `allow_hosts` + /// with specific hostnames whenever possible, so the user knows what's + /// being approved. Requesting it triggers a user approval prompt. #[serde(default)] pub allow_all_hosts: Option, /// Paths the command needs to write to outside the default-writable @@ -413,7 +413,7 @@ async fn run_terminal_tool( Some(acp_thread::SandboxWrap { writable_paths, extra_write_paths: effective.write_paths, - network: network_request_to_allowlist(&effective.network), + network: network_request_to_sandbox_network_access(&effective.network), allow_fs_write: effective.allow_fs_write_all, is_local: is_local_project, }) @@ -543,13 +543,17 @@ fn join_write_paths(raw_paths: &[String], base: Option<&Path>) -> Vec { .collect() } -/// Convert a (validated) network request into the allowlist enforced by the -/// in-process proxy. -fn network_request_to_allowlist(network: &NetworkRequest) -> http_proxy::Allowlist { +/// Convert a (validated) network request into the access mode enforced by the +/// terminal sandbox. +fn network_request_to_sandbox_network_access( + network: &NetworkRequest, +) -> acp_thread::SandboxNetworkAccess { match network { - NetworkRequest::None => http_proxy::Allowlist::empty(), - NetworkRequest::AnyHost => http_proxy::Allowlist::any(), - NetworkRequest::Hosts(hosts) => http_proxy::Allowlist::from_patterns(hosts.iter().cloned()), + NetworkRequest::None => acp_thread::SandboxNetworkAccess::None, + NetworkRequest::AnyHost => acp_thread::SandboxNetworkAccess::All, + NetworkRequest::Hosts(hosts) => acp_thread::SandboxNetworkAccess::Restricted( + http_proxy::Allowlist::from_patterns(hosts.iter().cloned()), + ), } } @@ -2570,6 +2574,27 @@ mod tests { assert!(err.contains("127.0.0.1"), "unexpected error: {err}"); } + #[test] + fn test_network_request_to_sandbox_network_access_uses_explicit_unrestricted_variant() { + match network_request_to_sandbox_network_access(&NetworkRequest::None) { + acp_thread::SandboxNetworkAccess::None => {} + other => panic!("expected no network access, got {other:?}"), + } + + match network_request_to_sandbox_network_access(&NetworkRequest::AnyHost) { + acp_thread::SandboxNetworkAccess::All => {} + other => panic!("expected unrestricted network access, got {other:?}"), + } + + match network_request_to_sandbox_network_access(&host_request(&["github.com"])) { + acp_thread::SandboxNetworkAccess::Restricted(allowlist) => { + assert!(allowlist.allows("github.com")); + assert!(!allowlist.allows("example.com")); + } + other => panic!("expected restricted network access, got {other:?}"), + } + } + #[test] fn test_input_schema_includes_sandbox_flags() { // The sandboxed terminal tool advertises these fields so the model can diff --git a/crates/http_proxy/src/allowlist.rs b/crates/http_proxy/src/allowlist.rs index e674c4c9c4f..fd6c37c77c7 100644 --- a/crates/http_proxy/src/allowlist.rs +++ b/crates/http_proxy/src/allowlist.rs @@ -227,8 +227,8 @@ impl Allowlist { Self::default() } - /// An allowlist that allows any host. Useful for `allow_all_hosts: true` - /// approval flow — we still proxy and observe, but skip the policy check. + /// An allowlist that allows any host. When used with the proxy, traffic is + /// still observed, but no host policy check is applied. pub fn any() -> Self { Self { patterns: Vec::new(), diff --git a/crates/http_proxy/tests/end_to_end.rs b/crates/http_proxy/tests/end_to_end.rs index a350324e4ee..076d07f8b28 100644 --- a/crates/http_proxy/tests/end_to_end.rs +++ b/crates/http_proxy/tests/end_to_end.rs @@ -284,8 +284,7 @@ fn ip_literal_connect_is_denied_for_pattern_allowlists() { #[test] fn ip_literal_connect_is_allowed_when_allowlist_allows_any() { - // `allow_all_hosts` matches the pre-allowlist `allow_network: true` - // semantics: unrestricted egress, IP literals included. + // `Allowlist::any()` preserves unrestricted proxy semantics, IP literals included. let (origin_addr, origin_join) = spawn_echo_origin(b"HTTP/1.1 200 OK\r\n\r\n"); let (proxy, mut events) = spawn_proxy(Allowlist::any());