From 186fd2d44dd357826bda04f1ac50ad53c1ccfff8 Mon Sep 17 00:00:00 2001 From: asto Date: Mon, 5 Oct 2026 01:49:28 +0800 Subject: [PATCH 1/3] refactor(skills): export install safety primitives Widen the skill-install safety surface to pub so embedders can reuse the real checks instead of keeping comment-aligned copies of them: - is_safe_path (path traversal / absolute-path / prefix-root rejection) and validate_skill_name_segment (single path-safe segment) were pub(crate); they are now pub with docs marking the embedder-reuse contract. The in-tree plugin installer already consumes both. - INSTALLED_FROM_MARKER stays pub; its doc now states the contract. - InstalledFromMarker (the .installed-from JSON schema) becomes a public struct with public fields so readers of the marker file deserialize through the shared schema instead of redefining it. - The symlink/hard-link entry rejection and the running size cap were inline in scan_tarball/extract_into; extracted unchanged into pub entry_type_is_link and pub add_entry_size, which both passes now share (scan measures header-declared sizes, extract the bytes actually read, so a forged header cannot skip the real bound). Pinvou's plugin/skill importer keeps a re-implementation whose header says it is aligned with this module by comment; it will switch to this API. Unit tests added for the link-entry and size-cap predicates; traversal and name-segment vectors already existed. No behavior change. Signed-off-by: asto --- crates/tui/src/skills/install.rs | 132 +++++++++++++++++++++++++------ 1 file changed, 110 insertions(+), 22 deletions(-) diff --git a/crates/tui/src/skills/install.rs b/crates/tui/src/skills/install.rs index 0869a33905..ee66081ba7 100644 --- a/crates/tui/src/skills/install.rs +++ b/crates/tui/src/skills/install.rs @@ -17,6 +17,13 @@ //! `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 already does; see +//! `crate::plugins::install`). //! //! # Hard rules //! @@ -86,6 +93,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 @@ -878,24 +889,34 @@ async fn sync_one_skill( // Internal helpers // ───────────────────────────────────────────────────────────────────────────── +/// On-disk schema of the [`INSTALLED_FROM_MARKER`] JSON file. +/// +/// v1 wrote `spec` + `checksum`; schema v2 (see [`write_installed_from_v2`]) +/// adds `schema_version`, `source_checksum`, `content_digest`, and +/// `installed_name`. 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, + pub source_checksum: Option, #[serde(default)] - #[allow(dead_code)] - schema_version: Option, + pub schema_version: Option, #[serde(default)] - #[allow(dead_code)] - content_digest: Option, + pub content_digest: Option, } impl InstalledFromMarker { - pub(crate) fn source_checksum(&self) -> &str { + /// Preferred checksum: the v2 `source_checksum` when present, else the v1 + /// `checksum` field. + pub fn source_checksum(&self) -> &str { self.source_checksum .as_deref() .filter(|s| !s.is_empty()) @@ -1270,10 +1291,7 @@ fn scan_tarball(bytes: &[u8], max_size: u64) -> Result { // 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 +1315,7 @@ fn scan_tarball(bytes: &[u8], max_size: u64) -> Result { } } - 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 +1482,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 +1512,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 +1543,15 @@ 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 single source of truth for +/// "does this entry escape its destination". Importers of any archive format +/// (tar, zip) call this on each entry path instead of keeping comment-aligned +/// copies of the check. +pub fn is_safe_path(path: &Path) -> bool { if path.is_absolute() { return false; } @@ -1543,12 +1565,51 @@ pub(crate) fn is_safe_path(path: &Path) -> bool { true } +/// Whether a tar entry carries link semantics (symbolic or hard link). Such +/// entries inside the selected install subtree are rejected — a SKILL.md +/// bundle has no use for them and they are a notorious escape foothold +/// (see the module hard rules). Entries outside the subtree are ignored. +/// +/// Embedder-reuse contract: the symlink-rejection judgment for tar-based +/// importers; zip-based importers apply the same judgment through the entry's +/// unix-mode file-type bits. +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 { + 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 { 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: every consumer that turns a user- or +/// archive-supplied name into an on-disk directory (skill update/uninstall, +/// plugin stage/place, SKILL.md frontmatter `name:`) funnels through this +/// validator so the judgment cannot drift between copies. +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 +1830,33 @@ 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)); + assert!(!entry_type_is_link(tar::EntryType::Regular)); + assert!(!entry_type_is_link(tar::EntryType::Directory)); + } + + #[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::(), + 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::(), + Some(InstallError::OversizedTarball { limit: 5 }) + )); + } + #[test] fn parse_frontmatter_extracts_name() { let body = b"---\nname: hello\ndescription: greeter\n---\nbody\n"; From 49c8b733ba4dd0aa71735eb2cb12220c206daaa8 Mon Sep 17 00:00:00 2001 From: asto Date: Tue, 6 Oct 2026 17:18:25 +0800 Subject: [PATCH 2/3] refactor(plugins): reuse the shared link predicate The exported entry_type_is_link judgment replaces the plugin installer's last inline is_symlink/is_hard_link disjunction, so the in-tree reuse claim in the skills::install module contract is true for it too. The plugin size gate keeps its own accumulation: it must raise PluginInstallError::OversizedBundle, not InstallError::OversizedTarball. Behavior identical (same expression, pure predicate); plugins::install suite stays green (24 passed). Signed-off-by: asto --- crates/tui/src/plugins/install/mod.rs | 3 ++- crates/tui/src/plugins/install/tarball.rs | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/tui/src/plugins/install/mod.rs b/crates/tui/src/plugins/install/mod.rs index 88cabfb1b1..faabd03e8d 100644 --- a/crates/tui/src/plugins/install/mod.rs +++ b/crates/tui/src/plugins/install/mod.rs @@ -5,7 +5,8 @@ //! root (`~/.codewhale/plugins//`). 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`). //! diff --git a/crates/tui/src/plugins/install/tarball.rs b/crates/tui/src/plugins/install/tarball.rs index 37203c8edf..3dfd0a77de 100644 --- a/crates/tui/src/plugins/install/tarball.rs +++ b/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()); } From 97fee5d1a930555c0fb41e839ccc4b859cecbadb Mon Sep 17 00:00:00 2001 From: asto Date: Tue, 6 Oct 2026 17:18:35 +0800 Subject: [PATCH 3/3] docs(skills): calibrate the embedder-reuse contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #81 found the contract wording overclaiming on several axes; this aligns each claim with what the code and the ecosystem actually do: - Module doc: the in-tree plugin installer consumes the judgments verbatim; importers with deliberately different verdict semantics (zip's normalize-and-accept traversal) register divergence with tests rather than restate an "aligned" copy. Drop the private-module path pointer external readers cannot resolve. - InstalledFromMarker: the marker history is v1 spec/url/checksum, v2 adding schema_version/source_checksum/content_digest/installed_name/ registry_version; url/installed_name/registry_version are intentionally not modeled by the struct. - source_checksum(): the v1 fallback leg is returned verbatim and can be empty; empty reads as "changed" (fail-safe) — now stated. - is_safe_path / validate_skill_name_segment: replace "single source of truth" / "cannot drift" (no mechanism enforces embedder adoption) with reviewable-home wording; stricter embedder allowlists are policy, not drift. - entry_type_is_link: separate the predicate from caller policy (subtree filtering), and state that zip keeps the unix-mode face pinned by tests. - InstallError: it is now part of the embedder contract (downcast pattern); drop the internal-only audience statement. - skills/mod.rs: list the new contract names in the downstream-consumer re-export set so the short skills::* path reaches them. - entry_type_is_link test: pin every non-link EntryType variant (GNULongLink in particular) so a tar-crate reclassification cannot silently widen the judgment. skills::install 22 passed, plugins::install 24 passed, fmt/clippy clean. Signed-off-by: asto --- crates/tui/src/skills/install.rs | 86 ++++++++++++++++++++++---------- crates/tui/src/skills/mod.rs | 5 +- 2 files changed, 63 insertions(+), 28 deletions(-) diff --git a/crates/tui/src/skills/install.rs b/crates/tui/src/skills/install.rs index ee66081ba7..5572654338 100644 --- a/crates/tui/src/skills/install.rs +++ b/crates/tui/src/skills/install.rs @@ -21,9 +21,12 @@ //! [`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 already does; see -//! `crate::plugins::install`). +//! 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 //! @@ -237,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}")] @@ -891,10 +896,13 @@ async fn sync_one_skill( /// On-disk schema of the [`INSTALLED_FROM_MARKER`] JSON file. /// -/// v1 wrote `spec` + `checksum`; schema v2 (see [`write_installed_from_v2`]) -/// adds `schema_version`, `source_checksum`, `content_digest`, and -/// `installed_name`. Unknown fields are ignored on read, so newer writers stay -/// forward-compatible with older readers. +/// 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. @@ -914,8 +922,11 @@ pub struct InstalledFromMarker { } impl InstalledFromMarker { - /// Preferred checksum: the v2 `source_checksum` when present, else the v1 - /// `checksum` field. + /// 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() @@ -1547,10 +1558,12 @@ fn is_within_selected_root(path: &str, prefix: &str, skill_root: &str) -> bool { /// directory: rejects absolute paths, Windows prefixes / root components, and /// any `..` segment (path traversal). /// -/// Embedder-reuse contract: this predicate is the single source of truth for -/// "does this entry escape its destination". Importers of any archive format -/// (tar, zip) call this on each entry path instead of keeping comment-aligned -/// copies of the check. +/// 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; @@ -1566,13 +1579,16 @@ pub fn is_safe_path(path: &Path) -> bool { } /// Whether a tar entry carries link semantics (symbolic or hard link). Such -/// entries inside the selected install subtree are rejected — a SKILL.md +/// 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). Entries outside the subtree are ignored. +/// (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 symlink-rejection judgment for tar-based -/// importers; zip-based importers apply the same judgment through the entry's -/// unix-mode file-type bits. +/// 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() } @@ -1605,10 +1621,11 @@ fn skill_target_path(name: &str, skills_dir: &Path) -> Result { /// skills/plugins directory. Rejects empty and whitespace-containing names, /// `.` / `..`, and any separator (`/`, `\`) or multi-component path. /// -/// Embedder-reuse contract: every consumer that turns a user- or -/// archive-supplied name into an on-disk directory (skill update/uninstall, -/// plugin stage/place, SKILL.md frontmatter `name:`) funnels through this -/// validator so the judgment cannot drift between copies. +/// 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}')"); @@ -1834,8 +1851,25 @@ mod tests { 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)); - assert!(!entry_type_is_link(tar::EntryType::Regular)); - assert!(!entry_type_is_link(tar::EntryType::Directory)); + // 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] diff --git a/crates/tui/src/skills/mod.rs b/crates/tui/src/skills/mod.rs index 3b9a6f00c1..a3dfe88b8d 100644 --- a/crates/tui/src/skills/mod.rs +++ b/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::{