forked from heavy-duty/ceremony
fix(forge): validate the completeness bound itself, on every page
@codex-reviewer-andresmgsl's three findings (#4712), each a route by which
an unprovable read could still be reported as a whole one — the guard
leaking the failure class it exists to stop.
1. x-total-count was never validated. `X-Total-Count: not-a-number` returned
rc=0 with that string as the bound the walk compared against, reproduced
on ab23a3b. Now required to be a canonical non-negative integer.
2. The total was read once. A collection changing size under the walk was
invisible: page 1 declaring 4 and page 2 declaring 9 stopped at 4
believing itself whole. Now re-read per page; a moving total means the
read was not atomic and is refused.
3. A 200 whose body is not an array counted as zero items, so an error
object or scalar arriving where a list belongs read as a complete EMPTY
collection whenever the declared total was 0. Now refused, quoting the
body. A genuinely empty array is still fine — covered.
Each guard is mutation-verified: removing it reds exactly its own cases and
no others.
Refs #188
This commit is contained in:
parent
87b088114a
commit
66e20f12f0
2 changed files with 99 additions and 6 deletions
|
|
@ -118,11 +118,36 @@ forge_api() {
|
||||||
fi
|
fi
|
||||||
forgejo_http_ok "$hdr" "$endpoint" || return 1
|
forgejo_http_ok "$hdr" "$endpoint" || return 1
|
||||||
|
|
||||||
|
# Re-read on EVERY page, not once (#4712). A board that changes size
|
||||||
|
# under the walk was invisible: page 1 declaring 4 and page 2 declaring
|
||||||
|
# 9 stopped at 4 believing itself whole. A moving total means the read
|
||||||
|
# cannot have been atomic, so it is refused rather than reconciled.
|
||||||
|
local page_total
|
||||||
|
page_total="$(forgejo_total_count "$hdr")" || return 1
|
||||||
if [ -z "$total" ]; then
|
if [ -z "$total" ]; then
|
||||||
total="$(forgejo_total_count "$hdr")" || return 1
|
total="$page_total"
|
||||||
|
elif [ "$page_total" != "$total" ]; then
|
||||||
|
cat >&2 <<EOF
|
||||||
|
forge_api: the declared total for '$endpoint' changed between pages — $total then $page_total (#188).
|
||||||
|
The collection moved under the walk, so no page set can be proven whole.
|
||||||
|
Refusing rather than reconciling a board that is already out of date.
|
||||||
|
EOF
|
||||||
|
return 1
|
||||||
fi
|
fi
|
||||||
|
|
||||||
pagejson="$(cat "$body")"
|
pagejson="$(cat "$body")"
|
||||||
n="$(jq 'if type == "array" then length else 0 end' <<<"$pagejson")"
|
# A 200 whose body is not a collection counted as zero items (#4712),
|
||||||
|
# so an error object or a scalar arriving where a list belongs read as
|
||||||
|
# a complete EMPTY collection whenever the declared total was 0.
|
||||||
|
if [ "$(jq -r 'type' <<<"$pagejson" 2>/dev/null)" != array ]; then
|
||||||
|
cat >&2 <<EOF
|
||||||
|
forge_api: '$endpoint' did not return a collection (#188).
|
||||||
|
Expected a JSON array; got: $(head -c 200 <<<"$pagejson")
|
||||||
|
Refusing: a body this shim cannot count must not be counted as empty.
|
||||||
|
EOF
|
||||||
|
return 1
|
||||||
|
fi
|
||||||
|
n="$(jq 'length' <<<"$pagejson")"
|
||||||
[ "$n" -gt 0 ] || break
|
[ "$n" -gt 0 ] || break
|
||||||
all="$(jq -s '.[0] + .[1]' <<<"$all"$'\n'"$pagejson")"
|
all="$(jq -s '.[0] + .[1]' <<<"$all"$'\n'"$pagejson")"
|
||||||
got=$((got + n))
|
got=$((got + n))
|
||||||
|
|
@ -158,6 +183,19 @@ forge_api: this forge did not send x-total-count — cannot prove the gather is
|
||||||
EOF
|
EOF
|
||||||
return 1
|
return 1
|
||||||
fi
|
fi
|
||||||
|
# Validate before it reaches arithmetic (#4712). `X-Total-Count:
|
||||||
|
# not-a-number` used to sail through and become the bound the walk was
|
||||||
|
# compared against — a guard whose own input was never checked.
|
||||||
|
case "$total" in
|
||||||
|
'' | *[!0-9]*)
|
||||||
|
cat >&2 <<EOF
|
||||||
|
forge_api: x-total-count is not a non-negative integer: '$total' (#188).
|
||||||
|
Refusing: the completeness bound must be a number, or the assert that
|
||||||
|
uses it proves nothing.
|
||||||
|
EOF
|
||||||
|
return 1
|
||||||
|
;;
|
||||||
|
esac
|
||||||
printf '%s\n' "$total"
|
printf '%s\n' "$total"
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -72,9 +72,12 @@ check "stripping the only parameter leaves a clean query" 0 "" \
|
||||||
# curl is stubbed as a function so these are hermetic. Each case writes the
|
# curl is stubbed as a function so these are hermetic. Each case writes the
|
||||||
# headers and body a real Forgejo would.
|
# headers and body a real Forgejo would.
|
||||||
|
|
||||||
# fake_forge <total> <pages…> — install a curl stub serving <pages> as
|
# fake_forge <total-spec> <pages…> — install a curl stub serving <pages> as
|
||||||
# successive page bodies, declaring <total> in x-total-count. An empty
|
# successive page bodies, declaring <total-spec> in x-total-count. An empty
|
||||||
# string for <total> omits the header entirely (@kimi's #4699 case).
|
# string omits the header entirely (@kimi's #4699 case). A comma-separated
|
||||||
|
# spec declares a DIFFERENT total per page ("4,9"), which is
|
||||||
|
# @codex-reviewer-andresmgsl's changing-between-pages case (#4700 / #4712):
|
||||||
|
# a server whose count moves under the walk cannot have been read whole.
|
||||||
fake_forge() {
|
fake_forge() {
|
||||||
FAKE_TOTAL="$1"; shift
|
FAKE_TOTAL="$1"; shift
|
||||||
FAKE_PAGES=("$@")
|
FAKE_PAGES=("$@")
|
||||||
|
|
@ -94,9 +97,16 @@ fake_forge() {
|
||||||
done
|
done
|
||||||
local page=1
|
local page=1
|
||||||
case "$url" in *page=*) page="${url##*page=}"; page="${page%%&*}" ;; esac
|
case "$url" in *page=*) page="${url##*page=}"; page="${page%%&*}" ;; esac
|
||||||
|
local total="$FAKE_TOTAL"
|
||||||
|
case "$FAKE_TOTAL" in
|
||||||
|
*,*)
|
||||||
|
total="$(printf '%s' "$FAKE_TOTAL" | cut -d, -f"$page")"
|
||||||
|
[ -n "$total" ] || total="$(printf '%s' "$FAKE_TOTAL" | cut -d, -f1)"
|
||||||
|
;;
|
||||||
|
esac
|
||||||
{
|
{
|
||||||
printf 'HTTP/1.1 200 OK\r\n'
|
printf 'HTTP/1.1 200 OK\r\n'
|
||||||
[ -n "$FAKE_TOTAL" ] && printf 'X-Total-Count: %s\r\n' "$FAKE_TOTAL"
|
[ -n "$total" ] && printf 'X-Total-Count: %s\r\n' "$total"
|
||||||
printf '\r\n'
|
printf '\r\n'
|
||||||
} >"$hdr"
|
} >"$hdr"
|
||||||
if [ "$page" -le "${#FAKE_PAGES[@]}" ]; then
|
if [ "$page" -le "${#FAKE_PAGES[@]}" ]; then
|
||||||
|
|
@ -141,6 +151,51 @@ check "a missing x-total-count refuses" 1 "did not send x-total-count" \
|
||||||
check "...and says why it cannot prove completeness" 1 "cannot prove the gather is complete" \
|
check "...and says why it cannot prove completeness" 1 "cannot prove the gather is complete" \
|
||||||
forge_api --paginate 'repos/o/r/issues' --jq '.[].number'
|
forge_api --paginate 'repos/o/r/issues' --jq '.[].number'
|
||||||
|
|
||||||
|
# @codex-reviewer-andresmgsl's #4712 findings. Each one is a route by which
|
||||||
|
# an unprovable read could still have been reported as a whole one — the
|
||||||
|
# guard leaking the failure class it was built to stop, which is why they
|
||||||
|
# are refusals rather than warnings.
|
||||||
|
|
||||||
|
# A total that is not a number went straight into arithmetic. Reproduced on
|
||||||
|
# ab23a3b: `X-Total-Count: not-a-number` returned rc=0 with that string as
|
||||||
|
# the total.
|
||||||
|
fake_forge 'not-a-number' '[{"number":1}]'
|
||||||
|
check "a non-numeric total is refused" 1 "not a non-negative integer" \
|
||||||
|
forge_api --paginate 'repos/o/r/issues' --jq '.[].number'
|
||||||
|
check "...and the refusal quotes what arrived" 1 "not-a-number" \
|
||||||
|
forge_api --paginate 'repos/o/r/issues' --jq '.[].number'
|
||||||
|
fake_forge '12x' '[{"number":1}]'
|
||||||
|
check "a partly-numeric total is refused" 1 "not a non-negative integer" \
|
||||||
|
forge_api --paginate 'repos/o/r/issues' --jq '.[].number'
|
||||||
|
fake_forge '-3' '[{"number":1}]'
|
||||||
|
check "a negative total is refused" 1 "not a non-negative integer" \
|
||||||
|
forge_api --paginate 'repos/o/r/issues' --jq '.[].number'
|
||||||
|
|
||||||
|
# A total that MOVES under the walk. The loop read it once, so a board
|
||||||
|
# changing size mid-gather was invisible: page 1 said 4, page 2 said 9, and
|
||||||
|
# the walk stopped at 4 believing itself complete.
|
||||||
|
fake_forge '4,9' '[{"number":1},{"number":2}]' '[{"number":3},{"number":4}]'
|
||||||
|
check "a total that changes between pages is refused" 1 "changed between pages" \
|
||||||
|
forge_api --paginate 'repos/o/r/issues' --jq '.[].number'
|
||||||
|
check "...and the refusal names both totals" 1 "4" \
|
||||||
|
forge_api --paginate 'repos/o/r/issues' --jq '.[].number'
|
||||||
|
|
||||||
|
# A 200 whose body is not a collection. `length` on a non-array counted 0,
|
||||||
|
# so an object or a scalar arriving where a list belongs read as a complete
|
||||||
|
# EMPTY collection when the declared total was 0 — silence dressed as a
|
||||||
|
# clean sweep.
|
||||||
|
fake_forge 0 '{"message":"Not found"}'
|
||||||
|
check "a non-array body is refused" 1 "did not return a collection" \
|
||||||
|
forge_api --paginate 'repos/o/r/issues' --jq '.[].number'
|
||||||
|
fake_forge 0 '"a string"'
|
||||||
|
check "a scalar body is refused" 1 "did not return a collection" \
|
||||||
|
forge_api --paginate 'repos/o/r/issues' --jq '.[].number'
|
||||||
|
# A genuinely empty collection is still fine — the refusal must not fire on
|
||||||
|
# a repo that legitimately has nothing.
|
||||||
|
fake_forge 0 '[]'
|
||||||
|
check "an empty collection is not an error" 0 "" \
|
||||||
|
forge_api --paginate 'repos/o/r/issues' --jq '.[].number'
|
||||||
|
|
||||||
# --- HTTP failures are named, not swallowed -----------------------------
|
# --- HTTP failures are named, not swallowed -----------------------------
|
||||||
# gh exits non-zero on an HTTP error; curl does not without -f, and -f
|
# gh exits non-zero on an HTTP error; curl does not without -f, and -f
|
||||||
# discards the body that explains why. So the status is read explicitly.
|
# discards the body that explains why. So the status is read explicitly.
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue