From ff5ed799c8b8e22526167771de2cd37af9281313 Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Tue, 9 Jun 2026 04:48:59 +0000 Subject: [PATCH 1/3] =?UTF-8?q?feat:=20implement=20issue=20#252=20?= =?UTF-8?q?=E2=80=94=20Compliance:=20secret=5Fscanning=5Fnon=5Fprovider=5F?= =?UTF-8?q?patterns?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- scripts/compliance-remediate.sh | 36 +++++ .../compliance-remediate/helpers/setup.bash | 46 +++++++ .../security-analysis.bats | 126 ++++++++++++++++++ test/scripts/compliance-remediate/stubs/gh | 40 ++++++ 4 files changed, 248 insertions(+) create mode 100644 test/scripts/compliance-remediate/helpers/setup.bash create mode 100644 test/scripts/compliance-remediate/security-analysis.bats create mode 100644 test/scripts/compliance-remediate/stubs/gh diff --git a/scripts/compliance-remediate.sh b/scripts/compliance-remediate.sh index d5058e454..4872f7971 100644 --- a/scripts/compliance-remediate.sh +++ b/scripts/compliance-remediate.sh @@ -145,6 +145,39 @@ remediate_setting() { fi } +# --------------------------------------------------------------------------- +# Direct API: nested security_and_analysis settings +# --------------------------------------------------------------------------- +# The security_and_analysis settings live under a nested object on the repo +# resource, so they can't be patched via `gh api -F key=value` (that only +# sends flat form fields). Build the nested JSON and PATCH via stdin. +remediate_security_analysis_setting() { + local repo="$1" setting="$2" expected_status="$3" check="$4" + + if [ "$DRY_RUN" = "true" ]; then + skip "[DRY] Would PATCH $ORG/$repo: security_and_analysis.$setting.status=$expected_status" + report_direct "$repo" "$check" \ + "DRY: would PATCH \`security_and_analysis.$setting.status=$expected_status\`" + return 0 + fi + + local payload + payload=$(jq -n \ + --arg key "$setting" \ + --arg status "$expected_status" \ + '{security_and_analysis: {($key): {status: $status}}}') + + if printf '%s' "$payload" | gh api -X PATCH "repos/$ORG/$repo" --input - > /dev/null 2>&1; then + ok "Fixed $ORG/$repo: security_and_analysis.$setting.status=$expected_status" + report_direct "$repo" "$check" \ + "Set \`security_and_analysis.$setting.status=$expected_status\`" + else + err "Failed to patch $ORG/$repo: security_and_analysis.$setting.status=$expected_status" + report_fail "$repo" "$check" \ + "PATCH security_and_analysis.$setting.status=$expected_status failed (token may lack repo-admin permission)" + fi +} + # --------------------------------------------------------------------------- # Direct API: disable check-suite auto-trigger for problematic apps # --------------------------------------------------------------------------- @@ -685,6 +718,9 @@ remediate_finding() { report_skip "$repo" "$check" \ "Dependabot config updates require human review — fix using standards/dependabot-policy.md" ;; + push-protection/secret_scanning_non_provider_patterns) + remediate_security_analysis_setting "$repo" "secret_scanning_non_provider_patterns" "enabled" "$check" + ;; push-protection/*) report_skip "$repo" "$check" \ "Push protection settings require security_and_analysis API scope — run scripts/apply-repo-settings.sh" diff --git a/test/scripts/compliance-remediate/helpers/setup.bash b/test/scripts/compliance-remediate/helpers/setup.bash new file mode 100644 index 000000000..ae8ac0656 --- /dev/null +++ b/test/scripts/compliance-remediate/helpers/setup.bash @@ -0,0 +1,46 @@ +#!/usr/bin/env bash +# Common test helpers for compliance-remediate bats suites. + +TT_REPO_ROOT="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")/../../../.." && pwd)" +export TT_REPO_ROOT + +TT_SCRIPT="${TT_REPO_ROOT}/scripts/compliance-remediate.sh" +export TT_SCRIPT + +TT_STUBS_DIR="${TT_REPO_ROOT}/test/scripts/compliance-remediate/stubs" +export TT_STUBS_DIR + +tt_make_tmpdir() { + TT_TMP="$(mktemp -d)" + export TT_TMP +} + +tt_cleanup_tmpdir() { + if [ -n "${TT_TMP:-}" ] && [ -d "${TT_TMP}" ]; then + rm -rf "${TT_TMP}" + fi +} + +# Install the fake `gh` binary on PATH for the test. +tt_install_gh_stub() { + local stub_dir="${TT_TMP}/bin" + mkdir -p "$stub_dir" + cp "${TT_STUBS_DIR}/gh" "$stub_dir/gh" + chmod +x "$stub_dir/gh" + PATH="${stub_dir}:${PATH}" + export PATH +} + +# Write a findings.json file with a single finding and echo its path. +# Args: repo, category, check +tt_write_finding() { + local repo="$1" category="$2" check="$3" + local path="${TT_TMP}/findings.json" + jq -n \ + --arg repo "$repo" \ + --arg category "$category" \ + --arg check "$check" \ + '[{repo:$repo, category:$category, check:$check, severity:"warning", detail:"test", standard_ref:"x"}]' \ + > "$path" + printf '%s' "$path" +} diff --git a/test/scripts/compliance-remediate/security-analysis.bats b/test/scripts/compliance-remediate/security-analysis.bats new file mode 100644 index 000000000..b61073866 --- /dev/null +++ b/test/scripts/compliance-remediate/security-analysis.bats @@ -0,0 +1,126 @@ +#!/usr/bin/env bats +# Tests for the push-protection security_and_analysis remediation path added +# to scripts/compliance-remediate.sh. +# +# Before this change, every push-protection/* finding was routed to the +# `report_skip` catch-all. This suite pins the new behaviour: the specific +# `secret_scanning_non_provider_patterns` finding now PATCHes the nested +# security_and_analysis setting on the repo, and the rest of the +# push-protection/* catch-all is preserved. + +bats_require_minimum_version 1.5.0 + +load 'helpers/setup' + +setup() { + tt_make_tmpdir + tt_install_gh_stub + export GH_STUB_LOG="${TT_TMP}/gh.log" + export GH_STUB_STDIN_LOG="${TT_TMP}/gh.stdin" + : >"$GH_STUB_LOG" + : >"$GH_STUB_STDIN_LOG" +} + +teardown() { + tt_cleanup_tmpdir +} + +# --------------------------------------------------------------------------- +# Apply path: secret_scanning_non_provider_patterns is auto-remediated +# --------------------------------------------------------------------------- + +@test "applies PATCH to security_and_analysis for secret_scanning_non_provider_patterns" { + findings="$(tt_write_finding ".github" "push-protection" "secret_scanning_non_provider_patterns")" + report_dir="${TT_TMP}/report" + + GH_TOKEN=fake \ + FINDINGS_FILE="$findings" \ + REPORT_DIR="$report_dir" \ + DRY_RUN=false \ + run bash "$TT_SCRIPT" + + [ "$status" -eq 0 ] + + # gh was called with `api -X PATCH repos/petry-projects/.github --input -` + grep -qE 'api -X PATCH repos/petry-projects/\.github (.*)--input -' "$GH_STUB_LOG" + + # The stdin payload must enable the specific setting via the nested object + grep -q '"security_and_analysis"' "$GH_STUB_STDIN_LOG" + grep -q '"secret_scanning_non_provider_patterns"' "$GH_STUB_STDIN_LOG" + grep -q '"enabled"' "$GH_STUB_STDIN_LOG" + + # The finding should land in the direct-fix table, not in skipped + grep -q 'secret_scanning_non_provider_patterns' "$report_dir/remediation-report.md" + ! grep -q 'secret_scanning_non_provider_patterns' "$report_dir/skipped.md" +} + +# --------------------------------------------------------------------------- +# Dry-run path: no API call is made, but the finding shows up as a dry-run +# entry on the remediation report (not in the skipped report). +# --------------------------------------------------------------------------- + +@test "dry-run does not call the GitHub API but records the planned change" { + findings="$(tt_write_finding ".github" "push-protection" "secret_scanning_non_provider_patterns")" + report_dir="${TT_TMP}/report" + + GH_TOKEN=fake \ + FINDINGS_FILE="$findings" \ + REPORT_DIR="$report_dir" \ + DRY_RUN=true \ + run bash "$TT_SCRIPT" + + [ "$status" -eq 0 ] + + # No PATCH call should have been logged + ! grep -qE 'api -X PATCH repos/petry-projects/\.github' "$GH_STUB_LOG" + + # And the finding still lands on the remediation report, not skipped + grep -q 'secret_scanning_non_provider_patterns' "$report_dir/remediation-report.md" +} + +# --------------------------------------------------------------------------- +# Failure path: when the PATCH fails, the finding is reported as failed +# rather than as a successful remediation. +# --------------------------------------------------------------------------- + +@test "failed PATCH is reported as a failure" { + findings="$(tt_write_finding ".github" "push-protection" "secret_scanning_non_provider_patterns")" + report_dir="${TT_TMP}/report" + + GH_TOKEN=fake \ + GH_STUB_EXIT=1 \ + GH_STUB_STDERR="HTTP 403: forbidden" \ + FINDINGS_FILE="$findings" \ + REPORT_DIR="$report_dir" \ + DRY_RUN=false \ + run bash "$TT_SCRIPT" + + # The script's main() exits non-zero when failed_count > 0 + [ "$status" -ne 0 ] + + # And the finding is in the skipped/failed report with a FAILED prefix + grep -q 'FAILED' "$report_dir/skipped.md" +} + +# --------------------------------------------------------------------------- +# Catch-all preserved: other push-protection/* findings still skip +# --------------------------------------------------------------------------- + +@test "non-remediated push-protection findings still hit the skip path" { + findings="$(tt_write_finding ".github" "push-protection" "open_secret_alerts")" + report_dir="${TT_TMP}/report" + + GH_TOKEN=fake \ + FINDINGS_FILE="$findings" \ + REPORT_DIR="$report_dir" \ + DRY_RUN=false \ + run bash "$TT_SCRIPT" + + [ "$status" -eq 0 ] + + # No PATCH call should be issued for this category + ! grep -qE 'api -X PATCH' "$GH_STUB_LOG" + + # Finding should be in the skipped report + grep -q 'open_secret_alerts' "$report_dir/skipped.md" +} diff --git a/test/scripts/compliance-remediate/stubs/gh b/test/scripts/compliance-remediate/stubs/gh new file mode 100644 index 000000000..1d774c4c4 --- /dev/null +++ b/test/scripts/compliance-remediate/stubs/gh @@ -0,0 +1,40 @@ +#!/usr/bin/env bash +# Fake `gh` binary used by compliance-remediate bats tests. +# +# Mirrors the pattern from test/workflows/feature-ideation/stubs/gh: behaviour +# is fully env-driven so individual tests can script success / failure without +# per-fixture stub variants. +# +# Recognized vars: +# GH_STUB_STDOUT — literal text emitted on stdout (default empty) +# GH_STUB_STDERR — literal text emitted on stderr (default empty) +# GH_STUB_EXIT — exit code (default 0) +# GH_STUB_LOG — append-only log of argv invocations, one shell-quoted +# line per call (via `printf '%q '`) so tests can +# re-parse losslessly. +# GH_STUB_STDIN_LOG — if set, the stub's stdin is appended to this file +# so tests can assert on the JSON payload sent to +# `gh api --input -`. + +set -euo pipefail + +if [ -n "${GH_STUB_LOG:-}" ]; then + # shellcheck disable=SC2059 + printf '%q ' "$@" >>"$GH_STUB_LOG" + printf '\n' >>"$GH_STUB_LOG" +fi + +if [ -n "${GH_STUB_STDIN_LOG:-}" ]; then + # Drain stdin so the calling shell isn't blocked. Empty stdin is fine. + cat >>"$GH_STUB_STDIN_LOG" || true +fi + +if [ -n "${GH_STUB_STDOUT:-}" ]; then + printf '%s' "$GH_STUB_STDOUT" +fi + +if [ -n "${GH_STUB_STDERR:-}" ]; then + printf '%s' "$GH_STUB_STDERR" >&2 +fi + +exit "${GH_STUB_EXIT:-0}" From 672780f0828e6610eaee22c305b7f4cee9e2165e Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Tue, 9 Jun 2026 04:53:48 +0000 Subject: [PATCH 2/3] fix(reviews): address review comments [skip ci-relay] --- scripts/compliance-remediate.sh | 2 +- .../security-analysis.bats | 32 +++++++++++++++++-- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/scripts/compliance-remediate.sh b/scripts/compliance-remediate.sh index 4872f7971..f9a45027a 100644 --- a/scripts/compliance-remediate.sh +++ b/scripts/compliance-remediate.sh @@ -718,7 +718,7 @@ remediate_finding() { report_skip "$repo" "$check" \ "Dependabot config updates require human review — fix using standards/dependabot-policy.md" ;; - push-protection/secret_scanning_non_provider_patterns) + push-protection/non_provider_patterns_enabled | push-protection/secret_scanning_non_provider_patterns) remediate_security_analysis_setting "$repo" "secret_scanning_non_provider_patterns" "enabled" "$check" ;; push-protection/*) diff --git a/test/scripts/compliance-remediate/security-analysis.bats b/test/scripts/compliance-remediate/security-analysis.bats index b61073866..434e4487f 100644 --- a/test/scripts/compliance-remediate/security-analysis.bats +++ b/test/scripts/compliance-remediate/security-analysis.bats @@ -26,10 +26,38 @@ teardown() { } # --------------------------------------------------------------------------- -# Apply path: secret_scanning_non_provider_patterns is auto-remediated +# Apply path: non_provider_patterns_enabled (canonical audit check name) +# is auto-remediated # --------------------------------------------------------------------------- -@test "applies PATCH to security_and_analysis for secret_scanning_non_provider_patterns" { +@test "applies PATCH to security_and_analysis for non_provider_patterns_enabled" { + findings="$(tt_write_finding ".github" "push-protection" "non_provider_patterns_enabled")" + report_dir="${TT_TMP}/report" + + GH_TOKEN=fake \ + FINDINGS_FILE="$findings" \ + REPORT_DIR="$report_dir" \ + DRY_RUN=false \ + run bash "$TT_SCRIPT" + + [ "$status" -eq 0 ] + + # gh was called with `api -X PATCH repos/petry-projects/.github --input -` + grep -qE 'api -X PATCH repos/petry-projects/\.github (.*)--input -' "$GH_STUB_LOG" + + # The stdin payload must enable the specific setting via the nested object + grep -q '"security_and_analysis"' "$GH_STUB_STDIN_LOG" + grep -q '"secret_scanning_non_provider_patterns"' "$GH_STUB_STDIN_LOG" + grep -q '"enabled"' "$GH_STUB_STDIN_LOG" + + # The finding should land in the direct-fix table, not in skipped + grep -q 'non_provider_patterns_enabled' "$report_dir/remediation-report.md" + ! grep -q 'non_provider_patterns_enabled' "$report_dir/skipped.md" +} + +# Backward-compat alias: the legacy `secret_scanning_non_provider_patterns` +# finding name is still routed to the same handler. +@test "applies PATCH to security_and_analysis for secret_scanning_non_provider_patterns (legacy alias)" { findings="$(tt_write_finding ".github" "push-protection" "secret_scanning_non_provider_patterns")" report_dir="${TT_TMP}/report" From c950da53fdf58bd1a865fa76fce30f2c21147b6a Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Tue, 9 Jun 2026 05:05:07 +0000 Subject: [PATCH 3/3] fix(reviews): address review comments [skip ci-relay] --- scripts/compliance-remediate.sh | 11 ++++++++--- test/scripts/compliance-remediate/helpers/setup.bash | 12 +++++++++--- test/scripts/compliance-remediate/stubs/gh | 7 ++++++- 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/scripts/compliance-remediate.sh b/scripts/compliance-remediate.sh index f9a45027a..6f02541ba 100644 --- a/scripts/compliance-remediate.sh +++ b/scripts/compliance-remediate.sh @@ -154,7 +154,7 @@ remediate_setting() { remediate_security_analysis_setting() { local repo="$1" setting="$2" expected_status="$3" check="$4" - if [ "$DRY_RUN" = "true" ]; then + if [[ "$DRY_RUN" == "true" ]]; then skip "[DRY] Would PATCH $ORG/$repo: security_and_analysis.$setting.status=$expected_status" report_direct "$repo" "$check" \ "DRY: would PATCH \`security_and_analysis.$setting.status=$expected_status\`" @@ -162,10 +162,15 @@ remediate_security_analysis_setting() { fi local payload - payload=$(jq -n \ + if ! payload=$(jq -n \ --arg key "$setting" \ --arg status "$expected_status" \ - '{security_and_analysis: {($key): {status: $status}}}') + '{security_and_analysis: {($key): {status: $status}}}') || [[ -z "$payload" ]]; then + err "Failed to build payload for $ORG/$repo: security_and_analysis.$setting.status=$expected_status" + report_fail "$repo" "$check" \ + "jq failed to build payload for security_and_analysis.$setting.status=$expected_status" + return 0 + fi if printf '%s' "$payload" | gh api -X PATCH "repos/$ORG/$repo" --input - > /dev/null 2>&1; then ok "Fixed $ORG/$repo: security_and_analysis.$setting.status=$expected_status" diff --git a/test/scripts/compliance-remediate/helpers/setup.bash b/test/scripts/compliance-remediate/helpers/setup.bash index ae8ac0656..87a4281b0 100644 --- a/test/scripts/compliance-remediate/helpers/setup.bash +++ b/test/scripts/compliance-remediate/helpers/setup.bash @@ -1,13 +1,17 @@ #!/usr/bin/env bash # Common test helpers for compliance-remediate bats suites. +set -euo pipefail TT_REPO_ROOT="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")/../../../.." && pwd)" +readonly TT_REPO_ROOT export TT_REPO_ROOT TT_SCRIPT="${TT_REPO_ROOT}/scripts/compliance-remediate.sh" +readonly TT_SCRIPT export TT_SCRIPT TT_STUBS_DIR="${TT_REPO_ROOT}/test/scripts/compliance-remediate/stubs" +readonly TT_STUBS_DIR export TT_STUBS_DIR tt_make_tmpdir() { @@ -16,16 +20,18 @@ tt_make_tmpdir() { } tt_cleanup_tmpdir() { - if [ -n "${TT_TMP:-}" ] && [ -d "${TT_TMP}" ]; then + if [[ -n "${TT_TMP:-}" && -d "${TT_TMP}" ]]; then rm -rf "${TT_TMP}" fi } # Install the fake `gh` binary on PATH for the test. tt_install_gh_stub() { - local stub_dir="${TT_TMP}/bin" + local tt_tmp="${TT_TMP:?TT_TMP must be set (call tt_make_tmpdir first)}" + local stubs_dir="${TT_STUBS_DIR:?TT_STUBS_DIR must be set}" + local stub_dir="${tt_tmp}/bin" mkdir -p "$stub_dir" - cp "${TT_STUBS_DIR}/gh" "$stub_dir/gh" + cp "${stubs_dir}/gh" "$stub_dir/gh" chmod +x "$stub_dir/gh" PATH="${stub_dir}:${PATH}" export PATH diff --git a/test/scripts/compliance-remediate/stubs/gh b/test/scripts/compliance-remediate/stubs/gh index 1d774c4c4..e94580ea0 100644 --- a/test/scripts/compliance-remediate/stubs/gh +++ b/test/scripts/compliance-remediate/stubs/gh @@ -26,7 +26,12 @@ fi if [ -n "${GH_STUB_STDIN_LOG:-}" ]; then # Drain stdin so the calling shell isn't blocked. Empty stdin is fine. - cat >>"$GH_STUB_STDIN_LOG" || true + # Don't suppress failures — a broken log path means broken test + # instrumentation, which would silently weaken payload assertions. + cat >>"$GH_STUB_STDIN_LOG" || { + echo "gh stub: failed to append stdin to GH_STUB_STDIN_LOG=$GH_STUB_STDIN_LOG" >&2 + exit 1 + } fi if [ -n "${GH_STUB_STDOUT:-}" ]; then