Don't call `#send` in form object to build file inputs
Before this commit, Simple Form was calling `#send` in the form object to check whether the resulting object was an attachment. That made the library open to DOS, information disclousure and execution of unintended action attacks if a form was built with user input. ```erb <%= simple_form_for @user do |f| %> <%= f.label @user_supplied_string %> ... <% end %> ``` The solution is try to figure out if an input is of type file by checking for methods present in the most popular Ruby Gems for file uploads. The current supported Gems are: `activestorage`, `carrierwave`, `paperclip`, `shrine` and `refile`. The code is relying on public APIs so it should be fine for now. It would be nice to have a single API to perform this check, so we'll suggest one for those libraries. Co-Authored-By: Felipe Renan <feelipe.renan@gmail.com>
This commit is contained in:
parent
62408e80ae
commit
8c91bd76a5
14
CHANGELOG.md
14
CHANGELOG.md
|
@ -1,5 +1,7 @@
|
||||||
## Unreleased
|
## Unreleased
|
||||||
|
|
||||||
|
## 5.0.0
|
||||||
|
|
||||||
### Enhancements
|
### Enhancements
|
||||||
* Set multiple attribute for grouped selects also. [@ollym](https://github.com/ollym)
|
* Set multiple attribute for grouped selects also. [@ollym](https://github.com/ollym)
|
||||||
* Removes or renames label classes. [Abduvakilov](https://github.com/Abduvakilov)
|
* Removes or renames label classes. [Abduvakilov](https://github.com/Abduvakilov)
|
||||||
|
@ -7,6 +9,18 @@
|
||||||
* Update bootstrap generator template to match v4.3.x. [@m5o](https://github.com/m5o)
|
* Update bootstrap generator template to match v4.3.x. [@m5o](https://github.com/m5o)
|
||||||
* Allow "required" attribute in generated select elements of PriorityInput. [@mcountis](https://github.com/mcountis)
|
* Allow "required" attribute in generated select elements of PriorityInput. [@mcountis](https://github.com/mcountis)
|
||||||
|
|
||||||
|
### Bug fix
|
||||||
|
* Do not call `#send` in form object to check whether the attribute is a file input. [@tegon](https://github.com/tegon)
|
||||||
|
|
||||||
|
## Deprecations
|
||||||
|
* The config `SimpleForm.file_methods` is deprecated and it has no effect. Simple Form now supports automatically discover of file inputs for the following Gems: activestorage, carrierwave, paperclip, refile and shrine. If you are using a custom method that is not from one of the supported Gems, please change your forms to pass the input type explicitly:
|
||||||
|
|
||||||
|
```erb
|
||||||
|
<%= form.input :avatar, as: :file %>
|
||||||
|
```
|
||||||
|
|
||||||
|
See http://blog.plataformatec.com.br/2019/09/incorrect-access-control-in-simple-form-cve-2019-16676 for more information.
|
||||||
|
|
||||||
## 4.1.0
|
## 4.1.0
|
||||||
|
|
||||||
### Enhancements
|
### Enhancements
|
||||||
|
|
|
@ -1,7 +1,7 @@
|
||||||
PATH
|
PATH
|
||||||
remote: .
|
remote: .
|
||||||
specs:
|
specs:
|
||||||
simple_form (4.1.0)
|
simple_form (5.0.0)
|
||||||
actionpack (>= 5.0)
|
actionpack (>= 5.0)
|
||||||
activemodel (>= 5.0)
|
activemodel (>= 5.0)
|
||||||
|
|
||||||
|
@ -299,4 +299,4 @@ DEPENDENCIES
|
||||||
simple_form!
|
simple_form!
|
||||||
|
|
||||||
BUNDLED WITH
|
BUNDLED WITH
|
||||||
1.17.1
|
1.17.3
|
||||||
|
|
|
@ -129,9 +129,6 @@ SimpleForm.setup do |config|
|
||||||
# change this configuration to true.
|
# change this configuration to true.
|
||||||
config.browser_validations = false
|
config.browser_validations = false
|
||||||
|
|
||||||
# Collection of methods to detect if a file type was given.
|
|
||||||
# config.file_methods = [ :mounted_as, :file?, :public_filename, :attached? ]
|
|
||||||
|
|
||||||
# Custom mappings for input types. This should be a hash containing a regexp
|
# Custom mappings for input types. This should be a hash containing a regexp
|
||||||
# to match as key, and the input type that will be used when the field name
|
# to match as key, and the input type that will be used when the field name
|
||||||
# matches the regexp as value.
|
# matches the regexp as value.
|
||||||
|
|
|
@ -38,6 +38,17 @@ to
|
||||||
See https://github.com/plataformatec/simple_form/pull/997 for more information.
|
See https://github.com/plataformatec/simple_form/pull/997 for more information.
|
||||||
WARN
|
WARN
|
||||||
|
|
||||||
|
FILE_METHODS_DEPRECATION_WARN = <<-WARN
|
||||||
|
[SIMPLE_FORM] SimpleForm.file_methods is deprecated and has no effect.
|
||||||
|
|
||||||
|
Since version 5, Simple Form now supports automatically discover of file inputs for the following Gems: activestorage, carrierwave, paperclip, refile and shrine.
|
||||||
|
If you are using a custom method that is not from one of the supported Gems, please change your forms to pass the input type explicitly:
|
||||||
|
|
||||||
|
<%= form.input :avatar, as: :file %>
|
||||||
|
|
||||||
|
See http://blog.plataformatec.com.br/2019/09/incorrect-access-control-in-simple-form-cve-2019-16676 for more information.
|
||||||
|
WARN
|
||||||
|
|
||||||
@@configured = false
|
@@configured = false
|
||||||
|
|
||||||
def self.configured? #:nodoc:
|
def self.configured? #:nodoc:
|
||||||
|
@ -120,10 +131,6 @@ See https://github.com/plataformatec/simple_form/pull/997 for more information.
|
||||||
mattr_accessor :browser_validations
|
mattr_accessor :browser_validations
|
||||||
@@browser_validations = true
|
@@browser_validations = true
|
||||||
|
|
||||||
# Collection of methods to detect if a file type was given.
|
|
||||||
mattr_accessor :file_methods
|
|
||||||
@@file_methods = %i[mounted_as file? public_filename attached?]
|
|
||||||
|
|
||||||
# Custom mappings for input types. This should be a hash containing a regexp
|
# Custom mappings for input types. This should be a hash containing a regexp
|
||||||
# to match as key, and the input type that will be used when the field name
|
# to match as key, and the input type that will be used when the field name
|
||||||
# matches the regexp as value, such as { /count/ => :integer }.
|
# matches the regexp as value, such as { /count/ => :integer }.
|
||||||
|
@ -265,6 +272,16 @@ See https://github.com/plataformatec/simple_form/pull/997 for more information.
|
||||||
@@form_class = value
|
@@form_class = value
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def self.file_methods=(file_methods)
|
||||||
|
ActiveSupport::Deprecation.warn(FILE_METHODS_DEPRECATION_WARN, caller)
|
||||||
|
@@file_methods = file_methods
|
||||||
|
end
|
||||||
|
|
||||||
|
def self.file_methods
|
||||||
|
ActiveSupport::Deprecation.warn(FILE_METHODS_DEPRECATION_WARN, caller)
|
||||||
|
@@file_methods
|
||||||
|
end
|
||||||
|
|
||||||
# Default way to setup Simple Form. Run rails generate simple_form:install
|
# Default way to setup Simple Form. Run rails generate simple_form:install
|
||||||
# to create a fresh initializer with all configuration values.
|
# to create a fresh initializer with all configuration values.
|
||||||
def self.setup
|
def self.setup
|
||||||
|
|
|
@ -572,9 +572,28 @@ module SimpleForm
|
||||||
}.try(:last) if SimpleForm.input_mappings
|
}.try(:last) if SimpleForm.input_mappings
|
||||||
end
|
end
|
||||||
|
|
||||||
|
# Internal: Try to discover whether an attribute corresponds to a file or not.
|
||||||
|
#
|
||||||
|
# Most upload Gems add some kind of attributes to the ActiveRecord's model they are included in.
|
||||||
|
# This method tries to guess if an attribute belongs to some of these Gems by checking the presence
|
||||||
|
# of their methods using `#respond_to?`.
|
||||||
|
#
|
||||||
|
# Note: This does not support multiple file upload inputs, as this is very application-specific.
|
||||||
|
#
|
||||||
|
# The order here was choosen based on the popularity of Gems and for commodity - e.g. the method
|
||||||
|
# with the suffix `_url` is present in three Gems, so it's checked with priority:
|
||||||
|
#
|
||||||
|
# - `#{attribute_name}_attachment` - ActiveStorage >= `5.2` and Refile >= `0.2.0` <= `0.4.0`
|
||||||
|
# - `#{attribute_name}_url` - Shrine >= `0.9.0`, Refile >= `0.6.0` and CarrierWave >= `0.2.1`
|
||||||
|
# - `#{attribute_name}_attacher` - Refile >= `0.4.0` and Shrine >= `0.9.0`
|
||||||
|
# - `#{attribute_name}_file_name` - Paperclip ~> `2.0` (added for backwards compatibility)
|
||||||
|
#
|
||||||
|
# Returns a Boolean.
|
||||||
def file_method?(attribute_name)
|
def file_method?(attribute_name)
|
||||||
file = @object.send(attribute_name) if @object.respond_to?(attribute_name)
|
@object.respond_to?("#{attribute_name}_attachment") ||
|
||||||
file && SimpleForm.file_methods.any? { |m| file.respond_to?(m) }
|
@object.respond_to?("#{attribute_name}_url") ||
|
||||||
|
@object.respond_to?("#{attribute_name}_attacher") ||
|
||||||
|
@object.respond_to?("#{attribute_name}_file_name")
|
||||||
end
|
end
|
||||||
|
|
||||||
def find_attribute_column(attribute_name)
|
def find_attribute_column(attribute_name)
|
||||||
|
|
|
@ -1,4 +1,4 @@
|
||||||
# frozen_string_literal: true
|
# frozen_string_literal: true
|
||||||
module SimpleForm
|
module SimpleForm
|
||||||
VERSION = "4.1.0".freeze
|
VERSION = "5.0.0".freeze
|
||||||
end
|
end
|
||||||
|
|
|
@ -239,31 +239,24 @@ class FormBuilderTest < ActionView::TestCase
|
||||||
assert_select 'form select#user_updated_at_1i.datetime'
|
assert_select 'form select#user_updated_at_1i.datetime'
|
||||||
end
|
end
|
||||||
|
|
||||||
test 'builder generates file for file columns' do
|
test 'builder generates file input for ActiveStorage >= 5.2 and Refile >= 0.2.0 <= 0.4.0' do
|
||||||
@user.avatar = MiniTest::Mock.new
|
with_form_for UserWithAttachment.build, :avatar
|
||||||
@user.avatar.expect(:public_filename, true)
|
assert_select 'form input#user_with_attachment_avatar.file'
|
||||||
@user.avatar.expect(:!, false)
|
|
||||||
|
|
||||||
with_form_for @user, :avatar
|
|
||||||
assert_select 'form input#user_avatar.file'
|
|
||||||
end
|
end
|
||||||
|
|
||||||
test 'builder generates file for activestorage entries' do
|
test 'builder generates file input for Shrine >= 0.9.0, Refile >= 0.6.0 and CarrierWave >= 0.2.1' do
|
||||||
@user.avatar = MiniTest::Mock.new
|
with_form_for UserWithAttachment.build, :cover
|
||||||
@user.avatar.expect(:attached?, false)
|
assert_select 'form input#user_with_attachment_cover.file'
|
||||||
@user.avatar.expect(:!, false)
|
|
||||||
|
|
||||||
with_form_for @user, :avatar
|
|
||||||
assert_select 'form input#user_avatar.file'
|
|
||||||
end
|
end
|
||||||
|
|
||||||
test 'builder generates file for attributes that are real db columns but have file methods' do
|
test 'builder generates file input for Refile >= 0.4.0 and Shrine >= 0.9.0' do
|
||||||
@user.home_picture = MiniTest::Mock.new
|
with_form_for UserWithAttachment.build, :profile_image
|
||||||
@user.home_picture.expect(:mounted_as, true)
|
assert_select 'form input#user_with_attachment_profile_image.file'
|
||||||
@user.home_picture.expect(:!, false)
|
end
|
||||||
|
|
||||||
with_form_for @user, :home_picture
|
test 'builder generates file input for Paperclip ~> 2.0' do
|
||||||
assert_select 'form input#user_home_picture.file'
|
with_form_for UserWithAttachment.build, :portrait
|
||||||
|
assert_select 'form input#user_with_attachment_portrait.file'
|
||||||
end
|
end
|
||||||
|
|
||||||
test 'build generates select if a collection is given' do
|
test 'build generates select if a collection is given' do
|
||||||
|
|
|
@ -333,3 +333,21 @@ end
|
||||||
|
|
||||||
class UserNumber1And2 < User
|
class UserNumber1And2 < User
|
||||||
end
|
end
|
||||||
|
|
||||||
|
class UserWithAttachment < User
|
||||||
|
def avatar_attachment
|
||||||
|
OpenStruct.new
|
||||||
|
end
|
||||||
|
|
||||||
|
def cover_url
|
||||||
|
"/uploads/cover.png"
|
||||||
|
end
|
||||||
|
|
||||||
|
def profile_image_attacher
|
||||||
|
OpenStruct.new
|
||||||
|
end
|
||||||
|
|
||||||
|
def portrait_file_name
|
||||||
|
"portrait.png"
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
|
@ -91,4 +91,5 @@ class ActionView::TestCase
|
||||||
alias :validating_user_path :user_path
|
alias :validating_user_path :user_path
|
||||||
alias :validating_users_path :user_path
|
alias :validating_users_path :user_path
|
||||||
alias :other_validating_user_path :user_path
|
alias :other_validating_user_path :user_path
|
||||||
|
alias :user_with_attachment_path :user_path
|
||||||
end
|
end
|
||||||
|
|
Loading…
Reference in New Issue