diff --git a/Cargo.lock b/Cargo.lock index 825df740..1dbfb626 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1171,6 +1171,7 @@ dependencies = [ "deepseek-secrets", "dirs", "dotenvy", + "flate2", "futures-util", "ignore", "image", @@ -1186,9 +1187,11 @@ dependencies = [ "rustyline 15.0.0", "serde", "serde_json", + "sha2", "shellexpand", "shlex", "starlark", + "tar", "tempfile", "thiserror 2.0.17", "tiny_http", @@ -1597,6 +1600,17 @@ dependencies = [ "winapi", ] +[[package]] +name = "filetime" +version = "0.2.27" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f98844151eee8917efc50bd9e8318cb963ae8b297431495d3f758616ea5c57db" +dependencies = [ + "cfg-if", + "libc", + "libredox", +] + [[package]] name = "find-msvc-tools" version = "0.1.7" @@ -2484,6 +2498,7 @@ checksum = "3d0b95e02c851351f877147b7deea7b1afb1df71b63aa5f8270716e0c5720616" dependencies = [ "bitflags 2.10.0", "libc", + "redox_syscall 0.7.4", ] [[package]] @@ -3063,7 +3078,7 @@ checksum = "2621685985a2ebf1c516881c026032ac7deafcda1a2c9b7850dc81e3dfcb64c1" dependencies = [ "cfg-if", "libc", - "redox_syscall", + "redox_syscall 0.5.18", "smallvec", "windows-link 0.2.1", ] @@ -3458,6 +3473,15 @@ dependencies = [ "bitflags 2.10.0", ] +[[package]] +name = "redox_syscall" +version = "0.7.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f450ad9c3b1da563fb6948a8e0fb0fb9269711c9c73d9ea1de5058c79c8d643a" +dependencies = [ + "bitflags 2.10.0", +] + [[package]] name = "redox_users" version = "0.4.6" @@ -4335,6 +4359,17 @@ dependencies = [ "syn 2.0.114", ] +[[package]] +name = "tar" +version = "0.4.45" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "22692a6476a21fa75fdfc11d452fda482af402c008cdbaf3476414e122040973" +dependencies = [ + "filetime", + "libc", + "xattr", +] + [[package]] name = "tempfile" version = "3.24.0" @@ -5487,6 +5522,16 @@ version = "0.13.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ea6fc2961e4ef194dcbfe56bb845534d0dc8098940c7e5c012a258bfec6701bd" +[[package]] +name = "xattr" +version = "1.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32e45ad4206f6d2479085147f02bc2ef834ac85886624a23575ae137c8aa8156" +dependencies = [ + "libc", + "rustix 1.1.3", +] + [[package]] name = "xdg-home" version = "1.3.0" diff --git a/crates/tui/Cargo.toml b/crates/tui/Cargo.toml index d52ea218..5e555b70 100644 --- a/crates/tui/Cargo.toml +++ b/crates/tui/Cargo.toml @@ -57,6 +57,9 @@ zeroize = "1.8.2" ignore = "0.4" image = { version = "0.25", default-features = false, features = ["png"] } pdf-extract = "0.7" +tar = "0.4" +flate2 = "1.1" +sha2 = "0.10" [dev-dependencies] wiremock = "0.6" diff --git a/crates/tui/src/skills/install.rs b/crates/tui/src/skills/install.rs new file mode 100644 index 00000000..fe55823c --- /dev/null +++ b/crates/tui/src/skills/install.rs @@ -0,0 +1,1116 @@ +//! Community-skill installer (#140). +//! +//! Pulls user-authored skills from GitHub or direct tarball URLs, validates them +//! against a path-traversal- and size-bounded extractor, and writes them into +//! `//`. No backend service, no auto-execution: every install +//! is gated by the per-domain [`crate::network_policy::NetworkPolicy`] and +//! validation rejects any tarball entry that escapes the destination directory. +//! +//! Public surface: +//! +//! * [`InstallSource`] — `github:owner/repo`, raw URL, or curated registry +//! name. Parsed from a single string with [`InstallSource::parse`]. +//! * [`install`] / [`update`] / [`uninstall`] — async install, atomic update, +//! and clean uninstall. All three preserve a `.installed-from` marker so the +//! bundled `skill-creator` (which lacks the marker) is never touched. +//! * [`InstallOutcome`] — `Installed` / `NeedsApproval(host)` / +//! `NetworkDenied(host)`. The `NeedsApproval` variant is returned without +//! side effects so the caller (slash-command, runtime API, etc.) can route +//! through its own approval flow. +//! +//! # Hard rules +//! +//! * Validation extracts to a temp directory first. The destination path is +//! 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. +//! * 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. + +use std::fs; +use std::io::{Read, Write}; +use std::path::{Component, Path, PathBuf}; + +use anyhow::{Context, Result, bail}; +use flate2::read::GzDecoder; +use serde::Deserialize; +use sha2::{Digest, Sha256}; +use thiserror::Error; + +use crate::network_policy::{Decision, NetworkPolicy, host_from_url}; + +/// Default registry. Falls back to a community-curated `index.json` hosted on +/// GitHub raw; users can override via `[skills] registry_url` in config.toml. +pub const DEFAULT_REGISTRY_URL: &str = + "https://raw.githubusercontent.com/Hmbown/deepseek-skills/main/index.json"; + +/// Default per-skill size cap (5 MiB). Honored at unpack time so a malicious +/// gzip bomb can't blow up RAM. +pub const DEFAULT_MAX_SIZE_BYTES: u64 = 5 * 1024 * 1024; + +/// File written under each installed skill so [`update`] / [`uninstall`] can +/// recover the original [`InstallSource`] without re-parsing user input. +pub const INSTALLED_FROM_MARKER: &str = ".installed-from"; + +/// File written under each trusted skill. Currently advisory (the install path +/// never auto-runs anything) — the runtime tool-invocation gate consults this +/// marker before executing scripts that ship with the skill. +pub const TRUSTED_MARKER: &str = ".trusted"; + +// ───────────────────────────────────────────────────────────────────────────── +// Source parsing +// ───────────────────────────────────────────────────────────────────────────── + +/// Where a skill is being installed from. See [`InstallSource::parse`] for the +/// accepted spec syntax. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum InstallSource { + /// `github:owner/repo`. Resolved to + /// `https://github.com///archive/refs/heads/main.tar.gz` + /// with a `master.tar.gz` fallback on 404. + GitHubRepo(String), + /// Raw `http(s)://…` tarball URL. Used as-is. + DirectUrl(String), + /// Curated registry lookup key. Looked up via the configured `registry_url`. + Registry(String), +} + +impl InstallSource { + /// Parse a user-supplied spec. Empty / whitespace-only input is rejected. + /// + /// * `github:owner/repo` → [`InstallSource::GitHubRepo`] + /// * `http://` or `https://` prefix → [`InstallSource::DirectUrl`] + /// * anything else → [`InstallSource::Registry`] + pub fn parse(spec: &str) -> Result { + let trimmed = spec.trim(); + if trimmed.is_empty() { + bail!("install source must not be empty"); + } + if let Some(rest) = trimmed.strip_prefix("github:") { + let rest = rest.trim(); + // Reject obviously bogus values up front. We intentionally accept + // case-insensitive owner/repo so `github:Hmbown/Foo` works. + let (owner, repo) = rest.split_once('/').with_context(|| { + format!("github source must be 'github:owner/repo' (got {spec})") + })?; + let owner = owner.trim(); + let repo = repo.trim().trim_end_matches('/'); + if owner.is_empty() || repo.is_empty() { + bail!("github source must be 'github:owner/repo' (got {spec})"); + } + if owner.contains('/') || repo.contains('/') { + bail!("github source must be 'github:owner/repo' (got {spec})"); + } + return Ok(Self::GitHubRepo(format!("{owner}/{repo}"))); + } + if trimmed.starts_with("https://") || trimmed.starts_with("http://") { + return Ok(Self::DirectUrl(trimmed.to_string())); + } + Ok(Self::Registry(trimmed.to_string())) + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Outcome / result types +// ───────────────────────────────────────────────────────────────────────────── + +/// Outcome of an install attempt. +#[derive(Debug)] +pub enum InstallOutcome { + /// The skill was installed (or already present and idempotent). + Installed(InstalledSkill), + /// The host requires user approval before the install can proceed. The + /// caller should surface this through whatever approval pathway it has and + /// retry once approved (typically by adding the host to the policy's + /// allow list). + NeedsApproval(String), + /// The host is denied by network policy. The install is aborted. + NetworkDenied(String), +} + +/// Metadata for a successfully installed skill. +#[derive(Debug, Clone)] +pub struct InstalledSkill { + /// Skill name (taken from SKILL.md frontmatter). + pub name: String, + /// Final on-disk path: `//`. + pub path: PathBuf, + /// SHA-256 over the downloaded tarball bytes. Used by [`update`] to detect + /// upstream changes without re-extracting; also surfaced for telemetry / + /// future signature-verification work. + #[allow(dead_code)] + pub source_checksum: String, +} + +/// Result of an [`update`] call. +#[derive(Debug)] +pub enum UpdateResult { + /// Upstream tarball is byte-identical to the on-disk checksum; no action. + NoChange, + /// Upstream changed and the on-disk install was atomically replaced. + Updated(InstalledSkill), + /// Network policy short-circuited the update. Same semantics as + /// [`InstallOutcome::NeedsApproval`]. + NeedsApproval(String), + /// Network policy denied the update. + NetworkDenied(String), +} + +/// Errors that can happen during install. Most variants are flattened into +/// `anyhow::Error` at the public boundary; this enum is used internally so +/// tests can pattern-match without parsing strings. +#[derive(Debug, Error)] +pub enum InstallError { + #[error("entry escapes destination directory: {0}")] + PathTraversal(String), + #[error("entry is too large; uncompressed total would exceed {limit} bytes")] + OversizedTarball { limit: u64 }, + #[error("missing SKILL.md in archive")] + MissingSkillMd, + #[error("SKILL.md frontmatter missing required field: {0}")] + MissingFrontmatterField(&'static str), + #[error("symlinks are not allowed in skill tarballs")] + SymlinkRejected, + #[error("skill '{0}' is already installed; use update or remove it first")] + AlreadyInstalled(String), + #[error("skill '{0}' was not installed via /skill install (no .installed-from marker)")] + NotInstalledHere(String), +} + +// ───────────────────────────────────────────────────────────────────────────── +// Public API +// ───────────────────────────────────────────────────────────────────────────── + +/// Install a community skill into `skills_dir`. +/// +/// Steps: +/// +/// 1. Resolve `source` to one or more candidate URLs (GitHub adds a +/// `master` fallback after `main`). +/// 2. Consult `network` for the host. `Allow` proceeds; `Deny` returns +/// [`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. +/// 5. Atomic-rename `.tmp/` → `/`. +/// 6. Write `.installed-from` and return [`InstalledSkill`]. +/// +/// `update = false` rejects an existing destination. Pass `update = true` +/// from [`update`] to allow replacement. +/// +/// Convenience wrapper over [`install_with_registry`] that uses the bundled +/// [`DEFAULT_REGISTRY_URL`]. Public for downstream consumers (tests, runtime +/// API) even though the slash-command path always goes through +/// [`install_with_registry`] so the user's configured registry wins. +#[allow(dead_code)] +pub async fn install( + source: InstallSource, + skills_dir: &Path, + max_size: u64, + network: &NetworkPolicy, + update: bool, +) -> Result { + install_with_registry( + source, + skills_dir, + max_size, + network, + update, + DEFAULT_REGISTRY_URL, + ) + .await +} + +/// Same as [`install`] but lets the caller override the registry URL. Useful +/// for tests; the slash-command path always uses the configured registry. +pub async fn install_with_registry( + source: InstallSource, + skills_dir: &Path, + max_size: u64, + network: &NetworkPolicy, + update: bool, + registry_url: &str, +) -> Result { + let urls = candidate_urls(&source, network, registry_url).await?; + let urls = match urls { + UrlResolution::Resolved(urls) => urls, + UrlResolution::NeedsApproval(host) => return Ok(InstallOutcome::NeedsApproval(host)), + UrlResolution::Denied(host) => return Ok(InstallOutcome::NetworkDenied(host)), + }; + + // Try each URL in order — GitHub returns 404 for `main` on master-only + // repos, and we don't want to fail the install on that. + let (bytes, source_url) = match download_first_success(&urls, network, max_size).await? { + DownloadOutcome::Bytes { bytes, url } => (bytes, url), + DownloadOutcome::NeedsApproval(host) => return Ok(InstallOutcome::NeedsApproval(host)), + DownloadOutcome::Denied(host) => return Ok(InstallOutcome::NetworkDenied(host)), + }; + + // Compute a checksum before unpacking so [`update`] can detect upstream + // no-op changes without redoing the extract. + let mut hasher = Sha256::new(); + hasher.update(&bytes); + let checksum = format!("{:x}", hasher.finalize()); + + let staged = stage_tarball(&bytes, skills_dir, max_size)?; + + // Move the staged dir into its final location. If `update` is set and the + // destination exists, replace it; otherwise reject. + let final_path = skills_dir.join(&staged.skill_name); + if final_path.exists() { + if !update { + // Clean up the staging dir before returning the error. + let _ = fs::remove_dir_all(&staged.staged_path); + return Err(InstallError::AlreadyInstalled(staged.skill_name).into()); + } + // Best-effort backup-then-replace; on failure we restore the original. + let backup = skills_dir.join(format!("{}.bak", staged.skill_name)); + // If a previous failed update left a stale `.bak/`, drop it. + if backup.exists() { + fs::remove_dir_all(&backup).ok(); + } + fs::rename(&final_path, &backup).with_context(|| { + format!( + "failed to backup existing skill at {}", + final_path.display() + ) + })?; + if let Err(err) = fs::rename(&staged.staged_path, &final_path) { + // Roll back: restore the backup so the user isn't left with an + // empty skill directory. + fs::rename(&backup, &final_path).ok(); + return Err(err).context("failed to install staged skill"); + } + fs::remove_dir_all(&backup).ok(); + } else { + if let Some(parent) = final_path.parent() { + fs::create_dir_all(parent).with_context(|| { + format!("failed to create skills directory {}", parent.display()) + })?; + } + fs::rename(&staged.staged_path, &final_path).context("failed to install staged skill")?; + } + + // Write the marker last so a partial install never leaves a stale + // .installed-from on disk. + let marker_body = serde_json::json!({ + "spec": source_spec_string(&source), + "url": source_url, + "checksum": checksum, + }) + .to_string(); + fs::write(final_path.join(INSTALLED_FROM_MARKER), marker_body).with_context(|| { + format!( + "failed to write {} marker for skill {}", + INSTALLED_FROM_MARKER, staged.skill_name + ) + })?; + + Ok(InstallOutcome::Installed(InstalledSkill { + name: staged.skill_name, + path: final_path, + source_checksum: checksum, + })) +} + +/// Re-fetch a previously installed skill and replace it on disk if the +/// upstream tarball changed. +/// +/// Reads `.installed-from` to recover the original [`InstallSource`], so +/// a skill installed via `/skill install github:foo/bar` can be updated via +/// `/skill update bar` without the user re-typing the spec. +/// +/// Convenience wrapper over [`update_with_registry`]. +#[allow(dead_code)] +pub async fn update( + name: &str, + skills_dir: &Path, + max_size: u64, + network: &NetworkPolicy, +) -> Result { + update_with_registry(name, skills_dir, max_size, network, DEFAULT_REGISTRY_URL).await +} + +/// Same as [`update`] but lets the caller override the registry URL. +pub async fn update_with_registry( + name: &str, + skills_dir: &Path, + max_size: u64, + network: &NetworkPolicy, + registry_url: &str, +) -> Result { + let target = skills_dir.join(name); + let marker_path = target.join(INSTALLED_FROM_MARKER); + if !marker_path.exists() { + return Err(InstallError::NotInstalledHere(name.to_string()).into()); + } + let marker_body = fs::read_to_string(&marker_path) + .with_context(|| format!("failed to read {}", marker_path.display()))?; + let marker: InstalledFromMarker = serde_json::from_str(&marker_body) + .with_context(|| format!("malformed {} for {name}", INSTALLED_FROM_MARKER))?; + + // Re-resolve the URL, taking the existing checksum as a short-circuit hint: + // we still hit the network so the user gets a useful "no upstream change" + // signal, but we skip the unpack step if the bytes match. + let source = InstallSource::parse(&marker.spec)?; + let urls = match candidate_urls(&source, network, registry_url).await? { + UrlResolution::Resolved(urls) => urls, + UrlResolution::NeedsApproval(host) => return Ok(UpdateResult::NeedsApproval(host)), + UrlResolution::Denied(host) => return Ok(UpdateResult::NetworkDenied(host)), + }; + let (bytes, _url) = match download_first_success(&urls, network, max_size).await? { + DownloadOutcome::Bytes { bytes, url } => (bytes, url), + DownloadOutcome::NeedsApproval(host) => return Ok(UpdateResult::NeedsApproval(host)), + DownloadOutcome::Denied(host) => return Ok(UpdateResult::NetworkDenied(host)), + }; + + let mut hasher = Sha256::new(); + hasher.update(&bytes); + let checksum = format!("{:x}", hasher.finalize()); + if checksum == marker.checksum { + return Ok(UpdateResult::NoChange); + } + + // Bytes changed — fall back to the regular install path with `update = true` + // so we get the same atomic-replace semantics. + let outcome = + install_with_registry(source, skills_dir, max_size, network, true, registry_url).await?; + match outcome { + InstallOutcome::Installed(installed) => Ok(UpdateResult::Updated(installed)), + InstallOutcome::NeedsApproval(host) => Ok(UpdateResult::NeedsApproval(host)), + InstallOutcome::NetworkDenied(host) => Ok(UpdateResult::NetworkDenied(host)), + } +} + +/// Remove a community-installed skill. +/// +/// Refuses to touch any directory that doesn't carry the `.installed-from` +/// marker — that's our cue that it's user-owned and not a system skill. +pub fn uninstall(name: &str, skills_dir: &Path) -> Result<()> { + let target = skills_dir.join(name); + if !target.exists() { + bail!("skill '{name}' is not installed at {}", target.display()); + } + if !target.join(INSTALLED_FROM_MARKER).exists() { + return Err(InstallError::NotInstalledHere(name.to_string()).into()); + } + fs::remove_dir_all(&target) + .with_context(|| format!("failed to remove {}", target.display()))?; + Ok(()) +} + +/// Mark a community-installed skill as trusted. Currently a marker file only; +/// callers that wire tool execution against `/scripts/` consult the file +/// before invoking anything. No-op if already trusted. +/// +/// Refuses to mark system skills (no `.installed-from`) so the bundled +/// `skill-creator` doesn't accidentally inherit elevated tool privileges. +pub fn trust(name: &str, skills_dir: &Path) -> Result<()> { + let target = skills_dir.join(name); + if !target.exists() { + bail!("skill '{name}' is not installed at {}", target.display()); + } + if !target.join(INSTALLED_FROM_MARKER).exists() { + return Err(InstallError::NotInstalledHere(name.to_string()).into()); + } + let marker = target.join(TRUSTED_MARKER); + if !marker.exists() { + fs::write( + &marker, + "Skill scripts/ are user-trusted. Delete this file to revoke.\n", + ) + .with_context(|| format!("failed to write {}", marker.display()))?; + } + Ok(()) +} + +/// Fetch the curated registry and return the parsed entries. +/// +/// Honours `network` (skipping the call entirely on Deny / Prompt). +pub async fn fetch_registry( + network: &NetworkPolicy, + registry_url: &str, +) -> Result { + let host = match host_from_url(registry_url) { + Some(host) => host, + None => bail!("invalid registry url: {registry_url}"), + }; + match network.decide(&host) { + Decision::Allow => {} + Decision::Deny => return Ok(RegistryFetchResult::Denied(host)), + Decision::Prompt => return Ok(RegistryFetchResult::NeedsApproval(host)), + } + let body = reqwest::get(registry_url) + .await + .with_context(|| format!("failed to fetch registry {registry_url}"))? + .error_for_status() + .with_context(|| format!("registry {registry_url} returned an error status"))? + .text() + .await + .with_context(|| format!("failed to read registry body from {registry_url}"))?; + let parsed: RegistryDocument = serde_json::from_str(&body) + .with_context(|| format!("failed to parse registry json from {registry_url}"))?; + Ok(RegistryFetchResult::Loaded(parsed)) +} + +// ───────────────────────────────────────────────────────────────────────────── +// Internal helpers +// ───────────────────────────────────────────────────────────────────────────── + +#[derive(Debug, Deserialize)] +struct InstalledFromMarker { + spec: String, + #[serde(default)] + checksum: String, +} + +/// Curated-registry document. The shape is intentionally minimal so adding +/// optional metadata later (homepage, version, signature) is forward-compatible. +#[derive(Debug, Clone, Deserialize)] +pub struct RegistryDocument { + /// Map of skill name → entry. + #[serde(default)] + pub skills: std::collections::BTreeMap, +} + +/// One row in the curated registry. `description` is optional so old indices +/// keep parsing. +#[derive(Debug, Clone, Deserialize)] +pub struct RegistryEntry { + /// Source spec (e.g. `github:owner/repo`). + pub source: String, + /// Optional human-readable description. + #[serde(default)] + pub description: Option, +} + +/// Successful registry fetch result. Same shape as [`InstallOutcome`] for the +/// network-policy outcomes so the caller can drop directly into approval flow. +#[derive(Debug)] +pub enum RegistryFetchResult { + Loaded(RegistryDocument), + NeedsApproval(String), + Denied(String), +} + +enum UrlResolution { + Resolved(Vec), + NeedsApproval(String), + Denied(String), +} + +enum DownloadOutcome { + Bytes { bytes: Vec, url: String }, + NeedsApproval(String), + Denied(String), +} + +/// Resolve the source spec into one or more candidate URLs to try in order. +async fn candidate_urls( + source: &InstallSource, + network: &NetworkPolicy, + registry_url: &str, +) -> Result { + match source { + InstallSource::GitHubRepo(repo) => { + // GitHub's archive endpoint lives on `codeload.github.com` after + // the redirect, but the public URL we hit is `github.com`. Both + // typically appear in user allow lists; we check the canonical + // host. + Ok(UrlResolution::Resolved(vec![ + format!("https://github.com/{repo}/archive/refs/heads/main.tar.gz"), + format!("https://github.com/{repo}/archive/refs/heads/master.tar.gz"), + ])) + } + InstallSource::DirectUrl(url) => Ok(UrlResolution::Resolved(vec![url.clone()])), + InstallSource::Registry(name) => { + match fetch_registry(network, registry_url).await? { + RegistryFetchResult::Loaded(doc) => { + let entry = doc + .skills + .get(name) + .with_context(|| format!("skill '{name}' not found in registry"))? + .clone(); + let inner = InstallSource::parse(&entry.source).with_context(|| { + format!( + "registry entry for '{name}' has invalid source: {}", + entry.source + ) + })?; + // Recurse only one level — registry pointing at registry is + // disallowed to avoid cycles. + if matches!(inner, InstallSource::Registry(_)) { + bail!("registry entry for '{name}' must not point to another registry"); + } + // Reuse this function for the inner source so GitHub fallback + // still applies. + Box::pin(candidate_urls(&inner, network, registry_url)).await + } + RegistryFetchResult::NeedsApproval(host) => Ok(UrlResolution::NeedsApproval(host)), + RegistryFetchResult::Denied(host) => Ok(UrlResolution::Denied(host)), + } + } + } +} + +/// Download the first URL whose host the policy allows and which returns 2xx. +/// Returns `NeedsApproval` if every candidate hit `Prompt`, or `Denied` if every +/// candidate was denied. +async fn download_first_success( + urls: &[String], + network: &NetworkPolicy, + max_size: u64, +) -> Result { + let mut last_status: Option = None; + let mut prompt_host: Option = None; + let mut denied_host: Option = None; + for url in urls { + let host = match host_from_url(url) { + Some(h) => h, + None => bail!("invalid download url: {url}"), + }; + match network.decide(&host) { + Decision::Allow => {} + Decision::Deny => { + denied_host.get_or_insert(host); + continue; + } + Decision::Prompt => { + prompt_host.get_or_insert(host); + continue; + } + } + match download_with_cap(url, max_size).await? { + DownloadAttempt::Bytes(bytes) => { + return Ok(DownloadOutcome::Bytes { + bytes, + url: url.clone(), + }); + } + DownloadAttempt::NotFound(status) => { + last_status = Some(status); + continue; + } + } + } + if let Some(host) = denied_host { + return Ok(DownloadOutcome::Denied(host)); + } + if let Some(host) = prompt_host { + return Ok(DownloadOutcome::NeedsApproval(host)); + } + bail!( + "failed to download skill (last status: {})", + last_status + .map(|s| s.to_string()) + .unwrap_or_else(|| "unknown".to_string()) + ); +} + +enum DownloadAttempt { + Bytes(Vec), + NotFound(reqwest::StatusCode), +} + +/// Stream a URL into memory with a size cap. Aborts on the first read that +/// would push the buffer over `max_size * 4` (the *4 accounts for compression; +/// the unpack step still enforces `max_size` on the *uncompressed* bytes). +async fn download_with_cap(url: &str, max_size: u64) -> Result { + let resp = reqwest::get(url) + .await + .with_context(|| format!("failed to GET {url}"))?; + let status = resp.status(); + if !status.is_success() { + if status == reqwest::StatusCode::NOT_FOUND { + return Ok(DownloadAttempt::NotFound(status)); + } + bail!("download {url} returned {status}"); + } + // Soft cap on the *compressed* download — well above max_size to allow + // for highly compressible payloads but still bounded. + let compressed_cap = max_size.saturating_mul(4); + let bytes = resp + .bytes() + .await + .with_context(|| format!("failed to read body of {url}"))?; + if (bytes.len() as u64) > compressed_cap { + bail!("download {url} exceeds compressed size cap of {compressed_cap} bytes"); + } + Ok(DownloadAttempt::Bytes(bytes.to_vec())) +} + +struct StagedSkill { + skill_name: String, + staged_path: PathBuf, +} + +/// Validate a tarball and extract it into `/.tmp/`. +fn stage_tarball(bytes: &[u8], skills_dir: &Path, max_size: u64) -> Result { + fs::create_dir_all(skills_dir) + .with_context(|| format!("failed to create skills directory {}", skills_dir.display()))?; + + // Two passes: first determine the skill name (and therefore the staged + // dir) by finding the SKILL.md, then extract under that staged dir. + // Both passes share the same archive bytes; we reset by wrapping fresh + // decoders. + + let scan = scan_tarball(bytes, max_size)?; + + // Prepare staged directory. Use a `.tmp` suffix so a crashed install + // never collides with a real name; remove any leftover from a prior + // failed attempt. + let staged_path = skills_dir.join(format!("{}.tmp", scan.skill_name)); + if staged_path.exists() { + fs::remove_dir_all(&staged_path).with_context(|| { + format!( + "failed to clean stale staging dir {}", + staged_path.display() + ) + })?; + } + fs::create_dir_all(&staged_path) + .with_context(|| format!("failed to create staging dir {}", staged_path.display()))?; + + // Second pass — extract. + let result = extract_into(&scan, bytes, &staged_path, max_size); + if let Err(err) = result { + // Cleanup on failure so a half-staged directory doesn't survive. + let _ = fs::remove_dir_all(&staged_path); + return Err(err); + } + + Ok(StagedSkill { + skill_name: scan.skill_name, + staged_path, + }) +} + +struct TarballScan { + /// Skill name from SKILL.md frontmatter. + skill_name: String, + /// Archive prefix to strip from each entry (e.g. `repo-main/`). May be empty. + prefix: String, + /// Sub-directory inside `prefix` that the SKILL.md lives in (`""` if root, + /// or `skills/` for repos that bundle multiple skills). + skill_root: String, +} + +/// 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. +fn scan_tarball(bytes: &[u8], max_size: u64) -> Result { + let cursor = std::io::Cursor::new(bytes); + let gz = GzDecoder::new(cursor); + let mut archive = tar::Archive::new(gz); + + let mut total_size: u64 = 0; + let mut prefix: Option = None; + let mut skill_md_relative: Option<(String, Vec)> = None; + + for entry in archive + .entries() + .context("failed to read tar entries (corrupt archive?)")? + { + 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")? + .to_path_buf(); + let path_str = path.to_string_lossy().into_owned(); + if !is_safe_path(&path) { + return Err(InstallError::PathTraversal(path_str).into()); + } + + // Track total size against `max_size` (uncompressed). We honor `header + // .size` rather than streaming-read every file; tar archives are + // self-describing so this is reliable for non-malicious inputs and + // catches the gzip-bomb case. + if let Ok(size) = header.size() { + total_size = total_size.saturating_add(size); + if total_size > max_size { + return Err(InstallError::OversizedTarball { limit: max_size }.into()); + } + } + + // Detect prefix from the first entry. GitHub archives wrap everything + // in `-/`; direct tarballs may have no prefix. We treat + // the first path component as the prefix iff the archive has more than + // one entry under it, but for SKILL.md detection we just strip the + // first component if every entry shares it. + if prefix.is_none() { + if let Some(Component::Normal(first)) = path.components().next() { + let candidate = first.to_string_lossy().into_owned(); + // Only treat the first component as a prefix if it's a + // directory-like (no extension and the path has more + // components). Otherwise leave prefix empty. + if path.components().count() > 1 { + prefix = Some(candidate); + } else { + prefix = Some(String::new()); + } + } else { + prefix = Some(String::new()); + } + } + + // SKILL.md detection. Match either: + // * `/SKILL.md` + // * `/skills//SKILL.md` + if entry_type.is_file() { + let stripped = strip_prefix(&path_str, prefix.as_deref().unwrap_or("")); + if stripped.eq_ignore_ascii_case("SKILL.md") + || stripped.starts_with("skills/") + && stripped.ends_with("/SKILL.md") + && stripped.matches('/').count() == 2 + { + let mut buf = Vec::new(); + entry + .read_to_end(&mut buf) + .context("failed to read SKILL.md from archive")?; + // Prefer the first match — we don't support multi-skill + // archives where a tarball ships several SKILL.mds at once. + if skill_md_relative.is_none() { + skill_md_relative = Some((stripped.to_string(), buf)); + } + } + } + } + + let prefix = prefix.unwrap_or_default(); + let (skill_md_path, skill_md_bytes) = skill_md_relative + .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 { + // strip trailing /SKILL.md + skill_md_path + .strip_suffix("/SKILL.md") + .unwrap_or("") + .to_string() + }; + + Ok(TarballScan { + skill_name: name, + prefix, + skill_root, + }) +} + +fn extract_into(scan: &TarballScan, bytes: &[u8], dest: &Path, max_size: u64) -> Result<()> { + let cursor = std::io::Cursor::new(bytes); + let gz = GzDecoder::new(cursor); + let mut archive = tar::Archive::new(gz); + + let mut total_size: u64 = 0; + let prefix_with_root = if scan.skill_root.is_empty() { + scan.prefix.clone() + } else if scan.prefix.is_empty() { + scan.skill_root.clone() + } else { + format!("{}/{}", scan.prefix, scan.skill_root) + }; + + for entry in archive + .entries() + .context("failed to read tar entries (corrupt archive?)")? + { + 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")? + .to_path_buf(); + let path_str = path.to_string_lossy().into_owned(); + if !is_safe_path(&path) { + return Err(InstallError::PathTraversal(path_str).into()); + } + + // Only extract entries that live under our skill root. For simple + // tarballs (`SKILL.md` at root) that's everything; for multi-skill + // repos it's the `skills//` slice. + let stripped = strip_prefix(&path_str, &prefix_with_root).into_owned(); + if stripped.is_empty() && entry_type.is_dir() { + // The root directory itself — already created. + continue; + } + if stripped == path_str && !prefix_with_root.is_empty() { + // Nothing to strip => entry is outside our subtree, skip. + continue; + } + // Defense-in-depth: re-validate the stripped path. + let stripped_path = Path::new(&stripped); + if !is_safe_path(stripped_path) { + return Err(InstallError::PathTraversal(stripped).into()); + } + + let target = dest.join(stripped_path); + // Final paranoia check: ensure the resolved target stays under dest. + // We can't canonicalize (target doesn't exist yet), so we walk + // components one more time after composing. + let target_components: Vec<_> = target.components().collect(); + let dest_components: Vec<_> = dest.components().collect(); + if !target_components.starts_with(dest_components.as_slice()) { + return Err(InstallError::PathTraversal(stripped).into()); + } + + if entry_type.is_dir() { + fs::create_dir_all(&target) + .with_context(|| format!("failed to create dir {}", target.display()))?; + continue; + } + if entry_type.is_file() { + if let Some(parent) = target.parent() { + fs::create_dir_all(parent) + .with_context(|| format!("failed to create dir {}", parent.display()))?; + } + // Read into a buffer so we can enforce `max_size`. Files inside + // a SKILL bundle are small; copying through a buffer is fine. + let mut buf = Vec::new(); + entry + .read_to_end(&mut buf) + .with_context(|| format!("failed to read {}", path.display()))?; + total_size = total_size.saturating_add(buf.len() as u64); + if total_size > max_size { + return Err(InstallError::OversizedTarball { limit: max_size }.into()); + } + let mut out = fs::OpenOptions::new() + .create_new(true) + .write(true) + .open(&target) + .with_context(|| format!("failed to create {}", target.display()))?; + out.write_all(&buf) + .with_context(|| format!("failed to write {}", target.display()))?; + } + } + Ok(()) +} + +/// Ensure a tar path has no `..` segments and is not absolute. +fn is_safe_path(path: &Path) -> bool { + if path.is_absolute() { + return false; + } + for component in path.components() { + match component { + Component::ParentDir => return false, + Component::Prefix(_) | Component::RootDir => return false, + _ => {} + } + } + true +} + +/// Strip a leading directory prefix (e.g. `repo-main/`) from a tarball path. +fn strip_prefix<'a>(path: &'a str, prefix: &str) -> std::borrow::Cow<'a, str> { + if prefix.is_empty() { + return std::borrow::Cow::Borrowed(path); + } + let with_slash = format!("{prefix}/"); + if let Some(rest) = path.strip_prefix(&with_slash) { + std::borrow::Cow::Owned(rest.to_string()) + } else if path == prefix { + std::borrow::Cow::Borrowed("") + } else { + std::borrow::Cow::Borrowed(path) + } +} + +/// Extract `name:` and ensure `description:` exist in the SKILL.md frontmatter. +/// Also verifies the leading `---` fence so we reject malformed files early. +fn parse_frontmatter_name(bytes: &[u8]) -> Result { + let content = std::str::from_utf8(bytes).context("SKILL.md is not valid UTF-8")?; + let trimmed = content.trim_start(); + if !trimmed.starts_with("---") { + bail!("SKILL.md is missing the leading '---' frontmatter fence"); + } + let after_open = &trimmed[3..]; + let close = after_open.find("---").ok_or_else(|| { + anyhow::anyhow!("SKILL.md is missing the closing '---' frontmatter fence") + })?; + let frontmatter = &after_open[..close]; + + let mut name: Option = None; + let mut has_description = false; + for raw in frontmatter.lines() { + let line = raw.trim(); + if line.is_empty() || line.starts_with('#') { + continue; + } + if let Some((key, value)) = line.split_once(':') { + let key = key.trim().to_ascii_lowercase(); + let value = value.trim().to_string(); + match key.as_str() { + "name" if !value.is_empty() => name = Some(value), + "description" if !value.is_empty() => has_description = true, + _ => {} + } + } + } + + let name = name.ok_or(InstallError::MissingFrontmatterField("name"))?; + if !has_description { + return Err(InstallError::MissingFrontmatterField("description").into()); + } + // Sanity check: name must be a single path-safe segment. + if name.contains('/') + || name.contains('\\') + || name == "." + || name == ".." + || name.contains(' ') + { + bail!("SKILL.md `name` must be a single path-safe segment (got '{name}')"); + } + Ok(name) +} + +fn source_spec_string(source: &InstallSource) -> String { + match source { + InstallSource::GitHubRepo(repo) => format!("github:{repo}"), + InstallSource::DirectUrl(url) => url.clone(), + InstallSource::Registry(name) => name.clone(), + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Tests +// ───────────────────────────────────────────────────────────────────────────── + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parse_github_source() { + let s = InstallSource::parse("github:Hmbown/test-skill").unwrap(); + assert_eq!( + s, + InstallSource::GitHubRepo("Hmbown/test-skill".to_string()) + ); + } + + #[test] + fn parse_github_source_rejects_missing_repo() { + let err = InstallSource::parse("github:Hmbown").unwrap_err(); + assert!(err.to_string().contains("github source must"), "got: {err}"); + } + + #[test] + fn parse_github_source_rejects_extra_slashes() { + let err = InstallSource::parse("github:Hmbown/repo/extra").unwrap_err(); + assert!(err.to_string().contains("github source must"), "got: {err}"); + } + + #[test] + fn parse_direct_url_source() { + let s = InstallSource::parse("https://example.com/skill.tar.gz").unwrap(); + assert_eq!( + s, + InstallSource::DirectUrl("https://example.com/skill.tar.gz".to_string()) + ); + let s = InstallSource::parse("http://example.com/skill.tar.gz").unwrap(); + assert_eq!( + s, + InstallSource::DirectUrl("http://example.com/skill.tar.gz".to_string()) + ); + } + + #[test] + fn parse_registry_source() { + let s = InstallSource::parse("my-skill").unwrap(); + assert_eq!(s, InstallSource::Registry("my-skill".to_string())); + } + + #[test] + fn parse_rejects_empty() { + assert!(InstallSource::parse("").is_err()); + assert!(InstallSource::parse(" ").is_err()); + } + + #[test] + fn is_safe_path_rejects_traversal() { + assert!(!is_safe_path(Path::new("../etc/passwd"))); + assert!(!is_safe_path(Path::new("foo/../bar"))); + assert!(!is_safe_path(Path::new("/etc/passwd"))); + assert!(is_safe_path(Path::new("foo/bar/baz"))); + assert!(is_safe_path(Path::new("SKILL.md"))); + } + + #[test] + fn parse_frontmatter_extracts_name() { + let body = b"---\nname: hello\ndescription: greeter\n---\nbody\n"; + assert_eq!(parse_frontmatter_name(body).unwrap(), "hello"); + } + + #[test] + fn parse_frontmatter_missing_name_fails() { + let body = b"---\ndescription: x\n---\n"; + let err = parse_frontmatter_name(body).unwrap_err(); + assert!(format!("{err}").contains("name")); + } + + #[test] + fn parse_frontmatter_missing_description_fails() { + let body = b"---\nname: x\n---\n"; + let err = parse_frontmatter_name(body).unwrap_err(); + assert!(format!("{err}").contains("description")); + } + + #[test] + fn parse_frontmatter_rejects_unsafe_name() { + let body = b"---\nname: ../evil\ndescription: x\n---\n"; + assert!(parse_frontmatter_name(body).is_err()); + + let body = b"---\nname: a name with spaces\ndescription: x\n---\n"; + assert!(parse_frontmatter_name(body).is_err()); + } + + #[test] + fn parse_frontmatter_requires_opening_fence() { + let body = b"name: hello\ndescription: x\n"; + assert!(parse_frontmatter_name(body).is_err()); + } + + #[test] + fn strip_prefix_handles_all_cases() { + assert_eq!(strip_prefix("foo/bar", "foo"), "bar"); + assert_eq!(strip_prefix("foo", "foo"), ""); + assert_eq!(strip_prefix("baz/bar", "foo"), "baz/bar"); + assert_eq!(strip_prefix("foo/bar", ""), "foo/bar"); + } + + #[test] + fn source_spec_string_roundtrips() { + assert_eq!( + source_spec_string(&InstallSource::GitHubRepo("a/b".into())), + "github:a/b" + ); + assert_eq!( + source_spec_string(&InstallSource::DirectUrl("https://x".into())), + "https://x" + ); + assert_eq!( + source_spec_string(&InstallSource::Registry("x".into())), + "x" + ); + } +} diff --git a/crates/tui/src/skills/mod.rs b/crates/tui/src/skills/mod.rs index 46932466..88207e82 100644 --- a/crates/tui/src/skills/mod.rs +++ b/crates/tui/src/skills/mod.rs @@ -1,6 +1,17 @@ //! Skill discovery and registry for local SKILL.md files. +pub mod install; mod system; +// Re-exports kept for documentation parity and downstream consumers; the +// binary itself imports directly from `skills::install`. `#[allow(...)]` +// silences the dead-code warning that fires because no `bin` source path +// references these names through `skills::*`. +#[allow(unused_imports)] +pub use install::{ + DEFAULT_MAX_SIZE_BYTES, DEFAULT_REGISTRY_URL, INSTALLED_FROM_MARKER, InstallOutcome, + InstallSource, InstalledSkill, RegistryDocument, RegistryEntry, RegistryFetchResult, + UpdateResult, +}; pub use system::install_system_skills; use std::fs; diff --git a/crates/tui/tests/skill_install.rs b/crates/tui/tests/skill_install.rs new file mode 100644 index 00000000..4789f7b5 --- /dev/null +++ b/crates/tui/tests/skill_install.rs @@ -0,0 +1,522 @@ +//! Integration tests for the community-skill installer (#140). +//! +//! These tests exercise the full validation pipeline against a tiny in-process +//! HTTP server, so the network gate, download cap, tarball validation, atomic +//! rename, and `.installed-from` marker all run end-to-end. The module is +//! pulled in via `#[path]` includes (matching `integration_mock_llm.rs`) so we +//! get access to private helpers without a separate library crate. + +use std::io::Write; +use std::path::Path; + +use flate2::Compression; +use flate2::write::GzEncoder; +use tempfile::TempDir; +use tiny_http::{Method, Response, Server}; + +// Pull the production source files into this test binary so the test can +// reach `install`'s public surface without a dedicated library crate. +// +// `install.rs` only references `crate::network_policy` so we just need that +// one helper module alongside `install` itself. +#[path = "../src/network_policy.rs"] +mod network_policy; + +#[path = "../src/skills/install.rs"] +#[allow(dead_code)] +mod install; + +use crate::install::{InstallOutcome, InstallSource, UpdateResult}; +use crate::network_policy::{DecisionToml, NetworkPolicy}; + +/// Construct a gzipped tarball from `(path, body)` pairs. Permissions are set +/// to 0o644 so umask differences across platforms don't perturb the bytes. +fn make_tarball(entries: &[(&str, &[u8])]) -> Vec { + let mut gz = GzEncoder::new(Vec::new(), Compression::default()); + { + let mut builder = tar::Builder::new(&mut gz); + for (path, body) in entries { + let mut header = tar::Header::new_gnu(); + header.set_size(body.len() as u64); + header.set_mode(0o644); + header.set_cksum(); + builder + .append_data(&mut header, path, *body) + .expect("append_data"); + } + builder.finish().expect("finish tar"); + } + gz.finish().expect("finish gz") +} + +fn skill_md(name: &str, description: &str) -> Vec { + format!( + "---\nname: {name}\ndescription: {description}\n---\n# {name}\n\nThis is a test skill.\n" + ) + .into_bytes() +} + +fn allow_all_policy() -> NetworkPolicy { + NetworkPolicy { + default: DecisionToml::Allow, + allow: Vec::new(), + deny: Vec::new(), + audit: false, + } +} + +fn deny_all_policy() -> NetworkPolicy { + NetworkPolicy { + default: DecisionToml::Deny, + allow: Vec::new(), + deny: Vec::new(), + audit: false, + } +} + +fn prompt_all_policy() -> NetworkPolicy { + NetworkPolicy { + default: DecisionToml::Prompt, + allow: Vec::new(), + deny: Vec::new(), + audit: false, + } +} + +/// Spawn a tiny HTTP server that serves `bytes` at any path with 200 OK and +/// returns the bound URL. The server replies to *every* request (we re-use it +/// across multiple installs in the same test). +fn spawn_tarball_server( + bytes: Vec, +) -> ( + String, + std::sync::mpsc::Sender<()>, + std::thread::JoinHandle<()>, +) { + let server = Server::http("127.0.0.1:0").expect("bind ephemeral port"); + let url = format!( + "http://{}/skill.tar.gz", + server.server_addr().to_ip().expect("ip addr") + ); + let (shutdown_tx, shutdown_rx) = std::sync::mpsc::channel::<()>(); + let handle = std::thread::spawn(move || { + loop { + // Poll-style with a small recv timeout so we can break out cleanly. + match server.recv_timeout(std::time::Duration::from_millis(100)) { + Ok(Some(req)) => { + if req.method() != &Method::Get { + continue; + } + let response = Response::from_data(bytes.clone()); + let _ = req.respond(response); + } + Ok(None) => {} + Err(_) => break, + } + if shutdown_rx.try_recv().is_ok() { + break; + } + } + }); + (url, shutdown_tx, handle) +} + +fn shutdown(tx: std::sync::mpsc::Sender<()>, handle: std::thread::JoinHandle<()>) { + let _ = tx.send(()); + let _ = handle.join(); +} + +#[tokio::test] +async fn install_happy_path_writes_skill_and_marker() { + let tarball = make_tarball(&[ + ( + "test-skill-main/SKILL.md", + &skill_md("test-skill", "Test skill"), + ), + ("test-skill-main/notes.txt", b"hello world"), + ]); + 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("install ok"); + + let installed = match outcome { + InstallOutcome::Installed(s) => s, + other => panic!("expected Installed, got {other:?}"), + }; + assert_eq!(installed.name, "test-skill"); + + let installed_dir = tmp.path().join("test-skill"); + assert!(installed_dir.is_dir(), "skill dir created"); + assert!(installed_dir.join("SKILL.md").is_file(), "SKILL.md present"); + assert!( + installed_dir.join("notes.txt").is_file(), + "extra file present" + ); + assert!( + installed_dir.join(install::INSTALLED_FROM_MARKER).is_file(), + ".installed-from marker present" + ); + + shutdown(tx, handle); +} + +#[tokio::test] +async fn install_rejects_path_traversal() { + // `tar::Builder::append_data` rejects `..` itself, so we craft the bad + // entry by writing the raw header bytes via `append`. + let mut gz = GzEncoder::new(Vec::new(), Compression::default()); + { + let mut builder = tar::Builder::new(&mut gz); + let body = skill_md("test-skill", "T"); + 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, "test-skill-main/SKILL.md", body.as_slice()) + .unwrap(); + + // Path-traversal entry. The `tar` crate's `set_path` rejects `..` + // itself, so we patch the raw 100-byte name field in the header. + let evil_body: &[u8] = b"not gonna happen"; + let mut evil_hdr = tar::Header::new_gnu(); + evil_hdr.set_size(evil_body.len() as u64); + evil_hdr.set_mode(0o644); + // Write a name with a `..` directly into the legacy "name" field. + let bytes = evil_hdr.as_old_mut(); + let evil_name = b"../etc/passwd"; + bytes.name[..evil_name.len()].copy_from_slice(evil_name); + evil_hdr.set_cksum(); + builder.append(&evil_hdr, evil_body).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 err = install::install( + InstallSource::DirectUrl(url), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect_err("path traversal must be rejected"); + let msg = format!("{err:#}"); + assert!( + msg.contains("escapes destination"), + "expected path-traversal error, got: {msg}" + ); + + shutdown(tx, handle); +} + +#[tokio::test] +async fn install_rejects_oversized_tarball() { + let big = vec![b'a'; 256 * 1024]; // 256 KiB per file + let mut entries: Vec<(String, Vec)> = Vec::new(); + entries.push(( + "test-skill-main/SKILL.md".to_string(), + skill_md("test-skill", "T"), + )); + for i in 0..50 { + entries.push((format!("test-skill-main/big-{i}.bin"), big.clone())); + } + let entry_refs: Vec<(&str, &[u8])> = entries + .iter() + .map(|(p, b)| (p.as_str(), b.as_slice())) + .collect(); + let tarball = make_tarball(&entry_refs); + let (url, tx, handle) = spawn_tarball_server(tarball); + + let tmp = TempDir::new().unwrap(); + let policy = allow_all_policy(); + let small_cap = 1024 * 1024; + let err = install::install( + InstallSource::DirectUrl(url), + tmp.path(), + small_cap, + &policy, + false, + ) + .await + .expect_err("oversized must be rejected"); + let msg = format!("{err:#}"); + assert!( + msg.contains("too large") || msg.contains("exceed"), + "expected size cap error, got: {msg}" + ); + + shutdown(tx, handle); +} + +#[tokio::test] +async fn install_rejects_missing_skill_md() { + let tarball = make_tarball(&[("repo-main/README.md", b"not a skill")]); + let (url, tx, handle) = spawn_tarball_server(tarball); + + let tmp = TempDir::new().unwrap(); + let policy = allow_all_policy(); + let err = install::install( + InstallSource::DirectUrl(url), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect_err("missing SKILL.md must be rejected"); + assert!(format!("{err:#}").contains("missing SKILL.md"), "{err:#}"); + + shutdown(tx, handle); +} + +#[tokio::test] +async fn install_rejects_missing_required_frontmatter() { + let tarball = make_tarball(&[("repo-main/SKILL.md", b"---\nname: test\n---\nbody\n")]); + let (url, tx, handle) = spawn_tarball_server(tarball); + + let tmp = TempDir::new().unwrap(); + let policy = allow_all_policy(); + let err = install::install( + InstallSource::DirectUrl(url), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect_err("missing description must be rejected"); + assert!(format!("{err:#}").contains("description"), "{err:#}"); + + shutdown(tx, handle); +} + +#[tokio::test] +async fn install_idempotent_then_uninstall_then_reinstall() { + let tarball_bytes = + make_tarball(&[("repo-main/SKILL.md", &skill_md("idem-skill", "Idempotent"))]); + let (url, tx, handle) = spawn_tarball_server(tarball_bytes); + + let tmp = TempDir::new().unwrap(); + let policy = allow_all_policy(); + + install::install( + InstallSource::DirectUrl(url.clone()), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect("first install ok"); + + // Second install with `update = false` must reject. + let err = install::install( + InstallSource::DirectUrl(url.clone()), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect_err("second install must reject"); + let msg = format!("{err:#}"); + assert!( + msg.contains("already installed"), + "expected already-installed error, got: {msg}" + ); + + // Uninstall then reinstall. + install::uninstall("idem-skill", tmp.path()).expect("uninstall ok"); + assert!(!tmp.path().join("idem-skill").exists()); + + install::install( + InstallSource::DirectUrl(url), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect("reinstall ok"); + + assert!(tmp.path().join("idem-skill").join("SKILL.md").is_file()); + shutdown(tx, handle); +} + +#[tokio::test] +async fn update_no_change_returns_nochange_without_overwriting() { + let tarball_bytes = + make_tarball(&[("repo-main/SKILL.md", &skill_md("upd-skill", "Update test"))]); + let (url, tx, handle) = spawn_tarball_server(tarball_bytes); + let tmp = TempDir::new().unwrap(); + let policy = allow_all_policy(); + + install::install( + InstallSource::DirectUrl(url.clone()), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .unwrap(); + + // Patch the marker so update() re-fetches the same URL. + let marker_path = tmp + .path() + .join("upd-skill") + .join(install::INSTALLED_FROM_MARKER); + let marker_body = std::fs::read_to_string(&marker_path).unwrap(); + let mut marker_json: serde_json::Value = serde_json::from_str(&marker_body).unwrap(); + marker_json["spec"] = serde_json::Value::String(url); + std::fs::write(&marker_path, marker_json.to_string()).unwrap(); + + // Capture mtime so we can confirm SKILL.md wasn't rewritten. + let skill_md_path = tmp.path().join("upd-skill").join("SKILL.md"); + let mtime_before = std::fs::metadata(&skill_md_path) + .unwrap() + .modified() + .unwrap(); + + let result = install::update( + "upd-skill", + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + ) + .await + .expect("update ok"); + assert!(matches!(result, UpdateResult::NoChange)); + + let mtime_after = std::fs::metadata(&skill_md_path) + .unwrap() + .modified() + .unwrap(); + assert_eq!(mtime_before, mtime_after, "SKILL.md must not be rewritten"); + shutdown(tx, handle); +} + +#[tokio::test] +async fn install_with_deny_policy_returns_network_denied() { + let tmp = TempDir::new().unwrap(); + let policy = deny_all_policy(); + let outcome = install::install( + InstallSource::DirectUrl("https://example.invalid/skill.tar.gz".to_string()), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect("policy outcome should be Ok"); + match outcome { + InstallOutcome::NetworkDenied(host) => { + assert!(host.contains("example.invalid"), "got host {host}"); + } + other => panic!("expected NetworkDenied, got {other:?}"), + } + + // Verify the temp dir is untouched. + assert!( + std::fs::read_dir(tmp.path()).unwrap().next().is_none(), + "temp dir must be untouched" + ); +} + +#[tokio::test] +async fn install_with_prompt_policy_returns_needs_approval() { + let tmp = TempDir::new().unwrap(); + let policy = prompt_all_policy(); + let outcome = install::install( + InstallSource::DirectUrl("https://example.invalid/skill.tar.gz".to_string()), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect("policy outcome should be Ok"); + match outcome { + InstallOutcome::NeedsApproval(host) => { + assert!(host.contains("example.invalid"), "got host {host}"); + } + other => panic!("expected NeedsApproval, got {other:?}"), + } + assert!( + std::fs::read_dir(tmp.path()).unwrap().next().is_none(), + "temp dir must be untouched on prompt" + ); +} + +#[tokio::test] +async fn install_rejects_symlink_entry() { + let mut gz = GzEncoder::new(Vec::new(), Compression::default()); + { + let mut builder = tar::Builder::new(&mut gz); + + let body = skill_md("link-skill", "x"); + 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/SKILL.md", body.as_slice()) + .unwrap(); + + 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/escape", Path::new("/etc/passwd")) + .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 err = install::install( + InstallSource::DirectUrl(url), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect_err("symlinks must be rejected"); + assert!(format!("{err:#}").contains("symlink"), "{err:#}"); + + shutdown(tx, handle); +} + +#[test] +fn uninstall_refuses_system_skill() { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path().join("system-skill"); + std::fs::create_dir_all(&dir).unwrap(); + let mut f = std::fs::File::create(dir.join("SKILL.md")).unwrap(); + f.write_all(b"---\nname: system-skill\ndescription: x\n---\n") + .unwrap(); + // No `.installed-from` marker — looks like a system skill. + + let err = install::uninstall("system-skill", tmp.path()).expect_err("must refuse"); + assert!(format!("{err:#}").contains("not installed via")); + assert!(dir.exists(), "directory must be left alone"); +}