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

Merge-consent gate fails OPEN on jq 1.6: a transport failure mid-pagination lets the docs-only exemption fire over a PARTIAL file list #643

New Issue
Closed
opened 2026-07-26 01:40:35 +02:00 by timothy · 6 comments
timothy commented 2026-07-26 01:40:35 +02:00
Owner
Copy Link
Copy Source

Found while landing #631 (running scripts/tests/ in CI for the first time). Fixed in PR #637; filed separately because it changes the security posture of the merge gate and that history should be findable on its own rather than buried inside a "run the tests in CI" PR.

The defect

.claude/hooks/pretooluse-merge-consent.sh decides whether a PR is docs-only and may therefore skip the Done-when / review-verdict checks entirely. To do that it must enumerate the changed-file list exhaustively, paging at 50 rows.

The guard that rejects a failed page leaned on jq's exit status:

raw=$(gq ".../pulls/$pr/files?limit=50&page=$page")   # empty on any transport failure
if ! printf '%s' "$raw" | jq -e 'type == "array" and all(.[]; ...)' >/dev/null 2>&1; then
  files_complete=no; break
fi

jq -e over EMPTY input exits 4 on jq ≥ 1.7 but 0 on jq 1.6. Verified against both binaries, not inferred:

input jq 1.8.2 jq 1.6
(empty) 4 0
[] 0 0
{"message":"internal error"} 1 1
[{}] 1 1

So on jq 1.6 the failed page passes the guard, and the chain continues:

  1. n=$(... jq -r 'length') over empty input yields ""
  2. [ "$n" -lt 50 ] errors ("integer expression expected") → false as an if condition, and set -e does not fire for a condition
  3. the loop walks past the failed page to the next one
  4. that next page legitimately returns [] → files_complete=yes
  5. …set over a PARTIAL list — the exemption fires over unread pages that may be pure code

This is the same defect the guard's own comment describes ("a transport/parse failure must not look like a legitimate short final page"), reintroduced one layer down by a jq version difference.

Blast radius

  • The CI runner ships jq 1.6 (confirmed from the job log: Preflight OK: jq-1.6).
  • The hook itself runs as a Claude Code PreToolUse hook on the developer machine, which currently has jq 1.8.2 — so the live gate was not exempting PRs wrongly in practice. The exposure was latent, not active.
  • The second jq -e site (the review-verdict/h10 status read, hook line ~224) had the same shape but fell through to vstate="" → deny. Fail-closed, so never a hole — it would only have surfaced the wrong message.

Why it survived

Two compounding reasons, both worth keeping:

  1. scripts/tests/ never ran in CI (#631) — so the only place the bug is observable was never exercised.
  2. test_transport_failure_mid_pagination_withholds_the_exemption already existed and asserts exactly the right thing, but can only observe the bug where the host jq is 1.6. It passes on a developer Mac with the bug fully present. A test whose verdict depends on an unpinned host binary is not a regression test.

Fix (in PR #637)

Reject an empty body explicitly rather than inferring it from jq's exit status, at both jq -e sites. A new test pins the property on any jq by shimming only the 1.6 empty-input behaviour; mutation-verified (revert the fix → red on jq 1.8.2).

All six gq consumers in the hook were enumerated rather than just the two found: lines 81, 150, 169 and 248 already guard explicitly, so site 96 was the only genuine fail-open.

Standing rule this leaves behind: never infer "empty input" from a jq exit status — check the string. Captured in ci.script-tests-job.

Done-when

  • Both jq -e sites reject an empty body explicitly rather than via exit status
  • A regression test pins the behaviour independently of the host jq version, mutation-verified
  • Every other gq consumer in the hook audited for the same shape
  • Fix implemented and independently reviewed; carried by PR #637 (the merge itself is that PR's event, not a precondition of it — de-circularised)
  • Adversarial review passed
Found while landing #631 (running `scripts/tests/` in CI for the first time). **Fixed in PR #637**; filed separately because it changes the security posture of the merge gate and that history should be findable on its own rather than buried inside a "run the tests in CI" PR. ## The defect `.claude/hooks/pretooluse-merge-consent.sh` decides whether a PR is *docs-only* and may therefore **skip the Done-when / review-verdict checks entirely**. To do that it must enumerate the changed-file list exhaustively, paging at 50 rows. The guard that rejects a failed page leaned on jq's exit status: ```bash raw=$(gq ".../pulls/$pr/files?limit=50&page=$page") # empty on any transport failure if ! printf '%s' "$raw" | jq -e 'type == "array" and all(.[]; ...)' >/dev/null 2>&1; then files_complete=no; break fi ``` **`jq -e` over EMPTY input exits 4 on jq ≥ 1.7 but 0 on jq 1.6.** Verified against both binaries, not inferred: | input | jq 1.8.2 | jq 1.6 | |---|---|---| | *(empty)* | **4** | **0** | | `[]` | 0 | 0 | | `{"message":"internal error"}` | 1 | 1 | | `[{}]` | 1 | 1 | So on jq 1.6 the failed page **passes** the guard, and the chain continues: 1. `n=$(... jq -r 'length')` over empty input yields `""` 2. `[ "$n" -lt 50 ]` errors ("integer expression expected") → false as an `if` condition, and `set -e` does not fire for a condition 3. the loop walks **past** the failed page to the next one 4. that next page legitimately returns `[]` → `files_complete=yes` 5. …set over a **PARTIAL** list — the exemption fires over unread pages that may be pure code This is the same defect the guard's own comment describes ("a transport/parse failure must not look like a legitimate short final page"), reintroduced one layer down by a jq version difference. ## Blast radius - The **CI runner ships jq 1.6** (confirmed from the job log: `Preflight OK: jq-1.6`). - The hook itself runs as a Claude Code PreToolUse hook on the developer machine, which currently has jq 1.8.2 — so the live gate was *not* exempting PRs wrongly in practice. The exposure was latent, not active. - The second `jq -e` site (the `review-verdict/h10` status read, hook line ~224) had the same shape but fell through to `vstate=""` → **deny**. Fail-closed, so never a hole — it would only have surfaced the wrong message. ## Why it survived Two compounding reasons, both worth keeping: 1. **`scripts/tests/` never ran in CI** (#631) — so the only place the bug is observable was never exercised. 2. `test_transport_failure_mid_pagination_withholds_the_exemption` **already existed and asserts exactly the right thing**, but can only observe the bug where the host jq is 1.6. It passes on a developer Mac with the bug fully present. A test whose verdict depends on an unpinned host binary is not a regression test. ## Fix (in PR #637) Reject an empty body **explicitly** rather than inferring it from jq's exit status, at both `jq -e` sites. A new test pins the property on *any* jq by shimming only the 1.6 empty-input behaviour; mutation-verified (revert the fix → red on jq 1.8.2). All six `gq` consumers in the hook were enumerated rather than just the two found: lines 81, 150, 169 and 248 already guard explicitly, so site 96 was the only genuine fail-open. **Standing rule this leaves behind: never infer "empty input" from a jq exit status — check the string.** Captured in `ci.script-tests-job`. ## Done-when - [x] Both `jq -e` sites reject an empty body explicitly rather than via exit status - [x] A regression test pins the behaviour independently of the host jq version, mutation-verified - [x] Every other `gq` consumer in the hook audited for the same shape - [x] Fix implemented and independently reviewed; carried by PR #637 (the merge itself is that PR's event, not a precondition of it — de-circularised) - [x] Adversarial review passed
timothy added the bugci-cdpriority: highsecurity labels 2026-07-26 01:40:35 +02:00
timothy added the in-progress label 2026-07-26 10:52:32 +02:00
timothy commented 2026-07-26 10:52:33 +02:00
Author
Owner
Copy Link
Copy Source

Claiming — picking this up together with #631, since both are resolved by PR #637. That PR is green on every check except the (correctly non-exempt) review-verdict/h10; its branch is stale against main (which has since gained #629's verdict-classifier extraction to the same file), so this session rebases it, re-verifies the suite, runs the adversarial review, and lands it.

Claiming — picking this up together with #631, since both are resolved by PR #637. That PR is green on every check except the (correctly non-exempt) `review-verdict/h10`; its branch is stale against `main` (which has since gained #629's verdict-classifier extraction to the same file), so this session rebases it, re-verifies the suite, runs the adversarial review, and lands it.
timothy commented 2026-07-26 10:56:00 +02:00
Author
Owner
Copy Link
Copy Source

Releasing this claim — redirected to other work before landing anything. No changes were pushed; PR #637 is untouched and still carries the fix.

One finding worth keeping for whoever picks this up: PR #637's branch is stale against main, which has since gained #629's verdict-classifier extraction to the same file (.claude/hooks/pretooluse-merge-consent.sh). A diff of the PR head against main therefore looks like it reverts #629 — it does not; that is an artifact of the stale base. I rebased it locally (clean, no conflicts) and re-verified: catalog --check OK, decisions_validate OK, 180 passed, and the new regression test is mutation-verified (deleting the site-1 empty-body guard reddens exactly test_transport_failure_withholds_exemption_even_on_jq16 on jq 1.8.2, i.e. host-independently). The rebase was not pushed.

So the remaining work is: rebase the PR branch, push, run the adversarial review, post the verdict via scripts/post-review-verdict.sh, and merge.

Releasing this claim — redirected to other work before landing anything. **No changes were pushed**; PR #637 is untouched and still carries the fix. One finding worth keeping for whoever picks this up: PR #637's branch is **stale** against `main`, which has since gained #629's verdict-classifier extraction to the same file (`.claude/hooks/pretooluse-merge-consent.sh`). A diff of the PR head against `main` therefore *looks* like it reverts #629 — it does not; that is an artifact of the stale base. I rebased it locally (clean, no conflicts) and re-verified: catalog `--check` OK, `decisions_validate` OK, **180 passed**, and the new regression test is mutation-verified (deleting the site-1 empty-body guard reddens exactly `test_transport_failure_withholds_exemption_even_on_jq16` on jq 1.8.2, i.e. host-independently). The rebase was not pushed. So the remaining work is: rebase the PR branch, push, run the adversarial review, post the verdict via `scripts/post-review-verdict.sh`, and merge.
timothy removed the in-progress label 2026-07-26 10:56:12 +02:00
timothy added the in-progress label 2026-07-26 10:59:17 +02:00
timothy commented 2026-07-26 10:59:20 +02:00
Author
Owner
Copy Link
Copy Source

Claiming — actively being worked in this session; the fix is already carried by PR #637 (which also closes #631). Labelling in-progress retroactively: this issue was split out of #631 mid-session and I filed it without claiming it, which left it briefly pickable by another session.

Closes when #637 merges.

Claiming — actively being worked in this session; the fix is already carried by PR #637 (which also closes #631). Labelling `in-progress` retroactively: this issue was split out of #631 mid-session and I filed it without claiming it, which left it briefly pickable by another session. Closes when #637 merges.
timothy commented 2026-07-26 11:23:52 +02:00
Author
Owner
Copy Link
Copy Source

Correction: severity was over-stated, and the first fix was incomplete

Severity. The issue body frames this as the gate "failing open". A cold re-review pointed out the calibration I got wrong: the docs-only exemption ends in decide allow "" — a passthrough to the normal permission prompt, not an auto-grant. Only the satisfied a+b+c path calls grant. So every bypass in this issue downgrades a mechanical deny/ask into a human prompt; none of them can produce a silent self-merge. That is still a real weakening — the gate's Done-when/verdict analysis is skipped entirely — but it is not the "unreviewed code merges itself" scenario the original framing implied. Recorded in ci.script-tests-job.

The first fix was incomplete. Re-review of the fix commit found two further defects, both reproduced:

  1. The validation domain didn't match the consumption domain. chunk emits (.previous_filename // empty) for every row regardless of status, but the guard validated that field only when .status == "renamed". A row marked modified — or Gitea's distinct copied — carrying a newline in previous_filename was still exempted. Verified EXEMPT against the previous commit for both statuses.

  2. The test covering that side was vacuous. Its payload's second segment was ErsatzTV/Program.cs, which the allow-list rejects on its own merits, so the test passed with the newline guard entirely removed. It asserted the outcome without ever exercising the mechanism — which is exactly why defect 1 went unnoticed, and is the same filter-hides-the-defect trap this guard is about.

Also hardened: a .. path component (docs/../ErsatzTV/Program.cs matched the anchored ^docs/), plus a positive control so the tightened row schema can't be satisfied by never exempting anything.

Two rules worth carrying forward, beyond these bugs:

  • Validate every field the extraction consumes, not the fields it is semantically supposed to contain.
  • A test whose payload fails for an unrelated reason asserts nothing — when testing a rejection, the payload must otherwise pass.

All mutation-verified; 123 tests green under both jq 1.8.2 and jq 1.6. Fix commits 6da45ab8 and 30263a66 on PR #637.

## Correction: severity was over-stated, and the first fix was incomplete **Severity.** The issue body frames this as the gate "failing open". A cold re-review pointed out the calibration I got wrong: the docs-only exemption ends in `decide allow ""` — a **passthrough to the normal permission prompt**, not an auto-grant. Only the satisfied a+b+c path calls `grant`. So every bypass in this issue downgrades a *mechanical* deny/ask into a *human* prompt; **none of them can produce a silent self-merge.** That is still a real weakening — the gate's Done-when/verdict analysis is skipped entirely — but it is not the "unreviewed code merges itself" scenario the original framing implied. Recorded in `ci.script-tests-job`. **The first fix was incomplete.** Re-review of the fix commit found two further defects, both reproduced: 1. **The validation domain didn't match the consumption domain.** `chunk` emits `(.previous_filename // empty)` for **every** row regardless of status, but the guard validated that field only when `.status == "renamed"`. A row marked `modified` — or Gitea's distinct `copied` — carrying a newline in `previous_filename` was still exempted. Verified EXEMPT against the previous commit for both statuses. 2. **The test covering that side was vacuous.** Its payload's second segment was `ErsatzTV/Program.cs`, which the allow-list rejects on its own merits, so the test passed with the newline guard *entirely removed*. It asserted the outcome without ever exercising the mechanism — which is exactly why defect 1 went unnoticed, and is the same filter-hides-the-defect trap this guard is about. Also hardened: a `..` path component (`docs/../ErsatzTV/Program.cs` matched the anchored `^docs/`), plus a positive control so the tightened row schema can't be satisfied by never exempting anything. **Two rules worth carrying forward, beyond these bugs:** - Validate every field the extraction **consumes**, not the fields it is semantically *supposed* to contain. - A test whose payload fails for an unrelated reason asserts nothing — when testing a rejection, the payload must otherwise **pass**. All mutation-verified; 123 tests green under both jq 1.8.2 and jq 1.6. Fix commits `6da45ab8` and `30263a66` on PR #637.
timothy commented 2026-07-26 13:58:21 +02:00
Author
Owner
Copy Link
Copy Source

Closing record

Outcome: Fixed in PR #637 (merged). Five defects closed in the docs-only exemption path of .claude/hooks/pretooluse-merge-consent.sh, four of them found by review rounds after the original fix.

Root cause: every one was an exhaustiveness failure in an enumeration whose completeness is load-bearing — the exemption is only safe if the changed-file list is complete, and each defect made an incomplete list look complete:

  1. jq -e over empty input exits 4 on jq ≥1.7 but 0 on jq 1.6, so a transport failure passed the guard and the loop walked past the failed page.
  2. A newline in a path split into two lines that each passed the allow-list, while the real path ended .cs.
  3. A short page read as the last page — Gitea caps limit at MAX_RESPONSE_ITEMS and may return fewer rows than asked.
  4. The enumeration was not bound to one head, so a force-push mid-pagination assembled a list belonging to no single commit.
  5. previous_filename was consumed on every row but validated only on renamed rows, and an array-valued .status slipped the closed allow-list (jq's index does subsequence matching on arrays).

Severity — corrected from the original filing. I first framed this as the gate "failing open". It is not: the exemption ends in decide allow "", a passthrough to the normal permission prompt, not an auto-grant. Every bypass downgrades a mechanical deny/ask to a human prompt; none can silently self-merge. A real weakening, worth fixing, but not what my first write-up implied.

Reusable knowledge:

  • Validate every field the extraction CONSUMES, not the fields it is semantically supposed to contain.
  • A test whose payload fails for an unrelated reason asserts nothing. The test covering the previous_filename side used ErsatzTV/Program.cs, which the allow-list rejects anyway — so it passed with the guard entirely removed, which is precisely why defect 5 survived the first fix.
  • When a security decision depends on having seen ALL of something, the termination condition must be positive and explicit ("the server said empty"), never inferred from a proxy ("fewer than we asked for", "jq didn't complain").
  • A security check that rejects everything looks identical to a working one from the failing side — my first .status allow-list gated all input because index(.status) evaluates .status against the array; only a positive control caught it.

Verification: every fix mutation-verified — reverting each reddens exactly its own test and nothing else. Positive controls (legitimate docs-only PR, docs→docs rename, short-page-then-empty-page) prevent the guard from being satisfied by never exempting anything. 216 tests green under jq 1.8.2 and jq 1.6. Four cold review rounds; the last two found nothing exploitable against real Gitea output.

Deferred: two theoretical narrowings left unfixed and stated rather than papered over — a literal file named .md matches .*\.md$, and docs/../x.cs is now rejected but only because .. is explicitly banned. Both need malformed 2xx output that real Gitea does not produce.

Docs updated: docs/decisions/records/ci/script-tests-job.md carries the generalised lesson and all three jq rules.

## Closing record **Outcome:** Fixed in PR #637 (merged). Five defects closed in the docs-only exemption path of `.claude/hooks/pretooluse-merge-consent.sh`, four of them found by review rounds *after* the original fix. **Root cause:** every one was an **exhaustiveness failure in an enumeration whose completeness is load-bearing** — the exemption is only safe if the changed-file list is complete, and each defect made an incomplete list look complete: 1. `jq -e` over empty input exits 4 on jq ≥1.7 but **0 on jq 1.6**, so a transport failure passed the guard and the loop walked past the failed page. 2. A **newline in a path** split into two lines that each passed the allow-list, while the real path ended `.cs`. 3. A **short page read as the last page** — Gitea caps `limit` at `MAX_RESPONSE_ITEMS` and may return fewer rows than asked. 4. The enumeration was **not bound to one head**, so a force-push mid-pagination assembled a list belonging to no single commit. 5. `previous_filename` was **consumed on every row but validated only on `renamed` rows**, and an array-valued `.status` slipped the closed allow-list (jq's `index` does subsequence matching on arrays). **Severity — corrected from the original filing.** I first framed this as the gate "failing open". It is not: the exemption ends in `decide allow ""`, a **passthrough to the normal permission prompt**, not an auto-grant. Every bypass downgrades a *mechanical* deny/ask to a *human* prompt; none can silently self-merge. A real weakening, worth fixing, but not what my first write-up implied. **Reusable knowledge:** - **Validate every field the extraction CONSUMES**, not the fields it is semantically supposed to contain. - **A test whose payload fails for an unrelated reason asserts nothing.** The test covering the `previous_filename` side used `ErsatzTV/Program.cs`, which the allow-list rejects anyway — so it passed with the guard entirely removed, which is precisely why defect 5 survived the first fix. - **When a security decision depends on having seen ALL of something, the termination condition must be positive and explicit** ("the server said empty"), never inferred from a proxy ("fewer than we asked for", "jq didn't complain"). - A security check that rejects *everything* looks identical to a working one from the failing side — my first `.status` allow-list gated all input because `index(.status)` evaluates `.status` against the array; only a positive control caught it. **Verification:** every fix mutation-verified — reverting each reddens exactly its own test and nothing else. Positive controls (legitimate docs-only PR, docs→docs rename, short-page-then-empty-page) prevent the guard from being satisfied by never exempting anything. 216 tests green under jq 1.8.2 **and** jq 1.6. Four cold review rounds; the last two found nothing exploitable against real Gitea output. **Deferred:** two theoretical narrowings left unfixed and stated rather than papered over — a literal file named `.md` matches `.*\.md$`, and `docs/../x.cs` is now rejected but only because `..` is explicitly banned. Both need malformed 2xx output that real Gitea does not produce. **Docs updated:** `docs/decisions/records/ci/script-tests-job.md` carries the generalised lesson and all three jq rules.
timothy removed the in-progress label 2026-07-26 13:59:25 +02:00
timothy commented 2026-07-26 20:22:02 +02:00
Author
Owner
Copy Link
Copy Source

Follow-ups now tracked as real issues rather than prose in a closed record: 649

Follow-ups now tracked as real issues rather than prose in a closed record: 649
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 bug ci-cd priority: high 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#643