diff --git a/.github/scripts/codex-security-review.js b/.github/scripts/codex-security-review.js index 3ac8f183e38..44f50399f8d 100644 --- a/.github/scripts/codex-security-review.js +++ b/.github/scripts/codex-security-review.js @@ -28,8 +28,8 @@ const completedMarker = (baseSha, headSha) => const reviewCommand = (headSha) => `${REVIEW_COMMAND} ${headSha}`; -const isOrganizationMember = (association) => - association === "MEMBER" || association === "OWNER"; +const hasWritePermission = (permission) => + permission === "write" || permission === "admin"; const hasCurrentReviewLabel = (pullRequest) => pullRequest.labels?.some( @@ -234,6 +234,28 @@ async function getPullRequest({ github, context, prNumber }) { return pullRequest; } +async function pullRequestAuthorCanWrite({ github, context, pullRequest }) { + const username = pullRequest.user?.login; + if (typeof username !== "string" || username.length === 0) { + return false; + } + + try { + const { data: access } = + await github.rest.repos.getCollaboratorPermissionLevel({ + owner: context.repo.owner, + repo: context.repo.repo, + username, + }); + return hasWritePermission(access.permission); + } catch (error) { + if (error?.status === 404) { + return false; + } + throw error; + } +} + async function getLiveMainSha({ github, context }) { const { data: mainRef } = await github.rest.git.getRef({ owner: context.repo.owner, @@ -364,10 +386,12 @@ async function prepare({ github, context, core }) { } if ( context.eventName === "pull_request_target" && - !isOrganizationMember(pullRequest.author_association) + !(await pullRequestAuthorCanWrite({ github, context, pullRequest })) ) { + const author = pullRequest.user?.login || "unknown author"; core.info( - `Pull request #${prNumber} requires authorization from a Block organization member.`, + `Pull request #${prNumber} author ${author} does not have write access ` + + `to ${context.repo.owner}/${context.repo.repo}; manual authorization is required.`, ); return; } @@ -479,7 +503,7 @@ async function invalidatePullRequestUpdate({ github, context, core }) { github, context, core, - existingOnlyForOrganizationMembers: true, + existingOnlyForTrustedAuthors: true, }); } @@ -489,7 +513,7 @@ async function invalidate({ core, prNumber: requestedPrNumber, existingOnly = false, - existingOnlyForOrganizationMembers = false, + existingOnlyForTrustedAuthors = false, }) { const prNumber = Number( requestedPrNumber ?? context.payload.pull_request?.number, @@ -506,10 +530,12 @@ async function invalidate({ } const existing = await findReviewComment({ github, context, prNumber }); + const trustedUnreviewedAuthor = + !existing && + existingOnlyForTrustedAuthors && + (await pullRequestAuthorCanWrite({ github, context, pullRequest })); const shouldOnlyUpdateExisting = - existingOnly || - (existingOnlyForOrganizationMembers && - isOrganizationMember(pullRequest.author_association)); + existingOnly || trustedUnreviewedAuthor; if (!existing && shouldOnlyUpdateExisting) { if (existingOnly || hasCurrentReviewLabel(pullRequest)) { await clearCurrentReview({ github, context, prNumber }); diff --git a/.github/scripts/codex-security-review.test.js b/.github/scripts/codex-security-review.test.js index 8358cb3642b..5ea60d57395 100644 --- a/.github/scripts/codex-security-review.test.js +++ b/.github/scripts/codex-security-review.test.js @@ -24,12 +24,14 @@ const CURRENT_REVIEW_LABEL = "codex-security-review-current"; function pullRequest({ authorAssociation = "CONTRIBUTOR", + authorLogin = "pr-author", baseSha = OLD_BASE_SHA, headSha = HEAD_SHA, labels = [], } = {}) { return { author_association: authorAssociation, + user: { login: authorLogin }, state: "open", base: { ref: "main", @@ -46,6 +48,7 @@ function pullRequest({ } function harness({ + authorPermission = "read", pull = pullRequest(), comments = [], files = [], @@ -63,6 +66,7 @@ function harness({ const notices = []; const info = []; const warnings = []; + const permissionChecks = []; const listComments = async () => storedComments; const listFiles = async () => files; let labelExists = false; @@ -122,6 +126,12 @@ function harness({ get: async () => ({ data: pull }), listFiles, }, + repos: { + getCollaboratorPermissionLevel: async (input) => { + permissionChecks.push(input); + return { data: { permission: authorPermission } }; + }, + }, }, }; const core = { @@ -150,6 +160,7 @@ function harness({ info, notices, outputs, + permissionChecks, removeLabelCalls, removedLabels, storedComments, @@ -231,24 +242,31 @@ 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, []); +test("pull request authorization uses live repository write access", async () => { + for (const authorPermission of ["write", "admin"]) { + const trusted = harness({ + authorPermission, + pull: pullRequest({ authorAssociation: "CONTRIBUTOR" }), + }); + trusted.context.eventName = "pull_request_target"; + trusted.context.payload.pull_request = { + number: 6816, + head: { sha: HEAD_SHA }, + author_association: "CONTRIBUTOR", + }; + + await prepare(trusted); + + assert.equal(trusted.outputs.get("authorized"), "true"); + assert.deepEqual(trusted.failures, []); + assert.deepEqual(trusted.permissionChecks, [ + { owner: "block", repo: "buzz", username: "pr-author" }, + ]); + } const external = harness({ - pull: pullRequest({ authorAssociation: "CONTRIBUTOR" }), + authorPermission: "read", + pull: pullRequest({ authorAssociation: "MEMBER" }), }); external.context.eventName = "pull_request_target"; external.context.payload.pull_request = { @@ -261,7 +279,7 @@ test("pull request authorization uses the live author association", async () => assert.equal(external.outputs.get("authorized"), undefined); assert.deepEqual(external.failures, []); - assert.match(external.info.at(-1), /requires authorization/); + assert.match(external.info.at(-1), /does not have write access/); }); test("PR mutation jobs use pull request write permission", () => { @@ -498,8 +516,9 @@ test("base reconciliation does not create comments on unreviewed PRs", async () assert.equal(state.updated.length, 0); }); -test("pull request updates invalidate member reviews without adding placeholders", async () => { +test("pull request updates invalidate trusted reviews without adding placeholders", async () => { const reviewed = harness({ + authorPermission: "write", pull: pullRequest({ authorAssociation: "MEMBER", headSha: OTHER_HEAD_SHA, @@ -529,6 +548,7 @@ test("pull request updates invalidate member reviews without adding placeholders assert.equal(reviewed.removedLabels.length, 1); const unreviewed = harness({ + authorPermission: "write", pull: pullRequest({ authorAssociation: "OWNER" }), }); unreviewed.context.eventName = "pull_request_target"; @@ -544,6 +564,7 @@ test("pull request updates invalidate member reviews without adding placeholders assert.equal(unreviewed.removeLabelCalls.length, 0); const external = harness({ + authorPermission: "read", pull: pullRequest({ authorAssociation: "CONTRIBUTOR" }), }); external.context.eventName = "pull_request_target"; diff --git a/.github/workflows/codex-security-review.yml b/.github/workflows/codex-security-review.yml index 0496809f5f9..d2fd8cda703 100644 --- a/.github/workflows/codex-security-review.yml +++ b/.github/workflows/codex-security-review.yml @@ -16,10 +16,11 @@ jobs: name: Authorize Security Review # 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. 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 + # Authors with live write or admin access to `block/buzz` are reviewed + # automatically. This calculated repository permission includes direct, + # team, organization, and enterprise grants. Outside contributors require + # this exact command from a Block organization member: + # @buzz-security-review if: >- github.repository == 'block/buzz' && ( ( @@ -301,7 +302,7 @@ jobs: codex-version: '0.149.0' model: ${{ env.CODEX_MODEL }} codex-args: '["-c","model_reasoning_effort=${{ env.CODEX_REASONING_EFFORT }}"]' - # The trusted authorization job already enforced repository membership. + # The trusted authorization job already enforced repository write access. allow-users: '*' safety-strategy: drop-sudo permission-profile: ':read-only'