The review-verdict/h10 exemption path decides from mutable PR state — three routes to a forged exemption #698

Closed
opened 2026-07-28 22:32:48 +02:00 by timothy · 3 comments
Owner

Found by cross-family adversarial review of the #672 PR. None of these are introduced by #672 — they predate it and survive it. They share one root cause, so they are filed together: the exemption decision in .gitea/workflows/review-verdict.yml is derived from mutable or unattributed PR state, and a machine-written success is never revalidated.

priority: high rather than medium because unlike #672 and #697 these need no workflow rewrite and no credential — routes 1 and 2 are driven entirely through ordinary PR operations.

Route 1 — retarget race against the docs-only classifier

  1. Create head H (malicious) and scratch base S, chosen so H vs S is docs-only.
  2. Open H → main. The trusted base workflow starts.
  3. Retarget the live PR to S while the job is queued or running.
  4. The changed-files endpoint now enumerates against S, so the diff reads docs-only. scripts/pr-changed-files.sh revalidates only .head.sha — never .base.ref/.base.sha — so its exhaustiveness guarantee holds while the base moved underneath it. The job posts h10=success on H.
  5. Retarget back to main. edited is not in the workflow's types:, so nothing reclassifies, and the forged exemption stands on H.

Note this is the exact shape of the #632 retarget hazard, one level down: that one is detected on the hook path by comparing the recorded base branch, and this one happens inside the workflow that writes the status.

Route 2 — hijack an open Renovate PR

The bot exemption reads pull_request.user.login, the immutable PR creator, not whoever pushed the current head. Force-push arbitrary application code onto an open Renovate branch: the PR is still authored by renovate, touches no PROTECTED path, and the workflow posts h10=success.

Route 3 — inherited success is never reclassified

The "already decided for this sha" short-circuit exits on any existing success before checking PR number, base, author or files. That guard exists for a good reason (never overwrite a real human verdict), but it cannot currently tell a human verdict from a machine exemption — so a success obtained via route 1, route 2, a sibling workflow (#697), a direct API call, or inherited across PRs by sha (#663) is accepted unchanged.

Why one issue

All three are "the gate trusts something it cannot attribute." A fix that distinguishes trusted human verdicts from machine exemptions by issuer and structured provenance, and revalidates machine-written success against the live PR/base, addresses all three. Fixing them one at a time invites exactly the drift ci.shared-pr-file-enumeration was written about.

Done-when

  • Route 1 reproduced on scratch PR #703 (opened into main, retargeted mid-run, granted Exempt: docs-only over a diff containing a C# file, retarget back reclassified nothing). Closed unmerged, branches deleted, no forged h10 left anywhere. The "then shown to fail after the fix" half is not observable pre-merge — pull_request_target resolves this workflow's definition from main (the self-test gap in ci.gate-trigger-base-resolved). Covered pre-merge by executed-behaviour tests; live re-check on main right after merge.
  • Route 2 fixed and unit-tested — the bot exemption now additionally requires every path to be a dependency manifest. Live reproduction is split to #708, not done here: it structurally needs a PR authored by renovate, and the only route is pushing code onto an open Renovate branch. Attempted on #679 behind a WIP guard; the permission classifier blocked the force-push needed to restore the branch, so it was abandoned and #679 was returned to byte-identical original state. Deferred deliberately rather than ticked on a unit test alone.
  • Machine-written success distinguishable and revalidated — by .creator.login (non-null) and a Review-verdict: description and a matching recorded base. The never-overwrite-a-human-verdict property is preserved and separately tested for success and failure, plus a mid-run verdict case.
  • edited decided and empirically verified — it fires. Verified via an edited-only probe workflow on a scratch base branch (the self-test gap blocks testing on main); the run was created, which only happens if the event fired and the branches: filter matched the new base. Honest limit: the job never got a runner slot, so this is the run's creation, not its log line. Recorded as giving detection, not atomicity — the ordering residual is #706.
  • release.verdict-status-check + ci.gate-trigger-base-resolved updated, plus new records ci.exemption-provenance and ci.grep-q-pipefail-inversion, and ci.shared-pr-file-enumeration (contract now 5 args).
  • Adversarial review passed — six cross-family rounds. Verdicts: BLOCKED (3 Blockers, 2 Highs) → BLOCKED (2H, 3M) → BLOCKED (3M) → BLOCKED (1M) → BLOCKED (1 High) → MERGEABLE. Findings not fixed here are tracked as #706, #707, #708.

Residuals deliberately left open, not silently dropped: #706 (status writes are not serialized — ABA retarget + human-rejection race), #707 (paging vs a live-advancing base), #708 (route-2 live reproduction), #697 (credentials that can write the status — a provenance check is not an authentication one).

Related

#672 (definition rewrite — fixed), #697 (credentials that can write the status), #663 (status is repo-global per sha), #632 (retarget detection on the hook path).

Found by cross-family adversarial review of the #672 PR. **None of these are introduced by #672** — they predate it and survive it. They share one root cause, so they are filed together: the exemption decision in `.gitea/workflows/review-verdict.yml` is derived from **mutable or unattributed PR state**, and a machine-written `success` is never revalidated. `priority: high` rather than medium because unlike #672 and #697 these need no workflow rewrite and no credential — routes 1 and 2 are driven entirely through ordinary PR operations. ## Route 1 — retarget race against the docs-only classifier 1. Create head `H` (malicious) and scratch base `S`, chosen so `H` vs `S` is docs-only. 2. Open `H → main`. The trusted base workflow starts. 3. **Retarget the live PR to `S`** while the job is queued or running. 4. The changed-files endpoint now enumerates against `S`, so the diff reads docs-only. `scripts/pr-changed-files.sh` revalidates only `.head.sha` — never `.base.ref`/`.base.sha` — so its exhaustiveness guarantee holds while the *base* moved underneath it. The job posts `h10=success` on `H`. 5. Retarget back to `main`. `edited` is not in the workflow's `types:`, so nothing reclassifies, and the forged exemption stands on `H`. Note this is the exact shape of the #632 retarget hazard, one level down: that one is detected on the *hook* path by comparing the recorded base branch, and this one happens inside the workflow that writes the status. ## Route 2 — hijack an open Renovate PR The bot exemption reads `pull_request.user.login`, the **immutable PR creator**, not whoever pushed the current head. Force-push arbitrary application code onto an open Renovate branch: the PR is still authored by `renovate`, touches no `PROTECTED` path, and the workflow posts `h10=success`. ## Route 3 — inherited success is never reclassified The "already decided for this sha" short-circuit exits on any existing `success` **before** checking PR number, base, author or files. That guard exists for a good reason (never overwrite a real human verdict), but it cannot currently tell a human verdict from a machine exemption — so a `success` obtained via route 1, route 2, a sibling workflow (#697), a direct API call, or inherited across PRs by sha (#663) is accepted unchanged. ## Why one issue All three are "the gate trusts something it cannot attribute." A fix that distinguishes **trusted human verdicts from machine exemptions by issuer and structured provenance**, and revalidates machine-written success against the live PR/base, addresses all three. Fixing them one at a time invites exactly the drift `ci.shared-pr-file-enumeration` was written about. ## Done-when - [x] **Route 1 reproduced** on scratch PR #703 (opened into `main`, retargeted mid-run, granted `Exempt: docs-only` over a diff containing a C# file, retarget back reclassified nothing). Closed unmerged, branches deleted, no forged `h10` left anywhere. The *"then shown to fail after the fix"* half is **not observable pre-merge** — `pull_request_target` resolves this workflow's definition from `main` (the self-test gap in `ci.gate-trigger-base-resolved`). Covered pre-merge by executed-behaviour tests; live re-check on `main` right after merge. - [x] **Route 2 fixed and unit-tested** — the bot exemption now additionally requires every path to be a dependency manifest. **Live reproduction is split to #708**, not done here: it structurally needs a PR authored by `renovate`, and the only route is pushing code onto an open Renovate branch. Attempted on #679 behind a WIP guard; the permission classifier blocked the force-push needed to restore the branch, so it was abandoned and #679 was returned to byte-identical original state. Deferred deliberately rather than ticked on a unit test alone. - [x] **Machine-written `success` distinguishable and revalidated** — by `.creator.login` (non-null) **and** a `Review-verdict:` description **and** a matching recorded base. The never-overwrite-a-human-verdict property is preserved and separately tested for `success` and `failure`, plus a mid-run verdict case. - [x] **`edited` decided and empirically verified** — it fires. Verified via an `edited`-only probe workflow on a scratch base branch (the self-test gap blocks testing on `main`); the run was created, which only happens if the event fired and the `branches:` filter matched the new base. Honest limit: the job never got a runner slot, so this is the run's creation, not its log line. Recorded as giving **detection, not atomicity** — the ordering residual is #706. - [x] **`release.verdict-status-check` + `ci.gate-trigger-base-resolved` updated**, plus new records `ci.exemption-provenance` and `ci.grep-q-pipefail-inversion`, and `ci.shared-pr-file-enumeration` (contract now 5 args). - [x] **Adversarial review passed** — six cross-family rounds. Verdicts: BLOCKED (3 Blockers, 2 Highs) → BLOCKED (2H, 3M) → BLOCKED (3M) → BLOCKED (1M) → BLOCKED (1 High) → **MERGEABLE**. Findings not fixed here are tracked as #706, #707, #708. **Residuals deliberately left open, not silently dropped:** #706 (status writes are not serialized — ABA retarget + human-rejection race), #707 (paging vs a live-advancing base), #708 (route-2 live reproduction), #697 (credentials that can write the status — a provenance check is not an authentication one). ## Related #672 (definition rewrite — fixed), #697 (credentials that can write the status), #663 (status is repo-global per sha), #632 (retarget detection on the hook path).
timothy added the ci-cdpriority: highsecurity labels 2026-07-28 22:32:48 +02:00
timothy added the in-progress label 2026-07-29 18:08:51 +02:00
Author
Owner

Claiming this (Claude Code orchestrator session, 2026-07-29). Selector ranked it top of priority: high with clear deps; pre-claim checks were clean — no open PR referencing #698, no remote branch naming it, no prior claiming comment.

Scope note for parallel sessions: I am taking #698 only (the exemption-path logic in .gitea/workflows/review-verdict.yml, routes 1–3). #697 is NOT claimed — its durable fix is a credential/infra change (job-token permissions, secret scoping) that belongs partly to server-management, and I'd rather not entangle a workflow-logic fix with a secrets rollout. If someone picks up #697 concurrently, expect a textual conflict only in review-verdict.yml's status-posting step.

Claiming this (Claude Code orchestrator session, 2026-07-29). Selector ranked it top of `priority: high` with clear deps; pre-claim checks were clean — no open PR referencing #698, no remote branch naming it, no prior claiming comment. Scope note for parallel sessions: I am taking **#698 only** (the exemption-path logic in `.gitea/workflows/review-verdict.yml`, routes 1–3). **#697 is NOT claimed** — its durable fix is a credential/infra change (job-token permissions, secret scoping) that belongs partly to server-management, and I'd rather not entangle a workflow-logic fix with a secrets rollout. If someone picks up #697 concurrently, expect a textual conflict only in `review-verdict.yml`'s status-posting step.
Author
Owner

PR #705 opened. Status against this issue's ## Done-when, stated honestly rather than optimistically:

  • Route 1 reproduced — probe PR #703, closed unmerged, branches deleted. Opened H → main, retargeted mid-run to a scratch base, the job read docs-only and posted h10=success — "Exempt: docs-only change"; retargeted back to main and nothing reclassified (created_at == updated_at), leaving a PR into main whose diff carried a C# file behind a green required check. The "then shown to fail after the fix" half is not observable pre-merge — pull_request_target resolves this workflow's definition from main (the self-test gap in ci.gate-trigger-base-resolved). Covered pre-merge by executed-behaviour tests, verified live on main right after merge.
  • Route 2 reproduced — not done, deliberately. It structurally needs a PR authored by renovate, so the only faithful reproduction is pushing code onto an open Renovate branch. I set a WIP guard on #679 (confirmed draft: true, mergeable: false) and the permission classifier then blocked the force-push required to restore the branch afterwards. Rather than mutate a live dependency PR with no clean way back, I stopped and restored #679 to byte-identical original state (title restored, draft: false, head still 378171e). Nothing was ever pushed to its branch. Covered by unit tests + the measured allow-list instead.
  • Machine-written success distinguishable and revalidated — by .creator.login (non-null) and a Review-verdict: description. Measured on the combined endpoint the job reads: user-posted statuses carry a creator, Actions-posted ones carry null. The never-overwrite-a-human-verdict property is preserved and separately tested for both success and failure.
  • edited decided and empirically verified — it fires. Since the self-test gap blocks testing on main, an edited-only probe workflow went on a scratch base branch and a PR was retargeted onto it; the run was created, which only happens if the event fired and the branches: filter matched the new base. Limit worth recording: the job never got a runner slot, so this is the run's creation, not its log line.
  • release.verdict-status-check + ci.gate-trigger-base-resolved updated — plus a new record ci.exemption-provenance and ci.shared-pr-file-enumeration (contract is now 5 args).
  • Adversarial review passed — cross-family review in flight.

Two corrections from the work, recorded because they change what the issue body claims:

  1. My earlier note that Renovate PR #20 had been exempted for a source change is false. #20 merged 2026-06-27; the gate landed 2026-07-25; it carries no h10 status. It is a valid counterfactual (identity-only would have exempted it), not a historical incident, and the record says so.
  2. Route 1 cannot be fixed by pinning the diff to two shas, which was my first instinct. Measured: compare/{base}...{head} returns no files on Gitea 1.25.4, and a --depth=1 fetch of the two shas has no merge base. The fix is a base binding with a stated residual window, closed durably by edited + re-derivation rather than perfectly.
PR #705 opened. Status against this issue's `## Done-when`, stated honestly rather than optimistically: - [x] **Route 1 reproduced** — probe PR #703, closed unmerged, branches deleted. Opened `H → main`, retargeted mid-run to a scratch base, the job read docs-only and posted `h10=success — "Exempt: docs-only change"`; retargeted back to `main` and nothing reclassified (`created_at == updated_at`), leaving a PR into `main` whose diff carried a C# file behind a green required check. The "then shown to fail after the fix" half is **not** observable pre-merge — `pull_request_target` resolves this workflow's definition from `main` (the self-test gap in `ci.gate-trigger-base-resolved`). Covered pre-merge by executed-behaviour tests, verified live on `main` right after merge. - [ ] **Route 2 reproduced** — **not done, deliberately.** It structurally needs a PR *authored by* `renovate`, so the only faithful reproduction is pushing code onto an open Renovate branch. I set a WIP guard on #679 (confirmed `draft: true`, `mergeable: false`) and the permission classifier then blocked the force-push required to restore the branch afterwards. Rather than mutate a live dependency PR with no clean way back, I stopped and restored #679 to byte-identical original state (title restored, `draft: false`, head still `378171e`). Nothing was ever pushed to its branch. Covered by unit tests + the measured allow-list instead. - [x] **Machine-written `success` distinguishable and revalidated** — by `.creator.login` (non-null) **and** a `Review-verdict:` description. Measured on the combined endpoint the job reads: user-posted statuses carry a creator, Actions-posted ones carry `null`. The never-overwrite-a-human-verdict property is preserved and separately tested for both `success` and `failure`. - [x] **`edited` decided and empirically verified** — it fires. Since the self-test gap blocks testing on `main`, an `edited`-only probe workflow went on a scratch base branch and a PR was retargeted onto it; the run was created, which only happens if the event fired and the `branches:` filter matched the new base. Limit worth recording: the job never got a runner slot, so this is the run's *creation*, not its log line. - [x] **`release.verdict-status-check` + `ci.gate-trigger-base-resolved` updated** — plus a new record `ci.exemption-provenance` and `ci.shared-pr-file-enumeration` (contract is now 5 args). - [ ] **Adversarial review passed** — cross-family review in flight. Two corrections from the work, recorded because they change what the issue body claims: 1. My earlier note that Renovate PR #20 *had been* exempted for a source change is **false**. #20 merged 2026-06-27; the gate landed 2026-07-25; it carries no `h10` status. It is a valid *counterfactual* (identity-only would have exempted it), not a historical incident, and the record says so. 2. Route 1 cannot be fixed by pinning the diff to two shas, which was my first instinct. Measured: `compare/{base}...{head}` returns no `files` on Gitea 1.25.4, and a `--depth=1` fetch of the two shas has no merge base. The fix is a base *binding* with a stated residual window, closed durably by `edited` + re-derivation rather than perfectly.
Author
Owner

Closing record

Outcome: Fixed and merged as PR #705 (8 commits, 57e33f9 → main at a3458e6). All three routes closed in the exemption path: the base is bound as a required 5th argument to the shared enumeration and re-checked either side of paging; the bot exemption now requires every path to be a dependency manifest, not just a matching author; and the never-overwrite short-circuit fires only for a status positively identified as a human verdict for this base. edited added to types:.

Root cause: one shape, three symptoms — the exemption decision rested on inputs the judged PR could mutate or that could not be attributed. /pulls/{n}/files diffs against the live base (retargetable), pull_request.user.login is the PR's immutable creator rather than whoever pushed the head, and a machine-written success was indistinguishable from a human verdict. Two further defects surfaced during review and share the theme: a pre-existing grep -q + pipefail inversion that let a large PR skip the PROTECTED guard entirely, and the human-verdict path being bindable to a different base.

Decisions/conventions changed: new ci.exemption-provenance and ci.grep-q-pipefail-inversion; updated ci.gate-trigger-base-resolved (the edited residual is resolved, and it was more serious than that record implied) and ci.shared-pr-file-enumeration (contract is now 5 args).

Reusable knowledge:

  • producer | grep -q… under set -o pipefail inverts on large input. grep exits at first match, the producer takes SIGPIPE (141), and a MATCH reads as a failed pipeline. A here-string is not the fix — bash spills large ones to temp storage and fails the same way. Count with grep -c, evaluate once at top level, validate numeric.
  • Compare against an exact literal; never parse a value out of human-readable text. Two extraction attempts were each defeated: greedy matching by an appended marker, first-match by the valid branch name main)evil truncating to main.
  • When several branches produce the same outcome, asserting the outcome cannot tell you which ran. A dead PROTECTED guard survived three review rounds with three "protected path" tests green. Assert the discriminator; add a cheap stderr sweep for command not found / integer expression expected / unbound variable.
  • A guard whose behaviour depends on a buffer threshold needs a test that crosses it. Every prior test used a handful of short paths, so the whole regime was invisible.

Verification: 385 tests (17+ new), each new guard mutation-verified red-then-green with positive controls so a deadlock cannot masquerade as a fix. Six cross-family adversarial rounds: BLOCKED (3 Blockers, 2 Highs) → BLOCKED (2H, 3M) → BLOCKED (3M) → BLOCKED (1M) → BLOCKED (1 High) → MERGEABLE. Route 1 reproduced live pre-fix (#703) and verified to fail post-fix (#709: success — Exempt: docs-only became pending, on a diff containing a C# file). edited and the branches: [main] fail-closed behaviour both confirmed live. CI green on the merged head.

Deferred: #706 (status writes are not serialized — ABA retarget + human-rejection race; edited gives detection, not atomicity), #707 (offset paging against a live-advancing base), #708 (route-2 live reproduction — permission-blocked here), #697 (credentials that can write the status; this is a provenance check, not an authentication one).

Docs updated: docs/ci-cd.md, docs/decisions/records/ci/exemption-provenance.md (new), docs/decisions/records/ci/grep-q-pipefail-inversion.md (new), docs/decisions/records/ci/gate-trigger-base-resolved.md, docs/decisions/records/ci/shared-pr-file-enumeration.md, regenerated docs/decisions/README.md.

Corrections made during the work, recorded because they were load-bearing: the claim that Renovate PR #20 had been exempted for a source change was false (it predates the gate — it is a counterfactual, not an incident); pinning the diff to two shas is impossible on Gitea 1.25.4 (compare returns no files, --depth=1 has no merge base); and the assertion that a ) in a branch name "mismatches — safe direction" was false for any branch starting with the target base.

## Closing record **Outcome:** Fixed and merged as PR #705 (8 commits, `57e33f9` → `main` at `a3458e6`). All three routes closed in the exemption path: the base is bound as a required 5th argument to the shared enumeration and re-checked either side of paging; the bot exemption now requires every path to be a dependency manifest, not just a matching author; and the never-overwrite short-circuit fires only for a status positively identified as a human verdict **for this base**. `edited` added to `types:`. **Root cause:** one shape, three symptoms — *the exemption decision rested on inputs the judged PR could mutate or that could not be attributed*. `/pulls/{n}/files` diffs against the **live** base (retargetable), `pull_request.user.login` is the PR's immutable **creator** rather than whoever pushed the head, and a machine-written `success` was indistinguishable from a human verdict. Two further defects surfaced during review and share the theme: a **pre-existing** `grep -q` + `pipefail` inversion that let a large PR skip the `PROTECTED` guard entirely, and the human-verdict path being bindable to a different base. **Decisions/conventions changed:** new `ci.exemption-provenance` and `ci.grep-q-pipefail-inversion`; updated `ci.gate-trigger-base-resolved` (the `edited` residual is resolved, and it was more serious than that record implied) and `ci.shared-pr-file-enumeration` (contract is now 5 args). **Reusable knowledge:** - **`producer | grep -q…` under `set -o pipefail` inverts on large input.** grep exits at first match, the producer takes SIGPIPE (141), and a MATCH reads as a failed pipeline. A here-string is *not* the fix — bash spills large ones to temp storage and fails the same way. **Count** with `grep -c`, evaluate once at top level, validate numeric. - **Compare against an exact literal; never parse a value out of human-readable text.** Two extraction attempts were each defeated: greedy matching by an appended marker, first-match by the *valid* branch name `main)evil` truncating to `main`. - **When several branches produce the same outcome, asserting the outcome cannot tell you which ran.** A dead `PROTECTED` guard survived three review rounds with three "protected path" tests green. Assert the discriminator; add a cheap stderr sweep for `command not found` / `integer expression expected` / `unbound variable`. - **A guard whose behaviour depends on a buffer threshold needs a test that crosses it.** Every prior test used a handful of short paths, so the whole regime was invisible. **Verification:** 385 tests (17+ new), each new guard mutation-verified red-then-green with positive controls so a deadlock cannot masquerade as a fix. Six cross-family adversarial rounds: BLOCKED (3 Blockers, 2 Highs) → BLOCKED (2H, 3M) → BLOCKED (3M) → BLOCKED (1M) → BLOCKED (1 High) → **MERGEABLE**. Route 1 reproduced live pre-fix (#703) and **verified to fail post-fix** (#709: `success — Exempt: docs-only` became `pending`, on a diff containing a C# file). `edited` and the `branches: [main]` fail-closed behaviour both confirmed live. CI green on the merged head. **Deferred:** #706 (status writes are not serialized — ABA retarget + human-rejection race; `edited` gives detection, not atomicity), #707 (offset paging against a live-advancing base), #708 (route-2 live reproduction — permission-blocked here), #697 (credentials that can write the status; this is a provenance check, not an authentication one). **Docs updated:** `docs/ci-cd.md`, `docs/decisions/records/ci/exemption-provenance.md` (new), `docs/decisions/records/ci/grep-q-pipefail-inversion.md` (new), `docs/decisions/records/ci/gate-trigger-base-resolved.md`, `docs/decisions/records/ci/shared-pr-file-enumeration.md`, regenerated `docs/decisions/README.md`. **Corrections made during the work, recorded because they were load-bearing:** the claim that Renovate PR #20 *had been* exempted for a source change was false (it predates the gate — it is a counterfactual, not an incident); pinning the diff to two shas is impossible on Gitea 1.25.4 (`compare` returns no `files`, `--depth=1` has no merge base); and the assertion that a `)` in a branch name "mismatches — safe direction" was false for any branch starting with the target base.
timothy removed the in-progress label 2026-07-30 21:26:16 +02:00
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#698