diff --git a/crates/tui/src/skills/install.rs b/crates/tui/src/skills/install.rs index 95ac9757..3303f3cf 100644 --- a/crates/tui/src/skills/install.rs +++ b/crates/tui/src/skills/install.rs @@ -24,8 +24,11 @@ //! only created (via atomic rename) once the tarball clears every check. //! Half-installed skills can never appear on disk. //! * Path traversal rejection covers both `..` segments and absolute paths. -//! Symlinks inside the archive are also rejected — there's no use case for -//! them in a SKILL.md bundle and they're a notorious foothold for escape. +//! Symlinks inside the selected skill subtree are rejected — there's no use +//! case for them in a SKILL.md bundle and they're a notorious foothold for +//! escape. Multi-skill repository archives may contain unrelated symlinks +//! outside that selected subtree; those entries are ignored and never +//! extracted. //! * No `+x` is granted on extracted files. The optional `/skill trust ` //! command writes a `.trusted` marker; tool-execution gating is a separate //! concern that lives next to the tool registry. @@ -238,8 +241,9 @@ pub enum InstallError { /// [`InstallOutcome::NetworkDenied`]; `Prompt` returns /// [`InstallOutcome::NeedsApproval`] without touching disk. /// 3. Stream the tarball into a tempfile (capped at `max_size`). -/// 4. Validate the archive (path-traversal, size, no symlinks, SKILL.md present -/// with required frontmatter fields) into a sibling `.tmp/` directory. +/// 4. Validate the archive (path-traversal, size, no symlinks in the selected +/// skill subtree, SKILL.md present with required frontmatter fields) into a +/// sibling `.tmp/` directory. /// 5. Atomic-rename `.tmp/` → `/`. /// 6. Write `.installed-from` and return [`InstalledSkill`]. /// @@ -1053,8 +1057,8 @@ struct TarballScan { } /// First pass: locate SKILL.md, validate frontmatter, compute total size, -/// reject path-traversal / symlink entries. We do not write anything in this -/// pass; that's the second pass's job. +/// reject path-traversal entries and symlinks inside the selected install +/// subtree. We do not write anything in this pass; that's the second pass's job. fn scan_tarball(bytes: &[u8], max_size: u64) -> Result { let cursor = std::io::Cursor::new(bytes); let gz = GzDecoder::new(cursor); @@ -1063,6 +1067,7 @@ fn scan_tarball(bytes: &[u8], max_size: u64) -> Result { let mut total_size: u64 = 0; let mut prefix: Option = None; let mut skill_md_relative: Option<(String, Vec)> = None; + let mut link_paths: Vec = Vec::new(); for entry in archive .entries() @@ -1071,9 +1076,6 @@ fn scan_tarball(bytes: &[u8], max_size: u64) -> Result { let mut entry = entry.context("failed to read tar entry")?; let header = entry.header().clone(); let entry_type = header.entry_type(); - if entry_type.is_symlink() || entry_type.is_hard_link() { - return Err(InstallError::SymlinkRejected.into()); - } let path = entry .path() .context("tar entry has invalid path")? @@ -1115,6 +1117,11 @@ fn scan_tarball(bytes: &[u8], max_size: u64) -> Result { } } + if entry_type.is_symlink() || entry_type.is_hard_link() { + link_paths.push(path_str); + continue; + } + // SKILL.md detection. Match either: // * `/SKILL.md` // * `/skills//SKILL.md` @@ -1143,11 +1150,6 @@ fn scan_tarball(bytes: &[u8], max_size: u64) -> Result { .ok_or(InstallError::MissingSkillMd) .map_err(anyhow::Error::from)?; - // Parse frontmatter to extract the skill name. We reuse the same parser - // shape as `SkillRegistry::parse_skill` but inline it here so we don't - // depend on the discovery module's private function. - let name = parse_frontmatter_name(&skill_md_bytes)?; - let skill_root = if skill_md_path == "SKILL.md" { String::new() } else { @@ -1158,6 +1160,17 @@ fn scan_tarball(bytes: &[u8], max_size: u64) -> Result { .to_string() }; + for link_path in link_paths { + if is_within_selected_root(&link_path, &prefix, &skill_root) { + return Err(InstallError::SymlinkRejected.into()); + } + } + + // Parse frontmatter to extract the skill name. We reuse the same parser + // shape as `SkillRegistry::parse_skill` but inline it here so we don't + // depend on the discovery module's private function. + let name = parse_frontmatter_name(&skill_md_bytes)?; + Ok(TarballScan { skill_name: name, prefix, @@ -1186,9 +1199,6 @@ fn extract_into(scan: &TarballScan, bytes: &[u8], dest: &Path, max_size: u64) -> let mut entry = entry.context("failed to read tar entry")?; let header = entry.header().clone(); let entry_type = header.entry_type(); - if entry_type.is_symlink() || entry_type.is_hard_link() { - return Err(InstallError::SymlinkRejected.into()); - } let path = entry .path() .context("tar entry has invalid path")? @@ -1215,6 +1225,9 @@ fn extract_into(scan: &TarballScan, bytes: &[u8], dest: &Path, max_size: u64) -> if !is_safe_path(stripped_path) { return Err(InstallError::PathTraversal(stripped).into()); } + if entry_type.is_symlink() || entry_type.is_hard_link() { + return Err(InstallError::SymlinkRejected.into()); + } let target = dest.join(stripped_path); // Final paranoia check: ensure the resolved target stays under dest. @@ -1258,6 +1271,24 @@ fn extract_into(scan: &TarballScan, bytes: &[u8], dest: &Path, max_size: u64) -> Ok(()) } +fn selected_root(prefix: &str, skill_root: &str) -> String { + if skill_root.is_empty() { + prefix.to_string() + } else if prefix.is_empty() { + skill_root.to_string() + } else { + format!("{prefix}/{skill_root}") + } +} + +fn is_within_selected_root(path: &str, prefix: &str, skill_root: &str) -> bool { + let root = selected_root(prefix, skill_root); + if root.is_empty() { + return true; + } + path == root || path.starts_with(&format!("{root}/")) +} + /// Ensure a tar path has no `..` segments and is not absolute. fn is_safe_path(path: &Path) -> bool { if path.is_absolute() { diff --git a/crates/tui/tests/skill_install.rs b/crates/tui/tests/skill_install.rs index 4789f7b5..c6a958ac 100644 --- a/crates/tui/tests/skill_install.rs +++ b/crates/tui/tests/skill_install.rs @@ -506,6 +506,75 @@ async fn install_rejects_symlink_entry() { shutdown(tx, handle); } +#[tokio::test] +async fn install_ignores_symlink_outside_selected_skill_root() { + let mut gz = GzEncoder::new(Vec::new(), Compression::default()); + { + let mut builder = tar::Builder::new(&mut gz); + + let mut link_hdr = tar::Header::new_gnu(); + link_hdr.set_entry_type(tar::EntryType::Symlink); + link_hdr.set_size(0); + link_hdr.set_mode(0o777); + builder + .append_link(&mut link_hdr, "repo-main/AGENTS.md", Path::new("CLAUDE.md")) + .unwrap(); + + let body = skill_md("nested-skill", "Nested skill"); + let mut hdr = tar::Header::new_gnu(); + hdr.set_size(body.len() as u64); + hdr.set_mode(0o644); + hdr.set_cksum(); + builder + .append_data( + &mut hdr, + "repo-main/skills/nested-skill/SKILL.md", + body.as_slice(), + ) + .unwrap(); + + let notes = b"selected subtree only"; + let mut notes_hdr = tar::Header::new_gnu(); + notes_hdr.set_size(notes.len() as u64); + notes_hdr.set_mode(0o644); + notes_hdr.set_cksum(); + builder + .append_data( + &mut notes_hdr, + "repo-main/skills/nested-skill/notes.txt", + notes.as_slice(), + ) + .unwrap(); + + builder.finish().unwrap(); + } + let tarball = gz.finish().unwrap(); + let (url, tx, handle) = spawn_tarball_server(tarball); + + let tmp = TempDir::new().unwrap(); + let policy = allow_all_policy(); + let outcome = install::install( + InstallSource::DirectUrl(url), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect("repo-level symlink outside selected skill root should be ignored"); + let installed = match outcome { + InstallOutcome::Installed(installed) => installed, + other => panic!("expected Installed, got {other:?}"), + }; + + assert_eq!(installed.name, "nested-skill"); + assert!(installed.path.join("SKILL.md").exists()); + assert!(installed.path.join("notes.txt").exists()); + assert!(!installed.path.join("AGENTS.md").exists()); + + shutdown(tx, handle); +} + #[test] fn uninstall_refuses_system_skill() { let tmp = TempDir::new().unwrap();