From 2b9bf539d323d09929a5cc76ba77e45a6df1c22d Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 26 Feb 2026 09:06:59 -0800 Subject: [PATCH] feat: include sandbox config with escalation request --- codex-rs/Cargo.lock | 1 + codex-rs/core/src/config/mod.rs | 5 +- codex-rs/core/src/exec.rs | 2 + codex-rs/core/src/sandboxing/mod.rs | 11 +- codex-rs/core/src/seatbelt_permissions.rs | 58 +- codex-rs/core/src/skills/permissions.rs | 5 +- codex-rs/core/src/tools/js_repl/mod.rs | 2 + .../tools/runtimes/shell/unix_escalation.rs | 476 +++++++++++++-- codex-rs/core/src/tools/sandboxing.rs | 2 + codex-rs/core/tests/suite/mod.rs | 1 + codex-rs/core/tests/suite/skill_approval.rs | 559 +++++++++++++++--- codex-rs/protocol/src/approvals.rs | 15 + codex-rs/protocol/src/models.rs | 26 + codex-rs/shell-escalation/Cargo.toml | 1 + codex-rs/shell-escalation/src/lib.rs | 10 + .../src/unix/escalate_protocol.rs | 33 ++ .../src/unix/escalate_server.rs | 211 ++++++- .../src/unix/escalation_policy.rs | 4 +- codex-rs/shell-escalation/src/unix/mod.rs | 5 + 19 files changed, 1227 insertions(+), 200 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index c888f9368d7..ee024099c91 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2299,6 +2299,7 @@ dependencies = [ "anyhow", "async-trait", "clap", + "codex-protocol", "codex-utils-absolute-path", "libc", "pretty_assertions", diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 306ba49c100..5f5579cc83a 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -49,8 +49,6 @@ use crate::project_doc::LOCAL_PROJECT_DOC_FILENAME; use crate::protocol::AskForApproval; use crate::protocol::ReadOnlyAccess; use crate::protocol::SandboxPolicy; -#[cfg(target_os = "macos")] -use crate::seatbelt_permissions::MacOsSeatbeltProfileExtensions; use crate::unified_exec::DEFAULT_MAX_BACKGROUND_TERMINAL_TIMEOUT_MS; use crate::unified_exec::MIN_EMPTY_YIELD_TIME_MS; use crate::windows_sandbox::WindowsSandboxLevelExt; @@ -66,6 +64,7 @@ use codex_protocol::config_types::TrustLevel; use codex_protocol::config_types::Verbosity; use codex_protocol::config_types::WebSearchMode; use codex_protocol::config_types::WindowsSandboxLevel; +use codex_protocol::models::MacOsSeatbeltProfileExtensions; use codex_protocol::openai_models::ModelsResponse; use codex_protocol::openai_models::ReasoningEffort; use codex_rmcp_client::OAuthCredentialsStoreMode; @@ -82,8 +81,6 @@ use std::path::Path; use std::path::PathBuf; #[cfg(test)] use tempfile::tempdir; -#[cfg(not(target_os = "macos"))] -type MacOsSeatbeltProfileExtensions = (); use crate::config::permissions::network_proxy_config_from_permissions; use crate::config::profile::ConfigProfile; diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 288ee7f2127..5a805a3b8b4 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -219,6 +219,8 @@ pub async fn process_exec_tool_call( enforce_managed_network, network: network.as_ref(), sandbox_policy_cwd: sandbox_cwd, + #[cfg(target_os = "macos")] + macos_seatbelt_profile_extensions: None, codex_linux_sandbox_exe: codex_linux_sandbox_exe.as_ref(), use_linux_sandbox_bwrap, windows_sandbox_level, diff --git a/codex-rs/core/src/sandboxing/mod.rs b/codex-rs/core/src/sandboxing/mod.rs index 56c7bff6a68..da03f00a62c 100644 --- a/codex-rs/core/src/sandboxing/mod.rs +++ b/codex-rs/core/src/sandboxing/mod.rs @@ -17,7 +17,7 @@ use crate::protocol::SandboxPolicy; #[cfg(target_os = "macos")] use crate::seatbelt::MACOS_PATH_TO_SEATBELT_EXECUTABLE; #[cfg(target_os = "macos")] -use crate::seatbelt::create_seatbelt_command_args; +use crate::seatbelt::create_seatbelt_command_args_with_extensions; #[cfg(target_os = "macos")] use crate::spawn::CODEX_SANDBOX_ENV_VAR; use crate::spawn::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; @@ -25,6 +25,8 @@ use crate::tools::sandboxing::SandboxablePreference; use codex_network_proxy::NetworkProxy; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::FileSystemPermissions; +#[cfg(target_os = "macos")] +use codex_protocol::models::MacOsSeatbeltProfileExtensions; use codex_protocol::models::PermissionProfile; pub use codex_protocol::models::SandboxPermissions; use codex_protocol::protocol::ReadOnlyAccess; @@ -73,6 +75,8 @@ pub(crate) struct SandboxTransformRequest<'a> { // to make shared ownership explicit across runtime/sandbox plumbing. pub network: Option<&'a NetworkProxy>, pub sandbox_policy_cwd: &'a Path, + #[cfg(target_os = "macos")] + pub macos_seatbelt_profile_extensions: Option<&'a MacOsSeatbeltProfileExtensions>, pub codex_linux_sandbox_exe: Option<&'a PathBuf>, pub use_linux_sandbox_bwrap: bool, pub windows_sandbox_level: WindowsSandboxLevel, @@ -342,6 +346,8 @@ impl SandboxManager { enforce_managed_network, network, sandbox_policy_cwd, + #[cfg(target_os = "macos")] + macos_seatbelt_profile_extensions, codex_linux_sandbox_exe, use_linux_sandbox_bwrap, windows_sandbox_level, @@ -370,12 +376,13 @@ impl SandboxManager { SandboxType::MacosSeatbelt => { let mut seatbelt_env = HashMap::new(); seatbelt_env.insert(CODEX_SANDBOX_ENV_VAR.to_string(), "seatbelt".to_string()); - let mut args = create_seatbelt_command_args( + let mut args = create_seatbelt_command_args_with_extensions( command.clone(), &effective_policy, sandbox_policy_cwd, enforce_managed_network, network, + macos_seatbelt_profile_extensions, ); let mut full_command = Vec::with_capacity(1 + args.len()); full_command.push(MACOS_PATH_TO_SEATBELT_EXECUTABLE.to_string()); diff --git a/codex-rs/core/src/seatbelt_permissions.rs b/codex-rs/core/src/seatbelt_permissions.rs index aacc3a77843..6b9fa681884 100644 --- a/codex-rs/core/src/seatbelt_permissions.rs +++ b/codex-rs/core/src/seatbelt_permissions.rs @@ -3,34 +3,9 @@ use std::collections::BTreeSet; use std::path::PathBuf; -#[allow(dead_code)] -#[derive(Debug, Clone, PartialEq, Eq, Default)] -pub enum MacOsPreferencesPermission { - // IMPORTANT: ReadOnly needs to be the default because it's the security-sensitive default. - // it's important for allowing cf prefs to work. - #[default] - ReadOnly, - ReadWrite, - None, -} - -#[allow(dead_code)] -#[derive(Debug, Clone, PartialEq, Eq, Default)] -pub enum MacOsAutomationPermission { - #[default] - None, - All, - BundleIds(Vec), -} - -#[allow(dead_code)] -#[derive(Debug, Clone, PartialEq, Eq, Default)] -pub struct MacOsSeatbeltProfileExtensions { - pub macos_preferences: MacOsPreferencesPermission, - pub macos_automation: MacOsAutomationPermission, - pub macos_accessibility: bool, - pub macos_calendar: bool, -} +pub use codex_protocol::models::MacOsAutomationPermission; +pub use codex_protocol::models::MacOsPreferencesPermission; +pub use codex_protocol::models::MacOsSeatbeltProfileExtensions; #[derive(Debug, Clone, PartialEq, Eq, Default)] pub(crate) struct SeatbeltExtensionPolicy { @@ -38,25 +13,26 @@ pub(crate) struct SeatbeltExtensionPolicy { pub(crate) dir_params: Vec<(String, PathBuf)>, } -impl MacOsSeatbeltProfileExtensions { - pub fn normalized(&self) -> Self { - let mut normalized = self.clone(); - if let MacOsAutomationPermission::BundleIds(bundle_ids) = &self.macos_automation { - let bundle_ids = normalize_bundle_ids(bundle_ids); - normalized.macos_automation = if bundle_ids.is_empty() { - MacOsAutomationPermission::None - } else { - MacOsAutomationPermission::BundleIds(bundle_ids) - }; - } - normalized +fn normalized_extensions( + extensions: &MacOsSeatbeltProfileExtensions, +) -> MacOsSeatbeltProfileExtensions { + let mut normalized = extensions.clone(); + if let MacOsAutomationPermission::BundleIds(bundle_ids) = &extensions.macos_automation { + let bundle_ids = normalize_bundle_ids(bundle_ids); + normalized.macos_automation = if bundle_ids.is_empty() { + MacOsAutomationPermission::None + } else { + MacOsAutomationPermission::BundleIds(bundle_ids) + }; } + + normalized } pub(crate) fn build_seatbelt_extensions( extensions: &MacOsSeatbeltProfileExtensions, ) -> SeatbeltExtensionPolicy { - let extensions = extensions.normalized(); + let extensions = normalized_extensions(extensions); let mut clauses = Vec::new(); match extensions.macos_preferences { diff --git a/codex-rs/core/src/skills/permissions.rs b/codex-rs/core/src/skills/permissions.rs index 4019a310799..89c49709e37 100644 --- a/codex-rs/core/src/skills/permissions.rs +++ b/codex-rs/core/src/skills/permissions.rs @@ -8,6 +8,7 @@ use codex_protocol::models::MacOsAutomationValue; use codex_protocol::models::MacOsPermissions; #[cfg(target_os = "macos")] use codex_protocol::models::MacOsPreferencesValue; +use codex_protocol::models::MacOsSeatbeltProfileExtensions; use codex_protocol::models::PermissionProfile; use codex_utils_absolute_path::AbsolutePathBuf; use dirs::home_dir; @@ -20,10 +21,6 @@ use crate::config::types::ShellEnvironmentPolicy; use crate::protocol::AskForApproval; use crate::protocol::ReadOnlyAccess; use crate::protocol::SandboxPolicy; -#[cfg(target_os = "macos")] -use crate::seatbelt_permissions::MacOsSeatbeltProfileExtensions; -#[cfg(not(target_os = "macos"))] -type MacOsSeatbeltProfileExtensions = (); pub(crate) fn compile_permission_profile( skill_dir: &Path, diff --git a/codex-rs/core/src/tools/js_repl/mod.rs b/codex-rs/core/src/tools/js_repl/mod.rs index aaa1ab29d1a..fdee8a517b7 100644 --- a/codex-rs/core/src/tools/js_repl/mod.rs +++ b/codex-rs/core/src/tools/js_repl/mod.rs @@ -613,6 +613,8 @@ impl JsReplManager { enforce_managed_network: has_managed_network_requirements, network: None, sandbox_policy_cwd: &turn.cwd, + #[cfg(target_os = "macos")] + macos_seatbelt_profile_extensions: None, codex_linux_sandbox_exe: turn.codex_linux_sandbox_exe.as_ref(), use_linux_sandbox_bwrap: turn .features diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index 5bcf809e213..084b245072e 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -12,12 +12,14 @@ use crate::skills::SkillMetadata; use crate::tools::runtimes::ExecveSessionApproval; use crate::tools::runtimes::build_command_spec; use crate::tools::sandboxing::SandboxAttempt; +use crate::tools::sandboxing::SandboxablePreference; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use codex_execpolicy::Decision; use codex_execpolicy::Policy; use codex_execpolicy::RuleMatch; use codex_protocol::config_types::WindowsSandboxLevel; +use codex_protocol::models::MacOsSeatbeltProfileExtensions; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::NetworkPolicyRuleAction; @@ -26,11 +28,15 @@ use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::SandboxPolicy; use codex_shell_command::bash::parse_shell_lc_plain_commands; use codex_shell_command::bash::parse_shell_lc_single_command_prefix; -use codex_shell_escalation::EscalateAction; use codex_shell_escalation::EscalateServer; +use codex_shell_escalation::EscalationDecision; +use codex_shell_escalation::EscalationExecution; +use codex_shell_escalation::EscalationPermissions; use codex_shell_escalation::EscalationPolicy; use codex_shell_escalation::ExecParams; use codex_shell_escalation::ExecResult; +use codex_shell_escalation::Permissions as EscalatedPermissions; +use codex_shell_escalation::PreparedExec; use codex_shell_escalation::ShellCommandExecutor; use codex_shell_escalation::Stopwatch; use codex_utils_absolute_path::AbsolutePathBuf; @@ -105,6 +111,15 @@ pub(super) async fn try_run_zsh_fork( sandbox_permissions, justification, arg0, + sandbox_policy_cwd: ctx.turn.cwd.clone(), + macos_seatbelt_profile_extensions: ctx + .turn + .config + .permissions + .macos_seatbelt_profile_extensions + .clone(), + codex_linux_sandbox_exe: ctx.turn.codex_linux_sandbox_exe.clone(), + use_linux_sandbox_bwrap: ctx.turn.features.enabled(Feature::UseLinuxSandboxBwrap), }; let main_execve_wrapper_exe = ctx .session @@ -136,6 +151,7 @@ pub(super) async fn try_run_zsh_fork( approval_policy: ctx.turn.approval_policy.value(), sandbox_policy: attempt.policy.clone(), sandbox_permissions: req.sandbox_permissions, + prompt_permissions: req.additional_permissions.clone(), stopwatch: stopwatch.clone(), }; @@ -146,7 +162,7 @@ pub(super) async fn try_run_zsh_fork( ); let exec_result = escalate_server - .exec(exec_params, cancel_token, &command_executor) + .exec(exec_params, cancel_token, Arc::new(command_executor)) .await .map_err(|err| ToolError::Rejected(err.to_string()))?; @@ -161,6 +177,7 @@ struct CoreShellActionProvider { approval_policy: AskForApproval, sandbox_policy: SandboxPolicy, sandbox_permissions: SandboxPermissions, + prompt_permissions: Option, stopwatch: Stopwatch, } @@ -182,6 +199,55 @@ impl CoreShellActionProvider { }) } + fn shell_request_escalation_execution( + sandbox_permissions: SandboxPermissions, + sandbox_policy: &SandboxPolicy, + additional_permissions: Option<&PermissionProfile>, + macos_seatbelt_profile_extensions: Option<&MacOsSeatbeltProfileExtensions>, + ) -> EscalationExecution { + match sandbox_permissions { + SandboxPermissions::UseDefault => EscalationExecution::TurnDefault, + SandboxPermissions::RequireEscalated => EscalationExecution::Unsandboxed, + SandboxPermissions::WithAdditionalPermissions => additional_permissions + .map(|_| { + // Shell request additional permissions were already normalized and + // merged into the first-attempt sandbox policy. + EscalationExecution::Permissions(EscalationPermissions::Permissions( + EscalatedPermissions { + sandbox_policy: sandbox_policy.clone(), + macos_seatbelt_profile_extensions: macos_seatbelt_profile_extensions + .cloned(), + }, + )) + }) + .unwrap_or(EscalationExecution::TurnDefault), + } + } + + fn skill_escalation_execution(skill: &SkillMetadata) -> EscalationExecution { + skill + .permissions + .as_ref() + .map(|permissions| { + EscalationExecution::Permissions(EscalationPermissions::Permissions( + EscalatedPermissions { + sandbox_policy: permissions.sandbox_policy.get().clone(), + macos_seatbelt_profile_extensions: permissions + .macos_seatbelt_profile_extensions + .clone(), + }, + )) + }) + .or_else(|| { + skill + .permission_profile + .clone() + .map(EscalationPermissions::PermissionProfile) + .map(EscalationExecution::Permissions) + }) + .unwrap_or(EscalationExecution::TurnDefault) + } + async fn prompt( &self, program: &AbsolutePathBuf, @@ -265,22 +331,21 @@ impl CoreShellActionProvider { program: &AbsolutePathBuf, argv: &[String], workdir: &AbsolutePathBuf, - additional_permissions: Option, + prompt_permissions: Option, + escalation_execution: EscalationExecution, decision_source: DecisionSource, - ) -> anyhow::Result { + ) -> anyhow::Result { let action = match decision { - Decision::Forbidden => EscalateAction::Deny { - reason: Some("Execution forbidden by policy".to_string()), - }, + Decision::Forbidden => { + EscalationDecision::deny(Some("Execution forbidden by policy".to_string())) + } Decision::Prompt => { if matches!( self.approval_policy, AskForApproval::Never | AskForApproval::Reject(RejectConfig { rules: true, .. }) ) { - EscalateAction::Deny { - reason: Some("Execution forbidden by policy".to_string()), - } + EscalationDecision::deny(Some("Execution forbidden by policy".to_string())) } else { match self .prompt( @@ -288,7 +353,7 @@ impl CoreShellActionProvider { argv, workdir, &self.stopwatch, - additional_permissions, + prompt_permissions, &decision_source, ) .await? @@ -296,9 +361,9 @@ impl CoreShellActionProvider { ReviewDecision::Approved | ReviewDecision::ApprovedExecpolicyAmendment { .. } => { if needs_escalation { - EscalateAction::Escalate + EscalationDecision::escalate(escalation_execution.clone()) } else { - EscalateAction::Run + EscalationDecision::run() } } ReviewDecision::ApprovedForSession => { @@ -323,9 +388,9 @@ impl CoreShellActionProvider { } if needs_escalation { - EscalateAction::Escalate + EscalationDecision::escalate(escalation_execution.clone()) } else { - EscalateAction::Run + EscalationDecision::run() } } ReviewDecision::NetworkPolicyAmendment { @@ -333,29 +398,29 @@ impl CoreShellActionProvider { } => match network_policy_amendment.action { NetworkPolicyRuleAction::Allow => { if needs_escalation { - EscalateAction::Escalate + EscalationDecision::escalate(escalation_execution.clone()) } else { - EscalateAction::Run + EscalationDecision::run() } } - NetworkPolicyRuleAction::Deny => EscalateAction::Deny { - reason: Some("User denied execution".to_string()), - }, - }, - ReviewDecision::Denied => EscalateAction::Deny { - reason: Some("User denied execution".to_string()), - }, - ReviewDecision::Abort => EscalateAction::Deny { - reason: Some("User cancelled execution".to_string()), + NetworkPolicyRuleAction::Deny => { + EscalationDecision::deny(Some("User denied execution".to_string())) + } }, + ReviewDecision::Denied => { + EscalationDecision::deny(Some("User denied execution".to_string())) + } + ReviewDecision::Abort => { + EscalationDecision::deny(Some("User cancelled execution".to_string())) + } } } } Decision::Allow => { if needs_escalation { - EscalateAction::Escalate + EscalationDecision::escalate(escalation_execution) } else { - EscalateAction::Run + EscalationDecision::run() } } }; @@ -373,7 +438,7 @@ impl EscalationPolicy for CoreShellActionProvider { program: &AbsolutePathBuf, argv: &[String], workdir: &AbsolutePathBuf, - ) -> anyhow::Result { + ) -> anyhow::Result { tracing::debug!( "Determining escalation action for command {program:?} with args {argv:?} in {workdir:?}" ); @@ -394,15 +459,13 @@ impl EscalationPolicy for CoreShellActionProvider { tracing::debug!( "Found session approval for {program:?}, allowing execution without further checks" ); - // TODO(mbolin): We need to include the permissions with the - // escalation decision so it can be run with the appropriate - // permissions. - let _permissions = approval + let execution = approval .skill .as_ref() - .and_then(|s| s.permission_profile.clone()); + .map(Self::skill_escalation_execution) + .unwrap_or(EscalationExecution::TurnDefault); - return Ok(EscalateAction::Escalate); + return Ok(EscalationDecision::escalate(execution)); } // In the usual case, the execve wrapper reports the command being @@ -424,6 +487,7 @@ impl EscalationPolicy for CoreShellActionProvider { argv, workdir, skill.permission_profile.clone(), + Self::skill_escalation_execution(&skill), decision_source, ) .await; @@ -464,13 +528,24 @@ impl EscalationPolicy for CoreShellActionProvider { } else { DecisionSource::UnmatchedCommandFallback }; + let escalation_execution = Self::shell_request_escalation_execution( + self.sandbox_permissions, + &self.sandbox_policy, + self.prompt_permissions.as_ref(), + self.turn + .config + .permissions + .macos_seatbelt_profile_extensions + .as_ref(), + ); self.process_decision( evaluation.decision, needs_escalation, program, argv, workdir, - None, + self.prompt_permissions.clone(), + escalation_execution, decision_source, ) .await @@ -488,6 +563,10 @@ struct CoreShellCommandExecutor { sandbox_permissions: SandboxPermissions, justification: Option, arg0: Option, + sandbox_policy_cwd: PathBuf, + macos_seatbelt_profile_extensions: Option, + codex_linux_sandbox_exe: Option, + use_linux_sandbox_bwrap: bool, } #[async_trait::async_trait] @@ -533,6 +612,126 @@ impl ShellCommandExecutor for CoreShellCommandExecutor { timed_out: result.timed_out, }) } + + async fn prepare_escalated_exec( + &self, + program: &AbsolutePathBuf, + argv: &[String], + workdir: &AbsolutePathBuf, + env: HashMap, + execution: EscalationExecution, + ) -> anyhow::Result { + let command = join_program_and_argv(program, argv); + let Some(first_arg) = argv.first() else { + return Err(anyhow::anyhow!( + "intercepted exec request must contain argv[0]" + )); + }; + + let prepared = match execution { + EscalationExecution::Unsandboxed => PreparedExec { + command, + cwd: workdir.to_path_buf(), + env, + arg0: Some(first_arg.clone()), + }, + EscalationExecution::TurnDefault => self.prepare_sandboxed_exec( + command, + workdir, + env, + &self.sandbox_policy, + None, + self.macos_seatbelt_profile_extensions.as_ref(), + )?, + EscalationExecution::Permissions(EscalationPermissions::PermissionProfile( + permission_profile, + )) => self.prepare_sandboxed_exec( + command, + workdir, + env, + &self.sandbox_policy, + Some(permission_profile), + None, + )?, + EscalationExecution::Permissions(EscalationPermissions::Permissions(permissions)) => { + self.prepare_sandboxed_exec( + command, + workdir, + env, + &permissions.sandbox_policy, + None, + permissions.macos_seatbelt_profile_extensions.as_ref(), + )? + } + }; + + Ok(prepared) + } +} + +impl CoreShellCommandExecutor { + fn prepare_sandboxed_exec( + &self, + command: Vec, + workdir: &AbsolutePathBuf, + env: HashMap, + sandbox_policy: &SandboxPolicy, + additional_permissions: Option, + #[cfg(target_os = "macos")] macos_seatbelt_profile_extensions: Option< + &MacOsSeatbeltProfileExtensions, + >, + #[cfg(not(target_os = "macos"))] _macos_seatbelt_profile_extensions: Option< + &MacOsSeatbeltProfileExtensions, + >, + ) -> anyhow::Result { + let (program, args) = command + .split_first() + .ok_or_else(|| anyhow::anyhow!("prepared command must not be empty"))?; + let sandbox_manager = crate::sandboxing::SandboxManager::new(); + let sandbox = sandbox_manager.select_initial( + sandbox_policy, + SandboxablePreference::Auto, + self.windows_sandbox_level, + self.network.is_some(), + ); + let mut exec_request = + sandbox_manager.transform(crate::sandboxing::SandboxTransformRequest { + spec: crate::sandboxing::CommandSpec { + program: program.clone(), + args: args.to_vec(), + cwd: workdir.to_path_buf(), + env, + expiration: ExecExpiration::DefaultTimeout, + sandbox_permissions: if additional_permissions.is_some() { + SandboxPermissions::WithAdditionalPermissions + } else { + SandboxPermissions::UseDefault + }, + additional_permissions, + justification: self.justification.clone(), + }, + policy: sandbox_policy, + sandbox, + enforce_managed_network: self.network.is_some(), + network: self.network.as_ref(), + sandbox_policy_cwd: &self.sandbox_policy_cwd, + #[cfg(target_os = "macos")] + macos_seatbelt_profile_extensions, + codex_linux_sandbox_exe: self.codex_linux_sandbox_exe.as_ref(), + use_linux_sandbox_bwrap: self.use_linux_sandbox_bwrap, + windows_sandbox_level: self.windows_sandbox_level, + })?; + if let Some(network) = exec_request.network.as_ref() { + network.apply_to_env(&mut exec_request.env); + } + + Ok(PreparedExec { + command: exec_request.command, + cwd: exec_request.cwd, + env: exec_request.env, + arg0: exec_request.arg0, + }) + } } #[derive(Debug, Eq, PartialEq)] @@ -601,14 +800,45 @@ fn join_program_and_argv(program: &AbsolutePathBuf, argv: &[String]) -> Vec SandboxAttempt<'a> { enforce_managed_network: self.enforce_managed_network, network, sandbox_policy_cwd: self.sandbox_cwd, + #[cfg(target_os = "macos")] + macos_seatbelt_profile_extensions: None, codex_linux_sandbox_exe: self.codex_linux_sandbox_exe, use_linux_sandbox_bwrap: self.use_linux_sandbox_bwrap, windows_sandbox_level: self.windows_sandbox_level, diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index 1509428f385..4cfe955b554 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -114,6 +114,7 @@ mod seatbelt; mod shell_command; mod shell_serialization; mod shell_snapshot; +mod skill_approval; mod skills; mod sqlite_state; mod stream_error_allows_next_turn; diff --git a/codex-rs/core/tests/suite/skill_approval.rs b/codex-rs/core/tests/suite/skill_approval.rs index a3cfd4d7fdf..c2c729be334 100644 --- a/codex-rs/core/tests/suite/skill_approval.rs +++ b/codex-rs/core/tests/suite/skill_approval.rs @@ -2,13 +2,16 @@ #![cfg(unix)] use anyhow::Result; +use codex_core::config::Config; use codex_core::features::Feature; use codex_protocol::config_types::ReasoningSummary; use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::ExecApprovalRequestEvent; use codex_protocol::protocol::Op; +use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::SandboxPolicy; use codex_protocol::user_input::UserInput; use core_test_support::responses::mount_function_call_agent_response; @@ -65,6 +68,24 @@ async fn submit_turn_with_policies( } fn write_skill_with_shell_script(home: &Path, name: &str, script_name: &str) -> Result { + write_skill_with_shell_script_contents( + home, + name, + script_name, + r#"#!/bin/sh +echo 'zsh-fork-stdout' +echo 'zsh-fork-stderr' >&2 +"#, + ) +} + +#[cfg(unix)] +fn write_skill_with_shell_script_contents( + home: &Path, + name: &str, + script_name: &str, + script_contents: &str, +) -> Result { use std::os::unix::fs::PermissionsExt; let skill_dir = home.join("skills").join(name); @@ -82,13 +103,7 @@ description: {name} skill )?; let script_path = scripts_dir.join(script_name); - fs::write( - &script_path, - r#"#!/bin/sh -echo 'zsh-fork-stdout' -echo 'zsh-fork-stderr' >&2 -"#, - )?; + fs::write(&script_path, script_contents)?; let mut permissions = fs::metadata(&script_path)?.permissions(); permissions.set_mode(0o755); fs::set_permissions(&script_path, permissions)?; @@ -129,34 +144,134 @@ fn supports_exec_wrapper_intercept(zsh_path: &Path) -> bool { } } -#[cfg(unix)] -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn shell_zsh_fork_prompts_for_skill_script_execution() -> Result<()> { - use codex_config::Constrained; - use codex_protocol::protocol::ReviewDecision; +#[derive(Clone)] +struct ZshForkRuntime { + zsh_path: PathBuf, + main_execve_wrapper_exe: PathBuf, +} - skip_if_no_network!(Ok(())); +impl ZshForkRuntime { + fn apply_to_config( + &self, + config: &mut Config, + approval_policy: AskForApproval, + sandbox_policy: SandboxPolicy, + ) { + use codex_config::Constrained; + config.features.enable(Feature::ShellTool); + config.features.enable(Feature::ShellZshFork); + config.zsh_path = Some(self.zsh_path.clone()); + config.main_execve_wrapper_exe = Some(self.main_execve_wrapper_exe.clone()); + config.permissions.allow_login_shell = false; + config.permissions.approval_policy = Constrained::allow_any(approval_policy); + config.permissions.sandbox_policy = Constrained::allow_any(sandbox_policy); + } +} + +fn restrictive_workspace_write_policy() -> SandboxPolicy { + SandboxPolicy::WorkspaceWrite { + writable_roots: Vec::new(), + read_only_access: Default::default(), + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + } +} + +fn zsh_fork_runtime(test_name: &str) -> Result> { let Some(zsh_path) = find_test_zsh_path()? else { - return Ok(()); + return Ok(None); }; if !supports_exec_wrapper_intercept(&zsh_path) { eprintln!( - "skipping zsh-fork skill test: zsh does not support EXEC_WRAPPER intercepts ({})", + "skipping {test_name}: zsh does not support EXEC_WRAPPER intercepts ({})", zsh_path.display() ); - return Ok(()); + return Ok(None); } let Ok(main_execve_wrapper_exe) = codex_utils_cargo_bin::cargo_bin("codex-execve-wrapper") else { - eprintln!("skipping zsh-fork skill test: unable to resolve `codex-execve-wrapper` binary"); + eprintln!("skipping {test_name}: unable to resolve `codex-execve-wrapper` binary"); + return Ok(None); + }; + + Ok(Some(ZshForkRuntime { + zsh_path, + main_execve_wrapper_exe, + })) +} + +async fn build_zsh_fork_test( + server: &wiremock::MockServer, + runtime: ZshForkRuntime, + approval_policy: AskForApproval, + sandbox_policy: SandboxPolicy, + pre_build_hook: F, +) -> Result +where + F: FnOnce(&Path) + Send + 'static, +{ + let mut builder = test_codex() + .with_pre_build_hook(pre_build_hook) + .with_config(move |config| { + runtime.apply_to_config(config, approval_policy, sandbox_policy); + }); + builder.build(server).await +} + +fn skill_script_command(test: &TestCodex, script_name: &str) -> Result<(String, String)> { + let script_path = fs::canonicalize( + test.codex_home_path() + .join("skills/mbolin-test-skill/scripts") + .join(script_name), + )?; + let script_path_str = script_path.to_string_lossy().into_owned(); + let command = shlex::try_join([script_path_str.as_str()])?; + Ok((script_path_str, command)) +} + +async fn wait_for_exec_approval_request(test: &TestCodex) -> Option { + wait_for_event_match(test.codex.as_ref(), |event| match event { + EventMsg::ExecApprovalRequest(request) => Some(Some(request.clone())), + EventMsg::TurnComplete(_) => Some(None), + _ => None, + }) + .await +} + +async fn wait_for_turn_complete(test: &TestCodex) { + wait_for_event(test.codex.as_ref(), |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; +} + +fn output_shows_sandbox_denial(output: &str) -> bool { + output.contains("Permission denied") + || output.contains("Operation not permitted") + || output.contains("Read-only file system") +} + +/// Focus on the approval payload: the skill should prompt before execution and +/// only advertise the permissions declared in its metadata. +#[cfg(unix)] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn shell_zsh_fork_prompts_for_skill_script_execution() -> Result<()> { + skip_if_no_network!(Ok(())); + + let Some(runtime) = zsh_fork_runtime("zsh-fork skill prompt test")? else { return Ok(()); }; let server = start_mock_server().await; let tool_call_id = "zsh-fork-skill-call"; - let mut builder = test_codex() - .with_pre_build_hook(|home| { + let test = build_zsh_fork_test( + &server, + runtime, + AskForApproval::OnRequest, + SandboxPolicy::new_workspace_write_policy(), + |home| { write_skill_with_shell_script(home, "mbolin-test-skill", "hello-mbolin.sh").unwrap(); write_skill_metadata( home, @@ -171,25 +286,11 @@ permissions: "#, ) .unwrap(); - }) - .with_config(move |config| { - config.features.enable(Feature::ShellTool); - config.features.enable(Feature::ShellZshFork); - config.zsh_path = Some(zsh_path.clone()); - config.main_execve_wrapper_exe = Some(main_execve_wrapper_exe); - config.permissions.allow_login_shell = false; - config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); - config.permissions.sandbox_policy = - Constrained::allow_any(SandboxPolicy::new_workspace_write_policy()); - }); - let test = builder.build(&server).await?; + }, + ) + .await?; - let script_path = fs::canonicalize( - test.codex_home_path() - .join("skills/mbolin-test-skill/scripts/hello-mbolin.sh"), - )?; - let script_path_str = script_path.to_string_lossy().into_owned(); - let command = shlex::try_join([script_path_str.as_str()])?; + let (script_path_str, command) = skill_script_command(&test, "hello-mbolin.sh")?; let arguments = shell_command_arguments(&command)?; let mocks = mount_function_call_agent_response(&server, tool_call_id, &arguments, "shell_command") @@ -203,12 +304,7 @@ permissions: ) .await?; - let maybe_approval = wait_for_event_match(test.codex.as_ref(), |event| match event { - EventMsg::ExecApprovalRequest(request) => Some(Some(request.clone())), - EventMsg::TurnComplete(_) => Some(None), - _ => None, - }) - .await; + let maybe_approval = wait_for_exec_approval_request(&test).await; let approval = match maybe_approval { Some(approval) => approval, None => { @@ -250,10 +346,7 @@ permissions: }) .await?; - wait_for_event(test.codex.as_ref(), |event| { - matches!(event, EventMsg::TurnComplete(_)) - }) - .await; + wait_for_turn_complete(&test).await; let call_output = mocks .completion @@ -268,58 +361,350 @@ permissions: Ok(()) } +/// Look for `additional_permissions == None`, then verify that both the first +/// run and the cached session-approval rerun stay inside the turn sandbox. #[cfg(unix)] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn shell_zsh_fork_still_enforces_workspace_write_sandbox() -> Result<()> { - use codex_config::Constrained; - use codex_protocol::protocol::AskForApproval; - +async fn shell_zsh_fork_skill_without_permissions_inherits_turn_sandbox() -> Result<()> { skip_if_no_network!(Ok(())); - let Some(zsh_path) = find_test_zsh_path()? else { + let Some(runtime) = zsh_fork_runtime("zsh-fork inherited skill sandbox test")? else { return Ok(()); }; - if !supports_exec_wrapper_intercept(&zsh_path) { - eprintln!( - "skipping zsh-fork sandbox test: zsh does not support EXEC_WRAPPER intercepts ({})", - zsh_path.display() - ); + + let outside_dir = tempfile::tempdir_in(std::env::current_dir()?)?; + let outside_path = outside_dir + .path() + .join("zsh-fork-skill-inherited-sandbox.txt"); + let outside_path_quoted = shlex::try_join([outside_path.to_string_lossy().as_ref()])?; + let script_contents = format!( + "#!/bin/sh\nprintf '%s' forbidden > {outside_path_quoted}\ncat {outside_path_quoted}\n" + ); + let outside_path_for_hook = outside_path.clone(); + let script_contents_for_hook = script_contents.clone(); + let workspace_write_policy = restrictive_workspace_write_policy(); + + let server = start_mock_server().await; + let test = build_zsh_fork_test( + &server, + runtime, + AskForApproval::OnRequest, + workspace_write_policy.clone(), + move |home| { + let _ = fs::remove_file(&outside_path_for_hook); + write_skill_with_shell_script_contents( + home, + "mbolin-test-skill", + "sandboxed.sh", + &script_contents_for_hook, + ) + .unwrap(); + }, + ) + .await?; + + let (script_path_str, command) = skill_script_command(&test, "sandboxed.sh")?; + + let first_call_id = "zsh-fork-skill-permissions-1"; + let first_arguments = shell_command_arguments(&command)?; + let first_mocks = mount_function_call_agent_response( + &server, + first_call_id, + &first_arguments, + "shell_command", + ) + .await; + + submit_turn_with_policies( + &test, + "use $mbolin-test-skill", + AskForApproval::OnRequest, + workspace_write_policy.clone(), + ) + .await?; + + let maybe_approval = wait_for_exec_approval_request(&test).await; + let approval = match maybe_approval { + Some(approval) => approval, + None => panic!("expected exec approval request before completion"), + }; + assert_eq!(approval.call_id, first_call_id); + assert_eq!(approval.command, vec![script_path_str.clone()]); + assert_eq!(approval.additional_permissions, None); + + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::ApprovedForSession, + }) + .await?; + + wait_for_turn_complete(&test).await; + + let first_output = first_mocks + .completion + .single_request() + .function_call_output(first_call_id)["output"] + .as_str() + .unwrap_or_default() + .to_string(); + assert!( + output_shows_sandbox_denial(&first_output) || !first_output.contains("forbidden"), + "expected inherited turn sandbox denial on first run, got output: {first_output:?}" + ); + assert!( + !outside_path.exists(), + "first run should not write outside the turn sandbox" + ); + + let second_call_id = "zsh-fork-skill-permissions-2"; + let second_arguments = shell_command_arguments(&command)?; + let second_mocks = mount_function_call_agent_response( + &server, + second_call_id, + &second_arguments, + "shell_command", + ) + .await; + + submit_turn_with_policies( + &test, + "use $mbolin-test-skill", + AskForApproval::OnRequest, + workspace_write_policy, + ) + .await?; + + let cached_approval = wait_for_exec_approval_request(&test).await; + assert!( + cached_approval.is_none(), + "expected second run to reuse the cached session approval" + ); + + let second_output = second_mocks + .completion + .single_request() + .function_call_output(second_call_id)["output"] + .as_str() + .unwrap_or_default() + .to_string(); + assert!( + output_shows_sandbox_denial(&second_output) || !second_output.contains("forbidden"), + "expected cached skill approval to retain inherited turn sandboxing, got output: {second_output:?}" + ); + assert!( + !outside_path.exists(), + "cached session approval should not widen a permissionless skill to full access" + ); + + Ok(()) +} + +/// The validation to focus on is: writes to the skill-approved folder succeed, +/// and writes to an unrelated folder fail, both before and after cached approval. +#[cfg(unix)] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn shell_zsh_fork_skill_session_approval_enforces_skill_permissions() -> Result<()> { + skip_if_no_network!(Ok(())); + + let Some(runtime) = zsh_fork_runtime("zsh-fork explicit skill sandbox test")? else { return Ok(()); - } - let Ok(main_execve_wrapper_exe) = codex_utils_cargo_bin::cargo_bin("codex-execve-wrapper") - else { - eprintln!( - "skipping zsh-fork sandbox test: unable to resolve `codex-execve-wrapper` binary" - ); + }; + + let outside_dir = tempfile::tempdir_in(std::env::current_dir()?)?; + let allowed_dir = outside_dir.path().join("allowed-output"); + let blocked_dir = outside_dir.path().join("blocked-output"); + fs::create_dir_all(&allowed_dir)?; + fs::create_dir_all(&blocked_dir)?; + + let allowed_path = allowed_dir.join("allowed.txt"); + let blocked_path = blocked_dir.join("blocked.txt"); + let allowed_path_quoted = shlex::try_join([allowed_path.to_string_lossy().as_ref()])?; + let blocked_path_quoted = shlex::try_join([blocked_path.to_string_lossy().as_ref()])?; + let script_contents = format!( + "#!/bin/sh\nprintf '%s' allowed > {allowed_path_quoted}\ncat {allowed_path_quoted}\nprintf '%s' forbidden > {blocked_path_quoted}\nif [ -f {blocked_path_quoted} ]; then echo blocked-created; fi\n" + ); + let allowed_dir_for_hook = allowed_dir.clone(); + let allowed_path_for_hook = allowed_path.clone(); + let blocked_path_for_hook = blocked_path.clone(); + let script_contents_for_hook = script_contents.clone(); + + let permissions_yaml = format!( + "permissions:\n file_system:\n write:\n - \"{}\"\n", + allowed_dir.display() + ); + + let workspace_write_policy = restrictive_workspace_write_policy(); + let server = start_mock_server().await; + let test = build_zsh_fork_test( + &server, + runtime, + AskForApproval::OnRequest, + workspace_write_policy.clone(), + move |home| { + let _ = fs::remove_file(&allowed_path_for_hook); + let _ = fs::remove_file(&blocked_path_for_hook); + fs::create_dir_all(&allowed_dir_for_hook).unwrap(); + fs::create_dir_all(blocked_path_for_hook.parent().unwrap()).unwrap(); + write_skill_with_shell_script_contents( + home, + "mbolin-test-skill", + "sandboxed.sh", + &script_contents_for_hook, + ) + .unwrap(); + write_skill_metadata(home, "mbolin-test-skill", &permissions_yaml).unwrap(); + }, + ) + .await?; + + let (script_path_str, command) = skill_script_command(&test, "sandboxed.sh")?; + + let first_call_id = "zsh-fork-skill-permissions-1"; + let first_arguments = shell_command_arguments(&command)?; + let first_mocks = mount_function_call_agent_response( + &server, + first_call_id, + &first_arguments, + "shell_command", + ) + .await; + + submit_turn_with_policies( + &test, + "use $mbolin-test-skill", + AskForApproval::OnRequest, + workspace_write_policy.clone(), + ) + .await?; + + let maybe_approval = wait_for_exec_approval_request(&test).await; + let approval = match maybe_approval { + Some(approval) => approval, + None => panic!("expected exec approval request before completion"), + }; + assert_eq!(approval.call_id, first_call_id); + assert_eq!(approval.command, vec![script_path_str.clone()]); + assert_eq!( + approval.additional_permissions, + Some(PermissionProfile { + file_system: Some(FileSystemPermissions { + read: None, + write: Some(vec![allowed_dir.clone()]), + }), + ..Default::default() + }) + ); + + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::ApprovedForSession, + }) + .await?; + + wait_for_turn_complete(&test).await; + + let first_output = first_mocks + .completion + .single_request() + .function_call_output(first_call_id)["output"] + .as_str() + .unwrap_or_default() + .to_string(); + assert!( + first_output.contains("allowed"), + "expected skill sandbox to permit writes to the approved folder, got output: {first_output:?}" + ); + assert_eq!(fs::read_to_string(&allowed_path)?, "allowed"); + assert!( + !blocked_path.exists(), + "first run should not write outside the explicit skill sandbox" + ); + assert!( + !first_output.contains("blocked-created"), + "blocked path should not have been created: {first_output:?}" + ); + + let second_call_id = "zsh-fork-skill-permissions-2"; + let second_arguments = shell_command_arguments(&command)?; + let second_mocks = mount_function_call_agent_response( + &server, + second_call_id, + &second_arguments, + "shell_command", + ) + .await; + + let _ = fs::remove_file(&allowed_path); + let _ = fs::remove_file(&blocked_path); + + submit_turn_with_policies( + &test, + "use $mbolin-test-skill", + AskForApproval::OnRequest, + workspace_write_policy, + ) + .await?; + + let cached_approval = wait_for_exec_approval_request(&test).await; + assert!( + cached_approval.is_none(), + "expected second run to reuse the cached session approval" + ); + + let second_output = second_mocks + .completion + .single_request() + .function_call_output(second_call_id)["output"] + .as_str() + .unwrap_or_default() + .to_string(); + assert!( + second_output.contains("allowed"), + "expected cached skill approval to retain the explicit skill sandbox, got output: {second_output:?}" + ); + assert_eq!(fs::read_to_string(&allowed_path)?, "allowed"); + assert!( + !blocked_path.exists(), + "cached session approval should not widen skill execution beyond the explicit skill sandbox" + ); + assert!( + !second_output.contains("blocked-created"), + "blocked path should not have been created after cached approval: {second_output:?}" + ); + + Ok(()) +} + +/// This stays narrow on purpose: the important check is that `WorkspaceWrite` +/// continues to deny writes outside the workspace even under `zsh-fork`. +#[cfg(unix)] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn shell_zsh_fork_still_enforces_workspace_write_sandbox() -> Result<()> { + skip_if_no_network!(Ok(())); + + let Some(runtime) = zsh_fork_runtime("zsh-fork workspace sandbox test")? else { return Ok(()); }; let server = start_mock_server().await; let tool_call_id = "zsh-fork-workspace-write-deny"; let outside_path = "/tmp/codex-zsh-fork-workspace-write-deny.txt"; - let workspace_write_policy = SandboxPolicy::WorkspaceWrite { - writable_roots: Vec::new(), - read_only_access: Default::default(), - network_access: false, - exclude_tmpdir_env_var: true, - exclude_slash_tmp: true, - }; - let policy_for_config = workspace_write_policy.clone(); + let workspace_write_policy = restrictive_workspace_write_policy(); let _ = fs::remove_file(outside_path); - let mut builder = test_codex() - .with_pre_build_hook(move |_| { + let test = build_zsh_fork_test( + &server, + runtime, + AskForApproval::Never, + workspace_write_policy.clone(), + move |_| { let _ = fs::remove_file(outside_path); - }) - .with_config(move |config| { - config.features.enable(Feature::ShellTool); - config.features.enable(Feature::ShellZshFork); - config.zsh_path = Some(zsh_path.clone()); - config.main_execve_wrapper_exe = Some(main_execve_wrapper_exe); - config.permissions.allow_login_shell = false; - config.permissions.approval_policy = Constrained::allow_any(AskForApproval::Never); - config.permissions.sandbox_policy = Constrained::allow_any(policy_for_config); - }); - let test = builder.build(&server).await?; + }, + ) + .await?; let command = format!("touch {outside_path}"); let arguments = shell_command_arguments(&command)?; @@ -335,7 +720,7 @@ async fn shell_zsh_fork_still_enforces_workspace_write_sandbox() -> Result<()> { ) .await?; - wait_for_turn_complete_without_skill_approval(&test).await; + wait_for_turn_complete(&test).await; let call_output = mocks .completion @@ -343,9 +728,7 @@ async fn shell_zsh_fork_still_enforces_workspace_write_sandbox() -> Result<()> { .function_call_output(tool_call_id); let output = call_output["output"].as_str().unwrap_or_default(); assert!( - output.contains("Permission denied") - || output.contains("Operation not permitted") - || output.contains("Read-only file system"), + output_shows_sandbox_denial(output), "expected sandbox denial, got output: {output:?}" ); assert!( diff --git a/codex-rs/protocol/src/approvals.rs b/codex-rs/protocol/src/approvals.rs index 98fe0280e3d..12bd65ec598 100644 --- a/codex-rs/protocol/src/approvals.rs +++ b/codex-rs/protocol/src/approvals.rs @@ -2,15 +2,30 @@ use std::collections::HashMap; use std::path::PathBuf; use crate::mcp::RequestId; +use crate::models::MacOsSeatbeltProfileExtensions; use crate::models::PermissionProfile; use crate::parse_command::ParsedCommand; use crate::protocol::FileChange; use crate::protocol::ReviewDecision; +use crate::protocol::SandboxPolicy; use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; use ts_rs::TS; +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Permissions { + pub sandbox_policy: SandboxPolicy, + pub macos_seatbelt_profile_extensions: Option, +} + +#[allow(clippy::large_enum_variant)] +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum EscalationPermissions { + PermissionProfile(PermissionProfile), + Permissions(Permissions), +} + /// Proposed execpolicy change to allow commands starting with this prefix. /// /// The `command` tokens form the prefix that would be added as an execpolicy diff --git a/codex-rs/protocol/src/models.rs b/codex-rs/protocol/src/models.rs index ff5030c75eb..d74401c609f 100644 --- a/codex-rs/protocol/src/models.rs +++ b/codex-rs/protocol/src/models.rs @@ -95,6 +95,32 @@ pub enum MacOsAutomationValue { BundleIds(Vec), } +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub enum MacOsPreferencesPermission { + // IMPORTANT: ReadOnly needs to be the default because it's the + // security-sensitive default and keeps cf prefs working. + #[default] + ReadOnly, + ReadWrite, + None, +} + +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub enum MacOsAutomationPermission { + #[default] + None, + All, + BundleIds(Vec), +} + +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub struct MacOsSeatbeltProfileExtensions { + pub macos_preferences: MacOsPreferencesPermission, + pub macos_automation: MacOsAutomationPermission, + pub macos_accessibility: bool, + pub macos_calendar: bool, +} + #[derive(Debug, Clone, Default, Eq, Hash, PartialEq, Serialize, Deserialize, JsonSchema, TS)] pub struct PermissionProfile { pub network: Option, diff --git a/codex-rs/shell-escalation/Cargo.toml b/codex-rs/shell-escalation/Cargo.toml index 85075cfb51e..fbc5bcd8c7b 100644 --- a/codex-rs/shell-escalation/Cargo.toml +++ b/codex-rs/shell-escalation/Cargo.toml @@ -12,6 +12,7 @@ path = "src/bin/main_execve_wrapper.rs" anyhow = { workspace = true } async-trait = { workspace = true } clap = { workspace = true, features = ["derive"] } +codex-protocol = { workspace = true } codex-utils-absolute-path = { workspace = true } libc = { workspace = true } serde = { workspace = true, features = ["derive"] } diff --git a/codex-rs/shell-escalation/src/lib.rs b/codex-rs/shell-escalation/src/lib.rs index 48bc1165811..1cc42a46db4 100644 --- a/codex-rs/shell-escalation/src/lib.rs +++ b/codex-rs/shell-escalation/src/lib.rs @@ -6,12 +6,22 @@ pub use unix::EscalateAction; #[cfg(unix)] pub use unix::EscalateServer; #[cfg(unix)] +pub use unix::EscalationDecision; +#[cfg(unix)] +pub use unix::EscalationExecution; +#[cfg(unix)] +pub use unix::EscalationPermissions; +#[cfg(unix)] pub use unix::EscalationPolicy; #[cfg(unix)] pub use unix::ExecParams; #[cfg(unix)] pub use unix::ExecResult; #[cfg(unix)] +pub use unix::Permissions; +#[cfg(unix)] +pub use unix::PreparedExec; +#[cfg(unix)] pub use unix::ShellCommandExecutor; #[cfg(unix)] pub use unix::Stopwatch; diff --git a/codex-rs/shell-escalation/src/unix/escalate_protocol.rs b/codex-rs/shell-escalation/src/unix/escalate_protocol.rs index 6002fdc57b1..ea38c7b2a6b 100644 --- a/codex-rs/shell-escalation/src/unix/escalate_protocol.rs +++ b/codex-rs/shell-escalation/src/unix/escalate_protocol.rs @@ -2,6 +2,7 @@ use std::collections::HashMap; use std::os::fd::RawFd; use std::path::PathBuf; +use codex_protocol::approvals::EscalationPermissions; use codex_utils_absolute_path::AbsolutePathBuf; use serde::Deserialize; use serde::Serialize; @@ -35,6 +36,38 @@ pub struct EscalateResponse { pub action: EscalateAction, } +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum EscalationDecision { + Run, + Escalate(EscalationExecution), + Deny { reason: Option }, +} + +#[allow(clippy::large_enum_variant)] +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum EscalationExecution { + /// Rerun the intercepted command outside any sandbox wrapper. + Unsandboxed, + /// Rerun using the turn's current sandbox configuration. + TurnDefault, + /// Rerun using an explicit sandbox configuration attached to the request. + Permissions(EscalationPermissions), +} + +impl EscalationDecision { + pub fn run() -> Self { + Self::Run + } + + pub fn escalate(execution: EscalationExecution) -> Self { + Self::Escalate(execution) + } + + pub fn deny(reason: Option) -> Self { + Self::Deny { reason } + } +} + #[derive(Clone, Serialize, Deserialize, Debug, PartialEq, Eq)] pub enum EscalateAction { /// The command should be run directly by the client. diff --git a/codex-rs/shell-escalation/src/unix/escalate_server.rs b/codex-rs/shell-escalation/src/unix/escalate_server.rs index 2951f584a6a..bb45c3ed718 100644 --- a/codex-rs/shell-escalation/src/unix/escalate_server.rs +++ b/codex-rs/shell-escalation/src/unix/escalate_server.rs @@ -15,6 +15,8 @@ use crate::unix::escalate_protocol::EXEC_WRAPPER_ENV_VAR; use crate::unix::escalate_protocol::EscalateAction; use crate::unix::escalate_protocol::EscalateRequest; use crate::unix::escalate_protocol::EscalateResponse; +use crate::unix::escalate_protocol::EscalationDecision; +use crate::unix::escalate_protocol::EscalationExecution; use crate::unix::escalate_protocol::LEGACY_BASH_EXEC_WRAPPER_ENV_VAR; use crate::unix::escalate_protocol::SuperExecMessage; use crate::unix::escalate_protocol::SuperExecResult; @@ -37,6 +39,16 @@ pub trait ShellCommandExecutor: Send + Sync { env: HashMap, cancel_rx: CancellationToken, ) -> anyhow::Result; + + /// Prepares an escalated subcommand for execution on the server side. + async fn prepare_escalated_exec( + &self, + program: &AbsolutePathBuf, + argv: &[String], + workdir: &AbsolutePathBuf, + env: HashMap, + execution: EscalationExecution, + ) -> anyhow::Result; } #[derive(Debug, serde::Deserialize, serde::Serialize)] @@ -62,6 +74,14 @@ pub struct ExecResult { pub timed_out: bool, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PreparedExec { + pub command: Vec, + pub cwd: PathBuf, + pub env: HashMap, + pub arg0: Option, +} + pub struct EscalateServer { bash_path: PathBuf, execve_wrapper: PathBuf, @@ -69,9 +89,9 @@ pub struct EscalateServer { } impl EscalateServer { - pub fn new

(bash_path: PathBuf, execve_wrapper: PathBuf, policy: P) -> Self + pub fn new(bash_path: PathBuf, execve_wrapper: PathBuf, policy: Policy) -> Self where - P: EscalationPolicy + Send + Sync + 'static, + Policy: EscalationPolicy + Send + Sync + 'static, { Self { bash_path, @@ -84,13 +104,17 @@ impl EscalateServer { &self, params: ExecParams, cancel_rx: CancellationToken, - command_executor: &dyn ShellCommandExecutor, + command_executor: Arc, ) -> anyhow::Result { let (escalate_server, escalate_client) = AsyncDatagramSocket::pair()?; let client_socket = escalate_client.into_inner(); // Only the client endpoint should cross exec into the wrapper process. client_socket.set_cloexec(false)?; - let escalate_task = tokio::spawn(escalate_task(escalate_server, self.policy.clone())); + let escalate_task = tokio::spawn(escalate_task( + escalate_server, + Arc::clone(&self.policy), + Arc::clone(&command_executor), + )); let mut env = std::env::vars().collect::>(); env.insert( ESCALATE_SOCKET_ENV_VAR.to_string(), @@ -126,6 +150,7 @@ impl EscalateServer { async fn escalate_task( socket: AsyncDatagramSocket, policy: Arc, + command_executor: Arc, ) -> anyhow::Result<()> { loop { let (_, mut fds) = socket.receive_with_fds().await?; @@ -134,9 +159,12 @@ async fn escalate_task( continue; } let stream_socket = AsyncSocket::from_fd(fds.remove(0))?; - let policy = policy.clone(); + let policy = Arc::clone(&policy); + let command_executor = Arc::clone(&command_executor); tokio::spawn(async move { - if let Err(err) = handle_escalate_session_with_policy(stream_socket, policy).await { + if let Err(err) = + handle_escalate_session_with_policy(stream_socket, policy, command_executor).await + { tracing::error!("escalate session failed: {err:?}"); } }); @@ -146,6 +174,7 @@ async fn escalate_task( async fn handle_escalate_session_with_policy( socket: AsyncSocket, policy: Arc, + command_executor: Arc, ) -> anyhow::Result<()> { let EscalateRequest { file, @@ -154,22 +183,22 @@ async fn handle_escalate_session_with_policy( env, } = socket.receive::().await?; let program = AbsolutePathBuf::resolve_path_against_base(file, workdir.as_path())?; - let action = policy + let decision = policy .determine_action(&program, &argv, &workdir) .await .context("failed to determine escalation action")?; - tracing::debug!("decided {action:?} for {program:?} {argv:?} {workdir:?}"); + tracing::debug!("decided {decision:?} for {program:?} {argv:?} {workdir:?}"); - match action { - EscalateAction::Run => { + match decision { + EscalationDecision::Run => { socket .send(EscalateResponse { action: EscalateAction::Run, }) .await?; } - EscalateAction::Escalate => { + EscalationDecision::Escalate(execution) => { socket .send(EscalateResponse { action: EscalateAction::Escalate, @@ -197,12 +226,23 @@ async fn handle_escalate_session_with_policy( )); } - let mut command = Command::new(program.as_path()); + let PreparedExec { + command, + cwd, + env, + arg0, + } = command_executor + .prepare_escalated_exec(&program, &argv, &workdir, env, execution) + .await?; + let (program, args) = command + .split_first() + .ok_or_else(|| anyhow::anyhow!("prepared escalated command must not be empty"))?; + let mut command = Command::new(program); command - .args(&argv[1..]) - .arg0(argv[0].clone()) + .args(args) + .arg0(arg0.unwrap_or_else(|| program.clone())) .envs(&env) - .current_dir(&workdir) + .current_dir(&cwd) .stdin(Stdio::null()) .stdout(Stdio::null()) .stderr(Stdio::null()); @@ -222,7 +262,7 @@ async fn handle_escalate_session_with_policy( }) .await?; } - EscalateAction::Deny { reason } => { + EscalationDecision::Deny { reason } => { socket .send(EscalateResponse { action: EscalateAction::Deny { reason }, @@ -236,13 +276,15 @@ async fn handle_escalate_session_with_policy( #[cfg(test)] mod tests { use super::*; + use codex_protocol::approvals::EscalationPermissions; + use codex_protocol::models::PermissionProfile; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use std::collections::HashMap; use std::path::PathBuf; struct DeterministicEscalationPolicy { - action: EscalateAction, + decision: EscalationDecision, } #[async_trait::async_trait] @@ -252,8 +294,8 @@ mod tests { _file: &AbsolutePathBuf, _argv: &[String], _workdir: &AbsolutePathBuf, - ) -> anyhow::Result { - Ok(self.action.clone()) + ) -> anyhow::Result { + Ok(self.decision.clone()) } } @@ -269,10 +311,82 @@ mod tests { file: &AbsolutePathBuf, _argv: &[String], workdir: &AbsolutePathBuf, - ) -> anyhow::Result { + ) -> anyhow::Result { assert_eq!(file, &self.expected_file); assert_eq!(workdir, &self.expected_workdir); - Ok(EscalateAction::Run) + Ok(EscalationDecision::run()) + } + } + + struct ForwardingShellCommandExecutor; + + #[async_trait::async_trait] + impl ShellCommandExecutor for ForwardingShellCommandExecutor { + async fn run( + &self, + _command: Vec, + _cwd: PathBuf, + _env: HashMap, + _cancel_rx: CancellationToken, + ) -> anyhow::Result { + unreachable!("run() is not used by handle_escalate_session_with_policy() tests") + } + + async fn prepare_escalated_exec( + &self, + program: &AbsolutePathBuf, + argv: &[String], + workdir: &AbsolutePathBuf, + env: HashMap, + _execution: EscalationExecution, + ) -> anyhow::Result { + Ok(PreparedExec { + command: std::iter::once(program.to_string_lossy().to_string()) + .chain(argv.iter().skip(1).cloned()) + .collect(), + cwd: workdir.to_path_buf(), + env, + arg0: argv.first().cloned(), + }) + } + } + + struct PermissionAssertingShellCommandExecutor { + expected_permissions: EscalationPermissions, + } + + #[async_trait::async_trait] + impl ShellCommandExecutor for PermissionAssertingShellCommandExecutor { + async fn run( + &self, + _command: Vec, + _cwd: PathBuf, + _env: HashMap, + _cancel_rx: CancellationToken, + ) -> anyhow::Result { + unreachable!("run() is not used by handle_escalate_session_with_policy() tests") + } + + async fn prepare_escalated_exec( + &self, + program: &AbsolutePathBuf, + argv: &[String], + workdir: &AbsolutePathBuf, + env: HashMap, + execution: EscalationExecution, + ) -> anyhow::Result { + assert_eq!( + execution, + EscalationExecution::Permissions(self.expected_permissions.clone()) + ); + Ok(PreparedExec { + command: std::iter::once(program.to_string_lossy().to_string()) + .chain(argv.iter().skip(1).cloned()) + .collect(), + cwd: workdir.to_path_buf(), + env, + arg0: argv.first().cloned(), + }) } } @@ -282,8 +396,9 @@ mod tests { let server_task = tokio::spawn(handle_escalate_session_with_policy( server, Arc::new(DeterministicEscalationPolicy { - action: EscalateAction::Run, + decision: EscalationDecision::run(), }), + Arc::new(ForwardingShellCommandExecutor), )); let mut env = HashMap::new(); @@ -326,6 +441,7 @@ mod tests { expected_file, expected_workdir: workdir.clone(), }), + Arc::new(ForwardingShellCommandExecutor), )); client @@ -353,8 +469,9 @@ mod tests { let server_task = tokio::spawn(handle_escalate_session_with_policy( server, Arc::new(DeterministicEscalationPolicy { - action: EscalateAction::Escalate, + decision: EscalationDecision::escalate(EscalationExecution::Unsandboxed), }), + Arc::new(ForwardingShellCommandExecutor), )); client @@ -387,4 +504,52 @@ mod tests { server_task.await? } + + #[tokio::test] + async fn handle_escalate_session_passes_permissions_to_executor() -> anyhow::Result<()> { + let (server, client) = AsyncSocket::pair()?; + let server_task = tokio::spawn(handle_escalate_session_with_policy( + server, + Arc::new(DeterministicEscalationPolicy { + decision: EscalationDecision::escalate(EscalationExecution::Permissions( + EscalationPermissions::PermissionProfile(PermissionProfile { + network: Some(true), + ..Default::default() + }), + )), + }), + Arc::new(PermissionAssertingShellCommandExecutor { + expected_permissions: EscalationPermissions::PermissionProfile(PermissionProfile { + network: Some(true), + ..Default::default() + }), + }), + )); + + client + .send(EscalateRequest { + file: PathBuf::from("/bin/sh"), + argv: vec!["sh".to_string(), "-c".to_string(), "exit 0".to_string()], + workdir: AbsolutePathBuf::current_dir()?, + env: HashMap::new(), + }) + .await?; + + let response = client.receive::().await?; + assert_eq!( + EscalateResponse { + action: EscalateAction::Escalate, + }, + response + ); + + client + .send_with_fds(SuperExecMessage { fds: Vec::new() }, &[]) + .await?; + + let result = client.receive::().await?; + assert_eq!(0, result.exit_code); + + server_task.await? + } } diff --git a/codex-rs/shell-escalation/src/unix/escalation_policy.rs b/codex-rs/shell-escalation/src/unix/escalation_policy.rs index 28e04e6f590..c3da252dd9f 100644 --- a/codex-rs/shell-escalation/src/unix/escalation_policy.rs +++ b/codex-rs/shell-escalation/src/unix/escalation_policy.rs @@ -1,6 +1,6 @@ use codex_utils_absolute_path::AbsolutePathBuf; -use crate::unix::escalate_protocol::EscalateAction; +use crate::unix::escalate_protocol::EscalationDecision; /// Decides what action to take in response to an execve request from a client. #[async_trait::async_trait] @@ -10,5 +10,5 @@ pub trait EscalationPolicy: Send + Sync { file: &AbsolutePathBuf, argv: &[String], workdir: &AbsolutePathBuf, - ) -> anyhow::Result; + ) -> anyhow::Result; } diff --git a/codex-rs/shell-escalation/src/unix/mod.rs b/codex-rs/shell-escalation/src/unix/mod.rs index 37e29e87754..6de12297a46 100644 --- a/codex-rs/shell-escalation/src/unix/mod.rs +++ b/codex-rs/shell-escalation/src/unix/mod.rs @@ -63,10 +63,15 @@ pub mod stopwatch; pub use self::escalate_client::run_shell_escalation_execve_wrapper; pub use self::escalate_protocol::EscalateAction; +pub use self::escalate_protocol::EscalationDecision; +pub use self::escalate_protocol::EscalationExecution; pub use self::escalate_server::EscalateServer; pub use self::escalate_server::ExecParams; pub use self::escalate_server::ExecResult; +pub use self::escalate_server::PreparedExec; pub use self::escalate_server::ShellCommandExecutor; pub use self::escalation_policy::EscalationPolicy; pub use self::execve_wrapper::main_execve_wrapper; pub use self::stopwatch::Stopwatch; +pub use codex_protocol::approvals::EscalationPermissions; +pub use codex_protocol::approvals::Permissions;