diff --git a/apps/pdfcraft-cli/src/main.rs b/apps/pdfcraft-cli/src/main.rs index d0f23242d..cfbf43270 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}"))?; @@ -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>, 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 { 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(); diff --git a/apps/pdfcraft-cli/tests/it/options.rs b/apps/pdfcraft-cli/tests/it/options.rs index 9576d59a8..7955157e2 100644 --- a/apps/pdfcraft-cli/tests/it/options.rs +++ b/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}"); + } +}