Skip to content

fix(2157): measure the two protection-readers, and scope both (backend#2157) - #328

Open
LukasWodka wants to merge 1 commit into
developfrom
fix/2157-protection-reader-scopes
Open

fix(2157): measure the two protection-readers, and scope both (backend#2157)#328
LukasWodka wants to merge 1 commit into
developfrom
fix/2157-protection-reader-scopes

Conversation

@LukasWodka

@LukasWodka LukasWodka commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

What this does

Six exemptions in scripts/mint-scope.py, two of them held back for a stated
reason rather than for lack of time:

reads branch protection; a narrower token may return less, not error

That is a real failure mode and it is invisible to a green run, so it was
measured rather than derived. Both rows drain.

before:  16 mint step(s) across 36 workflow file(s) / 6 unscoped, 6 exempted, 0 finding(s)
after:   16 mint step(s) across 36 workflow file(s) / 4 unscoped, 4 exempted, 0 finding(s)

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 scopes

Branch protection does not degrade silently. Without administration: read,
branches/{b}/protection answers 403, which read_protection() turns into an
error -- 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.

scope bricked-prs.py over backend + client-runtime + client + .github
full grant 4 findings, 0 COULD NOT AUDIT, exit 1
the five below 4 findings, 0 COULD NOT AUDIT, exit 1 -- identical
minus actions: read 3 COULD NOT AUDIT, exit 2
minus administration: read 12 COULD NOT AUDIT, exit 2

administration:read actions:read checks:read contents:read pull-requests:read

actions: read is the one nobody would guess, and it is not for any Actions
API.
gh pr list --json statusCheckRollup resolves commit.status underneath;
without actions: read GraphQL refuses that subfield on a private repo while
every other call is untouched. Nothing in the docs connects the two -- only
removing it shows the link. (An earlier probe against tracebloc/client appeared
to pass without it; client is public. Visibility, not permission.)

contents drops from write to read; issues: write and
organization-projects: write go entirely.

merge-settings-drift.yml -> the row was wrong about the endpoint

It 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
.

scope its own audit loop over all 19 active repos
full grant 19 readable, 0 unreadable
contents:write + issues:write 19 readable, 0 unreadable -- identical
contents: read + issues:write 0 readable, 19 unreadable
administration:read, no contents:write 0 readable, 19 unreadable
contents:write, no administration 19 readable, 0 unreadable

So administration: read is neither necessary nor sufficient here -- the
opposite of what this file and its own report claimed. It is dropped, along with
actions:read, checks:read, pull-requests:write and
organization-projects:write.

contents: write stays, and that is the uncomfortable part. It is the one
grant 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 read does not fail at mint time -- it fails 19 times
inside 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 administration while the token had lost
contents: write. Both now name the real cause, and say that every repo landing
in failed(unreadable) at once is certainly that.

What is NOT proven

gh issue comment on backend#1337 was not exercised -- writing a comment to
prove a write scope is not a read-only measurement. It is unchanged: the workflow
held issues: write before this PR and holds it after, so there is no delta to
regress. Everything else was run end to end.

Verification

mint-scope.py 0 findings * mint-scope-selftest.py 13 passed * make check
green (95 passed) * actionlint + yaml.safe_load on 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.py exemptions drop from six to four (fr-gate, set-pr-status, standards-sync, kanban-reconcile).

bricked-prs.yml now requests five read permissions: administration, actions, checks, contents, pull-requests. Classic protection 403s without administration:read (fail-closed). actions:read is required for GraphQL statusCheckRollup/commit.status on private repos, not for the Actions API.

merge-settings-drift.yml keeps contents: write plus issues: write. Merge-setting fields on GET /repos/{o}/{r} are returned only with push access; administration is neither necessary nor sufficient. Comments and the issue-comment remediation now say that, so an all-unreadable run is diagnosed as a lost contents: write grant rather than missing admin.

Reviewed by Cursor Bugbot for commit 776cb4a. Bugbot is set up for automated code reviews on this repo. Configure here.

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 saadqbal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants