From 6e2f194e0933c82246eb84aa0fd7283832bc435e Mon Sep 17 00:00:00 2001 From: Bo Anderson Date: Mon, 3 May 2021 20:25:03 +0100 Subject: [PATCH] rubocops/shell_commands: add cop for shell metacharacters in `exec` --- .../rubocops/shared/helper_functions.rb | 4 +- Library/Homebrew/rubocops/shell_commands.rb | 74 ++- .../test/rubocops/shell_commands_spec.rb | 425 ++++++++++-------- 3 files changed, 298 insertions(+), 205 deletions(-) diff --git a/Library/Homebrew/rubocops/shared/helper_functions.rb b/Library/Homebrew/rubocops/shared/helper_functions.rb index 34ef119114..40f522a7bf 100644 --- a/Library/Homebrew/rubocops/shared/helper_functions.rb +++ b/Library/Homebrew/rubocops/shared/helper_functions.rb @@ -54,7 +54,7 @@ module RuboCop end # Returns the string representation if node is of type str(plain) or dstr(interpolated) or const. - def string_content(node) + def string_content(node, strip_dynamic: false) case node.type when :str node.str_content @@ -62,7 +62,7 @@ module RuboCop content = "" node.each_child_node(:str, :begin) do |child| content += if child.begin_type? - child.source + strip_dynamic ? "" : child.source else child.str_content end diff --git a/Library/Homebrew/rubocops/shell_commands.rb b/Library/Homebrew/rubocops/shell_commands.rb index 2a6646557b..838c15ceef 100644 --- a/Library/Homebrew/rubocops/shell_commands.rb +++ b/Library/Homebrew/rubocops/shell_commands.rb @@ -7,6 +7,42 @@ require "rubocops/shared/helper_functions" module RuboCop module Cop module Style + # https://github.com/ruby/ruby/blob/v2_6_3/process.c#L2430-L2460 + SHELL_BUILTINS = %w[ + ! + . + : + break + case + continue + do + done + elif + else + esac + eval + exec + exit + export + fi + for + if + in + readonly + return + set + shift + then + times + trap + unset + until + while + ].freeze + + # https://github.com/ruby/ruby/blob/v2_6_3/process.c#L2495 + SHELL_METACHARACTERS = %W[* ? { } [ ] < > ( ) ~ & | \\ $ ; ' ` " \n #].freeze + # This cop makes sure that shell command arguments are separated. # # @api private @@ -27,8 +63,6 @@ module RuboCop ].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 @@ -49,13 +83,17 @@ module RuboCop 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) } + stripped_first_arg_str = string_content(first_arg, strip_dynamic: true) split_args = first_arg_str.shellsplit next if split_args.count <= 1 + # Only separate when no shell metacharacters are present + command = split_args.first + next if SHELL_BUILTINS.any?(command) + next if command&.include?("=") + next if SHELL_METACHARACTERS.any? { |meta| stripped_first_arg_str.include?(meta) } + good_args = split_args.map { |arg| "\"#{arg}\"" }.join(", ") method_string = if target_class "#{target_class}.#{target_method}" @@ -68,6 +106,32 @@ module RuboCop end end end + + # This cop disallows shell metacharacters in `exec` calls. + # + # @api private + class ExecShellMetacharacters < Base + include HelperFunctions + + MSG = "Don't use shell metacharacters in `exec`. "\ + "Implement the logic in Ruby instead, using methods like `$stdout.reopen`." + + RESTRICT_ON_SEND = [:exec].freeze + + def on_send(node) + return if node.receiver.present? && node.receiver != s(:const, nil, :Kernel) + return if node.arguments.count != 1 + + stripped_arg_str = string_content(node.arguments.first, strip_dynamic: true) + command = string_content(node.arguments.first).shellsplit.first + + return if SHELL_BUILTINS.none?(command) && + !command&.include?("=") && + SHELL_METACHARACTERS.none? { |meta| stripped_arg_str.include?(meta) } + + add_offense(node.arguments.first, message: MSG) + end + end end end end diff --git a/Library/Homebrew/test/rubocops/shell_commands_spec.rb b/Library/Homebrew/test/rubocops/shell_commands_spec.rb index 60ee8aac3a..9e142b7207 100644 --- a/Library/Homebrew/test/rubocops/shell_commands_spec.rb +++ b/Library/Homebrew/test/rubocops/shell_commands_spec.rb @@ -3,210 +3,239 @@ require "rubocops/shell_commands" -describe RuboCop::Cop::Style::ShellCommands do - subject(:cop) { described_class.new } +module RuboCop + module Cop + module Style + describe ShellCommands do + subject(:cop) { described_class.new } - context "when auditing shell commands" do - it "reports and corrects an offense when `system` arguments should be separated" do - expect_offense(<<~RUBY) - class Foo < Formula - def install - system "foo bar" - ^^^^^^^^^ Separate `system` commands into `\"foo\", \"bar\"` + context "when auditing shell commands" do + it "reports and corrects an offense when `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 + + expect_correction(<<~RUBY) + class Foo < Formula + def install + system "foo", "bar" + end + end + RUBY + end + + it "reports and corrects an offense when `system` arguments involving 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 + + expect_correction(<<~RUBY) + class Foo < Formula + def install + system "\#{bin}/foo", "bar" + end + end + RUBY + end + + it "reports no offenses when `system` with metacharacter arguments are called" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + def install + system "foo bar > baz" + end + end + RUBY + end + + it "reports no offenses when trailing arguments to `system` are unseparated" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + def install + system "foo", "bar baz" + end + end + RUBY + end + + it "reports no offenses when `Utils.popen` arguments are unseparated" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + def install + Utils.popen("foo bar") + end + end + RUBY + end + + it "reports and corrects an offense when `Utils.popen_read` arguments are unseparated" 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 + + expect_correction(<<~RUBY) + class Foo < Formula + def install + Utils.popen_read("foo", "bar") + end + end + RUBY + end + + it "reports and corrects an offense when `Utils.safe_popen_read` arguments are unseparated" 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 + + expect_correction(<<~RUBY) + class Foo < Formula + def install + Utils.safe_popen_read("foo", "bar") + end + end + RUBY + end + + it "reports and corrects an offense when `Utils.popen_write` arguments are unseparated" 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 + + expect_correction(<<~RUBY) + class Foo < Formula + def install + Utils.popen_write("foo", "bar") + end + end + RUBY + end + + it "reports and corrects an offense when `Utils.safe_popen_write` arguments are unseparated" 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 + + expect_correction(<<~RUBY) + class Foo < Formula + def install + Utils.safe_popen_write("foo", "bar") + end + end + RUBY + end + + it "reports and corrects an offense when `Utils.popen_read` arguments with interpolation are unseparated" 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 + + expect_correction(<<~RUBY) + class Foo < Formula + def install + Utils.popen_read("\#{bin}/foo", "bar") + end + end + RUBY + end + + it "reports no offenses when `Utils.popen_read` arguments with metacharacters are unseparated" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + def install + Utils.popen_read("foo bar > baz") + end + end + RUBY + end + + it "reports no offenses when trailing arguments to `Utils.popen_read` are unseparated" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + def install + Utils.popen_read("foo", "bar baz") + end + end + RUBY + end + + it "reports and corrects an offense when `Utils.popen_read` arguments are unseparated after a shell env" 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 + + expect_correction(<<~RUBY) + class Foo < Formula + def install + Utils.popen_read({ "SHELL" => "bash"}, "foo", "bar") + end + end + RUBY end end - RUBY + end - expect_correction(<<~RUBY) - class Foo < Formula - def install - system "foo", "bar" + describe ExecShellMetacharacters do + subject(:cop) { described_class.new } + + context "when auditing exec calls" do + it "reports aan offense when output piping is used" do + expect_offense(<<~RUBY) + fork do + exec "foo bar > output" + ^^^^^^^^^^^^^^^^^^ Don\'t use shell metacharacters in `exec`. Implement the logic in Ruby instead, using methods like `$stdout.reopen`. + end + RUBY + end + + it "reports no offenses when no metacharacters are used" do + expect_no_offenses(<<~RUBY) + fork do + exec "foo bar" + end + RUBY end end - RUBY - end - - it "reports and corrects an offense when `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 - - expect_correction(<<~RUBY) - class Foo < Formula - def install - system "\#{bin}/foo", "bar" - end - end - RUBY - end - - it "reports no offenses when `system` with metacharacter arguments are called" do - expect_no_offenses(<<~RUBY) - class Foo < Formula - def install - system "foo bar > baz" - end - end - RUBY - end - - it "reports no offenses when trailing arguments to `system` are unseparated" do - expect_no_offenses(<<~RUBY) - class Foo < Formula - def install - system "foo", "bar baz" - end - end - RUBY - end - - it "reports no offenses when `Utils.popen` arguments are unseparated" do - expect_no_offenses(<<~RUBY) - class Foo < Formula - def install - Utils.popen("foo bar") - end - end - RUBY - end - - it "reports and corrects an offense when `Utils.popen_read` arguments are unseparated" 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 - - expect_correction(<<~RUBY) - class Foo < Formula - def install - Utils.popen_read("foo", "bar") - end - end - RUBY - end - - it "reports and corrects an offense when `Utils.safe_popen_read` arguments are unseparated" 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 - - expect_correction(<<~RUBY) - class Foo < Formula - def install - Utils.safe_popen_read("foo", "bar") - end - end - RUBY - end - - it "reports and corrects an offense when `Utils.popen_write` arguments are unseparated" 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 - - expect_correction(<<~RUBY) - class Foo < Formula - def install - Utils.popen_write("foo", "bar") - end - end - RUBY - end - - it "reports and corrects an offense when `Utils.safe_popen_write` arguments are unseparated" 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 - - expect_correction(<<~RUBY) - class Foo < Formula - def install - Utils.safe_popen_write("foo", "bar") - end - end - RUBY - end - - it "reports and corrects an offense when `Utils.popen_read` arguments with interpolation are unseparated" 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 - - expect_correction(<<~RUBY) - class Foo < Formula - def install - Utils.popen_read("\#{bin}/foo", "bar") - end - end - RUBY - end - - it "reports no offenses when `Utils.popen_read` arguments with metacharacters are unseparated" do - expect_no_offenses(<<~RUBY) - class Foo < Formula - def install - Utils.popen_read("foo bar > baz") - end - end - RUBY - end - - it "reports no offenses when trailing arguments to `Utils.popen_read` are unseparated" do - expect_no_offenses(<<~RUBY) - class Foo < Formula - def install - Utils.popen_read("foo", "bar baz") - end - end - RUBY - end - - it "reports and corrects an offense when `Utils.popen_read` arguments are unseparated after 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 - - expect_correction(<<~RUBY) - class Foo < Formula - def install - Utils.popen_read({ "SHELL" => "bash"}, "foo", "bar") - end - end - RUBY + end end end end