From 7c83fa43ac9d6dac5e4c2eabbf5974ab423fcb46 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 14:32:18 -0500 Subject: [PATCH] Spelling: check the automation write roots before appending to the user dictionary (#1754) * engine: check the automation write roots before appending to the user dictionary * 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 | 16 +++++++++++++ 2 files changed, 42 insertions(+), 14 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 3323151cb..8bc88e646 100644 --- a/crates/engine/src/spell.rs +++ b/crates/engine/src/spell.rs @@ -699,6 +699,8 @@ pub fn add_user_word(word: &str) -> std::io::Result<()> { return Ok(()); } 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(&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)?; } @@ -1020,6 +1022,20 @@ SFX D Y 1\nSFX D 0 ed/S .\n"; assert_eq!(w, vec!["check", "done"]); } + #[test] + fn a_confined_client_cannot_write_the_user_dictionary_outside_its_roots() { + 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 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); + } + #[test] fn a_long_paragraph_with_a_huge_language_is_checked_in_linear_work() { use deckcraft_model::{Paragraph, Run, RunProps};