diff --git a/lib/rules/research_extensions.ex b/lib/rules/research_extensions.ex index 1be54388..fb54af0d 100644 --- a/lib/rules/research_extensions.ex +++ b/lib/rules/research_extensions.ex @@ -626,9 +626,11 @@ defmodule Hypatia.Rules.ResearchExtensions do # is decided before the EXIT trap runs, and a failing handler command # under `set -e` (e.g. `kill` of an already-exited PID) would itself # turn a PASSING step red, so the `|| true` is what keeps it honest. + # Only the quoted handler is dropped, so a command after it on the + # same line (`trap '…' EXIT; cargo test || true`) is still judged. has_or_true? = stripped_body - |> String.replace(~r/^[ \t]*trap[ \t].*$/m, "") + |> String.replace(~r/\btrap[ \t]+(?:'[^'\n]*'|"[^"\n]*")/, "trap") |> then(&Regex.match?(~r/\|\|\s*true\b/, &1)) # A step carrying `hypatia:ignore RE005 -- ` (or the diff --git a/lib/rules/workflow_hardening.ex b/lib/rules/workflow_hardening.ex index 06de1874..cb36b57e 100644 --- a/lib/rules/workflow_hardening.ex +++ b/lib/rules/workflow_hardening.ex @@ -1259,10 +1259,33 @@ defmodule Hypatia.Rules.WorkflowHardening do @doc """ Return true when a write operation's failure is masked, so a denied write - cannot turn the job red. + cannot turn the job red: a shell mask on the write itself, or a STEP-level + `continue-on-error: true` in text that performs a write. The step flag is + judged per job body, so it may belong to a sibling step. A job-level + `continue-on-error` is not counted: it is a deliberate, visible job setting + (standards' github-backup-mirror-reusable.yml documents and surfaces it). """ def masked_write?(content) when is_binary(content) do - Enum.any?(@write_masks, &Regex.match?(&1, content)) + Enum.any?(@write_masks, &Regex.match?(&1, content)) or + (step_continue_on_error?(content) and performs_contents_write?(content)) + end + + # `continue-on-error: true` indented under a `steps:` key, i.e. on a step. + defp step_continue_on_error?(content) do + lines = String.split(content, "\n") + + case Enum.find(lines, &Regex.match?(~r/^[ \t]*steps:[ \t]*$/, &1)) do + nil -> + false + + steps_line -> + steps_indent = indent_of(steps_line) + + Enum.any?(lines, fn line -> + Regex.match?(~r/^[ \t]*(?:-[ \t]+)?continue-on-error:[ \t]*true\b/, line) and + indent_of(line) > steps_indent + end) + end end # ─── Internals ────────────────────────────────────────────────────── @@ -1399,7 +1422,9 @@ defmodule Hypatia.Rules.WorkflowHardening do _ -> cond do - line == "" or indent_of(line) >= jobs_indent + 1 -> + # A comment between jobs (` # Publish`) does not end `jobs:`. + line == "" or indent_of(line) >= jobs_indent + 1 or + String.starts_with?(String.trim_leading(line), "#") -> {acc, true, jobs_indent, append_line(current, line)} true -> diff --git a/scripts/open-data-pr.sh b/scripts/open-data-pr.sh index 80b3aeb5..ff5d54bb 100644 --- a/scripts/open-data-pr.sh +++ b/scripts/open-data-pr.sh @@ -4,7 +4,7 @@ # # Usage: open-data-pr.sh <path>... # -# Commits <path>... on a new branch `inbox-steward/<run_id>-<suffix>`, pushes +# Commits <path>... on a new branch `inbox-steward/<run_id>-<attempt>-<suffix>`, pushes # it, opens a PR against main and queues a squash auto-merge. Used by # inbox-steward-intake.yml instead of `git push origin HEAD` to main, which # bypassed review and signing and was masked so a refusal stayed green. @@ -29,7 +29,9 @@ if git diff --cached --quiet; then exit 0 fi -branch="inbox-steward/${GITHUB_RUN_ID}-${suffix}" +# The attempt number keeps a re-run from colliding with a branch an earlier +# attempt already pushed (same run id, different history). +branch="inbox-steward/${GITHUB_RUN_ID}-${GITHUB_RUN_ATTEMPT:-1}-${suffix}" git switch -c "$branch" git commit -m "$title" git push origin "HEAD:refs/heads/${branch}" diff --git a/test/research_extensions_test.exs b/test/research_extensions_test.exs index f81872b3..f00d9273 100644 --- a/test/research_extensions_test.exs +++ b/test/research_extensions_test.exs @@ -450,6 +450,21 @@ defmodule Hypatia.Rules.ResearchExtensionsTest do File.rm_rf!(repo) end + test "a swallow after a trap on the same line is still reported" do + repo = + create_repo_with_workflow(""" + jobs: + x: + steps: + - name: run tests + run: | + trap 'kill "$PID" 2>/dev/null || true' EXIT; cargo test || true + """) + + assert [%{rule: "RE005"}] = ResearchExtensions.re005_test_swallows_exit(repo) + File.rm_rf!(repo) + end + test "ignores non-test steps with continue-on-error" do repo = create_repo_with_workflow(""" diff --git a/test/workflow_hardening_test.exs b/test/workflow_hardening_test.exs index aabd3f0e..69f30ec8 100644 --- a/test/workflow_hardening_test.exs +++ b/test/workflow_hardening_test.exs @@ -1046,6 +1046,70 @@ defmodule Hypatia.Rules.WorkflowHardeningTest do File.rm_rf!(repo) end + test "a comment between jobs does not hide the later jobs" do + repo = + create_repo_with_workflow(""" + name: CI + permissions: + contents: read + jobs: + test: + runs-on: ubuntu-latest + steps: + - run: mix test + # Publish + publish: + runs-on: ubuntu-latest + steps: + - run: git push origin HEAD + """) + + assert [%{severity: :high}] = WorkflowHardening.wh013_permission_starved_write(repo) + File.rm_rf!(repo) + end + + test "continue-on-error on a credentialed write counts as masked" do + repo = + create_repo_with_workflow(""" + name: Release + permissions: + contents: read + jobs: + publish: + runs-on: ubuntu-latest + steps: + - continue-on-error: true + env: + GH_TOKEN: ${{ secrets.RELEASE_PAT }} + run: gh release create v1 dist/* + """) + + assert [%{fix_recipe: "unmask-credentialed-write"}] = + WorkflowHardening.wh013_permission_starved_write(repo) + + File.rm_rf!(repo) + end + + test "a job-level continue-on-error is a deliberate setting, not a mask" do + repo = + create_repo_with_workflow(""" + name: Backup + permissions: + contents: read + jobs: + mirror: + runs-on: ubuntu-latest + continue-on-error: true + steps: + - env: + GH_TOKEN: ${{ secrets.BACKUP_PAT }} + run: git push backup HEAD:refs/heads/main && gh pr create --repo o/b --title x --body y + """) + + assert [] = WorkflowHardening.wh013_permission_starved_write(repo) + File.rm_rf!(repo) + end + test "a masked credentialed gh release is reported" do repo = create_repo_with_workflow("""