Engine: a macro run names the files it kept, refuses parameters it would ignore, and bounds its work

- A run that rolls back after writing a file no longer says "Nothing was changed.": the message
  says the workbook was put back and names the step whose file stays (`StepError::wrote_files`,
  also after a Stop with rollback).
- `macro.run` refuses an input the macro doesn't have (a misspelt input bound the selection),
  an input that isn't reference text, and `onConflict`, `overwrite`, `approve` or `timeLimit` of
  the wrong kind, which it used to ignore; `null` still counts as not given.
- A step no longer copies itself, and Repeat and If no longer copy their bodies, on every pass:
  a body is copied once per run. A run passes its commands at most 256 MB of parameters, all
  steps together, which bounds a run's work on the web build, where there is no clock.
- Every step except an approved file step runs inside the scripts' file sandbox, so a command
  that touches a file under a parameter other than `path` can't do so from a macro.
- Docs: moving cells that hold data counts as writing over them.
- Tests: `run_parameters_that_would_be_ignored_are_refused`,
  `a_run_passes_a_bounded_amount_of_parameters`, and the rollback message in
  `each_step_is_checked_for_writes_over_data`.

crates/engine/src/io.rs

@@ -611,7 +611,7 @@ impl Drop for ScriptSandbox {
 /// thread is being torn down).
 fn script_sandbox_check(path: &str) -> Result<()> {
     if SCRIPT_SANDBOX.try_with(|c| c.get() > 0).unwrap_or(true) {
-        return Err(EngineError::Other(format!("{path}: scripts can't read or write files")));
+        return Err(EngineError::Other(format!("{path}: scripts, and macro steps without the permission, can't read or write files")));
     }
     Ok(())
 }

crates/engine/src/macros/policy.rs

@@ -1,6 +1,8 @@
-//! What a macro step may run. Macros run commands, so this is the one place that keeps a macro
-//! from saving, opening or closing files, driving the app, running other macros, or reading and
-//! writing files without the person knowing.
+//! What a macro step may run. Macros run commands, so this keeps a macro from saving, opening or
+//! closing files, driving the app, running other macros, or reading and writing files without the
+//! person knowing. It goes by command id and the `path` parameter; the runner also runs every
+//! step except an approved file step inside the scripts' file sandbox, so a command that touches a
+//! file under another parameter can't either.
 
 use serde_json::Value as Json;
 
@@ -47,6 +49,11 @@ const READS_FILES: &[&str] = &["data.getData", "data.fromTextCsv", "insert.pictu
 /// File commands that only produce bytes unless given a `path` (then they write a file).
 const EXPORTS: &[&str] = &["file.exportPdf", "file.exportCsv", "file.exportHtml", "file.print", "file.printPreview", "file.saveBytes"];
 
+/// Whether command `id` with `params` writes a file (an export or print given a `path`).
+pub fn writes_file(id: &str, params: &Json) -> bool {
+    EXPORTS.contains(&id) && params.get("path").is_some_and(|p| !p.is_null())
+}
+
 /// 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"}`

crates/engine/src/macros/run.rs

@@ -18,7 +18,7 @@
 //! The steps run on a small machine with an explicit frame stack, so the debugger can stop
 //! between any two steps and pick up again on a later frame.
 
-use std::collections::BTreeSet;
+use std::collections::{BTreeSet, HashMap};
 use std::sync::Arc;
 
 use gridcraft_core::{CellRef, RangeRef, Value};
@@ -27,7 +27,7 @@ use gridcraft_model::{Cell, CompareOp, InputKind, Macro, OnConflict, RepeatOf, S
 use serde::Serialize;
 use serde_json::{Value as Json, json};
 
-use super::policy::{Policy, macro_policy};
+use super::policy::{Policy, macro_policy, writes_file};
 use super::resolve::{self, Env, Place, ResolveError};
 use crate::{EngineError, Result, Session, Trust, UiRequest};
 
@@ -38,6 +38,9 @@ pub const MAX_EXECUTED_STEPS: u64 = 100_000;
 pub const DEFAULT_TIME_LIMIT_MS: f64 = 60_000.0;
 /// The most cells an input may have for its content to be checked.
 pub const MAX_INPUT_CELLS: u64 = 1_000_000;
+/// The most bytes of parameters one run passes to its commands, all steps together: on the web
+/// build, where there is no clock, this and the step limit bound the work a run does.
+pub const MAX_PARAM_BYTES: u64 = 256 * 1024 * 1024;
 /// The most ranges a list of cells gives per sheet (the counts stay exact).
 const MAX_LISTED_RANGES: usize = 500;
 
@@ -58,11 +61,22 @@ pub struct StepError {
     /// The step would write over data and the conflict rule is ask: overwriting answers it.
     #[serde(skip_serializing_if = "std::ops::Not::not")]
     pub confirm: bool,
+    /// Files the run wrote before the step failed ("step 1 (file.exportCsv)"): rolling back the
+    /// workbook leaves them written.
+    #[serde(skip_serializing_if = "Vec::is_empty")]
+    pub wrote_files: Vec<String>,
 }
 
 impl StepError {
     fn new(step: &str, command: Option<&str>, message: impl Into<String>) -> StepError {
-        StepError { step: step.to_string(), command: command.map(str::to_string), message: message.into(), edge: false, confirm: false }
+        StepError {
+            step: step.to_string(),
+            command: command.map(str::to_string),
+            message: message.into(),
+            edge: false,
+            confirm: false,
+            wrote_files: Vec::new(),
+        }
     }
 
     fn resolve(step: &str, command: Option<&str>, e: ResolveError) -> StepError {
@@ -79,11 +93,30 @@ impl StepError {
     }
 
     pub fn into_engine(self, name: &str) -> EngineError {
-        let text = format!("Macro '{name}' stopped. {} Nothing was changed.", self.describe());
+        let text = format!("Macro '{name}' stopped. {} {}", self.describe(), rolled_back(&self.wrote_files));
         if self.confirm { EngineError::Confirm(text) } else { EngineError::Other(text) }
     }
 }
 
+/// What a run that rolled back leaves: "Nothing was changed.", or the files it wrote before.
+pub fn rolled_back(wrote_files: &[String]) -> String {
+    match wrote_files {
+        [] => "Nothing was changed.".into(),
+        [one] => format!("The workbook was put back; the file {one} wrote stays."),
+        many => format!("The workbook was put back; the files {} wrote stay.", join_and(many)),
+    }
+}
+
+/// The size of `v` as JSON text, near enough to bound how much a run passes to its commands.
+fn json_bytes(v: &Json) -> u64 {
+    match v {
+        Json::String(t) => t.len() as u64 + 2,
+        Json::Array(a) => a.iter().map(json_bytes).fold(2, u64::saturating_add),
+        Json::Object(o) => o.iter().map(|(k, v)| (k.len() as u64 + 3).saturating_add(json_bytes(v))).fold(2, u64::saturating_add),
+        _ => 8,
+    }
+}
+
 // ---------------------------------------------------------------- gate
 
 /// Checks every enabled step against the policy: denied commands and undeclared file access are
@@ -426,6 +459,13 @@ pub struct Machine {
     pub stopped: Option<(String, bool)>,
     /// The last step's resolved parameters (for the debugger).
     pub last_params: Option<Json>,
+    /// Files the run wrote ("step 1 (file.exportCsv)"): rolling the run back doesn't take them back.
+    pub wrote_files: Vec<String>,
+    /// Repeat and If bodies by the path of their frame: a body is copied the first time its step
+    /// runs, not on every pass.
+    bodies: HashMap<String, Arc<Vec<Step>>>,
+    /// Bytes of parameters passed to commands so far, against [`MAX_PARAM_BYTES`].
+    param_bytes: u64,
     /// Set unless the macro may overwrite: each step is checked for writes over data as it runs.
     guard: Option<Guard>,
 }
@@ -518,6 +558,9 @@ impl Machine {
             time_limit_ms: DEFAULT_TIME_LIMIT_MS,
             stopped: None,
             last_params: None,
+            wrote_files: Vec::new(),
+            bodies: HashMap::new(),
+            param_bytes: 0,
             guard: None,
         };
         m.settle();
@@ -574,12 +617,15 @@ impl Machine {
 
     /// Runs the current step and moves to the next one.
     pub fn step(&mut self, s: &mut Session) -> std::result::Result<(), StepError> {
-        let Some((path, st)) = self.current().map(|(p, st)| (p, st.clone())) else { return Ok(()) };
+        // The frame's steps are shared, so the step (and its bodies) isn't copied to run it.
+        let Some((path, steps, i)) = self.frames.last().map(|f| (child_path(&f.prefix, f.next), Arc::clone(&f.steps), f.next)) else { return Ok(()) };
+        let Some(st) = steps.get(i) else { return Ok(()) };
         if let Some(f) = self.frames.last_mut() {
             f.next += 1;
         }
-        let r = self.exec(s, &path, &st);
-        if r.is_err() {
+        let mut r = self.exec(s, &path, st);
+        if let Err(e) = r.as_mut() {
+            e.wrote_files.clone_from(&self.wrote_files);
             self.frames.clear();
         }
         self.settle();
@@ -618,7 +664,8 @@ impl Machine {
                 }
                 if items > 0 && body.iter().any(|b| b.enabled) {
                     let repeat = Some(Loop { of: *of, over: place, at: 0, count: items });
-                    self.frames.push(Frame { steps: Arc::new(body.clone()), next: 0, prefix: path.to_string(), repeat });
+                    let steps = self.body(path.to_string(), body);
+                    self.frames.push(Frame { steps, next: 0, prefix: path.to_string(), repeat });
                 }
                 Ok(())
             }
@@ -634,7 +681,9 @@ impl Machine {
                 };
                 let (branch, name) = if compare(&v, *op, value) { (then, "then") } else { (otherwise, "else") };
                 if branch.iter().any(|b| b.enabled) {
-                    self.frames.push(Frame { steps: Arc::new(branch.clone()), next: 0, prefix: format!("{path}.{name}"), repeat: None });
+                    let prefix = format!("{path}.{name}");
+                    let steps = self.body(prefix.clone(), branch);
+                    self.frames.push(Frame { steps, next: 0, prefix, repeat: None });
                 }
                 Ok(())
             }
@@ -674,6 +723,11 @@ impl Machine {
         Ok(())
     }
 
+    /// The shared copy of the body that runs under `prefix`.
+    fn body(&mut self, prefix: String, body: &[Step]) -> Arc<Vec<Step>> {
+        Arc::clone(self.bodies.entry(prefix).or_insert_with(|| Arc::new(body.to_vec())))
+    }
+
     fn env<'a>(&'a self, wb: &'a Workbook) -> Env<'a> {
         Env { wb, active_sheet: wb.active_sheet, inputs: &self.inputs, output: self.output, item: self.item() }
     }
@@ -698,13 +752,26 @@ impl Machine {
             }
             (_, Policy::Allow | Policy::Ask(_)) => {}
         }
+        self.param_bytes = self.param_bytes.saturating_add(json_bytes(&resolved));
+        if self.param_bytes > MAX_PARAM_BYTES {
+            return Err(fail(format!(
+                "The macro passed more than {} MB of parameters to its steps and was stopped.",
+                MAX_PARAM_BYTES / (1024 * 1024)
+            )));
+        }
+        let file_step = matches!(macro_policy(id, &resolved, self.trust), Policy::NeedsPermission(_));
+        let writes = writes_file(id, &resolved);
         self.last_params = Some(resolved.clone());
         let prev = s.docs.iter_mut().find(|d| d.uid == self.doc).map(|d| {
             d.edits = Some(Vec::new());
             d.wb.clone()
         });
         let queued = s.ui_requests.len();
+        // Only an approved file step reaches the file system: any other step runs as a script's
+        // does, so a command that reads or writes a file under another parameter name can't.
+        let sandbox = (!file_step).then(crate::io::ScriptSandbox::enter);
         let r = s.execute(id, resolved);
+        drop(sandbox);
         let edits = s.docs.iter_mut().find(|d| d.uid == self.doc).and_then(|d| d.edits.take()).unwrap_or_default();
         // Macros never open dialogs or web pages, and never start editing a cell; messages for
         // the person stay queued.
@@ -719,6 +786,9 @@ impl Machine {
             }
         }
         r.map_err(|e| fail(e.to_string()))?;
+        if writes {
+            self.wrote_files.push(format!("step {path} ({id})"));
+        }
         if let Some(b) = blocked {
             return Err(fail(b));
         }
@@ -895,6 +965,7 @@ pub fn begin(s: &mut Session, name: &str, p: &Json) -> Result<Begun> {
         .and_then(|i| d.wb.macros.get(i))
         .cloned()
         .ok_or_else(|| EngineError::Other(format!("There is no macro called '{name}'.")))?;
+    check_params(p, &m)?;
     let asks = gate(&m, trust)?;
     approve(s, &m, doc, &asks, p.get("approve").and_then(Json::as_bool).unwrap_or(false))?;
     let prepared = prepare(s, &m, p.get("inputs"))?;
@@ -929,7 +1000,7 @@ pub fn run(s: &mut Session, name: &str, p: &Json) -> Result<Json> {
         let name = &b.mac.name;
         machine.run_to_end(s).map_err(|e| e.into_engine(name))?;
         match machine.stopped {
-            Some((msg, true)) => Err(EngineError::Other(format!("Macro '{name}' stopped: {msg} Nothing was changed."))),
+            Some((msg, true)) => Err(EngineError::Other(format!("Macro '{name}' stopped: {msg} {}", rolled_back(&machine.wrote_files)))),
             stopped => {
                 let mut out = json!({"macro": name, "ran": machine.executed, "inputs": b.inputs});
                 if let Some((msg, _)) = stopped {
@@ -941,6 +1012,47 @@ pub fn run(s: &mut Session, name: &str, p: &Json) -> Result<Json> {
     })
 }
 
+/// Refuses run parameters that would otherwise be ignored: an input the macro doesn't have, an
+/// input given as something other than reference text, and `onConflict`, `overwrite`, `approve`
+/// or `timeLimit` of the wrong kind (`null` counts as not given).
+pub fn check_params(p: &Json, m: &Macro) -> Result<()> {
+    let refuse = |msg: String| Err(EngineError::Other(format!("Macro '{}': {msg}", m.name)));
+    let names = || m.inputs.iter().map(|i| i.name.clone()).collect::<Vec<_>>().join(", ");
+    match p.get("inputs") {
+        None | Some(Json::Null) => {}
+        Some(Json::String(_)) if !m.inputs.is_empty() => {}
+        Some(Json::String(_)) => return refuse("it takes no inputs.".into()),
+        Some(Json::Object(o)) => {
+            for (k, v) in o {
+                if !m.inputs.iter().any(|i| i.name.eq_ignore_ascii_case(k)) {
+                    return refuse(if m.inputs.is_empty() {
+                        format!("it has no input called '{k}'; it takes no inputs.")
+                    } else {
+                        format!("it has no input called '{k}' (its inputs: {}).", names())
+                    });
+                }
+                if !v.is_string() {
+                    return refuse(format!("give the input {k} as reference text, such as \"B2:D9\"."));
+                }
+            }
+        }
+        Some(_) => return refuse("give `inputs` as reference text, such as \"B2:D9\", or as {\"Input\": \"B2:D9\"}.".into()),
+    }
+    let given = |k: &str| p.get(k).filter(|v| !v.is_null());
+    if given("onConflict").is_some_and(|v| !v.is_string()) {
+        return refuse("`onConflict` must be ask, fail, overwrite, shiftDown or shiftRight.".into());
+    }
+    for k in ["overwrite", "approve"] {
+        if given(k).is_some_and(|v| !v.is_boolean()) {
+            return refuse(format!("`{k}` must be true or false."));
+        }
+    }
+    if given("timeLimit").is_some_and(|v| !v.as_f64().is_some_and(|t| t > 0.0)) {
+        return refuse("`timeLimit` must be a number of seconds above 0.".into());
+    }
+    Ok(())
+}
+
 /// The run's `timeLimit` (seconds) in milliseconds, or the default.
 fn time_limit(p: &Json) -> f64 {
     p.get("timeLimit").and_then(Json::as_f64).map(|t| (t * 1000.0).clamp(1.0, 600_000.0)).unwrap_or(DEFAULT_TIME_LIMIT_MS)

crates/engine/src/tests/macros.rs

@@ -252,6 +252,10 @@ fn each_step_is_checked_for_writes_over_data() {
                 (_, r) => panic!("{first} {rule}: {r:?}"),
             };
             assert!(e.contains("Step 2 (cell.set): It would write over data (B1 on Sheet1)"), "{e}");
+            // The workbook is put back; a file the run wrote stays, and the message says so.
+            let left =
+                if first == &export { "The workbook was put back; the file step 1 (file.exportCsv) wrote stays." } else { "Nothing was changed." };
+            assert!(e.ends_with(left), "{e}");
             assert_eq!(v(&s, "B1").display(), "keep", "{first} {rule}");
             assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start), "{first} {rule}: rolled back");
         }
@@ -271,6 +275,46 @@ fn each_step_is_checked_for_writes_over_data() {
     let _ = std::fs::remove_dir_all(&dir);
 }
 
+#[test]
+fn run_parameters_that_would_be_ignored_are_refused() {
+    let mut s = s();
+    data(&mut s);
+    put(&mut s, totals());
+    for (p, says) in [
+        (json!({"inputs": {"Inptu": "B2:C5"}}), "it has no input called 'Inptu' (its inputs: Input)."),
+        (json!({"inputs": {"Input": 5}}), "give the input Input as reference text"),
+        (json!({"inputs": ["B2:C5"]}), "give `inputs` as reference text"),
+        (json!({"inputs": "B2:C5", "onConflict": 1}), "`onConflict` must be"),
+        (json!({"inputs": "B2:C5", "overwrite": "yes"}), "`overwrite` must be true or false."),
+        (json!({"inputs": "B2:C5", "approve": 1}), "`approve` must be true or false."),
+        (json!({"inputs": "B2:C5", "timeLimit": "30"}), "`timeLimit` must be a number of seconds above 0."),
+        (json!({"inputs": "B2:C5", "timeLimit": 0}), "`timeLimit` must be a number of seconds above 0."),
+    ] {
+        let mut p = p;
+        p["name"] = json!("Totals");
+        let err = s.execute("macro.run", p.clone()).unwrap_err().to_string();
+        assert!(err.contains(&format!("Macro 'Totals': {says}")), "{p}: {err}");
+    }
+    put(&mut s, json!({"name": "None", "steps": []}));
+    let err = s.execute("macro.run", json!({"name": "None", "inputs": "A1"})).unwrap_err().to_string();
+    assert!(err.contains("Macro 'None': it takes no inputs."), "{err}");
+    // `null` is the same as not given, and an input's name matches in any case.
+    let p = json!({"name": "Totals", "inputs": {"input": "B2:C5"}, "onConflict": null, "overwrite": true, "timeLimit": null});
+    s.execute("macro.run", p).unwrap();
+}
+
+#[test]
+fn a_run_passes_a_bounded_amount_of_parameters() {
+    // A step with a 3 MB parameter in a Repeat of 100 rows would hand its command 300 MB; the run
+    // stops at 256 MB, which matters on the web build, where there is no time limit.
+    let mut s = s();
+    let big = "x".repeat(3_000_000);
+    let body = json!([{"command": "home.bold", "params": {"range": "A1", "note": big}}]);
+    put(&mut s, json!({"name": "Big", "output": {"onConflict": "overwrite"}, "steps": [{"type": "repeat", "over": "A1:A100", "body": body}]}));
+    let err = s.execute("macro.run", json!({"name": "Big"})).unwrap_err().to_string();
+    assert!(err.contains("passed more than 256 MB of parameters to its steps"), "{err}");
+}
+
 #[test]
 fn the_answer_to_the_question_counts_however_ask_was_given() {
     let mut s = s();

docs/macros.md

@@ -60,8 +60,9 @@ macro interface.
   whole macro (4,194,304 characters of JSON). A macro read from a file that breaks any of these is
   left out with a warning; it is never changed to fit. When a macro runs, a Repeat whose range has
   more rows or columns than its `max` is refused, and a run executes at most 100,000 steps (each
-  pass through a Repeat counts) and takes at most 60 seconds (`timeLimit` to change the time;
-  the web build has no clock, so there only the step limit applies).
+  pass through a Repeat counts), passes its commands at most 256 MB of parameters, all steps
+  together, and takes at most 60 seconds (`timeLimit` to change the time; the web build has no
+  clock, so there only the step and parameter limits apply).
 
 ## Tokens
 
@@ -107,17 +108,23 @@ refused rather than read as the header.
    including a blank one and a token, and the policy is checked again on the parameters as they
    are when the step runs. A macro that touches files, or that came with a workbook opened from
    a file, runs once the person approves it: the call fails with `approve: …` and runs with
-   `approve: true`. Approval lasts for the session, until the macro changes.
+   `approve: true`. Approval lasts for the session, until the macro changes. Every step other
+   than an approved file step runs inside the file sandbox that scripts use, so no other
+   parameter reaches a file either.
 2. **Conflicts.** Unless the conflict rule is `overwrite`, each step is checked as it runs. A
    step that writes over cells outside the inputs that held data when the run started, or deletes
    sheets or objects that were there, stops the run, which rolls the workbook back (a file already
-   written stays). The rule is `onConflict` (or the macro's `output.onConflict`): `ask` (the call
+   written stays). Moving such cells, as inserting or deleting rows or cells above them does,
+   counts as writing over them: the check compares cells by place. The rule is `onConflict` (or the macro's `output.onConflict`): `ask` (the call
    fails with `confirm: …`; `overwrite: true` answers it), `fail` or `overwrite`. Shifting the
    data to make room (`shiftDown`, `shiftRight`) needs to know where the macro writes before it
    runs, which this version doesn't work out, so those rules stop the run as `fail` does.
 3. **Run.** One undo step, "Run Macro: Totals". Any error rolls the whole run back and names the
    step: `Step 3.1 (cell.set): …` (paths are 1-based; `2.1` is inside a Repeat, `4.then.1` inside
-   an If). When a step inserts, deletes or moves sheets, rows, columns or cells, the inputs, the
+   an If). The message ends "Nothing was changed.", or names the files the run already wrote,
+   which stay. An input the macro doesn't have, an input that isn't reference text, and
+   `onConflict`, `overwrite`, `approve` or `timeLimit` of the wrong kind are refused rather than
+   ignored. When a step inserts, deletes or moves sheets, rows, columns or cells, the inputs, the
    output and the range of a Repeat in progress move with their cells, as references in formulas
    do; a step that deletes one of them, or changes the size of a range a Repeat is going over,
    stops the run.