From 0faf17b1e60e1dda562488802c8914d7956000cc Mon Sep 17 00:00:00 2001
From: Rylan Polster <rslpolster@gmail.com>
Date: Thu, 4 Jun 2020 21:05:38 -0400
Subject: [PATCH] use safe_popen_read instead of popen_read

RuboCop requires using Utils.safe_popen_read and Utils.safe_popen_write
instead of Utils.popen_read and Utils.popen_write respectively.

Using the "safe" version means that an error will be shown if the
command fails. Previously, when using `popen_read`, a failed command can
go unnoticed and have negative consequences that go undetected (as
happened for pipend in
https://github.com/Homebrew/homebrew-core/pull/55682)

RuboCop does not require Utils.safe_popen_read in a test do block
because there can be some legitimate uses for Utils.popen_read in these
cases.
---
 Library/Homebrew/rubocops/lines.rb           | 28 ++++++
 Library/Homebrew/test/rubocops/lines_spec.rb | 95 ++++++++++++++++++++
 2 files changed, 123 insertions(+)

diff --git a/Library/Homebrew/rubocops/lines.rb b/Library/Homebrew/rubocops/lines.rb
index 86b25ecebf..38e26af8b5 100644
--- a/Library/Homebrew/rubocops/lines.rb
+++ b/Library/Homebrew/rubocops/lines.rb
@@ -195,6 +195,34 @@ module RuboCop
         end
       end
 
+      class ShellCmd < FormulaCop
+        def audit_formula(_node, _class_node, _parent_class_node, body_node)
+          test = find_block(body_node, :test)
+
+          [:popen_read, :popen_write].each do |unsafe_command|
+            test_methods = []
+
+            unless test.nil?
+              find_instance_method_call(test, "Utils", unsafe_command) do |method|
+                test_methods << method.source_range
+              end
+            end
+
+            find_instance_method_call(body_node, "Utils", unsafe_command) do |method|
+              unless test_methods.include?(method.source_range)
+                problem "Use `Utils.safe_#{unsafe_command}` instead of `Utils.#{unsafe_command}`"
+              end
+            end
+          end
+        end
+
+        def autocorrect(node)
+          lambda do |corrector|
+            corrector.replace(node.loc.selector, "safe_#{node.method_name}")
+          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 19545be896..bb31d2e935 100644
--- a/Library/Homebrew/test/rubocops/lines_spec.rb
+++ b/Library/Homebrew/test/rubocops/lines_spec.rb
@@ -345,6 +345,101 @@ describe RuboCop::Cop::FormulaAudit::MpiCheck do
   end
 end
 
+describe RuboCop::Cop::FormulaAudit::ShellCmd do
+  subject(:cop) { described_class.new }
+
+  context "When auditing shell commands" do
+    it "Utils.popen_read should become Utils.safe_popen_read" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.popen_read "foo"
+            ^^^^^^^^^^^^^^^^^^^^^^ Use `Utils.safe_popen_read` instead of `Utils.popen_read`
+          end
+        end
+      RUBY
+    end
+
+    it "Utils.safe_popen_write should become Utils.popen_write" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.popen_write "foo"
+            ^^^^^^^^^^^^^^^^^^^^^^^ Use `Utils.safe_popen_write` instead of `Utils.popen_write`
+          end
+        end
+      RUBY
+    end
+
+    it "does not correct Utils.popen_read in test block" do
+      expect_no_offenses(<<~RUBY)
+        class Foo < Formula
+          def install; end
+          test do
+            Utils.popen_read "foo"
+          end
+        end
+      RUBY
+    end
+
+    it "corrects Utils.popen_read to Utils.safe_popen_read" do
+      source = <<~RUBY
+        class Foo < Formula
+          def install
+            Utils.popen_read "foo"
+          end
+        end
+      RUBY
+
+      corrected_source = <<~RUBY
+        class Foo < Formula
+          def install
+            Utils.safe_popen_read "foo"
+          end
+        end
+      RUBY
+
+      new_source = autocorrect_source(source)
+      expect(new_source).to eq(corrected_source)
+    end
+
+    it "corrects Utils.popen_write to Utils.safe_popen_write" do
+      source = <<~RUBY
+        class Foo < Formula
+          def install
+            Utils.popen_write "foo"
+          end
+        end
+      RUBY
+
+      corrected_source = <<~RUBY
+        class Foo < Formula
+          def install
+            Utils.safe_popen_write "foo"
+          end
+        end
+      RUBY
+
+      new_source = autocorrect_source(source)
+      expect(new_source).to eq(corrected_source)
+    end
+
+    it "does not correct to Utils.safe_popen_read in test block" do
+      source = <<~RUBY
+        class Foo < Formula
+          def install; end
+          test do
+            Utils.popen_write "foo"
+          end
+        end
+      RUBY
+
+      new_source = autocorrect_source(source)
+      expect(new_source).to eq(source)
+    end
+  end
+end
+
 describe RuboCop::Cop::FormulaAudit::Miscellaneous do
   subject(:cop) { described_class.new }