diff --git a/src/index.ts b/src/index.ts index a3230c7..ebc72ea 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 { 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({ diff --git a/src/utils/is-authorized-user.ts b/src/utils/is-authorized-user.ts index 62a0434..b8e74b6 100644 --- a/src/utils/is-authorized-user.ts +++ b/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); } diff --git a/tests/index.spec.ts b/tests/index.spec.ts index 6c6c4b5..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 }) => { @@ -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[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 }) => {