From e67a9aad5a3258ae30b60cd9ee794a9f3d357864 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 25 Jun 2026 00:44:21 +0000 Subject: [PATCH 1/2] fix: authorize roll commands against electron/electron only The /roll issue-comment handler authorized the commenter via getCollaboratorPermissionLevel(context.repo({username})), where context.repo() is derived from the repository the webhook comment was posted in rather than electron/electron, the repository the privileged roll actions actually modify. With an org-wide or multi-repo app installation, a user with write/admin on any other installed repository could post '/roll ' on a PR titled 'chore: bump chromium ...' and pass the authorization check, triggering handleChromiumCheck / handleNodeCheck against electron/electron. Bind the privilege check to the resource being acted upon: - isAuthorizedUser now evaluates the commenter's permission against REPOS.electron explicitly instead of the webhook's source repo. - The /roll command is only honored for comments originating from electron/electron. - Apply the same origin guard to the pull_request.closed handler, which reached the identical privileged rolls from a merged 'chore: bump ...' PR in any other installed repository. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01LL7ZzLv5mo4Wqf3BiWk8HB --- src/index.ts | 18 ++++++++++++++++-- src/utils/is-authorized-user.ts | 16 +++++++++++----- tests/index.spec.ts | 13 +++++++++++++ 3 files changed, 40 insertions(+), 7 deletions(-) diff --git a/src/index.ts b/src/index.ts index a3230c7..8469b4a 100644 --- a/src/index.ts +++ b/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 { REPOS, ROLL_TARGETS } from './constants.js'; import { isAuthorizedUser } 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,6 +68,15 @@ 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; diff --git a/src/utils/is-authorized-user.ts b/src/utils/is-authorized-user.ts index 62a0434..86f45ff 100644 --- a/src/utils/is-authorized-user.ts +++ b/src/utils/is-authorized-user.ts @@ -1,14 +1,20 @@ import type { Context } from 'probot'; +import { REPOS } from '../constants.js'; + export async function isAuthorizedUser( 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); } diff --git a/tests/index.spec.ts b/tests/index.spec.ts index 6c6c4b5..3fd05e4 100644 --- a/tests/index.spec.ts +++ b/tests/index.spec.ts @@ -75,6 +75,19 @@ describe('roller', () => { expect(handleNodeCheck).toHaveBeenCalledWith('main'); }); + it('ignores roll commands from repositories other than electron/electron', async () => { + vi.mocked(isAuthorizedUser).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[0]); + + expect(isAuthorizedUser).not.toHaveBeenCalled(); + expect(handleChromiumCheck).not.toHaveBeenCalled(); + expect(handleNodeCheck).not.toHaveBeenCalled(); + }); + it('blocks unauthorized users from triggering a roll', async () => { vi.mocked(isAuthorizedUser).mockResolvedValue(false); From 26d435cd3aa52d36c15bcc6cf02072d78030b2e0 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 25 Jun 2026 01:01:29 +0000 Subject: [PATCH 2/2] refactor: rename isAuthorizedUser to isAuthorizedElectronRepoUser Address review feedback from dsanders11: the authorization check is now hardcoded to evaluate permissions against electron/electron, so the generic name was misleading. Rename it to reflect that it specifically authorizes users against the electron/electron repository. No behavior change. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01LL7ZzLv5mo4Wqf3BiWk8HB --- src/index.ts | 4 ++-- src/utils/is-authorized-user.ts | 2 +- tests/index.spec.ts | 12 ++++++------ 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/index.ts b/src/index.ts index 8469b4a..ebc72ea 100644 --- a/src/index.ts +++ b/src/index.ts @@ -5,7 +5,7 @@ import { handleChromiumCheck } from './chromium-handler.js'; import { handleBuildImagesCheck } from './build-images-handler.js'; import { handleBuildImagesChromiumDepsCheck } from './build-images-chromium-deps-handler.js'; import { REPOS, ROLL_TARGETS } from './constants.js'; -import { isAuthorizedUser } from './utils/is-authorized-user.js'; +import { isAuthorizedElectronRepoUser } from './utils/is-authorized-user.js'; const handler = (robot: Probot) => { robot.on('pull_request.closed', async (context) => { @@ -83,7 +83,7 @@ const handler = (robot: Probot) => { } // 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({ diff --git a/src/utils/is-authorized-user.ts b/src/utils/is-authorized-user.ts index 86f45ff..b8e74b6 100644 --- a/src/utils/is-authorized-user.ts +++ b/src/utils/is-authorized-user.ts @@ -2,7 +2,7 @@ import type { Context } from 'probot'; import { REPOS } from '../constants.js'; -export async function isAuthorizedUser( +export async function isAuthorizedElectronRepoUser( context: Context<'issue_comment.created'>, username: string, ) { diff --git a/tests/index.spec.ts b/tests/index.spec.ts index 3fd05e4..4015a57 100644 --- a/tests/index.spec.ts +++ b/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 }) => { @@ -76,20 +76,20 @@ describe('roller', () => { }); it('ignores roll commands from repositories other than electron/electron', async () => { - vi.mocked(isAuthorizedUser).mockResolvedValue(true); + 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[0]); - expect(isAuthorizedUser).not.toHaveBeenCalled(); + 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 }) => {