diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index d188161ee..caac006bc 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -322,8 +322,9 @@ pub struct Session { pub recorder: Option, /// How many `execute` calls are running (0 between top-level commands). depth: u32, - /// Macros the person allowed to run in this session: (document uid, macro content hash). - pub macro_approvals: std::collections::HashSet<(u64, u64)>, + /// Macros the person allowed to run in this session: (document uid, the macro's content as + /// canonical JSON, from [`macros::fingerprint`]). + pub macro_approvals: std::collections::HashSet<(u64, String)>, /// Windows hidden with View › Hide. pub hidden_windows: Vec, /// Draw tab tool: "" (select), "pen" or "eraser"; pen colour and width. diff --git a/crates/engine/src/macros/mod.rs b/crates/engine/src/macros/mod.rs index 55933c1ac..457f10987 100644 --- a/crates/engine/src/macros/mod.rs +++ b/crates/engine/src/macros/mod.rs @@ -88,12 +88,11 @@ pub fn find(wb: &Workbook, name: &str) -> Option { wb.macros.iter().position(|m| m.name.eq_ignore_ascii_case(name)) } -/// A stable fingerprint of a macro's content: approval to run a macro from a file holds until -/// the macro changes. -pub fn hash(m: &Macro) -> u64 { - // FNV-1a over the macro's JSON. - let text = macro_json(m).to_string(); - text.bytes().fold(0xcbf2_9ce4_8422_2325u64, |h, b| (h ^ u64::from(b)).wrapping_mul(0x0100_0000_01b3)) +/// A macro's content as canonical JSON text (object keys sorted): approval to run a macro holds +/// until the macro changes. It is the text itself rather than a hash, so no two different macros +/// share it. +pub fn fingerprint(m: &Macro) -> String { + macro_json(m).to_string() } /// A copy of `wb` for xlsx with each macro in hidden names (`_gc_macro__`) instead of the diff --git a/crates/engine/src/macros/run.rs b/crates/engine/src/macros/run.rs index 57a70562e..36fd1ac41 100644 --- a/crates/engine/src/macros/run.rs +++ b/crates/engine/src/macros/run.rs @@ -144,7 +144,7 @@ pub fn approve(s: &mut Session, m: &Macro, doc: u64, asks: &[String], approve: b if asks.is_empty() { return Ok(()); } - let key = (doc, super::hash(m)); + let key = (doc, super::fingerprint(m)); if s.macro_approvals.contains(&key) { return Ok(()); } @@ -1227,7 +1227,7 @@ pub fn check(s: &mut Session, name: &str, p: &Json) -> Result { return Ok(out); } }; - let approved = s.macro_approvals.contains(&(doc, super::hash(&m))); + let approved = s.macro_approvals.contains(&(doc, super::fingerprint(&m))); if !asks.is_empty() && !approved { // Nothing from an unapproved macro runs, not even in a dry run. out["needsApproval"] = json!(asks); diff --git a/crates/engine/src/macros/tests.rs b/crates/engine/src/macros/tests.rs index f0845a54b..84792dc40 100644 --- a/crates/engine/src/macros/tests.rs +++ b/crates/engine/src/macros/tests.rs @@ -215,9 +215,9 @@ fn names() { assert_eq!(find(&wb, "Macro2"), None); let a = sample_macro("A"); let mut b = a.clone(); - assert_eq!(hash(&a), hash(&b)); + assert_eq!(fingerprint(&a), fingerprint(&b)); b.steps[0].note = "changed".into(); - assert_ne!(hash(&a), hash(&b)); + assert_ne!(fingerprint(&a), fingerprint(&b)); } #[test] diff --git a/crates/engine/src/tests/macros.rs b/crates/engine/src/tests/macros.rs index a928e9b53..a42a60dd7 100644 --- a/crates/engine/src/tests/macros.rs +++ b/crates/engine/src/tests/macros.rs @@ -486,6 +486,15 @@ fn macros_from_a_file_ask_before_running() { assert_eq!(v(&s, "A1").display(), "ran"); // Approved once per macro content: the second run doesn't ask. s.execute("macro.run", json!({"name": "Theirs"})).unwrap(); + // The approval holds the macro's whole content, not a hash two macros could share. + let m = s.doc().unwrap().wb.macros[0].clone(); + let doc = s.doc().unwrap().uid; + let key: (u64, String) = (doc, crate::macros::macro_json(&m).to_string()); + assert!(s.macro_approvals.contains(&key)); + // The JSON is canonical: the same macro with its keys in another order is the same macro. + let a = crate::macros::parse_macro(&json!({"name": "M", "steps": [{"command": "cell.set", "params": {"cell": "A1", "input": "x"}}]})).unwrap(); + let b = crate::macros::parse_macro(&json!({"steps": [{"params": {"input": "x", "cell": "A1"}, "command": "cell.set"}], "name": "M"})).unwrap(); + assert_eq!(crate::macros::fingerprint(&a), crate::macros::fingerprint(&b)); } #[test]