timothy/ersatztv
1
0
Fork 0
Code Issues 54 Pull Requests 6 Actions Packages Projects Releases 9 Wiki Activity

Head-ABA: a force-push H1->H2->H1 during paging defeats pr-changed-files.sh, and three contracts still deny it #803

New Issue
Closed
opened 2026-08-16 14:11:25 +02:00 by timothy · 3 comments
timothy commented 2026-08-16 14:11:25 +02:00
Owner
Copy Link
Copy Source

Split out of #778 (PR #802), where three cold-review rounds surfaced it but it sits outside that issue's scope.

The residual

scripts/pr-changed-files.sh binds a paged enumeration to .head.sha, .base.ref and .base.sha, captured before paging and re-checked after, failing closed on any observed movement. That is #707's fix and it works for a MONOTONIC move. It does not survive an ABA:

  • BASE alias — main -> scratch -> main. Fenced, but only for the one caller that also runs the monotonic change_target_branch event counter (ci.verdict-write-retarget-fence).
  • HEAD alias — a force-push H1 -> H2 -> H1 during pagination. Unfenced anywhere. Page 1 is enumerated at H1, middle pages at H2, and the final .head.sha comparison passes because the head is H1 again. No counter moves. A mixed file list can then produce a docs-only exemption success that no single head ever justified.

#778 classified this honestly in docs/remote-state-inventory.md (both affected rows now say the fence covers the base axis only). What it did not do is correct the older contracts that still assert more:

  • scripts/pr-changed-files.sh header — still says the enumeration is "bound to one head"
  • .gitea/workflows/review-verdict.yml — still describes the result as verified-complete
  • docs/decisions/records/release/verdict-status-check.md — absolute safety claim, now qualified by process.check-and-use-pins-a-version; needs the bounded-window wording and a Decisions-Edit: yes trailer

Why it was deferred rather than folded in

#778's scope was "enumerate every read-then-write site against remote state and classify it", plus fixes for what that enumeration found. The head-ABA predates #778 and lives in #707's mechanism; correcting three pre-existing contracts (one of them another active decision record) in a PR already at three review rounds is how a scoped change stops being reviewable. The inventory rows are accurate today, so the state is documented-and-tracked, not silently wrong.

Options

  1. A monotonic head-mutation fence, mirroring the retarget counter — count force-push events on the PR timeline before and after enumeration. Symmetric with the control that already exists, and the same abstain-and-hand-off termination argument applies.
  2. Enumerate against an immutable pair instead of the live PR — resolve base and head to commit shas once and diff those trees, so pagination cannot straddle two states.
  3. Do neither, and qualify the three contracts to match the inventory. Cheapest, and strictly better than today, since the claims are what mislead.

Option 1 is the recommendation; option 3 is the floor and should land regardless.

Done-when

  • The three stale contracts above qualified to match docs/remote-state-inventory.md
  • A decision recorded on whether to fence head aliasing or accept it, with the reason
  • If fencing: a test that reproduces the H1->H2->H1 straddle and goes red without the fence
  • Adversarial review passed
Split out of #778 (PR #802), where three cold-review rounds surfaced it but it sits outside that issue's scope. ## The residual `scripts/pr-changed-files.sh` binds a paged enumeration to `.head.sha`, `.base.ref` and `.base.sha`, captured before paging and re-checked after, failing closed on any observed movement. That is #707's fix and it works for a MONOTONIC move. It does not survive an **ABA**: - **BASE alias** — `main -> scratch -> main`. Fenced, but only for the one caller that also runs the monotonic `change_target_branch` event counter (`ci.verdict-write-retarget-fence`). - **HEAD alias** — a force-push `H1 -> H2 -> H1` during pagination. **Unfenced anywhere.** Page 1 is enumerated at H1, middle pages at H2, and the final `.head.sha` comparison passes because the head is H1 again. No counter moves. A mixed file list can then produce a docs-only exemption `success` that no single head ever justified. #778 classified this honestly in `docs/remote-state-inventory.md` (both affected rows now say the fence covers the base axis only). What it did **not** do is correct the older contracts that still assert more: - [x] `scripts/pr-changed-files.sh` header — still says the enumeration is "bound to one head" - [x] `.gitea/workflows/review-verdict.yml` — still describes the result as verified-complete - [x] `docs/decisions/records/release/verdict-status-check.md` — absolute safety claim, now qualified by `process.check-and-use-pins-a-version`; needs the bounded-window wording and a `Decisions-Edit: yes` trailer ## Why it was deferred rather than folded in #778's scope was "enumerate every read-then-write site against remote state and classify it", plus fixes for what that enumeration found. The head-ABA predates #778 and lives in #707's mechanism; correcting three pre-existing contracts (one of them another active decision record) in a PR already at three review rounds is how a scoped change stops being reviewable. The inventory rows are accurate today, so the state is documented-and-tracked, not silently wrong. ## Options 1. A monotonic **head-mutation** fence, mirroring the retarget counter — count force-push events on the PR timeline before and after enumeration. Symmetric with the control that already exists, and the same abstain-and-hand-off termination argument applies. 2. Enumerate against an **immutable** pair instead of the live PR — resolve base and head to commit shas once and diff those trees, so pagination cannot straddle two states. 3. Do neither, and qualify the three contracts to match the inventory. Cheapest, and strictly better than today, since the claims are what mislead. Option 1 is the recommendation; option 3 is the floor and should land regardless. ## Done-when - [x] The three stale contracts above qualified to match `docs/remote-state-inventory.md` - [x] A decision recorded on whether to fence head aliasing or accept it, with the reason - [x] If fencing: a test that reproduces the H1->H2->H1 straddle and goes red without the fence - [x] Adversarial review passed
timothy added the ci-cdpriority: medium labels 2026-08-16 14:11:34 +02:00
timothy commented 2026-08-16 16:15:31 +02:00
Author
Owner
Copy Link
Copy Source

Adding a third item found while closing #778, same family as the head-ABA above.

$sha is captured once and used all run in pretooluse-merge-consent.sh

sha comes from the PR snapshot at the top of the hook. Every later check — the CI combined status, the review-verdict/h10 status, the verdict-comment classification — is evaluated against that captured value, so a push landing mid-run is checked against the commit it replaced.

This is exactly the defect that was live for $base_ref until #778 re-read it immediately before the branch-protection lookup (PR #802, fix(778): round 4). The base got the fix because the stale read was inside the code that PR introduced; the sha case spans the pre-existing H10 logic, so it was documented in docs/remote-state-inventory.md and deferred here rather than folded into a PR already at five review rounds.

Bounded, not closed: a head the verdict does not cover cannot inherit the sha-bound required status, so the server refuses the merge. Nothing in the hook bounds it.

  • Re-read .head.sha and compare before the grant, symmetric with the base re-read, or record why the asymmetry is right
Adding a third item found while closing #778, same family as the head-ABA above. ## `$sha` is captured once and used all run in `pretooluse-merge-consent.sh` `sha` comes from the PR snapshot at the top of the hook. Every later check — the CI combined status, the `review-verdict/h10` status, the verdict-comment classification — is evaluated against that captured value, so a push landing mid-run is checked against the commit it replaced. This is **exactly** the defect that was live for `$base_ref` until #778 re-read it immediately before the branch-protection lookup (PR #802, `fix(778): round 4`). The base got the fix because the stale read was inside the code that PR introduced; the sha case spans the pre-existing H10 logic, so it was documented in `docs/remote-state-inventory.md` and deferred here rather than folded into a PR already at five review rounds. Bounded, not closed: a head the verdict does not cover cannot inherit the sha-bound required status, so the server refuses the merge. Nothing in the hook bounds it. - [ ] Re-read `.head.sha` and compare before the grant, symmetric with the base re-read, or record why the asymmetry is right
timothy added the in-progress label 2026-08-28 20:08:59 +02:00
timothy commented 2026-08-28 20:09:00 +02:00
Author
Owner
Copy Link
Copy Source

Claiming — Claude Code session, 2026-08-28. Bundling #664 with this: it is the SAME head-ABA defect, filed independently from the #649 cold review, and both cannot be closed apart.

Two measurements taken against the live instance before starting, because both bear on which option is available:

  1. Gitea is now 1.27.1, not 1.25.4. GET /compare/{base}...{head} still returns total_commits + commits and no files key (probed against origin/main~3...origin/main, 12 commits). So pr-changed-files.sh's header claim is still TRUE — option 2 (pin the diff to two shas) remains unavailable — but its version stamp is stale and will be re-dated.
  2. The timeline records every push as a pull_push event carrying {"is_force_push": bool, "commit_ids": [...]}. PR #761 has three with is_force_push: true; PR #802 has eighteen, all false. That count is monotonic and cannot alias, exactly like change_target_branch.

So option 1 is implementable and is what this will do, with option 3 (qualify the three contracts) landing alongside it rather than instead of it. A force-push fires synchronize, which is in this workflow's types:, so the abstain-and-hand-off termination argument carries over from ci.verdict-write-retarget-fence unchanged.

Claiming — Claude Code session, 2026-08-28. Bundling **#664** with this: it is the SAME head-ABA defect, filed independently from the #649 cold review, and both cannot be closed apart. Two measurements taken against the live instance before starting, because both bear on which option is available: 1. **Gitea is now 1.27.1, not 1.25.4.** `GET /compare/{base}...{head}` still returns `total_commits` + `commits` and **no `files`** key (probed against `origin/main~3...origin/main`, 12 commits). So `pr-changed-files.sh`'s header claim is still TRUE — option 2 (pin the diff to two shas) remains unavailable — but its version stamp is stale and will be re-dated. 2. **The timeline records every push as a `pull_push` event** carrying `{"is_force_push": bool, "commit_ids": [...]}`. PR #761 has three with `is_force_push: true`; PR #802 has eighteen, all false. That count is monotonic and cannot alias, exactly like `change_target_branch`. So **option 1 is implementable** and is what this will do, with option 3 (qualify the three contracts) landing alongside it rather than instead of it. A force-push fires `synchronize`, which is in this workflow's `types:`, so the abstain-and-hand-off termination argument carries over from `ci.verdict-write-retarget-fence` unchanged.
timothy commented 2026-08-29 01:30:05 +02:00
Author
Owner
Copy Link
Copy Source

Closing record

Outcome: Shipped in PR #873. The HEAD alias is now fenced: count_retargets becomes count_pr_mutations, counting pull_push alongside change_target_branch on one timeline walk, captured before classification and re-counted before the status POST — either count moving withholds the write. The advisory hook re-reads .head.sha at the same hoist and off the same response as the base re-read (a moved head denies, an unreadable one asks). All three overclaiming contracts are corrected, plus four paraphrases the first sweep missed. Two pre-existing fail-opens in the shared walk were found by review and fixed: an empty array first page was trusted on any page while the null arm required page > 1, and no row was validated before .type was selected on.

Root cause: Every binding scripts/pr-changed-files.sh performs compares a value against itself, which is ABA-vulnerable by construction. The base axis had been fenced with a monotonic counter since #706; the head axis had nothing, and three contracts asserted otherwise. Option 2 (pin the diff to two shas) stays unavailable: re-probed at Gitea 1.27.1 on 2026-08-28, GET /compare/{base}...{head} returns total_commits/commits and no files key.

Decisions/conventions changed: ci.verdict-write-retarget-fence amended in place (both axes, the head-axis section, two new residuals); ci.shared-pr-file-enumeration — the head binding is movement DETECTION, one-way, never a binding, and the enforced caller adds the fence; release.verdict-status-check — the "by construction" claim scoped to the head that has NO status, since a head can still be GIVEN a machine-written exemption. No new key; the key was kept because it is cited in six places and #763 set the precedent of amending this record for a larger addition.

Reusable knowledge:

  • The timeline records every push as a pull_push event with {"is_force_push", "commit_ids"}. Its COUNT is monotonic and is the only key an alias cannot defeat. Do not filter on is_force_push: an ordinary push also invalidates a mid-flight enumeration, and an ABA's restoring push can be non-forced when H1 is an ancestor.
  • The head ABA is not hypothetical. PR #761 really went 8798a1d -> 830a407 -> 8798a1d. Ordinary force-push-and-revert produces the alias; no attacker required.
  • A fence keyed on a count's VALUE rather than its MOVEMENT would withhold every exemption ever — every PR has a non-zero pull_push count, since it is created by a push. That is the #751 shape, and it is the first test to write.
  • Gitea creates the push comment BEFORE emitting the synchronize notification, so a run cannot abstain on its own trigger. That is an ordering property of the source; the 26-102s measured margin only corroborates it, and started_at is a lower bound (69s end to end on run 2385).
  • YAML strips a run: block's common indentation, so a line written at the block's base indent executes at COLUMN 0. That is what makes a backslash-continuation evasion reachable, and it is why a mutant placed at column 0 in the file proves nothing — it just breaks the YAML.

Verification: 1224 passed, 2 skipped (PYTHONPATH=. python3 -m pytest scripts/tests -q); ruff check + format clean; decisions-validate: OK; YAML and bash -n clean on every touched shell body. No .cs touched, so the BOM/format gate does not apply. Rebased onto #871/#872 mid-flight and re-verified green together (a whole-tree gate is only as green as its base). Fourteen mutations verified to redden the right tests, each mutant confirmed to PARSE first — a mutant that does not parse is not a measurement.

Deferred: #870 — the walk's null terminator is defeatable. ListIssueCommentsAndTimeline applies LIMIT/OFFSET in FindComments and filters afterwards, dropping CommentTypeCode rows into a nil slice that serialises as bare null, so a page of inline review comments reads as exhaustion while later pages hold events. Pre-existing; defeats the BASE fence identically. The fence is therefore documented as closing the ABA on a timeline with no such truncating block, NOT the ABA outright. Also noted: a flag reaching jq through a variable is outside the jq -e guard's declared scope, and that guard is function-scoped by design.

Docs updated: docs/decisions/records/ci/verdict-write-retarget-fence.md, docs/decisions/records/ci/shared-pr-file-enumeration.md, docs/decisions/records/release/verdict-status-check.md, docs/decisions/records/ci/actions-credential-scoping.md (rename annotation on a dated measurement quote), docs/decisions/README.md (regenerated), docs/remote-state-inventory.md (4 rows), docs/guard-inventory.md (row + summary count), docs/ci-cd.md.

## Closing record **Outcome:** Shipped in PR #873. The HEAD alias is now fenced: `count_retargets` becomes `count_pr_mutations`, counting `pull_push` alongside `change_target_branch` on one timeline walk, captured before classification and re-counted before the status POST — either count moving withholds the write. The advisory hook re-reads `.head.sha` at the same hoist and off the same response as the base re-read (a moved head denies, an unreadable one asks). All three overclaiming contracts are corrected, plus four paraphrases the first sweep missed. Two pre-existing fail-opens in the shared walk were found by review and fixed: an empty **array** first page was trusted on any page while the `null` arm required `page > 1`, and no row was validated before `.type` was selected on. **Root cause:** Every binding `scripts/pr-changed-files.sh` performs compares a value against itself, which is ABA-vulnerable by construction. The base axis had been fenced with a monotonic counter since #706; the head axis had nothing, and three contracts asserted otherwise. Option 2 (pin the diff to two shas) stays unavailable: re-probed at Gitea 1.27.1 on 2026-08-28, `GET /compare/{base}...{head}` returns `total_commits`/`commits` and no `files` key. **Decisions/conventions changed:** `ci.verdict-write-retarget-fence` amended in place (both axes, the head-axis section, two new residuals); `ci.shared-pr-file-enumeration` — the head binding is movement DETECTION, one-way, never a binding, and the enforced caller adds the fence; `release.verdict-status-check` — the "by construction" claim scoped to the head that has NO status, since a head can still be GIVEN a machine-written exemption. No new key; the key was kept because it is cited in six places and #763 set the precedent of amending this record for a larger addition. **Reusable knowledge:** - The timeline records every push as a `pull_push` event with `{"is_force_push", "commit_ids"}`. Its COUNT is monotonic and is the only key an alias cannot defeat. Do not filter on `is_force_push`: an ordinary push also invalidates a mid-flight enumeration, and an ABA's restoring push can be non-forced when H1 is an ancestor. - **The head ABA is not hypothetical.** PR #761 really went `8798a1d -> 830a407 -> 8798a1d`. Ordinary force-push-and-revert produces the alias; no attacker required. - **A fence keyed on a count's VALUE rather than its MOVEMENT would withhold every exemption ever** — every PR has a non-zero `pull_push` count, since it is created by a push. That is the #751 shape, and it is the first test to write. - Gitea creates the push comment BEFORE emitting the synchronize notification, so a run cannot abstain on its own trigger. That is an ordering property of the source; the 26-102s measured margin only corroborates it, and `started_at` is a lower bound (69s end to end on run 2385). - **YAML strips a `run:` block's common indentation, so a line written at the block's base indent executes at COLUMN 0.** That is what makes a backslash-continuation evasion reachable, and it is why a mutant placed at column 0 *in the file* proves nothing — it just breaks the YAML. **Verification:** 1224 passed, 2 skipped (`PYTHONPATH=. python3 -m pytest scripts/tests -q`); ruff check + format clean; `decisions-validate: OK`; YAML and `bash -n` clean on every touched shell body. No `.cs` touched, so the BOM/format gate does not apply. Rebased onto #871/#872 mid-flight and re-verified green together (a whole-tree gate is only as green as its base). Fourteen mutations verified to redden the right tests, each mutant confirmed to PARSE first — a mutant that does not parse is not a measurement. **Deferred:** **#870** — the walk's `null` terminator is defeatable. `ListIssueCommentsAndTimeline` applies LIMIT/OFFSET in `FindComments` and filters afterwards, dropping `CommentTypeCode` rows into a nil slice that serialises as bare `null`, so a page of inline review comments reads as exhaustion while later pages hold events. Pre-existing; defeats the BASE fence identically. The fence is therefore documented as closing the ABA on a timeline with no such truncating block, NOT the ABA outright. Also noted: a flag reaching jq through a variable is outside the `jq -e` guard's declared scope, and that guard is function-scoped by design. **Docs updated:** `docs/decisions/records/ci/verdict-write-retarget-fence.md`, `docs/decisions/records/ci/shared-pr-file-enumeration.md`, `docs/decisions/records/release/verdict-status-check.md`, `docs/decisions/records/ci/actions-credential-scoping.md` (rename annotation on a dated measurement quote), `docs/decisions/README.md` (regenerated), `docs/remote-state-inventory.md` (4 rows), `docs/guard-inventory.md` (row + summary count), `docs/ci-cd.md`.
timothy removed the in-progress label 2026-08-29 01:30:41 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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 ci-cd priority: medium
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#803