fix: authorize roll commands against electron/electron only#196
Merged
dsanders11 merged 2 commits intoJun 25, 2026
Conversation
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 <branch>' 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LL7ZzLv5mo4Wqf3BiWk8HB
dsanders11
requested changes
Jun 25, 2026
Member
There was a problem hiding this comment.
I don't think the change in this file is required? This should be either kept as a generic isAuthorizedUser for the repo from context (generic and usable in other parts of the codebase) or renamed to isAuthorizedElectronRepoUser to make it clear it's not a generic function. I'm in favor of the former.
Member
Author
There was a problem hiding this comment.
The point here is this is used as a "does the user have authority to do stuff on e/e" but the check runs in the context of the webhook which could be any repo roller is installed into (which is quite a few)
Member
Author
There was a problem hiding this comment.
I'll rename this method for now (isAuthorizedElectronRepoUser is accurate)
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LL7ZzLv5mo4Wqf3BiWk8HB
dsanders11
approved these changes
Jun 25, 2026
dsanders11
deleted the
claude/autopatch-scan-c14528b3-vuln-1895936-w8x2lh
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Roller is installed on multiple repos, no need to get wires crossed