diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index cd305f5..178a6f9 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -12,8 +12,9 @@ for ByronWilliamsCPA. It contains: - Org-level profile (`profile/README.md`) There is no Python package and no build system in this repo. The Bats test suite -under `tests/` covers `scripts/update-pinned-actions.sh` (run via `.github/workflows/shell-tests.yml`); -the other scripts in `scripts/` do not yet have automated tests. +under `tests/` covers both `scripts/update-pinned-actions.sh` and +`scripts/fleet-audit-sha-pins.sh` (all `tests/*.bats` files run via +`.github/workflows/shell-tests.yml`). ## Model Selection diff --git a/.github/workflows/shell-tests.yml b/.github/workflows/shell-tests.yml index 49bbabe..ad3cf7a 100644 --- a/.github/workflows/shell-tests.yml +++ b/.github/workflows/shell-tests.yml @@ -9,7 +9,7 @@ # # Run locally: # git submodule update --init --recursive -# ./tests/libs/bats-core/bin/bats tests/update-pinned-actions.bats +# ./tests/libs/bats-core/bin/bats tests/*.bats # ============================================================================ name: Shell Tests @@ -52,4 +52,12 @@ jobs: submodules: recursive - name: Run bats tests - run: ./tests/libs/bats-core/bin/bats tests/update-pinned-actions.bats + # Discover all .bats files under tests/ rather than naming a single + # file. Naming tests/update-pinned-actions.bats hid a class of bugs: + # tests added under tests/fleet-audit-sha-pins.bats (and any future + # *.bats sibling) were never executed by CI, so STRICT_AUDIT and + # REPO_LIMIT regressions could ship green. find + -print0/-exec keeps + # this glob-injection-safe and POSIX-clean. + run: | + find tests -maxdepth 1 -name '*.bats' -print0 \ + | xargs -0 ./tests/libs/bats-core/bin/bats diff --git a/CHANGELOG.md b/CHANGELOG.md index 202a98c..ba26f5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,20 @@ are no numbered releases. 2026) makes the SBOM scanner itself a supply-chain risk; Grype (Anchore) is unaffected. +- `scripts/fleet-audit-sha-pins.sh`: new `STRICT_AUDIT=1` environment toggle. + When set, the script exits with status 2 if any repo emitted a `repo,error` + row or if any org's repo count saturated `REPO_LIMIT`. CI gates that need + fail-closed semantics opt in via `STRICT_AUDIT=1`; the default flow remains + report-only (exit 0) so existing consumers are unaffected. Companion + change: `REPO_LIMIT` now reads from environment with a default of 1000 + (`REPO_LIMIT="${REPO_LIMIT:-1000}"`), letting tests exercise the + saturation WARN with a small stub-friendly value and giving operators an + escape hatch for fleets that grow beyond the default. +- `scripts/fleet-audit-sha-pins.sh`: `STRICT_AUDIT` now accepts the + case-insensitive truthy spellings `1`, `true`, and `yes`; previously only + the literal `"1"` was honoured and `STRICT_AUDIT=true` silently degraded + to report-only. + ### Breaking Changes - `python-security-analysis.yml`: the `safety` SCA scanner has been @@ -178,6 +192,14 @@ are no numbered releases. - `scripts/fleet-audit-sha-pins.sh`: distinguish legitimate HTTP 404 (no workflows directory) from real API failures (rate limit, 5xx, auth loss); real failures now emit a `repo,error` sentinel row and a stderr WARN line instead of silently appearing as `repo,0`. The 404 discriminator now requires the exact `(HTTP 404)` parenthesized form so an upstream 5xx response containing the substring "Not Found" cannot be misclassified as a missing resource. Replaces the regex-based `SKIP_OWNERS` check with a literal `case` match so operator-supplied values containing regex metacharacters (e.g., `.*`) cannot silently classify every owner as skipped. Adds a stderr WARN when a per-file 404 occurs mid-iteration (the directory listing succeeded but the specific file fetch returned 404, indicating a race or scope mismatch). Replaces the silent `|| true` masking on the base64 decode with explicit rc handling that propagates decode failures to the `repo,error` sentinel. Addresses the silent-failure and regex-injection findings from the PR #175 follow-up review. - `scripts/update-pinned-actions.sh`: `extract_tag_pins` and `extract_branch_pins` now accept refs that carry an inline `# comment` (e.g., `actions/checkout@v4 # stable`); the converter previously required the ref to be the last non-whitespace content on the line, leaving inline-annotated refs as lingering audit violations. The sed substitution in `--pin-tags --apply` now spans to end-of-line so any pre-existing inline comment is replaced cleanly by the new `# ` comment. Escapes sed-replacement metacharacters (`\`, `&`, `|`) in the resolved tag name AND sed-pattern metacharacters in `current_tag` and `full_action` before splicing into the substitution; a semver `.` in `current_tag` no longer matches arbitrary characters and tags containing the sed delimiter `|` no longer terminate the pattern field. Replaces the regex-based `--owner-allowlist` filter in both extractors with an awk literal-string lookup, eliminating the regex-injection vector where `--owner-allowlist '.*'` would silently classify every action as first-party. Replaces the `{ pipeline; } || true` wrappers in both extractors with explicit grep rc handling so a real failure (rc>=2: unreadable directory, permission denied) surfaces as a stderr WARN and a non-zero return instead of a silent empty result. Replaces the two-call `resolve_tag_sha` pattern with a single composite-jq fetch, removing the race window where a force-push or rate-limit hit between two fetches of the same ref could return disagreeing values. Makes the EXIT trap BSD-compatible by using `${VAR:+"$VAR"}` so `rm -f` never receives an empty-string argument on macOS. Hoists the `CHANGE_LOG` temp file and its cleanup trap to script scope so `pin_tags_main` no longer relies on a function-local `RETURN` trap; both extractor pipelines tolerate zero matches under `set -euo pipefail` via the new rc-aware path. - `tests/update-pinned-actions.bats`, `tests/fleet-audit-sha-pins.bats`: add eight new test cases covering the hardening above. The five from the original PR #175 follow-up commit (inline-comment conversion, custom `--owner-allowlist`, two-step annotated-tag dereferencing, audit emits `repo,error` on non-404 failure, audit treats 404 on the workflows directory as a legitimate zero) are joined by three regression-vector tests: sed-escape of `&` and `|` characters in tag-name comments, `--skip-owners` with regex metacharacters still counts violations, and mixed-success inner-loop (one workflow file fetches OK, a second fails, repo row flips to `error` not a partial numeric count). +- `scripts/update-pinned-actions.sh`: `resolve_tag_sha` now validates that the `gh api` composite jq response contains non-empty, non-`"null"` values for `.object.type` and `.object.sha`. The previous code accepted the literal string `"null"` (emitted by `jq -r` for missing fields) as a successful resolution, which produced patterns like `actions/checkout@null` that the caller wrote into the workflow file. The same guard now also fires after the second annotated-tag dereferencing call. Action refs that fail validation are marked SKIPPED rather than written with a malformed SHA. +- `tests/fleet-audit-sha-pins.bats`, `tests/update-pinned-actions.bats`: add nine more regression-vector tests guarding the Suggested-tier fixes. fleet-audit gains: REPO_LIMIT saturation WARN with env-overridable REPO_LIMIT, STRICT_AUDIT=1 fail-closed on saturation, STRICT_AUDIT=1 fail-closed on per-repo error, STRICT_AUDIT=1 clean-run regression guard, SKIP_OWNERS edge cases (single value, empty, double internal commas). update-pinned-actions gains: extract_branch_pins reports comment-annotated branch refs, --pin-tags --apply succeeds with only-first-party fixtures, --pin-tags --apply skips when annotated-tag dereferencing returns missing `.object.sha`. +- `scripts/fleet-audit-sha-pins.sh`: workflows-listing failure path now sets `audit_incomplete=true` before continuing, matching the per-file-fetch error path. The previous code emitted a `repo,error` sentinel but left the flag unset, so `STRICT_AUDIT=1` could exit 0 even when error rows were present. Verified by the new `STRICT_AUDIT=1 exits non-zero on workflows-listing failure` regression test. (Critical from the PR #177 self-review.) +- `.github/workflows/shell-tests.yml`: bats runner now discovers all `tests/*.bats` files via `find ... -print0 | xargs -0`. Previously it named only `tests/update-pinned-actions.bats`, so the 13 cases in `tests/fleet-audit-sha-pins.bats` (including the STRICT_AUDIT and REPO_LIMIT regression cases added in PR #176 and PR #177) never ran in CI. (Critical from the PR #177 self-review.) +- `scripts/fleet-audit-sha-pins.sh`: `gh repo list` invocation now captures stdout to a temp file rather than reading through process substitution, so the command's exit code surfaces. An auth-loss or rate-limit hit during enumeration now flags the audit as incomplete and emits a stderr WARN; previously the failure was discarded and the org silently contributed zero rows. +- `scripts/fleet-audit-sha-pins.sh`: validate `REPO_LIMIT` as a positive integer up front and exit 1 with a clear error message when it is empty, zero, negative, or non-numeric. Previously a bad value would crash mid-loop inside `[[ ... -eq $REPO_LIMIT ]]` under `set -euo pipefail`. +- `scripts/update-pinned-actions.sh`: `resolve_tag_sha` null guards now emit a stderr WARN before returning empty, so operators can distinguish a malformed GitHub API response from a missing tag. The action is still marked SKIPPED; only the diagnostic surface changes. +- `scripts/fleet-audit-sha-pins.sh`: strip trailing CR from each line before regex matching in the violation counter, so workflow files checked out on Windows or proxied through CRLF-normalizing intermediaries no longer leak `\r` into `BASH_REMATCH[4]` and inflate violation counts. ### Security diff --git a/scripts/fleet-audit-sha-pins.sh b/scripts/fleet-audit-sha-pins.sh index ba1659b..ec16296 100755 --- a/scripts/fleet-audit-sha-pins.sh +++ b/scripts/fleet-audit-sha-pins.sh @@ -13,10 +13,29 @@ # message written to stderr # Exit code: 0 even when violations or errors are present (this is a report, # not a gate). Errors are surfaced via the sentinel row plus stderr. +# Exception: STRICT_AUDIT (below) overrides this and exits 2 on any incomplete +# audit. A bad REPO_LIMIT value exits 1 immediately before any audit work. +# +# Environment overrides: +# REPO_LIMIT Max repos fetched per org via `gh repo list` (default 1000). +# Lower it for testing; raise it if your fleet exceeds 1000 +# repos. Must be a positive integer; the script exits 1 with +# an error if not. The script emits a stderr WARN when the +# result count equals REPO_LIMIT (saturation, audit may be +# truncated). +# STRICT_AUDIT When truthy (1, true, or yes; case-insensitive), exit 2 at +# the end of the run if any API failure produced a +# `repo,error` row, any org's `gh repo list` failed, or any +# org's repo count saturated REPO_LIMIT. Use this in CI +# gates that should fail closed when the audit is +# incomplete. The default (unset / "0" / "false" / "no") +# preserves report-only semantics. # # Usage: # ./scripts/fleet-audit-sha-pins.sh --org ByronWilliamsCPA [--org williaby] \ # [--skip-owners ByronWilliamsCPA,williaby] [--output audit.csv] +# STRICT_AUDIT=1 ./scripts/fleet-audit-sha-pins.sh --org ByronWilliamsCPA +# REPO_LIMIT=3 ./scripts/fleet-audit-sha-pins.sh --org ByronWilliamsCPA # ============================================================================= set -euo pipefail @@ -96,12 +115,39 @@ is_skipped_owner() { esac } -REPO_LIMIT=1000 +# REPO_LIMIT defaults to 1000 but may be overridden via environment for tests +# and for fleets that grow beyond the default. Validate as a positive integer +# up front so the loop below does not blow up inside `[[ ... -eq $REPO_LIMIT ]]` +# under `set -euo pipefail` when the operator passes a typo (`REPO_LIMIT=abc`). +REPO_LIMIT="${REPO_LIMIT:-1000}" +if [[ ! "$REPO_LIMIT" =~ ^[1-9][0-9]*$ ]]; then + echo "ERROR: REPO_LIMIT must be a positive integer (got: '$REPO_LIMIT')" >&2 + exit 1 +fi + +# Tracks whether the audit was incomplete due to API errors or saturation; +# STRICT_AUDIT consults this at exit to decide whether to fail closed. +audit_incomplete=false for org in "${ORGS[@]}"; do - mapfile -t repos < <(gh repo list "$org" --limit "$REPO_LIMIT" --json name --jq '.[].name') + # Capture `gh repo list` rc via a tmpfile rather than process substitution. + # `mapfile -t repos < <(gh repo list ...)` would discard a non-zero rc + # (auth loss, rate-limit mid-run, transient API hiccup), leaving `repos` + # empty and the org silently contributing zero rows without flagging + # the audit as incomplete. That defeats STRICT_AUDIT. + repo_list_tmp="$(mktemp)" + if ! gh repo list "$org" --limit "$REPO_LIMIT" --json name --jq '.[].name' \ + > "$repo_list_tmp" 2>/dev/null; then + echo "WARN: $org: gh repo list failed; org skipped, audit incomplete" >&2 + audit_incomplete=true + rm -f "$repo_list_tmp" + continue + fi + mapfile -t repos < "$repo_list_tmp" + rm -f "$repo_list_tmp" if [[ ${#repos[@]} -eq $REPO_LIMIT ]]; then echo "WARN: $org returned exactly $REPO_LIMIT repos; audit may be truncated. Raise REPO_LIMIT or page through results." >&2 + audit_incomplete=true fi for repo in "${repos[@]}"; do full="$org/$repo" @@ -113,6 +159,11 @@ for org in "${ORGS[@]}"; do if ! gh_api_safe "repos/$full/contents/.github/workflows" '.[].path'; then echo "WARN: $full: workflows listing failed: $GH_API_BODY" >&2 emit "$full,error" + # STRICT_AUDIT contract: every "$full,error" sentinel emitted must + # flag the audit as incomplete. The per-file-fetch error path + # below sets this flag too; both branches must stay in sync or + # STRICT_AUDIT can exit 0 while error rows are present. + audit_incomplete=true continue fi if [[ "$GH_API_STATUS" == "missing" ]] || [[ -z "$GH_API_BODY" ]]; then @@ -147,6 +198,12 @@ for org in "${ORGS[@]}"; do fi [[ -z "$content" ]] && continue while IFS= read -r line; do + # Strip a trailing CR before regex matching. Workflow files + # checked out on Windows or fetched through proxies can + # carry CRLF endings; without this, BASH_REMATCH[4] would + # capture a literal \r at the end of refs like "v4$'\r'", + # failing the 40-hex SHA test and inflating violations. + line="${line%$'\r'}" # Skip comment lines [[ "$line" =~ ^[[:space:]]*# ]] && continue if [[ "$line" =~ $TAG_OR_BRANCH_RE ]]; then @@ -167,8 +224,21 @@ for org in "${ORGS[@]}"; do if [[ "$api_error" == true ]]; then emit "$full,error" + audit_incomplete=true else emit "$full,$violations" fi done done + +# Fail closed when STRICT_AUDIT is enabled. The default flow remains +# report-only (exit 0) so existing CI consumers do not break. Accept the +# common truthy spellings (1, true, yes) case-insensitively rather than the +# literal "1" alone; "STRICT_AUDIT=true" silently degrading to report-only +# was a documented portability footgun. +strict_audit_lower="${STRICT_AUDIT:-0}" +strict_audit_lower="${strict_audit_lower,,}" +if [[ "$strict_audit_lower" =~ ^(1|true|yes)$ && "$audit_incomplete" == true ]]; then + echo "ERROR: STRICT_AUDIT=1 and the audit was incomplete (API errors and/or REPO_LIMIT saturation). Exiting non-zero." >&2 + exit 2 +fi diff --git a/scripts/update-pinned-actions.sh b/scripts/update-pinned-actions.sh index 175b348..74387ae 100755 --- a/scripts/update-pinned-actions.sh +++ b/scripts/update-pinned-actions.sh @@ -131,9 +131,25 @@ resolve_tag_sha() { obj_type="${pair%%|*}" obj_sha="${pair#*|}" + # jq emits the literal string "null" under -r when a referenced field + # is missing or null. Treat that as a resolution failure so the caller + # marks the action SKIPPED rather than writing "actions/checkout@null" + # into the workflow file. Emit a stderr WARN so operators can + # distinguish "tag doesn't exist" from "malformed GitHub API response." + if [[ -z "$obj_type" || -z "$obj_sha" || "$obj_type" == "null" || "$obj_sha" == "null" ]]; then + echo "WARN: $repo: $tag: null/empty .object.type or .object.sha; marking SKIPPED" >&2 + echo "" + return + fi + if [[ "$obj_type" == "tag" ]]; then # Annotated tag: resolve tag object to its target commit SHA obj_sha=$(gh api "repos/$repo/git/tags/$obj_sha" --jq '.object.sha' 2>/dev/null) || { echo ""; return; } + if [[ -z "$obj_sha" || "$obj_sha" == "null" ]]; then + echo "WARN: $repo: $tag: annotated tag dereferenced to null .object.sha; marking SKIPPED" >&2 + echo "" + return + fi fi echo "$obj_sha" @@ -283,7 +299,16 @@ pin_tags_main() { while IFS='|' read -r full_action current_tag new_sha latest_tag; do local _pat _rep safe_tag safe_action safe_current_tag # Escape sed-replacement metacharacters in latest_tag (the replacement - # side). Order matters: backslash first. + # side, not the pattern). Replacement strings have three specials: + # \ literal backslash (must escape first to avoid double-escape) + # & the full matched pattern (would splice the ref back in) + # | only special because the script uses `s|...|...|g`; if the + # delimiter changes to `/` (or any other char), this set must + # be updated to match. The matching pattern-side escape lives + # in escape_sed_pat() below. + # Maintenance note: if the sed delimiter ever changes from `|`, both + # the replacement-side escape set here AND the pattern-side + # escape_sed_pat() function below must be updated together. safe_tag="${latest_tag//\\/\\\\}" safe_tag="${safe_tag//&/\\&}" safe_tag="${safe_tag//|/\\|}" diff --git a/tests/fleet-audit-sha-pins.bats b/tests/fleet-audit-sha-pins.bats index 88c81c8..8edc9aa 100644 --- a/tests/fleet-audit-sha-pins.bats +++ b/tests/fleet-audit-sha-pins.bats @@ -248,3 +248,265 @@ STUB assert_output --partial "ByronWilliamsCPA/repo-partial-fail,error" refute_output --partial "ByronWilliamsCPA/repo-partial-fail,1" } + +@test "audit warns when gh repo list saturates REPO_LIMIT" { + # REPO_LIMIT is env-overridable. Stub returns exactly 3 repos and the test + # sets REPO_LIMIT=3; the script must emit a stderr WARN that this run may + # be truncated. Without the env override, exercising this WARN required + # arranging a 1000-repo stub. + cat > "$TEST_TMPDIR/bin/gh" <<'STUB' +#!/usr/bin/env bash +if [[ "$1 $2" == "auth status" ]]; then exit 0; fi +if [[ "$1" == "repo" && "$2" == "list" ]]; then + _apply_jq_flag '[{"name":"a"},{"name":"b"},{"name":"c"}]' "$@" + exit 0 +fi +if [[ "$1" == "api" ]]; then + # 404 on contents/.github/workflows so each repo records 0 cleanly + echo "gh: Not Found (HTTP 404)" >&2 + exit 1 +fi +exit 0 +STUB + chmod +x "$TEST_TMPDIR/bin/gh" + + REPO_LIMIT=3 run "$SCRIPT" --org ByronWilliamsCPA --output "$TEST_TMPDIR/out.csv" + assert_success + assert_output --partial "WARN: ByronWilliamsCPA returned exactly 3 repos" +} + +@test "audit STRICT_AUDIT=1 exits non-zero on REPO_LIMIT saturation" { + cat > "$TEST_TMPDIR/bin/gh" <<'STUB' +#!/usr/bin/env bash +if [[ "$1 $2" == "auth status" ]]; then exit 0; fi +if [[ "$1" == "repo" && "$2" == "list" ]]; then + _apply_jq_flag '[{"name":"a"},{"name":"b"}]' "$@" + exit 0 +fi +if [[ "$1" == "api" ]]; then + echo "gh: Not Found (HTTP 404)" >&2 + exit 1 +fi +exit 0 +STUB + chmod +x "$TEST_TMPDIR/bin/gh" + + REPO_LIMIT=2 STRICT_AUDIT=1 run "$SCRIPT" --org ByronWilliamsCPA --output "$TEST_TMPDIR/out.csv" + # Script exits 2 to distinguish "audit incomplete" from a generic error + [[ "$status" -eq 2 ]] || { echo "expected exit 2 under STRICT_AUDIT, got $status"; return 1; } + assert_output --partial "ERROR: STRICT_AUDIT=1 and the audit was incomplete" +} + +@test "audit STRICT_AUDIT=1 exits non-zero when any repo emits error sentinel" { + cat > "$TEST_TMPDIR/bin/gh" <<'STUB' +#!/usr/bin/env bash +if [[ "$1 $2" == "auth status" ]]; then exit 0; fi +if [[ "$1" == "repo" && "$2" == "list" ]]; then + _apply_jq_flag '[{"name":"repo-down"}]' "$@" + exit 0 +fi +if [[ "$1" == "api" ]]; then + echo "gh: API rate limit exceeded for user (HTTP 429)" >&2 + exit 1 +fi +exit 0 +STUB + chmod +x "$TEST_TMPDIR/bin/gh" + + STRICT_AUDIT=1 run "$SCRIPT" --org ByronWilliamsCPA --output "$TEST_TMPDIR/out.csv" + [[ "$status" -eq 2 ]] || { echo "expected exit 2 under STRICT_AUDIT, got $status"; return 1; } + assert_output --partial "ERROR: STRICT_AUDIT=1 and the audit was incomplete" + run cat "$TEST_TMPDIR/out.csv" + assert_output --partial "ByronWilliamsCPA/repo-down,error" +} + +@test "audit STRICT_AUDIT=1 remains exit 0 on a fully clean run (no errors, no saturation)" { + # Regression guard: a clean run under STRICT_AUDIT must still succeed. + # Otherwise CI gates would fire on every healthy audit. + _write_gh_stub + STRICT_AUDIT=1 run "$SCRIPT" --org ByronWilliamsCPA --output "$TEST_TMPDIR/out.csv" + assert_success + run cat "$TEST_TMPDIR/out.csv" + assert_output --partial "ByronWilliamsCPA/repo-dirty,2" + assert_output --partial "ByronWilliamsCPA/repo-clean,0" +} + +@test "audit --skip-owners with a single owner (no commas) skips correctly" { + # SKIP_OWNERS framing is ",$VAR," so a single value becomes ",actions," + # and matches an owner literally named "actions". Verifies the case + # statement handles the no-comma input. + _write_gh_stub + run "$SCRIPT" --org ByronWilliamsCPA --skip-owners 'actions' --output "$TEST_TMPDIR/out.csv" + assert_success + run cat "$TEST_TMPDIR/out.csv" + # repo-dirty has two third-party actions/* refs; both should now be skipped + assert_output --partial "ByronWilliamsCPA/repo-dirty,0" +} + +@test "audit --skip-owners with empty string does not skip anything" { + # Empty SKIP_OWNERS becomes the case pattern ",,". No real owner string + # matches that, so both violations in repo-dirty must still be counted. + # Positive controls: clean repo row must still be 0, and no spurious + # "skipping owner ''" WARN may leak (a future regression on the case + # framing would match empty owners on every ref). + _write_gh_stub + run "$SCRIPT" --org ByronWilliamsCPA --skip-owners '' --output "$TEST_TMPDIR/out.csv" + assert_success + refute_output --partial "skipping owner" + run cat "$TEST_TMPDIR/out.csv" + assert_output --partial "ByronWilliamsCPA/repo-dirty,2" + assert_output --partial "ByronWilliamsCPA/repo-clean,0" +} + +@test "audit --skip-owners with double internal commas still matches listed owners" { + # SKIP_OWNERS=",,actions,," builds the case framing ",,,actions,,," which + # contains noisy empty segments. Owner "actions" must still match the + # ",actions," substring without confusion from the adjacent empties. + # Both refs in repo-dirty (actions/checkout, actions/setup-python) share + # owner "actions" and should both be skipped. + _write_gh_stub + run "$SCRIPT" --org ByronWilliamsCPA --skip-owners ',,actions,,' \ + --output "$TEST_TMPDIR/out.csv" + assert_success + run cat "$TEST_TMPDIR/out.csv" + assert_output --partial "ByronWilliamsCPA/repo-dirty,0" +} + +@test "audit exits 1 when REPO_LIMIT is non-numeric" { + # The validation guard fires before any audit work. The script must reject + # garbage values up-front rather than crashing inside the `[[ -eq ]]` test + # under set -euo pipefail. + _write_gh_stub + REPO_LIMIT=abc run "$SCRIPT" --org ByronWilliamsCPA --output "$TEST_TMPDIR/out.csv" + [[ "$status" -eq 1 ]] || { echo "expected exit 1 for non-numeric REPO_LIMIT, got $status"; return 1; } + assert_output --partial "ERROR: REPO_LIMIT must be a positive integer" +} + +@test "audit exits 1 when REPO_LIMIT is zero" { + # Zero would silently truncate the audit to no repos; the validator must + # require a positive integer (^[1-9][0-9]*$), not just any integer. + _write_gh_stub + REPO_LIMIT=0 run "$SCRIPT" --org ByronWilliamsCPA --output "$TEST_TMPDIR/out.csv" + [[ "$status" -eq 1 ]] || { echo "expected exit 1 for zero REPO_LIMIT, got $status"; return 1; } + assert_output --partial "ERROR: REPO_LIMIT must be a positive integer" +} + +@test "audit treats REPO_LIMIT='' (empty) as use-default-1000" { + # Bash semantics: `${REPO_LIMIT:-1000}` substitutes the default when the + # variable is unset OR null/empty. So `REPO_LIMIT=''` is equivalent to + # not setting it -- the script should fall back to 1000 and run normally, + # not reject. This documents the intended behavior so a future + # refactor that switches to `${REPO_LIMIT-1000}` (no colon, treats empty + # as a literal value) is caught by CI. + _write_gh_stub + REPO_LIMIT='' run "$SCRIPT" --org ByronWilliamsCPA --output "$TEST_TMPDIR/out.csv" + assert_success + refute_output --partial "ERROR: REPO_LIMIT" +} + +@test "audit STRICT_AUDIT=1 exits non-zero when gh repo list fails" { + # mapfile via process substitution was previously swallowing the rc. + # Now `gh repo list` is captured to a tmpfile and the rc surfaces; an + # auth-loss or rate-limit mid-enumeration must mark the audit incomplete + # and exit 2 under STRICT_AUDIT. + cat > "$TEST_TMPDIR/bin/gh" <<'STUB' +#!/usr/bin/env bash +if [[ "$1 $2" == "auth status" ]]; then exit 0; fi +if [[ "$1" == "repo" && "$2" == "list" ]]; then + echo "gh: rate limit exceeded for user (HTTP 429)" >&2 + exit 1 +fi +exit 0 +STUB + chmod +x "$TEST_TMPDIR/bin/gh" + + STRICT_AUDIT=1 run "$SCRIPT" --org ByronWilliamsCPA --output "$TEST_TMPDIR/out.csv" + [[ "$status" -eq 2 ]] || { echo "expected exit 2 under STRICT_AUDIT with repo-list failure, got $status"; return 1; } + assert_output --partial "WARN: ByronWilliamsCPA: gh repo list failed" + assert_output --partial "ERROR: STRICT_AUDIT" +} + +@test "audit default flow stays exit 0 when gh repo list fails (report-only)" { + # Regression guard for the same path WITHOUT STRICT_AUDIT: the script must + # still exit 0 so existing report-only consumers do not break, even when + # an org enumeration fails. The WARN must still be emitted. + cat > "$TEST_TMPDIR/bin/gh" <<'STUB' +#!/usr/bin/env bash +if [[ "$1 $2" == "auth status" ]]; then exit 0; fi +if [[ "$1" == "repo" && "$2" == "list" ]]; then + echo "gh: rate limit exceeded for user (HTTP 429)" >&2 + exit 1 +fi +exit 0 +STUB + chmod +x "$TEST_TMPDIR/bin/gh" + + run "$SCRIPT" --org ByronWilliamsCPA --output "$TEST_TMPDIR/out.csv" + assert_success + assert_output --partial "WARN: ByronWilliamsCPA: gh repo list failed" +} + +@test "audit STRICT_AUDIT=true (string) is treated as truthy" { + # The Suggested-tier review flagged that STRICT_AUDIT=1 only matched the + # literal "1"; "true", "yes", and case variants would silently degrade to + # report-only. Confirm the widened predicate accepts "true" via the same + # repo-list-failure path used above. + cat > "$TEST_TMPDIR/bin/gh" <<'STUB' +#!/usr/bin/env bash +if [[ "$1 $2" == "auth status" ]]; then exit 0; fi +if [[ "$1" == "repo" && "$2" == "list" ]]; then + echo "gh: rate limit exceeded for user (HTTP 429)" >&2 + exit 1 +fi +exit 0 +STUB + chmod +x "$TEST_TMPDIR/bin/gh" + + STRICT_AUDIT=true run "$SCRIPT" --org ByronWilliamsCPA --output "$TEST_TMPDIR/out.csv" + [[ "$status" -eq 2 ]] || { echo "expected exit 2 under STRICT_AUDIT=true, got $status"; return 1; } +} + +@test "audit STRICT_AUDIT=YES (case-insensitive truthy) is treated as truthy" { + cat > "$TEST_TMPDIR/bin/gh" <<'STUB' +#!/usr/bin/env bash +if [[ "$1 $2" == "auth status" ]]; then exit 0; fi +if [[ "$1" == "repo" && "$2" == "list" ]]; then + echo "gh: rate limit exceeded for user (HTTP 429)" >&2 + exit 1 +fi +exit 0 +STUB + chmod +x "$TEST_TMPDIR/bin/gh" + + STRICT_AUDIT=YES run "$SCRIPT" --org ByronWilliamsCPA --output "$TEST_TMPDIR/out.csv" + [[ "$status" -eq 2 ]] || { echo "expected exit 2 under STRICT_AUDIT=YES, got $status"; return 1; } +} + +@test "audit STRICT_AUDIT=1 exits non-zero on workflows-listing failure (Critical #1 regression)" { + # This is the path that previously emitted "$full,error" + continue + # without setting audit_incomplete. Stub `gh repo list` to succeed, but + # `gh api` (workflows listing) to return a non-404 error. With the fix, + # the workflows-listing failure path must now flag audit_incomplete and + # STRICT_AUDIT=1 must exit 2. + cat > "$TEST_TMPDIR/bin/gh" <<'STUB' +#!/usr/bin/env bash +if [[ "$1 $2" == "auth status" ]]; then exit 0; fi +if [[ "$1" == "repo" && "$2" == "list" ]]; then + _apply_jq_flag '[{"name":"repo-down"}]' "$@" + exit 0 +fi +if [[ "$1" == "api" ]]; then + echo "gh: API rate limit exceeded for user (HTTP 429)" >&2 + exit 1 +fi +exit 0 +STUB + chmod +x "$TEST_TMPDIR/bin/gh" + + STRICT_AUDIT=1 run "$SCRIPT" --org ByronWilliamsCPA --output "$TEST_TMPDIR/out.csv" + [[ "$status" -eq 2 ]] || { echo "expected exit 2 on workflows-listing 429 under STRICT_AUDIT, got $status"; return 1; } + assert_output --partial "WARN: ByronWilliamsCPA/repo-down: workflows listing failed" + assert_output --partial "ERROR: STRICT_AUDIT" + run cat "$TEST_TMPDIR/out.csv" + assert_output --partial "ByronWilliamsCPA/repo-down,error" +} diff --git a/tests/update-pinned-actions.bats b/tests/update-pinned-actions.bats index e4f04e2..231417e 100644 --- a/tests/update-pinned-actions.bats +++ b/tests/update-pinned-actions.bats @@ -839,3 +839,161 @@ EOF "$pin_tags_dir/tag-pinned.yml" assert_success } + +@test "--pin-tags reports branch refs that carry an inline comment" { + # extract_branch_pins was updated to accept "# comment" suffixes; the + # existing branch-comment fixture (tag-pinned.yml's some-org/some-action@main) + # has no comment, so this path is unverified. Add a fixture with a + # comment-annotated branch ref and assert it surfaces in the WARN list. + _write_gh_stub_pin_tags + pin_tags_dir="$TEST_TMPDIR/pin_tags_workdir" + mkdir -p "$pin_tags_dir" + cat > "$pin_tags_dir/branch-with-comment.yml" <<'YML' +name: Fixture +on: push +jobs: + example: + runs-on: ubuntu-latest + steps: + - uses: some-org/branch-action@main # tracking head + - uses: actions/checkout@v4 +YML + + run "$SCRIPT" --pin-tags --workflows-dir "$pin_tags_dir" + assert_success + # The branch ref must appear in the WARN block even though it carries + # an inline "# tracking head" comment that the old regex would have + # treated as the line terminator. Anchor the assertion to the WARN + # header so a future regression that merely echoed the fixture name + # or unrelated context can't pass this test silently. + assert_output --partial "WARN: branch ref detected" + assert_output --partial "some-org/branch-action@main" +} + +@test "--pin-tags --apply on a workflow with no third-party refs succeeds without warnings" { + # Regression guard for the rc-aware extract_tag_pins/extract_branch_pins + # path: a fixture containing ONLY first-party refs (skipped by the + # owner allowlist) means grep matches lines but the awk filter emits + # nothing. The function must return 0 cleanly and the script must exit 0. + _write_gh_stub_pin_tags + pin_tags_dir="$TEST_TMPDIR/pin_tags_workdir" + mkdir -p "$pin_tags_dir" + cat > "$pin_tags_dir/all-first-party.yml" <<'YML' +name: Fixture +on: push +jobs: + example: + runs-on: ubuntu-latest + steps: + - uses: ByronWilliamsCPA/.github/.github/workflows/python-ci.yml@v1 + - uses: williaby/.github/.github/workflows/release-tag.yml@v1 +YML + + run "$SCRIPT" --pin-tags --apply --workflows-dir "$pin_tags_dir" + assert_success + assert_output --partial "No third-party tag-pinned actions found" + # First-party refs must be untouched + run grep -F "ByronWilliamsCPA/.github/.github/workflows/python-ci.yml@v1" \ + "$pin_tags_dir/all-first-party.yml" + assert_success +} + +@test "--pin-tags --apply skips when annotated-tag dereferencing returns missing .object.sha" { + # Stub the two-step dereferencing path so the second call returns an + # object without `.sha`. jq emits "null" under -r; resolve_tag_sha must + # detect that and return empty, causing the action to be marked SKIPPED + # rather than writing "actions/checkout@null" into the workflow file. + local annotated_sha="bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" # pragma: allowlist secret + cat > "$GH_BIN/gh" <&2; exit 1 ;; +esac +EOF + chmod +x "$GH_BIN/gh" + + pin_tags_dir="$TEST_TMPDIR/pin_tags_workdir" + mkdir -p "$pin_tags_dir" + cp "$FIXTURES/tag-pinned.yml" "$pin_tags_dir/" + + run "$SCRIPT" --pin-tags --apply --workflows-dir "$pin_tags_dir" + assert_success + assert_output --partial "SKIP: cannot resolve SHA" + + # The original v4 ref must remain untouched: the dereferencing failed, + # so no substitution should have happened for actions/checkout. + run grep -F "actions/checkout@v4" "$pin_tags_dir/tag-pinned.yml" + assert_success + # The literal string "null" must NOT have leaked into the workflow file + run grep -F "actions/checkout@null" "$pin_tags_dir/tag-pinned.yml" + assert_failure + # actions/setup-python had a clean lightweight-tag path; it should be + # converted normally + run grep -F "actions/setup-python@$SETUP_PYTHON_V5_LATEST_SHA" \ + "$pin_tags_dir/tag-pinned.yml" + assert_success +} + +@test "--pin-tags --apply skips when first git/refs/tags call returns null .object.type" { + # The Suggested-tier follow-up review noted only the SECOND-call null guard + # was tested. This exercises the FIRST-call guard (added at + # scripts/update-pinned-actions.sh:138): when the initial + # `repos/$repo/git/refs/tags/$tag` response has a malformed `.object` + # (e.g., `null`), jq -r emits the literal "null" for both fields and + # `resolve_tag_sha` must mark the action SKIPPED rather than writing + # `@null` into the workflow file. The stderr WARN added alongside the + # guard must also fire so operators can distinguish malformed responses + # from "tag does not exist". + cat > "$GH_BIN/gh" <&2; exit 1 ;; +esac +EOF + chmod +x "$GH_BIN/gh" + + pin_tags_dir="$TEST_TMPDIR/pin_tags_workdir" + mkdir -p "$pin_tags_dir" + cp "$FIXTURES/tag-pinned.yml" "$pin_tags_dir/" + + run "$SCRIPT" --pin-tags --apply --workflows-dir "$pin_tags_dir" + assert_success + assert_output --partial "SKIP: cannot resolve SHA" + # The new WARN must fire on the first-call null-object path. + assert_output --partial "null/empty .object.type or .object.sha" + + # actions/checkout was malformed: original v4 ref must remain. + run grep -F "actions/checkout@v4" "$pin_tags_dir/tag-pinned.yml" + assert_success + # The literal string "null" must NOT have leaked into the workflow file. + run grep -F "actions/checkout@null" "$pin_tags_dir/tag-pinned.yml" + assert_failure + # actions/setup-python had a clean lightweight-tag path; converted normally. + run grep -F "actions/setup-python@$SETUP_PYTHON_V5_LATEST_SHA" \ + "$pin_tags_dir/tag-pinned.yml" + assert_success +}