diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index 715415caf..eccb2fb46 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -471,10 +471,12 @@ jobs: # --- Whose verdict is it? (ersatztv#698 route 3) ------------------------------------- # This short-circuit used to exit on ANY existing `success`, which made an exemption this job # wrote indistinguishable from a verdict a human wrote. That is what let a forged exemption - # survive: obtained once — via the route-1 retarget race, a sibling workflow holding - # status-write credentials (#697), a direct API call, or inheritance across PRs by sha (#663) - # — it was thereafter accepted unchanged on every run, because the guard exited before it - # looked at the PR, the base, the author or the files. + # survive: obtained once — via the route-1 retarget race, a collaborator's own token, the + # `GITEA_TOKEN` injected into every job, `RENOVATE_TOKEN`, a direct API call, or inheritance + # across PRs by sha (#663) — it was thereafter accepted unchanged on every run, because the + # guard exited before it looked at the PR, the base, the author or the files. (`#697`'s + # registry credential can no longer post a status at all — see `ci.actions-credential-scoping` + # — but that closes only one route; the others above still work.) # # The guard still exists for its original reason: re-posting `pending` over a real human # verdict would un-approve a reviewed head and stall the PR. So it discriminates by PROVENANCE. @@ -491,9 +493,11 @@ jobs: # machine-written") an unrecognised shape would be trusted — the fail-open this issue is about. # # What this does NOT claim: the test asks "was this POSTed by a user credential", NOT "by a - # reviewer". `ETV_STATUS_AUTH` is basic auth, so head-controlled code can POST a success with a - # non-null creator AND an attacker-chosen `Review-verdict:` description, which this guard then - # preserves. That is #697 — provenance, not authentication. + # reviewer". `ETV_STATUS_AUTH`'s registry credential can no longer POST a status at all (#697, + # fixed by scoping it off `write:repository` — see `ci.actions-credential-scoping`), but any + # OTHER user credential — a collaborator's own token, `RENOVATE_TOKEN` — can still POST a + # success with a non-null creator AND an attacker-chosen `Review-verdict:` description, which + # this guard then preserves. That is provenance, not authentication. read_existing_verdict if [ "$ex_human" = yes ] && { [ "$ex_state" = "success" ] || [ "$ex_state" = "failure" ]; }; then echo "${CONTEXT} is already '${ex_state}' on ${SHA:0:7}, written by '${ex_creator}' as a human verdict — leaving it alone." diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 4375c419b..8479c8e39 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -413,6 +413,14 @@ the image build. `insecure-registries`**, so without this, cache/base-image/push over the HTTP registry fails (`http: server gave HTTP response to HTTPS client`). 3. `docker/login-action` with repo secrets `REGISTRY_USER` / `REGISTRY_PASSWORD`. + **`REGISTRY_PASSWORD` is a scoped PAT (`write:package` + `read:repository`), not an account + password** — deliberately, so head-resolved PR code cannot use it to forge a commit status + (`ci.actions-credential-scoping`, ersatztv#697). If a job ever fails with `token does not have at + least one of required scope(s)`, the fix is to narrow what the job does, **never** to widen the + token to `write:repository` or to put the admin password back. Note what the scope still reaches: + `write:package` covers `ersatztv:prod` (the tag prod's stack follows) and `ersatztv-ci:` (the + toolchain image five `container:` jobs execute), so this is the deployment supply chain, not an + inert endpoint — see `ci.actions-credential-scoping`. 4. `docker/build-push-action@v6`: amd64-only, `docker/Dockerfile`, `INFO_VERSION` build-arg, registry layer cache (`type=registry,ref=…:buildcache`, `cache-to … ignore-error=true`). @@ -834,7 +842,18 @@ as a human verdict — a non-null `.creator.login` **and** a `Review-verdict:` d **re-derived** rather than inherited. (Measured: a status POSTed with a user credential carries a creator; one POSTed by an Actions job carries `"creator": null`.) Without this, an exemption obtained once was accepted unchanged on every later run. This is a *provenance* check, not an authentication -one — someone who can POST statuses directly can still impersonate a verdict, which is ersatztv#697. +one — someone who can POST statuses directly can still impersonate a verdict (ersatztv#697). That +provenance asymmetry is *why* the credential scoping in `ci.actions-credential-scoping` mattered: a +forgery through a **user** credential inherits as a human verdict, while one through a job's +`GITEA_TOKEN` carries `creator: null` and is re-derived, so it must win a race. CI's registry secret +was a user credential — the admin account — and no longer carries status-write. **`RENOVATE_TOKEN` +still is one** (`write:repository`, a real bot account), and secrets are a per-repo store any +PR-added workflow can reference, so that route is narrowed rather than closed; tightening this check +from "non-null creator" to an allow-list of approved reviewers is what would close it +(ersatztv#742). A collaborator's own personal token still can, and no repo-side change closes that. +Note also that re-derivation is **not** a race the attacker can lose: it fires only on the trigger's +`types`, and posting a status is not one of them, so a POST timed after the last PR event stands +until the next one. Deciding either exemption requires the PR's **complete** changed-file list, which the workflow does not compute itself: it calls `scripts/pr-changed-files.sh`, the single shared implementation also @@ -901,12 +920,17 @@ as establishing that the gate cannot be forged (see the residual below, and ersa here **only** because this job never checks out or executes head-supplied code. Verified on this instance with four scratch PRs rather than inferred from GitHub; full rationale in `docs/decisions/records/ci/gate-trigger-base-resolved.md`. **This closes the rewrite route through - this workflow, not the class:** `docker-build.yml` is also head-resolved and its `ETV_STATUS_AUTH` - credentials can write statuses, so it can still forge `review-verdict/h10` — it must stay on - `pull_request` because it builds the PR's code, so it needs a read-only status identity instead - (ersatztv#697) — and the inventory is every workflow, not that one, because Gitea injects a - write-capable `GITEA_TOKEN` into every job and branch protection binds the *context*, not its - issuer. The exemption path has separate defects of its own (ersatztv#698). One operational + this workflow, not the class:** `docker-build.yml` is also head-resolved and must stay on + `pull_request` because it builds the PR's code, so it got the read-only status identity instead — + its `ETV_STATUS_AUTH` is now a PAT scoped `write:package` + `read:repository`, which the status + endpoint refuses (`ci.actions-credential-scoping`, ersatztv#697). The inventory was never that one + workflow, though: Gitea injects a write-capable `GITEA_TOKEN` into every job and branch protection + binds the *context*, not its issuer. Gitea >=1.26 with the Actions default set to **Restricted** + (server-management#714) binds the injected token, but does not close the class either — not against + a personal token, and not against `RENOVATE_TOKEN` (ersatztv#742). **And none of it is necessary: + direct pushes to `main` are server-side permitted, so the gate can be skipped without any forgery + (ersatztv#743) — a push whitelist would close more of this class than the upgrade does.** The + exemption path has separate defects of its own (ersatztv#698). One operational consequence of the trigger change: a PR whose base is not `main` now gets **no** `review-verdict/h10` at all. That is fail-closed. `edited` **is** now among the trigger's `types` (ersatztv#698), so a PR retargeted onto `main` reclassifies instead of staying statusless until its diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 79765fafa..029ca7310 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -36,6 +36,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` while this instance is below Gitea 1.26.0 — it is silently a NO-OP there, which is worse than absent because it reads in review as a constraint. 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). This record closes ONE route. It does not close the class, and four 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. | 2026-07-21 | [link](records/ci/cancelled-is-not-a-verdict.md) | @@ -46,7 +47,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.exemption-provenance` | The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR's live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR's immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires only for a status POSITIVELY identified as a human verdict for THIS base, meaning a non-null `.creator.login` AND a `Review-verdict:` description AND, when that description records a base (`(base: …)`, `release.verdict-status-check`), a base matching the PR's — tested by requiring the description to END with the exact literal `(base: )` and to contain exactly ONE such marker, never by extracting a value (see below); a present-but-different base is rejected, an absent one is not, since verdicts predating that convention carry none; every other shape, including any unrecognised one, is re-derived rather than trusted. The bot and docs-only exemptions are evaluated as INDEPENDENT predicates and the decision made afterwards, never as an `elif` chain. `edited` is in the workflow's `types:` so a retarget reclassifies — which gives DETECTION, not atomicity: status writes are not serialized, so a stale run can still post over a fresher one. That residual is now FENCED rather than merely tracked — the job refuses to write at all if the PR's timeline retarget COUNT moved while it was classifying (`ci.verdict-write-retarget-fence`, #706) — leaving only the sub-round-trip window that no API without compare-and-set can close. The PROTECTED path list additionally covers `.codex/` (#711), which mirrors `.claude/hooks/` byte for byte including the merge-consent hook — latent while that directory is untracked, live the moment it is tracked; the list stays ENUMERATIVE rather than derived, because a derived rule would have to be evaluated against the very file list being classified. Path predicates are evaluated by COUNTING with `grep -c`, never `\| grep -q` (SIGPIPE inversion) and never a here-string (temp-space failure) — see `ci.grep-q-pipefail-inversion`. | 2026-07-29 | [link](records/ci/exemption-provenance.md) | | `ci.format-gate-folder-mode` | The blocking `format` CI job (and matching pre-commit hook) runs `dotnet format whitespace . --folder --include ` instead of loading the full MSBuild/Roslyn solution, cutting the gate from ~480s to ~0.5s with unchanged whitespace/charset coverage. | 2026-07-19 | [link](records/ci/format-gate-folder-mode.md) | | `ci.functional-e2e-harness` | The `functional-e2e` CI job boots the PR's own code from source via `dotnet run` (`scripts/e2e-local.sh`) and runs deterministic assertions (`scripts/e2e-functional.sh`) as an advisory (non-blocking) job, not a `build` dependency or required check. Originally curl-only; since #445 the same job carries a second, headless-browser step for the contracts curl cannot express — see `ci.ui-e2e-harness`. | 2026-07-16 | [link](records/ci/functional-e2e-harness.md) | -| `ci.gate-trigger-base-resolved` | The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR's own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch's gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree's scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class: Gitea injects a write-capable `GITEA_TOKEN` into EVERY job, so any ref-resolved workflow — and a collaborator's own API token, since branch protection binds the context and not its issuer — can still forge `review-verdict/h10`. Tracked in #697; the exemption path has its own separate defects in #698. | 2026-07-28 | [link](records/ci/gate-trigger-base-resolved.md) | +| `ci.gate-trigger-base-resolved` | The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR's own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch's gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree's scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class: Gitea injects a write-capable `GITEA_TOKEN` into EVERY job, so any ref-resolved workflow — and a collaborator's own API token, since branch protection binds the context and not its issuer — can still forge `review-verdict/h10`. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI's registry secret was the ADMIN account's basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential's route (a user credential's forgery carries a real `creator` and is inherited as a human verdict; an Actions job's carries `creator: null` and is re-derived — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger's `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The injected token stays write-capable until Gitea >=1.26 with a Restricted default (server-management#714), and a collaborator's own token remains unfixable; the exemption path has its own separate defects in #698. | 2026-07-28 | [link](records/ci/gate-trigger-base-resolved.md) | | `ci.gitea-milestone-filter-noop` | Never filter issues with the server-side `?milestones=` parameter — fetch all open issues once and filter LOCALLY on each issue's `.milestone.title`. | 2026-07-21 | [link](records/ci/gitea-milestone-filter-noop.md) | | `ci.grep-q-pipefail-inversion` | In any script running under `set -o pipefail`, a security or classification predicate of the form `producer \| grep -q…` is FORBIDDEN: `grep -q` exits at its first match, the producer then takes SIGPIPE and exits 141 once the data exceeds the pipe buffer (~64K), so `pipefail` reports the pipeline as FAILED even though grep MATCHED — inverting the predicate exactly when the input is large. A here-string (`grep -q… <<< "$data"`) is ALSO forbidden: bash materialises a large here-string via temporary storage, so it fails when temp space is full or unwritable, and inside an `if`/`!` that failure flips the predicate the same way. COUNT instead — `n=$(printf '%s\n' "$data" \| grep -cE "$re")` — because `grep -c` drains stdin (no early exit, no SIGPIPE) over an ordinary pipe (no temp file). Read grep's status honestly: exit 1 means a zero count and is a legitimate answer, anything >1 is a real error. Evaluate the counts ONCE at TOP LEVEL, never inline inside an `if`/`elif` condition: inside `$( )` an `exit` leaves only the subshell and `set -e` does not fire, so an error silently reads as "no match". Validate that each result is numeric and fail closed if not. This applies to both the enforced gate `.gitea/workflows/review-verdict.yml` and the advisory hook `.claude/hooks/pretooluse-merge-consent.sh`. | 2026-07-29 | [link](records/ci/grep-q-pipefail-inversion.md) | | `ci.infra-shaped-red-under-load` | When a job dies inside a setup/cache step before your code compiles, check the runner host's load before diagnosing the diff, and never file a CI bug off one sample under pressure. | 2026-07-21 | [link](records/ci/infra-shaped-red-under-load.md) | diff --git a/docs/decisions/records/ci/actions-credential-scoping.md b/docs/decisions/records/ci/actions-credential-scoping.md new file mode 100644 index 000000000..6b0b296bf --- /dev/null +++ b/docs/decisions/records/ci/actions-credential-scoping.md @@ -0,0 +1,74 @@ +--- +key: ci.actions-credential-scoping +title: '2026-08-05 — CI''s registry credential is a scoped PAT, not the admin password, because Gitea cannot separate status-write from repo-write (#697)' +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` while this instance is below Gitea 1.26.0 — it is silently a NO-OP there, which is worse than absent because it reads in review as a constraint. 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). This record closes ONE route. It does not close the class, and four 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, 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/ci-image.yml`, `.gitea/workflows/renovate.yml`, `scripts/ci-detect-already-validated.sh` · issues: #697, #672, #698, #742, #743, #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.' +--- + +**What was wrong.** `REGISTRY_USER`/`REGISTRY_PASSWORD` were the **admin account's** basic auth, and +`docker-build.yml` triggers on `pull_request` — head-resolved — so a PR's own code got instance-admin +credentials. Basic auth carries no scope: the secret pushing an image administers every repo on the +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`). + +**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 +`ersatztv-ci:` (the toolchain image *executing* five `container:` jobs). A sha-named tag is not an +immutable artifact (no container tag immutability in Gitea 1.25 — INFERRED), so overwriting the pinned +tag is code execution inside CI, chaining back into the routes below. This is the deployment supply +chain for prod and CI itself. + +**Admin ownership is a real residual.** The PAT is minted under `timothy`, a site admin. The 403 proves +the scope gate binds the *status* endpoint ahead of any admin bypass; it does NOT establish that for +*package* endpoints, where Gitea resolves permission by owner and an admin passes object-level checks, +so the token's package reach is plausibly wider than this repo. A non-admin bot account would close +this, but is not free: packages live in a user namespace only its owner and admins can write. Both +halves INFERRED, neither probed. + +**Provenance, corrected.** `review-verdict.yml` leaves an existing `h10` alone only when it is +positively identifiable as human — non-null `.creator.login` plus a `Review-verdict:` description +(`release.verdict-status-check`). A user credential posts with a real creator and is INHERITED; an +Actions job posts `creator: null` and is re-derived. **That asymmetry is not protection.** Re-derivation +fires only on `opened|reopened|synchronize|ready_for_review|edited`, and posting a status is none of +them, so a POST timed after the last event stands until the attacker merges. The gain here is that PR +code can no longer escalate to instance admin — NOT that the durable forgery route is closed. + +**The boundary is everything reachable from a job, not the secret store.** The store is a useful lower +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` (`docker-build.yml` omits `persist-credentials: false`); +and jobs reach the runner's docker daemon. + +**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 +docs, NOT verified (2026-08-05): `permissions:` landed in 1.26.0 (Gitea PR #36173); no `app.ini` lever +at any version; Gitea rejects GitHub's `statuses`/`checks` scopes. + +**Surviving routes — this record is not a mitigation for any of them.** `RENOVATE_TOKEN` is a +`write:repository` bot PAT in the same store, posting with a real creator, and cannot be scoped down +because Renovate needs repo write (#742). The injected `GITEA_TOKEN` is write-capable in every job; +only Gitea >=1.26 with the Actions default set to **Restricted** binds it (server-management#714). A +collaborator's own token always can. `docker-build.yml` publishes `:prod` from a `v*` tag push and a tag +may point at ANY commit — a prod image with no PR, review or status (tag protections are empty). +**And none of it is necessary: direct pushes to `main` are server-side permitted, so the gate is +bypassable with no forgery at all (#743)** — a push whitelist closes more of this class than the 1.26 +upgrade. Treat this list as "at least these", never exhaustive. `SERVERMGMT_DEPLOY_KEY` remains in the +store though its `bump-prod-compose` job went in `1b5efd7b9`, and its key on `timothy/server-management` +is `read_only: false` — write access to the repo holding prod's GitOps stack definitions. Left in place +by explicit decision 2026-08-05; recorded so it is accepted, not forgotten. Severity throughout: push +access required, so a compromised contributor or subverted automated session, never an anonymous one. diff --git a/docs/decisions/records/ci/exemption-provenance.md b/docs/decisions/records/ci/exemption-provenance.md index 2be90abe7..9e0702b6a 100644 --- a/docs/decisions/records/ci/exemption-provenance.md +++ b/docs/decisions/records/ci/exemption-provenance.md @@ -79,9 +79,10 @@ test still fails: the guard degrades toward re-deriving, never toward trusting. **What this does NOT close.** Anyone who can POST statuses directly can write both a creator and a `Review-verdict:` description and impersonate a verdict; branch protection binds the *context*, not its -issuer. A provenance check, not an authentication one — that is `#697`, left open because its durable -fix is credential scoping, partly server-management territory. Severity as `#672`: requires push -access, so the threat model is a compromised contributor. +issuer. A provenance check, not an authentication one — that was `#697`'s registry credential, fixed by +scoping it off `write:repository` (`ci.actions-credential-scoping`); `GITEA_TOKEN`, `RENOVATE_TOKEN`, +and a collaborator's own token still can. Severity as `#672`: requires push access, so the threat model +is a compromised contributor. **Verification honesty.** Route 1 was reproduced live; the "and now it fails" half cannot be shown from a PR, because `pull_request_target` resolves this definition from `main` — the self-test gap diff --git a/docs/decisions/records/ci/gate-trigger-base-resolved.md b/docs/decisions/records/ci/gate-trigger-base-resolved.md index 975f6f3e9..f3a8c81f5 100644 --- a/docs/decisions/records/ci/gate-trigger-base-resolved.md +++ b/docs/decisions/records/ci/gate-trigger-base-resolved.md @@ -5,7 +5,7 @@ status: active since: '2026-07-28' supersedes: none superseded-by: none -rule: 'The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR''s own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch''s gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree''s scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class: Gitea injects a write-capable `GITEA_TOKEN` into EVERY job, so any ref-resolved workflow — and a collaborator''s own API token, since branch protection binds the context and not its issuer — can still forge `review-verdict/h10`. Tracked in #697; the exemption path has its own separate defects in #698.' +rule: 'The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR''s own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch''s gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree''s scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class: Gitea injects a write-capable `GITEA_TOKEN` into EVERY job, so any ref-resolved workflow — and a collaborator''s own API token, since branch protection binds the context and not its issuer — can still forge `review-verdict/h10`. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI''s registry secret was the ADMIN account''s basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential''s route (a user credential''s forgery carries a real `creator` and is inherited as a human verdict; an Actions job''s carries `creator: null` and is re-derived — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger''s `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The injected token stays write-capable until Gitea >=1.26 with a Restricted default (server-management#714), and a collaborator''s own token remains unfixable; the exemption path has its own separate defects in #698.' signals: 'workflow definition resolved from head, PR rewrites the gate that judges it, self-approve a required status check, pull_request_target vs pull_request, gate trigger branches filter, attacker-supplied base branch, how to test a change to review-verdict.yml, workflow not exercised by its own PR, gate edit goes live only on merge, required_approvals 0 does not bind an author, forged commit status inherited by sha · paths: `.gitea/workflows/review-verdict.yml`, `scripts/tests/test_pr_changed_files.py` · issues: #672, #663, #649, #622' mechanics: '`on: pull_request_target: {branches: [main], types: [opened, reopened, synchronize, ready_for_review, edited]}` (`edited` added by `ci.exemption-provenance` so a retarget reclassifies); asserted by `test_the_workflow_trigger_is_pull_request_TARGET_scoped_to_main` in `scripts/tests/test_pr_changed_files.py`; the job''s own context is renamed to `... (pull_request_target)` and must stay OUT of branch protection''s required list' --- @@ -54,7 +54,11 @@ inventory is not a short list: Gitea injects `GITEA_TOKEN` into **every** job, d read/**write**, so head-resolved, `push`-triggered and `workflow_dispatch` workflows alike are routes (1.24+ loads a dispatched definition from the selected branch). A collaborator's own API token is a route with no workflow at all — branch protection binds the *context*, not its issuer. Full inventory -in `#697`; the exemption path's own defects are `#698`. No in-repository test can establish +in `#697`, whose credential half is resolved in `ci.actions-credential-scoping` — the registry secret +no longer carries status-write. That does NOT leave the workflow routes provenance-free: any +PR-added workflow can reference `RENOVATE_TOKEN`, a `write:repository` bot PAT in the same store, +whose status carries a real creator and IS inherited (`#742`). The exemption path's +own defects are `#698`. No in-repository test can establish status-authority isolation: the sibling guard added here catches only plain-text naming of the context. diff --git a/docs/decisions/records/release/verdict-status-check.md b/docs/decisions/records/release/verdict-status-check.md index 9214417b8..00a7803d5 100644 --- a/docs/decisions/records/release/verdict-status-check.md +++ b/docs/decisions/records/release/verdict-status-check.md @@ -182,8 +182,16 @@ described as one: **Only this file's instance is closed, not the class.** Any head-resolved workflow holding credentials that can POST a commit status can still forge `review-verdict/h10`; - `docker-build.yml` demonstrably can, and must stay head-resolved because it builds the PR's own - code. Tracked in #697. So the "careless change rather than a hostile one" posture below still + `docker-build.yml` demonstrably could, and must stay head-resolved because it builds the PR's own + code — so #697 scoped its credential instead (`ci.actions-credential-scoping`), leaving AT LEAST + these: the injected `GITEA_TOKEN` (posts with `creator: null`), `RENOVATE_TOKEN` (a + `write:repository` bot PAT in the same secret store, so it posts with a real creator and IS + inherited, #742), a collaborator's own token, and the `v*` tag push — which matters less for + forging this status than for what else it does: `docker-build.yml` publishes `:prod` from a tagged + ref, and a tag may point at any commit, so it ships a prod image with no PR, review or status. + None of which is even required — direct pushes to `main` are server-side permitted, so the gate + can be skipped without forging anything (#743). So the "careless + change rather than a hostile one" posture below still describes the repo accurately — it is simply no longer *this* workflow that is the weakest link. An untrusted-contributor repo would still need the classification moved somewhere no PR can reach (server-side policy), not merely a base-pinned definition. diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index fefc61acd..b97bced4a 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -791,9 +791,13 @@ def test_no_OTHER_workflow_writes_the_review_verdict_status(): The trigger test above hardens ONE file, and by construction cannot see the more general problem: the gate is forgeable by *any* head-resolved workflow holding credentials that can POST a commit status, not only by the gate's own definition. `docker-build.yml` is exactly that — it - triggers on `pull_request` (correctly; it builds the PR's code) and carries `ETV_STATUS_AUTH`, - which a probe confirmed can WRITE statuses, not merely read them. That residual is tracked in - ersatztv#697 and is NOT closed by this test. + triggers on `pull_request` (correctly; it builds the PR's code) and carries `ETV_STATUS_AUTH`. + A probe once confirmed those credentials could WRITE statuses, not merely read them + (ersatztv#697); a follow-up probe confirmed `REGISTRY_PASSWORD` is now scoped off + `write:repository` and the same POST returns 403 (see `ci.actions-credential-scoping`). That + closes the registry-credential route, not the general problem — `GITEA_TOKEN`, `RENOVATE_TOKEN`, + and a collaborator's own token can all still POST a status, and none of that is closed by this + test. What this test does close is the cheap regression: a second workflow quietly starting to write the context. It is a guard against drift, not a security boundary — a workflow can still write @@ -1703,8 +1707,10 @@ def test_a_LEGACY_verdict_with_no_recorded_base_is_still_honoured(tmp_path): def test_an_APPENDED_base_cannot_override_the_real_one(tmp_path): - """The description is attacker-influencable by anyone who can POST a status (#697), so the parse - must not be trickable into reading a second, appended base. + """The description is attacker-influencable by anyone who can POST a status — the registry + credential's own route closed with #697, but `GITEA_TOKEN`, `RENOVATE_TOKEN`, and a + collaborator's own token still can — so the parse must not be trickable into reading a second, + appended base. This defeated the greedy `##` parse. The implementation no longer parses at all — it requires the description to END with the exact literal marker AND to contain exactly one marker — so a second