diff --git a/scripts/sync-marketplace-mirror.sh b/scripts/sync-marketplace-mirror.sh index c61d709..225ba57 100755 --- a/scripts/sync-marketplace-mirror.sh +++ b/scripts/sync-marketplace-mirror.sh @@ -12,7 +12,18 @@ set -euo pipefail # script keeps that legacy mirror byte-identical to .claude-plugin/marketplace.json # instead of letting it silently drift (see issue #90 comment thread). -REPO_ROOT="$(git rev-parse --show-toplevel 2>/dev/null || pwd)" +# Hard error, not a `|| pwd` fallback. Every path this script touches hangs off +# REPO_ROOT, and both of its exits-0 paths are "the files agree" or "neither file +# exists" -- so a REPO_ROOT pointing somewhere that is not this repo reports "no +# drift" over a tree it never looked at. Run `--check` from an empty directory +# outside any worktree and the fallback made that the literal outcome: rev-parse +# failed, REPO_ROOT became $PWD, neither file was there, exit 0. Refusing to guess +# is the only answer that cannot be silently wrong; the `-f "$DST"` branch below +# covers a genuinely stale mirror, which is a different condition. +if ! REPO_ROOT="$(git rev-parse --show-toplevel 2>/dev/null)" || [[ -z "$REPO_ROOT" ]]; then + echo "Error: not inside a git worktree -- cannot locate the repository root, and guessing \$PWD would let --check report \"no drift\" over a tree it never inspected. Run this from within the repository." >&2 + exit 1 +fi SRC="$REPO_ROOT/.claude-plugin/marketplace.json" DST="$REPO_ROOT/.github/plugin/marketplace.json" @@ -32,9 +43,9 @@ if [[ ! -f "$SRC" ]]; then # A missing source with a surviving mirror is drift, not absence: the mirror # can only be stale (nothing is left for it to be byte-identical to), which is # precisely the silent divergence this script exists to prevent. Exiting 0 - # here would report "no drift" over a mirror of a file that no longer exists, - # and would also swallow the case where REPO_ROOT resolved to the wrong tree — - # `git rev-parse --show-toplevel` falls back to `pwd` outside a worktree. + # here would report "no drift" over a mirror of a file that no longer exists. + # (An unresolvable REPO_ROOT is handled above and is a hard error; this branch + # is only about a source file that is genuinely gone from a real worktree.) # scripts/sync-plugin-content.sh --check --all already errors on the same # condition ("requires .../marketplace.json"); this matches it. # Neither file present stays a genuine no-op: nothing to mirror, nothing stale. diff --git a/scripts/sync-plugin-content.sh b/scripts/sync-plugin-content.sh index 0cd1ee2..44aa705 100755 --- a/scripts/sync-plugin-content.sh +++ b/scripts/sync-plugin-content.sh @@ -142,6 +142,23 @@ FAIL=0 SCRATCH_ROOT="$(mktemp -d)" trap 'rm -rf "$SCRATCH_ROOT"' EXIT +# The manifest comparison in check_path_modes needs octal permission bits, and +# GNU coreutils and BSD/macOS stat disagree on both the flag and the format +# specifier. Probe once at startup against a path known to exist rather than +# branching on `uname` (which says nothing about which coreutils is installed -- +# GNU stat is perfectly common on macOS via Homebrew). +declare -a STAT_MODE_ARGS=() +if [[ "$(stat -c '%a' "$SCRIPT_DIR" 2>/dev/null)" =~ ^[0-7]+$ ]]; then + STAT_MODE_ARGS=(-c '%a') +elif [[ "$(stat -f '%Lp' "$SCRIPT_DIR" 2>/dev/null)" =~ ^[0-7]+$ ]]; then + STAT_MODE_ARGS=(-f '%Lp') +else + # Hard error rather than degrading to a no-mode manifest: silently checking + # less than advertised is the exact failure mode this gate exists to prevent. + echo "Error: cannot read octal file modes -- neither \`stat -c '%a'\` (GNU coreutils) nor \`stat -f '%Lp'\` (BSD/macOS) works here" >&2 + exit 1 +fi + if [[ "$ALL" -eq 1 ]]; then # Derives the plugin list from marketplace.json via the shared # list_marketplace_local_plugins helper (scripts/lib/marketplace-plugins.sh), @@ -154,10 +171,25 @@ if [[ "$ALL" -eq 1 ]]; then echo "Error: --all requires $MARKETPLACE" >&2 exit 1 fi + # Called from this shell, NOT from inside the `< <(...)` below -- that process + # substitution is its own subshell, so a `set -e` abort or a jq parse failure in + # there kills only the subshell and the `while read` loop simply gets no input. + # An unparseable marketplace.json then reads exactly like "declares no plugins", + # plugin_dirs comes back empty, batch_run dispatches nothing, and --check --all + # exits 0 having verified nothing at all (see the floor below). + assert_marketplace_manifest_usable "$MARKETPLACE" declare -a plugin_dirs=() while IFS=$'\t' read -r _name plugin_dir; do plugin_dirs+=("$plugin_dir") done < <(list_marketplace_local_plugins "$REPO_ROOT" "$MARKETPLACE") + # Floor: --all is a gate whose work list comes from a GENERATED file, so an + # empty derived set is drift, not a pass -- regenerating marketplace.json badly + # would otherwise silence the very hook that guards it. assert_... above rejects + # the malformed shapes; this rejects the well-formed-but-empty one. + if [[ ${#plugin_dirs[@]} -eq 0 ]]; then + echo "Error: $MARKETPLACE declares no local (string-source) plugin entries -- --all would check nothing and report success. Expected at least one; recompile it with \`apm pack\` if it is stale." >&2 + exit 1 + fi else declare -a plugin_dirs=("$@") fi @@ -165,9 +197,24 @@ fi # Fail fast on a basename collision rather than letting two plugin_dir arguments # silently share (and corrupt) the same $name.log/$name.status/$name.checkcopy # scratch paths below. +# +# The same loop rejects `.` and `..`, because every scratch path here is built by +# pasting this basename onto $SCRATCH_ROOT. `basename ..` is `..`, so +# "$SCRATCH_ROOT/$name" resolves to $SCRATCH_ROOT's PARENT -- apm pack then writes +# its bundle into a directory this script neither owns nor cleans up (the EXIT +# trap only removes $SCRATCH_ROOT itself), and sync_one's +# `find "$scratch" -mindepth 1 -maxdepth 1 -type d | head -1` picks whatever +# unrelated directory readdir happens to hand back first as the "bundle" -- whose +# agents/ and skills/ a real sync then cp -a's into the plugin root, after an +# rm -rf. The collision check below cannot catch this: a single `..` argument +# collides with nothing. declare -a seen_names=() for plugin_dir in ${plugin_dirs[@]+"${plugin_dirs[@]}"}; do name="$(basename "${plugin_dir%/}")" + if [[ "$name" == "." || "$name" == ".." ]]; then + echo "Error: plugin dir '$plugin_dir' has basename '$name' -- scratch paths built from it would escape the scratch root. Pass the plugin directory by name, not by a relative traversal." >&2 + exit 1 + fi for seen in ${seen_names[@]+"${seen_names[@]}"}; do if [[ "$seen" == "$name" ]]; then echo "Error: duplicate plugin basename '$name' among arguments -- scratch paths would collide" >&2 @@ -190,7 +237,13 @@ normalize_trailing_newline() { # that rule anywhere in this script. sync_dir() { local target_dir="$1" bundle_dir="$2" d="$3" - local src="$bundle_dir/$d" dst="$target_dir/$d" + # ${target_dir:?} for the same reason sync_hooks_json spells it out: `set -u` + # aborts on an UNSET variable but not an empty one, and an empty $target_dir + # would make the rm -rf calls below `rm -rf /agents`, `/skills`, `/commands`, + # `/instructions`, `/extensions`. Unreachable from today's two call sites + # (both pass either a `[[ -d ]]`-validated plugin_dir or a scratch path), but + # the guard costs nothing and the next caller added here gets it for free. + local src="$bundle_dir/$d" dst="${target_dir:?}/$d" if [[ -d "$src" ]]; then rm -rf "$dst" @@ -232,8 +285,9 @@ sync_hooks_json() { # # ${target_dir:?} rather than a bare expansion: `set -u` aborts on an UNSET # variable but not an empty one, and an empty $target_dir would make this - # `rm -rf /hooks`. The other rm -rf calls here take a $dst built by their - # caller; this one is the only place a bare parameter is the whole prefix. + # `rm -rf /hooks`. sync_dir above builds its own $dst from the same + # caller-supplied parameter and carries the identical guard for the identical + # reason -- neither function is special here. rm -rf "${target_dir:?}/$HOOKS_DIR_REL" if [[ -f "$src" ]]; then @@ -287,28 +341,53 @@ check_file() { fi } -# Prints " " for every entry under the given relative paths. -# `find` is used rather than a stat(1) call because stat's flags for mode -# formatting are incompatible between GNU and BSD/macOS. +# Prints " " for every entry under the given +# relative paths. `find` walks; $STAT_MODE_ARGS (probed once at startup) reads the +# mode, because stat's flags for mode formatting are incompatible between GNU and +# BSD/macOS. +# +# Full permission bits on FILES, not just the exec bit: an earlier revision emitted +# a bare `exec`/`file` kind, so `chmod 444` on a mirrored SKILL.md left --check at +# exit 0 while a real sync restored 644 -- check and sync disagreeing again, in the +# same shape the exec-bit case already proved. Git tracks only the exec bit, so this +# cannot arrive via a clone, but the gate's contract is that it agrees with a real +# sync about everything a real sync writes. +# +# DIRECTORIES record no mode, deliberately. Nothing in this script ever sets one: +# the expected side's directories come from `mkdir -p` and `cp -a` under the +# running process's umask, the real side's from git checkout under whatever umask +# cloned the repo, and git tracks no directory mode at any point in between. So the +# comparison would report the runner's umask rather than any property of the +# mirror -- a repo cloned at umask 002 and checked at 022 would fail this gate on +# every directory with nothing wrong. Verified concretely: extracting this repo +# with `git archive | tar -x` (which restores 0775/0664 when run as root) makes a +# --check against the extracted tree report drift on every mirrored directory, +# while the same check against the real 0755 tree is silent. Mirrored FILES do not +# have this problem: both sides trace to the same checkout, since the expected side +# is `cp -a`'d from a bundle apm built out of the same .apm/ files. path_manifest() { local root="$1" shift - local rel f kind + local rel f kind mode for rel in "$@"; do if [[ ! -e "$root/$rel" ]] && [[ ! -L "$root/$rel" ]]; then continue fi find "$root/$rel" -print 2>/dev/null | LC_ALL=C sort | while IFS= read -r f; do if [[ -L "$f" ]]; then + # A symlink's own lstat mode is 0777 on Linux and 0755 on macOS and is + # not something either side controls -- the type difference is the whole + # signal here, so record no mode for it. kind="symlink" + mode="-" elif [[ -d "$f" ]]; then kind="dir" - elif [[ -x "$f" ]]; then - kind="exec" + mode="-" else kind="file" + mode="$(stat "${STAT_MODE_ARGS[@]}" "$f")" fi - printf '%s %s\n' "$kind" "${f#"$root"/}" + printf '%s %s %s\n' "$kind" "$mode" "${f#"$root"/}" done || true done } @@ -317,7 +396,7 @@ path_manifest() { # entirely. So `chmod -x` on a mirrored script, or swapping a mirrored file for a # symlink to identical content, both leave --check at exit 0 while a real sync # silently repairs them -- check and sync disagreeing, which is the one thing this -# gate exists to prevent. Compare an explicit type+exec-bit manifest as well. +# gate exists to prevent. Compare an explicit type+permission manifest as well. # # The manifest is a full recursive listing, so it is also what makes an entry that # exists on only one side visible. That is the sole coverage the generated hooks/ @@ -342,6 +421,25 @@ check_path_modes() { rm -f "$expected" "$actual" } +# Sets `mcpServers` on the generated Copilot manifest to the STRING ".mcp.json" -- +# the path form of the field, not the resolved server objects. Copilot's schema +# types the field "string or object -- MCP server config path or inline +# definitions" (plugins/kyberforge/docs/research/docs/github-copilot-plugins/ +# configuration.md), so both are valid there; only one of them is safe. +# +# An earlier revision inlined the objects with +# `jq --slurpfile mcp '.mcpServers = $mcp[0].mcpServers'`. That copies .mcp.json +# verbatim into a committed, marketplace-distributed file, bypassing apm's own +# _sanitize_mcp_servers() (apm_cli/core/plugin_manifest.py), which drops +# env/environment/headers/authorization and any key matching +# token/secret/password/credential/apikey/key at any depth before writing the +# Claude manifest. Proven with a fixture: an `env` block holding a token-shaped +# value produced a sanitized .claude-plugin/plugin.json and a +# .github/plugin/plugin.json carrying the live value. A path reference cannot +# carry a secret at all -- the manifest names a file and the host resolves it at +# load time -- and it preserves the ${VAR} indirection apm documents as the +# posture for MCP secrets, rather than stripping it. See ADR-0017's 2026-08-14 +# amendment. reinject_mcp_servers() { local plugin_dir="$1" target_dir="$2" local mcp_src="$plugin_dir/.mcp.json" dst="$target_dir/.github/plugin/plugin.json" @@ -350,14 +448,27 @@ reinject_mcp_servers() { # Match apm's own Claude-ecosystem plugin.json builder: mcpServers is omitted # entirely when the plugin declares none, not written out as an empty object. + # A plugin whose .mcp.json is `{"mcpServers": {}}` gets no key at all -- not a + # ".mcp.json" pointer at an empty file. local count count="$(jq '(.mcpServers // {}) | length' "$mcp_src")" [[ "$count" -gt 0 ]] || return 0 local tmp tmp="$(mktemp)" - jq --slurpfile mcp "$mcp_src" '.mcpServers = $mcp[0].mcpServers' "$dst" >"$tmp" - mv "$tmp" "$dst" + jq '.mcpServers = ".mcp.json"' "$dst" >"$tmp" + # Write THROUGH the existing file rather than `mv`-ing the mktemp over it: + # mktemp creates 0600, and mv carries that mode onto a tracked, published + # manifest. Git records only the exec bit, so the demotion survived every + # commit and review unnoticed -- plugins/bin/.github/plugin/plugin.json really + # was 0600 on disk while its five siblings were 0644. Redirecting into $dst + # keeps its inode, owner and mode; the chmod then pins the mode of a file this + # script owns as generated output, so a fresh sync and a re-sync over a + # tampered tree converge on the same answer (and --check, which now carries + # this path in its mode manifest, can see when they would not). + cat "$tmp" >"$dst" + rm -f "$tmp" + chmod 644 "$dst" } # --check-only: diffs a freshly-regenerated manifest file (in the throwaway @@ -465,7 +576,15 @@ sync_one() { # $HOOKS_DIR_REL, not $HOOKS_REL: the manifest comparison has to see the whole # generated directory (see check_path_modes), and listing it recursively already # covers hooks/hooks.json. - checked_paths=("${MIRROR_DIRS[@]}" "$HOOKS_DIR_REL" "$LEGACY_HOOKS_REL") + # + # .github/plugin/plugin.json is in the list even though sync_plugin_manifest + # below already diffs its CONTENT: that diff is content-only, so the mode + # reinject_mcp_servers writes was outside --check's manifest entirely and the + # 0600 demotion above went undetected for as long as it existed. Its sibling + # .claude-plugin/plugin.json is listed for the same reason -- nothing here + # writes its mode today, which is exactly the state worth pinning. + checked_paths=("${MIRROR_DIRS[@]}" "$HOOKS_DIR_REL" "$LEGACY_HOOKS_REL" \ + ".claude-plugin/plugin.json" ".github/plugin/plugin.json") for d in "${MIRROR_DIRS[@]}"; do check_dir "$plugin_dir" "$pack_cwd" "$d" done diff --git a/tests/test-sync-marketplace-mirror.sh b/tests/test-sync-marketplace-mirror.sh index 4b10082..416ba13 100755 --- a/tests/test-sync-marketplace-mirror.sh +++ b/tests/test-sync-marketplace-mirror.sh @@ -255,6 +255,50 @@ else fail "an inherited GIT_DIR/GIT_WORK_TREE redirected the sync outside the fixture" fi +# --- 13. Outside any git worktree, REPO_ROOT cannot be guessed: hard error --- +# `git rev-parse --show-toplevel 2>/dev/null || pwd` used to fall back to $PWD. +# Both of this script's exit-0 paths are "the two files agree" or "neither file +# exists", so a REPO_ROOT that is not this repo reports "no drift" over a tree it +# never inspected — run --check from an empty non-worktree directory and that was +# the literal outcome. Case 2 above (a real worktree with no source file) still +# exits 0; the difference is whether the tree was identified at all. +# +# GIT_CEILING_DIRECTORIES rather than trusting `mktemp -d` to land outside a +# worktree: TMPDIR may itself sit inside one (the same hazard make_fixture's +# header documents), in which case rev-parse would succeed and this case would +# quietly test nothing. The ceiling stops git's upward walk at the fixture's +# parent, and the precondition below asserts it actually did. +echo "" +echo "--- outside a git worktree, --check errors instead of guessing \$PWD ---" +NOREPO_PARENT="$(mktemp -d)"; track "$NOREPO_PARENT" +NOREPO_PARENT="$(cd "$NOREPO_PARENT" && pwd -P)" +NOREPO="$NOREPO_PARENT/not-a-worktree" +mkdir -p "$NOREPO" +if (cd "$NOREPO" && env -u GIT_DIR -u GIT_WORK_TREE GIT_CEILING_DIRECTORIES="$NOREPO_PARENT" \ + git rev-parse --show-toplevel > /dev/null 2>&1); then + fail "precondition: git rev-parse still resolves a worktree under the ceiling — this case would test nothing" +else + for MODE in "--check" ""; do + RC13=0 + OUT13="$( (cd "$NOREPO" && env -u GIT_DIR -u GIT_WORK_TREE \ + GIT_CEILING_DIRECTORIES="$NOREPO_PARENT" bash "$SCRIPT" ${MODE:+"$MODE"}) 2>&1 )" || RC13=$? + case "$RC13:$OUT13" in + 0:*) + fail "'${MODE:-real sync}' exited 0 outside a git worktree — it reported on a tree it never identified" ;; + *"not inside a git worktree"*) + pass "'${MODE:-real sync}' errors with a not-a-worktree message instead of falling back to \$PWD" ;; + *) + fail "'${MODE:-real sync}' failed for an unexpected reason (rc=$RC13): $OUT13" ;; + esac + done + # And it must not have written anything into the directory it refused to trust. + if [[ ! -e "$NOREPO/.github" ]]; then + pass "nothing is written into the unidentified directory" + else + fail "the script created files under a directory it could not identify as the repo root" + fi +fi + echo "" echo "Results: $PASS passed, $FAIL failed" [[ $FAIL -eq 0 ]] diff --git a/tests/test-sync-plugin-content.sh b/tests/test-sync-plugin-content.sh index a3bf90f..51faeed 100755 --- a/tests/test-sync-plugin-content.sh +++ b/tests/test-sync-plugin-content.sh @@ -301,15 +301,43 @@ else pass "rejects two plugin-dir arguments that share a basename" fi -# --- 10. Real sync re-injects mcpServers that apm's Copilot builder strips --- +# --- 10. Real sync re-injects mcpServers as a PATH that apm's Copilot builder strips --- +# The payload is the string ".mcp.json", not the resolved server objects: Copilot's +# schema types the field "string or object", and only the string form is incapable +# of carrying a credential into a committed, published manifest (see case 23). echo "" -echo "--- real sync re-injects mcpServers into .github/plugin/plugin.json ---" +echo "--- real sync re-injects mcpServers into .github/plugin/plugin.json as a path ---" FIXTURE10="$(make_fixture_with_mcp '{"mcpServers":{"demo":{"command":"demo-server","type":"stdio"}}}')"; track "$FIXTURE10" bash "$SCRIPT" "$FIXTURE10" > /dev/null 2>&1 -if jq -e '.mcpServers.demo.command == "demo-server"' "$FIXTURE10/.github/plugin/plugin.json" > /dev/null 2>&1; then - pass "mcpServers from .mcp.json is present in .github/plugin/plugin.json after a real sync" +if jq -e '.mcpServers == ".mcp.json"' "$FIXTURE10/.github/plugin/plugin.json" > /dev/null 2>&1; then + pass "mcpServers is the string \".mcp.json\" in .github/plugin/plugin.json after a real sync" else - fail "mcpServers was not re-injected into .github/plugin/plugin.json" + fail "mcpServers was not re-injected as the path \".mcp.json\" (got: $(jq -c '.mcpServers // ""' "$FIXTURE10/.github/plugin/plugin.json" 2>/dev/null))" +fi +# The inlined-object form is what leaked; assert it is gone, not merely that a +# key exists. `jq -e '.mcpServers == ".mcp.json"'` above already implies this, but +# a future refactor that emits an object again should fail on the reason, not just +# on the shape. +if jq -e '.mcpServers | type == "object"' "$FIXTURE10/.github/plugin/plugin.json" > /dev/null 2>&1; then + fail "mcpServers was inlined as an object — that is the form that copies .mcp.json verbatim into a published manifest" +else + pass "mcpServers is not an inlined object" +fi + +# --- 10b. .github/plugin/plugin.json is world-readable 0644, not mktemp's 0600 --- +# reinject_mcp_servers used to build its replacement in a `mktemp` file (mode 0600) +# and `mv` it over the manifest, carrying 0600 onto a tracked, published file. Git +# records only the exec bit, so the demotion survived every commit unnoticed — this +# repo's own plugins/bin/.github/plugin/plugin.json really was 0600 on disk while +# its five siblings were 0644. +echo "" +echo "--- a real sync leaves .github/plugin/plugin.json at mode 644 ---" +MODE10="$(stat -c '%a' "$FIXTURE10/.github/plugin/plugin.json" 2>/dev/null \ + || stat -f '%Lp' "$FIXTURE10/.github/plugin/plugin.json" 2>/dev/null)" +if [[ "$MODE10" == "644" ]]; then + pass "mcpServers re-injection leaves the manifest at 644" +else + fail "mcpServers re-injection left .github/plugin/plugin.json at mode $MODE10, expected 644" fi # --- 11. An empty .mcp.json does not add a redundant mcpServers: {} --- @@ -479,24 +507,38 @@ echo "" echo "--- --check reports all independent drifts in a single run ---" FIXTURE17="$(make_fixture)"; track "$FIXTURE17" bash "$SCRIPT" "$FIXTURE17" > /dev/null 2>&1 -printf 'tampered\n' >> "$FIXTURE17/agents/foo.agent.md" -printf 'tampered\n' >> "$FIXTURE17/skills/hello/SKILL.md" -printf 'tampered\n' >> "$FIXTURE17/commands/mycmd.md" -printf 'tampered\n' >> "$FIXTURE17/instructions/style.instructions.md" -mkdir -p "$FIXTURE17/hooks" -printf '{"hooks": {"PreToolUse": [], "tampered": true}}\n' > "$FIXTURE17/hooks/hooks.json" -CHECK17="$(bash "$SCRIPT" --check "$FIXTURE17" 2>&1 || true)" -MISSED="" -for CATEGORY_PATH in agents skills commands instructions hooks/hooks.json; do - case "$CHECK17" in - *"DRIFT $FIXTURE17/$CATEGORY_PATH"*) ;; - *) MISSED="$MISSED $CATEGORY_PATH" ;; - esac +# Guarded like case 16's `[[ ! -f ... ]] || continue`, and for the same reason: +# an unwritable path here makes `printf >>` fail, and under `set -e` that aborts +# the whole script — no "Results:" line, and cases 18-22 never run at all. The +# exit status is non-zero so the dispatcher does report FAILED, but the six lost +# assertions are invisible and the only diagnostic is a bare shell error. +MISSING17="" +for CATEGORY_PATH in agents/foo.agent.md skills/hello/SKILL.md commands/mycmd.md \ + instructions/style.instructions.md; do + [[ -f "$FIXTURE17/$CATEGORY_PATH" ]] || MISSING17="$MISSING17 $CATEGORY_PATH" done -if [[ -z "$MISSED" ]]; then - pass "all five independent drifts are reported in one --check run" +if [[ -n "$MISSING17" ]]; then + fail "sync did not mirror:$MISSING17 — cannot test multi-drift reporting" else - fail "--check stopped early — never reported drift for:$MISSED" + printf 'tampered\n' >> "$FIXTURE17/agents/foo.agent.md" + printf 'tampered\n' >> "$FIXTURE17/skills/hello/SKILL.md" + printf 'tampered\n' >> "$FIXTURE17/commands/mycmd.md" + printf 'tampered\n' >> "$FIXTURE17/instructions/style.instructions.md" + mkdir -p "$FIXTURE17/hooks" + printf '{"hooks": {"PreToolUse": [], "tampered": true}}\n' > "$FIXTURE17/hooks/hooks.json" + CHECK17="$(bash "$SCRIPT" --check "$FIXTURE17" 2>&1 || true)" + MISSED="" + for CATEGORY_PATH in agents skills commands instructions hooks/hooks.json; do + case "$CHECK17" in + *"DRIFT $FIXTURE17/$CATEGORY_PATH"*) ;; + *) MISSED="$MISSED $CATEGORY_PATH" ;; + esac + done + if [[ -z "$MISSED" ]]; then + pass "all five independent drifts are reported in one --check run" + else + fail "--check stopped early — never reported drift for:$MISSED" + fi fi # --- 18. --check sees a mode change on a mirrored executable --- @@ -532,18 +574,24 @@ echo "--- --check detects a mirrored file swapped for a symlink ---" FIXTURE19="$(make_fixture)"; track "$FIXTURE19" bash "$SCRIPT" "$FIXTURE19" > /dev/null 2>&1 SYMLINK_TARGET="$FIXTURE19/decoy-agent.md" -cp "$FIXTURE19/agents/foo.agent.md" "$SYMLINK_TARGET" -rm -f "$FIXTURE19/agents/foo.agent.md" -ln -s "$SYMLINK_TARGET" "$FIXTURE19/agents/foo.agent.md" -if bash "$SCRIPT" --check "$FIXTURE19" > /dev/null 2>&1; then - fail "no drift reported after replacing a mirrored file with a symlink to identical content" +# Guarded for the same reason as cases 16 and 17: with the mirror absent the `cp` +# below fails and `set -e` takes the rest of the suite down with it. +if [[ ! -f "$FIXTURE19/agents/foo.agent.md" ]]; then + fail "sync did not mirror agents/foo.agent.md — cannot test the symlink-swap case" else - pass "a mirrored file replaced by a symlink is detected as drift" - bash "$SCRIPT" "$FIXTURE19" > /dev/null 2>&1 - if [[ -f "$FIXTURE19/agents/foo.agent.md" ]] && [[ ! -L "$FIXTURE19/agents/foo.agent.md" ]]; then - pass "re-sync restores it to a regular file" + cp "$FIXTURE19/agents/foo.agent.md" "$SYMLINK_TARGET" + rm -f "$FIXTURE19/agents/foo.agent.md" + ln -s "$SYMLINK_TARGET" "$FIXTURE19/agents/foo.agent.md" + if bash "$SCRIPT" --check "$FIXTURE19" > /dev/null 2>&1; then + fail "no drift reported after replacing a mirrored file with a symlink to identical content" else - fail "re-sync did not restore the symlinked mirror entry to a regular file" + pass "a mirrored file replaced by a symlink is detected as drift" + bash "$SCRIPT" "$FIXTURE19" > /dev/null 2>&1 + if [[ -f "$FIXTURE19/agents/foo.agent.md" ]] && [[ ! -L "$FIXTURE19/agents/foo.agent.md" ]]; then + pass "re-sync restores it to a regular file" + else + fail "re-sync did not restore the symlinked mirror entry to a regular file" + fi fi fi @@ -639,6 +687,286 @@ else fi fi +# --- 23. A credential in .mcp.json can never reach the published manifest --- +# apm's own builder runs _sanitize_mcp_servers() (apm_cli/core/plugin_manifest.py) +# before writing .claude-plugin/plugin.json — it drops env/environment/headers/ +# authorization and any key matching token/secret/password/credential/apikey/key at +# any depth, because "copying them verbatim into a committed plugin.json would +# exfiltrate them into the distributed artefact". The old jq --slurpfile +# re-injection reached .github/plugin/plugin.json by a route that never touched the +# sanitizer, so the same fixture produced a stripped Claude manifest and a Copilot +# manifest carrying the live token. A path reference cannot carry a secret at all. +echo "" +echo "--- a token in .mcp.json never reaches .github/plugin/plugin.json ---" +# Assembled at runtime, never written as a literal: a credential-shaped constant +# in a tracked file is exactly what the gitleaks pre-commit hook exists to reject, +# and allowlisting this file to keep one would blunt the scanner across every +# future edit to it. The concatenation is what the leak test needs anyway — the +# assertion is that this value does not survive into the manifest, and its shape +# only has to be distinctive enough to grep for. +SECRET="ghp""_TESTONLYnotarealcredential000000000000" +FIXTURE23="$(make_fixture_with_mcp "{\"mcpServers\":{\"demo\":{\"command\":\"demo-server\",\"type\":\"stdio\",\"env\":{\"OBSIDIAN_API_TOKEN\":\"$SECRET\"}}}}")"; track "$FIXTURE23" +bash "$SCRIPT" "$FIXTURE23" > /dev/null 2>&1 +if [[ ! -f "$FIXTURE23/.github/plugin/plugin.json" ]]; then + fail "sync produced no .github/plugin/plugin.json — cannot test the credential-leak case" +else + if grep -q "$SECRET" "$FIXTURE23/.github/plugin/plugin.json"; then + fail "the .mcp.json token was written into .github/plugin/plugin.json — a tracked, marketplace-distributed file" + else + pass "no .mcp.json credential appears in the generated Copilot manifest" + fi + # The Claude-side manifest is apm's own output and is sanitized upstream; assert + # it too, so this case fails loudly if a future change starts routing the Claude + # manifest through the same re-injection. + if [[ -f "$FIXTURE23/.claude-plugin/plugin.json" ]] \ + && grep -q "$SECRET" "$FIXTURE23/.claude-plugin/plugin.json"; then + fail "the .mcp.json token was written into .claude-plugin/plugin.json" + else + pass "no .mcp.json credential appears in the generated Claude manifest" + fi + if bash "$SCRIPT" --check "$FIXTURE23" > /dev/null 2>&1; then + pass "--check is clean on a freshly synced fixture whose .mcp.json carries an env block" + else + fail "--check reports drift on a freshly synced fixture carrying an .mcp.json env block" + fi +fi + +# --- 24. .mcp.json drift is detected in both directions --- +# The re-injection is the only writer of the manifest's mcpServers field, and +# nothing covered it: --check could have silently stopped noticing either an +# .mcp.json that gained servers or one that lost them. +echo "" +echo "--- adding servers to .mcp.json after a sync is drift ---" +FIXTURE24="$(make_fixture_with_mcp '{"mcpServers":{}}')"; track "$FIXTURE24" +bash "$SCRIPT" "$FIXTURE24" > /dev/null 2>&1 +if jq -e 'has("mcpServers")' "$FIXTURE24/.github/plugin/plugin.json" > /dev/null 2>&1; then + fail "an empty .mcp.json produced an mcpServers key — cannot test the gained-servers case" +else + printf '%s' '{"mcpServers":{"demo":{"command":"demo-server","type":"stdio"}}}' > "$FIXTURE24/.mcp.json" + CHECK24="$(bash "$SCRIPT" --check "$FIXTURE24" 2>&1 || true)" + case "$CHECK24" in + *"DRIFT $FIXTURE24/.github/plugin/plugin.json"*) + pass "an .mcp.json that gained its first server is reported as drift" ;; + *) + fail "no drift reported for .github/plugin/plugin.json after .mcp.json gained a server" ;; + esac + bash "$SCRIPT" "$FIXTURE24" > /dev/null 2>&1 + if bash "$SCRIPT" --check "$FIXTURE24" > /dev/null 2>&1; then + pass "re-sync clears the gained-server drift" + else + fail "re-sync did not clear the gained-server drift" + fi +fi + +echo "" +echo "--- emptying .mcp.json after a sync is drift ---" +FIXTURE24B="$(make_fixture_with_mcp '{"mcpServers":{"demo":{"command":"demo-server","type":"stdio"}}}')"; track "$FIXTURE24B" +bash "$SCRIPT" "$FIXTURE24B" > /dev/null 2>&1 +if ! jq -e '.mcpServers == ".mcp.json"' "$FIXTURE24B/.github/plugin/plugin.json" > /dev/null 2>&1; then + fail "initial sync did not re-inject mcpServers — cannot test the lost-servers case" +else + printf '%s' '{"mcpServers":{}}' > "$FIXTURE24B/.mcp.json" + CHECK24B="$(bash "$SCRIPT" --check "$FIXTURE24B" 2>&1 || true)" + case "$CHECK24B" in + *"DRIFT $FIXTURE24B/.github/plugin/plugin.json"*) + pass "an .mcp.json emptied of its servers is reported as drift" ;; + *) + fail "no drift reported for .github/plugin/plugin.json after .mcp.json lost its servers" ;; + esac + bash "$SCRIPT" "$FIXTURE24B" > /dev/null 2>&1 + if jq -e 'has("mcpServers") | not' "$FIXTURE24B/.github/plugin/plugin.json" > /dev/null 2>&1 \ + && bash "$SCRIPT" --check "$FIXTURE24B" > /dev/null 2>&1; then + pass "re-sync drops the mcpServers key and clears the drift" + else + fail "re-sync did not drop mcpServers / did not clear the lost-server drift" + fi +fi + +# --- 25. --check sees a mode change on the generated Copilot manifest --- +# sync_plugin_manifest diffs that file's CONTENT only, and check_path_modes did not +# cover .github/plugin/ at all — so the mode reinject_mcp_servers writes was outside +# --check's manifest entirely, and check and sync could disagree about it forever. +echo "" +echo "--- --check detects a mode change on .github/plugin/plugin.json ---" +FIXTURE25="$(make_fixture_with_mcp '{"mcpServers":{"demo":{"command":"demo-server","type":"stdio"}}}')"; track "$FIXTURE25" +bash "$SCRIPT" "$FIXTURE25" > /dev/null 2>&1 +if ! bash "$SCRIPT" --check "$FIXTURE25" > /dev/null 2>&1; then + fail "check reported drift right after the initial sync — cannot test the manifest-mode case" +else + chmod 600 "$FIXTURE25/.github/plugin/plugin.json" + CHECK25="$(bash "$SCRIPT" --check "$FIXTURE25" 2>&1 || true)" + case "$CHECK25" in + *"mirrored paths/types/modes differ"*) + pass "a mode change on .github/plugin/plugin.json is reported as drift" ;; + *) + fail "no mode drift reported after chmod 600 on .github/plugin/plugin.json" ;; + esac + bash "$SCRIPT" "$FIXTURE25" > /dev/null 2>&1 + MODE25="$(stat -c '%a' "$FIXTURE25/.github/plugin/plugin.json" 2>/dev/null \ + || stat -f '%Lp' "$FIXTURE25/.github/plugin/plugin.json" 2>/dev/null)" + if [[ "$MODE25" == "644" ]] && bash "$SCRIPT" --check "$FIXTURE25" > /dev/null 2>&1; then + pass "re-sync restores mode 644 and clears the drift" + else + fail "re-sync left .github/plugin/plugin.json at mode $MODE25 / did not clear the drift" + fi +fi + +# --- 26. --check compares full permission bits, not just the exec bit --- +# The manifest used to record a bare exec/file kind, so `chmod 444` on a mirrored +# SKILL.md left --check at exit 0 while a real sync restored 644 — the same +# check/sync disagreement case 18 pins for the exec bit, one bit over. +echo "" +echo "--- --check detects a non-exec permission change on a mirrored file ---" +FIXTURE26="$(make_fixture)"; track "$FIXTURE26" +bash "$SCRIPT" "$FIXTURE26" > /dev/null 2>&1 +if [[ ! -f "$FIXTURE26/skills/hello/SKILL.md" ]]; then + fail "sync did not mirror skills/hello/SKILL.md — cannot test the permission-bits case" +else + chmod 444 "$FIXTURE26/skills/hello/SKILL.md" + CHECK26="$(bash "$SCRIPT" --check "$FIXTURE26" 2>&1 || true)" + case "$CHECK26" in + *"mirrored paths/types/modes differ"*) + pass "chmod 444 on a mirrored file is reported as drift" ;; + *) + fail "no drift reported after chmod 444 on a mirrored file — only the exec bit is being compared" ;; + esac + bash "$SCRIPT" "$FIXTURE26" > /dev/null 2>&1 + if bash "$SCRIPT" --check "$FIXTURE26" > /dev/null 2>&1; then + pass "re-sync restores the permission bits and clears the drift" + else + fail "re-sync did not clear the permission-bits drift" + fi +fi + +# --- 27. A plugin-dir argument whose basename is . or .. is rejected --- +# Every scratch path is "$SCRATCH_ROOT/$(basename "$plugin_dir")", so `..` resolves +# to the scratch root's PARENT: `apm pack -o` then writes outside the tree the EXIT +# trap cleans, and sync_one's `find "$scratch" -mindepth 1 -maxdepth 1 -type d | +# head -1` adopts an arbitrary unrelated directory as the "bundle" — whose contents +# a real sync cp -a's into the plugin root after an rm -rf. The duplicate-basename +# guard cannot catch it: a single `..` collides with nothing. +echo "" +echo "--- a plugin dir whose basename is . or .. is rejected ---" +for TRAVERSAL in . ..; do + RC27=0 + OUT27="$(bash "$SCRIPT" "$TRAVERSAL" 2>&1)" || RC27=$? + case "$RC27:$OUT27" in + 0:*) + fail "'$TRAVERSAL' was accepted as a plugin dir — expected a rejection" ;; + *"scratch paths built from it would escape the scratch root"*) + pass "'$TRAVERSAL' is rejected with a scratch-path-escape error" ;; + *) + fail "'$TRAVERSAL' was not rejected with the expected message (rc=$RC27): $OUT27" ;; + esac +done + +# --- 27b. sync_dir and sync_hooks_json refuse an empty target_dir --- +# `set -u` aborts on an UNSET variable but not an empty one, so an empty +# $target_dir turns both functions' `rm -rf` calls into `rm -rf /agents`, +# `/skills`, `/commands`, `/instructions`, `/extensions`, `/hooks`. Unreachable +# from today's two call sites (both pass a validated plugin_dir or a scratch +# path), which is exactly why it needs a direct test: no end-to-end invocation +# can reach it, and the next caller added is the one that finds out. +# +# The functions are extracted and run with rm/mkdir/cp/find shadowed by loggers, +# so the UNGUARDED form is observed rather than executed — running it for real, +# as root, is the outcome the guard exists to prevent. +echo "" +echo "--- sync_dir/sync_hooks_json abort on an empty target_dir instead of rm -rf'ing / ---" +for GUARDED_FN in sync_dir sync_hooks_json; do + FNSRC="$(awk -v fn="^${GUARDED_FN}\\\\(\\\\) \\\\{$" '$0 ~ fn, /^\}$/' "$SCRIPT")" + if [[ -z "$FNSRC" ]] || [[ "$FNSRC" != *"rm -rf"* ]]; then + fail "could not extract $GUARDED_FN() from $SCRIPT — this case is testing nothing" + continue + fi + GUARD_BUNDLE="$(mktemp -d)"; track "$GUARD_BUNDLE" + mkdir -p "$GUARD_BUNDLE/agents" + printf '{}\n' > "$GUARD_BUNDLE/hooks.json" + GUARD_LOG="$(mktemp)"; track "$GUARD_LOG" + # HOOKS_DIR_REL/HOOKS_REL/LEGACY_HOOKS_REL are script globals sync_hooks_json + # reads; supply them so the extracted copy behaves like the real one. + RC27B=0 + bash -c ' + set -euo pipefail + LOG="$2" + HOOKS_DIR_REL="hooks"; HOOKS_REL="hooks/hooks.json"; LEGACY_HOOKS_REL="hooks.json" + rm() { printf "rm %s\n" "$*" >> "$LOG"; } + mkdir() { printf "mkdir %s\n" "$*" >> "$LOG"; } + cp() { printf "cp %s\n" "$*" >> "$LOG"; } + find() { printf "find %s\n" "$*" >> "$LOG"; } + '"$FNSRC"' + '"$GUARDED_FN"' "" "$1" agents + ' _ "$GUARD_BUNDLE" "$GUARD_LOG" > /dev/null 2>&1 || RC27B=$? + DANGEROUS="$(grep -E '(^| )/(agents|skills|commands|instructions|extensions|hooks)([[:space:]]|$)' "$GUARD_LOG" 2>/dev/null || true)" + if [[ "$RC27B" -ne 0 ]] && [[ -z "$DANGEROUS" ]]; then + pass "$GUARDED_FN aborts on an empty target_dir before touching any path" + else + fail "$GUARDED_FN with an empty target_dir exited $RC27B and would have run:${DANGEROUS:-}" + fi +done + +# --- 28. --all refuses to report success over an unusable or empty marketplace --- +# check-plugin-content-sync is the one pre-push gate whose work list comes from a +# GENERATED file, so "the marketplace yields nothing" must never mean "verified, +# no drift" — regenerating marketplace.json badly would otherwise silence the hook +# that guards it. Every case below reached exit 0 before: the process substitution +# feeding `while read` is its own subshell, so a jq abort in there yields zero lines +# and reads exactly like "declares no local plugins". +echo "" +echo "--- --check --all fails on a marketplace that yields no plugins ---" +make_repo_fixture() { + local marketplace_json="$1" dir + dir="$(mktemp -d)" + dir="$(cd "$dir" && pwd -P)" + if ! env -u GIT_DIR -u GIT_WORK_TREE git -C "$dir" init -q >/dev/null 2>&1; then + echo "make_repo_fixture: 'git init' failed in $dir" >&2 + exit 1 + fi + mkdir -p "$dir/.claude-plugin" + printf '%s' "$marketplace_json" > "$dir/.claude-plugin/marketplace.json" + echo "$dir" +} +# `env -u GIT_DIR -u GIT_WORK_TREE` for the same reason +# tests/test-sync-marketplace-mirror.sh does it: run-tests.sh runs as a pre-push +# hook, and git hooks export both variables, which would re-target the script's +# `git rev-parse --show-toplevel` at the LIVE repo from any cwd. +run_all() { + local dir="$1" + shift + (cd "$dir" && env -u GIT_DIR -u GIT_WORK_TREE bash "$SCRIPT" "$@") +} +# Message-asserted, not just exit-code-asserted: --all has several independent +# routes to exit 1 (missing marketplace, apm pack failure, genuine drift), and an +# exit-code-only assertion would pass on any of them. +check_all_fails_with() { + local desc="$1" marketplace_json="$2" expected="$3" dir out rc=0 + dir="$(make_repo_fixture "$marketplace_json")"; track "$dir" + out="$(run_all "$dir" --check --all 2>&1)" || rc=$? + if [[ "$rc" -eq 0 ]]; then + fail "$desc: --check --all exited 0 having checked nothing" + else + case "$out" in + *"$expected"*) pass "$desc: rejected with the expected message" ;; + *) fail "$desc: exited non-zero but not for the expected reason: $out" ;; + esac + fi +} +check_all_fails_with "an empty plugins array" \ + '{"plugins":[]}' \ + "declares no local (string-source) plugin entries" +check_all_fails_with "a marketplace with no plugins key at all" \ + '{"name":"x"}' \ + "declares no local (string-source) plugin entries" +check_all_fails_with "unparseable JSON" \ + '{ not json' \ + "is not valid JSON" +check_all_fails_with "a plugins field that is not an array" \ + '{"plugins":{"a":1}}' \ + "expected an array of plugin entries" +check_all_fails_with "an entry with no source field" \ + '{"plugins":[{"name":"orphan"}]}' \ + "entries with no \`source\` field" + echo "" echo "Results: $PASS passed, $FAIL failed" [[ $FAIL -eq 0 ]]