diff --git a/.github/workflows/scorecard-analysis.yml b/.github/workflows/scorecard-analysis.yml index 8e793c8d70..15587bcc6f 100644 --- a/.github/workflows/scorecard-analysis.yml +++ b/.github/workflows/scorecard-analysis.yml @@ -1,16 +1,25 @@ name: Scorecard analysis on: + # Keep the canonical owner's own default branch covered. push: branches: ["main"] schedule: - cron: "30 1 * * 6" + # Product repositories retain only their repository-specific push/schedule + # trigger and delegate every implementation step to this versioned owner. + workflow_call: # Queue two default-branch pushes into one run rather than letting them stack # unbounded; cancel-in-progress stays false (same tradeoff as strix.yml) so a # security-scan run for an older main commit is never discarded mid-flight -- # it still finishes and uploads that commit's SARIF evidence, it is just no # longer allowed to run alongside a newer queued push for the same branch. +# (This deliberately does NOT scope by exact SHA: a ref-scoped group with +# cancel-in-progress: false is what bounds runaway concurrent Scorecard scans +# across a burst of pushes -- SHA-scoping would give every distinct commit its +# own group, restoring unlimited-parallel-scans, the exact resource-consumption +# problem this group exists to prevent. See #1768.) concurrency: group: scorecard-analysis-${{ github.ref }} cancel-in-progress: false diff --git a/CHANGELOG.d/20260903-reusable-default-branch-scorecard.md b/CHANGELOG.d/20260903-reusable-default-branch-scorecard.md new file mode 100644 index 0000000000..383490d779 --- /dev/null +++ b/CHANGELOG.d/20260903-reusable-default-branch-scorecard.md @@ -0,0 +1,14 @@ +## Reusable default-branch Scorecard owner + +- Centralize OSSF Scorecard execution, SARIF filtering, and code-scanning upload in + `.github/workflows/scorecard-analysis.yml` while preserving the canonical owner's + default-branch push and weekly schedule and exposing a `workflow_call` contract. +- Keep the ref-scoped, `cancel-in-progress: false` concurrency group `.github#1768` + already established (queue rather than cancel a burst of same-ref pushes, so an + in-flight scan's SARIF evidence for its own commit is never discarded). +- Keep consumer rollout incomplete until each repository replaces copied logic with + a thin caller pinned to the central merge commit SHA, declares the required caller + token permissions, preserves its actual default-branch and schedule triggers, + repairs documentation, and proves caller-context SARIF behavior with a governed + canary. `wardnet#160` and `semantic-data-portal#93` remain open repair branches + until that successor evidence exists. diff --git a/docs/doctoring/reusable-default-branch-scorecard-owner-20260903.md b/docs/doctoring/reusable-default-branch-scorecard-owner-20260903.md new file mode 100644 index 0000000000..20eaf298c1 --- /dev/null +++ b/docs/doctoring/reusable-default-branch-scorecard-owner-20260903.md @@ -0,0 +1,131 @@ +# Reusable default-branch Scorecard owner — 2026-09-03 + +## Incident and buyer-visible risk + +Repository-local `scorecard-analysis.yml` files in `ContextualWisdomLab/wardnet` and +`ContextualWisdomLab/semantic-data-portal` repeat the same OSSF Scorecard, SARIF filtering, and upload +implementation. Open deletion PRs `wardnet#160` and `semantic-data-portal#93` assumed the organization-required +`scorecard-pr.yml` fully replaced them. That assumption is false: the required workflow supplies pull-request +evidence, while the local workflows supply default-branch push and weekly scheduled evidence. Deleting them +without a successor would stop branch-history and scheduled SARIF refresh. + +The customer consequence is stale supply-chain posture after a merge: a pull request could be scanned before +landing, while the authoritative default branch and its later dependency/configuration drift receive no +corresponding Scorecard result. + +## Owner decision + +`ContextualWisdomLab/.github/.github/workflows/scorecard-analysis.yml` is the canonical implementation owner for +default-branch Scorecard analysis. It preserves its own `push` and `schedule` triggers and adds `workflow_call` +for product repositories. Consumers retain only the trigger and permission boundary that GitHub cannot express +centrally across independent repositories. + +The called workflow uses the caller's `github` context and `actions/checkout` therefore checks out the caller +repository. The caller's `GITHUB_TOKEN` permissions cannot be elevated by the called workflow, so each caller +must explicitly grant the required permissions. Consumers must pin the reusable workflow to the full immutable +**central merge commit SHA**, never `main`, another mutable branch, or an open PR head. + +## Canonical thin caller after this owner PR lands + +Replace `` and `` only after the central PR is merged: + +```yaml +name: Scorecard analysis + +on: + push: + branches: [""] + schedule: + - cron: "30 1 * * 6" + +permissions: read-all + +jobs: + scorecard_analysis: + permissions: + security-events: write + id-token: write + contents: read + issues: read + pull-requests: read + checks: read + uses: ContextualWisdomLab/.github/.github/workflows/scorecard-analysis.yml@ +``` + +Do not add `runs-on`, `steps`, copied Scorecard logic, inherited secrets, or a second concurrency group to the +caller job. The called owner already coalesces same-ref invocations; a caller-side group with an overlapping +identity could cancel its own called workflow. + +## Concurrency decision + +This PR's own earlier draft reasoned that GitHub concurrency admission follows event arrival order, not commit +ancestry, and scoped the group by `${{ github.repository }}`, `${{ github.ref }}`, and `${{ github.sha }}` with +`cancel-in-progress: true` so only duplicate invocations of the same immutable revision could cancel one another. +That reasoning is sound in isolation, but `.github#1768` (merged to `main` before this PR's own branch caught +up) had independently added a *different*, already-reviewed concurrency group to this same file: scoped by +`${{ github.ref }}` only, `cancel-in-progress: false`, so an in-flight scan for an older commit always finishes +and uploads that commit's SARIF evidence rather than being cancelled, and a burst of pushes queues (GitHub's +default single-pending-successor behavior) instead of running unboundedly in parallel. + +**Merging this branch as-is produced two `concurrency:` keys in one YAML mapping -- a real bug, not a stylistic +duplication: YAML resolves a repeated mapping key to its last occurrence, so the SHA-scoped block was silently +discarded at parse time regardless of author intent.** The two designs are also structurally incompatible as a +single `concurrency:` block, not just redundant: SHA-scoping gives every distinct commit its own group, which +means NOTHING ever queues behind anything else -- restoring the unbounded-concurrent-scans problem `#1768` +exists to prevent. Given this organization's standing priority of reducing GitHub Actions queue congestion +(a plan-level 60-job ceiling shared across the whole org), `#1768`'s ref-scoped, cancel-false group was kept as +authoritative and this PR's SHA-scoped block was removed. The narrower concern the SHA-scoped design addressed +(a delayed duplicate event for the exact same commit) remains a real, if much rarer, residual risk -- not +closed here. + +This also differs deliberately from `Current Head Run Coalescer`: that workflow performs queue-cleanup mutation, +so its active worker must finish and only the latest pending trigger is retained. + +## TDD and rollout evidence + +- RED `76617d0a1f4bd0126d0e610362328ace2dd02612`: contract requires `workflow_call`, preserved push/schedule, + reusable ownership, immutable action pins, credential hygiene, and SARIF upload behavior while the owner + workflow still lacks the reusable contract. +- GREEN `aaf0fa5241348648e43618f949f44b82028abaa2`: owner workflow implements the initial reusable contract. +- Review RED `ef88c78aa64b6922f50d4a6a3e34f1900d04694f`: parsed-YAML contracts require the exact-SHA concurrency + boundary while production still groups only by repository/ref. The same commit replaces comment-sensitive + substring checks with structural YAML assertions. +- Review GREEN `7f99d560e8eaa9ab2cec46600b3321e9b0700669`: production adds the exact source SHA to the group and records + the owner boundary for any future cross-revision cleanup. +- Focused reconstructed exact-content test before review: `3 passed`. +- **Post-review correction, before merge:** `.github#1768` landed its own, incompatible concurrency group for + this same file while this PR's branch was still in flight (see "Concurrency decision" above). The exact-SHA + group GREEN commit above is accurate as a record of this PR's own development, but is NOT the state that + merged -- the final concurrency block keeps `#1768`'s ref-scoped, `cancel-in-progress: false` group instead. +- Rollout remains incomplete until the central PR merges and each consumer pins the resulting merge SHA. + +## Consumer acceptance criteria + +For each consumer repository: + +1. Re-fetch the default branch and deletion-PR exact head. +2. Replace local implementation with the thin caller pinned to the central merge SHA. +3. Preserve the repository's actual default branch and weekly schedule. +4. Update repository documentation that names the local implementation. +5. Prove a default-branch push or governed canary invokes the central workflow in the caller context, checks out + the consumer commit, produces Scorecard output, and attempts SARIF upload under the declared permissions. +6. Confirm the central PR-required Scorecard and default-branch caller do not both trigger for the same event. +7. Confirm a delayed older-revision event cannot cancel a newer-revision scan. +8. Merge through ordinary protection unless the exact central queue-control chicken-and-egg condition applies. + +`wardnet#160` and `semantic-data-portal#93` remain open repair branches until these criteria are satisfied; they +must not be closed merely to reduce the PR count. + +## References + +GitHub. (2026). *Reusing workflow configurations*. GitHub Docs. +https://docs.github.com/actions/reference/workflows-and-actions/reusing-workflow-configurations + +GitHub. (2026). *Reuse workflows*. GitHub Docs. +https://docs.github.com/actions/how-tos/reuse-automations/reuse-workflows + +GitHub. (2026). *Control the concurrency of workflows and jobs*. GitHub Docs. +https://docs.github.com/actions/how-tos/write-workflows/choose-when-workflows-run/control-workflow-concurrency + +Open Source Security Foundation. (2026). *OSSF Scorecard action*. GitHub. +https://github.com/ossf/scorecard-action diff --git a/tests/test_reusable_default_branch_scorecard_contract.py b/tests/test_reusable_default_branch_scorecard_contract.py new file mode 100644 index 0000000000..2f5dd90c1b --- /dev/null +++ b/tests/test_reusable_default_branch_scorecard_contract.py @@ -0,0 +1,360 @@ +"""Contract tests for the reusable default-branch Scorecard workflow.""" + +from __future__ import annotations + +import ast +from collections import defaultdict +from pathlib import Path +from typing import TypeAlias + + +ContractScalar: TypeAlias = str | list[str] | None +ContractMapping: TypeAlias = dict[tuple[str, ...], ContractScalar] +BLOCK_SCALAR_MARKERS = frozenset({"|", "|-", ">", ">-"}) + +REPOSITORY_ROOT = Path(__file__).resolve().parents[1] +WORKFLOW_PATH = REPOSITORY_ROOT / ".github" / "workflows" / "scorecard-analysis.yml" + + +def _strip_inline_comment(line_text: str) -> str: + """Remove an unquoted YAML comment without truncating quoted hash characters.""" + single_quoted = False + double_quoted = False + escape_next = False + + for character_index, current_character in enumerate(line_text): + if escape_next: + escape_next = False + continue + if current_character == "\\" and double_quoted: + escape_next = True + continue + if current_character == "'" and not double_quoted: + single_quoted = not single_quoted + continue + if current_character == '"' and not single_quoted: + double_quoted = not double_quoted + continue + if ( + current_character == "#" + and not single_quoted + and not double_quoted + and ( + character_index == 0 + or line_text[character_index - 1].isspace() + ) + ): + return line_text[:character_index].rstrip() + + if single_quoted or double_quoted: + raise AssertionError(f"unterminated YAML quote: {line_text!r}") + return line_text.rstrip() + + +def _split_mapping_entry(entry_text: str) -> tuple[str, str]: + """Split one supported YAML mapping entry outside quotes and containers.""" + single_quoted = False + double_quoted = False + escape_next = False + brace_depth = 0 + bracket_depth = 0 + + for character_index, current_character in enumerate(entry_text): + if escape_next: + escape_next = False + continue + if current_character == "\\" and double_quoted: + escape_next = True + continue + if current_character == "'" and not double_quoted: + single_quoted = not single_quoted + continue + if current_character == '"' and not single_quoted: + double_quoted = not double_quoted + continue + if single_quoted or double_quoted: + continue + if current_character == "{": + brace_depth += 1 + elif current_character == "}": + if brace_depth <= 0: + raise AssertionError(f"unmatched YAML closing brace: {entry_text!r}") + brace_depth -= 1 + elif current_character == "[": + bracket_depth += 1 + elif current_character == "]": + if bracket_depth <= 0: + raise AssertionError( + f"unmatched YAML closing bracket: {entry_text!r}" + ) + bracket_depth -= 1 + elif ( + current_character == ":" + and brace_depth == 0 + and bracket_depth == 0 + ): + mapping_key = entry_text[:character_index].strip() + scalar_text = entry_text[character_index + 1 :].strip() + if not mapping_key: + raise AssertionError(f"empty YAML mapping key: {entry_text!r}") + return mapping_key, scalar_text + + if single_quoted or double_quoted or brace_depth or bracket_depth: + raise AssertionError(f"unterminated YAML mapping entry: {entry_text!r}") + raise AssertionError(f"unsupported YAML mapping entry: {entry_text!r}") + + +def _parse_scalar_value(scalar_text: str) -> ContractScalar: + """Parse only the scalar forms used by the governed workflow contract.""" + if not scalar_text: + return None + if scalar_text in BLOCK_SCALAR_MARKERS: + return scalar_text + if scalar_text[0] in {'"', "'", "["}: + parsed_value = ast.literal_eval(scalar_text) + if isinstance(parsed_value, list): + assert all( + isinstance(list_item, str) for list_item in parsed_value + ), "workflow contract accepts only inline string lists" + return parsed_value + assert isinstance(parsed_value, str), ( + "workflow contract accepts only string scalar literals" + ) + return parsed_value + return scalar_text + + +def _parse_workflow_contract(yaml_text: str) -> ContractMapping: + """Project supported YAML mappings into indentation-aware contract paths.""" + contract_mapping: ContractMapping = {} + path_stack: list[tuple[int, str]] = [] + sequence_counts: dict[tuple[str, ...], int] = defaultdict(int) + block_scalar_indent: int | None = None + + for raw_line in yaml_text.splitlines(): + if not raw_line.strip(): + continue + + leading_whitespace = raw_line[ + : len(raw_line) - len(raw_line.lstrip()) + ] + if "\t" in leading_whitespace: + raise AssertionError("tabs are not valid workflow indentation") + indent_width = len(leading_whitespace) + + if block_scalar_indent is not None: + if indent_width > block_scalar_indent: + continue + block_scalar_indent = None + + content_text = _strip_inline_comment(raw_line[indent_width:]) + if not content_text: + continue + + while path_stack and path_stack[-1][0] >= indent_width: + path_stack.pop() + parent_path = tuple( + path_component for _, path_component in path_stack + ) + + if content_text.startswith("- "): + item_index = sequence_counts[parent_path] + sequence_counts[parent_path] += 1 + item_component = f"[{item_index}]" + path_stack.append((indent_width, item_component)) + item_text = content_text[2:].strip() + if not item_text: + contract_mapping[parent_path + (item_component,)] = None + continue + + mapping_key, scalar_text = _split_mapping_entry(item_text) + item_path = parent_path + (item_component, mapping_key) + scalar_value = _parse_scalar_value(scalar_text) + contract_mapping[item_path] = scalar_value + if scalar_value is None: + path_stack.append((indent_width + 1, mapping_key)) + elif ( + isinstance(scalar_value, str) + and scalar_value in BLOCK_SCALAR_MARKERS + ): + block_scalar_indent = indent_width + continue + + mapping_key, scalar_text = _split_mapping_entry(content_text) + mapping_path = parent_path + (mapping_key,) + scalar_value = _parse_scalar_value(scalar_text) + contract_mapping[mapping_path] = scalar_value + if scalar_value is None: + path_stack.append((indent_width, mapping_key)) + elif ( + isinstance(scalar_value, str) + and scalar_value in BLOCK_SCALAR_MARKERS + ): + block_scalar_indent = indent_width + + return contract_mapping + + +def _load_workflow_contract() -> ContractMapping: + """Load the Scorecard workflow without undeclared test dependencies.""" + return _parse_workflow_contract(WORKFLOW_PATH.read_text(encoding="utf-8")) + + +def _mapping_contract( + workflow_contract: ContractMapping, + mapping_prefix: tuple[str, ...], +) -> dict[str, ContractScalar]: + """Return direct child values for one parsed mapping path.""" + return { + mapping_path[-1]: scalar_value + for mapping_path, scalar_value in workflow_contract.items() + if len(mapping_path) == len(mapping_prefix) + 1 + and mapping_path[: len(mapping_prefix)] == mapping_prefix + } + + +def _step_path_by_name( + workflow_contract: ContractMapping, + step_name: str, +) -> tuple[str, ...]: + """Return the sequence-item path for one named analysis step.""" + steps_prefix = ("jobs", "analysis", "steps") + for mapping_path, scalar_value in workflow_contract.items(): + if ( + len(mapping_path) == len(steps_prefix) + 2 + and mapping_path[: len(steps_prefix)] == steps_prefix + and mapping_path[-1] == "name" + and scalar_value == step_name + ): + return mapping_path[:-1] + raise AssertionError(f"missing Scorecard workflow step: {step_name}") + + +def test_contract_parser_ignores_comments_and_block_scalar_decoys() -> None: + """Comments and script literals must not satisfy workflow contracts.""" + fixture_text = """\ +# workflow_call: +name: "Parser # fixture" +on: + push: + branches: ["develop"] +jobs: + analysis: + steps: + - name: Script decoy + run: | + workflow_call: + uses: attacker/example@mutable + permissions: + security-events: write + - name: Checkout code + uses: actions/checkout@immutable # pinned release annotation + with: + persist-credentials: false +""" + fixture_contract = _parse_workflow_contract(fixture_text) + + assert fixture_contract[("name",)] == "Parser # fixture" + assert fixture_contract[("on", "push", "branches")] == ["develop"] + assert ("on", "workflow_call") not in fixture_contract + assert ( + "jobs", + "analysis", + "steps", + "[0]", + "uses", + ) not in fixture_contract + checkout_path = _step_path_by_name(fixture_contract, "Checkout code") + assert fixture_contract[checkout_path + ("uses",)] == ( + "actions/checkout@immutable" + ) + assert fixture_contract[ + checkout_path + ("with", "persist-credentials") + ] == "false" + + +def test_scorecard_analysis_is_reusable_without_losing_branch_history_triggers() -> None: + """Preserve push and scheduled SARIF refresh while enabling reuse.""" + workflow_contract = _load_workflow_contract() + + assert workflow_contract[("on", "workflow_call")] is None + assert workflow_contract[("on", "push", "branches")] == ["main"] + assert workflow_contract[("on", "schedule", "[0]", "cron")] == ( + "30 1 * * 6" + ) + + +def test_scorecard_analysis_never_discards_an_in_flight_scans_evidence() -> None: + """A newer queued push must never cancel an older scan mid-flight. + + .github#1768 (merged before this PR's own concurrency work landed) already + added a ref-scoped, cancel-in-progress: false group to this file for + exactly this reason: an in-flight Scorecard run's SARIF evidence for its + own commit must never be discarded, only serialized behind. This PR's own + earlier draft added a second, SHA-scoped, cancel-in-progress: true group to + the same file -- a real, independently-reasoned fix for a different + concern (the #1568-class stale-cancels-fresh race), but mutually exclusive + with #1768's group as a single `concurrency:` block: SHA-scoping gives + every distinct commit its own group, which would restore unbounded + concurrent scans across a push burst -- the exact problem #1768 closed, + and a direct regression of this org's standing Actions-queue-congestion + priority. Kept #1768's group as authoritative. + """ + workflow_contract = _load_workflow_contract() + + assert _mapping_contract(workflow_contract, ("concurrency",)) == { + "group": "scorecard-analysis-${{ github.ref }}", + "cancel-in-progress": "false", + } + + +def test_scorecard_analysis_keeps_authoritative_sarif_boundaries() -> None: + """Retain pinned analysis, credential hygiene, and SARIF upload.""" + workflow_contract = _load_workflow_contract() + + assert workflow_contract[("permissions",)] == "read-all" + assert _mapping_contract( + workflow_contract, + ("jobs", "analysis", "permissions"), + ) == { + "security-events": "write", + "id-token": "write", + "contents": "read", + "issues": "read", + "pull-requests": "read", + "checks": "read", + } + + checkout_path = _step_path_by_name(workflow_contract, "Checkout code") + assert workflow_contract[checkout_path + ("uses",)] == ( + "actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0" + ) + assert workflow_contract[ + checkout_path + ("with", "persist-credentials") + ] == "false" + + analysis_path = _step_path_by_name(workflow_contract, "Run analysis") + assert workflow_contract[analysis_path + ("uses",)] == ( + "ossf/scorecard-action@4eaacf0543bb3f2c246792bd56e8cdeffafb205a" + ) + assert _mapping_contract( + workflow_contract, + analysis_path + ("with",), + ) == { + "results_file": "results.sarif", + "results_format": "sarif", + "publish_results": "false", + } + + upload_path = _step_path_by_name( + workflow_contract, + "Upload to code scanning", + ) + assert workflow_contract[upload_path + ("continue-on-error",)] == "true" + assert workflow_contract[upload_path + ("uses",)] == ( + "github/codeql-action/upload-sarif@db488ddef3bf6cb639b32c2e9a7c0a7ea8271d28" + ) + assert _mapping_contract( + workflow_contract, + upload_path + ("with",), + ) == {"sarif_file": "results.sarif"}