address review: register the manifest path before validation and keep the Chinese manifest fallback optional

The build hook canonicalised each manifest row's file and embedded it
with include_bytes! wherever it led, so an absolute path, a .. path or
a symlink in a craft-fonts checkout could put any readable file of the
build machine into the binary. Manifests and font files must now
resolve inside the canonical checkout directory.

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 struct ScriptSandbox(std::marker::PhantomData<*const ()>);
+pub(crate) struct ScriptSandbox(());
 
 impl ScriptSandbox {
-    pub fn enter() -> Self {
+    pub(crate) fn enter() -> Self {
         let _ = SCRIPT_SANDBOX.try_with(|c| c.set(c.get().saturating_add(1)));
-        ScriptSandbox(std::marker::PhantomData)
+        ScriptSandbox(())
     }
 }
 

crates/engine/src/io/tests.rs

@@ -351,19 +351,6 @@ 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,7 +76,21 @@ 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> {
-    let manifest = dir.join("fonts/manifest.txt");
+    // 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)?;
     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)?;
@@ -84,7 +98,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(dir, required)? };
+    let chinese = if wasm { Vec::new() } else { chinese_font_rows(&root, 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);
@@ -92,7 +106,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 = dir.join(&row.file).canonicalize().map_err(|e| format!("{}: {e}", row.file))?;
+        let path = inside(root.join(&row.file)).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;
@@ -120,6 +134,10 @@ 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,8 +77,6 @@ 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) {
@@ -183,16 +181,15 @@ fn parse_and_probe(source: &str) -> Result<(Vec<Entry>, Probe), 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 });
+        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(),
+        });
     }
     // 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.
@@ -437,34 +434,3 @@ 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());
-    }
-}