From 81767af084c6300a8f7920b3561814329a298edd Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 19:58:11 +0200 Subject: [PATCH 01/12] fix(748): declare explicit token permissions on all six workflows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit So the owner-level Actions default can move to Restricted (server-management#714) without this repo's merge gate losing the ability to write `review-verdict/h10`. review-verdict.yml (job-level) code: write, issues: read, pull-requests: read the other five (top-level) code: read `code: write` because Gitea has no `statuses` scope — status-write IS Code-write (`reqRepoWriter(unit.TypeCode)`), per `ci.actions-credential-scoping`. The two read units are NOT spare: the gate job also GETs `/issues/{n}/timeline` (the #706 retarget fence) and, via `scripts/pr-changed-files.sh`, `/pulls/{n}` and `/pulls/{n}/files` (the #698 exemption path). Enumerated against every authenticated call the job makes; the set covers it exactly, with nothing over. Corrects #748's body: `docker-build.yml` does NOT write a status with the injected token. It has no `GITEA_TOKEN` reference at all — its `ETV_STATUS_AUTH` is the scoped REGISTRY_* PAT doing a read-only GET. Only review-verdict.yml needs write. No `packages:` unit is needed anywhere: all five `container:` blocks carry explicit `credentials:` using the REGISTRY_* PAT, so no job pulls the toolchain image with the injected token. refs #748 --- .gitea/workflows/ci-image.yml | 13 +++++++++++++ .gitea/workflows/dependency-scan.yml | 10 ++++++++++ .gitea/workflows/docker-build.yml | 13 +++++++++++++ .gitea/workflows/pr-checks.yml | 10 ++++++++++ .gitea/workflows/renovate.yml | 12 ++++++++++++ .gitea/workflows/review-verdict.yml | 19 +++++++++++++++++++ 6 files changed, 77 insertions(+) diff --git a/.gitea/workflows/ci-image.yml b/.gitea/workflows/ci-image.yml index 6c3be2e09..afda85a85 100644 --- a/.gitea/workflows/ci-image.yml +++ b/.gitea/workflows/ci-image.yml @@ -50,6 +50,19 @@ env: REGISTRY: 192.168.1.95:3000 CI_IMAGE: 192.168.1.95:3000/timothy/ersatztv-ci +# Explicit token scope (ersatztv#748) so the owner-level Actions default can move to Restricted +# (server-management#714). Declaring `permissions:` is EXHAUSTIVE, not additive: a unit omitted here +# is NOT granted, and that holds at any owner default — it is not conditional on Restricted being on. +# Only `review-verdict.yml` needs write; it declares that at the job and says why there. Full +# rationale and the per-workflow credential audit: docs/ci-cd.md -> "Workflow token scope". +# This workflow's registry pushes authenticate with the scoped REGISTRY_* PAT +# (`ci.actions-credential-scoping`), so the injected GITEA_TOKEN serves only its single +# `actions/checkout` — the repo's one checkout that still persists its credential into .git/config +# (ersatztv#746 excluded it deliberately). `code: read` is therefore what makes that persisted +# credential read-only, which is a gain this file gets and the others already had. +permissions: + code: read + jobs: build: name: Build & push CI image diff --git a/.gitea/workflows/dependency-scan.yml b/.gitea/workflows/dependency-scan.yml index 0245c981d..80707bb4c 100644 --- a/.gitea/workflows/dependency-scan.yml +++ b/.gitea/workflows/dependency-scan.yml @@ -33,6 +33,16 @@ env: DOTNET_CLI_USE_MSBUILD_SERVER: "0" MSBUILDDISABLENODEREUSE: "1" +# Explicit token scope (ersatztv#748) so the owner-level Actions default can move to Restricted +# (server-management#714). Declaring `permissions:` is EXHAUSTIVE, not additive: a unit omitted here +# is NOT granted, and that holds at any owner default — it is not conditional on Restricted being on. +# Only `review-verdict.yml` needs write; it declares that at the job and says why there. Full +# rationale and the per-workflow credential audit: docs/ci-cd.md -> "Workflow token scope". +# Holds no registry credential and reads nothing from the Gitea API; the injected GITEA_TOKEN serves +# only its one `actions/checkout`. +permissions: + code: read + jobs: scan: name: NuGet vulnerable packages diff --git a/.gitea/workflows/docker-build.yml b/.gitea/workflows/docker-build.yml index b34f61097..e10c19e62 100644 --- a/.gitea/workflows/docker-build.yml +++ b/.gitea/workflows/docker-build.yml @@ -113,6 +113,19 @@ env: DOTNET_CLI_USE_MSBUILD_SERVER: "0" # no persistent MSBuild server process MSBUILDDISABLENODEREUSE: "1" # MSBuild worker nodes exit with the build instead of lingering +# Explicit token scope (ersatztv#748) so the owner-level Actions default can move to Restricted +# (server-management#714). Declaring `permissions:` is EXHAUSTIVE, not additive: a unit omitted here +# is NOT granted, and that holds at any owner default — it is not conditional on Restricted being on. +# Only `review-verdict.yml` needs write; it declares that at the job and says why there. Full +# rationale and the per-workflow credential audit: docs/ci-cd.md -> "Workflow token scope". +# Its registry pushes, its five `container:` image pulls and its one commit-status GET +# (`ETV_STATUS_AUTH`, a read-only GET via scripts/ci-detect-already-validated.sh) ALL authenticate +# with the scoped REGISTRY_* PAT, never the injected token — which therefore serves only its eight +# `actions/checkout` steps. Note this file needs no `packages:` unit for that reason: the +# `container:` blocks carry explicit `credentials:`. +permissions: + code: read + jobs: # Answers "is the toolchain image still there?" in ONE place, so a deleted pin does not read as # five broken jobs and a broken diff (ersatztv#772). Deliberately container-free and deliberately diff --git a/.gitea/workflows/pr-checks.yml b/.gitea/workflows/pr-checks.yml index 8f39aef05..e93dc90f0 100644 --- a/.gitea/workflows/pr-checks.yml +++ b/.gitea/workflows/pr-checks.yml @@ -42,6 +42,16 @@ concurrency: group: ersatztv-pr-gates-${{ github.ref }} cancel-in-progress: true +# Explicit token scope (ersatztv#748) so the owner-level Actions default can move to Restricted +# (server-management#714). Declaring `permissions:` is EXHAUSTIVE, not additive: a unit omitted here +# is NOT granted, and that holds at any owner default — it is not conditional on Restricted being on. +# Only `review-verdict.yml` needs write; it declares that at the job and says why there. Full +# rationale and the per-workflow credential audit: docs/ci-cd.md -> "Workflow token scope". +# Holds no secrets at all and reads nothing from the Gitea API; the injected GITEA_TOKEN serves only +# its five `actions/checkout` steps. +permissions: + code: read + jobs: # BLOCKING (ersatztv#390): the CI toolchain image pin in docker-build.yml must name the image that # ci-image.yml actually last published — i.e. the short sha of the last commit to touch the image's diff --git a/.gitea/workflows/renovate.yml b/.gitea/workflows/renovate.yml index 114eb262e..e1b21623b 100644 --- a/.gitea/workflows/renovate.yml +++ b/.gitea/workflows/renovate.yml @@ -45,6 +45,18 @@ concurrency: group: ersatztv-renovate cancel-in-progress: false +# Explicit token scope (ersatztv#748) so the owner-level Actions default can move to Restricted +# (server-management#714). Declaring `permissions:` is EXHAUSTIVE, not additive: a unit omitted here +# is NOT granted, and that holds at any owner default — it is not conditional on Restricted being on. +# Only `review-verdict.yml` needs write; it declares that at the job and says why there. Full +# rationale and the per-workflow credential audit: docs/ci-cd.md -> "Workflow token scope". +# This workflow has no checkout step and never uses the injected GITEA_TOKEN for anything. Renovate's +# own branch/PR writes go through RENOVATE_TOKEN, a dedicated bot PAT the Actions default does not +# govern, and its container image comes from Docker Hub. Read-only is declared to STATE that the +# injected token is unused, not because any step needs it. +permissions: + code: read + jobs: renovate: name: Renovate diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index 024ffb82b..00fa3f2d8 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -118,6 +118,25 @@ jobs: set-verdict-status: name: Set review-verdict status runs-on: small # a few API calls; keep it off the build runners + # Explicit token scope (ersatztv#748), so the owner-level Actions default can be set to + # Restricted (server-management#714) without this job losing the ability to write the gate. + # + # DECLARING THIS IS EXHAUSTIVE — it is not additive. From the moment it merges, this job holds + # EXACTLY these units even while the owner default is still `permissive`. A missing unit does not + # degrade gracefully: the `curl -sf` dies under `set -euo pipefail` and `review-verdict/h10` is + # never posted, which blocks EVERY merge to `main` — including the PR that would repair it, and + # `block_admin_merge_override: true` (ersatztv#743) means there is no force-merge escape. Change + # this block only with the scratch-base-branch probe in `docs/ci-cd.md`; this workflow is never + # exercised by its own PR. + # + # Why `code: write` and not something narrower: Gitea has NO `statuses` scope (GitHub-only, and + # explicitly unsupported). `POST /repos/{o}/{r}/statuses/{sha}` is gated by + # `reqRepoWriter(unit.TypeCode)`, so status-write IS Code-write — see `ci.actions-credential-scoping`. + # `contents: write` would also work but additionally grants `releases: write`, which is not used. + permissions: + code: write # POST /statuses/{sha} (the gate) + the /commits/{sha}/status reads + issues: read # GET /issues/{n}/timeline — the retarget-count fence (#706) + pull-requests: read # scripts/pr-changed-files.sh -> GET /pulls/{n}/files (#698 exemption path) steps: # BASE, not head — see the header. `fetch-depth: 1` is enough: nothing here reads history, # only the working tree's `scripts/`. -- 2.47.3 From ca342db70b98e56bd34c97aea489bf5ad324b592 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 20:06:29 +0200 Subject: [PATCH 02/12] docs(748): record that `permissions:` binds, measured by matched probe Adds `docs/ci-cd.md` -> "Workflow token scope" (the section the inline comments on all six workflows point at) and corrects `ci.actions-credential-scoping`, which had carried "whether `permissions:` is honored here is still UNPROBED" and an explicit "do NOT add a `permissions:` key" since 2026-08-05. Settled by a matched scratch-base probe pair on Gitea 1.27.1, differing only in the gate job's single permission unit: code: write -> job success, `Posted review-verdict/h10-PROBE=success`, status creator: null code: read -> job failure at the POST, curl exit 22 (HTTP >=400 under -f), NO status written Both reached the classify step's start marker and both logged the retarget fence as trusted, so the read units were exercised and neither run was the #751 dropped-step shape. Both ran while the owner default was still `permissive`, which is what establishes the declaration binds irrespective of that default. The negative control is the load-bearing half: a green positive alone is equally consistent with `permissions:` being a no-op sitting beside a permissive default. Also corrects the two places that framed the persisted checkout credential's write-capability as decided by the instance default; since this change it is decided by `ci-image.yml`'s own `code: read` declaration. Decisions-Edit: yes Refs: #748 --- docs/ci-cd.md | 69 ++++++++++++++++++- docs/decisions/README.md | 2 +- .../records/ci/actions-credential-scoping.md | 37 +++++++--- 3 files changed, 93 insertions(+), 15 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 28a4d3621..59826f19f 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -147,14 +147,77 @@ A backup, fetch, or migration-smoke failure aborts before the live container is **Also avoid firing several pushes back-to-back** (e.g. a `[skip ci]` commit, then `main`, then a tag, all within ~1s). Observed once on this Gitea instance: the later events were silently dropped — no `ActionRun` records created at all, even though the runner was online and the workflow `active`. Pushing again, spaced out, created the runs normally. If a push/tag doesn't produce a run, re-push (or push an empty commit) rather than assuming the runner is broken. +## Workflow token scope (`permissions:`, ersatztv#748) + +Every workflow in this repo declares `permissions:`. Five declare `code: read` at the top level; +`review-verdict.yml` declares `code: write` + `issues: read` + `pull-requests: read` on its +`set-verdict-status` job and nothing at the top level. That is the whole convention — a new workflow +should declare the narrowest set it needs, and say at the declaration why it needs write if it does. + +**Declaring `permissions:` is exhaustive, not additive.** A unit omitted is a unit not granted, and +that holds *at any owner-level default* — it is not conditional on the instance default being +Restricted. This is the property that makes the block load-bearing rather than decorative, and it is +also what makes a wrong one dangerous: the gate job's API calls are `curl -sf` under +`set -euo pipefail`, so a missing unit is a 403 that kills the step, and `review-verdict/h10` is +never posted. That blocks **every** merge to `main`, including the PR that would repair it, and +since ersatztv#743 (`block_admin_merge_override: true`) there is no force-merge escape. + +`code: write` and not something narrower because **Gitea has no `statuses` scope**. +`POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so status-write *is* +Code-write (`ci.actions-credential-scoping`). `contents: write` would also work but additionally +grants `releases: write`, which nothing here uses. The two read units are not spare: the job GETs +`/issues/{n}/timeline` (the ersatztv#706 retarget fence) and, through +`scripts/pr-changed-files.sh`, `/pulls/{n}` and `/pulls/{n}/files` (the ersatztv#698 exemption path). + +No workflow needs a `packages:` unit. All five `container:` blocks in `docker-build.yml` carry +explicit `credentials:` using the scoped `REGISTRY_*` PAT, so no job pulls the toolchain image with +the injected token. + +### The binding was measured, not assumed (2026-08-27, Gitea 1.27.1) + +`ci.actions-credential-scoping` recorded for three weeks that whether `permissions:` is honored on +this instance was **unprobed** — 1.26.0 was the version it was said to start binding at, from +release notes rather than from an experiment. It was settled with the scratch-base probe below, run +as a matched pair that differed **only** in one unit: + +| Control | Gate job unit | Result | +|---|---|---| +| Positive | `code: write` | job `success`; `Posted review-verdict/h10-PROBE=success`; `creator: null` | +| Negative | `code: read` | job `failure`; `curl` exit 22 (HTTP ≥400 under `-f`) at the POST; **no status written** | + +Both runs logged `Retarget fence: 0 retarget event(s) … (trusted=yes)` and both reached the classify +step's start marker, so the reads succeeded in both and neither failure was the ersatztv#751 +dropped-step shape. The negative control is the load-bearing half: it is what shows the declaration +*binds* rather than being a no-op that happens to sit next to a permissive default. Both ran while +the owner default was still `permissive`, which is what establishes that the block is not +conditional on Restricted. + +`creator: null` on the positive control matters separately: it is the provenance signal +`review-verdict.yml` relies on to tell a machine-written status from a reviewer's +(`ci.exemption-provenance`, ersatztv#742). Approach A — scoping the built-in token — preserves it. +Moving status writes onto a PAT (approach B) would not, which is why B stays blocked. + +**What this does and does not close.** Scoping bounds what a job may *do*; it never bounds whether +attacker YAML *runs* — that needs a trigger filter (ersatztv#744). And `review-verdict.yml` still +holds repo-write. The property gained is not "workflows can no longer assert the verdict"; it is +that *only a workflow visibly opting into repo-write can*, instead of every job on the instance +holding write implicitly. Real, and weaker than closed. + +⚠️ **Changing any of these blocks requires the scratch-base probe**, not CI-green on the editing PR +— `review-verdict.yml` is base-resolved and is never exercised by its own PR. See the probe recipe +in the review-verdict gate section below. + ## Checkouts drop the persisted credential (ersatztv#746) Every `actions/checkout` in this repo **except `ci-image.yml`'s** (see the carve-out below) sets `persist-credentials: false`. Without it the action leaves an `Authorization` header in `.git/config`, and a head-resolved job then hands that credential to every script it runs, not just -to the checkout. That header is write-capable: this instance's Actions default token permission is -`permissive`, where it stands after a Restricted trial was reverted on 2026-08-05, and flipping it -back is ersatztv#748. No job that checks out +to the checkout. That header is write-capable *only while the job's token is*: this instance's Actions +default token permission is `permissive`, where it stands after a Restricted trial was reverted on +2026-08-05. Since ersatztv#748 that default no longer governs this repo's jobs — every workflow +declares `permissions:` explicitly (see the section above), so `ci-image.yml`'s persisted header is +read-only by declaration rather than by instance setting. Re-applying the owner-level default is +server-management#714. No job that checks out loses anything: none of them push with git, and anonymous access covers the fetches because the repo is public — `private: false`, and `GET /info/refs?service=git-upload-pack` answers unauthenticated, measured 2026-08-26 from the LAN. That measures the repository, not a runner's network path to it, diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 05fd2adee..382af8ff6 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -38,7 +38,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `blazor.rollback-tag` | The commit immediately preceding the Blazor-removal merge is tagged `blazor-final` (not a `v*` tag, so it doesn't trigger a prod release build) as the documented rollback/restore path. | 2026-07-11 | [link](records/blazor/rollback-tag.md) | | `blazor.ui-removed` | The legacy Blazor Server UI (`Pages/`, `Shared/`, `ViewModels/`, `Validators/`, MudBlazor + 8 other packages, Blazor Startup wiring) is fully deleted now that the SPA has parity; the legacy `MapWhen` branch is kept only for controllers/docs/OpenAPI/`LegacyUiRedirects`, and the catch-all fallback 302s any unmatched non-api/artwork/docs/openapi path to `/app`. | 2026-07-11 | [link](records/blazor/ui-removed.md) | | `channel.origin-marker` | A new `Channel.Origin` (`ChannelOrigin` enum — `Unknown`/`UserCreated`/`AutoTuned`) records how a channel row was created and is stamped exactly once at insert (`AutoTuned` in `CreateChannelFromLineupHandler`, `UserCreated` in `CreateChannelHandler`), and is never mutated on a later edit. It is surfaced as a raw `origin` field on `ChannelResponseModel`; the SPA badges only `AutoTuned`. Rows predating the column read `Unknown` — provenance is **not** back-filled. | 2026-07-23 | [link](records/channel/origin-marker.md) | -| `ci.actions-credential-scoping` | Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. Do NOT add a `permissions:` key to constrain the injected `GITEA_TOKEN` on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP, which is worse than absent because it reads in review as a constraint. That version precondition NO LONGER HOLDS: this instance was upgraded 1.25.4 -> 1.27.1 on 2026-08-05. What has NOT changed is that the consequence is unverified — whether `permissions:` is honored here is still UNPROBED, and there is still no API surface for it (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists a write-capable credential into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo's 16 checkouts now set it (#746), `ci-image.yml`'s being excluded for the reason in that section. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation. | 2026-08-05 | [link](records/ci/actions-credential-scoping.md) | +| `ci.actions-credential-scoping` | Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. `permissions:` on a workflow/job DOES bind on this instance — MEASURED 2026-08-27 on 1.27.1 by matched scratch-base probe PRs differing only in one unit (`code: write` posted the probe status; `code: read` 403ed the POST, curl exit 22, no status written), so every workflow here now declares it (#748). Two properties that make it usable: the declaration is EXHAUSTIVE, not additive — a unit omitted is not granted — and it binds REGARDLESS of the owner-level default, so it is not conditional on Restricted being enabled. The earlier form of this rule said the opposite ("do NOT add a `permissions:` key on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP"); that was correct at 1.25.4 and is retained here only so a reader meeting the old advice recognises it as superseded rather than contradicted. There is still no API surface for the owner-level default (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists a write-capable credential into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo's 16 checkouts now set it (#746), `ci-image.yml`'s being excluded for the reason in that section. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation. | 2026-08-05 | [link](records/ci/actions-credential-scoping.md) | | `ci.batch-pushes-no-cancel-route` | Hold review fixes, doc corrections and format fixes locally and push **once** — a superseded run cannot be cancelled from the agent side and holds a runner slot until it finishes. | 2026-07-21 | [link](records/ci/batch-pushes-no-cancel-route.md) | | `ci.build-once-rejected` | CI build-once (a shared compile artifact across jobs) was implemented, measured, and rejected for a 40-85% wall-clock regression; keep the #420 cross-run tree-identity skip instead. | 2026-07-18 | [link](records/ci/build-once-rejected.md) | | `ci.cancelled-is-not-a-verdict` | Treat a `cancelled` conclusion as "no verdict" — never as pass or fail — and report FAILED and CANCELLED counts separately in any CI monitor. THE COMBINED COMMIT-STATUS ENDPOINT CANNOT EXPRESS THIS: `GET /repos/{o}/{r}/commits/{sha}/status` has states `success`/`failure`/`pending`/`error` and NO `cancelled`, so it reports a cancelled job as `failure`. Anything polling that endpoint — which is what a CI monitor naturally polls, because it is the per-sha view the merge gate reads — must resolve the job-level `conclusion` via `actions/runs/{id}/jobs` before reporting a red. | 2026-07-21 | [link](records/ci/cancelled-is-not-a-verdict.md) | diff --git a/docs/decisions/records/ci/actions-credential-scoping.md b/docs/decisions/records/ci/actions-credential-scoping.md index 542df3f1c..009c73910 100644 --- a/docs/decisions/records/ci/actions-credential-scoping.md +++ b/docs/decisions/records/ci/actions-credential-scoping.md @@ -5,7 +5,7 @@ status: active since: '2026-08-05' supersedes: none superseded-by: none -rule: 'Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. Do NOT add a `permissions:` key to constrain the injected `GITEA_TOKEN` on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP, which is worse than absent because it reads in review as a constraint. That version precondition NO LONGER HOLDS: this instance was upgraded 1.25.4 -> 1.27.1 on 2026-08-05. What has NOT changed is that the consequence is unverified — whether `permissions:` is honored here is still UNPROBED, and there is still no API surface for it (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists a write-capable credential into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo''s 16 checkouts now set it (#746), `ci-image.yml`''s being excluded for the reason in that section. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation.' +rule: 'Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. `permissions:` on a workflow/job DOES bind on this instance — MEASURED 2026-08-27 on 1.27.1 by matched scratch-base probe PRs differing only in one unit (`code: write` posted the probe status; `code: read` 403ed the POST, curl exit 22, no status written), so every workflow here now declares it (#748). Two properties that make it usable: the declaration is EXHAUSTIVE, not additive — a unit omitted is not granted — and it binds REGARDLESS of the owner-level default, so it is not conditional on Restricted being enabled. The earlier form of this rule said the opposite ("do NOT add a `permissions:` key on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP"); that was correct at 1.25.4 and is retained here only so a reader meeting the old advice recognises it as superseded rather than contradicted. There is still no API surface for the owner-level default (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists a write-capable credential into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo''s 16 checkouts now set it (#746), `ci-image.yml`''s being excluded for the reason in that section. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation.' signals: 'admin password in CI secrets, registry credential scope, ETV_STATUS_AUTH can write statuses, forge review-verdict/h10, head-resolved workflow holds credentials, persist-credentials on actions/checkout, credential left in .git/config, masked git fetch yields empty changed set, Gitea token scopes, no status scope, write:package vs write:repository, permissions key no-op, GITEA_TOKEN default read/write, Restricted default token permissions, orphan secret, deploy key in secret store, toolchain image overwrite, prod floating tag write · paths: `.gitea/workflows/docker-build.yml`, `.gitea/workflows/pr-checks.yml`, `.gitea/workflows/dependency-scan.yml`, `.gitea/workflows/ci-image.yml`, `.gitea/workflows/renovate.yml`, `scripts/ci-detect-already-validated.sh` · issues: #697, #672, #698, #742, #743, #746, #748, #835, #420, server-management#714' mechanics: 'PAT `ci-registry-scoped-697`, scopes `write:package,read:repository`, stored as repo Actions secret `REGISTRY_PASSWORD`; `REGISTRY_USER` remains `timothy`. Verified 2026-08-05 on Gitea 1.25.4: registry push of a probe tag SUCCEEDED; `GET /commits/{sha}/status` 200; `POST /statuses/{sha}` REFUSED HTTP 403 `token does not have at least one of required scope(s), required=[write:repository], token scope=write:package,read:repository`. Probe artifacts deleted, confirmed 404. NOT measured with this token: the `container:` pull, the buildcache write and the base-image pull. Those rest on Gitea''s scope model (write implies read per category, read at tag `v1.25.4`) — INFERRED. Note WHICH run proves which: only the `container:` pull is exercised by a PR. `cache-to`/`cache-from` and the base-image pull are confined to the `build` job, which carries `if: github.event_name != ''pull_request''`, so they are first exercised on the post-merge push to `main` — AFTER the merge gate has passed. A wrong inference there reddens main, not the PR.' --- @@ -72,9 +72,11 @@ reach the runner's docker daemon. **The persisted checkout credential (#746).** `actions/checkout` writes an `Authorization` header into `.git/config` unless `persist-credentials: false` is set, so a job hands that credential to -every script it runs, not only to the checkout — and it is write-capable while this instance's -Actions default token permission is `permissive` — where it stands after a Restricted trial was -reverted on 2026-08-05 (#748). 15 of the repo's 16 checkouts +every script it runs, not only to the checkout — and it was write-capable while this instance's +Actions default token permission is `permissive` — where that default still stands after a +Restricted trial was reverted on 2026-08-05. Since #748 the default no longer decides it: +`ci-image.yml` declares `code: read`, so the credential it persists is read-only by declaration +rather than by instance setting. 15 of the repo's 16 checkouts now set it: `docker-build.yml` (8), `pr-checks.yml` (5), `dependency-scan.yml` (1) and `review-verdict.yml` (1, pre-existing). No job that checks out pushes with git, so none of them loses a capability; `renovate.yml` does push, but has no checkout step and uses `RENOVATE_TOKEN`. @@ -103,6 +105,17 @@ be written without an exemption list once #744 has landed. docs, NOT verified (2026-08-05): `permissions:` landed in 1.26.0 (Gitea PR #36173); no `app.ini` lever at any version; Gitea rejects GitHub's `statuses`/`checks` scopes. +MEASURED 2026-08-27 on 1.27.1 (#748), superseding the "honored here is UNPROBED" caveat above: a +matched pair of scratch-base probe PRs, identical except for the gate job's single permission unit. +`code: write` -> job success, `Posted review-verdict/h10-PROBE=success`, status `creator: null`. +`code: read` -> job failure at the POST, `curl` exit 22 (HTTP >=400 under `-f`), NO status written. +Both logged `Retarget fence: 0 retarget event(s) ... (trusted=yes)` and both reached the classify +step's start marker, so the `issues: read` and `pull-requests: read` units were exercised and +neither run was the #751 dropped-step shape. Both ran while the owner default was `permissive`, +which is what establishes the declaration is not conditional on Restricted. The negative control is +the load-bearing half — without it a green positive is equally consistent with `permissions:` being +a no-op beside a permissive default. Probe branches and PRs (#851, #852) deleted after reading. + **Version caveat — this record's measurements are pinned to 1.25.4, the instance is now 1.27.1.** The instance was upgraded mid-session on 2026-08-05 (#743). Everything above measured on 1.25.4 is therefore a *dated* claim, not a current one: the scope enum, the `reqRepoWriter` gate and the 403 @@ -114,13 +127,15 @@ re-probing. Re-verification of the 1.25.4-pinned claims across the CI docs is tr `write:repository` bot PAT in the same store and still cannot be scoped down, because Renovate needs repo write; what changed is that a verdict posted with it is no longer *inherited* — `renovate` is not in `H10_REVIEWERS`, so `review-verdict.yml` re-derives it (#742). It remains status-capable, so it can -still overwrite a status; it can no longer impersonate a reviewer. The injected `GITEA_TOKEN` is write-capable in every job; -only Gitea >=1.26 with the Actions default set to **Restricted** binds it (server-management#714). -Both halves of that condition have now been checked and it does not hold, because of the second one: -the version half IS satisfied (1.27.1), while the default was set to Restricted, verified, and -reverted to `permissive` on 2026-08-05 because this repo is not ready for it (#748). So the route is -open, and open for a known reason rather than an unprobed one — waiting on a further Gitea upgrade -would be waiting on the wrong thing. A +still overwrite a status; it can no longer impersonate a reviewer. The injected `GITEA_TOKEN` was write-capable in every job. +That is no longer the mechanism: #748 declared `permissions:` on all six workflows (2026-08-27), and +because the declaration binds irrespective of the owner default, five of them now hold `code: read` +and only `review-verdict.yml`'s gate job holds write. The owner-level Restricted default +(server-management#714) is therefore no longer what gates this route for THIS repo — it remains +worth applying as the instance-wide floor for repos that declare nothing, and it is now safe to +apply here, which was the blocker. What is NOT closed: `review-verdict.yml` still holds repo-write, +so the property is "only a workflow visibly opting into write can assert the verdict", not "no +workflow can". Scoping still bounds what a job may DO, never whether attacker YAML RUNS. A collaborator's own token always can — and after #742 an ALLOW-LISTED collaborator's still is inherited, which is the residual the allow-list explicitly does not touch. `docker-build.yml` publishes `:prod` from a `v*` tag push and a tag may point at ANY commit — a prod image with no PR, review or status (tag protections are empty). -- 2.47.3 From f39137d7ceb135fb55d5f7f4b204f0cb4ff35c2f Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 20:17:14 +0200 Subject: [PATCH 03/12] =?UTF-8?q?docs(748):=20the=20probe=20cannot=20clear?= =?UTF-8?q?=20the=20Restricted=20default=20=E2=80=94=20say=20so?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review correction. Both controls ran while the owner-level Actions default was `permissive`, so the probe shows the declaration binds DOWNWARD (a declared `code: read` really loses write). It says nothing about what a declared `code: write` receives under a RESTRICTED default. That distinction is the whole risk. GitHub's semantics let `permissions:` only narrow, never widen past the default; if Gitea copies that, Restricted caps the gate job at read and `review-verdict/h10` becomes unwritable — with no force-merge escape since #743. The previous wording said the flip "is now safe to apply here", which the evidence does not support. Corrected in both `docs/ci-cd.md` and the record's `rule` + surviving-routes prose: the flip stays gated on re-running the scratch-base probe UNDER Restricted, which is why server-management#714 stays open. Decisions-Edit: yes Refs: #748 --- docs/ci-cd.md | 12 ++++++++++-- docs/decisions/README.md | 2 +- .../records/ci/actions-credential-scoping.md | 17 ++++++++++------- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 59826f19f..9a16859ee 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -189,8 +189,16 @@ Both runs logged `Retarget fence: 0 retarget event(s) … (trusted=yes)` and bot step's start marker, so the reads succeeded in both and neither failure was the ersatztv#751 dropped-step shape. The negative control is the load-bearing half: it is what shows the declaration *binds* rather than being a no-op that happens to sit next to a permissive default. Both ran while -the owner default was still `permissive`, which is what establishes that the block is not -conditional on Restricted. +the owner default was still `permissive`, which establishes that the five read-only declarations +bind **today**, without any flip. + +**What the probe does NOT establish — read this before flipping the owner default.** It says nothing +about what a declared `code: write` receives under a **Restricted** default. GitHub's semantics let +`permissions:` only *narrow*, never widen past the default; if Gitea copies that, Restricted would +cap the gate job at read and `review-verdict/h10` would become unwritable — precisely the failure +this section warns about, with no force-merge escape. Both controls ran under `permissive`, so that +case is untested. server-management#714 stays open for exactly this: re-run the scratch-base probe +**under Restricted** before trusting the flip. `creator: null` on the positive control matters separately: it is the provenance signal `review-verdict.yml` relies on to tell a machine-written status from a reviewer's diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 382af8ff6..66218dcda 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -38,7 +38,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `blazor.rollback-tag` | The commit immediately preceding the Blazor-removal merge is tagged `blazor-final` (not a `v*` tag, so it doesn't trigger a prod release build) as the documented rollback/restore path. | 2026-07-11 | [link](records/blazor/rollback-tag.md) | | `blazor.ui-removed` | The legacy Blazor Server UI (`Pages/`, `Shared/`, `ViewModels/`, `Validators/`, MudBlazor + 8 other packages, Blazor Startup wiring) is fully deleted now that the SPA has parity; the legacy `MapWhen` branch is kept only for controllers/docs/OpenAPI/`LegacyUiRedirects`, and the catch-all fallback 302s any unmatched non-api/artwork/docs/openapi path to `/app`. | 2026-07-11 | [link](records/blazor/ui-removed.md) | | `channel.origin-marker` | A new `Channel.Origin` (`ChannelOrigin` enum — `Unknown`/`UserCreated`/`AutoTuned`) records how a channel row was created and is stamped exactly once at insert (`AutoTuned` in `CreateChannelFromLineupHandler`, `UserCreated` in `CreateChannelHandler`), and is never mutated on a later edit. It is surfaced as a raw `origin` field on `ChannelResponseModel`; the SPA badges only `AutoTuned`. Rows predating the column read `Unknown` — provenance is **not** back-filled. | 2026-07-23 | [link](records/channel/origin-marker.md) | -| `ci.actions-credential-scoping` | Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. `permissions:` on a workflow/job DOES bind on this instance — MEASURED 2026-08-27 on 1.27.1 by matched scratch-base probe PRs differing only in one unit (`code: write` posted the probe status; `code: read` 403ed the POST, curl exit 22, no status written), so every workflow here now declares it (#748). Two properties that make it usable: the declaration is EXHAUSTIVE, not additive — a unit omitted is not granted — and it binds REGARDLESS of the owner-level default, so it is not conditional on Restricted being enabled. The earlier form of this rule said the opposite ("do NOT add a `permissions:` key on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP"); that was correct at 1.25.4 and is retained here only so a reader meeting the old advice recognises it as superseded rather than contradicted. There is still no API surface for the owner-level default (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists a write-capable credential into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo's 16 checkouts now set it (#746), `ci-image.yml`'s being excluded for the reason in that section. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation. | 2026-08-05 | [link](records/ci/actions-credential-scoping.md) | +| `ci.actions-credential-scoping` | Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. `permissions:` on a workflow/job DOES bind on this instance — MEASURED 2026-08-27 on 1.27.1 by matched scratch-base probe PRs differing only in one unit (`code: write` posted the probe status; `code: read` 403ed the POST, curl exit 22, no status written), so every workflow here now declares it (#748). Two properties that make it usable: the declaration is EXHAUSTIVE, not additive — a unit omitted is not granted — and it binds while the owner-level default is `permissive`, which is what makes the five `code: read` declarations effective TODAY rather than only after a flip. NOT established: what a declared `code: write` does under a RESTRICTED default. GitHub semantics let `permissions:` only narrow, never widen past the default, and if Gitea copies that, Restricted would cap the gate job at read and `review-verdict/h10` would stop being writable — the exact catastrophic case. The probe ran under `permissive` and therefore CANNOT rule that out. Flip the owner default only behind the scratch-base probe re-run under Restricted (server-management#714, still open for this reason). The earlier form of this rule said the opposite ("do NOT add a `permissions:` key on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP"); that was correct at 1.25.4 and is retained here only so a reader meeting the old advice recognises it as superseded rather than contradicted. There is still no API surface for the owner-level default (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists a write-capable credential into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo's 16 checkouts now set it (#746), `ci-image.yml`'s being excluded for the reason in that section. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation. | 2026-08-05 | [link](records/ci/actions-credential-scoping.md) | | `ci.batch-pushes-no-cancel-route` | Hold review fixes, doc corrections and format fixes locally and push **once** — a superseded run cannot be cancelled from the agent side and holds a runner slot until it finishes. | 2026-07-21 | [link](records/ci/batch-pushes-no-cancel-route.md) | | `ci.build-once-rejected` | CI build-once (a shared compile artifact across jobs) was implemented, measured, and rejected for a 40-85% wall-clock regression; keep the #420 cross-run tree-identity skip instead. | 2026-07-18 | [link](records/ci/build-once-rejected.md) | | `ci.cancelled-is-not-a-verdict` | Treat a `cancelled` conclusion as "no verdict" — never as pass or fail — and report FAILED and CANCELLED counts separately in any CI monitor. THE COMBINED COMMIT-STATUS ENDPOINT CANNOT EXPRESS THIS: `GET /repos/{o}/{r}/commits/{sha}/status` has states `success`/`failure`/`pending`/`error` and NO `cancelled`, so it reports a cancelled job as `failure`. Anything polling that endpoint — which is what a CI monitor naturally polls, because it is the per-sha view the merge gate reads — must resolve the job-level `conclusion` via `actions/runs/{id}/jobs` before reporting a red. | 2026-07-21 | [link](records/ci/cancelled-is-not-a-verdict.md) | diff --git a/docs/decisions/records/ci/actions-credential-scoping.md b/docs/decisions/records/ci/actions-credential-scoping.md index 009c73910..3c1bebb67 100644 --- a/docs/decisions/records/ci/actions-credential-scoping.md +++ b/docs/decisions/records/ci/actions-credential-scoping.md @@ -5,7 +5,7 @@ status: active since: '2026-08-05' supersedes: none superseded-by: none -rule: 'Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. `permissions:` on a workflow/job DOES bind on this instance — MEASURED 2026-08-27 on 1.27.1 by matched scratch-base probe PRs differing only in one unit (`code: write` posted the probe status; `code: read` 403ed the POST, curl exit 22, no status written), so every workflow here now declares it (#748). Two properties that make it usable: the declaration is EXHAUSTIVE, not additive — a unit omitted is not granted — and it binds REGARDLESS of the owner-level default, so it is not conditional on Restricted being enabled. The earlier form of this rule said the opposite ("do NOT add a `permissions:` key on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP"); that was correct at 1.25.4 and is retained here only so a reader meeting the old advice recognises it as superseded rather than contradicted. There is still no API surface for the owner-level default (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists a write-capable credential into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo''s 16 checkouts now set it (#746), `ci-image.yml`''s being excluded for the reason in that section. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation.' +rule: 'Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. `permissions:` on a workflow/job DOES bind on this instance — MEASURED 2026-08-27 on 1.27.1 by matched scratch-base probe PRs differing only in one unit (`code: write` posted the probe status; `code: read` 403ed the POST, curl exit 22, no status written), so every workflow here now declares it (#748). Two properties that make it usable: the declaration is EXHAUSTIVE, not additive — a unit omitted is not granted — and it binds while the owner-level default is `permissive`, which is what makes the five `code: read` declarations effective TODAY rather than only after a flip. NOT established: what a declared `code: write` does under a RESTRICTED default. GitHub semantics let `permissions:` only narrow, never widen past the default, and if Gitea copies that, Restricted would cap the gate job at read and `review-verdict/h10` would stop being writable — the exact catastrophic case. The probe ran under `permissive` and therefore CANNOT rule that out. Flip the owner default only behind the scratch-base probe re-run under Restricted (server-management#714, still open for this reason). The earlier form of this rule said the opposite ("do NOT add a `permissions:` key on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP"); that was correct at 1.25.4 and is retained here only so a reader meeting the old advice recognises it as superseded rather than contradicted. There is still no API surface for the owner-level default (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists a write-capable credential into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo''s 16 checkouts now set it (#746), `ci-image.yml`''s being excluded for the reason in that section. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation.' signals: 'admin password in CI secrets, registry credential scope, ETV_STATUS_AUTH can write statuses, forge review-verdict/h10, head-resolved workflow holds credentials, persist-credentials on actions/checkout, credential left in .git/config, masked git fetch yields empty changed set, Gitea token scopes, no status scope, write:package vs write:repository, permissions key no-op, GITEA_TOKEN default read/write, Restricted default token permissions, orphan secret, deploy key in secret store, toolchain image overwrite, prod floating tag write · paths: `.gitea/workflows/docker-build.yml`, `.gitea/workflows/pr-checks.yml`, `.gitea/workflows/dependency-scan.yml`, `.gitea/workflows/ci-image.yml`, `.gitea/workflows/renovate.yml`, `scripts/ci-detect-already-validated.sh` · issues: #697, #672, #698, #742, #743, #746, #748, #835, #420, server-management#714' mechanics: 'PAT `ci-registry-scoped-697`, scopes `write:package,read:repository`, stored as repo Actions secret `REGISTRY_PASSWORD`; `REGISTRY_USER` remains `timothy`. Verified 2026-08-05 on Gitea 1.25.4: registry push of a probe tag SUCCEEDED; `GET /commits/{sha}/status` 200; `POST /statuses/{sha}` REFUSED HTTP 403 `token does not have at least one of required scope(s), required=[write:repository], token scope=write:package,read:repository`. Probe artifacts deleted, confirmed 404. NOT measured with this token: the `container:` pull, the buildcache write and the base-image pull. Those rest on Gitea''s scope model (write implies read per category, read at tag `v1.25.4`) — INFERRED. Note WHICH run proves which: only the `container:` pull is exercised by a PR. `cache-to`/`cache-from` and the base-image pull are confined to the `build` job, which carries `if: github.event_name != ''pull_request''`, so they are first exercised on the post-merge push to `main` — AFTER the merge gate has passed. A wrong inference there reddens main, not the PR.' --- @@ -112,7 +112,8 @@ matched pair of scratch-base probe PRs, identical except for the gate job's sing Both logged `Retarget fence: 0 retarget event(s) ... (trusted=yes)` and both reached the classify step's start marker, so the `issues: read` and `pull-requests: read` units were exercised and neither run was the #751 dropped-step shape. Both ran while the owner default was `permissive`, -which is what establishes the declaration is not conditional on Restricted. The negative control is +which is what establishes the five read-only declarations bind today. It does NOT establish what a +declared `code: write` gets under a Restricted default — see the caveat in `rule`. The negative control is the load-bearing half — without it a green positive is equally consistent with `permissions:` being a no-op beside a permissive default. Probe branches and PRs (#851, #852) deleted after reading. @@ -129,11 +130,13 @@ repo write; what changed is that a verdict posted with it is no longer *inherite in `H10_REVIEWERS`, so `review-verdict.yml` re-derives it (#742). It remains status-capable, so it can still overwrite a status; it can no longer impersonate a reviewer. The injected `GITEA_TOKEN` was write-capable in every job. That is no longer the mechanism: #748 declared `permissions:` on all six workflows (2026-08-27), and -because the declaration binds irrespective of the owner default, five of them now hold `code: read` -and only `review-verdict.yml`'s gate job holds write. The owner-level Restricted default -(server-management#714) is therefore no longer what gates this route for THIS repo — it remains -worth applying as the instance-wide floor for repos that declare nothing, and it is now safe to -apply here, which was the blocker. What is NOT closed: `review-verdict.yml` still holds repo-write, +and because the declaration binds under the current `permissive` default, five of them now hold +`code: read` and only `review-verdict.yml`'s gate job holds write. So the route is narrowed NOW, +without the owner-level flip. The owner-level Restricted default (server-management#714) remains +worth applying as the instance-wide floor for repos that declare nothing — but it is NOT yet shown +safe here: whether a declared `code: write` survives a Restricted default is unmeasured, and if +Gitea caps at the default the way GitHub does, the flip would un-writable the gate. That is the +remaining probe, not a formality. What is NOT closed: `review-verdict.yml` still holds repo-write, so the property is "only a workflow visibly opting into write can assert the verdict", not "no workflow can". Scoping still bounds what a job may DO, never whether attacker YAML RUNS. A collaborator's own token always can — and after #742 an ALLOW-LISTED collaborator's still is -- 2.47.3 From 84f001eec4da42e2be61b7e941c0109a81651631 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 20:28:00 +0200 Subject: [PATCH 04/12] fix(748): docker-build has three commit-status GET steps, not one Self-review of the comment I wrote: `ETV_STATUS_AUTH` is set on three steps (:186, :456, :617), each invoking scripts/ci-detect-already-validated.sh, not one. The claim about WHICH credential they use was right; the count was not. Refs: #748 --- .gitea/workflows/docker-build.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitea/workflows/docker-build.yml b/.gitea/workflows/docker-build.yml index e10c19e62..f5bf722eb 100644 --- a/.gitea/workflows/docker-build.yml +++ b/.gitea/workflows/docker-build.yml @@ -118,7 +118,7 @@ env: # is NOT granted, and that holds at any owner default — it is not conditional on Restricted being on. # Only `review-verdict.yml` needs write; it declares that at the job and says why there. Full # rationale and the per-workflow credential audit: docs/ci-cd.md -> "Workflow token scope". -# Its registry pushes, its five `container:` image pulls and its one commit-status GET +# Its registry pushes, its five `container:` image pulls and its three commit-status GET steps # (`ETV_STATUS_AUTH`, a read-only GET via scripts/ci-detect-already-validated.sh) ALL authenticate # with the scoped REGISTRY_* PAT, never the injected token — which therefore serves only its eight # `actions/checkout` steps. Note this file needs no `packages:` unit for that reason: the -- 2.47.3 From f4c44bfd6abc011a4977c9174ef961e42d984f31 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 20:36:45 +0200 Subject: [PATCH 05/12] =?UTF-8?q?fix(748):=20address=20the=20cold=20review?= =?UTF-8?q?=20=E2=80=94=20stale=20claim,=20probe=20scope,=20and=20a=20guar?= =?UTF-8?q?d?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2, all five findings plus the guard gap. HIGH — `docs/ci-cd.md` (review-verdict gate section) still asserted "Gitea injects a write-capable GITEA_TOKEN into every job" and that only a Restricted flip binds it. Both are false for this repo after this change, and that copy sits in the section someone reads WHILE editing the gate — i.e. the copy most likely to be believed, arguing that the five new `code: read` blocks are decorative. Rewritten to point at "Workflow token scope". MEDIUM — "the five read-only declarations bind today" was inferred from a JOB-LEVEL probe while the five use TOP-LEVEL. Both docs now state the level the probe carried (job-level, on `set-verdict-status`, the shipped shape), and mark the top-level form as inferred — noting it fails safe and that docker-build/pr-checks are `on: pull_request`, so this PR's own CI exercises it. MEDIUM — the new 1.27.1 measurement had been inserted ABOVE the paragraph saying "everything above is pinned to 1.25.4", which swept it into a caveat it contradicts. Moved below, and labelled as the re-probe that caveat asks for. MEDIUM — the record claimed the fence and the start marker showed both read units were exercised. The fence shows `issues: read`; what shows `pull-requests: read` is the `=success`, which is reachable only via `complete=yes`. Warrants now stated per unit. MEDIUM — `docker-build.yml`'s comment said "one commit-status GET" (three, at :186/:456/:617) and omitted `ETV_REGISTRY_AUTH` at :144 entirely. Enumeration corrected; the conclusion it supports was already right. LOW — `ci-image.yml`'s comment said read-only-persisted was "a gain this file gets and the others already had". The others persist NO credential; the two are not interchangeable and this file has the weaker property. GUARD — nothing pinned `code: write` on the gate job, and by that workflow's own base-resolved property the editing PR can never catch its removal. Two tests in `test_pr_changed_files.py`, both over the git-derived `_workflow_files()` population: the gate job's units by EQUALITY (an extra unit is a finding too — this is the repo's only CI repo-write), and every tracked workflow declaring a block at some level. Mutation-proved, all four reddening the intended test and the tree restoring green: code:write->read; drop pull-requests:read; add a top-level block to review-verdict.yml; drop dependency-scan.yml's block. Decisions-Edit: yes Refs: #748 --- .gitea/workflows/ci-image.yml | 4 +- .gitea/workflows/docker-build.yml | 8 +- docs/ci-cd.md | 33 ++++--- .../records/ci/actions-credential-scoping.md | 45 ++++++---- docs/guard-inventory.md | 2 +- scripts/tests/test_pr_changed_files.py | 86 +++++++++++++++++++ 6 files changed, 148 insertions(+), 30 deletions(-) diff --git a/.gitea/workflows/ci-image.yml b/.gitea/workflows/ci-image.yml index afda85a85..8fab69d6b 100644 --- a/.gitea/workflows/ci-image.yml +++ b/.gitea/workflows/ci-image.yml @@ -59,7 +59,9 @@ env: # (`ci.actions-credential-scoping`), so the injected GITEA_TOKEN serves only its single # `actions/checkout` — the repo's one checkout that still persists its credential into .git/config # (ersatztv#746 excluded it deliberately). `code: read` is therefore what makes that persisted -# credential read-only, which is a gain this file gets and the others already had. +# credential read-only. Note that is a DIFFERENT property from the other five, which persist no +# credential at all (`persist-credentials: false`) — read-only-persisted and not-persisted are not +# interchangeable, and this file has the weaker of the two. permissions: code: read diff --git a/.gitea/workflows/docker-build.yml b/.gitea/workflows/docker-build.yml index f5bf722eb..94cf1cdf2 100644 --- a/.gitea/workflows/docker-build.yml +++ b/.gitea/workflows/docker-build.yml @@ -118,9 +118,11 @@ env: # is NOT granted, and that holds at any owner default — it is not conditional on Restricted being on. # Only `review-verdict.yml` needs write; it declares that at the job and says why there. Full # rationale and the per-workflow credential audit: docs/ci-cd.md -> "Workflow token scope". -# Its registry pushes, its five `container:` image pulls and its three commit-status GET steps -# (`ETV_STATUS_AUTH`, a read-only GET via scripts/ci-detect-already-validated.sh) ALL authenticate -# with the scoped REGISTRY_* PAT, never the injected token — which therefore serves only its eight +# Every credentialed thing this file does uses the scoped REGISTRY_* PAT, never the injected token: +# its registry pushes, its five `container:` image pulls, its three commit-status GET steps +# (`ETV_STATUS_AUTH` at :186/:456/:617, each a read-only GET via +# scripts/ci-detect-already-validated.sh) and its registry tag READ (`ETV_REGISTRY_AUTH` at :144, via +# scripts/ci-toolchain-image-resolves.sh). The injected token therefore serves only its eight # `actions/checkout` steps. Note this file needs no `packages:` unit for that reason: the # `container:` blocks carry explicit `credentials:`. permissions: diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 9a16859ee..e21fd5620 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -188,9 +188,15 @@ as a matched pair that differed **only** in one unit: Both runs logged `Retarget fence: 0 retarget event(s) … (trusted=yes)` and both reached the classify step's start marker, so the reads succeeded in both and neither failure was the ersatztv#751 dropped-step shape. The negative control is the load-bearing half: it is what shows the declaration -*binds* rather than being a no-op that happens to sit next to a permissive default. Both ran while -the owner default was still `permissive`, which establishes that the five read-only declarations -bind **today**, without any flip. +*binds* rather than being a no-op that happens to sit next to a permissive default. Both controls carried the block **at job level**, on `set-verdict-status` — the same level and the +same job as the shipped `review-verdict.yml`, so the gate's own shape is what was measured. + +Both ran while the owner default was still `permissive`, so the binding holds **today**, without any +flip. One scope limit, stated rather than glossed: the other five workflows declare `permissions:` at +**top level**, a form the probe did not exercise. That is inferred, not measured — but the failure +direction is safe (a top-level key Gitea ignored would be a security no-op, never a brick), and +`docker-build.yml` and `pr-checks.yml` are `on: pull_request`, so this change's own CI exercises +theirs. **What the probe does NOT establish — read this before flipping the owner default.** It says nothing about what a declared `code: write` receives under a **Restricted** default. GitHub's semantics let @@ -211,9 +217,13 @@ holds repo-write. The property gained is not "workflows can no longer assert the that *only a workflow visibly opting into repo-write can*, instead of every job on the instance holding write implicitly. Real, and weaker than closed. -⚠️ **Changing any of these blocks requires the scratch-base probe**, not CI-green on the editing PR -— `review-verdict.yml` is base-resolved and is never exercised by its own PR. See the probe recipe -in the review-verdict gate section below. +⚠️ **Changing `review-verdict.yml`'s block requires the scratch-base probe**, not CI-green on the +editing PR — that workflow is base-resolved and is never exercised by its own PR, so its own checks +say nothing about the edit. See the probe recipe in the review-verdict gate section below. The other +five are ordinary: `docker-build.yml` and `pr-checks.yml` run on `pull_request` and are exercised by +the editing PR directly. (`docker-build.yml`'s `build` job carries +`if: github.event_name != 'pull_request'`, so its registry-push path is first exercised on the +post-merge push to `main` — that reddens `main`, not the gate, and is recoverable.) ## Checkouts drop the persisted credential (ersatztv#746) @@ -1515,10 +1525,13 @@ as establishing that the gate cannot be forged (see the residual below, and ersa `pull_request` because it builds the PR's code, so it got the read-only status identity instead — its `ETV_STATUS_AUTH` is now a PAT scoped `write:package` + `read:repository`, which the status endpoint refuses (`ci.actions-credential-scoping`, ersatztv#697). The inventory was never that one - workflow, though: Gitea injects a write-capable `GITEA_TOKEN` into every job and branch protection - binds the *context*, not its issuer. Gitea >=1.26 with the Actions default set to **Restricted** - (server-management#714) binds the injected token, but does not close the class either — not against - a personal token, and not against `RENOVATE_TOKEN` (ersatztv#742). **And none of it was necessary: + workflow, though, and branch protection binds the *context*, not its issuer. Gitea used to inject + a write-capable `GITEA_TOKEN` into every job; since ersatztv#748 every workflow here declares + `permissions:` explicitly, so five hold `code: read` and only `review-verdict.yml`'s gate job holds + write — see "Workflow token scope" above, which is the current statement of this. That narrows the + inventory; it does not close the class — not against a personal token, and not against + `RENOVATE_TOKEN` (ersatztv#742). The owner-level **Restricted** default (server-management#714) is + a separate, still-open lever whose effect on a declared `code: write` is unmeasured. **And none of it was necessary: direct pushes to `main` were server-side permitted, so the gate could be skipped without any forgery (ersatztv#743). That is now CLOSED — `main` carries `enable_push: false` **and** `block_admin_merge_override: true`, so it is reachable only through the PR merge path, the one path diff --git a/docs/decisions/records/ci/actions-credential-scoping.md b/docs/decisions/records/ci/actions-credential-scoping.md index 3c1bebb67..f808ea532 100644 --- a/docs/decisions/records/ci/actions-credential-scoping.md +++ b/docs/decisions/records/ci/actions-credential-scoping.md @@ -105,18 +105,6 @@ be written without an exemption list once #744 has landed. docs, NOT verified (2026-08-05): `permissions:` landed in 1.26.0 (Gitea PR #36173); no `app.ini` lever at any version; Gitea rejects GitHub's `statuses`/`checks` scopes. -MEASURED 2026-08-27 on 1.27.1 (#748), superseding the "honored here is UNPROBED" caveat above: a -matched pair of scratch-base probe PRs, identical except for the gate job's single permission unit. -`code: write` -> job success, `Posted review-verdict/h10-PROBE=success`, status `creator: null`. -`code: read` -> job failure at the POST, `curl` exit 22 (HTTP >=400 under `-f`), NO status written. -Both logged `Retarget fence: 0 retarget event(s) ... (trusted=yes)` and both reached the classify -step's start marker, so the `issues: read` and `pull-requests: read` units were exercised and -neither run was the #751 dropped-step shape. Both ran while the owner default was `permissive`, -which is what establishes the five read-only declarations bind today. It does NOT establish what a -declared `code: write` gets under a Restricted default — see the caveat in `rule`. The negative control is -the load-bearing half — without it a green positive is equally consistent with `permissions:` being -a no-op beside a permissive default. Probe branches and PRs (#851, #852) deleted after reading. - **Version caveat — this record's measurements are pinned to 1.25.4, the instance is now 1.27.1.** The instance was upgraded mid-session on 2026-08-05 (#743). Everything above measured on 1.25.4 is therefore a *dated* claim, not a current one: the scope enum, the `reqRepoWriter` gate and the 403 @@ -124,18 +112,45 @@ probe were all taken pre-upgrade and have NOT been re-run. They are recorded hon date and are the best evidence available, but do not cite them as current behaviour without re-probing. Re-verification of the 1.25.4-pinned claims across the CI docs is tracked separately. +**MEASURED 2026-08-27 on 1.27.1 (#748) — this block is NOT covered by the 1.25.4 caveat above; it is +the re-probe that caveat asks for.** It supersedes the "whether `permissions:` is honored here is +UNPROBED" claim. Method: a matched pair of scratch-base probe PRs, identical except for one +permission unit, the block carried **at job level on `set-verdict-status`** — the same level and the +same job as the shipped `review-verdict.yml`, so the gate's own shape is what was measured. + +- `code: write` -> job success, `Posted review-verdict/h10-PROBE=success`, status `creator: null`. +- `code: read` -> job failure at the POST, `curl` exit 22 (HTTP >=400 under `-f`), NO status written. + +What each observation licenses, kept separate from what it does not. `issues: read` was exercised: +both runs logged `Retarget fence: 0 retarget event(s) ... (trusted=yes)`, which requires +`count_retargets` to have walked `/issues/{n}/timeline`. `pull-requests: read` was exercised: the +positive control posted `=success`, and a `success` is reachable only through `complete=yes`, which +requires `scripts/pr-changed-files.sh` to have paged `/pulls/{n}` and `/pulls/{n}/files` to a +validated end — the fence and the start marker do NOT show this, the `success` does. Both runs +reached the classify step's start marker, so neither was the #751 dropped-step shape. + +The negative control is the load-bearing half — without it a green positive is equally consistent +with `permissions:` being a no-op sitting beside a permissive default. + +NOT established by this probe: what a declared `code: write` receives under a RESTRICTED owner +default (both controls ran under `permissive`) — see the caveat in `rule`; and the TOP-LEVEL +declaration form the other five workflows use, which is inferred rather than measured. That +inference fails safe (a top-level key Gitea ignored is a security no-op, never a brick), and +`docker-build.yml`/`pr-checks.yml` are `on: pull_request`, so their form is exercised by any PR +touching them. Probe branches and PRs (#851, #852) deleted after reading. + **Surviving routes — this record is not a mitigation for any of them.** `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same store and still cannot be scoped down, because Renovate needs repo write; what changed is that a verdict posted with it is no longer *inherited* — `renovate` is not in `H10_REVIEWERS`, so `review-verdict.yml` re-derives it (#742). It remains status-capable, so it can still overwrite a status; it can no longer impersonate a reviewer. The injected `GITEA_TOKEN` was write-capable in every job. -That is no longer the mechanism: #748 declared `permissions:` on all six workflows (2026-08-27), and -and because the declaration binds under the current `permissive` default, five of them now hold +That is no longer the mechanism: #748 declared `permissions:` on all six workflows (2026-08-27). +Because the declaration binds under the current `permissive` default, five of them now hold `code: read` and only `review-verdict.yml`'s gate job holds write. So the route is narrowed NOW, without the owner-level flip. The owner-level Restricted default (server-management#714) remains worth applying as the instance-wide floor for repos that declare nothing — but it is NOT yet shown safe here: whether a declared `code: write` survives a Restricted default is unmeasured, and if -Gitea caps at the default the way GitHub does, the flip would un-writable the gate. That is the +Gitea caps at the default the way GitHub does, the flip would make the gate unwritable. That is the remaining probe, not a formality. What is NOT closed: `review-verdict.yml` still holds repo-write, so the property is "only a workflow visibly opting into write can assert the verdict", not "no workflow can". Scoping still bounds what a job may DO, never whether attacker YAML RUNS. A diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index 16d7ae765..526664aec 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -133,7 +133,7 @@ The audit #806 asked for, recorded whichever way it came out, because "we looked | `test_guard_inventory.py` | `.claude/hooks/*.sh`, `.husky/*`, `scripts/tests/test_*.py`, workflow/hook callers | **yes** — set equality against this table | converted to the index; `.husky/_/` had been excluded only because `_` is a directory, so the obvious "make it recursive" edit would have reintroduced #778's defect here | | `test_hook_fire_log.py` | `.claude/hooks/*.sh` | **yes** — every hook must be instrumented | converted; an untracked scratch `.sh` used to demand instrumentation and redden the suite on that checkout alone | | `test_ci_image_pin_population.py` | `.gitea/workflows/*.yml` + `*.yaml` | **yes** — "docker-build is the ONLY workflow pinning the toolchain image" | converted, and `*.yaml` added: Gitea accepts both spellings, so a `.yaml` workflow was structurally invisible while the test read as covering all of them | -| `test_pr_changed_files.py` | `.gitea/workflows/*.y*ml` | **yes** — "no OTHER workflow writes the review-verdict status" | converted. Not on #806's list and found by cold review on the pushed head: an untracked `.yaml` dropped in `.gitea/workflows/` reddened two guards while absent from the index — the issue's list of files to assess was a starting point, not the population | +| `test_pr_changed_files.py` | `.gitea/workflows/*.y*ml` | **yes** — "no OTHER workflow writes the review-verdict status", and since ersatztv#748 "every tracked workflow declares a `permissions:` block" (both over the same git-derived population) | converted. Not on #806's list and found by cold review on the pushed head: an untracked `.yaml` dropped in `.gitea/workflows/` reddened two guards while absent from the index — the issue's list of files to assess was a starting point, not the population | | `web/src/api/pageSizeCallSites.guard.test.ts` | `web/src/**/*.{ts,tsx,mts,cts}` via `import.meta.glob` | **yes** — an unregistered discovered site fails | DEFERRED to #819, assessed not skipped. The glob is a documented workaround: `@types/node` is deliberately out of `tsconfig.app.json`, and wiring it in was tried and reverted (it leaked Node's `setTimeout` into the app project and broke three unrelated tests), so there is no `node:child_process` to reach the index from. Over-enumerates, so it fails loudly rather than going blind | | `test_ci_release_path_scan_job.py` | `.gitea/workflows/*.y*ml` + `scripts/**` | **no** — a fixture assembling a tmp harness, asserted about behaviour not membership | takes its file LIST from the index anyway, for hermeticity not completeness: `shutil.copytree` copied whatever was on disk, including untracked files and `scripts/__pycache__`, into a tree whose behaviour the probes then measure. Content still comes from the working tree. The copy is not a git repo, so the two files this step RUNS may not use the helper — see the fixture docstring | | `test_ci_dropped_step_guard.py` | the parsed workflow document | **no filesystem population at all** | unchanged; its residual is `MARKED_JOBS`, a SCOPE mirror of the required contexts on `main`, which #806 does not close | diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 5f685c971..4d2ac8007 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -3567,3 +3567,89 @@ 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". + """ + 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). + """ + import yaml + + missing = [] + for wf in _workflow_files(): + doc = yaml.safe_load(wf.read_text()) or {} + if doc.get("permissions") is not None: + continue + 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. + if jobs and all((job or {}).get("permissions") is not None for job in jobs): + continue + 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) 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." + ) -- 2.47.3 From efe711c5a9826a06fd026061153c5cd4efe5df10 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 20:59:23 +0200 Subject: [PATCH 06/12] =?UTF-8?q?fix(748):=20round=203=20=E2=80=94=20cite?= =?UTF-8?q?=20jobs=20not=20lines,=20and=20fix=20the=20boundary=20not=20the?= =?UTF-8?q?=20site?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every finding from the re-review of the round-2 fix. The round-2 comment fix shipped four line numbers that its own two added lines invalidated (:186/:456/:617 and :144 were already 188/458/619 and 146 in the committed file) — "broke a number while fixing numbers", in the commit that was fixing a number. Root cause: I transcribed them from the review report, which described the PRE-edit file, instead of re-deriving from the post-edit tree. The fix is not better numbers: docker-build.yml is ~1150 lines and any edit above a citation silently invalidates it, so the sites are now named by JOB (`toolchain-preflight`; `test`/`migrations`/`functional-e2e`), which the YAML itself defines and an edit cannot shift. Comment says so, so the next editor does not reintroduce line numbers. Round 2 split the per-unit warrants in the record but left `docs/ci-cd.md` asserting the retracted one — one wrong copy became two divergent copies. Both now state it the same way and per unit: the fence shows `issues: read`; what shows `pull-requests: read` is the `=success`, reachable only via `complete=yes`. The ⚠️ was over-generalized in round 1 and over-reassuring in round 2: it said "the other five are ordinary" and then justified two. Split — docker-build and pr-checks are `on: pull_request` and ARE exercised by an editing PR; ci-image (`push`, self-path-filtered), dependency-scan and renovate (`schedule` / `workflow_dispatch`) are NOT, and their first run is post-merge or next schedule. `ci-image.yml`'s comment claimed the other five "persist no credential (`persist-credentials: false`)". `renovate.yml` sets no such key — it persists nothing because it never checks out. Four of five set the flag. THE BOUNDARY, not the site. Round 2 fixed the one stale-claim site the review named. A grep by SUBJECT rather than by the corrected wording found the same superseded inventory in two more records, both of them ones a future editor resolves from the catalog when reasoning about forgery routes: * `ci.gate-trigger-base-resolved` — "Gitea injects GITEA_TOKEN into EVERY job, defaulting to read/write, so push- and workflow_dispatch-triggered workflows alike are routes". Now scoped to the workflows that exist today, with the note that the SHAPE is still open and that is what the new guard covers. * `release.verdict-status-check` — the same inventory in the H10 gate's own record. * `release.main-direct-push-disabled` — dated parenthetical, conclusion unaffected, corrected in passing. Guards: `test_..._declares_exactly_the_units_...` now states what it does NOT catch — it TRANSCRIBES the unit set rather than deriving it from the job's API calls, so a new `gh` call needing an undeclared unit leaves it green. Same honesty the sibling expression test already carries. And `test_every_tracked_workflow_declares_a_permissions_block` gained an anti-vacuity assert: `tracked_children` matches direct children only, so a moved `.gitea/workflows/` gave an EMPTY population and the test passed having examined nothing — the #778 shape its own docstring invokes. Measured before the guard (`git mv .gitea/workflows .gitea/wf-renamed` -> 1 passed); measured after -> red, and green again on restore. Correcting round 2's commit message: it said both new tests run over the git-derived `_workflow_files()` population. Only the second does; the first addresses `review-verdict.yml` directly. Also scoped the record's new measurement heading — it re-probes ONE item of the 1.25.4 caveat (the `permissions:` question) and does not discharge the scope enum, the `reqRepoWriter` gate or the `write:package` 403, which stay dated. Decisions-Edit: yes Refs: #748 --- .gitea/workflows/ci-image.yml | 7 ++-- .gitea/workflows/docker-build.yml | 9 +++-- docs/ci-cd.md | 38 ++++++++++++++----- .../records/ci/actions-credential-scoping.md | 8 ++-- .../records/ci/gate-trigger-base-resolved.md | 11 ++++-- .../release/main-direct-push-disabled.md | 3 +- .../records/release/verdict-status-check.md | 4 +- scripts/tests/test_pr_changed_files.py | 22 ++++++++++- 8 files changed, 76 insertions(+), 26 deletions(-) diff --git a/.gitea/workflows/ci-image.yml b/.gitea/workflows/ci-image.yml index 8fab69d6b..47b8a6664 100644 --- a/.gitea/workflows/ci-image.yml +++ b/.gitea/workflows/ci-image.yml @@ -59,9 +59,10 @@ env: # (`ci.actions-credential-scoping`), so the injected GITEA_TOKEN serves only its single # `actions/checkout` — the repo's one checkout that still persists its credential into .git/config # (ersatztv#746 excluded it deliberately). `code: read` is therefore what makes that persisted -# credential read-only. Note that is a DIFFERENT property from the other five, which persist no -# credential at all (`persist-credentials: false`) — read-only-persisted and not-persisted are not -# interchangeable, and this file has the weaker of the two. +# credential read-only. Note that is a DIFFERENT and WEAKER property than the other workflows have: +# the four that check out all set `persist-credentials: false` and so persist nothing, and +# `renovate.yml` persists nothing because it never checks out. Read-only-persisted and not-persisted +# are not interchangeable. permissions: code: read diff --git a/.gitea/workflows/docker-build.yml b/.gitea/workflows/docker-build.yml index 94cf1cdf2..a39b7df2f 100644 --- a/.gitea/workflows/docker-build.yml +++ b/.gitea/workflows/docker-build.yml @@ -120,10 +120,11 @@ env: # rationale and the per-workflow credential audit: docs/ci-cd.md -> "Workflow token scope". # Every credentialed thing this file does uses the scoped REGISTRY_* PAT, never the injected token: # its registry pushes, its five `container:` image pulls, its three commit-status GET steps -# (`ETV_STATUS_AUTH` at :186/:456/:617, each a read-only GET via -# scripts/ci-detect-already-validated.sh) and its registry tag READ (`ETV_REGISTRY_AUTH` at :144, via -# scripts/ci-toolchain-image-resolves.sh). The injected token therefore serves only its eight -# `actions/checkout` steps. Note this file needs no `packages:` unit for that reason: the +# (`ETV_STATUS_AUTH` in jobs `test`, `migrations` and `functional-e2e`, each a read-only GET via +# scripts/ci-detect-already-validated.sh) and its registry tag READ (`ETV_REGISTRY_AUTH` in job +# `toolchain-preflight`, via scripts/ci-toolchain-image-resolves.sh). The injected token therefore +# serves only its eight `actions/checkout` steps. Sites are named by JOB, not by line: this file is +# ~1150 lines and any edit above a citation silently invalidates it. Note this file needs no `packages:` unit for that reason: the # `container:` blocks carry explicit `credentials:`. permissions: code: read diff --git a/docs/ci-cd.md b/docs/ci-cd.md index e21fd5620..96dfa2719 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -185,11 +185,21 @@ as a matched pair that differed **only** in one unit: | Positive | `code: write` | job `success`; `Posted review-verdict/h10-PROBE=success`; `creator: null` | | Negative | `code: read` | job `failure`; `curl` exit 22 (HTTP ≥400 under `-f`) at the POST; **no status written** | -Both runs logged `Retarget fence: 0 retarget event(s) … (trusted=yes)` and both reached the classify -step's start marker, so the reads succeeded in both and neither failure was the ersatztv#751 -dropped-step shape. The negative control is the load-bearing half: it is what shows the declaration -*binds* rather than being a no-op that happens to sit next to a permissive default. Both controls carried the block **at job level**, on `set-verdict-status` — the same level and the -same job as the shipped `review-verdict.yml`, so the gate's own shape is what was measured. +Both runs reached the classify step's start marker, so neither failure was the ersatztv#751 +dropped-step shape. What shows each read unit was exercised is a different observation for each, and +they are not interchangeable: + +- **`issues: read`** — both runs logged `Retarget fence: 0 retarget event(s) … (trusted=yes)`, which + requires `count_retargets` to have walked `/issues/{n}/timeline`. +- **`pull-requests: read`** — the positive control posted `=success`, and a `success` is reachable + only through `complete=yes`, which requires `scripts/pr-changed-files.sh` to have paged + `/pulls/{n}` and `/pulls/{n}/files` to a validated end. The fence and the start marker do **not** + show this; the `success` does. + +The negative control is the load-bearing half: it is what shows the declaration *binds* rather than +being a no-op that happens to sit next to a permissive default. Both controls carried the block **at +job level**, on `set-verdict-status` — the same level and the same job as the shipped +`review-verdict.yml`, so the gate's own shape is what was measured. Both ran while the owner default was still `permissive`, so the binding holds **today**, without any flip. One scope limit, stated rather than glossed: the other five workflows declare `permissions:` at @@ -219,11 +229,19 @@ holding write implicitly. Real, and weaker than closed. ⚠️ **Changing `review-verdict.yml`'s block requires the scratch-base probe**, not CI-green on the editing PR — that workflow is base-resolved and is never exercised by its own PR, so its own checks -say nothing about the edit. See the probe recipe in the review-verdict gate section below. The other -five are ordinary: `docker-build.yml` and `pr-checks.yml` run on `pull_request` and are exercised by -the editing PR directly. (`docker-build.yml`'s `build` job carries -`if: github.event_name != 'pull_request'`, so its registry-push path is first exercised on the -post-merge push to `main` — that reddens `main`, not the gate, and is recoverable.) +say nothing about the edit. See the probe recipe in the review-verdict gate section below. + +The other five split two ways, and only one half is covered by an editing PR's own CI: + +- `docker-build.yml` and `pr-checks.yml` run on `pull_request`, so an editing PR exercises their + blocks directly. (`docker-build.yml`'s `build` job carries + `if: github.event_name != 'pull_request'`, so its registry-push path is first exercised on the + post-merge push to `main` — that reddens `main`, not the gate, and is recoverable.) +- `ci-image.yml` (`push`, path-filtered to itself), `dependency-scan.yml` and `renovate.yml` + (`schedule` / `workflow_dispatch` only) are **not** exercised by an editing PR at all — their first + run is after merge or on the next schedule. None is a required check, so a wrong block there + reddens a non-blocking run rather than the gate, but do not read an editing PR's green as covering + them. ## Checkouts drop the persisted credential (ersatztv#746) diff --git a/docs/decisions/records/ci/actions-credential-scoping.md b/docs/decisions/records/ci/actions-credential-scoping.md index f808ea532..85b1c0166 100644 --- a/docs/decisions/records/ci/actions-credential-scoping.md +++ b/docs/decisions/records/ci/actions-credential-scoping.md @@ -112,9 +112,11 @@ probe were all taken pre-upgrade and have NOT been re-run. They are recorded hon date and are the best evidence available, but do not cite them as current behaviour without re-probing. Re-verification of the 1.25.4-pinned claims across the CI docs is tracked separately. -**MEASURED 2026-08-27 on 1.27.1 (#748) — this block is NOT covered by the 1.25.4 caveat above; it is -the re-probe that caveat asks for.** It supersedes the "whether `permissions:` is honored here is -UNPROBED" claim. Method: a matched pair of scratch-base probe PRs, identical except for one +**MEASURED 2026-08-27 on 1.27.1 (#748) — this block is NOT covered by the 1.25.4 caveat above.** It +re-probes exactly ONE of that caveat's items, the `permissions:` question, and supersedes the +"whether `permissions:` is honored here is UNPROBED" claim. It does NOT discharge the caveat: the +scope enum, the `reqRepoWriter` gate and the `write:package` 403 are still 1.25.4-dated and still +un-re-run. Method: a matched pair of scratch-base probe PRs, identical except for one permission unit, the block carried **at job level on `set-verdict-status`** — the same level and the same job as the shipped `review-verdict.yml`, so the gate's own shape is what was measured. diff --git a/docs/decisions/records/ci/gate-trigger-base-resolved.md b/docs/decisions/records/ci/gate-trigger-base-resolved.md index 1df318d2c..2d4c8c626 100644 --- a/docs/decisions/records/ci/gate-trigger-base-resolved.md +++ b/docs/decisions/records/ci/gate-trigger-base-resolved.md @@ -50,9 +50,14 @@ authority the judged thing can assert is not a gate, not because an attack was e **The class is NOT closed, and this record must not be read as claiming otherwise.** This fixed one instance of "a ref-resolved workflow can obtain credentials that POST a commit status", and that -inventory is not a short list: Gitea injects `GITEA_TOKEN` into **every** job, defaulting to -read/**write**, so head-resolved, `push`-triggered and `workflow_dispatch` workflows alike are routes -(1.24+ loads a dispatched definition from the selected branch). A collaborator's own API token is a +inventory is not a short list. Gitea injects `GITEA_TOKEN` into **every** job, and it used to +default to read/**write**, so head-resolved, `push`-triggered and `workflow_dispatch` workflows alike +were routes (1.24+ loads a dispatched definition from the selected branch). Narrowed since #748 +(2026-08-27): every workflow in this repo now declares `permissions:` explicitly, and five hold +`code: read`, so none of them can POST a status. The route is closed for the workflows that exist +TODAY, not for the shape — a new workflow that declares write, or omits a declaration and inherits +the owner default, is a route again, which is why `test_pr_changed_files.py` asserts every tracked +workflow declares a block. A collaborator's own API token is a route with no workflow at all — branch protection binds the *context*, not its issuer. Full inventory in `#697`, whose credential half is resolved in `ci.actions-credential-scoping` — the registry secret no longer carries status-write. That does NOT leave the workflow routes provenance-free: any diff --git a/docs/decisions/records/release/main-direct-push-disabled.md b/docs/decisions/records/release/main-direct-push-disabled.md index 576bed3af..fa5b06d94 100644 --- a/docs/decisions/records/release/main-direct-push-disabled.md +++ b/docs/decisions/records/release/main-direct-push-disabled.md @@ -32,7 +32,8 @@ the only admin exempts everybody.** Both fields are now set; treat them as one c cite `enable_push: false` alone as the reason the gate holds. **What this actually closes, and what it does not.** It closes the *write-only* credential routes, -which is most of #743's own "who can do it" list: the injected `GITEA_TOKEN` (repo write, not admin), +which is most of #743's own "who can do it" list: the injected `GITEA_TOKEN` (repo write at the time, +narrowed to `code: read` in five of six workflows by #748; not admin either way), `RENOVATE_TOKEN`, and any non-admin collaborator PAT. Those can no longer reach `main` at all, by any path that skips the gate. diff --git a/docs/decisions/records/release/verdict-status-check.md b/docs/decisions/records/release/verdict-status-check.md index c74ceec93..0be6612f1 100644 --- a/docs/decisions/records/release/verdict-status-check.md +++ b/docs/decisions/records/release/verdict-status-check.md @@ -184,7 +184,9 @@ described as one: credentials that can POST a commit status can still forge `review-verdict/h10`; `docker-build.yml` demonstrably could, and must stay head-resolved because it builds the PR's own code — so #697 scoped its credential instead (`ci.actions-credential-scoping`), leaving AT LEAST - these: the injected `GITEA_TOKEN` (posts with `creator: null`), `RENOVATE_TOKEN` (a + these: the injected `GITEA_TOKEN` (posts with `creator: null`) — narrowed by #748, which declared + `permissions:` on all six workflows so only `review-verdict.yml`'s own gate job still holds + repo-write, though that job's token remains status-capable by construction — `RENOVATE_TOKEN` (a `write:repository` bot PAT in the same secret store, so it posts with a real creator — it is no longer *inherited* as a verdict, since #742 replaced the "non-null creator" test with membership in the workflow's `H10_REVIEWERS` allow-list and `renovate` is not on it, but it can still overwrite diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 4d2ac8007..685de1fd7 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -3603,6 +3603,15 @@ def test_the_verdict_gate_job_declares_exactly_the_units_its_api_calls_need(): 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, ( @@ -3636,8 +3645,19 @@ def test_every_tracked_workflow_declares_a_permissions_block(): """ 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 _workflow_files(): + for wf in workflows: doc = yaml.safe_load(wf.read_text()) or {} if doc.get("permissions") is not None: continue -- 2.47.3 From 01e51830ed62368c3f64c8c198d26ac5be88b4f7 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 21:15:05 +0200 Subject: [PATCH 07/12] =?UTF-8?q?fix(748):=20the=20boundary=20was=20one=20?= =?UTF-8?q?level=20up=20=E2=80=94=20the=20`rule:`=20field,=20not=20the=20b?= =?UTF-8?q?ody?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4. Round 3 corrected the BODY of `ci.gate-trigger-base-resolved` and left its `rule:` frontmatter, which `build_decisions_catalog.py` mirrors verbatim into `docs/decisions/README.md`. Three compounding consequences, all now fixed: * it contradicted the body of its own record twelve lines below it; * the catalog is where CLAUDE.md says a decision lookup STARTS, so the stale version was the one a future editor read FIRST and the corrected body sat behind it; * as stated it was false for this repo today, not merely dated — "any ref-resolved workflow can still forge `review-verdict/h10`". This is the same fixed-the-site-not-the-class shape recurring one level up. My round-3 sweep grepped body prose and missed it because the `rule:` field is a single long YAML line and my pattern was anchored on wording I had already corrected. Re-swept by subject with a wider pattern across `docs/` and `CLAUDE.md`; the remaining hits are past-tense, already corrected, or about a different subject (the secret store; "credentials that CAN post a status", which stays true). The rule now says what #748 does and does not close: current instances closed, the SHAPE open, and the new guard is what covers a newly added workflow. GRADING, not just correcting. The body asserted "none of them can POST a status" at full strength while `ci-cd.md` and `ci.actions-credential-scoping` both grade the same claim as resting on the TOP-LEVEL declaration form, which is inferred rather than probed. The corpus graded it in two places and asserted it ungraded in the third — the record about forgery routes. Both the rule and the body now carry the qualifier. Also: `docker-build.yml`'s round-3 meta-note had been inserted between "uses the REGISTRY_* PAT" and "needs no `packages:` unit for that reason", stealing the antecedent of "that reason". Moved to the end of the block. And `ci-cd.md` understated `ci-image.yml`'s trigger filter (`docker/ci/**` AND its own file, not just its own file). Decisions-Edit: yes Refs: #748 --- .gitea/workflows/docker-build.yml | 8 +++++--- docs/ci-cd.md | 2 +- docs/decisions/README.md | 2 +- docs/decisions/records/ci/gate-trigger-base-resolved.md | 6 ++++-- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/.gitea/workflows/docker-build.yml b/.gitea/workflows/docker-build.yml index a39b7df2f..7e2d2971f 100644 --- a/.gitea/workflows/docker-build.yml +++ b/.gitea/workflows/docker-build.yml @@ -123,9 +123,11 @@ env: # (`ETV_STATUS_AUTH` in jobs `test`, `migrations` and `functional-e2e`, each a read-only GET via # scripts/ci-detect-already-validated.sh) and its registry tag READ (`ETV_REGISTRY_AUTH` in job # `toolchain-preflight`, via scripts/ci-toolchain-image-resolves.sh). The injected token therefore -# serves only its eight `actions/checkout` steps. Sites are named by JOB, not by line: this file is -# ~1150 lines and any edit above a citation silently invalidates it. Note this file needs no `packages:` unit for that reason: the -# `container:` blocks carry explicit `credentials:`. +# serves only its eight `actions/checkout` steps. Note this file needs no `packages:` unit for that +# same reason: the `container:` blocks carry explicit `credentials:`. +# (Sites above are named by JOB, not by line number: this file is ~1150 lines, so any edit above a +# citation silently invalidates it — which is how the first version of this comment went stale two +# lines after it was written.) permissions: code: read diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 96dfa2719..59402c480 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -237,7 +237,7 @@ The other five split two ways, and only one half is covered by an editing PR's o blocks directly. (`docker-build.yml`'s `build` job carries `if: github.event_name != 'pull_request'`, so its registry-push path is first exercised on the post-merge push to `main` — that reddens `main`, not the gate, and is recoverable.) -- `ci-image.yml` (`push`, path-filtered to itself), `dependency-scan.yml` and `renovate.yml` +- `ci-image.yml` (`push`, path-filtered to `docker/ci/**` and its own file), `dependency-scan.yml` and `renovate.yml` (`schedule` / `workflow_dispatch` only) are **not** exercised by an editing PR at all — their first run is after merge or on the next schedule. None is a required check, so a wrong block there reddens a non-blocking run rather than the gate, but do not read an editing PR's green as covering diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 66218dcda..f1b443fb4 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -49,7 +49,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.exemption-provenance` | The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR's live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR's immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires for a `success` only when it is POSITIVELY identified as an APPROVED REVIEWER's verdict for THIS base, meaning a `.creator.login` that is a member of the workflow's `H10_REVIEWERS` allow-list (an existing `failure` takes the weaker ATTRIBUTABLE test and needs no membership, because inheriting a rejection can only withhold an exemption while re-deriving one can turn it green on an exempt PR) — non-null was too weak, since any account's credential satisfies it and `RENOVATE_TOKEN` is one that cannot be scoped down (#742) — AND a `Review-verdict:` description AND, when that description records a base (`(base: …)`, `release.verdict-status-check`), a base matching the PR's — tested by requiring the description to END with the exact literal `(base: )` and to contain exactly ONE such marker, never by extracting a value (see below); a present-but-different base is rejected, an absent one is not, since verdicts predating that convention carry none; every other shape, including any unrecognised one, is re-derived rather than trusted. The bot and docs-only exemptions are evaluated as INDEPENDENT predicates and the decision made afterwards, never as an `elif` chain. `edited` is in the workflow's `types:` so a retarget reclassifies — which gives DETECTION, not atomicity: status writes are not serialized, so a stale run can still post over a fresher one. That residual is now FENCED rather than merely tracked (NARROWED, not resolved — a retarget between the fence's final count and the POST still leaves a PERMANENT forged green, #849) — the job refuses to write at all if the PR's timeline retarget COUNT moved while it was classifying (`ci.verdict-write-retarget-fence`, #706) — leaving TWO windows, not one: the sub-round-trip window that no API without compare-and-set can close, AND the post-final-count/pre-POST window, which is not sub-round-trip and yields a PERMANENT green because the successor can consume the `edited` event and exit before the stale run posts last (#849). The PROTECTED path list additionally covers `CLAUDE.md` and `AGENTS.md` (#751) — they are not prose but the documents DEFINING the completion protocol, the merge-consent convention and the H10 rule, so protecting `.claude/` while the file specifying what it enforces stayed docs-only-exempt was the same self-exemption one directory over; driving the real classify body with a lone `CLAUDE.md` change produced an exemption `success`. `README.md` is deliberately not listed. It also covers `.codex/` (#711), which mirrors `.claude/hooks/` byte for byte including the merge-consent hook — latent while that directory is untracked, live the moment it is tracked; the list stays ENUMERATIVE rather than derived, because a derived rule would have to be evaluated against the very file list being classified. Reading the CURRENT status for input (3) must tolerate `statuses: null`: `GET /commits/{sha}/status` serialises a nil slice as `null`, not `[]`, on a head with no statuses yet, and an `array`-only gate made `read_existing_verdict` `exit 1` and post nothing at all (#751, `ci.workflow-run-body-no-expressions`) — `null` is accepted only when `total_count` is 0, so a body that merely lost its array is still refused. Path predicates are evaluated by COUNTING with `grep -c`, never `\| grep -q` (SIGPIPE inversion) and never a here-string (temp-space failure) — see `ci.grep-q-pipefail-inversion`. | 2026-07-29 | [link](records/ci/exemption-provenance.md) | | `ci.format-gate-folder-mode` | The blocking `format` CI job (and matching pre-commit hook) runs `dotnet format whitespace . --folder --include ` instead of loading the full MSBuild/Roslyn solution, cutting the gate from ~480s to ~0.5s with unchanged whitespace/charset coverage. | 2026-07-19 | [link](records/ci/format-gate-folder-mode.md) | | `ci.functional-e2e-harness` | The `functional-e2e` CI job boots the PR's own code from source via `dotnet run` (`scripts/e2e-local.sh`) and runs deterministic assertions (`scripts/e2e-functional.sh`) as an advisory (non-blocking) job, not a `build` dependency or required check. Originally curl-only; since #445 the same job carries a second, headless-browser step for the contracts curl cannot express — see `ci.ui-e2e-harness`. | 2026-07-16 | [link](records/ci/functional-e2e-harness.md) | -| `ci.gate-trigger-base-resolved` | The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR's own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch's gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree's scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class: Gitea injects a write-capable `GITEA_TOKEN` into EVERY job, so any ref-resolved workflow — and a collaborator's own API token, since branch protection binds the context and not its issuer — can still forge `review-verdict/h10`. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI's registry secret was the ADMIN account's basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential's route (an ALLOW-LISTED user credential's forgery carries a matching `creator` and is inherited as a verdict; an Actions job's carries `creator: null` and is re-derived, as since #742 is every account outside `H10_REVIEWERS` — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger's `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The injected token stays write-capable until Gitea >=1.26 with a Restricted default (server-management#714), and a collaborator's own token remains unfixable; the exemption path has its own separate defects in #698. | 2026-07-28 | [link](records/ci/gate-trigger-base-resolved.md) | +| `ci.gate-trigger-base-resolved` | The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR's own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch's gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree's scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class. Gitea injects `GITEA_TOKEN` into EVERY job and it USED to be write-capable everywhere, so any ref-resolved workflow could forge `review-verdict/h10`; since #748 (2026-08-27) every workflow here declares `permissions:` and five hold `code: read`, so no ref-resolved workflow that exists TODAY can post a status (the top-level declaration form those five use is INFERRED rather than probed — see `ci.actions-credential-scoping`). That closes the current instances, NOT the shape: a newly added workflow declaring write is a route again, which is why `test_pr_changed_files.py` asserts every tracked workflow declares a block. And a collaborator's own API token still can, since branch protection binds the context and not its issuer. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI's registry secret was the ADMIN account's basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential's route (an ALLOW-LISTED user credential's forgery carries a matching `creator` and is inherited as a verdict; an Actions job's carries `creator: null` and is re-derived, as since #742 is every account outside `H10_REVIEWERS` — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger's `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The injected token stays write-capable until Gitea >=1.26 with a Restricted default (server-management#714), and a collaborator's own token remains unfixable; the exemption path has its own separate defects in #698. | 2026-07-28 | [link](records/ci/gate-trigger-base-resolved.md) | | `ci.gitea-milestone-filter-noop` | Never filter issues with the server-side `?milestones=` parameter — fetch all open issues once and filter LOCALLY on each issue's `.milestone.title`. | 2026-07-21 | [link](records/ci/gitea-milestone-filter-noop.md) | | `ci.grep-q-pipefail-inversion` | In any script running under `set -o pipefail`, a security or classification predicate of the form `producer \| grep -q…` is FORBIDDEN: `grep -q` exits at its first match, the producer then takes SIGPIPE and exits 141 once the data exceeds the pipe buffer (~64K), so `pipefail` reports the pipeline as FAILED even though grep MATCHED — inverting the predicate exactly when the input is large. A here-string (`grep -q… <<< "$data"`) is ALSO forbidden: bash materialises a large here-string via temporary storage, so it fails when temp space is full or unwritable, and inside an `if`/`!` that failure flips the predicate the same way. COUNT instead — `n=$(printf '%s\n' "$data" \| grep -cE "$re")` — because `grep -c` drains stdin (no early exit, no SIGPIPE) over an ordinary pipe (no temp file). Read grep's status honestly: exit 1 means a zero count and is a legitimate answer, anything >1 is a real error. Evaluate the counts ONCE at TOP LEVEL, never inline inside an `if`/`elif` condition: inside `$( )` an `exit` leaves only the subshell and `set -e` does not fire, so an error silently reads as "no match". Validate that each result is numeric and fail closed if not. This applies to both the enforced gate `.gitea/workflows/review-verdict.yml` and the advisory hook `.claude/hooks/pretooluse-merge-consent.sh`. | 2026-07-29 | [link](records/ci/grep-q-pipefail-inversion.md) | | `ci.infra-shaped-red-under-load` | When a job dies inside a setup/cache step before your code compiles, check the runner host's load before diagnosing the diff, and never file a CI bug off one sample under pressure. | 2026-07-21 | [link](records/ci/infra-shaped-red-under-load.md) | diff --git a/docs/decisions/records/ci/gate-trigger-base-resolved.md b/docs/decisions/records/ci/gate-trigger-base-resolved.md index 2d4c8c626..ec647654b 100644 --- a/docs/decisions/records/ci/gate-trigger-base-resolved.md +++ b/docs/decisions/records/ci/gate-trigger-base-resolved.md @@ -5,7 +5,7 @@ status: active since: '2026-07-28' supersedes: none superseded-by: none -rule: 'The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR''s own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch''s gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree''s scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class: Gitea injects a write-capable `GITEA_TOKEN` into EVERY job, so any ref-resolved workflow — and a collaborator''s own API token, since branch protection binds the context and not its issuer — can still forge `review-verdict/h10`. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI''s registry secret was the ADMIN account''s basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential''s route (an ALLOW-LISTED user credential''s forgery carries a matching `creator` and is inherited as a verdict; an Actions job''s carries `creator: null` and is re-derived, as since #742 is every account outside `H10_REVIEWERS` — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger''s `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The injected token stays write-capable until Gitea >=1.26 with a Restricted default (server-management#714), and a collaborator''s own token remains unfixable; the exemption path has its own separate defects in #698.' +rule: 'The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR''s own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch''s gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree''s scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class. Gitea injects `GITEA_TOKEN` into EVERY job and it USED to be write-capable everywhere, so any ref-resolved workflow could forge `review-verdict/h10`; since #748 (2026-08-27) every workflow here declares `permissions:` and five hold `code: read`, so no ref-resolved workflow that exists TODAY can post a status (the top-level declaration form those five use is INFERRED rather than probed — see `ci.actions-credential-scoping`). That closes the current instances, NOT the shape: a newly added workflow declaring write is a route again, which is why `test_pr_changed_files.py` asserts every tracked workflow declares a block. And a collaborator''s own API token still can, since branch protection binds the context and not its issuer. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI''s registry secret was the ADMIN account''s basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential''s route (an ALLOW-LISTED user credential''s forgery carries a matching `creator` and is inherited as a verdict; an Actions job''s carries `creator: null` and is re-derived, as since #742 is every account outside `H10_REVIEWERS` — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger''s `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The injected token stays write-capable until Gitea >=1.26 with a Restricted default (server-management#714), and a collaborator''s own token remains unfixable; the exemption path has its own separate defects in #698.' signals: 'workflow definition resolved from head, PR rewrites the gate that judges it, self-approve a required status check, pull_request_target vs pull_request, gate trigger branches filter, attacker-supplied base branch, how to test a change to review-verdict.yml, workflow not exercised by its own PR, gate edit goes live only on merge, required_approvals 0 does not bind an author, forged commit status inherited by sha · paths: `.gitea/workflows/review-verdict.yml`, `scripts/tests/test_pr_changed_files.py` · issues: #672, #663, #649, #622' mechanics: '`on: pull_request_target: {branches: [main], types: [opened, reopened, synchronize, ready_for_review, edited]}` (`edited` added by `ci.exemption-provenance` so a retarget reclassifies); asserted by `test_the_workflow_trigger_is_pull_request_TARGET_scoped_to_main` in `scripts/tests/test_pr_changed_files.py`; the job''s own context is renamed to `... (pull_request_target)` and must stay OUT of branch protection''s required list' --- @@ -54,7 +54,9 @@ inventory is not a short list. Gitea injects `GITEA_TOKEN` into **every** job, a default to read/**write**, so head-resolved, `push`-triggered and `workflow_dispatch` workflows alike were routes (1.24+ loads a dispatched definition from the selected branch). Narrowed since #748 (2026-08-27): every workflow in this repo now declares `permissions:` explicitly, and five hold -`code: read`, so none of them can POST a status. The route is closed for the workflows that exist +`code: read`, so none of them can POST a status (that rests on the TOP-LEVEL declaration form, +which is inferred rather than probed — only the job-level form was measured; see +`ci.actions-credential-scoping`). The route is closed for the workflows that exist TODAY, not for the shape — a new workflow that declares write, or omits a declaration and inherits the owner default, is a route again, which is why `test_pr_changed_files.py` asserts every tracked workflow declares a block. A collaborator's own API token is a -- 2.47.3 From 8d071d04ff2b5ade501eadea2d2c89d855d3af67 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 21:28:45 +0200 Subject: [PATCH 08/12] fix(748): the same field's TAIL still said to wait for a Gitea upgrade MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 5, and the last mirror the concept-sweep finds. Round 4 corrected the middle of `ci.gate-trigger-base-resolved`'s `rule:` and left its tail, so one YAML line said both "five hold `code: read` since #748, under today's permissive default" and, ~600 characters later, "the injected token stays write-capable until Gitea >=1.26 with a Restricted default". Doubly stale. The version half was already wrong independently of #748: the instance went 1.25.4 -> 1.27.1 on 2026-08-05, so ">=1.26" has been satisfied for three weeks, and `ci.actions-credential-scoping` has said so since that date. A reader of that tail was told to wait for an upgrade that had already happened. Replaced with the framing the rest of the corpus now uses: the Restricted default is a separate OPEN lever whose effect on a declared `code: write` is unmeasured, and it is not something to wait on a Gitea release for. Catalog regenerated; verified the stale clause is absent from both the record and `docs/decisions/README.md`, and the corrected one present in both. Lesson this PR keeps re-teaching, recorded because it cost five rounds: a "sweep" that greps the wording I just corrected finds only what I already fixed. Rounds 3, 4 and 5 each found the next instance one structural level out — body prose, then the `rule:` field the catalog mirrors, then the far end of that same field. Sweep by SUBJECT, and treat a long generated-from field as needing its own read end to end rather than a match count. Decisions-Edit: yes Refs: #748 --- docs/decisions/README.md | 2 +- docs/decisions/records/ci/gate-trigger-base-resolved.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/decisions/README.md b/docs/decisions/README.md index f1b443fb4..37ad52e37 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -49,7 +49,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.exemption-provenance` | The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR's live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR's immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires for a `success` only when it is POSITIVELY identified as an APPROVED REVIEWER's verdict for THIS base, meaning a `.creator.login` that is a member of the workflow's `H10_REVIEWERS` allow-list (an existing `failure` takes the weaker ATTRIBUTABLE test and needs no membership, because inheriting a rejection can only withhold an exemption while re-deriving one can turn it green on an exempt PR) — non-null was too weak, since any account's credential satisfies it and `RENOVATE_TOKEN` is one that cannot be scoped down (#742) — AND a `Review-verdict:` description AND, when that description records a base (`(base: …)`, `release.verdict-status-check`), a base matching the PR's — tested by requiring the description to END with the exact literal `(base: )` and to contain exactly ONE such marker, never by extracting a value (see below); a present-but-different base is rejected, an absent one is not, since verdicts predating that convention carry none; every other shape, including any unrecognised one, is re-derived rather than trusted. The bot and docs-only exemptions are evaluated as INDEPENDENT predicates and the decision made afterwards, never as an `elif` chain. `edited` is in the workflow's `types:` so a retarget reclassifies — which gives DETECTION, not atomicity: status writes are not serialized, so a stale run can still post over a fresher one. That residual is now FENCED rather than merely tracked (NARROWED, not resolved — a retarget between the fence's final count and the POST still leaves a PERMANENT forged green, #849) — the job refuses to write at all if the PR's timeline retarget COUNT moved while it was classifying (`ci.verdict-write-retarget-fence`, #706) — leaving TWO windows, not one: the sub-round-trip window that no API without compare-and-set can close, AND the post-final-count/pre-POST window, which is not sub-round-trip and yields a PERMANENT green because the successor can consume the `edited` event and exit before the stale run posts last (#849). The PROTECTED path list additionally covers `CLAUDE.md` and `AGENTS.md` (#751) — they are not prose but the documents DEFINING the completion protocol, the merge-consent convention and the H10 rule, so protecting `.claude/` while the file specifying what it enforces stayed docs-only-exempt was the same self-exemption one directory over; driving the real classify body with a lone `CLAUDE.md` change produced an exemption `success`. `README.md` is deliberately not listed. It also covers `.codex/` (#711), which mirrors `.claude/hooks/` byte for byte including the merge-consent hook — latent while that directory is untracked, live the moment it is tracked; the list stays ENUMERATIVE rather than derived, because a derived rule would have to be evaluated against the very file list being classified. Reading the CURRENT status for input (3) must tolerate `statuses: null`: `GET /commits/{sha}/status` serialises a nil slice as `null`, not `[]`, on a head with no statuses yet, and an `array`-only gate made `read_existing_verdict` `exit 1` and post nothing at all (#751, `ci.workflow-run-body-no-expressions`) — `null` is accepted only when `total_count` is 0, so a body that merely lost its array is still refused. Path predicates are evaluated by COUNTING with `grep -c`, never `\| grep -q` (SIGPIPE inversion) and never a here-string (temp-space failure) — see `ci.grep-q-pipefail-inversion`. | 2026-07-29 | [link](records/ci/exemption-provenance.md) | | `ci.format-gate-folder-mode` | The blocking `format` CI job (and matching pre-commit hook) runs `dotnet format whitespace . --folder --include ` instead of loading the full MSBuild/Roslyn solution, cutting the gate from ~480s to ~0.5s with unchanged whitespace/charset coverage. | 2026-07-19 | [link](records/ci/format-gate-folder-mode.md) | | `ci.functional-e2e-harness` | The `functional-e2e` CI job boots the PR's own code from source via `dotnet run` (`scripts/e2e-local.sh`) and runs deterministic assertions (`scripts/e2e-functional.sh`) as an advisory (non-blocking) job, not a `build` dependency or required check. Originally curl-only; since #445 the same job carries a second, headless-browser step for the contracts curl cannot express — see `ci.ui-e2e-harness`. | 2026-07-16 | [link](records/ci/functional-e2e-harness.md) | -| `ci.gate-trigger-base-resolved` | The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR's own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch's gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree's scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class. Gitea injects `GITEA_TOKEN` into EVERY job and it USED to be write-capable everywhere, so any ref-resolved workflow could forge `review-verdict/h10`; since #748 (2026-08-27) every workflow here declares `permissions:` and five hold `code: read`, so no ref-resolved workflow that exists TODAY can post a status (the top-level declaration form those five use is INFERRED rather than probed — see `ci.actions-credential-scoping`). That closes the current instances, NOT the shape: a newly added workflow declaring write is a route again, which is why `test_pr_changed_files.py` asserts every tracked workflow declares a block. And a collaborator's own API token still can, since branch protection binds the context and not its issuer. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI's registry secret was the ADMIN account's basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential's route (an ALLOW-LISTED user credential's forgery carries a matching `creator` and is inherited as a verdict; an Actions job's carries `creator: null` and is re-derived, as since #742 is every account outside `H10_REVIEWERS` — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger's `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The injected token stays write-capable until Gitea >=1.26 with a Restricted default (server-management#714), and a collaborator's own token remains unfixable; the exemption path has its own separate defects in #698. | 2026-07-28 | [link](records/ci/gate-trigger-base-resolved.md) | +| `ci.gate-trigger-base-resolved` | The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR's own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch's gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree's scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class. Gitea injects `GITEA_TOKEN` into EVERY job and it USED to be write-capable everywhere, so any ref-resolved workflow could forge `review-verdict/h10`; since #748 (2026-08-27) every workflow here declares `permissions:` and five hold `code: read`, so no ref-resolved workflow that exists TODAY can post a status (the top-level declaration form those five use is INFERRED rather than probed — see `ci.actions-credential-scoping`). That closes the current instances, NOT the shape: a newly added workflow declaring write is a route again, which is why `test_pr_changed_files.py` asserts every tracked workflow declares a block. And a collaborator's own API token still can, since branch protection binds the context and not its issuer. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI's registry secret was the ADMIN account's basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential's route (an ALLOW-LISTED user credential's forgery carries a matching `creator` and is inherited as a verdict; an Actions job's carries `creator: null` and is re-derived, as since #742 is every account outside `H10_REVIEWERS` — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger's `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The owner-level Restricted default (server-management#714) remains a separate OPEN lever — its effect on a declared `code: write` is unmeasured, and the version half of that condition has been satisfied since the 1.25.4 -> 1.27.1 upgrade on 2026-08-05, so it is not something to wait on a Gitea release for. A collaborator's own token remains unfixable; the exemption path has its own separate defects in #698. | 2026-07-28 | [link](records/ci/gate-trigger-base-resolved.md) | | `ci.gitea-milestone-filter-noop` | Never filter issues with the server-side `?milestones=` parameter — fetch all open issues once and filter LOCALLY on each issue's `.milestone.title`. | 2026-07-21 | [link](records/ci/gitea-milestone-filter-noop.md) | | `ci.grep-q-pipefail-inversion` | In any script running under `set -o pipefail`, a security or classification predicate of the form `producer \| grep -q…` is FORBIDDEN: `grep -q` exits at its first match, the producer then takes SIGPIPE and exits 141 once the data exceeds the pipe buffer (~64K), so `pipefail` reports the pipeline as FAILED even though grep MATCHED — inverting the predicate exactly when the input is large. A here-string (`grep -q… <<< "$data"`) is ALSO forbidden: bash materialises a large here-string via temporary storage, so it fails when temp space is full or unwritable, and inside an `if`/`!` that failure flips the predicate the same way. COUNT instead — `n=$(printf '%s\n' "$data" \| grep -cE "$re")` — because `grep -c` drains stdin (no early exit, no SIGPIPE) over an ordinary pipe (no temp file). Read grep's status honestly: exit 1 means a zero count and is a legitimate answer, anything >1 is a real error. Evaluate the counts ONCE at TOP LEVEL, never inline inside an `if`/`elif` condition: inside `$( )` an `exit` leaves only the subshell and `set -e` does not fire, so an error silently reads as "no match". Validate that each result is numeric and fail closed if not. This applies to both the enforced gate `.gitea/workflows/review-verdict.yml` and the advisory hook `.claude/hooks/pretooluse-merge-consent.sh`. | 2026-07-29 | [link](records/ci/grep-q-pipefail-inversion.md) | | `ci.infra-shaped-red-under-load` | When a job dies inside a setup/cache step before your code compiles, check the runner host's load before diagnosing the diff, and never file a CI bug off one sample under pressure. | 2026-07-21 | [link](records/ci/infra-shaped-red-under-load.md) | diff --git a/docs/decisions/records/ci/gate-trigger-base-resolved.md b/docs/decisions/records/ci/gate-trigger-base-resolved.md index ec647654b..850b567da 100644 --- a/docs/decisions/records/ci/gate-trigger-base-resolved.md +++ b/docs/decisions/records/ci/gate-trigger-base-resolved.md @@ -5,7 +5,7 @@ status: active since: '2026-07-28' supersedes: none superseded-by: none -rule: 'The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR''s own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch''s gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree''s scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class. Gitea injects `GITEA_TOKEN` into EVERY job and it USED to be write-capable everywhere, so any ref-resolved workflow could forge `review-verdict/h10`; since #748 (2026-08-27) every workflow here declares `permissions:` and five hold `code: read`, so no ref-resolved workflow that exists TODAY can post a status (the top-level declaration form those five use is INFERRED rather than probed — see `ci.actions-credential-scoping`). That closes the current instances, NOT the shape: a newly added workflow declaring write is a route again, which is why `test_pr_changed_files.py` asserts every tracked workflow declares a block. And a collaborator''s own API token still can, since branch protection binds the context and not its issuer. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI''s registry secret was the ADMIN account''s basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential''s route (an ALLOW-LISTED user credential''s forgery carries a matching `creator` and is inherited as a verdict; an Actions job''s carries `creator: null` and is re-derived, as since #742 is every account outside `H10_REVIEWERS` — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger''s `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The injected token stays write-capable until Gitea >=1.26 with a Restricted default (server-management#714), and a collaborator''s own token remains unfixable; the exemption path has its own separate defects in #698.' +rule: 'The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR''s own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch''s gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree''s scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class. Gitea injects `GITEA_TOKEN` into EVERY job and it USED to be write-capable everywhere, so any ref-resolved workflow could forge `review-verdict/h10`; since #748 (2026-08-27) every workflow here declares `permissions:` and five hold `code: read`, so no ref-resolved workflow that exists TODAY can post a status (the top-level declaration form those five use is INFERRED rather than probed — see `ci.actions-credential-scoping`). That closes the current instances, NOT the shape: a newly added workflow declaring write is a route again, which is why `test_pr_changed_files.py` asserts every tracked workflow declares a block. And a collaborator''s own API token still can, since branch protection binds the context and not its issuer. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI''s registry secret was the ADMIN account''s basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential''s route (an ALLOW-LISTED user credential''s forgery carries a matching `creator` and is inherited as a verdict; an Actions job''s carries `creator: null` and is re-derived, as since #742 is every account outside `H10_REVIEWERS` — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger''s `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The owner-level Restricted default (server-management#714) remains a separate OPEN lever — its effect on a declared `code: write` is unmeasured, and the version half of that condition has been satisfied since the 1.25.4 -> 1.27.1 upgrade on 2026-08-05, so it is not something to wait on a Gitea release for. A collaborator''s own token remains unfixable; the exemption path has its own separate defects in #698.' signals: 'workflow definition resolved from head, PR rewrites the gate that judges it, self-approve a required status check, pull_request_target vs pull_request, gate trigger branches filter, attacker-supplied base branch, how to test a change to review-verdict.yml, workflow not exercised by its own PR, gate edit goes live only on merge, required_approvals 0 does not bind an author, forged commit status inherited by sha · paths: `.gitea/workflows/review-verdict.yml`, `scripts/tests/test_pr_changed_files.py` · issues: #672, #663, #649, #622' mechanics: '`on: pull_request_target: {branches: [main], types: [opened, reopened, synchronize, ready_for_review, edited]}` (`edited` added by `ci.exemption-provenance` so a retarget reclassifies); asserted by `test_the_workflow_trigger_is_pull_request_TARGET_scoped_to_main` in `scripts/tests/test_pr_changed_files.py`; the job''s own context is renamed to `... (pull_request_target)` and must stay OUT of branch protection''s required list' --- -- 2.47.3 From 548e058ea5757bd08b07473e80bea4c8ea376346 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 21:44:57 +0200 Subject: [PATCH 09/12] docs(748): align the persist-credentials clause with its own record body MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last item from the review, taken rather than deferred: it was graded non-blocking (generically true, and wrong in the CONSERVATIVE direction — it overstates a credential's capability, so nobody is told something is safe that isn't), but it is the same body-vs-`rule:` split as rounds 4 and 5, in the record this PR is centred on, and it is one clause in a file already open. `ci.actions-credential-scoping`'s `rule:` said `actions/checkout` "persists a write-capable credential"; its body two paragraphs down, corrected in round 2, says that since #748 the one persisted credential is read-only by declaration. The rule now says the same, and the catalog carries it. Decisions-Edit: yes Refs: #748 --- docs/decisions/README.md | 2 +- docs/decisions/records/ci/actions-credential-scoping.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 37ad52e37..93e2b3924 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -38,7 +38,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `blazor.rollback-tag` | The commit immediately preceding the Blazor-removal merge is tagged `blazor-final` (not a `v*` tag, so it doesn't trigger a prod release build) as the documented rollback/restore path. | 2026-07-11 | [link](records/blazor/rollback-tag.md) | | `blazor.ui-removed` | The legacy Blazor Server UI (`Pages/`, `Shared/`, `ViewModels/`, `Validators/`, MudBlazor + 8 other packages, Blazor Startup wiring) is fully deleted now that the SPA has parity; the legacy `MapWhen` branch is kept only for controllers/docs/OpenAPI/`LegacyUiRedirects`, and the catch-all fallback 302s any unmatched non-api/artwork/docs/openapi path to `/app`. | 2026-07-11 | [link](records/blazor/ui-removed.md) | | `channel.origin-marker` | A new `Channel.Origin` (`ChannelOrigin` enum — `Unknown`/`UserCreated`/`AutoTuned`) records how a channel row was created and is stamped exactly once at insert (`AutoTuned` in `CreateChannelFromLineupHandler`, `UserCreated` in `CreateChannelHandler`), and is never mutated on a later edit. It is surfaced as a raw `origin` field on `ChannelResponseModel`; the SPA badges only `AutoTuned`. Rows predating the column read `Unknown` — provenance is **not** back-filled. | 2026-07-23 | [link](records/channel/origin-marker.md) | -| `ci.actions-credential-scoping` | Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. `permissions:` on a workflow/job DOES bind on this instance — MEASURED 2026-08-27 on 1.27.1 by matched scratch-base probe PRs differing only in one unit (`code: write` posted the probe status; `code: read` 403ed the POST, curl exit 22, no status written), so every workflow here now declares it (#748). Two properties that make it usable: the declaration is EXHAUSTIVE, not additive — a unit omitted is not granted — and it binds while the owner-level default is `permissive`, which is what makes the five `code: read` declarations effective TODAY rather than only after a flip. NOT established: what a declared `code: write` does under a RESTRICTED default. GitHub semantics let `permissions:` only narrow, never widen past the default, and if Gitea copies that, Restricted would cap the gate job at read and `review-verdict/h10` would stop being writable — the exact catastrophic case. The probe ran under `permissive` and therefore CANNOT rule that out. Flip the owner default only behind the scratch-base probe re-run under Restricted (server-management#714, still open for this reason). The earlier form of this rule said the opposite ("do NOT add a `permissions:` key on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP"); that was correct at 1.25.4 and is retained here only so a reader meeting the old advice recognises it as superseded rather than contradicted. There is still no API surface for the owner-level default (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists a write-capable credential into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo's 16 checkouts now set it (#746), `ci-image.yml`'s being excluded for the reason in that section. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation. | 2026-08-05 | [link](records/ci/actions-credential-scoping.md) | +| `ci.actions-credential-scoping` | Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. `permissions:` on a workflow/job DOES bind on this instance — MEASURED 2026-08-27 on 1.27.1 by matched scratch-base probe PRs differing only in one unit (`code: write` posted the probe status; `code: read` 403ed the POST, curl exit 22, no status written), so every workflow here now declares it (#748). Two properties that make it usable: the declaration is EXHAUSTIVE, not additive — a unit omitted is not granted — and it binds while the owner-level default is `permissive`, which is what makes the five `code: read` declarations effective TODAY rather than only after a flip. NOT established: what a declared `code: write` does under a RESTRICTED default. GitHub semantics let `permissions:` only narrow, never widen past the default, and if Gitea copies that, Restricted would cap the gate job at read and `review-verdict/h10` would stop being writable — the exact catastrophic case. The probe ran under `permissive` and therefore CANNOT rule that out. Flip the owner default only behind the scratch-base probe re-run under Restricted (server-management#714, still open for this reason). The earlier form of this rule said the opposite ("do NOT add a `permissions:` key on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP"); that was correct at 1.25.4 and is retained here only so a reader meeting the old advice recognises it as superseded rather than contradicted. There is still no API surface for the owner-level default (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists whatever the job token can do into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo's 16 checkouts now set it (#746), `ci-image.yml`'s being excluded for the reason in that section — since #748 that one persisted credential is read-only by DECLARATION (`ci-image.yml` declares `code: read`) rather than by instance default. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation. | 2026-08-05 | [link](records/ci/actions-credential-scoping.md) | | `ci.batch-pushes-no-cancel-route` | Hold review fixes, doc corrections and format fixes locally and push **once** — a superseded run cannot be cancelled from the agent side and holds a runner slot until it finishes. | 2026-07-21 | [link](records/ci/batch-pushes-no-cancel-route.md) | | `ci.build-once-rejected` | CI build-once (a shared compile artifact across jobs) was implemented, measured, and rejected for a 40-85% wall-clock regression; keep the #420 cross-run tree-identity skip instead. | 2026-07-18 | [link](records/ci/build-once-rejected.md) | | `ci.cancelled-is-not-a-verdict` | Treat a `cancelled` conclusion as "no verdict" — never as pass or fail — and report FAILED and CANCELLED counts separately in any CI monitor. THE COMBINED COMMIT-STATUS ENDPOINT CANNOT EXPRESS THIS: `GET /repos/{o}/{r}/commits/{sha}/status` has states `success`/`failure`/`pending`/`error` and NO `cancelled`, so it reports a cancelled job as `failure`. Anything polling that endpoint — which is what a CI monitor naturally polls, because it is the per-sha view the merge gate reads — must resolve the job-level `conclusion` via `actions/runs/{id}/jobs` before reporting a red. | 2026-07-21 | [link](records/ci/cancelled-is-not-a-verdict.md) | diff --git a/docs/decisions/records/ci/actions-credential-scoping.md b/docs/decisions/records/ci/actions-credential-scoping.md index 85b1c0166..7b1d41dd8 100644 --- a/docs/decisions/records/ci/actions-credential-scoping.md +++ b/docs/decisions/records/ci/actions-credential-scoping.md @@ -5,7 +5,7 @@ status: active since: '2026-08-05' supersedes: none superseded-by: none -rule: 'Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. `permissions:` on a workflow/job DOES bind on this instance — MEASURED 2026-08-27 on 1.27.1 by matched scratch-base probe PRs differing only in one unit (`code: write` posted the probe status; `code: read` 403ed the POST, curl exit 22, no status written), so every workflow here now declares it (#748). Two properties that make it usable: the declaration is EXHAUSTIVE, not additive — a unit omitted is not granted — and it binds while the owner-level default is `permissive`, which is what makes the five `code: read` declarations effective TODAY rather than only after a flip. NOT established: what a declared `code: write` does under a RESTRICTED default. GitHub semantics let `permissions:` only narrow, never widen past the default, and if Gitea copies that, Restricted would cap the gate job at read and `review-verdict/h10` would stop being writable — the exact catastrophic case. The probe ran under `permissive` and therefore CANNOT rule that out. Flip the owner default only behind the scratch-base probe re-run under Restricted (server-management#714, still open for this reason). The earlier form of this rule said the opposite ("do NOT add a `permissions:` key on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP"); that was correct at 1.25.4 and is retained here only so a reader meeting the old advice recognises it as superseded rather than contradicted. There is still no API surface for the owner-level default (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists a write-capable credential into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo''s 16 checkouts now set it (#746), `ci-image.yml`''s being excluded for the reason in that section. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation.' +rule: 'Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. `permissions:` on a workflow/job DOES bind on this instance — MEASURED 2026-08-27 on 1.27.1 by matched scratch-base probe PRs differing only in one unit (`code: write` posted the probe status; `code: read` 403ed the POST, curl exit 22, no status written), so every workflow here now declares it (#748). Two properties that make it usable: the declaration is EXHAUSTIVE, not additive — a unit omitted is not granted — and it binds while the owner-level default is `permissive`, which is what makes the five `code: read` declarations effective TODAY rather than only after a flip. NOT established: what a declared `code: write` does under a RESTRICTED default. GitHub semantics let `permissions:` only narrow, never widen past the default, and if Gitea copies that, Restricted would cap the gate job at read and `review-verdict/h10` would stop being writable — the exact catastrophic case. The probe ran under `permissive` and therefore CANNOT rule that out. Flip the owner default only behind the scratch-base probe re-run under Restricted (server-management#714, still open for this reason). The earlier form of this rule said the opposite ("do NOT add a `permissions:` key on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP"); that was correct at 1.25.4 and is retained here only so a reader meeting the old advice recognises it as superseded rather than contradicted. There is still no API surface for the owner-level default (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists whatever the job token can do into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo''s 16 checkouts now set it (#746), `ci-image.yml`''s being excluded for the reason in that section — since #748 that one persisted credential is read-only by DECLARATION (`ci-image.yml` declares `code: read`) rather than by instance default. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation.' signals: 'admin password in CI secrets, registry credential scope, ETV_STATUS_AUTH can write statuses, forge review-verdict/h10, head-resolved workflow holds credentials, persist-credentials on actions/checkout, credential left in .git/config, masked git fetch yields empty changed set, Gitea token scopes, no status scope, write:package vs write:repository, permissions key no-op, GITEA_TOKEN default read/write, Restricted default token permissions, orphan secret, deploy key in secret store, toolchain image overwrite, prod floating tag write · paths: `.gitea/workflows/docker-build.yml`, `.gitea/workflows/pr-checks.yml`, `.gitea/workflows/dependency-scan.yml`, `.gitea/workflows/ci-image.yml`, `.gitea/workflows/renovate.yml`, `scripts/ci-detect-already-validated.sh` · issues: #697, #672, #698, #742, #743, #746, #748, #835, #420, server-management#714' mechanics: 'PAT `ci-registry-scoped-697`, scopes `write:package,read:repository`, stored as repo Actions secret `REGISTRY_PASSWORD`; `REGISTRY_USER` remains `timothy`. Verified 2026-08-05 on Gitea 1.25.4: registry push of a probe tag SUCCEEDED; `GET /commits/{sha}/status` 200; `POST /statuses/{sha}` REFUSED HTTP 403 `token does not have at least one of required scope(s), required=[write:repository], token scope=write:package,read:repository`. Probe artifacts deleted, confirmed 404. NOT measured with this token: the `container:` pull, the buildcache write and the base-image pull. Those rest on Gitea''s scope model (write implies read per category, read at tag `v1.25.4`) — INFERRED. Note WHICH run proves which: only the `container:` pull is exercised by a PR. `cache-to`/`cache-from` and the base-image pull are confined to the `build` job, which carries `if: github.event_name != ''pull_request''`, so they are first exercised on the post-merge push to `main` — AFTER the merge gate has passed. A wrong inference there reddens main, not the PR.' --- -- 2.47.3 From 5fe4af5708e3f987ba7c8f6c1b041a69b6ea4457 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 22:16:55 +0200 Subject: [PATCH 10/12] =?UTF-8?q?fix(748):=20drop=20ci-image.yml=20?= =?UTF-8?q?=E2=80=94=20it=20rides=20with=20#744,=20and=20CI=20proved=20why?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ci-image-pin` (a BLOCKING job) went red on the PR head, exactly as `ci.actions-credential-scoping` predicted: its `expected` is `git log -1 -- docker/ci .gitea/workflows/ci-image.yml`, so a PR that EDITS `ci-image.yml` re-points `expected` at its own commit and the existing pin reads as stale. I had read that warning and bundled the file anyway. Two further reasons not to pay the toll instead: * clearing it means letting ci-image.yml publish a toolchain image from this branch, then bumping all five pins — and a rebase recharges it. That is why `land-toolchain-image-change-separately` exists. * `fix/744-ci-image-branch-filter` is ACTIVE in another session right now (queued workflow_dispatch run 2340), and #744 exists precisely to change this file alone. Editing it here would collide. Verified my push did NOT publish an image: no ci-image run from this branch. So `ci-image.yml` is reverted to `origin/main` and the guard carries a ONE-NAME literal exemption with the reason and the tracking issue — the same exemption #746 already carries for that file's `persist-credentials`, and ersatztv#835 already tracks deleting BOTH once #744 lands. Mutation-checked that the exemption does not blunt the guard: dropping `renovate.yml`'s block still reds. THE CLAIM THAT HAD TO CHANGE, not just the counts. I had written in `ci.gate-trigger-base-resolved` that "no ref-resolved workflow that exists TODAY can post a status". With ci-image.yml excluded that is FALSE, and falsest in the worst place: it declares nothing, so it keeps the owner default, AND its unfiltered `push:` is path-scoped to itself, so any branch push runs its head-resolved YAML with a status-capable token. Both the record's `rule:` and its body now say ci-image.yml is the exception and is the worst-placed of the six, with #744 named as the issue that closes the route — scoping would not have closed it anyway, since scoping bounds what a job may DO and only a filter bounds whether it RUNS. Swept every dependent count and claim rather than the ones I remembered: six->five declaring, five->four holding `code: read`, and the reverted assertion that ci-image.yml's persisted checkout credential is read-only by declaration — it is not; it is still write-capable on the owner default, in `docs/ci-cd.md`, `ci.actions-credential-scoping` (body AND `rule:`), `release.verdict-status-check` and `release.main-direct-push-disabled`. New "carve-out" subsection in docs/ci-cd.md states it as a scheduling constraint, explicitly not a safety judgement. Decisions-Edit: yes Refs: #748 #744 #835 --- .gitea/workflows/ci-image.yml | 16 ------- docs/ci-cd.md | 44 ++++++++++++++----- docs/decisions/README.md | 4 +- .../records/ci/actions-credential-scoping.md | 11 ++--- .../records/ci/gate-trigger-base-resolved.md | 14 +++--- .../release/main-direct-push-disabled.md | 3 +- .../records/release/verdict-status-check.md | 3 +- docs/guard-inventory.md | 2 +- scripts/tests/test_pr_changed_files.py | 17 ++++++- 9 files changed, 72 insertions(+), 42 deletions(-) diff --git a/.gitea/workflows/ci-image.yml b/.gitea/workflows/ci-image.yml index 47b8a6664..6c3be2e09 100644 --- a/.gitea/workflows/ci-image.yml +++ b/.gitea/workflows/ci-image.yml @@ -50,22 +50,6 @@ env: REGISTRY: 192.168.1.95:3000 CI_IMAGE: 192.168.1.95:3000/timothy/ersatztv-ci -# Explicit token scope (ersatztv#748) so the owner-level Actions default can move to Restricted -# (server-management#714). Declaring `permissions:` is EXHAUSTIVE, not additive: a unit omitted here -# is NOT granted, and that holds at any owner default — it is not conditional on Restricted being on. -# Only `review-verdict.yml` needs write; it declares that at the job and says why there. Full -# rationale and the per-workflow credential audit: docs/ci-cd.md -> "Workflow token scope". -# This workflow's registry pushes authenticate with the scoped REGISTRY_* PAT -# (`ci.actions-credential-scoping`), so the injected GITEA_TOKEN serves only its single -# `actions/checkout` — the repo's one checkout that still persists its credential into .git/config -# (ersatztv#746 excluded it deliberately). `code: read` is therefore what makes that persisted -# credential read-only. Note that is a DIFFERENT and WEAKER property than the other workflows have: -# the four that check out all set `persist-credentials: false` and so persist nothing, and -# `renovate.yml` persists nothing because it never checks out. Read-only-persisted and not-persisted -# are not interchangeable. -permissions: - code: read - jobs: build: name: Build & push CI image diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 59402c480..4061f9e1f 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -149,7 +149,8 @@ A backup, fetch, or migration-smoke failure aborts before the live container is ## Workflow token scope (`permissions:`, ersatztv#748) -Every workflow in this repo declares `permissions:`. Five declare `code: read` at the top level; +Every workflow in this repo declares `permissions:` **except `ci-image.yml`** (carve-out below). +Four declare `code: read` at the top level; `review-verdict.yml` declares `code: write` + `issues: read` + `pull-requests: read` on its `set-verdict-status` job and nothing at the top level. That is the whole convention — a new workflow should declare the narrowest set it needs, and say at the declaration why it needs write if it does. @@ -202,7 +203,7 @@ job level**, on `set-verdict-status` — the same level and the same job as the `review-verdict.yml`, so the gate's own shape is what was measured. Both ran while the owner default was still `permissive`, so the binding holds **today**, without any -flip. One scope limit, stated rather than glossed: the other five workflows declare `permissions:` at +flip. One scope limit, stated rather than glossed: the other four declaring workflows use `permissions:` at **top level**, a form the probe did not exercise. That is inferred, not measured — but the failure direction is safe (a top-level key Gitea ignored would be a security no-op, never a brick), and `docker-build.yml` and `pr-checks.yml` are `on: pull_request`, so this change's own CI exercises @@ -231,17 +232,37 @@ holding write implicitly. Real, and weaker than closed. editing PR — that workflow is base-resolved and is never exercised by its own PR, so its own checks say nothing about the edit. See the probe recipe in the review-verdict gate section below. -The other five split two ways, and only one half is covered by an editing PR's own CI: +The other four that declare a block split two ways, and only one half is covered by an editing PR's +own CI: - `docker-build.yml` and `pr-checks.yml` run on `pull_request`, so an editing PR exercises their blocks directly. (`docker-build.yml`'s `build` job carries `if: github.event_name != 'pull_request'`, so its registry-push path is first exercised on the post-merge push to `main` — that reddens `main`, not the gate, and is recoverable.) -- `ci-image.yml` (`push`, path-filtered to `docker/ci/**` and its own file), `dependency-scan.yml` and `renovate.yml` - (`schedule` / `workflow_dispatch` only) are **not** exercised by an editing PR at all — their first - run is after merge or on the next schedule. None is a required check, so a wrong block there - reddens a non-blocking run rather than the gate, but do not read an editing PR's green as covering - them. +- `dependency-scan.yml` and `renovate.yml` (`schedule` / `workflow_dispatch` only) are **not** + exercised by an editing PR at all — their first run is on the next schedule. Neither is a required + check, so a wrong block there reddens a non-blocking run rather than the gate, but do not read an + editing PR's green as covering them. + +### The `ci-image.yml` carve-out — a scheduling constraint, not a safety judgement + +`ci-image.yml` declares **no** `permissions:` block, so it still receives the owner-level default and +its token is write-capable, as is the credential its checkout persists (it is also the one checkout +that does not set `persist-credentials: false`, ersatztv#746). That is the *worst*-placed of the six, +not the safest: its `push:` trigger is unfiltered and path-scoped to itself, so any branch push runs +its head-resolved YAML with a status-capable token. + +It is excluded for a scheduling reason. A PR editing that file re-points `ci-image-pin`'s `expected` +at the editing commit and staleness-fails a **blocking** job, and the same edit would publish a +toolchain image from the editing branch. Both are why +`release.format-as-you-touch-rebase`'s sibling rule `land-toolchain-image-change-separately` says the +file lands alone. It rides with **ersatztv#744**, which is also the issue that closes the trigger +route — scoping this file's token would not have closed it anyway, because scoping bounds what a job +may *do* and only a filter bounds whether it *runs*. + +`scripts/tests/test_pr_changed_files.py` carries this as a one-name literal exemption with the same +reference. **ersatztv#835 tracks deleting that exemption — and #746's matching one — once #744 has +landed.** Adding a second name to it is a finding, not a fix. ## Checkouts drop the persisted credential (ersatztv#746) @@ -252,7 +273,9 @@ to the checkout. That header is write-capable *only while the job's token is*: t default token permission is `permissive`, where it stands after a Restricted trial was reverted on 2026-08-05. Since ersatztv#748 that default no longer governs this repo's jobs — every workflow declares `permissions:` explicitly (see the section above), so `ci-image.yml`'s persisted header is -read-only by declaration rather than by instance setting. Re-applying the owner-level default is +read-only by declaration rather than by instance setting — EXCEPT `ci-image.yml`, which declares +nothing and so still receives the owner default; its persisted header stays write-capable, and that +is the one this section's carve-out is about. Re-applying the owner-level default is server-management#714. No job that checks out loses anything: none of them push with git, and anonymous access covers the fetches because the repo is public — `private: false`, and `GET /info/refs?service=git-upload-pack` answers unauthenticated, @@ -1545,7 +1568,8 @@ as establishing that the gate cannot be forged (see the residual below, and ersa endpoint refuses (`ci.actions-credential-scoping`, ersatztv#697). The inventory was never that one workflow, though, and branch protection binds the *context*, not its issuer. Gitea used to inject a write-capable `GITEA_TOKEN` into every job; since ersatztv#748 every workflow here declares - `permissions:` explicitly, so five hold `code: read` and only `review-verdict.yml`'s gate job holds + `permissions:` explicitly except `ci-image.yml`, so four hold `code: read` and only + `review-verdict.yml`'s gate job holds write — see "Workflow token scope" above, which is the current statement of this. That narrows the inventory; it does not close the class — not against a personal token, and not against `RENOVATE_TOKEN` (ersatztv#742). The owner-level **Restricted** default (server-management#714) is diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 93e2b3924..da0a1c710 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -38,7 +38,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `blazor.rollback-tag` | The commit immediately preceding the Blazor-removal merge is tagged `blazor-final` (not a `v*` tag, so it doesn't trigger a prod release build) as the documented rollback/restore path. | 2026-07-11 | [link](records/blazor/rollback-tag.md) | | `blazor.ui-removed` | The legacy Blazor Server UI (`Pages/`, `Shared/`, `ViewModels/`, `Validators/`, MudBlazor + 8 other packages, Blazor Startup wiring) is fully deleted now that the SPA has parity; the legacy `MapWhen` branch is kept only for controllers/docs/OpenAPI/`LegacyUiRedirects`, and the catch-all fallback 302s any unmatched non-api/artwork/docs/openapi path to `/app`. | 2026-07-11 | [link](records/blazor/ui-removed.md) | | `channel.origin-marker` | A new `Channel.Origin` (`ChannelOrigin` enum — `Unknown`/`UserCreated`/`AutoTuned`) records how a channel row was created and is stamped exactly once at insert (`AutoTuned` in `CreateChannelFromLineupHandler`, `UserCreated` in `CreateChannelHandler`), and is never mutated on a later edit. It is surfaced as a raw `origin` field on `ChannelResponseModel`; the SPA badges only `AutoTuned`. Rows predating the column read `Unknown` — provenance is **not** back-filled. | 2026-07-23 | [link](records/channel/origin-marker.md) | -| `ci.actions-credential-scoping` | Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. `permissions:` on a workflow/job DOES bind on this instance — MEASURED 2026-08-27 on 1.27.1 by matched scratch-base probe PRs differing only in one unit (`code: write` posted the probe status; `code: read` 403ed the POST, curl exit 22, no status written), so every workflow here now declares it (#748). Two properties that make it usable: the declaration is EXHAUSTIVE, not additive — a unit omitted is not granted — and it binds while the owner-level default is `permissive`, which is what makes the five `code: read` declarations effective TODAY rather than only after a flip. NOT established: what a declared `code: write` does under a RESTRICTED default. GitHub semantics let `permissions:` only narrow, never widen past the default, and if Gitea copies that, Restricted would cap the gate job at read and `review-verdict/h10` would stop being writable — the exact catastrophic case. The probe ran under `permissive` and therefore CANNOT rule that out. Flip the owner default only behind the scratch-base probe re-run under Restricted (server-management#714, still open for this reason). The earlier form of this rule said the opposite ("do NOT add a `permissions:` key on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP"); that was correct at 1.25.4 and is retained here only so a reader meeting the old advice recognises it as superseded rather than contradicted. There is still no API surface for the owner-level default (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists whatever the job token can do into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo's 16 checkouts now set it (#746), `ci-image.yml`'s being excluded for the reason in that section — since #748 that one persisted credential is read-only by DECLARATION (`ci-image.yml` declares `code: read`) rather than by instance default. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation. | 2026-08-05 | [link](records/ci/actions-credential-scoping.md) | +| `ci.actions-credential-scoping` | Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. `permissions:` on a workflow/job DOES bind on this instance — MEASURED 2026-08-27 on 1.27.1 by matched scratch-base probe PRs differing only in one unit (`code: write` posted the probe status; `code: read` 403ed the POST, curl exit 22, no status written), so five of the six workflows here now declare it (#748); `ci-image.yml` is excluded and rides with #744, leaving it on the owner default. Two properties that make it usable: the declaration is EXHAUSTIVE, not additive — a unit omitted is not granted — and it binds while the owner-level default is `permissive`, which is what makes the five `code: read` declarations effective TODAY rather than only after a flip. NOT established: what a declared `code: write` does under a RESTRICTED default. GitHub semantics let `permissions:` only narrow, never widen past the default, and if Gitea copies that, Restricted would cap the gate job at read and `review-verdict/h10` would stop being writable — the exact catastrophic case. The probe ran under `permissive` and therefore CANNOT rule that out. Flip the owner default only behind the scratch-base probe re-run under Restricted (server-management#714, still open for this reason). The earlier form of this rule said the opposite ("do NOT add a `permissions:` key on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP"); that was correct at 1.25.4 and is retained here only so a reader meeting the old advice recognises it as superseded rather than contradicted. There is still no API surface for the owner-level default (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists whatever the job token can do into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo's 16 checkouts now set it (#746), `ci-image.yml`'s being excluded for the reason in that section. #748 did NOT change that: it excluded `ci-image.yml` for the same scheduling reason, so that one persisted credential is still write-capable on the owner default. Both exclusions clear together when #744 lands. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation. | 2026-08-05 | [link](records/ci/actions-credential-scoping.md) | | `ci.batch-pushes-no-cancel-route` | Hold review fixes, doc corrections and format fixes locally and push **once** — a superseded run cannot be cancelled from the agent side and holds a runner slot until it finishes. | 2026-07-21 | [link](records/ci/batch-pushes-no-cancel-route.md) | | `ci.build-once-rejected` | CI build-once (a shared compile artifact across jobs) was implemented, measured, and rejected for a 40-85% wall-clock regression; keep the #420 cross-run tree-identity skip instead. | 2026-07-18 | [link](records/ci/build-once-rejected.md) | | `ci.cancelled-is-not-a-verdict` | Treat a `cancelled` conclusion as "no verdict" — never as pass or fail — and report FAILED and CANCELLED counts separately in any CI monitor. THE COMBINED COMMIT-STATUS ENDPOINT CANNOT EXPRESS THIS: `GET /repos/{o}/{r}/commits/{sha}/status` has states `success`/`failure`/`pending`/`error` and NO `cancelled`, so it reports a cancelled job as `failure`. Anything polling that endpoint — which is what a CI monitor naturally polls, because it is the per-sha view the merge gate reads — must resolve the job-level `conclusion` via `actions/runs/{id}/jobs` before reporting a red. | 2026-07-21 | [link](records/ci/cancelled-is-not-a-verdict.md) | @@ -49,7 +49,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.exemption-provenance` | The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR's live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR's immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires for a `success` only when it is POSITIVELY identified as an APPROVED REVIEWER's verdict for THIS base, meaning a `.creator.login` that is a member of the workflow's `H10_REVIEWERS` allow-list (an existing `failure` takes the weaker ATTRIBUTABLE test and needs no membership, because inheriting a rejection can only withhold an exemption while re-deriving one can turn it green on an exempt PR) — non-null was too weak, since any account's credential satisfies it and `RENOVATE_TOKEN` is one that cannot be scoped down (#742) — AND a `Review-verdict:` description AND, when that description records a base (`(base: …)`, `release.verdict-status-check`), a base matching the PR's — tested by requiring the description to END with the exact literal `(base: )` and to contain exactly ONE such marker, never by extracting a value (see below); a present-but-different base is rejected, an absent one is not, since verdicts predating that convention carry none; every other shape, including any unrecognised one, is re-derived rather than trusted. The bot and docs-only exemptions are evaluated as INDEPENDENT predicates and the decision made afterwards, never as an `elif` chain. `edited` is in the workflow's `types:` so a retarget reclassifies — which gives DETECTION, not atomicity: status writes are not serialized, so a stale run can still post over a fresher one. That residual is now FENCED rather than merely tracked (NARROWED, not resolved — a retarget between the fence's final count and the POST still leaves a PERMANENT forged green, #849) — the job refuses to write at all if the PR's timeline retarget COUNT moved while it was classifying (`ci.verdict-write-retarget-fence`, #706) — leaving TWO windows, not one: the sub-round-trip window that no API without compare-and-set can close, AND the post-final-count/pre-POST window, which is not sub-round-trip and yields a PERMANENT green because the successor can consume the `edited` event and exit before the stale run posts last (#849). The PROTECTED path list additionally covers `CLAUDE.md` and `AGENTS.md` (#751) — they are not prose but the documents DEFINING the completion protocol, the merge-consent convention and the H10 rule, so protecting `.claude/` while the file specifying what it enforces stayed docs-only-exempt was the same self-exemption one directory over; driving the real classify body with a lone `CLAUDE.md` change produced an exemption `success`. `README.md` is deliberately not listed. It also covers `.codex/` (#711), which mirrors `.claude/hooks/` byte for byte including the merge-consent hook — latent while that directory is untracked, live the moment it is tracked; the list stays ENUMERATIVE rather than derived, because a derived rule would have to be evaluated against the very file list being classified. Reading the CURRENT status for input (3) must tolerate `statuses: null`: `GET /commits/{sha}/status` serialises a nil slice as `null`, not `[]`, on a head with no statuses yet, and an `array`-only gate made `read_existing_verdict` `exit 1` and post nothing at all (#751, `ci.workflow-run-body-no-expressions`) — `null` is accepted only when `total_count` is 0, so a body that merely lost its array is still refused. Path predicates are evaluated by COUNTING with `grep -c`, never `\| grep -q` (SIGPIPE inversion) and never a here-string (temp-space failure) — see `ci.grep-q-pipefail-inversion`. | 2026-07-29 | [link](records/ci/exemption-provenance.md) | | `ci.format-gate-folder-mode` | The blocking `format` CI job (and matching pre-commit hook) runs `dotnet format whitespace . --folder --include ` instead of loading the full MSBuild/Roslyn solution, cutting the gate from ~480s to ~0.5s with unchanged whitespace/charset coverage. | 2026-07-19 | [link](records/ci/format-gate-folder-mode.md) | | `ci.functional-e2e-harness` | The `functional-e2e` CI job boots the PR's own code from source via `dotnet run` (`scripts/e2e-local.sh`) and runs deterministic assertions (`scripts/e2e-functional.sh`) as an advisory (non-blocking) job, not a `build` dependency or required check. Originally curl-only; since #445 the same job carries a second, headless-browser step for the contracts curl cannot express — see `ci.ui-e2e-harness`. | 2026-07-16 | [link](records/ci/functional-e2e-harness.md) | -| `ci.gate-trigger-base-resolved` | The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR's own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch's gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree's scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class. Gitea injects `GITEA_TOKEN` into EVERY job and it USED to be write-capable everywhere, so any ref-resolved workflow could forge `review-verdict/h10`; since #748 (2026-08-27) every workflow here declares `permissions:` and five hold `code: read`, so no ref-resolved workflow that exists TODAY can post a status (the top-level declaration form those five use is INFERRED rather than probed — see `ci.actions-credential-scoping`). That closes the current instances, NOT the shape: a newly added workflow declaring write is a route again, which is why `test_pr_changed_files.py` asserts every tracked workflow declares a block. And a collaborator's own API token still can, since branch protection binds the context and not its issuer. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI's registry secret was the ADMIN account's basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential's route (an ALLOW-LISTED user credential's forgery carries a matching `creator` and is inherited as a verdict; an Actions job's carries `creator: null` and is re-derived, as since #742 is every account outside `H10_REVIEWERS` — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger's `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The owner-level Restricted default (server-management#714) remains a separate OPEN lever — its effect on a declared `code: write` is unmeasured, and the version half of that condition has been satisfied since the 1.25.4 -> 1.27.1 upgrade on 2026-08-05, so it is not something to wait on a Gitea release for. A collaborator's own token remains unfixable; the exemption path has its own separate defects in #698. | 2026-07-28 | [link](records/ci/gate-trigger-base-resolved.md) | +| `ci.gate-trigger-base-resolved` | The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR's own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch's gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree's scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class. Gitea injects `GITEA_TOKEN` into EVERY job and it USED to be write-capable everywhere, so any ref-resolved workflow could forge `review-verdict/h10`; since #748 (2026-08-27) five of the six workflows here declare `permissions:` and four hold `code: read`, so those cannot post a status (the top-level declaration form they use is INFERRED rather than probed — see `ci.actions-credential-scoping`). `ci-image.yml` is the EXCEPTION and it is exactly the dangerous shape: it declares nothing, so it still receives the owner default (write-capable), and its unfiltered `push:` trigger is path-scoped to itself, so any branch push runs its head-resolved YAML with a status-capable token. #748 deliberately did not touch it — editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job, so it rides with #744, which is the issue that closes this route. That closes the current instances, NOT the shape: a newly added workflow declaring write is a route again, which is why `test_pr_changed_files.py` asserts every tracked workflow declares a block. And a collaborator's own API token still can, since branch protection binds the context and not its issuer. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI's registry secret was the ADMIN account's basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential's route (an ALLOW-LISTED user credential's forgery carries a matching `creator` and is inherited as a verdict; an Actions job's carries `creator: null` and is re-derived, as since #742 is every account outside `H10_REVIEWERS` — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger's `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The owner-level Restricted default (server-management#714) remains a separate OPEN lever — its effect on a declared `code: write` is unmeasured, and the version half of that condition has been satisfied since the 1.25.4 -> 1.27.1 upgrade on 2026-08-05, so it is not something to wait on a Gitea release for. A collaborator's own token remains unfixable; the exemption path has its own separate defects in #698. | 2026-07-28 | [link](records/ci/gate-trigger-base-resolved.md) | | `ci.gitea-milestone-filter-noop` | Never filter issues with the server-side `?milestones=` parameter — fetch all open issues once and filter LOCALLY on each issue's `.milestone.title`. | 2026-07-21 | [link](records/ci/gitea-milestone-filter-noop.md) | | `ci.grep-q-pipefail-inversion` | In any script running under `set -o pipefail`, a security or classification predicate of the form `producer \| grep -q…` is FORBIDDEN: `grep -q` exits at its first match, the producer then takes SIGPIPE and exits 141 once the data exceeds the pipe buffer (~64K), so `pipefail` reports the pipeline as FAILED even though grep MATCHED — inverting the predicate exactly when the input is large. A here-string (`grep -q… <<< "$data"`) is ALSO forbidden: bash materialises a large here-string via temporary storage, so it fails when temp space is full or unwritable, and inside an `if`/`!` that failure flips the predicate the same way. COUNT instead — `n=$(printf '%s\n' "$data" \| grep -cE "$re")` — because `grep -c` drains stdin (no early exit, no SIGPIPE) over an ordinary pipe (no temp file). Read grep's status honestly: exit 1 means a zero count and is a legitimate answer, anything >1 is a real error. Evaluate the counts ONCE at TOP LEVEL, never inline inside an `if`/`elif` condition: inside `$( )` an `exit` leaves only the subshell and `set -e` does not fire, so an error silently reads as "no match". Validate that each result is numeric and fail closed if not. This applies to both the enforced gate `.gitea/workflows/review-verdict.yml` and the advisory hook `.claude/hooks/pretooluse-merge-consent.sh`. | 2026-07-29 | [link](records/ci/grep-q-pipefail-inversion.md) | | `ci.infra-shaped-red-under-load` | When a job dies inside a setup/cache step before your code compiles, check the runner host's load before diagnosing the diff, and never file a CI bug off one sample under pressure. | 2026-07-21 | [link](records/ci/infra-shaped-red-under-load.md) | diff --git a/docs/decisions/records/ci/actions-credential-scoping.md b/docs/decisions/records/ci/actions-credential-scoping.md index 7b1d41dd8..fc4f9a4eb 100644 --- a/docs/decisions/records/ci/actions-credential-scoping.md +++ b/docs/decisions/records/ci/actions-credential-scoping.md @@ -5,7 +5,7 @@ status: active since: '2026-08-05' supersedes: none superseded-by: none -rule: 'Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. `permissions:` on a workflow/job DOES bind on this instance — MEASURED 2026-08-27 on 1.27.1 by matched scratch-base probe PRs differing only in one unit (`code: write` posted the probe status; `code: read` 403ed the POST, curl exit 22, no status written), so every workflow here now declares it (#748). Two properties that make it usable: the declaration is EXHAUSTIVE, not additive — a unit omitted is not granted — and it binds while the owner-level default is `permissive`, which is what makes the five `code: read` declarations effective TODAY rather than only after a flip. NOT established: what a declared `code: write` does under a RESTRICTED default. GitHub semantics let `permissions:` only narrow, never widen past the default, and if Gitea copies that, Restricted would cap the gate job at read and `review-verdict/h10` would stop being writable — the exact catastrophic case. The probe ran under `permissive` and therefore CANNOT rule that out. Flip the owner default only behind the scratch-base probe re-run under Restricted (server-management#714, still open for this reason). The earlier form of this rule said the opposite ("do NOT add a `permissions:` key on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP"); that was correct at 1.25.4 and is retained here only so a reader meeting the old advice recognises it as superseded rather than contradicted. There is still no API surface for the owner-level default (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists whatever the job token can do into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo''s 16 checkouts now set it (#746), `ci-image.yml`''s being excluded for the reason in that section — since #748 that one persisted credential is read-only by DECLARATION (`ci-image.yml` declares `code: read`) rather than by instance default. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation.' +rule: 'Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. `permissions:` on a workflow/job DOES bind on this instance — MEASURED 2026-08-27 on 1.27.1 by matched scratch-base probe PRs differing only in one unit (`code: write` posted the probe status; `code: read` 403ed the POST, curl exit 22, no status written), so five of the six workflows here now declare it (#748); `ci-image.yml` is excluded and rides with #744, leaving it on the owner default. Two properties that make it usable: the declaration is EXHAUSTIVE, not additive — a unit omitted is not granted — and it binds while the owner-level default is `permissive`, which is what makes the five `code: read` declarations effective TODAY rather than only after a flip. NOT established: what a declared `code: write` does under a RESTRICTED default. GitHub semantics let `permissions:` only narrow, never widen past the default, and if Gitea copies that, Restricted would cap the gate job at read and `review-verdict/h10` would stop being writable — the exact catastrophic case. The probe ran under `permissive` and therefore CANNOT rule that out. Flip the owner default only behind the scratch-base probe re-run under Restricted (server-management#714, still open for this reason). The earlier form of this rule said the opposite ("do NOT add a `permissions:` key on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP"); that was correct at 1.25.4 and is retained here only so a reader meeting the old advice recognises it as superseded rather than contradicted. There is still no API surface for the owner-level default (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists whatever the job token can do into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo''s 16 checkouts now set it (#746), `ci-image.yml`''s being excluded for the reason in that section. #748 did NOT change that: it excluded `ci-image.yml` for the same scheduling reason, so that one persisted credential is still write-capable on the owner default. Both exclusions clear together when #744 lands. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation.' signals: 'admin password in CI secrets, registry credential scope, ETV_STATUS_AUTH can write statuses, forge review-verdict/h10, head-resolved workflow holds credentials, persist-credentials on actions/checkout, credential left in .git/config, masked git fetch yields empty changed set, Gitea token scopes, no status scope, write:package vs write:repository, permissions key no-op, GITEA_TOKEN default read/write, Restricted default token permissions, orphan secret, deploy key in secret store, toolchain image overwrite, prod floating tag write · paths: `.gitea/workflows/docker-build.yml`, `.gitea/workflows/pr-checks.yml`, `.gitea/workflows/dependency-scan.yml`, `.gitea/workflows/ci-image.yml`, `.gitea/workflows/renovate.yml`, `scripts/ci-detect-already-validated.sh` · issues: #697, #672, #698, #742, #743, #746, #748, #835, #420, server-management#714' mechanics: 'PAT `ci-registry-scoped-697`, scopes `write:package,read:repository`, stored as repo Actions secret `REGISTRY_PASSWORD`; `REGISTRY_USER` remains `timothy`. Verified 2026-08-05 on Gitea 1.25.4: registry push of a probe tag SUCCEEDED; `GET /commits/{sha}/status` 200; `POST /statuses/{sha}` REFUSED HTTP 403 `token does not have at least one of required scope(s), required=[write:repository], token scope=write:package,read:repository`. Probe artifacts deleted, confirmed 404. NOT measured with this token: the `container:` pull, the buildcache write and the base-image pull. Those rest on Gitea''s scope model (write implies read per category, read at tag `v1.25.4`) — INFERRED. Note WHICH run proves which: only the `container:` pull is exercised by a PR. `cache-to`/`cache-from` and the base-image pull are confined to the `build` job, which carries `if: github.event_name != ''pull_request''`, so they are first exercised on the post-merge push to `main` — AFTER the merge gate has passed. A wrong inference there reddens main, not the PR.' --- @@ -75,8 +75,9 @@ into `.git/config` unless `persist-credentials: false` is set, so a job hands th every script it runs, not only to the checkout — and it was write-capable while this instance's Actions default token permission is `permissive` — where that default still stands after a Restricted trial was reverted on 2026-08-05. Since #748 the default no longer decides it: -`ci-image.yml` declares `code: read`, so the credential it persists is read-only by declaration -rather than by instance setting. 15 of the repo's 16 checkouts +`ci-image.yml` was deliberately left OUT of #748 (it rides with #744, same reason it was excluded +from #746), so it declares nothing, still receives the owner default, and the credential it persists +is still write-capable. It is the one file where the instance setting is what decides. 15 of the repo's 16 checkouts now set it: `docker-build.yml` (8), `pr-checks.yml` (5), `dependency-scan.yml` (1) and `review-verdict.yml` (1, pre-existing). No job that checks out pushes with git, so none of them loses a capability; `renovate.yml` does push, but has no checkout step and uses `RENOVATE_TOKEN`. @@ -136,7 +137,7 @@ with `permissions:` being a no-op sitting beside a permissive default. NOT established by this probe: what a declared `code: write` receives under a RESTRICTED owner default (both controls ran under `permissive`) — see the caveat in `rule`; and the TOP-LEVEL -declaration form the other five workflows use, which is inferred rather than measured. That +declaration form the other four declaring workflows use, which is inferred rather than measured. That inference fails safe (a top-level key Gitea ignored is a security no-op, never a brick), and `docker-build.yml`/`pr-checks.yml` are `on: pull_request`, so their form is exercised by any PR touching them. Probe branches and PRs (#851, #852) deleted after reading. @@ -146,7 +147,7 @@ touching them. Probe branches and PRs (#851, #852) deleted after reading. repo write; what changed is that a verdict posted with it is no longer *inherited* — `renovate` is not in `H10_REVIEWERS`, so `review-verdict.yml` re-derives it (#742). It remains status-capable, so it can still overwrite a status; it can no longer impersonate a reviewer. The injected `GITEA_TOKEN` was write-capable in every job. -That is no longer the mechanism: #748 declared `permissions:` on all six workflows (2026-08-27). +That is no longer the mechanism: #748 declared `permissions:` on five of the six workflows (2026-08-27); `ci-image.yml` is excluded and rides with #744. Because the declaration binds under the current `permissive` default, five of them now hold `code: read` and only `review-verdict.yml`'s gate job holds write. So the route is narrowed NOW, without the owner-level flip. The owner-level Restricted default (server-management#714) remains diff --git a/docs/decisions/records/ci/gate-trigger-base-resolved.md b/docs/decisions/records/ci/gate-trigger-base-resolved.md index 850b567da..e41bcd448 100644 --- a/docs/decisions/records/ci/gate-trigger-base-resolved.md +++ b/docs/decisions/records/ci/gate-trigger-base-resolved.md @@ -5,7 +5,7 @@ status: active since: '2026-07-28' supersedes: none superseded-by: none -rule: 'The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR''s own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch''s gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree''s scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class. Gitea injects `GITEA_TOKEN` into EVERY job and it USED to be write-capable everywhere, so any ref-resolved workflow could forge `review-verdict/h10`; since #748 (2026-08-27) every workflow here declares `permissions:` and five hold `code: read`, so no ref-resolved workflow that exists TODAY can post a status (the top-level declaration form those five use is INFERRED rather than probed — see `ci.actions-credential-scoping`). That closes the current instances, NOT the shape: a newly added workflow declaring write is a route again, which is why `test_pr_changed_files.py` asserts every tracked workflow declares a block. And a collaborator''s own API token still can, since branch protection binds the context and not its issuer. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI''s registry secret was the ADMIN account''s basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential''s route (an ALLOW-LISTED user credential''s forgery carries a matching `creator` and is inherited as a verdict; an Actions job''s carries `creator: null` and is re-derived, as since #742 is every account outside `H10_REVIEWERS` — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger''s `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The owner-level Restricted default (server-management#714) remains a separate OPEN lever — its effect on a declared `code: write` is unmeasured, and the version half of that condition has been satisfied since the 1.25.4 -> 1.27.1 upgrade on 2026-08-05, so it is not something to wait on a Gitea release for. A collaborator''s own token remains unfixable; the exemption path has its own separate defects in #698.' +rule: 'The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR''s own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch''s gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree''s scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class. Gitea injects `GITEA_TOKEN` into EVERY job and it USED to be write-capable everywhere, so any ref-resolved workflow could forge `review-verdict/h10`; since #748 (2026-08-27) five of the six workflows here declare `permissions:` and four hold `code: read`, so those cannot post a status (the top-level declaration form they use is INFERRED rather than probed — see `ci.actions-credential-scoping`). `ci-image.yml` is the EXCEPTION and it is exactly the dangerous shape: it declares nothing, so it still receives the owner default (write-capable), and its unfiltered `push:` trigger is path-scoped to itself, so any branch push runs its head-resolved YAML with a status-capable token. #748 deliberately did not touch it — editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job, so it rides with #744, which is the issue that closes this route. That closes the current instances, NOT the shape: a newly added workflow declaring write is a route again, which is why `test_pr_changed_files.py` asserts every tracked workflow declares a block. And a collaborator''s own API token still can, since branch protection binds the context and not its issuer. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI''s registry secret was the ADMIN account''s basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential''s route (an ALLOW-LISTED user credential''s forgery carries a matching `creator` and is inherited as a verdict; an Actions job''s carries `creator: null` and is re-derived, as since #742 is every account outside `H10_REVIEWERS` — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger''s `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The owner-level Restricted default (server-management#714) remains a separate OPEN lever — its effect on a declared `code: write` is unmeasured, and the version half of that condition has been satisfied since the 1.25.4 -> 1.27.1 upgrade on 2026-08-05, so it is not something to wait on a Gitea release for. A collaborator''s own token remains unfixable; the exemption path has its own separate defects in #698.' signals: 'workflow definition resolved from head, PR rewrites the gate that judges it, self-approve a required status check, pull_request_target vs pull_request, gate trigger branches filter, attacker-supplied base branch, how to test a change to review-verdict.yml, workflow not exercised by its own PR, gate edit goes live only on merge, required_approvals 0 does not bind an author, forged commit status inherited by sha · paths: `.gitea/workflows/review-verdict.yml`, `scripts/tests/test_pr_changed_files.py` · issues: #672, #663, #649, #622' mechanics: '`on: pull_request_target: {branches: [main], types: [opened, reopened, synchronize, ready_for_review, edited]}` (`edited` added by `ci.exemption-provenance` so a retarget reclassifies); asserted by `test_the_workflow_trigger_is_pull_request_TARGET_scoped_to_main` in `scripts/tests/test_pr_changed_files.py`; the job''s own context is renamed to `... (pull_request_target)` and must stay OUT of branch protection''s required list' --- @@ -53,10 +53,14 @@ instance of "a ref-resolved workflow can obtain credentials that POST a commit s inventory is not a short list. Gitea injects `GITEA_TOKEN` into **every** job, and it used to default to read/**write**, so head-resolved, `push`-triggered and `workflow_dispatch` workflows alike were routes (1.24+ loads a dispatched definition from the selected branch). Narrowed since #748 -(2026-08-27): every workflow in this repo now declares `permissions:` explicitly, and five hold -`code: read`, so none of them can POST a status (that rests on the TOP-LEVEL declaration form, -which is inferred rather than probed — only the job-level form was measured; see -`ci.actions-credential-scoping`). The route is closed for the workflows that exist +(2026-08-27): five of the six workflows here now declare `permissions:` explicitly and four hold +`code: read`, so those cannot POST a status (that rests on the TOP-LEVEL declaration form, which is +inferred rather than probed — only the job-level form was measured; see +`ci.actions-credential-scoping`). **`ci-image.yml` is NOT covered and is the worst-placed of the +six**: it declares nothing, still gets the owner default, and its unfiltered `push:` trigger is +path-scoped to itself, so any branch push runs its head-resolved YAML with a status-capable token. +That is #744's route to close, not this one's — #748 could not touch the file without reddening a +blocking job (`land-toolchain-image-change-separately`). The route is closed for the workflows that exist TODAY, not for the shape — a new workflow that declares write, or omits a declaration and inherits the owner default, is a route again, which is why `test_pr_changed_files.py` asserts every tracked workflow declares a block. A collaborator's own API token is a diff --git a/docs/decisions/records/release/main-direct-push-disabled.md b/docs/decisions/records/release/main-direct-push-disabled.md index fa5b06d94..93d972ec2 100644 --- a/docs/decisions/records/release/main-direct-push-disabled.md +++ b/docs/decisions/records/release/main-direct-push-disabled.md @@ -33,7 +33,8 @@ cite `enable_push: false` alone as the reason the gate holds. **What this actually closes, and what it does not.** It closes the *write-only* credential routes, which is most of #743's own "who can do it" list: the injected `GITEA_TOKEN` (repo write at the time, -narrowed to `code: read` in five of six workflows by #748; not admin either way), +narrowed to `code: read` in four of six workflows by #748, `ci-image.yml` still on the owner +default; not admin either way), `RENOVATE_TOKEN`, and any non-admin collaborator PAT. Those can no longer reach `main` at all, by any path that skips the gate. diff --git a/docs/decisions/records/release/verdict-status-check.md b/docs/decisions/records/release/verdict-status-check.md index 0be6612f1..652013855 100644 --- a/docs/decisions/records/release/verdict-status-check.md +++ b/docs/decisions/records/release/verdict-status-check.md @@ -185,7 +185,8 @@ described as one: `docker-build.yml` demonstrably could, and must stay head-resolved because it builds the PR's own code — so #697 scoped its credential instead (`ci.actions-credential-scoping`), leaving AT LEAST these: the injected `GITEA_TOKEN` (posts with `creator: null`) — narrowed by #748, which declared - `permissions:` on all six workflows so only `review-verdict.yml`'s own gate job still holds + `permissions:` on five of the six workflows (`ci-image.yml` rides with #744) so only + `review-verdict.yml`'s own gate job intentionally holds repo-write, though that job's token remains status-capable by construction — `RENOVATE_TOKEN` (a `write:repository` bot PAT in the same secret store, so it posts with a real creator — it is no longer *inherited* as a verdict, since #742 replaced the "non-null creator" test with membership in diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index 526664aec..e65db99ba 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -133,7 +133,7 @@ The audit #806 asked for, recorded whichever way it came out, because "we looked | `test_guard_inventory.py` | `.claude/hooks/*.sh`, `.husky/*`, `scripts/tests/test_*.py`, workflow/hook callers | **yes** — set equality against this table | converted to the index; `.husky/_/` had been excluded only because `_` is a directory, so the obvious "make it recursive" edit would have reintroduced #778's defect here | | `test_hook_fire_log.py` | `.claude/hooks/*.sh` | **yes** — every hook must be instrumented | converted; an untracked scratch `.sh` used to demand instrumentation and redden the suite on that checkout alone | | `test_ci_image_pin_population.py` | `.gitea/workflows/*.yml` + `*.yaml` | **yes** — "docker-build is the ONLY workflow pinning the toolchain image" | converted, and `*.yaml` added: Gitea accepts both spellings, so a `.yaml` workflow was structurally invisible while the test read as covering all of them | -| `test_pr_changed_files.py` | `.gitea/workflows/*.y*ml` | **yes** — "no OTHER workflow writes the review-verdict status", and since ersatztv#748 "every tracked workflow declares a `permissions:` block" (both over the same git-derived population) | converted. Not on #806's list and found by cold review on the pushed head: an untracked `.yaml` dropped in `.gitea/workflows/` reddened two guards while absent from the index — the issue's list of files to assess was a starting point, not the population | +| `test_pr_changed_files.py` | `.gitea/workflows/*.y*ml` | **yes** — "no OTHER workflow writes the review-verdict status", and since ersatztv#748 "every tracked workflow declares a `permissions:` block" (both over the same git-derived population; the second carries ONE literal exemption, `ci-image.yml`, which rides with #744 — ersatztv#835 tracks deleting it) | converted. Not on #806's list and found by cold review on the pushed head: an untracked `.yaml` dropped in `.gitea/workflows/` reddened two guards while absent from the index — the issue's list of files to assess was a starting point, not the population | | `web/src/api/pageSizeCallSites.guard.test.ts` | `web/src/**/*.{ts,tsx,mts,cts}` via `import.meta.glob` | **yes** — an unregistered discovered site fails | DEFERRED to #819, assessed not skipped. The glob is a documented workaround: `@types/node` is deliberately out of `tsconfig.app.json`, and wiring it in was tried and reverted (it leaked Node's `setTimeout` into the app project and broke three unrelated tests), so there is no `node:child_process` to reach the index from. Over-enumerates, so it fails loudly rather than going blind | | `test_ci_release_path_scan_job.py` | `.gitea/workflows/*.y*ml` + `scripts/**` | **no** — a fixture assembling a tmp harness, asserted about behaviour not membership | takes its file LIST from the index anyway, for hermeticity not completeness: `shutil.copytree` copied whatever was on disk, including untracked files and `scripts/__pycache__`, into a tree whose behaviour the probes then measure. Content still comes from the working tree. The copy is not a git repo, so the two files this step RUNS may not use the helper — see the fixture docstring | | `test_ci_dropped_step_guard.py` | the parsed workflow document | **no filesystem population at all** | unchanged; its residual is `MARKED_JOBS`, a SCOPE mirror of the required contexts on `main`, which #806 does not close | diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 685de1fd7..fc4d3bc62 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -3642,7 +3642,19 @@ def test_every_tracked_workflow_declares_a_permissions_block(): Population comes from the GIT INDEX via `_workflow_files()`, not a directory listing — same reason as the two completeness claims above (ersatztv#806/#778). + + ONE EXEMPTION, and it is a scheduling constraint rather than a judgement that the file is safe. + `ci-image.yml` is excluded because a PR that edits it re-points `ci-image-pin`'s `expected` at the + editing commit and staleness-fails a BLOCKING job, and because its unfiltered `push:` trigger is + path-scoped to itself, so the same edit would publish a toolchain image from the editing branch. + That is why `land-toolchain-image-change-separately` says it lands alone, and it rides with + ersatztv#744. This is the same exemption `ci.actions-credential-scoping` already carries for that + file's `persist-credentials`, and ersatztv#835 tracks removing BOTH once #744 has landed — at + which point delete this set rather than adding to it. Adding a second name here is a finding, not + a fix: the exemption is one file for one dated reason, not a mechanism. """ + # Deliberately a literal, not a pattern: a glob would silently absorb a future file. + EXEMPT = {"ci-image.yml"} import yaml workflows = _workflow_files() @@ -3666,10 +3678,13 @@ def test_every_tracked_workflow_declares_a_permissions_block(): # inherits the owner default, so `any` would pass a file that is half-covered. if jobs and all((job or {}).get("permissions") is not None for job in jobs): continue + if wf.name in EXEMPT: + continue 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) so a new file cannot silently inherit the " + "in this repo declares one (ersatztv#748) — except ci-image.yml, which rides with #744 — 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." ) -- 2.47.3 From 05dbadbd03ba856f52795d6c17f959d5950edb73 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 22:35:46 +0200 Subject: [PATCH 11/12] =?UTF-8?q?fix(748):=20round=207=20=E2=80=94=20and?= =?UTF-8?q?=20my=20self-deleting=20guard=20was=20itself=20dead=20code?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on the ci-image drop, plus one I found only because I mutated the claim instead of reading it. THE ONE THAT MATTERED. Round 6 added a "self-deleting" assert so the ci-image exemption cannot outlive #744 — and it was UNREACHABLE in exactly the case it exists for. The loop skipped any workflow that declares a block before it reached the EXEMPT branch, so giving ci-image.yml a block left the test GREEN. `a-guard-that-never-executed-proves-nothing`, in a guard written to prevent a silent hole, and I had already written "self-deleting" in a commit message. Caught by running the mutation and verifying it applied, not by reading the code. EXEMPT is now tested BEFORE `declares`, with a comment saying why that order is load-bearing. Both directions re-proved: ci-image gains a block -> red; renovate loses its block -> red; restored -> green. `docs/ci-cd.md` asserted P and not-P about the same file in one sentence — I had APPENDED an "EXCEPT ci-image.yml" to the round-2 claim that ci-image's persisted credential is read-only by declaration, rather than deleting the claim. Both halves were standing, in the section a reader consults for precisely that fact. Round-2 clause deleted. `ci.gate-trigger-base-resolved` still summarised, two sentences after the carve-out that falsifies it, that "the route is closed for the workflows that exist TODAY" — body AND `rule:`, so in the catalog. ci-image.yml IS a current instance and IS open. Now: NARROWED, not closed, with both reasons. `ci.actions-credential-scoping` still said five hold `code: read` — four do; five DECLARE, of which one declares write. Body and `rule:`. The same fact was four in ci-cd.md and five in the record, which is the divergence pattern from round 3. `land-toolchain-image-change-separately` is NOT a decision key — no record, no archive entry, not in the catalog. It is a memory filename, and round 6 propagated it into two more places including the catalog. That is the breadcrumb defect this repo has a rule against. All citations now point at `docs/ci-cd.md` -> "CI toolchain image", including the one that predates this PR. Strength: "any branch push runs its head-resolved YAML" overstated the trigger — only a push touching `docker/ci/**` or the file itself fires it. Kept the threat claim, which holds because a forging edit necessarily touches the file. Decisions-Edit: yes Refs: #748 #744 #835 --- docs/ci-cd.md | 21 ++++++++-------- docs/decisions/README.md | 4 +-- .../records/ci/actions-credential-scoping.md | 8 +++--- .../records/ci/gate-trigger-base-resolved.md | 14 ++++++----- scripts/tests/test_pr_changed_files.py | 25 +++++++++++++++---- 5 files changed, 45 insertions(+), 27 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 4061f9e1f..c31b8001b 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -249,14 +249,15 @@ own CI: `ci-image.yml` declares **no** `permissions:` block, so it still receives the owner-level default and its token is write-capable, as is the credential its checkout persists (it is also the one checkout that does not set `persist-credentials: false`, ersatztv#746). That is the *worst*-placed of the six, -not the safest: its `push:` trigger is unfiltered and path-scoped to itself, so any branch push runs -its head-resolved YAML with a status-capable token. +not the safest: its `push:` trigger has no `branches:` filter and is path-scoped to itself, so a push +touching `docker/ci/**` or that file — which a forging edit necessarily does — runs its head-resolved +YAML with a status-capable token, on any branch and with no PR. It is excluded for a scheduling reason. A PR editing that file re-points `ci-image-pin`'s `expected` at the editing commit and staleness-fails a **blocking** job, and the same edit would publish a toolchain image from the editing branch. Both are why -`release.format-as-you-touch-rebase`'s sibling rule `land-toolchain-image-change-separately` says the -file lands alone. It rides with **ersatztv#744**, which is also the issue that closes the trigger +`docs/ci-cd.md` -> "CI toolchain image" documents the two-step this would otherwise force, which is +why the file lands alone. It rides with **ersatztv#744**, which is also the issue that closes the trigger route — scoping this file's token would not have closed it anyway, because scoping bounds what a job may *do* and only a filter bounds whether it *runs*. @@ -271,12 +272,12 @@ Every `actions/checkout` in this repo **except `ci-image.yml`'s** (see the carve `.git/config`, and a head-resolved job then hands that credential to every script it runs, not just to the checkout. That header is write-capable *only while the job's token is*: this instance's Actions default token permission is `permissive`, where it stands after a Restricted trial was reverted on -2026-08-05. Since ersatztv#748 that default no longer governs this repo's jobs — every workflow -declares `permissions:` explicitly (see the section above), so `ci-image.yml`'s persisted header is -read-only by declaration rather than by instance setting — EXCEPT `ci-image.yml`, which declares -nothing and so still receives the owner default; its persisted header stays write-capable, and that -is the one this section's carve-out is about. Re-applying the owner-level default is -server-management#714. No job that checks out +2026-08-05. Since ersatztv#748 that default no longer governs MOST of this repo's jobs +— five of the six workflows declare `permissions:` explicitly (see the section above). But +`ci-image.yml` is not one of them: it is excluded from #748 for the same scheduling reason it was +excluded from #746, so it still receives the owner default and **the credential it persists is still +write-capable**. That is this section's carve-out, and #748 did not change it; both exclusions clear +together when ersatztv#744 lands. Re-applying the owner-level default is server-management#714. No job that checks out loses anything: none of them push with git, and anonymous access covers the fetches because the repo is public — `private: false`, and `GET /info/refs?service=git-upload-pack` answers unauthenticated, measured 2026-08-26 from the LAN. That measures the repository, not a runner's network path to it, diff --git a/docs/decisions/README.md b/docs/decisions/README.md index da0a1c710..cb7dd7663 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -38,7 +38,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `blazor.rollback-tag` | The commit immediately preceding the Blazor-removal merge is tagged `blazor-final` (not a `v*` tag, so it doesn't trigger a prod release build) as the documented rollback/restore path. | 2026-07-11 | [link](records/blazor/rollback-tag.md) | | `blazor.ui-removed` | The legacy Blazor Server UI (`Pages/`, `Shared/`, `ViewModels/`, `Validators/`, MudBlazor + 8 other packages, Blazor Startup wiring) is fully deleted now that the SPA has parity; the legacy `MapWhen` branch is kept only for controllers/docs/OpenAPI/`LegacyUiRedirects`, and the catch-all fallback 302s any unmatched non-api/artwork/docs/openapi path to `/app`. | 2026-07-11 | [link](records/blazor/ui-removed.md) | | `channel.origin-marker` | A new `Channel.Origin` (`ChannelOrigin` enum — `Unknown`/`UserCreated`/`AutoTuned`) records how a channel row was created and is stamped exactly once at insert (`AutoTuned` in `CreateChannelFromLineupHandler`, `UserCreated` in `CreateChannelHandler`), and is never mutated on a later edit. It is surfaced as a raw `origin` field on `ChannelResponseModel`; the SPA badges only `AutoTuned`. Rows predating the column read `Unknown` — provenance is **not** back-filled. | 2026-07-23 | [link](records/channel/origin-marker.md) | -| `ci.actions-credential-scoping` | Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. `permissions:` on a workflow/job DOES bind on this instance — MEASURED 2026-08-27 on 1.27.1 by matched scratch-base probe PRs differing only in one unit (`code: write` posted the probe status; `code: read` 403ed the POST, curl exit 22, no status written), so five of the six workflows here now declare it (#748); `ci-image.yml` is excluded and rides with #744, leaving it on the owner default. Two properties that make it usable: the declaration is EXHAUSTIVE, not additive — a unit omitted is not granted — and it binds while the owner-level default is `permissive`, which is what makes the five `code: read` declarations effective TODAY rather than only after a flip. NOT established: what a declared `code: write` does under a RESTRICTED default. GitHub semantics let `permissions:` only narrow, never widen past the default, and if Gitea copies that, Restricted would cap the gate job at read and `review-verdict/h10` would stop being writable — the exact catastrophic case. The probe ran under `permissive` and therefore CANNOT rule that out. Flip the owner default only behind the scratch-base probe re-run under Restricted (server-management#714, still open for this reason). The earlier form of this rule said the opposite ("do NOT add a `permissions:` key on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP"); that was correct at 1.25.4 and is retained here only so a reader meeting the old advice recognises it as superseded rather than contradicted. There is still no API surface for the owner-level default (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists whatever the job token can do into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo's 16 checkouts now set it (#746), `ci-image.yml`'s being excluded for the reason in that section. #748 did NOT change that: it excluded `ci-image.yml` for the same scheduling reason, so that one persisted credential is still write-capable on the owner default. Both exclusions clear together when #744 lands. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation. | 2026-08-05 | [link](records/ci/actions-credential-scoping.md) | +| `ci.actions-credential-scoping` | Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. `permissions:` on a workflow/job DOES bind on this instance — MEASURED 2026-08-27 on 1.27.1 by matched scratch-base probe PRs differing only in one unit (`code: write` posted the probe status; `code: read` 403ed the POST, curl exit 22, no status written), so five of the six workflows here now declare it (#748); `ci-image.yml` is excluded and rides with #744, leaving it on the owner default. Two properties that make it usable: the declaration is EXHAUSTIVE, not additive — a unit omitted is not granted — and it binds while the owner-level default is `permissive`, which is what makes the four `code: read` declarations effective TODAY rather than only after a flip. NOT established: what a declared `code: write` does under a RESTRICTED default. GitHub semantics let `permissions:` only narrow, never widen past the default, and if Gitea copies that, Restricted would cap the gate job at read and `review-verdict/h10` would stop being writable — the exact catastrophic case. The probe ran under `permissive` and therefore CANNOT rule that out. Flip the owner default only behind the scratch-base probe re-run under Restricted (server-management#714, still open for this reason). The earlier form of this rule said the opposite ("do NOT add a `permissions:` key on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP"); that was correct at 1.25.4 and is retained here only so a reader meeting the old advice recognises it as superseded rather than contradicted. There is still no API surface for the owner-level default (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists whatever the job token can do into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo's 16 checkouts now set it (#746), `ci-image.yml`'s being excluded for the reason in that section. #748 did NOT change that: it excluded `ci-image.yml` for the same scheduling reason, so that one persisted credential is still write-capable on the owner default. Both exclusions clear together when #744 lands. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation. | 2026-08-05 | [link](records/ci/actions-credential-scoping.md) | | `ci.batch-pushes-no-cancel-route` | Hold review fixes, doc corrections and format fixes locally and push **once** — a superseded run cannot be cancelled from the agent side and holds a runner slot until it finishes. | 2026-07-21 | [link](records/ci/batch-pushes-no-cancel-route.md) | | `ci.build-once-rejected` | CI build-once (a shared compile artifact across jobs) was implemented, measured, and rejected for a 40-85% wall-clock regression; keep the #420 cross-run tree-identity skip instead. | 2026-07-18 | [link](records/ci/build-once-rejected.md) | | `ci.cancelled-is-not-a-verdict` | Treat a `cancelled` conclusion as "no verdict" — never as pass or fail — and report FAILED and CANCELLED counts separately in any CI monitor. THE COMBINED COMMIT-STATUS ENDPOINT CANNOT EXPRESS THIS: `GET /repos/{o}/{r}/commits/{sha}/status` has states `success`/`failure`/`pending`/`error` and NO `cancelled`, so it reports a cancelled job as `failure`. Anything polling that endpoint — which is what a CI monitor naturally polls, because it is the per-sha view the merge gate reads — must resolve the job-level `conclusion` via `actions/runs/{id}/jobs` before reporting a red. | 2026-07-21 | [link](records/ci/cancelled-is-not-a-verdict.md) | @@ -49,7 +49,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.exemption-provenance` | The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR's live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR's immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires for a `success` only when it is POSITIVELY identified as an APPROVED REVIEWER's verdict for THIS base, meaning a `.creator.login` that is a member of the workflow's `H10_REVIEWERS` allow-list (an existing `failure` takes the weaker ATTRIBUTABLE test and needs no membership, because inheriting a rejection can only withhold an exemption while re-deriving one can turn it green on an exempt PR) — non-null was too weak, since any account's credential satisfies it and `RENOVATE_TOKEN` is one that cannot be scoped down (#742) — AND a `Review-verdict:` description AND, when that description records a base (`(base: …)`, `release.verdict-status-check`), a base matching the PR's — tested by requiring the description to END with the exact literal `(base: )` and to contain exactly ONE such marker, never by extracting a value (see below); a present-but-different base is rejected, an absent one is not, since verdicts predating that convention carry none; every other shape, including any unrecognised one, is re-derived rather than trusted. The bot and docs-only exemptions are evaluated as INDEPENDENT predicates and the decision made afterwards, never as an `elif` chain. `edited` is in the workflow's `types:` so a retarget reclassifies — which gives DETECTION, not atomicity: status writes are not serialized, so a stale run can still post over a fresher one. That residual is now FENCED rather than merely tracked (NARROWED, not resolved — a retarget between the fence's final count and the POST still leaves a PERMANENT forged green, #849) — the job refuses to write at all if the PR's timeline retarget COUNT moved while it was classifying (`ci.verdict-write-retarget-fence`, #706) — leaving TWO windows, not one: the sub-round-trip window that no API without compare-and-set can close, AND the post-final-count/pre-POST window, which is not sub-round-trip and yields a PERMANENT green because the successor can consume the `edited` event and exit before the stale run posts last (#849). The PROTECTED path list additionally covers `CLAUDE.md` and `AGENTS.md` (#751) — they are not prose but the documents DEFINING the completion protocol, the merge-consent convention and the H10 rule, so protecting `.claude/` while the file specifying what it enforces stayed docs-only-exempt was the same self-exemption one directory over; driving the real classify body with a lone `CLAUDE.md` change produced an exemption `success`. `README.md` is deliberately not listed. It also covers `.codex/` (#711), which mirrors `.claude/hooks/` byte for byte including the merge-consent hook — latent while that directory is untracked, live the moment it is tracked; the list stays ENUMERATIVE rather than derived, because a derived rule would have to be evaluated against the very file list being classified. Reading the CURRENT status for input (3) must tolerate `statuses: null`: `GET /commits/{sha}/status` serialises a nil slice as `null`, not `[]`, on a head with no statuses yet, and an `array`-only gate made `read_existing_verdict` `exit 1` and post nothing at all (#751, `ci.workflow-run-body-no-expressions`) — `null` is accepted only when `total_count` is 0, so a body that merely lost its array is still refused. Path predicates are evaluated by COUNTING with `grep -c`, never `\| grep -q` (SIGPIPE inversion) and never a here-string (temp-space failure) — see `ci.grep-q-pipefail-inversion`. | 2026-07-29 | [link](records/ci/exemption-provenance.md) | | `ci.format-gate-folder-mode` | The blocking `format` CI job (and matching pre-commit hook) runs `dotnet format whitespace . --folder --include ` instead of loading the full MSBuild/Roslyn solution, cutting the gate from ~480s to ~0.5s with unchanged whitespace/charset coverage. | 2026-07-19 | [link](records/ci/format-gate-folder-mode.md) | | `ci.functional-e2e-harness` | The `functional-e2e` CI job boots the PR's own code from source via `dotnet run` (`scripts/e2e-local.sh`) and runs deterministic assertions (`scripts/e2e-functional.sh`) as an advisory (non-blocking) job, not a `build` dependency or required check. Originally curl-only; since #445 the same job carries a second, headless-browser step for the contracts curl cannot express — see `ci.ui-e2e-harness`. | 2026-07-16 | [link](records/ci/functional-e2e-harness.md) | -| `ci.gate-trigger-base-resolved` | The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR's own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch's gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree's scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class. Gitea injects `GITEA_TOKEN` into EVERY job and it USED to be write-capable everywhere, so any ref-resolved workflow could forge `review-verdict/h10`; since #748 (2026-08-27) five of the six workflows here declare `permissions:` and four hold `code: read`, so those cannot post a status (the top-level declaration form they use is INFERRED rather than probed — see `ci.actions-credential-scoping`). `ci-image.yml` is the EXCEPTION and it is exactly the dangerous shape: it declares nothing, so it still receives the owner default (write-capable), and its unfiltered `push:` trigger is path-scoped to itself, so any branch push runs its head-resolved YAML with a status-capable token. #748 deliberately did not touch it — editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job, so it rides with #744, which is the issue that closes this route. That closes the current instances, NOT the shape: a newly added workflow declaring write is a route again, which is why `test_pr_changed_files.py` asserts every tracked workflow declares a block. And a collaborator's own API token still can, since branch protection binds the context and not its issuer. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI's registry secret was the ADMIN account's basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential's route (an ALLOW-LISTED user credential's forgery carries a matching `creator` and is inherited as a verdict; an Actions job's carries `creator: null` and is re-derived, as since #742 is every account outside `H10_REVIEWERS` — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger's `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The owner-level Restricted default (server-management#714) remains a separate OPEN lever — its effect on a declared `code: write` is unmeasured, and the version half of that condition has been satisfied since the 1.25.4 -> 1.27.1 upgrade on 2026-08-05, so it is not something to wait on a Gitea release for. A collaborator's own token remains unfixable; the exemption path has its own separate defects in #698. | 2026-07-28 | [link](records/ci/gate-trigger-base-resolved.md) | +| `ci.gate-trigger-base-resolved` | The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR's own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch's gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree's scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class. Gitea injects `GITEA_TOKEN` into EVERY job and it USED to be write-capable everywhere, so any ref-resolved workflow could forge `review-verdict/h10`; since #748 (2026-08-27) five of the six workflows here declare `permissions:` and four hold `code: read`, so those cannot post a status (the top-level declaration form they use is INFERRED rather than probed — see `ci.actions-credential-scoping`). `ci-image.yml` is the EXCEPTION and it is exactly the dangerous shape: it declares nothing, so it still receives the owner default (write-capable), and its `push:` trigger has no `branches:` filter and is path-scoped to itself, so a push touching `docker/ci/**` or that file — which a forging edit necessarily does — runs its head-resolved YAML with a status-capable token, on any branch and with no PR. #748 deliberately did not touch it — editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job, so it rides with #744, which is the issue that closes this route. So the route is NARROWED, not closed: `ci-image.yml` is a current instance that remains open, and the SHAPE is open besides — a newly added workflow declaring write is a route again, which is why `test_pr_changed_files.py` asserts every tracked workflow declares a block. And a collaborator's own API token still can, since branch protection binds the context and not its issuer. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI's registry secret was the ADMIN account's basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential's route (an ALLOW-LISTED user credential's forgery carries a matching `creator` and is inherited as a verdict; an Actions job's carries `creator: null` and is re-derived, as since #742 is every account outside `H10_REVIEWERS` — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger's `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The owner-level Restricted default (server-management#714) remains a separate OPEN lever — its effect on a declared `code: write` is unmeasured, and the version half of that condition has been satisfied since the 1.25.4 -> 1.27.1 upgrade on 2026-08-05, so it is not something to wait on a Gitea release for. A collaborator's own token remains unfixable; the exemption path has its own separate defects in #698. | 2026-07-28 | [link](records/ci/gate-trigger-base-resolved.md) | | `ci.gitea-milestone-filter-noop` | Never filter issues with the server-side `?milestones=` parameter — fetch all open issues once and filter LOCALLY on each issue's `.milestone.title`. | 2026-07-21 | [link](records/ci/gitea-milestone-filter-noop.md) | | `ci.grep-q-pipefail-inversion` | In any script running under `set -o pipefail`, a security or classification predicate of the form `producer \| grep -q…` is FORBIDDEN: `grep -q` exits at its first match, the producer then takes SIGPIPE and exits 141 once the data exceeds the pipe buffer (~64K), so `pipefail` reports the pipeline as FAILED even though grep MATCHED — inverting the predicate exactly when the input is large. A here-string (`grep -q… <<< "$data"`) is ALSO forbidden: bash materialises a large here-string via temporary storage, so it fails when temp space is full or unwritable, and inside an `if`/`!` that failure flips the predicate the same way. COUNT instead — `n=$(printf '%s\n' "$data" \| grep -cE "$re")` — because `grep -c` drains stdin (no early exit, no SIGPIPE) over an ordinary pipe (no temp file). Read grep's status honestly: exit 1 means a zero count and is a legitimate answer, anything >1 is a real error. Evaluate the counts ONCE at TOP LEVEL, never inline inside an `if`/`elif` condition: inside `$( )` an `exit` leaves only the subshell and `set -e` does not fire, so an error silently reads as "no match". Validate that each result is numeric and fail closed if not. This applies to both the enforced gate `.gitea/workflows/review-verdict.yml` and the advisory hook `.claude/hooks/pretooluse-merge-consent.sh`. | 2026-07-29 | [link](records/ci/grep-q-pipefail-inversion.md) | | `ci.infra-shaped-red-under-load` | When a job dies inside a setup/cache step before your code compiles, check the runner host's load before diagnosing the diff, and never file a CI bug off one sample under pressure. | 2026-07-21 | [link](records/ci/infra-shaped-red-under-load.md) | diff --git a/docs/decisions/records/ci/actions-credential-scoping.md b/docs/decisions/records/ci/actions-credential-scoping.md index fc4f9a4eb..5d39ec505 100644 --- a/docs/decisions/records/ci/actions-credential-scoping.md +++ b/docs/decisions/records/ci/actions-credential-scoping.md @@ -5,7 +5,7 @@ status: active since: '2026-08-05' supersedes: none superseded-by: none -rule: 'Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. `permissions:` on a workflow/job DOES bind on this instance — MEASURED 2026-08-27 on 1.27.1 by matched scratch-base probe PRs differing only in one unit (`code: write` posted the probe status; `code: read` 403ed the POST, curl exit 22, no status written), so five of the six workflows here now declare it (#748); `ci-image.yml` is excluded and rides with #744, leaving it on the owner default. Two properties that make it usable: the declaration is EXHAUSTIVE, not additive — a unit omitted is not granted — and it binds while the owner-level default is `permissive`, which is what makes the five `code: read` declarations effective TODAY rather than only after a flip. NOT established: what a declared `code: write` does under a RESTRICTED default. GitHub semantics let `permissions:` only narrow, never widen past the default, and if Gitea copies that, Restricted would cap the gate job at read and `review-verdict/h10` would stop being writable — the exact catastrophic case. The probe ran under `permissive` and therefore CANNOT rule that out. Flip the owner default only behind the scratch-base probe re-run under Restricted (server-management#714, still open for this reason). The earlier form of this rule said the opposite ("do NOT add a `permissions:` key on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP"); that was correct at 1.25.4 and is retained here only so a reader meeting the old advice recognises it as superseded rather than contradicted. There is still no API surface for the owner-level default (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists whatever the job token can do into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo''s 16 checkouts now set it (#746), `ci-image.yml`''s being excluded for the reason in that section. #748 did NOT change that: it excluded `ci-image.yml` for the same scheduling reason, so that one persisted credential is still write-capable on the owner default. Both exclusions clear together when #744 lands. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation.' +rule: 'Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. `permissions:` on a workflow/job DOES bind on this instance — MEASURED 2026-08-27 on 1.27.1 by matched scratch-base probe PRs differing only in one unit (`code: write` posted the probe status; `code: read` 403ed the POST, curl exit 22, no status written), so five of the six workflows here now declare it (#748); `ci-image.yml` is excluded and rides with #744, leaving it on the owner default. Two properties that make it usable: the declaration is EXHAUSTIVE, not additive — a unit omitted is not granted — and it binds while the owner-level default is `permissive`, which is what makes the four `code: read` declarations effective TODAY rather than only after a flip. NOT established: what a declared `code: write` does under a RESTRICTED default. GitHub semantics let `permissions:` only narrow, never widen past the default, and if Gitea copies that, Restricted would cap the gate job at read and `review-verdict/h10` would stop being writable — the exact catastrophic case. The probe ran under `permissive` and therefore CANNOT rule that out. Flip the owner default only behind the scratch-base probe re-run under Restricted (server-management#714, still open for this reason). The earlier form of this rule said the opposite ("do NOT add a `permissions:` key on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP"); that was correct at 1.25.4 and is retained here only so a reader meeting the old advice recognises it as superseded rather than contradicted. There is still no API surface for the owner-level default (`/api/v1/settings/actions` 404s at 1.27.1). The instance default HAS since been probed and is NOT unknown: it was set to Restricted, verified, and reverted to `permissive` on 2026-08-05 (#748), which is where it stands. Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). A second, separate consequence of the same boundary: `actions/checkout` persists whatever the job token can do into `.git/config` unless `persist-credentials: false` is set, and 15 of this repo''s 16 checkouts now set it (#746), `ci-image.yml`''s being excluded for the reason in that section. #748 did NOT change that: it excluded `ci-image.yml` for the same scheduling reason, so that one persisted credential is still write-capable on the owner default. Both exclusions clear together when #744 lands. Ordering is part of the rule: unmask the dependent fetches FIRST, because until then a credential regression presents as an empty changed-file set and a silently skipped check rather than a red job. This record closes ONE route. It does not close the class, and the later sections say exactly what survives — read them before citing this record as a mitigation.' signals: 'admin password in CI secrets, registry credential scope, ETV_STATUS_AUTH can write statuses, forge review-verdict/h10, head-resolved workflow holds credentials, persist-credentials on actions/checkout, credential left in .git/config, masked git fetch yields empty changed set, Gitea token scopes, no status scope, write:package vs write:repository, permissions key no-op, GITEA_TOKEN default read/write, Restricted default token permissions, orphan secret, deploy key in secret store, toolchain image overwrite, prod floating tag write · paths: `.gitea/workflows/docker-build.yml`, `.gitea/workflows/pr-checks.yml`, `.gitea/workflows/dependency-scan.yml`, `.gitea/workflows/ci-image.yml`, `.gitea/workflows/renovate.yml`, `scripts/ci-detect-already-validated.sh` · issues: #697, #672, #698, #742, #743, #746, #748, #835, #420, server-management#714' mechanics: 'PAT `ci-registry-scoped-697`, scopes `write:package,read:repository`, stored as repo Actions secret `REGISTRY_PASSWORD`; `REGISTRY_USER` remains `timothy`. Verified 2026-08-05 on Gitea 1.25.4: registry push of a probe tag SUCCEEDED; `GET /commits/{sha}/status` 200; `POST /statuses/{sha}` REFUSED HTTP 403 `token does not have at least one of required scope(s), required=[write:repository], token scope=write:package,read:repository`. Probe artifacts deleted, confirmed 404. NOT measured with this token: the `container:` pull, the buildcache write and the base-image pull. Those rest on Gitea''s scope model (write implies read per category, read at tag `v1.25.4`) — INFERRED. Note WHICH run proves which: only the `container:` pull is exercised by a PR. `cache-to`/`cache-from` and the base-image pull are confined to the `build` job, which carries `if: github.event_name != ''pull_request''`, so they are first exercised on the post-merge push to `main` — AFTER the merge gate has passed. A wrong inference there reddens main, not the PR.' --- @@ -23,7 +23,7 @@ with no PR. Scoping bounds what a job may DO; only a filter bounds whether it RU **#744**, not this record: editing `ci-image.yml` re-points `ci-image-pin`'s `expected` at the editing commit and staleness-fails a **blocking** job. That is a toll, not a wall — the documented two-step (publish `:`, then bump all five pins) clears it — but a rebase rewrites the sha and charges -it again, so it lands alone (`land-toolchain-image-change-separately`). +it again, so it lands alone (`docs/ci-cd.md` -> "CI toolchain image"). **What the scoped token still reaches — not "just a registry credential".** `write:package` over owner `timothy` writes `ersatztv:prod` (the floating tag prod's `jazz-media` stack follows) and @@ -148,8 +148,8 @@ repo write; what changed is that a verdict posted with it is no longer *inherite in `H10_REVIEWERS`, so `review-verdict.yml` re-derives it (#742). It remains status-capable, so it can still overwrite a status; it can no longer impersonate a reviewer. The injected `GITEA_TOKEN` was write-capable in every job. That is no longer the mechanism: #748 declared `permissions:` on five of the six workflows (2026-08-27); `ci-image.yml` is excluded and rides with #744. -Because the declaration binds under the current `permissive` default, five of them now hold -`code: read` and only `review-verdict.yml`'s gate job holds write. So the route is narrowed NOW, +Because the declaration binds under the current `permissive` default, FOUR of them now hold +`code: read` and only `review-verdict.yml`'s gate job holds write (the fifth declarer). So the route is narrowed NOW, without the owner-level flip. The owner-level Restricted default (server-management#714) remains worth applying as the instance-wide floor for repos that declare nothing — but it is NOT yet shown safe here: whether a declared `code: write` survives a Restricted default is unmeasured, and if diff --git a/docs/decisions/records/ci/gate-trigger-base-resolved.md b/docs/decisions/records/ci/gate-trigger-base-resolved.md index e41bcd448..f1d8e229b 100644 --- a/docs/decisions/records/ci/gate-trigger-base-resolved.md +++ b/docs/decisions/records/ci/gate-trigger-base-resolved.md @@ -5,7 +5,7 @@ status: active since: '2026-07-28' supersedes: none superseded-by: none -rule: 'The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR''s own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch''s gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree''s scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class. Gitea injects `GITEA_TOKEN` into EVERY job and it USED to be write-capable everywhere, so any ref-resolved workflow could forge `review-verdict/h10`; since #748 (2026-08-27) five of the six workflows here declare `permissions:` and four hold `code: read`, so those cannot post a status (the top-level declaration form they use is INFERRED rather than probed — see `ci.actions-credential-scoping`). `ci-image.yml` is the EXCEPTION and it is exactly the dangerous shape: it declares nothing, so it still receives the owner default (write-capable), and its unfiltered `push:` trigger is path-scoped to itself, so any branch push runs its head-resolved YAML with a status-capable token. #748 deliberately did not touch it — editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job, so it rides with #744, which is the issue that closes this route. That closes the current instances, NOT the shape: a newly added workflow declaring write is a route again, which is why `test_pr_changed_files.py` asserts every tracked workflow declares a block. And a collaborator''s own API token still can, since branch protection binds the context and not its issuer. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI''s registry secret was the ADMIN account''s basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential''s route (an ALLOW-LISTED user credential''s forgery carries a matching `creator` and is inherited as a verdict; an Actions job''s carries `creator: null` and is re-derived, as since #742 is every account outside `H10_REVIEWERS` — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger''s `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The owner-level Restricted default (server-management#714) remains a separate OPEN lever — its effect on a declared `code: write` is unmeasured, and the version half of that condition has been satisfied since the 1.25.4 -> 1.27.1 upgrade on 2026-08-05, so it is not something to wait on a Gitea release for. A collaborator''s own token remains unfixable; the exemption path has its own separate defects in #698.' +rule: 'The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR''s own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch''s gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree''s scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class. Gitea injects `GITEA_TOKEN` into EVERY job and it USED to be write-capable everywhere, so any ref-resolved workflow could forge `review-verdict/h10`; since #748 (2026-08-27) five of the six workflows here declare `permissions:` and four hold `code: read`, so those cannot post a status (the top-level declaration form they use is INFERRED rather than probed — see `ci.actions-credential-scoping`). `ci-image.yml` is the EXCEPTION and it is exactly the dangerous shape: it declares nothing, so it still receives the owner default (write-capable), and its `push:` trigger has no `branches:` filter and is path-scoped to itself, so a push touching `docker/ci/**` or that file — which a forging edit necessarily does — runs its head-resolved YAML with a status-capable token, on any branch and with no PR. #748 deliberately did not touch it — editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job, so it rides with #744, which is the issue that closes this route. So the route is NARROWED, not closed: `ci-image.yml` is a current instance that remains open, and the SHAPE is open besides — a newly added workflow declaring write is a route again, which is why `test_pr_changed_files.py` asserts every tracked workflow declares a block. And a collaborator''s own API token still can, since branch protection binds the context and not its issuer. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI''s registry secret was the ADMIN account''s basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential''s route (an ALLOW-LISTED user credential''s forgery carries a matching `creator` and is inherited as a verdict; an Actions job''s carries `creator: null` and is re-derived, as since #742 is every account outside `H10_REVIEWERS` — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger''s `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The owner-level Restricted default (server-management#714) remains a separate OPEN lever — its effect on a declared `code: write` is unmeasured, and the version half of that condition has been satisfied since the 1.25.4 -> 1.27.1 upgrade on 2026-08-05, so it is not something to wait on a Gitea release for. A collaborator''s own token remains unfixable; the exemption path has its own separate defects in #698.' signals: 'workflow definition resolved from head, PR rewrites the gate that judges it, self-approve a required status check, pull_request_target vs pull_request, gate trigger branches filter, attacker-supplied base branch, how to test a change to review-verdict.yml, workflow not exercised by its own PR, gate edit goes live only on merge, required_approvals 0 does not bind an author, forged commit status inherited by sha · paths: `.gitea/workflows/review-verdict.yml`, `scripts/tests/test_pr_changed_files.py` · issues: #672, #663, #649, #622' mechanics: '`on: pull_request_target: {branches: [main], types: [opened, reopened, synchronize, ready_for_review, edited]}` (`edited` added by `ci.exemption-provenance` so a retarget reclassifies); asserted by `test_the_workflow_trigger_is_pull_request_TARGET_scoped_to_main` in `scripts/tests/test_pr_changed_files.py`; the job''s own context is renamed to `... (pull_request_target)` and must stay OUT of branch protection''s required list' --- @@ -58,12 +58,14 @@ were routes (1.24+ loads a dispatched definition from the selected branch). Narr inferred rather than probed — only the job-level form was measured; see `ci.actions-credential-scoping`). **`ci-image.yml` is NOT covered and is the worst-placed of the six**: it declares nothing, still gets the owner default, and its unfiltered `push:` trigger is -path-scoped to itself, so any branch push runs its head-resolved YAML with a status-capable token. +path-scoped to itself, so a push touching `docker/ci/**` or that file — which a forging edit +necessarily does — runs its head-resolved YAML with a status-capable token, on any branch, with no PR. That is #744's route to close, not this one's — #748 could not touch the file without reddening a -blocking job (`land-toolchain-image-change-separately`). The route is closed for the workflows that exist -TODAY, not for the shape — a new workflow that declares write, or omits a declaration and inherits -the owner default, is a route again, which is why `test_pr_changed_files.py` asserts every tracked -workflow declares a block. A collaborator's own API token is a +blocking job (see `docs/ci-cd.md` -> "CI toolchain image" for the two-step it would force). So the +route is NARROWED, not closed: four of today's workflows can no longer post a status, `ci-image.yml` +still can, and the SHAPE is open besides — a new workflow that declares write, or omits a declaration +and inherits the owner default, is a route again, which is why `test_pr_changed_files.py` asserts +every tracked workflow declares a block. A collaborator's own API token is a route with no workflow at all — branch protection binds the *context*, not its issuer. Full inventory in `#697`, whose credential half is resolved in `ci.actions-credential-scoping` — the registry secret no longer carries status-write. That does NOT leave the workflow routes provenance-free: any diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index fc4d3bc62..ca83f68eb 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -3671,16 +3671,31 @@ def test_every_tracked_workflow_declares_a_permissions_block(): missing = [] for wf in workflows: doc = yaml.safe_load(wf.read_text()) or {} - if doc.get("permissions") is not None: - continue 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. - if jobs and all((job or {}).get("permissions") is not None for job in jobs): - continue + declares = doc.get("permissions") is not None or ( + bool(jobs) and all((job or {}).get("permissions") is not None for job in jobs) + ) + + # EXEMPT is tested BEFORE `declares`, deliberately. The obvious order (skip anything that + # declares, then handle exemptions) makes the assert below dead code in exactly the case it + # exists for — measured: with that order, giving ci-image.yml a block left this test GREEN. if wf.name in EXEMPT: + # SELF-DELETING. Without this, the day #744 lands and this file gains a block, the + # exemption silently becomes a permanent hole that only #835 remembers — and a follow-up + # issue goes stale exactly like a doc. Redden instead, so the tree tells you. + assert not declares, ( + f"{wf.name} is on this test's EXEMPT list but now DECLARES a permissions: block. The " + "exemption exists only because that file could not be edited without reddening " + "ci-image-pin (it rides with ersatztv#744). If #744 has landed, delete it from " + "EXEMPT here and drop the matching carve-outs in docs/ci-cd.md and " + "ci.actions-credential-scoping — ersatztv#835 tracks exactly that." + ) continue - missing.append(wf.name) + + 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) — except ci-image.yml, which rides with #744 — so " -- 2.47.3 From baf8e8083d03d63b5b21b0ad720529e7ed445ae9 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 22:56:00 +0200 Subject: [PATCH 12/12] docs(748): the phantom key survived one directory outside the sweep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 7 swept `docs/` for citations of `land-toolchain-image-change-separately` — which is a memory filename, not a decision key — and fixed all three. The fourth was in the test docstring the SAME commit restructured: the sweep population was a directory rather than "every place the key is cited". Verified tree-wide this time (`grep -r` excluding .git): zero citations remain. Decisions-Edit: yes Refs: #748 --- scripts/tests/test_pr_changed_files.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index ca83f68eb..196c09262 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -3647,9 +3647,10 @@ def test_every_tracked_workflow_declares_a_permissions_block(): `ci-image.yml` is excluded because a PR that edits it re-points `ci-image-pin`'s `expected` at the editing commit and staleness-fails a BLOCKING job, and because its unfiltered `push:` trigger is path-scoped to itself, so the same edit would publish a toolchain image from the editing branch. - That is why `land-toolchain-image-change-separately` says it lands alone, and it rides with - ersatztv#744. This is the same exemption `ci.actions-credential-scoping` already carries for that - file's `persist-credentials`, and ersatztv#835 tracks removing BOTH once #744 has landed — at + That is why it lands alone (`docs/ci-cd.md` -> "CI toolchain image" documents the two-step an + edit would otherwise force), and it rides with ersatztv#744. This is the same exemption + `ci.actions-credential-scoping` already carries for that file's `persist-credentials`, and + ersatztv#835 tracks removing BOTH once #744 has landed — at which point delete this set rather than adding to it. Adding a second name here is a finding, not a fix: the exemption is one file for one dated reason, not a mechanism. """ -- 2.47.3