diff --git a/lib/rules/research_extensions.ex b/lib/rules/research_extensions.ex index 253071d6..55c6a7be 100644 --- a/lib/rules/research_extensions.ex +++ b/lib/rules/research_extensions.ex @@ -792,25 +792,25 @@ defmodule Hypatia.Rules.ResearchExtensions do [] {_, [{on_line, _} | rest]} -> - inline = - on_line - |> String.split(":", parts: 2) - |> List.last() - |> String.replace(~r/\s+#.*$/, "") - # `on: &events` anchors the block mapping that follows. - |> String.replace(~r/^\s*&[A-Za-z0-9_-]+/, "") - |> String.trim() - - if inline != "" do + first = on_line |> String.split(":", parts: 2) |> List.last() + # `on: &events` anchors the block mapping that follows. + unanchor = &(&1 |> String.replace(~r/^\s*&[A-Za-z0-9_-]+/, "") |> String.trim()) + + if unanchor.(flow_text(first, false)) != "" do # A flow collection may continue past the `on:` line: read on until - # its brackets balance. + # its brackets balance. The lines are read as one text, so a quoted + # scalar spanning lines keeps its quotes and a comment ends at its + # own line; neither names an event or opens a bracket. rest |> Enum.map(&elem(&1, 0)) - |> Enum.reduce_while(inline, fn line, acc -> - if nesting_at(acc, byte_size(acc)) > 0, - do: {:cont, acc <> " " <> String.trim(line)}, + |> Enum.reduce_while(first, fn line, acc -> + if open_flow?(acc), + do: {:cont, acc <> "\n" <> line}, else: {:halt, acc} end) + |> flow_text(false) + |> String.replace("\n", " ") + |> then(unanchor) |> inline_events() else block = @@ -837,10 +837,16 @@ defmodule Hypatia.Rules.ResearchExtensions do # `on: {push: {paths: [issues/**]}}` names one event, push: only the map's # top-level keys are events. A scalar or list names each of its words. defp inline_events("{" <> _ = map) do + # Depth is read from the map with its quoted scalars blanked: a bracket + # in `tags: ["v1.0]"]` is text. Blanking keeps every byte offset. + code = flow_text(map, true) + ~r/["']?([A-Za-z_]+)["']?\s*:/ |> Regex.scan(map, return: :index) |> Enum.flat_map(fn [{start, _}, {from, len}] -> - if nesting_at(map, start) == 1, do: [binary_part(map, from, len)], else: [] + if nesting_at(code, min(start, byte_size(code))) == 1, + do: [binary_part(map, from, len)], + else: [] end) end @@ -848,6 +854,74 @@ defmodule Hypatia.Rules.ResearchExtensions do ~r/[A-Za-z_]+/ |> Regex.scan(scalar_or_list) |> List.flatten() end + # Whether flow text still has an open `{` or `[`. nesting_at/2 counts every + # bracket it is given, so quoted scalars are blanked first; the text has no + # comments left by the time it is read here. + defp open_flow?(flow) do + code = flow_text(flow, true) + nesting_at(code, byte_size(code)) > 0 + end + + # Flow text, possibly spanning lines, with its comments blanked and, when + # `blank_quoted`, the insides of its quoted scalars blanked too. Every + # byte keeps its offset (a blanked byte becomes a space; newlines stay). + # + # Unlike strip_yaml_comment/2, which reads one line, this reads the whole + # value: a quoted scalar that spans lines keeps its state, so a ` #` inside + # it is text. A quote opens a scalar only where a flow node starts (at the + # start, or after `{`, `[`, `,` or `:` and optional spaces), so the + # apostrophe in the plain scalar `Say 'hi` is text as well. + defp flow_text(text, blank_quoted), + do: flow_scan(text, blank_quoted, :plain, ?\n, nil, []) + + # state: :plain | :comment | {:quote, ?'} | {:quote, ?"} | :escape + # prev: the previous byte; sig: the previous non-space byte outside quotes + defp flow_scan(<<>>, _b, _state, _prev, _sig, acc), + do: acc |> Enum.reverse() |> IO.iodata_to_binary() + + defp flow_scan(<>, b, state, prev, sig, acc) do + keep = fn byte -> if b and byte != ?\n, do: ?\s, else: byte end + + case state do + :comment when c == ?\n -> + flow_scan(rest, b, :plain, c, sig, [c | acc]) + + :comment -> + flow_scan(rest, b, :comment, c, sig, [?\s | acc]) + + :escape -> + flow_scan(rest, b, {:quote, ?"}, c, sig, [keep.(c) | acc]) + + {:quote, ?"} when c == ?\\ -> + flow_scan(rest, b, :escape, c, sig, [keep.(c) | acc]) + + {:quote, q} when c == q -> + case {q, rest} do + # '' inside a single-quoted scalar is an escaped quote. + {?', <>} -> + flow_scan(more, b, {:quote, q}, ?', sig, [keep.(?'), keep.(?') | acc]) + + _ -> + flow_scan(rest, b, :plain, c, c, [c | acc]) + end + + {:quote, _} -> + flow_scan(rest, b, state, c, sig, [keep.(c) | acc]) + + :plain when c == ?# and prev in [?\s, ?\t, ?\n] -> + flow_scan(rest, b, :comment, c, sig, [?\s | acc]) + + :plain when c in [?', ?"] and sig in [nil, ?{, ?[, ?,, ?:] -> + flow_scan(rest, b, {:quote, c}, c, c, [c | acc]) + + :plain when c in [?\s, ?\t, ?\n, ?\r] -> + flow_scan(rest, b, :plain, c, sig, [c | acc]) + + :plain -> + flow_scan(rest, b, :plain, c, c, [c | acc]) + end + end + defp harden_runner_step?(job_lines) do with field_indent when is_integer(field_indent) <- job_field_indent(job_lines), step_lines when step_lines != [] <- lines_in_steps(job_lines, field_indent), @@ -923,7 +997,7 @@ defmodule Hypatia.Rules.ResearchExtensions 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, + if open_flow?(acc), do: {:cont, acc <> " " <> String.trim(line)}, else: {:halt, acc} end) diff --git a/test/research_extensions_test.exs b/test/research_extensions_test.exs index 6ce2bd1c..74893569 100644 --- a/test/research_extensions_test.exs +++ b/test/research_extensions_test.exs @@ -277,6 +277,119 @@ defmodule Hypatia.Rules.ResearchExtensionsTest do File.rm_rf!(repo) end + # An inline `on:` list may continue past its first line. Only whole-line + # comments are dropped before the rule reads it, so a continuation line + # can still carry a trailing comment. + test "an untrusted-input trigger on a continuation line of the `on:` list is an exposure" do + repo = + create_repo_with_workflow(""" + on: [push, + issue_comment] # replies to [bot] mentions + jobs: + reply: + runs-on: ubuntu-latest + steps: + - run: ./reply.sh ${{ secrets.BOT_PAT }} + """) + + assert [%{severity: :warn, detail: %{exposures: [exposure]}}] = + ResearchExtensions.re001_missing_harden_runner(repo) + + assert exposure =~ "issue_comment" + File.rm_rf!(repo) + end + + test "decoy: an event named in a comment on a continuation line is not a trigger" do + repo = + create_repo_with_workflow(""" + on: [push, + workflow_dispatch] # was pull_request_target until #12 + jobs: + sync: + runs-on: ubuntu-latest + steps: + - run: ./sync.sh ${{ secrets.BOT_PAT }} + """) + + assert [%{severity: :warn, detail: %{exposures: []}}] = + ResearchExtensions.re001_missing_harden_runner(repo) + + File.rm_rf!(repo) + end + + test "decoy: an open bracket in a continuation comment does not swallow the workflow" do + repo = + create_repo_with_workflow(""" + on: [push, + workflow_dispatch] # see [ops notes + jobs: + sync: + runs-on: ubuntu-latest + steps: + - run: ./issue_comment.sh ${{ secrets.BOT_PAT }} + """) + + assert [%{severity: :warn, detail: %{exposures: []}}] = + ResearchExtensions.re001_missing_harden_runner(repo) + + File.rm_rf!(repo) + end + + test "decoy: a bracket inside a quoted scalar of an `on:` map does not hide its events" do + repo = + create_repo_with_workflow(""" + on: {push: {tags: ["v1.0]"]}, issue_comment: {types: [created]}} + jobs: + reply: + runs-on: ubuntu-latest + steps: + - run: ./reply.sh ${{ secrets.BOT_PAT }} + """) + + assert [%{severity: :warn, detail: %{exposures: [exposure]}}] = + ResearchExtensions.re001_missing_harden_runner(repo) + + assert exposure =~ "issue_comment" + File.rm_rf!(repo) + end + + test "a quoted scalar spanning lines keeps its quotes: a ` #` inside it is not a comment" do + repo = + create_repo_with_workflow(""" + on: {workflow_dispatch: {inputs: {why: {description: "first line + foo #hi"}}}, issue_comment: {}} + jobs: + reply: + runs-on: ubuntu-latest + steps: + - run: ./reply.sh ${{ secrets.BOT_PAT }} + """) + + assert [%{severity: :warn, detail: %{exposures: [exposure]}}] = + ResearchExtensions.re001_missing_harden_runner(repo) + + assert exposure =~ "issue_comment" + File.rm_rf!(repo) + end + + test "an apostrophe inside a plain scalar does not open a quote" do + repo = + create_repo_with_workflow(""" + on: {workflow_dispatch: {inputs: {why: {description: Say 'hi}}}, issue_comment: {}} + jobs: + reply: + runs-on: ubuntu-latest + steps: + - run: ./reply.sh ${{ secrets.BOT_PAT }} + """) + + assert [%{severity: :warn, detail: %{exposures: [exposure]}}] = + ResearchExtensions.re001_missing_harden_runner(repo) + + assert exposure =~ "issue_comment" + File.rm_rf!(repo) + end + test "decoy: `issues:` under top-level permissions is not an issues trigger" do repo = create_repo_with_workflow("""