robustness: harden parsers, mask modes, scrub credentials (Phase 8.3)

Phase 8.3 of the post-firebaguette audit closes 13 MEDIUM findings plus the
deferred push-side half of H8.

Why: the audit's M-tier surface (credentials in logs, recursive walkers,
unbounded parsers, hooks-via-shared-cache, silent state destruction)
doesn't include single-shot exploits but is a steady leak of trust if
left in. Closing it now keeps the threat model coherent before MEDIUM
items grow into HIGH ones at scale.

Headline primitives this ship adds:

* git_cmd() — every git invocation prepends `-c core.hooksPath=/dev/null`
  so a malicious library cannot weaponise the operator's global hooks
  config (M12).
* scrub_stderr() — first line, no control bytes, redact ghp_/gho_/ghs_/
  ghu_/github_pat_/x-access-token: tokens (M3).
* fetch_and_fast_forward now refuses to reset --hard when porcelain is
  non-empty — prevents silent destruction of state from a previously
  crashed run (M10).
* config::sanitize_url_for_display — userinfo stripped from stored URL
  so PATs never land in config.toml / JSON / error chains (M1).
* skill::discover takes include_vendored: bool — defaults to false
  (respect .gitignore + skip node_modules/target). New --include-vendored
  flag on detect (M11).
* parse_frontmatter capped at 200 lines; unterminated frontmatter
  yields no metadata instead of scanning the whole body (M4).
* sanitize::validate_fork_name consolidated and hardened (control char
  reject + 64-byte cap) (M5).
* ProjectConfig + InstalledSkill get deny_unknown_fields; dedup by name
  and destination at load; entry count capped at 256 (M6).
* copy_dir_all rewritten as an explicit work-stack loop (M7); on Unix,
  copied file modes masked to 0o644|(src_mode & 0o100) so setuid/setgid/
  sticky/group-write/world-write are stripped (M8).
* detect's "already installed" dedup unions canonical-path AND
  lexical-path comparison so a deleted-then-replaced destination
  can't be silently re-detected as new (M9, via new
  path_safety::normalize_lexical).
* push apply loop is now continue-on-error per skill, with
  git::checkout_paths to roll back the cache working tree for a failed
  skill before continuing — closes the deferred push-side of H8.
* README + SECURITY.md document the canonical Homebrew tap install and
  the typo-squat risk (M13).

13 new unit tests; cargo test: 136 pass; clippy clean; cargo audit clean.

Author: Fernando Pinho

CHANGELOG.md

@@ -6,6 +6,27 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 
 ## [Unreleased]
 
+## [0.1.5] - 2026-05-22
+
+### Security & robustness
+
+Close the comprehensive audit's Phase 8.3: 13 MEDIUM findings plus the deferred push-side half of H8. No item here is single-shot exploitable, but each closes a credibility-eroding leak (credentials in logs), DoS vector (unbounded parsers, recursive walkers), or footgun (silently-discarded state, hook execution via shared cache).
+
+- **Credentials stripped from stored `library.url`** (M1). `skillctl init https://x-access-token:<PAT>@github.com/...` would store the full URL — token and all — in `config.toml`, then echo it back in JSON output, error chains, and CI logs. `init` now sanitises the URL (strips `user[:password]@` from `https://`/`http://` authority sections) before persisting; the one-time `git clone` still uses the original URL for authentication, but the token never lands on disk or in any later command's output. SSH forms (`git@host:...`, `ssh://git@host/...`) are unchanged.
+- **Git stderr scrubbed in every error chain** (M3). Each `git`-shell-out site used `String::from_utf8_lossy(&stderr).trim()` — which would faithfully echo credential-helper banners, proxy URLs containing PATs, ANSI control sequences, and stack traces past the first line. The new `git::scrub_stderr` helper takes the first non-empty line, strips C0/C1/DEL/ESC control bytes, and redacts known token prefixes (`ghp_*`, `gho_*`, `ghs_*`, `ghu_*`, `github_pat_*`, `x-access-token:*`) to `<prefix>***`. Applied uniformly across every git invocation.
+- **`core.hooksPath` neutralised on every git call** (M12). The library cache is a git repo whose `.git/config` is reachable from inside skill content. A malicious library that dropped a script at the operator's globally-configured `core.hooksPath` would have it executed by any `git commit` in the cache. Every `Command::new("git")` now goes through a `git_cmd()` helper that prepends `-c core.hooksPath=/dev/null`, so hook execution is impossible regardless of global or in-cache git config.
+- **`git status --porcelain` check before `reset --hard @{upstream}`** (M10). `fetch_and_fast_forward` used to unconditionally `git reset --hard @{upstream}`, silently destroying any uncommitted state left over from a previous skillctl run that crashed mid-commit (e.g. `replace_folder_contents` succeeded but `git push` failed). Now refuses to refresh when the cache reports any porcelain output, surfacing a clear "uncommitted changes — inspect with `git -C <cache> status`" message so the operator can investigate before any destruction happens.
+- **Frontmatter parser bounded at 200 lines** (M4). A SKILL.md with an opening `---` but no closing fence would force the parser to scan the entire (potentially multi-GiB) body — a cheap DoS reachable on every `skill::discover` call. Capped to `MAX_FRONTMATTER_LINES = 200`; unterminated frontmatter is now treated as "no frontmatter" (the skill is dropped from discovery rather than half-parsed).
+- **`validate_fork_name` rejects control characters and caps length** (M5). The previous fork-name validator only rejected empty / `.` / `..` / path separators — a name like `foo\0bar` would panic inside `CString::new` when later passed to `Command`. Now rejects any control char (NUL, ESC, ANSI, DEL, newline, CR, tab) and caps at 64 bytes. Consolidated as `sanitize::validate_fork_name` (was duplicated between `push.rs` and `pull.rs`).
+- **`.skills.toml` rejects unknown fields, duplicates, and overflow** (M6). Added `#[serde(deny_unknown_fields)]` on `ProjectConfig` and `InstalledSkill`, so a malicious PR can no longer smuggle unknown keys (which might later be load-bearing for an unreleased feature) into the deserialiser. Duplicate `name` or `destination` entries are rejected at load — silent dedup would make every command ambiguous about which entry wins. Capped at 256 entries to bound the diff-classifier work.
+- **`copy_dir_all` is iterative and masks mode bits** (M7 + M8). Converted from recursion to an explicit `Vec<(PathBuf, PathBuf)>` work stack, so an adversarial skill with 10k-deep nesting can no longer blow Rust's default 8 MiB thread stack. On Unix, copied file modes are now masked to `0o644 | (src_mode & 0o100)` — only the user-execute bit propagates; setuid, setgid, sticky, group-write, world-write, group-execute and world-execute are stripped. A library that drop-ins a setuid binary cannot weaponise the round-trip into elevated privileges on the destination.
+- **`detect` dedup unions canonical AND lexical comparison** (M9). The "already installed" set was built from `fs::canonicalize` only — silently dropping entries whose destination had been deleted from disk. An attacker who removed `.claude/skills/foo/` and dropped a replacement at the same path would have it re-detected as a new skill on the next `detect`. Now compares by canonical path (when both ends exist) AND lexical path (covers the deleted-destination case via the new `path_safety::normalize_lexical` helper).
+- **`detect` walker respects `.gitignore` and skips vendor dirs by default** (M11). A malicious npm package shipping its own `SKILL.md` under `node_modules/...` could be picked up by `skillctl detect --all` running in CI and uploaded to the library. `skill::discover` now takes an `include_vendored` parameter; the default (false) leans on `ignore::WalkBuilder`'s `.gitignore`/`.ignore` respect plus a hard-skip on `node_modules`/`target`. New CLI flag `skillctl detect --include-vendored` for the explicit opt-in.
+- **Homebrew tap typo-squat documented** (M13). Both README and SECURITY.md now prominently call out the canonical fully-qualified install (`brew install umanio-agency/homebrew-tap/skillctl`) and explain that anyone can ship a `skillctl.rb` formula under their own `homebrew-tap` repo. Pinning the owner avoids the typo-squat risk.
+- **`push --all` continues on per-skill failure** (H8 push-side). The pre-v0.1.5 apply loop used `?` inside the per-skill body, so one failing skill aborted the entire batch and orphaned the cache's working tree for the successful early skills (commit + push never happened, cache stayed dirty until the next `fetch_and_fast_forward` reset it). Now each apply is wrapped in an IIFE: on per-skill failure, the change is rolled back with `git checkout HEAD -- <library_relative>`, a warning is logged, and the loop continues. If all skills fail, the command exits cleanly with "nothing pushed". This closes the half of H8 deferred from v0.1.4.
+
+13 new unit tests added (3 path_safety lexical normalisation, 3 sanitize fork-name hardening, 4 `.skills.toml` deny/dedup/cap, 3 discover gitignore/node_modules/include-vendored, 2 frontmatter bound, 7 git stderr scrub, 3 fs_util mode-mask + deep nesting). `cargo test`: 136 pass; clippy clean; `cargo audit` clean.
+
 ## [0.1.4] - 2026-05-22
 
 ### Security & robustness

Cargo.lock

@@ -1,6 +1,6 @@
 [package]
 name = "skillctl"
-version = "0.1.4"
+version = "0.1.5"
 edition = "2024"
 rust-version = "1.85"
 description = "CLI to manage your personal agent skills library across projects"

Cargo.toml

@@ -43,6 +43,8 @@ Plus `init` (link a library). Every multi-skill flow supports `--tag` filtering
 brew install umanio-agency/homebrew-tap/skillctl
 ```
 
+Always use the **fully-qualified** form above — `<tap-owner>/<tap-repo>/skillctl`. The unqualified `brew install skillctl` would resolve to whichever tap is currently active in your Homebrew installation, and anyone can create a `homebrew-tap` repo under their own GitHub user and ship a `skillctl.rb` formula. Pinning the tap owner (`umanio-agency`) avoids that typo-squat risk.
+
 ### Cargo (any platform with Rust 1.85+)
 
 ```sh

README.md

@@ -19,3 +19,9 @@ Please do not open a public issue for security reports. We aim to acknowledge wi
 ## Supported versions
 
 The project is pre-v1; only the `main` branch is supported and security fixes land there. Once v1 ships, this section will document the supported release range.
+
+## Install channel hygiene
+
+- **Homebrew:** always use the fully-qualified tap name — `brew install umanio-agency/homebrew-tap/skillctl`. Homebrew taps are namespaced by their GitHub owner, and anyone can create a `homebrew-tap` repo and publish a `skillctl.rb` formula. Pinning the owner (`umanio-agency`) prevents typo-squat attacks where a malicious tap is added before the official one.
+- **crates.io:** the published crate is `skillctl` (owner: `pinho.dcj@gmail.com`). The historical name `skills-cli` is owned by an unrelated third party and is **not** affiliated with this project.
+- **Direct binaries:** the `curl | sh` and PowerShell installers serve assets from `github.com/umanio-agency/skillctl/releases/latest/download/…` and verify SHA-256 sums published alongside each release.

SECURITY.md

@@ -190,6 +190,13 @@ pub struct DetectArgs {
     /// `skills`, or `.claude/skills`). Required in non-interactive mode.
     #[arg(long, value_name = "PATH")]
     pub target: Option<PathBuf>,
+
+    /// Also walk paths normally ignored by `.gitignore` (e.g.
+    /// `node_modules/`, `vendor/`, `Pods/`). By default `detect` respects
+    /// the project's `.gitignore` so a `SKILL.md` smuggled inside a
+    /// third-party dependency cannot be silently shipped to the library.
+    #[arg(long)]
+    pub include_vendored: bool,
 }
 
 #[derive(Clone, Copy, Debug, ValueEnum)]

src/cli.rs

@@ -74,7 +74,7 @@ pub fn run(args: AddArgs, ctx: &Context) -> Result<()> {
         )?;
     }
 
-    let skills = skill::discover(&library_root)?;
+    let skills = skill::discover(&library_root, false)?;
     if skills.is_empty() {
         ui::outro(ctx, format!("no skills found in {}", library.url))?;
         emit_json(ctx, None, &[]);

src/commands/add.rs

@@ -17,7 +17,7 @@ use crate::error::AppError;
 use crate::fs_util;
 use crate::git;
 use crate::lock;
-use crate::path_safety::validate_relative_subpath;
+use crate::path_safety::{normalize_lexical, validate_relative_subpath};
 use crate::project_config::{self, InstalledSkill};
 use crate::skill::{self, Skill};
 use crate::ui;
@@ -59,18 +59,33 @@ pub fn run(args: DetectArgs, ctx: &Context) -> Result<()> {
     let _project_lock = lock::acquire_exclusive(&cwd, "project")?;
     let mut project_cfg = project_config::load(&cwd)?;
 
+    // Dedup keys for "already tracked in .skills.toml". Use BOTH the
+    // canonical and the lexical path: canonical handles symlinks and
+    // NFD/NFC variants on macOS when the destination exists; lexical
+    // covers the case where a tracked destination was deleted from disk
+    // (otherwise an attacker who removed `.claude/skills/foo/` and dropped
+    // a malicious replacement at the same path would have it re-detected
+    // as a new skill on the next `detect`).
     let installed_canonical: HashSet<PathBuf> = project_cfg
         .installed
         .iter()
         .filter_map(|i| std::fs::canonicalize(cwd.join(&i.destination)).ok())
         .collect();
+    let installed_lexical: HashSet<PathBuf> = project_cfg
+        .installed
+        .iter()
+        .map(|i| normalize_lexical(&cwd.join(&i.destination)))
+        .collect();
 
-    let local_skills = skill::discover(&cwd)?;
+    let local_skills = skill::discover(&cwd, args.include_vendored)?;
     let new_skills: Vec<Skill> = local_skills
         .into_iter()
-        .filter(|s| match std::fs::canonicalize(&s.path) {
-            Ok(c) => !installed_canonical.contains(&c),
-            Err(_) => true,
+        .filter(|s| {
+            let canonical_match = std::fs::canonicalize(&s.path)
+                .ok()
+                .is_some_and(|c| installed_canonical.contains(&c));
+            let lexical_match = installed_lexical.contains(&normalize_lexical(&s.path));
+            !canonical_match && !lexical_match
         })
         .collect();
 

src/commands/detect.rs

@@ -14,8 +14,14 @@ pub fn run(args: InitArgs, ctx: &Context) -> Result<()> {
 
     git::ensure_available().map_err(|e| AppError::Git(e.to_string()))?;
 
-    let url = args.url;
-    let dest = config::library_cache_path(&url).map_err(|e| AppError::Config(e.to_string()))?;
+    // `args.url` may carry an embedded `user:token@` for the one-time clone;
+    // we use it as-is to call `git clone`, but never persist or echo it.
+    // `display_url` is the sanitised form that lands in `config.toml`,
+    // logs, JSON output, and any later error chain referencing `library.url`.
+    let clone_url = args.url;
+    let display_url = config::sanitize_url_for_display(&clone_url);
+    let dest =
+        config::library_cache_path(&display_url).map_err(|e| AppError::Config(e.to_string()))?;
 
     if dest.exists() {
         fs::remove_dir_all(&dest)
@@ -26,21 +32,26 @@ pub fn run(args: InitArgs, ctx: &Context) -> Result<()> {
             .with_context(|| format!("creating cache dir at {}", parent.display()))?;
     }
 
-    ui::log_info(ctx, format!("cloning {url} into {} ...", dest.display()))?;
-    git::clone(&url, &dest).map_err(|e| AppError::Git(e.to_string()))?;
+    ui::log_info(
+        ctx,
+        format!("cloning {display_url} into {} ...", dest.display()),
+    )?;
+    git::clone(&clone_url, &dest).map_err(|e| AppError::Git(e.to_string()))?;
 
     let config = Config {
-        library: Some(Library { url: url.clone() }),
+        library: Some(Library {
+            url: display_url.clone(),
+        }),
     };
     config::save(&config).map_err(|e| AppError::Config(e.to_string()))?;
 
-    ui::log_success(ctx, format!("library configured: {url}"))?;
+    ui::log_success(ctx, format!("library configured: {display_url}"))?;
 
     if ctx.json {
         let out = serde_json::json!({
             "command": "init",
             "library": {
-                "url": url,
+                "url": display_url,
                 "cache_path": dest.display().to_string(),
             }
         });

src/commands/init.rs

@@ -37,7 +37,7 @@ pub fn run(args: ListArgs, ctx: &Context) -> Result<()> {
         eprintln!("warning: could not refresh library cache ({e}); using cached version");
     }
 
-    let skills = skill::discover(&repo)?;
+    let skills = skill::discover(&repo, false)?;
     let filtered: Vec<_> = skills
         .into_iter()
         .filter(|s| matches_tags(&s.tags, &args.tags, args.all_tags))