Przeglądaj źródła

Harden bump cache and revert workflows after review

- revert-failed-bumps: replace the time-based revert budget (anchored on
  the PR head, which a revert commit immediately replaces — never
  accumulating past 1) with a commit count: every nightly bump force-
  resets to one commit and every revert pass adds exactly one, so
  commits > MAX+1 is the budget without date math, pagination, or
  exposure to comment spoofing.
- revert-failed-bumps: filter the bump PR by head owner so a fork PR
  with a branch named bump/plugin-shas can't be selected.
- revert-failed-bumps: continue-on-error on the artifact download so a
  scan that died before uploading (infra error) doesn't fail the revert
  job — the missing-file guard downstream handles it.
- scan-plugins: add a per-ref concurrency group so concurrent scans
  don't lose one another's cache writes; key the cache on run_attempt
  so a re-run can save its own verdicts.
- scan-plugins: store the full source object in the cache and require
  source equality on lookup, so a repo/path change at the same SHA
  misses the cache instead of getting a stale verdict.
- scan-plugins / revert-failed-bumps: strip markdown control chars,
  wrap model-generated text in code spans (neutralizes auto-linked
  URLs), and redact key-shaped tokens before they reach the step
  summary, artifact, cache, or PR comment.
tobin 3 miesięcy temu
rodzic
commit
0963dd381c

+ 37 - 19
.github/workflows/revert-failed-bumps.yml

@@ -58,7 +58,12 @@ jobs:
       # run-verdicts.json (full per-entry verdicts for the PR comment). It is
       # uploaded by scan-plugins.yml for every relevant run so we can tell
       # "policy failures found" from "scan never ran" (infra error → no revert).
+      # The artifact won't exist when the scan died before the upload step
+      # (cache restore error, jq failure, timeout) — that is an infra error,
+      # not a policy failure, so the right move is to do nothing. The
+      # download must not fail the job; the next step handles the missing file.
       - name: Download scan verdicts
+        continue-on-error: true
         uses: actions/download-artifact@v4
         with:
           name: scan-verdicts
@@ -103,30 +108,39 @@ jobs:
           REPO: ${{ github.repository }}
         run: |
           set -euo pipefail
-          pr_number="$(gh pr list --repo "$REPO" --head "$BUMP_BRANCH" --base main --state open \
-            --json number -q '.[0].number // empty')"
-          if [[ -z "$pr_number" ]]; then
+          # Resolve the bump PR by head ref. `gh pr list --head <ref>` matches
+          # by ref name across forks, so reject any PR whose head repo isn't
+          # ours — a fork PR named bump/plugin-shas must never reach the
+          # contents:write paths below.
+          pr_json="$(gh api "repos/$REPO/pulls?head=${REPO%%/*}:$BUMP_BRANCH&base=main&state=open&per_page=1" \
+            --jq '.[0] // empty')"
+          if [[ -z "$pr_json" ]]; then
             echo "::warning::No open bump PR on $BUMP_BRANCH — nothing to revert."
             echo "act=false" >> "$GITHUB_OUTPUT"
             exit 0
           fi
-          # Loop bound: count this workflow's marker comments on the PR. The
-          # bump action force-resets the branch every night and edits the
-          # existing PR (comments survive), so the budget is scoped per night
-          # by only counting comments newer than the most recent commit that
-          # was NOT created by this workflow — i.e. the nightly bump commit.
-          head_sha="$(gh api "repos/$REPO/pulls/$pr_number" --jq '.head.sha')"
-          head_date="$(gh api "repos/$REPO/commits/$head_sha" --jq '.commit.author.date')"
-          passes="$(gh api "repos/$REPO/issues/$pr_number/comments" --paginate \
-            --jq "[.[] | select(.body | startswith(\"$REVERT_MARKER\")) | select(.created_at > \"$head_date\")] | length")"
-          echo "Revert passes since last bump commit ($head_sha @ $head_date): $passes"
-          if [[ "$passes" -ge "$MAX_REVERT_PASSES" ]]; then
-            echo "::error::Revert budget exhausted ($passes/$MAX_REVERT_PASSES passes since last bump). The cache or scan is likely broken — needs a human."
+          pr_number="$(jq -r '.number' <<<"$pr_json")"
+          head_repo="$(jq -r '.head.repo.full_name' <<<"$pr_json")"
+          head_sha="$(jq -r '.head.sha' <<<"$pr_json")"
+          # The list endpoint omits `commits`; the single-PR endpoint has it.
+          commit_count="$(gh api "repos/$REPO/pulls/$pr_number" --jq '.commits')"
+          if [[ "$head_repo" != "$REPO" ]]; then
+            echo "::error::Bump PR head is from $head_repo, not $REPO — refusing to act."
+            echo "act=false" >> "$GITHUB_OUTPUT"
+            exit 0
+          fi
+          # Loop bound: every nightly bump force-resets the branch to a single
+          # commit and every revert pass adds exactly one. Counting commits is
+          # therefore the per-night pass count + 1, with no date math, no
+          # pagination, and no exposure to comment spoofing.
+          if [[ "$commit_count" -gt $(( MAX_REVERT_PASSES + 1 )) ]]; then
+            echo "::error::Revert budget exhausted ($((commit_count - 1))/$MAX_REVERT_PASSES passes on this PR). The cache or scan is likely broken — needs a human."
             gh pr comment "$pr_number" --repo "$REPO" --body \
-              "$REVERT_MARKER"$'\n\n'"⚠️ Revert budget exhausted ($passes passes). The scan keeps failing after reverting — likely a cache or scan bug. Pausing automatic reverts until the next nightly bump."
+              "$REVERT_MARKER"$'\n\n'"⚠️ Revert budget exhausted ($((commit_count - 1)) passes). The scan keeps failing after reverting — likely a cache or scan bug. Pausing automatic reverts until the next nightly bump."
             echo "act=false" >> "$GITHUB_OUTPUT"
             exit 0
           fi
+          echo "Bump PR #$pr_number @ $head_sha ($commit_count commit(s))"
           {
             echo "act=true"
             echo "number=$pr_number"
@@ -249,10 +263,14 @@ jobs:
             echo
             echo "| Plugin | Violations |"
             echo "|---|---|"
-            # Pipes and newlines would break the markdown table; strip both.
+            # `violations` is model-generated text shaped by a cloned external
+            # repo. Strip markdown control characters and wrap in a code span
+            # so a prompt-injected upstream can't smuggle links/images/table
+            # breakouts into a public PR comment.
             jq -r --argjson rev "$REVERTED" \
-              '.[] | select(.name as $n | $rev | index($n))
-               | "| \(.name) | \(.violations | gsub("[|\n\r]"; " ") | .[0:200]) |"' \
+              'def neutralize: gsub("[|\n\r\\[\\]<>`]"; " ");
+               .[] | select(.name as $n | $rev | index($n))
+               | "| \(.name) | `\(.violations | neutralize | .[0:200])` |"' \
               scan-out/run-verdicts.json
             echo
             echo "These entries will be retried at their next upstream SHA. See the [scan run]($SCAN_RUN_URL) for full verdicts."

+ 52 - 16
.github/workflows/scan-plugins.yml

@@ -33,6 +33,13 @@ on:
 permissions:
   contents: read
 
+# Serialize scans per ref so concurrent runs (a re-dispatch racing the
+# original, or a manual dispatch) don't both restore the same cache, scan
+# overlapping sets, and lose one another's verdicts on save.
+concurrency:
+  group: scan-plugins-${{ github.event.pull_request.number || github.ref }}
+  cancel-in-progress: false
+
 env:
   MARKETPLACE: .claude-plugin/marketplace.json
   CACHE_DIR: ${{ github.workspace }}/.scan-cache
@@ -93,7 +100,9 @@ jobs:
         uses: actions/cache/restore@v4
         with:
           path: .scan-cache
-          key: scan-verdicts-${{ hashFiles('.github/policy/**') }}-${{ github.run_id }}
+          # run_attempt so a re-run can save its own verdicts (cache keys are
+          # immutable; without it a re-run would silently fail to save).
+          key: scan-verdicts-${{ hashFiles('.github/policy/**') }}-${{ github.run_id }}-${{ github.run_attempt }}
           restore-keys: |
             scan-verdicts-${{ hashFiles('.github/policy/**') }}-
 
@@ -145,13 +154,16 @@ jobs:
 
           changed_count="$(jq 'length' "$CACHE_DIR/changed.json")"
 
-          # Split changed entries into cached vs uncached.
+          # Split changed entries into cached vs uncached. A hit requires the
+          # *whole* source object (repo, sha, path, ref) to match the cached
+          # entry, not just name@sha — a repo migration or path change with the
+          # same SHA is different scan content and must miss the cache.
           jq -c -s \
             '.[0] as $cache
              | (.[1] | map(. + {key: (.name + "@" + (.source.sha // "")) })) as $entries
              | {
-                 to_scan:  [$entries[] | select(($cache[.key] // null) == null)],
-                 cached:   [$entries[] | select(($cache[.key] // null) != null)
+                 to_scan:  [$entries[] | select(($cache[.key].source // null) != .source)],
+                 cached:   [$entries[] | select(($cache[.key].source // null) == .source)
                             | . + {verdict: $cache[.key]}]
                }' \
             "$CACHE_DIR/verdicts.json" "$CACHE_DIR/changed.json" > "$CACHE_DIR/split.json"
@@ -228,26 +240,42 @@ jobs:
             echo '[]' > "$CACHE_DIR/scanned-raw.json"
           fi
 
+          # Defense in depth: the scan action runs Claude with Read access over
+          # a cloned external repo and ANTHROPIC_API_KEY in its process env. A
+          # successful prompt injection could coerce the model to put key
+          # material into `summary`/`violations`. The action's own step summary
+          # already carries that risk; this workflow adds an artifact and a PR
+          # comment, both public sinks. Scrub any key-shaped token here so it
+          # never reaches the cache, artifact, or comment.
+          jq -c '(.. | strings) |= gsub("sk-ant-[A-Za-z0-9_-]{8,}"; "[REDACTED]")' \
+            "$CACHE_DIR/scanned-raw.json" > "$CACHE_DIR/scanned-raw.json.tmp"
+          mv "$CACHE_DIR/scanned-raw.json.tmp" "$CACHE_DIR/scanned-raw.json"
+
           now="$(date -u +%Y-%m-%dT%H:%M:%SZ)"
 
-          # The action's `scanned` output has no SHA — join it with the change
-          # set by name to recover the SHA for the cache key.
+          # The action's `scanned` output has no SHA or source — join it with
+          # the change set by name to recover both for the cache key + the
+          # source-equality lookup guard.
           jq -c -s --arg now "$now" \
             '.[0] as $changed
              | (.[1] // []) as $scanned
-             | ($changed | map({(.name): .source.sha}) | add // {}) as $shas
-             | [$scanned[] | . + {sha: ($shas[.name] // ""), scanned_at: $now}]' \
+             | ($changed | map({(.name): .source}) | add // {}) as $srcs
+             | [$scanned[]
+                | . + {source: ($srcs[.name] // null), sha: ($srcs[.name].sha // ""), scanned_at: $now}]' \
             "$CACHE_DIR/changed.json" "$CACHE_DIR/scanned-raw.json" \
             > "$CACHE_DIR/fresh.json"
 
-          # Merge fresh verdicts into the cache, keyed by name@sha. Existing
-          # entries are preserved; new entries are added or overwrite stale
-          # ones for the same key (a re-scan after a flaky verdict).
+          # Merge fresh verdicts into the cache, keyed by name@sha. The
+          # full source object is stored so a future repo/path change with the
+          # same SHA fails the lookup guard. summary/violations are model
+          # output — truncate to bound cache size (the artifact carries the
+          # full text for the run that produced it).
           jq -c -s \
             '.[0] + ([.[1][] | select(.sha != "") | {(.name + "@" + .sha): {
+                source: .source,
                 passes: .passes,
-                summary: (.summary // ""),
-                violations: (.violations // ""),
+                summary: ((.summary // "") | .[0:300]),
+                violations: ((.violations // "") | .[0:500]),
                 scanned_at: .scanned_at
               }}] | add // {})' \
             "$CACHE_DIR/verdicts.json" "$CACHE_DIR/fresh.json" \
@@ -280,6 +308,12 @@ jobs:
             echo "total=$total"
           } >> "$GITHUB_OUTPUT"
 
+          # `summary` and `violations` are model-generated text shaped by a
+          # cloned external repo. Strip markdown control characters AND wrap
+          # in code spans before they hit a publicly-rendered sink — code
+          # spans neutralize auto-linked bare URLs that a prompt-injected
+          # upstream could smuggle in. Stripping backticks first stops a
+          # breakout from the code span.
           {
             echo "## Policy scan (with verdict cache)"
             echo
@@ -288,13 +322,15 @@ jobs:
             if [[ "$total" -gt 0 ]]; then
               echo "| Plugin | SHA | Passes | Source | Summary |"
               echo "|---|---|---|---|---|"
-              jq -r '.[] | "| \(.name) | `\(.sha[0:8])` | \(if .passes then "✅" else "❌" end) | \(.source) | \(.summary | .[0:120]) |"' \
+              jq -r 'def neutralize: gsub("[|\n\r\\[\\]<>`]"; " ");
+                .[] | "| \(.name) | `\(.sha[0:8])` | \(if .passes then "✅" else "❌" end) | \(.source) | `\(.summary | neutralize | .[0:120])` |"' \
                 "$CACHE_DIR/run-verdicts.json"
             fi
             if [[ "$fail_count" -gt 0 ]]; then
               echo
               echo "### Violations"
-              jq -r '.[] | select(.passes == false) | "- **\(.name)** — \(.violations)"' "$CACHE_DIR/run-verdicts.json"
+              jq -r 'def neutralize: gsub("[|\n\r\\[\\]<>`]"; " ");
+                .[] | select(.passes == false) | "- **\(.name)** — `\(.violations | neutralize | .[0:500])`"' "$CACHE_DIR/run-verdicts.json"
             fi
           } >> "$GITHUB_STEP_SUMMARY"
 
@@ -318,7 +354,7 @@ jobs:
         uses: actions/cache/save@v4
         with:
           path: .scan-cache
-          key: scan-verdicts-${{ hashFiles('.github/policy/**') }}-${{ github.run_id }}
+          key: scan-verdicts-${{ hashFiles('.github/policy/**') }}-${{ github.run_id }}-${{ github.run_attempt }}
 
       # Required-check gate. Fails on either fresh or cached policy failures —
       # a known-bad SHA must keep failing until it is reverted or upstream