feat(scripts): tag-to-SHA pinning tools + fleet baseline (#153)#175
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 13 minutes and 58 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds tooling and baseline artifacts to support fleet-wide SHA pinning of third-party GitHub Actions, as part of the CI-060 rollout tied to #153.
Changes:
- Extend
scripts/update-pinned-actions.shwith a--pin-tagsmode to convert third-party@vNrefs to 40-character commit SHAs plus a readable tag comment. - Add
scripts/fleet-audit-sha-pins.shto audit org repos for third-party non-SHA action refs and emit a per-repo CSV. - Add Bats tests, a synthetic workflow fixture, a baseline audit CSV, and changelog entries.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/update-pinned-actions.sh |
Adds --pin-tags mode, allowlist handling, and tag to SHA resolution and rewrite logic. |
scripts/fleet-audit-sha-pins.sh |
New audit script that enumerates workflow files via gh api and counts non-SHA third-party uses: refs. |
tests/update-pinned-actions.bats |
Adds new --pin-tags test coverage and a gh stub for tag to SHA resolution. |
tests/fleet-audit-sha-pins.bats |
New tests for the audit script, including CSV output and report-only semantics. |
tests/fixtures/workflows/tag-pinned.yml |
Adds a synthetic workflow fixture covering tag, SHA, first-party, and branch ref styles. |
docs/audits/2026-05-25-sha-pin-sweep-baseline.csv |
Adds baseline audit output for the ByronWilliamsCPA and williaby orgs. |
CHANGELOG.md |
Documents the new scripts and the fleet sweep work. |
|
|
||
| local CHANGE_LOG | ||
| CHANGE_LOG=$(mktemp) | ||
| trap 'rm -f "$CHANGE_LOG"' RETURN |
Comment on lines
+221
to
+229
| while IFS='|' read -r full_action current_tag new_sha latest_tag; do | ||
| local _pat _rep | ||
| _pat="${full_action//\//\\/}@${current_tag}" | ||
| _rep="${full_action//\//\\/}@${new_sha} # ${latest_tag}" | ||
| while IFS= read -r wf_file; do | ||
| local tmp_wf | ||
| tmp_wf=$(mktemp) | ||
| sed -E "s|${_pat}([[:space:]]*)$|${_rep}|g" "$wf_file" > "$tmp_wf" | ||
| mv "$tmp_wf" "$wf_file" |
Comment on lines
49
to
+60
| while [[ $# -gt 0 ]]; do | ||
| case "$1" in | ||
| --apply) APPLY=true; shift ;; | ||
| --pin-tags) PIN_TAGS=true; shift ;; | ||
| --owner-allowlist) | ||
| if [[ $# -lt 2 || -z "${2:-}" ]]; then | ||
| echo -e "${RED}ERROR: --owner-allowlist requires a comma-separated list${NC}" | ||
| usage 1 | ||
| fi | ||
| OWNER_ALLOWLIST="$2" | ||
| shift 2 | ||
| ;; |
Comment on lines
+38
to
+42
| # Matches uses: OWNER/REPO@REF where REF is a tag (v1.2.3) or branch name. | ||
| # Handles both top-level "uses:" and YAML step "- uses:" forms. | ||
| # A 40-character hex string (SHA) does NOT match this pattern. | ||
| # The pattern excludes refs that are exactly 40 hex chars (full SHA pins). | ||
| TAG_OR_BRANCH_RE='^[[:space:]]*(- )?uses:[[:space:]]+([a-zA-Z0-9_.-]+)/[a-zA-Z0-9_.-]+(/[a-zA-Z0-9_./-]*)?@([^[:space:]#]+)' |
Comment on lines
169
to
+170
| - Pin `slsa-framework/slsa-github-generator` to full commit SHA in `python-slsa.yml` (SonarCloud S7637) | ||
| - Swept consumer repos to pin third-party GitHub Actions by commit SHA. Defends against retroactive tag-pointer manipulation (CVE-2025-30066 class). Per-repo PRs filed; CI-060 promoted to `severity: important` in the standards manifest after sweep completes. |
Collaborator
Author
PR ReviewStatus: 0 Critical, 2 Important, 5 Suggested findings. CI all green. SonarCloud quality gate passed. Branch is Heads-up: CodeRabbit's "Review completed" check reports SUCCESS, but its actual review comment was a rate-limit notice ("more reviews available in 24 minutes"). The check badge is misleading; retrigger CodeRabbit after the rate limit clears if its review is on your merge gate. Important (should fix)
Suggested
ComplianceCHANGELOG.md updated (Added + Changed sections). PR title conventional. Em-dash scan: clean. Branch behind main; rebase before merge. GitHub Copilot was not auto-requested on this PR; verify the 🤖 Generated with Claude Code |
Four new Bats test cases assert the contract for --pin-tags: dry-run reporting, apply rewrites tag refs to SHA with trailing comment, first-party org refs left untouched, and branch refs reported without conversion. All four fail with "Unknown option: --pin-tags" until the implementation lands. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…to SHA Resolves the consumer side of issue #153 (CI-060). The --pin-tags mode finds tag-pinned third-party Actions, resolves each to the latest commit SHA in the same major, and rewrites the workflow file with a trailing human-readable tag comment. First-party org refs (ByronWilliamsCPA, williaby) are skipped via --owner-allowlist. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds tests/fleet-audit-sha-pins.bats with 2 tests that exercise the planned fleet-audit-sha-pins.sh script. Both tests fail because the script does not exist yet; Task 5 will implement it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Read-only audit script that enumerates third-party tag/branch refs across both orgs and emits a CSV work queue for the issue #153 sweep. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
41 repos enumerated; 206 total violations found. Top offenders: ledgerbase (76), PromptCraft (60), zen-mcp-server (28). 29 repos are fully clean; 3 repos have >20 violations (heavy lift for Task 8). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
williaby
force-pushed
the
claude/issue-153-sha-pin-sweep
branch
from
bf3adc5 to
fe9013e
Compare
|
Collaborator
Author
|
Follow-up PR #176 addresses all 7 findings from the review above: silent failures and regex injection in the audit script, regex/sed alignment in |
4 tasks
williaby
added a commit
that referenced
this pull request
May 26, 2026
Three robustness fixes surfaced by the PR #175 follow-up review: - API errors (rate limit, 5xx, auth loss) used to be swallowed by `2>/dev/null || true` and silently rendered as `repo,0`, which is indistinguishable from a clean repo. A new `gh_api_safe` helper classifies each call as ok / missing / error: HTTP 404 keeps the legitimate-zero semantics, every other failure now emits a `repo,error` sentinel row plus a stderr WARN line. - `gh repo list --limit 200` is raised to 1000 and now warns to stderr when the result saturates the limit, signalling possible truncation instead of silently dropping repos past the cap. - `is_skipped_owner` replaces the regex-injected `if [[ "$owner" =~ ^(${SKIP_OWNERS//,/|})$ ]]` with a literal comma-delimited `case` match, so operator-supplied `--skip-owners` values containing regex metacharacters cannot produce unintended matches. Tests cover both new behaviors: an HTTP 429 stub produces the `error` sentinel; an HTTP 404 stub still produces `0`. Co-Authored-By: Claude <noreply@anthropic.com>
williaby
added a commit
that referenced
this pull request
May 26, 2026
…itution Three fixes to `scripts/update-pinned-actions.sh --pin-tags` driven by the PR #175 follow-up review: - The `extract_tag_pins` and `extract_branch_pins` regexes anchored the ref with `[[:space:]]*$`, so refs annotated with an inline `# comment` (e.g. `actions/checkout@v4 # stable`) were never matched by the converter even though the audit script counts them as violations. The regex now accepts optional trailing `[[:space:]]+#.*`; the sed pipeline strips that comment before splitting on `@`, and the apply step's sed pattern likewise spans to end-of-line so the new ` # <tag>` comment replaces any pre-existing inline comment cleanly. - `latest_tag` was interpolated into the sed replacement string without escaping. A `safe_tag` shell expansion now escapes the three sed-replacement metacharacters (`\`, `&`, `|`, ordered to avoid re-escaping) before the value is spliced in. - `pin_tags_main` declared `local CHANGE_LOG` and registered a function-scoped `RETURN` trap whose behavior relied on the local variable still being in scope at trap-fire time. The trap is now hoisted to script scope, registered before the mode dispatch, and uses `${var:-}` so both modes share the same cleanup path. The legacy `--apply` branch retains its own `mktemp` assignments under the unified trap. - Both extractor pipelines are wrapped in `{ ...; } || true` so a repository with zero tag-pinned or zero branch-pinned refs no longer trips `set -euo pipefail` on the trailing `grep`'s exit code 1. This was a latent bug surfaced by the new inline-comment test fixture (which has no branch refs). New tests cover refs with inline comments, custom `--owner-allowlist`, and annotated-tag two-step dereferencing in `--pin-tags` mode. Co-Authored-By: Claude <noreply@anthropic.com>
williaby
added a commit
that referenced
this pull request
May 26, 2026
Document the SHA-pin tooling robustness fixes under `[Unreleased]` Changed: audit script error sentinel and pagination, converter regex alignment with the auditor, sed metacharacter escaping, trap scope hoist, and the five new bats test cases that exercise them. Co-Authored-By: Claude <noreply@anthropic.com>
williaby
added a commit
that referenced
this pull request
May 26, 2026
- Move the PR #175 follow-up entries from [Unreleased] ### Changed to a new [Unreleased] ### Fixed subsection per Keep-a-Changelog convention; the silent-failure, regex-injection, and converter/auditor regex divergence fixes are defect repairs, not behavior changes - Keep the gh-repo-list --limit raise (200 -> 1000) in ### Changed since it widens the audit's coverage rather than fixing a defect - Augment the entries with the additional hardening landed in this PR: exact "(HTTP 404)" discriminator, per-file 404 WARN, base64 decode rc propagation, escape_sed_pat for current_tag, awk literal-match for --owner-allowlist, grep rc=2 distinction, single-call resolve_tag_sha, BSD-portable EXIT trap - Update the test count from "five new test cases" to "eight new test cases" and document the three regression-vector tests added in the prior commit Addresses Important finding I-12 from the /pr-review of #176. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
williaby
added a commit
that referenced
this pull request
May 26, 2026
* fix(fleet-audit): distinguish API failures from legitimate zeros Three robustness fixes surfaced by the PR #175 follow-up review: - API errors (rate limit, 5xx, auth loss) used to be swallowed by `2>/dev/null || true` and silently rendered as `repo,0`, which is indistinguishable from a clean repo. A new `gh_api_safe` helper classifies each call as ok / missing / error: HTTP 404 keeps the legitimate-zero semantics, every other failure now emits a `repo,error` sentinel row plus a stderr WARN line. - `gh repo list --limit 200` is raised to 1000 and now warns to stderr when the result saturates the limit, signalling possible truncation instead of silently dropping repos past the cap. - `is_skipped_owner` replaces the regex-injected `if [[ "$owner" =~ ^(${SKIP_OWNERS//,/|})$ ]]` with a literal comma-delimited `case` match, so operator-supplied `--skip-owners` values containing regex metacharacters cannot produce unintended matches. Tests cover both new behaviors: an HTTP 429 stub produces the `error` sentinel; an HTTP 404 stub still produces `0`. Co-Authored-By: Claude <noreply@anthropic.com> * fix(pin-tags): convert refs with inline comments and harden sed substitution Three fixes to `scripts/update-pinned-actions.sh --pin-tags` driven by the PR #175 follow-up review: - The `extract_tag_pins` and `extract_branch_pins` regexes anchored the ref with `[[:space:]]*$`, so refs annotated with an inline `# comment` (e.g. `actions/checkout@v4 # stable`) were never matched by the converter even though the audit script counts them as violations. The regex now accepts optional trailing `[[:space:]]+#.*`; the sed pipeline strips that comment before splitting on `@`, and the apply step's sed pattern likewise spans to end-of-line so the new ` # <tag>` comment replaces any pre-existing inline comment cleanly. - `latest_tag` was interpolated into the sed replacement string without escaping. A `safe_tag` shell expansion now escapes the three sed-replacement metacharacters (`\`, `&`, `|`, ordered to avoid re-escaping) before the value is spliced in. - `pin_tags_main` declared `local CHANGE_LOG` and registered a function-scoped `RETURN` trap whose behavior relied on the local variable still being in scope at trap-fire time. The trap is now hoisted to script scope, registered before the mode dispatch, and uses `${var:-}` so both modes share the same cleanup path. The legacy `--apply` branch retains its own `mktemp` assignments under the unified trap. - Both extractor pipelines are wrapped in `{ ...; } || true` so a repository with zero tag-pinned or zero branch-pinned refs no longer trips `set -euo pipefail` on the trailing `grep`'s exit code 1. This was a latent bug surfaced by the new inline-comment test fixture (which has no branch refs). New tests cover refs with inline comments, custom `--owner-allowlist`, and annotated-tag two-step dereferencing in `--pin-tags` mode. Co-Authored-By: Claude <noreply@anthropic.com> * docs(changelog): record PR #175 follow-up fixes Document the SHA-pin tooling robustness fixes under `[Unreleased]` Changed: audit script error sentinel and pagination, converter regex alignment with the auditor, sed metacharacter escaping, trap scope hoist, and the five new bats test cases that exercise them. Co-Authored-By: Claude <noreply@anthropic.com> * fix(fleet-audit): tighten 404 detection and surface decode failures - Require exact "(HTTP 404)" parenthesized form in gh_api_safe so 5xx responses containing the substring "Not Found" cannot be misclassified as missing resources - Emit stderr WARN when a per-file 404 occurs mid-iteration (directory listing succeeded but file fetch returned 404, indicating a race or token-scope mismatch); the file is still skipped but no longer silent - Replace silent "|| true" on base64 decode with explicit rc capture; decode failure now flips api_error=true so the repo row becomes "error" instead of a misleading zero-violations count Addresses Important findings I-2, I-7, I-8 from the /pr-review of #176. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(pin-tags): harden regex, error handling, and trap portability - Add escape_sed_pat helper and apply it to current_tag and full_action before splicing into _pat. A semver "." in current_tag no longer matches arbitrary characters; tags containing the sed delimiter "|" no longer terminate the pattern field - Replace regex-based --owner-allowlist filter in extract_tag_pins and extract_branch_pins with awk literal-string lookup. --owner-allowlist '.*' would have classified every action as first-party under the old awk ERE; the new BEGIN-block builds a hash table of literal owner names - Replace "{ pipeline; } || true" wrappers with explicit grep rc capture. rc=1 (zero matches) returns 0 cleanly; rc>=2 (unreadable directory, permission denied) surfaces a stderr WARN and returns 1 instead of silently emitting an empty result - Replace two-call resolve_tag_sha pattern with a single composite-jq fetch. A force-push or rate-limit hit between two separate fetches of the same ref no longer returns disagreeing type/sha values - Use ${VAR:+"$VAR"} in the script-scope EXIT trap so "rm -f" never receives an empty-string argument; harmless on GNU coreutils but warns on BSD rm (macOS) Addresses Important findings I-1, I-3, I-9, I-10, I-11 from the /pr-review of #176. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test: cover regression vectors for sha-pin hardening Adds three regression-vector tests guarding the production fixes in this PR against future silent reversion: - tests/update-pinned-actions.bats: "preserves & and | literal in tag-name comment" sets gh release stub tagName to v4.5.0+amp&pipe|x; if safe_tag escaping is removed sed would expand "&" to the matched ref or treat "|" as a field terminator - tests/fleet-audit-sha-pins.bats: "--skip-owners with regex metacharacters does not silently skip violations" passes --skip-owners '.*'. Under the old regex-based skip check this would have zeroed every violation; the new literal case-match must still count repo-dirty as 2 violations - tests/fleet-audit-sha-pins.bats: "emits 'error' sentinel when one file fetches OK and another fails" exercises the mixed-success inner-loop path the existing 429 test does not reach. Stub returns a successful directory listing with two files, success on file A (with a violation), 429 on file B; the api_error propagation must flip the repo row to "error" not a misleading "1" Addresses Important findings I-4, I-5, I-6 from the /pr-review of #176. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(changelog): split bug fixes from changes; record new hardening - Move the PR #175 follow-up entries from [Unreleased] ### Changed to a new [Unreleased] ### Fixed subsection per Keep-a-Changelog convention; the silent-failure, regex-injection, and converter/auditor regex divergence fixes are defect repairs, not behavior changes - Keep the gh-repo-list --limit raise (200 -> 1000) in ### Changed since it widens the audit's coverage rather than fixing a defect - Augment the entries with the additional hardening landed in this PR: exact "(HTTP 404)" discriminator, per-file 404 WARN, base64 decode rc propagation, escape_sed_pat for current_tag, awk literal-match for --owner-allowlist, grep rc=2 distinction, single-call resolve_tag_sha, BSD-portable EXIT trap - Update the test count from "five new test cases" to "eight new test cases" and document the three regression-vector tests added in the prior commit Addresses Important finding I-12 from the /pr-review of #176. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(pin-tags): replace broken escape_sed_pat with sed implementation The Bash parameter-expansion form `${s//\{/\\{}` has a parser gotcha: the replacement field ends at the FIRST unescaped `}`, so what looked like `replacement=\\{` followed by a closing `}` actually parsed as `replacement=\\{`, expansion-close, then a LITERAL `}` concatenated to the result. Same bug on the `}` line. Every invocation appended two literal `}` characters to the output, producing patterns like `actions/checkout}}@v4}}` that sed could not match against the workflow file. Tests 40, 43, 45 (existing) and 46 (newly added) all failed because no substitution occurred yet rc paths still returned non-zero. Replace the broken parameter-expansion form with a sed-based escape that uses single-quoted bracket-expression metacharacter handling. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the tooling side of #153 (CVE-2025-30066 defense via SHA pinning).
What's in this PR
--pin-tagsmode forscripts/update-pinned-actions.sh: converts third-party@vNtag refs to 40-character commit SHAs with a trailing readable comment.scripts/fleet-audit-sha-pins.sh: read-only audit of third-party tag/branch refs across ByronWilliamsCPA and williaby orgs. Emits CSV.docs/audits/2026-05-25-sha-pin-sweep-baseline.csv: baseline produced by the audit. 12 dirty repos, 206 violations. 11 sweep PRs filed (see below).--pin-tags, 2 for the audit script). Full suite: 44 passing.What's NOT in this PR
ByronWilliamsCPA/.claudeafter the sweep PRs all merge and the after-audit confirms zero violations).Consumer-repo sweep PRs (filed, awaiting review)
ByronWilliamsCPA/cookiecutter-template-samplewas the only false positive (Jinja2{{cookiecutter}}/...@mainplaceholders that the audit regex matched but aren't real action refs).Known follow-ups
yourusername/...@main) as violations. Consider an.audit-ignoremechanism.google/clusterfuzzlite/.../@v1) -- those were resolved manually during the sweep.@v6) that have no patch release -- manual fallback used.williaby/*repos withgitPrivateKeyerrors. Once SHAs are pinned, they need a working maintenance bot. Separate issue should be filed if not already.Test plan
bats tests/update-pinned-actions.bats(42/42)bats tests/fleet-audit-sha-pins.bats(2/2)Tracking: #153