CLI: reject malformed --root options before dispatch (#1048)
* fix(cli): reject malformed root options before dispatch * test(cli): reject missing empty and repeated roots * docs: explain strict root arguments and validation * docs: log malformed root argument regression --------- Co-authored-by: Brandon Thomas <echelon@gmail.com>
This commit is contained in:
piero-83
GitHub
parent
ee1ee70463
commit
afb586e249
|
|
@ -144,6 +144,8 @@ Newest first: the date, what moved, and the new overall estimate.
|
|||
|
||||
- **2026-10-11 (M6):** Tab and Shift-Tab wrapping to the same field or another widget of that field refresh the newly opened editor from the accepted value before Save. Synthetic UI regressions cover both directions, same-frame Save, validation refusal and undo/redo. Estimates unchanged.
|
||||
|
||||
- **2026-10-11 (M13):** CLI run and MCP reject missing, empty, repeated and option-valued --root arguments before any tool/script execution or MCP initialization. A single-hyphen directory name remains valid; synthetic CLI regressions preserve a would-be overwritten file. Estimates unchanged.
|
||||
|
||||
- **2026-10-10 (M14, interface size):** Preferences ▸ Interface size scales the whole interface (50–250 %, setting `ui_scale`, control option `ui-scale`); 50 % brings an interface the system draws at 200 % back to 100 % (#456). Auto, the default, follows the desktop's text scaling: on Linux and the BSDs the desktop app reads GNOME's `text-scaling-factor` once at launch, so with Large Text (1.25) on a 1× display the interface no longer draws 13 px labels that show their pixels next to the system's text. The size is egui's zoom factor, so `pixels_per_point` grows with it and page rasters stay one texel per screen pixel; egui's own ⌘+/⌘− interface zoom is turned off so those keys stay page zoom. Still missing: KDE's font DPI and Windows' "Make text bigger" aren't read (both scale only text there), and egui's text is still greyscale-antialiased without the system's gamma, so small text looks rougher than GTK's at 100 %.
|
||||
- **2026-10-10 (M14, #496):** Shortcut displays share a UI formatter across toolbar search, registry menus, command palette, Help and tooltips. Modifier and key names use a dedicated catalog context in every bundled translation; missing entries keep the English key name rather than a command translation. German shows Strg/Umschalt/Entf, macOS retains its symbols, and runtime language changes refresh the display. Regression coverage checks localized layout widths, English/German switching, platform conventions and unchanged command execution. Milestone and effort estimates unchanged.
|
||||
|
||||
|
|
|
|||
|
|
@ -156,15 +156,29 @@ struct OptionSpec {
|
|||
takes_value: bool,
|
||||
}
|
||||
|
||||
/// Refuse a long option `command` doesn't take (a misspelling) before any file is read.
|
||||
/// 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") {
|
||||
// `flag` searches every token, including another option's value. Match that lookup so
|
||||
// a malformed confinement flag cannot be hidden there and fall back to no root.
|
||||
let mut root_seen = false;
|
||||
for (i, _) in args.iter().enumerate().filter(|(_, arg)| arg.as_str() == "--root") {
|
||||
if root_seen {
|
||||
return Err(format!("{command}: --root may only be supplied once").into());
|
||||
}
|
||||
root_seen = true;
|
||||
if args.get(i + 1).is_none_or(|value| value.is_empty() || value.starts_with("--")) {
|
||||
return Err(format!("{command}: --root needs a directory value").into());
|
||||
}
|
||||
}
|
||||
}
|
||||
let mut i = 0;
|
||||
while let Some(arg) = args.get(i) {
|
||||
if let Some(name) = arg.strip_prefix("--") {
|
||||
let Some(spec) = specs.iter().find(|spec| spec.name == arg) else {
|
||||
return Err(format!("{command}: unknown option --{name}").into());
|
||||
};
|
||||
// A missing or malformed value is left to the command, which reports it in its own
|
||||
// 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 };
|
||||
} else {
|
||||
|
|
|
|||
|
|
@ -0,0 +1,121 @@
|
|||
//! A malformed `--root` must never fall back to an unconfined automation session.
|
||||
|
||||
use std::path::PathBuf;
|
||||
use std::process::Command;
|
||||
use std::sync::atomic::{AtomicU64, Ordering};
|
||||
|
||||
const BIN: &str = env!("CARGO_BIN_EXE_pdfcraft-cli");
|
||||
const CANARY: &[u8] = b"KEEP EXISTING DESTINATION\n";
|
||||
|
||||
struct TestDir(PathBuf);
|
||||
|
||||
impl TestDir {
|
||||
fn new() -> Self {
|
||||
static NEXT: AtomicU64 = AtomicU64::new(0);
|
||||
for _ in 0..128 {
|
||||
let path = std::env::temp_dir().join(format!("pdfcraft-root-arguments-{}-{}", std::process::id(), NEXT.fetch_add(1, Ordering::Relaxed)));
|
||||
match std::fs::create_dir(&path) {
|
||||
Ok(()) => return Self(path),
|
||||
Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => continue,
|
||||
Err(error) => panic!("creating argument test directory: {error}"),
|
||||
}
|
||||
}
|
||||
panic!("no unused argument test directory after 128 attempts");
|
||||
}
|
||||
}
|
||||
|
||||
impl Drop for TestDir {
|
||||
fn drop(&mut self) {
|
||||
let _ = std::fs::remove_dir_all(&self.0);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn malformed_root_fails_before_a_script_can_overwrite_a_file() {
|
||||
let dir = TestDir::new();
|
||||
let destination = dir.0.join("existing.pdf");
|
||||
let script = dir.0.join("steps.json");
|
||||
let steps = serde_json::json!([
|
||||
{ "tool": "doc_create", "args": { "from": "blank", "width": 100, "height": 100 } },
|
||||
{ "tool": "doc_save", "args": { "doc": 1, "path": destination } },
|
||||
]);
|
||||
std::fs::write(&script, steps.to_string()).unwrap();
|
||||
std::fs::write(&destination, CANARY).unwrap();
|
||||
|
||||
for (flags, diagnostic) in [
|
||||
(vec!["--root"], "--root needs a directory value"),
|
||||
(vec!["--root", ""], "--root needs a directory value"),
|
||||
(vec!["--root", "--out", "escaped.png"], "--root needs a directory value"),
|
||||
(vec!["--out", "--root"], "--root needs a directory value"),
|
||||
(vec!["--root", ".", "--root"], "--root may only be supplied once"),
|
||||
(vec!["--root", ".", "--root", "."], "--root may only be supplied once"),
|
||||
(vec!["--root", ".", "--out", "--root"], "--root may only be supplied once"),
|
||||
] {
|
||||
// Both a valid script and a missing one: argument validation precedes script I/O.
|
||||
for script_name in ["steps.json", "missing.json"] {
|
||||
let out = Command::new(BIN).current_dir(&dir.0).args(["run", "--script", script_name]).args(&flags).output().unwrap();
|
||||
assert_eq!(out.status.code(), Some(1), "{script_name} {flags:?} unexpectedly succeeded");
|
||||
assert_eq!(String::from_utf8(out.stderr).unwrap().trim(), format!("pdfcraft-cli: run: {diagnostic}"));
|
||||
assert!(out.stdout.is_empty(), "a refused command must not execute or report tool results");
|
||||
assert_eq!(std::fs::read(&destination).unwrap(), CANARY);
|
||||
assert!(!dir.0.join("escaped.png").exists());
|
||||
}
|
||||
}
|
||||
assert_eq!(std::fs::read_dir(&dir.0).unwrap().count(), 2);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn malformed_root_fails_before_direct_tool_argument_validation() {
|
||||
let out = Command::new(BIN).args(["run", "doc_open", "not-key-value", "--root"]).output().unwrap();
|
||||
assert_eq!(out.status.code(), Some(1));
|
||||
assert_eq!(String::from_utf8(out.stderr).unwrap().trim(), "pdfcraft-cli: run: --root needs a directory value");
|
||||
assert!(out.stdout.is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_directory_name_starting_with_a_single_hyphen_remains_valid() {
|
||||
let dir = TestDir::new();
|
||||
std::fs::create_dir(dir.0.join("-documents")).unwrap();
|
||||
let out = Command::new(BIN)
|
||||
.current_dir(&dir.0)
|
||||
.args(["run", "doc_create", "from=blank", "width=100", "height=100", "--root", "-documents"])
|
||||
.output()
|
||||
.unwrap();
|
||||
assert!(out.status.success(), "{}", String::from_utf8_lossy(&out.stderr));
|
||||
assert!(serde_json::from_slice::<serde_json::Value>(&out.stdout).is_ok());
|
||||
}
|
||||
|
||||
#[cfg(feature = "mcp")]
|
||||
#[test]
|
||||
fn malformed_root_never_starts_or_initializes_an_mcp_server() {
|
||||
use std::io::Write as _;
|
||||
use std::process::Stdio;
|
||||
|
||||
let dir = TestDir::new();
|
||||
// Even if a directory matches the next option's name, it is not a root operand.
|
||||
std::fs::create_dir(dir.0.join("--compact")).unwrap();
|
||||
for (flags, diagnostic) in [
|
||||
(vec!["--root"], "--root needs a directory value"),
|
||||
(vec!["--root", ""], "--root needs a directory value"),
|
||||
(vec!["--root", "--compact"], "--root needs a directory value"),
|
||||
(vec!["--compact", "--root"], "--root needs a directory value"),
|
||||
(vec!["--root", ".", "--root"], "--root may only be supplied once"),
|
||||
(vec!["--root", ".", "--root", "./--compact"], "--root may only be supplied once"),
|
||||
] {
|
||||
let mut child = Command::new(BIN)
|
||||
.current_dir(&dir.0)
|
||||
.arg("mcp")
|
||||
.args(&flags)
|
||||
.stdin(Stdio::piped())
|
||||
.stdout(Stdio::piped())
|
||||
.stderr(Stdio::piped())
|
||||
.spawn()
|
||||
.unwrap();
|
||||
// Early rejection may close stdin before this write; that is the intended behavior.
|
||||
let _ = child.stdin.take().unwrap().write_all(b"{\"jsonrpc\":\"2.0\",\"id\":0,\"method\":\"initialize\",\"params\":{}}\n");
|
||||
let out = child.wait_with_output().unwrap();
|
||||
assert_eq!(out.status.code(), Some(1), "{flags:?} unexpectedly started the server");
|
||||
assert_eq!(String::from_utf8(out.stderr).unwrap().trim(), format!("pdfcraft-cli: mcp: {diagnostic}"));
|
||||
assert!(out.stdout.is_empty(), "a refused server must not answer initialize");
|
||||
}
|
||||
}
|
||||
|
|
@ -4,6 +4,10 @@ Start explicitly with `pdfcraft-cli mcp [--root DIR] [--compact]`. The server us
|
|||
stdin/stdout, opens no port, and stops on EOF. `--root` confines all reads and writes, including
|
||||
commands invoked through the common tools. Logs stay on stderr.
|
||||
|
||||
Supply `--root` at most once, followed by a nonempty directory path. Missing operands,
|
||||
repeated roots and another long option in place of the directory are refused before any
|
||||
script is read or server is started. Prefix a directory name beginning with `--` with `./`.
|
||||
|
||||
| Tool | Arguments |
|
||||
|---|---|
|
||||
| `command_list` | `doc?`, `filter?`, `enabled_only?`; returns `{commands: [...]}` with mapped tool schemas |
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@
|
|||
|
||||
> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-11 · **Change:** minor (M6: respect existing form-font encodings; retain source fonts on appearance fallback; estimates unchanged) · **Target:** Adobe Acrobat Pro (Acrobat DC, continuous track 26.002.21931, macOS)
|
||||
> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-11 · **Change:** minor (M6: keep accepted form values when Tab reopens the same field before Save; estimates unchanged) · **Target:** Adobe Acrobat Pro (Acrobat DC, continuous track 26.002.21931, macOS)
|
||||
> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-11 · **Change:** minor (M13: reject malformed CLI --root options before dispatch; estimates unchanged) · **Target:** Adobe Acrobat Pro (Acrobat DC, continuous track 26.002.21931, macOS)
|
||||
|
||||
Forward-looking: the milestones, the current focus and what comes next. The parity numbers behind
|
||||
it are in [target-app-parity.md](target-app-parity.md), the ranked work list in [gaps.md](gaps.md),
|
||||
|
|
@ -81,7 +82,7 @@ total matches the parity estimate.
|
|||
| M10 | OCR, create, export, print | 200–350 | 33% | 120–215 | Done: create from blank/text/PNG/JPEG/TIFF (multi-page)/GIF/BMP; export PNG/JPEG/TIFF and text; Print (Acrobat's sizing, n-up, booklet, poster, comments & forms, preview, the CUPS and Windows spoolers, print-ready PDF).<br>Done since: embedded/72/custom DPI choices for image imports; export all images; OCR (searchable image for pages, ranges and multiple files); single-sided cut-and-stack imposition with cut marks, through Print and doc_print; Create PDF from multiple files (PDFs, images and text, edited as pages in the grid). Missing: OCR languages beyond Latin, editable-text OCR output, Office export/import, web printing |
|
||||
| M11 | Optimize, preflight, PDF/A/X/UA, print production | 200–350 | 18% | 80–140 | Done: new `optimize` crate: Reduce File Size and the PDF Optimizer (images measured where drawn, bicubic downsampling, JPEG/ZIP recompression only when smaller, discard objects and user data, Flate clean-up, resource merging, object streams). Missing: fonts and transparency panels, space audit, preflight, PDF/A/X/UA |
|
||||
| M12 | Accessibility, compare, measure, search, XFA | 200–380 | 30% | 90–160 | Done: new `a11y` crate with the Accessibility Checker (all 32 rules, report, Fix/Skip/Explain, options dialog and results panel, agent tools).<br>Done since: 2D distance, perimeter and area measurements with persistent viewport calibration, snapping, live information and CSV export. Missing: autotag, Tags/Order/Content panels, Reading Order tool, alt-text workflow, compare, geospatial/3D measurement, search index, XFA |
|
||||
| M13 | Automation (MCP, Action Wizard, CLI) + AI providers | 60–120 | 45% | 25–45 | Done: MCP resources (document info, text, page images); headless tool table (123 tools incl. signing, optimizing, initial view, links, stamps, data exchange, comment review, forms authoring and scripts, redaction, sanitize, print, add content), opt-in MCP server over stdio, CLI `run`/`tools` (closed stdout pipes exit cleanly), UI control channel with drag. Missing: Action Wizard, AI providers |
|
||||
| M13 | Automation (MCP, Action Wizard, CLI) + AI providers | 60–120 | 45% | 25–45 | Done: MCP resources (document info, text, page images); headless tool table (123 tools incl. signing, optimizing, initial view, links, stamps, data exchange, comment review, forms authoring and scripts, redaction, sanitize, print, add content), opt-in MCP server over stdio, CLI `run`/`tools` (closed stdout pipes exit cleanly), UI control channel with drag. Missing: Action Wizard, AI providers<br>Done since: CLI run and MCP reject missing, empty, repeated and option-valued --root arguments before any tool/script execution or MCP initialization. A single-hyphen directory name remains valid; synthetic CLI regressions preserve a would-be overwritten file. |
|
||||
| M14 | 1.0 polish: performance, localization, installers | 120–250 | 15% | 80–150 | Done: PhotoCraft's translation system (`i18n/`: TSV catalogs, `tl!`, command-id and plural entries, system-language detection, strict catalog tests); every dialog, panel and notice goes through `tl!`. Catalogs of ≈ 2,000–2,140 entries for Japanese, Simplified and Traditional Chinese, Russian, Bulgarian, German, Spanish, French, Telugu, Hungarian, Ukrainian, Italian, Brazilian Portuguese and Arabic (logical-order catalog put into display order; layout not yet mirrored); Czech covers the menus. Signed and notarized macOS builds, signed Windows installers (x64, x86, ARM64), AppImage, deb, rpm, Flatpak and FreeBSD packages. #307 (bounded large-PDF memory): shared resource indexes and parser metadata, lazy inspection, bounded caches; a 9,156-page file inspects in ≈ 1.8 s at ≈ 500 MiB instead of exceeding 10 GiB. Shortcut display uses localized modifier/key names across menus, toolbar, palette, Help and tooltips (#496). Missing: a right-to-left layout, a Chinese UI face in releases, Hindi/Indonesian/Korean/Vietnamese, file-backed input and global memory budgets, performance budgets, keyboard-only operation |
|
||||
| | **Total** | **2,085–3,840** | **≈ 49% ready** | **≈ 770–1,400** | Area view of the same work: [target-app-parity.md](target-app-parity.md#by-feature-area) |
|
||||
|
||||
|
|
@ -127,6 +128,7 @@ M2 (renderer), M7 (content editing), M10 (OCR and Office), M11 (Preflight and st
|
|||
|---|---|---|
|
||||
| 2026-10-11 | minor | M6: respect existing form-font encodings; retain source fonts on appearance fallback. Estimates unchanged. |
|
||||
| 2026-10-11 | minor | M6: keep accepted form values when Tab reopens the same field before Save. Estimates unchanged. |
|
||||
| 2026-10-11 | minor | M13: reject malformed CLI --root options before dispatch. Estimates unchanged. |
|
||||
| 2026-10-10 | minor | M14: localized shortcut key names (#496), including runtime switching and native layout checks. Estimates unchanged. |
|
||||
| 2026-10-10 | minor | Re-judged convert and share with the cross-app gate rule (only a workflow that can't be completed at all blocks): partial, not blocking → stage alpha |
|
||||
| 2026-10-10 | minor | Added the alpha gate (six core workflows; convert and share fails → pre-alpha, 40–70 h to alpha) and rank 0 in the upcoming milestones |
|
||||
|
|
|
|||
Loading…
Reference in New Issue