mirror of
https://github.com/storytold/effectcraft.git
synced 2026-10-14 11:17:12 -04:00
Scripting: scripts can't grant themselves file and network access (#590)
A running script reached `prefs.set` through `app.run` and could turn on Settings > Scripting & Expressions > Allow Scripts to Write Files and Access Network (or turn off Warn User When Executing Files), which the File, Socket and system.callSystem gates then read: the permission the user withheld was the script's to take. Commands a script issues now go through `Session::execute_for_script`, which marks the session as running for a script for the command and everything it runs (engine.batch, ScriptUI callbacks, scheduled tasks). While it is, `prefs.set` refuses any change that would leave the Application Scripting settings different (compared on the resulting settings, so dotted keys, whole-page objects and batches are all covered), and a script's `prefs.reset` keeps them as the user set them. Command scripts read from a file (File > Scripts > Run Script File... with .json/.jsonl) count as scripts too. The UI, CLI, control channel and MCP still change them. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01QiAJeFttZp83Fh7YuzAR5f
This commit is contained in:
6 files changed
+102
-2
No files matched your search
@@ -465,6 +465,8 @@ fn run_script(s: &mut Session, p: &Value) -> Result<Value> {
|
||||
}
|
||||
_ => return Err(bad("file.runScript", "missing `path` (or inline `steps`)")),
|
||||
};
|
||||
// A command script read from a file is a script: it can't change what scripts may do.
|
||||
let from_file = p.get("steps").is_none();
|
||||
let mut results = vec![];
|
||||
for (i, step) in steps.iter().enumerate() {
|
||||
let (id, params) = match (step.get("command").or(step.get("id")).and_then(Value::as_str), step.get("method").and_then(Value::as_str)) {
|
||||
@@ -478,7 +480,8 @@ fn run_script(s: &mut Session, p: &Value) -> Result<Value> {
|
||||
if id == "file.runScript" || id == "script.run" {
|
||||
return Err(bad("file.runScript", "scripts can't run scripts"));
|
||||
}
|
||||
let r = s.execute(&id, params).map_err(|e| EngineError::Other(format!("step {} ({id}): {e}", i + 1)))?;
|
||||
let r = if from_file { s.execute_for_script(&id, params) } else { s.execute(&id, params) };
|
||||
let r = r.map_err(|e| EngineError::Other(format!("step {} ({id}): {e}", i + 1)))?;
|
||||
results.push(r);
|
||||
}
|
||||
Ok(json!({"steps": results.len(), "results": results}))
|
||||
|
||||
@@ -9,6 +9,10 @@ use crate::{EngineError, Result, Session, cmd, query};
|
||||
|
||||
// ---------------------------------------------------------------- prefs
|
||||
|
||||
/// Why a script can't change Application Scripting settings.
|
||||
const SCRIPT_PERMISSIONS: &str =
|
||||
"Scripts can't change what scripts are allowed to do. Change it yourself in Settings ▸ Scripting & Expressions ▸ Application Scripting.";
|
||||
|
||||
fn prefs_get(s: &mut Session, p: &Value) -> Result<Value> {
|
||||
let key = str_p(p, "key").unwrap_or("");
|
||||
s.prefs.get(key).ok_or_else(|| bad("prefs.get", format!("unknown setting `{key}`")))
|
||||
@@ -30,6 +34,9 @@ fn prefs_set(s: &mut Session, p: &Value) -> Result<Value> {
|
||||
for (k, v) in &pairs {
|
||||
next.set(k, v.clone()).map_err(|e| bad("prefs.set", e))?;
|
||||
}
|
||||
if s.running_for_script() && !next.scripting.same_permissions(&s.prefs.scripting) {
|
||||
return Err(EngineError::Other(SCRIPT_PERMISSIONS.into()));
|
||||
}
|
||||
s.prefs = next;
|
||||
s.prefs_changed();
|
||||
s.save_prefs();
|
||||
@@ -39,7 +46,12 @@ fn prefs_set(s: &mut Session, p: &Value) -> Result<Value> {
|
||||
|
||||
fn prefs_reset(s: &mut Session, p: &Value) -> Result<Value> {
|
||||
let page = str_p(p, "page");
|
||||
let before = s.prefs.scripting.clone();
|
||||
s.prefs.reset(page).map_err(|e| bad("prefs.reset", e))?;
|
||||
// A script's reset leaves what scripts may do as the user set it.
|
||||
if s.running_for_script() {
|
||||
s.prefs.scripting.keep_permissions(&before);
|
||||
}
|
||||
s.prefs_changed();
|
||||
s.save_prefs();
|
||||
s.toast(match page {
|
||||
|
||||
@@ -472,8 +472,16 @@ pub struct Session {
|
||||
pub footage_stamps: std::collections::HashMap<String, footage_reload::Stamp>,
|
||||
/// The stamps being taken on a worker thread ([`footage_reload::start_scan`]).
|
||||
pub footage_scan: Option<footage_reload::Scan>,
|
||||
/// How many script-issued commands are running ([`Session::execute_for_script`]): while one
|
||||
/// is, settings that guard what scripts may do can't change (see
|
||||
/// [`prefs::Scripting::same_permissions`]).
|
||||
pub script_calls: ScriptCalls,
|
||||
}
|
||||
|
||||
/// The count behind [`Session::running_for_script`]; only the engine changes it.
|
||||
#[derive(Clone, Copy, Debug, Default)]
|
||||
pub struct ScriptCalls(u32);
|
||||
|
||||
/// A thumbnail render ([`Session::thumbnail_job`]): (width, height, RGBA8).
|
||||
pub type ThumbnailJob = Box<dyn FnOnce() -> Option<(u32, u32, Vec<u8>)> + Send>;
|
||||
|
||||
@@ -559,6 +567,7 @@ impl Default for Session {
|
||||
check_footage_on_open: false,
|
||||
footage_stamps: Default::default(),
|
||||
footage_scan: None,
|
||||
script_calls: ScriptCalls::default(),
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -601,6 +610,22 @@ impl Session {
|
||||
self.execute(id, params)
|
||||
}
|
||||
|
||||
/// [`Session::execute`] for a command a script asked for (the JavaScript engine's
|
||||
/// `app.run`, ScriptUI callbacks…). The command and everything it runs count as the script's:
|
||||
/// it may not change the settings that limit scripts (Settings ▸ Scripting & Expressions ▸
|
||||
/// Application Scripting), which only the user can, from the UI, CLI or control channel.
|
||||
pub fn execute_for_script(&mut self, id: &str, params: Value) -> Result<Value> {
|
||||
self.script_calls.0 = self.script_calls.0.saturating_add(1);
|
||||
let r = self.execute(id, params);
|
||||
self.script_calls.0 = self.script_calls.0.saturating_sub(1);
|
||||
r
|
||||
}
|
||||
|
||||
/// Whether the command running was issued by a script ([`Session::execute_for_script`]).
|
||||
pub fn running_for_script(&self) -> bool {
|
||||
self.script_calls.0 > 0
|
||||
}
|
||||
|
||||
pub fn is_enabled(&self, id: &str) -> bool {
|
||||
commands::find(id).is_some_and(|c| (c.enabled)(self).is_ok())
|
||||
}
|
||||
|
||||
@@ -309,6 +309,25 @@ page!(Scripting {
|
||||
error_banner: bool = true,
|
||||
});
|
||||
|
||||
impl Scripting {
|
||||
/// Whether `other` grants scripts the same as this: Application Scripting (file and network
|
||||
/// access, the warning before executing files, the debugger). Scripts can't change these
|
||||
/// ([`crate::Session::execute_for_script`]): a script could otherwise give itself the
|
||||
/// access the user withheld.
|
||||
pub fn same_permissions(&self, other: &Scripting) -> bool {
|
||||
self.allow_scripts_write_files == other.allow_scripts_write_files
|
||||
&& self.warn_executing_files == other.warn_executing_files
|
||||
&& self.enable_js_debugger == other.enable_js_debugger
|
||||
}
|
||||
|
||||
/// Take `from`'s Application Scripting settings (see [`Scripting::same_permissions`]).
|
||||
pub fn keep_permissions(&mut self, from: &Scripting) {
|
||||
self.allow_scripts_write_files = from.allow_scripts_write_files;
|
||||
self.warn_executing_files = from.warn_executing_files;
|
||||
self.enable_js_debugger = from.enable_js_debugger;
|
||||
}
|
||||
}
|
||||
|
||||
/// One editable label: name and `#rrggbb` colour.
|
||||
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
|
||||
pub struct LabelPref {
|
||||
|
||||
@@ -152,7 +152,7 @@ pub(crate) fn exec(s: &mut Session, id: &str, params: J) -> Result<J, String> {
|
||||
if CREATES_LAYER.contains(&id) {
|
||||
s.state.selected_layers.clear();
|
||||
}
|
||||
let r = s.execute(id, params);
|
||||
let r = s.execute_for_script(id, params);
|
||||
if let Some(c) = target
|
||||
&& prev != Some(c)
|
||||
&& s.state.active_comp == Some(c)
|
||||
|
||||
@@ -362,6 +362,47 @@ fn security_gate_for_files() {
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
/// #590: a script can't give itself file and network access (or turn off the warning before
|
||||
/// executing files) by changing Settings ▸ Scripting & Expressions; the user still can.
|
||||
#[test]
|
||||
fn scripts_cant_change_what_scripts_may_do() {
|
||||
let mut s = session();
|
||||
let attempts = [
|
||||
"app.run('prefs.set', {key: 'scripting.allowScriptsWriteFiles', value: true})",
|
||||
"app.run('prefs.set', {values: {'scripting.warnExecutingFiles': false}})",
|
||||
"app.run('prefs.set', {key: 'scripting', value: {allowScriptsWriteFiles: true}})",
|
||||
"app.run('engine.batch', {steps: [{command: 'prefs.set', params: {key: 'scripting.enableJsDebugger', value: true}}]})",
|
||||
];
|
||||
for code in attempts {
|
||||
let o = run_code(&mut s, code, "grant.jsx");
|
||||
let e = o.error.unwrap_or_else(|| panic!("`{code}` was allowed"));
|
||||
assert!(e.message.contains("Scripts can't change what scripts are allowed to do"), "{code}: {e:?}");
|
||||
let sc = &s.prefs.scripting;
|
||||
assert!(!sc.allow_scripts_write_files && sc.warn_executing_files && !sc.enable_js_debugger, "{code}");
|
||||
}
|
||||
assert!(run_code(&mut s, "new Socket()", "net.jsx").error.is_some());
|
||||
// Other settings, and the expression editor's, are still the script's to change.
|
||||
ok(&mut s, "app.run('prefs.set', {values: {'general.undoLevels': 40, 'scripting.editorFontSize': 16}})");
|
||||
assert_eq!((s.prefs.general.undo_levels, s.prefs.scripting.editor_font_size), (40, 16));
|
||||
// The user grants access; a script's Reset Settings doesn't take it back or change it.
|
||||
s.execute("prefs.set", json!({"key": "scripting.allowScriptsWriteFiles", "value": true})).unwrap();
|
||||
ok(&mut s, "app.run('prefs.reset', {})");
|
||||
assert!(s.prefs.scripting.allow_scripts_write_files);
|
||||
assert_eq!(s.prefs.general.undo_levels, effectcraft_engine::prefs::Prefs::default().general.undo_levels);
|
||||
// A command script file is a script too.
|
||||
let dir = std::env::temp_dir().join(format!("ec-script-grant-{}", std::process::id()));
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
let f = dir.join("grant.json");
|
||||
std::fs::write(&f, r#"[{"command": "prefs.set", "params": {"key": "scripting.allowScriptsWriteFiles", "value": false}}]"#).unwrap();
|
||||
let e = s.execute("file.runScript", json!({"path": f.to_string_lossy()})).unwrap_err().to_string();
|
||||
assert!(e.contains("Scripts can't change"), "{e}");
|
||||
assert!(s.prefs.scripting.allow_scripts_write_files);
|
||||
// The user (UI, CLI, control channel) can take it back.
|
||||
s.execute("prefs.set", json!({"key": "scripting.allowScriptsWriteFiles", "value": false})).unwrap();
|
||||
assert!(!s.prefs.scripting.allow_scripts_write_files);
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn engine_entry_points() {
|
||||
let mut s = session();
|
||||
|
||||
Reference in new issue
Block a user