Merge pull request #2520 from Anubhav-Rai/automation-default-output

automation: a confined agent can't queue a render output outside the roots (#2479)

crates/automation/src/backend.rs

@@ -4,7 +4,7 @@ use effectcraft_engine::Session;
 use effectcraft_engine::time::Tick;
 use serde_json::{Value, json};
 
-use crate::confine::Confinement;
+use crate::confine::{Confinement, Lookup};
 use crate::{BridgeClient, Error, Result, base64, encode_png, recompress_png};
 
 pub struct Backend {
@@ -63,12 +63,11 @@ impl Backend {
     pub fn exec(&mut self, id: &str, params: Value) -> Result<Value> {
         let params = if params.is_null() { json!({}) } else { params };
         if let Some(c) = self.confinement.clone() {
-            let lookup = match c.check_command(id, &params)? {
-                Some(l @ (crate::confine::Lookup::ProjectFile | crate::confine::Lookup::QueueDefault)) => Some((l, 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 {
+            for lookup in c.check_command(id, &params)? {
+                let found = match lookup {
+                    Lookup::ProjectFile | Lookup::QueueDefault => self.run("project.summary", json!({}))?,
+                    Lookup::RenderOutputs => self.run("renderQueue.list", json!({}))?,
+                };
                 c.check_lookup(&lookup, &found)?;
             }
         }

crates/automation/src/confine.rs

@@ -6,10 +6,11 @@
 //! `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, ScriptUI panels and the events that reach them, Execute File,
-//! Reveal in Finder…) are refused while confined, as is granting scripts the file and network
-//! access the user withheld.
+//! Render Queue) are checked against where they would write. Queueing a render without naming
+//! its output needs a saved project, because the default output of an unsaved one is the home
+//! folder. Commands that run code or open other applications (scripts, ScriptUI panels and the
+//! events that reach them, Execute File, Reveal in Finder…) are refused while confined, as is
+//! granting scripts the file and network access the user withheld.
 
 use std::path::{Component, Path, PathBuf};
 
@@ -121,6 +122,9 @@ const WRITES: &[(&str, &[&str])] = &[
 /// 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"];
 
+/// Commands that queue a render, whose output defaults to the project's folder when not given.
+const QUEUES_AN_OUTPUT: &[&str] = &["renderQueue.add", "render.preRender", "file.createProxy"];
+
 /// What must be looked up before an implicitly writing command may run.
 #[derive(Debug, PartialEq, Eq)]
 pub enum Lookup {
@@ -128,8 +132,9 @@ pub enum Lookup {
     ProjectFile,
     /// Every output of the queued Render Queue items (`renderQueue.list`).
     RenderOutputs,
-    /// Whether the open project has a file (`project.summary` → `path`): a Render Queue item
-    /// added without `output` writes beside it, or to the home folder before the first save.
+    /// Whether the open project has a file (`project.summary` → `path`): a render queued without
+    /// `output` (Render Queue, Pre-render, Create Proxy) writes beside it, or to the home folder
+    /// before the first save.
     QueueDefault,
 }
 
@@ -169,7 +174,7 @@ impl Confinement {
 
     /// 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<Option<Lookup>> {
+    pub fn check_command(&self, id: &str, params: &Value) -> Result<Vec<Lookup>> {
         if REFUSED.contains(&id) || effectcraft_engine::commands::scripts::RUNS_SCRIPTS.contains(&id) {
             return Err(Error::BadArgs(format!("`{id}` is not available while file access is confined (--automation-read-root/--automation-write-root)")));
         }
@@ -192,11 +197,11 @@ impl Confinement {
                 if named.iter().chain(mixed).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() {
+                if !self.check_command(inner, &inner_params)?.is_empty() {
                     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);
+            return Ok(vec![]);
         }
         let (named, access) = match (keys_of(WRITES, id), keys_of(READS, id)) {
             (Some(k), _) => (k, Access::Write),
@@ -205,6 +210,10 @@ impl Confinement {
             (None, None) => (&[][..], Access::Read),
         };
         let mut given = false;
+        // Whether one of them is a file: not a Render Queue template, whose own output is relative
+        // and resolved like one the agent never gave, and not an array, which the commands that
+        // take a single output read as nothing at all.
+        let mut gave_a_file = false;
         for key in named {
             let Some(v) = params.get(*key).filter(|v| !v.is_null()) else { continue };
             given = true;
@@ -217,6 +226,7 @@ impl Confinement {
                 match p.as_str() {
                     Some(s) if matches!(id, "render.preRender" | "file.createProxy") && !looks_like_path(s) => {}
                     Some(s) => {
+                        gave_a_file |= v.is_string();
                         self.check(s, access)?;
                     }
                     None => return Err(Error::BadArgs(format!("`{id}`: `{key}` must be a path"))),
@@ -242,14 +252,19 @@ impl Confinement {
         if matches!(id, "file.openRecent" | "file.importRecent" | "anim.applyRecentPreset") && !given {
             return Err(Error::BadArgs(format!("`{id}`: give `path` while file access is confined")));
         }
-        // Without `output`, an unsaved project's item would write to the home folder (#2479).
-        if id == "renderQueue.add" && !given {
-            return Ok(Some(Lookup::QueueDefault));
+        let mut lookups = vec![];
+        // An output the agent did not name is derived from where the project sits: the Default
+        // Output Folder preference when that is set, beside the project file otherwise — and the
+        // home folder when the project has never been saved, outside every root (#2479). Only
+        // that last case is certainly outside, and reading the preference here would duplicate
+        // the engine's precedence, so an unsaved project is told to name its output instead.
+        if QUEUES_AN_OUTPUT.contains(&id) && !gave_a_file {
+            lookups.push(Lookup::QueueDefault);
         }
         if IMPLICIT_WRITES.contains(&id) && !(given && id == "file.save") {
-            return Ok(Some(if id == "file.save" || id == "file.incrementAndSave" { Lookup::ProjectFile } else { Lookup::RenderOutputs }));
+            lookups.push(if id == "file.save" || id == "file.incrementAndSave" { Lookup::ProjectFile } else { Lookup::RenderOutputs });
         }
-        Ok(None)
+        Ok(lookups)
     }
 
     /// Check what a [`Lookup`] found: `project.summary` or `renderQueue.list`'s result.
@@ -264,7 +279,7 @@ impl Confinement {
             Lookup::QueueDefault => match found.get("path").and_then(Value::as_str) {
                 Some(_) => Ok(()),
                 None => Err(Error::BadArgs(
-                    "`renderQueue.add`: the project has no file yet, so the default output would go to the home folder: give `output` inside an automation write root".into(),
+                    "the project has no file yet, so the default output would go to the home folder: give `output` inside an automation write root, or save the project inside one first".into(),
                 )),
             },
             Lookup::RenderOutputs => {

crates/automation/src/tests.rs

@@ -1479,6 +1479,59 @@ fn automation_roots_confine_file_access() {
     let _ = std::fs::remove_dir_all(&base);
 }
 
+/// #2479: a queued output the agent didn't name is derived from where the project sits, so on a
+/// project that has never been saved it lands in the home folder, outside every automation root.
+/// Queueing it there is refused, rather than leaving an item that can never be rendered.
+#[test]
+fn automation_roots_refuse_a_queued_output_from_an_unsaved_project() {
+    use crate::confine::Confinement;
+    let base = std::env::temp_dir().join(format!("ec-mcp-default-out-{}", std::process::id()));
+    let _ = std::fs::remove_dir_all(&base);
+    let write = base.join("out");
+    std::fs::create_dir_all(&write).unwrap();
+    let p = |f: &str| write.join(f).to_string_lossy().into_owned();
+    let mut s = server();
+    s.backend().confine(Confinement::new(&[], std::slice::from_ref(&write)).unwrap());
+    call_json(&mut s, "execute_command", json!({"command": "comp.new", "params": {"name": "A", "width": 64, "height": 64}}));
+    // Never saved: neither way of queueing a render may fall back to the home folder.
+    for command in ["renderQueue.add", "render.preRender", "file.createProxy"] {
+        let (c, err) = call(&mut s, "execute_command", json!({"command": command, "params": {"comp": "A"}}));
+        assert!(err, "`{command}` without an output should be refused: {c:?}");
+        let msg = c[0]["text"].as_str().unwrap_or_default();
+        assert!(msg.contains("the project has no file yet"), "`{command}`: {msg}");
+    }
+    // An array where a path goes: the engine reads a scalar, so this is no output either.
+    let (c, err) = call(&mut s, "execute_command", json!({"command": "file.createProxy", "params": {"comp": "A", "path": [p("arr.mov")]}}));
+    assert!(err, "an array where the output goes should be refused: {c:?}");
+    assert!(c[0]["text"].as_str().unwrap_or_default().contains("the project has no file yet"), "{c:?}");
+    // Naming an Output Module template instead of a file is the same: its own output is relative.
+    let (c, err) = call(&mut s, "execute_command", json!({"command": "render.preRender", "params": {"comp": "A", "output": "Lossless"}}));
+    assert!(err, "a template output should be refused too: {c:?}");
+    assert!(c[0]["text"].as_str().unwrap_or_default().contains("the project has no file yet"), "{c:?}");
+    let queue = call_json(&mut s, "execute_command", json!({"command": "renderQueue.list"}));
+    assert_eq!(queue["items"].as_array().map(Vec::len), Some(0), "nothing was queued: {queue}");
+    // Naming an output inside a write root is how it is done.
+    call_json(&mut s, "execute_command", json!({"command": "renderQueue.add", "params": {"comp": "A", "output": p("named.mp4")}}));
+    // A proxy named outside the roots is refused. Whether that reads as a write or a read is
+    // #1347's to classify, so this asserts the refusal and not its wording. The path is a sibling
+    // of the write root rather than `/elsewhere/…`, which is not absolute on Windows and would be
+    // refused for that instead.
+    let outside = base.join("elsewhere").join("proxy.mov").to_string_lossy().into_owned();
+    let (c, err) = call(&mut s, "execute_command", json!({"command": "file.createProxy", "params": {"comp": "A", "path": outside}}));
+    assert!(err, "a proxy outside the roots should be refused: {c:?}");
+    assert!(c[0]["text"].as_str().unwrap_or_default().contains("outside the automation roots"), "{c:?}");
+    // Once the project is saved there, the default output (beside it) is queued as before.
+    call_json(&mut s, "save_project", json!({"path": p("proj.ecproj")}));
+    // A name that is not a path is an Output Module template, and the output it resolves to lands
+    // beside the saved project: inside the write root, so it is allowed (#1347 relies on this).
+    call_json(&mut s, "execute_command", json!({"command": "file.createProxy", "params": {"comp": "A", "path": "ProxyTemplate"}}));
+    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"}));
+    let out = queue["items"][1]["outputPath"].as_str().unwrap_or_default();
+    assert!(out.starts_with(&*write.to_string_lossy()), "{queue}");
+    let _ = std::fs::remove_dir_all(&base);
+}
+
 /// #2131 (audit F-11, "Running code"): `script.run` and the Scripts menu are refused while file
 /// access is confined, but a ScriptUI panel's script runs just as freely — when the panel opens,
 /// when an event reaches one of its controls, or when one of its tasks comes due — and
@@ -1546,7 +1599,7 @@ fn blender_export_requires_an_absolute_path_in_a_write_root() {
     }
     let confinement = Confinement::new(std::slice::from_ref(&read), std::slice::from_ref(&write)).unwrap();
     let check = |path: &std::path::Path| confinement.check_command("camera.exportBlender", &json!({"path": path}));
-    assert_eq!(check(&write.join("scene.py")).unwrap(), None);
+    assert!(check(&write.join("scene.py")).unwrap().is_empty());
     for path in [read.join("scene.py"), base.join("outside.py"), write.join("../read/scene.py")] {
         assert!(check(&path).is_err(), "must deny {}", path.display());
     }

docs/mcp.md

@@ -12,8 +12,8 @@ written only under a write root; with no write root nothing is written. Every en
 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);
-`renderQueue.add` without `output` needs a saved project, since an unsaved one's default output
-would go to the home folder (#2479). Scripts
+`renderQueue.add`, `render.preRender` and `file.createProxy` without an output file need a saved
+project, since an unsaved one's default output would go to the home folder (#2479). 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.