Skip to content
Closed
Changes from 2 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
160 changes: 155 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,44 @@ 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)

// `cmd /c <gui-launcher> https://...` should be treated as dangerous even though the
// outer executable is `cmd`. Reuse the same heuristics we apply for direct launches.
let mut nested: Vec<String> = Vec::with_capacity(1 + cmd_args.len());
nested.push(first_cmd.to_string());
nested.extend(cmd_args.iter().cloned());
if is_direct_gui_launch(&nested) {
return true;
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 Detect nested PowerShell URL launches via cmd

After parsing cmd /c …, this branch only treats start, del/erase /f, or direct GUI launchers as dangerous. A command like cmd /c powershell -Command "Start-Process https://example.com" (or pwsh) contains a URL but won’t match is_direct_gui_launch, so is_dangerous_cmd returns false and approval is bypassed for a URL launch via PowerShell. This was previously caught by the generic URL-arg fallback and now regresses URL gating for nested PowerShell invocations on Windows.

Useful? React with 👍 / 👎.

}

false
}

fn is_direct_gui_launch(command: &[String]) -> bool {
Expand Down Expand Up @@ -288,6 +357,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 +403,51 @@ 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 cmd_msedge_with_url_is_dangerous() {
assert!(is_dangerous_command_windows(&vec_str(&[
"cmd",
"/c",
"msedge",
"https://example.com"
])));
}

#[test]
fn cmd_explorer_with_url_is_dangerous() {
assert!(is_dangerous_command_windows(&vec_str(&[
"cmd",
"/c",
"explorer.exe",
"https://example.com"
])));
}

#[test]
fn cmd_rundll32_fileprotocolhandler_with_url_is_dangerous() {
assert!(is_dangerous_command_windows(&vec_str(&[
"cmd",
"/c",
"rundll32",
"url.dll,fileprotocolhandler",
"https://example.com"
])));
}

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