fix(672): trigger the verdict gate on pull_request_target scoped to main #699
@@ -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:
|
||||
|
||||
+41
-3
@@ -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
|
||||
|
||||
@@ -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 <files>` 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=<name>` 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 `:<version>` 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: <MERGEABLE\|APPROVED\|BLOCKED\|NOT-MERGEABLE> @ <head-sha>` comment references the PR's current head sha (short-sha prefix match against the verdict's OWN `@ <sha>` 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 `"<n> 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) |
|
||||
|
||||
@@ -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`.
|
||||
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user