Merge pull request #3897 from storytold/fix/audit-automation-roots

Fix clipboard read-policy bypass and FULL lock held during checks (audit automation roots)

apps/vectorcraft/src/clipboard.rs

@@ -143,6 +143,13 @@ fn copied_file(paths: Vec<PathBuf>, mimes: &[&'static str]) -> Option<Flavour> {
             Some(p) if p.ends_with('\r') => PathBuf::from(p.trim_end_matches('\r')),
             _ => path,
         };
+        // A paste sent by automation reads only what its roots allow.
+        match path.to_str() {
+            Some(p) => vectorcraft_engine::file_access::check_read(p).ok()?,
+            // A lossy name would check another file than the one opened.
+            None if vectorcraft_engine::file_access::current().is_none() => {}
+            None => return None,
+        }
         let mut head = Vec::with_capacity(FILE_HEAD);
         std::fs::File::open(&path).ok()?.take(FILE_HEAD as u64).read_to_end(&mut head).ok()?;
         let mime = file_flavour(&path.to_string_lossy(), &head, mimes)?;

crates/doc/src/placed_document.rs

@@ -288,11 +288,23 @@ pub fn set_file_reader(f: FileReader) {
     let _ = FILE_READER.set(f);
 }
 
+/// Whether the caller may read the file at a path (`true`: it may). The engine registers one with
+/// [`set_read_check`]: the cache below is shared by every caller, and a cached file is handed out
+/// only to a caller that may read the file it came from.
+pub type ReadCheck = fn(&str) -> bool;
+
+static READ_CHECK: OnceLock<ReadCheck> = OnceLock::new();
+
+/// Register how a caller's right to read a file is checked (the first registration stays).
+pub fn set_read_check(f: ReadCheck) {
+    let _ = READ_CHECK.set(f);
+}
+
 /// The most bytes of files read again kept.
 const FULL_BYTES: usize = 512 << 20;
 
-/// Files read again, by key, the most recently read last.
-type FullFiles = Vec<(String, Arc<Vec<u8>>)>;
+/// Files read again, by key, with the path each was read from, the most recently read last.
+type FullFiles = Vec<(String, String, Arc<Vec<u8>>)>;
 
 static FULL: Mutex<FullFiles> = Mutex::new(Vec::new());
 
@@ -307,7 +319,11 @@ pub fn full_bytes_cached(p: &PlacedDocument, blob: &ImageBlob) -> Option<Arc<Vec
     if !blob.is_proxy() {
         return Some(blob.bytes.clone());
     }
-    full().iter().rev().find(|(k, _)| *k == p.key).map(|(_, b)| b.clone())
+    let allowed = |path: &str| READ_CHECK.get().is_none_or(|ok| ok(path));
+    // Copy the candidates, newest first, so the filesystem checks run without the lock.
+    let candidates: Vec<(String, Arc<Vec<u8>>)> =
+        full().iter().rev().filter(|(k, _, _)| *k == p.key).map(|(_, path, b)| (path.clone(), b.clone())).collect();
+    candidates.into_iter().find(|(path, _)| allowed(path)).map(|(_, b)| b)
 }
 
 /// The file's bytes: stored, read again already, or read again now ([`set_file_reader`]). `None`
@@ -318,10 +334,10 @@ pub fn full_bytes(p: &PlacedDocument, blob: &ImageBlob) -> Option<Arc<Vec<u8>>>
     }
     let b = Arc::new(FILE_READER.get()?(p)?);
     let mut c = full();
-    c.push((p.key.clone(), b.clone()));
-    let mut total: usize = c.iter().map(|(_, b)| b.len()).sum();
+    c.push((p.key.clone(), p.link.path.clone(), b.clone()));
+    let mut total: usize = c.iter().map(|(_, _, b)| b.len()).sum();
     while total > FULL_BYTES && c.len() > 1 {
-        total = total.saturating_sub(c.remove(0).1.len());
+        total = total.saturating_sub(c.remove(0).2.len());
     }
     Some(b)
 }

crates/engine/src/cmd/fileio/mod.rs

@@ -979,6 +979,10 @@ fn may_write(path: &str) -> Result<()> {
 #[cfg(not(target_arch = "wasm32"))]
 pub(crate) fn read_file(path: &str) -> Result<Vec<u8>> {
     may_read(path)?;
+    // A device or pipe may never end: only a regular file is read to its end.
+    if !std::fs::metadata(path).map_err(|e| EngineError::Other(format!("{path}: {e}")))?.is_file() {
+        return Err(EngineError::Other(format!("{path}: not a file")));
+    }
     std::fs::read(path).map_err(|e| EngineError::Other(format!("{path}: {e}")))
 }
 

crates/engine/src/cmd/findfiles/walk.rs

@@ -259,6 +259,10 @@ pub(crate) fn start_in(
     }
     // The walkers follow no links, so a folder automation may read holds all they read.
     crate::file_access::check_read(&folder.to_string_lossy())?;
+    // The check above reads a lossy string: under roots, a path it would alter names another folder.
+    if folder.to_str().is_none() && crate::file_access::current().is_some() {
+        return Err("automation path rejected: the folder's name isn't valid text".into());
+    }
     let seconds = if limits.seconds.is_finite() { limits.seconds.clamp(0.0, 3600.0) } else { 60.0 };
     let n = pool.reserve(threads.clamp(1, MAX_THREADS));
     if n == 0 {
@@ -279,6 +283,7 @@ pub(crate) fn start_in(
         walk: Mutex::new(Walk { root: Some(folder), alive: n, found: vec![0; items], ..Walk::default() }),
         wake: Condvar::new(),
         pool,
+        scope: crate::file_access::ScopeHandle::capture(),
     });
     let mut spawned = 0;
     for _ in 0..n {
@@ -325,6 +330,8 @@ struct Shared {
     /// Woken when folders are queued, a walker finishes a folder, or the search ends.
     wake: Condvar,
     pool: &'static Pool,
+    /// The automation roots in force when the search started, for the folders the walkers add.
+    scope: crate::file_access::ScopeHandle,
 }
 
 /// The state walkers change under the lock.
@@ -490,7 +497,10 @@ impl Shared {
             }
             queue.push((path, depth + 1));
         }
-        let home = rules.home_below(&listed);
+        // The home folder is added by a rule, not picked: it needs the picked folder's authority.
+        let home = rules
+            .home_below(&listed)
+            .filter(|h| !rules.is_refused(h) && self.scope.clone().run(|| crate::file_access::check_read(&h.to_string_lossy())).is_ok());
         {
             let mut w = self.lock();
             w.files += files;

crates/engine/src/cmd/place/document.rs

@@ -159,7 +159,11 @@ pub(crate) fn read_document(p: &PlacedDocument, bytes: &[u8]) -> Option<(Documen
 /// `p`'s file read again, while it is the file it was read from (how the document model gets it
 /// when only the preview is stored, [`vectorcraft_doc::placed_document::set_file_reader`]).
 pub(crate) fn read_file_again(p: &PlacedDocument) -> Option<Vec<u8>> {
-    let bytes = fileio::read_file(&p.link.path).ok()?;
+    // One byte over the limit tells a file that is too big, without reading it all.
+    let bytes = fileio::read_file_head(&p.link.path, MAX_FILE + 1).ok()?;
+    if bytes.len() > MAX_FILE {
+        return None;
+    }
     if p.link.hash.as_ref().is_some_and(|h| *h != hash_bytes(&bytes)) {
         return None;
     }
@@ -186,7 +190,10 @@ pub(crate) struct Source {
 pub(crate) fn source(name: &str, bytes: &[u8], path: Option<&str>, page: u32, bounding: bool, page_box: PageBox, cmd: &str) -> Result<Source> {
     // The same document placed the same way is read once (placing it again is free).
     let key = key_for(&hash_bytes(bytes), page, bounding, page_box);
-    if let Some(s) = lock_memo().iter().find(|s| s.key == key).cloned() {
+    // Resolving its links depends on where the file is and on what the caller may read, so a hit
+    // needs the same path, and nothing is shared with or from a confined (automation) caller.
+    let confined = crate::file_access::current().is_some();
+    if !confined && let Some((_, s)) = lock_memo().iter().find(|(at, s)| s.key == key && at.as_deref() == path).cloned() {
         return Ok(s);
     }
     let a = document_art(name, bytes, path, page, bounding, page_box, cmd)?;
@@ -206,11 +213,13 @@ pub(crate) fn source(name: &str, bytes: &[u8], path: Option<&str>, page: u32, bo
     // Drawing it needn't read it again.
     vectorcraft_doc::placed_document::prime(&shown, blob.bytes.clone(), (a.doc, art, a.frame));
     let s = Source { blob, key, page_box, frame: a.frame, warnings: a.warnings };
-    let mut m = lock_memo();
-    if m.len() >= MEMO_SIZE {
-        m.remove(0);
+    if !confined {
+        let mut m = lock_memo();
+        if m.len() >= MEMO_SIZE {
+            m.remove(0);
+        }
+        m.push((path.map(str::to_string), s.clone()));
     }
-    m.push(s.clone());
     Ok(s)
 }
 
@@ -218,9 +227,9 @@ pub(crate) fn source(name: &str, bytes: &[u8], path: Option<&str>, page: u32, bo
 const MEMO_SIZE: usize = 32;
 
 /// Files read lately for placing, by key.
-static MEMO: std::sync::Mutex<Vec<Source>> = std::sync::Mutex::new(Vec::new());
+static MEMO: std::sync::Mutex<Vec<(Option<String>, Source)>> = std::sync::Mutex::new(Vec::new());
 
-fn lock_memo() -> std::sync::MutexGuard<'static, Vec<Source>> {
+fn lock_memo() -> std::sync::MutexGuard<'static, Vec<(Option<String>, Source)>> {
     // Entries are only pushed and removed: a panic while held leaves it whole.
     MEMO.lock().unwrap_or_else(std::sync::PoisonError::into_inner)
 }

crates/engine/src/cmd/plugin.rs

@@ -350,7 +350,12 @@ fn load_folder(folder: &str) -> Value {
         return json!({"folder": folder, "error": e});
     }
     // A file in it may be a link to one elsewhere.
-    match registry::load_folder_where(std::path::Path::new(folder), |f| crate::file_access::check_read(&f.to_string_lossy())) {
+    match registry::load_folder_where(std::path::Path::new(folder), |f| match f.to_str() {
+        Some(f) => crate::file_access::check_read(f),
+        // A lossy name would check another file than the one loaded.
+        None if crate::file_access::current().is_none() => Ok(()),
+        None => Err(format!("automation path rejected: {} isn't valid text", f.display())),
+    }) {
         Ok(r) => {
             json!({"folder": folder, "loaded": r.loaded, "failed": r.failed.iter().map(|(f, e)| json!({"file": f, "error": e})).collect::<Vec<_>>()})
         }

crates/engine/src/cmd/prefscmds.rs

@@ -472,13 +472,14 @@ impl Prefs {
 
     /// The folders the app reads (fonts, plug-ins) or writes (Data Recovery copies, the Templates
     /// folder it makes for its dialogs) on its own, with the access each needs.
-    fn folders_mut(&mut self) -> [(&mut String, crate::file_access::Access); 4] {
+    fn folders_mut(&mut self) -> [(&mut String, &'static [crate::file_access::Access]); 4] {
         use crate::file_access::Access;
         [
-            (&mut self.fonts_folder, Access::Read),
-            (&mut self.plugins_folder, Access::Read),
-            (&mut self.recovery_folder, Access::Write),
-            (&mut self.templates_folder, Access::Write),
+            (&mut self.fonts_folder, &[Access::Read][..]),
+            (&mut self.plugins_folder, &[Access::Read][..]),
+            // Data Recovery lists and restores what is in its folder too.
+            (&mut self.recovery_folder, &[Access::Write, Access::Read][..]),
+            (&mut self.templates_folder, &[Access::Write][..]),
         ]
     }
 
@@ -494,7 +495,7 @@ impl Prefs {
             if folder.is_empty() || folder == old.trim() {
                 continue;
             }
-            if let Err(e) = roots.check(folder, access) {
+            if let Err(e) = access.iter().try_for_each(|a| roots.check(folder, *a)) {
                 refused.push(e);
                 *new = old.clone();
             }

crates/engine/src/file_access.rs

@@ -252,8 +252,9 @@ fn resolve(path: &str) -> Result<PathBuf, String> {
     }
 }
 
-/// Is `path` (resolved) `root` or inside it, component by component (without regard to case on
-/// Windows, whose file systems ignore it)?
+/// Is `path` (resolved) `root` or inside it, component by component (without regard to case in a
+/// Windows drive prefix only: resolved folder names have their real case, and a folder that differs
+/// in case alone may be another, on a case-sensitive Windows folder)?
 fn inside(path: &Path, root: &Path) -> bool {
     let mut path = path.components();
     root.components().all(|r| path.next().is_some_and(|p| same_component(p, r)))
@@ -263,7 +264,7 @@ fn same_component(a: Component, b: Component) -> bool {
     if a == b {
         return true;
     }
-    if !cfg!(windows) {
+    if !cfg!(windows) || !matches!((a, b), (Component::Prefix(_), Component::Prefix(_))) {
         return false;
     }
     match (a.as_os_str().to_str(), b.as_os_str().to_str()) {

crates/engine/src/lib.rs

@@ -1486,6 +1486,12 @@ impl Session {
         // Placed documents are read by the native format's loader.
         vectorcraft_doc::placed_document::set_loader(cmd::place::document::read_document);
         vectorcraft_doc::placed_document::set_file_reader(cmd::place::document::read_file_again);
+        vectorcraft_doc::placed_document::set_read_check(|path| file_access::check_read(path).is_ok());
+        // The background workers of placed documents read files in the scope that asked.
+        vectorcraft_render::placed_document::set_scope_capture(|| {
+            let scope = file_access::ScopeHandle::capture();
+            Box::new(move |f: &mut dyn FnMut()| scope.run(f))
+        });
         // Clicks find an image's own fills and strokes where their effects draw them.
         vectorcraft_doc::hit::set_image_paint(vectorcraft_render::effects::image_paint);
         // OpenType SVG glyphs are drawn as the SVG importer reads their documents.

crates/engine/src/tooling.rs

@@ -564,6 +564,13 @@ impl Session {
         if let Some(k) = p.get("key").and_then(Value::as_str) {
             values.insert(k.to_string(), p.get("value").cloned().unwrap_or(Value::Null));
         }
+        // The Place queue is read later, by a click outside any automation scope: its files must
+        // be readable now, under the roots of whoever sets it.
+        if let Some(entries) = values.get("queue").and_then(|q| q.get("entries")).and_then(Value::as_array) {
+            for path in entries.iter().filter_map(|e| e.pointer("/params/path").and_then(Value::as_str)) {
+                crate::file_access::check_read(path)?;
+            }
+        }
         if tool.unwrap_or(self.tool_id()) == "dimension" {
             if let Some(style) = values.get("style") {
                 vectorcraft_doc::dimension::DimensionOptions::from_params(style.clone())?;

crates/mcp/src/headless.rs

@@ -279,7 +279,8 @@ impl Headless {
     /// `app.open {path}`: any readable file as a new active document (`document.open`).
     pub fn open(&mut self, p: &Value) -> Result<Value, String> {
         let path = s(p, "path").ok_or("missing `path`")?;
-        self.session.execute("document.open", &json!({"path": path})).map_err(|e| e.to_string())
+        let roots = self.roots.clone();
+        file_access::confine(roots.as_ref(), || self.session.execute("document.open", &json!({"path": path}))).map_err(|e| e.to_string())
     }
 
     /// `app.save {path?, format?, options?}`: `document.save` (by default the document's own path and
@@ -289,12 +290,14 @@ impl Headless {
         if s(p, "path").is_none() && st.path.is_none() {
             return Err("missing `path` (document was never saved)".into());
         }
-        self.session.execute("document.save", p).map_err(|e| e.to_string())
+        let roots = self.roots.clone();
+        file_access::confine(roots.as_ref(), || self.session.execute("document.save", p)).map_err(|e| e.to_string())
     }
 
     /// `app.export {path?, format?, …}`: `document.export` (format defaults to the path's extension).
     pub fn export(&mut self, p: &Value) -> Result<Value, String> {
-        self.session.execute("document.export", p).map_err(|e| e.to_string())
+        let roots = self.roots.clone();
+        file_access::confine(roots.as_ref(), || self.session.execute("document.export", p)).map_err(|e| e.to_string())
     }
 }
 

crates/mcp/tests/automation_roots.rs

@@ -114,7 +114,13 @@ fn saves_exports_and_captures_go_only_into_the_write_root() {
         json!({"command": "prefs.set", "params": {"key": "recoveryFolder", "value": s(&base.join("outside"))}}),
         "outside the write root",
     );
-    ok(&mut server, "run_command", json!({"command": "prefs.set", "params": {"key": "recoveryFolder", "value": s(&base.join("out"))}}));
+    // Data Recovery lists and restores what is in its folder too: write access alone isn't enough.
+    refused(
+        &mut server,
+        "run_command",
+        json!({"command": "prefs.set", "params": {"key": "recoveryFolder", "value": s(&base.join("out"))}}),
+        "outside the read root",
+    );
     refused(&mut server, "run_command", json!({"command": "plugin.reload", "params": {"path": s(&base.join("outside"))}}), "outside the read root");
 
     // The document saved inside the write root saves again in place.
@@ -155,3 +161,12 @@ fn links_out_of_a_root_are_refused() {
     refused(&mut server, "export", json!({"path": s(&base.join("out/away/art.png"))}), "outside the write root");
     assert!(!base.join("outside/art.png").exists());
 }
+
+/// The Place tool keeps its queue until a click, which no scope confines: a queued file outside the
+/// read root is refused when the queue is set (whichever tool is active when it is).
+#[test]
+fn a_place_queue_names_only_files_inside_the_read_root() {
+    let (mut server, base) = confined("place-queue", true, true);
+    let queue = |file: &str| json!({"key": "queue", "value": {"entries": [{"params": {"path": s(&base.join(file))}, "name": "x", "width": 10, "height": 10}]}});
+    refused(&mut server, "run_command", json!({"command": "tool.setOption", "params": queue("outside/b.svg")}), "outside the read root");
+}

crates/render/src/placed_document.rs

@@ -120,12 +120,25 @@ impl Bitmaps {
     }
 }
 
+/// Runs a closure in the file-access scope of whoever scheduled a job (see [`set_scope_capture`]).
+pub type Scoped = Box<dyn FnOnce(&mut dyn FnMut()) + Send>;
+
+static SCOPE_CAPTURE: std::sync::OnceLock<fn() -> Scoped> = std::sync::OnceLock::new();
+
+/// Register how the scope the caller reads files in is carried to the background workers, which
+/// read placed files too (the first registration stays). The engine registers its automation roots.
+pub fn set_scope_capture(f: fn() -> Scoped) {
+    let _ = SCOPE_CAPTURE.set(f);
+}
+
 /// What a bitmap is made from.
 struct Job {
     key: BitmapKey,
     object: PlacedDocument,
     blob: ImageBlob,
     step: f64,
+    /// The scheduler's file-access scope, which the worker runs the job in.
+    scope: Option<Scoped>,
 }
 
 /// Device pixels per point of `object`'s box its preview `bytes` have.
@@ -192,12 +205,19 @@ fn pixels(v: f64) -> u16 {
 }
 
 /// Make `job`'s bitmap and keep it.
-fn run(r: &mut Renderer, job: Job) {
-    if let Some(pm) = make(r, &job.object, &job.blob, job.step) {
-        lock(&BITMAPS).insert(job.key.clone(), pm, job.step);
+fn run(r: &mut Renderer, mut job: Job) {
+    let scope = job.scope.take();
+    let mut body = || {
+        if let Some(pm) = make(r, &job.object, &job.blob, job.step) {
+            lock(&BITMAPS).insert(job.key.clone(), pm, job.step);
+        }
+        lock(&PENDING).remove(&job.key);
+        GENERATION.fetch_add(1, Ordering::Relaxed);
+    };
+    match scope {
+        Some(scoped) => scoped(&mut body),
+        None => body(),
     }
-    lock(&PENDING).remove(&job.key);
-    GENERATION.fetch_add(1, Ordering::Relaxed);
 }
 
 #[cfg(not(target_arch = "wasm32"))]
@@ -317,7 +337,7 @@ impl Renderer {
         let shown = match found {
             Some(pm) => Some(pm),
             None => {
-                schedule(Job { key, object: p.clone(), blob, step });
+                schedule(Job { key, object: p.clone(), blob, step, scope: SCOPE_CAPTURE.get().map(|capture| capture()) });
                 // Made here and now (no threads) or meanwhile another step's.
                 let mut b = lock(&BITMAPS);
                 b.get(&(p.key.clone(), step.to_bits())).or(nearest.map(|n| n.0))

crates/ui-egui/src/dialogs/package.rs

@@ -204,7 +204,12 @@ fn package_status_note(missing: usize, fonts: &str) -> String {
 
 /// `file.showPackage {folder}`: show a package folder in the file manager.
 pub fn show_package(app: &mut VectorcraftApp, p: &Value) -> Result<Value, String> {
+    app.refuse_automation("file.showPackage")?;
     let folder = p.get("folder").and_then(Value::as_str).ok_or("missing `folder`")?.to_string();
+    // A package is a folder: an executable, shortcut or app bundle would run, not be shown.
+    if !std::path::Path::new(&folder).is_dir() || folder.to_ascii_lowercase().ends_with(".app") {
+        return Err(format!("{folder} is not a package folder"));
+    }
     io::open_in_app(app, &folder)?;
     Ok(json!({ "folder": folder }))
 }