diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index cec4ebcdedfd..f647c6495663 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -209,6 +209,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 @@ -219,6 +221,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() @@ -226,18 +241,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(), @@ -247,7 +250,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 04cded30eb90..059b33c04dca 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; @@ -35,6 +36,13 @@ pub(crate) struct FileSystemSandboxRunner { helper_env: HashMap, } +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 { @@ -48,19 +56,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, @@ -92,8 +95,9 @@ impl FileSystemSandboxRunner { args: vec![CODEX_FS_HELPER_ARG1.to_string()], cwd: cwd.clone(), env: self.helper_env.clone(), - additional_permissions: Some( - self.helper_permissions(sandbox_context.additional_permissions.as_ref()), + additional_permissions: self.helper_permissions( + sandbox_context.additional_permissions.as_ref(), + /*include_helper_read_root*/ !sandbox_context.use_legacy_landlock, ), }; sandbox_manager @@ -117,36 +121,68 @@ 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 normalize_sandbox_policy_root_aliases(sandbox_policy: SandboxPolicy) -> SandboxPolicy { let mut sandbox_policy = sandbox_policy; match &mut sandbox_policy { @@ -281,10 +317,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 @@ -326,11 +373,13 @@ mod tests { use pretty_assertions::assert_eq; use crate::ExecServerRuntimePaths; + use crate::FileSystemSandboxContext; use super::FileSystemSandboxRunner; use super::helper_env; use super::helper_env_from_vars; use super::helper_env_key_is_allowed; + use super::helper_sandbox_inputs; use super::sandbox_policy_with_helper_runtime_defaults; #[test] @@ -365,7 +414,7 @@ mod tests { include_platform_defaults: false, readable_roots: Vec::new(), }, - network_access: false, + network_access: true, exclude_tmpdir_env_var: true, exclude_slash_tmp: true, }; @@ -387,6 +436,52 @@ mod tests { ); } + #[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_permissions_strip_network_grants() { let codex_self_exe = std::env::current_exe().expect("current exe"); @@ -403,15 +498,20 @@ mod tests { 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()]), - }), - })); + 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!( @@ -537,7 +637,11 @@ mod tests { ) .expect("absolute readable path"); - let permissions = runner.helper_permissions(/*additional_permissions*/ None); + let permissions = runner + .helper_permissions( + /*additional_permissions*/ None, /*include_helper_read_root*/ true, + ) + .expect("helper permissions"); assert_eq!(permissions.network, None); assert_eq!( @@ -548,4 +652,19 @@ mod tests { }) ); } + + #[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); + } }