hanzoai/dev
2
0
Fork 0
mirror of https://github.com/hanzoai/dev.git synced 2026-10-25 03:32:32 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(exec): detect absolute bash and flag checkout -- paths

Browse Source
This commit is contained in:
James Peter
2025-09-16 13:52:02 +10:00
parent 84fbdda104
commit 4744c220fe
5 changed files with 158 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/apply-patch/src/lib.rs
+31 -2
View File
@@ -32,6 +32,18 @@ pub const APPLY_PATCH_TOOL_INSTRUCTIONS: &str = include_str!("../apply_patch_too
const APPLY_PATCH_COMMANDS: [&str; 2] = ["apply_patch", "applypatch"];
fn is_bash_like(cmd: &str) -> bool {
let trimmed = cmd.trim_matches('"').trim_matches('\'');
if trimmed.eq_ignore_ascii_case("bash") || trimmed.eq_ignore_ascii_case("bash.exe") {
return true;
}
std::path::Path::new(trimmed)
.file_name()
.and_then(|s| s.to_str())
.map(|name| name.eq_ignore_ascii_case("bash") || name.eq_ignore_ascii_case("bash.exe"))
.unwrap_or(false)
}
/// Details about an embedded `apply_patch` heredoc found inside a larger shell script.
#[derive(Debug, Clone, PartialEq)]
pub struct EmbeddedApplyPatch {
@@ -127,7 +139,7 @@ pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch {
Err(e) => MaybeApplyPatch::PatchParseError(e),
},
// Bash heredoc form: (optional `cd <path> &&`) apply_patch <<'EOF' ...
[bash, flag, script] if bash == "bash" && flag == "-lc" => {
[bash, flag, script] if is_bash_like(bash) && flag == "-lc" => {
match extract_apply_patch_from_bash(script) {
Ok((body, _maybe_cd)) => match parse_patch(&body) {
Ok(source) => MaybeApplyPatch::Body(source),
@@ -244,7 +256,7 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApp
);
}
}
[bash, flag, script] if bash == "bash" && flag == "-lc" => {
[bash, flag, script] if is_bash_like(bash) && flag == "-lc" => {
if parse_patch(script).is_ok() {
return MaybeApplyPatchVerified::CorrectnessError(
ApplyPatchError::ImplicitInvocation,
@@ -962,6 +974,23 @@ mod tests {
));
}
#[test]
fn test_absolute_bash_detects_apply_patch() {
let script = heredoc_script("");
let args = vec!["/bin/bash".to_string(), "-lc".to_string(), script.clone()];
match maybe_parse_apply_patch(&args) {
MaybeApplyPatch::Body(ApplyPatchArgs { hunks, .. }) => {
assert_eq!(hunks, expected_single_add());
}
other => panic!("expected MaybeApplyPatch::Body got {other:?}"),
}
let dir = tempdir().unwrap();
assert!(matches!(
maybe_parse_apply_patch_verified(&args, dir.path()),
MaybeApplyPatchVerified::Body(_)
));
}
#[test]
fn test_implicit_patch_single_arg_is_error() {
let patch = "*** Begin Patch\n*** Add File: foo\n+hi\n*** End Patch".to_string();
codex-rs/core/src/codex.rs
+68 -42
View File
@@ -4754,7 +4754,12 @@ async fn handle_container_exec_with_params(
}
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
enum SensitiveGitKind { BranchChange, Reset, Revert }
enum SensitiveGitKind {
BranchChange,
PathCheckout,
Reset,
Revert,
}
fn detect_sensitive_git(script: &str) -> Option<SensitiveGitKind> {
// Goal: detect sensitive git invocations (branch changes, resets) while
@@ -4860,9 +4865,14 @@ async fn handle_container_exec_with_params(
i += 1;
match sub {
"checkout" => {
let args: Vec<&str> = raw_tokens[i..].iter().map(|t| strip_tok(t)).collect();
let has_path_delimiter = args.iter().any(|a| *a == "--");
if has_path_delimiter {
return Some(SensitiveGitKind::PathCheckout);
}
// If any of the strong branch-changing flags are present, flag it.
let mut saw_branch_change_flag = false;
let args: Vec<&str> = raw_tokens[i..].iter().map(|t| strip_tok(t)).collect();
for a in &args {
if matches!(*a, "-b" | "-B" | "--orphan" | "--detach") {
saw_branch_change_flag = true;
@@ -4870,13 +4880,13 @@ async fn handle_container_exec_with_params(
}
}
if saw_branch_change_flag { return Some(SensitiveGitKind::BranchChange); }
// If `--` is present, this is a path checkout, not branch.
if args.iter().any(|a| *a == "--") { continue; }
// `git checkout -` switches to previous branch.
if args.first().copied() == Some("-") { return Some(SensitiveGitKind::BranchChange); }
if args.first().copied() == Some("-") {
return Some(SensitiveGitKind::BranchChange);
}
// Heuristic: a single non-flag argument likely denotes a branch.
// To reduce false positives (e.g. `git checkout .`), only flag
// when the first arg does not start with '-' and is not a solitary '.' or '..'.
if let Some(first_arg) = args.first() {
let a = *first_arg;
if !a.starts_with('-') && a != "." && a != ".." {
@@ -4916,6 +4926,35 @@ async fn handle_container_exec_with_params(
None
}
fn guidance_for_sensitive_git(kind: SensitiveGitKind, original_label: &str, original_value: &str, suggested: &str) -> String {
match kind {
SensitiveGitKind::BranchChange => format!(
"Blocked git checkout/switch on a branch. Switching branches can discard or hide in-progress changes. Only continue if the user explicitly requested this branch change. Resend with 'confirm:' if you intend to proceed.\n\n{}: {}\nresend_exact_argv: {}",
original_label,
original_value,
suggested
),
SensitiveGitKind::PathCheckout => format!(
"Blocked git checkout -- <paths>. This command overwrites local modifications to the specified files. If you intentionally want to discard those edits, resend the exact command prefixed with 'confirm:'.\n\n{}: {}\nresend_exact_argv: {}",
original_label,
original_value,
suggested
),
SensitiveGitKind::Reset => format!(
"Blocked git reset. Reset rewrites the working tree/index and may delete local work. If backups exist and this was explicitly requested, resend prefixed with 'confirm:'.\n\n{}: {}\nresend_exact_argv: {}",
original_label,
original_value,
suggested
),
SensitiveGitKind::Revert => format!(
"Blocked git revert. Reverting commits alters history and should only happen when the user asks for it. If that’s the case, resend the command with 'confirm:'.\n\n{}: {}\nresend_exact_argv: {}",
original_label,
original_value,
suggested
),
}
}
// If the argv is a shell wrapper, analyze and optionally strip `confirm:`.
let mut params = params;
let mut seq_hint_for_exec = seq_hint;
@@ -4935,23 +4974,7 @@ async fn handle_container_exec_with_params(
let suggested = serde_json::to_string(&argv_confirm)
.unwrap_or_else(|_| "<failed to serialize suggested argv>".to_string());
let guidance = match kind {
SensitiveGitKind::BranchChange => format!(
"blocked potentially destructive git branch change. Git branching should only be performed when explicitly requested by the user. To proceed, resend the shell call with a confirmation prefix to indicate it was explicitly requested. Please use 'confirm:' to confirm it was requested.\n\noriginal_script: {}\nresend_exact_argv: {}",
script,
suggested
),
SensitiveGitKind::Reset => format!(
"git reset is potentially destructive and may overwrite changes you or other agents have made. It should be avoided unless absolutely necessary and code changes have been backed up. If this is the case, please prefix your command with 'confirm:', e.g., 'confirm: git reset'.\n\noriginal_script: {}\nresend_exact_argv: {}",
script,
suggested
),
SensitiveGitKind::Revert => format!(
"git revert modifies history and should only be used when the user explicitly asks. If you are certain this was requested, resend with 'confirm:' to proceed.\n\noriginal_script: {}\nresend_exact_argv: {}",
script,
suggested
),
};
let guidance = guidance_for_sensitive_git(kind, "original_script", &script, &suggested);
return ResponseInputItem::FunctionCallOutput {
call_id,
@@ -5147,8 +5170,27 @@ async fn handle_container_exec_with_params(
}
if i < params.command.len() {
let sub = strip_tok2(&params.command[i]);
let args: Vec<&str> = params.command[i + 1..].iter().map(|t| strip_tok2(t)).collect();
let kind = match sub {
"checkout" | "switch" => Some(SensitiveGitKind::BranchChange),
"checkout" => {
if args.iter().any(|a| *a == "--") {
Some(SensitiveGitKind::PathCheckout)
} else if args.iter().any(|a| matches!(*a, "-b" | "-B" | "--orphan" | "--detach")) {
Some(SensitiveGitKind::BranchChange)
} else if args.first().copied() == Some("-") {
Some(SensitiveGitKind::BranchChange)
} else if let Some(first_arg) = args.first() {
let a = *first_arg;
if !a.starts_with('-') && a != "." && a != ".." {
Some(SensitiveGitKind::BranchChange)
} else {
None
}
} else {
None
}
}
"switch" => Some(SensitiveGitKind::BranchChange),
"reset" => Some(SensitiveGitKind::Reset),
"revert" => Some(SensitiveGitKind::Revert),
_ => None,
@@ -5160,23 +5202,7 @@ async fn handle_container_exec_with_params(
format!("confirm: {}", params.command.join(" ")),
]).unwrap_or_else(|_| "<failed to serialize suggested argv>".to_string());
let guidance = match kind {
SensitiveGitKind::BranchChange => format!(
"blocked potentially destructive git branch change. Git branching should only be performed when explicitly requested by the user. To proceed, resend the shell call with a confirmation prefix to indicate it was explicitly requested. Please use 'confirm:' to confirm it was requested.\n\noriginal_argv: {:?}\nresend_example_argv: {}",
params.command,
suggested
),
SensitiveGitKind::Reset => format!(
"git reset is potentially destructive and may overwrite changes you or other agents have made. It should be avoided unless absolutely necessary and code changes have been backed up. If this is the case, please prefix your command with 'confirm:', e.g., 'confirm: git reset'.\n\noriginal_argv: {:?}\nresend_example_argv: {}",
params.command,
suggested
),
SensitiveGitKind::Revert => format!(
"git revert is potentially destructive and may overwrite changes you or other agents have made. It should be avoided unless absolutely necessary and code changes have been backed up. If this is the case, please prefix your command with 'confirm:', e.g., 'confirm: git revert'.\n\noriginal_argv: {:?}\nresend_example_argv: {}",
params.command,
suggested
),
};
let guidance = guidance_for_sensitive_git(kind, "original_argv", &format!("{:?}", params.command), &suggested);
return ResponseInputItem::FunctionCallOutput { call_id, output: FunctionCallOutputPayload { content: guidance, success: None } };
}
codex-rs/core/src/is_safe_command.rs
+14 -1
View File
@@ -1,5 +1,18 @@
use crate::bash::try_parse_bash;
use crate::bash::try_parse_word_only_commands_sequence;
use std::path::Path;
fn is_bash(cmd: &str) -> bool {
let trimmed = cmd.trim_matches('"').trim_matches('\'');
if trimmed.eq_ignore_ascii_case("bash") || trimmed.eq_ignore_ascii_case("bash.exe") {
return true;
}
Path::new(trimmed)
.file_name()
.and_then(|s| s.to_str())
.map(|name| name.eq_ignore_ascii_case("bash") || name.eq_ignore_ascii_case("bash.exe"))
.unwrap_or(false)
}
pub fn is_known_safe_command(command: &[String]) -> bool {
if is_safe_to_call_with_exec(command) {
@@ -13,7 +26,7 @@ pub fn is_known_safe_command(command: &[String]) -> bool {
// individual command in the script is itself a known‑safe command, then
// the composite expression is considered safe.
if let [bash, flag, script] = command {
if bash == "bash" && flag == "-lc" {
if is_bash(bash) && flag == "-lc" {
if let Some(tree) = try_parse_bash(script) {
if let Some(all_commands) =
try_parse_word_only_commands_sequence(&tree, script)
codex-rs/core/src/parse_command.rs
+31 -2
View File
@@ -4,6 +4,7 @@ use serde::Deserialize;
use serde::Serialize;
use shlex::split as shlex_split;
use shlex::try_join as shlex_try_join;
use std::path::Path;
#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)]
pub enum ParsedCommand {
@@ -44,6 +45,18 @@ fn shlex_join(tokens: &[String]) -> String {
.unwrap_or_else(|_| "<command included NUL byte>".to_string())
}
fn is_bash_executable(cmd: &str) -> bool {
let trimmed = cmd.trim_matches('"').trim_matches('\'');
if trimmed.eq_ignore_ascii_case("bash") || trimmed.eq_ignore_ascii_case("bash.exe") {
return true;
}
Path::new(trimmed)
.file_name()
.and_then(|s| s.to_str())
.map(|name| name.eq_ignore_ascii_case("bash") || name.eq_ignore_ascii_case("bash.exe"))
.unwrap_or(false)
}
/// DO NOT REVIEW THIS CODE BY HAND
/// This parsing code is quite complex and not easy to hand-modify.
/// The easiest way to iterate is to add unit tests and have Codex fix the implementation.
@@ -318,6 +331,22 @@ mod tests {
);
}
#[test]
fn supports_absolute_bash_sed_read() {
let args = vec![
"/bin/bash".to_string(),
"-lc".to_string(),
"sed -n '1,5p' README.md".to_string(),
];
assert_parsed(
&args,
vec![ParsedCommand::Read {
cmd: "sed -n '1,5p' README.md".to_string(),
name: "README.md".to_string(),
}],
);
}
#[test]
fn supports_npm_run_build_is_unknown() {
assert_parsed(
@@ -984,7 +1013,7 @@ fn normalize_tokens(cmd: &[String]) -> Vec<String> {
// Do not re-shlex already-tokenized input; just drop the prefix.
rest.to_vec()
}
[bash, flag, script] if bash == "bash" && (flag == "-c" || flag == "-lc") => {
[bash, flag, script] if is_bash_executable(bash) && (flag == "-c" || flag == "-lc") => {
shlex_split(script)
.unwrap_or_else(|| vec!["bash".to_string(), flag.clone(), script.clone()])
}
@@ -1146,7 +1175,7 @@ fn parse_bash_lc_commands(original: &[String]) -> Option<Vec<ParsedCommand>> {
let [bash, flag, script] = original else {
return None;
};
if bash != "bash" || flag != "-lc" {
if !is_bash_executable(bash) || flag != "-lc" {
return None;
}
if let Some(tree) = try_parse_bash(script) {
codex-rs/core/src/shell.rs
+14 -1
View File
@@ -1,6 +1,7 @@
use serde::Deserialize;
use serde::Serialize;
use shlex;
use std::path::Path;
use std::path::PathBuf;
#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)]
@@ -123,7 +124,7 @@ fn strip_bash_lc(command: &Vec<String>) -> Option<String> {
// exactly three items
[first, second, third]
// first two must be "bash", "-lc"
if first == "bash" && second == "-lc" =>
if is_bash_like(first) && second == "-lc" =>
{
Some(third.clone())
}
@@ -131,6 +132,18 @@ fn strip_bash_lc(command: &Vec<String>) -> Option<String> {
}
}
fn is_bash_like(cmd: &str) -> bool {
let trimmed = cmd.trim_matches('"').trim_matches('\'');
if trimmed.eq_ignore_ascii_case("bash") || trimmed.eq_ignore_ascii_case("bash.exe") {
return true;
}
Path::new(trimmed)
.file_name()
.and_then(|s| s.to_str())
.map(|name| name.eq_ignore_ascii_case("bash") || name.eq_ignore_ascii_case("bash.exe"))
.unwrap_or(false)
}
#[cfg(unix)]
fn detect_default_user_shell() -> Shell {
use libc::getpwuid;