diff --git a/crates/automation/README.md b/crates/automation/README.md index 7eb8ca119..cdaaa13e6 100644 --- a/crates/automation/README.md +++ b/crates/automation/README.md @@ -33,7 +33,7 @@ its undo/redo history. - For proportional image signature resizing, use the embedded image layer's width/height ratio rather than a previously stretched annotation rectangle. The GUI preserves this ratio at corners with the opposite corner anchored; edge midpoint handles change only their own axis. `comment_edit` accepts the resulting rectangle. - `doc_close` refuses to drop unsaved changes unless `discard_changes: true`. `doc_save` writes atomically, incrementally in place, and in full for a new path. Atomic output writes call `sync_all` on the staging file before replacing the destination. A write or sync error leaves the destination intact, removes staging and leaves the document dirty for retry. A filesystem that does not support syncing returns an error; there is no unsynced fallback. This flushes file contents, but does not guarantee persistence of the parent-directory rename across power loss. - With a root set, every read and write path must resolve inside it (symlinks and `..` included). Relative paths resolve inside the root, and `..` is resolved by name before the check. Every path outside the root gets the same refusal (` is outside the allowed directory `), whether or not it exists, so a confined agent can't probe the rest of the disk; on Windows another network share or device path (`\\host\share`, `\\?\UNC\…`, `\\.\…`) is refused without being contacted. "Not found" and other filesystem errors are reported only for paths inside the root. Tools that write several files into a folder name them after the document or input file, with separators, colons and control characters replaced by `_`, so a name can't lead them out of the folder. The root itself must be a folder. -- `Automation::with_automation_roots` holds a read folder and a write folder (`pdfcraft-cli mcp --automation-read-root DIR --automation-write-root DIR`, see [docs/mcp.md](../../docs/mcp.md)). Reads must lie inside the read root and writes inside the write root, links followed. A root left out grants none of its access. Refusals read `automation path rejected: outside the read root : `. It applies on top of `with_root`. +- `Automation::with_automation_roots` holds a read folder and a write folder (`pdfcraft-cli mcp --automation-read-root DIR --automation-write-root DIR`, see [docs/mcp.md](../../docs/mcp.md)). Reads must lie inside the read root and writes inside the write root, links followed. A root left out grants none of its access. Refusals read `automation path rejected: outside the read root : `. As with a root set, on Windows a path on another network share or device path is refused with that refusal without being contacted (#4003). It applies on top of `with_root`. ## Text search and cache limits diff --git a/crates/automation/src/file_access.rs b/crates/automation/src/file_access.rs index 4820743e2..1556e3c90 100644 --- a/crates/automation/src/file_access.rs +++ b/crates/automation/src/file_access.rs @@ -9,6 +9,9 @@ //! 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). +//! - 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. //! - Read and write access are separate: a root given alone grants only its own kind of access. @@ -97,7 +100,10 @@ impl AutomationRoots { Access::Write => Err(format!("automation filesystem access is not granted: write authority is absent: {shown}")), }; }; - if inside(&resolve(shown, target)?, folder) { + let absolute = absolute(shown, target)?; + // Another network share or a device is refused by name, as `--root` refuses it: even + // resolving the path would contact the host (#4003). + if !foreign_share(&absolute, folder) && inside(&resolve(shown, &absolute)?, folder) { return Ok(()); } let root = root_shown.display(); @@ -123,9 +129,8 @@ fn plain(path: PathBuf) -> PathBuf { } } -/// `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(shown: &str, path: &Path) -> Result { +/// `path` made absolute against the working directory, by name: nothing on disk is read. +fn absolute(shown: &str, path: &Path) -> Result { if path.as_os_str().is_empty() { return Err("automation path rejected: path is empty".into()); } @@ -133,10 +138,15 @@ fn resolve(shown: &str, path: &Path) -> Result { 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 {shown}: {e}"))?; + std::path::absolute(path).map_err(|e| format!("automation path rejected: cannot resolve {shown}: {e}")) +} + +/// `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(shown: &str, absolute: &Path) -> Result { // The names below the nearest existing folder, last first. let mut missing: Vec = vec![]; - let mut at = absolute.as_path(); + let mut at = absolute; loop { match std::fs::canonicalize(at) { Ok(mut found) => { @@ -189,6 +199,24 @@ fn same_component(a: Component, b: Component) -> bool { } } +/// Whether `path` names another network share or device namespace than `root` (Windows +/// `\\host\share`, `\\?\UNC\…`, `\\.\…`, `\\?\…`). Those are refused by name: even checking +/// that one exists would contact the host. Drive letters are left to the normal check. +pub(crate) fn foreign_share(path: &Path, root: &Path) -> bool { + use std::path::Prefix; + fn share(p: &Path) -> Option { + let Some(Component::Prefix(prefix)) = p.components().next() else { return None }; + let (kind, a, b) = match prefix.kind() { + Prefix::Disk(_) | Prefix::VerbatimDisk(_) => return None, + Prefix::UNC(host, share) | Prefix::VerbatimUNC(host, share) => ("unc", host, share), + Prefix::DeviceNS(name) => ("device", name, std::ffi::OsStr::new("")), + Prefix::Verbatim(name) => ("verbatim", name, std::ffi::OsStr::new("")), + }; + Some(format!("{kind}\\{}\\{}", a.to_string_lossy(), b.to_string_lossy()).to_lowercase()) + } + share(path).is_some_and(|s| share(root).as_ref() != Some(&s)) +} + /// A Windows device name (`CON`, `NUL`, `COM1`… also with an extension), which names no file. fn is_device_name(component: &str) -> bool { let stem = component.split('.').next().unwrap_or(component).trim_end_matches(' ').to_ascii_uppercase(); @@ -233,6 +261,27 @@ mod tests { let _ = std::fs::remove_dir_all(base); } + /// #4003: another share or a device namespace is refused by name, with the same words as any + /// other path outside the root, before anything is resolved (which would contact the host). + #[cfg(windows)] + #[test] + fn other_shares_and_devices_are_refused_before_they_are_resolved() { + let (base, roots) = folders("shares"); + for p in [ + r"\\.\pipe\pdfcraft-4003.pdf", + r"\\?\GLOBALROOT\Device\Null", + r"\\pdfcraft-no-such-host.invalid\share\a.pdf", + "//pdfcraft-no-such-host.invalid/share/a.pdf", + r"\\?\UNC\pdfcraft-no-such-host.invalid\share\a.pdf", + ] { + let e = roots.check(p, Access::Read).unwrap_err(); + assert!(e.starts_with("automation path rejected: outside the read root"), "{p}: {e}"); + let e = roots.check(p, Access::Write).unwrap_err(); + assert!(e.starts_with("automation path rejected: outside the write root"), "{p}: {e}"); + } + let _ = std::fs::remove_dir_all(base); + } + #[test] fn a_sibling_with_the_same_prefix_is_outside() { let (base, roots) = folders("prefix"); diff --git a/crates/automation/src/lib.rs b/crates/automation/src/lib.rs index a81927505..a032186c5 100644 --- a/crates/automation/src/lib.rs +++ b/crates/automation/src/lib.rs @@ -106,6 +106,7 @@ const MAX_BOOKMARK_POSITION: f64 = 1_000_000.0; const FIND_FIRST_BATCH: usize = 8; const FIND_MAX_BATCH: usize = 128; +use file_access::foreign_share; use text_cache::TextCache; type PageTexts = Vec<(usize, std::result::Result, String>)>; @@ -2372,24 +2373,6 @@ fn lexical(path: &Path) -> Option { Some(parts.iter().collect()) } -/// Whether `path` names another network share or device namespace than `root` (Windows -/// `\\host\share`, `\\?\UNC\…`, `\\.\…`, `\\?\…`). Those are refused by name: even checking -/// that one exists would contact the host. Drive letters are left to the normal check. -fn foreign_share(path: &Path, root: &Path) -> bool { - use std::path::{Component, Prefix}; - fn share(p: &Path) -> Option { - let Some(Component::Prefix(prefix)) = p.components().next() else { return None }; - let (kind, a, b) = match prefix.kind() { - Prefix::Disk(_) | Prefix::VerbatimDisk(_) => return None, - Prefix::UNC(host, share) | Prefix::VerbatimUNC(host, share) => ("unc", host, share), - Prefix::DeviceNS(name) => ("device", name, std::ffi::OsStr::new("")), - Prefix::Verbatim(name) => ("verbatim", name, std::ffi::OsStr::new("")), - }; - Some(format!("{kind}\\{}\\{}", a.to_string_lossy(), b.to_string_lossy()).to_lowercase()) - } - share(path).is_some_and(|s| share(root).as_ref() != Some(&s)) -} - // ---- JSON helpers ---------------------------------------------------------------------------- fn parse_style_ranges(value: Option<&Value>) -> Result> { diff --git a/crates/automation/tests/it/automation_roots.rs b/crates/automation/tests/it/automation_roots.rs index 3e2d024cd..a96870ca0 100644 --- a/crates/automation/tests/it/automation_roots.rs +++ b/crates/automation/tests/it/automation_roots.rs @@ -352,6 +352,19 @@ fn links_out_of_a_root_are_refused() { assert_eq!(names(&base.join("outside")), ["pic.png", "secret.pdf"]); } +/// #4003: without `--root`, another network share or a device is refused by name through the +/// tools too, with the same words as any other path outside a root. +#[cfg(windows)] +#[test] +fn other_shares_and_devices_are_refused_by_name() { + let (mut server, base) = confined("shares", true, true); + ok(&mut server, "doc_open", json!({"path": s(&base.join("in/a.pdf"))})); + for p in [r"\\.\pipe\pdfcraft-4003.pdf", r"\\pdfcraft-no-such-host.invalid\share\a.pdf", "//pdfcraft-no-such-host.invalid/share/a.pdf"] { + refused(&mut server, "doc_open", json!({"path": p}), "outside the read root"); + refused(&mut server, "doc_save", json!({"doc": 1, "path": p}), "outside the write root"); + } +} + #[test] fn without_roots_nothing_is_confined() { let base = folders("none"); diff --git a/docs/mcp.md b/docs/mcp.md index 77a3c9484..8aa30ec6e 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -90,8 +90,10 @@ config works across the suite) keep the agent to the folders you give it: nearest existing folder's links are followed and the rest must be plain names, so `..` below a folder that doesn't exist is refused, as is a link that leads nowhere (writing through it would create its target). The result is compared with the root by whole folder names: `/work2/x` is not - inside `/work`. On Windows case is ignored, UNC paths are resolved like drive-letter ones, and - device names (`CON`, `NUL`, `COM1`...) are refused. + inside `/work`. On Windows case is ignored, device names (`CON`, `NUL`, `COM1`...) are refused, + and a path on another network share or in a device namespace than the root's (`\\host\share`, + `\\?\UNC\...`, `\\.\...`) is refused by name, before it is resolved, so the host is never + contacted. A root on a share accepts the paths on that share. - A refused path is an ordinary tool error the agent can read and correct, for example `automation path rejected: outside the write root /work/project/out: /etc/x.pdf` or `automation filesystem access is not granted: write authority is absent: /work/project/out/x.pdf`.