From dcec00849122cf73c792fdfd73dc53ceb2ec5718 Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Wed, 4 Nov 2026 00:32:23 +0000 Subject: [PATCH 1/2] feat(project): a filesystem confined to the project root, for a project from elsewhere `icp project bundle` holds every path a manifest names to the project it is bundling, and refuses one that resolves outside. Loading a project holds nothing: the loader joins each path to its base and reads it through the host filesystem, which is right for the user's own tree and wrong for a project someone else handed over, a bundle an agent passes to a deploy service, say. A manifest in such a bundle could name `../../etc/hostname` as an argument file and have it read into the canister's arguments at load, before any caller could check. `files::Confined` is the host filesystem held to one directory. Every path is resolved against the root before the disk sees it: a relative path is joined, `.` dropped, `..` applied, and a path that comes to rest outside the root, by climbing, by being absolute elsewhere, or through a symlink inside the root that points out, is refused with `OutsideRoot`. The predicates answer false for such a path, scratch directories are made under the root, and `resolve` hands out the real path for a caller that needs one for something that does not read through the trait. The loader resolves every path a manifest names through the filesystem it is given, so confining that one seam holds all of them, canister directories and dependency projects included, without the loader enumerating its fields; the tests load one manifest through both filesystems to show the difference. Nothing changes for the CLI, which keeps loading through `HostFileSystem`. Co-Authored-By: Claude Fable 5.1 --- crates/icp-project/src/files.rs | 323 ++++++++++++++++++++++++++++++ crates/icp-project/src/project.rs | 107 ++++++++++ 2 files changed, 430 insertions(+) diff --git a/crates/icp-project/src/files.rs b/crates/icp-project/src/files.rs index 860cee6a9..e58ddf160 100644 --- a/crates/icp-project/src/files.rs +++ b/crates/icp-project/src/files.rs @@ -88,6 +88,199 @@ pub trait Scratch: Send + Sync { fn path(&self) -> &Path; } +/// This machine's filesystem, held to one directory. +/// +/// Every path is resolved against the root before the disk sees it: a +/// relative path is joined to the root, `.` is dropped, `..` is applied, and a +/// path that comes to rest outside the root — by climbing above it, by being +/// absolute somewhere else, or through a symlink inside the root that points +/// out — is refused with [`OutsideRoot`]. The predicates answer `false` for +/// such a path, as they do for any other failure, and scratch directories are +/// made under the root, so what a build writes and reads back stays inside. +/// +/// For a project whose files are not the user's own tree: a bundle handed +/// over by someone else, which `icp project bundle` wrote with every path +/// held to the project it came from, but which the loader would otherwise +/// take at its word. The loader resolves every path a manifest names +/// through the [`FileSystem`] it is given, so confining that one seam holds +/// all of them — the canister directories, the manifests in them, the +/// argument and variable files, the dependency projects — without the +/// loader enumerating its fields. +#[cfg(feature = "host")] +#[derive(Debug, Clone)] +pub struct Confined { + /// Canonical, so a canonicalized path compares against it. + root: PathBuf, +} + +/// A path a [`Confined`] filesystem was asked for that is not under its root. +#[cfg(feature = "host")] +#[derive(Debug, Snafu)] +#[snafu(display("'{path}' is outside '{root}', the directory this project is confined to"))] +pub struct OutsideRoot { + pub path: PathBuf, + pub root: PathBuf, +} + +/// Why a directory cannot be confined to. +#[cfg(feature = "host")] +#[derive(Debug, Snafu)] +pub enum ConfineError { + #[snafu(display("cannot resolve '{path}' to confine the project to it"))] + Resolve { + path: PathBuf, + source: std::io::Error, + }, + #[snafu(display("'{path}' resolves to a path that is not UTF-8"))] + NotUtf8 { path: PathBuf }, +} + +#[cfg(feature = "host")] +impl Confined { + /// Confine the host filesystem to `root`, which must exist. + pub fn new(root: &Path) -> Result { + let real = dunce::canonicalize(root).context(ResolveSnafu { path: root })?; + let root = PathBuf::from_path_buf(real).map_err(|_| ConfineError::NotUtf8 { + path: root.to_path_buf(), + })?; + Ok(Self { root }) + } + + /// The directory everything is held to. + pub fn root(&self) -> &Path { + &self.root + } + + /// Where `path` lies under the root, by its spelling alone: joined to the + /// root if relative, with `.` dropped and `..` applied, and refused if it + /// would come to rest outside. What a caller that needs a real path for + /// something that does not read through the trait — a runtime mounting a + /// directory — asks for; the trait's own reads also follow symlinks + /// before trusting the answer. + pub fn resolve(&self, path: &Path) -> Result { + let outside = || { + FsError::new(OutsideRoot { + path: path.to_path_buf(), + root: self.root.clone(), + }) + }; + let relative = if path.is_absolute() { + path.strip_prefix(&self.root).map_err(|_| outside())? + } else { + path + }; + let mut resolved = self.root.clone(); + for component in relative.components() { + match component { + Utf8Component::Normal(part) => resolved.push(part), + Utf8Component::CurDir => {} + Utf8Component::ParentDir => { + if resolved == self.root || !resolved.pop() { + return Err(outside()); + } + } + Utf8Component::RootDir | Utf8Component::Prefix(_) => return Err(outside()), + } + } + Ok(resolved) + } + + /// [`resolve`](Self::resolve), then follow the symlinks of as much of the + /// path as exists and check where that leads: a link inside the root to + /// a place outside it resolves outside, however it is spelled. + fn resolve_real(&self, path: &Path) -> Result { + let resolved = self.resolve(path)?; + let mut probe = resolved.as_path(); + loop { + if let Ok(real) = dunce::canonicalize(probe) { + if !real.starts_with(&self.root) { + return Err(FsError::new(OutsideRoot { + path: path.to_path_buf(), + root: self.root.clone(), + })); + } + return Ok(resolved); + } + match probe.parent() { + Some(parent) => probe = parent, + None => return Ok(resolved), + } + } + } +} + +#[cfg(feature = "host")] +#[async_trait] +impl FileSystem for Confined { + async fn read(&self, path: &Path) -> Result, FsError> { + HostFileSystem.read(&self.resolve_real(path)?).await + } + + async fn read_to_string(&self, path: &Path) -> Result { + HostFileSystem + .read_to_string(&self.resolve_real(path)?) + .await + } + + async fn write(&self, path: &Path, contents: &[u8]) -> Result<(), FsError> { + HostFileSystem + .write(&self.resolve_real(path)?, contents) + .await + } + + async fn create_dir_all(&self, path: &Path) -> Result<(), FsError> { + HostFileSystem + .create_dir_all(&self.resolve_real(path)?) + .await + } + + async fn copy(&self, from: &Path, to: &Path) -> Result<(), FsError> { + HostFileSystem + .copy(&self.resolve_real(from)?, &self.resolve_real(to)?) + .await + } + + async fn exists(&self, path: &Path) -> bool { + match self.resolve_real(path) { + Ok(real) => HostFileSystem.exists(&real).await, + Err(_) => false, + } + } + + async fn is_file(&self, path: &Path) -> bool { + match self.resolve_real(path) { + Ok(real) => HostFileSystem.is_file(&real).await, + Err(_) => false, + } + } + + async fn is_dir(&self, path: &Path) -> bool { + match self.resolve_real(path) { + Ok(real) => HostFileSystem.is_dir(&real).await, + Err(_) => false, + } + } + + async fn read_dir(&self, path: &Path) -> Result, FsError> { + HostFileSystem.read_dir(&self.resolve_real(path)?).await + } + + async fn canonicalize(&self, path: &Path) -> Option { + HostFileSystem + .canonicalize(&self.resolve_real(path).ok()?) + .await + .filter(|real| real.starts_with(&self.root)) + } + + async fn scratch_dir(&self) -> Result, FsError> { + let dir = camino_tempfile::Builder::new() + .prefix(".scratch-") + .tempdir_in(&self.root) + .map_err(FsError::new)?; + Ok(Box::new(HostScratch(dir))) + } +} + #[cfg(feature = "host")] /// The [`FileSystem`] backed by this machine's filesystem. #[derive(Debug, Default, Clone, Copy)] @@ -385,6 +578,136 @@ async fn list( mod tests { use super::*; + /// Every spelling of a path resolves under the root or is refused: a + /// relative one is joined, `.` and `..` are applied, an absolute one is + /// taken only under the root, and a symlink inside the root that points + /// out leads nowhere. + #[cfg(feature = "host")] + mod confined { + use super::*; + + fn root() -> (camino_tempfile::Utf8TempDir, Confined) { + let dir = camino_tempfile::Utf8TempDir::new().expect("temp dir"); + std::fs::create_dir_all(dir.path().join("project/canisters")).expect("mkdir"); + std::fs::write(dir.path().join("project/icp.yaml"), b"canisters: []").expect("write"); + std::fs::write(dir.path().join("project/canisters/a.wasm"), b"wasm").expect("write"); + std::fs::write(dir.path().join("outside.txt"), b"secret").expect("write"); + let fs = Confined::new(&dir.path().join("project")).expect("confine"); + (dir, fs) + } + + #[tokio::test] + async fn paths_resolve_under_the_root_by_any_spelling_inside_it() { + let (_dir, fs) = root(); + let root = fs.root().to_path_buf(); + for spelling in [ + "canisters/a.wasm", + "./canisters/a.wasm", + "canisters/../canisters/a.wasm", + root.join("canisters/a.wasm").as_str(), + ] { + assert_eq!(fs.read(Path::new(spelling)).await.expect(spelling), b"wasm"); + assert!(fs.is_file(Path::new(spelling)).await, "{spelling}"); + assert_eq!( + fs.canonicalize(Path::new(spelling)).await, + Some(root.join("canisters/a.wasm")), + "{spelling}" + ); + } + assert!(fs.is_dir(Path::new("canisters")).await); + assert!(fs.is_dir(&root).await); + assert_eq!( + fs.read_dir(&root).await.expect("list"), + vec![root.join("canisters"), root.join("icp.yaml")] + ); + assert!( + fs.resolve(Path::new("made/later")) + .unwrap() + .starts_with(&root) + ); + } + + #[tokio::test] + async fn paths_outside_the_root_are_refused_however_spelled() { + let (dir, fs) = root(); + let outside = dir.path().join("outside.txt"); + assert!( + outside.is_file(), + "the file exists, for the host filesystem to read" + ); + for spelling in [ + "../outside.txt", + "canisters/../../outside.txt", + outside.as_str(), + "/etc/hostname", + "../../../../../../etc/hostname", + ] { + let err = fs + .read(Path::new(spelling)) + .await + .expect_err(spelling) + .to_string(); + assert!(err.contains("is outside"), "{spelling}: {err}"); + assert!(!fs.exists(Path::new(spelling)).await, "{spelling}"); + assert!(!fs.is_file(Path::new(spelling)).await, "{spelling}"); + assert!( + fs.canonicalize(Path::new(spelling)).await.is_none(), + "{spelling}" + ); + assert!(fs.resolve(Path::new(spelling)).is_err(), "{spelling}"); + } + // Nor may anything be written there. + let err = fs + .write(Path::new("../planted"), b"x") + .await + .unwrap_err() + .to_string(); + assert!(err.contains("is outside"), "{err}"); + assert!(!dir.path().join("planted").exists()); + } + + #[cfg(unix)] + #[tokio::test] + async fn a_symlink_inside_pointing_out_is_refused() { + let (dir, fs) = root(); + let root = fs.root().to_path_buf(); + std::os::unix::fs::symlink(dir.path().join("outside.txt"), root.join("link.txt")) + .expect("symlink"); + std::os::unix::fs::symlink(dir.path(), root.join("up")).expect("symlink"); + for spelling in ["link.txt", "up/outside.txt"] { + let err = fs + .read(Path::new(spelling)) + .await + .expect_err(spelling) + .to_string(); + assert!(err.contains("is outside"), "{spelling}: {err}"); + assert!(!fs.exists(Path::new(spelling)).await, "{spelling}"); + } + // A link that leads back inside is inside: what matters is where + // the path comes to rest, not how it got there. + assert_eq!( + fs.read(Path::new("up/project/icp.yaml")).await.unwrap(), + b"canisters: []" + ); + } + + #[tokio::test] + async fn scratch_is_made_under_the_root() { + let (_dir, fs) = root(); + let scratch = fs.scratch_dir().await.expect("scratch"); + assert!(scratch.path().starts_with(fs.root())); + let out = scratch.path().join("out/module.wasm"); + fs.create_dir_all(&scratch.path().join("out")) + .await + .expect("mkdir"); + fs.write(&out, b"built").await.expect("write"); + assert_eq!(fs.read(&out).await.unwrap(), b"built"); + let path = scratch.path().to_path_buf(); + drop(scratch); + assert!(!path.exists()); + } + } + /// Builds a tree of empty files, creating parents as needed, and returns its /// root. fn tree(paths: &[&str]) -> camino_tempfile::Utf8TempDir { diff --git a/crates/icp-project/src/project.rs b/crates/icp-project/src/project.rs index e8f28cfbc..297ccd18f 100644 --- a/crates/icp-project/src/project.rs +++ b/crates/icp-project/src/project.rs @@ -1690,6 +1690,113 @@ pub async fn consolidate_manifest( }) } +/// A project loaded through a [`Confined`] filesystem reads nothing outside +/// its root, whatever its manifest names: the loader resolves every path +/// through the filesystem it is given, so the one rule holds them all. +#[cfg(all(test, feature = "host"))] +mod confined_tests { + use super::*; + use crate::canister::recipe::{Fetched, Resolve, ResolveError}; + use crate::files::{Confined, HostFileSystem}; + use crate::manifest::recipe::Recipe; + use camino_tempfile::Utf8TempDir; + + struct NoRecipes; + + #[async_trait::async_trait] + impl Resolve for NoRecipes { + async fn resolve(&self, _recipe: &Recipe) -> Result { + unreachable!("no manifest here names a recipe") + } + } + + /// A tree with a project directory and, beside it, what the project must + /// not reach: an argument file, and a canister directory of its own. + fn tree(manifest: &str) -> Utf8TempDir { + let dir = Utf8TempDir::new().expect("temp dir"); + let project = dir.path().join("project"); + std::fs::create_dir_all(&project).expect("mkdir"); + std::fs::write(project.join(PROJECT_MANIFEST), manifest).expect("write"); + std::fs::write(dir.path().join("outside.bin"), b"secret").expect("write"); + std::fs::create_dir_all(dir.path().join("elsewhere")).expect("mkdir"); + std::fs::write( + dir.path().join("elsewhere").join(CANISTER_MANIFEST), + "name: stray\nbuild:\n steps:\n - type: pre-built\n path: stray.wasm\n", + ) + .expect("write"); + dir + } + + async fn load(files: &dyn FileSystem, pdir: &Path) -> Result { + let m: ProjectManifest = load_manifest_from_path(files, &pdir.join(PROJECT_MANIFEST)) + .await + .map_err(|e| chain(&e))?; + consolidate_manifest(files, pdir, &NoRecipes, &m) + .await + .map_err(|e| chain(&e)) + } + + fn chain(e: &(dyn std::error::Error + 'static)) -> String { + let mut text = e.to_string(); + let mut source = e.source(); + while let Some(cause) = source { + text.push_str(": "); + text.push_str(&cause.to_string()); + source = cause.source(); + } + text + } + + #[tokio::test] + async fn an_argument_file_outside_the_root_is_read_by_the_host_and_refused_confined() { + let dir = tree(indoc::indoc! {" + canisters: + - name: backend + init_args: + path: ../outside.bin + format: bin + build: + steps: + - type: pre-built + path: backend.wasm + "}); + let pdir = dir.path().join("project"); + + let project = load(&HostFileSystem, &pdir) + .await + .expect("the host reads anywhere"); + let (_, backend) = &project.canisters["backend"]; + assert_eq!( + backend.init_args, + Some(CanisterArgs::Binary(b"secret".to_vec())) + ); + + let confined = Confined::new(&pdir).expect("confine"); + let err = load(&confined, &pdir) + .await + .expect_err("confined, the read is refused"); + assert!(err.contains("init_args"), "{err}"); + assert!(err.contains("is outside"), "{err}"); + } + + #[tokio::test] + async fn a_canister_directory_outside_the_root_is_found_by_the_host_and_not_confined() { + let dir = tree("canisters:\n - ../elsewhere\n"); + let pdir = dir.path().join("project"); + + let project = load(&HostFileSystem, &pdir) + .await + .expect("the host reads anywhere"); + assert!(project.canisters.contains_key("stray")); + + let confined = Confined::new(&pdir).expect("confine"); + let err = load(&confined, &pdir) + .await + .expect_err("confined, the directory is not there"); + assert!(err.contains("../elsewhere"), "{err}"); + } +} + #[cfg(all(test, feature = "host"))] mod recipe_sync_tests { use super::*; From 6f5b535ba00c5eff3e86e59ff1ca2bf3489cd79f Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Wed, 4 Nov 2026 19:00:07 +0000 Subject: [PATCH 2/2] fix(project): hold Confined to the root by spelling alone The symlink walk was the wrong layer. A symlink is the business of whoever puts a tree on disk: bundling writes only regular files and directories, and a consumer unpacking an archive it does not trust refuses any entry that is not, so the tree Confined reads has no links to follow. The user's own tree may link wherever it likes, and the host follows it. Confined now resolves every path lexically against the root and does nothing else. Canonicalizing the root in `new` is also what failed CI on macOS and Windows: the root was stored as `/private/var/...` or `runneradmin`, while every absolute path the loader passed was spelled as the caller got it (`/var/...`, `RUNNER~1`), and so refused. The root is now kept as spelled, and a unix test reproduces the failure through a symlinked alias of the project directory. The doc claim that one seam holds every loader read is narrowed: a local recipe template is read by the `Resolve` implementation, which a consumer holding an untrusted tree must confine or refuse itself. Co-Authored-By: Claude Fable 5.1 --- crates/icp-project/src/files.rs | 194 ++++++++++++++---------------- crates/icp-project/src/project.rs | 10 +- 2 files changed, 94 insertions(+), 110 deletions(-) diff --git a/crates/icp-project/src/files.rs b/crates/icp-project/src/files.rs index e58ddf160..b08c0f5c2 100644 --- a/crates/icp-project/src/files.rs +++ b/crates/icp-project/src/files.rs @@ -88,28 +88,45 @@ pub trait Scratch: Send + Sync { fn path(&self) -> &Path; } -/// This machine's filesystem, held to one directory. +/// This machine's filesystem, held to one directory by the spelling of each +/// path alone. /// /// Every path is resolved against the root before the disk sees it: a -/// relative path is joined to the root, `.` is dropped, `..` is applied, and a -/// path that comes to rest outside the root — by climbing above it, by being -/// absolute somewhere else, or through a symlink inside the root that points -/// out — is refused with [`OutsideRoot`]. The predicates answer `false` for -/// such a path, as they do for any other failure, and scratch directories are -/// made under the root, so what a build writes and reads back stays inside. +/// relative path is joined to the root, `.` is dropped, `..` is applied, and +/// a path that comes to rest outside the root — by climbing above it, or by +/// being absolute somewhere else — is refused with [`OutsideRoot`]. The +/// predicates answer `false` for such a path, as they do for any other +/// failure, and scratch directories are made under the root, so what a build +/// writes and reads back stays inside. /// /// For a project whose files are not the user's own tree: a bundle handed /// over by someone else, which `icp project bundle` wrote with every path /// held to the project it came from, but which the loader would otherwise -/// take at its word. The loader resolves every path a manifest names -/// through the [`FileSystem`] it is given, so confining that one seam holds -/// all of them — the canister directories, the manifests in them, the -/// argument and variable files, the dependency projects — without the -/// loader enumerating its fields. +/// take at its word. The loader resolves every path it reads itself through +/// the [`FileSystem`] it is given, so confining that one seam holds all of +/// them — the canister directories, the manifests in them, the argument and +/// variable files, the dependency projects — without the loader enumerating +/// its fields. A local recipe template is the one file the loader has read +/// for it rather than reading: the [`Resolve`](crate::canister::recipe::Resolve) +/// implementation fetches it, and one holding a tree it does not trust must +/// confine or refuse that itself. +/// +/// The check is lexical, so it holds only for a tree without symlinks. That +/// is the shape a bundle has: `icp project bundle` writes an archive of +/// regular files and directories, and a consumer unpacking one it does not +/// trust refuses any entry that is not, so nothing in the tree can lead +/// elsewhere. A tree that does carry symlinks is followed as the host +/// follows them, and that is deliberate — the user's own project may link +/// wherever it likes. The tree is also taken to be the caller's own while +/// the project is loaded, with nothing else rewriting it underneath. +/// +/// Paths are matched against the root as the caller spelled it, not as it +/// canonicalizes, so a caller that names the root through a symlink (macOS +/// keeps `/var` as a link to `/private/var`) or by its short name (Windows' +/// `RUNNER~1`) is understood as long as it keeps to the same spelling. #[cfg(feature = "host")] #[derive(Debug, Clone)] pub struct Confined { - /// Canonical, so a canonicalized path compares against it. root: PathBuf, } @@ -122,28 +139,13 @@ pub struct OutsideRoot { pub root: PathBuf, } -/// Why a directory cannot be confined to. -#[cfg(feature = "host")] -#[derive(Debug, Snafu)] -pub enum ConfineError { - #[snafu(display("cannot resolve '{path}' to confine the project to it"))] - Resolve { - path: PathBuf, - source: std::io::Error, - }, - #[snafu(display("'{path}' resolves to a path that is not UTF-8"))] - NotUtf8 { path: PathBuf }, -} - #[cfg(feature = "host")] impl Confined { - /// Confine the host filesystem to `root`, which must exist. - pub fn new(root: &Path) -> Result { - let real = dunce::canonicalize(root).context(ResolveSnafu { path: root })?; - let root = PathBuf::from_path_buf(real).map_err(|_| ConfineError::NotUtf8 { - path: root.to_path_buf(), - })?; - Ok(Self { root }) + /// Confine the host filesystem to `root`. + pub fn new(root: &Path) -> Self { + Self { + root: root.to_path_buf(), + } } /// The directory everything is held to. @@ -151,12 +153,11 @@ impl Confined { &self.root } - /// Where `path` lies under the root, by its spelling alone: joined to the - /// root if relative, with `.` dropped and `..` applied, and refused if it - /// would come to rest outside. What a caller that needs a real path for - /// something that does not read through the trait — a runtime mounting a - /// directory — asks for; the trait's own reads also follow symlinks - /// before trusting the answer. + /// Where `path` lies under the root: joined to the root if relative, + /// with `.` dropped and `..` applied, and refused if it would come to + /// rest outside. What a caller that needs a real path for something that + /// does not read through the trait — a runtime mounting a directory — + /// asks for. pub fn resolve(&self, path: &Path) -> Result { let outside = || { FsError::new(OutsideRoot { @@ -184,92 +185,60 @@ impl Confined { } Ok(resolved) } - - /// [`resolve`](Self::resolve), then follow the symlinks of as much of the - /// path as exists and check where that leads: a link inside the root to - /// a place outside it resolves outside, however it is spelled. - fn resolve_real(&self, path: &Path) -> Result { - let resolved = self.resolve(path)?; - let mut probe = resolved.as_path(); - loop { - if let Ok(real) = dunce::canonicalize(probe) { - if !real.starts_with(&self.root) { - return Err(FsError::new(OutsideRoot { - path: path.to_path_buf(), - root: self.root.clone(), - })); - } - return Ok(resolved); - } - match probe.parent() { - Some(parent) => probe = parent, - None => return Ok(resolved), - } - } - } } #[cfg(feature = "host")] #[async_trait] impl FileSystem for Confined { async fn read(&self, path: &Path) -> Result, FsError> { - HostFileSystem.read(&self.resolve_real(path)?).await + HostFileSystem.read(&self.resolve(path)?).await } async fn read_to_string(&self, path: &Path) -> Result { - HostFileSystem - .read_to_string(&self.resolve_real(path)?) - .await + HostFileSystem.read_to_string(&self.resolve(path)?).await } async fn write(&self, path: &Path, contents: &[u8]) -> Result<(), FsError> { - HostFileSystem - .write(&self.resolve_real(path)?, contents) - .await + HostFileSystem.write(&self.resolve(path)?, contents).await } async fn create_dir_all(&self, path: &Path) -> Result<(), FsError> { - HostFileSystem - .create_dir_all(&self.resolve_real(path)?) - .await + HostFileSystem.create_dir_all(&self.resolve(path)?).await } async fn copy(&self, from: &Path, to: &Path) -> Result<(), FsError> { HostFileSystem - .copy(&self.resolve_real(from)?, &self.resolve_real(to)?) + .copy(&self.resolve(from)?, &self.resolve(to)?) .await } async fn exists(&self, path: &Path) -> bool { - match self.resolve_real(path) { - Ok(real) => HostFileSystem.exists(&real).await, + match self.resolve(path) { + Ok(path) => HostFileSystem.exists(&path).await, Err(_) => false, } } async fn is_file(&self, path: &Path) -> bool { - match self.resolve_real(path) { - Ok(real) => HostFileSystem.is_file(&real).await, + match self.resolve(path) { + Ok(path) => HostFileSystem.is_file(&path).await, Err(_) => false, } } async fn is_dir(&self, path: &Path) -> bool { - match self.resolve_real(path) { - Ok(real) => HostFileSystem.is_dir(&real).await, + match self.resolve(path) { + Ok(path) => HostFileSystem.is_dir(&path).await, Err(_) => false, } } async fn read_dir(&self, path: &Path) -> Result, FsError> { - HostFileSystem.read_dir(&self.resolve_real(path)?).await + HostFileSystem.read_dir(&self.resolve(path)?).await } async fn canonicalize(&self, path: &Path) -> Option { - HostFileSystem - .canonicalize(&self.resolve_real(path).ok()?) - .await - .filter(|real| real.starts_with(&self.root)) + HostFileSystem.canonicalize(&self.resolve(path).ok()?).await } async fn scratch_dir(&self) -> Result, FsError> { @@ -579,9 +548,9 @@ mod tests { use super::*; /// Every spelling of a path resolves under the root or is refused: a - /// relative one is joined, `.` and `..` are applied, an absolute one is - /// taken only under the root, and a symlink inside the root that points - /// out leads nowhere. + /// relative one is joined, `.` and `..` are applied, and an absolute one + /// is taken only under the root. By its spelling alone: a symlink is the + /// extractor's business, not this type's. #[cfg(feature = "host")] mod confined { use super::*; @@ -592,7 +561,7 @@ mod tests { std::fs::write(dir.path().join("project/icp.yaml"), b"canisters: []").expect("write"); std::fs::write(dir.path().join("project/canisters/a.wasm"), b"wasm").expect("write"); std::fs::write(dir.path().join("outside.txt"), b"secret").expect("write"); - let fs = Confined::new(&dir.path().join("project")).expect("confine"); + let fs = Confined::new(&dir.path().join("project")); (dir, fs) } @@ -610,7 +579,9 @@ mod tests { assert!(fs.is_file(Path::new(spelling)).await, "{spelling}"); assert_eq!( fs.canonicalize(Path::new(spelling)).await, - Some(root.join("canisters/a.wasm")), + HostFileSystem + .canonicalize(&root.join("canisters/a.wasm")) + .await, "{spelling}" ); } @@ -666,29 +637,40 @@ mod tests { assert!(!dir.path().join("planted").exists()); } + /// What broke on macOS and Windows: the temp dir is reached through + /// a symlink (`/var` to `/private/var`) or a short name (`RUNNER~1`), + /// so a root compared by what it canonicalizes to refused every + /// absolute path spelled the way the caller got it. #[cfg(unix)] #[tokio::test] - async fn a_symlink_inside_pointing_out_is_refused() { - let (dir, fs) = root(); - let root = fs.root().to_path_buf(); - std::os::unix::fs::symlink(dir.path().join("outside.txt"), root.join("link.txt")) + async fn the_root_is_matched_as_the_caller_spells_it() { + let (dir, _) = root(); + std::os::unix::fs::symlink(dir.path().join("project"), dir.path().join("alias")) .expect("symlink"); - std::os::unix::fs::symlink(dir.path(), root.join("up")).expect("symlink"); - for spelling in ["link.txt", "up/outside.txt"] { - let err = fs - .read(Path::new(spelling)) - .await - .expect_err(spelling) - .to_string(); - assert!(err.contains("is outside"), "{spelling}: {err}"); - assert!(!fs.exists(Path::new(spelling)).await, "{spelling}"); - } - // A link that leads back inside is inside: what matters is where - // the path comes to rest, not how it got there. + let spelled = dir.path().join("alias"); + let fs = Confined::new(&spelled); assert_eq!( - fs.read(Path::new("up/project/icp.yaml")).await.unwrap(), + fs.read(&spelled.join("icp.yaml")).await.unwrap(), b"canisters: []" ); + assert_eq!( + fs.read(Path::new("icp.yaml")).await.unwrap(), + b"canisters: []" + ); + assert!(fs.is_dir(&spelled).await); + } + + /// The check is lexical: a symlink is followed as the host follows + /// it. A tree someone else handed over must have had its links + /// refused on the way in; the user's own tree may link where it + /// likes. + #[cfg(unix)] + #[tokio::test] + async fn a_symlink_is_followed_as_the_host_follows_it() { + let (dir, fs) = root(); + std::os::unix::fs::symlink(dir.path().join("outside.txt"), fs.root().join("link.txt")) + .expect("symlink"); + assert_eq!(fs.read(Path::new("link.txt")).await.unwrap(), b"secret"); } #[tokio::test] diff --git a/crates/icp-project/src/project.rs b/crates/icp-project/src/project.rs index 297ccd18f..4e5b8cd38 100644 --- a/crates/icp-project/src/project.rs +++ b/crates/icp-project/src/project.rs @@ -1691,8 +1691,10 @@ pub async fn consolidate_manifest( } /// A project loaded through a [`Confined`] filesystem reads nothing outside -/// its root, whatever its manifest names: the loader resolves every path -/// through the filesystem it is given, so the one rule holds them all. +/// its root, whatever path its manifest names: the loader resolves every +/// path it reads itself through the filesystem it is given, so the one rule +/// holds them all. (A local recipe template is read by the [`Resolve`] +/// implementation instead, which is why these tests supply none.) #[cfg(all(test, feature = "host"))] mod confined_tests { use super::*; @@ -1771,7 +1773,7 @@ mod confined_tests { Some(CanisterArgs::Binary(b"secret".to_vec())) ); - let confined = Confined::new(&pdir).expect("confine"); + let confined = Confined::new(&pdir); let err = load(&confined, &pdir) .await .expect_err("confined, the read is refused"); @@ -1789,7 +1791,7 @@ mod confined_tests { .expect("the host reads anywhere"); assert!(project.canisters.contains_key("stray")); - let confined = Confined::new(&pdir).expect("confine"); + let confined = Confined::new(&pdir); let err = load(&confined, &pdir) .await .expect_err("confined, the directory is not there");