Implement quoting on the exec commands and repair specs

2 files changed, 124 insertions, 91 deletions

diff --git a/lib/puppet/type/exec.rb b/lib/puppet/type/exec.rb
index 47d85dad4..41150ba94 100755
--- a/lib/puppet/type/exec.rb
+++ b/lib/puppet/type/exec.rb
@@ -163,7 +163,13 @@ module Puppet
             # Support both arrays and colon-separated fields.
             def value=(*values)
                 @value = values.flatten.collect { |val|
-                    val.split(":")
+                    if val =~ /;/ # recognize semi-colon separated paths
+                        val.split(";")
+                    elsif val =~ /^\w:[^:]*$/ # heuristic to avoid splitting a driveletter away
+                        val
+                    else
+                        val.split(":")
+                    end
                 }.flatten
             end
         end
@@ -444,6 +450,10 @@ module Puppet
                 reqs << str
             }
 
+            self[:command].scan(/^"([^"]+)"/) { |str|
+                reqs << str
+            }
+
             [:onlyif, :unless].each { |param|
                 next unless tmp = self[param]
 
@@ -498,29 +508,40 @@ module Puppet
 
         # Verify that we have the executable
         def checkexe(cmd)
-            if cmd =~ /^\//
-                exe = cmd.split(/ /)[0]
-                unless FileTest.exists?(exe)
-                    raise ArgumentError, "Could not find executable %s" % exe
-                end
-                unless FileTest.executable?(exe)
-                    raise ArgumentError,
-                        "%s is not executable" % exe
-                end
-            elsif path = self[:path]
-                exe = cmd.split(/ /)[0]
-                withenv :PATH => self[:path].join(":") do
-                    path = %{which #{exe}}.chomp
-                    if path == ""
-                        raise ArgumentError,
-                            "Could not find command '%s'" % exe
+            exe = extractexe(cmd)
+
+            if self[:path]
+                if Puppet.features.posix? and !File.exists?(exe)
+                    withenv :PATH => self[:path].join(File::PATH_SEPARATOR) do
+                        path = %{which #{exe}}.chomp
+                        if path == ""
+                            raise ArgumentError,
+                                "Could not find command '%s'" % exe
+                        else
+                            exe = File.join(path, exe)
+                        end
+                    end
+                elsif Puppet.features.win32? and !File.exists?(exe)
+                    self[:path].each do |path|
+                        [".exe", ".ps1", ".bat", ".com", ""].each do |extension|
+                            file = File.join(path, exe+extension)
+                            return if File.exists?(file)
+                        end
                     end
                 end
             else
                 raise ArgumentError,
-                    "%s is somehow not qualified with no search path" %
+                    "'%s' is somehow not qualified with no search path" %
                         self[:command]
             end
+            
+            unless FileTest.exists?(exe)
+                raise ArgumentError, "Could not find executable '%s'" % exe
+            end
+            unless FileTest.executable?(exe)
+                raise ArgumentError,
+                    "'%s' is not executable" % exe
+            end
         end
 
         def output
@@ -616,11 +637,19 @@ module Puppet
         end
 
         def validatecmd(cmd)
+            exe = extractexe(cmd)
             # if we're not fully qualified, require a path
-            if cmd !~ /^\//
-                if self[:path].nil?
-                    self.fail "'%s' is both unqualifed and specified no search path" % cmd
-                end
+            if File.expand_path(exe) != exe and self[:path].nil?
+                self.fail "'%s' is both unqualifed and specified no search path" % cmd
+            end
+        end
+        
+        def extractexe(cmd)
+            # easy case: command was quoted
+            if cmd =~ /^"([^"]+)"/
+                $1
+            else
+                cmd.split(/ /)[0]
             end
         end
     end
diff --git a/spec/unit/type/exec.rb b/spec/unit/type/exec.rb
index 813bdaea4..777fa0127 100755
--- a/spec/unit/type/exec.rb
+++ b/spec/unit/type/exec.rb
@@ -2,12 +2,13 @@
 
 require File.dirname(__FILE__) + '/../../spec_helper'
 
-module ExecModuleTesting
+describe Puppet::Type.type(:exec) do
+
     def create_resource(command, output, exitstatus, returns = [0])
         @user_name = 'some_user_name'
         @group_name = 'some_group_name'
         Puppet.features.stubs(:root?).returns(true)
-        @execer = Puppet::Type.type(:exec).new(:name => command, :path => %w{/usr/bin /bin}, :user => @user_name, :group => @group_name, :returns => returns)
+        @execer = Puppet::Type.type(:exec).new(:name => command, :path => @example_path, :user => @user_name, :group => @group_name, :returns => returns)
 
         status = stub "process"
         status.stubs(:exitstatus).returns(exitstatus)
@@ -26,94 +27,97 @@ module ExecModuleTesting
             @execer.property(:returns).expects(loglevel).with(line)
         end
     end
-end
 
-describe Puppet::Type.type(:exec) do
+    before do
+        @executable = Puppet.features.posix? ? '/bin/true' : 'C:/Program Files/something.exe'
+        @command = Puppet.features.posix? ? '/bin/true whatever' : '"C:/Program Files/something.exe" whatever'
+        File.stubs(:exists?).returns false
+        File.stubs(:exists?).with(@executable).returns true
+        @example_path = Puppet.features.posix? ? %w{/usr/bin /bin} : [ "C:/Program Files/something/bin", "C:/Ruby/bin" ]
+        File.stubs(:exists?).with(File.join(@example_path[0],"true")).returns true
+        File.stubs(:exists?).with(File.join(@example_path[0],"false")).returns true
+    end
+
     it "should return :executed_command as its event" do
-        resource = Puppet::Type.type(:exec).new :command => "/bin/true"
+        resource = Puppet::Type.type(:exec).new :command => @command
         resource.parameter(:returns).event.name.should == :executed_command
     end
-end
 
-describe Puppet::Type.type(:exec), " when execing" do
-    include ExecModuleTesting
+    describe "when execing" do
 
-    it "should use the 'run_and_capture' method to exec" do
-        command = "true"
-        create_resource(command, "", 0)
+        it "should use the 'run_and_capture' method to exec" do
+            command = "true"
+            create_resource(command, "", 0)
 
-        @execer.refresh.should == :executed_command
-    end
+            @execer.refresh.should == :executed_command
+        end
 
-    it "should report a failure" do
-        command = "false"
-        create_resource(command, "", 1)
+        it "should report a failure" do
+            command = "false"
+            create_resource(command, "", 1)
 
-        proc { @execer.refresh }.should raise_error(Puppet::Error)
-    end
-    
-    it "should not report a failure if the exit status is specified in a returns array" do
-        command = "false"
-        create_resource(command, "", 1, [0,1])
-        proc { @execer.refresh }.should_not raise_error(Puppet::Error)
-    end
-    
-    it "should report a failure if the exit status is not specified in a returns array" do
-        command = "false"
-        create_resource(command, "", 1, [0,100])
-        proc { @execer.refresh }.should raise_error(Puppet::Error)
-    end
-
-    it "should log the output on success" do
-        #Puppet::Util::Log.newdestination :console
-        command = "false"
-        output = "output1\noutput2\n"
-        create_logging_resource(command, output, 0, true, :err)
-        expect_output(output, :err)
-        @execer.refresh
-    end
+            proc { @execer.refresh }.should raise_error(Puppet::Error)
+        end
+        
+        it "should not report a failure if the exit status is specified in a returns array" do
+            command = "false"
+            create_resource(command, "", 1, [0,1])
+            proc { @execer.refresh }.should_not raise_error(Puppet::Error)
+        end
+        
+        it "should report a failure if the exit status is not specified in a returns array" do
+            command = "false"
+            create_resource(command, "", 1, [0,100])
+            proc { @execer.refresh }.should raise_error(Puppet::Error)
+        end
 
-    it "should log the output on failure" do
-        #Puppet::Util::Log.newdestination :console
-        command = "false"
-        output = "output1\noutput2\n"
-        create_logging_resource(command, output, 1, true, :err)
-        expect_output(output, :err)
+        it "should log the output on success" do
+            #Puppet::Util::Log.newdestination :console
+            command = "false"
+            output = "output1\noutput2\n"
+            create_logging_resource(command, output, 0, true, :err)
+            expect_output(output, :err)
+            @execer.refresh
+        end
 
-        proc { @execer.refresh }.should raise_error(Puppet::Error)
-    end
+        it "should log the output on failure" do
+            #Puppet::Util::Log.newdestination :console
+            command = "false"
+            output = "output1\noutput2\n"
+            create_logging_resource(command, output, 1, true, :err)
+            expect_output(output, :err)
 
-end
+            proc { @execer.refresh }.should raise_error(Puppet::Error)
+        end
 
+    end
 
-describe Puppet::Type.type(:exec), " when logoutput=>on_failure is set," do
-    include ExecModuleTesting
+    describe "when logoutput=>on_failure is set" do
 
-    it "should log the output on failure" do
-        #Puppet::Util::Log.newdestination :console
-        command = "false"
-        output = "output1\noutput2\n"
-        create_logging_resource(command, output, 1, :on_failure, :err)
-        expect_output(output, :err)
+        it "should log the output on failure" do
+            #Puppet::Util::Log.newdestination :console
+            command = "false"
+            output = "output1\noutput2\n"
+            create_logging_resource(command, output, 1, :on_failure, :err)
+            expect_output(output, :err)
 
-        proc { @execer.refresh }.should raise_error(Puppet::Error)
-    end
+            proc { @execer.refresh }.should raise_error(Puppet::Error)
+        end
 
-    it "shouldn't log the output on success" do
-        #Puppet::Util::Log.newdestination :console
-        command = "true"
-        output = "output1\noutput2\n"
-        create_logging_resource(command, output, 0, :on_failure, :err)
-        @execer.property(:returns).expects(:err).never
-        @execer.refresh
+        it "shouldn't log the output on success" do
+            #Puppet::Util::Log.newdestination :console
+            command = "true"
+            output = "output1\noutput2\n"
+            create_logging_resource(command, output, 0, :on_failure, :err)
+            @execer.property(:returns).expects(:err).never
+            @execer.refresh
+        end
     end
-end
 
-describe Puppet::Type.type(:exec) do
     it "should be able to autorequire files mentioned in the command" do
         catalog = Puppet::Resource::Catalog.new
-        catalog.add_resource Puppet::Type.type(:file).new(:name => "/bin/true")
-        @execer = Puppet::Type.type(:exec).new(:name => "/bin/true")
+        catalog.add_resource Puppet::Type.type(:file).new(:name => @executable)
+        @execer = Puppet::Type.type(:exec).new(:name => @command)
         catalog.add_resource @execer
 
         rels = @execer.autorequire