fix private-member PR audit triggers

.github/workflows/test.yml

@@ -1,11 +1,10 @@
 name: Test
 
-# Integration tests for the scan-github-actions composite action. Distinct from
-# ci.yml (which already dogfoods the reusable read-only path on this repo): this
-# suite exercises the composite path (untested by ci.yml) with a clean fixture
-# and a negative fixture, plus reusable/composite pin parity. The SARIF upload
-# path is intentionally not exercised here so this repo's code scanning stays
-# clean.
+# Tests for JavaScript-backed actions and the scan-github-actions composite.
+# Distinct from ci.yml (which already dogfoods the reusable read-only path on
+# this repo), this suite exercises the composite path with a clean fixture and a
+# negative fixture, plus reusable/composite pin parity. The SARIF upload path is
+# intentionally not exercised here so this repo's code scanning stays clean.
 #
 # All checks run in a single job so the suite surfaces as one status check
 # (`Test / integration`), which can be marked required in branch protection.
@@ -30,6 +29,9 @@ jobs:
         with:
           persist-credentials: false
 
+      - name: Test PR audit comment permissions
+        run: node --test actions/pr-audit-comment/pr_audit_comment.test.js
+
       # Guard: the reusable workflow and composite action must pin identical tool
       # versions, since they duplicate the zizmor + actionlint steps.
       - name: Assert reusable/composite pins match

actions/pr-audit-comment/README.md

@@ -45,3 +45,8 @@ Supported arguments:
 - `run-label=LABEL`
 - `dry-run`
 - `note="..."`
+
+In `association` permission mode, a trusted commenter who is also the PR author
+is accepted by matching their GitHub user IDs. Different commenters still
+require the PR author's fetched association to be `OWNER`, `MEMBER`, or
+`COLLABORATOR`.

actions/pr-audit-comment/pr_audit_comment.js

@@ -87,7 +87,8 @@ function parseArgs(body, commandRegex) {
 
 async function checkPermission({ github, context, core }) {
   const mode = process.env.PERMISSION_CHECK_MODE;
-  const commenter = context.payload.comment.user.login;
+  const commenterUser = context.payload.comment.user;
+  const commenter = commenterUser.login;
 
   const { data: pr } = await github.rest.pulls.get({
     owner: context.repo.owner,
@@ -103,7 +104,11 @@ async function checkPermission({ github, context, core }) {
       return null;
     }
 
-    if (!allowed.has(pr.author_association)) {
+    // The issue_comment payload can identify a private org member even when the
+    // repo-scoped token reports that same user as CONTRIBUTOR on pulls.get.
+    // A trusted commenter who authored the PR has already satisfied both checks.
+    const commenterIsPrAuthor = commenterUser.id != null && commenterUser.id === pr.user?.id;
+    if (!commenterIsPrAuthor && !allowed.has(pr.author_association)) {
       core.setFailed(`PR author @${pr.user.login} is not allowed to trigger Cyclops audits (${pr.author_association})`);
       return null;
     }
@@ -208,7 +213,7 @@ function publishEvent(payload) {
   }
 }
 
-module.exports = async ({ github, context, core }) => {
+async function handle({ github, context, core }) {
   // Only handle comments posted on pull requests; no-op on other events
   // so a misconfigured caller job exits cleanly instead of crashing.
   if (!context.payload.comment || !context.payload.issue?.pull_request) return;
@@ -273,4 +278,7 @@ module.exports = async ({ github, context, core }) => {
     });
     core.setFailed(error.message);
   }
-};
+}
+
+handle.checkPermission = checkPermission;
+module.exports = handle;

actions/pr-audit-comment/pr_audit_comment.test.js

@@ -0,0 +1,109 @@
+const assert = require("node:assert/strict");
+const test = require("node:test");
+
+const { checkPermission } = require("./pr_audit_comment");
+
+function fixture({
+  commenterAssociation = "MEMBER",
+  commenterId = 1,
+  commenterLogin = "member",
+  prAuthorAssociation = "CONTRIBUTOR",
+  prAuthorId = 1,
+  prAuthorLogin = "member",
+} = {}) {
+  const failures = [];
+  const pr = {
+    author_association: prAuthorAssociation,
+    head: { sha: "abc123" },
+    user: { id: prAuthorId, login: prAuthorLogin },
+  };
+
+  return {
+    core: { setFailed: (message) => failures.push(message) },
+    failures,
+    github: {
+      rest: {
+        pulls: {
+          get: async () => ({ data: pr }),
+        },
+      },
+    },
+    context: {
+      issue: { number: 609 },
+      payload: {
+        comment: {
+          author_association: commenterAssociation,
+          user: { id: commenterId, login: commenterLogin },
+        },
+      },
+      repo: { owner: "tempoxyz", repo: "zones" },
+    },
+    pr,
+  };
+}
+
+test.beforeEach(() => {
+  process.env.PERMISSION_CHECK_MODE = "association";
+});
+
+test("allows a trusted PR author when the token reports CONTRIBUTOR", async () => {
+  const input = fixture();
+
+  const result = await checkPermission(input);
+
+  assert.equal(result, input.pr);
+  assert.deepEqual(input.failures, []);
+});
+
+test("rejects an untrusted commenter even when they authored the PR", async () => {
+  const input = fixture({ commenterAssociation: "CONTRIBUTOR" });
+
+  const result = await checkPermission(input);
+
+  assert.equal(result, null);
+  assert.deepEqual(input.failures, [
+    "@member is not allowed to trigger Cyclops audits (CONTRIBUTOR)",
+  ]);
+});
+
+test("still rejects an untrusted PR author when a different member comments", async () => {
+  const input = fixture({ commenterId: 2, commenterLogin: "maintainer" });
+
+  const result = await checkPermission(input);
+
+  assert.equal(result, null);
+  assert.deepEqual(input.failures, [
+    "PR author @member is not allowed to trigger Cyclops audits (CONTRIBUTOR)",
+  ]);
+});
+
+test("does not treat matching logins with different user IDs as the same author", async () => {
+  const input = fixture({ commenterId: 2 });
+
+  const result = await checkPermission(input);
+
+  assert.equal(result, null);
+  assert.equal(input.failures.length, 1);
+});
+
+test("does not bypass the fetched author association when user IDs are missing", async () => {
+  const input = fixture({ commenterId: null, prAuthorId: null });
+
+  const result = await checkPermission(input);
+
+  assert.equal(result, null);
+  assert.equal(input.failures.length, 1);
+});
+
+test("allows the fetched author association when user IDs are missing", async () => {
+  const input = fixture({
+    commenterId: null,
+    prAuthorAssociation: "MEMBER",
+    prAuthorId: null,
+  });
+
+  const result = await checkPermission(input);
+
+  assert.equal(result, input.pr);
+  assert.deepEqual(input.failures, []);
+});