mirror of
https://github.com/openai/codex.git
synced 2026-03-01 04:03:47 +00:00
Compare commits
2 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
efc5556781 | ||
|
|
f38077e765 |
@@ -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<String>],
|
||||
exec_policy_fallback: &impl Fn(&[String]) -> Decision,
|
||||
match_options: &MatchOptions,
|
||||
) -> Option<ExecPolicyAmendment> {
|
||||
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<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
|
||||
@@ -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(),
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
@@ -8,6 +8,7 @@ use codex_protocol::config_types::Settings;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::Op;
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use core_test_support::responses::ev_assistant_message;
|
||||
@@ -19,6 +20,7 @@ use core_test_support::responses::sse;
|
||||
use core_test_support::responses::start_mock_server;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
use core_test_support::wait_for_event;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::Value;
|
||||
use serde_json::json;
|
||||
use std::fs;
|
||||
@@ -62,6 +64,33 @@ async fn submit_user_turn(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn expect_exec_approval(
|
||||
test: &core_test_support::test_codex::TestCodex,
|
||||
expected_command: &str,
|
||||
) -> codex_protocol::protocol::ExecApprovalRequestEvent {
|
||||
let event = wait_for_event(&test.codex, |event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_)
|
||||
)
|
||||
})
|
||||
.await;
|
||||
|
||||
match event {
|
||||
EventMsg::ExecApprovalRequest(approval) => {
|
||||
let last_arg = approval
|
||||
.command
|
||||
.last()
|
||||
.map(String::as_str)
|
||||
.unwrap_or_default();
|
||||
assert_eq!(last_arg, expected_command);
|
||||
approval
|
||||
}
|
||||
EventMsg::TurnComplete(_) => panic!("expected approval request before completion"),
|
||||
other => panic!("unexpected event: {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
fn assert_no_matched_rules_invariant(output_item: &Value) {
|
||||
let Some(output) = output_item.get("output").and_then(Value::as_str) else {
|
||||
panic!("function_call_output should include string output payload: {output_item:?}");
|
||||
@@ -161,6 +190,192 @@ async fn execpolicy_blocks_shell_invocation() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
#[cfg(unix)]
|
||||
async fn shell_command_absolute_path_uses_host_executable_rules() -> Result<()> {
|
||||
let git_path = "/tmp/codex-host-executable-shell/git";
|
||||
let command = format!("{git_path} status");
|
||||
|
||||
let mut builder = test_codex().with_config(move |config| {
|
||||
let policy_path = config.codex_home.join("rules").join("policy.rules");
|
||||
fs::create_dir_all(
|
||||
policy_path
|
||||
.parent()
|
||||
.expect("policy directory must have a parent"),
|
||||
)
|
||||
.expect("create policy directory");
|
||||
fs::write(
|
||||
&policy_path,
|
||||
format!(
|
||||
r#"host_executable(name = "git", paths = ["{git_path}"])
|
||||
prefix_rule(pattern = ["git", "status"], decision = "prompt")"#
|
||||
),
|
||||
)
|
||||
.expect("write policy file");
|
||||
});
|
||||
let server = start_mock_server().await;
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
let call_id = "shell-host-executable-prompt";
|
||||
let args = json!({
|
||||
"command": command,
|
||||
"timeout_ms": 1_000,
|
||||
});
|
||||
|
||||
mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_response_created("resp-shell-host-exec-1"),
|
||||
ev_function_call(call_id, "shell_command", &serde_json::to_string(&args)?),
|
||||
ev_completed("resp-shell-host-exec-1"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
let results_mock = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_assistant_message("msg-shell-host-exec-1", "done"),
|
||||
ev_completed("resp-shell-host-exec-2"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
submit_user_turn(
|
||||
&test,
|
||||
"run absolute git path through shell_command",
|
||||
AskForApproval::OnRequest,
|
||||
SandboxPolicy::DangerFullAccess,
|
||||
None,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let approval = expect_exec_approval(&test, &command).await;
|
||||
let reason = approval.reason.as_deref().unwrap_or_default();
|
||||
assert!(
|
||||
reason.contains("requires approval by policy"),
|
||||
"unexpected approval reason: {reason}",
|
||||
);
|
||||
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::Denied,
|
||||
})
|
||||
.await?;
|
||||
|
||||
wait_for_event(&test.codex, |event| {
|
||||
matches!(event, EventMsg::TurnComplete(_))
|
||||
})
|
||||
.await;
|
||||
|
||||
let output_item = results_mock.single_request().function_call_output(call_id);
|
||||
let output = output_item
|
||||
.get("output")
|
||||
.and_then(Value::as_str)
|
||||
.unwrap_or_default();
|
||||
assert!(
|
||||
output.contains("rejected by user"),
|
||||
"unexpected output: {output}",
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
#[cfg(unix)]
|
||||
async fn unified_exec_absolute_path_uses_host_executable_rules() -> Result<()> {
|
||||
let git_path = "/tmp/codex-host-executable-unified/git";
|
||||
let command = format!("{git_path} status");
|
||||
|
||||
let mut builder = test_codex().with_config(move |config| {
|
||||
config.features.enable(Feature::UnifiedExec);
|
||||
let policy_path = config.codex_home.join("rules").join("policy.rules");
|
||||
fs::create_dir_all(
|
||||
policy_path
|
||||
.parent()
|
||||
.expect("policy directory must have a parent"),
|
||||
)
|
||||
.expect("create policy directory");
|
||||
fs::write(
|
||||
&policy_path,
|
||||
format!(
|
||||
r#"host_executable(name = "git", paths = ["{git_path}"])
|
||||
prefix_rule(pattern = ["git", "status"], decision = "prompt")"#
|
||||
),
|
||||
)
|
||||
.expect("write policy file");
|
||||
});
|
||||
let server = start_mock_server().await;
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
let call_id = "unified-host-executable-prompt";
|
||||
let args = json!({
|
||||
"cmd": command,
|
||||
"shell": "/bin/bash",
|
||||
"yield_time_ms": 1_000,
|
||||
});
|
||||
|
||||
mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_response_created("resp-unified-host-exec-1"),
|
||||
ev_function_call(call_id, "exec_command", &serde_json::to_string(&args)?),
|
||||
ev_completed("resp-unified-host-exec-1"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
let results_mock = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_assistant_message("msg-unified-host-exec-1", "done"),
|
||||
ev_completed("resp-unified-host-exec-2"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
submit_user_turn(
|
||||
&test,
|
||||
"run absolute git path through exec_command",
|
||||
AskForApproval::OnRequest,
|
||||
SandboxPolicy::DangerFullAccess,
|
||||
None,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let approval = expect_exec_approval(&test, &command).await;
|
||||
let reason = approval.reason.as_deref().unwrap_or_default();
|
||||
assert!(
|
||||
reason.contains("requires approval by policy"),
|
||||
"unexpected approval reason: {reason}",
|
||||
);
|
||||
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::Denied,
|
||||
})
|
||||
.await?;
|
||||
|
||||
wait_for_event(&test.codex, |event| {
|
||||
matches!(event, EventMsg::TurnComplete(_))
|
||||
})
|
||||
.await;
|
||||
|
||||
let output_item = results_mock.single_request().function_call_output(call_id);
|
||||
let output = output_item
|
||||
.get("output")
|
||||
.and_then(Value::as_str)
|
||||
.unwrap_or_default();
|
||||
assert!(
|
||||
output.contains("rejected by user"),
|
||||
"unexpected output: {output}",
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn shell_command_empty_script_with_collaboration_mode_does_not_panic() -> Result<()> {
|
||||
let server = start_mock_server().await;
|
||||
|
||||
@@ -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<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`).
|
||||
///
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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"])));
|
||||
|
||||
Reference in New Issue
Block a user