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

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),