feat(runner-policy): add a minimum caller-permission contract term - #385
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 3m 27s —— View job Review complete
I traced the new Everything checks out:
No correctness, security, or maintainability issues found. The separation of Note: I was unable to execute |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a1f4a16f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
|
Verified and merging — with one correction to the blast-radius table, which went stale mid-flight. The table says So provisioning is governed by the new floor. The conclusion still holds, for a stronger reason than stated — not "out of scope", but "in scope and compliant":
Proved rather than reasoned — this branch's checker run against the live
Tests on this branch: 271 pass, 0 fail (up from 264 on Worth noting for the record: had #284 not already added those grants, this floor would have failed provisioning's gate on merge. The ordering was lucky rather than planned — a reason to keep backfilling a floor and granting the scope in the same change where possible. |
`allowedCallerPermissions` is an exact-match waiver the validator refuses unless it carries a `write`, so a callee whose whole requested set is read had no way to state what its callers must grant. `minimumCallerPermissions` adds the missing floor: schema term, ordered comparison (`none` < `read` < `write`) where a caller granting more passes and an unnamed or omitted scope fails closed, and a load-time check that a contract naming both terms is satisfiable. The waiver is untouched. Backfilled onto the three `7107b34` gate contracts, each derived from the callee's own `permissions:` block at that SHA: `pull-requests: read` and `actions: read` on `semantic-pr`, `do-not-merge-gate`, and `pr-issue-linkage`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GitHub downgrades a caller's write grants to read -- and write-only scopes to none -- on forked and Dependabot pull requests unless repository settings permit otherwise, so a caller's declared write is not the access the callee receives. A write floor checked against the declaration would admit exactly the callers it exists to catch. `minimumCallerPermissions` now rejects any non-read value at policy load, with the reason named; a write obligation belongs in `allowedCallerPermissions`, which is reviewed against the calling job rather than inferred from it. The restriction is on what a contract may require, not on how grants compare: the comparison stays ordered, so a caller granting write still clears a read floor, as do `read-all` and `write-all` -- each still clears it after an event-time downgrade lands at read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8a1f4a1 to
2429a22
Compare
Summary
approvedReusableWorkflowContractscould waive an exact caller permission setbut could not require a minimum one, so a consumer repinning to a reusable
that newly requests a read scope passed
Runner policywhile granting less thanthe callee needs. This adds the missing term,
minimumCallerPermissions, andbackfills it onto the three
7107b34gate contracts.allowedCallerPermissionsis unchanged — still an exact-match waiver thevalidator refuses unless it carries at least one
write. No existing contractentry, validator branch, or check is relaxed or removed.
The floor may only require
read. That is the resolution of the Codex P2 onthis PR, and it is what makes the two fields exactly complementary rather than
overlapping — reasoning below.
The term, and why it is separate rather than a relaxation of the waiver
The two fields answer different questions and neither implies the other.
allowedCallerPermissionsminimumCallerPermissionspermissions:block requestswritereadonly — a write floor is rejected at policy loadThe two are exactly complementary, and that is not incidental — see the Codex P2
resolution below. Every write obligation already has a home: the waiver,
reviewed against the calling job, which is the route #384 just used for
zizmor.yml@7107b34'ssecurity-events: write. Every read obligation nowhas one too. Merging them into a single field was considered and rejected: the
write requirement on the waiver is load-bearing — it is what makes the field a
privilege waiver rather than a mirror of the callee's declared permissions —
and the obligation #383 was filed about is entirely read, exactly the shape that
requirement makes inexpressible.
Codex P2, resolved by construction: no write floor can exist
A floor that could require
writewould be unsound. GitHub downgrades acaller's write grants to read — and write-only scopes to none — on forked
and Dependabot pull requests unless repository settings permit otherwise, so
a caller's declared
writeis not the access the callee receives. A write floorcompared against the YAML declaration would pass exactly the callers it exists
to catch.
Rather than model event-time downgrades — the policy can read neither repository
settings nor fork/Dependabot context at validation time, so such a check would be
a guess dressed as a check — the term is restricted to
readvalues. Thatdisposes of the finding by construction rather than by argument: no write floor
can be declared, so none can be silently downgraded.
The rule lives in the validator rather than the schema so the author of a
rejected contract is told why. That is the split
allowedCallerPermissionsalready uses in the opposite direction — the schemapermits an all-read waiver, and the validator rejects it with "must include at
least one write permission". Verified behavior, not intended: running
validatePolicyover the realpolicy.jsonwith the floor onsemantic-pr@7107b34mutated givesThe last two are the schema's own value domain: a floor value must name a real
grant, and
id-tokenis a write-only scope with noreadlevel to require.How permission comparison is ordered
The restriction is on what a contract may require, not on how grants
compare — the ordered comparison is unchanged. GitHub access is ordered
none<read<write, and a called workflow can only downgrade thecaller's
GITHUB_TOKEN, never elevate it (reusable-workflow docs), so thecheck remains a floor, not a match:
writewhere the contract requiresreadpasses;about them;
read-allandwrite-allboth clear a read floor — and each still clears itafter an event-time downgrade lands at
read, which is precisely why aread-only floor is sound where a write floor is not;
repository- or organization-defined defaults this policy cannot read, so they
can never prove the floor.
Job-level permissions override workflow-level, so the comparison runs against
effectivePermissions(workflow, job), the same surface the existing waivercheck uses.
write-allclearing the floor is arithmetic, not absolution: the floor waivesnothing, so a
write-allcaller still meets the ordinaryprivileged-control-planerules. A regression test asserts exactly that.A contract naming both fields is checked for satisfiability when the policy
loads: because the waiver is the only mapping such a caller may present, a
waiver falling short of its own floor would admit nothing at all, so it is
rejected as a configuration error rather than left to fail silently at every
call site.
Auto-approval: deliberately not a decline category
selectorResultInput,allowedCallerPermissions, and a nonemptyallowedSecretseach decline Dependabot auto-approval unconditionally, becauseeach is trusted for something the surface diff never inspects — what the
callee's steps do.
minimumCallerPermissionsis the opposite kind of term: itsays nothing about steps, only what the callee's
permissions:block requests,and that block is already part of the compared surface. A bump that changes it
is declined by the diff; a bump that does not carries the same floor. So the
term is added to
reviewedContractSurface— two surface-matching basesholding different floors must still be caught as ambiguous, and there is a test
for that — but not to the decline list.
(Moot for the three entries here: all three carry
selectorResultInputand arealready declined unconditionally.)
Correction to the backfill list I was given
The task brief said
semantic-pr.ymlanddo-not-merge-gate.ymldeclare onlyactions: read. They do not. Fetched frommelodic-software/ci-workflowsat
7107b34832a7b6db5d08d3b132621c599fbe5e50, each of the three declaresexactly one workflow-level
permissions:block, with no job-level overrideanywhere in the file:
permissions:at7107b34semantic-pr.yml(L93)pull-requests: read,actions: readdo-not-merge-gate.yml(L45)pull-requests: read,actions: readpr-issue-linkage.yml(L61)pull-requests: read,actions: readThis matches the table already recorded in #382's own body, so the brief's list
was the outlier. Each floor is the callee's whole declared set: the callee
narrows to that set, so a caller granting any less starves it.
Blast radius
No consumer breaks today. Nothing in the fleet needs a change before or after
this merges. Verified against the live default branches via
gh api, notlocal clones.
The floor is keyed to
path@SHA, so only callers pinned at7107b34for thesethree paths are governed at all.
runner-policy?7107b34for these three?provisioning.github/runner-policy.jsonand the managed materialization both presentpull-requests: readandactions: readci-runner.github/runner-policy.jsonnor.github/standards/runner-policy/exists (HTTP 404 on both)provisioningwas re-verified after #284 merged mid-flight, against its livedefault branch, in two ways:
gh apion all three caller files, confirming the7107b34pin and bothreadgrants on each calling job; and the component fromthis branch run over a
git archive origin/mainexport of that tree withreal owner evidence —
Identical result under
origin/main's policy, so this PR changes nothing forit.
What the gate actually buys, then: every caller still on an older SHA —
standardsitself,.github,dotfiles,medley,github-iac,claude-code-plugins,codex-plugins— grantspermissions: {}orpull-requests: readon its gate jobs. Each of those now fails pre-merge, inthe repin PR, instead of at workflow startup or with a runtime 403 in the
cancelled-prerequisite resolver.
provisioningreaching the same end state byhand, in #284, is the case for the gate rather than against it: nothing forced
that convergence to include the grants, and nothing would have caught it had it
not. That is why the change lands with zero present-day breakage — it catches
the next repin, not the current state.
Delivery is gated too:
components/runner-policy/policy.jsonis amanagedcomponent in
distribution/sync-manifest.yml, so the tightened policy reacheseach consumer through a reviewed sync PR, never at this PR's merge.
Test plan
Real results, run on this branch, rebased onto
origin/mainat0fb6464(post-#384).
node --test components/runner-policy/runner-policy.test.mjs— 272 pass,0 fail.
origin/mainmeasured the same way (git archive origin/mainintoa clean tree) is 264 pass, 0 fail; +8 net tests, three of them
table-driven case sets.
npm run lint:runner-policy— Runner policy passed.validatePolicycompilespolicy.schema.jsonwith Ajv 2020(
strict: true) on every run above, so the lint and test runs areschema-validating runs.
githubMinimumPermissionMapwas additionally probeddirectly against the component's own Ajv 8.20.0 with its real options before
being adopted: it compiles clean under
strict: true, accepts{"actions":"read"}, and rejects{"actions":"none"},{},{"id-token":"read"},{"models":"write"}, and unknown scopes — inheritingthe whole 17-scope table and its per-scope constraints through
$refratherthan duplicating them. The read-only rule sits in the validator, not here, for
the message-quality reason given above.
npm run lint:md— 0 issues, 112 files.lefthook run pre-commit— typos,editorconfig, gitleaks, markdownlint, biome all pass.
test:packages14/14,test:concurrency-policy24/24,test:dependabot-policy35/35,test:pr-convention-policy10/10,test:lefthook-dotnet12/12,lint:hooks"All good",lint:concurrency-policyandlint:dependabot-policypass. (lint:pr-convention-policyfails identicallyon
origin/mainin this environment — its npm script self-checks with a$(cat …)substitution Windowscmddoes not expand. Untouched here.)Proof the new validation bites
The unit tests cover the semantics; this is the end-to-end proof against the
real backfilled
policy.jsonand this repository's own real callers.standards'.github/atorigin/mainwas exported to a scratch root, thethree gate callers repinned to
7107b34, only the caller job'spermissions:varied, and
auditRepositoryrun with auto-approval disabled andfetchstubbed to throw so nothing could pass by network.
Against this branch's
policy.json:Against
origin/main'spolicy.json, those same five scenarios produce(no findings)every time — including both under-granted ones. That is the gapthis PR closes, reproduced rather than asserted.
The check reports the first shortfall in sorted scope order, matching
exactCanonicalMap's existing first-failure style, which is why thepermissions: {}scenario namesactionsand stops rather than also listingthe equally-missing
pull-requests.New tests
an all-read minimum admits a caller granting exactly it— the case that isinexpressible on
main.a caller granting more than the minimum clears it— awritegrant againsta
readfloor, on a contract carrying both terms. This is theload-bearing proof that the ordered comparison survived the read-only
restriction: only what a contract may require narrowed, not how grants are
compared.
a caller granting less than the minimum is rejected— omitted scope,explicit
{}, and an unrelated scope granted instead.a minimum caller permission floor cannot require write access—writeona read/write scope,
id-token: write, and a mixed map with one write value;all rejected at config-validation time. New for the P2.
read-all and write-all callers both clear a read floor, asserting thatwrite-allis nonetheless still caught by the privileged rules.a caller with no explicit permissions cannot prove a minimum.a minimum caller permission scope must name a real read grant—none,unknown scopes,
{}, andmodels: write.a contract naming both caller-permission terms must be satisfiable,re-targeted to a read floor the waiver omits entirely (the previous
writefloor is no longer a legal contract).
One row was added to the existing
Dependabot SHA bump declines ambiguous surface-matching reviewed contractstable so two bases differing only inminimumCallerPermissionsare proven to be caught.Not done here, deliberately
hosted-onlycontracts do not get the term. Not an oversight and notscope-trimming:
reusableWorkflowStatusreturns approved forhosted-onlybefore any permission check runs, so extending the floor there means extending
that path — a behavior change to a routing mode with no consumer in this
issue. It belongs in its own change with its own review.
ci-runner's callers pass norunnerinput, so they would fail therunner-inputcontract for an unrelated, pre-existing reason ifci-runnerever adopts the component. Already flagged in feat(runner-policy): register v0.14.2 contracts for five ci-workflows reusables #382; unchanged by this PR.
Proposal, not implemented: should the term be required?
Raised rather than built, per the brief. Recommendation: yes eventually, as a
registration-time check with a migration first — not now.
The honest shape of the rule, after the read-only restriction, is "a
runner-inputcontract must record the read scopes of its callee'spermissions:block as its floor" — the callee's write scopes stay the waiver'sbusiness, as
link-check'sissues: writeandzizmor'ssecurity-events: writealready are. Requiring it today would invalidate everyexisting entry at once, including several this PR does not touch, and the check
cannot derive the callee's block itself without a network fetch at
policy-load time, which the module deliberately does not do outside the
auto-approval path. The workable sequence is the one #382 proposed for untagged
SHAs: backfill the floor onto the remaining entries first, then add the
requirement as a registration-time check on
approvedReusableWorkflowContracts, so the failure lands on whoever adds anentry rather than on every downstream consumer simultaneously. Worth its own
issue once the backfill is complete.
Closes #383
Related
7107b34entries and recorded this gap in its own bodyas a known limitation; this PR closes it
zizmor.yml@7107b34'ssecurity-events: writethroughallowedCallerPermissions; this branch is rebased on it, and it is the workedexample of the write half of the ceiling/floor split
callers to
7107b34with bothreadgrants while this PR was open; theblast-radius table reflects the post-merge state
failure mode: rejected at startup, with the required check emitting no context
at all
timed_outprerequisite resolver behindthe
actions: readadditions🤖 Generated with Claude Code
https://claude.ai/code/session_013yvHrEronHPoznT1b3HtN5