Skip to content

merge-bot: GITHUB_TOKEN permissions are unused, since every write goes through the App token #521

Description

@ptr727

Raised by Copilot against ptr727/PhotoCleaner#38, which vendors the canonical merge-bot workflow. Filing it here rather than fixing it there, because the file is carried appliesTo: "*" and a one-repo change would diverge from the canonical and be reverted by the next re-vendor.

The finding

Every write in .github/workflows/merge-bot-pull-request.yml authenticates with the App token:

env:
  GH_TOKEN: ${{ steps.app-token.outputs.token }}

gh pr merge and gh pr merge --disable-auto both read GH_TOKEN, and actions/create-github-app-token authenticates from the client id and private key rather than from GITHUB_TOKEN. No job checks out code. So the job-level grants

permissions:
  contents: write
  pull-requests: write

hand GITHUB_TOKEN write access that nothing in the job uses.

Why it is worth acting on

This is a pull_request_target workflow holding an App private key, which is the highest-blast-radius shape in the fleet. The grant is not currently exploitable, since no step consumes GITHUB_TOKEN, but it is standing write access on the one workflow where a future step added carelessly would inherit it. Least privilege here is cheap.

It affects all four jobs (merge-dependabot, merge-codegen, merge-upstream-version, disable-auto-merge-on-maintainer-push) and therefore every repo carrying the file.

What needs deciding

  1. Whether to drop the job-level permissions blocks entirely or set them explicitly to none, and whether a workflow-level permissions: {} is the better shape given a future job might legitimately need GITHUB_TOKEN.
  2. Whether the audit should check this. The file is interface fidelity with a requiredJobKeys contract, so permissions are not compared today and the drift would not be flagged on any repo.
  3. The re-vendor sweep, since every repo carrying the file needs the corrected copy.

Verification note

I have not changed this in PhotoCleaner#38, which carries the canonical's permissions as-is. That PR's Copilot findings on this point are answered by pointing here.

Raised per AUDIT.md section 10, "fix systemic drift in the hub, not per repo", and following the ptr727/PhotoCleaner#33 precedent, where a mechanism affecting every repo of a type was reverted out of a single repo and returned to the hub.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions