From f38077e76522d7cbbbf738696cb6ec8e2d88df97 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Sat, 28 Feb 2026 09:01:03 -0800 Subject: [PATCH] core: resolve host_executable() rules during preflight --- codex-rs/core/src/exec_policy.rs | 135 +++++++++++++++++- .../command_safety/is_dangerous_command.rs | 29 +++- .../src/command_safety/is_safe_command.rs | 18 ++- 3 files changed, 175 insertions(+), 7 deletions(-) diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index b781f0bc7de..69a7cb933df 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -13,6 +13,7 @@ use codex_execpolicy::AmendError; use codex_execpolicy::Decision; use codex_execpolicy::Error as ExecPolicyRuleError; use codex_execpolicy::Evaluation; +use codex_execpolicy::MatchOptions; use codex_execpolicy::NetworkRuleProtocol; use codex_execpolicy::Policy; use codex_execpolicy::PolicyParser; @@ -221,7 +222,14 @@ impl ExecPolicyManager { used_complex_parsing, ) }; - let evaluation = exec_policy.check_multiple(commands.iter(), &exec_policy_fallback); + let match_options = MatchOptions { + resolve_host_executables: true, + }; + let evaluation = exec_policy.check_multiple_with_options( + commands.iter(), + &exec_policy_fallback, + &match_options, + ); let requested_amendment = derive_requested_execpolicy_amendment_from_prefix_rule( prefix_rule.as_ref(), @@ -229,6 +237,7 @@ impl ExecPolicyManager { exec_policy.as_ref(), &commands, &exec_policy_fallback, + &match_options, ); match evaluation.decision { @@ -630,6 +639,7 @@ fn derive_requested_execpolicy_amendment_from_prefix_rule( exec_policy: &Policy, commands: &[Vec], exec_policy_fallback: &impl Fn(&[String]) -> Decision, + match_options: &MatchOptions, ) -> Option { let prefix_rule = prefix_rule?; if prefix_rule.is_empty() { @@ -656,6 +666,7 @@ fn derive_requested_execpolicy_amendment_from_prefix_rule( &amendment.command, commands, exec_policy_fallback, + match_options, ) { Some(amendment) } else { @@ -668,6 +679,7 @@ fn prefix_rule_would_approve_all_commands( prefix_rule: &[String], commands: &[Vec], exec_policy_fallback: &impl Fn(&[String]) -> Decision, + match_options: &MatchOptions, ) -> bool { let mut policy_with_prefix_rule = exec_policy.clone(); if policy_with_prefix_rule @@ -679,7 +691,7 @@ fn prefix_rule_would_approve_all_commands( commands.iter().all(|command| { policy_with_prefix_rule - .check(command, exec_policy_fallback) + .check_with_options(command, exec_policy_fallback, match_options) .decision == Decision::Allow }) @@ -849,6 +861,15 @@ mod tests { path.to_string_lossy().into_owned() } + fn host_program_path(name: &str) -> String { + let executable_name = if cfg!(windows) { + format!("{name}.exe") + } else { + name.to_string() + }; + host_absolute_path(&["usr", "bin", &executable_name]) + } + fn starlark_string(value: &str) -> String { value.replace('\\', "\\\\").replace('"', "\\\"") } @@ -1398,6 +1419,115 @@ prefix_rule( ); } + #[tokio::test] + async fn absolute_path_exec_approval_requirement_matches_host_executable_rules() { + let git_path = host_program_path("git"); + let git_path_literal = starlark_string(&git_path); + let policy_src = format!( + r#" +host_executable(name = "git", paths = ["{git_path_literal}"]) +prefix_rule(pattern=["git"], decision="allow") +"# + ); + let mut parser = PolicyParser::new(); + parser + .parse("test.rules", &policy_src) + .expect("parse policy"); + let manager = ExecPolicyManager::new(Arc::new(parser.build())); + let command = vec![git_path, "status".to_string()]; + + let requirement = manager + .create_exec_approval_requirement_for_command(ExecApprovalRequest { + command: &command, + approval_policy: AskForApproval::UnlessTrusted, + sandbox_policy: &SandboxPolicy::new_read_only_policy(), + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }) + .await; + + assert_eq!( + requirement, + ExecApprovalRequirement::Skip { + bypass_sandbox: true, + proposed_execpolicy_amendment: None, + } + ); + } + + #[tokio::test] + async fn absolute_path_exec_approval_requirement_ignores_disallowed_host_executable_paths() { + let allowed_git_path = host_program_path("git"); + let disallowed_git_path = host_absolute_path(&[ + "opt", + "homebrew", + "bin", + if cfg!(windows) { "git.exe" } else { "git" }, + ]); + let allowed_git_path_literal = starlark_string(&allowed_git_path); + let policy_src = format!( + r#" +host_executable(name = "git", paths = ["{allowed_git_path_literal}"]) +prefix_rule(pattern=["git"], decision="prompt") +"# + ); + let mut parser = PolicyParser::new(); + parser + .parse("test.rules", &policy_src) + .expect("parse policy"); + let manager = ExecPolicyManager::new(Arc::new(parser.build())); + let command = vec![disallowed_git_path, "status".to_string()]; + + let requirement = manager + .create_exec_approval_requirement_for_command(ExecApprovalRequest { + command: &command, + approval_policy: AskForApproval::UnlessTrusted, + sandbox_policy: &SandboxPolicy::new_read_only_policy(), + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }) + .await; + + assert_eq!( + requirement, + ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(command)), + } + ); + } + + #[tokio::test] + async fn requested_prefix_rule_can_approve_absolute_path_commands() { + let command = vec![ + host_program_path("cargo"), + "install".to_string(), + "cargo-insta".to_string(), + ]; + let manager = ExecPolicyManager::default(); + + let requirement = manager + .create_exec_approval_requirement_for_command(ExecApprovalRequest { + command: &command, + approval_policy: AskForApproval::UnlessTrusted, + sandbox_policy: &SandboxPolicy::new_read_only_policy(), + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: Some(vec!["cargo".to_string(), "install".to_string()]), + }) + .await; + + assert_eq!( + requirement, + ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![ + "cargo".to_string(), + "install".to_string(), + ])), + } + ); + } + #[tokio::test] async fn exec_approval_requirement_respects_approval_policy() { let policy_src = r#"prefix_rule(pattern=["rm"], decision="prompt")"#; @@ -1952,6 +2082,7 @@ prefix_rule( &Policy::empty(), &commands, &|_: &[String]| Decision::Allow, + &MatchOptions::default(), ) } diff --git a/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs b/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs index 33b12bea607..9e4bfe16a8b 100644 --- a/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs +++ b/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs @@ -1,4 +1,5 @@ use crate::bash::parse_shell_lc_plain_commands; +use std::path::Path; #[cfg(windows)] #[path = "windows_dangerous_commands.rs"] mod windows_dangerous_commands; @@ -52,6 +53,32 @@ fn is_git_global_option_with_inline_value(arg: &str) -> bool { ) || ((arg.starts_with("-C") || arg.starts_with("-c")) && arg.len() > 2) } +pub(crate) fn executable_name_lookup_key(raw: &str) -> Option { + #[cfg(windows)] + { + Path::new(raw) + .file_name() + .and_then(|name| name.to_str()) + .map(|name| { + let name = name.to_ascii_lowercase(); + for suffix in [".exe", ".cmd", ".bat", ".com"] { + if let Some(stripped) = name.strip_suffix(suffix) { + return stripped.to_string(); + } + } + name + }) + } + + #[cfg(not(windows))] + { + Path::new(raw) + .file_name() + .and_then(|name| name.to_str()) + .map(std::borrow::ToOwned::to_owned) + } +} + /// Find the first matching git subcommand, skipping known global options that /// may appear before it (e.g., `-C`, `-c`, `--git-dir`). /// @@ -61,7 +88,7 @@ pub(crate) fn find_git_subcommand<'a>( subcommands: &[&str], ) -> Option<(usize, &'a str)> { let cmd0 = command.first().map(String::as_str)?; - if !cmd0.ends_with("git") { + if executable_name_lookup_key(cmd0).as_deref() != Some("git") { return None; } diff --git a/codex-rs/shell-command/src/command_safety/is_safe_command.rs b/codex-rs/shell-command/src/command_safety/is_safe_command.rs index e52079c74bb..b7349f220a4 100644 --- a/codex-rs/shell-command/src/command_safety/is_safe_command.rs +++ b/codex-rs/shell-command/src/command_safety/is_safe_command.rs @@ -1,4 +1,5 @@ use crate::bash::parse_shell_lc_plain_commands; +use crate::command_safety::is_dangerous_command::executable_name_lookup_key; // Find the first matching git subcommand, skipping known global options that // may appear before it (e.g., `-C`, `-c`, `--git-dir`). // Implemented in `is_dangerous_command` and shared here. @@ -47,10 +48,7 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool { return false; }; - match std::path::Path::new(&cmd0) - .file_name() - .and_then(|osstr| osstr.to_str()) - { + match executable_name_lookup_key(cmd0).as_deref() { Some(cmd) if cfg!(target_os = "linux") && matches!(cmd, "numfmt" | "tac") => true, #[rustfmt::skip] @@ -495,6 +493,18 @@ mod tests { ]))); } + #[test] + fn windows_git_full_path_is_safe() { + if !cfg!(windows) { + return; + } + + assert!(is_known_safe_command(&vec_str(&[ + r"C:\Program Files\Git\cmd\git.exe", + "status", + ]))); + } + #[test] fn bash_lc_safe_examples() { assert!(is_known_safe_command(&vec_str(&["bash", "-lc", "ls"])));