From 6a673e7339161cf5aa6711324bfe873846234b6b Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Sat, 28 Feb 2026 09:25:30 -0800 Subject: [PATCH] core: resolve host_executable() rules during preflight (#13065) ## Why [#12964](https://github.com/openai/codex/pull/12964) added `host_executable()` support to `codex-execpolicy`, and [#13046](https://github.com/openai/codex/pull/13046) adopted it in the zsh-fork interception path. The remaining gap was the preflight execpolicy check in `core/src/exec_policy.rs`. That path derives approval requirements before execution for `shell`, `shell_command`, and `unified_exec`, but it was still using the default exact-token matcher. As a result, a command that already included an absolute executable path, such as `/usr/bin/git status`, could still miss a basename rule like `prefix_rule(pattern = ["git"], ...)` during preflight even when the policy also defined a matching `host_executable(name = "git", ...)` entry. This PR brings the same opt-in `host_executable()` resolution to the preflight approval path when an absolute program path is already present in the parsed command. ## What Changed - updated `ExecPolicyManager::create_exec_approval_requirement_for_command()` in `core/src/exec_policy.rs` to use `check_multiple_with_options(...)` with `MatchOptions { resolve_host_executables: true }` - kept the existing shell parsing flow for approval derivation, but now allow basename rules to match absolute executable paths during preflight when `host_executable()` permits it - updated requested-prefix amendment evaluation to use the same host-executable-aware matching mode, so suggested `prefix_rule()` amendments are checked consistently for absolute-path commands - added preflight coverage for: - absolute-path commands that should match basename rules through `host_executable()` - absolute-path commands whose paths are not in the allowed `host_executable()` mapping - requested prefix-rule amendments for absolute-path commands ## Verification - `just fix -p codex-core` - `cargo test -p codex-core --lib exec_policy::tests::` --- 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 b781f0bc7..69a7cb933 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 33b12bea6..9e4bfe16a 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 e52079c74..b7349f220 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"])));