From 23c0107511d516dcd5978594384ba850e7d95fba Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Wed, 21 Oct 2026 14:01:51 -0400 Subject: [PATCH] Engine: macro policy checks a step's parameters after tokens resolve; any path counts as file access A path given as a token (`{"$ref": "Input"}` or `{{Input}}`) became a file name only when the step ran, after the policy had passed it, so a macro without permissions or approval could write a file, even from macro.check. Exports also wrote to blank paths the policy ignored. Any `path` other than null now needs the permission and approval, and the runner checks the policy again on the resolved parameters before the command runs. --- crates/engine/src/macros/policy.rs | 15 +++++++++-- crates/engine/src/macros/run.rs | 21 ++++++++++------ crates/engine/src/tests/macros.rs | 40 ++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 10 deletions(-) diff --git a/crates/engine/src/macros/policy.rs b/crates/engine/src/macros/policy.rs index 7329f567f..84bbc497b 100644 --- a/crates/engine/src/macros/policy.rs +++ b/crates/engine/src/macros/policy.rs @@ -43,8 +43,12 @@ const READS_FILES: &[&str] = &["data.getData", "data.fromTextCsv", "insert.pictu const EXPORTS: &[&str] = &["file.exportPdf", "file.exportCsv", "file.exportHtml", "file.print", "file.printPreview", "file.saveBytes"]; /// The policy for running command `id` with `params` as a macro step in a document with `trust`. +/// +/// Any `path` other than `null` counts as touching a file: a token such as `{"$ref": "Input"}` +/// becomes a path when the step runs, and exports write to `""` and `" "` as file names. The +/// runner checks the parameters both as written and as resolved. pub fn macro_policy(id: &str, params: &Json, trust: Trust) -> Policy { - let has_path = params.get("path").and_then(Json::as_str).is_some_and(|p| !p.trim().is_empty()); + let has_path = params.get("path").is_some_and(|p| !p.is_null()); let base = if EXPORTS.contains(&id) { if has_path { Policy::NeedsPermission("writes a file".into()) } else { Policy::Allow } } else if ALLOWED.contains(&id) { @@ -104,7 +108,14 @@ mod tests { // Without a path they touch no file. for id in ["data.getData", "insert.picture", "file.exportPdf", "file.saveBytes", "file.printPreview"] { assert_eq!(macro_policy(id, &none, Trust::Local), Policy::Allow, "{id}"); - assert_eq!(macro_policy(id, &json!({"path": " "}), Trust::Local), Policy::Allow, "{id}"); + assert_eq!(macro_policy(id, &json!({"path": null}), Trust::Local), Policy::Allow, "{id}"); + } + // A path that is blank or a token still touches a file once the step runs. + for path in [json!(""), json!(" "), json!({"$ref": "Input"}), json!("{{Input}}.csv"), json!(1)] { + for id in ["file.exportCsv", "data.fromTextCsv"] { + let p = json!({ "path": path }); + assert!(matches!(macro_policy(id, &p, Trust::Local), Policy::NeedsPermission(_)), "{id} {p}"); + } } for id in ["cell.set", "home.bold", "view.macros", "edit.copy", "file.properties"] { assert_eq!(macro_policy(id, &none, Trust::Local), Policy::Allow, "{id}"); diff --git a/crates/engine/src/macros/run.rs b/crates/engine/src/macros/run.rs index 3651deb0c..aebaa8c90 100644 --- a/crates/engine/src/macros/run.rs +++ b/crates/engine/src/macros/run.rs @@ -576,29 +576,34 @@ impl Machine { fn command(&mut self, s: &mut Session, path: &str, id: &str, params: &Json) -> std::result::Result<(), StepError> { let fail = |msg: String| StepError::new(path, Some(id), msg); - match macro_policy(id, params, self.trust) { - Policy::Deny(why) => return Err(fail(format!("{id} can't run in a macro: {why}."))), - Policy::NeedsPermission(what) => { + // The policy sees the parameters the command will get, after tokens became A1 text. + let resolved = { + let wb = &s.doc().map_err(|e| fail(e.to_string()))?.wb; + resolve::resolve_params(params, &self.env(wb)).map_err(|e| StepError::resolve(path, Some(id), e))? + }; + match (macro_policy(id, params, self.trust), macro_policy(id, &resolved, self.trust)) { + (_, Policy::Deny(why)) | (Policy::Deny(why), _) => return Err(fail(format!("{id} can't run in a macro: {why}."))), + (written, Policy::NeedsPermission(what)) => { if !self.permissions.iter().any(|p| p == id) { return Err(fail(format!("{id} {what}. Add it to the macro's permissions to allow it."))); } + // The gate asked the person about this step only if it saw the file access. + if !matches!(written, Policy::NeedsPermission(_)) { + return Err(fail(format!("{id} {what} only once its parameters are filled in, so it wasn't approved."))); + } if self.dry_run { self.partial = Some((path.to_string(), format!("{id} {what}"))); self.frames.clear(); return Ok(()); } } - Policy::Allow | Policy::Ask(_) => {} + (_, Policy::Allow | Policy::Ask(_)) => {} } if self.dry_run && !previewable(id) { self.partial = Some((path.to_string(), format!("{id} can't be previewed"))); self.frames.clear(); return Ok(()); } - let resolved = { - let wb = &s.doc().map_err(|e| fail(e.to_string()))?.wb; - resolve::resolve_params(params, &self.env(wb)).map_err(|e| StepError::resolve(path, Some(id), e))? - }; self.last_params = Some(resolved.clone()); let queued = s.ui_requests.len(); let r = s.execute(id, resolved); diff --git a/crates/engine/src/tests/macros.rs b/crates/engine/src/tests/macros.rs index fcc418b56..c3a2c4442 100644 --- a/crates/engine/src/tests/macros.rs +++ b/crates/engine/src/tests/macros.rs @@ -358,6 +358,46 @@ fn macros_are_bounded() { assert!(err.contains("ran longer than"), "{err}"); } +#[test] +fn a_token_in_a_path_needs_the_permission_and_approval() { + let mut s = s(); + let dir = std::env::temp_dir().join(format!("gc-macro-token-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let files = |dir: &std::path::Path| std::fs::read_dir(dir).unwrap().count(); + let in_dir = format!("{}/{{{{Input}}}}", dir.to_string_lossy()); + for path in [json!({"$ref": "Input"}), json!(in_dir), json!(" ")] { + let step = json!({"command": "file.exportCsv", "params": {"path": path}}); + put(&mut s, json!({"name": "Export", "inputs": [{"name": "Input"}], "steps": [step]})); + // Without the permission nothing runs, not even the check's dry run. + let err = s.execute("macro.run", json!({"name": "Export"})).unwrap_err().to_string(); + assert!(err.contains("Step 1 (file.exportCsv) writes a file. Add file.exportCsv to the macro's permissions"), "{path}: {err}"); + let c = s.execute("macro.check", json!({"name": "Export"})).unwrap(); + assert!(c["errors"][0].as_str().unwrap().contains("writes a file"), "{path}: {c}"); + assert!(c.get("footprint").is_none()); + // With it, the person approves first; the check doesn't run it before then. + put(&mut s, json!({"name": "Export", "inputs": [{"name": "Input"}], "steps": [step], "permissions": ["file.exportCsv"]})); + let r = s.execute("macro.run", json!({"name": "Export"})); + let Err(EngineError::Approve(q)) = &r else { panic!("{path}: {r:?}") }; + assert!(q.contains("file.exportCsv, which writes a file"), "{q}"); + let c = s.execute("macro.check", json!({"name": "Export"})).unwrap(); + assert_eq!(c["needsApproval"], json!(["runs file.exportCsv, which writes a file"]), "{path}"); + assert!(c.get("footprint").is_none()); + assert_eq!(files(&dir), 0, "{path}"); + assert!(!std::path::Path::new("A1.csv").exists() && !std::path::Path::new(" .csv").exists()); + } + // Once approved, the check's dry run stops before the step, and only the run writes the file. + let step = json!({"command": "file.exportCsv", "params": {"path": in_dir}}); + put(&mut s, json!({"name": "Export", "inputs": [{"name": "Input"}], "steps": [step], "permissions": ["file.exportCsv"]})); + s.execute("macro.run", json!({"name": "Export", "approve": true, "inputs": "B2"})).unwrap(); + assert_eq!(files(&dir), 1); + std::fs::remove_dir_all(&dir).unwrap(); + std::fs::create_dir_all(&dir).unwrap(); + let c = s.execute("macro.check", json!({"name": "Export", "inputs": "B2"})).unwrap(); + assert_eq!(c["footprint"]["partial"]["step"], "1", "{c}"); + assert_eq!(files(&dir), 0, "the check writes nothing"); + let _ = std::fs::remove_dir_all(&dir); +} + #[test] fn the_policy_keeps_macros_away_from_files_and_the_app() { let mut s = s();