Skip to content

Commit 8c91bd7

Browse files
tegonfeliperenan
andcommitted
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>
1 parent 62408e8 commit 8c91bd7

File tree

9 files changed

+91
-32
lines changed

9 files changed

+91
-32
lines changed

CHANGELOG.md

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,26 @@
11
## Unreleased
22

3+
## 5.0.0
4+
35
### Enhancements
46
* Set multiple attribute for grouped selects also. [@ollym](https://github.com/ollym)
57
* Removes or renames label classes. [Abduvakilov](https://github.com/Abduvakilov)
68
* Support to label custom classes for inline collections. [@feliperenan](https://github.com/feliperenan)
79
* Update bootstrap generator template to match v4.3.x. [@m5o](https://github.com/m5o)
810
* Allow "required" attribute in generated select elements of PriorityInput. [@mcountis](https://github.com/mcountis)
911

12+
### Bug fix
13+
* Do not call `#send` in form object to check whether the attribute is a file input. [@tegon](https://github.com/tegon)
14+
15+
## Deprecations
16+
* 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:
17+
18+
```erb
19+
<%= form.input :avatar, as: :file %>
20+
```
21+
22+
See http://blog.plataformatec.com.br/2019/09/incorrect-access-control-in-simple-form-cve-2019-16676 for more information.
23+
1024
## 4.1.0
1125

1226
### Enhancements

Gemfile.lock

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
PATH
22
remote: .
33
specs:
4-
simple_form (4.1.0)
4+
simple_form (5.0.0)
55
actionpack (>= 5.0)
66
activemodel (>= 5.0)
77

@@ -299,4 +299,4 @@ DEPENDENCIES
299299
simple_form!
300300

301301
BUNDLED WITH
302-
1.17.1
302+
1.17.3

lib/generators/simple_form/templates/config/initializers/simple_form.rb

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -129,9 +129,6 @@
129129
# change this configuration to true.
130130
config.browser_validations = false
131131

132-
# Collection of methods to detect if a file type was given.
133-
# config.file_methods = [ :mounted_as, :file?, :public_filename, :attached? ]
134-
135132
# Custom mappings for input types. This should be a hash containing a regexp
136133
# to match as key, and the input type that will be used when the field name
137134
# matches the regexp as value.

lib/simple_form.rb

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,17 @@ def %{name}(wrapper_options)
3838
See https://github.com/plataformatec/simple_form/pull/997 for more information.
3939
WARN
4040

41+
FILE_METHODS_DEPRECATION_WARN = <<-WARN
42+
[SIMPLE_FORM] SimpleForm.file_methods is deprecated and has no effect.
43+
44+
Since version 5, Simple Form now supports automatically discover of file inputs for the following Gems: activestorage, carrierwave, paperclip, refile and shrine.
45+
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:
46+
47+
<%= form.input :avatar, as: :file %>
48+
49+
See http://blog.plataformatec.com.br/2019/09/incorrect-access-control-in-simple-form-cve-2019-16676 for more information.
50+
WARN
51+
4152
@@configured = false
4253

4354
def self.configured? #:nodoc:
@@ -120,10 +131,6 @@ def self.configured? #:nodoc:
120131
mattr_accessor :browser_validations
121132
@@browser_validations = true
122133

123-
# Collection of methods to detect if a file type was given.
124-
mattr_accessor :file_methods
125-
@@file_methods = %i[mounted_as file? public_filename attached?]
126-
127134
# Custom mappings for input types. This should be a hash containing a regexp
128135
# to match as key, and the input type that will be used when the field name
129136
# matches the regexp as value, such as { /count/ => :integer }.
@@ -265,6 +272,16 @@ def self.form_class=(value)
265272
@@form_class = value
266273
end
267274

275+
def self.file_methods=(file_methods)
276+
ActiveSupport::Deprecation.warn(FILE_METHODS_DEPRECATION_WARN, caller)
277+
@@file_methods = file_methods
278+
end
279+
280+
def self.file_methods
281+
ActiveSupport::Deprecation.warn(FILE_METHODS_DEPRECATION_WARN, caller)
282+
@@file_methods
283+
end
284+
268285
# Default way to setup Simple Form. Run rails generate simple_form:install
269286
# to create a fresh initializer with all configuration values.
270287
def self.setup

lib/simple_form/form_builder.rb

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -572,9 +572,28 @@ def find_custom_type(attribute_name)
572572
}.try(:last) if SimpleForm.input_mappings
573573
end
574574

575+
# Internal: Try to discover whether an attribute corresponds to a file or not.
576+
#
577+
# Most upload Gems add some kind of attributes to the ActiveRecord's model they are included in.
578+
# This method tries to guess if an attribute belongs to some of these Gems by checking the presence
579+
# of their methods using `#respond_to?`.
580+
#
581+
# Note: This does not support multiple file upload inputs, as this is very application-specific.
582+
#
583+
# The order here was choosen based on the popularity of Gems and for commodity - e.g. the method
584+
# with the suffix `_url` is present in three Gems, so it's checked with priority:
585+
#
586+
# - `#{attribute_name}_attachment` - ActiveStorage >= `5.2` and Refile >= `0.2.0` <= `0.4.0`
587+
# - `#{attribute_name}_url` - Shrine >= `0.9.0`, Refile >= `0.6.0` and CarrierWave >= `0.2.1`
588+
# - `#{attribute_name}_attacher` - Refile >= `0.4.0` and Shrine >= `0.9.0`
589+
# - `#{attribute_name}_file_name` - Paperclip ~> `2.0` (added for backwards compatibility)
590+
#
591+
# Returns a Boolean.
575592
def file_method?(attribute_name)
576-
file = @object.send(attribute_name) if @object.respond_to?(attribute_name)
577-
file && SimpleForm.file_methods.any? { |m| file.respond_to?(m) }
593+
@object.respond_to?("#{attribute_name}_attachment") ||
594+
@object.respond_to?("#{attribute_name}_url") ||
595+
@object.respond_to?("#{attribute_name}_attacher") ||
596+
@object.respond_to?("#{attribute_name}_file_name")
578597
end
579598

580599
def find_attribute_column(attribute_name)

lib/simple_form/version.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
# frozen_string_literal: true
22
module SimpleForm
3-
VERSION = "4.1.0".freeze
3+
VERSION = "5.0.0".freeze
44
end

test/form_builder/general_test.rb

Lines changed: 13 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -239,31 +239,24 @@ def with_custom_form_for(object, *args, &block)
239239
assert_select 'form select#user_updated_at_1i.datetime'
240240
end
241241

242-
test 'builder generates file for file columns' do
243-
@user.avatar = MiniTest::Mock.new
244-
@user.avatar.expect(:public_filename, true)
245-
@user.avatar.expect(:!, false)
246-
247-
with_form_for @user, :avatar
248-
assert_select 'form input#user_avatar.file'
242+
test 'builder generates file input for ActiveStorage >= 5.2 and Refile >= 0.2.0 <= 0.4.0' do
243+
with_form_for UserWithAttachment.build, :avatar
244+
assert_select 'form input#user_with_attachment_avatar.file'
249245
end
250246

251-
test 'builder generates file for activestorage entries' do
252-
@user.avatar = MiniTest::Mock.new
253-
@user.avatar.expect(:attached?, false)
254-
@user.avatar.expect(:!, false)
255-
256-
with_form_for @user, :avatar
257-
assert_select 'form input#user_avatar.file'
247+
test 'builder generates file input for Shrine >= 0.9.0, Refile >= 0.6.0 and CarrierWave >= 0.2.1' do
248+
with_form_for UserWithAttachment.build, :cover
249+
assert_select 'form input#user_with_attachment_cover.file'
258250
end
259251

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

265-
with_form_for @user, :home_picture
266-
assert_select 'form input#user_home_picture.file'
257+
test 'builder generates file input for Paperclip ~> 2.0' do
258+
with_form_for UserWithAttachment.build, :portrait
259+
assert_select 'form input#user_with_attachment_portrait.file'
267260
end
268261

269262
test 'build generates select if a collection is given' do

test/support/models.rb

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,3 +333,21 @@ def name
333333

334334
class UserNumber1And2 < User
335335
end
336+
337+
class UserWithAttachment < User
338+
def avatar_attachment
339+
OpenStruct.new
340+
end
341+
342+
def cover_url
343+
"/uploads/cover.png"
344+
end
345+
346+
def profile_image_attacher
347+
OpenStruct.new
348+
end
349+
350+
def portrait_file_name
351+
"portrait.png"
352+
end
353+
end

test/test_helper.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,4 +91,5 @@ def company_user_path(*args)
9191
alias :validating_user_path :user_path
9292
alias :validating_users_path :user_path
9393
alias :other_validating_user_path :user_path
94+
alias :user_with_attachment_path :user_path
9495
end

0 commit comments

Comments
 (0)