Skip to content

ruleset_id()'s per_page Guard Fails Open on a Non-Array gh api Response #1971

Description

@ptr727

Summary

Found via an adversarial local-review pass dispatched while fixing #1880.
repo-config/configure.sh's ruleset_id() per_page=100 guard fails open on
any non-parseable gh api response, not only the CR case #1880 fixes,
because the guard's own command substitution failure never reaches set -e.

The defect

if [ "$(jqr 'length' <<<"$out")" -eq 100 ]; then

(repo-config/configure.sh, ruleset_id(), around line 145.) When gh api
exits 0 but $out is not a parseable JSON array (a truncated or
proxy-mangled 2xx body), jqr 'length' prints nothing and
[ "" -eq 100 ] prints integer expression expected to stderr and returns
2. That is the condition of an if, and this script sets only
set -Eeuo pipefail (line 39), never shopt -s inherit_errexit, so errexit
is off inside the $(...) subshell and the failure is swallowed. The guard
silently reads false, the following ids= extraction (line 150) also comes
back empty because $out never parsed, and ruleset_id returns 0, which
the caller reads as "not found."

At the apply call site (repo-config/configure.sh, around line 232), that
makes apply_ruleset() create a duplicate ruleset by name, the exact
outcome the guard's own comment says it exists to prevent. At the check
call site (around line 412), it likewise misreports "not found" instead of
aborting on an unreadable response.

The sibling jqr 'length' call site (count="$(jqr 'length' <<<"$entries")",
around line 645) avoids this because it runs after that call's own
jq_has 'type == "array"' <<<"$entries" guard. ruleset_id()'s per_page
guard has no such type check ahead of it, and the ids= extraction that
follows has no || return 1 of its own either.

Suggested fix

Validate $out is a JSON array (jq_has 'type == "array"' <<<"$out",
matching the line 645 pattern) before either the per_page-length guard or
the ids= extraction reads it, aborting with a "could not read live
state"-style message on failure, the same way the existing API-call guard
(line 138) already aborts on a gh api exit failure.

Context

Found via an adversarial local-review pass dispatched while driving #1880's
fix. Filed separately, per the convention #1880/#1881/#1882 were filed
under, to keep that fix scoped to the one line it was assigned.

Pre-existing on develop, and orthogonal to the CR-specific defect class
#1880/#1881/#1882 fix: this guard fails open on any non-array response,
with or without a trailing CR.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    scriptA defect in hub tooling

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions