fix(lib): vendor-skills validates lock fields, fullmatch guard

Two security-gate LOW notes closed on user ask: the SAFE guard uses
re.fullmatch so a trailing newline is rejected; commit (40 hex), source
(github.com owner/repo) and path (SAFE class, no traversal) are validated
before any URL is built, INVALID marker names the field. Suite 12 cases.

CHANGELOG.md

@@ -21,7 +21,9 @@ Format follows [Keep a Changelog](https://keepachangelog.com/).
   plugins.lock.json, text files only, never demos or binaries). The
   agent-skills curl loop became the shared `lib/vendor-skills.sh`
   (`vendor_pinned_skills <lock-key> [refresh]`, list or dict lock shapes,
-  `VENDOR_BASE_URL` for the hermetic suite `lib/tests/vendor-skills.test.sh`),
+  `VENDOR_BASE_URL` honoured only as `file://` for the hermetic suite
+  `lib/tests/vendor-skills.test.sh`, lock values validated: 40-hex commit,
+  github.com source, traversal-free paths, no trailing newline),
   used by install-plugins.sh Step 8e and update-all.sh 7.3; refresh keeps
   the file's convention and skips a skill that was never installed.
   Registered in link.sh, .gitignore,

lib/tests/vendor-skills.test.sh

@@ -5,7 +5,10 @@
 # throwaway repo), tmp+mv semantics (a missing upstream file leaves no dest
 # and no tmp), skip-when-present, refresh overwriting a stale copy, a
 # non-file:// VENDOR_BASE_URL override being ignored (warn, default URL),
-# and a "../evil" lock file being rejected before any fetch.
+# a "../evil" lock file being rejected before any fetch, a lock value
+# ending in a newline being rejected (the re.fullmatch fix), and a bad
+# commit/source/path on the lock entry itself being rejected before the
+# raw URL is ever built.
 set -u
 ROOT="$(cd "$(dirname "$0")/../.." && pwd)"
 LIB="$ROOT/lib/vendor-skills.sh"
@@ -29,47 +32,78 @@ mkdir -p "$FIXTURE_REPO/skills-external"
 # names a skill never installed locally (no skills-external/ dir), to
 # prove refresh skips it instead of installing it. "traversal-key" names
 # a well-behaved SKILL.md alongside a "../evil" file, to prove the whole
-# key is rejected before either one is fetched.
+# key is rejected before either one is fetched. "newline-key" names a
+# file whose value ends in a newline, to prove the SAFE-class check uses
+# re.fullmatch (a plain re.match "$" would let it through). "bad-commit-
+# key", "bad-source-key" and "bad-path-key" carry an otherwise-valid entry
+# with exactly one malformed field, to prove each is checked before the
+# raw URL is built. Every commit below is a real 40-hex sha1 (of the key's
+# own name) — only the three "bad-*-key" entries break that on purpose.
 cat > "$FIXTURE_REPO/plugins.lock.json" <<'JSON'
 {
   "list-key": {
     "source": "https://github.com/acme/list-repo",
-    "commit": "abc123",
+    "commit": "0b29f58330afad53522f9045ef48ba153bb5ab81",
     "skills": ["skill-list-a"]
   },
   "dict-key": {
     "source": "https://github.com/acme/dict-repo",
-    "commit": "def456",
+    "commit": "68ca98404892988c1cbe928dc12ef3ed144c8d70",
     "path": "somewhere/nested",
     "skills": {"skill-dict-a": ["SKILL.md", "references/notes.md"]}
   },
   "fail-key": {
     "source": "https://github.com/acme/fail-repo",
-    "commit": "789fail",
+    "commit": "898733695eac132c05aac536e7f86cdd89bdfe09",
     "skills": ["skill-fail-a"]
   },
   "missing-key": {
     "source": "https://github.com/acme/missing-repo",
-    "commit": "111missing",
+    "commit": "2e7a1c5865d4f2c9dc2e3354658f0e257f6110d8",
     "skills": ["skill-missing-a"]
   },
   "traversal-key": {
     "source": "https://github.com/acme/traversal-repo",
-    "commit": "222trav",
+    "commit": "9704a3bf7b366acd318b0d12dacc78774ff2ade1",
     "skills": {"skill-trav-a": ["SKILL.md", "../evil"]}
+  },
+  "newline-key": {
+    "source": "https://github.com/acme/newline-repo",
+    "commit": "12f27ef33f4cd777b9471b989a8e022e35c0ab74",
+    "skills": {"skill-nl-a": ["SKILL.md\n"]}
+  },
+  "bad-commit-key": {
+    "source": "https://github.com/acme/badcommit-repo",
+    "commit": "main",
+    "skills": ["skill-badcommit-a"]
+  },
+  "bad-source-key": {
+    "source": "https://evil.example.com/x/y",
+    "commit": "eeb5c78b15a6b1ffa3fb5d46d8c794bcd0446bdc",
+    "skills": ["skill-badsource-a"]
+  },
+  "bad-path-key": {
+    "source": "https://github.com/acme/badpath-repo",
+    "commit": "e341601ba6f6255378268e9aaec304de93a13d50",
+    "path": "../x",
+    "skills": {"skill-badpath-a": ["SKILL.md"]}
   }
 }
 JSON
 
-mkdir -p "$UPSTREAM/abc123/skills/skill-list-a"
-echo v1 > "$UPSTREAM/abc123/skills/skill-list-a/SKILL.md"
-mkdir -p "$UPSTREAM/def456/somewhere/nested/skill-dict-a/references"
-echo "dict skill" > "$UPSTREAM/def456/somewhere/nested/skill-dict-a/SKILL.md"
-echo "dict notes" > "$UPSTREAM/def456/somewhere/nested/skill-dict-a/references/notes.md"
-# fail-key: 789fail/skills/skill-fail-a/SKILL.md deliberately absent.
+LIST_SHA="0b29f58330afad53522f9045ef48ba153bb5ab81"
+DICT_SHA="68ca98404892988c1cbe928dc12ef3ed144c8d70"
+mkdir -p "$UPSTREAM/$LIST_SHA/skills/skill-list-a"
+echo v1 > "$UPSTREAM/$LIST_SHA/skills/skill-list-a/SKILL.md"
+mkdir -p "$UPSTREAM/$DICT_SHA/somewhere/nested/skill-dict-a/references"
+echo "dict skill" > "$UPSTREAM/$DICT_SHA/somewhere/nested/skill-dict-a/SKILL.md"
+echo "dict notes" \
+  > "$UPSTREAM/$DICT_SHA/somewhere/nested/skill-dict-a/references/notes.md"
+# fail-key: .../skills/skill-fail-a/SKILL.md deliberately absent.
 # missing-key: no $UPSTREAM tree at all — refresh must skip it on the
 # missing skills-external/ dir alone, before ever reaching curl.
-# traversal-key: no $UPSTREAM tree either — the "../evil" file must be
+# traversal-key, newline-key, bad-commit-key, bad-source-key and
+# bad-path-key: no $UPSTREAM tree either — every one of them must be
 # rejected by the lock reader itself, before any URL is built.
 
 export VENDOR_SKILLS_REPO_OVERRIDE="$FIXTURE_REPO"
@@ -97,7 +131,7 @@ check_bool FAIL_LEAVES_NOTHING "$([ ! -e "$fdest" ] && [ ! -e "$fdest.tmp" ] \
   && printf '%s' "$out" | grep -q 'not all files landed' && echo 1 || echo 0)"
 
 # ── SKIP_PRESENT — upstream changes, a plain re-run keeps the old copy ───
-echo v2-upstream-changed > "$UPSTREAM/abc123/skills/skill-list-a/SKILL.md"
+echo v2-upstream-changed > "$UPSTREAM/$LIST_SHA/skills/skill-list-a/SKILL.md"
 vendor_pinned_skills list-key >/dev/null 2>&1
 check_bool SKIP_PRESENT "$([ "$(cat "$dest")" = v1 ] && echo 1 || echo 0)"
 
@@ -108,7 +142,8 @@ check_bool REFRESH_OVERWRITES \
 
 # ── REFRESH_SKIPS_MISSING — refresh never installs a skill that has no
 # skills-external/<name> dir yet; it prints the standard "not installed"
-# skip line and never touches curl (no $UPSTREAM/111missing/ exists).
+# skip line and never touches curl (missing-key's sha has no $UPSTREAM
+# tree at all).
 mdest="$FIXTURE_REPO/skills-external/skill-missing-a"
 out="$(vendor_pinned_skills missing-key refresh 2>&1)"
 check_bool REFRESH_SKIPS_MISSING "$([ ! -e "$mdest" ] \
@@ -121,8 +156,8 @@ check_bool REFRESH_SKIPS_MISSING "$([ ! -e "$mdest" ] \
 # prefix is used instead. list-key's SKILL.md is already vendored (v2,
 # from REFRESH_OVERWRITES above), so this probe never touches curl
 # either way — the assertion is the warn line, and that the file:// mode
-# used everywhere else in this suite (asserted by the six cases above
-# and below) keeps working.
+# used everywhere else in this suite (asserted by the eleven other cases)
+# keeps working.
 out="$(VENDOR_BASE_URL="https://evil.example.com" \
   vendor_pinned_skills list-key 2>&1)"
 check_bool OVERRIDE_NON_FILE_IGNORED \
@@ -140,5 +175,43 @@ outside="$FIXTURE_REPO/skills-external/evil"
 check_bool REJECTS_TRAVERSAL "$([ "$rc" -ne 0 ] && [ ! -e "$tdir" ] \
   && [ ! -e "$outside" ] && echo 1 || echo 0)"
 
+# ── REJECTS_TRAILING_NEWLINE — a lock file value ending in a newline
+# ("SKILL.md\n") is rejected by the SAFE-class check (re.fullmatch, not
+# re.match): nothing is fetched for the key, and the err line names the
+# lock key.
+out="$(vendor_pinned_skills newline-key 2>&1)"
+rc=$?
+nldir="$FIXTURE_REPO/skills-external/skill-nl-a"
+check_bool REJECTS_TRAILING_NEWLINE "$([ "$rc" -ne 0 ] && [ ! -e "$nldir" ] \
+  && printf '%s' "$out" | grep -q 'newline-key' && echo 1 || echo 0)"
+
+# ── REJECTS_BAD_COMMIT — a lock entry whose "commit" is not 40 lowercase
+# hex chars ("main") is rejected before any URL is built.
+out="$(vendor_pinned_skills bad-commit-key 2>&1)"
+rc=$?
+bcdir="$FIXTURE_REPO/skills-external/skill-badcommit-a"
+check_bool REJECTS_BAD_COMMIT "$([ "$rc" -ne 0 ] && [ ! -e "$bcdir" ] \
+  && printf '%s' "$out" | grep -qF "rejected commit='main'" \
+  && echo 1 || echo 0)"
+
+# ── REJECTS_BAD_SOURCE — a lock entry whose "source" is not a
+# "https://github.com/<owner>/<repo>" URL is rejected before any URL is
+# built.
+out="$(vendor_pinned_skills bad-source-key 2>&1)"
+rc=$?
+bsdir="$FIXTURE_REPO/skills-external/skill-badsource-a"
+check_bool REJECTS_BAD_SOURCE "$([ "$rc" -ne 0 ] && [ ! -e "$bsdir" ] \
+  && printf '%s' "$out" \
+    | grep -qF "rejected source='https://evil.example.com/x/y'" \
+  && echo 1 || echo 0)"
+
+# ── REJECTS_BAD_PATH — a lock entry whose "path" walks outside the repo
+# ("../x") is rejected before any URL is built.
+out="$(vendor_pinned_skills bad-path-key 2>&1)"
+rc=$?
+bpdir="$FIXTURE_REPO/skills-external/skill-badpath-a"
+check_bool REJECTS_BAD_PATH "$([ "$rc" -ne 0 ] && [ ! -e "$bpdir" ] \
+  && printf '%s' "$out" | grep -qF "rejected path='../x'" && echo 1 || echo 0)"
+
 echo "PASS=$pass FAIL=$fail"
 [ "$fail" -eq 0 ]

lib/vendor-skills.sh

@@ -34,8 +34,14 @@
 #
 # Every skill name and file path from the lock is rejected — before any
 # URL is built or any file fetched — if it contains "..", starts with
-# "/", or holds a character outside [A-Za-z0-9._/-]; this guards against
-# a lock entry walking a fetch outside skills-external/<skill>/.
+# "/", or holds a character outside [A-Za-z0-9._/-] (checked with
+# re.fullmatch, so a trailing newline or other stray character cannot
+# slip past the "$" anchor the way it could under re.match); this guards
+# against a lock entry walking a fetch outside skills-external/<skill>/.
+# The entry's own "commit" (must be 40 lowercase hex chars), "source"
+# (must be "https://github.com/<owner>/<repo>", trailing slash optional)
+# and "path" (same SAFE class as a file, no traversal) are format-checked
+# the same way, before either is ever spliced into the raw-file URL.
 #
 # No `set -euo pipefail` here (mirrors lib/detect-plugins.sh): a sourced
 # lib must not change the caller's shell options.
@@ -71,10 +77,12 @@ fi
 # is a plain argv string — the caller (vendor_pinned_skills) already
 # checked it is either empty or a "file://" value, never the raw
 # VENDOR_BASE_URL.
-# rc 1 when the key, its commit or its skills are absent (nothing
-# printed), or when a skill name or file path fails the traversal/
-# character check (prints one "INVALID\t<offending value>" line, nothing
-# else — no URL is built and no file is fetched for that lock key).
+# rc 1 when the key, its commit, source or skills are absent (nothing
+# printed); when the commit, source or path fails its format check
+# (prints one "INVALID\t<field>=<offending value>" line); or when a
+# skill name or file path fails the traversal/character check (prints
+# one "INVALID\t<offending value>" line) — no URL is built and no file
+# is fetched for that lock key either way.
 # Reads the lock path, key and base override via argv — never
 # string-spliced into the script.
 _vendor_read_lock() {
@@ -82,10 +90,13 @@ _vendor_read_lock() {
 import json, re, sys
 
 SAFE = re.compile(r'^[A-Za-z0-9._/-]+$')
+COMMIT_RE = re.compile(r'^[0-9a-f]{40}$')
+SOURCE_RE = re.compile(
+    r'^https://github\.com/[A-Za-z0-9._-]+/[A-Za-z0-9._-]+/?$')
 
 
 def unsafe(value):
-    return ".." in value or value.startswith("/") or not SAFE.match(value)
+    return ".." in value or value.startswith("/") or not SAFE.fullmatch(value)
 
 
 lockfile, key, base_override = sys.argv[1], sys.argv[2], sys.argv[3]
@@ -93,13 +104,23 @@ with open(lockfile) as f:
     data = json.load(f)
 entry = data.get(key, {})
 sha = entry.get("commit", "")
-owner_repo = entry.get("source", "").rstrip("/").rsplit("github.com/", 1)[-1]
+source = entry.get("source", "")
 path = entry.get("path", "skills")
 skills = entry.get("skills", {})
 if isinstance(skills, list):
     skills = {name: ["SKILL.md"] for name in skills}
-if not sha or not owner_repo or not skills:
+if not sha or not source or not skills:
     sys.exit(1)
+if not COMMIT_RE.fullmatch(sha):
+    print(f"INVALID\tcommit={sha}")
+    sys.exit(1)
+if not SOURCE_RE.fullmatch(source):
+    print(f"INVALID\tsource={source}")
+    sys.exit(1)
+if unsafe(path):
+    print(f"INVALID\tpath={path}")
+    sys.exit(1)
+owner_repo = source.rstrip("/").rsplit("github.com/", 1)[-1]
 for name, files in skills.items():
     if unsafe(name):
         print(f"INVALID\t{name}")
@@ -148,14 +169,21 @@ _vendor_install_skill() {
 }
 
 # _vendor_report_lock_error <lock_key> <lock_out> — turns a failed
-# _vendor_read_lock into the right err line: a rejected name/path when
-# <lock_out> carries the "INVALID\t<value>" marker, the generic
-# no-commit-pinned hint otherwise.
+# _vendor_read_lock into the right err line: a rejected commit/source/
+# path when <lock_out> carries the "INVALID\t<field>=<value>" marker (the
+# field named in full), a rejected skill name/file when it carries the
+# plain "INVALID\t<value>" marker, the generic no-commit-pinned hint
+# otherwise.
 _vendor_report_lock_error() {
-  local lock_key="$1" lock_out="$2" msg
+  local lock_key="$1" lock_out="$2" rest msg
   if [[ "$lock_out" == INVALID$'\t'* ]]; then
-    msg="$lock_key: rejected '${lock_out#INVALID$'\t'}'"
+    rest="${lock_out#INVALID$'\t'}"
+    if [[ "$rest" == *=* ]]; then
+      msg="$lock_key: rejected ${rest%%=*}='${rest#*=}' — invalid format"
+    else
+      msg="$lock_key: rejected '$rest'"
       msg="$msg — path traversal or disallowed characters"
+    fi
     err "$msg"
     return
   fi