Files
pdfcraft/apps/pdfcraft-cli/tests/run_root.rs
T
jcl-bitandClaude Opus 5.5 70da0179a6 Security: --root confinement leaks and escapes (#136, #137) (#164)
* Automation: --root refuses every outside path the same way (#136)

With a root set, `resolve` canonicalized a path before checking it against
the root, so an existing outside path got "is outside the allowed
directory" while a missing one got the OS error ("No such file or
directory", "Not a directory", "invalid path"): a confined MCP client or
script could probe which files and folders exist anywhere on the disk.

Containment is now decided before anything about an outside path is
reported:
- `..` is resolved by name first (as Windows does), so a path can't pass
  through an outside folder on its way back in;
- on Windows another network share or device namespace (`\\host\share`,
  `\\?\UNC\...`, `\\.\...`) is refused by name, without contacting it;
- the deepest existing ancestor, links followed, must be inside the root
  before anything is said about the rest;
- a broken link is refused the same way, so whether its target exists
  stays hidden too.

Every outside path now gets `<path> is outside the allowed directory
<root>`, existing or not, file or folder, read or write. Filesystem errors
such as "not found" are still reported for paths inside the root.

Behaviour change: on Unix, `..` now means the parent by name, as on
Windows (`link/../x` is `x` beside `link`), and a write such as
`new/../x.pdf` that stays in the root now succeeds instead of failing with
"'..' is not allowed here".

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* CLI: script images stay inside --root (#137)

`pdfcraft-cli run --script steps.json --root DIR` wrote each step's
`"out"` PNG with `std::fs::write`, never through the root check the tools
use: an absolute `"out"` created or replaced any file the process could
write, and a relative one landed in the working directory instead of the
root.

The image is now saved through the new `Automation::write_output`, which
uses the same resolver and atomic write as a tool's own output: outside
paths get the usual "is outside the allowed directory" refusal (the step
fails, nothing is written), relative paths resolve inside the root, and
without --root the path is written as given, in place as before (so
`/dev/stdout` still works). `run <tool> --out` goes the same way.

Found while testing this, the same "." problem in two tools:
- a write path naming a folder, such as "." (the root itself), staged its
  temporary file beside that folder, i.e. outside the root for ".",
  overwriting and then deleting any file there called
  `.root.pdfcraft-tmp`. `write_atomic` now refuses a folder before
  writing anything;
- `image_save` adds an extension to a path without one, so "." became
  `root.png` beside the root. It now refuses a folder first.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Automation: files named after a document stay in the output folder

Found while reviewing the --root fixes for #136 and #137. Folder outputs
name their files after the document and joined that name straight onto the
resolved folder, without checking the result against the root:

- `doc_export_images` and `doc_export_all_images` used the whole document
  name, so `doc_create` with `name: "../../escape"` followed by
  `doc_export_images` wrote `escape_page_1.png` outside the root (an
  absolute name went anywhere);
- `page_extract` (separate files), `doc_split`, `ocr_recognize_files` and
  `action_run` took the last component, but on Windows a name such as
  `x/C:evil.pdf` gives `C:evil`, and `folder.join("C:evil-page1.pdf")`
  replaces the folder with a path relative to the current directory on C:.

Every such name now goes through `child()`, which turns separators,
colons and control characters into `_` (and prefixes a name of only dots),
so the file is always a direct child of the folder. `with_root` also
refuses a root that is not a folder.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-07 21:12:32 -04:00

93 lines
4.5 KiB
Rust

//! `pdfcraft-cli run --root`: a script's image output stays inside the root (#137).
use std::path::{Path, PathBuf};
use std::process::{Command, Output};
const PNG: &[u8] = b"\x89PNG\r\n\x1a\n";
/// `root/` next to `outside/` (holding `existing.txt`) in a fresh temporary directory.
fn sandbox(test: &str) -> PathBuf {
let base = std::env::temp_dir().join(format!("pdfcraft-cli-{test}-{}", std::process::id()));
let _ = std::fs::remove_dir_all(&base);
std::fs::create_dir_all(base.join("root")).unwrap();
std::fs::create_dir_all(base.join("outside")).unwrap();
std::fs::write(base.join("outside/existing.txt"), "SENTINEL\n").unwrap();
base
}
/// Run a two-step script (a blank page, then render it to `out`) from the working directory
/// `cwd`, with `--root root` when `root` is given.
fn render_to(base: &Path, cwd: &Path, root: Option<&Path>, out: &str) -> Output {
let script = base.join("steps.json");
let steps = serde_json::json!([
{ "tool": "doc_create", "args": { "from": "blank", "width": 100, "height": 100 } },
{ "tool": "page_render", "args": { "doc": 1, "page": 1, "dpi": 10 }, "out": out },
]);
std::fs::write(&script, steps.to_string()).unwrap();
let mut cmd = Command::new(env!("CARGO_BIN_EXE_pdfcraft-cli"));
cmd.current_dir(cwd).arg("run").arg("--script").arg(&script);
if let Some(root) = root {
cmd.arg("--root").arg(root);
}
cmd.output().unwrap()
}
fn is_png(path: &Path) -> bool {
std::fs::read(path).is_ok_and(|b| b.starts_with(PNG))
}
#[test]
fn script_image_output_is_confined_to_the_root() {
let base = sandbox("run-root");
let root = base.join("root");
let abs = |rel: &str| base.join(rel).to_str().unwrap().to_owned();
#[cfg_attr(not(windows), allow(unused_mut))]
let mut refused = vec![abs("outside/escaped.png"), abs("outside/existing.txt"), "../outside/escaped.png".into(), abs("outside/new/dir/x.png")];
#[cfg(windows)]
{
refused.push(r"..\outside\escaped.png".into());
refused.push(r"\\pdfcraft-test.invalid\share\x.png".into());
if let Some(drive) = (b'D'..=b'Z').rev().map(|d| format!("{}:\\", d as char)).find(|d| !Path::new(d).exists()) {
refused.push(format!("{drive}x.png"));
}
}
for out in &refused {
let o = render_to(&base, &base, Some(&root), out);
let stderr = String::from_utf8_lossy(&o.stderr);
assert!(!o.status.success(), "{out}: expected a refusal");
assert!(stderr.contains("step 2 (page_render)") && stderr.contains("is outside the allowed directory"), "{out}: {stderr}");
}
assert!(!base.join("outside/escaped.png").exists() && !base.join("outside/new").exists());
assert_eq!(std::fs::read_to_string(base.join("outside/existing.txt")).unwrap(), "SENTINEL\n");
// "." is the root itself: refused, without staging a temporary file beside the root.
std::fs::write(base.join(".root.pdfcraft-tmp"), "SENTINEL\n").unwrap();
let o = render_to(&base, &base, Some(&root), ".");
assert!(!o.status.success() && String::from_utf8_lossy(&o.stderr).contains("is a folder"), "{}", String::from_utf8_lossy(&o.stderr));
assert_eq!(std::fs::read_to_string(base.join(".root.pdfcraft-tmp")).unwrap(), "SENTINEL\n");
// A relative path resolves inside the root, not in the working directory.
let o = render_to(&base, &base, Some(&root), "relative.png");
assert!(o.status.success(), "{}", String::from_utf8_lossy(&o.stderr));
assert!(is_png(&root.join("relative.png")) && !base.join("relative.png").exists());
// So does an absolute one inside the root, and a new folder in it.
for (out, file) in [(abs("root/inside.png"), "inside.png"), ("pages/one.png".into(), "pages/one.png")] {
let o = render_to(&base, &base, Some(&root), &out);
assert!(o.status.success(), "{out}: {}", String::from_utf8_lossy(&o.stderr));
assert!(is_png(&root.join(file)), "{out}");
}
// Without --root, the path is used as given.
let o = render_to(&base, &base, None, "free.png");
assert!(o.status.success(), "{}", String::from_utf8_lossy(&o.stderr));
assert!(is_png(&base.join("free.png")));
// Written in place, not through a temporary file beside it, so a device still works.
#[cfg(unix)]
{
let o = render_to(&base, &base, None, "/dev/stdout");
assert!(o.status.success() && o.stdout.windows(PNG.len()).any(|w| w == PNG), "{}", String::from_utf8_lossy(&o.stderr));
}
let _ = std::fs::remove_dir_all(&base);
}