From 4744c2f04b2414b7dc625eed5d52986e46cf4491 Mon Sep 17 00:00:00 2001 From: RealtaNua Date: Mon, 12 Oct 2026 17:40:19 +0700 Subject: [PATCH 1/2] Fix #1373: --control without a valid port stops the desktop before it starts --control took the next argument as its port and dropped it when it wasn't a number, so the app started without a control server and `--control --automation-read-root DIR` opened DIR as a document. automation_args::ControlArg now reads --control or --control= once and requires a port from 1 to 65535; VECTORCRAFT_CONTROL_PORT must be one too, checked only when --control isn't given so --control and --version win over a bad value. Errors go through the same exit as the root flags (#1157). Tests: unit tests in automation_args.rs; tests/automation_args.rs runs the binary for each bad form and checks the exact message and a prompt failing exit. Co-Authored-By: Claude Opus 5.5 (1M context) --- apps/vectorcraft/src/automation_args.rs | 99 ++++++++++++++++++++++- apps/vectorcraft/src/main.rs | 19 ++++- apps/vectorcraft/tests/automation_args.rs | 26 ++++++ docs/control-protocol.md | 4 +- docs/development.md | 2 +- 5 files changed, 142 insertions(+), 8 deletions(-) diff --git a/apps/vectorcraft/src/automation_args.rs b/apps/vectorcraft/src/automation_args.rs index b2a5d9f0..26157b76 100644 --- a/apps/vectorcraft/src/automation_args.rs +++ b/apps/vectorcraft/src/automation_args.rs @@ -1,6 +1,8 @@ -//! Parse automation roots before starting any desktop services. A malformed flag must not -//! silently remove a root and leave the control channel unconfined. +//! Parse the control port and automation roots before starting any desktop services. A malformed +//! flag must not silently remove a root and leave the control channel unconfined, or leave the app +//! running without the control channel that was asked for. +use std::ffi::OsString; use std::path::PathBuf; pub struct AutomationArgs { @@ -46,6 +48,50 @@ impl AutomationArgs { } } +/// `--control ` or `--control=`, given at most once. +#[derive(Default)] +pub struct ControlArg { + pub port: Option, +} + +impl ControlArg { + /// Returns whether this argument is `--control`. The next argument is its port whatever it + /// is, so `--control --automation-read-root` reports the flag rather than reading it as a root. + pub fn consume(&mut self, arg: &str, args: &mut impl Iterator) -> Result { + let value = match arg.strip_prefix("--control") { + Some("") => args.next(), + Some(rest) => match rest.strip_prefix('=') { + Some(value) => Some(value.to_owned()), + None => return Ok(false), + }, + None => return Ok(false), + }; + if self.port.is_some() { + return Err("--control given twice".into()); + } + self.port = Some(port("--control", value.as_deref())?); + Ok(true) + } +} + +/// `VECTORCRAFT_CONTROL_PORT`, for when `--control` isn't given: unset or empty is no control +/// channel, anything else must be a port. +pub fn env_control_port(value: Option) -> Result, String> { + match value { + Some(v) if !v.is_empty() => port("VECTORCRAFT_CONTROL_PORT", Some(&v.to_string_lossy())).map(Some), + _ => Ok(None), + } +} + +/// A port from 1 to 65535 (the server would listen on 0's random port without saying which). +fn port(name: &str, value: Option<&str>) -> Result { + let need = format!("{name} needs a port number from 1 to 65535"); + match value { + None => Err(need), + Some(v) => v.parse().ok().filter(|&p| p != 0).ok_or(format!("{need}, not `{v}`")), + } +} + #[cfg(test)] mod tests { use super::*; @@ -87,4 +133,53 @@ mod tests { assert_eq!(roots.read_root, Some("-folder".into())); assert_eq!(roots.write_root, Some("./-folder".into())); } + + fn control(args: &[&str]) -> Result, String> { + let mut control = ControlArg::default(); + let mut args = args.iter().map(|a| a.to_string()); + while let Some(a) = args.next() { + assert!(control.consume(&a, &mut args)?, "{a}"); + } + Ok(control.port) + } + + #[test] + fn control_needs_a_port_from_1_to_65535() { + let need = "--control needs a port number from 1 to 65535"; + assert_eq!(control(&["--control"]), Err(need.into())); + assert_eq!(control(&["--control", "--automation-read-root"]), Err(format!("{need}, not `--automation-read-root`"))); + for bad in ["abc", "0", "70000", "-1", ""] { + assert_eq!(control(&["--control", bad]), Err(format!("{need}, not `{bad}`"))); + assert_eq!(control(&[&format!("--control={bad}")]), Err(format!("{need}, not `{bad}`"))); + } + assert_eq!(control(&["--control", "7979"]), Ok(Some(7979))); + assert_eq!(control(&["--control=65535"]), Ok(Some(65535))); + assert_eq!(control(&["--control", "7979", "--control=7980"]), Err("--control given twice".into())); + } + + #[test] + fn other_arguments_are_not_control() { + let mut control = ControlArg::default(); + let mut args = ["7979".to_owned()].into_iter(); + for other in ["--controls", "--control-port", "drawing.svg", "--in-window-menus"] { + assert!(!control.consume(other, &mut args).unwrap(), "{other}"); + } + assert_eq!(args.next().as_deref(), Some("7979")); + assert_eq!(control.port, None); + } + + #[test] + fn control_port_variable_is_a_port_or_unset() { + assert_eq!(env_control_port(None), Ok(None)); + assert_eq!(env_control_port(Some("".into())), Ok(None)); + assert_eq!(env_control_port(Some("7979".into())), Ok(Some(7979))); + let need = "VECTORCRAFT_CONTROL_PORT needs a port number from 1 to 65535"; + assert_eq!(env_control_port(Some("abc".into())), Err(format!("{need}, not `abc`"))); + assert_eq!(env_control_port(Some("0".into())), Err(format!("{need}, not `0`"))); + #[cfg(unix)] + { + use std::os::unix::ffi::OsStringExt; + assert_eq!(env_control_port(Some(OsString::from_vec(vec![0x39, 0xff]))), Err(format!("{need}, not `9\u{fffd}`"))); + } + } } diff --git a/apps/vectorcraft/src/main.rs b/apps/vectorcraft/src/main.rs index 07a5d94f..2fb4695d 100644 --- a/apps/vectorcraft/src/main.rs +++ b/apps/vectorcraft/src/main.rs @@ -6,7 +6,8 @@ //! `--in-window-menus` (or `VECTORCRAFT_IN_WINDOW_MENUS=1`) keeps the menus inside the window on //! macOS instead of the macOS menu bar (`mac_menu`). //! -//! `--control ` (or `VECTORCRAFT_CONTROL_PORT`) starts a localhost JSON-lines control server: +//! `--control ` (or `VECTORCRAFT_CONTROL_PORT`; a port from 1 to 65535, or the app doesn't +//! start) starts a localhost JSON-lines control server: //! `{"id":1,"method":"ui.inspect","params":{}}` → `{"id":1,"ok":true,"result":…}`. //! See `vectorcraft_ui_egui::control` for the methods. `--automation-read-root` and //! `--automation-write-root` (or `VECTORCRAFT_AUTOMATION_READ_ROOT` / `_WRITE_ROOT`) confine the @@ -605,7 +606,7 @@ fn native_options(custom_titlebar: bool) -> eframe::NativeOptions { fn main() -> std::process::ExitCode { // 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 mut control = automation_args::ControlArg::default(); 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")); @@ -613,7 +614,7 @@ fn main() -> std::process::ExitCode { 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) { + match roots_args.consume(&a, &mut args).and_then(|root| if root { Ok(true) } else { control.consume(&a, &mut args) }) { Ok(true) => continue, Ok(false) => {} Err(e) => { @@ -622,7 +623,6 @@ fn main() -> std::process::ExitCode { } } 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")); @@ -632,6 +632,17 @@ fn main() -> std::process::ExitCode { } } let automation_args::AutomationArgs { read_root, write_root, .. } = roots_args; + // After the arguments: `--control` and `--version` win over a bad environment value. + let control_port = match control.port { + Some(port) => Some(port), + None => match automation_args::env_control_port(std::env::var_os("VECTORCRAFT_CONTROL_PORT")) { + Ok(port) => port, + Err(e) => { + eprintln!("vectorcraft: {e}"); + return std::process::ExitCode::FAILURE; + } + }, + }; // Before desktop services, so every start-up warning is recorded (`logging`). let logger = logging::install(); vectorcraft_ui_egui::i18n::detect_system_lang_in_background(); diff --git a/apps/vectorcraft/tests/automation_args.rs b/apps/vectorcraft/tests/automation_args.rs index 8d2c667e..ffb8773e 100644 --- a/apps/vectorcraft/tests/automation_args.rs +++ b/apps/vectorcraft/tests/automation_args.rs @@ -9,12 +9,17 @@ 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) { + rejects_with_env(args, &[], message); +} + +fn rejects_with_env(args: &[&str], env: &[(&str, &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") + .envs(env.iter().copied()) .stdin(Stdio::null()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) @@ -68,3 +73,24 @@ fn explicit_dash_folder_does_not_consume_version() { assert_eq!(String::from_utf8(out.stdout).unwrap(), format!("vectorcraft {}\n", env!("CARGO_PKG_VERSION"))); assert!(out.stderr.is_empty()); } + +/// `--control` without a port no longer starts the app without a control server, and the +/// argument after it isn't reinterpreted (#1373). +#[test] +fn control_without_a_valid_port_fails_before_startup() { + let need = "--control needs a port number from 1 to 65535"; + rejects(&["--control", "--automation-read-root", ".", "--automation-write-root", "."], &format!("{need}, not `--automation-read-root`")); + rejects(&["--control"], need); + rejects(&["--control", "abc"], &format!("{need}, not `abc`")); + rejects(&["--control=0"], &format!("{need}, not `0`")); + rejects(&["--control="], &format!("{need}, not ``")); + rejects(&["--control", "7979", "--control", "7980"], "--control given twice"); + rejects_with_env(&[], &[("VECTORCRAFT_CONTROL_PORT", "abc")], "VECTORCRAFT_CONTROL_PORT needs a port number from 1 to 65535, not `abc`"); +} + +#[test] +fn version_wins_over_a_bad_control_port_variable() { + let out = Command::new(BIN).arg("--version").env("VECTORCRAFT_CONTROL_PORT", "abc").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"))); +} diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 719d8d0b..77c429ee 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -1,6 +1,8 @@ # Control protocol -`vectorcraft --control ` listens on `127.0.0.1:` (loopback only). One JSON request per line: +`vectorcraft --control ` listens on `127.0.0.1:` (loopback only). The port must be a number +from 1 to 65535 (`--control=` works too), given once; otherwise the app prints an error and exits +before it starts. One JSON request per line: `{"id": 1, "method": "ui.inspect", "params": {}}` → `{"id": 1, "ok": true, "result": {...}}` or `{"id":1,"ok":false,"error":"..."}`. Set `VECTORCRAFT_CONFIG_DIR` to a dedicated directory for an isolated native test profile. Preferences, diff --git a/docs/development.md b/docs/development.md index 665246a9..6bd3d391 100644 --- a/docs/development.md +++ b/docs/development.md @@ -78,7 +78,7 @@ By default VectorCraft's own crates log at `info` and everything else at `warn`. | Variable | Effect | |---|---| -| `VECTORCRAFT_CONTROL_PORT` | Same as `--control ` | +| `VECTORCRAFT_CONTROL_PORT` | Same as `--control `, which overrides it; a value that isn't a port from 1 to 65535 stops the app | | `VECTORCRAFT_IN_WINDOW_MENUS` | Any value but empty or `0`: same as `--in-window-menus` (see [macOS: the menu bar](#macos-the-menu-bar)) | | `VECTORCRAFT_NO_PREFS` | No preferences read or written, no default Data Recovery folder and no log file (agents' test runs) | | `WGPU_POWER_PREF` | Graphics adapter: `low`, `high` or `none` (see [Desktop graphics processor](#desktop-graphics-processor)) | From de777a1b094a0cea4810f37d52808379e207d082 Mon Sep 17 00:00:00 2001 From: RealtaNua Date: Mon, 12 Oct 2026 17:40:19 +0700 Subject: [PATCH 2/2] Contributors: Nael-Nathanael's entry (GitHub profile name) --- contributors/people.toml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contributors/people.toml b/contributors/people.toml index 783d8271..4a5b3694 100644 --- a/contributors/people.toml +++ b/contributors/people.toml @@ -86,3 +86,6 @@ sync_github_name = false real_name = "Aaron Guo" display_name = "Aaron Guo" sync_github_name = false + +[people.Nael-Nathanael] +sync_github_name = true