From 2df73ec3ea51624186fce00aa1f89fcc5b1c8720 Mon Sep 17 00:00:00 2001 From: Dmytro Sydorov Date: Mon, 3 Aug 2026 10:46:14 +0200 Subject: [PATCH 1/3] fix(auto-approve): poll ci state with github_token, not the approving pat The v0.36.1 release cut stalled ~70 min and was cancelled by hand. The bump PR had every check green and was mergeable, but was never approved: wait-for-ci.sh failed its first check-runs call 5 times and default-denied. BOT_APPROVER_PAT is a fine-grained PAT, and fine-grained PATs cannot call the Checks API at all. There is no Checks permission to grant, so this was never a token-configuration fix. Reading CI state needs no distinct identity though; only the approval does, because GitHub forbids self-approval. Split the two: - new ci-read-token input, used as GH_TOKEN on the CI-wait step only, falling back to github.token. The PAT stays on check-pr-ready, approve, auto-merge. - grant checks: read and statuses: read in the reusable workflow, and expose ci-read-token as an optional secret so a caller that cannot grant those can pass a classic PAT or App token instead. Callers must grant checks: read and statuses: read themselves: a reusable workflow can only downgrade the caller's permissions, never elevate them, so the workflow-side grant is inert on its own. Documented in both READMEs, the generated workflow doc, and mirrored into the composite smoke test. Diagnosability, the reason a 70-minute stall was the first symptom: surface gh's stderr instead of discarding it, and escalate both give-up paths from ::notice:: to ::error::. Neither changes the exit code, so the never-hard-fail contract holds. That error text is API-controlled and goes into a workflow command, so it is sanitized rather than trusted: CR as well as LF is collapsed (CR terminates a log line for the runner, so a raw one could forge a ::error:: line), non-ASCII is dropped so the length cap cannot split a UTF-8 sequence, and % is escaped so %0A/%25 cannot be decoded out of it. Failure paths that must not become new failures: last_error is reset per poll so a malformed-response poll cannot report an earlier API error and send the operator after token permissions; mktemp failure degrades to 'capture unavailable' rather than exiting 1 under set -e; and the error reader cannot abort the script through pipefail. Closes DEVOPS-1254 --- .../actions/auto-approve-bot-prs/README.md | 88 +++++++++-- .../actions/auto-approve-bot-prs/action.yml | 23 ++- .../auto-approve-bot-prs/src/wait-for-ci.sh | 84 +++++++++- .../auto-approve-bot-prs/test/gh_mock.bash | 20 ++- .../test/wait-for-ci.bats | 149 ++++++++++++++++++ .github/workflows/auto-approve-bot-prs.yaml | 20 +++ .../workflows/test-auto-approve-bot-prs.yaml | 6 + README.md | 12 ++ docs/workflows/auto-approve-bot-prs.md | 41 ++++- 9 files changed, 415 insertions(+), 28 deletions(-) diff --git a/.github/actions/auto-approve-bot-prs/README.md b/.github/actions/auto-approve-bot-prs/README.md index e39ec31..0e9d484 100644 --- a/.github/actions/auto-approve-bot-prs/README.md +++ b/.github/actions/auto-approve-bot-prs/README.md @@ -2,7 +2,12 @@ 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. +every failure mode 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 +34,83 @@ 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 polling
(check-runs + commit statuses) only. Defaults to the caller's
GITHUB_TOKEN, which is what you want:
reading CI state needs no distinct
identity, and only the approval does,
because GitHub forbids self-approval. Do NOT
point this at the approving PAT
on a private repository: fine-grained PATs
cannot call the Checks API at
all (there is no Checks permission to grant), so polling would fail
every time and the action would
default-deny forever. The CALLER workflow must
grant `checks: read` and `statuses: read`. | +| 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 on a private repository. +**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 this misconfiguration is invisible on a **public** repository, where +`/check-runs` answers with no credentials at all. Working there proves nothing +about a private caller. + +## Required caller permissions + +The caller workflow must grant these. 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: + 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..d236e8d 100644 --- a/.github/actions/auto-approve-bot-prs/action.yml +++ b/.github/actions/auto-approve-bot-prs/action.yml @@ -2,7 +2,9 @@ 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. + every failure mode degrades to an annotated skip and exit 0. Refusing to + approve is annotated at error level (it can block a release cut waiting on + the merge); that raises an annotation only and never a non-zero exit. inputs: trusted-authors: description: 'Comma-separated list of trusted bot logins' @@ -19,6 +21,17 @@ 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 polling (check-runs + commit statuses) only. + Defaults to the caller's GITHUB_TOKEN, which is what you want: reading CI + state needs no distinct identity, and only the approval does, because + GitHub forbids self-approval. Do NOT point this at the approving PAT on a + private repository: fine-grained PATs cannot call the Checks API at all + (there is no Checks permission to grant), so polling would fail every time + and the action would default-deny forever. + The CALLER workflow must grant `checks: read` and `statuses: read`. + required: false wait-max-attempts: description: 'Max polling attempts waiting for other CI checks' required: false @@ -60,7 +73,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..9e4c4e1 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 @@ -41,17 +41,65 @@ 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 likewise not inherited by that subshell, so it fires only +# once, on the real exit — the file is not deleted out from under the caller.) +# +# 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" } +# 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 on a private repo, +# which means the CI-read token cannot reach the Checks API. +# +# This text is attacker-adjacent: it is API-controlled and goes straight into a +# ::warning::/::error:: line, so it is sanitized rather than trusted. +# - CR and LF both terminate a log line for the runner, so a raw one in the +# error text would start a NEW line, and a line beginning `::` is a workflow +# command. Collapse both to spaces. +# - 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` in +# the source would otherwise be decoded into the annotation. Escape it last, +# after the cut, so the cap cannot bisect an escape we just wrote. +# 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. +gh_last_error() { + [ -n "$GH_ERR_FILE" ] || return 0 + [ -s "$GH_ERR_FILE" ] || return 0 + { LC_ALL=C tr '\n\r\t' ' ' < "$GH_ERR_FILE" \ + | LC_ALL=C tr -cd '\040-\176' \ + | cut -c1-300 \ + | sed 's/%/%25/g'; } 2>/dev/null || true +} + # jq_or_fail [jq-flags...] — run jq on $json with optional # flags (e.g. -r). Returns non-zero on parse failure. Callers must check # exit status; silent empty output here is not the same as success. @@ -68,14 +116,23 @@ 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="" for attempt in $(seq 1 "$max_attempts"); do 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 +154,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 +184,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 +196,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,11 +212,19 @@ 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" # 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" + # ::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}" + 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 @@ -209,5 +277,9 @@ for attempt in $(seq 1 "$max_attempts"); do sleep "$sleep_seconds" done -echo "::notice::Timed out waiting for other CI checks" +# ::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)}" 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..bda36ed 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 @@ -294,6 +294,155 @@ 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" ] + # No forged command may appear at the start of any line. + ! grep -qE '^::(error|set-output|warning)::(FORGED|name=x)' <<<"$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 — 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"* ]] + # Non-ascii is dropped rather than truncated mid-character. + ! grep -qP '[\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 — 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..b815c27 100644 --- a/.github/workflows/auto-approve-bot-prs.yaml +++ b/.github/workflows/auto-approve-bot-prs.yaml @@ -31,15 +31,32 @@ on: gh-access-token: description: 'GitHub PAT for approving PRs (must be different identity from PR author)' required: true + ci-read-token: + description: | + Optional. Token for the read-only CI poll, overriding the default of + the caller's GITHUB_TOKEN. Only needed as an escape hatch when the + caller cannot grant `checks: read` / `statuses: read` (see the job + permissions below); pass a classic PAT with `repo` scope or a GitHub + App token, both of which can reach the Checks API. A fine-grained PAT + cannot, so do NOT pass gh-access-token here. + 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. Every caller MUST declare + # `checks: read` and `statuses: read` too, or the poll 403s on a private repo + # 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 +77,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..43e02e1 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. +**`checks: read` and `statuses: read` are not optional on a private repo, 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..e987da3 100644 --- a/docs/workflows/auto-approve-bot-prs.md +++ b/docs/workflows/auto-approve-bot-prs.md @@ -23,8 +23,43 @@ 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. Token for the read-only CI
poll, overriding the default of the
caller's GITHUB_TOKEN. Only needed as an
escape hatch when the caller cannot
grant `checks: read` / `statuses: read` (see the
job permissions below); pass a classic
PAT with `repo` scope or a
GitHub App token, both of which
can reach the Checks API. A
fine-grained PAT cannot, so do NOT
pass gh-access-token here. | +| gh-access-token | true | GitHub PAT for approving PRs (must be different identity from PR author) | + +## Required caller permissions + +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. From fcb60d91a91ccdc87c52da8738b2e1c957e69fd0 Mon Sep 17 00:00:00 2001 From: Dmytro Sydorov Date: Mon, 3 Aug 2026 12:12:41 +0200 Subject: [PATCH 2/3] fix(auto-approve): sanitize check names too, and coerce the wait inputs Second adversarial review pass on the commit below. The sanitizer covered only gh's stderr. Check-run names and commit-status contexts are written by whoever posted the check, GitHub documents no character restriction on them, and they reached the failed/cancelled/pending log lines raw - so they were the wider injection channel, and the one left open. A CR in a check name forged an ::error:: line. Extracted sanitize_for_log and applied it to both channels. A non-numeric wait-* input reached sleep/seq and aborted under set -e with no ci_green emitted at all, which is a red job for a direct consumer of the composite (the documented usage has no continue-on-error). Inputs are coerced with a warning, and the contract wording now says precisely what cannot exit non-zero rather than claiming every failure mode. The bail reported only the last error, so four real 403s followed by one malformed response hid the actionable fault; it now carries the first as well. The length cap is applied after escaping too, so the emitted line is really bounded instead of growing threefold. Negative test assertions written as bare '! grep' were inert: bash does not abort on a !-inverted command under set -e, so they only had effect as a test's final line. Two were silently powerless. Replaced with assert_no_match, which does fail. All 15 DEVOPS-1254 tests are now verified failing against main. Docs: the caller permissions are no longer framed as private-repo-only (GITHUB_TOKEN is scoped by the permissions block whatever the repo's visibility), no longer stated unconditionally now that ci-read-token can satisfy them instead, and the reusable workflow no longer claims to mint an App token. --- .../actions/auto-approve-bot-prs/README.md | 32 +++--- .../actions/auto-approve-bot-prs/action.yml | 22 ++-- .../auto-approve-bot-prs/src/wait-for-ci.sh | 105 +++++++++++++----- .../test/wait-for-ci.bats | 104 ++++++++++++++++- README.md | 4 +- docs/workflows/auto-approve-bot-prs.md | 7 +- 6 files changed, 214 insertions(+), 60 deletions(-) diff --git a/.github/actions/auto-approve-bot-prs/README.md b/.github/actions/auto-approve-bot-prs/README.md index 0e9d484..38cd60a 100644 --- a/.github/actions/auto-approve-bot-prs/README.md +++ b/.github/actions/auto-approve-bot-prs/README.md @@ -1,8 +1,9 @@ # 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 an annotated skip and exit 0. +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 @@ -34,16 +35,16 @@ slow external checks have not shown up yet. -| 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 polling
(check-runs + commit statuses) only. Defaults to the caller's
GITHUB_TOKEN, which is what you want:
reading CI state needs no distinct
identity, and only the approval does,
because GitHub forbids self-approval. Do NOT
point this at the approving PAT
on a private repository: fine-grained PATs
cannot call the Checks API at
all (there is no Checks permission to grant), so polling would fail
every time and the action would
default-deny forever. The CALLER workflow must
grant `checks: read` and `statuses: read`. | -| 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 polling
(check-runs + commit statuses) only. Defaults to the caller's
GITHUB_TOKEN, which is what you want:
reading CI state needs no distinct
identity, and only the approval does,
because GitHub forbids self-approval. Do NOT
point this at the approving PAT:
fine-grained PATs cannot call the Checks
API at all (there is no
Checks permission to grant), so polling
would fail every time and the
action would default-deny forever. Unless you
set this, the CALLER workflow must
grant `checks: read` and `statuses: read` (a reusable workflow cannot add them for you). Supplying
a token here bypasses github.token for
the poll, so those grants are
then not needed. | +| 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 | @@ -58,7 +59,7 @@ are split: | `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 on a private repository. +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 @@ -71,8 +72,8 @@ about a private caller. ## Required caller permissions -The caller workflow must grant these. They cannot be added by this action or by -the reusable workflow that wraps it: per GitHub, *"the `GITHUB_TOKEN` 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`. @@ -95,6 +96,7 @@ this snippet without it reproduces the silent no-approve failure described above ```yaml jobs: auto-approve: + runs-on: ubuntu-latest permissions: contents: read pull-requests: write diff --git a/.github/actions/auto-approve-bot-prs/action.yml b/.github/actions/auto-approve-bot-prs/action.yml index d236e8d..16fa52c 100644 --- a/.github/actions/auto-approve-bot-prs/action.yml +++ b/.github/actions/auto-approve-bot-prs/action.yml @@ -1,10 +1,11 @@ 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 an annotated skip and exit 0. Refusing to - approve is annotated at error level (it can block a release cut waiting on - the merge); that raises an annotation only and never a non-zero exit. + 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 (it can block a + release cut waiting on the merge); that raises an annotation only and never a + non-zero exit. inputs: trusted-authors: description: 'Comma-separated list of trusted bot logins' @@ -26,11 +27,14 @@ inputs: Token for the read-only CI polling (check-runs + commit statuses) only. Defaults to the caller's GITHUB_TOKEN, which is what you want: reading CI state needs no distinct identity, and only the approval does, because - GitHub forbids self-approval. Do NOT point this at the approving PAT on a - private repository: fine-grained PATs cannot call the Checks API at all - (there is no Checks permission to grant), so polling would fail every time - and the action would default-deny forever. - The CALLER workflow must grant `checks: read` and `statuses: read`. + GitHub forbids self-approval. Do NOT point this at the approving PAT: + fine-grained PATs cannot call the Checks API at all (there is no Checks + permission to grant), so polling would fail every time and the action would + default-deny forever. + Unless you set this, the CALLER workflow must grant `checks: read` and + `statuses: read` (a reusable workflow cannot add them for you). Supplying + a token here bypasses github.token for the poll, so those grants are then + not needed. required: false wait-max-attempts: description: 'Max polling attempts waiting for other CI checks' 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 9e4c4e1..2216279 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,23 @@ 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}" +# 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. +numeric_or_default() { + local name="$1" value="$2" fallback="$3" + if [ -n "$value" ] && [ -z "${value//[0-9]/}" ] && [ "$value" -gt 0 ] 2>/dev/null; then + printf '%s' "$value" + return 0 + fi + echo "::warning::${name}='${value}' is not a positive integer; using ${fallback}" >&2 + printf '%s' "$fallback" +} +max_attempts="$(numeric_or_default WAIT_MAX_ATTEMPTS "${WAIT_MAX_ATTEMPTS:-90}" 90)" +min_attempts="$(numeric_or_default WAIT_MIN_ATTEMPTS "${WAIT_MIN_ATTEMPTS:-12}" 12)" +sleep_seconds="$(numeric_or_default WAIT_SLEEP_SECONDS "${WAIT_SLEEP_SECONDS:-10}" 10)" emit() { local k="$1" v="$2" @@ -44,8 +58,9 @@ emit() { # 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 likewise not inherited by that subshell, so it fires only -# once, on the real exit — the file is not deleted out from under the caller.) +# 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 @@ -72,32 +87,58 @@ gh_json() { printf '%s' "$body" } -# 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 on a private repo, -# which means the CI-read token cannot reach the Checks API. +# 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. # -# This text is attacker-adjacent: it is API-controlled and goes straight into a -# ::warning::/::error:: line, so it is sanitized rather than trusted. -# - CR and LF both terminate a log line for the runner, so a raw one in the -# error text would start a NEW line, and a line beginning `::` is a workflow -# command. Collapse both to spaces. +# 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` in -# the source would otherwise be decoded into the annotation. Escape it last, -# after the cut, so the cap cannot bisect an escape we just wrote. +# - `%` 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. +sanitize_for_log() { + { LC_ALL=C tr '\n\r\t' ' ' \ + | LC_ALL=C tr -cd '\040-\176' \ + | cut -c1-300 \ + | sed 's/%/%25/g' \ + | cut -c1-400 \ + | 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 - { LC_ALL=C tr '\n\r\t' ' ' < "$GH_ERR_FILE" \ - | LC_ALL=C tr -cd '\040-\176' \ - | cut -c1-300 \ - | sed 's/%/%25/g'; } 2>/dev/null || true + 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 } # jq_or_fail [jq-flags...] — run jq on $json with optional @@ -119,6 +160,10 @@ 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 poll_errored=0 @@ -215,6 +260,7 @@ for attempt in $(seq 1 "$max_attempts"); do # 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 )) @@ -223,7 +269,7 @@ for attempt in $(seq 1 "$max_attempts"); do # 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}" + echo "::error::Too many consecutive API errors (${consecutive_errors}); refusing to approve. Last error: ${last_error:-unknown}${first_error:+; first error: ${first_error}}" 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 @@ -240,7 +286,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) echo "::notice::Other CI checks failed; skipping approval. Failing: ${details:-unknown}" emit ci_green false exit 0 @@ -250,7 +297,7 @@ 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}" + echo "::notice::Cancelled checks did not get replaced within settle period; skipping approval. Cancelled: $(safe "${cr_cancelled_detail:-unknown}")" emit ci_green false exit 0 fi @@ -258,11 +305,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) 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" @@ -281,5 +328,5 @@ done # 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)}" +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:+; first error: ${first_error}}" emit ci_green false 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 bda36ed..bd1bb82 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,20 @@ 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() { + if grep -qP -- "$1" <<<"$2"; then + printf 'assert_no_match: unexpected match for %s\n' "$1" >&2 + return 1 + fi +} + @test "no check-runs → ci_green=true" { GH_MOCK_CHECK_RUNS_JSON='{"check_runs":[]}' run "$SCRIPT" [ "$status" -eq 0 ] @@ -382,8 +396,13 @@ kv() { grep "^$1=" "$GITHUB_OUTPUT" | tail -n1; } run "$SCRIPT" [ "$status" -eq 0 ] [ "$(kv ci_green)" = "ci_green=false" ] - # No forged command may appear at the start of any line. - ! grep -qE '^::(error|set-output|warning)::(FORGED|name=x)' <<<"$output" + # 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"* ]] @@ -391,6 +410,81 @@ kv() { grep "^$1=" "$GITHUB_OUTPUT" | tail -n1; } [[ "$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:"* ]] +} + +@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" +} + +@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 a positive integer"* ]] +} + +@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 @@ -413,8 +507,12 @@ kv() { grep "^$1=" "$GITHUB_OUTPUT" | tail -n1; } [ "$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. - ! grep -qP '[\x80-\xff]' <<<"$output" + assert_no_match '[\x80-\xff]' "$output" } @test "regression: devops-1254 — unusable TMPDIR must not hard-fail the script" { diff --git a/README.md b/README.md index 43e02e1..518ce13 100644 --- a/README.md +++ b/README.md @@ -521,8 +521,8 @@ jobs: to auto-approve (GitHub forbids self-review). When identity matches, the job skips gracefully instead of failing. -**`checks: read` and `statuses: read` are not optional on a private repo, and the -called workflow cannot supply them for you** — GitHub only lets a reusable +**`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 diff --git a/docs/workflows/auto-approve-bot-prs.md b/docs/workflows/auto-approve-bot-prs.md index e987da3..561f466 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 @@ -32,7 +34,8 @@ of the same name with GitHub App token minting and sparse checkout. ## Required caller permissions -The calling job must declare all four: +Unless the caller supplies the optional `ci-read-token` secret, the calling job +must declare all four: ```yaml jobs: From 20222c59d5c74977db82aa4a4f581839fd1a1e39 Mon Sep 17 00:00:00 2001 From: Dmytro Sydorov Date: Mon, 3 Aug 2026 12:49:09 +0200 Subject: [PATCH 3/3] fix(auto-approve): bound the wait inputs, reset first_error, close test gaps Round-3 adversarial review plus the AI reviewer's comments on the PR. Two defects that belong here rather than in a follow-up: - numeric_or_default had no upper bound, so the accept/reject line was an accident of int64 overflow in [ -gt ]. A plausible fat-finger like 1000000000 was accepted, and `for attempt in $(seq 1 N)` then had to materialise the whole list before the first poll: no output, no ci_green, a hang until GitHub's 6-hour timeout. That is a strictly worse version of the stall this PR exists to fix. Inputs are now range-checked (length first, so overflow never decides) and the loop is arithmetic, so nothing can be materialised. - first_error was set once and never cleared when the error streak resolved, so a 403 that had already recovered was reported as the first cause of a later, unrelated parse-failure streak. That is the same misattribution as F1, one variable over. It resets with consecutive_errors now. Log-channel and diagnostic fixes: - the rejected wait value was interpolated unsanitized; the sanitizer definitions move above the input coercion so it can be sanitized there too. - escaping now runs before truncation, so the cap bounds the line actually emitted instead of a pre-escape length that could triple, and truncation is marked rather than severing a name mid-word. - the pending/cancelled lists get a wider cap: 300 chars is right for one hostile string and wrong for a legitimately long list, and that line is how an operator answers 'why is this still running?'. - the first-error clause is suppressed when it equals the last, so the same 400-char blob is not printed twice. - a cancelled-but-never-replaced check is now ::error::. Unlike the failed-checks path it is not red anywhere else, so that annotation is the only signal. The annotation level now follows one stated rule instead of being ad hoc. Tests. assert_no_match failed open on a malformed regex (grep rc=2 read as 'no match') - the fourth instance of an assertion that cannot fail, in the guard written to prevent exactly that. It now branches on the return code. Added the gaps the AI reviewer found: commit-status .context injection on both the failed and pending paths (only check-run .name was covered, though both feed the same lines), positive assertions on the pending/cancelled name tests (an empty sanitizer result would have yielded 'pending: ' and still passed), and non-numeric WAIT_MAX_ATTEMPTS / WAIT_MIN_ATTEMPTS coverage. 65 -> 74 tests, all mutation-verified. Docs: the ci-read-token description was 11 lines where every other input is one, and auto-doc pads the whole column to its widest cell, so it rendered every generated table ~1400 chars wide. Shortened, long form stays in the prose. The caller-permission requirement is now consistently scoped to 'unless you pass ci-read-token' in all four places, and the README no longer asserts the public-repo leniency as fact for GITHUB_TOKEN - that was only ever established for the fine-grained PAT. --- .../actions/auto-approve-bot-prs/README.md | 28 ++-- .../actions/auto-approve-bot-prs/action.yml | 24 +-- .../auto-approve-bot-prs/src/wait-for-ci.sh | 155 ++++++++++-------- .../test/wait-for-ci.bats | 136 ++++++++++++++- .github/workflows/auto-approve-bot-prs.yaml | 15 +- README.md | 4 +- docs/workflows/auto-approve-bot-prs.md | 8 +- 7 files changed, 255 insertions(+), 115 deletions(-) diff --git a/.github/actions/auto-approve-bot-prs/README.md b/.github/actions/auto-approve-bot-prs/README.md index 38cd60a..da8dc3b 100644 --- a/.github/actions/auto-approve-bot-prs/README.md +++ b/.github/actions/auto-approve-bot-prs/README.md @@ -35,16 +35,16 @@ slow external checks have not shown up yet. -| 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 polling
(check-runs + commit statuses) only. Defaults to the caller's
GITHUB_TOKEN, which is what you want:
reading CI state needs no distinct
identity, and only the approval does,
because GitHub forbids self-approval. Do NOT
point this at the approving PAT:
fine-grained PATs cannot call the Checks
API at all (there is no
Checks permission to grant), so polling
would fail every time and the
action would default-deny forever. Unless you
set this, the CALLER workflow must
grant `checks: read` and `statuses: read` (a reusable workflow cannot add them for you). Supplying
a token here bypasses github.token for
the poll, so those grants are
then not needed. | -| 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 | @@ -66,9 +66,11 @@ permission to grant, and the fine-grained permissions reference lists no would default-deny forever. This is not hypothetical: it stalled the `v0.36.1` release cut for ~70 minutes (DEVOPS-1254). -Note that this misconfiguration is invisible on a **public** repository, where -`/check-runs` answers with no credentials at all. Working there proves nothing -about a private caller. +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 diff --git a/.github/actions/auto-approve-bot-prs/action.yml b/.github/actions/auto-approve-bot-prs/action.yml index 16fa52c..f9e7ce1 100644 --- a/.github/actions/auto-approve-bot-prs/action.yml +++ b/.github/actions/auto-approve-bot-prs/action.yml @@ -1,11 +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. 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 (it can block a - release cut waiting on the merge); that raises an annotation only and never a - non-zero exit. + 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' @@ -23,18 +24,7 @@ inputs: 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 polling (check-runs + commit statuses) only. - Defaults to the caller's GITHUB_TOKEN, which is what you want: reading CI - state needs no distinct identity, and only the approval does, because - GitHub forbids self-approval. Do NOT point this at the approving PAT: - fine-grained PATs cannot call the Checks API at all (there is no Checks - permission to grant), so polling would fail every time and the action would - default-deny forever. - Unless you set this, the CALLER workflow must grant `checks: read` and - `statuses: read` (a reusable workflow cannot add them for you). Supplying - a token here bypasses github.token for the poll, so those grants are then - not needed. + 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' 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 2216279..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,62 +31,6 @@ set -euo pipefail : "${PR_HEAD_SHA:?PR_HEAD_SHA required}" : "${SELF_RUN_ID:?SELF_RUN_ID required}" -# 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. -numeric_or_default() { - local name="$1" value="$2" fallback="$3" - if [ -n "$value" ] && [ -z "${value//[0-9]/}" ] && [ "$value" -gt 0 ] 2>/dev/null; then - printf '%s' "$value" - return 0 - fi - echo "::warning::${name}='${value}' is not a positive integer; using ${fallback}" >&2 - printf '%s' "$fallback" -} -max_attempts="$(numeric_or_default WAIT_MAX_ATTEMPTS "${WAIT_MAX_ATTEMPTS:-90}" 90)" -min_attempts="$(numeric_or_default WAIT_MIN_ATTEMPTS "${WAIT_MIN_ATTEMPTS:-12}" 12)" -sleep_seconds="$(numeric_or_default WAIT_SLEEP_SECONDS "${WAIT_SLEEP_SECONDS:-10}" 10)" - -emit() { - local k="$1" v="$2" - [ -n "${GITHUB_OUTPUT:-}" ] && printf '%s=%s\n' "$k" "$v" >> "$GITHUB_OUTPUT" - 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 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" -} - # sanitize_for_log — stdin to a single safe log line on stdout. # # EVERY externally-controlled string that reaches an `echo` in this script must @@ -115,12 +59,17 @@ gh_json() { # # 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' \ - | cut -c1-300 \ | sed 's/%/%25/g' \ - | cut -c1-400 \ + | 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 } @@ -141,6 +90,68 @@ 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" + [ -n "${GITHUB_OUTPUT:-}" ] && printf '%s=%s\n' "$k" "$v" >> "$GITHUB_OUTPUT" + 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 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" +} + # jq_or_fail [jq-flags...] — run jq on $json with optional # flags (e.g. -r). Returns non-zero on parse failure. Callers must check # exit status; silent empty output here is not the same as success. @@ -165,7 +176,9 @@ last_error="" # 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 @@ -265,11 +278,15 @@ for attempt in $(seq 1 "$max_attempts"); do # settle floor, and too many consecutive errors exit non-green. consecutive_errors=$(( consecutive_errors + 1 )) if [ "$consecutive_errors" -ge "$max_consecutive_errors" ]; then + # 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:+; first error: ${first_error}}" + 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 @@ -277,7 +294,11 @@ for attempt in $(seq 1 "$max_attempts"); do 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 )) @@ -287,7 +308,7 @@ for attempt in $(seq 1 "$max_attempts"); do # immediately — these are not transient and won't be replaced. if [ "$real_failed" -gt 0 ]; then # 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) + 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 @@ -297,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: $(safe "${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 @@ -305,7 +329,7 @@ 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' | sanitize_for_log) + 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 @@ -324,9 +348,12 @@ for attempt in $(seq 1 "$max_attempts"); do sleep "$sleep_seconds" done +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:+; first error: ${first_error}}" +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/wait-for-ci.bats b/.github/actions/auto-approve-bot-prs/test/wait-for-ci.bats index bd1bb82..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 @@ -25,10 +25,15 @@ kv() { grep "^$1=" "$GITHUB_OUTPUT" | tail -n1; } # 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() { - if grep -qP -- "$1" <<<"$2"; then - printf 'assert_no_match: unexpected match for %s\n' "$1" >&2 - return 1 - fi + 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" { @@ -439,6 +444,9 @@ assert_no_match() { 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" { @@ -451,6 +459,39 @@ assert_no_match() { [ "$(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" { @@ -465,7 +506,7 @@ assert_no_match() { ]}' run "$SCRIPT" [ "$status" -eq 0 ] [ "$(kv ci_green)" = "ci_green=false" ] - [[ "$output" == *"WAIT_SLEEP_SECONDS='not-a-duration' is not a positive integer"* ]] + [[ "$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" { @@ -527,6 +568,91 @@ assert_no_match() { [[ "$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 diff --git a/.github/workflows/auto-approve-bot-prs.yaml b/.github/workflows/auto-approve-bot-prs.yaml index b815c27..421e328 100644 --- a/.github/workflows/auto-approve-bot-prs.yaml +++ b/.github/workflows/auto-approve-bot-prs.yaml @@ -32,13 +32,7 @@ on: description: 'GitHub PAT for approving PRs (must be different identity from PR author)' required: true ci-read-token: - description: | - Optional. Token for the read-only CI poll, overriding the default of - the caller's GITHUB_TOKEN. Only needed as an escape hatch when the - caller cannot grant `checks: read` / `statuses: read` (see the job - permissions below); pass a classic PAT with `repo` scope or a GitHub - App token, both of which can reach the Checks API. A fine-grained PAT - cannot, so do NOT pass gh-access-token here. + 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: @@ -49,9 +43,10 @@ jobs: # 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. Every caller MUST declare - # `checks: read` and `statuses: read` too, or the poll 403s on a private repo - # and the action default-denies without ever approving. See DEVOPS-1254. + # 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 diff --git a/README.md b/README.md index 518ce13..0071508 100644 --- a/README.md +++ b/README.md @@ -521,8 +521,8 @@ jobs: to auto-approve (GitHub forbids self-review). When identity matches, the job skips gracefully instead of failing. -**`checks: read` and `statuses: read` are not optional, and the called workflow -cannot supply them for you** — GitHub only lets a reusable +**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 diff --git a/docs/workflows/auto-approve-bot-prs.md b/docs/workflows/auto-approve-bot-prs.md index 561f466..bf5faef 100644 --- a/docs/workflows/auto-approve-bot-prs.md +++ b/docs/workflows/auto-approve-bot-prs.md @@ -25,10 +25,10 @@ secret. -| SECRET | REQUIRED | DESCRIPTION | -|-----------------|----------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| ci-read-token | false | Optional. Token for the read-only CI
poll, overriding the default of the
caller's GITHUB_TOKEN. Only needed as an
escape hatch when the caller cannot
grant `checks: read` / `statuses: read` (see the
job permissions below); pass a classic
PAT with `repo` scope or a
GitHub App token, both of which
can reach the Checks API. A
fine-grained PAT cannot, so do NOT
pass gh-access-token here. | -| 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) |