diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index fead7b02e6..3ae9b23d67 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/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["'])/ diff --git a/Library/Homebrew/rubocops/lines.rb b/Library/Homebrew/rubocops/lines.rb index faa5f6ff80..93c9cd3ba9 100644 --- a/Library/Homebrew/rubocops/lines.rb +++ b/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 diff --git a/Library/Homebrew/test/rubocops/lines_spec.rb b/Library/Homebrew/test/rubocops/lines_spec.rb index 9c92e47850..95ad56c311 100644 --- a/Library/Homebrew/test/rubocops/lines_spec.rb +++ b/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