From 8fb42816324531fc52696aaf04d42629b48a784f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 10 Oct 2026 22:44:01 +0000 Subject: [PATCH] MCP: --automation-read-root / --automation-write-root confine the agent's file access (#500) `effectcraft-cli mcp` had no way to keep an agent's file access to a folder; the flags were unknown options. Both now take a folder (repeatable, as in PhotoCraft): with either given, files are read only under a read or write root and written only under a write root (none: no writes). Defaults are unchanged. automation::confine::Confinement checks every engine command the backend runs (execute_command, command_run, open/save_project, each engine.batch step), in both headless and bridge mode, before it runs: the file parameters of the commands that read or write files must be absolute paths inside the roots (symbolic links resolved through the longest existing ancestor, `..` refused), as must any other absolute path in a command's parameters. Commands that write where they aren't told are checked against the target they would write: Save / Increment and Save the open project's file, renderQueue.render every queued output (renderQueue.list). Scripts, Execute File, Reveal in Finder, Open Recent by index, `$N` file-path references in a batch, render_frame/screenshot paths outside a write root and the bridge's UI-driving tools are refused while confined. docs/mcp.md describes it. Test: a confined headless server opens and saves inside its roots and is refused outside them, for a relative path, `..`, a batch step, a `$N` path, Save to a read-only root's project, the Render Queue's default output there, a symbolic link out of the write root, and scripts; the unconfined server still writes anywhere. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01QiAJeFttZp83Fh7YuzAR5f --- apps/effectcraft-cli/src/main.rs | 17 +- crates/automation/src/backend.rs | 64 +++++-- crates/automation/src/confine.rs | 276 +++++++++++++++++++++++++++++++ crates/automation/src/lib.rs | 1 + crates/automation/src/tests.rs | 71 ++++++++ crates/automation/src/tools.rs | 13 ++ docs/mcp.md | 13 ++ 7 files changed, 437 insertions(+), 18 deletions(-) create mode 100644 crates/automation/src/confine.rs diff --git a/apps/effectcraft-cli/src/main.rs b/apps/effectcraft-cli/src/main.rs index a653b26b..fe523807 100644 --- a/apps/effectcraft-cli/src/main.rs +++ b/apps/effectcraft-cli/src/main.rs @@ -117,6 +117,9 @@ const USAGE: &str = "usage: effectcraft-cli : id or name, '-' = active comp; : id, '#n' or name; : JSON or bare string"; @@ -163,6 +166,9 @@ const VALUED: &[&str] = &[ "--webm-codec", "--audio-bitrate", "--opus-app", + // mcp + "--automation-read-root", + "--automation-write-root", ]; /// Options without a value. Any other `--option` is a usage error. @@ -232,6 +238,10 @@ impl Args { fn flag(&self, k: &str) -> bool { self.opts.iter().any(|(o, _)| o == k) } + /// Every value of a repeatable option, in order. + fn opts(&self, k: &str) -> Vec<&str> { + self.opts.iter().filter(|(o, _)| o == k).filter_map(|(_, v)| v.as_deref()).collect() + } fn opt(&self, k: &str) -> Option<&str> { self.opts.iter().rev().find(|(o, _)| o == k).and_then(|(_, v)| v.as_deref()) } @@ -595,7 +605,12 @@ fn run(cmd: &str, args: &Args, json_out: bool) -> Result<(), Failure> { if args.flag("--autosave") && args.opt("--bridge").is_some() { return usage_err("--autosave is for headless MCP; the bridged app owns its auto-saves"); } - let b = backend(args, false)?; + let mut b = backend(args, false)?; + let (read, write) = (args.opts("--automation-read-root"), args.opts("--automation-write-root")); + if !read.is_empty() || !write.is_empty() { + let roots = |r: Vec<&str>| r.into_iter().map(std::path::PathBuf::from).collect::>(); + b.confine(effectcraft_automation::confine::Confinement::new(&roots(read), &roots(write))?); + } let server = McpServer::new(b); let mut server = if args.flag("--autosave") { let root = diff --git a/crates/automation/src/backend.rs b/crates/automation/src/backend.rs index 685c978f..c3947c87 100644 --- a/crates/automation/src/backend.rs +++ b/crates/automation/src/backend.rs @@ -4,9 +4,16 @@ use effectcraft_engine::Session; use effectcraft_engine::time::Tick; use serde_json::{Value, json}; +use crate::confine::Confinement; use crate::{BridgeClient, Error, Result, base64, encode_png, recompress_png}; -pub enum Backend { +pub struct Backend { + target: Target, + /// File access confined to folders (`--automation-read-root` / `--automation-write-root`). + confinement: Option, +} + +enum Target { /// An in-process session (no window): engine commands, inspection and rendering. Headless(Box), /// A running desktop app reached over its control channel: everything, plus the live UI. @@ -24,43 +31,66 @@ pub struct Frame { impl Backend { pub fn headless(session: Session) -> Self { - Backend::Headless(Box::new(session)) + Backend { target: Target::Headless(Box::new(session)), confinement: None } } /// Bridge to the app's control channel at `addr` (`9877` or `127.0.0.1:9877`). pub fn bridge(addr: &str) -> Result { - Ok(Backend::Bridge(BridgeClient::new(addr)?)) + Ok(Backend { target: Target::Bridge(BridgeClient::new(addr)?), confinement: None }) + } + + /// Confine the files the commands run here may read and write ([`Confinement`]). + pub fn confine(&mut self, confinement: Confinement) { + self.confinement = Some(confinement); + } + + pub fn confinement(&self) -> Option<&Confinement> { + self.confinement.as_ref() } pub fn is_bridge(&self) -> bool { - matches!(self, Backend::Bridge(_)) + matches!(self.target, Target::Bridge(_)) } pub fn session(&mut self) -> Option<&mut Session> { - match self { - Backend::Headless(s) => Some(s), - Backend::Bridge(_) => None, + match &mut self.target { + Target::Headless(s) => Some(s), + Target::Bridge(_) => None, } } - /// Run an engine command by id. + /// Run an engine command by id (checked against the [`Confinement`], if any). pub fn exec(&mut self, id: &str, params: Value) -> Result { let params = if params.is_null() { json!({}) } else { params }; - match self { - Backend::Headless(s) => { + if let Some(c) = self.confinement.clone() { + let lookup = match c.check_command(id, ¶ms)? { + Some(crate::confine::Lookup::ProjectFile) => Some((crate::confine::Lookup::ProjectFile, self.run("project.summary", json!({}))?)), + Some(crate::confine::Lookup::RenderOutputs) => Some((crate::confine::Lookup::RenderOutputs, self.run("renderQueue.list", json!({}))?)), + None => None, + }; + if let Some((lookup, found)) = lookup { + c.check_lookup(&lookup, &found)?; + } + } + self.run(id, params) + } + + fn run(&mut self, id: &str, params: Value) -> Result { + match &mut self.target { + Target::Headless(s) => { let r = s.execute_checked(id, params); s.drain_events(); Ok(r?) } - Backend::Bridge(b) => b.call("engine.execute", json!({"command": id, "params": params})), + Target::Bridge(b) => b.call("engine.execute", json!({"command": id, "params": params})), } } /// Call a raw control-channel method (bridge only). pub fn control(&mut self, method: &str, params: Value) -> Result { - match self { - Backend::Bridge(b) => b.call(method, if params.is_null() { json!({}) } else { params }), - Backend::Headless(_) => Err(Error::BadArgs(NEED_BRIDGE.into())), + match &mut self.target { + Target::Bridge(b) => b.call(method, if params.is_null() { json!({}) } else { params }), + Target::Headless(_) => Err(Error::BadArgs(NEED_BRIDGE.into())), } } @@ -73,15 +103,15 @@ impl Backend { /// [`Backend::render`]; `transparent` keeps the frame's alpha instead of compositing it over /// the comp's background colour. pub fn render_with(&mut self, comp: Option<&Value>, time: Option, max_side: u32, transparent: bool) -> Result { - match self { - Backend::Headless(s) => { + match &mut self.target { + Target::Headless(s) => { let cid = s.resolve_comp(comp).map_err(|_| no_comp(comp))?; let t = time.map(Tick::from_seconds_f64).unwrap_or_else(|| s.time()); let (w, h, rgba) = s.render_rgba8_alpha(cid, t, max_side, transparent)?; let png = encode_png(w, h, rgba, max_side)?; Ok(Frame { png, comp: json!(cid.0), time: t.seconds(), width: w, height: h }) } - Backend::Bridge(b) => { + Target::Bridge(b) => { let mut p = json!({"max_side": max_side, "base64": true}); if transparent { p["transparent"] = json!(true); diff --git a/crates/automation/src/confine.rs b/crates/automation/src/confine.rs new file mode 100644 index 00000000..f396b3da --- /dev/null +++ b/crates/automation/src/confine.rs @@ -0,0 +1,276 @@ +//! File access confinement for automation (`effectcraft-cli mcp --automation-read-root DIR +//! --automation-write-root DIR`, as in PhotoCraft): an agent may only read files under a read +//! root (or a write root) and only write under a write root. +//! +//! Every engine command an agent runs ([`crate::Backend::exec`], also each step of an +//! `engine.batch`) is checked before it runs: its file parameters must be absolute paths inside +//! the roots (symbolic links resolved, no `..`), any other absolute path in its parameters too, +//! and commands that write to a file they are not given (Save, Increment and Save, rendering the +//! Render Queue) are checked against where they would write. Commands that run code or open +//! other applications (scripts, Execute File, Reveal in Finder…) are refused while confined. + +use std::path::{Component, Path, PathBuf}; + +use serde_json::Value; + +use crate::{Error, Result}; + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Access { + Read, + Write, +} + +/// The folders an agent may read and write. +#[derive(Clone, Debug)] +pub struct Confinement { + read: Vec, + write: Vec, +} + +/// Commands that run code, act outside the project or open other applications. +const REFUSED: &[&str] = &[ + "script.run", + "file.runScript", + "file.installScript", + "file.installScriptUIPanel", + "file.uninstallScript", + "file.executeFile", + "file.revealInFinder", + "layer.openSource", + "help.revealLogFile", + "file.autoSave", +]; + +/// Commands that read files named by these parameters. +const READS: &[(&str, &[&str])] = &[ + ("file.open", &["path"]), + ("file.openRecent", &["path"]), + ("file.import", &["path", "paths"]), + ("file.importMultiple", &["paths"]), + ("file.importRecent", &["path"]), + ("file.importLottie", &["path"]), + ("file.importTimeline", &["path"]), + ("file.replaceFootage", &["path"]), + ("file.relinkFootage", &["folder"]), + ("file.setProxy", &["path"]), + ("file.watchFolder", &["folder"]), + ("anim.applyPreset", &["path"]), + ("anim.applyRecentPreset", &["path"]), + ("keys.rpfCameraImport", &["path"]), + ("essential.importTemplate", &["path"]), + ("view.importGuides", &["path"]), + ("shortcuts.import", &["path"]), + ("effect.plugins.load", &["path", "folder"]), + ("mediaBrowser.go", &["path"]), + ("mediaBrowser.addFavorite", &["path"]), +]; + +/// Commands that write files named by these parameters. +const WRITES: &[(&str, &[&str])] = &[ + ("file.save", &["path"]), + ("file.saveAs", &["path"]), + ("file.saveCopy", &["path"]), + ("file.saveCopyAsXml", &["path"]), + ("file.collectFiles", &["folder"]), + ("file.exportLottie", &["path"]), + ("comp.saveFrameAs", &["path"]), + ("comp.saveFrameAsPsd", &["path"]), + ("comp.saveFrameAsExr", &["path"]), + ("anim.savePreset", &["path"]), + ("view.exportGuides", &["path"]), + ("render.saveCurrentPreview", &["path"]), + ("render.preRender", &["output"]), + ("essential.exportTemplate", &["path"]), + ("shortcuts.export", &["path"]), + ("renderQueue.add", &["output", "path"]), +]; + +/// Commands that write where they are not told: the project's file, the Render Queue's outputs. +const IMPLICIT_WRITES: &[&str] = &["file.save", "file.incrementAndSave", "renderQueue.render", "render.preRender"]; + +/// What must be looked up before an implicitly writing command may run. +#[derive(Debug, PartialEq, Eq)] +pub enum Lookup { + /// The open project's file (`project.summary` → `path`). + ProjectFile, + /// Every output of the queued Render Queue items (`renderQueue.list`). + RenderOutputs, +} + +impl Confinement { + /// Roots that must exist (each resolved to its real path). With no write root nothing may be + /// written; write roots may also be read. + pub fn new(read: &[PathBuf], write: &[PathBuf]) -> Result { + let real = |p: &PathBuf| p.canonicalize().map_err(|e| Error::BadArgs(format!("automation root {}: {e}", p.display()))); + let read = read.iter().map(real).collect::>>()?; + let write = write.iter().map(real).collect::>>()?; + Ok(Self { read, write }) + } + + fn roots(&self, access: Access) -> impl Iterator { + let read = if access == Access::Read { &self.read[..] } else { &[] }; + read.iter().chain(&self.write) + } + + /// `path` resolved, if it is inside the roots for `access`. + pub fn check(&self, path: &str, access: Access) -> Result { + let denied = |why: &str| { + let kind = if access == Access::Read { "read" } else { "write" }; + let roots: Vec = self.roots(access).map(|r| r.display().to_string()).collect(); + let roots = if roots.is_empty() { "none".to_string() } else { roots.join(", ") }; + Error::BadArgs(format!("cannot {kind} `{path}`: {why} (automation {kind} roots: {roots})")) + }; + let p = Path::new(path); + if !p.is_absolute() { + return Err(denied("give an absolute path")); + } + if p.components().any(|c| matches!(c, Component::ParentDir)) { + return Err(denied("`..` is not allowed")); + } + let real = resolve(p).ok_or_else(|| denied("it can't be resolved"))?; + if self.roots(access).any(|r| real.starts_with(r)) { Ok(real) } else { Err(denied("it is outside the automation roots")) } + } + + /// Check engine command `id` with `params` before it runs. Returns what must still be looked + /// up and passed to [`Confinement::check_lookup`] for commands that write implicitly. + pub fn check_command(&self, id: &str, params: &Value) -> Result> { + if REFUSED.contains(&id) { + return Err(Error::BadArgs(format!("`{id}` is not available while file access is confined (--automation-read-root/--automation-write-root)"))); + } + if id == "engine.batch" { + for step in params.get("steps").and_then(Value::as_array).into_iter().flatten() { + let Some(inner) = step.get("command").or_else(|| step.get("id")).and_then(Value::as_str) else { continue }; + let inner_params = step.get("params").cloned().unwrap_or(Value::Null); + let named = keys_of(READS, inner).or_else(|| keys_of(WRITES, inner)).unwrap_or(&[]); + // A step's `$N` reference is resolved only as the batch runs: never for a file. + if named.iter().filter_map(|k| inner_params.get(*k)).any(has_reference) { + return Err(Error::BadArgs(format!("`{inner}` in a batch: file paths can't be `$N` references while file access is confined"))); + } + if self.check_command(inner, &inner_params)?.is_some() { + return Err(Error::BadArgs(format!("`{inner}` writes to its project's or queue's files: run it on its own while file access is confined"))); + } + } + return Ok(None); + } + let (named, access) = match (keys_of(WRITES, id), keys_of(READS, id)) { + (Some(k), _) => (k, Access::Write), + (None, Some(k)) => (k, Access::Read), + (None, None) if id.starts_with("renderQueue.") || id == "prefs.set" => (&[][..], Access::Write), + (None, None) => (&[][..], Access::Read), + }; + let mut given = false; + for key in named { + let Some(v) = params.get(*key).filter(|v| !v.is_null()) else { continue }; + given = true; + let paths: Vec<&Value> = match v { + Value::Array(a) => a.iter().collect(), + v => vec![v], + }; + for p in paths { + // A Render Queue output may name a template instead of a file. + match p.as_str() { + Some(s) if id == "render.preRender" && !looks_like_path(s) => {} + Some(s) => { + self.check(s, access)?; + } + None => return Err(Error::BadArgs(format!("`{id}`: `{key}` must be a path"))), + } + } + } + // Any other absolute path the command is given. + let mut other = vec![]; + absolute_strings(params, named, &mut other); + for s in other { + self.check(&s, access)?; + } + // An entry of a recent list by index is a path the agent didn't give. + if matches!(id, "file.openRecent" | "file.importRecent" | "anim.applyRecentPreset") && !given { + return Err(Error::BadArgs(format!("`{id}`: give `path` while file access is confined"))); + } + if IMPLICIT_WRITES.contains(&id) && !(given && id == "file.save") { + return Ok(Some(if id == "file.save" || id == "file.incrementAndSave" { Lookup::ProjectFile } else { Lookup::RenderOutputs })); + } + Ok(None) + } + + /// Check what a [`Lookup`] found: `project.summary` or `renderQueue.list`'s result. + pub fn check_lookup(&self, lookup: &Lookup, found: &Value) -> Result<()> { + match lookup { + Lookup::ProjectFile => match found.get("path").and_then(Value::as_str) { + // Increment and Save writes beside it. + Some(p) => self.check(p, Access::Write).map(drop), + None => Err(Error::BadArgs("the project has no file yet: give `path` (Save As) inside an automation write root".into())), + }, + Lookup::RenderOutputs => { + let items = found.get("items").or(Some(found)).and_then(Value::as_array).cloned().unwrap_or_default(); + for it in items.iter().filter(|it| it.get("render").and_then(Value::as_bool) != Some(false)) { + let extra = it.get("extraOutputs").and_then(Value::as_array).into_iter().flatten().map(|o| o.get("outputPath")); + for p in std::iter::once(it.get("outputPath")).chain(extra).chain([it.get("logPath")]).flatten() { + if let Some(p) = p.as_str() { + self.check(p, Access::Write)?; + } + } + } + Ok(()) + } + } + } +} + +fn keys_of(table: &[(&str, &'static [&'static str])], id: &str) -> Option<&'static [&'static str]> { + table.iter().find(|(c, _)| *c == id).map(|(_, k)| *k) +} + +fn has_reference(v: &Value) -> bool { + match v { + Value::String(s) => s.starts_with('$'), + Value::Array(a) => a.iter().any(has_reference), + _ => false, + } +} + +/// A file path rather than a template name: has a separator or an extension. +fn looks_like_path(s: &str) -> bool { + s.contains(['/', '\\']) || Path::new(s).extension().is_some() +} + +/// Strings in `v` (recursively, except under the top-level `skip` keys) that are absolute paths, +/// `~` paths or `file:` URLs. +fn absolute_strings(v: &Value, skip: &[&str], out: &mut Vec) { + fn walk(v: &Value, out: &mut Vec, depth: usize) { + if depth > 64 { + return; + } + match v { + Value::String(s) => { + let t = s.trim(); + let drive = t.len() > 2 && t.as_bytes().get(1) == Some(&b':') && t.as_bytes().first().is_some_and(u8::is_ascii_alphabetic); + if t.starts_with('/') || t.starts_with('\\') || t.starts_with('~') || drive || t.to_ascii_lowercase().starts_with("file:") { + out.push(t.to_string()); + } + } + Value::Array(a) => a.iter().for_each(|x| walk(x, out, depth + 1)), + Value::Object(o) => o.values().for_each(|x| walk(x, out, depth + 1)), + _ => {} + } + } + match v { + Value::Object(o) => o.iter().filter(|(k, _)| !skip.contains(&k.as_str())).for_each(|(_, x)| walk(x, out, 0)), + v => walk(v, out, 0), + } +} + +/// `p` with symbolic links resolved: its longest existing ancestor's real path plus the rest +/// (which doesn't exist yet, so can't be a link). +fn resolve(p: &Path) -> Option { + let mut rest = vec![]; + let mut at = p; + loop { + if let Ok(real) = at.canonicalize() { + return Some(rest.iter().rev().fold(real, |acc: PathBuf, c| acc.join(c))); + } + rest.push(at.file_name()?.to_os_string()); + at = at.parent()?; + } +} diff --git a/crates/automation/src/lib.rs b/crates/automation/src/lib.rs index 6a673309..a755e0dc 100644 --- a/crates/automation/src/lib.rs +++ b/crates/automation/src/lib.rs @@ -19,6 +19,7 @@ mod autosave; pub mod backend; pub mod base64; pub mod bridge; +pub mod confine; mod long_job; pub mod server; pub mod tools; diff --git a/crates/automation/src/tests.rs b/crates/automation/src/tests.rs index 51d15694..37de10a5 100644 --- a/crates/automation/src/tests.rs +++ b/crates/automation/src/tests.rs @@ -1181,3 +1181,74 @@ fn command_tools_accept_id_as_an_alias() { let (_, err) = call(&mut s, "execute_command", json!({})); assert!(err, "neither key is an error"); } + +/// #500: `--automation-read-root` / `--automation-write-root` confine the files an agent reads +/// and writes: inside the roots works, outside (or relative, `..`, through a symbolic link, by +/// a batch step, by Save to the project's own file, from a script) is refused. +#[test] +fn automation_roots_confine_file_access() { + use crate::confine::Confinement; + let base = std::env::temp_dir().join(format!("ec-mcp-confine-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + let (read, write, outside) = (base.join("in"), base.join("out"), base.join("elsewhere")); + for d in [&read, &write, &outside] { + std::fs::create_dir_all(d).unwrap(); + } + let p = |d: &std::path::Path, f: &str| d.join(f).to_string_lossy().into_owned(); + // Unconfined (the default): anywhere. + let mut s = server(); + call_json(&mut s, "execute_command", json!({"command": "comp.new", "params": {"name": "A", "width": 64, "height": 64}})); + call_json(&mut s, "save_project", json!({"path": p(&outside, "free.ecproj")})); + call_json(&mut s, "save_project", json!({"path": p(&read, "source.ecproj")})); + + let mut s = server(); + s.backend().confine(Confinement::new(std::slice::from_ref(&read), std::slice::from_ref(&write)).unwrap()); + let denied = |s: &mut McpServer, name: &str, args: Value| { + let (c, err) = call(s, name, args.clone()); + assert!(err, "{name} {args} should be refused: {c:?}"); + c[0]["text"].as_str().unwrap_or_default().to_string() + }; + // Reading: inside a read root, not outside. + call_json(&mut s, "open_project", json!({"path": p(&read, "source.ecproj")})); + let msg = denied(&mut s, "open_project", json!({"path": p(&outside, "free.ecproj")})); + assert!(msg.contains("outside the automation roots"), "{msg}"); + // Writing: inside a write root only; not into a read root, not relative, not with `..`. + call_json(&mut s, "save_project", json!({"path": p(&write, "saved.ecproj")})); + assert!(write.join("saved.ecproj").is_file()); + denied(&mut s, "save_project", json!({"path": p(&read, "copy.ecproj")})); + denied(&mut s, "save_project", json!({"path": "relative.ecproj"})); + denied(&mut s, "save_project", json!({"path": p(&write, "../elsewhere/up.ecproj")})); + denied(&mut s, "execute_command", json!({"command": "comp.saveFrameAs", "params": {"path": p(&outside, "f.png")}})); + // Any other absolute path a command is given. + denied(&mut s, "execute_command", json!({"command": "renderQueue.add", "params": {"output": p(&outside, "movie.mp4")}})); + // A batch step. + let steps = json!([{"command": "comp.new", "params": {"name": "B"}}, {"command": "file.saveAs", "params": {"path": p(&outside, "b.ecproj")}}]); + denied(&mut s, "batch", json!({"steps": steps})); + let steps = json!([{"command": "file.saveAs", "params": {"path": "$1.path"}}]); + denied(&mut s, "batch", json!({"steps": steps})); + // Save (and Increment and Save) write the open project's own file: it must be writable. + call_json(&mut s, "open_project", json!({"path": p(&read, "source.ecproj")})); + denied(&mut s, "save_project", json!({})); + denied(&mut s, "execute_command", json!({"command": "file.incrementAndSave"})); + // Rendering the queue writes its items' outputs (by default beside the project). + call_json(&mut s, "execute_command", json!({"command": "renderQueue.add", "params": {"comp": "A"}})); + let queue = call_json(&mut s, "execute_command", json!({"command": "renderQueue.list"})); + assert!(queue["items"][0]["outputPath"].as_str().is_some_and(|o| o.starts_with(&*read.to_string_lossy())), "{queue}"); + let msg = denied(&mut s, "execute_command", json!({"command": "renderQueue.render"})); + assert!(msg.contains("cannot write"), "{msg}"); + call_json(&mut s, "open_project", json!({"path": p(&write, "saved.ecproj")})); + call_json(&mut s, "save_project", json!({})); + // Through a symbolic link out of a root. + #[cfg(unix)] + { + std::os::unix::fs::symlink(&outside, write.join("link")).unwrap(); + denied(&mut s, "save_project", json!({"path": p(&write.join("link"), "escaped.ecproj")})); + assert!(!outside.join("escaped.ecproj").exists()); + } + // Scripts can read and write anything: refused. + denied(&mut s, "run_script", json!({"code": "1 + 1"})); + denied(&mut s, "execute_command", json!({"command": "file.runScript", "params": {"path": p(&read, "x.jsx")}})); + // Writes to the files the agent named: none outside. + assert_eq!(std::fs::read_dir(&outside).unwrap().count(), 1, "only the unconfined save"); + let _ = std::fs::remove_dir_all(&base); +} diff --git a/crates/automation/src/tools.rs b/crates/automation/src/tools.rs index 12ea3167..fa86095b 100644 --- a/crates/automation/src/tools.rs +++ b/crates/automation/src/tools.rs @@ -103,12 +103,19 @@ pub fn available(bridge: bool) -> impl Iterator { TOOLS.iter().filter(move |t| bridge || !t.bridge_only) } +/// Tools refused while file access is confined: they drive the live app like a person (menus, +/// shortcuts, Open Recent…) or call control-channel methods directly, past the command checks. +const UNCONFINED_ONLY: &[&str] = &["control", "ui_click", "ui_drag", "ui_key", "ui_type", "ui_set", "script_ui"]; + /// Run a tool by name. pub fn run(b: &mut Backend, name: &str, args: &Value) -> Result { let t = find(name).ok_or_else(|| Error::BadArgs(format!("unknown tool `{name}`")))?; if t.bridge_only && !b.is_bridge() { return Err(Error::BadArgs(NEED_BRIDGE.into())); } + if b.confinement().is_some() && UNCONFINED_ONLY.contains(&name) { + return Err(Error::BadArgs(format!("`{name}` is not available while file access is confined (--automation-read-root/--automation-write-root)"))); + } let empty = json!({}); (t.run)(b, if args.is_null() { &empty } else { args }) } @@ -389,6 +396,9 @@ fn render_frame(b: &mut Backend, a: &Value) -> Result { let f = b.render_with(get(a, "comp"), get(a, "time").and_then(Value::as_f64), max_side, transparent)?; let mut info = json!({"comp": f.comp, "time": f.time, "width": f.width, "height": f.height}); if let Some(p) = get(a, "path").and_then(Value::as_str) { + if let Some(c) = b.confinement() { + c.check(p, crate::confine::Access::Write)?; + } std::fs::write(p, &f.png).map_err(|e| Error::Other(format!("cannot write {p}: {e}")))?; info["path"] = json!(p); } @@ -482,6 +492,9 @@ fn batch_tool(b: &mut Backend, a: &Value) -> Result { fn screenshot(b: &mut Backend, a: &Value) -> Result { let max_side = get(a, "max_side").and_then(Value::as_u64).unwrap_or(1600) as u32; let keep = get(a, "path").and_then(Value::as_str).map(str::to_string); + if let (Some(c), Some(p)) = (b.confinement(), &keep) { + c.check(p, crate::confine::Access::Write)?; + } let path = keep.clone().unwrap_or_else(|| std::env::temp_dir().join(format!("effectcraft-mcp-shot-{}.png", std::process::id())).to_string_lossy().into_owned()); b.control("ui.screenshot", obj(&[("path", Some(&json!(path))), ("panel", get(a, "panel")), ("id", get(a, "id"))]))?; diff --git a/docs/mcp.md b/docs/mcp.md index 0d7f83e9..1104927a 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -3,6 +3,19 @@ Run `effectcraft-cli mcp` for a headless session, or `effectcraft-cli mcp --bridge 9877` to use the desktop app's control channel. Existing ports are unchanged. +## Confining file access + +`--automation-read-root DIR` and `--automation-write-root DIR` (each repeatable, as in +PhotoCraft) confine what the agent can reach: files are read only under a read or write root and +written only under a write root; with no write root nothing is written. Every engine command +(`execute_command`, `command_run`, each `batch` step) is checked before it runs: its file +parameters must be absolute paths inside the roots (symbolic links resolved, no `..`), and so must +any other absolute path in its parameters. Save, Increment and Save and rendering the Render +Queue are checked against the files they write (the open project's, the queued outputs). Scripts +(`run_script`, `file.runScript`…), Execute File, Reveal in Finder, `$N` references to file paths +in a batch, and the bridge's UI-driving tools (`ui_click`, `ui_key`, `ui_type`, `ui_drag`, +`ui_set`, `script_ui`, `control`) are refused while confined. Without the flags nothing changes. + The server provides the same core conventions as FilmCraft #28: | Tool | Arguments | Result |