From b5bea876d5f3f5b6e7a354685bdec3825e948924 Mon Sep 17 00:00:00 2001 From: kewton Date: Sat, 3 Oct 2026 11:21:28 +0900 Subject: [PATCH 1/2] fix(tools): track cd/pushd cwd candidates in the bash write guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Bash write-target guard judged relative paths against the workspace root only, so `cd sub && tee link/f` (and readers such as `cd sub && cat link/secret`) traversed a `sub/link` symlink out of the workspace, `pushd`/`popd` were not recognized, and CDPATH could redirect a literal `cd`. Add a working-directory leaf module that builds the union of the directories the command could run in (root plus each determinable cd/pushd destination, appended not replaced), refuses a relative write that escapes from any candidate, refuses relative paths after an undecidable cd (CDPATH, loop, function), matches protected paths against the candidates, judges stage-2 relative reads the same way, and drops CDPATH from the `sh -c` environment. Two-point measurement: on the base tree the four new reject tests fail (e.g. "expected rejection: cd sub && tee link/f"); with the change all five pass. Mutation run (working_directory + wiring, 30-minute cap) was truncated after 10 of 94 mutants; the 10 observed survivors are equivalent under the test set and are explained in the worker report. 読み替え: the issue's problem table lists `cd sub && grep -r x link` as a read that must be rejected, but the referenced H-01 report defers bare words without `/` to #571. To satisfy the issue's 拒否する形 list, stage 2 also joins literal relative words onto every candidate; this only rejects paths that actually escape. 判断: `pushd` with no operand and `pushd ±N` record no working-directory target (they only rotate the stack); `cd` with no operand keeps its existing `~` rejection. 本文に無い指摘: `env cd sub && tee elink/f` is refused by the existing root-relative per-target check (root/elink escapes), not by the new candidate check. Co-authored-by: CommandCodeBot --- src/tools/bash.rs | 125 ++++++-- src/tools/bash_write_guard.rs | 170 ++++++++-- src/tools/bash_write_guard/command_prefix.rs | 2 + .../bash_write_guard/working_directory.rs | 300 ++++++++++++++++++ tests/issue568_bash_cwd_write_targets.rs | 209 ++++++++++++ 5 files changed, 765 insertions(+), 41 deletions(-) create mode 100644 src/tools/bash_write_guard/working_directory.rs create mode 100644 tests/issue568_bash_cwd_write_targets.rs diff --git a/src/tools/bash.rs b/src/tools/bash.rs index 30f8b914..43bbcc08 100644 --- a/src/tools/bash.rs +++ b/src/tools/bash.rs @@ -228,6 +228,7 @@ where let mut process = Command::new("sh"); process.arg("-c").arg(&command).current_dir(root); process.stdout(Stdio::piped()).stderr(Stdio::piped()); + remove_shell_cdpath(&mut process); let output = bounded_process::run_with_timeout_cancel_and_force( &mut process, timeout, @@ -782,32 +783,35 @@ pub fn path_confinement_rejection( message, }); } + let inspection = super::bash_write_guard::inspect_working_directory(command); for candidate in path_tokens::path_candidates(command) { - if bash_path_allowed(&candidate, &root, &raw_root) { + if !bash_path_allowed(&candidate, &root, &raw_root) { + return Some(path_reference_rejection(&candidate, &root, None)); + } + if candidate.starts_with('/') { continue; } - let nearest_relative = nearest_relative_form(&candidate, &root); - let guidance = workspace_relative_retry_guidance(&nearest_relative); - let root_display = root.to_string_lossy().to_string(); - let path_kind = if Path::new(&candidate).is_absolute() { - "absolute" - } else { - "relative" - }; - let reason = format!( - "{path_kind} path `{candidate}` resolves outside current workspace root `{root_display}`" - ); - return Some(BashPathConfinementRejection { - path: candidate.to_string(), - root: root_display.clone(), - nearest_relative: nearest_relative.clone(), - guidance: guidance.clone(), - operation: "path reference".to_string(), - reason, - message: format!( - "bash_path_confinement_error: rejected {path_kind} path `{candidate}` outside current workspace root `{root_display}`; use workspace-relative path `{nearest_relative}`; {guidance}" - ), - }); + if inspection.undecidable { + return Some(path_reference_rejection( + &candidate, + &root, + Some("follows a working directory change that cannot be determined"), + )); + } + for joined in inspection.relative_variants(&candidate) { + if super::path_guard::ensure_bash_write_target(&root, &joined).is_err() { + return Some(path_reference_rejection(&joined, &root, None)); + } + } + } + if inspection.has_working_directory() { + for word in &inspection.relative_words { + for joined in inspection.all_variants(word) { + if super::path_guard::ensure_bash_write_target(&root, &joined).is_err() { + return Some(path_reference_rejection(&joined, &root, None)); + } + } + } } None } @@ -816,6 +820,48 @@ pub fn workspace_relative_retry_guidance(nearest_relative: &str) -> String { format!("workspace相対で再実行せよ: {nearest_relative}") } +/// A shell consults `CDPATH` for a relative `cd` operand, which would let a +/// command leave the working directory the static guard modelled. The static +/// inspection cannot see the process environment, so the spawned shell never +/// inherits it. +fn remove_shell_cdpath(command: &mut Command) { + command.env_remove("CDPATH"); +} + +fn path_reference_rejection( + candidate: &str, + root: &Path, + working_directory_reason: Option<&str>, +) -> BashPathConfinementRejection { + let nearest_relative = nearest_relative_form(candidate, root); + let guidance = workspace_relative_retry_guidance(&nearest_relative); + let root_display = root.to_string_lossy().to_string(); + let path_kind = if Path::new(candidate).is_absolute() { + "absolute" + } else { + "relative" + }; + let reason = match working_directory_reason { + Some(suffix) => format!( + "{path_kind} path `{candidate}` {suffix} and resolves outside current workspace root `{root_display}`" + ), + None => format!( + "{path_kind} path `{candidate}` resolves outside current workspace root `{root_display}`" + ), + }; + BashPathConfinementRejection { + path: candidate.to_string(), + root: root_display.clone(), + nearest_relative: nearest_relative.clone(), + guidance: guidance.clone(), + operation: "path reference".to_string(), + reason, + message: format!( + "bash_path_confinement_error: rejected {path_kind} path `{candidate}` outside current workspace root `{root_display}`; use workspace-relative path `{nearest_relative}`; {guidance}" + ), + } +} + fn truncate_stream(value: &str) -> String { crate::util::excerpt_with_newline_marker( value, @@ -1005,6 +1051,39 @@ mod tests { use super::*; use std::sync::atomic::{AtomicBool, Ordering}; + #[test] + fn working_directory_shell_spawn_removes_cdpath() { + let mut command = Command::new("true"); + remove_shell_cdpath(&mut command); + assert!( + command + .get_envs() + .any(|(key, value)| key == "CDPATH" && value.is_none()), + "the shell that runs `sh -c` must not inherit CDPATH" + ); + } + + #[test] + fn working_directory_keeps_verify_auto_approval() { + let root = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(root.path().join("frontend")).unwrap(); + std::fs::create_dir_all(root.path().join("crates/x")).unwrap(); + for command in ["cd frontend && npm test", "cd crates/x && cargo test"] { + let plan = crate::planner::verify::normalize_runtime_bash_command_for_boundary( + command, + root.path(), + ) + .unwrap(); + assert!( + plan.segments.iter().all(|segment| matches!( + &segment.command, + crate::planner::verify::RuntimeNormalizedCommand::Verify(_) + )) && !has_recognized_workspace_mutation(&plan.normalized_command), + "{command} must stay auto-approved: {plan:?}" + ); + } + } + #[test] fn run_returns_nonzero_output_for_agent_feedback() { let dir = tempfile::tempdir().unwrap(); diff --git a/src/tools/bash_write_guard.rs b/src/tools/bash_write_guard.rs index c1bff07c..0f4f3727 100644 --- a/src/tools/bash_write_guard.rs +++ b/src/tools/bash_write_guard.rs @@ -2,6 +2,15 @@ use std::path::Path; mod ansi_c_quoting; mod command_prefix; +mod working_directory; + +pub(super) use working_directory::Inspection; + +/// Reads the working-directory candidates of a command for the second-stage +/// (`bash.rs`) path inspection. +pub(super) fn inspect_working_directory(command: &str) -> Inspection { + working_directory::inspect(command) +} #[derive(Debug, Clone, PartialEq, Eq)] pub(super) struct BashWriteConfinementRejection { @@ -29,7 +38,11 @@ pub(super) fn confinement_rejection( command: &str, root: &Path, ) -> Option { + let inspection = working_directory::inspect(command); for target in write_targets(command) { + if let Some(rejection) = working_directory_rejection(&target, &inspection, root) { + return Some(rejection); + } if target.operation == ansi_c_quoting::OPERATION { return Some(BashWriteConfinementRejection { reason: format!( @@ -83,6 +96,64 @@ pub(super) fn confinement_rejection( }); } } + for destination in &inspection.targets { + if let Err(error) = super::path_guard::ensure_bash_write_target(root, destination) { + return Some(BashWriteConfinementRejection { + reason: format!( + "Bash working directory target `{destination}` is outside the Gate 1 workspace boundary: {error}" + ), + path: destination.clone(), + operation: "working directory".to_string(), + }); + } + } + None +} + +/// Rejects a write target that the workspace root alone would allow but that a +/// `cd`/`pushd` candidate turns into an escape. Absolute, `~`, `$`, `-`, and +/// prefix-operation targets keep their existing handling. +fn working_directory_rejection( + target: &WriteTarget, + inspection: &working_directory::Inspection, + root: &Path, +) -> Option { + if matches!( + target.operation.as_str(), + "working directory" + | ansi_c_quoting::OPERATION + | command_prefix::UNRESOLVED_OPERATION + | command_prefix::UNVERIFIABLE_SPLIT_STRING_OPERATION + ) { + return None; + } + let path = &target.path; + if path.starts_with('/') || path.starts_with('~') || path.starts_with('$') || path == "-" { + return None; + } + if inspection.undecidable { + return Some(BashWriteConfinementRejection { + path: path.clone(), + operation: target.operation.clone(), + reason: format!( + "Bash {} target `{}` follows a working directory change that cannot be determined (CDPATH, loop, or function), so it cannot be proven to remain in the Gate 1 workspace boundary", + target.operation, path + ), + }); + } + for joined in inspection.relative_variants(path) { + if super::path_guard::ensure_bash_write_target(root, &joined).is_err() { + let reason = format!( + "Bash {} target `{joined}` is outside the Gate 1 workspace boundary relative to a working directory candidate", + target.operation + ); + return Some(BashWriteConfinementRejection { + path: joined, + operation: target.operation.clone(), + reason, + }); + } + } None } @@ -97,31 +168,51 @@ pub(crate) fn protected_path_mutation( root: &Path, protected_paths: &[String], ) -> Option { + let inspection = working_directory::inspect(command); write_targets(command) .into_iter() .filter(|target| target.operation != "working directory") .find_map(|target| { - let candidate = Path::new(&target.path); - let relative = if candidate.is_absolute() { - match candidate.strip_prefix(root) { - Ok(relative) => relative, - Err(_) => { - return protected_paths - .iter() - .find(|protected| candidate.ends_with(protected)) - .cloned(); - } - } - } else { - candidate - }; - protected_paths - .iter() - .find(|protected| path_matches(relative, Path::new(protected))) - .cloned() + let path = &target.path; + let mut candidates = vec![path.clone()]; + if !path.starts_with('/') + && !path.starts_with('~') + && !path.starts_with('$') + && path != "-" + { + candidates.extend(inspection.relative_variants(path)); + } + candidates + .into_iter() + .find_map(|candidate| protected_path_match(&candidate, root, protected_paths)) }) } +fn protected_path_match( + candidate: &str, + root: &Path, + protected_paths: &[String], +) -> Option { + let candidate = Path::new(candidate); + let relative = if candidate.is_absolute() { + match candidate.strip_prefix(root) { + Ok(relative) => relative, + Err(_) => { + return protected_paths + .iter() + .find(|protected| candidate.ends_with(protected)) + .cloned(); + } + } + } else { + candidate + }; + protected_paths + .iter() + .find(|protected| path_matches(relative, Path::new(protected))) + .cloned() +} + fn path_matches(candidate: &Path, protected: &Path) -> bool { candidate == protected || candidate.starts_with(protected) || protected.starts_with(candidate) } @@ -251,6 +342,17 @@ fn collect_program_targets(program: &str, arguments: &[&str], targets: &mut Vec< operation: "working directory".to_string(), }); } + "pushd" => { + if let Some(target) = operands.first().copied() + && !target.starts_with('+') + && !target.starts_with('-') + { + targets.push(WriteTarget { + path: target.to_string(), + operation: "working directory".to_string(), + }); + } + } "sudoedit" => { targets.extend(operands.into_iter().map(|path| WriteTarget { path: path.to_string(), @@ -737,6 +839,38 @@ mod tests { assert!(protected_path_mutation("cd tests", &root, &protected).is_none()); } + #[test] + fn working_directory_protected_path_mutation_matches_cd_targets() { + let fixture = tempfile::tempdir().unwrap(); + let root = fixture.path().join("ws"); + std::fs::create_dir_all(root.join("tests")).unwrap(); + std::fs::create_dir_all(root.join("sub/tests")).unwrap(); + let protected = vec!["tests/spec.rs".to_string()]; + + assert_eq!( + protected_path_mutation("cd tests && cp a.txt spec.rs", &root, &protected), + Some("tests/spec.rs".to_string()), + "a cd-relative protected target must be detected" + ); + assert!(protected_path_mutation("cd tests && touch other.rs", &root, &protected).is_none()); + assert!( + protected_path_mutation("cd sub/tests && touch spec.rs", &root, &protected).is_none() + ); + assert_eq!( + protected_path_mutation("cp a.txt tests/spec.rs", &root, &protected), + Some("tests/spec.rs".to_string()) + ); + } + + #[test] + fn working_directory_keeps_write_detection_unchanged() { + assert!(!has_recognized_mutation("cd frontend && npm test")); + assert!(!has_recognized_mutation("cd crates/x && cargo test")); + assert!(has_recognized_mutation("cd sub && tee link/f")); + assert!(has_recognized_mutation("pushd sub && tee f")); + assert!(!has_recognized_mutation("pushd sub")); + } + #[test] fn confinement_rejection_skips_dev_null_and_rejects_dash_working_directory() { let fixture = tempfile::tempdir().unwrap(); diff --git a/src/tools/bash_write_guard/command_prefix.rs b/src/tools/bash_write_guard/command_prefix.rs index c4036326..bd06e212 100644 --- a/src/tools/bash_write_guard/command_prefix.rs +++ b/src/tools/bash_write_guard/command_prefix.rs @@ -765,6 +765,8 @@ fn is_write_program(name: &str) -> bool { | "chmod" | "chown" | "cd" + | "pushd" + | "popd" | "sudoedit" ) } diff --git a/src/tools/bash_write_guard/working_directory.rs b/src/tools/bash_write_guard/working_directory.rs new file mode 100644 index 00000000..abc26e6f --- /dev/null +++ b/src/tools/bash_write_guard/working_directory.rs @@ -0,0 +1,300 @@ +//! Working-directory candidates for the Bash write-target inspector (Issue #568). +//! +//! The inspector judges a relative path against the workspace root, but a +//! command may `cd`/`pushd` first and the shell then resolves that relative path +//! against the changed directory. When the destination is a symlink that leaves +//! the workspace, a root-relative judgement misses it and the write (or read) +//! escapes. +//! +//! [`inspect`] walks the lexed tokens and builds the set of working directories +//! the command could run in. It starts at the workspace root and *adds* each +//! determinable `cd`/`pushd` destination instead of replacing, so a subshell, a +//! pipeline, or a `||` branch that keeps the old directory cannot hide the root +//! candidate. A destination whose value cannot be read statically (`$`, +//! backquotes, `~`, a glob, `..`, `-`, or an empty `cd`) adds nothing. A command +//! that mixes a `cd` with `CDPATH`, a loop, or a function definition is flagged +//! `undecidable`, and the caller refuses its relative writes. +//! +//! The lexical analysis (`shell_tokens`, `ShellToken`) and the prefix +//! `Resolution` are not changed here: this module only reads their output. + +use std::path::Path; + +use super::command_prefix::{self, Resolution}; +use super::{ShellToken, command_words, positional_operands}; + +/// The working-directory candidates and derived checks for one command. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct Inspection { + /// Candidate working directories. `""` is the workspace root; every other + /// entry is a workspace-relative path, or an absolute path for an absolute + /// `cd`/`pushd` destination. + pub(crate) bases: Vec, + /// `cd`/`pushd` destinations joined onto the candidates that preceded them, + /// to be confined exactly like a write target. + pub(crate) targets: Vec, + /// A `cd`/`pushd`/`popd` destination cannot be determined (CDPATH, loop, or + /// function definition), so relative paths after it must be refused. + pub(crate) undecidable: bool, + /// Literal relative words that a following command could read. The second + /// stage of the inspector joins these onto every candidate. + pub(crate) relative_words: Vec, +} + +impl Inspection { + /// Whether a `cd`/`pushd` added a candidate beyond the workspace root. + pub(crate) fn has_working_directory(&self) -> bool { + self.bases.iter().any(|base| !base.is_empty()) + } + + /// The base-joined forms of a relative path, excluding the root form that + /// the caller already judged on its own. + pub(crate) fn relative_variants(&self, path: &str) -> Vec { + self.bases + .iter() + .filter(|base| !base.is_empty()) + .map(|base| join(base, path)) + .collect() + } + + /// The base-joined forms of a relative path, including the root form. + pub(crate) fn all_variants(&self, path: &str) -> Vec { + self.bases.iter().map(|base| join(base, path)).collect() + } +} + +/// Reads the working-directory candidates out of a command. +pub(crate) fn inspect(command: &str) -> Inspection { + let mut inspection = Inspection { + bases: vec![String::new()], + targets: Vec::new(), + undecidable: false, + relative_words: Vec::new(), + }; + let Some(tokens) = super::shell_tokens(command) else { + return inspection; + }; + + let mut has_cd = false; + let mut cdpath = false; + let mut loop_keyword = false; + let mut function_keyword = false; + for word in tokens.iter().filter_map(word_token) { + if word.contains("CDPATH") { + cdpath = true; + } + if matches!(word, "for" | "while" | "until" | "select") { + loop_keyword = true; + } + if word == "function" { + function_keyword = true; + } + if is_relative_word(word) { + inspection.relative_words.push(word.to_string()); + } + } + if command.contains("()") { + function_keyword = true; + } + + for segment in tokens.split(|token| *token == ShellToken::SegmentEnd) { + let words = command_words(segment); + if words.is_empty() { + continue; + } + let Resolution::Program { + program, arguments, .. + } = command_prefix::resolve(&words) + else { + continue; + }; + let arguments: Vec<&str> = arguments.iter().map(String::as_str).collect(); + match program.as_str() { + "cd" => { + has_cd = true; + if let Some(target) = positional_operands(&arguments).first().copied() + && let Some(joined) = extend(&mut inspection.bases, target) + { + inspection.targets.extend(joined); + } + } + "pushd" => { + has_cd = true; + let Some(target) = positional_operands(&arguments).first().copied() else { + continue; + }; + // `pushd` with no operand and `pushd ±N` only rotate the + // directory stack, which holds nothing beyond the candidates, + // so they add no candidate. + if target.starts_with('+') || target.starts_with('-') { + continue; + } + if let Some(joined) = extend(&mut inspection.bases, target) { + inspection.targets.extend(joined); + } + } + "popd" => { + has_cd = true; + } + _ => {} + } + } + + inspection.undecidable = cdpath || (has_cd && (loop_keyword || function_keyword)); + inspection +} + +fn word_token(token: &ShellToken) -> Option<&str> { + match token { + ShellToken::Word(word) => Some(word.as_str()), + _ => None, + } +} + +/// Whether a `cd`/`pushd` destination is a literal path the inspector can join +/// onto the current candidates. +fn is_determinable(target: &str) -> bool { + !target.is_empty() + && target != "-" + && !target.contains(['$', '`', '~', '*', '?', '[', ']', '{', '}']) + && !target.split('/').any(|component| component == "..") +} + +/// Adds the candidates a destination produces and returns them for the caller +/// to confine. A destination that cannot be determined adds nothing. +fn extend(bases: &mut Vec, target: &str) -> Option> { + if !is_determinable(target) { + return None; + } + let joined: Vec = if Path::new(target).is_absolute() { + vec![target.to_string()] + } else { + bases.iter().map(|base| join(base, target)).collect() + }; + for path in &joined { + if !bases.contains(path) { + bases.push(path.clone()); + } + } + Some(joined) +} + +fn join(base: &str, path: &str) -> String { + if base.is_empty() { + path.to_string() + } else { + format!("{base}/{path}") + } +} + +/// Whether a word can be a literal relative path operand: no directory +/// separator, no expansion, no option, no assignment, and no whitespace. +fn is_relative_word(word: &str) -> bool { + !word.is_empty() + && word != "." + && word != ".." + && !word.starts_with('-') + && !word.contains('/') + && !word.contains(['$', '`', '~', '*', '?', '[', ']', '{', '}', '=', '\\']) + && !word.chars().any(char::is_whitespace) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn bases(command: &str) -> Vec { + inspect(command).bases + } + + #[test] + fn working_directory_candidates_start_at_the_root() { + let inspection = inspect("tee out.txt"); + assert_eq!(inspection.bases, vec![String::new()]); + assert!(!inspection.has_working_directory()); + assert!(!inspection.undecidable); + assert!(inspection.targets.is_empty()); + } + + #[test] + fn working_directory_adds_determinable_destinations() { + assert_eq!(bases("cd sub"), vec!["", "sub"]); + assert_eq!(bases("cd -P sub"), vec!["", "sub"]); + assert_eq!(bases("cd -- sub"), vec!["", "sub"]); + assert_eq!(bases("cd ./sub"), vec!["", "./sub"]); + assert_eq!(bases("cd sub/"), vec!["", "sub/"]); + assert_eq!(bases("builtin cd sub"), vec!["", "sub"]); + assert_eq!(bases("env cd sub"), vec!["", "sub"]); + assert_eq!(bases("pushd sub"), vec!["", "sub"]); + assert_eq!(bases("pushd -n sub"), vec!["", "sub"]); + } + + #[test] + fn working_directory_appends_instead_of_replacing() { + assert_eq!(bases("cd a && cd b"), vec!["", "a", "b", "a/b"]); + assert_eq!(bases("cd sub; cd sub"), vec!["", "sub", "sub/sub"]); + } + + #[test] + fn working_directory_records_targets_to_confine() { + let inspection = inspect("cd sub && cd link"); + assert_eq!(inspection.targets, vec!["sub", "link", "sub/link"]); + } + + #[test] + fn working_directory_skips_undeterminable_destinations() { + for command in [ + "cd", "cd ~", "cd ~/x", "cd ..", "cd ../x", "cd $VAR", "cd -", + ] { + let inspection = inspect(command); + assert!( + inspection.bases.iter().all(|base| base.is_empty()), + "no candidate expected for `{command}`: {:?}", + inspection.bases + ); + } + // An absolute destination is kept whole, not joined onto the root. + assert_eq!(inspect("cd /tmp").bases, vec!["", "/tmp"]); + } + + #[test] + fn working_directory_pushd_rotations_do_not_add_candidates() { + assert_eq!(bases("pushd +1"), vec![""]); + assert_eq!(bases("pushd"), vec![""]); + assert_eq!(bases("popd"), vec![""]); + } + + #[test] + fn working_directory_marks_cdpath_loops_and_functions_undecidable() { + assert!(inspect("CDPATH=sub cd link && tee f").undecidable); + assert!(inspect("export CDPATH=sub; cd link; tee f").undecidable); + assert!(inspect("for i in 1; do cd sub; done; tee f").undecidable); + assert!(inspect("f() { cd sub; }; f; tee f").undecidable); + assert!(inspect("function f { cd sub; }").undecidable); + assert!(!inspect("cd sub && tee link/f").undecidable); + assert!(!inspect("for i in 1; do tee f; done").undecidable); + assert!(!inspect("tee f").undecidable); + } + + #[test] + fn working_directory_collects_literal_relative_words() { + let inspection = inspect("cd sub && grep -r x link"); + assert!(inspection.relative_words.contains(&"link".to_string())); + assert!(!inspection.relative_words.contains(&"$VAR".to_string())); + assert!(!inspection.relative_words.contains(&"-r".to_string())); + assert!(!inspection.relative_words.contains(&"a/b".to_string())); + } + + #[test] + fn working_directory_variants_join_every_candidate() { + let inspection = inspect("cd sub"); + assert_eq!( + inspection.relative_variants("link/secret"), + vec!["sub/link/secret"] + ); + assert_eq!( + inspection.all_variants("link"), + vec!["link".to_string(), "sub/link".to_string()] + ); + } +} diff --git a/tests/issue568_bash_cwd_write_targets.rs b/tests/issue568_bash_cwd_write_targets.rs new file mode 100644 index 00000000..278562bd --- /dev/null +++ b/tests/issue568_bash_cwd_write_targets.rs @@ -0,0 +1,209 @@ +#![cfg(unix)] + +//! Issue #568: the Bash write-target guard judged a relative path against the +//! workspace root only, so a `cd`/`pushd` to a directory whose child is an +//! escaping symlink let the write (or read) leave the workspace, and +//! `pushd`/`popd` were not recognized at all. The guard now tracks the union of +//! the working directories the command could run in and refuses a relative +//! write (or read) that escapes from any of them. +//! +//! Every test name contains `working_directory` so the mutation-test filter +//! selects these tests together with the unit tests in +//! `bash_write_guard/working_directory.rs`. + +use std::path::PathBuf; + +use commandagent::mode::ExecutionMode; +use commandagent::tools::bash::path_confinement_rejection; +use commandagent::tools::registry::{ToolContext, ToolRegistry, tool_error_kind}; +use commandagent::tools::workspace_policy::WorkspacePolicy; +use serde_json::json; + +struct Fixture { + _dir: tempfile::TempDir, + root: PathBuf, +} + +/// The issue fixture: `sub/link`, `linked-outside`, and `a/b/link` are escaping +/// symlinks, the root has no `link`, the root `elink` escapes while `sub/elink` +/// is an inside directory. +fn fixture() -> Fixture { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().join("ws"); + let outside = dir.path().join("outside"); + for directory in [ + "sub/elink", + "sub/tests", + "tests", + "frontend", + "crates/x", + "src", + "a/b", + ] { + std::fs::create_dir_all(root.join(directory)).unwrap(); + } + std::fs::create_dir_all(&outside).unwrap(); + std::fs::write(outside.join("secret"), "outside-secret").unwrap(); + std::fs::write(root.join("a.txt"), "x").unwrap(); + std::fs::write(root.join("Cargo.toml"), "[package]\nname=\"x\"\n").unwrap(); + std::os::unix::fs::symlink(&outside, root.join("sub/link")).unwrap(); + std::os::unix::fs::symlink(&outside, root.join("linked-outside")).unwrap(); + std::os::unix::fs::symlink(&outside, root.join("a/b/link")).unwrap(); + std::os::unix::fs::symlink(&outside, root.join("elink")).unwrap(); + let root = root.canonicalize().unwrap(); + Fixture { _dir: dir, root } +} + +fn context(root: &std::path::Path, events: &std::path::Path) -> ToolContext { + ToolContext { + root: root.to_path_buf(), + mode: ExecutionMode::Act, + auto_approve: true, + interactive_approval: false, + offline: false, + workspace_policy: WorkspacePolicy::NormalTask, + eval_events_path: Some(events.to_path_buf()), + expected_paths: Vec::new(), + protected_paths: Vec::new(), + } +} + +#[test] +fn working_directory_rejects_writes_after_cd_and_pushd() { + let fixture = fixture(); + let root = &fixture.root; + let root_absolute = root.display(); + let cases = [ + // The issue table. + "cd sub && tee link/f".to_string(), + "cd sub && printf x > link/f".to_string(), + "pushd linked-outside && printf x > f".to_string(), + // Separators must not hide the cwd change. + "cd sub; tee link/f".to_string(), + "cd sub\ntee link/f".to_string(), + "cd sub || tee link/f".to_string(), + "cd sub | tee link/f".to_string(), + "(cd sub); tee link/f".to_string(), + "{ cd sub; tee link/f; }".to_string(), + "if cd sub; then tee link/f; fi".to_string(), + // cd spellings and repetition. + "cd -P sub && touch link/f".to_string(), + "cd -- sub && touch link/f".to_string(), + "cd \"sub\" && touch link/f".to_string(), + format!("cd {root_absolute}/sub && tee link/f"), + "cd a; cd b; tee link/f".to_string(), + "cd sub && cd link && touch f".to_string(), + "cd sub && cd link".to_string(), + // pushd forms. + "pushd sub && tee link/f".to_string(), + "pushd -n sub && tee link/f".to_string(), + // CDPATH. + "CDPATH=sub cd link && tee f".to_string(), + "export CDPATH=sub; cd link; tee f".to_string(), + // Loops and functions. + "for i in 1; do cd sub; done; tee link/f".to_string(), + "f() { cd sub; }; f; tee link/f".to_string(), + // Prefixed cd. + "builtin cd sub && tee link/f".to_string(), + "env cd sub && tee link/f".to_string(), + "env cd sub && tee elink/f".to_string(), + // ANSI-C quoting is refused first. + "cd sub && $'tee' link/f".to_string(), + ]; + for command in cases { + assert!( + path_confinement_rejection(&command, root).is_some(), + "expected rejection: {command}" + ); + } +} + +#[test] +fn working_directory_rejects_reads_after_cd() { + let fixture = fixture(); + let root = &fixture.root; + for command in [ + "cd sub && cat link/secret", + "cd sub && grep -r x link", + "cd sub && ls link", + ] { + assert!( + path_confinement_rejection(command, root).is_some(), + "expected rejection: {command}" + ); + } +} + +#[test] +fn working_directory_names_the_resolved_operation() { + let fixture = fixture(); + let root = &fixture.root; + for (command, operation) in [ + ("cd sub && tee link/f", "tee"), + ("cd sub && printf x > link/f", "output redirection"), + ("cd a; cd b; tee link/f", "tee"), + ("pushd linked-outside", "working directory"), + ] { + let rejection = path_confinement_rejection(command, root) + .unwrap_or_else(|| panic!("expected rejection: {command}")); + assert_eq!(rejection.operation, operation, "command: {command}"); + } +} + +#[test] +fn working_directory_keeps_inside_writes_and_verification_allowed() { + let fixture = fixture(); + let root = &fixture.root; + let root_absolute = root.display(); + let cases = [ + "cd src && touch a".to_string(), + "cd src && echo x > a.txt".to_string(), + "cd frontend && npm test > out.log".to_string(), + "cd crates/x && cargo test 2>&1 | tee test.log".to_string(), + format!("cd {root_absolute} && mkdir -p out"), + "cd sub && tee /dev/null".to_string(), + // Verify auto-approval is unaffected. + "cd frontend && npm test".to_string(), + "cd crates/x && cargo test".to_string(), + // No write, so the working-directory target alone stays allowed. + "pushd sub".to_string(), + "popd".to_string(), + "dirs".to_string(), + "cd link".to_string(), + ]; + for command in cases { + assert!( + path_confinement_rejection(&command, root).is_none(), + "expected allow: {command}" + ); + } +} + +#[test] +fn working_directory_blocks_escapes_before_execution() { + let fixture = fixture(); + let events = fixture._dir.path().join("events.jsonl"); + let registry = ToolRegistry::default(); + for command in [ + "cd sub && tee link/f", + "pushd linked-outside && printf x > f", + "cd sub && cat link/secret", + ] { + let error = registry + .execute( + "Bash", + &json!({ "command": command }), + &context(&fixture.root, &events), + ) + .unwrap_err(); + assert_eq!( + tool_error_kind(&error), + "bash_path_confinement_error", + "{command}" + ); + } + assert!( + !fixture.root.join("sub/link").join("f").exists(), + "the escaping write must not run" + ); +} From 175b61da5512cc3567d45f3e644ef021b3462634 Mon Sep 17 00:00:00 2001 From: kewton Date: Sat, 3 Oct 2026 14:38:45 +0900 Subject: [PATCH 2/2] fix(tools): close DIRSTACK and CDPATH-cwd read gaps in the bash guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit B1: `DIRSTACK[1]=sub/link` rewrites bash's directory stack, so a later `popd`, `pushd ±N`, or bare `pushd` can leave the workspace. A word that contains `DIRSTACK` now marks the working directory undecidable, like `CDPATH`. B2: after an undecidable `cd`/`pushd` (CDPATH, DIRSTACK, loop, or function), a command such as `CDPATH=sub cd link && cat secret` had its relative read words joined onto the candidates without the undecidable check. Stage two now refuses when the change is undecidable and at least one relative read word remains. Read words are taken only from segments that are not a working-directory change, so a pure `cd d0; cd d1; …` chain stays allowed while an absolute/home write under an undecidable cd keeps its old reason. Candidate cap: consecutive distinct `cd` destinations can double the set at each step. Growth now stops past 64 candidates and the destination becomes undecidable, and duplicate checks go through a `HashSet` so the check stays cheap at the cap. Tests: new reject integration cases for the DIRSTACK forms and for `CDPATH=sub cd link && {cat,grep} …`, a timed reject for `cd d0; …; cd d39; tee f` (returns under a second) with its no-write allow, plus unit tests for the absolute/home undecidable skip, the protected-path absolute/home skip, `pushd` write targets, and a popd loop. Co-authored-by: CommandCodeBot --- src/tools/bash.rs | 7 ++ src/tools/bash_write_guard.rs | 76 +++++++++++++++ .../bash_write_guard/working_directory.rs | 93 ++++++++++++++++--- tests/issue568_bash_cwd_write_targets.rs | 48 ++++++++++ 4 files changed, 213 insertions(+), 11 deletions(-) diff --git a/src/tools/bash.rs b/src/tools/bash.rs index 43bbcc08..1240bb8c 100644 --- a/src/tools/bash.rs +++ b/src/tools/bash.rs @@ -805,6 +805,13 @@ pub fn path_confinement_rejection( } } if inspection.has_working_directory() { + if inspection.undecidable && !inspection.relative_words.is_empty() { + return Some(path_reference_rejection( + &inspection.relative_words[0], + &root, + Some("follows a working directory change that cannot be determined"), + )); + } for word in &inspection.relative_words { for joined in inspection.all_variants(word) { if super::path_guard::ensure_bash_write_target(&root, &joined).is_err() { diff --git a/src/tools/bash_write_guard.rs b/src/tools/bash_write_guard.rs index 0f4f3727..31b1bf14 100644 --- a/src/tools/bash_write_guard.rs +++ b/src/tools/bash_write_guard.rs @@ -871,6 +871,82 @@ mod tests { assert!(!has_recognized_mutation("pushd sub")); } + #[test] + fn working_directory_undecidable_skips_absolute_and_home_targets() { + let fixture = tempfile::tempdir().unwrap(); + let root = fixture.path().join("ws"); + std::fs::create_dir_all(root.join("sub")).unwrap(); + std::fs::write(root.join("a.txt"), "x").unwrap(); + + assert!( + confinement_rejection( + &format!("for i in 1; do cd sub; done; tee {}/a.txt", root.display()), + &root, + ) + .is_none(), + "an absolute workspace target must stay allowed under an undecidable cd" + ); + + for command in [ + "for i in 1; do cd sub; done; tee ~/f", + "for i in 1; do cd sub; done; tee $HOME/f", + ] { + let rejection = confinement_rejection(command, &root) + .unwrap_or_else(|| panic!("expected rejection: {command}")); + assert!( + !rejection.reason.contains("cannot be determined"), + "`{command}` must keep its existing reason: {}", + rejection.reason + ); + } + } + + #[test] + fn working_directory_protected_path_mutation_skips_absolute_and_home_targets() { + let fixture = tempfile::tempdir().unwrap(); + let root = fixture.path().join("ws"); + std::fs::create_dir_all(root.join("tests")).unwrap(); + let protected = vec!["tests".to_string()]; + + for command in [ + "cd tests && cp a.txt /tmp/x", + "cd tests && cp a.txt $HOME/x", + ] { + assert!( + protected_path_mutation(command, &root, &protected).is_none(), + "`{command}` must not match the `tests` protected path" + ); + } + } + + #[test] + fn working_directory_write_targets_record_pushd_operands() { + let cases: &[(&str, &[(&str, &str)])] = &[ + ("pushd sub", &[("sub", "working directory")]), + ("pushd +1", &[]), + ("pushd -1", &[]), + ("pushd", &[]), + ]; + for (command, expected) in cases { + assert_eq!( + write_target_pairs(command), + expected_targets(expected), + "command: {command}" + ); + } + } + + #[test] + fn working_directory_rejects_write_after_popd_loop() { + let fixture = tempfile::tempdir().unwrap(); + let root = fixture.path().join("ws"); + std::fs::create_dir_all(&root).unwrap(); + assert!( + confinement_rejection("for i in 1; do popd; done; tee f", &root).is_some(), + "a relative write after an undecidable popd loop must be rejected" + ); + } + #[test] fn confinement_rejection_skips_dev_null_and_rejects_dash_working_directory() { let fixture = tempfile::tempdir().unwrap(); diff --git a/src/tools/bash_write_guard/working_directory.rs b/src/tools/bash_write_guard/working_directory.rs index abc26e6f..65e00b34 100644 --- a/src/tools/bash_write_guard/working_directory.rs +++ b/src/tools/bash_write_guard/working_directory.rs @@ -18,11 +18,17 @@ //! The lexical analysis (`shell_tokens`, `ShellToken`) and the prefix //! `Resolution` are not changed here: this module only reads their output. +use std::collections::HashSet; use std::path::Path; use super::command_prefix::{self, Resolution}; use super::{ShellToken, command_words, positional_operands}; +/// Upper bound on the working-directory candidates. Consecutive distinct `cd` +/// destinations can double the set at each step, so past this bound the +/// destination is treated as undeterminable instead of expanded further. +const MAX_CANDIDATES: usize = 64; + /// The working-directory candidates and derived checks for one command. #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct Inspection { @@ -33,11 +39,14 @@ pub(crate) struct Inspection { /// `cd`/`pushd` destinations joined onto the candidates that preceded them, /// to be confined exactly like a write target. pub(crate) targets: Vec, - /// A `cd`/`pushd`/`popd` destination cannot be determined (CDPATH, loop, or - /// function definition), so relative paths after it must be refused. + /// A `cd`/`pushd`/`popd` destination cannot be determined: `CDPATH` or + /// `DIRSTACK` can redirect it, a loop or function definition can repeat or + /// defer it, or its candidate set passed [`MAX_CANDIDATES`]. Relative paths + /// after it must be refused. pub(crate) undecidable: bool, - /// Literal relative words that a following command could read. The second - /// stage of the inspector joins these onto every candidate. + /// Literal relative read words (from segments that are not a working + /// directory change). The second stage of the inspector joins these onto + /// every candidate. pub(crate) relative_words: Vec, } @@ -77,34 +86,52 @@ pub(crate) fn inspect(command: &str) -> Inspection { let mut has_cd = false; let mut cdpath = false; + let mut dirstack = false; let mut loop_keyword = false; let mut function_keyword = false; for word in tokens.iter().filter_map(word_token) { if word.contains("CDPATH") { cdpath = true; } + if word.contains("DIRSTACK") { + dirstack = true; + } if matches!(word, "for" | "while" | "until" | "select") { loop_keyword = true; } if word == "function" { function_keyword = true; } - if is_relative_word(word) { - inspection.relative_words.push(word.to_string()); - } } if command.contains("()") { function_keyword = true; } + let mut overflow = false; for segment in tokens.split(|token| *token == ShellToken::SegmentEnd) { let words = command_words(segment); if words.is_empty() { continue; } + let resolution = command_prefix::resolve(&words); + // A working-directory change is consumed by the candidate walk below; + // its program and operands are not reads, so they do not feed the + // second-stage read check. + let cwd_program = matches!( + &resolution, + Resolution::Program { program, .. } + if matches!(program.as_str(), "cd" | "pushd" | "popd") + ); + if !cwd_program { + for word in &words { + if is_relative_word(word) { + inspection.relative_words.push((*word).to_string()); + } + } + } let Resolution::Program { program, arguments, .. - } = command_prefix::resolve(&words) + } = resolution else { continue; }; @@ -113,9 +140,11 @@ pub(crate) fn inspect(command: &str) -> Inspection { "cd" => { has_cd = true; if let Some(target) = positional_operands(&arguments).first().copied() + && !overflow && let Some(joined) = extend(&mut inspection.bases, target) { inspection.targets.extend(joined); + overflow = inspection.bases.len() > MAX_CANDIDATES; } } "pushd" => { @@ -129,8 +158,9 @@ pub(crate) fn inspect(command: &str) -> Inspection { if target.starts_with('+') || target.starts_with('-') { continue; } - if let Some(joined) = extend(&mut inspection.bases, target) { + if !overflow && let Some(joined) = extend(&mut inspection.bases, target) { inspection.targets.extend(joined); + overflow = inspection.bases.len() > MAX_CANDIDATES; } } "popd" => { @@ -140,7 +170,8 @@ pub(crate) fn inspect(command: &str) -> Inspection { } } - inspection.undecidable = cdpath || (has_cd && (loop_keyword || function_keyword)); + inspection.undecidable = + cdpath || dirstack || overflow || (has_cd && (loop_keyword || function_keyword)); inspection } @@ -171,8 +202,9 @@ fn extend(bases: &mut Vec, target: &str) -> Option> { } else { bases.iter().map(|base| join(base, target)).collect() }; + let mut seen: HashSet = bases.iter().cloned().collect(); for path in &joined { - if !bases.contains(path) { + if seen.insert(path.clone()) { bases.push(path.clone()); } } @@ -285,6 +317,45 @@ mod tests { assert!(!inspection.relative_words.contains(&"a/b".to_string())); } + #[test] + fn working_directory_marks_dirstack_undecidable() { + assert!(inspect("pushd . ; DIRSTACK[1]=sub/link; popd; tee f").undecidable); + assert!(inspect("DIRSTACK[1]=linked-outside; pushd +1").undecidable); + assert!(!inspect("tee f").undecidable); + } + + #[test] + fn working_directory_marks_popd_loop_undecidable() { + assert!(inspect("for i in 1; do popd; done; tee f").undecidable); + } + + #[test] + fn working_directory_caps_candidate_growth() { + let command = (0..40) + .map(|index| format!("cd d{index}")) + .collect::>() + .join("; "); + let inspection = inspect(&command); + assert!( + inspection.undecidable, + "past the cap the destination is not determinable" + ); + assert!( + inspection.bases.len() <= MAX_CANDIDATES * 2, + "candidate growth must stop at the cap: {}", + inspection.bases.len() + ); + } + + #[test] + fn working_directory_keeps_cwd_words_out_of_reads() { + let inspection = inspect("cd sub && cat secret"); + assert!(!inspection.relative_words.contains(&"cd".to_string())); + assert!(!inspection.relative_words.contains(&"sub".to_string())); + assert!(inspection.relative_words.contains(&"secret".to_string())); + assert!(inspect("cd d0; cd d1").relative_words.is_empty()); + } + #[test] fn working_directory_variants_join_every_candidate() { let inspection = inspect("cd sub"); diff --git a/tests/issue568_bash_cwd_write_targets.rs b/tests/issue568_bash_cwd_write_targets.rs index 278562bd..a5798859 100644 --- a/tests/issue568_bash_cwd_write_targets.rs +++ b/tests/issue568_bash_cwd_write_targets.rs @@ -134,6 +134,53 @@ fn working_directory_rejects_reads_after_cd() { } } +#[test] +fn working_directory_rejects_dirstack_and_cdpath_reads() { + let fixture = fixture(); + let root = &fixture.root; + let cases = [ + "pushd . ; DIRSTACK[1]=sub/link; popd; tee f", + "pushd . ; DIRSTACK[1]=sub/link; popd; printf x > f", + "pushd . ; DIRSTACK[1]=sub/link; pushd +1; tee f", + "pushd . ; DIRSTACK[1]=linked-outside; popd; tee f", + "pushd . ; DIRSTACK[1]=sub/link; popd; cat secret", + "CDPATH=sub cd link && cat secret", + "CDPATH=sub cd link && grep x secret", + ]; + for command in cases { + assert!( + path_confinement_rejection(command, root).is_some(), + "expected rejection: {command}" + ); + } +} + +#[test] +fn working_directory_caps_candidate_growth_within_a_second() { + let fixture = fixture(); + let root = &fixture.root; + let cds = (0..40) + .map(|index| format!("cd d{index}")) + .collect::>() + .join("; "); + + assert!( + path_confinement_rejection(&cds, root).is_none(), + "a long cd chain without a write must stay allowed" + ); + + let start = std::time::Instant::now(); + let rejection = path_confinement_rejection(&format!("{cds}; tee f"), root); + assert!( + start.elapsed().as_secs() < 1, + "the capped candidate walk must return within a second" + ); + assert!( + rejection.is_some(), + "a relative write after the cap must be rejected" + ); +} + #[test] fn working_directory_names_the_resolved_operation() { let fixture = fixture(); @@ -167,6 +214,7 @@ fn working_directory_keeps_inside_writes_and_verification_allowed() { "cd crates/x && cargo test".to_string(), // No write, so the working-directory target alone stays allowed. "pushd sub".to_string(), + "pushd sub; tee f".to_string(), "popd".to_string(), "dirs".to_string(), "cd link".to_string(),