MCP: render and screenshot PNGs go through a private temp folder (#2573)

The render_photo, render_preview and screenshot tools had the backend write their PNG to lightcraft-mcp-<pid>-<n>-<tag>.png right in the temp dir. On Linux and FreeBSD, where that is the shared /tmp, other users could read the rendered photos and screenshots, and a name someone else created first made the render fail. Each image now goes in a new lightcraft-mcp-<pid>-<random> folder, made 0700 on Unix and never reused when the name already exists, and the folder is removed with anything left in it once the image is read or the call fails. On Windows, and on macOS with its usual per-user TMPDIR, the temp dir was already private; there the PNG just sits one folder deeper.

crates/mcp/src/tests.rs

@@ -539,3 +539,66 @@ fn the_clipboard_tool_cuts_copies_and_pastes_in_the_app() {
     assert!(!r.is_error, "{r:?}");
     assert_eq!(b.0, vec![("ui.clipboard".to_string(), json!({"action": "paste", "text": "travel"}))]);
 }
+
+/// A running app that writes the PNG `ui.render` / `ui.screenshot` ask for. It notes each path and
+/// what its folder looked like then; with `dies_mid_write` it leaves a half-written temp file and
+/// fails instead, like an app that quits part-way through.
+struct PngWriter {
+    seen: Vec<(std::path::PathBuf, Option<std::fs::Metadata>)>,
+    dies_mid_write: bool,
+}
+
+impl Backend for PngWriter {
+    fn call(&mut self, method: &str, params: Value) -> Result<Value, String> {
+        assert!(matches!(method, "ui.render" | "ui.screenshot"), "{method}");
+        let path = std::path::PathBuf::from(params["path"].as_str().unwrap());
+        self.seen.push((path.clone(), path.parent().and_then(|d| std::fs::symlink_metadata(d).ok())));
+        if self.dies_mid_write {
+            std::fs::write(path.with_file_name(".lightcraft-mcp-test.tmp"), b"half a png").unwrap();
+            return Err("connection closed".into());
+        }
+        crate::write_image(&path, &lightcraft_raster::Rgba8::new(1, 1), 92)?;
+        Ok(json!({"width": 1, "height": 1}))
+    }
+    fn has_ui(&self) -> bool {
+        true
+    }
+    fn describe(&self) -> String {
+        "png writer".into()
+    }
+}
+
+/// Renders and screenshots reach the agent through a PNG the app writes. In a shared /tmp other
+/// users could read it or take its predictable name first and make the render fail, so it goes in
+/// a new folder of its own, private on Unix, that is gone afterwards even when the write fails
+/// (#2573).
+#[test]
+fn render_and_screenshot_pngs_go_through_a_private_folder() {
+    let mut b = PngWriter { seen: vec![], dies_mid_write: false };
+    for (tool, args) in [("render_photo", json!({"id": 1})), ("render_preview", json!({"id": 1})), ("screenshot", json!({}))] {
+        let r = call_tool(&mut b, tool, &args);
+        assert!(!r.is_error, "{tool}: {r:?}");
+        assert_eq!(r.image().map(|(_, mime)| mime), Some("image/png"), "{tool}: {r:?}");
+    }
+    b.dies_mid_write = true;
+    assert!(call_tool(&mut b, "screenshot", &json!({})).is_error);
+    assert_eq!(b.seen.len(), 4);
+    let temp = std::env::temp_dir();
+    let mut folders = vec![];
+    for (file, folder) in &b.seen {
+        let dir = file.parent().unwrap();
+        let meta = folder.as_ref().unwrap();
+        assert!(meta.is_dir() && !meta.file_type().is_symlink(), "{}", dir.display());
+        assert_ne!(dir, temp, "{} is right in the shared temp dir", file.display());
+        assert_eq!(dir.parent(), Some(temp.as_path()), "{}", file.display());
+        assert!(dir.file_name().unwrap().to_string_lossy().starts_with("lightcraft-mcp-"), "{}", dir.display());
+        #[cfg(unix)]
+        {
+            use std::os::unix::fs::PermissionsExt;
+            assert_eq!(meta.permissions().mode() & 0o777, 0o700, "{}", dir.display());
+        }
+        assert!(!dir.exists(), "{} was left behind", dir.display());
+        assert!(!folders.contains(&dir), "{} was used twice", dir.display());
+        folders.push(dir);
+    }
+}

crates/mcp/src/tools.rs

@@ -2,6 +2,7 @@
 //! per command in the engine's command registry (`cmd_<id with . → _>`), all forwarded to a
 //! [`Backend`] as control-channel methods.
 
+use std::hash::{BuildHasher, Hasher, RandomState};
 use std::sync::atomic::{AtomicU64, Ordering};
 
 use serde_json::{Map, Value, json};
@@ -437,9 +438,48 @@ fn activate(b: &mut dyn Backend, args: &Value) -> Result<(), String> {
     Ok(())
 }
 
-fn temp_path(tag: &str) -> std::path::PathBuf {
-    static N: AtomicU64 = AtomicU64::new(0);
-    std::env::temp_dir().join(format!("lightcraft-mcp-{}-{}-{tag}.png", std::process::id(), N.fetch_add(1, Ordering::Relaxed)))
+/// Where the backend writes a render or screenshot for us to read back: `<tag>.png` in a new
+/// `lightcraft-mcp-<pid>-<random>` folder in the temp dir, private to this user on Unix (0700).
+/// Straight in a shared /tmp, other users could read the PNG, and a name they took first made the
+/// render fail (#2573). Dropping this removes the folder and whatever is in it.
+struct TempPng {
+    dir: std::path::PathBuf,
+    file: std::path::PathBuf,
+}
+
+impl TempPng {
+    fn new(tag: &str) -> Result<Self, String> {
+        static N: AtomicU64 = AtomicU64::new(0);
+        let temp = std::env::temp_dir();
+        for _ in 0..100 {
+            let mut h = RandomState::new().build_hasher();
+            h.write_u64(N.fetch_add(1, Ordering::Relaxed));
+            let dir = temp.join(format!("lightcraft-mcp-{}-{:016x}", std::process::id(), h.finish()));
+            #[cfg(unix)]
+            let builder = {
+                use std::os::unix::fs::DirBuilderExt;
+                let mut b = std::fs::DirBuilder::new();
+                b.mode(0o700);
+                b
+            };
+            #[cfg(not(unix))]
+            let builder = std::fs::DirBuilder::new();
+            // Never recursive: a name someone else made first fails and is not used.
+            match builder.create(&dir) {
+                Ok(()) => return Ok(Self { file: dir.join(format!("{tag}.png")), dir }),
+                Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => continue,
+                Err(e) => return Err(format!("cannot make a private temporary folder in {}: {e}", temp.display())),
+            }
+        }
+        Err(format!("cannot make a private temporary folder in {}: every name tried is taken", temp.display()))
+    }
+}
+
+impl Drop for TempPng {
+    fn drop(&mut self) {
+        // Also removes the temp file a backend that died mid-write leaves next to the PNG.
+        let _ = std::fs::remove_dir_all(&self.dir);
+    }
 }
 
 /// Read a PNG the backend wrote, optionally downscale / re-encode as JPEG, and wrap it as image
@@ -512,26 +552,32 @@ fn render_photo(b: &mut dyn Backend, args: &Value) -> ToolResult {
         },
     };
     let Some(id) = id else { return ToolResult::error("no active photo: import or select one, or pass `id`") };
-    let file = temp_path("render");
-    let path = file.to_string_lossy().to_string();
+    let tmp = match TempPng::new("render") {
+        Ok(t) => t,
+        Err(e) => return ToolResult::error(e),
+    };
+    let path = tmp.file.to_string_lossy().to_string();
     let meta = match b.call("ui.render", json!({"id": id, "size": size, "path": path})) {
         Ok(m) => m,
         Err(e) => return ToolResult::error(e),
     };
     let mut meta = meta;
     meta["id"] = json!(id);
-    image_result(&file, None, args.get("format").and_then(Value::as_str).unwrap_or("png"), args.get("path").and_then(Value::as_str), meta)
+    image_result(&tmp.file, None, args.get("format").and_then(Value::as_str).unwrap_or("png"), args.get("path").and_then(Value::as_str), meta)
 }
 
 fn screenshot(b: &mut dyn Backend, args: &Value) -> ToolResult {
     if let Err(e) = check_save_path(b, args) {
         return ToolResult::error(e);
     }
-    let file = temp_path("screenshot");
-    let path = file.to_string_lossy().to_string();
+    let tmp = match TempPng::new("screenshot") {
+        Ok(t) => t,
+        Err(e) => return ToolResult::error(e),
+    };
+    let path = tmp.file.to_string_lossy().to_string();
     match b.call("ui.screenshot", json!({"path": path})) {
         Ok(meta) => image_result(
-            &file,
+            &tmp.file,
             Some(args.get("maxSize").and_then(Value::as_u64).unwrap_or(1600).clamp(64, 8192) as u32),
             args.get("format").and_then(Value::as_str).unwrap_or("png"),
             args.get("path").and_then(Value::as_str),