From 724cc7e1a2b035b2f9c35c8a17adfc5383746deb Mon Sep 17 00:00:00 2001 From: Jon Yurek Date: Fri, 30 Apr 2010 20:11:14 -0400 Subject: [PATCH] Changed Paperclip.run to take an array. Quotes this array for shell safety. --- lib/paperclip.rb | 38 ++++++++++++++++++++++++++++++------- lib/paperclip/attachment.rb | 2 +- lib/paperclip/geometry.rb | 2 +- lib/paperclip/thumbnail.rb | 29 +++++++++++++++------------- lib/paperclip/upfile.rb | 2 +- test/attachment_test.rb | 5 ++--- test/paperclip_test.rb | 25 ++++++++++++++++-------- test/thumbnail_test.rb | 12 ++++++------ 8 files changed, 75 insertions(+), 40 deletions(-) diff --git a/lib/paperclip.rb b/lib/paperclip.rb index ed86f281a..f7599a6b1 100644 --- a/lib/paperclip.rb +++ b/lib/paperclip.rb @@ -65,7 +65,7 @@ def options :image_magick_path => nil, :command_path => nil, :log => true, - :log_command => false, + :log_command => true, :swallow_stderr => true } end @@ -82,7 +82,7 @@ def interpolates key, &block 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 @@ def interpolates key, &block # 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 @@ class PaperclipError < StandardError #:nodoc: 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 ClassMethods # 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 ClassMethods # 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 diff --git a/lib/paperclip/attachment.rb b/lib/paperclip/attachment.rb index 26226d0d7..09ecf7b63 100644 --- a/lib/paperclip/attachment.rb +++ b/lib/paperclip/attachment.rb @@ -87,7 +87,7 @@ def assign uploaded_file 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) diff --git a/lib/paperclip/geometry.rb b/lib/paperclip/geometry.rb index 5554bd509..d6f7ab126 100644 --- a/lib/paperclip/geometry.rb +++ b/lib/paperclip/geometry.rb @@ -16,7 +16,7 @@ def initialize width = nil, height = nil, modifier = nil 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 diff --git a/lib/paperclip/thumbnail.rb b/lib/paperclip/thumbnail.rb index 20686208c..323e58d9f 100644 --- a/lib/paperclip/thumbnail.rb +++ b/lib/paperclip/thumbnail.rb @@ -23,6 +23,9 @@ def initialize file, options = {}, attachment = nil @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 @@ def make 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 @@ def make # 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 diff --git a/lib/paperclip/upfile.rb b/lib/paperclip/upfile.rb index 1d41ce14c..927e7f9cf 100644 --- a/lib/paperclip/upfile.rb +++ b/lib/paperclip/upfile.rb @@ -17,7 +17,7 @@ def content_type 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 diff --git a/test/attachment_test.rb b/test/attachment_test.rb index aefe018f5..ebcf33a6b 100644 --- a/test/attachment_test.rb +++ b/test/attachment_test.rb @@ -460,10 +460,9 @@ def do_after_all; end @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 diff --git a/test/paperclip_test.rb b/test/paperclip_test.rb index a10e3252d..ee7e5055c 100644 --- a/test/paperclip_test.rb +++ b/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 diff --git a/test/thumbnail_test.rb b/test/thumbnail_test.rb index 840d3299d..57f9811a1 100644 --- a/test/thumbnail_test.rb +++ b/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