From 837e613195349e139c6062ba5bf0a147d30175bf Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Thu, 16 Apr 2026 23:39:48 -0700 Subject: [PATCH 1/3] fix: keep fs sandbox helper compatible with apply_patch Co-authored-by: Codex noreply@openai.com --- codex-rs/core/src/codex.rs | 29 +- .../core/src/tools/runtimes/apply_patch.rs | 11 + .../src/tools/runtimes/apply_patch_tests.rs | 56 +++- codex-rs/exec-server/src/file_system.rs | 7 + codex-rs/exec-server/src/fs_sandbox.rs | 281 +++++++----------- codex-rs/exec-server/src/fs_sandbox_tests.rs | 224 ++++++++++++++ 6 files changed, 416 insertions(+), 192 deletions(-) create mode 100644 codex-rs/exec-server/src/fs_sandbox_tests.rs diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 3d50f7735e27..ee644d9abbdb 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1013,6 +1013,8 @@ impl TurnContext { ) -> FileSystemSandboxContext { FileSystemSandboxContext { sandbox_policy: self.sandbox_policy.get().clone(), + sandbox_policy_cwd: Some(self.cwd.clone()), + file_system_sandbox_policy: self.non_legacy_file_system_sandbox_policy(), windows_sandbox_level: self.windows_sandbox_level, windows_sandbox_private_desktop: self .config @@ -1023,6 +1025,19 @@ impl TurnContext { } } + fn non_legacy_file_system_sandbox_policy(&self) -> Option { + // Omit the derived split filesystem policy when it is equivalent to + // the legacy sandbox policy. This keeps turn-context payloads stable + // while both fields exist; once callers consume only the split policy, + // this comparison and the legacy projection should go away. + let legacy_file_system_sandbox_policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy( + self.sandbox_policy.get(), + &self.cwd, + ); + (self.file_system_sandbox_policy != legacy_file_system_sandbox_policy) + .then(|| self.file_system_sandbox_policy.clone()) + } + pub(crate) fn compact_prompt(&self) -> &str { self.compact_prompt .as_deref() @@ -1030,18 +1045,6 @@ impl TurnContext { } pub(crate) fn to_turn_context_item(&self) -> TurnContextItem { - let legacy_file_system_sandbox_policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy( - self.sandbox_policy.get(), - &self.cwd, - ); - // Omit the derived split filesystem policy when it is equivalent to - // the legacy sandbox policy. This keeps turn-context payloads stable - // while both fields exist; once callers consume only the split policy, - // this comparison and the legacy projection should go away. - let file_system_sandbox_policy = (self.file_system_sandbox_policy - != legacy_file_system_sandbox_policy) - .then(|| self.file_system_sandbox_policy.clone()); - TurnContextItem { turn_id: Some(self.sub_id.clone()), trace_id: self.trace_id.clone(), @@ -1051,7 +1054,7 @@ impl TurnContext { approval_policy: self.approval_policy.value(), sandbox_policy: self.sandbox_policy.get().clone(), network: self.turn_context_network_item(), - file_system_sandbox_policy, + file_system_sandbox_policy: self.non_legacy_file_system_sandbox_policy(), model: self.model_info.slug.clone(), personality: self.personality, collaboration_mode: Some(self.collaboration_mode.clone()), diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 1d5c8a1e36a1..aac8e6bc1340 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -22,6 +22,7 @@ use codex_protocol::error::SandboxErr; use codex_protocol::exec_output::ExecToolCallOutput; use codex_protocol::exec_output::StreamOutput; use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; @@ -74,8 +75,18 @@ impl ApplyPatchRuntime { return None; } + let legacy_file_system_sandbox_policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy( + attempt.policy, + attempt.sandbox_cwd, + ); + let file_system_sandbox_policy = (attempt.file_system_policy + != &legacy_file_system_sandbox_policy) + .then(|| attempt.file_system_policy.clone()); + Some(FileSystemSandboxContext { sandbox_policy: attempt.policy.clone(), + sandbox_policy_cwd: Some(attempt.sandbox_cwd.clone()), + file_system_sandbox_policy, windows_sandbox_level: attempt.windows_sandbox_level, windows_sandbox_private_desktop: attempt.windows_sandbox_private_desktop, use_legacy_landlock: attempt.use_legacy_landlock, diff --git a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs index 0ba3e131af3d..0511ca91b6f4 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs @@ -3,6 +3,9 @@ use crate::tools::sandboxing::SandboxAttempt; use codex_protocol::config_types::WindowsSandboxLevel; 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::NetworkSandboxPolicy; use codex_protocol::protocol::GranularApprovalConfig; @@ -99,7 +102,12 @@ fn file_system_sandbox_context_uses_active_attempt() { permissions_preapproved: false, }; let sandbox_policy = SandboxPolicy::new_read_only_policy(); - let file_system_policy = FileSystemSandboxPolicy::from(&sandbox_policy); + let mut file_system_policy = + FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, path.as_path()); + file_system_policy.entries.push(FileSystemSandboxEntry { + path: FileSystemPath::Path { path: path.clone() }, + access: FileSystemAccessMode::None, + }); let manager = SandboxManager::new(); let attempt = SandboxAttempt { sandbox: SandboxType::MacosSeatbelt, @@ -119,6 +127,11 @@ fn file_system_sandbox_context_uses_active_attempt() { .expect("sandbox context"); assert_eq!(sandbox.sandbox_policy, sandbox_policy); + assert_eq!(sandbox.sandbox_policy_cwd, Some(path.clone())); + assert_eq!( + sandbox.file_system_sandbox_policy, + Some(file_system_policy.clone()) + ); assert_eq!(sandbox.additional_permissions, Some(additional_permissions)); assert_eq!( sandbox.windows_sandbox_level, @@ -128,6 +141,47 @@ fn file_system_sandbox_context_uses_active_attempt() { assert_eq!(sandbox.use_legacy_landlock, true); } +#[test] +fn file_system_sandbox_context_omits_legacy_equivalent_policy() { + let path = std::env::temp_dir() + .join("apply-patch-runtime-legacy-equivalent.txt") + .abs(); + let req = ApplyPatchRequest { + action: ApplyPatchAction::new_add_for_test(&path, "hello".to_string()), + file_paths: vec![path.clone()], + changes: HashMap::new(), + exec_approval_requirement: ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + }, + additional_permissions: None, + permissions_preapproved: false, + }; + let sandbox_policy = SandboxPolicy::new_read_only_policy(); + let file_system_policy = + FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, path.as_path()); + let manager = SandboxManager::new(); + let attempt = SandboxAttempt { + sandbox: SandboxType::MacosSeatbelt, + policy: &sandbox_policy, + file_system_policy: &file_system_policy, + network_policy: NetworkSandboxPolicy::Restricted, + enforce_managed_network: false, + manager: &manager, + sandbox_cwd: &path, + codex_linux_sandbox_exe: None, + use_legacy_landlock: true, + windows_sandbox_level: WindowsSandboxLevel::RestrictedToken, + windows_sandbox_private_desktop: true, + }; + + let sandbox = ApplyPatchRuntime::file_system_sandbox_context_for_attempt(&req, &attempt) + .expect("sandbox context"); + + assert_eq!(sandbox.sandbox_policy_cwd, Some(path)); + assert_eq!(sandbox.file_system_sandbox_policy, None); +} + #[test] fn no_sandbox_attempt_has_no_file_system_context() { let path = std::env::temp_dir() diff --git a/codex-rs/exec-server/src/file_system.rs b/codex-rs/exec-server/src/file_system.rs index b09347686aed..64ebc891db46 100644 --- a/codex-rs/exec-server/src/file_system.rs +++ b/codex-rs/exec-server/src/file_system.rs @@ -1,6 +1,7 @@ use async_trait::async_trait; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; use tokio::io; @@ -41,6 +42,10 @@ pub struct ReadDirectoryEntry { #[serde(rename_all = "camelCase")] pub struct FileSystemSandboxContext { pub sandbox_policy: SandboxPolicy, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub sandbox_policy_cwd: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub file_system_sandbox_policy: Option, pub windows_sandbox_level: WindowsSandboxLevel, #[serde(default)] pub windows_sandbox_private_desktop: bool, @@ -53,6 +58,8 @@ impl FileSystemSandboxContext { pub fn new(sandbox_policy: SandboxPolicy) -> Self { Self { sandbox_policy, + sandbox_policy_cwd: None, + file_system_sandbox_policy: None, windows_sandbox_level: WindowsSandboxLevel::Disabled, windows_sandbox_private_desktop: false, use_legacy_landlock: false, diff --git a/codex-rs/exec-server/src/fs_sandbox.rs b/codex-rs/exec-server/src/fs_sandbox.rs index 7b0ac256d4db..3fdf35520d86 100644 --- a/codex-rs/exec-server/src/fs_sandbox.rs +++ b/codex-rs/exec-server/src/fs_sandbox.rs @@ -12,6 +12,7 @@ use codex_sandboxing::SandboxExecRequest; use codex_sandboxing::SandboxManager; use codex_sandboxing::SandboxTransformRequest; use codex_sandboxing::SandboxablePreference; +use codex_sandboxing::policy_transforms::merge_permission_profiles; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::canonicalize_preserving_symlinks; use tokio::io::AsyncWriteExt; @@ -27,11 +28,20 @@ use crate::local_file_system::current_sandbox_cwd; use crate::rpc::internal_error; use crate::rpc::invalid_request; +const PATH_ENV_VAR: &str = "PATH"; + #[derive(Clone, Debug)] pub(crate) struct FileSystemSandboxRunner { runtime_paths: ExecServerRuntimePaths, } +struct HelperSandboxInputs { + sandbox_policy: SandboxPolicy, + file_system_policy: FileSystemSandboxPolicy, + network_policy: NetworkSandboxPolicy, + cwd: AbsolutePathBuf, +} + impl FileSystemSandboxRunner { pub(crate) fn new(runtime_paths: ExecServerRuntimePaths) -> Self { Self { runtime_paths } @@ -42,19 +52,14 @@ impl FileSystemSandboxRunner { sandbox: &FileSystemSandboxContext, request: FsHelperRequest, ) -> Result { - let helper_sandbox_policy = normalize_sandbox_policy_root_aliases( - sandbox_policy_with_helper_runtime_defaults(&sandbox.sandbox_policy), - ); - let cwd = current_sandbox_cwd().map_err(io_error)?; - let cwd = AbsolutePathBuf::from_absolute_path(cwd.as_path()) - .map_err(|err| invalid_request(format!("current directory is not absolute: {err}")))?; - let file_system_policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy( - &helper_sandbox_policy, - cwd.as_path(), - ); - let network_policy = NetworkSandboxPolicy::Restricted; + let HelperSandboxInputs { + sandbox_policy, + file_system_policy, + network_policy, + cwd, + } = helper_sandbox_inputs(sandbox)?; let command = self.sandbox_exec_request( - &helper_sandbox_policy, + &sandbox_policy, &file_system_policy, network_policy, &cwd, @@ -85,9 +90,10 @@ impl FileSystemSandboxRunner { program: helper.as_path().as_os_str().to_owned(), args: vec![CODEX_FS_HELPER_ARG1.to_string()], cwd: cwd.clone(), - env: HashMap::new(), - additional_permissions: Some( - self.helper_permissions(sandbox_context.additional_permissions.as_ref()), + env: helper_env(), + additional_permissions: self.helper_permissions( + sandbox_context.additional_permissions.as_ref(), + /*include_helper_read_root*/ !sandbox_context.use_legacy_landlock, ), }; sandbox_manager @@ -111,36 +117,79 @@ impl FileSystemSandboxRunner { fn helper_permissions( &self, additional_permissions: Option<&PermissionProfile>, - ) -> PermissionProfile { - let helper_read_root = self - .runtime_paths - .codex_self_exe - .parent() - .and_then(|path| AbsolutePathBuf::from_absolute_path(path).ok()); - let file_system = - match additional_permissions.and_then(|permissions| permissions.file_system.clone()) { - Some(mut file_system) => { - if let Some(helper_read_root) = &helper_read_root { - let read_paths = file_system.read.get_or_insert_with(Vec::new); - if !read_paths.contains(helper_read_root) { - read_paths.push(helper_read_root.clone()); - } - } - Some(file_system) - } - None => helper_read_root.map(|helper_read_root| FileSystemPermissions { + include_helper_read_root: bool, + ) -> Option { + let inherited_permissions = additional_permissions + .map(|permissions| PermissionProfile { + network: None, + file_system: permissions.file_system.clone(), + }) + .filter(|permissions| !permissions.is_empty()); + let helper_permissions = include_helper_read_root + .then(|| { + self.runtime_paths + .codex_self_exe + .parent() + .and_then(|path| AbsolutePathBuf::from_absolute_path(path).ok()) + }) + .flatten() + .map(|helper_read_root| PermissionProfile { + network: None, + file_system: Some(FileSystemPermissions { read: Some(vec![helper_read_root]), write: None, }), - }; + }); - PermissionProfile { - network: None, - file_system, - } + merge_permission_profiles(inherited_permissions.as_ref(), helper_permissions.as_ref()) } } +fn helper_sandbox_inputs( + sandbox: &FileSystemSandboxContext, +) -> Result { + let sandbox_policy = normalize_sandbox_policy_root_aliases( + sandbox_policy_with_helper_runtime_defaults(&sandbox.sandbox_policy), + ); + let cwd = match &sandbox.sandbox_policy_cwd { + Some(cwd) => cwd.clone(), + None if sandbox.file_system_sandbox_policy.is_some() => { + return Err(invalid_request( + "fileSystemSandboxPolicy requires sandboxPolicyCwd".to_string(), + )); + } + None => { + let cwd = current_sandbox_cwd().map_err(io_error)?; + AbsolutePathBuf::from_absolute_path(cwd.as_path()).map_err(|err| { + invalid_request(format!("current directory is not absolute: {err}")) + })? + } + }; + let file_system_policy = sandbox + .file_system_sandbox_policy + .clone() + .unwrap_or_else(|| { + FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, cwd.as_path()) + }); + Ok(HelperSandboxInputs { + sandbox_policy, + file_system_policy, + network_policy: NetworkSandboxPolicy::Restricted, + cwd, + }) +} + +fn helper_env() -> HashMap { + std::env::var_os(PATH_ENV_VAR) + .map(|path| { + HashMap::from([( + PATH_ENV_VAR.to_string(), + path.to_string_lossy().into_owned(), + )]) + }) + .unwrap_or_default() +} + fn normalize_sandbox_policy_root_aliases(sandbox_policy: SandboxPolicy) -> SandboxPolicy { let mut sandbox_policy = sandbox_policy; match &mut sandbox_policy { @@ -255,10 +304,21 @@ fn spawn_command( fn sandbox_policy_with_helper_runtime_defaults(sandbox_policy: &SandboxPolicy) -> SandboxPolicy { let mut sandbox_policy = sandbox_policy.clone(); match &mut sandbox_policy { - SandboxPolicy::ReadOnly { access, .. } => enable_platform_defaults(access), + SandboxPolicy::ReadOnly { + access, + network_access, + } => { + enable_platform_defaults(access); + *network_access = false; + } SandboxPolicy::WorkspaceWrite { - read_only_access, .. - } => enable_platform_defaults(read_only_access), + read_only_access, + network_access, + .. + } => { + enable_platform_defaults(read_only_access); + *network_access = false; + } SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => {} } sandbox_policy @@ -285,140 +345,5 @@ fn json_error(err: serde_json::Error) -> JSONRPCErrorError { } #[cfg(test)] -mod tests { - use codex_protocol::models::FileSystemPermissions; - use codex_protocol::models::NetworkPermissions; - use codex_protocol::models::PermissionProfile; - use codex_protocol::protocol::ReadOnlyAccess; - use codex_protocol::protocol::SandboxPolicy; - use codex_utils_absolute_path::AbsolutePathBuf; - use pretty_assertions::assert_eq; - - use crate::ExecServerRuntimePaths; - - use super::FileSystemSandboxRunner; - use super::sandbox_policy_with_helper_runtime_defaults; - - #[test] - fn helper_sandbox_policy_enables_platform_defaults_for_read_only_access() { - let sandbox_policy = SandboxPolicy::ReadOnly { - access: ReadOnlyAccess::Restricted { - include_platform_defaults: false, - readable_roots: Vec::new(), - }, - network_access: false, - }; - - let updated = sandbox_policy_with_helper_runtime_defaults(&sandbox_policy); - - assert_eq!( - updated, - SandboxPolicy::ReadOnly { - access: ReadOnlyAccess::Restricted { - include_platform_defaults: true, - readable_roots: Vec::new(), - }, - network_access: false, - } - ); - } - - #[test] - fn helper_sandbox_policy_enables_platform_defaults_for_workspace_read_access() { - let sandbox_policy = SandboxPolicy::WorkspaceWrite { - writable_roots: Vec::new(), - read_only_access: ReadOnlyAccess::Restricted { - include_platform_defaults: false, - readable_roots: Vec::new(), - }, - network_access: false, - exclude_tmpdir_env_var: true, - exclude_slash_tmp: true, - }; - - let updated = sandbox_policy_with_helper_runtime_defaults(&sandbox_policy); - - assert_eq!( - updated, - SandboxPolicy::WorkspaceWrite { - writable_roots: Vec::new(), - read_only_access: ReadOnlyAccess::Restricted { - include_platform_defaults: true, - readable_roots: Vec::new(), - }, - network_access: false, - exclude_tmpdir_env_var: true, - exclude_slash_tmp: true, - } - ); - } - - #[test] - fn helper_permissions_strip_network_grants() { - let codex_self_exe = std::env::current_exe().expect("current exe"); - let runtime_paths = ExecServerRuntimePaths::new( - codex_self_exe.clone(), - /*codex_linux_sandbox_exe*/ None, - ) - .expect("runtime paths"); - let runner = FileSystemSandboxRunner::new(runtime_paths); - let readable = AbsolutePathBuf::from_absolute_path( - codex_self_exe.parent().expect("current exe parent"), - ) - .expect("absolute readable path"); - let writable = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path()) - .expect("absolute writable path"); - - let permissions = runner.helper_permissions(Some(&PermissionProfile { - network: Some(NetworkPermissions { - enabled: Some(true), - }), - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![writable.clone()]), - }), - })); - - assert_eq!(permissions.network, None); - assert_eq!( - permissions - .file_system - .as_ref() - .and_then(|fs| fs.write.clone()), - Some(vec![writable]) - ); - assert_eq!( - permissions - .file_system - .as_ref() - .and_then(|fs| fs.read.clone()), - Some(vec![readable]) - ); - } - - #[test] - fn helper_permissions_include_helper_read_root_without_additional_permissions() { - let codex_self_exe = std::env::current_exe().expect("current exe"); - let runtime_paths = ExecServerRuntimePaths::new( - codex_self_exe.clone(), - /*codex_linux_sandbox_exe*/ None, - ) - .expect("runtime paths"); - let runner = FileSystemSandboxRunner::new(runtime_paths); - let readable = AbsolutePathBuf::from_absolute_path( - codex_self_exe.parent().expect("current exe parent"), - ) - .expect("absolute readable path"); - - let permissions = runner.helper_permissions(/*additional_permissions*/ None); - - assert_eq!(permissions.network, None); - assert_eq!( - permissions.file_system, - Some(FileSystemPermissions { - read: Some(vec![readable]), - write: None, - }) - ); - } -} +#[path = "fs_sandbox_tests.rs"] +mod tests; diff --git a/codex-rs/exec-server/src/fs_sandbox_tests.rs b/codex-rs/exec-server/src/fs_sandbox_tests.rs new file mode 100644 index 000000000000..337f79a1c687 --- /dev/null +++ b/codex-rs/exec-server/src/fs_sandbox_tests.rs @@ -0,0 +1,224 @@ +use codex_protocol::models::FileSystemPermissions; +use codex_protocol::models::NetworkPermissions; +use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::NetworkSandboxPolicy; +use codex_protocol::protocol::ReadOnlyAccess; +use codex_protocol::protocol::SandboxPolicy; +use codex_utils_absolute_path::AbsolutePathBuf; +use pretty_assertions::assert_eq; + +use crate::ExecServerRuntimePaths; +use crate::FileSystemSandboxContext; + +use super::FileSystemSandboxRunner; +use super::PATH_ENV_VAR; +use super::helper_env; +use super::helper_sandbox_inputs; +use super::sandbox_policy_with_helper_runtime_defaults; + +#[test] +fn helper_sandbox_policy_enables_platform_defaults_for_read_only_access() { + let sandbox_policy = SandboxPolicy::ReadOnly { + access: ReadOnlyAccess::Restricted { + include_platform_defaults: false, + readable_roots: Vec::new(), + }, + network_access: false, + }; + + let updated = sandbox_policy_with_helper_runtime_defaults(&sandbox_policy); + + assert_eq!( + updated, + SandboxPolicy::ReadOnly { + access: ReadOnlyAccess::Restricted { + include_platform_defaults: true, + readable_roots: Vec::new(), + }, + network_access: false, + } + ); +} + +#[test] +fn helper_sandbox_policy_enables_platform_defaults_for_workspace_read_access() { + let sandbox_policy = SandboxPolicy::WorkspaceWrite { + writable_roots: Vec::new(), + read_only_access: ReadOnlyAccess::Restricted { + include_platform_defaults: false, + readable_roots: Vec::new(), + }, + network_access: true, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + + let updated = sandbox_policy_with_helper_runtime_defaults(&sandbox_policy); + + assert_eq!( + updated, + SandboxPolicy::WorkspaceWrite { + writable_roots: Vec::new(), + read_only_access: ReadOnlyAccess::Restricted { + include_platform_defaults: true, + readable_roots: Vec::new(), + }, + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + } + ); +} + +#[test] +fn helper_sandbox_inputs_use_context_cwd_and_file_system_policy() { + let cwd = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path()) + .expect("absolute temp dir"); + let sandbox_policy = SandboxPolicy::new_workspace_write_policy(); + let file_system_policy = + codex_protocol::permissions::FileSystemSandboxPolicy::from_legacy_sandbox_policy( + &sandbox_policy, + cwd.as_path(), + ); + let mut sandbox_context = FileSystemSandboxContext::new(sandbox_policy.clone()); + sandbox_context.sandbox_policy_cwd = Some(cwd.clone()); + sandbox_context.file_system_sandbox_policy = Some(file_system_policy.clone()); + + let inputs = helper_sandbox_inputs(&sandbox_context).expect("helper sandbox inputs"); + + assert_eq!(inputs.cwd, cwd); + assert_eq!(inputs.sandbox_policy, sandbox_policy); + assert_eq!(inputs.file_system_policy, file_system_policy); + assert_eq!(inputs.network_policy, NetworkSandboxPolicy::Restricted); +} + +#[test] +fn helper_sandbox_inputs_rejects_file_system_policy_without_cwd() { + let cwd = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path()) + .expect("absolute temp dir"); + let sandbox_policy = SandboxPolicy::new_workspace_write_policy(); + let file_system_policy = + codex_protocol::permissions::FileSystemSandboxPolicy::from_legacy_sandbox_policy( + &sandbox_policy, + cwd.as_path(), + ); + let mut sandbox_context = FileSystemSandboxContext::new(sandbox_policy); + sandbox_context.file_system_sandbox_policy = Some(file_system_policy); + + let err = match helper_sandbox_inputs(&sandbox_context) { + Ok(_) => panic!("expected invalid sandbox inputs"), + Err(err) => err, + }; + + assert_eq!( + err.message, + "fileSystemSandboxPolicy requires sandboxPolicyCwd" + ); +} + +#[test] +fn helper_env_preserves_only_path() { + let env = helper_env(); + + match std::env::var_os(PATH_ENV_VAR) { + Some(path) => assert_eq!( + env, + std::collections::HashMap::from([( + PATH_ENV_VAR.to_string(), + path.to_string_lossy().into_owned() + )]) + ), + None => assert_eq!(env, std::collections::HashMap::new()), + } +} + +#[test] +fn helper_permissions_strip_network_grants() { + let codex_self_exe = std::env::current_exe().expect("current exe"); + let runtime_paths = ExecServerRuntimePaths::new( + codex_self_exe.clone(), + /*codex_linux_sandbox_exe*/ None, + ) + .expect("runtime paths"); + let runner = FileSystemSandboxRunner::new(runtime_paths); + let readable = + AbsolutePathBuf::from_absolute_path(codex_self_exe.parent().expect("current exe parent")) + .expect("absolute readable path"); + let writable = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path()) + .expect("absolute writable path"); + + let permissions = runner + .helper_permissions( + Some(&PermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + file_system: Some(FileSystemPermissions { + read: Some(vec![]), + write: Some(vec![writable.clone()]), + }), + }), + /*include_helper_read_root*/ true, + ) + .expect("helper permissions"); + + assert_eq!(permissions.network, None); + assert_eq!( + permissions + .file_system + .as_ref() + .and_then(|fs| fs.write.clone()), + Some(vec![writable]) + ); + assert_eq!( + permissions + .file_system + .as_ref() + .and_then(|fs| fs.read.clone()), + Some(vec![readable]) + ); +} + +#[test] +fn helper_permissions_include_helper_read_root_without_additional_permissions() { + let codex_self_exe = std::env::current_exe().expect("current exe"); + let runtime_paths = ExecServerRuntimePaths::new( + codex_self_exe.clone(), + /*codex_linux_sandbox_exe*/ None, + ) + .expect("runtime paths"); + let runner = FileSystemSandboxRunner::new(runtime_paths); + let readable = + AbsolutePathBuf::from_absolute_path(codex_self_exe.parent().expect("current exe parent")) + .expect("absolute readable path"); + + let permissions = runner + .helper_permissions( + /*additional_permissions*/ None, /*include_helper_read_root*/ true, + ) + .expect("helper permissions"); + + assert_eq!(permissions.network, None); + assert_eq!( + permissions.file_system, + Some(FileSystemPermissions { + read: Some(vec![readable]), + write: None, + }) + ); +} + +#[test] +fn legacy_landlock_helper_permissions_do_not_add_helper_read_root() { + let codex_self_exe = std::env::current_exe().expect("current exe"); + let runtime_paths = + ExecServerRuntimePaths::new(codex_self_exe, /*codex_linux_sandbox_exe*/ None) + .expect("runtime paths"); + let runner = FileSystemSandboxRunner::new(runtime_paths); + + let permissions = runner.helper_permissions( + /*additional_permissions*/ None, /*include_helper_read_root*/ false, + ); + + assert_eq!(permissions, None); +} From 738e52a8f55e2362e0e31e627254e2994e2bcc33 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Thu, 16 Apr 2026 23:53:20 -0700 Subject: [PATCH 2/3] test: keep fs sandbox tests inline Co-authored-by: Codex noreply@openai.com --- codex-rs/exec-server/src/fs_sandbox.rs | 230 ++++++++++++++++++- codex-rs/exec-server/src/fs_sandbox_tests.rs | 224 ------------------ 2 files changed, 228 insertions(+), 226 deletions(-) delete mode 100644 codex-rs/exec-server/src/fs_sandbox_tests.rs diff --git a/codex-rs/exec-server/src/fs_sandbox.rs b/codex-rs/exec-server/src/fs_sandbox.rs index 3fdf35520d86..aaac8f38c396 100644 --- a/codex-rs/exec-server/src/fs_sandbox.rs +++ b/codex-rs/exec-server/src/fs_sandbox.rs @@ -345,5 +345,231 @@ fn json_error(err: serde_json::Error) -> JSONRPCErrorError { } #[cfg(test)] -#[path = "fs_sandbox_tests.rs"] -mod tests; +mod tests { + use codex_protocol::models::FileSystemPermissions; + use codex_protocol::models::NetworkPermissions; + use codex_protocol::models::PermissionProfile; + use codex_protocol::permissions::NetworkSandboxPolicy; + use codex_protocol::protocol::ReadOnlyAccess; + use codex_protocol::protocol::SandboxPolicy; + use codex_utils_absolute_path::AbsolutePathBuf; + use pretty_assertions::assert_eq; + + use crate::ExecServerRuntimePaths; + use crate::FileSystemSandboxContext; + + use super::FileSystemSandboxRunner; + use super::PATH_ENV_VAR; + use super::helper_env; + use super::helper_sandbox_inputs; + use super::sandbox_policy_with_helper_runtime_defaults; + + #[test] + fn helper_sandbox_policy_enables_platform_defaults_for_read_only_access() { + let sandbox_policy = SandboxPolicy::ReadOnly { + access: ReadOnlyAccess::Restricted { + include_platform_defaults: false, + readable_roots: Vec::new(), + }, + network_access: false, + }; + + let updated = sandbox_policy_with_helper_runtime_defaults(&sandbox_policy); + + assert_eq!( + updated, + SandboxPolicy::ReadOnly { + access: ReadOnlyAccess::Restricted { + include_platform_defaults: true, + readable_roots: Vec::new(), + }, + network_access: false, + } + ); + } + + #[test] + fn helper_sandbox_policy_enables_platform_defaults_for_workspace_read_access() { + let sandbox_policy = SandboxPolicy::WorkspaceWrite { + writable_roots: Vec::new(), + read_only_access: ReadOnlyAccess::Restricted { + include_platform_defaults: false, + readable_roots: Vec::new(), + }, + network_access: true, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + + let updated = sandbox_policy_with_helper_runtime_defaults(&sandbox_policy); + + assert_eq!( + updated, + SandboxPolicy::WorkspaceWrite { + writable_roots: Vec::new(), + read_only_access: ReadOnlyAccess::Restricted { + include_platform_defaults: true, + readable_roots: Vec::new(), + }, + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + } + ); + } + + #[test] + fn helper_sandbox_inputs_use_context_cwd_and_file_system_policy() { + let cwd = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path()) + .expect("absolute temp dir"); + let sandbox_policy = SandboxPolicy::new_workspace_write_policy(); + let file_system_policy = + codex_protocol::permissions::FileSystemSandboxPolicy::from_legacy_sandbox_policy( + &sandbox_policy, + cwd.as_path(), + ); + let mut sandbox_context = FileSystemSandboxContext::new(sandbox_policy.clone()); + sandbox_context.sandbox_policy_cwd = Some(cwd.clone()); + sandbox_context.file_system_sandbox_policy = Some(file_system_policy.clone()); + + let inputs = helper_sandbox_inputs(&sandbox_context).expect("helper sandbox inputs"); + + assert_eq!(inputs.cwd, cwd); + assert_eq!(inputs.sandbox_policy, sandbox_policy); + assert_eq!(inputs.file_system_policy, file_system_policy); + assert_eq!(inputs.network_policy, NetworkSandboxPolicy::Restricted); + } + + #[test] + fn helper_sandbox_inputs_rejects_file_system_policy_without_cwd() { + let cwd = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path()) + .expect("absolute temp dir"); + let sandbox_policy = SandboxPolicy::new_workspace_write_policy(); + let file_system_policy = + codex_protocol::permissions::FileSystemSandboxPolicy::from_legacy_sandbox_policy( + &sandbox_policy, + cwd.as_path(), + ); + let mut sandbox_context = FileSystemSandboxContext::new(sandbox_policy); + sandbox_context.file_system_sandbox_policy = Some(file_system_policy); + + let err = match helper_sandbox_inputs(&sandbox_context) { + Ok(_) => panic!("expected invalid sandbox inputs"), + Err(err) => err, + }; + + assert_eq!( + err.message, + "fileSystemSandboxPolicy requires sandboxPolicyCwd" + ); + } + + #[test] + fn helper_env_preserves_only_path() { + let env = helper_env(); + + match std::env::var_os(PATH_ENV_VAR) { + Some(path) => assert_eq!( + env, + std::collections::HashMap::from([( + PATH_ENV_VAR.to_string(), + path.to_string_lossy().into_owned() + )]) + ), + None => assert_eq!(env, std::collections::HashMap::new()), + } + } + + #[test] + fn helper_permissions_strip_network_grants() { + let codex_self_exe = std::env::current_exe().expect("current exe"); + let runtime_paths = ExecServerRuntimePaths::new( + codex_self_exe.clone(), + /*codex_linux_sandbox_exe*/ None, + ) + .expect("runtime paths"); + let runner = FileSystemSandboxRunner::new(runtime_paths); + let readable = AbsolutePathBuf::from_absolute_path( + codex_self_exe.parent().expect("current exe parent"), + ) + .expect("absolute readable path"); + let writable = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path()) + .expect("absolute writable path"); + + let permissions = runner + .helper_permissions( + Some(&PermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + file_system: Some(FileSystemPermissions { + read: Some(vec![]), + write: Some(vec![writable.clone()]), + }), + }), + /*include_helper_read_root*/ true, + ) + .expect("helper permissions"); + + assert_eq!(permissions.network, None); + assert_eq!( + permissions + .file_system + .as_ref() + .and_then(|fs| fs.write.clone()), + Some(vec![writable]) + ); + assert_eq!( + permissions + .file_system + .as_ref() + .and_then(|fs| fs.read.clone()), + Some(vec![readable]) + ); + } + + #[test] + fn helper_permissions_include_helper_read_root_without_additional_permissions() { + let codex_self_exe = std::env::current_exe().expect("current exe"); + let runtime_paths = ExecServerRuntimePaths::new( + codex_self_exe.clone(), + /*codex_linux_sandbox_exe*/ None, + ) + .expect("runtime paths"); + let runner = FileSystemSandboxRunner::new(runtime_paths); + let readable = AbsolutePathBuf::from_absolute_path( + codex_self_exe.parent().expect("current exe parent"), + ) + .expect("absolute readable path"); + + let permissions = runner + .helper_permissions( + /*additional_permissions*/ None, /*include_helper_read_root*/ true, + ) + .expect("helper permissions"); + + assert_eq!(permissions.network, None); + assert_eq!( + permissions.file_system, + Some(FileSystemPermissions { + read: Some(vec![readable]), + write: None, + }) + ); + } + + #[test] + fn legacy_landlock_helper_permissions_do_not_add_helper_read_root() { + let codex_self_exe = std::env::current_exe().expect("current exe"); + let runtime_paths = + ExecServerRuntimePaths::new(codex_self_exe, /*codex_linux_sandbox_exe*/ None) + .expect("runtime paths"); + let runner = FileSystemSandboxRunner::new(runtime_paths); + + let permissions = runner.helper_permissions( + /*additional_permissions*/ None, /*include_helper_read_root*/ false, + ); + + assert_eq!(permissions, None); + } +} diff --git a/codex-rs/exec-server/src/fs_sandbox_tests.rs b/codex-rs/exec-server/src/fs_sandbox_tests.rs deleted file mode 100644 index 337f79a1c687..000000000000 --- a/codex-rs/exec-server/src/fs_sandbox_tests.rs +++ /dev/null @@ -1,224 +0,0 @@ -use codex_protocol::models::FileSystemPermissions; -use codex_protocol::models::NetworkPermissions; -use codex_protocol::models::PermissionProfile; -use codex_protocol::permissions::NetworkSandboxPolicy; -use codex_protocol::protocol::ReadOnlyAccess; -use codex_protocol::protocol::SandboxPolicy; -use codex_utils_absolute_path::AbsolutePathBuf; -use pretty_assertions::assert_eq; - -use crate::ExecServerRuntimePaths; -use crate::FileSystemSandboxContext; - -use super::FileSystemSandboxRunner; -use super::PATH_ENV_VAR; -use super::helper_env; -use super::helper_sandbox_inputs; -use super::sandbox_policy_with_helper_runtime_defaults; - -#[test] -fn helper_sandbox_policy_enables_platform_defaults_for_read_only_access() { - let sandbox_policy = SandboxPolicy::ReadOnly { - access: ReadOnlyAccess::Restricted { - include_platform_defaults: false, - readable_roots: Vec::new(), - }, - network_access: false, - }; - - let updated = sandbox_policy_with_helper_runtime_defaults(&sandbox_policy); - - assert_eq!( - updated, - SandboxPolicy::ReadOnly { - access: ReadOnlyAccess::Restricted { - include_platform_defaults: true, - readable_roots: Vec::new(), - }, - network_access: false, - } - ); -} - -#[test] -fn helper_sandbox_policy_enables_platform_defaults_for_workspace_read_access() { - let sandbox_policy = SandboxPolicy::WorkspaceWrite { - writable_roots: Vec::new(), - read_only_access: ReadOnlyAccess::Restricted { - include_platform_defaults: false, - readable_roots: Vec::new(), - }, - network_access: true, - exclude_tmpdir_env_var: true, - exclude_slash_tmp: true, - }; - - let updated = sandbox_policy_with_helper_runtime_defaults(&sandbox_policy); - - assert_eq!( - updated, - SandboxPolicy::WorkspaceWrite { - writable_roots: Vec::new(), - read_only_access: ReadOnlyAccess::Restricted { - include_platform_defaults: true, - readable_roots: Vec::new(), - }, - network_access: false, - exclude_tmpdir_env_var: true, - exclude_slash_tmp: true, - } - ); -} - -#[test] -fn helper_sandbox_inputs_use_context_cwd_and_file_system_policy() { - let cwd = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path()) - .expect("absolute temp dir"); - let sandbox_policy = SandboxPolicy::new_workspace_write_policy(); - let file_system_policy = - codex_protocol::permissions::FileSystemSandboxPolicy::from_legacy_sandbox_policy( - &sandbox_policy, - cwd.as_path(), - ); - let mut sandbox_context = FileSystemSandboxContext::new(sandbox_policy.clone()); - sandbox_context.sandbox_policy_cwd = Some(cwd.clone()); - sandbox_context.file_system_sandbox_policy = Some(file_system_policy.clone()); - - let inputs = helper_sandbox_inputs(&sandbox_context).expect("helper sandbox inputs"); - - assert_eq!(inputs.cwd, cwd); - assert_eq!(inputs.sandbox_policy, sandbox_policy); - assert_eq!(inputs.file_system_policy, file_system_policy); - assert_eq!(inputs.network_policy, NetworkSandboxPolicy::Restricted); -} - -#[test] -fn helper_sandbox_inputs_rejects_file_system_policy_without_cwd() { - let cwd = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path()) - .expect("absolute temp dir"); - let sandbox_policy = SandboxPolicy::new_workspace_write_policy(); - let file_system_policy = - codex_protocol::permissions::FileSystemSandboxPolicy::from_legacy_sandbox_policy( - &sandbox_policy, - cwd.as_path(), - ); - let mut sandbox_context = FileSystemSandboxContext::new(sandbox_policy); - sandbox_context.file_system_sandbox_policy = Some(file_system_policy); - - let err = match helper_sandbox_inputs(&sandbox_context) { - Ok(_) => panic!("expected invalid sandbox inputs"), - Err(err) => err, - }; - - assert_eq!( - err.message, - "fileSystemSandboxPolicy requires sandboxPolicyCwd" - ); -} - -#[test] -fn helper_env_preserves_only_path() { - let env = helper_env(); - - match std::env::var_os(PATH_ENV_VAR) { - Some(path) => assert_eq!( - env, - std::collections::HashMap::from([( - PATH_ENV_VAR.to_string(), - path.to_string_lossy().into_owned() - )]) - ), - None => assert_eq!(env, std::collections::HashMap::new()), - } -} - -#[test] -fn helper_permissions_strip_network_grants() { - let codex_self_exe = std::env::current_exe().expect("current exe"); - let runtime_paths = ExecServerRuntimePaths::new( - codex_self_exe.clone(), - /*codex_linux_sandbox_exe*/ None, - ) - .expect("runtime paths"); - let runner = FileSystemSandboxRunner::new(runtime_paths); - let readable = - AbsolutePathBuf::from_absolute_path(codex_self_exe.parent().expect("current exe parent")) - .expect("absolute readable path"); - let writable = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path()) - .expect("absolute writable path"); - - let permissions = runner - .helper_permissions( - Some(&PermissionProfile { - network: Some(NetworkPermissions { - enabled: Some(true), - }), - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![writable.clone()]), - }), - }), - /*include_helper_read_root*/ true, - ) - .expect("helper permissions"); - - assert_eq!(permissions.network, None); - assert_eq!( - permissions - .file_system - .as_ref() - .and_then(|fs| fs.write.clone()), - Some(vec![writable]) - ); - assert_eq!( - permissions - .file_system - .as_ref() - .and_then(|fs| fs.read.clone()), - Some(vec![readable]) - ); -} - -#[test] -fn helper_permissions_include_helper_read_root_without_additional_permissions() { - let codex_self_exe = std::env::current_exe().expect("current exe"); - let runtime_paths = ExecServerRuntimePaths::new( - codex_self_exe.clone(), - /*codex_linux_sandbox_exe*/ None, - ) - .expect("runtime paths"); - let runner = FileSystemSandboxRunner::new(runtime_paths); - let readable = - AbsolutePathBuf::from_absolute_path(codex_self_exe.parent().expect("current exe parent")) - .expect("absolute readable path"); - - let permissions = runner - .helper_permissions( - /*additional_permissions*/ None, /*include_helper_read_root*/ true, - ) - .expect("helper permissions"); - - assert_eq!(permissions.network, None); - assert_eq!( - permissions.file_system, - Some(FileSystemPermissions { - read: Some(vec![readable]), - write: None, - }) - ); -} - -#[test] -fn legacy_landlock_helper_permissions_do_not_add_helper_read_root() { - let codex_self_exe = std::env::current_exe().expect("current exe"); - let runtime_paths = - ExecServerRuntimePaths::new(codex_self_exe, /*codex_linux_sandbox_exe*/ None) - .expect("runtime paths"); - let runner = FileSystemSandboxRunner::new(runtime_paths); - - let permissions = runner.helper_permissions( - /*additional_permissions*/ None, /*include_helper_read_root*/ false, - ); - - assert_eq!(permissions, None); -} From f6a81aeb7a6261f1efb34ec63fe0441ca94e3a92 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Fri, 17 Apr 2026 12:41:59 -0700 Subject: [PATCH 3/3] chore: drop duplicate fs helper PATH fix Co-authored-by: Codex --- codex-rs/exec-server/src/fs_sandbox.rs | 33 +------------------------- 1 file changed, 1 insertion(+), 32 deletions(-) diff --git a/codex-rs/exec-server/src/fs_sandbox.rs b/codex-rs/exec-server/src/fs_sandbox.rs index aaac8f38c396..b1ddbb43d37a 100644 --- a/codex-rs/exec-server/src/fs_sandbox.rs +++ b/codex-rs/exec-server/src/fs_sandbox.rs @@ -28,8 +28,6 @@ use crate::local_file_system::current_sandbox_cwd; use crate::rpc::internal_error; use crate::rpc::invalid_request; -const PATH_ENV_VAR: &str = "PATH"; - #[derive(Clone, Debug)] pub(crate) struct FileSystemSandboxRunner { runtime_paths: ExecServerRuntimePaths, @@ -90,7 +88,7 @@ impl FileSystemSandboxRunner { program: helper.as_path().as_os_str().to_owned(), args: vec![CODEX_FS_HELPER_ARG1.to_string()], cwd: cwd.clone(), - env: helper_env(), + env: HashMap::new(), additional_permissions: self.helper_permissions( sandbox_context.additional_permissions.as_ref(), /*include_helper_read_root*/ !sandbox_context.use_legacy_landlock, @@ -179,17 +177,6 @@ fn helper_sandbox_inputs( }) } -fn helper_env() -> HashMap { - std::env::var_os(PATH_ENV_VAR) - .map(|path| { - HashMap::from([( - PATH_ENV_VAR.to_string(), - path.to_string_lossy().into_owned(), - )]) - }) - .unwrap_or_default() -} - fn normalize_sandbox_policy_root_aliases(sandbox_policy: SandboxPolicy) -> SandboxPolicy { let mut sandbox_policy = sandbox_policy; match &mut sandbox_policy { @@ -359,8 +346,6 @@ mod tests { use crate::FileSystemSandboxContext; use super::FileSystemSandboxRunner; - use super::PATH_ENV_VAR; - use super::helper_env; use super::helper_sandbox_inputs; use super::sandbox_policy_with_helper_runtime_defaults; @@ -464,22 +449,6 @@ mod tests { ); } - #[test] - fn helper_env_preserves_only_path() { - let env = helper_env(); - - match std::env::var_os(PATH_ENV_VAR) { - Some(path) => assert_eq!( - env, - std::collections::HashMap::from([( - PATH_ENV_VAR.to_string(), - path.to_string_lossy().into_owned() - )]) - ), - None => assert_eq!(env, std::collections::HashMap::new()), - } - } - #[test] fn helper_permissions_strip_network_grants() { let codex_self_exe = std::env::current_exe().expect("current exe");