From 20b5785b612b09be33a1c84bba765f2a5e8b63f9 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 20:01:37 +0200 Subject: [PATCH 1/8] fix(744,835): main-only push trigger for ci-image.yml, and a guard for persist-credentials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ci-image.yml`'s `push` trigger had no `branches:` filter. Gitea resolves a `push` workflow's definition from the pushed branch, so any branch push touching `docker/ci/**` or the workflow file itself executed that branch's own YAML — no PR, no review, no status check — on a docker-capable runner holding the credential that writes `ersatztv:prod` and the `ersatztv-ci:` five `container:` jobs run. `ci.actions-credential-scoping` bounds what such a job may DO; it never bounded whether it runs. The tradeoff is decided, not asserted away. Branch publishing was load-bearing — `ci-image-pin` is blocking, so a PR changing `docker/ci/**` cannot move its pin without an image published from the branch first. That capability moves to `workflow_dispatch` on the branch, and the resulting three-step (push the docker/ci commit as HEAD, dispatch, commit the pin bump) is documented in ci-cd.md. `.gitea/workflows/ci-image.yml` leaves both its own `paths:` and `ci-image-pin`'s `expected` in the same change, and that is forced rather than preferred: under `branches: [main]` a self-triggering path can only publish AFTER the merge, so `expected` would advance to a commit no image exists for and redden a blocking job on EVERY subsequent PR. It is also what makes this PR landable — with the old `expected`, editing this file re-points it at this commit and `ci-image-pin` goes red for a change altering zero bytes of the image. Cost recorded in the pin job, in ci-cd.md and in the new record: a build-recipe change living only in ci-image.yml no longer republishes. With both mechanical blockers gone, `ci-image.yml`'s checkout takes `persist-credentials: false` — the sixteenth of sixteen — which is what #835 was waiting for. Its guard therefore ships with NO exemption list: population derived from the git index, both `.yml` and `.yaml`, a parser-completeness cross-check against a text scan so an unreachable step list is reported rather than silently uncovered, anti-vacuity in both directions, and a declared clause mutation on the guarded artifact re-run every suite. fixes #744 fixes #835 Decisions-Edit: yes --- .gitea/workflows/ci-image.yml | 35 +++- .gitea/workflows/pr-checks.yml | 36 ++-- docs/ci-cd.md | 94 +++++++--- docs/decisions/README.md | 1 + .../records/ci/actions-credential-scoping.md | 34 ++-- .../toolchain-image-publish-is-a-dispatch.md | 45 +++++ docs/guard-inventory.md | 4 +- scripts/e2e-ui.sh | 3 +- scripts/tests/mutation_manifest.py | 16 ++ .../test_guard_populations_derive_from_git.py | 2 + .../test_workflow_persist_credentials.py | 160 ++++++++++++++++++ 11 files changed, 371 insertions(+), 59 deletions(-) create mode 100644 docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md create mode 100644 scripts/tests/test_workflow_persist_credentials.py diff --git a/.gitea/workflows/ci-image.yml b/.gitea/workflows/ci-image.yml index 6c3be2e09..40aab42c5 100644 --- a/.gitea/workflows/ci-image.yml +++ b/.gitea/workflows/ci-image.yml @@ -4,9 +4,9 @@ name: Build CI Toolchain Image # pushes it to the Gitea container registry (ersatztv#390). The toolchain jobs in # docker-build.yml consume it via `container:`, pinned to an immutable :. # -# push touching docker/ci/** -> : (+ :latest only from main) -# workflow_dispatch -> manual rebuild -# schedule (weekly) -> picks up base-image security updates +# push to MAIN touching docker/ci/** -> : + :latest +# workflow_dispatch (any branch) -> : of that branch's HEAD +# schedule (weekly) -> picks up base-image security updates # # Deliberately separate from docker-build.yml: this image changes rarely (a Dockerfile edit or # the weekly cron), while docker-build.yml runs on every push/PR. Coupling them would rebuild a @@ -22,11 +22,26 @@ name: Build CI Toolchain Image # `http = true` config (it does not inherit the host daemon's insecure-registries setting). on: + # Publishing from a branch is a DELIBERATE act, not a side effect of pushing (ersatztv#744). + # Gitea resolves a `push` workflow's definition from the pushed branch, so an unfiltered `push` + # trigger ran this file's own YAML — attacker-supplied, unreviewed, with no status check in the + # loop — on a docker-capable runner holding the credential that writes `ersatztv:prod` and the + # `ersatztv-ci:` five `container:` jobs execute. `branches: [main]` closes that route. + # `workflow_dispatch` is still self-supplied from the selected branch and is the deliberate + # publish path (docs/ci-cd.md -> "CI toolchain image"); it takes someone who already has write + # access rather than falling out of an ordinary branch push. + # + # `.gitea/workflows/ci-image.yml` is NOT in `paths:`, and that is forced by `branches: [main]` + # rather than a preference: `ci-image-pin` derives its `expected` from the last commit touching + # the image sources, so if this file were a source, editing it would re-point `expected` at a + # commit whose image can only publish AFTER the merge — leaving every subsequent PR red on a + # blocking job until someone dispatched and re-pinned. The cost is stated in ci-cd.md: a change + # to HOW the image is built that lives only in this file no longer republishes on its own. workflow_dispatch: push: + branches: [main] paths: - 'docker/ci/**' - - '.gitea/workflows/ci-image.yml' schedule: # Mondays 05:00 UTC. Gitea registers `schedule` only from the default branch (main). # @@ -65,6 +80,13 @@ jobs: - name: Checkout uses: actions/checkout@v4 with: + # ersatztv#746's convention, applied here once #744 removed the reason it was skipped: + # without it the action leaves a write-capable Authorization header in .git/config for + # every later step. Nothing here pushes with git — the only git call is the + # `rev-parse --short HEAD` below — and the repo is public, so the clone needs no + # credential of its own. Guarded for every workflow by + # scripts/tests/test_workflow_persist_credentials.py (ersatztv#835). + persist-credentials: false # only docker/ci/Dockerfile is needed; no git describe/log here fetch-depth: 1 @@ -76,7 +98,10 @@ jobs: # Always publish the immutable : — that is what docker-build.yml pins. TAGS=("${CI_IMAGE}:${SHORT}") # :latest is a convenience/floating pointer for humans and the weekly rebuild; jobs must - # never consume it. Only main may move it. + # never consume it. Only main may move it — and since #744 the `push` trigger is + # main-only, so on this path the branch check is satisfied by construction. It stays + # because `workflow_dispatch` still selects any branch, which is exactly the case that + # must not move `:latest`. if [ "${GITHUB_REF}" = "refs/heads/main" ]; then TAGS+=("${CI_IMAGE}:latest") fi diff --git a/.gitea/workflows/pr-checks.yml b/.gitea/workflows/pr-checks.yml index 8f39aef05..0b3cf192d 100644 --- a/.gitea/workflows/pr-checks.yml +++ b/.gitea/workflows/pr-checks.yml @@ -51,9 +51,11 @@ jobs: # it manages docker/ci/Dockerfile's base pins (dockerfile manager) but cannot bump an opaque # `:` in `container.image`, so it would leave the pin behind every time. # - # Failing here forces the documented two-step (docs/ci-cd.md -> "CI toolchain image"): push the - # Dockerfile change, let ci-image.yml publish `:`, then update the pin to that sha. Seconds-long - # git+grep -> keep it off the build runners. + # Failing here forces the documented two-step (docs/ci-cd.md -> "CI toolchain image"): get the + # Dockerfile change published as `:`, then update the pin to that sha. Since ersatztv#744 the + # publish half of that two-step is a `workflow_dispatch` on the branch rather than a side effect of + # the push — ci-image.yml's `push` trigger is now `branches: [main]`. Seconds-long git+grep -> keep + # it off the build runners. ci-image-pin: name: CI image pin matches docker/ci runs-on: small @@ -69,14 +71,26 @@ jobs: - name: Verify the pin matches the last-published image run: | set -euo pipefail - # ci-image.yml tags the image `git rev-parse --short HEAD` of the push that built it, and it - # only builds on pushes touching these paths — so the published image is named by the last - # commit to touch them. + # ci-image.yml tags the image `git rev-parse --short HEAD` of the run that built it, and it + # only builds when `docker/ci/**` changes — so the published image is named by the last + # commit to touch that path. + # + # `.gitea/workflows/ci-image.yml` is deliberately NOT part of `expected` (ersatztv#744). + # It was, back when that file was one of its own trigger paths. It no longer is: under + # `branches: [main]` a self-triggering path would only publish AFTER the merge, so + # `expected` would advance to a commit no image yet exists for and redden this blocking + # job on every subsequent PR. The two must move together, and they did. + # + # What that costs, stated rather than assumed away: a change to HOW the image is built + # that lives ONLY in ci-image.yml (build-args, Dockerfile path, platforms) neither + # republishes nor invalidates the pin, so CI keeps running an image built by the previous + # recipe. Make such a change alongside a `docker/ci/**` edit, or dispatch + re-pin — see + # docs/ci-cd.md -> "CI toolchain image". # # Compare RESOLVED FULL shas, never the abbreviations: git auto-scales abbreviation length # with the repo's object count, so the tag built in CI from a `fetch-depth: 1` shallow clone # is 7 chars while `%h` here (full clone) is 8. Comparing those strings would fail always. - expected="$(git log -1 --format=%H -- docker/ci .gitea/workflows/ci-image.yml)" + expected="$(git log -1 --format=%H -- docker/ci)" mapfile -t pins < <(grep -oE 'ersatztv-ci:[0-9a-f]+' .gitea/workflows/docker-build.yml | cut -d: -f2 | sort -u) echo "Image sources last changed in: ${expected}" echo "Pins found in docker-build.yml: ${pins[*]} (${#pins[@]} distinct)" @@ -108,10 +122,10 @@ jobs: # in-repo remedy in that state: relax this length check in the same PR and say why. Note # that ci-image.yml still tags with a plain `--short` (auto-scaled), so "always 7" is an # empirical property of today's shallow clone, not an enforced invariant. Making the - # publisher emit `--short=7` is tracked as ersatztv#597. It is not blocked, just out of - # scope here: editing ci-image.yml re-points `expected` (above) at that commit, so it needs - # the branch's own publish-then-pin two-step (docs/ci-cd.md -> 'CI toolchain image') — - # ci-image.yml's push trigger has no branches: filter, so a feature branch does publish. + # publisher emit `--short=7` is tracked as ersatztv#597. That is no longer blocked by this + # job at all: since ersatztv#744, editing ci-image.yml does NOT re-point `expected`, so a + # `--short=7` change lands like any other PR. It does need a deliberate republish to take + # effect — see the note on `expected` above. if [ "${#pins[0]}" -ne 7 ]; then echo "::error::CI toolchain image pin ersatztv-ci:${pins[0]} is ${#pins[0]} chars, but ci-image.yml publishes 7-char tags (it tags with 'git rev-parse --short HEAD' from a fetch-depth:1 clone). A differently-sized abbreviation still resolves to the right commit, so this would pass every other check here — but NO such tag exists in the registry, and all five container: jobs would fail at image-pull time with 'manifest unknown'. Pin exactly: ersatztv-ci:${expected:0:7} (locally: git rev-parse --short=7 HEAD). See docs/ci-cd.md -> 'CI toolchain image'." exit 1 diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 28a4d3621..64479e764 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -149,8 +149,12 @@ A backup, fetch, or migration-smoke failure aborts before the live container is ## 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 +**Every** `actions/checkout` in this repo sets `persist-credentials: false` — with no exemption, +since ersatztv#744 removed the one that existed (see "The last exemption, and how it closed" below), +and ersatztv#835 now holds the rule mechanically rather than by convention: +`scripts/tests/test_workflow_persist_credentials.py` derives the workflow population from the git +index and asserts the flag on every checkout step it finds. 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 @@ -219,10 +223,18 @@ other three consumers — `test`, `migrations` and `functional-e2e` — whose ch shallow (`fetch-depth: 2`). `build` is the only one that checks out complete, which is why it is the only one that grafts. -**`ci-image.yml` is the one exception** and is tracked in ersatztv#744, which has to change that -file alone: any commit touching `.gitea/workflows/ci-image.yml` re-points `ci-image-pin`'s -`expected` at itself and reddens that blocking job, and the file's unfiltered `push` trigger -publishes a toolchain image from whatever branch the edit lands on. +### The last exemption, and how it closed + +`ci-image.yml`'s checkout was the one left unset, and the reason was mechanical rather than a +judgement that it was safe: any commit touching `.gitea/workflows/ci-image.yml` re-pointed +`ci-image-pin`'s `expected` at itself and reddened that blocking job, and the file's unfiltered +`push` trigger published a toolchain image from whatever branch the edit landed on. Both are gone +(ersatztv#744): the trigger is `branches: [main]` and the file is no longer one of `expected`'s +paths, so the flag went on in the same change. **What this costs the guard is nothing, and that is +the point** — ersatztv#835 was deliberately held back rather than shipped with a one-entry +`ci-image.yml` exemption, because such an entry outlives its reason silently: once the flag went on +there the exemption would still have passed and the guard would have been permanently blind to the +file it was written for. ## The workflow: `.gitea/workflows/docker-build.yml` @@ -1671,7 +1683,7 @@ so a missing system library fails the image build rather than a CI run. ⚠️ **`ARG PLAYWRIGHT_VERSION` must equal `web/package.json`'s `@playwright/test` pin**, which is deliberately **EXACT** (no caret): Playwright ties a browser *revision* to the package version, so a mismatch leaves no usable browser. Renovate bumps the npm pin but cannot know about this ARG — when it -does, bump the ARG, let `ci-image.yml` publish the new `:`, then update all five container pins. +does, bump the ARG, publish the new `:` (step 1 below), then update all five container pins. `scripts/e2e-ui.sh` guards the drift by *launching* a browser up front and failing with exactly that instruction (it probes by launch, not by path, because `chromium.executablePath()` reports the full-chromium path that a headless-shell-only image deliberately lacks). @@ -1684,11 +1696,14 @@ SDK matches the base's glibc/ICU. Keep the ffmpeg tag on that `FROM` equal to th ersatztv#299 seeded-media/scanner E2E follow-ups will need. **Bumping the toolchain is a deliberate two-step.** The jobs pin an immutable `:`, never -`:latest`, so a bad toolchain push cannot break every job at once: +`:latest`, so a bad toolchain push cannot break every job at once. Both steps land in the **same +PR** — `ci-image-pin` is blocking, so a PR that changes `docker/ci/**` without moving the pin is red: -1. Merge a `docker/ci/Dockerfile` change. `ci-image.yml` publishes `ersatztv-ci:` (+ `:latest` - from `main` only — a human pointer; **jobs must never consume it**). -2. In a follow-up PR, update the pin in `docker-build.yml` — **all five jobs together**. That PR's own +1. **Publish.** Push the branch with the `docker/ci/Dockerfile` change as HEAD, then dispatch + `ci-image.yml` on that branch; it publishes `ersatztv-ci:`. (`:latest` moves only from + `main` — a human pointer; **jobs must never consume it**.) Since ersatztv#744 a branch *push* + does not publish; see "Publishing from a branch is a dispatch, not a push" below. +2. **Pin.** Update the pin in `docker-build.yml` — **all five jobs together** — on top. That PR's own CI is what proves the new image works. The pin is repeated per job because `jobs..container.image` cannot read the workflow `env` context. @@ -1713,8 +1728,9 @@ registry outage rather than a one-character pin error. `ci-image-pin` therefore > emits 8, the correct pin becomes 8, and the gate's hardcoded 7 goes permanently red demanding a tag > with no image behind it. Making the publisher emit `--short=7` is tracked as ersatztv#597. -`ci-image.yml` triggers on pushes touching `docker/ci/**`, `workflow_dispatch`, and a weekly Monday -05:00 UTC cron (base-image security updates; Gitea registers `schedule` only from `main`). It runs on +`ci-image.yml` triggers on pushes **to `main`** touching `docker/ci/**`, `workflow_dispatch` on any +branch, and a weekly Monday 05:00 UTC cron (base-image security updates; Gitea registers `schedule` +only from `main`). It runs on `ubuntu-latest` — it was on `small` until server-management#639, where "docker-only" was found to be a poor proxy for "small": this is a full buildx of the .NET toolchain image, the heaviest job in that lane. Like `docker-build.yml`, it needs BuildKit's inline `http = true` for the HTTP @@ -1739,17 +1755,42 @@ registry. Renovate tracks the Dockerfile's image pins (`dockerfile` manager, see ⚠️ **A REBASE invalidates the pin.** The pin must equal the short sha of the commit that touched `docker/ci/**`, and a rebase **rewrites that commit's sha** — so `ci-image-pin` goes red on a branch that was green before, with a pin that still resolves to a real (now-orphaned) commit and an image -that still exists in the registry. Worse, the force-push usually does **not** rebuild: `ci-image.yml` -filters on `paths: docker/ci/**`, and a rebase that doesn't change the Dockerfile's *content* -produces no diff for that path, so nothing republishes. And you cannot simply re-dispatch it — -`ci-image.yml` tags `git rev-parse --short HEAD`, i.e. whatever the branch HEAD is when it runs, not -the commit that touched `docker/ci`. Those two coincide only when the `docker/ci` commit *is* HEAD. +that still exists in the registry. -Recovery (ersatztv#445 hit this): make the `docker/ci` commit be HEAD again — push a commit that -really does change `docker/ci/**`, let `ci-image.yml` publish `:`, then bump the pin in -a follow-up commit. That is the same two-step below, just re-run after the rebase. The cheapest way to -avoid it entirely is to **land a toolchain-image change on its own, before** the work that consumes -it, so the consuming branch never carries the `docker/ci` commit through a rebase. +### Publishing from a branch is a dispatch, not a push (ersatztv#744) + +**A branch push no longer publishes anything.** `ci-image.yml`'s `push` trigger is `branches: [main]`, +because Gitea resolves a `push` workflow's definition from the pushed branch — so an unfiltered +trigger meant any branch push touching those paths executed that branch's own YAML, unreviewed, on a +docker-capable runner holding the credential that writes `ersatztv:prod` and the toolchain image +five `container:` jobs run. Credential scoping (`ci.actions-credential-scoping`) bounds what such a +job may *do*; it never bounded whether it runs. + +The capability that removed is real and is **not** assumed away — it is replaced by +`workflow_dispatch`, which Gitea loads from the branch you select. The constraint to plan around is +that `ci-image.yml` tags `git rev-parse --short HEAD`, i.e. **the branch HEAD at dispatch time**, not +the commit that touched `docker/ci`. So the branch-side two-step is: + +1. Push the branch with the `docker/ci/**` change as **HEAD**. +2. Dispatch `ci-image.yml` on that branch (Actions → *Build CI Toolchain Image* → *Run workflow* → + pick the branch). It publishes `:`; `:latest` stays put, because only + `main` may move it. +3. Commit the pin bump in `docker-build.yml` on top. HEAD moves, but nothing needs republishing — + `expected` is the `docker/ci` commit, which the pin now names. + +After a rebase, redo all three: the sha is new, so the image must be republished under it. The +cheapest way to avoid the loop entirely is +still to **land a toolchain-image change on its own, before** the work that consumes it, so the +consuming branch never carries the `docker/ci` commit through a rebase. + +**A change that lives only in `ci-image.yml` publishes nothing.** That file is no longer one of its +own trigger paths, nor one of `ci-image-pin`'s `expected` paths — the two had to move together, +because under `branches: [main]` a self-triggering path only publishes *after* the merge, which would +advance `expected` to a commit no image exists for and redden a blocking job on every subsequent PR. +The consequence to live with: if you change **how** the image is built (build-args, Dockerfile path, +platforms) without touching `docker/ci/**`, CI keeps running an image built by the previous recipe +and nothing goes red. Either make the change alongside a `docker/ci/**` edit, or dispatch on `main` +after the merge and bump the pin. ### When the pinned tag disappears @@ -1762,8 +1803,8 @@ three different claims, and existence is the one that is not ours to make: the r server-management, and an owner-level Gitea *package cleanup rule* there (`keep_count` 15, `remove_days` 1, `remove_pattern` `.*`, and a `keep_pattern` that no 7-hex sha can match) deletes any sha tag once 15 newer versions of the package exist. `ci-image.yml` publishes a new `:` weekly -and on every push touching `docker/ci/**` or the workflow file, while the pin only moves when a human -bumps it — so a pin ages toward eviction on its own. That is what happened between 2026-08-11 and +and on every push to `main` touching `docker/ci/**` (plus every branch dispatch), while the pin only +moves when a human bumps it — so a pin ages toward eviction on its own. That is what happened between 2026-08-11 and 2026-08-13 (ersatztv#772): the tag vanished, and every `container:` job — **both required contexts included** — died after 1–2s with @@ -1885,7 +1926,8 @@ in `pr-checks.yml`, but it greps `docker-build.yml` where the pins live) fails i different tags. This exists because **Renovate manages `docker/ci/Dockerfile`'s base pins but cannot bump an opaque `:`** in `container.image` — so a Renovate base bump would otherwise publish a new image, test the *old* one, and merge with the -Dockerfile disagreeing with the pin. A red `ci-image-pin` means: let `ci-image.yml` publish the new +Dockerfile disagreeing with the pin. A red `ci-image-pin` means: dispatch `ci-image.yml` on the +branch to publish the new `:`, then update all five pins to it. **What it is and isn't worth.** Measured honestly (ersatztv#390): the image saves ~15–40s per job diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 05fd2adee..4334b16aa 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -65,6 +65,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.script-tests-job` | The `scripts/tests/` pytest suite runs on every PR as a dedicated `script-tests` job in `pr-checks.yml` (`runs-on: small`, `setup-python` + `pip install pytest pyyaml`, `PYTHONPATH=. python3 -m pytest scripts/tests -q`; since #780 it also runs a pinned ruff over a `git ls-files` population first), unconditionally rather than behind a `scripts/**` path filter, and **never as a step inside `decisions-guard`** — a job whose reds a standing rule instructs sessions to ignore must never host a gate whose reds are real. Any new CI gate must be reachable by a failure that is unambiguously attributable to it. | 2026-07-26 | [link](records/ci/script-tests-job.md) | | `ci.shared-pr-file-enumeration` | A PR's complete set of changed file paths is computed by exactly one implementation, `scripts/pr-changed-files.sh`, called by both `.claude/hooks/pretooluse-merge-consent.sh` (advisory — a failure falls through to a human prompt) and `.gitea/workflows/review-verdict.yml` (enforced — a failure must fail closed, because a match here posts the branch-protection-required `review-verdict/h10` status with nobody in the loop). The script owns exhaustiveness (pagination, rename/path validation, head-sha binding, base-ref binding — see `ci.exemption-provenance` — and base-TIP binding, #707: the ref answers "did this PR RETARGET", the tip answers "did the base ADVANCE mid-enumeration", and only the second can see `/pulls/{n}/files` recomputing each offset-paged page against a moved base and dropping a path out of an already-consumed range; both ends of the window are bound, and an advance BEFORE the window is deliberately not an error, or ordinary churn on `main` would fail every open PR) and returns exit 0 only for a verified-complete list; it does NOT classify paths — each caller keeps its own docs-only allow-list, and the two allow-lists differ on purpose and stay separate. | 2026-07-26 | [link](records/ci/shared-pr-file-enumeration.md) | | `ci.small-lane-git-only` | `runs-on: small` is defined by what a job does (git-only), not its usual runtime; the two `docker build` jobs (docker-build.yml, ci-image.yml) move to `ubuntu-latest` because their worst-case memory, not median runtime, was pinning the small lane's per-slot cap. | 2026-07-20 | [link](records/ci/small-lane-git-only.md) | +| `ci.toolchain-image-publish-is-a-dispatch` | A workflow whose `push` trigger has no `branches:` filter executes attacker-supplied YAML, because Gitea resolves a `push` workflow's definition from the pushed branch — so `ci-image.yml` is `branches: [main]`, and publishing a toolchain image from a feature branch is a deliberate `workflow_dispatch` on that branch rather than a side effect of pushing. The self-referencing trigger path `.gitea/workflows/ci-image.yml` came out of BOTH its own `paths:` and `ci-image-pin`'s `expected` in the same change: under a main-only trigger a self-triggering path can only publish AFTER the merge, so `expected` would advance to a commit no image exists for and redden a blocking job on every subsequent PR. The cost is stated, not assumed away — a change to HOW the image is built that lives only in `ci-image.yml` no longer republishes, so make it alongside a `docker/ci/**` edit or dispatch and re-pin. | 2026-08-27 | [link](records/ci/toolchain-image-publish-is-a-dispatch.md) | | `ci.ui-e2e-harness` | The UI-interactive E2E flows run as headless Playwright specs (`web/e2e/*.spec.ts`, driven by `scripts/e2e-ui.sh`) in a **second step of the existing advisory `functional-e2e` job**, never their own job; the browser is `chromium-headless-shell` **baked into the CI toolchain image** (`docker/ci/Dockerfile`, `PLAYWRIGHT_VERSION` kept equal to `web/package.json`'s EXACT `@playwright/test` pin), never installed per run; specs are `serial` with `retries: 0` and assert only contracts the curl harness structurally cannot reach. | 2026-07-25 | [link](records/ci/ui-e2e-harness.md) | | `ci.verdict-write-retarget-fence` | The `review-verdict/h10` job counts `change_target_branch` events on the PR's issue timeline at run start and again immediately before its POST, and writes NOTHING if the count moved. The COUNT is the key because the branch NAME is ABA-vulnerable — `main -> S -> main` reads `main` at both ends, which is how #698 route 1 obtained a forged exemption — while the event count is monotonic and cannot alias. This NARROWS the residual, it does not resolve it — a retarget between the FINAL pre-write count and the POST still yields a PERMANENT forged green, because the successor run can consume the `edited` event and exit before the stale run posts last (corrected 2026-08-27, #849). Abstaining is a handoff, not a stall, and that is the property the design rests on — for a run that ABSTAINS; it says nothing about one that already passed its final count and then posts (see the residual, corrected #849): every retarget fires `edited`, which is in this workflow's `types:`, so the event that makes a run abstain has already queued a successor whose window opens after it; the induction terminates when retargeting stops — but ONLY over runs that ABSTAIN. A run that already passed its final count is outside it: it writes whenever it gets there, so the run that writes LAST is not necessarily the one that classified last, and that is the permanent residual (#849). `updated_at` was REJECTED as the key because it also moves for comments and labels, which fire none of this workflow's `types:` — a run could abstain with no successor coming, which is a real stall. The count is trusted only when paging reached a validated EMPTY page; an untrusted count (unreadable page, non-array body, non-numeric length, page cap hit) blocks the exemption `success` ONLY and still lets `pending` through, because `pending` blocks the merge immediately while withholding it would strand ordinary PRs whenever the timeline is unreadable — the right trade, but NOT a free one ("for no safety gain" retracted 2026-08-27): a GENERIC `pending` masks a rejection landing in its own write window, post-write verification does not run for it, and a later run re-derives it into an exemption `success` (#849). SEPARATELY, and for the human-verdict race the fence does nothing about: after posting an exemption `success` the job re-reads `/statuses/{sha}` and, if a human `Review-verdict:` row appeared with an id ABOVE a high-water mark taken just before the POST, overwrites its own status with `pending` and logs an error. The repair is `pending`, NEVER a copy of the human's state, since re-posting their `failure` under the machine credential would attribute a human verdict to the job; its description is a SENTINEL that the classification refuses to grant an exemption over AND re-writes verbatim on every later run, so the block is a FIXED POINT rather than decaying — writing the generic `pending` description there instead erases the marker and the exemption simply returns one event later. The mark is captured BEFORE the last-moment re-read, not merely before the POST — a later mark leaves a multi-round-trip blind gap in which a verdict is neither seen by the re-read nor repaired afterwards. The id comparison is load-bearing: a mere presence test would fire forever on a base-mismatched verdict that `read_existing_verdict` deliberately declines to honour, deadlocking that PR's exemption permanently. Finally, a run whose last-moment re-read finds a sentinel it did not see at its FIRST read ABSTAINS instead of posting: that can only mean an overlapping run repaired a raced verdict mid-flight, and this run's `success` — frozen at classification time, with the human row below its own mark, so neither the fence nor the post-write check would catch it — would otherwise bury the rejection. That is the one path in this design that failed toward SUCCESS rather than `pending`. The post-write check counts TWO row shapes above the mark, not one — a human `Review-verdict:` row AND a machine sentinel — because with two overlapping runs the human row can sit BELOW the second run's mark while the first masks it and only then writes the sentinel, leaving the second to post its own `success` on top; counting the sentinel converges both runs on the fixed point instead. | 2026-08-03 | [link](records/ci/verdict-write-retarget-fence.md) | | `ci.verify-locally-ci-confirms` | Treat the local build/verify/review pass as the decision point and CI as confirmation — don't idle waiting on a run you have no reason to doubt. | 2026-07-21 | [link](records/ci/verify-locally-ci-confirms.md) | diff --git a/docs/decisions/records/ci/actions-credential-scoping.md b/docs/decisions/records/ci/actions-credential-scoping.md index 542df3f1c..f59a1d25b 100644 --- a/docs/decisions/records/ci/actions-credential-scoping.md +++ b/docs/decisions/records/ci/actions-credential-scoping.md @@ -17,13 +17,14 @@ instance. **Why the credential and not only the triggers.** Patching triggers enumerates *instances* of "a ref-resolved workflow obtains status-capable credentials", and adding a new workflow file is itself a -route, so that enumeration never completes. But it is not either/or: `ci-image.yml`'s unfiltered -`push:` is path-scoped to itself, so any branch push runs attacker YAML on a docker-capable runner -with no PR. Scoping bounds what a job may DO; only a filter bounds whether it RUNS. That filter is -**#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`). +route, so that enumeration never completes. But it is not either/or: `ci-image.yml`'s `push:` was +unfiltered and path-scoped to itself, so any branch push ran attacker YAML on a docker-capable runner +with no PR. Scoping bounds what a job may DO; only a filter bounds whether it RUNS. That filter landed +in **#744**, not this record, and its own decision is `ci.toolchain-image-publish-is-a-dispatch`: +`branches: [main]`, with branch publishing moved to a deliberate `workflow_dispatch`. It had to land +alone (`land-toolchain-image-change-separately`) because, until it also removed the file from +`ci-image-pin`'s `expected` paths, editing `ci-image.yml` re-pointed that job at the editing commit +and staleness-failed a **blocking** check on every rebase. **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 @@ -66,8 +67,8 @@ code can no longer escalate to instance admin — NOT that the durable forgery r bound — auditing it rather than the workflow set is what found `RENOVATE_TOKEN` and `SERVERMGMT_DEPLOY_KEY` below, since any PR-added workflow can reference any secret. But `GITEA_TOKEN` is injected and never in the store; nor is the credential -`actions/checkout` persists into `.git/config` (dropped on 15 of the repo's 16 checkouts in #746; -`ci-image.yml` is excluded and is head-resolved in the sense that matters — see below); and jobs +`actions/checkout` persists into `.git/config` (dropped on 15 of the repo's 16 checkouts in #746 and +on the sixteenth in #744 — see below); and jobs reach the runner's docker daemon. **The persisted checkout credential (#746).** `actions/checkout` writes an `Authorization` header @@ -91,12 +92,15 @@ three-dot diff against a branch point further back than N fails `no merge base` `|| true`, and without it a red `api-docs`/`Formatting` (`decisions lifecycle` would stay green with its diff-dependent checks skipped, because its validator fails open on an unresolvable base). -`ci-image.yml`'s single checkout is the one exception, and NOT because it is safe: its unfiltered -`push` trigger makes it head-resolved in the sense that matters. It is excluded because editing that -file re-points `ci-image-pin`'s `expected` at the editing commit and reddens a blocking job, and -because the same trigger would publish a toolchain image from the editing branch. It rides with -#744, which exists to change that file alone. #835 tracks the guard for this convention: it can only -be written without an exemption list once #744 has landed. +`ci-image.yml`'s single checkout was the one exception, and never because it was safe — its +unfiltered `push` trigger made it head-resolved in the sense that matters. It was excluded because +editing that file re-pointed `ci-image-pin`'s `expected` at the editing commit and reddened a blocking +job, and because the same trigger would publish a toolchain image from the editing branch. #744 +removed both (`ci.toolchain-image-publish-is-a-dispatch`) and set the flag in the same change, so the +count is now **16 of 16 with no exemption**. That is what let #835 ship the guard — +`scripts/tests/test_workflow_persist_credentials.py`, population derived from the git index — with no +exemption list at all: a one-entry list justified by a temporary condition elsewhere keeps passing +after that condition expires, leaving the guard permanently blind to the file it was written for. **Measured vs inferred.** Measured here: the `v1.25.4` scope enum (`access_token_scope.go`) has no `status` entry; the `reqRepoWriter` gate (`routers/api/v1/api.go`); the probes in `mechanics`. Read from diff --git a/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md b/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md new file mode 100644 index 000000000..7e11412b4 --- /dev/null +++ b/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md @@ -0,0 +1,45 @@ +--- +key: ci.toolchain-image-publish-is-a-dispatch +title: '2026-08-27 — `ci-image.yml` publishes on `push` to main only; from a branch it is a `workflow_dispatch` (#744)' +status: active +since: '2026-08-27' +supersedes: none +superseded-by: none +rule: 'A workflow whose `push` trigger has no `branches:` filter executes attacker-supplied YAML, because Gitea resolves a `push` workflow''s definition from the pushed branch — so `ci-image.yml` is `branches: [main]`, and publishing a toolchain image from a feature branch is a deliberate `workflow_dispatch` on that branch rather than a side effect of pushing. The self-referencing trigger path `.gitea/workflows/ci-image.yml` came out of BOTH its own `paths:` and `ci-image-pin`''s `expected` in the same change: under a main-only trigger a self-triggering path can only publish AFTER the merge, so `expected` would advance to a commit no image exists for and redden a blocking job on every subsequent PR. The cost is stated, not assumed away — a change to HOW the image is built that lives only in `ci-image.yml` no longer republishes, so make it alongside a `docker/ci/**` edit or dispatch and re-pin.' +signals: 'unfiltered push trigger, branches filter on push, attacker YAML on a runner, feature branch publishes an image, self-referencing workflow trigger, ci-image-pin expected goes stale, publish-then-pin two-step, toolchain image recovery after a rebase, workflow_dispatch on a branch · paths: `.gitea/workflows/ci-image.yml`, `.gitea/workflows/pr-checks.yml`, `docs/ci-cd.md`, `scripts/e2e-ui.sh` · issues: #744, #697, #746, #835, #445, #390, #594, #597' +mechanics: '`docs/ci-cd.md` -> "CI toolchain image" -> "Publishing from a branch is a dispatch, not a push"' +--- + +- **Credential scoping bounds what a job may DO; it never bounded whether the job RUNS.** That is the + seam `ci.actions-credential-scoping` explicitly leaves open, and this record closes one route through + it. `ci-image.yml`'s `push` trigger was path-scoped but unfiltered by branch, and Gitea resolves a + `push` workflow's definition from the pushed branch — so any branch push touching `docker/ci/**` or + the workflow file ran that branch's own YAML, with no PR, no review and no status check in the loop, + on a docker-capable runner holding the credential that writes `ersatztv:prod` and the + `ersatztv-ci:` that five `container:` jobs execute. `branches: [main]` closes it. + `workflow_dispatch` remains self-supplied from the selected branch and is not equivalent: it takes a + deliberate act by someone who already has write access. +- **The capability removed was real, so it is replaced rather than argued away.** An earlier attempt + (#697) justified the filter with "publishing from a feature branch was not load-bearing", which is + false: `ci-image-pin` is blocking, so a PR changing `docker/ci/**` without moving the pin is red, and + the only way to move the pin was to publish from the branch first. The replacement is the same + two-step with a dispatch in place of the push — push the `docker/ci` commit as HEAD, dispatch + `ci-image.yml` on that branch (it tags `git rev-parse --short HEAD`, so the two must coincide), then + commit the pin bump on top. A rebase rewrites the sha, so all three are redone; that was already true. +- **The self-path had to go with it, and that is forced rather than preferred.** `ci-image-pin` derives + `expected` from the last commit touching the image sources. While `.gitea/workflows/ci-image.yml` was + one of them, editing that file re-pointed `expected` at the editing commit — an annoyance under a + branch-publishing trigger (publish, then pin, inside the PR) but a **repo-wide** red under a main-only + one, since the image can then only be published after the merge. Every subsequent PR would be blocked + until someone dispatched and re-pinned. The two changes are one change. +- **What that costs, recorded because it is invisible otherwise:** a change to how the image is built + that lives only in `ci-image.yml` — build-args, Dockerfile path, platforms — neither republishes nor + invalidates the pin, so CI keeps running an image built by the previous recipe and nothing goes red. + The remedy is procedural: pair such a change with a `docker/ci/**` edit, or dispatch on `main` after + the merge and bump the pin. The weekly cron does not cover this — it moves `:latest`, never a pin. +- **The persist-credentials exemption was downstream of this, and closed with it.** `ci-image.yml`'s + checkout was the one left without `persist-credentials: false` (#746), for exactly the two mechanical + reasons above. Both gone, the flag went on in the same change, which is what let #835 ship its guard + with **no exemption list** — an exemption whose only justification is a temporary condition elsewhere + keeps passing after that condition expires, leaving the guard permanently blind to the file it was + written for. diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index 16d7ae765..d92180ce9 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -134,6 +134,7 @@ The audit #806 asked for, recorded whichever way it came out, because "we looked | `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_workflow_persist_credentials.py` | `.gitea/workflows/*.yml` + `*.yaml` | **yes** — every `actions/checkout` in every workflow must drop the persisted credential | derived from the index from the start (#835). Shipped with NO exemption list, which is why it waited for #744: the one non-compliant checkout would otherwise have bought a permanent entry that keeps passing after its reason expires | | `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 | @@ -220,6 +221,7 @@ recorded as unexamined rather than as cleared. | `scripts/tests/test_prove_fix.py` | the `script-tests` job | PROOF | NONE | — | | `scripts/tests/test_review_verdict_vocabulary.py` | the `script-tests` job, when the H10 verdict vocabulary stops being shared — a word added to the one declaration that fails to reach the write side or the read side | GUARD | MUTATION | `test_review_verdict_vocabulary.py::test_a_word_added_to_the_shared_source_reaches_BOTH_sides` | | `scripts/tests/test_remote_state_inventory.py` | the `script-tests` job, on an executable that talks to a remote service with no row in `docs/remote-state-inventory.md` | GUARD | MUTATION | `test_remote_state_inventory.py::test_every_in_scope_file_has_a_row_and_every_row_names_a_real_file` | +| `scripts/tests/test_workflow_persist_credentials.py` | the `script-tests` job, when an `actions/checkout` step no longer sets `persist-credentials: false` | GUARD | MUTATION | `test_workflow_persist_credentials.py::test_every_actions_checkout_DROPS_the_persisted_credential` | | `scripts/tests/test_worktree_ownership_guard.py` | the `script-tests` job | PROOF | NONE | — | ## The `MUTATION` column is executed, not asserted @@ -242,7 +244,7 @@ clause-provable and the entry is regraded. ## What the numbers say -39 guards, 6 tooling scripts, 20 proof files. **17 guards carry a mutation proof; 6 are +40 guards, 6 tooling scripts, 20 proof files. **18 guards carry a mutation proof; 6 are behaviour-only; 16 have none.** These figures are asserted against the table by `test_the_summary_counts_match_the_table` — they were wrong in the first draft (28/4/6/3/19 against a table holding 27/5/6/3/18), because a hand-maintained summary of a table is a second copy of it, diff --git a/scripts/e2e-ui.sh b/scripts/e2e-ui.sh index 8b90a724d..90e478f5f 100755 --- a/scripts/e2e-ui.sh +++ b/scripts/e2e-ui.sh @@ -54,7 +54,8 @@ if ! (cd "$REPO_ROOT/web" && node -e "require('@playwright/test').chromium.launc rm -f "$PW_PROBE_ERR" echo " In CI this means the browser baked into the toolchain image no longer matches" >&2 echo " web/package.json's @playwright/test pin (${PW_VERSION}): bump PLAYWRIGHT_VERSION in" >&2 - echo " docker/ci/Dockerfile to match, let ci-image.yml publish the new :, then update" >&2 + echo " docker/ci/Dockerfile to match, dispatch ci-image.yml on the branch to publish the" >&2 + echo " new : (a branch push no longer publishes -- ersatztv#744), then update" >&2 echo " all five container pins in docker-build.yml (docs/ci-cd.md -> 'CI toolchain image')." >&2 echo " Locally: cd web && npx playwright install chromium" >&2 exit 1 diff --git a/scripts/tests/mutation_manifest.py b/scripts/tests/mutation_manifest.py index 0c5509980..679d6635d 100644 --- a/scripts/tests/mutation_manifest.py +++ b/scripts/tests/mutation_manifest.py @@ -262,6 +262,22 @@ MUTATIONS: tuple[Mutation, ...] = ( "identical: emptying or corrupting the word list reddens the proof through the fail-closed " "validator, which says nothing about whether the read side still DERIVES from it.", ), + Mutation( + guard="scripts/tests/test_workflow_persist_credentials.py", + target=".gitea/workflows/ci-image.yml", + clause=""" persist-credentials: false + # only docker/ci/Dockerfile is needed; no git describe/log here""", + replacement=""" # only docker/ci/Dockerfile is needed; no git describe/log here""", + proof="test_workflow_persist_credentials.py::test_every_actions_checkout_DROPS_the_persisted_credential", + granularity=CLAUSE, + expect="persists a write-capable Authorization header into .git/config", + why="THE GUARD IS A TEST, so the mutation goes into the guarded ARTIFACT rather than into " + "the checker, per the checker-guard exception. The defect is the one the guard exists for: " + "an `actions/checkout` step that simply omits the key, which is what a newly added job gets " + "by default — not an explicit `true`, which nobody writes. `ci-image.yml` carries exactly " + "one checkout, so the clause is unambiguous there; the trailing comment line is part of it " + "only to make the match unique within the file.", + ), ) diff --git a/scripts/tests/test_guard_populations_derive_from_git.py b/scripts/tests/test_guard_populations_derive_from_git.py index 120b5e1ae..50278aa5c 100644 --- a/scripts/tests/test_guard_populations_derive_from_git.py +++ b/scripts/tests/test_guard_populations_derive_from_git.py @@ -44,6 +44,7 @@ from scripts.tests import test_guard_inventory as guard_inventory from scripts.tests import test_hook_fire_log as hook_fire from scripts.tests import test_pr_changed_files as pr_changed from scripts.tests import test_remote_state_inventory as remote_state +from scripts.tests import test_workflow_persist_credentials as persist_creds from scripts.tests import tracked_files REPO_ROOT = Path(__file__).resolve().parents[2] @@ -108,6 +109,7 @@ DERIVATIONS = ( ("test_ci_image_pin_population.workflow_files", image_pin.workflow_files, 5), ("test_remote_state_inventory.derived_population", remote_state.derived_population, 40), ("test_pr_changed_files._workflow_files", pr_changed._workflow_files, 5), + ("test_workflow_persist_credentials.workflow_files", persist_creds.workflow_files, 5), ) # The floor matters only to the anti-vacuity test; the two property tests take the pair, so an diff --git a/scripts/tests/test_workflow_persist_credentials.py b/scripts/tests/test_workflow_persist_credentials.py new file mode 100644 index 000000000..3381716d6 --- /dev/null +++ b/scripts/tests/test_workflow_persist_credentials.py @@ -0,0 +1,160 @@ +"""Every `actions/checkout` drops the persisted credential (ersatztv#835). + +WHAT THIS IS PROTECTING. `actions/checkout` writes an `Authorization` header into `.git/config` +unless `persist-credentials: false` is set, and this instance's Actions default token permission is +`permissive` — so that header is write-capable, and every later step in the job inherits it, not +just the checkout. ersatztv#746 set the flag on every checkout that existed then; nothing held the +convention afterwards. A job added tomorrow gets a checkout without the flag, and **nothing goes +red** — the silent-by-construction shape `docs/defect-shapes-773.md` §4 exists to catch. + +WHY THERE IS NO EXEMPTION LIST. `ci-image.yml`'s checkout was the one left unset, for a mechanical +reason in another issue (#744), and this guard was deliberately held back rather than shipped with a +one-entry exemption for it: an exemption outlives its reason silently — once #744 set the flag there, +the entry would still have passed and the guard would have been permanently blind to the very file it +was written for. #744 landed with this, so the assertion is universal over the derived population. + +POPULATION. The git index, via `tracked_files.tracked_paths` — not a filesystem walk and not a +hand-written list (`testing.guard-derives-population-from-source`, ersatztv#806). Both `*.yml` and +`*.yaml` are matched: Gitea accepts either spelling, so a `.yaml` workflow would otherwise be +structurally invisible to a check that reads as covering all of them. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import yaml + +from scripts.tests.tracked_files import tracked_paths + +WORKFLOWS = (".gitea/workflows", ("*.yml", "*.yaml")) + +CHECKOUT = "actions/checkout" + +# Matches the `uses:` line the YAML walk is supposed to reach. Used ONLY as an independent second +# opinion on how many there are — never as the population itself. +_USES_CHECKOUT = re.compile(r"^\s*-?\s*uses:\s*['\"]?actions/checkout(@|\s|['\"]|$)", re.M) + + +def workflow_files() -> list[Path]: + return tracked_paths(*WORKFLOWS) + + +def _checkout_steps(doc: object) -> list[tuple[str, int, dict]]: + """Every `actions/checkout` step in a parsed workflow, as (job id, index within job, step). + + Reads `jobs..steps` only, which is the whole shape this repo uses; a step list reached any + other way (a composite action, a reusable workflow) would be missed, and + `test_the_walk_finds_every_actions_checkout_the_TEXT_does` is what reports that rather than + letting it pass quietly. + """ + found: list[tuple[str, int, dict]] = [] + if not isinstance(doc, dict): + return found + jobs = doc.get("jobs") + if not isinstance(jobs, dict): + return found + for job_id, job in jobs.items(): + if not isinstance(job, dict): + continue + steps = job.get("steps") + if not isinstance(steps, list): + continue + for index, step in enumerate(steps): + if not isinstance(step, dict): + continue + uses = step.get("uses") + if isinstance(uses, str) and uses.split("@", 1)[0].strip() == CHECKOUT: + found.append((str(job_id), index, step)) + return found + + +def checkout_faults(rel: str, doc: object) -> list[str]: + """Human-readable faults for one workflow — accumulated, not failed fast. + + A missing key and an explicit `true` are the same defect and are reported the same way: what + matters is whether the credential is left behind, not how the step spelled it. + """ + faults: list[str] = [] + for job_id, index, step in _checkout_steps(doc): + with_block = step.get("with") + value = with_block.get("persist-credentials") if isinstance(with_block, dict) else None + if value is False: + continue + how = "does not set it at all" if value is None else f"sets it to {value!r}" + faults.append( + f"{rel}: job `{job_id}` step #{index} uses {CHECKOUT} and {how} — so the action " + f"persists a write-capable Authorization header into .git/config, and every later " + f"step in that job inherits it. Add `persist-credentials: false` under `with:` " + f"(ersatztv#746, guarded by ersatztv#835). There is deliberately no exemption list: " + f"if a checkout genuinely needs the credential, say so in the step and change this " + f"guard in the same PR." + ) + return faults + + +def test_every_actions_checkout_DROPS_the_persisted_credential() -> None: + faults: list[str] = [] + for path in workflow_files(): + rel = path.relative_to(Path(__file__).resolve().parents[2]).as_posix() + faults.extend(checkout_faults(rel, yaml.safe_load(path.read_text()))) + assert not faults, "\n".join(faults) + + +def test_the_walk_finds_every_actions_checkout_the_TEXT_does() -> None: + """The YAML walk and a plain text scan must agree on the count, per file. + + Without this, a step list the walk cannot reach — a composite action, a reusable workflow, a + shape act_runner grows later — makes checkouts structurally invisible while the guard above + still reports a clean run. Absence of output is not evidence; make the system report it. + """ + disagreements: list[str] = [] + for path in workflow_files(): + text = path.read_text() + walked = len(_checkout_steps(yaml.safe_load(text))) + scanned = len(_USES_CHECKOUT.findall(text)) + if walked != scanned: + disagreements.append( + f"{path.name}: the jobs..steps walk found {walked} {CHECKOUT} step(s) but the " + f"text scan found {scanned}. The walk is what the flag assertion runs over, so the " + f"difference is checkouts this guard cannot see — widen `_checkout_steps`." + ) + assert not disagreements, "\n".join(disagreements) + + +def test_the_population_and_the_checkout_set_are_NOT_empty() -> None: + """Anti-vacuity, in both directions. + + An empty population, or a population of workflows in which the walk finds no checkout at all, + makes the assertion above pass while measuring nothing — which is the failure this guard exists + to prevent, arriving through the guard itself. + """ + files = workflow_files() + assert files, ( + "`git ls-files` reported no .gitea/workflows/*.yml — the derivation is broken, not the repo, " + "and every assertion in this file would pass vacuously." + ) + total = sum(len(_checkout_steps(yaml.safe_load(p.read_text()))) for p in files) + assert total > 0, ( + f"Parsed {len(files)} workflow(s) and found no {CHECKOUT} step in any of them. Either the " + "repo really has none (then delete this guard and say why) or `_checkout_steps` stopped " + "matching, in which case the flag assertion is measuring an empty set." + ) + + +def test_the_fault_collector_reports_a_checkout_that_OMITS_the_flag() -> None: + """Negative control on the collector itself, over both defect spellings. + + The mutation proof (`mutation_manifest.py`) drives the real tree; this drives the collector + directly so a collector that silently stopped reporting is caught without waiting for the + harness, and so `true` is covered as well as an absent key. + """ + omitted = {"jobs": {"j": {"steps": [{"uses": "actions/checkout@v4"}]}}} + explicit_true = {"jobs": {"j": {"steps": [{"uses": "actions/checkout@v4", "with": {"persist-credentials": True}}]}}} + compliant = {"jobs": {"j": {"steps": [{"uses": "actions/checkout@v4", "with": {"persist-credentials": False}}]}}} + assert len(checkout_faults("synthetic.yml", omitted)) == 1 + assert "does not set it at all" in checkout_faults("synthetic.yml", omitted)[0] + assert len(checkout_faults("synthetic.yml", explicit_true)) == 1 + assert "sets it to True" in checkout_faults("synthetic.yml", explicit_true)[0] + assert checkout_faults("synthetic.yml", compliant) == [] -- 2.47.3 From 226f132b1ce4310c5d2e76008215e7b3b4f5a6fb Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 20:31:56 +0200 Subject: [PATCH 2/8] =?UTF-8?q?fix(744,835):=20review=20round=201=20?= =?UTF-8?q?=E2=80=94=20ref-class=20residual,=20retracted=20two-step,=20gua?= =?UTF-8?q?rd=20escapes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent cold reviews (Codex GPT-5.6, and a fresh Opus reviewer in an isolated worktree) both returned BLOCKED. All findings addressed: - The "what this does not close" statement sorted by the `branches:` KEYWORD, which mis-sorts `docker-build.yml`: it has `branches: [main]` and would pass, yet also `tags: ['v*']` and `pull_request:`, both head-resolved on the same registry credential — and a tag push is explicitly outside `release.main-direct-push-disabled`, so it needs no dispatch at all. Restated over the REF CLASS a trigger admits, in the record's `rule:` and in ci-cd.md, and #853 updated to match. - The recovery text offered "dispatch on `main` after the merge and bump the pin" as an alternative. It cannot work: `expected` is the last `docker/ci` commit, so a pin naming the workflow-only commit fails `ci-image-pin` and leaving the pin keeps consuming the old image. A `docker/ci/**` touch is the only remedy. - `ci-image.yml`'s own ROLLOUT NOTE still described the retracted merge-then-follow-up-PR two-step, five lines below the trigger summary the change had updated. - The guard: `uses: https://github.com/actions/checkout@v4` is a valid Gitea spelling and escaped both the walk and the text cross-check — canonicalised, and both spellings are now pinned in the negative control. Added two absence tests so the population's assumptions REPORT themselves rather than going blind: no tracked workflow below `.gitea/workflows/` (the direct-children derivation is shared with two sibling guards), and no local composite action (whose `action.yml` is invisible to both halves of the cross-check — the docstring had cited the wrong test for that case). - `ci-image-pin` no longer claims to prove PUBLICATION: it compares git shas and never queries the registry, which matters more now that publishing is a human dispatch. Existence stays `toolchain-preflight`'s job. - The `:latest` guard is now the SOLE protection on the one event that never exercised it (a dispatch selects any ref), and says so. - Stale contracts: the credential-scoping record's `rule:` frontmatter and body still said 15-of-16; the pin job's `::error::` still said "let ci-image.yml publish". The `docker/ci/Dockerfile` comment fixes were made, reddened `ci-image-pin` (any byte there re-points `expected`), and were reverted — the trap is now recorded in ci-cd.md and the record, with both stale line ranges deferred to #854. refs #744 refs #835 Decisions-Edit: yes --- .gitea/workflows/ci-image.yml | 30 ++++--- .gitea/workflows/pr-checks.yml | 12 ++- docs/ci-cd.md | 28 +++++- docs/decisions/README.md | 4 +- .../records/ci/actions-credential-scoping.md | 8 +- .../toolchain-image-publish-is-a-dispatch.md | 23 ++++- .../test_workflow_persist_credentials.py | 85 ++++++++++++++++--- 7 files changed, 152 insertions(+), 38 deletions(-) diff --git a/.gitea/workflows/ci-image.yml b/.gitea/workflows/ci-image.yml index 40aab42c5..f907437ef 100644 --- a/.gitea/workflows/ci-image.yml +++ b/.gitea/workflows/ci-image.yml @@ -5,7 +5,8 @@ name: Build CI Toolchain Image # docker-build.yml consume it via `container:`, pinned to an immutable :. # # push to MAIN touching docker/ci/** -> : + :latest -# workflow_dispatch (any branch) -> : of that branch's HEAD +# workflow_dispatch on main -> : of main's HEAD + :latest +# workflow_dispatch on a branch -> : of that branch's HEAD ONLY (never :latest) # schedule (weekly) -> picks up base-image security updates # # Deliberately separate from docker-build.yml: this image changes rarely (a Dockerfile edit or @@ -13,10 +14,12 @@ name: Build CI Toolchain Image # ~2GB toolchain image on every commit. # # ROLLOUT NOTE: the jobs pin an immutable :, never :latest — a broken toolchain image would -# otherwise block every converted job the moment it was pushed. Bumping the toolchain is therefore -# a deliberate two-step: merge a docker/ci/Dockerfile change (this workflow publishes a new :), -# then update the pin in docker-build.yml in a follow-up PR whose CI proves the new image works. -# See docs/ci-cd.md -> "CI toolchain image". +# otherwise block every converted job the moment it was pushed. Bumping the toolchain is a deliberate +# two-step, and since ersatztv#744 BOTH steps land in the SAME PR: publish (push the docker/ci commit +# as branch HEAD, dispatch this workflow on that branch), then commit the pin bump in +# docker-build.yml. Merging first is not available — `ci-image-pin` is blocking, so a PR that changes +# docker/ci/** without moving the pin is red. +# See docs/ci-cd.md -> "Publishing from a branch is a dispatch, not a push". # # Like docker-build.yml: the Gitea registry is HTTP-only, so BuildKit needs the inline # `http = true` config (it does not inherit the host daemon's insecure-registries setting). @@ -31,6 +34,11 @@ on: # publish path (docs/ci-cd.md -> "CI toolchain image"); it takes someone who already has write # access rather than falling out of an ordinary branch push. # + # A TAG push does not match this trigger either: Gitea's matcher gives a `branches:`-only filter no + # match for a non-branch ref, and there is no `tags:` key to satisfy instead. So a release cut no + # longer republishes the toolchain image as a side effect — which it could before, when `paths:` + # was the only filter. + # # `.gitea/workflows/ci-image.yml` is NOT in `paths:`, and that is forced by `branches: [main]` # rather than a preference: `ci-image-pin` derives its `expected` from the last commit touching # the image sources, so if this file were a source, editing it would re-point `expected` at a @@ -73,8 +81,9 @@ jobs: # toolchain image — the heaviest thing that ran in that lane. `small` is now # git-only and capped at 1g per job, which would OOM this build. # - # Rare trigger (pushes touching docker/ci + a weekly cron), so it costs the - # ubuntu-latest lane almost nothing, and ci-runner (.127) runs no prod workload. + # Rare trigger (main pushes touching docker/ci, a weekly cron, and the occasional + # branch dispatch), so it costs the ubuntu-latest lane almost nothing, and + # ci-runner (.127) runs no prod workload. runs-on: ubuntu-latest steps: - name: Checkout @@ -99,9 +108,10 @@ jobs: TAGS=("${CI_IMAGE}:${SHORT}") # :latest is a convenience/floating pointer for humans and the weekly rebuild; jobs must # never consume it. Only main may move it — and since #744 the `push` trigger is - # main-only, so on this path the branch check is satisfied by construction. It stays - # because `workflow_dispatch` still selects any branch, which is exactly the case that - # must not move `:latest`. + # main-only, so on that path the branch check is satisfied by construction. It is now the + # SOLE protection on the one event that never exercised it before: a `workflow_dispatch` + # selects any ref, and the branch-side publish path documented in ci-cd.md runs exactly + # that. Do not simplify this away on the reasoning that the trigger is already main-only. if [ "${GITHUB_REF}" = "refs/heads/main" ]; then TAGS+=("${CI_IMAGE}:latest") fi diff --git a/.gitea/workflows/pr-checks.yml b/.gitea/workflows/pr-checks.yml index 0b3cf192d..02588b175 100644 --- a/.gitea/workflows/pr-checks.yml +++ b/.gitea/workflows/pr-checks.yml @@ -43,9 +43,13 @@ concurrency: cancel-in-progress: true 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 - # sources. Without this detector, a PR that edits docker/ci/** publishes a NEW image but runs its own + # BLOCKING (ersatztv#390): the CI toolchain image pin in docker-build.yml must name the short sha of + # the last commit to touch the image's SOURCES (`docker/ci/**`). Read that as "the image ci-image.yml + # last published" only under the convention that every such commit is published — this job compares + # git shas and never queries the registry, so it cannot see a pin whose tag was never built or has + # been evicted. Existence is `toolchain-preflight`'s job, and the container jobs' pull is the backstop. + # Since ersatztv#744 publishing from a branch is a `workflow_dispatch`, so "was it published" is a + # human step this job does not observe. Without this detector, a PR that edits docker/ci/** publishes a NEW image but runs its own # jobs against the OLD pin: CI green-lights a toolchain it never executed, and once merged, main's # Dockerfile silently disagrees with what CI runs. **Renovate actively generates exactly that PR** — # it manages docker/ci/Dockerfile's base pins (dockerfile manager) but cannot bump an opaque @@ -136,7 +140,7 @@ jobs: exit 1 fi if [ "$pin_full" != "$expected" ]; then - echo "::error::CI toolchain image pin is stale: docker-build.yml pins ersatztv-ci:${pins[0]} ($pin_full), but docker/ci was last changed in $expected. Your jobs are testing an image that is NOT built from this PR's docker/ci. Let ci-image.yml publish the new :, then update the pin in ALL jobs to it (docs/ci-cd.md -> 'CI toolchain image')." + echo "::error::CI toolchain image pin is stale: docker-build.yml pins ersatztv-ci:${pins[0]} ($pin_full), but docker/ci was last changed in $expected. Your jobs are testing an image that is NOT built from this PR's docker/ci. Publish the new : — push this commit as branch HEAD and dispatch ci-image.yml on the branch (a branch PUSH no longer publishes, ersatztv#744) — then update the pin in ALL jobs to it (docs/ci-cd.md -> 'CI toolchain image')." exit 1 fi echo "Pin is current: ersatztv-ci:${pins[0]} resolves to $pin_full = docker/ci's last change." diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 64479e764..fce64cf1d 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1766,6 +1766,21 @@ docker-capable runner holding the credential that writes `ersatztv:prod` and the five `container:` jobs run. Credential scoping (`ci.actions-credential-scoping`) bounds what such a job may *do*; it never bounded whether it runs. +**What this does not close — and sort it by REF CLASS, not by which keyword a trigger carries.** +`docker-build.yml` has `branches: [main]`, so a keyword test passes it; it also has `tags: ['v*']` +and `pull_request:`, both head-resolved and both reaching the same registry credential. A tag push +is outside `release.main-direct-push-disabled`, so `git push origin :refs/tags/v99.9.9` runs +that commit's `docker-build.yml` and can publish `ersatztv:prod` — no dispatch needed. Separately, +`ci-image.yml`, `docker-build.yml`, `dependency-scan.yml` and `renovate.yml` all carry an +unrestricted `workflow_dispatch`, which loads its definition from the selected ref. The filter here +removes the *drive-by* route, where merely pushing a branch was enough; it is not a boundary against +a malicious writer. The whole remaining class is tracked in #853. + +**A tag push does not fire `ci-image.yml`.** Gitea's push matcher gives a `branches:`-only filter no +match for a non-branch ref, and there is no `tags:` key to satisfy instead — so a release cut no +longer republishes the toolchain image as a side effect, which it could when `paths:` was the only +filter. + The capability that removed is real and is **not** assumed away — it is replaced by `workflow_dispatch`, which Gitea loads from the branch you select. The constraint to plan around is that `ci-image.yml` tags `git rev-parse --short HEAD`, i.e. **the branch HEAD at dispatch time**, not @@ -1783,14 +1798,23 @@ cheapest way to avoid the loop entirely is still to **land a toolchain-image change on its own, before** the work that consumes it, so the consuming branch never carries the `docker/ci` commit through a rebase. +> **`docker/ci/**` has no cheap-edit path, comments included.** Editing any byte there re-points +> `ci-image-pin`'s `expected` at the editing commit, so the five `container:` pins go stale and a +> blocking job fails — a one-line comment fix costs a full publish-and-repin, and a rebase charges it +> again. Ride prose fixes in that directory with the next genuine toolchain bump. The stale +> publish instruction still in `docker/ci/Dockerfile` is #854, deferred on exactly these grounds. + **A change that lives only in `ci-image.yml` publishes nothing.** That file is no longer one of its own trigger paths, nor one of `ci-image-pin`'s `expected` paths — the two had to move together, because under `branches: [main]` a self-triggering path only publishes *after* the merge, which would advance `expected` to a commit no image exists for and redden a blocking job on every subsequent PR. The consequence to live with: if you change **how** the image is built (build-args, Dockerfile path, platforms) without touching `docker/ci/**`, CI keeps running an image built by the previous recipe -and nothing goes red. Either make the change alongside a `docker/ci/**` edit, or dispatch on `main` -after the merge and bump the pin. +and nothing goes red. **Make the change alongside a `docker/ci/**` edit** — a comment bump in the +Dockerfile is enough. That is not one option of two: publishing after the merge and then pinning +does not work, because `expected` is the last `docker/ci` commit, so a pin naming the workflow-only +commit fails `ci-image-pin` and leaving the pin where it is keeps consuming the old image. The +`docker/ci` touch is what gives the new recipe a commit the pin is allowed to name. ### When the pinned tag disappears diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 4334b16aa..d89593e32 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. 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 all 16 of this repo's checkouts now set it — 15 in #746 and `ci-image.yml`'s in #744, once `ci.toolchain-image-publish-is-a-dispatch` removed the two mechanical reasons it was excluded; the convention is held with no exemption list by `scripts/tests/test_workflow_persist_credentials.py` (#835). 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) | @@ -65,7 +65,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.script-tests-job` | The `scripts/tests/` pytest suite runs on every PR as a dedicated `script-tests` job in `pr-checks.yml` (`runs-on: small`, `setup-python` + `pip install pytest pyyaml`, `PYTHONPATH=. python3 -m pytest scripts/tests -q`; since #780 it also runs a pinned ruff over a `git ls-files` population first), unconditionally rather than behind a `scripts/**` path filter, and **never as a step inside `decisions-guard`** — a job whose reds a standing rule instructs sessions to ignore must never host a gate whose reds are real. Any new CI gate must be reachable by a failure that is unambiguously attributable to it. | 2026-07-26 | [link](records/ci/script-tests-job.md) | | `ci.shared-pr-file-enumeration` | A PR's complete set of changed file paths is computed by exactly one implementation, `scripts/pr-changed-files.sh`, called by both `.claude/hooks/pretooluse-merge-consent.sh` (advisory — a failure falls through to a human prompt) and `.gitea/workflows/review-verdict.yml` (enforced — a failure must fail closed, because a match here posts the branch-protection-required `review-verdict/h10` status with nobody in the loop). The script owns exhaustiveness (pagination, rename/path validation, head-sha binding, base-ref binding — see `ci.exemption-provenance` — and base-TIP binding, #707: the ref answers "did this PR RETARGET", the tip answers "did the base ADVANCE mid-enumeration", and only the second can see `/pulls/{n}/files` recomputing each offset-paged page against a moved base and dropping a path out of an already-consumed range; both ends of the window are bound, and an advance BEFORE the window is deliberately not an error, or ordinary churn on `main` would fail every open PR) and returns exit 0 only for a verified-complete list; it does NOT classify paths — each caller keeps its own docs-only allow-list, and the two allow-lists differ on purpose and stay separate. | 2026-07-26 | [link](records/ci/shared-pr-file-enumeration.md) | | `ci.small-lane-git-only` | `runs-on: small` is defined by what a job does (git-only), not its usual runtime; the two `docker build` jobs (docker-build.yml, ci-image.yml) move to `ubuntu-latest` because their worst-case memory, not median runtime, was pinning the small lane's per-slot cap. | 2026-07-20 | [link](records/ci/small-lane-git-only.md) | -| `ci.toolchain-image-publish-is-a-dispatch` | A workflow whose `push` trigger has no `branches:` filter executes attacker-supplied YAML, because Gitea resolves a `push` workflow's definition from the pushed branch — so `ci-image.yml` is `branches: [main]`, and publishing a toolchain image from a feature branch is a deliberate `workflow_dispatch` on that branch rather than a side effect of pushing. The self-referencing trigger path `.gitea/workflows/ci-image.yml` came out of BOTH its own `paths:` and `ci-image-pin`'s `expected` in the same change: under a main-only trigger a self-triggering path can only publish AFTER the merge, so `expected` would advance to a commit no image exists for and redden a blocking job on every subsequent PR. The cost is stated, not assumed away — a change to HOW the image is built that lives only in `ci-image.yml` no longer republishes, so make it alongside a `docker/ci/**` edit or dispatch and re-pin. | 2026-08-27 | [link](records/ci/toolchain-image-publish-is-a-dispatch.md) | +| `ci.toolchain-image-publish-is-a-dispatch` | A `push` trigger reachable from any ref other than `main` — that is `branches:` AND `tags:`, judged by the ref class it admits and never by which keyword is present — executes ref-supplied YAML, because Gitea resolves a `push` workflow's definition from the pushed ref — so `ci-image.yml` is `branches: [main]`, and publishing a toolchain image from a feature branch is a deliberate `workflow_dispatch` on that branch rather than a side effect of pushing. The self-referencing trigger path `.gitea/workflows/ci-image.yml` came out of BOTH its own `paths:` and `ci-image-pin`'s `expected` in the same change: under a main-only trigger a self-triggering path can only publish AFTER the merge, so `expected` would advance to a commit no image exists for and redden a blocking job on every subsequent PR. The cost is stated, not assumed away — a change to HOW the image is built that lives only in `ci-image.yml` no longer republishes, and the ONLY remedy is to make it alongside a `docker/ci/**` edit: publishing after the merge and then pinning cannot work, because `expected` is the last `docker/ci` commit and would reject that pin. This closes the push route INTO THIS FILE, not the class: `docker-build.yml` remains reachable from an arbitrary ref by a `v*` tag push and by `pull_request`, and four workflows carry an unrestricted `workflow_dispatch` (#853). | 2026-08-27 | [link](records/ci/toolchain-image-publish-is-a-dispatch.md) | | `ci.ui-e2e-harness` | The UI-interactive E2E flows run as headless Playwright specs (`web/e2e/*.spec.ts`, driven by `scripts/e2e-ui.sh`) in a **second step of the existing advisory `functional-e2e` job**, never their own job; the browser is `chromium-headless-shell` **baked into the CI toolchain image** (`docker/ci/Dockerfile`, `PLAYWRIGHT_VERSION` kept equal to `web/package.json`'s EXACT `@playwright/test` pin), never installed per run; specs are `serial` with `retries: 0` and assert only contracts the curl harness structurally cannot reach. | 2026-07-25 | [link](records/ci/ui-e2e-harness.md) | | `ci.verdict-write-retarget-fence` | The `review-verdict/h10` job counts `change_target_branch` events on the PR's issue timeline at run start and again immediately before its POST, and writes NOTHING if the count moved. The COUNT is the key because the branch NAME is ABA-vulnerable — `main -> S -> main` reads `main` at both ends, which is how #698 route 1 obtained a forged exemption — while the event count is monotonic and cannot alias. This NARROWS the residual, it does not resolve it — a retarget between the FINAL pre-write count and the POST still yields a PERMANENT forged green, because the successor run can consume the `edited` event and exit before the stale run posts last (corrected 2026-08-27, #849). Abstaining is a handoff, not a stall, and that is the property the design rests on — for a run that ABSTAINS; it says nothing about one that already passed its final count and then posts (see the residual, corrected #849): every retarget fires `edited`, which is in this workflow's `types:`, so the event that makes a run abstain has already queued a successor whose window opens after it; the induction terminates when retargeting stops — but ONLY over runs that ABSTAIN. A run that already passed its final count is outside it: it writes whenever it gets there, so the run that writes LAST is not necessarily the one that classified last, and that is the permanent residual (#849). `updated_at` was REJECTED as the key because it also moves for comments and labels, which fire none of this workflow's `types:` — a run could abstain with no successor coming, which is a real stall. The count is trusted only when paging reached a validated EMPTY page; an untrusted count (unreadable page, non-array body, non-numeric length, page cap hit) blocks the exemption `success` ONLY and still lets `pending` through, because `pending` blocks the merge immediately while withholding it would strand ordinary PRs whenever the timeline is unreadable — the right trade, but NOT a free one ("for no safety gain" retracted 2026-08-27): a GENERIC `pending` masks a rejection landing in its own write window, post-write verification does not run for it, and a later run re-derives it into an exemption `success` (#849). SEPARATELY, and for the human-verdict race the fence does nothing about: after posting an exemption `success` the job re-reads `/statuses/{sha}` and, if a human `Review-verdict:` row appeared with an id ABOVE a high-water mark taken just before the POST, overwrites its own status with `pending` and logs an error. The repair is `pending`, NEVER a copy of the human's state, since re-posting their `failure` under the machine credential would attribute a human verdict to the job; its description is a SENTINEL that the classification refuses to grant an exemption over AND re-writes verbatim on every later run, so the block is a FIXED POINT rather than decaying — writing the generic `pending` description there instead erases the marker and the exemption simply returns one event later. The mark is captured BEFORE the last-moment re-read, not merely before the POST — a later mark leaves a multi-round-trip blind gap in which a verdict is neither seen by the re-read nor repaired afterwards. The id comparison is load-bearing: a mere presence test would fire forever on a base-mismatched verdict that `read_existing_verdict` deliberately declines to honour, deadlocking that PR's exemption permanently. Finally, a run whose last-moment re-read finds a sentinel it did not see at its FIRST read ABSTAINS instead of posting: that can only mean an overlapping run repaired a raced verdict mid-flight, and this run's `success` — frozen at classification time, with the human row below its own mark, so neither the fence nor the post-write check would catch it — would otherwise bury the rejection. That is the one path in this design that failed toward SUCCESS rather than `pending`. The post-write check counts TWO row shapes above the mark, not one — a human `Review-verdict:` row AND a machine sentinel — because with two overlapping runs the human row can sit BELOW the second run's mark while the first masks it and only then writes the sentinel, leaving the second to post its own `success` on top; counting the sentinel converges both runs on the fixed point instead. | 2026-08-03 | [link](records/ci/verdict-write-retarget-fence.md) | | `ci.verify-locally-ci-confirms` | Treat the local build/verify/review pass as the decision point and CI as confirmation — don't idle waiting on a run you have no reason to doubt. | 2026-07-21 | [link](records/ci/verify-locally-ci-confirms.md) | diff --git a/docs/decisions/records/ci/actions-credential-scoping.md b/docs/decisions/records/ci/actions-credential-scoping.md index f59a1d25b..01cdd932d 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. 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 all 16 of this repo''s checkouts now set it — 15 in #746 and `ci-image.yml`''s in #744, once `ci.toolchain-image-publish-is-a-dispatch` removed the two mechanical reasons it was excluded; the convention is held with no exemption list by `scripts/tests/test_workflow_persist_credentials.py` (#835). 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,9 +75,9 @@ reach the runner's docker daemon. 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 -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 +reverted on 2026-08-05 (#748). All 16 of the repo's checkouts +now set it: `docker-build.yml` (8), `pr-checks.yml` (5), `dependency-scan.yml` (1), +`review-verdict.yml` (1, pre-existing) and `ci-image.yml` (1, added in #744). 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`. **Unmask the dependent fetches BEFORE dropping a persisted credential** — the rule, not a note about diff --git a/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md b/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md index 7e11412b4..e5735f85d 100644 --- a/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md +++ b/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md @@ -5,8 +5,8 @@ status: active since: '2026-08-27' supersedes: none superseded-by: none -rule: 'A workflow whose `push` trigger has no `branches:` filter executes attacker-supplied YAML, because Gitea resolves a `push` workflow''s definition from the pushed branch — so `ci-image.yml` is `branches: [main]`, and publishing a toolchain image from a feature branch is a deliberate `workflow_dispatch` on that branch rather than a side effect of pushing. The self-referencing trigger path `.gitea/workflows/ci-image.yml` came out of BOTH its own `paths:` and `ci-image-pin`''s `expected` in the same change: under a main-only trigger a self-triggering path can only publish AFTER the merge, so `expected` would advance to a commit no image exists for and redden a blocking job on every subsequent PR. The cost is stated, not assumed away — a change to HOW the image is built that lives only in `ci-image.yml` no longer republishes, so make it alongside a `docker/ci/**` edit or dispatch and re-pin.' -signals: 'unfiltered push trigger, branches filter on push, attacker YAML on a runner, feature branch publishes an image, self-referencing workflow trigger, ci-image-pin expected goes stale, publish-then-pin two-step, toolchain image recovery after a rebase, workflow_dispatch on a branch · paths: `.gitea/workflows/ci-image.yml`, `.gitea/workflows/pr-checks.yml`, `docs/ci-cd.md`, `scripts/e2e-ui.sh` · issues: #744, #697, #746, #835, #445, #390, #594, #597' +rule: 'A `push` trigger reachable from any ref other than `main` — that is `branches:` AND `tags:`, judged by the ref class it admits and never by which keyword is present — executes ref-supplied YAML, because Gitea resolves a `push` workflow''s definition from the pushed ref — so `ci-image.yml` is `branches: [main]`, and publishing a toolchain image from a feature branch is a deliberate `workflow_dispatch` on that branch rather than a side effect of pushing. The self-referencing trigger path `.gitea/workflows/ci-image.yml` came out of BOTH its own `paths:` and `ci-image-pin`''s `expected` in the same change: under a main-only trigger a self-triggering path can only publish AFTER the merge, so `expected` would advance to a commit no image exists for and redden a blocking job on every subsequent PR. The cost is stated, not assumed away — a change to HOW the image is built that lives only in `ci-image.yml` no longer republishes, and the ONLY remedy is to make it alongside a `docker/ci/**` edit: publishing after the merge and then pinning cannot work, because `expected` is the last `docker/ci` commit and would reject that pin. This closes the push route INTO THIS FILE, not the class: `docker-build.yml` remains reachable from an arbitrary ref by a `v*` tag push and by `pull_request`, and four workflows carry an unrestricted `workflow_dispatch` (#853).' +signals: 'unfiltered push trigger, branches filter on push, attacker YAML on a runner, feature branch publishes an image, self-referencing workflow trigger, ci-image-pin expected goes stale, publish-then-pin two-step, toolchain image recovery after a rebase, workflow_dispatch on a branch · paths: `.gitea/workflows/ci-image.yml`, `.gitea/workflows/pr-checks.yml`, `.gitea/workflows/docker-build.yml`, `docker/ci/Dockerfile`, `docs/ci-cd.md`, `docs/decisions/records/ci/actions-credential-scoping.md`, `scripts/e2e-ui.sh` · issues: #744, #853, #697, #746, #835, #445, #390, #594, #597' mechanics: '`docs/ci-cd.md` -> "CI toolchain image" -> "Publishing from a branch is a dispatch, not a push"' --- @@ -35,8 +35,23 @@ mechanics: '`docs/ci-cd.md` -> "CI toolchain image" -> "Publishing from a branch - **What that costs, recorded because it is invisible otherwise:** a change to how the image is built that lives only in `ci-image.yml` — build-args, Dockerfile path, platforms — neither republishes nor invalidates the pin, so CI keeps running an image built by the previous recipe and nothing goes red. - The remedy is procedural: pair such a change with a `docker/ci/**` edit, or dispatch on `main` after - the merge and bump the pin. The weekly cron does not cover this — it moves `:latest`, never a pin. + The remedy is a `docker/ci/**` edit in the same change — a Dockerfile comment bump suffices — and it + is the only one: publishing after the merge and then pinning is not an alternative, because + `expected` remains the last `docker/ci` commit and rejects a pin naming the workflow-only commit, + while leaving the pin alone keeps consuming the old image. The weekly cron does not cover it either + — that moves `:latest`, never a pin. A corollary worth its own line: **`docker/ci/**` has no + cheap-edit path**, so even a comment fix there costs a publish-and-repin and should ride the next + genuine toolchain bump (#854). +- **This closes ONE route into ONE file, not the class — and the residual is NOT all dispatch.** + Sort by the ref class a trigger admits, not by which keyword it carries, or the enumeration + mis-sorts: `docker-build.yml` has `branches: [main]` and would pass a keyword test, yet it also + has `tags: ['v*']` and `pull_request:`, both head-resolved and both holding the same registry + credential — and a tag push is explicitly outside `release.main-direct-push-disabled`, so + `git push origin :refs/tags/v99.9.9` runs that commit's workflow and can publish + `ersatztv:prod` with no dispatch at all. On top of that, four workflows carry an unrestricted + `workflow_dispatch`. What #744 changed is that none of this is any longer a side effect of an + ordinary branch push. Read it as protection against an accidental or drive-by push, **not** as a + boundary against a malicious or compromised writer; the whole remaining class is tracked in #853. - **The persist-credentials exemption was downstream of this, and closed with it.** `ci-image.yml`'s checkout was the one left without `persist-credentials: false` (#746), for exactly the two mechanical reasons above. Both gone, the flag went on in the same change, which is what let #835 ship its guard diff --git a/scripts/tests/test_workflow_persist_credentials.py b/scripts/tests/test_workflow_persist_credentials.py index 3381716d6..d4d1d2d93 100644 --- a/scripts/tests/test_workflow_persist_credentials.py +++ b/scripts/tests/test_workflow_persist_credentials.py @@ -3,8 +3,8 @@ WHAT THIS IS PROTECTING. `actions/checkout` writes an `Authorization` header into `.git/config` unless `persist-credentials: false` is set, and this instance's Actions default token permission is `permissive` — so that header is write-capable, and every later step in the job inherits it, not -just the checkout. ersatztv#746 set the flag on every checkout that existed then; nothing held the -convention afterwards. A job added tomorrow gets a checkout without the flag, and **nothing goes +just the checkout. ersatztv#746 set the flag on every checkout but one; nothing held the convention +afterwards. A job added tomorrow gets a checkout without the flag, and **nothing goes red** — the silent-by-construction shape `docs/defect-shapes-773.md` §4 exists to catch. WHY THERE IS NO EXEMPTION LIST. `ci-image.yml`'s checkout was the one left unset, for a mechanical @@ -26,15 +26,28 @@ from pathlib import Path import yaml -from scripts.tests.tracked_files import tracked_paths +from scripts.tests.tracked_files import _git_ls_files, tracked_paths -WORKFLOWS = (".gitea/workflows", ("*.yml", "*.yaml")) +WORKFLOW_DIR = ".gitea/workflows" +WORKFLOWS = (WORKFLOW_DIR, ("*.yml", "*.yaml")) CHECKOUT = "actions/checkout" -# Matches the `uses:` line the YAML walk is supposed to reach. Used ONLY as an independent second -# opinion on how many there are — never as the population itself. -_USES_CHECKOUT = re.compile(r"^\s*-?\s*uses:\s*['\"]?actions/checkout(@|\s|['\"]|$)", re.M) +# Gitea accepts a full action URL as well as `owner/repo`, so `https://github.com/actions/checkout@v4` +# is the same step spelled differently and must not escape. Canonicalise before comparing. +_ACTION_URL = re.compile(r"^(?:https?://)?(?:[^/]+\.[^/]+)/(?=[^/]+/[^/]+)") + +# Matches the `uses:` line the YAML walk is supposed to reach, in either spelling. Used ONLY as an +# independent second opinion on how many there are — never as the population itself. +_USES_CHECKOUT = re.compile( + r"^\s*-?\s*uses:\s*['\"]?(?:(?:https?://)?[^/'\"\s]+\.[^/'\"\s]+/)?actions/checkout(@|\s|['\"]|$)", + re.M, +) + + +def _canonical_action(uses: str) -> str: + """`https://github.com/actions/checkout@v4` and `actions/checkout@v4` are the same action.""" + return _ACTION_URL.sub("", uses.strip().strip("'\"")).split("@", 1)[0].strip() def workflow_files() -> list[Path]: @@ -44,10 +57,12 @@ def workflow_files() -> list[Path]: def _checkout_steps(doc: object) -> list[tuple[str, int, dict]]: """Every `actions/checkout` step in a parsed workflow, as (job id, index within job, step). - Reads `jobs..steps` only, which is the whole shape this repo uses; a step list reached any - other way (a composite action, a reusable workflow) would be missed, and - `test_the_walk_finds_every_actions_checkout_the_TEXT_does` is what reports that rather than - letting it pass quietly. + Reads `jobs..steps` only, which is the whole shape this repo uses. Two different escapes are + reported by two different tests, and they are not interchangeable: + `test_the_walk_finds_every_actions_checkout_the_TEXT_does` covers a shape INSIDE a workflow file + that the walk cannot reach, because both halves read the same files. A **composite action** is + invisible to both halves — its `action.yml` is not in the workflow population at all — so that + case is reported by `test_no_LOCAL_COMPOSITE_ACTION_carries_an_unchecked_checkout` instead. """ found: list[tuple[str, int, dict]] = [] if not isinstance(doc, dict): @@ -65,7 +80,7 @@ def _checkout_steps(doc: object) -> list[tuple[str, int, dict]]: if not isinstance(step, dict): continue uses = step.get("uses") - if isinstance(uses, str) and uses.split("@", 1)[0].strip() == CHECKOUT: + if isinstance(uses, str) and _canonical_action(uses) == CHECKOUT: found.append((str(job_id), index, step)) return found @@ -158,3 +173,49 @@ def test_the_fault_collector_reports_a_checkout_that_OMITS_the_flag() -> None: assert len(checkout_faults("synthetic.yml", explicit_true)) == 1 assert "sets it to True" in checkout_faults("synthetic.yml", explicit_true)[0] assert checkout_faults("synthetic.yml", compliant) == [] + + # Gitea accepts a full action URL, so the same step spelled as a URL must not escape either — + # pinned rather than probed once, because a canonicalisation that quietly stopped working would + # leave the guard green over a checkout it no longer recognises as one. + for spelling in ( + "https://github.com/actions/checkout@v4", + "github.com/actions/checkout@v4", + ): + as_url = {"jobs": {"j": {"steps": [{"uses": spelling}]}}} + assert len(checkout_faults("synthetic.yml", as_url)) == 1, spelling + assert _USES_CHECKOUT.search(f" - uses: {spelling}"), spelling + assert not _USES_CHECKOUT.search(" - uses: actions/setup-node@v4") + + +def test_no_workflow_hides_in_a_SUBDIRECTORY() -> None: + """The population is direct children of `.gitea/workflows`; prove nothing sits below it. + + `tracked_children` is direct-children-only by design (see `tracked_files`), which is right for a + flat directory and blind the moment one stops being flat. This does not widen the population — + it makes the assumption REPORT itself, so a nested workflow reddens the guard instead of being + silently uncovered, and it covers the two sibling guards that derive the same directory the same + way (`test_ci_image_pin_population.py`, `test_pr_changed_files.py`). + """ + nested = sorted( + path for path in _git_ls_files() if path.startswith(f"{WORKFLOW_DIR}/") and "/" in path[len(WORKFLOW_DIR) + 1 :] + ) + assert not nested, ( + f"tracked workflow file(s) below {WORKFLOW_DIR}/: {nested}. The population here is direct " + "children only, so these are NOT checked for `persist-credentials: false` — widen the " + "derivation (and the sibling guards that share it) before adding them." + ) + + +def test_no_LOCAL_COMPOSITE_ACTION_carries_an_unchecked_checkout() -> None: + """A checkout inside a local composite action runs in the job but is outside the workflow set. + + The repo has no `action.yml`/`action.yaml` today, so rather than write a parser for a shape that + does not exist, assert the absence: the first one added reddens this and its author widens the + guard, instead of the guard reading as complete while a whole class walked around it. + """ + actions = sorted(path for path in _git_ls_files() if path.rpartition("/")[2] in {"action.yml", "action.yaml"}) + assert not actions, ( + f"local composite action definition(s) found: {actions}. A composite action's steps run in " + "the job, so an `actions/checkout` in one is subject to the same rule and is NOT covered by " + "the workflow-file population above — extend `_checkout_steps` to walk them." + ) -- 2.47.3 From d91091e911223674ff46ecb1a92ff0483ff1eab6 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 20:58:05 +0200 Subject: [PATCH 3/8] =?UTF-8?q?fix(744,835):=20review=20round=202=20?= =?UTF-8?q?=E2=80=94=20the=20"forced"=20claim=20was=20FALSE;=20case=20+=20?= =?UTF-8?q?dotless-host=20escapes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both round-2 cold reviews returned BLOCKED. The most important finding is that a claim this change made three times over was simply wrong. **"Removing the self-path is FORCED" is false.** The argument was that under `branches: [main]` a self-triggering path can only publish after the merge. It cannot publish by PUSH — but the retained branch `workflow_dispatch` publishes it fine: dispatch the branch at the ci-image.yml commit, then pin it, and `ci-image-pin` resolves both to that commit and passes. So keeping the self-reference is workable, and the decision rests entirely on price, not necessity: keeping it charges a full ~2GB publish plus a five-pin bump for EVERY edit to that file, comments included, redone after every rebase — the cost that reverted #697 and forced this to land alone — against the cost of dropping it, a recipe change living only in ci-image.yml that never republishes. The second was chosen because that file is edited far more for triggers and comments than for build recipe. Restated as a decided tradeoff, with both prices, in the record, ci-cd.md and both workflow comments. #744's Done-when asks for the tradeoff to be DECIDED rather than asserted away; a false necessity claim was the opposite of that. **The guard had two more escapes of the class round 1 closed for URLs.** A Gitea action host is not required to contain a dot (`http://your-git-server/actions/checkout@v4` is the documented self-hosted spelling), and the forge is case-insensitive on owner/repo (`Actions/Checkout@v4` runs the real checkout). Both escaped the walk AND the text scan identically, so the count cross-check agreed and the guard reported clean. Replaced the host heuristic with last-two-path-segments and made both halves case-insensitive; all three spellings are pinned in the negative control. **A new stale-but-green state was reachable by following the documented remedy literally.** The dispatch builds HEAD's copy of every file, so a `ci-image.yml` recipe change committed AFTER the `docker/ci` commit publishes an image built by the old recipe while the pin and `expected` both name the `docker/ci` commit — green over an image the merged recipe would not produce. Step 1 now states the ordering. Also: the credential-scoping record's `rule:` frontmatter still said #744 was tracked-open and still asserted the mechanism this change removes, contradicting its own body — round 1 fixed the 15-of-16 clause inside that same string and stopped, a sweep by keyword rather than by concept. `ci-image-pin`'s comment still said a docker/ci PR "publishes a NEW image". Restored the #445 attribution the recovery rewrite dropped. Renamed the composite -action test to say what it asserts (that none EXISTS), and filed #855: nothing mechanically couples `ci-image.yml`'s `push.paths` to `ci-image-pin`'s `expected` now that the shared self-reference is gone. refs #744 refs #835 Decisions-Edit: yes --- .gitea/workflows/ci-image.yml | 17 ++++--- .gitea/workflows/pr-checks.yml | 38 +++++++++------- docs/ci-cd.md | 27 +++++++----- docs/decisions/README.md | 4 +- .../records/ci/actions-credential-scoping.md | 2 +- .../toolchain-image-publish-is-a-dispatch.md | 19 +++++--- .../test_workflow_persist_credentials.py | 44 ++++++++++++++----- 7 files changed, 98 insertions(+), 53 deletions(-) diff --git a/.gitea/workflows/ci-image.yml b/.gitea/workflows/ci-image.yml index f907437ef..98754cfdc 100644 --- a/.gitea/workflows/ci-image.yml +++ b/.gitea/workflows/ci-image.yml @@ -39,12 +39,17 @@ on: # longer republishes the toolchain image as a side effect — which it could before, when `paths:` # was the only filter. # - # `.gitea/workflows/ci-image.yml` is NOT in `paths:`, and that is forced by `branches: [main]` - # rather than a preference: `ci-image-pin` derives its `expected` from the last commit touching - # the image sources, so if this file were a source, editing it would re-point `expected` at a - # commit whose image can only publish AFTER the merge — leaving every subsequent PR red on a - # blocking job until someone dispatched and re-pinned. The cost is stated in ci-cd.md: a change - # to HOW the image is built that lives only in this file no longer republishes on its own. + # `.gitea/workflows/ci-image.yml` is NOT in `paths:`, and it left `ci-image-pin`'s `expected` in + # the same change. That pairing is a DECIDED TRADEOFF, not a necessity: keeping it works, because + # the dispatch above can publish the ci-image.yml commit itself and the pin then matches. The + # price is what decided it — that route charges a full ~2GB publish plus a five-pin bump for + # EVERY edit to this file, comments included, and a rebase charges it again. The cost of the side + # taken is stated here and in ci-cd.md: a change to HOW the image is built that lives only in + # this file no longer republishes on its own, so pair it with a `docker/ci/**` edit. + # + # `paths:` here and `ci-image-pin`'s `expected` pathspec in pr-checks.yml MUST name the same + # sources, and nothing mechanically enforces that since the shared self-reference went — + # ersatztv#855. workflow_dispatch: push: branches: [main] diff --git a/.gitea/workflows/pr-checks.yml b/.gitea/workflows/pr-checks.yml index 02588b175..86e7b1fd8 100644 --- a/.gitea/workflows/pr-checks.yml +++ b/.gitea/workflows/pr-checks.yml @@ -49,7 +49,9 @@ jobs: # git shas and never queries the registry, so it cannot see a pin whose tag was never built or has # been evicted. Existence is `toolchain-preflight`'s job, and the container jobs' pull is the backstop. # Since ersatztv#744 publishing from a branch is a `workflow_dispatch`, so "was it published" is a - # human step this job does not observe. Without this detector, a PR that edits docker/ci/** publishes a NEW image but runs its own + # human step this job does not observe. + # + # Without this detector, a PR that edits docker/ci/** ships a new image RECIPE while running its own # jobs against the OLD pin: CI green-lights a toolchain it never executed, and once merged, main's # Dockerfile silently disagrees with what CI runs. **Renovate actively generates exactly that PR** — # it manages docker/ci/Dockerfile's base pins (dockerfile manager) but cannot bump an opaque @@ -75,21 +77,27 @@ jobs: - name: Verify the pin matches the last-published image run: | set -euo pipefail - # ci-image.yml tags the image `git rev-parse --short HEAD` of the run that built it, and it - # only builds when `docker/ci/**` changes — so the published image is named by the last - # commit to touch that path. + # ci-image.yml tags the image `git rev-parse --short HEAD` of the run that built it. Only + # its filtered `push` clause requires a `docker/ci/**` change; the weekly `schedule` and a + # `workflow_dispatch` both build the selected ref's HEAD whatever it touched. So `expected` + # is not a model of every tag in the registry — it is the one tag a PR is REQUIRED to be + # pinned to: the last commit to change the image's sources. # - # `.gitea/workflows/ci-image.yml` is deliberately NOT part of `expected` (ersatztv#744). - # It was, back when that file was one of its own trigger paths. It no longer is: under - # `branches: [main]` a self-triggering path would only publish AFTER the merge, so - # `expected` would advance to a commit no image yet exists for and redden this blocking - # job on every subsequent PR. The two must move together, and they did. - # - # What that costs, stated rather than assumed away: a change to HOW the image is built - # that lives ONLY in ci-image.yml (build-args, Dockerfile path, platforms) neither - # republishes nor invalidates the pin, so CI keeps running an image built by the previous - # recipe. Make such a change alongside a `docker/ci/**` edit, or dispatch + re-pin — see - # docs/ci-cd.md -> "CI toolchain image". + # `.gitea/workflows/ci-image.yml` is deliberately NOT part of `expected` (ersatztv#744), + # and that is a DECIDED TRADEOFF, not a necessity. Keeping it is workable — dispatch the + # branch at the ci-image.yml commit, then pin it — but it prices every edit to that file, + # comments included, at a full ~2GB publish plus a five-pin bump, redone after every + # rebase. Dropping it prices the opposite risk: a change to HOW the image is built living + # ONLY in ci-image.yml (build-args, Dockerfile path, platforms) neither republishes nor + # invalidates the pin, so CI keeps running an image built by the previous recipe. The + # second was chosen because that file is edited far more often for triggers, comments and + # runner placement than for build recipe. Make a recipe change alongside a `docker/ci/**` + # edit — a comment bump suffices, and it is the ONLY remedy: pinning the workflow-only + # commit is rejected here, because `expected` is the last `docker/ci` commit. + # Nothing MECHANICALLY couples this pathspec to `ci-image.yml`'s `on.push.paths`; before + # #744 the shared self-reference kept them in step. Divergence is silent and green in the + # dangerous direction — tracked in ersatztv#855. + # See docs/ci-cd.md -> "Publishing from a branch is a dispatch, not a push". # # Compare RESOLVED FULL shas, never the abbreviations: git auto-scales abbreviation length # with the repo's object count, so the tag built in CI from a `fetch-depth: 1` shallow clone diff --git a/docs/ci-cd.md b/docs/ci-cd.md index fce64cf1d..4cf6fe642 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1781,20 +1781,24 @@ match for a non-branch ref, and there is no `tags:` key to satisfy instead — s longer republishes the toolchain image as a side effect, which it could when `paths:` was the only filter. -The capability that removed is real and is **not** assumed away — it is replaced by -`workflow_dispatch`, which Gitea loads from the branch you select. The constraint to plan around is -that `ci-image.yml` tags `git rev-parse --short HEAD`, i.e. **the branch HEAD at dispatch time**, not -the commit that touched `docker/ci`. So the branch-side two-step is: +The capability removed is real and is **not** assumed away — it is replaced by `workflow_dispatch`, +which Gitea loads from the branch you select. The constraint to plan around is that `ci-image.yml` +tags `git rev-parse --short HEAD`, i.e. **the branch HEAD at dispatch time**, not the commit that +touched `docker/ci`. So the branch-side two-step is: -1. Push the branch with the `docker/ci/**` change as **HEAD**. +1. Push the branch with the `docker/ci/**` change as **HEAD**. Anything that changes how the image is + built — a `ci-image.yml` edit to build-args, Dockerfile path or platforms — must already be + **below** that commit, because the dispatch builds HEAD's copy of every file. Commit it afterwards + and the published image was built by the *old* recipe while the pin and `expected` both name the + `docker/ci` commit, so `ci-image-pin` is green over an image the merged recipe would not produce. 2. Dispatch `ci-image.yml` on that branch (Actions → *Build CI Toolchain Image* → *Run workflow* → pick the branch). It publishes `:`; `:latest` stays put, because only `main` may move it. 3. Commit the pin bump in `docker-build.yml` on top. HEAD moves, but nothing needs republishing — `expected` is the `docker/ci` commit, which the pin now names. -After a rebase, redo all three: the sha is new, so the image must be republished under it. The -cheapest way to avoid the loop entirely is +After a rebase, redo all three: the sha is new, so the image must be republished under it. That is the +loop ersatztv#445 paid twice when `main` moved mid-review. The cheapest way to avoid it entirely is still to **land a toolchain-image change on its own, before** the work that consumes it, so the consuming branch never carries the `docker/ci` commit through a rebase. @@ -1805,9 +1809,12 @@ consuming branch never carries the `docker/ci` commit through a rebase. > publish instruction still in `docker/ci/Dockerfile` is #854, deferred on exactly these grounds. **A change that lives only in `ci-image.yml` publishes nothing.** That file is no longer one of its -own trigger paths, nor one of `ci-image-pin`'s `expected` paths — the two had to move together, -because under `branches: [main]` a self-triggering path only publishes *after* the merge, which would -advance `expected` to a commit no image exists for and redden a blocking job on every subsequent PR. +own trigger paths, nor one of `ci-image-pin`'s `expected` paths. That pairing is a **decided +tradeoff, not a necessity** — keeping the file in `expected` still works, since the branch dispatch +can publish the ci-image.yml commit itself and the pin then matches. What it costs is the reason it +went: that route charges a full ~2 GB publish and a five-pin bump for *every* edit to the file, a +comment included, and a rebase charges it again. That price reverted the first attempt (#697) and is +why this change had to land alone. The chosen side pays the opposite risk instead, below. The consequence to live with: if you change **how** the image is built (build-args, Dockerfile path, platforms) without touching `docker/ci/**`, CI keeps running an image built by the previous recipe and nothing goes red. **Make the change alongside a `docker/ci/**` edit** — a comment bump in the diff --git a/docs/decisions/README.md b/docs/decisions/README.md index d89593e32..cb5bf1deb 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 all 16 of this repo's checkouts now set it — 15 in #746 and `ci-image.yml`'s in #744, once `ci.toolchain-image-publish-is-a-dispatch` removed the two mechanical reasons it was excluded; the convention is held with no exemption list by `scripts/tests/test_workflow_persist_credentials.py` (#835). 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. 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. That landed for `ci-image.yml` in #744 (`ci.toolchain-image-publish-is-a-dispatch`) — deliberately NOT bundled here, because until it also removed the file from `ci-image-pin`'s `expected`, editing it re-pointed that job at the editing commit and reddened a blocking check. 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 all 16 of this repo's checkouts now set it — 15 in #746 and `ci-image.yml`'s in #744, once `ci.toolchain-image-publish-is-a-dispatch` removed the two mechanical reasons it was excluded; the convention is held with no exemption list by `scripts/tests/test_workflow_persist_credentials.py` (#835). 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) | @@ -65,7 +65,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.script-tests-job` | The `scripts/tests/` pytest suite runs on every PR as a dedicated `script-tests` job in `pr-checks.yml` (`runs-on: small`, `setup-python` + `pip install pytest pyyaml`, `PYTHONPATH=. python3 -m pytest scripts/tests -q`; since #780 it also runs a pinned ruff over a `git ls-files` population first), unconditionally rather than behind a `scripts/**` path filter, and **never as a step inside `decisions-guard`** — a job whose reds a standing rule instructs sessions to ignore must never host a gate whose reds are real. Any new CI gate must be reachable by a failure that is unambiguously attributable to it. | 2026-07-26 | [link](records/ci/script-tests-job.md) | | `ci.shared-pr-file-enumeration` | A PR's complete set of changed file paths is computed by exactly one implementation, `scripts/pr-changed-files.sh`, called by both `.claude/hooks/pretooluse-merge-consent.sh` (advisory — a failure falls through to a human prompt) and `.gitea/workflows/review-verdict.yml` (enforced — a failure must fail closed, because a match here posts the branch-protection-required `review-verdict/h10` status with nobody in the loop). The script owns exhaustiveness (pagination, rename/path validation, head-sha binding, base-ref binding — see `ci.exemption-provenance` — and base-TIP binding, #707: the ref answers "did this PR RETARGET", the tip answers "did the base ADVANCE mid-enumeration", and only the second can see `/pulls/{n}/files` recomputing each offset-paged page against a moved base and dropping a path out of an already-consumed range; both ends of the window are bound, and an advance BEFORE the window is deliberately not an error, or ordinary churn on `main` would fail every open PR) and returns exit 0 only for a verified-complete list; it does NOT classify paths — each caller keeps its own docs-only allow-list, and the two allow-lists differ on purpose and stay separate. | 2026-07-26 | [link](records/ci/shared-pr-file-enumeration.md) | | `ci.small-lane-git-only` | `runs-on: small` is defined by what a job does (git-only), not its usual runtime; the two `docker build` jobs (docker-build.yml, ci-image.yml) move to `ubuntu-latest` because their worst-case memory, not median runtime, was pinning the small lane's per-slot cap. | 2026-07-20 | [link](records/ci/small-lane-git-only.md) | -| `ci.toolchain-image-publish-is-a-dispatch` | A `push` trigger reachable from any ref other than `main` — that is `branches:` AND `tags:`, judged by the ref class it admits and never by which keyword is present — executes ref-supplied YAML, because Gitea resolves a `push` workflow's definition from the pushed ref — so `ci-image.yml` is `branches: [main]`, and publishing a toolchain image from a feature branch is a deliberate `workflow_dispatch` on that branch rather than a side effect of pushing. The self-referencing trigger path `.gitea/workflows/ci-image.yml` came out of BOTH its own `paths:` and `ci-image-pin`'s `expected` in the same change: under a main-only trigger a self-triggering path can only publish AFTER the merge, so `expected` would advance to a commit no image exists for and redden a blocking job on every subsequent PR. The cost is stated, not assumed away — a change to HOW the image is built that lives only in `ci-image.yml` no longer republishes, and the ONLY remedy is to make it alongside a `docker/ci/**` edit: publishing after the merge and then pinning cannot work, because `expected` is the last `docker/ci` commit and would reject that pin. This closes the push route INTO THIS FILE, not the class: `docker-build.yml` remains reachable from an arbitrary ref by a `v*` tag push and by `pull_request`, and four workflows carry an unrestricted `workflow_dispatch` (#853). | 2026-08-27 | [link](records/ci/toolchain-image-publish-is-a-dispatch.md) | +| `ci.toolchain-image-publish-is-a-dispatch` | A `push` trigger reachable from any ref other than `main` — that is `branches:` AND `tags:`, judged by the ref class it admits and never by which keyword is present — executes ref-supplied YAML, because Gitea resolves a `push` workflow's definition from the pushed ref — so `ci-image.yml` is `branches: [main]`, and publishing a toolchain image from a feature branch is a deliberate `workflow_dispatch` on that branch rather than a side effect of pushing. The self-referencing trigger path `.gitea/workflows/ci-image.yml` came out of BOTH its own `paths:` and `ci-image-pin`'s `expected` in the same change — a DECIDED tradeoff, not a necessity: keeping it is workable via the branch dispatch, but prices every edit to that file, comments included, at a ~2GB publish plus a five-pin bump, redone after every rebase. The cost is stated, not assumed away — a change to HOW the image is built that lives only in `ci-image.yml` no longer republishes, and the ONLY remedy is to make it alongside a `docker/ci/**` edit: publishing after the merge and then pinning cannot work, because `expected` is the last `docker/ci` commit and would reject that pin. This closes the push route INTO THIS FILE, not the class: `docker-build.yml` remains reachable from an arbitrary ref by a `v*` tag push and by `pull_request`, and four workflows carry an unrestricted `workflow_dispatch` (#853). | 2026-08-27 | [link](records/ci/toolchain-image-publish-is-a-dispatch.md) | | `ci.ui-e2e-harness` | The UI-interactive E2E flows run as headless Playwright specs (`web/e2e/*.spec.ts`, driven by `scripts/e2e-ui.sh`) in a **second step of the existing advisory `functional-e2e` job**, never their own job; the browser is `chromium-headless-shell` **baked into the CI toolchain image** (`docker/ci/Dockerfile`, `PLAYWRIGHT_VERSION` kept equal to `web/package.json`'s EXACT `@playwright/test` pin), never installed per run; specs are `serial` with `retries: 0` and assert only contracts the curl harness structurally cannot reach. | 2026-07-25 | [link](records/ci/ui-e2e-harness.md) | | `ci.verdict-write-retarget-fence` | The `review-verdict/h10` job counts `change_target_branch` events on the PR's issue timeline at run start and again immediately before its POST, and writes NOTHING if the count moved. The COUNT is the key because the branch NAME is ABA-vulnerable — `main -> S -> main` reads `main` at both ends, which is how #698 route 1 obtained a forged exemption — while the event count is monotonic and cannot alias. This NARROWS the residual, it does not resolve it — a retarget between the FINAL pre-write count and the POST still yields a PERMANENT forged green, because the successor run can consume the `edited` event and exit before the stale run posts last (corrected 2026-08-27, #849). Abstaining is a handoff, not a stall, and that is the property the design rests on — for a run that ABSTAINS; it says nothing about one that already passed its final count and then posts (see the residual, corrected #849): every retarget fires `edited`, which is in this workflow's `types:`, so the event that makes a run abstain has already queued a successor whose window opens after it; the induction terminates when retargeting stops — but ONLY over runs that ABSTAIN. A run that already passed its final count is outside it: it writes whenever it gets there, so the run that writes LAST is not necessarily the one that classified last, and that is the permanent residual (#849). `updated_at` was REJECTED as the key because it also moves for comments and labels, which fire none of this workflow's `types:` — a run could abstain with no successor coming, which is a real stall. The count is trusted only when paging reached a validated EMPTY page; an untrusted count (unreadable page, non-array body, non-numeric length, page cap hit) blocks the exemption `success` ONLY and still lets `pending` through, because `pending` blocks the merge immediately while withholding it would strand ordinary PRs whenever the timeline is unreadable — the right trade, but NOT a free one ("for no safety gain" retracted 2026-08-27): a GENERIC `pending` masks a rejection landing in its own write window, post-write verification does not run for it, and a later run re-derives it into an exemption `success` (#849). SEPARATELY, and for the human-verdict race the fence does nothing about: after posting an exemption `success` the job re-reads `/statuses/{sha}` and, if a human `Review-verdict:` row appeared with an id ABOVE a high-water mark taken just before the POST, overwrites its own status with `pending` and logs an error. The repair is `pending`, NEVER a copy of the human's state, since re-posting their `failure` under the machine credential would attribute a human verdict to the job; its description is a SENTINEL that the classification refuses to grant an exemption over AND re-writes verbatim on every later run, so the block is a FIXED POINT rather than decaying — writing the generic `pending` description there instead erases the marker and the exemption simply returns one event later. The mark is captured BEFORE the last-moment re-read, not merely before the POST — a later mark leaves a multi-round-trip blind gap in which a verdict is neither seen by the re-read nor repaired afterwards. The id comparison is load-bearing: a mere presence test would fire forever on a base-mismatched verdict that `read_existing_verdict` deliberately declines to honour, deadlocking that PR's exemption permanently. Finally, a run whose last-moment re-read finds a sentinel it did not see at its FIRST read ABSTAINS instead of posting: that can only mean an overlapping run repaired a raced verdict mid-flight, and this run's `success` — frozen at classification time, with the human row below its own mark, so neither the fence nor the post-write check would catch it — would otherwise bury the rejection. That is the one path in this design that failed toward SUCCESS rather than `pending`. The post-write check counts TWO row shapes above the mark, not one — a human `Review-verdict:` row AND a machine sentinel — because with two overlapping runs the human row can sit BELOW the second run's mark while the first masks it and only then writes the sentinel, leaving the second to post its own `success` on top; counting the sentinel converges both runs on the fixed point instead. | 2026-08-03 | [link](records/ci/verdict-write-retarget-fence.md) | | `ci.verify-locally-ci-confirms` | Treat the local build/verify/review pass as the decision point and CI as confirmation — don't idle waiting on a run you have no reason to doubt. | 2026-07-21 | [link](records/ci/verify-locally-ci-confirms.md) | diff --git a/docs/decisions/records/ci/actions-credential-scoping.md b/docs/decisions/records/ci/actions-credential-scoping.md index 01cdd932d..e1670365c 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 all 16 of this repo''s checkouts now set it — 15 in #746 and `ci-image.yml`''s in #744, once `ci.toolchain-image-publish-is-a-dispatch` removed the two mechanical reasons it was excluded; the convention is held with no exemption list by `scripts/tests/test_workflow_persist_credentials.py` (#835). 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. 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. That landed for `ci-image.yml` in #744 (`ci.toolchain-image-publish-is-a-dispatch`) — deliberately NOT bundled here, because until it also removed the file from `ci-image-pin`''s `expected`, editing it re-pointed that job at the editing commit and reddened a blocking check. 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 all 16 of this repo''s checkouts now set it — 15 in #746 and `ci-image.yml`''s in #744, once `ci.toolchain-image-publish-is-a-dispatch` removed the two mechanical reasons it was excluded; the convention is held with no exemption list by `scripts/tests/test_workflow_persist_credentials.py` (#835). 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.' --- diff --git a/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md b/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md index e5735f85d..07345a5fd 100644 --- a/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md +++ b/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md @@ -5,7 +5,7 @@ status: active since: '2026-08-27' supersedes: none superseded-by: none -rule: 'A `push` trigger reachable from any ref other than `main` — that is `branches:` AND `tags:`, judged by the ref class it admits and never by which keyword is present — executes ref-supplied YAML, because Gitea resolves a `push` workflow''s definition from the pushed ref — so `ci-image.yml` is `branches: [main]`, and publishing a toolchain image from a feature branch is a deliberate `workflow_dispatch` on that branch rather than a side effect of pushing. The self-referencing trigger path `.gitea/workflows/ci-image.yml` came out of BOTH its own `paths:` and `ci-image-pin`''s `expected` in the same change: under a main-only trigger a self-triggering path can only publish AFTER the merge, so `expected` would advance to a commit no image exists for and redden a blocking job on every subsequent PR. The cost is stated, not assumed away — a change to HOW the image is built that lives only in `ci-image.yml` no longer republishes, and the ONLY remedy is to make it alongside a `docker/ci/**` edit: publishing after the merge and then pinning cannot work, because `expected` is the last `docker/ci` commit and would reject that pin. This closes the push route INTO THIS FILE, not the class: `docker-build.yml` remains reachable from an arbitrary ref by a `v*` tag push and by `pull_request`, and four workflows carry an unrestricted `workflow_dispatch` (#853).' +rule: 'A `push` trigger reachable from any ref other than `main` — that is `branches:` AND `tags:`, judged by the ref class it admits and never by which keyword is present — executes ref-supplied YAML, because Gitea resolves a `push` workflow''s definition from the pushed ref — so `ci-image.yml` is `branches: [main]`, and publishing a toolchain image from a feature branch is a deliberate `workflow_dispatch` on that branch rather than a side effect of pushing. The self-referencing trigger path `.gitea/workflows/ci-image.yml` came out of BOTH its own `paths:` and `ci-image-pin`''s `expected` in the same change — a DECIDED tradeoff, not a necessity: keeping it is workable via the branch dispatch, but prices every edit to that file, comments included, at a ~2GB publish plus a five-pin bump, redone after every rebase. The cost is stated, not assumed away — a change to HOW the image is built that lives only in `ci-image.yml` no longer republishes, and the ONLY remedy is to make it alongside a `docker/ci/**` edit: publishing after the merge and then pinning cannot work, because `expected` is the last `docker/ci` commit and would reject that pin. This closes the push route INTO THIS FILE, not the class: `docker-build.yml` remains reachable from an arbitrary ref by a `v*` tag push and by `pull_request`, and four workflows carry an unrestricted `workflow_dispatch` (#853).' signals: 'unfiltered push trigger, branches filter on push, attacker YAML on a runner, feature branch publishes an image, self-referencing workflow trigger, ci-image-pin expected goes stale, publish-then-pin two-step, toolchain image recovery after a rebase, workflow_dispatch on a branch · paths: `.gitea/workflows/ci-image.yml`, `.gitea/workflows/pr-checks.yml`, `.gitea/workflows/docker-build.yml`, `docker/ci/Dockerfile`, `docs/ci-cd.md`, `docs/decisions/records/ci/actions-credential-scoping.md`, `scripts/e2e-ui.sh` · issues: #744, #853, #697, #746, #835, #445, #390, #594, #597' mechanics: '`docs/ci-cd.md` -> "CI toolchain image" -> "Publishing from a branch is a dispatch, not a push"' --- @@ -26,12 +26,17 @@ mechanics: '`docs/ci-cd.md` -> "CI toolchain image" -> "Publishing from a branch two-step with a dispatch in place of the push — push the `docker/ci` commit as HEAD, dispatch `ci-image.yml` on that branch (it tags `git rev-parse --short HEAD`, so the two must coincide), then commit the pin bump on top. A rebase rewrites the sha, so all three are redone; that was already true. -- **The self-path had to go with it, and that is forced rather than preferred.** `ci-image-pin` derives - `expected` from the last commit touching the image sources. While `.gitea/workflows/ci-image.yml` was - one of them, editing that file re-pointed `expected` at the editing commit — an annoyance under a - branch-publishing trigger (publish, then pin, inside the PR) but a **repo-wide** red under a main-only - one, since the image can then only be published after the merge. Every subsequent PR would be blocked - until someone dispatched and re-pinned. The two changes are one change. +- **Dropping the self-path is a DECIDED TRADEOFF, and calling it forced would be wrong.** `ci-image-pin` + derives `expected` from the last commit touching the image sources; while `.gitea/workflows/ci-image.yml` + was one of them, editing that file re-pointed `expected` at the editing commit. Retaining it stays + workable *because* the branch dispatch survives — push the ci-image.yml commit as HEAD, dispatch, + pin it — so the necessity argument does not hold. What does hold is the price: that route charges a + full ~2GB publish plus a five-pin bump for **every** edit to the file, a comment included, and a + rebase charges it again. That price is what reverted #697's attempt and what forced this change to + land alone. Dropping it prices the opposite risk instead (the bullet below). The second was chosen + because `ci-image.yml` is edited far more often for triggers, comments and runner placement than for + build recipe, so the first cost is paid constantly and the second only on a change that can be + paired with a `docker/ci` touch deliberately. - **What that costs, recorded because it is invisible otherwise:** a change to how the image is built that lives only in `ci-image.yml` — build-args, Dockerfile path, platforms — neither republishes nor invalidates the pin, so CI keeps running an image built by the previous recipe and nothing goes red. diff --git a/scripts/tests/test_workflow_persist_credentials.py b/scripts/tests/test_workflow_persist_credentials.py index d4d1d2d93..b3b03dc48 100644 --- a/scripts/tests/test_workflow_persist_credentials.py +++ b/scripts/tests/test_workflow_persist_credentials.py @@ -31,23 +31,38 @@ from scripts.tests.tracked_files import _git_ls_files, tracked_paths WORKFLOW_DIR = ".gitea/workflows" WORKFLOWS = (WORKFLOW_DIR, ("*.yml", "*.yaml")) +# Compared LOWERCASE throughout: Gitea resolves an action ref through a forge that is +# case-insensitive on owner/repo, so `uses: Actions/Checkout@v4` runs the real checkout. A +# case-sensitive comparison missed it in BOTH halves identically, so the count cross-check agreed +# and the guard reported clean over a step that persists the credential. CHECKOUT = "actions/checkout" -# Gitea accepts a full action URL as well as `owner/repo`, so `https://github.com/actions/checkout@v4` -# is the same step spelled differently and must not escape. Canonicalise before comparing. -_ACTION_URL = re.compile(r"^(?:https?://)?(?:[^/]+\.[^/]+)/(?=[^/]+/[^/]+)") - -# Matches the `uses:` line the YAML walk is supposed to reach, in either spelling. Used ONLY as an -# independent second opinion on how many there are — never as the population itself. +# Matches the `uses:` line the YAML walk is supposed to reach, in every spelling below. Used ONLY as +# an independent second opinion on how many there are — never as the population itself. _USES_CHECKOUT = re.compile( - r"^\s*-?\s*uses:\s*['\"]?(?:(?:https?://)?[^/'\"\s]+\.[^/'\"\s]+/)?actions/checkout(@|\s|['\"]|$)", - re.M, + r"^\s*-?\s*uses:\s*['\"]?(?:[^'\"\s]*/)?actions/checkout(@|\s|['\"]|$)", + re.M | re.I, ) def _canonical_action(uses: str) -> str: - """`https://github.com/actions/checkout@v4` and `actions/checkout@v4` are the same action.""" - return _ACTION_URL.sub("", uses.strip().strip("'\"")).split("@", 1)[0].strip() + """The last two path segments of a `uses:` value, without its `@version`. + + Gitea accepts a full action URL as well as `owner/repo`, and the host in one is NOT required to + contain a dot — `http://your-git-server/actions/checkout@v4` is documented and valid. So this + takes the last two segments rather than trying to recognise a hostname: every spelling of the + same action canonicalises to `actions/checkout`, with no host heuristic to get wrong. + + It over-matches in one direction on purpose: a genuinely different action whose path happens to + END in `actions/checkout` would be treated as a checkout and required to carry the flag. That + costs a spurious requirement on an action nobody has; the opposite error costs a live credential. + """ + ref = uses.strip().strip("'\"").split("@", 1)[0].strip().lower() + ref = re.sub(r"^https?://", "", ref) + segments = [seg for seg in ref.split("/") if seg] + if ref.startswith("./") or ref.startswith("../") or len(segments) < 2: + return ref + return "/".join(segments[-2:]) def workflow_files() -> list[Path]: @@ -62,7 +77,7 @@ def _checkout_steps(doc: object) -> list[tuple[str, int, dict]]: `test_the_walk_finds_every_actions_checkout_the_TEXT_does` covers a shape INSIDE a workflow file that the walk cannot reach, because both halves read the same files. A **composite action** is invisible to both halves — its `action.yml` is not in the workflow population at all — so that - case is reported by `test_no_LOCAL_COMPOSITE_ACTION_carries_an_unchecked_checkout` instead. + case is reported by `test_no_LOCAL_COMPOSITE_ACTION_exists_AT_ALL` instead. """ found: list[tuple[str, int, dict]] = [] if not isinstance(doc, dict): @@ -179,7 +194,12 @@ def test_the_fault_collector_reports_a_checkout_that_OMITS_the_flag() -> None: # leave the guard green over a checkout it no longer recognises as one. for spelling in ( "https://github.com/actions/checkout@v4", + # A Gitea action host is not required to contain a dot — this is the documented + # self-hosted spelling, and a dot-requiring canonicaliser let it through silently. + "http://your-git-server/actions/checkout@v4", "github.com/actions/checkout@v4", + # The forge is case-insensitive on owner/repo, so this runs the real checkout. + "Actions/Checkout@v4", ): as_url = {"jobs": {"j": {"steps": [{"uses": spelling}]}}} assert len(checkout_faults("synthetic.yml", as_url)) == 1, spelling @@ -206,7 +226,7 @@ def test_no_workflow_hides_in_a_SUBDIRECTORY() -> None: ) -def test_no_LOCAL_COMPOSITE_ACTION_carries_an_unchecked_checkout() -> None: +def test_no_LOCAL_COMPOSITE_ACTION_exists_AT_ALL() -> None: """A checkout inside a local composite action runs in the job but is outside the workflow set. The repo has no `action.yml`/`action.yaml` today, so rather than write a parser for a shape that -- 2.47.3 From 2734536a3a93a7efe4cd581f59dcc8f520c05392 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 21:34:11 +0200 Subject: [PATCH 4/8] =?UTF-8?q?fix(744,835):=20review=20round=203=20?= =?UTF-8?q?=E2=80=94=20the=20filter=20is=20REF-SUPPLIED,=20so=20this=20clo?= =?UTF-8?q?ses=20drive-by=20only?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 (two independent cold reviews, both BLOCKED, no blocker/high between them except one guard escape) found the security framing itself overstated. **The `branches:` filter is loaded from the pushed ref, like every other line in the file.** A branch whose `ci-image.yml` deletes the filter fires exactly as before, on the same runner, with the same credential. The mechanism this change is built on — "Gitea resolves a push workflow's definition from the pushed ref" — applies to the fix as much as to the hole, and three bolded sentences said "closes that route" without noticing. What is actually closed is the DRIVE-BY case: publication as a side effect of an ordinary push of a legitimate `docker/ci` change, no deliberate act anywhere. That is real and worth closing, and it is not a boundary against someone who intends to run their own YAML. Corrected in the workflow, ci-cd.md and the record's `rule:` and bullet 1, with an explicit "do not cite this record as that boundary"; #853 updated to lead with it, and a scope note posted on #744 because the issue body frames the fix more strongly than it delivers. **Three more spellings of `actions/checkout` escaped both halves of the guard**, in the same class as rounds 1 and 2: a `.git` suffix (which is how Gitea itself writes the clone URL), `@`-userinfo before the host (splitting on the FIRST `@` stripped the host, not the version), and a doubled slash. Canonicalisation now rsplits, drops a `.git`, and absorbs empty segments; all seven spellings are pinned. Two more absence tests so the remaining boundaries report themselves instead of going blind: no job delegates to a reusable workflow (both halves would see zero and AGREE, so the count cross-check cannot report it), and nothing tracked under `.github/workflows` — which Gitea reads only when `.gitea/workflows` is absent, an unprobed precedence rule, in a directory that already exists here. Also: "had to land alone" / "forced this change to land alone" still carried the necessity premise round 2 retracted — landing alone was the cheaper option, not the only one; `ci-image-pin` was called "blocking" as though branch protection required it (it does not — the block comes from the merge-consent hook reading the COMBINED status); the pin step was still named "Verify the pin matches the last-published image" while doing no registry query at all; ci-cd.md still documented `expected` as `docker/ci/**` or `ci-image.yml`; `release.main-direct-push-disabled` carried its own stale 15-of-16 count; the ordering constraint said "below" where "in that commit or below it" is correct; and the #854 deferral undercounted the stale Dockerfile comments at one when there are two. refs #744 refs #835 Decisions-Edit: yes --- .gitea/workflows/ci-image.yml | 30 ++++---- .gitea/workflows/pr-checks.yml | 2 +- docs/ci-cd.md | 12 ++-- docs/decisions/README.md | 2 +- .../toolchain-image-publish-is-a-dispatch.md | 16 +++-- .../release/main-direct-push-disabled.md | 2 +- .../test_workflow_persist_credentials.py | 72 +++++++++++++++++-- 7 files changed, 104 insertions(+), 32 deletions(-) diff --git a/.gitea/workflows/ci-image.yml b/.gitea/workflows/ci-image.yml index 98754cfdc..faf1e848d 100644 --- a/.gitea/workflows/ci-image.yml +++ b/.gitea/workflows/ci-image.yml @@ -15,10 +15,11 @@ name: Build CI Toolchain Image # # ROLLOUT NOTE: the jobs pin an immutable :, never :latest — a broken toolchain image would # otherwise block every converted job the moment it was pushed. Bumping the toolchain is a deliberate -# two-step, and since ersatztv#744 BOTH steps land in the SAME PR: publish (push the docker/ci commit -# as branch HEAD, dispatch this workflow on that branch), then commit the pin bump in -# docker-build.yml. Merging first is not available — `ci-image-pin` is blocking, so a PR that changes -# docker/ci/** without moving the pin is red. +# two-step, and BOTH steps land in the SAME PR: publish (push the docker/ci commit as branch HEAD, +# dispatch this workflow on that branch), then commit the pin bump in docker-build.yml. Merging first +# is not available: a PR that changes docker/ci/** without moving the pin turns `ci-image-pin` red, +# and the merge-consent hook reads the COMBINED commit status, so it will not auto-grant. That much +# predates ersatztv#744 — what #744 changed is how the publish half is performed. # See docs/ci-cd.md -> "Publishing from a branch is a dispatch, not a push". # # Like docker-build.yml: the Gitea registry is HTTP-only, so BuildKit needs the inline @@ -29,15 +30,20 @@ on: # Gitea resolves a `push` workflow's definition from the pushed branch, so an unfiltered `push` # trigger ran this file's own YAML — attacker-supplied, unreviewed, with no status check in the # loop — on a docker-capable runner holding the credential that writes `ersatztv:prod` and the - # `ersatztv-ci:` five `container:` jobs execute. `branches: [main]` closes that route. - # `workflow_dispatch` is still self-supplied from the selected branch and is the deliberate - # publish path (docs/ci-cd.md -> "CI toolchain image"); it takes someone who already has write - # access rather than falling out of an ordinary branch push. + # `ersatztv-ci:` five `container:` jobs execute. # - # A TAG push does not match this trigger either: Gitea's matcher gives a `branches:`-only filter no - # match for a non-branch ref, and there is no `tags:` key to satisfy instead. So a release cut no - # longer republishes the toolchain image as a side effect — which it could before, when `paths:` - # was the only filter. + # BE PRECISE ABOUT WHAT THIS BUYS, because the mechanism cuts both ways: the filter below is read + # from the pushed ref like everything else in this file, so a branch that DELETES it re-enables + # the route. What closes is the DRIVE-BY case — an ordinary push of a legitimate `docker/ci` + # change publishing an image nobody asked for, with no deliberate act anywhere. This is NOT a + # boundary against a malicious or compromised writer and must not be cited as one; that class is + # ersatztv#853. `workflow_dispatch` is self-supplied from the selected branch in the same way, and + # is the deliberate publish path (docs/ci-cd.md -> "CI toolchain image"). + # + # A `v*` tag push does not match this trigger either: there is no `tags:` key, and a `branches:` + # filter is compared against a branch ref. The exact matcher semantics are not probed here; the + # observable claim is the one that matters — a release cut no longer republishes the toolchain + # image as a side effect. # # `.gitea/workflows/ci-image.yml` is NOT in `paths:`, and it left `ci-image-pin`'s `expected` in # the same change. That pairing is a DECIDED TRADEOFF, not a necessity: keeping it works, because diff --git a/.gitea/workflows/pr-checks.yml b/.gitea/workflows/pr-checks.yml index 86e7b1fd8..29d00eff1 100644 --- a/.gitea/workflows/pr-checks.yml +++ b/.gitea/workflows/pr-checks.yml @@ -74,7 +74,7 @@ jobs: # need real history: `git log -- ` on a shallow clone can't find the last # commit that touched the image sources fetch-depth: 0 - - name: Verify the pin matches the last-published image + - name: Verify the pin matches the image-source commit run: | set -euo pipefail # ci-image.yml tags the image `git rev-parse --short HEAD` of the run that built it. Only diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 4cf6fe642..0b2378dde 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1805,8 +1805,10 @@ consuming branch never carries the `docker/ci` commit through a rebase. > **`docker/ci/**` has no cheap-edit path, comments included.** Editing any byte there re-points > `ci-image-pin`'s `expected` at the editing commit, so the five `container:` pins go stale and a > blocking job fails — a one-line comment fix costs a full publish-and-repin, and a rebase charges it -> again. Ride prose fixes in that directory with the next genuine toolchain bump. The stale -> publish instruction still in `docker/ci/Dockerfile` is #854, deferred on exactly these grounds. +> again. Ride prose fixes in that directory with the next genuine toolchain bump. The TWO stale +> publish instructions still in `docker/ci/Dockerfile` — the `PLAYWRIGHT_VERSION` bump comment, and +> the rebase warning's "re-dispatching does not help", which is now the opposite of the documented +> recovery — are #854, deferred on exactly these grounds. **A change that lives only in `ci-image.yml` publishes nothing.** That file is no longer one of its own trigger paths, nor one of `ci-image-pin`'s `expected` paths. That pairing is a **decided @@ -1952,9 +1954,9 @@ what it establishes is the auth + HTTP-registry push path, not the toolchain bui **Bumping the pin is enforced, not remembered.** The `ci-image-pin` job (blocking, PR-only; defined in `pr-checks.yml`, but it greps `docker-build.yml` where the pins live) fails if -`docker-build.yml`'s pin isn't the short sha of the last commit to touch `docker/ci/**` or -`ci-image.yml`, if that pin isn't exactly 7 chars long (see above), or if the five jobs ever pin -different tags. This exists because **Renovate manages +`docker-build.yml`'s pin isn't the short sha of the last commit to touch `docker/ci/**` (since +ersatztv#744 that path alone — `ci-image.yml` is deliberately not part of it), if that pin isn't +exactly 7 chars long (see above), or if the five jobs ever pin different tags. This exists because **Renovate manages `docker/ci/Dockerfile`'s base pins but cannot bump an opaque `:`** in `container.image` — so a Renovate base bump would otherwise publish a new image, test the *old* one, and merge with the Dockerfile disagreeing with the pin. A red `ci-image-pin` means: dispatch `ci-image.yml` on the diff --git a/docs/decisions/README.md b/docs/decisions/README.md index cb5bf1deb..057c85e72 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -65,7 +65,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.script-tests-job` | The `scripts/tests/` pytest suite runs on every PR as a dedicated `script-tests` job in `pr-checks.yml` (`runs-on: small`, `setup-python` + `pip install pytest pyyaml`, `PYTHONPATH=. python3 -m pytest scripts/tests -q`; since #780 it also runs a pinned ruff over a `git ls-files` population first), unconditionally rather than behind a `scripts/**` path filter, and **never as a step inside `decisions-guard`** — a job whose reds a standing rule instructs sessions to ignore must never host a gate whose reds are real. Any new CI gate must be reachable by a failure that is unambiguously attributable to it. | 2026-07-26 | [link](records/ci/script-tests-job.md) | | `ci.shared-pr-file-enumeration` | A PR's complete set of changed file paths is computed by exactly one implementation, `scripts/pr-changed-files.sh`, called by both `.claude/hooks/pretooluse-merge-consent.sh` (advisory — a failure falls through to a human prompt) and `.gitea/workflows/review-verdict.yml` (enforced — a failure must fail closed, because a match here posts the branch-protection-required `review-verdict/h10` status with nobody in the loop). The script owns exhaustiveness (pagination, rename/path validation, head-sha binding, base-ref binding — see `ci.exemption-provenance` — and base-TIP binding, #707: the ref answers "did this PR RETARGET", the tip answers "did the base ADVANCE mid-enumeration", and only the second can see `/pulls/{n}/files` recomputing each offset-paged page against a moved base and dropping a path out of an already-consumed range; both ends of the window are bound, and an advance BEFORE the window is deliberately not an error, or ordinary churn on `main` would fail every open PR) and returns exit 0 only for a verified-complete list; it does NOT classify paths — each caller keeps its own docs-only allow-list, and the two allow-lists differ on purpose and stay separate. | 2026-07-26 | [link](records/ci/shared-pr-file-enumeration.md) | | `ci.small-lane-git-only` | `runs-on: small` is defined by what a job does (git-only), not its usual runtime; the two `docker build` jobs (docker-build.yml, ci-image.yml) move to `ubuntu-latest` because their worst-case memory, not median runtime, was pinning the small lane's per-slot cap. | 2026-07-20 | [link](records/ci/small-lane-git-only.md) | -| `ci.toolchain-image-publish-is-a-dispatch` | A `push` trigger reachable from any ref other than `main` — that is `branches:` AND `tags:`, judged by the ref class it admits and never by which keyword is present — executes ref-supplied YAML, because Gitea resolves a `push` workflow's definition from the pushed ref — so `ci-image.yml` is `branches: [main]`, and publishing a toolchain image from a feature branch is a deliberate `workflow_dispatch` on that branch rather than a side effect of pushing. The self-referencing trigger path `.gitea/workflows/ci-image.yml` came out of BOTH its own `paths:` and `ci-image-pin`'s `expected` in the same change — a DECIDED tradeoff, not a necessity: keeping it is workable via the branch dispatch, but prices every edit to that file, comments included, at a ~2GB publish plus a five-pin bump, redone after every rebase. The cost is stated, not assumed away — a change to HOW the image is built that lives only in `ci-image.yml` no longer republishes, and the ONLY remedy is to make it alongside a `docker/ci/**` edit: publishing after the merge and then pinning cannot work, because `expected` is the last `docker/ci` commit and would reject that pin. This closes the push route INTO THIS FILE, not the class: `docker-build.yml` remains reachable from an arbitrary ref by a `v*` tag push and by `pull_request`, and four workflows carry an unrestricted `workflow_dispatch` (#853). | 2026-08-27 | [link](records/ci/toolchain-image-publish-is-a-dispatch.md) | +| `ci.toolchain-image-publish-is-a-dispatch` | A `push` trigger reachable from any ref other than `main` — that is `branches:` AND `tags:`, judged by the ref class it admits and never by which keyword is present — executes ref-supplied YAML, because Gitea resolves a `push` workflow's definition from the pushed ref — so `ci-image.yml` is `branches: [main]`, and publishing a toolchain image from a feature branch is a deliberate `workflow_dispatch` on that branch rather than a side effect of pushing. Be precise about what a `branches:` filter buys: it is loaded from the pushed ref like the rest of the file, so a branch that deletes it re-enables the route — this removes the DRIVE-BY case, and is not a boundary against a writer who intends to run their own YAML. The self-referencing trigger path `.gitea/workflows/ci-image.yml` came out of BOTH its own `paths:` and `ci-image-pin`'s `expected` in the same change — a DECIDED tradeoff, not a necessity: keeping it is workable via the branch dispatch, but prices every edit to that file, comments included, at a ~2GB publish plus a five-pin bump, redone after every rebase. The cost is stated, not assumed away — a change to HOW the image is built that lives only in `ci-image.yml` no longer republishes, and the ONLY remedy is to make it alongside a `docker/ci/**` edit: publishing after the merge and then pinning cannot work, because `expected` is the last `docker/ci` commit and would reject that pin. This closes the push route INTO THIS FILE, not the class: `docker-build.yml` remains reachable from an arbitrary ref by a `v*` tag push and by `pull_request`, and four workflows carry an unrestricted `workflow_dispatch` (#853). | 2026-08-27 | [link](records/ci/toolchain-image-publish-is-a-dispatch.md) | | `ci.ui-e2e-harness` | The UI-interactive E2E flows run as headless Playwright specs (`web/e2e/*.spec.ts`, driven by `scripts/e2e-ui.sh`) in a **second step of the existing advisory `functional-e2e` job**, never their own job; the browser is `chromium-headless-shell` **baked into the CI toolchain image** (`docker/ci/Dockerfile`, `PLAYWRIGHT_VERSION` kept equal to `web/package.json`'s EXACT `@playwright/test` pin), never installed per run; specs are `serial` with `retries: 0` and assert only contracts the curl harness structurally cannot reach. | 2026-07-25 | [link](records/ci/ui-e2e-harness.md) | | `ci.verdict-write-retarget-fence` | The `review-verdict/h10` job counts `change_target_branch` events on the PR's issue timeline at run start and again immediately before its POST, and writes NOTHING if the count moved. The COUNT is the key because the branch NAME is ABA-vulnerable — `main -> S -> main` reads `main` at both ends, which is how #698 route 1 obtained a forged exemption — while the event count is monotonic and cannot alias. This NARROWS the residual, it does not resolve it — a retarget between the FINAL pre-write count and the POST still yields a PERMANENT forged green, because the successor run can consume the `edited` event and exit before the stale run posts last (corrected 2026-08-27, #849). Abstaining is a handoff, not a stall, and that is the property the design rests on — for a run that ABSTAINS; it says nothing about one that already passed its final count and then posts (see the residual, corrected #849): every retarget fires `edited`, which is in this workflow's `types:`, so the event that makes a run abstain has already queued a successor whose window opens after it; the induction terminates when retargeting stops — but ONLY over runs that ABSTAIN. A run that already passed its final count is outside it: it writes whenever it gets there, so the run that writes LAST is not necessarily the one that classified last, and that is the permanent residual (#849). `updated_at` was REJECTED as the key because it also moves for comments and labels, which fire none of this workflow's `types:` — a run could abstain with no successor coming, which is a real stall. The count is trusted only when paging reached a validated EMPTY page; an untrusted count (unreadable page, non-array body, non-numeric length, page cap hit) blocks the exemption `success` ONLY and still lets `pending` through, because `pending` blocks the merge immediately while withholding it would strand ordinary PRs whenever the timeline is unreadable — the right trade, but NOT a free one ("for no safety gain" retracted 2026-08-27): a GENERIC `pending` masks a rejection landing in its own write window, post-write verification does not run for it, and a later run re-derives it into an exemption `success` (#849). SEPARATELY, and for the human-verdict race the fence does nothing about: after posting an exemption `success` the job re-reads `/statuses/{sha}` and, if a human `Review-verdict:` row appeared with an id ABOVE a high-water mark taken just before the POST, overwrites its own status with `pending` and logs an error. The repair is `pending`, NEVER a copy of the human's state, since re-posting their `failure` under the machine credential would attribute a human verdict to the job; its description is a SENTINEL that the classification refuses to grant an exemption over AND re-writes verbatim on every later run, so the block is a FIXED POINT rather than decaying — writing the generic `pending` description there instead erases the marker and the exemption simply returns one event later. The mark is captured BEFORE the last-moment re-read, not merely before the POST — a later mark leaves a multi-round-trip blind gap in which a verdict is neither seen by the re-read nor repaired afterwards. The id comparison is load-bearing: a mere presence test would fire forever on a base-mismatched verdict that `read_existing_verdict` deliberately declines to honour, deadlocking that PR's exemption permanently. Finally, a run whose last-moment re-read finds a sentinel it did not see at its FIRST read ABSTAINS instead of posting: that can only mean an overlapping run repaired a raced verdict mid-flight, and this run's `success` — frozen at classification time, with the human row below its own mark, so neither the fence nor the post-write check would catch it — would otherwise bury the rejection. That is the one path in this design that failed toward SUCCESS rather than `pending`. The post-write check counts TWO row shapes above the mark, not one — a human `Review-verdict:` row AND a machine sentinel — because with two overlapping runs the human row can sit BELOW the second run's mark while the first masks it and only then writes the sentinel, leaving the second to post its own `success` on top; counting the sentinel converges both runs on the fixed point instead. | 2026-08-03 | [link](records/ci/verdict-write-retarget-fence.md) | | `ci.verify-locally-ci-confirms` | Treat the local build/verify/review pass as the decision point and CI as confirmation — don't idle waiting on a run you have no reason to doubt. | 2026-07-21 | [link](records/ci/verify-locally-ci-confirms.md) | diff --git a/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md b/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md index 07345a5fd..6c2ff2abf 100644 --- a/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md +++ b/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md @@ -5,7 +5,7 @@ status: active since: '2026-08-27' supersedes: none superseded-by: none -rule: 'A `push` trigger reachable from any ref other than `main` — that is `branches:` AND `tags:`, judged by the ref class it admits and never by which keyword is present — executes ref-supplied YAML, because Gitea resolves a `push` workflow''s definition from the pushed ref — so `ci-image.yml` is `branches: [main]`, and publishing a toolchain image from a feature branch is a deliberate `workflow_dispatch` on that branch rather than a side effect of pushing. The self-referencing trigger path `.gitea/workflows/ci-image.yml` came out of BOTH its own `paths:` and `ci-image-pin`''s `expected` in the same change — a DECIDED tradeoff, not a necessity: keeping it is workable via the branch dispatch, but prices every edit to that file, comments included, at a ~2GB publish plus a five-pin bump, redone after every rebase. The cost is stated, not assumed away — a change to HOW the image is built that lives only in `ci-image.yml` no longer republishes, and the ONLY remedy is to make it alongside a `docker/ci/**` edit: publishing after the merge and then pinning cannot work, because `expected` is the last `docker/ci` commit and would reject that pin. This closes the push route INTO THIS FILE, not the class: `docker-build.yml` remains reachable from an arbitrary ref by a `v*` tag push and by `pull_request`, and four workflows carry an unrestricted `workflow_dispatch` (#853).' +rule: 'A `push` trigger reachable from any ref other than `main` — that is `branches:` AND `tags:`, judged by the ref class it admits and never by which keyword is present — executes ref-supplied YAML, because Gitea resolves a `push` workflow''s definition from the pushed ref — so `ci-image.yml` is `branches: [main]`, and publishing a toolchain image from a feature branch is a deliberate `workflow_dispatch` on that branch rather than a side effect of pushing. Be precise about what a `branches:` filter buys: it is loaded from the pushed ref like the rest of the file, so a branch that deletes it re-enables the route — this removes the DRIVE-BY case, and is not a boundary against a writer who intends to run their own YAML. The self-referencing trigger path `.gitea/workflows/ci-image.yml` came out of BOTH its own `paths:` and `ci-image-pin`''s `expected` in the same change — a DECIDED tradeoff, not a necessity: keeping it is workable via the branch dispatch, but prices every edit to that file, comments included, at a ~2GB publish plus a five-pin bump, redone after every rebase. The cost is stated, not assumed away — a change to HOW the image is built that lives only in `ci-image.yml` no longer republishes, and the ONLY remedy is to make it alongside a `docker/ci/**` edit: publishing after the merge and then pinning cannot work, because `expected` is the last `docker/ci` commit and would reject that pin. This closes the push route INTO THIS FILE, not the class: `docker-build.yml` remains reachable from an arbitrary ref by a `v*` tag push and by `pull_request`, and four workflows carry an unrestricted `workflow_dispatch` (#853).' signals: 'unfiltered push trigger, branches filter on push, attacker YAML on a runner, feature branch publishes an image, self-referencing workflow trigger, ci-image-pin expected goes stale, publish-then-pin two-step, toolchain image recovery after a rebase, workflow_dispatch on a branch · paths: `.gitea/workflows/ci-image.yml`, `.gitea/workflows/pr-checks.yml`, `.gitea/workflows/docker-build.yml`, `docker/ci/Dockerfile`, `docs/ci-cd.md`, `docs/decisions/records/ci/actions-credential-scoping.md`, `scripts/e2e-ui.sh` · issues: #744, #853, #697, #746, #835, #445, #390, #594, #597' mechanics: '`docs/ci-cd.md` -> "CI toolchain image" -> "Publishing from a branch is a dispatch, not a push"' --- @@ -16,9 +16,13 @@ mechanics: '`docs/ci-cd.md` -> "CI toolchain image" -> "Publishing from a branch `push` workflow's definition from the pushed branch — so any branch push touching `docker/ci/**` or the workflow file ran that branch's own YAML, with no PR, no review and no status check in the loop, on a docker-capable runner holding the credential that writes `ersatztv:prod` and the - `ersatztv-ci:` that five `container:` jobs execute. `branches: [main]` closes it. - `workflow_dispatch` remains self-supplied from the selected branch and is not equivalent: it takes a - deliberate act by someone who already has write access. + `ersatztv-ci:` that five `container:` jobs execute. `branches: [main]` closes the **drive-by** + route, and only that: the filter is loaded from the pushed ref like every other line in the file, so + a branch that deletes it re-enables the route in one edit. What goes away is publication as a side + effect of an ordinary push of a legitimate change — a real and frequently-trodden path, and not a + boundary against someone who intends to run their own YAML. Do not cite this record as that + boundary. `workflow_dispatch` is self-supplied from the selected branch in the same way, and is now + the deliberate publish path. - **The capability removed was real, so it is replaced rather than argued away.** An earlier attempt (#697) justified the filter with "publishing from a feature branch was not load-bearing", which is false: `ci-image-pin` is blocking, so a PR changing `docker/ci/**` without moving the pin is red, and @@ -32,8 +36,8 @@ mechanics: '`docs/ci-cd.md` -> "CI toolchain image" -> "Publishing from a branch workable *because* the branch dispatch survives — push the ci-image.yml commit as HEAD, dispatch, pin it — so the necessity argument does not hold. What does hold is the price: that route charges a full ~2GB publish plus a five-pin bump for **every** edit to the file, a comment included, and a - rebase charges it again. That price is what reverted #697's attempt and what forced this change to - land alone. Dropping it prices the opposite risk instead (the bullet below). The second was chosen + rebase charges it again. That price is what reverted #697's attempt and what made landing this change + alone the cheaper option. Dropping it prices the opposite risk instead (the bullet below). The second was chosen because `ci-image.yml` is edited far more often for triggers, comments and runner placement than for build recipe, so the first cost is paid constantly and the second only on a change that can be paired with a `docker/ci` touch deliberately. diff --git a/docs/decisions/records/release/main-direct-push-disabled.md b/docs/decisions/records/release/main-direct-push-disabled.md index 576bed3af..7ca5c0933 100644 --- a/docs/decisions/records/release/main-direct-push-disabled.md +++ b/docs/decisions/records/release/main-direct-push-disabled.md @@ -67,7 +67,7 @@ ordering is the part worth keeping, as a rule for the next time: unmask the depe then set the flag. Under `|| true` a credential regression does not fail the job — it yields an empty changed-file set that the skip logic reads as "nothing changed". The population is also wider than the six checkouts #743 recorded: eight in `docker-build.yml`, five in `pr-checks.yml`, one in -`dependency-scan.yml`, plus `review-verdict.yml`'s pre-existing one — 15 of the repo's 16. Details in +`dependency-scan.yml`, plus `review-verdict.yml`'s pre-existing one — 15 of the repo's 16; the sixteenth, `ci-image.yml`'s, followed in #744. Details in `ci.actions-credential-scoping`. **Why this is not redundant with the Husky pre-push hooks.** `.husky/pre-push` guards (H6 done-when, diff --git a/scripts/tests/test_workflow_persist_credentials.py b/scripts/tests/test_workflow_persist_credentials.py index b3b03dc48..a8aa7986e 100644 --- a/scripts/tests/test_workflow_persist_credentials.py +++ b/scripts/tests/test_workflow_persist_credentials.py @@ -17,6 +17,11 @@ POPULATION. The git index, via `tracked_files.tracked_paths` — not a filesyste hand-written list (`testing.guard-derives-population-from-source`, ersatztv#806). Both `*.yml` and `*.yaml` are matched: Gitea accepts either spelling, so a `.yaml` workflow would otherwise be structurally invisible to a check that reads as covering all of them. + +`.github/workflows` is OUT of the population and is asserted empty rather than assumed so: Gitea +reads it only when `.gitea/workflows` is absent, which is a precedence rule this repo has not probed, +and `.github/` already exists here (ISSUE_TEMPLATE), so it is a plausible place to add a workflow by +habit. `test_no_workflow_hides_in_a_SUBDIRECTORY` covers nesting below `.gitea/workflows`. """ from __future__ import annotations @@ -40,7 +45,7 @@ CHECKOUT = "actions/checkout" # Matches the `uses:` line the YAML walk is supposed to reach, in every spelling below. Used ONLY as # an independent second opinion on how many there are — never as the population itself. _USES_CHECKOUT = re.compile( - r"^\s*-?\s*uses:\s*['\"]?(?:[^'\"\s]*/)?actions/checkout(@|\s|['\"]|$)", + r"^\s*-?\s*uses:\s*['\"]?(?:[^'\"\s]*/)?actions/checkout(?:\.git)?(@|\s|['\"]|$)", re.M | re.I, ) @@ -56,12 +61,20 @@ def _canonical_action(uses: str) -> str: It over-matches in one direction on purpose: a genuinely different action whose path happens to END in `actions/checkout` would be treated as a checkout and required to carry the flag. That costs a spurious requirement on an action nobody has; the opposite error costs a live credential. + Spellings normalised here, each of which escaped an earlier version of this function: an absolute + URL with or without a scheme, a host with no dot, a doubled slash, `@`-userinfo before the host, + a `.git` suffix, and any letter case. """ - ref = uses.strip().strip("'\"").split("@", 1)[0].strip().lower() + # rsplit, not split: a URL may carry userinfo (`https://user@host/actions/checkout@v4`), and + # splitting on the FIRST `@` would strip the host instead of the version. + ref = uses.strip().strip("'\"").rsplit("@", 1)[0].strip().lower() ref = re.sub(r"^https?://", "", ref) + # Empty segments absorb a doubled slash; `.git` is how Gitea itself writes the clone URL. segments = [seg for seg in ref.split("/") if seg] + if segments: + segments[-1] = re.sub(r"\.git$", "", segments[-1]) if ref.startswith("./") or ref.startswith("../") or len(segments) < 2: - return ref + return "/".join(segments) if segments else ref return "/".join(segments[-2:]) @@ -72,12 +85,15 @@ def workflow_files() -> list[Path]: def _checkout_steps(doc: object) -> list[tuple[str, int, dict]]: """Every `actions/checkout` step in a parsed workflow, as (job id, index within job, step). - Reads `jobs..steps` only, which is the whole shape this repo uses. Two different escapes are - reported by two different tests, and they are not interchangeable: + Reads `jobs..steps` only, which is the whole shape this repo uses. Different escapes are + reported by different tests, and they are not interchangeable: `test_the_walk_finds_every_actions_checkout_the_TEXT_does` covers a shape INSIDE a workflow file that the walk cannot reach, because both halves read the same files. A **composite action** is invisible to both halves — its `action.yml` is not in the workflow population at all — so that - case is reported by `test_no_LOCAL_COMPOSITE_ACTION_exists_AT_ALL` instead. + case is reported by `test_no_LOCAL_COMPOSITE_ACTION_exists_AT_ALL` instead. A **reusable + workflow** (`jobs..uses:`) is a third shape, invisible to both halves in the same way, and is + reported by `test_no_job_DELEGATES_to_a_reusable_workflow` — it is not covered by the text + cross-check, which would see zero on both sides and agree. """ found: list[tuple[str, int, dict]] = [] if not isinstance(doc, dict): @@ -200,6 +216,11 @@ def test_the_fault_collector_reports_a_checkout_that_OMITS_the_flag() -> None: "github.com/actions/checkout@v4", # The forge is case-insensitive on owner/repo, so this runs the real checkout. "Actions/Checkout@v4", + # Gitea writes the clone URL with the suffix; both halves must accept it. + "https://github.com/actions/checkout.git@v4", + # Userinfo before the host — splitting on the FIRST `@` used to strip the host. + "https://user@github.com/actions/checkout@v4", + "https://github.com//actions/checkout@v4", ): as_url = {"jobs": {"j": {"steps": [{"uses": spelling}]}}} assert len(checkout_faults("synthetic.yml", as_url)) == 1, spelling @@ -239,3 +260,42 @@ def test_no_LOCAL_COMPOSITE_ACTION_exists_AT_ALL() -> None: "the job, so an `actions/checkout` in one is subject to the same rule and is NOT covered by " "the workflow-file population above — extend `_checkout_steps` to walk them." ) + + +def test_no_job_DELEGATES_to_a_reusable_workflow() -> None: + """A `jobs..uses:` callee's steps run in the job but are outside the walk. + + Both halves would see zero and AGREE, so the count cross-check reports nothing — the same blind + spot the composite-action test exists for. Gitea Actions has no `workflow_call` today, so assert + the absence rather than parse a shape that cannot occur: the first one added reddens here and its + author widens the guard. + """ + delegating = [] + for path in workflow_files(): + doc = yaml.safe_load(path.read_text()) + jobs = doc.get("jobs") if isinstance(doc, dict) else None + if not isinstance(jobs, dict): + continue + for job_id, job in jobs.items(): + if isinstance(job, dict) and isinstance(job.get("uses"), str): + delegating.append(f"{path.name}: job `{job_id}` -> {job['uses']}") + assert not delegating, ( + f"job(s) delegating to a reusable workflow: {delegating}. The callee's `actions/checkout` " + "steps run in this job and are subject to the same rule, but are NOT in the population " + "above — extend the walk before adding one." + ) + + +def test_no_workflow_lives_under_dot_GITHUB() -> None: + """`.github/workflows` is out of the population, so prove it is empty rather than assume it. + + Gitea reads `.github/workflows` only when `.gitea/workflows` is absent — a precedence rule this + repo has never probed — and `.github/` already exists here, so it is a plausible place to add a + workflow out of habit. Cheap to assert; silently uncovered otherwise. + """ + stray = sorted(p for p in _git_ls_files() if p.startswith(".github/workflows/")) + assert not stray, ( + f"tracked workflow file(s) under .github/workflows: {stray}. This guard's population is " + f"{WORKFLOW_DIR} only — decide whether Gitea runs these and widen the population or delete " + "them, but do not leave them unchecked." + ) -- 2.47.3 From aa406acd26fa8ebbb000865cf7eea39a75f61c85 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 21:41:48 +0200 Subject: [PATCH 5/8] fix(744): apply the ci-cd.md edits the round-3 batch script silently dropped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A batched edit script asserts each old string then writes ONCE at the end, so a single failed assert discards every earlier replacement in the same run — the `batched-edit-script-loses-every-edit` trap. Round 3's ci-cd.md script aborted on its last block, and the five earlier edits it reported were never written. The workflow, pr-checks and record halves of that round landed from separate scripts, so only this file was affected, and only a whole-repo CONCEPT sweep caught it: `docs/ci-cd.md` still carried the headline **A branch push no longer publishes anything**, the very sentence round 3 exists to narrow. Now applied, one write per edit: the drive-by narrowing of the security claim, the does-not-close heading, the tag-push wording, priced-decision language for landing alone, and the ordering constraint reading 'in that commit or below it'. refs #744 --- docs/ci-cd.md | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 0b2378dde..4c766bfde 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1759,14 +1759,22 @@ that still exists in the registry. ### Publishing from a branch is a dispatch, not a push (ersatztv#744) -**A branch push no longer publishes anything.** `ci-image.yml`'s `push` trigger is `branches: [main]`, -because Gitea resolves a `push` workflow's definition from the pushed branch — so an unfiltered -trigger meant any branch push touching those paths executed that branch's own YAML, unreviewed, on a -docker-capable runner holding the credential that writes `ersatztv:prod` and the toolchain image -five `container:` jobs run. Credential scoping (`ci.actions-credential-scoping`) bounds what such a -job may *do*; it never bounded whether it runs. +**A branch push of this file's committed form no longer publishes anything.** `ci-image.yml`'s +`push` trigger is `branches: [main]`, because Gitea resolves a `push` workflow's definition from the +pushed branch — so an unfiltered trigger meant any branch push touching those paths executed that +branch's own YAML, unreviewed, on a docker-capable runner holding the credential that writes +`ersatztv:prod` and the toolchain image five `container:` jobs run. Credential scoping +(`ci.actions-credential-scoping`) bounds what such a job may *do*; it never bounded whether it runs. -**What this does not close — and sort it by REF CLASS, not by which keyword a trigger carries.** +**Read that heading exactly as narrow as it is written.** The filter is loaded from the pushed ref +like every other line in the file, so a branch that *deletes* it re-enables the route in one edit. +What is closed is the **drive-by** case: an ordinary push of a legitimate `docker/ci` change +publishing an image nobody asked for, with no deliberate act anywhere in the loop. Against someone +who intends to run their own YAML this is not a boundary at all, and citing it as one is the mistake +to avoid — see #853. + +**What this does not close — beginning with this file's own filter, then sorted by REF CLASS +rather than by which keyword a trigger carries.** `docker-build.yml` has `branches: [main]`, so a keyword test passes it; it also has `tags: ['v*']` and `pull_request:`, both head-resolved and both reaching the same registry credential. A tag push is outside `release.main-direct-push-disabled`, so `git push origin :refs/tags/v99.9.9` runs @@ -1776,10 +1784,9 @@ unrestricted `workflow_dispatch`, which loads its definition from the selected r removes the *drive-by* route, where merely pushing a branch was enough; it is not a boundary against a malicious writer. The whole remaining class is tracked in #853. -**A tag push does not fire `ci-image.yml`.** Gitea's push matcher gives a `branches:`-only filter no -match for a non-branch ref, and there is no `tags:` key to satisfy instead — so a release cut no -longer republishes the toolchain image as a side effect, which it could when `paths:` was the only -filter. +**A `v*` tag push does not fire `ci-image.yml`.** There is no `tags:` key, and a `branches:` filter +is compared against a branch ref — so a release cut no longer republishes the toolchain image as a +side effect. The exact matcher semantics are unprobed; the observable claim is the one stated. The capability removed is real and is **not** assumed away — it is replaced by `workflow_dispatch`, which Gitea loads from the branch you select. The constraint to plan around is that `ci-image.yml` @@ -1787,8 +1794,8 @@ tags `git rev-parse --short HEAD`, i.e. **the branch HEAD at dispatch time**, no touched `docker/ci`. So the branch-side two-step is: 1. Push the branch with the `docker/ci/**` change as **HEAD**. Anything that changes how the image is - built — a `ci-image.yml` edit to build-args, Dockerfile path or platforms — must already be - **below** that commit, because the dispatch builds HEAD's copy of every file. Commit it afterwards + built — a `ci-image.yml` edit to build-args, Dockerfile path or platforms — must be **in that + commit or below it**, because the dispatch builds HEAD's whole tree. Commit it afterwards and the published image was built by the *old* recipe while the pin and `expected` both name the `docker/ci` commit, so `ci-image-pin` is green over an image the merged recipe would not produce. 2. Dispatch `ci-image.yml` on that branch (Actions → *Build CI Toolchain Image* → *Run workflow* → @@ -1816,7 +1823,7 @@ tradeoff, not a necessity** — keeping the file in `expected` still works, sinc can publish the ci-image.yml commit itself and the pin then matches. What it costs is the reason it went: that route charges a full ~2 GB publish and a five-pin bump for *every* edit to the file, a comment included, and a rebase charges it again. That price reverted the first attempt (#697) and is -why this change had to land alone. The chosen side pays the opposite risk instead, below. +why this change was landed alone — also a priced decision, not a rule. The chosen side pays the opposite risk instead, below. The consequence to live with: if you change **how** the image is built (build-args, Dockerfile path, platforms) without touching `docker/ci/**`, CI keeps running an image built by the previous recipe and nothing goes red. **Make the change alongside a `docker/ci/**` edit** — a comment bump in the -- 2.47.3 From 43b1e45b55548ef5e1f452e40ecedb08acf94b6a Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 21:56:29 +0200 Subject: [PATCH 6/8] fix(744): qualify the two 'ci-image-pin is blocking' conclusions, rewrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-4 self-review. `ci-image-pin` is NOT a branch-protection required context — pr-checks.yml says so in as many words (branch protection requires only `Build & test (.NET)`, `EF migration integrity` and `review-verdict/h10`). The block is real but comes from the merge-consent hook reading the COMBINED commit status. 'Blocking' is pre-existing shorthand for this job and is left alone where it is only a label; the two sites that hang a CONCLUSION on it ('so a PR changing docker/ci without moving the pin is red, and the only way to move the pin was...') are this branch's, and are qualified. Fixing the workflow copy last round and not its twin in the record is the fix-one-site-miss-its-twin shape. refs #744 Decisions-Edit: yes --- docs/ci-cd.md | 6 ++++-- .../records/ci/toolchain-image-publish-is-a-dispatch.md | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 4c766bfde..ee85554c8 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1697,7 +1697,8 @@ ersatztv#299 seeded-media/scanner E2E follow-ups will need. **Bumping the toolchain is a deliberate two-step.** The jobs pin an immutable `:`, never `:latest`, so a bad toolchain push cannot break every job at once. Both steps land in the **same -PR** — `ci-image-pin` is blocking, so a PR that changes `docker/ci/**` without moving the pin is red: +PR** — a PR that changes `docker/ci/**` without moving the pin turns `ci-image-pin` red, and the +merge-consent hook reads the combined status, so it will not auto-grant: 1. **Publish.** Push the branch with the `docker/ci/Dockerfile` change as HEAD, then dispatch `ci-image.yml` on that branch; it publishes `ersatztv-ci:`. (`:latest` moves only from @@ -1823,7 +1824,8 @@ tradeoff, not a necessity** — keeping the file in `expected` still works, sinc can publish the ci-image.yml commit itself and the pin then matches. What it costs is the reason it went: that route charges a full ~2 GB publish and a five-pin bump for *every* edit to the file, a comment included, and a rebase charges it again. That price reverted the first attempt (#697) and is -why this change was landed alone — also a priced decision, not a rule. The chosen side pays the opposite risk instead, below. +why this change was landed alone — also a priced decision, not a rule. The chosen side pays the +opposite risk instead, below. The consequence to live with: if you change **how** the image is built (build-args, Dockerfile path, platforms) without touching `docker/ci/**`, CI keeps running an image built by the previous recipe and nothing goes red. **Make the change alongside a `docker/ci/**` edit** — a comment bump in the diff --git a/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md b/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md index 6c2ff2abf..245e83cd2 100644 --- a/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md +++ b/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md @@ -25,8 +25,10 @@ mechanics: '`docs/ci-cd.md` -> "CI toolchain image" -> "Publishing from a branch the deliberate publish path. - **The capability removed was real, so it is replaced rather than argued away.** An earlier attempt (#697) justified the filter with "publishing from a feature branch was not load-bearing", which is - false: `ci-image-pin` is blocking, so a PR changing `docker/ci/**` without moving the pin is red, and - the only way to move the pin was to publish from the branch first. The replacement is the same + false: a PR changing `docker/ci/**` without moving the pin turns `ci-image-pin` red, and the + merge-consent hook reads the COMBINED commit status, so it will not auto-grant — the job is not + itself a branch-protection required context, but the effect is the same. The only way to move the + pin was to publish from the branch first. The replacement is the same two-step with a dispatch in place of the push — push the `docker/ci` commit as HEAD, dispatch `ci-image.yml` on that branch (it tags `git rev-parse --short HEAD`, so the two must coincide), then commit the pin bump on top. A rebase rewrites the sha, so all three are redone; that was already true. -- 2.47.3 From a4d9d8f788cbc351e86a8fc427a73bbede03fa57 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 22:21:24 +0200 Subject: [PATCH 7/8] docs(744): record the branch-dispatch probe, dated and with its limit stated The replacement publish path was the one load-bearing runtime claim in this change, flagged by every review round as asserted-in-prose rather than measured. Now measured. Dispatched ci-image.yml on this branch at 43b1e45 (run 2340, conclusion: success). Gitea bound the job to head_branch=fix/744-ci-image-branch-filter and head_sha=43b1e45b5554, so a workflow_dispatch does load both the definition and the tree from a selected NON-DEFAULT ref; the run published ersatztv-ci:43b1e45, i.e. it tags the branch HEAD at dispatch time as documented; and :latest still resolved to the digest it carried before the dispatch. The limit of that last observation is recorded with it rather than glossed: docker/ci was unchanged, so the rebuild was content-identical and the new : carries the same digest as :latest. It shows :latest did not CHANGE; it cannot by itself distinguish 'not pushed' from 'pushed the same digest'. The refs/heads/main guard is what decides that, and it is called out at its own site. refs #744 Decisions-Edit: yes --- docs/ci-cd.md | 13 ++++++++++++- .../ci/toolchain-image-publish-is-a-dispatch.md | 5 ++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index ee85554c8..37e369453 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1806,7 +1806,18 @@ touched `docker/ci`. So the branch-side two-step is: `expected` is the `docker/ci` commit, which the pin now names. After a rebase, redo all three: the sha is new, so the image must be republished under it. That is the -loop ersatztv#445 paid twice when `main` moved mid-review. The cheapest way to avoid it entirely is +loop ersatztv#445 paid twice when `main` moved mid-review. + +> **Probed on this instance 2026-08-27**, because the whole branch-side path rests on it. Dispatched +> `ci-image.yml` on `fix/744-ci-image-branch-filter` at `43b1e45` (run 2340, `conclusion: success`). +> Gitea bound the job to `head_branch: fix/744-ci-image-branch-filter` / `head_sha: 43b1e45b5554…`, +> so it loads both the definition and the tree from the selected non-default ref, and the run +> published `ersatztv-ci:43b1e45` — it tags the branch HEAD at dispatch time, as described above. +> `:latest` still resolved to `sha256:7baf403c…`, the digest it carried before the dispatch. Note the +> limit of that last check: `docker/ci` was unchanged, so the rebuild was content-identical and the +> new `:` carries the same digest as `:latest`. It therefore shows `:latest` did not CHANGE, and +> cannot by itself distinguish "not pushed" from "pushed the same digest" — the `refs/heads/main` +> guard in the workflow is what makes that decision. The cheapest way to avoid it entirely is still to **land a toolchain-image change on its own, before** the work that consumes it, so the consuming branch never carries the `docker/ci` commit through a rebase. diff --git a/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md b/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md index 245e83cd2..029b58685 100644 --- a/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md +++ b/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md @@ -7,7 +7,7 @@ supersedes: none superseded-by: none rule: 'A `push` trigger reachable from any ref other than `main` — that is `branches:` AND `tags:`, judged by the ref class it admits and never by which keyword is present — executes ref-supplied YAML, because Gitea resolves a `push` workflow''s definition from the pushed ref — so `ci-image.yml` is `branches: [main]`, and publishing a toolchain image from a feature branch is a deliberate `workflow_dispatch` on that branch rather than a side effect of pushing. Be precise about what a `branches:` filter buys: it is loaded from the pushed ref like the rest of the file, so a branch that deletes it re-enables the route — this removes the DRIVE-BY case, and is not a boundary against a writer who intends to run their own YAML. The self-referencing trigger path `.gitea/workflows/ci-image.yml` came out of BOTH its own `paths:` and `ci-image-pin`''s `expected` in the same change — a DECIDED tradeoff, not a necessity: keeping it is workable via the branch dispatch, but prices every edit to that file, comments included, at a ~2GB publish plus a five-pin bump, redone after every rebase. The cost is stated, not assumed away — a change to HOW the image is built that lives only in `ci-image.yml` no longer republishes, and the ONLY remedy is to make it alongside a `docker/ci/**` edit: publishing after the merge and then pinning cannot work, because `expected` is the last `docker/ci` commit and would reject that pin. This closes the push route INTO THIS FILE, not the class: `docker-build.yml` remains reachable from an arbitrary ref by a `v*` tag push and by `pull_request`, and four workflows carry an unrestricted `workflow_dispatch` (#853).' signals: 'unfiltered push trigger, branches filter on push, attacker YAML on a runner, feature branch publishes an image, self-referencing workflow trigger, ci-image-pin expected goes stale, publish-then-pin two-step, toolchain image recovery after a rebase, workflow_dispatch on a branch · paths: `.gitea/workflows/ci-image.yml`, `.gitea/workflows/pr-checks.yml`, `.gitea/workflows/docker-build.yml`, `docker/ci/Dockerfile`, `docs/ci-cd.md`, `docs/decisions/records/ci/actions-credential-scoping.md`, `scripts/e2e-ui.sh` · issues: #744, #853, #697, #746, #835, #445, #390, #594, #597' -mechanics: '`docs/ci-cd.md` -> "CI toolchain image" -> "Publishing from a branch is a dispatch, not a push"' +mechanics: '`docs/ci-cd.md` -> "CI toolchain image" -> "Publishing from a branch is a dispatch, not a push"; branch-dispatch probe 2026-08-27 (run 2340, published `ersatztv-ci:43b1e45`)' --- - **Credential scoping bounds what a job may DO; it never bounded whether the job RUNS.** That is the @@ -32,6 +32,9 @@ mechanics: '`docs/ci-cd.md` -> "CI toolchain image" -> "Publishing from a branch two-step with a dispatch in place of the push — push the `docker/ci` commit as HEAD, dispatch `ci-image.yml` on that branch (it tags `git rev-parse --short HEAD`, so the two must coincide), then commit the pin bump on top. A rebase rewrites the sha, so all three are redone; that was already true. + Probed rather than reasoned: dispatching on `fix/744-ci-image-branch-filter` at `43b1e45` succeeded + (run 2340), the job bound to that branch and sha, and it published `ersatztv-ci:43b1e45` without + moving `:latest` (2026-08-27, `docs/ci-cd.md`). - **Dropping the self-path is a DECIDED TRADEOFF, and calling it forced would be wrong.** `ci-image-pin` derives `expected` from the last commit touching the image sources; while `.gitea/workflows/ci-image.yml` was one of them, editing that file re-pointed `expected` at the editing commit. Retaining it stays -- 2.47.3 From 3dd5109279d216a8190d68061f3152a7ab1283f0 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 27 Aug 2026 22:22:29 +0200 Subject: [PATCH 8/8] docs(744): close the probe blockquote, and drop an overclaim it had just caveated Self-review of the probe commit, on its own head. Two defects in it: - The blockquote swallowed the start of the sentence that follows it, so 'The cheapest way to avoid it entirely is' rendered inside the quote while its continuation rendered outside. Closed the quote and made the sentence stand alone. - The decision record asserted the probe published the tag 'without moving :latest'. That is the exact overclaim the ci-cd.md probe note had just been careful to caveat: the rebuild was content-identical, so the observation is that :latest did not CHANGE, which cannot distinguish 'not pushed' from 'pushed the same digest'. The record now says :latest resolved to its pre-dispatch digest and points at the stated limit instead of restating a stronger claim than the evidence supports. refs #744 Decisions-Edit: yes --- docs/ci-cd.md | 8 +++++--- .../records/ci/toolchain-image-publish-is-a-dispatch.md | 5 +++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 37e369453..afdbf921d 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1817,9 +1817,11 @@ loop ersatztv#445 paid twice when `main` moved mid-review. > limit of that last check: `docker/ci` was unchanged, so the rebuild was content-identical and the > new `:` carries the same digest as `:latest`. It therefore shows `:latest` did not CHANGE, and > cannot by itself distinguish "not pushed" from "pushed the same digest" — the `refs/heads/main` -> guard in the workflow is what makes that decision. The cheapest way to avoid it entirely is -still to **land a toolchain-image change on its own, before** the work that consumes it, so the -consuming branch never carries the `docker/ci` commit through a rebase. +> guard in the workflow is what makes that decision. + +The cheapest way to avoid the rebase loop entirely is still to **land a toolchain-image change on its +own, before** the work that consumes it, so the consuming branch never carries the `docker/ci` commit +through a rebase. > **`docker/ci/**` has no cheap-edit path, comments included.** Editing any byte there re-points > `ci-image-pin`'s `expected` at the editing commit, so the five `container:` pins go stale and a diff --git a/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md b/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md index 029b58685..11df65ba0 100644 --- a/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md +++ b/docs/decisions/records/ci/toolchain-image-publish-is-a-dispatch.md @@ -33,8 +33,9 @@ mechanics: '`docs/ci-cd.md` -> "CI toolchain image" -> "Publishing from a branch `ci-image.yml` on that branch (it tags `git rev-parse --short HEAD`, so the two must coincide), then commit the pin bump on top. A rebase rewrites the sha, so all three are redone; that was already true. Probed rather than reasoned: dispatching on `fix/744-ci-image-branch-filter` at `43b1e45` succeeded - (run 2340), the job bound to that branch and sha, and it published `ersatztv-ci:43b1e45` without - moving `:latest` (2026-08-27, `docs/ci-cd.md`). + (run 2340), the job bound to that branch and sha, and it published `ersatztv-ci:43b1e45`; `:latest` + still resolved to its pre-dispatch digest, a check whose stated limit lives with the probe + (2026-08-27, `docs/ci-cd.md`). - **Dropping the self-path is a DECIDED TRADEOFF, and calling it forced would be wrong.** `ci-image-pin` derives `expected` from the last commit touching the image sources; while `.gitea/workflows/ci-image.yml` was one of them, editing that file re-pointed `expected` at the editing commit. Retaining it stays -- 2.47.3