fix(748): declare permissions: on all six workflows, and prove the declaration binds (#860)
Build ErsatzTV Image / CI toolchain image resolves (push) Successful in 7s
Build ErsatzTV Image / Delimiter ban (release path) (push) Successful in 18s
Build ErsatzTV Image / Build & test (.NET) (push) Successful in 8m40s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (push) Successful in 6m11s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (push) Successful in 6m4s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (push) Skipped
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (push) Skipped
Build ErsatzTV Image / Build & push image (amd64) (push) Successful in 4m20s
Build ErsatzTV Image / CI toolchain image resolves (push) Successful in 7s
Build ErsatzTV Image / Delimiter ban (release path) (push) Successful in 18s
Build ErsatzTV Image / Build & test (.NET) (push) Successful in 8m40s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (push) Successful in 6m11s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (push) Successful in 6m4s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (push) Skipped
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (push) Skipped
Build ErsatzTV Image / Build & push image (amd64) (push) Successful in 4m20s
Co-authored-by: Timothy <timothy@noreply.gitea.tblindustries.be>
This commit was merged in pull request #860.
This commit is contained in:
@@ -3567,3 +3567,118 @@ def test_an_UNREADABLE_history_page_2_also_repairs_rather_than_leaving_green(tmp
|
||||
assert seq[0]["state"] == "success" and seq[1]["state"] == "pending", (
|
||||
f"expected an exemption then a repair to pending; got {seq}"
|
||||
)
|
||||
|
||||
|
||||
# --- Workflow token scope (ersatztv#748) -------------------------------------------------------
|
||||
#
|
||||
# Both assertions guard a property whose violation is SILENT and, for the first, unrecoverable.
|
||||
# `permissions:` is exhaustive: dropping a unit does not degrade, it 403s the `curl -sf` under
|
||||
# `set -euo pipefail`, so `review-verdict/h10` is never posted and EVERY merge to `main` blocks —
|
||||
# including the PR that would repair it, with no force-merge escape since ersatztv#743. And because
|
||||
# `review-verdict.yml` is base-resolved, the PR making that edit CANNOT catch it: its own run uses
|
||||
# the definition already on `main`. A test on the tree is the only place this is checkable at review
|
||||
# time. Measured basis for the unit list: docs/ci-cd.md -> "Workflow token scope".
|
||||
|
||||
|
||||
def _declared_permissions(path, job=None):
|
||||
import yaml
|
||||
|
||||
doc = yaml.safe_load(path.read_text()) or {}
|
||||
if job is None:
|
||||
return doc.get("permissions")
|
||||
return ((doc.get("jobs") or {}).get(job) or {}).get("permissions")
|
||||
|
||||
|
||||
def test_the_verdict_gate_job_declares_exactly_the_units_its_api_calls_need():
|
||||
"""The gate job's block is load-bearing in BOTH directions, so this pins the whole mapping.
|
||||
|
||||
Not just `code: write`. A missing read unit is quieter but still wrong: without `issues: read`
|
||||
the retarget fence cannot establish a trusted count and the exemption is withheld, and without
|
||||
`pull-requests: read` the enumeration degrades to `complete=no` and every exempt PR falls to a
|
||||
generic `pending`. Those are recoverable, unlike a missing `code: write`, but they silently turn
|
||||
the two exemption classes off — the ersatztv#751 failure shape, arrived at from a different
|
||||
direction.
|
||||
|
||||
Equality, not containment: an EXTRA unit is a finding too. This job holds the only repo-write in
|
||||
the repo's CI, so widening it (`contents: write`, a stray `packages:`) is exactly the drift the
|
||||
scoping in ersatztv#697/#748 exists to prevent, and it would land unremarked as "just adding a
|
||||
permission".
|
||||
|
||||
WHAT THIS DOES NOT CATCH, stated because the test's name over-promises: it TRANSCRIBES the unit
|
||||
set, it does not DERIVE it from the job's API calls. Add a new `gh` call to the workflow that
|
||||
needs an undeclared unit and this test stays green while the gate breaks in production — the
|
||||
failure this test exists for, arriving from the other side. Deriving the mapping would mean
|
||||
parsing every URL out of a 1300-line shell body and knowing Gitea's endpoint-to-unit table, which
|
||||
is a different and much larger change. So: when you add an API call to this job, the unit list
|
||||
here is a checklist to revisit by hand, not a net that will tell you. (Same honesty as
|
||||
`test_the_verdict_workflow_has_NO_expression_delimiter_in_any_run_body` above.)
|
||||
"""
|
||||
wf = REPO_ROOT / ".gitea" / "workflows" / "review-verdict.yml"
|
||||
assert _declared_permissions(wf) is None, (
|
||||
"review-verdict.yml grew a TOP-LEVEL permissions: block. Only the job-level one on "
|
||||
"set-verdict-status was probed (docs/ci-cd.md -> 'Workflow token scope'); adding a "
|
||||
"top-level default changes what other jobs in this file would inherit and is unmeasured."
|
||||
)
|
||||
assert _declared_permissions(wf, "set-verdict-status") == {
|
||||
"code": "write", # POST /statuses/{sha} — Gitea has no `statuses` scope, so this IS it
|
||||
"issues": "read", # GET /issues/{n}/timeline — the ersatztv#706 retarget fence
|
||||
"pull-requests": "read", # scripts/pr-changed-files.sh -> /pulls/{n}, /pulls/{n}/files
|
||||
}, (
|
||||
"the review-verdict gate job's permissions: block no longer matches the unit set that was "
|
||||
"measured against its API calls. Dropping `code: write` makes review-verdict/h10 UNWRITABLE "
|
||||
"and blocks every merge with no force-merge escape (ersatztv#743), and this workflow is "
|
||||
"base-resolved so the PR changing it cannot detect that. Re-run the scratch-base probe "
|
||||
"(docs/ci-cd.md -> Review-verdict gate) before changing this."
|
||||
)
|
||||
|
||||
|
||||
def test_every_tracked_workflow_declares_a_permissions_block():
|
||||
"""Otherwise the convention is prose only, and a NEW workflow is the case that breaks it.
|
||||
|
||||
A workflow added without `permissions:` inherits the owner-level Actions default, which is
|
||||
`permissive` today — i.e. a full read/write repository token, which is status-capable and can
|
||||
therefore forge `review-verdict/h10` (`ci.actions-credential-scoping`). That is silent: nothing
|
||||
reddens, the new file simply holds more than it needs.
|
||||
|
||||
Population comes from the GIT INDEX via `_workflow_files()`, not a directory listing — same
|
||||
reason as the two completeness claims above (ersatztv#806/#778).
|
||||
|
||||
NO EXEMPTIONS, deliberately. `ci-image.yml` briefly needed one: editing that file re-pointed
|
||||
`ci-image-pin`'s `expected` at the editing commit and reddened a BLOCKING job, and its own
|
||||
`paths:` made the edit publish an image. ersatztv#744 took that path out of both
|
||||
(`ci.toolchain-image-publish-is-a-dispatch`), so this asserts over the whole derived population
|
||||
with nothing carved out — which is the form ersatztv#835 asked for. If a future file seems to
|
||||
need an exemption, that is a finding about the file, not about this test.
|
||||
"""
|
||||
import yaml
|
||||
|
||||
workflows = _workflow_files()
|
||||
# Anti-vacuity. `tracked_children` matches direct children of one directory, so a renamed or
|
||||
# moved `.gitea/workflows/` yields an EMPTY population and every completeness claim below passes
|
||||
# having examined nothing — the #778 shape this test's own docstring invokes. `_git_ls_files`'s
|
||||
# assert covers the whole index, not this subset, so it does not catch it. Measured: after
|
||||
# `git mv .gitea/workflows .gitea/wf-renamed` this test passed with 0 files before this guard.
|
||||
assert workflows, (
|
||||
"no tracked workflows found under .gitea/workflows — this test examined NOTHING and would "
|
||||
"have passed vacuously. The directory was renamed/moved, or the glob no longer matches."
|
||||
)
|
||||
|
||||
missing = []
|
||||
for wf in workflows:
|
||||
doc = yaml.safe_load(wf.read_text()) or {}
|
||||
jobs = (doc.get("jobs") or {}).values()
|
||||
# Top-level absent is fine only if EVERY job declares its own — one undeclared job still
|
||||
# inherits the owner default, so `any` would pass a file that is half-covered.
|
||||
declares = doc.get("permissions") is not None or (
|
||||
bool(jobs) and all((job or {}).get("permissions") is not None for job in jobs)
|
||||
)
|
||||
|
||||
if not declares:
|
||||
missing.append(wf.name)
|
||||
assert not missing, (
|
||||
f"these workflows declare no permissions: block at either level: {missing}. Every workflow "
|
||||
"in this repo declares one (ersatztv#748), with NO exemption since #744 landed, so a new "
|
||||
"file cannot silently inherit the "
|
||||
"owner-level default. Read-only (`permissions: {code: read}` at top level) is the "
|
||||
"convention; declare write only with a stated reason at the declaration."
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user