diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index c888f9368d..ee024099c9 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 306ba49c10..5f5579cc83 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 288ee7f212..a184d81bc1 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -219,6 +219,7 @@ pub async fn process_exec_tool_call( enforce_managed_network, network: network.as_ref(), sandbox_policy_cwd: sandbox_cwd, + 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 56c7bff6a6..14813b0736 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,7 @@ use crate::tools::sandboxing::SandboxablePreference; use codex_network_proxy::NetworkProxy; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::FileSystemPermissions; +use codex_protocol::models::MacOsSeatbeltProfileExtensions; use codex_protocol::models::PermissionProfile; pub use codex_protocol::models::SandboxPermissions; use codex_protocol::protocol::ReadOnlyAccess; @@ -73,6 +74,7 @@ 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, + 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 +344,7 @@ impl SandboxManager { enforce_managed_network, network, sandbox_policy_cwd, + macos_seatbelt_profile_extensions, codex_linux_sandbox_exe, use_linux_sandbox_bwrap, windows_sandbox_level, @@ -370,12 +373,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 aacc3a7784..6b9fa68188 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 4019a31079..89c49709e3 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 066cb8258c..d4c9f510cd 100644 --- a/codex-rs/core/src/tools/js_repl/mod.rs +++ b/codex-rs/core/src/tools/js_repl/mod.rs @@ -616,6 +616,7 @@ impl JsReplManager { enforce_managed_network: has_managed_network_requirements, network: None, sandbox_policy_cwd: &turn.cwd, + 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 3b1a0c9067..3a16461b4a 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,14 @@ 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::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; @@ -94,6 +99,10 @@ pub(super) async fn try_run_zsh_fork( let exec_policy = Arc::new(RwLock::new( ctx.session.services.exec_policy.current().as_ref().clone(), )); + let escalation_permissions = CoreShellActionProvider::shell_request_escalation_permissions( + &sandbox_policy, + req.additional_permissions.as_ref(), + ); let command_executor = CoreShellCommandExecutor { command, cwd: sandbox_cwd, @@ -105,6 +114,9 @@ pub(super) async fn try_run_zsh_fork( sandbox_permissions, justification, arg0, + sandbox_policy_cwd: ctx.turn.cwd.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 +148,8 @@ 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(), + escalation_permissions, stopwatch: stopwatch.clone(), }; @@ -146,7 +160,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 +175,8 @@ struct CoreShellActionProvider { approval_policy: AskForApproval, sandbox_policy: SandboxPolicy, sandbox_permissions: SandboxPermissions, + prompt_permissions: Option, + escalation_permissions: Option, stopwatch: Stopwatch, } @@ -182,6 +198,40 @@ impl CoreShellActionProvider { }) } + fn shell_request_escalation_permissions( + sandbox_policy: &SandboxPolicy, + additional_permissions: Option<&PermissionProfile>, + ) -> Option { + additional_permissions.map(|_| { + // Shell request additional permissions were already normalized and + // merged into the first-attempt sandbox policy. + EscalationPermissions::Permissions(EscalatedPermissions { + sandbox_policy: sandbox_policy.clone(), + macos_seatbelt_profile_extensions: None, + }) + }) + } + + fn skill_escalation_permissions(skill: &SkillMetadata) -> Option { + skill + .permissions + .as_ref() + .map(|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) + }) + } + async fn prompt( &self, program: &AbsolutePathBuf, @@ -249,39 +299,32 @@ impl CoreShellActionProvider { program: &AbsolutePathBuf, argv: &[String], workdir: &AbsolutePathBuf, - additional_permissions: Option, + prompt_permissions: Option, + escalation_permissions: Option, 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( - program, - argv, - workdir, - &self.stopwatch, - additional_permissions, - ) + .prompt(program, argv, workdir, &self.stopwatch, prompt_permissions) .await? { ReviewDecision::Approved | ReviewDecision::ApprovedExecpolicyAmendment { .. } => { if needs_escalation { - EscalateAction::Escalate + EscalationDecision::escalate(escalation_permissions) } else { - EscalateAction::Run + EscalationDecision::run() } } ReviewDecision::ApprovedForSession => { @@ -306,9 +349,9 @@ impl CoreShellActionProvider { } if needs_escalation { - EscalateAction::Escalate + EscalationDecision::escalate(escalation_permissions) } else { - EscalateAction::Run + EscalationDecision::run() } } ReviewDecision::NetworkPolicyAmendment { @@ -316,29 +359,29 @@ impl CoreShellActionProvider { } => match network_policy_amendment.action { NetworkPolicyRuleAction::Allow => { if needs_escalation { - EscalateAction::Escalate + EscalationDecision::escalate(escalation_permissions) } 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_permissions) } else { - EscalateAction::Run + EscalationDecision::run() } } }; @@ -356,7 +399,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:?}" ); @@ -377,15 +420,12 @@ 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 permissions = approval .skill .as_ref() - .and_then(|s| s.permission_profile.clone()); + .and_then(Self::skill_escalation_permissions); - return Ok(EscalateAction::Escalate); + return Ok(EscalationDecision::escalate(permissions)); } // In the usual case, the execve wrapper reports the command being @@ -407,6 +447,7 @@ impl EscalationPolicy for CoreShellActionProvider { argv, workdir, skill.permission_profile.clone(), + Self::skill_escalation_permissions(&skill), decision_source, ) .await; @@ -453,7 +494,8 @@ impl EscalationPolicy for CoreShellActionProvider { program, argv, workdir, - None, + self.prompt_permissions.clone(), + self.escalation_permissions.clone(), decision_source, ) .await @@ -471,6 +513,9 @@ struct CoreShellCommandExecutor { sandbox_permissions: SandboxPermissions, justification: Option, arg0: Option, + sandbox_policy_cwd: PathBuf, + codex_linux_sandbox_exe: Option, + use_linux_sandbox_bwrap: bool, } #[async_trait::async_trait] @@ -516,6 +561,112 @@ impl ShellCommandExecutor for CoreShellCommandExecutor { timed_out: result.timed_out, }) } + + async fn prepare_escalated_exec( + &self, + program: &AbsolutePathBuf, + argv: &[String], + workdir: &AbsolutePathBuf, + env: HashMap, + permissions: Option, + ) -> 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 Some(permissions) = permissions else { + return Ok(PreparedExec { + command, + cwd: workdir.to_path_buf(), + env, + arg0: Some(first_arg.clone()), + }); + }; + + let prepared = match permissions { + EscalationPermissions::PermissionProfile(permission_profile) => self + .prepare_sandboxed_exec( + command, + workdir, + env, + &self.sandbox_policy, + Some(permission_profile), + None, + )?, + 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, + 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, + 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)] @@ -584,14 +735,46 @@ 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, + 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/skill_approval.rs b/codex-rs/core/tests/suite/skill_approval.rs index 8abd8f5924..72c6432727 100644 --- a/codex-rs/core/tests/suite/skill_approval.rs +++ b/codex-rs/core/tests/suite/skill_approval.rs @@ -76,6 +76,24 @@ async fn wait_for_turn_complete_without_skill_approval(test: &TestCodex) { } 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); @@ -93,13 +111,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)?; @@ -275,6 +287,207 @@ permissions: Ok(()) } +#[cfg(unix)] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn shell_zsh_fork_skill_session_approval_enforces_skill_permissions() -> Result<()> { + use codex_config::Constrained; + use codex_protocol::protocol::ReviewDecision; + + skip_if_no_network!(Ok(())); + + let Some(zsh_path) = find_test_zsh_path()? else { + return Ok(()); + }; + if !supports_exec_wrapper_intercept(&zsh_path) { + eprintln!( + "skipping zsh-fork skill permissions test: zsh does not support EXEC_WRAPPER intercepts ({})", + zsh_path.display() + ); + return Ok(()); + } + let Ok(main_execve_wrapper_exe) = codex_utils_cargo_bin::cargo_bin("codex-execve-wrapper") + else { + eprintln!( + "skipping zsh-fork skill permissions test: unable to resolve `codex-execve-wrapper` binary" + ); + return Ok(()); + }; + + let outside_dir = tempfile::tempdir()?; + let outside_path = outside_dir.path().join("zsh-fork-skill-permissions.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 server = start_mock_server().await; + let mut builder = test_codex() + .with_pre_build_hook(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(); + write_skill_metadata( + home, + "mbolin-test-skill", + r#" +permissions: + file_system: + write: + - "./output" +"#, + ) + .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::DangerFullAccess); + }); + let test = builder.build(&server).await?; + + let script_path = fs::canonicalize( + test.codex_home_path() + .join("skills/mbolin-test-skill/scripts/sandboxed.sh"), + )?; + let script_path_str = script_path.to_string_lossy().into_owned(); + let command = shlex::try_join([script_path_str.as_str()])?; + + 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, + SandboxPolicy::DangerFullAccess, + ) + .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 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![PathBuf::from("./output")]), + }), + ..Default::default() + }) + ); + + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::ApprovedForSession, + }) + .await?; + + wait_for_event(test.codex.as_ref(), |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .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("Permission denied") + || first_output.contains("Operation not permitted") + || first_output.contains("Read-only file system") + || !first_output.contains("forbidden"), + "expected skill sandbox denial on first run, got output: {first_output:?}" + ); + assert!( + !outside_path.exists(), + "first run should not write outside the approved skill 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, + SandboxPolicy::DangerFullAccess, + ) + .await?; + + let cached_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; + 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("Permission denied") + || second_output.contains("Operation not permitted") + || second_output.contains("Read-only file system") + || !second_output.contains("forbidden"), + "expected cached skill approval to retain sandboxing, got output: {second_output:?}" + ); + assert!( + !outside_path.exists(), + "cached session approval should not widen skill execution to full access" + ); + + Ok(()) +} + #[cfg(unix)] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn shell_zsh_fork_still_enforces_workspace_write_sandbox() -> Result<()> { diff --git a/codex-rs/protocol/src/approvals.rs b/codex-rs/protocol/src/approvals.rs index f742dc6328..c64aabdcd4 100644 --- a/codex-rs/protocol/src/approvals.rs +++ b/codex-rs/protocol/src/approvals.rs @@ -2,14 +2,29 @@ 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::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 ff5030c75e..d74401c609 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 85075cfb51..fbc5bcd8c7 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 48bc116581..3273c6fd74 100644 --- a/codex-rs/shell-escalation/src/lib.rs +++ b/codex-rs/shell-escalation/src/lib.rs @@ -6,12 +6,20 @@ pub use unix::EscalateAction; #[cfg(unix)] pub use unix::EscalateServer; #[cfg(unix)] +pub use unix::EscalationDecision; +#[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 6002fdc57b..a21b2e9c1e 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,35 @@ pub struct EscalateResponse { pub action: EscalateAction, } +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct EscalationDecision { + pub action: EscalateAction, + pub permissions: Option, +} + +impl EscalationDecision { + pub fn run() -> Self { + Self { + action: EscalateAction::Run, + permissions: None, + } + } + + pub fn escalate(permissions: Option) -> Self { + Self { + action: EscalateAction::Escalate, + permissions, + } + } + + pub fn deny(reason: Option) -> Self { + Self { + action: EscalateAction::Deny { reason }, + permissions: None, + } + } +} + #[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 2951f584a6..bc67d0a8b1 100644 --- a/codex-rs/shell-escalation/src/unix/escalate_server.rs +++ b/codex-rs/shell-escalation/src/unix/escalate_server.rs @@ -6,6 +6,7 @@ use std::sync::Arc; use std::time::Duration; use anyhow::Context as _; +use codex_protocol::approvals::EscalationPermissions; use codex_utils_absolute_path::AbsolutePathBuf; use tokio::process::Command; use tokio_util::sync::CancellationToken; @@ -15,6 +16,7 @@ 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::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, + permissions: Option, + ) -> 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,7 +183,10 @@ 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 EscalationDecision { + action, + permissions, + } = policy .determine_action(&program, &argv, &workdir) .await .context("failed to determine escalation action")?; @@ -197,12 +229,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, permissions) + .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()); @@ -236,13 +279,14 @@ async fn handle_escalate_session_with_policy( #[cfg(test)] mod tests { use super::*; + 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 +296,8 @@ mod tests { _file: &AbsolutePathBuf, _argv: &[String], _workdir: &AbsolutePathBuf, - ) -> anyhow::Result { - Ok(self.action.clone()) + ) -> anyhow::Result { + Ok(self.decision.clone()) } } @@ -269,10 +313,79 @@ 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, + _permissions: Option, + ) -> 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, + permissions: Option, + ) -> anyhow::Result { + assert_eq!(permissions, Some(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 +395,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 +440,7 @@ mod tests { expected_file, expected_workdir: workdir.clone(), }), + Arc::new(ForwardingShellCommandExecutor), )); client @@ -353,8 +468,9 @@ mod tests { let server_task = tokio::spawn(handle_escalate_session_with_policy( server, Arc::new(DeterministicEscalationPolicy { - action: EscalateAction::Escalate, + decision: EscalationDecision::escalate(None), }), + Arc::new(ForwardingShellCommandExecutor), )); client @@ -387,4 +503,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(Some( + 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 28e04e6f59..c3da252dd9 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 37e29e8775..6cd95733d4 100644 --- a/codex-rs/shell-escalation/src/unix/mod.rs +++ b/codex-rs/shell-escalation/src/unix/mod.rs @@ -63,10 +63,14 @@ 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_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;