Skip to content

Commit

Permalink
Don't call #send in form object to build file inputs
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
tegon and feliperenan committed Sep 27, 2019
1 parent 62408e8 commit 8c91bd7
Show file tree
Hide file tree
Showing 9 changed files with 91 additions and 32 deletions.
14 changes: 14 additions & 0 deletions CHANGELOG.md
@@ -1,12 +1,26 @@
## Unreleased

## 5.0.0

### Enhancements
* Set multiple attribute for grouped selects also. [@ollym](https://github.com/ollym)
* Removes or renames label classes. [Abduvakilov](https://github.com/Abduvakilov)
* Support to label custom classes for inline collections. [@feliperenan](https://github.com/feliperenan)
* 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)

### 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

### Enhancements
Expand Down
4 changes: 2 additions & 2 deletions Gemfile.lock
@@ -1,7 +1,7 @@
PATH
remote: .
specs:
simple_form (4.1.0)
simple_form (5.0.0)
actionpack (>= 5.0)
activemodel (>= 5.0)

Expand Down Expand Up @@ -299,4 +299,4 @@ DEPENDENCIES
simple_form!

BUNDLED WITH
1.17.1
1.17.3
Expand Up @@ -129,9 +129,6 @@
# change this configuration to true.
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
# to match as key, and the input type that will be used when the field name
# matches the regexp as value.
Expand Down
25 changes: 21 additions & 4 deletions lib/simple_form.rb
Expand Up @@ -38,6 +38,17 @@ def %{name}(wrapper_options)
See https://github.com/plataformatec/simple_form/pull/997 for more information.
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

def self.configured? #:nodoc:
Expand Down Expand Up @@ -120,10 +131,6 @@ def self.configured? #:nodoc:
mattr_accessor :browser_validations
@@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
# 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 }.
Expand Down Expand Up @@ -265,6 +272,16 @@ def self.form_class=(value)
@@form_class = value
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
# to create a fresh initializer with all configuration values.
def self.setup
Expand Down
23 changes: 21 additions & 2 deletions lib/simple_form/form_builder.rb
Expand Up @@ -572,9 +572,28 @@ def find_custom_type(attribute_name)
}.try(:last) if SimpleForm.input_mappings
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)
file = @object.send(attribute_name) if @object.respond_to?(attribute_name)
file && SimpleForm.file_methods.any? { |m| file.respond_to?(m) }
@object.respond_to?("#{attribute_name}_attachment") ||
@object.respond_to?("#{attribute_name}_url") ||
@object.respond_to?("#{attribute_name}_attacher") ||
@object.respond_to?("#{attribute_name}_file_name")
end

def find_attribute_column(attribute_name)
Expand Down
2 changes: 1 addition & 1 deletion lib/simple_form/version.rb
@@ -1,4 +1,4 @@
# frozen_string_literal: true
module SimpleForm
VERSION = "4.1.0".freeze
VERSION = "5.0.0".freeze
end
33 changes: 13 additions & 20 deletions test/form_builder/general_test.rb
Expand Up @@ -239,31 +239,24 @@ def with_custom_form_for(object, *args, &block)
assert_select 'form select#user_updated_at_1i.datetime'
end

test 'builder generates file for file columns' do
@user.avatar = MiniTest::Mock.new
@user.avatar.expect(:public_filename, true)
@user.avatar.expect(:!, false)

with_form_for @user, :avatar
assert_select 'form input#user_avatar.file'
test 'builder generates file input for ActiveStorage >= 5.2 and Refile >= 0.2.0 <= 0.4.0' do
with_form_for UserWithAttachment.build, :avatar
assert_select 'form input#user_with_attachment_avatar.file'
end

test 'builder generates file for activestorage entries' do
@user.avatar = MiniTest::Mock.new
@user.avatar.expect(:attached?, false)
@user.avatar.expect(:!, false)

with_form_for @user, :avatar
assert_select 'form input#user_avatar.file'
test 'builder generates file input for Shrine >= 0.9.0, Refile >= 0.6.0 and CarrierWave >= 0.2.1' do
with_form_for UserWithAttachment.build, :cover
assert_select 'form input#user_with_attachment_cover.file'
end

test 'builder generates file for attributes that are real db columns but have file methods' do
@user.home_picture = MiniTest::Mock.new
@user.home_picture.expect(:mounted_as, true)
@user.home_picture.expect(:!, false)
test 'builder generates file input for Refile >= 0.4.0 and Shrine >= 0.9.0' do
with_form_for UserWithAttachment.build, :profile_image
assert_select 'form input#user_with_attachment_profile_image.file'
end

with_form_for @user, :home_picture
assert_select 'form input#user_home_picture.file'
test 'builder generates file input for Paperclip ~> 2.0' do
with_form_for UserWithAttachment.build, :portrait
assert_select 'form input#user_with_attachment_portrait.file'
end

test 'build generates select if a collection is given' do
Expand Down
18 changes: 18 additions & 0 deletions test/support/models.rb
Expand Up @@ -333,3 +333,21 @@ def name

class UserNumber1And2 < User
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
1 change: 1 addition & 0 deletions test/test_helper.rb
Expand Up @@ -91,4 +91,5 @@ def company_user_path(*args)
alias :validating_user_path :user_path
alias :validating_users_path :user_path
alias :other_validating_user_path :user_path
alias :user_with_attachment_path :user_path
end

0 comments on commit 8c91bd7

Please sign in to comment.