Merge PR #247 from isaac-levine: MCP: confine file access with --automation-read-root and --automation-write-root

# Conflicts:
#	apps/deckcraft-cli/src/main.rs
#	crates/engine/src/cmd/file.rs
#	crates/engine/src/lib.rs

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<PathBuf>,
+    pub write_root: Option<PathBuf>,
+    read_given: bool,
+    write_given: bool,
+}
+
+impl AutomationArgs {
+    pub fn new(read_root: Option<PathBuf>, write_root: Option<PathBuf>) -> 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<Item = String>) -> Result<bool, String> {
+        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}=<dir> 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()));
+    }
+}

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<String> = 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<dyn Backend> = 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)
         }
     };