diff --git a/skills/github-project/SKILL.md b/skills/github-project/SKILL.md index 63775cd..03351f4 100644 --- a/skills/github-project/SKILL.md +++ b/skills/github-project/SKILL.md @@ -48,10 +48,10 @@ Requires `allow_auto_merge`, `pull_request_target`, bot detection, `gh pr merge ```bash gh pr view PR --repo OWNER/REPO --json autoMergeRequest --jq .autoMergeRequest -gh api repos/OWNER/REPO/branches/main/protection/required_pull_request_reviews \ - --jq '.bypass_pull_request_allowances.apps[].slug' ``` +Bypass actors: `references/security-config.md`. + ### GitHub Actions Failing ```bash @@ -63,6 +63,7 @@ gh run rerun RUN_ID --repo OWNER/REPO ### Security & Compliance Quick Checks ```bash +gh api repos/OWNER/REPO/rules/branches/main gh api repos/OWNER/REPO/branches/main/protection \ --jq '{rcr: .required_conversation_resolution.enabled, admins: .enforce_admins.enabled}' gh api repos/OWNER/REPO/code-scanning/default-setup --jq '.state' diff --git a/skills/github-project/checkpoints.yaml b/skills/github-project/checkpoints.yaml index ae803f9..71ae0cb 100644 --- a/skills/github-project/checkpoints.yaml +++ b/skills/github-project/checkpoints.yaml @@ -336,12 +336,27 @@ mechanical: by required status checks and review requirements. Severity is info (not error) because netresearch's org default leaves this off — admins retain bypass for emergency response. Tighten to true if your org - requires admin-bind policy. + requires admin-bind policy. This covers CLASSIC protection only — a + ruleset carries its own bypass_actors list, so also read + `gh api repos/OWNER/REPO/rulesets/ID --jq '.bypass_actors'` before + concluding who can bypass (references/security-config.md). - id: GH-31 - type: gh_api - endpoint: "repos/{owner}/{repo}/branches/{default_branch}/protection" - json_path: ".required_conversation_resolution.enabled" + type: command + target: | + # Conversation resolution can come from EITHER classic branch protection + # OR a ruleset, and the classic endpoint is blind to rulesets — reading it + # alone fails a repository that is correctly configured through a ruleset. + # See references/security-config.md. + command -v gh >/dev/null 2>&1 || exit 0 + R=$(gh repo view --json nameWithOwner --jq .nameWithOwner 2>/dev/null) || exit 0 + B=$(gh repo view --json defaultBranchRef --jq .defaultBranchRef.name 2>/dev/null) || exit 0 + classic=$(gh api "repos/$R/branches/$B/protection" \ + --jq '.required_conversation_resolution.enabled // false' 2>/dev/null || echo false) + ruleset=$(gh api "repos/$R/rules/branches/$B" \ + --jq 'any(.[]?; .type == "pull_request" + and .parameters.required_review_thread_resolution == true)' 2>/dev/null || echo false) + [ "$classic" = "true" ] || [ "$ruleset" = "true" ] severity: error desc: >- required_conversation_resolution must be enabled. Without it, PR @@ -422,8 +437,11 @@ llm_reviews: leaves this off so admins retain bypass for emergency response; tighten to true only if your org requires admin-bind policy. Do NOT report this as an error on its own — flag it as info/advisory. - 2. Run: gh api repos/OWNER/REPO/branches/BRANCH/protection --jq '.required_conversation_resolution.enabled' - Must be true. Without this, unresolved review threads do not block merges. + 2. Run BOTH, because a ruleset is invisible to the classic endpoint: + gh api repos/OWNER/REPO/branches/BRANCH/protection --jq '.required_conversation_resolution.enabled' + gh api repos/OWNER/REPO/rules/branches/BRANCH --jq 'any(.[]?; .type=="pull_request" and .parameters.required_review_thread_resolution == true)' + Either being true is enough. Without both being false, unresolved + review threads do not block merges. This is the primary error-level enforcement check (paired with GH-31). 3. Note the interaction: enforce_admins=false means admins COULD bypass required_conversation_resolution. Surface this as an advisory trade-off, diff --git a/skills/github-project/references/security-config.md b/skills/github-project/references/security-config.md index 6275e0b..07efd61 100644 --- a/skills/github-project/references/security-config.md +++ b/skills/github-project/references/security-config.md @@ -464,3 +464,140 @@ curl -fsSL "https://sonarcloud.io/api/qualitygates/project_status?projectKey=KEY curl -fsSL "https://sonarcloud.io/api/hotspots/search?projectKey=KEY&branch=main&status=TO_REVIEW&ps=30" \ | jq -r '.hotspots[]? | "\(.component | sub(".*:"; "")):\(.line // "?") \(.securityCategory // "")"' ``` + +## Effective Branch Rules + +Reading what a branch actually enforces, and auditing whether a required check +can fail. + +### Two rule sources, and only one of them is obvious + +A branch can be governed by **classic branch protection** and by **rulesets** +at the same time. GitHub composes them and applies the most restrictive result. +The classic endpoint cannot see rulesets, so reading it alone reports a branch +as unprotected that is in fact fully gated: + +```bash +# Classic protection — returns real values, but is BLIND to rulesets +gh api repos/OWNER/REPO/branches/main/protection + +# Rulesets — the effective rules from every active ruleset on the branch +gh api repos/OWNER/REPO/rules/branches/main +``` + +Observed on a repository where both were configured: + +| Field, classic endpoint | Reads as | Reality (rulesets) | +|---|---|---| +| `required_status_checks: null` | nothing is required | 23 required contexts | +| `required_approving_review_count: 0` | no review required | 1 required approval | +| `required_conversation_resolution: true` | correct | also required | +| `enforce_admins: false` | correct | plus per-ruleset bypass actors | + +The two failure shapes are asymmetric and both are bad: + +- **`null` reads as "not configured".** `required_status_checks` is absent from + the classic payload entirely when a ruleset supplies the checks. +- **`0` reads as a real answer.** `required_approving_review_count: 0` is the + *classic* setting, not the effective one. Nothing in the response says a + ruleset requires 1. + +A compliance document was written off the classic endpoint alone and attested +that the repository required neither review nor status checks. Both statements +were false, and nothing in the response hinted at it. + +**Read both. When they disagree, the effective answer is the more restrictive +one.** Name which endpoint a claim came from whenever the claim lands in a +document, an issue or a PR body. + +### Bypass actors live on the ruleset, not on the branch + +`enforce_admins` covers classic protection only. Rulesets carry their own list: + +```bash +gh api repos/OWNER/REPO/rulesets/RULESET_ID \ + --jq '{name, enforcement, bypass: [.bypass_actors[]? | {actor_type, actor_id, bypass_mode}]}' +``` + +`bypass_mode: always` lets the actor push straight past the rule; +`pull_request` lets it merge a pull request that does not satisfy it. The REST +API returns `actor_id` as a number and does not resolve it to a role name — say +"repository role id 5" rather than guessing "admin" unless you resolved it. + +### Auditing whether a required check can actually fail + +A context being in the required list does not mean it gates anything. + +### A required check that is always skipped enforces nothing + +```bash +gh api "repos/OWNER/REPO/commits/SHA/check-runs?per_page=100" \ + --jq '.check_runs[] | "\(.conclusion // .status)\t\(.name)"' | sort -u +``` + +Cross-check every required context against that list. A context whose +conclusion is `skipped` on every commit is a requirement that no state of the +code can violate. Observed case: `fuzz / Fuzz Tests` was required and always +skipped, because the job that produced it passed no inputs to its reusable +workflow and the suite defaulted to off. The job that actually ran the suite was +a different context and was not required at all. + +### Check-run names come from the JOB name, not the workflow + +Two jobs calling the same reusable workflow produce check-runs under +**different** prefixes, because the prefix is the calling job's name: + +```yaml +# .github/workflows/checks.yml +jobs: + fuzz: # -> "fuzz / Fuzz Tests" + uses: org/reusables/.github/workflows/fuzz.yml@main + +# .github/workflows/ci.yml +jobs: + fuzz-mutation: # -> "fuzz-mutation / Fuzz Tests" + uses: org/reusables/.github/workflows/fuzz.yml@main + with: { run-fuzz-tests: true } +``` + +The two names look interchangeable and are not. Do not infer which workflow +emits a context — measure it: + +```bash +for id in $(gh run list --repo OWNER/REPO --commit SHA --limit 30 --json databaseId --jq '.[].databaseId'); do + wf=$(gh api repos/OWNER/REPO/actions/runs/$id --jq .path) + gh api "repos/OWNER/REPO/actions/runs/$id/jobs?per_page=100" --jq '.jobs[].name' \ + | sed "s|^|$(basename "$wf")\t|" +done | sort -u +``` + +### A gate job is only worth requiring if it reads its own `needs` + +Requiring one aggregate context instead of many is a good pattern — it makes a +job that is missing from `gate.needs` the only failure mode, rather than a +silent coverage loss. It is only sound if the gate actually evaluates its +dependencies: + +```yaml +- name: Fail unless every job succeeded or was skipped + env: + RESULTS: ${{ toJSON(needs) }} + run: | + bad=$(jq -r 'to_entries[] | select(.value.result != "success" and .value.result != "skipped") | "\(.key)=\(.value.result)"' <<<"$RESULTS") + [ -z "$bad" ] || { echo "::error::gate failed — $bad"; exit 1; } +``` + +Read the gate's steps before requiring it. A job with `if: always()` and no +result evaluation is a green rubber stamp, and requiring it would be worse than +requiring nothing. Treating `skipped` as passing is deliberate: jobs skipped by +an event gate must not block the merge queue. + +### Before changing a required-context list + +1. Back up the ruleset: `gh api repos/OWNER/REPO/rulesets/ID > backup.json`. +2. Confirm every context you are about to require reports on `merge_group`, not + only on `pull_request` — requiring a context the queue never produces wedges + the queue for everyone. +3. Apply with `gh api -X PUT repos/OWNER/REPO/rulesets/ID --input new.json`. +4. Read the effective list back from `rules/branches/BRANCH` and confirm the + next pull request reports every newly required context.