From ad79b071ce8dfa22647cb4910530735c7cbb988b Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sun, 25 Oct 2026 02:40:21 -0400 Subject: [PATCH 1/2] Engine: a macro's inputs, output and Repeat ranges follow their cells when a step moves them Inputs, the output and a Repeat's range were bound once, by sheet index and coordinates. A step that inserted a sheet in front of an input's sheet sent every later token to the new sheet, and rows inserted above an input left the tokens on the cells that moved into its place; the run wrote into the wrong cells and reported success. - Commands record the structural edits they make as they rewrite references (`Ctx::edits`; while a macro step runs, `DocState::edits` collects them). - After each step the machine matches the sheets as the run's guard does and moves the inputs, the output and the ranges of the Repeats in progress by those edits, with the formula engine's own adjustment. - A step that deletes an input's or the output's cells, or their sheet, or changes the size of a range a Repeat is going over, stops the run, which rolls back. - Test: `inputs_and_the_output_follow_their_cells_when_a_step_moves_them` (a sheet inserted in front, rows inserted above, an input deleted, a Repeat's range grown). --- crates/engine/src/cmd/edit.rs | 1 + crates/engine/src/cmd/insert.rs | 2 +- crates/engine/src/cmd/mod.rs | 18 +++++++- crates/engine/src/cmd/sheet.rs | 18 ++++---- crates/engine/src/lib.rs | 4 ++ crates/engine/src/macros/run.rs | 68 ++++++++++++++++++++++++++++++- crates/engine/src/tests/macros.rs | 53 ++++++++++++++++++++++++ docs/macros.md | 5 ++- 8 files changed, 155 insertions(+), 14 deletions(-) diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 102edc2d3..556d41759 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -1386,6 +1386,7 @@ fn paste_special(s: &mut Session, p: &Json) -> Result { gridcraft_formula::adjust::Edit::MoveToSheet { from: src, to_sheet: dest_name, to_row: at.row, to_col: at.col } }; rewrite_all_formulas(&mut cx.wb, &src_sheet_name, &e); + cx.edits.push((src_sheet_name.clone(), e)); cx.structural = true; } let end = at.offset_clamped((tiles_r * h) as i64 - 1, (tiles_c * w) as i64 - 1); diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index 3c45031af..0078cb4fe 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -294,7 +294,7 @@ fn insert_table(s: &mut Session, p: &Json) -> Result { let e = Edit::InsertCells { axis: Axis::Rows, range: RangeRef::new(r.start, CellRef::new(r.start.row, r.end.col)) }; shift_cell_features(sh, &e); let name = sh.name.clone(); - rewrite(&mut cx.wb, &name, &e); + rewrite(cx, &name, &e); let sh = cx.sheet_mut(sheet)?; for (i, c) in (r.start.col..=r.end.col).enumerate() { sh.set_value(CellRef::new(r.start.row, c), gridcraft_core::Value::text(format!("Column{}", i + 1))); diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 6f58f18ee..55da0377e 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -340,6 +340,9 @@ pub struct Ctx<'a> { /// The command applied sheet protection itself (row/column inserts and deletes the /// protection allows), so cells it shifts aren't refused as edits of locked cells. pub protection_checked: bool, + /// The structural edits the command made, in order (the sheet's name, the edit), as + /// references were rewritten for them. + pub edits: Vec<(String, gridcraft_formula::adjust::Edit)>, } pub(crate) fn edit(s: &mut Session, f: impl FnOnce(&mut Ctx) -> Result) -> Result { @@ -357,15 +360,26 @@ pub(crate) fn commit(d: &mut DocState, f: impl FnOnce(&mut Ctx) -> Result) #[allow(clippy::disallowed_methods)] let t0 = if cfg!(target_arch = "wasm32") { None } else { std::env::var_os("GRIDCRAFT_PROFILE").map(|_| std::time::Instant::now()) }; let mut sel = d.selection.clone(); - let mut ctx = Ctx { wb: (*d.wb).clone(), changed: Vec::new(), structural: false, sel: &mut sel, fit_rows: Vec::new(), protection_checked: false }; + let mut ctx = Ctx { + wb: (*d.wb).clone(), + changed: Vec::new(), + structural: false, + sel: &mut sel, + fit_rows: Vec::new(), + protection_checked: false, + edits: Vec::new(), + }; let r = f(&mut ctx)?; - let Ctx { mut wb, changed, structural, mut fit_rows, protection_checked, .. } = ctx; + let Ctx { mut wb, changed, structural, mut fit_rows, protection_checked, edits, .. } = ctx; if !protection_checked { check_protection(&d.wb, &wb, &d.unlocked_ranges)?; } if !structural { check_arrays(&d.wb, &wb)?; } + if let Some(log) = d.edits.as_mut() { + log.extend(edits); + } if changed.len() <= 200_000 { fit_rows.extend(changed.iter().map(|(s, c)| (*s, c.row))); } diff --git a/crates/engine/src/cmd/sheet.rs b/crates/engine/src/cmd/sheet.rs index e6caa1dc7..dfab93736 100644 --- a/crates/engine/src/cmd/sheet.rs +++ b/crates/engine/src/cmd/sheet.rs @@ -223,7 +223,9 @@ fn shift_sheet_features(sh: &mut Sheet, axis: Axis, at: u32, count: u32, insert: } /// Rewrites every formula reference (and chart series) for a structural edit on `target`. -pub(super) fn rewrite(wb: &mut gridcraft_model::Workbook, target: &str, e: &Edit) { +pub(super) fn rewrite(cx: &mut Ctx, target: &str, e: &Edit) { + cx.edits.push((target.to_string(), e.clone())); + let wb = &mut cx.wb; super::edit::rewrite_all_formulas(wb, target, e); for si in 0..wb.sheets.len() { let host = wb.sheets.get(si).map(|s| s.name.clone()).unwrap_or_default(); @@ -307,7 +309,7 @@ fn insert_lines(s: &mut Session, p: &Json, axis: Axis) -> Result { } } } - rewrite(&mut cx.wb, &name, &Edit::Insert { axis, at, count }); + rewrite(cx, &name, &Edit::Insert { axis, at, count }); cx.structural = true; Ok(Json::Null) })?; @@ -372,7 +374,7 @@ fn delete_lines(s: &mut Session, p: &Json, axis: Axis) -> Result { } shift_sheet_features(sh, axis, at, count, false); } - rewrite(&mut cx.wb, &name, &Edit::Delete { axis, at, count }); + rewrite(cx, &name, &Edit::Delete { axis, at, count }); } cx.structural = true; Ok(Json::Null) @@ -424,7 +426,7 @@ fn move_columns(s: &mut Session, p: &Json) -> Result { sh.cells.shift_cols(before, count as i64); shift_sheet_features(sh, Axis::Cols, before, count, true); } - rewrite(&mut cx.wb, &name, &Edit::Insert { axis: Axis::Cols, at: before, count }); + rewrite(cx, &name, &Edit::Insert { axis: Axis::Cols, at: before, count }); let mut touched = Vec::new(); { @@ -462,14 +464,14 @@ fn move_columns(s: &mut Session, p: &Json) -> Result { for cell in touched { cx.touch(sheet, cell); } - rewrite(&mut cx.wb, &name, &Edit::Move { from: source, to_row: 0, to_col: destination }); + rewrite(cx, &name, &Edit::Move { from: source, to_row: 0, to_col: destination }); { let sh = cx.sheet_mut(sheet)?; sh.cells.shift_cols(delete_at + count, -(count as i64)); shift_sheet_features(sh, Axis::Cols, delete_at, count, false); } - rewrite(&mut cx.wb, &name, &Edit::Delete { axis: Axis::Cols, at: delete_at, count }); + rewrite(cx, &name, &Edit::Delete { axis: Axis::Cols, at: delete_at, count }); *cx.sel = crate::Selection { active: CellRef::new(active_row, final_start), anchor: CellRef::new(active_row, final_start), @@ -514,7 +516,7 @@ fn insert_cells(s: &mut Session, p: &Json) -> Result { } shift_cell_features(sh, &e); let name = cx.wb.sheet(sheet).map(|s| s.name.clone()).unwrap_or_default(); - rewrite(&mut cx.wb, &name, &e); + rewrite(cx, &name, &e); cx.structural = true; Ok(Json::Null) })?; @@ -549,7 +551,7 @@ fn delete_cells(s: &mut Session, p: &Json) -> Result { shift_cell_features(sh, &e); } // References into the deleted cells become #REF!; the rest move. - rewrite(&mut cx.wb, &name, &e); + rewrite(cx, &name, &e); cx.structural = true; Ok(Json::Null) })?; diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 151c2bc06..23a09c9ee 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -206,6 +206,9 @@ pub struct DocState { /// workbook is opened from a file, carried by content pasted from such a workbook, and never /// changed by a trust decision. pub trust: Trust, + /// While a macro step runs: the structural edits its command made (the sheet's name, the + /// edit), so the macro's inputs, output and Repeat ranges follow their cells as formulas do. + pub edits: Option>, /// The references `edit.goTo` with `record: true` went to in this document, newest first, /// without names, each area written with its sheet (`Sheet1!B14:C22,Sheet1!F19:G30`). pub go_to_recent: Vec, @@ -299,6 +302,7 @@ impl DocState { content_kind: None, scripts_trusted, trust, + edits: None, go_to_recent: vec![], } } diff --git a/crates/engine/src/macros/run.rs b/crates/engine/src/macros/run.rs index 8ae1e3496..36c4cae00 100644 --- a/crates/engine/src/macros/run.rs +++ b/crates/engine/src/macros/run.rs @@ -22,6 +22,7 @@ use std::collections::BTreeSet; use std::sync::Arc; use gridcraft_core::{CellRef, RangeRef, Value}; +use gridcraft_formula::adjust::Edit; use gridcraft_model::{Cell, CompareOp, InputKind, Macro, OnConflict, RepeatOf, Sheet, Step, StepKind, Workbook}; use serde::Serialize; use serde_json::{Value as Json, json}; @@ -645,6 +646,34 @@ impl Machine { } } + /// After a step that inserted, deleted or moved sheets, rows, columns or cells, moves the + /// inputs, the output and the ranges of the Repeats in progress with their cells, as the + /// references in formulas move. A step that deletes one of them, or changes the size of a + /// range a Repeat is going over, stops the run. + fn follow(&mut self, before: &Workbook, after: &Workbook, edits: &[(String, Edit)]) -> std::result::Result<(), String> { + let same_sheets = before.sheets.len() == after.sheets.len() && before.sheets.iter().zip(&after.sheets).all(|(b, a)| b.name == a.name); + if same_sheets && edits.is_empty() { + return Ok(()); + } + let (pairs, _) = pair_sheets(before, after); + let follow = |what: &str, p: Place| follow_place(before, after, &pairs, edits, p).ok_or_else(|| format!("It deleted the cells of {what}.")); + for (name, place, _) in self.inputs.iter_mut() { + *place = follow(name, *place)?; + } + if let Some(o) = self.output { + self.output = Some(follow("the output anchor", o)?); + } + for f in self.frames.iter_mut() { + let Some(l) = f.repeat.as_mut() else { continue }; + let over = follow(&format!("the range Repeat {} goes over", f.prefix), l.over)?; + if (over.range.height(), over.range.width()) != (l.over.range.height(), l.over.range.width()) { + return Err(format!("It changed the size of the range Repeat {} goes over.", f.prefix)); + } + l.over = over; + } + Ok(()) + } + 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() } } @@ -670,9 +699,13 @@ impl Machine { (_, Policy::Allow | Policy::Ask(_)) => {} } self.last_params = Some(resolved.clone()); - let prev = self.guard.as_ref().and_then(|_| s.docs.iter().find(|d| d.uid == self.doc)).map(|d| d.wb.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(); let r = s.execute(id, resolved); + 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. let mut blocked = None; @@ -689,8 +722,11 @@ impl Machine { if let Some(b) = blocked { return Err(fail(b)); } + let after = s.docs.iter().find(|d| d.uid == self.doc).map(|d| d.wb.clone()).ok_or_else(|| fail("The workbook was closed.".into()))?; + if let Some(prev) = &prev { + self.follow(prev, &after, &edits).map_err(fail)?; + } if let (Some(g), Some(prev)) = (self.guard.as_mut(), prev) { - let after = s.docs.iter().find(|d| d.uid == self.doc).map(|d| d.wb.clone()).ok_or_else(|| fail("The workbook was closed.".into()))?; let what = g.check(&prev, &after, &self.inputs); if !what.is_empty() { let msg = format!("It would write over data ({what})."); @@ -701,6 +737,34 @@ impl Machine { } } +/// Where the cells of `p` are after a step: on the same sheet wherever it moved (`pairs`, from +/// [`pair_sheets`]), and moved by each structural edit the step made there, or `None` when the +/// step deleted them (or their sheet). +fn follow_place(before: &Workbook, after: &Workbook, pairs: &[(usize, Option)], edits: &[(String, Edit)], p: Place) -> Option { + let sheet = pairs.iter().find(|(b, _)| *b == p.sheet).and_then(|(_, a)| *a)?; + let mut name = after.sheet(sheet)?.name.clone(); + let old_name = before.sheet(p.sheet).map(|s| s.name.as_str()).unwrap_or_default(); + let mut text = format!("{}!{}", resolve::quote_sheet(&name), p.range.a1()); + for (target, e) in edits { + if target != &name && target != old_name { + continue; + } + let expr = gridcraft_formula::parse(&text).ok()?; + text = gridcraft_formula::print(&gridcraft_formula::adjust::adjust(expr, target, target, e)); + if text.contains("#REF!") { + return None; + } + // A cut and paste can take the cells to another sheet. + let env = Env { wb: after, active_sheet: usize::MAX, inputs: &[], output: None, item: None }; + name = after.sheet(resolve::resolve_ref(&text, &env).ok()?.sheet)?.name.clone(); + } + let env = Env { wb: after, active_sheet: usize::MAX, inputs: &[], output: None, item: None }; + if text.contains('$') { + text = text.replace('$', ""); + } + resolve::resolve_ref(&text, &env).ok() +} + /// If's comparison of a cell's value with a step's `value`, the way Excel's `=`, `<`… compare. fn compare(v: &Value, op: CompareOp, want: &Json) -> bool { let empty = v.is_empty() || v.as_text() == Some(""); diff --git a/crates/engine/src/tests/macros.rs b/crates/engine/src/tests/macros.rs index 262f50af9..68d0b428a 100644 --- a/crates/engine/src/tests/macros.rs +++ b/crates/engine/src/tests/macros.rs @@ -3,6 +3,7 @@ use std::sync::Arc; +use gridcraft_core::CellRef; use serde_json::{Value as Json, json}; use super::{s, v}; @@ -176,6 +177,58 @@ fn conflicts_ask_fail_or_overwrite() { assert_eq!(v(&s, "H1").display(), "400"); } +/// The value of `cell` on the sheet called `sheet`. +fn on(s: &Session, sheet: &str, cell: &str) -> String { + let wb = &s.doc().unwrap().wb; + wb.sheet(wb.sheet_index(sheet).unwrap()).unwrap().value(CellRef::parse(cell).unwrap()).display() +} + +#[test] +fn inputs_and_the_output_follow_their_cells_when_a_step_moves_them() { + // A sheet inserted in front: the input is still on Sheet1, not on whatever sheet is first now. + let mut s = s(); + let steps = json!([ + {"command": "home.insertSheet", "params": {"name": "Summary", "before": 0}}, + {"command": "cell.set", "params": {"cell": "Input", "input": "x"}} + ]); + put(&mut s, json!({"name": "Front", "inputs": [{"name": "Input", "kind": "cell"}], "output": {"onConflict": "overwrite"}, "steps": steps})); + s.execute("macro.run", json!({"name": "Front", "inputs": "Sheet1!B2"})).unwrap(); + assert_eq!((on(&s, "Sheet1", "B2"), on(&s, "Summary", "B2")), ("x".into(), "".into())); + + // Rows inserted above: the input and the output move down with their cells. + let mut s = super::s(); + set(&mut s, "B5", "in"); + let steps = json!([ + {"command": "home.insertRows", "params": {"rows": "2:3"}}, + {"command": "cell.set", "params": {"cell": "Input", "input": "x"}}, + {"command": "cell.set", "params": {"cell": "Output", "input": "y"}} + ]); + put( + &mut s, + json!({"name": "Down", "inputs": [{"name": "Input", "kind": "cell"}], "output": {"anchor": "D5", "onConflict": "overwrite"}, "steps": steps}), + ); + s.execute("macro.run", json!({"name": "Down", "inputs": "B5"})).unwrap(); + assert_eq!((v(&s, "B7").display(), v(&s, "D7").display()), ("x".into(), "y".into())); + assert_eq!((v(&s, "B5").display(), v(&s, "D5").display()), ("".into(), "".into())); + + // A step that deletes an input's cells stops the run. + let mut s = super::s(); + set(&mut s, "B5", "in"); + let steps = json!([{"command": "home.deleteRows", "params": {"rows": "5"}}, {"command": "cell.set", "params": {"cell": "Input", "input": "x"}}]); + put(&mut s, json!({"name": "Gone", "inputs": [{"name": "Input", "kind": "cell"}], "output": {"onConflict": "overwrite"}, "steps": steps})); + let start = s.doc().unwrap().wb.clone(); + let err = s.execute("macro.run", json!({"name": "Gone", "inputs": "B5"})).unwrap_err().to_string(); + assert!(err.contains("Step 1 (home.deleteRows): It deleted the cells of Input."), "{err}"); + assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start)); + + // A step inside a Repeat that changes the size of the range it goes over stops the run. + let mut s = super::s(); + let body = json!([{"command": "home.insertRows", "params": {"rows": "2"}}]); + put(&mut s, json!({"name": "Grow", "output": {"onConflict": "overwrite"}, "steps": [{"type": "repeat", "over": "A1:A3", "body": body}]})); + let err = s.execute("macro.run", json!({"name": "Grow"})).unwrap_err().to_string(); + assert!(err.contains("Step 1.1 (home.insertRows): It changed the size of the range Repeat 1 goes over."), "{err}"); +} + #[test] fn each_step_is_checked_for_writes_over_data() { let dir = std::env::temp_dir().join(format!("gc-macro-guard-{}", std::process::id())); diff --git a/docs/macros.md b/docs/macros.md index daf0a2c35..184cc16ad 100644 --- a/docs/macros.md +++ b/docs/macros.md @@ -117,7 +117,10 @@ refused rather than read as the header. 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). + an If). 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. ## Storage From 65cbcdd6bd96c415700c861510e66a4b6b6a284c Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sun, 25 Oct 2026 02:44:15 -0400 Subject: [PATCH 2/2] 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 | 2 +- crates/engine/src/macros/policy.rs | 13 ++- crates/engine/src/macros/run.rs | 132 ++++++++++++++++++++++++++--- crates/engine/src/tests/macros.rs | 44 ++++++++++ docs/macros.md | 17 ++-- 5 files changed, 189 insertions(+), 19 deletions(-) diff --git a/crates/engine/src/io.rs b/crates/engine/src/io.rs index b8ef1abe9..d47b78520 100644 --- a/crates/engine/src/io.rs +++ b/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(()) } diff --git a/crates/engine/src/macros/policy.rs b/crates/engine/src/macros/policy.rs index 57d974920..24e7724ad 100644 --- a/crates/engine/src/macros/policy.rs +++ b/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"}` diff --git a/crates/engine/src/macros/run.rs b/crates/engine/src/macros/run.rs index 36c4cae00..3d293e609 100644 --- a/crates/engine/src/macros/run.rs +++ b/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, } impl StepError { fn new(step: &str, command: Option<&str>, message: impl Into) -> 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, + /// Files the run wrote ("step 1 (file.exportCsv)"): rolling the run back doesn't take them back. + pub wrote_files: Vec, + /// 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>>, + /// 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, } @@ -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> { + 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 { .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 { 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 { }) } +/// 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::>().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) diff --git a/crates/engine/src/tests/macros.rs b/crates/engine/src/tests/macros.rs index 68d0b428a..0df51122c 100644 --- a/crates/engine/src/tests/macros.rs +++ b/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(); diff --git a/docs/macros.md b/docs/macros.md index 184cc16ad..a3f18ebb6 100644 --- a/docs/macros.md +++ b/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.