-
Notifications
You must be signed in to change notification settings - Fork 0
fix(security): adversarial review findings — hydrator RCE + name collision + CI dispatch idempotency #43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(security): adversarial review findings — hydrator RCE + name collision + CI dispatch idempotency #43
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,122 +2,163 @@ | |
| # hydrate-pointers.sh — Fetch the canonical body content for every pointer-style | ||
| # agent in plugins/, replacing the stub body while preserving the pointer frontmatter. | ||
| # | ||
| # A "pointer agent" is any plugins/<plug>/agents/<name>.md whose frontmatter | ||
| # declares `prompt_source:` and/or `prompt_url:` and `owner_repo:`. The body | ||
| # below the frontmatter is replaced with the content fetched from prompt_url. | ||
| # Anything between the frontmatter closing `---` and the body's first markdown | ||
| # heading (`#`) — typically the warning comment — is preserved as a banner. | ||
| # SECURITY HARDENING (adversarial review findings #2 + #5): | ||
| # 1. Allowlist of source hosts (prevents pointing prompt_url at attacker-controlled origins). | ||
| # 2. prompt_sha required on every pointer for content-integrity pinning. Fetched content | ||
| # hash MUST match the pinned SHA-256 before being written. | ||
| # 3. Fetched content is passed to Python via stdin (NOT shell heredoc), so embedded | ||
| # """ or python source in the fetched body cannot escape into the script context. | ||
| # | ||
| # A "pointer agent" is any plugins/<plug>/agents/<name>.md whose frontmatter declares | ||
| # `prompt_url:`, `owner_repo:`, and `prompt_sha:` (sha256 hex). | ||
| # | ||
| # Exit codes: | ||
| # 0 — pointers were already current (nothing fetched-and-changed) | ||
| # 1 — one or more pointers were refreshed; review `git diff` and commit | ||
| # 2 — fetch failure (network, 404, etc.) | ||
| # | ||
| # Designed to be safe to re-run; can be wired into CI as a scheduled freshness check. | ||
| # 1 — pointers were refreshed; review `git diff` and commit | ||
| # 2 — fetch failure or SHA mismatch | ||
| # 3 — disallowed source host | ||
| # 4 — pointer missing required prompt_sha | ||
|
|
||
| set -euo pipefail | ||
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" | ||
| REPO_ROOT="$(git -C "$SCRIPT_DIR" rev-parse --show-toplevel)" | ||
| cd "$REPO_ROOT" | ||
|
|
||
| # Allowlist: pointer-source hosts we trust. | ||
| ALLOWED_HOSTS=( | ||
| "raw.githubusercontent.com" | ||
| ) | ||
|
|
||
| red() { printf '\033[0;31m%s\033[0m\n' "$1"; } | ||
| yellow() { printf '\033[0;33m%s\033[0m\n' "$1"; } | ||
| green() { printf '\033[0;32m%s\033[0m\n' "$1"; } | ||
| dim() { printf '\033[0;90m%s\033[0m\n' "$1"; } | ||
|
|
||
| host_allowed() { | ||
| local host="$1" | ||
| for ah in "${ALLOWED_HOSTS[@]}"; do | ||
| [ "$host" = "$ah" ] && return 0 | ||
| done | ||
| return 1 | ||
| } | ||
|
|
||
| CHANGED=0 | ||
| FETCH_FAIL=0 | ||
| DISALLOWED=0 | ||
| NO_SHA=0 | ||
|
|
||
| # Discover every pointer agent (frontmatter has both owner_repo and prompt_url). | ||
| mapfile -t POINTERS < <(grep -lrE '^prompt_url:' plugins/*/agents/*.md 2>/dev/null || true) | ||
|
|
||
| if [ "${#POINTERS[@]}" -eq 0 ]; then | ||
| dim "No pointer agents found." | ||
| exit 0 | ||
| fi | ||
|
|
||
| echo "=== hydrate-pointers ===" | ||
| echo "=== hydrate-pointers (hardened) ===" | ||
| echo "Found ${#POINTERS[@]} pointer agent(s)." | ||
| echo "Allowed hosts: ${ALLOWED_HOSTS[*]}" | ||
| echo "" | ||
|
|
||
| for f in "${POINTERS[@]}"; do | ||
| name=$(basename "$f" .md) | ||
|
|
||
| # Extract prompt_url from frontmatter | ||
| prompt_url=$(awk '/^---$/{c++; next} c==1 && /^prompt_url:/{sub(/^prompt_url:[[:space:]]*/, ""); print; exit}' "$f") | ||
| owner_repo=$(awk '/^---$/{c++; next} c==1 && /^owner_repo:/{sub(/^owner_repo:[[:space:]]*/, ""); print; exit}' "$f") | ||
| prompt_sha=$(awk '/^---$/{c++; next} c==1 && /^prompt_sha:/{sub(/^prompt_sha:[[:space:]]*/, ""); print; exit}' "$f") | ||
|
|
||
| if [ -z "$prompt_url" ]; then | ||
| yellow " SKIP $f: missing prompt_url" | ||
| yellow " SKIP $name: missing prompt_url" | ||
| continue | ||
| fi | ||
|
|
||
| # Require prompt_sha (content-integrity pin). | ||
| if [ -z "$prompt_sha" ]; then | ||
| red " REJECT $name: missing prompt_sha (content-integrity pin required)" | ||
| red " Add to frontmatter: prompt_sha: <sha256 hex of expected content>" | ||
| NO_SHA=$((NO_SHA + 1)) | ||
| continue | ||
| fi | ||
|
|
||
| # Validate host against allowlist. | ||
| host=$(printf '%s\n' "$prompt_url" | awk -F/ '{print $3}') | ||
| if ! host_allowed "$host"; then | ||
| red " REJECT $name: disallowed host '$host'" | ||
| red " Allowed: ${ALLOWED_HOSTS[*]}" | ||
| DISALLOWED=$((DISALLOWED + 1)) | ||
| continue | ||
| fi | ||
|
|
||
| echo " hydrating $name from $owner_repo ..." | ||
| echo " hydrating $name from $owner_repo (host=$host, sha=${prompt_sha:0:12}…)" | ||
|
|
||
| # Fetch the canonical body (5s timeout, follow redirects, fail on HTTP errors) | ||
| fetched=$(curl -fsSL --max-time 5 "$prompt_url" 2>/dev/null || echo "__FETCH_FAILED__") | ||
| if [ "$fetched" = "__FETCH_FAILED__" ]; then | ||
| # Fetch into a temp file (NOT shell variable) to prevent injection. | ||
| tmp=$(mktemp) | ||
| if ! curl -fsSL --max-time 5 --max-filesize 1048576 "$prompt_url" -o "$tmp" 2>/dev/null; then | ||
| red " FETCH FAILED: $prompt_url" | ||
| rm -f "$tmp" | ||
| FETCH_FAIL=$((FETCH_FAIL + 1)) | ||
| continue | ||
| fi | ||
|
|
||
| # The fetched content typically includes its own frontmatter. Strip it | ||
| # (between first two `---` lines) so we only inject the body. | ||
| fetched_body=$(printf '%s' "$fetched" | awk ' | ||
| BEGIN { in_fm=0; saw_fm_end=0 } | ||
| /^---$/ { | ||
| if (!in_fm && NR==1) { in_fm=1; next } | ||
| else if (in_fm && !saw_fm_end) { saw_fm_end=1; next } | ||
| } | ||
| { if (!in_fm || saw_fm_end) print } | ||
| ') | ||
|
|
||
| # Build the new file: keep frontmatter as-is, keep the warning comment block, | ||
| # replace everything below the first `# ` heading with the canonical body. | ||
| /usr/bin/python3 - <<PY | ||
| import pathlib, re, sys | ||
| p = pathlib.Path("$f") | ||
| text = p.read_text() | ||
| # Split frontmatter | ||
| m = re.match(r"^---\n(.*?)\n---\n(.*)$", text, re.DOTALL) | ||
| if not m: | ||
| # Verify SHA-256. | ||
| actual_sha=$(shasum -a 256 "$tmp" | awk '{print $1}') | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hash verification is hard-coded to Useful? React with 👍 / 👎. |
||
| if [ "$actual_sha" != "$prompt_sha" ]; then | ||
| red " SHA MISMATCH: expected $prompt_sha, got $actual_sha" | ||
| red " If upstream changed intentionally, update prompt_sha in frontmatter." | ||
| rm -f "$tmp" | ||
| FETCH_FAIL=$((FETCH_FAIL + 1)) | ||
| continue | ||
| fi | ||
|
|
||
| # Hand off to Python via env var (NOT heredoc interpolation). | ||
| POINTER_PATH="$f" FETCHED_FILE="$tmp" python3 - <<'PY' | ||
| import os, pathlib, re, sys | ||
| pointer_path = pathlib.Path(os.environ["POINTER_PATH"]) | ||
| fetched = pathlib.Path(os.environ["FETCHED_FILE"]).read_text(encoding="utf-8") | ||
|
|
||
| # Strip any frontmatter from fetched content (we keep the local pointer's frontmatter). | ||
| m = re.match(r"^---\n.*?\n---\n(.*)$", fetched, re.DOTALL) | ||
| fetched_body = (m.group(1) if m else fetched).strip() | ||
|
|
||
| text = pointer_path.read_text(encoding="utf-8") | ||
| fm_m = re.match(r"^---\n(.*?)\n---\n(.*)$", text, re.DOTALL) | ||
| if not fm_m: | ||
| sys.exit("frontmatter not parseable") | ||
| frontmatter = m.group(1) | ||
| rest = m.group(2) | ||
| # Find HTML comment block (the warning) — keep it | ||
| comment_match = re.match(r"^\s*(<!--.*?-->)", rest, re.DOTALL) | ||
| comment = comment_match.group(1).rstrip() if comment_match else "" | ||
| # Build new content (normalize whitespace to prevent diff drift on idempotent re-runs) | ||
| new_body = """$fetched_body""".strip() | ||
| frontmatter = fm_m.group(1) | ||
| rest = fm_m.group(2) | ||
|
|
||
| # Preserve the warning HTML comment block (no trailing whitespace capture). | ||
| comment_m = re.match(r"^\s*(<!--.*?-->)", rest, re.DOTALL) | ||
| comment = comment_m.group(1).rstrip() if comment_m else "" | ||
| sep = "\n\n" if comment else "" | ||
| new_text = f"---\n{frontmatter}\n---\n\n{comment}{sep}{new_body}\n" | ||
| # Only write if changed | ||
| new_text = f"---\n{frontmatter}\n---\n\n{comment}{sep}{fetched_body}\n" | ||
|
|
||
| if new_text != text: | ||
| p.write_text(new_text) | ||
| print(f" refreshed: {p}") | ||
| pointer_path.write_text(new_text, encoding="utf-8") | ||
| print(f" refreshed: {pointer_path}") | ||
| else: | ||
| print(f" unchanged: {p}") | ||
| print(f" unchanged: {pointer_path}") | ||
| PY | ||
|
|
||
| # Track whether the file is now modified relative to git index | ||
| rm -f "$tmp" | ||
|
|
||
| if ! git diff --quiet -- "$f"; then | ||
| CHANGED=$((CHANGED + 1)) | ||
| fi | ||
| done | ||
|
|
||
| echo "" | ||
| echo "=== Summary ===" | ||
| if [ "$FETCH_FAIL" -gt 0 ]; then | ||
| red "Fetch failures: $FETCH_FAIL" | ||
| fi | ||
| [ "$DISALLOWED" -gt 0 ] && red "Disallowed hosts: $DISALLOWED" | ||
| [ "$NO_SHA" -gt 0 ] && red "Missing prompt_sha: $NO_SHA" | ||
| [ "$FETCH_FAIL" -gt 0 ] && red "Fetch/SHA failures: $FETCH_FAIL" | ||
| if [ "$CHANGED" -gt 0 ]; then | ||
| yellow "Hydrated: $CHANGED file(s). Review with 'git diff' and commit." | ||
| elif [ "$FETCH_FAIL" -eq 0 ]; then | ||
| elif [ "$FETCH_FAIL" -eq 0 ] && [ "$DISALLOWED" -eq 0 ] && [ "$NO_SHA" -eq 0 ]; then | ||
| green "All pointer agents are current." | ||
| fi | ||
|
|
||
| [ "$NO_SHA" -gt 0 ] && exit 4 | ||
| [ "$DISALLOWED" -gt 0 ] && exit 3 | ||
|
Comment on lines
+160
to
+161
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The script now exits with Useful? React with 👍 / 👎. |
||
| [ "$FETCH_FAIL" -gt 0 ] && exit 2 | ||
| [ "$CHANGED" -gt 0 ] && exit 1 | ||
| exit 0 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -256,6 +256,30 @@ for name, profile in data.get('profiles', {}).items(): | |
| fi | ||
| echo "" | ||
|
|
||
| # --- 8b. Cross-kind name collision check (adversarial review #1) --- | ||
| # A canonical name must be unique across all kind-subdirs. Without this check, | ||
| # `canonical/agents/foo.md` and `canonical/skills/foo.md` would collide silently | ||
| # (dispatch.sh's first-match-wins lookup would project the agent and orphan the skill). | ||
| echo "Checking canonical name uniqueness across kind-subdirs..." | ||
| collisions=$(python3 -c " | ||
| import pathlib | ||
| from collections import defaultdict | ||
| seen = defaultdict(list) | ||
| for p in pathlib.Path('$REPO_DIR/canonical').rglob('*.md'): | ||
| if p.name == 'README.md': continue | ||
| seen[p.stem].append(str(p.relative_to('$REPO_DIR'))) | ||
| for name, paths in seen.items(): | ||
| if len(paths) > 1: | ||
| print(name + ': ' + ' '.join(paths)) | ||
| ") | ||
| if [ -n "$collisions" ]; then | ||
| echo "$collisions" | while IFS= read -r line; do | ||
| red " ERROR: canonical name collision: $line" | ||
| ERRORS=$((ERRORS + 1)) | ||
|
Comment on lines
+276
to
+278
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The new cross-kind collision block prints errors but does not actually fail linting, because Useful? React with 👍 / 👎. |
||
| done | ||
| fi | ||
| echo "" | ||
|
|
||
| # --- 9. Projection-must-have-canonical (Phase E lock) --- | ||
| # Every projection file (agent/skill/command/mcp in plugins/) MUST derive | ||
| # from a canonical/<kind>/<name>.md or canonical/<name>.md source. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new idempotency gate relies on
git diff --quiet -- canonical/ plugins/, which only detects changes to tracked files and ignores newly created untracked files. Ifdispatch.sh syncmaterializes missing projection/state files (for example after adding a canonical), this check can still pass even though regeneration produced filesystem drift, so CI can miss exactly the divergence this guard is meant to catch.Useful? React with 👍 / 👎.