address review: make ScriptSandbox !Send so it drops on the thread that entered it

cargo xtask keytips ran every command keytips.json names, with its
params, under the user's file permissions (file.exportPdf could replace
any file, also during --check). It also wrote target, excel and note
strings into generated Rust without escaping. Probes now run inside the
engine's ScriptSandbox, and those fields may not hold quotes,
backslashes, braces or control characters.

crates/engine/src/io.rs

@@ -755,12 +755,12 @@ thread_local! {
 /// While one is alive, [`read_file`] and [`write_file`] on this thread refuse every path: a
 /// stored script's steps run inside one (#2178), so no command reaches the file system from a
 /// script whatever its parameters are called. Dropping it (also on unwind) lifts the refusal.
-pub(crate) struct ScriptSandbox(());
+pub struct ScriptSandbox(std::marker::PhantomData<*const ()>);
 
 impl ScriptSandbox {
-    pub(crate) fn enter() -> Self {
+    pub fn enter() -> Self {
         let _ = SCRIPT_SANDBOX.try_with(|c| c.set(c.get().saturating_add(1)));
-        ScriptSandbox(())
+        ScriptSandbox(std::marker::PhantomData)
     }
 }
 

crates/engine/src/io/tests.rs

@@ -351,6 +351,19 @@ fn script_sandbox_refuses_file_access_until_dropped() {
     let _ = std::fs::remove_dir_all(&dir);
 }
 
+/// The guard's drop lowers the counter of the thread it runs on, so it must not move between threads:
+/// naming the trait's method is ambiguous (a compile error) if `ScriptSandbox` is `Send`.
+#[test]
+fn script_sandbox_is_not_send() {
+    trait AmbiguousIfSend<A> {
+        fn check() {}
+    }
+    impl<T: ?Sized> AmbiguousIfSend<()> for T {}
+    struct IsSend;
+    impl<T: ?Sized + Send> AmbiguousIfSend<IsSend> for T {}
+    <ScriptSandbox as AmbiguousIfSend<_>>::check();
+}
+
 /// A minimal compound (CFB/OLE2) file — FAT in sector 0, directory in sector 1 — holding a
 /// root entry and the given stream names.
 fn cfb(streams: &[&str]) -> Vec<u8> {

crates/xlsx/src/passthrough/write.rs

@@ -254,36 +254,27 @@ pub(crate) fn sheet_names_in(formula: &str) -> Vec<String> {
 /// Whether a kept chart sheet can be written back: its chart's series must still refer to sheets
 /// that exist under the same names (the chart is kept as it was, so a series on a deleted or
 /// renamed sheet would be left pointing at nothing), and no worksheet may have taken its name.
-///
-/// `usable` caches, by relationship id, whether that chart sheet's graph refers only to sheets
-/// that exist (`false` when the relationship is missing): entries sharing one graph are analysed
-/// once, and only the verdict is kept, not the references it came from.
-fn chart_sheet_usable<'p>(wb: &Workbook, p: &'p Preserved, s: &'p PreservedSheet, usable: &mut HashMap<&'p str, bool>) -> bool {
+fn chart_sheet_usable(wb: &Workbook, p: &Preserved, s: &PreservedSheet) -> bool {
     let same = |a: &str, b: &str| a.chars().take(31).collect::<String>().eq_ignore_ascii_case(&b.chars().take(31).collect::<String>());
     if wb.sheets.iter().any(|w| same(&w.name, &s.name)) {
         return false;
     }
-    *usable.entry(s.rel_id.as_str()).or_insert_with(|| {
-        let Some(root) = p.rels.iter().find(|r| r.id == s.rel_id) else { return false };
-        let parts = reachable(p, root);
-        let mut formulas = vec![];
-        for q in p.parts.iter().filter(|q| parts.contains(&normalize(&q.name)) && q.content_type.contains("chart")) {
-            if let Ok(x) = xml::parse(&q.data) {
-                chart_formulas(&x, &mut formulas);
-            }
+    let Some(root) = p.rels.iter().find(|r| r.id == s.rel_id) else { return false };
+    let parts = reachable(p, root);
+    let mut formulas = vec![];
+    for q in p.parts.iter().filter(|q| parts.contains(&normalize(&q.name)) && q.content_type.contains("chart")) {
+        if let Ok(x) = xml::parse(&q.data) {
+            chart_formulas(&x, &mut formulas);
         }
-        formulas.iter().flat_map(|f| sheet_names_in(f)).all(|n| wb.sheets.iter().any(|w| same(&w.name, &n)))
-    })
+    }
+    formulas.iter().flat_map(|f| sheet_names_in(f)).all(|n| wb.sheets.iter().any(|w| same(&w.name, &n)))
 }
 
 /// Writes the workbook's kept parts and relationships. Chart sheets that can't be written back
 /// (see [`chart_sheet_usable`]) are left out with the parts only they reach.
 pub fn write_workbook(wb: &Workbook, wb_rels: &mut Rels, out: &mut Out) -> WorkbookKept {
     let Some(p) = wb.preserved.as_deref() else { return WorkbookKept::default() };
-    let (live, dropped): (Vec<&PreservedSheet>, Vec<&PreservedSheet>) = {
-        let mut refs = HashMap::new();
-        p.sheets.iter().partition(|s| chart_sheet_usable(wb, p, s, &mut refs))
-    };
+    let (live, dropped): (Vec<&PreservedSheet>, Vec<&PreservedSheet>) = p.sheets.iter().partition(|s| chart_sheet_usable(wb, p, s));
     let root_of = |s: &PreservedSheet| p.rels.iter().find(|r| r.id == s.rel_id);
     let dropped_ids: HashSet<&str> = dropped.iter().map(|s| s.rel_id.as_str()).collect();
     // Parts reached only by dropped chart sheets go with them.

crates/xlsx/src/tests/passthrough_tests.rs

@@ -696,32 +696,6 @@ fn chartsheet_package() -> Vec<u8> {
     ])
 }
 
-/// Thousands of chart-sheet entries naming one chart sheet's relationship: its graph is walked
-/// and its charts parsed once when saving, not once per entry.
-#[test]
-fn chart_sheet_entries_sharing_a_graph_are_checked_once() {
-    let bytes = chartsheet_package();
-    let mut z = zip::ZipArchive::new(std::io::Cursor::new(bytes)).unwrap();
-    let mut parts: Vec<(String, Vec<u8>)> = Vec::new();
-    for i in 0..z.len() {
-        let mut f = z.by_index(i).unwrap();
-        let mut data = Vec::new();
-        f.read_to_end(&mut data).unwrap();
-        parts.push((f.name().to_string(), data));
-    }
-    let extra: String = (0..3_000).map(|i| format!(r#"<sheet name="Copy{i}" sheetId="{}" r:id="rId2"/>"#, 100 + i)).collect();
-    for (name, data) in &mut parts {
-        if name == "xl/workbook.xml" {
-            *data = String::from_utf8(data.clone()).unwrap().replace("</sheets>", &format!("{extra}</sheets>")).into_bytes();
-        }
-    }
-    let refs: Vec<(&str, &[u8])> = parts.iter().map(|(n, d)| (n.as_str(), d.as_slice())).collect();
-    let (wb, _) = read_xlsx(&make_zip(&refs)).unwrap();
-    let u = Unpacked::new(&write_xlsx(&wb).unwrap());
-    u.assert_valid();
-    assert!(written_sheets(&u).iter().any(|(n, _)| n == "Chart1"));
-}
-
 /// The sheets of a written workbook: (name, state) in file order.
 fn written_sheets(u: &Unpacked) -> Vec<(String, String)> {
     let wb = xml::parse(u.parts.get("xl/workbook.xml").unwrap()).unwrap();

xtask/src/keytips.rs

@@ -77,6 +77,8 @@ fn probe_live_commands(calls: &[(String, String)]) -> Probe {
         let params = if params.is_empty() { serde_json::json!({}) } else { serde_json::from_str(params).unwrap_or_default() };
         let mut s = Session::new();
         s.new_workbook();
+        // The JSON picks the commands and their params: none of them may touch the file system.
+        let _sandbox = gridcraft_engine::io::ScriptSandbox::enter();
         let status = match s.execute(id, params.clone()) {
             Ok(_) => {
                 if s.journal.iter().any(|(jid, _)| jid == id) {
@@ -181,15 +183,16 @@ fn parse_and_probe(source: &str) -> Result<(Vec<Entry>, Probe), String> {
                 }
             }
         }
-        entries.push(Entry {
-            seq,
-            kind,
-            target,
-            pending,
-            params,
-            excel: e.get("excel").and_then(|v| v.as_str()).unwrap_or("").to_string(),
-            note: e.get("note").and_then(|v| v.as_str()).unwrap_or("").to_string(),
-        });
+        let excel = e.get("excel").and_then(|v| v.as_str()).unwrap_or("").to_string();
+        let note = e.get("note").and_then(|v| v.as_str()).unwrap_or("").to_string();
+        // These are written into generated Rust: a target inside string literals and format
+        // strings, the annotations after `//`. Refuse anything that could end either.
+        for (field, text) in [("target", &target), ("excel", &excel), ("note", &note)] {
+            if text.chars().any(|c| c.is_control() || matches!(c, '"' | '\\' | '{' | '}')) {
+                return Err(format!("{at}: `{field}` must not hold quotes, backslashes, braces or control characters: {text:?}"));
+            }
+        }
+        entries.push(Entry { seq, kind, target, pending, params, excel, note });
     }
     // JSON order is authoritative and curated: the first entry for a tab is its canonical
     // badge (legacy aliases come later), and groups stay together. Do not sort.
@@ -434,3 +437,34 @@ fn reset(h: &mut Harness) {
     key(h, Key::Escape); // leave keytip mode entirely
 }
 "#;
+
+#[cfg(all(test, feature = "engine"))]
+mod tests {
+    use super::*;
+
+    /// Text that would end a comment or string literal in the generated Rust is refused.
+    #[test]
+    fn annotations_and_targets_cannot_escape_generated_code() {
+        for entry in [
+            r#"{"seq": "Q", "kind": "command", "target": "edit.copy", "note": "x\nfn evil() {}"}"#,
+            r#"{"seq": "Q", "kind": "command", "target": "edit.copy", "excel": "a\rb"}"#,
+            r#"{"seq": "Q", "kind": "dialog", "target": "x\"); evil(); (\""}"#,
+            r#"{"seq": "Q", "kind": "control", "target": "{0}"}"#,
+        ] {
+            let doc = format!(r#"{{"sequences": [{entry}]}}"#);
+            assert!(parse_and_probe(&doc).is_err(), "{entry}");
+        }
+        assert!(parse_and_probe(r#"{"sequences": [{"seq": "Q", "kind": "control", "target": "fontSize", "note": "Font Size box"}]}"#).is_ok());
+    }
+
+    /// A probed command may not write files, whatever its params say.
+    #[test]
+    fn probes_cannot_touch_the_file_system() {
+        let path = std::env::temp_dir().join(format!("keytips-probe-{}.pdf", std::process::id()));
+        let _ = fs::remove_file(&path);
+        let calls = vec![("file.exportPdf".to_string(), serde_json::json!({ "path": path.to_string_lossy() }).to_string())];
+        let probe = probe_live_commands(&calls);
+        assert_eq!(probe.values().copied().collect::<Vec<_>>(), [2]);
+        assert!(!path.exists());
+    }
+}