diff --git a/.github/scripts/codex-security-review.js b/.github/scripts/codex-security-review.js index 3ac8f183e3..c36e1ae6d2 100644 --- a/.github/scripts/codex-security-review.js +++ b/.github/scripts/codex-security-review.js @@ -28,8 +28,30 @@ const completedMarker = (baseSha, headSha) => const reviewCommand = (headSha) => `${REVIEW_COMMAND} ${headSha}`; -const isOrganizationMember = (association) => - association === "MEMBER" || association === "OWNER"; +// Org membership is invisible to the workflow token when a member keeps it +// private, so trust is repo write access. `maintain` reports as `write`. +async function hasWriteAccess({ github, context, core, username }) { + if (!username) { + return false; + } + try { + const { data } = await withGithubRetry( + () => + github.rest.repos.getCollaboratorPermissionLevel({ + owner: context.repo.owner, + repo: context.repo.repo, + username, + }), + { core }, + ); + return data.permission === "admin" || data.permission === "write"; + } catch (error) { + if (error?.status === 404) { + return false; + } + throw error; + } +} const hasCurrentReviewLabel = (pullRequest) => pullRequest.labels?.some( @@ -330,6 +352,15 @@ async function prepare({ github, context, core }) { requestedHeadSha = context.payload.pull_request?.head?.sha || ""; } else if (context.eventName === "issue_comment") { prNumber = Number(context.payload.issue?.number); + const commenter = context.payload.comment?.user?.login; + if ( + !(await hasWriteAccess({ github, context, core, username: commenter })) + ) { + core.info( + `Review commands require write access to ${context.repo.owner}/${context.repo.repo}.`, + ); + return; + } const command = context.payload.comment?.body || ""; const match = /^@buzz-security-review ([0-9a-f]{40})$/.exec(command); if (!match) { @@ -364,10 +395,15 @@ async function prepare({ github, context, core }) { } if ( context.eventName === "pull_request_target" && - !isOrganizationMember(pullRequest.author_association) + !(await hasWriteAccess({ + github, + context, + core, + username: pullRequest.user?.login, + })) ) { core.info( - `Pull request #${prNumber} requires authorization from a Block organization member.`, + `Pull request #${prNumber} requires authorization from a user with write access.`, ); return; } @@ -479,17 +515,35 @@ async function invalidatePullRequestUpdate({ github, context, core }) { github, context, core, - existingOnlyForOrganizationMembers: true, + existingOnlyForTrustedAuthors: true, }); } +// A failed lookup must not skip stale-marking, so errors count as untrusted +// and the conservative stale notice is posted. +async function isTrustedForInvalidation({ github, context, core, pullRequest }) { + try { + return await hasWriteAccess({ + github, + context, + core, + username: pullRequest.user?.login, + }); + } catch (error) { + core.warning( + `Could not read PR author permission (status ${githubErrorStatus(error)}); treating the author as untrusted.`, + ); + return false; + } +} + async function invalidate({ github, context, core, prNumber: requestedPrNumber, existingOnly = false, - existingOnlyForOrganizationMembers = false, + existingOnlyForTrustedAuthors = false, }) { const prNumber = Number( requestedPrNumber ?? context.payload.pull_request?.number, @@ -506,11 +560,12 @@ async function invalidate({ } const existing = await findReviewComment({ github, context, prNumber }); - const shouldOnlyUpdateExisting = - existingOnly || - (existingOnlyForOrganizationMembers && - isOrganizationMember(pullRequest.author_association)); - if (!existing && shouldOnlyUpdateExisting) { + if ( + !existing && + (existingOnly || + (existingOnlyForTrustedAuthors && + (await isTrustedForInvalidation({ github, context, core, pullRequest })))) + ) { if (existingOnly || hasCurrentReviewLabel(pullRequest)) { await clearCurrentReview({ github, context, prNumber }); } @@ -535,7 +590,7 @@ ${STALE_MARKER} > > The current range is \`${liveMainSha}...${pullRequest.head.sha}\`. > A new review must complete for this exact range. When manual authorization -> is required, a Block organization member must comment exactly +> is required, a user with write access must comment exactly > \`${reviewCommand(pullRequest.head.sha)}\` to authorize a new review. > Any previous review applies only to its recorded range. `; diff --git a/.github/scripts/codex-security-review.test.js b/.github/scripts/codex-security-review.test.js index 8358cb3642..7ae3f852c3 100644 --- a/.github/scripts/codex-security-review.test.js +++ b/.github/scripts/codex-security-review.test.js @@ -23,13 +23,14 @@ const MARKER = ""; const CURRENT_REVIEW_LABEL = "codex-security-review-current"; function pullRequest({ - authorAssociation = "CONTRIBUTOR", + author = "outside-contributor", baseSha = OLD_BASE_SHA, headSha = HEAD_SHA, labels = [], } = {}) { return { - author_association: authorAssociation, + author_association: "CONTRIBUTOR", + user: { login: author }, state: "open", base: { ref: "main", @@ -51,6 +52,11 @@ function harness({ files = [], labeledIssues = [], liveMainShas = [BASE_SHA], + permissions = { + "block-member": "write", + "outside-contributor": "read", + "renovate[bot]": "none", + }, } = {}) { const storedComments = [...comments]; const created = []; @@ -118,6 +124,25 @@ function harness({ } }, }, + repos: { + getCollaboratorPermissionLevel: async ({ username }) => { + const permission = permissions[username]; + if (permission === undefined) { + throw Object.assign(new Error("not found"), { status: 404 }); + } + if (permission instanceof Error) { + throw permission; + } + if (Array.isArray(permission)) { + const next = permission.shift(); + if (next instanceof Error) { + throw next; + } + return { data: { permission: next } }; + } + return { data: { permission } }; + }, + }, pulls: { get: async () => ({ data: pull }), listFiles, @@ -135,7 +160,10 @@ function harness({ actor: "block-member", eventName: "issue_comment", payload: { - comment: { body: `@buzz-security-review ${HEAD_SHA}` }, + comment: { + body: `@buzz-security-review ${HEAD_SHA}`, + user: { login: "block-member" }, + }, issue: { number: 6816 }, }, repo: { owner: "block", repo: "buzz" }, @@ -231,37 +259,134 @@ 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 = { +function pullRequestTrigger(options) { + const state = harness(options); + state.context.eventName = "pull_request_target"; + state.context.payload.pull_request = { number: 6816, head: { sha: HEAD_SHA }, author_association: "MEMBER", }; + return state; +} - await prepare(external); +test("pull request authorization uses the live author's write access", async () => { + // Private org members report CONTRIBUTOR; write access still authorizes. + const member = pullRequestTrigger({ pull: pullRequest({ author: "block-member" }) }); + await prepare(member); + assert.equal(member.outputs.get("authorized"), "true"); + assert.deepEqual(member.failures, []); - assert.equal(external.outputs.get("authorized"), undefined); - assert.deepEqual(external.failures, []); - assert.match(external.info.at(-1), /requires authorization/); + for (const author of [ + "outside-contributor", + "nonexistent-user", + "renovate[bot]", + ]) { + const untrusted = pullRequestTrigger({ pull: pullRequest({ author }) }); + await prepare(untrusted); + assert.equal(untrusted.outputs.get("authorized"), undefined); + assert.deepEqual(untrusted.failures, []); + assert.match(untrusted.info.at(-1), /requires authorization/); + } +}); + +test("permission lookup errors fail closed", async () => { + const failure = Object.assign(new Error("forbidden"), { status: 403 }); + const author = pullRequestTrigger({ + pull: pullRequest({ author: "block-member" }), + permissions: { "block-member": failure }, + }); + await assert.rejects(prepare(author), /forbidden/); + assert.equal(author.outputs.size, 0); + + const commenter = harness({ permissions: { "block-member": failure } }); + await assert.rejects(prepare(commenter), /forbidden/); + assert.equal(commenter.outputs.size, 0); +}); + +test("transient permission lookup errors are retried", async () => { + const unavailable = Object.assign(new Error("unavailable"), { + status: 503, + response: { headers: { "retry-after": "0" } }, + }); + const state = harness({ + permissions: { "block-member": [unavailable, "write"] }, + }); + await prepare(state); + assert.equal(state.outputs.get("authorized"), "true"); + assert.equal(state.warnings.length, 1); +}); + +test("stale-marking proceeds when the permission lookup fails", async () => { + const state = harness({ + pull: pullRequest({ author: "block-member", headSha: OTHER_HEAD_SHA }), + permissions: { + "block-member": Object.assign(new Error("forbidden"), { status: 403 }), + }, + comments: [ + reviewComment( + `${MARKER}\n\nold review`, + ), + ], + }); + state.context.eventName = "pull_request_target"; + state.context.payload.pull_request = { number: 6816 }; + await state.github.rest.issues.addLabels({ + issue_number: 6816, + labels: [CURRENT_REVIEW_LABEL], + }); + + await invalidatePullRequestUpdate(state); + + assert.equal(state.updated.length, 1); + assert.match(state.updated[0].body, /review required for the current range/); + assert.equal(state.removedLabels.length, 1); + + const unreviewed = harness({ + pull: pullRequest({ author: "block-member" }), + permissions: { + "block-member": Object.assign(new Error("forbidden"), { status: 403 }), + }, + }); + unreviewed.context.eventName = "pull_request_target"; + unreviewed.context.payload.pull_request = { number: 6816 }; + + await invalidatePullRequestUpdate(unreviewed); + + assert.equal(unreviewed.created.length, 1); + assert.equal(unreviewed.warnings.length, 1); +}); + +test("review commands require a commenter with write access", async () => { + for (const login of ["outside-contributor", "nonexistent-user"]) { + const state = harness(); + state.context.payload.comment.user.login = login; + await prepare(state); + assert.equal(state.outputs.size, 0); + assert.deepEqual(state.failures, []); + assert.match(state.info.at(-1), /require write access/); + } + + const malformed = harness(); + malformed.context.payload.comment = { + body: "@buzz-security-review foo", + user: { login: "outside-contributor" }, + }; + await prepare(malformed); + assert.deepEqual(malformed.failures, []); + assert.equal(malformed.outputs.size, 0); + + const trusted = harness({ permissions: { "block-member": "admin" } }); + await prepare(trusted); + assert.equal(trusted.outputs.get("authorized"), "true"); +}); + +test("workflow leaves comment authorization to the trusted prepare step", () => { + const workflow = readFileSync( + path.join(__dirname, "../workflows/codex-security-review.yml"), + "utf8", + ); + assert.doesNotMatch(workflow, /comment\.author_association/); }); test("PR mutation jobs use pull request write permission", () => { @@ -501,7 +626,7 @@ 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({ - authorAssociation: "MEMBER", + author: "block-member", headSha: OTHER_HEAD_SHA, }), comments: [ @@ -529,7 +654,7 @@ test("pull request updates invalidate member reviews without adding placeholders assert.equal(reviewed.removedLabels.length, 1); const unreviewed = harness({ - pull: pullRequest({ authorAssociation: "OWNER" }), + pull: pullRequest({ author: "block-member" }), }); unreviewed.context.eventName = "pull_request_target"; unreviewed.context.payload.pull_request = { @@ -544,7 +669,7 @@ test("pull request updates invalidate member reviews without adding placeholders assert.equal(unreviewed.removeLabelCalls.length, 0); const external = harness({ - pull: pullRequest({ authorAssociation: "CONTRIBUTOR" }), + pull: pullRequest({ author: "outside-contributor" }), }); external.context.eventName = "pull_request_target"; external.context.payload.pull_request = { diff --git a/.github/workflows/codex-security-review.yml b/.github/workflows/codex-security-review.yml index 766dc430b9..eb6d01bf6d 100644 --- a/.github/workflows/codex-security-review.yml +++ b/.github/workflows/codex-security-review.yml @@ -16,10 +16,10 @@ 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 + # The trusted prepare step checks the live PR author's repo permission + # (write, maintain, or admin), not author_association, which hides private + # org membership from the workflow token. Other PRs require this exact + # command from a user with write access: @buzz-security-review if: >- github.repository == 'block/buzz' && ( ( @@ -28,8 +28,7 @@ jobs: ) || ( github.event_name == 'issue_comment' && github.event.issue.pull_request && - startsWith(github.event.comment.body, '@buzz-security-review ') && - contains(fromJSON('["MEMBER", "OWNER"]'), github.event.comment.author_association) + startsWith(github.event.comment.body, '@buzz-security-review ') ) ) runs-on: ubuntu-latest @@ -307,7 +306,8 @@ jobs: codex-version: '0.150.1' 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 required repo write access for the + # PR author or the authorizing commenter. allow-users: '*' safety-strategy: drop-sudo permission-profile: ':read-only'