diff --git a/crates/engine/src/cmd/sketch_create.rs b/crates/engine/src/cmd/sketch_create.rs index c6eb0178..f363ae49 100644 --- a/crates/engine/src/cmd/sketch_create.rs +++ b/crates/engine/src/cmd/sketch_create.rs @@ -186,11 +186,8 @@ fn text_source(p: &Value, cmd: &str, old: Option<(String, Vec2, f64, f64, Option Some(_) => return Err(bad(cmd, "`font` must be a font family name or a font file path")), }; let style = text_style(p, cmd, s0)?; - // A font file an agent names must lie in its folders (installed families are found by name). - if let Some(path) = font.as_deref().and_then(solidcraft_sketch::font_file_path) { - solidcraft_io::vfs::check(&path.to_string_lossy()).map_err(|e| bad(cmd, e.to_string()))?; - } - // An unknown font, an unreadable file or a style the font lacks is an error now, not a lost link later. + // An unknown font, an unreadable file or a style the font lacks is an error now, not a lost + // link later; so is a font file outside an agent's folders (the font reader asks `vfs`). solidcraft_sketch::font_data_styled(font.as_deref(), style.bold, style.italic).map_err(|e| bad(cmd, e.to_string()))?; Ok(LinkSource::Text { text, at, height, angle, font, style }) } diff --git a/crates/engine/src/tests/path_scope.rs b/crates/engine/src/tests/path_scope.rs index d0f1ee66..3f2961a1 100644 --- a/crates/engine/src/tests/path_scope.rs +++ b/crates/engine/src/tests/path_scope.rs @@ -64,3 +64,32 @@ fn file_commands_stay_inside_the_agents_folders() { s.execute("file.export", &json!({"path": at(&out, "x.stl")})).unwrap(); let _ = std::fs::remove_dir_all(&base); } + +/// A design names the font file of its text, and opening it reads that file: for an agent, a +/// font outside its folders stays unread, even one already read (and cached) for the user. +#[test] +fn a_designs_font_file_stays_inside_the_agents_folders() { + let base = solidcraft_io::vfs::temp_dir().join(format!("solidcraft-font-scope-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + let (root, out) = (base.join("root"), base.join("out")); + std::fs::create_dir_all(&root).unwrap(); + std::fs::create_dir_all(&out).unwrap(); + let font = out.join("f.ttf"); + std::fs::write(&font, solidcraft_sketch::font_data(None).unwrap().bytes()).unwrap(); + let mut s = Session::default(); + s.execute("sketch.create", &json!({"plane": "XY"})).unwrap(); + s.execute("sketch.text", &json!({"text": "A", "at": [0, 0], "font": font.to_string_lossy()})).unwrap(); + s.execute("sketch.finish", &json!({})).unwrap(); + let design = root.join("text.solidcraft"); + std::fs::write(&design, solidcraft_io::write_design(&s.doc)).unwrap(); + let open = json!({"path": design.to_string_lossy()}); + // The sketch's warning says what its text couldn't read. + let warnings = |s: &Session| s.model.results.iter().filter_map(|r| r.warning.clone()).collect::>().join("; "); + s.execute("doc.open", &open).unwrap(); + assert!(warnings(&s).is_empty(), "the user's own open reads it: {}", warnings(&s)); + scoped(Some(PathScope::new([root.clone()])), || { + s.execute("doc.open", &open).unwrap(); + assert!(warnings(&s).contains("outside the folders this agent may use"), "{}", warnings(&s)); + }); + let _ = std::fs::remove_dir_all(&base); +} diff --git a/crates/io/src/vfs.rs b/crates/io/src/vfs.rs index 1140edfe..9e6b305d 100644 --- a/crates/io/src/vfs.rs +++ b/crates/io/src/vfs.rs @@ -369,6 +369,8 @@ pub fn scoped(scope: Option, f: impl FnOnce() -> T) -> T { SCOPE.with(|s| *s.borrow_mut() = prev); } } + // Sketch text reads the font files a design names, below this crate: it asks here first. + solidcraft_doc::sketch::set_font_file_check(|p| check(&p.to_string_lossy()).map_err(|e| e.to_string())); let _restore = Restore(SCOPE.with(|s| s.replace(scope))); f() } diff --git a/crates/sketch/src/fonts.rs b/crates/sketch/src/fonts.rs index 5843ce19..769959a1 100644 --- a/crates/sketch/src/fonts.rs +++ b/crates/sketch/src/fonts.rs @@ -24,6 +24,15 @@ const MAX_CACHED_BYTES: usize = 128 << 20; /// Longest font name or path accepted. const MAX_FONT_NAME: usize = 1024; +/// Asked before a font file named by its path is read. A design names such files, and an +/// agent's folders (`vfs::PathScope`) live above this crate; the first check set stays. +static FILE_CHECK: OnceLock std::result::Result<(), String>> = OnceLock::new(); + +/// Set the check [`font_data`] runs on a font file's path before reading it. +pub fn set_font_file_check(check: fn(&Path) -> std::result::Result<(), String>) { + let _ = FILE_CHECK.set(check); +} + /// The bytes of a font file and the face to use in it (collections hold several faces). #[derive(Clone)] pub struct FontData { @@ -59,6 +68,10 @@ pub fn font_data(name: Option<&str>) -> Result { } if is_path(name) { let path = expand(name); + // Before the cache too: a file read once for the user is not the agent's to read. + if let Some(check) = FILE_CHECK.get() { + check(&path).map_err(|e| SketchError::Invalid(format!("font file `{name}`: {e}")))?; + } let bytes = read_font(&path).map_err(|e| SketchError::Invalid(format!("font file `{name}`: {e}")))?; let index = 0; ttf_parser::Face::parse(&bytes, index) @@ -306,13 +319,6 @@ fn normalize(s: &str) -> String { s.chars().filter(|c| c.is_alphanumeric()).flat_map(char::to_lowercase).collect() } -/// The file a font name refers to (after `~` and `%VAR%` expansion), or `None` for the built-in -/// font or an installed family, so callers can vet the path before [`font_data`] reads it. -pub fn font_file_path(name: &str) -> Option { - let name = name.trim(); - (is_path(name) && name.len() <= MAX_FONT_NAME).then(|| expand(name)) -} - fn is_path(name: &str) -> bool { name.contains('/') || name.contains('\\') || has_font_extension(name) } diff --git a/crates/sketch/src/lib.rs b/crates/sketch/src/lib.rs index 835fa216..197fd812 100644 --- a/crates/sketch/src/lib.rs +++ b/crates/sketch/src/lib.rs @@ -43,7 +43,7 @@ pub use curves::{ }; pub use decompose::Budget; pub use dims::{DimFrame, DimLayout, chain_centres, default_text, dim_frame, dim_layout, encode_text}; -pub use fonts::{DEFAULT_FONT_FAMILY, FontData, families_in, find_style_in, font_data, font_data_styled, font_file_path, installed_families}; +pub use fonts::{DEFAULT_FONT_FAMILY, FontData, families_in, find_style_in, font_data, font_data_styled, installed_families, set_font_file_check}; pub use freeform_offset::{FreeformOffset, OFFSET_TOLERANCE}; pub use link::{Link, LinkGeom, LinkKind, LinkSource, MAX_LINK_ENTITIES, SKETCH_AXIS_HALF, sketch_axis_geom}; pub use model::{ diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index e6f36e56..cc851035 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -159,20 +159,28 @@ fn batch(app: &mut SolveApp, p: &Value) -> Outcome { ok(json!({"completed": results.len(), "results": results, "undone": 0})) } -pub fn handle(app: &mut SolveApp, ctx: &egui::Context, req: &ControlRequest) -> Outcome { - // A request from an MCP server carries the folders its agent may use; every file the - // request reaches must lie inside them (docs/mcp.md). - let Some(allowed) = req.params.get("allow_paths") else { return dispatch(app, ctx, &req.method, &req.params) }; +/// A request from an MCP server carries the folders its agent may use (`allow_paths`); every +/// file the request reaches must lie inside them (docs/mcp.md). `None`: it names none. +pub fn request_scope(app: &SolveApp, params: &Value) -> Result, String> { + let Some(allowed) = params.get("allow_paths") else { return Ok(None) }; let Some(roots) = allowed.as_array().and_then(|a| a.iter().map(|r| r.as_str().map(std::path::PathBuf::from)).collect::>>()) else { - return err("`allow_paths` must be a list of folders"); + return Err("`allow_paths` must be a list of folders".into()); + }; + // Plus the app's own recovery folder and, when it is a safe one, the open design's folder. + let roots = roots.into_iter().chain(solidcraft_engine::recovery::default_dir()); + Ok(Some(vfs::PathScope::new(roots).with_design(app.session.path.as_deref()))) +} + +pub fn handle(app: &mut SolveApp, ctx: &egui::Context, req: &ControlRequest) -> Outcome { + let scope = match request_scope(app, &req.params) { + Ok(Some(scope)) => scope, + Ok(None) => return dispatch(app, ctx, &req.method, &req.params), + Err(e) => return err(e), }; let mut p = req.params.clone(); if let Some(o) = p.as_object_mut() { o.remove("allow_paths"); } - // Plus the app's own recovery folder and, when it is a safe one, the open design's folder. - let roots = roots.into_iter().chain(solidcraft_engine::recovery::default_dir()); - let scope = vfs::PathScope::new(roots).with_design(app.session.path.as_deref()); vfs::scoped(Some(scope), || dispatch(app, ctx, &req.method, &p)) } @@ -867,6 +875,33 @@ mod tests { let _ = std::fs::remove_dir_all(&base); } + /// An MCP server's `doc.open` and imports load off the window thread (`file_load`), where + /// the agent's folders hold as they do for every other request. + #[test] + fn file_loads_stay_inside_allow_paths() { + let mut h = crate::scenario::Harness::new(); + let base = vfs::temp_dir().join(format!("solidcraft-load-scope-{}", std::process::id())); + let (root, out) = (base.join("root"), base.join("out")); + std::fs::create_dir_all(&root).unwrap(); + std::fs::create_dir_all(&out).unwrap(); + let mut s = Session::default(); + s.execute("solid.box", &json!({"length": 10, "width": 10, "height": 10})).unwrap(); + for d in [&root, &out] { + std::fs::write(d.join("part.solidcraft"), solidcraft_engine::io::write_design(&s.doc)).unwrap(); + } + s.execute("file.export", &json!({"path": out.join("part.step").to_string_lossy()})).unwrap(); + let load = |command: &str, d: &std::path::Path, file: &str| json!({"command": command, "params": {"path": d.join(file).to_string_lossy()}, "allow_paths": [root.to_string_lossy()]}); + for (command, file) in [("doc.open", "part.solidcraft"), ("file.insert_step", "part.step")] { + let (v, _) = h.call_queued("engine.execute", load(command, &out, file)); + assert!(v["ok"] == false && v.to_string().contains("outside the folders this agent may use"), "{command}: {v}"); + } + let (v, _) = h.call_queued("engine.execute", load("doc.open", &root, "part.solidcraft")); + assert_eq!(v["ok"], true, "{v}"); + let (v, _) = h.call_queued("engine.execute", json!({"command": "doc.open", "params": {"path": "x"}, "allow_paths": "/"})); + assert!(v["error"].as_str().is_some_and(|e| e.contains("allow_paths")), "{v}"); + let _ = std::fs::remove_dir_all(&base); + } + /// Each step runs as `engine.execute` runs it: same results, same design. #[test] fn engine_batch_matches_engine_execute() { diff --git a/crates/ui-egui/src/file_load.rs b/crates/ui-egui/src/file_load.rs index b1807533..ea941c33 100644 --- a/crates/ui-egui/src/file_load.rs +++ b/crates/ui-egui/src/file_load.rs @@ -37,11 +37,13 @@ pub struct Request { pub kind: Kind, /// A control-channel caller waiting for the command's reply. pub reply: Option>, + /// The folders that caller's agent may use: the worker reads the file inside them. + pub scope: Option, } impl Request { pub fn new(path: &str, kind: Kind) -> Self { - Request { path: path.into(), kind, reply: None } + Request { path: path.into(), kind, reply: None, scope: None } } fn insert(&self) -> bool { self.kind == Kind::Insert @@ -180,7 +182,14 @@ pub fn control(app: &mut SolveApp, req: crate::control::ControlRequest) -> Optio _ => return Some(req), }; let Some(path) = req.params.get("params").and_then(|p| p.get("path")).and_then(Value::as_str) else { return Some(req) }; - let _ = begin_request(app, Request { path: path.into(), kind, reply: Some(req.reply.clone()) }); + let scope = match crate::control::request_scope(app, &req.params) { + Ok(scope) => scope, + Err(e) => { + let _ = req.reply.send(json!({"ok": false, "error": e})); + return None; + } + }; + let _ = begin_request(app, Request { path: path.into(), kind, reply: Some(req.reply.clone()), scope }); None } @@ -238,6 +247,7 @@ fn start(app: &mut SolveApp) { let (tx, rx) = std::sync::mpsc::channel(); let (preview_tx, preview_rx) = std::sync::mpsc::channel(); let (command, params) = request.command(); + let scope = request.scope.clone(); let thumbnail = request.kind == Kind::Open; let spawned = std::thread::Builder::new().name("file-load".into()).spawn(move || { let preview_began = std::time::Instant::now(); @@ -256,9 +266,10 @@ fn start(app: &mut SolveApp) { } })); } - let result = - std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| compute(scratch, command, ¶ms, thumbnail, &worker_phase, &worker_progress))) - .unwrap_or_else(|_| Err("the file operation failed; the current design is unchanged".into())); + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + compute(scratch, command, ¶ms, scope, thumbnail, &worker_phase, &worker_progress) + })) + .unwrap_or_else(|_| Err("the file operation failed; the current design is unchanged".into())); let _ = tx.send(result); }); match spawned { @@ -364,15 +375,26 @@ fn preview_work_fits(scene: &solidcraft_engine::render::Scene, camera: &solidcra } #[cfg(not(target_arch = "wasm32"))] -fn compute(mut session: Session, command: &str, params: &Value, thumbnail: bool, phase: &AtomicU8, progress: &Arc) -> Job { +fn compute( + mut session: Session, + command: &str, + params: &Value, + scope: Option, + thumbnail: bool, + phase: &AtomicU8, + progress: &Arc, +) -> Job { let profile = solidcraft_engine::geom::env::var_os("PROFILE_OPEN").is_some(); let began = std::time::Instant::now(); let cancelled = || Err("file operation cancelled".to_string()); if progress.is_cancelled() { return cancelled(); } - // The timeline's rebuild reports each feature and stops at the next one after Cancel. - let result = solidcraft_engine::doc::progress::watch(progress.clone(), || session.execute(command, params)).map_err(|e| e.to_string())?; + // The timeline's rebuild reports each feature and stops at the next one after Cancel. An + // agent's scope is per thread, so it is set again here. + let result = + solidcraft_engine::io::vfs::scoped(scope, || solidcraft_engine::doc::progress::watch(progress.clone(), || session.execute(command, params))) + .map_err(|e| e.to_string())?; if progress.is_cancelled() { return cancelled(); } diff --git a/docs/mcp.md b/docs/mcp.md index b423a159..afd5e4fb 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -55,7 +55,8 @@ a new file must go into a folder that exists. On Windows a path must be on a loc file name (alternate data streams), reserved device names with any extension (`NUL`, `CON`, `COM1.txt`, …) and short 8.3 names (`PROGRA~1`) are refused before any file system call, and the resolved path is checked again, so a link or junction that leads to a share is refused too. This -includes a `sketch.text` font given as a file path; installed fonts named by family still work. +includes a `sketch.text` font given as a file path and the font file a design's text names when +the design is opened; installed fonts named by family still work. The server spells its own folders (scratch, recovery) with long names, so they work when the temporary folder's path has a short name (`…\NAME~1.XYZ\AppData\Local\Temp`, as Windows often gives when the user folder's name is long).