diff --git a/.github/workflows/actions-poll-analyzer-coverage.yml b/.github/workflows/actions-poll-analyzer-coverage.yml new file mode 100644 index 00000000..bb42b772 --- /dev/null +++ b/.github/workflows/actions-poll-analyzer-coverage.yml @@ -0,0 +1,95 @@ +name: Actions poll analyzer coverage + +on: + pull_request: + +permissions: + contents: read + +concurrency: + group: actions-poll-analyzer-coverage-${{ github.repository }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + exact-head-coverage: + runs-on: ubuntu-24.04 + timeout-minutes: 20 + env: + FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true + steps: + - name: Fail closed without a pull request number + env: + PR_NUMBER: ${{ github.event.pull_request.number }} + run: | + set -euo pipefail + if [ -z "${PR_NUMBER}" ]; then + echo "Indirect invocation without a pull request number is not allowed." + exit 1 + fi + + - name: Checkout exact pull request head + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ github.event.pull_request.head.sha }} + persist-credentials: false + + - name: Set up Python + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: '3.13' + + - name: Install hash-locked test dependencies + run: python -m pip install --disable-pip-version-check --no-cache-dir --require-hashes -r requirements-test.txt + + - name: Checkout verified Coverage.py source + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + repository: coveragepy/coveragepy + ref: 4c0e7ff425ecbb33e2b994b41118a71eb4e39021 # 7.15.4 + path: .tools/coveragepy + persist-credentials: false + + - name: Verify 100% analyzer statements and branches + env: + PYTHONPATH: ${{ github.workspace }}/.tools/coveragepy:${{ github.workspace }} + run: | + set -euo pipefail + python -m coverage erase + python -m coverage run --branch \ + --source=appguardrail_core.actions_poll_analyzer \ + -m pytest -q \ + tests/test_actions_poll_structural_analyzer.py \ + tests/test_github_actions_poll_bounds.py \ + tests/test_github_actions_poll_bound_control_flow_regression.py \ + tests/test_github_actions_poll_bound_aliases.py \ + tests/test_github_actions_poll_bound_bare_exit.py \ + tests/test_github_actions_poll_bound_command_whitespace.py \ + tests/test_github_actions_poll_bound_current_head_regressions.py \ + tests/test_github_actions_poll_bound_late_initialization.py \ + tests/test_github_actions_poll_bound_mixed_safety.py \ + tests/test_github_actions_poll_bound_review_20260902.py \ + tests/test_github_actions_poll_bound_review_precision.py \ + tests/test_github_actions_poll_bound_state_reset.py \ + tests/test_github_actions_poll_bound_unreachable_exit.py \ + tests/test_github_actions_poll_bounds_large_job.py \ + tests/test_github_actions_poll_control_flow_review.py \ + tests/test_github_actions_poll_deadline_tightening.py + python -m coverage report \ + --include=appguardrail_core/actions_poll_analyzer.py \ + --precision=2 \ + --show-missing \ + --fail-under=100 + + - name: Verify exact unrounded statement coverage + run: | + python -m scripts.ci.verify_module_coverage \ + --module appguardrail_core/actions_poll_analyzer.py \ + --test tests/test_actions_poll_structural_analyzer.py + + - name: Compile production and test modules + run: >- + python -m compileall -q + appguardrail_core/actions_poll_analyzer.py + tests/test_actions_poll_structural_analyzer.py + tests/test_github_actions_poll_bounds.py + tests/test_github_actions_poll_bound_control_flow_regression.py diff --git a/CHANGELOG.d/1087-actions-poll-structural-analyzer.md b/CHANGELOG.d/1087-actions-poll-structural-analyzer.md new file mode 100644 index 00000000..06f96ecb --- /dev/null +++ b/CHANGELOG.d/1087-actions-poll-structural-analyzer.md @@ -0,0 +1,3 @@ +# Security + +- Add a bounded structural GitHub Actions + shell analyzer that classifies transport-only polling loops by causal control flow (loop-local initialization, forward `-gt`/`-ge` total bounds, reachable fail-closed exits, and statically positive owning-job timeouts) while preserving the packaged `github-actions-transport-only-poll-bound` and `github-actions-transport-failure-budget-poll-bound` identities as migration oracles. Helper loops, sibling-job timeouts, reversed comparisons, unreachable exits, and quoted or comment text cannot donate safety. See issue #1087 and ADR-0009 (Proposed). diff --git a/appguardrail_core/actions_poll_analyzer.py b/appguardrail_core/actions_poll_analyzer.py new file mode 100644 index 00000000..5aaaff34 --- /dev/null +++ b/appguardrail_core/actions_poll_analyzer.py @@ -0,0 +1,563 @@ +"""Bounded structural analyzer for GitHub Actions polling loops. + +The packaged regex family for issue #1087 remains the migration oracle. +This module classifies literal-shell ``while`` polls by causal control flow: +initialization must precede the candidate loop, total bounds must converge +on that loop, and helper or sibling jobs cannot donate safety. +""" + +from __future__ import annotations + +from dataclasses import dataclass +import re + + +TRANSPORT_ONLY_POLL_BOUND = "github-actions-transport-only-poll-bound" +TRANSPORT_FAILURE_BUDGET_POLL_BOUND = ( + "github-actions-transport-failure-budget-poll-bound" +) + +_JOB_KEY = re.compile(r"^ ([A-Za-z0-9_][A-Za-z0-9_.-]*)\s*:\s*(?:#.*)?$") +_TIMEOUT_KEY = re.compile(r"^ timeout-minutes\s*:\s*(.*?)\s*$") +_RUN_KEY = re.compile(r"^(\s*)(?:-\s+)?run\s*:\s*\|([+-])?\s*(?:#.*)?$") +_POSITIVE_INT = re.compile(r"^[1-9][0-9]*$") +_POSITIVE_EXPR = re.compile(r"^\$\{\{\s*([1-9][0-9]*)\s*\}\}$") +_ASSIGN = re.compile(r"^([A-Za-z_][A-Za-z0-9_]*)=(.*)$") +_INCREMENT = re.compile( + r"^([A-Za-z_][A-Za-z0-9_]*)=\s*\$\(\(\s*\$?\1\s*\+\s*1\s*\)\)$" +) +_TEST = re.compile( + r"\[\s*(.+?)\s+-(gt|ge|lt|le)\s+(.+?)\s*\]" +) +_GH_API = re.compile(r"(? tuple[PollLoopAssessment, ...]: + """Classify polling loops in conventional GitHub Actions workflow YAML. + + Args: + workflow_text: Complete workflow document text. + + Returns: + One assessment per infinite ``while`` loop that executes ``gh api``. + """ + assessments: list[PollLoopAssessment] = [] + for job in _parse_jobs(workflow_text.replace("\r\n", "\n").replace("\r", "\n")): + owning_timeout = _is_positive_timeout(job.timeout_minutes) + for shell in job.shells: + assessments.extend(_classify_shell(job.name, shell, owning_timeout)) + return tuple(assessments) + + +def poll_bound_rule_ids(assessments: tuple[PollLoopAssessment, ...]) -> tuple[str, ...]: + """Map unbounded assessments onto the existing packaged detector identities. + + Args: + assessments: Classifier output from :func:`classify_poll_loops`. + + Returns: + Existing rule IDs in assessment order. Historical transport names keep + ``github-actions-transport-only-poll-bound``; renamed budgets keep + ``github-actions-transport-failure-budget-poll-bound``. + """ + rule_ids: list[str] = [] + for item in assessments: + if not item.is_transport_only_unbounded: + continue + if item.historical_transport_names: + rule_ids.append(TRANSPORT_ONLY_POLL_BOUND) + continue + rule_ids.append(TRANSPORT_FAILURE_BUDGET_POLL_BOUND) + return tuple(rule_ids) + + +def _is_positive_timeout(value: str) -> bool: + """Return whether a timeout-minutes value is a static positive bound.""" + stripped = value.split("#", 1)[0].strip() + if _POSITIVE_INT.fullmatch(stripped): + return True + matched = _POSITIVE_EXPR.fullmatch(stripped) + return matched is not None + + +def _parse_jobs(workflow_text: str) -> tuple[_Job, ...]: + """Extract conventional two-space jobs, timeouts, and literal ``run`` blocks.""" + lines = workflow_text.splitlines() + start = _jobs_section_index(lines) + if start is None: + return () + jobs: list[_Job] = [] + current_name = "" + timeout = "" + shells: list[str] = [] + run_indent = -1 + run_lines: list[str] = [] + content_indent: int | None = None + for line in lines[start + 1 :]: + if run_indent >= 0: + if not line.strip(): + run_lines.append("") + continue + indent = len(line) - len(line.lstrip(" ")) + if indent > run_indent: + if content_indent is None: + content_indent = indent + run_lines.append(line[min(content_indent, len(line)):]) + continue + shells.append("\n".join(run_lines)) + run_indent = -1 + run_lines = [] + content_indent = None + job_match = _JOB_KEY.match(line) + if job_match: + if current_name: + jobs.append(_Job(current_name, timeout, tuple(shells))) + current_name = job_match.group(1) + timeout = "" + shells = [] + continue + if not current_name: + continue + timeout_match = _TIMEOUT_KEY.match(line) + if timeout_match: + timeout = timeout_match.group(1) + continue + run_match = _RUN_KEY.match(line) + if run_match: + run_indent = len(run_match.group(1)) + run_lines = [] + content_indent = None + if run_indent >= 0: + shells.append("\n".join(run_lines)) + if current_name: + jobs.append(_Job(current_name, timeout, tuple(shells))) + return tuple(jobs) + + +def _jobs_section_index(lines: list[str]) -> int | None: + """Return the index of a column-zero ``jobs:`` mapping key.""" + for index, line in enumerate(lines): + stripped = line.split("#", 1)[0].rstrip() + if stripped == "jobs:": + return index + return None + + +def _classify_shell(job_name: str, shell: str, owning_timeout: bool) -> tuple[PollLoopAssessment, ...]: + """Classify infinite ``gh api`` polls inside one literal shell block.""" + statements = _shell_statements(shell) + loops = _loop_spans(statements) + assessments: list[PollLoopAssessment] = [] + occupied = _loop_body_indexes(loops) + for loop in loops: + body = statements[loop.start + 1 : loop.end] + commands = tuple(item.command for item in body) + if loop.condition not in {":", "true"}: + continue + if not any(_GH_API.search(command) for command in commands): + continue + assignments = _prelude_assignments(statements, loop.start, occupied) + frames, top_commands = _walk_if_frames(commands) + transport = _has_transport_failure_budget(frames, top_commands) + converges, exit_reachable = _total_bound_flags(assignments, frames, top_commands) + loop_local = converges and exit_reachable + historical = _uses_historical_names(assignments, commands) + assessments.append( + PollLoopAssessment( + job_name=job_name, + transport_failure_budget=transport, + loop_local_total_bound=loop_local, + owning_job_timeout=owning_timeout, + comparison_converges=converges, + exit_reachable=exit_reachable, + is_transport_only_unbounded=transport and not loop_local and not owning_timeout, + historical_transport_names=historical, + ) + ) + return tuple(assessments) + + +def _uses_historical_names(assignments: dict[str, str], commands: tuple[str, ...]) -> bool: + """Return whether the historical transport-budget identifier is present.""" + if "max_poll_transport_failures" in assignments: + return True + return any("max_poll_transport_failures" in command for command in commands) + + +def _prelude_assignments(statements: tuple[_Statement, ...], loop_start: int, occupied: frozenset[int]) -> dict[str, str]: + """Collect depth-zero assignments that precede the candidate loop.""" + values: dict[str, str] = {} + for statement in statements[:loop_start]: + if statement.index in occupied: + continue + parsed = _assignment(statement.command) + if parsed is None: + continue + values[parsed[0]] = parsed[1] + return values + + +def _loop_body_indexes(loops: tuple[_LoopSpan, ...]) -> frozenset[int]: + """Return statement indexes that belong to any loop body, excluding headers.""" + indexes: set[int] = set() + for loop in loops: + indexes.update(range(loop.start + 1, loop.end)) + return frozenset(indexes) + + +def _loop_spans(statements: tuple[_Statement, ...]) -> tuple[_LoopSpan, ...]: + """Match ``while`` headers to their corresponding ``done`` terminators.""" + stack: list[tuple[int, str]] = [] + spans: list[_LoopSpan] = [] + for statement in statements: + command = statement.command + while_match = _WHILE.match(command) + if while_match: + token = while_match.group(1).strip().split(None, 1) + condition = token[0] if token else "" + stack.append((statement.index, condition.rstrip(";"))) + continue + if command == "done" or command.startswith("done "): + if not stack: + continue + start, condition = stack.pop() + spans.append(_LoopSpan(start, statement.index, condition)) + return tuple(spans) + + +def _shell_statements(shell: str) -> tuple[_Statement, ...]: + """Split a shell block into executable statements, skipping heredoc bodies.""" + statements: list[_Statement] = [] + heredoc_end: str | None = None + index = 0 + for raw_line in shell.splitlines(): + if heredoc_end is not None: + if raw_line.strip() == heredoc_end: + heredoc_end = None + continue + marker = _heredoc_marker(raw_line) + executable = _mask_inert(raw_line) + for part in executable.split(";"): + command = part.strip() + if command.startswith("do "): + command = command[3:].strip() + if command == "do": + continue + if command.startswith("then "): + statements.append(_Statement(index, "then")) + index += 1 + command = command[5:].strip() + if command: + statements.append(_Statement(index, command)) + index += 1 + if marker is not None: + heredoc_end = marker + return tuple(statements) + + +def _heredoc_marker(line: str) -> str | None: + """Return a heredoc terminator token when the line starts a heredoc.""" + matched = _HEREDOC.search(line) + if matched is None: + return None + return matched.group(1) or matched.group(2) or matched.group(3) + + +def _mask_inert(line: str) -> str: + """Replace comments and quoted data with spaces, keeping substitutions.""" + chars: list[str] = [] + index = 0 + length = len(line) + while index < length: + current = line[index] + if current == "\\" and index + 1 < length: + chars.extend(" ") + index += 2 + continue + if current == "'": + index += 1 + while index < length and line[index] != "'": + chars.append(" ") + index += 1 + if index < length: + chars.append(" ") + index += 1 + continue + if current == '"': + index += 1 + while index < length and line[index] != '"': + if line[index] == "\\" and index + 1 < length: + chars.extend(" ") + index += 2 + continue + kept, consumed = _keep_expansion(line, index) + if consumed: + chars.append(kept) + index += consumed + continue + chars.append(" ") + index += 1 + if index < length: + chars.append(" ") + index += 1 + continue + if current == "#" and (not chars or chars[-1] in " \t"): + break + kept, consumed = _keep_expansion(line, index) + if consumed: + chars.append(kept) + index += consumed + continue + chars.append(current) + index += 1 + return "".join(chars) + + +def _keep_expansion(text: str, index: int) -> tuple[str, int]: + """Keep ``$var``, ``${var}``, or ``$(...)`` starting at ``index``.""" + if index >= len(text) or text[index] != "$": + return "", 0 + if index + 1 < len(text) and text[index + 1] == "(": + if index + 2 < len(text) and text[index + 2] == "(": + return _balanced_dollar(text, index) + inner, consumed = _dollar_paren(text, index) + return " $(" + _mask_inert(inner) + ") ", consumed + if index + 1 < len(text) and text[index + 1] == "{": + end = text.find("}", index + 2) + if end < 0: + return text[index:], len(text) - index + return text[index : end + 1], end + 1 - index + end = index + 1 + while end < len(text) and (text[end].isalnum() or text[end] == "_"): + end += 1 + if end == index + 1: + return "$", 1 + return text[index:end], end - index + + +def _dollar_paren(text: str, start: int) -> tuple[str, int]: + """Return the inner text of ``$(...)`` and the consumed character count.""" + depth = 1 + index = start + 2 + inner_start = index + while index < len(text) and depth: + if text[index] == "(": + depth += 1 + if text[index] == ")": + depth -= 1 + if depth == 0: + return text[inner_start:index], index + 1 - start + index += 1 + return text[inner_start:], len(text) - start + + +def _balanced_dollar(text: str, start: int) -> tuple[str, int]: + """Keep a ``$((...))`` arithmetic expansion intact.""" + depth = 0 + index = start + 1 + while index < len(text): + if text[index] == "(": + depth += 1 + if text[index] == ")": + depth -= 1 + if depth == 0: + return text[start : index + 1], index + 1 - start + index += 1 + return text[start:], len(text) - start + + +def _walk_if_frames(commands: tuple[str, ...]) -> tuple[tuple[_IfFrame, ...], tuple[str, ...]]: + """Collect ``if`` frames and top-level commands from a loop body.""" + stack: list[list[object]] = [] + completed: list[_IfFrame] = [] + top: list[str] = [] + pending: str | None = None + for command in commands: + if command.startswith("if "): + pending = command[3:].strip() + continue + if command == "then": + condition = pending or "" + pending = None + stack.append([condition, True, []]) + continue + if command == "fi": + if not stack: + continue + condition, reachable, body = stack.pop() + completed.append( + _IfFrame(str(condition), "then", tuple(str(item) for item in body), bool(reachable)) + ) + continue + if stack: + condition, reachable, body = stack[-1] + if reachable: + body.append(command) + if _TRANSFER.fullmatch(command) and reachable: + stack[-1][1] = False + continue + top.append(command) + return tuple(completed), tuple(top) + + +def _has_transport_failure_budget(frames: tuple[_IfFrame, ...], top_commands: tuple[str, ...]) -> bool: + """Return whether a negated ``gh api`` branch increments a retry counter.""" + del top_commands + for frame in frames: + if "!" not in frame.condition or _GH_API.search(frame.condition) is None: + continue + if any(_INCREMENT.match(command) for command in frame.commands): + return True + return False + + +def _total_bound_flags(assignments: dict[str, str], frames: tuple[_IfFrame, ...], top_commands: tuple[str, ...]) -> tuple[bool, bool]: + """Return ``(comparison_converges, exit_reachable)`` for loop-local totals.""" + deadlines = { + name + for name, value in assignments.items() + if _is_deadline_init(value) + } + counters = {name for name, value in assignments.items() if value.strip() == "0"} + limits = { + name + for name, value in assignments.items() + if _POSITIVE_INT.fullmatch(value.strip()) + } + top_increments = { + matched.group(1) + for command in top_commands + if (matched := _INCREMENT.match(command)) + } + converges = False + exit_reachable = False + for frame in frames: + test = _TEST.search(frame.condition) + if test is None: + continue + left, operator, right = test.group(1), test.group(2), test.group(3) + direction = _comparison_direction(left, operator, right) + if direction != "forward": + continue + if not _is_total_comparison(left, right, deadlines, counters, limits, top_increments): + continue + converges = True + if frame.reachable and any(_FAIL_EXIT.match(command) for command in frame.commands): + exit_reachable = True + return converges, exit_reachable + + +def _is_deadline_init(value: str) -> bool: + """Return whether an assignment initializes a wall-clock deadline.""" + return "date" in value and "%s" in value and _DEADLINE_OFFSET.search(value) is not None + + +def _comparison_direction(left: str, operator: str, right: str) -> str: + """Classify a test operator as forward, reversed, or unrelated.""" + left_kind = _operand_kind(left) + right_kind = _operand_kind(right) + forward_ops = operator in {"gt", "ge"} + if left_kind == "now" and right_kind.startswith("var:"): + return "forward" if forward_ops else "reversed" + if left_kind.startswith("var:") and right_kind == "now": + return "forward" if not forward_ops else "reversed" + if left_kind.startswith("var:") and right_kind.startswith("var:"): + return "forward" if forward_ops else "reversed" + return "none" + + +def _operand_kind(text: str) -> str: + """Classify a test operand as current time, variable, or other text.""" + if "date" in text and "%s" in text: + return "now" + matched = _VAR_REF.search(text) + if matched: + return "var:" + matched.group(1) + return "other" + + +def _is_total_comparison(left: str, right: str, deadlines: set[str], counters: set[str], limits: set[str], top_increments: set[str]) -> bool: + """Return whether a forward comparison is a loop-local total bound.""" + names = set(_VAR_REF.findall(left + " " + right)) + if names & deadlines: + return True + counter_hit = names & counters & top_increments + limit_hit = names & limits + return bool(counter_hit) and bool(limit_hit) + + +def _assignment(command: str) -> tuple[str, str] | None: + """Parse a simple shell assignment command.""" + matched = _ASSIGN.match(command) + if matched is None: + return None + return matched.group(1), matched.group(2) diff --git a/docs/adr/0009-actions-poll-structural-analyzer.md b/docs/adr/0009-actions-poll-structural-analyzer.md new file mode 100644 index 00000000..d284bd95 --- /dev/null +++ b/docs/adr/0009-actions-poll-structural-analyzer.md @@ -0,0 +1,44 @@ +# ADR-0009: Structural GitHub Actions poll-bound analyzer + +**Status:** Proposed +**Date:** 2026-09-07 + +## Context + +Issue #1087 records a verified control-plane defect: a GitHub Actions verdict poll whose retry budget counted only `gh api` transport failures. Healthy API responses with no verdict could sleep and repeat until a shared runner was retained. PR #1088 packages that pattern as regex detectors (`github-actions-transport-only-poll-bound` and `github-actions-transport-failure-budget-poll-bound`) with a reviewed adjacency-window grammar. + +Those regex rules are migration oracles, not a claim of universal shell parsing. Helper loops, reversed comparisons, unreachable `exit`, quoted or comment text, and sibling-job timeouts are causal control-flow facts. Encoding each new shape as another regex family widens false-positive/false-negative risk and still cannot prove that initialization precedes the candidate loop. + +G-06 therefore needs a bounded structural analyzer over conventional Actions jobs and literal `run: |` shell, without replacing or rewriting the packaged regex identities. + +## Decision + +Add `appguardrail_core.actions_poll_analyzer.classify_poll_loops` as an additive structural classifier: + +- Parse only the conventional two-space workflow subset (`jobs.*.timeout-minutes` and `jobs.*.steps[*].run` block scalars). Do not add a YAML dependency. +- Classify infinite `while :` / `while true` loops that execute `gh api`. +- Treat a transport-failure counter as a budget, not a total bound. +- Accept a total bound only when deadline or attempt state is initialized before this loop, the comparison converges with forward `-gt`/`-ge` (or the swapped equivalent), and a nonzero `exit` is reachable on that path. +- Treat statically positive owning-job `timeout-minutes` literals and `${{ N }}` constant expressions as runner bounds. Zero, negative, empty, dynamic, and sibling-job values are not safety. +- Ignore quoted, commented, and heredoc text when recovering executable commands. +- Map unbounded historical `max_poll_transport_failures` loops onto `github-actions-transport-only-poll-bound` and renamed budgets onto `github-actions-transport-failure-budget-poll-bound` without changing regex IDs. + +This slice does not hook `_scan_file`. The regex corpus remains the production scanner oracle until a later, separately reviewed emission path is proven not to double-count findings. + +## Consequences + +Positive: + +- Causal poll-bound facts (init-before-loop, loop-local convergence, reachable fail-closed exit, owning-job timeout) are testable without a new regex family. +- Existing #1088 detector IDs and fixtures remain migration oracles. + +Negative: + +- Composite actions, generated workflows, cross-file state, noncanonical YAML, and unmodeled shell/fail-fast selection stay out of scope. +- Production scans still emit findings from the regex rules until an emission hook is reviewed. + +## Alternatives + +- **Keep expanding the regex grammar.** Rejected: adjacency windows cannot prove causal initialization or unreachable transfers, and each repair tends to create a new false-positive class. +- **Add PyYAML or a general shell parser.** Rejected for this slice: the packaged scanner has no YAML dependency, and a general parser would overclaim coverage of unreviewed shell. +- **Replace regex IDs immediately via `_scan_file`.** Rejected: a scanner hook can double-emit against the #1088 corpus. Analyzer-only classification preserves the regex owner and keeps this successor stacked. diff --git a/docs/adr/README.md b/docs/adr/README.md index 984420a9..b30036e8 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -10,6 +10,7 @@ | [0004](0004-tenant-network-boundaries.md) | Tenant authority and outbound destinations are explicit security boundaries | Accepted | | [0005](0005-remediation-authority.md) | Deterministic autofix is limited to proven semantics-preserving transforms | Accepted | | [0006](0006-automation-authority.md) | Autonomous development remains separate from independent merge/release authority | Accepted | +| [0009](0009-actions-poll-structural-analyzer.md) | Structural GitHub Actions poll-bound analyzer is additive to the #1088 regex corpus | Proposed | ## ADR triggers diff --git a/tests/test_actions_poll_structural_analyzer.py b/tests/test_actions_poll_structural_analyzer.py new file mode 100644 index 00000000..7dcad711 --- /dev/null +++ b/tests/test_actions_poll_structural_analyzer.py @@ -0,0 +1,723 @@ +"""Structural GitHub Actions poll-bound analyzer regressions for issue #1087. + +These tests are the G-06 RED contract. They call ``classify_poll_loops`` +directly against realistic workflow YAML so the analyzer can replace regex +adjacency windows without inventing a new detector family. Existing packaged +identities remain migration oracles. +""" + +from __future__ import annotations + +from dataclasses import FrozenInstanceError +from pathlib import Path + +import pytest + +from appguardrail_core.actions_poll_analyzer import ( + TRANSPORT_FAILURE_BUDGET_POLL_BOUND, + TRANSPORT_ONLY_POLL_BOUND, + PollLoopAssessment, + classify_poll_loops, + poll_bound_rule_ids, +) + + +_FIXTURES = Path(__file__).parent / "fixtures" / "security_corpus" +_HISTORICAL_VULN = _FIXTURES / "github_actions_transport_only_poll_vulnerable.yml" +_HISTORICAL_FIXED = _FIXTURES / "github_actions_transport_only_poll_fixed.yml" + + +def _workflow( + shell: str, + *, + job: str = "review", + timeout: str | None = None, + extra_jobs: str = "", + run_key: str = "run: |", +) -> str: + """Wrap a literal shell block in conventional two-space Actions YAML.""" + timeout_line = f" timeout-minutes: {timeout}\n" if timeout is not None else "" + body = "\n".join(f" {line}" if line else " " for line in shell.strip("\n").splitlines()) + return ( + "name: Required review\n" + "on:\n" + " pull_request_target:\n" + "jobs:\n" + f"{extra_jobs}" + f" {job}:\n" + " runs-on: ubuntu-24.04\n" + f"{timeout_line}" + " steps:\n" + " - name: Wait for current-head verdict\n" + " env:\n" + " GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}\n" + f" {run_key}\n" + f"{body}\n" + ) + + +def _historical_transport_shell(*, extra_before: str = "", extra_in_loop: str = "") -> str: + """Return the verified transport-failure budget from the source incident.""" + before = extra_before.rstrip("\n") + prefix = f"{before}\n" if before else "" + in_loop = extra_in_loop.rstrip("\n") + loop_extra = f"\n{in_loop}" if in_loop else "" + return f""" +{prefix}set -euo pipefail +verdict="" +review_poll_failures=0 +max_poll_transport_failures=3 +poll_interval_seconds=60 +while :; do{loop_extra} + if ! reviews="$(timeout 30s gh api --paginate "repos/${{GITHUB_REPOSITORY}}/pulls/1/reviews?per_page=100")"; then + review_poll_failures=$((review_poll_failures + 1)) + if [ "$review_poll_failures" -ge "$max_poll_transport_failures" ]; then + exit 1 + fi + sleep "$poll_interval_seconds" + continue + fi + review_poll_failures=0 + verdict="$(printf '%s\\n' "$reviews" | jq -r '.[] | select(.state == "APPROVED") | .state' | tail -1)" + if [ -n "$verdict" ]; then + break + fi + sleep "$poll_interval_seconds" +done +""" + + +def _renamed_transport_shell(*, extra_before: str = "", extra_in_loop: str = "") -> str: + """Return the identifier-agnostic transport-failure budget companion.""" + before = extra_before.rstrip("\n") + prefix = f"{before}\n" if before else "" + in_loop = extra_in_loop.rstrip("\n") + loop_extra = f"\n{in_loop}" if in_loop else "" + return f""" +{prefix}set -euo pipefail +api_error_streak=0 +transport_error_budget=4 +poll_interval_seconds=30 +while :; do{loop_extra} + if ! response="$(gh api repos/example/repo/pulls/7/reviews)"; then + api_error_streak=$((api_error_streak + 1)) + if [ "$api_error_streak" -ge "$transport_error_budget" ]; then + exit 1 + fi + continue + fi + api_error_streak=0 + sleep "$poll_interval_seconds" +done +""" + + +def _polls(workflow: str) -> tuple[PollLoopAssessment, ...]: + """Classify every polling loop in one workflow document.""" + return classify_poll_loops(workflow) + + +def _unbounded(workflow: str) -> tuple[PollLoopAssessment, ...]: + """Return only the transport-only unbounded assessments.""" + return tuple(item for item in _polls(workflow) if item.is_transport_only_unbounded) + + +def test_assessment_is_frozen_and_exposes_causal_fields() -> None: + """Callers cannot mutate a classified loop after the analyzer returns.""" + assessment = PollLoopAssessment( + job_name="review", + transport_failure_budget=True, + loop_local_total_bound=False, + owning_job_timeout=False, + comparison_converges=False, + exit_reachable=False, + is_transport_only_unbounded=True, + ) + + with pytest.raises(FrozenInstanceError): + assessment.is_transport_only_unbounded = False # type: ignore[misc] + assert assessment.job_name == "review" + assert TRANSPORT_ONLY_POLL_BOUND == "github-actions-transport-only-poll-bound" + assert ( + TRANSPORT_FAILURE_BUDGET_POLL_BOUND + == "github-actions-transport-failure-budget-poll-bound" + ) + + +def test_historical_vulnerable_poll_is_transport_only_unbounded() -> None: + """The protected-predecessor incident remains a positive structural finding.""" + workflow = _HISTORICAL_VULN.read_text(encoding="utf-8") + + findings = _unbounded(workflow) + + assert len(findings) == 1 + item = findings[0] + assert item.job_name == "review-verdict" + assert item.transport_failure_budget is True + assert item.loop_local_total_bound is False + assert item.owning_job_timeout is False + assert item.comparison_converges is False + assert item.is_transport_only_unbounded is True + assert poll_bound_rule_ids(findings) == (TRANSPORT_ONLY_POLL_BOUND,) + + +def test_helper_deadline_loop_cannot_donate_safety_to_later_poll() -> None: + """A bounded helper loop is not causal safety for a later transport-only poll.""" + shell = """ +set -euo pipefail +review_poll_failures=0 +max_poll_transport_failures=3 +helper_deadline=$(( $(date -u +%s) + 30 )) +while :; do + if [ "$(date -u +%s)" -ge "$helper_deadline" ]; then + exit 1 + fi + break +done +while :; do + if ! reviews="$(gh api repos/example/repo/pulls/1/reviews)"; then + review_poll_failures=$((review_poll_failures + 1)) + if [ "$review_poll_failures" -ge "$max_poll_transport_failures" ]; then + exit 1 + fi + continue + fi + review_poll_failures=0 + sleep 30 +done +""" + workflow = _workflow(shell) + + findings = _unbounded(workflow) + + assert len(findings) == 1 + assert findings[0].job_name == "review" + assert findings[0].transport_failure_budget is True + assert findings[0].loop_local_total_bound is False + assert findings[0].is_transport_only_unbounded is True + assert poll_bound_rule_ids(findings) == (TRANSPORT_ONLY_POLL_BOUND,) + + +def test_same_loop_wall_clock_deadline_is_a_total_bound() -> None: + """A loop-local date +%s -ge deadline with reachable exit 1 is finite.""" + workflow = _HISTORICAL_FIXED.read_text(encoding="utf-8") + + assessments = _polls(workflow) + + assert assessments + assert not _unbounded(workflow) + item = assessments[0] + assert item.job_name == "review-verdict" + assert item.transport_failure_budget is True + assert item.loop_local_total_bound is True + assert item.comparison_converges is True + assert item.exit_reachable is True + assert item.is_transport_only_unbounded is False + assert poll_bound_rule_ids(assessments) == () + + +def test_same_loop_total_attempt_counter_is_a_total_bound() -> None: + """A loop-local attempt counter compared with -ge and exit 1 bounds the poll.""" + shell = """ +set -euo pipefail +review_poll_failures=0 +max_poll_transport_failures=3 +poll_attempts=0 +max_poll_attempts=120 +while :; do + poll_attempts=$((poll_attempts + 1)) + if [ "$poll_attempts" -ge "$max_poll_attempts" ]; then + exit 1 + fi + if ! reviews="$(gh api repos/example/repo/pulls/1/reviews)"; then + review_poll_failures=$((review_poll_failures + 1)) + if [ "$review_poll_failures" -ge "$max_poll_transport_failures" ]; then + exit 1 + fi + continue + fi + review_poll_failures=0 + sleep 30 +done +""" + workflow = _workflow(shell) + + item = _polls(workflow)[0] + + assert item.loop_local_total_bound is True + assert item.comparison_converges is True + assert item.exit_reachable is True + assert item.is_transport_only_unbounded is False + + +def test_owning_job_timeout_minutes_literal_is_a_total_bound() -> None: + """A statically positive owning-job timeout-minutes is a hard runner bound.""" + workflow = _workflow(_historical_transport_shell(), timeout="30") + + item = _polls(workflow)[0] + + assert item.owning_job_timeout is True + assert item.transport_failure_budget is True + assert item.is_transport_only_unbounded is False + + +def test_owning_job_timeout_minutes_constant_expression_is_a_total_bound() -> None: + """A statically positive ${{ N }} timeout expression bounds the owning job.""" + workflow = _workflow(_renamed_transport_shell(), job="required-review", timeout="${{ 30 }}") + + item = _polls(workflow)[0] + + assert item.owning_job_timeout is True + assert item.is_transport_only_unbounded is False + assert poll_bound_rule_ids((item,)) == () + + +def test_reversed_deadline_comparison_is_not_a_total_bound() -> None: + """A -lt clock comparison does not expire and cannot suppress the finding.""" + extra = ' if [ "$(date -u +%s)" -lt "$poll_deadline_epoch" ]; then\n exit 1\n fi' + shell = _historical_transport_shell( + extra_before="poll_deadline_epoch=$(( $(date -u +%s) + 10800 ))", + extra_in_loop=extra, + ) + workflow = _workflow(shell) + + item = _polls(workflow)[0] + + assert item.comparison_converges is False + assert item.loop_local_total_bound is False + assert item.is_transport_only_unbounded is True + + +def test_unreachable_exit_after_unconditional_break_is_not_safety() -> None: + """Textual exit 1 after an unconditional break cannot establish finiteness.""" + extra = ' if [ "$(date -u +%s)" -ge "$poll_deadline_epoch" ]; then\n break\n exit 1\n fi' + shell = _historical_transport_shell( + extra_before="poll_deadline_epoch=$(( $(date -u +%s) + 10800 ))", + extra_in_loop=extra, + ) + workflow = _workflow(shell) + + item = _polls(workflow)[0] + + assert item.exit_reachable is False + assert item.loop_local_total_bound is False + assert item.is_transport_only_unbounded is True + + +def test_unreachable_exit_after_unconditional_exit_zero_is_not_safety() -> None: + """An exit 0 transfer makes a later fail-closed exit unreachable.""" + extra = ' if [ "$(date -u +%s)" -ge "$poll_deadline_epoch" ]; then\n exit 0\n exit 1\n fi' + shell = _renamed_transport_shell( + extra_before="poll_deadline_epoch=$(( $(date -u +%s) + 600 ))", + extra_in_loop=extra, + ) + workflow = _workflow(shell) + + item = _polls(workflow)[0] + + assert item.exit_reachable is False + assert item.is_transport_only_unbounded is True + assert poll_bound_rule_ids((item,)) == (TRANSPORT_FAILURE_BUDGET_POLL_BOUND,) + + +def test_quoted_and_comment_poll_tokens_are_not_loops() -> None: + """Quoted or commented while/gh api/exit 1 text is not executable evidence.""" + workflow = _workflow( + """ +set -euo pipefail +# while :; do gh api repos/example/repo; exit 1; done +echo "while :; do gh api repos/example/repo/pulls/1/reviews; sleep 30; done" +printf '%s\\n' 'while true; do gh api; exit 1; done' +cat <<'EOF' +while :; do + gh api repos/example/repo + exit 1 +done +EOF +""" + ) + + assert _polls(workflow) == () + assert poll_bound_rule_ids(()) == () + + +def test_sibling_job_timeout_does_not_sanitize_unbounded_poll() -> None: + """A timeout on another job cannot terminate this job's polling loop.""" + helper = """ bounded-helper: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + steps: + - run: | + max_poll_attempts=2 + poll_attempts=0 + poll_deadline_epoch=$(( $(date -u +%s) + 60 )) + while :; do + poll_attempts=$((poll_attempts + 1)) + if [ "$poll_attempts" -ge "$max_poll_attempts" ]; then exit 1; fi + if [ "$(date -u +%s)" -ge "$poll_deadline_epoch" ]; then exit 1; fi + gh api repos/example/repo + sleep 1 + done +""" + workflow = _workflow( + _historical_transport_shell(), + job="required-review", + extra_jobs=helper, + ) + + findings = _unbounded(workflow) + jobs = {item.job_name: item for item in _polls(workflow)} + + assert "required-review" in jobs + assert jobs["required-review"].owning_job_timeout is False + assert jobs["required-review"].is_transport_only_unbounded is True + assert jobs["bounded-helper"].owning_job_timeout is True + assert jobs["bounded-helper"].is_transport_only_unbounded is False + assert any(item.job_name == "required-review" for item in findings) + + +@pytest.mark.parametrize("operator", ["-gt", "-ge"]) +def test_forward_clock_operator_is_a_converging_total_bound(operator: str) -> None: + """Forward -gt and -ge clock comparisons both expire.""" + extra = f' if [ "$(date -u +%s)" {operator} "$poll_deadline_epoch" ]; then\n exit 1\n fi' + shell = _renamed_transport_shell( + extra_before="poll_deadline_epoch=$(( $(date +%s) + 600 ))", + extra_in_loop=extra, + ) + + item = _polls(_workflow(shell))[0] + + assert item.comparison_converges is True + assert item.loop_local_total_bound is True + assert item.is_transport_only_unbounded is False + + +@pytest.mark.parametrize("operator", ["-gt", "-ge"]) +def test_forward_attempt_operator_is_a_converging_total_bound(operator: str) -> None: + """Forward -gt and -ge total-attempt comparisons both terminate the loop.""" + extra = f' poll_attempts=$((poll_attempts + 1))\n if [ "$poll_attempts" {operator} "$max_poll_attempts" ]; then\n exit 1\n fi' + shell = _renamed_transport_shell( + extra_before="poll_attempts=0\nmax_poll_attempts=12", + extra_in_loop=extra, + ) + + item = _polls(_workflow(shell))[0] + + assert item.comparison_converges is True + assert item.is_transport_only_unbounded is False + + +@pytest.mark.parametrize("timeout", ["0", "${{ 0 }}", "${{ -1 }}", "${{ inputs.timeout }}", ""]) +def test_unproved_timeout_expression_is_not_owning_job_safety(timeout: str) -> None: + """Zero, negative, empty, and dynamic timeouts do not bound the runner.""" + workflow = _workflow(_renamed_transport_shell(), timeout=timeout) + + item = _polls(workflow)[0] + + assert item.owning_job_timeout is False + assert item.is_transport_only_unbounded is True + + +def test_timeout_after_steps_still_belongs_to_the_owning_job() -> None: + """YAML key order must not hide a statically positive owning-job timeout.""" + workflow = """ +name: Required review +on: pull_request_target +jobs: + review: + runs-on: ubuntu-24.04 + steps: + - run: | + review_poll_failures=0 + max_poll_transport_failures=3 + while true; do + if ! reviews="$(gh api repos/example/repo/pulls/1/reviews)"; then + review_poll_failures=$((review_poll_failures + 1)) + if [ "$review_poll_failures" -ge "$max_poll_transport_failures" ]; then + exit 1 + fi + continue + fi + review_poll_failures=0 + sleep 30 + done + timeout-minutes: 20 +""" + + item = _polls(workflow)[0] + + assert item.owning_job_timeout is True + assert item.is_transport_only_unbounded is False + + +def test_continue_before_deadline_exit_is_unreachable() -> None: + """An unconditional continue before exit 1 leaves the clock guard unenforced.""" + extra = ' if [ "$(date +%s)" -ge "$poll_deadline_epoch" ]; then\n continue\n exit 1\n fi' + shell = _renamed_transport_shell( + extra_before="poll_deadline_epoch=$(( $(date +%s) + 300 ))", + extra_in_loop=extra, + ) + + item = _polls(_workflow(shell))[0] + + assert item.exit_reachable is False + assert item.is_transport_only_unbounded is True + + +def test_late_deadline_initialization_is_not_causal_safety() -> None: + """A deadline first assigned after the loop begins cannot precede the loop.""" + shell = """ +set -euo pipefail +api_error_streak=0 +transport_error_budget=4 +while :; do + poll_deadline_epoch=$(( $(date +%s) + 300 )) + if [ "$(date +%s)" -ge "$poll_deadline_epoch" ]; then + exit 1 + fi + if ! response="$(gh api repos/example/repo/pulls/7/reviews)"; then + api_error_streak=$((api_error_streak + 1)) + if [ "$api_error_streak" -ge "$transport_error_budget" ]; then + exit 1 + fi + continue + fi + api_error_streak=0 + sleep 30 +done +""" + + item = _polls(_workflow(shell))[0] + + assert item.loop_local_total_bound is False + assert item.is_transport_only_unbounded is True + + +def test_logging_only_deadline_comparison_is_not_a_guard() -> None: + """A converging comparison that only logs does not terminate the runner.""" + extra = ' if [ "$(date +%s)" -ge "$poll_deadline_epoch" ]; then\n echo "deadline passed"\n fi' + shell = _renamed_transport_shell( + extra_before="poll_deadline_epoch=$(( $(date +%s) + 300 ))", + extra_in_loop=extra, + ) + + item = _polls(_workflow(shell))[0] + + assert item.exit_reachable is False + assert item.loop_local_total_bound is False + assert item.is_transport_only_unbounded is True + + +def test_empty_and_non_workflow_text_has_no_poll_loops() -> None: + """Documents without conventional jobs and literal run blocks are empty.""" + assert classify_poll_loops("") == () + assert classify_poll_loops("name: not a workflow\n") == () + assert classify_poll_loops("jobs:\n") == () + assert classify_poll_loops("jobs:\n review:\n runs-on: ubuntu-24.04\n") == () + + +def test_run_block_chomp_marker_is_still_literal_shell() -> None: + """The YAML block chomp marker must not hide an executable poll.""" + workflow = _workflow(_historical_transport_shell(), run_key="run: |-") + + assert _unbounded(workflow) + assert _unbounded(workflow)[0].is_transport_only_unbounded is True + + +def test_poll_without_transport_budget_is_not_the_transport_only_finding() -> None: + """The G-06 finding is a transport-failure budget that is not a total bound.""" + workflow = _workflow( + """ +set -euo pipefail +while :; do + reviews="$(gh api repos/example/repo/pulls/1/reviews)" + sleep 30 +done +""" + ) + + assessments = _polls(workflow) + + assert assessments + assert assessments[0].transport_failure_budget is False + assert assessments[0].is_transport_only_unbounded is False + assert poll_bound_rule_ids(assessments) == () + + +def test_swapped_forward_clock_operands_still_converge() -> None: + """deadline -lt now is the same expiring relation as now -gt deadline.""" + extra = ' if [ "$poll_deadline_epoch" -lt "$(date +%s)" ]; then\n exit 1\n fi' + shell = _renamed_transport_shell( + extra_before="poll_deadline_epoch=$(( $(date +%s) + 600 ))", + extra_in_loop=extra, + ) + + item = _polls(_workflow(shell))[0] + + assert item.comparison_converges is True + assert item.loop_local_total_bound is True + assert item.is_transport_only_unbounded is False + + +def test_tab_separated_gh_api_remains_executable() -> None: + """Command evidence uses word adjacency, not a single-space token.""" + workflow = _workflow( + """ +set -euo pipefail +review_poll_failures=0 +max_poll_transport_failures=3 +while :; do + if ! reviews="$(gh\tapi repos/example/repo/pulls/1/reviews)"; then + review_poll_failures=$((review_poll_failures + 1)) + if [ "$review_poll_failures" -ge "$max_poll_transport_failures" ]; then + exit 1 + fi + continue + fi + review_poll_failures=0 + sleep 30 +done +""" + ) + + assert _unbounded(workflow)[0].is_transport_only_unbounded is True + + +def test_double_quoted_command_substitution_is_executable_poll() -> None: + """gh api inside $(...) remains executable even when the expansion is quoted.""" + workflow = _workflow( + """ +set -euo pipefail +api_error_streak=0 +transport_error_budget=4 +while :; do + if ! response="$(gh api repos/example/repo/pulls/7/reviews)"; then + api_error_streak=$((api_error_streak + 1)) + if [ "$api_error_streak" -ge "$transport_error_budget" ]; then + exit 1 + fi + continue + fi + api_error_streak=0 + sleep 30 +done +""" + ) + + assert _unbounded(workflow)[0].transport_failure_budget is True + + +def test_one_line_fail_closed_deadline_guard_is_a_total_bound() -> None: + """A compact if/then/exit/fi clock guard is still loop-local safety.""" + extra = ' if [ "$(date +%s)" -ge "$poll_deadline_epoch" ]; then exit 1; fi' + shell = _renamed_transport_shell( + extra_before="poll_deadline_epoch=$(( $(date +%s) + 10800 ))", + extra_in_loop=extra, + ) + + item = _polls(_workflow(shell))[0] + + assert item.loop_local_total_bound is True + assert item.exit_reachable is True + assert item.is_transport_only_unbounded is False + + +def test_coverage_edges_keep_causal_boundaries() -> None: + """Parser edges must not invent loops or donate non-causal safety.""" + workflow = """ +name: Required review +on: pull_request_target +jobs: # mapping + ignored-before-first-job: true + review: + runs-on: ubuntu-24.04 + steps: + - run: | + set -euo pipefail + echo \\x + echo "foo\\"bar" + echo 'unterminated + echo "unterminated + echo ${UNCLOSED + echo $(unclosed + echo $ + broken=$((1+ + nested="$(echo $(date +%s))" + cat < None: + """deadline -gt now does not expire and cannot bound the poll.""" + extra = ' if [ "$poll_deadline_epoch" -gt "$(date +%s)" ]; then\n exit 1\n fi' + shell = _renamed_transport_shell( + extra_before="poll_deadline_epoch=$(( $(date +%s) + 600 ))", + extra_in_loop=extra, + ) + + item = _polls(_workflow(shell))[0] + + assert item.comparison_converges is False + assert item.is_transport_only_unbounded is True + + +def test_reversed_attempt_comparison_is_not_a_total_bound() -> None: + """A -lt attempt comparison does not terminate when the counter grows.""" + extra = ' poll_attempts=$((poll_attempts + 1))\n if [ "$poll_attempts" -lt "$max_poll_attempts" ]; then\n exit 1\n fi' + shell = _renamed_transport_shell( + extra_before="poll_attempts=0\nmax_poll_attempts=12", + extra_in_loop=extra, + ) + + item = _polls(_workflow(shell))[0] + + assert item.comparison_converges is False + assert item.is_transport_only_unbounded is True + + +def test_then_without_if_and_le_deadline_remain_unbounded() -> None: + """A dangling then and a non-expiring -le clock guard are not safety.""" + extra = ' then\n if [ "$(date +%s)" -le "$poll_deadline_epoch" ]; then\n exit 1\n fi' + shell = _renamed_transport_shell( + extra_before="poll_deadline_epoch=$(( $(date +%s) + 600 ))", + extra_in_loop=extra, + ) + + item = _polls(_workflow(shell))[0] + + assert item.comparison_converges is False + assert item.is_transport_only_unbounded is True + +