Skip to content

feat: add authorize-commenter action - #286

Draft
cryptodev-2s wants to merge 4 commits into
mainfrom
feat/authorize-commenter
Draft

feat: add authorize-commenter action#286
cryptodev-2s wants to merge 4 commits into
mainfrom
feat/authorize-commenter

Conversation

@cryptodev-2s

@cryptodev-2s cryptodev-2s commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Workflows triggered by issue_comment run from the default branch with access to secrets. The two in MetaMask/core that use them, update-changelogs and publish-preview, gate only on the comment body, so on a public repository anyone who can comment can start them. publish-preview publishes to npm. There is nothing shared to check the commenter, so today each workflow would have to grow its own copy of the check.

This adds an authorize-commenter action. It reads the commenter's repository permission and fails unless that permission is write or admin. A workflow runs it as its first job and gives every privileged job needs on that job, so a refusal skips them.

A few notes on the choices:

  • It fails rather than returning an authorized output. Action outputs are strings, and every non empty string is truthy in a workflow expression, so a consumer writing the natural if: needs.authorize.outputs.authorized would get 'false' and a wide open gate. Failing removes the trap instead of documenting around it, and it means the integration is just needs, with no condition to forget.
  • It uses the permission API rather than github.event.comment.author_association. The association returns NONE for org members whose membership is private, and its COLLABORATOR value does not distinguish a read only collaborator from one who can push.
  • The permission field collapses custom roles onto admin, write, read and none, so admin and write are exactly the logins that can push. Everyone else reads back as read on a public repository and none on a private one.
  • The default GITHUB_TOKEN is enough. I was unsure whether that endpoint would need a token minted by get-token, so I tested it: with only contents: read it reads write for a maintainer and read for octocat. See the run on this pull request.

test-authorize-commenter.yml covers both directions, a login with write access being authorized and octocat being refused. It triggers on pull_request directly rather than through workflow_call like the other test-*.yml workflows, because those are not wired into main.yml and so never run. The positive case is skipped on fork pull requests, where the author legitimately has read access.

Worth knowing for whoever adopts this: it authorizes the person who wrote the comment, not the code they wrote it on. A workflow that passes this gate and then checks out the pull request head or merge ref runs untrusted code with exactly the privileges the gate was protecting. That caveat is in the action description too.

Adoption in MetaMask/core will be a separate pull request covering both workflows. See MetaMask/core#10077, where this came up.

@cryptodev-2s cryptodev-2s self-assigned this Sep 2, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit d30ca1c. Configure here.

if: github.event.pull_request.head.repo.fork == false
uses: ./.github/actions/authorize-commenter
with:
username: ${{ github.actor }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bot PRs fail write-access test

Low Severity

The write-access step treats github.actor as a collaborator with push rights on every non-fork pull request. Same-repository bot PRs, including Dependabot, usually get none from the permission API, so that step fails and the new check goes red.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit d30ca1c. Configure here.

@cryptodev-2s
cryptodev-2s marked this pull request as draft September 2, 2026 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant