diff --git a/lib/hypatia/workflow.ex b/lib/hypatia/workflow.ex index de90e6c3..f4b06a2a 100644 --- a/lib/hypatia/workflow.ex +++ b/lib/hypatia/workflow.ex @@ -261,36 +261,61 @@ defmodule Hypatia.Workflow do The shell's rules, not YAML's: a quote opens anywhere outside quotes and carries across lines; a `#` opens a comment only where a word starts - (line start, or after a blank or `; & | ( ) < >`); a backslash escapes the + (line start, or after a blank, `; & | ( < >`, or a subshell's `)`); a backslash escapes the next byte outside single quotes. """ def shell_mask(script, opts \\ []) when is_binary(script), - do: mask(script, nil, true, false, Keyword.get(opts, :quotes, false), []) + do: mask(script, nil, true, false, Keyword.get(opts, :quotes, false), [], []) - @word_breaks [?\s, ?\t, ?;, ?&, ?|, ?(, ?), ?<, ?>] + # `)` is decided by what it closes (outside quotes): a subshell's `)` ends + # the word, so a `#` after it opens a comment; the `)` of `$(…)` does not, + # so `$(name)#suffix` is one word (Codex and CodeRabbit on #1129). `parens` + # is the stack of open `(`: :subst for `$(`, :sub for a bare `(`. A `)` + # with nothing open (a `case` pattern) ends the word. + @word_breaks [?\s, ?\t, ?;, ?&, ?|, ?(, ?<, ?>] - defp mask(<<>>, _q, _ws?, _cmt?, _qs?, acc), do: acc |> Enum.reverse() |> IO.iodata_to_binary() - defp mask(<>, q, _, _, qs?, acc), do: mask(r, q, true, false, qs?, [?\n | acc]) - defp mask(<<_, r::binary>>, q, ws?, true, qs?, acc), do: mask(r, q, ws?, true, qs?, [?\s | acc]) + defp mask(<<>>, _q, _ws?, _cmt?, _qs?, _parens, acc), + do: acc |> Enum.reverse() |> IO.iodata_to_binary() - defp mask(<>, nil, true, _, qs?, acc), - do: mask(r, nil, true, true, qs?, [?\s | acc]) + defp mask(<>, q, _, _, qs?, ps, acc), + do: mask(r, q, true, false, qs?, ps, [?\n | acc]) - defp mask(<>, nil, _, _, qs?, acc) when c in [?', ?"], - do: mask(r, c, false, false, qs?, [c | acc]) + defp mask(<<_, r::binary>>, q, ws?, true, qs?, ps, acc), + do: mask(r, q, ws?, true, qs?, ps, [?\s | acc]) - defp mask(<>, c, _, _, qs?, acc), do: mask(r, nil, false, false, qs?, [c | acc]) + defp mask(<>, nil, true, _, qs?, ps, acc), + do: mask(r, nil, true, true, qs?, ps, [?\s | acc]) - defp mask(<>, q, _, _, qs?, acc) when q != ?' do + defp mask(<>, nil, _, _, qs?, ps, acc) when c in [?', ?"], + do: mask(r, c, false, false, qs?, ps, [c | acc]) + + defp mask(<>, c, _, _, qs?, ps, acc), + do: mask(r, nil, false, false, qs?, ps, [c | acc]) + + defp mask(<>, q, _, _, qs?, ps, acc) when q != ?' do kept = if q != nil and qs?, do: [blank(c), ?\s], else: [c, ?\\] - mask(r, q, false, false, qs?, kept ++ acc) + mask(r, q, false, false, qs?, ps, kept ++ acc) + end + + defp mask(<>, nil, _, _, qs?, ps, acc), + do: mask(r, nil, true, false, qs?, [:subst | ps], [?(, ?$ | acc]) + + defp mask(<>, nil, _, _, qs?, ps, acc), + do: mask(r, nil, true, false, qs?, [:sub | ps], [?( | acc]) + + defp mask(<>, nil, _, _, qs?, ps, acc) do + case ps do + [:subst | rest] -> mask(r, nil, false, false, qs?, rest, [?) | acc]) + [_ | rest] -> mask(r, nil, true, false, qs?, rest, [?) | acc]) + [] -> mask(r, nil, true, false, qs?, [], [?) | acc]) + end end - defp mask(<>, nil, _, _, qs?, acc), - do: mask(r, nil, c in @word_breaks, false, qs?, [c | acc]) + defp mask(<>, nil, _, _, qs?, ps, acc), + do: mask(r, nil, c in @word_breaks, false, qs?, ps, [c | acc]) - defp mask(<>, q, _, _, qs?, acc), - do: mask(r, q, false, false, qs?, [if(qs?, do: blank(c), else: c) | acc]) + defp mask(<>, q, _, _, qs?, ps, acc), + do: mask(r, q, false, false, qs?, ps, [if(qs?, do: blank(c), else: c) | acc]) defp blank(?\n), do: ?\n defp blank(_), do: ?\s diff --git a/lib/rules/workflow_audit.ex b/lib/rules/workflow_audit.ex index ee9a8329..ed9bbceb 100644 --- a/lib/rules/workflow_audit.ex +++ b/lib/rules/workflow_audit.ex @@ -5427,7 +5427,9 @@ defmodule Hypatia.Rules.WorkflowAudit do code = Hypatia.Workflow.shell_mask(run) bare = Hypatia.Workflow.shell_mask(run, quotes: true) - ~r/\bexit\s+/ + # Blanks only: a masked comment must not join `exit` to the next line + # (Codex on #1110). + ~r/\bexit[ \t]+/ |> Regex.scan(bare, return: :index) |> Enum.any?(fn [{off, len}] -> arg = binary_part(code, off + len, byte_size(code) - off - len) @@ -5457,7 +5459,8 @@ defmodule Hypatia.Rules.WorkflowAudit do defp later_command?(line) do line |> String.replace("&&", " ") - |> String.split(~r/[;&]/) + # `&>`, `>&2` and `<&` are redirections, not a new command (Codex on #1110). + |> String.split(~r/;|(?&])&(?![>&])/) |> Enum.drop(1) |> Enum.any?(&(String.trim(&1) != "")) end diff --git a/test/rules/always_reads_cancelled_needs_test.exs b/test/rules/always_reads_cancelled_needs_test.exs index 04f964f3..96e7042f 100644 --- a/test/rules/always_reads_cancelled_needs_test.exs +++ b/test/rules/always_reads_cancelled_needs_test.exs @@ -313,6 +313,37 @@ defmodule Hypatia.Rules.AlwaysReadsCancelledNeedsTest do assert check(%{"f.yml" => wf}) == [] end + # Codex on #1110, after merge. + test "a bare exit with a comment does not take the next line as its argument" do + wf = + String.replace(@verdict, "|| exit 1", "|| true\n exit # done\n \"$CODE\"") + + assert check(%{"f.yml" => wf}) == [] + end + + test "an &> redirection is not a later command" do + wf = + String.replace( + @verdict, + "[ \"$DEED_RESULT\" = success ] && [ \"$K9_RESULT\" = success ] || exit 1", + "test \"$DEED_RESULT\" = success && echo ok &>/dev/null" + ) + + assert [%{rule: "WF033"}] = check(%{"f.yml" => wf}) + end + + # CodeRabbit on #1129. + test "a comment after a subshell does not hide the exit on the next line" do + wf = + String.replace( + @verdict, + "[ \"$DEED_RESULT\" = success ] && [ \"$K9_RESULT\" = success ] || exit 1", + "(true)# it's a note\n [ \"$DEED_RESULT\" = success ] || exit 1" + ) + + assert [%{rule: "WF033"}] = check(%{"f.yml" => wf}) + end + test "it is part of the full audit" do %{findings: fs} = WorkflowAudit.audit(["fork-governance.yml"], %{"fork-governance.yml" => @verdict}) diff --git a/test/rules/ci_failure_triage_test.exs b/test/rules/ci_failure_triage_test.exs index 80575a4c..b6537c0b 100644 --- a/test/rules/ci_failure_triage_test.exs +++ b/test/rules/ci_failure_triage_test.exs @@ -862,6 +862,21 @@ defmodule Hypatia.Rules.CiFailureTriageTest do assert [%{file: "sk.yml", key: "checkout ref"}] = Enum.filter(found, &(&1.file == "sk.yml")) end + # Codex on #1110, after merge. + test "a # straight after a command substitution is part of the word" do + wf = + "jobs:\n a:\n steps:\n - run: grid --label=$(name)#suffix --hypatia-ref #{@old}\n" + + found = T.check_hypatia_pin_drift(Map.put(template_set(@new), "cs.yml", wf)) + assert [%{file: "cs.yml", line: 4}] = Enum.filter(found, &(&1.file == "cs.yml")) + end + + # CodeRabbit on #1129. + test "a flag commented out after a subshell is not a pin" do + wf = "jobs:\n a:\n steps:\n - run: (cd x)# grid --hypatia-ref #{@old}\n" + assert T.check_hypatia_pin_drift(Map.put(template_set(@new), "ss.yml", wf)) == [] + end + test "a variable HYPATIA_SHA is not a pin" do set = Map.put(template_set(@new), "v.yml", "env:\n HYPATIA_SHA: ${{ inputs.sha }}\n") assert T.check_hypatia_pin_drift(set) == []