From 755266cf9f93b2a7cf1a221e342b733917d8f7c3 Mon Sep 17 00:00:00 2001 From: Hamza Date: Wed, 21 Oct 2026 18:45:04 +0500 Subject: [PATCH 1/2] cli: the argument bugs photocraft#7071 lists are live here too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of them, measured on main: pdfcraft-cli extract f.pdf --pages 1 --out --password (exit 0, and the PDF is in a file called `--password`) pdfcraft-cli check-one --edit f.pdf pdfcraft-cli: check-one: missing file pdfcraft-cli info /tmp/café.pdf (that é as one Latin-1 byte) thread 'main' panicked at library/std/src/env.rs:878:51: called `Result::unwrap()` on an `Err` value - An option whose value is a **file or folder** refuses a value that is another option, and names the way out for a file that really starts with `-`: `./--name`, as `--root` already offered. That also closes the gap the option walk left for those: an unknown option in a value position was skipped along with the option it followed, so `combine --out --nonsense` wrote `--nonsense` instead of saying what `--nonsense` was. Only those options. A password or a title can be any string, `--page` included — `an_option_shaped_password_is_not_parsed_as_a_page_flag` pins that, and a bad `--page` or `--dpi` is reported by the command in its own terms either way. - `positional` asks the option table which tokens are values, instead of skipping the one after every option. The token after a flag that takes none is an argument: `check-one --edit f.pdf` read the file as `--edit`'s operand and then reported it missing. - Arguments come through `args_os`, so one that is not UTF-8 is a usage error rather than a panic. `std::env::args` panics while collecting them, so even `--help` ended that way. tests/options.rs covers each, and each fails without its fix (the file called `--password` comes back; "check-one: missing file"; exit 101 with that report). --- apps/pdfcraft-cli/src/main.rs | 116 ++++++++++++++++++++++------- apps/pdfcraft-cli/tests/options.rs | 81 ++++++++++++++++++++ 2 files changed, 169 insertions(+), 28 deletions(-) diff --git a/apps/pdfcraft-cli/src/main.rs b/apps/pdfcraft-cli/src/main.rs index 5888e5111..3fd67e3bb 100644 --- a/apps/pdfcraft-cli/src/main.rs +++ b/apps/pdfcraft-cli/src/main.rs @@ -84,7 +84,24 @@ help and feedback: https://discord.gg/artcraft" ); fn main() -> ExitCode { - let args: Vec = std::env::args().skip(1).collect(); + // `std::env::args` panics on an argument that is not UTF-8 ("called `Result::unwrap()` on an + // `Err` value"), which a Latin-1 file name is, and it panics while collecting them: even + // `--help` ended that way. Such a name cannot be passed on either, since the paths here are + // `str`, so it is a usage error (photocraft#1108). + let mut args: Vec = Vec::new(); + for arg in std::env::args_os().skip(1) { + match arg.into_string() { + Ok(a) => args.push(a), + Err(bad) => { + let _ = writeln!( + std::io::stderr().lock(), + "pdfcraft-cli: `{}` is not valid UTF-8, so it cannot be used as an argument", + bad.to_string_lossy() + ); + return ExitCode::FAILURE; + } + } + } // ` --help` too; only right after the command, so a later `-h` (a value) still reaches it. let command = match args.get(1).map(String::as_str) { Some("--help" | "-h") => Some("--help"), @@ -169,7 +186,7 @@ fn version() -> Result<(), CliError> { /// found so scripts and CI can gate on it, and when the audit itself cannot run. fn audit(args: &[String]) -> Result<(), CliError> { validate_options("audit", args, AUDIT_OPTIONS)?; - let path = *positional(args).first().ok_or("audit: missing file")?; + let path = *positional(args, AUDIT_OPTIONS).first().ok_or("audit: missing file")?; let bytes = read(path)?; let password = flag(args, "--password"); let doc = pdfcraft_cos::Document::open_with_password(bytes.clone(), password).map_err(|e| format!("{path}: {e}"))?; @@ -203,6 +220,28 @@ struct OptionSpec { takes_value: bool, } +/// The options whose value is a file or folder. Only these refuse a value that is another +/// option: a file named `--password` is never what was meant, while a password can be anything +/// (`--password --page` is a password of `--page`, which tests/text.rs pins), and so can a title. +/// The way to name a file that does start with `-` is `./--name`, as `--root` already offered. +const PATH_VALUE_OPTIONS: [&str; 6] = ["--out", "--out-dir", "--json", "--root", "--control", "--script"]; + +/// Whether a token can be the value of a path option. Another option cannot: `--out --password` +/// used to write the PDF to a file called `--password` and exit 0 (photocraft#6944). +fn can_be_path_value(name: &str, value: Option<&str>) -> bool { + !PATH_VALUE_OPTIONS.contains(&name) || value.is_some_and(|v| !v.is_empty() && !v.starts_with("--")) +} + +/// The complaint about a path option whose value is missing or is another option. +fn needs_value(command: &str, name: &str, value: Option<&str>) -> String { + match value { + Some(v) if !v.is_empty() => { + format!("{command}: {name} needs a value and `{v}` is another option; write `./{v}` for a file of that name") + } + _ => format!("{command}: {name} needs a value"), + } +} + /// Refuse unknown options and malformed confinement flags before any file is read. fn validate_options(command: &str, args: &[String], specs: &[OptionSpec]) -> Result<(), CliError> { if specs.iter().any(|spec| spec.name == "--root") { @@ -225,9 +264,19 @@ fn validate_options(command: &str, args: &[String], specs: &[OptionSpec]) -> Res let Some(spec) = specs.iter().find(|spec| spec.name == arg) else { return Err(format!("{command}: unknown option --{name}").into()); }; - // Other missing or malformed values are left to the command, which reports them in its own - // terms (`text` and `extract` explain what a page value must be). - i += if spec.takes_value { 2 } else { 1 }; + // A file's name is not another option, which also closes the gap the walk left for + // those: an unknown option in that value position was skipped along with it. Other + // values, missing or malformed, are left to the command, which reports them in its + // own terms (`text` and `extract` explain what a page value must be). + i += if spec.takes_value { + let value = args.get(i + 1).map(String::as_str); + if !can_be_path_value(spec.name, value) { + return Err(needs_value(command, spec.name, value).into()); + } + 2 + } else { + 1 + }; } else { i += 1; } @@ -273,19 +322,24 @@ const UI_OPTIONS: &[OptionSpec] = &[OptionSpec { name: "--control", takes_value: #[cfg(feature = "mcp")] const MCP_OPTIONS: &[OptionSpec] = &[OptionSpec { name: "--root", takes_value: true }, OptionSpec { name: "--compact", takes_value: false }]; -fn positional(args: &[String]) -> Vec<&str> { +/// The arguments that are not options or their values. Which tokens are values is the spec's +/// answer: skipping the one after every option made `check-one --edit file.pdf` report "missing +/// file", the file having been read as `--edit`'s value. +fn positional<'a>(args: &'a [String], specs: &[OptionSpec]) -> Vec<&'a str> { let mut out = Vec::new(); - let mut skip = false; - for a in args { - if skip { - skip = false; - continue; + let mut i = 0; + while let Some(a) = args.get(i) { + match specs.iter().find(|spec| spec.name == a.as_str()) { + Some(spec) if spec.takes_value => i += 2, + // An option that takes no value, and an unknown one, which `validate_options` has + // already refused by the time this runs. + Some(_) => i += 1, + None if a.starts_with("--") => i += 1, + None => { + out.push(a.as_str()); + i += 1; + } } - if a.starts_with("--") { - skip = true; - continue; - } - out.push(a.as_str()); } out } @@ -296,7 +350,7 @@ fn read(path: &str) -> Result>, String> { fn info(args: &[String]) -> Result<(), CliError> { validate_options("info", args, INFO_OPTIONS)?; - let path = *positional(args).first().ok_or("info: missing file")?; + let path = *positional(args, INFO_OPTIONS).first().ok_or("info: missing file")?; let bytes = read(path)?; let password = flag(args, "--password"); let mut info = inspect(bytes.clone(), password).map_err(|e| e.to_string())?; @@ -347,7 +401,7 @@ fn info(args: &[String]) -> Result<(), CliError> { fn text(args: &[String]) -> Result<(), CliError> { validate_options("text", args, TEXT_OPTIONS)?; - let path = *positional(args).first().ok_or("text: missing file")?; + let path = *positional(args, TEXT_OPTIONS).first().ok_or("text: missing file")?; let mut selected_page = None; // Validate every supplied value before reading, retaining the first valid selection. let mut options = args.iter(); @@ -472,7 +526,13 @@ fn edit(args: &[String]) -> Result<(), CliError> { continue; } } - let value = args.get(i + 1).map(String::as_str).ok_or_else(|| format!("edit: {arg} needs a value"))?; + // `edit` parses its own options (they run in order), so `--out` is checked here in the + // same way: it wrote a PDF to a file called `--full`. + let value = args.get(i + 1).map(String::as_str); + if !can_be_path_value(arg, value) { + return Err(needs_value("edit", arg, value).into()); + } + let value = value.ok_or_else(|| format!("edit: {arg} needs a value"))?; match arg { "--out" => out = Some(value), "--password" => password = Some(value), @@ -545,7 +605,7 @@ fn file_stem(path: &str) -> String { fn combine(args: &[String]) -> Result<(), CliError> { validate_options("combine", args, COMBINE_OPTIONS)?; let out = flag(args, "--out").ok_or("combine: missing --out")?; - let inputs = positional(args); + let inputs = positional(args, COMBINE_OPTIONS); if inputs.len() < 2 { return Err("combine: give at least two input files".into()); } @@ -556,7 +616,7 @@ fn combine(args: &[String]) -> Result<(), CliError> { fn extract(args: &[String]) -> Result<(), CliError> { validate_options("extract", args, EXTRACT_OPTIONS)?; - let path = *positional(args).first().ok_or("extract: missing file")?; + let path = *positional(args, EXTRACT_OPTIONS).first().ok_or("extract: missing file")?; let out = flag(args, "--out").ok_or("extract: missing --out")?; let pages = page_list(flag(args, "--pages").ok_or("extract: missing --pages")?)?; let mut session = pdfcraft_engine::Session::new(); @@ -568,7 +628,7 @@ fn extract(args: &[String]) -> Result<(), CliError> { fn split(args: &[String]) -> Result<(), CliError> { validate_options("split", args, SPLIT_OPTIONS)?; use pdfcraft_engine::SplitBy; - let path = *positional(args).first().ok_or("split: missing file")?; + let path = *positional(args, SPLIT_OPTIONS).first().ok_or("split: missing file")?; let by = match (flag(args, "--every"), flag(args, "--before")) { (Some(n), None) => SplitBy::PageCount(n.parse().map_err(|_| "bad --every")?), (None, Some(list)) => SplitBy::Before(page_list(list)?), @@ -592,7 +652,7 @@ fn split(args: &[String]) -> Result<(), CliError> { fn render(args: &[String]) -> Result<(), CliError> { validate_options("render", args, RENDER_OPTIONS)?; - let path = *positional(args).first().ok_or("render: missing file")?; + let path = *positional(args, RENDER_OPTIONS).first().ok_or("render: missing file")?; let page: usize = flag(args, "--page").unwrap_or("1").parse().map_err(|_| "bad --page")?; let page_index = page.checked_sub(1).ok_or("--page must be at least 1")?; let dpi: f32 = flag(args, "--dpi").unwrap_or("96").parse().map_err(|_| "bad --dpi")?; @@ -646,7 +706,7 @@ fn render(args: &[String]) -> Result<(), CliError> { /// Child-process body for `check`: prints one JSON line. fn check_one(args: &[String]) -> Result<(), CliError> { validate_options("check-one", args, CHECK_ONE_OPTIONS)?; - let path = *positional(args).first().ok_or("check-one: missing file")?; + let path = *positional(args, CHECK_ONE_OPTIONS).first().ok_or("check-one: missing file")?; let dpi: f32 = flag(args, "--dpi").unwrap_or("36").parse().map_err(|_| "bad --dpi")?; let start = Instant::now(); let bytes = read(path)?; @@ -739,7 +799,7 @@ fn collect(paths: &[&str]) -> Vec { fn check(args: &[String]) -> Result<(), CliError> { validate_options("check", args, CHECK_OPTIONS)?; - let files = collect(&positional(args)); + let files = collect(&positional(args, CHECK_OPTIONS)); if files.is_empty() { return Err("check: no PDF files found".into()); } @@ -924,9 +984,9 @@ fn run(args: &[String]) -> Result<(), CliError> { } return Ok(()); } - let tool = *positional(args).first().ok_or("run: missing tool name (see `pdfcraft-cli tools`)")?; + let tool = *positional(args, RUN_OPTIONS).first().ok_or("run: missing tool name (see `pdfcraft-cli tools`)")?; let mut obj = serde_json::Map::new(); - for kv in positional(args).iter().skip(1) { + for kv in positional(args, RUN_OPTIONS).iter().skip(1) { let (k, v) = kv.split_once('=').ok_or(format!("run: expected key=value, got {kv:?}"))?; let value = serde_json::from_str(v).unwrap_or_else(|_| serde_json::Value::String(v.to_string())); obj.insert(k.to_string(), value); @@ -992,7 +1052,7 @@ fn ui(args: &[String]) -> Result<(), CliError> { // Refused rather than truncated, which would quietly reach some other port. let port = u16::try_from(port).map_err(|_| format!("{file}: {port} is not a port number"))?; let token = info["token"].as_str().ok_or(format!("{file}: no token"))?; - let pos = positional(args); + let pos = positional(args, UI_OPTIONS); let method = *pos.first().ok_or("ui: missing method (state, inspect, click, drag, type, key, command, commands, set, open, screenshot)")?; let method = if method.starts_with("ui.") { method.to_string() } else { format!("ui.{method}") }; let mut params = serde_json::Map::new(); diff --git a/apps/pdfcraft-cli/tests/options.rs b/apps/pdfcraft-cli/tests/options.rs index 9576d59a8..14b3e617c 100644 --- a/apps/pdfcraft-cli/tests/options.rs +++ b/apps/pdfcraft-cli/tests/options.rs @@ -27,3 +27,84 @@ fn non_edit_commands_reject_unknown_options_before_io() { assert!(stderr.contains("unknown option") && stderr.contains(unknown), "{args:?}: {stderr}"); } } + +/// photocraft#6944: an option took the next token whatever it was, so a line with a value missing +/// in the middle of it did something else entirely — `extract in.pdf --pages 1 --out --password` +/// wrote the PDF to a file called `--password` and exited 0. The unknown option hidden in a value +/// position passed the check above too, since the walk skipped it along with the option. +/// +/// Only the options whose value is a file or folder refuse one: a password or a title can be any +/// string, `--page` included, which `text`'s own tests pin and this does not change. +#[test] +fn a_file_option_refuses_a_value_that_is_another_option() { + let dir = std::env::temp_dir().join(format!("pdfcraft-cli-values-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("temp dir"); + for (option, args) in [ + ("--out", vec!["extract", "missing.pdf", "--pages", "1", "--out", "--password"]), + ("--out", vec!["combine", "a.pdf", "b.pdf", "--out", "--nonsense"]), + ("--out", vec!["render", "missing.pdf", "--out", "--dpi", "96"]), + ("--out-dir", vec!["split", "missing.pdf", "--every", "2", "--out-dir", "--password"]), + ("--json", vec!["check", "missing.pdf", "--json", "--dpi"]), + ("--control", vec!["ui", "--control", "--out", "state"]), + // `edit` parses its own options, in the order given, and had the same gap. + ("--out", vec!["edit", "missing.pdf", "--out", "--full"]), + ] { + let out = Command::new(BIN).current_dir(&dir).args(&args).output().expect("run cli"); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!(!out.status.success(), "{args:?} unexpectedly succeeded"); + assert!(stderr.contains(&format!("{option} needs a value")), "{args:?}: {stderr}"); + assert!(stderr.contains("another option"), "{args:?}: {stderr}"); + } + assert_eq!(std::fs::read_dir(&dir).expect("read dir").count(), 0, "nothing was written"); + let _ = std::fs::remove_dir_all(&dir); +} + +/// And a file whose name really does start with `-` is still reachable, the way the shell reaches +/// one and `--root` already documented: `./--name`. The error is then about the input file, which +/// is as far as these lines get here. +#[test] +fn a_file_whose_name_starts_with_dashes_is_still_reachable() { + for args in [ + vec!["extract", "missing.pdf", "--pages", "1", "--out", "./--dash.pdf"], + vec!["edit", "missing.pdf", "--out", "./--dash.pdf"], + vec!["info", "./--dash.pdf"], + ] { + let out = Command::new(BIN).args(&args).output().expect("run cli"); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!(!out.status.success(), "{args:?}: no such file either way"); + assert!(stderr.contains("No such file"), "{args:?}: {stderr}"); + assert!(!stderr.contains("needs a value"), "{args:?}: {stderr}"); + } +} + +/// The token after an option that takes no value is not its value: `check-one --edit file.pdf` +/// reported "check-one: missing file", the file having been read as `--edit`'s operand. +#[test] +fn a_flag_that_takes_no_value_does_not_swallow_the_file() { + for args in [vec!["check-one", "--edit", "missing.pdf"], vec!["check-one", "missing.pdf", "--edit"]] { + let out = Command::new(BIN).args(&args).output().expect("run cli"); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!(!out.status.success(), "{args:?}"); + assert!(stderr.contains("missing.pdf") && stderr.contains("No such file"), "{args:?}: {stderr}"); + assert!(!stderr.contains("missing file"), "the file was found, and then not found: {stderr}"); + } +} + +/// photocraft#1108: an argument that is not UTF-8 panicked in `std::env::args` ("called +/// `Result::unwrap()` on an `Err` value"), while collecting the arguments — so it panicked even +/// for `--help`. Such a name cannot be used here, where paths are `str`, so it is a usage error. +#[cfg(unix)] +#[test] +fn an_argument_that_is_not_utf8_is_an_error_not_a_panic() { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + + for args in [vec![OsStr::new("info"), OsStr::from_bytes(b"/tmp/caf\xe9.pdf")], vec![OsStr::new("--help"), OsStr::from_bytes(b"caf\xe9")]] { + let out = Command::new(BIN).args(&args).output().expect("run cli"); + let stderr = String::from_utf8_lossy(&out.stderr); + assert_eq!(out.status.code(), Some(1), "an error, not a panic (101): {stderr}"); + assert!(stderr.contains("not valid UTF-8"), "{stderr}"); + assert!(!stderr.contains("panicked"), "{stderr}"); + } +} From 19f40e6554415bd897c5315e4863762625d3a557 Mon Sep 17 00:00:00 2001 From: Hamza Date: Thu, 22 Oct 2026 21:21:00 +0500 Subject: [PATCH 2/2] The two file tests ask which file, not how the OS says it is missing Review of #3849: both tests asserted `stderr.contains("No such file")`, which is the Unix wording. On Windows the same error reads "The system cannot find the file specified. (os error 2)", so `test (windows-latest)` failed on them. Neither test is about the wording. What they are about: - `a_flag_that_takes_no_value_does_not_swallow_the_file`: the error is about `missing.pdf`, and is not "check-one: missing file" -- the file reached the command instead of being eaten as `--edit`'s operand. - `a_file_whose_name_starts_with_dashes_is_still_reachable`: the error names the file the command could not open (`missing.pdf`, or `./--dash.pdf` for the `info` case), and is not a usage error about a value or an unknown option. Both now assert exactly that, plus the absence of our own usage phrases. Every string they match is one this CLI prints, so the OS has no say in it. --- apps/pdfcraft-cli/tests/options.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/apps/pdfcraft-cli/tests/options.rs b/apps/pdfcraft-cli/tests/options.rs index 14b3e617c..7955157e2 100644 --- a/apps/pdfcraft-cli/tests/options.rs +++ b/apps/pdfcraft-cli/tests/options.rs @@ -65,16 +65,19 @@ fn a_file_option_refuses_a_value_that_is_another_option() { /// is as far as these lines get here. #[test] fn a_file_whose_name_starts_with_dashes_is_still_reachable() { - for args in [ - vec!["extract", "missing.pdf", "--pages", "1", "--out", "./--dash.pdf"], - vec!["edit", "missing.pdf", "--out", "./--dash.pdf"], - vec!["info", "./--dash.pdf"], + // The error is about the file the command could not open, so the test asks which file it + // names -- not how the OS words "not found", which differs on Windows (#3849 review). + for (names, args) in [ + ("missing.pdf", vec!["extract", "missing.pdf", "--pages", "1", "--out", "./--dash.pdf"]), + ("missing.pdf", vec!["edit", "missing.pdf", "--out", "./--dash.pdf"]), + ("./--dash.pdf", vec!["info", "./--dash.pdf"]), ] { let out = Command::new(BIN).args(&args).output().expect("run cli"); let stderr = String::from_utf8_lossy(&out.stderr); assert!(!out.status.success(), "{args:?}: no such file either way"); - assert!(stderr.contains("No such file"), "{args:?}: {stderr}"); - assert!(!stderr.contains("needs a value"), "{args:?}: {stderr}"); + assert!(stderr.contains(names), "{args:?}: the error is about {names}: {stderr}"); + // Not a usage error: the `./--dash.pdf` reached the command as a file name. + assert!(!stderr.contains("needs a value") && !stderr.contains("unknown option"), "{args:?}: {stderr}"); } } @@ -86,8 +89,10 @@ fn a_flag_that_takes_no_value_does_not_swallow_the_file() { let out = Command::new(BIN).args(&args).output().expect("run cli"); let stderr = String::from_utf8_lossy(&out.stderr); assert!(!out.status.success(), "{args:?}"); - assert!(stderr.contains("missing.pdf") && stderr.contains("No such file"), "{args:?}: {stderr}"); + // Which file the error is about, not how the OS says it is missing (#3849 review). + assert!(stderr.contains("missing.pdf"), "{args:?}: the error is about the file: {stderr}"); assert!(!stderr.contains("missing file"), "the file was found, and then not found: {stderr}"); + assert!(!stderr.contains("needs a value"), "{args:?}: {stderr}"); } }