mirror of
https://github.com/openai/codex.git
synced 2026-09-07 15:40:00 +00:00
clean up from review comments.
This commit is contained in:
@@ -250,13 +250,17 @@ impl ExecPolicyManager {
|
||||
prefix_rule,
|
||||
} = req;
|
||||
let exec_policy = self.current();
|
||||
let (commands, used_complex_parsing) = commands_for_exec_policy(original_command);
|
||||
#[cfg(windows)]
|
||||
let powershell_commands = try_parse_powershell_command_sequence(
|
||||
original_command,
|
||||
PowershellCommandSequenceParseMode::ExecPolicy,
|
||||
)
|
||||
.filter(|commands| !commands.is_empty());
|
||||
let (commands, used_complex_parsing) = if let Some(commands) = powershell_commands.clone() {
|
||||
(commands, false)
|
||||
} else {
|
||||
commands_for_exec_policy(original_command)
|
||||
};
|
||||
// Keep heredoc prefix parsing for rule evaluation so existing
|
||||
// allow/prompt/forbidden rules still apply, but avoid auto-derived
|
||||
// amendments when only the heredoc fallback parser matched.
|
||||
@@ -291,14 +295,30 @@ impl ExecPolicyManager {
|
||||
|
||||
#[cfg(windows)]
|
||||
if powershell_commands.is_some() {
|
||||
for rule_match in &mut evaluation.matched_rules {
|
||||
if let RuleMatch::HeuristicsRuleMatch {
|
||||
command: matched_command,
|
||||
..
|
||||
} = rule_match
|
||||
{
|
||||
*matched_command = original_command.to_vec();
|
||||
let outer_policy_matches = exec_policy.matches_for_command_with_options(
|
||||
original_command,
|
||||
/*heuristics_fallback*/ None,
|
||||
&match_options,
|
||||
);
|
||||
if outer_policy_matches.is_empty() {
|
||||
for rule_match in &mut evaluation.matched_rules {
|
||||
if let RuleMatch::HeuristicsRuleMatch {
|
||||
command: matched_command,
|
||||
..
|
||||
} = rule_match
|
||||
{
|
||||
*matched_command = original_command.to_vec();
|
||||
}
|
||||
}
|
||||
} else {
|
||||
evaluation.matched_rules.retain(is_policy_match);
|
||||
evaluation.matched_rules.extend(outer_policy_matches);
|
||||
evaluation.decision = evaluation
|
||||
.matched_rules
|
||||
.iter()
|
||||
.map(RuleMatch::decision)
|
||||
.max()
|
||||
.expect("invariant failed: matched_rules must be non-empty");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -338,19 +358,10 @@ impl ExecPolicyManager {
|
||||
}
|
||||
}
|
||||
Decision::Allow => ExecApprovalRequirement::Skip {
|
||||
// Bypass sandbox only when every parsed command segment is
|
||||
// explicitly allowed by execpolicy.
|
||||
bypass_sandbox: commands.iter().all(|command| {
|
||||
exec_policy
|
||||
.matches_for_command_with_options(
|
||||
command,
|
||||
/*heuristics_fallback*/ None,
|
||||
&match_options,
|
||||
)
|
||||
.iter()
|
||||
.any(|rule_match| {
|
||||
is_policy_match(rule_match) && rule_match.decision() == Decision::Allow
|
||||
})
|
||||
// Bypass sandbox only when the allow decision came entirely
|
||||
// from explicit execpolicy allow rules.
|
||||
bypass_sandbox: evaluation.matched_rules.iter().all(|rule_match| {
|
||||
is_policy_match(rule_match) && rule_match.decision() == Decision::Allow
|
||||
}),
|
||||
proposed_execpolicy_amendment: if auto_amendment_allowed {
|
||||
try_derive_execpolicy_amendment_for_allow_rules(&evaluation.matched_rules)
|
||||
@@ -736,15 +747,6 @@ fn commands_for_exec_policy(command: &[String]) -> (Vec<Vec<String>>, bool) {
|
||||
return (commands, false);
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
if let Some(commands) = try_parse_powershell_command_sequence(
|
||||
command,
|
||||
PowershellCommandSequenceParseMode::ExecPolicy,
|
||||
) && !commands.is_empty()
|
||||
{
|
||||
return (commands, false);
|
||||
}
|
||||
|
||||
if let Some(single_command) = parse_shell_lc_single_command_prefix(command) {
|
||||
return (vec![single_command], true);
|
||||
}
|
||||
|
||||
@@ -698,6 +698,18 @@ async fn evaluates_powershell_wrapped_inner_commands_against_prefix_rules() {
|
||||
]),
|
||||
vec_str(&["Remove-Item"]),
|
||||
),
|
||||
(
|
||||
r#"prefix_rule(pattern=["git", "push"], decision="forbidden")"#.to_string(),
|
||||
vec_str(&[
|
||||
"powershell.exe",
|
||||
"-Version",
|
||||
"5.1",
|
||||
"-NoExit",
|
||||
"-Command",
|
||||
"git push origin main",
|
||||
]),
|
||||
vec_str(&["git", "push"]),
|
||||
),
|
||||
];
|
||||
|
||||
for (policy_src, command, matched_prefix) in cases {
|
||||
@@ -724,6 +736,33 @@ async fn evaluates_powershell_wrapped_inner_commands_against_prefix_rules() {
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
#[tokio::test]
|
||||
async fn powershell_wrapper_rules_still_apply_when_inner_commands_are_parsed() {
|
||||
assert_exec_approval_requirement_for_command(
|
||||
ExecApprovalRequirementScenario {
|
||||
policy_src: Some(
|
||||
concat!(
|
||||
r#"prefix_rule(pattern=["powershell.exe"], decision="forbidden")"#,
|
||||
"\n",
|
||||
r#"prefix_rule(pattern=["Get-Content"], decision="allow")"#,
|
||||
)
|
||||
.to_string(),
|
||||
),
|
||||
command: vec_str(&["powershell.exe", "-Command", "Get-Content Cargo.toml"]),
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
sandbox_policy: SandboxPolicy::new_read_only_policy(),
|
||||
file_system_sandbox_policy: read_only_file_system_sandbox_policy(),
|
||||
sandbox_permissions: SandboxPermissions::UseDefault,
|
||||
prefix_rule: None,
|
||||
},
|
||||
ExecApprovalRequirement::Forbidden {
|
||||
reason: "`powershell.exe -Command 'Get-Content Cargo.toml'` rejected: policy forbids commands starting with `powershell.exe`".to_string(),
|
||||
},
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
#[tokio::test]
|
||||
async fn unmatched_powershell_wrappers_keep_outer_command_heuristics() {
|
||||
|
||||
@@ -91,7 +91,8 @@ pub fn try_parse_powershell_command_sequence(
|
||||
) -> Option<Vec<Vec<String>>> {
|
||||
let (executable, args) = command.split_first()?;
|
||||
if is_powershell_executable(executable) {
|
||||
parse_powershell_invocation(executable, args, mode)
|
||||
let parser_executable = trusted_powershell_parser_executable(executable)?;
|
||||
parse_powershell_invocation(&parser_executable, args, mode)
|
||||
} else {
|
||||
None
|
||||
}
|
||||
@@ -149,8 +150,13 @@ fn parse_powershell_invocation(
|
||||
{
|
||||
return None;
|
||||
}
|
||||
_ if lower.starts_with('-') => {
|
||||
return None;
|
||||
_ if looks_like_powershell_flag(&lower) => {
|
||||
if mode == PowershellCommandSequenceParseMode::SafeCommand {
|
||||
return None;
|
||||
}
|
||||
|
||||
idx += powershell_wrapper_flag_length(args, idx);
|
||||
continue;
|
||||
}
|
||||
_ => {
|
||||
if mode == PowershellCommandSequenceParseMode::ExecPolicy {
|
||||
@@ -173,6 +179,22 @@ pub(crate) fn parse_powershell_script_to_commands(
|
||||
try_parse_powershell_ast_commands(executable, script)
|
||||
}
|
||||
|
||||
fn trusted_powershell_parser_executable(exe: &str) -> Option<String> {
|
||||
let executable_name = std::path::Path::new(exe)
|
||||
.file_name()
|
||||
.and_then(|osstr| osstr.to_str())
|
||||
.unwrap_or(exe)
|
||||
.to_ascii_lowercase();
|
||||
|
||||
let parser_executable = match executable_name.as_str() {
|
||||
"powershell" | "powershell.exe" => try_find_powershell_executable_blocking()?,
|
||||
"pwsh" | "pwsh.exe" => try_find_pwsh_executable_blocking()?,
|
||||
_ => return None,
|
||||
};
|
||||
|
||||
Some(parser_executable.as_path().to_string_lossy().into_owned())
|
||||
}
|
||||
|
||||
pub(crate) fn is_powershell_executable(exe: &str) -> bool {
|
||||
let executable_name = std::path::Path::new(exe)
|
||||
.file_name()
|
||||
@@ -209,6 +231,10 @@ fn quote_argument(arg: &str) -> String {
|
||||
format!("'{}'", arg.replace('\'', "''"))
|
||||
}
|
||||
|
||||
fn looks_like_powershell_flag(lower: &str) -> bool {
|
||||
lower.starts_with('-') || lower.starts_with('/')
|
||||
}
|
||||
|
||||
fn is_powershell_no_arg_parse_flag(lower: &str) -> bool {
|
||||
POWERSHELL_NO_ARG_PARSE_FLAGS.contains(&lower)
|
||||
}
|
||||
@@ -251,6 +277,18 @@ fn split_flag_inline_value(lower: &str) -> Option<(&str, &str)> {
|
||||
lower.split_once(':')
|
||||
}
|
||||
|
||||
fn powershell_wrapper_flag_length(args: &[String], idx: usize) -> usize {
|
||||
let Some(next_arg) = args.get(idx + 1) else {
|
||||
return 1;
|
||||
};
|
||||
|
||||
if looks_like_powershell_flag(&next_arg.to_ascii_lowercase()) {
|
||||
1
|
||||
} else {
|
||||
2
|
||||
}
|
||||
}
|
||||
|
||||
/// This function attempts to find a powershell.exe executable on the system.
|
||||
pub fn try_find_powershell_executable_blocking() -> Option<AbsolutePathBuf> {
|
||||
try_find_powershellish_executable_in_path(&["powershell.exe"])
|
||||
@@ -321,7 +359,10 @@ fn is_powershellish_executable_available(powershell_or_pwsh_exe: &std::path::Pat
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::PowershellCommandSequenceParseMode;
|
||||
use super::extract_powershell_command;
|
||||
use super::try_parse_powershell_command_sequence;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
fn extracts_basic_powershell_command() {
|
||||
@@ -369,4 +410,49 @@ mod tests {
|
||||
let (_shell, script) = extract_powershell_command(&cmd).expect("extract");
|
||||
assert_eq!(script, "Get-ChildItem | Select-String foo");
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
#[test]
|
||||
fn exec_policy_parsing_ignores_ordinary_wrapper_flags() {
|
||||
let command = vec![
|
||||
"powershell.exe".to_string(),
|
||||
"-Version".to_string(),
|
||||
"5.1".to_string(),
|
||||
"-NoExit".to_string(),
|
||||
"-Command".to_string(),
|
||||
"Get-Content 'foo bar'".to_string(),
|
||||
];
|
||||
|
||||
assert_eq!(
|
||||
try_parse_powershell_command_sequence(
|
||||
&command,
|
||||
PowershellCommandSequenceParseMode::ExecPolicy,
|
||||
),
|
||||
Some(vec![
|
||||
vec!["Get-Content".to_string(), "foo bar".to_string(),]
|
||||
]),
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
#[test]
|
||||
fn uses_trusted_system_powershell_for_ast_parsing() {
|
||||
let command = vec![
|
||||
r"C:\repo\powershell.exe".to_string(),
|
||||
"-NoExit".to_string(),
|
||||
"-Command".to_string(),
|
||||
"Get-Content Cargo.toml".to_string(),
|
||||
];
|
||||
|
||||
assert_eq!(
|
||||
try_parse_powershell_command_sequence(
|
||||
&command,
|
||||
PowershellCommandSequenceParseMode::ExecPolicy,
|
||||
),
|
||||
Some(vec![vec![
|
||||
"Get-Content".to_string(),
|
||||
"Cargo.toml".to_string(),
|
||||
]]),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user