From 1a3d35aee3c24d2aebbca40aa5f1c149884369f9 Mon Sep 17 00:00:00 2001 From: Noah Gift Date: Thu, 22 Oct 2026 17:19:54 +0200 Subject: [PATCH 1/2] pr-gate authorize: fix membership lookup, bounded retry, fail-closed (aprender#5951) The probe discarded gh's stderr, so a 90 ms indeterminate on both controls left no evidence of cause. Now: gh-missing check, per-attempt status captured and named in every error, 3 attempts (5/10/20 s) for transient 5xx/429/timeout only, all non-definitive outcomes fail closed. Adds tests/pr-gate-authorize-test.sh. Refs: paiml/aprender#5951 Co-Authored-By: Claude Sonnet 5.5 --- .github/workflows/pr-gate.yml | 78 +++++++++++++++++++++++++-------- tests/pr-gate-authorize-test.sh | 58 ++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 18 deletions(-) create mode 100755 tests/pr-gate-authorize-test.sh diff --git a/.github/workflows/pr-gate.yml b/.github/workflows/pr-gate.yml index 54fedfb..28911ef 100644 --- a/.github/workflows/pr-gate.yml +++ b/.github/workflows/pr-gate.yml @@ -84,23 +84,65 @@ jobs: # instrument this whole file exists to route around. unset GITHUB_TOKEN GH_TOKEN - # Returns: member | notmember | indeterminate + # membership USER -> sets MEMBERSHIP_RESULT (member | notmember | + # indeterminate) and MEMBERSHIP_DETAIL (the HTTP status / error that + # produced it). Called directly, not in $(...), so both survive. + # # `indeterminate` is a THIRD answer and must never collapse into # `notmember` -- conflating "they are not a member" with "I could # not ask" is the exact defect that closed two owners' PRs. + # + # The previous version threw the error text away, so a probe that + # failed in 90 ms on a runner with no usable gh (aprender#5951: + # gx10-blackwell, both controls indeterminate) left no evidence of + # WHY. Now every non-definitive outcome names its status. + # + # Only TRANSIENT failures are retried (5xx, 429, secondary rate + # limit, timeouts/connection errors), 3 attempts, backoff 5/10/20 s. + # Auth failures, a missing gh, 302 and unparseable replies are + # deterministic: retrying them only delays the same answer. + BACKOFF="${PRGATE_BACKOFF:-5 10 20}" membership() { - local user="$1" resp rc state - resp=$(gh api "orgs/${ORG}/memberships/${user}" 2>&1) && rc=0 || rc=$? - if [ "$rc" -eq 0 ]; then - state=$(printf '%s' "$resp" | jq -r '.state // "unknown"') - # "pending" = invited, not accepted. Not a member yet. - if [ "$state" = "active" ]; then echo member; else echo notmember; fi + local user="$1" resp rc state attempt=0 delay + MEMBERSHIP_RESULT=indeterminate + MEMBERSHIP_DETAIL="no attempt made" + if ! command -v gh >/dev/null 2>&1; then + MEMBERSHIP_DETAIL="gh not found on this runner (PATH=${PATH})" return 0 fi - case "$resp" in - *"HTTP 404"*) echo notmember ;; - *) echo indeterminate ;; - esac + for delay in $BACKOFF; do + attempt=$((attempt + 1)) + resp=$(gh api "orgs/${ORG}/memberships/${user}" 2>&1) && rc=0 || rc=$? + if [ "$rc" -eq 0 ]; then + state=$(printf '%s' "$resp" | jq -r '.state // "unknown"' 2>/dev/null) || state=unparseable + case "$state" in + active) MEMBERSHIP_RESULT=member; MEMBERSHIP_DETAIL="HTTP 200 state=active" ;; + pending) MEMBERSHIP_RESULT=notmember; MEMBERSHIP_DETAIL="HTTP 200 state=pending (invited, not accepted)" ;; + unparseable) MEMBERSHIP_DETAIL="HTTP 200 but reply is not JSON: $(printf '%s' "$resp" | head -c 200)" ;; + *) MEMBERSHIP_DETAIL="HTTP 200 with unexpected state=${state}" ;; + esac + return 0 + fi + resp=$(printf '%s' "$resp" | tr '\n' ' ' | head -c 300) + case "$resp" in + *"HTTP 404"*) + # Definitive ONLY because this call runs with org-admin auth; + # with a token that cannot see private members a 404 would be + # a lie, which is what the instrument probe below detects. + MEMBERSHIP_RESULT=notmember; MEMBERSHIP_DETAIL="HTTP 404"; return 0 ;; + *"HTTP 5"[0-9][0-9]*|*"HTTP 429"*|*"rate limit"*|*"secondary"*|*[Tt]imeout*|*"timed out"*|*"connection"*|*"EOF"*|*"dial tcp"*) + MEMBERSHIP_DETAIL="transient (attempt ${attempt}/3, rc=${rc}): ${resp}" + if [ "$attempt" -lt 3 ]; then + echo "membership(${user}): ${MEMBERSHIP_DETAIL}; retrying in ${delay}s" >&2 + sleep "$delay" + fi ;; + *) + MEMBERSHIP_DETAIL="non-retryable (rc=${rc}): ${resp}" + return 0 ;; + esac + done + MEMBERSHIP_RESULT=indeterminate + return 0 } # -------------------------------------------------------------- @@ -114,12 +156,12 @@ jobs: PROBE_MEMBER=noahgift # org owner, private membership PROBE_STRANGER=paiml-pr-gate-sentinel-no-such-user - probe_pos=$(membership "$PROBE_MEMBER") - probe_neg=$(membership "$PROBE_STRANGER") - echo "instrument probe: ${PROBE_MEMBER}=${probe_pos} (want member), ${PROBE_STRANGER}=${probe_neg} (want notmember)" + membership "$PROBE_MEMBER"; probe_pos=$MEMBERSHIP_RESULT; detail_pos=$MEMBERSHIP_DETAIL + membership "$PROBE_STRANGER"; probe_neg=$MEMBERSHIP_RESULT; detail_neg=$MEMBERSHIP_DETAIL + echo "instrument probe: ${PROBE_MEMBER}=${probe_pos} [${detail_pos}] (want member), ${PROBE_STRANGER}=${probe_neg} [${detail_neg}] (want notmember)" if [ "$probe_pos" != "member" ] || [ "$probe_neg" != "notmember" ]; then - echo "::error::pr-gate instrument probe FAILED (${PROBE_MEMBER}=${probe_pos}, ${PROBE_STRANGER}=${probe_neg}) -- refusing to judge anyone with a check that cannot demonstrate both answers" + echo "::error::pr-gate instrument probe FAILED (${PROBE_MEMBER}=${probe_pos} [${detail_pos}], ${PROBE_STRANGER}=${probe_neg} [${detail_neg}]) -- refusing to judge anyone with a check that cannot demonstrate both answers" # Fail RED and leave the PR OPEN. The harm is asymmetric and # measured: auto-closing a legitimate owner's PR has happened # repeatedly and is invisible to them, while an unauthorized PR @@ -150,8 +192,8 @@ jobs: exit 0 fi - author_state=$(membership "$PR_AUTHOR") - echo "author=${PR_AUTHOR} type=${PR_AUTHOR_TYPE} membership=${author_state}" + membership "$PR_AUTHOR"; author_state=$MEMBERSHIP_RESULT + echo "author=${PR_AUTHOR} type=${PR_AUTHOR_TYPE} membership=${author_state} [${MEMBERSHIP_DETAIL}]" case "$author_state" in member) @@ -164,7 +206,7 @@ jobs: echo "$PR_AUTHOR: not a member -- REJECTED" ;; *) # Could not ask. Not a verdict. Never close on this. - echo "::error::could not determine ${ORG} membership for ${PR_AUTHOR} -- leaving the PR open for a human" + echo "::error::could not determine ${ORG} membership for ${PR_AUTHOR} [${MEMBERSHIP_DETAIL}] -- leaving the PR open for a human" exit 1 ;; esac diff --git a/tests/pr-gate-authorize-test.sh b/tests/pr-gate-authorize-test.sh new file mode 100755 index 0000000..7a59c8a --- /dev/null +++ b/tests/pr-gate-authorize-test.sh @@ -0,0 +1,58 @@ +#!/usr/bin/env bash +# Test for the `authorize` step of .github/workflows/pr-gate.yml (aprender#5951). +# Extracts the real step script from the workflow and runs it against a stubbed +# `gh`. Set PRGATE_MUTATE=failopen to prove the test discriminates (it rewires +# the fail-closed branches into passes and the test must go RED). +set -uo pipefail +cd "$(dirname "$0")/.." +WF=.github/workflows/pr-gate.yml +T=$(mktemp -d); trap 'rm -rf "${T:?}"' EXIT + +awk '/id: authz/{a=1} a&&/run: \|/{r=1;next} r&&/^ - name:/{exit} r{sub(/^ /,"");print}' "$WF" > "$T/step.sh" +[ -s "$T/step.sh" ] || { echo "FAIL: could not extract step"; exit 2; } +if [ "${PRGATE_MUTATE:-}" = failopen ]; then + sed -i 's/^\( *\)exit 1 ;;$/\1echo "allow=true" >> "$GITHUB_OUTPUT"; exit 0 ;;/' "$T/step.sh" + sed -i 's/^\( *\)exit 1$/\1echo "allow=true" >> "$GITHUB_OUTPUT"; exit 0/' "$T/step.sh" +fi + +mkdir -p "$T/bin" +cat > "$T/bin/gh" <<'STUB' +#!/usr/bin/env bash +# gh api orgs/paiml/memberships/; replies from $STUB/ (rc|text per line, last repeats) +user=${2##*/}; f="$STUB/$user"; n=$(cat "$f.n" 2>/dev/null || echo 0); n=$((n+1)); echo $n > "$f.n" +line=$(sed -n "${n}p" "$f"); [ -n "$line" ] || line=$(tail -n1 "$f") +rc=${line%%|*}; txt=${line#*|}; printf '%s\n' "$txt"; exit "$rc" +STUB +chmod +x "$T/bin/gh" + +pass=0; fail=0 +run() { + local name=$1 author=$2 want_rc=$3 want_allow=$4 out rc allow + : > "$T/out"; export STUB="$T/s" GITHUB_OUTPUT="$T/out" PRGATE_BACKOFF="0 0 0" + rm -f "${STUB:?}"/*.n + out=$(PATH="$T/bin:$PATH" PR_AUTHOR=$author PR_AUTHOR_TYPE=User bash "$T/step.sh" 2>&1); rc=$? + allow=$(sed -n 's/^allow=//p' "$T/out" | tail -n1) + if [ "$rc" = "$want_rc" ] && [ "${allow:-}" = "$want_allow" ]; then + echo "ok $name"; pass=$((pass+1)) + else + echo "FAIL $name: rc=$rc (want $want_rc) allow='${allow:-}' (want '$want_allow')"; printf '%s\n' "$out" | tail -n 4; fail=$((fail+1)) + fi +} +seed() { rm -rf "${T:?}/s"; mkdir -p "$T/s"; } +MEMBER='0|{"state":"active"}'; NOT='1|gh: Not Found (HTTP 404)'; S503='1|gh: Service Unavailable (HTTP 503)' +CTRL() { printf '%s\n' "$MEMBER" > "$T/s/noahgift"; printf '%s\n' "$NOT" > "$T/s/paiml-pr-gate-sentinel-no-such-user"; } + +seed; CTRL; printf '%s\n' "$MEMBER" > "$T/s/alice"; run "member -> pass" alice 0 true +seed; CTRL; printf '%s\n' "$NOT" > "$T/s/mallory"; run "non-member -> rejected (allow=false)" mallory 0 false +seed; CTRL; printf '%s\n%s\n%s\n' "$S503" "$S503" "$MEMBER" > "$T/s/alice"; run "5xx twice then member -> pass" alice 0 true +seed; CTRL; printf '%s\n' "$S503" > "$T/s/alice"; run "persistent 5xx -> fail-closed" alice 1 "" +seed; CTRL; printf '%s\n' '1|gh: Resource not accessible (HTTP 302)' > "$T/s/alice"; run "302 -> fail-closed" alice 1 "" +seed; CTRL; printf '%s\n' '1|gh: Bad credentials (HTTP 401)' > "$T/s/alice"; run "401 -> fail-closed" alice 1 "" +seed; CTRL; printf '%s\n' '0|proxy error' > "$T/s/alice"; run "200 non-JSON -> fail-closed" alice 1 "" +seed; printf '%s\n' "$S503" > "$T/s/noahgift"; printf '%s\n' "$S503" > "$T/s/paiml-pr-gate-sentinel-no-such-user"; printf '%s\n' "$MEMBER" > "$T/s/alice"; run "probe indeterminate -> fail-closed" alice 1 "" +seed; printf '%s\n' "$MEMBER" > "$T/s/noahgift"; printf '%s\n' "$MEMBER" > "$T/s/paiml-pr-gate-sentinel-no-such-user"; run "blind instrument -> fail-closed" alice 1 "" +seed; CTRL; : > "$T/out"; export GITHUB_OUTPUT="$T/out" PRGATE_BACKOFF="0 0 0" +out=$(PATH="/nonexistent" PR_AUTHOR=alice PR_AUTHOR_TYPE=User /bin/bash "$T/step.sh" 2>&1); rc=$? +if [ "$rc" = 1 ] && grep -q 'gh not found' <<<"$out"; then echo "ok gh missing -> fail-closed naming cause"; pass=$((pass+1)); else echo "FAIL gh missing: rc=$rc"; fail=$((fail+1)); fi + +echo "passed=$pass failed=$fail"; [ "$fail" = 0 ] From d38b82c839a788c2b38c0cd42c9d07cb4dd2476e Mon Sep 17 00:00:00 2001 From: Noah Gift Date: Thu, 22 Oct 2026 17:25:36 +0200 Subject: [PATCH 2/2] pr-gate authorize: decide from event author_association, no host credentials, fail-closed (aprender#5951) Refs: paiml/aprender#5951 Co-Authored-By: Claude Sonnet 5.5 --- .github/workflows/pr-gate.yml | 205 ++++++++++---------------------- tests/pr-gate-authorize-test.sh | 70 ++++++----- 2 files changed, 99 insertions(+), 176 deletions(-) diff --git a/.github/workflows/pr-gate.yml b/.github/workflows/pr-gate.yml index 28911ef..e369db6 100644 --- a/.github/workflows/pr-gate.yml +++ b/.github/workflows/pr-gate.yml @@ -40,178 +40,95 @@ jobs: id: authz shell: bash env: + # Everything the decision reads comes from the event payload and is + # passed through env:, never interpolated into the script body. + EVENT_NAME: ${{ github.event_name }} PR_AUTHOR: ${{ github.event.pull_request.user.login }} PR_AUTHOR_TYPE: ${{ github.event.pull_request.user.type }} + PR_ASSOCIATION: ${{ github.event.pull_request.author_association }} run: | set -uo pipefail # ============================================================== - # WHY THIS USES THE RUNNER'S AMBIENT ORG AUTH + # DECISION COMES FROM THE EVENT PAYLOAD ONLY (aprender#5951). # - # Two different APIs have now been caught giving this gate a - # confidently wrong answer, both for the same underlying reason: - # the default GITHUB_TOKEN cannot see PRIVATE org memberships, - # and every paiml owner's membership is private. + # Operator: "the fix belongs in the job, not on gx10" and "have + # authorize read author_association from the event". Standing + # rule: no workflow may use credentials that live on a host. + # This step therefore makes NO API call and never invokes `gh`: + # the earlier membership probe depended on the runner's ambient + # `gh` login, and a runner without one (gx10-blackwell) turned + # every PR red with "indeterminate". # - # 1. `orgs.checkMembershipForUser` with GITHUB_TOKEN answers 404 - # for a private member -- byte-for-byte identical to "not a - # member". This closed @noahgift's PRs until he was - # allowlisted by name (#12), then closed @alfredodeza's - # paiml/infra#340 twice while `gate` and `validate` were - # green on the same commit. + # Known limitation, accepted: GitHub reports a PRIVATE org + # member's association as COLLABORATOR rather than MEMBER. That + # is why COLLABORATOR is on the allow-list. The cost is that an + # outside collaborator is indistinguishable from a private + # member; collaborators already hold repo access granted by an + # owner, so this does not widen who can open PRs. # - # 2. `author_association` from the payload looked like the fix - # -- the REST API reports MEMBER for those same PRs. It is - # NOT the fix. MEASURED on the live webhook for #340: - # author_association=COLLABORATOR. GitHub downgrades the - # payload's association for private members too, so a real - # org owner is indistinguishable from an outside - # collaborator. That attempt (#54) is what this replaces. - # - # The runner's ambient `gh` auth holds admin:org and therefore - # sees the truth. This is the same decision, for the same reason, - # already taken by lane-liveness.sh (CLAUDE.md: "Uses the - # runner's ambient org gh auth -- never github.token"). - # - # Safe here: this job has exactly ONE input it does not control, - # a login string, and there is NO actions/checkout anywhere in - # this workflow. No PR-authored code is fetched or run. + # FAIL-CLOSED: anything not on the allow-list -- including an + # empty, missing or unknown value -- fails. A missing value never + # passes on a PR event. + # Allow-list: OWNER MEMBER COLLABORATOR. # ============================================================== - ORG=paiml - - # Deliberately NOT the workflow token: that token is the blind - # instrument this whole file exists to route around. - unset GITHUB_TOKEN GH_TOKEN + # Strip anything that could forge a workflow command before echoing. + clean() { printf '%s' "$1" | tr -c 'A-Za-z0-9_.-' '?' | head -c 64; } - # membership USER -> sets MEMBERSHIP_RESULT (member | notmember | - # indeterminate) and MEMBERSHIP_DETAIL (the HTTP status / error that - # produced it). Called directly, not in $(...), so both survive. - # - # `indeterminate` is a THIRD answer and must never collapse into - # `notmember` -- conflating "they are not a member" with "I could - # not ask" is the exact defect that closed two owners' PRs. - # - # The previous version threw the error text away, so a probe that - # failed in 90 ms on a runner with no usable gh (aprender#5951: - # gx10-blackwell, both controls indeterminate) left no evidence of - # WHY. Now every non-definitive outcome names its status. - # - # Only TRANSIENT failures are retried (5xx, 429, secondary rate - # limit, timeouts/connection errors), 3 attempts, backoff 5/10/20 s. - # Auth failures, a missing gh, 302 and unparseable replies are - # deterministic: retrying them only delays the same answer. - BACKOFF="${PRGATE_BACKOFF:-5 10 20}" - membership() { - local user="$1" resp rc state attempt=0 delay - MEMBERSHIP_RESULT=indeterminate - MEMBERSHIP_DETAIL="no attempt made" - if ! command -v gh >/dev/null 2>&1; then - MEMBERSHIP_DETAIL="gh not found on this runner (PATH=${PATH})" - return 0 - fi - for delay in $BACKOFF; do - attempt=$((attempt + 1)) - resp=$(gh api "orgs/${ORG}/memberships/${user}" 2>&1) && rc=0 || rc=$? - if [ "$rc" -eq 0 ]; then - state=$(printf '%s' "$resp" | jq -r '.state // "unknown"' 2>/dev/null) || state=unparseable - case "$state" in - active) MEMBERSHIP_RESULT=member; MEMBERSHIP_DETAIL="HTTP 200 state=active" ;; - pending) MEMBERSHIP_RESULT=notmember; MEMBERSHIP_DETAIL="HTTP 200 state=pending (invited, not accepted)" ;; - unparseable) MEMBERSHIP_DETAIL="HTTP 200 but reply is not JSON: $(printf '%s' "$resp" | head -c 200)" ;; - *) MEMBERSHIP_DETAIL="HTTP 200 with unexpected state=${state}" ;; - esac - return 0 - fi - resp=$(printf '%s' "$resp" | tr '\n' ' ' | head -c 300) - case "$resp" in - *"HTTP 404"*) - # Definitive ONLY because this call runs with org-admin auth; - # with a token that cannot see private members a 404 would be - # a lie, which is what the instrument probe below detects. - MEMBERSHIP_RESULT=notmember; MEMBERSHIP_DETAIL="HTTP 404"; return 0 ;; - *"HTTP 5"[0-9][0-9]*|*"HTTP 429"*|*"rate limit"*|*"secondary"*|*[Tt]imeout*|*"timed out"*|*"connection"*|*"EOF"*|*"dial tcp"*) - MEMBERSHIP_DETAIL="transient (attempt ${attempt}/3, rc=${rc}): ${resp}" - if [ "$attempt" -lt 3 ]; then - echo "membership(${user}): ${MEMBERSHIP_DETAIL}; retrying in ${delay}s" >&2 - sleep "$delay" - fi ;; - *) - MEMBERSHIP_DETAIL="non-retryable (rc=${rc}): ${resp}" - return 0 ;; - esac - done - MEMBERSHIP_RESULT=indeterminate - return 0 + allow() { + echo "allow=true" >> "$GITHUB_OUTPUT" + echo "reason=$1" >> "$GITHUB_OUTPUT" + echo "${2:-$PR_AUTHOR}: $1 -- ALLOWED" } - # -------------------------------------------------------------- - # PROBE THE INSTRUMENT BEFORE TRUSTING ITS VERDICT. - # A membership check that cannot see members reports everyone as - # an outsider, and that is precisely how this gate failed twice. - # So it must answer a known member and a known non-member - # correctly, right now, on this runner, before it is allowed to - # judge anybody. - # -------------------------------------------------------------- - PROBE_MEMBER=noahgift # org owner, private membership - PROBE_STRANGER=paiml-pr-gate-sentinel-no-such-user - - membership "$PROBE_MEMBER"; probe_pos=$MEMBERSHIP_RESULT; detail_pos=$MEMBERSHIP_DETAIL - membership "$PROBE_STRANGER"; probe_neg=$MEMBERSHIP_RESULT; detail_neg=$MEMBERSHIP_DETAIL - echo "instrument probe: ${PROBE_MEMBER}=${probe_pos} [${detail_pos}] (want member), ${PROBE_STRANGER}=${probe_neg} [${detail_neg}] (want notmember)" - - if [ "$probe_pos" != "member" ] || [ "$probe_neg" != "notmember" ]; then - echo "::error::pr-gate instrument probe FAILED (${PROBE_MEMBER}=${probe_pos} [${detail_pos}], ${PROBE_STRANGER}=${probe_neg} [${detail_neg}]) -- refusing to judge anyone with a check that cannot demonstrate both answers" - # Fail RED and leave the PR OPEN. The harm is asymmetric and - # measured: auto-closing a legitimate owner's PR has happened - # repeatedly and is invisible to them, while an unauthorized PR - # left open under a red required check is visible to every - # human on the repo. - exit 1 - fi + case "${EVENT_NAME:-}" in + pull_request_target|pull_request) ;; + merge_group|push) + # No PR author exists on these events. They are reachable only + # by someone who already passed this gate on the PR (merge + # queue entry / push to a protected branch need write access), + # and the Close step below acts on a PR that does not exist + # here. Allowing is safe; documented decision, not a default. + allow "no PR author on ${EVENT_NAME:-} event (already gated at PR time)" "${EVENT_NAME:-}" + exit 0 ;; + *) + echo "::error::pr-gate authorize: unsupported event '$(clean "${EVENT_NAME:-}")' -- refusing (fail-closed)" + exit 1 ;; + esac - # -------------------------------------------------------------- - # Decision. - # -------------------------------------------------------------- - # Explicitly allowlisted EXTERNAL contributors only. Org members - # are authorized structurally, below -- adding one here is how - # this gate hid its own blindness for four months. - case "$PR_AUTHOR" in - guyernest|spudnic) # guyernest: pmcp lead developer; spudnic: authorized contributor (operator 2026-09-26) -- outside collaborators, not org members - echo "allow=true" >> "$GITHUB_OUTPUT" - echo "reason=allowlisted external contributor" >> "$GITHUB_OUTPUT" - echo "$PR_AUTHOR: allowlisted external contributor -- ALLOWED" + # Explicitly allowlisted EXTERNAL contributors (outside collaborators). + case "${PR_AUTHOR:-}" in + guyernest|spudnic) # guyernest: pmcp lead developer; spudnic: authorized contributor (operator 2026-09-26) + allow "allowlisted external contributor" exit 0 ;; esac # Bots (dependabot, renovate, actions-user) are always allowed. - if [ "$PR_AUTHOR_TYPE" = "Bot" ]; then - echo "allow=true" >> "$GITHUB_OUTPUT" - echo "reason=bot" >> "$GITHUB_OUTPUT" - echo "$PR_AUTHOR: bot -- ALLOWED" + if [ "${PR_AUTHOR_TYPE:-}" = "Bot" ]; then + allow "bot" exit 0 fi - membership "$PR_AUTHOR"; author_state=$MEMBERSHIP_RESULT - echo "author=${PR_AUTHOR} type=${PR_AUTHOR_TYPE} membership=${author_state} [${MEMBERSHIP_DETAIL}]" - - case "$author_state" in - member) - echo "allow=true" >> "$GITHUB_OUTPUT" - echo "reason=active ${ORG} org member" >> "$GITHUB_OUTPUT" - echo "$PR_AUTHOR: active ${ORG} org member -- ALLOWED" ;; - notmember) + case "${PR_ASSOCIATION:-}" in + OWNER|MEMBER|COLLABORATOR) + allow "author_association=${PR_ASSOCIATION}" + exit 0 ;; + CONTRIBUTOR|FIRST_TIMER|FIRST_TIME_CONTRIBUTOR|MANNEQUIN|NONE) echo "allow=false" >> "$GITHUB_OUTPUT" - echo "reason=not an active ${ORG} org member and not allowlisted" >> "$GITHUB_OUTPUT" - echo "$PR_AUTHOR: not a member -- REJECTED" ;; + echo "reason=author_association=${PR_ASSOCIATION} is not OWNER/MEMBER/COLLABORATOR and author is not allowlisted" >> "$GITHUB_OUTPUT" + echo "::error::pr-gate: author_association=${PR_ASSOCIATION} for $(clean "${PR_AUTHOR:-}") -- REJECTED" + exit 1 ;; *) - # Could not ask. Not a verdict. Never close on this. - echo "::error::could not determine ${ORG} membership for ${PR_AUTHOR} [${MEMBERSHIP_DETAIL}] -- leaving the PR open for a human" + # Empty, missing or unknown: could not establish who this is. + # Fail red, leave the PR open for a human (no allow=false, so + # the Close step does not run). + echo "::error::pr-gate: author_association='$(clean "${PR_ASSOCIATION:-}")' is empty or unknown -- refusing (fail-closed)" exit 1 ;; esac - name: Close unauthorized PR - if: steps.authz.outputs.allow != 'true' + if: ${{ !cancelled() && steps.authz.outputs.allow == 'false' }} uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 env: REASON: ${{ steps.authz.outputs.reason }} diff --git a/tests/pr-gate-authorize-test.sh b/tests/pr-gate-authorize-test.sh index 7a59c8a..4ab7b85 100755 --- a/tests/pr-gate-authorize-test.sh +++ b/tests/pr-gate-authorize-test.sh @@ -1,8 +1,9 @@ #!/usr/bin/env bash # Test for the `authorize` step of .github/workflows/pr-gate.yml (aprender#5951). -# Extracts the real step script from the workflow and runs it against a stubbed -# `gh`. Set PRGATE_MUTATE=failopen to prove the test discriminates (it rewires -# the fail-closed branches into passes and the test must go RED). +# Extracts the REAL step script and runs it with event env vars. `gh` is a stub +# that exits 99 and records every call: the step must make zero calls. +# Mutants (must go RED): PRGATE_MUTATE=allownone (NONE on allow-list), +# PRGATE_MUTATE=failopen (fail-closed branches become passes). set -uo pipefail cd "$(dirname "$0")/.." WF=.github/workflows/pr-gate.yml @@ -10,49 +11,54 @@ T=$(mktemp -d); trap 'rm -rf "${T:?}"' EXIT awk '/id: authz/{a=1} a&&/run: \|/{r=1;next} r&&/^ - name:/{exit} r{sub(/^ /,"");print}' "$WF" > "$T/step.sh" [ -s "$T/step.sh" ] || { echo "FAIL: could not extract step"; exit 2; } -if [ "${PRGATE_MUTATE:-}" = failopen ]; then - sed -i 's/^\( *\)exit 1 ;;$/\1echo "allow=true" >> "$GITHUB_OUTPUT"; exit 0 ;;/' "$T/step.sh" - sed -i 's/^\( *\)exit 1$/\1echo "allow=true" >> "$GITHUB_OUTPUT"; exit 0/' "$T/step.sh" -fi +case "${PRGATE_MUTATE:-}" in + allownone) sed -i 's/OWNER|MEMBER|COLLABORATOR)/OWNER|MEMBER|COLLABORATOR|NONE)/' "$T/step.sh" ;; + failopen) sed -i 's/^\( *\)exit 1 ;;$/\1exit 0 ;;/; s/^\( *\)exit 1$/\1exit 0/' "$T/step.sh" ;; +esac mkdir -p "$T/bin" cat > "$T/bin/gh" <<'STUB' #!/usr/bin/env bash -# gh api orgs/paiml/memberships/; replies from $STUB/ (rc|text per line, last repeats) -user=${2##*/}; f="$STUB/$user"; n=$(cat "$f.n" 2>/dev/null || echo 0); n=$((n+1)); echo $n > "$f.n" -line=$(sed -n "${n}p" "$f"); [ -n "$line" ] || line=$(tail -n1 "$f") -rc=${line%%|*}; txt=${line#*|}; printf '%s\n' "$txt"; exit "$rc" +echo "gh $*" >> "$GH_CALLS"; exit 99 STUB -chmod +x "$T/bin/gh" +chmod +x "$T/bin/gh"; export GH_CALLS="$T/gh.calls" pass=0; fail=0 +# run NAME WANT_RC WANT_ALLOW [VAR=VAL ...] (env cleared of event vars first) run() { - local name=$1 author=$2 want_rc=$3 want_allow=$4 out rc allow - : > "$T/out"; export STUB="$T/s" GITHUB_OUTPUT="$T/out" PRGATE_BACKOFF="0 0 0" - rm -f "${STUB:?}"/*.n - out=$(PATH="$T/bin:$PATH" PR_AUTHOR=$author PR_AUTHOR_TYPE=User bash "$T/step.sh" 2>&1); rc=$? + local name=$1 want_rc=$2 want_allow=$3 out rc allow; shift 3 + : > "$T/out" + out=$(env -u EVENT_NAME -u PR_AUTHOR -u PR_AUTHOR_TYPE -u PR_ASSOCIATION \ + PATH="$T/bin:$PATH" GITHUB_OUTPUT="$T/out" "$@" bash "$T/step.sh" 2>&1); rc=$? allow=$(sed -n 's/^allow=//p' "$T/out" | tail -n1) if [ "$rc" = "$want_rc" ] && [ "${allow:-}" = "$want_allow" ]; then echo "ok $name"; pass=$((pass+1)) else - echo "FAIL $name: rc=$rc (want $want_rc) allow='${allow:-}' (want '$want_allow')"; printf '%s\n' "$out" | tail -n 4; fail=$((fail+1)) + echo "FAIL $name: rc=$rc (want $want_rc) allow='${allow:-}' (want '$want_allow')"; printf '%s\n' "$out" | tail -n 3; fail=$((fail+1)) fi } -seed() { rm -rf "${T:?}/s"; mkdir -p "$T/s"; } -MEMBER='0|{"state":"active"}'; NOT='1|gh: Not Found (HTTP 404)'; S503='1|gh: Service Unavailable (HTTP 503)' -CTRL() { printf '%s\n' "$MEMBER" > "$T/s/noahgift"; printf '%s\n' "$NOT" > "$T/s/paiml-pr-gate-sentinel-no-such-user"; } +PR=(EVENT_NAME=pull_request_target PR_AUTHOR=alice PR_AUTHOR_TYPE=User) +for a in OWNER MEMBER COLLABORATOR; do run "$a -> pass" 0 true "${PR[@]}" PR_ASSOCIATION=$a; done +run "CONTRIBUTOR -> fail, allow=false" 1 false "${PR[@]}" PR_ASSOCIATION=CONTRIBUTOR +run "NONE -> fail, allow=false" 1 false "${PR[@]}" PR_ASSOCIATION=NONE +run "FIRST_TIME_CONTRIBUTOR -> fail" 1 false "${PR[@]}" PR_ASSOCIATION=FIRST_TIME_CONTRIBUTOR +run "FIRST_TIMER -> fail" 1 false "${PR[@]}" PR_ASSOCIATION=FIRST_TIMER +run "MANNEQUIN -> fail" 1 false "${PR[@]}" PR_ASSOCIATION=MANNEQUIN +run "empty -> fail-closed (open)" 1 "" "${PR[@]}" PR_ASSOCIATION= +run "missing (unset) -> fail-closed" 1 "" "${PR[@]}" +run "garbage -> fail-closed" 1 "" "${PR[@]}" PR_ASSOCIATION=SUPERUSER +run "lowercase member -> fail-closed" 1 "" "${PR[@]}" PR_ASSOCIATION=member +run "injection value -> fail, no cmd" 1 "" "${PR[@]}" 'PR_ASSOCIATION=X +::set-output name=allow::true' +run "allowlisted external, NONE -> pass" 0 true EVENT_NAME=pull_request_target PR_AUTHOR=guyernest PR_AUTHOR_TYPE=User PR_ASSOCIATION=NONE +run "bot -> pass" 0 true EVENT_NAME=pull_request_target PR_AUTHOR=dependabot[bot] PR_AUTHOR_TYPE=Bot PR_ASSOCIATION=NONE +run "merge_group -> pass (no PR author)" 0 true EVENT_NAME=merge_group +run "push -> pass (no PR author)" 0 true EVENT_NAME=push +run "unknown event -> fail-closed" 1 "" EVENT_NAME=issues +run "no event at all -> fail-closed" 1 "" -seed; CTRL; printf '%s\n' "$MEMBER" > "$T/s/alice"; run "member -> pass" alice 0 true -seed; CTRL; printf '%s\n' "$NOT" > "$T/s/mallory"; run "non-member -> rejected (allow=false)" mallory 0 false -seed; CTRL; printf '%s\n%s\n%s\n' "$S503" "$S503" "$MEMBER" > "$T/s/alice"; run "5xx twice then member -> pass" alice 0 true -seed; CTRL; printf '%s\n' "$S503" > "$T/s/alice"; run "persistent 5xx -> fail-closed" alice 1 "" -seed; CTRL; printf '%s\n' '1|gh: Resource not accessible (HTTP 302)' > "$T/s/alice"; run "302 -> fail-closed" alice 1 "" -seed; CTRL; printf '%s\n' '1|gh: Bad credentials (HTTP 401)' > "$T/s/alice"; run "401 -> fail-closed" alice 1 "" -seed; CTRL; printf '%s\n' '0|proxy error' > "$T/s/alice"; run "200 non-JSON -> fail-closed" alice 1 "" -seed; printf '%s\n' "$S503" > "$T/s/noahgift"; printf '%s\n' "$S503" > "$T/s/paiml-pr-gate-sentinel-no-such-user"; printf '%s\n' "$MEMBER" > "$T/s/alice"; run "probe indeterminate -> fail-closed" alice 1 "" -seed; printf '%s\n' "$MEMBER" > "$T/s/noahgift"; printf '%s\n' "$MEMBER" > "$T/s/paiml-pr-gate-sentinel-no-such-user"; run "blind instrument -> fail-closed" alice 1 "" -seed; CTRL; : > "$T/out"; export GITHUB_OUTPUT="$T/out" PRGATE_BACKOFF="0 0 0" -out=$(PATH="/nonexistent" PR_AUTHOR=alice PR_AUTHOR_TYPE=User /bin/bash "$T/step.sh" 2>&1); rc=$? -if [ "$rc" = 1 ] && grep -q 'gh not found' <<<"$out"; then echo "ok gh missing -> fail-closed naming cause"; pass=$((pass+1)); else echo "FAIL gh missing: rc=$rc"; fail=$((fail+1)); fi +if [ -s "$GH_CALLS" ]; then echo "FAIL gh invoked from step:"; cat "$GH_CALLS"; fail=$((fail+1)); else echo "ok step never invoked gh (0 calls)"; pass=$((pass+1)); fi +# the extracted script must not mention gh as a command nor host credentials +if grep -Eq '(^|[^A-Za-z_-])gh (api|auth|pr)|unset GH_TOKEN|ambient' <(grep -v '^ *#' "$T/step.sh"); then echo "FAIL step text still references host gh"; fail=$((fail+1)); else echo "ok no host-gh text in step code"; pass=$((pass+1)); fi echo "passed=$pass failed=$fail"; [ "$fail" = 0 ]