Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Always compact array parameters rather than setting them to nil #12251

Closed
wants to merge 17 commits into from

10 participants

Christopher Keele Steve Richert Godfrey Chan Bernard Potocki Max Joseph Zidell Yves Senn Rafael Mendonça França Arthur Nogueira Neves Aaron Patterson
Christopher Keele

Rebase reapplication of #9569, thanks to @collectiveidea and @laserlemon

The CVE-2013-0155 security vulnerability outlines how array parameters
containing nil values can be dangerous when accepted directly into
Active Record finder methods. The previous fix has been to detect array
parameters with nil values and to clobber the entire array into nil.

Instead, compacting the array solves the security issue and causes less
surprise in the case where array parameters are expected, as is often
the case when accepting nested attributes for collections.

Steve Richert laserlemon Always compact array parameters rather than setting them to nil
Rebase reapplication of #9569, thanks to @collectiveidea and @laserlemon

The CVE-2013-0155 security vulnerability outlines how array parameters
containing nil values can be dangerous when accepted directly into
Active Record finder methods. The previous fix has been to detect array
parameters with nil values and to clobber the entire array into nil.

Instead, compacting the array solves the security issue and causes less
surprise in the case where array parameters are expected, as is often
the case when accepting nested attributes for collections.
7343c01
Christopher Keele

Sheer beauty

Do it for him.

josephzidell and others added some commits
Joseph Zidell josephzidell Fixed typo in documentation 58d64dd
Joseph Zidell josephzidell Fixed return strings in documentation bf49506
Yves Senn senny Merge pull request #12440 from arunagw/minor-fix-running-unit-test-file
Directory name in RUNNING_UNIT_TESTS.rdoc [ci skip]
86780e2
Rafael Mendonça França rafaelfranca Merge pull request #12437 from websiteswithclass/master
Fixed typo in documentation
874ca68
Arthur Nogueira Neves arthurnn add regression test for set_inverse_instance on add_to_target 37cd223
Aaron Patterson tenderlove wrap logging around the actual query call itself.
This is to be consistent with the way the mysql2 adapter times queries
ffbefc7
Aaron Patterson tenderlove stop adding singleton methods to the PG connection cc04a4a
Aaron Patterson tenderlove stop adding singleton methods to the mysql2 adapter 6828ae7
Aaron Patterson tenderlove stop adding singleton methods to the SQLite3 connection 46c57ec
Aaron Patterson tenderlove rm LogIntercepter a0420d4
Aaron Patterson tenderlove prepare the statement inside the begin / rescue block 19fc886
Aaron Patterson tenderlove log every sql statement, even when they error 98e0016
Aaron Patterson tenderlove log the statement name along with the SQL 2ae9166
Aaron Patterson tenderlove instrumenter can't be cached because the app could be called from
different threads.
21a71cd
Steve Richert laserlemon Always compact array parameters rather than setting them to nil
Rebase reapplication of #9569, thanks to @collectiveidea and @laserlemon

The CVE-2013-0155 security vulnerability outlines how array parameters
containing nil values can be dangerous when accepted directly into
Active Record finder methods. The previous fix has been to detect array
parameters with nil values and to clobber the entire array into nil.

Instead, compacting the array solves the security issue and causes less
surprise in the case where array parameters are expected, as is often
the case when accepting nested attributes for collections.
d45f61a
Christopher Keele christhekeele Re-rebase off of master. 1755937
Max

just testing doing on my smart phone. testing...

Godfrey Chan
Owner

Hey guys, thank you for submitting this and expressing your concerns about this issue! Speaking for myself, I understand where you are coming from, and I agree we could do better. Unfortunately, this PR does not meet our requirements for an acceptable solution to the problem.

Please head over to #13420 for the background and contribute your ideas. Thanks again! :heart: :green_heart: :blue_heart: :yellow_heart: :purple_heart:

Bernard Potocki

May I ask for squashing this one and removing those strange commits inside? It's really hard to review it.

Except of that it looks that the only difference is that it's not changing empty array to nil but leaves it as array - this is nice as type is not changed. As I strongly believe that CVE-2013-0155 should use at least blank? instead of nil? the main idea is to be "secure by default" - even with cost of breaking some API's for now.

Fortunately you can disable deep munge in Rails 4.1 so if you need proper params handling you have tools for that. In the mean time I will gladly hear suggestions in #13215, as this is probably first try to fix problem without tradeoffs.

EDIT: Table for this one:

JSON Hash
{"person":null} {'person' => nil}
{"person":[]} {'person' => []}
{"person":[null]} {'person' => []}
{"person":[null, null, ...]} {'person' => []}
{"person":["foo", null]} {'person' => ["foo"]}

As mentioned above it maintains type passed in JSON and blank? will detect if it's empty.

Godfrey Chan chancancode was assigned
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 16, 2013
  1. Steve Richert Christopher Keele

    Always compact array parameters rather than setting them to nil

    laserlemon authored christhekeele committed
    Rebase reapplication of #9569, thanks to @collectiveidea and @laserlemon
    
    The CVE-2013-0155 security vulnerability outlines how array parameters
    containing nil values can be dangerous when accepted directly into
    Active Record finder methods. The previous fix has been to detect array
    parameters with nil values and to clobber the entire array into nil.
    
    Instead, compacting the array solves the security issue and causes less
    surprise in the case where array parameters are expected, as is often
    the case when accepting nested attributes for collections.
Commits on Oct 3, 2013
  1. Joseph Zidell
  2. Joseph Zidell
Commits on Oct 4, 2013
  1. Yves Senn

    Merge pull request #12440 from arunagw/minor-fix-running-unit-test-file

    senny authored
    Directory name in RUNNING_UNIT_TESTS.rdoc [ci skip]
  2. Rafael Mendonça França

    Merge pull request #12437 from websiteswithclass/master

    rafaelfranca authored
    Fixed typo in documentation
  3. Arthur Nogueira Neves Rafael Mendonça França
  4. Aaron Patterson

    wrap logging around the actual query call itself.

    tenderlove authored
    This is to be consistent with the way the mysql2 adapter times queries
  5. Aaron Patterson
  6. Aaron Patterson
  7. Aaron Patterson
  8. Aaron Patterson

    rm LogIntercepter

    tenderlove authored
  9. Aaron Patterson
  10. Aaron Patterson
  11. Aaron Patterson
  12. Aaron Patterson
Commits on Oct 6, 2013
  1. Steve Richert Christopher Keele

    Always compact array parameters rather than setting them to nil

    laserlemon authored christhekeele committed
    Rebase reapplication of #9569, thanks to @collectiveidea and @laserlemon
    
    The CVE-2013-0155 security vulnerability outlines how array parameters
    containing nil values can be dangerous when accepted directly into
    Active Record finder methods. The previous fix has been to detect array
    parameters with nil values and to clobber the entire array into nil.
    
    Instead, compacting the array solves the security issue and causes less
    surprise in the case where array parameters are expected, as is often
    the case when accepting nested attributes for collections.
  2. Christopher Keele
This page is out of date. Refresh to see the latest.
1  actionpack/lib/action_dispatch/request/utils.rb
View
@@ -9,7 +9,6 @@ def deep_munge(hash)
when Array
v.grep(Hash) { |x| deep_munge(x) }
v.compact!
- hash[k] = nil if v.empty?
when Hash
deep_munge(v)
end
4 actionpack/test/dispatch/request/json_params_parsing_test.rb
View
@@ -32,7 +32,7 @@ def teardown
test "nils are stripped from collections" do
assert_parses(
- {"person" => nil},
+ {"person" => []},
"{\"person\":[null]}", { 'CONTENT_TYPE' => 'application/json' }
)
assert_parses(
@@ -40,7 +40,7 @@ def teardown
"{\"person\":[\"foo\",null]}", { 'CONTENT_TYPE' => 'application/json' }
)
assert_parses(
- {"person" => nil},
+ {"person" => []},
"{\"person\":[null, null]}", { 'CONTENT_TYPE' => 'application/json' }
)
end
4 actionpack/test/dispatch/request/query_string_parsing_test.rb
View
@@ -84,8 +84,8 @@ def teardown
assert_parses({"action" => nil}, "action")
assert_parses({"action" => {"foo" => nil}}, "action[foo]")
assert_parses({"action" => {"foo" => { "bar" => nil }}}, "action[foo][bar]")
- assert_parses({"action" => {"foo" => { "bar" => nil }}}, "action[foo][bar][]")
- assert_parses({"action" => {"foo" => nil }}, "action[foo][]")
+ assert_parses({"action" => {"foo" => { "bar" => [] }}}, "action[foo][bar][]")
+ assert_parses({"action" => {"foo" => [] }}, "action[foo][]")
assert_parses({"action"=>{"foo"=>[{"bar"=>nil}]}}, "action[foo][][bar]")
end
12 actionview/lib/action_view/helpers/asset_tag_helper.rb
View
@@ -224,14 +224,14 @@ def image_tag(source, options={})
#
# ==== Examples
#
- # image_tag('rails.png')
- # # => <img alt="Rails" src="/assets/rails.png" />
+ # image_alt('rails.png')
+ # # => Rails
#
- # image_tag('hyphenated-file-name.png')
- # # => <img alt="Hyphenated file name" src="/assets/hyphenated-file-name.png" />
+ # image_alt('hyphenated-file-name.png')
+ # # => Hyphenated file name
#
- # image_tag('underscored_file_name.png')
- # # => <img alt="Underscored file name" src="/assets/underscored_file_name.png" />
+ # image_alt('underscored_file_name.png')
+ # # => Underscored file name
def image_alt(src)
File.basename(src, '.*').sub(/-[[:xdigit:]]{32}\z/, '').tr('-_', ' ').capitalize
end
11 activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
View
@@ -424,13 +424,14 @@ def close
protected
- def log(sql, name = "SQL", binds = [])
+ def log(sql, name = "SQL", binds = [], statement_name = nil)
@instrumenter.instrument(
"sql.active_record",
- :sql => sql,
- :name => name,
- :connection_id => object_id,
- :binds => binds) { yield }
+ :sql => sql,
+ :name => name,
+ :connection_id => object_id,
+ :statement_name => statement_name,
+ :binds => binds) { yield }
rescue => e
message = "#{e.class.name}: #{e.message}: #{sql}"
@logger.error message if @logger
46 activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb
View
@@ -134,35 +134,31 @@ def substitute_at(column, index)
end
def exec_query(sql, name = 'SQL', binds = [])
- log(sql, name, binds) do
- result = without_prepared_statement?(binds) ? exec_no_cache(sql, binds) :
- exec_cache(sql, binds)
-
- types = {}
- fields = result.fields
- fields.each_with_index do |fname, i|
- ftype = result.ftype i
- fmod = result.fmod i
- types[fname] = OID::TYPE_MAP.fetch(ftype, fmod) { |oid, mod|
- warn "unknown OID: #{fname}(#{oid}) (#{sql})"
- OID::Identity.new
- }
- end
-
- ret = ActiveRecord::Result.new(fields, result.values, types)
- result.clear
- return ret
+ result = without_prepared_statement?(binds) ? exec_no_cache(sql, name, binds) :
+ exec_cache(sql, name, binds)
+
+ types = {}
+ fields = result.fields
+ fields.each_with_index do |fname, i|
+ ftype = result.ftype i
+ fmod = result.fmod i
+ types[fname] = OID::TYPE_MAP.fetch(ftype, fmod) { |oid, mod|
+ warn "unknown OID: #{fname}(#{oid}) (#{sql})"
+ OID::Identity.new
+ }
end
+
+ ret = ActiveRecord::Result.new(fields, result.values, types)
+ result.clear
+ return ret
end
def exec_delete(sql, name = 'SQL', binds = [])
- log(sql, name, binds) do
- result = without_prepared_statement?(binds) ? exec_no_cache(sql, binds) :
- exec_cache(sql, binds)
- affected = result.cmd_tuples
- result.clear
- affected
- end
+ result = without_prepared_statement?(binds) ? exec_no_cache(sql, name, binds) :
+ exec_cache(sql, name, binds)
+ affected = result.cmd_tuples
+ result.clear
+ affected
end
alias :exec_update :exec_delete
28 activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
View
@@ -767,27 +767,29 @@ def initialize_type_map
FEATURE_NOT_SUPPORTED = "0A000" # :nodoc:
- def exec_no_cache(sql, binds)
- @connection.async_exec(sql)
+ def exec_no_cache(sql, name, binds)
+ log(sql, name, binds) { @connection.async_exec(sql) }
end
- def exec_cache(sql, binds)
+ def exec_cache(sql, name, binds)
stmt_key = prepare_statement(sql)
- # Clear the queue
- @connection.get_last_result
- @connection.send_query_prepared(stmt_key, binds.map { |col, val|
- type_cast(val, col)
- })
- @connection.block
- @connection.get_last_result
- rescue PGError => e
+ log(sql, name, binds, stmt_key) do
+ @connection.send_query_prepared(stmt_key, binds.map { |col, val|
+ type_cast(val, col)
+ })
+ @connection.block
+ @connection.get_last_result
+ end
+ rescue ActiveRecord::StatementInvalid => e
+ pgerror = e.original_exception
+
# Get the PG code for the failure. Annoyingly, the code for
# prepared statements whose return value may have changed is
# FEATURE_NOT_SUPPORTED. Check here for more details:
# http://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/utils/cache/plancache.c#l573
begin
- code = e.result.result_error_field(PGresult::PG_DIAG_SQLSTATE)
+ code = pgerror.result.result_error_field(PGresult::PG_DIAG_SQLSTATE)
rescue
raise e
end
@@ -812,6 +814,8 @@ def prepare_statement(sql)
unless @statements.key? sql_key
nextkey = @statements.next_key
@connection.prepare nextkey, sql
+ # Clear the queue
+ @connection.get_last_result
@statements[sql_key] = nextkey
end
@statements[sql_key]
14 activerecord/test/cases/adapters/mysql2/connection_test.rb
View
@@ -3,14 +3,14 @@
class MysqlConnectionTest < ActiveRecord::TestCase
def setup
super
+ @subscriber = SQLSubscriber.new
+ ActiveSupport::Notifications.subscribe('sql.active_record', @subscriber)
@connection = ActiveRecord::Base.connection
- @connection.extend(LogIntercepter)
- @connection.intercepted = true
end
def teardown
- @connection.intercepted = false
- @connection.logged = []
+ ActiveSupport::Notifications.unsubscribe(@subscriber)
+ super
end
def test_no_automatic_reconnection_after_timeout
@@ -72,14 +72,14 @@ def test_mysql_set_session_variable_to_default
def test_logs_name_show_variable
@connection.show_variable 'foo'
- assert_equal "SCHEMA", @connection.logged[0][1]
+ assert_equal "SCHEMA", @subscriber.logged[0][1]
end
def test_logs_name_rename_column_sql
@connection.execute "CREATE TABLE `bar_baz` (`foo` varchar(255))"
- @connection.logged = []
+ @subscriber.logged.clear
@connection.send(:rename_column_sql, 'bar_baz', 'foo', 'foo2')
- assert_equal "SCHEMA", @connection.logged[0][1]
+ assert_equal "SCHEMA", @subscriber.logged[0][1]
ensure
@connection.execute "DROP TABLE `bar_baz`"
end
32 activerecord/test/cases/adapters/postgresql/connection_test.rb
View
@@ -7,14 +7,14 @@ class NonExistentTable < ActiveRecord::Base
def setup
super
+ @subscriber = SQLSubscriber.new
+ ActiveSupport::Notifications.subscribe('sql.active_record', @subscriber)
@connection = ActiveRecord::Base.connection
- @connection.extend(LogIntercepter)
- @connection.intercepted = true
end
def teardown
- @connection.intercepted = false
- @connection.logged = []
+ ActiveSupport::Notifications.unsubscribe(@subscriber)
+ super
end
def test_encoding
@@ -47,38 +47,48 @@ def test_connection_options
def test_tables_logs_name
@connection.tables('hello')
- assert_equal 'SCHEMA', @connection.logged[0][1]
+ assert_equal 'SCHEMA', @subscriber.logged[0][1]
end
def test_indexes_logs_name
@connection.indexes('items', 'hello')
- assert_equal 'SCHEMA', @connection.logged[0][1]
+ assert_equal 'SCHEMA', @subscriber.logged[0][1]
end
def test_table_exists_logs_name
@connection.table_exists?('items')
- assert_equal 'SCHEMA', @connection.logged[0][1]
+ assert_equal 'SCHEMA', @subscriber.logged[0][1]
end
def test_table_alias_length_logs_name
@connection.instance_variable_set("@table_alias_length", nil)
@connection.table_alias_length
- assert_equal 'SCHEMA', @connection.logged[0][1]
+ assert_equal 'SCHEMA', @subscriber.logged[0][1]
end
def test_current_database_logs_name
@connection.current_database
- assert_equal 'SCHEMA', @connection.logged[0][1]
+ assert_equal 'SCHEMA', @subscriber.logged[0][1]
end
def test_encoding_logs_name
@connection.encoding
- assert_equal 'SCHEMA', @connection.logged[0][1]
+ assert_equal 'SCHEMA', @subscriber.logged[0][1]
end
def test_schema_names_logs_name
@connection.schema_names
- assert_equal 'SCHEMA', @connection.logged[0][1]
+ assert_equal 'SCHEMA', @subscriber.logged[0][1]
+ end
+
+ def test_statement_key_is_logged
+ bindval = 1
+ @connection.exec_query('SELECT $1::integer', 'SQL', [[nil, bindval]])
+ name = @subscriber.payloads.last[:statement_name]
+ assert name
+ res = @connection.exec_query("EXPLAIN (FORMAT JSON) EXECUTE #{name}(#{bindval})")
+ plan = res.column_types['QUERY PLAN'].type_cast res.rows.first.first
+ assert_operator plan.length, :>, 0
end
# Must have with_manual_interventions set to true for this
22 activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb
View
@@ -21,8 +21,8 @@ def setup
)
eosql
- @conn.extend(LogIntercepter)
- @conn.intercepted = true
+ @subscriber = SQLSubscriber.new
+ ActiveSupport::Notifications.subscribe('sql.active_record', @subscriber)
end
def test_valid_column
@@ -31,16 +31,16 @@ def test_valid_column
end
# sqlite databases should be able to support any type and not
- # just the ones mentioned in the native_database_types.
- # Therefore test_invalid column should always return true
+ # just the ones mentioned in the native_database_types.
+ # Therefore test_invalid column should always return true
# even if the type is not valid.
def test_invalid_column
assert @conn.valid_type?(:foobar)
end
def teardown
- @conn.intercepted = false
- @conn.logged = []
+ ActiveSupport::Notifications.unsubscribe(@subscriber)
+ super
end
def test_column_types
@@ -256,7 +256,7 @@ def test_tables
def test_tables_logs_name
assert_logged [['SCHEMA', []]] do
@conn.tables('hello')
- assert_not_nil @conn.logged.first.shift
+ assert_not_nil @subscriber.logged.first.shift
end
end
@@ -268,7 +268,7 @@ def test_indexes_logs_name
def test_table_exists_logs_name
assert @conn.table_exists?('items')
- assert_equal 'SCHEMA', @conn.logged[0][1]
+ assert_equal 'SCHEMA', @subscriber.logged[0][1]
end
def test_columns
@@ -306,10 +306,10 @@ def test_columns_with_not_null
end
def test_indexes_logs
- assert_difference('@conn.logged.length') do
+ assert_difference('@subscriber.logged.length') do
@conn.indexes('items')
end
- assert_match(/items/, @conn.logged.last.first)
+ assert_match(/items/, @subscriber.logged.last.first)
end
def test_no_indexes
@@ -370,7 +370,7 @@ def test_respond_to_disable_extension
def assert_logged logs
yield
- assert_equal logs, @conn.logged
+ assert_equal logs, @subscriber.logged
end
end
13 activerecord/test/cases/associations/inverse_associations_test.rb
View
@@ -446,6 +446,19 @@ def test_raise_record_not_found_error_when_no_ids_are_passed
def test_trying_to_use_inverses_that_dont_exist_should_raise_an_error
assert_raise(ActiveRecord::InverseOfAssociationNotFoundError) { Man.first.secret_interests }
end
+
+ def test_child_instance_should_point_to_parent_without_saving
+ man = Man.new
+ i = Interest.create(:topic => 'Industrial Revolution Re-enactment')
+
+ man.interests << i
+ assert_not_nil i.man
+
+ i.man.name = "Charles"
+ assert_equal i.man.name, man.name
+
+ assert !man.persisted?
+ end
end
class InverseBelongsToTests < ActiveRecord::TestCase
25 activerecord/test/cases/helper.rb
View
@@ -119,21 +119,24 @@ def travel_to(time, &block)
end
end
-module LogIntercepter
- attr_accessor :logged, :intercepted
- def self.extended(base)
- base.logged = []
+class SQLSubscriber
+ attr_reader :logged
+ attr_reader :payloads
+
+ def initialize
+ @logged = []
+ @payloads = []
end
- def log(sql, name = 'SQL', binds = [], &block)
- if @intercepted
- @logged << [sql, name, binds]
- yield
- else
- super(sql, name,binds, &block)
- end
+
+ def start(name, id, payload)
+ @payloads << payload
+ @logged << [payload[:sql], payload[:name], payload[:binds]]
end
+
+ def finish(name, id, payload); end
end
+
module InTimeZone
private
7 railties/lib/rails/rack/logger.rb
View
@@ -11,7 +11,6 @@ class Logger < ActiveSupport::LogSubscriber
def initialize(app, taggers = nil)
@app = app
@taggers = taggers || []
- @instrumenter = ActiveSupport::Notifications.instrumenter
end
def call(env)
@@ -33,7 +32,8 @@ def call_app(request, env)
logger.debug ''
end
- @instrumenter.start 'request.action_dispatch', request: request
+ instrumenter = ActiveSupport::Notifications.instrumenter
+ instrumenter.start 'request.action_dispatch', request: request
logger.info started_request_message(request)
resp = @app.call(env)
resp[2] = ::Rack::BodyProxy.new(resp[2]) { finish(request) }
@@ -70,7 +70,8 @@ def compute_tags(request)
private
def finish(request)
- @instrumenter.finish 'request.action_dispatch', request: request
+ instrumenter = ActiveSupport::Notifications.instrumenter
+ instrumenter.finish 'request.action_dispatch', request: request
end
def development?
Something went wrong with that request. Please try again.