diff --git a/.github/workflows/actions-poll-analyzer-coverage.yml b/.github/workflows/actions-poll-analyzer-coverage.yml index bb42b772..30ab578d 100644 --- a/.github/workflows/actions-poll-analyzer-coverage.yml +++ b/.github/workflows/actions-poll-analyzer-coverage.yml @@ -59,6 +59,7 @@ jobs: --source=appguardrail_core.actions_poll_analyzer \ -m pytest -q \ tests/test_actions_poll_structural_analyzer.py \ + tests/test_actions_poll_analyzer_emit.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 \ @@ -91,5 +92,6 @@ jobs: python -m compileall -q appguardrail_core/actions_poll_analyzer.py tests/test_actions_poll_structural_analyzer.py + tests/test_actions_poll_analyzer_emit.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-analyzer-emit.md b/CHANGELOG.d/1087-actions-poll-analyzer-emit.md new file mode 100644 index 00000000..cc608402 --- /dev/null +++ b/CHANGELOG.d/1087-actions-poll-analyzer-emit.md @@ -0,0 +1,3 @@ +# Security + +- Emit structural GitHub Actions poll-bound findings through production `_scan_file` using the existing `github-actions-transport-only-poll-bound` and `github-actions-transport-failure-budget-poll-bound` identities. Regex and analyzer hits for the same rule on the same file merge to one finding; helper loops, reversed comparisons, unreachable exits, and split `while`/`do` polls that regex adjacency misses can now be reported. Quoted or comment text stays negative. See issue #1087. diff --git a/appguardrail_core/actions_poll_analyzer.py b/appguardrail_core/actions_poll_analyzer.py index 5aaaff34..2bab5d04 100644 --- a/appguardrail_core/actions_poll_analyzer.py +++ b/appguardrail_core/actions_poll_analyzer.py @@ -140,6 +140,34 @@ def poll_bound_rule_ids(assessments: tuple[PollLoopAssessment, ...]) -> tuple[st return tuple(rule_ids) +def additional_poll_bound_rule_ids( + workflow_text: str, + existing_rule_ids: tuple[str, ...] = (), +) -> tuple[str, ...]: + """Return poll-bound rule IDs not already reported for the same file. + + Merge is keyed by rule identity. Regex hits for an existing detector ID + are preserved; the classifier contributes an ID only when that identity + is absent so a file cannot double-count the packaged #1088 corpus. + + Args: + workflow_text: Complete workflow document text. + existing_rule_ids: Detector identities already emitted for this file. + + Returns: + Existing packaged rule IDs in first-seen analyzer order. + """ + already = set(existing_rule_ids) + extra: list[str] = [] + seen: set[str] = set() + for rule_id in poll_bound_rule_ids(classify_poll_loops(workflow_text)): + if rule_id in already or rule_id in seen: + continue + seen.add(rule_id) + extra.append(rule_id) + return tuple(extra) + + def _is_positive_timeout(value: str) -> bool: """Return whether a timeout-minutes value is a static positive bound.""" stripped = value.split("#", 1)[0].strip() @@ -231,6 +259,7 @@ def _classify_shell(job_name: str, shell: str, owning_timeout: bool) -> tuple[Po converges, exit_reachable = _total_bound_flags(assignments, frames, top_commands) loop_local = converges and exit_reachable historical = _uses_historical_names(assignments, commands) + terminates = _success_path_terminates(top_commands) assessments.append( PollLoopAssessment( job_name=job_name, @@ -239,7 +268,12 @@ def _classify_shell(job_name: str, shell: str, owning_timeout: bool) -> tuple[Po 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, + is_transport_only_unbounded=( + transport + and not loop_local + and not owning_timeout + and not terminates + ), historical_transport_names=historical, ) ) @@ -469,6 +503,20 @@ def _walk_if_frames(commands: tuple[str, ...]) -> tuple[tuple[_IfFrame, ...], tu return tuple(completed), tuple(top) +def _success_path_terminates(top_commands: tuple[str, ...]) -> bool: + """Return whether a reachable top-level transfer ends the poll. + + An unconditional ``break``, ``exit``, or ``return`` removes the back edge. + A top-level ``continue`` skips the rest of the body and repeats the loop. + """ + for command in top_commands: + if _TRANSFER.fullmatch(command): + return not command.startswith("continue") + if _FAIL_EXIT.fullmatch(command): + return True + return False + + 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 diff --git a/scanner/cli/appguardrail.py b/scanner/cli/appguardrail.py index 0d853d64..ce8bba07 100644 --- a/scanner/cli/appguardrail.py +++ b/scanner/cli/appguardrail.py @@ -88,6 +88,7 @@ render_report, supported_report_types, ) +from appguardrail_core.actions_poll_analyzer import additional_poll_bound_rule_ids from appguardrail_core.rules import build_rule_metadata from appguardrail_core.scan_paths import ScanPathContext, build_scan_path_context @@ -2916,6 +2917,53 @@ def _run_codegraph_index(scan_path: Path): return _run_codegraph_command([codegraph, "status"], workdir, "status") +_POLL_WORKFLOW_INCLUDE = ( + ".github/workflows/*.yml", + ".github/workflows/*.yaml", +) + + +def _poll_analyzer_location(content: str) -> tuple[int, str]: + """Return a display line and snippet for an analyzer-only poll finding.""" + for index, raw_line in enumerate(content.splitlines(), start=1): + stripped = raw_line.strip() + if stripped.startswith("while"): + return index, stripped[:120] + return 1, "" + + +def _append_actions_poll_analyzer_findings( + content: str, findings: list, rel_path_str: str, build_finding +) -> None: + """Append structural poll-bound findings not already present for this file. + + Merge is keyed by ``(rule_id, file)``. Regex hits keep their packaged + identity; the classifier adds an ID only when that identity is absent. + """ + extra_ids = additional_poll_bound_rule_ids( + content, tuple(item["rule_id"] for item in findings) + ) + if not extra_ids: + return + templates = { + rule["id"]: rule for rule in SCAN_RULES if rule["id"] in extra_ids + } + line_num, snippet = _poll_analyzer_location(content) + for rule_id in extra_ids: + rule = templates[rule_id] + findings.append( + build_finding( + "appguardrail-rule", + rule_id, + rule["severity"], + rule["message"], + rel_path_str, + line_num, + snippet, + ) + ) + + def _scan_file( file_path: Path, base_path: Path, @@ -2927,6 +2975,10 @@ def _scan_file( Direct callers may omit ``path_context`` and retain the historical safe fallback. Batch callers should build one context and reuse it for every file so root classification and normalized prefix construction happen once. + + GitHub Actions workflow paths also run the structural poll-loop classifier. + Analyzer identities merge with regex hits by ``(rule_id, file)`` so the + packaged transport-only family is emitted once. """ findings = [] context = path_context or build_scan_path_context(base_path) @@ -3027,6 +3079,21 @@ def _scan_file( snippet, ) ) + if ext in {".yml", ".yaml"}: + if rel_path_for_filters is None: + rel_path_for_filters = _display_path( + context.relative_candidate(file_path) + ) + if _path_allowed_by_rule( + rel_path_for_filters, _POLL_WORKFLOW_INCLUDE, () + ): + if rel_path_str is None: + rel_path_str = _sanitize_terminal_output( + _display_path(context.relative_candidate(file_path)) + ) + _append_actions_poll_analyzer_findings( + content, findings, rel_path_str, build_finding + ) except (OSError, PermissionError): pass diff --git a/tests/test_actions_poll_analyzer_emit.py b/tests/test_actions_poll_analyzer_emit.py new file mode 100644 index 00000000..d4c7880a --- /dev/null +++ b/tests/test_actions_poll_analyzer_emit.py @@ -0,0 +1,324 @@ +"""Emit structural poll-bound findings through production ``_scan_file``. + +G-06 successor of the additive analyzer: regex adjacency still misses split +``while``/``do`` helper loops, unreachable fail-closed exits, and renamed +budgets with the same shape. Production scanning must surface those with the +existing detector identities and must not double-count the #1088 corpus. +""" + +from __future__ import annotations + +from pathlib import Path + +from scanner.cli.appguardrail import ( + SCAN_RULES, + _append_actions_poll_analyzer_findings, + _poll_analyzer_location, + _scan_file, +) + + +_HISTORICAL = "github-actions-transport-only-poll-bound" +_GENERIC = "github-actions-transport-failure-budget-poll-bound" +_POLL_IDS = {_HISTORICAL, _GENERIC} +_FIXTURES = Path(__file__).parent / "fixtures" / "security_corpus" + + +def _scan_workflow( + tmp_path: Path, content: str, *, name: str = "required-review.yml" +) -> list[dict]: + """Scan one GitHub Actions workflow through the production file scanner.""" + workflow = tmp_path / ".github" / "workflows" / name + workflow.parent.mkdir(parents=True, exist_ok=True) + workflow.write_text(content, encoding="utf-8") + return _scan_file(workflow, tmp_path) + + +def _poll_ids(findings: list[dict]) -> list[str]: + """Return the two poll-bound identities in scan order.""" + return [finding["rule_id"] for finding in findings if finding["rule_id"] in _POLL_IDS] + + +def _workflow(shell: str) -> str: + """Wrap a literal shell block in conventional two-space Actions YAML.""" + 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" + " review:\n" + " runs-on: ubuntu-24.04\n" + " steps:\n" + " - name: Wait for current-head verdict\n" + " run: |\n" + f"{body}\n" + ) + + +def _historical_transport_split_do(*, extra_before: str = "", extra_in_loop: str = "") -> str: + """Return the historical transport budget with ``do`` on the following line. + + Packaged regex requires ``while :; do`` on one line, so this shape is an + analyzer-only positive unless production ``_scan_file`` runs the classifier. + """ + 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 + [ -n "$verdict" ] && break + sleep "$poll_interval_seconds" +done +""" + + +def _renamed_transport_split_do() -> str: + """Return the identifier-agnostic transport budget with split ``do``.""" + return """ +set -euo pipefail +api_error_streak=0 +transport_error_budget=4 +poll_interval_seconds=30 +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 "$poll_interval_seconds" +done +""" + + +def test_packaged_poll_identities_remain_the_only_two_rule_ids() -> None: + """Emission must reuse the #1088 identities; it must not invent a third.""" + poll_rules = [rule for rule in SCAN_RULES if rule["id"] in _POLL_IDS] + assert {rule["id"] for rule in poll_rules} == _POLL_IDS + assert "github-actions-poll-structural-analyzer" not in { + rule["id"] for rule in SCAN_RULES + } + + +def test_historical_vulnerable_fixture_emits_the_rule_once(tmp_path: Path) -> None: + """Regex plus analyzer must not double-count the pinned vulnerable oracle.""" + content = ( + _FIXTURES / "github_actions_transport_only_poll_vulnerable.yml" + ).read_text(encoding="utf-8") + + findings = _scan_workflow(tmp_path, content) + + assert _poll_ids(findings).count(_HISTORICAL) == 1 + assert _GENERIC not in _poll_ids(findings) + + +def test_historical_fixed_fixture_stays_negative_for_both_identities( + tmp_path: Path, +) -> None: + """The protected wall-clock repair remains a negative oracle for both IDs.""" + content = ( + _FIXTURES / "github_actions_transport_only_poll_fixed.yml" + ).read_text(encoding="utf-8") + + assert _poll_ids(_scan_workflow(tmp_path, content)) == [] + + +def test_scan_file_emits_analyzer_only_helper_loop_with_split_do( + tmp_path: Path, +) -> None: + """A bounded helper loop cannot hide a later split-do 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 +""" + + findings = _scan_workflow(tmp_path, _workflow(shell)) + + assert _poll_ids(findings).count(_HISTORICAL) == 1 + assert _GENERIC not in _poll_ids(findings) + + +def test_scan_file_emits_renamed_budget_when_do_is_on_the_next_line( + tmp_path: Path, +) -> None: + """Identifier-agnostic transport budgets with split ``do`` stay detectable.""" + findings = _scan_workflow(tmp_path, _workflow(_renamed_transport_split_do())) + + assert _poll_ids(findings).count(_GENERIC) == 1 + assert _HISTORICAL not in _poll_ids(findings) + + +def test_scan_file_emits_reversed_clock_comparison_with_split_do( + tmp_path: Path, +) -> None: + """A ``-lt`` deadline comparison does not expire and must remain a finding.""" + shell = _historical_transport_split_do( + extra_before="poll_deadline_epoch=$(( $(date -u +%s) + 10800 ))", + extra_in_loop=' if [ "$(date -u +%s)" -lt "$poll_deadline_epoch" ]; then\n exit 1\n fi', + ) + + assert _poll_ids(_scan_workflow(tmp_path, _workflow(shell))).count(_HISTORICAL) == 1 + + +def test_scan_file_emits_unreachable_exit_after_unconditional_break( + tmp_path: Path, +) -> None: + """Textual ``exit 1`` after ``break`` cannot donate loop-local safety.""" + shell = _historical_transport_split_do( + extra_before="poll_deadline_epoch=$(( $(date -u +%s) + 10800 ))", + extra_in_loop=( + ' if [ "$(date -u +%s)" -ge "$poll_deadline_epoch" ]; then\n' + " break\n" + " exit 1\n" + " fi" + ), + ) + + assert _poll_ids(_scan_workflow(tmp_path, _workflow(shell))).count(_HISTORICAL) == 1 + + +def test_quoted_and_comment_poll_text_stays_negative(tmp_path: Path) -> None: + """Quoted, commented, and heredoc poll tokens are not executable evidence.""" + shell = """ +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 _poll_ids(_scan_workflow(tmp_path, _workflow(shell))) == [] + + +def test_quoted_decoy_does_not_hide_split_do_poll(tmp_path: Path) -> None: + """Quoted command text next to a real split-do poll is not a suppressor.""" + shell = """ +set -euo pipefail +review_poll_failures=0 +max_poll_transport_failures=3 +echo "while :; do gh api repos/example/repo/pulls/1/reviews; sleep 30; 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 +""" + + assert _poll_ids(_scan_workflow(tmp_path, _workflow(shell))).count(_HISTORICAL) == 1 + + +def test_non_workflow_yaml_does_not_emit_poll_identities(tmp_path: Path) -> None: + """Analyzer emission stays scoped to GitHub Actions workflow paths.""" + target = tmp_path / "deploy.yml" + target.write_text(_workflow(_historical_transport_split_do()), encoding="utf-8") + + assert _poll_ids(_scan_file(target, tmp_path)) == [] + + +def test_yaml_extension_workflow_emits_analyzer_only_helper_loop( + tmp_path: Path, +) -> None: + """The ``*.yaml`` workflow glob must emit the same analyzer-only identity.""" + findings = _scan_workflow( + tmp_path, + _workflow(_historical_transport_split_do()), + name="required-review.yaml", + ) + + assert _poll_ids(findings).count(_HISTORICAL) == 1 + + +def test_workflow_without_poll_tokens_does_not_emit(tmp_path: Path) -> None: + """A conventional workflow with no polling loop stays negative.""" + content = """name: Required review +on: pull_request_target +jobs: + review: + runs-on: ubuntu-24.04 + steps: + - run: echo hi +""" + + assert _poll_ids(_scan_workflow(tmp_path, content)) == [] + + +def test_poll_analyzer_location_uses_while_line_or_falls_back() -> None: + """Analyzer-only findings point at the first while line when one exists.""" + assert _poll_analyzer_location("name: x\n") == (1, "") + line, snippet = _poll_analyzer_location("name: x\n while :\n") + assert line == 2 + assert snippet.startswith("while") + + +def test_append_is_a_no_op_when_regex_already_reported_the_identity() -> None: + """Direct merge keeps a pre-existing regex finding as the sole identity.""" + findings = [{"rule_id": _HISTORICAL}] + _append_actions_poll_analyzer_findings( + ( + _FIXTURES / "github_actions_transport_only_poll_vulnerable.yml" + ).read_text(encoding="utf-8"), + findings, + ".github/workflows/required-review.yml", + lambda *args, **kwargs: (_ for _ in ()).throw( + AssertionError("regex hit must not grow an analyzer duplicate") + ), + ) + + assert findings == [{"rule_id": _HISTORICAL}] diff --git a/tests/test_actions_poll_structural_analyzer.py b/tests/test_actions_poll_structural_analyzer.py index 7dcad711..9f9aee03 100644 --- a/tests/test_actions_poll_structural_analyzer.py +++ b/tests/test_actions_poll_structural_analyzer.py @@ -17,6 +17,7 @@ TRANSPORT_FAILURE_BUDGET_POLL_BOUND, TRANSPORT_ONLY_POLL_BOUND, PollLoopAssessment, + additional_poll_bound_rule_ids, classify_poll_loops, poll_bound_rule_ids, ) @@ -721,3 +722,84 @@ def test_then_without_if_and_le_deadline_remain_unbounded() -> None: assert item.is_transport_only_unbounded is True +def test_additional_poll_bound_rule_ids_skips_regex_hits_for_the_same_file() -> None: + """File-scoped merge keeps the packaged identity when regex already reported it.""" + workflow = _HISTORICAL_VULN.read_text(encoding="utf-8") + + assert additional_poll_bound_rule_ids(workflow) == (TRANSPORT_ONLY_POLL_BOUND,) + assert additional_poll_bound_rule_ids( + workflow, (TRANSPORT_ONLY_POLL_BOUND,) + ) == () + assert additional_poll_bound_rule_ids( + _HISTORICAL_FIXED.read_text(encoding="utf-8") + ) == () + + +def test_additional_poll_bound_rule_ids_emits_each_identity_once() -> None: + """Two unbounded loops of the same family still contribute one rule ID.""" + other = """ sibling-poll: + runs-on: ubuntu-24.04 + steps: + - run: | + review_poll_failures=0 + max_poll_transport_failures=3 + while :; do + if ! reviews="$(gh api repos/example/repo/pulls/2/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(_historical_transport_shell(), extra_jobs=other) + ids = poll_bound_rule_ids(_polls(workflow)) + + assert ids == (TRANSPORT_ONLY_POLL_BOUND, TRANSPORT_ONLY_POLL_BOUND) + assert additional_poll_bound_rule_ids(workflow) == (TRANSPORT_ONLY_POLL_BOUND,) + assert additional_poll_bound_rule_ids( + workflow, (TRANSPORT_ONLY_POLL_BOUND,) + ) == () + + +def test_additional_poll_bound_rule_ids_keeps_renamed_budget_identity() -> None: + """Renamed transport budgets stay on the identifier-agnostic packaged ID.""" + workflow = _workflow(_renamed_transport_shell()) + + assert additional_poll_bound_rule_ids(workflow) == ( + TRANSPORT_FAILURE_BUDGET_POLL_BOUND, + ) + assert additional_poll_bound_rule_ids( + workflow, (TRANSPORT_FAILURE_BUDGET_POLL_BOUND,) + ) == () + + +@pytest.mark.parametrize("terminator", ["break", "exit", "exit 0", "exit 1", "return"]) +def test_unconditional_success_path_terminator_is_finite(terminator: str) -> None: + """A reachable top-level transfer removes the polling back edge.""" + extra = f" {terminator}" + shell = _historical_transport_shell(extra_in_loop=extra) + + item = _polls(_workflow(shell))[0] + + assert item.transport_failure_budget is True + assert item.is_transport_only_unbounded is False + assert additional_poll_bound_rule_ids(_workflow(shell)) == () + + +def test_top_level_continue_does_not_terminate_the_poll() -> None: + """A top-level continue skips later commands and repeats the loop.""" + extra = " continue\n break" + shell = _historical_transport_shell(extra_in_loop=extra) + + item = _polls(_workflow(shell))[0] + + assert item.is_transport_only_unbounded is True + assert additional_poll_bound_rule_ids(_workflow(shell)) == ( + TRANSPORT_ONLY_POLL_BOUND, + ) + +