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