fix(code_safety): js_exec_sync flags only calls a shell parses

lib/rules/code_safety.ex

@@ -608,7 +608,31 @@ defmodule Hypatia.Rules.CodeSafety do
     %{
       id: :js_exec_sync,
       severity: :high,
-      pattern: ~r/execSync\s*\(|child_process/,
+      # Only calls a shell parses:
+      #   * execSync(, and exec( under a name the file imports from
+      #     node:child_process ({exec}, {exec as run}, {exec: run}) --
+      #     a local function named exec is not a shell;
+      #   * <ns>.exec( / <ns>.execSync( where <ns> is the name the file binds
+      #     node:child_process to (import * as ns / import ns / require /
+      #     await import), and
+      #     require('child_process').exec( -- any argument shape;
+      #   * spawn/spawnSync/execFile/execFileSync (or a name the file imports
+      #     them as) called with a `shell` option (bare or quoted key) set to
+      #     true or to a shell path. fork() ignores `shell`, so it is not here.
+      # Importing node:child_process is not a finding, nor is a shell-free
+      # call with an argv array, RegExp.exec, or a `shell` key outside a
+      # process call. The <ns> calls are matched per file by
+      # pattern_for/2 (`bound_calls`), so every call counts, on its own line.
+      # Not covered: a shell option passed through a variable, tainted argv to
+      # a shell-free call, and call arguments nested more than two
+      # parentheses deep -- a pattern cannot trace values (mathesis#90).
+      # Known false positive: a `shell` key nested inside another option of a
+      # process call (`spawn(c, a, {env: {shell: 'x'}})`) is reported; telling
+      # object depth apart needs a parser, and a rare false alarm is the safer
+      # error for a high-severity rule.
+      pattern:
+        ~r/\bexecSync\s*\(|require\(\s*['"](?:node:)?child_process['"]\s*\)\s*\.exec(?:Sync)?\s*\(|\b(?:spawn|spawnSync|execFile|execFileSync)\s*\((?:[^;()]|\((?:[^()]|\([^()]*\))*\))*?['"]?\bshell['"]?\s*:\s*(?:true\b|['"`])/,
+      bound_calls: :child_process,
       cwe: "CWE-78",
       description: "Shell execution -- validate input before passing to shell"
     },
@@ -898,7 +922,7 @@ defmodule Hypatia.Rules.CodeSafety do
       # line where the match starts) is not a finding; nor is `unsafe {`
       # inside a string literal. Matches are excused after the scan, so a
       # block whose brace is on the next line is judged as one block.
-      matches = Regex.scan(rule.pattern, subject, return: :index)
+      matches = Regex.scan(pattern_for(rule, subject), subject, return: :index)
 
       matches =
         if rule.id == :unsafe_block,
@@ -1624,10 +1648,85 @@ defmodule Hypatia.Rules.CodeSafety do
   """
   def stub_crypto_patterns, do: @stub_crypto_patterns
 
+  # A file's bindings of node:child_process. A regex cannot carry a binding
+  # to every later call (a match consumes it), so the rule's pattern is
+  # widened per file:
+  #   * namespaces (`import * as cp from`, `import cp from`,
+  #     `const cp = require(...)`) add `cp.exec(` and `cp.execSync(`;
+  #   * destructured exec or execSync (`import {exec, execSync as run}
+  #     from`, `const {exec: run} = require(...)`) adds `exec(` / `run(`;
+  #   * a renamed spawn/spawnSync/execFile/execFileSync (`{spawn as run}`)
+  #     adds `run(` with a `shell` option, like the unrenamed names.
+  # `require(...)` may also be `import(...)` or `await import(...)`.
+  # Regex sources, compiled at call time: a compiled regex in a module
+  # attribute does not survive OTP 28.
+  @cp_source ~S|['"](?:node:)?child_process['"]|
+  @cp_load ~S/(?:require|(?:await\s+)?import)\(\s*/
+  @cp_namespace ~S|(?:import\s+(?:\*\s+as\s+)?([A-Za-z_$][\w$]*)\s+from\s*| <>
+                  @cp_source <>
+                  ~S/|(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*/ <>
+                  @cp_load <> @cp_source <> ~S|\s*\))|
+  @cp_destructured ~S|(?:import\s*\{([^}]*)\}\s*from\s*| <>
+                     @cp_source <>
+                     ~S/|(?:const|let|var)\s*\{([^}]*)\}\s*=\s*/ <>
+                     @cp_load <> @cp_source <> ~S|\s*\))|
+  # The argument span of a call up to a `shell` option set to true or to a
+  # shell path; arguments may nest parentheses two deep.
+  @shell_option_tail ~S/\s*\((?:[^;()]|\((?:[^()]|\([^()]*\))*\))*?['"]?\bshell['"]?\s*:\s*(?:true\b|['"`])/
+
+  @doc false
+  def pattern_for(%{bound_calls: :child_process} = rule, content) do
+    namespaces = captures(Regex.compile!(@cp_namespace), content)
+
+    bindings =
+      @cp_destructured
+      |> Regex.compile!()
+      |> captures(content)
+      |> Enum.flat_map(&String.split(&1, ","))
+      |> Enum.flat_map(fn entry ->
+        case Regex.run(
+               ~r/^\s*(exec|execSync|spawn|spawnSync|execFile|execFileSync)\s*(?:(?:as|:)\s*([A-Za-z_$][\w$]*))?\s*$/,
+               entry
+             ) do
+          [_, name, local] -> [{name, local}]
+          [_, name] -> [{name, name}]
+          nil -> []
+        end
+      end)
+      |> Enum.uniq()
+
+    # exec/execSync run a shell whatever the arguments; the spawn family
+    # does only with a `shell` option (unrenamed names are in the pattern).
+    execs = for {name, local} <- bindings, name in ["exec", "execSync"], do: local
+
+    spawns =
+      for {name, local} <- bindings, name not in ["exec", "execSync"], local != name, do: local
+
+    extra =
+      Enum.map(namespaces, &("(?<![\\w$.])" <> Regex.escape(&1) <> "\\.exec(?:Sync)?\\s*\\(")) ++
+        Enum.map(execs, &("(?<![\\w$.])" <> Regex.escape(&1) <> "\\s*\\(")) ++
+        Enum.map(spawns, &("(?<![\\w$.])" <> Regex.escape(&1) <> @shell_option_tail))
+
+    case extra do
+      [] -> rule.pattern
+      extra -> Regex.compile!(Enum.join([Regex.source(rule.pattern) | extra], "|"))
+    end
+  end
+
+  def pattern_for(rule, _content), do: rule.pattern
+
+  defp captures(regex, content) do
+    regex
+    |> Regex.scan(content, capture: :all_but_first)
+    |> List.flatten()
+    |> Enum.reject(&(&1 == ""))
+    |> Enum.uniq()
+  end
+
   @doc "Scan JavaScript/TypeScript content for web security issues"
   def scan_web_security(content) do
     Enum.flat_map(@javascript_patterns, fn rule ->
-      case Regex.scan(rule.pattern, content) do
+      case Regex.scan(pattern_for(rule, content), content) do
         [] ->
           []
 

test/code_safety_test.exs

@@ -867,4 +867,114 @@ defmodule Hypatia.Rules.CodeSafetyTest do
       end
     end
   end
+
+  describe "js_exec_sync flags only calls a shell parses (mathesis#90)" do
+    defp exec_sync?(code),
+      do: Enum.any?(CodeSafety.scan_content(code, "javascript"), &(&1.rule == :js_exec_sync))
+
+    test "importing node:child_process is not a finding" do
+      refute exec_sync?("import {spawnSync} from 'node:child_process';\n")
+      refute exec_sync?("const cp = require('child_process');\n")
+    end
+
+    test "spawnSync and execFileSync with an argv array and no shell are not findings" do
+      code = """
+      import {spawnSync, execFileSync} from 'node:child_process';
+      const git = findTool('git');
+      const r = spawnSync(git, ['ls-remote', `https://github.com/${repo}`], {encoding: 'utf8'});
+      execFileSync('/usr/bin/sha256sum', [path], {encoding: 'utf8'});
+      """
+
+      refute exec_sync?(code)
+    end
+
+    test "RegExp.exec and a function named exec are not findings" do
+      refute exec_sync?("const m = /^v(\\d+)/.exec(tag);\nconst n = re.exec(line);\n")
+      refute exec_sync?("const t = re.exec(`${prefix}-${name}`);\n")
+      refute exec_sync?("function exec(cmd) { return cmd; }\nexec('safe');\n")
+      refute exec_sync?("const exec = (c) => c;\nexec(cmd);\n")
+    end
+
+    test "a shell key outside a process call is not a finding" do
+      refute exec_sync?("const preferences = {shell: true};\n")
+      refute exec_sync?("spawnSync(git, ['status'])\nconst prefs = {shell: true}\n")
+    end
+
+    test "execSync and exec run a shell" do
+      assert exec_sync?("const out = execSync(`git log ${ref}`);\n")
+      assert exec_sync?("import {exec} from 'node:child_process';\nexec(cmd, cb);\n")
+      assert exec_sync?("import {spawn, exec as run} from 'child_process';\nrun(cmd);\n")
+      assert exec_sync?("const { exec: sh } = require('node:child_process');\nsh(cmd);\n")
+      assert exec_sync?("import {execSync as run} from 'node:child_process';\nrun(cmd);\n")
+      assert exec_sync?("const {execSync: run} = require('child_process');\nrun(cmd);\n")
+      refute exec_sync?("import {spawnSync} from 'node:child_process';\nexec(cmd);\n")
+      assert exec_sync?("require('child_process').exec(`ls ${dir}`, cb);\n")
+    end
+
+    test "exec through the name child_process is bound to, any argument shape" do
+      assert exec_sync?("import * as cp from 'node:child_process';\ncp.exec('rm -rf ' + dir);\n")
+      assert exec_sync?("import cp from 'child_process';\ncp.execSync(cmd);\n")
+      assert exec_sync?("const proc = require('child_process');\nproc.exec(cmd, cb);\n")
+      refute exec_sync?("import * as cp from 'node:child_process';\nother.exec(cmd);\n")
+    end
+
+    test "the finding is on the call's line, not the import's" do
+      code = "import * as cp from 'node:child_process';\n\n\ncp.exec(cmd);\n"
+      [f] = Enum.filter(CodeSafety.scan_content(code, "javascript"), &(&1.rule == :js_exec_sync))
+      assert f.lines == [4]
+    end
+
+    test "every call through a bound name counts, each on its own line" do
+      code = "const cp = require('child_process');\ncp.exec('first');\ncp.exec('second');\n"
+      [f] = Enum.filter(CodeSafety.scan_content(code, "javascript"), &(&1.rule == :js_exec_sync))
+      assert f.occurrences == 2
+      assert f.lines == [2, 3]
+
+      [w] = Enum.filter(CodeSafety.scan_web_security(code), &(&1.rule == :js_exec_sync))
+      assert w.occurrences == 2
+    end
+
+    test "two bindings in one file are both followed" do
+      code =
+        "import * as cp from 'node:child_process';\nconst proc = require('child_process');\n" <>
+          "proc.execSync(a);\ncp.exec(b);\n"
+
+      [f] = Enum.filter(CodeSafety.scan_content(code, "javascript"), &(&1.rule == :js_exec_sync))
+      assert f.lines == [3, 4]
+    end
+
+    test "a shell option makes spawn and execFile shell calls" do
+      assert exec_sync?("spawnSync(cmd, args, {shell: true});\n")
+      assert exec_sync?("spawn(cmd, { shell : '/bin/bash' });\n")
+      assert exec_sync?("execFileSync(cmd, args, {encoding: 'utf8', shell: true});\n")
+      assert exec_sync?("spawnSync(cmd, args, {'shell': true});\n")
+      assert exec_sync?("spawnSync(cmd, args, {\"shell\": true});\n")
+      assert exec_sync?("spawn(path.join(dir, 'x'), [a], {\n  shell: true\n});\n")
+      refute exec_sync?("spawnSync(cmd, args, {shell: false});\n")
+      assert exec_sync?("spawn(resolve(path.join(dir, 'x')), args, {shell: true});\n")
+    end
+
+    test "fork ignores the shell option" do
+      refute exec_sync?(
+               "import {fork} from 'node:child_process';\nfork(mod, args, {shell: true});\n"
+             )
+    end
+
+    test "a renamed spawn or execFile with a shell option is a shell call" do
+      assert exec_sync?(
+               "import {spawn as run} from 'node:child_process';\nrun(cmd, a, {shell: true});\n"
+             )
+
+      assert exec_sync?(
+               "const {execFileSync: x} = require('child_process');\nx(cmd, a, {shell: true});\n"
+             )
+
+      refute exec_sync?("import {spawn as run} from 'node:child_process';\nrun(cmd, a);\n")
+    end
+
+    test "a dynamic import binds child_process too" do
+      assert exec_sync?("const cp = await import('node:child_process');\ncp.exec(command);\n")
+      assert exec_sync?("const {exec: sh} = await import('child_process');\nsh(command);\n")
+    end
+  end
 end