Skip to content

🔒 Fix command injection vulnerability in command_exists #74

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
badMade wants to merge 2 commits into
base: main
Choose a base branch
from
Open

🔒 Fix command injection vulnerability in command_exists #74

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6da80a9
🔒 Fix command injection vulnerability in command_exists
badMade Apr 30, 2026
42e4e86
🔒 Fix command injection vulnerability in command_exists
badMade Apr 30, 2026
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
22 changes: 21 additions & 1 deletion rust/crates/tools/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5151,7 +5151,9 @@ fn detect_powershell_shell() -> std::io::Result<&'static str> {
fn command_exists(command: &str) -> bool {
std::process::Command::new("sh")
.arg("-c")
.arg(format!("command -v {command} >/dev/null 2>&1"))
.arg("command -v \"$1\" >/dev/null 2>&1")
.arg("--")
.arg(command)
Comment on lines 5151 to +5156

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

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

command_exists now avoids string interpolation (good), but it still hard-depends on invoking sh. On systems without sh (notably native Windows), this will always return false, causing detect_powershell_shell() to report PowerShell missing even when pwsh/powershell is present. Consider making command_exists cross-platform (e.g., PATH scan in Rust and/or a Windows-specific implementation using where) or at least cfg(unix)/cfg(windows) split so PowerShell detection works on Windows without MSYS/Git-Bash.

Copilot uses AI. Check for mistakes.
.status()
Comment on lines 5151 to 5157

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

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

command_exists still shells out to sh, so it will always return false on systems/environments where sh is not on PATH (even if the target command exists). It also adds process-spawn overhead for a simple PATH lookup. Consider implementing this without invoking a shell (e.g., iterate PATH with std::env::split_paths like rust/crates/runtime/src/sandbox.rs:280-283), which avoids both the sh dependency and the injection class entirely.

Copilot uses AI. Check for mistakes.
.map(|status| status.success())
.unwrap_or(false)
Expand Down Expand Up @@ -5351,6 +5353,24 @@ pub mod pdf_extract;

#[cfg(test)]
mod tests {

#[test]
fn test_command_exists_valid() {
assert!(super::command_exists("ls") || super::command_exists("dir"));
}
Comment on lines +5357 to +5360

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

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

These new tests assume a Unix-like environment (command_exists shells out to sh) and that ls/dir are available. As written they will fail on Windows (and potentially minimal environments). Consider gating with #[cfg(unix)] or choosing a command that’s guaranteed to exist given the implementation (e.g., sh itself).

Copilot uses AI. Check for mistakes.

#[test]
fn test_command_exists_invalid() {
assert!(!super::command_exists(
"nonexistentcommandthatshouldneverexist123"
));
}

#[test]
fn test_command_exists_injection() {
assert!(!super::command_exists("ls; echo injected"));
assert!(!super::command_exists("ls && echo injected"));
Comment on lines +5371 to +5372

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

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

The injection regression test uses ls; echo injected / ls && echo injected. In a hostile or unusual environment it’s possible (though unlikely) to have an executable literally named ls; echo injected, which would make this test fail even though the injection fix is correct. To make the test robust, use a definitely-nonexistent command name as the prefix (e.g., reuse nonexistentcommandthatshouldneverexist123 and append ; echo injected / && echo injected).

Suggested change
assert!(!super::command_exists("ls; echo injected"));
assert!(!super::command_exists("ls && echo injected"));
assert!(!super::command_exists(
"nonexistentcommandthatshouldneverexist123; echo injected"
));
assert!(!super::command_exists(
"nonexistentcommandthatshouldneverexist123 && echo injected"
));

Copilot uses AI. Check for mistakes.
}
Comment on lines +5369 to +5373

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

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

test_command_exists_injection only asserts the result is false. That can be a weak regression test because it would still pass even if the vulnerable code executed the injected portion but ultimately returned non-zero, and it could theoretically fail if an executable with that literal name exists in PATH. Consider asserting a side effect does not occur (e.g., try "sh; touch <tmpfile>" / "sh && touch <tmpfile>" and verify the tmpfile was not created) to prove no extra commands are executed.

Copilot uses AI. Check for mistakes.
use std::collections::BTreeMap;
use std::collections::BTreeSet;
use std::fs;
Expand Down
Loading