From 20d2a3b99861cc4a0e89be6c07fa950f9dedf0ee Mon Sep 17 00:00:00 2001
From: prxnvt <53838599+prxnvt@users.noreply.github.com>
Date: Wed, 21 Oct 2026 15:20:38 -0400
Subject: [PATCH] Model: a macro name with control characters is refused on
 every path

Only parse_macro refused a name holding a line break or other control
character. Macros that came from the macros field of a JSON workbook
were checked with check_limits alone, so a file could bring in a macro
named "a\nb" that macro.save and macro.rename would refuse. The check
now lives in Macro::check_limits, so every path applies it, and such a
macro is dropped with the usual warning when the file is opened.
---
 crates/engine/src/macros/mod.rs   |  3 ---
 crates/engine/src/macros/tests.rs | 13 +++++++++++++
 crates/model/src/macros.rs        |  6 +++++-
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/crates/engine/src/macros/mod.rs b/crates/engine/src/macros/mod.rs
index 141323650..6b3ccdbd8 100644
--- a/crates/engine/src/macros/mod.rs
+++ b/crates/engine/src/macros/mod.rs
@@ -30,9 +30,6 @@ pub fn parse_macro(v: &Json) -> Result<Macro, String> {
     let mut m: Macro = serde_json::from_value(v).map_err(|e| format!("This isn't a valid macro: {e}"))?;
     m.name = m.name.trim().to_string();
     m.check_limits()?;
-    if m.name.chars().any(char::is_control) {
-        return Err("A macro name can't hold line breaks or other control characters.".into());
-    }
     check_size(&m)?;
     Ok(m)
 }
diff --git a/crates/engine/src/macros/tests.rs b/crates/engine/src/macros/tests.rs
index 1b417c8a0..fea61f6c3 100644
--- a/crates/engine/src/macros/tests.rs
+++ b/crates/engine/src/macros/tests.rs
@@ -193,3 +193,16 @@ fn names() {
     assert_eq!(find(&wb, " MACRO1 "), Some(0));
     assert_eq!(find(&wb, "Macro2"), None);
 }
+
+#[test]
+fn json_field_macros_get_the_name_check_too() {
+    // A JSON workbook can't bring in a name that macro.save and macro.rename refuse.
+    let mut wb = Workbook::new();
+    wb.macros.push(Macro { name: "a\nb".into(), ..Macro::default() });
+    wb.macros.push(Macro { name: "Fine".into(), ..Macro::default() });
+    let bytes = crate::io::save_bytes(&wb, "ctl.json").unwrap();
+    let (back, warnings) = crate::io::open_bytes("ctl.json", &bytes).unwrap();
+    let names: Vec<&str> = back.macros.iter().map(|m| m.name.as_str()).collect();
+    assert_eq!(names, ["Fine"]);
+    assert!(warnings.len() == 1 && warnings[0].starts_with("1 macro "), "{warnings:?}");
+}
diff --git a/crates/model/src/macros.rs b/crates/model/src/macros.rs
index fa898984d..caee00805 100644
--- a/crates/model/src/macros.rs
+++ b/crates/model/src/macros.rs
@@ -63,7 +63,8 @@ impl Macro {
         count(&self.steps)
     }
 
-    /// Checks the size limits ([`MAX_MACRO_STEPS`], [`MAX_MACRO_DEPTH`], name and text lengths).
+    /// Checks the size limits ([`MAX_MACRO_STEPS`], [`MAX_MACRO_DEPTH`], name and text lengths) and
+    /// that the name has no control characters.
     pub fn check_limits(&self) -> Result<(), String> {
         let name = self.name.trim();
         if name.is_empty() {
@@ -72,6 +73,9 @@ impl Macro {
         if name.chars().count() > MAX_MACRO_NAME {
             return Err(format!("A macro name can be at most {MAX_MACRO_NAME} characters long."));
         }
+        if name.chars().any(char::is_control) {
+            return Err("A macro name can't hold line breaks or other control characters.".into());
+        }
         if self.description.chars().count() > MAX_MACRO_TEXT {
             return Err(format!("A macro description can be at most {MAX_MACRO_TEXT} characters long."));
         }