From e3e6a461fecd482645ecea60a970574e75e5c82b Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 22:20:07 +0000 Subject: [PATCH] address review: check the original Path for the dictionary write; isolate the confinement test --- crates/engine/src/file_access.rs | 40 +++++++++++++++++++++----------- crates/engine/src/spell.rs | 7 ++++-- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/crates/engine/src/file_access.rs b/crates/engine/src/file_access.rs index 303e4c783..173ad4af0 100644 --- a/crates/engine/src/file_access.rs +++ b/crates/engine/src/file_access.rs @@ -85,14 +85,20 @@ impl AutomationRoots { /// 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> { + self.check_path(Path::new(path), access) + } + + /// [`Self::check`] for a path that may not be UTF-8: the checked path is the one opened. + pub fn check_path(&self, path: &Path, access: Access) -> Result<(), String> { + let path_text = path.display(); let roots = match access { Access::Read => &self.read, Access::Write => &self.write, }; 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}")), + Access::Read => Err(format!("automation filesystem access is not granted: read authority is absent: {path_text}")), + Access::Write => Err(format!("automation filesystem access is not granted: write authority is absent: {path_text}")), }; } let resolved = resolve(path)?; @@ -101,8 +107,8 @@ impl AutomationRoots { } let roots = roots.iter().map(|root| root.shown.display().to_string()).collect::>().join(", "); match access { - Access::Read => Err(format!("automation path rejected: outside the read roots {roots}: {path}")), - Access::Write => Err(format!("automation path rejected: outside the write roots {roots}: {path}")), + Access::Read => Err(format!("automation path rejected: outside the read roots {roots}: {path_text}")), + Access::Write => Err(format!("automation path rejected: outside the write roots {roots}: {path_text}")), } } } @@ -191,17 +197,23 @@ pub fn check_write(path: &str) -> Result<(), String> { current().map_or(Ok(()), |r| r.check(path, Access::Write)) } +/// May `path` be written here? Always, without roots in force. For a path that may not be UTF-8. +pub fn check_write_path(path: &Path) -> Result<(), String> { + current().map_or(Ok(()), |r| r.check_path(path, Access::Write)) +} + /// `path` made absolute with its links followed; for a path that doesn't exist (yet), its nearest /// existing folder's links followed and the rest appended. -fn resolve(path: &str) -> Result { - if path.is_empty() { +fn resolve(path: &Path) -> Result { + if path.as_os_str().is_empty() { return Err("automation path rejected: path is empty".into()); } + let shown = path.display(); // Checked as given: making `…\NUL` absolute gives the device path `\\.\NUL` on Windows. - if cfg!(windows) && Path::new(path).components().any(|c| matches!(c, Component::Normal(n) if is_device_name(&n.to_string_lossy()))) { - return Err(format!("automation path rejected: reserved device names are not allowed: {path}")); + if cfg!(windows) && path.components().any(|c| matches!(c, Component::Normal(n) if is_device_name(&n.to_string_lossy()))) { + return Err(format!("automation path rejected: reserved device names are not allowed: {shown}")); } - let absolute = std::path::absolute(path).map_err(|e| format!("automation path rejected: cannot resolve {path}: {e}"))?; + let absolute = std::path::absolute(path).map_err(|e| format!("automation path rejected: cannot resolve {shown}: {e}"))?; // The names below the nearest existing folder, last first. let mut missing: Vec = vec![]; let mut at = absolute.as_path(); @@ -212,11 +224,11 @@ fn resolve(path: &str) -> Result { return Ok(found); } Err(e) => match std::fs::symlink_metadata(at) { - Ok(m) if m.file_type().is_symlink() => return Err(format!("automation path rejected: a link that leads nowhere: {path}")), + Ok(m) if m.file_type().is_symlink() => return Err(format!("automation path rejected: a link that leads nowhere: {shown}")), // There, yet its real path can't be had (no permission): nothing to compare. - Ok(_) => return Err(format!("automation path rejected: cannot resolve {path}: {e}")), + Ok(_) => return Err(format!("automation path rejected: cannot resolve {shown}: {e}")), Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} - Err(e) => return Err(format!("automation path rejected: cannot resolve {path}: {e}")), + Err(e) => return Err(format!("automation path rejected: cannot resolve {shown}: {e}")), }, } let parent = at.parent(); @@ -226,12 +238,12 @@ fn resolve(path: &str) -> Result { at = parent; } (Some(Component::ParentDir), _) => { - return Err(format!("automation path rejected: parent traversal below a folder that doesn't exist: {path}")); + return Err(format!("automation path rejected: parent traversal below a folder that doesn't exist: {shown}")); } // Down to the root (or drive) and it doesn't exist either. _ => { let e = std::io::Error::from(std::io::ErrorKind::NotFound); - return Err(format!("automation path rejected: cannot resolve {path}: {e}")); + return Err(format!("automation path rejected: cannot resolve {shown}: {e}")); } } } diff --git a/crates/engine/src/spell.rs b/crates/engine/src/spell.rs index 2ea8f6fe0..cf423c5fc 100644 --- a/crates/engine/src/spell.rs +++ b/crates/engine/src/spell.rs @@ -669,7 +669,7 @@ pub fn add_user_word(word: &str) -> std::io::Result<()> { } let path = user_dictionary_path().ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "no folder for the user dictionary"))?; // A confined automation client may write only inside its write roots, this file included. - crate::file_access::check_write(&path.to_string_lossy()).map_err(|e| std::io::Error::new(std::io::ErrorKind::PermissionDenied, e))?; + crate::file_access::check_write_path(&path).map_err(|e| std::io::Error::new(std::io::ErrorKind::PermissionDenied, e))?; if let Some(d) = path.parent() { std::fs::create_dir_all(d)?; } @@ -960,9 +960,12 @@ SFX D Y 1\nSFX D 0 ed/S .\n"; let root = std::env::temp_dir().join(format!("deckcraft-dict-root-{}", std::process::id())); std::fs::create_dir_all(&root).unwrap(); let roots = crate::file_access::AutomationRoots::new(std::slice::from_ref(&root), std::slice::from_ref(&root)).unwrap(); - // The user dictionary lives in the home folder (or a test's own folder), never in `root`. + // The user dictionary is a file of its own outside `root`, not whatever HOME points at. + let outside = std::env::temp_dir().join(format!("deckcraft-dict-outside-{}", std::process::id())); + set_user_dictionary_path(outside.join("user.dic")); let r = crate::file_access::confine(roots.as_ref(), || add_user_word("zqxconfinedword")); assert_eq!(r.map_err(|e| e.kind()), Err(std::io::ErrorKind::PermissionDenied)); + assert!(!outside.exists(), "the refused write created nothing"); let _ = std::fs::remove_dir_all(&root); }