diff --git a/CHANGELOG.md b/CHANGELOG.md index 3cbd974..bdf2cc3 100644 --- a/CHANGELOG.md +++ b/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 [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, diff --git a/lib/tests/vendor-skills.test.sh b/lib/tests/vendor-skills.test.sh index f96f062..9169ab5 100755 --- a/lib/tests/vendor-skills.test.sh +++ b/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/ 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//" 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 ] diff --git a/lib/vendor-skills.sh b/lib/vendor-skills.sh index 3a6ac47..8880703 100644 --- a/lib/vendor-skills.sh +++ b/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//. +# "/", 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//. +# The entry's own "commit" (must be 40 lowercase hex chars), "source" +# (must be "https://github.com//", 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" 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=" line); or when a +# skill name or file path fails the traversal/character check (prints +# one "INVALID\t" 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 — turns a failed -# _vendor_read_lock into the right err line: a rejected name/path when -# carries the "INVALID\t" marker, the generic -# no-commit-pinned hint otherwise. +# _vendor_read_lock into the right err line: a rejected commit/source/ +# path when carries the "INVALID\t=" marker (the +# field named in full), a rejected skill name/file when it carries the +# plain "INVALID\t" 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'}'" - msg="$msg — path traversal or disallowed characters" + 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