diff --git a/bb-cli/src/bb/mod.rs b/bb-cli/src/bb/mod.rs index 390e8ddc5..ff1732e98 100644 --- a/bb-cli/src/bb/mod.rs +++ b/bb-cli/src/bb/mod.rs @@ -16,5 +16,6 @@ pub mod skills_config; pub mod skills_doctor; pub mod skills_install; pub mod skills_models; +pub mod skills_slug; pub mod skills_targets; pub mod workspace; diff --git a/bb-cli/src/bb/skills_install.rs b/bb-cli/src/bb/skills_install.rs index a141ae15d..075e0b484 100644 --- a/bb-cli/src/bb/skills_install.rs +++ b/bb-cli/src/bb/skills_install.rs @@ -17,6 +17,7 @@ use super::skills_models::{ InstallOperation, InstallPlanResponse, InstalledSkillMetadata, InstalledSkillRequest, SkillDetail, Warning, }; +use super::skills_slug::{confined_skill_path, ensure_confined_skill_path, validate_slug}; use super::skills_targets::{ backup_unmanaged_path, copy_dir_recursive, finish_link, iso8601_utc, link_into_target, remove_any, rollback_link, BackupOutcome, LinkOutcome, ResolvedTarget, Scope, @@ -215,7 +216,12 @@ impl PackageReplacement { } } -pub fn replace_managed_dir(staging: &Path, final_dir: &Path) -> Result { +pub fn replace_managed_dir( + root: &Path, + staging: &Path, + final_dir: &Path, +) -> Result { + ensure_confined_skill_path(root, final_dir)?; let final_metadata = fs::symlink_metadata(final_dir).ok(); let final_exists = final_metadata.is_some(); let is_bb_owned = final_metadata.is_some_and(|metadata| metadata.is_dir()) @@ -344,6 +350,18 @@ pub fn execute_plan( plan: InstallPlanResponse, options: &ExecuteOptions, ) -> Result { + // Validate the entire untrusted plan before the first operation can fetch + // an artifact or mutate the filesystem. Deserialization already applies + // this contract; this second gate protects programmatic future callers. + for operation in &plan.operations { + validate_slug(&operation.skill.slug).with_context(|| { + format!( + "invalid skill slug in install plan operation `{}`", + operation.action + ) + })?; + } + let mut execution = PlanExecution { plan_id: plan.plan_id, warnings: plan.warnings, @@ -395,7 +413,7 @@ fn execute_install_operation( .as_ref() .context("install operation did not include artifact metadata")?; - let final_dir = canonical_dir(config, options.scope, slug); + let final_dir = confined_skill_path(&canonical_root(config, options.scope), slug)?; // Source provenance comes from the catalog detail; failures downgrade to // missing provenance rather than blocking the install. @@ -466,7 +484,13 @@ fn persist_download(config: &SkillsConfig, slug: &str, version_id: &str, bytes: if fs::create_dir_all(&downloads).is_err() { return; } - let _ = fs::write(downloads.join(format!("{slug}-{version_id}.zip")), bytes); + let version_key = sha256_hex(version_id.as_bytes()); + let Ok(download_path) = confined_skill_path(&downloads, slug) + .map(|path| path.with_file_name(format!("{slug}-{version_key}.zip"))) + else { + return; + }; + let _ = fs::write(download_path, bytes); } fn write_package( @@ -478,6 +502,7 @@ fn write_package( let parent = final_dir .parent() .context("package directory has no parent")?; + ensure_confined_skill_path(parent, final_dir)?; fs::create_dir_all(parent).with_context(|| format!("create {}", parent.display()))?; let staging = parent.join(format!(".{}.tmp-{}", metadata.slug, unique_suffix())); @@ -499,7 +524,7 @@ fn write_package( serde_json::to_vec_pretty(metadata).context("serialize install metadata")?, ) .context("write install metadata")?; - replace_managed_dir(&staging, final_dir) + replace_managed_dir(parent, &staging, final_dir) })(); if result.is_err() && staging.exists() { let _ = fs::remove_dir_all(&staging); @@ -513,6 +538,7 @@ pub fn link_targets( targets: &[ResolvedTarget], slug: &str, ) -> Result> { + validate_slug(slug)?; let mut links = Vec::new(); for target in targets { for base_dir in &target.base_dirs { @@ -653,6 +679,7 @@ pub fn install_local_path( let parent = final_dir .parent() .context("package directory has no parent")?; + ensure_confined_skill_path(parent, &final_dir)?; fs::create_dir_all(parent).with_context(|| format!("create {}", parent.display()))?; let staging = parent.join(format!(".{slug}.tmp-{}", unique_suffix())); if staging.exists() { @@ -685,7 +712,7 @@ pub fn install_local_path( serde_json::to_vec_pretty(&metadata).context("serialize install metadata")?, ) .context("write install metadata")?; - let package_replacement = replace_managed_dir(&staging, &final_dir)?; + let package_replacement = replace_managed_dir(parent, &staging, &final_dir)?; let links = match link_targets(&final_dir, targets, &slug) { Ok(links) => links, Err(error) => { @@ -717,18 +744,6 @@ pub fn install_local_path( }) } -fn validate_slug(slug: &str) -> Result<()> { - let valid = !slug.is_empty() - && slug - .chars() - .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '-' || c == '_'); - if valid { - Ok(()) - } else { - anyhow::bail!("invalid skill name `{slug}`; use lowercase letters, digits, `-`, and `_`") - } -} - /// Deterministic content hash over a directory's files (path + bytes). fn hash_directory(dir: &Path) -> Result { let mut entries = Vec::new(); @@ -803,7 +818,7 @@ pub fn remove_skill( ) -> Result { use super::skills_targets::{inspect_link, remove_any, LinkState, TargetRegistry}; - let final_dir = canonical_dir(config, scope, slug); + let final_dir = confined_skill_path(&canonical_root(config, scope), slug)?; let metadata = read_metadata(&final_dir).ok(); if metadata.is_none() && !(include_unmanaged && force) { return Err(failure( @@ -840,7 +855,7 @@ pub fn remove_skill( let canonical_root = final_dir.parent(); for target in targets { for base_dir in &target.base_dirs { - let link_path = base_dir.join(slug); + let link_path = confined_skill_path(base_dir, slug)?; // The agents target's directory is the canonical packages root // itself (skills live there; other targets link to it), so its // entry is never a link to remove — package removal below @@ -875,7 +890,8 @@ pub fn remove_skill( } } // Clean up legacy Phase 1 copies under /targets/. - let legacy = config.legacy_target_dir(&target.name).join(slug); + let legacy_root = config.legacy_target_dir(&target.name); + let legacy = confined_skill_path(&legacy_root, slug)?; if legacy.exists() { if legacy.join(META_FILE_NAME).is_file() || (include_unmanaged && force) { remove_any(&legacy)?; @@ -999,7 +1015,7 @@ mod tests { fs::create_dir_all(&staging).expect("create staging skill"); fs::write(staging.join("SKILL.md"), "new").expect("write new skill"); - let backup = replace_managed_dir(&staging, &final_dir) + let backup = replace_managed_dir(&temp, &staging, &final_dir) .expect("replace managed skill") .finish() .expect("finish replacement"); @@ -1029,7 +1045,7 @@ mod tests { fs::create_dir_all(&staging).expect("create staging skill"); fs::write(staging.join("SKILL.md"), "new").expect("write new skill"); - let recovery = replace_managed_dir(&staging, &final_dir) + let recovery = replace_managed_dir(&temp, &staging, &final_dir) .expect("replace managed skill") .restore(&final_dir) .expect("restore previous package"); @@ -1052,7 +1068,7 @@ mod tests { fs::create_dir_all(&staging).expect("create staging skill"); fs::write(staging.join("SKILL.md"), "new").expect("write new skill"); - let recovery = replace_managed_dir(&staging, &final_dir) + let recovery = replace_managed_dir(&temp, &staging, &final_dir) .expect("replace unmanaged skill") .restore(&final_dir) .expect("restore unmanaged skill"); @@ -1079,7 +1095,7 @@ mod tests { fs::create_dir_all(&staging).expect("create staging skill"); fs::write(staging.join("SKILL.md"), "new").expect("write new skill"); - replace_managed_dir(&staging, &final_dir) + replace_managed_dir(&temp, &staging, &final_dir) .expect("replace manual symlink") .restore(&final_dir) .expect("restore manual symlink"); diff --git a/bb-cli/src/bb/skills_models.rs b/bb-cli/src/bb/skills_models.rs index 248860a0d..353209373 100644 --- a/bb-cli/src/bb/skills_models.rs +++ b/bb-cli/src/bb/skills_models.rs @@ -2,7 +2,7 @@ use std::collections::BTreeMap; -use serde::{Deserialize, Serialize}; +use serde::{de::Error as _, Deserialize, Deserializer, Serialize}; use serde_json::Value; pub use builderbot_auth::preferences::{ @@ -168,11 +168,21 @@ pub struct InstallOperation { #[derive(Debug, Deserialize)] pub struct PlanSkill { + #[serde(deserialize_with = "deserialize_skill_slug")] pub slug: String, pub version_id: String, pub content_sha256: String, } +fn deserialize_skill_slug<'de, D>(deserializer: D) -> Result +where + D: Deserializer<'de>, +{ + let slug = String::deserialize(deserializer)?; + super::skills_slug::validate_slug(&slug).map_err(D::Error::custom)?; + Ok(slug) +} + #[derive(Debug, Deserialize)] pub struct PlanArtifact { pub id: String, @@ -207,3 +217,70 @@ pub struct InstalledSkillMetadata { pub local_source: bool, pub pinned: bool, } + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + #[test] + fn install_plan_rejects_unsafe_slugs_for_every_action() { + let oversized = "a".repeat(super::super::skills_slug::MAX_SKILL_SLUG_BYTES + 1); + let unsafe_slugs = [ + "", + ".", + "..", + "../escape", + "foo/bar", + r"foo\bar", + "/absolute", + r"C:\absolute", + r"C:relative", + r"\\server\share", + oversized.as_str(), + ]; + + for action in ["install", "update", "remove", "noop", "future"] { + for slug in unsafe_slugs { + let plan = json!({ + "plan_id": "malicious", + "operations": [{ + "action": action, + "skill": { + "slug": slug, + "version_id": "version-1", + "content_sha256": "content-sha" + }, + "artifact": null, + "installed_via": "explicit" + }], + "warnings": [] + }); + let error = serde_json::from_value::(plan) + .expect_err("unsafe slug must reject the entire plan"); + assert!(error.to_string().contains("invalid skill name")); + } + } + } + + #[test] + fn install_plan_accepts_valid_marketplace_slug() { + let plan = json!({ + "plan_id": "valid", + "operations": [{ + "action": "noop", + "skill": { + "slug": "builderbot-tools", + "version_id": "version-1", + "content_sha256": "content-sha" + }, + "artifact": null, + "installed_via": "explicit" + }], + "warnings": [] + }); + + let plan = serde_json::from_value::(plan).expect("valid plan"); + assert_eq!(plan.operations[0].skill.slug, "builderbot-tools"); + } +} diff --git a/bb-cli/src/bb/skills_slug.rs b/bb-cli/src/bb/skills_slug.rs new file mode 100644 index 000000000..3f6efb528 --- /dev/null +++ b/bb-cli/src/bb/skills_slug.rs @@ -0,0 +1,112 @@ +//! Skill slug validation and path confinement. +//! +//! Marketplace plan data is untrusted. Keep every skill name to one portable +//! path component before joining it to an installation root. + +use std::path::{Component, Path, PathBuf}; + +use anyhow::{Context, Result}; + +pub const MAX_SKILL_SLUG_BYTES: usize = 128; + +pub fn validate_slug(slug: &str) -> Result<()> { + let portable_component = !slug.is_empty() + && slug.len() <= MAX_SKILL_SLUG_BYTES + && !slug.starts_with('-') + && slug + .chars() + .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '-' || c == '_') + && matches!( + Path::new(slug).components().collect::>().as_slice(), + [Component::Normal(_)] + ); + + if portable_component { + Ok(()) + } else { + anyhow::bail!( + "invalid skill name `{slug}`; use 1-{MAX_SKILL_SLUG_BYTES} bytes of lowercase letters, digits, `-`, and `_`, without a leading `-`" + ) + } +} + +/// Joins a validated slug to a filesystem root and verifies the resulting +/// lexical path remains an immediate child. This is deliberately repeated at +/// mutation boundaries so future callers cannot bypass plan validation. +pub fn confined_skill_path(root: &Path, slug: &str) -> Result { + validate_slug(slug)?; + let path = root.join(slug); + if path.parent() != Some(root) || path.file_name() != Some(slug.as_ref()) { + anyhow::bail!( + "skill path {} escapes installation root {}", + path.display(), + root.display() + ); + } + Ok(path) +} + +pub fn ensure_confined_skill_path(root: &Path, path: &Path) -> Result<()> { + let slug = path + .file_name() + .and_then(|name| name.to_str()) + .context("skill path has no UTF-8 file name")?; + let expected = confined_skill_path(root, slug)?; + if path != expected { + anyhow::bail!( + "skill path {} is outside installation root {}", + path.display(), + root.display() + ); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn accepts_portable_marketplace_slugs() { + for slug in ["builderbot-tools", "ach_tables", "a11y-web-audit-and-fix"] { + assert!(validate_slug(slug).is_ok(), "expected valid slug: {slug}"); + } + } + + #[test] + fn rejects_traversal_absolute_windows_and_oversized_slugs() { + let oversized = "a".repeat(MAX_SKILL_SLUG_BYTES + 1); + for slug in [ + "", + ".", + "..", + "../escape", + "foo/bar", + r"foo\bar", + "/absolute", + r"C:\absolute", + r"C:relative", + r"\\server\share", + "--project", + "has space", + "Uppercase", + oversized.as_str(), + ] { + assert!( + validate_slug(slug).is_err(), + "expected invalid slug: {slug:?}" + ); + } + } + + #[test] + fn confined_paths_are_immediate_children() { + let root = Path::new("skills"); + assert_eq!( + confined_skill_path(root, "demo").unwrap(), + root.join("demo") + ); + assert!(confined_skill_path(root, "../escape").is_err()); + assert!(ensure_confined_skill_path(root, Path::new("other/demo")).is_err()); + } +} diff --git a/bb-cli/src/bb/skills_targets.rs b/bb-cli/src/bb/skills_targets.rs index 74d61e1e2..18cb9d865 100644 --- a/bb-cli/src/bb/skills_targets.rs +++ b/bb-cli/src/bb/skills_targets.rs @@ -20,6 +20,7 @@ use serde::Serialize; use super::skills_api::{exit_codes, failure, MarketplaceClient}; use super::skills_config::{SkillsConfig, META_FILE_NAME}; use super::skills_models::{CapabilitiesResponse, TargetConfig}; +use super::skills_slug::confined_skill_path; const CAPABILITIES_CACHE_FILE: &str = "capabilities.json"; @@ -209,7 +210,7 @@ pub fn link_into_target( slug: &str, prefer_symlink: bool, ) -> Result { - let link_path = base_dir.join(slug); + let link_path = confined_skill_path(base_dir, slug)?; // If the path already resolves to the canonical package (for example // ~/.claude/skills is itself a symlink into the packages dir), leave it. diff --git a/bb-cli/tests/bb_e2e.rs b/bb-cli/tests/bb_e2e.rs index 123580155..47b038ac2 100644 --- a/bb-cli/tests/bb_e2e.rs +++ b/bb-cli/tests/bb_e2e.rs @@ -2706,6 +2706,86 @@ fn bb_skills_install_downloads_verifies_and_installs_into_isolated_home() { fs::remove_dir_all(temp).expect("remove temp dir"); } +#[test] +fn bb_skills_install_rejects_unsafe_plan_before_artifact_fetch_or_root_escape() { + let temp = temp_test_dir("bb-skills-unsafe-plan-slug"); + let bb_home = temp.join("bb-home"); + write_bb_org_config(&bb_home, "test"); + let skills_home = temp.join("skills-home"); + let agents_dir = temp.join("agents-skills"); + let outside = temp.join("outside-sentinel"); + fs::create_dir_all(&outside).expect("create outside sentinel"); + fs::write(outside.join("keep"), "untouched").expect("write outside sentinel"); + + let malicious_plan = json!({ + "plan_id": "malicious", + "operations": [{ + "action": "install", + "skill": { + "slug": "../outside-sentinel", + "version_id": "version-1", + "content_sha256": "content-sha" + }, + "artifact": { + "id": "artifact-1", + "download_url": "/must-not-fetch", + "sha256": "unused", + "size_bytes": 1 + }, + "installed_via": "explicit" + }], + "warnings": [] + }); + let server = MockServer::start(vec![ + capabilities_response(&agents_dir), + MockResponse::json(malicious_plan), + ]); + + let output = bb_command() + .env("BB_HOME", &bb_home) + .env("BB_SKILLS_HOME", &skills_home) + .env("BB_SKILLS_PACKAGES_DIR", skills_home.join("packages")) + .env("KGOOSE_BASE_URL", &server.base_url) + .args([ + "skills", + "install", + "builderbot-tools", + "--target", + "agents", + "--yes", + "--json", + ]) + .output() + .expect("run bb skills install"); + let requests = server.finish(); + let (_stdout, stderr) = output_text(&output); + + assert!(!output.status.success()); + assert!( + stderr.contains("invalid skill name"), + "stderr was: {stderr}" + ); + assert_eq!( + requests.len(), + 2, + "artifact or detail fetch escaped validation" + ); + assert_eq!( + fs::read_to_string(outside.join("keep")).unwrap(), + "untouched" + ); + assert!(!skills_home.join("outside-sentinel").exists()); + assert!(!agents_dir.join("outside-sentinel").exists()); + assert!( + !skills_home.join("downloads").exists() + || fs::read_dir(skills_home.join("downloads")) + .unwrap() + .next() + .is_none() + ); + fs::remove_dir_all(temp).expect("remove temp dir"); +} + #[test] fn bb_skills_install_and_update_restore_package_when_target_linking_fails() { for action in ["install", "update"] {