From b1905d3754ecf2de042876e4090b2ade776df85f Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 12 Dec 2025 16:14:12 -0800 Subject: [PATCH 1/2] fix: added test helpers for platform-specific paths (#7954) This addresses post-merge feedback from https://github.com/openai/codex/pull/7856. --- codex-rs/Cargo.lock | 2 +- codex-rs/app-server/Cargo.toml | 1 - codex-rs/app-server/tests/common/lib.rs | 3 ++ codex-rs/app-server/tests/suite/config.rs | 15 ++----- .../app-server/tests/suite/v2/config_rpc.rs | 24 ++++------ codex-rs/core/src/config/mod.rs | 18 +++----- codex-rs/core/src/environment_context.rs | 21 ++++++--- codex-rs/core/tests/common/Cargo.toml | 1 + codex-rs/core/tests/common/lib.rs | 45 +++++++++++++++++++ 9 files changed, 84 insertions(+), 46 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 523347d5db..a949ce94e9 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -837,7 +837,6 @@ dependencies = [ "codex-login", "codex-protocol", "codex-rmcp-client", - "codex-utils-absolute-path", "codex-utils-json-to-toml", "core_test_support", "mcp-types", @@ -1887,6 +1886,7 @@ dependencies = [ "base64", "codex-core", "codex-protocol", + "codex-utils-absolute-path", "notify", "regex-lite", "serde_json", diff --git a/codex-rs/app-server/Cargo.toml b/codex-rs/app-server/Cargo.toml index 211902d4ac..cb3f51bb82 100644 --- a/codex-rs/app-server/Cargo.toml +++ b/codex-rs/app-server/Cargo.toml @@ -27,7 +27,6 @@ codex-protocol = { workspace = true } codex-app-server-protocol = { workspace = true } codex-feedback = { workspace = true } codex-rmcp-client = { workspace = true } -codex-utils-absolute-path = { workspace = true } codex-utils-json-to-toml = { workspace = true } chrono = { workspace = true } serde = { workspace = true, features = ["derive"] } diff --git a/codex-rs/app-server/tests/common/lib.rs b/codex-rs/app-server/tests/common/lib.rs index a095a713a0..f3950595be 100644 --- a/codex-rs/app-server/tests/common/lib.rs +++ b/codex-rs/app-server/tests/common/lib.rs @@ -14,6 +14,9 @@ pub use core_test_support::format_with_current_shell; pub use core_test_support::format_with_current_shell_display; pub use core_test_support::format_with_current_shell_display_non_login; pub use core_test_support::format_with_current_shell_non_login; +pub use core_test_support::test_path_buf_with_windows; +pub use core_test_support::test_tmp_path; +pub use core_test_support::test_tmp_path_buf; pub use mcp_process::McpProcess; pub use mock_model_server::create_mock_chat_completions_server; pub use mock_model_server::create_mock_chat_completions_server_unchecked; diff --git a/codex-rs/app-server/tests/suite/config.rs b/codex-rs/app-server/tests/suite/config.rs index 41d9e8d68a..84b268a3c2 100644 --- a/codex-rs/app-server/tests/suite/config.rs +++ b/codex-rs/app-server/tests/suite/config.rs @@ -1,5 +1,6 @@ use anyhow::Result; use app_test_support::McpProcess; +use app_test_support::test_tmp_path; use app_test_support::to_response; use codex_app_server_protocol::GetUserSavedConfigResponse; use codex_app_server_protocol::JSONRPCResponse; @@ -14,7 +15,6 @@ use codex_protocol::config_types::ReasoningSummary; use codex_protocol::config_types::SandboxMode; use codex_protocol::config_types::Verbosity; use codex_protocol::openai_models::ReasoningEffort; -use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use std::collections::HashMap; use std::path::Path; @@ -24,11 +24,7 @@ use tokio::time::timeout; const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); fn create_config_toml(codex_home: &Path) -> std::io::Result<()> { - let writable_root = if cfg!(windows) { - r"C:\Users\codex\AppData\Local\Temp" - } else { - "/tmp" - }; + let writable_root = test_tmp_path(); let config_toml = codex_home.join("config.toml"); std::fs::write( config_toml, @@ -84,12 +80,7 @@ async fn get_config_toml_parses_all_fields() -> Result<()> { .await??; let config: GetUserSavedConfigResponse = to_response(resp)?; - let writable_root = if cfg!(windows) { - r"C:\Users\codex\AppData\Local\Temp" - } else { - "/tmp" - }; - let writable_root = AbsolutePathBuf::from_absolute_path(writable_root)?; + let writable_root = test_tmp_path(); let expected = GetUserSavedConfigResponse { config: UserSavedConfig { approval_policy: Some(AskForApproval::OnRequest), diff --git a/codex-rs/app-server/tests/suite/v2/config_rpc.rs b/codex-rs/app-server/tests/suite/v2/config_rpc.rs index 9b974773cb..199d2f88e6 100644 --- a/codex-rs/app-server/tests/suite/v2/config_rpc.rs +++ b/codex-rs/app-server/tests/suite/v2/config_rpc.rs @@ -1,5 +1,7 @@ use anyhow::Result; use app_test_support::McpProcess; +use app_test_support::test_path_buf_with_windows; +use app_test_support::test_tmp_path_buf; use app_test_support::to_response; use codex_app_server_protocol::AskForApproval; use codex_app_server_protocol::ConfigBatchWriteParams; @@ -18,7 +20,6 @@ use codex_app_server_protocol::ToolsV2; use codex_app_server_protocol::WriteStatus; use pretty_assertions::assert_eq; use serde_json::json; -use std::path::PathBuf; use tempfile::TempDir; use tokio::time::timeout; @@ -135,11 +136,8 @@ view_image = false #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn config_read_includes_system_layer_and_overrides() -> Result<()> { let codex_home = TempDir::new()?; - let (user_dir, system_dir) = if cfg!(windows) { - (r"C:\Users\user", r"C:\System") - } else { - ("/user", "/system") - }; + let user_dir = test_path_buf_with_windows("/user", Some(r"C:\Users\user")); + let system_dir = test_path_buf_with_windows("/system", Some(r"C:\System")); write_config( &codex_home, &format!( @@ -167,7 +165,7 @@ approval_policy = "never" [sandbox_workspace_write] writable_roots = [{}] "#, - serde_json::json!(system_dir) + serde_json::json!(system_dir.clone()) ), )?; @@ -218,7 +216,7 @@ writable_roots = [{}] .sandbox_workspace_write .as_ref() .expect("sandbox workspace write"); - assert_eq!(sandbox.writable_roots, vec![PathBuf::from(system_dir)]); + assert_eq!(sandbox.writable_roots, vec![system_dir]); assert_eq!( origins .get("sandbox_workspace_write.writable_roots.0") @@ -361,11 +359,7 @@ async fn config_batch_write_applies_multiple_edits() -> Result<()> { let mut mcp = McpProcess::new(codex_home.path()).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; - let writable_root = if cfg!(windows) { - r"C:\Users\codex\AppData\Local\Temp" - } else { - "/tmp" - }; + let writable_root = test_tmp_path_buf(); let batch_id = mcp .send_config_batch_write_request(ConfigBatchWriteParams { file_path: Some(codex_home.path().join("config.toml").display().to_string()), @@ -378,7 +372,7 @@ async fn config_batch_write_applies_multiple_edits() -> Result<()> { ConfigEdit { key_path: "sandbox_workspace_write".to_string(), value: json!({ - "writable_roots": [writable_root], + "writable_roots": [writable_root.clone()], "network_access": false }), merge_strategy: MergeStrategy::Replace, @@ -420,7 +414,7 @@ async fn config_batch_write_applies_multiple_edits() -> Result<()> { .sandbox_workspace_write .as_ref() .expect("sandbox workspace write"); - assert_eq!(sandbox.writable_roots, vec![PathBuf::from(writable_root)]); + assert_eq!(sandbox.writable_roots, vec![writable_root]); assert!(!sandbox.network_access); Ok(()) diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 95a9894ac0..df46a5fdd5 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1315,6 +1315,7 @@ mod tests { use crate::features::Feature; use super::*; + use core_test_support::test_absolute_path; use pretty_assertions::assert_eq; use std::time::Duration; @@ -1413,11 +1414,7 @@ network_access = true # This should be ignored. } ); - let writable_root = if cfg!(windows) { - "C:\\my\\workspace" - } else { - "/my/workspace" - }; + let writable_root = test_absolute_path("/my/workspace"); let sandbox_workspace_write = format!( r#" sandbox_mode = "workspace-write" @@ -1429,7 +1426,7 @@ writable_roots = [ exclude_tmpdir_env_var = true exclude_slash_tmp = true "#, - serde_json::json!(writable_root.to_string_lossy()) + serde_json::json!(writable_root) ); let sandbox_workspace_write_cfg = toml::from_str::(&sandbox_workspace_write) @@ -1453,7 +1450,7 @@ exclude_slash_tmp = true resolution, SandboxPolicyResolution { policy: SandboxPolicy::WorkspaceWrite { - writable_roots: vec!["/my/workspace".try_into().unwrap()], + writable_roots: vec![writable_root.clone()], network_access: false, exclude_tmpdir_env_var: true, exclude_slash_tmp: true, @@ -1477,7 +1474,7 @@ exclude_slash_tmp = true [projects."/tmp/test"] trust_level = "trusted" "#, - serde_json::json!(writable_root.to_string_lossy()) + serde_json::json!(writable_root) ); let sandbox_workspace_write_cfg = toml::from_str::(&sandbox_workspace_write) @@ -1501,10 +1498,7 @@ trust_level = "trusted" resolution, SandboxPolicyResolution { policy: SandboxPolicy::WorkspaceWrite { - writable_roots: vec![ - AbsolutePathBuf::from_absolute_path("/my/workspace") - .expect("absolute path") - ], + writable_roots: vec![writable_root], network_access: false, exclude_tmpdir_env_var: true, exclude_slash_tmp: true, diff --git a/codex-rs/core/src/environment_context.rs b/codex-rs/core/src/environment_context.rs index b0ae385c1d..fc4ae174df 100644 --- a/codex-rs/core/src/environment_context.rs +++ b/codex-rs/core/src/environment_context.rs @@ -192,6 +192,8 @@ mod tests { use crate::shell::ShellType; use super::*; + use core_test_support::test_path_buf; + use core_test_support::test_tmp_path_buf; use pretty_assertions::assert_eq; fn fake_shell() -> Shell { @@ -216,12 +218,19 @@ mod tests { #[test] fn serialize_workspace_write_environment_context() { - let cwd = if cfg!(windows) { "C:\\repo" } else { "/repo" }; - let writable_root = if cfg!(windows) { "C:\\tmp" } else { "/tmp" }; + let cwd = test_path_buf("/repo"); + let writable_root = test_tmp_path_buf(); + let cwd_str = cwd.to_str().expect("cwd is valid utf-8"); + let writable_root_str = writable_root + .to_str() + .expect("writable root is valid utf-8"); let context = EnvironmentContext::new( - Some(PathBuf::from(cwd)), + Some(cwd.clone()), Some(AskForApproval::OnRequest), - Some(workspace_write_policy(vec![cwd, writable_root], false)), + Some(workspace_write_policy( + vec![cwd_str, writable_root_str], + false, + )), fake_shell(), ); @@ -236,7 +245,9 @@ mod tests { {writable_root} bash -"# +"#, + cwd = cwd.display(), + writable_root = writable_root.display(), ); assert_eq!(context.serialize_to_xml(), expected); diff --git a/codex-rs/core/tests/common/Cargo.toml b/codex-rs/core/tests/common/Cargo.toml index 09da4bc701..95ea788b4c 100644 --- a/codex-rs/core/tests/common/Cargo.toml +++ b/codex-rs/core/tests/common/Cargo.toml @@ -13,6 +13,7 @@ assert_cmd = { workspace = true } base64 = { workspace = true } codex-core = { workspace = true, features = ["test-support"] } codex-protocol = { workspace = true } +codex-utils-absolute-path = { workspace = true } notify = { workspace = true } regex-lite = { workspace = true } serde_json = { workspace = true } diff --git a/codex-rs/core/tests/common/lib.rs b/codex-rs/core/tests/common/lib.rs index 7b3c53cd30..61bac30cb1 100644 --- a/codex-rs/core/tests/common/lib.rs +++ b/codex-rs/core/tests/common/lib.rs @@ -6,7 +6,9 @@ use codex_core::CodexConversation; use codex_core::config::Config; use codex_core::config::ConfigOverrides; use codex_core::config::ConfigToml; +use codex_utils_absolute_path::AbsolutePathBuf; use regex_lite::Regex; +use std::path::PathBuf; #[cfg(target_os = "linux")] use assert_cmd::cargo::cargo_bin; @@ -25,6 +27,49 @@ pub fn assert_regex_match<'s>(pattern: &str, actual: &'s str) -> regex_lite::Cap .unwrap_or_else(|| panic!("regex {pattern:?} did not match {actual:?}")) } +pub fn test_path_buf_with_windows(unix_path: &str, windows_path: Option<&str>) -> PathBuf { + if cfg!(windows) { + if let Some(windows) = windows_path { + PathBuf::from(windows) + } else { + let mut path = PathBuf::from(r"C:\"); + path.extend( + unix_path + .trim_start_matches('/') + .split('/') + .filter(|segment| !segment.is_empty()), + ); + path + } + } else { + PathBuf::from(unix_path) + } +} + +pub fn test_path_buf(unix_path: &str) -> PathBuf { + test_path_buf_with_windows(unix_path, None) +} + +pub fn test_absolute_path_with_windows( + unix_path: &str, + windows_path: Option<&str>, +) -> AbsolutePathBuf { + AbsolutePathBuf::from_absolute_path(test_path_buf_with_windows(unix_path, windows_path)) + .expect("test path should be absolute") +} + +pub fn test_absolute_path(unix_path: &str) -> AbsolutePathBuf { + test_absolute_path_with_windows(unix_path, None) +} + +pub fn test_tmp_path() -> AbsolutePathBuf { + test_absolute_path_with_windows("/tmp", Some(r"C:\Users\codex\AppData\Local\Temp")) +} + +pub fn test_tmp_path_buf() -> PathBuf { + test_tmp_path().into_path_buf() +} + /// Returns a default `Config` whose on-disk state is confined to the provided /// temporary directory. Using a per-test directory keeps tests hermetic and /// avoids clobbering a developer’s real `~/.codex`. From 711ee57b57ec9ae4ffac7235c3db163cde1edb4a Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 12 Dec 2025 16:52:24 -0800 Subject: [PATCH 2/2] fix: consolidate logic related to checking list of safe commands I noted that the logic for determining whether an `rg` or `git` invocation was safe diverged between `is_safe_command.rs` and `windows_safe_commands.rs`, so this PR reconciles the two in a shared `shared_rules.rs` file. --- .../src/command_safety/is_safe_command.rs | 63 +-------- codex-rs/core/src/command_safety/mod.rs | 1 + .../core/src/command_safety/shared_rules.rs | 129 ++++++++++++++++++ .../command_safety/windows_safe_commands.rs | 63 +-------- 4 files changed, 138 insertions(+), 118 deletions(-) create mode 100644 codex-rs/core/src/command_safety/shared_rules.rs diff --git a/codex-rs/core/src/command_safety/is_safe_command.rs b/codex-rs/core/src/command_safety/is_safe_command.rs index 01a52026e2..2e42244875 100644 --- a/codex-rs/core/src/command_safety/is_safe_command.rs +++ b/codex-rs/core/src/command_safety/is_safe_command.rs @@ -1,4 +1,6 @@ use crate::bash::parse_shell_lc_plain_commands; +use crate::command_safety::shared_rules::is_safe_git_command; +use crate::command_safety::shared_rules::is_safe_ripgrep_command; use crate::command_safety::windows_safe_commands::is_safe_command_windows; pub fn is_known_safe_command(command: &[String]) -> bool { @@ -108,33 +110,10 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool { } // Ripgrep - Some("rg") => { - const UNSAFE_RIPGREP_OPTIONS_WITH_ARGS: &[&str] = &[ - // Takes an arbitrary command that is executed for each match. - "--pre", - // Takes a command that can be used to obtain the local hostname. - "--hostname-bin", - ]; - const UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS: &[&str] = &[ - // Calls out to other decompression tools, so do not auto-approve - // out of an abundance of caution. - "--search-zip", - "-z", - ]; - - !command.iter().any(|arg| { - UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS.contains(&arg.as_str()) - || UNSAFE_RIPGREP_OPTIONS_WITH_ARGS - .iter() - .any(|&opt| arg == opt || arg.starts_with(&format!("{opt}="))) - }) - } + Some("rg") => is_safe_ripgrep_command(command), // Git - Some("git") => matches!( - command.get(1).map(String::as_str), - Some("branch" | "status" | "log" | "diff" | "show") - ), + Some("git") => is_safe_git_command(command), // Rust Some("cargo") if command.get(1).map(String::as_str) == Some("check") => true, @@ -280,40 +259,6 @@ mod tests { } } - #[test] - fn ripgrep_rules() { - // Safe ripgrep invocations – none of the unsafe flags are present. - assert!(is_safe_to_call_with_exec(&vec_str(&[ - "rg", - "Cargo.toml", - "-n" - ]))); - - // Unsafe flags that do not take an argument (present verbatim). - for args in [ - vec_str(&["rg", "--search-zip", "files"]), - vec_str(&["rg", "-z", "files"]), - ] { - assert!( - !is_safe_to_call_with_exec(&args), - "expected {args:?} to be considered unsafe due to zip-search flag", - ); - } - - // Unsafe flags that expect a value, provided in both split and = forms. - for args in [ - vec_str(&["rg", "--pre", "pwned", "files"]), - vec_str(&["rg", "--pre=pwned", "files"]), - vec_str(&["rg", "--hostname-bin", "pwned", "files"]), - vec_str(&["rg", "--hostname-bin=pwned", "files"]), - ] { - assert!( - !is_safe_to_call_with_exec(&args), - "expected {args:?} to be considered unsafe due to external-command flag", - ); - } - } - #[test] fn windows_powershell_full_path_is_safe() { if !cfg!(windows) { diff --git a/codex-rs/core/src/command_safety/mod.rs b/codex-rs/core/src/command_safety/mod.rs index caf5c9f6ea..bd226d1356 100644 --- a/codex-rs/core/src/command_safety/mod.rs +++ b/codex-rs/core/src/command_safety/mod.rs @@ -1,3 +1,4 @@ pub mod is_dangerous_command; pub mod is_safe_command; +mod shared_rules; pub mod windows_safe_commands; diff --git a/codex-rs/core/src/command_safety/shared_rules.rs b/codex-rs/core/src/command_safety/shared_rules.rs new file mode 100644 index 0000000000..bd3ee8157f --- /dev/null +++ b/codex-rs/core/src/command_safety/shared_rules.rs @@ -0,0 +1,129 @@ +//! Shared safelist logic for commands that are allowed on both Unix and Windows. +//! Keep these helpers read-only and conservative. + +/// Ensures the ripgrep invocation does not contain flags that could run +/// arbitrary commands. +pub(crate) fn is_safe_ripgrep_command>(words: &[S]) -> bool { + const UNSAFE_RIPGREP_OPTIONS_WITH_ARGS: &[&str] = &[ + // Takes an arbitrary command that is executed for each match. + "--pre", + // Takes a command that can be used to obtain the local hostname. + "--hostname-bin", + ]; + const UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS: &[&str] = &[ + // Calls out to other decompression tools, so do not auto-approve out of + // an abundance of caution. + "--search-zip", + "-z", + ]; + + !words.iter().skip(1).any(|arg| { + let arg_lc = arg.as_ref().to_ascii_lowercase(); + UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS.contains(&arg_lc.as_str()) + || UNSAFE_RIPGREP_OPTIONS_WITH_ARGS + .iter() + .any(|opt| arg_lc == *opt || arg_lc.starts_with(&format!("{opt}="))) + }) +} + +/// Ensures a Git command sticks to whitelisted read-only subcommands and flags. +pub(crate) fn is_safe_git_command>(words: &[S]) -> bool { + const SAFE_SUBCOMMANDS: &[&str] = &["status", "log", "show", "diff", "cat-file"]; + + let mut iter = words.iter().skip(1); + while let Some(arg) = iter.next() { + let arg = arg.as_ref(); + let arg_lc = arg.to_ascii_lowercase(); + + if arg.starts_with('-') { + if arg.eq_ignore_ascii_case("-c") || arg.eq_ignore_ascii_case("--config") { + if iter.next().is_none() { + return false; + } + continue; + } + + if arg_lc.starts_with("-c=") + || arg_lc.starts_with("--config=") + || arg_lc.starts_with("--git-dir=") + || arg_lc.starts_with("--work-tree=") + { + continue; + } + + if arg.eq_ignore_ascii_case("--git-dir") || arg.eq_ignore_ascii_case("--work-tree") { + if iter.next().is_none() { + return false; + } + continue; + } + + continue; + } + + return SAFE_SUBCOMMANDS.contains(&arg_lc.as_str()); + } + + false +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn ripgrep_rules() { + // Safe ripgrep invocations – none of the unsafe flags are present. + assert!(is_safe_ripgrep_command(&["rg", "Cargo.toml", "-n"])); + + // Unsafe flags that do not take an argument (present verbatim). + for args in [ + &["rg", "--search-zip", "files"][..], + &["rg", "-z", "files"][..], + ] { + assert!( + !is_safe_ripgrep_command(args), + "expected {args:?} to be considered unsafe due to zip-search flag", + ); + } + + // Unsafe flags that expect a value, provided in both split and = forms. + for args in [ + &["rg", "--pre", "pwned", "files"][..], + &["rg", "--pre=pwned", "files"][..], + &["rg", "--hostname-bin", "pwned", "files"][..], + &["rg", "--hostname-bin=pwned", "files"][..], + ] { + assert!( + !is_safe_ripgrep_command(args), + "expected {args:?} to be considered unsafe due to external-command flag", + ); + } + } + + #[test] + fn git_rules() { + assert!(is_safe_git_command(&["git", "status"])); + assert!(is_safe_git_command(&[ + "git", + "-c", + "core.pager=cat", + "show", + "HEAD:foo.rs" + ])); + assert!(is_safe_git_command(&[ + "git", + "--work-tree=.", + "--git-dir=.git", + "diff" + ])); + + assert!(!is_safe_git_command(&["git"])); + assert!(!is_safe_git_command(&["git", "branch"])); + assert!(!is_safe_git_command(&["git", "fetch"])); + assert!(!is_safe_git_command(&["git", "-c"])); + assert!(!is_safe_git_command(&["git", "--config"])); + assert!(!is_safe_git_command(&["git", "--git-dir"])); + assert!(!is_safe_git_command(&["git", "--work-tree"])); + } +} diff --git a/codex-rs/core/src/command_safety/windows_safe_commands.rs b/codex-rs/core/src/command_safety/windows_safe_commands.rs index ac479a4d2c..daeae7cd92 100644 --- a/codex-rs/core/src/command_safety/windows_safe_commands.rs +++ b/codex-rs/core/src/command_safety/windows_safe_commands.rs @@ -5,6 +5,9 @@ use std::path::Path; use std::process::Command; use std::sync::LazyLock; +use crate::command_safety::shared_rules::is_safe_git_command; +use crate::command_safety::shared_rules::is_safe_ripgrep_command; + const POWERSHELL_PARSER_SCRIPT: &str = include_str!("powershell_parser.ps1"); /// On Windows, we conservatively allow only clearly read-only PowerShell invocations @@ -270,7 +273,7 @@ fn is_safe_powershell_command(words: &[String]) -> bool { "git" => is_safe_git_command(words), - "rg" => is_safe_ripgrep(words), + "rg" => is_safe_ripgrep_command(words), // Extra safety: explicitly prohibit common side-effecting cmdlets regardless of args. "set-content" | "add-content" | "out-file" | "new-item" | "remove-item" | "move-item" @@ -286,64 +289,6 @@ fn is_safe_powershell_command(words: &[String]) -> bool { } } -/// Checks that an `rg` invocation avoids options that can spawn arbitrary executables. -fn is_safe_ripgrep(words: &[String]) -> bool { - const UNSAFE_RIPGREP_OPTIONS_WITH_ARGS: &[&str] = &["--pre", "--hostname-bin"]; - const UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS: &[&str] = &["--search-zip", "-z"]; - - !words.iter().skip(1).any(|arg| { - let arg_lc = arg.to_ascii_lowercase(); - // Examples rejected here: "pwsh -Command 'rg --pre cat pattern'" and "pwsh -Command 'rg --search-zip pattern'". - UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS.contains(&arg_lc.as_str()) - || UNSAFE_RIPGREP_OPTIONS_WITH_ARGS - .iter() - .any(|opt| arg_lc == *opt || arg_lc.starts_with(&format!("{opt}="))) - }) -} - -/// Ensures a Git command sticks to whitelisted read-only subcommands and flags. -fn is_safe_git_command(words: &[String]) -> bool { - const SAFE_SUBCOMMANDS: &[&str] = &["status", "log", "show", "diff", "cat-file"]; - - let mut iter = words.iter().skip(1); - while let Some(arg) = iter.next() { - let arg_lc = arg.to_ascii_lowercase(); - - if arg.starts_with('-') { - if arg.eq_ignore_ascii_case("-c") || arg.eq_ignore_ascii_case("--config") { - if iter.next().is_none() { - // Examples rejected here: "pwsh -Command 'git -c'" and "pwsh -Command 'git --config'". - return false; - } - continue; - } - - if arg_lc.starts_with("-c=") - || arg_lc.starts_with("--config=") - || arg_lc.starts_with("--git-dir=") - || arg_lc.starts_with("--work-tree=") - { - continue; - } - - if arg.eq_ignore_ascii_case("--git-dir") || arg.eq_ignore_ascii_case("--work-tree") { - if iter.next().is_none() { - // Examples rejected here: "pwsh -Command 'git --git-dir'" and "pwsh -Command 'git --work-tree'". - return false; - } - continue; - } - - continue; - } - - return SAFE_SUBCOMMANDS.contains(&arg_lc.as_str()); - } - - // Examples rejected here: "pwsh -Command 'git'" and "pwsh -Command 'git status --short | Remove-Item foo'". - false -} - #[cfg(all(test, windows))] mod tests { use super::*;