Automation roots: compare paths exactly; recovery reads only regular entry files (#2205)

* fix(engine): exact root containment and regular-file recovery entries

* Apply review fixes

- crates/engine/src/recovery.rs

---------

Co-authored-by: aiongg <67543387+aiongg@users.noreply.github.com>

crates/engine/src/file_access.rs

@@ -10,8 +10,9 @@
 //!   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 one of the roots of its kind, compared by whole components
-//!   (`/work2` isn't inside `/work`), without regard to case on Windows. Two roots don't make their
-//!   common parent one.
+//!   (`/work2` isn't inside `/work`), in the case the file system stores (so a case-variant name
+//!   reaches the same folder on an ordinary Windows volume, and a distinct sibling in a
+//!   case-sensitive one stays outside). Two roots don't make their common parent one.
 //! - 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
@@ -255,24 +256,13 @@ fn resolve(path: &Path) -> 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? Both are canonical, so each
+/// component carries the case the file system stores: a path that differs only in case names the
+/// same folder on an ordinary Windows volume (and resolves to the same components), and a distinct
+/// sibling in a case-sensitive folder (which must not pass as the root).
 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 {
-    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(),
-        _ => false,
-    }
+    root.components().all(|r| path.next() == Some(r))
 }
 
 /// A Windows device name (`CON`, `NUL`, `COM1`… also with an extension), which names no file.
@@ -571,6 +561,14 @@ mod tests {
         let _ = std::fs::remove_dir_all(base);
     }
 
+    #[test]
+    fn inside_compares_components_exactly() {
+        assert!(inside(Path::new("/work/in/a"), Path::new("/work/in")));
+        assert!(inside(Path::new("/work/in"), Path::new("/work/in")));
+        assert!(!inside(Path::new("/work/In/a"), Path::new("/work/in")), "a case-variant sibling is another folder");
+        assert!(!inside(Path::new("/work/in2/a"), Path::new("/work/in")));
+    }
+
     #[test]
     fn device_names() {
         for d in ["CON", "nul", "Com1", "LPT9.txt", "aux.designcraft", "NUL "] {

crates/engine/src/recovery.rs

@@ -441,6 +441,49 @@ fn files(dir: &Path, uid: u64) -> (PathBuf, PathBuf) {
     (dir.join(format!("{uid}.designcraft")), dir.join(format!("{uid}.json")))
 }
 
+/// The bytes of the recovery entry file at `path`, which must be a regular file: the folder was
+/// checked, and a link planted among its entries would lead the read anywhere.
+fn read_entry_file(path: &Path) -> std::io::Result<Vec<u8>> {
+    use std::io::Read;
+    let refuse = || std::io::Error::other(format!("{}: a recovery entry must be a regular file", path.display()));
+    let before = std::fs::symlink_metadata(path)?;
+    #[cfg(not(target_arch = "wasm32"))]
+    if is_link_like(&before) {
+        return Err(refuse());
+    }
+    if !before.file_type().is_file() {
+        return Err(refuse());
+    }
+    #[cfg(windows)]
+    let mut file = {
+        use std::os::windows::fs::OpenOptionsExt;
+        // A reparse point swapped in after the check is opened as itself, not followed.
+        const FILE_FLAG_OPEN_REPARSE_POINT: u32 = 0x0020_0000;
+        std::fs::OpenOptions::new().read(true).custom_flags(FILE_FLAG_OPEN_REPARSE_POINT).open(path)?
+    };
+    #[cfg(not(windows))]
+    let mut file = std::fs::File::open(path)?;
+    let opened = file.metadata()?;
+    #[cfg(windows)]
+    if is_link_like(&opened) {
+        return Err(refuse());
+    }
+    if !opened.file_type().is_file() {
+        return Err(refuse());
+    }
+    // The name was checked, then opened: make sure what was opened is what was checked.
+    #[cfg(unix)]
+    {
+        use std::os::unix::fs::MetadataExt;
+        if opened.dev() != before.dev() || opened.ino() != before.ino() {
+            return Err(refuse());
+        }
+    }
+    let mut bytes = Vec::new();
+    file.read_to_end(&mut bytes)?;
+    Ok(bytes)
+}
+
 /// Remove a document's recovery entry (it was saved or closed). In a folder that is not this
 /// run's own, only an entry this run wrote or recovered is removed.
 pub fn discard(dir: &Path, uid: u64) {
@@ -761,9 +804,9 @@ pub fn list(dir: &Path) -> Vec<(u64, Value)> {
         }
         let Some(uid) = p.file_stem().and_then(std::ffi::OsStr::to_str).and_then(|s| s.parse::<u64>().ok()) else { continue };
         // Metadata is a file on disk: anything but an object (corrupt, hand-edited) counts as none.
-        let meta = std::fs::read_to_string(p.with_extension("json"))
+        let meta = read_entry_file(&p.with_extension("json"))
             .ok()
-            .and_then(|t| serde_json::from_str::<Value>(&t).ok())
+            .and_then(|t| serde_json::from_slice::<Value>(&t).ok())
             .filter(Value::is_object)
             .unwrap_or(json!({}));
         out.push((uid, meta));
@@ -810,7 +853,7 @@ fn open_with_publisher(s: &mut Session, dir: &Path, mut publish: impl FnMut(&Pat
         #[cfg(not(target_arch = "wasm32"))]
         validate_recovery_dir(dir)?;
         let (doc, _) = files(dir, uid);
-        let Ok(bytes) = std::fs::read(&doc) else { continue };
+        let Ok(bytes) = read_entry_file(&doc) else { continue };
         let mut d = match crate::cmd::from_bytes(&bytes) {
             Ok(d) => d,
             Err(_) => continue,

crates/engine/src/recovery/tests.rs

@@ -183,6 +183,37 @@ fn symlink_recovery_directory_is_rejected() {
     assert!(std::fs::symlink_metadata(&link).unwrap().file_type().is_symlink());
 }
 
+/// A link planted among a checked folder's entries doesn't get its target read: not as metadata
+/// (listed as is) and not as a document (recovered).
+#[cfg(unix)]
+#[test]
+fn linked_recovery_entries_are_not_read() {
+    use std::os::unix::fs::symlink;
+
+    let root = tempfile::tempdir().unwrap();
+    let (dir, elsewhere) = (root.path().join("recovery"), root.path().join("elsewhere"));
+    std::fs::create_dir(&dir).unwrap();
+    std::fs::create_dir(&elsewhere).unwrap();
+    // A real document whose metadata is a link to a private file.
+    assert_eq!(save(&unsaved("real"), &dir).unwrap(), 1);
+    let uid = list(&dir)[0].0;
+    std::fs::write(elsewhere.join("secret.json"), br#"{"path": "/home/someone/secret.designcraft"}"#).unwrap();
+    std::fs::remove_file(dir.join(format!("{uid}.json"))).unwrap();
+    symlink(elsewhere.join("secret.json"), dir.join(format!("{uid}.json"))).unwrap();
+    // A document entry that is a link to a document elsewhere.
+    assert_eq!(save(&unsaved("elsewhere"), &elsewhere).unwrap(), 1);
+    let linked = list(&elsewhere)[0].0;
+    symlink(elsewhere.join(format!("{linked}.designcraft")), dir.join("9000000.designcraft")).unwrap();
+
+    let entries = list(&dir);
+    assert_eq!(entries.iter().map(|e| e.0).collect::<Vec<_>>(), [uid, 9_000_000]);
+    assert_eq!(entries[0].1, json!({}), "the linked metadata counts as none");
+    let mut s = Session::new();
+    assert_eq!(open(&mut s, &dir).unwrap().len(), 1);
+    assert_eq!(texts(&s), ["real"]);
+    assert!(std::fs::symlink_metadata(dir.join("9000000.designcraft")).unwrap().file_type().is_symlink(), "left as it was");
+}
+
 #[test]
 fn a_run_has_a_locked_folder_of_its_own() {
     let root = tempfile::tempdir().unwrap();