From 5fd74c06892311cb9c1bc8842988d46088615b89 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 3 Sep 2026 11:34:27 -0700 Subject: [PATCH 1/2] Fix configure.sh's Ruleset Type-Loop CRLF False Pass check_ruleset()'s parameterized-rule loop extracted the rule types driving its `.type==$t` selects with a plain `jq -r`, the same unstripped-CRLF exposure #1123/#1234 already fixed for the settings comparison and the ruleset name lookup. On a native Windows jq, that extraction's `-r` raw-output mode appends a trailing carriage return to `$t`. Neither the payload's own `.type` values nor the live API's carry that CR, so `select(.type==$t)` matches nothing on either side, `[] | first` collapses to `null` on both sides, and the assert then reports every parameterized rule's parameters as matching without ever comparing the real objects. That is a silent false pass, not a false fail: real drift in a rule's parameters (review-thread resolution, required-check names, Copilot review policy) would go undetected on exactly the platform this fix chain targets. Route that extraction through the jqr() helper #1229 introduced, and do the same for the enforcement and rule-type-set comparisons in the same function for consistency with check_settings()'s established symmetric treatment, even though those two are not proven exploitable the same way (unlike $t, both sides of those two comparisons already read through plain jq -r, so if Windows jq's CRLF quirk applies symmetrically to both sides, the comparison still passes correctly). Reproduced the false pass directly: running the real check_ruleset() function under a jq shim that adds a trailing CR to -r output (the documented native-Windows behavior) reports a false "ok" on real drift (2 vs 1 required approving reviews) before this fix, and correctly fails on the same input after it. The fixed code was also confirmed to still pass when there is genuinely no drift, and to behave identically to the unfixed code under a real (non-Windows) jq, so the fix changes nothing on the platform this host can verify. The Windows-native code path itself remains unverified on this Linux host, same as #1123/#1234. Fixes #1246 Co-Authored-By: Claude Opus 5 (1M context) --- repo-config/configure.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/repo-config/configure.sh b/repo-config/configure.sh index 8ca5d66b..b01da32d 100755 --- a/repo-config/configure.sh +++ b/repo-config/configure.sh @@ -268,13 +268,13 @@ check_ruleset() { # payload-file - the live ruleset must match the committed pol fail "ruleset '$rname' - could not read live state" return fi - want_enf="$(jq -r '.enforcement' "$file")" - assert "ruleset '$rname' enforcement = $want_enf" test "$(jq -r '.enforcement' <<<"$live")" = "$want_enf" + want_enf="$(jqr '.enforcement' "$file")" + assert "ruleset '$rname' enforcement = $want_enf" test "$(jqr '.enforcement' <<<"$live")" = "$want_enf" # The live rule-type set must equal the payload's, compared in both directions. # Checking only that each payload type is present live misses a rule someone added by hand. # That is drift this script exists to catch, and it passed as clean before. local want_types got_types - if ! want_types="$(jq -r '[.rules[].type] | sort | join(",")' "$file")"; then + if ! want_types="$(jqr '[.rules[].type] | sort | join(",")' "$file")"; then fail "ruleset payload $file did not parse" return fi @@ -282,7 +282,7 @@ check_ruleset() { # payload-file - the live ruleset must match the committed pol fail "ruleset payload $file declares no rules" return fi - got_types="$(jq -r '[.rules[].type] | sort | join(",")' <<<"$live")" + got_types="$(jqr '[.rules[].type] | sort | join(",")' <<<"$live")" assert "'$rname' rule set = $want_types" test "$got_types" = "$want_types" # The bypass list is reported and never asserted, because no payload declares one. # Who may bypass a ruleset is a human decision taken in the UI, so code states what is there and judges nothing. @@ -309,7 +309,7 @@ check_ruleset() { # payload-file - the live ruleset must match the committed pol elif type == "array" then map(w(f)) | f else f end; def n: w(if type=="array" then (if length==0 then . elif (all(.[]; type=="string" or type=="number")) then sort elif (all(.[]; type=="object" and has("context"))) then sort_by(.context) else . end) else . end); n' - ptypes="$(jq -r '[.rules[] | select(has("parameters")) | .type] | .[]' "$file")" + ptypes="$(jqr '[.rules[] | select(has("parameters")) | .type] | .[]' "$file")" while IFS= read -r t; do [ -z "$t" ] && continue # shellcheck disable=SC2016 # $t is a jq --arg variable, not a shell expansion From fddb41ac42f297f2b123f70e6dcd5d32fd7798d2 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 3 Sep 2026 11:47:59 -0700 Subject: [PATCH 2/2] Guard configure.sh's Ruleset Payload Reads Against Malformed JSON check_ruleset()'s .enforcement extraction was unguarded: on a malformed ruleset payload, jqr's jq stage would exit non-zero and set -Eeuo pipefail would abort the whole configure.sh check invocation, skipping every ruleset and setting after the failing one, with only a bare "jq: parse error: ..." naming neither the file nor the ruleset. An initial guard on that one read was dead in practice: rname, read three lines earlier from the same file, was itself unguarded, and because .name and .enforcement are both plain field accesses against the same parsed document, they fail together on every JSON-shape problem a real malformed payload would hit - so the abort still happened at the earlier line before the new guard was ever reached. Guarded rname too, the actual first read of the payload, the same way want_types a few lines down already is: fail "ruleset payload $file did not parse"; return. Confirmed empirically with a malformed payload file: before this commit the harness aborts uncaught with a bare jq parse error; after, it reports the named FAIL and continues, matching the driver's own architecture (check_ruleset "$develop_ruleset"; check_ruleset "$main_ruleset"; check_settings), whose point is that one bad payload is isolated and reported rather than fatal to the other checks. This does not weaken the check - a parse failure still reports an explicit FAIL, never a match. Also routed the bypass-list extraction through jqr(), the one read in this function left on plain jq -r after the earlier CRLF fix. It only feeds a note() display line, not a comparison, but a stray trailing \r in that line is the same class of platform-specific corruption the rest of this function was just cleaned up for. Addresses review findings on #1246. Co-Authored-By: Claude Opus 5 (1M context) --- repo-config/configure.sh | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/repo-config/configure.sh b/repo-config/configure.sh index b01da32d..1acdb5c9 100755 --- a/repo-config/configure.sh +++ b/repo-config/configure.sh @@ -251,7 +251,10 @@ check_ruleset() { # payload-file - the live ruleset must match the committed pol fail "ruleset payload $file missing" return fi - rname="$(jqr '.name // empty' "$file")" + if ! rname="$(jqr '.name // empty' "$file")"; then + fail "ruleset payload $file did not parse" + return + fi if [ -z "$rname" ]; then fail "ruleset payload $file has no name" return @@ -268,7 +271,10 @@ check_ruleset() { # payload-file - the live ruleset must match the committed pol fail "ruleset '$rname' - could not read live state" return fi - want_enf="$(jqr '.enforcement' "$file")" + if ! want_enf="$(jqr '.enforcement' "$file")"; then + fail "ruleset payload $file did not parse" + return + fi assert "ruleset '$rname' enforcement = $want_enf" test "$(jqr '.enforcement' <<<"$live")" = "$want_enf" # The live rule-type set must equal the payload's, compared in both directions. # Checking only that each payload type is present live misses a rule someone added by hand. @@ -288,7 +294,7 @@ check_ruleset() { # payload-file - the live ruleset must match the committed pol # Who may bypass a ruleset is a human decision taken in the UI, so code states what is there and judges nothing. # It is surfaced on every run rather than left invisible, since it is the field that decides who the rules do not apply to. local bypass - bypass="$(jq -r '[.bypass_actors[]? | "\(.actor_type) \(.actor_id) \(.bypass_mode)"] | join("; ")' <<<"$live")" + bypass="$(jqr '[.bypass_actors[]? | "\(.actor_type) \(.actor_id) \(.bypass_mode)"] | join("; ")' <<<"$live")" note "ruleset '$rname' bypass list: ${bypass:-none} (not managed by this script)" # Every parameterized rule is compared on its whole parameters object rather than on selected fields. # Naming fields one at a time meant a payload could declare a parameter the check never read.