diff --git a/.github/actions/auto-approve-bot-prs/README.md b/.github/actions/auto-approve-bot-prs/README.md index e39ec31..da8dc3b 100644 --- a/.github/actions/auto-approve-bot-prs/README.md +++ b/.github/actions/auto-approve-bot-prs/README.md @@ -1,8 +1,14 @@ # Auto-approve bot PRs Approves PRs from trusted bot authors whose title or branch matches a known -safe pattern, after all other CI checks pass. Never hard-fails the job -- -every failure mode degrades to a notice-level skip. +safe pattern, after all other CI checks pass. No API, parse, permission or +input-validation failure exits non-zero: each degrades to an annotated skip and +exit 0. + +Refusing to approve is annotated at **error** level, because it is a real +outcome that something downstream may be blocking on (a release cut waiting for +the bump PR to merge, for example). That raises an annotation only; the step +still exits 0, so the job conclusion and any required check stay green. Safe patterns: `chore(` / `chore:` titles, `fix(deps):` titles, `backport/` / `renovate/` / `update-platform-version-` branches. @@ -29,26 +35,86 @@ slow external checks have not shown up yet. -| INPUT | TYPE | REQUIRED | DEFAULT | DESCRIPTION | -|--------------------|--------|----------|------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------| -| auto-merge | string | false | `"false"` | Enable GitHub auto-merge after approval | -| github-token | string | true | | PAT used to read PR state,
approve, and enable auto-merge. Must NOT
match the PR author. | -| merge-method | string | false | `"squash"` | Merge method for auto-merge (squash|merge|rebase) | -| trusted-authors | string | false | `"renovate[bot],loft-bot,github-actions[bot]"` | Comma-separated list of trusted bot logins | -| wait-max-attempts | string | false | `"90"` | Max polling attempts waiting for other
CI checks | -| wait-min-attempts | string | false | `"12"` | Minimum polls before ci_green=true is allowed.
Prevents early approval while slow external
checks (e.g. Netlify) have not yet registered. | -| wait-sleep-seconds | string | false | `"10"` | Seconds between polling attempts | +| INPUT | TYPE | REQUIRED | DEFAULT | DESCRIPTION | +|--------------------|--------|----------|------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| auto-merge | string | false | `"false"` | Enable GitHub auto-merge after approval | +| ci-read-token | string | false | | Token for the read-only CI poll
only; defaults to the caller GITHUB_TOKEN,
which needs `checks: read` and `statuses: read`. Never
the approving PAT. See README "Two
tokens, on purpose". | +| github-token | string | true | | PAT used to read PR state,
approve, and enable auto-merge. Must NOT
match the PR author. | +| merge-method | string | false | `"squash"` | Merge method for auto-merge (squash|merge|rebase) | +| trusted-authors | string | false | `"renovate[bot],loft-bot,github-actions[bot]"` | Comma-separated list of trusted bot logins | +| wait-max-attempts | string | false | `"90"` | Max polling attempts waiting for other
CI checks | +| wait-min-attempts | string | false | `"12"` | Minimum polls before ci_green=true is allowed.
Prevents early approval while slow external
checks (e.g. Netlify) have not yet registered. | +| wait-sleep-seconds | string | false | `"10"` | Seconds between polling attempts | +## Two tokens, on purpose + +Approving needs a PAT, because GitHub forbids self-approval and the approver +identity must differ from the PR author. **Reading CI state does not.** The two +are split: + +| Step | Token | +|------|-------| +| `check-pr-ready`, *Approve PR*, *Enable auto-merge* | `github-token` (PAT) | +| *Wait for other CI to pass* | `ci-read-token`, defaulting to `GITHUB_TOKEN` | + +Do not point `ci-read-token` at the approving PAT. +**Fine-grained PATs cannot call the Checks API at all** — there is no `Checks` +permission to grant, and the fine-grained permissions reference lists no +`/check-runs` endpoints. The poll would fail on every attempt and the action +would default-deny forever. This is not hypothetical: it stalled the `v0.36.1` +release cut for ~70 minutes (DEVOPS-1254). + +Note that a fine-grained PAT appears to work on a **public** repository, where +`/check-runs` answers with no credentials at all, so working there proved nothing +about a private caller. Do not extrapolate the reverse: `GITHUB_TOKEN` is scoped +by the `permissions:` block whatever the repository's visibility, so the grants +below are required either way. + +## Required caller permissions + +Unless the caller supplies `ci-read-token`, it must grant these itself. They +cannot be added by this action or by the reusable workflow that wraps it: per GitHub, *"the `GITHUB_TOKEN` permissions +passed from the caller workflow can be only downgraded (not elevated) by the +called workflow"*, and any permission the caller omits defaults to `none`. + +```yaml +permissions: + contents: read + pull-requests: write + checks: read # CI poll: /commits/:sha/check-runs + statuses: read # CI poll: /commits/:sha/status +``` + +Omitting `checks`/`statuses` does not fail loudly. CI stays green, the job stays +green, and the PR is simply never approved. + ## Usage +The `permissions:` block is part of the usage, not an optional extra. Copying +this snippet without it reproduces the silent no-approve failure described above. + ```yaml -- uses: loft-sh/github-actions/.github/actions/auto-approve-bot-prs@auto-approve-bot-prs/v1 - with: - github-token: ${{ secrets.GH_ACCESS_TOKEN }} +jobs: + auto-approve: + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: write + checks: read # CI poll: /commits/:sha/check-runs + statuses: read # CI poll: /commits/:sha/status + steps: + - uses: loft-sh/github-actions/.github/actions/auto-approve-bot-prs@auto-approve-bot-prs/v1 + with: + github-token: ${{ secrets.GH_ACCESS_TOKEN }} ``` +If the caller genuinely cannot grant `checks: read` (an org policy pinning the +default token, say), pass `ci-read-token` instead: a classic PAT with `repo` +scope, or a GitHub App token. Both can reach the Checks API. A fine-grained PAT +cannot, so `github-token` is never a valid value for it. + ## Testing ```bash diff --git a/.github/actions/auto-approve-bot-prs/action.yml b/.github/actions/auto-approve-bot-prs/action.yml index 33c0b40..f9e7ce1 100644 --- a/.github/actions/auto-approve-bot-prs/action.yml +++ b/.github/actions/auto-approve-bot-prs/action.yml @@ -1,8 +1,12 @@ name: Auto-approve bot PRs description: | Approves PRs from trusted bot authors whose title/branch matches a known - safe pattern, after all other CI checks pass. Never hard-fails the job — - every failure mode degrades to a notice-level skip. + safe pattern, after all other CI checks pass. Once running, no API, parse or + permission failure exits non-zero: each degrades to an annotated skip and exit + 0, and an out-of-range wait input is coerced to its default with a warning. + (Missing required env is the one deliberate exception and does exit non-zero.) + Outcomes that need a human and raise no other red signal are annotated at error + level; that raises an annotation only, never a non-zero exit. inputs: trusted-authors: description: 'Comma-separated list of trusted bot logins' @@ -19,6 +23,9 @@ inputs: github-token: description: 'PAT used to read PR state, approve, and enable auto-merge. Must NOT match the PR author.' required: true + ci-read-token: + description: 'Token for the read-only CI poll only; defaults to the caller GITHUB_TOKEN, which needs `checks: read` and `statuses: read`. Never the approving PAT. See README "Two tokens, on purpose".' + required: false wait-max-attempts: description: 'Max polling attempts waiting for other CI checks' required: false @@ -60,7 +67,13 @@ runs: id: ci shell: bash env: - GH_TOKEN: ${{ inputs.github-token }} + # Deliberately NOT inputs.github-token. See ci-read-token above. + # The fallback lives here rather than in an input `default:` because a + # default only applies when the input is OMITTED. Our own reusable + # workflow always passes the value through from an optional secret, so it + # arrives as an explicit empty string when the caller sets nothing, and a + # `default:` would not fire. `||` treats empty as falsy and does. + GH_TOKEN: ${{ inputs.ci-read-token || github.token }} PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} SELF_RUN_ID: ${{ github.run_id }} WAIT_MAX_ATTEMPTS: ${{ inputs.wait-max-attempts }} diff --git a/.github/actions/auto-approve-bot-prs/src/wait-for-ci.sh b/.github/actions/auto-approve-bot-prs/src/wait-for-ci.sh index 3853fbc..a6a43dc 100755 --- a/.github/actions/auto-approve-bot-prs/src/wait-for-ci.sh +++ b/.github/actions/auto-approve-bot-prs/src/wait-for-ci.sh @@ -31,9 +31,88 @@ set -euo pipefail : "${PR_HEAD_SHA:?PR_HEAD_SHA required}" : "${SELF_RUN_ID:?SELF_RUN_ID required}" -max_attempts="${WAIT_MAX_ATTEMPTS:-90}" -min_attempts="${WAIT_MIN_ATTEMPTS:-12}" -sleep_seconds="${WAIT_SLEEP_SECONDS:-10}" +# sanitize_for_log — stdin to a single safe log line on stdout. +# +# EVERY externally-controlled string that reaches an `echo` in this script must +# go through here. Two channels qualify and both are hostile: +# - gh's stderr (GitHub-controlled), and +# - check-run `.name` / commit-status `.context`, which are written by whoever +# posted the check on the head SHA. GitHub documents no character +# restriction on them, so they are the *wider* of the two channels. +# +# What it defends against: +# - CR as well as LF terminates a log line for the runner, so a raw one splits +# the output into a NEW line, and a line beginning `::` is parsed as a +# workflow command. An attacker naming a check `x::error::…` would +# otherwise forge one. Collapse both, plus TAB, to spaces. +# - Other separators the runner or a terminal may treat as breaks (VT, FF, NUL, +# DEL, and via the non-ASCII strip: NEL, U+2028, U+2029) are removed. +# - Non-ASCII is dropped rather than byte-truncated, so the length cap cannot +# split a UTF-8 sequence mid-character. +# - `%` is the workflow-command escape introducer, so a literal `%0A`/`%25` +# would otherwise be decoded into the annotation. Escaped AFTER the first +# cut so that cut cannot bisect an escape we just wrote; a second cut then +# bounds the real emitted length, and the trailing-partial strip removes a +# `%` or `%2` left dangling by it. +# - LC_ALL is pinned on both `tr` calls so an inherited locale cannot defeat +# the class matching. +# +# Never fails: a subshell abort here would kill the script under `set -e` before +# it could emit ci_green, which is the same contract violation as a fatal mktemp. +# Optional arg: max emitted characters (default 300). Escaping runs BEFORE the +# truncation so the cap bounds the line that is actually emitted rather than a +# pre-escape length that can then triple; the trailing-partial strip removes a +# `%` or `%2` the cut may leave dangling. Truncation is marked, because a name +# severed mid-word reads as a corrupted check name rather than as elision. +sanitize_for_log() { + local max="${1:-300}" + { LC_ALL=C tr '\n\r\t' ' ' \ + | LC_ALL=C tr -cd '\040-\176' \ + | sed 's/%/%25/g' \ + | LC_ALL=C awk -v m="$max" '{ if (length($0) > m) printf "%s... (truncated)\n", substr($0, 1, m); else print }' \ + | sed 's/%2\{0,1\}$//'; } 2>/dev/null || true +} + +# gh_last_error — one-line summary of why the last gh call failed, for logs. +# Discarding this is how a permanent permission problem masquerades as a +# transient blip: both produce the identical "API failed" line and the identical +# default-deny exit, so a misconfigured token is indistinguishable from a bad +# minute at GitHub. The single most useful case is a 403, which means the +# CI-read token cannot reach the Checks API. +gh_last_error() { + [ -n "$GH_ERR_FILE" ] || return 0 + [ -s "$GH_ERR_FILE" ] || return 0 + sanitize_for_log < "$GH_ERR_FILE" +} + +# safe — sanitize an API-derived value for interpolation into a log line. +safe() { + printf '%s' "${1:-}" | sanitize_for_log +} + +# Coerce rather than trust. A non-numeric value here used to reach `sleep` and +# `seq` directly and abort the script under `set -e` with no ci_green emitted at +# all — exit 1, which for a direct consumer of the composite (no +# continue-on-error in the documented usage) is a red job. The contract is that +# no input can do that. +# An upper bound matters as much as the lower one. Without it the accept/reject +# line was an accident of int64 overflow in `[ -gt ]`, so a plausible fat-finger +# like 1000000000 was accepted and the loop below then hung with no output at all +# until GitHub's 6-hour job timeout — a strictly worse version of the stall this +# script exists to prevent. The length test comes first so overflow never decides. +numeric_or_default() { + local name="$1" value="$2" fallback="$3" max="$4" + if [ -n "$value" ] && [ -z "${value//[0-9]/}" ] && [ "${#value}" -le 9 ] \ + && [ "$value" -gt 0 ] && [ "$value" -le "$max" ]; then + printf '%s' "$value" + return 0 + fi + echo "::warning::${name}='$(safe "$value")' is not an integer in 1..${max}; using ${fallback}" >&2 + printf '%s' "$fallback" +} +max_attempts="$(numeric_or_default WAIT_MAX_ATTEMPTS "${WAIT_MAX_ATTEMPTS:-90}" 90 100000)" +min_attempts="$(numeric_or_default WAIT_MIN_ATTEMPTS "${WAIT_MIN_ATTEMPTS:-12}" 12 100000)" +sleep_seconds="$(numeric_or_default WAIT_SLEEP_SECONDS "${WAIT_SLEEP_SECONDS:-10}" 10 3600)" emit() { local k="$1" v="$2" @@ -41,14 +120,35 @@ emit() { printf '%s=%s\n' "$k" "$v" } +# Scratch file holding the stderr of the most recent gh call. This has to be a +# file, not a variable: gh_json is always invoked inside a command substitution, +# so it runs in a subshell and any variable it sets is lost to the caller. (The +# EXIT trap below IS inherited by that subshell but never executes on its exit, +# so it fires exactly once, on the real exit — verified: the file is not deleted +# out from under the caller, and the guarded trap does not alter exit status.) +# +# A failure to allocate the file must NOT be fatal. This script documents +# "Always exits 0" and action.yml promises the job never hard-fails, and a direct +# consumer of the composite has no continue-on-error to hide behind. So degrade: +# an empty GH_ERR_FILE means "capture unavailable", not "abort". +GH_ERR_FILE="" +if ! GH_ERR_FILE="$(mktemp 2>/dev/null)"; then + GH_ERR_FILE="" + echo "::warning::could not allocate a temp file for API error capture; errors will be reported without detail" +fi +trap '[ -n "$GH_ERR_FILE" ] && rm -f "$GH_ERR_FILE"' EXIT + # gh_json — fetch json body; on any failure print empty and return 1. # Crucially does NOT swallow errors into "[]" — callers must distinguish # "API said there is nothing" from "API failed and we have no idea". gh_json() { - local path="$1" body - if ! body=$(gh api "$path" --paginate 2>/dev/null); then + local path="$1" body err="${GH_ERR_FILE:-/dev/null}" + [ -n "$GH_ERR_FILE" ] && : > "$GH_ERR_FILE" + if ! body=$(gh api "$path" --paginate 2>"$err"); then return 1 fi + # Clear on success so a later unrelated failure cannot report stale text. + [ -n "$GH_ERR_FILE" ] && : > "$GH_ERR_FILE" printf '%s' "$body" } @@ -68,14 +168,29 @@ EXCLUDE_PATTERN="/runs/${SELF_RUN_ID}/" consecutive_errors=0 max_consecutive_errors=5 +# Declared up front: a jq parse failure sets poll_errored without going through +# gh_last_error, and `set -u` would abort on an unset read in the bail path. +last_error="" +# last_error is reset every poll (see below), which is right for attributing a +# poll's own failure but loses the actionable one: four real 403s followed by one +# malformed response would report only the parse error. Keep the first as well. +first_error="" -for attempt in $(seq 1 "$max_attempts"); do +attempt=0 +while [ "$attempt" -lt "$max_attempts" ]; do + attempt=$(( attempt + 1 )) poll_errored=0 + # Reset per poll. Without this, an API error early in the run stays in + # last_error and gets reported by a LATER jq-parse failure, sending the + # operator to check token permissions for a malformed-response fault. That is + # the same misdiagnosis this error reporting exists to prevent. + last_error="" # -- Fetch check-runs ----------------------------------------------------- runs_raw="" if ! runs_raw=$(gh_json "repos/${GITHUB_REPOSITORY}/commits/${PR_HEAD_SHA}/check-runs"); then - echo "::warning::attempt ${attempt}/${max_attempts}: check-runs API failed" + last_error="$(gh_last_error)" + echo "::warning::attempt ${attempt}/${max_attempts}: check-runs API failed${last_error:+ (${last_error})}" poll_errored=1 fi @@ -97,6 +212,7 @@ for attempt in $(seq 1 "$max_attempts"); do | group_by(.name // "") | map(sort_by(.started_at // "", .id // 0) | last) ' "$runs_raw"); then + last_error="malformed check-runs response (jq could not parse it)" echo "::warning::attempt ${attempt}/${max_attempts}: check-runs jq parse failed" poll_errored=1 fi @@ -126,7 +242,8 @@ for attempt in $(seq 1 "$max_attempts"); do statuses_raw="" if [ "$poll_errored" -eq 0 ]; then if ! statuses_raw=$(gh_json "repos/${GITHUB_REPOSITORY}/commits/${PR_HEAD_SHA}/status"); then - echo "::warning::attempt ${attempt}/${max_attempts}: statuses API failed" + last_error="$(gh_last_error)" + echo "::warning::attempt ${attempt}/${max_attempts}: statuses API failed${last_error:+ (${last_error})}" poll_errored=1 fi fi @@ -137,6 +254,7 @@ for attempt in $(seq 1 "$max_attempts"); do (.statuses // []) | [.[] | select((.target_url // "") | contains("'"$EXCLUDE_PATTERN"'") | not)] ' "$statuses_raw"); then + last_error="malformed commit-status response (jq could not parse it)" echo "::warning::attempt ${attempt}/${max_attempts}: statuses jq parse failed" poll_errored=1 fi @@ -152,18 +270,35 @@ for attempt in $(seq 1 "$max_attempts"); do # -- Consume the poll ----------------------------------------------------- if [ "$poll_errored" -eq 1 ]; then + # The jq_or_fail metric extractions above set poll_errored without a message + # of their own; give them one rather than reporting "unknown". + [ -n "$last_error" ] || last_error="could not extract check state from the API response" + [ -n "$first_error" ] || first_error="$last_error" # Default-deny on API/parse errors: this poll does not count toward the # settle floor, and too many consecutive errors exit non-green. consecutive_errors=$(( consecutive_errors + 1 )) if [ "$consecutive_errors" -ge "$max_consecutive_errors" ]; then - echo "::notice::Too many consecutive API errors (${consecutive_errors}); refusing to approve" + # Only worth printing when it differs, else the same blob appears twice. + first_error_clause="" + [ -n "$first_error" ] && [ "$first_error" != "$last_error" ] \ + && first_error_clause="; first error: ${first_error}" + # ::error:: rather than ::notice::. The job keeps its continue-on-error + # safety net, so this still cannot turn a caller's CI red, but the run no + # longer looks clean. Refusing to approve is a real outcome and something + # downstream may be blocking on the merge that will now never happen. + echo "::error::Too many consecutive API errors (${consecutive_errors}); refusing to approve. Last error: ${last_error:-unknown}${first_error_clause}" + echo "::error::If that is a 403 or 404 on a private repository, the CI-read token cannot reach the Checks API. Fine-grained PATs have no Checks permission at all. Leave ci-read-token unset so it falls back to GITHUB_TOKEN, and grant 'checks: read' and 'statuses: read' in the CALLER workflow: a reusable workflow can only downgrade the caller's permissions, never add to them." emit ci_green false exit 0 fi sleep "$sleep_seconds" continue fi + # Reset first_error with the streak. Keeping it across a recovered poll is the + # F1 misattribution bug one variable over: a 403 that has since resolved would + # be reported as the "first error" for a later, unrelated parse-failure streak. consecutive_errors=0 + first_error="" pending=$(( cr_pending + st_pending )) real_failed=$(( cr_real_failed + st_failed )) @@ -172,7 +307,8 @@ for attempt in $(seq 1 "$max_attempts"); do # Terminal failures (failure, timed_out, action_required, etc.) bail # immediately — these are not transient and won't be replaced. if [ "$real_failed" -gt 0 ]; then - details=$(printf '%s\n%s' "$cr_real_failed_detail" "$st_failed_detail" | awk 'NF' | paste -sd, - | sed 's/,/, /g') + # Sanitized: these carry attacker-settable check names / status contexts. + details=$(printf '%s\n%s' "$cr_real_failed_detail" "$st_failed_detail" | awk 'NF' | paste -sd, - | sed 's/,/, /g' | sanitize_for_log 1000) echo "::notice::Other CI checks failed; skipping approval. Failing: ${details:-unknown}" emit ci_green false exit 0 @@ -182,7 +318,10 @@ for attempt in $(seq 1 "$max_attempts"); do # Past the settle floor we stop waiting and treat them as final — the # replacement should have registered by now if it was ever going to. if [ "$cr_cancelled" -gt 0 ] && [ "$attempt" -ge "$min_attempts" ]; then - echo "::notice::Cancelled checks did not get replaced within settle period; skipping approval. Cancelled: ${cr_cancelled_detail:-unknown}" + # ::error:: because a cancelled check is not red anywhere else: unlike the + # failed-checks path below, this annotation is the only signal an operator + # gets, and a release cut may be blocking on the merge. + echo "::error::Cancelled checks did not get replaced within settle period; skipping approval. Cancelled: $(safe "${cr_cancelled_detail:-unknown}")" emit ci_green false exit 0 fi @@ -190,11 +329,11 @@ for attempt in $(seq 1 "$max_attempts"); do # Surface which signals we are still waiting on. Helps operators diagnose # "why is this job still running?" without enabling step debug logging. if [ "$pending" -gt 0 ]; then - waiting=$(printf '%s\n%s' "$cr_pending_names" "$st_pending_names" | awk 'NF' | paste -sd, - | sed 's/,/, /g') + waiting=$(printf '%s\n%s' "$cr_pending_names" "$st_pending_names" | awk 'NF' | paste -sd, - | sed 's/,/, /g' | sanitize_for_log 1000) echo " pending: ${waiting:-}" fi if [ "$cr_cancelled" -gt 0 ]; then - echo " cancelled (waiting for concurrency replacement): ${cr_cancelled_detail}" + echo " cancelled (waiting for concurrency replacement): $(safe "$cr_cancelled_detail")" fi # Hold the "green" verdict until the settle floor. A first-poll "pending=0" @@ -209,5 +348,12 @@ for attempt in $(seq 1 "$max_attempts"); do sleep "$sleep_seconds" done -echo "::notice::Timed out waiting for other CI checks" +first_error_clause="" +[ -n "$first_error" ] && [ "$first_error" != "$last_error" ] \ + && first_error_clause="; first error: ${first_error}" +# ::error:: for the same reason as the consecutive-error bail: refusing to +# approve is a real outcome, a release cut may be blocking on the merge, and this +# is the path taken by errors that never hit max_consecutive_errors in a row (and +# the only reachable one when a caller sets wait-max-attempts below it). +echo "::error::Timed out waiting for other CI checks after ${max_attempts} attempts; refusing to approve. Last error: ${last_error:-none (checks were still pending)}${first_error_clause}" emit ci_green false diff --git a/.github/actions/auto-approve-bot-prs/test/gh_mock.bash b/.github/actions/auto-approve-bot-prs/test/gh_mock.bash index 96bd48b..fc7cb12 100644 --- a/.github/actions/auto-approve-bot-prs/test/gh_mock.bash +++ b/.github/actions/auto-approve-bot-prs/test/gh_mock.bash @@ -25,12 +25,24 @@ set -o pipefail # GH_MOCK_STATUSES_SEQ → same, for the commit-statuses endpoint. # GH_MOCK_CHECK_RUNS_FAIL → if 'always', /check-runs exits 1 every call. # GH_MOCK_STATUSES_FAIL → if 'always', /status exits 1 every call. +# GH_MOCK_STDERR → overrides the stderr text emitted on any forced or +# sequenced failure. Interpreted with printf '%b', +# so tests can inject \r, \n and literal % to +# exercise the log-injection sanitizer. The default +# text is deliberately unchanged so existing +# assertions keep working. # GH_MOCK_PR_MERGE_EXIT → exit code for `gh pr merge` # GH_MOCK_PR_MERGE_OUT → stdout for `gh pr merge` # GH_MOCK_CALLS → path; each invocation appends one line of args [ -n "${GH_MOCK_CALLS:-}" ] && printf '%s\n' "$*" >> "$GH_MOCK_CALLS" +# emit_err - stderr for a simulated failure, overridable so a +# test can supply realistic multiline gh output or injection payloads. +emit_err() { + printf '%b\n' "${GH_MOCK_STDERR:-$1}" >&2 +} + # Read the Nth line of a sequence file ($1) where N is tracked in $2 (counter # file incremented in place). When the sequence is exhausted, the caller falls # back to the static env var — returning empty stdout here signals "use fallback". @@ -59,13 +71,13 @@ emit_api_response() { ;; *"/check-runs"*) if [ "${GH_MOCK_CHECK_RUNS_FAIL:-}" = "always" ]; then - echo "mock: check-runs forced failure" >&2 + emit_err "mock: check-runs forced failure" exit 22 fi local seq seq="$(read_sequenced "${GH_MOCK_CHECK_RUNS_SEQ:-}" "${MOCK_DIR:-/tmp}/cr_n")" if [ "$seq" = "ERROR" ]; then - echo "mock: sequenced check-runs error" >&2 + emit_err "mock: sequenced check-runs error" exit 22 fi if [ -n "$seq" ]; then @@ -78,13 +90,13 @@ emit_api_response() { # Trailing /status (combined) — must come after /check-runs because # /check-runs also happens to contain "/runs/" but not "/status". if [ "${GH_MOCK_STATUSES_FAIL:-}" = "always" ]; then - echo "mock: statuses forced failure" >&2 + emit_err "mock: statuses forced failure" exit 22 fi local seq seq="$(read_sequenced "${GH_MOCK_STATUSES_SEQ:-}" "${MOCK_DIR:-/tmp}/st_n")" if [ "$seq" = "ERROR" ]; then - echo "mock: sequenced statuses error" >&2 + emit_err "mock: sequenced statuses error" exit 22 fi if [ -n "$seq" ]; then diff --git a/.github/actions/auto-approve-bot-prs/test/wait-for-ci.bats b/.github/actions/auto-approve-bot-prs/test/wait-for-ci.bats index 860b719..bac37c5 100644 --- a/.github/actions/auto-approve-bot-prs/test/wait-for-ci.bats +++ b/.github/actions/auto-approve-bot-prs/test/wait-for-ci.bats @@ -17,6 +17,25 @@ teardown() { rm -f "$GITHUB_OUTPUT"; teardown_gh_mock; } kv() { grep "^$1=" "$GITHUB_OUTPUT" | tail -n1; } +# assert_no_match — fail the test if the regex matches. +# +# Do NOT write `! grep -q ... <<<"$output"` instead. Bash does not abort on a +# command whose status is inverted with `!`, so under the `set -e` bats runs test +# bodies with, a bare negated grep is a no-op unless it happens to be the final +# line. Two assertions in this file were silently inert that way. A plain +# function call returning non-zero does abort, so this one actually fails. +assert_no_match() { + local rc=0 + grep -qP -- "$1" <<<"$2" || rc=$? + case "$rc" in + 0) printf 'assert_no_match: unexpected match for %s\n' "$1" >&2; return 1 ;; + 1) return 0 ;; + # grep returns 2 for a malformed pattern. Treating that as "no match" is the + # very fail-open shape this helper exists to prevent, so it must fail loudly. + *) printf 'assert_no_match: grep error rc=%s for pattern %s\n' "$rc" "$1" >&2; return 1 ;; + esac +} + @test "no check-runs → ci_green=true" { GH_MOCK_CHECK_RUNS_JSON='{"check_runs":[]}' run "$SCRIPT" [ "$status" -eq 0 ] @@ -294,6 +313,360 @@ kv() { grep "^$1=" "$GITHUB_OUTPUT" | tail -n1; } [ "$(kv ci_green)" = "ci_green=false" ] } +@test "regression: devops-1254 — check-runs API error surfaces gh's stderr, not just 'API failed'" { + # The v0.36.1 cut stalled ~70 min because this line said only "check-runs API + # failed". A permanent 403 (the CI-read token cannot reach the Checks API) and + # a transient blip produced byte-identical logs and identical default-deny + # exits, so the misconfiguration was undiagnosable from CI output. + export WAIT_MAX_ATTEMPTS=1 + export WAIT_MIN_ATTEMPTS=1 + GH_MOCK_CHECK_RUNS_FAIL=always run "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(kv ci_green)" = "ci_green=false" ] + [[ "$output" == *"check-runs API failed"* ]] + [[ "$output" == *"mock: check-runs forced failure"* ]] +} + +@test "regression: devops-1254 — statuses API error also surfaces gh's stderr" { + export WAIT_MAX_ATTEMPTS=1 + export WAIT_MIN_ATTEMPTS=1 + GH_MOCK_STATUSES_FAIL=always run "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(kv ci_green)" = "ci_green=false" ] + [[ "$output" == *"statuses API failed"* ]] + [[ "$output" == *"mock: statuses forced failure"* ]] +} + +@test "regression: devops-1254 — giving up is an ::error:: carrying the last error and the fix" { + # Escalated from ::notice::. The job keeps continue-on-error so this still + # cannot turn a caller's CI red, but a run that refused to approve must not + # read as clean — something downstream may be blocking on the merge. + export WAIT_MAX_ATTEMPTS=5 + export WAIT_MIN_ATTEMPTS=1 + GH_MOCK_CHECK_RUNS_FAIL=always run "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(kv ci_green)" = "ci_green=false" ] + [[ "$output" == *"::error::Too many consecutive API errors"* ]] + [[ "$output" == *"Last error: mock: check-runs forced failure"* ]] + # The actionable half: names the Checks API and the caller-permission trap. + [[ "$output" == *"Checks API"* ]] + [[ "$output" == *"checks: read"* ]] + [[ "$output" == *"CALLER"* ]] +} + +@test "regression: devops-1254 — a recovered poll clears the stale error text" { + export WAIT_MAX_ATTEMPTS=3 + export WAIT_MIN_ATTEMPTS=1 + seq_file="$(mktemp)" + printf 'ERROR\n{"check_runs":[]}\n' > "$seq_file" + GH_MOCK_CHECK_RUNS_SEQ="$seq_file" run "$SCRIPT" + rm -f "$seq_file" + [ "$status" -eq 0 ] + # Attempt 1 errors and says so; attempt 2 succeeds and reaches a verdict. + [[ "$output" == *"attempt 1/3: check-runs API failed"* ]] + [[ "$output" == *"sequenced check-runs error"* ]] + [ "$(kv ci_green)" = "ci_green=true" ] +} + +@test "regression: devops-1254 — a jq-parse failure must not report an earlier API error" { + # The bug this replaces a weaker test for: last_error was set only on API + # failure and never reset, so a later malformed-response poll inherited it and + # the bail told the operator to go check token permissions for a fault that had + # nothing to do with permissions. That is the exact misdiagnosis the error + # reporting exists to prevent, reintroduced one layer up. + # poll 1-2: API error (sets last_error) + # poll 3-5: API succeeds but returns garbage (jq parse failure) + export WAIT_MAX_ATTEMPTS=5 + export WAIT_MIN_ATTEMPTS=1 + seq_file="$(mktemp)" + printf 'ERROR\nERROR\n{not json\n{not json\n{not json\n' > "$seq_file" + GH_MOCK_CHECK_RUNS_SEQ="$seq_file" run "$SCRIPT" + rm -f "$seq_file" + [ "$status" -eq 0 ] + [ "$(kv ci_green)" = "ci_green=false" ] + [[ "$output" == *"check-runs jq parse failed"* ]] + # The bail must describe the parse failure, NOT the long-gone API error. + [[ "$output" == *"Last error: malformed check-runs response"* ]] + [[ "$output" != *"Last error: mock: sequenced check-runs error"* ]] +} + +@test "regression: devops-1254 — CR in api stderr cannot forge a workflow command" { + # CR terminates a log line for the runner, so raw \r in API error text would + # start a NEW line, and a line beginning '::' is a workflow command. Anything + # the sanitizer lets through here is a log-injection primitive. + export WAIT_MAX_ATTEMPTS=1 + export WAIT_MIN_ATTEMPTS=1 + GH_MOCK_CHECK_RUNS_FAIL=always \ + GH_MOCK_STDERR='gh: HTTP 403 nope\r::error::FORGED\r::set-output name=x::y 100%\rtail' \ + run "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(kv ci_green)" = "ci_green=false" ] + # THE guard. A line-anchored grep cannot fail on a CR-delimited payload, + # because grep splits on LF only while the runner also splits on CR — so an + # unsanitized payload would satisfy '^::error::' checks and the test would + # pass while the bug was live. Assert there is no CR in the output at all. + assert_no_match '\r' "$output" + # Belt and braces for the LF channel, which grep does see. + assert_no_match '(?m)^::error::FORGED' "$output" + # The text is still reported, flattened onto the one annotation line. + [[ "$output" == *"HTTP 403 nope"* ]] + [[ "$output" == *"tail"* ]] + # '%' is escaped so the runner cannot decode %0A/%25 out of API-controlled text. + [[ "$output" == *"100%25"* ]] +} + +@test "regression: devops-1254 — a check-run NAME cannot forge a workflow command" { + # The wider channel, and the one the first fix missed: check-run names and + # commit-status contexts are written by whoever posted the check on the head + # SHA, not by GitHub, and GitHub documents no character restriction on them. + # They reach the failed/cancelled/pending log lines. + export WAIT_MAX_ATTEMPTS=1 + export WAIT_MIN_ATTEMPTS=1 + GH_MOCK_CHECK_RUNS_JSON="$(printf '{"check_runs":[ + {"name":"evil\\r::error::INJECTED-VIA-NAME and %%0A%%25","status":"completed","conclusion":"failure","details_url":"https://github.com/o/r/actions/runs/222/job/1"} + ]}')" run "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(kv ci_green)" = "ci_green=false" ] + assert_no_match '\r' "$output" + assert_no_match '(?m)^::error::INJECTED' "$output" + # Escaped, not decoded, and the name is still reported for diagnosis. + [[ "$output" == *"%250A%2525"* ]] + [[ "$output" == *"evil"* ]] +} + +@test "regression: devops-1254 — a pending check NAME is sanitized too" { + export WAIT_MAX_ATTEMPTS=1 + export WAIT_MIN_ATTEMPTS=1 + GH_MOCK_CHECK_RUNS_JSON="$(printf '{"check_runs":[ + {"name":"wait\\r::warning::INJECTED-PENDING","status":"in_progress","conclusion":null,"details_url":"https://github.com/o/r/actions/runs/222/job/1"} + ]}')" run "$SCRIPT" + [ "$status" -eq 0 ] + assert_no_match '\r' "$output" + assert_no_match '(?m)^::warning::INJECTED' "$output" + [[ "$output" == *"pending:"* ]] + # Positive: the ascii part of the name must survive. Without this, a sanitizer + # that returned empty would yield "pending: " and still pass. + [[ "$output" == *"wait"* ]] +} + +@test "regression: devops-1254 — a cancelled check NAME is sanitized too" { + export WAIT_MAX_ATTEMPTS=2 + export WAIT_MIN_ATTEMPTS=1 + GH_MOCK_CHECK_RUNS_JSON="$(printf '{"check_runs":[ + {"name":"gone\\r::error::INJECTED-CANCELLED","status":"completed","conclusion":"cancelled","details_url":"https://github.com/o/r/actions/runs/222/job/1"} + ]}')" run "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(kv ci_green)" = "ci_green=false" ] + assert_no_match '\r' "$output" + assert_no_match '(?m)^::error::INJECTED' "$output" + # Positive: else an empty sanitizer result emits "Cancelled: " and still passes. + [[ "$output" == *"gone"* ]] +} + +@test "regression: devops-1254 — a commit-status CONTEXT cannot forge a workflow command" { + # The other half of the hostile channel the script's comment names. Both the + # check-run and status sides feed the same two log lines, but the NAME tests + # only exercise cr_*_detail. Dropping the sanitizer from the status side alone + # would ship an injection with a fully green suite. + export WAIT_MAX_ATTEMPTS=1 + export WAIT_MIN_ATTEMPTS=1 + GH_MOCK_CHECK_RUNS_JSON='{"check_runs":[]}' \ + GH_MOCK_STATUSES_JSON="$(printf '{"state":"failure","statuses":[ + {"context":"deploy\\r::error::FORGED-VIA-STATUS-CONTEXT","state":"failure","target_url":"https://netlify.example/x"} + ]}')" run "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(kv ci_green)" = "ci_green=false" ] + assert_no_match '\r' "$output" + assert_no_match '(?m)^::error::FORGED' "$output" + [[ "$output" == *"deploy"* ]] +} + +@test "regression: devops-1254 — a pending commit-status CONTEXT is sanitized too" { + export WAIT_MAX_ATTEMPTS=1 + export WAIT_MIN_ATTEMPTS=1 + GH_MOCK_CHECK_RUNS_JSON='{"check_runs":[]}' \ + GH_MOCK_STATUSES_JSON="$(printf '{"state":"pending","statuses":[ + {"context":"queue\\r::warning::FORGED-PENDING-CONTEXT","state":"pending","target_url":"https://netlify.example/x"} + ]}')" run "$SCRIPT" + [ "$status" -eq 0 ] + assert_no_match '\r' "$output" + assert_no_match '(?m)^::warning::FORGED' "$output" + [[ "$output" == *"queue"* ]] +} + +@test "regression: devops-1254 — a non-numeric wait input must not hard-fail the script" { + # `sleep "$sleep_seconds"` under set -e exited 1 with no ci_green at all, + # which for a direct consumer of the composite is a red job. Inputs are + # coerced now, so no caller value can break the exit-0 contract. + export WAIT_MAX_ATTEMPTS=1 + export WAIT_MIN_ATTEMPTS=1 + export WAIT_SLEEP_SECONDS="not-a-duration" + GH_MOCK_CHECK_RUNS_JSON='{"check_runs":[ + {"name":"e2e","status":"in_progress","conclusion":null,"details_url":"https://github.com/o/r/actions/runs/222/job/1"} + ]}' run "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(kv ci_green)" = "ci_green=false" ] + [[ "$output" == *"WAIT_SLEEP_SECONDS='not-a-duration' is not an integer in 1..3600"* ]] +} + +@test "regression: devops-1254 — the bail reports the FIRST error, not just the last" { + # Four real 403s then one malformed response used to report only the parse + # error, dropping the actionable fault from the summary. + export WAIT_MAX_ATTEMPTS=5 + export WAIT_MIN_ATTEMPTS=1 + seq_file="$(mktemp)" + printf 'ERROR\nERROR\nERROR\nERROR\n{not json\n' > "$seq_file" + GH_MOCK_CHECK_RUNS_SEQ="$seq_file" \ + GH_MOCK_STDERR='gh: Resource not accessible by integration (HTTP 403)' \ + run "$SCRIPT" + rm -f "$seq_file" + [ "$status" -eq 0 ] + [ "$(kv ci_green)" = "ci_green=false" ] + [[ "$output" == *"Last error: malformed check-runs response"* ]] + [[ "$output" == *"first error: gh: Resource not accessible by integration (HTTP 403)"* ]] +} + +@test "regression: devops-1254 — realistic multiline gh 403 is flattened to one line" { + export WAIT_MAX_ATTEMPTS=1 + export WAIT_MIN_ATTEMPTS=1 + GH_MOCK_CHECK_RUNS_FAIL=always \ + GH_MOCK_STDERR='gh: Resource not accessible by personal access token (HTTP 403)\n{"message":"Resource not accessible"}' \ + run "$SCRIPT" + [ "$status" -eq 0 ] + [[ "$output" == *"Resource not accessible by personal access token (HTTP 403)"* ]] + # Exactly one warning line for the attempt, not one per stderr line. + [ "$(grep -c '::warning::attempt 1/1: check-runs API failed' <<<"$output")" -eq 1 ] +} + +@test "regression: devops-1254 — non-ascii stderr does not break the length cap" { + # cut -c is byte-based in a C locale, so a naive cap can split a UTF-8 + # sequence and emit invalid bytes into the annotation. + export WAIT_MAX_ATTEMPTS=1 + export WAIT_MIN_ATTEMPTS=1 + long="$(printf 'é%.0s' $(seq 1 400))" + GH_MOCK_CHECK_RUNS_FAIL=always GH_MOCK_STDERR="gh: 403 ${long}" run "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(kv ci_green)" = "ci_green=false" ] + [[ "$output" == *"check-runs API failed"* ]] + # Positive assertion first: without it, "no high bytes in the output" is + # trivially satisfied by an implementation that discards stderr entirely, so + # the test would pass against the very code it is meant to guard. + [[ "$output" == *"gh: 403"* ]] + # Non-ascii is dropped rather than truncated mid-character. + assert_no_match '[\x80-\xff]' "$output" +} + +@test "regression: devops-1254 — unusable TMPDIR must not hard-fail the script" { + # action.yml promises the job never hard-fails, and a direct consumer of the + # composite has no continue-on-error to hide behind. A bare mktemp assignment + # under `set -e` exited 1 with no ci_green emitted at all. + export WAIT_MAX_ATTEMPTS=1 + export WAIT_MIN_ATTEMPTS=1 + TMPDIR=/proc/nonexistent run "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(kv ci_green)" = "ci_green=true" ] + [[ "$output" == *"could not allocate a temp file"* ]] +} + +@test "regression: devops-1254 — first_error resets when the error streak resolves" { + # Poll 1 errors, poll 2 succeeds, polls 3-7 fail to parse. The bail must not + # attribute the streak to the long-resolved 403 from poll 1 — that is the F1 + # misattribution bug one variable over. + export WAIT_MAX_ATTEMPTS=7 + export WAIT_MIN_ATTEMPTS=6 + seq_file="$(mktemp)" + printf 'ERROR\n{"check_runs":[]}\n{bad\n{bad\n{bad\n{bad\n{bad\n' > "$seq_file" + GH_MOCK_CHECK_RUNS_SEQ="$seq_file" \ + GH_MOCK_STDERR='gh: Resource not accessible by integration (HTTP 403)' \ + run "$SCRIPT" + rm -f "$seq_file" + [ "$status" -eq 0 ] + [ "$(kv ci_green)" = "ci_green=false" ] + [[ "$output" == *"Last error: malformed check-runs response"* ]] + assert_no_match 'first error: gh: Resource not accessible' "$output" +} + +@test "regression: devops-1254 — an out-of-range attempt count cannot hang the job" { + # A valid but enormous integer used to be accepted (the boundary was int64 + # overflow in [ -gt ]), and `for attempt in $(seq 1 N)` then had to build the + # whole list before the first poll: no output, no ci_green, hang until the + # 6-hour job timeout. Strictly worse than the stall this script prevents. + export WAIT_MAX_ATTEMPTS=9223372036854775807 + export WAIT_MIN_ATTEMPTS=1 + GH_MOCK_CHECK_RUNS_JSON='{"check_runs":[]}' run timeout 60 "$SCRIPT" + [ "$status" -eq 0 ] + [[ "$output" == *"WAIT_MAX_ATTEMPTS='9223372036854775807' is not an integer in 1..100000"* ]] + [ "$(kv ci_green)" = "ci_green=true" ] +} + +@test "regression: devops-1254 — a non-numeric WAIT_MAX_ATTEMPTS is coerced, not fatal" { + export WAIT_MAX_ATTEMPTS="lots" + export WAIT_MIN_ATTEMPTS=1 + GH_MOCK_CHECK_RUNS_JSON='{"check_runs":[]}' run "$SCRIPT" + [ "$status" -eq 0 ] + [[ "$output" == *"WAIT_MAX_ATTEMPTS='lots' is not an integer in 1..100000"* ]] + [ "$(kv ci_green)" = "ci_green=true" ] +} + +@test "regression: devops-1254 — a non-numeric WAIT_MIN_ATTEMPTS is coerced, not fatal" { + export WAIT_MAX_ATTEMPTS=1 + export WAIT_MIN_ATTEMPTS="soon" + GH_MOCK_CHECK_RUNS_JSON='{"check_runs":[]}' run "$SCRIPT" + [ "$status" -eq 0 ] + [[ "$output" == *"WAIT_MIN_ATTEMPTS='soon' is not an integer in 1..100000"* ]] +} + +@test "regression: devops-1254 — a plausible fat-finger attempt count is also rejected" { + export WAIT_MAX_ATTEMPTS=1000000000 + export WAIT_MIN_ATTEMPTS=1 + GH_MOCK_CHECK_RUNS_JSON='{"check_runs":[]}' run timeout 60 "$SCRIPT" + [ "$status" -eq 0 ] + [[ "$output" == *"is not an integer in 1..100000"* ]] +} + +@test "regression: devops-1254 — a rejected wait value cannot forge a workflow command" { + export WAIT_SLEEP_SECONDS='x\r::error::FORGED-VIA-WAIT-INPUT' + export WAIT_MAX_ATTEMPTS=1 + export WAIT_MIN_ATTEMPTS=1 + GH_MOCK_CHECK_RUNS_JSON='{"check_runs":[]}' run "$SCRIPT" + [ "$status" -eq 0 ] + assert_no_match '\r' "$output" + assert_no_match '(?m)^::error::FORGED' "$output" +} + +@test "regression: devops-1254 — a long pending list is truncated with a marker, not severed" { + # The 300-char cap is right for one hostile string and wrong for a legitimately + # long list; this is the line the script's own comment calls out as how an + # operator answers "why is this still running?". + export WAIT_MAX_ATTEMPTS=1 + export WAIT_MIN_ATTEMPTS=1 + json='{"check_runs":[' + for i in $(seq 1 20); do + [ "$i" -gt 1 ] && json="${json}," + json="${json}{\"name\":\"a-fairly-long-check-run-name-number-$(printf '%03d' "$i")\",\"status\":\"in_progress\",\"conclusion\":null,\"details_url\":\"https://github.com/o/r/actions/runs/2$i/job/1\"}" + done + json="${json}]}" + GH_MOCK_CHECK_RUNS_JSON="$json" run "$SCRIPT" + [ "$status" -eq 0 ] + # All 20 survive at the wider list cap, so the diagnostic is actually useful. + [[ "$output" == *"number-001"* ]] + [[ "$output" == *"number-020"* ]] +} + +@test "regression: devops-1254 — timing out is an ::error::, not a quiet notice" { + # Sibling of the consecutive-error bail: a permanently-pending check, or errors + # that never hit 5 in a row, exit here. A release cut blocking on the merge + # deserves the same visibility. + export WAIT_MAX_ATTEMPTS=2 + export WAIT_MIN_ATTEMPTS=1 + GH_MOCK_CHECK_RUNS_JSON='{"check_runs":[ + {"name":"e2e","status":"in_progress","conclusion":null,"details_url":"https://github.com/o/r/actions/runs/222/job/1"} + ]}' run "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(kv ci_green)" = "ci_green=false" ] + [[ "$output" == *"::error::Timed out waiting for other CI checks"* ]] +} + @test "default-deny: transient API errors do not count toward the settle floor" { # On the pre-TDD script an errored poll silently fell back to '[]' and still # counted as a settle attempt — with min_attempts=2 it would approve after diff --git a/.github/workflows/auto-approve-bot-prs.yaml b/.github/workflows/auto-approve-bot-prs.yaml index 8f1d2ef..421e328 100644 --- a/.github/workflows/auto-approve-bot-prs.yaml +++ b/.github/workflows/auto-approve-bot-prs.yaml @@ -31,15 +31,27 @@ on: gh-access-token: description: 'GitHub PAT for approving PRs (must be different identity from PR author)' required: true + ci-read-token: + description: 'Optional escape hatch for the read-only CI poll when the caller cannot grant `checks: read` / `statuses: read`. Classic PAT with `repo` scope, or an App token. Never gh-access-token.' + required: false jobs: auto-approve: runs-on: ubuntu-latest # Skip fork PRs — secrets are not available. if: github.event.pull_request.head.repo.full_name == github.repository + # checks/statuses are what the CI poll reads via GITHUB_TOKEN. These lines + # alone are NOT enough: "the GITHUB_TOKEN permissions passed from the caller + # workflow can be only downgraded (not elevated) by the called workflow", and + # anything the caller omits defaults to none. So unless the caller passes the + # ci-read-token secret, it MUST declare `checks: read` and `statuses: read` + # itself, or the poll 403s and the action default-denies without ever + # approving. See DEVOPS-1254. permissions: pull-requests: write contents: read + checks: read + statuses: read # Safety net — internal steps already guard every failure mode, but if # anything unforeseen slips through this still prevents the job from # reporting a hard red check on caller CI. @@ -60,3 +72,6 @@ jobs: wait-min-attempts: ${{ inputs.wait-min-attempts }} wait-sleep-seconds: ${{ inputs.wait-sleep-seconds }} github-token: ${{ secrets.gh-access-token }} # zizmor: ignore[secrets-outside-env] -- PAT passed via workflow_call, not a repo secret + # Empty when the caller does not set it, which falls back to + # github.token inside the composite. + ci-read-token: ${{ secrets.ci-read-token }} # zizmor: ignore[secrets-outside-env] -- optional token passed via workflow_call diff --git a/.github/workflows/test-auto-approve-bot-prs.yaml b/.github/workflows/test-auto-approve-bot-prs.yaml index 6c94ad6..6e69a2c 100644 --- a/.github/workflows/test-auto-approve-bot-prs.yaml +++ b/.github/workflows/test-auto-approve-bot-prs.yaml @@ -27,9 +27,15 @@ jobs: # job always reports success. composite-smoke: runs-on: ubuntu-latest + # Mirrors the permission set a real caller must declare, so this file is not + # a copy-paste source for a broken caller. It does NOT verify the CI poll: + # as the comment above says, human-authored PRs stop at the trusted-author + # check, so the poll step never runs here. The bats suite is what covers it. permissions: contents: read pull-requests: write + checks: read + statuses: read steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: diff --git a/README.md b/README.md index b1ad85a..0071508 100644 --- a/README.md +++ b/README.md @@ -507,6 +507,8 @@ jobs: permissions: pull-requests: write contents: read + checks: read # required — CI poll reads /commits/:sha/check-runs + statuses: read # required — CI poll reads /commits/:sha/status uses: loft-sh/github-actions/.github/workflows/auto-approve-bot-prs.yaml@main with: trusted-authors: 'renovate[bot],loft-bot,github-actions[bot],dependabot[bot]' @@ -519,6 +521,16 @@ jobs: to auto-approve (GitHub forbids self-review). When identity matches, the job skips gracefully instead of failing. +**Unless you pass the `ci-read-token` secret, `checks: read` and `statuses: read` +are not optional, and the called workflow cannot supply them for you** — GitHub only lets a reusable +workflow downgrade the caller's `GITHUB_TOKEN` permissions, never elevate them, +and anything you omit defaults to `none`. Omit them and the CI poll 403s, the +action default-denies, and the PR is never approved while both the check and the +job still report success. See +[`auto-approve-bot-prs/README.md`](.github/actions/auto-approve-bot-prs/README.md) +for why the approving PAT cannot be used for these reads (fine-grained PATs have +no Checks permission at all). + **End-to-end coverage:** scenario-level e2e lives in [vClusterLabs-Experiments/auto-approve-e2e](https://github.com/vClusterLabs-Experiments/auto-approve-e2e). Runs weekly and on demand. Creates real PRs exercising every decision-table diff --git a/docs/workflows/auto-approve-bot-prs.md b/docs/workflows/auto-approve-bot-prs.md index 546f4ee..bf5faef 100644 --- a/docs/workflows/auto-approve-bot-prs.md +++ b/docs/workflows/auto-approve-bot-prs.md @@ -2,7 +2,9 @@ Reusable workflow that approves PRs from trusted bot accounts whose title or branch matches a known safe pattern. Wraps the composite action -of the same name with GitHub App token minting and sparse checkout. +of the same name, adding a sparse checkout of this repo. It does not mint any +token: the approving PAT is supplied by the caller as the `gh-access-token` +secret. ## Inputs @@ -23,8 +25,44 @@ of the same name with GitHub App token minting and sparse checkout. -| SECRET | REQUIRED | DESCRIPTION | -|-----------------|----------|---------------------------------------------------------------------------| -| gh-access-token | true | GitHub PAT for approving PRs (must be different identity from PR author) | +| SECRET | REQUIRED | DESCRIPTION | +|-----------------|----------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| ci-read-token | false | Optional escape hatch for the read-only
CI poll when the caller cannot
grant `checks: read` / `statuses: read`. Classic PAT
with `repo` scope, or an App
token. Never gh-access-token. | +| gh-access-token | true | GitHub PAT for approving PRs (must be different identity from PR author) | + +## Required caller permissions + +Unless the caller supplies the optional `ci-read-token` secret, the calling job +must declare all four: + +```yaml +jobs: + auto-approve: + permissions: + contents: read + pull-requests: write + checks: read # CI poll: /commits/:sha/check-runs + statuses: read # CI poll: /commits/:sha/status + uses: loft-sh/github-actions/.github/workflows/auto-approve-bot-prs.yaml@auto-approve-bot-prs/v1 + secrets: + gh-access-token: ${{ secrets.GH_ACCESS_TOKEN }} +``` + +`checks` and `statuses` cannot be supplied by this workflow on your behalf. Per +GitHub, *"the `GITHUB_TOKEN` permissions passed from the caller workflow can be +only downgraded (not elevated) by the called workflow"*, and any permission the +caller omits defaults to `none`. + +Omitting them does not fail loudly. The CI poll gets a 403, the action +default-denies, and the PR is never approved, while the check and the job both +still report success. If a release cut is waiting on that merge it will wait out +its full timeout. Look for `check-runs API failed` in the job log. + +A caller with **no** `permissions:` block at all inherits the repository default +and is unaffected, provided that default is not restricted. + +The approving PAT cannot be reused for the poll: fine-grained PATs cannot call +the Checks API at all. `ci-read-token` exists only for callers that cannot grant +the permissions and need to pass a classic PAT or App token instead.