diff --git a/src/commands/skill.rs b/src/commands/skill.rs index d4c9993..f0f8d2d 100644 --- a/src/commands/skill.rs +++ b/src/commands/skill.rs @@ -2,7 +2,7 @@ use crossterm::style::Stylize; use directories::UserDirs; use semver::Version; use std::fs; -use std::path::PathBuf; +use std::path::{Component, Path, PathBuf}; /// Subcommands for `hotdata manage skills`. #[derive(clap::Subcommand)] @@ -230,35 +230,87 @@ fn download_and_extract_from_url(url: &str) -> Result<(), String> { // Extract into ~/.hotdata/skills/ let store_dir = home_dir().join(".hotdata").join("skills"); - fs::create_dir_all(&store_dir).map_err(|e| format!("error creating directory: {e}"))?; + extract_skills_archive(&bytes, &store_dir) +} + +/// Unpack the `skills/` tree of a gzipped tarball into `store_dir`. +/// +/// The whole archive is validated before anything is written, so an archive +/// with an invalid entry path leaves the store untouched. +fn extract_skills_archive(bytes: &[u8], store_dir: &Path) -> Result<(), String> { + for_each_skill_entry(bytes, |_, _| Ok(()))?; - let gz = flate2::read::GzDecoder::new(std::io::Cursor::new(bytes)); - let mut archive = tar::Archive::new(gz); + fs::create_dir_all(store_dir).map_err(|e| format!("error creating directory: {e}"))?; + for_each_skill_entry(bytes, |mut entry, rel| { + let dest = store_dir.join(&rel); + ensure_no_symlinks(store_dir, &rel)?; + if entry.header().entry_type().is_dir() { + return fs::create_dir_all(&dest).map_err(|e| format!("error creating directory: {e}")); + } + if let Some(parent) = dest.parent() { + fs::create_dir_all(parent).map_err(|e| format!("error creating directory: {e}"))?; + } + entry + .unpack(&dest) + .map(|_| ()) + .map_err(|e| format!("error extracting {}: {e}", rel.display())) + }) +} +/// Call `f` with each regular file or directory under `skills/`, along with its +/// path relative to `skills/`. Other entry types and paths are skipped. +fn for_each_skill_entry<'b>( + bytes: &'b [u8], + mut f: impl FnMut(tar::Entry<'_, flate2::read::GzDecoder<&'b [u8]>>, PathBuf) -> Result<(), String>, +) -> Result<(), String> { + let mut archive = tar::Archive::new(flate2::read::GzDecoder::new(bytes)); for entry in archive .entries() .map_err(|e| format!("error reading archive: {e}"))? { - let mut entry = entry.map_err(|e| format!("error reading archive entry: {e}"))?; + let entry = entry.map_err(|e| format!("error reading archive entry: {e}"))?; let path = entry .path() .map_err(|e| format!("error reading entry path: {e}"))? .into_owned(); - let rel = match path.strip_prefix("skills/") { - Ok(r) if !r.as_os_str().is_empty() => r.to_path_buf(), - _ => continue, - }; - - let dest = store_dir.join(&rel); - if let Some(parent) = dest.parent() { - fs::create_dir_all(parent).map_err(|e| format!("error creating directory: {e}"))?; + // Archive paths are untrusted; keep writes inside store_dir. + if path + .components() + .any(|c| !matches!(c, Component::Normal(_) | Component::CurDir)) + { + return Err(format!("invalid path in skill archive: {}", path.display())); } - entry - .unpack(&dest) - .map_err(|e| format!("error extracting {}: {e}", rel.display()))?; + let mut components = path.components(); + if components.next() != Some(Component::Normal("skills".as_ref())) { + continue; + } + let rel: PathBuf = components.collect(); + let kind = entry.header().entry_type(); + if rel.as_os_str().is_empty() || !(kind.is_file() || kind.is_dir()) { + continue; + } + f(entry, rel)?; } + Ok(()) +} +/// Refuse to write through a symlink already present under `store_dir`. +fn ensure_no_symlinks(store_dir: &Path, rel: &Path) -> Result<(), String> { + let mut path = store_dir.to_path_buf(); + for component in rel.components() { + path.push(component); + if path + .symlink_metadata() + .is_ok_and(|m| m.file_type().is_symlink()) + { + return Err(format!( + "error extracting {}: {} is a symlink", + rel.display(), + path.display() + )); + } + } Ok(()) } @@ -754,4 +806,177 @@ mod tests { "the surviving hotdata skill must be left in place" ); } + + enum TestEntry<'a> { + Dir(&'a str), + File(&'a str, &'a [u8]), + Symlink(&'a str, &'a Path), + HardLink(&'a str, &'a Path), + } + + /// Build a gzipped tarball, writing header paths verbatim so entries the + /// `tar` builder would normalise can still be exercised. + fn build_archive(entries: &[TestEntry]) -> Vec { + let gz = flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::default()); + let mut builder = tar::Builder::new(gz); + for entry in entries { + let (path, data, kind, link): (&str, &[u8], _, Option<&Path>) = match entry { + TestEntry::Dir(p) => (p, &[], tar::EntryType::Directory, None), + TestEntry::File(p, d) => (p, d, tar::EntryType::Regular, None), + TestEntry::Symlink(p, t) => (p, &[], tar::EntryType::Symlink, Some(t)), + TestEntry::HardLink(p, t) => (p, &[], tar::EntryType::Link, Some(t)), + }; + let mut header = tar::Header::new_gnu(); + let gnu = header.as_gnu_mut().unwrap(); + gnu.name[..path.len()].copy_from_slice(path.as_bytes()); + if let Some(target) = link { + let target = target.to_str().unwrap().as_bytes(); + gnu.linkname[..target.len()].copy_from_slice(target); + } + header.set_entry_type(kind); + header.set_mode(if kind.is_dir() { 0o755 } else { 0o644 }); + header.set_size(data.len() as u64); + header.set_cksum(); + builder.append(&header, data).unwrap(); + } + builder.into_inner().unwrap().finish().unwrap() + } + + fn store_in(tmp: &tempfile::TempDir) -> PathBuf { + tmp.path().join("store") + } + + #[test] + fn extracts_skills_archive_into_store() { + let tmp = tempfile::tempdir().unwrap(); + let store = store_in(&tmp); + let archive = build_archive(&[ + TestEntry::Dir("skills/"), + TestEntry::Dir("skills/hotdata/"), + TestEntry::File("skills/hotdata/SKILL.md", b"---\nversion: 1.2.3\n---\n"), + TestEntry::File("skills/hotdata/subskills/search/SKILL.md", b"search"), + ]); + + extract_skills_archive(&archive, &store).unwrap(); + + assert_eq!( + fs::read(store.join("hotdata/SKILL.md")).unwrap(), + b"---\nversion: 1.2.3\n---\n" + ); + assert_eq!( + fs::read(store.join("hotdata/subskills/search/SKILL.md")).unwrap(), + b"search" + ); + } + + #[test] + fn ignores_entries_outside_skills_dir() { + let tmp = tempfile::tempdir().unwrap(); + let store = store_in(&tmp); + let archive = build_archive(&[ + TestEntry::File("README.md", b"readme"), + TestEntry::File("other/hotdata/SKILL.md", b"other"), + TestEntry::File("skills/hotdata/SKILL.md", b"skill"), + ]); + + extract_skills_archive(&archive, &store).unwrap(); + + assert_eq!(fs::read(store.join("hotdata/SKILL.md")).unwrap(), b"skill"); + assert!(!store.join("README.md").exists()); + assert!(!store.join("other").exists()); + assert!(!tmp.path().join("README.md").exists()); + assert!(!tmp.path().join("other").exists()); + } + + #[test] + fn rejects_parent_dir_entries() { + for path in ["skills/../escaped", "skills/hotdata/../../escaped"] { + let tmp = tempfile::tempdir().unwrap(); + let store = store_in(&tmp); + let archive = build_archive(&[ + TestEntry::File("skills/hotdata/SKILL.md", b"skill"), + TestEntry::File(path, b"escaped"), + ]); + + let err = extract_skills_archive(&archive, &store).unwrap_err(); + + assert!( + err.contains("invalid path in skill archive"), + "{path}: {err}" + ); + assert!(!tmp.path().join("escaped").exists(), "{path}"); + assert!( + !store.join("hotdata/SKILL.md").exists(), + "{path}: nothing should be extracted from a rejected archive" + ); + } + } + + #[test] + fn rejects_absolute_path_entries() { + let tmp = tempfile::tempdir().unwrap(); + let store = store_in(&tmp); + let outside = tmp.path().join("outside.md"); + let archive = build_archive(&[TestEntry::File(outside.to_str().unwrap(), b"outside")]); + + let err = extract_skills_archive(&archive, &store).unwrap_err(); + + assert!(err.contains("invalid path in skill archive"), "{err}"); + assert!(!outside.exists()); + } + + #[test] + fn skips_symlink_entries() { + let tmp = tempfile::tempdir().unwrap(); + let store = store_in(&tmp); + let outside = tmp.path().join("outside"); + fs::create_dir_all(&outside).unwrap(); + let archive = build_archive(&[ + TestEntry::Symlink("skills/hotdata/linked", &outside), + TestEntry::File("skills/hotdata/linked/SKILL.md", b"through link"), + ]); + + extract_skills_archive(&archive, &store).unwrap(); + + assert!(!outside.join("SKILL.md").exists()); + let linked = store.join("hotdata/linked"); + assert!(!linked.symlink_metadata().unwrap().file_type().is_symlink()); + assert_eq!(fs::read(linked.join("SKILL.md")).unwrap(), b"through link"); + } + + #[test] + fn skips_hard_link_entries() { + let tmp = tempfile::tempdir().unwrap(); + let store = store_in(&tmp); + let outside = tmp.path().join("outside.md"); + fs::write(&outside, b"original").unwrap(); + let archive = build_archive(&[ + TestEntry::HardLink("skills/hotdata/SKILL.md", &outside), + TestEntry::File("skills/hotdata/README.md", b"readme"), + ]); + + extract_skills_archive(&archive, &store).unwrap(); + + assert!(!store.join("hotdata/SKILL.md").exists()); + assert_eq!( + fs::read(store.join("hotdata/README.md")).unwrap(), + b"readme" + ); + assert_eq!(fs::read(&outside).unwrap(), b"original"); + } + + #[cfg(unix)] + #[test] + fn rejects_existing_symlink_in_store() { + let tmp = tempfile::tempdir().unwrap(); + let store = store_in(&tmp); + let outside = tmp.path().join("outside"); + fs::create_dir_all(&outside).unwrap(); + fs::create_dir_all(&store).unwrap(); + std::os::unix::fs::symlink(&outside, store.join("hotdata")).unwrap(); + let archive = build_archive(&[TestEntry::File("skills/hotdata/SKILL.md", b"skill")]); + + assert!(extract_skills_archive(&archive, &store).is_err()); + assert!(!outside.join("SKILL.md").exists()); + } }