collective
Browse / github.com/storytold/pdfcraft / patch
CLI: reject missing option operands before file I/O
CLI: reject missing option operands before file I/O ## What this changes Reject a value-taking CLI option when its operand is missing. Previously, `render input.pdf --out output.png --dpi` could silently use the default DPI and write an output file. The common validator now reports the missing value before file I/O. Existing path checks, empty passwords, option-shaped password values, and the `--root` diagnostic remain unchanged. ## How you verified it - Regression tests for missing operands, including a synthetic PDF that previously produced output; each affected non-path option was checked before the fix. - `cargo test --locked -p pdfcraf
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- DAnaeto (imported from pull request #4903, open)
- Base
1dd2737b47a9c1c9562f0af563053d1bf510e4bf- Size
- +104 −15 in 4 files
- Review
- pending
- Shared
- 2026-10-28
- Id
6f5e72a103f4a5958f906eae
How it went
No other operator has reported using it yet.
Diff
raw · apps/pdfcraft-cli/src/options.rs apps/pdfcraft-cli/tests/it/options.rs apps/pdfcraft-cli/tests/it/render.rs apps/pdfcraft-cli/tests/it/text.rs
@@ -54,12 +54,11 @@ pub(crate) fn validate_options(command: &str, args: &[String], specs: &[OptionSp let Some(spec) = specs.iter().find(|spec| spec.name == arg) else { return Err(format!("{command}: unknown option --{name}").into()); };- // 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 value.is_none() {+ return Err(needs_value(command, spec.name, value).into());+ } if !can_be_path_value(spec.name, value) { return Err(needs_value(command, spec.name, value).into()); }@@ -113,3 +113,63 @@ fn an_argument_that_is_not_utf8_is_an_error_not_a_panic() { assert!(!stderr.contains("panicked"), "{stderr}"); } }++fn assert_missing_value(args: &[&str], command: &str, option: &str) {+ use std::sync::atomic::{AtomicUsize, Ordering};+ static NEXT: AtomicUsize = AtomicUsize::new(0);+ let dir = std::env::temp_dir().join(format!(+ "pdfcraft-cli-missing-{}-{}-{}",+ std::process::id(),+ std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_nanos(),+ NEXT.fetch_add(1, Ordering::Relaxed)+ ));+ std::fs::create_dir(&dir).unwrap();+ let out = Command::new(BIN).current_dir(&dir).args(args).output().unwrap();+ let stderr = String::from_utf8_lossy(&out.stderr);+ assert_eq!(out.status.code(), Some(1), "{args:?}: {stderr}");+ assert!(stderr.contains(&format!("{command}: {option} needs a value")), "{args:?}: {stderr}");+ assert!(!stderr.contains("missing.pdf"), "{args:?}: {stderr}");+ assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 0, "{args:?}: wrote files");+ std::fs::remove_dir(&dir).unwrap();+}++macro_rules! missing_value_test {+ ($name:ident, $command:literal, $option:literal, [$($arg:literal),+ $(,)?]) => {+ #[test]+ fn $name() {+ assert_missing_value(&[$($arg),+], $command, $option);+ }+ };+}++missing_value_test!(missing_render_dpi, "render", "--dpi", ["render", "missing.pdf", "--out", "unused.png", "--dpi"]);+missing_value_test!(missing_render_page, "render", "--page", ["render", "missing.pdf", "--out", "unused.png", "--page"]);+missing_value_test!(missing_info_password, "info", "--password", ["info", "missing.pdf", "--password"]);+missing_value_test!(missing_check_timeout, "check", "--timeout", ["check", "missing.pdf", "--timeout"]);+missing_value_test!(missing_split_out_dir, "split", "--out-dir", ["split", "missing.pdf", "--every", "1", "--out-dir"]);+missing_value_test!(missing_split_every, "split", "--every", ["split", "missing.pdf", "--every"]);+missing_value_test!(missing_split_before, "split", "--before", ["split", "missing.pdf", "--before"]);+missing_value_test!(missing_extract_pages, "extract", "--pages", ["extract", "missing.pdf", "--pages"]);+missing_value_test!(missing_repeated_render_dpi, "render", "--dpi", ["render", "missing.pdf", "--out", "unused.png", "--dpi", "36", "--dpi"]);+missing_value_test!(missing_audit_password, "audit", "--password", ["audit", "missing.pdf", "--password"]);+missing_value_test!(missing_text_page, "text", "--page", ["text", "missing.pdf", "--page"]);+missing_value_test!(missing_text_password, "text", "--password", ["text", "missing.pdf", "--password"]);+missing_value_test!(missing_combine_out, "combine", "--out", ["combine", "missing.pdf", "--out"]);+missing_value_test!(missing_extract_out, "extract", "--out", ["extract", "missing.pdf", "--out"]);+missing_value_test!(missing_extract_password, "extract", "--password", ["extract", "missing.pdf", "--password"]);+missing_value_test!(missing_split_password, "split", "--password", ["split", "missing.pdf", "--every", "1", "--password"]);+missing_value_test!(missing_render_out, "render", "--out", ["render", "missing.pdf", "--out"]);+missing_value_test!(missing_render_password, "render", "--password", ["render", "missing.pdf", "--out", "unused.png", "--password"]);+missing_value_test!(missing_check_one_dpi, "check-one", "--dpi", ["check-one", "missing.pdf", "--dpi"]);+missing_value_test!(missing_check_dpi, "check", "--dpi", ["check", "missing.pdf", "--dpi"]);+missing_value_test!(missing_check_json, "check", "--json", ["check", "missing.pdf", "--json"]);+missing_value_test!(missing_run_script, "run", "--script", ["run", "--script"]);+missing_value_test!(missing_run_out, "run", "--out", ["run", "--out"]);+missing_value_test!(missing_ui_control, "ui", "--control", ["ui", "--control"]);+missing_value_test!(missing_ui_out, "ui", "--out", ["ui", "--out"]);+missing_value_test!(missing_batch_out_dir, "batch", "--out-dir", ["batch", "--out-dir"]);+missing_value_test!(missing_batch_steps, "batch", "--steps", ["batch", "--steps"]);+missing_value_test!(missing_batch_ppi, "batch", "--ppi", ["batch", "--ppi"]);+missing_value_test!(missing_batch_quality, "batch", "--quality", ["batch", "--quality"]);+missing_value_test!(missing_batch_dpi, "batch", "--dpi", ["batch", "--dpi"]);+missing_value_test!(missing_batch_language, "batch", "--language", ["batch", "--language"]);@@ -112,3 +112,31 @@ fn a_page_or_dpi_the_renderer_cannot_use_is_refused_before_reading_the_file() { assert!(!out.exists(), "{option} {value}: no output may be written"); } }++#[test]+fn missing_render_values_do_not_write_output() {+ let pdf = fixture("missing-values");+ let original = std::fs::read(&pdf).unwrap();+ for option in ["--dpi", "--page"] {+ let out = tmp("missing-values", &format!("{}.png", &option[2..]));+ let _ = std::fs::remove_file(&out);+ let result = Command::new(BIN).args(["render", pdf.to_str().unwrap(), "--out", out.to_str().unwrap(), option]).output().unwrap();+ assert_eq!(result.status.code(), Some(1), "{option}: {}", String::from_utf8_lossy(&result.stderr));+ assert!(String::from_utf8_lossy(&result.stderr).contains(&format!("render: {option} needs a value")));+ assert!(!out.exists());+ assert_eq!(std::fs::read(&pdf).unwrap(), original);+ }+ let out = tmp("missing-values", "default.png");+ let _ = std::fs::remove_file(&out);+ let result = Command::new(BIN).args(["render", pdf.to_str().unwrap(), "--out", out.to_str().unwrap()]).output().unwrap();+ assert!(result.status.success(), "{}", String::from_utf8_lossy(&result.stderr));+ assert!(out.exists());+ assert_eq!(std::fs::read(&pdf).unwrap(), original);+}++#[test]+fn info_accepts_an_empty_password() {+ let pdf = fixture("empty-password");+ let result = Command::new(BIN).args(["info", pdf.to_str().unwrap(), "--password", ""]).output().unwrap();+ assert!(result.status.success(), "{}", String::from_utf8_lossy(&result.stderr));+}@@ -167,23 +167,25 @@ mod page_argument_tests { let bytes = fixture(3); std::fs::write(&path, &bytes).unwrap(); let missing = dir.0.join("missing.pdf");+ let bad_page = "pdfcraft-cli: bad --page: expected a positive page number";+ let missing_page = "pdfcraft-cli: text: --page needs a value"; let invalid = [- vec!["--page", "0"],- vec!["--page", ""],- vec!["--page", "bad"],- vec!["--page", "-1"],- vec!["--page", "1.5"],- vec!["--page", "18446744073709551616"],- vec!["--page"],- vec!["--page", "--password", "unused"],- vec!["--page", "2", "--page"],+ (vec!["--page", "0"], bad_page),+ (vec!["--page", ""], bad_page),+ (vec!["--page", "bad"], bad_page),+ (vec!["--page", "-1"], bad_page),+ (vec!["--page", "1.5"], bad_page),+ (vec!["--page", "18446744073709551616"], bad_page),+ (vec!["--page"], missing_page),+ (vec!["--page", "--password", "unused"], bad_page),+ (vec!["--page", "2", "--page"], missing_page), ]; for input in [&path, &missing] {- for args in &invalid {+ for (args, expected) in &invalid { let out = Command::new(BIN).arg("text").arg(input).args(args).output().unwrap(); assert_eq!(out.status.code(), Some(1), "{args:?}: {out:?}"); assert!(out.stdout.is_empty(), "{args:?}: no text may be printed");- assert_eq!(String::from_utf8(out.stderr).unwrap().trim(), "pdfcraft-cli: bad --page: expected a positive page number");+ assert_eq!(String::from_utf8(out.stderr).unwrap().trim(), *expected); assert_eq!(std::fs::read(&path).unwrap(), bytes); assert!(!missing.exists()); }