diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 9210c975f9..084fdbab74 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -844,15 +844,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["'])/ diff --git a/Library/Homebrew/rubocops/lines.rb b/Library/Homebrew/rubocops/lines.rb index faa5f6ff80..d8708c2362 100644 --- a/Library/Homebrew/rubocops/lines.rb +++ b/Library/Homebrew/rubocops/lines.rb @@ -252,6 +252,55 @@ module RuboCop 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| + # Continue if a shell metacharacter is 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 + + # Continue if a shell metacharacter is 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 + class Miscellaneous < FormulaCop def audit_formula(_node, _class_node, _parent_class_node, body_node) # FileUtils is included in Formula diff --git a/Library/Homebrew/test/rubocops/lines_spec.rb b/Library/Homebrew/test/rubocops/lines_spec.rb index 9c92e47850..395c3de540 100644 --- a/Library/Homebrew/test/rubocops/lines_spec.rb +++ b/Library/Homebrew/test/rubocops/lines_spec.rb @@ -574,6 +574,150 @@ describe RuboCop::Cop::FormulaAudit::ShellVariables do end end +describe RuboCop::Cop::FormulaAudit::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 + end +end + describe RuboCop::Cop::FormulaAudit::Miscellaneous do subject(:cop) { described_class.new }