Commit 38c51d6f by Jon Yurek

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.
parent c9c5227c
......@@ -11,8 +11,10 @@ World(AttachmentHelpers)
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 @@ module Paperclip
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
......
......@@ -13,9 +13,10 @@ module Paperclip
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.
......
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 Paperclip
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 @@ module Paperclip
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 @@ module Paperclip
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
......
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
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 @@ module Paperclip
record.errors.add(attribute, :blank, options)
end
end
def self.helper_method_name
:validates_attachment_presence
end
end
module HelperMethods
......
......@@ -10,6 +10,10 @@ module Paperclip
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
......
......@@ -126,6 +126,7 @@ end
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
......@@ -133,6 +134,7 @@ 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 @@ end
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 @@ require './test/helper'
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,7 +22,7 @@ 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
......@@ -29,7 +30,7 @@ class ValidateAttachmentContentTypeMatcherTest < Test::Unit::TestCase
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
......@@ -37,8 +38,8 @@ class ValidateAttachmentContentTypeMatcherTest < Test::Unit::TestCase
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
......@@ -46,7 +47,7 @@ class ValidateAttachmentContentTypeMatcherTest < Test::Unit::TestCase
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
......
......@@ -6,8 +6,9 @@ class ValidateAttachmentPresenceMatcherTest < Test::Unit::TestCase
reset_table("dummies") do |d|
d.string :avatar_file_name
end
@dummy_class = reset_class "Dummy"
@dummy_class.has_attached_file :avatar
reset_class "Dummy"
Dummy.has_attached_file :avatar
Dummy.do_not_validate_attachment_file_type :avatar
@matcher = self.class.validate_attachment_presence(:avatar)
end
......@@ -17,7 +18,7 @@ class ValidateAttachmentPresenceMatcherTest < Test::Unit::TestCase
context "given a class with a matching validation" do
setup do
@dummy_class.validates_attachment_presence :avatar
Dummy.validates_attachment_presence :avatar
end
should_accept_dummy_class
......@@ -30,12 +31,12 @@ class ValidateAttachmentPresenceMatcherTest < Test::Unit::TestCase
d.string :avatar_content_type
end
@dummy_class.class_eval do
Dummy.class_eval do
validates_attachment_presence :avatar
validates_attachment_content_type :avatar, :content_type => 'image/gif'
end
@dummy = @dummy_class.new
@dummy = Dummy.new
@matcher = self.class.validate_attachment_presence(:avatar)
end
......@@ -47,11 +48,11 @@ class ValidateAttachmentPresenceMatcherTest < 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_presence :avatar, :if => :go
attr_accessor :go
end
@dummy = @dummy_class.new
@dummy = Dummy.new
@dummy.avatar = nil
end
......
......@@ -7,8 +7,9 @@ class ValidateAttachmentSizeMatcherTest < Test::Unit::TestCase
d.string :avatar_file_name
d.integer :avatar_file_size
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
end
context "of limited size" do
......@@ -19,17 +20,17 @@ class ValidateAttachmentSizeMatcherTest < Test::Unit::TestCase
end
context "given a class with a validation that's too high" do
setup { @dummy_class.validates_attachment_size :avatar, :in => 256..2048 }
setup { Dummy.validates_attachment_size :avatar, :in => 256..2048 }
should_reject_dummy_class
end
context "given a class with a validation that's too low" do
setup { @dummy_class.validates_attachment_size :avatar, :in => 0..1024 }
setup { Dummy.validates_attachment_size :avatar, :in => 0..1024 }
should_reject_dummy_class
end
context "given a class with a validation that matches" do
setup { @dummy_class.validates_attachment_size :avatar, :in => 256..1024 }
setup { Dummy.validates_attachment_size :avatar, :in => 256..1024 }
should_accept_dummy_class
end
end
......@@ -38,23 +39,23 @@ class ValidateAttachmentSizeMatcherTest < Test::Unit::TestCase
setup{ @matcher = self.class.validate_attachment_size(:avatar) }
context "given a class with an upper limit" do
setup { @dummy_class.validates_attachment_size :avatar, :less_than => 1 }
setup { Dummy.validates_attachment_size :avatar, :less_than => 1 }
should_accept_dummy_class
end
context "given a class with a lower limit" do
setup { @dummy_class.validates_attachment_size :avatar, :greater_than => 1 }
setup { Dummy.validates_attachment_size :avatar, :greater_than => 1 }
should_accept_dummy_class
end
end
context "using an :if to control the validation" do
setup do
@dummy_class.class_eval do
Dummy.class_eval do
validates_attachment_size :avatar, :greater_than => 1024, :if => :go
attr_accessor :go
end
@dummy = @dummy_class.new
@dummy = Dummy.new
@matcher = self.class.validate_attachment_size(:avatar).greater_than(1024)
end
......@@ -71,9 +72,9 @@ class ValidateAttachmentSizeMatcherTest < Test::Unit::TestCase
context "post processing" do
setup do
@dummy_class.validates_attachment_size :avatar, :greater_than => 1024
Dummy.validates_attachment_size :avatar, :greater_than => 1024
@dummy = @dummy_class.new
@dummy = Dummy.new
@matcher = self.class.validate_attachment_size(:avatar).greater_than(1024)
end
......
require './test/helper'
class AttachmentFileNameValidatorTest < Test::Unit::TestCase
def setup
rebuild_model
@dummy = Dummy.new
super
end
def build_validator(options)
@validator = Paperclip::Validators::AttachmentFileNameValidator.new(options.merge(
:attributes => :avatar
))
end
context "with a failing validation" do
setup do
build_validator :matches => /.*\.png$/, :allow_nil => false
@dummy.stubs(:avatar_file_name => "data.txt")
@validator.validate(@dummy)
end
should "add error to the base object" do
assert @dummy.errors[:avatar].present?,
"Error not added to base attribute"
end
should "add error to base object as a string" do
assert_kind_of String, @dummy.errors[:avatar].first,
"Error added to base attribute as something other than a String"
end
end
should "not add error to the base object with a successful validation" do
build_validator :matches => /.*\.png$/, :allow_nil => false
@dummy.stubs(:avatar_file_name => "image.png")
@validator.validate(@dummy)
assert @dummy.errors[:avatar].blank?, "Error was added to base attribute"
end
context "whitelist format" do
context "with an allowed type" do
context "as a single regexp" do
setup do
build_validator :matches => /.*\.jpg$/
@dummy.stubs(:avatar_file_name => "image.jpg")
@validator.validate(@dummy)
end
should "not set an error message" do
assert @dummy.errors[:avatar_file_name].blank?
end
end
context "as a list" do
setup do
build_validator :matches => [/.*\.png$/, /.*\.jpe?g$/]
@dummy.stubs(:avatar_file_name => "image.jpg")
@validator.validate(@dummy)
end
should "not set an error message" do
assert @dummy.errors[:avatar_file_name].blank?
end
end
end
context "with a disallowed type" do
should "set a correct default error message" do
build_validator :matches => /^text\/.*/
@dummy.stubs(:avatar_file_name => "image.jpg")
@validator.validate(@dummy)
assert @dummy.errors[:avatar_file_name].present?
assert_includes @dummy.errors[:avatar_file_name], "is invalid"
end
should "set a correct custom error message" do
build_validator :matches => /.*\.png$/, :message => "should be a PNG image"
@dummy.stubs(:avatar_file_name => "image.jpg")
@validator.validate(@dummy)
assert_includes @dummy.errors[:avatar_file_name], "should be a PNG image"
end
end
end
context "blacklist format" do
context "with an allowed type" do
context "as a single regexp" do
setup do
build_validator :not => /^text\/.*/
@dummy.stubs(:avatar_file_name => "image.jpg")
@validator.validate(@dummy)
end
should "not set an error message" do
assert @dummy.errors[:avatar_file_name].blank?
end
end
context "as a list" do
setup do
build_validator :not => [/.*\.png$/, /.*\.jpe?g$/]
@dummy.stubs(:avatar_file_name => "image.gif")
@validator.validate(@dummy)
end
should "not set an error message" do
assert @dummy.errors[:avatar_file_name].blank?
end
end
end
context "with a disallowed type" do
should "set a correct default error message" do
build_validator :not => /data.*/
@dummy.stubs(:avatar_file_name => "data.txt")
@validator.validate(@dummy)
assert @dummy.errors[:avatar_file_name].present?
assert_includes @dummy.errors[:avatar_file_name], "is invalid"
end
should "set a correct custom error message" do
build_validator :not => /.*\.png$/, :message => "should not be a PNG image"
@dummy.stubs(:avatar_file_name => "image.png")
@validator.validate(@dummy)
assert_includes @dummy.errors[:avatar_file_name], "should not be a PNG image"
end
end
end
context "using the helper" do
setup do
Dummy.validates_attachment_file_name :avatar, :matches => /.*\.jpg$/
end
should "add the validator to the class" do
assert Dummy.validators_on(:avatar).any?{ |validator| validator.kind == :attachment_file_name }
end
end
context "given options" do
should "raise argument error if no required argument was given" do
assert_raises(ArgumentError) do
build_validator :message => "Some message"
end
end
should "not raise argument error if :matches was given" do
build_validator :matches => /.*\.jpg$/
end
should "not raise argument error if :not was given" do
build_validator :not => /.*\.jpg$/
end
end
end
......@@ -61,40 +61,41 @@ class ValidatorsTest < Test::Unit::TestCase
end
end
context 'when no content_type validation exists' do
context 'with no other validations on the Dummy#avatar attachment' do
setup do
ActiveSupport::Deprecation.silenced = false
reset_class("Dummy")
Dummy.has_attached_file :avatar
Paperclip.reset_duplicate_clash_check!
end
should 'emit a deprecation warning' do
assert_deprecated do
should 'raise an error when no content_type validation exists' do
assert_raises(Paperclip::Errors::MissingRequiredValidatorError) do
Dummy.new(:avatar => File.new(fixture_file("12k.png")))
end
end
# should 'raise an error' do
# assert_raises(Paperclip::Errors::NoContentTypeValidator) do
# Dummy.new(:avatar => File.new(fixture_file("12k.png")))
# end
# end
end
context 'when a content_type validation exists' do
setup do
should 'not raise an error when a content_type validation exists' do
Dummy.validates_attachment :avatar, :content_type => { :content_type => "image/jpeg" }
ActiveSupport::Deprecation.silenced = false
assert_nothing_raised(Paperclip::Errors::MissingRequiredValidatorError) do
Dummy.new(:avatar => File.new(fixture_file("12k.png")))
end
end
should 'not emit a deprecation warning' do
assert_not_deprecated do
should 'not raise an error when a file_name validation exists' do
Dummy.validates_attachment :avatar, :file_name => { :matches => /png$/ }
assert_nothing_raised(Paperclip::Errors::MissingRequiredValidatorError) do
Dummy.new(:avatar => File.new(fixture_file("12k.png")))
end
end
# should 'not raise an error' do
# assert_nothing_raised(Paperclip::Errors::NoContentTypeValidator) do
# Dummy.new(:avatar => File.new(fixture_file("12k.png")))
# end
# end
should 'not raise an error when a the validation has been explicitly rejected' do
Dummy.validates_attachment :avatar, :file_type_ignorance => true
assert_nothing_raised(Paperclip::Errors::MissingRequiredValidatorError) do
Dummy.new(:avatar => File.new(fixture_file("12k.png")))
end
end
end
end
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment