Changed Paperclip.run to take an array. Quotes this array for shell safety.

lib/paperclip.rb

@@ -65,7 +65,7 @@ module Paperclip
         :image_magick_path => nil,
         :command_path      => nil,
         :log               => true,
-        :log_command       => false,
+        :log_command       => true,
         :swallow_stderr    => true
       }
     end
@@ -82,7 +82,7 @@ module Paperclip
       Paperclip::Interpolations[key] = block
     end
 
-    # The run method takes a command to execute and a string of parameters
+    # The run method takes a command to execute and an array of parameters
     # that get passed to it. The command is prefixed with the :command_path
     # option from Paperclip.options. If you have many commands to run and
     # they are in different paths, the suggested course of action is to
@@ -91,21 +91,37 @@ module Paperclip
     # If the command returns with a result code that is not one of the
     # expected_outcodes, a PaperclipCommandLineError will be raised. Generally
     # a code of 0 is expected, but a list of codes may be passed if necessary.
+    # These codes should be passed as a hash as the last argument, like so:
+    #
+    #   Paperclip.run("echo", "something", :expected_outcodes => [0,1,2,3])
     #
     # This method can log the command being run when 
     # Paperclip.options[:log_command] is set to true (defaults to false). This
     # will only log if logging in general is set to true as well.
-    def run cmd, params = "", expected_outcodes = 0
-      command = %Q[#{path_for_command(cmd)} #{params}].gsub(/\s+/, " ")
+    def run cmd, *params
+      options           = params.last.is_a?(Hash) ? params.pop : {}
+      expected_outcodes = options[:expected_outcodes] || [0]
+      params            = quote_command_options(*params).join(" ")
+
+      command = %Q[#{path_for_command(cmd)} #{params}]
       command = "#{command} 2>#{bit_bucket}" if Paperclip.options[:swallow_stderr]
       Paperclip.log(command) if Paperclip.options[:log_command]
+
       output = `#{command}`
-      unless [expected_outcodes].flatten.include?($?.exitstatus)
-        raise PaperclipCommandLineError, "Error while running #{cmd}"
+      unless expected_outcodes.include?($?.exitstatus)
+        raise PaperclipCommandLineError,
+          "Error while running #{cmd}. Expected return code to be #{expected_outcodes.join(", ")} but was #{$?.exitstatus}",
+          output
       end
       output
     end
 
+    def quote_command_options(*options)
+      options.map do |option|
+        option.split("'").map{|m| "'#{m}'" }.join("\\'")
+      end
+    end
+
     def bit_bucket #:nodoc:
       File.exists?("/dev/null") ? "/dev/null" : "NUL"
     end
@@ -149,6 +165,11 @@ module Paperclip
   end
 
   class PaperclipCommandLineError < StandardError #:nodoc:
+    attr_accessor :output
+    def initialize(msg = nil, output = nil)
+      super(msg)
+      @output = output
+    end
   end
 
   class NotIdentifiedByImageMagickError < PaperclipError #:nodoc:
@@ -197,7 +218,7 @@ module Paperclip
     #   Defaults to true. This option used to be called :whiny_thumbanils, but this is
     #   deprecated.
     # * +convert_options+: When creating thumbnails, use this free-form options
-    #   field to pass in various convert command options.  Typical options are "-strip" to
+    #   array to pass in various convert command options.  Typical options are "-strip" to
     #   remove all Exif data from the image (save space for thumbnails and avatars) or
     #   "-depth 8" to specify the bit depth of the resulting conversion.  See ImageMagick
     #   convert documentation for more options: (http://www.imagemagick.org/script/convert.php)
@@ -214,6 +235,9 @@ module Paperclip
     #   NOTE: While not deprecated yet, it is not recommended to specify options this way.
     #   It is recommended that :convert_options option be included in the hash passed to each
     #   :styles for compatability with future versions.
+    #   NOTE: Strings supplied to :convert_options are split on space in order to undergo
+    #   shell quoting for safety. If your options require a space, please pre-split them
+    #   and pass an array to :convert_options instead.
     # * +storage+: Chooses the storage backend where the files will be stored. The current
     #   choices are :filesystem and :s3. The default is :filesystem. Make sure you read the
     #   documentation for Paperclip::Storage::Filesystem and Paperclip::Storage::S3

lib/paperclip/attachment.rb

@@ -87,7 +87,7 @@ module Paperclip
       return nil if uploaded_file.nil?
 
       @queued_for_write[:original]   = uploaded_file.to_tempfile
-      instance_write(:file_name,       uploaded_file.original_filename.strip.gsub(/[^A-Za-z\d\.\-_]+/, '_'))
+      instance_write(:file_name,       uploaded_file.original_filename.strip)
       instance_write(:content_type,    uploaded_file.content_type.to_s.strip)
       instance_write(:file_size,       uploaded_file.size.to_i)
       instance_write(:updated_at,      Time.now)

lib/paperclip/geometry.rb

@@ -16,7 +16,7 @@ module Paperclip
     def self.from_file file
       file = file.path if file.respond_to? "path"
       geometry = begin
-                   Paperclip.run("identify", %Q[-format "%wx%h" "#{file}"[0]])
+                   Paperclip.run("identify", "-format", "%wx%h", "#{file}[0]")
                  rescue PaperclipCommandLineError
                    ""
                  end

lib/paperclip/thumbnail.rb

@@ -23,6 +23,9 @@ module Paperclip
       @whiny               = options[:whiny].nil? ? true : options[:whiny]
       @format              = options[:format]
 
+      @source_file_options = @source_file_options.split(/\s+/) if @source_file_options.respond_to?(:split)
+      @convert_options     = @convert_options.split(/\s+/)     if @convert_options.respond_to?(:split)
+
       @current_format      = File.extname(@file.path)
       @basename            = File.basename(@file.path, @current_format)
       
@@ -45,16 +48,17 @@ module Paperclip
       dst = Tempfile.new([@basename, @format].compact.join("."))
       dst.binmode
 
-      command = <<-end_command
-        #{ source_file_options }
-        "#{ File.expand_path(src.path) }[0]"
-        #{ transformation_command }
-        "#{ File.expand_path(dst.path) }"
-      end_command
-
       begin
-        success = Paperclip.run("convert", command.gsub(/\s+/, " "))
-      rescue PaperclipCommandLineError
+        options = [
+          source_file_options,
+          "#{ File.expand_path(src.path) }[0]",
+          transformation_command,
+          convert_options,
+          "#{ File.expand_path(dst.path) }"
+        ].flatten.compact
+
+        success = Paperclip.run("convert", *options)
+      rescue PaperclipCommandLineError => e
         raise PaperclipError, "There was an error processing the thumbnail for #{@basename}" if @whiny
       end
 
@@ -65,10 +69,9 @@ module Paperclip
     # into the thumbnail.
     def transformation_command
       scale, crop = @current_geometry.transformation_to(@target_geometry, crop?)
-      trans = ""
-      trans << " -resize \"#{scale}\"" unless scale.nil? || scale.empty?
-      trans << " -crop \"#{crop}\" +repage" if crop
-      trans << " #{convert_options}" if convert_options?
+      trans = []
+      trans << "-resize" << scale unless scale.nil? || scale.empty?
+      trans << "-crop" << crop << "+repage" if crop
       trans
     end
   end

lib/paperclip/upfile.rb

@@ -17,7 +17,7 @@ module Paperclip
       when "csv", "xml", "css"       then "text/#{type}"
       else
         # On BSDs, `file` doesn't give a result code of 1 if the file doesn't exist.
-        content_type = (Paperclip.run("file", "--mime-type #{self.path}").split(':').last.strip rescue "application/x-#{type}")
+        content_type = (Paperclip.run("file", "--mime-type", self.path).split(':').last.strip rescue "application/x-#{type}")
         content_type = "application/x-#{type}" if content_type.match(/\(.*?\)/)
         content_type
       end

test/attachment_test.rb

@@ -460,10 +460,9 @@ class AttachmentTest < Test::Unit::TestCase
       @dummy.avatar = @not_file
     end
     
-    should "remove strange letters and replace with underscore (_)" do
-      assert_equal "sheep_say_b_.png", @dummy.avatar.original_filename
+    should "not remove strange letters" do
+      assert_equal "sheep_say_bæ.png", @dummy.avatar.original_filename
     end
-    
   end
 
   context "An attachment" do

test/paperclip_test.rb

@@ -16,8 +16,8 @@ class PaperclipTest < Test::Unit::TestCase
       should "execute the right command" do
         Paperclip.expects(:path_for_command).with("convert").returns("/usr/bin/convert")
         Paperclip.expects(:bit_bucket).returns("/dev/null")
-        Paperclip.expects(:"`").with("/usr/bin/convert one.jpg two.jpg 2>/dev/null")
-        Paperclip.run("convert", "one.jpg two.jpg")
+        Paperclip.expects(:"`").with("/usr/bin/convert 'one.jpg' 'two.jpg' 2>/dev/null")
+        Paperclip.run("convert", "one.jpg", "two.jpg")
       end
     end
   end
@@ -35,8 +35,8 @@ class PaperclipTest < Test::Unit::TestCase
     should "execute the right command" do
       Paperclip.expects(:path_for_command).with("convert").returns("convert")
       Paperclip.expects(:bit_bucket).returns("/dev/null")
-      Paperclip.expects(:"`").with("convert one.jpg two.jpg 2>/dev/null")
-      Paperclip.run("convert", "one.jpg two.jpg")
+      Paperclip.expects(:"`").with("convert 'one.jpg' 'two.jpg' 2>/dev/null")
+      Paperclip.run("convert", "one.jpg", "two.jpg")
     end
   end
 
@@ -45,8 +45,8 @@ class PaperclipTest < Test::Unit::TestCase
       Paperclip.options[:image_magick_path] = nil
       Paperclip.options[:command_path]      = nil
       Paperclip.stubs(:bit_bucket).returns("/dev/null")
-      Paperclip.expects(:log).with("this is the command 2>/dev/null")
-      Paperclip.expects(:"`").with("this is the command 2>/dev/null")
+      Paperclip.expects(:log).with("this 'is the command' 2>/dev/null")
+      Paperclip.expects(:"`").with("this 'is the command' 2>/dev/null")
       Paperclip.options[:log_command] = true
       Paperclip.run("this","is the command")
     end
@@ -55,13 +55,22 @@ class PaperclipTest < Test::Unit::TestCase
       Paperclip.options[:image_magick_path] = nil
       Paperclip.options[:command_path]      = nil
       Paperclip.stubs(:bit_bucket).returns("/dev/null")
-      Paperclip.expects(:log).with("this is the command 2>/dev/null").never
-      Paperclip.expects(:"`").with("this is the command 2>/dev/null")
+      Paperclip.expects(:log).with("this 'is the command' 2>/dev/null").never
+      Paperclip.expects(:"`").with("this 'is the command' 2>/dev/null")
       Paperclip.options[:log_command] = false
       Paperclip.run("this","is the command")
     end
   end
 
+  should "prevent dangerous characters in the command via quoting" do
+    Paperclip.options[:image_magick_path] = nil
+    Paperclip.options[:command_path]      = nil
+    Paperclip.options[:log_command]       = false
+    Paperclip.options[:swallow_stderr]    = false
+    Paperclip.expects(:"`").with(%q[this 'is' 'jack'\''s' '`command`' 'line!'])
+    Paperclip.run("this", "is", "jack's", "`command`", "line!")
+  end
+
   context "Paperclip.bit_bucket" do
     context "on systems without /dev/null" do
       setup do

test/thumbnail_test.rb

@@ -92,7 +92,7 @@ class ThumbnailTest < Test::Unit::TestCase
 
       should "send the right command to convert when sent #make" do
         Paperclip.expects(:"`").with do |arg|
-          arg.match %r{convert\s+"#{File.expand_path(@thumb.file.path)}\[0\]"\s+-resize\s+\"x50\"\s+-crop\s+\"100x50\+114\+0\"\s+\+repage\s+".*?"}
+          arg.match %r{convert '#{File.expand_path(@thumb.file.path)}\[0\]' '-resize' 'x50' '-crop' '100x50\+114\+0' '\+repage' '.*?'}
         end
         @thumb.make
       end
@@ -111,12 +111,12 @@ class ThumbnailTest < Test::Unit::TestCase
       end
 
       should "have source_file_options value set" do
-        assert_equal "-strip", @thumb.source_file_options
+        assert_equal ["-strip"], @thumb.source_file_options
       end
 
       should "send the right command to convert when sent #make" do
         Paperclip.expects(:"`").with do |arg|
-          arg.match %r{convert\s+-strip\s+"#{File.expand_path(@thumb.file.path)}\[0\]"\s+-resize\s+"x50"\s+-crop\s+"100x50\+114\+0"\s+\+repage\s+".*?"}
+          arg.match %r{convert '-strip' '#{File.expand_path(@thumb.file.path)}\[0\]' '-resize' 'x50' '-crop' '100x50\+114\+0' '\+repage' '.*?'}
         end
         @thumb.make
       end
@@ -149,12 +149,12 @@ class ThumbnailTest < Test::Unit::TestCase
       end
 
       should "have convert_options value set" do
-        assert_equal "-strip -depth 8", @thumb.convert_options
+        assert_equal %w"-strip -depth 8", @thumb.convert_options
       end
 
       should "send the right command to convert when sent #make" do
         Paperclip.expects(:"`").with do |arg|
-          arg.match %r{convert\s+"#{File.expand_path(@thumb.file.path)}\[0\]"\s+-resize\s+"x50"\s+-crop\s+"100x50\+114\+0"\s+\+repage\s+-strip\s+-depth\s+8\s+".*?"}
+          arg.match %r{convert '#{File.expand_path(@thumb.file.path)}\[0\]' '-resize' 'x50' '-crop' '100x50\+114\+0' '\+repage' '-strip' '-depth' '8' '.*?'}
         end
         @thumb.make
       end
@@ -187,7 +187,7 @@ class ThumbnailTest < Test::Unit::TestCase
       end
       
       should "not get resized by default" do
-        assert_no_match(/-resize/, @thumb.transformation_command)
+        assert !@thumb.transformation_command.include?("-resize")
       end
     end
   end