Permalink
Browse files

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

…afety.
  • Loading branch information...
1 parent b65ea24 commit 724cc7e1a2b035b2f9c35c8a17adfc5383746deb @jyurek jyurek committed May 1, 2010
View
@@ -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
@@ -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)
@@ -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
View
@@ -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
View
@@ -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
View
@@ -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
View
@@ -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
View
@@ -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

0 comments on commit 724cc7e

Please sign in to comment.