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..454ebfb2b 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,31 @@ 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 -`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 + 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 four scratch PRs rather than inferred from GitHub; full rationale in + `docs/decisions/records/ci/gate-trigger-base-resolved.md`. **This closes the rewrite route through + this workflow, not the class:** `docker-build.yml` is also head-resolved and its `ETV_STATUS_AUTH` + credentials can write statuses, so it can still forge `review-verdict/h10` — it must stay on + `pull_request` because it builds the PR's code, so it needs a read-only status identity instead + (ersatztv#697) — and the inventory is every workflow, not that one, because Gitea injects a + write-capable `GITEA_TOKEN` into every job and branch protection binds the *context*, not its + issuer. The exemption path has separate defects of its own (ersatztv#698). One operational + 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. - **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 @@ -814,6 +836,22 @@ Two properties of that call are security-relevant and are pinned by tests in 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 ceab1df90..972090d81 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. 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) | @@ -121,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 new file mode 100644 index 000000000..e6298b13c --- /dev/null +++ b/docs/decisions/records/ci/gate-trigger-base-resolved.md @@ -0,0 +1,72 @@ +--- +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. 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, 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' +--- + +`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 +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 **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.** 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`, 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 +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.** 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 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. 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 +`review-verdict/h10`); adding it would let the workflow satisfy the gate by merely running. **A trap +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/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/docs/decisions/records/release/verdict-status-check.md b/docs/decisions/records/release/verdict-status-check.md index 9175d1f3b..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`' --- @@ -172,16 +172,22 @@ 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. - The same head-execution behaviour is what makes the rollout self-hosting in the good case: #630's + **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 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 526e505e2..e48af2dcb 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -489,6 +489,94 @@ 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() + # 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"], ( + 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_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 = [] + # `*.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 + # 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