diff --git a/README.md b/README.md index d190e9eb..a0b03023 100644 --- a/README.md +++ b/README.md @@ -150,6 +150,7 @@ deckcraft-cli commands format. # list commands deckcraft-cli run --sample --cmd 'slide.new={"layout":"titleOnly","title":"Hello"}' --save hello.deckcraft deckcraft-cli mcp # MCP server over stdio (headless) deckcraft-cli mcp --connect 7990 # drive the running app (deckcraft --control 7990) +deckcraft-cli mcp --automation-read-root DIR --automation-write-root DIR # confine the headless agent to folders ``` ## Roadmap diff --git a/apps/deckcraft-cli/src/automation_args.rs b/apps/deckcraft-cli/src/automation_args.rs new file mode 100644 index 00000000..8f0cabf8 --- /dev/null +++ b/apps/deckcraft-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(), "deck.pptx".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("--sample", &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("deck.pptx")); + 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("--connect"), 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/deckcraft-cli/src/main.rs b/apps/deckcraft-cli/src/main.rs index 84ab2452..4029fea7 100644 --- a/apps/deckcraft-cli/src/main.rs +++ b/apps/deckcraft-cli/src/main.rs @@ -10,13 +10,16 @@ //! deckcraft-cli commands [FILTER] # list commands (JSON) //! deckcraft-cli describe ID # one command //! deckcraft-cli app [--port PORT] COMMAND [JSON] | --method METHOD [JSON] # drive the running app -//! deckcraft-cli mcp [--connect PORT] [--sample] # MCP server over stdio +//! deckcraft-cli mcp [--connect PORT] [--sample] [--automation-read-root DIR] [--automation-write-root DIR] +//! # MCP server over stdio //! deckcraft-cli bench [FILE | --sample] [--runs N] [--json] # time open, render, save, export //! deckcraft-cli links | --version //! ``` #![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] #![forbid(unsafe_code)] +mod automation_args; + use std::process::ExitCode; use deckcraft_engine::Session; @@ -44,7 +47,11 @@ const USAGE: &str = "usage: deckcraft-cli info FILE deckcraft-cli commands [FILTER] deckcraft-cli describe COMMAND deckcraft-cli app [--port PORT] COMMAND [JSON] | --method METHOD [JSON] - deckcraft-cli mcp [--connect PORT] [--sample] + deckcraft-cli mcp [--connect PORT] [--sample] [--automation-read-root DIR] [--automation-write-root DIR] + the two roots confine every file the agent's commands read (open, pictures, + audio, video) and write (save, export, PDF) to those folders, links followed; + a root left out grants none of its access. They run the in-process server, so + they conflict with --connect (a running app is not confined by them). deckcraft-cli bench [FILE | --sample] [--runs N] [--json] deckcraft-cli links | --version"; @@ -522,13 +529,18 @@ fn app(args: &[String]) -> Result<(), String> { } fn mcp(args: &[String]) -> Result<(), String> { + use deckcraft_engine::file_access::{self, AutomationRoots}; use deckcraft_mcp::{Backend, Headless, Remote, Server, control_addr}; let mut connect: Option = None; let mut sample = false; - let mut it = args.iter(); + let mut roots_args = automation_args::AutomationArgs::new(None, None); + let mut it = args.iter().cloned(); while let Some(a) = it.next() { + if roots_args.consume(&a, &mut it)? { + continue; + } match a.as_str() { - "--connect" => connect = Some(it.next().cloned().ok_or("--connect needs a port or host:port")?), + "--connect" => connect = Some(it.next().ok_or("--connect needs a port or host:port")?), "--sample" => sample = true, "--help" | "-h" => { outln!("{USAGE}"); @@ -537,13 +549,27 @@ fn mcp(args: &[String]) -> Result<(), String> { other => return Err(format!("unknown mcp option `{other}`")), } } + let roots = AutomationRoots::new(roots_args.read_root.as_deref(), roots_args.write_root.as_deref())?; + if roots.is_some() && connect.is_some() { + return Err("--automation-read-root and --automation-write-root confine the in-process server; a running app is not confined by them, \ +so drop --connect (or use --connect without the roots)" + .into()); + } let backend: Box = match connect { Some(c) => Box::new(Remote::connect(&control_addr(&c)).map_err(|e| format!("can't reach the app at {c}: {e}"))?), None => { let mut h = Headless::new(); + // `--sample` is the operator's own choice, made before the roots come into force. if sample { deckcraft_engine::sample::open_sample(&mut h.session).map_err(|e| e.to_string())?; } + if let Some(roots) = roots { + // Every thread of this process, not only the server's: nothing reaches past the roots. + file_access::confine_process(roots.clone()); + let show = |root: Option<&std::path::Path>| root.map_or_else(|| "none".to_string(), |r| r.display().to_string()); + eprintln!("deckcraft-cli: files confined: read root {}, write root {}", show(roots.read_root()), show(roots.write_root())); + h = h.with_automation_roots(Some(roots)); + } Box::new(h) } }; diff --git a/apps/deckcraft-cli/tests/cli.rs b/apps/deckcraft-cli/tests/cli.rs new file mode 100644 index 00000000..e9e1f856 --- /dev/null +++ b/apps/deckcraft-cli/tests/cli.rs @@ -0,0 +1,118 @@ +//! `deckcraft-cli mcp` with the automation root flags, run as the real binary. +// Integration tests: unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + +use std::io::Write; +use std::path::PathBuf; +use std::process::{Command, Output, Stdio}; + +use serde_json::Value; + +fn bin() -> Command { + Command::new(env!("CARGO_BIN_EXE_deckcraft-cli")) +} + +fn run(args: &[&str]) -> Output { + bin().args(args).output().unwrap() +} + +fn tmpdir(tag: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!("dc-cli-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + dir +} + +/// Send `calls` (tool name, arguments) to `deckcraft-cli mcp `; returns whether it exited +/// cleanly, its stderr, and the stdout lines after `initialize`, one per call. +fn mcp_session(args: &[&str], calls: &[(&str, Value)]) -> (bool, String, Vec) { + let mut child = bin().arg("mcp").args(args).stdin(Stdio::piped()).stdout(Stdio::piped()).stderr(Stdio::piped()).spawn().unwrap(); + { + let stdin = child.stdin.as_mut().unwrap(); + writeln!(stdin, r#"{{"jsonrpc":"2.0","id":1,"method":"initialize","params":{{"protocolVersion":"2025-06-18"}}}}"#).unwrap(); + for (i, (name, arguments)) in calls.iter().enumerate() { + let call = serde_json::json!({"jsonrpc": "2.0", "id": i + 2, "method": "tools/call", "params": {"name": name, "arguments": arguments}}); + writeln!(stdin, "{call}").unwrap(); + } + } + let out = child.wait_with_output().unwrap(); + let stdout = String::from_utf8(out.stdout).unwrap(); + let replies = stdout.lines().skip(1).map(|l| serde_json::from_str(l).unwrap()).collect(); + (out.status.success(), String::from_utf8_lossy(&out.stderr).into_owned(), replies) +} + +fn text(reply: &Value) -> &str { + reply["result"]["content"][0]["text"].as_str().unwrap() +} + +#[test] +fn mcp_automation_roots_confine_the_server() { + let base = tmpdir("roots"); + let (work, elsewhere) = (base.join("work"), base.join("elsewhere")); + std::fs::create_dir_all(&work).unwrap(); + std::fs::create_dir_all(&elsewhere).unwrap(); + std::fs::write(work.join("in.txt"), "Inside text").unwrap(); + std::fs::write(elsewhere.join("secret.txt"), "TOP SECRET").unwrap(); + let work_s = work.to_str().unwrap(); + let secret = elsewhere.join("secret.txt").to_str().unwrap().to_string(); + + // Both spellings of the flags; inside works, outside is refused with a readable message. + let read_flag = format!("--automation-read-root={work_s}"); + let calls = [ + ("open_presentation", serde_json::json!({"path": work.join("in.txt").to_str().unwrap()})), + ("open_presentation", serde_json::json!({"path": secret})), + ("save_presentation", serde_json::json!({"path": work.join("saved.deckcraft").to_str().unwrap()})), + ("save_presentation", serde_json::json!({"path": elsewhere.join("saved.deckcraft").to_str().unwrap()})), + ]; + let (success, stderr, replies) = mcp_session(&[&read_flag, "--automation-write-root", work_s], &calls); + assert!(success, "{stderr}"); + assert!(stderr.contains("files confined"), "{stderr}"); + assert_eq!(replies[0]["result"]["isError"], false, "{}", replies[0]); + assert_eq!(replies[1]["result"]["isError"], true); + assert!(text(&replies[1]).contains("outside the read root"), "{}", text(&replies[1])); + assert_eq!(replies[2]["result"]["isError"], false, "{}", replies[2]); + assert!(work.join("saved.deckcraft").exists()); + assert_eq!(replies[3]["result"]["isError"], true); + assert!(text(&replies[3]).contains("outside the write root"), "{}", text(&replies[3])); + assert!(!elsewhere.join("saved.deckcraft").exists()); + + // A root left out grants none of its access. + let save = [("new_presentation", serde_json::json!({})), calls[2].clone()]; + let (_, _, replies) = mcp_session(&[&read_flag], &save); + assert_eq!(replies[1]["result"]["isError"], true); + assert!(text(&replies[1]).contains("write authority is absent"), "{}", text(&replies[1])); + + // No flags: nothing is confined. + let (success, _, replies) = mcp_session(&[], &calls[1..2]); + assert!(success); + assert_eq!(replies[0]["result"]["isError"], false, "{}", replies[0]); + let _ = std::fs::remove_dir_all(base); +} + +#[test] +fn mcp_automation_root_flags_are_checked_up_front() { + let base = tmpdir("roots-args"); + let dir = base.to_str().unwrap(); + let missing = base.join("missing"); + let fails = |args: &[&str], needle: &str| { + let mut full = vec!["mcp"]; + full.extend_from_slice(args); + let o = run(&full); + assert!(!o.status.success(), "{args:?}"); + let err = String::from_utf8_lossy(&o.stderr).into_owned(); + assert!(err.contains(needle), "{args:?}: {err}"); + }; + // The flags confine the in-process server, so they cannot be combined with --connect. + fails(&["--connect", "7979", "--automation-read-root", dir], "--connect"); + fails(&["--automation-write-root", dir, "--connect", "7979"], "--connect"); + fails(&["--automation-read-root", dir, "--automation-read-root", dir], "given twice"); + fails(&["--automation-write-root", dir, "--automation-write-root", dir], "given twice"); + fails(&["--automation-read-root"], "needs a folder"); + fails(&["--automation-write-root", "--sample"], "needs a folder"); + fails(&["--automation-read-root", missing.to_str().unwrap()], "cannot open the automation read root"); + fails(&["--automation-write-root="], "needs a non-empty folder"); + // A file is not a folder. + std::fs::write(base.join("f.txt"), "x").unwrap(); + fails(&["--automation-write-root", base.join("f.txt").to_str().unwrap()], "cannot open the automation write root"); + let _ = std::fs::remove_dir_all(base); +} diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index d7356b23..8bcb5c5c 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -165,7 +165,7 @@ pub fn add_opened(s: &mut Session, name: &str, path: Option, p: Presenta #[cfg(not(target_arch = "wasm32"))] fn open(s: &mut Session, p: &Value) -> Result { let path = str_param(p, "path").ok_or_else(|| bad("file.open", "missing `path`"))?; - let bytes = std::fs::read(path).map_err(|e| EngineError::Other(format!("{path}: {e}")))?; + let bytes = crate::io::read_file(path).map_err(EngineError::Other)?; let (doc, damaged) = open_checked(path, &bytes)?; let native = deckcraft_format::sniff(&bytes) || deckcraft_pptx::sniff(&bytes); // A damaged file opens as an untitled repaired copy, so Save can't overwrite the original @@ -332,16 +332,8 @@ fn mark_saved(s: &mut Session, path: Option) -> Result<()> { Ok(()) } -#[cfg(not(target_arch = "wasm32"))] fn write_file(path: &str, bytes: &[u8]) -> Result<()> { - // Write to a temporary sibling then rename, so a failed save never truncates the old file. - let tmp = format!("{path}.saving"); - std::fs::write(&tmp, bytes).map_err(|e| EngineError::Other(format!("{path}: {e}")))?; - std::fs::rename(&tmp, path).map_err(|e| EngineError::Other(format!("{path}: {e}"))) -} -#[cfg(target_arch = "wasm32")] -fn write_file(_path: &str, _bytes: &[u8]) -> Result<()> { - Err(EngineError::Other("saving to a path is not available in the browser".into())) + crate::io::write_file(path, bytes).map_err(EngineError::Other) } fn save(s: &mut Session, p: &Value) -> Result { diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index ab9680b0..f2e54ed5 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -308,9 +308,8 @@ pub(crate) fn media_bytes(p: &Value, cmd: &str) -> Result<(String, Vec)> { let bytes = base64_decode(d).ok_or_else(|| bad(cmd, "invalid base64 `data`"))?; return Ok((str_param(p, "name").unwrap_or("media").to_string(), bytes)); } - #[cfg(not(target_arch = "wasm32"))] if let Some(path) = str_param(p, "path") { - let bytes = std::fs::read(path).map_err(|e| EngineError::Other(format!("{path}: {e}")))?; + let bytes = crate::io::read_file(path).map_err(EngineError::Other)?; let name = std::path::Path::new(path).file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_else(|| "media".into()); return Ok((name, bytes)); } diff --git a/crates/engine/src/file_access.rs b/crates/engine/src/file_access.rs new file mode 100644 index 00000000..d5ee0a99 --- /dev/null +++ b/crates/engine/src/file_access.rs @@ -0,0 +1,479 @@ +//! The folders automation may read and write. +//! +//! `deckcraft-cli mcp --automation-read-root --automation-write-root ` confines the +//! files an agent's commands reach. [`AutomationRoots`] holds the two folders. While they are in force, every path a command +//! reads goes through [`check_read`] and every path it writes through [`check_write`]: +//! +//! - 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. +//! +//! Roots are in force on a thread inside [`confine`], and in the whole process after +//! [`confine_process`] (`deckcraft-cli mcp`), except inside [`unconfined`]. 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::cell::RefCell; +use std::ffi::OsString; +use std::path::{Component, Path, PathBuf}; +use std::sync::{Arc, OnceLock}; + +/// 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> { + let root = match access { + Access::Read => self.read.as_ref(), + Access::Write => self.write.as_ref(), + }; + let Some(Root { resolved: folder, shown }) = root else { + return match access { + Access::Read => Err(format!("automation filesystem access is not granted: read authority is absent: {path}")), + Access::Write => Err(format!("automation filesystem access is not granted: write authority is absent: {path}")), + }; + }; + if inside(&resolve(path)?, folder) { + return Ok(()); + } + let root = shown.display(); + match access { + Access::Read => Err(format!("automation path rejected: outside the read root {root}: {path}")), + Access::Write => Err(format!("automation path rejected: outside the write root {root}: {path}")), + } + } +} + +/// Where the roots are in force on a thread ([`confine`], [`unconfined`]). +#[derive(Clone, Debug)] +enum Scope { + Confined(Arc), + Unconfined, +} + +thread_local! { + /// This thread's scope; `None`: the process's roots ([`confine_process`]). + static SCOPE: RefCell> = const { RefCell::new(None) }; +} + +/// The roots in force in every thread outside [`confine`] and [`unconfined`]. +static PROCESS: OnceLock> = OnceLock::new(); + +/// Put `roots` in force in the whole process, for good. `false` when roots were in force already +/// (they stay as they were). +pub fn confine_process(roots: Arc) -> bool { + PROCESS.set(roots).is_ok() +} + +/// Puts the scope it replaced back when dropped (also when `f` panics). +struct Restore(Option); + +impl Drop for Restore { + fn drop(&mut self) { + let previous = self.0.take(); + SCOPE.with(|s| *s.borrow_mut() = previous); + } +} + +fn with_scope(scope: Option, f: impl FnOnce() -> T) -> T { + let _restore = Restore(SCOPE.with(|s| s.replace(scope))); + f() +} + +/// Run `f` with `roots` in force on this thread (`None`: as things stand). +pub fn confine(roots: Option<&Arc>, f: impl FnOnce() -> T) -> T { + match roots { + Some(r) => with_scope(Some(Scope::Confined(r.clone())), f), + None => f(), + } +} + +/// Run `f` with no roots in force on this thread: what the person at the keyboard does, and the +/// app's own folders. +pub fn unconfined(f: impl FnOnce() -> T) -> T { + with_scope(Some(Scope::Unconfined), f) +} + +/// The roots in force on this thread, if any. +pub fn current() -> Option> { + match SCOPE.with(|s| s.borrow().clone()) { + Some(Scope::Confined(r)) => Some(r), + Some(Scope::Unconfined) => None, + None => PROCESS.get().cloned(), + } +} + +/// `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, + } +} + +/// May `path` be read here? Always, without roots in force. +pub fn check_read(path: &str) -> Result<(), String> { + current().map_or(Ok(()), |r| r.check(path, Access::Read)) +} + +/// May `path` be written here? Always, without roots in force. +pub fn check_write(path: &str) -> Result<(), String> { + current().map_or(Ok(()), |r| r.check(path, Access::Write)) +} + +/// `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(path: &str) -> Result { + if path.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::new(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: {path}")); + } + let absolute = std::path::absolute(path).map_err(|e| format!("automation path rejected: cannot resolve {path}: {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: {path}")), + // There, yet its real path can't be had (no permission): nothing to compare. + Ok(_) => return Err(format!("automation path rejected: cannot resolve {path}: {e}")), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => return Err(format!("automation path rejected: cannot resolve {path}: {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: {path}")); + } + // 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 {path}: {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!("dc-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.deckcraft"), "x").unwrap(); + std::fs::write(base.join("outside/secret.deckcraft"), "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.deckcraft")), 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.deckcraft")), 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.deckcraft")), 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.deckcraft")), 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.deckcraft"), "x").unwrap(); + assert!(roots.check(&s(base.join("in2/b.deckcraft")), Access::Read).is_err(), "in2 is not inside in"); + assert!(roots.check(&format!("{}2/c.deckcraft", 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.deckcraft", 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.deckcraft", 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.deckcraft", 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.deckcraft", 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.deckcraft"))).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.deckcraft")), 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.deckcraft"), &base.join("in/leak.deckcraft")) { + 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.deckcraft")), 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.deckcraft"), &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.deckcraft")), 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.deckcraft")), 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.deckcraft")); + assert_eq!(roots.check(&upper, Access::Read), Ok(()), "{upper}"); + assert_eq!(roots.check(&s(base.join("In/Sub/New.deckcraft")), 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); + } + + #[test] + fn scopes_nest_and_stay_on_their_thread() { + let (base, roots) = folders("scopes"); + let outside = s(base.join("outside/secret.deckcraft")); + assert_eq!(check_read(&outside), Ok(()), "nothing is confined by default"); + confine(Some(&roots), || { + assert!(check_read(&outside).is_err()); + assert_eq!(unconfined(|| check_read(&outside)), Ok(())); + assert!(check_read(&outside).is_err(), "restored after unconfined"); + let o = outside.clone(); + let there = std::thread::spawn(move || check_read(&o).is_ok()).join().unwrap(); + assert!(there, "another thread is not confined by this thread's scope"); + }); + assert_eq!(check_read(&outside), Ok(()), "restored after confine"); + let r = std::panic::catch_unwind(|| confine(Some(&roots), || panic!("boom"))); + assert!(r.is_err()); + assert_eq!(check_read(&outside), Ok(()), "restored after a panic"); + assert_eq!(confine(None, || check_read(&outside)), Ok(())); + let _ = std::fs::remove_dir_all(base); + } + + #[test] + fn device_names() { + for d in ["CON", "nul", "Com1", "LPT9.txt", "aux.deckcraft", "NUL "] { + assert!(is_device_name(d), "{d}"); + } + for f in ["console", "com10", "COM0", "lpt", "a.deckcraft", "nul-x"] { + assert!(!is_device_name(f), "{f}"); + } + } + + /// Every file a command reads or writes by path goes through `io` (`read_file`, + /// `write_file`), which check the roots. Engine code that reaches for `std::fs` itself would + /// bypass them, so none may: every file under `src/` (commands, `lib.rs`, `guard.rs`...) + /// except `io.rs`, this module, `recovery.rs` (the recovery folder is set by the host + /// application, never by a command's path, and a headless session has none) and the test + /// files. + #[test] + fn commands_do_not_touch_the_file_system_themselves() { + fn walk(dir: &Path, files: &mut Vec) { + for entry in std::fs::read_dir(dir).unwrap() { + let path = entry.unwrap().path(); + if path.is_dir() { + walk(&path, files); + } else if path.extension().is_some_and(|e| e == "rs") { + files.push(path); + } + } + } + let src = Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); + let mut files = vec![]; + walk(&src, &mut files); + assert!(files.iter().any(|f| f.ends_with("lib.rs")) && files.iter().any(|f| f.ends_with("cmd/file.rs")), "{files:?}"); + let mut offenders = vec![]; + for path in files { + let name = path.file_name().and_then(|n| n.to_str()).unwrap_or(""); + // `io` and this module are the checked gateway; `tests*.rs` are test-only modules. + if path.parent() == Some(src.as_path()) && matches!(name, "io.rs" | "file_access.rs" | "recovery.rs") || name.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("#[cfg(test)]").next().unwrap_or(""); + for (n, line) in code.lines().enumerate() { + let line = line.trim_start(); + let touches = ["std::fs", "File::open", "File::create", "OpenOptions"].iter().any(|w| line.contains(w)); + if !line.starts_with("//") && touches { + offenders.push(format!("{}:{}: {line}", path.display(), n + 1)); + } + } + } + assert!(offenders.is_empty(), "use crate::io::{{read_file, write_file}}: {offenders:#?}"); + } +} diff --git a/crates/engine/src/io.rs b/crates/engine/src/io.rs new file mode 100644 index 00000000..0ae00c3e --- /dev/null +++ b/crates/engine/src/io.rs @@ -0,0 +1,35 @@ +//! Reading and writing the files a command names by path. +//! +//! Every such read and write goes through [`read_file`] and [`write_file`], so the automation +//! roots ([`crate::file_access`]) are checked in one place. A command module that called +//! `std::fs` itself would bypass them; a test in `file_access` keeps that from happening. + +/// The bytes of the file at `path`, inside the automation read root when there is one. +#[cfg(not(target_arch = "wasm32"))] +pub fn read_file(path: &str) -> Result, String> { + crate::file_access::check_read(path)?; + std::fs::read(path).map_err(|e| format!("{path}: {e}")) +} + +#[cfg(target_arch = "wasm32")] +pub fn read_file(path: &str) -> Result, String> { + Err(format!("{path}: files are opened through the browser on the web")) +} + +/// Write `bytes` to the file at `path`, inside the automation write root when there is one. +/// +/// The bytes go to a temporary sibling first and are renamed over `path`, so a failed save never +/// truncates the old file. The sibling is in the same folder, so it is inside the root whenever +/// `path` is. +#[cfg(not(target_arch = "wasm32"))] +pub fn write_file(path: &str, bytes: &[u8]) -> Result<(), String> { + crate::file_access::check_write(path)?; + let tmp = format!("{path}.saving"); + std::fs::write(&tmp, bytes).map_err(|e| format!("{path}: {e}"))?; + std::fs::rename(&tmp, path).map_err(|e| format!("{path}: {e}")) +} + +#[cfg(target_arch = "wasm32")] +pub fn write_file(path: &str, _bytes: &[u8]) -> Result<(), String> { + Err(format!("{path}: saving to a path is not available in the browser")) +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 7e367974..57e131e7 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -11,8 +11,10 @@ pub mod cmd; pub mod connect; pub mod drawtable; pub mod fonts; +pub mod file_access; pub mod guard; pub mod icons; +pub mod io; pub mod keys; pub mod links; pub mod recovery; diff --git a/crates/mcp/src/headless.rs b/crates/mcp/src/headless.rs index a6cea8f9..36a27418 100644 --- a/crates/mcp/src/headless.rs +++ b/crates/mcp/src/headless.rs @@ -1,6 +1,9 @@ //! An in-process engine session that answers the engine-level control-channel methods itself. use deckcraft_engine::{Mods, PointerEvent, PointerKind, Session, ToolKind}; +use std::sync::Arc; + +use deckcraft_engine::file_access::{self, AutomationRoots}; use serde_json::{Value, json}; use crate::backend::Backend; @@ -8,6 +11,7 @@ use crate::backend::Backend; /// Headless backend: a [`Session`] rendering slides on the CPU. pub struct Headless { pub session: Session, + roots: Option>, } impl Default for Headless { @@ -59,12 +63,19 @@ pub fn tool_kind(name: &str, preset: Option<&str>) -> Option { impl Headless { pub fn new() -> Self { - Self { session: Session::new() } + Self { session: Session::new(), roots: None } } /// A session with a fresh presentation. pub fn with_document() -> Self { - Self { session: Session::with_new() } + Self { session: Session::with_new(), roots: None } + } + + /// Confine every file the session's commands read and write to `roots` + /// (`deckcraft-cli mcp --automation-read-root … --automation-write-root …`). + pub fn with_automation_roots(mut self, roots: Option>) -> Self { + self.roots = roots; + self } fn exec(&mut self, id: &str, params: &Value) -> Result { @@ -110,7 +121,7 @@ impl Headless { ); let png = img.to_png_at(deckcraft_engine::render::dpi_for_scale(scale)); if let Some(path) = s(p, "path") { - std::fs::write(path, &png).map_err(|e| format!("{path}: {e}"))?; + deckcraft_engine::io::write_file(path, &png)?; return Ok(json!({"path": path, "width": img.width, "height": img.height})); } Ok(json!({"png": deckcraft_engine::cmd::base64_encode(&png), "width": img.width, "height": img.height, "slide": i})) @@ -119,6 +130,19 @@ impl Headless { impl Backend for Headless { fn call(&mut self, method: &str, params: Value) -> Result { + let roots = self.roots.clone(); + file_access::confine(roots.as_ref(), || self.answer(method, params)) + } + fn has_ui(&self) -> bool { + false + } + fn describe(&self) -> String { + "headless (in-process engine; no window)".into() + } +} + +impl Headless { + fn answer(&mut self, method: &str, params: Value) -> Result { let p = ¶ms; match method { "engine.execute" | "command" | "ui.menu.invoke" => { @@ -152,10 +176,4 @@ impl Backend for Headless { other => Err(format!("unknown method `{other}`")), } } - fn has_ui(&self) -> bool { - false - } - fn describe(&self) -> String { - "headless (in-process engine; no window)".into() - } } diff --git a/crates/mcp/tests/automation_roots.rs b/crates/mcp/tests/automation_roots.rs new file mode 100644 index 00000000..f8595ff9 --- /dev/null +++ b/crates/mcp/tests/automation_roots.rs @@ -0,0 +1,220 @@ +//! `deckcraft-cli mcp --automation-read-root --automation-write-root `: through the MCP +//! protocol, files open and load only from inside the read root and are saved and exported only +//! inside the write root. Paths that leave a root (`..`, another folder, a sibling with the same +//! prefix, a link) are refused, and nothing is written outside. +// Integration tests: unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + +use std::path::{Path, PathBuf}; + +use deckcraft_engine::file_access::AutomationRoots; +use deckcraft_mcp::{Headless, Server}; +use serde_json::{Value, json}; + +/// A 1x1 PNG, so `insert_picture` has 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 fresh folder holding `in` (the read root, with `a.txt` and `pic.png`), `out` (the write root), +/// `outside` (with `secret.txt` and `pic.png`) and `in2`, and a server confined to `in` and `out` +/// (either may be left out). The server starts with a new presentation. +fn confined(tag: &str, read: bool, write: bool) -> (Server, PathBuf) { + let base = std::env::temp_dir().join(format!("dc-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.txt"), "Inside text").unwrap(); + std::fs::write(base.join("in/pic.png"), PNG).unwrap(); + std::fs::write(base.join("outside/secret.txt"), "TOP SECRET").unwrap(); + std::fs::write(base.join("outside/pic.png"), PNG).unwrap(); + std::fs::write(base.join("in2/c.txt"), "Sibling").unwrap(); + 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(); + let mut server = Server::new(Box::new(Headless::with_document().with_automation_roots(roots))); + ok(&mut server, "new_presentation", json!({})); + (server, base) +} + +fn s(p: &Path) -> String { + p.to_string_lossy().into_owned() +} + +/// `tools/call` -> (isError, the first text block). +fn call(server: &mut Server, name: &str, args: Value) -> (bool, String) { + let line = json!({"jsonrpc": "2.0", "id": 1, "method": "tools/call", "params": {"name": name, "arguments": args}}).to_string(); + let reply: Value = serde_json::from_str(&server.handle_line(&line).unwrap()).unwrap(); + let result = &reply["result"]; + let text = result["content"].as_array().unwrap().iter().find_map(|c| c["text"].as_str()).unwrap_or_default().to_string(); + (result["isError"] == true, text) +} + +fn ok(server: &mut Server, name: &str, args: Value) -> String { + let (error, text) = call(server, name, args.clone()); + assert!(!error, "{name} {args}: {text}"); + text +} + +fn refused(server: &mut Server, 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}"); +} + +fn run(server: &mut Server, command: &str, params: Value) -> (bool, String) { + call(server, "run_command", json!({"command": command, "params": params})) +} + +/// Everything the open presentation says, as one string. +fn deck_text(server: &mut Server) -> String { + format!("{} {}", ok(server, "inspect_document", json!({})), ok(server, "inspect_slide", json!({}))) +} + +#[test] +fn presentations_open_only_from_inside_the_read_root() { + let (mut server, base) = confined("open", true, true); + let sep = std::path::MAIN_SEPARATOR; + ok(&mut server, "open_presentation", json!({"path": s(&base.join("in/a.txt"))})); + assert!(deck_text(&mut server).contains("Inside text")); + + refused(&mut server, "open_presentation", json!({"path": s(&base.join("outside/secret.txt"))}), "outside the read root"); + refused( + &mut server, + "open_presentation", + json!({"path": format!("{}{sep}..{sep}outside{sep}secret.txt", s(&base.join("in")))}), + "outside the read root", + ); + refused(&mut server, "open_presentation", json!({"path": s(&base.join("in2/c.txt"))}), "outside the read root"); + // The write root isn't readable: the two kinds of access are separate. + std::fs::write(base.join("out/d.txt"), "d").unwrap(); + refused(&mut server, "open_presentation", json!({"path": s(&base.join("out/d.txt"))}), "outside the read root"); + // The same through the engine command and the control-channel method. + let (error, text) = run(&mut server, "file.open", json!({"path": s(&base.join("outside/secret.txt"))})); + assert!(error && text.contains("outside the read root"), "{text}"); + refused(&mut server, "call_app", json!({"method": "app.open", "params": {"path": s(&base.join("outside/secret.txt"))}}), "outside the read root"); + // Nothing from the refused files reached the deck. + let text = deck_text(&mut server); + assert!(!text.contains("TOP SECRET") && !text.contains("Sibling"), "{text}"); + assert!(text.contains("Inside text"), "the refused opens left the deck it had: {text}"); + let _ = std::fs::remove_dir_all(base); +} + +#[test] +fn pictures_media_and_other_path_reads_are_confined() { + let (mut server, base) = confined("media", true, true); + ok(&mut server, "insert_picture", json!({"path": s(&base.join("in/pic.png"))})); + let before = ok(&mut server, "inspect_slide", json!({})); + refused(&mut server, "insert_picture", json!({"path": s(&base.join("outside/pic.png"))}), "outside the read root"); + for command in ["insert.audio", "insert.video", "insert.picture", "picture.change"] { + let (error, text) = run(&mut server, command, json!({"path": s(&base.join("outside/pic.png"))})); + assert!(error && text.contains("outside the read root"), "{command}: {text}"); + } + assert_eq!(ok(&mut server, "inspect_slide", json!({})), before, "refused reads change nothing"); + + // Media inside the root gets as far as being read: a refusal there would name the root. + for command in ["insert.audio", "insert.video"] { + let (_, text) = run(&mut server, command, json!({"path": s(&base.join("in/pic.png"))})); + assert!(!text.contains("automation"), "{command}: {text}"); + } + let _ = std::fs::remove_dir_all(base); +} + +#[test] +fn every_writing_command_is_refused_outside_the_write_root() { + let (mut server, base) = confined("write", true, true); + let outside = |name: &str| s(&base.join("outside").join(name)); + let inside = |name: &str| s(&base.join("out").join(name)); + ok(&mut server, "add_slide", json!({"title": "Second"})); + + refused(&mut server, "save_presentation", json!({"path": outside("x.deckcraft")}), "outside the write root"); + refused(&mut server, "export", json!({"path": outside("x.pptx")}), "outside the write root"); + refused(&mut server, "export", json!({"path": outside("x.pdf")}), "outside the write root"); + refused(&mut server, "export", json!({"path": outside("x.png")}), "outside the write root"); + refused(&mut server, "export", json!({"path": outside("x.txt")}), "outside the write root"); + refused(&mut server, "export", json!({"path": outside("all.png"), "all": true}), "outside the write root"); + refused(&mut server, "render_slide", json!({"path": outside("r.png")}), "outside the write root"); + refused(&mut server, "call_app", json!({"method": "app.save", "params": {"path": outside("y.deckcraft")}}), "outside the write root"); + refused(&mut server, "call_app", json!({"method": "app.export", "params": {"path": outside("y.pdf")}}), "outside the write root"); + for command in ["file.save", "file.saveAs", "file.export"] { + let (error, text) = run(&mut server, command, json!({"path": outside("z.deckcraft")})); + assert!(error && text.contains("outside the write root"), "{command}: {text}"); + } + // A sibling folder with the same prefix, and `..` out of the root. + refused(&mut server, "save_presentation", json!({"path": s(&base.join("out2/x.deckcraft"))}), "outside the write root"); + let sep = std::path::MAIN_SEPARATOR; + refused( + &mut server, + "save_presentation", + json!({"path": format!("{}{sep}..{sep}outside{sep}x.deckcraft", s(&base.join("out")))}), + "outside the write root", + ); + // The read root isn't writable. + refused(&mut server, "save_presentation", json!({"path": s(&base.join("in/x.deckcraft"))}), "outside the write root"); + + let leftovers: Vec<_> = std::fs::read_dir(base.join("outside")).unwrap().map(|e| e.unwrap().file_name()).collect(); + assert_eq!(leftovers.len(), 2, "nothing was written beside secret.txt and pic.png: {leftovers:?}"); + assert!(!base.join("out2").exists() && !base.join("in/x.deckcraft").exists()); + + // Inside the write root all of it works. + ok(&mut server, "save_presentation", json!({"path": inside("x.deckcraft")})); + ok(&mut server, "export", json!({"path": inside("x.pptx")})); + ok(&mut server, "export", json!({"path": inside("x.pdf")})); + ok(&mut server, "export", json!({"path": inside("all.png"), "all": true})); + ok(&mut server, "render_slide", json!({"path": inside("r.png")})); + for f in ["x.deckcraft", "x.pptx", "x.pdf", "all1.png", "all2.png", "r.png"] { + assert!(base.join("out").join(f).exists(), "{f}"); + } + assert!(!base.join("out/x.deckcraft.saving").exists(), "the temporary file is renamed away"); + let _ = std::fs::remove_dir_all(base); +} + +#[test] +fn one_root_grants_only_its_own_access() { + let (mut read_only, base) = confined("read-only", true, false); + ok(&mut read_only, "open_presentation", json!({"path": s(&base.join("in/a.txt"))})); + refused(&mut read_only, "save_presentation", json!({"path": s(&base.join("out/x.deckcraft"))}), "write authority is absent"); + refused(&mut read_only, "export", json!({"path": s(&base.join("out/x.pdf"))}), "write authority is absent"); + refused(&mut read_only, "render_slide", json!({"path": s(&base.join("out/r.png"))}), "write authority is absent"); + // Calls that return bytes instead of writing a path keep working. + let (error, _) = call(&mut read_only, "render_slide", json!({"scale": 0.25})); + assert!(!error); + assert!(std::fs::read_dir(base.join("out")).unwrap().next().is_none()); + let _ = std::fs::remove_dir_all(base); + + let (mut write_only, base) = confined("write-only", false, true); + refused(&mut write_only, "open_presentation", json!({"path": s(&base.join("in/a.txt"))}), "read authority is absent"); + refused(&mut write_only, "insert_picture", json!({"path": s(&base.join("in/pic.png"))}), "read authority is absent"); + ok(&mut write_only, "save_presentation", json!({"path": s(&base.join("out/x.deckcraft"))})); + assert!(base.join("out/x.deckcraft").exists()); + let _ = std::fs::remove_dir_all(base); +} + +#[cfg(unix)] +#[test] +fn links_out_of_a_root_are_refused() { + let (mut server, base) = confined("links", true, true); + std::os::unix::fs::symlink(base.join("outside/secret.txt"), base.join("in/leak.txt")).unwrap(); + std::os::unix::fs::symlink(base.join("outside"), base.join("out/away")).unwrap(); + std::os::unix::fs::symlink(base.join("outside/gone.deckcraft"), base.join("out/dangling.deckcraft")).unwrap(); + refused(&mut server, "open_presentation", json!({"path": s(&base.join("in/leak.txt"))}), "outside the read root"); + refused(&mut server, "save_presentation", json!({"path": s(&base.join("out/away/x.deckcraft"))}), "outside the write root"); + refused(&mut server, "save_presentation", json!({"path": s(&base.join("out/dangling.deckcraft"))}), "a link that leads nowhere"); + assert!(!base.join("outside/x.deckcraft").exists() && !base.join("outside/gone.deckcraft").exists()); + assert!(!deck_text(&mut server).contains("TOP SECRET")); + let _ = std::fs::remove_dir_all(base); +} + +#[test] +fn without_roots_nothing_is_confined() { + let base = std::env::temp_dir().join(format!("dc-mcp-roots-none-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + std::fs::create_dir_all(&base).unwrap(); + std::fs::write(base.join("a.txt"), "Anywhere").unwrap(); + let mut server = Server::new(Box::new(Headless::with_document().with_automation_roots(AutomationRoots::new(None, None).unwrap()))); + ok(&mut server, "open_presentation", json!({"path": s(&base.join("a.txt"))})); + ok(&mut server, "save_presentation", json!({"path": s(&base.join("b.deckcraft"))})); + assert!(base.join("b.deckcraft").exists()); + let _ = std::fs::remove_dir_all(base); +} diff --git a/docs/mcp.md b/docs/mcp.md index 07284e70..e676f5de 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -13,6 +13,58 @@ deck. } ``` +## Confining file access + +```sh +deckcraft-cli mcp --automation-read-root /work/project --automation-write-root /work/project/out +``` + +```json +{"mcpServers": {"deckcraft": {"command": "deckcraft-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 command reads must lie inside the **read root**: `open_presentation` and + `file.open`, and `insert_picture`, `insert.picture`, `picture.change`, `insert.audio` and + `insert.video` with a `path`. Pictures, audio and video are copied into the deck when they are + inserted, so playing them later reads nothing from disk. +- Every file a command writes must lie inside the **write root**: `save_presentation`, `export` + (every format, and every slide of an image export), `file.save`, `file.saveAs`, `file.export`, + and `render_slide` with a `path`, plus the `.saving` temporary file a save goes through. +- 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). 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 (`\\server\share\...`) 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.pptx` or + `automation filesystem access is not granted: write authority is absent: /work/project/out/x.pptx`. +- The roots must be existing folders; a missing one stops the server with an error. Each flag is + given once (`--flag ` or `--flag=`). + +The flags confine the in-process server, so `--connect` together with them is an error: the server +would otherwise talk to an app that isn't confined. DeckCraft's desktop app has no equivalent flags +yet, so a running app (`deckcraft --control`) is not confined. `--sample` is the operator's own +choice and is opened before the roots apply. + +What the roots do not cover: commands that take no path still read and change the open deck, and +`inspect_document` and `inspect_slide` return it. The system font scan and the app's own recovery +folder are fixed paths, not agent input, and are not checked. Anything that reads or writes a path +goes through one place in the engine (`io::read_file` and `io::write_file`); a test keeps the rest of the +engine from touching the file system on its own. + +The checks hold for every thread of the server process. 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 | What it does |