From c97ceb90a088378e1f38ddebbe45eb1a7044ce80 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 27 Oct 2026 03:44:56 +0000 Subject: [PATCH 1/2] RE001: read workflows as YAML does (quoted keys, aliases, with: inputs) Codex review of 6f57606: - A declared job that reads secrets by a computed key is reported even when harden-runner is installed: a fixed :secrets list cannot describe it, and declarations are attestations whether or not the job is hardened. The fix text says the entry can simply be removed. - Quoted keys are the same keys. One matcher, key_line?/2, now serves the top-level env: and jobs:, and the job's env:, runs-on:, steps: and if:. A quoted "runs-on" used to drop the whole job from RE001, and a quoted top-level "env" hid inherited secrets from drift checks. The exposure model's uses:/ref:/repository:/run: keys get the same. - The harden-runner policy is read from the step's with: inputs (block or flow mapping), with YAML comments stripped, so `uses: ...@v2 # egress-policy: block` no longer attests block. - YAML aliases are expanded before jobs are read: `env: *common`, `<<: *defaults` and `deploy: *build` bring the anchored lines into the job, at the alias's line number. Expansion is limited to jobs and the top-level env:, within a 5,000-line budget per workflow, so a billion-laughs file costs a bounded amount of work. Also takes CodeRabbit's nit: deed_test.exs uses ExUnit's per-test tmp_dir instead of hand-made directories under System.tmp_dir!(). research_extensions_test + deed_test: 161 tests, 0 failures; the 9 new or extended tests fail against 6f57606. kyaml_workflow_test 10/10. RE001 on real trees is unchanged: hypatia 9, standards 15, rsr-template-repo 0, choreographic-types 1. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01J4QvDithtn5Xk5MbSPm4Px --- CHANGELOG.adoc | 8 +- lib/rules/research_extensions.ex | 297 ++++++++++++++++++++++++++---- test/deed_test.exs | 60 +++--- test/research_extensions_test.exs | 271 +++++++++++++++++++++++++++ 4 files changed, 575 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index a6ccd388..a8596062 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -25,13 +25,19 @@ to declare egress instead of adopting harden-runner everywhere. RE001 now: * reports drift (the declaration and the workflow disagree on secrets) and stale declarations (the job is gone, runs elsewhere, or reads no real secret), and checks a declaration even when harden-runner satisfies the job, including a - claimed `:enforcement harden-runner-*` on a job that no longer installs it; + claimed `:enforcement harden-runner-*` on a job that no longer installs it, + and a claimed `harden-runner-block` on a step whose `with:` input (YAML + comments aside) leaves the policy at audit; * names the job and gives the exact `(job …)` entry in its fix text; * reads every `${{ … }}` expression whole, even one a folded scalar splits across lines, and counts only the top-level `secrets` context (matched without regard to case), in property or index form, outside string literals; a read by a key computed at run time (`secrets[env.X]`, `toJSON(secrets)`) is its own finding, since no declaration can list it; +* reads the workflow as YAML does in two more ways: a quoted key (`"env":`, + `'runs-on':`) is the same key, and an alias (`env: *common`, `<<: *defaults`, + `deploy: *build`) brings its anchor's lines into the job that uses it, within + a budget of copied lines that stops a "billion laughs" file; * keeps #966's exposure model as the severity layer: an undeclared job is `:high` when the credential shares a step with fetched or untrusted code, and otherwise `:warn`, with or without an exposure (owner ruling diff --git a/lib/rules/research_extensions.ex b/lib/rules/research_extensions.ex index 7966247f..8434f06a 100644 --- a/lib/rules/research_extensions.ex +++ b/lib/rules/research_extensions.ex @@ -179,9 +179,9 @@ defmodule Hypatia.Rules.ResearchExtensions do # Where naming that head means fetching its code: a checkout's `ref:` or # `repository:`, or a git checkout/fetch/switch in a `run:` script. A head # SHA in `env:`, an `if:` or a `concurrency:` group only names it. - # Block (`ref: …`) or flow (`with: {ref: …}`) form. - @re001_checkout_key ~r/(?:^|[\s{,])(?:ref|repository)\s*:/ - @re001_checkout_action ~r/^\s*(?:-\s+)?uses:\s*["']?actions\/checkout@/ + # Block (`ref: …`) or flow (`with: {ref: …}`) form, the key plain or quoted. + @re001_checkout_key ~r/(?:^|[\s{,])["']?(?:ref|repository)["']?\s*:/ + @re001_checkout_action ~r/^\s*(?:-\s+)?["']?uses["']?\s*:\s*["']?actions\/checkout@/ # One git command naming the head: no `;`, `&` or `|` between them. `\s` # spans the line break of a folded `run: >-`. @re001_git_head_fetch ~r/\bgit\s+(?:checkout|fetch|switch)\b[^;&|]*?(?:github\.event\.pull_request\.head\.|github\.event\.workflow_run\.head(?:\.|_(?:sha|branch|repository)\b)|github\.head_ref)/ @@ -239,7 +239,9 @@ defmodule Hypatia.Rules.ResearchExtensions do observation or a model of what each action reaches. Full-line comments and reusable-workflow jobs without a direct `runs-on` - field are ignored. Provenance: StepSecurity Harden-Runner deployment guide. + field are ignored. Keys are matched plain or quoted, as YAML reads them, and + YAML aliases are expanded before jobs are read (`expand_aliases/1`). + Provenance: StepSecurity Harden-Runner deployment guide. ## Why this is structural, not a regex @@ -294,8 +296,9 @@ defmodule Hypatia.Rules.ResearchExtensions do # already satisfies the rule: what it lists, and the enforcement it # claims, must still be true. {:ok, decl} -> + # A fixed list cannot describe a key computed at run time, hardened or not. dynamic = - if job.dynamic_line && not job.hardened?, + if job.dynamic_line, do: [re001_dynamic(job, deed_file, true)], else: [] @@ -345,6 +348,7 @@ defmodule Hypatia.Rules.ResearchExtensions do |> String.split("\n") |> Enum.with_index(1) |> Enum.reject(fn {line, _} -> String.starts_with?(String.trim_leading(line), "#") end) + |> expand_aliases() untrusted_events = active_lines @@ -414,6 +418,116 @@ defmodule Hypatia.Rules.ResearchExtensions do end) end + # YAML anchors and aliases, which GitHub expands in workflows: `*name` + # repeats the node anchored `&name`, so `env: *common`, `<<: *defaults` or + # `deploy: *build` brings the anchored lines into the job that holds the + # alias. Each alias is expanded in place before any job is read: the + # anchored lines are copied in after it, re-indented and carrying the + # alias's line number, so a finding points at the alias. A `<<:` merge puts + # its lines into the mapping that holds the key, at the key's indent; any + # other alias's lines go at the column of its `*`, under the node it stands + # for. The anchor is the nearest one of that name above the alias. + @anchor ~r/(?:^|[\s\[{,])&([A-Za-z0-9_-]+)/ + @alias ~r/(?:^|[\s\[{,])\*([A-Za-z0-9_-]+)/ + @merge_key ~r/^\s*(?:-\s+)?(?:<<|"<<"|'<<')\s*:/ + # Nested anchors can multiply copies exponentially (the "billion laughs" + # file), so expansion stops after this many copied lines per workflow. + @alias_budget 5_000 + defp expand_aliases(lines) do + anchors = + for {{line, n}, index} <- Enum.with_index(lines), + text = strip_yaml_comment(line), + [_, {at, len}] <- Regex.scan(@anchor, text, return: :index) do + {binary_part(text, at, len), n, anchored_lines(lines, index, text, at + len)} + end + + # Only what jobs read is expanded in place: the jobs and the top-level + # env:. An anchor kept elsewhere is expanded where a job uses it. + {expanded, _} = + Enum.flat_map_reduce(lines, {nil, @alias_budget}, fn {line, _} = entry, {section, budget} -> + section = + case Regex.run(~r/^["']?([^\s"':]+)["']?\s*:/, line) do + [_, key] -> key + nil -> section + end + + if section in ["jobs", "env"] do + {copies, budget} = expand_line(entry, anchors, budget) + {copies, {section, budget}} + else + {[entry], {section, budget}} + end + end) + + expanded + end + + defp expand_aliases(lines, anchors, budget) do + Enum.flat_map_reduce(lines, budget, &expand_line(&1, anchors, &2)) + end + + # One line followed by copies of the anchors its aliases name. + defp expand_line(entry, _anchors, budget) when budget <= 0, do: {[entry], budget} + + defp expand_line({line, n} = entry, anchors, budget) do + text = strip_yaml_comment(line) + + {copies, budget} = + for [_, {at, len}] <- Regex.scan(@alias, text, return: :index), + name = binary_part(text, at, len), + {_, _, body} <- + anchors |> Enum.filter(fn {a, m, _} -> a == name and m < n end) |> Enum.take(-1), + reduce: {[], budget} do + {copies, budget} when budget <= 0 -> + {copies, budget} + + {copies, budget} -> + target = + if Regex.match?(@merge_key, text), + do: indent_of(text), + else: String.length(binary_part(text, 0, at - 1)) + + {body, budget} = expand_aliases(body, anchors, budget - length(body)) + {copies ++ Enum.map(reindent(body, target), fn {copy, _} -> {copy, n} end), budget} + end + + {[entry | copies], budget} + end + + # The lines an anchor names: what follows it on its own line, then every + # line indented deeper than that line. + defp anchored_lines(lines, index, text, after_name) do + {_, n} = Enum.at(lines, index) + indent = indent_of(text) + inline = text |> binary_part(after_name, byte_size(text) - after_name) |> String.trim() + + inline_lines = + if inline == "", do: [], else: [{String.duplicate(" ", indent + 2) <> inline, n}] + + block = + lines + |> Enum.drop(index + 1) + |> Enum.take_while(fn {line, _} -> String.trim(line) == "" or indent_of(line) > indent end) + + inline_lines ++ block + end + + defp reindent(lines, target) do + base = + lines + |> Enum.map(&elem(&1, 0)) + |> Enum.reject(&(String.trim(&1) == "")) + |> Enum.map(&indent_of/1) + |> Enum.min(fn -> 0 end) + + Enum.map(lines, fn {line, n} -> + if String.trim(line) == "", + do: {"", n}, + else: + {String.duplicate(" ", indent_of(line) - base + target) <> String.trim_leading(line), n} + end) + end + # Every secret a job reads inside ${{ … }}, as {NAME, line}: the property # form secrets.NAME and the index form secrets['NAME'] / secrets["NAME"], # anywhere in the expression (${{ vars.X || secrets.Y }} reads Y). GitHub @@ -525,9 +639,16 @@ defmodule Hypatia.Rules.ResearchExtensions do detail: %{ exposures: job.exposures, fix: - "Read each secret by its literal name (secrets.NAME) and declare the set, " <> - "or add harden-runner as the first step of this job with egress-policy: block — " <> - "standards ADR-009." + if(job.hardened?, + do: + "Read each secret by its literal name (secrets.NAME) and declare the set, " <> + "or remove the job's entry from #{deed_file}: its harden-runner step " <> + "already satisfies RE001 — standards ADR-009.", + else: + "Read each secret by its literal name (secrets.NAME) and declare the set, " <> + "or add harden-runner as the first step of this job with egress-policy: block — " <> + "standards ADR-009." + ) } } end @@ -661,7 +782,7 @@ defmodule Hypatia.Rules.ResearchExtensions do # Only the step's own `run:` property: an `env:` variable named `run` is a # nested key, deeper than the step's properties. case Enum.split_while(step, fn {line, _} -> - case Regex.run(~r/^(\s*(?:-\s+)?)run:/, line) do + case Regex.run(~r/^(\s*(?:-\s+)?)(?:run|"run"|'run')\s*:/, line) do [_, prefix] -> String.length(prefix) != key_column _ -> true end @@ -671,7 +792,9 @@ defmodule Hypatia.Rules.ResearchExtensions do {_, [{run_line, _} | rest]} -> [prefix, value] = - Regex.run(~r/^(\s*(?:-\s+)?)run:\s*(.*)$/, run_line, capture: :all_but_first) + Regex.run(~r/^(\s*(?:-\s+)?)(?:run|"run"|'run')\s*:\s*(.*)$/, run_line, + capture: :all_but_first + ) key_column = String.length(prefix) @@ -718,11 +841,12 @@ defmodule Hypatia.Rules.ResearchExtensions do end end - # The lines under the job's own `env:` key. # The workflow's top-level `env:` (key at column 0) with its indented body. # Every job, and every step in it, inherits these variables. defp workflow_env_lines(active_lines) do - case Enum.drop_while(active_lines, fn {line, _} -> not Regex.match?(~r/^env:/, line) end) do + case Enum.drop_while(active_lines, fn {line, _} -> + not (indent_of(line) == 0 and key_line?(line, "env")) + end) do [env_line | rest] -> body = Enum.take_while(rest, fn {line, _} -> line == "" or indent_of(line) > 0 end) [env_line | body] @@ -732,12 +856,12 @@ defmodule Hypatia.Rules.ResearchExtensions do end end + # The lines under the job's own `env:` key. defp job_env_lines(job_lines) do with field_indent when is_integer(field_indent) <- job_field_indent(job_lines), index when is_integer(index) <- Enum.find_index(job_lines, fn {line, _} -> - indent_of(line) == field_indent and - String.starts_with?(String.trim_leading(line), "env:") + indent_of(line) == field_indent and key_line?(line, "env") end) do job_lines |> Enum.drop(index) @@ -757,10 +881,9 @@ defmodule Hypatia.Rules.ResearchExtensions do with field_indent when is_integer(field_indent) <- job_field_indent(job_lines), {line, _} <- Enum.find(job_lines, fn {line, _} -> - indent_of(line) == field_indent and - String.starts_with?(String.trim_leading(line), "if:") + indent_of(line) == field_indent and key_line?(line, "if") end), - [_, condition] <- Regex.run(~r/^\s*if:\s*(.*?)\s*$/, line), + [_, condition] <- Regex.run(~r/^\s*["']?if["']?\s*:\s*(.*?)\s*$/, line), condition = condition |> String.replace(~r/^\$\{\{\s*|\s*\}\}$/, ""), test = ~S"github\.event_name\s*==\s*'([a-z_]+)'", true <- Regex.match?(~r/^\s*#{test}(?:\s*\|\|\s*#{test})*\s*$/, condition) do @@ -882,12 +1005,18 @@ defmodule Hypatia.Rules.ResearchExtensions do field_indent -> Enum.any?(job_lines, fn {line, _} -> - indent_of(line) == field_indent and - String.starts_with?(String.trim_leading(line), "#{field}:") + indent_of(line) == field_indent and key_line?(line, field) end) end end + # A line that opens the mapping key `key`: YAML reads `env:`, `"env":` and + # `'env':` as the same key, and so does GitHub. + defp key_line?(line, key) do + key = Regex.escape(key) + Regex.match?(~r/^\s*(?:#{key}|"#{key}"|'#{key}')\s*:(?:\s|$)/, line) + 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), @@ -942,8 +1071,7 @@ defmodule Hypatia.Rules.ResearchExtensions do 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:") + indent_of(line) == field_indent and key_line?(line, "steps") end) do nil -> [] @@ -966,30 +1094,135 @@ defmodule Hypatia.Rules.ResearchExtensions do |> Enum.min(fn -> nil end) end - # The egress-policy of the job's harden-runner step, block or flow `with:`; - # harden-runner defaults to audit when none is set. nil without the step. + # The egress-policy input of the job's harden-runner step; harden-runner + # defaults to audit when none is set. nil without the step. defp harden_runner_policy(steps) do case Enum.find(steps, fn step -> Enum.any?(step, fn {line, _} -> harden_runner_uses?(line) end) end) do + nil -> nil + step -> step |> step_inputs() |> Map.get("egress-policy", "audit") + end + end + + # One step's `with:` inputs as %{name => value}, from a block mapping under + # the key or a flow mapping on its line. A YAML comment sets no input, so + # comments are stripped first: `uses: …@v2 # egress-policy: block` leaves + # the policy at its default. + @with_key ~r/^(\s*(?:-\s+)?)(?:with|"with"|'with')\s*:(?:\s(.*))?$/ + @block_input ~r/^\s*["']?([A-Za-z0-9_-]+)["']?\s*:(?:\s+(.*))?$/ + @flow_input ~r/[{,]\s*["']?([A-Za-z0-9_-]+)["']?\s*:\s*("(?:[^"\\]|\\.)*"|'(?:[^']|'')*'|[^,}]*)/ + defp step_inputs(step) do + lines = Enum.map(step, fn {line, _} -> strip_yaml_comment(line) end) + + case Enum.find_index(lines, &Regex.match?(@with_key, &1)) do nil -> - nil + %{} + + index -> + [with_line | rest] = Enum.drop(lines, index) + [lead, inline] = with_key_parts(with_line) - step -> - Enum.find_value(step, "audit", fn {line, _} -> - case Regex.run(~r/(?:^|[\s{,])egress-policy:\s*["']?([A-Za-z-]+)/, line) do - [_, policy] -> policy - nil -> nil + if String.starts_with?(inline, "{") do + # A flow mapping may continue past the `with:` line. + rest + |> Enum.reduce_while(inline, fn line, acc -> + if nesting_at(acc, byte_size(acc)) > 0, + do: {:cont, acc <> " " <> String.trim(line)}, + else: {:halt, acc} + end) + |> then(&Regex.scan(@flow_input, &1, capture: :all_but_first)) + |> Map.new(fn [name, value] -> {name, scalar_value(value)} end) + else + column = String.length(lead) + body = Enum.take_while(rest, &(String.trim(&1) == "" or indent_of(&1) > column)) + + child_indent = + body + |> Enum.reject(&(String.trim(&1) == "")) + |> Enum.map(&indent_of/1) + |> Enum.min(fn -> nil end) + + for line <- body, + indent_of(line) == child_indent, + [_ | captures] <- [Regex.run(@block_input, line)], + into: %{} do + case captures do + [name, value] -> {name, scalar_value(value)} + [name] -> {name, ""} + end end - end) + end + end + end + + defp with_key_parts(line) do + case Regex.run(@with_key, line) do + [_, lead, inline] -> [lead, String.trim(inline)] + [_, lead] -> [lead, ""] + end + end + + # A plain or quoted scalar's text. + defp scalar_value(value) do + value = String.trim(value) + + cond do + Regex.match?(~r/^"(?:[^"\\]|\\.)*"$/, value) -> + String.slice(value, 1..-2//1) + + Regex.match?(~r/^'(?:[^']|'')*'$/, value) -> + value |> String.slice(1..-2//1) |> String.replace("''", "'") + + true -> + value end end + # A line without its YAML comment: a `#` at the start or after whitespace, + # outside a quoted scalar. A quote opens a scalar only where one can start + # (after whitespace or a flow indicator), so the apostrophe in `it's` does not. + defp strip_yaml_comment(line) do + line + |> String.graphemes() + |> Enum.reduce_while({[], nil, " ", false}, fn c, {acc, quote, prev, escaped} -> + cond do + quote == "\"" and escaped -> + {:cont, {[c | acc], quote, c, false}} + + quote == "\"" and c == "\\" -> + {:cont, {[c | acc], quote, c, true}} + + quote != nil and c == quote -> + {:cont, {[c | acc], nil, c, false}} + + quote != nil -> + {:cont, {[c | acc], quote, c, false}} + + c == "#" and String.trim(prev) == "" -> + {:halt, {acc, nil, c, false}} + + c in ["'", "\""] and (String.trim(prev) == "" or prev in ["{", "[", ","]) -> + {:cont, {[c | acc], c, c, false}} + + true -> + {:cont, {[c | acc], nil, c, false}} + end + end) + |> elem(0) + |> Enum.reverse() + |> Enum.join() + |> String.trim_trailing() + end + # GitHub resolves an action's owner/repo without regard to case # (Step-Security/Harden-Runner@v2 installs the same action); the `uses:` # key itself is case-sensitive YAML. defp harden_runner_uses?(line) do - Regex.match?(~r/^\s*(?:-\s+)?uses:\s*["']?(?i:step-security\/harden-runner)@/, line) + Regex.match?( + ~r/^\s*(?:-\s+)?(?:uses|"uses"|'uses')\s*:\s*["']?(?i:step-security\/harden-runner)@/, + line + ) end defp block_scalar?(line), do: Regex.match?(~r/:\s*[>|][0-9+-]*\s*(?:#.*)?$/, line) @@ -1000,7 +1233,7 @@ defmodule Hypatia.Rules.ResearchExtensions do {_in_jobs, _indent, groups} = Enum.reduce(lines, {false, nil, []}, fn {line, _} = entry, {in_jobs, indent, groups} -> cond do - Regex.match?(~r/^jobs:\s*(?:#.*)?$/, line) -> + Regex.match?(~r/^(?:jobs|"jobs"|'jobs')\s*:\s*(?:#.*)?$/, line) -> {true, nil, groups} not in_jobs -> diff --git a/test/deed_test.exs b/test/deed_test.exs index 2382eb3e..2484bb10 100644 --- a/test/deed_test.exs +++ b/test/deed_test.exs @@ -6,20 +6,26 @@ defmodule Hypatia.DeedTest do alias Hypatia.Deed + # ExUnit gives each test its own directory under tmp/, emptied as the test starts. + @moduletag :tmp_dir + @fixtures Path.expand("fixtures/deed_egress", __DIR__) - @tmp_dir System.tmp_dir!() - defp repo_with_deed(name, text) do - repo = Path.join(@tmp_dir, "deed_test_#{System.unique_integer([:positive])}") - # unique_integer restarts with each VM, so clear what an earlier run left. - File.rm_rf!(repo) + # A new repo under the test's tmp_dir; a test may make several. + defp new_repo(tmp) do + repo = Path.join(tmp, "repo_#{System.unique_integer([:positive])}") File.mkdir_p!(repo) + repo + end + + defp repo_with_deed(tmp, name, text) do + repo = new_repo(tmp) File.write!(Path.join(repo, name), text) repo end - defp repo_with_fixture(sub, name) do - repo_with_deed(name, File.read!(Path.join([@fixtures, sub, name]))) + defp repo_with_fixture(tmp, sub, name) do + repo_with_deed(tmp, name, File.read!(Path.join([@fixtures, sub, name]))) end describe "parse/1" do @@ -164,8 +170,8 @@ defmodule Hypatia.DeedTest do describe "egress_declarations/1 against the standards fixtures" do for name <- File.ls!(Path.expand("fixtures/deed_egress/valid", __DIR__)) do @name name - test "accepts valid/#{name}" do - repo = repo_with_fixture("valid", @name) + test "accepts valid/#{name}", %{tmp_dir: tmp} do + repo = repo_with_fixture(tmp, "valid", @name) assert {:ok, @name, jobs} = Deed.egress_declarations(repo) assert is_list(jobs) end @@ -173,24 +179,26 @@ defmodule Hypatia.DeedTest do for name <- File.ls!(Path.expand("fixtures/deed_egress/invalid", __DIR__)) do @name name - test "rejects invalid/#{name}" do - repo = repo_with_fixture("invalid", @name) + test "rejects invalid/#{name}", %{tmp_dir: tmp} do + repo = repo_with_fixture(tmp, "invalid", @name) assert {:error, @name, reason} = Deed.egress_declarations(repo) assert is_binary(reason) end end - test "takes :policy at most once, before or after the jobs" do + test "takes :policy at most once, before or after the jobs", %{tmp_dir: tmp} do hdr = ";; SPDX-License-Identifier: CC-BY-SA-4.0\n(repo-deed :schema-version \"1.0.0\" " job = ~s|(job :workflow ".github/workflows/a.yml" :job "a" | <> ~s|:secrets ("A") :endpoints () :enforcement none)| - repo = repo_with_deed("x_chora.deed", hdr <> ~s|(egress #{job} :policy "a"))|) + repo = repo_with_deed(tmp, "x_chora.deed", hdr <> ~s|(egress #{job} :policy "a"))|) assert {:ok, _, [_]} = Deed.egress_declarations(repo) - repo = repo_with_deed("x_chora.deed", hdr <> ~s|(egress :policy "a" :policy "b" #{job}))|) + repo = + repo_with_deed(tmp, "x_chora.deed", hdr <> ~s|(egress :policy "a" :policy "b" #{job}))|) + assert {:error, _, "(egress …) repeats :policy"} = Deed.egress_declarations(repo) end @@ -202,8 +210,8 @@ defmodule Hypatia.DeedTest do assert Enum.map(jobs, & &1.job) == ["notify", "sonarqube", "trigger-boj"] end - test "returns the declared jobs with their secrets" do - repo = repo_with_fixture("valid", "template-pair_chora.deed") + test "returns the declared jobs with their secrets", %{tmp_dir: tmp} do + repo = repo_with_fixture(tmp, "valid", "template-pair_chora.deed") assert {:ok, _, [notify, sonar]} = Deed.egress_declarations(repo) @@ -219,10 +227,8 @@ defmodule Hypatia.DeedTest do end describe "repo_deed_path/1" do - test "no deed, one deed, two deeds" do - repo = Path.join(@tmp_dir, "deed_none_#{System.unique_integer([:positive])}") - File.rm_rf!(repo) - File.mkdir_p!(repo) + test "no deed, one deed, two deeds", %{tmp_dir: tmp} do + repo = new_repo(tmp) assert Deed.repo_deed_path(repo) == :none assert Deed.egress_declarations(repo) == :none @@ -236,29 +242,27 @@ defmodule Hypatia.DeedTest do assert {:error, _} = Deed.repo_deed_path(repo) end - test "the stem is deed.abnf's: letters, digits, '-', '.', '_', at least one" do + test "the stem is deed.abnf's: letters, digits, '-', '.', '_', at least one", %{ + tmp_dir: tmp + } do for {name, ok?} <- [ {"my.project-x_chora.deed", true}, {"bad name_chora.deed", false}, {"_chora.deed", false} ] do - repo = Path.join(@tmp_dir, "deed_stem_#{System.unique_integer([:positive])}") - File.rm_rf!(repo) - File.mkdir_p!(repo) - File.write!(Path.join(repo, name), "") + repo = repo_with_deed(tmp, name, "") if ok?, do: assert({:ok, _} = Deed.repo_deed_path(repo), name), else: assert({:error, "illegal deed filename stem" <> _} = Deed.repo_deed_path(repo), name) - - File.rm_rf!(repo) end end - test "a _chora.deed must be a repo-deed" do + test "a _chora.deed must be a repo-deed", %{tmp_dir: tmp} do repo = repo_with_deed( + tmp, "x_chora.deed", ";; SPDX-License-Identifier: CC-BY-SA-4.0\n(praxis-deed :schema-version \"1\")\n" ) diff --git a/test/research_extensions_test.exs b/test/research_extensions_test.exs index 1bc8da76..bad21ccd 100644 --- a/test/research_extensions_test.exs +++ b/test/research_extensions_test.exs @@ -1042,6 +1042,277 @@ defmodule Hypatia.Rules.ResearchExtensionsTest do # harden-runner audits when no egress-policy is set. assert [%{reason: default}] = check.("") assert default =~ "runs `egress-policy: audit`" + + # Quoted inputs and quoted keys are the same input to YAML. + assert check.(" \"with\":\n 'egress-policy': \"block\"\n") == [] + + assert check.( + " with: {allowed-endpoints: \"a.com:443, b.com:443\", egress-policy: block}\n" + ) == [] + + # A YAML comment sets no input: these steps run the default, audit. + assert [_] = check.(" # with:\n # egress-policy: block\n") + assert [_] = check.(" with:\n disable-sudo: true # egress-policy: block\n") + assert [_] = check.(" with: {disable-sudo: true} # {egress-policy: block}\n") + + # Text inside another input's value is not the policy input. + assert [_] = check.(" with:\n note: \"x, egress-policy: block\"\n") + end + + test "a comment on the uses: line does not set the policy" do + repo = + create_repo_with_workflow( + """ + jobs: + notify: + runs-on: ubuntu-24.04 + steps: + - uses: step-security/harden-runner@v2 # egress-policy: block + - run: send --pass=${{ secrets.SMTP_PASS }} + """, + "notify.yml" + ) + + write_deed( + repo, + ~s|(egress (job :workflow ".github/workflows/notify.yml" :job "notify" | <> + ~s|:secrets ("SMTP_PASS") :endpoints () :enforcement harden-runner-block))| + ) + + assert [%{reason: reason}] = ResearchExtensions.re001_missing_harden_runner(repo) + assert reason =~ "runs `egress-policy: audit`" + File.rm_rf!(repo) + end + + test "a hardened job's declaration cannot cover a computed key" do + repo = + create_repo_with_workflow( + """ + jobs: + notify: + runs-on: ubuntu-24.04 + steps: + - uses: step-security/harden-runner@v2 + with: + egress-policy: block + - run: send --pass=${{ secrets.SMTP_PASS }} --x=${{ secrets[env.NAME] }} + """, + "notify.yml" + ) + + write_deed( + repo, + ~s|(egress (job :workflow ".github/workflows/notify.yml" :job "notify" | <> + ~s|:secrets ("SMTP_PASS") :endpoints () :enforcement harden-runner-block))| + ) + + assert [finding] = ResearchExtensions.re001_missing_harden_runner(repo) + assert finding.severity == :warn + assert finding.line == 8 + assert finding.reason =~ "computed at run time" + assert finding.reason =~ "does not cover it" + assert finding.detail.fix =~ "remove the job's entry from x_chora.deed" + File.rm_rf!(repo) + end + + test "quoted keys are the same keys: \"env\", 'runs-on', \"steps\", \"jobs\"" do + repo = + create_repo_with_workflow( + """ + "env": + HIDDEN: ${{ secrets.HIDDEN }} + "jobs": + notify: + 'runs-on': ubuntu-24.04 + "steps": + - run: send --pass=${{ secrets.SMTP_PASS }} + """, + "notify.yml" + ) + + # The job inherits HIDDEN from the quoted top-level env:, so a + # declaration of SMTP_PASS alone has drifted. + write_deed( + repo, + ~s|(egress (job :workflow ".github/workflows/notify.yml" :job "notify" | <> + ~s|:secrets ("SMTP_PASS") :endpoints () :enforcement none))| + ) + + assert [finding] = ResearchExtensions.re001_missing_harden_runner(repo) + assert finding.reason =~ "HIDDEN" + File.rm_rf!(repo) + end + + test "a quoted job-level 'env': holds the job's credentials" do + repo = + create_repo_with_workflow(""" + on: pull_request_target + jobs: + build: + runs-on: ubuntu-24.04 + 'env': + TOKEN: ${{ secrets.DEPLOY_TOKEN }} + steps: + - uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} + - run: make + """) + + assert [%{severity: :high}] = ResearchExtensions.re001_missing_harden_runner(repo) + File.rm_rf!(repo) + end + + test "quoted step keys still expose the credential: \"uses\", 'ref', \"run\"" do + for step <- [ + ~s| - "uses": actions/checkout@v4\n with:\n 'ref': ${{ github.head_ref }}\n - run: make|, + ~s| - uses: actions/checkout@v4\n - "run": npm install| + ] do + repo = + create_repo_with_workflow(""" + on: pull_request_target + jobs: + build: + runs-on: ubuntu-24.04 + env: + TOKEN: ${{ secrets.DEPLOY_TOKEN }} + steps: + #{step} + """) + + assert [%{severity: :high}] = ResearchExtensions.re001_missing_harden_runner(repo), step + File.rm_rf!(repo) + end + end + + test "an alias brings its anchor's secrets into the job" do + workflow = fn second_job -> + """ + jobs: + build: + runs-on: ubuntu-24.04 + env: &common + HIDDEN: ${{ secrets.HIDDEN }} + steps: + - run: make + #{second_job} + """ + end + + deploy_entry = + ~s|(job :workflow ".github/workflows/notify.yml" :job "deploy" | <> + ~s|:secrets ("VISIBLE") :endpoints () :enforcement none)| + + build_entry = + ~s|(job :workflow ".github/workflows/notify.yml" :job "build" | <> + ~s|:secrets ("HIDDEN") :endpoints () :enforcement none)| + + check = fn second_job -> + repo = create_repo_with_workflow(workflow.(second_job), "notify.yml") + write_deed(repo, "(egress #{build_entry} #{deploy_entry})") + findings = ResearchExtensions.re001_missing_harden_runner(repo) + File.rm_rf!(repo) + findings + end + + # env: *common + assert [%{line: 10, reason: plain}] = + check.(""" + deploy: + runs-on: ubuntu-24.04 + env: *common + steps: + - run: send ${{ secrets.VISIBLE }} + """) + + assert plain =~ "`deploy`" + assert plain =~ "HIDDEN" + + # A merge key, `<<: *common`, inside the job's own env:. + assert [%{reason: merged}] = + check.(""" + deploy: + runs-on: ubuntu-24.04 + env: + <<: *common + OTHER: x + steps: + - run: send ${{ secrets.VISIBLE }} + """) + + assert merged =~ "HIDDEN" + + # The same declaration, without the alias, is exact. + assert check.(""" + deploy: + runs-on: ubuntu-24.04 + steps: + - run: send ${{ secrets.VISIBLE }} + """) == [] + end + + test "a job that is an alias of another job is read as that job" do + repo = + create_repo_with_workflow( + """ + jobs: + build: &job + runs-on: ubuntu-24.04 + steps: + - run: send ${{ secrets.SMTP_PASS }} + deploy: *job + release: + <<: *job + name: release + """, + "notify.yml" + ) + + write_deed( + repo, + ~s|(egress (job :workflow ".github/workflows/notify.yml" :job "build" | <> + ~s|:secrets ("SMTP_PASS") :endpoints () :enforcement none))| + ) + + findings = ResearchExtensions.re001_missing_harden_runner(repo) + assert Enum.map(findings, & &1.line) == [6, 8] + assert Enum.all?(findings, &(&1.detail.fix =~ ~s|:secrets ("SMTP_PASS")|)) + assert Enum.any?(findings, &(&1.reason =~ "`deploy`")) + assert Enum.any?(findings, &(&1.reason =~ "`release`")) + File.rm_rf!(repo) + end + + test "nested anchors that multiply (the billion-laughs file) are expanded within a budget" do + anchors = + for i <- 1..9 do + prev = if i == 1, do: "${{ secrets.SMTP_PASS }}", else: "*a#{i - 1}" + " a#{i}: &a#{i} [#{Enum.map_join(1..10, ", ", fn _ -> prev end)}]" + end + + repo = + create_repo_with_workflow( + """ + on: push + x: + #{Enum.join(anchors, "\n")} + jobs: + notify: + runs-on: ubuntu-24.04 + env: + ALL: *a9 + steps: + - run: make + """, + "notify.yml" + ) + + {micros, findings} = + :timer.tc(fn -> ResearchExtensions.re001_missing_harden_runner(repo) end) + + assert [%{detail: %{fix: fix}}] = findings + assert fix =~ ~s|:secrets ("SMTP_PASS")| + assert micros < 10_000_000 + File.rm_rf!(repo) end test "a quoted job key matches its declaration" do From 05fc9a4cf8638380684fe118d40f7ce8547bdf2c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 27 Oct 2026 03:59:08 +0000 Subject: [PATCH 2/2] RE001: expand YAML aliases by node, not by text Codex review of c97ceb9: - A `<<:` merge no longer overrides a key the destination mapping sets itself. YAML gives the explicit key precedence, and the anchored entries used to land first: a job's own unhardened steps: could be read as the anchor's hardened ones, suppressing RE001. - Anchors and aliases are recognised only where a YAML node starts, outside quoted scalars and block-scalar text, and a `,` counts only inside a flow collection. `echo "x *common"`, `echo x *common` and a `run: |` line `ls *common` stay text. - An alias that is a key's whole value is replaced by its anchor's inline content, so `run: *script` reads as `run: |` with the script under it and step_run_lines sees the commands. `run: &script |` was also misread (the anchor hid the block header) and is fixed in step_run_lines and block_scalar?. research_extensions_test + deed_test: 164 tests, 0 failures; the 3 new tests fail against c97ceb9. kyaml_workflow_test 10/10. RE001 on real trees is unchanged: hypatia 9, standards 15, rsr-template-repo 0, choreographic-types 1. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01J4QvDithtn5Xk5MbSPm4Px --- CHANGELOG.adoc | 6 +- lib/rules/research_extensions.ex | 261 +++++++++++++++++++++++------- test/research_extensions_test.exs | 103 ++++++++++++ 3 files changed, 311 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index a8596062..05a79c9b 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -36,8 +36,10 @@ to declare egress instead of adopting harden-runner everywhere. RE001 now: `toJSON(secrets)`) is its own finding, since no declaration can list it; * reads the workflow as YAML does in two more ways: a quoted key (`"env":`, `'runs-on':`) is the same key, and an alias (`env: *common`, `<<: *defaults`, - `deploy: *build`) brings its anchor's lines into the job that uses it, within - a budget of copied lines that stops a "billion laughs" file; + `run: *script`, `deploy: *build`) brings its anchor's node into the job that + uses it, a merge never overriding a key the job sets itself, within a budget + of copied lines that stops a "billion laughs" file; an alias is read only + where a YAML node starts, never inside a quoted or block scalar; * keeps #966's exposure model as the severity layer: an undeclared job is `:high` when the credential shares a step with fetched or untrusted code, and otherwise `:warn`, with or without an exposure (owner ruling diff --git a/lib/rules/research_extensions.ex b/lib/rules/research_extensions.ex index 8434f06a..869ab5d6 100644 --- a/lib/rules/research_extensions.ex +++ b/lib/rules/research_extensions.ex @@ -419,32 +419,55 @@ defmodule Hypatia.Rules.ResearchExtensions do end # YAML anchors and aliases, which GitHub expands in workflows: `*name` - # repeats the node anchored `&name`, so `env: *common`, `<<: *defaults` or - # `deploy: *build` brings the anchored lines into the job that holds the - # alias. Each alias is expanded in place before any job is read: the - # anchored lines are copied in after it, re-indented and carrying the - # alias's line number, so a finding points at the alias. A `<<:` merge puts - # its lines into the mapping that holds the key, at the key's indent; any - # other alias's lines go at the column of its `*`, under the node it stands - # for. The anchor is the nearest one of that name above the alias. - @anchor ~r/(?:^|[\s\[{,])&([A-Za-z0-9_-]+)/ - @alias ~r/(?:^|[\s\[{,])\*([A-Za-z0-9_-]+)/ - @merge_key ~r/^\s*(?:-\s+)?(?:<<|"<<"|'<<')\s*:/ + # stands for the node anchored `&name`, so `env: *common`, `run: *script`, + # `<<: *defaults` or `deploy: *build` brings the anchored node into the job + # that holds the alias. Each alias is expanded in place before any job is + # read, and every line it brings carries the alias's line number, so a + # finding points at the alias: + # + # * an alias that is a key's whole value, or a list item, is replaced by + # what follows its anchor on the anchor's line (a scalar, a block + # scalar's `|`, a flow collection), and the anchor's indented lines + # follow at the column of the `*`; + # * a `<<:` merge puts the anchored mapping's entries into the mapping + # that holds the key, at the key's column, minus every entry whose key + # that mapping sets itself: YAML gives the explicit key precedence; + # * an alias inside a flow collection is followed by a copy of the + # anchored lines, deeper than the line. + # + # Anchors and aliases are recognised only where a node starts, outside + # quoted scalars and block-scalar text, so `echo "x *common"` and a `run: |` + # line `ls *common` stay text. The anchor is the nearest one of that name + # above the alias. + @anchor ~r/(?:^\s*(?:-\s+)*|:\s+|[\[{,]\s*)&([A-Za-z0-9_-]+)(?=\s|$)/ + @alias ~r/(?:^\s*(?:-\s+)*|:\s+|[\[{,]\s*)\*([A-Za-z0-9_-]+)\s*(?=$|[,\]}])/ + @merge_key ~r/^(\s*(?:-\s+)?)(?:<<|"<<"|'<<')\s*:/ + @mapping_key ~r/^\s*(?:-\s+)?(?:"([^"]*)"|'([^']*)'|([^\s"'#:][^:]*?))\s*:(?:\s|$)/ + @block_scalar_start ~r/(?:^\s*|[:-]\s+)(?:&[A-Za-z0-9_-]+\s+)?[>|][0-9+-]*\s*$/ + @block_scalar_header ~r/^[>|][0-9+-]*$/ # Nested anchors can multiply copies exponentially (the "billion laughs" # file), so expansion stops after this many copied lines per workflow. @alias_budget 5_000 defp expand_aliases(lines) do + codes = yaml_codes(lines) + anchors = - for {{line, n}, index} <- Enum.with_index(lines), - text = strip_yaml_comment(line), - [_, {at, len}] <- Regex.scan(@anchor, text, return: :index) do - {binary_part(text, at, len), n, anchored_lines(lines, index, text, at + len)} + for {{{line, n}, code}, index} <- lines |> Enum.zip(codes) |> Enum.with_index(), + code != nil, + [_, {at, len}] <- Regex.scan(@anchor, code, return: :index), + node_start?(code, at) do + {binary_part(code, at, len), n, + anchored(lines, index, strip_yaml_comment(line), at + len)} end # Only what jobs read is expanded in place: the jobs and the top-level # env:. An anchor kept elsewhere is expanded where a job uses it. {expanded, _} = - Enum.flat_map_reduce(lines, {nil, @alias_budget}, fn {line, _} = entry, {section, budget} -> + lines + |> Enum.zip(codes) + |> Enum.with_index() + |> Enum.flat_map_reduce({nil, @alias_budget}, fn {{{line, _} = entry, code}, index}, + {section, budget} -> section = case Regex.run(~r/^["']?([^\s"':]+)["']?\s*:/, line) do [_, key] -> key @@ -452,8 +475,8 @@ defmodule Hypatia.Rules.ResearchExtensions do end if section in ["jobs", "env"] do - {copies, budget} = expand_line(entry, anchors, budget) - {copies, {section, budget}} + {out, budget} = expand_line(entry, code, lines, index, anchors, budget) + {out, {section, budget}} else {[entry], {section, budget}} end @@ -463,55 +486,174 @@ defmodule Hypatia.Rules.ResearchExtensions do end defp expand_aliases(lines, anchors, budget) do - Enum.flat_map_reduce(lines, budget, &expand_line(&1, anchors, &2)) + lines + |> Enum.zip(yaml_codes(lines)) + |> Enum.with_index() + |> Enum.flat_map_reduce(budget, fn {{entry, code}, index}, budget -> + expand_line(entry, code, lines, index, anchors, budget) + end) end - # One line followed by copies of the anchors its aliases name. - defp expand_line(entry, _anchors, budget) when budget <= 0, do: {[entry], budget} + # One line with its aliases expanded. A line of block-scalar text (code nil) + # holds no alias. + defp expand_line(entry, nil, _lines, _index, _anchors, budget), do: {[entry], budget} - defp expand_line({line, n} = entry, anchors, budget) do - text = strip_yaml_comment(line) + defp expand_line(entry, _code, _lines, _index, _anchors, budget) when budget <= 0, + do: {[entry], budget} - {copies, budget} = - for [_, {at, len}] <- Regex.scan(@alias, text, return: :index), - name = binary_part(text, at, len), - {_, _, body} <- + defp expand_line({line, n} = entry, code, lines, index, anchors, budget) do + matches = + for [_, {at, len}] <- Regex.scan(@alias, code, return: :index), + node_start?(code, at), + name = binary_part(code, at, len), + {_, _, node} <- anchors |> Enum.filter(fn {a, m, _} -> a == name and m < n end) |> Enum.take(-1), - reduce: {[], budget} do - {copies, budget} when budget <= 0 -> - {copies, budget} - - {copies, budget} -> - target = - if Regex.match?(@merge_key, text), - do: indent_of(text), - else: String.length(binary_part(text, 0, at - 1)) - - {body, budget} = expand_aliases(body, anchors, budget - length(body)) - {copies ++ Enum.map(reindent(body, target), fn {copy, _} -> {copy, n} end), budget} + do: {at, len, node} + + # The alias that ends the line is the whole value of its key or item. + {value, in_flow} = + case List.last(matches) do + {at, len, _} = last -> + if String.trim(binary_part(code, at + len, byte_size(code) - at - len)) == "", + do: {last, Enum.drop(matches, -1)}, + else: {nil, matches} + + nil -> + {nil, []} end - {[entry | copies], budget} + {flow_copies, budget} = + Enum.flat_map_reduce(in_flow, budget, fn {at, _, {inline, block}}, budget -> + indent = indent_of(code) + inline_lines = if inline == "", do: [], else: [{indent_spaces(indent + 2) <> inline, n}] + {copy, budget} = anchored_block(inline, inline_lines ++ block, anchors, budget) + {renumber(reindent(copy, column(code, at - 1)), n), budget} + end) + + case value do + nil -> + {[entry | flow_copies], budget} + + {at, _, {inline, block}} -> + text = strip_yaml_comment(line) + head = String.trim_trailing(binary_part(text, 0, at - 1) <> inline) + {block, budget} = anchored_block(inline, block, anchors, budget - 1) + + block = + case Regex.run(@merge_key, code) do + [_, prefix] -> + merge_column = String.length(prefix) + + block + |> reindent(merge_column) + |> drop_entries(merge_column, sibling_keys(lines, index, merge_column)) + + nil -> + reindent(block, column(code, at - 1)) + end + + # The inline part may hold aliases of its own (`&a [*b, *c]`). + {head_lines, budget} = + if inline == "" or Regex.match?(@block_scalar_header, inline), + do: {[{head, n}], budget}, + else: expand_line({head, n}, yaml_code(head), [{head, n}], 0, anchors, budget) + + {head_lines ++ renumber(block, n) ++ flow_copies, budget} + end end - # The lines an anchor names: what follows it on its own line, then every - # line indented deeper than that line. - defp anchored_lines(lines, index, text, after_name) do - {_, n} = Enum.at(lines, index) - indent = indent_of(text) - inline = text |> binary_part(after_name, byte_size(text) - after_name) |> String.trim() + # An anchor's indented lines, their own aliases expanded. Under a block + # scalar's header they are text and are copied as they are. + defp anchored_block(inline, block, anchors, budget) do + if Regex.match?(@block_scalar_header, inline), + do: {block, budget - length(block)}, + else: expand_aliases(block, anchors, budget - length(block)) + end - inline_lines = - if inline == "", do: [], else: [{String.duplicate(" ", indent + 2) <> inline, n}] + # The node an anchor names: what follows the anchor on its own line, and + # every line indented deeper than that line. + defp anchored(lines, index, text, after_name) do + inline = text |> binary_part(after_name, byte_size(text) - after_name) |> String.trim() + indent = indent_of(text) block = lines |> Enum.drop(index + 1) |> Enum.take_while(fn {line, _} -> String.trim(line) == "" or indent_of(line) > indent end) - inline_lines ++ block + {inline, block} end + # The keys the mapping holding line `index` sets at `column` itself. + defp sibling_keys(lines, index, column) do + {before, [_ | after_line]} = Enum.split(lines, index) + + in_mapping = fn ls -> + Enum.take_while(ls, fn {line, _} -> String.trim(line) == "" or indent_of(line) >= column end) + end + + (in_mapping.(Enum.reverse(before)) ++ in_mapping.(after_line)) + |> Enum.filter(fn {line, _} -> indent_of(line) == column end) + |> Enum.flat_map(fn {line, _} -> mapping_key(line) end) + |> Enum.reject(&(&1 == "<<")) + end + + # Merged entries, less those whose key the destination mapping sets. + defp drop_entries(lines, column, keys) do + lines + |> Enum.chunk_while( + [], + fn {line, _} = entry, acc -> + if indent_of(line) == column and String.trim(line) != "" and acc != [], + do: {:cont, Enum.reverse(acc), [entry]}, + else: {:cont, [entry | acc]} + end, + fn + [] -> {:cont, []} + acc -> {:cont, Enum.reverse(acc), []} + end + ) + |> Enum.reject(fn [{first, _} | _] -> Enum.any?(mapping_key(first), &(&1 in keys)) end) + |> Enum.concat() + end + + defp mapping_key(line) do + case Regex.run(@mapping_key, strip_yaml_comment(line)) do + nil -> [] + captures -> captures |> tl() |> Enum.reject(&(&1 == "")) |> Enum.take(1) + end + end + + # Each line's YAML structure, for finding anchors and aliases: the line + # without its comment and with quoted scalars blanked, byte offsets kept; + # nil for a line of block-scalar text. + defp yaml_codes(lines) do + {codes, _} = + Enum.map_reduce(lines, nil, fn {line, _}, scalar_indent -> + if scalar_indent && (String.trim(line) == "" or indent_of(line) > scalar_indent) do + {nil, scalar_indent} + else + code = yaml_code(line) + {code, if(Regex.match?(@block_scalar_start, code), do: indent_of(line))} + end + end) + + codes + end + + defp yaml_code(line), do: strip_yaml_comment(line, true) + + # A `,` separates nodes only inside a flow collection; in a plain scalar + # (`run: echo a, *b`) it is text. The name starts at `at`, after `*` or `&`. + defp node_start?(code, at) do + before = code |> binary_part(0, at - 1) |> String.trim_trailing() + not String.ends_with?(before, ",") or nesting_at(code, at) > 0 + end + + defp column(text, byte_offset), do: String.length(binary_part(text, 0, byte_offset)) + defp indent_spaces(n), do: String.duplicate(" ", n) + defp renumber(lines, n), do: Enum.map(lines, fn {line, _} -> {line, n} end) + defp reindent(lines, target) do base = lines @@ -523,8 +665,7 @@ defmodule Hypatia.Rules.ResearchExtensions do Enum.map(lines, fn {line, n} -> if String.trim(line) == "", do: {"", n}, - else: - {String.duplicate(" ", indent_of(line) - base + target) <> String.trim_leading(line), n} + else: {indent_spaces(indent_of(line) - base + target) <> String.trim_leading(line), n} end) end @@ -798,8 +939,9 @@ defmodule Hypatia.Rules.ResearchExtensions do key_column = String.length(prefix) + # A block scalar, its header after any anchor (`run: &script |`). body = - if Regex.match?(~r/^[>|][0-9+-]*\s*(?:#.*)?$/, value) do + if Regex.match?(~r/^(?:&[A-Za-z0-9_-]+\s+)?[>|][0-9+-]*\s*(?:#.*)?$/, value) do rest |> Enum.take_while(fn {line, _} -> String.trim(line) == "" or indent_of(line) > key_column @@ -1182,22 +1324,26 @@ defmodule Hypatia.Rules.ResearchExtensions do # A line without its YAML comment: a `#` at the start or after whitespace, # outside a quoted scalar. A quote opens a scalar only where one can start # (after whitespace or a flow indicator), so the apostrophe in `it's` does not. - defp strip_yaml_comment(line) do + # With `blank_quoted`, each quoted scalar's text becomes spaces of the same + # byte length, so what remains is structure at its original offsets. + defp strip_yaml_comment(line, blank_quoted \\ false) do + text = fn c -> if blank_quoted, do: String.duplicate(" ", byte_size(c)), else: c end + line |> String.graphemes() |> Enum.reduce_while({[], nil, " ", false}, fn c, {acc, quote, prev, escaped} -> cond do quote == "\"" and escaped -> - {:cont, {[c | acc], quote, c, false}} + {:cont, {[text.(c) | acc], quote, c, false}} quote == "\"" and c == "\\" -> - {:cont, {[c | acc], quote, c, true}} + {:cont, {[text.(c) | acc], quote, c, true}} quote != nil and c == quote -> {:cont, {[c | acc], nil, c, false}} quote != nil -> - {:cont, {[c | acc], quote, c, false}} + {:cont, {[text.(c) | acc], quote, c, false}} c == "#" and String.trim(prev) == "" -> {:halt, {acc, nil, c, false}} @@ -1225,7 +1371,8 @@ defmodule Hypatia.Rules.ResearchExtensions do ) end - defp block_scalar?(line), do: Regex.match?(~r/:\s*[>|][0-9+-]*\s*(?:#.*)?$/, line) + defp block_scalar?(line), + do: Regex.match?(~r/:\s*(?:&[A-Za-z0-9_-]+\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. diff --git a/test/research_extensions_test.exs b/test/research_extensions_test.exs index bad21ccd..7845e5ea 100644 --- a/test/research_extensions_test.exs +++ b/test/research_extensions_test.exs @@ -1282,6 +1282,109 @@ defmodule Hypatia.Rules.ResearchExtensionsTest do File.rm_rf!(repo) end + test "a key the job sets itself wins over the same key from a merge" do + # The anchor's steps install harden-runner; the job's own steps do not. + repo = + create_repo_with_workflow( + """ + jobs: + build: &defaults + runs-on: ubuntu-24.04 + env: + HIDDEN: ${{ secrets.HIDDEN }} + steps: + - uses: step-security/harden-runner@v2 + with: + egress-policy: block + - run: make + deploy: + <<: *defaults + env: + OTHER: x + steps: + - run: send ${{ secrets.VISIBLE }} + """, + "notify.yml" + ) + + write_deed(repo, "") + + # deploy holds VISIBLE only (its env: replaces the merged one) and has + # no hardener (its steps: replace the merged ones). + assert [%{line: 16, detail: %{fix: fix}}] = + ResearchExtensions.re001_missing_harden_runner(repo) + + assert fix =~ ~s|:job "deploy" :secrets ("VISIBLE")| + File.rm_rf!(repo) + end + + test "alias-like text inside a scalar is text, not an alias" do + for step <- [ + ~s| - run: echo "x *common"|, + ~s| - run: echo x *common|, + ~s| - run: echo a, *common|, + ~s| - run: \|\n ls *common| + ] do + repo = + create_repo_with_workflow( + """ + jobs: + build: + runs-on: ubuntu-24.04 + env: &common + HIDDEN: ${{ secrets.HIDDEN }} + steps: + - run: make + deploy: + runs-on: ubuntu-24.04 + steps: + - run: send ${{ secrets.VISIBLE }} + #{step} + """, + "notify.yml" + ) + + write_deed( + repo, + ~s|(egress (job :workflow ".github/workflows/notify.yml" :job "build" | <> + ~s|:secrets ("HIDDEN") :endpoints () :enforcement none) | <> + ~s|(job :workflow ".github/workflows/notify.yml" :job "deploy" | <> + ~s|:secrets ("VISIBLE") :endpoints () :enforcement none))| + ) + + assert ResearchExtensions.re001_missing_harden_runner(repo) == [], step + File.rm_rf!(repo) + end + end + + test "a block-scalar alias keeps its script, and the script its exposure" do + repo = + create_repo_with_workflow( + """ + jobs: + build: + runs-on: ubuntu-24.04 + steps: + - run: &script | + npm install + deploy --token=${{ secrets.TOKEN }} + deploy: + runs-on: ubuntu-24.04 + steps: + - run: *script + """, + "notify.yml" + ) + + write_deed(repo, "") + + # npm install runs in the step that holds TOKEN, in both jobs. + assert [%{severity: :high, line: 7}, %{severity: :high, line: 11}] = + ResearchExtensions.re001_missing_harden_runner(repo) + + File.rm_rf!(repo) + end + test "nested anchors that multiply (the billion-laughs file) are expanded within a budget" do anchors = for i <- 1..9 do