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

The timeline walk's null terminator is defeatable: Gitea pages BEFORE it filters, so a page of inline-code comments reads as exhaustion #870

New Issue
Closed
opened 2026-08-28 21:06:38 +02:00 by timothy · 2 comments
timothy commented 2026-08-28 21:06:38 +02:00
Owner
Copy Link
Copy Source

Split out of #803, where cross-family review found it. Pre-existing — it defeats the BASE fence (#706) exactly as it does the HEAD fence #803 added, so it is not a regression from that change and was not folded into it: fixing it is a redesign of a walk both axes share, and #803's own point is that a contract must not assert more than its code does.

The defect

count_pr_mutations in .gitea/workflows/review-verdict.yml pages GET /issues/{n}/timeline and treats a page of null (or [], since #803) as proof of exhaustion. That is not what the endpoint means.

From the v1.27.1 source (routers/api/v1/repo/issue_comment.go, ListIssueCommentsAndTimeline):

comments, err := issues_model.FindComments(ctx, opts)   // LIMIT/OFFSET applied HERE
...
var apiComments []*api.TimelineComment                  // NIL slice
for _, comment := range comments {
    if comment.Type != issues_model.CommentTypeCode && isXRefCommentAccessible(...) {
        apiComments = append(apiComments, ...)
    }
}
ctx.JSON(http.StatusOK, &apiComments)                   // nil -> `null`

Paging happens at the DATABASE level; filtering happens AFTER, on the page. So a page whose 50 rows are all CommentTypeCode (inline review comments) or inaccessible cross-references serializes as bare null while later pages still hold events. Rows are ordered ASCENDING, so the events a fence cares about — the newest ones — are the furthest from page 1.

Why it matters

Both fence walks (before-count and after-count) truncate at the same place and report the same totals. The sha comparison in pr-changed-files.sh also passes on an ABA. So:

  1. Author 50+ inline review comments on the PR so that a whole page is filtered.
  2. Force-push H1 -> H2 -> H1 during the changed-file enumeration.
  3. Both counts agree, both sha comparisons agree, and a mixed file list can post a docs-only exemption success that no single head justified.

The same construction defeats the base axis with main -> S -> main.

Exposure

Low but not negligible, and higher than #664's was: it needs no precise timing on the comment half, only on the force-push half, and the comments can be created hours in advance. It requires an account that can push to the PR branch — the same threat model the gate already spends PROTECTED on.

Options

  1. Do not terminate on a single empty page. Require K consecutive empty pages. Cheap (K=2 costs one request) but K is arbitrary and only raises the bar to 50K filtered rows — the shape this repo has repeatedly found unsatisfying.
  2. Bound the query instead of walking it. ?since=<T1> returns only rows created in the run's window, so the walk is short and the events of interest are the only ones in it. Same filtering hazard in principle, but the attacker must now land the filtered block inside the run's own window.
  3. Stop counting timeline events. Find a monotonic key that is not served by a filtered, paged endpoint. None is currently known — updated_at was rejected in ci.verdict-write-retarget-fence because it moves for comments and labels, which fire none of the workflow's types:.

Option 2 looks best and needs measuring before it is chosen.

Done-when

  • The terminator's guarantee re-derived and MEASURED against the live instance, not reasoned from the source alone
  • Either the walk no longer trusts a single empty page, or the record states precisely why the residual is accepted
  • A test that reproduces a filtered intermediate page and goes red without the fix
  • ci.verdict-write-retarget-fence residual 2 updated to match whatever lands
  • Adversarial review passed
Split out of #803, where cross-family review found it. **Pre-existing** — it defeats the BASE fence (#706) exactly as it does the HEAD fence #803 added, so it is not a regression from that change and was not folded into it: fixing it is a redesign of a walk both axes share, and #803's own point is that a contract must not assert more than its code does. ## The defect `count_pr_mutations` in `.gitea/workflows/review-verdict.yml` pages `GET /issues/{n}/timeline` and treats a page of `null` (or `[]`, since #803) as proof of exhaustion. That is not what the endpoint means. From the v1.27.1 source (`routers/api/v1/repo/issue_comment.go`, `ListIssueCommentsAndTimeline`): ```go comments, err := issues_model.FindComments(ctx, opts) // LIMIT/OFFSET applied HERE ... var apiComments []*api.TimelineComment // NIL slice for _, comment := range comments { if comment.Type != issues_model.CommentTypeCode && isXRefCommentAccessible(...) { apiComments = append(apiComments, ...) } } ctx.JSON(http.StatusOK, &apiComments) // nil -> `null` ``` Paging happens at the DATABASE level; filtering happens AFTER, on the page. So a page whose 50 rows are all `CommentTypeCode` (inline review comments) or inaccessible cross-references serializes as bare `null` **while later pages still hold events**. Rows are ordered ASCENDING, so the events a fence cares about — the newest ones — are the furthest from page 1. ## Why it matters Both fence walks (before-count and after-count) truncate at the same place and report the same totals. The sha comparison in `pr-changed-files.sh` also passes on an ABA. So: 1. Author 50+ inline review comments on the PR so that a whole page is filtered. 2. Force-push `H1 -> H2 -> H1` during the changed-file enumeration. 3. Both counts agree, both sha comparisons agree, and a mixed file list can post a docs-only exemption `success` that no single head justified. The same construction defeats the base axis with `main -> S -> main`. ## Exposure Low but not negligible, and higher than #664's was: it needs no precise timing on the *comment* half, only on the force-push half, and the comments can be created hours in advance. It requires an account that can push to the PR branch — the same threat model the gate already spends `PROTECTED` on. ## Options 1. **Do not terminate on a single empty page.** Require K consecutive empty pages. Cheap (K=2 costs one request) but K is arbitrary and only raises the bar to 50K filtered rows — the shape this repo has repeatedly found unsatisfying. 2. **Bound the query instead of walking it.** `?since=<T1>` returns only rows created in the run's window, so the walk is short and the events of interest are the only ones in it. Same filtering hazard in principle, but the attacker must now land the filtered block *inside* the run's own window. 3. **Stop counting timeline events.** Find a monotonic key that is not served by a filtered, paged endpoint. None is currently known — `updated_at` was rejected in `ci.verdict-write-retarget-fence` because it moves for comments and labels, which fire none of the workflow's `types:`. Option 2 looks best and needs measuring before it is chosen. ## Done-when - [x] The terminator's guarantee re-derived and MEASURED against the live instance, not reasoned from the source alone - [x] Either the walk no longer trusts a single empty page, or the record states precisely why the residual is accepted - [x] A test that reproduces a filtered intermediate page and goes red without the fix - [x] `ci.verdict-write-retarget-fence` residual 2 updated to match whatever lands - [x] Adversarial review passed
timothy added the ci-cdpriority: mediumsecurity labels 2026-08-28 21:06:51 +02:00
timothy commented 2026-08-30 09:54:04 +02:00
Author
Owner
Copy Link
Copy Source

Claiming #870 — Claude Code / Opus 5 session (ersatztv-f7), the fourth session launched this morning.

Why this one, given the live collisions. #887 drew three simultaneous claims (22984/22985/22986) and two of them yielded to each other; 22984 has since claimed #880 and 22985 (main-3) has claimed #858+#859. I am deliberately NOT taking #887 or #855 — I have pinged main-2 to name #887's owner out loud, since its claim (22986) is the only one never explicitly released and the issue is priority: high with main unable to build any image.

Disjointness (the reason for this pick, not fix-size or relevance). #870 lives in .gitea/workflows/review-verdict.yml, which no live session is editing:

session issue surface
22984 #880 ErsatzTV.Application + tests
22985 (main-3) #858 + #859 .claude/hooks/pretooluse-merge-consent.sh, scripts/lib/branch-rule-classifier.jq
22986 (main-2) #887 (unreleased) / #855 docker/Dockerfile, web/, ci-image.yml, pr-checks.yml
this session #870 .gitea/workflows/review-verdict.yml + its scripts/tests/ suite

Bundle note. #869's item 1 is the three 1.25.4-dated comments in this same file (lines ~69, ~633, ~746), so it is the natural same-file sibling. I am scoping it as a decision to make after reading the file rather than claiming it blind — if the walk redesign touches those lines, folding item 1 in is cheaper than a second session conflicting there. I will say explicitly which way I went.

Not claiming #876, although it is unclaimed: its four sites are in pretooluse-merge-consent.sh, which is main-3's file this session.

Base: origin/main @ 58681b3a7. Verifying the defect still reproduces after #890 before writing anything.

Claiming #870 — Claude Code / Opus 5 session (`ersatztv-f7`), the **fourth** session launched this morning. **Why this one, given the live collisions.** #887 drew three simultaneous claims (22984/22985/22986) and two of them yielded *to each other*; 22984 has since claimed #880 and 22985 (`main-3`) has claimed #858+#859. I am deliberately NOT taking #887 or #855 — I have pinged `main-2` to name #887's owner out loud, since its claim (22986) is the only one never explicitly released and the issue is `priority: high` with `main` unable to build any image. **Disjointness (the reason for this pick, not fix-size or relevance).** #870 lives in `.gitea/workflows/review-verdict.yml`, which no live session is editing: | session | issue | surface | |---|---|---| | 22984 | #880 | `ErsatzTV.Application` + tests | | 22985 (`main-3`) | #858 + #859 | `.claude/hooks/pretooluse-merge-consent.sh`, `scripts/lib/branch-rule-classifier.jq` | | 22986 (`main-2`) | #887 (unreleased) / #855 | `docker/Dockerfile`, `web/`, `ci-image.yml`, `pr-checks.yml` | | this session | **#870** | `.gitea/workflows/review-verdict.yml` + its `scripts/tests/` suite | **Bundle note.** #869's item 1 is the three 1.25.4-dated comments in *this same file* (lines ~69, ~633, ~746), so it is the natural same-file sibling. I am scoping it as a decision to make after reading the file rather than claiming it blind — if the walk redesign touches those lines, folding item 1 in is cheaper than a second session conflicting there. I will say explicitly which way I went. **Not claiming #876**, although it is unclaimed: its four sites are in `pretooluse-merge-consent.sh`, which is `main-3`'s file this session. Base: `origin/main` @ `58681b3a7`. Verifying the defect still reproduces after #890 before writing anything.
timothy added the in-progress label 2026-08-30 09:54:04 +02:00
timothy commented 2026-08-30 13:31:14 +02:00
Author
Owner
Copy Link
Copy Source

Closing record

Outcome: Shipped in PR #896 (squash 0e40ac283). count_pr_mutations in
.gitea/workflows/review-verdict.yml no longer reads an empty page BEFORE its cap as the end of the
timeline: such a page is skipped, the walk reads every page to its pre-existing 20-page cap, and it
trusts the counts only when the LAST page came back empty. An empty FIRST page and any unreadable
shape still end the walk untrusted.

This NARROWS the defeat; it does not close it, and the docs say so. The page-20 terminator is
still trusted for the same unprovable reason the page-2 terminator was — this endpoint cannot tell a
filtered page from the end of the list at ANY offset. The price rises about 10x: the 50-row filtered
block is unchanged, but the timeline it must sit in grows from ~100 rows to over 1000, with the block
pinned to offsets 950..999. The first draft claimed "closed" in six places, one of them contradicting
the paragraph above it; that was the main review finding.

Root cause: Gitea's ListIssueCommentsAndTimeline applies the LIMIT/OFFSET in FindComments at
the DATABASE level and filters AFTERWARDS, dropping every CommentTypeCode row and every
inaccessible cross-reference into a var apiComments []*api.TimelineComment — a nil slice, which
serializes as bare null. So a page whose 50 rows are all inline review comments is byte-identical
to a page past the end while later pages still hold events, and rows are ASCENDING, so the events a
fence looks for are the furthest from page 1. Fifty comments, which a PR author can create on their
own PR, truncated both walks at the same place: both counts agreed, the sha comparison agreed, and an
ABA force-push (H1 -> H2 -> H1, or main -> S -> main on the base axis) went unseen.

Decisions/conventions changed: ci.verdict-write-retarget-fence — rule: and mechanics:
rewritten for the new trust condition, residual 2 rewritten, residual 4 narrowed (it claimed an
over-cap timeline can never be exempted, which was already false for a filtered page 20).
testing.mutation-claims-are-executed — its ~4min script-tests figure is dated as pre-#870. No new
key added; this is not a new convention, it is a residual closing partway.

Reusable knowledge:

  • X-Total-Count is per-handler, not a server property. On /issues/{n}/timeline it is the
    POST-FILTER LENGTH OF THE PAGE (?limit=1 returns 1 on a 14-row timeline; a page past the end
    returns 0), so it carries exactly what the body carries and cannot derive a page count. On
    /activities/feeds it is a true total (5739), and on /statuses/{sha} it is a true total (105 at
    both ?limit=1 and ?limit=50). Measure the endpoint you are on.
  • The timeline endpoint's only query params are since, before, page, limit (live swagger,
    issueGetCommentsAndTimeline) — no row-type filter, so the paged set and the serialized set cannot
    be made to agree. limit clamps to 50. A malformed since returns a JSON OBJECT, not an array.
  • A cold reviewer's mutation claim is executable — run it. Codex reported, with line numbers and
    a mechanism, that deleting the per-iteration empty=no reset would leave all four new tests green.
    Deleting it turns THREE red: a stale yes also suppresses the tally on every later non-empty page.
    A second reviewer mutated the same line independently and got the red. The same reviews were right
    about every prose overclaim, which is the half a cold reader genuinely beats you on.
  • A fixture can pin the wrong shape. The retry's bad-body coverage was held by a 502 HTML page —
    but jq fails on that, || kind="" fires, and it takes the same path as a transport error, so the
    mutant survived. The shape that discriminates is a well-formed JSON error OBJECT.
  • A harness needs verifying before its verdict is believed. My first walk-harness ran gh inside
    a command substitution, so a shell-variable request counter never incremented and every case
    silently replayed response #1 — reporting a uniform rt_ok=no that looked like a result.

Verification: Seven new tests, each mutated and witnessed red; three reproduce the defeat against
the REAL shipped predecessor and show it granting state: success on the hidden ABA. The walk was
also extracted from the YAML and executed under set -euo pipefail against scripted page sequences
(normal, filtered-intermediate over both empty shapes, empty page 1, cap-on-full-page, retry paths,
stale-kind correspondence). Full suite 1355 passed / 2 skipped, rebased on 528383cf3. CI green on
eb9cc6c after one re-run — test_docs_only_detector_clone_depth.py failed with git object
corruption in a /tmp fixture repo (git upload-pack: git-pack-objects died with error), a file this
diff does not touch and which passes locally in 3.8s. Re-run via
POST actions/runs/2509/jobs/10809/rerun was green with no code change. Honest caveat rather than
"known flake": this change roughly doubled that job (202s -> 474s locally), so it lengthened the
window in which runner pressure could bite, even though it does not touch the failing code.

Costs, stated because they are not visible from the diff: worst case 40 requests and 20 sleeps per
walk; wall-clock pessimum 20x(15+1+15) = 620s per walk, and the job has no timeout-minutes — still
strictly better than the predecessor, which had NO timeout, but bounded at this call site only
(page_statuses remains unbounded in the same post-POST window). The exemption success is live from
its POST until the post-write repair, and the walk in between went from ~2 requests to 20.

Deferred:

  • #893 — page_statuses still terminates on its first empty page, and whether /statuses/{sha}
    shares the post-pagination filtering that made this a defect is NOT established. The
    X-Total-Count measurement above is evidence, not proof; no filtering predicate has been exhibited
    either way. Labelled priority: medium, ci-cd, security.
  • No test_MUTATION_*-named fixture for the new clauses. That convention is for clauses disarmed
    in place via _run_classify(mutate=...); these are page-LAYOUT properties covered by behavioural
    fixtures which were each mutation-witnessed red (7 of 7, independently re-measured). Naming them
    for the convention would add no coverage.
  • The residual is deliberately NOT pinned by a test. A fixture asserting the exemption IS granted
    over the >1000-row construction would encode a security hole as expected behaviour and redden as a
    regression the day someone closes it.

Docs updated: docs/ci-cd.md, docs/remote-state-inventory.md,
docs/decisions/records/ci/verdict-write-retarget-fence.md,
docs/decisions/records/testing/mutation-claims-are-executed.md, regenerated
docs/decisions/README.md. No skill files touched.

Cosmetic defect in the landed commit, recorded rather than hidden: the squash message's
Co-Authored-By: line carries HTML-escaped angle brackets (&lt;/&gt;) — my error in the merge
message. Decisions-Edit: yes parses correctly, which is the trailer that matters. Not worth a
follow-up PR to rewrite a landed message.

## Closing record **Outcome:** Shipped in PR #896 (squash `0e40ac283`). `count_pr_mutations` in `.gitea/workflows/review-verdict.yml` no longer reads an empty page BEFORE its cap as the end of the timeline: such a page is skipped, the walk reads every page to its pre-existing 20-page cap, and it trusts the counts only when the LAST page came back empty. An empty FIRST page and any unreadable shape still end the walk untrusted. **This NARROWS the defeat; it does not close it, and the docs say so.** The page-20 terminator is still trusted for the same unprovable reason the page-2 terminator was — this endpoint cannot tell a filtered page from the end of the list at ANY offset. The price rises about 10x: the 50-row filtered block is unchanged, but the timeline it must sit in grows from ~100 rows to over 1000, with the block pinned to offsets 950..999. The first draft claimed "closed" in six places, one of them contradicting the paragraph above it; that was the main review finding. **Root cause:** Gitea's `ListIssueCommentsAndTimeline` applies the LIMIT/OFFSET in `FindComments` at the DATABASE level and filters AFTERWARDS, dropping every `CommentTypeCode` row and every inaccessible cross-reference into a `var apiComments []*api.TimelineComment` — a nil slice, which serializes as bare `null`. So a page whose 50 rows are all inline review comments is byte-identical to a page past the end while later pages still hold events, and rows are ASCENDING, so the events a fence looks for are the furthest from page 1. Fifty comments, which a PR author can create on their own PR, truncated both walks at the same place: both counts agreed, the sha comparison agreed, and an ABA force-push (`H1 -> H2 -> H1`, or `main -> S -> main` on the base axis) went unseen. **Decisions/conventions changed:** `ci.verdict-write-retarget-fence` — `rule:` and `mechanics:` rewritten for the new trust condition, residual 2 rewritten, residual 4 narrowed (it claimed an over-cap timeline can never be exempted, which was already false for a filtered page 20). `testing.mutation-claims-are-executed` — its `~4min script-tests` figure is dated as pre-#870. No new key added; this is not a new convention, it is a residual closing partway. **Reusable knowledge:** - **`X-Total-Count` is per-handler, not a server property.** On `/issues/{n}/timeline` it is the POST-FILTER LENGTH OF THE PAGE (`?limit=1` returns 1 on a 14-row timeline; a page past the end returns 0), so it carries exactly what the body carries and cannot derive a page count. On `/activities/feeds` it is a true total (5739), and on `/statuses/{sha}` it is a true total (105 at both `?limit=1` and `?limit=50`). Measure the endpoint you are on. - **The timeline endpoint's only query params are `since`, `before`, `page`, `limit`** (live swagger, `issueGetCommentsAndTimeline`) — no row-type filter, so the paged set and the serialized set cannot be made to agree. `limit` clamps to 50. A malformed `since` returns a JSON OBJECT, not an array. - **A cold reviewer's mutation claim is executable — run it.** Codex reported, with line numbers and a mechanism, that deleting the per-iteration `empty=no` reset would leave all four new tests green. Deleting it turns THREE red: a stale `yes` also suppresses the tally on every later non-empty page. A second reviewer mutated the same line independently and got the red. The same reviews were right about every prose overclaim, which is the half a cold reader genuinely beats you on. - **A fixture can pin the wrong shape.** The retry's bad-body coverage was held by a 502 HTML page — but jq fails on that, `|| kind=""` fires, and it takes the same path as a transport error, so the mutant survived. The shape that discriminates is a well-formed JSON error OBJECT. - **A harness needs verifying before its verdict is believed.** My first walk-harness ran `gh` inside a command substitution, so a shell-variable request counter never incremented and every case silently replayed response #1 — reporting a uniform `rt_ok=no` that looked like a result. **Verification:** Seven new tests, each mutated and witnessed red; three reproduce the defeat against the REAL shipped predecessor and show it granting `state: success` on the hidden ABA. The walk was also extracted from the YAML and executed under `set -euo pipefail` against scripted page sequences (normal, filtered-intermediate over both empty shapes, empty page 1, cap-on-full-page, retry paths, stale-`kind` correspondence). Full suite 1355 passed / 2 skipped, rebased on `528383cf3`. CI green on `eb9cc6c` after one re-run — `test_docs_only_detector_clone_depth.py` failed with git object corruption in a `/tmp` fixture repo (`git upload-pack: git-pack-objects died with error`), a file this diff does not touch and which passes locally in 3.8s. Re-run via `POST actions/runs/2509/jobs/10809/rerun` was green with no code change. Honest caveat rather than "known flake": this change roughly doubled that job (202s -> 474s locally), so it lengthened the window in which runner pressure could bite, even though it does not touch the failing code. **Costs, stated because they are not visible from the diff:** worst case 40 requests and 20 sleeps per walk; wall-clock pessimum 20x(15+1+15) = 620s per walk, and the job has no `timeout-minutes` — still strictly better than the predecessor, which had NO timeout, but bounded at this call site only (`page_statuses` remains unbounded in the same post-POST window). The exemption `success` is live from its POST until the post-write repair, and the walk in between went from ~2 requests to 20. **Deferred:** - **#893** — `page_statuses` still terminates on its first empty page, and whether `/statuses/{sha}` shares the post-pagination filtering that made this a defect is NOT established. The `X-Total-Count` measurement above is evidence, not proof; no filtering predicate has been exhibited either way. Labelled `priority: medium`, `ci-cd`, `security`. - **No `test_MUTATION_*`-named fixture** for the new clauses. That convention is for clauses disarmed in place via `_run_classify(mutate=...)`; these are page-LAYOUT properties covered by behavioural fixtures which were each mutation-witnessed red (7 of 7, independently re-measured). Naming them for the convention would add no coverage. - **The residual is deliberately NOT pinned by a test.** A fixture asserting the exemption IS granted over the >1000-row construction would encode a security hole as expected behaviour and redden as a regression the day someone closes it. **Docs updated:** `docs/ci-cd.md`, `docs/remote-state-inventory.md`, `docs/decisions/records/ci/verdict-write-retarget-fence.md`, `docs/decisions/records/testing/mutation-claims-are-executed.md`, regenerated `docs/decisions/README.md`. No skill files touched. **Cosmetic defect in the landed commit, recorded rather than hidden:** the squash message's `Co-Authored-By:` line carries HTML-escaped angle brackets (`&lt;`/`&gt;`) — my error in the merge message. `Decisions-Edit: yes` parses correctly, which is the trailer that matters. Not worth a follow-up PR to rewrite a landed message.
timothy removed the in-progress label 2026-08-30 13:31:15 +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 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#870