automation: compare canonical root components exactly and test roots-only escape confinement (#4003) (#4569)

crates/automation/src/file_access.rs

@@ -15,8 +15,8 @@
 //! - A path on another network share or in a device namespace (`\\host\share`, `\\.\pipe\x`)
 //!   than the root's is refused by name, before anything is resolved: resolving it would contact
 //!   the host.
-//! - 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 the root, compared by whole canonical components (`/work2` isn't
+//!   inside `/work`), with the drive letter compared without regard to case on Windows.
 //! - Read and write access are separate: a root given alone grants only its own kind of access.
 //!
 //! Without roots nothing is checked. A path is checked, then opened: a process that can change the
@@ -204,22 +204,23 @@ pub(crate) fn follow(shown: &str, path: &Path, root: &Path) -> Toward {
     if inside(&at, root) { Toward::Inside } else { Toward::Outside }
 }
 
-/// 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 (the drive letter without
+/// regard to case on Windows)?
 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)))
 }
 
 fn same_component(a: Component, b: Component) -> bool {
+    use std::path::Prefix;
     if a == b {
         return true;
     }
-    if !cfg!(windows) {
-        return false;
-    }
-    match (a.as_os_str().to_str(), b.as_os_str().to_str()) {
-        (Some(a), Some(b)) => a.to_lowercase() == b.to_lowercase(),
+    match (a, b) {
+        (Component::Prefix(a), Component::Prefix(b)) => match (a.kind(), b.kind()) {
+            (Prefix::Disk(a) | Prefix::VerbatimDisk(a), Prefix::Disk(b) | Prefix::VerbatimDisk(b)) => a.eq_ignore_ascii_case(&b),
+            _ => false,
+        },
         _ => false,
     }
 }
@@ -466,6 +467,37 @@ mod tests {
         let _ = std::fs::remove_dir_all(base);
     }
 
+    /// #4003: canonical path components are compared as the file system gave them, with only the
+    /// Windows drive letter matched ASCII case-insensitively, so a sibling whose name `to_lowercase`
+    /// folds onto the root's (`K` U+212A → `k`) or a differently-cased canonical component is not
+    /// taken as inside the root.
+    #[test]
+    fn canonical_components_are_compared_exactly() {
+        let (base, _) = folders("canonical-exact");
+        std::fs::create_dir_all(base.join("ink")).unwrap();
+        let kelvin = base.join("in\u{212A}");
+        std::fs::create_dir_all(&kelvin).unwrap();
+        std::fs::write(kelvin.join("secret.pdf"), "x").unwrap();
+        // U+212A folds to K. A case-insensitive volume stores both names as one directory, so
+        // `check` would be asked to reject a path inside its own root. The component comparison
+        // itself does not need that volume.
+        let distinct = base.join("ink").canonicalize().ok() != kelvin.canonicalize().ok();
+        if distinct {
+            let roots = AutomationRoots::new(Some(&base.join("ink")), None).unwrap().unwrap();
+            let e = roots.check(&s(kelvin.join("secret.pdf")), Access::Read).unwrap_err();
+            assert!(e.starts_with("automation path rejected: outside the read root"), "{e}");
+        }
+        assert!(!inside(Path::new("/work/in\u{212A}/secret.pdf"), Path::new("/work/ink")));
+        assert!(!inside(Path::new(r"\\?\C:\work\IN\a.pdf"), Path::new(r"\\?\C:\work\in")));
+        #[cfg(windows)]
+        {
+            assert!(inside(Path::new(r"\\?\c:\work\in\a.pdf"), Path::new(r"\\?\C:\work\in")));
+            assert!(inside(Path::new(r"c:\work\in\a.pdf"), Path::new(r"\\?\C:\work\in")));
+            assert!(!inside(Path::new(r"\\?\D:\work\in\a.pdf"), Path::new(r"\\?\C:\work\in")));
+        }
+        let _ = std::fs::remove_dir_all(base);
+    }
+
     /// Every file a tool reads or writes goes through `Automation::resolve`, which holds it to the
     /// roots. A function that reaches for the file system without resolving a path first would
     /// bypass them, so each use of `std::fs` or `File::open` must come after a `resolve` call in

crates/automation/tests/it/hostile_args.rs

@@ -11,9 +11,10 @@
 //!
 //! 1. **Nothing panics.** A panic in a tool is a crashed MCP server or CLI (AGENTS.md §4), so
 //!    every call must come back as `Ok` or as a `ToolError` the agent can read.
-//! 2. **Nothing escapes `--root`.** Every path argument is tried with `..` traversal, absolute
-//!    paths, Windows drive and UNC paths, and a planted symbolic link. The call must fail, and
-//!    afterwards nothing outside the root may have been created.
+//! 2. **Nothing escapes `--root` or the automation roots.** Every path argument is tried with `..`
+//!    traversal, absolute paths, Windows drive and UNC paths, and a planted symbolic link, under
+//!    both `--root` and `--automation-read-root`/`--automation-write-root` (#4003). The call must
+//!    fail, and afterwards nothing outside the root may have been created.
 //!
 //! The second is the mechanized form of the D3 audit. Reading the code says every handler resolves
 //! its own paths and that no tool dispatches another internally; this keeps that true as tools are
@@ -22,6 +23,7 @@
 use std::panic::{AssertUnwindSafe, catch_unwind};
 use std::path::{Path, PathBuf};
 
+use pdfcraft_automation::file_access::AutomationRoots;
 use pdfcraft_automation::{Automation, ToolError, tools};
 use serde_json::{Value, json};
 
@@ -84,6 +86,7 @@ fn zoo() -> Vec<Value> {
 /// bug can only touch what the test made. `linked`: the root holds `link-out`, a symbolic link
 /// to `base`.
 fn escapes(base: &std::path::Path, linked: bool) -> Vec<String> {
+    let root = base.join("root");
     let mut out: Vec<String> = vec![
         "../outside.pdf".into(),
         "../../outside.pdf".into(),
@@ -93,13 +96,16 @@ fn escapes(base: &std::path::Path, linked: bool) -> Vec<String> {
         base.join("outside.pdf").to_string_lossy().into_owned(),
         base.join("new-outside.pdf").to_string_lossy().into_owned(),
         base.join("sub").join("new-outside.pdf").to_string_lossy().into_owned(),
+        root.join("..").join("outside.pdf").to_string_lossy().into_owned(),
     ];
     if linked {
         out.push("link-out/outside.pdf".into());
+        out.push(root.join("link-out").join("outside.pdf").to_string_lossy().into_owned());
     }
     // Backslashes separate only on Windows; elsewhere this is an ordinary file name in the root.
     if cfg!(windows) {
         out.push(r"..\..\outside.pdf".into());
+        out.push(r"\\.\pipe\pdfcraft-hostile-escape.pdf".into());
     }
     out
 }
@@ -220,6 +226,20 @@ fn empty_dir(dir: &Path) {
     }
 }
 
+/// `action_run` keeps going when one input path is refused and reports that refusal on the file.
+/// That is still a confinement refusal: every file has an error naming the boundary, and none
+/// has an output.
+fn per_file_confinement_refusal(content: &[pdfcraft_automation::Content]) -> bool {
+    let Some(files) = content.iter().find_map(|c| match c {
+        pdfcraft_automation::Content::Json(v) => v.get("files").and_then(Value::as_array),
+        pdfcraft_automation::Content::Png { .. } => None,
+    }) else {
+        return false;
+    };
+    !files.is_empty()
+        && files.iter().all(|file| file.get("output").is_none() && file.get("error").and_then(Value::as_str).is_some_and(|e| e.contains("outside")))
+}
+
 /// Real values for the arguments a tool checks before the others, so a hostile value reaches the
 /// argument it is meant for instead of failing on a placeholder. `sign_document` checks its
 /// digital ID first and its document second: this ID opens with its password, and each call gets
@@ -234,6 +254,12 @@ fn fixture(a: &mut Automation, root: &Path, tool: &str) -> serde_json::Map<Strin
             let args = json!({ "doc": doc, "id": "id.p12", "password": "test", "out": "signed.pdf" });
             args.as_object().cloned().unwrap_or_default()
         }
+        // Without a step, action_run returns "give action or steps" before it resolves folder
+        // or paths, and that error would be counted as a confinement refusal.
+        "action_run" => {
+            let args = json!({ "steps": [{ "step": "set_title", "arg": "Confined" }] });
+            args.as_object().cloned().unwrap_or_default()
+        }
         _ => serde_json::Map::new(),
     }
 }
@@ -313,28 +339,34 @@ fn no_tool_escapes_the_root() {
         }
     };
 
-    let mut a = Automation::new().with_root(&root).expect("root");
-
-    // A real document, so tools reach their path arguments instead of failing on `doc` first.
-    let doc = blank_doc(&mut a);
+    let roots = AutomationRoots::new(Some(&root), Some(&root)).expect("roots");
+    let modes = [("--root", Automation::new().with_root(&root).expect("root")), ("automation roots", Automation::new().with_automation_roots(roots))];
 
     let mut leaked = Vec::new();
     let mut tried = 0usize;
     let mut refused = 0usize;
 
-    for def in tools() {
-        for (prop, is_list) in path_props(&def) {
-            for escape in escapes(&base, linked) {
-                let mut obj = plausible_with(&def, Some(doc));
-                obj.insert(prop.clone(), if is_list { json!([escape]) } else { json!(escape) });
-                let args = Value::Object(obj);
-                tried += 1;
-                match catch_unwind(AssertUnwindSafe(|| a.call(def.name, &args))) {
-                    // A path that leaves the root must be refused. Succeeding means the tool
-                    // either used it or ignored an argument it declares — both are faults.
-                    Ok(Ok(_)) => leaked.push(format!("{} accepted {prop}={escape:?}", def.name)),
-                    Ok(Err(_)) => refused += 1,
-                    Err(_) => leaked.push(format!("{} panicked on {prop}={escape:?}", def.name)),
+    for (mode, mut a) in modes {
+        // A real document, so tools reach their path arguments instead of failing on `doc` first.
+        let doc = blank_doc(&mut a);
+
+        for def in tools() {
+            for (prop, is_list) in path_props(&def) {
+                for escape in escapes(&base, linked) {
+                    let mut obj = plausible_with(&def, Some(doc));
+                    obj.insert(prop.clone(), if is_list { json!([escape]) } else { json!(escape) });
+                    let args = Value::Object(obj);
+                    tried += 1;
+                    match catch_unwind(AssertUnwindSafe(|| a.call(def.name, &args))) {
+                        // A path that leaves the root must be refused. Succeeding means the tool
+                        // either used it or ignored an argument it declares — both are faults.
+                        // action_run reports a refused input inside an otherwise successful
+                        // result, one error per file, and writes nothing for that file.
+                        Ok(Ok(content)) if per_file_confinement_refusal(&content) => refused += 1,
+                        Ok(Ok(_)) => leaked.push(format!("{mode}: {} accepted {prop}={escape:?}", def.name)),
+                        Ok(Err(_)) => refused += 1,
+                        Err(_) => leaked.push(format!("{mode}: {} panicked on {prop}={escape:?}", def.name)),
+                    }
                 }
             }
         }