Fix issues with Windows based file URIs

Previously, specifying a Windows file URI of the form 'file:///C:/foo'
as a file source failed to strip the leading slash when attempting to
source the file. Also there was ambiguity after values were munged (a
value of the form 'C:/foo' could either be a Windows file path or a
URI whose scheme is 'C').

This commit changes the file source to be more deliberate in how it
validates source properties, including only allowing absolute paths
and 'puppet' and 'file' URIs, which are both absolute and
hierarchical. Also it uses the Puppet::Util.path_to_uri method to
handle file path to URI translation issues.

Previously, if a request was created using a Windows file URI of the
form 'file:///C:/foo', then the set_uri_key method wasn't stripping
the leading slash, and setting the request key to '/C:/foo'. This
caused problems when attempting to collect metadata for Windows
files. This commit changes the request class to use the
Puppet::Util.uri_to_path method to handle URI path to file path
translation issues.

Previously, if a file URI was created programmatically using ruby's
built-in URI class, such as occurs when specifying file source URIs,
then calling URI#to_s omits the authority component, e.g. 'file:/foo'
instead of 'file:///foo'. This commit changes the URI regex to not
require two slashes, but note that the order of operations is
important as Windows file paths will match the URI regex and can be
successfully parsed: URI.parse('c:/foo').scheme == 'c'

lib/puppet/indirector/request.rb

@@ -76,9 +76,8 @@ def initialize(indirection_name, method, key_or_instance, options_or_instance =
       # because it rewrites the key.  We could otherwise strip server/port/etc
       # info out in the REST class, but it seemed bad design for the REST
       # class to rewrite the key.
-      if key.to_s =~ /^[a-z]:[\/\\]/i # It's an absolute path for Windows.
-        @key = key
-      elsif key.to_s =~ /^\w+:\/\// # it's a URI
+
+      if key.to_s =~ /^\w+:\// and not Puppet::Util.absolute_path?(key.to_s) # it's a URI
         set_uri_key(key)
       else
         @key = key
@@ -173,7 +172,7 @@ def set_uri_key(key)
 
     # Just short-circuit these to full paths
     if uri.scheme == "file"
-      @key = URI.unescape(uri.path)
+      @key = Puppet::Util.uri_to_path(uri)
       return
     end
 

lib/puppet/type/file/source.rb

@@ -66,21 +66,33 @@ class sendmail {
     validate do |sources|
       sources = [sources] unless sources.is_a?(Array)
       sources.each do |source|
+        next if Puppet::Util.absolute_path?(source)
+
         begin
           uri = URI.parse(URI.escape(source))
         rescue => detail
           self.fail "Could not understand source #{source}: #{detail}"
         end
 
-        self.fail "Cannot use URLs of type '#{uri.scheme}' as source for fileserving" unless uri.scheme.nil? or %w{file puppet}.include?(uri.scheme) or (Puppet.features.microsoft_windows? and uri.scheme =~ /^[a-z]$/i)
+        self.fail "Cannot use relative URLs '#{source}'" unless uri.absolute?
+        self.fail "Cannot use opaque URLs '#{source}'" unless uri.hierarchical?
+        self.fail "Cannot use URLs of type '#{uri.scheme}' as source for fileserving" unless %w{file puppet}.include?(uri.scheme)
       end
     end
 
     SEPARATOR_REGEX = [Regexp.escape(File::SEPARATOR.to_s), Regexp.escape(File::ALT_SEPARATOR.to_s)].join
 
     munge do |sources|
       sources = [sources] unless sources.is_a?(Array)
-      sources.collect { |source| source.sub(/[#{SEPARATOR_REGEX}]+$/, '') }
+      sources.map do |source|
+        source = source.sub(/[#{SEPARATOR_REGEX}]+$/, '')
+
+        if Puppet::Util.absolute_path?(source)
+          URI.unescape(Puppet::Util.path_to_uri(source).to_s)
+        else
+          source
+        end
+      end
     end
 
     def change_to_s(currentvalue, newvalue)
@@ -164,11 +176,11 @@ def metadata
     end
 
     def local?
-      found? and uri and (uri.scheme || "file") == "file"
+      found? and scheme == "file"
     end
 
     def full_path
-      URI.unescape(uri.path) if found? and uri
+      Puppet::Util.uri_to_path(uri) if found?
     end
 
     def server
@@ -178,12 +190,13 @@ def server
     def port
       (uri and uri.port) or Puppet.settings[:masterport]
     end
-
     private
 
-    def uri
-      return nil if metadata.source =~ /^[a-z]:[\/\\]/i # Abspath for Windows
+    def scheme
+      (uri and uri.scheme)
+    end
 
+    def uri
       @uri ||= URI.parse(URI.escape(metadata.source))
     end
   end

spec/integration/type/file_spec.rb

@@ -314,6 +314,195 @@ def build_path(dir)
 
       (get_mode(generated) & 007777).should == managed_mode
     end
+
+    describe "when recursing remote directories" do
+      describe "when sourceselect first" do
+        describe "for a directory" do
+          it "should recursively copy the first directory that exists" do
+            one = File.expand_path('thisdoesnotexist')
+            two = tmpdir('two')
+
+            FileUtils.mkdir_p(File.join(two, 'three'))
+            FileUtils.touch(File.join(two, 'three', 'four'))
+
+            obj = Puppet::Type.newfile(
+                               :path    => path,
+                               :ensure  => :directory,
+                               :backup  => false,
+                               :recurse => true,
+                               :sourceselect => :first,
+                               :source => [one, two]
+                               )
+
+            catalog.add_resource obj
+            catalog.apply
+
+            File.should be_directory(path)
+            File.should_not be_exist(File.join(path, 'one'))
+            File.should be_exist(File.join(path, 'three', 'four'))
+          end
+
+          it "should recursively copy an empty directory" do
+            one = File.expand_path('thisdoesnotexist')
+            two = tmpdir('two')
+            three = tmpdir('three')
+
+            FileUtils.mkdir_p(two)
+            FileUtils.mkdir_p(three)
+            FileUtils.touch(File.join(three, 'a'))
+
+            obj = Puppet::Type.newfile(
+                               :path    => path,
+                               :ensure  => :directory,
+                               :backup  => false,
+                               :recurse => true,
+                               :sourceselect => :first,
+                               :source => [one, two, three]
+                               )
+
+            catalog.add_resource obj
+            catalog.apply
+
+            File.should be_directory(path)
+            File.should_not be_exist(File.join(path, 'a'))
+          end
+
+          it "should only recurse one level" do
+            one = tmpdir('one')
+            FileUtils.mkdir_p(File.join(one, 'a', 'b'))
+            FileUtils.touch(File.join(one, 'a', 'b', 'c'))
+
+            two = tmpdir('two')
+            FileUtils.mkdir_p(File.join(two, 'z'))
+            FileUtils.touch(File.join(two, 'z', 'y'))
+
+            obj = Puppet::Type.newfile(
+                               :path    => path,
+                               :ensure  => :directory,
+                               :backup  => false,
+                               :recurse => true,
+                               :recurselimit => 1,
+                               :sourceselect => :first,
+                               :source => [one, two]
+                               )
+
+            catalog.add_resource obj
+            catalog.apply
+
+            File.should be_exist(File.join(path, 'a'))
+            File.should_not be_exist(File.join(path, 'a', 'b'))
+            File.should_not be_exist(File.join(path, 'z'))
+          end
+        end
+
+        describe "for a file" do
+          it "should copy the first file that exists" do
+            one = File.expand_path('thisdoesnotexist')
+            two = tmpfile('two')
+            File.open(two, "w") { |f| f.print 'yay' }
+            three = tmpfile('three')
+            File.open(three, "w") { |f| f.print 'no' }
+
+            obj = Puppet::Type.newfile(
+                               :path    => path,
+                               :ensure  => :file,
+                               :backup  => false,
+                               :sourceselect => :first,
+                               :source => [one, two, three]
+                               )
+
+            catalog.add_resource obj
+            catalog.apply
+
+            File.read(path).should == 'yay'
+          end
+
+          it "should copy an empty file" do
+            one = File.expand_path('thisdoesnotexist')
+            two = tmpfile('two')
+            FileUtils.touch(two)
+            three = tmpfile('three')
+            File.open(three, "w") { |f| f.print 'no' }
+
+            obj = Puppet::Type.newfile(
+                               :path    => path,
+                               :ensure  => :file,
+                               :backup  => false,
+                               :sourceselect => :first,
+                               :source => [one, two, three]
+                               )
+
+            catalog.add_resource obj
+            catalog.apply
+
+            File.read(path).should == ''
+          end
+        end
+      end
+
+      describe "when sourceselect all" do
+        describe "for a directory" do
+          it "should recursively copy all sources from the first valid source" do
+            one = tmpdir('one')
+            two = tmpdir('two')
+            three = tmpdir('three')
+            four = tmpdir('four')
+
+            [one, two, three, four].each {|dir| FileUtils.mkdir_p(dir)}
+
+            File.open(File.join(one, 'a'), "w") { |f| f.print one }
+            File.open(File.join(two, 'a'), "w") { |f| f.print two }
+            File.open(File.join(two, 'b'), "w") { |f| f.print two }
+            File.open(File.join(three, 'a'), "w") { |f| f.print three }
+            File.open(File.join(three, 'c'), "w") { |f| f.print three }
+
+            obj = Puppet::Type.newfile(
+                               :path    => path,
+                               :ensure  => :directory,
+                               :backup  => false,
+                               :recurse => true,
+                               :sourceselect => :all,
+                               :source => [one, two, three, four]
+                               )
+
+            catalog.add_resource obj
+            catalog.apply
+
+            File.read(File.join(path, 'a')).should == one
+            File.read(File.join(path, 'b')).should == two
+            File.read(File.join(path, 'c')).should == three
+          end
+
+          it "should only recurse one level from each valid source" do
+            one = tmpdir('one')
+            FileUtils.mkdir_p(File.join(one, 'a', 'b'))
+            FileUtils.touch(File.join(one, 'a', 'b', 'c'))
+
+            two = tmpdir('two')
+            FileUtils.mkdir_p(File.join(two, 'z'))
+            FileUtils.touch(File.join(two, 'z', 'y'))
+
+            obj = Puppet::Type.newfile(
+                               :path    => path,
+                               :ensure  => :directory,
+                               :backup  => false,
+                               :recurse => true,
+                               :recurselimit => 1,
+                               :sourceselect => :all,
+                               :source => [one, two]
+                               )
+
+            catalog.add_resource obj
+            catalog.apply
+
+            File.should be_exist(File.join(path, 'a'))
+            File.should_not be_exist(File.join(path, 'a', 'b'))
+            File.should be_exist(File.join(path, 'z'))
+            File.should_not be_exist(File.join(path, 'z', 'y'))
+          end
+        end
+      end
+    end
   end
 
   describe "when generating resources" do

spec/shared_behaviours/file_serving.rb

@@ -43,13 +43,14 @@
   end
 
   it "should use the file terminus when the 'file' URI scheme is used" do
-    uri = "file:///fakemod/my/file"
+    uri = Puppet::Util.path_to_uri(File.expand_path('/fakemod/my/other file'))
+    uri.scheme.should == 'file'
     @indirection.terminus(:file).expects(:find)
-    @indirection.find(uri)
+    @indirection.find(uri.to_s)
   end
 
   it "should use the file terminus when a fully qualified path is provided" do
-    uri = "/fakemod/my/file"
+    uri = File.expand_path("/fakemod/my/file")
     @indirection.terminus(:file).expects(:find)
     @indirection.find(uri)
   end

spec/unit/configurer/downloader_spec.rb

@@ -97,8 +97,8 @@
 
   describe "when creating the catalog to do the downloading" do
     before do
-      @path = make_absolute("/download/path")
-      @dler = Puppet::Configurer::Downloader.new("foo", @path, "source")
+      @path = File.expand_path("/download/path")
+      @dler = Puppet::Configurer::Downloader.new("foo", @path, File.expand_path("source"))
     end
 
     it "should create a catalog and add the file to it" do

spec/unit/indirector/direct_file_server_spec.rb

@@ -17,20 +17,21 @@ module Testing; end
 
     @server = @direct_file_class.new
 
-    @uri = "file:///my/local"
+    @path = File.expand_path('/my/local')
+    @uri = Puppet::Util.path_to_uri(@path).to_s
 
     @request = Puppet::Indirector::Request.new(:mytype, :find, @uri)
   end
 
   describe Puppet::Indirector::DirectFileServer, "when finding a single file" do
 
     it "should return nil if the file does not exist" do
-      FileTest.expects(:exists?).with("/my/local").returns false
+      FileTest.expects(:exists?).with(@path).returns false
       @server.find(@request).should be_nil
     end
 
     it "should return a Content instance created with the full path to the file if the file exists" do
-      FileTest.expects(:exists?).with("/my/local").returns true
+      FileTest.expects(:exists?).with(@path).returns true
       @model.expects(:new).returns(:mycontent)
       @server.find(@request).should == :mycontent
     end
@@ -41,11 +42,11 @@ module Testing; end
     before do
       @data = mock 'content'
       @data.stubs(:collect)
-      FileTest.expects(:exists?).with("/my/local").returns true
+      FileTest.expects(:exists?).with(@path).returns true
     end
 
     it "should pass the full path to the instance" do
-      @model.expects(:new).with { |key, options| key == "/my/local" }.returns(@data)
+      @model.expects(:new).with { |key, options| key == @path }.returns(@data)
       @server.find(@request)
     end
 
@@ -60,19 +61,19 @@ module Testing; end
 
   describe Puppet::Indirector::DirectFileServer, "when searching for multiple files" do
     it "should return nil if the file does not exist" do
-      FileTest.expects(:exists?).with("/my/local").returns false
+      FileTest.expects(:exists?).with(@path).returns false
       @server.find(@request).should be_nil
     end
 
     it "should use :path2instances from the terminus_helper to return instances if the file exists" do
-      FileTest.expects(:exists?).with("/my/local").returns true
+      FileTest.expects(:exists?).with(@path).returns true
       @server.expects(:path2instances)
       @server.search(@request)
     end
 
     it "should pass the original request to :path2instances" do
-      FileTest.expects(:exists?).with("/my/local").returns true
-      @server.expects(:path2instances).with(@request, "/my/local")
+      FileTest.expects(:exists?).with(@path).returns true
+      @server.expects(:path2instances).with(@request, @path)
       @server.search(@request)
     end
   end