From cf907f098853075627ab3ec9eb071631b7875185 Mon Sep 17 00:00:00 2001 From: Timothy Date: Tue, 28 Jul 2026 21:28:38 +0200 Subject: [PATCH 1/6] fix(672): trigger the verdict gate on pull_request_target scoped to main MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `review-verdict.yml` writes `review-verdict/h10`, the status branch protection requires on `main`. It triggered on `pull_request`, and Gitea resolves a `pull_request` workflow DEFINITION from the PR's own head commit — so a PR editing this file ran its own rewritten copy and could post `h10=success` for itself. `PROTECTED` did not help (the rewrite defines it too), and neither did branch protection, which requires the context rather than an author and carries `required_approvals: 0`. The #649 base-ref checkout binds the SCRIPTS this job runs to the base. It cannot bind the job definition. Only the trigger can. Measured on this instance (Gitea 1.25.4) with three throwaway scratch PRs rather than inferred from GitHub, since the premise is a claim about someone else's software: A. A PR rewriting the workflow posted its own probe context and the real `review-verdict/h10` was never written — the base definition never ran. B. Under `pull_request_target` the identical rewrite was ignored: the BASE definition ran and posted `h10=pending`, on `opened` and `synchronize` alike, with `secrets` still available. C. With `branches: [main]`, a PR into a non-main base produced no run and no status at all. The probes only ever posted probe-named contexts, never a forged `h10`. `branches: [main]` is half the fix, not a refinement: base resolution means the BASE branch supplies the definition, so without it the rewrite simply moves to an attacker-pushed base — and a status forged there is inherited by any later real PR with the same head sha (#663). `pull_request_target` is safe here only because this job never checks out or executes head-supplied code; the base-ref checkout is what makes the trigger usable, so the two are one decision. Rejected `required_approvals: 1` as the cheaper fix: Gitea forbids approving your own PR and this is effectively a single-maintainer repo, so it would deadlock every PR rather than gate the dangerous ones. Three mutations confirm the new test discriminates rather than merely passing: reverting to `pull_request`, dropping the `branches` filter, and re-adding `pull_request` alongside the safe trigger each go red with a distinct message. It parses the YAML instead of substring-matching because `pull_request` is a prefix of `pull_request_target`. Refs: #672 Decisions-Edit: yes --- .gitea/workflows/review-verdict.yml | 44 +++++++++++-- docs/ci-cd.md | 15 ++++- docs/decisions/README.md | 1 + .../records/ci/gate-trigger-base-resolved.md | 62 +++++++++++++++++++ .../records/ci/shared-pr-file-enumeration.md | 13 +++- scripts/tests/test_pr_changed_files.py | 55 ++++++++++++++++ 6 files changed, 180 insertions(+), 10 deletions(-) create mode 100644 docs/decisions/records/ci/gate-trigger-base-resolved.md diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index fedfc6841..237b699d2 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -34,10 +34,13 @@ name: Review verdict # status and no further pushes to re-trigger it — Renovate would stall silently. This workflow # therefore takes no cancelling concurrency group. # -# This job's OWN status context ("Review verdict / Set review-verdict status (pull_request)") is -# NOT the required check and is not what gates merges — `review-verdict/h10`, the status it POSTS, -# is. Keeping them distinct is deliberate: a workflow cannot be allowed to satisfy the gate merely -# by running successfully. +# This job's OWN status context ("Review verdict / Set review-verdict status (pull_request_target)") +# is NOT the required check and is not what gates merges — `review-verdict/h10`, the status it +# POSTS, is. Keeping them distinct is deliberate: a workflow cannot be allowed to satisfy the gate +# merely by running successfully. The context string carries the trigger name, so the #672 switch +# renamed it; that is safe only because it was never in branch protection's required list (which is +# the two `docker-build.yml` job contexts plus `review-verdict/h10`). Adding it there later would +# undo the distinction this paragraph exists to protect. # # THE CHANGED-FILE ENUMERATION IS NOT INLINE HERE (ersatztv#649). It lives in # `scripts/pr-changed-files.sh`, the single implementation this job and the advisory hook @@ -54,9 +57,40 @@ name: Review verdict # would be the PR's own. `base.sha` is the commit the PR merges INTO: already on `main`, already # reviewed. `persist-credentials: false` because nothing here pushes, and a checkout that leaves a # token in `.git/config` hands it to every script the job runs. +# +# WHY THE TRIGGER IS `pull_request_target`, NOT `pull_request` (ersatztv#672). The base-ref checkout +# above binds the SCRIPTS this job runs to the base. It does not bind the job DEFINITION. Gitea +# resolves a `pull_request` workflow definition from the PR's own head commit, so a PR editing THIS +# FILE ran its own rewritten copy — which could delete the checkout above, or skip straight to +# posting `review-verdict/h10=success` for its head sha. `PROTECTED` did not help (the rewrite +# defines `PROTECTED` too) and neither did branch protection, which requires the *context* and +# carries `required_approvals: 0`, so a self-posted success satisfied it outright. +# +# Measured on this instance (Gitea 1.25.4) rather than inferred from GitHub, because the whole point +# is that the gate's authority is derived, not asserted. A scratch PR rewriting this file to post a +# distinct probe context posted exactly that context, and the real `review-verdict/h10` was never +# written at all — the base's definition never ran. Under `pull_request_target` the same rewrite was +# ignored: the BASE definition ran and posted `h10=pending`, on both `opened` and `synchronize`, +# with `secrets` still available. +# +# `pull_request_target` is normally the DANGEROUS trigger, and it is worth being explicit about why +# that reputation does not transfer here. Its footgun is running untrusted HEAD code with a +# privileged token. This job never checks out the head and never executes anything the PR supplies: +# it checks out `base.sha` and runs only scripts from that tree. The base-ref checkout is what makes +# this trigger safe, so the two must be read as one decision — reintroducing a head checkout under +# this trigger would be far worse than the bug being fixed here. +# +# `branches: [main]` IS LOAD-BEARING, not cosmetic. Base resolution means the BASE branch supplies +# the definition, so without this filter a PR opened into an attacker-pushed base branch would run +# THAT branch's rewritten gate — trading a head-supplied definition for a base-supplied one and +# closing nothing. It matters more than it looks because a commit status is repo-global per sha +# (#663): a `success` forged on a head sha under a scratch base is inherited by a later, real PR +# into `main` carrying the same head. With the filter, a PR whose base is not `main` produces no run +# and no status at all (verified the same way). on: - pull_request: + pull_request_target: + branches: [main] types: [opened, reopened, synchronize, ready_for_review] defaults: diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 71a8d8c2a..ce4f9038a 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -786,7 +786,7 @@ carries no base of its own, so a merge driven through the Gitea UI or API is una comparator is the base *branch*, never its tip sha — a base that merely advances is ordinary churn, and comparing tips would invalidate every open verdict on every unrelated merge to `main`. -**Exemptions** are handled by `review-verdict.yml` on every `pull_request` event, which posts the +**Exemptions** are handled by `review-verdict.yml` on every `pull_request_target` event, which posts the status as `success` for **Renovate-authored** PRs (it uses `platformAutomerge: true`, so a required verdict with no exemption would stall every dependency bump) and for **docs-only** PRs, and as `pending` for everything else so the block has a visible reason. Both exemptions are **void when the @@ -800,9 +800,20 @@ used by the advisory hook `.claude/hooks/pretooluse-merge-consent.sh` (ersatztv# reads that script's **exit status** — a non-zero exit means "could not tell" and withholds the exemption; its stdout is meaningless on any failure path and is never consumed. -Two properties of that call are security-relevant and are pinned by tests in +Three properties of this workflow are security-relevant and are pinned by tests in `scripts/tests/test_pr_changed_files.py`: +- **The trigger is `pull_request_target`, scoped to `branches: [main]`** — never plain + `pull_request` (ersatztv#672). Gitea resolves a `pull_request` workflow *definition* from the PR's + own head, so under that trigger a PR editing `review-verdict.yml` ran its own rewritten copy and + could post `review-verdict/h10=success` for itself. The base-ref checkout below binds the scripts + this job runs; only the trigger binds the definition. The `branches` filter is half the fix, not a + refinement of it: base resolution means the *base branch* supplies the gate, so an unfiltered + trigger merely moves the rewrite to an attacker-pushed base — and a status forged there is + inherited by any later PR carrying the same head sha (ersatztv#663). `pull_request_target` is safe + here **only** because this job never checks out or executes head-supplied code. Verified on this + instance with three scratch PRs rather than inferred from GitHub; full rationale in + `docs/decisions/records/ci/gate-trigger-base-resolved.md`. - **The checkout takes the PR's BASE ref**, `ref: ${{ github.event.pull_request.base.sha }}` with `persist-credentials: false` — never the head. This job judges the PR, so the PR must not supply the code that judges it; a head checkout would let a PR edit the enumeration to return an empty diff --git a/docs/decisions/README.md b/docs/decisions/README.md index ceab1df90..8b4385f24 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -45,6 +45,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.docs-only-skip-steps` | A docs-only change must still run every required job (`test`, `migrations`) so their commit-status contexts always report; each heavy job runs `scripts/ci-detect-docs-only.sh` first and gates its real STEPS on `if: steps.detect.outputs.docs_only != 'true'`, never `if:`-skips the whole job (an `if:`-skipped job reports `skipped`, not `success`, which branch protection may never unblock on). Detection biases toward running more on any doubt. | 2026-07-17 | [link](records/ci/docs-only-skip-steps.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. | 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.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) | | `ci.jq-version-contract` | Every shell gate that shells out to `jq` is authored to the jq 1.6-compatible subset, because the CI runner ships jq 1.6 while every developer Mac ships 1.8.x. `scripts/jq-preflight.sh` (no args) prints the parsed version and asserts a floor of 1.6 in every gate job's log; `scripts/jq-preflight.sh --expect 1.6` additionally pins and fails loudly, but ONLY in the `script-tests` job. `review-verdict.yml` never pins — it writes the branch-protection-required `review-verdict/h10` status, so a hard pin there would turn any jq bump into a repo-wide merge deadlock. | 2026-07-26 | [link](records/ci/jq-version-contract.md) | diff --git a/docs/decisions/records/ci/gate-trigger-base-resolved.md b/docs/decisions/records/ci/gate-trigger-base-resolved.md new file mode 100644 index 000000000..71b3d0969 --- /dev/null +++ b/docs/decisions/records/ci/gate-trigger-base-resolved.md @@ -0,0 +1,62 @@ +--- +key: ci.gate-trigger-base-resolved +title: '2026-07-28 — `review-verdict.yml` triggers on `pull_request_target` scoped to `branches: [main]`, so the PR under judgment cannot supply the gate''s own definition (#672)' +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.' +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, 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]}`; 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' +--- + +`ci.shared-pr-file-enumeration` established that this job checks out the PR's **base** ref so the PR +cannot supply the *scripts* that judge it. That binding was real but partial, and the record said so: +it does not bind the job **definition**. This record closes the remaining half. + +**What was actually wrong.** Gitea, like GitHub, resolves a `pull_request` workflow definition from +the PR's own head commit. A PR editing `review-verdict.yml` therefore ran its own rewritten copy, +which could delete the base checkout or skip straight to posting `review-verdict/h10=success` for its +head sha. Two things that look like they should have prevented it did not: `PROTECTED` is defined by +the same rewritten file, and branch protection requires the *context*, not an author, while carrying +`required_approvals: 0`. + +**Measured, not inferred.** The premise is a claim about someone else's software, so it was settled +on this instance (Gitea 1.25.4) with three scratch PRs rather than by analogy to GitHub. A PR +rewriting the workflow to post a distinct probe context posted exactly that context, and the real +`review-verdict/h10` was never written — the base's definition never ran at all. Under +`pull_request_target` the identical rewrite was ignored: the base definition ran and posted +`h10=pending`, on both `opened` and `synchronize`, with `secrets` still available. With +`branches: [main]` added, a PR into a non-`main` base produced no run and no status whatsoever. The +probes posted only probe-named contexts, never a forged `h10`. + +**Why `branches: [main]` is load-bearing rather than tidy.** Base resolution means the *base branch* +supplies the definition. Dropping the filter would trade a head-supplied gate for a base-supplied one +and close nothing, because anyone who can push a branch can make that branch a base. It is worse than +a lateral move: a commit status is repo-global per sha (`#663`), so a `success` forged against a +scratch base is **inherited** by a later, genuine PR into `main` carrying the same head. The filter is +what keeps "the base" and "`main`" the same thing. + +**Why `pull_request_target` is not the footgun it usually is.** Its standard danger is executing +untrusted head code with a privileged token. This job executes none: it checks out `base.sha` with +`persist-credentials: false` and runs only scripts from that tree. The trigger and the base-ref +checkout are therefore one decision and must be read together — under this trigger, a head checkout +would hand a privileged token to PR-supplied code, which is strictly worse than #672 was. + +**Options not taken.** `required_approvals: 1` was the cheapest mechanical fix and is unusable here: +Gitea forbids approving your own PR, and this is effectively a single-maintainer repo, so it would +deadlock every PR rather than gate the dangerous ones. Verifying the *author* of the status needs an +actor the PR cannot control, and the tampered workflow holds the same `GITEA_TOKEN`; it buys nothing +without a second identity. "Accept and document" was the honest baseline — write access is already +trust — but the whole point of the H10 mechanism is that merge-consent is *derived from state* rather +than assertable, and this was a way to assert it. + +**Severity, stated plainly.** This was never remotely exploitable: pushing a branch here requires +write access, so the threat model is a compromised contributor, who has other paths. It is fixed +because a gate whose authority can be asserted by the thing it judges is not a gate, not because an +attack was expected. + +**Residual.** The job's own status context is renamed to `... (pull_request_target)` by the switch. +That is safe only because it was never one of branch protection's required contexts, which remain the +two `docker-build.yml` job contexts plus `review-verdict/h10`; adding it would let the workflow +satisfy the gate merely by running. The per-sha inheritance property behind `#663` is untouched here. diff --git a/docs/decisions/records/ci/shared-pr-file-enumeration.md b/docs/decisions/records/ci/shared-pr-file-enumeration.md index 196627dcc..db4dfc91e 100644 --- a/docs/decisions/records/ci/shared-pr-file-enumeration.md +++ b/docs/decisions/records/ci/shared-pr-file-enumeration.md @@ -89,8 +89,15 @@ runs its own rewritten copy — which can delete this checkout, or simply post requires the *context*, not an author, and carries `required_approvals: 0`. An earlier revision of this paragraph said the workflow "cannot be rewritten by that same PR to weaken its own judgment", which is true of the scripts and false of the workflow — and stated in the one sentence a reader -resolving this record from the catalog is most likely to stop at. Contained today only by push -access being required; tracked in #672. +resolving this record from the catalog is most likely to stop at. + +**That half is now closed, elsewhere — see `ci.gate-trigger-base-resolved` (#672).** The workflow +triggers on `pull_request_target` scoped to `branches: [main]`, so Gitea resolves its definition from +the base rather than the head. The paragraph above is kept in the past tense rather than deleted +because it names the distinction this record turns on: the base-ref checkout binds the *scripts*, and +only the trigger binds the *definition*. Note the dependency runs the other way too — that checkout is +what makes `pull_request_target` safe to use at all here, since this job never executes head-supplied +code. An earlier revision of this record stated the requirement in the future tense, because the wiring was staged over two PRs: the workflow runs the BASE version of the gate, and until the shared script @@ -100,7 +107,7 @@ through the combined status. That staging is complete. Both halves are asserted matching it — `head.sha` for `base.sha` is a nine-character diff, and a text-level check would still pass if a second checkout step took the head afterwards and won. -**What is deliberately NOT claimed.** Beyond the head-resolution hole above, the `PROTECTED` +**What is deliberately NOT claimed.** The `PROTECTED` path list remains the guard that stops a bot-authored PR from editing the gate and exempting itself, and mutation testing was what established that `PROTECTED` is load-bearing only on the BOT path — it and `DOCS_ONLY` are disjoint patterns, so on the docs-only path that clause can never fire. A diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 526e505e2..aff2204a3 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -489,6 +489,61 @@ def _classify_step(): raise AssertionError("no step in review-verdict.yml posts review-verdict/h10") +def _workflow_triggers(): + """The `on:` block, tolerating YAML 1.1's `on` -> True coercion. + + `yaml.safe_load` parses the bare key `on` as the BOOLEAN True, not the string "on" — so the + obvious `wf["on"]` raises KeyError against a perfectly valid workflow. Looking up both is not + defensive padding: a test that died on a KeyError here would read as "the trigger assertion is + broken" rather than "the trigger changed", which is the wrong failure to hand a maintainer. + """ + import yaml + wf = yaml.safe_load(WORKFLOW.read_text()) + on = wf.get("on", wf.get(True)) + assert isinstance(on, dict), f"review-verdict.yml has no parseable `on:` mapping (got {on!r})" + return on + + +def test_the_workflow_trigger_is_pull_request_TARGET_scoped_to_main(): + """The definition-rewrite hole (ersatztv#672) — sibling of the base-ref checkout below. + + That checkout binds the SCRIPTS this job runs to the base. It cannot bind the job DEFINITION: + Gitea resolves a `pull_request` workflow definition from the PR's own head, so a PR editing + review-verdict.yml ran its own rewritten copy and could post `review-verdict/h10=success` for + itself. Branch protection does not care who posted the context, and carries + `required_approvals: 0`. + + Both halves are asserted because either alone closes nothing: + + - `pull_request_target` resolves the definition from the base. + - `branches: [main]` keeps "the base" from being an attacker-pushed branch. Base resolution + without it merely moves the rewrite from the head to a scratch base — and since a commit + status is repo-global per sha (ersatztv#663), a success forged there is inherited by a later + real PR into `main` with the same head. + + Parsed, not substring-matched, for the reason the checkout test gives — and here the text-level + version is actively broken rather than merely weak: `pull_request` is a PREFIX of + `pull_request_target`, so `"pull_request" in text` cannot tell the safe trigger from the + vulnerable one, and `"pull_request_target" in text` stays green when a plain `pull_request` + trigger is ADDED back alongside it. + """ + on = _workflow_triggers() + assert "pull_request_target" in on, ( + "review-verdict.yml must trigger on `pull_request_target`; under plain `pull_request` " + "Gitea takes the workflow DEFINITION from the PR head, so a PR can rewrite the gate that " + "judges it (ersatztv#672)") + assert "pull_request" not in on, ( + "review-verdict.yml still carries a plain `pull_request` trigger — that runs the " + "head-supplied definition, which is exactly the ersatztv#672 hole, and it would run " + "ALONGSIDE the safe one rather than replacing it") + + branches = (on["pull_request_target"] or {}).get("branches") + assert branches == ["main"], ( + f"`pull_request_target.branches` is {branches!r}; it must be exactly ['main']. Base " + "resolution means the BASE branch supplies the gate, so an unfiltered trigger lets a PR " + "into an attacker-pushed base run that branch's rewritten copy (ersatztv#672)") + + def test_the_workflow_checks_out_the_BASE_ref_never_the_head(): """The self-rewrite hole. This workflow judges the PR, so the PR must not supply the code that judges it: checking out `head.sha` would let a PR rewrite `scripts/pr-changed-files.sh` to -- 2.47.3 From 8b73234d78a27fe1ba4a5e6d171fbef834cbbab1 Mon Sep 17 00:00:00 2001 From: Timothy Date: Tue, 28 Jul 2026 21:41:26 +0200 Subject: [PATCH 2/6] docs(672): record that the fix closes the route, not the class (#697) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Probing rather than reasoning turned up a second instance of the same vulnerability class while this fix was in review. `docker-build.yml` also triggers on `pull_request`, so it is head-resolved too, and it carries `ETV_STATUS_AUTH` (`REGISTRY_USER:REGISTRY_PASSWORD`) for the #420 revalidation read. Basic auth is not scoped: an account that can read commit statuses can write them. Confirmed with a scratch PR that POSTed a probe-named context using those credentials and succeeded — so a PR rewriting `docker-build.yml` can still post `review-verdict/h10=success` for its own head. That workflow cannot take the same fix. It builds and tests the PR's code, so it must resolve from the head; `pull_request_target` there would be the real footgun. It needs a read-only status identity instead. Filed as #697. The `rule:` field carries the limitation, not just the body, because the predecessor record's documented failure was exactly a reassuring sentence in the position a catalog reader stops at. Also trims the record to the 60-line prose ceiling. Adding it at 62 lines pushed p90 past the ceiling and reddened the blocking `script-tests` job — which is ersatztv#688 reproducing live, not a defect in this change. Refs: #672 Refs: #697 Decisions-Edit: yes --- docs/ci-cd.md | 6 +++- docs/decisions/README.md | 2 +- .../records/ci/gate-trigger-base-resolved.md | 30 ++++++++++++------- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index ce4f9038a..e3ace06b8 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -813,7 +813,11 @@ Three properties of this workflow are security-relevant and are pinned by tests inherited by any later PR carrying the same head sha (ersatztv#663). `pull_request_target` is safe here **only** because this job never checks out or executes head-supplied code. Verified on this instance with three scratch PRs rather than inferred from GitHub; full rationale in - `docs/decisions/records/ci/gate-trigger-base-resolved.md`. + `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). - **The checkout takes the PR's BASE ref**, `ref: ${{ github.event.pull_request.base.sha }}` with `persist-credentials: false` — never the head. This job judges the PR, so the PR must not supply the code that judges it; a head checkout would let a PR edit the enumeration to return an empty diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 8b4385f24..ca21c92b2 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -45,7 +45,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.docs-only-skip-steps` | A docs-only change must still run every required job (`test`, `migrations`) so their commit-status contexts always report; each heavy job runs `scripts/ci-detect-docs-only.sh` first and gates its real STEPS on `if: steps.detect.outputs.docs_only != 'true'`, never `if:`-skips the whole job (an `if:`-skipped job reports `skipped`, not `success`, which branch protection may never unblock on). Detection biases toward running more on any doubt. | 2026-07-17 | [link](records/ci/docs-only-skip-steps.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. | 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: any head-resolved workflow holding credentials that can POST a commit status can still forge `review-verdict/h10`, and `docker-build.yml` demonstrably can (`ETV_STATUS_AUTH`, confirmed by probe, tracked in #697). It must stay on `pull_request` because it builds the PR's code, so it needs a different fix — a read-only status identity, not a trigger change. | 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.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) | | `ci.jq-version-contract` | Every shell gate that shells out to `jq` is authored to the jq 1.6-compatible subset, because the CI runner ships jq 1.6 while every developer Mac ships 1.8.x. `scripts/jq-preflight.sh` (no args) prints the parsed version and asserts a floor of 1.6 in every gate job's log; `scripts/jq-preflight.sh --expect 1.6` additionally pins and fails loudly, but ONLY in the `script-tests` job. `review-verdict.yml` never pins — it writes the branch-protection-required `review-verdict/h10` status, so a hard pin there would turn any jq bump into a repo-wide merge deadlock. | 2026-07-26 | [link](records/ci/jq-version-contract.md) | diff --git a/docs/decisions/records/ci/gate-trigger-base-resolved.md b/docs/decisions/records/ci/gate-trigger-base-resolved.md index 71b3d0969..4c06a2f1b 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.' +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: any head-resolved workflow holding credentials that can POST a commit status can still forge `review-verdict/h10`, and `docker-build.yml` demonstrably can (`ETV_STATUS_AUTH`, confirmed by probe, tracked in #697). It must stay on `pull_request` because it builds the PR''s code, so it needs a different fix — a read-only status identity, not a trigger change.' 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, 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]}`; 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' --- @@ -44,17 +44,25 @@ checkout are therefore one decision and must be read together — under this tri would hand a privileged token to PR-supplied code, which is strictly worse than #672 was. **Options not taken.** `required_approvals: 1` was the cheapest mechanical fix and is unusable here: -Gitea forbids approving your own PR, and this is effectively a single-maintainer repo, so it would -deadlock every PR rather than gate the dangerous ones. Verifying the *author* of the status needs an -actor the PR cannot control, and the tampered workflow holds the same `GITEA_TOKEN`; it buys nothing -without a second identity. "Accept and document" was the honest baseline — write access is already -trust — but the whole point of the H10 mechanism is that merge-consent is *derived from state* rather -than assertable, and this was a way to assert it. +Gitea forbids approving your own PR and this is effectively a single-maintainer repo, so it would +deadlock every PR rather than gate the dangerous ones. Verifying the status *author* needs an actor +the PR cannot control, and the tampered workflow holds the same `GITEA_TOKEN`. -**Severity, stated plainly.** This was never remotely exploitable: pushing a branch here requires -write access, so the threat model is a compromised contributor, who has other paths. It is fixed -because a gate whose authority can be asserted by the thing it judges is not a gate, not because an -attack was expected. +**Severity, stated plainly.** Never remotely exploitable: pushing a branch requires write access, so +the threat model is a compromised contributor, who has other paths. Fixed because a gate whose +authority can be asserted by the thing it judges is not a gate — not because an attack was expected. + +**The class is NOT closed, and this record must not be read as claiming otherwise.** The bug fixed +here is one instance of "a head-resolved workflow holds credentials that can POST a commit status." +`docker-build.yml` is another: it triggers on `pull_request`, so it is head-resolved, and it carries +`ETV_STATUS_AUTH` (`REGISTRY_USER:REGISTRY_PASSWORD`) for the #420 revalidation read. Basic auth is +not scoped, so an account that can read statuses can write them — confirmed by the same probe method, +with a probe-named context, not asserted. A PR rewriting `docker-build.yml` can therefore still post +`review-verdict/h10=success` for its own head. That workflow **must** stay head-resolved, because it +builds and tests the PR's code; `pull_request_target` there would be the genuine footgun. It needs a +read-only status identity instead, tracked in `#697`. Stated here rather than left implicit because +the predecessor record's specific documented failure was a reassuring sentence in the position a +catalog reader stops at. **Residual.** The job's own status context is renamed to `... (pull_request_target)` by the switch. That is safe only because it was never one of branch protection's required contexts, which remain the -- 2.47.3 From 35a8ea8aefc1066e92d6b534e6ac360cb359d438 Mon Sep 17 00:00:00 2001 From: Timothy Date: Tue, 28 Jul 2026 21:49:22 +0200 Subject: [PATCH 3/6] fix(672): review round 1 -- pin the trigger set exactly, sweep the stale claims Cold review found the first cut of the test satisfiable by a still-vulnerable config, and two prose claims that outran the evidence. The test asserted "pull_request_target present, pull_request absent". Adding `workflow_dispatch:` or `push:` ALONGSIDE it kept that green, and both are ref-resolved with secrets, so either one restores an equivalent self-supplied-definition path. Enumerating those two would have the same hole one trigger later, so the assertion now pins the whole set: exactly {pull_request_target}, nothing else. Verified by mutation -- adding `workflow_dispatch` now reds. Adds the guard that would have caught the residual below rather than only the instance: no workflow OTHER than review-verdict.yml may reference `review-verdict/h10` in executable lines. Scoped honestly in its docstring as a drift guard, not a security boundary -- a workflow can still write the status through an indirection a text scan cannot see. `release.verdict-status-check` item 4 still asserted, in the present tense, that a PR editing review-verdict.yml is judged by its own edited copy. That is now false for this workflow, and it is the record a reader resolving the gate from the catalog actually lands on. Past-tensed, with the surviving residual named. The probe count said three. There were four; the omitted one is the only one with a negative result, which is what turns an honest partial into an overclaim. Corrected in both the record and ci-cd.md, along with what was NOT measured (`reopened`/`ready_for_review` firing under the new trigger). Also records two operational consequences a maintainer will otherwise hit cold: retargeting a PR onto `main` leaves it statusless until its next push (`edited` is not in `types:`), and the required contexts carry a literal `(pull_request)` suffix, so repeating this fix on docker-build.yml would rename them and deadlock merges unless branch protection is edited in the same operation. Trimmed the record back under the 60-line prose ceiling -- for the second time this session, which is ersatztv#688 reproducing, not a defect here. Refs: #672 Refs: #697 Decisions-Edit: yes --- docs/ci-cd.md | 7 +- .../records/ci/gate-trigger-base-resolved.md | 72 ++++++++++--------- .../records/release/verdict-status-check.md | 24 ++++--- scripts/tests/test_pr_changed_files.py | 47 +++++++++--- 4 files changed, 96 insertions(+), 54 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index e3ace06b8..4e2efa755 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -812,12 +812,15 @@ Three properties of this workflow are security-relevant and are pinned by tests trigger merely moves the rewrite to an attacker-pushed base — and a status forged there is inherited by any later PR carrying the same head sha (ersatztv#663). `pull_request_target` is safe here **only** because this job never checks out or executes head-supplied code. Verified on this - instance with three scratch PRs rather than inferred from GitHub; full rationale in + 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). + (ersatztv#697). One operational consequence: a PR whose base is not `main` now gets **no** + `review-verdict/h10` at all. That is fail-closed, but `edited` is not among the trigger's `types`, + so a PR opened against another base and later **retargeted** onto `main` stays statusless until its + next push — if a retargeted PR looks stalled with the check simply missing, that is why. - **The checkout takes the PR's BASE ref**, `ref: ${{ github.event.pull_request.base.sha }}` with `persist-credentials: false` — never the head. This job judges the PR, so the PR must not supply the code that judges it; a head checkout would let a PR edit the enumeration to return an empty diff --git a/docs/decisions/records/ci/gate-trigger-base-resolved.md b/docs/decisions/records/ci/gate-trigger-base-resolved.md index 4c06a2f1b..d9ceb2df6 100644 --- a/docs/decisions/records/ci/gate-trigger-base-resolved.md +++ b/docs/decisions/records/ci/gate-trigger-base-resolved.md @@ -11,37 +11,37 @@ mechanics: '`on: pull_request_target: {branches: [main], types: [opened, reopene --- `ci.shared-pr-file-enumeration` established that this job checks out the PR's **base** ref so the PR -cannot supply the *scripts* that judge it. That binding was real but partial, and the record said so: -it does not bind the job **definition**. This record closes the remaining half. +cannot supply the *scripts* that judge it — a real but partial binding, as that record said: it does +not bind the job **definition**. This closes the remaining half. **What was actually wrong.** Gitea, like GitHub, resolves a `pull_request` workflow definition from -the PR's own head commit. A PR editing `review-verdict.yml` therefore ran its own rewritten copy, -which could delete the base checkout or skip straight to posting `review-verdict/h10=success` for its -head sha. Two things that look like they should have prevented it did not: `PROTECTED` is defined by -the same rewritten file, and branch protection requires the *context*, not an author, while carrying -`required_approvals: 0`. +the PR's own head, so a PR editing `review-verdict.yml` ran its own rewritten copy — which could +delete the base checkout or skip straight to posting `review-verdict/h10=success` for its head sha. +Two things that look preventive were not: `PROTECTED` is defined by the same rewritten file, and +branch protection requires the *context*, not an author, while carrying `required_approvals: 0`. **Measured, not inferred.** The premise is a claim about someone else's software, so it was settled -on this instance (Gitea 1.25.4) with three scratch PRs rather than by analogy to GitHub. A PR -rewriting the workflow to post a distinct probe context posted exactly that context, and the real -`review-verdict/h10` was never written — the base's definition never ran at all. Under -`pull_request_target` the identical rewrite was ignored: the base definition ran and posted -`h10=pending`, on both `opened` and `synchronize`, with `secrets` still available. With -`branches: [main]` added, a PR into a non-`main` base produced no run and no status whatsoever. The -probes posted only probe-named contexts, never a forged `h10`. +on this instance (Gitea 1.25.4) with **four** scratch PRs, not by analogy to GitHub. A PR rewriting +the workflow posted its own probe context and the real `review-verdict/h10` was never written — the +base definition never ran. Under `pull_request_target` the identical rewrite was ignored: the base +definition ran and posted `h10=pending`, on `opened` and `synchronize` alike, with `secrets` still +available. With `branches: [main]`, a PR into a non-`main` base produced no run and no status at all. +The fourth probe is the negative one that established the residual below; it is counted because +omitting it is what turns an honest partial into an overclaim. The probes posted only probe-named +contexts, never a forged `h10`. Not measured: whether `reopened`/`ready_for_review` fire here. **Why `branches: [main]` is load-bearing rather than tidy.** Base resolution means the *base branch* -supplies the definition. Dropping the filter would trade a head-supplied gate for a base-supplied one -and close nothing, because anyone who can push a branch can make that branch a base. It is worse than -a lateral move: a commit status is repo-global per sha (`#663`), so a `success` forged against a -scratch base is **inherited** by a later, genuine PR into `main` carrying the same head. The filter is -what keeps "the base" and "`main`" the same thing. +supplies the definition, and anyone who can push a branch can make it a base — so dropping the filter +trades a head-supplied gate for a base-supplied one and closes nothing. Worse than lateral: a commit +status is repo-global per sha (`#663`), so a `success` forged against a scratch base is **inherited** +by a later genuine PR into `main` with the same head. The filter keeps "the base" and "`main`" the +same thing. **Why `pull_request_target` is not the footgun it usually is.** Its standard danger is executing untrusted head code with a privileged token. This job executes none: it checks out `base.sha` with -`persist-credentials: false` and runs only scripts from that tree. The trigger and the base-ref -checkout are therefore one decision and must be read together — under this trigger, a head checkout -would hand a privileged token to PR-supplied code, which is strictly worse than #672 was. +`persist-credentials: false` and runs only that tree's scripts. Trigger and checkout are therefore +one decision — under this trigger a head checkout would hand a privileged token to PR-supplied code, +strictly worse than #672 was. **Options not taken.** `required_approvals: 1` was the cheapest mechanical fix and is unusable here: Gitea forbids approving your own PR and this is effectively a single-maintainer repo, so it would @@ -54,17 +54,19 @@ authority can be asserted by the thing it judges is not a gate — not because a **The class is NOT closed, and this record must not be read as claiming otherwise.** The bug fixed here is one instance of "a head-resolved workflow holds credentials that can POST a commit status." -`docker-build.yml` is another: it triggers on `pull_request`, so it is head-resolved, and it carries -`ETV_STATUS_AUTH` (`REGISTRY_USER:REGISTRY_PASSWORD`) for the #420 revalidation read. Basic auth is -not scoped, so an account that can read statuses can write them — confirmed by the same probe method, -with a probe-named context, not asserted. A PR rewriting `docker-build.yml` can therefore still post -`review-verdict/h10=success` for its own head. That workflow **must** stay head-resolved, because it -builds and tests the PR's code; `pull_request_target` there would be the genuine footgun. It needs a -read-only status identity instead, tracked in `#697`. Stated here rather than left implicit because -the predecessor record's specific documented failure was a reassuring sentence in the position a -catalog reader stops at. +`docker-build.yml` is another: head-resolved (`on: pull_request`), carrying `ETV_STATUS_AUTH` +(`REGISTRY_USER:REGISTRY_PASSWORD`) for the #420 revalidation read. Basic auth is not scoped, so an +account that can read statuses can write them — confirmed by probe, not asserted. A PR rewriting it +can still post `review-verdict/h10=success` for its own head, and it **must** stay head-resolved +because it builds the PR's code, so it needs a read-only status identity instead (`#697`). Stated +here rather than implied because the predecessor record's documented failure was a reassuring +sentence in exactly the position a catalog reader stops at. -**Residual.** The job's own status context is renamed to `... (pull_request_target)` by the switch. -That is safe only because it was never one of branch protection's required contexts, which remain the -two `docker-build.yml` job contexts plus `review-verdict/h10`; adding it would let the workflow -satisfy the gate merely by running. The per-sha inheritance property behind `#663` is untouched here. +**Residual.** The job's own context is renamed to `... (pull_request_target)`, safe only because it +was never one of branch protection's required contexts (the two `docker-build.yml` contexts plus +`review-verdict/h10`); adding it would let the workflow satisfy the gate by merely running. **A trap +for #697:** those two carry the literal suffix `(pull_request)`, so giving `docker-build.yml` the +same treatment renames them and deadlocks every merge unless branch protection is edited in the same +operation. A PR based off a non-`main` branch now gets no status where it previously got one — +fail-closed, and it removes a `#663` inheritance hazard — but `edited` is absent from `types:`, so a +PR retargeted onto `main` stays statusless until its next `synchronize`. diff --git a/docs/decisions/records/release/verdict-status-check.md b/docs/decisions/records/release/verdict-status-check.md index 9175d1f3b..9a15b3253 100644 --- a/docs/decisions/records/release/verdict-status-check.md +++ b/docs/decisions/records/release/verdict-status-check.md @@ -172,15 +172,21 @@ described as one: gate earlier in the hook. That carve-out does not auto-grant — it passes through to an ordinary permission prompt — so the exposure is a missing warning on a merge a human is already confirming, not a silent merge. Worth knowing before reading "the hook denies on a retarget" as unconditional. -4. **A PR that edits `review-verdict.yml` is judged by its own edited copy.** Gitea runs - `pull_request` workflows from the PR **head**, not the base — confirmed on the very PR that - introduced this workflow (#630): `review-verdict.yml` does not exist on `main`, yet its job ran - and posted a status. So `PROTECTED` is a guardrail against *accidental* self-exemption, **not** a - tamper-proof control: a PR that rewrote the workflow would be classified by the rewritten rules. - Acceptable for a two-account repo (`timothy`, `renovate`) where the threat is a careless change - rather than a hostile one; it would not be for an untrusted-contributor repo, which would need - the classification moved somewhere the PR cannot edit (a base-branch-pinned workflow, or - server-side policy). +4. **A PR that edits `review-verdict.yml` WAS judged by its own edited copy — closed in #672, see + `ci.gate-trigger-base-resolved`.** Gitea runs `pull_request` workflows from the PR **head**, not + the base — confirmed on the very PR that introduced this workflow (#630): `review-verdict.yml` + does not exist on `main`, yet its job ran and posted a status. `PROTECTED` was therefore a + guardrail against *accidental* self-exemption, **not** a tamper-proof control: a PR that rewrote + the workflow would be classified by the rewritten rules. The workflow now triggers on + `pull_request_target` scoped to `branches: [main]`, so its definition is taken from the base. + + **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 + 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. The same head-execution behaviour is what makes the rollout self-hosting in the good case: #630's own run correctly identified it as touching `.claude/` and `scripts/`, refused both exemptions, diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index aff2204a3..bb23ba193 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -528,14 +528,16 @@ def test_the_workflow_trigger_is_pull_request_TARGET_scoped_to_main(): trigger is ADDED back alongside it. """ on = _workflow_triggers() - assert "pull_request_target" in on, ( - "review-verdict.yml must trigger on `pull_request_target`; under plain `pull_request` " - "Gitea takes the workflow DEFINITION from the PR head, so a PR can rewrite the gate that " - "judges it (ersatztv#672)") - assert "pull_request" not in on, ( - "review-verdict.yml still carries a plain `pull_request` trigger — that runs the " - "head-supplied definition, which is exactly the ersatztv#672 hole, and it would run " - "ALONGSIDE the safe one rather than replacing it") + # EXACT SET, not "target present and plain absent". Cold review found the weaker pair of + # assertions green after ADDING `workflow_dispatch:` or `push:` alongside the safe trigger — + # both are ref-resolved and both get secrets, so either one restores an equivalent + # self-supplied-definition path while the test reports clean. Enumerating the two known-bad + # extra triggers would have the same hole one trigger later; pinning the whole set does not. + assert set(on) == {"pull_request_target"}, ( + f"review-verdict.yml must trigger on `pull_request_target` and NOTHING else; got " + f"{sorted(map(str, on))}. Plain `pull_request` takes the workflow DEFINITION from the PR " + "head (ersatztv#672), and `push`/`workflow_dispatch` resolve it from an arbitrary ref — " + "any of them, ADDED ALONGSIDE rather than replacing, reopens the hole") branches = (on["pull_request_target"] or {}).get("branches") assert branches == ["main"], ( @@ -544,6 +546,35 @@ def test_the_workflow_trigger_is_pull_request_TARGET_scoped_to_main(): "into an attacker-pushed base run that branch's rewritten copy (ersatztv#672)") +def test_no_OTHER_workflow_writes_the_review_verdict_status(): + """`review-verdict.yml` is the only workflow allowed to write `review-verdict/h10`. + + 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. + + 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 + the status through an indirection this scan cannot see (a variable, a script it calls). Said + plainly so nobody reads a green here as "only one writer is possible". + """ + offenders = [] + for wf in sorted((REPO_ROOT / ".gitea" / "workflows").glob("*.yml")): + if wf.name == WORKFLOW.name: + continue + # CODE lines only — `docs`-style commentary in a workflow header legitimately discusses the + # context by name, and a raw-text scan would red on the prose explaining this very rule. + if "review-verdict/h10" in _code_lines(wf): + offenders.append(wf.name) + assert not offenders, ( + f"{offenders} reference `review-verdict/h10` in executable lines. Only review-verdict.yml " + "may write the gate's own status; another workflow doing so is a forgery path (see " + "ersatztv#697) or, at best, a second implementation of the gate that will drift") + + def test_the_workflow_checks_out_the_BASE_ref_never_the_head(): """The self-rewrite hole. This workflow judges the PR, so the PR must not supply the code that judges it: checking out `head.sha` would let a PR rewrite `scripts/pr-changed-files.sh` to -- 2.47.3 From b2a5c72bfe2a56d21b05699c318f478470bc83be Mon Sep 17 00:00:00 2001 From: Timothy Date: Tue, 28 Jul 2026 22:41:48 +0200 Subject: [PATCH 4/6] docs(672): widen the residual to the real inventory (#697, #698) Cross-family review established the residual is materially larger than the previous commit said, and that saying "docker-build.yml / ETV_STATUS_AUTH" understates it. Gitea injects `GITEA_TOKEN` into EVERY job automatically, defaulting to read/write. So the set of workflows that can POST `review-verdict/h10` is not a short list to audit -- it is all of them, plus `workflow_dispatch` (1.24+ loads the definition from the selected branch) and `push`-triggered ones. A collaborator's own write-scoped API token is a route with no workflow at all, because branch protection binds the required CONTEXT, not its issuer. Recorded in #697. The same review found three defects in the exemption path itself, none of them introduced here and none closed here: a retarget race that enumerates a docs-only diff against a scratch base while the enumerator revalidates only head.sha, a Renovate-PR hijack (the exemption reads the immutable PR creator, not who pushed the head), and an inherited `success` that short-circuits before any PR/base/author/file check. Filed together as #698, since they share one root cause -- the gate trusts state it cannot attribute. Also drops the claim that the three properties are "pinned by tests". The tests pin the workflow's SHAPE; no in-repository test can establish status-authority isolation, and the sibling-workflow guard added in the previous commit catches only a workflow that names the context in plain text. Trimmed to the 60-line prose ceiling for the third time in this session. That is ersatztv#688 -- the ceiling is now deleting rationale two reviewers asked for. Refs: #672 Refs: #697 Refs: #698 Decisions-Edit: yes --- docs/ci-cd.md | 10 +++-- docs/decisions/README.md | 2 +- .../records/ci/gate-trigger-base-resolved.md | 38 +++++++++---------- 3 files changed, 27 insertions(+), 23 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 4e2efa755..e5bbbaa10 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -800,8 +800,9 @@ used by the advisory hook `.claude/hooks/pretooluse-merge-consent.sh` (ersatztv# reads that script's **exit status** — a non-zero exit means "could not tell" and withholds the exemption; its stdout is meaningless on any failure path and is never consumed. -Three properties of this workflow are security-relevant and are pinned by tests in -`scripts/tests/test_pr_changed_files.py`: +Three properties of this workflow are security-relevant and are **structurally** asserted by tests in +`scripts/tests/test_pr_changed_files.py` — those tests pin the workflow's shape, which is not the same +as establishing that the gate cannot be forged (see the residual below, and ersatztv#697/#698): - **The trigger is `pull_request_target`, scoped to `branches: [main]`** — never plain `pull_request` (ersatztv#672). Gitea resolves a `pull_request` workflow *definition* from the PR's @@ -817,7 +818,10 @@ Three properties of this workflow are security-relevant and are pinned by tests 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). One operational consequence: a PR whose base is not `main` now gets **no** + (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 + consequence of the trigger change: a PR whose base is not `main` now gets **no** `review-verdict/h10` at all. That is fail-closed, but `edited` is not among the trigger's `types`, so a PR opened against another base and later **retargeted** onto `main` stays statusless until its next push — if a retargeted PR looks stalled with the check simply missing, that is why. diff --git a/docs/decisions/README.md b/docs/decisions/README.md index ca21c92b2..1a8da2425 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -45,7 +45,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.docs-only-skip-steps` | A docs-only change must still run every required job (`test`, `migrations`) so their commit-status contexts always report; each heavy job runs `scripts/ci-detect-docs-only.sh` first and gates its real STEPS on `if: steps.detect.outputs.docs_only != 'true'`, never `if:`-skips the whole job (an `if:`-skipped job reports `skipped`, not `success`, which branch protection may never unblock on). Detection biases toward running more on any doubt. | 2026-07-17 | [link](records/ci/docs-only-skip-steps.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: any head-resolved workflow holding credentials that can POST a commit status can still forge `review-verdict/h10`, and `docker-build.yml` demonstrably can (`ETV_STATUS_AUTH`, confirmed by probe, tracked in #697). It must stay on `pull_request` because it builds the PR's code, so it needs a different fix — a read-only status identity, not a trigger change. | 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`. 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.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.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) | | `ci.jq-version-contract` | Every shell gate that shells out to `jq` is authored to the jq 1.6-compatible subset, because the CI runner ships jq 1.6 while every developer Mac ships 1.8.x. `scripts/jq-preflight.sh` (no args) prints the parsed version and asserts a floor of 1.6 in every gate job's log; `scripts/jq-preflight.sh --expect 1.6` additionally pins and fails loudly, but ONLY in the `script-tests` job. `review-verdict.yml` never pins — it writes the branch-protection-required `review-verdict/h10` status, so a hard pin there would turn any jq bump into a repo-wide merge deadlock. | 2026-07-26 | [link](records/ci/jq-version-contract.md) | diff --git a/docs/decisions/records/ci/gate-trigger-base-resolved.md b/docs/decisions/records/ci/gate-trigger-base-resolved.md index d9ceb2df6..54fa8e5a1 100644 --- a/docs/decisions/records/ci/gate-trigger-base-resolved.md +++ b/docs/decisions/records/ci/gate-trigger-base-resolved.md @@ -5,14 +5,14 @@ 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: any head-resolved workflow holding credentials that can POST a commit status can still forge `review-verdict/h10`, and `docker-build.yml` demonstrably can (`ETV_STATUS_AUTH`, confirmed by probe, tracked in #697). It must stay on `pull_request` because it builds the PR''s code, so it needs a different fix — a read-only status identity, not a trigger change.' +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.' 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, 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]}`; 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' --- -`ci.shared-pr-file-enumeration` established that this job checks out the PR's **base** ref so the PR -cannot supply the *scripts* that judge it — a real but partial binding, as that record said: it does -not bind the job **definition**. This closes the remaining half. +`ci.shared-pr-file-enumeration` had this job check out the PR's **base** ref so the PR cannot supply +the *scripts* that judge it — real but partial, as that record said: it does not bind the job +**definition**. This closes that half. **What was actually wrong.** Gitea, like GitHub, resolves a `pull_request` workflow definition from the PR's own head, so a PR editing `review-verdict.yml` ran its own rewritten copy — which could @@ -38,29 +38,29 @@ by a later genuine PR into `main` with the same head. The filter keeps "the base same thing. **Why `pull_request_target` is not the footgun it usually is.** Its standard danger is executing -untrusted head code with a privileged token. This job executes none: it checks out `base.sha` with -`persist-credentials: false` and runs only that tree's scripts. Trigger and checkout are therefore -one decision — under this trigger a head checkout would hand a privileged token to PR-supplied code, -strictly worse than #672 was. +untrusted head code with a privileged token; this job executes none, checking out `base.sha` with +`persist-credentials: false` and running only that tree's scripts. Trigger and checkout are one +decision — under this trigger a head checkout would be strictly worse than #672 was. **Options not taken.** `required_approvals: 1` was the cheapest mechanical fix and is unusable here: Gitea forbids approving your own PR and this is effectively a single-maintainer repo, so it would deadlock every PR rather than gate the dangerous ones. Verifying the status *author* needs an actor the PR cannot control, and the tampered workflow holds the same `GITEA_TOKEN`. -**Severity, stated plainly.** Never remotely exploitable: pushing a branch requires write access, so +**Severity, stated plainly.** Never remotely exploitable — pushing a branch requires write access, so the threat model is a compromised contributor, who has other paths. Fixed because a gate whose -authority can be asserted by the thing it judges is not a gate — not because an attack was expected. +authority the judged thing can assert is not a gate, not because an attack was expected. -**The class is NOT closed, and this record must not be read as claiming otherwise.** The bug fixed -here is one instance of "a head-resolved workflow holds credentials that can POST a commit status." -`docker-build.yml` is another: head-resolved (`on: pull_request`), carrying `ETV_STATUS_AUTH` -(`REGISTRY_USER:REGISTRY_PASSWORD`) for the #420 revalidation read. Basic auth is not scoped, so an -account that can read statuses can write them — confirmed by probe, not asserted. A PR rewriting it -can still post `review-verdict/h10=success` for its own head, and it **must** stay head-resolved -because it builds the PR's code, so it needs a read-only status identity instead (`#697`). Stated -here rather than implied because the predecessor record's documented failure was a reassuring -sentence in exactly the position a catalog reader stops at. +**The class is NOT closed, and this record must not be read as claiming otherwise.** This fixed one +instance of "a ref-resolved workflow can obtain credentials that POST a commit status", and that +inventory is not a short list: Gitea injects `GITEA_TOKEN` into **every** job, defaulting to +read/**write**, so head-resolved, `push`-triggered and `workflow_dispatch` workflows alike are routes +(1.24+ loads a dispatched definition from the selected branch). A collaborator's own API token is a +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 +status-authority isolation: the sibling-workflow guard added here catches only plain-text naming of +the context. Spelled out because the predecessor record's documented failure was a reassuring +sentence exactly where a catalog reader stops. **Residual.** The job's own context is renamed to `... (pull_request_target)`, safe only because it was never one of branch protection's required contexts (the two `docker-build.yml` contexts plus -- 2.47.3 From 20b7171fbacca3a4db88753975df0642c68e71be Mon Sep 17 00:00:00 2001 From: Timothy Date: Tue, 28 Jul 2026 23:03:15 +0200 Subject: [PATCH 5/6] fix(672): review round 2 -- correct a stale rule: field, document the self-test gap Re-review of the fix commits returned MERGEABLE (nine trigger mutations all caught, every prior finding verified against independent sources) with four low-severity findings. All four are addressed here. F1: `release.verdict-status-check`'s `rule:` frontmatter still said "A `pull_request` workflow auto-passes the two exempt classes". Round 1 past-tensed that record's BODY and left its `rule:` stale -- which is the exact failure mode the previous commit cites as the reason to put limitations in `rule:` in the first place. The catalog row mirrors this field verbatim and it mirrors again per-`key:` into MemPalace, so a stale `rule:` propagates further than a stale paragraph. F2: same record, "is what makes the rollout self-hosting" -> past tense. It described #630 and now reads as a live property. F3, the one that matters operationally: base resolution cuts BOTH ways. A change to `review-verdict.yml` is no longer exercised by its own PR -- the PR runs the version already on `main` -- so an edit goes live only ON MERGE, repo-wide, having never run. A broken edit merges green and then breaks the gate for every subsequent PR, and the PR that would repair it is gated by the same broken workflow. The recipe for verifying one safely (scratch base + throwaway PR + probe-named context) now lives in docs/ci-cd.md, which is where an operator looks, rather than in the record. F4: the sibling-workflow guard globbed `*.yml`, so a workflow added as `.yaml` would be silently unscanned. Latent today, which is when it is cheap. The record lost its meta-justification paragraph to the 60-line prose ceiling. Fifth trim this session; the operational recipe moving to ci-cd.md is better placement anyway, but it was forced rather than chosen. ersatztv#688. Refs: #672 Decisions-Edit: yes --- docs/ci-cd.md | 16 ++++++ docs/decisions/README.md | 2 +- .../records/ci/gate-trigger-base-resolved.md | 56 +++++++++---------- .../records/release/verdict-status-check.md | 4 +- scripts/tests/test_pr_changed_files.py | 4 +- 5 files changed, 50 insertions(+), 32 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index e5bbbaa10..454ebfb2b 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -836,6 +836,22 @@ as establishing that the gate cannot be forged (see the residual below, and ersa A PR whose base predates ersatztv#658 has no such script on its base ref; that case posts `pending` with the reason rather than dying with no status at all. +⚠️ **Changing `review-verdict.yml` itself: it is not exercised by its own PR.** Base resolution cuts +both ways — the PR editing this workflow runs the version already on `main`, so an edit goes live +**only on merge**, repo-wide, having never run. A broken edit merges green and then breaks the gate +for every subsequent PR, and the PR that would repair it is gated by the same broken workflow. Do not +trust the editing PR's own checks. Verify the way ersatztv#672 did: + +1. Push a scratch **base** branch carrying the candidate workflow. +2. Open a throwaway PR from a scratch head *into that base*, so the candidate is the definition that + runs. Have it post a **probe-named** context (e.g. `review-verdict/h10-PROBE`), never the real + `review-verdict/h10` — a probe must not be able to forge the gate it is testing. +3. Read the resulting commit statuses to see which definition actually ran, then delete both + branches. + +The same shape is what makes a `branches:`/`types:` change verifiable at all, since neither can be +observed from the editing PR. + It lives in its **own workflow file** on purpose: `pr-checks.yml` sets `cancel-in-progress: true`, and a cancelled run there would leave an exempt PR with no status and no further push to re-trigger it. Its own job context (`Review verdict / Set review-verdict status`) is **not** the diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 1a8da2425..972090d81 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -122,7 +122,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `release.prepush-clean-worktree-guard` | A fail-open pre-push hook blocks a push when any file in the branch's diff vs `origin/main` also has uncommitted working-tree or index changes, since a stale-index commit (e.g. `git reset --soft` + `git add` over an edited-but-unstaged fix) can silently push, CI-test, and get reviewed a different tree than the one on disk. Scope is precise to pushed-diff files; escape hatch `ETV_ALLOW_DIRTY_PUSH=1`. | 2026-07-17 | [link](records/release/prepush-clean-worktree-guard.md) | | `release.promotion-floating-prod` | Prod tracks the floating `:prod` image reference; a tag build's immutable `:` image is scanned first, then promotion happens via a separate manual `DeployStack`, with daily auto-update only as a fallback — tag with enough runway before 03:00 to avoid an unscanned promotion. | 2026-07-13 | [link](records/release/promotion-floating-prod.md) | | `release.review-verdict-gate` | A PR may not merge until a `Review-verdict: @ ` comment references the PR's current head sha (short-sha prefix match against the verdict's OWN `@ ` field, marker at COLUMN 0 (no indent, so indented code blocks cannot self-approve), whole-word verdict token, fenced code blocks stripped with markdown fence-length semantics, negative wins over positive on the same head); folds into the H6 merge-consent hook as condition (c). The grammar lives in ONE tested place, `scripts/check-review-verdict.sh` — #629 found three false-opens that survived because it was implemented inline and untested while this record described stricter behaviour than the code had. | 2026-07-12 | [link](records/release/review-verdict-gate.md) | -| `release.verdict-status-check` | The H10 review verdict is written as a `review-verdict/h10` Gitea **commit status** on the exact reviewed sha by `scripts/post-review-verdict.sh`, and that context is a REQUIRED status check on `main`. Because a status belongs to one sha, a later commit cannot inherit it, so Gitea's own `merge_when_checks_succeed` refuses to merge a head no one reviewed. The PreToolUse hook additionally refuses to SCHEDULE an auto-merge unless that status is already green on head. A `pull_request` workflow auto-passes the two exempt classes (Renovate-authored, docs-only) unless the PR touches a protected path (`.claude/`, `.gitea/`, `.husky/`, `scripts/`, `docker/ci/`). This extends — does not supersede — `release.review-verdict-gate` (#303 H10), whose comment convention remains the human-readable artifact and the hook's condition (c). | 2026-07-25 | [link](records/release/verdict-status-check.md) | +| `release.verdict-status-check` | The H10 review verdict is written as a `review-verdict/h10` Gitea **commit status** on the exact reviewed sha by `scripts/post-review-verdict.sh`, and that context is a REQUIRED status check on `main`. Because a status belongs to one sha, a later commit cannot inherit it, so Gitea's own `merge_when_checks_succeed` refuses to merge a head no one reviewed. The PreToolUse hook additionally refuses to SCHEDULE an auto-merge unless that status is already green on head. A `pull_request_target` workflow auto-passes the two exempt classes (Renovate-authored, docs-only) unless the PR touches a protected path (`.claude/`, `.gitea/`, `.husky/`, `scripts/`, `docker/ci/`). This extends — does not supersede — `release.review-verdict-gate` (#303 H10), whose comment convention remains the human-readable artifact and the hook's condition (c). | 2026-07-25 | [link](records/release/verdict-status-check.md) | | `rulebuilder.relative-date-macros` | The visual rule builder's `inLast`/`notInLast` date operators compile to/parse from the pre-existing `CustomMultiFieldQueryParser` macros `released_inthelast`/`released_notinthelast` and `added_inthelast`/`added_notinthelast`, value form `" day\|week\|month\|year"`; there is no backend change. | 2026-07-23 | [link](records/rulebuilder/relative-date-macros.md) | | `scan.collections-scan-status` | `GET /api/v1/media-sources/collections-scan-status` reports a family-global (not per-source), boolean-only active-scan set read from `IEntityLocker`; the SPA reconciles authoritatively against it (with a grace-tick helper) instead of a fixed client-side timeout. | 2026-07-12 | [link](records/scan/collections-scan-status.md) | | `scan.getoraddfolder-db-lookup` | `ILibraryRepository.GetOrAddFolder` resolves the existing folder via a DB query on `(LibraryPathId, Path)`, not the caller's `LibraryPath.LibraryFolders` in-memory navigation, since that navigation is only eager-loaded on the local scan path and is null on remote (Jellyfin) callers. | 2026-07-20 | [link](records/scan/getoraddfolder-db-lookup.md) | diff --git a/docs/decisions/records/ci/gate-trigger-base-resolved.md b/docs/decisions/records/ci/gate-trigger-base-resolved.md index 54fa8e5a1..37f5085bb 100644 --- a/docs/decisions/records/ci/gate-trigger-base-resolved.md +++ b/docs/decisions/records/ci/gate-trigger-base-resolved.md @@ -21,31 +21,29 @@ Two things that look preventive were not: `PROTECTED` is defined by the same rew branch protection requires the *context*, not an author, while carrying `required_approvals: 0`. **Measured, not inferred.** The premise is a claim about someone else's software, so it was settled -on this instance (Gitea 1.25.4) with **four** scratch PRs, not by analogy to GitHub. A PR rewriting -the workflow posted its own probe context and the real `review-verdict/h10` was never written — the -base definition never ran. Under `pull_request_target` the identical rewrite was ignored: the base -definition ran and posted `h10=pending`, on `opened` and `synchronize` alike, with `secrets` still -available. With `branches: [main]`, a PR into a non-`main` base produced no run and no status at all. -The fourth probe is the negative one that established the residual below; it is counted because -omitting it is what turns an honest partial into an overclaim. The probes posted only probe-named -contexts, never a forged `h10`. Not measured: whether `reopened`/`ready_for_review` fire here. +on this instance (Gitea 1.25.4) with **four** scratch PRs, not by analogy to GitHub: `pull_request` ran +the head's rewrite and never wrote the real `h10`; `pull_request_target` ignored the identical rewrite +and the base definition posted `h10=pending` on `opened` and `synchronize` alike, secrets available; +`branches: [main]` produced no run at all from a non-`main` base; and the fourth — the negative one +that established the residual below — is counted because omitting it turns an honest partial into an +overclaim. Probes posted only probe-named contexts, never a forged `h10`. Full results in #699. Not +measured: whether `reopened`/`ready_for_review` fire here. -**Why `branches: [main]` is load-bearing rather than tidy.** Base resolution means the *base branch* -supplies the definition, and anyone who can push a branch can make it a base — so dropping the filter -trades a head-supplied gate for a base-supplied one and closes nothing. Worse than lateral: a commit -status is repo-global per sha (`#663`), so a `success` forged against a scratch base is **inherited** -by a later genuine PR into `main` with the same head. The filter keeps "the base" and "`main`" the -same thing. +**Why `branches: [main]` is load-bearing rather than tidy.** The *base branch* supplies the +definition, and anyone who can push a branch can make it a base — so dropping the filter trades a +head-supplied gate for a base-supplied one and closes nothing. Worse than lateral: a commit status is +repo-global per sha (`#663`), so a `success` forged against a scratch base is **inherited** by a later +genuine PR into `main` with the same head. **Why `pull_request_target` is not the footgun it usually is.** Its standard danger is executing untrusted head code with a privileged token; this job executes none, checking out `base.sha` with `persist-credentials: false` and running only that tree's scripts. Trigger and checkout are one decision — under this trigger a head checkout would be strictly worse than #672 was. -**Options not taken.** `required_approvals: 1` was the cheapest mechanical fix and is unusable here: -Gitea forbids approving your own PR and this is effectively a single-maintainer repo, so it would -deadlock every PR rather than gate the dangerous ones. Verifying the status *author* needs an actor -the PR cannot control, and the tampered workflow holds the same `GITEA_TOKEN`. +**Options not taken.** `required_approvals: 1`, the cheapest mechanical fix, is unusable here: Gitea +forbids approving your own PR and this is effectively a single-maintainer repo, so it deadlocks every +PR instead of gating the dangerous ones. Verifying the status *author* needs an actor the PR cannot +control, and the tampered workflow holds the same `GITEA_TOKEN`. **Severity, stated plainly.** Never remotely exploitable — pushing a branch requires write access, so the threat model is a compromised contributor, who has other paths. Fixed because a gate whose @@ -56,17 +54,19 @@ instance of "a ref-resolved workflow can obtain credentials that POST a commit s inventory is not a short list: Gitea injects `GITEA_TOKEN` into **every** job, defaulting to read/**write**, so head-resolved, `push`-triggered and `workflow_dispatch` workflows alike are routes (1.24+ loads a dispatched definition from the selected branch). A collaborator's own API token is a -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 -status-authority isolation: the sibling-workflow guard added here catches only plain-text naming of -the context. Spelled out because the predecessor record's documented failure was a reassuring -sentence exactly where a catalog reader stops. +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 +status-authority isolation: the sibling guard added here catches only plain-text naming of the +context. + +**The gate is no longer exercised by its own PR** — base resolution cuts both ways, so an edit here +goes live only on merge, repo-wide, untested. How to verify one safely is in `docs/ci-cd.md`. **Residual.** The job's own context is renamed to `... (pull_request_target)`, safe only because it was never one of branch protection's required contexts (the two `docker-build.yml` contexts plus `review-verdict/h10`); adding it would let the workflow satisfy the gate by merely running. **A trap -for #697:** those two carry the literal suffix `(pull_request)`, so giving `docker-build.yml` the -same treatment renames them and deadlocks every merge unless branch protection is edited in the same -operation. A PR based off a non-`main` branch now gets no status where it previously got one — -fail-closed, and it removes a `#663` inheritance hazard — but `edited` is absent from `types:`, so a -PR retargeted onto `main` stays statusless until its next `synchronize`. +for #697:** those two carry the literal `(pull_request)` suffix, so giving `docker-build.yml` the same +treatment renames them and deadlocks merges unless branch protection is edited in the same operation. +A non-`main` base now yields no status where it previously got one — fail-closed, and it removes a +`#663` hazard — but `edited` is absent from `types:`, so a PR retargeted onto `main` stays statusless +until its next `synchronize`. diff --git a/docs/decisions/records/release/verdict-status-check.md b/docs/decisions/records/release/verdict-status-check.md index 9a15b3253..c4941e402 100644 --- a/docs/decisions/records/release/verdict-status-check.md +++ b/docs/decisions/records/release/verdict-status-check.md @@ -5,7 +5,7 @@ status: active since: '2026-07-25' supersedes: none superseded-by: none -rule: 'The H10 review verdict is written as a `review-verdict/h10` Gitea **commit status** on the exact reviewed sha by `scripts/post-review-verdict.sh`, and that context is a REQUIRED status check on `main`. Because a status belongs to one sha, a later commit cannot inherit it, so Gitea''s own `merge_when_checks_succeed` refuses to merge a head no one reviewed. The PreToolUse hook additionally refuses to SCHEDULE an auto-merge unless that status is already green on head. A `pull_request` workflow auto-passes the two exempt classes (Renovate-authored, docs-only) unless the PR touches a protected path (`.claude/`, `.gitea/`, `.husky/`, `scripts/`, `docker/ci/`). This extends — does not supersede — `release.review-verdict-gate` (#303 H10), whose comment convention remains the human-readable artifact and the hook''s condition (c).' +rule: 'The H10 review verdict is written as a `review-verdict/h10` Gitea **commit status** on the exact reviewed sha by `scripts/post-review-verdict.sh`, and that context is a REQUIRED status check on `main`. Because a status belongs to one sha, a later commit cannot inherit it, so Gitea''s own `merge_when_checks_succeed` refuses to merge a head no one reviewed. The PreToolUse hook additionally refuses to SCHEDULE an auto-merge unless that status is already green on head. A `pull_request_target` workflow auto-passes the two exempt classes (Renovate-authored, docs-only) unless the PR touches a protected path (`.claude/`, `.gitea/`, `.husky/`, `scripts/`, `docker/ci/`). This extends — does not supersede — `release.review-verdict-gate` (#303 H10), whose comment convention remains the human-readable artifact and the hook''s condition (c).' signals: 'merge_when_checks_succeed freezes consent, auto-merge merges an unreviewed head, verdict bound to sha, review-verdict/h10 required check, post-review-verdict.sh, Renovate platformAutomerge exemption · paths: `scripts/post-review-verdict.sh`, `.gitea/workflows/review-verdict.yml`, `.claude/hooks/pretooluse-merge-consent.sh` · issues: #622, #303 (H6/H10), #242, #619' mechanics: '`scripts/tests/test_post_review_verdict.py` (incl. a TOCTOU head-moved case and cross-checks against the hook''s own condition-(c) regexes); branch protection `status_check_contexts` on `main`' --- @@ -188,6 +188,6 @@ described as one: An untrusted-contributor repo would still need the classification moved somewhere no PR can reach (server-side policy), not merely a base-pinned definition. - The same head-execution behaviour is what makes the rollout self-hosting in the good case: #630's + The same head-execution behaviour was what made the rollout self-hosting in the good case: #630's own run correctly identified it as touching `.claude/` and `scripts/`, refused both exemptions, and posted `review-verdict/h10=pending` with an actionable description. diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index bb23ba193..e48af2dcb 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -562,7 +562,9 @@ def test_no_OTHER_workflow_writes_the_review_verdict_status(): plainly so nobody reads a green here as "only one writer is possible". """ offenders = [] - for wf in sorted((REPO_ROOT / ".gitea" / "workflows").glob("*.yml")): + # `*.y*ml`, not `*.yml` — a workflow added as `.yaml` is just as executable and would otherwise + # be silently unscanned. Latent today (no `.yaml` exists), which is exactly when it is cheap. + for wf in sorted((REPO_ROOT / ".gitea" / "workflows").glob("*.y*ml")): if wf.name == WORKFLOW.name: continue # CODE lines only — `docs`-style commentary in a workflow header legitimately discusses the -- 2.47.3 From ceef16081daca54d01178a4a9b069041385607eb Mon Sep 17 00:00:00 2001 From: Timothy Date: Tue, 28 Jul 2026 23:17:14 +0200 Subject: [PATCH 6/6] docs(672): make the self-test gap discoverable (signals + section pointer) Re-review of the round-2 head returned MERGEABLE with one LOW: the record's `signals:` field did not mention the self-test gap. That field is the semantic discovery surface -- it is what the MemPalace mirror matches on -- so the most operationally dangerous property of this change ("a gate edit goes live only on merge, having never run") was unreachable by anyone searching for it. Someone asking "how do I test a change to review-verdict.yml" would have found nothing. Adds three signal phrases and points the record's one-line reference at a section rather than at a ~1050-line file. No behaviour change; frontmatter and prose only. Verified the frontmatter still parses under PyYAML rather than the validator's hand parser, per #674 -- an apostrophe in a single-quoted scalar is exactly what that hand parser cannot see. Refs: #672 Decisions-Edit: yes --- docs/decisions/records/ci/gate-trigger-base-resolved.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/decisions/records/ci/gate-trigger-base-resolved.md b/docs/decisions/records/ci/gate-trigger-base-resolved.md index 37f5085bb..e6298b13c 100644 --- a/docs/decisions/records/ci/gate-trigger-base-resolved.md +++ b/docs/decisions/records/ci/gate-trigger-base-resolved.md @@ -6,7 +6,7 @@ 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.' -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, 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' +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]}`; 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' --- @@ -60,7 +60,7 @@ status-authority isolation: the sibling guard added here catches only plain-text context. **The gate is no longer exercised by its own PR** — base resolution cuts both ways, so an edit here -goes live only on merge, repo-wide, untested. How to verify one safely is in `docs/ci-cd.md`. +goes live only on merge, repo-wide, untested. Verify one safely per `docs/ci-cd.md` → Review-verdict gate. **Residual.** The job's own context is renamed to `... (pull_request_target)`, safe only because it was never one of branch protection's required contexts (the two `docker-build.yml` contexts plus -- 2.47.3