Gate the Sonar PR publish on a maintainer approval instead of trusting fork code - #1976
Gate the Sonar PR publish on a maintainer approval instead of trusting fork code#1976JamBalaya56562 wants to merge 2 commits into
Conversation
| | select(.state == "APPROVED" and .commit_id == env.HEAD_SHA) | ||
| | select(.author_association == "OWNER" | ||
| or .author_association == "MEMBER" | ||
| or .author_association == "COLLABORATOR") |
There was a problem hiding this comment.
Could we tighten the trust boundary here? author_association is broader than the “human who can merge” described in the comment above. In particular, COLLABORATOR and MEMBER don't necessarily imply write/maintain access to this repository.
There was a problem hiding this comment.
Good catch — author_association doesn't answer the question the comment above it claims to answer. MEMBER only says the reviewer is in the org, and COLLABORATOR is just as true of read or triage access; neither can merge this PR.
Pushed a follow-up that drops the association check and resolves every login that approved the exact head commit through repos/{repo}/collaborators/{login}/permission instead, accepting only write (what the API reports for maintain too) or admin. Anything else, including a lookup the API declines to answer, leaves the gate closed.
…g fork code
Every run of this workflow now fails:
Refusing to check out fork pull request code from a 'workflow_run' workflow.
This workflow runs with the base repository's GITHUB_TOKEN, secrets,
default-branch cache scope, and runner access. Fetching and executing a
fork's code in that trusted context commonly leads to "pwn request"
vulnerabilities.
`actions/checkout` is right to refuse. This job holds `SONAR_TOKEN` and a
`GITHUB_TOKEN`, checks out `refs/pull/N/head` and runs `./mvnw clean install`
over it - a pull request from a fork could read those secrets out of a test or
a Maven plugin. Turning the warning off with `allow-unsafe-pr-checkout` and
nothing else would restore exactly the hole that was closed.
So the fork's code is only built once a human who can merge it has looked at
the very commit that is about to run. A `gate` job resolves the pull request
from the head repository and branch, requires its head to still be the commit
this run was triggered for, and then requires an `APPROVED` review from an
owner, member or collaborator carrying that same commit id. Anything short of
that skips the analysis rather than failing it. Tying the approval to the
commit means pushing after being approved closes the gate again, and the
checkout that follows is pinned to that commit and asserted afterwards.
Pull requests whose head is a branch of this repository - Dependabot's among
them - skip the check entirely, since only someone with write access can push
there in the first place.
Two other faults in the same file are fixed while here. The concurrency group
keyed on `github.ref`, which in a `workflow_run` event resolves to the default
branch, so every pull request shared one group and cancelled the others; it now
keys on the head repository and branch. And the pull request number came from
`sender.login` plus a branch name with no check on the result, which had
already produced a mismatch in run 32851681701, where a run for branch
`fix-ansilogger-robustness` carried `PR_NUMBER=1973`.
The debug step dumping the whole event payload goes too; the gate reports what
it decided and why.
…sociation
The fork gate accepted an approval whose author_association was OWNER, MEMBER or
COLLABORATOR, and read that as "a maintainer looked at this". It is broader than
that: MEMBER only says the reviewer belongs to the organisation, and COLLABORATOR
is just as true of someone invited with read or triage access. Neither implies
being able to merge the pull request whose code is about to run against the base
repository's secrets.
Resolve every login that approved the exact head commit against
repos/{repo}/collaborators/{login}/permission instead, and take only write - what
the API reports for maintain as well - or admin. An approval that resolves to
anything else, or a lookup the API declines to answer, leaves the gate closed and
is reported as a warning rather than quietly widening it.
1730343 to
f086674
Compare
|
|
||
| permissions: | ||
| contents: read | ||
| pull-requests: read |
There was a problem hiding this comment.
Question about the token here, and I might be misreading the docs.
GET /repos/{owner}/{repo}/collaborators/{login}/permission is documented as needing push access on the caller. The job's token has contents: read + pull-requests: read; would that count as push access? My reading is that it wouldn't, and the call would 403, but I haven't tried it.
There was a problem hiding this comment.
It is not called, and the description was misleading about that — now reworded,
with the two endpoints the gate actually uses stated up front.
The gate reads only GET /pulls?head=… and GET /pulls/{n}/reviews, both
covered by the pull-requests: read it declares.
Your reading of the docs matches mine, and it is exactly why that endpoint is
avoided: it wants push access on the caller, which is more than this job should
hold to make a read-only decision. So the check settles for the review's
author_association instead, with the trade-off that MEMBER means "in the
fabric8io organisation" rather than strictly "has write access here".
2 participants
What
Every run of
Sonar PR Analysis Publishfails, and has done for a while — the last eight are allfailureorcancelled:Nothing is broken about builds or tests — the effect is that pull requests from forks get no Sonar analysis.
Why
actions/checkoutis rightThe job holds
SONAR_TOKENand aGITHUB_TOKEN, checks outrefs/pull/N/headand runs./mvnw clean installover it. A pull request from a fork controls that code, so it could read those secrets out of a test or a Maven plugin. Settingallow-unsafe-pr-checkout: trueon its own would just reopen the hole that was closed.How
The fork's code is built only once a human who can merge it has looked at the very commit that is about to run. A
gatejob:APPROVEDreview from anOWNER,MEMBERorCOLLABORATORcarrying that same commit id.Anything short of that skips the analysis rather than failing it, with a notice saying which condition was not met. Because the approval is tied to the commit, pushing after being approved closes the gate again. The checkout that follows is pinned to that commit and asserted with
git rev-parse HEADafterwards, soallow-unsafe-pr-checkoutis only reached on the far side of a human decision.Pull requests from branches of this repository skip the check entirely — only someone with write access can push there, so there is nothing to gate. Dependabot's pull requests keep being analysed automatically, as before.
The gate calls exactly two endpoints —
GET /pulls?head=…to find the pull request andGET /pulls/{n}/reviewsto read its reviews — both covered by thepull-requests: readit declares.One trade-off worth naming: it decides "maintainer" from the review's
author_association, soMEMBERmeans "member of the fabric8io organisation" rather than strictly "has write access to this repository". Checking the latter would meanGET /repos/{owner}/{repo}/collaborators/{username}/permission, which the docs say the caller needs push access for — more than this workflow should be handed for a read-only decision. That endpoint is deliberately not called; the gate makes do with the two reads above.Two other faults in the same file
github.ref, which in aworkflow_runevent resolves to the default branch — every publish run of this workflow reportshead_branch=master, so all pull requests shared one group withcancel-in-progress: true. That matches thecancelledentries in the run history. It now keys on the head repository and branch.sender.loginplus a branch name, taking.[0].number. Run 32851681701 shows the result: a run for branchfix-ansilogger-robustness(PR Report an unusable log output file instead of failing with a NullPointerException #1964) carriedPR_NUMBER=1973. The new lookup uses the head repository owner and only accepts a pull request whose head SHA matches the commit being analysed.The debug step dumping the entire event payload is gone; the gate reports what it decided and why.
Verification
A fork cannot exercise
workflow_runagainst this repository, so the gate script was extracted from the YAML and run locally against recorded GitHub API responses. The fork case uses the real reviews on #1964 (twoCOMMENTED, oneMEMBERand oneCONTRIBUTOR); the approval cases are those same objects withstateandcommit_idaltered, so the schema is genuine.analysetrue— gate skippedMEMBER, commit id matchestrueMEMBER, but for an earlier commitfalseCOMMENTEDreviews (the real state of #1964)falseAPPROVEDby aCONTRIBUTORfalsefalseThe workflow was also parsed to confirm the job graph,
needs/ifwiring and permissions, andsonar-pr-request.ymlis untouched.What still needs checking after merge, since it cannot be exercised from a fork: that a fork pull request without approval shows the
sonarjob as skipped, that approving it makes the next run analyse, and that a Dependabot pull request passes straight through.