Skip to content

check_ruleset()'s Type/Enforcement Comparisons Carry the Same Windows CRLF Exposure #1234 Left Unfixed #1246

Description

@ptr727

Summary

repo-config/configure.sh's check_ruleset() has more of the same asymmetric CRLF exposure #1123/#1229/#1234 already fixed for check_settings() and for .name, on payload-file reads inside check_ruleset() itself that #1234's fix did not cover, because #1234 was scoped to .name only.

The definite defect: the parameterized-rule comparison loop silently no-ops

ptypes="$(jq -r '[.rules[] | select(has("parameters")) | .type] | .[]' "$file")"
while IFS= read -r t; do
    [ -z "$t" ] && continue
    want="$(jq -S -c --arg t "$t" "[.rules[] | select(.type==\$t) | .parameters] | first | $norm" "$file")"
    got="$(jq -S -c --arg t "$t" "[.rules[] | select(.type==\$t) | .parameters] | first | $norm" <<<"$live")"
    assert "'$rname' rule '$t' parameters match the payload" test "$got" = "$want"
done <<<"$ptypes"

(repo-config/configure.sh, around lines 312-320.) $t is read from a plain jq -r extraction of the payload file (line 312), the same unconditional-native-Windows-jq-CRLF-on-stdout exposure #1123/#1234 describe. On a CR-suffixed $t (e.g. "pull_request\r"), select(.type==$t) matches nothing on either side, since neither the payload's own .type values nor the live API's are CR-suffixed. [] | first is null, and the norm/n jq filter defined above the loop maps null to null on both branches, so want and got both become the literal string "null". test "$got" = "$want" is then "null" = "null", which is true, so assert reports the rule's parameters as matching for every parameterized rule, with no comparison of the actual parameters object ever happening. This is a stronger failure than #1234's false-FAIL: it is a silent false-PASS that would mask real drift in pull_request, required_status_checks, or copilot_code_review parameters (review-thread resolution, required-check names, Copilot review policy) on exactly the platform this whole fix chain targets.

The likely-but-less-certain defect: enforcement and rule-type-set comparisons

want_enf="$(jq -r '.enforcement' "$file")"
assert "ruleset '$rname' enforcement = $want_enf" test "$(jq -r '.enforcement' <<<"$live")" = "$want_enf"
...
if ! want_types="$(jq -r '[.rules[].type] | sort | join(",")' "$file")"; then
...
got_types="$(jq -r '[.rules[].type] | sort | join(",")' <<<"$live")"
assert "'$rname' rule set = $want_types" test "$got_types" = "$want_types"

(same function, around lines 271-285.) Both the payload-file side and the live-API side of these two comparisons are still plain jq -r, unlike check_settings(), which after PR #1229 wraps every extraction on both sides in the jqr() helper for consistency (repo-config/configure.sh lines 337, 348, 352, 355, 357, 361). Whether these two specific comparisons are actually exploitable depends on whether native Windows jq's documented CRLF-on-stdout behavior applies to every -r invocation regardless of source (file argument vs. <<< stdin), in which case both sides pick up a trailing \r and the comparison still passes correctly by symmetry, same as check_settings()'s original bug before #1123 hit only the asymmetric case. This is unverified on a real Windows host either way, consistent with #1123/#1234's own stated verification limits.

Suggested fix

Route the payload-file reads at all of these sites through the same jqr() helper #1229 introduced, matching the treatment check_settings() already gets, so every .enforcement/.type extraction (both the payload-file side and the live side, for symmetry and to match check_settings()'s established pattern) uses jqr rather than plain jq -r.

Context

Found via an adversarial local-review pass while implementing #1234 (PR fixing the .name lookup only). Filed separately to keep #1234's PR scoped to what it actually covers, per the fleet's decline-scope-expansion practice.

Unverified on a real Windows host, same as #1123/#1234.

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

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions