fix(2157): measure the two protection-readers, and scope both (backend#2157) - #328
fix(2157): measure the two protection-readers, and scope both (backend#2157)#328LukasWodka wants to merge 1 commit into
Conversation
Their exemption reason was that branch protection can return LESS to a narrower token rather than erroring -- a 200 with a field missing, which a checker reads as "not configured" on a green run. Right worry, so it was measured instead of argued, and it landed on the other workflow. bricked-prs: protection does NOT degrade silently. Without administration:read it answers 403 and read_protection() reports an error. Narrowed to five READ scopes; bricked-prs.py over four repos gives output identical to the full grant. actions:read is required and non-obvious -- it is what lets GraphQL resolve statusCheckRollup -> commit.status on a private repo. merge-settings-drift: reads no branch protection at all. It reads GET /repos, and that endpoint IS the silent one -- the four merge-setting fields are returned only to a caller with PUSH access, 200 either way. contents:write is the one grant it measurably cannot lose; five others go. Every dropped permission was re-added one at a time to prove it load-bearing. Refs tracebloc/backend#2157 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
saadqbal
left a comment
There was a problem hiding this comment.
Nice, careful PR 👍 — and the reason it's approvable in one pass is that the two claims that could have been wrong were measured instead of argued.
I verified both. merge-settings-drift.yml reads no branch protection at all — zero matches — so the old exemption row was wrong about which endpoint its own workflow used, which is a good argument for measuring rather than reasoning. And contents: write is genuinely load-bearing there: the measurement table (19 readable at contents: write, 0 at contents: read) matches the API's behaviour of returning those four merge-setting fields only to a push-access caller, 200 with the keys simply absent otherwise. Documenting it as "a READ requirement, not a write one" with a don't-fix-this header is the right call, since that is exactly the scope someone will try to narrow next.
On the other side, read_protection() in caller-drift.py confirms the bricked-prs finding: 404 is treated as a fact, and everything else — including the 403 you get without administration: read — sets out.error and returns. So it fails closed, and the row's "may return less, not error" worry was the wrong shape for that workflow. Narrowing it to five read scopes, each annotated with the call that needs it, is the outcome that worry should always have had.
The part I'd keep: the four remaining exemptions now carry a decision ("needs its own window", high exposure) rather than "not yet measured", and the comment says not to fold the two kinds back together. That distinction is what stops an exemption list turning back into a to-do list nobody reads.
What this does
Six exemptions in
scripts/mint-scope.py, two of them held back for a statedreason rather than for lack of time:
That is a real failure mode and it is invisible to a green run, so it was
measured rather than derived. Both rows drain.
How it was measured
App tokens were minted at 14 different scope combinations against the live
endpoints, then -- the part that matters -- each workflow's own script was run at
the candidate scope and compared to the full grant, and then every permission was
removed one at a time to prove it load-bearing. A scope that merely looks
sufficient and one that is are indistinguishable from a field dump; they are not
indistinguishable from the script's exit code.
Runs: round 1 *
round 2 *
round 3
(read-only; every call a GET, on a throwaway branch now deleted).
The result, which is not what either row predicted
bricked-prs.yml-> five READ scopesBranch protection does not degrade silently. Without
administration: read,branches/{b}/protectionanswers 403, whichread_protection()turns into anerror -- the audit fails closed exactly as designed. It also keeps the 404
"Branch not protected" on an unprotected branch, the fact
read_protection()relies on to tell no classic protection from could not read.
bricked-prs.pyover backend + client-runtime + client + .githubCOULD NOT AUDIT, exit 1COULD NOT AUDIT, exit 1 -- identicalactions: readCOULD NOT AUDIT, exit 2administration: readCOULD NOT AUDIT, exit 2administration:readactions:readchecks:readcontents:readpull-requests:readactions: readis the one nobody would guess, and it is not for any ActionsAPI.
gh pr list --json statusCheckRollupresolvescommit.statusunderneath;without
actions: readGraphQL refuses that subfield on a private repo whileevery other call is untouched. Nothing in the docs connects the two -- only
removing it shows the link. (An earlier probe against
tracebloc/clientappearedto pass without it;
clientis public. Visibility, not permission.)contentsdrops from write to read;issues: writeandorganization-projects: writego entirely.merge-settings-drift.yml-> the row was wrong about the endpointIt reads no branch protection at all. It reads
GET /repos/{owner}/{repo}--and that is the endpoint that degrades silently. The four merge-setting fields
are returned only to a caller with PUSH access; otherwise a 200 with the keys
simply absent.
contents:write+issues:writecontents:read +issues:writeadministration:read, nocontents:writecontents:write, noadministrationSo
administration: readis neither necessary nor sufficient here -- theopposite of what this file and its own report claimed. It is dropped, along with
actions:read,checks:read,pull-requests:writeandorganization-projects:write.contents: writestays, and that is the uncomfortable part. It is the onegrant backend#2157 set out to delete everywhere, and this is the one workflow that
measurably cannot lose it: the grant buys this job a read it cannot otherwise
perform. Narrowing it to
readdoes not fail at mint time -- it fails 19 timesinside the loop, where the existing non-boolean guard catches it and reddens the
run. Loud, but only because that guard exists.
Two prose fixes that came with the measurement
The workflow's fail-closed guard and its issue-comment remediation both said the
merge fields need admin. They need push. An operator following the old text
would have gone looking at
administrationwhile the token had lostcontents: write. Both now name the real cause, and say that every repo landingin
failed(unreadable)at once is certainly that.What is NOT proven
gh issue commenton backend#1337 was not exercised -- writing a comment toprove a write scope is not a read-only measurement. It is unchanged: the workflow
held
issues: writebefore this PR and holds it after, so there is no delta toregress. Everything else was run end to end.
Verification
mint-scope.py0 findings *mint-scope-selftest.py13 passed *make checkgreen (95 passed) *
actionlint+yaml.safe_loadon both workflows.Four rows remain (
fr-gate,set-pr-status,standards-sync,kanban-reconcile) -- the high-exposure set, each needing its own window.Refs tracebloc/backend#2157
Note
Medium Risk
Changes GitHub App installation token permissions for org-wide audit jobs. Wrong scopes can fail closed or silently omit merge-setting fields, but the new grants were measured against live endpoints.
Overview
Scopes two org-wide audit workflows that previously minted the App’s full grant, after measuring live endpoints instead of deriving from docs.
mint-scope.pyexemptions drop from six to four (fr-gate,set-pr-status,standards-sync,kanban-reconcile).bricked-prs.ymlnow requests five read permissions:administration,actions,checks,contents,pull-requests. Classic protection 403s withoutadministration:read(fail-closed).actions:readis required for GraphQLstatusCheckRollup/commit.statuson private repos, not for the Actions API.merge-settings-drift.ymlkeepscontents: writeplusissues: write. Merge-setting fields onGET /repos/{o}/{r}are returned only with push access;administrationis neither necessary nor sufficient. Comments and the issue-comment remediation now say that, so an all-unreadable run is diagnosed as a lostcontents: writegrant rather than missing admin.Reviewed by Cursor Bugbot for commit 776cb4a. Bugbot is set up for automated code reviews on this repo. Configure here.