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

pr-changed-files.sh head binding cannot see an A→B→A force-push across its paging round-trips #664

New Issue
Closed
opened 2026-07-26 23:28:42 +02:00 by timothy · 2 comments
timothy commented 2026-07-26 23:28:42 +02:00
Owner
Copy Link
Copy Source

Found by the cold review of the #649 PR. Pre-existing — the script is unchanged by that PR (it landed in #658), which is why this was filed rather than fixed there.

The gap

scripts/pr-changed-files.sh pages pulls/{n}/files until it sees a validated empty page, then re-reads the PR head once and refuses if it differs from the expected sha. That closes A→B: pages assembled across a force-push belong to no single commit, and the final read catches it.

It does not close A→B→A:

  1. Head is A. Page 1 returns 50 docs paths. A protected path sits on page 2.
  2. Force-push B before page 2 is requested. Page 2 for B returns [] — a validated empty page, so complete=yes.
  3. Force-push back to A before the binding read.
  4. sha_after == expected_sha. The enumeration is declared complete and bound, over a partial list that omits the protected path.

Both complete=yes and the sha binding hold, and the result is a docs-only exemption for a PR that touches a protected path.

Why this is hard rather than an oversight

The binding is a post-hoc check on a list assembled over several round-trips. Closing it properly needs one of:

  • a commit-pinned files endpoint — Gitea's pulls/{n}/files has no sha parameter, so there is nothing to pin to;
  • reading the head before page 1 as well and comparing both ends — narrows the window but does not close it, since the same round trip can happen entirely between the two reads;
  • computing the diff locally from a fetched base and head, which removes the API from the trust path altogether but makes the advisory hook considerably heavier and needs a checkout on the runner side.

The third is the only one that actually closes it, and it is a real redesign rather than a patch.

Exposure

Very low. It requires two precisely-timed force-pushes bracketing a specific HTTP request in a job that finishes in seconds, by the one human account with push rights, against a gate whose stated threat model is a careless change rather than a hostile one. Filing it so the limitation is written down where the enumeration's guarantees are described — ci.shared-pr-file-enumeration currently reads as though head binding closes the race, and it closes only the one-way case.

Done-when

  • A decision recorded: narrow the guarantee in ci.shared-pr-file-enumeration to "detects one-way head movement", or implement local diff computation
  • If accepted rather than fixed: the record states the A→B→A case explicitly, so a future reader does not conclude the binding is complete
  • If fixed: a test reproduces the A→B→A sequence and fails without the fix
  • Adversarial review passed
Found by the cold review of the #649 PR. **Pre-existing** — the script is unchanged by that PR (it landed in #658), which is why this was filed rather than fixed there. ## The gap `scripts/pr-changed-files.sh` pages `pulls/{n}/files` until it sees a validated empty page, then re-reads the PR head **once** and refuses if it differs from the expected sha. That closes A→B: pages assembled across a force-push belong to no single commit, and the final read catches it. It does not close **A→B→A**: 1. Head is `A`. Page 1 returns 50 docs paths. A protected path sits on page 2. 2. Force-push `B` before page 2 is requested. Page 2 for `B` returns `[]` — a validated empty page, so `complete=yes`. 3. Force-push back to `A` before the binding read. 4. `sha_after == expected_sha`. The enumeration is declared complete and bound, over a **partial** list that omits the protected path. Both `complete=yes` and the sha binding hold, and the result is a docs-only exemption for a PR that touches a protected path. ## Why this is hard rather than an oversight The binding is a *post-hoc* check on a list assembled over several round-trips. Closing it properly needs one of: - a **commit-pinned** files endpoint — Gitea's `pulls/{n}/files` has no `sha` parameter, so there is nothing to pin to; - reading the head **before** page 1 as well and comparing both ends — narrows the window but does not close it, since the same round trip can happen entirely between the two reads; - computing the diff **locally** from a fetched base and head, which removes the API from the trust path altogether but makes the advisory hook considerably heavier and needs a checkout on the runner side. The third is the only one that actually closes it, and it is a real redesign rather than a patch. ## Exposure Very low. It requires two precisely-timed force-pushes bracketing a specific HTTP request in a job that finishes in seconds, by the one human account with push rights, against a gate whose stated threat model is a careless change rather than a hostile one. Filing it so the limitation is written down where the enumeration's guarantees are described — `ci.shared-pr-file-enumeration` currently reads as though head binding closes the race, and it closes only the one-way case. ## Done-when - [x] A decision recorded: narrow the guarantee in `ci.shared-pr-file-enumeration` to "detects one-way head movement", or implement local diff computation - [x] If accepted rather than fixed: the record states the A→B→A case explicitly, so a future reader does not conclude the binding is complete - [x] If fixed: a test reproduces the A→B→A sequence and fails without the fix - [x] Adversarial review passed
timothy added the ci-cdpriority: lowsecurity labels 2026-07-26 23:28:42 +02:00
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 as part of the #803 bundle — Claude Code session, 2026-08-28. #803 is the same defect written up independently (it also carries two siblings found while closing #778), so the head-ABA is being fixed once and both issues close together. Work and review rounds will be tracked on #803.

Claiming as part of the **#803** bundle — Claude Code session, 2026-08-28. #803 is the same defect written up independently (it also carries two siblings found while closing #778), so the head-ABA is being fixed once and both issues close together. Work and review rounds will be tracked on #803.
timothy commented 2026-08-29 01:30:31 +02:00
Author
Owner
Copy Link
Copy Source

Closing record

Outcome: Closed by PR #873, together with #803 — the same defect, filed independently. #664 came out of the #649 cold review; #803 out of #778's read-then-write inventory. Fixed once: the enforced caller now fences its write on the monotonic pull_push count, so an A -> B -> A force-push spanning pr-changed-files.sh's paging withholds the exemption instead of passing it.

Root cause: The head binding is a post-hoc comparison of a value against itself, which is ABA-vulnerable by construction — this issue's own analysis was right that no tighter check inside the script can close it. What it under-weighted is that a caller CAN, with a monotonic key: the timeline's pull_push count moves by two where the sha moves by zero.

Decisions/conventions changed: ci.shared-pr-file-enumeration — the script's guarantee is stated as one-way MOVEMENT DETECTION rather than a binding, in the rule:, the mechanics: and the body (the frontmatter still said "bound to BOTH the given sha", contradicting a body that had already been narrowed), and the new caller-side fence is recorded as covering the ENFORCED path only. ci.verdict-write-retarget-fence amended to carry both axes.

Reusable knowledge:

  • This issue's third Done-when box asked for a test reproducing A -> B -> A. Writing it exposed the thing worth carrying: the ABA costs TWO push events, so a fence keyed on the count's VALUE rather than its MOVEMENT would withhold every exemption ever issued — every PR has a non-zero pull_push count, because it is created by a push.
  • The alias is not adversarial-only. PR #761 really went 8798a1d -> 830a407 -> 8798a1d through ordinary force-push-and-revert.
  • The exposure analysis here said "two precisely-timed force-pushes bracketing a specific HTTP request". That is right for the enforced gate, and the fence now covers it — but the enumerator's contract had ALSO been read as a guarantee by the advisory hook and by the workflow's own call site, and correcting those three (plus four paraphrases) was as much of the fix as the fence was.

Verification: Covered by PR #873's gate — 1224 passed, 2 skipped; ruff clean; decisions-validate OK; CI green on the merged head. The A -> B -> A reproduction is test_a_HEAD_ABA_DURING_the_run_posts_NOTHING, mutation-proved to redden when the fence arm is removed.

Deferred: #870 — the timeline walk's null terminator is defeatable (Gitea pages before it filters), which caps what the fence closes. Documented in the record rather than left implicit, since this issue's own complaint was a guarantee stated more broadly than the code supports.

Docs updated: docs/decisions/records/ci/shared-pr-file-enumeration.md, docs/decisions/records/ci/verdict-write-retarget-fence.md, docs/decisions/records/release/verdict-status-check.md, docs/remote-state-inventory.md, docs/guard-inventory.md, docs/ci-cd.md, docs/decisions/README.md (regenerated).

## Closing record **Outcome:** Closed by PR #873, together with **#803** — the same defect, filed independently. #664 came out of the #649 cold review; #803 out of #778's read-then-write inventory. Fixed once: the enforced caller now fences its write on the monotonic `pull_push` count, so an `A -> B -> A` force-push spanning `pr-changed-files.sh`'s paging withholds the exemption instead of passing it. **Root cause:** The head binding is a post-hoc comparison of a value against itself, which is ABA-vulnerable by construction — this issue's own analysis was right that no tighter check *inside the script* can close it. What it under-weighted is that a caller CAN, with a monotonic key: the timeline's `pull_push` count moves by two where the sha moves by zero. **Decisions/conventions changed:** `ci.shared-pr-file-enumeration` — the script's guarantee is stated as one-way MOVEMENT DETECTION rather than a binding, in the `rule:`, the `mechanics:` and the body (the frontmatter still said "bound to BOTH the given sha", contradicting a body that had already been narrowed), and the new caller-side fence is recorded as covering the ENFORCED path only. `ci.verdict-write-retarget-fence` amended to carry both axes. **Reusable knowledge:** - This issue's third Done-when box asked for a test reproducing `A -> B -> A`. Writing it exposed the thing worth carrying: the ABA costs TWO push events, so a fence keyed on the count's VALUE rather than its MOVEMENT would withhold every exemption ever issued — every PR has a non-zero `pull_push` count, because it is created by a push. - The alias is not adversarial-only. PR #761 really went `8798a1d -> 830a407 -> 8798a1d` through ordinary force-push-and-revert. - The exposure analysis here said "two precisely-timed force-pushes bracketing a specific HTTP request". That is right for the enforced gate, and the fence now covers it — but the enumerator's contract had ALSO been read as a guarantee by the advisory hook and by the workflow's own call site, and correcting those three (plus four paraphrases) was as much of the fix as the fence was. **Verification:** Covered by PR #873's gate — 1224 passed, 2 skipped; ruff clean; decisions-validate OK; CI green on the merged head. The `A -> B -> A` reproduction is `test_a_HEAD_ABA_DURING_the_run_posts_NOTHING`, mutation-proved to redden when the fence arm is removed. **Deferred:** **#870** — the timeline walk's `null` terminator is defeatable (Gitea pages before it filters), which caps what the fence closes. Documented in the record rather than left implicit, since this issue's own complaint was a guarantee stated more broadly than the code supports. **Docs updated:** `docs/decisions/records/ci/shared-pr-file-enumeration.md`, `docs/decisions/records/ci/verdict-write-retarget-fence.md`, `docs/decisions/records/release/verdict-status-check.md`, `docs/remote-state-inventory.md`, `docs/guard-inventory.md`, `docs/ci-cd.md`, `docs/decisions/README.md` (regenerated).
timothy removed the in-progress label 2026-08-29 01:30:42 +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: low security
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#664