From ebd1b5c7cb8c5715b6b1cb1c9e09a4bdb9e92103 Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Fri, 25 Sep 2026 23:19:15 +0000 Subject: [PATCH] fix: reject path-like network and environment names Network and environment names are used as path components under `.icp` (the network's state directory and `.ids.json`), but were never validated. Hold both to the same rule canister names already follow: non-empty, ASCII letters, digits, `_` and `-` only. --- CHANGELOG.md | 2 + crates/icp-project/src/project.rs | 62 ++++++++++++++++++++++++++++--- 2 files changed, 58 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2db52e95d..167aa60d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ air-gapped signing # Unreleased +fix: reject path-like network and environment names + # v1.6.0 * chore: the macOS `icp` release binaries are now code-signed with DFINITY's Developer ID certificate. This improves the Keychain experience for identities stored in the keyring. macOS used to treat each unsigned release as a different app, so choosing "Always Allow" lasted only until the next upgrade. The signed binary keeps the same identity across releases, so after you allow access once more on the first signed version, it stays allowed through later upgrades. The signed binary is what you get from the GitHub release (including the shell installer) and npm. `brew install icp-cli` from homebrew-core is not signed, because Homebrew builds that formula from source on its own infrastructure. The binary is not notarized, so a release archive downloaded through a browser still gets a Gatekeeper warning; the shell installer and npm are unaffected. diff --git a/crates/icp-project/src/project.rs b/crates/icp-project/src/project.rs index 9d21dfb04..e8f28cfbc 100644 --- a/crates/icp-project/src/project.rs +++ b/crates/icp-project/src/project.rs @@ -138,6 +138,16 @@ pub enum ConsolidateManifestError { ))] InvalidDependencyAlias { alias: String }, + #[snafu(display( + "network name '{name}' is invalid: only ASCII letters, digits, '_' and '-' are allowed" + ))] + InvalidNetworkName { name: String }, + + #[snafu(display( + "environment name '{name}' is invalid: only ASCII letters, digits, '_' and '-' are allowed" + ))] + InvalidEnvironmentName { name: String }, + #[snafu(display("project declares two dependencies with the same alias '{alias}'"))] DuplicateDependencyAlias { alias: String }, @@ -318,14 +328,17 @@ fn is_glob(s: &str) -> bool { s.contains('*') || s.contains('?') || s.contains('[') || s.contains('{') } -/// Whether `name` is a valid canister name or dependency alias: non-empty and -/// containing only ASCII letters, digits, `_`, or `-`. +/// Whether `name` is a valid canister name, dependency alias, network name or +/// environment name: non-empty and containing only ASCII letters, digits, `_`, +/// or `-`. /// /// A single strict rule keeps names safe for every purpose they are reused for — -/// store-key segments, `PUBLIC_CANISTER_ID:` env vars, DNS subdomains, and -/// archive paths — so no per-site sanitizing is needed. In particular `:` is the -/// dependency namespace separator, and `.` / `/` would be ambiguous in -/// subdomains and paths. +/// store-key segments, `PUBLIC_CANISTER_ID:` env vars, DNS subdomains, +/// archive paths, and the path components under `.icp` that network and +/// environment names become — so no per-site sanitizing is needed. In +/// particular `:` is the dependency namespace separator, `.` / `/` would be +/// ambiguous in subdomains and paths, and `/`, `..` or an absolute name would +/// escape `.icp`. fn is_valid_name(name: &str) -> bool { !name.is_empty() && name @@ -1449,6 +1462,10 @@ pub async fn consolidate_manifest( Item::Manifest(ms) => ms.clone(), }; + if !is_valid_name(&m.name) { + return InvalidNetworkNameSnafu { name: m.name }.fail(); + } + match networks.entry(m.name.to_owned()) { // Duplicate Entry::Occupied(e) => { @@ -1529,6 +1546,10 @@ pub async fn consolidate_manifest( Item::Manifest(ms) => ms.clone(), }; + if !is_valid_name(&m.name) { + return InvalidEnvironmentNameSnafu { name: m.name }.fail(); + } + match environments.entry(m.name.to_owned()) { // Duplicate Entry::Occupied(e) => { @@ -2608,6 +2629,35 @@ environments: ); } + #[tokio::test] + async fn path_like_network_and_environment_names_are_rejected() { + // Both names become path components under `.icp` (the network's state + // directory, the environment's `.ids.json`), so anything that could + // leave that directory must be refused before it is ever joined. + for bad in ["/tmp/evil", "../../evil", "a/b", "..", "a.b", ""] { + let tmp = Utf8TempDir::new().unwrap(); + let body = manifest( + &[], + &format!("networks:\n - name: \"{bad}\"\n mode: managed\n"), + ); + write(tmp.path(), "icp.yaml", &body); + let err = consolidate(tmp.path()).await.unwrap_err(); + assert!( + matches!(err, ConsolidateManifestError::InvalidNetworkName { .. }), + "network {bad:?}: got {err:?}" + ); + + let tmp = Utf8TempDir::new().unwrap(); + let body = manifest(&[], &format!("environments:\n - name: \"{bad}\"\n")); + write(tmp.path(), "icp.yaml", &body); + let err = consolidate(tmp.path()).await.unwrap_err(); + assert!( + matches!(err, ConsolidateManifestError::InvalidEnvironmentName { .. }), + "environment {bad:?}: got {err:?}" + ); + } + } + #[tokio::test] async fn dot_in_canister_name_is_rejected() { let tmp = Utf8TempDir::new().unwrap();