Automation: --automation-read-root and --automation-write-root may each be given more than once (#3546)

Squashed from #3631 onto main.

crates/engine/src/file_access.rs

@@ -2,15 +2,16 @@
 //!
 //! `vectorcraft-cli mcp --automation-read-root <dir> --automation-write-root <dir>`, and
 //! `vectorcraft --control <port>` with the same flags, confine the files an agent's commands
-//! reach. [`AutomationRoots`] holds the two folders. While they are in force, every path a command
-//! reads goes through [`check_read`] and every path it writes through [`check_write`]:
+//! reach; each flag may be given more than once (#3546). [`AutomationRoots`] holds the folders of
+//! each kind. While they are in force, every path a command reads goes through [`check_read`] and
+//! every path it writes through [`check_write`]:
 //!
 //! - The path is made absolute against the working directory, as the file system reads it. Its
 //!   links are followed: for a file that doesn't exist yet, the links of its nearest existing
 //!   folder, the rest of the path being plain names (`..` below a missing folder is refused, and so
 //!   is a link that leads nowhere).
-//! - The result must lie inside the root, compared by whole components (`/work2` isn't inside
-//!   `/work`), without regard to case on Windows.
+//! - The result must lie inside one of the roots of its kind, compared by whole components
+//!   (`/work2` isn't inside `/work`), without regard to case on Windows.
 //! - Read and write access are separate: a root given alone grants only its own kind of access.
 //!
 //! Roots are in force on a thread inside [`confine`], and in the whole process after
@@ -35,11 +36,11 @@ pub enum Access {
     Write,
 }
 
-/// The folders automation may read and write; a root left out grants no access of its kind.
+/// The folders automation may read and write; a kind with no root grants no access of its kind.
 #[derive(Debug)]
 pub struct AutomationRoots {
-    read: Option<Root>,
-    write: Option<Root>,
+    read: Vec<Root>,
+    write: Vec<Root>,
 }
 
 #[derive(Debug)]
@@ -65,48 +66,59 @@ impl Root {
 }
 
 impl AutomationRoots {
-    /// The roots `read` and `write`, which must be existing folders; `None` when neither is given
-    /// (nothing is confined then).
-    pub fn new(read: Option<&Path>, write: Option<&Path>) -> Result<Option<Arc<Self>>, String> {
-        if read.is_none() && write.is_none() {
+    /// The roots `read` and `write`, which must be existing folders (one given twice counts once);
+    /// `None` when neither kind has any (nothing is confined then).
+    pub fn new(read: &[&Path], write: &[&Path]) -> Result<Option<Arc<Self>>, String> {
+        if read.is_empty() && write.is_empty() {
             return Ok(None);
         }
-        let read = read.map(|p| Root::open(p, Access::Read)).transpose()?;
-        let write = write.map(|p| Root::open(p, Access::Write)).transpose()?;
-        Ok(Some(Arc::new(Self { read, write })))
-    }
-
-    /// The read root, absolute with its links followed.
-    pub fn read_root(&self) -> Option<&Path> {
-        self.read.as_ref().map(|r| r.shown.as_path())
-    }
-
-    /// The write root, absolute with its links followed.
-    pub fn write_root(&self) -> Option<&Path> {
-        self.write.as_ref().map(|r| r.shown.as_path())
-    }
-
-    /// May the file or folder at `path` be read (or written)? `Err` says why not.
-    pub fn check(&self, path: &str, access: Access) -> Result<(), String> {
-        let root = match access {
-            Access::Read => self.read.as_ref(),
-            Access::Write => self.write.as_ref(),
+        let open = |paths: &[&Path], access: Access| -> Result<Vec<Root>, String> {
+            let mut roots: Vec<Root> = Vec::with_capacity(paths.len());
+            for path in paths {
+                let root = Root::open(path, access)?;
+                if !roots.iter().any(|r| r.resolved == root.resolved) {
+                    roots.push(root);
+                }
+            }
+            Ok(roots)
         };
-        let Some(Root { resolved: folder, shown }) = root else {
+        Ok(Some(Arc::new(Self { read: open(read, Access::Read)?, write: open(write, Access::Write)? })))
+    }
+
+    /// May the file or folder at `path` be read (or written)? It must lie inside one of the roots
+    /// of that kind. `Err` says why not.
+    pub fn check(&self, path: &str, access: Access) -> Result<(), String> {
+        let roots = match access {
+            Access::Read => &self.read,
+            Access::Write => &self.write,
+        };
+        // The messages are spelled out whole, as the translation catalogs match them.
+        if roots.is_empty() {
             return match access {
                 Access::Read => Err(format!("automation filesystem access is not granted: read authority is absent: {path}")),
                 Access::Write => Err(format!("automation filesystem access is not granted: write authority is absent: {path}")),
             };
-        };
-        if inside(&resolve(path)?, folder) {
+        }
+        let resolved = resolve(path)?;
+        if roots.iter().any(|r| inside(&resolved, &r.resolved)) {
             return Ok(());
         }
-        let root = shown.display();
-        match access {
-            Access::Read => Err(format!("automation path rejected: outside the read root {root}: {path}")),
-            Access::Write => Err(format!("automation path rejected: outside the write root {root}: {path}")),
+        let shown: Vec<String> = roots.iter().map(|r| r.shown.display().to_string()).collect();
+        match (shown.as_slice(), access) {
+            ([root], Access::Read) => Err(format!("automation path rejected: outside the read root {root}: {path}")),
+            ([root], Access::Write) => Err(format!("automation path rejected: outside the write root {root}: {path}")),
+            (_, Access::Read) => Err(format!("automation path rejected: outside the read roots {roots}: {path}", roots = shown.join(", "))),
+            (_, Access::Write) => Err(format!("automation path rejected: outside the write roots {roots}: {path}", roots = shown.join(", "))),
         }
     }
+
+    /// The roots for people: `read A, B; write C` (`none` for a kind without any).
+    pub fn describe(&self) -> String {
+        let list = |roots: &[Root]| {
+            if roots.is_empty() { "none".to_string() } else { roots.iter().map(|r| r.shown.display().to_string()).collect::<Vec<_>>().join(", ") }
+        };
+        format!("read {}; write {}", list(&self.read), list(&self.write))
+    }
 }
 
 /// Where the roots are in force on a thread ([`confine`], [`unconfined`]).
@@ -280,7 +292,7 @@ mod tests {
         }
         std::fs::write(base.join("in/a.svg"), "<svg/>").unwrap();
         std::fs::write(base.join("outside/secret.svg"), "<svg/>").unwrap();
-        let roots = AutomationRoots::new(Some(&base.join("in")), Some(&base.join("out"))).unwrap().unwrap();
+        let roots = AutomationRoots::new(&[&base.join("in")], &[&base.join("out")]).unwrap().unwrap();
         (base, roots)
     }
 
@@ -340,18 +352,20 @@ mod tests {
     #[test]
     fn a_missing_root_is_an_error_and_none_is_no_confinement() {
         let (base, _) = folders("missing");
-        let e = AutomationRoots::new(Some(&base.join("nope")), None).unwrap_err();
+        let e = AutomationRoots::new(&[&base.join("nope")], &[]).unwrap_err();
         assert!(e.starts_with("cannot open the automation read root"), "{e}");
-        let e = AutomationRoots::new(None, Some(&base.join("in/a.svg"))).unwrap_err();
+        let e = AutomationRoots::new(&[], &[&base.join("in/a.svg")]).unwrap_err();
         assert!(
             e.starts_with("cannot open the automation write root")
                 && e.ends_with(&std::io::Error::from(std::io::ErrorKind::NotADirectory).to_string()),
             "{e}"
         );
-        assert!(AutomationRoots::new(Some(Path::new("")), None).unwrap_err().starts_with("cannot open the automation read root ``"));
-        assert!(AutomationRoots::new(None, None).unwrap().is_none());
+        assert!(AutomationRoots::new(&[Path::new("")], &[]).unwrap_err().starts_with("cannot open the automation read root ``"));
+        assert!(AutomationRoots::new(&[], &[]).unwrap().is_none());
+        // A missing folder among several is an error too.
+        assert!(AutomationRoots::new(&[&base.join("in"), &base.join("nope")], &[]).unwrap_err().starts_with("cannot open the automation read root"));
         // One root alone grants only its own access.
-        let read_only = AutomationRoots::new(Some(&base.join("in")), None).unwrap().unwrap();
+        let read_only = AutomationRoots::new(&[&base.join("in")], &[]).unwrap().unwrap();
         let e = read_only.check(&s(base.join("in/x.svg")), Access::Write).unwrap_err();
         assert!(e.contains("write authority is absent"), "{e}");
         let _ = std::fs::remove_dir_all(base);
@@ -388,11 +402,55 @@ mod tests {
         assert_eq!(roots.check(&s(base.join("in/alias/x.svg")), Access::Read), Ok(()));
         // A root given through a link is the folder it leads to.
         assert!(made(&base.join("in"), &base.join("in-link")));
-        let linked = AutomationRoots::new(Some(&base.join("in-link")), None).unwrap().unwrap();
+        let linked = AutomationRoots::new(&[&base.join("in-link")], &[]).unwrap().unwrap();
         assert_eq!(linked.check(&s(base.join("in/a.svg")), Access::Read), Ok(()));
         let _ = std::fs::remove_dir_all(base);
     }
 
+    /// #3546: several roots of a kind: a path inside any of them passes, one inside none is
+    /// refused naming them all, and read and write stay separate.
+    #[test]
+    fn several_roots_of_a_kind_each_grant_their_access() {
+        let (base, _) = folders("several");
+        std::fs::create_dir_all(base.join("out2")).unwrap();
+        std::fs::write(base.join("in2/b.svg"), "<svg/>").unwrap();
+        let roots = AutomationRoots::new(&[&base.join("in"), &base.join("in2")], &[&base.join("out"), &base.join("out2")]).unwrap().unwrap();
+        assert_eq!(roots.check(&s(base.join("in/a.svg")), Access::Read), Ok(()));
+        assert_eq!(roots.check(&s(base.join("in2/b.svg")), Access::Read), Ok(()));
+        let e = roots.check(&s(base.join("outside/secret.svg")), Access::Read).unwrap_err();
+        assert!(e.starts_with("automation path rejected: outside the read roots ") && e.contains(", "), "{e}");
+        assert_eq!(roots.check(&s(base.join("out/x.png")), Access::Write), Ok(()));
+        assert_eq!(roots.check(&s(base.join("out2/new/x.png")), Access::Write), Ok(()));
+        assert!(roots.check(&s(base.join("outside/x.png")), Access::Write).unwrap_err().contains("outside the write roots"));
+        // Read and write access stay separate with several roots.
+        assert!(roots.check(&s(base.join("in2/b.svg")), Access::Write).is_err());
+        assert!(roots.check(&s(base.join("out2/x.png")), Access::Read).is_err());
+        // `..` out of one root into another root's sibling is still outside.
+        let sep = std::path::MAIN_SEPARATOR;
+        let up = format!("{}{sep}..{sep}outside{sep}secret.svg", s(base.join("in2")));
+        assert!(roots.check(&up, Access::Read).is_err());
+        // A folder given twice (here once through `..`) counts once.
+        let twice = AutomationRoots::new(&[&base.join("in"), &base.join("in/sub/..")], &[]).unwrap().unwrap();
+        assert!(!twice.describe().contains(", "), "{}", twice.describe());
+        assert!(twice.describe().ends_with("; write none"), "{}", twice.describe());
+        let _ = std::fs::remove_dir_all(base);
+    }
+
+    /// A link inside one root that leads into another root of the same kind stays inside; one that
+    /// leads out of all of them is outside.
+    #[cfg(unix)]
+    #[test]
+    fn links_between_several_roots() {
+        let (base, _) = folders("several-links");
+        std::fs::write(base.join("in2/b.svg"), "<svg/>").unwrap();
+        std::os::unix::fs::symlink(base.join("in2/b.svg"), base.join("in/to-in2.svg")).unwrap();
+        std::os::unix::fs::symlink(base.join("outside/secret.svg"), base.join("in2/leak.svg")).unwrap();
+        let roots = AutomationRoots::new(&[&base.join("in"), &base.join("in2")], &[]).unwrap().unwrap();
+        assert_eq!(roots.check(&s(base.join("in/to-in2.svg")), Access::Read), Ok(()));
+        assert!(roots.check(&s(base.join("in2/leak.svg")), Access::Read).unwrap_err().contains("outside the read roots"));
+        let _ = std::fs::remove_dir_all(base);
+    }
+
     #[cfg(windows)]
     #[test]
     fn windows_ignores_case_and_refuses_devices() {

crates/engine/src/tests.rs

@@ -3145,7 +3145,7 @@ fn file_list_lists_a_folder_within_the_read_root() {
         json!([{"name": "a.pdf", "kind": "file", "size": 4}, {"name": "b.svg", "kind": "file", "size": 6}, {"name": "sub", "kind": "folder"}])
     );
     assert_eq!(r["truncated"], false);
-    let roots = crate::file_access::AutomationRoots::new(Some(&base.join("in")), None).unwrap().unwrap();
+    let roots = crate::file_access::AutomationRoots::new(&[&base.join("in")], &[]).unwrap().unwrap();
     crate::file_access::confine(Some(&roots), || {
         assert!(s.execute("file.list", &json!({"path": path("in/sub")})).is_ok());
         assert!(s.execute("file.list", &json!({"path": path("outside")})).is_err(), "outside the read root");

crates/engine/src/tests_linkfiles.rs

@@ -306,7 +306,7 @@ fn a_copy_stays_inside_the_write_root() {
     let allowed = root.join("allowed");
     write(&allowed.join("photo.png"), b"kept as it is");
     std::os::unix::fs::symlink(allowed.join("photo.png"), root.join("doc/photo.png")).unwrap();
-    let roots = crate::file_access::AutomationRoots::new(Some(&root), Some(&allowed)).unwrap();
+    let roots = crate::file_access::AutomationRoots::new(&[&root], &[&allowed]).unwrap();
     let r = crate::file_access::confine(roots.as_ref(), || s.execute("links.relink", &json!({"ids": [id], "path": found, "copy": true})));
     assert!(matches!(&r, Err(EngineError::Other(m)) if m.contains("outside the write root")), "{r:?}");
     assert!(!root.join("doc/photo 2.png").exists());
@@ -332,7 +332,7 @@ fn an_svg_reads_its_links_within_the_read_root() {
     };
     let (inside, outside) = (import("in"), import("out"));
     assert_eq!(inside, outside, "both are read with no roots in force");
-    let roots = crate::file_access::AutomationRoots::new(Some(&root.join("in")), None).unwrap();
+    let roots = crate::file_access::AutomationRoots::new(&[&root.join("in")], &[]).unwrap();
     crate::file_access::confine(roots.as_ref(), || {
         assert_eq!(import("in"), inside, "inside the read root: read as before");
         assert_ne!(import("out"), outside, "outside the read root: not read");

crates/engine/src/tests_scripts.rs

@@ -113,7 +113,7 @@ fn links_out_of_the_folder_and_the_automation_read_root_keep_reads_inside() {
     assert_eq!(statuses(&r), ["notFollowed", "checked"]);
     assert_eq!(r["outside"], json!([]));
     // Under a read root, the script and its includes are read inside it, and a script outside isn't.
-    let roots = crate::file_access::AutomationRoots::new(Some(&dir.join("scripts")), None).unwrap().unwrap();
+    let roots = crate::file_access::AutomationRoots::new(&[&dir.join("scripts")], &[]).unwrap().unwrap();
     let mut s = Session::new();
     let r = crate::file_access::confine(Some(&roots), || s.execute("script.check", &json!({"path": main, "includes": true}))).unwrap();
     assert_eq!(statuses(&r), ["notFollowed", "checked"]);