Merge pull request #2521 from Anubhav-Rai/automation-refuse-script-code-main

automation: a confined agent can't start a script's code (#2131 F-11, "Running code")

crates/automation/src/confine.rs

@@ -7,7 +7,9 @@
 //! 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.
+//! 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};
 
@@ -35,6 +37,15 @@ const REFUSED: &[&str] = &[
     "file.installScript",
     "file.installScriptUIPanel",
     "file.uninstallScript",
+    // A ScriptUI panel's script runs when the panel opens, when an event reaches one of its
+    // controls, and when one of its `app.scheduleTask` tasks comes due.
+    "window.scriptPanel",
+    "window.restoreScriptPanels",
+    "script.runTasks",
+    "scriptui.click",
+    "scriptui.set",
+    "scriptui.mouse",
+    "scriptui.close",
     "file.executeFile",
     "file.revealInFinder",
     "layer.openSource",
@@ -138,6 +149,15 @@ impl Confinement {
         if REFUSED.contains(&id) {
             return Err(Error::BadArgs(format!("`{id}` is not available while file access is confined (--automation-read-root/--automation-write-root)")));
         }
+        // Scripts can't grant themselves the access the user withheld
+        // (`effectcraft_engine::prefs::Scripting::same_permissions`), and neither may a confined
+        // agent: `prefs.set` saves the settings file, so a script run long after this session
+        // would still have it. (`prefs.reset` only puts the defaults back, which grant nothing.)
+        if id == "prefs.set" && prefs_keys(params).iter().any(|k| is_scripting_pref(k)) {
+            return Err(Error::BadArgs(
+                "`prefs.set`: the Scripting settings decide what a script may do, so they can't be changed while file access is confined".into(),
+            ));
+        }
         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 };
@@ -218,6 +238,22 @@ impl Confinement {
     }
 }
 
+/// Whether a `prefs.set` key reaches the Scripting page. `Prefs::set` turns the key into a JSON
+/// pointer (`key.replace('.', "/")`), so `scripting/allowScriptsWriteFiles` resolves as well as
+/// the dotted spelling, and the page on its own replaces every setting on it.
+fn is_scripting_pref(key: &str) -> bool {
+    key.split(['.', '/']).next().is_some_and(|page| page.eq_ignore_ascii_case("scripting"))
+}
+
+/// The settings `prefs.set` is given: `key`, and the keys of `values`.
+fn prefs_keys(params: &Value) -> Vec<&str> {
+    let mut keys: Vec<&str> = params.get("key").and_then(Value::as_str).into_iter().collect();
+    if let Some(Value::Object(m)) = params.get("values") {
+        keys.extend(m.keys().map(String::as_str));
+    }
+    keys
+}
+
 fn keys_of(table: &[(&str, &'static [&'static str])], id: &str) -> Option<&'static [&'static str]> {
     table.iter().find(|(c, _)| *c == id).map(|(_, k)| *k)
 }

crates/automation/src/tests.rs

@@ -1416,3 +1416,60 @@ 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, "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
+/// `prefs.set` could grant every script run afterwards the file and network access the user
+/// withheld, in the settings file, long after the session. All of that is refused too.
+#[test]
+fn automation_roots_refuse_running_script_code() {
+    use crate::confine::Confinement;
+    let base = std::env::temp_dir().join(format!("ec-mcp-scripts-{}", std::process::id()));
+    let _ = std::fs::remove_dir_all(&base);
+    std::fs::create_dir_all(&base).unwrap();
+    let mut s = server();
+    s.backend().confine(Confinement::new(&[], std::slice::from_ref(&base)).unwrap());
+    let refused = |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()
+    };
+    // Every way a script's code starts.
+    for (command, params) in [
+        ("window.scriptPanel", json!({"name": "Layer Tools.jsx"})),
+        ("window.restoreScriptPanels", json!({})),
+        ("script.runTasks", json!({})),
+        ("scriptui.click", json!({"window": 1, "widget": 2})),
+        ("scriptui.set", json!({"window": 1, "widget": 2, "value": 1})),
+        ("scriptui.mouse", json!({"window": 1, "widget": 2, "type": "click"})),
+        ("scriptui.close", json!({"window": 1})),
+    ] {
+        let msg = refused(&mut s, "execute_command", json!({"command": command, "params": params}));
+        assert!(msg.contains("not available while file access is confined"), "`{command}`: {msg}");
+    }
+    // Including as a batch step.
+    let msg = refused(&mut s, "batch", json!({"steps": [{"command": "window.scriptPanel", "params": {"name": "x.jsx"}}]}));
+    assert!(msg.contains("not available while file access is confined"), "{msg}");
+    // What scripts may do stays as the user left it, by either spelling of the setting.
+    for params in [
+        json!({"key": "scripting.allowScriptsWriteFiles", "value": true}),
+        json!({"key": "scripting.allow_scripts_write_files", "value": true}),
+        json!({"values": {"scripting.warnExecutingFiles": false}}),
+        json!({"values": {"general.showToolTips": false, "scripting.enableJsDebugger": true}}),
+        // `Prefs::set` makes a JSON pointer of the key, so this spelling resolves too…
+        json!({"key": "scripting/allowScriptsWriteFiles", "value": true}),
+        // …and the page on its own replaces every setting on it.
+        json!({"key": "scripting", "value": {"allowScriptsWriteFiles": true}}),
+        json!({"values": {"scripting": {"warnExecutingFiles": false}}}),
+    ] {
+        let msg = refused(&mut s, "execute_command", json!({"command": "prefs.set", "params": params}));
+        assert!(msg.contains("what a script may do"), "{msg}");
+    }
+    let got = call_json(&mut s, "execute_command", json!({"command": "prefs.get", "params": {"key": "scripting.allowScriptsWriteFiles"}}));
+    assert_eq!(got, json!(false), "{got}");
+    // Other settings, and looking at the script windows, still work.
+    call_json(&mut s, "execute_command", json!({"command": "prefs.set", "params": {"key": "general.showToolTips", "value": false}}));
+    call_json(&mut s, "execute_command", json!({"command": "scriptui.list"}));
+    let _ = std::fs::remove_dir_all(&base);
+}