fix: authorize roll commands against electron/electron only

src/index.ts

@@ -4,17 +4,22 @@ import { handleNodeCheck } from './node-handler.js';
 import { handleChromiumCheck } from './chromium-handler.js';
 import { handleBuildImagesCheck } from './build-images-handler.js';
 import { handleBuildImagesChromiumDepsCheck } from './build-images-chromium-deps-handler.js';
-import { ROLL_TARGETS } from './constants.js';
-import { isAuthorizedUser } from './utils/is-authorized-user.js';
+import { REPOS, ROLL_TARGETS } from './constants.js';
+import { isAuthorizedElectronRepoUser } from './utils/is-authorized-user.js';
 
 const handler = (robot: Probot) => {
   robot.on('pull_request.closed', async (context) => {
     const d = debug('roller/github:pull_request.closed');
 
-    const { pull_request: pr } = context.payload;
+    const { pull_request: pr, repository } = context.payload;
 
     if (!pr.merged) return;
 
+    // Merging a `chore: bump ...` PR triggers privileged rolls against electron/electron,
+    // so only react to merges in that repository. Otherwise a merged PR with a crafted
+    // title in any other repo where this app is installed could trigger a roll.
+    if (repository.full_name !== `${REPOS.electron.owner}/${REPOS.electron.repo}`) return;
+
     const isNodePR = pr.title.startsWith(`chore: bump ${ROLL_TARGETS.node.name}`);
     const isChromiumPR = pr.title.startsWith(`chore: bump ${ROLL_TARGETS.chromium.name}`);
 
@@ -63,13 +68,22 @@ const handler = (robot: Probot) => {
       return;
     }
 
+    // Roll commands perform privileged actions against electron/electron, so only
+    // honor them when issued from that repository. This prevents permissions on
+    // unrelated repos where this app is installed from authorizing a roll.
+    const { repository } = context.payload;
+    if (repository.full_name !== `${REPOS.electron.owner}/${REPOS.electron.repo}`) {
+      d(`Ignoring roll command from ${repository.full_name} - only electron/electron is allowed`);
+      return;
+    }
+
     if (!issue.pull_request) {
       d(`Invalid usage - only roll PRs can be triggered with the roll command`);
       return;
     }
 
     // Allow all users with push access to run commands
-    if (!(await isAuthorizedUser(context, comment.user.login))) {
+    if (!(await isAuthorizedElectronRepoUser(context, comment.user.login))) {
       d(`@${comment.user.login} is not authorized to run roller commands - stopping`);
       await context.octokit.rest.issues.createComment(
         context.repo({

src/utils/is-authorized-user.ts

@@ -1,14 +1,20 @@
 import type { Context } from 'probot';
 
-export async function isAuthorizedUser(
+import { REPOS } from '../constants.js';
+
+export async function isAuthorizedElectronRepoUser(
   context: Context<'issue_comment.created'>,
   username: string,
 ) {
-  const { data } = await context.octokit.rest.repos.getCollaboratorPermissionLevel(
-    context.repo({
-      username,
-    }),
-  );
+  // Authorization for roll commands must be evaluated against electron/electron,
+  // the repository the privileged roll actions modify - not against the repository
+  // the webhook comment was posted in. Otherwise write access on any other repo
+  // where this app is installed would be enough to trigger privileged rolls.
+  const { data } = await context.octokit.rest.repos.getCollaboratorPermissionLevel({
+    owner: REPOS.electron.owner,
+    repo: REPOS.electron.repo,
+    username,
+  });
 
   return ['admin', 'write'].includes(data.permission);
 }

tests/index.spec.ts

@@ -5,7 +5,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 import handler from '../src/index.js';
 import { handleChromiumCheck } from '../src/chromium-handler.js';
 import { handleNodeCheck } from '../src/node-handler.js';
-import { isAuthorizedUser } from '../src/utils/is-authorized-user.js';
+import { isAuthorizedElectronRepoUser } from '../src/utils/is-authorized-user.js';
 
 import issueCommentRollCreatedEvent from './fixtures/issue_comment_roll.created.json' with { type: 'json' };
 
@@ -44,7 +44,7 @@ describe('roller', () => {
     });
 
     it('rolls Chromium when an authorized user comments /roll main', async () => {
-      vi.mocked(isAuthorizedUser).mockResolvedValue(true);
+      vi.mocked(isAuthorizedElectronRepoUser).mockResolvedValue(true);
 
       nock(GH_API)
         .post('/repos/electron/electron/issues/0/comments', ({ body }) => {
@@ -59,7 +59,7 @@ describe('roller', () => {
     });
 
     it('rolls Node.js when an authorized user comments /roll main', async () => {
-      vi.mocked(isAuthorizedUser).mockResolvedValue(true);
+      vi.mocked(isAuthorizedElectronRepoUser).mockResolvedValue(true);
 
       nock(GH_API)
         .post('/repos/electron/electron/issues/0/comments', ({ body }) => {
@@ -75,8 +75,21 @@ describe('roller', () => {
       expect(handleNodeCheck).toHaveBeenCalledWith('main');
     });
 
+    it('ignores roll commands from repositories other than electron/electron', async () => {
+      vi.mocked(isAuthorizedElectronRepoUser).mockResolvedValue(true);
+
+      const event = JSON.parse(JSON.stringify(issueCommentRollCreatedEvent));
+      event.payload.repository.name = 'other-repo';
+      event.payload.repository.full_name = 'electron/other-repo';
+      await probot.receive(event as Parameters<typeof probot.receive>[0]);
+
+      expect(isAuthorizedElectronRepoUser).not.toHaveBeenCalled();
+      expect(handleChromiumCheck).not.toHaveBeenCalled();
+      expect(handleNodeCheck).not.toHaveBeenCalled();
+    });
+
     it('blocks unauthorized users from triggering a roll', async () => {
-      vi.mocked(isAuthorizedUser).mockResolvedValue(false);
+      vi.mocked(isAuthorizedElectronRepoUser).mockResolvedValue(false);
 
       nock(GH_API)
         .post('/repos/electron/electron/issues/0/comments', ({ body }) => {