From 7420a2b33740a915d9a2cb68313288d1ca3e4bb2 Mon Sep 17 00:00:00 2001 From: Linwei Shang Date: Thu, 25 Jun 2026 14:37:24 -0400 Subject: [PATCH 1/6] feat(sync-plugin): forbid symlinks in plugin dirs/files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Declared `dirs`/`files` entries are resolved on the host before the WASI sandbox boundary, so the lexical "relative, no `..`" check was not enough: an entry that is a symlink — or traverses a symlinked parent component — would let a preopen or a host read resolve to a target outside the canister directory. Add `first_symlink_component`, a shared helper that walks each component of a declared path under the base directory and rejects the entry if any prefix is a symlink. Apply it to `dirs` (runtime preopen) and `files` (host read), with new `SymlinkDir`/`SymlinkFile` error variants. Symlinks inside a preopen that escape it remain handled by cap-std at runtime. Symlinks are forbidden outright for now; the restriction can be relaxed later if a safe use case emerges. Update the concept/reference/design docs, which previously implied all symlink escapes were rejected. Tests: unit tests for the helper, a fixture-gated runtime test for the SymlinkDir wiring, and two end-to-end tests asserting deploy fails when a declared dir/file points outside the project via a symlink. Co-Authored-By: Claude Opus 4.8 (1M context) --- Cargo.lock | 1 + crates/icp-cli/tests/sync_tests.rs | 104 +++++++++++++++++++++++ crates/icp-sync-plugin/Cargo.toml | 3 + crates/icp-sync-plugin/DESIGN.md | 18 +++- crates/icp-sync-plugin/src/lib.rs | 2 + crates/icp-sync-plugin/src/path.rs | 111 +++++++++++++++++++++++++ crates/icp-sync-plugin/src/runtime.rs | 39 +++++++++ crates/icp/src/canister/sync/plugin.rs | 11 +++ docs/concepts/sync-plugins.md | 3 +- docs/reference/configuration.md | 2 + 10 files changed, 291 insertions(+), 3 deletions(-) create mode 100644 crates/icp-sync-plugin/src/path.rs diff --git a/Cargo.lock b/Cargo.lock index 01e1e5af..ab3e4ed5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3822,6 +3822,7 @@ dependencies = [ "async-trait", "bytes", "camino", + "camino-tempfile", "candid", "console 0.16.3", "hex", diff --git a/crates/icp-cli/tests/sync_tests.rs b/crates/icp-cli/tests/sync_tests.rs index c00087e1..a51361f4 100644 --- a/crates/icp-cli/tests/sync_tests.rs +++ b/crates/icp-cli/tests/sync_tests.rs @@ -465,6 +465,110 @@ async fn sync_plugin_registers_seed_data() { ); } +/// A `dirs:` entry that is a symlink (here pointing outside the project) is +/// rejected before the plugin runs, so a preopen cannot escape the canister +/// directory. Symlinks are forbidden outright for now — see +/// `crates/icp-sync-plugin/DESIGN.md`. +#[cfg(unix)] +#[tokio::test] +async fn sync_plugin_rejects_symlinked_dir() { + let ctx = TestContext::new(); + let project_dir = ctx.create_project_dir("icp"); + + let (canister_wasm, plugin_wasm) = build_sync_plugin_example(); + + // A real directory *outside* the project, and a symlink to it inside the + // project that the manifest declares as a `dirs:` entry. + let outside = ctx.home_path().join("outside-seed-data"); + create_dir_all(&outside).expect("failed to create outside dir"); + write_string(&outside.join("fruit-01.txt"), "apple").expect("failed to write fruit-01.txt"); + std::os::unix::fs::symlink(&outside, project_dir.join("seed-data")) + .expect("failed to create symlink"); + + let pm = formatdoc! {r#" + canisters: + - name: my-canister + build: + steps: + - type: script + command: cp '{canister_wasm}' "$ICP_WASM_OUTPUT_PATH" + sync: + steps: + - type: plugin + path: {plugin_wasm} + dirs: + - seed-data + + {NETWORK_RANDOM_PORT} + {ENVIRONMENT_RANDOM_PORT} + "#}; + write_string(&project_dir.join("icp.yaml"), &pm).expect("failed to write project manifest"); + + let _g = ctx.start_network_in(&project_dir, "random-network").await; + ctx.ping_until_healthy(&project_dir, "random-network"); + + clients::icp(&ctx, &project_dir, Some("random-environment".to_string())) + .mint_cycles(10 * TRILLION); + + ctx.icp() + .current_dir(&project_dir) + .args(["deploy", "--environment", "random-environment"]) + .assert() + .failure() + .stderr(contains("symlink").and(contains("seed-data"))); +} + +/// A `files:` entry that is a symlink (here pointing outside the project) is +/// rejected before the host reads it, so a read cannot escape the canister +/// directory. +#[cfg(unix)] +#[tokio::test] +async fn sync_plugin_rejects_symlinked_file() { + let ctx = TestContext::new(); + let project_dir = ctx.create_project_dir("icp"); + + let (canister_wasm, plugin_wasm) = build_sync_plugin_example(); + + // A real file *outside* the project, and a symlink to it inside the project + // that the manifest declares as a `files:` entry. + 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"); + + let pm = formatdoc! {r#" + canisters: + - name: my-canister + build: + steps: + - type: script + command: cp '{canister_wasm}' "$ICP_WASM_OUTPUT_PATH" + sync: + steps: + - type: plugin + path: {plugin_wasm} + files: + - config.txt + + {NETWORK_RANDOM_PORT} + {ENVIRONMENT_RANDOM_PORT} + "#}; + write_string(&project_dir.join("icp.yaml"), &pm).expect("failed to write project manifest"); + + let _g = ctx.start_network_in(&project_dir, "random-network").await; + ctx.ping_until_healthy(&project_dir, "random-network"); + + clients::icp(&ctx, &project_dir, Some("random-environment".to_string())) + .mint_cycles(10 * TRILLION); + + ctx.icp() + .current_dir(&project_dir) + .args(["deploy", "--environment", "random-environment"]) + .assert() + .failure() + .stderr(contains("symlink").and(contains("config.txt"))); +} + #[tokio::test] async fn sync_script_icp_env_vars() { let ctx = TestContext::new(); diff --git a/crates/icp-sync-plugin/Cargo.toml b/crates/icp-sync-plugin/Cargo.toml index 2f8ae325..216e9c76 100644 --- a/crates/icp-sync-plugin/Cargo.toml +++ b/crates/icp-sync-plugin/Cargo.toml @@ -23,5 +23,8 @@ wasmtime-wasi.workspace = true [build-dependencies] camino.workspace = true +[dev-dependencies] +camino-tempfile.workspace = true + [lints] workspace = true diff --git a/crates/icp-sync-plugin/DESIGN.md b/crates/icp-sync-plugin/DESIGN.md index 5f149e26..63c77916 100644 --- a/crates/icp-sync-plugin/DESIGN.md +++ b/crates/icp-sync-plugin/DESIGN.md @@ -79,6 +79,19 @@ each `dir` from `base_dir.join(dir)` and passes `files` inline in lines (see stdio capture below); `stdio`, when set, receives the rolling progress lines live. +### Declared-path safety (no symlinks) + +Declared `dirs`/`files` entries are resolved on the host *before* the WASI +sandbox boundary, so the lexical "relative, no `..`" check is not enough: a +declared entry that *is* a symlink — or that traverses a symlinked parent +component — would let a preopen or a host read resolve to a target outside the +canister directory. `first_symlink_component` (in `path.rs`, shared with the +CLI's `files` reader) walks each component of the declared path under +`base_dir` and rejects the entry if any prefix is a symlink. Symlinks are +forbidden outright for now; the restriction can be relaxed later if a safe use +case emerges. (Symlinks *inside* a preopen that escape it are a separate +concern, already rejected by the WASI sandbox — cap-std — at runtime.) + ### `HostState` and bindgen ```rust @@ -145,5 +158,6 @@ pub struct Adapter { ### `crates/icp/src/canister/sync/plugin.rs` Resolves the wasm (local read or remote HTTP fetch into the package cache), -verifies sha256, reads the inline `files` (rejecting absolute or `..` paths), -then calls `icp_sync_plugin::run_plugin(...)`. +verifies sha256, reads the inline `files` (rejecting absolute, `..`, or +symlinked paths via `first_symlink_component`), then calls +`icp_sync_plugin::run_plugin(...)`. diff --git a/crates/icp-sync-plugin/src/lib.rs b/crates/icp-sync-plugin/src/lib.rs index 6c78f715..cae2e484 100644 --- a/crates/icp-sync-plugin/src/lib.rs +++ b/crates/icp-sync-plugin/src/lib.rs @@ -1,3 +1,5 @@ +mod path; mod runtime; +pub use path::first_symlink_component; pub use runtime::{RunPluginError, run_plugin}; diff --git a/crates/icp-sync-plugin/src/path.rs b/crates/icp-sync-plugin/src/path.rs new file mode 100644 index 00000000..79db0d48 --- /dev/null +++ b/crates/icp-sync-plugin/src/path.rs @@ -0,0 +1,111 @@ +//! Path-safety helpers shared between the host runtime (`dirs` preopens) and +//! the CLI's plugin sync (`files` reads). + +use camino::{Utf8Component, Utf8Path, Utf8PathBuf}; + +/// Walks `rel` one component at a time under `base` and returns the first +/// prefix that is a symlink, if any. +/// +/// Declared `dirs`/`files` entries are resolved on the host *before* the WASI +/// sandbox boundary, so a symlinked entry — or an entry that traverses a +/// symlinked directory — would let a preopen or a host read escape `base` to an +/// arbitrary location on disk (the lexical "no `..`, not absolute" check does +/// not catch this). Rejecting any symlink in the declared portion keeps every +/// preopen and read anchored within `base`. Symlinks *inside* a preopen that +/// escape it are separately rejected by the WASI sandbox (cap-std) at runtime. +/// +/// `base` itself may be reached through symlinks (e.g. the project lives under +/// a symlinked path); only the declared relative portion is checked. +/// +/// `rel` is expected to be relative and free of `..` (callers validate that +/// lexically first); `.` components are ignored. Components that do not exist +/// are not symlinks, so a missing path returns `None` and the subsequent read +/// or preopen surfaces the not-found error. +pub fn first_symlink_component(base: &Utf8Path, rel: &str) -> Option { + let mut current = base.to_path_buf(); + for component in Utf8Path::new(rel).components() { + if let Utf8Component::Normal(name) = component { + current.push(name); + match std::fs::symlink_metadata(current.as_std_path()) { + Ok(meta) if meta.file_type().is_symlink() => return Some(current), + _ => {} + } + } + } + None +} + +#[cfg(all(test, unix))] +mod tests { + use super::*; + use std::os::unix::fs::symlink; + + use camino_tempfile::tempdir; + + #[test] + fn plain_relative_path_has_no_symlink() { + let tmp = tempdir().unwrap(); + let base = tmp.path(); + std::fs::create_dir_all(base.join("a/b")).unwrap(); + std::fs::write(base.join("a/b/file.txt"), b"hi").unwrap(); + + assert_eq!(first_symlink_component(base, "a/b"), None); + assert_eq!(first_symlink_component(base, "a/b/file.txt"), None); + } + + #[test] + fn final_entry_is_symlink() { + let tmp = tempdir().unwrap(); + let base = tmp.path(); + std::fs::create_dir_all(base.join("real")).unwrap(); + symlink(base.join("real"), base.join("link")).unwrap(); + + assert_eq!( + first_symlink_component(base, "link"), + Some(base.join("link")) + ); + } + + #[test] + fn intermediate_component_is_symlink() { + let tmp = tempdir().unwrap(); + let base = tmp.path(); + // base/real/inner exists; base/link -> base/real, so "link/inner" + // traverses a symlink even though "inner" itself is a real dir. + std::fs::create_dir_all(base.join("real/inner")).unwrap(); + symlink(base.join("real"), base.join("link")).unwrap(); + + assert_eq!( + first_symlink_component(base, "link/inner"), + Some(base.join("link")) + ); + } + + #[test] + fn missing_path_is_not_a_symlink() { + let tmp = tempdir().unwrap(); + let base = tmp.path(); + assert_eq!(first_symlink_component(base, "does/not/exist"), None); + } + + #[test] + fn dot_components_are_ignored() { + let tmp = tempdir().unwrap(); + let base = tmp.path(); + std::fs::create_dir_all(base.join("a")).unwrap(); + assert_eq!(first_symlink_component(base, "./a"), None); + } + + #[test] + fn symlinked_base_is_allowed() { + // A symlink *above* the declared portion (i.e. reaching `base`) is fine; + // only components of `rel` are checked. + let tmp = tempdir().unwrap(); + let real_base = tmp.path().join("real-base"); + std::fs::create_dir_all(real_base.join("data")).unwrap(); + let linked_base = tmp.path().join("linked-base"); + symlink(&real_base, &linked_base).unwrap(); + + assert_eq!(first_symlink_component(&linked_base, "data"), None); + } +} diff --git a/crates/icp-sync-plugin/src/runtime.rs b/crates/icp-sync-plugin/src/runtime.rs index 1b96d9e1..2c442196 100644 --- a/crates/icp-sync-plugin/src/runtime.rs +++ b/crates/icp-sync-plugin/src/runtime.rs @@ -146,6 +146,11 @@ pub enum RunPluginError { ))] UnsafeDir { dir: String }, + #[snafu(display( + "plugin dir '{dir}' resolves through a symlink ('{link}'); symlinks are not allowed in plugin dirs" + ))] + SymlinkDir { dir: String, link: Utf8PathBuf }, + #[snafu(display("failed to preopen directory '{dir}' for the plugin"))] PreopenDir { source: wasmtime::Error, @@ -231,6 +236,13 @@ pub fn run_plugin( !p.is_absolute() && !p.components().any(|c| c == Utf8Component::ParentDir), UnsafeDirSnafu { dir } ); + // Reject symlinks in the declared path: neither the final entry nor any + // intermediate component may be a symlink, so the preopen cannot escape + // `base_dir` to a target elsewhere on disk. (Symlinks *inside* a preopen + // that escape it are separately rejected by the WASI sandbox.) + if let Some(link) = crate::first_symlink_component(&base_dir, dir) { + return SymlinkDirSnafu { dir, link }.fail(); + } let host_path = base_dir.join(dir); wasi_builder .preopened_dir( @@ -528,6 +540,33 @@ mod tests { assert!(matches!(result, Err(RunPluginError::PreopenDir { .. }))); } + #[cfg(unix)] + #[test] + fn symlinked_dir_is_rejected() { + let Some(wasm_path) = option_env!("TEST_PLUGIN_WASM") else { + return; + }; + use std::os::unix::fs::symlink; + let tmp = camino_tempfile::tempdir().expect("create tempdir"); + let base = tmp.path(); + std::fs::create_dir_all(base.join("real")).expect("create real dir"); + symlink(base.join("real"), base.join("link")).expect("create symlink"); + + let result = run_plugin( + wasm_path.into(), + base.to_path_buf(), + vec!["link".to_string()], + vec![], + anon(), + dummy_agent(), + None, + anon(), + "test".to_string(), + None, + ); + assert!(matches!(result, Err(RunPluginError::SymlinkDir { .. }))); + } + #[test] fn plugin_success_returns_ok() { let Some(wasm_path) = option_env!("TEST_PLUGIN_WASM") else { diff --git a/crates/icp/src/canister/sync/plugin.rs b/crates/icp/src/canister/sync/plugin.rs index 33100b79..c61d28de 100644 --- a/crates/icp/src/canister/sync/plugin.rs +++ b/crates/icp/src/canister/sync/plugin.rs @@ -18,6 +18,11 @@ pub enum PluginError { ))] UnsafeFilePath { name: String }, + #[snafu(display( + "plugin file '{name}' resolves through a symlink ('{link}'); symlinks are not allowed in plugin files" + ))] + SymlinkFile { name: String, link: Utf8PathBuf }, + #[snafu(display("failed to read plugin input file at '{path}'"))] ReadFile { source: crate::fs::IoError, @@ -71,6 +76,12 @@ pub(super) async fn sync( .any(|c| c == camino::Utf8Component::ParentDir), UnsafeFilePathSnafu { name } ); + // Reject symlinks in the declared path: neither the final entry nor any + // intermediate component may be a symlink, so the host read cannot + // escape the canister directory to a file elsewhere on disk. + if let Some(link) = icp_sync_plugin::first_symlink_component(¶ms.path, name) { + return SymlinkFileSnafu { name, link }.fail(); + } let abs = params.path.join(name); let content = read_to_string(abs.as_ref()).context(ReadFileSnafu { path: abs })?; files.push((name.clone(), content)); diff --git a/docs/concepts/sync-plugins.md b/docs/concepts/sync-plugins.md index e4bfd17f..09e7bc36 100644 --- a/docs/concepts/sync-plugins.md +++ b/docs/concepts/sync-plugins.md @@ -98,7 +98,8 @@ The plugin runs with a deliberately narrow capability surface. - Each directory in `dirs:` is preopened **read-only**. The plugin sees it at the same relative path it used in the manifest (e.g. `dirs: ["assets"]` is visible as `assets/` inside the guest) and traverses it with standard filesystem APIs (`std::fs` in Rust). - Files in `files:` are read by the host up front and passed inline in `sync-exec-input.files`. The plugin reads their content from the input struct, not from disk. -- Any path outside a preopen is invisible. Writes, creates, deletes, renames, and symlinks that escape a preopen are rejected. Paths in `dirs:`/`files:` must be relative and may not contain `..`. +- Any path outside a preopen is invisible. Writes, creates, deletes, renames, and symlinks that escape a preopen are rejected by the sandbox at runtime. +- Paths in `dirs:`/`files:` must be relative and may not contain `..`. They also may not be — or traverse — a symlink: each declared entry is rejected if it or any of its parent components is a symlink, so a declared path cannot resolve to a target outside the canister directory. (This restriction may be relaxed later if a safe use case emerges.) ### Capabilities diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index 8e41da9e..fa5ab514 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -165,6 +165,8 @@ sync: | `dirs` | array of string | No | Directories (relative to the canister directory) the plugin may read; each is preopened read-only via WASI | | `files` | array of string | No | Files (relative to the canister directory) read by the host and passed inline to the plugin | +Entries in `dirs:`/`files:` must be relative, may not contain `..`, and may not be — or traverse — a symlink, so a declared path cannot resolve to a target outside the canister directory. + The plugin runs in a WASI sandbox: it can call update and query methods on the canister being synced and read the declared `dirs`/`files`, but cannot open network sockets, spawn subprocesses, or write to disk. See [Sync Plugins](../concepts/sync-plugins.md) for the mechanism and [Writing a Sync Plugin](../guides/writing-sync-plugins.md) to author one. ## Recipes From 121c20e2cf276dd1eb0ca291a1ddad1bcc8d63b7 Mon Sep 17 00:00:00 2001 From: Linwei Shang Date: Thu, 25 Jun 2026 14:40:50 -0400 Subject: [PATCH 2/6] docs(changelog): note symlink restriction for sync-plugin dirs/files Recorded under the Unreleased > Experimental subsection, per the changelog convention for the (experimental) sync-plugin feature. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a4eac8c..1e9bf7f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ bump. Currently experimental: sync plugins. # Unreleased +## Experimental + +* feat(sync-plugin): `plugin` sync steps now reject any `dirs`/`files` entry that is, or traverses, a symlink. Together with the existing relative-path and `..` checks, this keeps a declared path from resolving to a target outside the canister directory. The restriction may be relaxed in a future release if a safe use case emerges. + # v1.0.1 * feat: `icp identity import` can now be used with a `--delegation` flag to import a delegated identity. This is most useful for containers or other internal-only delegations; for anything involving a network, `icp identity delegation request` remains the recommended way to work with delegations. From 48e5b4e4b11a98fb625c852a3db3cdd296b6fcb1 Mon Sep 17 00:00:00 2001 From: Linwei Shang Date: Thu, 25 Jun 2026 14:46:46 -0400 Subject: [PATCH 3/6] fix(sync-plugin): reject drive-relative paths; report relative symlink Address Copilot review on #627: - Harden the lexical check: a Windows drive-relative path like `C:foo` carries a `Prefix` component but `is_absolute()` returns false, so the old check let it through and `join` would discard the base directory. Replace the ad-hoc `is_absolute() + ..` check with a shared `escapes_base` that rejects `ParentDir`, `RootDir`, and `Prefix(_)`, mirroring the bundler's escape checks. Used for both `dirs` and `files`. - `first_symlink_component` now returns the offending sub-path relative to the base (e.g. `link` or `link/inner`) instead of the fully joined host path, so the error no longer leaks the absolute on-disk location. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/icp-sync-plugin/src/lib.rs | 2 +- crates/icp-sync-plugin/src/path.rs | 68 ++++++++++++++++++++------ crates/icp-sync-plugin/src/runtime.rs | 8 +-- crates/icp/src/canister/sync/plugin.rs | 6 +-- 4 files changed, 57 insertions(+), 27 deletions(-) diff --git a/crates/icp-sync-plugin/src/lib.rs b/crates/icp-sync-plugin/src/lib.rs index cae2e484..4326b5c0 100644 --- a/crates/icp-sync-plugin/src/lib.rs +++ b/crates/icp-sync-plugin/src/lib.rs @@ -1,5 +1,5 @@ mod path; mod runtime; -pub use path::first_symlink_component; +pub use path::{escapes_base, first_symlink_component}; pub use runtime::{RunPluginError, run_plugin}; diff --git a/crates/icp-sync-plugin/src/path.rs b/crates/icp-sync-plugin/src/path.rs index 79db0d48..0e9e8082 100644 --- a/crates/icp-sync-plugin/src/path.rs +++ b/crates/icp-sync-plugin/src/path.rs @@ -3,31 +3,55 @@ use camino::{Utf8Component, Utf8Path, Utf8PathBuf}; +/// Returns `true` if `rel` cannot be safely joined onto a base directory +/// because it contains a component that would escape it: `..`, a filesystem +/// root, or a (Windows) drive prefix such as `C:` — the latter makes a path +/// drive-relative even without a leading separator, so `is_absolute()` returns +/// `false` yet joining it discards the base. Mirrors the escape checks in the +/// bundler (`crates/icp-cli/src/operations/bundle.rs`). +/// +/// Callers reject such paths before resolving them; `first_symlink_component` +/// only inspects `Normal` components and so would not otherwise catch these. +pub fn escapes_base(rel: &str) -> bool { + Utf8Path::new(rel).components().any(|c| { + matches!( + c, + Utf8Component::ParentDir | Utf8Component::RootDir | Utf8Component::Prefix(_) + ) + }) +} + /// Walks `rel` one component at a time under `base` and returns the first -/// prefix that is a symlink, if any. +/// sub-path of `rel` (relative to `base`) that is a symlink, if any. /// /// Declared `dirs`/`files` entries are resolved on the host *before* the WASI /// sandbox boundary, so a symlinked entry — or an entry that traverses a /// symlinked directory — would let a preopen or a host read escape `base` to an -/// arbitrary location on disk (the lexical "no `..`, not absolute" check does -/// not catch this). Rejecting any symlink in the declared portion keeps every -/// preopen and read anchored within `base`. Symlinks *inside* a preopen that -/// escape it are separately rejected by the WASI sandbox (cap-std) at runtime. +/// arbitrary location on disk (the lexical [`escapes_base`] check does not catch +/// this). Rejecting any symlink in the declared portion keeps every preopen and +/// read anchored within `base`. Symlinks *inside* a preopen that escape it are +/// separately rejected by the WASI sandbox (cap-std) at runtime. +/// +/// The returned path is relative to `base` (e.g. `link` or `link/inner`), +/// matching what the user wrote in the manifest, so it can be surfaced in an +/// error without leaking the absolute on-disk location. /// /// `base` itself may be reached through symlinks (e.g. the project lives under /// a symlinked path); only the declared relative portion is checked. /// -/// `rel` is expected to be relative and free of `..` (callers validate that -/// lexically first); `.` components are ignored. Components that do not exist -/// are not symlinks, so a missing path returns `None` and the subsequent read -/// or preopen surfaces the not-found error. +/// `rel` is expected to be relative and free of `..` (callers validate that via +/// [`escapes_base`] first); `.` components are ignored. Components that do not +/// exist are not symlinks, so a missing path returns `None` and the subsequent +/// read or preopen surfaces the not-found error. pub fn first_symlink_component(base: &Utf8Path, rel: &str) -> Option { - let mut current = base.to_path_buf(); + let mut host = base.to_path_buf(); + let mut relative = Utf8PathBuf::new(); for component in Utf8Path::new(rel).components() { if let Utf8Component::Normal(name) = component { - current.push(name); - match std::fs::symlink_metadata(current.as_std_path()) { - Ok(meta) if meta.file_type().is_symlink() => return Some(current), + host.push(name); + relative.push(name); + match std::fs::symlink_metadata(host.as_std_path()) { + Ok(meta) if meta.file_type().is_symlink() => return Some(relative), _ => {} } } @@ -42,6 +66,18 @@ mod tests { use camino_tempfile::tempdir; + #[test] + fn escapes_base_flags_unsafe_components() { + assert!(!escapes_base("a/b")); + assert!(!escapes_base("./a")); + assert!(escapes_base("../a")); + assert!(escapes_base("a/../b")); + // Absolute paths carry a `RootDir` component on this (Unix) host. The + // Windows drive-prefix case (`C:foo`) only parses as `Prefix` on + // Windows and so cannot be exercised here. + assert!(escapes_base("/abs")); + } + #[test] fn plain_relative_path_has_no_symlink() { let tmp = tempdir().unwrap(); @@ -62,7 +98,7 @@ mod tests { assert_eq!( first_symlink_component(base, "link"), - Some(base.join("link")) + Some(Utf8PathBuf::from("link")) ); } @@ -75,9 +111,11 @@ mod tests { std::fs::create_dir_all(base.join("real/inner")).unwrap(); symlink(base.join("real"), base.join("link")).unwrap(); + // The reported path is the offending sub-path relative to `base`, + // i.e. the symlinked component, not the trailing real directory. assert_eq!( first_symlink_component(base, "link/inner"), - Some(base.join("link")) + Some(Utf8PathBuf::from("link")) ); } diff --git a/crates/icp-sync-plugin/src/runtime.rs b/crates/icp-sync-plugin/src/runtime.rs index 2c442196..7acd3596 100644 --- a/crates/icp-sync-plugin/src/runtime.rs +++ b/crates/icp-sync-plugin/src/runtime.rs @@ -13,7 +13,7 @@ const MAX_WASM_STACK: usize = 512 * 1024; const PLUGIN_COMPUTE_LIMIT_SECS: u64 = 60; use bytes::Bytes; -use camino::{Utf8Component, Utf8PathBuf}; +use camino::Utf8PathBuf; use candid::{Encode, Principal}; use ic_agent::Agent; use snafu::prelude::*; @@ -231,11 +231,7 @@ pub fn run_plugin( // same relative path it used in the manifest. let mut wasi_builder = wasmtime_wasi::WasiCtxBuilder::new(); for dir in &dirs { - let p = Utf8PathBuf::from(dir); - ensure!( - !p.is_absolute() && !p.components().any(|c| c == Utf8Component::ParentDir), - UnsafeDirSnafu { dir } - ); + ensure!(!crate::escapes_base(dir), UnsafeDirSnafu { dir }); // Reject symlinks in the declared path: neither the final entry nor any // intermediate component may be a symlink, so the preopen cannot escape // `base_dir` to a target elsewhere on disk. (Symlinks *inside* a preopen diff --git a/crates/icp/src/canister/sync/plugin.rs b/crates/icp/src/canister/sync/plugin.rs index c61d28de..2986c9ff 100644 --- a/crates/icp/src/canister/sync/plugin.rs +++ b/crates/icp/src/canister/sync/plugin.rs @@ -68,12 +68,8 @@ pub(super) async fn sync( let mut files: Vec<(String, String)> = Vec::new(); for name in adapter.files.as_deref().unwrap_or(&[]) { - let p = Utf8PathBuf::from(name); ensure!( - !p.is_absolute() - && !p - .components() - .any(|c| c == camino::Utf8Component::ParentDir), + !icp_sync_plugin::escapes_base(name), UnsafeFilePathSnafu { name } ); // Reject symlinks in the declared path: neither the final entry nor any From 7d56cafcfb5a1a5407c9286a94197af7d51d40cb Mon Sep 17 00:00:00 2001 From: Linwei Shang Date: Thu, 25 Jun 2026 14:55:35 -0400 Subject: [PATCH 4/6] refactor(sync-plugin): read input files inside run_plugin Move the host-side reading of declared `files` from the CLI's `sync/plugin.rs` into `run_plugin`, alongside the existing `dirs` preopen. `run_plugin` now takes `files: Vec` (manifest-relative names) instead of pre-read `(name, content)` pairs and opens them itself from `base_dir`. This keeps all filesystem access and path-safety logic in one place: the `escapes_base` and `first_symlink_component` helpers are now crate-private (no longer re-exported), and the file-path error variants (`UnsafeFile`, `SymlinkFile`, `ReadFile`) live on `RunPluginError`. The CLI just forwards the manifest's `dirs`/`files` strings. Behavior is unchanged: the same checks apply to `files` as before, now surfaced via `PluginError::Run`. Add runtime-level tests for the moved logic (missing file -> ReadFile, symlinked file -> SymlinkFile) and update DESIGN.md. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/icp-sync-plugin/DESIGN.md | 45 ++++++++----- crates/icp-sync-plugin/src/lib.rs | 1 - crates/icp-sync-plugin/src/path.rs | 4 +- crates/icp-sync-plugin/src/runtime.rs | 92 ++++++++++++++++++++++++-- crates/icp/src/canister/sync/plugin.rs | 44 ++---------- 5 files changed, 120 insertions(+), 66 deletions(-) diff --git a/crates/icp-sync-plugin/DESIGN.md b/crates/icp-sync-plugin/DESIGN.md index 63c77916..bcf5ebab 100644 --- a/crates/icp-sync-plugin/DESIGN.md +++ b/crates/icp-sync-plugin/DESIGN.md @@ -63,7 +63,7 @@ pub fn run_plugin( wasm_path: Utf8PathBuf, base_dir: Utf8PathBuf, dirs: Vec, - files: Vec<(String, String)>, + files: Vec, target_canister_id: Principal, agent: Agent, proxy: Option, @@ -73,24 +73,32 @@ pub fn run_plugin( ) -> Result, RunPluginError> ``` -`dirs` and `files` come directly from the manifest adapter. The runtime preopens -each `dir` from `base_dir.join(dir)` and passes `files` inline in -`SyncExecInput`. The returned `Vec` is the plugin's persistent stderr -lines (see stdio capture below); `stdio`, when set, receives the rolling -progress lines live. +`dirs` and `files` are the manifest-relative path strings, straight from the +adapter. The runtime owns *all* filesystem access anchored at `base_dir`: it +preopens each `dir` from `base_dir.join(dir)` and reads each `file` from +`base_dir.join(file)`, passing the contents inline in `SyncExecInput`. Keeping +both inside the runtime means the path-safety logic (below) lives in one place +and stays private to this crate — the CLI just forwards strings. The returned +`Vec` is the plugin's persistent stderr lines (see stdio capture below); +`stdio`, when set, receives the rolling progress lines live. ### Declared-path safety (no symlinks) Declared `dirs`/`files` entries are resolved on the host *before* the WASI -sandbox boundary, so the lexical "relative, no `..`" check is not enough: a -declared entry that *is* a symlink — or that traverses a symlinked parent -component — would let a preopen or a host read resolve to a target outside the -canister directory. `first_symlink_component` (in `path.rs`, shared with the -CLI's `files` reader) walks each component of the declared path under -`base_dir` and rejects the entry if any prefix is a symlink. Symlinks are -forbidden outright for now; the restriction can be relaxed later if a safe use -case emerges. (Symlinks *inside* a preopen that escape it are a separate -concern, already rejected by the WASI sandbox — cap-std — at runtime.) +sandbox boundary, so a lexical "relative, no `..`" check is not enough on two +counts. First, a Windows drive-relative path such as `C:foo` carries a `Prefix` +component yet is not "absolute", so joining it would discard `base_dir`; +`escapes_base` (in `path.rs`) rejects `..`, root, and drive-prefix components, +mirroring the bundler's checks. Second, a declared entry that *is* a symlink — +or that traverses a symlinked parent component — would let a preopen or a read +resolve outside the canister directory; `first_symlink_component` walks each +component of the declared path under `base_dir` and rejects the entry if any +prefix is a symlink (returning the offending sub-path relative to `base_dir`, so +errors don't leak absolute on-disk paths). Both helpers are crate-private and +applied uniformly to `dirs` and `files`. Symlinks are forbidden outright for +now; the restriction can be relaxed later if a safe use case emerges. (Symlinks +*inside* a preopen that escape it are a separate concern, already rejected by +the WASI sandbox — cap-std — at runtime.) ### `HostState` and bindgen @@ -158,6 +166,7 @@ pub struct Adapter { ### `crates/icp/src/canister/sync/plugin.rs` Resolves the wasm (local read or remote HTTP fetch into the package cache), -verifies sha256, reads the inline `files` (rejecting absolute, `..`, or -symlinked paths via `first_symlink_component`), then calls -`icp_sync_plugin::run_plugin(...)`. +verifies sha256, then calls `icp_sync_plugin::run_plugin(...)`, forwarding the +manifest's `dirs`/`files` strings unchanged. The runtime — not the CLI — opens +those paths and enforces the path-safety checks, so the CLI no longer touches +the plugin's input files itself. diff --git a/crates/icp-sync-plugin/src/lib.rs b/crates/icp-sync-plugin/src/lib.rs index 4326b5c0..cdeee3e2 100644 --- a/crates/icp-sync-plugin/src/lib.rs +++ b/crates/icp-sync-plugin/src/lib.rs @@ -1,5 +1,4 @@ mod path; mod runtime; -pub use path::{escapes_base, first_symlink_component}; pub use runtime::{RunPluginError, run_plugin}; diff --git a/crates/icp-sync-plugin/src/path.rs b/crates/icp-sync-plugin/src/path.rs index 0e9e8082..289af290 100644 --- a/crates/icp-sync-plugin/src/path.rs +++ b/crates/icp-sync-plugin/src/path.rs @@ -12,7 +12,7 @@ use camino::{Utf8Component, Utf8Path, Utf8PathBuf}; /// /// Callers reject such paths before resolving them; `first_symlink_component` /// only inspects `Normal` components and so would not otherwise catch these. -pub fn escapes_base(rel: &str) -> bool { +pub(crate) fn escapes_base(rel: &str) -> bool { Utf8Path::new(rel).components().any(|c| { matches!( c, @@ -43,7 +43,7 @@ pub fn escapes_base(rel: &str) -> bool { /// [`escapes_base`] first); `.` components are ignored. Components that do not /// exist are not symlinks, so a missing path returns `None` and the subsequent /// read or preopen surfaces the not-found error. -pub fn first_symlink_component(base: &Utf8Path, rel: &str) -> Option { +pub(crate) fn first_symlink_component(base: &Utf8Path, rel: &str) -> Option { let mut host = base.to_path_buf(); let mut relative = Utf8PathBuf::new(); for component in Utf8Path::new(rel).components() { diff --git a/crates/icp-sync-plugin/src/runtime.rs b/crates/icp-sync-plugin/src/runtime.rs index 7acd3596..947c21c6 100644 --- a/crates/icp-sync-plugin/src/runtime.rs +++ b/crates/icp-sync-plugin/src/runtime.rs @@ -157,6 +157,22 @@ pub enum RunPluginError { dir: Utf8PathBuf, }, + #[snafu(display( + "plugin file '{name}' is not a safe relative path (no absolute paths or '..' allowed)" + ))] + UnsafeFile { name: String }, + + #[snafu(display( + "plugin file '{name}' resolves through a symlink ('{link}'); symlinks are not allowed in plugin files" + ))] + SymlinkFile { name: String, link: Utf8PathBuf }, + + #[snafu(display("failed to read plugin input file at {path}"))] + ReadFile { + source: std::io::Error, + path: Utf8PathBuf, + }, + #[snafu(display("failed to instantiate wasm component at {path}"))] Instantiate { source: wasmtime::Error, @@ -177,7 +193,7 @@ pub fn run_plugin( wasm_path: Utf8PathBuf, base_dir: Utf8PathBuf, dirs: Vec, - files: Vec<(String, String)>, + files: Vec, target_canister_id: Principal, agent: Agent, proxy: Option, @@ -231,12 +247,12 @@ pub fn run_plugin( // same relative path it used in the manifest. let mut wasi_builder = wasmtime_wasi::WasiCtxBuilder::new(); for dir in &dirs { - ensure!(!crate::escapes_base(dir), UnsafeDirSnafu { dir }); + ensure!(!crate::path::escapes_base(dir), UnsafeDirSnafu { dir }); // Reject symlinks in the declared path: neither the final entry nor any // intermediate component may be a symlink, so the preopen cannot escape // `base_dir` to a target elsewhere on disk. (Symlinks *inside* a preopen // that escape it are separately rejected by the WASI sandbox.) - if let Some(link) = crate::first_symlink_component(&base_dir, dir) { + if let Some(link) = crate::path::first_symlink_component(&base_dir, dir) { return SymlinkDirSnafu { dir, link }.fail(); } let host_path = base_dir.join(dir); @@ -250,6 +266,24 @@ pub fn run_plugin( .context(PreopenDirSnafu { dir: host_path })?; } + // Read each declared file on the host and pass its content inline. The same + // path-safety checks as `dirs` apply: reject escaping or symlinked paths so + // a read cannot leave `base_dir`. + let mut file_inputs: Vec = Vec::with_capacity(files.len()); + for name in &files { + ensure!(!crate::path::escapes_base(name), UnsafeFileSnafu { name }); + if let Some(link) = crate::path::first_symlink_component(&base_dir, name) { + return SymlinkFileSnafu { name, link }.fail(); + } + let path = base_dir.join(name); + let content = + std::fs::read_to_string(path.as_std_path()).context(ReadFileSnafu { path })?; + file_inputs.push(FileInput { + name: name.clone(), + content, + }); + } + let persistent_stderr: Arc>> = Arc::default(); let stdout_capture = LineCapture::new("stdout", stdio.clone(), None); let stderr_capture = LineCapture::new("stderr", stdio.clone(), Some(persistent_stderr.clone())); @@ -297,10 +331,7 @@ pub fn run_plugin( canister_id: target_canister_id.to_text(), environment, dirs, - files: files - .into_iter() - .map(|(name, content)| FileInput { name, content }) - .collect(), + files: file_inputs, identity_principal: identity_principal.to_text(), proxy_canister_id: proxy.map(|p| p.to_text()), }; @@ -563,6 +594,53 @@ mod tests { assert!(matches!(result, Err(RunPluginError::SymlinkDir { .. }))); } + #[test] + fn read_file_error_on_missing_file() { + let Some(wasm_path) = option_env!("TEST_PLUGIN_WASM") else { + return; + }; + let result = run_plugin( + wasm_path.into(), + ".".into(), + vec![], + vec!["nonexistent_file.txt".to_string()], + anon(), + dummy_agent(), + None, + anon(), + "test".to_string(), + None, + ); + assert!(matches!(result, Err(RunPluginError::ReadFile { .. }))); + } + + #[cfg(unix)] + #[test] + fn symlinked_file_is_rejected() { + let Some(wasm_path) = option_env!("TEST_PLUGIN_WASM") else { + return; + }; + use std::os::unix::fs::symlink; + let tmp = camino_tempfile::tempdir().expect("create tempdir"); + let base = tmp.path(); + std::fs::write(base.join("real.txt"), b"data").expect("write real file"); + symlink(base.join("real.txt"), base.join("link.txt")).expect("create symlink"); + + let result = run_plugin( + wasm_path.into(), + base.to_path_buf(), + vec![], + vec!["link.txt".to_string()], + anon(), + dummy_agent(), + None, + anon(), + "test".to_string(), + None, + ); + assert!(matches!(result, Err(RunPluginError::SymlinkFile { .. }))); + } + #[test] fn plugin_success_returns_ok() { let Some(wasm_path) = option_env!("TEST_PLUGIN_WASM") else { diff --git a/crates/icp/src/canister/sync/plugin.rs b/crates/icp/src/canister/sync/plugin.rs index 2986c9ff..be782001 100644 --- a/crates/icp/src/canister/sync/plugin.rs +++ b/crates/icp/src/canister/sync/plugin.rs @@ -5,30 +5,12 @@ use icp_sync_plugin::{RunPluginError, run_plugin}; use snafu::prelude::*; use tokio::sync::mpsc::Sender; -use crate::{ - canister::wasm, fs::read_to_string, manifest::adapter::plugin::Adapter, package::PackageCache, -}; +use crate::{canister::wasm, manifest::adapter::plugin::Adapter, package::PackageCache}; use super::Params; #[derive(Debug, Snafu)] pub enum PluginError { - #[snafu(display( - "plugin file path '{name}' is not a safe relative path (no absolute paths or '..' allowed)" - ))] - UnsafeFilePath { name: String }, - - #[snafu(display( - "plugin file '{name}' resolves through a symlink ('{link}'); symlinks are not allowed in plugin files" - ))] - SymlinkFile { name: String, link: Utf8PathBuf }, - - #[snafu(display("failed to read plugin input file at '{path}'"))] - ReadFile { - source: crate::fs::IoError, - path: Utf8PathBuf, - }, - #[snafu(transparent)] Wasm { source: wasm::WasmError }, @@ -61,27 +43,13 @@ pub(super) async fn sync( ) .await?; - // 2. Collect inputs: `dirs` stays as manifest strings (runtime preopens them), - // `files` are read on the host and passed inline. + // 2. Collect inputs as manifest strings. `run_plugin` preopens the `dirs` + // and reads the `files` itself — both anchored at `base_dir`, and both + // subject to the runtime's path-safety checks (no escaping or symlinked + // paths). let base_dir = Utf8PathBuf::from(params.path.as_str()); let dirs: Vec = adapter.dirs.clone().unwrap_or_default(); - - let mut files: Vec<(String, String)> = Vec::new(); - for name in adapter.files.as_deref().unwrap_or(&[]) { - ensure!( - !icp_sync_plugin::escapes_base(name), - UnsafeFilePathSnafu { name } - ); - // Reject symlinks in the declared path: neither the final entry nor any - // intermediate component may be a symlink, so the host read cannot - // escape the canister directory to a file elsewhere on disk. - if let Some(link) = icp_sync_plugin::first_symlink_component(¶ms.path, name) { - return SymlinkFileSnafu { name, link }.fail(); - } - let abs = params.path.join(name); - let content = read_to_string(abs.as_ref()).context(ReadFileSnafu { path: abs })?; - files.push((name.clone(), content)); - } + let files: Vec = adapter.files.clone().unwrap_or_default(); // 3. Run the plugin (blocking call — signal Tokio that this thread will block). let identity_principal = agent From 73bad5e2b8d1496e5ee130f7896c86cbba9c9aea Mon Sep 17 00:00:00 2001 From: Linwei Shang Date: Thu, 25 Jun 2026 15:01:49 -0400 Subject: [PATCH 5/6] test(sync-plugin): cover the Windows drive-prefix case for escapes_base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `escapes_base` assertions lived in the `cfg(all(test, unix))` module, so the Windows-specific `Prefix` rejection (`C:foo`) — the whole reason that branch exists — was never compiled or run. CI runs on Windows, so split the tests: - A cross-platform `escapes_base_tests` module: shared assertions, plus a `#[cfg(windows)]` test asserting drive-relative/absolute/UNC prefixes escape, and a `#[cfg(unix)]` test documenting that `C:foo` is a plain filename there. - The symlink tests stay unix-only (they create symlinks) as `symlink_tests`. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/icp-sync-plugin/src/path.rs | 46 ++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/crates/icp-sync-plugin/src/path.rs b/crates/icp-sync-plugin/src/path.rs index 289af290..316e0e87 100644 --- a/crates/icp-sync-plugin/src/path.rs +++ b/crates/icp-sync-plugin/src/path.rs @@ -59,25 +59,53 @@ pub(crate) fn first_symlink_component(base: &Utf8Path, rel: &str) -> Option Date: Thu, 25 Jun 2026 15:12:16 -0400 Subject: [PATCH 6/6] fix: doc comment was stale Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- crates/icp-sync-plugin/src/path.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/icp-sync-plugin/src/path.rs b/crates/icp-sync-plugin/src/path.rs index 316e0e87..625f7ac6 100644 --- a/crates/icp-sync-plugin/src/path.rs +++ b/crates/icp-sync-plugin/src/path.rs @@ -1,5 +1,5 @@ -//! Path-safety helpers shared between the host runtime (`dirs` preopens) and -//! the CLI's plugin sync (`files` reads). +//! Path-safety helpers used by the host runtime to validate declared `dirs`/`files` +//! entries before preopening directories or reading files under the canister base dir. use camino::{Utf8Component, Utf8Path, Utf8PathBuf};