diff --git a/codex-rs/core/src/tools/handlers/view_image.rs b/codex-rs/core/src/tools/handlers/view_image.rs index fd4e3910db..29592a3fef 100644 --- a/codex-rs/core/src/tools/handlers/view_image.rs +++ b/codex-rs/core/src/tools/handlers/view_image.rs @@ -279,6 +279,10 @@ mod tests { use crate::tools::context::ToolInvocation; use crate::turn_diff_tracker::TurnDiffTracker; use codex_protocol::models::PermissionProfile; + use codex_protocol::permissions::FileSystemAccessMode; + use codex_protocol::permissions::FileSystemSandboxEntry; + use codex_protocol::permissions::FileSystemSandboxPolicy; + use codex_protocol::permissions::NetworkSandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; use core_test_support::TempDirExt; @@ -373,7 +377,13 @@ mod tests { panic!("primary environment should be ready"); }; environment.config_mut().permission_profile = - PermissionProfileSnapshot::legacy(PermissionProfile::read_only()); + PermissionProfileSnapshot::legacy(PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry::new( + image_cwd.into(), + FileSystemAccessMode::Read, + )]), + NetworkSandboxPolicy::Restricted, + )); let turn = Arc::new(turn); let result = ViewImageHandler::default() diff --git a/codex-rs/exec-server/src/capability_discovery.rs b/codex-rs/exec-server/src/capability_discovery.rs index 4799db98d2..a32d84f87a 100644 --- a/codex-rs/exec-server/src/capability_discovery.rs +++ b/codex-rs/exec-server/src/capability_discovery.rs @@ -85,7 +85,7 @@ async fn discover_root( #[cfg(target_os = "windows")] if sandbox.is_some_and(|context| { - context.should_run_in_sandbox() && !context.windows_sandbox_is_requested() + context.should_read_from_sandbox() && !context.windows_sandbox_is_requested() }) { discovery.error = Some("filesystem sandbox is unavailable on this executor".to_string()); return discovery; diff --git a/codex-rs/exec-server/src/environment.rs b/codex-rs/exec-server/src/environment.rs index bc1e68491f..29c1502b5c 100644 --- a/codex-rs/exec-server/src/environment.rs +++ b/codex-rs/exec-server/src/environment.rs @@ -989,7 +989,7 @@ impl Environment { if params.roots.iter().any(|root| { root.sandbox .as_ref() - .is_some_and(crate::FileSystemSandboxContext::should_run_in_sandbox) + .is_some_and(crate::FileSystemSandboxContext::should_read_from_sandbox) }) && !client .environment_info() .await? diff --git a/codex-rs/exec-server/src/local_file_system.rs b/codex-rs/exec-server/src/local_file_system.rs index f4b29363f2..ea0a0f0346 100644 --- a/codex-rs/exec-server/src/local_file_system.rs +++ b/codex-rs/exec-server/src/local_file_system.rs @@ -92,7 +92,7 @@ impl LocalFileSystem { }) } - fn file_system_for<'a>( + fn file_system_for_reads<'a>( &'a self, sandbox: Option<&'a FileSystemSandboxContext>, ) -> io::Result<( @@ -102,10 +102,27 @@ impl LocalFileSystem { if let Some(sandbox) = sandbox { sandbox.validate_file_system_paths_for_current_host()?; } - if sandbox.is_some_and(FileSystemSandboxContext::should_run_in_sandbox) { + if sandbox.is_some_and(FileSystemSandboxContext::should_read_from_sandbox) { Ok((self.sandboxed()?, sandbox)) } else { - Ok((&self.unsandboxed, sandbox)) + Ok((&self.unsandboxed, None)) + } + } + + fn file_system_for_writes<'a>( + &'a self, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> io::Result<( + &'a dyn ExecutorFileSystem, + Option<&'a FileSystemSandboxContext>, + )> { + if let Some(sandbox) = sandbox { + sandbox.validate_file_system_paths_for_current_host()?; + } + if sandbox.is_some_and(FileSystemSandboxContext::should_write_into_sandbox) { + Ok((self.sandboxed()?, sandbox)) + } else { + Ok((&self.unsandboxed, None)) } } } @@ -119,10 +136,12 @@ impl LocalFileSystem { if let Some(sandbox) = sandbox { sandbox.validate_file_system_paths_for_current_host()?; } - if sandbox.is_some_and(FileSystemSandboxContext::should_run_in_sandbox) { + if sandbox.is_some_and(FileSystemSandboxContext::should_read_from_sandbox) { return self.sandboxed()?.open_file_for_read(path, sandbox).await; } - self.unsandboxed.open_file_for_read(path, sandbox).await + self.unsandboxed + .open_file_for_read(path, /*sandbox*/ None) + .await } async fn canonicalize( @@ -130,14 +149,14 @@ impl LocalFileSystem { path: &PathUri, sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult { - let (file_system, sandbox) = self.file_system_for(sandbox)?; + let (file_system, sandbox) = self.file_system_for_reads(sandbox)?; file_system.canonicalize(path, sandbox).await } #[tracing::instrument( name = "fs.read_file", skip_all, - fields(sandboxed = sandbox.is_some_and(FileSystemSandboxContext::should_run_in_sandbox)) + fields(sandboxed = sandbox.is_some_and(FileSystemSandboxContext::should_read_from_sandbox)) )] async fn read_file( &self, @@ -145,7 +164,7 @@ impl LocalFileSystem { options: ReadFileOptions, sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult> { - let (file_system, sandbox) = self.file_system_for(sandbox)?; + let (file_system, sandbox) = self.file_system_for_reads(sandbox)?; file_system.read_file(path, options, sandbox).await } @@ -154,7 +173,7 @@ impl LocalFileSystem { path: &PathUri, sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult { - let (file_system, sandbox) = self.file_system_for(sandbox)?; + let (file_system, sandbox) = self.file_system_for_reads(sandbox)?; file_system.read_file_stream(path, sandbox).await } @@ -165,7 +184,7 @@ impl LocalFileSystem { options: WriteFileOptions, sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult<()> { - let (file_system, sandbox) = self.file_system_for(sandbox)?; + let (file_system, sandbox) = self.file_system_for_writes(sandbox)?; file_system .write_file(path, contents, options, sandbox) .await @@ -177,14 +196,14 @@ impl LocalFileSystem { options: CreateDirectoryOptions, sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult<()> { - let (file_system, sandbox) = self.file_system_for(sandbox)?; + let (file_system, sandbox) = self.file_system_for_writes(sandbox)?; file_system.create_directory(path, options, sandbox).await } #[tracing::instrument( name = "fs.get_metadata", skip_all, - fields(sandboxed = sandbox.is_some_and(FileSystemSandboxContext::should_run_in_sandbox)) + fields(sandboxed = sandbox.is_some_and(FileSystemSandboxContext::should_read_from_sandbox)) )] async fn get_metadata( &self, @@ -192,7 +211,7 @@ impl LocalFileSystem { options: GetMetadataOptions, sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult { - let (file_system, sandbox) = self.file_system_for(sandbox)?; + let (file_system, sandbox) = self.file_system_for_reads(sandbox)?; file_system.get_metadata(path, options, sandbox).await } @@ -201,7 +220,7 @@ impl LocalFileSystem { path: &PathUri, sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult> { - let (file_system, sandbox) = self.file_system_for(sandbox)?; + let (file_system, sandbox) = self.file_system_for_reads(sandbox)?; file_system.read_directory(path, sandbox).await } @@ -211,7 +230,7 @@ impl LocalFileSystem { options: WalkOptions, sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult { - let (file_system, sandbox) = self.file_system_for(sandbox)?; + let (file_system, sandbox) = self.file_system_for_reads(sandbox)?; file_system.walk(path, options, sandbox).await } @@ -221,7 +240,7 @@ impl LocalFileSystem { options: RemoveOptions, sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult<()> { - let (file_system, sandbox) = self.file_system_for(sandbox)?; + let (file_system, sandbox) = self.file_system_for_writes(sandbox)?; file_system.remove(path, options, sandbox).await } @@ -232,7 +251,7 @@ impl LocalFileSystem { options: CopyOptions, sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult<()> { - let (file_system, sandbox) = self.file_system_for(sandbox)?; + let (file_system, sandbox) = self.file_system_for_writes(sandbox)?; file_system .copy(source_path, destination_path, options, sandbox) .await @@ -1173,7 +1192,9 @@ fn file_metadata(metadata: std::fs::Metadata, is_symlink: bool) -> FileMetadata } fn reject_platform_sandbox_context(sandbox: Option<&FileSystemSandboxContext>) -> io::Result<()> { - if sandbox.is_some_and(FileSystemSandboxContext::should_run_in_sandbox) { + if sandbox.is_some_and(|context| { + context.should_read_from_sandbox() || context.should_write_into_sandbox() + }) { return Err(io::Error::new( io::ErrorKind::InvalidInput, "sandboxed filesystem operations require configured runtime paths", diff --git a/codex-rs/exec-server/src/remote_file_system.rs b/codex-rs/exec-server/src/remote_file_system.rs index 33d3b0be69..353dd24f32 100644 --- a/codex-rs/exec-server/src/remote_file_system.rs +++ b/codex-rs/exec-server/src/remote_file_system.rs @@ -68,7 +68,7 @@ impl RemoteFileSystem { let response = client .fs_canonicalize(FsCanonicalizeParams { path: path.clone(), - sandbox: remote_sandbox_context(sandbox), + sandbox: sandbox.cloned(), }) .await .map_err(map_remote_error)?; @@ -87,7 +87,7 @@ impl RemoteFileSystem { .fs_read_file(FsReadFileParams { path: path.clone(), follow_symlinks: (!options.follow_symlinks).then_some(false), - sandbox: remote_sandbox_context(sandbox), + sandbox: sandbox.cloned(), }) .await .map_err(map_remote_error)?; @@ -123,7 +123,7 @@ impl RemoteFileSystem { path: path.clone(), data_base64: STANDARD.encode(contents), follow_symlinks: (!options.follow_symlinks).then_some(false), - sandbox: remote_sandbox_context(sandbox), + sandbox: sandbox.cloned(), }) .await; self.metadata_requests.lock().await.clear(); @@ -144,7 +144,7 @@ impl RemoteFileSystem { path: path.clone(), recursive: Some(options.recursive), follow_symlinks: (!options.follow_symlinks).then_some(false), - sandbox: remote_sandbox_context(sandbox), + sandbox: sandbox.cloned(), }) .await; self.metadata_requests.lock().await.clear(); @@ -204,7 +204,7 @@ impl RemoteFileSystem { .fs_get_metadata(FsGetMetadataParams { path: path.clone(), follow_symlinks: (!options.follow_symlinks).then_some(false), - sandbox: remote_sandbox_context(sandbox), + sandbox: sandbox.cloned(), }) .await .map_err(map_remote_error)?; @@ -228,7 +228,7 @@ impl RemoteFileSystem { let response = client .fs_read_directory(FsReadDirectoryParams { path: path.clone(), - sandbox: remote_sandbox_context(sandbox), + sandbox: sandbox.cloned(), }) .await .map_err(map_remote_error)?; @@ -255,7 +255,7 @@ impl RemoteFileSystem { .fs_walk(FsWalkParams { path: path.clone(), options, - sandbox: remote_sandbox_context(sandbox), + sandbox: sandbox.cloned(), }) .await .map_err(map_remote_error) @@ -275,7 +275,7 @@ impl RemoteFileSystem { recursive: Some(options.recursive), force: Some(options.force), follow_symlinks: (!options.follow_symlinks).then_some(false), - sandbox: remote_sandbox_context(sandbox), + sandbox: sandbox.cloned(), }) .await; self.metadata_requests.lock().await.clear(); @@ -297,7 +297,7 @@ impl RemoteFileSystem { source_path: source_path.clone(), destination_path: destination_path.clone(), recursive: options.recursive, - sandbox: remote_sandbox_context(sandbox), + sandbox: sandbox.cloned(), }) .await; self.metadata_requests.lock().await.clear(); @@ -407,12 +407,6 @@ impl ExecutorFileSystem for RemoteFileSystem { } } -fn remote_sandbox_context( - sandbox: Option<&FileSystemSandboxContext>, -) -> Option { - sandbox.cloned() -} - fn map_remote_error(error: ExecServerError) -> io::Error { match error { ExecServerError::Server { code, message } if code == NOT_FOUND_ERROR_CODE => { @@ -435,61 +429,10 @@ mod path_uri_tests; #[cfg(test)] mod tests { - use codex_protocol::models::PermissionProfile; - use codex_protocol::permissions::FileSystemAccessMode; - use codex_protocol::permissions::FileSystemPath; - use codex_protocol::permissions::FileSystemSandboxEntry; - use codex_protocol::permissions::FileSystemSandboxPolicy; - use codex_protocol::permissions::FileSystemSpecialPath; - use codex_protocol::permissions::NetworkSandboxPolicy; - use codex_utils_absolute_path::AbsolutePathBuf; - use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; use super::*; - #[test] - fn remote_sandbox_context_preserves_cwd_for_absolute_permissions() { - let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { - path: FileSystemPath::Path { - path: absolute_test_path("remote-root").into(), - }, - access: FileSystemAccessMode::Read, - missing_path_behavior: None, - }]); - let permissions = - PermissionProfile::from_runtime_permissions(&policy, NetworkSandboxPolicy::Restricted); - let cwd = path_uri("executor-checkout"); - let sandbox_context = - FileSystemSandboxContext::from_permission_profile(permissions, cwd.clone()); - - let remote_context = - remote_sandbox_context(Some(&sandbox_context)).expect("remote sandbox context"); - - assert_eq!(remote_context.cwd, cwd); - } - - #[test] - fn remote_sandbox_context_preserves_required_cwd() { - let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { - path: FileSystemPath::Special { - value: FileSystemSpecialPath::project_roots(/*subpath*/ None), - }, - access: FileSystemAccessMode::Write, - missing_path_behavior: None, - }]); - let permissions = - PermissionProfile::from_runtime_permissions(&policy, NetworkSandboxPolicy::Restricted); - let cwd = path_uri("host-checkout"); - let sandbox_context = - FileSystemSandboxContext::from_permission_profile(permissions, cwd.clone()); - - let remote_context = - remote_sandbox_context(Some(&sandbox_context)).expect("remote sandbox context"); - - assert_eq!(remote_context.cwd, cwd); - } - #[test] fn transport_errors_map_to_broken_pipe() { let errors = [ @@ -519,13 +462,4 @@ mod tests { ] ); } - - fn absolute_test_path(name: &str) -> AbsolutePathBuf { - let path = std::env::temp_dir().join(name); - AbsolutePathBuf::from_absolute_path(&path).expect("absolute path") - } - - fn path_uri(name: &str) -> PathUri { - PathUri::from_abs_path(&absolute_test_path(name)) - } } diff --git a/codex-rs/exec-server/src/sandboxed_file_system.rs b/codex-rs/exec-server/src/sandboxed_file_system.rs index 61c3c06126..0d3e90fa05 100644 --- a/codex-rs/exec-server/src/sandboxed_file_system.rs +++ b/codex-rs/exec-server/src/sandboxed_file_system.rs @@ -444,7 +444,7 @@ fn require_platform_sandbox( sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult<&FileSystemSandboxContext> { sandbox - .filter(|sandbox| sandbox.should_run_in_sandbox()) + .filter(|sandbox| sandbox.should_read_from_sandbox() || sandbox.should_write_into_sandbox()) .ok_or_else(|| { io::Error::new( io::ErrorKind::InvalidInput, diff --git a/codex-rs/exec-server/src/sandboxed_file_system_path_uri_tests.rs b/codex-rs/exec-server/src/sandboxed_file_system_path_uri_tests.rs index f39f01e0e4..ff8875cc69 100644 --- a/codex-rs/exec-server/src/sandboxed_file_system_path_uri_tests.rs +++ b/codex-rs/exec-server/src/sandboxed_file_system_path_uri_tests.rs @@ -11,6 +11,122 @@ use tokio::io; use super::*; +/// Reads and writes follow their own policies, and temporary rules follow the executor's OS. +#[test] +fn sandbox_routing_uses_independent_read_write_policies_and_executor_temp_rules() { + use FileSystemAccessMode::Deny; + use FileSystemAccessMode::Read; + use FileSystemAccessMode::Write; + use FileSystemSpecialPath::Minimal; + use FileSystemSpecialPath::Root; + use FileSystemSpecialPath::SlashTmp; + use FileSystemSpecialPath::Tmpdir; + + for (cwd, slash_tmp_applies, foreign_path) in [ + ("file:///workspace", true, "file:///C:/private"), + ("file:///C:/workspace", false, "file:///private"), + ] { + let cwd = PathUri::parse(cwd).expect("valid executor cwd"); + for (entries, expected) in [ + (vec![(Root, Read)], (false, true)), + (vec![(Root, Write)], (false, false)), + (vec![(Root, Read), (Minimal, Deny)], (true, true)), + (vec![(Root, Write), (Minimal, Deny)], (true, false)), + ( + vec![(Root, Read), (SlashTmp, Deny)], + (slash_tmp_applies, true), + ), + ( + vec![(Root, Write), (SlashTmp, Deny)], + (slash_tmp_applies, slash_tmp_applies), + ), + (vec![(Root, Read), (Tmpdir, Deny)], (true, true)), + (vec![(Root, Write), (Tmpdir, Deny)], (true, true)), + ] { + let policy = FileSystemSandboxPolicy::restricted( + entries + .into_iter() + .map(|(value, access)| { + FileSystemSandboxEntry::new(FileSystemPath::Special { value }, access) + }) + .collect(), + ); + let context = FileSystemSandboxContext::from_permission_profile( + PermissionProfile::from_runtime_permissions( + &policy, + NetworkSandboxPolicy::Restricted, + ), + cwd.clone(), + ); + assert_eq!( + ( + context.should_read_from_sandbox(), + context.should_write_into_sandbox() + ), + expected, + "cwd={cwd}, policy={policy:?}", + ); + } + + let foreign = PathUri::parse(foreign_path).expect("valid foreign path"); + for (entries, expected) in [ + ( + vec![FileSystemSandboxEntry::new(cwd.clone().into(), Read)], + (true, true), + ), + ( + vec![ + FileSystemSandboxEntry::new(FileSystemPath::Special { value: Root }, Write), + FileSystemSandboxEntry::new(foreign.clone().into(), Deny), + ], + (true, true), + ), + ( + vec![ + FileSystemSandboxEntry::new(FileSystemPath::Special { value: Root }, Read), + FileSystemSandboxEntry::new(foreign.clone().into(), Write), + ], + (false, true), + ), + ( + vec![ + FileSystemSandboxEntry::new(FileSystemPath::Special { value: Root }, Write), + FileSystemSandboxEntry::new(foreign.into(), Write), + ], + (false, false), + ), + ] { + let policy = FileSystemSandboxPolicy::restricted(entries); + let context = FileSystemSandboxContext::from_permission_profile( + PermissionProfile::from_runtime_permissions( + &policy, + NetworkSandboxPolicy::Restricted, + ), + cwd.clone(), + ); + assert_eq!( + ( + context.should_read_from_sandbox(), + context.should_write_into_sandbox() + ), + expected, + "cwd={cwd}, policy={policy:?}", + ); + } + } + + let opaque = PathUri::parse("file:///%00/bad/path/YQ").expect("valid opaque path"); + let context = + FileSystemSandboxContext::from_permission_profile(PermissionProfile::read_only(), opaque); + assert_eq!( + ( + context.should_read_from_sandbox(), + context.should_write_into_sandbox() + ), + (false, true), + ); +} + #[tokio::test] async fn sandboxed_file_system_rejects_non_native_uri_as_invalid_input() { let runtime_paths = ExecServerRuntimePaths::new( diff --git a/codex-rs/exec-server/src/server/file_system_handler.rs b/codex-rs/exec-server/src/server/file_system_handler.rs index cdddddf689..54ea3e29c1 100644 --- a/codex-rs/exec-server/src/server/file_system_handler.rs +++ b/codex-rs/exec-server/src/server/file_system_handler.rs @@ -82,7 +82,7 @@ impl FileSystemHandler { sandbox .validate_file_system_paths_for_current_host() .is_ok() - && sandbox.should_run_in_sandbox() + && sandbox.should_read_from_sandbox() && (!cfg!(target_os = "windows") || sandbox.windows_sandbox_is_requested()) && params .roots diff --git a/codex-rs/exec-server/tests/environment.rs b/codex-rs/exec-server/tests/environment.rs index 814ae7f2d2..55b6ba3c53 100644 --- a/codex-rs/exec-server/tests/environment.rs +++ b/codex-rs/exec-server/tests/environment.rs @@ -12,11 +12,19 @@ use codex_exec_server::ExecutorCapabilityDiscoveryCache; use codex_exec_server::REMOTE_ENVIRONMENT_ID; use codex_exec_server::SelectedCapabilityRootsStatus; use codex_exec_server_protocol::CAPABILITY_ROOTS_DISCOVER_METHOD; +use codex_exec_server_protocol::CapabilityRootDiscoverRequest; +use codex_exec_server_protocol::CapabilityRootsDiscoverParams; +use codex_file_system::FileSystemSandboxContext; use codex_http_client::HttpClientFactory; use codex_http_client::OutboundProxyPolicy; use codex_http_client::cache_system_proxy_route_for_test; use codex_protocol::capabilities::CapabilityRootLocation; use codex_protocol::capabilities::SelectedCapabilityRoot; +use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::NetworkSandboxPolicy; use codex_utils_path_uri::PathUri; use common::exec_server::exec_server; use futures::SinkExt; @@ -107,6 +115,138 @@ async fn prepared_remote_environment_uses_configured_system_proxy() -> anyhow::R Ok(()) } +/// Older discovery executors receive direct-read contexts unchanged but never receive restricted reads. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn capability_discovery_without_sandbox_support_only_sends_full_disk_reads() +-> anyhow::Result<()> { + let server = exec_server().await?; + let listener = TcpListener::bind("127.0.0.1:0").await?; + let proxy_url = format!("ws://{}", listener.local_addr()?); + let upstream_url = server.websocket_url().to_string(); + let (requests_tx, mut requests_rx) = tokio::sync::mpsc::unbounded_channel(); + let _proxy = AbortOnDropHandle::new(tokio::spawn(async move { + let (stream, _) = listener.accept().await?; + let mut downstream = accept_async(stream).await?; + let (mut upstream, _) = connect_async(&upstream_url).await?; + loop { + tokio::select! { + message = downstream.next() => { + let Some(message) = message.transpose()? else { break }; + if let Message::Text(text) = &message { + let request: serde_json::Value = serde_json::from_str(text.as_ref())?; + if request.get("method").and_then(serde_json::Value::as_str) + == Some(CAPABILITY_ROOTS_DISCOVER_METHOD) + { + requests_tx.send(request)?; + } + } + upstream.send(message).await?; + } + message = upstream.next() => { + let Some(mut message) = message.transpose()? else { break }; + if let Message::Text(text) = &message { + let mut response: serde_json::Value = serde_json::from_str(text.as_ref())?; + for pointer in [ + "/result/environmentInfo/capabilities/capabilityDiscoverySandbox", + "/result/capabilities/capabilityDiscoverySandbox", + ] { + if let Some(capability) = response.pointer_mut(pointer) { + *capability = false.into(); + } + } + message = Message::Text(response.to_string().into()); + } + downstream.send(message).await?; + } + } + } + Ok::<(), anyhow::Error>(()) + })); + let manager = + EnvironmentManager::create_for_tests(Some(proxy_url), /*local_runtime_paths*/ None).await; + let environment = manager + .default_environment() + .context("remote environment")?; + assert!( + !environment + .info() + .await? + .capabilities + .capability_discovery_sandbox + ); + + let workspace = tempfile::tempdir()?; + let cwd = PathUri::from_host_native_path(workspace.path())?; + let full_read = FileSystemSandboxContext::from_permission_profile( + PermissionProfile::read_only(), + cwd.clone(), + ); + let request = CapabilityRootDiscoverRequest { + id: "root".to_string(), + path: cwd.clone(), + sandbox: Some(full_read), + }; + let unrestricted = CapabilityRootDiscoverRequest { + id: "unrestricted".to_string(), + path: cwd.clone(), + sandbox: Some(FileSystemSandboxContext::from_permission_profile( + PermissionProfile::Disabled, + cwd.clone(), + )), + }; + let response = environment + .discover_capability_roots(CapabilityRootsDiscoverParams { + roots: vec![request.clone(), unrestricted.clone()], + }) + .await?; + assert_eq!( + response + .roots + .iter() + .map(|root| (root.id.as_str(), &root.error)) + .collect::>(), + vec![("root", &None), ("unrestricted", &None)] + ); + let sent = timeout(Duration::from_secs(5), requests_rx.recv()) + .await? + .context("discovery request")?; + let sent: CapabilityRootsDiscoverParams = serde_json::from_value(sent["params"].clone())?; + assert_eq!(sent.roots, vec![request.clone(), unrestricted]); + + let restricted = FileSystemSandboxContext::from_permission_profile( + PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry::new( + cwd.clone().into(), + FileSystemAccessMode::Read, + )]), + NetworkSandboxPolicy::Restricted, + ), + cwd, + ); + let error = environment + .discover_capability_roots(CapabilityRootsDiscoverParams { + roots: vec![ + request.clone(), + CapabilityRootDiscoverRequest { + sandbox: Some(restricted), + ..request + }, + ], + }) + .await + .expect_err("restricted reads must not fall back to old discovery"); + assert!( + error + .to_string() + .contains("exec-server does not support sandboxed capability discovery") + ); + assert_eq!( + requests_rx.try_recv(), + Err(tokio::sync::mpsc::error::TryRecvError::Empty) + ); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[serial_test::serial(remote_exec_server)] async fn selected_capability_inspection_tracks_connection_recovery() -> anyhow::Result<()> { diff --git a/codex-rs/exec-server/tests/file_system/shared.rs b/codex-rs/exec-server/tests/file_system/shared.rs index 928d65cd7a..4b3ecd0b9a 100644 --- a/codex-rs/exec-server/tests/file_system/shared.rs +++ b/codex-rs/exec-server/tests/file_system/shared.rs @@ -4,11 +4,10 @@ use codex_exec_server::CopyOptions; use codex_exec_server::CreateDirectoryOptions; #[cfg(unix)] use codex_exec_server::ExecServerRuntimePaths; -#[cfg(unix)] use codex_exec_server::ExecutorFileSystem; use codex_exec_server::FILE_READ_CHUNK_SIZE; use codex_exec_server::FileMetadata; -#[cfg(unix)] +use codex_exec_server::FileSystemSandboxContext; use codex_exec_server::LocalFileSystem; use codex_exec_server::ReadDirectoryEntry; use codex_exec_server::RemoveOptions; @@ -23,6 +22,12 @@ use codex_file_system::MAX_WALK_ENTRIES; use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemPath; +use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::FileSystemSpecialPath; +use codex_protocol::permissions::NetworkSandboxPolicy; use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy; use codex_sandboxing::policy_transforms::effective_network_sandbox_policy; use codex_utils_path_uri::PathUri; @@ -903,6 +908,181 @@ async fn file_system_sandboxed_metadata_and_read_allow_readable_root( Ok(()) } +/// Full-disk read permission allows all read APIs without a helper; mutations still require one. +#[tokio::test] +async fn file_system_full_disk_read_skips_sandbox_only_for_reads() -> Result<()> { + let file_system = LocalFileSystem::unsandboxed(); + let tmp = TempDir::new()?; + let file = tmp.path().join("note.txt"); + std::fs::write(&file, b"note")?; + let file_uri = PathUri::from_host_native_path(&file)?; + let root_uri = PathUri::from_host_native_path(tmp.path())?; + let destination = PathUri::from_host_native_path(tmp.path().join("destination"))?; + let sandbox = FileSystemSandboxContext::from_permission_profile( + PermissionProfile::read_only(), + root_uri.clone(), + ); + + assert_eq!( + file_system.canonicalize(&file_uri, Some(&sandbox)).await?, + PathUri::from_host_native_path(std::fs::canonicalize(&file)?)? + ); + assert_eq!( + file_system + .read_file(&file_uri, Default::default(), Some(&sandbox)) + .await?, + b"note" + ); + assert_eq!( + file_system + .read_file_stream(&file_uri, Some(&sandbox)) + .await? + .try_collect::>() + .await? + .concat(), + b"note" + ); + let metadata = file_system + .get_metadata(&file_uri, Default::default(), Some(&sandbox)) + .await?; + assert_eq!( + metadata, + FileMetadata { + is_directory: false, + is_file: true, + is_symlink: false, + size: 4, + created_at_ms: metadata.created_at_ms, + modified_at_ms: metadata.modified_at_ms, + } + ); + assert_eq!( + file_system + .read_directory(&root_uri, Some(&sandbox)) + .await?, + vec![ReadDirectoryEntry { + file_name: "note.txt".to_string(), + is_directory: false, + is_file: true, + }] + ); + assert_eq!( + file_system + .walk( + &root_uri, + WalkOptions { + max_depth: 1, + max_directories: 2, + max_entries: 2, + follow_directory_symlinks: false, + prune_hidden_directories: false, + }, + Some(&sandbox), + ) + .await?, + WalkOutcome { + entries: vec![WalkEntry { + path: file_uri.clone(), + kind: WalkEntryKind::File, + }], + errors: Vec::new(), + truncated: false, + } + ); + + let mutations = [ + file_system + .write_file( + &file_uri, + b"changed".to_vec(), + Default::default(), + Some(&sandbox), + ) + .await, + file_system + .create_directory( + &destination, + CreateDirectoryOptions { + recursive: false, + follow_symlinks: true, + }, + Some(&sandbox), + ) + .await, + file_system + .remove( + &file_uri, + RemoveOptions { + recursive: false, + force: false, + follow_symlinks: true, + }, + Some(&sandbox), + ) + .await, + file_system + .copy( + &file_uri, + &destination, + CopyOptions { recursive: false }, + Some(&sandbox), + ) + .await, + ]; + for mutation in mutations { + let error = mutation.expect_err("restricted writes must require the sandbox"); + assert_eq!( + error.to_string(), + "sandboxed filesystem operations require configured runtime paths" + ); + } + assert_eq!(std::fs::read(&file)?, b"note"); + assert!(!tmp.path().join("destination").exists()); + Ok(()) +} + +/// A read deny keeps both ordinary and streaming reads from falling back to direct access. +#[tokio::test] +async fn file_system_restricted_reads_require_sandbox() -> Result<()> { + let file_system = LocalFileSystem::unsandboxed(); + let tmp = TempDir::new()?; + let file = tmp.path().join("note.txt"); + std::fs::write(&file, b"note")?; + let file_uri = PathUri::from_host_native_path(&file)?; + let sandbox = FileSystemSandboxContext::from_permission_profile( + PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + FileSystemAccessMode::Read, + ), + FileSystemSandboxEntry::new(absolute_path(file).into(), FileSystemAccessMode::Deny), + ]), + NetworkSandboxPolicy::Restricted, + ), + PathUri::from_host_native_path(tmp.path())?, + ); + + let read = file_system + .read_file(&file_uri, Default::default(), Some(&sandbox)) + .await + .map(|_| ()); + let stream = file_system + .read_file_stream(&file_uri, Some(&sandbox)) + .await + .map(|_| ()); + for result in [read, stream] { + let error = result.expect_err("restricted reads must require the sandbox"); + assert_eq!( + error.to_string(), + "sandboxed filesystem operations require configured runtime paths" + ); + } + Ok(()) +} + #[cfg(unix)] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn sandboxed_file_operations_cannot_read_helper_siblings() -> Result<()> { diff --git a/codex-rs/exec-server/tests/file_system_windows.rs b/codex-rs/exec-server/tests/file_system_windows.rs index 7bc41bfe7c..37e1cc4042 100644 --- a/codex-rs/exec-server/tests/file_system_windows.rs +++ b/codex-rs/exec-server/tests/file_system_windows.rs @@ -236,10 +236,20 @@ async fn file_system_operations_can_reject_junctions_in_any_path_component( .file_system .read_file(&uri(&existing)?, no_follow_read, Some(&sandbox)) .await; - if is_unsupported_restricted_token_host(&read_result) { + assert_eq!(read_result?, b"unchanged"); + let write_result = context + .file_system + .write_file( + &uri(&existing)?, + b"unchanged".to_vec(), + no_follow_write, + Some(&sandbox), + ) + .await; + if is_unsupported_restricted_token_host(&write_result) { return Ok(()); } - assert_eq!(read_result?, b"unchanged"); + write_result?; assert!( context .file_system @@ -380,6 +390,17 @@ async fn file_system_remote_fs_helper_respects_windows_sandbox_write_policy( } } + let readable_file = readonly_dir.join("readable.txt"); + std::fs::write(&readable_file, b"readable")?; + let read_result = file_system + .read_file( + &PathUri::from_host_native_path(&readable_file)?, + ReadFileOptions::default(), + Some(&sandbox), + ) + .await; + assert_eq!(read_result?, b"readable"); + let blocked_file = readonly_dir.join("blocked.txt"); if sandbox_type == SandboxType::WindowsMxc && !codex_sandboxing::windows_mxc_available() { let error = file_system @@ -402,34 +423,23 @@ async fn file_system_remote_fs_helper_respects_windows_sandbox_write_policy( return Ok(()); } - let readable_file = readonly_dir.join("readable.txt"); - std::fs::write(&readable_file, b"readable")?; - let read_result = file_system - .read_file( - &PathUri::from_host_native_path(&readable_file)?, - ReadFileOptions::default(), - Some(&sandbox), - ) - .await; - // Some local Windows hosts cannot create restricted tokens. Reaching that - // error still proves the remote fs helper went through the Windows sandbox - // launcher; before the wrapper fix this read would have run unsandboxed. - if sandbox_type == SandboxType::WindowsRestrictedToken - && is_unsupported_restricted_token_host(&read_result) - { - return Ok(()); - } - assert_eq!(read_result?, b"readable"); - - let error = file_system + let write_result = file_system .write_file( &PathUri::from_host_native_path(&blocked_file)?, b"blocked".to_vec(), WriteFileOptions::default(), Some(&sandbox), ) - .await - .expect_err("write outside the sandbox should fail"); + .await; + // Some local Windows hosts cannot create restricted tokens. Reaching that + // error still proves the write went through the Windows sandbox launcher. + if sandbox_type == SandboxType::WindowsRestrictedToken + && is_unsupported_restricted_token_host(&write_result) + { + assert!(!blocked_file.exists()); + return Ok(()); + } + let error = write_result.expect_err("write outside the sandbox should fail"); assert!( !blocked_file.exists(), "sandboxed fs helper must not create blocked file after error: {error}" @@ -562,14 +572,19 @@ async fn file_system_private_desktop_survives_helper_exits_and_separates_permiss let mut sandbox = workspace_write_sandbox(tmp.path().to_path_buf()); sandbox.windows_sandbox_private_desktop = true; let before = process_private_desktops()?; - let read = file_system - .read_file(&uri, ReadFileOptions::default(), Some(&sandbox)) + let write = file_system + .write_file( + &uri, + b"initial".to_vec(), + WriteFileOptions::default(), + Some(&sandbox), + ) .await; - if is_unsupported_restricted_token_host(&read) { + if is_unsupported_restricted_token_host(&write) { eprintln!("Skipping private desktop reuse: this host cannot create restricted tokens"); return Ok(()); } - assert_eq!(read?, b"initial"); + write?; // Ownership must outlive each helper; a desktop held only by the helper disappears here. let warmed = process_private_desktops()?; @@ -610,15 +625,6 @@ async fn file_system_private_desktop_survives_helper_exits_and_separates_permiss let mut readonly = read_only_sandbox_for_cwd(tmp.path().to_path_buf())?; readonly.windows_sandbox_selection = WindowsSandboxSelection::RestrictedToken; readonly.windows_sandbox_private_desktop = true; - assert_eq!( - file_system - .read_file(&uri, ReadFileOptions::default(), Some(&readonly)) - .await?, - b"updated again" - ); - let separated = process_private_desktops()?; - assert!(warmed.is_subset(&separated)); - assert_eq!(separated.difference(&warmed).count(), 1); file_system .write_file( &uri, @@ -629,6 +635,15 @@ async fn file_system_private_desktop_survives_helper_exits_and_separates_permiss .await .expect_err("read-only filesystem requests must reject writes"); assert_eq!(std::fs::read(&path)?, b"updated again"); + let separated = process_private_desktops()?; + assert!(warmed.is_subset(&separated)); + assert_eq!(separated.difference(&warmed).count(), 1); + assert_eq!( + file_system + .read_file(&uri, ReadFileOptions::default(), Some(&readonly)) + .await?, + b"updated again" + ); assert_eq!(process_private_desktops()?, separated); Ok(()) } diff --git a/codex-rs/ext/skills/src/provider/executor.rs b/codex-rs/ext/skills/src/provider/executor.rs index c6717e74c9..4aeac8a467 100644 --- a/codex-rs/ext/skills/src/provider/executor.rs +++ b/codex-rs/ext/skills/src/provider/executor.rs @@ -333,7 +333,7 @@ async fn read_bounded_text( "failed to read executor skill resource {resource}: {err}" )) }; - if sandbox.is_some_and(FileSystemSandboxContext::should_run_in_sandbox) + if sandbox.is_some_and(FileSystemSandboxContext::should_read_from_sandbox) && path.infer_path_convention() == Some(PathConvention::Windows) && sandbox.is_some_and(|context| !context.windows_sandbox_is_requested()) { diff --git a/codex-rs/file-system/src/lib.rs b/codex-rs/file-system/src/lib.rs index 0abe9dba3d..b8ddeddce2 100644 --- a/codex-rs/file-system/src/lib.rs +++ b/codex-rs/file-system/src/lib.rs @@ -11,7 +11,6 @@ use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSandboxEntryMissingPathBehavior; -use codex_protocol::permissions::FileSystemSandboxKind; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::FileSystemSandboxPolicyContext; use codex_protocol::permissions::FileSystemSpecialPath; @@ -401,14 +400,20 @@ impl FileSystemSandboxContext { } } - pub fn should_run_in_sandbox(&self) -> bool { - if self.validate_file_system_paths_for_current_host().is_err() { - // A sandbox context for another host must not select the unsandboxed filesystem. - return true; - } - let file_system_policy = self.permissions.file_system_sandbox_policy(); - matches!(file_system_policy.kind, FileSystemSandboxKind::Restricted) - && !file_system_policy.has_full_disk_write_access() + /// Whether filesystem reads need a platform sandbox on the selected executor. + pub fn should_read_from_sandbox(&self) -> bool { + !self + .permissions + .file_system_sandbox_policy() + .has_full_disk_read_access_for_convention(self.cwd.infer_path_convention()) + } + + /// Whether filesystem writes need a platform sandbox on the selected executor. + pub fn should_write_into_sandbox(&self) -> bool { + !self + .permissions + .file_system_sandbox_policy() + .has_full_disk_write_access_for_convention(self.cwd.infer_path_convention()) } /// Whether this context selects either supported Windows sandbox implementation. diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index 4e8e469a49..c7e802dd08 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -674,6 +674,14 @@ impl FileSystemSandboxPolicy { } pub fn has_denied_read_restrictions(&self) -> bool { + // TODO(anp) Migrate callers to select the executor's path convention explicitly. + self.has_denied_read_restrictions_for_convention(Some(PathConvention::native())) + } + + fn has_denied_read_restrictions_for_convention( + &self, + convention: Option, + ) -> bool { matches!(self.kind, FileSystemSandboxKind::Restricted) && self.entries.iter().any(|entry| { entry.access == FileSystemAccessMode::Deny @@ -681,7 +689,7 @@ impl FileSystemSandboxPolicy { &entry.path, FileSystemPath::Special { value: FileSystemSpecialPath::SlashTmp, - } if !cfg!(unix) + } if convention == Some(PathConvention::Windows) ) }) } @@ -888,19 +896,30 @@ impl FileSystemSandboxPolicy { file_system_policy } - /// Returns true when filesystem reads are unrestricted. + /// Returns true when filesystem reads are unrestricted on this host. pub fn has_full_disk_read_access(&self) -> bool { + // TODO(anp) Migrate callers to select the executor's path convention explicitly. + self.has_full_disk_read_access_for_convention(Some(PathConvention::native())) + } + + /// Returns true when filesystem reads are unrestricted on the selected executor. + /// If the convention is unknown, a `:slash_tmp` denial is still treated as a restriction. + pub fn has_full_disk_read_access_for_convention( + &self, + convention: Option, + ) -> bool { match self.kind { FileSystemSandboxKind::Unrestricted | FileSystemSandboxKind::ExternalSandbox => true, FileSystemSandboxKind::Restricted => { self.has_root_access(FileSystemAccessMode::can_read) - && !self.has_denied_read_restrictions() + && !self.has_denied_read_restrictions_for_convention(convention) } } } /// Returns true when filesystem writes are unrestricted on this host. pub fn has_full_disk_write_access(&self) -> bool { + // TODO(anp) Migrate callers to select the executor's path convention explicitly. self.has_full_disk_write_access_for_convention(Some(PathConvention::native())) } @@ -912,7 +931,8 @@ impl FileSystemSandboxPolicy { self.has_full_disk_write_access_for_convention(context.cwd.infer_path_convention()) } - fn has_full_disk_write_access_for_convention( + /// Returns true when filesystem writes are unrestricted for the selected path convention. + pub fn has_full_disk_write_access_for_convention( &self, convention: Option, ) -> bool { diff --git a/codex-rs/protocol/src/permissions/target_tests.rs b/codex-rs/protocol/src/permissions/target_tests.rs index bcca625071..5842440e22 100644 --- a/codex-rs/protocol/src/permissions/target_tests.rs +++ b/codex-rs/protocol/src/permissions/target_tests.rs @@ -231,6 +231,49 @@ fn slash_tmp_denials_follow_the_executor_convention() { } } +/// Full-disk read policy shares the native check and ignores `:slash_tmp` only on Windows. +#[test] +fn full_disk_read_policy_uses_the_executor_convention() { + use FileSystemAccessMode::Read; + use FileSystemAccessMode::Write; + use FileSystemSpecialPath::Minimal; + use FileSystemSpecialPath::Root; + use FileSystemSpecialPath::SlashTmp; + use FileSystemSpecialPath::Tmpdir; + + for (root_access, denial, posix, windows, unknown) in [ + (Read, None, true, true, true), + (Read, Some(SlashTmp), false, true, false), + (Read, Some(Tmpdir), false, false, false), + (Write, Some(Minimal), false, false, false), + (Write, Some(Root), false, false, false), + ] { + let mut entries = vec![FileSystemSandboxEntry::new( + FileSystemPath::Special { value: Root }, + root_access, + )]; + if let Some(value) = denial { + entries.push(deny(FileSystemPath::Special { value })); + } + let policy = FileSystemSandboxPolicy::restricted(entries); + for (convention, expected) in [ + (Some(PathConvention::Posix), posix), + (Some(PathConvention::Windows), windows), + (None, unknown), + ] { + assert_eq!( + policy.has_full_disk_read_access_for_convention(convention), + expected, + "policy={policy:?}, convention={convention:?}", + ); + } + assert_eq!( + policy.has_full_disk_read_access(), + policy.has_full_disk_read_access_for_convention(Some(PathConvention::native())), + ); + } +} + #[test] fn remote_workspace_write_keeps_metadata_protected() { let cwd = uri("file:///C:/repo");