From 62b0d0bd601040faed125bbec099e129a337fe6e Mon Sep 17 00:00:00 2001 From: Tanish Manku Date: Wed, 7 Oct 2026 23:58:26 -0700 Subject: [PATCH] Automation: an allowed command can't run a denied one on its behalf (#861) Untrusted sessions (MCP, the control channel) checked only the top-level command and the steps of `actions.play`. Any other command that runs commands internally ran them unchecked, so the allowed `file.automate.conditionalModeChange` could run `image.mode.*`, which the desktop gate denies because it resolves the user's configured colour profiles. `Session::dispatch` now applies `Session::authorize` to every command before any side effect, nested ones included, which closes the class rather than this one command. Sessions without a gate (local UI, `photocraft-cli run`) are unchanged. --- book/src/security/automation-security.md | 4 +++- crates/automation/src/workspace.rs | 8 ++++++++ crates/engine/src/file_cmds/tests.rs | 15 +++++++++++++++ crates/engine/src/jobs.rs | 6 ++++++ crates/engine/src/lib.rs | 5 +++-- 5 files changed, 35 insertions(+), 3 deletions(-) diff --git a/book/src/security/automation-security.md b/book/src/security/automation-security.md index 15ac5f9b..49be151f 100644 --- a/book/src/security/automation-security.md +++ b/book/src/security/automation-security.md @@ -33,7 +33,9 @@ Automation is a privilege boundary because requests can cause filesystem access, are rejected before file effects. - Engine commands that still use ambient paths fail closed at the automation boundary. Synthetic UI input reaches the same command policy, and automation-triggered file hooks do not run - user-configured script-event paths. + user-configured script-event paths. The policy also covers every command a command runs on its + own behalf (an action's steps, `file.automate.conditionalModeChange` running `image.mode.*`), so + an allowed command can't reach a denied one. - `prefs.set` applies the engine's dotted-path normalization before checking for filesystem-bearing preference sections, including paths with leading or repeated dots. It refuses whole-preferences updates and those sections; individual safe preference keys remain available. diff --git a/crates/automation/src/workspace.rs b/crates/automation/src/workspace.rs index 3427a7c2..2244a2b8 100644 --- a/crates/automation/src/workspace.rs +++ b/crates/automation/src/workspace.rs @@ -460,6 +460,14 @@ mod tests { assert!(authorize_engine_command("prefs.set", &serde_json::json!({"path": "colorSettings.workingRgb", "value": "outside.icc"})).is_err()); assert!(authorize_engine_step("file.open", &serde_json::json!({})).is_err()); assert!(authorize_engine_step("actions.play", &serde_json::json!({})).is_ok()); + // An allowed command can't reach a denied one by running it on its own behalf. + let mut session = photocraft_engine::Session::new(); + session.execute("file.new", serde_json::json!({"width": 4, "height": 4})).unwrap(); + session.authorize = Some(authorize_desktop_engine_step); + let params = serde_json::json!({"to": "grayscale"}); + assert!(authorize_desktop_engine_command("file.automate.conditionalModeChange", ¶ms).is_ok()); + assert!(session.execute("file.automate.conditionalModeChange", params).is_err()); + assert_eq!(session.active().unwrap().doc.mode, photocraft_engine::doc::ColorMode::Rgb); assert!(authorize_desktop_engine_step("file.saveACopy", &serde_json::json!({})).is_err()); } diff --git a/crates/engine/src/file_cmds/tests.rs b/crates/engine/src/file_cmds/tests.rs index 8de181fb..2b5f8f9f 100644 --- a/crates/engine/src/file_cmds/tests.rs +++ b/crates/engine/src/file_cmds/tests.rs @@ -239,6 +239,21 @@ fn fit_image_and_conditional_mode_change() { assert!(s.execute("file.automate.conditionalModeChange", json!({"to": "duotone"})).is_err()); } +#[test] +fn conditional_mode_change_cannot_run_a_mode_change_the_gate_denies() { + fn deny_mode(id: &str, _: &Value) -> Result<()> { + if id.starts_with("image.mode.") { Err(EngineError::Other(format!("automation command `{id}` is disabled"))) } else { Ok(()) } + } + let mut s = session(4, 4, 8); + s.authorize = Some(deny_mode); + let e = s.execute("file.automate.conditionalModeChange", json!({"to": "grayscale"})).unwrap_err(); + assert!(e.to_string().contains("image.mode.grayscale"), "{e}"); + assert_eq!(doc(&s).mode, ColorMode::Rgb, "the refused nested step leaves the document alone"); + // Commands that compose only allowed steps still run. + s.execute("file.automate.fitImage", json!({"width": 2, "height": 2})).unwrap(); + assert_eq!(doc(&s).size.width, 2); +} + #[test] fn flatten_all_layer_effects_and_masks() { for depth in [8, 16, 32] { diff --git a/crates/engine/src/jobs.rs b/crates/engine/src/jobs.rs index f3f78d04..0bbacbec 100644 --- a/crates/engine/src/jobs.rs +++ b/crates/engine/src/jobs.rs @@ -594,6 +594,12 @@ impl Session { if !(params.is_object() || params.is_null()) { return Err(EngineError::BadParams { cmd: id.to_string(), msg: "params must be a JSON object".into() }); } + // Untrusted sessions gate every command, including the ones a command runs on its own + // behalf (`file.automate.conditionalModeChange` runs `image.mode.*`), before any side + // effect. + if let Some(gate) = self.authorize { + gate(id, ¶ms)?; + } // A floating selection drops before any other command (Undo puts it back instead). if let Some(v) = crate::float_cmds::before_command(self, id)? { return Ok(Started::Done(v)); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 9dd1fa1f..48aad051 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -303,8 +303,9 @@ pub struct Session { pub preset_store: Option, /// Window › Actions. The list persists with the preset store when one is attached. pub actions: actions_cmds::ActionState, - /// Per-step gate for `actions.play`. Untrusted sessions (MCP, the control channel) install - /// the same check a top-level command sees. `None` runs every step, which is what a local + /// Gate for every command the session runs, including the ones a command runs on its own + /// behalf and `actions.play` steps. Untrusted sessions (MCP, the control channel) install + /// the same check a top-level request sees. `None` runs everything, which is what a local /// UI and `photocraft-cli run` do. pub authorize: Option Result<()>>, /// Background jobs (see [`jobs`]).