Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 76 additions & 24 deletions codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,9 @@ pub(super) async fn try_run_zsh_fork(
let sandbox_exec_request = attempt
.env_for(spec, req.network.as_ref())
.map_err(|err| ToolError::Codex(err.into()))?;
// Keep env/network/sandbox metadata from `attempt.env_for()`, but build the
// script from the original shell argv. `attempt.env_for()` may wrap the
// command with `sandbox-exec` on macOS, and passing those wrapper flags
// (`-p`, `-D...`) through zsh breaks the zsh-fork path before subcommand
// approval runs.
let crate::sandboxing::ExecRequest {
command: _sandbox_command,
cwd: _sandbox_cwd,
command,
cwd: sandbox_cwd,
env: sandbox_env,
network: sandbox_network,
expiration: _sandbox_expiration,
Expand All @@ -90,7 +85,7 @@ pub(super) async fn try_run_zsh_fork(
justification,
arg0,
} = sandbox_exec_request;
let ParsedShellCommand { script, login } = extract_shell_script(command)?;
let ParsedShellCommand { script, login } = extract_shell_script(&command)?;
let effective_timeout = Duration::from_millis(
req.timeout_ms
.unwrap_or(crate::exec::DEFAULT_EXEC_COMMAND_TIMEOUT_MS),
Expand All @@ -99,6 +94,8 @@ pub(super) async fn try_run_zsh_fork(
ctx.session.services.exec_policy.current().as_ref().clone(),
));
let command_executor = CoreShellCommandExecutor {
command,
cwd: sandbox_cwd,
sandbox_policy,
sandbox,
env: sandbox_env,
Expand Down Expand Up @@ -438,6 +435,8 @@ impl EscalationPolicy for CoreShellActionProvider {
}

struct CoreShellCommandExecutor {
command: Vec<String>,
cwd: PathBuf,
sandbox_policy: SandboxPolicy,
sandbox: SandboxType,
env: HashMap<String, String>,
Expand All @@ -452,8 +451,8 @@ struct CoreShellCommandExecutor {
impl ShellCommandExecutor for CoreShellCommandExecutor {
async fn run(
&self,
command: Vec<String>,
cwd: PathBuf,
_command: Vec<String>,
_cwd: PathBuf,
env: HashMap<String, String>,
cancel_rx: CancellationToken,
) -> anyhow::Result<ExecResult> {
Expand All @@ -466,8 +465,8 @@ impl ShellCommandExecutor for CoreShellCommandExecutor {

let result = crate::sandboxing::execute_env(
crate::sandboxing::ExecRequest {
command,
cwd,
command: self.command.clone(),
cwd: self.cwd.clone(),
env: exec_env,
network: self.network.clone(),
expiration: ExecExpiration::Cancellation(cancel_rx),
Expand Down Expand Up @@ -500,19 +499,20 @@ struct ParsedShellCommand {
}

fn extract_shell_script(command: &[String]) -> Result<ParsedShellCommand, ToolError> {
match command {
[_, flag, script, ..] if flag == "-c" => Ok(ParsedShellCommand {
script: script.clone(),
login: false,
}),
[_, flag, script, ..] if flag == "-lc" => Ok(ParsedShellCommand {
script: script.clone(),
login: true,
}),
_ => Err(ToolError::Rejected(
"unexpected shell command format for zsh-fork execution".to_string(),
)),
// Commands reaching zsh-fork can be wrapped by environment/sandbox helpers, so
// we search for the first `-c`/`-lc` triple anywhere in the argv rather
// than assuming it is the first positional form.
if let Some((script, login)) = command.windows(3).find_map(|parts| match parts {
[_, flag, script] if flag == "-c" => Some((script.to_owned(), false)),
[_, flag, script] if flag == "-lc" => Some((script.to_owned(), true)),
_ => None,
}) {
return Ok(ParsedShellCommand { script, login });
}

Err(ToolError::Rejected(
"unexpected shell command format for zsh-fork execution".to_string(),
))
}

fn map_exec_result(
Expand Down Expand Up @@ -586,6 +586,58 @@ mod tests {
);
}

#[test]
fn extract_shell_script_supports_wrapped_command_prefixes() {
assert_eq!(
extract_shell_script(&[
"/usr/bin/env".into(),
"CODEX_EXECVE_WRAPPER=1".into(),
"/bin/zsh".into(),
"-lc".into(),
"echo hello".into()
])
.unwrap(),
ParsedShellCommand {
script: "echo hello".to_string(),
login: true,
}
);

assert_eq!(
extract_shell_script(&[
"sandbox-exec".into(),
"-p".into(),
"sandbox_policy".into(),
"/bin/zsh".into(),
"-c".into(),
"pwd".into(),
])
.unwrap(),
ParsedShellCommand {
script: "pwd".to_string(),
login: false,
}
);
}

#[test]
fn extract_shell_script_rejects_unsupported_shell_invocation() {
let err = extract_shell_script(&[
"sandbox-exec".into(),
"-fc".into(),
"echo not supported".into(),
])
.unwrap_err();
assert!(matches!(err, super::ToolError::Rejected(_)));
assert_eq!(
match err {
super::ToolError::Rejected(reason) => reason,
_ => "".to_string(),
},
"unexpected shell command format for zsh-fork execution"
);
}

#[test]
fn join_program_and_argv_replaces_original_argv_zero() {
assert_eq!(
Expand Down
88 changes: 88 additions & 0 deletions codex-rs/core/tests/suite/skill_approval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -558,3 +558,91 @@ permissions:

Ok(())
}

#[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;

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 sandbox 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 sandbox test: unable to resolve `codex-execve-wrapper` binary"
);
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 _ = fs::remove_file(outside_path);
let mut builder = test_codex()
.with_pre_build_hook(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?;

let command = format!("touch {outside_path}");
let arguments = shell_command_arguments(&command)?;
let mocks =
mount_function_call_agent_response(&server, tool_call_id, &arguments, "shell_command")
.await;

submit_turn_with_policies(
&test,
"write outside workspace with zsh fork",
AskForApproval::Never,
workspace_write_policy,
)
.await?;

wait_for_turn_complete_without_skill_approval(&test).await;

let call_output = mocks
.completion
.single_request()
.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"),
"expected sandbox denial, got output: {output:?}"
);
assert!(
!Path::new(outside_path).exists(),
"command should not write outside workspace under WorkspaceWrite policy"
);

Ok(())
}
Loading