Merge pull request #10873 from Bo98/shell_commands_cop

Promote shell commands audit to global cop

Library/Homebrew/cmd/gist-logs.rb

@@ -85,9 +85,9 @@ module Homebrew
 
   # Causes some terminals to display secure password entry indicators.
   def noecho_gets
-    system "stty -echo"
+    system "stty", "-echo"
     result = $stdin.gets
-    system "stty echo"
+    system "stty", "echo"
     puts
     result
   end

Library/Homebrew/cmd/log.rb

@@ -51,7 +51,7 @@ module Homebrew
 
   def git_log(cd_dir, path = nil, tap = nil, args:)
     cd cd_dir
-    repo = Utils.popen_read("git rev-parse --show-toplevel").chomp
+    repo = Utils.popen_read("git", "rev-parse", "--show-toplevel").chomp
     if tap
       name = tap.to_s
       git_cd = "$(brew --repo #{tap})"

Library/Homebrew/dev-cmd/bump-formula-pr.rb

@@ -103,7 +103,7 @@ module Homebrew
     end
 
     formula.tap.path.cd do
-      unless Utils.popen_read("git remote -v").match?(%r{^homebrew.*Homebrew/homebrew-core.*$})
+      unless Utils.popen_read("git", "remote", "-v").match?(%r{^homebrew.*Homebrew/homebrew-core.*$})
         ohai "Adding #{homebrew_core_remote} remote"
         safe_system "git", "remote", "add", homebrew_core_remote, homebrew_core_url
       end
@@ -193,7 +193,7 @@ module Homebrew
         end
         check_new_version(formula, tap_full_name, url: old_url, tag: new_tag, args: args) if new_version.blank?
         resource_path, forced_version = fetch_resource(formula, new_version, old_url, tag: new_tag)
-        new_revision = Utils.popen_read("git -C \"#{resource_path}\" rev-parse -q --verify HEAD")
+        new_revision = Utils.popen_read("git", "-C", resource_path.to_s, "rev-parse", "-q", "--verify", "HEAD")
         new_revision = new_revision.strip
       elsif new_revision.blank?
         odie "#{formula}: the current URL requires specifying a `--revision=` argument."

Library/Homebrew/language/node.rb

@@ -30,7 +30,7 @@ module Language
         prepack_removed = pkg_json["scripts"]&.delete("prepack")
         package.atomic_write(JSON.pretty_generate(pkg_json)) if prepare_removed || prepack_removed
       end
-      output = Utils.popen_read("npm pack --ignore-scripts")
+      output = Utils.popen_read("npm", "pack", "--ignore-scripts")
       raise "npm failed to pack #{Dir.pwd}" if !$CHILD_STATUS.exitstatus.zero? || output.lines.empty?
 
       output.lines.last.chomp

Library/Homebrew/os/linux.rb

@@ -11,7 +11,7 @@ module OS
     sig { returns(String) }
     def os_version
       if which("lsb_release")
-        lsb_info = Utils.popen_read("lsb_release -a")
+        lsb_info = Utils.popen_read("lsb_release", "-a")
         description = lsb_info[/^Description:\s*(.*)$/, 1]
         codename = lsb_info[/^Codename:\s*(.*)$/, 1]
         if codename.blank? || (codename == "n/a")

Library/Homebrew/os/mac/xcode.rb

@@ -348,7 +348,7 @@ module OS
       end
 
       def detect_clang_version
-        version_output = Utils.popen_read("#{PKG_PATH}/usr/bin/clang --version")
+        version_output = Utils.popen_read("#{PKG_PATH}/usr/bin/clang", "--version")
         version_output[/clang-(\d+\.\d+\.\d+(\.\d+)?)/, 1]
       end
 

Library/Homebrew/rubocops.rb

@@ -12,6 +12,8 @@ require "rubocop-rails"
 require "rubocop-rspec"
 require "rubocop-sorbet"
 
+require "rubocops/shell_commands"
+
 require "rubocops/formula_desc"
 require "rubocops/components_order"
 require "rubocops/components_redundancy"

Library/Homebrew/rubocops/lines.rb

@@ -648,58 +648,6 @@ module RuboCop
           problem "Formulae should not depend on :tuntap" if depends_on? :tuntap
         end
       end
-
-      # This cop makes sure that shell command arguments are separated.
-      #
-      # @api private
-      class ShellCommands < FormulaCop
-        extend AutoCorrector
-
-        def audit_formula(_node, _class_node, _parent_class_node, body_node)
-          # Match shell commands separated by spaces in the same string
-          shell_cmd_with_spaces_regex = /[^"' ]*(?:\s[^"' ]*)+/
-
-          popen_commands = [
-            :popen_read,
-            :safe_popen_read,
-            :popen_write,
-            :safe_popen_write,
-          ]
-
-          shell_metacharacters = %w[> < < | ; : & * $ ? : ~ + @ !` ( ) [ ]]
-
-          find_every_method_call_by_name(body_node, :system).each do |method|
-            # Only separate when no shell metacharacters are present
-            next if shell_metacharacters.any? { |meta| string_content(parameters(method).first).include?(meta) }
-
-            next unless (match = regex_match_group(parameters(method).first, shell_cmd_with_spaces_regex))
-
-            good_args = match[0].gsub(" ", "\", \"")
-            offending_node(parameters(method).first)
-            problem "Separate `system` commands into `\"#{good_args}\"`" do |corrector|
-              corrector.replace(@offensive_node.source_range, @offensive_node.source.gsub(" ", "\", \""))
-            end
-          end
-
-          popen_commands.each do |command|
-            find_instance_method_call(body_node, "Utils", command) do |method|
-              index = parameters(method).first.hash_type? ? 1 : 0
-
-              # Only separate when no shell metacharacters are present
-              next if shell_metacharacters.any? { |meta| string_content(parameters(method)[index]).include?(meta) }
-
-              next unless (match = regex_match_group(parameters(method)[index], shell_cmd_with_spaces_regex))
-
-              good_args = match[0].gsub(" ", "\", \"")
-              offending_node(parameters(method)[index])
-              problem "Separate `Utils.#{command}` commands into `\"#{good_args}\"`" do |corrector|
-                good_args = @offensive_node.source.gsub(" ", "\", \"")
-                corrector.replace(@offensive_node.source_range, good_args)
-              end
-            end
-          end
-        end
-      end
     end
   end
 end

Library/Homebrew/rubocops/shared/helper_functions.rb

@@ -68,6 +68,15 @@ module RuboCop
             end
           end
           content
+        when :send
+          if node.method?(:+) && (node.receiver.str_type? || node.receiver.dstr_type?)
+            content = string_content(node.receiver)
+            arg = node.arguments.first
+            content += string_content(arg) if arg
+            content
+          else
+            ""
+          end
         when :const
           node.const_name
         when :sym

Library/Homebrew/rubocops/shell_commands.rb

@@ -0,0 +1,73 @@
+# typed: true
+# frozen_string_literal: true
+
+require "active_support/core_ext/array/access"
+require "rubocops/shared/helper_functions"
+
+module RuboCop
+  module Cop
+    module Style
+      # This cop makes sure that shell command arguments are separated.
+      #
+      # @api private
+      class ShellCommands < Base
+        include HelperFunctions
+        extend AutoCorrector
+
+        MSG = "Separate `%<method>s` commands into `%<good_args>s`"
+
+        TARGET_METHODS = [
+          [nil, :system],
+          [nil, :safe_system],
+          [nil, :quiet_system],
+          [:Utils, :popen_read],
+          [:Utils, :safe_popen_read],
+          [:Utils, :popen_write],
+          [:Utils, :safe_popen_write],
+        ].freeze
+        RESTRICT_ON_SEND = TARGET_METHODS.map(&:second).uniq.freeze
+
+        SHELL_METACHARACTERS = %w[> < < | ; : & * $ ? : ~ + @ ! ` ( ) [ ]].freeze
+
+        def on_send(node)
+          TARGET_METHODS.each do |target_class, target_method|
+            next unless node.method_name == target_method
+
+            target_receivers = if target_class.nil?
+              [nil, s(:const, nil, :Kernel), s(:const, nil, :Homebrew)]
+            else
+              [s(:const, nil, target_class)]
+            end
+            next unless target_receivers.include?(node.receiver)
+
+            first_arg = node.arguments.first
+            arg_count = node.arguments.count
+            if first_arg&.hash_type? # popen methods allow env hash
+              first_arg = node.arguments.second
+              arg_count -= 1
+            end
+            next if first_arg.nil? || arg_count >= 2
+
+            first_arg_str = string_content(first_arg)
+
+            # Only separate when no shell metacharacters are present
+            next if SHELL_METACHARACTERS.any? { |meta| first_arg_str.include?(meta) }
+
+            split_args = first_arg_str.shellsplit
+            next if split_args.count <= 1
+
+            good_args = split_args.map { |arg| "\"#{arg}\"" }.join(", ")
+            method_string = if target_class
+              "#{target_class}.#{target_method}"
+            else
+              target_method.to_s
+            end
+            add_offense(first_arg, message: format(MSG, method: method_string, good_args: good_args)) do |corrector|
+              corrector.replace(first_arg.source_range, good_args)
+            end
+          end
+        end
+      end
+    end
+  end
+end

Library/Homebrew/test/language/node_spec.rb

@@ -4,7 +4,7 @@
 require "language/node"
 
 describe Language::Node do
-  let(:npm_pack_cmd) { "npm pack --ignore-scripts" }
+  let(:npm_pack_cmd) { ["npm", "pack", "--ignore-scripts"] }
 
   describe "#setup_npm_environment" do
     it "calls prepend_path when node formula exists only during the first call" do
@@ -31,7 +31,7 @@ describe Language::Node do
       mktmpdir.cd do
         path = Pathname("package.json")
         path.atomic_write("{\"scripts\":{\"prepare\": \"ls\", \"prepack\": \"ls\", \"test\": \"ls\"}}")
-        allow(Utils).to receive(:popen_read).with(npm_pack_cmd).and_return(`echo pack.tgz`)
+        allow(Utils).to receive(:popen_read).with(*npm_pack_cmd).and_return(`echo pack.tgz`)
         described_class.pack_for_installation
         expect(path.read).not_to include("prepare")
         expect(path.read).not_to include("prepack")
@@ -44,19 +44,19 @@ describe Language::Node do
     npm_install_arg = Pathname("libexec")
 
     it "raises error with non zero exitstatus" do
-      allow(Utils).to receive(:popen_read).with(npm_pack_cmd).and_return(`false`)
+      allow(Utils).to receive(:popen_read).with(*npm_pack_cmd).and_return(`false`)
       expect { described_class.std_npm_install_args(npm_install_arg) }.to \
         raise_error("npm failed to pack #{Dir.pwd}")
     end
 
     it "raises error with empty npm pack output" do
-      allow(Utils).to receive(:popen_read).with(npm_pack_cmd).and_return(`true`)
+      allow(Utils).to receive(:popen_read).with(*npm_pack_cmd).and_return(`true`)
       expect { described_class.std_npm_install_args(npm_install_arg) }.to \
         raise_error("npm failed to pack #{Dir.pwd}")
     end
 
     it "does not raise error with a zero exitstatus" do
-      allow(Utils).to receive(:popen_read).with(npm_pack_cmd).and_return(`echo pack.tgz`)
+      allow(Utils).to receive(:popen_read).with(*npm_pack_cmd).and_return(`echo pack.tgz`)
       resp = described_class.std_npm_install_args(npm_install_arg)
       expect(resp).to include("--prefix=#{npm_install_arg}", "#{Dir.pwd}/pack.tgz")
     end

Library/Homebrew/test/rubocops/shell_commands_spec.rb

@@ -1,9 +1,9 @@
 # typed: false
 # frozen_string_literal: true
 
-require "rubocops/lines"
+require "rubocops/shell_commands"
 
-describe RuboCop::Cop::FormulaAuditStrict::ShellCommands do
+describe RuboCop::Cop::Style::ShellCommands do
   subject(:cop) { described_class.new }
 
   context "when auditing shell commands" do

Library/Homebrew/utils/github.rb

@@ -481,7 +481,7 @@ module GitHub
     pr_message = info[:pr_message]
 
     sourcefile_path.parent.cd do
-      git_dir = Utils.popen_read("git rev-parse --git-dir").chomp
+      git_dir = Utils.popen_read("git", "rev-parse", "--git-dir").chomp
       shallow = !git_dir.empty? && File.exist?("#{git_dir}/shallow")
       changed_files = [sourcefile_path]
       changed_files += additional_files if additional_files.present?
@@ -500,7 +500,7 @@ module GitHub
 
         unless args.commit?
           if args.no_fork?
-            remote_url = Utils.popen_read("git remote get-url --push origin").chomp
+            remote_url = Utils.popen_read("git", "remote", "get-url", "--push", "origin").chomp
             username = tap.user
           else
             begin