diff --git a/.github/branch-protection/dev.json b/.github/branch-protection/dev.json new file mode 100644 index 0000000..d82fe22 --- /dev/null +++ b/.github/branch-protection/dev.json @@ -0,0 +1,61 @@ +{ + "name": "dev branch protection", + "target": "branch", + "enforcement": "active", + "bypass_actors": [], + "conditions": { + "ref_name": { + "include": [ + "refs/heads/dev" + ], + "exclude": [] + } + }, + "rules": [ + { + "type": "deletion" + }, + { + "type": "non_fast_forward" + }, + { + "type": "pull_request", + "parameters": { + "required_approving_review_count": 1, + "dismiss_stale_reviews_on_push": true, + "require_code_owner_review": true, + "require_last_push_approval": true, + "required_review_thread_resolution": true + } + }, + { + "type": "required_status_checks", + "parameters": { + "strict_required_status_checks_policy": true, + "do_not_enforce_on_create": false, + "required_status_checks": [ + { + "context": "CI Summary", + "integration_id": 15368 + }, + { + "context": "DCO sign-off", + "integration_id": 15368 + }, + { + "context": "dependency-review", + "integration_id": 15368 + }, + { + "context": "Analyze (python)", + "integration_id": 15368 + }, + { + "context": "Analyze (javascript)", + "integration_id": 15368 + } + ] + } + } + ] +} diff --git a/.github/branch-protection/main.json b/.github/branch-protection/main.json new file mode 100644 index 0000000..cd003e5 --- /dev/null +++ b/.github/branch-protection/main.json @@ -0,0 +1,61 @@ +{ + "name": "main branch protection", + "target": "branch", + "enforcement": "active", + "bypass_actors": [], + "conditions": { + "ref_name": { + "include": [ + "refs/heads/main" + ], + "exclude": [] + } + }, + "rules": [ + { + "type": "deletion" + }, + { + "type": "non_fast_forward" + }, + { + "type": "pull_request", + "parameters": { + "required_approving_review_count": 2, + "dismiss_stale_reviews_on_push": true, + "require_code_owner_review": true, + "require_last_push_approval": true, + "required_review_thread_resolution": true + } + }, + { + "type": "required_status_checks", + "parameters": { + "strict_required_status_checks_policy": true, + "do_not_enforce_on_create": false, + "required_status_checks": [ + { + "context": "CI Summary", + "integration_id": 15368 + }, + { + "context": "DCO sign-off", + "integration_id": 15368 + }, + { + "context": "dependency-review", + "integration_id": 15368 + }, + { + "context": "Analyze (python)", + "integration_id": 15368 + }, + { + "context": "Analyze (javascript)", + "integration_id": 15368 + } + ] + } + } + ] +} diff --git a/.github/workflows/branch-protection-audit.yml b/.github/workflows/branch-protection-audit.yml new file mode 100644 index 0000000..6b964e1 --- /dev/null +++ b/.github/workflows/branch-protection-audit.yml @@ -0,0 +1,54 @@ +name: Branch Protection Audit + +# Proves that dev and main enforce the rulesets declared in +# .github/branch-protection/ instead of trusting documentation (issue #298). +# Pull requests only validate the declared files; the live comparison runs on +# a schedule and on demand, and a drift result fails the run. + +on: + pull_request: + branches: [dev, main] + paths: + - ".github/branch-protection/**" + - "scripts/check_branch_protection.py" + - ".github/workflows/branch-protection-audit.yml" + schedule: + - cron: "0 6 * * 1" # weekly, Monday 06:00 UTC + workflow_dispatch: + +permissions: + contents: read + +jobs: + audit: + name: Branch protection audit + runs-on: ubuntu-latest + steps: + - name: Checkout repository + uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 + + - name: Set up Python 3.11 + uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 + with: + python-version: "3.11" + + - name: Validate declared rulesets + run: python scripts/check_branch_protection.py --validate-only + + - name: Compare effective rules with declared rulesets + if: github.event_name != 'pull_request' + env: + # A read-only token can read effective branch rules. Bypass actors are + # only visible to a token that can administer rulesets; without + # BRANCH_PROTECTION_AUDIT_TOKEN they are reported as unverified. + GITHUB_TOKEN: ${{ secrets.BRANCH_PROTECTION_AUDIT_TOKEN || github.token }} + run: python scripts/check_branch_protection.py --repo "$GITHUB_REPOSITORY" --evidence branch-protection-evidence.json + + - name: Retain evidence + if: always() && github.event_name != 'pull_request' + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + with: + name: branch-protection-evidence + path: branch-protection-evidence.json + if-no-files-found: warn + retention-days: 90 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1f051a7..b47ba03 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,11 +5,18 @@ on: branches: - dev - main + # Post-merge assurance: re-run the full gate on the merged result, so a green + # PR run against an older base is not the only evidence for dev and main. + push: + branches: + - dev + - main -# Cancel superseded runs on the same PR to save runner minutes +# Cancel superseded runs on the same PR to save runner minutes. Post-merge runs +# are never cancelled, so every commit on dev and main keeps its own result. concurrency: - group: ci-${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: true + group: ci-${{ github.workflow }}-${{ github.event_name == 'push' && github.sha || github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} # Least privilege: jobs only read repo contents permissions: diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index eadba0a..3f654e3 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -5,6 +5,10 @@ on: branches: - dev - main + push: + branches: + - dev + - main schedule: - cron: "0 3 * * 1" # weekly, Monday 03:00 UTC diff --git a/.github/workflows/update-learn-page.yml b/.github/workflows/update-learn-page.yml index 92818c3..173323e 100644 --- a/.github/workflows/update-learn-page.yml +++ b/.github/workflows/update-learn-page.yml @@ -4,9 +4,12 @@ on: push: branches: [dev] -# Only the final commit step writes; nothing here needs any other scope. +# dev is protected, so the refreshed statistics are proposed through a pull +# request instead of being pushed to dev directly (issue #298). contents:write +# covers the bot branch; pull-requests:write opens or updates the PR. permissions: contents: write + pull-requests: write concurrency: group: update-learn-page-${{ github.ref }} @@ -19,6 +22,9 @@ jobs: steps: - name: Checkout repository uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 + with: + # The bot branch push must use the same token as the PR so CI starts. + token: ${{ secrets.STATS_BOT_TOKEN || github.token }} - name: Set up Python 3.11 uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 @@ -37,13 +43,25 @@ jobs: echo "changed=true" >> "$GITHUB_OUTPUT" fi - - name: Commit and push + - name: Propose refreshed statistics if: steps.diff.outputs.changed == 'true' env: - TARGET_REF: ${{ github.ref_name }} + BASE_REF: ${{ github.ref_name }} + BOT_BRANCH: docs/refresh-learn-page-stats + # Pull requests opened with the default GITHUB_TOKEN do not start + # workflows, so required checks never report. A maintainer-provided + # STATS_BOT_TOKEN (GitHub App or fine-grained token with contents and + # pull-requests write) lets CI run on the bot PR like any other. + GH_TOKEN: ${{ secrets.STATS_BOT_TOKEN || github.token }} run: | git config user.name "github-actions[bot]" git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + git switch -c "$BOT_BRANCH" git add docs/learn/index.html README.md - git commit -s -m "docs: refresh learn page and README statistics [skip ci]" - git push origin "HEAD:$TARGET_REF" + git commit -s -m "docs: refresh learn page and README statistics" + git push --force origin "HEAD:refs/heads/$BOT_BRANCH" + if [ -z "$(gh pr list --head "$BOT_BRANCH" --base "$BASE_REF" --state open --json number --jq '.[].number')" ]; then + gh pr create --base "$BASE_REF" --head "$BOT_BRANCH" \ + --title "docs: refresh learn page and README statistics" \ + --body "Automated refresh generated by \`.github/scripts/update_learn_page.py\` from \`$BASE_REF\` at $GITHUB_SHA. Review and merge through the normal protected-branch flow." + fi diff --git a/CHANGELOG.md b/CHANGELOG.md index 142503e..a4be53e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ OpenShield uses [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Added +- Branch protection declared as versioned GitHub rulesets for `dev` and `main`, with a scheduled drift audit that retains evidence (#298) +- CI and CodeQL post-merge runs on `dev` and `main` (#298) - Azure Network Layer Assurance API with 20-domain coverage, network-rule classification, and authoritative IP forwarding and direct Internet route checks - Azure Resource Graph inventory snapshots as the first OpenShield Evidence Graph foundation - Azure Data Link Layer Assurance API with LLC and MAC coverage plus ExpressRoute Direct MACsec checks diff --git a/docs/ci-pipeline.md b/docs/ci-pipeline.md index efa2de4..d02d2d6 100644 --- a/docs/ci-pipeline.md +++ b/docs/ci-pipeline.md @@ -157,15 +157,63 @@ Compose's `local` profile is not production configuration. Its credentials are d The branch flow is `feature/* → dev → main`. Protection is applied to the two **destinations**; feature branches stay unprotected for fast iteration. ``` -feat/* fix/* docs/* ──PR──▶ dev ──PR──▶ main ──▶ production environment - (unprotected) (gate) (stricter gate) +feat/* fix/* docs/* infra/* ──PR──▶ dev ──PR──▶ main ──▶ production environment + (unprotected) (gate) (stricter gate) ``` -- **`dev`** — requires all CI checks above + CodeQL, 1 approving review, and "branches up to date before merging". -- **`main`** — everything `dev` requires, plus stricter review (2 approvals / code owners), `enforce_admins`, and the **Enforce dev to main source** check, which blocks any PR into `main` whose source branch is not `dev`. (To permit emergency hotfixes straight to `main`, widen that job's condition to also accept `hotfix/*`.) -- **`production` environment** — required reviewers with "prevent self-review", so a `dev → main` deployment cannot be approved by its own author. +### Declared state (source of truth) -CI runs at **both** merge points (`on: pull_request` targets `dev` and `main`), so the same gates apply on the way into `dev` and again, stricter, on the way into `main`. +Protection is declared as code in GitHub's repository-ruleset format: + +| Branch | File | Reviews | Required checks | +|---|---|---|---| +| `dev` | [`.github/branch-protection/dev.json`](../.github/branch-protection/dev.json) | 1 approval, code owner review, approval of the latest push, stale approvals dismissed, conversations resolved | `CI Summary`, `DCO sign-off`, `dependency-review`, `Analyze (python)`, `Analyze (javascript)` — strict (head must be up to date) | +| `main` | [`.github/branch-protection/main.json`](../.github/branch-protection/main.json) | Same as `dev`, but **2 approvals** | Same as `dev`, strict | + +Both rulesets block branch deletion and force pushes and declare **no standing bypass actors**. `CI Summary` already fails when any CI job fails, including **Enforce dev to main source**, which blocks any PR into `main` whose source branch is not `dev`. `require_last_push_approval` means the person who pushed the last commit cannot provide the approval that satisfies the rule, so a promotion cannot be self-approved. + +`tests/test_check_branch_protection.py` fails if a declared ruleset is weakened (fewer approvals, non-strict checks, a bypass actor) or names a required check that no workflow job produces — a misspelled required check would otherwise block every merge. + +### Applying the rulesets (repository administrators) + +Only an administrator can change protection. Import each file once, either through **Settings → Rules → Rulesets → New ruleset → Import a ruleset**, or with the API: + +```bash +gh api -X POST repos/OWASP/openshield/rulesets --input .github/branch-protection/dev.json +gh api -X POST repos/OWASP/openshield/rulesets --input .github/branch-protection/main.json +``` + +To change an existing ruleset, edit the JSON file in a reviewed PR first, then apply it with `gh api -X PUT repos/OWASP/openshield/rulesets/ --input `. Once the rulesets are active, remove the legacy classic protection so there is one source of truth. The **production** environment should keep required reviewers with **Prevent self-review** enabled. + +### Effective state (evidence) + +The **Branch Protection Audit** workflow (`.github/workflows/branch-protection-audit.yml`) runs weekly and on demand. It compares the rules GitHub actually enforces (`GET /repos/{repo}/rules/branches/{branch}`) with the declared files, fails on any drift, and uploads a JSON evidence record as the `branch-protection-evidence` artifact (kept 90 days). Run it locally with: + +```bash +python scripts/check_branch_protection.py --validate-only +GITHUB_TOKEN= python scripts/check_branch_protection.py --repo OWASP/openshield --evidence evidence.json +``` + +A read-only token can see enforced rules. Bypass actors are only returned to a token that can administer rulesets; store one as the `BRANCH_PROTECTION_AUDIT_TOKEN` secret, otherwise bypass actors are reported as *unverified* rather than assumed compliant. + +As of the 2026-08-21 audit recorded in issue #298, both branches reported `protected: true` but required status-check enforcement was `off` and no rulesets existed. Until an administrator applies the rulesets above and the audit passes, treat the review and check requirements in this section as **declared, not enforced**. + +To demonstrate enforcement after applying (acceptance criteria for #298): open a throwaway PR with a deliberately failing test and confirm merge is blocked; push to `dev` after that PR's last CI run and confirm the stale head cannot merge; open a `dev → main` PR and confirm its author cannot satisfy the approval requirement. + +### Emergency changes + +There is no standing bypass. If a production incident requires merging without the normal gates: + +1. Open an issue labelled `priority: critical` describing the incident, the change and why the gates cannot be met. +2. An administrator temporarily switches the affected ruleset's enforcement to **Evaluate** (or adds themselves as a bypass actor), merges the reviewed fix, and restores **Active** immediately. +3. Record the time window, the actor and the merged commit on the issue, then run the Branch Protection Audit workflow and attach its evidence to the issue. +4. The change still receives a retrospective review from a second maintainer. + +### Post-merge CI and automated statistics + +CI and CodeQL also run on every push to `dev` and `main`, so each merged commit has its own result instead of relying only on a PR run against an older base. Post-merge runs are never cancelled. + +The **Update Learn Page and README Stats** workflow no longer pushes to `dev`. When statistics change it force-updates the `docs/refresh-learn-page-stats` branch and opens (or refreshes) a pull request, which goes through the same protected flow. Pull requests opened with the default `GITHUB_TOKEN` do not start workflows, so administrators should provide a `STATS_BOT_TOKEN` secret (GitHub App token or fine-grained token with contents and pull-requests write) and allow GitHub Actions to create pull requests in repository settings. --- diff --git a/scripts/check_branch_protection.py b/scripts/check_branch_protection.py new file mode 100644 index 0000000..1ae812d --- /dev/null +++ b/scripts/check_branch_protection.py @@ -0,0 +1,297 @@ +"""Compare effective GitHub branch rules against the versioned rulesets. + +The desired protection for ``dev`` and ``main`` lives in +``.github/branch-protection/.json`` in GitHub's repository-ruleset +import format. This script reads the rules GitHub actually enforces on each +branch and fails when they are weaker than that declared state, so protection +is demonstrated by an automated assertion rather than by documentation alone. + +Usage: + python scripts/check_branch_protection.py --validate-only + GITHUB_TOKEN=... python scripts/check_branch_protection.py \\ + --repo OWASP/openshield --evidence branch-protection-evidence.json + +``--validate-only`` checks the declared rulesets offline. Without it the script +queries the GitHub REST API. ``GET /repos/{repo}/rules/branches/{branch}`` is +readable with a read-only token; ruleset bypass actors are only returned to +tokens that can administer rulesets, so when they are hidden the script +reports them as unverified instead of treating them as compliant. +""" + +import argparse +import json +import os +import ssl +import sys +import http.client +from datetime import datetime, timezone +from pathlib import Path +from typing import Any, Callable, Dict, List, Optional, Tuple + +REPO_ROOT = Path(__file__).resolve().parents[1] +DEFAULT_CONFIG_DIR = REPO_ROOT / ".github" / "branch-protection" +DEFAULT_BRANCHES = ("dev", "main") +API_HOST = "api.github.com" + +# Promotion to main must never be approvable by its own author alone. +MIN_APPROVALS = {"dev": 1, "main": 2} +REQUIRED_RULE_TYPES = ("deletion", "non_fast_forward", "pull_request", "required_status_checks") +PULL_REQUEST_FLAGS = ( + "dismiss_stale_reviews_on_push", + "require_code_owner_review", + "require_last_push_approval", + "required_review_thread_resolution", +) + +Fetcher = Callable[[str], Any] + + +def load_expected(config_dir: Path, branch: str) -> Dict[str, Any]: + """Load the declared ruleset for ``branch``.""" + with open(config_dir / f"{branch}.json", encoding="utf-8") as handle: + return json.load(handle) + + +def _rules_by_type(rules: List[Dict[str, Any]], rule_type: str) -> List[Dict[str, Any]]: + return [rule for rule in rules if rule.get("type") == rule_type] + + +def _required_contexts(rule: Dict[str, Any]) -> List[str]: + checks = (rule.get("parameters") or {}).get("required_status_checks") or [] + return [check.get("context", "") for check in checks] + + +def validate_declared(branch: str, ruleset: Dict[str, Any]) -> List[str]: + """Return problems in a declared ruleset; an empty list means it is valid.""" + problems: List[str] = [] + if ruleset.get("target") != "branch": + problems.append(f"{branch}: ruleset target must be 'branch'") + if ruleset.get("enforcement") != "active": + problems.append(f"{branch}: ruleset enforcement must be 'active'") + if ruleset.get("bypass_actors"): + problems.append(f"{branch}: declared ruleset must not grant standing bypass actors") + include = ((ruleset.get("conditions") or {}).get("ref_name") or {}).get("include") or [] + if f"refs/heads/{branch}" not in include: + problems.append(f"{branch}: ruleset does not target refs/heads/{branch}") + + rules = ruleset.get("rules") or [] + for rule_type in REQUIRED_RULE_TYPES: + if not _rules_by_type(rules, rule_type): + problems.append(f"{branch}: declared ruleset is missing the '{rule_type}' rule") + + for rule in _rules_by_type(rules, "pull_request"): + params = rule.get("parameters") or {} + if params.get("required_approving_review_count", 0) < MIN_APPROVALS.get(branch, 1): + problems.append(f"{branch}: declared ruleset requires fewer than {MIN_APPROVALS.get(branch, 1)} approvals") + for flag in PULL_REQUEST_FLAGS: + if params.get(flag) is not True: + problems.append(f"{branch}: declared pull_request rule must set {flag}") + + for rule in _rules_by_type(rules, "required_status_checks"): + params = rule.get("parameters") or {} + if params.get("strict_required_status_checks_policy") is not True: + problems.append(f"{branch}: declared status checks must be strict (up to date before merge)") + if "CI Summary" not in _required_contexts(rule): + problems.append(f"{branch}: declared status checks must include 'CI Summary'") + return problems + + +def compare_effective( + branch: str, + expected: Dict[str, Any], + effective_rules: List[Dict[str, Any]], +) -> List[str]: + """Return every way the effective branch rules are weaker than declared.""" + problems: List[str] = [] + expected_rules = expected.get("rules") or [] + + for rule_type in REQUIRED_RULE_TYPES: + if _rules_by_type(expected_rules, rule_type) and not _rules_by_type(effective_rules, rule_type): + problems.append(f"{branch}: '{rule_type}' rule is not enforced") + + effective_pr = [rule.get("parameters") or {} for rule in _rules_by_type(effective_rules, "pull_request")] + for rule in _rules_by_type(expected_rules, "pull_request"): + params = rule.get("parameters") or {} + if not effective_pr: + break + wanted = params.get("required_approving_review_count", 0) + actual = max(p.get("required_approving_review_count", 0) for p in effective_pr) + if actual < wanted: + problems.append(f"{branch}: requires {actual} approving review(s), expected at least {wanted}") + for flag in PULL_REQUEST_FLAGS: + if params.get(flag) and not any(p.get(flag) for p in effective_pr): + problems.append(f"{branch}: pull_request rule does not enforce {flag}") + + effective_checks = _rules_by_type(effective_rules, "required_status_checks") + enforced_contexts = {context for rule in effective_checks for context in _required_contexts(rule)} + strict = any( + (rule.get("parameters") or {}).get("strict_required_status_checks_policy") for rule in effective_checks + ) + for rule in _rules_by_type(expected_rules, "required_status_checks"): + if not effective_checks: + break + for context in _required_contexts(rule): + if context not in enforced_contexts: + problems.append(f"{branch}: required status check '{context}' is not enforced") + if (rule.get("parameters") or {}).get("strict_required_status_checks_policy") and not strict: + problems.append(f"{branch}: status checks are not strict; a stale head can merge") + return problems + + +def audit_bypass(branch: str, rulesets: List[Dict[str, Any]]) -> Tuple[List[str], List[str]]: + """Return (problems, notes) about bypass actors on the rulesets in force.""" + problems: List[str] = [] + notes: List[str] = [] + for ruleset in rulesets: + name = ruleset.get("name", ruleset.get("id")) + if "bypass_actors" not in ruleset: + notes.append(f"{branch}: bypass actors on ruleset '{name}' are not visible to this token (unverified)") + elif ruleset["bypass_actors"]: + actors = ", ".join( + f"{actor.get('actor_type')}:{actor.get('actor_id')}({actor.get('bypass_mode')})" + for actor in ruleset["bypass_actors"] + ) + problems.append(f"{branch}: ruleset '{name}' grants standing bypass to {actors}") + return problems, notes + + +class GitHubApiError(Exception): + """The GitHub API returned a non-success status.""" + + def __init__(self, status: int) -> None: + super().__init__(f"GitHub API returned HTTP {status}") + self.status = status + + +def github_fetcher(token: Optional[str]) -> Fetcher: + """Return a JSON GET helper bound to the GitHub REST API host. + + The host and scheme are fixed; only the request path varies, so a crafted + value can never redirect the request to another host or a file:// URL. + """ + + def fetch(path: str) -> Any: + if not path.startswith("/"): + raise ValueError(f"API path must be absolute: {path!r}") + headers = { + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + "User-Agent": "openshield-branch-protection-audit", + } + if token: + headers["Authorization"] = f"Bearer {token}" + # Fixed host with certificate and hostname verification from the default context. + connection = http.client.HTTPSConnection( # nosemgrep: python.lang.security.audit.httpsconnection-detected.httpsconnection-detected # noqa: E501 + API_HOST, timeout=30, context=ssl.create_default_context() + ) + try: + connection.request("GET", path, headers=headers) + response = connection.getresponse() + body = response.read() + finally: + connection.close() + if response.status >= 400: + raise GitHubApiError(response.status) + return json.loads(body) + + return fetch + + +def audit_branch(repo: str, branch: str, expected: Dict[str, Any], fetch: Fetcher) -> Dict[str, Any]: + """Audit one branch and return its evidence record.""" + effective_rules = fetch(f"/repos/{repo}/rules/branches/{branch}") or [] + problems = compare_effective(branch, expected, effective_rules) + + ruleset_ids = sorted({rule["ruleset_id"] for rule in effective_rules if rule.get("ruleset_id") is not None}) + rulesets = [fetch(f"/repos/{repo}/rulesets/{ruleset_id}") for ruleset_id in ruleset_ids] + bypass_problems, notes = audit_bypass(branch, rulesets) + problems.extend(bypass_problems) + + # Classic branch protection is not reported by the rules API. Record what + # the branch summary exposes so the evidence shows it, but only rulesets + # count as the declared, auditable state. + classic = (fetch(f"/repos/{repo}/branches/{branch}") or {}).get("protection") or {} + if classic.get("enabled"): + level = (classic.get("required_status_checks") or {}).get("enforcement_level", "unknown") + notes.append( + f"{branch}: classic branch protection is enabled (status-check enforcement: {level}); " + "it is not auditable here and should be replaced by the declared ruleset" + ) + + return { + "branch": branch, + "compliant": not problems, + "problems": problems, + "notes": notes, + "effective_rules": effective_rules, + "ruleset_ids": ruleset_ids, + } + + +def main(argv: Optional[List[str]] = None, fetch: Optional[Fetcher] = None) -> int: + parser = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) + parser.add_argument("--repo", default=os.environ.get("GITHUB_REPOSITORY", "OWASP/openshield")) + parser.add_argument("--branch", action="append", dest="branches", help="branch to audit (repeatable)") + parser.add_argument("--config-dir", type=Path, default=DEFAULT_CONFIG_DIR) + parser.add_argument("--evidence", type=Path, help="write a JSON evidence record to this path") + parser.add_argument("--validate-only", action="store_true", help="only validate the declared rulesets") + args = parser.parse_args(argv) + + branches = args.branches or list(DEFAULT_BRANCHES) + declared_problems: List[str] = [] + expected: Dict[str, Dict[str, Any]] = {} + for branch in branches: + try: + expected[branch] = load_expected(args.config_dir, branch) + except (OSError, json.JSONDecodeError) as exc: + declared_problems.append(f"{branch}: cannot load declared ruleset: {exc}") + continue + declared_problems.extend(validate_declared(branch, expected[branch])) + + if declared_problems: + for problem in declared_problems: + print(f"DECLARED: {problem}") + return 1 + if args.validate_only: + print(f"Declared rulesets are valid for: {', '.join(branches)}") + return 0 + + fetch = fetch or github_fetcher(os.environ.get("GITHUB_TOKEN")) + records = [] + for branch in branches: + try: + records.append(audit_branch(args.repo, branch, expected[branch], fetch)) + except GitHubApiError as exc: + records.append( + {"branch": branch, "compliant": False, "problems": [f"{branch}: GitHub API error {exc.status}"]} + ) + except (OSError, http.client.HTTPException, ValueError) as exc: + # An unreachable API or unreadable response is not evidence of + # protection: fail closed. + records.append( + {"branch": branch, "compliant": False, "problems": [f"{branch}: GitHub API unreachable: {exc}"]} + ) + + compliant = all(record["compliant"] for record in records) + for record in records: + status = "OK" if record["compliant"] else "DRIFT" + print(f"[{status}] {args.repo}@{record['branch']}") + for problem in record["problems"]: + print(f" - {problem}") + for note in record.get("notes", []): + print(f" ! {note}") + + if args.evidence: + evidence = { + "repository": args.repo, + "checked_at": datetime.now(timezone.utc).isoformat(), + "compliant": compliant, + "branches": records, + } + args.evidence.write_text(json.dumps(evidence, indent=2) + "\n", encoding="utf-8") + + return 0 if compliant else 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_check_branch_protection.py b/tests/test_check_branch_protection.py new file mode 100644 index 0000000..7271a6d --- /dev/null +++ b/tests/test_check_branch_protection.py @@ -0,0 +1,187 @@ +"""Tests for the branch-protection drift audit (issue #298).""" + +import copy +import json +from pathlib import Path + +import pytest +import yaml + +from scripts import check_branch_protection as audit + +REPO_ROOT = Path(__file__).parents[1] +CONFIG_DIR = REPO_ROOT / ".github" / "branch-protection" + + +def _declared(branch): + return audit.load_expected(CONFIG_DIR, branch) + + +def _effective_from_declared(branch, ruleset_id=7): + """Rules as GET /rules/branches/{branch} reports them when fully applied.""" + rules = copy.deepcopy(_declared(branch)["rules"]) + for rule in rules: + rule["ruleset_source_type"] = "Repository" + rule["ruleset_id"] = ruleset_id + return rules + + +def _fetcher(rules_by_branch, ruleset_detail=None): + detail = ruleset_detail if ruleset_detail is not None else {"id": 7, "name": "protection", "bypass_actors": []} + + def fetch(path): + if "/rules/branches/" in path: + return rules_by_branch[path.rsplit("/", 1)[1]] + if "/rulesets/" in path: + return detail + if "/branches/" in path: + return {"protection": {"enabled": True, "required_status_checks": {"enforcement_level": "off"}}} + raise AssertionError(f"unexpected path {path}") + + return fetch + + +@pytest.mark.parametrize("branch", ["dev", "main"]) +def test_committed_rulesets_are_valid(branch): + assert audit.validate_declared(branch, _declared(branch)) == [] + + +def test_main_requires_two_person_review_and_dev_one(): + def approvals(branch): + rule = next(r for r in _declared(branch)["rules"] if r["type"] == "pull_request") + return rule["parameters"]["required_approving_review_count"] + + assert approvals("dev") >= 1 + assert approvals("main") >= 2 + + +@pytest.mark.parametrize("branch", ["dev", "main"]) +def test_required_checks_match_real_workflow_job_names(branch): + workflows = REPO_ROOT / ".github" / "workflows" + names = set() + for path in workflows.glob("*.yml"): + for job_id, job in (yaml.safe_load(path.read_text(encoding="utf-8")).get("jobs") or {}).items(): + name = job.get("name", job_id) + languages = ((job.get("strategy") or {}).get("matrix") or {}).get("language") + if languages and "${{ matrix.language }}" in name: + names.update(name.replace("${{ matrix.language }}", lang) for lang in languages) + else: + names.add(name) + + rule = next(r for r in _declared(branch)["rules"] if r["type"] == "required_status_checks") + missing = [c["context"] for c in rule["parameters"]["required_status_checks"] if c["context"] not in names] + assert missing == [], "required checks must name jobs that actually run, or merges block forever" + + +def test_declared_validation_rejects_weakened_rulesets(): + ruleset = _declared("main") + ruleset["bypass_actors"] = [{"actor_type": "OrganizationAdmin", "actor_id": 1, "bypass_mode": "always"}] + for rule in ruleset["rules"]: + if rule["type"] == "pull_request": + rule["parameters"]["required_approving_review_count"] = 1 + rule["parameters"]["require_last_push_approval"] = False + if rule["type"] == "required_status_checks": + rule["parameters"]["strict_required_status_checks_policy"] = False + + problems = audit.validate_declared("main", ruleset) + assert any("bypass" in p for p in problems) + assert any("fewer than 2 approvals" in p for p in problems) + assert any("require_last_push_approval" in p for p in problems) + assert any("strict" in p for p in problems) + + +def test_fully_applied_rules_are_compliant(): + record = audit.audit_branch("o/r", "main", _declared("main"), _fetcher({"main": _effective_from_declared("main")})) + assert record["compliant"] is True + assert record["problems"] == [] + + +def test_unprotected_branch_is_reported_as_drift(): + """The state observed on 2026-08-21: protected flag set but no rules enforced.""" + record = audit.audit_branch("o/r", "dev", _declared("dev"), _fetcher({"dev": []})) + assert record["compliant"] is False + assert "dev: 'required_status_checks' rule is not enforced" in record["problems"] + assert "dev: 'pull_request' rule is not enforced" in record["problems"] + + +def test_missing_check_non_strict_and_self_approval_are_drift(): + rules = _effective_from_declared("main") + for rule in rules: + if rule["type"] == "required_status_checks": + rule["parameters"]["strict_required_status_checks_policy"] = False + rule["parameters"]["required_status_checks"] = [ + c for c in rule["parameters"]["required_status_checks"] if c["context"] != "CI Summary" + ] + if rule["type"] == "pull_request": + rule["parameters"]["required_approving_review_count"] = 1 + rule["parameters"]["require_last_push_approval"] = False + + problems = audit.compare_effective("main", _declared("main"), rules) + assert "main: required status check 'CI Summary' is not enforced" in problems + assert "main: status checks are not strict; a stale head can merge" in problems + assert "main: requires 1 approving review(s), expected at least 2" in problems + assert "main: pull_request rule does not enforce require_last_push_approval" in problems + + +def test_standing_bypass_actor_is_drift(): + detail = { + "id": 7, + "name": "main", + "bypass_actors": [{"actor_type": "RepositoryRole", "actor_id": 5, "bypass_mode": "always"}], + } + record = audit.audit_branch( + "o/r", "main", _declared("main"), _fetcher({"main": _effective_from_declared("main")}, detail) + ) + assert record["compliant"] is False + assert any("standing bypass" in p for p in record["problems"]) + + +def test_hidden_bypass_actors_are_unverified_not_compliant_evidence(): + detail = {"id": 7, "name": "main"} + record = audit.audit_branch( + "o/r", "main", _declared("main"), _fetcher({"main": _effective_from_declared("main")}, detail) + ) + assert record["compliant"] is True + assert any("unverified" in note for note in record["notes"]) + + +def test_main_writes_evidence_and_fails_on_drift(tmp_path, capsys): + evidence = tmp_path / "evidence.json" + fetch = _fetcher({"dev": _effective_from_declared("dev"), "main": []}) + + code = audit.main(["--repo", "o/r", "--evidence", str(evidence)], fetch=fetch) + + assert code == 1 + record = json.loads(evidence.read_text(encoding="utf-8")) + assert record["compliant"] is False + assert [b["branch"] for b in record["branches"]] == ["dev", "main"] + assert "[DRIFT] o/r@main" in capsys.readouterr().out + + +def test_validate_only_does_not_call_the_api(): + def fetch(_path): + raise AssertionError("validate-only must stay offline") + + assert audit.main(["--validate-only"], fetch=fetch) == 0 + + +def test_classic_protection_is_noted_but_not_counted_as_enforcement(): + record = audit.audit_branch("o/r", "dev", _declared("dev"), _fetcher({"dev": []})) + assert record["compliant"] is False + assert any("classic branch protection is enabled (status-check enforcement: off)" in n for n in record["notes"]) + + +def test_unreachable_api_fails_closed(capsys): + def fetch(_path): + raise OSError("offline") + + assert audit.main(["--repo", "o/r"], fetch=fetch) == 1 + assert "GitHub API unreachable" in capsys.readouterr().out + + +def test_api_error_status_fails_closed(capsys): + def fetch(_path): + raise audit.GitHubApiError(403) + + assert audit.main(["--repo", "o/r", "--branch", "dev"], fetch=fetch) == 1 + assert "GitHub API error 403" in capsys.readouterr().out