Engine: macro approvals are keyed on the macro's canonical JSON, not a 64-bit hash

Approvals were stored as (document, FNV-1a hash of the macro's JSON).
FNV-1a is not collision resistant, so a crafted workbook could hold a
harmless macro and a different one with the same hash, and approving
the first would let the second run without asking.

The key is now the macro's canonical JSON text itself (object keys
sorted), so two different macros never share an approval.
macros::hash is replaced by macros::fingerprint.
This commit is contained in:
prxnvt
2026-10-22 02:26:32 -04:00
parent b51d588cac
commit 06bbcb504a
5 changed files with 21 additions and 12 deletions
+3 -2
View File
@@ -322,8 +322,9 @@ pub struct Session {
pub recorder: Option<record::Recorder>,
/// 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<usize>,
/// Draw tab tool: "" (select), "pen" or "eraser"; pen colour and width.
+5 -6
View File
@@ -88,12 +88,11 @@ pub fn find(wb: &Workbook, name: &str) -> Option<usize> {
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_<n>_<k>`) instead of the
+2 -2
View File
@@ -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<Json> {
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);
+2 -2
View File
@@ -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]
+9
View File
@@ -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]