From 184ac7108de7ac47542c8d5c3a5c088c2e2ef11b Mon Sep 17 00:00:00 2001 From: "Jonathan D.A. Jewell" <6759885+hyperpolymath@users.noreply.github.com> Date: Mon, 31 Aug 2026 21:15:58 +0100 Subject: [PATCH] fix(security): validate actions lock integrity --- .github/workflows/actions.lock | 4 +- lib/rules/actions_lock.ex | 491 +++++++++++++++++++++++++++++++ lib/rules/rules.ex | 34 ++- lib/rules/workflow_audit.ex | 123 ++++---- lib/rules/workflow_hardening.ex | 36 +++ test/rules/actions_lock_test.exs | 295 +++++++++++++++++++ test/workflow_audit_test.exs | 49 ++- test/workflow_hardening_test.exs | 71 +++++ 8 files changed, 1025 insertions(+), 78 deletions(-) create mode 100644 lib/rules/actions_lock.ex create mode 100644 test/rules/actions_lock_test.exs diff --git a/.github/workflows/actions.lock b/.github/workflows/actions.lock index 793349be..ea759d18 100644 --- a/.github/workflows/actions.lock +++ b/.github/workflows/actions.lock @@ -302,12 +302,12 @@ dependencies: repo_id: 623796603 'hyperpolymath/a2ml-ecosystem@main': ref: 'main' - commit: 'sha1-c73022e759ed52a6156768a2fde43636ae4d7a7c' + commit: 'sha1-c992d2882ee1e62bf5c78b5f9a1893a6a16730e4' owner_id: 6759885 repo_id: 1275649586 'hyperpolymath/k9-ecosystem@main': ref: 'main' - commit: 'sha1-c74f04c77a36247a813493945e803178b93d170f' + commit: 'sha1-3f250fba42e432c7ff47b48f59525bec3357136b' owner_id: 6759885 repo_id: 1275650185 'returntocorp/semgrep-action@v1': diff --git a/lib/rules/actions_lock.ex b/lib/rules/actions_lock.ex new file mode 100644 index 00000000..fce24544 --- /dev/null +++ b/lib/rules/actions_lock.ex @@ -0,0 +1,491 @@ +# SPDX-License-Identifier: MPL-2.0 +# Copyright (c) 2026 Jonathan D.A. Jewell (hyperpolymath) + +defmodule Hypatia.Rules.ActionsLock do + @moduledoc """ + Strict local validation for machine-generated `gh actions-lock` v0.0.2 files. + + This module validates lockfile *shape and association*. It does not replace + `gh actions-lock --verify`, which remains authoritative for resolving refs + against GitHub. A symbolic action is accepted locally only when the complete + lock is valid, the current workflow lists that exact normalized ref, and the + matching dependency carries a canonical `sha1-<40 hex>` commit. + + The parser is deliberately line-oriented. `actions.lock` is generated by a + single tool with a versioned stable format, and Hypatia must not add a general + YAML dependency merely to consume that format. Unknown or malformed v0.0.2 + lines fail closed rather than being guessed at. + """ + + @supported_version "v0.0.2" + @action_ref ~r/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.\/-]+@[^\s':]+$/ + @dependency_key ~r/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+@[^\s':]+$/ + @sha_ref ~r/^[0-9A-Fa-f]{40}$/ + + defstruct version: nil, workflows: %{}, dependencies: %{} + + @type dependency :: %{ + ref: String.t(), + commit: String.t(), + owner_id: pos_integer(), + repo_id: pos_integer(), + uses: MapSet.t(String.t()) + } + @type t :: %__MODULE__{ + version: String.t(), + workflows: %{String.t() => MapSet.t(String.t())}, + dependencies: %{String.t() => dependency()} + } + + @doc """ + Parse and validate a complete v0.0.2 lockfile. + + Returns `{:error, reason}` for missing, unsupported, malformed, incomplete, + duplicate, or contradictory input. One bad dependency invalidates the whole + lock so callers cannot silently accept the remaining entries. + """ + @spec parse(term()) :: {:ok, t()} | {:error, term()} + def parse(content) when is_binary(content) and byte_size(content) > 0 do + initial = %{ + section: :preamble, + version: nil, + workflows: %{}, + current_workflow: nil, + dependencies: %{}, + current_dependency: nil + } + + content + |> String.split("\n") + |> Enum.with_index(1) + |> Enum.reduce_while({:ok, initial}, fn {raw_line, line_number}, {:ok, state} -> + line = String.trim_trailing(raw_line, "\r") + + case consume_line(line, line_number, state) do + {:ok, next} -> {:cont, {:ok, next}} + {:error, _reason} = error -> {:halt, error} + end + end) + |> finish() + end + + def parse(_), do: {:error, :missing_or_nonbinary_lock} + + @doc """ + Whether `slug` is validly locked for `workflow_path`. + + A lock entry for another workflow never authorizes the current workflow. + Basenames are resolved under `.github/workflows/` for compatibility with the + workflow-audit caller, which loads files by basename. + """ + @spec pinned?(t() | nil, String.t(), String.t()) :: boolean() + def pinned?(%__MODULE__{} = lock, workflow_path, slug) + when is_binary(workflow_path) and is_binary(slug) do + path = normalise_workflow_path(workflow_path) + ref = normalise_ref(slug) + + lock.workflows + |> Map.get(path, MapSet.new()) + |> MapSet.member?(ref) and Map.has_key?(lock.dependencies, ref) + end + + def pinned?(_, _, _), do: false + + @doc """ + Return dependency refs from a fully validated lock. + + This exists only for compatibility with the former global `locked_refs/1` + API. Security decisions must use `pinned?/3`, which also checks workflow + association. + """ + @spec dependency_refs(t()) :: MapSet.t(String.t()) + def dependency_refs(%__MODULE__{} = lock) do + lock.dependencies |> Map.keys() |> MapSet.new() + end + + @doc """ + Normalize an action slug to the root `owner/repository@ref` lock key. + """ + @spec normalise_ref(String.t()) :: String.t() + def normalise_ref(slug) when is_binary(slug) do + case String.split(slug, "@", parts: 2) do + [path, ref] -> + owner_repo = path |> String.split("/") |> Enum.take(2) |> Enum.join("/") + String.downcase(owner_repo) <> "@" <> ref + + _ -> + String.downcase(slug) + end + end + + defp normalise_workflow_path(path) do + normalized = String.replace(path, "\\", "/") + + if String.starts_with?(normalized, ".github/workflows/") do + normalized + else + ".github/workflows/" <> Path.basename(normalized) + end + end + + defp consume_line(line, _line_number, state) when line == "", do: {:ok, state} + + defp consume_line("#" <> _comment, _line_number, state), do: {:ok, state} + + defp consume_line(line, line_number, %{section: :preamble} = state) do + cond do + match = Regex.run(~r/^version: '([^']+)'$/, line) -> + case match do + [_, @supported_version] when is_nil(state.version) -> + {:ok, %{state | version: @supported_version}} + + [_, @supported_version] -> + line_error(line_number, :duplicate_version) + + [_, version] -> + line_error(line_number, {:unsupported_version, version}) + end + + line == "workflows:" and state.version == @supported_version -> + {:ok, %{state | section: :workflows}} + + true -> + line_error(line_number, :expected_version_or_workflows) + end + end + + defp consume_line(line, line_number, %{section: :workflows} = state) do + cond do + line == "dependencies:" -> + {:ok, %{state | section: :dependencies, current_workflow: nil}} + + match = Regex.run(~r/^ '(\.github\/workflows\/[^']+\.(?:yml|yaml))': \[\]$/, line) -> + [_, path] = match + put_workflow(state, path, false, line_number) + + match = Regex.run(~r/^ '(\.github\/workflows\/[^']+\.(?:yml|yaml))':$/, line) -> + [_, path] = match + put_workflow(state, path, true, line_number) + + match = Regex.run(~r/^ - '([^']+)'$/, line) -> + [_, ref] = match + put_workflow_ref(state, ref, line_number) + + true -> + line_error(line_number, :malformed_workflow_entry) + end + end + + defp consume_line(line, line_number, %{section: :dependencies} = state) do + cond do + match = Regex.run(~r/^ '([^']+)':$/, line) -> + [_, key] = match + + with {:ok, finalized} <- finalize_dependency(state) do + if Regex.match?(@dependency_key, key) do + {:ok, %{finalized | current_dependency: {key, %{}, line_number}}} + else + line_error(line_number, {:malformed_dependency_key, key}) + end + end + + match = Regex.run(~r/^ ref: '([^']+)'$/, line) -> + [_, ref] = match + put_dependency_field(state, :ref, ref, line_number) + + match = Regex.run(~r/^ commit: 'sha1-([0-9A-Fa-f]{40})'$/, line) -> + [_, commit] = match + put_dependency_field(state, :commit, String.downcase(commit), line_number) + + Regex.match?(~r/^ commit:/, line) -> + line_error(line_number, :malformed_dependency_commit) + + match = Regex.run(~r/^ (owner_id|repo_id): ([0-9]+)$/, line) -> + [_, field, value] = match + parsed = String.to_integer(value) + + if parsed > 0 do + put_dependency_field(state, identity_field(field), parsed, line_number) + else + line_error(line_number, {:nonpositive_dependency_identity, field}) + end + + Regex.match?(~r/^ (?:owner_id|repo_id):/, line) -> + line_error(line_number, :malformed_dependency_identity) + + line == " uses:" -> + put_dependency_field(state, :uses, MapSet.new(), line_number) + + match = Regex.run(~r/^ - '([^']+)'$/, line) -> + [_, ref] = match + + if Regex.match?(@action_ref, ref) do + put_transitive_ref(state, ref, line_number) + else + line_error(line_number, {:malformed_transitive_ref, ref}) + end + + true -> + line_error(line_number, :malformed_dependency_entry) + end + end + + defp put_workflow(state, path, has_items, line_number) do + if Map.has_key?(state.workflows, path) do + line_error(line_number, {:duplicate_workflow, path}) + else + current = if has_items, do: path, else: nil + + {:ok, + %{ + state + | workflows: Map.put(state.workflows, path, MapSet.new()), + current_workflow: current + }} + end + end + + defp put_workflow_ref(%{current_workflow: nil}, _ref, line_number) do + line_error(line_number, :workflow_ref_without_workflow) + end + + defp put_workflow_ref(state, ref, line_number) do + if Regex.match?(@action_ref, ref) do + normalized = normalise_ref(ref) + refs = Map.fetch!(state.workflows, state.current_workflow) + + if MapSet.member?(refs, normalized) do + line_error(line_number, {:duplicate_workflow_ref, state.current_workflow, ref}) + else + workflows = Map.put(state.workflows, state.current_workflow, MapSet.put(refs, normalized)) + {:ok, %{state | workflows: workflows}} + end + else + line_error(line_number, {:malformed_workflow_ref, ref}) + end + end + + defp put_dependency_field(%{current_dependency: nil}, _field, _value, line_number) do + line_error(line_number, :dependency_field_without_dependency) + end + + defp put_dependency_field(state, field, value, line_number) do + {key, dependency, header_line} = state.current_dependency + + if Map.has_key?(dependency, field) do + line_error(line_number, {:duplicate_dependency_field, key, field}) + else + {:ok, %{state | current_dependency: {key, Map.put(dependency, field, value), header_line}}} + end + end + + defp identity_field("owner_id"), do: :owner_id + defp identity_field("repo_id"), do: :repo_id + + defp put_transitive_ref(%{current_dependency: nil}, _ref, line_number) do + line_error(line_number, :dependency_detail_without_dependency) + end + + defp put_transitive_ref(state, ref, line_number) do + {key, dependency, header_line} = state.current_dependency + + case Map.fetch(dependency, :uses) do + :error -> + line_error(line_number, {:transitive_ref_without_uses, key}) + + {:ok, uses} -> + normalized = normalise_ref(ref) + + if MapSet.member?(uses, normalized) do + line_error(line_number, {:duplicate_transitive_ref, key, ref}) + else + updated = Map.put(dependency, :uses, MapSet.put(uses, normalized)) + {:ok, %{state | current_dependency: {key, updated, header_line}}} + end + end + end + + defp finalize_dependency(%{current_dependency: nil} = state), do: {:ok, state} + + defp finalize_dependency(state) do + {key, dependency, header_line} = state.current_dependency + normalized = normalise_ref(key) + + with {:ok, ref} <- fetch_dependency_field(dependency, :ref, key, header_line), + {:ok, commit} <- fetch_dependency_field(dependency, :commit, key, header_line), + {:ok, owner_id} <- fetch_dependency_field(dependency, :owner_id, key, header_line), + {:ok, repo_id} <- fetch_dependency_field(dependency, :repo_id, key, header_line), + :ok <- validate_dependency_consistency(key, ref, commit, header_line), + :ok <- + validate_repository_identity( + state.dependencies, + normalized, + owner_id, + repo_id, + header_line + ), + false <- Map.has_key?(state.dependencies, normalized) do + stored = %{ + ref: ref, + commit: commit, + owner_id: owner_id, + repo_id: repo_id, + uses: Map.get(dependency, :uses, MapSet.new()) + } + + {:ok, + %{ + state + | dependencies: Map.put(state.dependencies, normalized, stored), + current_dependency: nil + }} + else + true -> line_error(header_line, {:duplicate_dependency, key}) + {:error, _reason} = error -> error + end + end + + defp validate_repository_identity(dependencies, normalized, owner_id, repo_id, line_number) do + [repository, _ref] = String.split(normalized, "@", parts: 2) + [owner, _repo] = String.split(repository, "/", parts: 2) + + with :ok <- validate_owner_id(dependencies, owner, owner_id, line_number), + :ok <- validate_repo_id(dependencies, repository, repo_id, line_number), + :ok <- validate_repo_id_uniqueness(dependencies, repository, repo_id, line_number) do + :ok + end + end + + defp validate_owner_id(dependencies, owner, owner_id, line_number) do + existing = + Enum.find(dependencies, fn {dependency_ref, _dependency} -> + [repository, _ref] = String.split(dependency_ref, "@", parts: 2) + [dependency_owner, _repo] = String.split(repository, "/", parts: 2) + dependency_owner == owner + end) + + case existing do + nil -> + :ok + + {_ref, %{owner_id: ^owner_id}} -> + :ok + + {_ref, dependency} -> + line_error( + line_number, + {:owner_identity_mismatch, owner, dependency.owner_id, owner_id} + ) + end + end + + defp validate_repo_id(dependencies, repository, repo_id, line_number) do + existing = + Enum.find(dependencies, fn {dependency_ref, _dependency} -> + [dependency_repository, _ref] = String.split(dependency_ref, "@", parts: 2) + dependency_repository == repository + end) + + case existing do + nil -> + :ok + + {_ref, %{repo_id: ^repo_id}} -> + :ok + + {_ref, dependency} -> + line_error( + line_number, + {:repository_identity_mismatch, repository, dependency.repo_id, repo_id} + ) + end + end + + defp validate_repo_id_uniqueness(dependencies, repository, repo_id, line_number) do + existing = + Enum.find(dependencies, fn {dependency_ref, dependency} -> + [dependency_repository, _ref] = String.split(dependency_ref, "@", parts: 2) + dependency.repo_id == repo_id and dependency_repository != repository + end) + + case existing do + nil -> + :ok + + {dependency_ref, _dependency} -> + [other_repository, _ref] = String.split(dependency_ref, "@", parts: 2) + + line_error( + line_number, + {:repository_id_reused, repo_id, other_repository, repository} + ) + end + end + + defp fetch_dependency_field(dependency, field, key, line_number) do + case Map.fetch(dependency, field) do + {:ok, value} -> {:ok, value} + :error -> line_error(line_number, {:missing_dependency_field, key, field}) + end + end + + defp validate_dependency_consistency(key, declared_ref, commit, line_number) do + [_path, key_ref] = String.split(key, "@", parts: 2) + + cond do + Regex.match?(@sha_ref, key_ref) and String.downcase(key_ref) != commit -> + line_error(line_number, {:sha_key_commit_mismatch, key}) + + not Regex.match?(@sha_ref, key_ref) and key_ref != declared_ref -> + line_error(line_number, {:symbolic_key_ref_mismatch, key, declared_ref}) + + true -> + :ok + end + end + + defp finish({:error, _reason} = error), do: error + + defp finish({:ok, %{section: :dependencies} = state}) do + with {:ok, finalized} <- finalize_dependency(state), + :ok <- validate_workflow_dependencies(finalized), + :ok <- validate_transitive_dependencies(finalized) do + {:ok, + %__MODULE__{ + version: finalized.version, + workflows: finalized.workflows, + dependencies: finalized.dependencies + }} + end + end + + defp finish({:ok, _state}), do: {:error, :incomplete_lockfile} + + defp validate_transitive_dependencies(state) do + missing = + for {source, dependency} <- state.dependencies, + target <- dependency.uses, + not Map.has_key?(state.dependencies, target), + do: {source, target} + + case missing do + [] -> :ok + entries -> {:error, {:transitive_dependencies_missing, Enum.sort(entries)}} + end + end + + defp validate_workflow_dependencies(state) do + missing = + for {path, refs} <- state.workflows, + ref <- refs, + not Map.has_key?(state.dependencies, ref), + do: {path, ref} + + case missing do + [] -> :ok + entries -> {:error, {:workflow_dependencies_missing, Enum.sort(entries)}} + end + end + + defp line_error(line_number, reason), do: {:error, {:line, line_number, reason}} +end diff --git a/lib/rules/rules.ex b/lib/rules/rules.ex index 6f5dfe5f..a876783f 100644 --- a/lib/rules/rules.ex +++ b/lib/rules/rules.ex @@ -571,27 +571,35 @@ defmodule Hypatia.Rules do Check a workflow YAML file for common issues. Returns a list of findings. """ - def scan_workflow(content) do - findings = [] - - # Unpinned actions - unpinned = Regex.scan(~r/uses:\s*([^\s]+@v\d+)/, content) + def scan_workflow(content, opts \\ []) do + filename = Keyword.get(opts, :filename, "workflow.yml") + audit_opts = Keyword.take(opts, [:actions_lock]) findings = - findings ++ - Enum.map(unpinned, fn [_full, action_ref] -> + %{filename => content} + |> Hypatia.Rules.WorkflowAudit.check_unpinned_actions(audit_opts) + |> Enum.flat_map(fn + %{type: :unpinned_action, action_ref: action_ref} -> suggestion = case SecurityErrors.pin_action(action_ref) do {:ok, pinned} -> " -- fix: #{pinned}" _ -> "" end - %{ - rule: "unpinned_action", - severity: :high, - description: "Unpinned action: #{action_ref}#{suggestion}" - } - end) + [ + %{ + rule: "unpinned_action", + severity: :high, + description: "Unpinned action: #{action_ref}#{suggestion}" + } + ] + + %{type: :invalid_actions_lock, detail: detail} -> + [%{rule: "invalid_actions_lock", severity: :high, description: detail}] + + _accepted_or_unrelated -> + [] + end) # Missing permissions findings = diff --git a/lib/rules/workflow_audit.ex b/lib/rules/workflow_audit.ex index 13de1f77..d31669dd 100644 --- a/lib/rules/workflow_audit.ex +++ b/lib/rules/workflow_audit.ex @@ -269,12 +269,25 @@ defmodule Hypatia.Rules.WorkflowAudit do hints from the `@known_good_shas` table. """ def check_unpinned_actions(workflow_contents, opts \\ []) do - locked = locked_refs(Keyword.get(opts, :actions_lock)) + case parse_actions_lock(Keyword.get(opts, :actions_lock)) do + {:invalid, finding} -> + # A present-but-invalid lock is one indivisible integrity failure. Do + # not also recommend inline SHA pins: `gh actions-lock` deliberately + # owns those symbolic refs, and replacing them inline can remove their + # transitive dependency and repository-identity coverage. + [finding] + + {:usable, lock} -> + scan_unpinned_actions(workflow_contents, lock) + end + end + defp scan_unpinned_actions(workflow_contents, lock) do Enum.flat_map(workflow_contents, fn {filename, content} -> Hypatia.Rules.WorkflowHardening.wh004_scan_content(filename, content) - |> Enum.map(fn finding -> + |> Enum.flat_map(fn finding -> slug = finding.detail.uses + # Reconstruct action_ref + ref from the canonical "slug" # (e.g. "owner/repo@vN.N.N") emitted by WH004. [action_ref, ref] = @@ -283,8 +296,6 @@ defmodule Hypatia.Rules.WorkflowAudit do [_a] -> [slug, ""] end - _ = action_ref - cond do # A repository under GitHub Actions lockfile enforcement pins its # actions in `.github/workflows/actions.lock`, NOT inline. The @@ -300,64 +311,52 @@ defmodule Hypatia.Rules.WorkflowAudit do # that regression. Established 2026-08-07 on this repository: # inline-pinning 40 refs put 14 workflows into startup_failure # and removed dtolnay/rust-toolchain from 7 lockfile entries. - MapSet.member?(locked, normalise_ref(slug)) -> - %{ - type: :lockfile_pinned_accepted, - file: filename, - action_ref: slug, - severity: :info, - action: :accept_with_rationale, - rationale: - "Pinned by .github/workflows/actions.lock, which resolves " <> - "this symbolic ref to a verified commit and locks the " <> - "transitive dependencies of composite actions. Inline " <> - "SHA-pinning would remove it from the lockfile." - } + Hypatia.Rules.ActionsLock.pinned?(lock, filename, slug) -> + [] Hypatia.Rules.SecurityErrors.pin_exempt?(slug) -> - %{ - type: :pin_exempt_accepted, - file: filename, - action_ref: slug, - severity: :info, - action: :accept_with_rationale, - rationale: Hypatia.Rules.SecurityErrors.pin_exemption_reason(slug) - } + [ + %{ + type: :pin_exempt_accepted, + file: filename, + action_ref: slug, + severity: :info, + action: :accept_with_rationale, + rationale: Hypatia.Rules.SecurityErrors.pin_exemption_reason(slug) + } + ] true -> severity = if ref in ["main", "master"], do: :high, else: :medium - %{ - type: :unpinned_action, - file: filename, - action_ref: slug, - severity: severity, - action: :pin_sha, - known_sha: Map.get(Hypatia.Rules.SecurityErrors.sha_pins(), action_ref) - } + [ + %{ + type: :unpinned_action, + file: filename, + action_ref: slug, + severity: severity, + action: :pin_sha, + known_sha: Map.get(Hypatia.Rules.SecurityErrors.sha_pins(), action_ref) + } + ] end end) end) end @doc """ - Extract the set of action refs pinned by a `gh actions-lock` lockfile. - - The lockfile is machine-generated with a stable shape: refs appear as - quoted `'owner/repo@ref'` tokens, both as `workflows:` list entries and - as `dependencies:` keys. Collecting every such token covers both, and - covers transitive composite dependencies for free. + Compatibility accessor for dependency refs in a fully valid lockfile. - Returns an empty set for `nil` or unparseable input, so a repository - without lockfile enforcement keeps the previous behaviour exactly. + Do not use this global set for authorization: only + `Hypatia.Rules.ActionsLock.pinned?/3` also proves workflow association. """ def locked_refs(nil), do: MapSet.new() def locked_refs(lock_content) when is_binary(lock_content) do - ~r/'([A-Za-z0-9_.-]+\/[A-Za-z0-9_.\/-]+@[^']+)'/ - |> Regex.scan(lock_content) - |> Enum.map(fn [_, ref] -> normalise_ref(ref) end) - |> MapSet.new() + case Hypatia.Rules.ActionsLock.parse(lock_content) do + {:ok, lock} -> Hypatia.Rules.ActionsLock.dependency_refs(lock) + {:error, _reason} -> MapSet.new() + end end def locked_refs(_), do: MapSet.new() @@ -369,13 +368,27 @@ defmodule Hypatia.Rules.WorkflowAudit do `gh actions-lock` records them. """ def normalise_ref(slug) do - case String.split(slug, "@", parts: 2) do - [path, ref] -> - owner_repo = path |> String.split("/") |> Enum.take(2) |> Enum.join("/") - String.downcase(owner_repo <> "@" <> ref) + Hypatia.Rules.ActionsLock.normalise_ref(slug) + end - _ -> - String.downcase(slug) + defp parse_actions_lock(nil), do: {:usable, nil} + + defp parse_actions_lock(content) do + case Hypatia.Rules.ActionsLock.parse(content) do + {:ok, lock} -> + {:usable, lock} + + {:error, reason} -> + {:invalid, + %{ + type: :invalid_actions_lock, + file: "actions.lock", + severity: :high, + action: :regenerate, + detail: + "Invalid .github/workflows/actions.lock: #{inspect(reason)}. " <> + "Regenerate and verify it with gh actions-lock." + }} end end @@ -789,15 +802,15 @@ defmodule Hypatia.Rules.WorkflowAudit do # itself contains a `run:` step. We need to check per-job, not per-file. # Split content by job boundaries (job names start at column 0 or 2 spaces) # and find which job has the scorecard action with publish_results. - + # Find all job sections and check if the scorecard+publish job has run steps job_sections = Regex.split(~r/\n(?:^| )[a-zA-Z_][a-zA-Z0-9_-]*:\s*\n/m, stripped) - - scorecard_job_has_run = + + scorecard_job_has_run = Enum.any?(job_sections, fn section -> scorecard_in_job? = Regex.match?(~r/uses:\s*ossf\/scorecard-action@/, section) publish_in_job? = Regex.match?(~r/publish_results:\s*true/, section) - + if scorecard_in_job? and publish_in_job? do # Check if this same job section has a run: step Regex.match?(~r/^\s+- name:[^\n]*\n\s+run:/m, section) or diff --git a/lib/rules/workflow_hardening.ex b/lib/rules/workflow_hardening.ex index 2ce73695..601f568b 100644 --- a/lib/rules/workflow_hardening.ex +++ b/lib/rules/workflow_hardening.ex @@ -288,6 +288,24 @@ defmodule Hypatia.Rules.WorkflowHardening do release can substitute the implementation. """ def wh004_unpinned_uses(repo_path) do + lock_path = Path.join([repo_path, ".github", "workflows", "actions.lock"]) + + case File.read(lock_path) do + {:error, :enoent} -> + wh004_scan_repo(repo_path, nil) + + {:error, reason} -> + [invalid_actions_lock_finding(reason)] + + {:ok, content} -> + case Hypatia.Rules.ActionsLock.parse(content) do + {:ok, lock} -> wh004_scan_repo(repo_path, lock) + {:error, reason} -> [invalid_actions_lock_finding(reason)] + end + end + end + + defp wh004_scan_repo(repo_path, lock) do # Path-walking wrapper: enumerate workflow files and delegate per-file # scanning to `wh004_scan_content/2` so the same detection logic is # callable both from a repo-path walker (this function) and from a @@ -299,10 +317,28 @@ defmodule Hypatia.Rules.WorkflowHardening do |> Enum.flat_map(fn path -> content = File.read!(path) rel = Path.relative_to(path, repo_path) + wh004_scan_content(rel, content) + |> Enum.reject(fn finding -> + Hypatia.Rules.ActionsLock.pinned?(lock, rel, finding.detail.uses) + end) end) end + defp invalid_actions_lock_finding(reason) do + %{ + rule: "invalid_actions_lock", + file: ".github/workflows/actions.lock", + severity: :high, + reason: "actions.lock failed closed: #{inspect(reason)}", + action: :regenerate, + detail: %{ + kind: :invalid_actions_lock, + fix: "Regenerate and verify the lock with `gh actions-lock`." + } + } + end + @doc """ WH004 core scanner: detect unpinned `uses:` references in a single workflow file's content. Returns the canonical WH004 finding shape diff --git a/test/rules/actions_lock_test.exs b/test/rules/actions_lock_test.exs new file mode 100644 index 00000000..e7e9c9eb --- /dev/null +++ b/test/rules/actions_lock_test.exs @@ -0,0 +1,295 @@ +# SPDX-License-Identifier: MPL-2.0 + +defmodule Hypatia.Rules.ActionsLockTest do + use ExUnit.Case, async: true + + alias Hypatia.Rules.ActionsLock + alias Hypatia.Rules + + @valid_lock """ + # This file is machine-generated by `gh actions-lock`. + version: 'v0.0.2' + workflows: + '.github/workflows/ci.yml': + - 'actions/checkout@v7.0.1' + '.github/workflows/other.yml': [] + dependencies: + 'actions/checkout@v7.0.1': + ref: 'v7.0.1' + commit: 'sha1-3d3c42e5aac5ba805825da76410c181273ba90b1' + owner_id: 44036562 + repo_id: 197814629 + """ + + @workflow """ + permissions: + contents: read + jobs: + test: + steps: + - uses: actions/checkout@v7.0.1 + """ + + test "valid lock pins only the associated workflow and exact ref" do + assert {:ok, lock} = ActionsLock.parse(@valid_lock) + assert ActionsLock.pinned?(lock, "ci.yml", "actions/checkout@v7.0.1") + assert ActionsLock.pinned?(lock, ".github/workflows/ci.yml", "Actions/Checkout@v7.0.1") + + refute ActionsLock.pinned?(lock, "other.yml", "actions/checkout@v7.0.1") + refute ActionsLock.pinned?(lock, "ci.yml", "actions/checkout@v6.0.0") + end + + test "accepts uppercase hexadecimal commit suffixes" do + lock = + String.replace( + @valid_lock, + "3d3c42e5aac5ba805825da76410c181273ba90b1", + "3D3C42E5AAC5BA805825DA76410C181273BA90B1" + ) + + assert {:ok, parsed} = ActionsLock.parse(lock) + assert ActionsLock.pinned?(parsed, "ci.yml", "actions/checkout@v7.0.1") + end + + test "normalises repository identity but preserves case-sensitive refs" do + lock = String.replace(@valid_lock, "v7.0.1", "ReleaseV7") + + assert {:ok, parsed} = ActionsLock.parse(lock) + assert ActionsLock.pinned?(parsed, "ci.yml", "Actions/Checkout@ReleaseV7") + refute ActionsLock.pinned?(parsed, "ci.yml", "actions/checkout@releasev7") + end + + test "rejects missing, short, and non-hex commit fields" do + missing = Regex.replace(~r/^\s*commit:.*\n/m, @valid_lock, "") + + short = + String.replace( + @valid_lock, + "sha1-3d3c42e5aac5ba805825da76410c181273ba90b1", + "sha1-1234" + ) + + non_hex = + String.replace( + @valid_lock, + "sha1-3d3c42e5aac5ba805825da76410c181273ba90b1", + "sha1-zd3c42e5aac5ba805825da76410c181273ba90b1" + ) + + assert {:error, {:line, _, {:missing_dependency_field, _, :commit}}} = + ActionsLock.parse(missing) + + assert {:error, {:line, _, :malformed_dependency_commit}} = ActionsLock.parse(short) + assert {:error, {:line, _, :malformed_dependency_commit}} = ActionsLock.parse(non_hex) + end + + test "rejects unsupported versions and missing dependency entries" do + assert {:error, {:line, 2, {:unsupported_version, "v9"}}} = + @valid_lock + |> String.replace("v0.0.2", "v9") + |> ActionsLock.parse() + + missing_dependency = """ + version: 'v0.0.2' + workflows: + '.github/workflows/ci.yml': + - 'actions/checkout@v7.0.1' + dependencies: + """ + + assert {:error, {:workflow_dependencies_missing, [{_, "actions/checkout@v7.0.1"}]}} = + ActionsLock.parse(missing_dependency) + end + + test "rejects symbolic-ref and SHA-key contradictions" do + symbolic = String.replace(@valid_lock, "ref: 'v7.0.1'", "ref: 'v6.0.0'") + + assert {:error, {:line, _, {:symbolic_key_ref_mismatch, _, _}}} = + ActionsLock.parse(symbolic) + + sha_key = """ + version: 'v0.0.2' + workflows: + '.github/workflows/ci.yml': + - 'actions/checkout@aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' + dependencies: + 'actions/checkout@aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa': + ref: 'v7.0.1' + commit: 'sha1-bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb' + owner_id: 44036562 + repo_id: 197814629 + """ + + assert {:error, {:line, _, {:sha_key_commit_mismatch, _}}} = ActionsLock.parse(sha_key) + end + + test "requires each positive repository identity exactly once" do + for field <- [:owner_id, :repo_id] do + missing = Regex.replace(~r/^\s*#{field}:.*\n/m, @valid_lock, "") + + assert {:error, {:line, _, {:missing_dependency_field, _, ^field}}} = + ActionsLock.parse(missing) + end + + zero = String.replace(@valid_lock, "owner_id: 44036562", "owner_id: 0") + + assert {:error, {:line, _, {:nonpositive_dependency_identity, "owner_id"}}} = + ActionsLock.parse(zero) + + malformed = String.replace(@valid_lock, "repo_id: 197814629", "repo_id: unknown") + assert {:error, {:line, _, :malformed_dependency_identity}} = ActionsLock.parse(malformed) + + duplicate = + String.replace( + @valid_lock, + "owner_id: 44036562", + "owner_id: 44036562\n owner_id: 44036562" + ) + + assert {:error, {:line, _, {:duplicate_dependency_field, _, :owner_id}}} = + ActionsLock.parse(duplicate) + end + + test "rejects contradictory owner and repository identities" do + contradictory_owner = """ + version: 'v0.0.2' + workflows: + '.github/workflows/ci.yml': [] + dependencies: + 'actions/checkout@v7': + ref: 'v7' + commit: 'sha1-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' + owner_id: 44036562 + repo_id: 197814629 + 'actions/cache@v6': + ref: 'v6' + commit: 'sha1-bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb' + owner_id: 999 + repo_id: 215566462 + """ + + assert {:error, {:line, _, {:owner_identity_mismatch, "actions", 44_036_562, 999}}} = + ActionsLock.parse(contradictory_owner) + + contradictory_repo = + contradictory_owner + |> String.replace("'actions/cache@v6'", "'actions/checkout@v6'") + |> String.replace("owner_id: 999", "owner_id: 44036562") + + assert {:error, + {:line, _, + {:repository_identity_mismatch, "actions/checkout", 197_814_629, 215_566_462}}} = + ActionsLock.parse(contradictory_repo) + end + + test "rejects a repository id reused by a different repository" do + reused_id = """ + version: 'v0.0.2' + workflows: + '.github/workflows/ci.yml': [] + dependencies: + 'actions/checkout@v7': + ref: 'v7' + commit: 'sha1-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' + owner_id: 44036562 + repo_id: 197814629 + 'other/cache@v6': + ref: 'v6' + commit: 'sha1-bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb' + owner_id: 123 + repo_id: 197814629 + """ + + assert {:error, + {:line, _, {:repository_id_reused, 197_814_629, "actions/checkout", "other/cache"}}} = + ActionsLock.parse(reused_id) + end + + test "validates and resolves transitive dependency entries" do + transitive = """ + version: 'v0.0.2' + workflows: + '.github/workflows/ci.yml': + - 'owner/composite@v1' + dependencies: + 'owner/composite@v1': + ref: 'v1' + commit: 'sha1-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' + owner_id: 1 + repo_id: 2 + uses: + - 'other/child@bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb' + 'other/child@bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb': + ref: 'v2' + commit: 'sha1-bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb' + owner_id: 3 + repo_id: 4 + """ + + assert {:ok, parsed} = ActionsLock.parse(transitive) + + assert parsed.dependencies["owner/composite@v1"].uses == + MapSet.new(["other/child@bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"]) + + missing = + Regex.replace( + ~r/^ 'other\/child@bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb':.*\z/ms, + transitive, + "" + ) + + assert {:error, {:transitive_dependencies_missing, [{"owner/composite@v1", _}]}} = + ActionsLock.parse(missing) + + duplicate = + String.replace( + transitive, + " - 'other/child@bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'", + " - 'other/child@bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'\n" <> + " - 'other/child@bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'" + ) + + assert {:error, {:line, _, {:duplicate_transitive_ref, "owner/composite@v1", _}}} = + ActionsLock.parse(duplicate) + end + + test "rejects non-canonical dependency keys" do + subpath_key = + String.replace( + @valid_lock, + " 'actions/checkout@v7.0.1':\n", + " 'actions/checkout/subpath@v7.0.1':\n" + ) + + colon_key = + String.replace( + @valid_lock, + " 'actions/checkout@v7.0.1':\n", + " 'actions/checkout@refs:heads/main':\n" + ) + + assert {:error, {:line, _, {:malformed_dependency_key, _}}} = + ActionsLock.parse(subpath_key) + + assert {:error, {:line, _, {:malformed_dependency_key, _}}} = ActionsLock.parse(colon_key) + end + + test "parses the repository's authoritative generated lockfile" do + content = File.read!(".github/workflows/actions.lock") + assert {:ok, parsed} = ActionsLock.parse(content) + assert map_size(parsed.workflows) > 0 + assert map_size(parsed.dependencies) > 0 + end + + test "legacy Rules workflow scanner delegates to path-aware lock semantics" do + assert Rules.scan_workflow(@workflow, filename: "ci.yml", actions_lock: @valid_lock) == [] + + findings = Rules.scan_workflow(@workflow, filename: "other.yml", actions_lock: @valid_lock) + assert Enum.any?(findings, &(&1.rule == "unpinned_action")) + + malformed = String.replace(@valid_lock, ~r/sha1-[0-9a-f]{40}/, "sha1-short") + findings = Rules.scan_workflow(@workflow, filename: "ci.yml", actions_lock: malformed) + assert Enum.any?(findings, &(&1.rule == "invalid_actions_lock")) + refute Enum.any?(findings, &(&1.rule == "unpinned_action")) + end +end diff --git a/test/workflow_audit_test.exs b/test/workflow_audit_test.exs index 057c4545..edbe0443 100644 --- a/test/workflow_audit_test.exs +++ b/test/workflow_audit_test.exs @@ -811,22 +811,26 @@ defmodule Hypatia.Rules.WorkflowAuditTest do workflows: '.github/workflows/ci.yml': - 'actions/checkout@v7.0.1' + '.github/workflows/c.yml': + - 'actions/checkout@v7.0.1' + - 'github/codeql-action@v4.37.3' dependencies: 'actions/checkout@v7.0.1': ref: 'v7.0.1' commit: 'sha1-3d3c42e5aac5ba805825da76410c181273ba90b1' + owner_id: 44036562 + repo_id: 197814629 'github/codeql-action@v4.37.3': ref: 'v4.37.3' commit: 'sha1-0000000000000000000000000000000000000000' + owner_id: 9919 + repo_id: 148644467 """ @wf %{"ci.yml" => "jobs:\n a:\n steps:\n - uses: actions/checkout@v7.0.1\n"} test "a ref covered by the lockfile is accepted, not reported unpinned" do - [f] = WorkflowAudit.check_unpinned_actions(@wf, actions_lock: @lock) - assert f.type == :lockfile_pinned_accepted - assert f.severity == :info - assert f.rationale =~ "actions.lock" + assert WorkflowAudit.check_unpinned_actions(@wf, actions_lock: @lock) == [] end test "the SAME ref without a lockfile is still reported unpinned" do @@ -847,14 +851,18 @@ defmodule Hypatia.Rules.WorkflowAuditTest do # `gh actions-lock` records github/codeql-action/init@vN under # github/codeql-action@vN; matching the raw slug would miss it. wf = %{"c.yml" => " - uses: github/codeql-action/init@v4.37.3\n"} - [f] = WorkflowAudit.check_unpinned_actions(wf, actions_lock: @lock) - assert f.type == :lockfile_pinned_accepted + assert WorkflowAudit.check_unpinned_actions(wf, actions_lock: @lock) == [] end - test "lockfile matching is case-insensitive on the slug" do + test "lockfile matching is case-insensitive on owner/repository identity" do wf = %{"c.yml" => " - uses: Actions/Checkout@v7.0.1\n"} + assert WorkflowAudit.check_unpinned_actions(wf, actions_lock: @lock) == [] + end + + test "lockfile matching preserves case-sensitive refs" do + wf = %{"c.yml" => " - uses: actions/checkout@V7.0.1\n"} [f] = WorkflowAudit.check_unpinned_actions(wf, actions_lock: @lock) - assert f.type == :lockfile_pinned_accepted + assert f.type == :unpinned_action end test "a different ref of a locked action is NOT accepted" do @@ -862,5 +870,30 @@ defmodule Hypatia.Rules.WorkflowAuditTest do [f] = WorkflowAudit.check_unpinned_actions(wf, actions_lock: @lock) assert f.type == :unpinned_action end + + test "a dependency listed only for another workflow does not authorize this one" do + wf = %{"other.yml" => " - uses: actions/checkout@v7.0.1\n"} + [f] = WorkflowAudit.check_unpinned_actions(wf, actions_lock: @lock) + assert f.type == :unpinned_action + end + + test "a malformed lock is explicit and authorizes no refs" do + malformed = String.replace(@lock, ~r/sha1-[0-9a-f]{40}/, "sha1-short", global: false) + findings = WorkflowAudit.check_unpinned_actions(@wf, actions_lock: malformed) + + assert [%{type: :invalid_actions_lock, action: :regenerate}] = findings + refute Enum.any?(findings, &(&1.type == :unpinned_action)) + end + + test "an invalid lock is visible even when a workflow has no symbolic actions" do + malformed = String.replace(@lock, "version: 'v0.0.2'", "version: 'v9'") + + findings = + WorkflowAudit.check_unpinned_actions(%{"empty.yml" => "jobs: {}\n"}, + actions_lock: malformed + ) + + assert [%{type: :invalid_actions_lock, action: :regenerate}] = findings + end end end diff --git a/test/workflow_hardening_test.exs b/test/workflow_hardening_test.exs index 37bab4cb..ccfe5908 100644 --- a/test/workflow_hardening_test.exs +++ b/test/workflow_hardening_test.exs @@ -16,6 +16,27 @@ defmodule Hypatia.Rules.WorkflowHardeningTest do repo end + defp write_actions_lock( + repo, + workflow_path, + commit \\ "3d3c42e5aac5ba805825da76410c181273ba90b1" + ) do + content = """ + version: 'v0.0.2' + workflows: + '#{workflow_path}': + - 'actions/checkout@v7.0.1' + dependencies: + 'actions/checkout@v7.0.1': + ref: 'v7.0.1' + commit: 'sha1-#{commit}' + owner_id: 44036562 + repo_id: 197814629 + """ + + File.write!(Path.join([repo, ".github", "workflows", "actions.lock"]), content) + end + setup context do on_exit(fn -> if context[:repo] do @@ -135,6 +156,56 @@ defmodule Hypatia.Rules.WorkflowHardeningTest do File.rm_rf!(repo) end + test "accepts only a valid lock entry associated with the current workflow" do + repo = + create_repo_with_workflow(""" + jobs: + x: + steps: + - uses: actions/checkout@v7.0.1 + """) + + write_actions_lock(repo, ".github/workflows/test.yml") + assert WorkflowHardening.wh004_unpinned_uses(repo) == [] + + write_actions_lock(repo, ".github/workflows/other.yml") + assert [%{rule: "WH004"}] = WorkflowHardening.wh004_unpinned_uses(repo) + File.rm_rf!(repo) + end + + test "malformed lock commit fails closed" do + repo = + create_repo_with_workflow(""" + jobs: + x: + steps: + - uses: actions/checkout@v7.0.1 + """) + + write_actions_lock(repo, ".github/workflows/test.yml", "short") + + assert [ + %{ + rule: "invalid_actions_lock", + file: ".github/workflows/actions.lock", + severity: :high, + action: :regenerate + } + ] = WorkflowHardening.wh004_unpinned_uses(repo) + + File.rm_rf!(repo) + end + + test "invalid lock is reported even when no workflow has a symbolic action" do + repo = create_repo_with_workflow("jobs: {}\n") + write_actions_lock(repo, ".github/workflows/test.yml", "short") + + assert [%{rule: "invalid_actions_lock", severity: :high}] = + WorkflowHardening.wh004_unpinned_uses(repo) + + File.rm_rf!(repo) + end + test "accepts 40-char SHA-pinned actions" do repo = create_repo_with_workflow("""