Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 35 additions & 9 deletions .github/scripts/codex-security-review.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -479,7 +503,7 @@ async function invalidatePullRequestUpdate({ github, context, core }) {
github,
context,
core,
existingOnlyForOrganizationMembers: true,
existingOnlyForTrustedAuthors: true,
});
}

Expand All @@ -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,
Expand All @@ -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 });
Expand Down
57 changes: 39 additions & 18 deletions .github/scripts/codex-security-review.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -46,6 +48,7 @@ function pullRequest({
}

function harness({
authorPermission = "read",
pull = pullRequest(),
comments = [],
files = [],
Expand All @@ -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;
Expand Down Expand Up @@ -122,6 +126,12 @@ function harness({
get: async () => ({ data: pull }),
listFiles,
},
repos: {
getCollaboratorPermissionLevel: async (input) => {
permissionChecks.push(input);
return { data: { permission: authorPermission } };
},
},
},
};
const core = {
Expand Down Expand Up @@ -150,6 +160,7 @@ function harness({
info,
notices,
outputs,
permissionChecks,
removeLabelCalls,
removedLabels,
storedComments,
Expand Down Expand Up @@ -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 = {
Expand All @@ -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", () => {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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";
Expand All @@ -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";
Expand Down
11 changes: 6 additions & 5 deletions .github/workflows/codex-security-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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 <full-head-sha>
# 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 <full-head-sha>
if: >-
github.repository == 'block/buzz' && (
(
Expand Down Expand Up @@ -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'
Expand Down
Loading