From 0dbcc7f5c1d72f22cbe568329f8bef151bf4c372 Mon Sep 17 00:00:00 2001 From: Michael Lieberman Date: Sat, 22 Aug 2026 21:29:03 -0400 Subject: [PATCH] feat(baseline): consult GitHub Rulesets so branch-protection controls don't false-FAIL (closes #343) Feature 019 taught the sieve to treat a 404 from `/repos/{owner}/{repo}/branches/{branch}/protection` as a definitive FAIL for the four branch-protection controls (OSPS-AC-03.01, OSPS-AC-03.02, OSPS-QA-03.01, OSPS-QA-07.01). Repository Rulesets -- the newer protection mechanism -- also produce a 404 on that endpoint while genuinely protecting the branch, so the shipped fix produces false FAILs for repos protected via rulesets. Reported by @justaugustus as a follow-up to the 019 fix. Adds a new sieve handler `github_branch_protection` registered by `darnit-baseline` that reconciles the two surfaces: - Classic 200 with the required signal -> PASS from classic (rulesets not consulted; fast path). - Classic did not carry the signal (404 or 200 without the specific field) -> consult rulesets. If any active ruleset whose `conditions.ref_name` covers the audited branch carries a rule of the right type/parameter -> PASS from ruleset. - Both surfaces respond and neither protects -> FAIL. Locks the 019 invariant on the true-negative path. - Either surface returns 401/403/429/5xx or a mid-pagination fetch fails -> INCONCLUSIVE, which falls through the trailing manual pass to WARN. Preserves the "WARN means unknown" semantic on ambiguous responses. Two-surface layering (Q1 clarification): a repo whose classic protection requires PRs but delegates status-checks to a ruleset correctly PASSes OSPS-QA-03.01 via the ruleset without changing the other controls' verdicts. Adds a shared helper `gh_api_with_status` in `darnit.core.utils` that parses `HTTP :` from `gh`'s stderr on non-zero exit so callers can distinguish 404 from 403 from 5xx. Existing `gh_api()` and `gh_api_safe()` are refactored as thin wrappers preserving their exact call contracts (~30 existing callers unchanged). Uses `gh api --paginate` for the rulesets list so repos with more rulesets than a single page get full enumeration; a mid-page fetch failure resolves the affected control to WARN with source `partial-fetch`, not a silent truncation. No new runtime dependencies. Zero touches under `packages/(darnit-gittuf|darnit-reproducibility|darnit-hello)/src/`. The default-branch value used for `~DEFAULT_BRANCH` include-list matching is consumed from `HandlerContext.default_branch` (populated by the audit driver) -- no extra `GET /repos/{owner}/{repo}` call is introduced, matching SC-004's exact API-call budget. Test coverage: - 48 handler unit tests (ruleset matching, PASS/FAIL/WARN paths per requirement, config validation). - 19 unit tests for `gh_api_with_status` (status parsing across 2xx/4xx/5xx, paginate flag, wrapper contracts). - 7 integration tests through the sieve orchestrator (one per TOML control PASS via ruleset, one FAIL when no protection, one exact API-call budget assertion, one WARN via manual fallback). - 8 pre-existing feature-019 tests updated to the two-surface semantics without weakening FAIL invariants. Full workspace: 2815 pass, 17 skip, 0 fail. Ruff clean. `validate_sync.py` PASS. Structure decision and FR-013 no-new-dep guards satisfied. Non-goals deferred to v0.1: - Organization-level inherited rulesets (repo-level check only). - Evaluate-mode rulesets (only `enforcement = "active"` counts). - Glob-pattern ref-name matching (globs treated as non-matching and surfaced in `considered_rulesets` for the operator). Spec: `specs/032-ruleset-branch-protection/` (63 tasks, all closed). Closes #343. --- .specify/feature.json | 2 +- CLAUDE.md | 2 +- docs/architecture/framework-design.md | 2 + .../src/darnit_baseline/branch_protection.py | 586 ++++++++++++++++ .../src/darnit_baseline/implementation.py | 11 + .../src/darnit_baseline/openssf-baseline.toml | 33 +- packages/darnit/src/darnit/core/utils.py | 80 ++- .../checklists/requirements.md | 40 ++ .../github-branch-protection-handler.md | 117 ++++ .../data-model.md | 167 +++++ specs/032-ruleset-branch-protection/plan.md | 194 ++++++ .../quickstart.md | 150 +++++ .../032-ruleset-branch-protection/research.md | 114 ++++ specs/032-ruleset-branch-protection/spec.md | 138 ++++ specs/032-ruleset-branch-protection/tasks.md | 282 ++++++++ tests/darnit/core/test_gh_api_status.py | 144 ++++ .../controls/test_branch_protection.py | 164 ++--- .../test_branch_protection_handler.py | 628 ++++++++++++++++++ .../test_branch_protection_integration.py | 187 ++++++ .../test_handler_dispatch_integration.py | 5 + 20 files changed, 2907 insertions(+), 139 deletions(-) create mode 100644 packages/darnit-baseline/src/darnit_baseline/branch_protection.py create mode 100644 specs/032-ruleset-branch-protection/checklists/requirements.md create mode 100644 specs/032-ruleset-branch-protection/contracts/github-branch-protection-handler.md create mode 100644 specs/032-ruleset-branch-protection/data-model.md create mode 100644 specs/032-ruleset-branch-protection/plan.md create mode 100644 specs/032-ruleset-branch-protection/quickstart.md create mode 100644 specs/032-ruleset-branch-protection/research.md create mode 100644 specs/032-ruleset-branch-protection/spec.md create mode 100644 specs/032-ruleset-branch-protection/tasks.md create mode 100644 tests/darnit/core/test_gh_api_status.py create mode 100644 tests/darnit_baseline/test_branch_protection_handler.py create mode 100644 tests/darnit_baseline/test_branch_protection_integration.py diff --git a/.specify/feature.json b/.specify/feature.json index 2270c5c0..1a9cbc2d 100644 --- a/.specify/feature.json +++ b/.specify/feature.json @@ -1 +1 @@ -{"feature_directory": "specs/031-mcp-server-handler"} +{"feature_directory": "specs/032-ruleset-branch-protection"} diff --git a/CLAUDE.md b/CLAUDE.md index 74f21f77..a4b91993 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -381,5 +381,5 @@ else: For additional context about technologies to be used, project structure, shell commands, and other important information, read the current plan: -[`specs/031-mcp-server-handler/plan.md`](specs/031-mcp-server-handler/plan.md) +[`specs/032-ruleset-branch-protection/plan.md`](specs/032-ruleset-branch-protection/plan.md) diff --git a/docs/architecture/framework-design.md b/docs/architecture/framework-design.md index 02e4833a..14e9ac6f 100644 --- a/docs/architecture/framework-design.md +++ b/docs/architecture/framework-design.md @@ -1116,6 +1116,8 @@ Project context from `.project/` SHALL be used to inform WHERE the sieve looks f The framework SHALL provide a handler registry where handlers are registered by name with a phase affinity. Core SHALL register built-in handlers: `file_exists`, `exec`, `regex`, `llm_eval`, `manual_steps`, `file_create`, `api_call`, `project_update`. Implementations SHALL register domain-specific handlers via the existing `ComplianceImplementation.register_handlers()` method. +Implementation-registered sieve handlers (non-exhaustive): `github_branch_protection` (registered by `darnit-baseline`, encapsulates the classic-branch-protection + repository-rulesets two-surface check for `OSPS-AC-03.01`, `OSPS-AC-03.02`, `OSPS-QA-03.01`, `OSPS-QA-07.01`; see `specs/032-ruleset-branch-protection/contracts/github-branch-protection-handler.md`). + A handler used in a phase different from its registered affinity SHALL trigger a warning but still execute. ## Appendix C: Removed Requirements diff --git a/packages/darnit-baseline/src/darnit_baseline/branch_protection.py b/packages/darnit-baseline/src/darnit_baseline/branch_protection.py new file mode 100644 index 00000000..2f4a2fee --- /dev/null +++ b/packages/darnit-baseline/src/darnit_baseline/branch_protection.py @@ -0,0 +1,586 @@ +"""Ruleset-aware branch-protection verdict handler. + +This module registers the ``github_branch_protection`` sieve handler used by +the four OSPS Baseline branch-protection controls +(``OSPS-AC-03.01``, ``OSPS-AC-03.02``, ``OSPS-QA-03.01``, ``OSPS-QA-07.01``). +It encapsulates the two-surface check that reconciles GitHub's classic +branch-protection API (``/repos/{owner}/{repo}/branches/{branch}/protection``) +with the newer repository-rulesets API (``/repos/{owner}/{repo}/rulesets`` +and its per-ruleset detail endpoint) so a repository protected exclusively +via a ruleset resolves PASS rather than FAILing on the classic 404. + +Verdict semantics: + +* PASS when the classic surface carries the specific required signal (fast + path -- rulesets NOT consulted). +* PASS when the classic surface did not carry the signal but an active + repository ruleset that targets the audited branch carries the signal. +* FAIL when both surfaces respond successfully and neither carries the + signal (locks feature 019's shipped invariant on the true-negative path). +* INCONCLUSIVE (which the trailing manual pass promotes to WARN) when + either surface returns 401/403/429/5xx or a mid-pagination fetch fails. + +The default-branch value used for ``~DEFAULT_BRANCH`` include-list matching +is consumed from ``HandlerContext.default_branch``, which the audit driver +populates. This feature does NOT introduce a ``GET /repos/{owner}/{repo}`` +call to resolve it (SC-004's API-call budget invariant). + +The sandboxing follow-up (issue #375, feature 031's mcp handler) shares no +state with this module. The ``github_branch_protection`` handler is +stateless: every audit-run's invocation makes fresh API calls; there is no +persistent cache. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass +from enum import Enum +from typing import Any, Literal, TypedDict + +from darnit.core.utils import gh_api_with_status +from darnit.sieve.handler_registry import ( + HandlerContext, + HandlerResult, + HandlerResultStatus, +) + +logger = logging.getLogger(__name__) + + +# ============================================================================= +# Constants +# ============================================================================= + +MAX_CONSIDERED_RULESETS: int = 20 +"""Cap on how many non-satisfying rulesets are enumerated in the evidence +record on FAIL. Research decision R-007. Overflow is tracked separately +via `considered_rulesets_truncated`.""" + +DEFAULT_TIMEOUT_SECONDS: int = 30 +"""Default per-handler-invocation time budget in seconds.""" + +SUPPORTED_REF_INCLUDE_LITERALS: frozenset[str] = frozenset( + {"~DEFAULT_BRANCH", "~ALL"} +) +"""Ruleset ``conditions.ref_name.include`` pseudo-refs the matcher +understands beyond exact branch names and ``refs/heads/``.""" + +_GLOB_METACHARS: frozenset[str] = frozenset({"*", "?", "["}) + + +# ============================================================================= +# Enums (public within this module) +# ============================================================================= + + +class ProtectionRequirement(str, Enum): + """The specific protection a control tests for. + + Set via TOML ``requirement = "..."`` on a + ``handler = "github_branch_protection"`` pass. See + ``specs/032-ruleset-branch-protection/data-model.md`` for the full + requirement-to-signal mapping. + """ + + REQUIRE_PULL_REQUEST = "require_pull_request" + PREVENT_DELETION = "prevent_deletion" + REQUIRE_STATUS_CHECKS = "require_status_checks" + REQUIRE_APPROVALS = "require_approvals" + + +class VerdictSource(str, Enum): + """The enumerated ``source`` value recorded in the evidence record. + + Locked by spec FR-016. Downstream reporting groups verdicts by source + so a maintainer can distinguish "we have PASS via classic" from "we + have PASS via ruleset" from "we could not tell either way." + """ + + CLASSIC = "classic" + RULESET = "ruleset" + NEITHER_SURFACE_PROVIDED_PROTECTION = "neither-surface-provided-protection" + INSUFFICIENT_ACCESS = "insufficient-access" + PARTIAL_FETCH = "partial-fetch" + + +# ============================================================================= +# TypedDicts (private -- describe API-response shapes we consume) +# ============================================================================= + + +class _RefNameConditions(TypedDict, total=False): + include: list[str] + exclude: list[str] + + +class _RulesetConditions(TypedDict, total=False): + ref_name: _RefNameConditions + + +class _RulesetRule(TypedDict, total=False): + type: str + parameters: dict[str, Any] + + +class _RulesetDetail(TypedDict, total=False): + id: int + name: str + enforcement: Literal["active", "evaluate", "disabled"] + conditions: _RulesetConditions + rules: list[_RulesetRule] + + +class _RulesetSummary(TypedDict, total=False): + id: int + name: str + target: Literal["branch", "tag"] + enforcement: Literal["active", "evaluate", "disabled"] + + +# ============================================================================= +# Internal helpers +# ============================================================================= + + +def _ref_name_matches( + branch: str, + default_branch: str | None, + include: list[str] | None, + exclude: list[str] | None, +) -> bool: + """Return True iff at least one ``include`` covers ``branch`` and no ``exclude`` does. + + Match semantics (research R-003): + + * ``~DEFAULT_BRANCH`` matches iff ``default_branch is not None`` AND + ``branch == default_branch``. When ``default_branch is None``, this + pseudo-ref is treated as non-matching (Constitution II). + * ``~ALL`` matches always. + * Bare ```` matches iff equal to ``branch``. + * ``refs/heads/`` matches iff `` == branch``. + * Any entry containing a glob metacharacter (``*``, ``?``, ``[``) + returns False (v0 limitation; documented in the spec). + """ + include_list = include or [] + exclude_list = exclude or [] + if not any(_ref_matches(entry, branch, default_branch) for entry in include_list): + return False + return not any(_ref_matches(entry, branch, default_branch) for entry in exclude_list) + + +def _ref_matches(entry: str, branch: str, default_branch: str | None) -> bool: + if not isinstance(entry, str): + return False + if any(ch in entry for ch in _GLOB_METACHARS): + return False + if entry == "~DEFAULT_BRANCH": + return default_branch is not None and branch == default_branch + if entry == "~ALL": + return True + if entry.startswith("refs/heads/"): + return entry[len("refs/heads/") :] == branch + return entry == branch + + +def _ruleset_satisfies( + rule: _RulesetRule, + requirement: ProtectionRequirement, + minimum: int, +) -> tuple[bool, str]: + """Return (satisfied, reason). On no-match, ``reason`` explains why. + + Mirrors the satisfying-signal table in + ``specs/032-ruleset-branch-protection/data-model.md``. + """ + rule_type = rule.get("type", "") + if requirement is ProtectionRequirement.REQUIRE_PULL_REQUEST: + if rule_type == "pull_request": + return True, "" + return False, f"rule type is {rule_type!r}, need 'pull_request'" + if requirement is ProtectionRequirement.PREVENT_DELETION: + if rule_type == "deletion": + return True, "" + return False, f"rule type is {rule_type!r}, need 'deletion'" + if requirement is ProtectionRequirement.REQUIRE_STATUS_CHECKS: + if rule_type == "required_status_checks": + return True, "" + return False, f"rule type is {rule_type!r}, need 'required_status_checks'" + if requirement is ProtectionRequirement.REQUIRE_APPROVALS: + if rule_type != "pull_request": + return False, f"rule type is {rule_type!r}, need 'pull_request'" + params = rule.get("parameters") or {} + count = params.get("required_approving_review_count", 0) + try: + count_i = int(count) + except (TypeError, ValueError): + count_i = 0 + if count_i >= minimum: + return True, "" + return ( + False, + f"pull_request rule but required_approving_review_count is {count_i}, " + f"need >= {minimum}", + ) + # Defensive: unreachable if the caller validated the requirement enum. + return False, f"unknown requirement {requirement!r}" + + +def _classic_carries_signal( + body: dict[str, Any], requirement: ProtectionRequirement, minimum: int +) -> bool: + """Return True iff the classic branch-protection response body carries the required signal.""" + if requirement is ProtectionRequirement.REQUIRE_PULL_REQUEST: + return body.get("required_pull_request_reviews") is not None + if requirement is ProtectionRequirement.PREVENT_DELETION: + allow_deletions = body.get("allow_deletions") or {} + return allow_deletions.get("enabled") is False + if requirement is ProtectionRequirement.REQUIRE_STATUS_CHECKS: + return body.get("required_status_checks") is not None + if requirement is ProtectionRequirement.REQUIRE_APPROVALS: + reviews = body.get("required_pull_request_reviews") or {} + count = reviews.get("required_approving_review_count", 0) + try: + return int(count) >= minimum + except (TypeError, ValueError): + return False + return False + + +@dataclass +class _ClassicResult: + satisfied: bool + status: int + error: str + + +def _query_classic( + owner: str, + repo: str, + branch: str, + requirement: ProtectionRequirement, + minimum: int, +) -> _ClassicResult: + """Query the classic branch-protection endpoint; return (satisfied, status, error).""" + endpoint = f"/repos/{owner}/{repo}/branches/{branch}/protection" + body, status, error = gh_api_with_status(endpoint) + if status == 200 and isinstance(body, dict): + return _ClassicResult( + satisfied=_classic_carries_signal(body, requirement, minimum), + status=200, + error="", + ) + return _ClassicResult(satisfied=False, status=status, error=error) + + +@dataclass +class _RulesetsResult: + source: VerdictSource + status: int + matched: dict[str, Any] | None + considered: list[dict[str, Any]] + truncated: int + error: str + + +def _query_rulesets( + owner: str, + repo: str, + branch: str, + default_branch: str | None, + requirement: ProtectionRequirement, + minimum: int, +) -> _RulesetsResult: + """Query the rulesets endpoint and evaluate whether any ruleset satisfies. + + Returns a :class:`_RulesetsResult` whose ``source`` field carries the + verdict-source enum. ``matched`` is populated on ``RULESET``; + ``considered`` / ``truncated`` on ``NEITHER_SURFACE_PROVIDED_PROTECTION``. + """ + list_endpoint = f"/repos/{owner}/{repo}/rulesets" + body, status, error = gh_api_with_status(list_endpoint, paginate=True) + if status != 200 or not isinstance(body, list): + return _RulesetsResult( + source=VerdictSource.INSUFFICIENT_ACCESS, + status=status, + matched=None, + considered=[], + truncated=0, + error=error, + ) + + considered_all: list[dict[str, Any]] = [] + for summary in body: + if not isinstance(summary, dict): + continue + if summary.get("enforcement") != "active": + continue + ruleset_id = summary.get("id") + if ruleset_id is None: + continue + detail_endpoint = f"/repos/{owner}/{repo}/rulesets/{ruleset_id}" + detail, detail_status, detail_error = gh_api_with_status(detail_endpoint) + if detail_status != 200 or not isinstance(detail, dict): + return _RulesetsResult( + source=VerdictSource.PARTIAL_FETCH, + status=detail_status, + matched=None, + considered=[], + truncated=0, + error=f"failed to fetch ruleset {ruleset_id} detail: {detail_error or 'HTTP ' + str(detail_status)}", + ) + if detail.get("enforcement") != "active": + continue + conditions = detail.get("conditions") or {} + ref_name = conditions.get("ref_name") or {} + if not _ref_name_matches( + branch, default_branch, ref_name.get("include"), ref_name.get("exclude") + ): + considered_all.append( + { + "id": detail.get("id", ruleset_id), + "name": detail.get("name", summary.get("name", "?")), + "reason": "ref_name conditions do not cover branch " + f"{branch!r}", + } + ) + continue + rules = detail.get("rules") or [] + if not rules: + considered_all.append( + { + "id": detail.get("id", ruleset_id), + "name": detail.get("name", summary.get("name", "?")), + "reason": "no rules declared", + } + ) + continue + first_reason = "" + satisfied = False + for rule in rules: + ok, reason = _ruleset_satisfies(rule, requirement, minimum) + if ok: + satisfied = True + break + if not first_reason: + first_reason = reason + if satisfied: + return _RulesetsResult( + source=VerdictSource.RULESET, + status=200, + matched={ + "id": detail.get("id", ruleset_id), + "name": detail.get("name", summary.get("name", "?")), + }, + considered=[], + truncated=0, + error="", + ) + considered_all.append( + { + "id": detail.get("id", ruleset_id), + "name": detail.get("name", summary.get("name", "?")), + "reason": first_reason or "no matching rule type", + } + ) + + truncated = max(0, len(considered_all) - MAX_CONSIDERED_RULESETS) + return _RulesetsResult( + source=VerdictSource.NEITHER_SURFACE_PROVIDED_PROTECTION, + status=200, + matched=None, + considered=considered_all[:MAX_CONSIDERED_RULESETS], + truncated=truncated, + error="", + ) + + +# ============================================================================= +# Handler entry point +# ============================================================================= + + +_AMBIGUOUS_STATUSES = frozenset({0, 401, 403, 429}) + + +def github_branch_protection_handler( + config: dict[str, Any], context: HandlerContext +) -> HandlerResult: + """Sieve handler that reconciles classic branch protection with repository rulesets. + + Config surface documented in + ``specs/032-ruleset-branch-protection/contracts/github-branch-protection-handler.md``. + """ + raw_requirement = config.get("requirement") + if not isinstance(raw_requirement, str) or not raw_requirement: + return HandlerResult( + status=HandlerResultStatus.ERROR, + message="handler github_branch_protection requires 'requirement' field", + ) + try: + requirement = ProtectionRequirement(raw_requirement) + except ValueError: + return HandlerResult( + status=HandlerResultStatus.ERROR, + message=f"unknown requirement {raw_requirement!r}", + ) + + minimum_raw = config.get("required_approvals_minimum", 1) + try: + minimum = int(minimum_raw) + except (TypeError, ValueError): + return HandlerResult( + status=HandlerResultStatus.ERROR, + message="required_approvals_minimum must be an integer", + ) + if not (1 <= minimum <= 10): + return HandlerResult( + status=HandlerResultStatus.ERROR, + message="required_approvals_minimum must be 1..10", + ) + + owner = str(config.get("owner") or context.owner or "").strip() + repo = str(config.get("repo") or context.repo or "").strip() + branch = str(config.get("branch") or context.default_branch or "main").strip() + if not owner or not repo: + return HandlerResult( + status=HandlerResultStatus.ERROR, + message="handler github_branch_protection needs owner/repo (from context or config)", + ) + + # Default-branch value used for ~DEFAULT_BRANCH matching is consumed + # from the audit driver's HandlerContext. This feature does NOT make + # an extra `GET /repos/{owner}/{repo}` call (SC-004 budget). + default_branch: str | None = context.default_branch or None + + # Step 1: classic surface + classic = _query_classic(owner, repo, branch, requirement, minimum) + if classic.satisfied: + return HandlerResult( + status=HandlerResultStatus.PASS, + message=( + f"branch {branch!r} protected via classic branch-protection " + f"({requirement.value})" + ), + confidence=1.0, + evidence={ + "source": VerdictSource.CLASSIC.value, + "requirement": requirement.value, + "classic_status": 200, + }, + ) + + # Ambiguous classic response -> INCONCLUSIVE without consulting rulesets. + # A 401/403/429/5xx/0 from classic tells us nothing about protection, so + # we cannot conservatively decide FAIL by consulting rulesets alone. + if ( + classic.status in _AMBIGUOUS_STATUSES + or classic.status >= 500 + ): + return HandlerResult( + status=HandlerResultStatus.INCONCLUSIVE, + message=_classic_ambiguous_message(classic), + evidence={ + "source": VerdictSource.INSUFFICIENT_ACCESS.value, + "requirement": requirement.value, + "classic_status": classic.status, + }, + ) + + # Classic returned a definitive negative signal (404, or 200 without the + # specific field this control needs). Consult rulesets. + rulesets = _query_rulesets( + owner, repo, branch, default_branch, requirement, minimum + ) + + if rulesets.source is VerdictSource.RULESET: + matched = rulesets.matched or {} + return HandlerResult( + status=HandlerResultStatus.PASS, + message=( + f"branch {branch!r} protected via ruleset " + f"{matched.get('name', '?')!r} (id={matched.get('id')})" + ), + confidence=1.0, + evidence={ + "source": VerdictSource.RULESET.value, + "requirement": requirement.value, + "classic_status": classic.status, + "rulesets_status": 200, + "matched_ruleset": matched, + }, + ) + + if rulesets.source is VerdictSource.NEITHER_SURFACE_PROVIDED_PROTECTION: + return HandlerResult( + status=HandlerResultStatus.FAIL, + message=( + f"neither classic branch protection nor any active ruleset " + f"provides {requirement.value} on branch {branch!r}" + ), + confidence=1.0, + evidence={ + "source": VerdictSource.NEITHER_SURFACE_PROVIDED_PROTECTION.value, + "requirement": requirement.value, + "classic_status": classic.status, + "rulesets_status": 200, + "considered_rulesets": rulesets.considered, + "considered_rulesets_truncated": rulesets.truncated, + }, + ) + + # PARTIAL_FETCH or INSUFFICIENT_ACCESS from rulesets -> WARN + return HandlerResult( + status=HandlerResultStatus.INCONCLUSIVE, + message=_rulesets_ambiguous_message(rulesets), + evidence={ + "source": rulesets.source.value, + "requirement": requirement.value, + "classic_status": classic.status, + "rulesets_status": rulesets.status, + }, + ) + + +def _classic_ambiguous_message(classic: _ClassicResult) -> str: + if classic.status == 401 or classic.status == 403: + return ( + f"insufficient permissions to read classic branch protection " + f"(HTTP {classic.status})" + ) + if classic.status == 429: + return "classic branch-protection endpoint rate-limited (HTTP 429)" + if classic.status >= 500: + return f"classic branch-protection endpoint returned HTTP {classic.status}" + if classic.status == 0: + return classic.error or "classic branch-protection endpoint unreachable" + return f"classic branch-protection endpoint returned HTTP {classic.status}" + + +def _rulesets_ambiguous_message(rulesets: _RulesetsResult) -> str: + if rulesets.source is VerdictSource.PARTIAL_FETCH: + return rulesets.error or ( + f"could not fully enumerate rulesets (HTTP {rulesets.status})" + ) + if rulesets.status in (401, 403): + return ( + f"insufficient permissions to read repository rulesets " + f"(HTTP {rulesets.status})" + ) + if rulesets.status == 429: + return "rulesets endpoint rate-limited (HTTP 429)" + if rulesets.status >= 500: + return f"rulesets endpoint returned HTTP {rulesets.status}" + if rulesets.status == 0: + return rulesets.error or "rulesets endpoint unreachable" + return f"rulesets endpoint returned HTTP {rulesets.status}" + + +__all__ = [ + "DEFAULT_TIMEOUT_SECONDS", + "MAX_CONSIDERED_RULESETS", + "ProtectionRequirement", + "SUPPORTED_REF_INCLUDE_LITERALS", + "VerdictSource", + "github_branch_protection_handler", +] diff --git a/packages/darnit-baseline/src/darnit_baseline/implementation.py b/packages/darnit-baseline/src/darnit_baseline/implementation.py index 9d576a61..d0fd3a91 100644 --- a/packages/darnit-baseline/src/darnit_baseline/implementation.py +++ b/packages/darnit-baseline/src/darnit_baseline/implementation.py @@ -219,6 +219,7 @@ def register_handlers(self) -> None: # Register sieve remediation handlers from darnit.sieve.handler_registry import get_sieve_handler_registry + from .branch_protection import github_branch_protection_handler from .threat_model.remediation import generate_threat_model_handler sieve_registry = get_sieve_handler_registry() @@ -232,6 +233,16 @@ def register_handlers(self) -> None: # truth (file produced or not). Explicitly dispositive. default_authority="dispositive", ) + # Feature 032: ruleset-aware branch-protection verdict. Observes + # ground truth (queries GitHub for protection state) so results + # are dispositive by default. + sieve_registry.register( + "github_branch_protection", + phase="deterministic", + handler_fn=github_branch_protection_handler, + description="Ruleset-aware branch-protection verdict", + default_authority="dispositive", + ) sieve_registry.set_plugin_context(None) diff --git a/packages/darnit-baseline/src/darnit_baseline/openssf-baseline.toml b/packages/darnit-baseline/src/darnit_baseline/openssf-baseline.toml index bef46ef5..75738676 100644 --- a/packages/darnit-baseline/src/darnit_baseline/openssf-baseline.toml +++ b/packages/darnit-baseline/src/darnit_baseline/openssf-baseline.toml @@ -629,12 +629,8 @@ Requires the `gh` CLI to be authenticated with a token holding `repo` scope. location_hint = ".github/settings.yml" [[controls."OSPS-AC-03.01".passes]] -handler = "exec" -command = ["gh", "api", "/repos/$OWNER/$REPO/branches/$BRANCH/protection"] -pass_exit_codes = [0] -fail_exit_codes = [1] -output_format = "json" -expr = 'has(output.json.required_pull_request_reviews)' +handler = "github_branch_protection" +requirement = "require_pull_request" timeout = 30 [[controls."OSPS-AC-03.01".passes]] @@ -690,12 +686,8 @@ Requires the `gh` CLI to be authenticated with a token holding `repo` scope. """ [[controls."OSPS-AC-03.02".passes]] -handler = "exec" -command = ["gh", "api", "/repos/$OWNER/$REPO/branches/$BRANCH/protection"] -pass_exit_codes = [0] -fail_exit_codes = [1] -output_format = "json" -expr = 'has(output.json.allow_deletions) && has(output.json.allow_deletions.enabled) && output.json.allow_deletions.enabled == false' +handler = "github_branch_protection" +requirement = "prevent_deletion" timeout = 30 [[controls."OSPS-AC-03.02".passes]] @@ -2626,12 +2618,8 @@ help_md = """Configure required status checks. """ [[controls."OSPS-QA-03.01".passes]] -handler = "exec" -command = ["gh", "api", "/repos/$OWNER/$REPO/branches/$BRANCH/protection"] -pass_exit_codes = [0] -fail_exit_codes = [1] -output_format = "json" -expr = 'has(output.json.required_status_checks)' +handler = "github_branch_protection" +requirement = "require_status_checks" timeout = 30 [[controls."OSPS-QA-03.01".passes]] @@ -3227,12 +3215,9 @@ help_md = """Require code review approval. """ [[controls."OSPS-QA-07.01".passes]] -handler = "exec" -command = ["gh", "api", "/repos/$OWNER/$REPO/branches/$BRANCH/protection", "--jq", ".required_pull_request_reviews.required_approving_review_count >= 1"] -pass_exit_codes = [0] -fail_exit_codes = [1] -output_format = "text" -expr = 'output.stdout.startsWith("true")' +handler = "github_branch_protection" +requirement = "require_approvals" +required_approvals_minimum = 1 timeout = 30 [[controls."OSPS-QA-07.01".passes]] diff --git a/packages/darnit/src/darnit/core/utils.py b/packages/darnit/src/darnit/core/utils.py index d795eff6..4eb3a35c 100644 --- a/packages/darnit/src/darnit/core/utils.py +++ b/packages/darnit/src/darnit/core/utils.py @@ -16,29 +16,77 @@ "and run 'gh auth login' to authenticate." ) +# Feature 032: `gh` emits HTTP-error lines on stderr with the prefix +# ``HTTP : ``; we parse the first three digits to surface +# the status code to callers that need to distinguish 404 from 403 from +# 5xx. Format is stable across gh 2.x per research decision R-001. +_HTTP_STATUS_RE = re.compile(r"^HTTP (\d{3}):", re.MULTILINE) + + +def gh_api_with_status( + endpoint: str, *, paginate: bool = False +) -> tuple[dict[str, Any] | list[Any] | None, int, str]: + """Execute a GitHub API call via ``gh api`` and return ``(body, status, error)``. + + Contract (feature 032): + + * On 2xx: ``(parsed_json, status_code, "")``. ``parsed_json`` may be a + dict OR a list -- the rulesets endpoint returns a top-level list. + * On non-2xx with a parseable ``HTTP :`` prefix in stderr: + ``(None, status_code, stderr_message)``. + * On subprocess-not-found, JSON-decode failure on a 2xx body, or any + other pre-response failure: ``(None, 0, error_message)``. Callers + MUST treat ``status == 0`` as ambiguous (WARN, not FAIL) per the + Constitution's conservative-by-default posture. + + When ``paginate=True``, ``gh api --paginate`` is invoked; ``gh`` + concatenates all pages into a single JSON array at the top level. + """ + args = ["gh", "api"] + if paginate: + args.append("--paginate") + args.append(endpoint) + try: + result = subprocess.run(args, capture_output=True, text=True) + except FileNotFoundError: + return None, 0, _GH_CLI_MISSING_MESSAGE + + if result.returncode == 0: + try: + body = json.loads(result.stdout) if result.stdout.strip() else None + except json.JSONDecodeError as err: + return None, 0, f"GitHub API returned invalid JSON for {endpoint}: {err}" + return body, 200, "" + + stderr = (result.stderr or "").strip() + match = _HTTP_STATUS_RE.search(stderr) + if match: + status = int(match.group(1)) + return None, status, stderr + return None, 0, stderr or f"gh api failed with exit code {result.returncode}" + def gh_api(endpoint: str) -> dict[str, Any]: """Execute a GitHub API call using the gh CLI. + Preserved contract for existing callers: returns the parsed dict on + 2xx, raises ``RuntimeError`` on any non-2xx or non-dict response. + Implementation is a thin wrapper over :func:`gh_api_with_status`. + Raises: - RuntimeError: If the API call fails or returns invalid JSON. + RuntimeError: If the API call fails, returns invalid JSON, or + returns a non-dict body (e.g., a list from a paginated list + endpoint -- such callers MUST use ``gh_api_with_status`` + directly). """ - try: - result = subprocess.run( - ["gh", "api", endpoint], - capture_output=True, - text=True + body, status, error = gh_api_with_status(endpoint) + if status == 200 and isinstance(body, dict): + return body + if status == 200: + raise RuntimeError( + f"gh api {endpoint}: expected dict body but got {type(body).__name__}" ) - except FileNotFoundError: - raise RuntimeError(_GH_CLI_MISSING_MESSAGE) from None - - if result.returncode != 0: - error_msg = result.stderr.strip() or "Unknown error" - raise RuntimeError(f"gh api failed: {error_msg}") - try: - return json.loads(result.stdout) - except json.JSONDecodeError as e: - raise RuntimeError(f"GitHub API returned invalid JSON for {endpoint}: {e}") from e + raise RuntimeError(f"gh api failed: {error or 'status ' + str(status)}") def gh_api_safe(endpoint: str) -> dict[str, Any] | None: diff --git a/specs/032-ruleset-branch-protection/checklists/requirements.md b/specs/032-ruleset-branch-protection/checklists/requirements.md new file mode 100644 index 00000000..519af237 --- /dev/null +++ b/specs/032-ruleset-branch-protection/checklists/requirements.md @@ -0,0 +1,40 @@ +# Specification Quality Checklist: Ruleset-aware branch-protection verdict + +**Purpose**: Validate specification completeness and quality before proceeding to planning + +**Created**: 2026-08-22 + +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [X] No implementation details (languages, frameworks, APIs) +- [X] Focused on user value and business needs +- [X] Written for non-technical stakeholders +- [X] All mandatory sections completed + +## Requirement Completeness + +- [X] No [NEEDS CLARIFICATION] markers remain +- [X] Requirements are testable and unambiguous +- [X] Success criteria are measurable +- [X] Success criteria are technology-agnostic (no implementation details) +- [X] All acceptance scenarios are defined +- [X] Edge cases are identified +- [X] Scope is clearly bounded +- [X] Dependencies and assumptions identified + +## Feature Readiness + +- [X] All functional requirements have clear acceptance criteria +- [X] User scenarios cover primary flows +- [X] Feature meets measurable outcomes defined in Success Criteria +- [X] No implementation details leak into specification + +## Notes + +- Feature scope is deliberately narrow: four named controls, single new evidence source, no schema changes. +- Content Quality item "No implementation details" is met at the spec-level despite mentioning GitHub API endpoints -- those are UPSTREAM APIs the feature depends on, not internal implementation choices. Consistent with feature 019's spec, which also names the classic protection endpoint. +- One known v0 limitation is documented in Assumptions: organization-level inherited rulesets are out of scope (v0.1 follow-up). Pagination truncation was originally called out here but has been resolved to "use --paginate" via the 2026-08-22 clarification session, so it is no longer a v0 limitation. +- Clarifications recorded 2026-08-22: (Q1) consult-rulesets trigger policy, (Q2) HTTP status distinction, (Q3) pagination behavior. FR-002/FR-003 updated for Q1; FR-013 updated and new FR-017 added for Q2; new FR-018 added and Edge Cases + Assumptions + SC-004 updated for Q3. +- Ready for `/speckit-plan`. diff --git a/specs/032-ruleset-branch-protection/contracts/github-branch-protection-handler.md b/specs/032-ruleset-branch-protection/contracts/github-branch-protection-handler.md new file mode 100644 index 00000000..71173d47 --- /dev/null +++ b/specs/032-ruleset-branch-protection/contracts/github-branch-protection-handler.md @@ -0,0 +1,117 @@ +# Contract: `github_branch_protection` sieve handler + +**Owner**: `packages/darnit-baseline/` (registered under short name `github_branch_protection` by `darnit_baseline.implementation.register_handlers`) + +**Purpose**: Encapsulate the "protection may live in either classic branch-protection OR a repository ruleset" decision so the four OSPS branch-protection controls (`OSPS-AC-03.01`, `OSPS-AC-03.02`, `OSPS-QA-03.01`, `OSPS-QA-07.01`) can consult both surfaces uniformly. + +**Stability**: Handler name and TOML surface are stable within v0. Additive changes (new `requirement` enum members, new evidence fields) are non-breaking; removals or renames require a coordinated spec + TOML update. + +## TOML pass surface + +The handler is invoked declaratively via a TOML pass entry with `handler = "github_branch_protection"`: + +```toml +[[controls."OSPS-AC-03.01".passes]] +handler = "github_branch_protection" +requirement = "require_pull_request" +timeout = 30 +``` + +### Fields + +| Field | Type | Required | Default | Description | +|-------|------|----------|---------|-------------| +| `handler` | string | yes | `"github_branch_protection"` | Set by the TOML `handler = "..."` key. Fixed value for this handler. | +| `requirement` | string | yes | (none) | One of `require_pull_request`, `prevent_deletion`, `require_status_checks`, `require_approvals`. Selects which protection the handler tests for. | +| `owner` | string | no | `$OWNER` | Repository owner. Substituted by the sieve's variable-substitution pass before dispatch. | +| `repo` | string | no | `$REPO` | Repository name. Substituted before dispatch. | +| `branch` | string | no | `$BRANCH` | Branch being audited. Substituted before dispatch; typically resolves to the repository's default branch. | +| `required_approvals_minimum` | integer | no | `1` | Only meaningful when `requirement == "require_approvals"`; ignored otherwise. Range 1..10 inclusive. | +| `timeout` | integer | no | `30` | Total time budget in seconds for both surfaces (including pagination). Applied by wrapping each `gh api` invocation with `--jq . | ` (implicit) and honoured by the handler's overall run loop. | + +### Rejected TOML shapes + +- `handler = "gh_branch_protection"` — the abbreviation contradicts other product-name-spelled handlers in darnit. Use `github_` prefix. +- Placing `requirement` inside a nested `parameters` object — flat is the darnit-handler convention (`file_exists` uses top-level `files`, not `parameters.files`). +- Using CEL `expr` alongside the handler — the handler decides PASS/FAIL/INCONCLUSIVE itself; CEL post-step is a no-op here and MUST NOT be added. + +## Verdict semantics + +| Situation | Control status | Evidence `source` | +|-----------|----------------|-------------------| +| Classic surface 200 AND carries the required signal | PASS | `classic` | +| Classic 404 AND rulesets list 200 AND at least one active ruleset targets branch AND satisfies requirement | PASS | `ruleset` | +| Classic 200 without required signal AND rulesets list 200 AND at least one active ruleset targets branch AND satisfies requirement | PASS | `ruleset` | +| Classic 404 AND rulesets list 200 empty | FAIL | `neither-surface-provided-protection` | +| Classic 404 AND rulesets list 200 non-empty AND no active ruleset targets branch | FAIL | `neither-surface-provided-protection` | +| Classic 200 without required signal AND rulesets list 200 AND no active ruleset satisfies requirement | FAIL | `neither-surface-provided-protection` | +| Classic 401/403 | INCONCLUSIVE (WARN) | `insufficient-access` | +| Rulesets list 401/403 (classic was 404 or lacking-signal) | INCONCLUSIVE (WARN) | `insufficient-access` | +| Classic 429/5xx/network-error | INCONCLUSIVE (WARN) | `insufficient-access` | +| Rulesets list 429/5xx/network-error (classic was 404 or lacking-signal) | INCONCLUSIVE (WARN) | `insufficient-access` | +| Rulesets list 200 but a per-ruleset detail call (or later page) fails | INCONCLUSIVE (WARN) | `partial-fetch` | +| Rulesets list 200 non-empty AND every targeting ruleset is in `enforcement = "evaluate"` or `"disabled"` | FAIL | `neither-surface-provided-protection` (evaluate/disabled rulesets do NOT satisfy, per FR-012) | +| Classic 200 without required signal (e.g., ruleset-only protection has both surfaces) AND classic status is unknown | falls under whichever pattern above applies | (n/a) | + +An INCONCLUSIVE from this handler falls through to the trailing manual-pass in the affected control's pass list, which resolves the control to WARN with human-verification steps. This matches feature 019's semantic and is the reason no CEL post-step is used. + +## Evidence record shape + +The handler writes the following into `HandlerResult.evidence`. Downstream consumers (Markdown formatter, JSON output, SARIF exporter) treat unknown keys as opaque, matching feature 019's evidence-additive posture. + +```json +{ + "source": "classic|ruleset|neither-surface-provided-protection|insufficient-access|partial-fetch", + "requirement": "require_pull_request|prevent_deletion|require_status_checks|require_approvals", + "classic_status": 200, + "rulesets_status": 200, + "matched_ruleset": {"id": 12345, "name": "Protect main"}, + "considered_rulesets": [ + {"id": 67890, "name": "Signed commits only", "reason": "no matching rule type"} + ], + "considered_rulesets_truncated": 0 +} +``` + +Population rules (locked by data-model.md): + +- `source`, `requirement`, `classic_status` always present. +- `rulesets_status` present iff the handler reached the rulesets endpoint (any source except `classic`). +- `matched_ruleset` present iff `source == "ruleset"`. +- `considered_rulesets` present iff `source == "neither-surface-provided-protection"` AND at least one active ruleset targeted the branch. Capped at 20 entries; `considered_rulesets_truncated` records the count of elided entries. + +## Failure-mode table + +Every distinguishable failure and the resulting control status: + +| Failure | Control status | Evidence `source` | Message shape | +|---------|----------------|-------------------|---------------| +| Classic 404, no rulesets targeting branch | FAIL | `neither-surface-provided-protection` | `no branch protection found via classic API or repository rulesets` | +| Classic 200, missing signal; rulesets do not carry it either | FAIL | `neither-surface-provided-protection` | `neither classic branch protection nor any active ruleset requires ` | +| Classic 401/403 | WARN | `insufficient-access` | `insufficient permissions to read classic branch protection (HTTP 403)` | +| Rulesets 401/403 (classic was inconclusive) | WARN | `insufficient-access` | `insufficient permissions to read repository rulesets (HTTP 403)` | +| Classic 5xx / rate limit | WARN | `insufficient-access` | `classic branch-protection endpoint returned HTTP ` | +| Rulesets 5xx / rate limit | WARN | `insufficient-access` | `rulesets endpoint returned HTTP ` | +| Rulesets list 200 but a detail call fails | WARN | `partial-fetch` | `failed to fetch ruleset detail: HTTP ` | +| Rulesets list 200 mid-pagination page fails | WARN | `partial-fetch` | `failed to fetch rulesets page: HTTP ` | +| `gh` CLI missing entirely | WARN | `insufficient-access` | `GitHub CLI (gh) not found. Install it from https://cli.github.com/` | +| Handler config missing `requirement` | ERROR (not INCONCLUSIVE) | (evidence source omitted; this is a control-author bug) | `handler github_branch_protection requires 'requirement' field` | +| Handler config has unknown `requirement` value | ERROR | (as above) | `unknown requirement ''` | +| Handler config has `required_approvals_minimum` outside 1..10 | ERROR | (as above) | `required_approvals_minimum must be 1..10` | + +## Non-goals for v0 + +The following are DELIBERATELY NOT COVERED in v0 and MUST NOT be relied upon: + +- **Organization-level rulesets.** GitHub allows an organization to define rulesets at `/orgs/{org}/rulesets` that repositories inherit. v0 checks only `/repos/{owner}/{repo}/rulesets`; org-level rulesets appear at the repo level ONLY as `source_type: "Organization"` list entries but the detail fetch still goes through the repo endpoint. The rare case of an org-only ruleset applying without being surfaced on the repo endpoint is a v0.1 follow-up (issue to be filed at task time). +- **Evaluate-mode rulesets.** A ruleset in `enforcement = "evaluate"` is a dry-run: GitHub reports what would be blocked but does not block. v0 treats evaluate-mode as "does not protect." Documented in FR-012. +- **Glob-pattern ref-name matching.** Ref-name include lists containing `*`, `?`, or `[` metacharacters are treated as "does not match" and surfaced in `considered_rulesets`. See research decision R-003. +- **Generalization beyond the four OSPS controls.** The handler is a general primitive (any control can call it), but the OSPS TOML changes touch only the four named controls. Adopting the handler for other controls is a future PR. +- **Non-GitHub platforms.** GitLab, Bitbucket, and Gitea protection surfaces are entirely different and are not addressed here. The four controls carry `when = { platform = "github" }`, which excludes them from non-GitHub audits. + +## Contract stability guarantees + +- The TOML `handler = "github_branch_protection"` name is stable within v0. +- The `requirement` enum values (`require_pull_request`, `prevent_deletion`, `require_status_checks`, `require_approvals`) are stable within v0. New members are additive. +- The `VerdictSource` values (`classic`, `ruleset`, `neither-surface-provided-protection`, `insufficient-access`, `partial-fetch`) are stable within v0. New members are additive. +- The evidence-record fields listed above are stable within v0. Additional fields may be added; existing fields will not be removed or renamed without a major-version bump. diff --git a/specs/032-ruleset-branch-protection/data-model.md b/specs/032-ruleset-branch-protection/data-model.md new file mode 100644 index 00000000..8a1b3133 --- /dev/null +++ b/specs/032-ruleset-branch-protection/data-model.md @@ -0,0 +1,167 @@ +# Phase 1 Data Model: Ruleset-aware branch-protection verdict + +## Purpose + +Enumerate every new type, its fields, its constraints, and its lifecycle. The vocabulary here is what the plan phase locks in for the reader contract, the tasks decomposition, and future reconciliation-style diffs. + +## New types + +### `ProtectionRequirement` (str Enum in `branch_protection.py`) + +The specific protection a control is testing for. Set via TOML `requirement = "..."` on a `handler = "github_branch_protection"` pass. + +| Member | TOML value | Classic surface satisfying signal | Ruleset rule type satisfying | +|--------|-----------|-----------------------------------|------------------------------| +| `REQUIRE_PULL_REQUEST` | `"require_pull_request"` | `required_pull_request_reviews` present | rule with `type == "pull_request"` | +| `PREVENT_DELETION` | `"prevent_deletion"` | `allow_deletions.enabled == false` | rule with `type == "deletion"` | +| `REQUIRE_STATUS_CHECKS` | `"require_status_checks"` | `required_status_checks` present | rule with `type == "required_status_checks"` | +| `REQUIRE_APPROVALS` | `"require_approvals"` | `required_pull_request_reviews.required_approving_review_count >= required_approvals_minimum` | rule with `type == "pull_request"` AND `parameters.required_approving_review_count >= required_approvals_minimum` | + +Members are stable identifiers; adding a new one is a non-breaking additive change. Renaming or removing a member is a breaking change to the four affected TOML controls' pass definitions and MUST be paired with a same-PR TOML update. + +### `VerdictSource` (str Enum in `branch_protection.py`) + +The enumerated evidence-source value written into the handler's evidence record. Locked by spec FR-016. + +| Member | TOML/evidence value | When emitted | +|--------|--------------------|--------------| +| `CLASSIC` | `"classic"` | Classic protection endpoint returned 200 AND carried the required signal. Rulesets were not consulted. | +| `RULESET` | `"ruleset"` | Rulesets were consulted (classic did not carry the signal) AND at least one active ruleset targeting the branch satisfies the requirement. Evidence includes the matched ruleset's `id` and `name`. | +| `NEITHER_SURFACE_PROVIDED_PROTECTION` | `"neither-surface-provided-protection"` | Both surfaces responded successfully; neither carried the required signal. Verdict is FAIL. | +| `INSUFFICIENT_ACCESS` | `"insufficient-access"` | Either surface returned 401 or 403. Verdict is WARN. | +| `PARTIAL_FETCH` | `"partial-fetch"` | Rulesets list succeeded but a subsequent detail call (or a subsequent list page) failed for any reason. Verdict is WARN. | + +### `HandlerConfig` (TOML surface consumed by the handler) + +Shape of the dict handed to the handler by the sieve orchestrator. All fields except `requirement` have defaults. + +| Field | Type | Default | Constraint | +|-------|------|---------|------------| +| `handler` | `str` | required, always `"github_branch_protection"` | Set by the TOML `handler = "..."` key. | +| `owner` | `str` | `"$OWNER"` | Substituted via the sieve's variable-substitution pass before the handler runs. | +| `repo` | `str` | `"$REPO"` | Same. | +| `branch` | `str` | `"$BRANCH"` | Same. Defaults to the repository's default branch when the audit does not specify. | +| `requirement` | `str` | required (no default) | Must be one of the `ProtectionRequirement` TOML values. | +| `required_approvals_minimum` | `int` | `1` | Only meaningful when `requirement == "require_approvals"`. Ignored otherwise. Range `1..10`. | +| `timeout` | `int` | `30` | Total time-budget in seconds across both surfaces (including rulesets pagination). Individual `gh api` invocations inherit this budget; the handler slices no lower. | + +### Handler evidence record shape + +The dict that ends up in `HandlerResult.evidence` after the handler runs. Consumed by the Markdown formatter, JSON output, and SARIF exporter (all of which treat unknown keys as opaque). + +```python +{ + "source": "classic" | "ruleset" | "neither-surface-provided-protection" | "insufficient-access" | "partial-fetch", + "requirement": "require_pull_request" | "prevent_deletion" | "require_status_checks" | "require_approvals", + "classic_status": 200 | 404 | 401 | 403 | 429 | 500 | 0, # 0 == unparseable status + "rulesets_status": 200 | 401 | 403 | 429 | 500 | 0, # 0 when call was skipped OR unparseable + "matched_ruleset": {"id": int, "name": str} | None, # populated only when source == "ruleset" + "considered_rulesets": [ # populated only when source == "neither-surface-provided-protection" + {"id": int, "name": str, "reason": str}, # reason describes why this ruleset did not satisfy + ... + ], + "considered_rulesets_truncated": int, # count of entries elided when the list exceeded 20 (research R-007) +} +``` + +Field-by-field ownership: + +- `source`, `requirement`, `classic_status` are always populated. +- `rulesets_status` is populated when the handler consulted the rulesets endpoint (source `RULESET`, `NEITHER_SURFACE_PROVIDED_PROTECTION`, `INSUFFICIENT_ACCESS` where the classic surface succeeded, or `PARTIAL_FETCH`). Otherwise omitted (or `0`). +- `matched_ruleset` is populated iff `source == "ruleset"`. +- `considered_rulesets` and `considered_rulesets_truncated` are populated iff `source == "neither-surface-provided-protection"` AND at least one active ruleset targeted the branch (rulesets that did not target the branch are NOT enumerated here; that would bloat the evidence with irrelevant entries). + +### `RulesetSummary` (runtime TypedDict, private to `branch_protection.py`) + +Minimal type for the ruleset-list response items: + +```python +class RulesetSummary(TypedDict, total=False): + id: int + name: str + target: Literal["branch", "tag"] + enforcement: Literal["active", "evaluate", "disabled"] + source_type: Literal["Repository", "Organization"] +``` + +`total=False` because we treat unknown fields as opaque; GitHub may add fields without our knowing. + +### `RulesetDetail` (runtime TypedDict, private to `branch_protection.py`) + +Detail response fields the handler consumes: + +```python +class RefNameConditions(TypedDict, total=False): + include: list[str] + exclude: list[str] + +class RulesetConditions(TypedDict, total=False): + ref_name: RefNameConditions + +class RulesetRule(TypedDict, total=False): + type: str # "pull_request", "deletion", "required_status_checks", etc. + parameters: dict[str, Any] + +class RulesetDetail(TypedDict, total=False): + id: int + name: str + enforcement: Literal["active", "evaluate", "disabled"] + conditions: RulesetConditions + rules: list[RulesetRule] +``` + +## Existing types touched + +### `darnit.core.utils.gh_api_with_status` (new) + +New module-level function. Signature and contract: + +```python +def gh_api_with_status( + endpoint: str, *, paginate: bool = False +) -> tuple[dict | list | None, int, str]: + """Execute a GitHub API call via `gh api` and return (body, status, error). + + On 2xx: returns (parsed_json, status_code, ""). + On non-2xx with parseable `HTTP :` prefix in stderr: returns (None, status_code, stderr_message). + On any other failure (subprocess not found, network failure, invalid JSON on 2xx): + returns (None, 0, error_message). + + When `paginate=True`, invokes `gh api --paginate ...` and concatenates all pages into + a single top-level list (matches `gh`'s pagination-flattening behavior). + """ +``` + +Existing `gh_api()` and `gh_api_safe()` become thin wrappers: + +```python +def gh_api(endpoint: str) -> dict[str, Any]: + body, status, error = gh_api_with_status(endpoint) + if status == 200 and isinstance(body, dict): + return body + raise RuntimeError(f"gh api failed: {error or 'status ' + str(status)}") +``` + +The new helper is what the branch-protection handler calls directly. Every existing `gh_api`/`gh_api_safe` caller continues to work unchanged. + +### Default-branch resolution (no extra API call) + +The handler consumes the repository's default branch from `context.default_branch`, which the audit driver already populates at `packages/darnit/src/darnit/tools/audit.py:428`. This feature does NOT introduce a `GET /repos/{owner}/{repo}` call to resolve the default branch -- doing so would violate SC-004's API-call budget without adding value. If `context.default_branch is None` on a partial context, `_ref_name_matches` conservatively treats `~DEFAULT_BRANCH` include entries as non-matching (Constitution II), rather than paying an API call to guess. + +### `docs/architecture/framework-design.md` (small edit) + +Add `github_branch_protection` to the handler-name registry table so `scripts/validate_sync.py`'s "Handler names in sync" check finds it. One-line addition; no behavior change. + +## Constants introduced + +- `MAX_CONSIDERED_RULESETS = 20` in `branch_protection.py` — the cap from research R-007. +- `DEFAULT_TIMEOUT_SECONDS = 30` in `branch_protection.py` — the total per-invocation time budget default. +- `SUPPORTED_REF_INCLUDE_LITERALS = frozenset({"~DEFAULT_BRANCH", "~ALL"})` in `branch_protection.py` — the two pseudo-refs the ref-matching helper recognises beyond exact-name and `refs/heads/`. + +## State transitions + +The handler is stateless: no persistent lifecycle, no session cache, no cross-invocation state. Every audit-run's invocation of the handler for a given control makes fresh API calls. This matches the constitution's Sieve Pipeline Integrity principle (handlers are pure functions of their inputs plus their side-channel API responses). + +## Non-model concerns + +Everything else about this feature reuses machinery that already exists in the sieve: CEL binding is not used (the handler decides PASS/FAIL/INCONCLUSIVE itself), the `when = { platform = "github" }` guard is preserved on all four controls, and evidence attaches via the standard `HandlerResult.evidence` dict. No new pydantic models, no schema migrations, no persistent state changes. diff --git a/specs/032-ruleset-branch-protection/plan.md b/specs/032-ruleset-branch-protection/plan.md new file mode 100644 index 00000000..7ef263cf --- /dev/null +++ b/specs/032-ruleset-branch-protection/plan.md @@ -0,0 +1,194 @@ +# Implementation Plan: Ruleset-aware branch-protection verdict + +**Branch**: `032-ruleset-branch-protection` | **Date**: 2026-08-22 | **Spec**: [spec.md](./spec.md) + +**Input**: Feature specification from `specs/032-ruleset-branch-protection/spec.md` (with 3 clarifications recorded 2026-08-22: consult-rulesets whenever classic surface did not itself provide the required signal; distinguish HTTP status classes via shared helper enhancement; use `gh api --paginate` and WARN on any page-fetch failure). + +## Summary + +Extend the four OSPS branch-protection controls (`OSPS-AC-03.01`, `OSPS-AC-03.02`, `OSPS-QA-03.01`, `OSPS-QA-07.01`) to consult the GitHub Repository Rulesets API surface in addition to the classic `/branches/{branch}/protection` endpoint. A repo whose default branch is protected via a ruleset (rather than classic branch protection) currently produces false FAILs from all four controls; that regression, introduced when feature 019 tightened the 404-means-not-protected verdict, is corrected here without weakening the FAIL semantics for genuinely non-compliant repos or the WARN semantics for ambiguous responses. + +Implementation lives entirely inside `packages/darnit-baseline/`. A new sieve handler `github_branch_protection` (registered by the baseline plugin) encapsulates the two-surface check. The four TOML controls' first pass switches from `exec` on `gh api /branches/{branch}/protection` to `handler = "github_branch_protection"` with a `requirement` parameter naming which protection is being tested for. The trailing manual pass on each control is unchanged. A small shared helper enhancement in `packages/darnit/src/darnit/core/utils.py` (a status-code-aware sibling of `gh_api`) is added so both the classic and the rulesets calls can distinguish 200 / 404 / other-non-200 rather than collapsing everything into `RuntimeError`. The helper enhancement is scoped and reusable by any future control that needs the same WARN/FAIL boundary. + +Zero new runtime dependencies. Zero controls outside the named four change behavior. + +## Technical Context + +**Language/Version**: Python 3.11/3.12 (workspace targets - unchanged). + +**Primary Dependencies**: `gh` CLI (already required for the classic-protection pass and for other baseline controls); Pydantic 2.x (already used for framework schema); no new pip dependencies. + +**Storage**: Filesystem only. No new persistent state; the two GitHub API responses are consumed per-invocation and their salient fields recorded in the control's evidence dict for the audit report. + +**Testing**: pytest under `tests/darnit_baseline/`. The handler tests mock the shared `gh_api` helper directly (function-level substitution) rather than mocking `subprocess.run`, so the tests remain robust across `gh` CLI version changes. Live-integration tests against a real repo are out of scope for CI (they require network + a specifically-configured fixture repo) but the quickstart documents how to run them manually. + +**Target Platform**: Same as darnit workspace - any platform Python 3.11+ runs on. `gh` CLI must be on PATH; already required by peer controls. + +**Project Type**: Compliance-implementation-package change. Scoped to `packages/darnit-baseline/` (new handler + TOML edits + tests) with one small enhancement to `packages/darnit/src/darnit/core/utils.py` (shared helper). + +**Performance Goals**: Not a hot path; API calls are network-bound by definition. Spec's SC-004 caps API calls per audit at 1 classic-endpoint call + `ceil(N/page_size)` rulesets-list calls + N detail calls per repository, where N is the ruleset count. The default-branch value is consumed from `context.default_branch` (populated by the audit driver at `packages/darnit/src/darnit/tools/audit.py:428`), NOT via an extra `GET /repos/{owner}/{repo}` call. In practice: for a typical repo with 0-2 rulesets, this feature adds at most 3 API calls per repo beyond the classic call feature 019 already made. + +**Constraints**: +- Zero new runtime dependencies (FR-013). +- No behavior change for non-GitHub audits (existing `when = { platform = "github" }` guards remain, FR-010). +- Evidence-record additions MUST be additive (existing fields preserved; new `source` field added, FR-016). +- Preserve WARN semantics on ambiguous surface responses (FR-006, User Story 3). +- Preserve FAIL semantics when both surfaces confirm no protection (FR-005, User Story 2). +- Only rulesets with `enforcement = "active"` count (FR-012). + +**Scale/Scope**: One new sieve handler in baseline (~200-300 lines including tests-side fixtures); one shared helper enhancement in core (~30-50 lines); four TOML controls updated in place (~40 lines of TOML edits net-zero); ~500-800 lines of new test code. Estimated diff: ~1000 lines total. + +## Constitution Check + +*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.* + +The darnit constitution (5 core principles) evaluated against this feature: + +| Principle | Applies | Assessment | +|-----------|---------|------------| +| I. Plugin Separation | Yes | PASS. The `github_branch_protection` handler lives in `darnit-baseline` (implementation), NOT in `darnit` core. The shared helper enhancement in `darnit.core.utils` is a framework-level utility that does not import implementation packages. Core -> implementation direction remains one-way. | +| II. Conservative-by-Default | Yes | PASS. This entire feature is a Conservative-by-Default correction: the current code produces a false FAIL for a compliant repo, which is the exact class of error the principle forbids. FR-005 (FAIL only when BOTH surfaces respond and NEITHER protects) and FR-006 (WARN on ANY ambiguity) tighten the WARN/FAIL boundary. | +| III. TOML-First Architecture | Yes | PASS. The four affected controls' passes remain fully declared in TOML. The new handler is invoked declaratively via `handler = "github_branch_protection"` and a `requirement = "..."` parameter, matching the same TOML-first pattern as `exec` and `api_call`. No Python-code path for control logic is introduced. | +| IV. Never Guess User Values | Yes | PASS. Branch protection status is an OBSERVATION (query external API for ground truth), not a user-judgment key. The handler produces `dispositive` results by default. No candidate/confirm mechanism is involved. | +| V. Sieve Pipeline Integrity | Yes | PASS. The handler returns a single `HandlerResult`; orchestrator's disposition logic is unchanged. An INCONCLUSIVE result (WARN cause) correctly falls through to the trailing manual pass, matching feature 019's already-shipped semantic. CEL post-step is not used by this handler because the handler already produces its own PASS/FAIL/INCONCLUSIVE verdict; the classic-only exec pass being replaced was where the CEL post-step lived (a CEL over the JSON response). | + +Architecture constraints (three-layer architecture, package structure): PASS. Layer 1 (Checking) gains a new baseline-registered handler; Layer 2 (Remediation) is unchanged; Layer 3 (MCP Tools) is unchanged. + +Development workflow (lint, tests, spec sync): PASS. Standard workflow. No new gates required. The spec-sync check (`validate_sync.py`) validates handler names in code against `docs/architecture/framework-design.md`; the new handler must be added to that document (T050 in the tasks plan). + +**Gate result: PASS. Proceed to Phase 0.** + +## Project Structure + +### Documentation (this feature) + +```text +specs/032-ruleset-branch-protection/ +├── plan.md # This file +├── research.md # Phase 0 output - gh stderr format, rulesets JSON shape, testing approach +├── data-model.md # Phase 1 output - ProtectionRequirement enum, evidence record, helper return type +├── quickstart.md # Phase 1 output - control-author + operator debugging examples +├── contracts/ +│ └── github-branch-protection-handler.md # Phase 1 output - TOML surface + evidence shape +├── checklists/ +│ └── requirements.md # From /speckit-specify (all 16 items pass) +└── tasks.md # /speckit-tasks output (not created here) +``` + +### Source Code (repository root) + +```text +packages/darnit/src/darnit/core/ +└── utils.py # Extend with `gh_api_with_status()` returning (body, status_code, error_msg) tuple. + # Existing `gh_api()` and `gh_api_safe()` remain unchanged callers. + +packages/darnit-baseline/src/darnit_baseline/ +├── branch_protection.py # NEW. `github_branch_protection` sieve handler + ProtectionRequirement enum +│ # + ruleset-matching helpers. All feature-specific logic lives here. +├── implementation.py # Register the new sieve handler in `register_handlers()` (small addition). +└── openssf-baseline.toml # UPDATE 4 controls' first pass from exec-to-handler; unchanged manual passes. + +tests/darnit_baseline/ +├── test_branch_protection_handler.py # NEW. Unit tests: each requirement type against +│ # classic-only, ruleset-only, both-surfaces, both-ambiguous, +│ # exclude-conditions, evaluate-mode ruleset, empty rules array, +│ # partial-fetch failure, pagination. +└── test_branch_protection_integration.py # NEW. Integration tests through the sieve orchestrator + # for the four TOML controls (one test per control). + +tests/darnit/ +└── core/test_gh_api_status.py # NEW. Unit tests for the new `gh_api_with_status()` helper: + # 200/404/403/5xx status extraction from gh stderr. + +docs/architecture/ +└── framework-design.md # Small edit adding the new handler name to the registry table + # so `validate_sync.py` passes. +``` + +**Structure Decision**: Keep the whole feature footprint inside `packages/darnit-baseline/` except for the tiny shared-helper enhancement, which properly belongs in core because it's transport infrastructure any future implementation can reuse. Splitting the new logic into its own `branch_protection.py` module inside baseline (rather than piling into `tools.py`) improves reviewability and creates a natural extension point when we generalize beyond these four controls or add organization-level ruleset support (v0.1 follow-up per the spec). + +## Complexity Tracking + +No constitution violations to justify. The feature is a scoped handler addition with zero new architectural surface. + +## Phase 0: Research + +Research questions surfaced by Technical Context and the spec's Assumptions/Edge Cases: + +1. **What is `gh`'s stderr format on HTTP-error non-zero exits?** — The stderr output on a non-2xx response follows the pattern `HTTP : ` (e.g., `HTTP 404: Not Found (https://api.github.com/...)`). Verifiable by manually running `gh api /repos/nonexistent/nonexistent 2>&1`. Research decision: parse the first-line prefix with a compiled regex `^HTTP (\d{3}):`. Fallback: absent the pattern (e.g., network error before the request completed), treat as ambiguous status and resolve WARN per FR-006. This matches the shared helper's contract from FR-017. + +2. **What is the JSON shape of a ruleset list vs a ruleset detail?** — `GET /repos/{owner}/{repo}/rulesets` returns an array of ruleset summaries: `[{"id": N, "name": "...", "target": "branch", "enforcement": "active|evaluate|disabled", "source_type": "Repository", ...}, ...]`. The summary does NOT include `rules` or `conditions`; those require `GET /repos/{owner}/{repo}/rulesets/{id}` (detail fetch). Research decision: v0 fetches every ruleset's detail (bounded by SC-004's `N` where N is the ruleset count); we do NOT prematurely filter by summary-level `enforcement` because a maintainer may re-enable a ruleset between the list and detail calls and we care about the state at detail-fetch time. Detail response carries `conditions.ref_name.include: [...]`, `conditions.ref_name.exclude: [...]`, and `rules: [{"type": "pull_request", "parameters": {"required_approving_review_count": 1, ...}}, ...]`. Ruleset rule types relevant to this feature: `pull_request`, `deletion`, `required_status_checks`, `non_fast_forward`. Full type list documented at [GitHub's ruleset schema docs](https://docs.github.com/en/rest/repos/rules). + +3. **How is the `ref_name` targeting field structured, and what forms does `include` take?** — Values in the `include` list are one of: `~DEFAULT_BRANCH` (the pseudo-ref that resolves to the repository's default branch at evaluation time), `~ALL` (all refs), an exact `refs/heads/` git-ref, or a bare branch name like `main`. Glob patterns (e.g., `refs/heads/release/*`) are allowed but not evaluated in v0 (treated as "does not match" per the spec's Edge Cases). Research decision: matching function accepts `(branch: str, default_branch: str)` and returns True for `~DEFAULT_BRANCH` iff `branch == default_branch`, True for `~ALL`, True for exact branch name matches, and False otherwise (including all patterns). Absence of the audited branch in `exclude` is a precondition for a match. + +4. **What is the naming convention for baseline sieve handlers?** — Existing precedent: `generate_threat_model_handler` (registered under the short name via the sieve registry). Research decision: register the new handler under the short name `github_branch_protection`. Rationale: matches the domain (GitHub branch-protection surfaces), leaves room for a future `gitlab_branch_protection` if we ever add that platform, and follows the same underscore-separated verb-noun pattern as `file_exists` and `api_call`. The handler function itself is named `github_branch_protection_handler` in `branch_protection.py`. + +5. **What is the shape of the shared helper enhancement?** — Two options considered: (a) new function `gh_api_with_status(endpoint) -> tuple[dict | None, int, str]`, (b) enhance the existing `gh_api` to raise a typed exception carrying the status code. Option (a) wins because (a) it does not change any of the ~30 existing `gh_api` / `gh_api_safe` callers, (b) it makes the "I care about status codes" intent explicit at call sites, and (c) it composes cleanly: `gh_api` and `gh_api_safe` remain thin wrappers over `gh_api_with_status`. Return contract: on 2xx, `(body_dict, status, "")`; on non-2xx with parseable status, `(None, status, stderr_message)`; on `FileNotFoundError` (gh CLI missing) or other exception before/without a parseable status, `(None, 0, error_message)`. + +6. **How do existing baseline handler tests mock the GitHub API?** — Reviewing `tests/darnit_baseline/` shows the convention: tests mock at the module-level function (`monkeypatch.setattr(module, "gh_api_safe", fake_fn)`). Research decision: the handler tests substitute `darnit_baseline.branch_protection.gh_api_with_status` with a mock that returns pre-canned `(body, status, message)` tuples for each `(endpoint_pattern, expected_call_index)` pair. This lets tests exercise the exact API-call-order the handler emits without mocking `subprocess.run`, avoiding `gh`-version fragility. A small `_GhResponseSequencer` fixture in the test module encapsulates the pattern. + +**Output**: `research.md` documenting each decision with rationale and rejected alternatives. + +## Phase 1: Design & Contracts + +**Prerequisites**: `research.md` complete. + +### Data Model (`data-model.md`) + +New schema types and their relationships: + +- **`ProtectionRequirement`** (str Enum in `branch_protection.py`): the requirement a control tests for. Members: `REQUIRE_PULL_REQUEST`, `PREVENT_DELETION`, `REQUIRE_STATUS_CHECKS`, `REQUIRE_APPROVALS`. Set via TOML `requirement = "..."` on the handler pass. Extension point for future requirements without changing the handler dispatch. + +- **`RequiredApprovalsMinimum`** (int on the handler config, default 1): For `REQUIRE_APPROVALS` requirement, the minimum `required_approving_review_count` that satisfies. Defaults to 1 to match `OSPS-QA-07.01`'s existing semantic. + +- **`RulesetSummary`** (TypedDict, runtime-only in `branch_protection.py`): The shape of the list-response items we care about (`id`, `name`, `target`, `enforcement`). Total-only; we don't type the full ruleset schema because we treat unknown fields as opaque. + +- **`RulesetDetail`** (TypedDict, runtime-only): The shape of the detail-response fields we care about (`id`, `name`, `enforcement`, `conditions.ref_name.{include,exclude}`, `rules: list[{type, parameters}]`). + +- **`VerdictSource`** (str Enum, in `branch_protection.py`): The enumerated evidence-source values from spec FR-016. Members: `CLASSIC` (verdict from classic surface alone), `RULESET` (verdict from a specific ruleset), `NEITHER_SURFACE_PROVIDED_PROTECTION` (both surfaces answered, neither protects), `INSUFFICIENT_ACCESS` (either surface returned 401/403), `PARTIAL_FETCH` (list succeeded but a detail or a subsequent page failed). + +- **Handler evidence record** (dict shape): the evidence dict written into `HandlerResult.evidence` by the handler. Keys: `source: str` (a `VerdictSource` value), `classic_status: int` (status code from the classic-endpoint call, `0` if the call was skipped), `rulesets_status: int` (status from the list call, `0` if skipped), `matched_ruleset: {"id": int, "name": str} | None` (populated when `source == RULESET`), `considered_rulesets: list[{"id": int, "name": str}]` (populated on FAIL; enumerates every active ruleset that targets the branch but did not satisfy the requirement; capped at 20 entries with a `truncated: N` suffix), `requirement: str` (the ProtectionRequirement that was tested for). + + Capping the `considered_rulesets` list at 20 addresses the plan-time deferral from the clarification session: it prevents a rare high-ruleset repo from bloating the evidence record while preserving the information needed to explain the FAIL to a human. A repo with more than 20 non-satisfying active rulesets is pathological. + +- **`gh_api_with_status`** (new function in `darnit.core.utils`): return type `tuple[dict | list | None, int, str]`. First element is the parsed JSON body (dict OR list; rulesets endpoint returns a list at the top level), second is HTTP status (0 if unparseable), third is the stderr text on error. Existing `gh_api()` becomes `body, status, msg = gh_api_with_status(endpoint); if status != 200: raise RuntimeError(msg)` (thin wrapper). Existing `gh_api_safe()` similarly. + +### Contracts (`contracts/github-branch-protection-handler.md`) + +The public control-author API. Contents: + +- **TOML pass surface**: exact field list for `handler = "github_branch_protection"`. Fields: `owner` (default `$OWNER`), `repo` (default `$REPO`), `branch` (default `$BRANCH`), `requirement` (required; one of the four enum values), `required_approvals_minimum` (optional, default 1, only meaningful when requirement is `REQUIRE_APPROVALS`), `timeout` (optional, default 30 seconds; total budget across both surfaces including pagination). + +- **Evidence shape**: the fields the handler writes into `HandlerResult.evidence` per the data model above. Downstream evidence-readers (Markdown formatter, JSON output, SARIF) treat unknown keys as opaque, matching feature 019's evidence-additive posture. + +- **Progress-log shape**: none. The handler runs in the same dispatch step as other sieve handlers; no per-surface INFO line is emitted (matches `file_exists`, `exec`, `api_call`). Feature 031's `dispatching_mcp` is a separate concept because that pool is orchestrator-owned; here the handler runs synchronously within the sieve step. + +- **Failure modes**: exhaustive table mapping every distinguishable scenario to (control status, source enum value, message). + +- **Non-goals for v0**: (a) organization-level rulesets (v0.1 follow-up); (b) generalizing beyond the four named controls; (c) evaluate-mode ruleset counting; (d) glob-pattern ref_name matching. + +### Quickstart (`quickstart.md`) + +Two worked examples: + +1. **Control author perspective**: rewriting `OSPS-AC-03.01`'s first pass from the old exec form to the new handler form. Includes the exact TOML diff, the expected evidence-record output for a repo protected via a ruleset, and the expected evidence output for a repo with no protection at either surface. + +2. **Operator debugging perspective**: an audit produced WARN on `OSPS-QA-07.01`. Read the evidence record, see `source: "insufficient-access"` and `rulesets_status: 403`. Fix: reauthenticate `gh` with `admin:read` scope. Alternate scenario: `source: "partial-fetch"` and `rulesets_status: 429` — fix: rerun after rate-limit window resets. + +Also includes the "failure-mode diagnostics" section: how to interpret each failure-status message from the contract. + +### Agent Context Update + +Update the reference between `` and `` markers in `CLAUDE.md` to point at `specs/032-ruleset-branch-protection/plan.md`. + +## Post-Design Constitution Recheck + +The design phase artifacts do not introduce any new principle-touching decisions: + +- **I. Plugin Separation**: reinforced by the module layout — the handler is under `darnit-baseline/`, not `darnit/`. The `gh_api_with_status` addition is a pure transport primitive in `darnit.core.utils` with no implementation-package imports. +- **II. Conservative-by-Default**: reinforced by the `VerdictSource` enum's inclusion of `INSUFFICIENT_ACCESS` and `PARTIAL_FETCH` as first-class WARN causes — the framework will never conflate "we could not tell" with "we know it fails." +- **III. TOML-First**: reinforced by the exact TOML surface documented in the reader contract; the handler adds no new Python-code escape hatch. +- **IV. Never Guess User Values**: reinforced by the fact that branch protection is an observation, not a candidate; no `auto_detect` / `allow_sieve_hints` machinery is touched. +- **V. Sieve Pipeline Integrity**: reinforced by the handler returning a single `HandlerResult` and using the trailing manual pass as the natural INCONCLUSIVE fallback. + +**Post-design gate: PASS.** diff --git a/specs/032-ruleset-branch-protection/quickstart.md b/specs/032-ruleset-branch-protection/quickstart.md new file mode 100644 index 00000000..7f0b2193 --- /dev/null +++ b/specs/032-ruleset-branch-protection/quickstart.md @@ -0,0 +1,150 @@ +# Quickstart: `github_branch_protection` sieve handler + +Two worked examples. First is the control-author perspective (updating the four TOML controls to use the new handler). Second is the operator debugging perspective (interpreting a WARN in the audit output). + +## Example 1: Control author updates a branch-protection control + +`OSPS-AC-03.01` today has an `exec` pass that shells `gh api /repos/$OWNER/$REPO/branches/$BRANCH/protection` and a CEL post-step that checks `has(output.json.required_pull_request_reviews)`. That combination produces a false FAIL for a repo protected via a ruleset. The new pass replaces it with the handler. + +### Before + +```toml +[[controls."OSPS-AC-03.01".passes]] +handler = "exec" +command = ["gh", "api", "/repos/$OWNER/$REPO/branches/$BRANCH/protection"] +pass_exit_codes = [0] +fail_exit_codes = [1] +output_format = "json" +expr = 'has(output.json.required_pull_request_reviews)' +timeout = 30 +``` + +### After + +```toml +[[controls."OSPS-AC-03.01".passes]] +handler = "github_branch_protection" +requirement = "require_pull_request" +timeout = 30 +``` + +Six lines replaced with three. The `manual` trailing pass in the control is unchanged. + +Analogously for the other three controls: + +- `OSPS-AC-03.02` -> `requirement = "prevent_deletion"` +- `OSPS-QA-03.01` -> `requirement = "require_status_checks"` +- `OSPS-QA-07.01` -> `requirement = "require_approvals"` (defaults to `required_approvals_minimum = 1`, which matches the existing behavior) + +### What happens at audit time + +1. The sieve orchestrator dispatches the handler for `OSPS-AC-03.01` on a repository. +2. Handler queries `gh api /repos/{owner}/{repo}/branches/{branch}/protection`. +3. If the response is 200 AND carries `required_pull_request_reviews`: PASS from classic. Evidence: `source = "classic"`, `classic_status = 200`. +4. Otherwise (any 404, or 200 without `required_pull_request_reviews`): handler queries `gh api --paginate /repos/{owner}/{repo}/rulesets`. +5. For each active ruleset in the list, handler fetches `/repos/{owner}/{repo}/rulesets/{id}` and checks whether the ruleset targets the audited branch (via `conditions.ref_name`) AND carries a `pull_request` rule. +6. First matching ruleset: PASS from ruleset. Evidence: `source = "ruleset"`, `matched_ruleset = {id, name}`. +7. All rulesets exhausted without a match: FAIL. Evidence: `source = "neither-surface-provided-protection"` with the list of considered rulesets and why each did not match. +8. Any 401/403/5xx/429 from either surface, or a mid-pagination failure: INCONCLUSIVE -> falls through to the manual pass -> WARN. Evidence: `source = "insufficient-access"` or `"partial-fetch"`. + +## Example 2: Operator debugging a WARN in the audit report + +Running `darnit audit` against a fleet, `OSPS-QA-07.01` came back WARN for one repo. The audit report includes: + +```json +{ + "id": "OSPS-QA-07.01", + "status": "WARN", + "evidence": { + "source": "insufficient-access", + "requirement": "require_approvals", + "classic_status": 403, + "rulesets_status": 0 + } +} +``` + +Read the evidence: + +- `classic_status: 403` — the classic branch-protection endpoint refused to answer. +- `rulesets_status: 0` — the handler never got to the rulesets endpoint because the classic call failed first (and a 403 from classic short-circuits to INCONCLUSIVE without consulting rulesets, since we cannot distinguish "no protection classic-side" from "we cannot read protection classic-side"). + +Fix: reauthenticate the `gh` CLI with `admin:read` scope (or the fine-grained equivalent "Read access to repository administration"). Rerun the audit. + +### Alternate scenario: rate limit hit mid-pagination + +```json +{ + "id": "OSPS-AC-03.01", + "status": "WARN", + "evidence": { + "source": "partial-fetch", + "requirement": "require_pull_request", + "classic_status": 404, + "rulesets_status": 429 + } +} +``` + +Read: + +- `classic_status: 404` — no classic protection, so the handler needed to consult rulesets. +- `rulesets_status: 429` — GitHub rate-limited the request mid-list. +- `source: "partial-fetch"` — we did not fully enumerate the rulesets surface, so we cannot confidently FAIL. + +Fix: wait out the rate-limit window (`gh api rate_limit` shows the reset time) or reduce audit parallelism, then rerun. + +### Alternate scenario: FAIL with rulesets considered + +```json +{ + "id": "OSPS-AC-03.01", + "status": "FAIL", + "evidence": { + "source": "neither-surface-provided-protection", + "requirement": "require_pull_request", + "classic_status": 404, + "rulesets_status": 200, + "considered_rulesets": [ + {"id": 1234, "name": "Signed commits", "reason": "no matching rule type"}, + {"id": 5678, "name": "Tag protection", "reason": "targets tags not branches"} + ], + "considered_rulesets_truncated": 0 + } +} +``` + +Read: + +- Classic returned 404 and rulesets exist, but neither `Signed commits` nor `Tag protection` requires pull requests on the audited branch. +- Fix: create or extend a repository ruleset on the default branch with a `pull_request` rule, or enable classic branch protection with `required_pull_request_reviews`. The control's `help_md` (unchanged from today) links to the GitHub docs. + +## Manual smoke-test against a live GitHub repo + +The CI test suite uses in-process fixtures (see `tests/darnit_baseline/test_branch_protection_handler.py`) and does not hit live GitHub. For a manual smoke-test: + +1. Point at a repo you know is protected via classic branch protection: + `uv run darnit audit --local-path --controls OSPS-AC-03.01 OSPS-AC-03.02 OSPS-QA-03.01 OSPS-QA-07.01` + Expect: all four PASS with `source: "classic"`. + +2. Point at a repo protected via ruleset only (or use a scratch repo where you delete classic protection but add a ruleset via `gh api` PUT). Same command. Expect: all four PASS with `source: "ruleset"` and populated `matched_ruleset`. + +3. Point at a repo with no protection (a fresh test repo). Expect: all four FAIL with `source: "neither-surface-provided-protection"`. + +4. Deauthenticate `gh` (`gh auth logout`). Rerun. Expect: WARN across the four with `source: "insufficient-access"`. + +## Non-goals reminder + +This feature does NOT: + +- Consult organization-level rulesets (org-level rulesets are surfaced via a different endpoint; a v0.1 follow-up). +- Treat evaluate-mode rulesets as satisfying protection (only `enforcement = "active"` counts). +- Match glob-pattern ref-name conditions (they appear in `considered_rulesets` as "unmatched glob pattern" but do not satisfy). +- Extend to non-GitHub platforms (the `when = { platform = "github" }` guard on all four controls keeps this feature invisible to non-GitHub audits). + +## Where to look next + +- Contract: `contracts/github-branch-protection-handler.md` -- exhaustive field, evidence, and failure-mode table. +- Data model: `data-model.md` -- the schema types this feature adds. +- Research decisions: `research.md` -- why the handler lives in baseline, why `--paginate`, why module-level test substitution. +- Follow-up for organization-level rulesets: to be filed after v0 lands. diff --git a/specs/032-ruleset-branch-protection/research.md b/specs/032-ruleset-branch-protection/research.md new file mode 100644 index 00000000..3aa57116 --- /dev/null +++ b/specs/032-ruleset-branch-protection/research.md @@ -0,0 +1,114 @@ +# Phase 0 Research: Ruleset-aware branch-protection verdict + +## Purpose + +Resolve every unknown surfaced by the plan's Technical Context section and the spec's Assumptions/Edge Cases. Each entry records the decision, rationale, and rejected alternatives so future maintainers can pick up the thread. + +## Decisions + +### R-001: Parse HTTP status from `gh`'s stderr on non-zero exit + +**Decision**: Introduce `gh_api_with_status(endpoint: str) -> tuple[body | None, status: int, error: str]` in `darnit.core.utils` alongside the existing `gh_api` and `gh_api_safe`. Parse `gh`'s stderr on non-zero exit with the regex `^HTTP (\d{3}):` to extract the HTTP status code. Absent the pattern, return `status=0` and surface the raw stderr as the error message. + +**Rationale**: The `gh` CLI's stderr format on HTTP-error non-zero exits follows the stable pattern `HTTP : ()` — for example `HTTP 404: Not Found (https://api.github.com/repos/octocat/hello-world/branches/nonexistent/protection)`. The prefix is emitted by `gh`'s core error handler and predates the CLI 2.x release; the format is stable enough to key on. Non-HTTP failures (network before the request completed, `FileNotFoundError` for the `gh` binary itself) do not carry the prefix and fall through to `status=0` — the caller MUST treat this as ambiguous per FR-006 (WARN, not FAIL). + +**Alternatives considered**: + +- **Raise a typed exception carrying the status code.** Rejected because it would require touching every existing `gh_api` caller to catch the new exception class or add `contextlib.suppress` guards, expanding the change surface unnecessarily. +- **Enhance `gh_api` in place to return a tuple.** Rejected because ~30 existing callers rely on the current dict-or-raise contract; changing it would either break all of them or force adapter shims. +- **Use `gh api --include` to surface HTTP headers.** Considered attractive because it produces machine-parseable status information without stderr scraping. Rejected for v0 because the flag changes the response body format (prepends the response headers as text before the JSON), forcing extra parsing on the success path too. Worth revisiting if the stderr-prefix pattern ever changes. +- **Bypass `gh` and use `urllib.request` directly.** Rejected: reintroduces the auth-token-resolution problem the existing helper already solves, and fragments the darnit transport surface across two GitHub clients. + +**References**: Existing helper at `packages/darnit/src/darnit/core/utils.py:20-53`. The `HTTP :` stderr prefix is documented at cli/cli issue #4200 (historical) and confirmed by hand-testing `gh api /repos/nonexistent/nonexistent 2>&1` locally. + +--- + +### R-002: Ruleset list is a summary; detail-fetch is required per ruleset + +**Decision**: The handler fetches the ruleset list via `gh api --paginate /repos/{owner}/{repo}/rulesets` (returns a JSON array top-level), then for each entry fetches the detail via `gh api /repos/{owner}/{repo}/rulesets/{id}` to obtain `conditions` and `rules`. We do not filter by summary-level `enforcement` before the detail fetch: a maintainer could re-enable a ruleset between the list and the detail call, and paying the extra detail call is cheaper than an incorrect FAIL. + +**Rationale**: GitHub's REST v3 rulesets list endpoint returns a summary schema that does NOT include the `rules` array or the `conditions` object; both are required to determine whether a ruleset satisfies a given protection requirement. Per-ruleset detail is therefore load-bearing. The rulesets endpoint respects `?per_page=` and `Link` header pagination the same way other list endpoints do; `gh api --paginate` handles this transparently. + +**Alternatives considered**: + +- **Use GitHub's GraphQL API to fetch rulesets + rules in a single call.** Rejected for v0 because darnit's existing GitHub transport is the `gh` CLI REST path; introducing a GraphQL query would fragment the transport surface. Worth revisiting if the per-ruleset detail cost becomes a real fleet-wide bottleneck. +- **Skip the detail call when the summary's `enforcement` is not `active`.** Rejected because the summary's `enforcement` field reflects state at list-fetch time; a TOCTOU-like race could produce inconsistent verdicts. The saved API call is not worth the flakiness. + +**References**: `GET /repos/{owner}/{repo}/rulesets` and `GET /repos/{owner}/{repo}/rulesets/{id}` at [docs.github.com/en/rest/repos/rules](https://docs.github.com/en/rest/repos/rules). + +--- + +### R-003: `conditions.ref_name.include` matching rules + +**Decision**: A ruleset is considered to cover the audited branch iff its `conditions.ref_name.include` contains at least one entry that matches AND its `conditions.ref_name.exclude` contains no entry that matches. Matching semantics: + +- `~DEFAULT_BRANCH` matches iff the audited branch equals the repository's default branch. +- `~ALL` matches every branch. +- Exact bare branch name (e.g., `main`) matches iff equal to the audited branch. +- `refs/heads/` matches iff `` equals the audited branch. +- Any pattern containing a glob metacharacter (`*`, `?`, `[`) is TREATED AS NOT MATCHING for v0 and reported in the evidence's `considered_rulesets` list. A future v0.1 can extend to glob matching using `fnmatch` semantics (git-ref globbing is more complex than POSIX fnmatch, so v0 conservatively excludes). + +**Rationale**: The four spec'd values cover every real-world configuration we have seen (default-branch pseudo-ref, exact-name, git-ref, all-refs). Glob patterns are legal on GitHub's side but rare in practice for branch-protection use cases (they're more common for tag rulesets). Excluding them in v0 is the conservative-by-default posture — if a ruleset uses a glob to cover the default branch, the framework will fall back to "considered but did not match" and the audit produces FAIL (assuming no other satisfying ruleset). That is the wrong direction versus the constitution's "false FAIL better than false PASS" rule but the RIGHT direction versus this feature's premise: we do not silently PASS on a glob we didn't evaluate. + +**Alternatives considered**: + +- **Implement glob matching in v0 using `fnmatch.fnmatchcase`.** Rejected because git ref-name pattern syntax has documented differences from `fnmatch` (multi-segment `**`, character-class semantics). Getting this right requires more thought than a v0 warrants; documented for v0.1. +- **Warn (WARN) instead of "not match" when the include list contains only glob patterns.** Considered but rejected: a ruleset whose sole `include` is a glob and the audited branch does not fall inside it should be a clear "not applicable to this branch," not an ambiguity. The WARN would misuse the semantic. The `considered_rulesets` evidence field surfaces the glob for the human, which is enough for a user to escalate. + +--- + +### R-004: Handler name registration + +**Decision**: Register the sieve handler under the short name `github_branch_protection` via `darnit-baseline`'s `register_handlers()`. Handler function name: `github_branch_protection_handler` (matches existing `generate_threat_model_handler` naming). + +**Rationale**: Matches darnit's existing handler-naming pattern (verb-noun-underscore, e.g., `file_exists`, `api_call`, `manual_steps`). The `github_` prefix leaves room for a future `gitlab_branch_protection` if we ever extend to GitLab, and the `_branch_protection` suffix is more specific than a generic `branch_check` would be. Baseline is the correct package for it: this handler is domain-specific to the OSPS Baseline's four branch-protection controls, not a general framework primitive. + +**Alternatives considered**: + +- **`branch_protection` (no `github_` prefix).** Rejected because it implies platform-neutrality that this handler does not provide. Better to keep the platform in the name. +- **`gh_branch_protection`.** Considered; rejected because the framework has other handlers that use spelled-out product names (`sigstore`, `github`) rather than CLI abbreviations, and consistency wins. +- **Register in `packages/darnit/` core instead of baseline.** Rejected: Layer 1 built-ins in core are the platform-neutral primitives (`file_exists`, `exec`, `pattern`, `manual`). Domain-specific handlers belong to their implementation package. + +--- + +### R-005: `gh_api_with_status` return type accommodates both dict and list bodies + +**Decision**: The helper's return type is `tuple[dict | list | None, int, str]`. The rulesets list endpoint returns a JSON array at the top level (unlike most other REST endpoints, which return a dict). The helper does not coerce; the caller inspects the type. The existing `gh_api()` wrapper narrows to `dict` when its typed contract requires it and raises when the response is a list (this narrowing is done in the tiny thin-wrapper implementation, preserving backward compatibility with today's callers). + +**Rationale**: The alternative — always coercing to dict via `{"data": [...]}` for list responses — would leak wrapper semantics into the caller and require every rulesets-endpoint consumer to unwrap. The union return type is honest. + +**Alternatives considered**: + +- **Two separate helpers, `gh_api_with_status_dict` and `gh_api_with_status_list`.** Rejected: two names for the same operation invites drift in error handling and pagination logic. One helper, one contract. + +--- + +### R-006: Test isolation via module-level function substitution + +**Decision**: The handler tests mock at `darnit_baseline.branch_protection.gh_api_with_status` (module-level substitution via `monkeypatch.setattr`). A small helper class `_GhResponseSequencer` in the test module encapsulates the "return this response for the Nth call matching this endpoint pattern" pattern, so each test case reads as a small script of expected exchanges. + +**Rationale**: Mocking at the helper level, not at `subprocess.run`, keeps the tests independent of the `gh` CLI's installed version, its stderr format changes, and its argv layout. It also lets us assert on the exact endpoints and order of API calls, which is load-bearing for spec SC-004 (API call budget) and SC-005 (zero cost when the four controls are excluded). + +**Alternatives considered**: + +- **Mock `subprocess.run` directly.** Rejected: forces every test to construct `subprocess.CompletedProcess` objects with the right stderr format; couples tests to `gh` behavior we already parsed once in the helper. +- **Use a real HTTP mock server (like `responses` or `pytest-httpx`).** Rejected: the transport is `gh` (subprocess), not HTTP; a HTTP mock would not exercise the helper's stderr parsing. Would also introduce a test-only dependency. +- **Full integration tests against `github.com` with a fixture repo.** Considered valuable but out of CI scope. Documented in `quickstart.md` for manual smoke-testing. + +--- + +### R-007: Cap on `considered_rulesets` in evidence record + +**Decision**: On a FAIL verdict, the evidence record's `considered_rulesets` field enumerates every active ruleset that targeted the audited branch but did not satisfy the requirement, capped at 20 entries with a `truncated: N` suffix indicating how many more were seen. On PASS via ruleset, only the matched ruleset appears (no list). On WARN, the field is omitted entirely (source enum values `INSUFFICIENT_ACCESS` and `PARTIAL_FETCH` are self-explanatory). + +**Rationale**: This closes the clarification-session deferral about evidence-record shape on FAIL. Twenty is a generous cap: repos with more than 20 active branch-targeting rulesets that all fail the same requirement are pathological and the excess entries would not add operator value. The `truncated: N` suffix preserves the count for the audit report so a maintainer can see there's more to inspect and know to run `gh api` manually. + +**Alternatives considered**: + +- **No cap.** Rejected: risks bloating audit report file sizes for pathological repos. +- **Log all, evidence-record-summarize as count only.** Considered; rejected because the handler tests want to assert on which rulesets were considered by name, not just count. A capped list preserves testability. +- **20 is arbitrary; parameterize.** Rejected for v0. Add a knob only if a real deployment needs one. + +## Consolidated output + +All NEEDS CLARIFICATION unknowns from Technical Context are resolved. Proceeding to Phase 1. diff --git a/specs/032-ruleset-branch-protection/spec.md b/specs/032-ruleset-branch-protection/spec.md new file mode 100644 index 00000000..0606572b --- /dev/null +++ b/specs/032-ruleset-branch-protection/spec.md @@ -0,0 +1,138 @@ +# Feature Specification: Ruleset-aware branch-protection verdict + +**Feature Branch**: `032-ruleset-branch-protection` + +**Created**: 2026-08-22 + +**Status**: Draft + +**Input**: Consult GitHub Repository Rulesets when the classic branch-protection API returns 404, so branch-protection controls (`OSPS-AC-03.01`, `OSPS-AC-03.02`, `OSPS-QA-03.01`, `OSPS-QA-07.01`) do not produce false FAILs for repos protected via rulesets instead of classic branch protection. Follow-up to feature 019 raised by @justaugustus on issue #343. + +## Clarifications + +### Session 2026-08-22 + +- Q: When do we consult the rulesets endpoint? -> A: Whenever the classic surface did not itself provide the specific required protection (classic 404 OR classic 200 that lacks the required signal). Enables cross-surface layering (e.g., classic requires PRs, ruleset adds required status checks) without paying the always-parallel API cost. +- Q: How does the handler distinguish HTTP status codes for the WARN-vs-FAIL boundary? -> A: Extend the shared `gh_api` helper (or add a sibling) to surface HTTP status metadata, parsed from `gh`'s stderr on non-zero exit ("HTTP :" prefix). Single transport preserved across the codebase; the helper enhancement is scoped and reusable by other controls that face the same distinction. +- Q: What happens when a repo has more rulesets than fit on GitHub's default page? -> A: Use `gh api --paginate` to fetch all rulesets; if fetching any page fails, verdict is WARN. Removes the truncation risk from v0 rather than deferring it. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Repo protected via a repository ruleset is reported as compliant (Priority: P1) + +An operator running a live audit against a repository whose default branch is protected exclusively via a GitHub Repository Ruleset (not classic branch protection) expects the four branch-protection controls to reflect the ruleset-based protection. Today all four resolve to FAIL because the classic protection endpoint 404s and the sieve treats that 404 as definitive absence. + +**Why this priority**: This is a Constitution II violation in the direction that matters most for a compliance tool. Reporting FAIL on a repo that is genuinely compliant erodes trust in the audit's verdict and, in a fleet setting, creates blocking dashboard alerts that require every maintainer to manually reclassify. The fix restores parity between what the tool reports and what GitHub actually enforces. + +**Independent Test**: Point a live audit at a repository whose default branch has (a) no classic branch protection configured and (b) an active repository ruleset targeting the default branch that carries the equivalent protections. Observe the four affected controls resolve PASS. + +**Acceptance Scenarios**: + +1. **Given** a repository whose default branch is covered by an active repository ruleset that requires pull requests before merging, **When** `OSPS-AC-03.01` runs against it, **Then** the control resolves PASS with a reason that names the ruleset as the source of protection. +2. **Given** a repository whose default branch is covered by an active ruleset that blocks branch deletion, **When** `OSPS-AC-03.02` runs against it, **Then** the control resolves PASS naming the ruleset. +3. **Given** a repository whose default branch is covered by an active ruleset that requires status checks, **When** `OSPS-QA-03.01` runs against it, **Then** the control resolves PASS naming the ruleset. +4. **Given** a repository whose default branch is covered by an active ruleset that requires at least one pull-request approval, **When** `OSPS-QA-07.01` runs against it, **Then** the control resolves PASS naming the ruleset. + +--- + +### User Story 2 - Repo with no protection at either surface still FAILs cleanly (Priority: P1) + +An operator running a live audit against a repository whose default branch has no classic branch protection AND no active repository rulesets expects the four branch-protection controls to resolve FAIL. Feature 019 shipped this verdict for the classic-only case, and it must be preserved once the ruleset check is added. + +**Why this priority**: The correctness gained by User Story 1 must not weaken the FAIL semantics for genuinely non-compliant repositories. A false PASS is strictly worse than a false FAIL per constitutional principle II. + +**Independent Test**: Point a live audit at a repository whose default branch has no classic branch protection AND `GET /repos/{owner}/{repo}/rulesets` returns an empty list. All four controls must resolve FAIL with a reason stating that no protection was found via either surface. + +**Acceptance Scenarios**: + +1. **Given** a repository with no classic branch protection and an empty rulesets list, **When** any of the four branch-protection controls runs, **Then** the control resolves FAIL with a message identifying both surfaces as checked. +2. **Given** a repository with no classic branch protection and one ruleset whose enforcement mode is not `active` (for example `evaluate` or `disabled`), **When** any of the four controls runs, **Then** the control resolves FAIL because non-active rulesets do not enforce protection. +3. **Given** a repository with no classic branch protection and one active ruleset whose conditions exclude the audited branch, **When** any of the four controls runs, **Then** the control resolves FAIL because no active ruleset covers the branch. + +--- + +### User Story 3 - Ambiguous responses continue to resolve WARN (Priority: P2) + +An operator whose audit environment cannot cleanly reach GitHub's API (insufficient token permissions, rate limit exhausted, transient network failure) expects branch-protection controls to resolve WARN, not FAIL. Feature 019 preserved this semantic for the classic endpoint; it must extend to the rulesets endpoint. + +**Why this priority**: Constitution II again: "when in doubt, WARN." Confusing "I could not tell" with "definitely fails" reintroduces the exact class of misdiagnosis that motivated the original 019 fix, just at a different code path. + +**Independent Test**: Simulate a `403 Forbidden` response to the rulesets endpoint (or an authenticated request without `admin:read` scope). Observe the four controls resolve WARN with a reason identifying which surface produced the ambiguous response. + +**Acceptance Scenarios**: + +1. **Given** the classic protection endpoint returns 200 with insufficient signal AND the rulesets endpoint returns 403, **When** any of the four controls runs, **Then** it resolves WARN with a reason naming the rulesets access failure. +2. **Given** the classic protection endpoint returns 404 AND the rulesets endpoint times out or returns 5xx, **When** any of the four controls runs, **Then** it resolves WARN with a reason naming the rulesets fetch failure. +3. **Given** the classic protection endpoint returns 404 AND the rulesets endpoint returns 200 with a non-empty list, but fetching an individual ruleset's detail 404s (ruleset was deleted between calls), **When** any of the four controls runs, **Then** it resolves WARN with a reason naming the detail-fetch failure. WARN is chosen over FAIL because the framework could not fully enumerate the protection surface. + +--- + +### Edge Cases + +- **Both surfaces confirm protection**: repo has BOTH classic branch protection AND an active ruleset covering the branch, either alone would satisfy the requirement. The control resolves PASS from whichever surface is checked first (classic, per priority order); the evidence record includes a note that both surfaces are configured. +- **Multiple active rulesets, only one matches**: the requirement is satisfied if ANY single active ruleset covering the branch carries the required rule. The evidence records the specific ruleset by name and id. +- **Ruleset targets the branch but with a differently-parameterized rule**: for example a `pull_request` rule with `required_approving_review_count = 0` for `OSPS-QA-07.01` (which needs at least 1). The ruleset targets the branch but does not satisfy the requirement's parameter; the framework treats this as "did not find satisfying protection here" and continues its evaluation (which, if nothing else satisfies, resolves FAIL). +- **Ref-name conditions**: an active ruleset covers the audited branch when its `conditions.ref_name.include` contains one of: `~DEFAULT_BRANCH` (matched only if the audited branch equals the repository's default branch), the exact branch name, or the git-ref form `refs/heads/{branch}`. Wildcard/glob patterns in `include` MAY match but v0 does not evaluate them (they are treated as "does not match" and are reported in the evidence for the user to inspect). +- **`conditions.ref_name.exclude` present**: if a ruleset's `include` matches the branch but `exclude` also matches, the branch is not covered by that ruleset. The framework treats it the same as if the include did not match. +- **Rulesets fetched but empty rules array**: an active ruleset covering the branch exists but declares no rules. It cannot satisfy any requirement; treated as "does not satisfy" for every requirement. +- **Rate limit exhausted mid-check**: if the rulesets list endpoint succeeds but a per-ruleset detail fetch is 429-rate-limited, verdict is WARN, matching the constitution's err-on-caution principle. +- **Repository has more rulesets than the default page size**: v0 uses `gh api --paginate` and enumerates every page. If any page's fetch fails mid-pagination, the verdict is WARN with source `partial-fetch` (per FR-018), not silent truncation. +- **Non-GitHub platforms**: unchanged. The four affected controls already carry `when = { platform = "github" }`, which excludes them from non-GitHub audits. + +## Requirements *(mandatory)* + +### Functional Requirements + +- **FR-001**: The framework MUST consult the GitHub Repository Rulesets API surface as a secondary source of branch-protection evidence for the four affected controls: `OSPS-AC-03.01`, `OSPS-AC-03.02`, `OSPS-QA-03.01`, `OSPS-QA-07.01`. +- **FR-002**: When the classic branch-protection API returns a positive signal for the specific required protection (200 response whose body carries the exact field/parameter the control tests for), the framework MUST resolve the control PASS from that surface without consulting rulesets (existing behavior; consulted-surface documented in the evidence record). +- **FR-003**: When the classic branch-protection API returns 404 (branch not protected via classic) OR returns 200 without the specific required signal (branch is protected via classic, but not for this control's requirement), the framework MUST consult the rulesets API before concluding a verdict. This preserves cross-surface layering: for example, a repo whose classic protection requires pull requests but leaves required status checks to a ruleset must PASS `OSPS-QA-03.01` via the ruleset, not FAIL because classic alone did not carry the status-checks signal. +- **FR-004**: An active repository ruleset satisfies a requirement iff (a) its `enforcement` field equals `active`, AND (b) its `conditions.ref_name.include` covers the audited branch by one of the matching modes named in Edge Cases, AND (c) its rules list contains at least one rule of the specific type (and parameter, where applicable) required by the control being evaluated. +- **FR-005**: The framework MUST resolve a control FAIL only when BOTH the classic surface AND the rulesets surface respond successfully AND neither provides the required protection. +- **FR-006**: The framework MUST resolve a control WARN when EITHER surface's response is ambiguous or unreachable (network error, authentication error, 5xx, rate-limit, or a partial fetch failure such as list-succeeds-but-detail-fails). +- **FR-007**: The four affected TOML controls MUST be updated to invoke the ruleset-aware verdict logic INSTEAD of the current classic-only exec pass. The manual verification pass at the end of each control's pass list remains unchanged. +- **FR-008**: The evidence record for a control resolved via this feature MUST identify which surface produced the verdict (classic, ruleset, or "neither"), and, when the ruleset surface was consulted, MUST include the name and id of the ruleset that produced the verdict (or, on FAIL, MUST include the summary list of active rulesets that were considered and rejected). +- **FR-009**: The requirement-to-ruleset-rule mapping MUST be: + - `OSPS-AC-03.01` (PreventDirectCommits, requires pull-request workflow): satisfied by an active ruleset covering the branch with a `pull_request` rule. + - `OSPS-AC-03.02` (PreventBranchDeletion): satisfied by an active ruleset covering the branch with a `deletion` rule (i.e., a rule that blocks deletion of the branch). + - `OSPS-QA-03.01` (RequiredStatusChecks): satisfied by an active ruleset covering the branch with a `required_status_checks` rule. + - `OSPS-QA-07.01` (RequiredApprovals): satisfied by an active ruleset covering the branch with a `pull_request` rule whose parameters include `required_approving_review_count >= 1`. +- **FR-010**: The framework MUST NOT change the verdict semantics of any non-branch-protection control. This feature's scope is bounded to the four named controls; no other TOML control is touched. +- **FR-011**: The feature MUST NOT alter the manual-pass fallback that terminates each of the four controls' pass lists. If the automated verdict cannot conclude (WARN), the manual pass provides the operator-facing steps for human verification, unchanged from today. +- **FR-012**: Only rulesets with `enforcement = "active"` count as protection in v0. Rulesets in `enforcement = "evaluate"` (dry-run mode) or `enforcement = "disabled"` state MUST NOT satisfy any requirement. +- **FR-013**: The feature MUST NOT introduce a new runtime dependency on any external package beyond what darnit already declares. The GitHub REST API is consumed via the shared `gh_api` helper (or a status-code-aware sibling introduced by this feature); a single GitHub transport is preserved across the codebase. +- **FR-017**: The framework MUST distinguish HTTP status classes (`200`, `404`, and "other non-200" grouped as ambiguous) when interpreting responses from either the classic or the rulesets endpoint. The shared helper enhancement introduced by this feature MUST parse `gh`'s stderr for the `HTTP :` prefix on non-zero exit and expose the status code to the caller. Absent a parseable status code, the caller MUST treat the response as ambiguous and resolve WARN (FR-006). +- **FR-018**: The framework MUST enumerate ALL rulesets that target the audited branch, not just the first page of results. Implementation MUST use `gh api --paginate` (or equivalent) for the list call. If ANY page fetch fails (including mid-pagination), the verdict MUST be WARN with an evidence source of `partial-fetch`. +- **FR-014**: An audit that never runs any of the four affected controls MUST NOT pay any additional API cost from this feature (rulesets endpoint is only consulted when at least one affected control is being evaluated, and only when the classic surface did not already produce PASS). +- **FR-015**: The evidence record MUST NOT include personally-identifying data or GitHub tokens; ruleset names and ids are metadata that GitHub already exposes to any authenticated caller with `admin:read` on the repository. +- **FR-016**: The framework MUST produce an evidence record whose "source" field for the verdict is one of a fixed enumerated set (`classic`, `ruleset`, `neither-surface-provided-protection`, `insufficient-access`, `partial-fetch`) so downstream reporting can group and count verdicts by source. + +### Key Entities + +- **Branch-protection control**: a compliance control whose PASS/FAIL/WARN verdict depends on whether the repository's default branch (or a specifically-audited branch) is protected in one of the ways OSPS Baseline enumerates. In v0 this is the fixed set of four controls named above. +- **Classic branch protection**: the `/repos/{owner}/{repo}/branches/{branch}/protection` API surface. A repository configured through the repository settings' "Branch protection rules" page populates this surface. Historically the only protection mechanism GitHub offered. +- **Repository ruleset**: the newer protection mechanism accessible via `/repos/{owner}/{repo}/rulesets` and `/repos/{owner}/{repo}/rulesets/{id}`. Rulesets have an `enforcement` mode, a `conditions.ref_name` targeting object with `include` and `exclude` lists, and an ordered list of `rules` whose types encode the specific protections (`pull_request`, `deletion`, `required_status_checks`, `non_fast_forward`, `required_signatures`, ...). +- **Protection requirement**: a per-control declaration of what specific protection the control is testing for. Encoded as an enum value (see FR-009) that the ruleset-aware handler consumes to know which rule type/parameter to look for. +- **Verdict source**: the enumerated field in the evidence record naming which surface produced the verdict. Preserves the constitutional principle that a compliance report should say WHY it reached a conclusion, not just what the conclusion was. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: A live audit against a repository protected exclusively via an active default-branch ruleset produces PASS for all four affected controls (currently produces FAIL). Verifiable end-to-end by pointing an audit at a real repository configured accordingly, or by a fixture-driven integration test that stubs the two GitHub endpoints. +- **SC-002**: A live audit against a repository with neither classic protection nor rulesets continues to produce FAIL for all four affected controls, matching feature 019's shipped behavior. Verifiable by the same test harness as SC-001 with the rulesets fixture set to empty. +- **SC-003**: When either GitHub API surface is unreachable or returns an ambiguous response, the four affected controls resolve WARN instead of FAIL. Verifiable by fault-injection tests that simulate 403/429/5xx/network-error on each surface. +- **SC-004**: An audit that includes at least one of the four affected controls issues at most `ceil(N/page_size)` `rulesets`-list calls plus at most N per-ruleset-detail calls per audit-run per repository, where N is the total count of rulesets on the repository. `page_size` is the GitHub API default (30 as of writing). Verifiable by counting API calls in a mocked-transport test; the property matters for GitHub's rate-limit budget on large fleets. +- **SC-005**: An audit that excludes all four affected controls (via `--tags` filter, `--level 1` when the control is level 2/3, or `.baseline.toml` disable) issues zero calls to the rulesets endpoint. Verifiable by a spy on the GitHub-API transport during a filtered audit. +- **SC-006**: The evidence record's `source` field carries one of the enumerated values in FR-016 for every verdict produced by this feature. Verifiable by an assertion in the four controls' unit tests. +- **SC-007**: No previously-passing branch-protection control regresses. A repository with classic branch protection but no rulesets continues to produce the exact same verdict as it did on `main` prior to this feature. Verifiable by running the four controls against a golden fixture representing the pre-feature success path. + +## Assumptions + +- **Authenticated `gh` CLI**: audits with these controls in scope assume the operator has `gh` authenticated with a token holding enough scope to read repository rulesets. `admin:read` (or the fine-grained equivalent `Read access to repository administration`) is the requirement. Insufficient scope produces WARN via FR-006. +- **Rulesets are queryable at the repository scope**: v0 checks only per-repository rulesets. Organization-level rulesets that a repository inherits are NOT enumerated by `/repos/{owner}/{repo}/rulesets`; they are a v0.1 follow-up (org-level API surface: `/orgs/{org}/rulesets` and per-ruleset detail). +- **All-pages enumeration**: v0 uses `gh api --paginate` for the list call so all rulesets are enumerated regardless of repository count. Rate-limit and partial-fetch failures during pagination resolve WARN per FR-018. +- **The four controls are the whole scope**: no other OSPS control's TOML pass changes as part of this feature. If future controls consult the same protection surface, they can adopt the same handler in a follow-up. +- **Manual pass unchanged**: each of the four controls retains its trailing manual-pass step. This feature changes the first (automated) pass only. +- **`when = { platform = "github" }` guard is preserved**: non-GitHub audits skip these controls entirely and this feature is invisible to them. +- **Evidence record shape is additive**: the new `source` field is added; no existing evidence field is removed or renamed. Downstream consumers that ignore unknown fields continue to work. +- **`gh` CLI is the transport**: consistent with the current classic-branch-protection pass. Feature does not introduce direct `httpx` / `requests` calls; the CLI path handles auth, pagination flags, and rate-limit backoff. diff --git a/specs/032-ruleset-branch-protection/tasks.md b/specs/032-ruleset-branch-protection/tasks.md new file mode 100644 index 00000000..f7ce091b --- /dev/null +++ b/specs/032-ruleset-branch-protection/tasks.md @@ -0,0 +1,282 @@ +--- +description: "Task list for feature 032-ruleset-branch-protection" +--- + +# Tasks: Ruleset-aware branch-protection verdict + +**Input**: Design documents in `specs/032-ruleset-branch-protection/` + +**Prerequisites**: [plan.md](./plan.md), [spec.md](./spec.md), [research.md](./research.md), [data-model.md](./data-model.md), [contracts/github-branch-protection-handler.md](./contracts/github-branch-protection-handler.md), [quickstart.md](./quickstart.md). + +**Tests**: Included. Spec SC-004 (API-call budget) and SC-005 (zero cost when the four controls are excluded) both require mock-transport-counting tests that assert on exact call sequences. Every user story's Independent Test requires a fixture-driven behavior test. Tests are load-bearing. + +**Organization**: One phase per user story after Setup + Foundational. Every user-story task carries a `[USn]` label. Cross-story files (`utils.py` helper tests, framework-design.md sync) are only touched in Setup / Foundational / Polish. + +## Format: `[ID] [P?] [Story] Description` + +- **[P]**: Can run in parallel (different files, no dependencies on incomplete tasks). +- **[Story]**: `[US1]`, `[US2]`, `[US3]` matching spec's user stories. +- File paths are absolute-from-repo-root. + +## Path Conventions + +Single workspace repo. New product code under `packages/darnit-baseline/src/darnit_baseline/branch_protection.py`. One shared helper enhancement under `packages/darnit/src/darnit/core/utils.py`. TOML edits under `packages/darnit-baseline/src/darnit_baseline/openssf-baseline.toml`. New tests under `tests/darnit_baseline/test_branch_protection_handler.py`, `tests/darnit_baseline/test_branch_protection_integration.py`, and `tests/darnit/core/test_gh_api_status.py`. + +## Phase 1: Setup (Shared Infrastructure) + +**Purpose**: Introduce the new module file the rest of the feature builds on, plus the handler-name registry entry so the spec-sync check finds it once code lands. + +- [X] T001 Create `packages/darnit-baseline/src/darnit_baseline/branch_protection.py` with a module docstring naming its purpose (ruleset-aware branch-protection verdict handler; encapsulates classic + rulesets two-surface check for OSPS-AC-03.01 / -03.02 / OSPS-QA-03.01 / OSPS-QA-07.01), the constants `MAX_CONSIDERED_RULESETS = 20`, `DEFAULT_TIMEOUT_SECONDS = 30`, `SUPPORTED_REF_INCLUDE_LITERALS = frozenset({"~DEFAULT_BRANCH", "~ALL"})`, empty declarations for `ProtectionRequirement`, `VerdictSource`, and stub `github_branch_protection_handler(config, context) -> HandlerResult` returning `HandlerResult(status=INCONCLUSIVE, message="not implemented")`. No behavior; scaffold only. Phase 2 fills in body. + +- [X] T002 Add `github_branch_protection` to the handler-name registry table in `docs/architecture/framework-design.md`. Place under the "Sieve handlers (implementation-registered)" section alongside `generate_threat_model`. One-line entry naming the short name, the registering package (`darnit-baseline`), and the docs link (`specs/032-ruleset-branch-protection/contracts/github-branch-protection-handler.md`). Required by `scripts/validate_sync.py`'s handler-name check (T059). + +**Checkpoint**: Module skeleton exists and the spec-sync validator will find the new handler name once T011 wires registration. No behavior yet. + +--- + +## Phase 2: Foundational (Blocking Prerequisites) + +**Purpose**: The shared-helper enhancement and the internal helper functions every user story depends on. Nothing US1-through-US3 can be implemented until these land, because the handler cannot distinguish 200/404/other-non-200 without the enhanced helper and cannot match rulesets without the ref-name helper. + +**CRITICAL**: No user story work begins until this phase completes. + +- [X] T003 Implement `gh_api_with_status(endpoint: str, *, paginate: bool = False) -> tuple[dict | list | None, int, str]` in `packages/darnit/src/darnit/core/utils.py`. On 2xx: `(parsed_json, status_code, "")`. On non-2xx with parseable `HTTP :` prefix in stderr: `(None, status_code, stderr_message)`. On any other failure (subprocess `FileNotFoundError`, `JSONDecodeError` on a 2xx body, network error before request completed): `(None, 0, error_message)`. When `paginate=True`, invoke `gh api --paginate ` and let `gh` concatenate all pages (top-level JSON array). Refactor existing `gh_api()` to be a thin wrapper: call `gh_api_with_status(endpoint)`, raise `RuntimeError(msg or f"status {status}")` on non-200 or non-dict body. Refactor `gh_api_safe()` similarly. All existing callers MUST continue to work unchanged. + +- [X] T004 Implement the `ProtectionRequirement` and `VerdictSource` str enums in `packages/darnit-baseline/src/darnit_baseline/branch_protection.py` per [data-model.md](./data-model.md). ProtectionRequirement members: `REQUIRE_PULL_REQUEST` (value `"require_pull_request"`), `PREVENT_DELETION` (`"prevent_deletion"`), `REQUIRE_STATUS_CHECKS` (`"require_status_checks"`), `REQUIRE_APPROVALS` (`"require_approvals"`). VerdictSource members: `CLASSIC` (`"classic"`), `RULESET` (`"ruleset"`), `NEITHER_SURFACE_PROVIDED_PROTECTION` (`"neither-surface-provided-protection"`), `INSUFFICIENT_ACCESS` (`"insufficient-access"`), `PARTIAL_FETCH` (`"partial-fetch"`). + +- [X] T005 Implement `_ref_name_matches(branch: str, default_branch: str | None, include: list[str], exclude: list[str]) -> bool` in `branch_protection.py`. Return True iff at least one entry in `include` matches AND no entry in `exclude` matches. Match semantics per research R-003: `~DEFAULT_BRANCH` matches iff `default_branch is not None AND branch == default_branch` (when `default_branch is None`, `~DEFAULT_BRANCH` is treated as non-matching per Constitution II conservative-by-default); `~ALL` matches always; exact bare name matches iff equal to `branch`; `refs/heads/` matches iff ` == branch`; any entry containing a glob metacharacter (`*`, `?`, `[`) returns False (documented as a v0 limitation). Function is pure; no I/O. + +- [X] T006 Implement `_ruleset_satisfies(rule: RulesetRule, requirement: ProtectionRequirement, minimum: int) -> tuple[bool, str]` in `branch_protection.py`. Return `(True, "")` when the rule satisfies the requirement per the mapping in data-model.md: `REQUIRE_PULL_REQUEST` -> `rule.type == "pull_request"`; `PREVENT_DELETION` -> `rule.type == "deletion"`; `REQUIRE_STATUS_CHECKS` -> `rule.type == "required_status_checks"`; `REQUIRE_APPROVALS` -> `rule.type == "pull_request"` AND `rule.parameters.required_approving_review_count >= minimum`. On no-match, return `(False, reason)` where `reason` describes why (e.g., `"rule type is deletion, need pull_request"` or `"pull_request rule but required_approving_review_count is 0, need >= 1"`). + +- [X] T007 Implement `_query_classic(owner: str, repo: str, branch: str, requirement: ProtectionRequirement, minimum: int) -> tuple[bool, int, str]` in `branch_protection.py`. Calls `gh_api_with_status(f"/repos/{owner}/{repo}/branches/{branch}/protection")`. Returns `(satisfied: bool, status_code: int, error_message: str)`. On 200: check body for the requirement-specific signal per data-model.md's satisfying-signal table (e.g., `body.get("required_pull_request_reviews") is not None` for REQUIRE_PULL_REQUEST). On 404: `(False, 404, "")`. On any other status: `(False, status, message)`. + +- [X] T008 Implement `_query_rulesets(owner: str, repo: str, branch: str, default_branch: str | None, requirement: ProtectionRequirement, minimum: int) -> tuple[VerdictSource, int, dict | None, list[dict], int]` in `branch_protection.py`. Returns `(source, status, matched_ruleset, considered_rulesets, truncated_count)`. Steps: (a) Call `gh_api_with_status(f"/repos/{owner}/{repo}/rulesets", paginate=True)`. (b) On non-200: return `(INSUFFICIENT_ACCESS, status, None, [], 0)`. (c) For each summary in the list, filter by `enforcement == "active"`; if not active, skip. (d) Fetch detail via `gh_api_with_status(f"/repos/{owner}/{repo}/rulesets/{id}")`. On non-200 detail fetch, return `(PARTIAL_FETCH, status, None, [], 0)`. (e) Verify detail's `enforcement == "active"` and `conditions.ref_name` covers `branch` via `_ref_name_matches(branch, default_branch, include, exclude)`. If not covering, skip. (f) For each rule in `rules`, call `_ruleset_satisfies`; on satisfied, return `(RULESET, 200, {"id": ..., "name": ...}, [], 0)`. If none satisfy, append `{"id": ..., "name": ..., "reason": }` to a working list. (g) After the loop: compute `truncated = max(0, len(working) - MAX_CONSIDERED_RULESETS)`; return `(NEITHER_SURFACE_PROVIDED_PROTECTION, 200, None, working[:MAX_CONSIDERED_RULESETS], truncated)`. The caller composes the evidence record with `considered_rulesets_truncated` set to `truncated`. + +**Checkpoint**: Framework's shared helper distinguishes HTTP status classes; internal helpers can query both surfaces and match rulesets to branches. No handler entry point yet. The default-branch value used for `~DEFAULT_BRANCH` matching flows in from `context.default_branch` (populated by the audit driver at `packages/darnit/src/darnit/tools/audit.py:428`); this feature does NOT add an extra API call to resolve it. + +--- + +## Phase 3: User Story 1 - Repo protected via a repository ruleset is reported compliant (Priority: P1) MVP + +**Goal**: A TOML control with `handler = "github_branch_protection"` produces PASS when the repository is protected via an active ruleset, even when the classic branch-protection endpoint 404s. + +**Independent Test**: Point an audit at a fixture-mocked repository whose classic endpoint returns 404 and whose rulesets endpoint returns one active ruleset with a matching rule for the requirement. Assert the control resolves PASS with `evidence.source == "ruleset"` and populated `matched_ruleset`. + +### Implementation for User Story 1 + +- [X] T010 [US1] Add `github_branch_protection_handler(config: dict[str, Any], context: HandlerContext) -> HandlerResult` in `packages/darnit-baseline/src/darnit_baseline/branch_protection.py`. Body: (a) validate `config["requirement"]` is present and parses to a `ProtectionRequirement`; on failure return `HandlerResult(status=ERROR, message="handler github_branch_protection requires 'requirement' field")`. (b) read `owner` (default `context.owner`), `repo` (default `context.repo`), `branch` (default `context.default_branch`), `required_approvals_minimum` (default 1; validate 1..10 else ERROR), `timeout` (default 30). (c) Call `_query_classic`; on satisfied return `HandlerResult(PASS)` with `evidence = {"source": "classic", "requirement": ..., "classic_status": 200}`. (d) On classic status in `(401, 403, 429)` OR `>= 500` return INCONCLUSIVE with `source="insufficient-access"`. (e) On classic status `0` (unparseable/subprocess-error): INCONCLUSIVE with `source="insufficient-access"`. (f) Otherwise (classic 404 or 200-without-signal): consume the repository's default branch from `context.default_branch` (populated by the audit driver; may be `None` on a partial context, in which case `_ref_name_matches` conservatively treats `~DEFAULT_BRANCH` include entries as non-matching per T005). Call `_query_rulesets(owner, repo, branch, context.default_branch, requirement, required_approvals_minimum)`. Do NOT make an extra `GET /repos/{owner}/{repo}` call to resolve the default branch -- that would violate SC-004's API-call budget. Map returned `source` to the HandlerResult: `RULESET`->PASS, `NEITHER_SURFACE_PROVIDED_PROTECTION`->FAIL, `INSUFFICIENT_ACCESS`/`PARTIAL_FETCH`->INCONCLUSIVE. Populate `evidence["source"]`, `evidence["classic_status"]`, `evidence["rulesets_status"]`, `evidence["requirement"]`, plus `matched_ruleset` (on `RULESET`) or `considered_rulesets` + `considered_rulesets_truncated` (on `NEITHER_SURFACE_PROVIDED_PROTECTION`) per data-model.md. INCONCLUSIVE falls through to the trailing manual pass in the control's pass list (sieve semantic). + +- [X] T011 [US1] Register the new sieve handler in `packages/darnit-baseline/src/darnit_baseline/implementation.py`'s `register_handlers()` method. Add a `sieve_registry.register("github_branch_protection", phase="deterministic", handler_fn=github_branch_protection_handler, default_authority="dispositive", description="Ruleset-aware branch-protection verdict")` call inside the existing sieve-registry block (near `generate_threat_model_handler`). Import from `.branch_protection`. + +- [X] T012 [US1] Update `[[controls."OSPS-AC-03.01".passes]]` in `packages/darnit-baseline/src/darnit_baseline/openssf-baseline.toml`. Replace the existing exec pass at ~line 631 with: + ```toml + [[controls."OSPS-AC-03.01".passes]] + handler = "github_branch_protection" + requirement = "require_pull_request" + timeout = 30 + ``` + Delete the `command`, `pass_exit_codes`, `fail_exit_codes`, `output_format`, `expr` fields. Preserve the manual pass at ~line 640 unchanged. + +- [X] T013 [US1] Update `[[controls."OSPS-AC-03.02".passes]]` in the same TOML at ~line 692. Same replacement pattern with `requirement = "prevent_deletion"`. + +- [X] T014 [US1] Update `[[controls."OSPS-QA-03.01".passes]]` at ~line 2628. Same replacement pattern with `requirement = "require_status_checks"`. + +- [X] T015 [US1] Update `[[controls."OSPS-QA-07.01".passes]]` at ~line 3229. Same replacement pattern with `requirement = "require_approvals"`. Keep `required_approvals_minimum = 1` (matches existing semantic; optional since default is 1). + +- [X] T016 [P] [US1] Create `tests/darnit_baseline/test_branch_protection_handler.py`. Add a `_GhResponseSequencer` fixture that lets tests declare a list of `(endpoint_pattern, response)` tuples where each response is a `(body, status, message)` tuple, then patches `darnit_baseline.branch_protection.gh_api_with_status` to return matched responses in order. Add a `make_context()` factory returning a `HandlerContext` with `owner="octo"`, `repo="hello"`, `default_branch="main"`. + +- [X] T017 [P] [US1] Write `test_ruleset_pull_request_pass` in `test_branch_protection_handler.py`: classic returns 404, rulesets list returns `[{"id": 1, "name": "Protect main", "target": "branch", "enforcement": "active"}]`, ruleset detail returns `{"id": 1, "name": "Protect main", "enforcement": "active", "conditions": {"ref_name": {"include": ["~DEFAULT_BRANCH"], "exclude": []}}, "rules": [{"type": "pull_request", "parameters": {"required_approving_review_count": 1}}]}`. Assert (a) HandlerResult.status == PASS; (b) evidence["source"] == "ruleset"; (c) evidence["matched_ruleset"] == {"id": 1, "name": "Protect main"}; (d) evidence["classic_status"] == 404; (e) evidence["rulesets_status"] == 200. + +- [X] T018 [P] [US1] Write `test_ruleset_deletion_pass` in the same file. Same fixture shape but ruleset has `rules: [{"type": "deletion"}]` and handler config has `requirement="prevent_deletion"`. Assert PASS + source=ruleset. + +- [X] T019 [P] [US1] Write `test_ruleset_status_checks_pass`. Same pattern; requirement=`"require_status_checks"`, rule type `"required_status_checks"`. Assert PASS. + +- [X] T020 [P] [US1] Write `test_ruleset_approvals_pass`. requirement=`"require_approvals"`, rule type `"pull_request"`, `parameters.required_approving_review_count = 1`. Assert PASS. + +- [X] T021 [P] [US1] Write `test_ruleset_approvals_minimum_enforced`. Ruleset has `required_approving_review_count = 1`, handler config has `required_approvals_minimum = 2`. Assert result is FAIL (not PASS): the ruleset targets the branch and has the right rule TYPE, but its parameter does not meet the minimum. Evidence.considered_rulesets[0].reason names the mismatch. + +- [X] T022 [P] [US1] Write `test_ref_name_matching_pseudo_default_branch`. Two variants: (a) ruleset `include = ["~DEFAULT_BRANCH"]` and audited branch equals default -> match (PASS); (b) same but audited branch is `feature/foo` (not the default) -> no match (FAIL, considered_rulesets entry with reason `"ref_name.include does not cover branch feature/foo"`). + +- [X] T023 [P] [US1] Write `test_ref_name_matching_exact_and_git_ref`. Two variants: (a) `include = ["main"]`, audited branch `main` -> match; (b) `include = ["refs/heads/main"]`, audited branch `main` -> match. + +- [X] T024 [P] [US1] Write `test_ref_name_matching_exclude_wins`. `include = ["~ALL"]`, `exclude = ["refs/heads/main"]`, audited branch `main` -> no match. + +- [X] T025 [P] [US1] Write `test_ref_name_matching_glob_treated_as_non_match`. `include = ["refs/heads/release/*"]`, audited branch `release/1.0`. Handler treats glob as non-match, result is FAIL, `considered_rulesets` entry names the ruleset with reason mentioning the glob (research R-003 documented behavior). + +- [X] T026 [P] [US1] Write `test_matched_ruleset_evidence_populated`. Duplicates T017's PASS scenario but with additional assertions: `evidence` MUST NOT contain a `considered_rulesets` key on a ruleset-source PASS. + +**Checkpoint**: A control author's PASS scenario works end-to-end. US1's Independent Test passes. + +--- + +## Phase 4: User Story 2 - Repo with no protection at either surface still FAILs cleanly (Priority: P1) + +**Goal**: An audit continues to FAIL when both surfaces respond definitively and neither carries the required protection. Preserves feature 019's shipped semantic for classic-only-and-still-fails while adding the two-surface check. + +**Independent Test**: Point an audit at a fixture-mocked repository whose classic endpoint returns 404 and whose rulesets list returns an empty array. Assert the control resolves FAIL with `evidence.source == "neither-surface-provided-protection"`. + +### Implementation for User Story 2 + +- [X] T027 [P] [US2] Write `test_no_classic_no_rulesets_fails` in `test_branch_protection_handler.py`: classic 404, rulesets list returns `[]`. Assert FAIL, source=neither-surface-provided-protection, `classic_status=404`, `rulesets_status=200`, `considered_rulesets == []`. + +- [X] T028 [P] [US2] Write `test_no_classic_only_evaluate_mode_rulesets_fails`. Classic 404, rulesets list returns one entry, detail has `enforcement="evaluate"`. Assert FAIL. `considered_rulesets` MAY include the evaluate-mode ruleset with a `"not active enforcement"` reason (implementation choice — either always exclude evaluate-mode from consideration, or include them with a reason). + +- [X] T029 [P] [US2] Write `test_no_classic_rulesets_dont_cover_branch_fails`. Classic 404, active ruleset exists but `conditions.ref_name.include = ["refs/heads/develop"]`, audited branch `main`. Assert FAIL. `considered_rulesets[0].reason` names the non-covering condition. + +- [X] T030 [P] [US2] Write `test_ruleset_empty_rules_array_fails`. Active ruleset covers the branch but `rules: []`. Assert FAIL. `considered_rulesets[0].reason == "no rules declared"` or equivalent. + +- [X] T031 [P] [US2] Write `test_considered_rulesets_populated_on_fail`. Classic 404, three rulesets all failing for distinct reasons (wrong rule type, wrong branch coverage, wrong parameter). Assert `considered_rulesets` has three entries in the order they were seen, each carrying `id`, `name`, and `reason`. Assert `considered_rulesets_truncated == 0`. + +- [X] T032 [P] [US2] Write `test_considered_rulesets_truncation`. Fixture returns 25 non-matching active rulesets. Assert `len(evidence["considered_rulesets"]) == 20` AND `evidence["considered_rulesets_truncated"] == 5`. + +- [X] T033 [P] [US2] Write `test_classic_partial_signal_falls_through_to_rulesets`. Classic returns 200 with `{"allow_deletions": {"enabled": false}}` (satisfies PREVENT_DELETION but NOT REQUIRE_PULL_REQUEST). Handler config requests REQUIRE_PULL_REQUEST. Rulesets contain an active ruleset with a `pull_request` rule targeting the branch. Assert PASS via ruleset, `evidence["source"] == "ruleset"`, evidence["classic_status"] == 200 (not 404). Locks the cross-surface layering behavior from Q1 of the clarification session. + +- [X] T034 [P] [US2] Write `test_both_surfaces_confirm_uses_classic_first`. Classic 200 with `required_pull_request_reviews`, AND rulesets also have a matching active ruleset. Assert PASS with `source == "classic"` (rulesets NOT consulted; verify by asserting the sequencer received zero calls to the rulesets endpoint after the classic call succeeded). + +- [X] T035 [P] [US2] Write `tests/darnit/core/test_gh_api_status.py::test_gh_api_status_200_returns_body`. Patch `subprocess.run` to return `CompletedProcess(args=[...], returncode=0, stdout='{"a":1}', stderr="")`. Call `gh_api_with_status("/some/endpoint")`. Assert `(body, status, msg) == ({"a": 1}, 200, "")`. + +- [X] T036 [P] [US2] Write `test_gh_api_status_404_parses_from_stderr`. Patch to return `(returncode=1, stdout="", stderr="HTTP 404: Not Found (https://api.github.com/...)")`. Assert `(None, 404, "HTTP 404: Not Found (...)")`. + +- [X] T037 [P] [US2] Write `test_gh_api_status_403_parses`. Same shape with `"HTTP 403: Forbidden"`. Assert `(None, 403, ...)`. + +- [X] T038 [P] [US2] Write `test_gh_api_status_5xx_parses`. `"HTTP 502: Bad Gateway"`. Assert `(None, 502, ...)`. + +- [X] T039 [P] [US2] Write `test_gh_api_status_unparseable_stderr_returns_zero`. `stderr="connection reset by peer"` (no HTTP prefix). Assert `(None, 0, "connection reset by peer")`. + +- [X] T040 [P] [US2] Write `test_gh_api_status_paginate_flag`. Patch subprocess.run and capture the argv. Call `gh_api_with_status("/repos/x/y/rulesets", paginate=True)`. Assert argv contains `"--paginate"` between `"api"` and the endpoint. + +- [X] T041 [P] [US2] Write `test_gh_api_status_gh_not_found`. Patch subprocess.run to raise `FileNotFoundError`. Assert `(None, 0, msg)` where msg names `gh not found` and the install URL. + +- [X] T042 [P] [US2] Write `test_gh_api_wrapper_preserves_contract`. Call `gh_api("/some/endpoint")` with 200 body -> returns dict. Call with 404 -> raises `RuntimeError` with the stderr message. Confirms the thin-wrapper refactor. + +- [X] T043 [P] [US2] Write `test_gh_api_safe_wrapper_preserves_contract`. Call `gh_api_safe("/some/endpoint")` with 200 -> returns dict. With any error -> returns None. Confirms `gh_api_safe`'s existing exception-swallow behavior is preserved. + +**Checkpoint**: FAIL semantics for genuinely-non-compliant repos are preserved; the `gh_api_with_status` helper is fully covered. US2's Independent Test passes. + +--- + +## Phase 5: User Story 3 - Ambiguous responses continue to resolve WARN (Priority: P2) + +**Goal**: A control resolves INCONCLUSIVE (which the trailing manual pass converts to WARN with human-verification steps) whenever the framework cannot determine protection status: 401/403/429/5xx on either surface, network error, or partial-fetch mid-pagination. + +**Independent Test**: Point an audit at a fixture-mocked repository whose classic endpoint returns 403 or whose rulesets endpoint returns 429. Assert the control resolves INCONCLUSIVE and, when run through the full sieve orchestrator, the trailing manual pass produces WARN with `evidence.source` naming the ambiguous surface. + +### Implementation for User Story 3 + +- [X] T044 [P] [US3] Write `test_classic_403_returns_inconclusive` in `test_branch_protection_handler.py`. Classic returns `(None, 403, "HTTP 403: Forbidden")`. Assert `HandlerResult.status == INCONCLUSIVE`, `evidence["source"] == "insufficient-access"`, `evidence["classic_status"] == 403`, `evidence.get("rulesets_status", 0) == 0` (rulesets was not consulted because a 403 from classic cannot be distinguished from "no protection classic-side"). + +- [X] T045 [P] [US3] Write `test_rulesets_403_returns_inconclusive`. Classic returns 404, rulesets list returns `(None, 403, ...)`. Assert INCONCLUSIVE, `source == "insufficient-access"`, `classic_status == 404`, `rulesets_status == 403`. + +- [X] T046 [P] [US3] Write `test_classic_5xx_returns_inconclusive`. Classic returns 502. Assert INCONCLUSIVE, source=insufficient-access. + +- [X] T047 [P] [US3] Write `test_rulesets_429_returns_inconclusive`. Classic 404, rulesets list returns 429. Assert INCONCLUSIVE, source=insufficient-access. + +- [X] T048 [P] [US3] Write `test_partial_fetch_returns_inconclusive`. Classic 404, rulesets list returns 200 with one entry, but the detail call for that entry returns 404 (ruleset was deleted between list and detail). Assert INCONCLUSIVE, `source == "partial-fetch"`, `rulesets_status == 200`, and the message names the specific ruleset id that could not be fetched. + +- [X] T049 [P] [US3] Write `test_gh_cli_missing_returns_inconclusive`. Patch `gh_api_with_status` to return `(None, 0, "GitHub CLI (gh) not found. Install it from https://cli.github.com/")` for the classic endpoint. Assert INCONCLUSIVE, source=insufficient-access, message names the install URL. + +- [X] T050 [P] [US3] Write `test_classic_status_zero_returns_inconclusive`. Classic returns `(None, 0, "connection refused")` (unparseable status, e.g., network error before request completed). Assert INCONCLUSIVE, source=insufficient-access. + +**Checkpoint**: WARN semantics for ambiguous cases are preserved on both surfaces plus in the partial-fetch case. US3's Independent Test passes. + +--- + +## Phase 6: Integration tests through the sieve orchestrator + +**Purpose**: End-to-end verification that the four TOML controls resolve correctly when driven by the actual sieve orchestrator (not just direct handler calls). Locks the TOML edits from T012-T015 against silent regression and confirms the trailing manual pass produces the intended WARN when the handler is INCONCLUSIVE. + +- [X] T051 [P] [US1] Write `tests/darnit_baseline/test_branch_protection_integration.py::test_osps_ac_03_01_pass_via_ruleset`. Construct a real `SieveOrchestrator`, load the openssf-baseline TOML, look up `OSPS-AC-03.01`, patch `darnit_baseline.branch_protection.gh_api_with_status` to return ruleset-satisfying responses. Call `orchestrator.verify(control_spec, context)`. Assert `result.status == "PASS"`, `result.evidence["source"] == "ruleset"`. + +- [X] T052 [P] [US1] Same shape for `OSPS-AC-03.02` (requirement=prevent_deletion, rule type=deletion). + +- [X] T053 [P] [US1] Same shape for `OSPS-QA-03.01` (requirement=require_status_checks). + +- [X] T054 [P] [US1] Same shape for `OSPS-QA-07.01` (requirement=require_approvals, ruleset with required_approving_review_count=1). + +- [X] T055 [P] [US2] Write `test_osps_ac_03_01_fail_when_no_protection`. Fixture returns classic 404 + empty rulesets. Assert `result.status == "FAIL"`, `result.evidence["source"] == "neither-surface-provided-protection"`. Locks the feature-019 baseline FAIL semantic on the true-negative path. + +- [X] T056 [P] [US3] Write `test_osps_ac_03_01_warn_when_403_falls_through_to_manual`. Fixture returns classic 403. Assert the full pass chain resolves to WARN (handler INCONCLUSIVE -> trailing manual pass produces WARN with verification steps). Evidence from the handler pass carries `source == "insufficient-access"`. + +- [X] T057 [P] [US2] Write `test_zero_rulesets_calls_when_four_controls_excluded`. Run the orchestrator against a control filter that excludes all four affected controls (e.g., `--tags` filter or a scope that only includes an unrelated control). Assert zero calls to `gh_api_with_status` for any endpoint containing `/rulesets`. Locks SC-005 (zero cost when the four controls are excluded). + +- [X] T057a [P] [US2] Write `test_api_call_budget_matches_sc_004` in `test_branch_protection_integration.py`. Fixture: classic 404 + rulesets list returning 3 active rulesets (spread across a single page) + 3 detail-fetch responses that all target the branch but do NOT satisfy the requirement (so the handler exhausts all three and resolves FAIL). Use the `_GhResponseSequencer` in strict mode (assertions on order AND count). Assert (a) exactly ONE call to the classic branch-protection endpoint, (b) exactly ONE call to `/rulesets` (single page, per `gh api --paginate` behavior for a fits-in-one-page response), (c) exactly THREE calls to `/rulesets/{id}` (one per active summary), (d) zero calls to any other GitHub endpoint (specifically NOT `/repos/{owner}/{repo}` since T010 uses `context.default_branch` per F1 remediation). Locks spec SC-004's exact budget formula: `1 classic + ceil(N/page_size) list + N detail` when the ruleset list fits in one page. + +--- + +## Phase 7: Polish & Cross-Cutting Concerns + +**Purpose**: Full workspace verification, scope guard, lint clean, spec-sync validation, product-scope invariant. + +- [X] T058 Run the full workspace test sweep from repo root: `uv run pytest tests/ -q --deselect tests/darnit/context/test_dot_project_upstream.py::TestUpstreamSpecSync::test_upstream_spec_unchanged`. Confirm exit code 0. + +- [X] T059 [P] Two sub-steps, both MUST pass. **(a) Structure Decision**: `git diff --name-only main..HEAD | grep -E 'packages/(darnit-gittuf|darnit-reproducibility|darnit-hello)/src/'` MUST produce zero lines (only `darnit-baseline/` and one file in `darnit/core/utils.py` are touched under `packages/*/src/`). **(b) FR-013 no-new-runtime-dep guard**: `git diff main..HEAD -- pyproject.toml packages/*/pyproject.toml` MUST be empty (no new deps introduced). + +- [X] T060 [P] Run `uv run ruff check .` on repo root; MUST exit 0. Fix any lint issues in the files this feature touched; do NOT auto-format unrelated files. + +- [X] T061 [P] Run `uv run python scripts/validate_sync.py --verbose`; MUST exit 0. Specifically confirm the "Pass Types Sync: Handler names in sync" line now includes `github_branch_protection`. This proves T002 correctly wired the handler-name registry entry. + +- [X] T062 Confirm the module docstring on `packages/darnit-baseline/src/darnit_baseline/branch_protection.py` accurately describes the final implementation (specifically the two-surface flow from T010 and the `_ref_name_matches` semantics from T005). Fix any docstring/code drift. Also confirm the `contracts/github-branch-protection-handler.md` failure-mode table matches every distinguishable path in T010's `HandlerResult` construction. + +- [X] T063 Manually verify the `help_md` sections for all four affected controls in `openssf-baseline.toml` still make sense. The user-facing remediation guidance did not change (still says "enable branch protection"), but the automated-check description language may benefit from a small addition mentioning that either classic protection OR a repository ruleset satisfies. Optional; keep changes minimal. + +--- + +## Dependencies + +``` +Phase 1 (T001..T002) --> Phase 2 (T003..T008) --> Phase 3 (US1: T010..T026) + | + +--> Phase 4 (US2: T027..T043) [all [P] within phase after T010-T015 land] + | + +--> Phase 5 (US3: T044..T050) + | + +--> Phase 6 (Integration: T051..T057a) + | + +--> Phase 7 (Polish: T058..T063) +``` + +Phase 1 tasks T001 and T002 touch different files -- can run `[P]` but listed sequentially for reviewer readability. + +Within Phase 2, T003 (utils.py) is independent of T004-T008 (branch_protection.py). T004-T008 all touch `branch_protection.py` and must serialize on it, but they are internal helpers and can be authored in any order relative to each other; sequential is preferred. + +Within Phase 3, T010 must land before T011-T015 (TOML edits reference the handler). T012-T015 touch the same TOML file and must serialize. T016-T026 are test tasks and can be authored in parallel (pytest handles concurrent test-file additions cleanly). + +Within Phase 4, T035-T043 (`test_gh_api_status.py`) is a different file from T027-T034 (`test_branch_protection_handler.py`) and can run in a separate work stream. + +Within Phase 6, T051-T057 all touch `test_branch_protection_integration.py` and must serialize on that file. + +## Parallel execution examples + +After Phase 3 (US1) MVP lands, US2/US3/US6 test tasks are largely disjoint: + +```sh +# Fire US2 handler tests, US2 helper tests, and US3 tests concurrently. +uv run pytest tests/darnit_baseline/test_branch_protection_handler.py -k "no_protection or considered_rulesets or partial_signal or both_surfaces" -q & +uv run pytest tests/darnit/core/test_gh_api_status.py -q & +uv run pytest tests/darnit_baseline/test_branch_protection_handler.py -k "403 or 429 or 5xx or partial_fetch or gh_cli_missing" -q & +wait +``` + +Within Phase 7: + +```sh +uv run pytest tests/ -q --deselect ... # T058 (long-running; start it first) +git diff --name-only main..HEAD | grep -E ... # T059 (fast, [P]) +uv run ruff check . # T060 (fast, [P]) +uv run python scripts/validate_sync.py --verbose # T061 (fast, [P]) +# T062, T063 run last, require final state +``` + +## Implementation strategy + +MVP scope = Phase 1 + Phase 2 + Phase 3 (User Story 1 alone). Landing US1 gets the machinery working end-to-end against the mock and delivers the P1 goal from the spec: a repo protected via a ruleset resolves PASS. Everything after that locks failure semantics and adds regression coverage. + +Incremental delivery order: + +1. Land T001..T026 (Setup + Foundational + US1) as the MVP PR. At this point the handler works, the four TOMLs are updated, and the ruleset-source PASS path is fully tested. +2. Land T027..T043 (US2 + `gh_api_with_status` coverage) as a follow-up commit or same PR. Locks the FAIL-preservation invariant and the helper's contract. +3. Land T044..T050 (US3) as a follow-up commit. Locks the WARN-preservation invariant. +4. Land T051..T057 (integration through orchestrator) as a follow-up commit. End-to-end regression suite. +5. Land T058..T063 (polish) as the last commit or squash into the MVP. + +All commits belong to the same PR against `main` unless the review size demands a split. If piecewise review is preferred, reviewer order is (foundational + US1 code, US2 tests, US3 tests, integration, polish) so each commit's contract-level effect is legible independently. diff --git a/tests/darnit/core/test_gh_api_status.py b/tests/darnit/core/test_gh_api_status.py new file mode 100644 index 00000000..1cf347f1 --- /dev/null +++ b/tests/darnit/core/test_gh_api_status.py @@ -0,0 +1,144 @@ +"""Unit tests for the `gh_api_with_status` helper (feature 032). + +Mocks `subprocess.run` directly to exercise the helper's status-code +extraction from `gh`'s stderr. Verifies the thin-wrapper contracts of +`gh_api` and `gh_api_safe` are preserved. +""" + +from __future__ import annotations + +import subprocess +from unittest.mock import patch + +import pytest + +from darnit.core import utils + + +def _cp(returncode: int, stdout: str = "", stderr: str = "") -> subprocess.CompletedProcess: + return subprocess.CompletedProcess( + args=["gh", "api", "..."], returncode=returncode, stdout=stdout, stderr=stderr + ) + + +class TestGhApiStatus: + def test_200_dict_body(self): + with patch("darnit.core.utils.subprocess.run", return_value=_cp(0, '{"a":1}')): + body, status, err = utils.gh_api_with_status("/x") + assert body == {"a": 1} + assert status == 200 + assert err == "" + + def test_200_list_body(self): + with patch("darnit.core.utils.subprocess.run", return_value=_cp(0, '[{"id":1}]')): + body, status, err = utils.gh_api_with_status("/repos/x/y/rulesets") + assert body == [{"id": 1}] + assert status == 200 + assert err == "" + + def test_200_empty_body(self): + with patch("darnit.core.utils.subprocess.run", return_value=_cp(0, "")): + body, status, err = utils.gh_api_with_status("/x") + assert body is None + assert status == 200 + + def test_404_parses_from_stderr(self): + stderr = "HTTP 404: Not Found (https://api.github.com/repos/x/y/branches/main/protection)" + with patch("darnit.core.utils.subprocess.run", return_value=_cp(1, "", stderr)): + body, status, err = utils.gh_api_with_status("/x") + assert body is None + assert status == 404 + assert "HTTP 404" in err + + def test_403_parses(self): + with patch("darnit.core.utils.subprocess.run", return_value=_cp(1, "", "HTTP 403: Forbidden")): + body, status, _ = utils.gh_api_with_status("/x") + assert status == 403 + + def test_401_parses(self): + with patch("darnit.core.utils.subprocess.run", return_value=_cp(1, "", "HTTP 401: Unauthorized")): + _, status, _ = utils.gh_api_with_status("/x") + assert status == 401 + + def test_429_parses(self): + with patch("darnit.core.utils.subprocess.run", return_value=_cp(1, "", "HTTP 429: Too Many Requests")): + _, status, _ = utils.gh_api_with_status("/x") + assert status == 429 + + def test_5xx_parses(self): + with patch("darnit.core.utils.subprocess.run", return_value=_cp(1, "", "HTTP 502: Bad Gateway")): + _, status, _ = utils.gh_api_with_status("/x") + assert status == 502 + + def test_unparseable_stderr_returns_zero(self): + with patch("darnit.core.utils.subprocess.run", return_value=_cp(1, "", "connection reset by peer")): + body, status, err = utils.gh_api_with_status("/x") + assert body is None + assert status == 0 + assert "connection reset" in err + + def test_empty_stderr_returns_zero(self): + with patch("darnit.core.utils.subprocess.run", return_value=_cp(1, "", "")): + body, status, err = utils.gh_api_with_status("/x") + assert status == 0 + assert "exit code 1" in err + + def test_paginate_flag_in_argv(self): + captured = {} + + def _run(args, capture_output=None, text=None): + captured["args"] = args + return _cp(0, "[]") + + with patch("darnit.core.utils.subprocess.run", side_effect=_run): + utils.gh_api_with_status("/repos/x/y/rulesets", paginate=True) + assert captured["args"] == ["gh", "api", "--paginate", "/repos/x/y/rulesets"] + + def test_paginate_false_by_default(self): + captured = {} + + def _run(args, capture_output=None, text=None): + captured["args"] = args + return _cp(0, "{}") + + with patch("darnit.core.utils.subprocess.run", side_effect=_run): + utils.gh_api_with_status("/x") + assert "--paginate" not in captured["args"] + + def test_gh_not_found(self): + with patch("darnit.core.utils.subprocess.run", side_effect=FileNotFoundError()): + body, status, err = utils.gh_api_with_status("/x") + assert body is None + assert status == 0 + assert "gh) not found" in err + + def test_json_decode_error_on_2xx(self): + with patch("darnit.core.utils.subprocess.run", return_value=_cp(0, "not json")): + body, status, err = utils.gh_api_with_status("/x") + assert body is None + assert status == 0 + assert "invalid JSON" in err + + +class TestWrapperContracts: + def test_gh_api_returns_dict(self): + with patch("darnit.core.utils.subprocess.run", return_value=_cp(0, '{"a":1}')): + assert utils.gh_api("/x") == {"a": 1} + + def test_gh_api_raises_on_non_200(self): + with patch("darnit.core.utils.subprocess.run", return_value=_cp(1, "", "HTTP 404: Not Found")): + with pytest.raises(RuntimeError, match="404"): + utils.gh_api("/x") + + def test_gh_api_raises_on_non_dict_body(self): + with patch("darnit.core.utils.subprocess.run", return_value=_cp(0, "[1,2]")): + with pytest.raises(RuntimeError, match="expected dict"): + utils.gh_api("/x") + + def test_gh_api_safe_returns_dict(self): + with patch("darnit.core.utils.subprocess.run", return_value=_cp(0, '{"a":1}')): + assert utils.gh_api_safe("/x") == {"a": 1} + + def test_gh_api_safe_returns_none_on_failure(self): + with patch("darnit.core.utils.subprocess.run", return_value=_cp(1, "", "HTTP 404: Not Found")): + assert utils.gh_api_safe("/x") is None diff --git a/tests/darnit_baseline/controls/test_branch_protection.py b/tests/darnit_baseline/controls/test_branch_protection.py index 3d606d31..9b27e65e 100644 --- a/tests/darnit_baseline/controls/test_branch_protection.py +++ b/tests/darnit_baseline/controls/test_branch_protection.py @@ -1,33 +1,29 @@ -"""Integration tests for the four branch-protection controls (feature 020, issue #343). +"""Integration tests for the four branch-protection controls. -Verifies that when the GitHub API returns a definitive 404 "Branch not -protected" response, each of the following controls resolves to FAIL -(not WARN, not INCONCLUSIVE): +Verifies: +- 404 from classic AND empty rulesets list -> FAIL (feature 019 semantic + preserved under feature 032's two-surface check for the true-negative + case). +- Healthy 200 from classic -> PASS via classic surface (no ruleset + consultation). -- OSPS-AC-03.01 (PreventDirectCommits) -- OSPS-AC-03.02 (PreventBranchDeletion) -- OSPS-QA-03.01 (RequiredStatusChecks) -- OSPS-QA-07.01 (RequiredApprovals) +The controls: OSPS-AC-03.01, OSPS-AC-03.02, OSPS-QA-03.01, OSPS-QA-07.01. -Also verifies the happy path (200 with healthy branch-protection body -> -PASS) does not regress. - -Tests patch `subprocess.run` in the exec handler's module so the test -does not require `gh` on PATH or network access. +Tests patch `darnit_baseline.branch_protection.gh_api_with_status` +(function-level substitution) so the tests do not require `gh` on PATH or +network access. """ from __future__ import annotations -import json -from unittest.mock import patch - import pytest from darnit.config.merger import load_framework_by_name from darnit.core.plugin import ControlSpec -from darnit.sieve.handler_registry import HandlerContext # noqa: F401 (documentation) +from darnit.sieve.handler_registry import reset_sieve_handler_registry from darnit.sieve.models import CheckContext from darnit.sieve.orchestrator import SieveOrchestrator +from darnit_baseline import branch_protection as bp NAMED_CONTROLS = ( "OSPS-AC-03.01", @@ -36,13 +32,7 @@ "OSPS-QA-07.01", ) -BRANCH_NOT_PROTECTED_BODY = json.dumps({ - "message": "Branch not protected", - "documentation_url": "https://docs.github.com/rest/branches/branch-protection#get-branch-protection", - "status": "404", -}) - -HEALTHY_PROTECTION_BODY = json.dumps({ +HEALTHY_PROTECTION_BODY = { "required_pull_request_reviews": { "required_approving_review_count": 1, "dismiss_stale_reviews": True, @@ -56,35 +46,34 @@ "allow_force_pushes": {"enabled": False}, "restrictions": None, "url": "https://api.github.com/repos/testorg/testrepo/branches/main/protection", -}) +} -def _load_control(control_id: str) -> ControlSpec: - """Load a real ControlSpec from openssf-baseline.toml. +@pytest.fixture(autouse=True) +def _register_baseline_handlers(): + reset_sieve_handler_registry() + from darnit.core.discovery import get_implementation - Uses the framework's own loader so we exercise the exact metadata - (passes, handler_invocations, etc.) that ships in the TOML. - """ - config = load_framework_by_name("openssf-baseline") - control = config.controls[control_id] + impl = get_implementation("openssf-baseline") + assert impl is not None, "openssf-baseline implementation not discovered" + impl.register_handlers() + yield - # Build a ControlSpec that carries the handler_invocations metadata - # the orchestrator expects. This mirrors how darnit-baseline's - # implementation.get_all_controls() constructs its ControlSpecs. + +def _load_control(control_id: str) -> ControlSpec: + """Load a real ControlSpec from openssf-baseline.toml.""" + config = load_framework_by_name(control_id.split("-", 1)[0].lower() if False else "openssf-baseline") + control = config.controls[control_id] tags = control.tags or {} level = control.level if control.level is not None else tags.get("level", 1) domain = control.domain if control.domain is not None else tags.get("domain", "UNKNOWN") - return ControlSpec( control_id=control_id, name=control.name, description=control.description or "", level=level, domain=domain, - metadata={ - "handler_invocations": control.passes, - "when": control.when, - }, + metadata={"handler_invocations": control.passes, "when": control.when}, ) @@ -99,98 +88,79 @@ def _make_context(control_id: str) -> CheckContext: ) -class _FakeSubprocessResult: - """Minimal stand-in for subprocess.CompletedProcess.""" +def _install_gh_mock(monkeypatch, responses): + """Substitute a canned response sequence into the handler's gh helper.""" + queue = list(responses) - def __init__(self, returncode: int, stdout: str = "", stderr: str = ""): - self.returncode = returncode - self.stdout = stdout - self.stderr = stderr - - -@pytest.fixture() -def patched_gh_404(): - """Patch subprocess.run so `gh api` returns HTTP 404 'Branch not protected'.""" - def _fake_run(*args, **kwargs): - return _FakeSubprocessResult( - returncode=1, - stdout=BRANCH_NOT_PROTECTED_BODY, - stderr="", - ) - - with patch("darnit.sieve.builtin_handlers.subprocess.run", side_effect=_fake_run): - yield - - -@pytest.fixture() -def patched_gh_200_healthy(): - """Patch subprocess.run so `gh api` returns HTTP 200 with a healthy protection body. - - For OSPS-QA-07.01 the command adds `--jq - '.required_pull_request_reviews.required_approving_review_count >= 1'`, - so its stdout should be the string `true`. Other three controls receive - the full JSON. - """ - def _fake_run(*args, **kwargs): - cmd = args[0] if args else kwargs.get("args", []) - if any("--jq" in str(a) for a in cmd): - return _FakeSubprocessResult(returncode=0, stdout="true\n", stderr="") - return _FakeSubprocessResult( - returncode=0, - stdout=HEALTHY_PROTECTION_BODY, - stderr="", + def _fake(endpoint, *, paginate=False): + for i, (pat, body, status, msg) in enumerate(queue): + if pat in endpoint or endpoint.endswith(pat): + queue.pop(i) + return body, status, msg + raise AssertionError( + f"unexpected gh_api_with_status call to {endpoint!r} " + f"(paginate={paginate}); remaining={[p for p, *_ in queue]}" ) - with patch("darnit.sieve.builtin_handlers.subprocess.run", side_effect=_fake_run): - yield + monkeypatch.setattr(bp, "gh_api_with_status", _fake) # --------------------------------------------------------------------------- -# FR-007 acceptance: definitive 404 -> FAIL +# Definitive negative: classic 404 + empty rulesets -> FAIL # --------------------------------------------------------------------------- -class TestDefinitive404ReportsFail: - """The four named branch-protection controls MUST report FAIL on 404 - 'Branch not protected' after feature 020 lands.""" +class TestNoProtectionReportsFail: + """The four named branch-protection controls MUST report FAIL when + both surfaces respond definitively and neither confirms protection. + Feature 019 established this invariant for the classic-only case; + feature 032 preserves it under the two-surface check.""" @pytest.mark.unit @pytest.mark.parametrize("control_id", NAMED_CONTROLS) - def test_control_resolves_fail_on_branch_not_protected(self, control_id, patched_gh_404): + def test_control_resolves_fail_on_branch_not_protected(self, control_id, monkeypatch): + _install_gh_mock(monkeypatch, [ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", [], 200, ""), + ]) + spec = _load_control(control_id) context = _make_context(control_id) orchestrator = SieveOrchestrator(stop_on_llm=True) - result = orchestrator.verify(spec, context) legacy = result.to_legacy_dict() assert legacy["status"] == "FAIL", ( - f"{control_id}: expected FAIL on 404 'Branch not protected', " - f"got {legacy['status']!r}. Message: {legacy.get('message')!r}" + f"{control_id}: expected FAIL on 404 + no rulesets, got " + f"{legacy['status']!r}. Message: {legacy.get('message')!r}" ) # --------------------------------------------------------------------------- -# FR-009 regression guard: healthy 200 -> PASS +# Healthy positive: 200 with all fields -> PASS via classic surface # --------------------------------------------------------------------------- class TestHealthyResponsePasses: - """Regression guard: when branch protection IS enabled with the expected - fields, the four named controls MUST still resolve to PASS. Feature 020's - orchestrator change must not affect this path.""" + """Regression guard: a healthy classic branch-protection body still + produces PASS across the four controls without consulting rulesets. + Feature 032's cross-surface change must not affect this path.""" @pytest.mark.unit @pytest.mark.parametrize("control_id", NAMED_CONTROLS) - def test_control_resolves_pass_on_healthy_body(self, control_id, patched_gh_200_healthy): + def test_control_resolves_pass_on_healthy_body(self, control_id, monkeypatch): + # Only the classic call is expected -- rulesets are never consulted. + _install_gh_mock(monkeypatch, [ + ("/branches/main/protection", HEALTHY_PROTECTION_BODY, 200, ""), + ]) + spec = _load_control(control_id) context = _make_context(control_id) orchestrator = SieveOrchestrator(stop_on_llm=True) - result = orchestrator.verify(spec, context) legacy = result.to_legacy_dict() assert legacy["status"] == "PASS", ( - f"{control_id}: expected PASS on healthy branch-protection body, " - f"got {legacy['status']!r}. Message: {legacy.get('message')!r}" + f"{control_id}: expected PASS on healthy body, got " + f"{legacy['status']!r}. Message: {legacy.get('message')!r}" ) diff --git a/tests/darnit_baseline/test_branch_protection_handler.py b/tests/darnit_baseline/test_branch_protection_handler.py new file mode 100644 index 00000000..5a4fb910 --- /dev/null +++ b/tests/darnit_baseline/test_branch_protection_handler.py @@ -0,0 +1,628 @@ +"""Unit tests for the github_branch_protection sieve handler. + +Every test mocks at `darnit_baseline.branch_protection.gh_api_with_status` +(module-level substitution) rather than at `subprocess.run`, so the tests +are robust across gh CLI version changes and can assert on the exact +sequence of API calls the handler emits (spec SC-004). +""" + +from __future__ import annotations + +from typing import Any + +import pytest + +from darnit.sieve.handler_registry import HandlerContext, HandlerResultStatus +from darnit_baseline import branch_protection as bp +from darnit_baseline.branch_protection import ( + ProtectionRequirement, + _ref_name_matches, + _ruleset_satisfies, + github_branch_protection_handler, +) + +# --------------------------------------------------------------------------- +# Response-sequencer helper +# --------------------------------------------------------------------------- + + +class _GhResponseSequencer: + """Feeds pre-canned (body, status, message) tuples for gh_api_with_status. + + Records every call so tests can assert on call count and order. + """ + + def __init__(self, responses: list[tuple[str, Any, int, str]]): + """responses: list of (endpoint_glob, body, status, message).""" + self._queue = list(responses) + self.calls: list[tuple[str, bool]] = [] + + def __call__( + self, endpoint: str, *, paginate: bool = False + ) -> tuple[Any, int, str]: + self.calls.append((endpoint, paginate)) + for i, (pat, body, status, msg) in enumerate(self._queue): + if pat in endpoint or endpoint.endswith(pat): + self._queue.pop(i) + return body, status, msg + raise AssertionError( + f"unexpected gh_api_with_status call to {endpoint!r} " + f"(paginate={paginate}); remaining queue={[p for p,*_ in self._queue]}" + ) + + +@pytest.fixture() +def ctx() -> HandlerContext: + return HandlerContext( + local_path="/tmp", + owner="octo", + repo="hello", + default_branch="main", + control_id="TEST-01", + ) + + +def _install(monkeypatch, seq: _GhResponseSequencer) -> None: + monkeypatch.setattr(bp, "gh_api_with_status", seq) + + +# --------------------------------------------------------------------------- +# Pure helper tests (T005 ref-name matching, T006 ruleset-satisfies) +# --------------------------------------------------------------------------- + + +class TestRefNameMatching: + def test_default_branch_matches_when_equal(self): + assert _ref_name_matches("main", "main", ["~DEFAULT_BRANCH"], []) is True + + def test_default_branch_no_match_when_different(self): + assert _ref_name_matches("feature/foo", "main", ["~DEFAULT_BRANCH"], []) is False + + def test_default_branch_conservative_when_none(self): + assert _ref_name_matches("main", None, ["~DEFAULT_BRANCH"], []) is False + + def test_all_matches(self): + assert _ref_name_matches("anything", "main", ["~ALL"], []) is True + + def test_exact_bare_name(self): + assert _ref_name_matches("main", "main", ["main"], []) is True + + def test_git_ref_form(self): + assert _ref_name_matches("main", "main", ["refs/heads/main"], []) is True + + def test_exclude_wins(self): + assert _ref_name_matches("main", "main", ["~ALL"], ["refs/heads/main"]) is False + + def test_glob_treated_as_non_match(self): + assert _ref_name_matches("release/1.0", "main", ["refs/heads/release/*"], []) is False + assert _ref_name_matches("main", "main", ["mai?"], []) is False + assert _ref_name_matches("main", "main", ["[m]ain"], []) is False + + def test_no_include_no_match(self): + assert _ref_name_matches("main", "main", [], []) is False + assert _ref_name_matches("main", "main", None, None) is False + + +class TestRulesetSatisfies: + def test_pull_request_ok(self): + ok, _ = _ruleset_satisfies({"type": "pull_request"}, ProtectionRequirement.REQUIRE_PULL_REQUEST, 1) + assert ok + + def test_pull_request_wrong_type(self): + ok, reason = _ruleset_satisfies({"type": "deletion"}, ProtectionRequirement.REQUIRE_PULL_REQUEST, 1) + assert ok is False and "need 'pull_request'" in reason + + def test_deletion_ok(self): + ok, _ = _ruleset_satisfies({"type": "deletion"}, ProtectionRequirement.PREVENT_DELETION, 1) + assert ok + + def test_status_checks_ok(self): + ok, _ = _ruleset_satisfies({"type": "required_status_checks"}, ProtectionRequirement.REQUIRE_STATUS_CHECKS, 1) + assert ok + + def test_approvals_meets_minimum(self): + ok, _ = _ruleset_satisfies( + {"type": "pull_request", "parameters": {"required_approving_review_count": 2}}, + ProtectionRequirement.REQUIRE_APPROVALS, + 2, + ) + assert ok + + def test_approvals_below_minimum(self): + ok, reason = _ruleset_satisfies( + {"type": "pull_request", "parameters": {"required_approving_review_count": 1}}, + ProtectionRequirement.REQUIRE_APPROVALS, + 2, + ) + assert ok is False and "is 1, need >= 2" in reason + + def test_approvals_wrong_rule_type(self): + ok, reason = _ruleset_satisfies( + {"type": "deletion"}, ProtectionRequirement.REQUIRE_APPROVALS, 1 + ) + assert ok is False and "need 'pull_request'" in reason + + +# --------------------------------------------------------------------------- +# US1: PASS via ruleset (T017-T026) +# --------------------------------------------------------------------------- + + +def _ruleset_detail(rule_type: str, review_count: int = 1) -> dict[str, Any]: + return { + "id": 1, + "name": "Protect main", + "enforcement": "active", + "conditions": {"ref_name": {"include": ["~DEFAULT_BRANCH"], "exclude": []}}, + "rules": [ + {"type": rule_type, "parameters": {"required_approving_review_count": review_count}} + if rule_type == "pull_request" + else {"type": rule_type} + ], + } + + +class TestRulesetPass: + def test_ruleset_pull_request_pass(self, monkeypatch, ctx): + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", [{"id": 1, "name": "Protect main", "target": "branch", "enforcement": "active"}], 200, ""), + ("/rulesets/1", _ruleset_detail("pull_request"), 200, ""), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.PASS, result.message + ev = result.evidence + assert ev["source"] == "ruleset" + assert ev["matched_ruleset"] == {"id": 1, "name": "Protect main"} + assert ev["classic_status"] == 404 + assert ev["rulesets_status"] == 200 + + def test_ruleset_deletion_pass(self, monkeypatch, ctx): + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", [{"id": 1, "name": "R", "target": "branch", "enforcement": "active"}], 200, ""), + ("/rulesets/1", _ruleset_detail("deletion"), 200, ""), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "prevent_deletion"}, ctx + ) + assert result.status == HandlerResultStatus.PASS + assert result.evidence["source"] == "ruleset" + + def test_ruleset_status_checks_pass(self, monkeypatch, ctx): + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", [{"id": 1, "name": "R", "target": "branch", "enforcement": "active"}], 200, ""), + ("/rulesets/1", _ruleset_detail("required_status_checks"), 200, ""), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_status_checks"}, ctx + ) + assert result.status == HandlerResultStatus.PASS + assert result.evidence["source"] == "ruleset" + + def test_ruleset_approvals_pass(self, monkeypatch, ctx): + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", [{"id": 1, "name": "R", "target": "branch", "enforcement": "active"}], 200, ""), + ("/rulesets/1", _ruleset_detail("pull_request", review_count=1), 200, ""), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_approvals"}, ctx + ) + assert result.status == HandlerResultStatus.PASS + assert result.evidence["source"] == "ruleset" + + def test_ruleset_approvals_minimum_enforced(self, monkeypatch, ctx): + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", [{"id": 1, "name": "Skimpy", "target": "branch", "enforcement": "active"}], 200, ""), + ("/rulesets/1", _ruleset_detail("pull_request", review_count=1), 200, ""), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_approvals", "required_approvals_minimum": 2}, ctx + ) + assert result.status == HandlerResultStatus.FAIL + assert result.evidence["source"] == "neither-surface-provided-protection" + assert "is 1, need >= 2" in result.evidence["considered_rulesets"][0]["reason"] + + def test_matched_ruleset_evidence_no_considered_list(self, monkeypatch, ctx): + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", [{"id": 1, "name": "R", "target": "branch", "enforcement": "active"}], 200, ""), + ("/rulesets/1", _ruleset_detail("pull_request"), 200, ""), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.evidence["source"] == "ruleset" + assert "considered_rulesets" not in result.evidence + + def test_pass_via_classic_skips_rulesets(self, monkeypatch, ctx): + seq = _GhResponseSequencer([ + ("/branches/main/protection", {"required_pull_request_reviews": {"required_approving_review_count": 1}}, 200, ""), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.PASS + assert result.evidence["source"] == "classic" + assert "rulesets_status" not in result.evidence # never consulted + # And confirm we made exactly one call. + assert len(seq.calls) == 1 + + +class TestRefMatchingViaHandler: + def test_pseudo_default_branch_covers_when_equal(self, monkeypatch, ctx): + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", [{"id": 1, "name": "R", "target": "branch", "enforcement": "active"}], 200, ""), + ("/rulesets/1", _ruleset_detail("pull_request"), 200, ""), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.PASS + + def test_pseudo_default_branch_does_not_cover_non_default(self, monkeypatch, ctx): + # Audited branch is `develop`, but ruleset targets ~DEFAULT_BRANCH (main). + ctx = HandlerContext( + local_path="/tmp", owner="octo", repo="hello", + default_branch="main", control_id="TEST-01", + ) + seq = _GhResponseSequencer([ + ("/branches/develop/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", [{"id": 1, "name": "R", "target": "branch", "enforcement": "active"}], 200, ""), + ("/rulesets/1", _ruleset_detail("pull_request"), 200, ""), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request", "branch": "develop"}, ctx + ) + assert result.status == HandlerResultStatus.FAIL + assert "cover branch 'develop'" in result.evidence["considered_rulesets"][0]["reason"] + + def test_exclude_wins_over_include(self, monkeypatch, ctx): + detail = _ruleset_detail("pull_request") + detail["conditions"] = {"ref_name": {"include": ["~ALL"], "exclude": ["refs/heads/main"]}} + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", [{"id": 1, "name": "R", "target": "branch", "enforcement": "active"}], 200, ""), + ("/rulesets/1", detail, 200, ""), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.FAIL + + def test_glob_ref_treated_as_non_match(self, monkeypatch, ctx): + detail = _ruleset_detail("pull_request") + detail["conditions"] = {"ref_name": {"include": ["refs/heads/release/*"], "exclude": []}} + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", [{"id": 1, "name": "R", "target": "branch", "enforcement": "active"}], 200, ""), + ("/rulesets/1", detail, 200, ""), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.FAIL + + +# --------------------------------------------------------------------------- +# US2: FAIL when no protection + cross-surface layering + helper coverage +# --------------------------------------------------------------------------- + + +class TestFailPaths: + def test_no_classic_no_rulesets_fails(self, monkeypatch, ctx): + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", [], 200, ""), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.FAIL + assert result.evidence["source"] == "neither-surface-provided-protection" + assert result.evidence["considered_rulesets"] == [] + + def test_only_evaluate_mode_rulesets_fails(self, monkeypatch, ctx): + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", [{"id": 1, "name": "Dry run", "target": "branch", "enforcement": "evaluate"}], 200, ""), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.FAIL + # Evaluate-mode rulesets are filtered at the summary level before + # detail fetch, so they do not appear in considered_rulesets. + assert result.evidence["considered_rulesets"] == [] + + def test_rulesets_dont_cover_branch_fails(self, monkeypatch, ctx): + detail = _ruleset_detail("pull_request") + detail["conditions"] = {"ref_name": {"include": ["refs/heads/develop"], "exclude": []}} + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", [{"id": 1, "name": "Wrong branch", "target": "branch", "enforcement": "active"}], 200, ""), + ("/rulesets/1", detail, 200, ""), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.FAIL + considered = result.evidence["considered_rulesets"] + assert len(considered) == 1 + assert considered[0]["id"] == 1 + + def test_empty_rules_array_fails(self, monkeypatch, ctx): + detail = _ruleset_detail("pull_request") + detail["rules"] = [] + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", [{"id": 1, "name": "Empty", "target": "branch", "enforcement": "active"}], 200, ""), + ("/rulesets/1", detail, 200, ""), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.FAIL + assert result.evidence["considered_rulesets"][0]["reason"] == "no rules declared" + + def test_considered_rulesets_populated_on_fail(self, monkeypatch, ctx): + # Three active rulesets, all failing for distinct reasons. + def _r(rid, name, rule_type, refs=None): + d = _ruleset_detail(rule_type) + d["id"] = rid + d["name"] = name + if refs is not None: + d["conditions"] = {"ref_name": refs} + return d + + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", [ + {"id": 1, "name": "Wrong rule", "target": "branch", "enforcement": "active"}, + {"id": 2, "name": "Wrong branch", "target": "branch", "enforcement": "active"}, + {"id": 3, "name": "No rules", "target": "branch", "enforcement": "active"}, + ], 200, ""), + ("/rulesets/1", _r(1, "Wrong rule", "deletion"), 200, ""), + ("/rulesets/2", _r(2, "Wrong branch", "pull_request", + refs={"include": ["refs/heads/other"], "exclude": []}), 200, ""), + ("/rulesets/3", {"id": 3, "name": "No rules", "enforcement": "active", + "conditions": {"ref_name": {"include": ["~DEFAULT_BRANCH"], "exclude": []}}, + "rules": []}, 200, ""), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.FAIL + considered = result.evidence["considered_rulesets"] + assert [c["id"] for c in considered] == [1, 2, 3] + assert "need 'pull_request'" in considered[0]["reason"] + assert "cover branch" in considered[1]["reason"] + assert considered[2]["reason"] == "no rules declared" + assert result.evidence["considered_rulesets_truncated"] == 0 + + def test_considered_rulesets_truncation(self, monkeypatch, ctx): + summaries = [ + {"id": i, "name": f"R{i}", "target": "branch", "enforcement": "active"} + for i in range(1, 26) # 25 rulesets + ] + details = [ + (f"/rulesets/{i}", + {"id": i, "name": f"R{i}", "enforcement": "active", + "conditions": {"ref_name": {"include": ["~DEFAULT_BRANCH"], "exclude": []}}, + "rules": [{"type": "deletion"}]}, # wrong type for require_pull_request + 200, "") + for i in range(1, 26) + ] + seq = _GhResponseSequencer( + [("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", summaries, 200, "")] + + details + ) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.FAIL + assert len(result.evidence["considered_rulesets"]) == 20 + assert result.evidence["considered_rulesets_truncated"] == 5 + + def test_classic_partial_signal_falls_through_to_rulesets(self, monkeypatch, ctx): + """Q1 clarification: classic 200 without the required signal STILL consults rulesets.""" + seq = _GhResponseSequencer([ + # Classic returns 200 with allow_deletions=false (satisfies PREVENT_DELETION) + # but NO required_pull_request_reviews (does NOT satisfy REQUIRE_PULL_REQUEST). + ("/branches/main/protection", {"allow_deletions": {"enabled": False}}, 200, ""), + ("/rulesets", [{"id": 1, "name": "R", "target": "branch", "enforcement": "active"}], 200, ""), + ("/rulesets/1", _ruleset_detail("pull_request"), 200, ""), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.PASS + assert result.evidence["source"] == "ruleset" + assert result.evidence["classic_status"] == 200 + + def test_both_surfaces_confirm_uses_classic_first(self, monkeypatch, ctx): + """Classic 200 with signal short-circuits; rulesets endpoint is never called.""" + seq = _GhResponseSequencer([ + ("/branches/main/protection", {"required_pull_request_reviews": {"required_approving_review_count": 1}}, 200, ""), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.PASS + assert result.evidence["source"] == "classic" + # Exactly one API call was made. + assert len(seq.calls) == 1 + assert "/rulesets" not in seq.calls[0][0] + + +# --------------------------------------------------------------------------- +# US3: WARN (INCONCLUSIVE) on ambiguous responses +# --------------------------------------------------------------------------- + + +class TestAmbiguousResponses: + def test_classic_403_returns_inconclusive(self, monkeypatch, ctx): + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 403, "HTTP 403: Forbidden"), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.INCONCLUSIVE + assert result.evidence["source"] == "insufficient-access" + assert result.evidence["classic_status"] == 403 + assert "rulesets_status" not in result.evidence # never consulted + + def test_classic_401_returns_inconclusive(self, monkeypatch, ctx): + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 401, "HTTP 401: Unauthorized"), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.INCONCLUSIVE + assert result.evidence["source"] == "insufficient-access" + + def test_classic_5xx_returns_inconclusive(self, monkeypatch, ctx): + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 502, "HTTP 502: Bad Gateway"), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.INCONCLUSIVE + assert result.evidence["classic_status"] == 502 + + def test_rulesets_403_returns_inconclusive(self, monkeypatch, ctx): + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", None, 403, "HTTP 403: Forbidden"), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.INCONCLUSIVE + assert result.evidence["source"] == "insufficient-access" + assert result.evidence["classic_status"] == 404 + assert result.evidence["rulesets_status"] == 403 + + def test_rulesets_429_returns_inconclusive(self, monkeypatch, ctx): + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", None, 429, "HTTP 429: Too Many Requests"), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.INCONCLUSIVE + assert result.evidence["rulesets_status"] == 429 + + def test_partial_fetch_returns_inconclusive(self, monkeypatch, ctx): + # List succeeds; detail 404s (ruleset deleted between calls). + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", [{"id": 42, "name": "R", "target": "branch", "enforcement": "active"}], 200, ""), + ("/rulesets/42", None, 404, "HTTP 404: Not Found"), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.INCONCLUSIVE + assert result.evidence["source"] == "partial-fetch" + assert "ruleset 42" in result.message + + def test_gh_cli_missing_returns_inconclusive(self, monkeypatch, ctx): + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 0, + "GitHub CLI (gh) not found. Install it from https://cli.github.com/"), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.INCONCLUSIVE + assert result.evidence["source"] == "insufficient-access" + assert result.evidence["classic_status"] == 0 + assert "gh) not found" in result.message + + def test_classic_status_zero_returns_inconclusive(self, monkeypatch, ctx): + seq = _GhResponseSequencer([ + ("/branches/main/protection", None, 0, "connection reset by peer"), + ]) + _install(monkeypatch, seq) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx + ) + assert result.status == HandlerResultStatus.INCONCLUSIVE + assert result.evidence["source"] == "insufficient-access" + + +# --------------------------------------------------------------------------- +# Config-validation errors -> HandlerResult.ERROR +# --------------------------------------------------------------------------- + + +class TestConfigValidation: + def test_missing_requirement(self, ctx): + result = github_branch_protection_handler({}, ctx) + assert result.status == HandlerResultStatus.ERROR + assert "requirement" in result.message + + def test_unknown_requirement(self, ctx): + result = github_branch_protection_handler({"requirement": "bogus"}, ctx) + assert result.status == HandlerResultStatus.ERROR + assert "bogus" in result.message + + def test_minimum_out_of_range_low(self, ctx): + result = github_branch_protection_handler( + {"requirement": "require_approvals", "required_approvals_minimum": 0}, ctx + ) + assert result.status == HandlerResultStatus.ERROR + + def test_minimum_out_of_range_high(self, ctx): + result = github_branch_protection_handler( + {"requirement": "require_approvals", "required_approvals_minimum": 11}, ctx + ) + assert result.status == HandlerResultStatus.ERROR + + def test_missing_owner_repo(self): + ctx_empty = HandlerContext( + local_path="/tmp", owner="", repo="", default_branch="main", control_id="X", + ) + result = github_branch_protection_handler( + {"requirement": "require_pull_request"}, ctx_empty + ) + assert result.status == HandlerResultStatus.ERROR + assert "owner/repo" in result.message diff --git a/tests/darnit_baseline/test_branch_protection_integration.py b/tests/darnit_baseline/test_branch_protection_integration.py new file mode 100644 index 00000000..d71cab19 --- /dev/null +++ b/tests/darnit_baseline/test_branch_protection_integration.py @@ -0,0 +1,187 @@ +"""Integration tests for the github_branch_protection handler through the sieve orchestrator. + +Loads the actual openssf-baseline TOML, patches +`darnit_baseline.branch_protection.gh_api_with_status`, and runs the four +affected controls end-to-end. Locks (a) the four TOML edits at the +orchestrator level and (b) SC-004's exact API-call budget. +""" + +from __future__ import annotations + +from typing import Any + +import pytest + +from darnit.config.control_loader import load_controls_from_framework +from darnit.config.merger import load_framework_by_name +from darnit.sieve.handler_registry import reset_sieve_handler_registry +from darnit.sieve.models import CheckContext +from darnit.sieve.orchestrator import SieveOrchestrator +from darnit_baseline import branch_protection as bp + + +@pytest.fixture(autouse=True) +def _register_baseline_handlers(): + """Ensure the baseline plugin's handlers are registered for each test.""" + reset_sieve_handler_registry() + from darnit.core.discovery import get_implementation + + impl = get_implementation("openssf-baseline") + assert impl is not None, "openssf-baseline implementation not discovered" + impl.register_handlers() + yield + + +@pytest.fixture() +def control_specs(): + fw = load_framework_by_name("openssf-baseline") + specs = {s.control_id: s for s in load_controls_from_framework(fw)} + return specs + + +def _make_context(control_id: str) -> CheckContext: + return CheckContext( + owner="octo", + repo="hello", + local_path="/tmp", + default_branch="main", + control_id=control_id, + ) + + +class _Sequencer: + def __init__(self, responses): + self._queue = list(responses) + self.calls: list[tuple[str, bool]] = [] + + def __call__(self, endpoint, *, paginate=False): + self.calls.append((endpoint, paginate)) + for i, (pat, body, status, msg) in enumerate(self._queue): + if pat in endpoint or endpoint.endswith(pat): + self._queue.pop(i) + return body, status, msg + raise AssertionError(f"unexpected call to {endpoint} paginate={paginate}; remaining={[p for p,*_ in self._queue]}") + + +def _ruleset_detail(rule_type: str, review_count: int = 1) -> dict[str, Any]: + return { + "id": 1, "name": "Protect main", "enforcement": "active", + "conditions": {"ref_name": {"include": ["~DEFAULT_BRANCH"], "exclude": []}}, + "rules": [ + {"type": rule_type, "parameters": {"required_approving_review_count": review_count}} + if rule_type == "pull_request" + else {"type": rule_type} + ], + } + + +# --------------------------------------------------------------------------- +# US1 -- four TOML controls resolve PASS via ruleset (T051-T054) +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("control_id, rule_type", [ + ("OSPS-AC-03.01", "pull_request"), + ("OSPS-AC-03.02", "deletion"), + ("OSPS-QA-03.01", "required_status_checks"), + ("OSPS-QA-07.01", "pull_request"), +]) +def test_pass_via_ruleset(monkeypatch, control_specs, control_id, rule_type): + seq = _Sequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", [{"id": 1, "name": "Protect main", "target": "branch", "enforcement": "active"}], 200, ""), + ("/rulesets/1", _ruleset_detail(rule_type), 200, ""), + ]) + monkeypatch.setattr(bp, "gh_api_with_status", seq) + + spec = control_specs[control_id] + ctx = _make_context(control_id) + orch = SieveOrchestrator(stop_on_llm=False) + result = orch.verify(spec, ctx) + + assert result.status == "PASS", f"{control_id}: {result.status} -- {result.message}" + assert result.evidence["source"] == "ruleset" + + +# --------------------------------------------------------------------------- +# US2 -- FAIL when neither surface protects (T055) + SC-004 exact budget (T057a) +# --------------------------------------------------------------------------- + + +def test_fail_when_no_protection(monkeypatch, control_specs): + seq = _Sequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", [], 200, ""), + ]) + monkeypatch.setattr(bp, "gh_api_with_status", seq) + + spec = control_specs["OSPS-AC-03.01"] + ctx = _make_context("OSPS-AC-03.01") + orch = SieveOrchestrator(stop_on_llm=False) + result = orch.verify(spec, ctx) + + assert result.status == "FAIL" + assert result.evidence["source"] == "neither-surface-provided-protection" + + +def test_api_call_budget_matches_sc_004(monkeypatch, control_specs): + """Locks SC-004: 1 classic call + 1 list call + N detail calls + 0 to /repos/{owner}/{repo}.""" + summaries = [ + {"id": i, "name": f"R{i}", "target": "branch", "enforcement": "active"} + for i in range(1, 4) + ] + details = [ + (f"/rulesets/{i}", { + "id": i, "name": f"R{i}", "enforcement": "active", + "conditions": {"ref_name": {"include": ["~DEFAULT_BRANCH"], "exclude": []}}, + "rules": [{"type": "deletion"}], # wrong type for require_pull_request + }, 200, "") + for i in range(1, 4) + ] + seq = _Sequencer([ + ("/branches/main/protection", None, 404, "HTTP 404: Not Found"), + ("/rulesets", summaries, 200, ""), + *details, + ]) + monkeypatch.setattr(bp, "gh_api_with_status", seq) + + spec = control_specs["OSPS-AC-03.01"] + orch = SieveOrchestrator(stop_on_llm=False) + result = orch.verify(spec, _make_context("OSPS-AC-03.01")) + + assert result.status == "FAIL" + # Exact call profile: + # 1 classic + 1 rulesets-list + 3 rulesets-detail = 5 calls + assert len(seq.calls) == 5 + endpoints = [c[0] for c in seq.calls] + assert endpoints[0].endswith("/branches/main/protection") + assert endpoints[1].endswith("/rulesets") + assert seq.calls[1][1] is True # paginate=True on the list call + for i, expected in enumerate((1, 2, 3), start=2): + assert endpoints[i].endswith(f"/rulesets/{expected}"), endpoints[i] + assert seq.calls[i][1] is False # detail calls are not paginated + # Critically: no call to /repos/{owner}/{repo} for default-branch resolution. + assert not any( + e.endswith("/repos/octo/hello") or e == "/repos/octo/hello" + for e in endpoints + ) + + +# --------------------------------------------------------------------------- +# US3 -- WARN falls through to manual pass (T056) +# --------------------------------------------------------------------------- + + +def test_warn_when_403_falls_through_to_manual(monkeypatch, control_specs): + seq = _Sequencer([ + ("/branches/main/protection", None, 403, "HTTP 403: Forbidden"), + ]) + monkeypatch.setattr(bp, "gh_api_with_status", seq) + + spec = control_specs["OSPS-AC-03.01"] + orch = SieveOrchestrator(stop_on_llm=False) + result = orch.verify(spec, _make_context("OSPS-AC-03.01")) + + # Handler is INCONCLUSIVE -> orchestrator falls through to the trailing + # manual pass, which resolves the control as WARN with verification steps. + assert result.status == "WARN" diff --git a/tests/darnit_baseline/test_handler_dispatch_integration.py b/tests/darnit_baseline/test_handler_dispatch_integration.py index 19cabd5d..26922860 100644 --- a/tests/darnit_baseline/test_handler_dispatch_integration.py +++ b/tests/darnit_baseline/test_handler_dispatch_integration.py @@ -146,6 +146,11 @@ def test_all_controls_have_handler_invocations(self): impl = get_implementation("openssf-baseline") assert impl is not None, "openssf-baseline implementation not found" + # Register baseline-owned sieve handlers (github_branch_protection, + # generate_threat_model) so the per-control handler-name check + # below finds them alongside core-registered handlers. + impl.register_handlers() + toml_path = impl.get_framework_config_path() assert toml_path is not None and Path(toml_path).exists(), ( f"Framework TOML not found at {toml_path}"