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/ui-egui/build.rs

@@ -76,21 +76,7 @@ fn emit_craft_fonts() {
 
 /// One `CraftFont { .. }` initialiser per manifest row GridCraft may embed.
 fn craft_fonts(dir: &std::path::Path, wasm: bool, required: bool) -> Result<String, String> {
-    // Every file embedded comes from inside the checkout: a manifest row (or a symlink) naming a
-    // path outside it would put that file of the build machine into the binary.
-    let root = dir.canonicalize().map_err(|e| format!("{}: {e}", dir.display()))?;
-    let inside = |path: std::path::PathBuf| -> Result<std::path::PathBuf, String> {
-        let real = path.canonicalize().map_err(|e| format!("{}: {e}", path.display()))?;
-        if real.starts_with(&root) { Ok(real) } else { Err(format!("{} is outside the craft-fonts checkout {}", path.display(), root.display())) }
-    };
-    // Registered before validation, by the path as given (and its folder, to see the manifest appear
-    // or a symlink replaced): a build that skipped the fonts must rebuild once they are there.
-    let manifest_path = dir.join("fonts/manifest.txt");
-    println!("cargo::rerun-if-changed={}", manifest_path.display());
-    if let Some(parent) = manifest_path.parent() {
-        println!("cargo::rerun-if-changed={}", parent.display());
-    }
-    let manifest = inside(manifest_path)?;
+    let manifest = dir.join("fonts/manifest.txt");
     println!("cargo::rerun-if-changed={}", manifest.display());
     let text = std::fs::read_to_string(&manifest).map_err(|e| format!("{}: {e}", manifest.display()))?;
     let rows = craft_fonts_policy::parse_manifest(&text)?;
@@ -98,7 +84,7 @@ fn craft_fonts(dir: &std::path::Path, wasm: bool, required: bool) -> Result<Stri
     if required {
         craft_fonts_policy::check_required(&rows)?;
     }
-    let chinese = if wasm { Vec::new() } else { chinese_font_rows(&root, required)? };
+    let chinese = if wasm { Vec::new() } else { chinese_font_rows(dir, required)? };
     for row in rows.iter().chain(&chinese) {
         if craft_fonts_policy::barred(&row.family) {
             println!("cargo::warning=craft-fonts: not embedding {} {} (repository asset policy)", row.family, row.style);
@@ -106,7 +92,7 @@ fn craft_fonts(dir: &std::path::Path, wasm: bool, required: bool) -> Result<Stri
     }
     let mut out = String::new();
     for row in craft_fonts_policy::embeddable(&rows, wasm).into_iter().chain(craft_fonts_policy::embeddable(&chinese, wasm)) {
-        let path = inside(root.join(&row.file)).map_err(|e| format!("{}: {e}", row.file))?;
+        let path = dir.join(&row.file).canonicalize().map_err(|e| format!("{}: {e}", row.file))?;
         println!("cargo::rerun-if-changed={}", path.display());
         let scripts: Vec<String> = row.scripts.iter().map(|s| format!("{s:?}")).collect();
         let family = &row.family;
@@ -134,10 +120,6 @@ fn chinese_font_rows(dir: &std::path::Path, required: bool) -> Result<Vec<craft_
             Ok(Vec::new())
         }
     };
-    // `dir` is the canonical checkout: the manifest itself must not lead out of it either.
-    if manifest.canonicalize().is_ok_and(|real| !real.starts_with(dir)) {
-        return optional(format!("{} is outside the craft-fonts checkout", manifest.display()));
-    }
     match std::fs::read_to_string(&manifest) {
         Ok(text) => match craft_fonts_policy::parse_manifest(&text).and_then(|rows| {
             craft_fonts_policy::check_declared_scripts(&rows)?;

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());
+    }
+}