fix(ci): gate security review on repo write access (#7986)

🤖 The Codex Security Review gate trusted GitHub's `author_association` (`MEMBER`/`OWNER`), but GitHub only reports `MEMBER` to viewers who can see the membership. The workflow `GITHUB_TOKEN` cannot see private `block` org membership, which is GitHub's default, so most Block employees read as `CONTRIBUTOR` and their PRs never reviewed automatically. The same applied to the comment path, where private members could not authorize a review with `@buzz-security-review <sha>`. Trust is now repo write access, read from `GET /repos/{owner}/{repo}/collaborators/{user}/permission` via `getCollaboratorPermissionLevel`, which is independent of membership visibility and works with the authorize job's existing `contents: read` and `pull-requests: read` permissions. A user is trusted only when `permission` is `write` (which includes `maintain`) or `admin`. A successful lookup is not enough: on this public repository, an account with no relationship to it returns `200` with `read`, not 404. Outside contributors can only push to forks and come back `read`, so they still need an explicit command from a trusted user. The check covers the PR author on `pull_request_target`, the commenter on `issue_comment` (the job-level `author_association` filter is removed so the trusted script makes that decision), and the stale-marking path on PR updates. Transient API errors are retried. Authorization fails closed: any lookup error other than 404 fails the job instead of authorizing. Stale-marking is conservative: an existing review is always marked stale without consulting permissions, and a failed lookup counts as untrusted, so a previous range's review can never appear current for a new head. A commenter without write access is logged and ignored instead of failing the run. --------- Signed-off-by: Will Pfleger <pfleger.will@gmail.com> Co-authored-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
2026-10-27 12:40:30 +00:00 · 2026-09-30 12:19:56 -04:00
parent 95b018c871
commit 0ee6093790
3 changed files with 230 additions and 50 deletions
--- 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.
 `;
--- a/.github/scripts/codex-security-review.test.js
+++ b/.github/scripts/codex-security-review.test.js
@@ -23,13 +23,14 @@ const MARKER = "<!-- codex-security-review -->";
 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<!-- codex-security-review-range:${BASE_SHA}...${HEAD_SHA} -->\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 = {