From e5131324084c0b4e3fb27d2f0afb2183cb1c6696 Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Thu, 5 Nov 2026 21:41:21 +0000 Subject: [PATCH 01/10] feat(project): an untrusted project is held to the shape a bundle has A project the CLI loads is trusted the way the user's own checkout is: a build step may run any command, a recipe may come from any file or URL, a module may be downloaded from anywhere, and a manifest may name any path on the machine, through any symlink. A project from elsewhere (a bundle an agent produced, a checkout nobody has read) should get none of that, and until now nothing in the crate said what it should get instead; a consumer such as the imcp2 deploy server had to assemble the rule itself from the seams. `icp_project::untrusted` now states the rule, which is the shape `icp project bundle` writes, and supplies the pieces that hold a project to it: - `files::Confined::refusing_links` is the confined filesystem of #866 with one more refusal: a path that passes through a symlink anywhere under the root, wherever the link leads. The lexical `Confined::new` stays as it is, for a tree whose links were refused on the way in. Each component below the root is looked at as it is on disk, so no canonicalizing, and the root is still matched as the caller spells it. - `untrusted::NoRecipes` is the `Resolve` that fetches nothing, so a project naming a recipe fails to load rather than loading and then being refused: the recipe is build instructions read from wherever the manifest points, and the loader reads it before anyone can look. - `untrusted::vet` refuses a loaded project with a script build or sync step, or a module or sync plugin fetched from a URL, and `untrusted::Vetted` wraps a `ProjectLoad` so every load is vetted. The CLI gets a global `--untrusted` flag that composes the three: the project root is located up front, the filesystem is confined to it and refuses links, no recipe is fetched, and the loader is vetted. Nothing changes without the flag. A checksum on a module is not asked for: the same untrusted hand wrote the module and the checksum, so it says nothing about what the project can reach. Ref: #863, #865 Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 1 + crates/icp-app/src/context/init.rs | 53 ++++- crates/icp-cli/src/complete.rs | 1 + crates/icp-cli/src/main.rs | 8 + crates/icp-cli/tests/untrusted_tests.rs | 222 ++++++++++++++++++ crates/icp-project/src/files.rs | 168 +++++++++++++- crates/icp-project/src/lib.rs | 4 + crates/icp-project/src/untrusted.rs | 294 ++++++++++++++++++++++++ docs/reference/cli.md | 3 + 9 files changed, 740 insertions(+), 14 deletions(-) create mode 100644 crates/icp-cli/tests/untrusted_tests.rs create mode 100644 crates/icp-project/src/untrusted.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index bd34d7523..b05eb685f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ air-gapped signing, extensions # Unreleased +* feat: `--untrusted`, a global flag, treats the project as one someone else handed over rather than the user's own: a bundle an agent produced, a checkout nobody has read. The project is loaded through a filesystem held to the project root that refuses a symlink anywhere under it, so a manifest can name no file outside the project and no link to one; no recipe is fetched; and a project with a script build or sync step, or a module or sync plugin fetched from a URL, is refused before any command acts on it. What is left is the shape `icp project bundle` writes. Without the flag nothing changes: the user's own project may still run scripts, use recipes, and link or reach wherever it likes. * chore: macOS on Intel (`x86_64-apple-darwin`) is no longer a release target. Releases ship no Intel macOS archive, the shell installer refuses that platform, and npm no longer publishes `@icp-sdk/icp-cli-darwin-x64`, so installing `@icp-sdk/icp-cli` on an Intel Mac warns of an unsupported platform. Homebrew dropped Intel macOS in September 2026, and GitHub is retiring its Intel macOS runners. v1.8.0 is the last release with an Intel macOS build. ## Experimental diff --git a/crates/icp-app/src/context/init.rs b/crates/icp-app/src/context/init.rs index b0dc0bf0d..ea13813fa 100644 --- a/crates/icp-app/src/context/init.rs +++ b/crates/icp-app/src/context/init.rs @@ -12,7 +12,14 @@ use icp_project::store_artifact::ArtifactStore; use std::time::Duration; use crate::{agent, identity, identity::PasswordFunc}; -use icp_project::{Lazy, ProjectLoadImpl, host::Host, manifest, store_id}; +use icp_project::{ + Lazy, ProjectLoad, ProjectLoadImpl, + canister::recipe::Resolve, + files::{Confined, FileSystem, HostFileSystem}, + host::Host, + manifest::{self, ProjectRootLocate as _}, + store_id, untrusted, +}; #[derive(Debug, Snafu)] pub enum ContextInitError { @@ -36,13 +43,25 @@ pub enum ContextInitError { PackageCache { source: icp_project::fs::lock::LockError, }, + + #[snafu(display("failed to locate the project to treat as untrusted"))] + UntrustedRoot { + source: manifest::ProjectRootLocateError, + }, } +/// `untrusted` treats the project as one someone else handed over: it is +/// loaded through a filesystem confined to its root that refuses symlinks, no +/// recipe is fetched, and a project with a script step or a remote module or +/// plugin is refused before any command acts on it. See +/// [`icp_project::untrusted`]. The root is located up front, since the +/// filesystem is held to it from the start, so there has to be a project. pub fn initialize( project_root_override: Option, debug: bool, password_func: PasswordFunc, pem_session_duration: Option, + untrusted: bool, ) -> Result { // Setup global directory structure let dirs = Arc::new(Directories::new().context(DirectoriesSnafu)?); @@ -100,14 +119,25 @@ pub fn initialize( pkg_cache.clone(), )); - // Recipes - let recipe = Arc::new(RecipeFetcher { - http_client, - pkg_cache, - }); + // Recipes: fetched from wherever the manifest points, unless the project + // is untrusted, in which case none is. + let recipe: Arc = if untrusted { + Arc::new(untrusted::NoRecipes) + } else { + Arc::new(RecipeFetcher { + http_client, + pkg_cache, + }) + }; - // The project's files come from this machine. - let files = Arc::new(icp_project::files::HostFileSystem); + // The project's files come from this machine: the whole of it for the + // user's own project, the project directory alone for an untrusted one. + let files: Arc = if untrusted { + let root = project_root_locate.locate().context(UntrustedRootSnafu)?; + Arc::new(Confined::refusing_links(&root)) + } else { + Arc::new(HostFileSystem) + }; // Canister builder let builder = Arc::new(Builder::new(wasm.clone(), files.clone())); @@ -125,8 +155,11 @@ pub fn initialize( files: files.clone(), }; - let pload = Lazy::new(pload); - let pload = Arc::new(pload); + let pload: Arc = if untrusted { + Arc::new(Lazy::new(untrusted::Vetted::new(pload))) + } else { + Arc::new(Lazy::new(pload)) + }; // Telemetry data bag (written by subsystems, read at session finish) let telemetry_data = Arc::new(crate::telemetry_data::TelemetryData::default()); diff --git a/crates/icp-cli/src/complete.rs b/crates/icp-cli/src/complete.rs index be3e877ee..d181d91ce 100644 --- a/crates/icp-cli/src/complete.rs +++ b/crates/icp-cli/src/complete.rs @@ -66,6 +66,7 @@ fn context() -> Option<&'static Context> { false, Arc::new(|| Err("cannot prompt while completing".to_string())), None, + false, ) .ok() }) diff --git a/crates/icp-cli/src/main.rs b/crates/icp-cli/src/main.rs index 8cf2e5e64..0a0f1342c 100644 --- a/crates/icp-cli/src/main.rs +++ b/crates/icp-cli/src/main.rs @@ -88,6 +88,12 @@ struct Cli { #[arg(long, global = true, value_name = "FILE", help_heading = heading::GLOBAL_PARAMETERS, value_hint = ValueHint::FilePath)] identity_password_file: Option, + /// Treat the project as one someone else handed over: refuse a script + /// build or sync step, a recipe, a module or plugin fetched from a URL, + /// a path outside the project root, and a symlink under it + #[arg(long, default_value = "false", global = true, help_heading = heading::GLOBAL_PARAMETERS)] + untrusted: bool, + /// Generate markdown documentation for all commands and exit #[arg(long, hide = true)] markdown_help: bool, @@ -135,6 +141,7 @@ async fn run() -> Result<(), Error> { identity_password_file: matches .get_one::("identity_password_file") .cloned(), + untrusted: matches.get_flag("untrusted"), markdown_help: false, command: None, }, @@ -214,6 +221,7 @@ async fn run() -> Result<(), Error> { cli.debug, password_func, pem_session_duration, + cli.untrusted, )?; // Extension commands are left out of telemetry: telemetry parses against diff --git a/crates/icp-cli/tests/untrusted_tests.rs b/crates/icp-cli/tests/untrusted_tests.rs new file mode 100644 index 000000000..4064a800f --- /dev/null +++ b/crates/icp-cli/tests/untrusted_tests.rs @@ -0,0 +1,222 @@ +//! `--untrusted` holds a project to what one from elsewhere may do: see +//! `icp_project::untrusted`. Each test here sets up a tree the CLI would act +//! on as the user's own, and shows the flag refusing it, naming what it +//! refused. The one tree the flag accepts is the shape a bundle has. + +use indoc::indoc; +use predicates::{prelude::PredicateBooleanExt, str::contains}; + +use crate::common::TestContext; +use icp_project::fs::write_string; + +mod common; + +/// A project directory holding a module of its own, and beside it a file the +/// project must not reach. +fn project(ctx: &TestContext, manifest: &str) -> camino::Utf8PathBuf { + let project_dir = ctx.create_project_dir("icp"); + let wasm = ctx.make_asset("example_icp_mo.wasm"); + std::fs::copy(&wasm, project_dir.join("backend.wasm")).expect("copy wasm"); + std::fs::write(project_dir.parent().unwrap().join("outside.bin"), b"secret") + .expect("write outside"); + write_string(&project_dir.join("icp.yaml"), manifest).expect("write manifest"); + project_dir +} + +#[test] +fn a_bundle_shaped_project_builds_as_untrusted() { + let ctx = TestContext::new(); + let project_dir = project( + &ctx, + indoc! {r#" + canisters: + - name: backend + build: + steps: + - type: pre-built + path: backend.wasm + "#}, + ); + + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .success(); +} + +#[test] +fn a_script_step_is_refused_as_untrusted_but_not_otherwise() { + let ctx = TestContext::new(); + let project_dir = project( + &ctx, + indoc! {r#" + canisters: + - name: backend + build: + steps: + - type: script + command: cp backend.wasm "$ICP_WASM_OUTPUT_PATH" + "#}, + ); + + ctx.icp() + .current_dir(&project_dir) + .args(["build"]) + .assert() + .success(); + + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .failure() + .stderr( + contains("more than an untrusted project may") + .and(contains("canister 'backend' has a script build step")), + ); +} + +#[test] +fn a_recipe_is_not_fetched_as_untrusted() { + let ctx = TestContext::new(); + let project_dir = project( + &ctx, + indoc! {r#" + canisters: + - name: backend + recipe: + type: file://recipe.hbs + "#}, + ); + std::fs::write( + project_dir.join("recipe.hbs"), + "build:\n steps:\n - type: pre-built\n path: backend.wasm\n", + ) + .expect("write recipe"); + + ctx.icp() + .current_dir(&project_dir) + .args(["build"]) + .assert() + .success(); + + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .failure() + .stderr(contains("recipe 'file://recipe.hbs' is not fetched")); +} + +#[test] +fn a_path_outside_the_project_is_refused_as_untrusted() { + let ctx = TestContext::new(); + let project_dir = project( + &ctx, + indoc! {r#" + canisters: + - name: backend + init_args: + path: ../outside.bin + format: bin + build: + steps: + - type: pre-built + path: backend.wasm + "#}, + ); + + ctx.icp() + .current_dir(&project_dir) + .args(["build"]) + .assert() + .success(); + + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .failure() + .stderr(contains("outside.bin").and(contains("is outside"))); +} + +#[cfg(unix)] +#[test] +fn a_symlink_under_the_project_is_refused_as_untrusted() { + let ctx = TestContext::new(); + let project_dir = project( + &ctx, + indoc! {r#" + canisters: + - name: backend + init_args: + path: args.bin + format: bin + build: + steps: + - type: pre-built + path: backend.wasm + "#}, + ); + std::os::unix::fs::symlink( + project_dir.parent().unwrap().join("outside.bin"), + project_dir.join("args.bin"), + ) + .expect("symlink"); + + ctx.icp() + .current_dir(&project_dir) + .args(["build"]) + .assert() + .success(); + + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .failure() + .stderr( + contains("args.bin") + .and(contains("passes through")) + .and(contains("symlink")), + ); +} + +#[test] +fn untrusted_needs_a_project_to_hold_to() { + let ctx = TestContext::new(); + let empty = ctx.home_path().join("nowhere"); + std::fs::create_dir_all(&empty).expect("mkdir"); + + ctx.icp() + .current_dir(&empty) + .args(["identity", "list", "--untrusted"]) + .assert() + .failure() + .stderr(contains( + "failed to locate the project to treat as untrusted", + )); +} + +#[test] +fn the_flag_is_global() { + let ctx = TestContext::new(); + let project_dir = project( + &ctx, + indoc! {r#" + canisters: + - name: backend + build: + steps: + - type: pre-built + path: backend.wasm + "#}, + ); + + ctx.icp() + .current_dir(&project_dir) + .args(["--untrusted", "build", "backend"]) + .assert() + .success(); +} diff --git a/crates/icp-project/src/files.rs b/crates/icp-project/src/files.rs index e392c5de0..8a3729e95 100644 --- a/crates/icp-project/src/files.rs +++ b/crates/icp-project/src/files.rs @@ -104,10 +104,33 @@ pub trait Scratch: Send + Sync { /// The check is lexical, so it holds only for a tree without symlinks, which /// is what a bundle is; a link is followed as the host follows it. The root /// is matched as the caller spelled it, not as it canonicalizes. +/// +/// A tree that nobody has held to that shape — a directory on disk that +/// someone else wrote, rather than an archive unpacked by a consumer that +/// refused its links — is asked for through +/// [`refusing_links`](Self::refusing_links) instead. Each component of a +/// path below the root is then looked at as it is on disk, and a path that +/// passes through a symlink anywhere under the root is refused with +/// [`ThroughLink`], wherever the link leads: one that points inside the root +/// is as unwelcome as one that points out, since the rule is the shape of the +/// tree rather than where any one link goes. The root itself and everything +/// above it are the caller's, and are not looked at. #[cfg(feature = "host")] #[derive(Debug, Clone)] pub struct Confined { root: PathBuf, + links: Links, +} + +/// What a [`Confined`] filesystem does with a symlink under its root. +#[cfg(feature = "host")] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Links { + /// Follow it, as the host does. For a tree that has none, or whose links + /// are the caller's own. + Follow, + /// Refuse any path that passes through one. + Refuse, } /// A path a [`Confined`] filesystem was asked for that is not under its root. @@ -119,12 +142,35 @@ pub struct OutsideRoot { pub root: PathBuf, } +/// A path a [`Confined`] filesystem that refuses links was asked for that +/// passes through one. +#[cfg(feature = "host")] +#[derive(Debug, Snafu)] +#[snafu(display( + "'{path}' passes through '{link}', which is a symlink, and this project may not have any" +))] +pub struct ThroughLink { + pub path: PathBuf, + pub link: PathBuf, +} + #[cfg(feature = "host")] impl Confined { - /// Confine the host filesystem to `root`. + /// Confine the host filesystem to `root`, following any symlink under it + /// as the host does. pub fn new(root: &Path) -> Self { Self { root: root.to_path_buf(), + links: Links::Follow, + } + } + + /// Confine the host filesystem to `root`, and refuse any path that passes + /// through a symlink under it. + pub fn refusing_links(root: &Path) -> Self { + Self { + root: root.to_path_buf(), + links: Links::Refuse, } } @@ -135,9 +181,10 @@ impl Confined { /// 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. + /// rest outside — or, for a filesystem that refuses links, if it passes + /// through one on the way there. 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 { @@ -163,8 +210,40 @@ impl Confined { Utf8Component::RootDir | Utf8Component::Prefix(_) => return Err(outside()), } } + if self.links == Links::Refuse { + self.refuse_links(path, &resolved)?; + } Ok(resolved) } + + /// Refuse `resolved` if any component of it below the root is a symlink. + /// + /// Each component is looked at as it is on disk rather than through + /// what it points at, so a link is seen wherever it leads, and a dangling + /// one is seen too. The walk stops at the first component that is not + /// there: nothing beyond it can be a link, and whether the path exists + /// is for the operation itself to find out and report. + fn refuse_links(&self, path: &Path, resolved: &Path) -> Result<(), FsError> { + let mut at = self.root.clone(); + for component in resolved + .strip_prefix(&self.root) + .expect("resolved under the root") + .components() + { + at.push(component.as_str()); + match std::fs::symlink_metadata(&at) { + Ok(meta) if meta.file_type().is_symlink() => { + return Err(FsError::new(ThroughLink { + path: path.to_path_buf(), + link: at, + })); + } + Ok(_) => {} + Err(_) => break, + } + } + Ok(()) + } } #[cfg(feature = "host")] @@ -652,6 +731,87 @@ mod tests { assert_eq!(fs.read(Path::new("link.txt")).await.unwrap(), b"secret"); } + /// Asked to refuse links, the filesystem refuses a path through one + /// wherever it leads: out of the root, back inside it, or nowhere. + /// A link in the middle of a path counts as much as one at its end. + #[cfg(unix)] + #[tokio::test] + async fn refusing_links_refuses_a_path_through_one_wherever_it_leads() { + use std::os::unix::fs::symlink; + let (dir, _) = root(); + let project = dir.path().join("project"); + symlink(dir.path().join("outside.txt"), project.join("out.txt")).expect("symlink"); + symlink(project.join("icp.yaml"), project.join("in.yaml")).expect("symlink"); + symlink(project.join("nowhere"), project.join("dangling")).expect("symlink"); + symlink(project.join("canisters"), project.join("dir-link")).expect("symlink"); + let fs = Confined::refusing_links(&project); + + for (spelling, link) in [ + ("out.txt", "out.txt"), + ("in.yaml", "in.yaml"), + ("dangling", "dangling"), + ("dir-link/a.wasm", "dir-link"), + ("./dir-link/../dir-link/a.wasm", "dir-link"), + ] { + let err = fs + .read(Path::new(spelling)) + .await + .expect_err(spelling) + .to_string(); + assert!(err.contains("passes through"), "{spelling}: {err}"); + assert!(err.contains(link), "{spelling}: {err}"); + assert!(!fs.exists(Path::new(spelling)).await, "{spelling}"); + assert!(!fs.is_file(Path::new(spelling)).await, "{spelling}"); + assert!(!fs.is_dir(Path::new(spelling)).await, "{spelling}"); + assert!(fs.resolve(Path::new(spelling)).is_err(), "{spelling}"); + } + // Nor may a link be written through. + let err = fs + .write(Path::new("out.txt"), b"x") + .await + .unwrap_err() + .to_string(); + assert!(err.contains("passes through"), "{err}"); + assert_eq!( + std::fs::read(dir.path().join("outside.txt")).unwrap(), + b"secret" + ); + + // Regular files and directories are read as before, and a path + // that is not there yet resolves, since nothing on it is a link. + assert_eq!( + fs.read(Path::new("canisters/a.wasm")).await.unwrap(), + b"wasm" + ); + assert!(fs.is_dir(Path::new("canisters")).await); + assert!(fs.resolve(Path::new("made/later")).is_ok()); + assert_eq!( + fs.read_dir(&project).await.expect("list").len(), + 6, + "listing shows the links; reading through them is what is refused" + ); + } + + /// The root itself may be reached through a link: it is the caller's + /// spelling, and only what lies below it is held to shape. + #[cfg(unix)] + #[tokio::test] + async fn refusing_links_still_takes_the_root_as_the_caller_spells_it() { + let (dir, _) = root(); + std::os::unix::fs::symlink(dir.path().join("project"), dir.path().join("alias")) + .expect("symlink"); + let spelled = dir.path().join("alias"); + let fs = Confined::refusing_links(&spelled); + assert_eq!( + fs.read(&spelled.join("icp.yaml")).await.unwrap(), + b"canisters: []" + ); + assert_eq!( + fs.read(Path::new("canisters/a.wasm")).await.unwrap(), + b"wasm" + ); + } + #[tokio::test] async fn scratch_is_made_under_the_root() { let (_dir, fs) = root(); diff --git a/crates/icp-project/src/lib.rs b/crates/icp-project/src/lib.rs index ec17458fc..5c2517d7d 100644 --- a/crates/icp-project/src/lib.rs +++ b/crates/icp-project/src/lib.rs @@ -43,6 +43,7 @@ pub mod random; pub mod signal; pub mod store_artifact; pub mod store_id; +pub mod untrusted; /// The per-project state directory. `cache` holds what can be thrown away and /// rebuilt (ids for managed networks, build artifacts); `data` holds what @@ -219,6 +220,9 @@ pub enum ProjectLoadError { Project { source: project::ConsolidateManifestError, }, + + #[snafu(display("the project asks for more than an untrusted project may"))] + Untrusted { source: untrusted::Refused }, } #[async_trait] diff --git a/crates/icp-project/src/untrusted.rs b/crates/icp-project/src/untrusted.rs new file mode 100644 index 000000000..17f9ec4c3 --- /dev/null +++ b/crates/icp-project/src/untrusted.rs @@ -0,0 +1,294 @@ +//! Holding a project someone else handed over to what it may do. +//! +//! The CLI trusts a project the way it trusts the user's own checkout: a +//! build step may run any command, a recipe may come from any file or URL, a +//! module may be downloaded from anywhere, and a manifest may name any path on +//! the machine. A project from elsewhere — an archive an agent passed to a +//! deploy service, a checkout the user has not read — gets none of that. What +//! it may do is the shape `icp project bundle` writes, since a bundle is what +//! such a project usually is: +//! +//! - no script step, for building or syncing: those are host subprocesses; +//! - no recipe: a recipe is build instructions fetched from a file or a URL +//! at load, and a bundle has expanded every one of them; +//! - no remote module or plugin: the project carries what it installs; +//! - no path outside the project root, and no symlink under it. +//! +//! The last is the business of the [`FileSystem`](crate::files::FileSystem) +//! the project is loaded through — a [`Confined`](crate::files::Confined) +//! that refuses links — because the loader reads argument and variable files +//! into the model as it loads, before any caller could look. The recipe rule +//! is the business of the [`Resolve`] seam for the same reason: the loader +//! asks it for the template, and [`NoRecipes`] says no. The rest can be seen +//! in the loaded project, and [`vet`] refuses it there. [`Vetted`] wraps a +//! loader so that every load is vetted before the project is handed out, and +//! a consumer composing its own host holds the three together: +//! +//! ```ignore +//! let files = Arc::new(Confined::refusing_links(&root)); +//! let project = Vetted::new(ProjectLoadImpl { +//! project_root_locate, +//! recipe: Arc::new(NoRecipes), +//! files: files.clone(), +//! }); +//! ``` +//! +//! A checksum on a module is not asked for. It is the bundler's promise about +//! its own archive, and the same untrusted hand wrote both, so it says nothing +//! about what the project can reach. + +use async_trait::async_trait; +use snafu::{ResultExt, Snafu}; + +use crate::{ + Project, ProjectLoad, ProjectLoadError, UntrustedSnafu, + canister::recipe::{Fetched, Resolve, ResolveError}, + manifest::{ + adapter::prebuilt::SourceField, + canister::{BuildStep, SyncStep}, + recipe::Recipe, + }, + prelude::*, +}; + +/// Why a loaded project may not be acted on as untrusted. +#[derive(Debug, Snafu)] +pub enum Refused { + #[snafu(display( + "canister '{canister}' has a script build step, which an untrusted project may not run" + ))] + ScriptBuildStep { canister: String }, + + #[snafu(display( + "canister '{canister}' has a script sync step, which an untrusted project may not run" + ))] + ScriptSyncStep { canister: String }, + + #[snafu(display( + "canister '{canister}' fetches its module from '{url}'; an untrusted project must carry \ + its modules" + ))] + RemoteModule { canister: String, url: String }, + + #[snafu(display( + "canister '{canister}' fetches a sync plugin from '{url}'; an untrusted project must \ + carry its plugins" + ))] + RemotePlugin { canister: String, url: String }, +} + +/// Refuse a loaded project that asks for more than an untrusted one may. +/// +/// Looks at every canister's build and sync steps. What the loader has +/// already read or fetched — argument files, recipe templates — is not here +/// to look at, and is held by the seams it was read through instead. +pub fn vet(project: &Project) -> Result<(), Refused> { + for (_, (_, canister)) in &project.canisters { + let name = &canister.name; + for step in &canister.build.steps { + match step { + BuildStep::Script(_) => { + return ScriptBuildStepSnafu { canister: name }.fail(); + } + BuildStep::Prebuilt(adapter) => { + if let SourceField::Remote(remote) = &adapter.source { + return RemoteModuleSnafu { + canister: name, + url: &remote.url, + } + .fail(); + } + } + } + } + for step in &canister.sync.steps { + match step { + SyncStep::Script(_) => { + return ScriptSyncStepSnafu { canister: name }.fail(); + } + SyncStep::Plugin(adapter) => { + if let SourceField::Remote(remote) = &adapter.source { + return RemotePluginSnafu { + canister: name, + url: &remote.url, + } + .fail(); + } + } + } + } + } + Ok(()) +} + +/// The [`Resolve`] that fetches nothing. +/// +/// A recipe is resolved while the project loads, so a project that names one +/// fails to load rather than loading and then being refused — the one place +/// the rule can be applied before the template is read from wherever the +/// manifest points. +pub struct NoRecipes; + +/// A recipe named by a project loaded as untrusted. +#[derive(Debug, Snafu)] +#[snafu(display( + "recipe '{recipe}' is not fetched: an untrusted project must spell out its build and sync \ + steps, as a bundle does" +))] +pub struct RecipeRefused { + pub recipe: String, +} + +#[async_trait] +impl Resolve for NoRecipes { + async fn resolve(&self, recipe: &Recipe) -> Result { + Err(ResolveError::new(RecipeRefused { + recipe: recipe.recipe_type.to_string(), + })) + } +} + +/// A [`ProjectLoad`] that hands out only a project [`vet`] accepts. +pub struct Vetted(L); + +impl Vetted { + pub fn new(loader: L) -> Self { + Self(loader) + } +} + +#[async_trait] +impl ProjectLoad for Vetted { + async fn load(&self) -> Result { + let project = self.0.load().await?; + vet(&project).context(UntrustedSnafu)?; + Ok(project) + } + + async fn exists(&self) -> Result { + self.0.exists().await + } + + fn member_dir(&self) -> Option { + self.0.member_dir() + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::{ + MockProjectLoader, + manifest::{ + adapter::{prebuilt, script}, + canister::{BuildSteps, SyncSteps}, + }, + }; + + async fn project_with(build: Vec, sync: Vec) -> Project { + let mut project = MockProjectLoader::minimal().load().await.unwrap(); + let (_, canister) = project.canisters.get_mut("backend").unwrap(); + canister.build = BuildSteps { steps: build }; + canister.sync = SyncSteps { steps: sync }; + project + } + + fn local(path: &str) -> BuildStep { + BuildStep::Prebuilt(prebuilt::Adapter { + source: SourceField::Local(prebuilt::LocalSource { path: path.into() }), + sha256: None, + }) + } + + fn script(command: &str) -> script::Adapter { + script::Adapter { + command: script::CommandField::Command(command.to_owned()), + } + } + + fn plugin(source: &str) -> SyncStep { + SyncStep::Plugin(Box::new( + serde_yaml::from_str(&format!("{source}\nsha256: abc")).unwrap(), + )) + } + + #[tokio::test] + async fn a_bundle_shaped_project_is_accepted() { + let project = project_with(vec![local("backend.wasm")], vec![plugin("path: p.wasm")]).await; + vet(&project).expect("pre-built from the project, plugin from the project"); + } + + #[tokio::test] + async fn a_script_step_is_refused() { + let err = + vet(&project_with(vec![BuildStep::Script(script("make"))], vec![]).await).unwrap_err(); + assert!(matches!(err, Refused::ScriptBuildStep { ref canister } if canister == "backend")); + assert!(err.to_string().contains("script build step"), "{err}"); + + let err = vet(&project_with( + vec![local("backend.wasm")], + vec![SyncStep::Script(script("upload"))], + ) + .await) + .unwrap_err(); + assert!(matches!(err, Refused::ScriptSyncStep { .. })); + assert!(err.to_string().contains("script sync step"), "{err}"); + } + + #[tokio::test] + async fn a_remote_module_or_plugin_is_refused() { + let remote = BuildStep::Prebuilt(prebuilt::Adapter { + source: SourceField::Remote(prebuilt::RemoteSource { + url: "https://example.com/backend.wasm".into(), + }), + sha256: Some("abc".into()), + }); + let err = vet(&project_with(vec![remote], vec![]).await).unwrap_err(); + assert!(matches!(err, Refused::RemoteModule { .. })); + assert!( + err.to_string().contains("example.com/backend.wasm"), + "{err}" + ); + + let err = vet(&project_with( + vec![local("backend.wasm")], + vec![plugin("url: https://example.com/plugin.wasm")], + ) + .await) + .unwrap_err(); + assert!(matches!(err, Refused::RemotePlugin { .. })); + assert!(err.to_string().contains("example.com/plugin.wasm"), "{err}"); + } + + #[tokio::test] + async fn no_recipe_is_fetched_whatever_it_names() { + for spec in [ + "file://recipe.hbs", + "https://example.com/recipe.hbs", + "@dfinity/rust@v1", + ] { + let recipe: Recipe = serde_yaml::from_str(&format!("type: \"{spec}\"")).unwrap(); + let err = match NoRecipes.resolve(&recipe).await { + Ok(_) => panic!("{spec} was fetched"), + Err(err) => err.to_string(), + }; + assert!(err.contains("not fetched"), "{spec}: {err}"); + assert!(err.contains(spec), "{spec}: {err}"); + } + } + + #[tokio::test] + async fn a_vetted_loader_refuses_what_vet_refuses() { + let project = project_with(vec![BuildStep::Script(script("make"))], vec![]).await; + let loader = Vetted::new(MockProjectLoader::new(project)); + assert!(loader.exists().await.unwrap()); + let err = loader.load().await.unwrap_err(); + assert!(matches!(err, ProjectLoadError::Untrusted { .. }), "{err}"); + + let loader = Vetted::new(MockProjectLoader::minimal()); + loader + .load() + .await + .expect("the minimal project is pre-built"); + } +} diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 9c14edebb..89243e29e 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -120,6 +120,9 @@ This document contains the help content for the `icp` command-line program. Default value: `false` * `--identity-password-file ` — Read identity password from a file instead of prompting +* `--untrusted` — Treat the project as one someone else handed over: refuse a script build or sync step, a recipe, a module or plugin fetched from a URL, a path outside the project root, and a symlink under it + + Default value: `false` From b88f5b0e8a2d80345a785563cf3c54275f5638a6 Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Thu, 5 Nov 2026 22:00:30 +0000 Subject: [PATCH 02/10] refactor(project): say what an untrusted step may be, not what it may not `untrusted::vet` listed the steps it refused: a script, a module from a URL, a plugin from a URL. A kind of step added later would have passed unexamined. It now holds every step to the two an untrusted project may have, `build_step_allowed` (a pre-built module read from a file in the project) and `sync_step_allowed` (a plugin read from a file in the project), and names the first step that is neither, with the step itself on one line so the reader sees what it was. Co-Authored-By: Claude Fable 5.1 --- crates/icp-cli/tests/untrusted_tests.rs | 5 +- crates/icp-project/src/untrusted.rs | 191 +++++++++++++++--------- 2 files changed, 122 insertions(+), 74 deletions(-) diff --git a/crates/icp-cli/tests/untrusted_tests.rs b/crates/icp-cli/tests/untrusted_tests.rs index 4064a800f..462b02d48 100644 --- a/crates/icp-cli/tests/untrusted_tests.rs +++ b/crates/icp-cli/tests/untrusted_tests.rs @@ -73,7 +73,10 @@ fn a_script_step_is_refused_as_untrusted_but_not_otherwise() { .failure() .stderr( contains("more than an untrusted project may") - .and(contains("canister 'backend' has a script build step")), + .and(contains( + "canister 'backend' has a build step that is not a pre-built module", + )) + .and(contains("(script) cp backend.wasm")), ); } diff --git a/crates/icp-project/src/untrusted.rs b/crates/icp-project/src/untrusted.rs index 17f9ec4c3..db821e42e 100644 --- a/crates/icp-project/src/untrusted.rs +++ b/crates/icp-project/src/untrusted.rs @@ -6,23 +6,28 @@ //! the machine. A project from elsewhere — an archive an agent passed to a //! deploy service, a checkout the user has not read — gets none of that. What //! it may do is the shape `icp project bundle` writes, since a bundle is what -//! such a project usually is: +//! such a project usually is, and it is said as what is allowed rather than +//! what is not, so that a kind of step added later is refused until someone +//! decides otherwise: //! -//! - no script step, for building or syncing: those are host subprocesses; -//! - no recipe: a recipe is build instructions fetched from a file or a URL -//! at load, and a bundle has expanded every one of them; -//! - no remote module or plugin: the project carries what it installs; -//! - no path outside the project root, and no symlink under it. +//! - a build step is a pre-built module read from a file in the project +//! ([`build_step_allowed`]); +//! - a sync step is a plugin read from a file in the project +//! ([`sync_step_allowed`]); +//! - a recipe is never fetched: it is build instructions read from a file or +//! a URL at load, and a bundle has expanded every one of them; +//! - a path names something under the project root, through no symlink. //! //! The last is the business of the [`FileSystem`](crate::files::FileSystem) //! the project is loaded through — a [`Confined`](crate::files::Confined) //! that refuses links — because the loader reads argument and variable files //! into the model as it loads, before any caller could look. The recipe rule //! is the business of the [`Resolve`] seam for the same reason: the loader -//! asks it for the template, and [`NoRecipes`] says no. The rest can be seen -//! in the loaded project, and [`vet`] refuses it there. [`Vetted`] wraps a -//! loader so that every load is vetted before the project is handed out, and -//! a consumer composing its own host holds the three together: +//! asks it for the template, and [`NoRecipes`] says no. The steps can be seen +//! in the loaded project, and [`vet`] holds them to the list there. +//! [`Vetted`] wraps a loader so that every load is vetted before the project +//! is handed out, and a consumer composing its own host holds the three +//! together: //! //! ```ignore //! let files = Arc::new(Confined::refusing_links(&root)); @@ -55,72 +60,84 @@ use crate::{ #[derive(Debug, Snafu)] pub enum Refused { #[snafu(display( - "canister '{canister}' has a script build step, which an untrusted project may not run" + "canister '{canister}' has a build step that is not a pre-built module from a file in \ + the project, which is all an untrusted project may have: {step}" ))] - ScriptBuildStep { canister: String }, + BuildStep { canister: String, step: String }, #[snafu(display( - "canister '{canister}' has a script sync step, which an untrusted project may not run" + "canister '{canister}' has a sync step that is not a plugin from a file in the project, \ + which is all an untrusted project may have: {step}" ))] - ScriptSyncStep { canister: String }, + SyncStep { canister: String, step: String }, +} - #[snafu(display( - "canister '{canister}' fetches its module from '{url}'; an untrusted project must carry \ - its modules" - ))] - RemoteModule { canister: String, url: String }, +/// Whether `step` is the one build step an untrusted project may have: a +/// pre-built module read from a file in the project. A script is a host +/// subprocess, and a module fetched from a URL reaches the network and the +/// package cache on the strength of an untrusted manifest. +pub fn build_step_allowed(step: &BuildStep) -> bool { + matches!(step, BuildStep::Prebuilt(adapter) if matches!(adapter.source, SourceField::Local(_))) +} - #[snafu(display( - "canister '{canister}' fetches a sync plugin from '{url}'; an untrusted project must \ - carry its plugins" - ))] - RemotePlugin { canister: String, url: String }, +/// Whether `step` is the one sync step an untrusted project may have: a +/// plugin read from a file in the project. It runs sandboxed either way; it +/// is the download that is refused. +pub fn sync_step_allowed(step: &SyncStep) -> bool { + matches!(step, SyncStep::Plugin(adapter) if matches!(adapter.source, SourceField::Local(_))) } -/// Refuse a loaded project that asks for more than an untrusted one may. +/// Hold a loaded project's steps to [`build_step_allowed`] and +/// [`sync_step_allowed`], naming the first that is neither. /// -/// Looks at every canister's build and sync steps. What the loader has -/// already read or fetched — argument files, recipe templates — is not here -/// to look at, and is held by the seams it was read through instead. +/// What the loader has already read or fetched — argument files, recipe +/// templates — is not here to look at, and is held by the seams it was read +/// through instead. pub fn vet(project: &Project) -> Result<(), Refused> { for (_, (_, canister)) in &project.canisters { - let name = &canister.name; - for step in &canister.build.steps { - match step { - BuildStep::Script(_) => { - return ScriptBuildStepSnafu { canister: name }.fail(); - } - BuildStep::Prebuilt(adapter) => { - if let SourceField::Remote(remote) = &adapter.source { - return RemoteModuleSnafu { - canister: name, - url: &remote.url, - } - .fail(); - } - } + if let Some(step) = canister + .build + .steps + .iter() + .find(|step| !build_step_allowed(step)) + { + return BuildStepSnafu { + canister: &canister.name, + step: one_line(step), } + .fail(); } - for step in &canister.sync.steps { - match step { - SyncStep::Script(_) => { - return ScriptSyncStepSnafu { canister: name }.fail(); - } - SyncStep::Plugin(adapter) => { - if let SourceField::Remote(remote) = &adapter.source { - return RemotePluginSnafu { - canister: name, - url: &remote.url, - } - .fail(); - } - } + if let Some(step) = canister + .sync + .steps + .iter() + .find(|step| !sync_step_allowed(step)) + { + return SyncStepSnafu { + canister: &canister.name, + step: one_line(step), } + .fail(); } } Ok(()) } +/// A step as it displays, on one line: a step displays its kind and then +/// its fields or commands one per line, and an error message has room for +/// one, so the lines after the first are run together with `;` between. +fn one_line(step: &impl std::fmt::Display) -> String { + let text = step.to_string(); + let mut lines = text.lines().map(str::trim).filter(|line| !line.is_empty()); + let kind = lines.next().unwrap_or_default(); + let rest = lines.collect::>().join("; "); + if rest.is_empty() { + kind.to_owned() + } else { + format!("{kind} {rest}") + } +} + /// The [`Resolve`] that fetches nothing. /// /// A recipe is resolved while the project loads, so a project that names one @@ -218,25 +235,27 @@ mod tests { vet(&project).expect("pre-built from the project, plugin from the project"); } + /// A script step and a remote module are the two other build steps there + /// are. Neither is on the list, and the message says what the list is + /// and what the step was. #[tokio::test] - async fn a_script_step_is_refused() { + async fn a_build_step_that_is_not_a_local_prebuilt_module_is_refused() { let err = vet(&project_with(vec![BuildStep::Script(script("make"))], vec![]).await).unwrap_err(); - assert!(matches!(err, Refused::ScriptBuildStep { ref canister } if canister == "backend")); - assert!(err.to_string().contains("script build step"), "{err}"); + assert!(matches!(err, Refused::BuildStep { ref canister, .. } if canister == "backend")); + let text = err.to_string(); + assert!( + text.contains("not a pre-built module from a file in the project"), + "{text}" + ); + assert!(text.contains("(script) make"), "{text}"); - let err = vet(&project_with( - vec![local("backend.wasm")], - vec![SyncStep::Script(script("upload"))], - ) - .await) - .unwrap_err(); - assert!(matches!(err, Refused::ScriptSyncStep { .. })); - assert!(err.to_string().contains("script sync step"), "{err}"); - } + let several = script::Adapter { + command: script::CommandField::Commands(vec!["make".into(), "cp a b".into()]), + }; + let err = vet(&project_with(vec![BuildStep::Script(several)], vec![]).await).unwrap_err(); + assert!(err.to_string().contains("(script) make; cp a b"), "{err}"); - #[tokio::test] - async fn a_remote_module_or_plugin_is_refused() { let remote = BuildStep::Prebuilt(prebuilt::Adapter { source: SourceField::Remote(prebuilt::RemoteSource { url: "https://example.com/backend.wasm".into(), @@ -244,19 +263,45 @@ mod tests { sha256: Some("abc".into()), }); let err = vet(&project_with(vec![remote], vec![]).await).unwrap_err(); - assert!(matches!(err, Refused::RemoteModule { .. })); + assert!(matches!(err, Refused::BuildStep { .. })); assert!( err.to_string().contains("example.com/backend.wasm"), "{err}" ); + // One good step does not carry a bad one. + let err = vet(&project_with( + vec![local("backend.wasm"), BuildStep::Script(script("make"))], + vec![], + ) + .await) + .unwrap_err(); + assert!(matches!(err, Refused::BuildStep { .. })); + } + + #[tokio::test] + async fn a_sync_step_that_is_not_a_local_plugin_is_refused() { + let err = vet(&project_with( + vec![local("backend.wasm")], + vec![SyncStep::Script(script("upload"))], + ) + .await) + .unwrap_err(); + assert!(matches!(err, Refused::SyncStep { .. })); + let text = err.to_string(); + assert!( + text.contains("not a plugin from a file in the project"), + "{text}" + ); + assert!(text.contains("script"), "{text}"); + let err = vet(&project_with( vec![local("backend.wasm")], vec![plugin("url: https://example.com/plugin.wasm")], ) .await) .unwrap_err(); - assert!(matches!(err, Refused::RemotePlugin { .. })); + assert!(matches!(err, Refused::SyncStep { .. })); assert!(err.to_string().contains("example.com/plugin.wasm"), "{err}"); } From a2248db1e8165f0dabd9449763c2883479270434 Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Thu, 5 Nov 2026 23:59:18 +0000 Subject: [PATCH 03/10] fix(untrusted): read every module through the confined filesystem, and anchor a relative root Two gaps a review turned up in what `--untrusted` holds a project to. The module a pre-built step or a plugin step names was fetched by the CLI's shared fetcher, which reads a local source straight off the host: its checksum was taken of a file outside the root or through a symlink, and for a plugin the unchecked path went on to Wasmtime, which opens it for itself. `untrusted::LocalModules` is the `wasm::Fetch` of an untrusted project: every module is read through the confined filesystem first, so the refusal happens before any checksum and before the path goes anywhere, and its message is the filesystem's own; a module named by URL is not fetched. A relative `--project-root-override` or `ICP_PROJECT_ROOT`, such as `bundle`, came back from the locator as spelled, and the confined filesystem then held `bundle/icp.yaml` to `bundle/bundle/icp.yaml`, so a valid project failed to load. The override is now joined to the working directory before the locator is built, spelled as the user spelled it, with nothing canonicalized. Co-Authored-By: Claude Fable 5.1 --- crates/icp-app/src/context/init.rs | 51 ++++-- crates/icp-cli/tests/untrusted_tests.rs | 109 ++++++++++++ crates/icp-project/src/untrusted.rs | 222 +++++++++++++++++++++++- 3 files changed, 359 insertions(+), 23 deletions(-) diff --git a/crates/icp-app/src/context/init.rs b/crates/icp-app/src/context/init.rs index ea13813fa..9e779f88c 100644 --- a/crates/icp-app/src/context/init.rs +++ b/crates/icp-app/src/context/init.rs @@ -14,7 +14,7 @@ use std::time::Duration; use crate::{agent, identity, identity::PasswordFunc}; use icp_project::{ Lazy, ProjectLoad, ProjectLoadImpl, - canister::recipe::Resolve, + canister::{recipe::Resolve, wasm}, files::{Confined, FileSystem, HostFileSystem}, host::Host, manifest::{self, ProjectRootLocate as _}, @@ -52,10 +52,11 @@ pub enum ContextInitError { /// `untrusted` treats the project as one someone else handed over: it is /// loaded through a filesystem confined to its root that refuses symlinks, no -/// recipe is fetched, and a project with a script step or a remote module or -/// plugin is refused before any command acts on it. See -/// [`icp_project::untrusted`]. The root is located up front, since the -/// filesystem is held to it from the start, so there has to be a project. +/// recipe is fetched, every module is read from the project through that +/// filesystem, and a project with a script step or a remote module or plugin +/// is refused before any command acts on it. See [`icp_project::untrusted`]. +/// The root is located up front, since the filesystem is held to it from the +/// start, so there has to be a project. pub fn initialize( project_root_override: Option, debug: bool, @@ -94,6 +95,12 @@ pub fn initialize( let cwd: PathBuf = PathBuf::try_from(current_dir().context(CwdSnafu)?).context(Utf8PathSnafu)?; + // A relative override is taken from that working directory, spelled as + // the user spelled it. Everything below joins paths to the root the + // locator hands back, and the filesystem an untrusted project is confined + // to holds them to that root by its spelling, so the root has to be a + // whole path before anything is built on it. + let project_root_override = project_root_override.map(|dir| cwd.join(dir)); let project_root_locate = Arc::new(manifest::ProjectRootLocateImpl::new( cwd, project_root_override, @@ -113,11 +120,26 @@ pub fn initialize( // holder. let pkg_cache = Arc::new(dirs.package_cache().context(PackageCacheSnafu)?); - // Wasm modules named by a manifest but not contained in the project - let wasm = Arc::new(crate::wasm::Fetcher::new( - http_client.clone(), - pkg_cache.clone(), - )); + // The project's files come from this machine: the whole of it for the + // user's own project, the project directory alone for an untrusted one. + let files: Arc = if untrusted { + let root = project_root_locate.locate().context(UntrustedRootSnafu)?; + Arc::new(Confined::refusing_links(&root)) + } else { + Arc::new(HostFileSystem) + }; + + // Wasm modules named by a manifest: fetched and cached when not contained + // in the project, unless the project is untrusted, in which case each is + // read from the project through the filesystem confined to it. + let wasm: Arc = if untrusted { + Arc::new(untrusted::LocalModules::new(files.clone())) + } else { + Arc::new(crate::wasm::Fetcher::new( + http_client.clone(), + pkg_cache.clone(), + )) + }; // Recipes: fetched from wherever the manifest points, unless the project // is untrusted, in which case none is. @@ -130,15 +152,6 @@ pub fn initialize( }) }; - // The project's files come from this machine: the whole of it for the - // user's own project, the project directory alone for an untrusted one. - let files: Arc = if untrusted { - let root = project_root_locate.locate().context(UntrustedRootSnafu)?; - Arc::new(Confined::refusing_links(&root)) - } else { - Arc::new(HostFileSystem) - }; - // Canister builder let builder = Arc::new(Builder::new(wasm.clone(), files.clone())); diff --git a/crates/icp-cli/tests/untrusted_tests.rs b/crates/icp-cli/tests/untrusted_tests.rs index 462b02d48..fa7d8e4a4 100644 --- a/crates/icp-cli/tests/untrusted_tests.rs +++ b/crates/icp-cli/tests/untrusted_tests.rs @@ -186,6 +186,115 @@ fn a_symlink_under_the_project_is_refused_as_untrusted() { ); } +/// The module a step names goes through the confined filesystem too, before +/// its path is handed to whatever opens it, so a module outside the project +/// is refused as its own path and not merely once the build tries to copy it. +#[test] +fn a_module_outside_the_project_is_refused_as_untrusted() { + let ctx = TestContext::new(); + let project_dir = project( + &ctx, + indoc! {r#" + canisters: + - name: backend + build: + steps: + - type: pre-built + path: ../outside.wasm + "#}, + ); + std::fs::copy( + ctx.make_asset("example_icp_mo.wasm"), + project_dir.parent().unwrap().join("outside.wasm"), + ) + .expect("copy wasm outside"); + + ctx.icp() + .current_dir(&project_dir) + .args(["build"]) + .assert() + .success(); + + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .failure() + .stderr(contains("outside.wasm").and(contains("is outside"))); +} + +#[cfg(unix)] +#[test] +fn a_module_through_a_symlink_is_refused_as_untrusted() { + let ctx = TestContext::new(); + let project_dir = project( + &ctx, + indoc! {r#" + canisters: + - name: backend + build: + steps: + - type: pre-built + path: linked.wasm + "#}, + ); + std::os::unix::fs::symlink( + project_dir.join("backend.wasm"), + project_dir.join("linked.wasm"), + ) + .expect("symlink"); + + ctx.icp() + .current_dir(&project_dir) + .args(["build"]) + .assert() + .success(); + + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .failure() + .stderr( + contains("linked.wasm") + .and(contains("passes through")) + .and(contains("symlink")), + ); +} + +/// A relative project root is taken from the working directory. The confined +/// filesystem holds paths to the root by its spelling, so a root spelled +/// `icp` would hold `icp/icp.yaml` to `icp/icp/icp.yaml` if it were not. +#[test] +fn a_relative_project_root_is_taken_from_the_working_directory() { + let ctx = TestContext::new(); + let project_dir = project( + &ctx, + indoc! {r#" + canisters: + - name: backend + build: + steps: + - type: pre-built + path: backend.wasm + "#}, + ); + let parent = project_dir.parent().unwrap(); + + ctx.icp() + .current_dir(parent) + .args(["build", "--untrusted", "--project-root-override", "icp"]) + .assert() + .success(); + + ctx.icp() + .current_dir(parent) + .env("ICP_PROJECT_ROOT", "icp") + .args(["build", "--untrusted"]) + .assert() + .success(); +} + #[test] fn untrusted_needs_a_project_to_hold_to() { let ctx = TestContext::new(); diff --git a/crates/icp-project/src/untrusted.rs b/crates/icp-project/src/untrusted.rs index db821e42e..9fcf71931 100644 --- a/crates/icp-project/src/untrusted.rs +++ b/crates/icp-project/src/untrusted.rs @@ -26,8 +26,11 @@ //! asks it for the template, and [`NoRecipes`] says no. The steps can be seen //! in the loaded project, and [`vet`] holds them to the list there. //! [`Vetted`] wraps a loader so that every load is vetted before the project -//! is handed out, and a consumer composing its own host holds the three -//! together: +//! is handed out. The module a step names is the business of the +//! [`wasm::Fetch`] seam, since a step hands the path it gets back to whatever +//! opens the module — the plugin runtime opens it for itself — so +//! [`LocalModules`] reads each one through the confined filesystem first. A +//! consumer composing its own host holds the four together: //! //! ```ignore //! let files = Arc::new(Confined::refusing_links(&root)); @@ -36,18 +39,27 @@ //! recipe: Arc::new(NoRecipes), //! files: files.clone(), //! }); +//! let wasm = LocalModules::new(files.clone()); //! ``` //! //! A checksum on a module is not asked for. It is the bundler's promise about //! its own archive, and the same untrusted hand wrote both, so it says nothing //! about what the project can reach. +use std::sync::Arc; + use async_trait::async_trait; -use snafu::{ResultExt, Snafu}; +use icp_events::StepReporter; +use sha2::{Digest, Sha256}; +use snafu::{ResultExt, Snafu, ensure}; use crate::{ Project, ProjectLoad, ProjectLoadError, UntrustedSnafu, - canister::recipe::{Fetched, Resolve, ResolveError}, + canister::{ + recipe::{Fetched, Resolve, ResolveError}, + wasm, + }, + files::{FileSystem, FsError}, manifest::{ adapter::prebuilt::SourceField, canister::{BuildStep, SyncStep}, @@ -165,6 +177,96 @@ impl Resolve for NoRecipes { } } +/// The [`wasm::Fetch`] of an untrusted project: every module is a file in it. +/// +/// A module is read through the project's filesystem — the [`Confined`] that +/// refuses links — so one outside the root, or reached through a symlink, is +/// refused here: before a checksum is taken of it, and before its path goes +/// back to a step that hands it to something that opens the file for itself, +/// as the plugin runtime does. A module named by URL is not fetched. [`vet`] +/// has already refused the step that names one; this is the seam saying the +/// same to a caller that never loaded the project. +/// +/// [`Confined`]: crate::files::Confined +pub struct LocalModules { + files: Arc, +} + +impl LocalModules { + pub fn new(files: Arc) -> Self { + Self { files } + } + + async fn module( + &self, + source: &SourceField, + base_dir: &Path, + sha256: Option<&str>, + reporter: &StepReporter, + ) -> Result { + let local = match source { + SourceField::Local(local) => local, + SourceField::Remote(remote) => return RemoteSnafu { url: &remote.url }.fail(), + }; + let path = base_dir.join(&local.path); + reporter.info(format!("Reading wasm: {}", local.path)); + let bytes = self.files.read(&path).await?; + if let Some(expected) = sha256 { + reporter.info("Verifying checksum"); + let actual = hex::encode(Sha256::digest(&bytes)); + ensure!( + actual == expected, + ChecksumSnafu { + path, + expected, + actual, + } + ); + } + Ok(path) + } +} + +/// A module an untrusted project names that it is not given. +#[derive(Debug, Snafu)] +pub enum ModuleRefused { + #[snafu(display( + "module '{url}' is not fetched: an untrusted project's modules are files in it, as a \ + bundle's are" + ))] + Remote { url: String }, + + /// The filesystem the project is confined to would not read it: it lies + /// outside the root, passes through a link, or is not there. Its message + /// names the path and says which, so it is passed through as it is. + #[snafu(transparent)] + ReadModule { source: FsError }, + + #[snafu(display( + "module '{path}' is not the one the manifest names: its sha256 is {actual}, not {expected}" + ))] + Checksum { + path: PathBuf, + expected: String, + actual: String, + }, +} + +#[async_trait] +impl wasm::Fetch for LocalModules { + async fn wasm( + &self, + source: &SourceField, + base_dir: &Path, + sha256: Option<&str>, + reporter: &StepReporter, + ) -> Result { + self.module(source, base_dir, sha256, reporter) + .await + .map_err(wasm::FetchError::new) + } +} + /// A [`ProjectLoad`] that hands out only a project [`vet`] accepts. pub struct Vetted(L); @@ -336,4 +438,116 @@ mod tests { .await .expect("the minimal project is pre-built"); } + + /// The modules go through a [`Confined`](crate::files::Confined) that + /// refuses links, which is a host filesystem, so this is held to the + /// `host` feature as that is. + #[cfg(feature = "host")] + mod modules { + use super::*; + use crate::canister::wasm::Fetch as _; + use crate::files::Confined; + use crate::manifest::adapter::prebuilt::{LocalSource, RemoteSource}; + + /// A project holding `backend.wasm`, beside a file it must not + /// reach, and on unix a link in the project to that file. + fn project() -> (camino_tempfile::Utf8TempDir, LocalModules, PathBuf) { + let dir = camino_tempfile::tempdir().unwrap(); + let root = dir.path().join("project"); + std::fs::create_dir(&root).unwrap(); + std::fs::write(root.join("backend.wasm"), b"module").unwrap(); + std::fs::write(dir.path().join("outside.wasm"), b"secret").unwrap(); + #[cfg(unix)] + std::os::unix::fs::symlink(dir.path().join("outside.wasm"), root.join("link.wasm")) + .unwrap(); + let files = Arc::new(Confined::refusing_links(&root)); + (dir, LocalModules::new(files), root) + } + + async fn fetch( + modules: &LocalModules, + base_dir: &Path, + source: SourceField, + sha256: Option<&str>, + ) -> Result { + modules + .wasm(&source, base_dir, sha256, &StepReporter::null()) + .await + .map_err(|err| err.to_string()) + } + + fn local(path: &str) -> SourceField { + SourceField::Local(LocalSource { path: path.into() }) + } + + #[tokio::test] + async fn a_module_in_the_project_is_read_and_its_checksum_checked() { + let (_dir, modules, root) = project(); + let sha = hex::encode(Sha256::digest(b"module")); + + let path = fetch(&modules, &root, local("backend.wasm"), None) + .await + .expect("no checksum to check"); + assert_eq!(path, root.join("backend.wasm")); + fetch(&modules, &root, local("backend.wasm"), Some(&sha)) + .await + .expect("the right checksum"); + + let err = fetch(&modules, &root, local("backend.wasm"), Some("00")) + .await + .unwrap_err(); + assert!(err.contains("backend.wasm"), "{err}"); + assert!(err.contains(&sha), "{err}"); + assert!(err.contains("not 00"), "{err}"); + } + + /// Refused on the read, before any checksum is taken and before the + /// path is returned to be opened by something else. + #[tokio::test] + async fn a_module_outside_the_project_is_refused() { + let (dir, modules, root) = project(); + for source in [ + local("../outside.wasm"), + local(dir.path().join("outside.wasm").as_str()), + ] { + let err = fetch(&modules, &root, source, None).await.unwrap_err(); + assert!(err.contains("is outside"), "{err}"); + assert!(err.contains("outside.wasm"), "{err}"); + } + // Nor does naming it from a canister directory below the root help. + let err = fetch( + &modules, + &root.join("src"), + local("../../outside.wasm"), + None, + ) + .await + .unwrap_err(); + assert!(err.contains("outside.wasm"), "{err}"); + } + + #[cfg(unix)] + #[tokio::test] + async fn a_module_through_a_link_is_refused() { + let (_dir, modules, root) = project(); + let err = fetch(&modules, &root, local("link.wasm"), None) + .await + .unwrap_err(); + assert!(err.contains("passes through"), "{err}"); + assert!(err.contains("link.wasm"), "{err}"); + } + + #[tokio::test] + async fn a_module_named_by_url_is_not_fetched() { + let (_dir, modules, root) = project(); + let remote = SourceField::Remote(RemoteSource { + url: "https://example.com/backend.wasm".into(), + }); + let err = fetch(&modules, &root, remote, Some("abc")) + .await + .unwrap_err(); + assert!(err.contains("is not fetched"), "{err}"); + assert!(err.contains("example.com/backend.wasm"), "{err}"); + } + } } From 6d944b7094903c8a03ea077d8ae8cd97d9f5f502 Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Fri, 6 Nov 2026 14:38:04 +0000 Subject: [PATCH 04/10] refactor(cli): put --untrusted on build, deploy and sync alone A deploy service handed a project by an agent is what asks for the flag, and those are the commands it runs; nothing else acts on what a project says. The flag is read off the command before the context is composed, since it is the context that applies it. On every other command it is now an unexpected argument. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 2 +- crates/icp-cli/src/commands/build.rs | 5 ++++- crates/icp-cli/src/commands/deploy.rs | 5 ++++- crates/icp-cli/src/commands/sync.rs | 5 ++++- crates/icp-cli/src/main.rs | 24 ++++++++++++++++-------- crates/icp-cli/src/options.rs | 12 ++++++++++++ crates/icp-cli/tests/untrusted_tests.rs | 24 ++++++++++++++++++------ docs/reference/cli.md | 6 +++--- 8 files changed, 62 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b05eb685f..1afea4bb1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ air-gapped signing, extensions # Unreleased -* feat: `--untrusted`, a global flag, treats the project as one someone else handed over rather than the user's own: a bundle an agent produced, a checkout nobody has read. The project is loaded through a filesystem held to the project root that refuses a symlink anywhere under it, so a manifest can name no file outside the project and no link to one; no recipe is fetched; and a project with a script build or sync step, or a module or sync plugin fetched from a URL, is refused before any command acts on it. What is left is the shape `icp project bundle` writes. Without the flag nothing changes: the user's own project may still run scripts, use recipes, and link or reach wherever it likes. +* feat: `--untrusted`, on `build`, `deploy` and `sync`, treats the project as one someone else handed over rather than the user's own: a bundle an agent produced, a checkout nobody has read. The project is loaded through a filesystem held to the project root that refuses a symlink anywhere under it, so a manifest can name no file outside the project and no link to one; no recipe is fetched; and a project with a script build or sync step, or a module or sync plugin fetched from a URL, is refused before the command acts on it. What is left is the shape `icp project bundle` writes. Without the flag nothing changes: the user's own project may still run scripts, use recipes, and link or reach wherever it likes. * chore: macOS on Intel (`x86_64-apple-darwin`) is no longer a release target. Releases ship no Intel macOS archive, the shell installer refuses that platform, and npm no longer publishes `@icp-sdk/icp-cli-darwin-x64`, so installing `@icp-sdk/icp-cli` on an Intel Mac warns of an unsupported platform. Homebrew dropped Intel macOS in September 2026, and GitHub is retiring its Intel macOS runners. v1.8.0 is the last release with an Intel macOS build. ## Experimental diff --git a/crates/icp-cli/src/commands/build.rs b/crates/icp-cli/src/commands/build.rs index e6dcfc876..85b226709 100644 --- a/crates/icp-cli/src/commands/build.rs +++ b/crates/icp-cli/src/commands/build.rs @@ -9,7 +9,7 @@ use tracing::info; use icp_project::operations::build::build_many; use crate::{ - options::{EnvironmentOpt, arg_struct_change_help}, + options::{EnvironmentOpt, UntrustedOpt, arg_struct_change_help}, render::rendered, }; @@ -22,6 +22,9 @@ pub(crate) struct BuildArgs { #[command(flatten)] pub(crate) environment: BuildEnvironmentOpt, + + #[command(flatten)] + pub(crate) untrusted: UntrustedOpt, } arg_struct_change_help!( diff --git a/crates/icp-cli/src/commands/deploy.rs b/crates/icp-cli/src/commands/deploy.rs index 781814947..8e6e6377e 100644 --- a/crates/icp-cli/src/commands/deploy.rs +++ b/crates/icp-cli/src/commands/deploy.rs @@ -20,7 +20,7 @@ use tracing::info; use crate::options::EnvironmentOpt; use crate::{ commands::{args::ArgsOpt, canister::create}, - options::{IdentityOpt, arg_struct_change_help}, + options::{IdentityOpt, UntrustedOpt, arg_struct_change_help}, render::rendered, }; @@ -97,6 +97,9 @@ pub(crate) struct DeployArgs { /// `upgrade_args` in the manifest. #[command(flatten)] pub(crate) args_opt: ArgsOpt, + + #[command(flatten)] + pub(crate) untrusted: UntrustedOpt, } arg_struct_change_help!( diff --git a/crates/icp-cli/src/commands/sync.rs b/crates/icp-cli/src/commands/sync.rs index a82eba23a..fcb28e048 100644 --- a/crates/icp-cli/src/commands/sync.rs +++ b/crates/icp-cli/src/commands/sync.rs @@ -12,7 +12,7 @@ use tracing::info; use icp_project::operations::{readiness::wait_until_serving_queries, sync::sync_many}; use crate::{ - options::{EnvironmentOpt, IdentityOpt}, + options::{EnvironmentOpt, IdentityOpt, UntrustedOpt}, render::rendered, }; @@ -32,6 +32,9 @@ pub(crate) struct SyncArgs { #[command(flatten)] pub(crate) identity: IdentityOpt, + + #[command(flatten)] + pub(crate) untrusted: UntrustedOpt, } pub(crate) async fn exec(ctx: &Context, args: &SyncArgs) -> Result<(), anyhow::Error> { diff --git a/crates/icp-cli/src/main.rs b/crates/icp-cli/src/main.rs index 0a0f1342c..c449b1f1a 100644 --- a/crates/icp-cli/src/main.rs +++ b/crates/icp-cli/src/main.rs @@ -88,12 +88,6 @@ struct Cli { #[arg(long, global = true, value_name = "FILE", help_heading = heading::GLOBAL_PARAMETERS, value_hint = ValueHint::FilePath)] identity_password_file: Option, - /// Treat the project as one someone else handed over: refuse a script - /// build or sync step, a recipe, a module or plugin fetched from a URL, - /// a path outside the project root, and a symlink under it - #[arg(long, default_value = "false", global = true, help_heading = heading::GLOBAL_PARAMETERS)] - untrusted: bool, - /// Generate markdown documentation for all commands and exit #[arg(long, hide = true)] markdown_help: bool, @@ -141,7 +135,6 @@ async fn run() -> Result<(), Error> { identity_password_file: matches .get_one::("identity_password_file") .cloned(), - untrusted: matches.get_flag("untrusted"), markdown_help: false, command: None, }, @@ -221,7 +214,7 @@ async fn run() -> Result<(), Error> { cli.debug, password_func, pem_session_duration, - cli.untrusted, + command.untrusted(), )?; // Extension commands are left out of telemetry: telemetry parses against @@ -274,6 +267,21 @@ enum Invocation { Extension(icp_app::extension::Installed, ArgMatches), } +impl Invocation { + /// Whether the command was asked to treat the project as untrusted. The + /// flag belongs to the commands that act on what a project says — build, + /// deploy, sync — but it is the context every command runs in that + /// applies it, so it is read off the command before that is composed. + fn untrusted(&self) -> bool { + match self { + Invocation::BuiltIn(Command::Build(args)) => args.untrusted.untrusted, + Invocation::BuiltIn(Command::Deploy(args)) => args.untrusted.untrusted, + Invocation::BuiltIn(Command::Sync(args)) => args.untrusted.untrusted, + Invocation::BuiltIn(_) | Invocation::Extension(..) => false, + } + } +} + /// Dispatch the command to its handler. async fn dispatch(ctx: &icp_app::context::Context, command: Command) -> Result<(), Error> { match command { diff --git a/crates/icp-cli/src/options.rs b/crates/icp-cli/src/options.rs index 7a3bd4446..a1bb844de 100644 --- a/crates/icp-cli/src/options.rs +++ b/crates/icp-cli/src/options.rs @@ -40,6 +40,18 @@ impl From for IdentitySelection { } } +/// `--untrusted`, for a command that acts on what a project says: build, +/// deploy, sync. A deploy service handed a project by an agent is what asks +/// for it, and those are the commands it runs. +#[derive(Args, Clone, Debug, Default)] +pub(crate) struct UntrustedOpt { + /// Treat the project as one someone else handed over: refuse a script + /// build or sync step, a recipe, a module or plugin fetched from a URL, + /// a path outside the project root, and a symlink under it + #[arg(long)] + pub(crate) untrusted: bool, +} + #[derive(Args, Clone, Debug, Default)] #[clap(group(ArgGroup::new("environment-select").multiple(false)))] pub(crate) struct EnvironmentOpt { diff --git a/crates/icp-cli/tests/untrusted_tests.rs b/crates/icp-cli/tests/untrusted_tests.rs index fa7d8e4a4..b9817c158 100644 --- a/crates/icp-cli/tests/untrusted_tests.rs +++ b/crates/icp-cli/tests/untrusted_tests.rs @@ -303,7 +303,7 @@ fn untrusted_needs_a_project_to_hold_to() { ctx.icp() .current_dir(&empty) - .args(["identity", "list", "--untrusted"]) + .args(["build", "--untrusted"]) .assert() .failure() .stderr(contains( @@ -311,8 +311,10 @@ fn untrusted_needs_a_project_to_hold_to() { )); } +/// The flag is on the three commands that act on what a project says, and +/// each refuses before it reaches any network. It is on no other command. #[test] -fn the_flag_is_global() { +fn the_flag_is_on_build_deploy_and_sync_alone() { let ctx = TestContext::new(); let project_dir = project( &ctx, @@ -321,14 +323,24 @@ fn the_flag_is_global() { - name: backend build: steps: - - type: pre-built - path: backend.wasm + - type: script + command: cp backend.wasm "$ICP_WASM_OUTPUT_PATH" "#}, ); + for command in ["build", "deploy", "sync"] { + ctx.icp() + .current_dir(&project_dir) + .args([command, "--untrusted"]) + .assert() + .failure() + .stderr(contains("more than an untrusted project may")); + } + ctx.icp() .current_dir(&project_dir) - .args(["--untrusted", "build", "backend"]) + .args(["identity", "list", "--untrusted"]) .assert() - .success(); + .failure() + .stderr(contains("unexpected argument '--untrusted'")); } diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 89243e29e..14ba613fd 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -120,9 +120,6 @@ This document contains the help content for the `icp` command-line program. Default value: `false` * `--identity-password-file ` — Read identity password from a file instead of prompting -* `--untrusted` — Treat the project as one someone else handed over: refuse a script build or sync step, a recipe, a module or plugin fetched from a URL, a path outside the project root, and a symlink under it - - Default value: `false` @@ -139,6 +136,7 @@ Build canisters ###### **Options:** * `-e`, `--environment ` — Override the environment to build for. By default, the local environment is used +* `--untrusted` — Treat the project as one someone else handed over: refuse a script build or sync step, a recipe, a module or plugin fetched from a URL, a path outside the project root, and a symlink under it @@ -1074,6 +1072,7 @@ using --args or --args-file: - `bin`: Raw binary (only valid for file references) +* `--untrusted` — Treat the project as one someone else handed over: refuse a script build or sync step, a recipe, a module or plugin fetched from a URL, a path outside the project root, and a symlink under it @@ -1922,6 +1921,7 @@ Synchronize canisters * `--proxy ` — Principal of a proxy canister to route sync plugin calls to the target canister through * `-e`, `--environment ` — Override the environment to connect to. By default, the local environment is used * `--identity ` — The user identity to run this command as +* `--untrusted` — Treat the project as one someone else handed over: refuse a script build or sync step, a recipe, a module or plugin fetched from a URL, a path outside the project root, and a symlink under it From 007b57ab643dea4cc3dd4a482768fb249da000ff Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Fri, 6 Nov 2026 14:45:13 +0000 Subject: [PATCH 05/10] docs(untrusted): say what an untrusted project may have, not what it may not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The vet has been a list of what is allowed since ae3367ef; the flag's help, the changelog, the context's doc comment and the symlink message still read as a list of refusals. Each now states the list — a pre-built module from a file in the project, a plugin from a file in the project, no recipe, every path a regular file or directory under the root — and that anything else is refused. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 2 +- crates/icp-app/src/context/init.rs | 15 ++++++++------- crates/icp-cli/src/options.rs | 8 +++++--- crates/icp-project/src/files.rs | 3 ++- docs/reference/cli.md | 6 +++--- 5 files changed, 19 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1afea4bb1..a4b254dd0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ air-gapped signing, extensions # Unreleased -* feat: `--untrusted`, on `build`, `deploy` and `sync`, treats the project as one someone else handed over rather than the user's own: a bundle an agent produced, a checkout nobody has read. The project is loaded through a filesystem held to the project root that refuses a symlink anywhere under it, so a manifest can name no file outside the project and no link to one; no recipe is fetched; and a project with a script build or sync step, or a module or sync plugin fetched from a URL, is refused before the command acts on it. What is left is the shape `icp project bundle` writes. Without the flag nothing changes: the user's own project may still run scripts, use recipes, and link or reach wherever it likes. +* feat: `--untrusted`, on `build`, `deploy` and `sync`, holds the project to what one someone else handed over may have — a bundle an agent produced, a checkout nobody has read. That is the shape `icp project bundle` writes, stated as what is allowed so that a kind of step added later is refused until someone decides otherwise: each build step is a pre-built module from a file in the project, each sync step is a plugin from a file in the project, no recipe is used, and every path names a regular file or directory under the project root, through no symlink. Anything else is refused before the command acts on it, naming what was found. Without the flag nothing changes: the user's own project may still run scripts, use recipes, fetch modules, and link or reach wherever it likes. * chore: macOS on Intel (`x86_64-apple-darwin`) is no longer a release target. Releases ship no Intel macOS archive, the shell installer refuses that platform, and npm no longer publishes `@icp-sdk/icp-cli-darwin-x64`, so installing `@icp-sdk/icp-cli` on an Intel Mac warns of an unsupported platform. Homebrew dropped Intel macOS in September 2026, and GitHub is retiring its Intel macOS runners. v1.8.0 is the last release with an Intel macOS build. ## Experimental diff --git a/crates/icp-app/src/context/init.rs b/crates/icp-app/src/context/init.rs index 9e779f88c..5010666cb 100644 --- a/crates/icp-app/src/context/init.rs +++ b/crates/icp-app/src/context/init.rs @@ -50,13 +50,14 @@ pub enum ContextInitError { }, } -/// `untrusted` treats the project as one someone else handed over: it is -/// loaded through a filesystem confined to its root that refuses symlinks, no -/// recipe is fetched, every module is read from the project through that -/// filesystem, and a project with a script step or a remote module or plugin -/// is refused before any command acts on it. See [`icp_project::untrusted`]. -/// The root is located up front, since the filesystem is held to it from the -/// start, so there has to be a project. +/// `untrusted` holds the project to what one someone else handed over may +/// have, the shape a bundle has — see [`icp_project::untrusted`] for the +/// list: it is loaded through a filesystem confined to its root on which +/// every path is a regular file or directory, no recipe is used, every +/// module is read from the project through that filesystem, and the loaded +/// project is vetted for the steps on the list before any command acts on +/// it. The root is located up front, since the filesystem is held to it from +/// the start, so there has to be a project. pub fn initialize( project_root_override: Option, debug: bool, diff --git a/crates/icp-cli/src/options.rs b/crates/icp-cli/src/options.rs index a1bb844de..a331e6cf6 100644 --- a/crates/icp-cli/src/options.rs +++ b/crates/icp-cli/src/options.rs @@ -45,9 +45,11 @@ impl From for IdentitySelection { /// for it, and those are the commands it runs. #[derive(Args, Clone, Debug, Default)] pub(crate) struct UntrustedOpt { - /// Treat the project as one someone else handed over: refuse a script - /// build or sync step, a recipe, a module or plugin fetched from a URL, - /// a path outside the project root, and a symlink under it + /// Hold the project to what one someone else handed over may have, which + /// is the shape `icp project bundle` writes: each build step a pre-built + /// module from a file in the project, each sync step a plugin from a file + /// in the project, no recipe, and every path a regular file or directory + /// under the project root. Anything else is refused #[arg(long)] pub(crate) untrusted: bool, } diff --git a/crates/icp-project/src/files.rs b/crates/icp-project/src/files.rs index 8a3729e95..a6b90ef3a 100644 --- a/crates/icp-project/src/files.rs +++ b/crates/icp-project/src/files.rs @@ -147,7 +147,8 @@ pub struct OutsideRoot { #[cfg(feature = "host")] #[derive(Debug, Snafu)] #[snafu(display( - "'{path}' passes through '{link}', which is a symlink, and this project may not have any" + "'{path}' passes through '{link}', which is a symlink, and every path in this project must \ + name a regular file or directory under its root" ))] pub struct ThroughLink { pub path: PathBuf, diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 14ba613fd..9a51be2e7 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -136,7 +136,7 @@ Build canisters ###### **Options:** * `-e`, `--environment ` — Override the environment to build for. By default, the local environment is used -* `--untrusted` — Treat the project as one someone else handed over: refuse a script build or sync step, a recipe, a module or plugin fetched from a URL, a path outside the project root, and a symlink under it +* `--untrusted` — Hold the project to what one someone else handed over may have, which is the shape `icp project bundle` writes: each build step a pre-built module from a file in the project, each sync step a plugin from a file in the project, no recipe, and every path a regular file or directory under the project root. Anything else is refused @@ -1072,7 +1072,7 @@ using --args or --args-file: - `bin`: Raw binary (only valid for file references) -* `--untrusted` — Treat the project as one someone else handed over: refuse a script build or sync step, a recipe, a module or plugin fetched from a URL, a path outside the project root, and a symlink under it +* `--untrusted` — Hold the project to what one someone else handed over may have, which is the shape `icp project bundle` writes: each build step a pre-built module from a file in the project, each sync step a plugin from a file in the project, no recipe, and every path a regular file or directory under the project root. Anything else is refused @@ -1921,7 +1921,7 @@ Synchronize canisters * `--proxy ` — Principal of a proxy canister to route sync plugin calls to the target canister through * `-e`, `--environment ` — Override the environment to connect to. By default, the local environment is used * `--identity ` — The user identity to run this command as -* `--untrusted` — Treat the project as one someone else handed over: refuse a script build or sync step, a recipe, a module or plugin fetched from a URL, a path outside the project root, and a symlink under it +* `--untrusted` — Hold the project to what one someone else handed over may have, which is the shape `icp project bundle` writes: each build step a pre-built module from a file in the project, each sync step a plugin from a file in the project, no recipe, and every path a regular file or directory under the project root. Anything else is refused From 4d32b8e4e12384e59285864392e78cf461202d13 Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Fri, 6 Nov 2026 15:27:36 +0000 Subject: [PATCH 06/10] refactor(untrusted): hold what a project runs, not what it reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The line is between reading and running. Reading is not restricted: the loader reads argument files, manifests and recipe templates from wherever the manifest points, through the ordinary host filesystem and fetchers, and a recipe's rendered steps are held like any other. What runs is held to a list: a build step is a pre-built module, a sync step is a plugin, and the file each names is within the project — judged by where it really is, root and file both canonicalized, so a link is seen for where it leads and a link within the project is let be. That makes the vet the whole rule, applied to the loaded project, and retires the link-refusing filesystem, the recipe refusal, the module fetch wrapper, and the up-front root locate and relative-root anchoring they needed. `Refused` is boxed on the load error, which it would otherwise push past clippy's size threshold. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 2 +- crates/icp-app/src/context/init.rs | 73 +-- crates/icp-cli/src/options.rs | 9 +- crates/icp-cli/tests/untrusted_tests.rs | 210 +++----- crates/icp-project/src/files.rs | 169 +------ crates/icp-project/src/lib.rs | 5 +- crates/icp-project/src/untrusted.rs | 630 +++++++++++------------- docs/reference/cli.md | 6 +- 8 files changed, 382 insertions(+), 722 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a4b254dd0..a76399037 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ air-gapped signing, extensions # Unreleased -* feat: `--untrusted`, on `build`, `deploy` and `sync`, holds the project to what one someone else handed over may have — a bundle an agent produced, a checkout nobody has read. That is the shape `icp project bundle` writes, stated as what is allowed so that a kind of step added later is refused until someone decides otherwise: each build step is a pre-built module from a file in the project, each sync step is a plugin from a file in the project, no recipe is used, and every path names a regular file or directory under the project root, through no symlink. Anything else is refused before the command acts on it, naming what was found. Without the flag nothing changes: the user's own project may still run scripts, use recipes, fetch modules, and link or reach wherever it likes. +* feat: `--untrusted`, on `build`, `deploy` and `sync`, holds the project to what one someone else handed over may run — a bundle an agent produced, a checkout nobody has read. The list is stated as what is allowed, so that a kind of step added later is refused until someone decides otherwise: each build step is a pre-built module, each sync step is a plugin, and the file each names is within the project, judged by where it really is once every link is followed and every `..` applied. Anything else is refused before the command acts on it, naming what was found. Reading is not restricted: argument files, manifests and recipes are read from wherever the manifest points, and the steps a recipe renders to are held to the same list. Without the flag nothing changes: the user's own project may still run scripts and take its modules from anywhere. * chore: macOS on Intel (`x86_64-apple-darwin`) is no longer a release target. Releases ship no Intel macOS archive, the shell installer refuses that platform, and npm no longer publishes `@icp-sdk/icp-cli-darwin-x64`, so installing `@icp-sdk/icp-cli` on an Intel Mac warns of an unsupported platform. Homebrew dropped Intel macOS in September 2026, and GitHub is retiring its Intel macOS runners. v1.8.0 is the last release with an Intel macOS build. ## Experimental diff --git a/crates/icp-app/src/context/init.rs b/crates/icp-app/src/context/init.rs index 5010666cb..b340dc998 100644 --- a/crates/icp-app/src/context/init.rs +++ b/crates/icp-app/src/context/init.rs @@ -13,12 +13,8 @@ use std::time::Duration; use crate::{agent, identity, identity::PasswordFunc}; use icp_project::{ - Lazy, ProjectLoad, ProjectLoadImpl, - canister::{recipe::Resolve, wasm}, - files::{Confined, FileSystem, HostFileSystem}, - host::Host, - manifest::{self, ProjectRootLocate as _}, - store_id, untrusted, + Lazy, ProjectLoad, ProjectLoadImpl, files::HostFileSystem, host::Host, manifest, store_id, + untrusted, }; #[derive(Debug, Snafu)] @@ -43,21 +39,13 @@ pub enum ContextInitError { PackageCache { source: icp_project::fs::lock::LockError, }, - - #[snafu(display("failed to locate the project to treat as untrusted"))] - UntrustedRoot { - source: manifest::ProjectRootLocateError, - }, } /// `untrusted` holds the project to what one someone else handed over may -/// have, the shape a bundle has — see [`icp_project::untrusted`] for the -/// list: it is loaded through a filesystem confined to its root on which -/// every path is a regular file or directory, no recipe is used, every -/// module is read from the project through that filesystem, and the loaded -/// project is vetted for the steps on the list before any command acts on -/// it. The root is located up front, since the filesystem is held to it from -/// the start, so there has to be a project. +/// run — see [`icp_project::untrusted`] for the list: the loaded project is +/// vetted for its steps and for where the file each names resolves, before +/// any command acts on it. Reading is not restricted, so the project is +/// loaded through the same filesystem and fetchers as the user's own. pub fn initialize( project_root_override: Option, debug: bool, @@ -96,12 +84,6 @@ pub fn initialize( let cwd: PathBuf = PathBuf::try_from(current_dir().context(CwdSnafu)?).context(Utf8PathSnafu)?; - // A relative override is taken from that working directory, spelled as - // the user spelled it. Everything below joins paths to the root the - // locator hands back, and the filesystem an untrusted project is confined - // to holds them to that root by its spelling, so the root has to be a - // whole path before anything is built on it. - let project_root_override = project_root_override.map(|dir| cwd.join(dir)); let project_root_locate = Arc::new(manifest::ProjectRootLocateImpl::new( cwd, project_root_override, @@ -121,37 +103,20 @@ pub fn initialize( // holder. let pkg_cache = Arc::new(dirs.package_cache().context(PackageCacheSnafu)?); - // The project's files come from this machine: the whole of it for the - // user's own project, the project directory alone for an untrusted one. - let files: Arc = if untrusted { - let root = project_root_locate.locate().context(UntrustedRootSnafu)?; - Arc::new(Confined::refusing_links(&root)) - } else { - Arc::new(HostFileSystem) - }; + // The project's files come from this machine + let files = Arc::new(HostFileSystem); - // Wasm modules named by a manifest: fetched and cached when not contained - // in the project, unless the project is untrusted, in which case each is - // read from the project through the filesystem confined to it. - let wasm: Arc = if untrusted { - Arc::new(untrusted::LocalModules::new(files.clone())) - } else { - Arc::new(crate::wasm::Fetcher::new( - http_client.clone(), - pkg_cache.clone(), - )) - }; + // Wasm modules named by a manifest but not contained in the project + let wasm = Arc::new(crate::wasm::Fetcher::new( + http_client.clone(), + pkg_cache.clone(), + )); - // Recipes: fetched from wherever the manifest points, unless the project - // is untrusted, in which case none is. - let recipe: Arc = if untrusted { - Arc::new(untrusted::NoRecipes) - } else { - Arc::new(RecipeFetcher { - http_client, - pkg_cache, - }) - }; + // Recipes, fetched from wherever the manifest points + let recipe = Arc::new(RecipeFetcher { + http_client, + pkg_cache, + }); // Canister builder let builder = Arc::new(Builder::new(wasm.clone(), files.clone())); @@ -170,7 +135,7 @@ pub fn initialize( }; let pload: Arc = if untrusted { - Arc::new(Lazy::new(untrusted::Vetted::new(pload))) + Arc::new(Lazy::new(untrusted::Vetted::new(pload, files.clone()))) } else { Arc::new(Lazy::new(pload)) }; diff --git a/crates/icp-cli/src/options.rs b/crates/icp-cli/src/options.rs index a331e6cf6..69b810a9e 100644 --- a/crates/icp-cli/src/options.rs +++ b/crates/icp-cli/src/options.rs @@ -45,11 +45,10 @@ impl From for IdentitySelection { /// for it, and those are the commands it runs. #[derive(Args, Clone, Debug, Default)] pub(crate) struct UntrustedOpt { - /// Hold the project to what one someone else handed over may have, which - /// is the shape `icp project bundle` writes: each build step a pre-built - /// module from a file in the project, each sync step a plugin from a file - /// in the project, no recipe, and every path a regular file or directory - /// under the project root. Anything else is refused + /// Hold the project to what one someone else handed over may run: each + /// build step a pre-built module, each sync step a plugin, and the file + /// each names within the project once links are followed. Anything else + /// is refused. Reading is not restricted #[arg(long)] pub(crate) untrusted: bool, } diff --git a/crates/icp-cli/tests/untrusted_tests.rs b/crates/icp-cli/tests/untrusted_tests.rs index b9817c158..7574a98f1 100644 --- a/crates/icp-cli/tests/untrusted_tests.rs +++ b/crates/icp-cli/tests/untrusted_tests.rs @@ -1,7 +1,7 @@ -//! `--untrusted` holds a project to what one from elsewhere may do: see +//! `--untrusted` holds a project to what one from elsewhere may run: see //! `icp_project::untrusted`. Each test here sets up a tree the CLI would act //! on as the user's own, and shows the flag refusing it, naming what it -//! refused. The one tree the flag accepts is the shape a bundle has. +//! refused — or letting it be, where the project only reads. use indoc::indoc; use predicates::{prelude::PredicateBooleanExt, str::contains}; @@ -11,32 +11,41 @@ use icp_project::fs::write_string; mod common; -/// A project directory holding a module of its own, and beside it a file the -/// project must not reach. +/// A project directory holding a module of its own, and beside it a file +/// outside the project: `outside.bin`, and `outside.wasm`, a module. fn project(ctx: &TestContext, manifest: &str) -> camino::Utf8PathBuf { let project_dir = ctx.create_project_dir("icp"); let wasm = ctx.make_asset("example_icp_mo.wasm"); + let outside = project_dir.parent().unwrap(); std::fs::copy(&wasm, project_dir.join("backend.wasm")).expect("copy wasm"); - std::fs::write(project_dir.parent().unwrap().join("outside.bin"), b"secret") - .expect("write outside"); + std::fs::copy(&wasm, outside.join("outside.wasm")).expect("copy wasm outside"); + std::fs::write(outside.join("outside.bin"), b"secret").expect("write outside"); write_string(&project_dir.join("icp.yaml"), manifest).expect("write manifest"); project_dir } +const PREBUILT: &str = indoc! {r#" + canisters: + - name: backend + build: + steps: + - type: pre-built + path: backend.wasm +"#}; + +const SCRIPT: &str = indoc! {r#" + canisters: + - name: backend + build: + steps: + - type: script + command: cp backend.wasm "$ICP_WASM_OUTPUT_PATH" +"#}; + #[test] fn a_bundle_shaped_project_builds_as_untrusted() { let ctx = TestContext::new(); - let project_dir = project( - &ctx, - indoc! {r#" - canisters: - - name: backend - build: - steps: - - type: pre-built - path: backend.wasm - "#}, - ); + let project_dir = project(&ctx, PREBUILT); ctx.icp() .current_dir(&project_dir) @@ -48,17 +57,7 @@ fn a_bundle_shaped_project_builds_as_untrusted() { #[test] fn a_script_step_is_refused_as_untrusted_but_not_otherwise() { let ctx = TestContext::new(); - let project_dir = project( - &ctx, - indoc! {r#" - canisters: - - name: backend - build: - steps: - - type: script - command: cp backend.wasm "$ICP_WASM_OUTPUT_PATH" - "#}, - ); + let project_dir = project(&ctx, SCRIPT); ctx.icp() .current_dir(&project_dir) @@ -80,8 +79,10 @@ fn a_script_step_is_refused_as_untrusted_but_not_otherwise() { ); } +/// A recipe is read and rendered like any other; it is the steps it renders +/// to that are held to the list. #[test] -fn a_recipe_is_not_fetched_as_untrusted() { +fn a_recipe_is_rendered_and_its_steps_held_to_the_list() { let ctx = TestContext::new(); let project_dir = project( &ctx, @@ -92,28 +93,36 @@ fn a_recipe_is_not_fetched_as_untrusted() { type: file://recipe.hbs "#}, ); + let recipe = project_dir.join("recipe.hbs"); + std::fs::write( - project_dir.join("recipe.hbs"), + &recipe, "build:\n steps:\n - type: pre-built\n path: backend.wasm\n", ) .expect("write recipe"); - ctx.icp() .current_dir(&project_dir) - .args(["build"]) + .args(["build", "--untrusted"]) .assert() .success(); + std::fs::write( + &recipe, + "build:\n steps:\n - type: script\n command: cp backend.wasm \"$ICP_WASM_OUTPUT_PATH\"\n", + ) + .expect("write recipe"); ctx.icp() .current_dir(&project_dir) .args(["build", "--untrusted"]) .assert() .failure() - .stderr(contains("recipe 'file://recipe.hbs' is not fetched")); + .stderr(contains("not a pre-built module").and(contains("(script) cp backend.wasm"))); } +/// Reading is not restricted: an argument file outside the project is read +/// as it would be for the user's own project. #[test] -fn a_path_outside_the_project_is_refused_as_untrusted() { +fn reading_outside_the_project_is_not_restricted() { let ctx = TestContext::new(); let project_dir = project( &ctx, @@ -130,65 +139,13 @@ fn a_path_outside_the_project_is_refused_as_untrusted() { "#}, ); - ctx.icp() - .current_dir(&project_dir) - .args(["build"]) - .assert() - .success(); - ctx.icp() .current_dir(&project_dir) .args(["build", "--untrusted"]) .assert() - .failure() - .stderr(contains("outside.bin").and(contains("is outside"))); -} - -#[cfg(unix)] -#[test] -fn a_symlink_under_the_project_is_refused_as_untrusted() { - let ctx = TestContext::new(); - let project_dir = project( - &ctx, - indoc! {r#" - canisters: - - name: backend - init_args: - path: args.bin - format: bin - build: - steps: - - type: pre-built - path: backend.wasm - "#}, - ); - std::os::unix::fs::symlink( - project_dir.parent().unwrap().join("outside.bin"), - project_dir.join("args.bin"), - ) - .expect("symlink"); - - ctx.icp() - .current_dir(&project_dir) - .args(["build"]) - .assert() .success(); - - ctx.icp() - .current_dir(&project_dir) - .args(["build", "--untrusted"]) - .assert() - .failure() - .stderr( - contains("args.bin") - .and(contains("passes through")) - .and(contains("symlink")), - ); } -/// The module a step names goes through the confined filesystem too, before -/// its path is handed to whatever opens it, so a module outside the project -/// is refused as its own path and not merely once the build tries to copy it. #[test] fn a_module_outside_the_project_is_refused_as_untrusted() { let ctx = TestContext::new(); @@ -203,11 +160,6 @@ fn a_module_outside_the_project_is_refused_as_untrusted() { path: ../outside.wasm "#}, ); - std::fs::copy( - ctx.make_asset("example_icp_mo.wasm"), - project_dir.parent().unwrap().join("outside.wasm"), - ) - .expect("copy wasm outside"); ctx.icp() .current_dir(&project_dir) @@ -220,12 +172,18 @@ fn a_module_outside_the_project_is_refused_as_untrusted() { .args(["build", "--untrusted"]) .assert() .failure() - .stderr(contains("outside.wasm").and(contains("is outside"))); + .stderr( + contains("names '") + .and(contains("outside.wasm")) + .and(contains("outside the project")), + ); } +/// A link is judged by where it leads: out of the project is refused, within +/// it is fine. #[cfg(unix)] #[test] -fn a_module_through_a_symlink_is_refused_as_untrusted() { +fn a_module_linked_out_of_the_project_is_refused_and_one_linked_within_is_not() { let ctx = TestContext::new(); let project_dir = project( &ctx, @@ -238,18 +196,15 @@ fn a_module_through_a_symlink_is_refused_as_untrusted() { path: linked.wasm "#}, ); - std::os::unix::fs::symlink( - project_dir.join("backend.wasm"), - project_dir.join("linked.wasm"), - ) - .expect("symlink"); + let link = project_dir.join("linked.wasm"); + std::os::unix::fs::symlink(project_dir.parent().unwrap().join("outside.wasm"), &link) + .expect("symlink out"); ctx.icp() .current_dir(&project_dir) .args(["build"]) .assert() .success(); - ctx.icp() .current_dir(&project_dir) .args(["build", "--untrusted"]) @@ -257,28 +212,25 @@ fn a_module_through_a_symlink_is_refused_as_untrusted() { .failure() .stderr( contains("linked.wasm") - .and(contains("passes through")) - .and(contains("symlink")), + .and(contains("resolves to")) + .and(contains("outside.wasm")) + .and(contains("outside the project")), ); + + std::fs::remove_file(&link).expect("remove link"); + std::os::unix::fs::symlink(project_dir.join("backend.wasm"), &link).expect("symlink within"); + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .success(); } -/// A relative project root is taken from the working directory. The confined -/// filesystem holds paths to the root by its spelling, so a root spelled -/// `icp` would hold `icp/icp.yaml` to `icp/icp/icp.yaml` if it were not. +/// Where the file really is decides, so the root may be spelled relatively. #[test] fn a_relative_project_root_is_taken_from_the_working_directory() { let ctx = TestContext::new(); - let project_dir = project( - &ctx, - indoc! {r#" - canisters: - - name: backend - build: - steps: - - type: pre-built - path: backend.wasm - "#}, - ); + let project_dir = project(&ctx, PREBUILT); let parent = project_dir.parent().unwrap(); ctx.icp() @@ -295,38 +247,12 @@ fn a_relative_project_root_is_taken_from_the_working_directory() { .success(); } -#[test] -fn untrusted_needs_a_project_to_hold_to() { - let ctx = TestContext::new(); - let empty = ctx.home_path().join("nowhere"); - std::fs::create_dir_all(&empty).expect("mkdir"); - - ctx.icp() - .current_dir(&empty) - .args(["build", "--untrusted"]) - .assert() - .failure() - .stderr(contains( - "failed to locate the project to treat as untrusted", - )); -} - /// The flag is on the three commands that act on what a project says, and /// each refuses before it reaches any network. It is on no other command. #[test] fn the_flag_is_on_build_deploy_and_sync_alone() { let ctx = TestContext::new(); - let project_dir = project( - &ctx, - indoc! {r#" - canisters: - - name: backend - build: - steps: - - type: script - command: cp backend.wasm "$ICP_WASM_OUTPUT_PATH" - "#}, - ); + let project_dir = project(&ctx, SCRIPT); for command in ["build", "deploy", "sync"] { ctx.icp() diff --git a/crates/icp-project/src/files.rs b/crates/icp-project/src/files.rs index a6b90ef3a..e392c5de0 100644 --- a/crates/icp-project/src/files.rs +++ b/crates/icp-project/src/files.rs @@ -104,33 +104,10 @@ pub trait Scratch: Send + Sync { /// The check is lexical, so it holds only for a tree without symlinks, which /// is what a bundle is; a link is followed as the host follows it. The root /// is matched as the caller spelled it, not as it canonicalizes. -/// -/// A tree that nobody has held to that shape — a directory on disk that -/// someone else wrote, rather than an archive unpacked by a consumer that -/// refused its links — is asked for through -/// [`refusing_links`](Self::refusing_links) instead. Each component of a -/// path below the root is then looked at as it is on disk, and a path that -/// passes through a symlink anywhere under the root is refused with -/// [`ThroughLink`], wherever the link leads: one that points inside the root -/// is as unwelcome as one that points out, since the rule is the shape of the -/// tree rather than where any one link goes. The root itself and everything -/// above it are the caller's, and are not looked at. #[cfg(feature = "host")] #[derive(Debug, Clone)] pub struct Confined { root: PathBuf, - links: Links, -} - -/// What a [`Confined`] filesystem does with a symlink under its root. -#[cfg(feature = "host")] -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum Links { - /// Follow it, as the host does. For a tree that has none, or whose links - /// are the caller's own. - Follow, - /// Refuse any path that passes through one. - Refuse, } /// A path a [`Confined`] filesystem was asked for that is not under its root. @@ -142,36 +119,12 @@ pub struct OutsideRoot { pub root: PathBuf, } -/// A path a [`Confined`] filesystem that refuses links was asked for that -/// passes through one. -#[cfg(feature = "host")] -#[derive(Debug, Snafu)] -#[snafu(display( - "'{path}' passes through '{link}', which is a symlink, and every path in this project must \ - name a regular file or directory under its root" -))] -pub struct ThroughLink { - pub path: PathBuf, - pub link: PathBuf, -} - #[cfg(feature = "host")] impl Confined { - /// Confine the host filesystem to `root`, following any symlink under it - /// as the host does. + /// Confine the host filesystem to `root`. pub fn new(root: &Path) -> Self { Self { root: root.to_path_buf(), - links: Links::Follow, - } - } - - /// Confine the host filesystem to `root`, and refuse any path that passes - /// through a symlink under it. - pub fn refusing_links(root: &Path) -> Self { - Self { - root: root.to_path_buf(), - links: Links::Refuse, } } @@ -182,10 +135,9 @@ impl Confined { /// 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 — or, for a filesystem that refuses links, if it passes - /// through one on the way there. What a caller that needs a real path - /// for something that does not read through the trait — a runtime - /// mounting a directory — asks for. + /// 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 { @@ -211,40 +163,8 @@ impl Confined { Utf8Component::RootDir | Utf8Component::Prefix(_) => return Err(outside()), } } - if self.links == Links::Refuse { - self.refuse_links(path, &resolved)?; - } Ok(resolved) } - - /// Refuse `resolved` if any component of it below the root is a symlink. - /// - /// Each component is looked at as it is on disk rather than through - /// what it points at, so a link is seen wherever it leads, and a dangling - /// one is seen too. The walk stops at the first component that is not - /// there: nothing beyond it can be a link, and whether the path exists - /// is for the operation itself to find out and report. - fn refuse_links(&self, path: &Path, resolved: &Path) -> Result<(), FsError> { - let mut at = self.root.clone(); - for component in resolved - .strip_prefix(&self.root) - .expect("resolved under the root") - .components() - { - at.push(component.as_str()); - match std::fs::symlink_metadata(&at) { - Ok(meta) if meta.file_type().is_symlink() => { - return Err(FsError::new(ThroughLink { - path: path.to_path_buf(), - link: at, - })); - } - Ok(_) => {} - Err(_) => break, - } - } - Ok(()) - } } #[cfg(feature = "host")] @@ -732,87 +652,6 @@ mod tests { assert_eq!(fs.read(Path::new("link.txt")).await.unwrap(), b"secret"); } - /// Asked to refuse links, the filesystem refuses a path through one - /// wherever it leads: out of the root, back inside it, or nowhere. - /// A link in the middle of a path counts as much as one at its end. - #[cfg(unix)] - #[tokio::test] - async fn refusing_links_refuses_a_path_through_one_wherever_it_leads() { - use std::os::unix::fs::symlink; - let (dir, _) = root(); - let project = dir.path().join("project"); - symlink(dir.path().join("outside.txt"), project.join("out.txt")).expect("symlink"); - symlink(project.join("icp.yaml"), project.join("in.yaml")).expect("symlink"); - symlink(project.join("nowhere"), project.join("dangling")).expect("symlink"); - symlink(project.join("canisters"), project.join("dir-link")).expect("symlink"); - let fs = Confined::refusing_links(&project); - - for (spelling, link) in [ - ("out.txt", "out.txt"), - ("in.yaml", "in.yaml"), - ("dangling", "dangling"), - ("dir-link/a.wasm", "dir-link"), - ("./dir-link/../dir-link/a.wasm", "dir-link"), - ] { - let err = fs - .read(Path::new(spelling)) - .await - .expect_err(spelling) - .to_string(); - assert!(err.contains("passes through"), "{spelling}: {err}"); - assert!(err.contains(link), "{spelling}: {err}"); - assert!(!fs.exists(Path::new(spelling)).await, "{spelling}"); - assert!(!fs.is_file(Path::new(spelling)).await, "{spelling}"); - assert!(!fs.is_dir(Path::new(spelling)).await, "{spelling}"); - assert!(fs.resolve(Path::new(spelling)).is_err(), "{spelling}"); - } - // Nor may a link be written through. - let err = fs - .write(Path::new("out.txt"), b"x") - .await - .unwrap_err() - .to_string(); - assert!(err.contains("passes through"), "{err}"); - assert_eq!( - std::fs::read(dir.path().join("outside.txt")).unwrap(), - b"secret" - ); - - // Regular files and directories are read as before, and a path - // that is not there yet resolves, since nothing on it is a link. - assert_eq!( - fs.read(Path::new("canisters/a.wasm")).await.unwrap(), - b"wasm" - ); - assert!(fs.is_dir(Path::new("canisters")).await); - assert!(fs.resolve(Path::new("made/later")).is_ok()); - assert_eq!( - fs.read_dir(&project).await.expect("list").len(), - 6, - "listing shows the links; reading through them is what is refused" - ); - } - - /// The root itself may be reached through a link: it is the caller's - /// spelling, and only what lies below it is held to shape. - #[cfg(unix)] - #[tokio::test] - async fn refusing_links_still_takes_the_root_as_the_caller_spells_it() { - let (dir, _) = root(); - std::os::unix::fs::symlink(dir.path().join("project"), dir.path().join("alias")) - .expect("symlink"); - let spelled = dir.path().join("alias"); - let fs = Confined::refusing_links(&spelled); - assert_eq!( - fs.read(&spelled.join("icp.yaml")).await.unwrap(), - b"canisters: []" - ); - assert_eq!( - fs.read(Path::new("canisters/a.wasm")).await.unwrap(), - b"wasm" - ); - } - #[tokio::test] async fn scratch_is_made_under_the_root() { let (_dir, fs) = root(); diff --git a/crates/icp-project/src/lib.rs b/crates/icp-project/src/lib.rs index 5c2517d7d..831620667 100644 --- a/crates/icp-project/src/lib.rs +++ b/crates/icp-project/src/lib.rs @@ -222,7 +222,10 @@ pub enum ProjectLoadError { }, #[snafu(display("the project asks for more than an untrusted project may"))] - Untrusted { source: untrusted::Refused }, + Untrusted { + #[snafu(source(from(untrusted::Refused, Box::new)))] + source: Box, + }, } #[async_trait] diff --git a/crates/icp-project/src/untrusted.rs b/crates/icp-project/src/untrusted.rs index 9fcf71931..d13a05294 100644 --- a/crates/icp-project/src/untrusted.rs +++ b/crates/icp-project/src/untrusted.rs @@ -1,69 +1,49 @@ -//! Holding a project someone else handed over to what it may do. +//! Holding a project someone else handed over to what it may run. //! //! The CLI trusts a project the way it trusts the user's own checkout: a -//! build step may run any command, a recipe may come from any file or URL, a -//! module may be downloaded from anywhere, and a manifest may name any path on -//! the machine. A project from elsewhere — an archive an agent passed to a -//! deploy service, a checkout the user has not read — gets none of that. What -//! it may do is the shape `icp project bundle` writes, since a bundle is what -//! such a project usually is, and it is said as what is allowed rather than -//! what is not, so that a kind of step added later is refused until someone -//! decides otherwise: +//! build step may run any command, a sync step any script, and either may +//! take its module or plugin from anywhere on the machine. A project from +//! elsewhere — an archive an agent passed to a deploy service, a checkout the +//! user has not read — may not. The line is between reading and running. +//! Reading is not restricted: the loader reads argument files, environment +//! files, manifests and recipe templates from wherever the manifest points, +//! and a plugin reads the files it declares, through the ordinary filesystem. +//! What runs is held to a list, stated as what is allowed so that a kind of +//! step added later is refused until someone decides otherwise: //! -//! - a build step is a pre-built module read from a file in the project -//! ([`build_step_allowed`]); -//! - a sync step is a plugin read from a file in the project -//! ([`sync_step_allowed`]); -//! - a recipe is never fetched: it is build instructions read from a file or -//! a URL at load, and a bundle has expanded every one of them; -//! - a path names something under the project root, through no symlink. +//! - a build step is a pre-built module ([`build_step_allowed`]); +//! - a sync step is a plugin ([`sync_step_allowed`]); +//! - the module or plugin a step names is a file within the project: its +//! path, with every link followed and every `..` applied, lies under the +//! project root resolved the same way ([`vet`]). //! -//! The last is the business of the [`FileSystem`](crate::files::FileSystem) -//! the project is loaded through — a [`Confined`](crate::files::Confined) -//! that refuses links — because the loader reads argument and variable files -//! into the model as it loads, before any caller could look. The recipe rule -//! is the business of the [`Resolve`] seam for the same reason: the loader -//! asks it for the template, and [`NoRecipes`] says no. The steps can be seen -//! in the loaded project, and [`vet`] holds them to the list there. -//! [`Vetted`] wraps a loader so that every load is vetted before the project -//! is handed out. The module a step names is the business of the -//! [`wasm::Fetch`] seam, since a step hands the path it gets back to whatever -//! opens the module — the plugin runtime opens it for itself — so -//! [`LocalModules`] reads each one through the confined filesystem first. A -//! consumer composing its own host holds the four together: +//! A recipe is read and rendered as the project loads, and the steps it +//! produces are on the list or not like any other. A module or plugin named +//! by URL is not on it: a download at build time is what a bundle has already +//! done. The loaded [`Project`] shows every step, so the list is applied to +//! it: [`vet`] holds a project to it, and [`Vetted`] wraps a loader so that +//! every load is vetted before the project is handed out. A consumer +//! composing its own host wraps its loader the same way: //! //! ```ignore -//! let files = Arc::new(Confined::refusing_links(&root)); -//! let project = Vetted::new(ProjectLoadImpl { -//! project_root_locate, -//! recipe: Arc::new(NoRecipes), -//! files: files.clone(), -//! }); -//! let wasm = LocalModules::new(files.clone()); +//! let project = Vetted::new(ProjectLoadImpl { project_root_locate, recipe, files: files.clone() }, files); //! ``` //! //! A checksum on a module is not asked for. It is the bundler's promise about //! its own archive, and the same untrusted hand wrote both, so it says nothing -//! about what the project can reach. +//! about what the project runs. use std::sync::Arc; use async_trait::async_trait; -use icp_events::StepReporter; -use sha2::{Digest, Sha256}; -use snafu::{ResultExt, Snafu, ensure}; +use snafu::{ResultExt, Snafu}; use crate::{ Project, ProjectLoad, ProjectLoadError, UntrustedSnafu, - canister::{ - recipe::{Fetched, Resolve, ResolveError}, - wasm, - }, - files::{FileSystem, FsError}, + files::FileSystem, manifest::{ adapter::prebuilt::SourceField, canister::{BuildStep, SyncStep}, - recipe::Recipe, }, prelude::*, }; @@ -72,65 +52,122 @@ use crate::{ #[derive(Debug, Snafu)] pub enum Refused { #[snafu(display( - "canister '{canister}' has a build step that is not a pre-built module from a file in \ - the project, which is all an untrusted project may have: {step}" + "canister '{canister}' has a build step that is not a pre-built module, which is all an \ + untrusted project may have: {step}" ))] BuildStep { canister: String, step: String }, #[snafu(display( - "canister '{canister}' has a sync step that is not a plugin from a file in the project, \ - which is all an untrusted project may have: {step}" + "canister '{canister}' has a sync step that is not a plugin, which is all an untrusted \ + project may have: {step}" ))] SyncStep { canister: String, step: String }, + + #[snafu(display( + "canister '{canister}' names '{path}', which resolves to '{resolved}', outside the \ + project at '{root}': an untrusted project runs only what is within it" + ))] + OutsidePath { + canister: String, + path: PathBuf, + resolved: PathBuf, + root: PathBuf, + }, + + #[snafu(display("canister '{canister}' names '{path}', which does not resolve to a file"))] + UnresolvedPath { canister: String, path: PathBuf }, + + #[snafu(display("the project root '{root}' does not resolve to a directory"))] + UnresolvedRoot { root: PathBuf }, } /// Whether `step` is the one build step an untrusted project may have: a -/// pre-built module read from a file in the project. A script is a host -/// subprocess, and a module fetched from a URL reaches the network and the -/// package cache on the strength of an untrusted manifest. +/// pre-built module from a file. A script is a host subprocess, and a module +/// fetched from a URL is a download on the strength of an untrusted manifest. pub fn build_step_allowed(step: &BuildStep) -> bool { matches!(step, BuildStep::Prebuilt(adapter) if matches!(adapter.source, SourceField::Local(_))) } /// Whether `step` is the one sync step an untrusted project may have: a -/// plugin read from a file in the project. It runs sandboxed either way; it -/// is the download that is refused. +/// plugin from a file. It runs sandboxed either way; it is a script and a +/// download that are not on the list. pub fn sync_step_allowed(step: &SyncStep) -> bool { matches!(step, SyncStep::Plugin(adapter) if matches!(adapter.source, SourceField::Local(_))) } -/// Hold a loaded project's steps to [`build_step_allowed`] and -/// [`sync_step_allowed`], naming the first that is neither. +/// Hold a loaded project to the list: every step [`build_step_allowed`] or +/// [`sync_step_allowed`], and the file each names within the project. /// -/// What the loader has already read or fetched — argument files, recipe -/// templates — is not here to look at, and is held by the seams it was read -/// through instead. -pub fn vet(project: &Project) -> Result<(), Refused> { - for (_, (_, canister)) in &project.canisters { - if let Some(step) = canister - .build - .steps - .iter() - .find(|step| !build_step_allowed(step)) - { - return BuildStepSnafu { - canister: &canister.name, - step: one_line(step), - } - .fail(); +/// Within is decided on the real paths: the file's and the root's, each with +/// every link followed and every `..` applied by `files`, so a link out of +/// the project is seen for where it leads and a link within it is let be. A +/// path that does not resolve is refused too, since nothing can be said +/// about where it would lead. +pub async fn vet(project: &Project, files: &dyn FileSystem) -> Result<(), Refused> { + let root = files + .canonicalize(&project.dir) + .await + .ok_or_else(|| UnresolvedRootSnafu { root: &project.dir }.build())?; + + for (_, (canister_dir, canister)) in &project.canisters { + for step in &canister.build.steps { + let local = match step { + BuildStep::Prebuilt(adapter) if build_step_allowed(step) => match &adapter.source { + SourceField::Local(local) => local, + SourceField::Remote(_) => unreachable!("allowed is local"), + }, + _ => { + return BuildStepSnafu { + canister: &canister.name, + step: one_line(step), + } + .fail(); + } + }; + within(files, &root, canister, &canister_dir.join(&local.path)).await?; + } + for step in &canister.sync.steps { + let local = match step { + SyncStep::Plugin(adapter) if sync_step_allowed(step) => match &adapter.source { + SourceField::Local(local) => local, + SourceField::Remote(_) => unreachable!("allowed is local"), + }, + _ => { + return SyncStepSnafu { + canister: &canister.name, + step: one_line(step), + } + .fail(); + } + }; + within(files, &root, canister, &canister_dir.join(&local.path)).await?; + } + } + Ok(()) +} + +/// Refuse `path` unless it resolves to somewhere under the resolved `root`. +async fn within( + files: &dyn FileSystem, + root: &Path, + canister: &crate::Canister, + path: &Path, +) -> Result<(), Refused> { + let resolved = files.canonicalize(path).await.ok_or_else(|| { + UnresolvedPathSnafu { + canister: &canister.name, + path, } - if let Some(step) = canister - .sync - .steps - .iter() - .find(|step| !sync_step_allowed(step)) - { - return SyncStepSnafu { - canister: &canister.name, - step: one_line(step), - } - .fail(); + .build() + })?; + if !resolved.starts_with(root) { + return OutsidePathSnafu { + canister: &canister.name, + path, + resolved, + root, } + .fail(); } Ok(()) } @@ -150,160 +187,115 @@ fn one_line(step: &impl std::fmt::Display) -> String { } } -/// The [`Resolve`] that fetches nothing. -/// -/// A recipe is resolved while the project loads, so a project that names one -/// fails to load rather than loading and then being refused — the one place -/// the rule can be applied before the template is read from wherever the -/// manifest points. -pub struct NoRecipes; - -/// A recipe named by a project loaded as untrusted. -#[derive(Debug, Snafu)] -#[snafu(display( - "recipe '{recipe}' is not fetched: an untrusted project must spell out its build and sync \ - steps, as a bundle does" -))] -pub struct RecipeRefused { - pub recipe: String, -} - -#[async_trait] -impl Resolve for NoRecipes { - async fn resolve(&self, recipe: &Recipe) -> Result { - Err(ResolveError::new(RecipeRefused { - recipe: recipe.recipe_type.to_string(), - })) - } -} - -/// The [`wasm::Fetch`] of an untrusted project: every module is a file in it. -/// -/// A module is read through the project's filesystem — the [`Confined`] that -/// refuses links — so one outside the root, or reached through a symlink, is -/// refused here: before a checksum is taken of it, and before its path goes -/// back to a step that hands it to something that opens the file for itself, -/// as the plugin runtime does. A module named by URL is not fetched. [`vet`] -/// has already refused the step that names one; this is the seam saying the -/// same to a caller that never loaded the project. -/// -/// [`Confined`]: crate::files::Confined -pub struct LocalModules { +/// A [`ProjectLoad`] that hands out only a project [`vet`] accepts. +pub struct Vetted { + loader: L, files: Arc, } -impl LocalModules { - pub fn new(files: Arc) -> Self { - Self { files } - } - - async fn module( - &self, - source: &SourceField, - base_dir: &Path, - sha256: Option<&str>, - reporter: &StepReporter, - ) -> Result { - let local = match source { - SourceField::Local(local) => local, - SourceField::Remote(remote) => return RemoteSnafu { url: &remote.url }.fail(), - }; - let path = base_dir.join(&local.path); - reporter.info(format!("Reading wasm: {}", local.path)); - let bytes = self.files.read(&path).await?; - if let Some(expected) = sha256 { - reporter.info("Verifying checksum"); - let actual = hex::encode(Sha256::digest(&bytes)); - ensure!( - actual == expected, - ChecksumSnafu { - path, - expected, - actual, - } - ); - } - Ok(path) - } -} - -/// A module an untrusted project names that it is not given. -#[derive(Debug, Snafu)] -pub enum ModuleRefused { - #[snafu(display( - "module '{url}' is not fetched: an untrusted project's modules are files in it, as a \ - bundle's are" - ))] - Remote { url: String }, - - /// The filesystem the project is confined to would not read it: it lies - /// outside the root, passes through a link, or is not there. Its message - /// names the path and says which, so it is passed through as it is. - #[snafu(transparent)] - ReadModule { source: FsError }, - - #[snafu(display( - "module '{path}' is not the one the manifest names: its sha256 is {actual}, not {expected}" - ))] - Checksum { - path: PathBuf, - expected: String, - actual: String, - }, -} - -#[async_trait] -impl wasm::Fetch for LocalModules { - async fn wasm( - &self, - source: &SourceField, - base_dir: &Path, - sha256: Option<&str>, - reporter: &StepReporter, - ) -> Result { - self.module(source, base_dir, sha256, reporter) - .await - .map_err(wasm::FetchError::new) - } -} - -/// A [`ProjectLoad`] that hands out only a project [`vet`] accepts. -pub struct Vetted(L); - impl Vetted { - pub fn new(loader: L) -> Self { - Self(loader) + /// `files` is what the paths are resolved through: the filesystem the + /// project is on. + pub fn new(loader: L, files: Arc) -> Self { + Self { loader, files } } } #[async_trait] impl ProjectLoad for Vetted { async fn load(&self) -> Result { - let project = self.0.load().await?; - vet(&project).context(UntrustedSnafu)?; + let project = self.loader.load().await?; + vet(&project, self.files.as_ref()) + .await + .context(UntrustedSnafu)?; Ok(project) } async fn exists(&self) -> Result { - self.0.exists().await + self.loader.exists().await } fn member_dir(&self) -> Option { - self.0.member_dir() + self.loader.member_dir() } } #[cfg(test)] mod tests { + use std::collections::HashMap; + use super::*; use crate::{ MockProjectLoader, + files::{FsError, Scratch}, manifest::{ adapter::{prebuilt, script}, canister::{BuildSteps, SyncSteps}, }, }; + /// A filesystem that resolves the paths it is told to and nothing else, + /// standing in for links and `..` on disk: the vet asks for nothing but + /// where a path really is. + struct Resolves(HashMap); + + #[async_trait] + impl FileSystem for Resolves { + async fn canonicalize(&self, path: &Path) -> Option { + self.0.get(path).cloned() + } + async fn read(&self, _: &Path) -> Result, FsError> { + unreachable!("the vet reads nothing") + } + async fn read_to_string(&self, _: &Path) -> Result { + unreachable!("the vet reads nothing") + } + async fn write(&self, _: &Path, _: &[u8]) -> Result<(), FsError> { + unreachable!("the vet writes nothing") + } + async fn create_dir_all(&self, _: &Path) -> Result<(), FsError> { + unreachable!("the vet writes nothing") + } + async fn copy(&self, _: &Path, _: &Path) -> Result<(), FsError> { + unreachable!("the vet writes nothing") + } + async fn exists(&self, _: &Path) -> bool { + unreachable!("the vet looks at nothing but where a path is") + } + async fn is_file(&self, _: &Path) -> bool { + unreachable!("the vet looks at nothing but where a path is") + } + async fn is_dir(&self, _: &Path) -> bool { + unreachable!("the vet looks at nothing but where a path is") + } + async fn read_dir(&self, _: &Path) -> Result, FsError> { + unreachable!("the vet reads nothing") + } + async fn scratch_dir(&self) -> Result, FsError> { + unreachable!("the vet writes nothing") + } + } + + /// The minimal project is at `/project`, and this is its disk: a module + /// of its own, a plugin, a link out to `/elsewhere/secret.wasm`, a link + /// within to the module, and a climb that lands outside. + fn disk() -> Arc { + Arc::new(Resolves(HashMap::from([ + ("/project".into(), "/project".into()), + ( + "/project/backend.wasm".into(), + "/project/backend.wasm".into(), + ), + ("/project/p.wasm".into(), "/project/p.wasm".into()), + ("/project/out.wasm".into(), "/elsewhere/secret.wasm".into()), + ("/project/in.wasm".into(), "/project/backend.wasm".into()), + ( + "/project/../elsewhere/secret.wasm".into(), + "/elsewhere/secret.wasm".into(), + ), + ]))) + } + async fn project_with(build: Vec, sync: Vec) -> Project { let mut project = MockProjectLoader::minimal().load().await.unwrap(); let (_, canister) = project.canisters.get_mut("backend").unwrap(); @@ -312,6 +304,10 @@ mod tests { project } + async fn vetted(build: Vec, sync: Vec) -> Result<(), Refused> { + vet(&project_with(build, sync).await, disk().as_ref()).await + } + fn local(path: &str) -> BuildStep { BuildStep::Prebuilt(prebuilt::Adapter { source: SourceField::Local(prebuilt::LocalSource { path: path.into() }), @@ -333,8 +329,9 @@ mod tests { #[tokio::test] async fn a_bundle_shaped_project_is_accepted() { - let project = project_with(vec![local("backend.wasm")], vec![plugin("path: p.wasm")]).await; - vet(&project).expect("pre-built from the project, plugin from the project"); + vetted(vec![local("backend.wasm")], vec![plugin("path: p.wasm")]) + .await + .expect("pre-built from the project, plugin from the project"); } /// A script step and a remote module are the two other build steps there @@ -342,20 +339,20 @@ mod tests { /// and what the step was. #[tokio::test] async fn a_build_step_that_is_not_a_local_prebuilt_module_is_refused() { - let err = - vet(&project_with(vec![BuildStep::Script(script("make"))], vec![]).await).unwrap_err(); + let err = vetted(vec![BuildStep::Script(script("make"))], vec![]) + .await + .unwrap_err(); assert!(matches!(err, Refused::BuildStep { ref canister, .. } if canister == "backend")); let text = err.to_string(); - assert!( - text.contains("not a pre-built module from a file in the project"), - "{text}" - ); + assert!(text.contains("not a pre-built module"), "{text}"); assert!(text.contains("(script) make"), "{text}"); let several = script::Adapter { command: script::CommandField::Commands(vec!["make".into(), "cp a b".into()]), }; - let err = vet(&project_with(vec![BuildStep::Script(several)], vec![]).await).unwrap_err(); + let err = vetted(vec![BuildStep::Script(several)], vec![]) + .await + .unwrap_err(); assert!(err.to_string().contains("(script) make; cp a b"), "{err}"); let remote = BuildStep::Prebuilt(prebuilt::Adapter { @@ -364,7 +361,7 @@ mod tests { }), sha256: Some("abc".into()), }); - let err = vet(&project_with(vec![remote], vec![]).await).unwrap_err(); + let err = vetted(vec![remote], vec![]).await.unwrap_err(); assert!(matches!(err, Refused::BuildStep { .. })); assert!( err.to_string().contains("example.com/backend.wasm"), @@ -372,182 +369,113 @@ mod tests { ); // One good step does not carry a bad one. - let err = vet(&project_with( + let err = vetted( vec![local("backend.wasm"), BuildStep::Script(script("make"))], vec![], ) - .await) + .await .unwrap_err(); assert!(matches!(err, Refused::BuildStep { .. })); } #[tokio::test] async fn a_sync_step_that_is_not_a_local_plugin_is_refused() { - let err = vet(&project_with( + let err = vetted( vec![local("backend.wasm")], vec![SyncStep::Script(script("upload"))], ) - .await) + .await .unwrap_err(); assert!(matches!(err, Refused::SyncStep { .. })); let text = err.to_string(); - assert!( - text.contains("not a plugin from a file in the project"), - "{text}" - ); + assert!(text.contains("not a plugin"), "{text}"); assert!(text.contains("script"), "{text}"); - let err = vet(&project_with( + let err = vetted( vec![local("backend.wasm")], vec![plugin("url: https://example.com/plugin.wasm")], ) - .await) + .await .unwrap_err(); assert!(matches!(err, Refused::SyncStep { .. })); assert!(err.to_string().contains("example.com/plugin.wasm"), "{err}"); } + /// Where a path leads is what counts, not how it is spelled: a link out + /// is refused for where it lands, a link within is fine, and a climb + /// out is refused. #[tokio::test] - async fn no_recipe_is_fetched_whatever_it_names() { - for spec in [ - "file://recipe.hbs", - "https://example.com/recipe.hbs", - "@dfinity/rust@v1", - ] { - let recipe: Recipe = serde_yaml::from_str(&format!("type: \"{spec}\"")).unwrap(); - let err = match NoRecipes.resolve(&recipe).await { - Ok(_) => panic!("{spec} was fetched"), - Err(err) => err.to_string(), - }; - assert!(err.contains("not fetched"), "{spec}: {err}"); - assert!(err.contains(spec), "{spec}: {err}"); + async fn the_file_a_step_names_is_held_to_where_it_resolves() { + vetted(vec![local("in.wasm")], vec![]) + .await + .expect("a link within the project"); + + for path in ["out.wasm", "../elsewhere/secret.wasm"] { + let err = vetted(vec![local(path)], vec![]).await.unwrap_err(); + assert!( + matches!(err, Refused::OutsidePath { ref resolved, ref root, .. } + if resolved == "/elsewhere/secret.wasm" && root == "/project"), + "{path}: {err}" + ); + assert!( + err.to_string().contains("outside the project"), + "{path}: {err}" + ); } + + let err = vetted(vec![local("backend.wasm")], vec![plugin("path: out.wasm")]) + .await + .unwrap_err(); + assert!(matches!(err, Refused::OutsidePath { .. }), "{err}"); + + let err = vetted(vec![local("missing.wasm")], vec![]) + .await + .unwrap_err(); + assert!(matches!(err, Refused::UnresolvedPath { .. }), "{err}"); + assert!(err.to_string().contains("missing.wasm"), "{err}"); } #[tokio::test] async fn a_vetted_loader_refuses_what_vet_refuses() { let project = project_with(vec![BuildStep::Script(script("make"))], vec![]).await; - let loader = Vetted::new(MockProjectLoader::new(project)); + let loader = Vetted::new(MockProjectLoader::new(project), disk()); assert!(loader.exists().await.unwrap()); let err = loader.load().await.unwrap_err(); assert!(matches!(err, ProjectLoadError::Untrusted { .. }), "{err}"); - let loader = Vetted::new(MockProjectLoader::minimal()); + let loader = Vetted::new(MockProjectLoader::minimal(), disk()); loader .load() .await - .expect("the minimal project is pre-built"); + .expect("the minimal project is pre-built from its own file"); } - /// The modules go through a [`Confined`](crate::files::Confined) that - /// refuses links, which is a host filesystem, so this is held to the - /// `host` feature as that is. - #[cfg(feature = "host")] - mod modules { - use super::*; - use crate::canister::wasm::Fetch as _; - use crate::files::Confined; - use crate::manifest::adapter::prebuilt::{LocalSource, RemoteSource}; - - /// A project holding `backend.wasm`, beside a file it must not - /// reach, and on unix a link in the project to that file. - fn project() -> (camino_tempfile::Utf8TempDir, LocalModules, PathBuf) { - let dir = camino_tempfile::tempdir().unwrap(); - let root = dir.path().join("project"); - std::fs::create_dir(&root).unwrap(); - std::fs::write(root.join("backend.wasm"), b"module").unwrap(); - std::fs::write(dir.path().join("outside.wasm"), b"secret").unwrap(); - #[cfg(unix)] - std::os::unix::fs::symlink(dir.path().join("outside.wasm"), root.join("link.wasm")) - .unwrap(); - let files = Arc::new(Confined::refusing_links(&root)); - (dir, LocalModules::new(files), root) - } - - async fn fetch( - modules: &LocalModules, - base_dir: &Path, - source: SourceField, - sha256: Option<&str>, - ) -> Result { - modules - .wasm(&source, base_dir, sha256, &StepReporter::null()) - .await - .map_err(|err| err.to_string()) - } - - fn local(path: &str) -> SourceField { - SourceField::Local(LocalSource { path: path.into() }) - } - - #[tokio::test] - async fn a_module_in_the_project_is_read_and_its_checksum_checked() { - let (_dir, modules, root) = project(); - let sha = hex::encode(Sha256::digest(b"module")); - - let path = fetch(&modules, &root, local("backend.wasm"), None) - .await - .expect("no checksum to check"); - assert_eq!(path, root.join("backend.wasm")); - fetch(&modules, &root, local("backend.wasm"), Some(&sha)) - .await - .expect("the right checksum"); - - let err = fetch(&modules, &root, local("backend.wasm"), Some("00")) - .await - .unwrap_err(); - assert!(err.contains("backend.wasm"), "{err}"); - assert!(err.contains(&sha), "{err}"); - assert!(err.contains("not 00"), "{err}"); - } - - /// Refused on the read, before any checksum is taken and before the - /// path is returned to be opened by something else. - #[tokio::test] - async fn a_module_outside_the_project_is_refused() { - let (dir, modules, root) = project(); - for source in [ - local("../outside.wasm"), - local(dir.path().join("outside.wasm").as_str()), - ] { - let err = fetch(&modules, &root, source, None).await.unwrap_err(); - assert!(err.contains("is outside"), "{err}"); - assert!(err.contains("outside.wasm"), "{err}"); - } - // Nor does naming it from a canister directory below the root help. - let err = fetch( - &modules, - &root.join("src"), - local("../../outside.wasm"), - None, - ) + /// On a real disk the links are followed by the host, which is what the + /// CLI resolves through. + #[cfg(all(unix, feature = "host"))] + #[tokio::test] + async fn on_disk_a_link_is_followed_to_where_it_leads() { + use crate::files::HostFileSystem; + + let dir = camino_tempfile::tempdir().unwrap(); + let root = dir.path().join("project"); + std::fs::create_dir(&root).unwrap(); + std::fs::write(root.join("backend.wasm"), b"module").unwrap(); + std::fs::write(dir.path().join("secret.wasm"), b"secret").unwrap(); + std::os::unix::fs::symlink(dir.path().join("secret.wasm"), root.join("out.wasm")).unwrap(); + std::os::unix::fs::symlink(root.join("backend.wasm"), root.join("in.wasm")).unwrap(); + + let mut project = project_with(vec![local("in.wasm")], vec![]).await; + project.dir = root.clone(); + project.canisters.get_mut("backend").unwrap().0 = root.clone(); + vet(&project, &HostFileSystem) .await - .unwrap_err(); - assert!(err.contains("outside.wasm"), "{err}"); - } - - #[cfg(unix)] - #[tokio::test] - async fn a_module_through_a_link_is_refused() { - let (_dir, modules, root) = project(); - let err = fetch(&modules, &root, local("link.wasm"), None) - .await - .unwrap_err(); - assert!(err.contains("passes through"), "{err}"); - assert!(err.contains("link.wasm"), "{err}"); - } + .expect("a link to the project's own module"); - #[tokio::test] - async fn a_module_named_by_url_is_not_fetched() { - let (_dir, modules, root) = project(); - let remote = SourceField::Remote(RemoteSource { - url: "https://example.com/backend.wasm".into(), - }); - let err = fetch(&modules, &root, remote, Some("abc")) - .await - .unwrap_err(); - assert!(err.contains("is not fetched"), "{err}"); - assert!(err.contains("example.com/backend.wasm"), "{err}"); - } + project.canisters.get_mut("backend").unwrap().1.build = BuildSteps { + steps: vec![local("out.wasm")], + }; + let err = vet(&project, &HostFileSystem).await.unwrap_err(); + assert!(matches!(err, Refused::OutsidePath { .. }), "{err}"); } } diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 9a51be2e7..fe50b4796 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -136,7 +136,7 @@ Build canisters ###### **Options:** * `-e`, `--environment ` — Override the environment to build for. By default, the local environment is used -* `--untrusted` — Hold the project to what one someone else handed over may have, which is the shape `icp project bundle` writes: each build step a pre-built module from a file in the project, each sync step a plugin from a file in the project, no recipe, and every path a regular file or directory under the project root. Anything else is refused +* `--untrusted` — Hold the project to what one someone else handed over may run: each build step a pre-built module, each sync step a plugin, and the file each names within the project once links are followed. Anything else is refused. Reading is not restricted @@ -1072,7 +1072,7 @@ using --args or --args-file: - `bin`: Raw binary (only valid for file references) -* `--untrusted` — Hold the project to what one someone else handed over may have, which is the shape `icp project bundle` writes: each build step a pre-built module from a file in the project, each sync step a plugin from a file in the project, no recipe, and every path a regular file or directory under the project root. Anything else is refused +* `--untrusted` — Hold the project to what one someone else handed over may run: each build step a pre-built module, each sync step a plugin, and the file each names within the project once links are followed. Anything else is refused. Reading is not restricted @@ -1921,7 +1921,7 @@ Synchronize canisters * `--proxy ` — Principal of a proxy canister to route sync plugin calls to the target canister through * `-e`, `--environment ` — Override the environment to connect to. By default, the local environment is used * `--identity ` — The user identity to run this command as -* `--untrusted` — Hold the project to what one someone else handed over may have, which is the shape `icp project bundle` writes: each build step a pre-built module from a file in the project, each sync step a plugin from a file in the project, no recipe, and every path a regular file or directory under the project root. Anything else is refused +* `--untrusted` — Hold the project to what one someone else handed over may run: each build step a pre-built module, each sync step a plugin, and the file each names within the project once links are followed. Anything else is refused. Reading is not restricted From 065b250871ff37ec6a3d3e2abc5c951aef6e6663 Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Fri, 6 Nov 2026 15:47:44 +0000 Subject: [PATCH 07/10] test(untrusted): a chain of links, and a linked directory on the way, lead where they lead Co-Authored-By: Claude Fable 5.1 --- crates/icp-project/src/untrusted.rs | 48 +++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 13 deletions(-) diff --git a/crates/icp-project/src/untrusted.rs b/crates/icp-project/src/untrusted.rs index d13a05294..299ebf81e 100644 --- a/crates/icp-project/src/untrusted.rs +++ b/crates/icp-project/src/untrusted.rs @@ -451,31 +451,53 @@ mod tests { } /// On a real disk the links are followed by the host, which is what the - /// CLI resolves through. + /// CLI resolves through: to the end of a chain of them, and through a + /// linked directory on the way. #[cfg(all(unix, feature = "host"))] #[tokio::test] async fn on_disk_a_link_is_followed_to_where_it_leads() { use crate::files::HostFileSystem; + use std::os::unix::fs::symlink; let dir = camino_tempfile::tempdir().unwrap(); let root = dir.path().join("project"); std::fs::create_dir(&root).unwrap(); std::fs::write(root.join("backend.wasm"), b"module").unwrap(); std::fs::write(dir.path().join("secret.wasm"), b"secret").unwrap(); - std::os::unix::fs::symlink(dir.path().join("secret.wasm"), root.join("out.wasm")).unwrap(); - std::os::unix::fs::symlink(root.join("backend.wasm"), root.join("in.wasm")).unwrap(); - - let mut project = project_with(vec![local("in.wasm")], vec![]).await; + symlink(root.join("backend.wasm"), root.join("in.wasm")).unwrap(); + // out.wasm -> hop.wasm -> ../secret.wasm + symlink(dir.path().join("secret.wasm"), root.join("hop.wasm")).unwrap(); + symlink(root.join("hop.wasm"), root.join("out.wasm")).unwrap(); + // outdir -> .. so that outdir/secret.wasm is the file outside + symlink(dir.path(), root.join("outdir")).unwrap(); + // chain of three inside: a -> b -> c -> backend.wasm + symlink(root.join("backend.wasm"), root.join("c.wasm")).unwrap(); + symlink(root.join("c.wasm"), root.join("b.wasm")).unwrap(); + symlink(root.join("b.wasm"), root.join("a.wasm")).unwrap(); + + let mut project = project_with(vec![], vec![]).await; project.dir = root.clone(); project.canisters.get_mut("backend").unwrap().0 = root.clone(); - vet(&project, &HostFileSystem) - .await - .expect("a link to the project's own module"); - project.canisters.get_mut("backend").unwrap().1.build = BuildSteps { - steps: vec![local("out.wasm")], - }; - let err = vet(&project, &HostFileSystem).await.unwrap_err(); - assert!(matches!(err, Refused::OutsidePath { .. }), "{err}"); + for path in ["in.wasm", "a.wasm"] { + project.canisters.get_mut("backend").unwrap().1.build = BuildSteps { + steps: vec![local(path)], + }; + vet(&project, &HostFileSystem) + .await + .unwrap_or_else(|err| panic!("{path} leads to the project's own module: {err}")); + } + + for path in ["out.wasm", "outdir/secret.wasm"] { + project.canisters.get_mut("backend").unwrap().1.build = BuildSteps { + steps: vec![local(path)], + }; + let err = vet(&project, &HostFileSystem).await.unwrap_err(); + assert!( + matches!(err, Refused::OutsidePath { ref resolved, .. } + if resolved.file_name() == Some("secret.wasm")), + "{path}: {err}" + ); + } } } From 480f0d2b56928962fbfc5239ef4ab7054d3d06a2 Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Mon, 9 Nov 2026 12:46:46 +0000 Subject: [PATCH 08/10] fix(untrusted): what a step names must be a regular file Canonicalization says where a path lands, not what is there, so a directory or a named pipe within the project passed the vet. A pipe is then read from whoever writes to it, and one nobody writes to is waited on forever, at the checksum or at the build. The vet now asks the filesystem whether the resolved path is a regular file and refuses otherwise, naming what it found, with the mock, on-disk and CLI tests to show it. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 2 +- crates/icp-cli/src/options.rs | 6 +- crates/icp-cli/tests/untrusted_tests.rs | 45 ++++++++ crates/icp-project/src/untrusted.rs | 141 +++++++++++++++++++----- docs/reference/cli.md | 6 +- 5 files changed, 164 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a76399037..81c293aef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ air-gapped signing, extensions # Unreleased -* feat: `--untrusted`, on `build`, `deploy` and `sync`, holds the project to what one someone else handed over may run — a bundle an agent produced, a checkout nobody has read. The list is stated as what is allowed, so that a kind of step added later is refused until someone decides otherwise: each build step is a pre-built module, each sync step is a plugin, and the file each names is within the project, judged by where it really is once every link is followed and every `..` applied. Anything else is refused before the command acts on it, naming what was found. Reading is not restricted: argument files, manifests and recipes are read from wherever the manifest points, and the steps a recipe renders to are held to the same list. Without the flag nothing changes: the user's own project may still run scripts and take its modules from anywhere. +* feat: `--untrusted`, on `build`, `deploy` and `sync`, holds the project to what one someone else handed over may run — a bundle an agent produced, a checkout nobody has read. The list is stated as what is allowed, so that a kind of step added later is refused until someone decides otherwise: each build step is a pre-built module, each sync step is a plugin, and what each names is a regular file within the project, judged by where it really is once every link is followed and every `..` applied. Anything else is refused before the command acts on it, naming what was found. Reading is not restricted: argument files, manifests and recipes are read from wherever the manifest points, and the steps a recipe renders to are held to the same list. Without the flag nothing changes: the user's own project may still run scripts and take its modules from anywhere. * chore: macOS on Intel (`x86_64-apple-darwin`) is no longer a release target. Releases ship no Intel macOS archive, the shell installer refuses that platform, and npm no longer publishes `@icp-sdk/icp-cli-darwin-x64`, so installing `@icp-sdk/icp-cli` on an Intel Mac warns of an unsupported platform. Homebrew dropped Intel macOS in September 2026, and GitHub is retiring its Intel macOS runners. v1.8.0 is the last release with an Intel macOS build. ## Experimental diff --git a/crates/icp-cli/src/options.rs b/crates/icp-cli/src/options.rs index 69b810a9e..a691ba1af 100644 --- a/crates/icp-cli/src/options.rs +++ b/crates/icp-cli/src/options.rs @@ -46,9 +46,9 @@ impl From for IdentitySelection { #[derive(Args, Clone, Debug, Default)] pub(crate) struct UntrustedOpt { /// Hold the project to what one someone else handed over may run: each - /// build step a pre-built module, each sync step a plugin, and the file - /// each names within the project once links are followed. Anything else - /// is refused. Reading is not restricted + /// build step a pre-built module, each sync step a plugin, and what each + /// names a regular file within the project once links are followed. + /// Anything else is refused. Reading is not restricted #[arg(long)] pub(crate) untrusted: bool, } diff --git a/crates/icp-cli/tests/untrusted_tests.rs b/crates/icp-cli/tests/untrusted_tests.rs index 7574a98f1..4711fb876 100644 --- a/crates/icp-cli/tests/untrusted_tests.rs +++ b/crates/icp-cli/tests/untrusted_tests.rs @@ -226,6 +226,51 @@ fn a_module_linked_out_of_the_project_is_refused_and_one_linked_within_is_not() .success(); } +/// What a step names must be a regular file: a directory is refused, and on +/// unix so is a named pipe, before the build would try to read it and wait +/// on a writer that never comes. +#[test] +fn a_module_that_is_not_a_regular_file_is_refused_as_untrusted() { + let ctx = TestContext::new(); + let project_dir = project( + &ctx, + indoc! {r#" + canisters: + - name: backend + build: + steps: + - type: pre-built + path: module.wasm + "#}, + ); + let module = project_dir.join("module.wasm"); + + std::fs::create_dir(&module).expect("directory"); + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .failure() + .stderr(contains("module.wasm").and(contains("not a regular file"))); + + #[cfg(unix)] + { + std::fs::remove_dir(&module).expect("remove directory"); + let made = std::process::Command::new("mkfifo") + .arg(&module) + .status() + .expect("mkfifo"); + assert!(made.success(), "mkfifo"); + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .timeout(std::time::Duration::from_secs(60)) + .assert() + .failure() + .stderr(contains("module.wasm").and(contains("not a regular file"))); + } +} + /// Where the file really is decides, so the root may be spelled relatively. #[test] fn a_relative_project_root_is_taken_from_the_working_directory() { diff --git a/crates/icp-project/src/untrusted.rs b/crates/icp-project/src/untrusted.rs index 299ebf81e..e2189eb9d 100644 --- a/crates/icp-project/src/untrusted.rs +++ b/crates/icp-project/src/untrusted.rs @@ -13,9 +13,10 @@ //! //! - a build step is a pre-built module ([`build_step_allowed`]); //! - a sync step is a plugin ([`sync_step_allowed`]); -//! - the module or plugin a step names is a file within the project: its -//! path, with every link followed and every `..` applied, lies under the -//! project root resolved the same way ([`vet`]). +//! - the module or plugin a step names is a regular file within the +//! project: its path, with every link followed and every `..` applied, +//! lies under the project root resolved the same way, and what is there is +//! a file rather than a directory or a pipe ([`vet`]). //! //! A recipe is read and rendered as the project loads, and the steps it //! produces are on the list or not like any other. A module or plugin named @@ -74,7 +75,17 @@ pub enum Refused { root: PathBuf, }, - #[snafu(display("canister '{canister}' names '{path}', which does not resolve to a file"))] + #[snafu(display( + "canister '{canister}' names '{path}', which is '{resolved}', not a regular file: an \ + untrusted project runs only a file within it" + ))] + NotAFile { + canister: String, + path: PathBuf, + resolved: PathBuf, + }, + + #[snafu(display("canister '{canister}' names '{path}', which does not resolve to anything"))] UnresolvedPath { canister: String, path: PathBuf }, #[snafu(display("the project root '{root}' does not resolve to a directory"))] @@ -96,13 +107,17 @@ pub fn sync_step_allowed(step: &SyncStep) -> bool { } /// Hold a loaded project to the list: every step [`build_step_allowed`] or -/// [`sync_step_allowed`], and the file each names within the project. +/// [`sync_step_allowed`], and the file each names a regular file within the +/// project. /// /// Within is decided on the real paths: the file's and the root's, each with /// every link followed and every `..` applied by `files`, so a link out of /// the project is seen for where it leads and a link within it is let be. A /// path that does not resolve is refused too, since nothing can be said -/// about where it would lead. +/// about where it would lead. So is one that resolves to something other +/// than a regular file: a directory cannot be read as a module, and a pipe +/// is read from whoever is writing to it, which is not the project — and +/// a read of one that nobody writes to waits forever. pub async fn vet(project: &Project, files: &dyn FileSystem) -> Result<(), Refused> { let root = files .canonicalize(&project.dir) @@ -146,7 +161,8 @@ pub async fn vet(project: &Project, files: &dyn FileSystem) -> Result<(), Refuse Ok(()) } -/// Refuse `path` unless it resolves to somewhere under the resolved `root`. +/// Refuse `path` unless it resolves to a regular file under the resolved +/// `root`. async fn within( files: &dyn FileSystem, root: &Path, @@ -169,6 +185,14 @@ async fn within( } .fail(); } + if !files.is_file(&resolved).await { + return NotAFileSnafu { + canister: &canister.name, + path, + resolved, + } + .fail(); + } Ok(()) } @@ -222,7 +246,7 @@ impl ProjectLoad for Vetted { #[cfg(test)] mod tests { - use std::collections::HashMap; + use std::collections::{HashMap, HashSet}; use super::*; use crate::{ @@ -235,14 +259,21 @@ mod tests { }; /// A filesystem that resolves the paths it is told to and nothing else, - /// standing in for links and `..` on disk: the vet asks for nothing but - /// where a path really is. - struct Resolves(HashMap); + /// standing in for links and `..` on disk, and says which of the places + /// they resolve to hold a regular file: the vet asks for nothing but + /// where a path really is and what is there. + struct Resolves { + paths: HashMap, + files: HashSet, + } #[async_trait] impl FileSystem for Resolves { async fn canonicalize(&self, path: &Path) -> Option { - self.0.get(path).cloned() + self.paths.get(path).cloned() + } + async fn is_file(&self, path: &Path) -> bool { + self.files.contains(path) } async fn read(&self, _: &Path) -> Result, FsError> { unreachable!("the vet reads nothing") @@ -262,9 +293,6 @@ mod tests { async fn exists(&self, _: &Path) -> bool { unreachable!("the vet looks at nothing but where a path is") } - async fn is_file(&self, _: &Path) -> bool { - unreachable!("the vet looks at nothing but where a path is") - } async fn is_dir(&self, _: &Path) -> bool { unreachable!("the vet looks at nothing but where a path is") } @@ -278,22 +306,31 @@ mod tests { /// The minimal project is at `/project`, and this is its disk: a module /// of its own, a plugin, a link out to `/elsewhere/secret.wasm`, a link - /// within to the module, and a climb that lands outside. + /// within to the module, a climb that lands outside, and a directory + /// `dir` of its own. fn disk() -> Arc { - Arc::new(Resolves(HashMap::from([ - ("/project".into(), "/project".into()), - ( + Arc::new(Resolves { + paths: HashMap::from([ + ("/project".into(), "/project".into()), + ( + "/project/backend.wasm".into(), + "/project/backend.wasm".into(), + ), + ("/project/p.wasm".into(), "/project/p.wasm".into()), + ("/project/out.wasm".into(), "/elsewhere/secret.wasm".into()), + ("/project/in.wasm".into(), "/project/backend.wasm".into()), + ( + "/project/../elsewhere/secret.wasm".into(), + "/elsewhere/secret.wasm".into(), + ), + ("/project/dir".into(), "/project/dir".into()), + ]), + files: HashSet::from([ "/project/backend.wasm".into(), - "/project/backend.wasm".into(), - ), - ("/project/p.wasm".into(), "/project/p.wasm".into()), - ("/project/out.wasm".into(), "/elsewhere/secret.wasm".into()), - ("/project/in.wasm".into(), "/project/backend.wasm".into()), - ( - "/project/../elsewhere/secret.wasm".into(), + "/project/p.wasm".into(), "/elsewhere/secret.wasm".into(), - ), - ]))) + ]), + }) } async fn project_with(build: Vec, sync: Vec) -> Project { @@ -435,6 +472,24 @@ mod tests { assert!(err.to_string().contains("missing.wasm"), "{err}"); } + /// Where a path leads must hold a regular file: a directory within the + /// project is refused as a module and as a plugin, and the message says + /// what was there instead. + #[tokio::test] + async fn what_a_step_names_must_be_a_regular_file() { + let err = vetted(vec![local("dir")], vec![]).await.unwrap_err(); + assert!( + matches!(err, Refused::NotAFile { ref resolved, .. } if resolved == "/project/dir"), + "{err}" + ); + assert!(err.to_string().contains("not a regular file"), "{err}"); + + let err = vetted(vec![local("backend.wasm")], vec![plugin("path: dir")]) + .await + .unwrap_err(); + assert!(matches!(err, Refused::NotAFile { .. }), "{err}"); + } + #[tokio::test] async fn a_vetted_loader_refuses_what_vet_refuses() { let project = project_with(vec![BuildStep::Script(script("make"))], vec![]).await; @@ -452,7 +507,9 @@ mod tests { /// On a real disk the links are followed by the host, which is what the /// CLI resolves through: to the end of a chain of them, and through a - /// linked directory on the way. + /// linked directory on the way. And what is at the end must be a + /// regular file: a directory and a named pipe are refused, the pipe + /// before anything tries to read it. #[cfg(all(unix, feature = "host"))] #[tokio::test] async fn on_disk_a_link_is_followed_to_where_it_leads() { @@ -474,6 +531,15 @@ mod tests { symlink(root.join("backend.wasm"), root.join("c.wasm")).unwrap(); symlink(root.join("c.wasm"), root.join("b.wasm")).unwrap(); symlink(root.join("b.wasm"), root.join("a.wasm")).unwrap(); + // a directory, and a pipe nobody writes to, each with a link to it + std::fs::create_dir(root.join("dir.wasm")).unwrap(); + symlink(root.join("dir.wasm"), root.join("to-dir.wasm")).unwrap(); + let made = std::process::Command::new("mkfifo") + .arg(root.join("pipe.wasm")) + .status() + .unwrap(); + assert!(made.success(), "mkfifo"); + symlink(root.join("pipe.wasm"), root.join("to-pipe.wasm")).unwrap(); let mut project = project_with(vec![], vec![]).await; project.dir = root.clone(); @@ -499,5 +565,22 @@ mod tests { "{path}: {err}" ); } + + for (path, at) in [ + ("dir.wasm", "dir.wasm"), + ("to-dir.wasm", "dir.wasm"), + ("pipe.wasm", "pipe.wasm"), + ("to-pipe.wasm", "pipe.wasm"), + ] { + project.canisters.get_mut("backend").unwrap().1.build = BuildSteps { + steps: vec![local(path)], + }; + let err = vet(&project, &HostFileSystem).await.unwrap_err(); + assert!( + matches!(err, Refused::NotAFile { ref resolved, .. } + if resolved.file_name() == Some(at)), + "{path}: {err}" + ); + } } } diff --git a/docs/reference/cli.md b/docs/reference/cli.md index fe50b4796..732a082ee 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -136,7 +136,7 @@ Build canisters ###### **Options:** * `-e`, `--environment ` — Override the environment to build for. By default, the local environment is used -* `--untrusted` — Hold the project to what one someone else handed over may run: each build step a pre-built module, each sync step a plugin, and the file each names within the project once links are followed. Anything else is refused. Reading is not restricted +* `--untrusted` — Hold the project to what one someone else handed over may run: each build step a pre-built module, each sync step a plugin, and what each names a regular file within the project once links are followed. Anything else is refused. Reading is not restricted @@ -1072,7 +1072,7 @@ using --args or --args-file: - `bin`: Raw binary (only valid for file references) -* `--untrusted` — Hold the project to what one someone else handed over may run: each build step a pre-built module, each sync step a plugin, and the file each names within the project once links are followed. Anything else is refused. Reading is not restricted +* `--untrusted` — Hold the project to what one someone else handed over may run: each build step a pre-built module, each sync step a plugin, and what each names a regular file within the project once links are followed. Anything else is refused. Reading is not restricted @@ -1921,7 +1921,7 @@ Synchronize canisters * `--proxy ` — Principal of a proxy canister to route sync plugin calls to the target canister through * `-e`, `--environment ` — Override the environment to connect to. By default, the local environment is used * `--identity ` — The user identity to run this command as -* `--untrusted` — Hold the project to what one someone else handed over may run: each build step a pre-built module, each sync step a plugin, and the file each names within the project once links are followed. Anything else is refused. Reading is not restricted +* `--untrusted` — Hold the project to what one someone else handed over may run: each build step a pre-built module, each sync step a plugin, and what each names a regular file within the project once links are followed. Anything else is refused. Reading is not restricted From f82b68fb76df4a9d8bc4ee13963f6e035947e4c9 Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Tue, 10 Nov 2026 00:31:07 +0000 Subject: [PATCH 09/10] fix(untrusted): ask what a step names is of the path as spelled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The vet judged where a module leads on its resolved path, and then asked the filesystem what was there by that same resolved path. A filesystem that knows paths by how its caller spells them — `Confined`, with the project reached through a link, as a macOS temp dir is through `/var` — resolves the module to a place not under that spelling and so has no answer, and a module within the project was refused as not a file. The CLI composes the vet over the host filesystem, where the two spellings answer alike, so nothing shipped was affected; an embedder composing it over a confined one would have been. Where a path leads is still judged resolved; what is there is now asked of the path as spelled, which a link is followed from either way. The test double follows a path to where it resolves before answering, and an on-disk test holds a root spelled through a link to accepting its own module and still refusing a directory. Co-Authored-By: Claude Fable 5.1 --- crates/icp-project/src/untrusted.rs | 52 ++++++++++++++++++++++++++--- 1 file changed, 48 insertions(+), 4 deletions(-) diff --git a/crates/icp-project/src/untrusted.rs b/crates/icp-project/src/untrusted.rs index e2189eb9d..f15244632 100644 --- a/crates/icp-project/src/untrusted.rs +++ b/crates/icp-project/src/untrusted.rs @@ -163,6 +163,12 @@ pub async fn vet(project: &Project, files: &dyn FileSystem) -> Result<(), Refuse /// Refuse `path` unless it resolves to a regular file under the resolved /// `root`. +/// +/// Where the path leads is judged on the resolved path; what is there is +/// asked of the path as spelled. The two answers agree, since a link is +/// followed either way, and a filesystem that knows paths by how its +/// caller spells them — one confined to a root reached through a link — +/// has no answer for the resolved one. async fn within( files: &dyn FileSystem, root: &Path, @@ -185,7 +191,7 @@ async fn within( } .fail(); } - if !files.is_file(&resolved).await { + if !files.is_file(path).await { return NotAFileSnafu { canister: &canister.name, path, @@ -260,8 +266,9 @@ mod tests { /// A filesystem that resolves the paths it is told to and nothing else, /// standing in for links and `..` on disk, and says which of the places - /// they resolve to hold a regular file: the vet asks for nothing but - /// where a path really is and what is there. + /// they resolve to hold a regular file — asked of a path by any + /// spelling, followed to where it resolves, as the host follows a link. + /// The vet asks for nothing but where a path really is and what is there. struct Resolves { paths: HashMap, files: HashSet, @@ -273,7 +280,9 @@ mod tests { self.paths.get(path).cloned() } async fn is_file(&self, path: &Path) -> bool { - self.files.contains(path) + self.paths + .get(path) + .is_some_and(|resolved| self.files.contains(resolved)) } async fn read(&self, _: &Path) -> Result, FsError> { unreachable!("the vet reads nothing") @@ -583,4 +592,39 @@ mod tests { ); } } + + /// A filesystem that holds paths to a root as the caller spells it — the + /// confined one, with the project reached through a link, as a macOS + /// temp dir is through `/var` — resolves a module to its real place, + /// which is not under that spelling. The vet must not then ask it what + /// is at the real place: a module within the project is accepted, and + /// what is not a file is still refused. + #[cfg(all(unix, feature = "host"))] + #[tokio::test] + async fn on_disk_a_root_spelled_through_a_link_is_asked_by_that_spelling() { + use crate::files::Confined; + use std::os::unix::fs::symlink; + + let dir = camino_tempfile::tempdir().unwrap(); + let real = dir.path().join("project"); + std::fs::create_dir(&real).unwrap(); + std::fs::write(real.join("backend.wasm"), b"module").unwrap(); + std::fs::create_dir(real.join("dir.wasm")).unwrap(); + let spelled = dir.path().join("alias"); + symlink(&real, &spelled).unwrap(); + let files = Confined::new(&spelled); + + let mut project = project_with(vec![local("backend.wasm")], vec![]).await; + project.dir = spelled.clone(); + project.canisters.get_mut("backend").unwrap().0 = spelled.clone(); + vet(&project, &files) + .await + .expect("the project's own module, with the root spelled through a link"); + + project.canisters.get_mut("backend").unwrap().1.build = BuildSteps { + steps: vec![local("dir.wasm")], + }; + let err = vet(&project, &files).await.unwrap_err(); + assert!(matches!(err, Refused::NotAFile { .. }), "{err}"); + } } From 5f68f0284c257dd5687ecf0041adab76a05fb204 Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Tue, 10 Nov 2026 01:03:56 +0000 Subject: [PATCH 10/10] refactor(untrusted): one match per step kind, and less said twice vet asked build_step_allowed and then matched the step again with an unreachable arm; local_module and local_plugin now answer both at once, and match every kind so a new one must be decided on. init.rs keeps to the change it needs. The CLI tests drop the cases the unit tests already hold on disk (a directory, a pipe, a link within). Co-Authored-By: Claude --- CHANGELOG.md | 2 +- crates/icp-app/src/context/init.rs | 20 ++-- crates/icp-cli/tests/untrusted_tests.rs | 58 +--------- crates/icp-project/src/untrusted.rs | 135 ++++++++++-------------- 4 files changed, 65 insertions(+), 150 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 81c293aef..6afc5400f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ air-gapped signing, extensions # Unreleased -* feat: `--untrusted`, on `build`, `deploy` and `sync`, holds the project to what one someone else handed over may run — a bundle an agent produced, a checkout nobody has read. The list is stated as what is allowed, so that a kind of step added later is refused until someone decides otherwise: each build step is a pre-built module, each sync step is a plugin, and what each names is a regular file within the project, judged by where it really is once every link is followed and every `..` applied. Anything else is refused before the command acts on it, naming what was found. Reading is not restricted: argument files, manifests and recipes are read from wherever the manifest points, and the steps a recipe renders to are held to the same list. Without the flag nothing changes: the user's own project may still run scripts and take its modules from anywhere. +* feat: `--untrusted`, on `build`, `deploy` and `sync`, holds a project someone else handed over to what it may run: each build step is a pre-built module, each sync step is a plugin, and the file each names is a regular file within the project once every link is followed. Anything else is refused before the command acts on it, naming what was found. Reading is not restricted: argument files, manifests and recipes are read from wherever the manifest points, and the steps a recipe renders to are held to the same list. * chore: macOS on Intel (`x86_64-apple-darwin`) is no longer a release target. Releases ship no Intel macOS archive, the shell installer refuses that platform, and npm no longer publishes `@icp-sdk/icp-cli-darwin-x64`, so installing `@icp-sdk/icp-cli` on an Intel Mac warns of an unsupported platform. Homebrew dropped Intel macOS in September 2026, and GitHub is retiring its Intel macOS runners. v1.8.0 is the last release with an Intel macOS build. ## Experimental diff --git a/crates/icp-app/src/context/init.rs b/crates/icp-app/src/context/init.rs index b340dc998..a39edb627 100644 --- a/crates/icp-app/src/context/init.rs +++ b/crates/icp-app/src/context/init.rs @@ -12,10 +12,7 @@ use icp_project::store_artifact::ArtifactStore; use std::time::Duration; use crate::{agent, identity, identity::PasswordFunc}; -use icp_project::{ - Lazy, ProjectLoad, ProjectLoadImpl, files::HostFileSystem, host::Host, manifest, store_id, - untrusted, -}; +use icp_project::{Lazy, ProjectLoad, ProjectLoadImpl, host::Host, manifest, store_id, untrusted}; #[derive(Debug, Snafu)] pub enum ContextInitError { @@ -41,11 +38,8 @@ pub enum ContextInitError { }, } -/// `untrusted` holds the project to what one someone else handed over may -/// run — see [`icp_project::untrusted`] for the list: the loaded project is -/// vetted for its steps and for where the file each names resolves, before -/// any command acts on it. Reading is not restricted, so the project is -/// loaded through the same filesystem and fetchers as the user's own. +/// With `untrusted`, the loaded project is vetted by [`icp_project::untrusted`] +/// before any command acts on it. pub fn initialize( project_root_override: Option, debug: bool, @@ -103,21 +97,21 @@ pub fn initialize( // holder. let pkg_cache = Arc::new(dirs.package_cache().context(PackageCacheSnafu)?); - // The project's files come from this machine - let files = Arc::new(HostFileSystem); - // Wasm modules named by a manifest but not contained in the project let wasm = Arc::new(crate::wasm::Fetcher::new( http_client.clone(), pkg_cache.clone(), )); - // Recipes, fetched from wherever the manifest points + // Recipes let recipe = Arc::new(RecipeFetcher { http_client, pkg_cache, }); + // The project's files come from this machine. + let files = Arc::new(icp_project::files::HostFileSystem); + // Canister builder let builder = Arc::new(Builder::new(wasm.clone(), files.clone())); diff --git a/crates/icp-cli/tests/untrusted_tests.rs b/crates/icp-cli/tests/untrusted_tests.rs index 4711fb876..7d447f75f 100644 --- a/crates/icp-cli/tests/untrusted_tests.rs +++ b/crates/icp-cli/tests/untrusted_tests.rs @@ -179,11 +179,10 @@ fn a_module_outside_the_project_is_refused_as_untrusted() { ); } -/// A link is judged by where it leads: out of the project is refused, within -/// it is fine. +/// A link is judged by where it leads: out of the project is refused. #[cfg(unix)] #[test] -fn a_module_linked_out_of_the_project_is_refused_and_one_linked_within_is_not() { +fn a_module_linked_out_of_the_project_is_refused_as_untrusted() { let ctx = TestContext::new(); let project_dir = project( &ctx, @@ -216,59 +215,6 @@ fn a_module_linked_out_of_the_project_is_refused_and_one_linked_within_is_not() .and(contains("outside.wasm")) .and(contains("outside the project")), ); - - std::fs::remove_file(&link).expect("remove link"); - std::os::unix::fs::symlink(project_dir.join("backend.wasm"), &link).expect("symlink within"); - ctx.icp() - .current_dir(&project_dir) - .args(["build", "--untrusted"]) - .assert() - .success(); -} - -/// What a step names must be a regular file: a directory is refused, and on -/// unix so is a named pipe, before the build would try to read it and wait -/// on a writer that never comes. -#[test] -fn a_module_that_is_not_a_regular_file_is_refused_as_untrusted() { - let ctx = TestContext::new(); - let project_dir = project( - &ctx, - indoc! {r#" - canisters: - - name: backend - build: - steps: - - type: pre-built - path: module.wasm - "#}, - ); - let module = project_dir.join("module.wasm"); - - std::fs::create_dir(&module).expect("directory"); - ctx.icp() - .current_dir(&project_dir) - .args(["build", "--untrusted"]) - .assert() - .failure() - .stderr(contains("module.wasm").and(contains("not a regular file"))); - - #[cfg(unix)] - { - std::fs::remove_dir(&module).expect("remove directory"); - let made = std::process::Command::new("mkfifo") - .arg(&module) - .status() - .expect("mkfifo"); - assert!(made.success(), "mkfifo"); - ctx.icp() - .current_dir(&project_dir) - .args(["build", "--untrusted"]) - .timeout(std::time::Duration::from_secs(60)) - .assert() - .failure() - .stderr(contains("module.wasm").and(contains("not a regular file"))); - } } /// Where the file really is decides, so the root may be spelled relatively. diff --git a/crates/icp-project/src/untrusted.rs b/crates/icp-project/src/untrusted.rs index f15244632..d87edc920 100644 --- a/crates/icp-project/src/untrusted.rs +++ b/crates/icp-project/src/untrusted.rs @@ -5,34 +5,21 @@ //! take its module or plugin from anywhere on the machine. A project from //! elsewhere — an archive an agent passed to a deploy service, a checkout the //! user has not read — may not. The line is between reading and running. -//! Reading is not restricted: the loader reads argument files, environment -//! files, manifests and recipe templates from wherever the manifest points, -//! and a plugin reads the files it declares, through the ordinary filesystem. -//! What runs is held to a list, stated as what is allowed so that a kind of -//! step added later is refused until someone decides otherwise: +//! Reading is not restricted: argument files, manifests and recipe templates +//! are read from wherever the manifest points. What runs is held to a list, +//! stated as what is allowed so that a kind of step added later is refused +//! until someone decides otherwise: //! -//! - a build step is a pre-built module ([`build_step_allowed`]); -//! - a sync step is a plugin ([`sync_step_allowed`]); -//! - the module or plugin a step names is a regular file within the -//! project: its path, with every link followed and every `..` applied, -//! lies under the project root resolved the same way, and what is there is -//! a file rather than a directory or a pipe ([`vet`]). +//! - a build step is a pre-built module, and a sync step is a plugin; +//! - the file either names is a regular file within the project: its path, +//! with every link followed and every `..` applied, lies under the project +//! root resolved the same way. //! -//! A recipe is read and rendered as the project loads, and the steps it -//! produces are on the list or not like any other. A module or plugin named -//! by URL is not on it: a download at build time is what a bundle has already -//! done. The loaded [`Project`] shows every step, so the list is applied to -//! it: [`vet`] holds a project to it, and [`Vetted`] wraps a loader so that -//! every load is vetted before the project is handed out. A consumer -//! composing its own host wraps its loader the same way: -//! -//! ```ignore -//! let project = Vetted::new(ProjectLoadImpl { project_root_locate, recipe, files: files.clone() }, files); -//! ``` -//! -//! A checksum on a module is not asked for. It is the bundler's promise about -//! its own archive, and the same untrusted hand wrote both, so it says nothing -//! about what the project runs. +//! A module or plugin named by URL is not on the list, and the steps a recipe +//! renders to are held to it like any other. A checksum is not asked for: the +//! same untrusted hand wrote it and the module. [`vet`] applies the list to a +//! loaded [`Project`], and [`Vetted`] wraps a loader so that every load is +//! vetted before the project is handed out. use std::sync::Arc; @@ -43,7 +30,7 @@ use crate::{ Project, ProjectLoad, ProjectLoadError, UntrustedSnafu, files::FileSystem, manifest::{ - adapter::prebuilt::SourceField, + adapter::prebuilt::{LocalSource, SourceField}, canister::{BuildStep, SyncStep}, }, prelude::*, @@ -92,32 +79,37 @@ pub enum Refused { UnresolvedRoot { root: PathBuf }, } -/// Whether `step` is the one build step an untrusted project may have: a -/// pre-built module from a file. A script is a host subprocess, and a module -/// fetched from a URL is a download on the strength of an untrusted manifest. -pub fn build_step_allowed(step: &BuildStep) -> bool { - matches!(step, BuildStep::Prebuilt(adapter) if matches!(adapter.source, SourceField::Local(_))) +/// The file `step` names, if it is the one build step an untrusted project +/// may have: a pre-built module from a file. +fn local_module(step: &BuildStep) -> Option<&LocalSource> { + match step { + BuildStep::Prebuilt(adapter) => local_source(&adapter.source), + BuildStep::Script(_) => None, + } +} + +/// The file `step` names, if it is the one sync step an untrusted project +/// may have: a plugin from a file. +fn local_plugin(step: &SyncStep) -> Option<&LocalSource> { + match step { + SyncStep::Plugin(adapter) => local_source(&adapter.source), + SyncStep::Script(_) => None, + } } -/// Whether `step` is the one sync step an untrusted project may have: a -/// plugin from a file. It runs sandboxed either way; it is a script and a -/// download that are not on the list. -pub fn sync_step_allowed(step: &SyncStep) -> bool { - matches!(step, SyncStep::Plugin(adapter) if matches!(adapter.source, SourceField::Local(_))) +fn local_source(source: &SourceField) -> Option<&LocalSource> { + match source { + SourceField::Local(local) => Some(local), + SourceField::Remote(_) => None, + } } -/// Hold a loaded project to the list: every step [`build_step_allowed`] or -/// [`sync_step_allowed`], and the file each names a regular file within the -/// project. +/// Hold a loaded project to the list. /// -/// Within is decided on the real paths: the file's and the root's, each with -/// every link followed and every `..` applied by `files`, so a link out of -/// the project is seen for where it leads and a link within it is let be. A -/// path that does not resolve is refused too, since nothing can be said -/// about where it would lead. So is one that resolves to something other -/// than a regular file: a directory cannot be read as a module, and a pipe -/// is read from whoever is writing to it, which is not the project — and -/// a read of one that nobody writes to waits forever. +/// Within is decided on the real paths, the file's and the root's, as +/// `files` canonicalizes them, so a link is judged by where it leads. A path +/// that does not resolve is refused, and so is one that is not a regular +/// file: a read of a pipe nobody writes to waits forever. pub async fn vet(project: &Project, files: &dyn FileSystem) -> Result<(), Refused> { let root = files .canonicalize(&project.dir) @@ -126,34 +118,22 @@ pub async fn vet(project: &Project, files: &dyn FileSystem) -> Result<(), Refuse for (_, (canister_dir, canister)) in &project.canisters { for step in &canister.build.steps { - let local = match step { - BuildStep::Prebuilt(adapter) if build_step_allowed(step) => match &adapter.source { - SourceField::Local(local) => local, - SourceField::Remote(_) => unreachable!("allowed is local"), - }, - _ => { - return BuildStepSnafu { - canister: &canister.name, - step: one_line(step), - } - .fail(); + let Some(local) = local_module(step) else { + return BuildStepSnafu { + canister: &canister.name, + step: one_line(step), } + .fail(); }; within(files, &root, canister, &canister_dir.join(&local.path)).await?; } for step in &canister.sync.steps { - let local = match step { - SyncStep::Plugin(adapter) if sync_step_allowed(step) => match &adapter.source { - SourceField::Local(local) => local, - SourceField::Remote(_) => unreachable!("allowed is local"), - }, - _ => { - return SyncStepSnafu { - canister: &canister.name, - step: one_line(step), - } - .fail(); + let Some(local) = local_plugin(step) else { + return SyncStepSnafu { + canister: &canister.name, + step: one_line(step), } + .fail(); }; within(files, &root, canister, &canister_dir.join(&local.path)).await?; } @@ -162,13 +142,9 @@ pub async fn vet(project: &Project, files: &dyn FileSystem) -> Result<(), Refuse } /// Refuse `path` unless it resolves to a regular file under the resolved -/// `root`. -/// -/// Where the path leads is judged on the resolved path; what is there is -/// asked of the path as spelled. The two answers agree, since a link is -/// followed either way, and a filesystem that knows paths by how its -/// caller spells them — one confined to a root reached through a link — -/// has no answer for the resolved one. +/// `root`. What is there is asked of the path as spelled, not as resolved: a +/// link is followed either way, and a filesystem confined to a root reached +/// through a link has no answer for the resolved one. async fn within( files: &dyn FileSystem, root: &Path, @@ -202,9 +178,8 @@ async fn within( Ok(()) } -/// A step as it displays, on one line: a step displays its kind and then -/// its fields or commands one per line, and an error message has room for -/// one, so the lines after the first are run together with `;` between. +/// A step as it displays, on one line: its kind, then the lines after the +/// first run together with `;` between. fn one_line(step: &impl std::fmt::Display) -> String { let text = step.to_string(); let mut lines = text.lines().map(str::trim).filter(|line| !line.is_empty());