From 8341959e5ece4a2ddf6bd49d4611515c1ac35f28 Mon Sep 17 00:00:00 2001 From: Bruno-Alumn <171815746+Bruno-Alumn@users.noreply.github.com> Date: Thu, 22 Oct 2026 16:22:43 +0200 Subject: [PATCH] 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---.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-- 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 | 63 ++++++++++++++++++++++++++++++++++++++++ crates/mcp/src/tools.rs | 64 +++++++++++++++++++++++++++++++++++------ 2 files changed, 118 insertions(+), 9 deletions(-) diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index 59e7ba3eb..7cc61a318 100644 --- a/crates/mcp/src/tests.rs +++ b/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)>, + dies_mid_write: bool, +} + +impl Backend for PngWriter { + fn call(&mut self, method: &str, params: Value) -> Result { + 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); + } +} diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 8f782bb9d..11c25f087 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -2,6 +2,7 @@ //! per command in the engine's command registry (`cmd_`), 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: `.png` in a new +/// `lightcraft-mcp--` 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 { + 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),