Merge PR #3849: cli: the argument bugs photocraft#7071 lists are live here too

Conflicts: apps/pdfcraft-cli/src/main.rs (render, check_one, check): kept main's bodies, positional() takes the option spec.

apps/pdfcraft-cli/src/main.rs

@@ -84,7 +84,24 @@ help and feedback: https://discord.gg/artcraft"
 );
 
 fn main() -> ExitCode {
-    let args: Vec<String> = 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<String> = 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;
+            }
+        }
+    }
     // `<command> --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}"))?;
@@ -208,6 +225,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") {
@@ -230,9 +269,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;
         }
@@ -278,19 +327,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
 }
@@ -301,7 +355,7 @@ fn read(path: &str) -> Result<Arc<Vec<u8>>, 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");
     // The renderer takes only the user password of R2–R4 files; `Session::open` takes either.
@@ -354,7 +408,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();
@@ -482,7 +536,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),
@@ -569,7 +629,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());
     }
@@ -580,7 +640,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();
@@ -592,7 +652,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) => {
             // A count the splitter cannot hold is refused here, before the file is opened,
@@ -637,7 +697,7 @@ fn parse_dpi(value: &str) -> Result<f32, String> {
 
 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().ok().filter(|page| *page > 0).ok_or("bad --page: expected a positive page number")?;
     let page_index = page - 1;
@@ -696,7 +756,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 = parse_dpi(flag(args, "--dpi").unwrap_or("36"))?;
     let start = Instant::now();
     let bytes = read(path)?;
@@ -802,7 +862,7 @@ fn check(args: &[String]) -> Result<(), CliError> {
     );
     let dpi = flag(args, "--dpi").unwrap_or("36").to_string();
     parse_dpi(&dpi)?;
-    let files = collect(&positional(args));
+    let files = collect(&positional(args, CHECK_OPTIONS));
     if files.is_empty() {
         return Err("check: no PDF files found".into());
     }
@@ -985,9 +1045,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);
@@ -1053,7 +1113,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();

apps/pdfcraft-cli/tests/it/options.rs

@@ -27,3 +27,89 @@ 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() {
+    // 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(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}");
+    }
+}
+
+/// 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:?}");
+        // 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}");
+    }
+}
+
+/// 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}");
+    }
+}