From 3d4caa917acfa6742244aa29d9472d434eebbd2a Mon Sep 17 00:00:00 2001 From: piero-83 Date: Sun, 11 Oct 2026 11:17:58 +0200 Subject: [PATCH 1/4] fix(app): reject malformed automation roots before startup --- apps/vectorcraft/src/automation_args.rs | 90 +++++++++++++++++++++++++ apps/vectorcraft/src/main.rs | 54 ++++++++------- 2 files changed, 121 insertions(+), 23 deletions(-) create mode 100644 apps/vectorcraft/src/automation_args.rs diff --git a/apps/vectorcraft/src/automation_args.rs b/apps/vectorcraft/src/automation_args.rs new file mode 100644 index 00000000..b2a5d9f0 --- /dev/null +++ b/apps/vectorcraft/src/automation_args.rs @@ -0,0 +1,90 @@ +//! Parse automation roots before starting any desktop services. A malformed flag must not +//! silently remove a root and leave the control channel unconfined. + +use std::path::PathBuf; + +pub struct AutomationArgs { + pub read_root: Option, + pub write_root: Option, + read_given: bool, + write_given: bool, +} + +impl AutomationArgs { + pub fn new(read_root: Option, write_root: Option) -> Self { + Self { read_root, write_root, read_given: false, write_given: false } + } + + /// Returns whether this argument is a root flag. CLI values override environment defaults + /// once; a second occurrence is an error, including when the two spellings are mixed. + pub fn consume(&mut self, arg: &str, args: &mut impl Iterator) -> Result { + let (flag, inline) = arg.split_once('=').map_or((arg, None), |(flag, value)| (flag, Some(value))); + let (root, given) = match flag { + "--automation-read-root" => (&mut self.read_root, &mut self.read_given), + "--automation-write-root" => (&mut self.write_root, &mut self.write_given), + _ => return Ok(false), + }; + if *given { + return Err(format!("{flag} given twice")); + } + let value = match inline { + Some(value) => value.to_owned(), + None => { + let value = args.next().ok_or_else(|| format!("{flag} needs a folder"))?; + if value.starts_with('-') { + return Err(format!("{flag} needs a folder; use {flag}= for a folder starting with '-'")); + } + value + } + }; + if value.is_empty() { + return Err(format!("{flag} needs a non-empty folder")); + } + *root = Some(value.into()); + *given = true; + Ok(true) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn roots_override_environment_once_and_leave_other_arguments() { + let mut roots = AutomationArgs::new(Some("environment".into()), None); + let mut args = ["a folder".to_owned(), "drawing.svg".to_owned()].into_iter(); + assert!(roots.consume("--automation-read-root", &mut args).unwrap()); + assert!(roots.consume("--automation-write-root=out", &mut args).unwrap()); + assert!(!roots.consume("--in-window-menus", &mut args).unwrap()); + assert_eq!(roots.read_root, Some("a folder".into())); + assert_eq!(roots.write_root, Some("out".into())); + assert_eq!(args.next().as_deref(), Some("drawing.svg")); + assert!(roots.consume("--automation-read-root=other", &mut args).is_err()); + assert_eq!(roots.read_root, Some("a folder".into())); + } + + #[test] + fn invalid_operands_preserve_environment_roots() { + for flag in ["--automation-read-root", "--automation-write-root"] { + for operand in [None, Some(""), Some("--version"), Some("--control"), Some("-folder")] { + let mut roots = AutomationArgs::new(Some("read".into()), Some("write".into())); + let mut args = operand.map(str::to_owned).into_iter(); + assert!(roots.consume(flag, &mut args).is_err(), "{flag} {operand:?}"); + assert_eq!(roots.read_root, Some("read".into())); + assert_eq!(roots.write_root, Some("write".into())); + } + let mut roots = AutomationArgs::new(None, None); + assert!(roots.consume(&format!("{flag}="), &mut std::iter::empty()).is_err()); + } + } + + #[test] + fn explicit_dash_folder_and_relative_dash_folder_are_valid() { + let mut roots = AutomationArgs::new(None, None); + assert!(roots.consume("--automation-read-root=-folder", &mut std::iter::empty()).unwrap()); + assert!(roots.consume("--automation-write-root", &mut ["./-folder".to_owned()].into_iter()).unwrap()); + assert_eq!(roots.read_root, Some("-folder".into())); + assert_eq!(roots.write_root, Some("./-folder".into())); + } +} diff --git a/apps/vectorcraft/src/main.rs b/apps/vectorcraft/src/main.rs index 33c39bee..f9a96b47 100644 --- a/apps/vectorcraft/src/main.rs +++ b/apps/vectorcraft/src/main.rs @@ -21,6 +21,7 @@ compile_error!("windows7 requires --target x86_64-win7-windows-msvc; the ordinar #[cfg(all(target_vendor = "win7", not(feature = "windows7")))] compile_error!("the win7 target requires --no-default-features --features windows7"); +mod automation_args; mod clipboard; mod control_server; #[cfg(feature = "wgpu")] @@ -585,7 +586,36 @@ fn native_options(custom_titlebar: bool) -> eframe::NativeOptions { } fn main() -> std::process::ExitCode { - // First, so every start-up warning is recorded (`logging`). + // Validate roots before logging, font scans, preferences or the window: a missing value + // must not clear an environment root and start the control channel without confinement. + let mut control_port: Option = std::env::var("VECTORCRAFT_CONTROL_PORT").ok().and_then(|p| p.parse().ok()); + let root_env = |name: &str| std::env::var_os(name).filter(|v| !v.is_empty()).map(std::path::PathBuf::from); + let mut roots_args = + automation_args::AutomationArgs::new(root_env("VECTORCRAFT_AUTOMATION_READ_ROOT"), root_env("VECTORCRAFT_AUTOMATION_WRITE_ROOT")); + let mut files = Vec::new(); + let mut in_window_menus = std::env::var_os("VECTORCRAFT_IN_WINDOW_MENUS").is_some_and(|v| !v.is_empty() && v != "0"); + let mut args = std::env::args().skip(1); + while let Some(a) = args.next() { + match roots_args.consume(&a, &mut args) { + Ok(true) => continue, + Ok(false) => {} + Err(e) => { + eprintln!("vectorcraft: {e}"); + return std::process::ExitCode::FAILURE; + } + } + match a.as_str() { + "--control" => control_port = args.next().and_then(|p| p.parse().ok()), + "--in-window-menus" => in_window_menus = true, + "--version" => { + println!("vectorcraft {}", env!("CARGO_PKG_VERSION")); + return std::process::ExitCode::SUCCESS; + } + _ => files.push(a), + } + } + let automation_args::AutomationArgs { read_root, write_root, .. } = roots_args; + // Before desktop services, so every start-up warning is recorded (`logging`). let logger = logging::install(); vectorcraft_ui_egui::i18n::detect_system_lang_in_background(); // Before the first font scan (the app's start): the fonts font services load (#579). @@ -595,28 +625,6 @@ fn main() -> std::process::ExitCode { mac_fonts::install(); // VectorCraft's own Fonts folder, which text.addFontFiles copies fonts into. prefs_dir::install_fonts(); - let mut control_port: Option = std::env::var("VECTORCRAFT_CONTROL_PORT").ok().and_then(|p| p.parse().ok()); - let root_env = |name: &str| std::env::var_os(name).filter(|v| !v.is_empty()).map(std::path::PathBuf::from); - let mut read_root = root_env("VECTORCRAFT_AUTOMATION_READ_ROOT"); - let mut write_root = root_env("VECTORCRAFT_AUTOMATION_WRITE_ROOT"); - let mut files = Vec::new(); - let mut in_window_menus = std::env::var_os("VECTORCRAFT_IN_WINDOW_MENUS").is_some_and(|v| !v.is_empty() && v != "0"); - let mut args = std::env::args().skip(1); - while let Some(a) = args.next() { - match a.as_str() { - "--control" => control_port = args.next().and_then(|p| p.parse().ok()), - "--automation-read-root" => read_root = args.next().map(std::path::PathBuf::from), - "--automation-write-root" => write_root = args.next().map(std::path::PathBuf::from), - _ if a.starts_with("--automation-read-root=") => read_root = a.split_once('=').map(|(_, d)| d.into()), - _ if a.starts_with("--automation-write-root=") => write_root = a.split_once('=').map(|(_, d)| d.into()), - "--in-window-menus" => in_window_menus = true, - "--version" => { - println!("vectorcraft {}", env!("CARGO_PKG_VERSION")); - return std::process::ExitCode::SUCCESS; - } - _ => files.push(a), - } - } // The log file lives in the settings directory, next to the preferences; opened after the // arguments, so `--version` leaves no file behind. Records logged until now are written to it // first. Runs without preferences (agents' test runs) log to standard error only, so they From d9c48bddeff024851d83d4ffe714975f69e4838d Mon Sep 17 00:00:00 2001 From: piero-83 Date: Sun, 11 Oct 2026 11:18:31 +0200 Subject: [PATCH 2/4] test(app): reject root errors before initialization --- apps/vectorcraft/tests/automation_args.rs | 70 +++++++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 apps/vectorcraft/tests/automation_args.rs diff --git a/apps/vectorcraft/tests/automation_args.rs b/apps/vectorcraft/tests/automation_args.rs new file mode 100644 index 00000000..2086b030 --- /dev/null +++ b/apps/vectorcraft/tests/automation_args.rs @@ -0,0 +1,70 @@ +// Integration tests may panic on failure (AGENTS.md › Robustness). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + +use std::process::{Command, Stdio}; +use std::time::{Duration, Instant}; + +const BIN: &str = env!("CARGO_BIN_EXE_vectorcraft"); + +/// Invalid roots must exit before desktop initialization on every platform, including a CI +/// machine with no display. A regression must fail rather than hang with a window open. +fn rejects(args: &[&str], message: &str) { + let mut child = Command::new(BIN) + .args(args) + .env_remove("VECTORCRAFT_CONTROL_PORT") + .env_remove("VECTORCRAFT_AUTOMATION_READ_ROOT") + .env_remove("VECTORCRAFT_AUTOMATION_WRITE_ROOT") + .env("VECTORCRAFT_NO_PREFS", "1") + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap(); + let deadline = Instant::now() + Duration::from_secs(5); + let timed_out = loop { + if child.try_wait().unwrap().is_some() { + break false; + } + if Instant::now() >= deadline { + child.kill().unwrap(); + break true; + } + std::thread::sleep(Duration::from_millis(10)); + }; + let out = child.wait_with_output().unwrap(); + assert!(!timed_out, "{args:?}: desktop started or did not exit promptly"); + assert!(!out.status.success(), "{args:?}"); + assert!(out.stdout.is_empty(), "{args:?}: {:?}", out.stdout); + // Exact stderr excludes unrelated startup warnings or display errors that could otherwise + // make a malformed invocation fail for an unrelated reason. + assert_eq!(String::from_utf8(out.stderr).unwrap(), format!("vectorcraft: {message}\n"), "{args:?}"); +} + +#[test] +fn missing_empty_or_option_operands_fail_before_startup() { + for flag in ["--automation-read-root", "--automation-write-root"] { + rejects(&["--control", "7979", flag], &format!("{flag} needs a folder")); + rejects(&[flag], &format!("{flag} needs a folder")); + rejects(&["--control", "7979", &format!("{flag}=")], &format!("{flag} needs a non-empty folder")); + rejects(&[flag, ""], &format!("{flag} needs a non-empty folder")); + for next in ["--version", "--control", "--automation-write-root", "-folder"] { + rejects(&[flag, next], &format!("{flag} needs a folder; use {flag}= for a folder starting with '-'")); + } + } +} + +#[test] +fn duplicate_roots_fail_for_both_spellings() { + for flag in ["--automation-read-root", "--automation-write-root"] { + rejects(&[flag, ".", &format!("{flag}=.")], &format!("{flag} given twice")); + rejects(&[&format!("{flag}=."), flag, "."], &format!("{flag} given twice")); + } +} + +#[test] +fn explicit_dash_folder_does_not_consume_version() { + let out = Command::new(BIN).args(["--automation-read-root=-folder", "--automation-write-root", "./-folder", "--version"]).output().unwrap(); + assert!(out.status.success(), "{}", String::from_utf8_lossy(&out.stderr)); + assert_eq!(String::from_utf8(out.stdout).unwrap(), format!("vectorcraft {}\n", env!("CARGO_PKG_VERSION"))); + assert!(out.stderr.is_empty()); +} From dea33fe5b40c35b2aeb6d7b5d8aa381ac16c20aa Mon Sep 17 00:00:00 2001 From: piero-83 Date: Sun, 11 Oct 2026 11:18:43 +0200 Subject: [PATCH 3/4] test(cli): preserve confinement after rejected paths --- apps/vectorcraft-cli/tests/cli.rs | 47 +++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/apps/vectorcraft-cli/tests/cli.rs b/apps/vectorcraft-cli/tests/cli.rs index 5b17de4b..37bb28c2 100644 --- a/apps/vectorcraft-cli/tests/cli.rs +++ b/apps/vectorcraft-cli/tests/cli.rs @@ -94,6 +94,15 @@ fn mcp_automation_roots() { let svg = r#""#; std::fs::write(base.join("in/a.svg"), svg).unwrap(); std::fs::write(base.join("outside/b.svg"), svg).unwrap(); + std::fs::write(base.join("outside/victim.png"), b"keep this file").unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::symlink; + symlink(base.join("outside/b.svg"), base.join("in/escape.svg")).unwrap(); + symlink(base.join("outside"), base.join("out/escape")).unwrap(); + symlink(base.join("outside/victim.png"), base.join("out/victim.png")).unwrap(); + symlink(base.join("outside/missing.png"), base.join("out/dangling.png")).unwrap(); + } let dir = |d: &str| base.join(d).to_string_lossy().into_owned(); let fails = |args: &[&str], why: &str| { let out = Command::new(BIN).args(args).stdin(Stdio::null()).output().unwrap(); @@ -114,15 +123,32 @@ fn mcp_automation_roots() { .unwrap(); let call = |id: u64, name: &str, args: Value| json!({"jsonrpc":"2.0","id":id,"method":"tools/call","params":{"name":name,"arguments":args}}); let file = |d: &str, f: &str| base.join(d).join(f).to_string_lossy().into_owned(); + let mut calls = vec![ + call(1, "open_file", json!({"path": file("in", "a.svg")})), + call(2, "open_file", json!({"path": file("outside", "b.svg")})), + call(3, "export", json!({"path": file("out", "a.png")})), + call(4, "export", json!({"path": file("in", "a.png")})), + call(5, "save_file", json!({"path": file("outside", "a.vectorcraft")})), + call(6, "open_file", json!({"path": file("in", "../outside/b.svg")})), + call(7, "export", json!({"path": file("out", "../outside/traversal.png")})), + ]; + let mut expected_errors = vec![false, true, false, true, true, true, true]; + #[cfg(unix)] + { + calls.extend([ + call(8, "open_file", json!({"path": file("in", "escape.svg")})), + call(9, "export", json!({"path": file("out", "escape/escaped.png")})), + call(10, "export", json!({"path": file("out", "victim.png")})), + call(11, "export", json!({"path": file("out", "dangling.png")})), + ]); + expected_errors.extend([true; 4]); + } + // An access error is per request: the server continues and can still save inside its root. + calls.push(call(12, "save_file", json!({"path": file("out", "allowed.vectorcraft")}))); + expected_errors.push(false); { let mut stdin = child.stdin.take().unwrap(); - for m in [ - call(1, "open_file", json!({"path": file("in", "a.svg")})), - call(2, "open_file", json!({"path": file("outside", "b.svg")})), - call(3, "export", json!({"path": file("out", "a.png")})), - call(4, "export", json!({"path": file("in", "a.png")})), - call(5, "save_file", json!({"path": file("outside", "a.vectorcraft")})), - ] { + for m in calls { writeln!(stdin, "{m}").unwrap(); } } @@ -130,9 +156,14 @@ fn mcp_automation_roots() { assert!(out.status.success()); let replies: Vec = String::from_utf8(out.stdout).unwrap().lines().map(|l| serde_json::from_str(l).unwrap()).collect(); let errors: Vec = replies.iter().map(|r| r["result"]["isError"] == true).collect(); - assert_eq!(errors, [false, true, false, true, true], "{replies:?}"); + assert_eq!(errors, expected_errors, "{replies:?}"); assert!(replies[1]["result"]["content"][0]["text"].as_str().unwrap().contains("outside the read root")); assert!(base.join("out/a.png").is_file() && !base.join("in/a.png").exists() && !base.join("outside/a.vectorcraft").exists()); + assert!(base.join("out/allowed.vectorcraft").is_file()); + for denied in ["traversal.png", "escaped.png", "missing.png"] { + assert!(!base.join("outside").join(denied).exists(), "{denied}"); + } + assert_eq!(std::fs::read(base.join("outside/victim.png")).unwrap(), b"keep this file"); } /// End-to-end protocol sweep over stdio against the compiled binary. The in-process From 87a9fb06cd0f647728f96b5d08f1ec53f282c803 Mon Sep 17 00:00:00 2001 From: piero-83 Date: Sun, 11 Oct 2026 11:18:49 +0200 Subject: [PATCH 4/4] docs: explain strict automation root arguments --- docs/control-protocol.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 3c564457..39b02ce4 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -36,6 +36,11 @@ nowhere and Windows device names are refused. A root left out grants none of its existing folders, or the app doesn't start; without `--control` they are ignored. See `docs/mcp.md` › Confining file access for the details. +Each root flag may be given once; a command-line root overrides its environment default. Missing or +empty values, another option used as a value, and duplicate flags stop the desktop before it starts +any services, even without `--control`. For a folder name starting with `-`, use `--flag=-folder` +or the relative path `./-folder`. + What is confined: the requests themselves, the frames that carry input they inject (`ui.click`, `ui.key`, `ui.text`…, so a menu item an agent clicks is confined too), the background saves and exports they start, and window captures. The person at the keyboard is not: files opened from the command line, Finder or a