diff --git a/src/tools/bash.rs b/src/tools/bash.rs index 30f8b914..1240bb8c 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,42 @@ 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() { + 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() { + return Some(path_reference_rejection(&joined, &root, None)); + } + } + } } None } @@ -816,6 +827,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 +1058,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..31b1bf14 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,114 @@ 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 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/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..65e00b34 --- /dev/null +++ b/src/tools/bash_write_guard/working_directory.rs @@ -0,0 +1,371 @@ +//! 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::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 { + /// 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` 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 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, +} + +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 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 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, .. + } = resolution + 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() + && !overflow + && let Some(joined) = extend(&mut inspection.bases, target) + { + inspection.targets.extend(joined); + overflow = inspection.bases.len() > MAX_CANDIDATES; + } + } + "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 !overflow && let Some(joined) = extend(&mut inspection.bases, target) { + inspection.targets.extend(joined); + overflow = inspection.bases.len() > MAX_CANDIDATES; + } + } + "popd" => { + has_cd = true; + } + _ => {} + } + } + + inspection.undecidable = + cdpath || dirstack || overflow || (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() + }; + let mut seen: HashSet = bases.iter().cloned().collect(); + for path in &joined { + if seen.insert(path.clone()) { + 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_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"); + 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..a5798859 --- /dev/null +++ b/tests/issue568_bash_cwd_write_targets.rs @@ -0,0 +1,257 @@ +#![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_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(); + 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(), + "pushd sub; tee f".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" + ); +}