From bc10e5b3900b26c638aa44c2a9bc15950be733ae Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 28 May 2026 14:10:40 -0700 Subject: [PATCH] runtime: prepend zsh fork bin dir to PATH (#23768) ## Why #23756 makes packaged Codex builds include and default to the bundled zsh fork. The important reason to put that fork's directory at the front of `PATH` is to keep executable-level escalation working after a command leaves the original shell and later re-enters zsh through `env`. The expected chain is: 1. The zsh fork runs the top-level shell command. 2. That command launches another program, such as `python3`, while inheriting the `EXEC_WRAPPER` environment and the escalation socket fd. 3. That program spawns a shell script whose shebang is `#!/usr/bin/env zsh` rather than `#!/bin/zsh`, and it does not close the escalation fd. 4. `/usr/bin/env` resolves `zsh` through `PATH`, so it must find the packaged zsh fork before the system zsh. 5. Commands inside that nested script are intercepted by the zsh fork and can still request escalation from Codex. If `PATH` resolves `zsh` to the system shell instead, the nested script loses zsh-fork exec interception. Commands that should request escalation can then run only in the original sandbox, or fail there, without Codex ever receiving the approval request. Shell snapshots make this slightly more subtle: a snapshot can restore an older `PATH` after the child shell starts. This PR treats the zsh fork `PATH` prepend as an explicit environment override so snapshot wrapping preserves it. ## What Changed - Added shared zsh-fork runtime helpers that prepend the configured zsh executable parent directory to `PATH` without duplicate entries. - Applied the zsh fork `PATH` prepend to both zsh-fork `shell_command` launches and unified-exec zsh-fork launches before sandbox command construction. - Kept the shell-command zsh-fork backend API narrow: it derives the configured zsh path from session services and rebuilds its sandbox environment from `req.env`, rather than accepting a second, competing environment map or a separately threaded bin dir. - Kept Unix-only zsh-fork `PATH` mutation out of Windows clippy-visible mutability. - Added coverage for duplicate `PATH` entries, for preserving the zsh fork prepend through shell snapshot wrapping, and for the nested `python3` -> `#!/usr/bin/env zsh` escalation flow. ## Testing - `just fmt` - `just fix -p codex-core` I left final test validation to CI after the latest review-comment cleanup. Before that cleanup, `just test -p codex-core zsh_fork` passed locally for the zsh-fork-focused tests. --- codex-rs/core/src/tools/runtimes/mod.rs | 40 +++++ codex-rs/core/src/tools/runtimes/mod_tests.rs | 93 ++++++++++ codex-rs/core/src/tools/runtimes/shell.rs | 16 +- .../tools/runtimes/shell/unix_escalation.rs | 4 +- .../core/src/tools/runtimes/unified_exec.rs | 17 +- codex-rs/core/tests/suite/approvals.rs | 162 ++++++++++++++++++ 6 files changed, 329 insertions(+), 3 deletions(-) diff --git a/codex-rs/core/src/tools/runtimes/mod.rs b/codex-rs/core/src/tools/runtimes/mod.rs index bba1c572e..45e1754da 100644 --- a/codex-rs/core/src/tools/runtimes/mod.rs +++ b/codex-rs/core/src/tools/runtimes/mod.rs @@ -22,6 +22,8 @@ use codex_sandboxing::SandboxCommand; use codex_sandboxing::SandboxType; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashMap; +#[cfg(unix)] +use std::path::Path; pub(crate) mod apply_patch; pub(crate) mod shell; @@ -70,6 +72,44 @@ pub(crate) fn exec_env_for_sandbox_permissions( env } +#[cfg(unix)] +fn prepend_path_entry(env: &mut HashMap, path_entry: &str) -> String { + let updated_path = match env.get("PATH") { + Some(path) if !path.is_empty() => std::iter::once(path_entry) + .chain(path.split(':').filter(|entry| *entry != path_entry)) + .collect::>() + .join(":"), + _ => path_entry.to_string(), + }; + env.insert("PATH".to_string(), updated_path.clone()); + updated_path +} + +#[cfg(unix)] +pub(crate) fn prepend_zsh_fork_bin_to_path( + env: &mut HashMap, + shell_zsh_path: &Path, +) -> Option { + let zsh_bin_dir = shell_zsh_path + .parent() + .map(|path| path.to_string_lossy().to_string())?; + Some(prepend_path_entry(env, &zsh_bin_dir)) +} + +#[cfg(unix)] +pub(crate) fn apply_zsh_fork_path_prepend( + env: &mut HashMap, + explicit_env_overrides: &mut HashMap, + shell_zsh_path: &Path, +) { + let Some(updated_path) = prepend_zsh_fork_bin_to_path(env, shell_zsh_path) else { + return; + }; + // Snapshot wrapping restores explicit overrides after sourcing the shell + // snapshot, so capture this PATH override there as well. + explicit_env_overrides.insert("PATH".to_string(), updated_path); +} + pub(crate) fn disable_powershell_profile_for_elevated_windows_sandbox( command: &[String], shell_type: Option<&ShellType>, diff --git a/codex-rs/core/src/tools/runtimes/mod_tests.rs b/codex-rs/core/src/tools/runtimes/mod_tests.rs index fd10f2224..539bbb90d 100644 --- a/codex-rs/core/src/tools/runtimes/mod_tests.rs +++ b/codex-rs/core/src/tools/runtimes/mod_tests.rs @@ -143,6 +143,48 @@ async fn explicit_escalation_prepares_exec_without_managed_network() -> anyhow:: Ok(()) } +#[cfg(unix)] +#[test] +fn apply_zsh_fork_path_prepend_uses_shell_parent() { + let mut env = HashMap::from([("PATH".to_string(), "/usr/bin:/bin".to_string())]); + let mut explicit_env_overrides = HashMap::new(); + + apply_zsh_fork_path_prepend( + &mut env, + &mut explicit_env_overrides, + PathBuf::from("/package/codex-resources/zsh/bin/zsh").as_path(), + ); + + let expected = "/package/codex-resources/zsh/bin:/usr/bin:/bin"; + assert_eq!(env.get("PATH").map(String::as_str), Some(expected)); + assert_eq!( + explicit_env_overrides.get("PATH").map(String::as_str), + Some(expected) + ); +} + +#[cfg(unix)] +#[test] +fn apply_zsh_fork_path_prepend_moves_existing_shell_parent_to_front() { + let mut env = HashMap::from([( + "PATH".to_string(), + "/usr/bin:/package/codex-resources/zsh/bin:/bin:/package/codex-resources/zsh/bin" + .to_string(), + )]); + let mut explicit_env_overrides = HashMap::new(); + + apply_zsh_fork_path_prepend( + &mut env, + &mut explicit_env_overrides, + PathBuf::from("/package/codex-resources/zsh/bin/zsh").as_path(), + ); + + assert_eq!( + env.get("PATH").map(String::as_str), + Some("/package/codex-resources/zsh/bin:/usr/bin:/bin") + ); +} + #[test] fn explicit_escalation_keeps_user_proxy_env_without_codex_marker() { let env = HashMap::from([ @@ -818,6 +860,57 @@ fn maybe_wrap_shell_lc_with_snapshot_applies_explicit_path_override() { assert_eq!(String::from_utf8_lossy(&output.stdout), "/worktree/bin"); } +#[cfg(unix)] +#[test] +fn maybe_wrap_shell_lc_with_snapshot_preserves_zsh_fork_path_prepend() { + let dir = tempdir().expect("create temp dir"); + let snapshot_path = dir.path().join("snapshot.sh"); + std::fs::write( + &snapshot_path, + "# Snapshot file\nexport PATH='/snapshot/bin'\n", + ) + .expect("write snapshot"); + let session_shell = shell_with_snapshot( + ShellType::Bash, + "/bin/bash", + snapshot_path.abs(), + dir.path().abs(), + ); + let command = vec![ + "/bin/bash".to_string(), + "-lc".to_string(), + "printf '%s' \"$PATH\"".to_string(), + ]; + let zsh_path = dir + .path() + .join("codex-resources") + .join("zsh") + .join("bin") + .join("zsh"); + let zsh_bin_dir = zsh_path.parent().expect("zsh path should have parent"); + let mut env = HashMap::from([("PATH".to_string(), "/worktree/bin".to_string())]); + let mut explicit_env_overrides = HashMap::new(); + apply_zsh_fork_path_prepend(&mut env, &mut explicit_env_overrides, zsh_path.as_path()); + let rewritten = maybe_wrap_shell_lc_with_snapshot( + &command, + &session_shell, + &dir.path().abs(), + &explicit_env_overrides, + &env, + ); + let output = Command::new(&rewritten[0]) + .args(&rewritten[1..]) + .env("PATH", env.get("PATH").expect("PATH should be set")) + .output() + .expect("run rewritten command"); + + assert!(output.status.success(), "command failed: {output:?}"); + assert_eq!( + String::from_utf8_lossy(&output.stdout), + format!("{}:/worktree/bin", zsh_bin_dir.display()) + ); +} + #[test] fn maybe_wrap_shell_lc_with_snapshot_does_not_embed_override_values_in_argv() { let dir = tempdir().expect("create temp dir"); diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index df63f6cd4..8b8fe0450 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -20,6 +20,8 @@ use crate::shell::ShellType; use crate::tools::flat_tool_name; use crate::tools::network_approval::NetworkApprovalMode; use crate::tools::network_approval::NetworkApprovalSpec; +#[cfg(unix)] +use crate::tools::runtimes::apply_zsh_fork_path_prepend; use crate::tools::runtimes::build_sandbox_command; use crate::tools::runtimes::disable_powershell_profile_for_elevated_windows_sandbox; use crate::tools::runtimes::exec_env_for_sandbox_permissions; @@ -234,11 +236,23 @@ impl ToolRuntime for ShellRuntime { let managed_network = managed_network_for_sandbox_permissions(req.network.as_ref(), req.sandbox_permissions); let env = exec_env_for_sandbox_permissions(&req.env, req.sandbox_permissions); + let explicit_env_overrides = req.explicit_env_overrides.clone(); + #[cfg(unix)] + let (env, explicit_env_overrides) = { + let mut env = env; + let mut explicit_env_overrides = explicit_env_overrides; + if self.backend == ShellRuntimeBackend::ShellCommandZshFork + && let Some(shell_zsh_path) = ctx.session.services.shell_zsh_path.as_deref() + { + apply_zsh_fork_path_prepend(&mut env, &mut explicit_env_overrides, shell_zsh_path); + } + (env, explicit_env_overrides) + }; let command = maybe_wrap_shell_lc_with_snapshot( &req.command, session_shell.as_ref(), &req.cwd, - &req.explicit_env_overrides, + &explicit_env_overrides, &env, ); let command = disable_powershell_profile_for_elevated_windows_sandbox( 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 fef8db5ca..998a1c02f 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -16,6 +16,7 @@ use crate::sandboxing::SandboxPermissions; use crate::shell::ShellType; use crate::tools::runtimes::build_sandbox_command; use crate::tools::runtimes::exec_env_for_sandbox_permissions; +use crate::tools::runtimes::prepend_zsh_fork_bin_to_path; use crate::tools::sandboxing::PermissionRequestPayload; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::ToolCtx; @@ -116,7 +117,8 @@ pub(super) async fn try_run_zsh_fork( return Ok(None); } - let env = exec_env_for_sandbox_permissions(&req.env, req.sandbox_permissions); + let mut env = exec_env_for_sandbox_permissions(&req.env, req.sandbox_permissions); + prepend_zsh_fork_bin_to_path(&mut env, shell_zsh_path); let command = build_sandbox_command(command, &req.cwd, &env, req.additional_permissions.clone())?; let options = ExecOptions { diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index c613f198e..01054a036 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -17,6 +17,8 @@ use crate::shell::ShellType; use crate::tools::flat_tool_name; use crate::tools::network_approval::NetworkApprovalMode; use crate::tools::network_approval::NetworkApprovalSpec; +#[cfg(unix)] +use crate::tools::runtimes::apply_zsh_fork_path_prepend; use crate::tools::runtimes::build_sandbox_command; use crate::tools::runtimes::disable_powershell_profile_for_elevated_windows_sandbox; use crate::tools::runtimes::exec_env_for_sandbox_permissions; @@ -261,6 +263,19 @@ impl<'a> ToolRuntime for UnifiedExecRunt if let Some(network) = managed_network { network.apply_to_env(&mut env); } + let explicit_env_overrides = req.explicit_env_overrides.clone(); + #[cfg(unix)] + let explicit_env_overrides = { + let mut explicit_env_overrides = explicit_env_overrides; + if let UnifiedExecShellMode::ZshFork(zsh_fork_config) = &self.shell_mode { + apply_zsh_fork_path_prepend( + &mut env, + &mut explicit_env_overrides, + zsh_fork_config.shell_zsh_path.as_path(), + ); + } + explicit_env_overrides + }; let environment_is_remote = req.environment.is_remote(); let command = if environment_is_remote { base_command.to_vec() @@ -269,7 +284,7 @@ impl<'a> ToolRuntime for UnifiedExecRunt base_command, session_shell.as_ref(), &req.cwd, - &req.explicit_env_overrides, + &explicit_env_overrides, &env, ) }; diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index e16a4a14a..2a5f8fc43 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -46,6 +46,8 @@ use serde_json::Value; use serde_json::json; use std::env; use std::fs; +#[cfg(unix)] +use std::os::unix::fs::PermissionsExt; use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; @@ -2528,6 +2530,166 @@ async fn spawned_subagent_execpolicy_amendment_propagates_to_parent_session() -> Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[cfg(unix)] +async fn env_zsh_script_spawned_by_python_can_request_escalation_under_zsh_fork() -> Result<()> { + skip_if_no_network!(Ok(())); + + let Some(runtime) = zsh_fork_runtime("zsh-fork env zsh nested escalation test")? else { + return Ok(()); + }; + + let approval_policy = AskForApproval::OnRequest; + let permission_profile = restrictive_workspace_write_profile(); + let outside_dir = tempfile::tempdir_in(std::env::current_dir()?)?; + let outside_path = outside_dir.path().join("zsh-fork-env-zsh-escalated.txt"); + let outside_path_arg = shlex::try_join([outside_path.to_string_lossy().as_ref()])?; + let rules = r#"prefix_rule(pattern=["touch"], decision="prompt")"#.to_string(); + + let server = start_mock_server().await; + let outside_path_for_hook = outside_path.clone(); + let test = build_zsh_fork_test( + &server, + runtime, + approval_policy, + permission_profile.clone(), + move |home| { + let _ = fs::remove_file(&outside_path_for_hook); + let rules_dir = home.join("rules"); + fs::create_dir_all(&rules_dir).unwrap(); + fs::write(rules_dir.join("default.rules"), &rules).unwrap(); + }, + ) + .await?; + + let script_path = test.cwd.path().join("runs-under-env-zsh"); + fs::write( + &script_path, + format!( + "#!/usr/bin/env zsh\ntouch {outside_path_arg}\nprint -r -- nested-env-zsh-complete\n" + ), + )?; + let mut script_permissions = fs::metadata(&script_path)?.permissions(); + script_permissions.set_mode(0o755); + fs::set_permissions(&script_path, script_permissions)?; + + let script_literal = serde_json::to_string(script_path.to_string_lossy().as_ref())?; + let python_script = format!( + "import subprocess; subprocess.run([{script_literal}], check=True, close_fds=False)" + ); + let command = shlex::try_join(["python3", "-c", python_script.as_str()])?; + + let call_id = "zsh-fork-env-zsh-nested-escalation"; + let event = shell_event( + call_id, + &command, + /*timeout_ms*/ 30_000, + SandboxPermissions::UseDefault, + )?; + let _ = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-zsh-fork-env-zsh-1"), + event, + ev_completed("resp-zsh-fork-env-zsh-1"), + ]), + ) + .await; + let results = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-zsh-fork-env-zsh-1", "done"), + ev_completed("resp-zsh-fork-env-zsh-2"), + ]), + ) + .await; + + let session_model = test.session_configured.model.clone(); + let (sandbox_policy, permission_profile) = + turn_permission_fields(permission_profile, test.cwd.path()); + test.codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "run nested env zsh script through python".into(), + text_elements: Vec::new(), + }], + environments: None, + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: codex_protocol::protocol::ThreadSettingsOverrides { + cwd: Some(test.cwd.path().to_path_buf()), + approval_policy: Some(approval_policy), + approvals_reviewer: Some(ApprovalsReviewer::User), + sandbox_policy: Some(sandbox_policy), + permission_profile, + collaboration_mode: Some(codex_protocol::config_types::CollaborationMode { + mode: codex_protocol::config_types::ModeKind::Default, + settings: codex_protocol::config_types::Settings { + model: session_model, + reasoning_effort: None, + developer_instructions: None, + }, + }), + ..Default::default() + }, + }) + .await?; + + let approval_event = wait_for_event_with_timeout( + &test.codex, + |event| { + matches!( + event, + EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_) + ) + }, + Duration::from_secs(10), + ) + .await; + let EventMsg::ExecApprovalRequest(approval) = approval_event else { + panic!("expected nested zsh script to request approval before completion"); + }; + assert!( + approval.command.iter().any(|arg| arg.ends_with("/touch")) + && approval + .command + .iter() + .any(|arg| arg == outside_path.to_string_lossy().as_ref()), + "expected approval for nested touch command, got: {:?}", + approval.command + ); + + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::Approved, + }) + .await?; + + wait_for_completion(&test).await; + + let result = parse_result(&results.single_request().function_call_output(call_id)); + assert_eq!( + result.exit_code.unwrap_or(0), + 0, + "nested env zsh script should complete successfully: {}", + result.stdout + ); + assert!( + result.stdout.contains("nested-env-zsh-complete"), + "nested script did not report completion: {}", + result.stdout + ); + assert!( + outside_path.exists(), + "approved nested touch should create the out-of-workspace file" + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[cfg(unix)] async fn matched_prefix_rule_runs_unsandboxed_under_zsh_fork() -> Result<()> {