From 12604bbd79feb4a75b5e8ea69d6ec7ac821ac16c Mon Sep 17 00:00:00 2001 From: Noah Gift Date: Thu, 22 Oct 2026 20:42:46 +0200 Subject: [PATCH] Revert "pr-gate authorize: decide from event author_association" (#92) Revert rule (two-way door): #92 breaks the gate. Private org members arrive in the payload as CONTRIBUTOR, so pr-gate auto-closed noahgift's aprender#5981 (18:36:00Z) with the "organization members" comment, and #5982 closed at 18:38:14Z. Number: 2 org-owner PRs closed since the 18:03:45Z merge; 0 can open. Re-land with a members/ probe for private members. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/pr-gate.yml | 163 ++++++++++++++++++++------------ tests/pr-gate-authorize-test.sh | 64 ------------- 2 files changed, 102 insertions(+), 125 deletions(-) delete mode 100755 tests/pr-gate-authorize-test.sh diff --git a/.github/workflows/pr-gate.yml b/.github/workflows/pr-gate.yml index e369db6..54fedfb 100644 --- a/.github/workflows/pr-gate.yml +++ b/.github/workflows/pr-gate.yml @@ -40,95 +40,136 @@ 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 # ============================================================== - # DECISION COMES FROM THE EVENT PAYLOAD ONLY (aprender#5951). + # WHY THIS USES THE RUNNER'S AMBIENT ORG AUTH # - # 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". + # 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. # - # 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. + # 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. # - # 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. + # 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. # ============================================================== - # Strip anything that could forge a workflow command before echoing. - clean() { printf '%s' "$1" | tr -c 'A-Za-z0-9_.-' '?' | head -c 64; } + ORG=paiml - allow() { - echo "allow=true" >> "$GITHUB_OUTPUT" - echo "reason=$1" >> "$GITHUB_OUTPUT" - echo "${2:-$PR_AUTHOR}: $1 -- ALLOWED" + # Deliberately NOT the workflow token: that token is the blind + # instrument this whole file exists to route around. + unset GITHUB_TOKEN GH_TOKEN + + # Returns: member | notmember | indeterminate + # `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. + 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 + return 0 + fi + case "$resp" in + *"HTTP 404"*) echo notmember ;; + *) echo indeterminate ;; + esac } - 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 + # -------------------------------------------------------------- + # 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 - # 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" + 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)" + + 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" + # 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 + + # -------------------------------------------------------------- + # 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" exit 0 ;; esac # Bots (dependabot, renovate, actions-user) are always allowed. - if [ "${PR_AUTHOR_TYPE:-}" = "Bot" ]; then - allow "bot" + if [ "$PR_AUTHOR_TYPE" = "Bot" ]; then + echo "allow=true" >> "$GITHUB_OUTPUT" + echo "reason=bot" >> "$GITHUB_OUTPUT" + echo "$PR_AUTHOR: bot -- ALLOWED" exit 0 fi - case "${PR_ASSOCIATION:-}" in - OWNER|MEMBER|COLLABORATOR) - allow "author_association=${PR_ASSOCIATION}" - exit 0 ;; - CONTRIBUTOR|FIRST_TIMER|FIRST_TIME_CONTRIBUTOR|MANNEQUIN|NONE) + author_state=$(membership "$PR_AUTHOR") + echo "author=${PR_AUTHOR} type=${PR_AUTHOR_TYPE} membership=${author_state}" + + 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) echo "allow=false" >> "$GITHUB_OUTPUT" - 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 ;; + echo "reason=not an active ${ORG} org member and not allowlisted" >> "$GITHUB_OUTPUT" + echo "$PR_AUTHOR: not a member -- REJECTED" ;; *) - # 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)" + # 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" exit 1 ;; esac - name: Close unauthorized PR - if: ${{ !cancelled() && steps.authz.outputs.allow == 'false' }} + if: steps.authz.outputs.allow != 'true' 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 deleted file mode 100755 index 4ab7b85..0000000 --- a/tests/pr-gate-authorize-test.sh +++ /dev/null @@ -1,64 +0,0 @@ -#!/usr/bin/env bash -# Test for the `authorize` step of .github/workflows/pr-gate.yml (aprender#5951). -# 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 -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; } -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 -echo "gh $*" >> "$GH_CALLS"; exit 99 -STUB -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 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 3; fail=$((fail+1)) - fi -} -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 "" - -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 ]