From f7623c8044a540b698772104df01b83cf958bfa3 Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Wed, 4 Nov 2026 19:11:35 +0000 Subject: [PATCH 1/6] fix(bundle): hold a bundle to regular files and directories A symlink inside a plugin's `files:` or `dirs:` directory was recorded as a link entry with its target unchecked, so a link to a file outside the project reached that file on whatever machine unpacked the bundle. A `files:` entry that was itself a symlink to a file outside the project was read straight into the bundle: it was validated lexically before the build, when it need not have existed, and the read followed the link. The archive walk is now the bundler's own rather than tar's. Every entry met is canonicalized and must land under the canonical project directory: one that does is appended as what it points at, so a project may link to its own files freely, and one that does not fails the bundle, naming the link and where it leads. A dangling link, a link back to a directory being copied, and anything that is neither a file nor a directory fail the same way. A `files:` entry is held to the project again once the build has run and it exists. The walk appends entries in sorted order, which makes the archive reproducible for a given tree where tar's own walk left the order to the filesystem. This is the bundling side of the rule that `files::Confined` (#866) relies on: a bundle holds no symlinks, a consumer refuses any archive entry that is not a regular file or directory (#863), and so the tree a confined loader reads has nothing in it that leads elsewhere. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 4 + crates/icp-cli/tests/bundle_tests.rs | 190 ++++++++++++++++++++ crates/icp-project/src/operations/bundle.rs | 180 ++++++++++++++++--- 3 files changed, 350 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a5bafb1b..63fd255bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ air-gapped signing, extensions # Unreleased +## Experimental + +* fix(bundle): `icp project bundle` holds only regular files and directories. A symlink inside a plugin's `files:` or `dirs:` directory used to be recorded as a link with its target unchecked, so a link to a file outside the project reached that file on whatever machine unpacked the bundle. A link to somewhere inside the project is now copied as what it points at, so a project may link to its own files freely; one that leads outside the project, dangles, or leads back to a directory being copied fails the bundle, naming the link and where it leads. A `files:` entry that is itself a symlink to a file outside the project is refused the same way, where it used to be read into the bundle. + * 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. # v1.8.0 diff --git a/crates/icp-cli/tests/bundle_tests.rs b/crates/icp-cli/tests/bundle_tests.rs index 669c38556..e9d153cd9 100644 --- a/crates/icp-cli/tests/bundle_tests.rs +++ b/crates/icp-cli/tests/bundle_tests.rs @@ -964,6 +964,196 @@ fn bundle_rejects_source_outside_project() { .stderr(contains("my-canister").and(contains("outside the project directory"))); } +/// A project with one canister whose plugin sync step declares `files: +/// seed: `, for the symlink tests: what `data/` holds is up to each. +#[cfg(unix)] +fn plugin_project_with_files_entry(ctx: &TestContext, entry: &str) -> PathBuf { + let project_dir = ctx.create_project_dir("icp"); + let wasm_src = ctx.make_asset("example_icp_mo.wasm"); + write(&project_dir.join("plugin.wasm"), b"\x00asm\x01\x00\x00\x00") + .expect("failed to write plugin wasm"); + let pm = formatdoc! {r#" + canisters: + - name: my-canister + build: + steps: + - type: script + command: cp '{wasm_src}' "$ICP_WASM_OUTPUT_PATH" + sync: + steps: + - type: plugin + path: plugin.wasm + files: + seed: {entry} + "#}; + write_string(&project_dir.join("icp.yaml"), &pm).expect("failed to write project manifest"); + project_dir +} + +/// Every entry of the archive at `path`, as its archive path, entry type and +/// contents. +#[cfg(unix)] +fn archive_entries(path: &Path) -> Vec<(String, tar::EntryType, Vec)> { + let bytes = fs::read(path.as_std_path()).expect("failed to read bundle"); + let gz = GzDecoder::new(BufReader::new(bytes.as_slice())); + let mut archive = Archive::new(gz); + let mut out = Vec::new(); + for entry in archive.entries().expect("failed to read archive entries") { + let mut entry = entry.expect("failed to read archive entry"); + let path = entry + .path() + .expect("failed to get entry path") + .to_string_lossy() + .into_owned(); + let kind = entry.header().entry_type(); + let mut data = Vec::new(); + entry.read_to_end(&mut data).expect("failed to read entry"); + out.push((path, kind, data)); + } + out +} + +/// A symlink inside a plugin directory that points somewhere inside the +/// project is copied into the bundle as what it points at — a file, or a whole +/// directory — so the archive holds regular files only and a project may link +/// to its own files freely. +#[cfg(unix)] +#[test] +fn bundle_copies_symlinks_inside_the_project_as_what_they_point_at() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "data"); + + create_dir_all(&project_dir.join("data")).expect("failed to create dir"); + create_dir_all(&project_dir.join("common")).expect("failed to create dir"); + write_string(&project_dir.join("data/real.txt"), "real").expect("failed to write file"); + write_string(&project_dir.join("common/c.txt"), "common").expect("failed to write file"); + std::os::unix::fs::symlink("real.txt", project_dir.join("data/link.txt")) + .expect("failed to create symlink"); + std::os::unix::fs::symlink("../common", project_dir.join("data/shared")) + .expect("failed to create symlink"); + + let bundle_path = project_dir.join("bundle.tar.gz"); + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", bundle_path.as_str()]) + .assert() + .success(); + + let entries = archive_entries(&bundle_path); + assert!( + entries + .iter() + .all(|(_, kind, _)| !kind.is_symlink() && !kind.is_hard_link()), + "the archive holds a link: {:?}", + entries.iter().map(|(p, k, _)| (p, *k)).collect::>() + ); + for (path, contents) in [ + ("plugins/my-canister/0/files/data/real.txt", "real"), + ("plugins/my-canister/0/files/data/link.txt", "real"), + ("plugins/my-canister/0/files/data/shared/c.txt", "common"), + ] { + let (_, kind, data) = entries + .iter() + .find(|(p, _, _)| p == path) + .unwrap_or_else(|| panic!("{path} missing from {entries:?}")); + assert!(kind.is_file(), "{path} is a {kind:?}"); + assert_eq!(data, contents.as_bytes(), "{path}"); + } +} + +/// A symlink inside a plugin directory that leads outside the project fails +/// the bundle, naming the link and where it leads, rather than being recorded +/// as a link for the unpacking machine to follow. +#[cfg(unix)] +#[test] +fn bundle_rejects_symlink_pointing_outside_the_project() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "data"); + + let outside = ctx.home_path().join("outside-secret.txt"); + write_string(&outside, "secret").expect("failed to write outside file"); + create_dir_all(&project_dir.join("data")).expect("failed to create dir"); + std::os::unix::fs::symlink(&outside, project_dir.join("data/leak.txt")) + .expect("failed to create symlink"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "bundle.tar.gz"]) + .assert() + .failure() + .stderr( + contains("leak.txt") + .and(contains("outside-secret.txt")) + .and(contains("outside the project directory")), + ); +} + +/// A symlink to nothing cannot be copied as what it points at, and is named +/// rather than skipped. +#[cfg(unix)] +#[test] +fn bundle_rejects_dangling_symlink() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "data"); + + create_dir_all(&project_dir.join("data")).expect("failed to create dir"); + std::os::unix::fs::symlink("nowhere.txt", project_dir.join("data/gone.txt")) + .expect("failed to create symlink"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "bundle.tar.gz"]) + .assert() + .failure() + .stderr( + contains("gone.txt") + .and(contains("nowhere.txt")) + .and(contains("does not exist")), + ); +} + +/// A symlink back to a directory the bundle is in the middle of copying would +/// copy it without end; it is refused, naming the link. +#[cfg(unix)] +#[test] +fn bundle_rejects_symlink_cycle() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "data"); + + create_dir_all(&project_dir.join("data/inner")).expect("failed to create dir"); + std::os::unix::fs::symlink("../../data", project_dir.join("data/inner/back")) + .expect("failed to create symlink"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "bundle.tar.gz"]) + .assert() + .failure() + .stderr(contains("back").and(contains("cycle"))); +} + +/// A `files:` entry that is itself a symlink to a file outside the project is +/// refused like any other source outside the project, where the lexical check +/// before the build would have let it through and the read followed it. +#[cfg(unix)] +#[test] +fn bundle_rejects_symlinked_plugin_file_outside_the_project() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "config.txt"); + + let outside = ctx.home_path().join("outside-secret.txt"); + write_string(&outside, "secret").expect("failed to write outside file"); + std::os::unix::fs::symlink(&outside, project_dir.join("config.txt")) + .expect("failed to create symlink"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "bundle.tar.gz"]) + .assert() + .failure() + .stderr(contains("my-canister").and(contains("outside the project directory"))); +} + /// The environment reaching build steps as `ICP_CLI_ENVIRONMENT` defaults to `ic` /// for a bundle, rather than the `local` the rest of the CLI defaults to. #[test] diff --git a/crates/icp-project/src/operations/bundle.rs b/crates/icp-project/src/operations/bundle.rs index e561f9641..9309bfb97 100644 --- a/crates/icp-project/src/operations/bundle.rs +++ b/crates/icp-project/src/operations/bundle.rs @@ -248,6 +248,39 @@ pub enum BundleError { path: PathBuf, source: crate::files::FsError, }, + + #[snafu(display( + "'{path}' resolves to '{target}', outside the project directory '{root}'; a symlink \ + there points out of the project, and a bundle holds only what is inside it" + ))] + LinkEscapesProject { + path: PathBuf, + target: PathBuf, + root: PathBuf, + }, + + #[snafu(display("'{path}' is a symlink to '{target}', which does not exist"))] + DanglingLink { path: PathBuf, target: String }, + + #[snafu(display( + "'{path}' is a symlink back to '{target}', a directory the bundle is already copying; \ + a bundle cannot hold a cycle" + ))] + LinkCycle { path: PathBuf, target: PathBuf }, + + #[snafu(display("cannot resolve '{path}' to add it to the bundle"))] + ResolveArchiveEntry { + path: PathBuf, + source: std::io::Error, + }, + + #[snafu(display( + "'{path}' is neither a file nor a directory, which are all a bundle can hold" + ))] + UnsupportedEntry { path: PathBuf }, + + #[snafu(transparent)] + ListDir { source: crate::fs::IoError }, } /// In-memory bytes destined for a single tar entry. @@ -510,6 +543,7 @@ pub async fn create_bundle( write_archive( files, output, + &canonical_project_dir, &manifests, &bundle_artifacts, &args_files, @@ -1444,13 +1478,13 @@ struct ArchiveWriter { impl ArchiveWriter { fn new(inner: W) -> Self { let mut builder = Builder::new(inner); - // Record symlinks as Symlink entries rather than slurping their targets — keeps secrets - // outside the project from leaking via a symlinked asset dir. + // Every path handed to the builder is canonical, so there is no link left for it to + // follow; this is the backstop that keeps a link out of the archive should one get + // through, where it would otherwise be slurped as whatever it reaches. builder.follow_symlinks(false); - // Strip mtime/uid/gid from entry headers so they are metadata-normalized across machines. - // Note this does not make the archive fully byte-reproducible: `append_dir` relies on - // `append_dir_all`, which walks `read_dir` in the filesystem's order, so entry ordering - // within a directory can still differ between machines. + // Strip mtime/uid/gid from entry headers so they are metadata-normalized across + // machines, and `dir` appends a directory's entries in sorted order, so the same tree + // gives the same archive wherever it is bundled. builder.mode(tar::HeaderMode::Deterministic); Self { builder } } @@ -1468,19 +1502,74 @@ impl ArchiveWriter { } /// Appends a directory's whole contents, walking the host filesystem rather - /// than the [`FileSystem`] seam the rest of bundling reads through. + /// than the [`FileSystem`] seam the rest of bundling reads through: the seam + /// reports a link's target as the file it points at and has no way to say + /// otherwise, and a bundle holds only regular files and directories. + /// Bundling from anything but a host filesystem needs the seam to describe + /// links first. /// - /// `tar`'s walk is what keeps a symlink a symlink, per - /// [`new`](ArchiveWriter::new); the seam reports a link's target as the file - /// it points at and has no way to say otherwise, so reading through it would - /// slurp whatever the link reaches instead. Bundling from anything but a - /// host filesystem needs the seam to describe links first. - fn dir(&mut self, src_path: &Path, archive_prefix: &str) -> Result<(), BundleError> { + /// Every entry met is resolved through its symlinks and must land under + /// `root`, the canonical project directory. One that does is appended as + /// what it points at, so a project may link to its own files freely; one + /// that does not is refused, naming the link and where it leads, as is a + /// link that dangles or leads back to a directory already being copied. + /// Entries go in sorted order, so the same tree gives the same archive. + fn dir( + &mut self, + src_path: &Path, + archive_prefix: &str, + root: &Path, + ) -> Result<(), BundleError> { + self.walk(src_path, archive_prefix, root, &mut Vec::new()) + } + + /// `open` holds the canonical directories on the way down from `dir`'s + /// start, which is how a link back to one of them is told from a second + /// link to a directory already copied and finished with. + fn walk( + &mut self, + src_path: &Path, + archive_path: &str, + root: &Path, + open: &mut Vec, + ) -> Result<(), BundleError> { + let real = resolve_within_root(src_path, root)?; + if open.contains(&real) { + return LinkCycleSnafu { + path: src_path.to_path_buf(), + target: real, + } + .fail(); + } self.builder - .append_dir_all(archive_prefix, src_path.as_std_path()) + .append_dir(archive_path, real.as_std_path()) .context(WriteArchiveEntrySnafu { - path: PathBuf::from(archive_prefix), - }) + path: PathBuf::from(archive_path), + })?; + open.push(real.clone()); + for entry in crate::fs::read_dir(&real)? { + let Some(name) = entry.file_name() else { + continue; + }; + let entry_archive_path = format!("{archive_path}/{name}"); + let target = resolve_within_root(&entry, root)?; + let meta = std::fs::metadata(&target).context(ResolveArchiveEntrySnafu { + path: entry.clone(), + })?; + if meta.is_dir() { + self.walk(&entry, &entry_archive_path, root, open)?; + } else if meta.is_file() { + self.builder + .append_path_with_name(target.as_std_path(), &entry_archive_path) + .context(WriteArchiveEntrySnafu { + path: PathBuf::from(entry_archive_path), + })?; + } else { + return UnsupportedEntrySnafu { path: entry }.fail(); + } + } + open.pop(); + Ok(()) } fn into_inner(self) -> Result { @@ -1491,6 +1580,7 @@ impl ArchiveWriter { async fn write_archive( files: &dyn FileSystem, output: &Path, + canonical_project_dir: &Path, manifests: &[InstanceManifest], artifacts: &BundleArtifacts, args_files: &[ArgsFile], @@ -1564,17 +1654,24 @@ async fn write_archive( } for d in &artifacts.plugin_dirs { - archive.dir(&d.src_path, &d.archive_prefix)?; + archive.dir(&d.src_path, &d.archive_prefix, canonical_project_dir)?; } for pf in &artifacts.plugin_files { - let data = files - .read(&pf.src_path) - .await - .context(ReadPluginFileSnafu { - canister: pf.canister_name.clone(), - file: pf.orig_file.clone(), - })?; + // Validated lexically before the build, when it need not have existed; + // now it does, so a symlink in it is followed and held to the project + // like every other entry. + let src = canonicalize_within_project( + files, + &pf.src_path, + canonical_project_dir, + &pf.canister_name, + ) + .await?; + let data = files.read(&src).await.context(ReadPluginFileSnafu { + canister: pf.canister_name.clone(), + file: pf.orig_file.clone(), + })?; archive.bytes(&pf.archive_path, &data)?; } @@ -1819,6 +1916,41 @@ fn is_absolute_bind_mount_host(mount: &str) -> bool { !h.is_empty() && (h[0] == b'/' || h[0] == b'\\') } +/// Where `path` comes to rest with every symlink followed, which must be under +/// `root`. Walks the host directly, as [`ArchiveWriter::dir`] does: it is the +/// one place bundling has to know whether a path is a link, and the +/// [`FileSystem`] seam cannot say. +fn resolve_within_root(path: &Path, root: &Path) -> Result { + let real = match dunce::canonicalize(path) { + Ok(real) => real, + Err(source) => { + if let Ok(target) = std::fs::read_link(path) { + return DanglingLinkSnafu { + path: path.to_path_buf(), + target: target.display().to_string(), + } + .fail(); + } + return Err(BundleError::ResolveArchiveEntry { + path: path.to_path_buf(), + source, + }); + } + }; + let real = PathBuf::from_path_buf(real).map_err(|_| BundleError::CanonicalizePath { + path: path.to_path_buf(), + })?; + if !real.starts_with(root) { + return LinkEscapesProjectSnafu { + path: path.to_path_buf(), + target: real, + root: root.to_path_buf(), + } + .fail(); + } + Ok(real) +} + async fn canonicalize(files: &dyn FileSystem, path: &Path) -> Result { files .canonicalize(path) From 3290a83b111f0b8321a0c3bf58b15487ce8ff9c5 Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Wed, 4 Nov 2026 21:13:13 +0000 Subject: [PATCH 2/6] fix(bundle): refuse the output, a non-UTF-8 name and a looping link by name Three holes in the archive walk, from review: - The output path is checked against the copied directories by its spelling, which a symlink inside one of them gets around: with `assets/shared -> ../shared` and `--output shared/bundle.tar.gz`, the walk met the archive it was writing and copied a partial copy of it into itself. The canonical output now travels with the walk, and an entry that resolves to it is refused, named by the path it was reached by. - `crate::fs::read_dir` skips a name that is not UTF-8, which for a bundle meant a file silently left out where tar's walk had carried it. The walk lists directories itself and refuses such a name, since the archive cannot carry it either way. - A link was called dangling whenever canonicalizing it failed and `read_link` succeeded, which also covers a link that loops or leads somewhere unreadable. Only a `NotFound` is dangling now; the rest keep their own cause. Entries met under a link are listed where the directory really is but named by the path they were reached by, so every message says what the user wrote rather than where it led. Co-Authored-By: Claude Fable 5.1 --- crates/icp-cli/tests/bundle_tests.rs | 79 ++++++++++++++++++++ crates/icp-project/src/operations/bundle.rs | 81 ++++++++++++++++++--- 2 files changed, 148 insertions(+), 12 deletions(-) diff --git a/crates/icp-cli/tests/bundle_tests.rs b/crates/icp-cli/tests/bundle_tests.rs index e9d153cd9..e9f5ae874 100644 --- a/crates/icp-cli/tests/bundle_tests.rs +++ b/crates/icp-cli/tests/bundle_tests.rs @@ -1132,6 +1132,85 @@ fn bundle_rejects_symlink_cycle() { .stderr(contains("back").and(contains("cycle"))); } +/// A symlink that loops cannot be resolved at all, and that is the reason +/// given, rather than the link being called dangling. +#[cfg(unix)] +#[test] +fn bundle_rejects_symlink_loop() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "data"); + + create_dir_all(&project_dir.join("data")).expect("failed to create dir"); + std::os::unix::fs::symlink("self", project_dir.join("data/self")) + .expect("failed to create symlink"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "bundle.tar.gz"]) + .assert() + .failure() + .stderr( + contains("cannot resolve") + .and(contains("data/self")) + .and(contains("does not exist").not()), + ); +} + +/// The output path is checked against the copied directories by its spelling, +/// which a symlink inside one of them can get around: the walk then meets the +/// bundle it is writing, and refuses it rather than copying a partial archive +/// into itself. +#[cfg(unix)] +#[test] +fn bundle_rejects_output_reached_through_symlink() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "assets"); + + create_dir_all(&project_dir.join("assets")).expect("failed to create dir"); + create_dir_all(&project_dir.join("shared")).expect("failed to create dir"); + write_string(&project_dir.join("shared/s.txt"), "s").expect("failed to write file"); + std::os::unix::fs::symlink("../shared", project_dir.join("assets/shared")) + .expect("failed to create symlink"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "shared/bundle.tar.gz"]) + .assert() + .failure() + .stderr( + contains("assets/shared/bundle.tar.gz") + .and(contains("own output file")) + .and(contains("still being written")), + ); +} + +/// A file whose name is not UTF-8 cannot be named in the archive. The bundle +/// says so rather than leaving the file out, as the glob machinery elsewhere +/// does when it lists a directory. +#[cfg(target_os = "linux")] +#[test] +fn bundle_rejects_a_name_that_is_not_utf8() { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "data"); + + create_dir_all(&project_dir.join("data")).expect("failed to create dir"); + let stray = project_dir + .join("data") + .into_std_path_buf() + .join(OsStr::from_bytes(b"caf\xe9.txt")); + fs::write(&stray, b"x").expect("failed to write file"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "bundle.tar.gz"]) + .assert() + .failure() + .stderr(contains("not UTF-8").and(contains("data"))); +} + /// A `files:` entry that is itself a symlink to a file outside the project is /// refused like any other source outside the project, where the lexical check /// before the build would have let it through and the read followed it. diff --git a/crates/icp-project/src/operations/bundle.rs b/crates/icp-project/src/operations/bundle.rs index 9309bfb97..e7cec57f7 100644 --- a/crates/icp-project/src/operations/bundle.rs +++ b/crates/icp-project/src/operations/bundle.rs @@ -279,8 +279,22 @@ pub enum BundleError { ))] UnsupportedEntry { path: PathBuf }, - #[snafu(transparent)] - ListDir { source: crate::fs::IoError }, + #[snafu(display("failed to list '{path}' to add it to the bundle"))] + ListDir { + path: PathBuf, + source: std::io::Error, + }, + + #[snafu(display( + "'{dir}' holds an entry named '{name}' that is not UTF-8, which a bundle cannot carry" + ))] + NameNotUtf8 { dir: PathBuf, name: String }, + + #[snafu(display( + "'{path}' is the bundle's own output file '{output}', reached through a symlink, and \ + it is still being written; choose an output path outside what the bundle copies" + ))] + OutputReachedThroughLink { path: PathBuf, output: PathBuf }, } /// In-memory bytes destined for a single tar entry. @@ -464,7 +478,7 @@ pub async fn create_bundle( let canonical_sync_dirs = validate_source_paths(project_dir, &canisters, &canonical_project_dir)?; validate_env_var_files(files, &canisters, &canonical_project_dir).await?; - validate_output_path(files, output, &canonical_sync_dirs).await?; + let canonical_output = validate_output_path(files, output, &canonical_sync_dirs).await?; build_many( canisters.clone(), @@ -544,6 +558,7 @@ pub async fn create_bundle( files, output, &canonical_project_dir, + &canonical_output, &manifests, &bundle_artifacts, &args_files, @@ -1512,15 +1527,17 @@ impl ArchiveWriter { /// `root`, the canonical project directory. One that does is appended as /// what it points at, so a project may link to its own files freely; one /// that does not is refused, naming the link and where it leads, as is a - /// link that dangles or leads back to a directory already being copied. + /// link that dangles, leads back to a directory already being copied, or + /// reaches `output`, the bundle itself, which is being written meanwhile. /// Entries go in sorted order, so the same tree gives the same archive. fn dir( &mut self, src_path: &Path, archive_prefix: &str, root: &Path, + output: &Path, ) -> Result<(), BundleError> { - self.walk(src_path, archive_prefix, root, &mut Vec::new()) + self.walk(src_path, archive_prefix, root, output, &mut Vec::new()) } /// `open` holds the canonical directories on the way down from `dir`'s @@ -1531,6 +1548,7 @@ impl ArchiveWriter { src_path: &Path, archive_path: &str, root: &Path, + output: &Path, open: &mut Vec, ) -> Result<(), BundleError> { let real = resolve_within_root(src_path, root)?; @@ -1547,17 +1565,26 @@ impl ArchiveWriter { path: PathBuf::from(archive_path), })?; open.push(real.clone()); - for entry in crate::fs::read_dir(&real)? { - let Some(name) = entry.file_name() else { + // Listed where the directory really is, but each entry is named by the + // path it was reached by, which is what a message about it should say. + for listed in list_dir(&real)? { + let Some(name) = listed.file_name() else { continue; }; + let entry = src_path.join(name); let entry_archive_path = format!("{archive_path}/{name}"); let target = resolve_within_root(&entry, root)?; let meta = std::fs::metadata(&target).context(ResolveArchiveEntrySnafu { path: entry.clone(), })?; if meta.is_dir() { - self.walk(&entry, &entry_archive_path, root, open)?; + self.walk(&entry, &entry_archive_path, root, output, open)?; + } else if target == output { + return OutputReachedThroughLinkSnafu { + path: entry, + output: output.to_path_buf(), + } + .fail(); } else if meta.is_file() { self.builder .append_path_with_name(target.as_std_path(), &entry_archive_path) @@ -1581,6 +1608,7 @@ async fn write_archive( files: &dyn FileSystem, output: &Path, canonical_project_dir: &Path, + canonical_output: &Path, manifests: &[InstanceManifest], artifacts: &BundleArtifacts, args_files: &[ArgsFile], @@ -1654,7 +1682,12 @@ async fn write_archive( } for d in &artifacts.plugin_dirs { - archive.dir(&d.src_path, &d.archive_prefix, canonical_project_dir)?; + archive.dir( + &d.src_path, + &d.archive_prefix, + canonical_project_dir, + canonical_output, + )?; } for pf in &artifacts.plugin_files { @@ -1857,11 +1890,14 @@ fn resolve_within_project( /// Refuse to write the bundle output into a directory we are about to recursively archive — /// otherwise the partial bundle file would be included in itself. +/// Refuse an output path inside a directory the bundle copies, and hand back +/// its canonical form for the walk, which can reach the output through a +/// symlink that this lexical check cannot see. async fn validate_output_path( files: &dyn FileSystem, output: &Path, canonical_sync_dirs: &[PathBuf], -) -> Result<(), BundleError> { +) -> Result { let canonical_output = canonicalize_output(files, output).await?; for sync_dir in canonical_sync_dirs { if canonical_output.starts_with(sync_dir) { @@ -1872,7 +1908,7 @@ async fn validate_output_path( .fail(); } } - Ok(()) + Ok(canonical_output) } fn validate_network_for_bundle(net: &NetworkManifest) -> Result<(), BundleError> { @@ -1916,6 +1952,23 @@ fn is_absolute_bind_mount_host(mount: &str) -> bool { !h.is_empty() && (h[0] == b'/' || h[0] == b'\\') } +/// A directory's entries as full paths, sorted. [`crate::fs::read_dir`] skips a +/// name that is not UTF-8, which for a bundle would mean a file silently left +/// out; here it is refused instead, since the archive cannot name it. +fn list_dir(dir: &Path) -> Result, BundleError> { + let mut out = Vec::new(); + for entry in std::fs::read_dir(dir).context(ListDirSnafu { path: dir })? { + let entry = entry.context(ListDirSnafu { path: dir })?; + let path = PathBuf::from_path_buf(entry.path()).map_err(|_| BundleError::NameNotUtf8 { + dir: dir.to_path_buf(), + name: entry.file_name().to_string_lossy().into_owned(), + })?; + out.push(path); + } + out.sort(); + Ok(out) +} + /// Where `path` comes to rest with every symlink followed, which must be under /// `root`. Walks the host directly, as [`ArchiveWriter::dir`] does: it is the /// one place bundling has to know whether a path is a link, and the @@ -1924,7 +1977,11 @@ fn resolve_within_root(path: &Path, root: &Path) -> Result let real = match dunce::canonicalize(path) { Ok(real) => real, Err(source) => { - if let Ok(target) = std::fs::read_link(path) { + // A link whose target is missing is dangling; a link that loops, + // or leads somewhere unreadable, fails for its own reason, kept. + if source.kind() == std::io::ErrorKind::NotFound + && let Ok(target) = std::fs::read_link(path) + { return DanglingLinkSnafu { path: path.to_path_buf(), target: target.display().to_string(), From ac2fee43c33f7b7837d20f15244d3b5380185c0f Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Thu, 5 Nov 2026 19:57:39 +0000 Subject: [PATCH 3/6] docs(bundle): keep the Intel macOS chore out of Experimental, drop a stale doc comment The new `## Experimental` heading sat above the existing Intel macOS chore entry, which made that general change read as experimental. The heading and the bundle entry now follow it, as in v1.8.0. `validate_output_path` kept its old doc comment above the new one. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 4 ++-- crates/icp-project/src/operations/bundle.rs | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63fd255bf..b8a91fdd2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,12 +8,12 @@ air-gapped signing, extensions # Unreleased +* 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 * fix(bundle): `icp project bundle` holds only regular files and directories. A symlink inside a plugin's `files:` or `dirs:` directory used to be recorded as a link with its target unchecked, so a link to a file outside the project reached that file on whatever machine unpacked the bundle. A link to somewhere inside the project is now copied as what it points at, so a project may link to its own files freely; one that leads outside the project, dangles, or leads back to a directory being copied fails the bundle, naming the link and where it leads. A `files:` entry that is itself a symlink to a file outside the project is refused the same way, where it used to be read into the bundle. -* 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. - # v1.8.0 * fix: `icp sync` no longer needs controller rights. Its check that each canister is running used `canister_status`, which only a controller or a listed status viewer may read, so an identity the canister merely authorizes (a developer preparing an SNS-owned frontend for a commit proposal, say) was refused before anything was uploaded. It now probes with a query, as `icp deploy` already does after starting a canister. A stopped canister is still refused with the same advice, after the probe's 30-second wait rather than at once. diff --git a/crates/icp-project/src/operations/bundle.rs b/crates/icp-project/src/operations/bundle.rs index e7cec57f7..24b7827f4 100644 --- a/crates/icp-project/src/operations/bundle.rs +++ b/crates/icp-project/src/operations/bundle.rs @@ -1888,8 +1888,6 @@ fn resolve_within_project( Ok(resolved) } -/// Refuse to write the bundle output into a directory we are about to recursively archive — -/// otherwise the partial bundle file would be included in itself. /// Refuse an output path inside a directory the bundle copies, and hand back /// its canonical form for the walk, which can reach the output through a /// symlink that this lexical check cannot see. From 6c698af5cd7248c2a9a1ea8b51a934a33af20d7c Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Tue, 10 Nov 2026 01:03:56 +0000 Subject: [PATCH 4/6] refactor(bundle): resolve each directory once on the walk, and list names The walk resolved a directory as an entry and again on entering it; the resolved path is now handed down. list_dir returns names rather than paths the caller took the names back out of. Doc comments and the changelog entry say the same in fewer words. Co-Authored-By: Claude --- CHANGELOG.md | 2 +- crates/icp-project/src/operations/bundle.rs | 74 +++++++++++---------- 2 files changed, 39 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b8a91fdd2..bd34d7523 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ air-gapped signing, extensions ## Experimental -* fix(bundle): `icp project bundle` holds only regular files and directories. A symlink inside a plugin's `files:` or `dirs:` directory used to be recorded as a link with its target unchecked, so a link to a file outside the project reached that file on whatever machine unpacked the bundle. A link to somewhere inside the project is now copied as what it points at, so a project may link to its own files freely; one that leads outside the project, dangles, or leads back to a directory being copied fails the bundle, naming the link and where it leads. A `files:` entry that is itself a symlink to a file outside the project is refused the same way, where it used to be read into the bundle. +* fix(bundle): `icp project bundle` holds only regular files and directories. A symlink inside a plugin's `files:` or `dirs:` directory used to be recorded as a link with its target unchecked, so one pointing outside the project reached that file on whatever machine unpacked the bundle. A link to somewhere inside the project is now copied as what it points at; one that leads outside the project, dangles, or leads back to a directory being copied fails the bundle, naming the link and where it leads. The same holds for a `files:` entry that is itself a symlink. # v1.8.0 diff --git a/crates/icp-project/src/operations/bundle.rs b/crates/icp-project/src/operations/bundle.rs index 24b7827f4..f8bfed73d 100644 --- a/crates/icp-project/src/operations/bundle.rs +++ b/crates/icp-project/src/operations/bundle.rs @@ -1517,19 +1517,13 @@ impl ArchiveWriter { } /// Appends a directory's whole contents, walking the host filesystem rather - /// than the [`FileSystem`] seam the rest of bundling reads through: the seam - /// reports a link's target as the file it points at and has no way to say - /// otherwise, and a bundle holds only regular files and directories. - /// Bundling from anything but a host filesystem needs the seam to describe - /// links first. + /// than the [`FileSystem`] seam the rest of bundling reads through, which + /// cannot say whether a path is a link. /// - /// Every entry met is resolved through its symlinks and must land under - /// `root`, the canonical project directory. One that does is appended as - /// what it points at, so a project may link to its own files freely; one - /// that does not is refused, naming the link and where it leads, as is a - /// link that dangles, leads back to a directory already being copied, or - /// reaches `output`, the bundle itself, which is being written meanwhile. - /// Entries go in sorted order, so the same tree gives the same archive. + /// Every entry is resolved through its symlinks and must land under `root`, + /// the canonical project directory: it is then appended as what it points + /// at, in sorted order. A link that leads outside, dangles, leads back to + /// a directory being copied, or reaches `output` is refused by name. fn dir( &mut self, src_path: &Path, @@ -1537,21 +1531,29 @@ impl ArchiveWriter { root: &Path, output: &Path, ) -> Result<(), BundleError> { - self.walk(src_path, archive_prefix, root, output, &mut Vec::new()) + let real = resolve_within_root(src_path, root)?; + self.walk( + src_path, + real, + archive_prefix, + root, + output, + &mut Vec::new(), + ) } - /// `open` holds the canonical directories on the way down from `dir`'s - /// start, which is how a link back to one of them is told from a second - /// link to a directory already copied and finished with. + /// `real` is where `src_path` resolves, and `open` the canonical + /// directories on the way down to it, which tells a link back to one of + /// them from a second link to a directory already copied. fn walk( &mut self, src_path: &Path, + real: PathBuf, archive_path: &str, root: &Path, output: &Path, open: &mut Vec, ) -> Result<(), BundleError> { - let real = resolve_within_root(src_path, root)?; if open.contains(&real) { return LinkCycleSnafu { path: src_path.to_path_buf(), @@ -1564,21 +1566,19 @@ impl ArchiveWriter { .context(WriteArchiveEntrySnafu { path: PathBuf::from(archive_path), })?; - open.push(real.clone()); // Listed where the directory really is, but each entry is named by the // path it was reached by, which is what a message about it should say. - for listed in list_dir(&real)? { - let Some(name) = listed.file_name() else { - continue; - }; - let entry = src_path.join(name); + let names = list_dir(&real)?; + open.push(real); + for name in names { + let entry = src_path.join(&name); let entry_archive_path = format!("{archive_path}/{name}"); let target = resolve_within_root(&entry, root)?; let meta = std::fs::metadata(&target).context(ResolveArchiveEntrySnafu { path: entry.clone(), })?; if meta.is_dir() { - self.walk(&entry, &entry_archive_path, root, output, open)?; + self.walk(&entry, target, &entry_archive_path, root, output, open)?; } else if target == output { return OutputReachedThroughLinkSnafu { path: entry, @@ -1950,21 +1950,23 @@ fn is_absolute_bind_mount_host(mount: &str) -> bool { !h.is_empty() && (h[0] == b'/' || h[0] == b'\\') } -/// A directory's entries as full paths, sorted. [`crate::fs::read_dir`] skips a -/// name that is not UTF-8, which for a bundle would mean a file silently left -/// out; here it is refused instead, since the archive cannot name it. -fn list_dir(dir: &Path) -> Result, BundleError> { - let mut out = Vec::new(); +/// The names in a directory, sorted. [`crate::fs::read_dir`] skips a name that +/// is not UTF-8, which would leave a file out of the bundle; here it is refused. +fn list_dir(dir: &Path) -> Result, BundleError> { + let mut names = Vec::new(); for entry in std::fs::read_dir(dir).context(ListDirSnafu { path: dir })? { let entry = entry.context(ListDirSnafu { path: dir })?; - let path = PathBuf::from_path_buf(entry.path()).map_err(|_| BundleError::NameNotUtf8 { - dir: dir.to_path_buf(), - name: entry.file_name().to_string_lossy().into_owned(), - })?; - out.push(path); + let name = entry + .file_name() + .into_string() + .map_err(|name| BundleError::NameNotUtf8 { + dir: dir.to_path_buf(), + name: name.to_string_lossy().into_owned(), + })?; + names.push(name); } - out.sort(); - Ok(out) + names.sort(); + Ok(names) } /// Where `path` comes to rest with every symlink followed, which must be under From 2e91364fc86160f1bea4eb92d2ad6efaa54eb45e Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Tue, 10 Nov 2026 02:33:19 +0000 Subject: [PATCH 5/6] fix(bundle): resolve the output once it exists, for the walk to refuse The canonical output was worked out before the build, when an output path that is a dangling symlink resolves to its own spelling. Creating the file follows the link, so the walk was told to refuse a path the bundle was not written to, and one linked into a copied directory was read into itself without end. The output is now resolved after it is created. Co-Authored-By: Claude --- crates/icp-cli/tests/bundle_tests.rs | 24 +++++++++++++++++++++ crates/icp-project/src/operations/bundle.rs | 19 ++++++++-------- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/crates/icp-cli/tests/bundle_tests.rs b/crates/icp-cli/tests/bundle_tests.rs index e9f5ae874..3b2bb89c6 100644 --- a/crates/icp-cli/tests/bundle_tests.rs +++ b/crates/icp-cli/tests/bundle_tests.rs @@ -1184,6 +1184,30 @@ fn bundle_rejects_output_reached_through_symlink() { ); } +/// The same when the output path is itself a dangling symlink into such a +/// directory: where the bundle is really written is known only once it is +/// created, and that is what the walk is told to refuse. +#[cfg(unix)] +#[test] +fn bundle_rejects_output_that_is_a_link_into_a_copied_directory() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "assets"); + + create_dir_all(&project_dir.join("assets")).expect("failed to create dir"); + create_dir_all(&project_dir.join("shared")).expect("failed to create dir"); + std::os::unix::fs::symlink("../shared", project_dir.join("assets/shared")) + .expect("failed to create symlink"); + std::os::unix::fs::symlink("shared/actual.tar.gz", project_dir.join("bundle.tar.gz")) + .expect("failed to create symlink"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "bundle.tar.gz"]) + .assert() + .failure() + .stderr(contains("assets/shared/actual.tar.gz").and(contains("own output file"))); +} + /// A file whose name is not UTF-8 cannot be named in the archive. The bundle /// says so rather than leaving the file out, as the glob machinery elsewhere /// does when it lists a directory. diff --git a/crates/icp-project/src/operations/bundle.rs b/crates/icp-project/src/operations/bundle.rs index f8bfed73d..df459c365 100644 --- a/crates/icp-project/src/operations/bundle.rs +++ b/crates/icp-project/src/operations/bundle.rs @@ -478,7 +478,7 @@ pub async fn create_bundle( let canonical_sync_dirs = validate_source_paths(project_dir, &canisters, &canonical_project_dir)?; validate_env_var_files(files, &canisters, &canonical_project_dir).await?; - let canonical_output = validate_output_path(files, output, &canonical_sync_dirs).await?; + validate_output_path(files, output, &canonical_sync_dirs).await?; build_many( canisters.clone(), @@ -558,7 +558,6 @@ pub async fn create_bundle( files, output, &canonical_project_dir, - &canonical_output, &manifests, &bundle_artifacts, &args_files, @@ -1608,7 +1607,6 @@ async fn write_archive( files: &dyn FileSystem, output: &Path, canonical_project_dir: &Path, - canonical_output: &Path, manifests: &[InstanceManifest], artifacts: &BundleArtifacts, args_files: &[ArgsFile], @@ -1646,6 +1644,10 @@ async fn write_archive( let file = File::create(output.as_std_path()).context(CreateOutputSnafu { path: output.to_path_buf(), })?; + // Resolved now that it exists, for the walk, which can reach the output + // through a symlink: before this it may itself have been a dangling link, + // whose target only creating it brought about. + let canonical_output = canonicalize(files, output).await?; let gz = GzEncoder::new(BufWriter::new(file), Compression::default()); let mut archive = ArchiveWriter::new(gz); @@ -1686,7 +1688,7 @@ async fn write_archive( &d.src_path, &d.archive_prefix, canonical_project_dir, - canonical_output, + &canonical_output, )?; } @@ -1888,14 +1890,13 @@ fn resolve_within_project( Ok(resolved) } -/// Refuse an output path inside a directory the bundle copies, and hand back -/// its canonical form for the walk, which can reach the output through a -/// symlink that this lexical check cannot see. +/// Refuse to write the bundle output into a directory we are about to recursively archive — +/// otherwise the partial bundle file would be included in itself. async fn validate_output_path( files: &dyn FileSystem, output: &Path, canonical_sync_dirs: &[PathBuf], -) -> Result { +) -> Result<(), BundleError> { let canonical_output = canonicalize_output(files, output).await?; for sync_dir in canonical_sync_dirs { if canonical_output.starts_with(sync_dir) { @@ -1906,7 +1907,7 @@ async fn validate_output_path( .fail(); } } - Ok(canonical_output) + Ok(()) } fn validate_network_for_bundle(net: &NetworkManifest) -> Result<(), BundleError> { From 8cbd12e2b5ec29c3eca65a0b6a5bc42607e227e9 Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Tue, 10 Nov 2026 14:43:48 +0000 Subject: [PATCH 6/6] fix(bundle): hold a files: entry to what the walk holds, and resolve from where a link led A files: entry that is itself a link to the output was read into the bundle half-written, and one that is a named pipe was read forever; both are now refused as they are on the walk. The walk resolved each entry by the path it was reached by, following every link crossed so far again, so a chain of directory links longer than the host follows in one path failed; each entry is now resolved from the directory's real path. Co-Authored-By: Claude --- crates/icp-cli/tests/bundle_tests.rs | 91 +++++++++++++++++++++ crates/icp-project/src/operations/bundle.rs | 41 +++++++--- 2 files changed, 121 insertions(+), 11 deletions(-) diff --git a/crates/icp-cli/tests/bundle_tests.rs b/crates/icp-cli/tests/bundle_tests.rs index 3b2bb89c6..31d71b4c5 100644 --- a/crates/icp-cli/tests/bundle_tests.rs +++ b/crates/icp-cli/tests/bundle_tests.rs @@ -1208,6 +1208,97 @@ fn bundle_rejects_output_that_is_a_link_into_a_copied_directory() { .stderr(contains("assets/shared/actual.tar.gz").and(contains("own output file"))); } +/// A `files:` entry that is itself a link to the output is refused like one +/// met on the walk, rather than read into the bundle half-written. +#[cfg(unix)] +#[test] +fn bundle_rejects_a_files_entry_that_links_to_the_output() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "config.txt"); + + std::os::unix::fs::symlink("bundle.tar.gz", project_dir.join("config.txt")) + .expect("failed to create symlink"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "bundle.tar.gz"]) + .assert() + .failure() + .stderr(contains("config.txt").and(contains("own output file"))); +} + +/// Nor is a `files:` entry that is not a regular file read: a named pipe +/// nobody writes to would hold the bundle up forever. +#[cfg(unix)] +#[test] +fn bundle_rejects_a_files_entry_that_is_not_a_regular_file() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "config.txt"); + + let made = std::process::Command::new("mkfifo") + .arg(project_dir.join("config.txt")) + .status() + .expect("mkfifo"); + assert!(made.success(), "mkfifo"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "bundle.tar.gz"]) + .timeout(std::time::Duration::from_secs(60)) + .assert() + .failure() + .stderr(contains("config.txt").and(contains("neither a file nor a directory"))); +} + +/// Each directory link is resolved from where the last one led, not from the +/// start again, so a chain of them longer than the host follows in one path +/// (40 on Linux) is still copied. +#[cfg(unix)] +#[test] +fn bundle_follows_a_long_chain_of_directory_links() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "data"); + + const LINKS: usize = 45; + create_dir_all(&project_dir.join("data")).expect("failed to create dir"); + for i in 0..=LINKS { + create_dir_all(&project_dir.join(format!("chain/d{i}"))).expect("failed to create dir"); + } + std::os::unix::fs::symlink("../chain/d0", project_dir.join("data/next")) + .expect("failed to create symlink"); + for i in 0..LINKS { + std::os::unix::fs::symlink( + format!("../d{}", i + 1), + project_dir.join(format!("chain/d{i}/next")), + ) + .expect("failed to create symlink"); + } + write_string( + &project_dir.join(format!("chain/d{LINKS}/leaf.txt")), + "leaf", + ) + .expect("failed to write file"); + + let bundle_path = project_dir.join("bundle.tar.gz"); + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", bundle_path.as_str()]) + .assert() + .success(); + + let leaf = format!( + "plugins/my-canister/0/files/data/{}leaf.txt", + "next/".repeat(LINKS + 1) + ); + let entries = archive_entries(&bundle_path); + assert!( + entries + .iter() + .any(|(path, _, data)| *path == leaf && data == b"leaf"), + "{leaf} missing" + ); +} + /// A file whose name is not UTF-8 cannot be named in the archive. The bundle /// says so rather than leaving the file out, as the glob machinery elsewhere /// does when it lists a directory. diff --git a/crates/icp-project/src/operations/bundle.rs b/crates/icp-project/src/operations/bundle.rs index df459c365..af18a8f5a 100644 --- a/crates/icp-project/src/operations/bundle.rs +++ b/crates/icp-project/src/operations/bundle.rs @@ -1530,7 +1530,7 @@ impl ArchiveWriter { root: &Path, output: &Path, ) -> Result<(), BundleError> { - let real = resolve_within_root(src_path, root)?; + let real = resolve_within_root(src_path, src_path, root)?; self.walk( src_path, real, @@ -1565,14 +1565,17 @@ impl ArchiveWriter { .context(WriteArchiveEntrySnafu { path: PathBuf::from(archive_path), })?; - // Listed where the directory really is, but each entry is named by the - // path it was reached by, which is what a message about it should say. + // Listed and resolved from where the directory really is, so the links + // crossed to get here are not followed again for every entry — a long + // enough chain of them is more than the host follows in one path. Each + // entry is still named by the path it was reached by, which is what a + // message about it should say. let names = list_dir(&real)?; - open.push(real); + open.push(real.clone()); for name in names { let entry = src_path.join(&name); let entry_archive_path = format!("{archive_path}/{name}"); - let target = resolve_within_root(&entry, root)?; + let target = resolve_within_root(&real.join(&name), &entry, root)?; let meta = std::fs::metadata(&target).context(ResolveArchiveEntrySnafu { path: entry.clone(), })?; @@ -1703,6 +1706,21 @@ async fn write_archive( &pf.canister_name, ) .await?; + // Held to what the walk holds an entry to: not the bundle itself, and + // a regular file, since a pipe nobody writes to is read forever. + if src == canonical_output { + return OutputReachedThroughLinkSnafu { + path: pf.src_path.clone(), + output: canonical_output, + } + .fail(); + } + if !files.is_file(&src).await { + return UnsupportedEntrySnafu { + path: pf.src_path.clone(), + } + .fail(); + } let data = files.read(&src).await.context(ReadPluginFileSnafu { canister: pf.canister_name.clone(), file: pf.orig_file.clone(), @@ -1971,10 +1989,11 @@ fn list_dir(dir: &Path) -> Result, BundleError> { } /// Where `path` comes to rest with every symlink followed, which must be under -/// `root`. Walks the host directly, as [`ArchiveWriter::dir`] does: it is the +/// `root`. `named` is the path a message about it gives: the one it was reached +/// by. Walks the host directly, as [`ArchiveWriter::dir`] does: it is the /// one place bundling has to know whether a path is a link, and the /// [`FileSystem`] seam cannot say. -fn resolve_within_root(path: &Path, root: &Path) -> Result { +fn resolve_within_root(path: &Path, named: &Path, root: &Path) -> Result { let real = match dunce::canonicalize(path) { Ok(real) => real, Err(source) => { @@ -1984,23 +2003,23 @@ fn resolve_within_root(path: &Path, root: &Path) -> Result && let Ok(target) = std::fs::read_link(path) { return DanglingLinkSnafu { - path: path.to_path_buf(), + path: named.to_path_buf(), target: target.display().to_string(), } .fail(); } return Err(BundleError::ResolveArchiveEntry { - path: path.to_path_buf(), + path: named.to_path_buf(), source, }); } }; let real = PathBuf::from_path_buf(real).map_err(|_| BundleError::CanonicalizePath { - path: path.to_path_buf(), + path: named.to_path_buf(), })?; if !real.starts_with(root) { return LinkEscapesProjectSnafu { - path: path.to_path_buf(), + path: named.to_path_buf(), target: real, root: root.to_path_buf(), }