fix: greet fork pull requests with a token that can post the comment - #181
Merged
Conversation
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
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.
Why
The greeting workflow has never greeted anyone from a fork.
#179 and #180 were both opened from a fork and both failed the same way, on the call that posts the comment:
The cause is not the
permissions:block. That block already asks for exactly what the endpoint wants, and the 403 response says as much inx-accepted-github-permissions: issues=write; pull_requests=write. What the job was actually handed is in its ownSet up joblog, and it is not what the file asked for:Same workflow file, same
permissions:block, samepull_requesttrigger.permissions:is a ceiling rather than a floor: a pull request from a fork gets a read-only token and no secrets underpull_request, so the write the job asks for is never granted and the greeting cannot be posted.Only forks hit that, which is why nothing noticed since the workflow landed on 4 September. Every green run on
pull_requestcame from a branch in this repo and loggedSkipping...Not First Contribution, because the author was a maintainer. Of the four fork pull requests the workflow has seen, #176 and #177 stalled ataction_requiredand never ran at all, and #179 and #180 ran and 403'd. Zero greetings posted, and the workflow reported success the whole time.What it does
Moves the pull request trigger to
pull_request_target, which runs the same job in the base repository's context and therefore with the write token the comment needs. Theissuestrigger, thepermissions:block and both message bodies are untouched.actions/first-interactionneeds no change to suit it: it branches onpayload.pull_requestand posts tocontext.issue.number, andpull_request_targetdelivers the samepull_requestpayload withaction: opened.The decision worth arguing with
pull_request_targetis the well-known footgun, so it deserves stating plainly rather than leaving in a comment. It runs with a write token and secret access in the base repository's context, and the way that gets exploited is a workflow that checks out and then executes code from the pull request.This job does neither. It is a single step, there is no
actions/checkoutanywhere in the file, and underpull_request_targetthe workflow definition is read frommainrather than from the pull request, so a fork cannot edit this file to change what runs.The residual risk I would rather name than bury:
actions/first-interaction@v3is a mutable tag, and after this change it runs with a write token and secret access where before it got a read-only one. If that tag were ever moved to hostile code, the blast radius is larger than it was. Pinning shuts it:I have left it out to keep this the one-line fix it should be, and because nothing else in
.github/workflowspins by SHA either: all 17 distinctuses:in this repo are tags. That makes it a repo-wide question rather than something to settle quietly here. Say the word and I will add it, on this pull request or across all the workflows.What is tested and what is not
Nothing here is covered by a test, and there is no workflow linter in the repo to add one to. What I did check by hand:
on:mapping. The trigger set really isissues: [opened]pluspull_request_target: [opened].github.context.payload.pull_requestandgithub.context.issue.number, both of whichpull_request_targetsupplies.What I cannot show is a greeting actually being posted. No run of this workflow has ever reached
createCommentsuccessfully on any trigger,issuesincluded, so the success path is unproven rather than repaired: this change removes a failure that was definitely happening, and does not prove the happy path. The next first-time contributor is the real test. If it fails again, read theGITHUB_TOKEN Permissionsgroup at the top of the run log first, since that is what made the diagnosis obvious here.I also expect, but have not confirmed, that this clears the
action_requiredstall that caught #176 and #177, on the reasoning thatpull_request_targetruns base-branch code rather than fork code. The documentation I could find does not say so plainly, so please treat that as a hope and not a claim.