From 5d924d44cff3826da012160034ea1e0696cba41d Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Sun, 4 May 2025 12:32:51 -0700 Subject: [PATCH] fix: ensure apply_patch resolves relative paths against workdir or project cwd (#810) https://github.com/openai/codex/pull/800 kicked off some work to be more disciplined about honoring the `cwd` param passed in rather than assuming `std::env::current_dir()` as the `cwd`. As part of this, we need to ensure `apply_patch` calls honor the appropriate `cwd` as well, which is significant if the paths in the `apply_patch` arg are not absolute paths themselves. Failing that: - The `apply_patch` function call can contain an optional`workdir` param, so: - If specified and is an absolute path, it should be used to resolve relative paths - If specified and is a relative path, should be resolved against `Config.cwd` and then any relative paths will be resolved against the result - If `workdir` is not specified on the function call, relative paths should be resolved against `Config.cwd` Note that we had a similar issue in the TypeScript CLI that was fixed in https://github.com/openai/codex/pull/556. As part of the fix, this PR introduces `ApplyPatchAction` so clients can deal with that instead of the raw `HashMap`. This enables us to enforce, by construction, that all paths contained in the `ApplyPatchAction` are absolute paths. --- codex-rs/apply-patch/src/lib.rs | 45 +++++++-- codex-rs/core/src/codex.rs | 156 ++++++++++++++------------------ codex-rs/core/src/safety.rs | 25 ++--- 3 files changed, 115 insertions(+), 111 deletions(-) diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index 090eab18..fef7d4f3 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -95,7 +95,7 @@ pub enum ApplyPatchFileChange { pub enum MaybeApplyPatchVerified { /// `argv` corresponded to an `apply_patch` invocation, and these are the /// resulting proposed file changes. - Body(HashMap), + Body(ApplyPatchAction), /// `argv` could not be parsed to determine whether it corresponds to an /// `apply_patch` invocation. ShellParseError(Error), @@ -106,7 +106,38 @@ pub enum MaybeApplyPatchVerified { NotApplyPatch, } -pub fn maybe_parse_apply_patch_verified(argv: &[String]) -> MaybeApplyPatchVerified { +#[derive(Debug)] +/// ApplyPatchAction is the result of parsing an `apply_patch` command. By +/// construction, all paths should be absolute paths. +pub struct ApplyPatchAction { + changes: HashMap, +} + +impl ApplyPatchAction { + pub fn is_empty(&self) -> bool { + self.changes.is_empty() + } + + /// Returns the changes that would be made by applying the patch. + pub fn changes(&self) -> &HashMap { + &self.changes + } + + /// Should be used exclusively for testing. (Not worth the overhead of + /// creating a feature flag for this.) + pub fn new_add_for_test(path: &Path, content: String) -> Self { + if !path.is_absolute() { + panic!("path must be absolute"); + } + + let changes = HashMap::from([(path.to_path_buf(), ApplyPatchFileChange::Add { content })]); + Self { changes } + } +} + +/// cwd must be an absolute path so that we can resolve relative paths in the +/// patch. +pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApplyPatchVerified { match maybe_parse_apply_patch(argv) { MaybeApplyPatch::Body(hunks) => { let mut changes = HashMap::new(); @@ -114,14 +145,14 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String]) -> MaybeApplyPatchVerif match hunk { Hunk::AddFile { path, contents } => { changes.insert( - path, + cwd.join(path), ApplyPatchFileChange::Add { content: contents.clone(), }, ); } Hunk::DeleteFile { path } => { - changes.insert(path, ApplyPatchFileChange::Delete); + changes.insert(cwd.join(path), ApplyPatchFileChange::Delete); } Hunk::UpdateFile { path, @@ -138,17 +169,17 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String]) -> MaybeApplyPatchVerif } }; changes.insert( - path.clone(), + cwd.join(path), ApplyPatchFileChange::Update { unified_diff, - move_path, + move_path: move_path.map(|p| cwd.join(p)), new_content: contents, }, ); } } } - MaybeApplyPatchVerified::Body(changes) + MaybeApplyPatchVerified::Body(ApplyPatchAction { changes }) } MaybeApplyPatch::ShellParseError(e) => MaybeApplyPatchVerified::ShellParseError(e), MaybeApplyPatch::PatchParseError(e) => MaybeApplyPatchVerified::CorrectnessError(e.into()), diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 8f3420ac..c74d0079 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -12,6 +12,7 @@ use async_channel::Sender; use codex_apply_patch::maybe_parse_apply_patch_verified; use codex_apply_patch::print_summary; use codex_apply_patch::AffectedPaths; +use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::ApplyPatchFileChange; use codex_apply_patch::MaybeApplyPatchVerified; use fs_err as fs; @@ -271,7 +272,7 @@ impl Session { pub async fn request_patch_approval( &self, sub_id: String, - changes: &HashMap, + action: &ApplyPatchAction, reason: Option, grant_root: Option, ) -> oneshot::Receiver { @@ -279,7 +280,7 @@ impl Session { let event = Event { id: sub_id.clone(), msg: EventMsg::ApplyPatchApprovalRequest { - changes: convert_apply_patch_to_protocol(changes), + changes: convert_apply_patch_to_protocol(action), reason, grant_root, }, @@ -304,19 +305,13 @@ impl Session { state.approved_commands.insert(cmd); } - async fn notify_exec_command_begin( - &self, - sub_id: &str, - call_id: &str, - command: Vec, - cwd: PathBuf, - ) { + async fn notify_exec_command_begin(&self, sub_id: &str, call_id: &str, params: &ExecParams) { let event = Event { id: sub_id.to_string(), msg: EventMsg::ExecCommandBegin { call_id: call_id.to_string(), - command, - cwd, + command: params.command.clone(), + cwd: params.cwd.clone(), }, }; let _ = self.tx_event.send(event).await; @@ -886,8 +881,12 @@ async fn handle_function_call( match name.as_str() { "container.exec" | "shell" => { // parse command - let params = match serde_json::from_str::(&arguments) { - Ok(v) => v, + let params: ExecParams = match serde_json::from_str::(&arguments) { + Ok(shell_tool_call_params) => ExecParams { + command: shell_tool_call_params.command, + cwd: sess.resolve_path(shell_tool_call_params.workdir.clone()), + timeout_ms: shell_tool_call_params.timeout_ms, + }, Err(e) => { // allow model to re-sample let output = ResponseInputItem::FunctionCallOutput { @@ -902,7 +901,7 @@ async fn handle_function_call( }; // check if this was a patch, and apply it if so - match maybe_parse_apply_patch_verified(¶ms.command) { + match maybe_parse_apply_patch_verified(¶ms.command, ¶ms.cwd) { MaybeApplyPatchVerified::Body(changes) => { return apply_patch(sess, sub_id, call_id, changes).await; } @@ -924,9 +923,6 @@ async fn handle_function_call( MaybeApplyPatchVerified::NotApplyPatch => (), } - // this was not a valid patch, execute command - let workdir = sess.resolve_path(params.workdir.clone()); - // safety checks let safety = { let state = sess.state.lock().unwrap(); @@ -944,7 +940,7 @@ async fn handle_function_call( .request_command_approval( sub_id.clone(), params.command.clone(), - workdir.clone(), + params.cwd.clone(), None, ) .await; @@ -980,20 +976,11 @@ async fn handle_function_call( } }; - sess.notify_exec_command_begin( - &sub_id, - &call_id, - params.command.clone(), - workdir.clone(), - ) - .await; + sess.notify_exec_command_begin(&sub_id, &call_id, ¶ms) + .await; let output_result = process_exec_tool_call( - ExecParams { - command: params.command.clone(), - cwd: workdir.clone(), - timeout_ms: params.timeout_ms, - }, + params.clone(), sandbox_type, sess.ctrl_c.clone(), &sess.sandbox_policy, @@ -1050,7 +1037,7 @@ async fn handle_function_call( .request_command_approval( sub_id.clone(), params.command.clone(), - workdir, + params.cwd.clone(), Some("command failed; retry without sandbox?".to_string()), ) .await; @@ -1071,23 +1058,13 @@ async fn handle_function_call( // Emit a fresh Begin event so progress bars reset. let retry_call_id = format!("{call_id}-retry"); - let cwd = sess.resolve_path(params.workdir.clone()); - sess.notify_exec_command_begin( - &sub_id, - &retry_call_id, - params.command.clone(), - cwd.clone(), - ) - .await; + sess.notify_exec_command_begin(&sub_id, &retry_call_id, ¶ms) + .await; // This is an escalated retry; the policy will not be // examined and the sandbox has been set to `None`. let retry_output_result = process_exec_tool_call( - ExecParams { - command: params.command.clone(), - cwd: cwd.clone(), - timeout_ms: params.timeout_ms, - }, + params, SandboxType::None, sess.ctrl_c.clone(), &sess.sandbox_policy, @@ -1180,7 +1157,7 @@ async fn apply_patch( sess: &Session, sub_id: String, call_id: String, - changes: HashMap, + action: ApplyPatchAction, ) -> ResponseInputItem { let writable_roots_snapshot = { let guard = sess.writable_roots.lock().unwrap(); @@ -1188,7 +1165,7 @@ async fn apply_patch( }; let auto_approved = match assess_patch_safety( - &changes, + &action, sess.approval_policy, &writable_roots_snapshot, &sess.cwd, @@ -1198,7 +1175,7 @@ async fn apply_patch( // Compute a readable summary of path changes to include in the // approval request so the user can make an informed decision. let rx_approve = sess - .request_patch_approval(sub_id.clone(), &changes, None, None) + .request_patch_approval(sub_id.clone(), &action, None, None) .await; match rx_approve.await.unwrap_or_default() { ReviewDecision::Approved | ReviewDecision::ApprovedForSession => false, @@ -1227,7 +1204,7 @@ async fn apply_patch( // Verify write permissions before touching the filesystem. let writable_snapshot = { sess.writable_roots.lock().unwrap().clone() }; - if let Some(offending) = first_offending_path(&changes, &writable_snapshot, &sess.cwd) { + if let Some(offending) = first_offending_path(&action, &writable_snapshot, &sess.cwd) { let root = offending.parent().unwrap_or(&offending).to_path_buf(); let reason = Some(format!( @@ -1236,7 +1213,7 @@ async fn apply_patch( )); let rx = sess - .request_patch_approval(sub_id.clone(), &changes, reason.clone(), Some(root.clone())) + .request_patch_approval(sub_id.clone(), &action, reason.clone(), Some(root.clone())) .await; if !matches!( @@ -1263,7 +1240,7 @@ async fn apply_patch( msg: EventMsg::PatchApplyBegin { call_id: call_id.clone(), auto_approved, - changes: convert_apply_patch_to_protocol(&changes), + changes: convert_apply_patch_to_protocol(&action), }, }) .await; @@ -1272,37 +1249,43 @@ async fn apply_patch( let mut stderr = Vec::new(); // Enforce writable roots. If a write is blocked, collect offending root // and prompt the user to extend permissions. - let mut result = apply_changes_from_apply_patch_and_report(&changes, &mut stdout, &mut stderr); + let mut result = apply_changes_from_apply_patch_and_report(&action, &mut stdout, &mut stderr); if let Err(err) = &result { if err.kind() == std::io::ErrorKind::PermissionDenied { // Determine first offending path. - let offending_opt = changes.iter().find_map(|(path, change)| { - let path_ref = match change { - ApplyPatchFileChange::Add { .. } => path, - ApplyPatchFileChange::Delete => path, - ApplyPatchFileChange::Update { .. } => path, - }; + let offending_opt = action + .changes() + .iter() + .flat_map(|(path, change)| match change { + ApplyPatchFileChange::Add { .. } => vec![path.as_ref()], + ApplyPatchFileChange::Delete => vec![path.as_ref()], + ApplyPatchFileChange::Update { + move_path: Some(move_path), + .. + } => { + vec![path.as_ref(), move_path.as_ref()] + } + ApplyPatchFileChange::Update { + move_path: None, .. + } => vec![path.as_ref()], + }) + .find_map(|path: &Path| { + // ApplyPatchAction promises to guarantee absolute paths. + if !path.is_absolute() { + panic!("apply_patch invariant failed: path is not absolute: {path:?}"); + } - // Reuse safety normalization logic: treat absolute path. - let abs = if path_ref.is_absolute() { - path_ref.clone() - } else { - // TODO(mbolin): If workdir was supplied with apply_patch call, - // relative paths should be resolved against it. - sess.cwd.join(path_ref) - }; - - let writable = { - let roots = sess.writable_roots.lock().unwrap(); - roots.iter().any(|root| abs.starts_with(root)) - }; - if writable { - None - } else { - Some(path_ref.clone()) - } - }); + let writable = { + let roots = sess.writable_roots.lock().unwrap(); + roots.iter().any(|root| path.starts_with(root)) + }; + if writable { + None + } else { + Some(path.to_path_buf()) + } + }); if let Some(offending) = offending_opt { let root = offending.parent().unwrap_or(&offending).to_path_buf(); @@ -1314,7 +1297,7 @@ async fn apply_patch( let rx = sess .request_patch_approval( sub_id.clone(), - &changes, + &action, reason.clone(), Some(root.clone()), ) @@ -1328,7 +1311,7 @@ async fn apply_patch( stdout.clear(); stderr.clear(); result = apply_changes_from_apply_patch_and_report( - &changes, + &action, &mut stdout, &mut stderr, ); @@ -1374,10 +1357,11 @@ async fn apply_patch( /// `writable_roots` (after normalising). If all paths are acceptable, /// returns None. fn first_offending_path( - changes: &HashMap, + action: &ApplyPatchAction, writable_roots: &[PathBuf], cwd: &Path, ) -> Option { + let changes = action.changes(); for (path, change) in changes { let candidate = match change { ApplyPatchFileChange::Add { .. } => path, @@ -1411,9 +1395,8 @@ fn first_offending_path( None } -fn convert_apply_patch_to_protocol( - changes: &HashMap, -) -> HashMap { +fn convert_apply_patch_to_protocol(action: &ApplyPatchAction) -> HashMap { + let changes = action.changes(); let mut result = HashMap::with_capacity(changes.len()); for (path, change) in changes { let protocol_change = match change { @@ -1436,11 +1419,11 @@ fn convert_apply_patch_to_protocol( } fn apply_changes_from_apply_patch_and_report( - changes: &HashMap, + action: &ApplyPatchAction, stdout: &mut impl std::io::Write, stderr: &mut impl std::io::Write, ) -> std::io::Result<()> { - match apply_changes_from_apply_patch(changes) { + match apply_changes_from_apply_patch(action) { Ok(affected_paths) => { print_summary(&affected_paths, stdout)?; } @@ -1452,13 +1435,12 @@ fn apply_changes_from_apply_patch_and_report( Ok(()) } -fn apply_changes_from_apply_patch( - changes: &HashMap, -) -> anyhow::Result { +fn apply_changes_from_apply_patch(action: &ApplyPatchAction) -> anyhow::Result { let mut added: Vec = Vec::new(); let mut modified: Vec = Vec::new(); let mut deleted: Vec = Vec::new(); + let changes = action.changes(); for (path, change) in changes { match change { ApplyPatchFileChange::Add { content } => { diff --git a/codex-rs/core/src/safety.rs b/codex-rs/core/src/safety.rs index 3d98be6c..ac1b30a6 100644 --- a/codex-rs/core/src/safety.rs +++ b/codex-rs/core/src/safety.rs @@ -1,9 +1,9 @@ -use std::collections::HashMap; use std::collections::HashSet; use std::path::Component; use std::path::Path; use std::path::PathBuf; +use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::ApplyPatchFileChange; use crate::exec::SandboxType; @@ -19,12 +19,12 @@ pub enum SafetyCheck { } pub fn assess_patch_safety( - changes: &HashMap, + action: &ApplyPatchAction, policy: AskForApproval, writable_roots: &[PathBuf], cwd: &Path, ) -> SafetyCheck { - if changes.is_empty() { + if action.is_empty() { return SafetyCheck::Reject { reason: "empty patch".to_string(), }; @@ -41,7 +41,7 @@ pub fn assess_patch_safety( } } - if is_write_patch_constrained_to_writable_paths(changes, writable_roots, cwd) { + if is_write_patch_constrained_to_writable_paths(action, writable_roots, cwd) { SafetyCheck::AutoApprove { sandbox_type: SandboxType::None, } @@ -114,7 +114,7 @@ pub fn get_platform_sandbox() -> Option { } fn is_write_patch_constrained_to_writable_paths( - changes: &HashMap, + action: &ApplyPatchAction, writable_roots: &[PathBuf], cwd: &Path, ) -> bool { @@ -164,7 +164,7 @@ fn is_write_patch_constrained_to_writable_paths( }) }; - for (path, change) in changes { + for (path, change) in action.changes() { match change { ApplyPatchFileChange::Add { .. } | ApplyPatchFileChange::Delete => { if !is_path_writable(path) { @@ -198,18 +198,9 @@ mod tests { // Helper to build a single‑entry map representing a patch that adds a // file at `p`. - let make_add_change = |p: PathBuf| { - let mut m = HashMap::new(); - m.insert( - p.clone(), - ApplyPatchFileChange::Add { - content: String::new(), - }, - ); - m - }; + let make_add_change = |p: PathBuf| ApplyPatchAction::new_add_for_test(&p, "".to_string()); - let add_inside = make_add_change(PathBuf::from("inner.txt")); + let add_inside = make_add_change(cwd.join("inner.txt")); let add_outside = make_add_change(parent.join("outside.txt")); assert!(is_write_patch_constrained_to_writable_paths(