diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 91932b30f..6b57b020f 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2241,8 +2241,8 @@ dependencies = [ "anyhow", "async-trait", "clap", + "codex-utils-absolute-path", "libc", - "path-absolutize", "pretty_assertions", "serde", "serde_json", 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 796506e13..1b8e79f22 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -31,8 +31,8 @@ use codex_shell_escalation::ExecParams; use codex_shell_escalation::ExecResult; use codex_shell_escalation::ShellCommandExecutor; use codex_shell_escalation::Stopwatch; +use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashMap; -use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; @@ -175,9 +175,9 @@ impl CoreShellActionProvider { async fn prompt( &self, - program: &Path, + program: &AbsolutePathBuf, argv: &[String], - workdir: &Path, + workdir: &AbsolutePathBuf, stopwatch: &Stopwatch, ) -> anyhow::Result { let command = join_program_and_argv(program, argv); @@ -208,7 +208,7 @@ impl CoreShellActionProvider { /// Because we should be intercepting execve(2) calls, `program` should be /// an absolute path. The idea is that we check to see whether it matches /// any skills. - async fn find_skill(&self, program: &Path) -> Option { + async fn find_skill(&self, program: &AbsolutePathBuf) -> Option { let force_reload = false; let skills_outcome = self .session @@ -217,12 +217,13 @@ impl CoreShellActionProvider { .skills_for_cwd(&self.turn.cwd, force_reload) .await; + let program_path = program.as_path(); for skill in skills_outcome.skills { // We intentionally ignore "enabled" status here for now. let Some(skill_root) = skill.path_to_skills_md.parent() else { continue; }; - if program.starts_with(skill_root.join("scripts")) { + if program_path.starts_with(skill_root.join("scripts")) { return Some(skill); } } @@ -234,9 +235,9 @@ impl CoreShellActionProvider { &self, decision: Decision, needs_escalation: bool, - program: &Path, + program: &AbsolutePathBuf, argv: &[String], - workdir: &Path, + workdir: &AbsolutePathBuf, ) -> anyhow::Result { let action = match decision { Decision::Forbidden => EscalateAction::Deny { @@ -304,9 +305,9 @@ impl CoreShellActionProvider { impl EscalationPolicy for CoreShellActionProvider { async fn determine_action( &self, - program: &Path, + program: &AbsolutePathBuf, argv: &[String], - workdir: &Path, + workdir: &AbsolutePathBuf, ) -> anyhow::Result { tracing::debug!( "Determining escalation action for command {program:?} with args {argv:?} in {workdir:?}" @@ -483,7 +484,7 @@ fn map_exec_result( /// The intercepted `argv` includes `argv[0]`, but once we have normalized the /// executable path in `program`, we should replace the original `argv[0]` /// rather than duplicating it as an apparent user argument. -fn join_program_and_argv(program: &Path, argv: &[String]) -> Vec { +fn join_program_and_argv(program: &AbsolutePathBuf, argv: &[String]) -> Vec { std::iter::once(program.to_string_lossy().to_string()) .chain(argv.iter().skip(1).cloned()) .collect::>() @@ -497,8 +498,8 @@ mod tests { use super::map_exec_result; use crate::exec::SandboxType; use codex_shell_escalation::ExecResult; + use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; - use std::path::Path; use std::time::Duration; #[test] @@ -523,13 +524,16 @@ mod tests { fn join_program_and_argv_replaces_original_argv_zero() { assert_eq!( join_program_and_argv( - Path::new("/tmp/tool"), + &AbsolutePathBuf::from_absolute_path("/tmp/tool").unwrap(), &["./tool".into(), "--flag".into(), "value".into()], ), vec!["/tmp/tool", "--flag", "value"] ); assert_eq!( - join_program_and_argv(Path::new("/tmp/tool"), &["./tool".into()]), + join_program_and_argv( + &AbsolutePathBuf::from_absolute_path("/tmp/tool").unwrap(), + &["./tool".into()] + ), vec!["/tmp/tool"] ); } diff --git a/codex-rs/shell-escalation/Cargo.toml b/codex-rs/shell-escalation/Cargo.toml index f608b9608..85075cfb5 100644 --- a/codex-rs/shell-escalation/Cargo.toml +++ b/codex-rs/shell-escalation/Cargo.toml @@ -12,8 +12,8 @@ path = "src/bin/main_execve_wrapper.rs" anyhow = { workspace = true } async-trait = { workspace = true } clap = { workspace = true, features = ["derive"] } +codex-utils-absolute-path = { workspace = true } libc = { workspace = true } -path-absolutize = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } socket2 = { workspace = true, features = ["all"] } diff --git a/codex-rs/shell-escalation/src/unix/escalate_client.rs b/codex-rs/shell-escalation/src/unix/escalate_client.rs index 5c363f7d6..f9c866df4 100644 --- a/codex-rs/shell-escalation/src/unix/escalate_client.rs +++ b/codex-rs/shell-escalation/src/unix/escalate_client.rs @@ -4,6 +4,7 @@ use std::os::fd::FromRawFd as _; use std::os::fd::OwnedFd; use anyhow::Context as _; +use codex_utils_absolute_path::AbsolutePathBuf; use crate::unix::escalate_protocol::ESCALATE_SOCKET_ENV_VAR; use crate::unix::escalate_protocol::EXEC_WRAPPER_ENV_VAR; @@ -50,7 +51,7 @@ pub async fn run_shell_escalation_execve_wrapper( .send(EscalateRequest { file: file.clone().into(), argv: argv.clone(), - workdir: std::env::current_dir()?, + workdir: AbsolutePathBuf::current_dir()?, env, }) .await diff --git a/codex-rs/shell-escalation/src/unix/escalate_protocol.rs b/codex-rs/shell-escalation/src/unix/escalate_protocol.rs index 6be00fe4b..6002fdc57 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_utils_absolute_path::AbsolutePathBuf; use serde::Deserialize; use serde::Serialize; @@ -17,11 +18,14 @@ pub const LEGACY_BASH_EXEC_WRAPPER_ENV_VAR: &str = "BASH_EXEC_WRAPPER"; /// The client sends this to the server to request an exec() call. #[derive(Clone, Serialize, Deserialize, Debug, PartialEq, Eq)] pub struct EscalateRequest { - /// The absolute path to the executable to run, i.e. the first arg to exec. + /// The executable path from the intercepted exec call. + /// + /// This may be relative, in which case it should be resolved against + /// `workdir`. pub file: PathBuf, /// The argv, including the program name (argv[0]). pub argv: Vec, - pub workdir: PathBuf, + pub workdir: AbsolutePathBuf, pub env: HashMap, } diff --git a/codex-rs/shell-escalation/src/unix/escalate_server.rs b/codex-rs/shell-escalation/src/unix/escalate_server.rs index 56db822eb..2951f584a 100644 --- a/codex-rs/shell-escalation/src/unix/escalate_server.rs +++ b/codex-rs/shell-escalation/src/unix/escalate_server.rs @@ -6,7 +6,7 @@ use std::sync::Arc; use std::time::Duration; use anyhow::Context as _; -use path_absolutize::Absolutize as _; +use codex_utils_absolute_path::AbsolutePathBuf; use tokio::process::Command; use tokio_util::sync::CancellationToken; @@ -114,8 +114,9 @@ impl EscalateServer { }, params.command, ]; + let workdir = AbsolutePathBuf::try_from(params.workdir)?; let result = command_executor - .run(command, PathBuf::from(¶ms.workdir), env, cancel_rx) + .run(command, workdir.to_path_buf(), env, cancel_rx) .await?; escalate_task.abort(); Ok(result) @@ -152,14 +153,13 @@ async fn handle_escalate_session_with_policy( workdir, env, } = socket.receive::().await?; - let file = PathBuf::from(&file).absolutize()?.into_owned(); - let workdir = PathBuf::from(&workdir).absolutize()?.into_owned(); + let program = AbsolutePathBuf::resolve_path_against_base(file, workdir.as_path())?; let action = policy - .determine_action(file.as_path(), &argv, &workdir) + .determine_action(&program, &argv, &workdir) .await .context("failed to determine escalation action")?; - tracing::debug!("decided {action:?} for {file:?} {argv:?} {workdir:?}"); + tracing::debug!("decided {action:?} for {program:?} {argv:?} {workdir:?}"); match action { EscalateAction::Run => { @@ -197,7 +197,7 @@ async fn handle_escalate_session_with_policy( )); } - let mut command = Command::new(file); + let mut command = Command::new(program.as_path()); command .args(&argv[1..]) .arg0(argv[0].clone()) @@ -236,9 +236,9 @@ async fn handle_escalate_session_with_policy( #[cfg(test)] mod tests { use super::*; + use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use std::collections::HashMap; - use std::path::Path; use std::path::PathBuf; struct DeterministicEscalationPolicy { @@ -249,14 +249,33 @@ mod tests { impl EscalationPolicy for DeterministicEscalationPolicy { async fn determine_action( &self, - _file: &Path, + _file: &AbsolutePathBuf, _argv: &[String], - _workdir: &Path, + _workdir: &AbsolutePathBuf, ) -> anyhow::Result { Ok(self.action.clone()) } } + struct AssertingEscalationPolicy { + expected_file: AbsolutePathBuf, + expected_workdir: AbsolutePathBuf, + } + + #[async_trait::async_trait] + impl EscalationPolicy for AssertingEscalationPolicy { + async fn determine_action( + &self, + file: &AbsolutePathBuf, + _argv: &[String], + workdir: &AbsolutePathBuf, + ) -> anyhow::Result { + assert_eq!(file, &self.expected_file); + assert_eq!(workdir, &self.expected_workdir); + Ok(EscalateAction::Run) + } + } + #[tokio::test] async fn handle_escalate_session_respects_run_in_sandbox_decision() -> anyhow::Result<()> { let (server, client) = AsyncSocket::pair()?; @@ -277,7 +296,7 @@ mod tests { .send(EscalateRequest { file: PathBuf::from("/bin/echo"), argv: vec!["echo".to_string()], - workdir: PathBuf::from("/tmp"), + workdir: AbsolutePathBuf::try_from(PathBuf::from("/tmp"))?, env, }) .await?; @@ -292,6 +311,42 @@ mod tests { server_task.await? } + #[tokio::test] + async fn handle_escalate_session_resolves_relative_file_against_request_workdir() + -> anyhow::Result<()> { + let (server, client) = AsyncSocket::pair()?; + let tmp = tempfile::TempDir::new()?; + let workdir = tmp.path().join("workspace"); + std::fs::create_dir(&workdir)?; + let workdir = AbsolutePathBuf::try_from(workdir)?; + let expected_file = workdir.join("bin/tool")?; + let server_task = tokio::spawn(handle_escalate_session_with_policy( + server, + Arc::new(AssertingEscalationPolicy { + expected_file, + expected_workdir: workdir.clone(), + }), + )); + + client + .send(EscalateRequest { + file: PathBuf::from("./bin/tool"), + argv: vec!["./bin/tool".to_string()], + workdir, + env: HashMap::new(), + }) + .await?; + + let response = client.receive::().await?; + assert_eq!( + EscalateResponse { + action: EscalateAction::Run, + }, + response + ); + server_task.await? + } + #[tokio::test] async fn handle_escalate_session_executes_escalated_command() -> anyhow::Result<()> { let (server, client) = AsyncSocket::pair()?; @@ -310,7 +365,7 @@ mod tests { "-c".to_string(), r#"if [ "$KEY" = VALUE ]; then exit 42; else exit 1; fi"#.to_string(), ], - workdir: std::env::current_dir()?, + workdir: AbsolutePathBuf::current_dir()?, env: HashMap::from([("KEY".to_string(), "VALUE".to_string())]), }) .await?; diff --git a/codex-rs/shell-escalation/src/unix/escalation_policy.rs b/codex-rs/shell-escalation/src/unix/escalation_policy.rs index 09e74e271..28e04e6f5 100644 --- a/codex-rs/shell-escalation/src/unix/escalation_policy.rs +++ b/codex-rs/shell-escalation/src/unix/escalation_policy.rs @@ -1,4 +1,4 @@ -use std::path::Path; +use codex_utils_absolute_path::AbsolutePathBuf; use crate::unix::escalate_protocol::EscalateAction; @@ -7,8 +7,8 @@ use crate::unix::escalate_protocol::EscalateAction; pub trait EscalationPolicy: Send + Sync { async fn determine_action( &self, - file: &Path, + file: &AbsolutePathBuf, argv: &[String], - workdir: &Path, + workdir: &AbsolutePathBuf, ) -> anyhow::Result; }