[Puppet-dev] (puppetlabs/facter) (#6955) Remove relative dirs from fact search path

[wearetherobots]

Please review pull request #203: (#6955) Remove relative dirs from fact search path opened by (jeffweiss)

Description:

Prior to this commit, relative directories were permitted the search
path for facts, which meant that both the user and the path in which
facter was run could influence which facts were located. This situation
could have led in unintended code being executed.

This commit forces all paths (from $LOAD_PATH, ENV["FACTORLIB"] or
Facter.search_path) to explicitly be absolute paths.

• Opened: Sat May 12 05:42:04 UTC 2012
• Based on: puppetlabs:master (1bd98fd94b5e577216b80789ee9f933dd5cc74f8)
• Requested merge: jeffweiss:ticket/master/6955_remove_relative_dirs_from_search_path (4a2d358ce6e390038e13d529b218c727ce9a45db)

Diff follows:

diff --git a/lib/facter/util/loader.rb b/lib/facter/util/loader.rb
index 4bdc82f..13266d8 100644
--- a/lib/facter/util/loader.rb
+++ b/lib/facter/util/loader.rb
@@ -1,10 +1,12 @@
 require 'facter'
+require 'pathname'
 
 # Load facts on demand.
 class Facter::Util::Loader
 
   def initialize
     @loaded = []
+    @valid_path = {}
   end
 
   # Load all resolutions for a single fact.
@@ -53,14 +55,21 @@ def search_path
     result = []
     result += $LOAD_PATH.collect { |d| File.join(d, "facter") }
     if ENV.include?("FACTERLIB")
-      result += ENV["FACTERLIB"].split(":")
+      result += ENV["FACTERLIB"].split(File::PATH_SEPARATOR)
     end
 
     # This allows others to register additional paths we should search.
     result += Facter.search_path
 
-    result
+    result.select { |dir| valid_search_path? dir }
   end
+  
+  def valid_search_path?(path)
+    return @valid_path[path] unless @valid_path[path].nil?
+    
+    return @valid_path[path] = Pathname.new(path).absolute?
+  end
+  private :valid_search_path?
 
   private
 
diff --git a/spec/unit/util/loader_spec.rb b/spec/unit/util/loader_spec.rb
index 7653484..2848742 100755
--- a/spec/unit/util/loader_spec.rb
+++ b/spec/unit/util/loader_spec.rb
@@ -33,6 +33,62 @@ def load_file(file)
     Facter::Util::Loader.new.should respond_to(:search_path)
   end
 
+  describe "#valid_seach_path?" do
+    before do
+      @loader = Facter::Util::Loader.new
+      @settings = mock 'settings'
+      @settings.stubs(:value).returns "/eh"
+    end
+    
+    it "should cache the result of a previous check" do
+      Pathname.any_instance.expects(:absolute?).returns(true).once
+      
+      # we explicitly want two calls here to check that we get
+      # the second from the cache
+      @loader.should be_valid_search_path "/foo"
+      @loader.should be_valid_search_path "/foo"
+    end
+    
+    [
+      '.',
+      '..',
+      '...',
+      '.foo',
+      '../foo',
+      'foo',
+      'foo/bar',
+      'foo/../bar',
+      ' ',
+      ' /',
+      ' \/',
+    ].each do |dir|
+      
+      it "should be false for relative path #{dir}" do
+        @loader.should_not be_valid_search_path dir
+      end
+      
+    end
+    
+    [
+      '/.',
+      '/..',
+      '/...',
+      '/.foo',
+      '/../foo',
+      '/foo',
+      '/foo/bar',
+      '/foo/../bar',
+      '/ ',
+      '/ /..',
+    ].each do |dir|
+      
+      it "should be true for absolute path #{dir}" do
+        @loader.should be_valid_search_path dir
+      end
+      
+    end
+  end
+
   describe "when determining the search path" do
     before do
       @loader = Facter::Util::Loader.new
@@ -42,12 +98,22 @@ def load_file(file)
 
     it "should include the facter subdirectory of all paths in ruby LOAD_PATH" do
       dirs = $LOAD_PATH.collect { |d| File.join(d, "facter") }
+      @loader.stubs(:valid_search_path?).returns(true)
       paths = @loader.search_path
 
       dirs.each do |dir|
         paths.should be_include(dir)
       end
     end
+    
+    it "should exclude invalid search paths" do
+      dirs = $LOAD_PATH.collect { |d| File.join(d, "facter") }
+      @loader.stubs(:valid_search_path?).returns(false)
+      paths = @loader.search_path
+      dirs.each do |dir|
+        paths.should_not be_include(dir)
+      end
+    end
 
     it "should include all search paths registered with Facter" do
       Facter.expects(:search_path).returns %w{/one /two}

    

--
You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to puppet-dev@googlegroups.com.
To unsubscribe from this group, send email to puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.