Merge pull request #3398 from chef/jdm/1848

Allow spaces in files for remote_file

7 files changed, 82 insertions, 23 deletions

diff --git a/lib/chef/mixin/uris.rb b/lib/chef/mixin/uris.rb
index 6c1ae793e2..0136b55f6a 100644
--- a/lib/chef/mixin/uris.rb
+++ b/lib/chef/mixin/uris.rb
@@ -28,6 +28,17 @@ class Chef
         # From open-uri
         !!(%r{\A[A-Za-z][A-Za-z0-9+\-\.]*://} =~ source)
       end
+
+
+      def as_uri(source)
+        begin
+          URI.parse(source)
+        rescue URI::InvalidURIError
+          Chef::Log.warn("#{source} was an invalid URI. Trying to escape invalid characters")
+          URI.parse(URI.escape(source))
+        end
+      end
+
     end
   end
 end
diff --git a/lib/chef/provider/remote_file/content.rb b/lib/chef/provider/remote_file/content.rb
index 938a7e58f4..4f450ce333 100644
--- a/lib/chef/provider/remote_file/content.rb
+++ b/lib/chef/provider/remote_file/content.rb
@@ -20,6 +20,7 @@
 require 'uri'
 require 'tempfile'
 require 'chef/file_content_management/content_base'
+require 'chef/mixin/uris'
 
 class Chef
   class Provider
@@ -28,6 +29,8 @@ class Chef
 
         private
 
+        include Chef::Mixin::Uris
+
         def file_for_provider
           Chef::Log.debug("#{@new_resource} checking for changes")
 
@@ -48,7 +51,7 @@ class Chef
             uri = if Chef::Provider::RemoteFile::Fetcher.network_share?(source)
               source
             else
-              URI.parse(source)
+              as_uri(source)
             end
             raw_file = grab_file_from_uri(uri)
           rescue SocketError, Errno::ECONNREFUSED, Errno::ENOENT, Errno::EACCES, Timeout::Error, Net::HTTPServerException, Net::HTTPFatalError, Net::FTPError => e
diff --git a/lib/chef/provider/remote_file/local_file.rb b/lib/chef/provider/remote_file/local_file.rb
index e78311f2c3..026206b64e 100644
--- a/lib/chef/provider/remote_file/local_file.rb
+++ b/lib/chef/provider/remote_file/local_file.rb
@@ -32,15 +32,21 @@ class Chef
           @new_resource = new_resource
           @uri = uri
         end
-        
+
         # CHEF-4472: Remove the leading slash from windows paths that we receive from a file:// URI
-        def fix_windows_path(path) 
-          path.gsub(/^\/([a-zA-Z]:)/,'\1')  
+        def fix_windows_path(path)
+          path.gsub(/^\/([a-zA-Z]:)/,'\1')
+        end
+
+        def source_path
+          @source_path ||= begin
+            path = URI.unescape(uri.path)
+            Chef::Platform.windows? ? fix_windows_path(path) : path
+          end
         end
 
         # Fetches the file at uri, returning a Tempfile-like File handle
         def fetch
-          source_path = Chef::Platform.windows? ? fix_windows_path(uri.path) : uri.path
           tempfile = Chef::FileContentManagement::Tempfile.new(new_resource).tempfile
           Chef::Log.debug("#{new_resource} staging #{source_path} to #{tempfile.path}")
           FileUtils.cp(source_path, tempfile.path)
diff --git a/lib/chef/resource/remote_file.rb b/lib/chef/resource/remote_file.rb
index 69614f0298..df9fb7ad76 100644
--- a/lib/chef/resource/remote_file.rb
+++ b/lib/chef/resource/remote_file.rb
@@ -21,6 +21,7 @@ require 'uri'
 require 'chef/resource/file'
 require 'chef/provider/remote_file'
 require 'chef/mixin/securable'
+require 'chef/mixin/uris'
 
 class Chef
   class Resource
@@ -127,6 +128,8 @@ class Chef
 
       private
 
+      include Chef::Mixin::Uris
+
       def validate_source(source)
         source = Array(source).flatten
         raise ArgumentError, "#{resource_name} has an empty source" if source.empty?
@@ -140,7 +143,7 @@ class Chef
       end
 
       def absolute_uri?(source)
-        Chef::Provider::RemoteFile::Fetcher.network_share?(source) or (source.kind_of?(String) and URI.parse(source).absolute?) 
+        Chef::Provider::RemoteFile::Fetcher.network_share?(source) or (source.kind_of?(String) and as_uri(source).absolute?)
       rescue URI::InvalidURIError
         false
       end
diff --git a/spec/unit/mixin/uris_spec.rb b/spec/unit/mixin/uris_spec.rb
index 07e4f34508..d4985c4f67 100644
--- a/spec/unit/mixin/uris_spec.rb
+++ b/spec/unit/mixin/uris_spec.rb
@@ -26,20 +26,32 @@ end
 describe Chef::Mixin::Uris do
   let (:uris) { Chef::UrisTest.new }
 
-  it "matches 'scheme://foo.com'" do
-    expect(uris.uri_scheme?('scheme://foo.com')).to eq(true)
+  describe "#uri_scheme?" do
+    it "matches 'scheme://foo.com'" do
+      expect(uris.uri_scheme?('scheme://foo.com')).to eq(true)
+    end
+
+    it "does not match 'c:/foo.com'" do
+      expect(uris.uri_scheme?('c:/foo.com')).to eq(false)
+    end
+
+    it "does not match '/usr/bin/foo.com'" do
+      expect(uris.uri_scheme?('/usr/bin/foo.com')).to eq(false)
+    end
+
+    it "does not match 'c:/foo.com://bar.com'" do
+      expect(uris.uri_scheme?('c:/foo.com://bar.com')).to eq(false)
+    end
   end
 
-  it "does not match 'c:/foo.com'" do
-    expect(uris.uri_scheme?('c:/foo.com')).to eq(false)
-  end
-
-  it "does not match '/usr/bin/foo.com'" do
-    expect(uris.uri_scheme?('/usr/bin/foo.com')).to eq(false)
-  end
+  describe "#as_uri" do
+    it "parses a file scheme uri with spaces" do
+      expect{ uris.as_uri("file:///c:/foo bar.txt") }.not_to raise_exception
+    end
 
-  it "does not match 'c:/foo.com://bar.com'" do
-    expect(uris.uri_scheme?('c:/foo.com://bar.com')).to eq(false)
+    it "returns a URI object" do
+      expect( uris.as_uri("file:///c:/foo bar.txt") ).to be_a(URI)
+    end
   end
 
 end
diff --git a/spec/unit/provider/remote_file/local_file_spec.rb b/spec/unit/provider/remote_file/local_file_spec.rb
index b33d82f624..575996a540 100644
--- a/spec/unit/provider/remote_file/local_file_spec.rb
+++ b/spec/unit/provider/remote_file/local_file_spec.rb
@@ -25,26 +25,45 @@ describe Chef::Provider::RemoteFile::LocalFile do
   let(:new_resource) { Chef::Resource::RemoteFile.new("local file backend test (new_resource)") }
   let(:current_resource) { Chef::Resource::RemoteFile.new("local file backend test (current_resource)") }
   subject(:fetcher) { Chef::Provider::RemoteFile::LocalFile.new(uri, new_resource, current_resource) }
-  
-  context "when parsing source path" do
+
+  context "when parsing source path on windows" do
+
+    before do
+      allow(Chef::Platform).to receive(:windows?).and_return(true)
+    end
+
     describe "when given local unix path" do
       let(:uri) { URI.parse("file:///nyan_cat.png") }
       it "returns a correct unix path" do
-        expect(fetcher.fix_windows_path(uri.path)).to eq("/nyan_cat.png")
+        expect(fetcher.source_path).to eq("/nyan_cat.png")
       end
     end
 
     describe "when given local windows path" do
       let(:uri) { URI.parse("file:///z:/windows/path/file.txt") }
       it "returns a valid windows local path" do
-        expect(fetcher.fix_windows_path(uri.path)).to eq("z:/windows/path/file.txt")
+        expect(fetcher.source_path).to eq("z:/windows/path/file.txt")
+      end
+    end
+
+    describe "when given local windows path with spaces" do
+      let(:uri) { URI.parse(URI.escape("file:///z:/windows/path/foo & bar.txt")) }
+      it "returns a valid windows local path" do
+        expect(fetcher.source_path).to eq("z:/windows/path/foo & bar.txt")
       end
     end
 
     describe "when given unc windows path" do
       let(:uri) { URI.parse("file:////server/share/windows/path/file.txt") }
       it "returns a valid windows unc path" do
-        expect(fetcher.fix_windows_path(uri.path)).to eq("//server/share/windows/path/file.txt")
+        expect(fetcher.source_path).to eq("//server/share/windows/path/file.txt")
+      end
+    end
+
+    describe "when given unc windows path with spaces" do
+      let(:uri) { URI.parse(URI.escape("file:////server/share/windows/path/foo & bar.txt")) }
+      it "returns a valid windows unc path" do
+        expect(fetcher.source_path).to eq("//server/share/windows/path/foo & bar.txt")
       end
     end
   end
@@ -73,7 +92,7 @@ describe Chef::Provider::RemoteFile::LocalFile do
     it "stages the local file to a temporary file" do
       expect(Chef::FileContentManagement::Tempfile).to receive(:new).with(new_resource).and_return(chef_tempfile)
       expect(::FileUtils).to receive(:cp).with(uri.path, tempfile.path)
-      expect(tempfile).to receive(:close)            
+      expect(tempfile).to receive(:close)
 
       result = fetcher.fetch
       expect(result).to eq(tempfile)
diff --git a/spec/unit/resource/remote_file_spec.rb b/spec/unit/resource/remote_file_spec.rb
index 0b3df4eb2c..0a379ff574 100644
--- a/spec/unit/resource/remote_file_spec.rb
+++ b/spec/unit/resource/remote_file_spec.rb
@@ -60,6 +60,11 @@ describe Chef::Resource::RemoteFile do
       expect(@resource.source).to eql([ "\\\\fakey\\fakerton\\fake.txt" ])
     end
 
+    it 'should accept file URIs with spaces' do
+      @resource.source("file:///C:/foo bar")
+      expect(@resource.source).to eql(["file:///C:/foo bar"])
+    end
+
     it "should accept a delayed evalutator (string) for the remote file source" do
       @resource.source Chef::DelayedEvaluator.new {"http://opscode.com/"}
       expect(@resource.source).to eql([ "http://opscode.com/" ])