diff --git a/.github/scripts/codex-security-review.js b/.github/scripts/codex-security-review.js index ac54adb6658..3ac8f183e38 100644 --- a/.github/scripts/codex-security-review.js +++ b/.github/scripts/codex-security-review.js @@ -28,6 +28,16 @@ const completedMarker = (baseSha, headSha) => const reviewCommand = (headSha) => `${REVIEW_COMMAND} ${headSha}`; +const isOrganizationMember = (association) => + association === "MEMBER" || association === "OWNER"; + +const hasCurrentReviewLabel = (pullRequest) => + pullRequest.labels?.some( + (label) => + (typeof label === "string" ? label : label?.name) === + CURRENT_REVIEW_LABEL, + ) ?? false; + const isObject = (value) => value !== null && typeof value === "object" && !Array.isArray(value); @@ -352,6 +362,15 @@ async function prepare({ github, context, core }) { ); return; } + if ( + context.eventName === "pull_request_target" && + !isOrganizationMember(pullRequest.author_association) + ) { + core.info( + `Pull request #${prNumber} requires authorization from a Block organization member.`, + ); + return; + } if (pullRequest.head.sha !== requestedHeadSha) { core.setFailed( `Pull request #${prNumber} moved after this review was authorized. ` + @@ -362,6 +381,7 @@ async function prepare({ github, context, core }) { const baseSha = await getLiveMainSha({ github, context }); const commitRange = `${baseSha}...${pullRequest.head.sha}`; + core.setOutput("authorized", "true"); core.setOutput("pr_number", String(prNumber)); core.setOutput("trigger_actor", context.actor); core.setOutput("base_sha", baseSha); @@ -455,9 +475,12 @@ async function prepareBaseReconciliation({ github, context, core }) { } async function invalidatePullRequestUpdate({ github, context, core }) { - const association = context.payload.pull_request?.author_association || ""; - const existingOnly = association === "MEMBER" || association === "OWNER"; - await invalidate({ github, context, core, existingOnly }); + await invalidate({ + github, + context, + core, + existingOnlyForOrganizationMembers: true, + }); } async function invalidate({ @@ -466,6 +489,7 @@ async function invalidate({ core, prNumber: requestedPrNumber, existingOnly = false, + existingOnlyForOrganizationMembers = false, }) { const prNumber = Number( requestedPrNumber ?? context.payload.pull_request?.number, @@ -482,8 +506,14 @@ async function invalidate({ } const existing = await findReviewComment({ github, context, prNumber }); - if (!existing && existingOnly) { - await clearCurrentReview({ github, context, prNumber }); + const shouldOnlyUpdateExisting = + existingOnly || + (existingOnlyForOrganizationMembers && + isOrganizationMember(pullRequest.author_association)); + if (!existing && shouldOnlyUpdateExisting) { + if (existingOnly || hasCurrentReviewLabel(pullRequest)) { + await clearCurrentReview({ github, context, prNumber }); + } core.info(`PR #${prNumber} has no Codex security review to invalidate.`); return; } diff --git a/.github/scripts/codex-security-review.test.js b/.github/scripts/codex-security-review.test.js index 0e6d8068ead..8358cb3642b 100644 --- a/.github/scripts/codex-security-review.test.js +++ b/.github/scripts/codex-security-review.test.js @@ -1,6 +1,8 @@ "use strict"; const assert = require("node:assert/strict"); +const { readFileSync } = require("node:fs"); +const path = require("node:path"); const test = require("node:test"); const { @@ -20,8 +22,14 @@ const NEW_BASE_SHA = "e".repeat(40); const MARKER = ""; const CURRENT_REVIEW_LABEL = "codex-security-review-current"; -function pullRequest({ baseSha = OLD_BASE_SHA, headSha = HEAD_SHA } = {}) { +function pullRequest({ + authorAssociation = "CONTRIBUTOR", + baseSha = OLD_BASE_SHA, + headSha = HEAD_SHA, + labels = [], +} = {}) { return { + author_association: authorAssociation, state: "open", base: { ref: "main", @@ -33,6 +41,7 @@ function pullRequest({ baseSha = OLD_BASE_SHA, headSha = HEAD_SHA } = {}) { repo: { full_name: "outside/buzz" }, }, changed_files: 1, + labels, }; } @@ -48,6 +57,7 @@ function harness({ const updated = []; const addedLabels = []; const removedLabels = []; + const removeLabelCalls = []; const outputs = new Map(); const failures = []; const notices = []; @@ -85,6 +95,7 @@ function harness({ addedLabels.push(input); }, removeLabel: async (input) => { + removeLabelCalls.push(input); if (!labelExists) { throw Object.assign(new Error("not found"), { status: 404 }); } @@ -139,6 +150,7 @@ function harness({ info, notices, outputs, + removeLabelCalls, removedLabels, storedComments, updated, @@ -198,6 +210,7 @@ test("prepare binds a member command to the named head SHA", async () => { await prepare(current); assert.deepEqual(current.failures, []); + assert.equal(current.outputs.get("authorized"), "true"); assert.equal(current.outputs.get("head_sha"), HEAD_SHA); assert.equal(current.outputs.get("base_sha"), BASE_SHA); assert.notEqual(current.outputs.get("base_sha"), OLD_BASE_SHA); @@ -218,6 +231,62 @@ test("prepare binds a member command to the named head SHA", async () => { ); }); +test("pull request authorization uses the live author association", async () => { + const member = harness({ + pull: pullRequest({ authorAssociation: "MEMBER" }), + }); + member.context.eventName = "pull_request_target"; + member.context.payload.pull_request = { + number: 6816, + head: { sha: HEAD_SHA }, + author_association: "CONTRIBUTOR", + }; + + await prepare(member); + + assert.equal(member.outputs.get("authorized"), "true"); + assert.deepEqual(member.failures, []); + + const external = harness({ + pull: pullRequest({ authorAssociation: "CONTRIBUTOR" }), + }); + external.context.eventName = "pull_request_target"; + external.context.payload.pull_request = { + number: 6816, + head: { sha: HEAD_SHA }, + author_association: "MEMBER", + }; + + await prepare(external); + + assert.equal(external.outputs.get("authorized"), undefined); + assert.deepEqual(external.failures, []); + assert.match(external.info.at(-1), /requires authorization/); +}); + +test("PR mutation jobs use pull request write permission", () => { + const workflow = readFileSync( + path.join(__dirname, "../workflows/codex-security-review.yml"), + "utf8", + ); + for (const jobName of [ + "reconcile-base-reviews", + "invalidate-previous-review", + "post-review", + ]) { + const start = workflow.indexOf(` ${jobName}:\n`); + assert.notEqual(start, -1, `missing workflow job ${jobName}`); + const remainder = workflow.slice(start + 2); + const nextJob = remainder.search(/^ [a-z][a-z0-9-]*:\n/m); + const job = + nextJob === -1 + ? workflow.slice(start) + : workflow.slice(start, start + 2 + nextJob); + assert.match(job, /^ pull-requests: write$/m); + assert.doesNotMatch(job, /^ issues: write$/m); + } +}); + test("prepare rejects review commands without a full exact SHA", async () => { const state = harness(); state.context.payload.comment.body = "@buzz-security-review"; @@ -431,7 +500,10 @@ test("base reconciliation does not create comments on unreviewed PRs", async () test("pull request updates invalidate member reviews without adding placeholders", async () => { const reviewed = harness({ - pull: pullRequest({ headSha: OTHER_HEAD_SHA }), + pull: pullRequest({ + authorAssociation: "MEMBER", + headSha: OTHER_HEAD_SHA, + }), comments: [ reviewComment( `${MARKER}\n\nold review`, @@ -441,7 +513,7 @@ test("pull request updates invalidate member reviews without adding placeholders reviewed.context.eventName = "pull_request_target"; reviewed.context.payload.pull_request = { number: 6816, - author_association: "MEMBER", + author_association: "CONTRIBUTOR", }; await reviewed.github.rest.issues.addLabels({ issue_number: 6816, @@ -456,23 +528,28 @@ test("pull request updates invalidate member reviews without adding placeholders ); assert.equal(reviewed.removedLabels.length, 1); - const unreviewed = harness(); + const unreviewed = harness({ + pull: pullRequest({ authorAssociation: "OWNER" }), + }); unreviewed.context.eventName = "pull_request_target"; unreviewed.context.payload.pull_request = { number: 6816, - author_association: "OWNER", + author_association: "CONTRIBUTOR", }; await invalidatePullRequestUpdate(unreviewed); assert.equal(unreviewed.created.length, 0); assert.equal(unreviewed.updated.length, 0); + assert.equal(unreviewed.removeLabelCalls.length, 0); - const external = harness(); + const external = harness({ + pull: pullRequest({ authorAssociation: "CONTRIBUTOR" }), + }); external.context.eventName = "pull_request_target"; external.context.payload.pull_request = { number: 6816, - author_association: "CONTRIBUTOR", + author_association: "MEMBER", }; await invalidatePullRequestUpdate(external); diff --git a/.github/workflows/codex-security-review.yml b/.github/workflows/codex-security-review.yml index ea6cd844bfc..0496809f5f9 100644 --- a/.github/workflows/codex-security-review.yml +++ b/.github/workflows/codex-security-review.yml @@ -17,14 +17,14 @@ jobs: # This workflow posts an advisory review; its skipped jobs are not a merge # gate and must not be configured as required status checks. # MEMBER and OWNER are GitHub's associations for members of the `block` - # organization. Outside contributors require this exact command from one + # organization. The trusted prepare step checks the live PR instead of the + # event snapshot. Outside contributors require this exact command from one # of those members: @buzz-security-review if: >- github.repository == 'block/buzz' && ( ( github.event_name == 'pull_request_target' && - github.event.pull_request.draft == false && - contains(fromJSON('["MEMBER", "OWNER"]'), github.event.pull_request.author_association) + github.event.pull_request.draft == false ) || ( github.event_name == 'issue_comment' && github.event.issue.pull_request && @@ -38,6 +38,7 @@ jobs: contents: read pull-requests: read outputs: + authorized: ${{ steps.pr.outputs.authorized }} pr_number: ${{ steps.pr.outputs.pr_number }} trigger_actor: ${{ steps.pr.outputs.trigger_actor }} base_sha: ${{ steps.pr.outputs.base_sha }} @@ -111,8 +112,7 @@ jobs: cancel-in-progress: true permissions: contents: read - issues: write - pull-requests: read + pull-requests: write steps: - name: Checkout trusted workflow support if: matrix.pr_number != 0 @@ -192,8 +192,7 @@ jobs: cancel-in-progress: true permissions: contents: read - issues: write - pull-requests: read + pull-requests: write steps: - name: Checkout trusted workflow support uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -212,6 +211,7 @@ jobs: security-review: name: Run Codex Security Review needs: prepare-review + if: needs.prepare-review.outputs.authorized == 'true' runs-on: ubuntu-latest environment: codex-review timeout-minutes: 30 @@ -453,8 +453,7 @@ jobs: cancel-in-progress: true permissions: contents: read - issues: write - pull-requests: read + pull-requests: write env: CODEX_MODEL: gpt-5.6-sol REVIEW_JSON: ${{ needs.security-review.outputs.review_json }}