refactor(skills): export install safety primitives

crates/tui/src/plugins/install/mod.rs

@@ -5,7 +5,8 @@
 //! root (`~/.codewhale/plugins/<name>/`). This module deliberately mirrors
 //! [`crate::skills::install`]: the download, network-gating, traversal
 //! rejection, and marker machinery is *reused* from there (`fetch_tarball`,
-//! `is_safe_path`, `write_installed_from_v2`, `INSTALLED_FROM_MARKER`), while
+//! `is_safe_path`, `entry_type_is_link`, `validate_skill_name_segment`,
+//! `write_installed_from_v2`, `INSTALLED_FROM_MARKER`), while
 //! the scan/extract step is plugin-shaped (a bundle is rooted at the single
 //! supported plugin manifest in the tree, not at a `SKILL.md`).
 //!

crates/tui/src/plugins/install/tarball.rs

@@ -15,7 +15,7 @@ use std::path::Path;
 use anyhow::{Context, Result};
 use flate2::read::GzDecoder;
 
-use crate::skills::install::is_safe_path;
+use crate::skills::install::{entry_type_is_link, is_safe_path};
 
 use super::PluginInstallError;
 use super::stage::{StagedPlugin, fresh_staging_dir, validate_staged};
@@ -155,7 +155,7 @@ fn extract_into(scan: &TarballScan, bytes: &[u8], dest: &Path, max_size: u64) ->
         if !is_safe_path(stripped_path) {
             return Err(PluginInstallError::PathTraversal(stripped).into());
         }
-        if entry_type.is_symlink() || entry_type.is_hard_link() {
+        if entry_type_is_link(entry_type) {
             return Err(PluginInstallError::SymlinkRejected.into());
         }
 

crates/tui/src/skills/install.rs

@@ -17,6 +17,16 @@
 //!   `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.
+//! * The safety primitives — [`is_safe_path`], [`entry_type_is_link`],
+//!   [`add_entry_size`], [`validate_skill_name_segment`], and the
+//!   [`InstalledFromMarker`] schema behind [`INSTALLED_FROM_MARKER`] — are the
+//!   embedder-reuse contract: importers that keep their own archives should
+//!   call these instead of maintaining comment-aligned copies of the checks.
+//!   The in-tree plugin installer consumes the judgments verbatim. An
+//!   importer whose sanitizer has deliberately different verdict semantics
+//!   (zip's normalize-and-accept traversal, say) keeps that judgment local
+//!   and registers the divergence with tests rather than restating an
+//!   "aligned" copy.
 //!
 //! # Hard rules
 //!
@@ -86,6 +96,10 @@ const SYNC_REGISTRY_CONCURRENCY: usize = 8;
 
 /// File written under each installed skill so [`update`] / [`uninstall`] can
 /// recover the original [`InstallSource`] without re-parsing user input.
+///
+/// Embedder-reuse contract: embedders that honor ownership-marked installs
+/// must reference this constant rather than restating the marker name, so a
+/// rename or format change here cannot silently desynchronize their copies.
 pub const INSTALLED_FROM_MARKER: &str = ".installed-from";
 
 /// File written under each trusted skill. Currently advisory (the install path
@@ -226,8 +240,10 @@ pub enum UpdateResult {
 }
 
 /// 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.
+/// `anyhow::Error` at the public boundary. In-tree code and tests
+/// pattern-match to avoid parsing strings, and embedders consuming the
+/// exported safety primitives (e.g. [`add_entry_size`]) downcast through
+/// `anyhow::Error` to recover the typed variant.
 #[derive(Debug, Error)]
 pub enum InstallError {
     #[error("entry escapes destination directory: {0}")]
@@ -878,24 +894,40 @@ async fn sync_one_skill(
 // Internal helpers
 // ─────────────────────────────────────────────────────────────────────────────
 
+/// On-disk schema of the [`INSTALLED_FROM_MARKER`] JSON file.
+///
+/// v1 wrote `spec`, `url`, and `checksum`; schema v2 (see
+/// [`write_installed_from_v2`]) keeps those and adds `schema_version`,
+/// `source_checksum`, `content_digest`, `installed_name`, and
+/// `registry_version`. This struct models only the keys an installer or
+/// reader acts on: `url`, `installed_name`, and `registry_version` are
+/// intentionally not modeled. Unknown fields are ignored on read, so newer
+/// writers stay forward-compatible with older readers.
+///
+/// Embedder-reuse contract: embedders reading a marker file deserialize
+/// through this struct rather than redefining the schema.
 #[derive(Debug, Deserialize)]
-pub(crate) struct InstalledFromMarker {
-    pub(crate) spec: String,
+pub struct InstalledFromMarker {
+    /// Install spec the skill was installed from (e.g. `github:owner/repo`).
+    pub spec: String,
     /// v1 download checksum field.
     #[serde(default)]
-    checksum: String,
+    pub checksum: String,
     #[serde(default)]
-    source_checksum: Option<String>,
+    pub source_checksum: Option<String>,
     #[serde(default)]
-    #[allow(dead_code)]
-    schema_version: Option<u32>,
+    pub schema_version: Option<u32>,
     #[serde(default)]
-    #[allow(dead_code)]
-    content_digest: Option<String>,
+    pub content_digest: Option<String>,
 }
 
 impl InstalledFromMarker {
-    pub(crate) fn source_checksum(&self) -> &str {
+    /// Preferred checksum: the v2 `source_checksum` when present and
+    /// non-empty, else the v1 `checksum` field verbatim. A malformed v1
+    /// marker can hold an empty string here; callers comparing against a
+    /// freshly computed digest read that as "changed", the fail-safe
+    /// direction.
+    pub fn source_checksum(&self) -> &str {
         self.source_checksum
             .as_deref()
             .filter(|s| !s.is_empty())
@@ -1270,10 +1302,7 @@ fn scan_tarball(bytes: &[u8], max_size: u64) -> Result<TarballScan> {
         // 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());
-            }
+            total_size = add_entry_size(total_size, size, max_size)?;
         }
 
         // Detect prefix from the first entry. GitHub archives wrap everything
@@ -1297,7 +1326,7 @@ fn scan_tarball(bytes: &[u8], max_size: u64) -> Result<TarballScan> {
             }
         }
 
-        if entry_type.is_symlink() || entry_type.is_hard_link() {
+        if entry_type_is_link(entry_type) {
             link_paths.push(path_str);
             continue;
         }
@@ -1464,7 +1493,7 @@ 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() {
+        if entry_type_is_link(entry_type) {
             return Err(InstallError::SymlinkRejected.into());
         }
 
@@ -1494,10 +1523,7 @@ fn extract_into(scan: &TarballScan, bytes: &[u8], dest: &Path, max_size: u64) ->
             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());
-            }
+            total_size = add_entry_size(total_size, buf.len() as u64, max_size)?;
             let mut out = fs::OpenOptions::new()
                 .create_new(true)
                 .write(true)
@@ -1528,8 +1554,17 @@ fn is_within_selected_root(path: &str, prefix: &str, skill_root: &str) -> bool {
     path == root || path.starts_with(&format!("{root}/"))
 }
 
-/// Ensure a tar path has no `..` segments and is not absolute.
-pub(crate) fn is_safe_path(path: &Path) -> bool {
+/// Whether an archive entry path is safe to join under a destination
+/// directory: rejects absolute paths, Windows prefixes / root components, and
+/// any `..` segment (path traversal).
+///
+/// Embedder-reuse contract: this predicate is the reviewable home of the
+/// "does this entry escape its destination" judgment. Archive importers whose
+/// reject semantics match it call it on each entry path instead of keeping
+/// comment-aligned copies; sanitizers with different verdict semantics (zip's
+/// normalize-and-accept, say) keep theirs local and register the divergence
+/// with tests.
+pub fn is_safe_path(path: &Path) -> bool {
     if path.is_absolute() {
         return false;
     }
@@ -1543,12 +1578,55 @@ pub(crate) fn is_safe_path(path: &Path) -> bool {
     true
 }
 
+/// Whether a tar entry carries link semantics (symbolic or hard link). Such
+/// entries are rejected inside the selected install subtree — a SKILL.md
+/// bundle has no use for them and they are a notorious escape foothold
+/// (see the module hard rules). This predicate is only the judgment; whether
+/// an entry is considered at all (the scanner skips entries outside the
+/// subtree) is caller policy.
+///
+/// Embedder-reuse contract: the link-rejection judgment for tar-based
+/// importers. Zip entries have no entry type, so zip-based importers keep
+/// the unix-mode file-type-bits face of the same judgment and pin both faces
+/// with tests.
+pub fn entry_type_is_link(entry_type: tar::EntryType) -> bool {
+    entry_type.is_symlink() || entry_type.is_hard_link()
+}
+
+/// Accumulate one entry's size into the running uncompressed total, rejecting
+/// the archive once the total would exceed `max_size` (the gzip-bomb bound).
+///
+/// Saturates rather than overflowing, and `total == max_size` is still
+/// allowed — only strictly-greater totals reject as
+/// [`InstallError::OversizedTarball`].
+///
+/// Embedder-reuse contract: the size-bound half of the archive-entry safety
+/// checks. Callers pass the header-declared size while scanning and the bytes
+/// actually read while extracting, so a forged header cannot skip the real
+/// measurement.
+pub fn add_entry_size(total: u64, added: u64, max_size: u64) -> Result<u64> {
+    let total = total.saturating_add(added);
+    if total > max_size {
+        return Err(InstallError::OversizedTarball { limit: max_size }.into());
+    }
+    Ok(total)
+}
+
 fn skill_target_path(name: &str, skills_dir: &Path) -> Result<PathBuf> {
     let name = validate_skill_name_segment(name)?;
     Ok(skills_dir.join(name))
 }
 
-pub(crate) fn validate_skill_name_segment(name: &str) -> Result<&str> {
+/// Whether a skill name is a single path-safe segment, safe to join under a
+/// skills/plugins directory. Rejects empty and whitespace-containing names,
+/// `.` / `..`, and any separator (`/`, `\`) or multi-component path.
+///
+/// Embedder-reuse contract: consumers that turn a user- or archive-supplied
+/// name into an on-disk directory (skill update/uninstall, plugin
+/// stage/place, SKILL.md frontmatter `name:`) funnel through this validator
+/// so the judgments stay reviewable in one place. An embedder may layer a
+/// stricter allowlist on top; that is caller policy, not drift.
+pub fn validate_skill_name_segment(name: &str) -> Result<&str> {
     if name.is_empty() || name.trim() != name || name.chars().any(char::is_whitespace) {
         bail!("skill name must be a single path-safe segment (got '{name}')");
     }
@@ -1769,6 +1847,50 @@ mod tests {
         assert!(is_safe_path(Path::new("SKILL.md")));
     }
 
+    #[test]
+    fn entry_type_is_link_accepts_only_link_entries() {
+        assert!(entry_type_is_link(tar::EntryType::Symlink));
+        assert!(entry_type_is_link(tar::EntryType::Link));
+        // The judgment is an exact match on the two link flavors; pin every
+        // other named variant so a future tar-crate reclassification cannot
+        // silently widen it. GNULongLink in particular is a metadata header
+        // ("long link name"), not a link entry.
+        for other in [
+            tar::EntryType::Regular,
+            tar::EntryType::Directory,
+            tar::EntryType::Char,
+            tar::EntryType::Block,
+            tar::EntryType::Fifo,
+            tar::EntryType::Continuous,
+            tar::EntryType::GNULongName,
+            tar::EntryType::GNULongLink,
+            tar::EntryType::GNUSparse,
+            tar::EntryType::XHeader,
+            tar::EntryType::XGlobalHeader,
+        ] {
+            assert!(!entry_type_is_link(other), "{other:?} must not be a link");
+        }
+    }
+
+    #[test]
+    fn add_entry_size_rejects_entries_past_the_cap() {
+        assert_eq!(add_entry_size(0, 4, 5).unwrap(), 4);
+        // Exactly at the cap is still allowed (the check is `>`, not `>=`).
+        assert_eq!(add_entry_size(4, 1, 5).unwrap(), 5);
+        // One byte past the cap rejects with the oversized error.
+        let err = add_entry_size(5, 1, 5).unwrap_err();
+        assert!(matches!(
+            err.downcast_ref::<InstallError>(),
+            Some(InstallError::OversizedTarball { limit: 5 })
+        ));
+        // Saturating: a huge entry must trip the cap, not wrap past it.
+        let err = add_entry_size(u64::MAX, 1, 5).unwrap_err();
+        assert!(matches!(
+            err.downcast_ref::<InstallError>(),
+            Some(InstallError::OversizedTarball { limit: 5 })
+        ));
+    }
+
     #[test]
     fn parse_frontmatter_extracts_name() {
         let body = b"---\nname: hello\ndescription: greeter\n---\nbody\n";

crates/tui/src/skills/mod.rs

@@ -17,8 +17,9 @@ mod system;
 #[allow(unused_imports)]
 pub use install::{
     DEFAULT_MAX_SIZE_BYTES, DEFAULT_REGISTRY_URL, INSTALLED_FROM_MARKER, InstallOutcome,
-    InstallSource, InstalledSkill, RegistryDocument, RegistryEntry, RegistryFetchResult,
-    SkillSyncOutcome, SyncResult, UpdateResult, default_cache_skills_dir,
+    InstallSource, InstalledFromMarker, InstalledSkill, RegistryDocument, RegistryEntry,
+    RegistryFetchResult, SkillSyncOutcome, SyncResult, UpdateResult, add_entry_size,
+    default_cache_skills_dir, entry_type_is_link, is_safe_path, validate_skill_name_segment,
 };
 #[allow(unused_imports)]
 pub use roots::{