diff --git a/.github/workflows/fork-review.yml b/.github/workflows/fork-review.yml index 292d0a9..263de21 100644 --- a/.github/workflows/fork-review.yml +++ b/.github/workflows/fork-review.yml @@ -71,7 +71,6 @@ jobs: github-token: ${{ github.token }} retries: 3 script: | - const allowedAssociations = new Set(["OWNER", "MEMBER"]); const issue = context.payload.issue; const comment = context.payload.comment; const prNumber = Number(process.env.PR_NUMBER); @@ -84,8 +83,25 @@ jobs: core.setFailed("The issue_comment event did not contain the exact /codex-review command"); return; } - if (!allowedAssociations.has(comment?.author_association)) { - core.setFailed("/codex-review can only be run by a repository owner or organization member"); + const commandAuthor = context.payload.comment?.user?.login; + if (!commandAuthor) { + core.setFailed("Cannot verify the /codex-review comment author"); + return; + } + let permission; + try { + const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ + owner: context.repo.owner, + repo: context.repo.repo, + username: commandAuthor, + }); + permission = data.permission; + } catch (error) { + core.setFailed("Could not verify repository access for /codex-review; refusing to run"); + return; + } + if (!["write", "maintain", "admin"].includes(permission)) { + core.setFailed("/codex-review requires write, maintain, or admin access to this repository"); return; } if (!Number.isFinite(prNumber) || prNumber !== Number(issue.number)) { diff --git a/README.md b/README.md index b06cfea..40190a4 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ Composite GitHub Action for running [`openai/codex-action`](https://github.com/o - Azure OpenAI support - PR summary comment upsert - inline review comments when findings can be anchored to the diff -- `/codex-review` PR comment trigger for repository owners and organization members +- `/codex-review` PR comment trigger for users with repository write access - maintainer-triggered, patch-only reviews for pull requests from forks - reusable central review logic for multiple repositories @@ -91,7 +91,7 @@ jobs: GitHub event triggers belong to the consumer repository, so a composite action cannot register a slash command by itself. The pin-sync workflow described below automatically adds `.github/workflows/codex-review-command.yml` to the three managed consumer repositories. For another consumer repository, copy [`examples/codex-review-command.yml`](examples/codex-review-command.yml) there manually. -Once that workflow is present on the consumer repository's default branch, a repository owner or organization member can add this exact PR comment: +Once that workflow is present on the consumer repository's default branch, a user with write, maintain, or admin repository access (including an outside collaborator) can add this exact PR comment: ```text /codex-review @@ -101,7 +101,8 @@ The workflow intentionally: - listens only for newly created PR comments - accepts only the exact `/codex-review` comment -- accepts only GitHub `OWNER` and `MEMBER` author associations +- verifies the comment author has write, maintain, or admin repository access through the GitHub API; author association alone does not grant access +- acknowledges authorized commands with 👀; reaction failures warn without blocking the review - refuses draft PRs - routes same-repository PRs through the existing full-checkout review - routes fork PRs through the patch-only reusable workflow @@ -119,7 +120,7 @@ The composite action remains restricted to trusted same-repository pull requests The command workflow resolves the PR before selecting one of two paths: - Same-repository PRs use the composite action and retain dependency installation plus full-checkout inspection. -- Fork PRs use `.github/workflows/fork-review.yml` only after an `OWNER` or `MEMBER` posts the exact `/codex-review` command. +- Fork PRs use `.github/workflows/fork-review.yml` only after a user with verified repository write access posts the exact `/codex-review` command. The fork workflow deliberately does not execute fork-controlled code: diff --git a/action.yml b/action.yml index 8c78494..daa5e0b 100644 --- a/action.yml +++ b/action.yml @@ -84,7 +84,6 @@ runs: if (context.eventName === "issue_comment") { const issue = context.payload.issue; const comment = context.payload.comment; - const allowedAssociations = new Set(["OWNER", "MEMBER"]); if (!issue?.pull_request) { core.setFailed("/codex-review can only be used on a pull request"); @@ -96,8 +95,25 @@ runs: return; } - if (!allowedAssociations.has(comment?.author_association)) { - core.setFailed("/codex-review can only be run by a repository owner or organization member"); + const commandAuthor = context.payload.comment?.user?.login; + if (!commandAuthor) { + core.setFailed("Cannot verify the /codex-review comment author"); + return; + } + let permission; + try { + const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ + owner: context.repo.owner, + repo: context.repo.repo, + username: commandAuthor, + }); + permission = data.permission; + } catch (error) { + core.setFailed("Could not verify repository access for /codex-review; refusing to run"); + return; + } + if (!["write", "maintain", "admin"].includes(permission)) { + core.setFailed("/codex-review requires write, maintain, or admin access to this repository"); return; } } diff --git a/examples/codex-review-command-arc-codemods.yml b/examples/codex-review-command-arc-codemods.yml index 9f5b66c..a8c9bd9 100644 --- a/examples/codex-review-command-arc-codemods.yml +++ b/examples/codex-review-command-arc-codemods.yml @@ -18,16 +18,43 @@ jobs: name: Resolve pull request if: >- github.event.issue.pull_request && - github.event.comment.body == '/codex-review' && - contains(fromJSON('["OWNER","MEMBER"]'), github.event.comment.author_association) + github.event.comment.body == '/codex-review' runs-on: ubuntu-latest permissions: - pull-requests: read # Route trusted and fork reviews using PR metadata. + pull-requests: write # Read PR metadata and react to PR conversation comments. issues: write # Acknowledge the command comment with a reaction. outputs: is_draft: ${{ steps.resolve.outputs.is_draft }} is_fork: ${{ steps.resolve.outputs.is_fork }} steps: + - name: Authorize command + uses: actions/github-script@373c709c69115d41ff229c7e5df9f8788daa9553 # v9 + with: + github-token: ${{ github.token }} + retries: 3 + script: | + const commandAuthor = context.payload.comment?.user?.login; + if (!commandAuthor) { + core.setFailed("Cannot verify the /codex-review comment author"); + return; + } + let permission; + try { + const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ + owner: context.repo.owner, + repo: context.repo.repo, + username: commandAuthor, + }); + permission = data.permission; + } catch (error) { + core.setFailed("Could not verify repository access for /codex-review; refusing to run"); + return; + } + if (!["write", "maintain", "admin"].includes(permission)) { + core.setFailed("/codex-review requires write, maintain, or admin access to this repository"); + return; + } + - name: Acknowledge command uses: actions/github-script@373c709c69115d41ff229c7e5df9f8788daa9553 # v9 with: diff --git a/examples/codex-review-command.yml b/examples/codex-review-command.yml index 91bb03b..08750d7 100644 --- a/examples/codex-review-command.yml +++ b/examples/codex-review-command.yml @@ -18,16 +18,43 @@ jobs: name: Resolve pull request if: >- github.event.issue.pull_request && - github.event.comment.body == '/codex-review' && - contains(fromJSON('["OWNER","MEMBER"]'), github.event.comment.author_association) + github.event.comment.body == '/codex-review' runs-on: ubuntu-latest permissions: - pull-requests: read # Route trusted and fork reviews using PR metadata. + pull-requests: write # Read PR metadata and react to PR conversation comments. issues: write # Acknowledge the command comment with a reaction. outputs: is_draft: ${{ steps.resolve.outputs.is_draft }} is_fork: ${{ steps.resolve.outputs.is_fork }} steps: + - name: Authorize command + uses: actions/github-script@373c709c69115d41ff229c7e5df9f8788daa9553 # v9 + with: + github-token: ${{ github.token }} + retries: 3 + script: | + const commandAuthor = context.payload.comment?.user?.login; + if (!commandAuthor) { + core.setFailed("Cannot verify the /codex-review comment author"); + return; + } + let permission; + try { + const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ + owner: context.repo.owner, + repo: context.repo.repo, + username: commandAuthor, + }); + permission = data.permission; + } catch (error) { + core.setFailed("Could not verify repository access for /codex-review; refusing to run"); + return; + } + if (!["write", "maintain", "admin"].includes(permission)) { + core.setFailed("/codex-review requires write, maintain, or admin access to this repository"); + return; + } + - name: Acknowledge command uses: actions/github-script@373c709c69115d41ff229c7e5df9f8788daa9553 # v9 with: diff --git a/tests/command-authorization.test.cjs b/tests/command-authorization.test.cjs new file mode 100644 index 0000000..e46e32a --- /dev/null +++ b/tests/command-authorization.test.cjs @@ -0,0 +1,97 @@ +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const { test } = require('node:test'); +const AsyncFunction = Object.getPrototypeOf(async () => {}).constructor; + +function script(file, stepName, indent) { + const source = fs.readFileSync(path.join(__dirname, '..', file), 'utf8'); + const step = source.split(`- name: ${stepName}\n`)[1]; + assert(step, `Missing ${stepName}`); + const lines = step.split('script: |\n')[1].split('\n'); + const body = []; + for (const line of lines) { + if (line.trim() && !line.startsWith(' '.repeat(indent))) break; + body.push(line.slice(indent)); + } + return new AsyncFunction('context', 'github', 'core', 'process', body.join('\n')); +} +const templates = ['examples/codex-review-command.yml', 'examples/codex-review-command-arc-codemods.yml']; +const targets = [ + ...templates.map(file => ({ file, step: 'Authorize command', indent: 12 })), + { file: 'action.yml', step: 'Resolve pull request context', indent: 10 }, + { file: '.github/workflows/fork-review.yml', step: 'Authorize fork review command', indent: 12 }, +]; +for (const target of targets) { + const run = script(target.file, target.step, target.indent); + async function check({ permission = 'write', association = 'COLLABORATOR', lookupError, missingAuthor = false } = {}) { + const errors = [], lookups = [], outputs = {}; + let pullReads = 0; + const context = { + eventName: 'issue_comment', actor: 'admin-rerunning-workflow', + repo: { owner: 'codemod', repo: 'example' }, + payload: { issue: { number: 12, pull_request: {} }, comment: { + id: 123, body: '/codex-review', author_association: association, + user: missingAuthor ? undefined : { login: 'original-comment-author' }, + } }, + }; + const github = { rest: { + repos: { getCollaboratorPermissionLevel: async args => { + lookups.push(args); + if (lookupError) throw new Error('permission lookup unavailable'); + return { data: { permission, role_name: 'custom-role' } }; + } }, + pulls: { get: async () => { + pullReads++; + return { data: { + number: 12, draft: false, + base: { sha: 'a'.repeat(40), ref: 'main', repo: { full_name: 'codemod/example' } }, + head: { sha: 'b'.repeat(40), repo: { full_name: target.file.includes('fork-review') ? 'fork/example' : 'codemod/example' } }, + } }; + } }, + } }; + await run(context, github, { + setFailed: msg => errors.push(msg), setOutput: (key, value) => { outputs[key] = value; }, + }, { env: { PR_NUMBER: '12' } }); + if (!missingAuthor) assert.deepEqual(lookups, [{ owner: 'codemod', repo: 'example', username: 'original-comment-author' }]); + else assert.equal(lookups.length, 0); + return { errors, outputs, pullReads }; + } + for (const permission of ['write', 'maintain', 'admin']) { + test(`${target.file}: outside collaborator with ${permission} is authorized`, async () => { + assert.deepEqual((await check({ permission })).errors, []); + }); + } + for (const permission of ['read', 'triage', 'none', 'unexpected', null]) { + test(`${target.file}: even MEMBER with ${permission} is rejected before PR access`, async () => { + const result = await check({ permission, association: 'MEMBER' }); + assert.match(result.errors[0], /requires write/); + assert.equal(result.pullReads, 0); + assert.deepEqual(result.outputs, {}); + }); + } + test(`${target.file}: unknown association does not override verified write access`, async () => { + assert.deepEqual((await check({ association: 'NONE' })).errors, []); + }); + for (const options of [{ lookupError: true }, { missingAuthor: true }]) { + test(`${target.file}: authorization fails closed for ${JSON.stringify(options)}`, async () => { + const result = await check(options); + assert.equal(result.errors.length, 1); + assert.equal(result.pullReads, 0); + assert.deepEqual(result.outputs, {}); + }); + } +} +for (const file of templates) { + const acknowledge = script(file, 'Acknowledge command', 12); + test(`${file}: acknowledges original command; API rejection remains non-fatal`, async () => { + const warnings = [], calls = []; + const context = { repo: { owner: 'codemod', repo: 'example' }, payload: { comment: { id: 123 } } }; + const github = { rest: { reactions: { createForIssueComment: async args => { calls.push(args); } } } }; + await acknowledge(context, github, { warning: msg => warnings.push(msg) }); + assert.deepEqual(calls, [{ owner: 'codemod', repo: 'example', comment_id: 123, content: 'eyes' }]); + github.rest.reactions.createForIssueComment = async () => { throw new Error('403'); }; + await acknowledge(context, github, { warning: msg => warnings.push(msg) }); + assert.equal(warnings.length, 1); + }); +} diff --git a/tests/fork-review.test.cjs b/tests/fork-review.test.cjs index 14365aa..010f8a0 100644 --- a/tests/fork-review.test.cjs +++ b/tests/fork-review.test.cjs @@ -48,16 +48,6 @@ function assertComparison(args) { assert.equal(args.repo, 'codemod'); } -for (const association of ['OWNER', 'MEMBER', 'NONE', 'CONTRIBUTOR', 'COLLABORATOR']) { - test(`authorization: ${association}`, async () => { - const input = structuredClone(context); - input.payload.comment.author_association = association; - const result = state(); - await authorize(require, { env: { PR_NUMBER: '2403' } }, input, { rest: { pulls: { get: async () => ({ data: pr }) } } }, result.core); - assert.equal(result.errors.length > 0, !['OWNER', 'MEMBER'].includes(association)); - }); -} - test('a push/reset during collection cannot change the reviewed diff or file list', async t => { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'fork-review-test-')); t.after(() => fs.rmSync(dir, { recursive: true, force: true }));