Skip to content
Open
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
135 changes: 133 additions & 2 deletions codex-rs/core/src/exec_policy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -221,14 +222,22 @@ 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(),
&evaluation.matched_rules,
exec_policy.as_ref(),
&commands,
&exec_policy_fallback,
&match_options,
);

match evaluation.decision {
Expand Down Expand Up @@ -630,6 +639,7 @@ fn derive_requested_execpolicy_amendment_from_prefix_rule(
exec_policy: &Policy,
commands: &[Vec<String>],
exec_policy_fallback: &impl Fn(&[String]) -> Decision,
match_options: &MatchOptions,
) -> Option<ExecPolicyAmendment> {
let prefix_rule = prefix_rule?;
if prefix_rule.is_empty() {
Expand All @@ -656,6 +666,7 @@ fn derive_requested_execpolicy_amendment_from_prefix_rule(
&amendment.command,
commands,
exec_policy_fallback,
match_options,
) {
Some(amendment)
} else {
Expand All @@ -668,6 +679,7 @@ fn prefix_rule_would_approve_all_commands(
prefix_rule: &[String],
commands: &[Vec<String>],
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
Expand All @@ -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
})
Expand Down Expand Up @@ -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('"', "\\\"")
}
Expand Down Expand Up @@ -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")"#;
Expand Down Expand Up @@ -1952,6 +2082,7 @@ prefix_rule(
&Policy::empty(),
&commands,
&|_: &[String]| Decision::Allow,
&MatchOptions::default(),
)
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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<String> {
#[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`).
///
Expand All @@ -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;
}

Expand Down
18 changes: 14 additions & 4 deletions codex-rs/shell-command/src/command_safety/is_safe_command.rs
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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"])));
Expand Down
Loading