From 7461c297918fab20139a429069ec4d836e216af0 Mon Sep 17 00:00:00 2001 From: Isaac Levine <80065258+isaac-levine@users.noreply.github.com> Date: Thu, 15 Oct 2026 03:11:12 -0400 Subject: [PATCH] MCP: confine file access with --automation-read-root and --automation-write-root (#1448) * Automation: check the read and write roots on every path a tool reads or writes * CLI: add --automation-read-root and --automation-write-root to mcp * Docs: document --automation-read-root and --automation-write-root for mcp * #1448: count main's doc_batch helpers as resolved by their caller Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Brandon Thomas Co-authored-by: Claude Opus 5.5 --- apps/pdfcraft-cli/src/automation_args.rs | 90 +++++ apps/pdfcraft-cli/src/main.rs | 43 ++- apps/pdfcraft-cli/tests/automation_roots.rs | 123 +++++++ crates/automation/README.md | 1 + crates/automation/src/a11y.rs | 2 +- crates/automation/src/file_access.rs | 381 ++++++++++++++++++++ crates/automation/src/lib.rs | 58 ++- crates/automation/tests/automation_roots.rs | 376 +++++++++++++++++++ docs/mcp.md | 64 +++- 9 files changed, 1130 insertions(+), 8 deletions(-) create mode 100644 apps/pdfcraft-cli/src/automation_args.rs create mode 100644 apps/pdfcraft-cli/tests/automation_roots.rs create mode 100644 crates/automation/src/file_access.rs create mode 100644 crates/automation/tests/automation_roots.rs diff --git a/apps/pdfcraft-cli/src/automation_args.rs b/apps/pdfcraft-cli/src/automation_args.rs new file mode 100644 index 000000000..868cdf66a --- /dev/null +++ b/apps/pdfcraft-cli/src/automation_args.rs @@ -0,0 +1,90 @@ +//! Parse `--automation-read-root` / `--automation-write-root` before the server starts. A malformed +//! flag must not silently remove a root and leave the server unconfined. + +use std::path::PathBuf; + +pub struct AutomationArgs { + pub read_root: Option, + pub write_root: Option, + read_given: bool, + write_given: bool, +} + +impl AutomationArgs { + pub fn new(read_root: Option, write_root: Option) -> Self { + Self { read_root, write_root, read_given: false, write_given: false } + } + + /// Returns whether this argument is a root flag. A second occurrence of a flag is an error, + /// including when the two spellings (`--flag DIR`, `--flag=DIR`) are mixed. + pub fn consume(&mut self, arg: &str, args: &mut impl Iterator) -> Result { + let (flag, inline) = arg.split_once('=').map_or((arg, None), |(flag, value)| (flag, Some(value))); + let (root, given) = match flag { + "--automation-read-root" => (&mut self.read_root, &mut self.read_given), + "--automation-write-root" => (&mut self.write_root, &mut self.write_given), + _ => return Ok(false), + }; + if *given { + return Err(format!("{flag} given twice")); + } + let value = match inline { + Some(value) => value.to_owned(), + None => { + let value = args.next().ok_or_else(|| format!("{flag} needs a folder"))?; + if value.starts_with('-') { + return Err(format!("{flag} needs a folder; use {flag}= for a folder starting with '-'")); + } + value + } + }; + if value.is_empty() { + return Err(format!("{flag} needs a non-empty folder")); + } + *root = Some(value.into()); + *given = true; + Ok(true) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn roots_are_taken_once_and_other_arguments_are_left() { + let mut roots = AutomationArgs::new(None, None); + let mut args = ["a folder".to_owned(), "report.pdf".to_owned()].into_iter(); + assert!(roots.consume("--automation-read-root", &mut args).unwrap()); + assert!(roots.consume("--automation-write-root=out", &mut args).unwrap()); + assert!(!roots.consume("--compact", &mut args).unwrap()); + assert_eq!(roots.read_root, Some("a folder".into())); + assert_eq!(roots.write_root, Some("out".into())); + assert_eq!(args.next().as_deref(), Some("report.pdf")); + assert!(roots.consume("--automation-read-root=other", &mut args).is_err()); + assert_eq!(roots.read_root, Some("a folder".into())); + } + + #[test] + fn invalid_operands_leave_roots_unchanged() { + for flag in ["--automation-read-root", "--automation-write-root"] { + for operand in [None, Some(""), Some("--version"), Some("--root"), Some("-folder")] { + let mut roots = AutomationArgs::new(Some("read".into()), Some("write".into())); + let mut args = operand.map(str::to_owned).into_iter(); + assert!(roots.consume(flag, &mut args).is_err(), "{flag} {operand:?}"); + assert_eq!(roots.read_root, Some("read".into())); + assert_eq!(roots.write_root, Some("write".into())); + } + let mut roots = AutomationArgs::new(None, None); + assert!(roots.consume(&format!("{flag}="), &mut std::iter::empty()).is_err()); + } + } + + #[test] + fn explicit_dash_folder_and_relative_dash_folder_are_valid() { + let mut roots = AutomationArgs::new(None, None); + assert!(roots.consume("--automation-read-root=-folder", &mut std::iter::empty()).unwrap()); + assert!(roots.consume("--automation-write-root", &mut ["./-folder".to_owned()].into_iter()).unwrap()); + assert_eq!(roots.read_root, Some("-folder".into())); + assert_eq!(roots.write_root, Some("./-folder".into())); + } +} diff --git a/apps/pdfcraft-cli/src/main.rs b/apps/pdfcraft-cli/src/main.rs index e749eb54d..0e24800d3 100644 --- a/apps/pdfcraft-cli/src/main.rs +++ b/apps/pdfcraft-cli/src/main.rs @@ -17,6 +17,8 @@ #![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] +#[cfg(feature = "mcp")] +mod automation_args; mod batch; use std::io::Write; @@ -33,8 +35,14 @@ use pdfcraft_render::{PageRenderer, RenderConfig, RenderRequest, RequestKind, in macro_rules! mcp_usage { () => { "\ -pdfcraft-cli mcp [--root DIR] [--compact] MCP server on stdin/stdout (opt-in) +pdfcraft-cli mcp [--root DIR] [--compact] [--automation-read-root DIR] [--automation-write-root DIR] + MCP server on stdin/stdout (opt-in) --compact lists a core set of tools plus tool_search and tool_call + --automation-read-root / --automation-write-root confine the files the + agent's tools read (open, combine, import, fonts, certificates…) and + write (save, export, extract, split, reports…) to DIR, links followed; + a root left out grants none of its access. --root, when also given, + applies as well " }; } @@ -901,11 +909,42 @@ fn run(args: &[String]) -> Result<(), CliError> { print_output(&auto, content, flag(args, "--out")) } +/// Take `--automation-read-root` / `--automation-write-root` (each `--flag DIR` or `--flag=DIR`, +/// once) out of `args`, and open the roots, which must be existing folders. Returns the arguments +/// that are left. A malformed root flag is an error, never a root silently left out. +#[cfg(feature = "mcp")] +fn automation_roots(command: &str, args: &[String]) -> Result<(Option>, Vec), String> { + let mut found = automation_args::AutomationArgs::new(None, None); + let mut left = Vec::new(); + let mut it = args.iter().cloned(); + while let Some(arg) = it.next() { + if !found.consume(&arg, &mut it).map_err(|e| format!("{command}: {e}"))? { + left.push(arg); + } + } + let roots = pdfcraft_automation::file_access::AutomationRoots::new(found.read_root.as_deref(), found.write_root.as_deref()) + .map_err(|e| format!("{command}: {e}"))?; + Ok((roots, left)) +} + #[cfg(feature = "mcp")] fn mcp(args: &[String]) -> Result<(), CliError> { + let (roots, args) = automation_roots("mcp", args)?; + let args = args.as_slice(); validate_options("mcp", args, MCP_OPTIONS)?; let compact = args.iter().any(|a| a == "--compact"); - let mut server = pdfcraft_automation::mcp::McpServer::new(automation(args)?).with_compact(compact); + let mut auto = automation(args)?; + if let Some(roots) = roots { + let show = |root: Option<&Path>| root.map_or_else(|| "none".to_string(), |r| r.display().to_string()); + let _ = writeln!( + std::io::stderr().lock(), + "pdfcraft-cli: files confined: read root {}, write root {}", + show(roots.read_root()), + show(roots.write_root()) + ); + auto = auto.with_automation_roots(Some(roots)); + } + let mut server = pdfcraft_automation::mcp::McpServer::new(auto).with_compact(compact); let _ = writeln!( std::io::stderr().lock(), "pdfcraft-cli: MCP server on stdio (protocol {}{}); close stdin to stop", diff --git a/apps/pdfcraft-cli/tests/automation_roots.rs b/apps/pdfcraft-cli/tests/automation_roots.rs new file mode 100644 index 000000000..162d79baf --- /dev/null +++ b/apps/pdfcraft-cli/tests/automation_roots.rs @@ -0,0 +1,123 @@ +//! `pdfcraft-cli mcp --automation-read-root DIR --automation-write-root DIR`, on the real binary: +//! both spellings of the flags confine the server, and a malformed flag stops it instead of +//! leaving it unconfined. +#![cfg(feature = "mcp")] + +use std::io::Write as _; +use std::path::{Path, PathBuf}; +use std::process::{Command, Output, Stdio}; + +use serde_json::{Value, json}; + +const BIN: &str = env!("CARGO_BIN_EXE_pdfcraft-cli"); + +/// `in/a.pdf` and `outside/secret.pdf`, blank one-page PDFs made by the binary itself. +fn folders(tag: &str) -> PathBuf { + let base = std::env::temp_dir().join(format!("pdfcraft-cli-roots-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + for d in ["in", "out", "outside"] { + std::fs::create_dir_all(base.join(d)).unwrap(); + } + for f in ["in/a.pdf", "outside/secret.pdf"] { + let script = json!([ + {"tool": "doc_create", "args": {"from": "blank", "pages": 1, "width": 100, "height": 100}}, + {"tool": "doc_save", "args": {"doc": 1, "path": f}}, + ]); + std::fs::write(base.join("steps.json"), script.to_string()).unwrap(); + let out = Command::new(BIN).args(["run", "--script"]).arg(base.join("steps.json")).arg("--root").arg(&base).output().unwrap(); + assert!(out.status.success(), "{}", String::from_utf8_lossy(&out.stderr)); + } + base +} + +/// Run `mcp `, send `calls` as `tools/call` requests and return the replies. +fn session(flags: &[String], calls: &[(&str, Value)]) -> (Vec, String) { + let mut child = Command::new(BIN).arg("mcp").args(flags).stdin(Stdio::piped()).stdout(Stdio::piped()).stderr(Stdio::piped()).spawn().unwrap(); + let mut input = String::from("{\"jsonrpc\":\"2.0\",\"id\":0,\"method\":\"initialize\",\"params\":{}}\n"); + for (i, (name, args)) in calls.iter().enumerate() { + let r = json!({"jsonrpc": "2.0", "id": i + 1, "method": "tools/call", "params": {"name": name, "arguments": args}}); + input.push_str(&format!("{r}\n")); + } + child.stdin.take().unwrap().write_all(input.as_bytes()).unwrap(); + let out = child.wait_with_output().unwrap(); + assert!(out.status.success()); + let replies = String::from_utf8(out.stdout).unwrap().lines().skip(1).map(|l| serde_json::from_str(l).unwrap()).collect(); + (replies, String::from_utf8(out.stderr).unwrap()) +} + +fn s(p: &Path) -> String { + p.to_string_lossy().into_owned() +} + +fn flags(spelling: &str, base: &Path) -> Vec { + let (r, w) = (s(&base.join("in")), s(&base.join("out"))); + if spelling == "equals" { + vec![format!("--automation-read-root={r}"), format!("--automation-write-root={w}")] + } else { + vec!["--automation-read-root".into(), r, "--automation-write-root".into(), w] + } +} + +fn text(reply: &Value) -> String { + reply["result"]["content"][0]["text"].as_str().unwrap_or_default().to_owned() +} + +#[test] +fn both_spellings_confine_the_server() { + for spelling in ["equals", "separate"] { + let base = folders(spelling); + let calls = [ + ("doc_open", json!({"path": s(&base.join("in/a.pdf"))})), + ("doc_open", json!({"path": s(&base.join("outside/secret.pdf"))})), + ("doc_save", json!({"doc": 1, "path": s(&base.join("out/copy.pdf"))})), + ("doc_save", json!({"doc": 1, "path": s(&base.join("outside/copy.pdf"))})), + ]; + let (replies, stderr) = session(&flags(spelling, &base), &calls); + assert_eq!(replies[0]["result"]["isError"], false, "{spelling}: {replies:?}"); + assert!(text(&replies[1]).contains("automation path rejected: outside the read root"), "{spelling}: {replies:?}"); + assert_eq!(replies[2]["result"]["isError"], false, "{spelling}: {replies:?}"); + assert!(text(&replies[3]).contains("automation path rejected: outside the write root"), "{spelling}: {replies:?}"); + assert!(base.join("out/copy.pdf").is_file() && !base.join("outside/copy.pdf").exists()); + assert!(stderr.contains("files confined: read root") && stderr.contains("write root"), "{stderr}"); + } +} + +#[test] +fn a_root_left_out_grants_none_of_its_access() { + let base = folders("one"); + let read_only = vec!["--automation-read-root".to_string(), s(&base.join("in"))]; + let calls = [("doc_open", json!({"path": s(&base.join("in/a.pdf"))})), ("doc_save", json!({"doc": 1, "path": s(&base.join("out/x.pdf"))}))]; + let (replies, _) = session(&read_only, &calls); + assert_eq!(replies[0]["result"]["isError"], false); + assert!(text(&replies[1]).contains("write authority is absent"), "{replies:?}"); + assert!(!base.join("out/x.pdf").exists()); +} + +#[test] +fn no_flags_means_no_confinement() { + let base = folders("none"); + let (replies, stderr) = session(&[], &[("doc_open", json!({"path": s(&base.join("outside/secret.pdf"))}))]); + assert_eq!(replies[0]["result"]["isError"], false); + assert!(!stderr.contains("confined"), "{stderr}"); +} + +fn fails(args: &[&str]) -> String { + let out: Output = Command::new(BIN).arg("mcp").args(args).stdin(Stdio::null()).output().unwrap(); + assert_eq!(out.status.code(), Some(1), "{args:?} should fail"); + assert!(out.stdout.is_empty(), "{args:?} must not start a server"); + String::from_utf8(out.stderr).unwrap() +} + +#[test] +fn malformed_flags_are_errors_not_a_missing_root() { + let dir = s(&std::env::temp_dir()); + assert!(fails(&["--automation-read-root"]).contains("--automation-read-root needs a folder")); + assert!(fails(&["--automation-write-root", "--compact"]).contains("--automation-write-root needs a folder; use --automation-write-root=")); + assert!(fails(&["--automation-read-root="]).contains("--automation-read-root needs a non-empty folder")); + assert!(fails(&["--automation-read-root", &dir, "--automation-read-root", &dir]).contains("--automation-read-root given twice")); + assert!(fails(&["--automation-write-root", &dir, &format!("--automation-write-root={dir}")]).contains("--automation-write-root given twice")); + let missing = fails(&["--automation-read-root", "/definitely/not/a/folder"]); + assert!(missing.contains("mcp: cannot open the automation read root"), "{missing}"); + let file = std::env::current_exe().unwrap(); + assert!(fails(&["--automation-write-root", &s(&file)]).contains("cannot open the automation write root")); +} diff --git a/crates/automation/README.md b/crates/automation/README.md index 32dec0bf2..cb1d85c1f 100644 --- a/crates/automation/README.md +++ b/crates/automation/README.md @@ -27,6 +27,7 @@ mcp::McpServer::new(a).serve(stdin, stdout)? // newline-delimited JS - For proportional image signature resizing, use the embedded image layer's width/height ratio rather than a previously stretched annotation rectangle. The GUI preserves this ratio at corners with the opposite corner anchored; edge midpoint handles change only their own axis. `comment_edit` accepts the resulting rectangle. - `doc_close` refuses to drop unsaved changes unless `discard_changes: true`. `doc_save` writes atomically, incrementally in place, and in full for a new path. Atomic output writes call `sync_all` on the staging file before replacing the destination. A write or sync error leaves the destination intact, removes staging and leaves the document dirty for retry. A filesystem that does not support syncing returns an error; there is no unsynced fallback. This flushes file contents, but does not guarantee persistence of the parent-directory rename across power loss. - With a root set, every read and write path must resolve inside it (symlinks and `..` included). Relative paths resolve inside the root, and `..` is resolved by name before the check. Every path outside the root gets the same refusal (` is outside the allowed directory `), whether or not it exists, so a confined agent can't probe the rest of the disk; on Windows another network share or device path (`\\host\share`, `\\?\UNC\…`, `\\.\…`) is refused without being contacted. "Not found" and other filesystem errors are reported only for paths inside the root. Tools that write several files into a folder name them after the document or input file, with separators, colons and control characters replaced by `_`, so a name can't lead them out of the folder. The root itself must be a folder. +- `Automation::with_automation_roots` holds a read folder and a write folder (`pdfcraft-cli mcp --automation-read-root DIR --automation-write-root DIR`, see [docs/mcp.md](../../docs/mcp.md)). Reads must lie inside the read root and writes inside the write root, links followed. A root left out grants none of its access. Refusals read `automation path rejected: outside the read root : `. It applies on top of `with_root`. ## Text search and cache limits diff --git a/crates/automation/src/a11y.rs b/crates/automation/src/a11y.rs index 60c1d2b3f..85df2d95d 100644 --- a/crates/automation/src/a11y.rs +++ b/crates/automation/src/a11y.rs @@ -568,7 +568,7 @@ impl Automation { // other tools (the write check passes missing files, which stay per-file errors below). let paths = a.strs("paths")?; for p in &paths { - self.resolve(p, true)?; + self.resolve_for(p, crate::file_access::Access::Read, true)?; } std::fs::create_dir_all(&folder).map_err(|e| failed(e.to_string()))?; let ocr = pdfcraft_engine::ocr::engine().map_err(failed)?; diff --git a/crates/automation/src/file_access.rs b/crates/automation/src/file_access.rs new file mode 100644 index 000000000..55b10662f --- /dev/null +++ b/crates/automation/src/file_access.rs @@ -0,0 +1,381 @@ +//! The folders automation may read and write. +//! +//! `pdfcraft-cli mcp --automation-read-root --automation-write-root ` confines the +//! files an agent's tools reach, with the names and wording VectorCraft uses. [`AutomationRoots`] +//! holds the two folders. Every path a tool reads is checked with [`Access::Read`] and every path +//! it writes with [`Access::Write`], in `Automation::resolve`, which all of them go through: +//! +//! - The path is made absolute against the working directory, as the file system reads it. Its +//! links are followed: for a file that doesn't exist yet, the links of its nearest existing +//! folder, the rest of the path being plain names (`..` below a missing folder is refused, and so +//! is a link that leads nowhere). +//! - The result must lie inside the root, compared by whole components (`/work2` isn't inside +//! `/work`), without regard to case on Windows. +//! - Read and write access are separate: a root given alone grants only its own kind of access. +//! +//! Without roots nothing is checked. A path is checked, then opened: a process that can change the +//! folders in between (swapping in a link) can still win that race. The roots keep an agent to the +//! files it was given; they don't guard against other programs on the computer. + +use std::ffi::OsString; +use std::path::{Component, Path, PathBuf}; +use std::sync::Arc; + +/// Read access or write access. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Access { + Read, + Write, +} + +/// The folders automation may read and write; a root left out grants no access of its kind. +#[derive(Debug)] +pub struct AutomationRoots { + read: Option, + write: Option, +} + +#[derive(Debug)] +struct Root { + /// The folder made absolute, links followed: what paths are compared with. + resolved: PathBuf, + /// The same without Windows' `\\?\` prefix, for people. + shown: PathBuf, +} + +impl Root { + /// The folder at `path`, which must exist. + fn open(path: &Path, access: Access) -> Result { + let kind = match access { + Access::Read => "read", + Access::Write => "write", + }; + let resolved = std::fs::canonicalize(path) + .and_then(|p| if p.is_dir() { Ok(p) } else { Err(std::io::ErrorKind::NotADirectory.into()) }) + .map_err(|e| format!("cannot open the automation {kind} root `{}`: {e}", path.display()))?; + Ok(Self { shown: plain(resolved.clone()), resolved }) + } +} + +impl AutomationRoots { + /// The roots `read` and `write`, which must be existing folders; `None` when neither is given + /// (nothing is confined then). + pub fn new(read: Option<&Path>, write: Option<&Path>) -> Result>, String> { + if read.is_none() && write.is_none() { + return Ok(None); + } + let read = read.map(|p| Root::open(p, Access::Read)).transpose()?; + let write = write.map(|p| Root::open(p, Access::Write)).transpose()?; + Ok(Some(Arc::new(Self { read, write }))) + } + + /// The read root, absolute with its links followed. + pub fn read_root(&self) -> Option<&Path> { + self.read.as_ref().map(|r| r.shown.as_path()) + } + + /// The write root, absolute with its links followed. + pub fn write_root(&self) -> Option<&Path> { + self.write.as_ref().map(|r| r.shown.as_path()) + } + + /// May the file or folder at `path` be read (or written)? `Err` says why not. + pub fn check(&self, path: &str, access: Access) -> Result<(), String> { + self.check_path(path, Path::new(path), access) + } + + /// [`check`](Self::check) for a path that was already put together from `shown` (the way the + /// caller wrote it, which is what refusals quote). + pub fn check_path(&self, shown: &str, target: &Path, access: Access) -> Result<(), String> { + let root = match access { + Access::Read => self.read.as_ref(), + Access::Write => self.write.as_ref(), + }; + let Some(Root { resolved: folder, shown: root_shown }) = root else { + return match access { + Access::Read => Err(format!("automation filesystem access is not granted: read authority is absent: {shown}")), + Access::Write => Err(format!("automation filesystem access is not granted: write authority is absent: {shown}")), + }; + }; + if inside(&resolve(shown, target)?, folder) { + return Ok(()); + } + let root = root_shown.display(); + match access { + Access::Read => Err(format!("automation path rejected: outside the read root {root}: {shown}")), + Access::Write => Err(format!("automation path rejected: outside the write root {root}: {shown}")), + } + } +} + +/// `path` without Windows' `\\?\` prefix (which `canonicalize` adds), for people to read. +fn plain(path: PathBuf) -> PathBuf { + if !cfg!(windows) { + return path; + } + let text = path.to_string_lossy(); + if let Some(rest) = text.strip_prefix(r"\\?\UNC\") { + return PathBuf::from(format!(r"\\{rest}")); + } + match text.strip_prefix(r"\\?\") { + Some(rest) => PathBuf::from(rest), + None => path, + } +} + +/// `path` made absolute with its links followed; for a path that doesn't exist (yet), its nearest +/// existing folder's links followed and the rest appended. +fn resolve(shown: &str, path: &Path) -> Result { + if path.as_os_str().is_empty() { + return Err("automation path rejected: path is empty".into()); + } + // Checked as given: making `…\NUL` absolute gives the device path `\\.\NUL` on Windows. + if cfg!(windows) && path.components().any(|c| matches!(c, Component::Normal(n) if is_device_name(&n.to_string_lossy()))) { + return Err(format!("automation path rejected: reserved device names are not allowed: {shown}")); + } + let absolute = std::path::absolute(path).map_err(|e| format!("automation path rejected: cannot resolve {shown}: {e}"))?; + // The names below the nearest existing folder, last first. + let mut missing: Vec = vec![]; + let mut at = absolute.as_path(); + loop { + match std::fs::canonicalize(at) { + Ok(mut found) => { + found.extend(missing.iter().rev()); + return Ok(found); + } + Err(e) => match std::fs::symlink_metadata(at) { + Ok(m) if m.file_type().is_symlink() => return Err(format!("automation path rejected: a link that leads nowhere: {shown}")), + // There, yet its real path can't be had (no permission): nothing to compare. + Ok(_) => return Err(format!("automation path rejected: cannot resolve {shown}: {e}")), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => return Err(format!("automation path rejected: cannot resolve {shown}: {e}")), + }, + } + let parent = at.parent(); + match (at.components().next_back(), parent) { + (Some(Component::Normal(name)), Some(parent)) => { + missing.push(name.to_os_string()); + at = parent; + } + (Some(Component::ParentDir), _) => { + return Err(format!("automation path rejected: parent traversal below a folder that doesn't exist: {shown}")); + } + // Down to the root (or drive) and it doesn't exist either. + _ => { + let e = std::io::Error::from(std::io::ErrorKind::NotFound); + return Err(format!("automation path rejected: cannot resolve {shown}: {e}")); + } + } + } +} + +/// Is `path` (resolved) `root` or inside it, component by component (without regard to case on +/// Windows, whose file systems ignore it)? +fn inside(path: &Path, root: &Path) -> bool { + let mut path = path.components(); + root.components().all(|r| path.next().is_some_and(|p| same_component(p, r))) +} + +fn same_component(a: Component, b: Component) -> bool { + if a == b { + return true; + } + if !cfg!(windows) { + return false; + } + match (a.as_os_str().to_str(), b.as_os_str().to_str()) { + (Some(a), Some(b)) => a.to_lowercase() == b.to_lowercase(), + _ => false, + } +} + +/// A Windows device name (`CON`, `NUL`, `COM1`… also with an extension), which names no file. +fn is_device_name(component: &str) -> bool { + let stem = component.split('.').next().unwrap_or(component).trim_end_matches(' ').to_ascii_uppercase(); + let numbered = |prefix: &str| stem.strip_prefix(prefix).is_some_and(|n| matches!(n, "1" | "2" | "3" | "4" | "5" | "6" | "7" | "8" | "9")); + matches!(stem.as_str(), "CON" | "PRN" | "AUX" | "NUL" | "CLOCK$" | "CONIN$" | "CONOUT$") || numbered("COM") || numbered("LPT") +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A fresh folder holding `in` (read root), `out` (write root) and `outside`. + fn folders(name: &str) -> (PathBuf, Arc) { + let base = std::env::temp_dir().join(format!("pc-file-access-{name}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + for d in ["in/sub", "out", "outside", "in2"] { + std::fs::create_dir_all(base.join(d)).unwrap(); + } + std::fs::write(base.join("in/a.pdf"), "x").unwrap(); + std::fs::write(base.join("outside/secret.pdf"), "x").unwrap(); + let roots = AutomationRoots::new(Some(&base.join("in")), Some(&base.join("out"))).unwrap().unwrap(); + (base, roots) + } + + fn s(p: PathBuf) -> String { + p.to_string_lossy().into_owned() + } + + #[test] + fn paths_inside_the_roots_pass_and_others_dont() { + let (base, roots) = folders("inside"); + assert_eq!(roots.check(&s(base.join("in/a.pdf")), Access::Read), Ok(())); + assert_eq!(roots.check(&s(base.join("in")), Access::Read), Ok(()), "the root itself"); + assert_eq!(roots.check(&s(base.join("in/sub/new.pdf")), Access::Read), Ok(()), "missing files are checked by their folder"); + assert_eq!(roots.check(&s(base.join("out/new/deeper/x.png")), Access::Write), Ok(()), "and by their nearest folder"); + let e = roots.check(&s(base.join("outside/secret.pdf")), Access::Read).unwrap_err(); + assert!(e.starts_with("automation path rejected: outside the read root"), "{e}"); + // Read and write access are separate. + let e = roots.check(&s(base.join("in/a.pdf")), Access::Write).unwrap_err(); + assert!(e.contains("outside the write root"), "{e}"); + assert!(roots.check(&s(base.join("out/x.png")), Access::Read).unwrap_err().contains("outside the read root")); + let _ = std::fs::remove_dir_all(base); + } + + #[test] + fn a_sibling_with_the_same_prefix_is_outside() { + let (base, roots) = folders("prefix"); + std::fs::write(base.join("in2/b.pdf"), "x").unwrap(); + assert!(roots.check(&s(base.join("in2/b.pdf")), Access::Read).is_err(), "in2 is not inside in"); + assert!(roots.check(&format!("{}2/c.pdf", s(base.join("in"))), Access::Read).is_err()); + let _ = std::fs::remove_dir_all(base); + } + + #[test] + fn parent_traversal_is_resolved_or_refused() { + let (base, roots) = folders("dotdot"); + let sep = std::path::MAIN_SEPARATOR; + let up = format!("{}{sep}..{sep}outside{sep}secret.pdf", s(base.join("in"))); + assert!(roots.check(&up, Access::Read).unwrap_err().contains("outside the read root"), "`..` out of the root"); + let back = format!("{}{sep}..{sep}a.pdf", s(base.join("in/sub"))); + assert_eq!(roots.check(&back, Access::Read), Ok(()), "`..` that stays inside"); + let missing = format!("{}{sep}nope{sep}..{sep}..{sep}outside{sep}x.pdf", s(base.join("out"))); + let e = roots.check(&missing, Access::Write).unwrap_err(); + // Windows resolves `..` by name first (so does opening the file there): then it's outside. + assert!(e.contains("parent traversal below a folder that doesn't exist") || e.contains("outside the write root"), "{e}"); + let _ = std::fs::remove_dir_all(base); + } + + #[test] + fn relative_paths_resolve_against_the_working_directory() { + let (base, roots) = folders("relative"); + // The tests' working directory is the crate's folder: not inside the roots. + assert!(roots.check("a.pdf", Access::Read).unwrap_err().contains("outside the read root")); + assert!(roots.check("", Access::Read).unwrap_err().contains("path is empty")); + let _ = std::fs::remove_dir_all(base); + } + + #[test] + fn a_missing_root_is_an_error_and_none_is_no_confinement() { + let (base, _) = folders("missing"); + let e = AutomationRoots::new(Some(&base.join("nope")), None).unwrap_err(); + assert!(e.starts_with("cannot open the automation read root"), "{e}"); + let e = AutomationRoots::new(None, Some(&base.join("in/a.pdf"))).unwrap_err(); + assert!( + e.starts_with("cannot open the automation write root") + && e.ends_with(&std::io::Error::from(std::io::ErrorKind::NotADirectory).to_string()), + "{e}" + ); + assert!(AutomationRoots::new(Some(Path::new("")), None).unwrap_err().starts_with("cannot open the automation read root ``")); + assert!(AutomationRoots::new(None, None).unwrap().is_none()); + // One root alone grants only its own access. + let read_only = AutomationRoots::new(Some(&base.join("in")), None).unwrap().unwrap(); + let e = read_only.check(&s(base.join("in/x.pdf")), Access::Write).unwrap_err(); + assert!(e.contains("write authority is absent"), "{e}"); + let _ = std::fs::remove_dir_all(base); + } + + #[test] + fn links_are_followed_before_comparing() { + let (base, roots) = folders("links"); + #[cfg(unix)] + let made = |target: &Path, link: &Path| std::os::unix::fs::symlink(target, link).is_ok(); + #[cfg(windows)] + let made = |target: &Path, link: &Path| { + let made = + if target.is_dir() { std::os::windows::fs::symlink_dir(target, link) } else { std::os::windows::fs::symlink_file(target, link) }; + made.is_ok() + }; + #[cfg(not(any(unix, windows)))] + let made = |_: &Path, _: &Path| false; + // Windows needs Developer Mode or elevation to make links: nothing to test without them. + if !made(&base.join("outside/secret.pdf"), &base.join("in/leak.pdf")) { + eprintln!("skipped: symbolic links can't be made here"); + let _ = std::fs::remove_dir_all(base); + return; + } + assert!(roots.check(&s(base.join("in/leak.pdf")), Access::Read).unwrap_err().contains("outside the read root"), "a file link out"); + assert!(made(&base.join("outside"), &base.join("out/away"))); + let e = roots.check(&s(base.join("out/away/new.png")), Access::Write).unwrap_err(); + assert!(e.contains("outside the write root"), "a folder link out: {e}"); + assert!(made(&base.join("outside/gone.pdf"), &base.join("out/dangling.png"))); + let e = roots.check(&s(base.join("out/dangling.png")), Access::Write).unwrap_err(); + assert!(e.contains("a link that leads nowhere"), "writing would create the file outside: {e}"); + // A link to a folder inside the root stays inside. + assert!(made(&base.join("in/sub"), &base.join("in/alias"))); + assert_eq!(roots.check(&s(base.join("in/alias/x.pdf")), Access::Read), Ok(())); + // A root given through a link is the folder it leads to. + assert!(made(&base.join("in"), &base.join("in-link"))); + let linked = AutomationRoots::new(Some(&base.join("in-link")), None).unwrap().unwrap(); + assert_eq!(linked.check(&s(base.join("in/a.pdf")), Access::Read), Ok(())); + let _ = std::fs::remove_dir_all(base); + } + + #[cfg(windows)] + #[test] + fn windows_ignores_case_and_refuses_devices() { + let (base, roots) = folders("case"); + let upper = s(base.join("IN/A.PDF")); + assert_eq!(roots.check(&upper, Access::Read), Ok(()), "{upper}"); + assert_eq!(roots.check(&s(base.join("In/Sub/New.pdf")), Access::Read), Ok(())); + let e = roots.check(&s(base.join("out/NUL")), Access::Write).unwrap_err(); + assert!(e.contains("reserved device names"), "{e}"); + assert!(roots.check(&s(base.join("out/com1.txt")), Access::Write).is_err()); + assert_eq!(roots.check(&s(base.join("out/console.txt")), Access::Write), Ok(())); + let _ = std::fs::remove_dir_all(base); + } + + /// Every file a tool reads or writes goes through `Automation::resolve`, which holds it to the + /// roots. A function that reaches for the file system without resolving a path first would + /// bypass them, so each use of `std::fs` or `File::open` must come after a `resolve` call in + /// the same function (or be one of the helpers that only ever get a resolved path). + #[test] + fn tools_do_not_touch_the_file_system_without_resolving_the_path() { + // Helpers that are only called with a resolved path. `doc_batch` (batch.rs) resolves its + // inputs and out_dir, walks only inside them (skipping links) and resolves every + // discovered file again before `process` reads it or `publish` writes it. + const RESOLVED_BY_CALLER: &[&str] = &["write_atomic", "write_atomic_with", "read_review_sidecar", "process", "linked", "walk", "publish"]; + let dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); + let mut offenders = vec![]; + for entry in std::fs::read_dir(&dir).unwrap() { + let path = entry.unwrap().path(); + if path.extension().is_none_or(|e| e != "rs") + || path.file_name().is_some_and(|n| n == "file_access.rs" || n.to_string_lossy().starts_with("tests")) + { + continue; + } + let text = std::fs::read_to_string(&path).unwrap(); + // Test modules (which build their own fixtures) come last in each file. + let code = text.split("\n#[cfg(test)]").next().unwrap_or(""); + let lines: Vec<&str> = code.lines().collect(); + for (n, line) in lines.iter().enumerate() { + let trimmed = line.trim_start(); + if trimmed.starts_with("//") + || !(trimmed.contains("std::fs") + || trimmed.contains("File::open") + || trimmed.contains("File::create") + || trimmed.contains("OpenOptions")) + { + continue; + } + let start = (0..=n).rev().find(|&i| lines[i].trim_start().contains("fn ") && lines[i].trim_start().ends_with('{')).unwrap_or(0); + let name = lines[start].split("fn ").nth(1).and_then(|r| r.split(['(', '<']).next()).unwrap_or(""); + if !RESOLVED_BY_CALLER.contains(&name) && !lines[start..=n].iter().any(|l| l.contains("resolve")) { + offenders.push(format!("{}:{}: in `{name}`: {}", path.display(), n + 1, trimmed)); + } + } + } + assert!(offenders.is_empty(), "resolve the path with `self.resolve` first: {offenders:#?}"); + } +} diff --git a/crates/automation/src/lib.rs b/crates/automation/src/lib.rs index 71f66e035..f2d5cbbed 100644 --- a/crates/automation/src/lib.rs +++ b/crates/automation/src/lib.rs @@ -7,7 +7,10 @@ //! server ([`mcp`]), `pdfcraft-cli run` and (later) the UI control channel. //! - Pages are **1-based** in every tool, as people number them. Rectangles are in PDF points //! with the origin at the top-left of the displayed page. -//! - An optional root directory confines every path a tool reads or writes. +//! - An optional root directory confines every path a tool reads or writes. Separately, +//! [`file_access::AutomationRoots`] (`pdfcraft-cli mcp --automation-read-root` and +//! `--automation-write-root`) holds a read folder and a write folder, each left out meaning no +//! access of its kind. #![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] @@ -16,6 +19,7 @@ mod batch; mod comments; mod content; mod conventions; +pub mod file_access; mod forms; mod links; #[cfg(feature = "mcp")] @@ -108,6 +112,8 @@ const FIND_MAX_WARNING_BYTES: usize = 1024; pub struct Automation { session: Session, root: Option, + /// The read and write folders of `--automation-read-root` / `--automation-write-root`. + roots: Option>, /// Synchronous renderers, rebuilt when a document's working bytes change. renderers: HashMap>, PageRenderer)>, /// Extracted page text, under one memory budget across all open documents. @@ -122,7 +128,7 @@ impl Default for Automation { impl Automation { pub fn new() -> Self { - Self { session: Session::new(), root: None, renderers: HashMap::new(), texts: TextCache::default() } + Self { session: Session::new(), root: None, roots: None, renderers: HashMap::new(), texts: TextCache::default() } } /// Confine every path the tools read or write to `root` (relative paths resolve inside it). @@ -135,6 +141,14 @@ impl Automation { Ok(self) } + /// Confine the files the tools read to the read root and the files they write to the write + /// root (`None`: no confinement). Applies on top of [`with_root`](Self::with_root) when both + /// are set. + pub fn with_automation_roots(mut self, roots: Option>) -> Self { + self.roots = roots; + self + } + /// Use a fixed clock for saves (deterministic output in tests). pub fn with_clock(mut self, clock: fn() -> i64) -> Self { self.session = std::mem::take(&mut self.session).with_clock(clock); @@ -158,7 +172,7 @@ impl Automation { /// Returns where the file went. pub fn write_output(&self, path: &str, bytes: &[u8]) -> Result { let target = self.resolve(path, true)?; - let special = self.root.is_none() && std::fs::metadata(&target).is_ok_and(|m| !m.is_file() && !m.is_dir()); + let special = self.root.is_none() && self.roots.is_none() && std::fs::metadata(&target).is_ok_and(|m| !m.is_file() && !m.is_dir()); if special { std::fs::write(&target, bytes).map_err(|e| failed(format!("{path}: {e}")))?; } else { @@ -538,6 +552,12 @@ impl Automation { .iter() .map(|p| self.resolve(p.as_str().ok_or_else(|| ToolError::InvalidArgs("directory must be a path".into()))?, false)) .collect::>>()? + } else if let Some(roots) = &self.roots { + // The installed fonts are outside the roots: look in the read root instead. + let read = roots + .read_root() + .ok_or_else(|| failed("automation filesystem access is not granted: read authority is absent: installed fonts"))?; + vec![self.resolve(&read.to_string_lossy(), false)?] } else if let Some(root) = &self.root { vec![root.clone()] } else { @@ -889,7 +909,14 @@ impl Automation { let id = doc.id; let target = match a.opt_str("path")? { Some(p) => self.resolve(p, true)?, - None => PathBuf::from(doc.path.clone().ok_or_else(|| failed("the document has never been saved: pass a path"))?), + None => { + // Saving in place writes where the file was opened from, which the read root + // allows and the write root may not. + let own = doc.path.clone().ok_or_else(|| failed("the document has never been saved: pass a path"))?; + let own_path = PathBuf::from(&own); + self.check_roots(&own, &own_path, file_access::Access::Write)?; + own_path + } }; let same_file = doc.path.as_deref().is_some_and(|p| Path::new(p) == target); // Saving to a new file is a full rewrite unless asked otherwise, like Save As. @@ -2006,6 +2033,29 @@ impl Automation { /// namespace is refused without touching it, and the deepest existing ancestor (links /// followed) must be inside the root before anything about the rest is reported. fn resolve(&self, path: &str, for_write: bool) -> Result { + let access = if for_write { file_access::Access::Write } else { file_access::Access::Read }; + self.resolve_for(path, access, for_write) + } + + /// [`resolve`](Self::resolve) with the kind of access named, for the places where a file that + /// is only read may still be missing (a path checked up front, whose absence is reported per + /// file later). The automation roots, if any, get the last word. + fn resolve_for(&self, path: &str, access: file_access::Access, may_be_missing: bool) -> Result { + let target = self.resolve_in_root(path, may_be_missing)?; + self.check_roots(path, &target, access)?; + Ok(target) + } + + /// Hold `target`, which `path` named, to the automation read or write root. Always passes + /// without roots. + fn check_roots(&self, path: &str, target: &Path, access: file_access::Access) -> Result<()> { + match &self.roots { + Some(roots) => roots.check_path(path, target, access).map_err(failed), + None => Ok(()), + } + } + + fn resolve_in_root(&self, path: &str, for_write: bool) -> Result { let p = Path::new(path); let Some(root) = &self.root else { return Ok(p.to_path_buf()) }; let outside = || failed(format!("{path} is outside the allowed directory {}", root.display())); diff --git a/crates/automation/tests/automation_roots.rs b/crates/automation/tests/automation_roots.rs new file mode 100644 index 000000000..94958ea2f --- /dev/null +++ b/crates/automation/tests/automation_roots.rs @@ -0,0 +1,376 @@ +//! `pdfcraft-cli mcp --automation-read-root --automation-write-root `: through the MCP +//! protocol, files open and load only from inside the read root and are written only inside the +//! write root. Paths that leave a root (`..`, another folder, a sibling with the same prefix, a +//! link) are refused with the wording VectorCraft uses, nothing is read into the result and +//! nothing is written outside. +#![cfg(feature = "mcp")] + +use std::path::{Path, PathBuf}; + +use pdfcraft_automation::Automation; +use pdfcraft_automation::file_access::AutomationRoots; +use pdfcraft_automation::mcp::McpServer; +use serde_json::{Value, json}; + +/// A 1x1 PNG, so the tools that take a picture have something real to read. +const PNG: &[u8] = &[ + 0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x0d, 0x49, 0x48, 0x44, 0x52, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x01, + 0x08, 0x06, 0x00, 0x00, 0x00, 0x1f, 0x15, 0xc4, 0x89, 0x00, 0x00, 0x00, 0x0d, 0x49, 0x44, 0x41, 0x54, 0x78, 0x9c, 0x63, 0xf8, 0xcf, 0xc0, 0xf0, + 0x1f, 0x00, 0x05, 0x00, 0x01, 0xff, 0x89, 0x99, 0x3d, 0x1d, 0x00, 0x00, 0x00, 0x00, 0x49, 0x45, 0x4e, 0x44, 0xae, 0x42, 0x60, 0x82, +]; + +/// A one-page PDF reading `text`, with a 1x1 picture on it. +fn pdf(text: &str) -> Vec { + let body = format!("q 100 0 0 100 20 20 cm /Im1 Do Q BT /F1 24 Tf 20 150 Td ({text}) Tj ET"); + let objs: Vec> = vec![ + b"<< /Type /Catalog /Pages 2 0 R >>".to_vec(), + b"<< /Type /Pages /Kids [4 0 R] /Count 1 /MediaBox [0 0 200 300] >>".to_vec(), + b"<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica >>".to_vec(), + b"<< /Type /Page /Parent 2 0 R /Contents 5 0 R /Resources << /Font << /F1 3 0 R >> /XObject << /Im1 6 0 R >> >> >>".to_vec(), + format!("<< /Length {} >>\nstream\n{body}\nendstream", body.len()).into_bytes(), + b"<< /Type /XObject /Subtype /Image /Width 1 /Height 1 /ColorSpace /DeviceRGB /BitsPerComponent 8 /Length 3 >>\nstream\n\xff\x00\x00\nendstream" + .to_vec(), + ]; + let mut out = b"%PDF-1.7\n".to_vec(); + let mut offsets = Vec::new(); + for (i, o) in objs.iter().enumerate() { + offsets.push(out.len()); + out.extend_from_slice(format!("{} 0 obj\n", i + 1).as_bytes()); + out.extend_from_slice(o); + out.extend_from_slice(b"\nendobj\n"); + } + let xref = out.len(); + out.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objs.len() + 1).as_bytes()); + for o in offsets { + out.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes()); + } + out.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", objs.len() + 1).as_bytes()); + out +} + +/// A fresh folder holding `in` (the read root, with `a.pdf`, `b.pdf` and `pic.png`), `out` (the +/// write root), `outside` (with `secret.pdf` and a picture) and `in2`. +fn folders(tag: &str) -> PathBuf { + let base = std::env::temp_dir().join(format!("pdfcraft-mcp-roots-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + for d in ["in", "out", "outside", "in2"] { + std::fs::create_dir_all(base.join(d)).unwrap(); + } + std::fs::write(base.join("in/a.pdf"), pdf("Inside page")).unwrap(); + std::fs::write(base.join("in/b.pdf"), pdf("Second inside")).unwrap(); + std::fs::write(base.join("in/pic.png"), PNG).unwrap(); + std::fs::write(base.join("outside/secret.pdf"), pdf("TOP SECRET")).unwrap(); + std::fs::write(base.join("outside/pic.png"), PNG).unwrap(); + std::fs::write(base.join("in2/c.pdf"), pdf("Sibling")).unwrap(); + base +} + +/// A server confined to `in` and `out` (either may be left out). +fn confined(tag: &str, read: bool, write: bool) -> (McpServer, PathBuf) { + let base = folders(tag); + let (in_dir, out_dir) = (base.join("in"), base.join("out")); + let roots = AutomationRoots::new(read.then_some(in_dir.as_path()), write.then_some(out_dir.as_path())).unwrap(); + (McpServer::new(Automation::new().with_automation_roots(roots).with_clock(|| 1_700_000_000)), base) +} + +fn s(p: &Path) -> String { + p.to_string_lossy().into_owned() +} + +fn rpc(server: &mut McpServer, method: &str, params: Value) -> Value { + let line = json!({"jsonrpc": "2.0", "id": 1, "method": method, "params": params}).to_string(); + serde_json::from_str(&server.handle_line(&line).unwrap()).unwrap() +} + +/// `tools/call` -> (isError, all the text of the reply). +fn call(server: &mut McpServer, name: &str, args: Value) -> (bool, String) { + let reply = rpc(server, "tools/call", json!({"name": name, "arguments": args})); + let result = &reply["result"]; + let mut text = + result["content"].as_array().map(|c| c.iter().filter_map(|c| c["text"].as_str()).collect::>().join("\n")).unwrap_or_default(); + text.push_str(&result["structuredContent"].to_string()); + text.push_str(&reply["error"].to_string()); + (result["isError"] == true || reply["error"].is_object(), text) +} + +fn ok(server: &mut McpServer, name: &str, args: Value) -> Value { + let reply = rpc(server, "tools/call", json!({"name": name, "arguments": args})); + assert!(reply["result"]["isError"] == false, "{name} {args}: {reply}"); + reply["result"]["structuredContent"].clone() +} + +fn refused(server: &mut McpServer, name: &str, args: Value, why: &str) { + let (error, text) = call(server, name, args.clone()); + assert!(error && text.contains(why), "{name} {args} should be refused ({why}): {text}"); + assert!(!text.contains("TOP SECRET"), "{name} {args} let the file's text through: {text}"); +} + +/// The text of document 1, through the resource a client can read it with. +fn text_of(server: &mut McpServer, doc: u64) -> String { + rpc(server, "resources/read", json!({"uri": format!("pdfcraft://doc/{doc}/text")}))["result"]["contents"][0]["text"] + .as_str() + .unwrap_or_default() + .to_owned() +} + +fn names(dir: &Path) -> Vec { + let mut names: Vec = std::fs::read_dir(dir).unwrap().map(|e| e.unwrap().file_name().to_string_lossy().into_owned()).collect(); + names.sort(); + names +} + +#[test] +fn files_open_only_from_inside_the_read_root() { + let (mut server, base) = confined("open", true, true); + let sep = std::path::MAIN_SEPARATOR; + ok(&mut server, "doc_open", json!({"path": s(&base.join("in/a.pdf"))})); + assert!(text_of(&mut server, 1).contains("Inside page")); + + refused(&mut server, "doc_open", json!({"path": s(&base.join("outside/secret.pdf"))}), "outside the read root"); + refused(&mut server, "doc_open", json!({"path": format!("{}{sep}..{sep}outside{sep}secret.pdf", s(&base.join("in")))}), "outside the read root"); + // A sibling whose name starts with the root's name is not inside it. + refused(&mut server, "doc_open", json!({"path": s(&base.join("in2/c.pdf"))}), "outside the read root"); + // The write root is not readable: the two kinds of access are separate. + std::fs::write(base.join("out/d.pdf"), pdf("Written")).unwrap(); + refused(&mut server, "doc_open", json!({"path": s(&base.join("out/d.pdf"))}), "outside the read root"); + // A relative path is the working directory's (the crate's folder here): outside. + refused(&mut server, "doc_open", json!({"path": "Cargo.toml"}), "outside the read root"); + // Nothing was opened by the refused calls. + assert_eq!(ok(&mut server, "doc_list", json!({}))["documents"].as_array().unwrap().len(), 1); +} + +#[test] +fn tools_that_read_a_path_are_held_to_the_read_root() { + let (mut server, base) = confined("reads", true, true); + ok(&mut server, "doc_open", json!({"path": s(&base.join("in/a.pdf"))})); + let (secret, pic, gone) = (s(&base.join("outside/secret.pdf")), s(&base.join("outside/pic.png")), s(&base.join("outside/gone.bin"))); + let out = s(&base.join("out/result.pdf")); + for (tool, args) in [ + ("page_insert_file", json!({"doc": 1, "path": secret, "at": 1})), + ("page_replace", json!({"doc": 1, "pages": [1], "path": secret})), + ("doc_combine", json!({"paths": [s(&base.join("in/a.pdf")), secret], "out": out})), + ("doc_create_multiple", json!({"paths": [secret], "mode": "combine", "out": out})), + ("doc_create", json!({"from": "images", "paths": [pic]})), + ("stamp_custom", json!({"doc": 1, "page": 1, "path": secret, "at": [10, 10]})), + ("page_add_image", json!({"doc": 1, "page": 1, "path": pic})), + ("image_edit", json!({"doc": 1, "page": 1, "image": 1, "action": "replace", "path": pic})), + ("comment_add", json!({"doc": 1, "page": 1, "type": "attachment", "at": [10, 10], "path": secret})), + ("fill_sign_add", json!({"doc": 1, "page": 1, "type": "signature", "at": [10, 10], "path": pic})), + ("form_set_image", json!({"doc": 1, "field": "f", "path": pic})), + ("doc_import_data", json!({"doc": 1, "path": secret})), + ("doc_watermark", json!({"doc": 1, "file": pic})), + ("doc_background", json!({"doc": 1, "file": pic})), + ("form_merge_data", json!({"paths": [secret], "path": out})), + ("ocr_recognize_files", json!({"paths": [secret], "folder": s(&base.join("out/ocr"))})), + ("sign_trust", json!({"paths": [secret]})), + ("sign_document", json!({"doc": 1, "id": secret, "out": out})), + ("text_font_import", json!({"doc": 1, "page": 1, "path": gone})), + ("text_font_files", json!({"directories": [s(&base.join("outside"))]})), + ] { + refused(&mut server, tool, args, "outside the read root"); + } + // The refused calls changed neither the document nor the disk. + assert!(!text_of(&mut server, 1).contains("TOP SECRET")); + assert_eq!(ok(&mut server, "doc_list", json!({}))["documents"].as_array().unwrap().len(), 1); + assert_eq!(names(&base.join("out")), Vec::::new()); + + // One file per output: a file that can't be read is reported in its own entry, with the same refusal. + let (_, text) = call(&mut server, "doc_create_multiple", json!({"paths": [secret], "mode": "separate", "out_dir": s(&base.join("out/sep"))})); + assert!(text.contains("outside the read root") && !text.contains("TOP SECRET"), "{text}"); + assert!(names(&base.join("out/sep")).is_empty()); + let (_, text) = call(&mut server, "action_run", json!({"paths": [secret], "folder": s(&base.join("out/act")), "action": "Add Page Numbers"})); + assert!(text.contains("outside the read root") && !text.contains("TOP SECRET"), "{text}"); + assert!(names(&base.join("out/act")).is_empty()); + std::fs::remove_dir_all(base.join("out/act")).unwrap(); + + // The same tools work on files inside the root. + std::fs::remove_dir_all(base.join("out/sep")).unwrap(); + ok(&mut server, "page_insert_file", json!({"doc": 1, "path": s(&base.join("in/b.pdf")), "at": 2})); + assert!(text_of(&mut server, 1).contains("Second inside")); + ok(&mut server, "page_add_image", json!({"doc": 1, "page": 1, "path": s(&base.join("in/pic.png"))})); + ok(&mut server, "doc_combine", json!({"paths": [s(&base.join("in/a.pdf")), s(&base.join("in/b.pdf"))], "out": out})); + assert_eq!(names(&base.join("out")), ["result.pdf"]); + // Without a folder to list, the installed fonts are outside the read root: it is searched instead. + ok(&mut server, "text_font_files", json!({})); + ok(&mut server, "text_font_files", json!({"directories": [s(&base.join("in"))]})); +} + +#[test] +fn tools_that_write_a_path_are_held_to_the_write_root() { + let (mut server, base) = confined("writes", true, true); + let input = |name: &str| s(&base.join("in").join(name)); + ok(&mut server, "doc_open", json!({"path": input("a.pdf")})); + ok(&mut server, "doc_open", json!({"path": input("b.pdf")})); + let writes = |dir: &str| -> Vec<(&'static str, Value)> { + let file = |name: &str| s(&base.join(dir).join(name)); + vec![ + ("doc_save", json!({"doc": 1, "path": file("saved.pdf")})), + ("page_extract", json!({"doc": 1, "pages": [1], "out": file("extract.pdf")})), + ("page_extract", json!({"doc": 1, "pages": [1], "separate": true, "out_dir": file("pages")})), + ("doc_combine", json!({"paths": [input("a.pdf"), input("b.pdf")], "out": file("combined.pdf")})), + ("doc_create_multiple", json!({"paths": [input("a.pdf")], "mode": "combine", "out": file("multiple.pdf")})), + ("doc_create_multiple", json!({"paths": [input("a.pdf")], "mode": "separate", "out_dir": file("separate")})), + ("doc_split", json!({"doc": 1, "every": 1, "out_dir": file("parts")})), + ("doc_reduce", json!({"doc": 1, "path": file("reduced.pdf")})), + ("doc_optimize", json!({"doc": 1, "path": file("optimized.pdf")})), + ("doc_print", json!({"doc": 1, "path": file("printed.pdf")})), + ("doc_export_text", json!({"doc": 1, "path": file("text.txt")})), + ("doc_export_office", json!({"doc": 1, "path": file("export.docx")})), + ("doc_export_data", json!({"doc": 1, "path": file("data.xfdf")})), + ("doc_export_images", json!({"doc": 1, "folder": file("renders")})), + ("doc_export_all_images", json!({"doc": 1, "folder": file("images")})), + ("image_save", json!({"doc": 1, "page": 1, "image": 1, "path": file("image.png")})), + ("measure_export", json!({"doc": 1, "out": file("measurements.csv")})), + ("comments_summarize", json!({"doc": 1, "out": file("summary.pdf")})), + ("accessibility_report", json!({"doc": 1, "path": file("report.html")})), + ("accessibility_review_record", json!({"before": 1, "after": 2, "path": file("record.json")})), + ("doc_compare_report", json!({"doc": 1, "other": 2, "path": file("compare.pdf")})), + ("form_merge_data", json!({"paths": [input("a.pdf")], "path": file("merged.csv")})), + ("action_run", json!({"paths": [input("a.pdf")], "folder": file("actions"), "action": "Add Page Numbers"})), + ("ocr_recognize_files", json!({"paths": [input("a.pdf")], "folder": file("ocr")})), + ("sign_id_create", json!({"name": "Test", "password": "secret12", "path": file("id.p12"), "key": "p256"})), + ] + }; + + for (tool, args) in writes("outside") { + refused(&mut server, tool, args, "outside the write root"); + } + // The read root is not writable either. + for (tool, args) in writes("in") { + refused(&mut server, tool, args, "outside the write root"); + } + // `..` out of the write root, and a sibling that shares its prefix. + let sep = std::path::MAIN_SEPARATOR; + refused( + &mut server, + "doc_save", + json!({"doc": 1, "path": format!("{}{sep}..{sep}outside{sep}up.pdf", s(&base.join("out")))}), + "outside the write root", + ); + std::fs::create_dir_all(base.join("out2")).unwrap(); + refused(&mut server, "doc_save", json!({"doc": 1, "path": s(&base.join("out2/x.pdf"))}), "outside the write root"); + // Saving in place writes to the folder the file was opened from: the read root, not the write root. + refused(&mut server, "doc_save", json!({"doc": 1}), "outside the write root"); + + assert_eq!(names(&base.join("outside")), ["pic.png", "secret.pdf"], "nothing was written outside the roots"); + assert_eq!(names(&base.join("in")), ["a.pdf", "b.pdf", "pic.png"], "nothing was written to the read root"); + assert!(names(&base.join("out2")).is_empty()); + assert!(names(&base.join("out")).is_empty()); + + // The same calls land in the write root. + for (tool, args) in writes("out") { + let (error, text) = call(&mut server, tool, args.clone()); + // (Printing to a PDF and the like may not apply to this fixture: a refusal for the roots is the failure.) + assert!(!text.contains("automation path rejected") && !text.contains("automation filesystem access"), "{tool} {args}: {text}"); + let _ = error; + } + assert!(base.join("out/saved.pdf").is_file() && base.join("out/combined.pdf").is_file() && base.join("out/text.txt").is_file()); + assert_eq!(names(&base.join("outside")), ["pic.png", "secret.pdf"]); +} + +#[test] +fn signing_writes_and_reads_only_inside_the_roots() { + // One folder for both roots, so a digital ID can be made and then read back. + let base = folders("signing"); + let work = base.join("work"); + std::fs::create_dir_all(&work).unwrap(); + std::fs::write(work.join("a.pdf"), pdf("Work page")).unwrap(); + let roots = AutomationRoots::new(Some(&work), Some(&work)).unwrap(); + let mut server = McpServer::new(Automation::new().with_automation_roots(roots).with_clock(|| 1_700_000_000)); + ok(&mut server, "doc_open", json!({"path": s(&work.join("a.pdf"))})); + ok(&mut server, "sign_id_create", json!({"name": "Test", "password": "secret12", "path": s(&work.join("id.p12")), "key": "p256"})); + refused( + &mut server, + "sign_id_create", + json!({"name": "Test", "password": "secret12", "path": s(&base.join("outside/id.p12")), "key": "p256"}), + "outside the write root", + ); + refused( + &mut server, + "sign_document", + json!({"doc": 1, "id": s(&work.join("id.p12")), "password": "secret12", "out": s(&base.join("outside/signed.pdf"))}), + "outside the write root", + ); + ok(&mut server, "sign_document", json!({"doc": 1, "id": s(&work.join("id.p12")), "password": "secret12", "out": s(&work.join("signed.pdf"))})); + assert!(work.join("signed.pdf").is_file()); + assert_eq!(names(&base.join("outside")), ["pic.png", "secret.pdf"]); +} + +#[test] +fn a_root_left_out_grants_none_of_its_access() { + let (mut server, base) = confined("read-only", true, false); + ok(&mut server, "doc_open", json!({"path": s(&base.join("in/a.pdf"))})); + for target in ["out/doc.pdf", "in/doc.pdf"] { + refused(&mut server, "doc_save", json!({"doc": 1, "path": s(&base.join(target))}), "write authority is absent"); + } + refused(&mut server, "doc_save", json!({"doc": 1}), "write authority is absent"); + refused(&mut server, "doc_export_text", json!({"doc": 1, "path": s(&base.join("out/t.txt"))}), "write authority is absent"); + refused(&mut server, "page_extract", json!({"doc": 1, "pages": [1], "out": s(&base.join("out/e.pdf"))}), "write authority is absent"); + refused(&mut server, "doc_split", json!({"doc": 1, "every": 1, "out_dir": s(&base.join("out/parts"))}), "write authority is absent"); + assert!(names(&base.join("out")).is_empty() && names(&base.join("in")) == ["a.pdf", "b.pdf", "pic.png"]); + // A tool that gives its result back instead of writing it still can. + ok(&mut server, "page_extract", json!({"doc": 1, "pages": [1]})); + + let (mut server, base) = confined("write-only", false, true); + refused(&mut server, "doc_open", json!({"path": s(&base.join("in/a.pdf"))}), "read authority is absent"); + refused( + &mut server, + "doc_combine", + json!({"paths": [s(&base.join("in/a.pdf")), s(&base.join("in/b.pdf"))], "out": s(&base.join("out/c.pdf"))}), + "read authority is absent", + ); + refused(&mut server, "text_font_files", json!({}), "read authority is absent"); + ok(&mut server, "doc_create", json!({"from": "blank", "pages": 1, "width": 200, "height": 300})); + ok(&mut server, "doc_save", json!({"doc": 1, "path": s(&base.join("out/blank.pdf"))})); + assert!(base.join("out/blank.pdf").is_file()); +} + +#[test] +fn links_out_of_a_root_are_refused() { + let (mut server, base) = confined("links", true, true); + #[cfg(unix)] + let linked = std::os::unix::fs::symlink(base.join("outside"), base.join("out/away")).is_ok() + && std::os::unix::fs::symlink(base.join("outside/secret.pdf"), base.join("in/leak.pdf")).is_ok() + && std::os::unix::fs::symlink(base.join("outside/nowhere.pdf"), base.join("out/dangling.pdf")).is_ok(); + #[cfg(windows)] + let linked = std::os::windows::fs::symlink_dir(base.join("outside"), base.join("out/away")).is_ok() + && std::os::windows::fs::symlink_file(base.join("outside/secret.pdf"), base.join("in/leak.pdf")).is_ok() + && std::os::windows::fs::symlink_file(base.join("outside/nowhere.pdf"), base.join("out/dangling.pdf")).is_ok(); + #[cfg(not(any(unix, windows)))] + let linked = false; + // Windows needs Developer Mode or elevation to make links. + if !linked { + eprintln!("skipped: symbolic links can't be made here"); + return; + } + ok(&mut server, "doc_open", json!({"path": s(&base.join("in/a.pdf"))})); + refused(&mut server, "doc_open", json!({"path": s(&base.join("in/leak.pdf"))}), "outside the read root"); + refused(&mut server, "doc_save", json!({"doc": 1, "path": s(&base.join("out/away/doc.pdf"))}), "outside the write root"); + refused(&mut server, "doc_save", json!({"doc": 1, "path": s(&base.join("out/away/deeper/doc.pdf"))}), "outside the write root"); + refused(&mut server, "doc_save", json!({"doc": 1, "path": s(&base.join("out/dangling.pdf"))}), "a link that leads nowhere"); + assert_eq!(names(&base.join("outside")), ["pic.png", "secret.pdf"]); +} + +#[test] +fn without_roots_nothing_is_confined() { + let base = folders("none"); + let mut server = McpServer::new(Automation::new()); + ok(&mut server, "doc_open", json!({"path": s(&base.join("outside/secret.pdf"))})); + assert!(text_of(&mut server, 1).contains("TOP SECRET")); + ok(&mut server, "doc_save", json!({"doc": 1, "path": s(&base.join("in2/copy.pdf"))})); + assert!(base.join("in2/copy.pdf").is_file()); +} + +#[test] +fn the_roots_apply_on_top_of_a_root() { + let base = folders("both"); + let roots = AutomationRoots::new(Some(&base.join("in")), Some(&base.join("out"))).unwrap(); + // `--root` resolves relative paths inside `in`, which the read root allows and the write root does not. + let mut server = McpServer::new(Automation::new().with_root(base.join("in")).unwrap().with_automation_roots(roots)); + ok(&mut server, "doc_open", json!({"path": "a.pdf"})); + refused(&mut server, "doc_save", json!({"doc": 1, "path": "copy.pdf"}), "outside the write root"); + refused(&mut server, "doc_open", json!({"path": "../outside/secret.pdf"}), "outside the allowed directory"); + assert!(!base.join("in/copy.pdf").exists()); + // The write root alone is not enough: the other root still has its say. + refused(&mut server, "doc_save", json!({"doc": 1, "path": "../out/copy.pdf"}), "outside the allowed directory"); +} diff --git a/docs/mcp.md b/docs/mcp.md index 830a6c312..96493487c 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1,6 +1,6 @@ # MCP conventions -Start explicitly with `pdfcraft-cli mcp [--root DIR] [--compact]`. The server uses JSON lines on +Start explicitly with `pdfcraft-cli mcp [--root DIR] [--compact] [--automation-read-root DIR] [--automation-write-root DIR]`. The server uses JSON lines on 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. @@ -8,6 +8,68 @@ Supply `--root` at most once, followed by a nonempty directory path. Missing ope 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 `./`. +## Confining file access + +```sh +pdfcraft-cli mcp --automation-read-root /work/project --automation-write-root /work/project/out +``` + +```json +{"mcpServers": {"pdfcraft": {"command": "/abs/path/target/release/pdfcraft-cli", + "args": ["mcp", "--automation-read-root", "/work/project", "--automation-write-root", "/work/project/out"]}}} +``` + +The two flags (VectorCraft, PhotoCraft and the other Craft apps take the same ones, so one client +config works across the suite) keep the agent to the folders you give it: + +- Every file a tool reads must lie inside the **read root**: `doc_open`, `page_insert_file`, + `page_replace`, `doc_combine` and `doc_create_multiple` inputs, `doc_create` images, stamp, + picture, attachment, signature and form-image files, `doc_watermark` and `doc_background` files, + `doc_import_data`, `form_merge_data`, the files `action_run` and `ocr_recognize_files` process, + digital ID, graphic and certificate files for signing and `sign_trust`, `text_font_import`, and + the folders `text_font_files` searches. Without `directories`, `text_font_files` searches the read + root instead of the installed fonts. +- Every file a tool writes must lie inside the **write root**: `doc_save` (also saving in place), + `page_extract`, `doc_combine`, `doc_create_multiple`, `doc_split`, `doc_reduce`, `doc_optimize`, + `doc_print` to a file, the `doc_export_*` tools, `image_save`, `measure_export`, + `comments_summarize`, the accessibility report and review record, `doc_compare_report`, + `form_merge_data`, the output folders of `action_run` and `ocr_recognize_files`, `sign_id_create` + and `sign_document`. +- Read and write access are separate. Give the same folder twice for both. A root left out grants + none of its access: with only `--automation-read-root`, nothing can be written to disk, though + tools that give their result back instead of a path still do. Without either flag nothing changes. +- A path is made absolute against the server's working directory, then its links are followed: a file + (or folder) link that leads out of a root is outside it. For a file that doesn't exist yet, its + nearest existing folder's links are followed and the rest must be plain names, so `..` below a + folder that doesn't exist is refused, as is a link that leads nowhere (writing through it would + create its target). The result is compared with the root by whole folder names: `/work2/x` is not + inside `/work`. On Windows case is ignored, UNC paths are resolved like drive-letter ones, and + device names (`CON`, `NUL`, `COM1`...) are refused. +- A refused path is an ordinary tool error the agent can read and correct, for example + `automation path rejected: outside the write root /work/project/out: /etc/x.pdf` or + `automation filesystem access is not granted: write authority is absent: /work/project/out/x.pdf`. + Tools that handle several files report a refused one in its own entry. +- The roots must be existing folders; a missing one stops the server with an error. Each flag is + given once (`--flag ` or `--flag=`). +- `--root DIR` still works and can be combined with these flags. Both then apply: a path must be + inside `--root` and inside the matching automation root. + +`pdfcraft-cli mcp` always runs the session in its own process (there is no flag to connect it to a +running app), so the roots cover every server it starts. The desktop app is not confined by them. + +What the roots do not cover: tools that take no path still read and change the open documents, and +`text_extract` and the other inspection tools return their content. Keychain and Windows +certificate-store identities for `sign_document` are not files and are not checked. Sending a job +to a printer is not a file write. Every path a tool reads or writes goes through +`Automation::resolve`, and a unit test keeps the tool modules from touching the file system +without it. + +A path is checked, then opened: another program that swaps a link in between can still win that +race, so the roots keep an agent to the files it was given rather than guarding against other +software on the computer. + +## Tools + | Tool | Arguments | |---|---| | `command_list` | `doc?`, `filter?`, `enabled_only?`; returns `{commands: [...]}` with mapped tool schemas |