mirror of
https://github.com/storytold/pdfcraft.git
synced 2026-10-28 20:18:06 -06:00
fix(automation): roots-only mode refuses other shares and devices by name (#4289)
With only --automation-read-root / --automation-write-root set, a path went straight to std::fs::canonicalize, so a UNC, WebDAV or device path (`\host\share\x.pdf`, `\.\pipe\x`) was resolved, contacting the host, before it was refused. `--root` mode already refuses those by name (`foreign_share`). `foreign_share` moves into file_access, and `AutomationRoots::check_path` applies it to the path made absolute (by name, nothing read) before following links, with the same refusal as any other path outside the root. A root on a share still accepts that share. `resolve` is split into `absolute` (empty path, device names, made absolute) and `resolve` (links followed), so `out\NUL` keeps its own refusal. Part of #4003 (the first of its three points).
This commit is contained in:
@@ -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 (`<path> is outside the allowed directory <root>`), 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 <root>: <path>`. 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 <root>: <path>`. 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
|
||||
|
||||
|
||||
@@ -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<PathBuf, String> {
|
||||
/// `path` made absolute against the working directory, by name: nothing on disk is read.
|
||||
fn absolute(shown: &str, path: &Path) -> Result<PathBuf, String> {
|
||||
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<PathBuf, String> {
|
||||
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<PathBuf, String> {
|
||||
// The names below the nearest existing folder, last first.
|
||||
let mut missing: Vec<OsString> = 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<String> {
|
||||
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");
|
||||
|
||||
@@ -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<Arc<PageText>, String>)>;
|
||||
@@ -2372,24 +2373,6 @@ fn lexical(path: &Path) -> Option<PathBuf> {
|
||||
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<String> {
|
||||
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<Vec<pdfcraft_engine::RangeStyle>> {
|
||||
|
||||
@@ -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");
|
||||
|
||||
+4
-2
@@ -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`.
|
||||
|
||||
Reference in New Issue
Block a user