collective
Browse / github.com/storytold/effectcraft / patch
automation: a command that writes a file it is given is checked as a write (#2131 F-11)
automation: a command that writes a file it is given is checked as a write (#2131 F-11) Part of #2131 — the "Unlisted writers are checked as reads" item. The others (relative paths, indirect footage reads, `window.scriptPanel`, the depth-64 walk) are separate; the script one is #2521. ## The breach A command in neither `READS` nor `WRITES` falls to the catch-all that checks every absolute path in its parameters — as a **read** (`confine.rs:156-186`). Two of them write. `file.exportTimeline` writes the timeline file, and the pre-rendered ProRes movies of the layers the format can't express into the same folder (`cmd/timeline.rs:167,177`). Before this change, with `--automation-
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- Anubhav-Rai (imported from pull request #2522, open)
- Base
bc4b48f67deef2458e9dd3e3b0e592d5f75768a1- Size
- +76 −24 in 2 files
- Review
- pending
- Shared
- 2026-10-28
- Id
a965f9149608fda0e3b68724
How it went
No other operator has reported using it yet.
Diff
raw · crates/automation/src/confine.rs crates/automation/src/tests.rs
@@ -4,7 +4,8 @@ //! //! 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,+//! the roots (symbolic links resolved, no `..`) — the ones it writes inside a write root, and a+//! command can do both — 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.@@ -50,6 +51,7 @@ const READS: &[(&str, &[&str])] = &[ ("file.importMultiple", &["paths"]), ("file.importRecent", &["path"]), ("file.importLottie", &["path"]),+ ("templates.create", &["path"]), ("file.importTimeline", &["path"]), ("file.replaceFootage", &["path"]), ("file.relinkFootage", &["folder"]),@@ -72,6 +74,12 @@ const WRITES: &[(&str, &[&str])] = &[ ("file.saveAs", &["path"]), ("file.saveCopy", &["path"]), ("file.saveCopyAsXml", &["path"]),+ // Writes the timeline file, and the pre-rendered movies of the layers the format can't+ // express into the same folder.+ ("file.exportTimeline", &["path"]),+ // Saves the new project (`file.saveAs`, from inside the command) and extracts the template's+ // footage; with neither given, the footage goes into the settings store.+ ("templates.create", &["projectPath", "footageDir"]), ("file.collectFiles", &["folder"]), ("file.exportLottie", &["path"]), ("comp.saveFrameAs", &["path"]),@@ -142,7 +150,7 @@ impl Confinement { 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(&[]);+ let named = [keys_of(WRITES, inner).unwrap_or_default(), keys_of(READS, inner).unwrap_or_default()].concat(); // 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")));@@ -153,36 +161,37 @@ impl Confinement { } 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 writes = keys_of(WRITES, id).unwrap_or_default();+ let reads = keys_of(READS, id).unwrap_or_default();+ // Anything the command is given that the tables don't name: a write for a command that+ // writes, a read otherwise.+ let rest = if writes.is_empty() && !(id.starts_with("renderQueue.") || id == "prefs.set") { Access::Read } else { Access::Write }; 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)?;+ for (keys, access) in [(writes, Access::Write), (reads, Access::Read)] {+ for key in keys {+ 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"))), }- 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);+ absolute_strings(params, &[writes, reads].concat(), &mut other); for s in other {- self.check(&s, access)?;+ self.check(&s, rest)?; } // 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 {@@ -1380,3 +1380,46 @@ fn automation_roots_confine_file_access() { assert_eq!(std::fs::read_dir(&outside).unwrap().count(), 1, "only the unconfined save"); let _ = std::fs::remove_dir_all(&base); }++/// #2131 (audit F-11, "Unlisted writers are checked as reads"): a command that writes a file it+/// is given belongs in `WRITES`, even when it also reads one. Exporting a timeline and creating+/// a project from a template wrote into a read-only root, because an unlisted path is checked as+/// a read.+#[test]+fn automation_roots_check_an_unlisted_writer_as_a_write() {+ use crate::confine::Confinement;+ let base = std::env::temp_dir().join(format!("ec-mcp-writers-{}", std::process::id()));+ let _ = std::fs::remove_dir_all(&base);+ let (read, write) = (base.join("in"), base.join("out"));+ for d in [&read, &write] {+ std::fs::create_dir_all(d).unwrap();+ }+ let p = |d: &std::path::Path, f: &str| d.join(f).to_string_lossy().into_owned();+ let mut s = server();+ s.backend().confine(Confinement::new(std::slice::from_ref(&read), std::slice::from_ref(&write)).unwrap());+ let refused = |s: &mut McpServer, args: Value| {+ let (c, err) = call(s, "execute_command", args.clone());+ assert!(err, "{args} should be refused: {c:?}");+ c[0]["text"].as_str().unwrap_or_default().to_string()+ };+ call_json(&mut s, "execute_command", json!({"command": "comp.new", "params": {"name": "A", "width": 64, "height": 64}}));+ // File ▸ Export ▸ Premiere Pro Project: the timeline file, and any pre-render beside it.+ let msg = refused(&mut s, json!({"command": "file.exportTimeline", "params": {"comp": "A", "path": p(&read, "t.xml")}}));+ assert!(msg.contains("cannot write"), "{msg}");+ call_json(&mut s, "execute_command", json!({"command": "file.exportTimeline", "params": {"comp": "A", "path": p(&write, "t.xml")}}));+ assert!(write.join("t.xml").is_file(), "the export inside the write root went through");+ assert_eq!(std::fs::read_dir(&read).unwrap().count(), 0, "nothing was written into the read root");+ // New Project from Template saves the project itself (`file.saveAs` from inside the command,+ // where the confinement no longer sees it) and extracts the template's footage.+ let msg = refused(&mut s, json!({"command": "templates.create", "params": {"id": "title-card", "projectPath": p(&read, "t.ecproj")}}));+ assert!(msg.contains("cannot write"), "{msg}");+ let args = json!({"command": "templates.create", "params": {"id": "title-card", "projectPath": p(&write, "t.ecproj"), "footageDir": p(&read, "f")}});+ assert!(refused(&mut s, args).contains("cannot write"));+ call_json(&mut s, "execute_command", json!({"command": "templates.create", "params": {"id": "title-card", "projectPath": p(&write, "t.ecproj")}}));+ assert!(write.join("t.ecproj").is_file(), "the project inside the write root went through");+ // The template it reads is still a read: a read root is enough for `path`, and refusing it+ // for another reason means the confinement let it by.+ let msg = refused(&mut s, json!({"command": "templates.create", "params": {"path": p(&read, "none.ectemplate")}}));+ assert!(!msg.contains("automation"), "a template in a read root is readable: {msg}");+ let _ = std::fs::remove_dir_all(&base);+}