Merge pull request #7691 from Rylan12/popen_read-audit

style: separate shell command arguments

Library/Homebrew/dev-cmd/audit.rb

@@ -873,15 +873,6 @@ module Homebrew
       # TODO: check could be in RuboCop
       problem "`env :userpaths` in formulae is deprecated" if line.include?("env :userpaths")
 
-      if line =~ /system ((["'])[^"' ]*(?:\s[^"' ]*)+\2)/
-        bad_system = Regexp.last_match(1)
-        unless %w[| < > & ; *].any? { |c| bad_system.include? c }
-          good_system = bad_system.gsub(" ", "\", \"")
-          # TODO: check could be in RuboCop
-          problem "Use `system #{good_system}` instead of `system #{bad_system}` "
-        end
-      end
-
       # TODO: check could be in RuboCop
       problem "`#{Regexp.last_match(1)}` is now unnecessary" if line =~ /(require ["']formula["'])/
 

Library/Homebrew/rubocops/lines.rb

@@ -555,6 +555,55 @@ module RuboCop
           end
         end
       end
+
+      class ShellCommands < FormulaCop
+        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}\"`"
+          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}\"`"
+            end
+          end
+        end
+
+        def autocorrect(node)
+          lambda do |corrector|
+            good_args = node.source.gsub(" ", "\", \"")
+            corrector.replace(node.source_range, good_args)
+          end
+        end
+      end
     end
   end
 end

Library/Homebrew/test/rubocops/lines_spec.rb

@@ -1096,3 +1096,252 @@ describe RuboCop::Cop::FormulaAuditStrict::MakeCheck do
 
   include_examples "formulae exist", described_class::MAKE_CHECK_ALLOWLIST
 end
+
+describe RuboCop::Cop::FormulaAuditStrict::ShellCommands do
+  subject(:cop) { described_class.new }
+
+  context "When auditing shell commands" do
+    it "system arguments should be separated" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            system "foo bar"
+                   ^^^^^^^^^ Separate `system` commands into `\"foo\", \"bar\"`
+          end
+        end
+      RUBY
+    end
+
+    it "system arguments with string interpolation should be separated" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            system "\#{bin}/foo bar"
+                   ^^^^^^^^^^^^^^^^ Separate `system` commands into `\"\#{bin}/foo\", \"bar\"`
+          end
+        end
+      RUBY
+    end
+
+    it "system arguments with metacharacters should not be separated" do
+      expect_no_offenses(<<~RUBY)
+        class Foo < Formula
+          def install
+            system "foo bar > baz"
+          end
+        end
+      RUBY
+    end
+
+    it "only the first system argument should be separated" do
+      expect_no_offenses(<<~RUBY)
+        class Foo < Formula
+          def install
+            system "foo", "bar baz"
+          end
+        end
+      RUBY
+    end
+
+    it "Utils.popen arguments should not be separated" do
+      expect_no_offenses(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.popen("foo bar")
+          end
+        end
+      RUBY
+    end
+
+    it "Utils.popen_read arguments should be separated" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.popen_read("foo bar")
+                             ^^^^^^^^^ Separate `Utils.popen_read` commands into `\"foo\", \"bar\"`
+          end
+        end
+      RUBY
+    end
+
+    it "Utils.safe_popen_read arguments should be separated" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.safe_popen_read("foo bar")
+                                  ^^^^^^^^^ Separate `Utils.safe_popen_read` commands into `\"foo\", \"bar\"`
+          end
+        end
+      RUBY
+    end
+
+    it "Utils.popen_write arguments should be separated" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.popen_write("foo bar")
+                              ^^^^^^^^^ Separate `Utils.popen_write` commands into `\"foo\", \"bar\"`
+          end
+        end
+      RUBY
+    end
+
+    it "Utils.safe_popen_write arguments should be separated" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.safe_popen_write("foo bar")
+                                   ^^^^^^^^^ Separate `Utils.safe_popen_write` commands into `\"foo\", \"bar\"`
+          end
+        end
+      RUBY
+    end
+
+    it "Utils.popen_read arguments with string interpolation should be separated" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.popen_read("\#{bin}/foo bar")
+                             ^^^^^^^^^^^^^^^^ Separate `Utils.popen_read` commands into `\"\#{bin}/foo\", \"bar\"`
+          end
+        end
+      RUBY
+    end
+
+    it "Utils.popen_read arguments with metacharacters should not be separated" do
+      expect_no_offenses(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.popen_read("foo bar > baz")
+          end
+        end
+      RUBY
+    end
+
+    it "only the first Utils.popen_read argument should be separated" do
+      expect_no_offenses(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.popen_read("foo", "bar baz")
+          end
+        end
+      RUBY
+    end
+
+    it "Utils.popen_read arguments should be separated following a shell variable" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.popen_read({ "SHELL" => "bash"}, "foo bar")
+                                                   ^^^^^^^^^ Separate `Utils.popen_read` commands into `\"foo\", \"bar\"`
+          end
+        end
+      RUBY
+    end
+
+    it "separates shell commands in system" do
+      source = <<~RUBY
+        class Foo < Formula
+          def install
+            system "foo bar"
+          end
+        end
+      RUBY
+
+      corrected_source = <<~RUBY
+        class Foo < Formula
+          def install
+            system "foo", "bar"
+          end
+        end
+      RUBY
+
+      new_source = autocorrect_source(source)
+      expect(new_source).to eq(corrected_source)
+    end
+
+    it "separates shell commands with string interpolation in system" do
+      source = <<~RUBY
+        class Foo < Formula
+          def install
+            system "\#{foo}/bar baz"
+          end
+        end
+      RUBY
+
+      corrected_source = <<~RUBY
+        class Foo < Formula
+          def install
+            system "\#{foo}/bar", "baz"
+          end
+        end
+      RUBY
+
+      new_source = autocorrect_source(source)
+      expect(new_source).to eq(corrected_source)
+    end
+
+    it "separates shell commands in Utils.popen_read" do
+      source = <<~RUBY
+        class Foo < Formula
+          def install
+            Utils.popen_read("foo bar")
+          end
+        end
+      RUBY
+
+      corrected_source = <<~RUBY
+        class Foo < Formula
+          def install
+            Utils.popen_read("foo", "bar")
+          end
+        end
+      RUBY
+
+      new_source = autocorrect_source(source)
+      expect(new_source).to eq(corrected_source)
+    end
+
+    it "separates shell commands with string interpolation in Utils.popen_read" do
+      source = <<~RUBY
+        class Foo < Formula
+          def install
+            Utils.popen_read("\#{foo}/bar baz")
+          end
+        end
+      RUBY
+
+      corrected_source = <<~RUBY
+        class Foo < Formula
+          def install
+            Utils.popen_read("\#{foo}/bar", "baz")
+          end
+        end
+      RUBY
+
+      new_source = autocorrect_source(source)
+      expect(new_source).to eq(corrected_source)
+    end
+
+    it "separates shell commands following a shell variable in Utils.popen_read" do
+      source = <<~RUBY
+        class Foo < Formula
+          def install
+            Utils.popen_read({ "SHELL" => "bash" }, "foo bar")
+          end
+        end
+      RUBY
+
+      corrected_source = <<~RUBY
+        class Foo < Formula
+          def install
+            Utils.popen_read({ "SHELL" => "bash" }, "foo", "bar")
+          end
+        end
+      RUBY
+
+      new_source = autocorrect_source(source)
+      expect(new_source).to eq(corrected_source)
+    end
+  end
+end