fix(672): trigger the verdict gate on pull_request_target scoped to main #699

Merged

timothy merged 6 commits from fix/672-review-verdict-head-resolution into main 2026-07-28 23:44:20 +02:00

Conversation 1 Commits 6 Files Changed 7

+269 -23

timothy commented 2026-07-28 22:42:35 +02:00

Owner

Copy Link

Copy Source

fixes #672

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 — so a PR editing this file ran its own rewritten copy and could post h10=success for itself. The #649 base-ref checkout binds the scripts this job runs; only the trigger binds the definition.

Measured, not inferred

Four throwaway scratch PRs on this instance (Gitea 1.25.4), because the premise is a claim about someone else's software. Every probe posted a probe-named context — never a forged h10.

Probe	Question	Result
#693	Is pull_request head-resolved?	Yes — head's rewrite ran; real h10 never written
#694	Is pull_request_target base-resolved?	Yes — base gate ran, posted h10=pending; rewrite ignored. Fires on opened and synchronize; secrets available
#695	Does branches: [main] block a non-main base?	Yes — no run, no statuses
#696	Can another head-resolved workflow's credentials write the status?	Yes — this is the residual, see below

branches: [main] is half the fix, not a refinement: base resolution means the base branch supplies the definition, so without it the rewrite just moves to an attacker-pushed base — and a status forged there is inherited by any later 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. Trigger and base-ref checkout are one decision.

What this does NOT close

Probe #696 found the same bypass through docker-build.yml, and review widened it further: Gitea injects a write-capable GITEA_TOKEN into every job, and branch protection binds the context, not its issuer. Filed as #697. Three further defects in the exemption path (retarget race, Renovate-PR hijack, unreclassified inherited success) — none introduced here — are filed as #698. The decision record carries this limitation in its rule: field, not buried in the body, because the predecessor record's documented failure was exactly a reassuring sentence where a catalog reader stops.

Tests

New test_the_workflow_trigger_is_pull_request_TARGET_scoped_to_main pins the exact trigger set (cold review found "target present, plain absent" stayed green when workflow_dispatch was added alongside). Plus a guard that no other workflow names review-verdict/h10 in executable lines — scoped honestly in its docstring as a drift guard, not a security boundary.

Mutations verified red: revert to pull_request; drop branches; re-add pull_request alongside; add workflow_dispatch; another workflow writing the context. scripts/tests 340 passed; decisions_validate OK.

Transitional note for this PR

This PR's head defines only pull_request_target while main still defines only pull_request, so neither trigger matches and this PR gets no auto-posted h10. That is fail-closed; the verdict is written directly by scripts/post-review-verdict.sh. PRs opened after this merges behave normally.

Reviews

Two independent cold reviews (same-family worktree-isolated + cross-family Codex). Both initially BLOCKED on doc overclaim and the residual; both called the code change a strict improvement that should land. Actionable findings fixed in the two follow-up commits; the rest filed as #697/#698.

Docs

docs/ci-cd.md, new docs/decisions/records/ci/gate-trigger-base-resolved.md, ci.shared-pr-file-enumeration and release.verdict-status-check past-tensed, catalog regenerated.

fixes #672 `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 — so a PR editing this file ran its own rewritten copy and could post `h10=success` for itself. The #649 base-ref checkout binds the *scripts* this job runs; only the trigger binds the *definition*. ## Measured, not inferred Four throwaway scratch PRs on this instance (Gitea 1.25.4), because the premise is a claim about someone else's software. Every probe posted a **probe-named** context — never a forged `h10`. | Probe | Question | Result | |---|---|---| | #693 | Is `pull_request` head-resolved? | **Yes** — head's rewrite ran; real `h10` never written | | #694 | Is `pull_request_target` base-resolved? | **Yes** — base gate ran, posted `h10=pending`; rewrite ignored. Fires on `opened` *and* `synchronize`; secrets available | | #695 | Does `branches: [main]` block a non-`main` base? | **Yes** — no run, no statuses | | #696 | Can another head-resolved workflow's credentials write the status? | **Yes** — this is the residual, see below | `branches: [main]` is half the fix, not a refinement: base resolution means the *base branch* supplies the definition, so without it the rewrite just moves to an attacker-pushed base — and a status forged there is inherited by any later 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. Trigger and base-ref checkout are one decision. ## What this does NOT close Probe #696 found the same bypass through `docker-build.yml`, and review widened it further: Gitea injects a write-capable `GITEA_TOKEN` into **every** job, and branch protection binds the *context*, not its issuer. Filed as **#697**. Three further defects in the exemption path (retarget race, Renovate-PR hijack, unreclassified inherited success) — none introduced here — are filed as **#698**. The decision record carries this limitation in its `rule:` field, not buried in the body, because the predecessor record's documented failure was exactly a reassuring sentence where a catalog reader stops. ## Tests New `test_the_workflow_trigger_is_pull_request_TARGET_scoped_to_main` pins the **exact** trigger set (cold review found "target present, plain absent" stayed green when `workflow_dispatch` was *added* alongside). Plus a guard that no other workflow names `review-verdict/h10` in executable lines — scoped honestly in its docstring as a drift guard, not a security boundary. Mutations verified red: revert to `pull_request`; drop `branches`; re-add `pull_request` alongside; add `workflow_dispatch`; another workflow writing the context. `scripts/tests` 340 passed; `decisions_validate` OK. ## Transitional note for this PR This PR's head defines only `pull_request_target` while `main` still defines only `pull_request`, so neither trigger matches and **this PR gets no auto-posted `h10`**. That is fail-closed; the verdict is written directly by `scripts/post-review-verdict.sh`. PRs opened after this merges behave normally. ## Reviews Two independent cold reviews (same-family worktree-isolated + cross-family Codex). Both initially BLOCKED on doc overclaim and the residual; both called the code change a strict improvement that should land. Actionable findings fixed in the two follow-up commits; the rest filed as #697/#698. ## Docs `docs/ci-cd.md`, new `docs/decisions/records/ci/gate-trigger-base-resolved.md`, `ci.shared-pr-file-enumeration` and `release.verdict-status-check` past-tensed, catalog regenerated.

timothy added 4 commits 2026-07-28 22:42:36 +02:00

fix(672): trigger the verdict gate on pull_request_target scoped to main ... cf907f0988

`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

docs(672): record that the fix closes the route, not the class (#697) ... 8b73234d78

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

fix(672): review round 1 -- pin the trigger set exactly, sweep the stale claims ... 35a8ea8aef

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(672): widen the residual to the real inventory (#697, #698) ... b2a5c72bfe

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

timothy added 1 commit 2026-07-28 23:03:19 +02:00

fix(672): review round 2 -- correct a stale rule: field, document the self-test gap ... 20b7171fba

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

timothy added 1 commit 2026-07-28 23:17:17 +02:00

docs(672): make the self-test gap discoverable (signals + section pointer) ... ceef16081d

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

timothy commented 2026-07-28 23:26:11 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ ceef160

Three independent cold reviews across two model families. Rounds 1-2 BLOCKED on doc overclaim and residual scope; both reviewers called the code a strict improvement. Final head re-reviewed: nine trigger mutations all caught, sibling guard verified by planting a real .yaml workflow, frontmatter round-tripped under PyYAML not the hand parser, catalog consistent, 340 script tests green. Residual is documented not hidden and filed as #697/#698. Author-orchestrator posting; reviews performed by independent worktree-isolated agents.

Review-verdict: MERGEABLE @ ceef160 Three independent cold reviews across two model families. Rounds 1-2 BLOCKED on doc overclaim and residual scope; both reviewers called the code a strict improvement. Final head re-reviewed: nine trigger mutations all caught, sibling guard verified by planting a real .yaml workflow, frontmatter round-tripped under PyYAML not the hand parser, catalog consistent, 340 script tests green. Residual is documented not hidden and filed as #697/#698. Author-orchestrator posting; reviews performed by independent worktree-isolated agents.

timothy referenced this pull request 2026-07-28 23:26:39 +02:00

review-verdict.yml is head-resolved, so a PR editing it can self-approve the required check #672

timothy merged commit 0c492defac into main 2026-07-28 23:44:20 +02:00

timothy deleted branch fix/672-review-verdict-head-resolution 2026-07-28 23:44:20 +02:00

timothy referenced this issue from a commit 2026-07-28 23:44:21 +02:00

Merge pull request 'fix(672): trigger the verdict gate on pull_request_target scoped to main' (#699) from fix/672-review-verdict-head-resolution into main

timothy referenced this pull request 2026-07-29 00:02:01 +02:00

review-verdict.yml is head-resolved, so a PR editing it can self-approve the required check #672

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#699