Skip to content

fix(ci): use pull_request_target in readme-pr-check so fork PRs work#3840

Open
yoryocoruxo-ai wants to merge 1 commit intomodelcontextprotocol:mainfrom
yoryocoruxo-ai:fix/readme-pr-check-fork-permissions
Open

fix(ci): use pull_request_target in readme-pr-check so fork PRs work#3840
yoryocoruxo-ai wants to merge 1 commit intomodelcontextprotocol:mainfrom
yoryocoruxo-ai:fix/readme-pr-check-fork-permissions

Conversation

@yoryocoruxo-ai
Copy link
Copy Markdown

@yoryocoruxo-ai yoryocoruxo-ai commented Apr 5, 2026

Summary

The README PR Check workflow is currently failing on every PR opened from a fork (which is the vast majority of contributions to this repo). Example failed runs from the last couple of days: #24010860831, #24010684932, #24007350839, #24003674827, and many more — the full list on the workflow page shows the pattern: anything from a fork is red, only the one internal-branch run is green.

All of them fail the same way:

HttpError: Resource not accessible by integration
POST /repos/modelcontextprotocol/servers/issues/{n}/labels
status: 403
x-accepted-github-permissions: issues=write; pull_requests=write

Root cause

The workflow triggers on pull_request. When a pull_request event comes from a fork, the GITHUB_TOKEN is read-only regardless of the permissions: block in the job — that is the documented GitHub Actions security model. So addLabels / createComment can never succeed for the 99% case this workflow was written for.

Fix

  1. Switch the trigger to pull_request_target. This runs the workflow in the context of the base repo, so the permissions: block is actually honored and the token can write labels/comments.
  2. Add issues: write next to pull-requests: write, since addLabels and createComment both hit the /issues/ endpoint (the 403 response header even tells us: x-accepted-github-permissions: issues=write; pull_requests=write).
  3. Update the if: guard on check-readme-only to match the new event name.

Why pull_request_target is safe here

pull_request_target is normally dangerous because it runs with write privileges against github.event.pull_request.head.ref — i.e. attacker-controlled code. This workflow never does a checkout and never executes anything from the PR. It only calls octokit (pulls.listFiles, issues.addLabels, issues.createComment) which read metadata and write labels/comments. There is no code-execution surface from the fork.

Test plan

  • Merge and confirm the next fork PR against README.md gets labeled readme: pending and commented instead of erroring
  • Confirm /i-promise-this-is-not-a-new-server still swaps labels correctly (the handle-confirmation job was already fine — it runs on issue_comment which doesn't have the fork-token problem — but I added issues: write there too for consistency since it calls addLabels / removeLabel)
  • Confirm non-README PRs are still skipped (the files.length !== 1 || files[0].filename !== 'README.md' check is unchanged)

The README PR Check workflow triggers on 'pull_request', which gives the
GITHUB_TOKEN read-only permissions when the PR comes from a fork. This
caused every fork PR (the vast majority) to fail with:

  HttpError: Resource not accessible by integration (403)
  POST /repos/.../issues/{n}/labels

Switch to 'pull_request_target' so the workflow runs in the base repo
context with the permissions declared in the job. This is safe here
because the workflow never checks out PR code — it only calls the GitHub
REST/GraphQL API to add a label and post a comment.

Also add 'issues: write' alongside 'pull-requests: write' since
addLabels/createComment use the issues endpoint internally.
@cliffhall cliffhall added the bug Something isn't working label Apr 20, 2026
@cliffhall
Copy link
Copy Markdown
Member

@claude review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 21, 2026

Claude finished @cliffhall's task in 1m 25s —— View job


PR Review

  • Read the changed workflow file
  • Analyze the diff and security implications
  • Post review feedback

Summary: This is a correct, minimal, and well-reasoned fix. The diagnosis, solution, and security justification are all sound. Here's a detailed breakdown:


What changed (4 lines)

Location Before After
Trigger event pull_request: pull_request_target:
check-readme-only if: guard github.event_name == 'pull_request' github.event_name == 'pull_request_target'
check-readme-only permissions pull-requests: write pull-requests: write + issues: write
handle-confirmation permissions pull-requests: write pull-requests: write + issues: write

Root cause analysis ✅

The diagnosis is correct. GitHub's documented security model grants the GITHUB_TOKEN read-only permissions for fork PRs on the pull_request event, regardless of what's declared in the permissions: block. This is intentional and cannot be overridden at the job level — switching the event trigger is the only correct solution.


Security assessment ✅ Safe

pull_request_target is the right fix, and the security justification in the PR description is accurate. The critical risk with pull_request_target is when a workflow checks out and executes code from the PR branch (attacker-controlled) with write tokens. This workflow does neither:

The entire script body only calls the GitHub REST/GraphQL API:

  • pulls.listFiles() — reads metadata
  • issues.listComments() — reads comments
  • issues.addLabels() — writes a label
  • issues.createComment() — posts a comment
  • issues.removeLabel() / graphql minimizeComment — in the confirmation handler

No actions/checkout step, no run: steps, no code from the PR is ever fetched or executed. The attack surface is zero.


Permission additions ✅ Correct

Adding issues: write is necessary. The 403 response header the PR cites (x-accepted-github-permissions: issues=write; pull_requests=write) is the definitive signal — addLabels and createComment both route through the /issues/ API endpoint and require this scope. Adding it to handle-confirmation as well is consistent and correct since that job also calls addLabels and removeLabel.


Minor observations (not blockers)

  1. types: [opened] only — a PR that starts with non-README files and is then amended to only touch README won't trigger the check. This pre-existed and is out of scope here.

  2. No synchronize event — similarly, if a contributor pushes new commits to a README-only PR, the workflow won't re-evaluate. Again, pre-existing behavior.

  3. contents: read — this permission remains in check-readme-only but is unused (no checkout). Harmless, but could be dropped in a follow-up cleanup if desired.


Verdict

Approve. The fix is correct, the minimal diff is appropriate, and the pull_request_target usage is demonstrably safe for this workflow's specific API-only operations. The PR description provides unusually good documentation of the security rationale, which is appreciated for a change like this.

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants