Merge pull request #9171 from joshcooper/rubocop_7x_security

(PUP-1172) Resolve Security cops 7.x

.rubocop_todo.yml

@@ -927,23 +927,6 @@ Naming/VariableName:
 Naming/VariableNumber:
   Enabled: false
 
-Security/Eval:
-  Exclude:
-    - 'lib/puppet/interface/action.rb'
-    - 'lib/puppet/interface/action_builder.rb'
-    - 'lib/puppet/pops/loader/ruby_data_type_instantiator.rb'
-    - 'lib/puppet/pops/loader/ruby_function_instantiator.rb'
-    - 'lib/puppet/pops/loader/ruby_legacy_function_instantiator.rb'
-
-Security/Open:
-  Exclude:
-    - 'lib/puppet/file_system/file_impl.rb'
-    - 'lib/puppet/file_system/posix.rb'
-    - 'lib/puppet/provider/package/appdmg.rb'
-    - 'lib/puppet/provider/package/windows/package.rb'
-    - 'lib/puppet/util/command_line/trollop.rb'
-    - 'lib/puppet/util/execution.rb'
-
 # Configuration parameters: EnforcedStyle, AllowModifiersOnSymbols.
 # SupportedStyles: inline, group
 Style/AccessModifierDeclarations:

examples/enc/regexp_nodes/regexp_nodes.rb

@@ -133,7 +133,7 @@ def matched_in_patternfile?(filepath, matchthis)
     patternlist = []
 
     begin
-      open(filepath).each do |l|
+      File.open(filepath).each do |l|
         l.chomp!
 
         next if l =~ /^$/

lib/puppet/file_system/file_impl.rb

@@ -150,7 +150,7 @@ def lstat(path)
   end
 
   def compare_stream(path, stream)
-    open(path, 0, 'rb') { |this| FileUtils.compare_stream(this, stream) }
+    ::File.open(path, 0, 'rb') { |this| FileUtils.compare_stream(this, stream) }
   end
 
   def chmod(mode, path)

lib/puppet/file_system/posix.rb

@@ -10,7 +10,7 @@ def binread(path)
   # issue this method reimplements the faster 2.0 version that will correctly
   # compare binary File and StringIO streams.
   def compare_stream(path, stream)
-    open(path, 0, 'rb') do |this|
+    ::File.open(path, 'rb') do |this|
       bsize = stream_blksize(this, stream)
       sa = "".force_encoding('ASCII-8BIT')
       sb = "".force_encoding('ASCII-8BIT')

lib/puppet/interface/action.rb

@@ -264,12 +264,14 @@ def #{@name}(#{decl.join(", ")})
 end
 WRAPPER
 
+    # It should be possible to rewrite this code to use `define_method`
+    # instead of `class/instance_eval` since Ruby 1.8 is long dead.
     if @face.is_a?(Class)
-      @face.class_eval do eval wrapper, nil, file, line end
+      @face.class_eval do eval wrapper, nil, file, line end # rubocop:disable Security/Eval
       @face.send(:define_method, internal_name, &block)
       @when_invoked = @face.instance_method(name)
     else
-      @face.instance_eval do eval wrapper, nil, file, line end
+      @face.instance_eval do eval wrapper, nil, file, line end # rubocop:disable Security/Eval
       @face.meta_def(internal_name, &block)
       @when_invoked = @face.method(name).unbind
     end

lib/puppet/interface/action_builder.rb

@@ -4,6 +4,8 @@
 # within the context of a new instance of this class.
 # @api public
 class Puppet::Interface::ActionBuilder
+  extend Forwardable
+
   # The action under construction
   # @return [Puppet::Interface::Action]
   # @api private
@@ -141,15 +143,8 @@ def render_as(value = nil)
     property = setter.to_s.chomp('=')
 
     unless method_defined? property
-      # Using eval because the argument handling semantics are less awful than
-      # when we use the define_method/block version.  The later warns on older
-      # Ruby versions if you pass the wrong number of arguments, but carries
-      # on, which is totally not what we want. --daniel 2011-04-18
-      eval <<-METHOD
-        def #{property}(value)
-          @action.#{property} = value
-        end
-      METHOD
+      # ActionBuilder#<property> delegates to Action#<setter>
+      def_delegator :@action, setter, property
     end
   end
 

lib/puppet/pops/loader/ruby_data_type_instantiator.rb

@@ -19,7 +19,7 @@ def self.create(loader, typed_name, source_ref, ruby_code_string)
     # make the private loader available in a binding to allow it to be passed on
     loader_for_type = loader.private_loader
     here = get_binding(loader_for_type)
-    created = eval(ruby_code_string, here, source_ref, 1)
+    created = eval(ruby_code_string, here, source_ref, 1) # rubocop:disable Security/Eval
     unless created.is_a?(Puppet::Pops::Types::PAnyType)
       raise ArgumentError, _("The code loaded from %{source_ref} did not produce a data type when evaluated. Got '%{klass}'") % { source_ref: source_ref, klass: created.class }
     end

lib/puppet/pops/loader/ruby_function_instantiator.rb

@@ -19,7 +19,7 @@ def self.create(loader, typed_name, source_ref, ruby_code_string)
     # make the private loader available in a binding to allow it to be passed on
     loader_for_function = loader.private_loader
     here = get_binding(loader_for_function)
-    created = eval(ruby_code_string, here, source_ref, 1)
+    created = eval(ruby_code_string, here, source_ref, 1) # rubocop:disable Security/Eval
     unless created.is_a?(Class)
       raise ArgumentError, _("The code loaded from %{source_ref} did not produce a Function class when evaluated. Got '%{klass}'") % { source_ref: source_ref, klass: created.class }
     end

lib/puppet/pops/loader/ruby_legacy_function_instantiator.rb

@@ -37,7 +37,7 @@ def self.create(loader, typed_name, source_ref, ruby_code_string)
       # This will do the 3x loading and define the "function_<name>" and "real_function_<name>" methods
       # in the anonymous module used to hold function definitions.
       #
-      func_info = eval(ruby_code_string, here, source_ref, 1)
+      func_info = eval(ruby_code_string, here, source_ref, 1) # rubocop:disable Security/Eval
 
       # Validate what was loaded
       unless func_info.is_a?(Hash)

lib/puppet/provider/package/appdmg.rb

@@ -66,7 +66,7 @@ def self.installpkgdmg(source, name)
         end
       end
 
-      open(cached_source) do |dmg|
+      File.open(cached_source) do |dmg|
         xml_str = hdiutil "mount", "-plist", "-nobrowse", "-readonly", "-mountrandom", "/tmp", dmg.path
           ptable = Puppet::Util::Plist::parse_plist(xml_str)
           # JJM Filter out all mount-paths into a single array, discard the rest.

lib/puppet/provider/package/windows/package.rb

@@ -43,9 +43,9 @@ def self.with_key(&block)
         [KEY64, KEY32].each do |mode|
           mode |= KEY_READ
           begin
-            open(hive, 'Software\Microsoft\Windows\CurrentVersion\Uninstall', mode) do |uninstall|
+            self.open(hive, 'Software\Microsoft\Windows\CurrentVersion\Uninstall', mode) do |uninstall|
               each_key(uninstall) do |name, wtime|
-                open(hive, "#{uninstall.keyname}\\#{name}", mode) do |key|
+                self.open(hive, "#{uninstall.keyname}\\#{name}", mode) do |key|
                   yield key, values_by_name(key, reg_value_names_to_load)
                 end
               end

lib/puppet/util/command_line/trollop.rb

@@ -649,7 +649,7 @@ def parse_io_parameter param, arg
     else
       require 'open-uri'
       begin
-        open param
+        URI.parse(param).open
       rescue SystemCallError => e
         raise CommandlineError, _("file or url for option '%{arg}' cannot be opened: %{value0}") % { arg: arg, value0: e.message }, e.backtrace
       end

lib/puppet/util/execution.rb

@@ -77,7 +77,8 @@ def self.execpipe(command, failonfail = true)
     # a predictable output
     english_env = ENV.to_hash.merge( {'LANG' => 'C', 'LC_ALL' => 'C'} )
     output = Puppet::Util.withenv(english_env) do
-      open("| #{command_str} 2>&1") do |pipe|
+      # We are intentionally using 'pipe' with open to launch a process
+      open("| #{command_str} 2>&1") do |pipe| # rubocop:disable Security/Open
         yield pipe
       end
     end

spec/unit/provider/package/appdmg_spec.rb

@@ -11,7 +11,7 @@
     before do
       fh = double('filehandle', path: '/tmp/foo')
       resource[:source] = "foo.dmg"
-      allow(described_class).to receive(:open).and_yield(fh)
+      allow(File).to receive(:open).and_yield(fh)
       allow(Dir).to receive(:mktmpdir).and_return("/tmp/testtmp123")
       allow(FileUtils).to receive(:remove_entry_secure)
     end