From abf564e152ed9c6063f953e74dfa4ccb309334ec Mon Sep 17 00:00:00 2001 From: kewton Date: Fri, 9 Oct 2026 10:56:22 +0900 Subject: [PATCH 1/2] Reject relative paths after a glob or brace cd destination A `cd`/`pushd` destination holding a glob or brace metacharacter (`* ? [ ] { }`) added no working-directory candidate and no "cannot determine" mark, so a following relative path was judged against the workspace root alone and an outward symlink inside the destination let a read or write escape (Issue #635, found reviewing #613). The walk now flags such a destination `glob_destination` and, like the existing CDPATH/loop/function cases, `undecidable`, so the write side keeps its existing refusal. The second stage refuses the `/`-less relative words under the new mark too; the mark is narrower than `undecidable` and is set only for a glob/brace destination, so a command with no `cd` (`env | grep CDPATH`, `echo $CDPATH`) keeps its handling. A quoted or escaped bracket (`cd "s[2]"`) is flagged the same way, since the lexical read cannot tell it apart from an executed one. Co-authored-by: CommandCodeBot --- src/tools/bash.rs | 8 +- .../bash_write_guard/working_directory.rs | 41 +++- tests/issue635_bash_glob_cd_destination.rs | 213 ++++++++++++++++++ 3 files changed, 254 insertions(+), 8 deletions(-) create mode 100644 tests/issue635_bash_glob_cd_destination.rs diff --git a/src/tools/bash.rs b/src/tools/bash.rs index e7dc35cc..2fcaf573 100644 --- a/src/tools/bash.rs +++ b/src/tools/bash.rs @@ -826,7 +826,13 @@ pub fn path_confinement_rejection( { return Some(path_reference_rejection(&word, &root, None)); } - if inspection.has_working_directory() { + // Issue #635: a glob or brace `cd`/`pushd` destination adds no candidate, so + // `has_working_directory` stays false, but the relative words after it still + // follow an undetermined working directory and must be refused. The mark is + // narrower than `undecidable`: it is set only for a glob/brace destination, + // so a command with no `cd` (`env | grep CDPATH`, `echo $CDPATH`) keeps its + // existing handling. + if inspection.has_working_directory() || inspection.glob_destination { if inspection.undecidable && !inspection.relative_words.is_empty() { return Some(path_reference_rejection( &inspection.relative_words[0], diff --git a/src/tools/bash_write_guard/working_directory.rs b/src/tools/bash_write_guard/working_directory.rs index e1b73a04..af7d855b 100644 --- a/src/tools/bash_write_guard/working_directory.rs +++ b/src/tools/bash_write_guard/working_directory.rs @@ -13,7 +13,10 @@ //! 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. +//! `undecidable`, and the caller refuses its relative writes. A destination that +//! holds a glob or brace metacharacter adds nothing and is flagged +//! `glob_destination` too, so the caller refuses the relative paths after it +//! even when the workspace root is the only candidate (Issue #635). //! //! The lexical analysis (`shell_tokens`, `ShellToken`) and the prefix //! `Resolution` are not changed here: this module only reads their output. @@ -45,6 +48,12 @@ pub(crate) struct Inspection { /// defer it, or its candidate set passed [`MAX_CANDIDATES`]. Relative paths /// after it must be refused. pub(crate) undecidable: bool, + /// A `cd`/`pushd` destination holds a glob or brace metacharacter + /// (`* ? [ ] { }`), so the directory it names cannot be read from its + /// spelling and no candidate beyond the workspace root is added (Issue + /// #635). Relative paths after it must be refused even though the workspace + /// root is the only candidate. + pub(crate) glob_destination: 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. @@ -59,6 +68,7 @@ impl Inspection { bases: vec![String::new()], targets: Vec::new(), undecidable: false, + glob_destination: false, relative_words: Vec::new(), } } @@ -76,6 +86,7 @@ impl Inspection { self.targets.extend(other.targets); self.relative_words.extend(other.relative_words); self.undecidable |= other.undecidable; + self.glob_destination |= other.glob_destination; } /// Whether a `cd`/`pushd` added a candidate beyond the workspace root. @@ -137,6 +148,7 @@ fn walk(text: &str, ambiguous: bool, cr_mode: CrMode) -> Inspection { }; let mut has_cd = false; + let mut glob_destination = false; let mut cdpath = false; let mut dirstack = false; let mut loop_keyword = false; @@ -191,12 +203,14 @@ fn walk(text: &str, ambiguous: bool, cr_mode: CrMode) -> Inspection { 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; + if let Some(target) = positional_operands(&arguments).first().copied() { + if is_glob_destination(target) { + glob_destination = true; + } + if !overflow && let Some(joined) = extend(&mut inspection.bases, target) { + inspection.targets.extend(joined); + overflow = inspection.bases.len() > MAX_CANDIDATES; + } } } "pushd" => { @@ -210,6 +224,9 @@ fn walk(text: &str, ambiguous: bool, cr_mode: CrMode) -> Inspection { if target.starts_with('+') || target.starts_with('-') { continue; } + if is_glob_destination(target) { + glob_destination = true; + } if !overflow && let Some(joined) = extend(&mut inspection.bases, target) { inspection.targets.extend(joined); overflow = inspection.bases.len() > MAX_CANDIDATES; @@ -222,10 +239,12 @@ fn walk(text: &str, ambiguous: bool, cr_mode: CrMode) -> Inspection { } } + inspection.glob_destination = glob_destination; inspection.undecidable = ambiguous || cdpath || dirstack || overflow + || glob_destination || (has_cd && (loop_keyword || function_keyword)); inspection } @@ -246,6 +265,14 @@ fn is_determinable(target: &str) -> bool { && !target.split('/').any(|component| component == "..") } +/// Whether a `cd`/`pushd` destination holds a glob or brace metacharacter, so +/// the directory it names cannot be read from its spelling (Issue #635). The +/// de-quoted text marks a quoted or escaped bracket (`cd "s[2]"`) the same way, +/// because the lexical read does not tell it apart from an executed one. +fn is_glob_destination(target: &str) -> bool { + target.contains(['*', '?', '[', ']', '{', '}']) +} + /// 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> { diff --git a/tests/issue635_bash_glob_cd_destination.rs b/tests/issue635_bash_glob_cd_destination.rs new file mode 100644 index 00000000..14fa247c --- /dev/null +++ b/tests/issue635_bash_glob_cd_destination.rs @@ -0,0 +1,213 @@ +#![cfg(unix)] + +//! Issue #635: a `cd`/`pushd` destination that holds a glob or brace +//! metacharacter (`* ? [ ] { }`) added no working-directory candidate and did +//! not mark the walk "cannot be determined", so a following relative path was +//! judged against the workspace root alone and an outward symlink inside the +//! destination escaped. The walk now flags such a destination, and the second +//! stage refuses the `/`-less relative words under that flag too. A destination +//! whose brackets are quoted or escaped (`cd "s[2]"`) is flagged the same way, +//! because the lexical read cannot tell it apart from an executed glob. +//! +//! The write rows are pinned to the write-side operation and the existing +//! "working directory change that cannot be determined" reason, so they are +//! refused by `W` (the write side) and not by a protected-path match `P`. + +use std::path::PathBuf; + +use commandagent::tools::bash::path_confinement_rejection; + +/// The fixed operation a read-side refusal carries. +const READ_OPERATION: &str = "path reference"; +/// The existing write-side working-directory reason. The refusal must carry +/// this reason and not a protected-path match. +const CWD_REASON: &str = "follows a working directory change that cannot be determined"; + +struct Fixture { + _dir: tempfile::TempDir, + root: PathBuf, +} + +/// A tempdir outside the prefix `is_system_prefix_allowed` accepts (`/usr`, +/// `/bin`, `/opt`, `/etc`, `/tmp`), so an escaping symlink target is never +/// admitted as a system path. `s2` is an ordinary directory holding the outward +/// symlink `lf4` and the protected-looking `tests/spec.rs`; `s[2]` is a literal +/// directory name holding the outward symlink `lf5`; `sub/deep` holds the +/// outward symlink `lf3`. The workspace root itself holds no symlink, so a +/// root-level `ls *` stays inside. +fn fixture() -> Fixture { + let dir = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).expect("create tempdir"); + let root = dir.path().join("ws"); + for directory in ["s2/tests", "s[2]", "sub", "sub/deep", "src"] { + std::fs::create_dir_all(root.join(directory)).unwrap(); + } + std::fs::write(root.join("f"), "x").unwrap(); + std::fs::write(root.join("sub/f"), "x").unwrap(); + std::fs::write(root.join("src/f"), "x").unwrap(); + std::fs::write(root.join("s2/tests/spec.rs"), "x").unwrap(); + let outside = dir.path().join("outside"); + std::fs::create_dir_all(&outside).unwrap(); + std::fs::write(outside.join("secret"), "outside-secret").unwrap(); + std::os::unix::fs::symlink(&outside, root.join("s2/lf4")).unwrap(); + std::os::unix::fs::symlink(&outside, root.join("s[2]/lf5")).unwrap(); + std::os::unix::fs::symlink(&outside, root.join("sub/deep/lf3")).unwrap(); + let root = root.canonicalize().unwrap(); + Fixture { _dir: dir, root } +} + +fn assert_glob_destination_read_rejected(root: &std::path::Path, commands: &[String]) { + for command in commands { + let rejection = path_confinement_rejection(command, root) + .unwrap_or_else(|| panic!("expected rejection: {command}")); + assert_eq!( + rejection.operation, READ_OPERATION, + "command: {command} ({})", + rejection.reason + ); + assert!( + rejection.reason.contains(CWD_REASON), + "command: {command}: {}", + rejection.reason + ); + } +} + +/// The issue table, reads: every shape whose `cd`/`pushd` destination holds a +/// glob or brace metacharacter must be refused even though only the workspace +/// root is a working-directory candidate. +#[test] +fn glob_cd_destination_rejects_relative_reads() { + let fixture = fixture(); + let root = &fixture.root; + let root_absolute = root.display(); + let cases = [ + // Row 1: `*`, `?`, and an executed `[2]`. + "cd s2* && cat lf4".to_string(), + "cd s? && cat lf4".to_string(), + "cd s[2] && cat lf4".to_string(), + // Row 2: brace lists, with `&&`, `;`, and a trailing space. + "cd {s2,x} && cat lf4".to_string(), + "cd {s2,x}; cat lf4".to_string(), + "cd s{2,} ; cat lf4".to_string(), + // Row 3: `-P`, `-L`, and `--` before the destination. + "cd -P s2* && cat lf4".to_string(), + "cd -L s2* && cat lf4".to_string(), + "cd -- s2* && cat lf4".to_string(), + // Row 4: a second `cd` after the glob destination. + "cd s2* && cd . && cat lf4".to_string(), + "cd sub && cd de* && cat lf3".to_string(), + "cd su? && cd deep && cat lf3".to_string(), + // Row 5: pushd. + "pushd s2* && cat lf4".to_string(), + "pushd {s2,x} && cat lf4".to_string(), + // Row 6: subshell, brace group, and `if`. + "(cd s2* && cat lf4)".to_string(), + "{ cd s2*; cat lf4; }".to_string(), + "if cd s2*; then cat lf4; fi".to_string(), + // Row 7: control operators and a newline between the two commands. + "cd s2*\ncat lf4".to_string(), + "cd s2*; cat lf4".to_string(), + "cd s2* || cat lf4".to_string(), + // Row 8: quoted or escaped literal brackets. + "cd \"s[2]\" && cat lf5".to_string(), + "cd 's[2]' && cat lf5".to_string(), + r"cd s\[2\] && cat lf5".to_string(), + // A prefixed `cd` and an absolute destination keep their handling. + "env cd s2* && cat lf4".to_string(), + format!("cd {root_absolute}/s2* && cat lf4"), + ]; + assert_glob_destination_read_rejected(root, &cases); +} + +/// The issue table, writes: a relative write after a glob destination is refused +/// by the write side (`W=1`), pinned to the write operation and the existing +/// working-directory reason. +#[test] +fn glob_cd_destination_rejects_relative_writes() { + let fixture = fixture(); + let root = &fixture.root; + let cases = [ + ("cd s2* && echo x > lf4", "output redirection"), + ("cd s2* && tee lf4", "tee"), + ("cd s2* && cp f lf4", "cp"), + ("cd s2* && touch lf4", "touch"), + ("cd {s2,x} && printf x > lf4", "output redirection"), + ("cd \"s[2]\" && touch lf5", "touch"), + ("pushd s2* && tee lf4", "tee"), + ]; + for (command, operation) in cases { + let rejection = path_confinement_rejection(command, root) + .unwrap_or_else(|| panic!("expected write rejection: {command}")); + assert_eq!(rejection.operation, operation, "command: {command}"); + assert!( + rejection.reason.contains(CWD_REASON), + "command: {command}: {}", + rejection.reason + ); + } +} + +/// The issue table, protected file: a glob `cd` into a directory that holds a +/// protected-looking path, then `rm`/`tee`/`>` that path. The write is refused +/// by the write side with the existing working-directory reason (`W=1`), so it +/// is not a protected-path match and `P` stays 0. +#[test] +fn glob_cd_destination_rejects_protected_file_writes() { + let fixture = fixture(); + let root = &fixture.root; + let cases = [ + ("cd s2* && rm tests/spec.rs", "rm"), + ("cd s2* && tee tests/spec.rs", "tee"), + ("cd s2* && echo x > tests/spec.rs", "output redirection"), + ]; + for (command, operation) in cases { + let rejection = path_confinement_rejection(command, root) + .unwrap_or_else(|| panic!("expected write rejection: {command}")); + assert_eq!(rejection.operation, operation, "command: {command}"); + assert!( + rejection.reason.contains(CWD_REASON), + "command: {command}: {}", + rejection.reason + ); + } +} + +/// Destinations that do not hold a glob or brace metacharacter, and globs used +/// for reading rather than as the `cd` destination, keep their existing +/// handling. A `CDPATH` word without a `cd` must not become newly refused. +#[test] +fn non_glob_cd_and_glob_reads_are_unchanged() { + let fixture = fixture(); + let root = &fixture.root; + for command in [ + "cd sub && cat f", + "cd sub && cargo test", + "cd sub && echo x > out", + "cd sub && cd deep && cat f", + "pushd sub", + "cd src && ls *", + "cat s2*/x", + "env | grep CDPATH", + "echo $CDPATH", + ] { + assert!( + path_confinement_rejection(command, root).is_none(), + "expected allow: {command}" + ); + } +} + +/// A glob destination that is only the working directory (no relative read or +/// write after it) is refused as well, because the destination itself cannot be +/// read from its spelling: `cd s2*` alone. +#[test] +fn glob_cd_destination_alone_is_rejected() { + let fixture = fixture(); + let root = &fixture.root; + let cases = [ + "cd s2*".to_string(), + "pushd s2*".to_string(), + "cd {s2,x}".to_string(), + ]; + assert_glob_destination_read_rejected(root, &cases); +} From da7292b0efc16241136ba7428cd8970d86f1a411 Mon Sep 17 00:00:00 2001 From: kewton Date: Fri, 9 Oct 2026 11:39:00 +0900 Subject: [PATCH 2/2] Add glob-destination merge and candidate-cap detector tests Pin two behaviours that had no failing test (Issue #635 review, cargo mutants missed 3 / 1): - `Inspection::merge` must union the glob-destination mark: a unit test where only one carriage-return reading saw a glob or brace destination asserts the merged mark survives, so a `|=` read as `&=` fails. The integration shape `cd \rs2* && cat lf4` fixes R=1. - The candidate cap must count a growth that passes MAX_CANDIDATES without landing on it: forty repeated `cd sub` add one candidate each, then `cd r` doubles the set past the bound. A unit test asserts the walk is undecidable and an integration test asserts the relative write is refused, for the `cd` and `pushd` branch alike, so a `>` read as `==` fails. Co-authored-by: CommandCodeBot --- .../bash_write_guard/working_directory.rs | 44 ++++++++++++++++++ tests/issue635_bash_glob_cd_destination.rs | 46 +++++++++++++++++++ 2 files changed, 90 insertions(+) diff --git a/src/tools/bash_write_guard/working_directory.rs b/src/tools/bash_write_guard/working_directory.rs index af7d855b..e792115f 100644 --- a/src/tools/bash_write_guard/working_directory.rs +++ b/src/tools/bash_write_guard/working_directory.rs @@ -429,6 +429,26 @@ mod tests { ); } + /// Issue #635: the cap check must count a jump that passes [`MAX_CANDIDATES`] + /// without landing on it. Forty repeated `cd sub` add exactly one candidate + /// each (lengths 2..=41), then a single `cd r` doubles the set to 82, which + /// skips 64; the destination past the cap must still be undecidable. A `>` + /// read as `==` never sees 64 and leaves the mark clear. + #[test] + fn working_directory_caps_a_growth_that_skips_the_bound() { + let command = format!("{}cd r", "cd sub; ".repeat(40)); + let inspection = inspect(&command); + assert!( + inspection.bases.len() > MAX_CANDIDATES, + "the fixture must pass the bound: {}", + inspection.bases.len() + ); + assert!( + inspection.undecidable, + "a candidate set that passed the bound without landing on it must be undecidable" + ); + } + #[test] fn working_directory_keeps_cwd_words_out_of_reads() { let inspection = inspect("cd sub && cat secret"); @@ -527,6 +547,30 @@ mod tests { assert_eq!(merged.bases, vec![String::new(), "cr\rdir".to_string()]); } + /// Issue #635: the union of the two carriage-return readings must keep the + /// glob-destination mark, so the caller's `/`-less word check still runs + /// when only one reading saw a glob or brace destination. Reading the `|=` + /// as `&=` clears the mark whenever the other reading did not raise it. + #[test] + fn working_directory_merge_keeps_the_glob_destination_mark() { + let mut merged = Inspection::empty(); + let mut glob_reading = Inspection::empty(); + glob_reading.glob_destination = true; + merged.merge(glob_reading); + assert!( + merged.glob_destination, + "a glob destination in the other reading must survive the union" + ); + + let mut already_marked = Inspection::empty(); + already_marked.glob_destination = true; + already_marked.merge(Inspection::empty()); + assert!( + already_marked.glob_destination, + "a union with a reading that saw no glob destination must not clear the mark" + ); + } + /// Issue #613: only the WordChar reading sees the `\r` in a `cd` /// destination, so the merged candidate is what keeps it. A `/`-less operand /// that is an outward symlink *inside* the CR-named directory is reached by diff --git a/tests/issue635_bash_glob_cd_destination.rs b/tests/issue635_bash_glob_cd_destination.rs index 14fa247c..b20878fc 100644 --- a/tests/issue635_bash_glob_cd_destination.rs +++ b/tests/issue635_bash_glob_cd_destination.rs @@ -172,6 +172,52 @@ fn glob_cd_destination_rejects_protected_file_writes() { } } +/// A glob or brace destination that only one of the two carriage-return +/// readings sees (`cd \rs2*`): the historical reading splits on `\r` and finds +/// no operand, the shell reading keeps `\rs2*` as a glob destination. The +/// command must be refused. The `merge` unit test in `working_directory.rs` +/// pins that the mark itself survives the union; here the R=1 value is fixed. +#[test] +fn glob_cd_destination_mark_survives_the_carriage_return_union() { + let fixture = fixture(); + let root = &fixture.root; + for command in [ + // The historical reading splits on `\r` (`cd` has no operand); the shell + // reading keeps it in the word (`\rs[2]`). + "cd \r\"s[2]\" && cat lf5", + // The issue's shape: `\r` then a `*` destination. + "cd \rs2* && cat lf4", + ] { + assert!( + path_confinement_rejection(command, root).is_some(), + "expected rejection: {command}" + ); + } +} + +/// The candidate cap must count a jump that passes [`MAX_CANDIDATES`] without +/// landing on it: forty repeated `cd sub` add one candidate each, then `cd r` +/// doubles the set and skips 64. Past the cap the relative write must be refused +/// (`undecidable`), for the `cd` and the `pushd` branch alike; a `>` read as `==` +/// leaves the mark clear and allows the write. +#[test] +fn glob_cd_destination_cap_rejects_relative_writes() { + let fixture = fixture(); + let root = &fixture.root; + for command in [ + format!("{}cd r; tee f", "cd sub; ".repeat(40)), + format!("{}pushd r; tee f", "pushd sub; ".repeat(40)), + ] { + let rejection = path_confinement_rejection(&command, root) + .unwrap_or_else(|| panic!("expected a reject past the cap: {command}")); + assert!( + rejection.reason.contains(CWD_REASON), + "the cap must refuse via the working-directory reason: {}", + rejection.reason + ); + } +} + /// Destinations that do not hold a glob or brace metacharacter, and globs used /// for reading rather than as the `cd` destination, keep their existing /// handling. A `CDPATH` word without a `cd` must not become newly refused.