fix(research_extensions): read on: brackets outside comments and quotes

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,17 +837,91 @@ 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
 
   defp inline_events(scalar_or_list) 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(<<c, rest::binary>>, 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.
+          {?', <<?', more::binary>>} ->
+            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)

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("""