Permalink
Browse files

Raise unless content type or name validation

It is now a requirement for attachments to do one of three things:
  1. Have a content_type validation (e.g. "image/*")
  2. Have a filename validation (e.g. *.png, *.gif)
  3. Explicitly *not* have one of those validations

The intent is to make the default more secure, and you have to
explicitly reject the security of a validation in order to not have one.
  • Loading branch information...
jyurek committed Jan 31, 2014
1 parent c9c5227 commit 38c51d6f30b9996e81f482c25fd315fd0213989c
@@ -11,8 +11,10 @@ def attachment_path(filename)
When /^I modify my attachment definition to:$/ do |definition|
content = in_current_dir { File.read("app/models/user.rb") }
+ name = content[/has_attached_file :\w+/][/:\w+/]
content.gsub!(/has_attached_file.+end/m, <<-FILE)
#{definition}
+ do_not_validate_attachment_file_type #{name}
end
FILE
@@ -69,6 +69,7 @@ def attach_attachment(name, definition = nil)
snippet += ", \n"
snippet += definition
end
+ snippet += "\ndo_not_validate_attachment_file_type :#{name}\n"
in_current_dir do
transform_file("app/models/user.rb") do |content|
content.sub(/end\Z/, "#{snippet}\nend")
@@ -392,11 +392,8 @@ def active_validator_classes
end
def ensure_required_validations!
- if ! active_validator_classes.include?(Paperclip::Validators::AttachmentContentTypeValidator)
- ActiveSupport::Deprecation.warn(
- "You must define a content_type validator to ensure you only accept files of the correct type. Failure to do so will raise an error in Paperclip versions >= 4.0"
- )
- # raise Paperclip::Errors::NoContentTypeValidator.new('you must define a content type validation')
+ if (active_validator_classes & Paperclip::REQUIRED_VALIDATORS).empty?
+ raise Paperclip::Errors::MissingRequiredValidatorError
end
end
View
@@ -13,9 +13,10 @@ class StorageMethodNotFound < Paperclip::Error
class CommandNotFoundError < Paperclip::Error
end
- # Thrown when no content_type validation exists
- # class NoContentTypeValidator < Paperclip::Error
- # end
+ # Attachments require a content_type or file_name validator,
+ # or to have explicitly opted out of them.
+ class MissingRequiredValidatorError < Paperclip::Error
+ end
# Will be thrown when ImageMagic cannot determine the uploaded file's
# metadata, usually this would mean the file is not an image.
@@ -1,9 +1,11 @@
require 'active_model'
require 'active_support/concern'
require 'paperclip/validators/attachment_content_type_validator'
+require 'paperclip/validators/attachment_file_name_validator'
require 'paperclip/validators/attachment_presence_validator'
require 'paperclip/validators/attachment_size_validator'
require 'paperclip/validators/media_type_spoof_detection_validator'
+require 'paperclip/validators/attachment_file_type_ignorance_validator'
module Paperclip
module Validators
@@ -14,6 +16,8 @@ module Validators
include HelperMethods
end
+ ::Paperclip::REQUIRED_VALIDATORS = [AttachmentFileNameValidator, AttachmentContentTypeValidator, AttachmentFileTypeIgnoranceValidator]
+
module ClassMethods
# This method is a shortcut to validator classes that is in
# "Attachment...Validator" format. It is almost the same thing as the
@@ -40,7 +44,7 @@ def validates_attachment(*attributes)
local_options = attributes + [validator_options]
conditional_options = options.slice(:if, :unless)
local_options.last.merge!(conditional_options)
- send(:"validates_attachment_#{validator_kind}", *local_options)
+ send(Paperclip::Validators.const_get(constant.to_s).helper_method_name, *local_options)
end
end
end
@@ -6,6 +6,10 @@ def initialize(options)
super
end
+ def self.helper_method_name
+ :validates_attachment_content_type
+ end
+
def validate_each(record, attribute, value)
base_attribute = attribute.to_sym
attribute = "#{attribute}_content_type".to_sym
@@ -0,0 +1,80 @@
+module Paperclip
+ module Validators
+ class AttachmentFileNameValidator < ActiveModel::EachValidator
+ def initialize(options)
+ options[:allow_nil] = true unless options.has_key?(:allow_nil)
+ super
+ end
+
+ def self.helper_method_name
+ :validates_attachment_file_name
+ end
+
+ def validate_each(record, attribute, value)
+ base_attribute = attribute.to_sym
+ attribute = "#{attribute}_file_name".to_sym
+ value = record.send :read_attribute_for_validation, attribute
+
+ return if (value.nil? && options[:allow_nil]) || (value.blank? && options[:allow_blank])
+
+ validate_whitelist(record, attribute, value)
+ validate_blacklist(record, attribute, value)
+
+ if record.errors.include? attribute
+ record.errors[attribute].each do |error|
+ record.errors.add base_attribute, error
+ end
+ end
+ end
+
+ def validate_whitelist(record, attribute, value)
+ if allowed.present? && allowed.none? { |type| type === value }
+ mark_invalid record, attribute, allowed
+ end
+ end
+
+ def validate_blacklist(record, attribute, value)
+ if forbidden.present? && forbidden.any? { |type| type === value }
+ mark_invalid record, attribute, forbidden
+ end
+ end
+
+ def mark_invalid(record, attribute, patterns)
+ record.errors.add attribute, :invalid, options.merge(:names => patterns.join(', '))
+ end
+
+ def allowed
+ [options[:matches]].flatten.compact
+ end
+
+ def forbidden
+ [options[:not]].flatten.compact
+ end
+
+ def check_validity!
+ unless options.has_key?(:matches) || options.has_key?(:not)
+ raise ArgumentError, "You must pass in either :matches or :not to the validator"
+ end
+ end
+ end
+
+ module HelperMethods
+ # Places ActiveModel validations on the name of the file
+ # assigned. The possible options are:
+ # * +matches+: Allowed filename patterns as Regexps. Can be a single one
+ # or an array.
+ # * +not+: Forbidden file name patterns, specified the same was as +matches+.
+ # * +message+: The message to display when the uploaded file has an invalid
+ # name.
+ # * +if+: A lambda or name of an instance method. Validation will only
+ # be run is this lambda or method returns true.
+ # * +unless+: Same as +if+ but validates if lambda or method returns false.
+ def validates_attachment_file_name(*attr_names)
+ options = _merge_attributes(attr_names)
+ validates_with AttachmentFileNameValidator, options.dup
+ validate_before_processing AttachmentFileNameValidator, options.dup
+ end
+ end
+ end
+end
+
@@ -0,0 +1,29 @@
+require 'active_model/validations/presence'
+
+module Paperclip
+ module Validators
+ class AttachmentFileTypeIgnoranceValidator < ActiveModel::EachValidator
+ def validate_each(record, attribute, value)
+ # This doesn't do anything. It's just to mark that you don't care about
+ # the file_names or content_types of your incoming attachments.
+ end
+
+ def self.helper_method_name
+ :do_not_validate_attachment_file_type
+ end
+ end
+
+ module HelperMethods
+ # Places ActiveModel validations on the presence of a file.
+ # Options:
+ # * +if+: A lambda or name of an instance method. Validation will only
+ # be run if this lambda or method returns true.
+ # * +unless+: Same as +if+ but validates if lambda or method returns false.
+ def do_not_validate_attachment_file_type(*attr_names)
+ options = _merge_attributes(attr_names)
+ validates_with AttachmentFileTypeIgnoranceValidator, options.dup
+ end
+ end
+ end
+end
+
@@ -8,6 +8,10 @@ def validate_each(record, attribute, value)
record.errors.add(attribute, :blank, options)
end
end
+
+ def self.helper_method_name
+ :validates_attachment_presence
+ end
end
module HelperMethods
@@ -10,6 +10,10 @@ def initialize(options)
super
end
+ def self.helper_method_name
+ :validates_attachment_size
+ end
+
def validate_each(record, attr_name, value)
base_attr_name = attr_name
attr_name = "#{attr_name}_file_size".to_sym
@@ -5,6 +5,7 @@ class AttachmentDefinitionsTest < Test::Unit::TestCase
reset_class "Dummy"
Dummy.has_attached_file :avatar, {:path => "abc"}
Dummy.has_attached_file :other_attachment, {:url => "123"}
+ Dummy.do_not_validate_attachment_file_type :avatar
expected = {:avatar => {:path => "abc"}, :other_attachment => {:url => "123"}}
assert_equal expected, Dummy.attachment_definitions
View
@@ -126,13 +126,15 @@ def rebuild_model options = {}
def rebuild_class options = {}
reset_class("Dummy").tap do |klass|
klass.has_attached_file :avatar, options
+ klass.do_not_validate_attachment_file_type :avatar
Paperclip.reset_duplicate_clash_check!
end
end
def rebuild_meta_class_of obj, options = {}
(class << obj; self; end).tap do |metaklass|
metaklass.has_attached_file :avatar, options
+ metaklass.do_not_validate_attachment_file_type :avatar
Paperclip.reset_duplicate_clash_check!
end
end
@@ -170,21 +172,21 @@ def silence_warnings
def should_accept_dummy_class
should "accept the class" do
- assert_accepts @matcher, @dummy_class
+ assert_accepts @matcher, Dummy
end
should "accept an instance of that class" do
- assert_accepts @matcher, @dummy_class.new
+ assert_accepts @matcher, Dummy.new
end
end
def should_reject_dummy_class
should "reject the class" do
- assert_rejects @matcher, @dummy_class
+ assert_rejects @matcher, Dummy
end
should "reject an instance of that class" do
- assert_rejects @matcher, @dummy_class.new
+ assert_rejects @matcher, Dummy.new
end
end
@@ -85,7 +85,6 @@ class HttpUrlProxyAdapterTest < Test::Unit::TestCase
begin
@subject.close
rescue Exception
- binding.pry
true
end
end
@@ -3,7 +3,7 @@
class HaveAttachedFileMatcherTest < Test::Unit::TestCase
context "have_attached_file" do
setup do
- @dummy_class = reset_class "Dummy"
+ reset_class "Dummy"
reset_table "dummies"
@matcher = self.class.have_attached_file(:avatar)
end
@@ -15,7 +15,8 @@ class HaveAttachedFileMatcherTest < Test::Unit::TestCase
context "given a class with an attachment" do
setup do
modify_table("dummies"){|d| d.string :avatar_file_name }
- @dummy_class.has_attached_file :avatar
+ Dummy.has_attached_file :avatar
+ Dummy.do_not_validate_attachment_file_type :avatar
end
should_accept_dummy_class
@@ -8,8 +8,9 @@ class ValidateAttachmentContentTypeMatcherTest < Test::Unit::TestCase
d.string :avatar_file_name
d.string :avatar_content_type
end
- @dummy_class = reset_class "Dummy"
- @dummy_class.has_attached_file :avatar
+ reset_class "Dummy"
+ Dummy.do_not_validate_attachment_file_type :avatar
+ Dummy.has_attached_file :avatar
@matcher = self.class.validate_attachment_content_type(:avatar).
allowing(%w(image/png image/jpeg)).
rejecting(%w(audio/mp3 application/octet-stream))
@@ -21,32 +22,32 @@ class ValidateAttachmentContentTypeMatcherTest < Test::Unit::TestCase
context "given a class with a validation that doesn't match" do
setup do
- @dummy_class.validates_attachment_content_type :avatar, :content_type => %r{audio/.*}
+ Dummy.validates_attachment_content_type :avatar, :content_type => %r{audio/.*}
end
should_reject_dummy_class
end
context "given a class with a matching validation" do
setup do
- @dummy_class.validates_attachment_content_type :avatar, :content_type => %r{image/.*}
+ Dummy.validates_attachment_content_type :avatar, :content_type => %r{image/.*}
end
should_accept_dummy_class
end
context "given a class with other validations but matching types" do
setup do
- @dummy_class.validates_presence_of :title
- @dummy_class.validates_attachment_content_type :avatar, :content_type => %r{image/.*}
+ Dummy.validates_presence_of :title
+ Dummy.validates_attachment_content_type :avatar, :content_type => %r{image/.*}
end
should_accept_dummy_class
end
context "given a class that matches and a matcher that only specifies 'allowing'" do
setup do
- @dummy_class.validates_attachment_content_type :avatar, :content_type => %r{image/.*}
+ Dummy.validates_attachment_content_type :avatar, :content_type => %r{image/.*}
@matcher = self.class.validate_attachment_content_type(:avatar).
allowing(%w(image/png image/jpeg))
end
@@ -56,7 +57,7 @@ class ValidateAttachmentContentTypeMatcherTest < Test::Unit::TestCase
context "given a class that does not match and a matcher that only specifies 'allowing'" do
setup do
- @dummy_class.validates_attachment_content_type :avatar, :content_type => %r{audio/.*}
+ Dummy.validates_attachment_content_type :avatar, :content_type => %r{audio/.*}
@matcher = self.class.validate_attachment_content_type(:avatar).
allowing(%w(image/png image/jpeg))
end
@@ -66,7 +67,7 @@ class ValidateAttachmentContentTypeMatcherTest < Test::Unit::TestCase
context "given a class that matches and a matcher that only specifies 'rejecting'" do
setup do
- @dummy_class.validates_attachment_content_type :avatar, :content_type => %r{image/.*}
+ Dummy.validates_attachment_content_type :avatar, :content_type => %r{image/.*}
@matcher = self.class.validate_attachment_content_type(:avatar).
rejecting(%w(audio/mp3 application/octet-stream))
end
@@ -76,7 +77,7 @@ class ValidateAttachmentContentTypeMatcherTest < Test::Unit::TestCase
context "given a class that does not match and a matcher that only specifies 'rejecting'" do
setup do
- @dummy_class.validates_attachment_content_type :avatar, :content_type => %r{audio/.*}
+ Dummy.validates_attachment_content_type :avatar, :content_type => %r{audio/.*}
@matcher = self.class.validate_attachment_content_type(:avatar).
rejecting(%w(audio/mp3 application/octet-stream))
end
@@ -86,14 +87,14 @@ class ValidateAttachmentContentTypeMatcherTest < Test::Unit::TestCase
context "using an :if to control the validation" do
setup do
- @dummy_class.class_eval do
+ Dummy.class_eval do
validates_attachment_content_type :avatar, :content_type => %r{image/*} , :if => :go
attr_accessor :go
end
@matcher = self.class.validate_attachment_content_type(:avatar).
allowing(%w(image/png image/jpeg)).
rejecting(%w(audio/mp3 application/octet-stream))
- @dummy = @dummy_class.new
+ @dummy = Dummy.new
end
should "run the validation if the control is true" do
Oops, something went wrong.

1 comment on commit 38c51d6

@jyurek I know this fixes http://homakov.blogspot.com/2014/02/paperclip-vulnerability-leading-to-xss.html I'm wondering about v2.8.0 though. Many of these files aren't even in that version, but do you know if that version still has the problem that this fixes?

Please sign in to comment.