mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
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.
This commit is contained in:
@@ -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<String, String>, 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::<Vec<_>>()
|
||||
.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<String, String>,
|
||||
shell_zsh_path: &Path,
|
||||
) -> Option<String> {
|
||||
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<String, String>,
|
||||
explicit_env_overrides: &mut HashMap<String, String>,
|
||||
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>,
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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<ShellRequest, ExecToolCallOutput> 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(
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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<UnifiedExecRequest, UnifiedExecProcess> 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<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
|
||||
base_command,
|
||||
session_shell.as_ref(),
|
||||
&req.cwd,
|
||||
&req.explicit_env_overrides,
|
||||
&explicit_env_overrides,
|
||||
&env,
|
||||
)
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user