From b79fc506e2f119f9188edb791b900df5170e155b Mon Sep 17 00:00:00 2001 From: BFlat Date: Tue, 3 Nov 2026 23:24:57 +0700 Subject: [PATCH] Auto mode deletes inside projects whose path has capitals, and reads quoted paths with spaces whole (#682) Auto mode's delete check compares paths as written and reads quoted or escaped paths with spaces as one argument, so it neither refuses deletes inside a project with capitals in its path nor lets a delete of the folder beside the project through. Replays #357. Co-authored-by: samurai --- crates/engine/src/tools/interaction_tools.rs | 109 ++++++++++++++++++- crates/engine/src/tools/native_formats.rs | 5 +- docs/ROADMAP.md | 4 +- docs/pitfalls.md | 10 ++ 4 files changed, 120 insertions(+), 8 deletions(-) diff --git a/crates/engine/src/tools/interaction_tools.rs b/crates/engine/src/tools/interaction_tools.rs index c60755ba..54ab57b5 100644 --- a/crates/engine/src/tools/interaction_tools.rs +++ b/crates/engine/src/tools/interaction_tools.rs @@ -527,14 +527,16 @@ pub fn never_unasked(ws: Option<&Workspace>, tool: &str, input: &Value) -> Optio (system || ours).then(|| refuse("changes system files or Craft Anything's own records")).flatten() } "Bash" | "PowerShell" => { - let c = s("command").to_ascii_lowercase(); + let command = s("command"); + let c = command.to_ascii_lowercase(); if PUBLISH.iter().any(|k| c.contains(k)) { return refuse("publishes or pushes anything"); } if SYSTEM_COMMANDS.iter().any(|k| c.contains(k)) { return refuse("changes the system, its settings or its users, or shuts it down"); } - if DELETES.iter().any(|k| c.contains(k)) && deletes_outside(&c, ws) { + // Paths keep their case: on a Mac or Linux `/Users/x` isn't `/users/x`. + if DELETES.iter().any(|k| c.contains(k)) && deletes_outside(&command, ws) { return refuse("deletes anything outside the project or the temp folder"); } None @@ -584,17 +586,60 @@ const DELETES: &[&str] = &["rm ", "rmdir", "rd /s", "del ", "erase ", "remove-it fn deletes_outside(command: &str, ws: Option<&Workspace>) -> bool { let temp = std::env::temp_dir(); let roots: Vec = ws.map(|w| w.root.clone()).into_iter().chain([temp, PathBuf::from("/tmp"), PathBuf::from("c:/tmp")]).collect(); - command.split(|c: char| c.is_whitespace() || c == '"' || c == '\'').any(|tok| { - if tok.contains("..") || tok.starts_with('~') || tok.contains("$home") || tok.contains("%userprofile%") || tok.contains("$env:") { + shell_words(command).iter().any(|tok| { + let tok = tok.as_str(); + let lower = tok.to_ascii_lowercase(); + if tok.contains("..") || tok.starts_with('~') || lower.contains("$home") || lower.contains("%userprofile%") || lower.contains("$env:") { return true; } // Git Bash writes C:\x as /c/x. - let tok = tok.strip_prefix("/c/").map(|r| format!("c:/{r}")).unwrap_or_else(|| tok.to_string()); + let tok = tok.strip_prefix("/c/").or_else(|| tok.strip_prefix("/C/")).map(|r| format!("c:/{r}")).unwrap_or_else(|| tok.to_string()); let absolute = tok.starts_with('/') || tok.get(1..3).is_some_and(|d| d == ":/" || d == ":\\"); absolute && !roots.iter().any(|r| inside(r, Path::new(&tok))) }) } +/// A command's arguments the way a shell reads them: quoted text (`"a b"`, `'a b'`) and an escaped +/// space (`a\ b`) stay in one argument, so a path with spaces is checked whole. A backslash before +/// anything else is kept as it is (Windows paths). +fn shell_words(command: &str) -> Vec { + let mut words = Vec::new(); + let mut word = String::new(); + let mut started = false; + let mut quote: Option = None; + let mut chars = command.chars().peekable(); + while let Some(c) = chars.next() { + match quote { + Some(q) if c == q => quote = None, + Some(_) => word.push(c), + None if c == '"' || c == '\'' => { + quote = Some(c); + started = true; + } + None if c == '\\' && chars.peek().is_some_and(|n| n.is_whitespace()) => { + if let Some(n) = chars.next() { + word.push(n); + } + started = true; + } + None if c.is_whitespace() => { + if started { + words.push(std::mem::take(&mut word)); + started = false; + } + } + None => { + word.push(c); + started = true; + } + } + } + if started { + words.push(word); + } + words +} + /// What the person said to an approval card. pub(crate) enum Verdict { Yes, @@ -1039,6 +1084,60 @@ mod tests { assert!(v["result"]["note"].as_str().unwrap().contains("decide questions yourself")); } + #[test] + fn auto_mode_deletes_inside_a_project_whose_path_has_capitals() { + // Regression: the command was lower-cased before its paths were compared, so on a Mac + // (`/Users/…`, `/var/folders/…/T/`) deleting the project's own build folder was refused. + let (_d, mut w) = ws(); + w.root = PathBuf::from(if cfg!(windows) { "D:/Projects/MyGame" } else { "/Users/Me/Projects/MyGame" }); + let root = w.root.display().to_string(); + let rm = |cmd: String| never_unasked(Some(&w), "Bash", &json!({"command": cmd})); + assert_eq!(rm(format!("rm -rf \"{root}/app/Build\"")), None); + assert_eq!(rm(format!("RM -RF \"{root}/Intermediate\"")), None, "the command's own case doesn't matter"); + assert!(rm(format!("rm -rf \"{root}/../Other\"")).is_some(), "climbing out is still refused"); + assert!(rm("rm -rf $HOME/stuff".into()).is_some()); + let elsewhere = if cfg!(windows) { "D:/Projects/Other" } else { "/Users/Me/Projects/Other" }; + assert!(rm(format!("rm -rf \"{elsewhere}\"")).is_some(), "a neighbouring folder is still outside"); + } + + #[test] + fn auto_mode_reads_a_quoted_path_with_spaces_as_one_path() { + // Regression: arguments were split at every space, quotes included, so the first half of + // `"…/MyGame Backup/report"` read as the project folder and the folder beside it was deleted. + let (_d, mut w) = ws(); + let projects = if cfg!(windows) { "D:/Unreal Projects" } else { "/Users/Me/Unreal Projects" }; + w.root = PathBuf::from(format!("{projects}/MyGame")); + let rm = |cmd: String| never_unasked(Some(&w), "Bash", &json!({"command": cmd})); + for beside in [ + format!("rm -rf \"{projects}/MyGame Backup/report\""), + format!("rm -rf '{projects}/MyGame Backup'"), + format!("rm -rf {}/MyGame\\ Backup", projects.replace(' ', "\\ ")), + ] { + assert!(rm(beside.clone()).is_some(), "{beside}"); + } + for own in [ + format!("rm -rf \"{projects}/MyGame/app/Build\""), + format!("rm -rf '{projects}/MyGame/Saved Games'"), + format!("rm -rf {}/MyGame/app/Build ./Intermediate", projects.replace(' ', "\\ ")), + ] { + assert_eq!(rm(own.clone()), None, "{own}"); + } + // The case that went through: a parent without spaces, so the first half matched the project. + let plain = if cfg!(windows) { "D:/Projects" } else { "/Users/Me/Projects" }; + w.root = PathBuf::from(format!("{plain}/MyGame")); + let beside = format!("rm -rf \"{plain}/MyGame Backup/report\""); + assert!(never_unasked(Some(&w), "Bash", &json!({"command": beside})).is_some(), "{beside}"); + } + + #[test] + fn shell_words_keep_quoted_and_escaped_spaces() { + assert_eq!(shell_words(r#"rm -rf "a b/c" 'd e' f\ g h"#), ["rm", "-rf", "a b/c", "d e", "f g", "h"]); + assert_eq!(shell_words(r"del C:\x\y.txt"), ["del", r"C:\x\y.txt"], "Windows backslashes stay"); + assert_eq!(shell_words(r#"rm "" x"#), ["rm", "", "x"]); + assert_eq!(shell_words("rm \"unclosed path"), ["rm", "unclosed path"]); + assert!(shell_words(" ").is_empty()); + } + #[test] fn a_project_edited_in_place_can_change_its_files_but_not_craft_anythings_records() { let d = tempfile::tempdir().unwrap(); diff --git a/crates/engine/src/tools/native_formats.rs b/crates/engine/src/tools/native_formats.rs index 85b3df36..b85204de 100644 --- a/crates/engine/src/tools/native_formats.rs +++ b/crates/engine/src/tools/native_formats.rs @@ -988,10 +988,13 @@ mod tests { #[test] fn real_binary_end_to_end_if_present() { + // `/bin/ls` is ELF on Linux and a universal Mach-O on a Mac. + let system = if cfg!(target_os = "macos") { "Mac" } else { "Linux" }; let Ok(bytes) = std::fs::read("/bin/ls") else { return }; let e = engine_with("ls", &bytes); let r = call(&e, "core_target_triage", json!({})); - assert!(r["result"]["plain_english"].as_str().unwrap().contains("Linux")); + let said = r["result"]["plain_english"].as_str().unwrap(); + assert!(said.contains(system), "{said}"); let r = call(&e, "core_overview_show", json!({})); assert!(r["result"]["counts"]["functions"].as_u64().unwrap() > 50); let r = call(&e, "native_layout_show", json!({"detail": "summary"})); diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index a28fa379..d302dae5 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -148,12 +148,12 @@ Owner: maintainers · Status: in progress Review comes before new work while the queue is long, smallest and clean PRs first. A PR is merged only when it's green and a maintainer has fully reviewed it, code as well as wording. - [ ] Maintainers: protect `main` (PRs required, squash only); the only ruleset covers `release` -- [ ] Auto mode's delete check is a safety bug that shows up as Mac test failures. It splits a +- [x] Auto mode's delete check is a safety bug that shows up as Mac test failures. It splits a command on spaces, so `rm -rf "C:/x/Game Backup/y"` from a project at `C:/x/Game` goes ahead unasked, and it lower-cases paths, so on a Mac or Linux a delete inside a project with capitals in its path is refused; `auto_mode_keeps_going_but_never_does_the_dangerous_things` fails on a Mac for that. `real_binary_end_to_end_if_present` fails there too, expecting - `/bin/ls` to be Linux — in PR #357 + `/bin/ls` to be Linux: both fixed (#357) - [ ] **decision** First-time contributors' CI: GitHub holds their runs for approval, so their PRs show no `gate` run (#490 and #413 among them, and #413 holds up #455). Approve each push, or change the repository's approval setting diff --git a/docs/pitfalls.md b/docs/pitfalls.md index 8ef50835..981d70ba 100644 --- a/docs/pitfalls.md +++ b/docs/pitfalls.md @@ -509,6 +509,16 @@ builder loop saved the reset only after a successful turn. A failed or cancelled removed the note, so reopening kept the old unchanged-turn count. Persist deferred request state on terminal errors too, outside provider retries; keep the original outcome if that save also fails. +**Lower-casing a command, then comparing its paths.** Auto mode lower-cased a shell command to +match keywords, then checked its paths against the project folder with that same text; Windows +compares paths without case, so only a Mac showed it: deleting `/Users/…/app/Build` in the +person's own project was refused. Match keywords on a lower-cased copy and compare paths as +written; test a path rule with capitals and a folder outside the temp directory. +The same check split the command at every space, quotes included, so `"…/MyGame Backup/x"` +began with the project's path and went through; it had been noted as "only refuses too much" +without trying a folder named like the project. Read arguments as the shell does, and test a +path rule with the folder beside the target, not only the target. + **A filter written for one kind of text, run on another.** The stand-in's secret scan was written for API answers, then also ran on recorded scripts: `this.token=t` became `[dropped]` and the scripts broke, and "Basic Membership" lost a word. It also tested whole tokens, so a secret inside a URL or