Skip to content
Closed
Changes from 1 commit
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
120 changes: 115 additions & 5 deletions codex-rs/core/src/command_safety/windows_dangerous_commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,13 @@ fn is_dangerous_powershell(command: &[String]) -> bool {
.collect();
let has_url = args_have_url(&parsed.tokens);

// Keep parity with Unix-style `rm -f` checks: in PowerShell, `Remove-Item` (and
// common aliases like `rm`) becomes meaningfully more dangerous when `-Force`
// (or `-f`) is present.
if is_powershell_force_delete(&tokens_lc) {
return true;
}

if has_url
&& tokens_lc.iter().any(|t| {
matches!(
Expand Down Expand Up @@ -85,6 +92,39 @@ fn is_dangerous_powershell(command: &[String]) -> bool {
false
}

fn is_powershell_force_delete(tokens_lc: &[String]) -> bool {
let Some(first) = tokens_lc.first() else {
return false;
};

// `rm`, `ri`, `del`, `erase` are common aliases for `Remove-Item`.
if !matches!(
first.as_str(),
"remove-item" | "rm" | "ri" | "del" | "erase"
) {
return false;
}

tokens_lc.iter().any(|t| {
// Common truthy forms.
if matches!(t.as_str(), "-force" | "-f" | "-rf" | "-fr") {
return true;
}

// Only treat explicit truthy assignments as force. Avoid flagging `-Force:$false`.
let value = if let Some(v) = t.strip_prefix("-force:") {
v
} else if let Some(v) = t.strip_prefix("-f:") {
v
} else {
return false;
};

let value = value.trim();
value.eq_ignore_ascii_case("true") || value.eq_ignore_ascii_case("$true") || value == "1"
})
}

fn is_dangerous_cmd(command: &[String]) -> bool {
let Some((exe, rest)) = command.split_first() else {
return false;
Expand All @@ -107,15 +147,35 @@ fn is_dangerous_cmd(command: &[String]) -> bool {
}
}

let Some(first_cmd) = iter.next() else {
let Some(first_cmd_raw) = iter.next() else {
return false;
};
// Classic `cmd /c start https://...` ShellExecute path.
if !first_cmd.eq_ignore_ascii_case("start") {

// The command body sometimes arrives as a single token (e.g. `cmd /c "del /f foo"`).
// Best-effort split it and then append any remaining argv tokens.
let mut cmd_tokens: Vec<String> = if first_cmd_raw.contains(char::is_whitespace) {
shlex_split(first_cmd_raw).unwrap_or_else(|| vec![first_cmd_raw.to_string()])
} else {
vec![first_cmd_raw.to_string()]
};
cmd_tokens.extend(iter.cloned());

let Some((first_cmd, cmd_args)) = cmd_tokens.split_first() else {
return false;
};
let first_cmd_lc = first_cmd.to_ascii_lowercase();

// Classic `cmd /c start https://...` ShellExecute path.
if first_cmd_lc == "start" {
return args_have_url(cmd_args);
}

// Parity with Unix `rm -f`: CMD `del/erase /f` forces deletion of read-only files.
if matches!(first_cmd_lc.as_str(), "del" | "erase") {
return cmd_args.iter().any(|a| a.eq_ignore_ascii_case("/f"));
}
Comment on lines 159 to 189
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve URL checks for non-start cmd /c invocations

With this change, is_dangerous_cmd only treats start and del/erase /f as dangerous and returns false for any other command body. That drops the previous args_have_url fallback, so cmd /c msedge https://example.com, cmd /c rundll32 url.dll,fileprotocolhandler https://..., etc. are no longer flagged even though they launch URLs via cmd. This is a regression because the cmd wrapper is the executable being inspected, so is_direct_gui_launch won’t catch the nested browser invocation; as a result the approval gate can be bypassed for URL launches.

Useful? React with 👍 / 👎.

let remaining: Vec<String> = iter.cloned().collect();
args_have_url(&remaining)

false
}

fn is_direct_gui_launch(command: &[String]) -> bool {
Expand Down Expand Up @@ -288,6 +348,42 @@ mod tests {
])));
}

#[test]
fn powershell_remove_item_force_is_dangerous() {
assert!(is_dangerous_command_windows(&vec_str(&[
"powershell",
"-Command",
"Remove-Item -Force foo.txt"
])));
}

#[test]
fn powershell_rm_f_alias_is_dangerous() {
assert!(is_dangerous_command_windows(&vec_str(&[
"powershell",
"-Command",
"rm -f foo.txt"
])));
}

#[test]
fn powershell_remove_item_without_force_is_not_flagged() {
assert!(!is_dangerous_command_windows(&vec_str(&[
"powershell",
"-Command",
"Remove-Item foo.txt"
])));
}

#[test]
fn powershell_remove_item_force_false_is_not_flagged() {
assert!(!is_dangerous_command_windows(&vec_str(&[
"powershell",
"-Command",
"Remove-Item -Force:$false foo.txt"
])));
}

#[test]
fn cmd_start_with_url_is_dangerous() {
assert!(is_dangerous_command_windows(&vec_str(&[
Expand All @@ -298,6 +394,20 @@ mod tests {
])));
}

#[test]
fn cmd_del_force_is_dangerous() {
assert!(is_dangerous_command_windows(&vec_str(&[
"cmd", "/c", "del", "/f", "foo.txt"
])));
}

#[test]
fn cmd_del_without_force_is_not_flagged() {
assert!(!is_dangerous_command_windows(&vec_str(&[
"cmd", "/c", "del", "foo.txt"
])));
}

#[test]
fn msedge_with_url_is_dangerous() {
assert!(is_dangerous_command_windows(&vec_str(&[
Expand Down
Loading