fix(643): close two more fail-opens in the docs-only enumeration, found by cold review
An independent cross-family review of the jq-1.6 fix found two further ways the docs-only exemption can fire over an incomplete file list — both reachable with NO transport error, so neither had anything to do with the original bug. 1. HIGH — a path containing a newline. `chunk` flattens paths into newline-delimited text before the allow-list grep, so a filename of "safe.md\ndocs/Program.cs" splits into two lines that BOTH match the allow-list, while the real single path ends in .cs. Git permits newlines in filenames and the reviewer reproduced the bypass against this hook. Now rejected outright at the row-schema guard, on both `filename` and `previous_filename`: no docs path contains a control character, so failing closed costs nothing. 2. HIGH — a short page read as the last page. `n < 50` assumed the server's page size is the 50 we requested, but Gitea caps `limit` at the server-wide MAX_RESPONSE_ITEMS (default 50, configurable) and may return fewer. A 30-row docs page followed by a page of code completed the enumeration over a PARTIAL list. Only a validated EMPTY page may now terminate it; the page<=40 cap still fails closed, and the cost is one extra request. 3. MEDIUM — the enumeration was not bound to one head. Paging is several round-trips, so a force-push between them assembles a list belonging to no single commit: page 1 from head A plus a short docs tail from head B, with B's code page never read. The head sha is re-read after enumeration and the exemption refused if it moved. All three mutation-verified: reverting each fix reddens exactly its own test and nothing else. A positive control (short page then empty page) pins that the stricter terminator still exempts a genuinely docs-only PR, so "never terminate early" cannot be satisfied by never exempting anything. 118 passed under BOTH jq 1.8.2 and jq 1.6. The record now states the generalisable lesson: every defect here was an exhaustiveness failure in an enumeration whose completeness is load-bearing. When a security decision depends on having seen ALL of something, the termination condition must be positive and explicit, never inferred from a proxy. Refs #643, #631
This commit is contained in:
@@ -115,8 +115,19 @@ while [ "$page" -le 40 ]; do
|
||||
if [ -z "${raw//[[:space:]]/}" ]; then
|
||||
files_complete=no; break
|
||||
fi
|
||||
#
|
||||
# CR/LF in a path is REJECTED outright (ersatztv#643 review). `chunk` below flattens paths into
|
||||
# newline-delimited text, so a filename containing a newline splits into TWO lines that are each
|
||||
# matched against the allow-list separately: `"safe.md\ndocs/Program.cs"` yields `safe.md` and
|
||||
# `docs/Program.cs`, both of which pass, while the actual single path ends in `.cs`. Git permits
|
||||
# newlines in filenames, so this is reachable, and it was reproduced against this hook. Failing
|
||||
# closed on control characters is the cheap fix; no decision/docs path ever contains one.
|
||||
if ! printf '%s' "$raw" \
|
||||
| jq -e 'type == "array" and all(.[]; (.filename | type == "string" and length > 0) and (if .status == "renamed" then (.previous_filename | type == "string" and length > 0) else true end))' \
|
||||
| jq -e 'type == "array" and all(.[];
|
||||
(.filename | type == "string" and length > 0 and (test("[\\r\\n]") | not))
|
||||
and (if .status == "renamed"
|
||||
then (.previous_filename | type == "string" and length > 0 and (test("[\\r\\n]") | not))
|
||||
else true end))' \
|
||||
>/dev/null 2>&1; then
|
||||
files_complete=no; break
|
||||
fi
|
||||
@@ -127,10 +138,26 @@ while [ "$page" -le 40 ]; do
|
||||
n=$(printf '%s' "$raw" | jq -r 'length')
|
||||
chunk=$(printf '%s' "$raw" | jq -r '.[] | (.filename // empty), (.previous_filename // empty)')
|
||||
[ -n "$chunk" ] && files=$(printf '%s\n%s' "$files" "$chunk")
|
||||
if [ "$n" -lt 50 ]; then files_complete=yes; break; fi
|
||||
# Terminate ONLY on an explicitly validated EMPTY page — never on a merely SHORT one
|
||||
# (ersatztv#643 review). "Fewer than 50 rows means last page" assumes the server's page size is
|
||||
# the 50 we asked for, but Gitea caps `limit` at the server-wide `MAX_RESPONSE_ITEMS` (default 50,
|
||||
# configurable) and is free to return fewer. A 30-row page followed by a page of code would set
|
||||
# files_complete=yes over a PARTIAL list — the same fail-open, reached without any transport error.
|
||||
# Costs one extra request per enumeration; the `page <= 40` cap still fails closed.
|
||||
if [ "$n" -eq 0 ]; then files_complete=yes; break; fi
|
||||
page=$((page + 1))
|
||||
done
|
||||
files=$(printf '%s\n' "$files" | grep -v '^$' || true)
|
||||
# Bind the enumeration to ONE head (ersatztv#643 review). Paging is several round-trips; a
|
||||
# force-push between them means page 1 came from head A and page 2 from head B, so the assembled
|
||||
# list belongs to no single commit — B's code page can be skipped entirely while B's docs page
|
||||
# reads as a clean short tail. Re-read the head and refuse the exemption if it moved.
|
||||
if [ "$files_complete" = yes ]; then
|
||||
sha_after=$(printf '%s' "$(gq "repos/$owner/$repo/pulls/$pr")" | jq -r '.head.sha // ""' 2>/dev/null || true)
|
||||
if [ -z "$sha_after" ] || [ "$sha_after" != "$sha" ]; then
|
||||
files_complete=no
|
||||
fi
|
||||
fi
|
||||
if [ "$files_complete" = yes ] && [ -n "$files" ] && ! printf '%s\n' "$files" | grep -qvE '^(docs/|\.claude/|\.husky/|\.gitea/|.*\.md$)'; then
|
||||
# Docs/process-only PR: the Done-when + review-verdict gate doesn't apply — but this exemption is a
|
||||
# file-TYPE bypass, NOT the a+b+c "provably reviewed & ready" proof, so it does NOT auto-grant. It
|
||||
|
||||
@@ -62,6 +62,27 @@ a PR skips the Done-when checks, it was covered by an existing test, and that te
|
||||
on a developer Mac (jq 1.8) — only in CI, where the suite had never run. **Standing rule it leaves
|
||||
behind: never infer "empty input" from a jq exit status; check the string.**
|
||||
|
||||
An independent cross-family review of that fix then found **two further fail-opens in the same
|
||||
enumeration, both reachable with no transport error at all** (#643):
|
||||
|
||||
- **A path containing a newline.** The file list is flattened into newline-delimited text before the
|
||||
allow-list grep, so `"safe.md\ndocs/Program.cs"` splits into two lines that each pass while the
|
||||
real path ends in `.cs`. Git permits newlines in filenames; it was reproduced against the hook.
|
||||
Now rejected outright — a docs path never contains a control character.
|
||||
- **A short page read as the last page.** Gitea caps `limit` at the server-wide `MAX_RESPONSE_ITEMS`
|
||||
and may return fewer rows than asked for, so `n < 50` does not mean "end of list". Only a
|
||||
validated EMPTY page may terminate the enumeration.
|
||||
|
||||
Plus a **Medium**: paging is several round-trips, so a force-push between them yields a list
|
||||
belonging to no single commit. The head sha is now re-read after enumeration and the exemption
|
||||
refused if it moved.
|
||||
|
||||
**The generalisable lesson is about the SHAPE of this guard, not any one bug.** Every defect here
|
||||
was an *exhaustiveness* failure in an enumeration whose completeness is load-bearing: each looked
|
||||
like a complete list and wasn't. 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").
|
||||
|
||||
It is not yet a *required* status check — `main` requires only `Build & test (.NET)`,
|
||||
`EF migration integrity` and `review-verdict/h10`. It reddens the run; promoting it to required is a
|
||||
branch-protection change left deliberately separate.
|
||||
|
||||
@@ -55,7 +55,16 @@ if "/pulls/" in url and "/files" in url:
|
||||
print(json.dumps(entry)); sys.exit(0)
|
||||
|
||||
if "/pulls/" in url:
|
||||
print(json.dumps({"head": {"sha": os.environ["STUB_SHA"]}, "body": "no linked issue here"}))
|
||||
# Optional: a second head sha served from the Nth PR-object read onward, modelling a
|
||||
# force-push landing between pagination round-trips.
|
||||
shas = [os.environ["STUB_SHA"]]
|
||||
alt = state / "pr_sha_after.txt"
|
||||
ctr = state / "pr_reads.txt"
|
||||
nread = int(ctr.read_text()) if ctr.exists() else 0
|
||||
ctr.write_text(str(nread + 1))
|
||||
if alt.exists() and nread >= 1:
|
||||
shas = [alt.read_text().strip()]
|
||||
print(json.dumps({"head": {"sha": shas[0]}, "body": "no linked issue here"}))
|
||||
sys.exit(0)
|
||||
|
||||
print("{}")
|
||||
@@ -336,3 +345,54 @@ def test_transport_failure_withholds_exemption_even_on_jq16(hook_jq16):
|
||||
page 2 = transport failure, page 3 = a legitimate empty page. Pre-fix this returned True."""
|
||||
hook_jq16.set_pages(_rows([f"docs/f{i}.md" for i in range(50)]), "ERROR", [])
|
||||
assert hook_jq16.exempted() is False
|
||||
|
||||
|
||||
# --- #643 review findings: fail-opens independent of the jq version -----------------------------
|
||||
|
||||
|
||||
def test_newline_in_filename_does_not_split_into_two_passing_paths(hook):
|
||||
"""A path containing a newline must NOT be flattened into two allow-list-passing lines.
|
||||
|
||||
`chunk` renders paths as newline-delimited text, so `"safe.md\\ndocs/Program.cs"` becomes two
|
||||
lines — `safe.md` and `docs/Program.cs` — which BOTH match the docs allow-list, while the real
|
||||
single path ends in `.cs`. Git permits newlines in filenames, so this is reachable. Fail closed
|
||||
on control characters."""
|
||||
hook.set_pages([{"filename": "safe.md\ndocs/Program.cs", "status": "added"}])
|
||||
assert hook.exempted() is False
|
||||
|
||||
|
||||
def test_newline_in_previous_filename_is_also_rejected(hook):
|
||||
"""Same hole via the rename side — `previous_filename` is flattened identically."""
|
||||
hook.set_pages([{"filename": "docs/ok.md", "previous_filename": "safe.md\nErsatzTV/Program.cs",
|
||||
"status": "renamed"}])
|
||||
assert hook.exempted() is False
|
||||
|
||||
|
||||
def test_short_NONTERMINAL_page_does_not_end_the_enumeration(hook):
|
||||
""""Fewer rows than we asked for" must not be read as "last page".
|
||||
|
||||
Gitea caps `limit` at the server-wide MAX_RESPONSE_ITEMS (default 50, configurable) and may
|
||||
return fewer rows than requested. A 30-row docs page followed by a page of code would otherwise
|
||||
complete the enumeration over a PARTIAL list — the same fail-open, reached with no transport
|
||||
error at all. Only a validated EMPTY page may terminate it."""
|
||||
hook.set_pages(_rows([f"docs/f{i}.md" for i in range(30)]),
|
||||
_rows(["ErsatzTV/Program.cs"]),
|
||||
[])
|
||||
assert hook.exempted() is False
|
||||
|
||||
|
||||
def test_short_page_then_empty_page_still_exempts_a_genuinely_docs_only_pr(hook):
|
||||
"""Positive control for the change above: the stricter terminator must not break the happy path.
|
||||
|
||||
Without this, 'never terminate on a short page' could be satisfied by never exempting anything."""
|
||||
hook.set_pages(_rows([f"docs/f{i}.md" for i in range(30)]), [])
|
||||
assert hook.exempted() is True
|
||||
|
||||
|
||||
def test_head_moving_mid_enumeration_withholds_the_exemption(hook, tmp_path):
|
||||
"""Paging is several round-trips; a force-push between them means the assembled list belongs to
|
||||
no single commit. Page 1 from head A can be combined with a short docs tail from head B while
|
||||
B's code page is never read. Re-read the head and refuse if it moved."""
|
||||
(tmp_path / "state" / "pr_sha_after.txt").write_text("b" * 40)
|
||||
hook.set_pages(_rows([f"docs/f{i}.md" for i in range(30)]), [])
|
||||
assert hook.exempted() is False
|
||||
|
||||
Reference in New Issue
Block a user