From a0d1162976fac1e4a8b8fd49a3948e9d6bad6f40 Mon Sep 17 00:00:00 2001 From: skylar taylor-barrick Date: Sun, 18 Oct 2026 15:20:15 -0600 Subject: [PATCH] Desktop: refuse unknown command-line options instead of ignoring them (#836) (#844) A mistyped or unsupported flag (e.g. --automation-write-roots) used to be dropped silently, so the app started unconfined. It now exits 2 with 'unknown option', like a bad automation root. A bare -- ends the options. Adds a desktop control-channel test that file.save / file.saveAs outside the write root are refused and create nothing, while saves inside succeed, and documents that the root flags arrived in 0.7.0 (0.6.0 ignores them). --- apps/figcraft/src/args.rs | 14 ++++++++++++-- apps/figcraft/src/args_tests.rs | 15 ++++++++++++++- apps/figcraft/src/main.rs | 9 ++++++++- crates/ui-egui/src/control_roots_tests.rs | 18 ++++++++++++++++++ docs/control-protocol.md | 5 ++++- 5 files changed, 56 insertions(+), 5 deletions(-) diff --git a/apps/figcraft/src/args.rs b/apps/figcraft/src/args.rs index 3b4136d9..11798f5b 100644 --- a/apps/figcraft/src/args.rs +++ b/apps/figcraft/src/args.rs @@ -2,7 +2,9 @@ //! [--automation-write-root ]… [files…] [figcraft://join?…]`. An invite link //! (what the system passes when a `figcraft://` link is opened) joins that session once the window //! is up; everything else that isn't a flag is a file to open, as a path or a `file://` URL (the -//! Linux desktop entry passes URLs, `%U`, so that it can also hand over invite links). +//! Linux desktop entry passes URLs, `%U`, so that it can also hand over invite links). Any other +//! `--flag` is unknown and stops the app at startup (#836): a mistyped root flag must never leave +//! the app running unconfined. After a bare `--`, every argument is a file. /// What the app was started with. #[derive(Debug, Default, PartialEq, Eq)] @@ -17,6 +19,8 @@ pub struct Args { /// which is refused at startup. pub read_roots: Vec, pub write_roots: Vec, + /// `--flags` the app doesn't know, as given. + pub unknown: Vec, } /// An invite link rather than a file (`figcraft:` links, in any case). @@ -55,6 +59,10 @@ pub fn parse(args: &[String]) -> Args { let mut out = Args::default(); let mut it = args.iter(); while let Some(a) = it.next() { + if a == "--" { + out.files.extend(it.by_ref().cloned()); + break; + } let (flag, inline) = a.split_once('=').map_or((a.as_str(), None), |(f, v)| (f, Some(v.to_string()))); let roots = match flag { "--automation-read-root" => Some(&mut out.read_roots), @@ -69,7 +77,9 @@ pub fn parse(args: &[String]) -> Args { out.join = Some(a.clone()); } else if let Some(path) = file_url_path(a) { out.files.push(path); - } else if !a.starts_with("--") { + } else if a.starts_with("--") { + out.unknown.push(a.clone()); + } else { out.files.push(a.clone()); } } diff --git a/apps/figcraft/src/args_tests.rs b/apps/figcraft/src/args_tests.rs index 2ae29b3c..9c23a3f4 100644 --- a/apps/figcraft/src/args_tests.rs +++ b/apps/figcraft/src/args_tests.rs @@ -9,7 +9,7 @@ fn args(a: &[&str]) -> Vec { #[test] fn files_and_the_control_port() { assert_eq!(parse(&args(&[])), Args::default()); - let a = parse(&args(&["--control", "4242", "a.figcraft", "--verbose", "b.svg"])); + let a = parse(&args(&["--control", "4242", "a.figcraft", "b.svg"])); assert_eq!(a, Args { control: Some(4242), files: args(&["a.figcraft", "b.svg"]), ..Args::default() }); // A missing or bad port is ignored, not taken as a file. assert_eq!(parse(&args(&["--control"])), Args::default()); @@ -26,6 +26,19 @@ fn automation_roots_repeat_and_are_never_files() { assert_eq!(parse(&args(&["--automation-write-root"])).write_roots, args(&[""])); } +#[test] +fn unknown_flags_are_refused_never_ignored() { + // #836: a mistyped root flag (or one an older release lacks) must stop the app, not start it + // unconfined. Both forms, with and without `=`. + let a = parse(&args(&["--automation-write-roots", "out", "--automation-write-dir=out", "--verbose", "a.figcraft"])); + assert_eq!(a.unknown, args(&["--automation-write-roots", "--automation-write-dir=out", "--verbose"])); + assert!(a.write_roots.is_empty()); + // Single-dash arguments are left as they were (files); after `--` everything is a file. + let a = parse(&args(&["--control", "1", "--", "--odd.figcraft", "--automation-write-root=x"])); + assert_eq!(a.files, args(&["--odd.figcraft", "--automation-write-root=x"])); + assert!(a.unknown.is_empty() && a.write_roots.is_empty()); +} + #[test] fn invite_links_join_instead_of_opening() { let link = "figcraft://join?host=192.168.1.20:47900&file=abc&token=AbCdEfGhIjKlMnOpQrStUv"; diff --git a/apps/figcraft/src/main.rs b/apps/figcraft/src/main.rs index ddb70204..1bba81e8 100644 --- a/apps/figcraft/src/main.rs +++ b/apps/figcraft/src/main.rs @@ -10,7 +10,8 @@ //! `--automation-read-root ` / `--automation-write-root ` (each repeatable) confine the //! files the window and the control channel read and write (`figcraft_engine::file_access`); the //! app's own folders (settings, recovery copies, installed plugins) and opening the files given on -//! the command line are not. A root that isn't an existing folder stops the app at startup. +//! the command line are not. A root that isn't an existing folder stops the app at startup, and so +//! does any flag the app doesn't know (a mistyped root flag must not start the app unconfined). #![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] #![cfg_attr(all(target_os = "windows", not(debug_assertions)), windows_subsystem = "windows")] @@ -129,6 +130,12 @@ fn main() -> eframe::Result { return Ok(()); } let args = args::parse(&args); + if let Some(flag) = args.unknown.first() { + eprintln!( + "FigCraft: unknown option {flag}\nUsage: figcraft [--control PORT] [--automation-read-root DIR]... [--automation-write-root DIR]... [FILE | figcraft://join?...]..." + ); + std::process::exit(2); + } let roots = match ( args.read_roots.is_empty() && args.write_roots.is_empty(), figcraft_engine::file_access::Roots::new(&args.read_roots, &args.write_roots), diff --git a/crates/ui-egui/src/control_roots_tests.rs b/crates/ui-egui/src/control_roots_tests.rs index cc006506..35454bb5 100644 --- a/crates/ui-egui/src/control_roots_tests.rs +++ b/crates/ui-egui/src/control_roots_tests.rs @@ -57,6 +57,24 @@ fn a_confined_app_writes_only_inside_its_write_root() { assert!(app.session.execute("file.export", &json!({"format": "svg", "path": p("outside/own.svg")})).is_ok()); } +#[test] +fn a_confined_app_saves_only_inside_its_write_root() { + // #836: saving over the control channel, both as a new file and to the current one. + let (mut app, d) = confined_app("save"); + let p = |rel: &str| d.0.join(rel).to_string_lossy().into_owned(); + for cmd in ["file.saveAs", "file.save"] { + let e = handle(&mut app, "engine.execute", json!({"command": cmd, "params": {"path": p("outside/a.figcraft")}})).unwrap_err(); + assert!(e.contains("automation path rejected"), "{cmd}: {e}"); + let e = handle(&mut app, "engine.execute", json!({"command": cmd, "params": {"path": p("out/../outside/b.figcraft")}})).unwrap_err(); + assert!(e.contains("automation path rejected"), "{cmd}: {e}"); + } + assert_eq!(std::fs::read_dir(d.0.join("outside")).unwrap().count(), 0, "nothing written outside the root"); + handle(&mut app, "engine.execute", json!({"command": "file.saveAs", "params": {"path": p("out/in.figcraft")}})).unwrap(); + assert!(d.0.join("out/in.figcraft").is_file()); + // Saving again goes to the same file, inside the root. + handle(&mut app, "engine.execute", json!({"command": "file.save", "params": {}})).unwrap(); +} + #[test] fn window_captures_are_written_through_the_roots_or_returned() { let (app, d) = confined_app("capture"); diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 3ae9d287..858d2435 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -71,7 +71,10 @@ and what a person does in the window are held to the same folders. Files are rea read or write root and written only inside a write root; anything else is an ordinary error reply (`automation path rejected: …`), and recent files outside the roots aren't listed. Paths are resolved as `docs/mcp.md` › Confining file access describes. A root that isn't an existing folder -stops the app before its window opens. Without the flags nothing changes. +stops the app before its window opens, and so does any option the app doesn't know (a mistyped +flag such as `--automation-write-roots` exits with `unknown option`, never starts unconfined). +Without the flags nothing changes. The flags arrived in FigCraft 0.7.0: 0.6.0 and earlier ignore +them and confine nothing. Not confined: opening the files given on the command line when the app starts and the documents the system hands over (Finder, Open With; saving them later is confined like any write), and the