diff --git a/.agents/skills/add-pr-reviewer-to-repo/SKILL.md b/.agents/skills/add-pr-reviewer-to-repo/SKILL.md index 63b5a76..d25c22b 100644 --- a/.agents/skills/add-pr-reviewer-to-repo/SKILL.md +++ b/.agents/skills/add-pr-reviewer-to-repo/SKILL.md @@ -17,6 +17,8 @@ Use this skill when you are asked to add AI-powered PR review to a repo, or to u > **To re-trigger:** re-request a review from `docker-agent` in the sidebar (click the refresh icon next to their name). This fires a `review_requested` event and starts a fresh review. > > **`/review` comment:** still works but is deprecated. Prefer the sidebar workflow. +> +> **External / fork contributors:** auto-review only runs on org members' own PRs. To review an external contributor's PR, an org member requests `docker-agent` as a reviewer — the review is authorized by the **requester**, not the PR author. Make sure to communicate this to contributors when onboarding a repo — it's the main daily interaction pattern and easy to miss if someone only reads the workflow YAML. @@ -29,7 +31,7 @@ The reusable workflow handles all of the following internally. **Do not add call | Concern | How it's handled internally | | ------- | --------------------------- | | **Bot comment filtering** | All jobs in the reusable workflow carry comprehensive `if:` conditions that skip `docker-agent`, `docker-agent[bot]`, any `Bot`-type user, and comments containing `` / `` HTML markers. | -| **Org membership / authorization** | A dedicated `check-org-membership` step runs before any review work begins. PR authors and comment authors are verified as org members or collaborators. Callers never need their own `author_association` checks. | +| **Org membership / authorization** | A dedicated `check-org-membership` step runs before any review work begins. Auto-review verifies the **PR author**; a requested review verifies the **requester** (so a maintainer can pull an external contributor's PR into review); comment paths verify the commenter. Callers never need their own `author_association` checks. | | **PR vs issue comment disambiguation** | The reusable workflow checks `github.event.issue.pull_request` internally. Plain issue comments on non-PR issues are ignored automatically. | | **Draft PR skipping** | Handled internally — draft PRs are not reviewed. | | **Concurrent review guard** | A cache-based lock (`pr-review-lock---*`) prevents duplicate reviews from racing on the same PR. | diff --git a/.github/workflows/review-pr.yml b/.github/workflows/review-pr.yml index cfd2730..59aaf6c 100644 --- a/.github/workflows/review-pr.yml +++ b/.github/workflows/review-pr.yml @@ -305,6 +305,14 @@ jobs: PR_SOURCE: ${{ steps.pr.outputs.source }} ORG: docker COMMENT_AUTHOR: ${{ github.event.comment.user.login }} + # review_requested authorizes the requesting maintainer, not the PR + # author, so an external contributor's PR can be reviewed on request. + # On the direct same-repo path the requester is the trusted event sender; + # on the fork/workflow_run path check-org-membership re-derives it from + # the PR timeline (the artifact is never trusted for authorization). + EVENT_NAME: ${{ github.event_name }} + EVENT_ACTION: ${{ github.event.action }} + REQUESTER: ${{ github.event.sender.login }} run: node "$DOCKER_AGENT_ACTION_ROOT/dist/check-org-membership.js" - name: Create check run diff --git a/AGENTS.md b/AGENTS.md index ea903c2..c29eb5e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,7 +30,7 @@ Anything else here (workflows under `.github/workflows/`, scripts, tests) exists │ ├── add-reaction/ # Adds emoji reactions to issue/PR comments. │ │ ├── index.ts # Entry → bundled to dist/add-reaction.js │ │ └── __tests__/ -│ ├── check-org-membership/ # Verifies a user belongs to a GitHub org; also resolves PR author via pulls.get. +│ ├── check-org-membership/ # Authorizes a review: auto-run on PR-author membership, review_requested on the (trusted, timeline-derived) requester. Resolves PR author via pulls.get. │ │ ├── index.ts # Entry → bundled to dist/check-org-membership.js (standalone CLI + library). │ │ └── __tests__/ │ ├── credentials/ # Fetches AWS secrets via OIDC, exports PAT and AI keys. diff --git a/review-pr/README.md b/review-pr/README.md index 097d327..83905fe 100644 --- a/review-pr/README.md +++ b/review-pr/README.md @@ -167,15 +167,15 @@ with: | Trigger | Behavior | | ------------------------------------ | ---------------------------------------------------------------------------------------------------------------------------------------- | -| Request review from `docker-agent` | **Primary trigger.** Add `docker-agent` as a reviewer in the PR sidebar — review starts automatically, shown as a check run. | -| PR opened/ready | Auto-reviews when a PR is opened or marked ready for review. | +| Request review from `docker-agent` | **Primary trigger.** Add `docker-agent` as a reviewer in the PR sidebar — review starts automatically, shown as a check run. Authorized by the requesting org member, so it also works for external/fork contributors' PRs. | +| PR opened/ready | Auto-reviews when a PR is opened or marked ready for review (org-member-authored PRs). | | ~~`/review`~~ _(deprecated)_ | Re-trigger a review, or trigger manually when auto-review hasn't run (e.g. after a force-push). Shows as a check run if `checks: write` is granted. | | Reply to review comment | Responds in-thread and captures feedback to improve future reviews. | | `@docker-agent` mention | Answers questions and clarifies review findings. Works in both PR-level issue comments and inline file-line review comments, including on fork PRs (via the trigger workflow). | > **Built-in defense-in-depth:** > -> 1. **Verifies org membership** before every review (auto-review checks the PR author; `/review` checks the commenter) +> 1. **Verifies org membership** before every review. Auto-review checks the **PR author** (so only org members' PRs are reviewed automatically); a **requested review** checks the **requester**, so a maintainer can pull an external contributor's PR into review on demand; `/review` checks the commenter > 2. **Prevents bot cascades** — replies from bots (except `docker-agent`) are ignored > 3. **Fork PRs work automatically** with the two-workflow setup — the trigger → `workflow_run` pattern provides OIDC/secret access regardless of fork status @@ -186,7 +186,7 @@ The workflow YAML examples above are the complete, recommended setup. The reusab | Protection | How it's handled | | ---------- | ---------------- | | **Bot comment filtering** | All jobs in the reusable workflow filter out `docker-agent`, `docker-agent[bot]`, any `Bot`-type user, and comments with ``/`` markers. No caller-side filtering needed. | -| **Org membership / authorization** | A `check-org-membership` step runs before any review work. PR authors and comment authors are verified as org members or collaborators via OIDC. Callers never need `author_association` checks. | +| **Org membership / authorization** | A `check-org-membership` step runs before any review work. Auto-review verifies the **PR author**; a requested review verifies the **requester** (so an external contributor's PR can be reviewed when an org member requests it); comment / `/review` paths verify the commenter. All via OIDC. Callers never need `author_association` checks. | | **PR vs issue comment** | The reusable workflow checks `github.event.issue.pull_request` internally. Plain issue comments on non-PR issues are silently ignored. | | **Draft PR skipping** | Draft PRs are skipped internally — no caller condition needed. | | **Concurrent review guard** | A cache-based lock (`pr-review-lock---*`) prevents duplicate reviews from racing on the same PR. | diff --git a/src/check-org-membership/__tests__/check-org-membership.test.ts b/src/check-org-membership/__tests__/check-org-membership.test.ts index 41c9031..d6ec667 100644 --- a/src/check-org-membership/__tests__/check-org-membership.test.ts +++ b/src/check-org-membership/__tests__/check-org-membership.test.ts @@ -5,32 +5,54 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; vi.mock('@actions/core'); -const { mockCheckMembershipForUser, mockGetPull, MockOctokit, constructorTokens } = vi.hoisted( - () => { - const mockCheckMembershipForUser = vi.fn().mockResolvedValue({}); // 204 = member - const mockGetPull = vi.fn().mockResolvedValue({ data: { user: { login: 'bob' } } }); - - // Track which auth token was passed to each new Octokit() instance, in order. - // Index 0 = first instance created, 1 = second, etc. - const constructorTokens: string[] = []; - - class MockOctokit { - constructor({ auth }: { auth: string }) { - constructorTokens.push(auth); - } - rest = { - orgs: { checkMembershipForUser: mockCheckMembershipForUser }, - pulls: { get: mockGetPull }, - }; +const { + mockCheckMembershipForUser, + mockGetPull, + mockListEventsForTimeline, + mockPaginate, + MockOctokit, + constructorTokens, +} = vi.hoisted(() => { + const mockCheckMembershipForUser = vi.fn().mockResolvedValue({}); // 204 = member + const mockGetPull = vi.fn().mockResolvedValue({ data: { user: { login: 'bob' } } }); + const mockListEventsForTimeline = vi.fn(); + const mockPaginate = vi.fn().mockResolvedValue([]); + + // Track which auth token was passed to each new Octokit() instance, in order. + // Index 0 = first instance created, 1 = second, etc. + const constructorTokens: string[] = []; + + class MockOctokit { + paginate = mockPaginate; + rest = { + orgs: { checkMembershipForUser: mockCheckMembershipForUser }, + pulls: { get: mockGetPull }, + issues: { listEventsForTimeline: mockListEventsForTimeline }, + }; + constructor({ auth }: { auth: string }) { + constructorTokens.push(auth); } - - return { mockCheckMembershipForUser, mockGetPull, MockOctokit, constructorTokens }; - }, -); + } + + return { + mockCheckMembershipForUser, + mockGetPull, + mockListEventsForTimeline, + mockPaginate, + MockOctokit, + constructorTokens, + }; +}); vi.mock('@octokit/rest', () => ({ Octokit: MockOctokit })); -import { checkOrgMembership, resolvePrAuthor } from '../index.js'; +import { + checkOrgMembership, + evaluateMembership, + type MembershipInputs, + resolvePrAuthor, + resolveReviewRequester, +} from '../index.js'; const ORG_TOKEN = 'fake-org-token'; const REPO_TOKEN = 'fake-repo-token'; @@ -38,9 +60,56 @@ const ORG = 'docker'; const USERNAME = 'alice'; beforeEach(() => { + vi.clearAllMocks(); constructorTokens.length = 0; }); +/** Make checkOrgMembership resolve "member" only for the listed logins. */ +function membersAre(...members: string[]): void { + mockCheckMembershipForUser.mockImplementation(({ username }: { username: string }) => + members.includes(username) + ? Promise.resolve({}) + : Promise.reject(Object.assign(new Error('Not Found'), { status: 404 })), + ); +} + +/** Build a timeline review_requested event. */ +function reviewRequestedEvent(requester: string, reviewer = 'docker-agent') { + return { + event: 'review_requested', + actor: { login: requester }, + review_requester: { login: requester }, + requested_reviewer: { login: reviewer }, + }; +} + +/** Build a timeline review_request_removed event. */ +function reviewRequestRemovedEvent(remover: string, reviewer = 'docker-agent') { + return { + event: 'review_request_removed', + actor: { login: remover }, + review_requester: { login: remover }, + requested_reviewer: { login: reviewer }, + }; +} + +function inputs(overrides: Partial = {}): MembershipInputs { + return { + orgToken: ORG_TOKEN, + repoToken: REPO_TOKEN, + org: ORG, + reviewerLogin: 'docker-agent', + repository: 'docker/myrepo', + prSource: 'event', + eventName: 'pull_request', + eventAction: 'opened', + prNumber: 1, + commentAuthor: '', + trustedRequester: '', + ...overrides, + }; +} + // --------------------------------------------------------------------------- // checkOrgMembership // --------------------------------------------------------------------------- @@ -136,19 +205,332 @@ describe('resolvePrAuthor', () => { }); // --------------------------------------------------------------------------- -// Token isolation: checkOrgMembership uses ORG_TOKEN, resolvePrAuthor uses REPO_TOKEN +// resolveReviewRequester (trusted, timeline-derived) // --------------------------------------------------------------------------- -describe('token isolation', () => { - it('checkOrgMembership uses orgToken, resolvePrAuthor uses repoToken — never swapped', async () => { - mockCheckMembershipForUser.mockResolvedValueOnce({}); - mockGetPull.mockResolvedValueOnce({ data: { user: { login: 'eve' } } }); +describe('resolveReviewRequester', () => { + it('returns the requester of the latest review_requested event for the reviewer', async () => { + mockPaginate.mockResolvedValueOnce([ + { event: 'labeled' }, + reviewRequestedEvent('early-maintainer'), + reviewRequestedEvent('latest-maintainer'), + ]); + + const requester = await resolveReviewRequester( + REPO_TOKEN, + 'docker', + 'myrepo', + 7, + 'docker-agent', + ); + + expect(requester).toBe('latest-maintainer'); + expect(mockPaginate).toHaveBeenCalledWith(mockListEventsForTimeline, { + owner: 'docker', + repo: 'myrepo', + issue_number: 7, + per_page: 100, + }); + }); + + it('ignores review_requested events targeting a different reviewer', async () => { + mockPaginate.mockResolvedValueOnce([reviewRequestedEvent('maintainer', 'someone-else')]); + + const requester = await resolveReviewRequester( + REPO_TOKEN, + 'docker', + 'myrepo', + 7, + 'docker-agent', + ); + + expect(requester).toBe(''); + }); + + it('returns empty string when there is no review request', async () => { + mockPaginate.mockResolvedValueOnce([{ event: 'commented' }, { event: 'labeled' }]); + + const requester = await resolveReviewRequester( + REPO_TOKEN, + 'docker', + 'myrepo', + 7, + 'docker-agent', + ); + + expect(requester).toBe(''); + }); + + it('falls back to actor.login when review_requester is absent', async () => { + mockPaginate.mockResolvedValueOnce([ + { + event: 'review_requested', + actor: { login: 'actor-m' }, + requested_reviewer: { login: 'docker-agent' }, + }, + ]); + + const requester = await resolveReviewRequester( + REPO_TOKEN, + 'docker', + 'myrepo', + 7, + 'docker-agent', + ); + + expect(requester).toBe('actor-m'); + }); + + it('returns empty string when the latest event removes the review request', async () => { + mockPaginate.mockResolvedValueOnce([ + reviewRequestedEvent('maintainer'), + reviewRequestRemovedEvent('maintainer'), + ]); + + const requester = await resolveReviewRequester( + REPO_TOKEN, + 'docker', + 'myrepo', + 7, + 'docker-agent', + ); + + expect(requester).toBe(''); + }); + + it('honors a re-request after a removal (latest still-effective request wins)', async () => { + mockPaginate.mockResolvedValueOnce([ + reviewRequestedEvent('maintainer'), + reviewRequestRemovedEvent('maintainer'), + reviewRequestedEvent('later-maintainer'), + ]); + + const requester = await resolveReviewRequester( + REPO_TOKEN, + 'docker', + 'myrepo', + 7, + 'docker-agent', + ); + + expect(requester).toBe('later-maintainer'); + }); + + it('replays chronologically even when the API returns events out of order', async () => { + // The removal happened after the request, but the timeline came back reversed. + // Sorting by created_at must still let the later retraction win, so a stale + // request never authorizes a fork PR regardless of array order. + mockPaginate.mockResolvedValueOnce([ + { ...reviewRequestRemovedEvent('maintainer'), created_at: '2026-06-25T13:00:00Z' }, + { ...reviewRequestedEvent('maintainer'), created_at: '2026-06-25T12:00:00Z' }, + ]); + + const requester = await resolveReviewRequester( + REPO_TOKEN, + 'docker', + 'myrepo', + 7, + 'docker-agent', + ); + + expect(requester).toBe(''); + }); + + it('ignores a removal targeting a different reviewer', async () => { + mockPaginate.mockResolvedValueOnce([ + reviewRequestedEvent('maintainer'), + reviewRequestRemovedEvent('maintainer', 'someone-else'), + ]); + + const requester = await resolveReviewRequester( + REPO_TOKEN, + 'docker', + 'myrepo', + 7, + 'docker-agent', + ); + + expect(requester).toBe('maintainer'); + }); + + it('uses the repo token', async () => { + mockPaginate.mockResolvedValueOnce([]); + + await resolveReviewRequester(REPO_TOKEN, 'docker', 'myrepo', 7, 'docker-agent'); + + expect(constructorTokens).toEqual([REPO_TOKEN]); + }); +}); + +// --------------------------------------------------------------------------- +// evaluateMembership — the two authorization paths +// --------------------------------------------------------------------------- + +describe('evaluateMembership', () => { + it('auto-run: authorizes an org-member PR author (via author)', async () => { + membersAre('alice-author'); + mockGetPull.mockResolvedValueOnce({ data: { user: { login: 'alice-author' } } }); - await checkOrgMembership(ORG_TOKEN, ORG, USERNAME); - await resolvePrAuthor(REPO_TOKEN, 'docker', 'myrepo', 5); + const decision = await evaluateMembership(inputs({ eventAction: 'synchronize', prNumber: 3 })); - // First Octokit instance (for checkOrgMembership) must use ORG_TOKEN - // Second Octokit instance (for resolvePrAuthor) must use REPO_TOKEN - expect(constructorTokens).toEqual([ORG_TOKEN, REPO_TOKEN]); + expect(decision).toEqual({ isMember: true, subject: 'alice-author', via: 'author' }); + // Author membership is enough — no requester lookup needed. + expect(mockPaginate).not.toHaveBeenCalled(); + }); + + it('auto-run: denies an external (non-member) PR author', async () => { + membersAre(); // nobody is a member + mockGetPull.mockResolvedValueOnce({ data: { user: { login: 'ext-author' } } }); + + const decision = await evaluateMembership(inputs({ eventAction: 'opened' })); + + expect(decision).toEqual({ isMember: false, subject: 'ext-author', via: 'none' }); + expect(mockPaginate).not.toHaveBeenCalled(); + }); + + it('review_requested (direct): authorizes an external PR via the requesting maintainer', async () => { + membersAre('maintainer'); + mockGetPull.mockResolvedValueOnce({ data: { user: { login: 'ext-author' } } }); + + const decision = await evaluateMembership( + inputs({ eventAction: 'review_requested', trustedRequester: 'maintainer' }), + ); + + expect(decision).toEqual({ isMember: true, subject: 'maintainer', via: 'requester' }); + // Direct path uses the trusted event sender — no timeline lookup. + expect(mockPaginate).not.toHaveBeenCalled(); + }); + + it('review_requested: the REQUESTER env value is only trusted on the direct same-repo triple', async () => { + // Defense-in-depth: a PR_SOURCE=event path whose event is NOT a + // pull_request:review_requested must never trust the env-supplied requester, + // even if that login is a real org member. Here an auto-run "opened" event + // carries a member login in trustedRequester; it must be ignored. + membersAre('sneaky-member'); + mockGetPull.mockResolvedValueOnce({ data: { user: { login: 'ext-author' } } }); + + const decision = await evaluateMembership( + inputs({ eventAction: 'opened', trustedRequester: 'sneaky-member' }), + ); + + expect(decision).toEqual({ isMember: false, subject: 'ext-author', via: 'none' }); + expect(mockPaginate).not.toHaveBeenCalled(); + }); + + it('review_requested (direct): denies when the requester is not an org member', async () => { + membersAre(); // neither author nor requester is a member + mockGetPull.mockResolvedValueOnce({ data: { user: { login: 'ext-author' } } }); + + const decision = await evaluateMembership( + inputs({ eventAction: 'review_requested', trustedRequester: 'outsider' }), + ); + + // The denial subject is the requester who actually failed path 2, not the author. + expect(decision).toEqual({ isMember: false, subject: 'outsider', via: 'none' }); + }); + + it('review_requested (fork/trigger): authorizes via the timeline-derived requester', async () => { + membersAre('maintainer'); + mockGetPull.mockResolvedValueOnce({ data: { user: { login: 'ext-author' } } }); + mockPaginate.mockResolvedValueOnce([reviewRequestedEvent('maintainer')]); + + const decision = await evaluateMembership( + inputs({ + prSource: 'trigger', + eventName: 'workflow_run', + eventAction: 'completed', + prNumber: 7, + }), + ); + + expect(decision).toEqual({ isMember: true, subject: 'maintainer', via: 'requester' }); + }); + + it('fork/trigger auto-run (no review requested): denies an external PR', async () => { + membersAre('maintainer'); + mockGetPull.mockResolvedValueOnce({ data: { user: { login: 'ext-author' } } }); + mockPaginate.mockResolvedValueOnce([]); // no review_requested event in the timeline + + const decision = await evaluateMembership( + inputs({ + prSource: 'trigger', + eventName: 'workflow_run', + eventAction: 'completed', + prNumber: 7, + }), + ); + + expect(decision).toEqual({ isMember: false, subject: 'ext-author', via: 'none' }); + }); + + it('fork/trigger: a forged timeline requester is still validated against real org membership', async () => { + // Even if the (fork-influenced) PR claimed a member, the requester is taken + // from the trusted timeline AND re-checked against the org. A non-member there + // is denied, and the denial subject names that non-member requester. + membersAre(); // the timeline actor is NOT actually a member + mockGetPull.mockResolvedValueOnce({ data: { user: { login: 'ext-author' } } }); + mockPaginate.mockResolvedValueOnce([reviewRequestedEvent('not-a-member')]); + + const decision = await evaluateMembership( + inputs({ + prSource: 'trigger', + eventName: 'workflow_run', + eventAction: 'completed', + prNumber: 7, + }), + ); + + expect(decision).toEqual({ isMember: false, subject: 'not-a-member', via: 'none' }); + }); + + it('fork/trigger: a retracted review request does not authorize an external PR', async () => { + // Access-control regression: a maintainer requested docker-agent then removed + // that request. The stale requester must NOT authorize the external PR. + membersAre('maintainer'); + mockGetPull.mockResolvedValueOnce({ data: { user: { login: 'ext-author' } } }); + mockPaginate.mockResolvedValueOnce([ + reviewRequestedEvent('maintainer'), + reviewRequestRemovedEvent('maintainer'), + ]); + + const decision = await evaluateMembership( + inputs({ + prSource: 'trigger', + eventName: 'workflow_run', + eventAction: 'completed', + prNumber: 7, + }), + ); + + expect(decision).toEqual({ isMember: false, subject: 'ext-author', via: 'none' }); + }); + + it('issue_comment: authorizes an org-member commenter (via comment)', async () => { + membersAre('commenter'); + + const decision = await evaluateMembership( + inputs({ eventName: 'issue_comment', eventAction: 'created', commentAuthor: 'commenter' }), + ); + + expect(decision).toEqual({ isMember: true, subject: 'commenter', via: 'comment' }); + // Comment path never resolves the PR author or the timeline. + expect(mockGetPull).not.toHaveBeenCalled(); + expect(mockPaginate).not.toHaveBeenCalled(); + }); + + it('issue_comment: denies a non-member commenter', async () => { + membersAre(); + + const decision = await evaluateMembership( + inputs({ eventName: 'issue_comment', eventAction: 'created', commentAuthor: 'ext' }), + ); + + expect(decision).toEqual({ isMember: false, subject: 'ext', via: 'none' }); + }); + + it('throws on an invalid PR number for PR-driven paths', async () => { + await expect(evaluateMembership(inputs({ prNumber: Number.NaN }))).rejects.toThrow( + /Invalid pr-number/, + ); }); }); diff --git a/src/check-org-membership/index.ts b/src/check-org-membership/index.ts index 4c275d0..65f7ab6 100644 --- a/src/check-org-membership/index.ts +++ b/src/check-org-membership/index.ts @@ -2,11 +2,27 @@ // SPDX-License-Identifier: Apache-2.0 /** - * check-org-membership — verify whether a GitHub user belongs to an org. + * check-org-membership — decide whether a PR review is authorized. + * + * Two authorization paths feed the same review pipeline: + * 1. Auto-run — a PR opened/updated by an org MEMBER is reviewed + * automatically. Authorized on the PR AUTHOR's membership. + * 2. Review-requested — when `docker-agent` is requested as a reviewer, the + * review is authorized on the REQUESTER's membership, not + * the author's. This lets a maintainer pull an EXTERNAL + * contributor's PR into the review pipeline on demand. + * + * Security note: the requester is only ever taken from a TRUSTED source — the + * `github.event.sender.login` of a same-repo pull_request event, or (on the + * fork / workflow_run path) re-derived from the PR timeline via the GitHub API. + * It is NEVER read from the trigger artifact, because a fork PR controls its own + * trigger workflow and could otherwise forge a member login. * * Exported functions: * checkOrgMembership(orgToken, org, username) → boolean * resolvePrAuthor(repoToken, owner, repo, prNumber) → string + * resolveReviewRequester(repoToken, owner, repo, prNumber, reviewerLogin) → string + * evaluateMembership(inputs) → { isMember, subject, via } * * CLI (invoked as a shell run step via dist/check-org-membership.js): * All inputs are read from environment variables: @@ -15,8 +31,12 @@ * GITHUB_REPOSITORY "owner/repo" (standard GitHub Actions env var) * ORG GitHub org name to check (e.g. "docker") * PR_SOURCE "event" | "trigger" | "input" - * PR_NUMBER PR number as string (required when PR_SOURCE != 'event') - * COMMENT_AUTHOR User login (required when PR_SOURCE == 'event') + * PR_NUMBER PR number as string (required for PR-driven paths) + * COMMENT_AUTHOR User login (used on the issue_comment path) + * EVENT_NAME github.event_name (issue_comment | pull_request | …) + * EVENT_ACTION github.event.action (e.g. "review_requested") + * REQUESTER github.event.sender.login (trusted; direct path only) + * AGENT_LOGIN Reviewer login to match (default "docker-agent") * * Outputs are written via @actions/core.setOutput (writes to $GITHUB_OUTPUT): * is_member "true" | "false" @@ -83,6 +103,193 @@ export async function resolvePrAuthor( return pr.user?.login ?? ''; } +// --------------------------------------------------------------------------- +// Core function: review-requester resolution (trusted, server-side) +// --------------------------------------------------------------------------- + +interface TimelineReviewEvent { + /** + * 'review_requested' grants the request, 'review_request_removed' revokes it; + * any other timeline event type is ignored. Typed as a literal union (widened + * with `string` so the cast below still accepts the full timeline) to surface + * the revocation case to future readers. + */ + event?: 'review_requested' | 'review_request_removed' | (string & {}); + created_at?: string; + actor?: { login?: string } | null; + review_requester?: { login?: string } | null; + requested_reviewer?: { login?: string } | null; +} + +/** + * Return the login of the user whose review request for `reviewerLogin` is still + * in effect on the PR, or '' if there is no such request (or it was revoked). + * + * Derived from the PR timeline via the GitHub API using `repoToken`. This is the + * authoritative, non-forgeable source for the requester on the fork / workflow_run + * path, where the triggering event payload is not directly available and the + * trigger artifact is written in the untrusted fork context. + */ +export async function resolveReviewRequester( + repoToken: string, + owner: string, + repo: string, + prNumber: number, + reviewerLogin: string, +): Promise { + const octokit = new Octokit({ auth: repoToken }); + const events = (await octokit.paginate(octokit.rest.issues.listEventsForTimeline, { + owner, + repo, + issue_number: prNumber, + per_page: 100, + })) as unknown as TimelineReviewEvent[]; + + // Replay request/removal events for this reviewer in chronological order: a + // 'review_requested' sets the current requester, a later 'review_request_removed' + // clears it. This handles re-request-after-removal correctly and, critically, + // ensures a retracted request never authorizes the review on the fork / + // workflow_run path. Sort by created_at rather than trusting the array order: the + // timeline endpoint returns ascending order in practice but does not guarantee it, + // and an out-of-order request/removal pair would otherwise leave a retracted + // request live — which the org-membership re-check cannot catch, since the stale + // login is a real member. (ISO-8601 timestamps sort lexicographically.) + events.sort((a, b) => { + const at = a.created_at ?? ''; + const bt = b.created_at ?? ''; + return at < bt ? -1 : at > bt ? 1 : 0; + }); + + let requester = ''; + for (const ev of events) { + if (ev.requested_reviewer?.login !== reviewerLogin) continue; + if (ev.event === 'review_requested') { + requester = ev.review_requester?.login ?? ev.actor?.login ?? ''; + } else if (ev.event === 'review_request_removed') { + requester = ''; + } + } + return requester; +} + +// --------------------------------------------------------------------------- +// Authorization decision +// --------------------------------------------------------------------------- + +export interface MembershipInputs { + orgToken: string; + repoToken: string; + org: string; + reviewerLogin: string; + repository: string; + prSource: string; + eventName: string; + eventAction: string; + prNumber: number; + commentAuthor: string; + /** Trusted requester login from github.event.sender.login (direct path only). */ + trustedRequester: string; +} + +export interface MembershipDecision { + isMember: boolean; + subject: string; + via: 'comment' | 'author' | 'requester' | 'none'; +} + +function parseRepository(repository: string): { owner: string; repo: string } { + const slashIdx = repository.indexOf('/'); + if (slashIdx < 0) { + throw new Error(`Invalid GITHUB_REPOSITORY: '${repository}' (expected 'owner/repo')`); + } + return { owner: repository.slice(0, slashIdx), repo: repository.slice(slashIdx + 1) }; +} + +/** + * Resolve the review requester from a trusted source only. + * - Direct same-repo pull_request review_requested → github.event.sender.login. + * - Fork / workflow_run path → re-derived from the PR timeline (server-side). + * Returns '' when there is no trustworthy requester. + */ +async function resolveTrustedRequester( + inputs: MembershipInputs, + owner: string, + repo: string, +): Promise { + const { prSource, eventName, eventAction, trustedRequester } = inputs; + // SECURITY: trustedRequester (REQUESTER = github.event.sender.login) is only + // authoritative for a DIRECT same-repo pull_request:review_requested event, + // which the workflow tags PR_SOURCE=event. The fork / workflow_run path is + // tagged PR_SOURCE=trigger and is resolved from the server-side timeline below, + // because its trigger artifact is written in the untrusted fork context. Gate + // on the full (event, pull_request, review_requested) triple so that if a + // caller ever routes another trigger through PR_SOURCE=event, the env-supplied + // requester is never trusted — it falls through to '' (deny), not the timeline. + if (prSource === 'event' && eventName === 'pull_request' && eventAction === 'review_requested') { + return trustedRequester; + } + if (prSource === 'trigger') { + try { + return await resolveReviewRequester( + inputs.repoToken, + owner, + repo, + inputs.prNumber, + inputs.reviewerLogin, + ); + } catch (err: unknown) { + core.warning( + `Failed to resolve review requester for #${inputs.prNumber}: ${ + err instanceof Error ? err.message : String(err) + }`, + ); + return ''; + } + } + return ''; +} + +/** + * Decide whether the review is authorized, returning the subject and the path + * that granted it. See the module header for the two authorization paths. + */ +export async function evaluateMembership(inputs: MembershipInputs): Promise { + const { orgToken, org, prSource, eventName, commentAuthor } = inputs; + + // Comment-driven triggers (e.g. /review) authorize the commenter. EVENT_NAME may + // be absent when called by an older caller; fall back to the presence of a + // comment author to detect this path. + const isCommentTrigger = + prSource === 'event' && + (eventName === 'issue_comment' || (eventName === '' && commentAuthor !== '')); + if (isCommentTrigger) { + const ok = commentAuthor !== '' && (await checkOrgMembership(orgToken, org, commentAuthor)); + return { isMember: ok, subject: commentAuthor, via: ok ? 'comment' : 'none' }; + } + + const { owner, repo } = parseRepository(inputs.repository); + if (!Number.isInteger(inputs.prNumber) || inputs.prNumber <= 0) { + throw new Error(`Invalid pr-number: '${inputs.prNumber}' (expected positive integer)`); + } + + // Path 1 — auto-run: the PR author must be an org member. + const author = await resolvePrAuthor(inputs.repoToken, owner, repo, inputs.prNumber); + if (author && (await checkOrgMembership(orgToken, org, author))) { + return { isMember: true, subject: author, via: 'author' }; + } + + // Path 2 — review-requested: an org member who requested the review authorizes + // it, even when the PR author is external (not an org member). + const requester = await resolveTrustedRequester(inputs, owner, repo); + if (requester && (await checkOrgMembership(orgToken, org, requester))) { + return { isMember: true, subject: requester, via: 'requester' }; + } + + // Report whoever actually failed the membership check: the requester when path 2 + // was attempted (a requester was resolved), otherwise the PR author from path 1. + return { isMember: false, subject: requester || author, via: 'none' }; +} + // --------------------------------------------------------------------------- // CLI entry point // --------------------------------------------------------------------------- @@ -91,10 +298,6 @@ async function main(): Promise { const orgToken = process.env.ORG_MEMBERSHIP_TOKEN ?? ''; const repoToken = process.env.GITHUB_APP_TOKEN ?? ''; const org = process.env.ORG ?? ''; - const prSource = process.env.PR_SOURCE ?? ''; - const prNumberStr = process.env.PR_NUMBER ?? ''; - const commentAuthor = process.env.COMMENT_AUTHOR ?? ''; - const repository = process.env.GITHUB_REPOSITORY ?? ''; if (!orgToken) { core.setFailed('ORG_MEMBERSHIP_TOKEN is not set — ensure setup-credentials ran successfully.'); @@ -105,40 +308,33 @@ async function main(): Promise { return; } - let username: string; - - if (prSource === 'event') { - username = commentAuthor; - } else { - const prNumber = parseInt(prNumberStr, 10); - if (!Number.isInteger(prNumber) || prNumber <= 0) { - core.setFailed(`Invalid pr-number: '${prNumberStr}' (expected positive integer)`); - return; - } - const slashIdx = repository.indexOf('/'); - if (slashIdx < 0) { - core.setFailed(`Invalid GITHUB_REPOSITORY: '${repository}' (expected 'owner/repo')`); - return; - } - const owner = repository.slice(0, slashIdx); - const repo = repository.slice(slashIdx + 1); - try { - username = await resolvePrAuthor(repoToken, owner, repo, prNumber); - } catch (err: unknown) { - core.setFailed( - `Failed to resolve PR author for #${prNumber}: ${err instanceof Error ? err.message : String(err)}`, - ); - return; - } - } + const inputs: MembershipInputs = { + orgToken, + repoToken, + org, + reviewerLogin: process.env.AGENT_LOGIN ?? 'docker-agent', + repository: process.env.GITHUB_REPOSITORY ?? '', + prSource: process.env.PR_SOURCE ?? '', + eventName: process.env.EVENT_NAME ?? '', + eventAction: process.env.EVENT_ACTION ?? '', + prNumber: Number.parseInt(process.env.PR_NUMBER ?? '', 10), + commentAuthor: process.env.COMMENT_AUTHOR ?? '', + trustedRequester: process.env.REQUESTER ?? '', + }; try { - const isMember = await checkOrgMembership(orgToken, org, username); - core.setOutput('is_member', String(isMember)); - if (isMember) { - core.info(`✅ ${username} is a ${org} org member — proceeding with review`); + const decision = await evaluateMembership(inputs); + core.setOutput('is_member', String(decision.isMember)); + if (decision.isMember) { + const reason = + decision.via === 'requester' + ? `review requested by ${org} org member @${decision.subject}` + : `@${decision.subject} is a ${org} org member`; + core.info(`✅ ${reason} — proceeding with review`); } else { - core.info(`⏭️ ${username} is not a ${org} org member — skipping review`); + core.info( + `⏭️ Not authorized to review (subject: @${decision.subject || 'unknown'}) — skipping`, + ); } } catch (err: unknown) { const status = (err as { status?: number }).status;