From 9063945b3e5b0af5f325de33b7be94beb40530e3 Mon Sep 17 00:00:00 2001 From: Bo Anderson Date: Wed, 17 Mar 2021 15:27:26 +0000 Subject: [PATCH] Promote shell commands audit to global cop --- Library/Homebrew/rubocops.rb | 2 + Library/Homebrew/rubocops/lines.rb | 52 ------------- .../rubocops/shared/helper_functions.rb | 9 +++ Library/Homebrew/rubocops/shell_commands.rb | 73 +++++++++++++++++++ .../{text => }/shell_commands_spec.rb | 4 +- 5 files changed, 86 insertions(+), 54 deletions(-) create mode 100644 Library/Homebrew/rubocops/shell_commands.rb rename Library/Homebrew/test/rubocops/{text => }/shell_commands_spec.rb (98%) diff --git a/Library/Homebrew/rubocops.rb b/Library/Homebrew/rubocops.rb index 0d17d82478..aeff5d68a2 100644 --- a/Library/Homebrew/rubocops.rb +++ b/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" diff --git a/Library/Homebrew/rubocops/lines.rb b/Library/Homebrew/rubocops/lines.rb index 8500cfe518..3bc725df76 100644 --- a/Library/Homebrew/rubocops/lines.rb +++ b/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 diff --git a/Library/Homebrew/rubocops/shared/helper_functions.rb b/Library/Homebrew/rubocops/shared/helper_functions.rb index 63858e3697..34ef119114 100644 --- a/Library/Homebrew/rubocops/shared/helper_functions.rb +++ b/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 diff --git a/Library/Homebrew/rubocops/shell_commands.rb b/Library/Homebrew/rubocops/shell_commands.rb new file mode 100644 index 0000000000..2a6646557b --- /dev/null +++ b/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 `%s` commands into `%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 diff --git a/Library/Homebrew/test/rubocops/text/shell_commands_spec.rb b/Library/Homebrew/test/rubocops/shell_commands_spec.rb similarity index 98% rename from Library/Homebrew/test/rubocops/text/shell_commands_spec.rb rename to Library/Homebrew/test/rubocops/shell_commands_spec.rb index 9699092063..60ee8aac3a 100644 --- a/Library/Homebrew/test/rubocops/text/shell_commands_spec.rb +++ b/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