diff --git a/apps/effectcraft/src/main.rs b/apps/effectcraft/src/main.rs index 83015f1a..f5ad8063 100644 --- a/apps/effectcraft/src/main.rs +++ b/apps/effectcraft/src/main.rs @@ -1,6 +1,8 @@ //! EffectCraft desktop app. //! -//! Usage: `effectcraft [--control ] [--demo|--empty|--home] [project.ecproj | media files…]` +//! Usage: `effectcraft [--control ] [--demo|--empty|--home] [--] [project.ecproj | media files…]` +//! +//! An unknown option or a bad port prints an error and exits with status 1 before anything starts. //! //! Without a project the app starts with an empty Untitled Project, as After Effects does; //! `--demo` opens the demo project instead (also Help ▸ Open Demo Project). @@ -70,22 +72,51 @@ struct Args { home: Option, } -/// Parse the arguments after the program name; `None` for `--version`. `env_port` is -/// `EFFECTCRAFT_CONTROL_PORT`. -fn parse_args(args: impl IntoIterator, env_port: Option) -> Option { - let mut out = Args { control_port: env_port.and_then(|p| p.parse().ok()), ..Default::default() }; +/// Parse the arguments after the program name; `Ok(None)` for `--version`. `env_port` is +/// `EFFECTCRAFT_CONTROL_PORT`, used when `--control` isn't given. An unknown option or a bad port +/// is an error, so it is neither imported as a file nor dropped; after `--` every argument is a file. +fn parse_args(args: impl IntoIterator, env_port: Result) -> Result, String> { + let mut out = Args::default(); let mut args = args.into_iter(); + let mut options = true; while let Some(a) = args.next() { + if !options { + out.files.push(a); + continue; + } match a.as_str() { - "--control" => out.control_port = args.next().and_then(|p| p.parse().ok()), + "--" => options = false, + "--control" => out.control_port = Some(control_port("--control", args.next().as_deref())?), "--demo" => out.demo = true, "--empty" => out.demo = false, "--home" => out.home = Some(true), - "--version" => return None, - _ => out.files.push(a), + "--version" => return Ok(None), + _ => match a.strip_prefix("--control=") { + Some(v) => out.control_port = Some(control_port("--control", Some(v))?), + None if a.starts_with('-') && a != "-" => return Err(format!("unknown option `{a}`")), + None => out.files.push(a), + }, } } - Some(out) + if out.control_port.is_none() { + out.control_port = match env_port { + Ok(v) if v.is_empty() => None, + Ok(v) => Some(control_port("EFFECTCRAFT_CONTROL_PORT", Some(&v))?), + Err(std::env::VarError::NotPresent) => None, + Err(std::env::VarError::NotUnicode(v)) => Some(control_port("EFFECTCRAFT_CONTROL_PORT", Some(&v.to_string_lossy()))?), + }; + } + Ok(Some(out)) +} + +/// A control-channel port from 1 to 65535 (0 would listen on a port nobody is told); `name` is +/// the flag or variable it came from. +fn control_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}`")), + } } fn main() -> eframe::Result { @@ -94,9 +125,17 @@ fn main() -> eframe::Result { // feed the compatibility report. effectcraft_engine::logging::install(); effectcraft_engine::logging::install_panic_hook(); - let Some(Args { control_port, files, demo, home }) = parse_args(std::env::args().skip(1), std::env::var("EFFECTCRAFT_CONTROL_PORT").ok()) else { - println!("effectcraft {}", env!("CARGO_PKG_VERSION")); - return Ok(()); + let Args { control_port, files, demo, home } = match parse_args(std::env::args().skip(1), std::env::var("EFFECTCRAFT_CONTROL_PORT")) { + Ok(Some(args)) => args, + Ok(None) => { + println!("effectcraft {}", env!("CARGO_PKG_VERSION")); + return Ok(()); + } + // Before the session log, preferences and the window: nothing has started yet. + Err(e) => { + eprintln!("effectcraft: {e}"); + std::process::exit(1); + } }; // The session log lives in the settings folder (`Logs/effectcraft.log`, the previous two runs // kept): a launch from the Dock or a menu has no terminal. Opened after the arguments, so @@ -425,9 +464,18 @@ fn disable_app_nap() { #[cfg(test)] mod tests { use super::{Args, parse_args}; + use std::env::VarError; + + fn parse_env(args: &[&str], env_port: Result) -> Result, String> { + parse_args(args.iter().map(|a| a.to_string()), env_port) + } fn parse(args: &[&str]) -> Option { - parse_args(args.iter().map(|a| a.to_string()), None) + parse_env(args, Err(VarError::NotPresent)).unwrap() + } + + fn fails(args: &[&str]) -> String { + parse_env(args, Err(VarError::NotPresent)).unwrap_err() } #[cfg(target_os = "linux")] @@ -477,6 +525,53 @@ mod tests { let a = parse(&["--control", "9877", "--home", "a.ecproj"]).unwrap_or_default(); assert_eq!((a.control_port, a.home, a.files), (Some(9877), Some(true), vec!["a.ecproj".to_string()])); assert_eq!(parse(&["--version"]), None); - assert_eq!(parse_args(Vec::new(), Some("9000".into())).map(|a| a.control_port), Some(Some(9000))); + assert_eq!(parse_env(&[], Ok("9000".into())).unwrap().map(|a| a.control_port), Some(Some(9000))); + } + + /// A missing, non-numeric or zero `--control` value stops the launch instead of starting + /// without a control server, and the argument after `--control` isn't reinterpreted (#997). + #[test] + fn control_needs_a_port_from_1_to_65535() { + let need = "--control needs a port number from 1 to 65535"; + assert_eq!(fails(&["--control"]), need); + assert_eq!(fails(&["--control", "--demo"]), format!("{need}, not `--demo`")); + assert_eq!(fails(&["--control", "--version"]), format!("{need}, not `--version`")); + for bad in ["abc", "0", "70000", "-1", ""] { + assert_eq!(fails(&["--control", bad]), format!("{need}, not `{bad}`")); + assert_eq!(fails(&[&format!("--control={bad}")]), format!("{need}, not `{bad}`")); + } + assert_eq!(parse(&["--control=9877"]).map(|a| a.control_port), Some(Some(9877))); + assert_eq!(parse(&["--control", "65535"]).map(|a| a.control_port), Some(Some(65535))); + } + + /// An unknown option is an error, not a file to import (#997); `--` ends the options. + #[test] + fn unknown_options_are_errors_not_files() { + for bad in ["--automation-read-root", "--automation-write-root=/tmp", "-h", "--Demo", "--demo=yes"] { + assert_eq!(fails(&[bad, "/tmp/folder"]), format!("unknown option `{bad}`")); + } + // Parsed left to right: an error before `--version` wins. + assert_eq!(fails(&["--nope", "--version"]), "unknown option `--nope`"); + let a = parse(&["clip.mov", "-", "--", "--demo", "-name.png"]).unwrap_or_default(); + assert_eq!(a.files, ["clip.mov", "-", "--demo", "-name.png"]); + assert!(!a.demo); + } + + /// `--control` wins over `EFFECTCRAFT_CONTROL_PORT`; the variable is checked only when it is + /// used, so a bad value doesn't stop `--control` or `--version`. + #[test] + fn control_port_variable_is_checked_when_used() { + let need = "EFFECTCRAFT_CONTROL_PORT needs a port number from 1 to 65535"; + assert_eq!(parse_env(&[], Ok("abc".into())).unwrap_err(), format!("{need}, not `abc`")); + assert_eq!(parse_env(&[], Ok("0".into())).unwrap_err(), format!("{need}, not `0`")); + assert_eq!(parse_env(&[], Ok(String::new())), Ok(Some(Args::default()))); + assert_eq!(parse_env(&["--control", "9877"], Ok("abc".into())).unwrap().map(|a| a.control_port), Some(Some(9877))); + assert_eq!(parse_env(&["--version"], Ok("abc".into())), Ok(None)); + #[cfg(unix)] + { + use std::os::unix::ffi::OsStringExt; + let not_utf8 = VarError::NotUnicode(std::ffi::OsString::from_vec(vec![0x39, 0xff])); + assert_eq!(parse_env(&[], Err(not_utf8)).unwrap_err(), format!("{need}, not `9\u{fffd}`")); + } } } diff --git a/apps/effectcraft/tests/args.rs b/apps/effectcraft/tests/args.rs new file mode 100644 index 00000000..8367d2a9 --- /dev/null +++ b/apps/effectcraft/tests/args.rs @@ -0,0 +1,73 @@ +//! The desktop binary rejects a bad command line before it starts anything (#997). +// Integration tests: unwrapping and panicking on failure is fine here. +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + +use std::path::PathBuf; +use std::process::{Command, Stdio}; +use std::time::{Duration, Instant}; + +/// A settings folder of its own, so a launch that got past the arguments would show up as +/// preferences (Recent Footage) written there instead of in the user's settings. +fn config_dir(name: &str) -> PathBuf { + let d = std::env::temp_dir().join(format!("ec-args-test-{}-{name}", std::process::id())); + let _ = std::fs::remove_dir_all(&d); + std::fs::create_dir_all(&d).unwrap(); + d +} + +/// Runs the app, which must exit at once with `effectcraft: ` and a failure status. A +/// regression fails instead of leaving a window open: the process is killed after 5 s. +fn rejects(name: &str, args: &[&str], env: &[(&str, &str)], message: &str) { + let config = config_dir(name); + let mut cmd = Command::new(env!("CARGO_BIN_EXE_effectcraft")); + cmd.args(args).env_remove("EFFECTCRAFT_CONTROL_PORT").env_remove("RUST_LOG").env("EFFECTCRAFT_CONFIG_DIR", &config); + for (k, v) in env { + cmd.env(k, v); + } + let mut child = cmd.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:?}: the app started or did not exit promptly"); + assert_eq!(out.status.code(), Some(1), "{args:?}"); + assert!(out.stdout.is_empty(), "{args:?}: {:?}", String::from_utf8_lossy(&out.stdout)); + assert_eq!(String::from_utf8(out.stderr).unwrap(), format!("effectcraft: {message}\n"), "{args:?}"); + let written: Vec<_> = std::fs::read_dir(&config).unwrap().collect(); + assert!(written.is_empty(), "{args:?}: wrote {written:?}"); + let _ = std::fs::remove_dir_all(&config); +} + +#[test] +fn unknown_options_stop_the_launch_before_anything_is_imported() { + let folder = config_dir("folder"); + rejects("read-root", &["--control", "47613", "--automation-read-root", folder.to_str().unwrap()], &[], "unknown option `--automation-read-root`"); + rejects("write-root", &["--automation-write-root=/tmp"], &[], "unknown option `--automation-write-root=/tmp`"); + rejects("short", &["-h"], &[], "unknown option `-h`"); + let _ = std::fs::remove_dir_all(&folder); +} + +#[test] +fn a_bad_control_port_stops_the_launch() { + let need = "--control needs a port number from 1 to 65535"; + rejects("missing", &["--control"], &[], need); + rejects("flag", &["--control", "--demo"], &[], &format!("{need}, not `--demo`")); + rejects("text", &["--control", "abc"], &[], &format!("{need}, not `abc`")); + rejects("zero", &["--control=0"], &[], &format!("{need}, not `0`")); + rejects("env", &[], &[("EFFECTCRAFT_CONTROL_PORT", "abc")], "EFFECTCRAFT_CONTROL_PORT needs a port number from 1 to 65535, not `abc`"); +} + +#[test] +fn version_still_prints_with_a_bad_port_variable() { + let out = Command::new(env!("CARGO_BIN_EXE_effectcraft")).arg("--version").env("EFFECTCRAFT_CONTROL_PORT", "abc").output().unwrap(); + assert!(out.status.success(), "{}", String::from_utf8_lossy(&out.stderr)); + assert_eq!(String::from_utf8(out.stdout).unwrap(), format!("effectcraft {}\n", env!("CARGO_PKG_VERSION"))); +} diff --git a/contributors/people.toml b/contributors/people.toml index babbefe6..9a04d8d1 100644 --- a/contributors/people.toml +++ b/contributors/people.toml @@ -67,3 +67,6 @@ display_name = "Jason Brashear" [people.nanox333] display_name = "nanox333" + +[people.Nael-Nathanael] +sync_github_name = true diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 41458cd3..f5572959 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -6,6 +6,9 @@ The desktop app exposes a control channel for agents, tests and the MCP bridge: effectcraft --control 9877 # or EFFECTCRAFT_CONTROL_PORT=9877 ``` +The port must be a number from 1 to 65535, and `--control` overrides the variable. A bad port or an +unknown option prints an error and the app exits with status 1 before it opens a window. + - **Transport:** TCP on `127.0.0.1` only, one JSON object per line in each direction. Connections are persistent; requests on one connection are answered in order. - **Request:** `{"id": 1, "method": "engine.execute", "params": {...}}` (`id` is echoed back, `params`