diff --git a/scripts/compliance-audit.sh b/scripts/compliance-audit.sh index 544a9fe6..9acef7b7 100755 --- a/scripts/compliance-audit.sh +++ b/scripts/compliance-audit.sh @@ -992,6 +992,24 @@ _ruleset_contents_one() { return 0 fi + # --- Check parameterless rule types codified in the source of truth -------- + # Rules like `deletion` and `non_fast_forward` in pr-quality.json carry no + # `.parameters` block, so the parameter-level diff below does not inspect + # them. Run this before the `$rtype` early return so that a live ruleset + # missing both `$rtype` and a parameterless rule reports all missing rules in + # a single audit run. + local req_types req_type has_req_type + req_types=$(jq -r '.rules[]? | select((has("parameters") | not) or .parameters == null) | .type // empty' "$codified_file" 2>/dev/null || echo "") + while IFS= read -r req_type; do + [ -z "$req_type" ] && continue + has_req_type=$(echo "$rs" | jq --arg t "$req_type" 'any(.rules[]?; .type==$t)' 2>/dev/null || echo "false") + if [ "$has_req_type" != "true" ]; then + add_finding "$repo" "rulesets" "ruleset-drift-$name-missing-rule-$req_type" "error" \ + "Ruleset \`$name\` is missing required \`$req_type\` rule codified in \`standards/rulesets/$name.json\`; run \`scripts/apply-rulesets.sh --repo $ORG/$repo\` to converge." \ + "$std_ref" + fi + done <<< "$req_types" + # A ruleset that exists but has lost the codified rule type entirely is drift. local has_rule has_rule=$(echo "$rs" | jq --arg t "$rtype" 'any(.rules[]?; .type==$t)' 2>/dev/null || echo "false") @@ -1314,7 +1332,8 @@ check_legacy_rulesets() { check_codeowners() { local repo="$1" - # CODEOWNERS can be in root, .github/, or docs/ + # CODEOWNERS can be in .github/, root, or docs/ (probed in GitHub's actual + # precedence order: .github/CODEOWNERS → CODEOWNERS → docs/CODEOWNERS). local found=false local codeowners_content="" # #437: distinguish a genuinely-absent file (a deterministic HTTP 404 on every @@ -1324,7 +1343,7 @@ check_codeowners() { # (#487): capture the raw response + status that gh_api's `--jq` masks, retry # non-deterministic failures, and treat any non-404 failure as inconclusive. local any_unreadable=false - for path in CODEOWNERS .github/CODEOWNERS docs/CODEOWNERS; do + for path in .github/CODEOWNERS CODEOWNERS docs/CODEOWNERS; do local raw="" rc=1 i for i in 1 2 3; do raw=$(gh api "repos/$ORG/$repo/contents/$path" 2>&1) && rc=0 || rc=$? @@ -1364,14 +1383,21 @@ check_codeowners() { return 0 fi - # Extract non-comment, non-blank owner lines for accurate matching. - # Each such line has the form: [ ...] + # Extract non-comment, non-blank owner lines (stripping carriage returns and + # inline `#` comments) for accurate matching. Each such line has the form: + # [ ...] # Standard (codeowners-standard.md): # 1. @combsmith/org-leads MUST be the FIRST owner on every line. - # 2. Additional teams (@combsmith/) are allowed. - # 3. Individual users (@username without "/") are forbidden. + # 2. Additional teams (@combsmith/) are allowed on path-specific rules; + # the default `*` catch-all rule MUST list only `@combsmith/org-leads`. + # 3. Individual users and non-org owners are forbidden — every owner token + # must be a team in the form `@combsmith/`. local owner_lines - owner_lines=$(echo "$codeowners_content" | grep -v '^[[:space:]]*#' | grep -v '^[[:space:]]*$') + owner_lines=$(printf '%s\n' "$codeowners_content" \ + | tr -d '\r' \ + | grep -v '^[[:space:]]*#' \ + | sed -E 's/[[:space:]]+#.*$//' \ + | grep -v '^[[:space:]]*$' || true) if [ -z "$owner_lines" ]; then add_finding "$repo" "settings" "codeowners-empty" "error" \ @@ -1381,46 +1407,64 @@ check_codeowners() { fi # Rule 1: @combsmith/org-leads MUST be the first owner on every line - # (the first whitespace-separated token after the pattern). + # (the first whitespace-separated token after the pattern, normalizing any + # `\ ` escaped spaces inside the path pattern first). local bad_first_owner="" while IFS= read -r line; do [ -z "$line" ] && continue - # awk $2 = first owner token after the pattern + # Collapse `\ ` escaped spaces in the pattern so awk $2 is the first owner token local first_owner - first_owner=$(echo "$line" | awk '{print $2}') - if [ "$first_owner" != "@combsmith/org-leads" ]; then + first_owner=$(printf '%s\n' "$line" | sed 's/\\ /\\x20/g' | awk '{print $2}') + if [ "$first_owner" != "@${ORG}/org-leads" ]; then bad_first_owner="$line" break fi done <<< "$owner_lines" if [ -n "$bad_first_owner" ]; then add_finding "$repo" "settings" "codeowners-org-leads-not-first" "error" \ - "CODEOWNERS owner lines must list \`@combsmith/org-leads\` as the FIRST owner. Offending line: \`$bad_first_owner\`" \ + "CODEOWNERS owner lines must list \`@${ORG}/org-leads\` as the FIRST owner. Offending line: \`$bad_first_owner\`" \ "standards/codeowners-standard.md" fi - # Rule 3: no individual users — every owner token must be a team (contain "/"). - # Collect owner tokens from all lines (everything starting with @). + # Rule 3: no individual users, non-org owners, or malformed team slugs — + # every owner token (fields 2..NF on each owner line after normalizing `\ ` + # escaped spaces in the pattern) must match `@combsmith/` where + # GitHub team slugs are lowercase alphanumeric and hyphens (`[a-z0-9-]+`). local individual_owners - individual_owners=$(echo "$owner_lines" \ - | tr ' \t' '\n' \ - | grep -E '^@' \ - | grep -vE '^@[^/]+/' \ + individual_owners=$(printf '%s\n' "$owner_lines" \ + | sed 's/\\ /\\x20/g' \ + | awk -v org="$ORG" '{ + for (i = 2; i <= NF; i++) { + if ($i ~ /^#/) break + if ($i !~ ("^@" org "/[a-z0-9-]+$")) print $i + } + }' \ | sort -u || true) if [ -n "$individual_owners" ]; then local joined joined=$(echo "$individual_owners" | tr '\n' ' ') add_finding "$repo" "settings" "codeowners-individual-users" "error" \ - "CODEOWNERS contains forbidden individual user owners: ${joined% } — only teams (@combsmith/) are allowed; manage membership via teams" \ + "CODEOWNERS contains forbidden individual user, non-org, or invalid-slug owners: ${joined% } — only teams (@${ORG}/) are allowed; manage membership via teams" \ "standards/codeowners-standard.md" fi - # Advisory: a catch-all `*` pattern should exist so files unmatched by any - # path-specific rule still have an owner. - if ! echo "$owner_lines" | awk '{print $1}' | grep -qxF '*'; then + # Catch-all `*` rule: a default `*` pattern should exist so files unmatched by + # any path-specific rule still have an owner, and it must list ONLY + # `@combsmith/org-leads` (additional teams belong on path-specific rules only, + # otherwise they can satisfy `require_code_owner_review` repo-wide). + if ! printf '%s\n' "$owner_lines" | sed 's/\\ /\\x20/g' | awk '{print $1}' | grep -qxF '*'; then add_finding "$repo" "settings" "codeowners-no-catchall" "warning" \ "CODEOWNERS has no default \`*\` catch-all pattern — files not matched by a path rule will have no owner and \`require_code_owner_review\` will not apply to them" \ "standards/codeowners-standard.md" + else + local bad_catchall="" + bad_catchall=$(printf '%s\n' "$owner_lines" \ + | awk '$1 == "*" && NF > 2 { print; exit }') + if [ -n "$bad_catchall" ]; then + add_finding "$repo" "settings" "codeowners-catchall-extra-owners" "error" \ + "Default \`*\` catch-all line in CODEOWNERS must list ONLY \`@${ORG}/org-leads\` (\`* @${ORG}/org-leads\`); assign additional teams on path-specific rules instead. Offending line: \`$bad_catchall\`" \ + "standards/codeowners-standard.md" + fi fi } diff --git a/standards/codeowners-standard.md b/standards/codeowners-standard.md index d57796ba..e798a330 100644 --- a/standards/codeowners-standard.md +++ b/standards/codeowners-standard.md @@ -12,9 +12,13 @@ Every owner line in a CODEOWNERS file MUST follow these rules: 1. **`@combsmith/org-leads` MUST be the FIRST owner** on every owner line (so it always satisfies `require_code_owner_review`, regardless of path-specific overrides). -2. **Additional teams are allowed** as subsequent owners when finer-grained - ownership is desired (e.g., `@combsmith/security-leads` for - `/SECURITY.md`). +2. **Additional teams are allowed on path-specific rules** as subsequent owners + when finer-grained ownership is desired (e.g., `@combsmith/security-leads` + for `/SECURITY.md`). **The default `*` catch-all rule MUST list ONLY + `@combsmith/org-leads`** (`* @combsmith/org-leads`) with no additional teams + (because GitHub permits any listed owner on a matching rule to satisfy + `require_code_owner_review`, adding another team to `*` would grant that + team repo-wide code-owner approval standing). 3. **Individual named users are forbidden** as owners. All owners MUST be teams in the form `@combsmith/`. Manage individual membership through team membership instead. @@ -28,11 +32,14 @@ Every owner line in a CODEOWNERS file MUST follow these rules: /.github/ @combsmith/org-leads @combsmith/platform-leads ``` -The legacy direct-listing pattern is **forbidden**: +The legacy direct-listing pattern and extra teams on the `*` catch-all are **forbidden**: ```text # DO NOT USE — individuals and bot accounts as owners * @don-petry @dependabot-automerge-petry + +# DO NOT USE — additional teams on the default '*' catch-all +* @combsmith/org-leads @combsmith/security-leads ``` ## Why diff --git a/standards/rulesets/README.md b/standards/rulesets/README.md index 692b7d32..542ea04a 100644 --- a/standards/rulesets/README.md +++ b/standards/rulesets/README.md @@ -20,7 +20,7 @@ these rulesets are enforced. | File | Ruleset | Target | Enforces | |------|---------|--------|----------| -| [`pr-quality.json`](pr-quality.json) | `pr-quality` | `~DEFAULT_BRANCH` | 1 approval, code-owner review, thread resolution, dismiss-stale, squash-only merge | +| [`pr-quality.json`](pr-quality.json) | `pr-quality` | `~DEFAULT_BRANCH` | 1 approval, code-owner review, thread resolution, dismiss-stale, last-push approval, squash-only merge, block force-pushes & branch deletion | | [`code-quality.json`](code-quality.json) | `code-quality` | `~DEFAULT_BRANCH` | Required status checks (SonarCloud, CodeQL, agent-shield, dependency-audit), and code scanning merge protection: CodeQL, security alerts `medium_or_higher`, alerts `errors` — a floor the applier never lowers ([standard](../github-settings.md#code-scanning-merge-protection-all-repositories)) | | [`release-channel-tags.json`](release-channel-tags.json) | `release-channel-tags` | `refs/tags/**` | Blocks unauthorized **update/deletion** of any tag (protects moving channel pointers + version tags); creation stays free | | [`duplicate-decl-gate.json`](duplicate-decl-gate.json) | `duplicate-decl-gate` | `~DEFAULT_BRANCH` | Required status check `duplicate-decl-gate` — the #1485 corruption-class merge backstop. **Targeted** to the reusable-hosting meta-repos only (see below) | diff --git a/standards/rulesets/pr-quality.json b/standards/rulesets/pr-quality.json index 35d6d0e1..5f07464a 100644 --- a/standards/rulesets/pr-quality.json +++ b/standards/rulesets/pr-quality.json @@ -20,6 +20,12 @@ } }, "rules": [ + { + "type": "deletion" + }, + { + "type": "non_fast_forward" + }, { "type": "pull_request", "parameters": { diff --git a/test/scripts/apply-rulesets/apply-rulesets.bats b/test/scripts/apply-rulesets/apply-rulesets.bats index 96cd6c4e..efa8b745 100644 --- a/test/scripts/apply-rulesets/apply-rulesets.bats +++ b/test/scripts/apply-rulesets/apply-rulesets.bats @@ -102,6 +102,15 @@ EOF [ "$output" = "false" ] } +@test "pr-quality.json: enforces branch deletion and force-push protection (deletion + non_fast_forward)" { + # Anti-divergence guard: github-settings.md#pr-quality requires Allow force + # pushes: No (non_fast_forward) and Allow deletions: No (deletion), which + # OpenSSF Scorecard Branch-Protection also checks on the default branch. + run jq -r '[.rules[].type] | sort | join(",")' "$RULESETS_DIR/pr-quality.json" + [ "$status" -eq 0 ] + [ "$output" = "deletion,non_fast_forward,pull_request" ] +} + @test "release-channel-tags.json: tag ruleset, generic refs/tags/** include, update+deletion only" { run jq -er '.target == "tag" and (.conditions.ref_name.include == ["refs/tags/**"]) diff --git a/test/scripts/compliance-audit/codeowners-catchall.bats b/test/scripts/compliance-audit/codeowners-catchall.bats index 13901b28..41f19307 100644 --- a/test/scripts/compliance-audit/codeowners-catchall.bats +++ b/test/scripts/compliance-audit/codeowners-catchall.bats @@ -77,3 +77,105 @@ REPO_ROOT="$(cd "$BATS_TEST_DIRNAME/../../.." && pwd)" run _has_catchall "$(cat "$REPO_ROOT/.github/CODEOWNERS")" [ "$status" -eq 0 ] } + +# --------------------------------------------------------------------------- +# End-to-end check_codeowners validation (precedence, inline comments, +# catch-all extra owners, non-org/bare owner tokens) +# --------------------------------------------------------------------------- + +_run_codeowners_body() { + local body="$1" + local tmp + tmp="$(mktemp -d "$BATS_TEST_TMPDIR/co.XXXXXX")" + mkdir -p "$tmp/bin" + printf '#!/usr/bin/env bash\nexit 0\n' > "$tmp/bin/sleep" + chmod +x "$tmp/bin/sleep" + local b64 + b64="$(printf '%s' "$body" | base64 | tr -d '\n')" + cat > "$tmp/bin/gh" < "$REPORT_DIR/findings.json" + # shellcheck disable=SC1090 + source "'"$REPO_ROOT"'/scripts/compliance-audit.sh" + check_codeowners "demo-repo" + cat "$REPORT_DIR/findings.json" + ' + local rc=$? + rm -rf "$tmp" + return "$rc" +} + +@test "check_codeowners: inline '#' comments on catch-all and path rules are ignored" { + body=$'* @combsmith/org-leads # default owner (not @individual)\n/security/ @combsmith/org-leads @combsmith/security-leads # cc @someone\n' + run _run_codeowners_body "$body" + [ "$status" -eq 0 ] + [ "$output" = "[]" ] +} + +@test "check_codeowners: catch-all '*' with extra team is flagged (codeowners-catchall-extra-owners)" { + body=$'* @combsmith/org-leads @combsmith/other-team\n' + run _run_codeowners_body "$body" + [ "$status" -eq 0 ] + [[ "$output" == *"codeowners-catchall-extra-owners"* ]] + [[ "$output" == *"error"* ]] +} + +@test "check_codeowners: non-org team, bare username, or malformed team slug is flagged (codeowners-individual-users)" { + body=$'* @combsmith/org-leads\n/docs/ @combsmith/org-leads @other-org/team bareuser @combsmith/Org_Leads.With.Dots\n' + run _run_codeowners_body "$body" + [ "$status" -eq 0 ] + [[ "$output" == *"codeowners-individual-users"* ]] + [[ "$output" == *"@other-org/team"* ]] + [[ "$output" == *"bareuser"* ]] + [[ "$output" == *"@combsmith/Org_Leads.With.Dots"* ]] +} + +@test "check_codeowners: CRLF line endings do not trigger false positives" { + body=$'* @combsmith/org-leads\r\n/docs/ @combsmith/org-leads @combsmith/doc-leads\r\n' + run _run_codeowners_body "$body" + [ "$status" -eq 0 ] + [ "$output" = "[]" ] +} + +@test "check_codeowners: escaped-space path patterns do not mis-tokenize owners" { + body=$'* @combsmith/org-leads\nmy\\ dir/ @combsmith/org-leads\npath/with\\ multiple\\ spaces/ @combsmith/org-leads @combsmith/platform-leads\n' + run _run_codeowners_body "$body" + [ "$status" -eq 0 ] + [ "$output" = "[]" ] +} + +@test "check_codeowners: probes .github/CODEOWNERS before root CODEOWNERS (GitHub precedence)" { + local tmp + tmp="$(mktemp -d "$BATS_TEST_TMPDIR/co-prec.XXXXXX")" + mkdir -p "$tmp/bin" + printf '#!/usr/bin/env bash\nexit 0\n' > "$tmp/bin/sleep" + chmod +x "$tmp/bin/sleep" + local good_b64 bad_b64 + good_b64="$(printf '* @combsmith/org-leads\n' | base64 | tr -d '\n')" + bad_b64="$(printf '* @individual\n' | base64 | tr -d '\n')" + cat > "$tmp/bin/gh" <&2; exit 1 ;; +esac +EOF + chmod +x "$tmp/bin/gh" + run env PATH="$tmp/bin:$PATH" REPORT_DIR="$tmp" bash -c ' + set -uo pipefail + echo "[]" > "$REPORT_DIR/findings.json" + # shellcheck disable=SC1090 + source "'"$REPO_ROOT"'/scripts/compliance-audit.sh" + check_codeowners "demo-repo" + cat "$REPORT_DIR/findings.json" + ' + rm -rf "$tmp" + [ "$status" -eq 0 ] + [ "$output" = "[]" ] +} diff --git a/test/scripts/compliance-audit/ruleset-contents-drift.bats b/test/scripts/compliance-audit/ruleset-contents-drift.bats index 8ed41dde..d0368c5e 100644 --- a/test/scripts/compliance-audit/ruleset-contents-drift.bats +++ b/test/scripts/compliance-audit/ruleset-contents-drift.bats @@ -63,9 +63,12 @@ case "\$path" in ;; */rulesets/101) prq='$prq' + prq_default='$PRQ_PARAMS' if [ "\$prq" = "FETCHFAIL" ]; then echo "gh: Server Error (HTTP 500)" >&2; exit 1; fi if [ "\$prq" = "NORULE" ]; then echo '{"name":"pr-quality","id":101,"rules":[{"type":"deletion","parameters":null}]}'; exit 0; fi - echo "{\"name\":\"pr-quality\",\"id\":101,\"source\":\"combsmith/demo-repo\",\"rules\":[{\"type\":\"pull_request\",\"parameters\":\$prq}]}" + if [ "\$prq" = "NODELETION" ]; then echo "{\"name\":\"pr-quality\",\"id\":101,\"source\":\"combsmith/demo-repo\",\"rules\":[{\"type\":\"non_fast_forward\"},{\"type\":\"pull_request\",\"parameters\":\$prq_default}]}"; exit 0; fi + if [ "\$prq" = "NONONFF" ]; then echo "{\"name\":\"pr-quality\",\"id\":101,\"source\":\"combsmith/demo-repo\",\"rules\":[{\"type\":\"deletion\"},{\"type\":\"pull_request\",\"parameters\":\$prq_default}]}"; exit 0; fi + echo "{\"name\":\"pr-quality\",\"id\":101,\"source\":\"combsmith/demo-repo\",\"rules\":[{\"type\":\"deletion\"},{\"type\":\"non_fast_forward\"},{\"type\":\"pull_request\",\"parameters\":\$prq}]}" ;; */rulesets/102) cq='$cq' @@ -186,9 +189,24 @@ MOCK # --------------------------------------------------------------------------- # A ruleset that exists but has lost the codified rule type entirely is drift # --------------------------------------------------------------------------- -@test "pr-quality present but missing its pull_request rule → finding" { +@test "pr-quality present but missing its pull_request rule → finding (plus any missing parameterless rule)" { run _run_contents "NORULE" "$CQ_PARAMS" [ "$status" -eq 0 ] - [[ "$output" == *"ruleset-drift-pr-quality-missing-rule"* ]] + [[ "$output" == *"ruleset-drift-pr-quality-missing-rule\""* ]] + [[ "$output" == *"ruleset-drift-pr-quality-missing-rule-non_fast_forward"* ]] + [[ "$output" == *"error"* ]] +} + +@test "pr-quality present but missing its deletion rule → finding" { + run _run_contents "NODELETION" "$CQ_PARAMS" + [ "$status" -eq 0 ] + [[ "$output" == *"ruleset-drift-pr-quality-missing-rule-deletion"* ]] + [[ "$output" == *"error"* ]] +} + +@test "pr-quality present but missing its non_fast_forward rule → finding" { + run _run_contents "NONONFF" "$CQ_PARAMS" + [ "$status" -eq 0 ] + [[ "$output" == *"ruleset-drift-pr-quality-missing-rule-non_fast_forward"* ]] [[ "$output" == *"error"* ]] }