(PUP-11772) Resolve Security/Open

When opening a file path, use File.open When opening a URL, use URI.parse(..).open The Windows package class includes our Registry module which defines `open`. Use the fully qualified name to avoid rubocop confusion.
puppetlabs · Nov 21, 2023 · 283ba4c · 283ba4c
1 parent 03b3df3
commit 283ba4c
Show file tree

Hide file tree

Showing 9 changed files with 10 additions and 18 deletions.
diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
@@ -926,15 +926,6 @@ Security/Eval:
     - '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:

diff --git a/examples/enc/regexp_nodes/regexp_nodes.rb b/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 =~ /^$/

diff --git a/lib/puppet/file_system/file_impl.rb b/lib/puppet/file_system/file_impl.rb
@@ -151,7 +151,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)

diff --git a/lib/puppet/file_system/posix.rb b/lib/puppet/file_system/posix.rb
@@ -11,7 +11,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 = String.new.force_encoding('ASCII-8BIT')
       sb = String.new.force_encoding('ASCII-8BIT')

diff --git a/lib/puppet/provider/package/appdmg.rb b/lib/puppet/provider/package/appdmg.rb
@@ -67,7 +67,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.

diff --git a/lib/puppet/provider/package/windows/package.rb b/lib/puppet/provider/package/windows/package.rb
@@ -44,9 +44,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

diff --git a/lib/puppet/util/command_line/trollop.rb b/lib/puppet/util/command_line/trollop.rb
@@ -650,7 +650,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

diff --git a/lib/puppet/util/execution.rb b/lib/puppet/util/execution.rb
@@ -78,7 +78,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

diff --git a/spec/unit/provider/package/appdmg_spec.rb b/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