From b8cca58c92afbbd8e9d52e25bf09019c34b61738 Mon Sep 17 00:00:00 2001 From: "Jonathan D.A. Jewell" <6759885+hyperpolymath@users.noreply.github.com> Date: Tue, 22 Sep 2026 12:13:21 +0100 Subject: [PATCH] fix(rules): restore the RE001 structural detectors deleted by #773 (#826) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #773 described itself as "documentation-only" and stated that validation was not run. Its measured diff is 10 insertions and 104 deletions: it removed eight private helpers and replaced the RE001 job analysis with two regexes over raw job text. That broke the rule in BOTH directions at once: * false positive — `uses: "step-security/harden-runner@..."` is valid YAML and does install the hardener, but the regex requires an unquoted value, so RE001 fired against jobs that were already hardened; * false negative — the same string inside a `run: |` or `env: >` block scalar is shell or data, not a step, but the regex matched it and suppressed a genuine finding. Two of the deleted helpers are named for the exact fixes whose tests now fail: `harden_runner_uses?/1` (ab1f855) and `block_scalar?/1` (e047d46). Restores `direct_job_field?/2`, `harden_runner_step?/1`, `job_field_indent/1`, `lines_in_steps/2`, `step_indent/2`, `harden_runner_uses?/1` and `block_scalar?/1`, and re-points the two detectors at them. The @doc is kept and genuinely expanded — which is what #773 claimed to do but did not; it reworded the prose slightly while deleting the logic, and dropped the "without a direct `runs-on` field" precision that `direct_job_field?/2` implements. The new section documents why the check is structural rather than line-oriented, naming both failure directions. Mutants (a passing suite proves nothing until one dies) — every red below is an assertion failure, not a compile error: 1. harden detection reverted to the #773 regex -> BOTH tests red (quoted-uses false positive AND block-scalar false negative) 2. runs-on detection reverted to the #773 regex -> :120 red, reporting a finding at line 8 for a `runs-on` nested inside an `env:` block scalar 3. `block_scalar?/1` neutered to always-false -> :120 red Clean control: 1593 tests, 0 failures. Secondary defect, same issue: tests/e2e.sh printed `tail -120` on failure. ExUnit prints failure diagnostics INLINE as each test fails, not collected at the end, so at ~1600 tests under --trace no fixed tail window can contain one. Measured here: the failing run's log is 4819 lines and `tail -120` contains zero failure blocks — which is why five consecutive red CI runs were undiagnosable and this had to be reproduced locally. The harness now selects failure blocks by content (from the first `N) test` to the end, capped), and falls back to a tail only when no failure block exists, which means the run never reached the tests. Closes #826 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0113HQM9LVGkNCzU1WwkJZSV --- lib/rules/research_extensions.ex | 137 ++++++++++++++++++++++++++++--- tests/e2e.sh | 30 ++++++- 2 files changed, 154 insertions(+), 13 deletions(-) diff --git a/lib/rules/research_extensions.ex b/lib/rules/research_extensions.ex index 2eac987a..93197d04 100644 --- a/lib/rules/research_extensions.ex +++ b/lib/rules/research_extensions.ex @@ -159,11 +159,35 @@ defmodule Hypatia.Rules.ResearchExtensions do @doc """ RE001: Reports each locally executed workflow job that references - `${{ secrets.* }}` without installing `step-security/harden-runner` - in that job. Fully commented lines and reusable-only jobs are ignored. + `${{ secrets.* }}` without a `step-security/harden-runner@...` step in the + same job. Full-line comments and reusable-workflow jobs without a direct + `runs-on` field are ignored. - Each warning points to the first active secret reference in the affected - job. Provenance: StepSecurity Harden-Runner deployment guide. + Each finding points to the first matching secret reference in the job. + Provenance: StepSecurity Harden-Runner deployment guide. + + ## Why this is structural, not a regex + + The check is per job, and both halves of it are indentation-sensitive: + + * **`runs-on` must belong to this job directly.** A reusable job + (`uses:` at job level) delegates its runtime to the source workflow, so a + sibling job's `runs-on` must not satisfy it. `direct_job_field?/2` + therefore compares against the job's own field indent rather than + matching `runs-on:` anywhere in the block. + + * **The hardener must be a real step.** `harden_runner_step?/1` walks + `steps:` by indent and accepts the reference only as a step entry or a + direct property of one. Two cases make a line-oriented match wrong in + *opposite* directions, so both are handled explicitly: + + - `uses: "step-security/harden-runner@..."` — quoted scalars are valid + YAML and still install the hardener. Missing them raises a **false + positive** against an already-hardened job. + - the same text inside a block scalar (`run: |`, `env: >`) is shell or + data, not a step. `block_scalar?/1` opens a skip region until the + indent closes, because treating it as a step raises a **false + negative** that suppresses a genuine finding. Severity: `:warn`. Action: `:report`. """ @@ -185,8 +209,6 @@ defmodule Hypatia.Rules.ResearchExtensions do active_lines |> workflow_job_lines() |> Enum.flat_map(fn job_lines -> - active_content = Enum.map_join(job_lines, "\n", &elem(&1, 0)) - secret_line = Enum.find(job_lines, fn {line, _} -> Regex.match?(~r/\$\{\{\s*secrets\.[A-Za-z_][A-Za-z0-9_]*/, line) @@ -194,10 +216,8 @@ defmodule Hypatia.Rules.ResearchExtensions do # Reusable jobs delegate their runtime to the source workflow. A sibling's # runner or hardener cannot establish this job's execution policy. - local_runner? = Regex.match?(~r/^\s+runs-on:/m, active_content) - - installs_harden? = - Regex.match?(~r/^\s+(?:-\s+)?uses:\s*step-security\/harden-runner@/m, active_content) + local_runner? = direct_job_field?(job_lines, "runs-on") + installs_harden? = harden_runner_step?(job_lines) if not is_nil(secret_line) and local_runner? and not installs_harden? do {_source, line} = secret_line @@ -226,6 +246,103 @@ defmodule Hypatia.Rules.ResearchExtensions do end) end + defp direct_job_field?(job_lines, field) do + case job_field_indent(job_lines) do + nil -> + false + + field_indent -> + Enum.any?(job_lines, fn {line, _} -> + indent_of(line) == field_indent and + String.starts_with?(String.trim_leading(line), "#{field}:") + end) + end + end + + defp harden_runner_step?(job_lines) do + with field_indent when is_integer(field_indent) <- job_field_indent(job_lines), + step_lines when step_lines != [] <- lines_in_steps(job_lines, field_indent), + step_indent when is_integer(step_indent) <- step_indent(step_lines, field_indent) do + case Enum.reduce_while(step_lines, {false, nil, nil}, fn {line, _}, + {in_step, property_indent, + scalar_indent} -> + indent = indent_of(line) + + cond do + scalar_indent && indent > scalar_indent -> + {:cont, {in_step, property_indent, scalar_indent}} + + indent == step_indent and Regex.match?(~r/^\s*-\s+/, line) -> + if harden_runner_uses?(line) do + {:halt, true} + else + {:cont, {true, nil, block_scalar?(line) && indent}} + end + + in_step and indent > step_indent -> + direct_property? = is_nil(property_indent) or indent == property_indent + property_indent = property_indent || indent + + if direct_property? and harden_runner_uses?(line) do + {:halt, true} + else + {:cont, {in_step, property_indent, block_scalar?(line) && indent}} + end + + true -> + {:cont, {in_step, property_indent, nil}} + end + end) do + true -> true + _ -> false + end + else + _ -> false + end + end + + defp job_field_indent([_header | job_lines]) do + job_lines + |> Enum.map(&elem(&1, 0)) + |> Enum.reject(&(String.trim(&1) == "")) + |> Enum.map(&indent_of/1) + |> Enum.min(fn -> nil end) + end + + defp job_field_indent([]), do: nil + + defp lines_in_steps(job_lines, field_indent) do + case Enum.find_index(job_lines, fn {line, _} -> + indent_of(line) == field_indent and + String.starts_with?(String.trim_leading(line), "steps:") + end) do + nil -> + [] + + index -> + job_lines + |> Enum.drop(index + 1) + |> Enum.take_while(fn {line, _} -> + String.trim(line) == "" or indent_of(line) > field_indent + end) + end + end + + defp step_indent(step_lines, field_indent) do + step_lines + |> Enum.filter(fn {line, _} -> + indent_of(line) > field_indent and Regex.match?(~r/^\s*-\s+/, line) + end) + |> Enum.map(fn {line, _} -> indent_of(line) end) + |> Enum.min(fn -> nil end) + end + + defp harden_runner_uses?(line) do + Regex.match?(~r/^\s*(?:-\s+)?uses:\s*["']?step-security\/harden-runner@/, line) + end + + defp block_scalar?(line), do: Regex.match?(~r/:\s*[>|][0-9+-]*\s*(?:#.*)?$/, line) + # Follow block-style jobs by indentation, retaining physical source lines. # As with the other research rules, this is a local static text analysis. defp workflow_job_lines(lines) do diff --git a/tests/e2e.sh b/tests/e2e.sh index 9023ef10..0212ee82 100755 --- a/tests/e2e.sh +++ b/tests/e2e.sh @@ -141,16 +141,40 @@ if $HAS_ELIXIR; then # no assertion, no stacktrace, making the red job impossible to diagnose # from CI. Capture the run, judge it by mix's own exit status, and echo the # output when it fails. + # + # `tail -N` was then the second defect, and it is why five consecutive red + # runs were undiagnosable (issue #826). ExUnit prints failure diagnostics + # INLINE, as each test fails — it does not collect them at the end. Under + # `--trace` this suite emits roughly two lines per test across ~1600 tests, + # so any fixed tail window contains only trace tail and the summary: grepping + # the full logs of five red runs for '^\s*[0-9]+) test ' returned nothing. + # A window sized for a summary cannot show diagnostics emitted inline. + # + # So print the failure BLOCKS, selected by content, not by position: from the + # first ` 1) test ...` to the end of the log. Fall back to a tail only when + # no failure block exists, which means the run died some other way (a compile + # error, or a startup crash) and the tail is then the informative part. _mix_log="${TMPDIR:-/tmp}/hypatia-mix-test.$$.log" if mix test --trace >"$_mix_log" 2>&1; then pass "Elixir unit tests pass" else fail_test "Elixir unit tests" - echo "--- mix test output (tail 120) ---" - tail -120 "$_mix_log" + _n_fail=$(grep -cE '^[[:space:]]*[0-9]+\) test ' "$_mix_log" || true) + echo "--- mix test: ${_n_fail} failure block(s) ---" + if [ "${_n_fail}" -gt 0 ]; then + # Everything from the first failure block onward, capped so a + # pathological run cannot flood the CI log. + sed -n '/^[[:space:]]*1) test /,$p' "$_mix_log" | head -500 + else + echo "(no ExUnit failure block found — the run did not reach the tests)" + tail -120 "$_mix_log" + fi echo "--- end mix test output ---" + echo "full log retained at: $_mix_log" + fi + if [ "${HYPATIA_KEEP_MIX_LOG:-0}" != "1" ]; then + rm -f "$_mix_log" fi - rm -f "$_mix_log" echo "" fi