From 087fde79af44579e1dcbbfe1552cf2f96dac7c7c Mon Sep 17 00:00:00 2001 From: Daniel Kim <79390398+sox8502@users.noreply.github.com> Date: Sun, 19 Jul 2026 17:41:14 -0700 Subject: [PATCH] Contain file serving to the files dir even through symlinks (#97) * Contain file serving to the files dir even through symlinks is_safe_rel_path only blocks lexical escapes (..); a symlink inside the files dir pointing outside it was followed by read_file, read_report_markdown, and the report-title scan, so anything reachable on disk could be served to whatever can reach the local API. Resolve every served path with canonicalize() and require it to stay under the canonicalized files dir. Internal symlinks keep working and now also appear in listings (DirEntry::metadata never follows links, so they were silently dropped before); escaping ones are skipped from the listing and fingerprint, and refused by the serve endpoints. Delete resolves parent segments the same way but removes a final-component symlink literally, never its target. * Review nits: shared followed_metadata helper, drop redundant pre-check Extract the symlink-aware metadata lookup shared by collect_tree and hash_dir, let resolve_contained handle read_report_markdown's path validation, and document the containment guarantee (and the accepted check-vs-use gap) at module level. --- src/local/files.rs | 211 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 192 insertions(+), 19 deletions(-) diff --git a/src/local/files.rs b/src/local/files.rs index f8cdad6a..09956053 100644 --- a/src/local/files.rs +++ b/src/local/files.rs @@ -11,6 +11,11 @@ //! kept out of the experiment-slug space by `experiments::unique_slug`), //! including `project/memory.md` — the agent's persisted project memory, //! inlined into the playbook (see `memory.rs`). +//! +//! Serving is contained to the files dir: requested paths are relative +//! (`is_safe_rel_path`) and must still resolve inside it once symlinks are +//! followed (`resolve_contained`), so nothing outside can be listed, read, +//! or deleted through the API. use std::collections::hash_map::DefaultHasher; use std::collections::HashMap; @@ -62,7 +67,8 @@ pub fn ensure_dir(project: &LocalProject) -> Result { } /// Relative, no `..`/`.` segments, no backslashes — a requested path can't -/// escape the files dir. +/// escape the files dir. Lexical only; symlink containment is enforced by +/// `resolve_contained`. pub fn is_safe_rel_path(p: &str) -> bool { !p.is_empty() && !p.starts_with('/') @@ -72,6 +78,58 @@ pub fn is_safe_rel_path(p: &str) -> bool { .any(|seg| seg == ".." || seg == "." || seg.is_empty()) } +/// True when `path` resolves (following symlinks) to a location inside +/// `canonical_base`. Anything that fails to resolve is treated as outside. +fn resolves_inside(canonical_base: &Path, path: &Path) -> bool { + path.canonicalize() + .map(|c| c.starts_with(canonical_base)) + .unwrap_or(false) +} + +/// Metadata for a listed entry: an escaping symlink yields `None` (skip), a +/// contained symlink its *followed* target metadata (`DirEntry::metadata` +/// never follows links). Shared by the tree walk and the fingerprint so both +/// see exactly the same entries. +fn followed_metadata( + canonical_base: &Path, + entry: &std::fs::DirEntry, +) -> Option { + let ft = entry.file_type().ok()?; + if ft.is_symlink() { + if !resolves_inside(canonical_base, &entry.path()) { + return None; + } + std::fs::metadata(entry.path()).ok() + } else { + entry.metadata().ok() + } +} + +/// Join `rel_path` onto `base` and resolve it, requiring the result to stay +/// inside `base` once every symlink is followed. `is_safe_rel_path` already +/// blocks lexical escapes (`..`); this closes the remaining hole — a symlink +/// inside the dir pointing outside it. Internal symlinks still work. +/// +/// Check and use are separate syscalls, so a link swapped in between could +/// still escape — accepted: the API is localhost-only and the files dir is +/// written by the same user it would expose. +fn resolve_contained(base: &Path, rel_path: &str) -> Result { + if !is_safe_rel_path(rel_path) { + return Err(anyhow!("invalid file path: {rel_path}")); + } + let canonical_base = base + .canonicalize() + .map_err(|e| anyhow!("Could not resolve {}: {}", base.display(), e))?; + let path = canonical_base.join(rel_path); + let canonical = path + .canonicalize() + .map_err(|e| anyhow!("Could not read {}: {}", path.display(), e))?; + if !canonical.starts_with(&canonical_base) { + return Err(anyhow!("path escapes the files dir: {rel_path}")); + } + Ok(canonical) +} + /// Best-effort content type from a file extension (serving files). pub fn content_type_for_path(path: &str) -> &'static str { match path @@ -176,8 +234,14 @@ fn report_title(md_path: &Path, fallback: &str) -> String { } /// Recursively build the tree under `dir`, counting nodes against -/// `MAX_ENTRIES`. Returns (children, hit_cap). -fn collect_tree(dir: &Path, rel_prefix: &str, seen: &mut usize) -> (Vec, bool) { +/// `MAX_ENTRIES`. Returns (children, hit_cap). Symlinks resolving outside +/// `canonical_base` are skipped — the serve endpoints would refuse them. +fn collect_tree( + canonical_base: &Path, + dir: &Path, + rel_prefix: &str, + seen: &mut usize, +) -> (Vec, bool) { let mut out = Vec::new(); let mut truncated = false; let Ok(entries) = std::fs::read_dir(dir) else { @@ -191,7 +255,9 @@ fn collect_tree(dir: &Path, rel_prefix: &str, seen: &mut usize) -> (Vec (Vec, ) -> Result { let dir = ensure_dir(project)?; + let canonical = dir + .canonicalize() + .map_err(|e| anyhow!("Could not resolve {}: {}", dir.display(), e))?; let mut seen = 0; - let (mut entries, truncated) = collect_tree(&dir, "", &mut seen); + let (mut entries, truncated) = collect_tree(&canonical, &canonical, "", &mut seen); let by_slug: HashMap<&str, &LocalExperiment> = experiments.iter().map(|e| (e.slug.as_str(), e)).collect(); for entry in entries.iter_mut().filter(|e| e.is_dir) { @@ -265,28 +335,34 @@ pub fn list( /// A report folder's `report.md` body, by dir-relative folder path. pub fn read_report_markdown(project: &LocalProject, folder: &str) -> Result { - if !is_safe_rel_path(folder) { - return Err(anyhow!("invalid report path: {folder}")); - } - let path = files_dir(project).join(folder).join("report.md"); + let path = resolve_contained(&files_dir(project), &format!("{folder}/report.md"))?; std::fs::read_to_string(&path).map_err(|e| anyhow!("Could not read {}: {}", path.display(), e)) } /// One file in the files dir, by dir-relative path. pub fn read_file(project: &LocalProject, rel_path: &str) -> Result> { - if !is_safe_rel_path(rel_path) { - return Err(anyhow!("invalid file path: {rel_path}")); - } - let path = files_dir(project).join(rel_path); + let path = resolve_contained(&files_dir(project), rel_path)?; std::fs::read(&path).map_err(|e| anyhow!("Could not read {}: {}", path.display(), e)) } /// Delete a file or folder (report) in the files dir. +/// +/// The final component is deleted literally — a symlink is removed, never +/// followed — but every parent segment must resolve inside the files dir, or +/// `a/b` with `a -> /elsewhere` would delete outside it. pub fn delete_entry(project: &LocalProject, rel_path: &str) -> Result<()> { if !is_safe_rel_path(rel_path) { return Err(anyhow!("invalid file path: {rel_path}")); } - let path = files_dir(project).join(rel_path); + let base = files_dir(project); + let parent = match rel_path.rsplit_once('/') { + Some((parent_rel, _)) => resolve_contained(&base, parent_rel)?, + None => base + .canonicalize() + .map_err(|e| anyhow!("Could not resolve {}: {}", base.display(), e))?, + }; + let name = rel_path.rsplit('/').next().unwrap_or(rel_path); + let path = parent.join(name); let md = std::fs::symlink_metadata(&path) .map_err(|e| anyhow!("Could not stat {}: {}", path.display(), e))?; if md.is_dir() { @@ -300,12 +376,15 @@ pub fn delete_entry(project: &LocalProject, rel_path: &str) -> Result<()> { /// Cheap change fingerprint (paths + sizes + mtimes) for the SSE diff loop. /// A missing dir hashes to a stable value, so first creation is a change. pub fn fingerprint(project: &LocalProject) -> u64 { - let dir = files_dir(project); let mut hasher = DefaultHasher::new(); - hash_dir(&dir, &dir, &mut hasher, &mut 0); + if let Ok(canonical) = files_dir(project).canonicalize() { + hash_dir(&canonical, &canonical, &mut hasher, &mut 0); + } hasher.finish() } +/// Hash the tree under `dir`, skipping (like `collect_tree`) symlinks that +/// resolve outside `base`, so the fingerprint tracks exactly what's listed. fn hash_dir(base: &Path, dir: &Path, hasher: &mut DefaultHasher, seen: &mut usize) { let Ok(entries) = std::fs::read_dir(dir) else { return; @@ -318,7 +397,9 @@ fn hash_dir(base: &Path, dir: &Path, hasher: &mut DefaultHasher, seen: &mut usiz if is_ignored(&name) { continue; } - let Ok(md) = entry.metadata() else { continue }; + let Some(md) = followed_metadata(base, &entry) else { + continue; + }; *seen += 1; if let Ok(rel) = entry.path().strip_prefix(base) { rel.to_string_lossy().hash(hasher); @@ -331,3 +412,95 @@ fn hash_dir(base: &Path, dir: &Path, hasher: &mut DefaultHasher, seen: &mut usiz } } } + +#[cfg(all(test, unix))] +mod tests { + use super::*; + use std::os::unix::fs::symlink; + + /// Fresh scratch dir with a `base/` (the files dir under test) and an + /// `outside/` holding a file symlinks will try to escape to. + fn scratch() -> (PathBuf, PathBuf, PathBuf) { + let root = std::env::temp_dir().join(format!("orx-files-{}", uuid::Uuid::new_v4())); + let base = root.join("base"); + let outside = root.join("outside"); + std::fs::create_dir_all(&base).unwrap(); + std::fs::create_dir_all(&outside).unwrap(); + std::fs::write(outside.join("secret.txt"), "secret").unwrap(); + (root, base, outside) + } + + #[test] + fn rejects_lexical_escapes() { + let (root, base, _) = scratch(); + for bad in ["../x", "/etc/passwd", "a/../b", "a/./b", "", "a\\b"] { + assert!(resolve_contained(&base, bad).is_err(), "accepted {bad:?}"); + } + std::fs::remove_dir_all(root).unwrap(); + } + + #[test] + fn blocks_symlink_file_escape() { + let (root, base, outside) = scratch(); + symlink(outside.join("secret.txt"), base.join("link.txt")).unwrap(); + assert!(resolve_contained(&base, "link.txt").is_err()); + std::fs::remove_dir_all(root).unwrap(); + } + + #[test] + fn blocks_symlink_dir_escape() { + let (root, base, outside) = scratch(); + symlink(&outside, base.join("sub")).unwrap(); + assert!(resolve_contained(&base, "sub/secret.txt").is_err()); + std::fs::remove_dir_all(root).unwrap(); + } + + #[test] + fn allows_internal_symlink() { + let (root, base, _) = scratch(); + std::fs::write(base.join("real.txt"), "data").unwrap(); + symlink("real.txt", base.join("alias.txt")).unwrap(); + let resolved = resolve_contained(&base, "alias.txt").unwrap(); + assert_eq!(std::fs::read_to_string(resolved).unwrap(), "data"); + std::fs::remove_dir_all(root).unwrap(); + } + + #[test] + fn allows_regular_files() { + let (root, base, _) = scratch(); + std::fs::create_dir(base.join("exp")).unwrap(); + std::fs::write(base.join("exp/report.md"), "# T").unwrap(); + assert!(resolve_contained(&base, "exp/report.md").is_ok()); + std::fs::remove_dir_all(root).unwrap(); + } + + #[test] + fn listing_skips_escaping_symlinks_keeps_internal() { + let (root, base, outside) = scratch(); + std::fs::write(base.join("real.txt"), "data").unwrap(); + symlink("real.txt", base.join("alias.txt")).unwrap(); + symlink(outside.join("secret.txt"), base.join("leak.txt")).unwrap(); + symlink(&outside, base.join("leakdir")).unwrap(); + let canonical = base.canonicalize().unwrap(); + let (entries, truncated) = collect_tree(&canonical, &canonical, "", &mut 0); + let names: Vec<&str> = entries.iter().map(|e| e.name.as_str()).collect(); + assert_eq!(names, ["alias.txt", "real.txt"]); + assert!(!truncated); + std::fs::remove_dir_all(root).unwrap(); + } + + #[test] + fn listing_hides_report_title_behind_escaping_symlink() { + let (root, base, outside) = scratch(); + std::fs::write(outside.join("report.md"), "# Leaked heading").unwrap(); + std::fs::create_dir(base.join("exp")).unwrap(); + symlink(outside.join("report.md"), base.join("exp/report.md")).unwrap(); + let canonical = base.canonicalize().unwrap(); + let (entries, _) = collect_tree(&canonical, &canonical, "", &mut 0); + assert_eq!(entries.len(), 1); + assert_eq!(entries[0].report_title, None); + // The dangling entry itself is skipped too, not just the title. + assert!(entries[0].children.is_empty()); + std::fs::remove_dir_all(root).unwrap(); + } +}