From fe3717f2e400010a4cd0e7dd4f294e7091c25625 Mon Sep 17 00:00:00 2001 From: bherbruck Date: Tue, 13 Oct 2026 18:51:19 -0400 Subject: [PATCH] fix: control port needs a per-launch token; MCP tool annotations tell the whole truth (#205) (#901) solidcraft --control PORT writes a random per-launch token to a user-only file (/control/PORT.token; 0700/0600 on Unix, icacls on Windows; SOLIDCRAFT_CONTROL_TOKEN overrides) and refuses any connection whose first request lacks it. mcp --connect, the demo and packaging smoke clients send it. MCP execute/batch/open/save/export/screenshot are annotated destructive and open-world, new_design destructive. Docs: control-protocol.md, mcp.md, AGENTS.md, pending release note in releasing.md. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 5 +- CLAUDE.md | 5 +- Cargo.lock | 1 + apps/solidcraft-cli/src/main.rs | 6 +- apps/solidcraft/Cargo.toml | 1 + apps/solidcraft/src/control_server.rs | 124 +++++++++++++++++++++++- apps/solidcraft/src/main.rs | 6 +- crates/engine/src/control_auth.rs | 131 ++++++++++++++++++++++++++ crates/engine/src/lib.rs | 1 + crates/engine/src/recovery.rs | 15 +++ crates/mcp/src/backend.rs | 19 +++- crates/mcp/src/tests.rs | 32 ++++++- crates/mcp/src/tools.rs | 67 +++++++------ docs/control-protocol.md | 42 ++++++++- docs/mcp.md | 14 +++ docs/releasing.md | 16 ++++ examples/demo/enclosure_mcp.py | 21 ++++- packaging/linux/flatpak-smoke.py | 5 +- packaging/linux/snap-smoke.py | 7 +- 19 files changed, 465 insertions(+), 53 deletions(-) create mode 100644 crates/engine/src/control_auth.rs diff --git a/AGENTS.md b/AGENTS.md index fb6123da..3e2fd37a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -122,7 +122,10 @@ before the draft is verified. ## Running and looking at the app - `cargo run --release -p solidcraft -- --sample --control PORT` (sample design + control channel). Pick a free port. Without a display, run it under `Xvfb :99` with `DISPLAY=:99`. -- Drive it with JSON lines on `127.0.0.1:PORT` (see `docs/control-protocol.md`): +- Drive it with JSON lines on `127.0.0.1:PORT` (see `docs/control-protocol.md`). The first + request on a connection carries the launch's token as `"token"`: read it from + `~/.local/share/solidcraft/control/PORT.token`, or start the app with + `SOLIDCRAFT_CONTROL_TOKEN=$(openssl rand -hex 32)` and send that. - `{"id":1,"method":"engine.execute","params":{"command":"solid.extrude","params":{"distance":20}}}` - `{"id":2,"method":"ui.start","params":{"command":"sketch.create"}}` then `ui.click {x, y}` - `{"id":3,"method":"ui.screenshot","params":{"path":"/tmp/shot.png"}}` then read the PNG. diff --git a/CLAUDE.md b/CLAUDE.md index b2003eea..201b44dc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -99,7 +99,10 @@ before the draft is verified. ## Running and looking at the app - `cargo run --release -p solidcraft -- --sample --control PORT` (sample design + control channel). Pick a free port. Without a display, run it under `Xvfb :99` with `DISPLAY=:99`. -- Drive it with JSON lines on `127.0.0.1:PORT` (see `docs/control-protocol.md`): +- Drive it with JSON lines on `127.0.0.1:PORT` (see `docs/control-protocol.md`). The first + request on a connection carries the launch's token as `"token"`: read it from + `~/.local/share/solidcraft/control/PORT.token`, or start the app with + `SOLIDCRAFT_CONTROL_TOKEN=$(openssl rand -hex 32)` and send that. - `{"id":1,"method":"engine.execute","params":{"command":"solid.extrude","params":{"distance":20}}}` - `{"id":2,"method":"ui.start","params":{"command":"sketch.create"}}` then `ui.click {x, y}` - `{"id":3,"method":"ui.screenshot","params":{"path":"/tmp/shot.png"}}` then read the PNG. diff --git a/Cargo.lock b/Cargo.lock index bfcee2cc..951c0af2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3286,6 +3286,7 @@ dependencies = [ "eframe", "egui", "embed-resource", + "getrandom 0.3.4", "hidapi", "log", "quick-xml", diff --git a/apps/solidcraft-cli/src/main.rs b/apps/solidcraft-cli/src/main.rs index de72081b..584c45d0 100644 --- a/apps/solidcraft-cli/src/main.rs +++ b/apps/solidcraft-cli/src/main.rs @@ -9,7 +9,7 @@ //! solidcraft-cli commands the command registry as JSON //! solidcraft-cli recipe translate an oracle recipe to a script //! solidcraft-cli oracle ... replay recipes and compare with expected.json -//! solidcraft-cli mcp [--in design|script] [--connect HOST:PORT] [--allow-path DIR]... [--allow-any-path] +//! solidcraft-cli mcp [--in design|script] [--connect HOST:PORT [--token T]] [--allow-path DIR]... [--allow-any-path] //! MCP server on stdio (docs/mcp.md) //! solidcraft-cli licences SolidCraft's licence and the third-party licences //! solidcraft-cli icon --out F.png|F.ico [--size N] the app icon (packaging; drawn, not a file) @@ -44,7 +44,7 @@ const USAGE: &str = "usage: solidcraft-cli recipe solidcraft-cli oracle ... [--json] solidcraft-cli fuzz [--from N] [--count N] [--jobs J] [--timeout S] [--steps N] [--minimise] [--out DIR] [--report FILE] - solidcraft-cli mcp [--in design.solidcraft|script.json] [--connect 127.0.0.1:PORT] [--allow-path DIR]... [--allow-any-path] + solidcraft-cli mcp [--in design.solidcraft|script.json] [--connect 127.0.0.1:PORT [--token T]] [--allow-path DIR]... [--allow-any-path] solidcraft-cli crashes [list [--json] | show | replay | dir | clear []] solidcraft-cli licences solidcraft-cli icon --out icon.png [--size 256] | --out icon.ico @@ -333,7 +333,7 @@ fn cmd_mcp(args: &[String]) -> Result<(), String> { } let backend: Box = match (flag(args, "--connect"), flag(args, "--in")) { (Some(_), Some(_)) => return Err("mcp: use either --connect or --in, not both".into()), - (Some(addr), None) => Box::new(solidcraft_mcp::Remote::connect(addr).map_err(|e| { + (Some(addr), None) => Box::new(solidcraft_mcp::Remote::connect_with_token(addr, flag(args, "--token")).map_err(|e| { let port = addr.rsplit(':').next().filter(|p| p.parse::().is_ok()).unwrap_or("PORT"); format!( "cannot connect to {addr}: {e}\nno SolidCraft app is listening there; start it with \ diff --git a/apps/solidcraft/Cargo.toml b/apps/solidcraft/Cargo.toml index 01cbcc33..ca1974d7 100644 --- a/apps/solidcraft/Cargo.toml +++ b/apps/solidcraft/Cargo.toml @@ -18,6 +18,7 @@ egui = { workspace = true } serde_json = { workspace = true } log = { workspace = true } rfd = { workspace = true } +getrandom = "0.3" [target.'cfg(any(target_os = "windows", target_os = "linux"))'.dependencies] quick-xml = { workspace = true } diff --git a/apps/solidcraft/src/control_server.rs b/apps/solidcraft/src/control_server.rs index 792b10b3..bb670a6f 100644 --- a/apps/solidcraft/src/control_server.rs +++ b/apps/solidcraft/src/control_server.rs @@ -9,18 +9,39 @@ use std::time::Duration; use serde_json::{Value, json}; use solidcraft_engine::MAX_LINE; +use solidcraft_engine::control_auth; use solidcraft_ui_egui::ControlRequest; -/// Connections served at once. The channel has no authentication beyond being bound to -/// loopback, so a flood must not be able to spawn threads without end. +/// Connections served at once, so a flood must not be able to spawn threads without end. const MAX_CONNECTIONS: usize = 16; /// Lines that are not JSON before the connection is closed, so garbage cannot loop forever. const MAX_BAD_LINES: usize = 8; static OPEN: AtomicUsize = AtomicUsize::new(0); +/// The token file this launch wrote, removed when the app ends. +static TOKEN_FILE: std::sync::OnceLock = std::sync::OnceLock::new(); + +/// A new random token: 32 bytes from the operating system, as hex. +fn new_token() -> Option { + let mut bytes = [0u8; 32]; + getrandom::fill(&mut bytes).ok()?; + Some(bytes.iter().map(|b| format!("{b:02x}")).collect()) +} + +/// Remove this launch's token file (at exit). +pub fn remove_token_file() { + if let Some(p) = TOKEN_FILE.get() { + let _ = std::fs::remove_file(p); + } +} pub fn start(port: u16, ctx: egui::Context) -> Receiver { let (tx, rx) = channel::(); + // Every connection must show the token before anything runs (docs/control-protocol.md). + let Some(token) = control_auth::env_token().or_else(new_token) else { + eprintln!("solidcraft: control server not started: no random token available"); + return rx; + }; let listener = match TcpListener::bind(("127.0.0.1", port)) { Ok(l) => l, Err(e) => { @@ -28,7 +49,23 @@ pub fn start(port: u16, ctx: egui::Context) -> Receiver { return rx; } }; - eprintln!("solidcraft: control server listening on 127.0.0.1:{port}"); + match control_auth::token_path(port) { + Some(path) => match control_auth::write_token_file(&path, &token) { + Ok(()) => { + eprintln!("solidcraft: control server listening on 127.0.0.1:{port} (token in {})", path.display()); + let _ = TOKEN_FILE.set(path); + } + Err(e) => eprintln!( + "solidcraft: control server listening on 127.0.0.1:{port}, but its token file {} could not be written ({e}); \ + clients need SOLIDCRAFT_CONTROL_TOKEN", + path.display() + ), + }, + None => { + eprintln!("solidcraft: control server listening on 127.0.0.1:{port}; no data folder for its token, clients need SOLIDCRAFT_CONTROL_TOKEN") + } + } + let token = std::sync::Arc::new(token); std::thread::spawn(move || { for stream in listener.incoming().flatten() { if OPEN.fetch_add(1, Ordering::Relaxed) >= MAX_CONNECTIONS { @@ -39,8 +76,9 @@ pub fn start(port: u16, ctx: egui::Context) -> Receiver { } let tx = tx.clone(); let ctx = ctx.clone(); + let token = token.clone(); std::thread::spawn(move || { - serve(stream, tx, ctx); + serve(stream, &token, tx, ctx); OPEN.fetch_sub(1, Ordering::Relaxed); }); } @@ -80,11 +118,17 @@ fn read_line(r: &mut impl BufRead, max: usize) -> std::io::Result } } -fn serve(stream: TcpStream, tx: Sender, ctx: egui::Context) { +/// The reply to a connection that didn't show the token; the connection is then closed. +fn unauthorized() -> Value { + json!({"ok": false, "error": "unauthorized: send the control token as \"token\" (see docs/control-protocol.md)"}) +} + +fn serve(stream: TcpStream, token: &str, tx: Sender, ctx: egui::Context) { let Ok(read) = stream.try_clone() else { return }; let mut out = stream; let mut reader = BufReader::new(read); let mut bad = 0usize; + let mut authenticated = false; loop { let line = match read_line(&mut reader, MAX_LINE) { Ok(Some(l)) => String::from_utf8_lossy(&l).into_owned(), @@ -99,6 +143,16 @@ fn serve(stream: TcpStream, tx: Sender, ctx: egui::Context) { } let reply = match serde_json::from_str::(&line) { Ok(msg) => { + // The first request must carry the token; later ones on the same connection may. + if !authenticated { + match msg.get("token").and_then(Value::as_str) { + Some(t) if control_auth::tokens_match(t, token) => authenticated = true, + _ => { + let _ = writeln!(out, "{}", unauthorized()); + break; + } + } + } let id = msg.get("id").cloned().unwrap_or(Value::Null); let method = msg.get("method").and_then(Value::as_str).unwrap_or("").to_string(); let params = msg.get("params").cloned().unwrap_or(json!({})); @@ -113,6 +167,10 @@ fn serve(stream: TcpStream, tx: Sender, ctx: egui::Context) { } r } + Err(_) if !authenticated => { + let _ = writeln!(out, "{}", unauthorized()); + break; + } Err(e) => { bad += 1; json!({"ok": false, "error": format!("bad JSON: {e}")}) @@ -131,6 +189,62 @@ fn serve(stream: TcpStream, tx: Sender, ctx: egui::Context) { mod tests { use super::*; + /// #205: a connection runs nothing until a request carries the launch's token; a wrong or + /// missing token, or garbage, is refused and the connection closed. Once shown, the token + /// need not be repeated on that connection. + #[test] + fn requests_run_only_after_the_token() { + use std::io::Read; + let token = "0123456789abcdef0123456789abcdef"; + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap(); + let (tx, rx) = channel::(); + let ran = std::sync::Arc::new(std::sync::Mutex::new(Vec::::new())); + let seen = ran.clone(); + std::thread::spawn(move || { + for req in rx { + seen.lock().unwrap().push(req.method.clone()); + let _ = req.reply.send(json!({"ok": true, "result": null})); + } + }); + std::thread::spawn(move || { + for stream in listener.incoming().flatten() { + let tx = tx.clone(); + std::thread::spawn(move || serve(stream, token, tx, egui::Context::default())); + } + }); + let session = |lines: &[String]| { + let mut s = TcpStream::connect(addr).unwrap(); + s.set_read_timeout(Some(Duration::from_secs(10))).unwrap(); + for l in lines { + let _ = writeln!(s, "{l}"); + } + let _ = s.shutdown(std::net::Shutdown::Write); + let mut text = String::new(); + let _ = s.read_to_string(&mut text); + text.lines().map(|l| serde_json::from_str::(l).unwrap()).collect::>() + }; + for first in [ + json!({"id": 1, "method": "app.quit", "params": {"force": true}}).to_string(), + json!({"id": 1, "method": "app.quit", "token": "0123456789abcdef0123456789abcdee"}).to_string(), + json!({"id": 1, "method": "app.quit", "token": "0123"}).to_string(), + json!({"id": 1, "method": "app.quit", "token": 7}).to_string(), + "not json".to_string(), + ] { + let replies = session(&[first.clone(), json!({"id": 2, "method": "app.quit", "token": token}).to_string()]); + assert_eq!(replies.len(), 1, "{first}: closed after refusing: {replies:?}"); + assert!(replies[0]["error"].as_str().is_some_and(|e| e.starts_with("unauthorized")), "{first}: {replies:?}"); + } + assert!(ran.lock().unwrap().is_empty(), "nothing ran without the token"); + let replies = session(&[ + json!({"id": 1, "method": "ui.inspect", "token": token}).to_string(), + json!({"id": 2, "method": "engine.commands"}).to_string(), + ]); + assert_eq!(replies.len(), 2, "{replies:?}"); + assert!(replies.iter().all(|r| r["ok"] == true), "{replies:?}"); + assert_eq!(*ran.lock().unwrap(), ["ui.inspect", "engine.commands"]); + } + /// A request longer than the cap is refused instead of allocated. #[test] fn lines_past_the_cap_are_refused() { diff --git a/apps/solidcraft/src/main.rs b/apps/solidcraft/src/main.rs index 8fa1896f..78eb0ccb 100644 --- a/apps/solidcraft/src/main.rs +++ b/apps/solidcraft/src/main.rs @@ -286,7 +286,7 @@ fn main() -> eframe::Result { b.with_x11(); })); } - eframe::run_native( + let result = eframe::run_native( "SolidCraft", options, Box::new(move |cc| { @@ -344,7 +344,9 @@ fn main() -> eframe::Result { space_mouse, })) }), - ) + ); + control_server::remove_token_file(); + result } /// Whether to run through XWayland rather than native Wayland. winit has no file drag-and-drop diff --git a/crates/engine/src/control_auth.rs b/crates/engine/src/control_auth.rs new file mode 100644 index 00000000..0f0af314 --- /dev/null +++ b/crates/engine/src/control_auth.rs @@ -0,0 +1,131 @@ +//! The control channel's per-launch token (docs/control-protocol.md). `solidcraft --control PORT` +//! makes a random token, writes it to a file only the user can read +//! ([`token_path`]) and refuses every connection whose first request doesn't carry it, so other +//! users on the same machine (loopback is shared, on Windows across remote-desktop sessions too) +//! can't drive the app. Clients find it with [`client_token`]. + +use std::io::{Error, ErrorKind, Result, Write}; +use std::path::{Path, PathBuf}; + +/// Shortest token accepted from `SOLIDCRAFT_CONTROL_TOKEN` (hex characters). +pub const MIN_TOKEN_LEN: usize = 32; + +/// The token file of the app listening on `port`: `/control/.token` +/// (`SOLIDCRAFT_CONTROL_DIR` replaces the folder). +pub fn token_path(port: u16) -> Option { + let dir = match solidcraft_geom::env::var_os("CONTROL_DIR").filter(|d| !d.is_empty()) { + Some(d) => PathBuf::from(d), + None => crate::recovery::data_dir()?.join("control"), + }; + Some(dir.join(format!("{port}.token"))) +} + +/// A token set on purpose with `SOLIDCRAFT_CONTROL_TOKEN` (test harnesses, sandboxed packages +/// whose data folder the client can't see), if it is long enough. +pub fn env_token() -> Option { + solidcraft_geom::env::var("CONTROL_TOKEN").map(|t| t.trim().to_string()).filter(|t| t.len() >= MIN_TOKEN_LEN) +} + +/// The token a client sends to the app on `port`: `explicit` (`--token`), else +/// `SOLIDCRAFT_CONTROL_TOKEN`, else the app's token file. +pub fn client_token(port: u16, explicit: Option<&str>) -> Option { + if let Some(t) = explicit.map(str::trim).filter(|t| !t.is_empty()) { + return Some(t.to_string()); + } + if let Some(t) = env_token() { + return Some(t); + } + // Read directly, not through `vfs`: the app's own file, outside any agent's folders. + let text = std::fs::read_to_string(token_path(port)?).ok().filter(|t| t.len() <= 4096)?; + Some(text.trim().to_string()).filter(|t| !t.is_empty()) +} + +/// `a == b`, taking the same time wherever they differ. +pub fn tokens_match(a: &str, b: &str) -> bool { + let (a, b) = (a.as_bytes(), b.as_bytes()); + a.len() == b.len() && a.iter().zip(b).fold(0u8, |acc, (x, y)| acc | (x ^ y)) == 0 +} + +/// Write `token` to `path`, readable and writable only by the user: the folder is made `0700` +/// and the file `0600` on Unix; on Windows the file's inherited permissions are replaced by one +/// entry for the user (`icacls`) before the token goes in. A file left by an earlier launch is +/// replaced. +pub fn write_token_file(path: &Path, token: &str) -> Result<()> { + let dir = path.parent().ok_or_else(|| Error::new(ErrorKind::InvalidInput, "no folder"))?; + std::fs::create_dir_all(dir)?; + let _ = std::fs::remove_file(path); + let mut opts = std::fs::OpenOptions::new(); + opts.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; + std::fs::set_permissions(dir, std::fs::Permissions::from_mode(0o700))?; + opts.mode(0o600); + } + let mut f = opts.open(path)?; + #[cfg(windows)] + restrict_to_user(path)?; + f.write_all(token.as_bytes())?; + f.write_all(b"\n")?; + f.sync_all() +} + +/// Windows: drop inherited entries and grant only the current user full access. +#[cfg(windows)] +fn restrict_to_user(path: &Path) -> Result<()> { + use std::os::windows::process::CommandExt; + let user = std::env::var("USERNAME").map_err(|_| Error::other("USERNAME is not set"))?; + let domain = std::env::var("USERDOMAIN").ok().filter(|d| !d.is_empty()); + let who = domain.map_or(user.clone(), |d| format!("{d}\\{user}")); + let status = std::process::Command::new("icacls") + .arg(path) + .args(["/inheritance:r", "/grant:r", &format!("{who}:F")]) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .creation_flags(0x0800_0000) + .status()?; + if status.success() { Ok(()) } else { Err(Error::other(format!("icacls failed to restrict {}", path.display()))) } +} + +/// Removes the token file when the app ends. +pub struct TokenFile(pub PathBuf); + +impl Drop for TokenFile { + fn drop(&mut self) { + let _ = std::fs::remove_file(&self.0); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn tokens_compare_whole() { + assert!(tokens_match("abc123", "abc123")); + assert!(!tokens_match("abc123", "abc124")); + assert!(!tokens_match("abc123", "abc12")); + assert!(!tokens_match("", "a")); + } + + /// The token file is the user's alone, and a client reads it back. + #[cfg(unix)] + #[test] + fn the_token_file_is_private_to_the_user() { + use std::os::unix::fs::PermissionsExt; + let dir = std::env::temp_dir().join(format!("solidcraft-control-auth-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let path = dir.join("control/7878.token"); + std::fs::create_dir_all(dir.join("control")).unwrap(); + std::fs::write(&path, "stale").unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + write_token_file(&path, "0123456789abcdef0123456789abcdef").unwrap(); + let mode = |p: &Path| std::fs::metadata(p).unwrap().permissions().mode() & 0o777; + assert_eq!(mode(&path), 0o600); + assert_eq!(mode(path.parent().unwrap()), 0o700); + assert_eq!(std::fs::read_to_string(&path).unwrap().trim(), "0123456789abcdef0123456789abcdef"); + drop(TokenFile(path.clone())); + assert!(!path.exists()); + let _ = std::fs::remove_dir_all(&dir); + } +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index eb7e297e..6a1aa4b9 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -8,6 +8,7 @@ #![forbid(unsafe_code)] pub mod cmd; +pub mod control_auth; pub mod crash; pub mod frames; pub mod fuzz; diff --git a/crates/engine/src/recovery.rs b/crates/engine/src/recovery.rs index 55adcdee..1e5c10dd 100644 --- a/crates/engine/src/recovery.rs +++ b/crates/engine/src/recovery.rs @@ -46,6 +46,21 @@ pub fn default_dir() -> Option { Some(dir) } +/// SolidCraft's per-user data folder (the recovery folder's parent unless +/// `SOLIDCRAFT_RECOVERY_DIR` moves that): `%LOCALAPPDATA%\SolidCraft`, +/// `~/Library/Application Support/SolidCraft`, or `$XDG_DATA_HOME/solidcraft` +/// (`~/.local/share/solidcraft`). +pub fn data_dir() -> Option { + let env = |k: &str| std::env::var_os(k).filter(|d| !d.is_empty()).map(PathBuf::from); + if cfg!(windows) { + Some(env("LOCALAPPDATA")?.join("SolidCraft")) + } else if cfg!(target_os = "macos") { + Some(env("HOME")?.join("Library/Application Support/SolidCraft")) + } else { + Some(env("XDG_DATA_HOME").or_else(|| env("HOME").map(|h| h.join(".local/share")))?.join("solidcraft")) + } +} + /// The recovery folder under the per-user data `root`, and the folder earlier releases used. /// Windows and macOS name the app's folder in title case, other systems in lower case. fn dirs_under(root: &Path, title_case: bool) -> (PathBuf, PathBuf) { diff --git a/crates/mcp/src/backend.rs b/crates/mcp/src/backend.rs index aa3559d8..e3811dba 100644 --- a/crates/mcp/src/backend.rs +++ b/crates/mcp/src/backend.rs @@ -31,11 +31,22 @@ pub struct Remote { addr: String, conn: Option<(BufReader, TcpStream)>, next_id: u64, + /// The app's control token, sent with every request (docs/control-protocol.md). + token: Option, } impl Remote { + /// Connect to the app at `addr`, with its control token found the usual way + /// (`SOLIDCRAFT_CONTROL_TOKEN`, else the app's token file for that port). pub fn connect(addr: &str) -> std::io::Result { - let mut r = Self { addr: addr.to_string(), conn: None, next_id: 1 }; + Self::connect_with_token(addr, None) + } + + /// [`Remote::connect`] with the token given (`--token`); `None` finds it the usual way. + pub fn connect_with_token(addr: &str, token: Option<&str>) -> std::io::Result { + let port = addr.rsplit(':').next().and_then(|p| p.parse::().ok()).unwrap_or(0); + let token = solidcraft_engine::control_auth::client_token(port, token); + let mut r = Self { addr: addr.to_string(), conn: None, next_id: 1, token }; r.reconnect()?; Ok(r) } @@ -95,7 +106,11 @@ impl Backend for Remote { if let (Some(scope), Some(o)) = (solidcraft_engine::io::vfs::current_scope(), params.as_object_mut()) { o.insert("allow_paths".into(), json!(scope.roots().iter().map(|r| r.to_string_lossy()).collect::>())); } - let line = json!({"id": id, "method": method, "params": params}).to_string(); + let mut msg = json!({"id": id, "method": method, "params": params}); + if let Some(t) = &self.token { + msg["token"] = json!(t); + } + let line = msg.to_string(); let reply = match self.roundtrip(&line) { Ok(r) => r, // One retry on a fresh connection (the app may have restarted). diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index 11d9217d..66664669 100644 --- a/crates/mcp/src/tests.rs +++ b/crates/mcp/src/tests.rs @@ -544,13 +544,17 @@ fn a_bridged_request_carries_the_agents_folders() { let mut line = String::new(); BufReader::new(stream.try_clone().unwrap()).read_line(&mut line).unwrap(); let req: Value = serde_json::from_str(&line).unwrap(); - writeln!(&stream, "{}", json!({"id": req["id"], "ok": true, "result": req["params"]})).unwrap(); + let mut result = req["params"].clone(); + result["sent_token"] = req["token"].clone(); + writeln!(&stream, "{}", json!({"id": req["id"], "ok": true, "result": result})).unwrap(); }); - let mut b = crate::Remote::connect(&addr).unwrap(); + // The app's control token goes with the request too (#205). + let mut b = crate::Remote::connect_with_token(&addr, Some("0123456789abcdef0123456789abcdef")).unwrap(); let dir = std::fs::canonicalize(std::env::temp_dir()).unwrap(); let got = scoped(Some(PathScope::new([dir.clone()])), || crate::Backend::call(&mut b, "engine.execute", json!({"command": "file.new"}))).unwrap(); assert_eq!(got["allow_paths"], json!([dir.to_string_lossy()]), "{got}"); assert_eq!(got["command"], "file.new"); + assert_eq!(got["sent_token"], "0123456789abcdef0123456789abcdef"); echo.join().unwrap(); } @@ -862,3 +866,27 @@ fn instructions_name_only_real_tools() { assert!(tools.iter().any(|t| t == name), "the instructions name `{name}`, which is not a tool"); } } + +/// Tool annotations never understate what a tool may do (#205): `screenshot` writes a file when +/// given a `path`, and `execute`, `batch`, `save`, `export` and `open` can overwrite or delete +/// files, so none of them is read-only and all are destructive and reach the file system. +#[test] +fn tool_annotations_tell_the_whole_truth() { + let tools = crate::tool_definitions(); + let hints = |name: &str| { + let t = tools.iter().find(|t| t["name"] == name).unwrap_or_else(|| panic!("no tool {name}")); + let a = &t["annotations"]; + (a["readOnlyHint"].as_bool().unwrap(), a["destructiveHint"].as_bool().unwrap(), a["openWorldHint"].as_bool().unwrap()) + }; + for name in ["execute", "batch", "save", "export", "open", "screenshot"] { + assert_eq!(hints(name), (false, true, true), "{name}"); + } + for name in ["list_commands", "inspect_design", "measure", "body_topology", "crash_reports"] { + assert_eq!(hints(name), (true, false, false), "{name}"); + } + assert_eq!(hints("new_design"), (false, true, false)); + for name in ["set_parameter", "undo", "redo"] { + assert_eq!(hints(name), (false, false, false), "{name}"); + } + assert!(tools.iter().all(|t| t["annotations"]["readOnlyHint"].is_boolean()), "every tool is annotated"); +} diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 498790ca..1545e23e 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -23,13 +23,33 @@ fn obj(props: Value, required: &[&str]) -> Value { o } -fn tool(name: &str, title: &str, desc: &str, schema: Value, read_only: bool, destructive: bool) -> Value { +/// What a tool may do, for the client's approval prompts (MCP tool annotations). Clients that +/// auto-approve on these hints must not be told less than the truth (#205). +#[derive(Clone, Copy)] +enum Effect { + /// Only looks. + ReadOnly, + /// Changes the design in memory, undoably. + Edits, + /// Replaces the design in memory (unsaved changes are lost). + Replaces, + /// May read, write, overwrite or delete files on disk (any command, a file path). + Files, +} + +fn tool(name: &str, title: &str, desc: &str, schema: Value, effect: Effect) -> Value { + let (read_only, destructive, open_world) = match effect { + Effect::ReadOnly => (true, false, false), + Effect::Edits => (false, false, false), + Effect::Replaces => (false, true, false), + Effect::Files => (false, true, true), + }; json!({ "name": name, "title": title, "description": desc, "inputSchema": schema, - "annotations": {"title": title, "readOnlyHint": read_only, "destructiveHint": destructive, "idempotentHint": read_only, "openWorldHint": false}, + "annotations": {"title": title, "readOnlyHint": read_only, "destructiveHint": destructive, "idempotentHint": read_only, "openWorldHint": open_world}, }) } @@ -49,8 +69,7 @@ pub fn tool_definitions() -> Vec { substring `filter` over id, label, tab and panel (not the parameter docs, so a command id \ finds that command rather than every command whose docs mention it).", obj(json!({"filter": string("Substring to match, e.g. \"sketch\", \"fillet\", \"pattern\"")}), &[]), - true, - false, + Effect::ReadOnly, ), tool( "execute", @@ -73,8 +92,7 @@ pub fn tool_definitions() -> Vec { }), &["command"], ), - false, - false, + Effect::Files, ), tool( "batch", @@ -95,8 +113,7 @@ pub fn tool_definitions() -> Vec { }), &["commands"], ), - false, - false, + Effect::Files, ), tool( "inspect_design", @@ -112,8 +129,7 @@ pub fn tool_definitions() -> Vec { }), &[], ), - true, - false, + Effect::ReadOnly, ), tool( "measure", @@ -121,8 +137,7 @@ pub fn tool_definitions() -> Vec { "Volume (mm³), surface area (mm²), centre of mass, bounding box and face/edge/vertex counts of one body or all \ bodies, with totals.", obj(json!({"body": string("Body name (default: all bodies)")}), &[]), - true, - false, + Effect::ReadOnly, ), tool( "body_topology", @@ -130,8 +145,7 @@ pub fn tool_definitions() -> Vec { "The edges (index, midpoint, length, endpoints) and faces (index, area, centroid, plane normal) of a body. Use \ an edge midpoint as the [x,y,z] point that picks the edge for fillet and chamfer.", obj(json!({"body": string("Body name, e.g. \"Body1\"")}), &["body"]), - true, - false, + Effect::ReadOnly, ), tool( "set_parameter", @@ -148,8 +162,7 @@ pub fn tool_definitions() -> Vec { }), &["name", "value"], ), - false, - false, + Effect::Edits, ), tool( "screenshot", @@ -171,8 +184,7 @@ pub fn tool_definitions() -> Vec { }), &[], ), - true, - false, + Effect::Files, ), tool( "export", @@ -187,8 +199,7 @@ pub fn tool_definitions() -> Vec { }), &["path"], ), - false, - false, + Effect::Files, ), tool( "crash_reports", @@ -205,18 +216,16 @@ pub fn tool_definitions() -> Vec { }), &[], ), - true, - false, + Effect::ReadOnly, ), - tool("undo", "Undo", "Undo the last change to the design.", obj(json!({}), &[]), false, false), - tool("redo", "Redo", "Redo the last undone change.", obj(json!({}), &[]), false, false), + tool("undo", "Undo", "Undo the last change to the design.", obj(json!({}), &[]), Effect::Edits), + tool("redo", "Redo", "Redo the last undone change.", obj(json!({}), &[]), Effect::Edits), tool( "new_design", "New design", "Start a new, empty design (unsaved changes to the current one are discarded).", obj(json!({"name": string("Design name (default Untitled)")}), &[]), - false, - true, + Effect::Replaces, ), tool( "open", @@ -225,8 +234,7 @@ pub fn tool_definitions() -> Vec { Import base feature) or a 3MF/STL mesh (.3mf/.stl: mesh bodies). Replaces the current design; insert into it \ instead with the file.insert_step (STEP) or file.insert_mesh (3MF/STL) commands.", obj(json!({"path": string("Path to a .solidcraft, .step/.stp, .3mf or .stl file")}), &["path"]), - false, - true, + Effect::Files, ), tool( "save", @@ -236,8 +244,7 @@ pub fn tool_definitions() -> Vec { json!({"path": string("Path to save to (default: the current file)"), "format": {"type": "string", "enum": ["container", "json"], "description": "Design encoding (default: JSON for .json, container otherwise)"}}), &[], ), - false, - false, + Effect::Files, ), ] } diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 0df5ea0d..12a0b52f 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -6,6 +6,42 @@ line `{"id": …, "ok": true, "result": …}` or `{"id": …, "ok": false, "erro handled on the UI thread between frames; pointer and keyboard input is injected as real egui events, one pointer event per frame. +### Token + +Every connection must authenticate. At start the app makes a random 256-bit token for this +launch and writes it, as one line of hex, to a file only the user can read: + +| System | Token file | +|---|---| +| Linux | `$XDG_DATA_HOME/solidcraft/control/PORT.token` (`~/.local/share/solidcraft/control/PORT.token`) | +| macOS | `~/Library/Application Support/SolidCraft/control/PORT.token` | +| Windows | `%LOCALAPPDATA%\SolidCraft\control\PORT.token` | + +On Unix the folder is `0700` and the file `0600`; on Windows the file's inherited permissions are +replaced by a single entry for the user. `SOLIDCRAFT_CONTROL_DIR` moves the folder. The file is +removed when the app exits, and the next launch writes a new token. The token file's path is +printed to stderr at start-up. + +The first request on a connection must carry the token as a top-level `"token"` member +(`{"id": 1, "method": "ui.inspect", "params": {}, "token": "…"}`); later requests on the same +connection may leave it out. A first line without the right token is answered with +`{"ok": false, "error": "unauthorized: …"}` and the connection is closed, without running +anything. Tokens are compared in constant time. + +`solidcraft-cli mcp --connect 127.0.0.1:PORT` reads the token file for that port by itself +(`--token T` or `SOLIDCRAFT_CONTROL_TOKEN` give one explicitly). For a test harness, or a +sandboxed package whose data folder the client can't see, start the app with +`SOLIDCRAFT_CONTROL_TOKEN` set to at least 32 hex characters (for example +`openssl rand -hex 32`) and the app uses that token instead of making one; the file is still +written. A one-line client in Python: + +```python +token = (Path.home() / ".local/share/solidcraft/control/7878.token").read_text().strip() +sock.sendall((json.dumps({"id": 1, "method": "ui.inspect", "params": {}, "token": token}) + "\n").encode()) +``` + +### Limits + The channel is untrusted input. A request line may be at most 16 MiB; a longer one is answered with `{"ok": false, "error": "request too large"}` and the connection is closed, as are connections that send eight lines that are not JSON. At most 16 connections are served at once @@ -19,9 +55,9 @@ reads, writes or deletes (command paths such as `file.export`'s `path` or `doc.r `dir`, `ui.render`, `ui.screenshot`) must then lie inside one of those folders after `..` and symlinks are resolved, or the request fails with `… outside the folders this agent may use …`. The app adds its own recovery folder and, when it lies safely inside the home folder and not in -a hidden folder, the open design's folder (whose hidden files stay out of reach). The bridged MCP server sends its folders this way on every request ([mcp.md](mcp.md)). Without -`allow_paths` file paths are not limited, and the channel itself has no authentication beyond -listening on loopback only. +a hidden folder, the open design's folder (whose hidden files stay out of reach). The bridged +MCP server sends its folders this way on every request ([mcp.md](mcp.md)). Without +`allow_paths` file paths are not limited: a client holding the token can do what the user can. ## Engine diff --git a/docs/mcp.md b/docs/mcp.md index 4978d7f2..1408c3f7 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -19,6 +19,11 @@ solidcraft --control 7878 & solidcraft-cli mcp --connect 127.0.0.1:7878 ``` +Bridged, the server finds the app's control token in the token file the app wrote for that port +([control-protocol.md](control-protocol.md#token)); `--token T` or `SOLIDCRAFT_CONTROL_TOKEN` give +it explicitly when the app runs in a sandbox whose data folder the server can't see (prefer the +variable: command-line arguments are visible to other users in the process list). + stdout carries only protocol messages; errors go to stderr. Bridged screenshots are written to a temporary file by the app and read back by the server, so both must run on the same machine. @@ -67,6 +72,15 @@ folder of the design open in it, under the same rules, so give absolute paths: a resolves against the app's working folder, not the server's. The app's own file dialogs are never limited. +### Tool annotations + +Each tool says what it may do, for clients that ask before running tools (or auto-approve on +these hints). `list_commands`, `inspect_design`, `measure`, `body_topology` and `crash_reports` +are read-only. `set_parameter`, `undo` and `redo` change the design undoably. `new_design` +replaces it (destructive). `execute`, `batch`, `open`, `save`, `export` and `screenshot` (whose +`path` writes a file) can read, write, overwrite or delete files: they are not read-only, +`destructiveHint` and `openWorldHint` are true. + ### Client configuration Claude Code (project `.mcp.json`, or `claude mcp add solidcraft -- solidcraft-cli mcp`; a setup diff --git a/docs/releasing.md b/docs/releasing.md index ca097582..8fce478e 100644 --- a/docs/releasing.md +++ b/docs/releasing.md @@ -98,6 +98,22 @@ new commits were added. An existing tag or published release is never overwritte version after publication. Do not push the version tag yourself: publishing the draft creates it at the draft's target commit. +## Notes for the next release + +Items to carry into the next release's notes (step 3 below), then remove from here: + +- **Security (agents and the control channel, [#205](https://github.com/bherbruck/solidcraft/issues/205)).** + Upgrading is recommended for anyone who drives SolidCraft from an AI agent or runs it with + `--control` on a shared machine. MCP tool calls now reach files only in the open design's + folder (inside home, without its hidden files), SolidCraft's recovery folder and folders + given with `--allow-path`; `..`, symlinks that lead out and Windows UNC, device, stream and + short-name paths are refused ([#578](https://github.com/bherbruck/solidcraft/pull/578), thanks + @kravens). `doc.recovery_discard` deletes only real recovery entries. The control port now + needs a per-launch token from a file only the user can read, so other local users can't drive + the app; `solidcraft-cli mcp --connect` finds it by itself, other clients must send it (see + docs/control-protocol.md). MCP tool annotations no longer understate side effects. Reported by + @CalvinEthos. + ## Verify and publish When the task authorizes shipping a release, agents should complete these steps without diff --git a/examples/demo/enclosure_mcp.py b/examples/demo/enclosure_mcp.py index d8d62777..1faa2f0c 100755 --- a/examples/demo/enclosure_mcp.py +++ b/examples/demo/enclosure_mcp.py @@ -19,6 +19,7 @@ import socket import subprocess import sys import time +from pathlib import Path HERE = os.path.dirname(os.path.abspath(__file__)) @@ -63,10 +64,28 @@ class Mcp: self.p.wait(timeout=10) +def control_token(port): + """The app's control token: SOLIDCRAFT_CONTROL_TOKEN, else the token file the app wrote for + this port (docs/control-protocol.md).""" + if os.environ.get("SOLIDCRAFT_CONTROL_TOKEN"): + return os.environ["SOLIDCRAFT_CONTROL_TOKEN"].strip() + if os.environ.get("SOLIDCRAFT_CONTROL_DIR"): + folder = Path(os.environ["SOLIDCRAFT_CONTROL_DIR"]) + elif sys.platform == "win32": + folder = Path(os.environ["LOCALAPPDATA"]) / "SolidCraft" / "control" + elif sys.platform == "darwin": + folder = Path.home() / "Library/Application Support/SolidCraft/control" + else: + data = os.environ.get("XDG_DATA_HOME") or str(Path.home() / ".local/share") + folder = Path(data) / "solidcraft" / "control" + return (folder / f"{port}.token").read_text().strip() + + def control(port, method, params=None): """One request on the app's control channel (camera moves).""" with socket.create_connection(("127.0.0.1", port)) as s: - s.sendall((json.dumps({"id": 1, "method": method, "params": params or {}}) + "\n").encode()) + msg = {"id": 1, "method": method, "params": params or {}, "token": control_token(port)} + s.sendall((json.dumps(msg) + "\n").encode()) buf = b"" while not buf.endswith(b"\n"): chunk = s.recv(1 << 16) diff --git a/packaging/linux/flatpak-smoke.py b/packaging/linux/flatpak-smoke.py index 71a14962..7b360f73 100644 --- a/packaging/linux/flatpak-smoke.py +++ b/packaging/linux/flatpak-smoke.py @@ -28,6 +28,9 @@ work=$(mktemp -d) export XDG_CONFIG_HOME="$work/config" XDG_DATA_HOME="$work/data" XDG_CACHE_HOME="$work/cache" mkdir -p "$XDG_CONFIG_HOME" "$XDG_DATA_HOME" "$XDG_CACHE_HOME" unset WGPU_BACKEND WGPU_BACKENDS +# The control channel's token (docs/control-protocol.md), set here so this client knows it. +SOLIDCRAFT_CONTROL_TOKEN=$(od -An -N32 -tx1 /dev/urandom | tr -d ' \n') +export SOLIDCRAFT_CONTROL_TOKEN solidcraft --control 18837 >"$work/app.log" 2>&1 & app=$! cleanup() { @@ -44,7 +47,7 @@ for ((attempt=0; attempt<300; attempt++)); do sleep .1 done test "$ready" = 1 -for request in '{"id":1,"method":"ui.inspect","params":{}}' '{"id":2,"method":"engine.execute","params":{"command":"solid.box","params":{"length":30,"width":20,"height":10}}}' '{"id":3,"method":"document.inspect","params":{"measure":true}}'; do +for request in "{\"id\":1,\"method\":\"ui.inspect\",\"params\":{},\"token\":\"$SOLIDCRAFT_CONTROL_TOKEN\"}" '{"id":2,"method":"engine.execute","params":{"command":"solid.box","params":{"length":30,"width":20,"height":10}}}' '{"id":3,"method":"document.inspect","params":{"measure":true}}'; do printf '%s\n' "$request" >&3 IFS= read -r -t 30 reply <&3 printf '%s\n' "$reply" diff --git a/packaging/linux/snap-smoke.py b/packaging/linux/snap-smoke.py index c910296b..1e249198 100755 --- a/packaging/linux/snap-smoke.py +++ b/packaging/linux/snap-smoke.py @@ -17,6 +17,7 @@ import signal import socket import subprocess import sys +import secrets import tempfile import time @@ -26,8 +27,10 @@ flatpak_smoke = importlib.util.module_from_spec(_spec) _spec.loader.exec_module(flatpak_smoke) PORTS = {"x11": 18839, "wayland": 18840} +# The control channel's token (docs/control-protocol.md), given to the app so this client knows it. +TOKEN = secrets.token_hex(32) REQUESTS = [ - {"id": 1, "method": "ui.inspect", "params": {}}, + {"id": 1, "method": "ui.inspect", "params": {}, "token": TOKEN}, {"id": 2, "method": "engine.execute", "params": {"command": "solid.box", "params": {"length": 30, "width": 20, "height": 10}}}, {"id": 3, "method": "document.inspect", "params": {"measure": True}}, @@ -67,7 +70,7 @@ def run_backend(backend, out): runtime = Path(f"/run/user/{os.getuid()}") if not runtime.is_dir(): raise RuntimeError(f"{runtime} is missing: snaps expect the login session's runtime directory") - env = dict(os.environ, XDG_RUNTIME_DIR=str(runtime)) + env = dict(os.environ, XDG_RUNTIME_DIR=str(runtime), SOLIDCRAFT_CONTROL_TOKEN=TOKEN) for name in ("WAYLAND_DISPLAY", "WAYLAND_SOCKET", "DISPLAY", "SOLIDCRAFT_WAYLAND", "SOLVECRAFT_WAYLAND"): env.pop(name, None) # The snap can write to the (non-hidden) home folder, not to the host's /tmp.