fix(pr-review): API-fallback collaborator check + authorized_users input (#253)

[cbeaulieu-gt]

Closes #253

Summary

Fixes a pre-existing bug in pr-review/action.yml where self-PRs by maintainers with private org membership were blocked as "external" because GitHub does not disclose private membership in pull_request* event payloads — author_association returns CONTRIBUTOR even for actual MEMBER/admin users. Concrete reproducer: glitchwerks/mom-bot#20 (run 25602562131, log line AUTHOR_ASSOCIATION: CONTRIBUTOR despite cbeaulieu-gt being repo admin).

Three-step authorization

The auth gate now evaluates in this order, authorizing on the first hit:

1. authorized_users CSV match — explicit allowlist input (highest precedence). Whitespace-tolerant, case-insensitive, comma-boundary matching prevents substring collisions (bob vs bobby).
2. author_association fast-path — OWNER / MEMBER / COLLABORATOR. Unchanged behavior for public-membership cases.
3. API-fallback collaborator check — gh api repos/$REPO/collaborators/$ACTOR. HTTP 204 → authorize; non-zero exit (404, 403, 429, network error) → block. Any non-zero exit is intentionally treated as "not a collaborator" — no silent authorization on transient errors.

Changes

• pr-review/action.yml — adds authorized_users composite-action input; rewrites the "Check author association" step with the three-step logic above; documents the failure mode.
• .github/workflows/claude-pr-review.yml — adds authorized_users to workflow_call.inputs; threads through to the composite step.

Backwards compatibility

External consumers who do not set authorized_users retain identical behavior for any case where step 2 (author_association) would have authorized. The new step 3 (API fallback) only fires when step 2 would have blocked — strictly additive.

Test plan

• CI: actionlint passes (verified locally on both commits).
• Validation via floated-tag-on-external-consumer (the verification rule from glitchwerks/claude-configs skills/github-actions/SKILL.md § 13 applies — this PR's own dogfood will execute the OLD @v2 code, not the branch code, because pr-review/action.yml is itself a self-referenced composite action).
• After release, glitchwerks/mom-bot#20 re-run authorizes cbeaulieu-gt via step 3 (API fallback) and the review actually runs.
• authorized_users allowlist verified by setting it to a known username on a temporary PR; verifies CSV parsing tolerates "a, b, c" whitespace.

Notes

• This change does not refactor pr-review/action.yml to delegate to the existing check-auth/ action. That was option (3) in fix(pr-review): author_association check is blind to private org membership — self-PRs get blocked as external #253; the user picked options (1)+(2). A future cleanup issue can fold the auth logic into check-auth/ if duplication becomes a maintenance burden.
• Two commits: implementation + a follow-up CSV whitespace-tolerance fix from the code review pass.

🤖 Generated by Claude Code on behalf of @cbeaulieu-gt

[github-actions]

Claude finished @cbeaulieu-gt's task in 4m 21s —— View job

---

PR Review Complete

Reviewed PR #254: fix(pr-review): API-fallback collaborator check + authorized_users input

---

Summary

This PR successfully addresses the private org membership authorization bug (#253) with a well-structured three-step authorization flow. The implementation is secure, handles edge cases correctly, and maintains backwards compatibility. The code is thoroughly documented with clear inline comments explaining the rationale for each step.

Key strengths:

• Fail-secure error handling (API errors block authorization, never silently grant access)
• Robust CSV parsing with case-insensitive matching and boundary-comma technique preventing substring collisions
• Comprehensive inline documentation explaining the authorization flow and failure modes
• Proper precedence ordering (explicit allowlist > event payload > API fallback)

Code security analysis:

• No injection vulnerabilities identified (GitHub username constraints + proper quoting prevent regex/command injection)
• No TOCTOU races that could compromise authorization
• Appropriate use of URL encoding in API calls via gh api
• Correct fail-secure behavior on transient errors

Backwards compatibility verified:

• External consumers not setting authorized_users retain identical behavior for all previously-authorized cases
• Step 3 (API fallback) only activates when step 2 would have blocked — strictly additive ✓

---

Findings

🟢 Medium

API rate limiting implications not documented in user-facing guidance

The API-fallback collaborator check (pr-review/action.yml:213) will invoke gh api repos/$REPO/collaborators/$ACTOR for every PR from a user whose author_association is not OWNER/MEMBER/COLLABORATOR. In repositories with many contributors who have private org membership, this could approach API rate limits (typically 1000 requests/hour for GITHUB_TOKEN).

While this is acknowledged as an accepted trade-off in the PR description, it is not documented in the action's user-facing description or usage guidance. Consider adding a note to the authorized_users input description (pr-review/action.yml:11-13) mentioning:

• The API call is made only when author_association check doesn't authorize
• Each API call counts against the repository's GITHUB_TOKEN rate limit
• High-traffic repos may want to proactively populate authorized_users for known private-org members to avoid the API call

Location: pr-review/action.yml:11-14

---

Nit

Step name no longer reflects full scope

The step name "Check author association" (pr-review/action.yml:149) now under-represents the step's functionality, which includes:

1. authorized_users CSV allowlist checking
2. author_association field checking
3. API-fallback collaborator verification

Consider renaming to "Check authorization" or "Authorize PR author" for accuracy.

Location: pr-review/action.yml:149

---

Nit

Comment wording imprecise about whitespace handling

The comment on lines 187-190 states "an unstripped " bob " would silently fail to match "bob"" but the implementation uses tr -d '[:space:]' which removes ALL occurrences of whitespace characters from the entire string, not just leading/trailing whitespace around individual tokens.

While the behavior is correct and safe (GitHub usernames cannot contain spaces), the comment could be more precise:

# Whitespace is stripped from both the CSV list and the actor name before
# comparison, so "alice, bob, charlie" becomes "alice,bob,charlie" — this
# tolerates copy-paste from human-written lists and ensures " bob " matches "bob".

Location: pr-review/action.yml:187-190

---

Edge Cases Verified

The CSV parsing correctly handles:

• ✓ Whitespace variations: "alice, bob", "alice,bob", "alice ,bob" all equivalent
• ✓ Case insensitivity: "Bob" matches actor "bob"
• ✓ Substring collision prevention: "bob" in list does not match actor "bobby" (boundary-comma technique)
• ✓ Empty input: authorized_users: "" safely skips step 1 via [ -n "$AUTHORIZED_USERS" ] guard
• ✓ Trailing/leading commas: "alice,bob," and ",alice,bob" parse correctly
• ✓ Multiple consecutive commas: "alice,,bob" (treated as empty tokens, actor "bob" still matches)
• ✓ Whitespace-only input: becomes empty string after tr -d '[:space:]', safely skipped

The API fallback correctly handles:

• ✓ Network errors: exit non-zero → block authorization (fail-secure)
• ✓ Rate limit errors (429): exit non-zero → block authorization (documented trade-off)
• ✓ 404 Not Found: exit non-zero → block authorization (expected for non-collaborators)
• ✓ 204 No Content: exit 0 → authorize (expected for collaborators)

---

Test Plan Status

From PR description:

• CI: actionlint passes
• Validation via floated-tag-on-external-consumer (standard for self-referenced composite actions)
• Post-release verification on glitchwerks/mom-bot#20
• authorized_users CSV parsing validation

The incomplete test items are standard for pre-merge state and should be completed post-release per the repository's validation workflow.

---

Verdict

Verdict: APPROVE

---

[cbeaulieu-gt]

v2.6.0 release withdrawn. This PR was shipped as part of v2.6.0, but the release was rolled back: v2 floating tag is now back at e92a334 (v2.5.1).

The API-fallback collaborator check added here cannot enumerate collaborators with github.token's default scopes — pull_request_target runs return non-zero from gh api repos/.../collaborators/<user> because the bot identity lacks the push relationship the endpoint requires. Tracking under #255.

The CSV authorized_users allowlist (step 1) and author_association fast-path (step 2) work correctly; the API fallback (step 3) does not. A successor PR will redesign step 3 — likely moving the check to use the App token instead of github.token, after the App-token resolve step (which moves it from "bonus protection" to "primary mechanism", aligning with #250's identity model).

Code on main is unchanged; only the v2 distribution pointer was rolled back.

🤖 Generated by Claude Code on behalf of @cbeaulieu-gt