diff --git a/codex-rs/core/tests/suite/remote_env.rs b/codex-rs/core/tests/suite/remote_env.rs index 36c9b35c30f..1e781a669b4 100644 --- a/codex-rs/core/tests/suite/remote_env.rs +++ b/codex-rs/core/tests/suite/remote_env.rs @@ -59,26 +59,38 @@ fn absolute_path(path: PathBuf) -> AbsolutePathBuf { } fn read_only_sandbox(readable_root: PathBuf) -> FileSystemSandboxContext { - FileSystemSandboxContext::new(SandboxPolicy::ReadOnly { - access: ReadOnlyAccess::Restricted { - include_platform_defaults: false, - readable_roots: vec![absolute_path(readable_root)], + let readable_root = absolute_path(readable_root); + // The policy is evaluated in the remote container, so use a container path + // for cwd instead of capturing the local test runner cwd. + FileSystemSandboxContext::from_legacy_sandbox_policy( + SandboxPolicy::ReadOnly { + access: ReadOnlyAccess::Restricted { + include_platform_defaults: false, + readable_roots: vec![readable_root.clone()], + }, + network_access: false, }, - network_access: false, - }) + readable_root, + ) } fn workspace_write_sandbox(writable_root: PathBuf) -> FileSystemSandboxContext { - FileSystemSandboxContext::new(SandboxPolicy::WorkspaceWrite { - writable_roots: vec![absolute_path(writable_root)], - read_only_access: ReadOnlyAccess::Restricted { - include_platform_defaults: false, - readable_roots: vec![], + let writable_root = absolute_path(writable_root); + // The policy is evaluated in the remote container, so use a container path + // for cwd instead of capturing the local test runner cwd. + FileSystemSandboxContext::from_legacy_sandbox_policy( + SandboxPolicy::WorkspaceWrite { + writable_roots: vec![writable_root.clone()], + read_only_access: ReadOnlyAccess::Restricted { + include_platform_defaults: false, + readable_roots: vec![], + }, + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, }, - network_access: false, - exclude_tmpdir_env_var: true, - exclude_slash_tmp: true, - }) + writable_root, + ) } fn assert_normalized_path_rejected(error: &std::io::Error) { diff --git a/codex-rs/exec-server/src/file_system.rs b/codex-rs/exec-server/src/file_system.rs index 77676548ffb..001c500f1b8 100644 --- a/codex-rs/exec-server/src/file_system.rs +++ b/codex-rs/exec-server/src/file_system.rs @@ -1,11 +1,14 @@ use async_trait::async_trait; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxKind; use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::FileSystemSpecialPath; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; +use std::path::Path; use tokio::io; #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -103,6 +106,31 @@ impl FileSystemSandboxContext { matches!(file_system_policy.kind, FileSystemSandboxKind::Restricted) && !file_system_policy.has_full_disk_write_access() } + + pub(crate) fn drop_cwd_if_unused(mut self) -> Self { + let file_system_policy = self.permissions.file_system_sandbox_policy(); + if !file_system_policy_has_cwd_dependent_entries(&file_system_policy) { + self.cwd = None; + } + self + } +} + +pub(crate) fn file_system_policy_has_cwd_dependent_entries( + file_system_policy: &FileSystemSandboxPolicy, +) -> bool { + file_system_policy + .entries + .iter() + .any(|entry| match &entry.path { + FileSystemPath::GlobPattern { pattern } => !Path::new(pattern).is_absolute(), + FileSystemPath::Special { + value: + FileSystemSpecialPath::CurrentWorkingDirectory + | FileSystemSpecialPath::ProjectRoots { .. }, + } => true, + FileSystemPath::Path { .. } | FileSystemPath::Special { .. } => false, + }) } pub type FileSystemResult = io::Result; diff --git a/codex-rs/exec-server/src/fs_sandbox.rs b/codex-rs/exec-server/src/fs_sandbox.rs index 5f347e6e1c4..d8261fae145 100644 --- a/codex-rs/exec-server/src/fs_sandbox.rs +++ b/codex-rs/exec-server/src/fs_sandbox.rs @@ -21,6 +21,7 @@ use tokio::process::Command; use crate::ExecServerRuntimePaths; use crate::FileSystemSandboxContext; +use crate::file_system::file_system_policy_has_cwd_dependent_entries; use crate::fs_helper::CODEX_FS_HELPER_ARG1; use crate::fs_helper::FsHelperPayload; use crate::fs_helper::FsHelperRequest; @@ -52,12 +53,12 @@ impl FileSystemSandboxRunner { ) -> Result { let cwd = sandbox_cwd(sandbox)?; let mut file_system_policy = sandbox.permissions.file_system_sandbox_policy(); - let helper_read_root = if sandbox.use_legacy_landlock { - None + let helper_read_roots = if sandbox.use_legacy_landlock { + Vec::new() } else { - helper_read_root(&self.runtime_paths) + helper_read_roots(&self.runtime_paths) }; - add_helper_runtime_permissions(&mut file_system_policy, helper_read_root, cwd.as_path()); + add_helper_runtime_permissions(&mut file_system_policy, &helper_read_roots, cwd.as_path()); normalize_file_system_policy_root_aliases(&mut file_system_policy); let network_policy = NetworkSandboxPolicy::Restricted; let sandbox_policy = @@ -133,33 +134,24 @@ fn sandbox_cwd(sandbox: &FileSystemSandboxContext) -> Result bool { - file_system_policy - .entries - .iter() - .any(|entry| match &entry.path { - FileSystemPath::GlobPattern { pattern } => !std::path::Path::new(pattern).is_absolute(), - FileSystemPath::Special { - value: - FileSystemSpecialPath::CurrentWorkingDirectory - | FileSystemSpecialPath::ProjectRoots { .. }, - } => true, - FileSystemPath::Path { .. } | FileSystemPath::Special { .. } => false, - }) -} - -fn helper_read_root(runtime_paths: &ExecServerRuntimePaths) -> Option { - runtime_paths - .codex_self_exe - .parent() - .and_then(|path| AbsolutePathBuf::from_absolute_path(path).ok()) +fn helper_read_roots(runtime_paths: &ExecServerRuntimePaths) -> Vec { + let mut roots = Vec::new(); + for path in std::iter::once(runtime_paths.codex_self_exe.as_path()) + .chain(runtime_paths.codex_linux_sandbox_exe.as_deref().into_iter()) + { + if let Some(parent) = path.parent() + && let Ok(root) = AbsolutePathBuf::from_absolute_path(parent) + && !roots.contains(&root) + { + roots.push(root); + } + } + roots } fn add_helper_runtime_permissions( file_system_policy: &mut FileSystemSandboxPolicy, - helper_read_root: Option, + helper_read_roots: &[AbsolutePathBuf], cwd: &std::path::Path, ) { if !file_system_policy.has_full_disk_read_access() { @@ -174,19 +166,18 @@ fn add_helper_runtime_permissions( } } - let Some(helper_read_root) = helper_read_root else { - return; - }; - if file_system_policy.can_read_path_with_cwd(helper_read_root.as_path(), cwd) { - return; - } + for helper_read_root in helper_read_roots { + if file_system_policy.can_read_path_with_cwd(helper_read_root.as_path(), cwd) { + continue; + } - file_system_policy.entries.push(FileSystemSandboxEntry { - path: FileSystemPath::Path { - path: helper_read_root, - }, - access: FileSystemAccessMode::Read, - }); + file_system_policy.entries.push(FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: helper_read_root.clone(), + }, + access: FileSystemAccessMode::Read, + }); + } } fn compatibility_sandbox_policy( @@ -371,7 +362,7 @@ mod tests { use super::helper_env; use super::helper_env_from_vars; use super::helper_env_key_is_allowed; - use super::helper_read_root; + use super::helper_read_roots; use super::sandbox_cwd; #[test] @@ -388,7 +379,7 @@ mod tests { let mut policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, cwd.as_path()); - add_helper_runtime_permissions(&mut policy, /*helper_read_root*/ None, cwd.as_path()); + add_helper_runtime_permissions(&mut policy, /*helper_read_roots*/ &[], cwd.as_path()); assert!(policy.include_platform_defaults()); } @@ -410,7 +401,7 @@ mod tests { let mut policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, cwd.as_path()); - add_helper_runtime_permissions(&mut policy, /*helper_read_root*/ None, cwd.as_path()); + add_helper_runtime_permissions(&mut policy, /*helper_read_roots*/ &[], cwd.as_path()); assert!(policy.include_platform_defaults()); } @@ -449,7 +440,7 @@ mod tests { add_helper_runtime_permissions( &mut policy, - helper_read_root(&runtime_paths), + &helper_read_roots(&runtime_paths), cwd.as_path(), ); @@ -611,10 +602,44 @@ mod tests { add_helper_runtime_permissions( &mut policy, - helper_read_root(&runtime_paths), + &helper_read_roots(&runtime_paths), cwd.as_path(), ); assert!(policy.can_read_path_with_cwd(readable.as_path(), cwd.as_path())); } + + #[test] + fn helper_permissions_include_linux_sandbox_alias_parent() { + let root = tempfile::tempdir().expect("temp dir"); + let codex_self_exe = root.path().join("bin").join("codex"); + let codex_linux_sandbox_exe = root.path().join("aliases").join("codex-linux-sandbox"); + let runtime_paths = + ExecServerRuntimePaths::new(codex_self_exe, Some(codex_linux_sandbox_exe)) + .expect("runtime paths"); + let cwd = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path()) + .expect("absolute cwd"); + let sandbox_policy = SandboxPolicy::ReadOnly { + access: ReadOnlyAccess::Restricted { + include_platform_defaults: false, + readable_roots: Vec::new(), + }, + network_access: false, + }; + let mut policy = + FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, cwd.as_path()); + let codex_parent = AbsolutePathBuf::from_absolute_path(root.path().join("bin")) + .expect("absolute codex parent"); + let alias_parent = AbsolutePathBuf::from_absolute_path(root.path().join("aliases")) + .expect("absolute alias parent"); + + add_helper_runtime_permissions( + &mut policy, + &helper_read_roots(&runtime_paths), + cwd.as_path(), + ); + + assert!(policy.can_read_path_with_cwd(codex_parent.as_path(), cwd.as_path())); + assert!(policy.can_read_path_with_cwd(alias_parent.as_path(), cwd.as_path())); + } } diff --git a/codex-rs/exec-server/src/remote_file_system.rs b/codex-rs/exec-server/src/remote_file_system.rs index dc269505a1d..fc5cdb01d21 100644 --- a/codex-rs/exec-server/src/remote_file_system.rs +++ b/codex-rs/exec-server/src/remote_file_system.rs @@ -50,7 +50,7 @@ impl ExecutorFileSystem for RemoteFileSystem { let response = client .fs_read_file(FsReadFileParams { path: path.clone(), - sandbox: sandbox.cloned(), + sandbox: remote_sandbox_context(sandbox), }) .await .map_err(map_remote_error)?; @@ -74,7 +74,7 @@ impl ExecutorFileSystem for RemoteFileSystem { .fs_write_file(FsWriteFileParams { path: path.clone(), data_base64: STANDARD.encode(contents), - sandbox: sandbox.cloned(), + sandbox: remote_sandbox_context(sandbox), }) .await .map_err(map_remote_error)?; @@ -93,7 +93,7 @@ impl ExecutorFileSystem for RemoteFileSystem { .fs_create_directory(FsCreateDirectoryParams { path: path.clone(), recursive: Some(options.recursive), - sandbox: sandbox.cloned(), + sandbox: remote_sandbox_context(sandbox), }) .await .map_err(map_remote_error)?; @@ -110,7 +110,7 @@ impl ExecutorFileSystem for RemoteFileSystem { let response = client .fs_get_metadata(FsGetMetadataParams { path: path.clone(), - sandbox: sandbox.cloned(), + sandbox: remote_sandbox_context(sandbox), }) .await .map_err(map_remote_error)?; @@ -133,7 +133,7 @@ impl ExecutorFileSystem for RemoteFileSystem { let response = client .fs_read_directory(FsReadDirectoryParams { path: path.clone(), - sandbox: sandbox.cloned(), + sandbox: remote_sandbox_context(sandbox), }) .await .map_err(map_remote_error)?; @@ -161,7 +161,7 @@ impl ExecutorFileSystem for RemoteFileSystem { path: path.clone(), recursive: Some(options.recursive), force: Some(options.force), - sandbox: sandbox.cloned(), + sandbox: remote_sandbox_context(sandbox), }) .await .map_err(map_remote_error)?; @@ -182,7 +182,7 @@ impl ExecutorFileSystem for RemoteFileSystem { source_path: source_path.clone(), destination_path: destination_path.clone(), recursive: options.recursive, - sandbox: sandbox.cloned(), + sandbox: remote_sandbox_context(sandbox), }) .await .map_err(map_remote_error)?; @@ -190,6 +190,14 @@ impl ExecutorFileSystem for RemoteFileSystem { } } +fn remote_sandbox_context( + sandbox: Option<&FileSystemSandboxContext>, +) -> Option { + sandbox + .cloned() + .map(FileSystemSandboxContext::drop_cwd_if_unused) +} + fn map_remote_error(error: ExecServerError) -> io::Error { match error { ExecServerError::Server { code, message } if code == NOT_FOUND_ERROR_CODE => { @@ -208,10 +216,58 @@ fn map_remote_error(error: ExecServerError) -> io::Error { #[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 pretty_assertions::assert_eq; use super::*; + #[test] + fn remote_sandbox_context_drops_unused_cwd() { + let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: absolute_test_path("remote-root"), + }, + access: FileSystemAccessMode::Read, + }]); + let permissions = + PermissionProfile::from_runtime_permissions(&policy, NetworkSandboxPolicy::Restricted); + let sandbox_context = FileSystemSandboxContext::from_permission_profile_with_cwd( + permissions, + absolute_test_path("host-checkout"), + ); + + let remote_context = + remote_sandbox_context(Some(&sandbox_context)).expect("remote sandbox context"); + + assert_eq!(remote_context.cwd, None); + } + + #[test] + fn remote_sandbox_context_preserves_required_cwd() { + let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: FileSystemAccessMode::Write, + }]); + let permissions = + PermissionProfile::from_runtime_permissions(&policy, NetworkSandboxPolicy::Restricted); + let cwd = absolute_test_path("host-checkout"); + let sandbox_context = + FileSystemSandboxContext::from_permission_profile_with_cwd(permissions, cwd.clone()); + + let remote_context = + remote_sandbox_context(Some(&sandbox_context)).expect("remote sandbox context"); + + assert_eq!(remote_context.cwd, Some(cwd)); + } + #[test] fn transport_errors_map_to_broken_pipe() { let errors = [ @@ -241,4 +297,9 @@ 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") + } } diff --git a/codex-rs/exec-server/src/server/handler/tests.rs b/codex-rs/exec-server/src/server/handler/tests.rs index 9d1e4c470cf..e4c743fa2bb 100644 --- a/codex-rs/exec-server/src/server/handler/tests.rs +++ b/codex-rs/exec-server/src/server/handler/tests.rs @@ -317,7 +317,7 @@ async fn read_process_until_closed( handler: &ExecServerHandler, process_id: ProcessId, ) -> (String, Option) { - let deadline = tokio::time::Instant::now() + Duration::from_secs(2); + let deadline = tokio::time::Instant::now() + Duration::from_secs(5); let mut output = String::new(); let mut exit_code = None; let mut after_seq = None; @@ -346,7 +346,7 @@ async fn read_process_until_closed( after_seq = response.next_seq.checked_sub(1).or(after_seq); assert!( tokio::time::Instant::now() < deadline, - "process should close within 2s" + "process should close within 5s" ); } }