diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 17a9b805bf..141c53c54b 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,30 @@ +* The MySQL adapter now cast numbers and booleans bind parameters to to string for safety reasons. + + When comparing a string and a number in a query, MySQL convert the string to a number. So for + instance `"foo" = 0`, will implicitly cast `"foo"` to `0` and will evaluate to `TRUE` which can + lead to security vulnerabilities. + + Active Record already protect against that vulnerability when it knows the type of the column + being compared, however until now it was still vulnerable when using bind parameters: + + ```ruby + User.where("login_token = ?", 0).first + ``` + + Would perform: + + ```sql + SELECT * FROM `users` WHERE `login_token` = 0 LIMIT 1; + ``` + + Now it will perform: + + ```sql + SELECT * FROM `users` WHERE `login_token` = '0' LIMIT 1; + ``` + + *Jean Boussier* + * Fixture configurations (`_fixture`) are now strictly validated. If an error will be raised if that entry contains unknown keys while previously it diff --git a/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb b/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb index 9e85162013..eb6edbf8e5 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb @@ -43,6 +43,13 @@ module ActiveRecord _type_cast(value) end + # Quote a value to be used as a bound parameter of unknown type. For example, + # MySQL might perform dangerous castings when comparing a string to a number, + # so this method will cast numbers to string. + def quote_bound_value(value) + _quote(value) + end + # If you are having to call this function, you are likely doing something # wrong. The column does not have sufficient type information if the user # provided a custom type on the class level either explicitly (via diff --git a/activerecord/lib/active_record/connection_adapters/mysql/quoting.rb b/activerecord/lib/active_record/connection_adapters/mysql/quoting.rb index f649b64f7d..cdb4f12a4e 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql/quoting.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql/quoting.rb @@ -6,6 +6,21 @@ module ActiveRecord module ConnectionAdapters module MySQL module Quoting # :nodoc: + def quote_bound_value(value) + case value + when Numeric + _quote(value.to_s) + when BigDecimal + _quote(value.to_s("F")) + when true + "'1'" + when false + "'0'" + else + _quote(value) + end + end + def quote_column_name(name) self.class.quoted_column_names[name] ||= "`#{super.gsub('`', '``')}`" end diff --git a/activerecord/lib/active_record/sanitization.rb b/activerecord/lib/active_record/sanitization.rb index df8027b6d0..1545c28a07 100644 --- a/activerecord/lib/active_record/sanitization.rb +++ b/activerecord/lib/active_record/sanitization.rb @@ -183,13 +183,13 @@ module ActiveRecord if value.respond_to?(:map) && !value.acts_like?(:string) values = value.map { |v| v.respond_to?(:id_for_database) ? v.id_for_database : v } if values.empty? - c.quote(nil) + c.quote_bound_value(nil) else - values.map! { |v| c.quote(v) }.join(",") + values.map! { |v| c.quote_bound_value(v) }.join(",") end else value = value.id_for_database if value.respond_to?(:id_for_database) - c.quote(value) + c.quote_bound_value(value) end end diff --git a/activerecord/test/cases/adapters/mysql2/quoting_test.rb b/activerecord/test/cases/adapters/mysql2/quoting_test.rb new file mode 100644 index 0000000000..54e3426377 --- /dev/null +++ b/activerecord/test/cases/adapters/mysql2/quoting_test.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require "cases/helper" + +class Mysql2QuotingTest < ActiveRecord::Mysql2TestCase + def setup + super + @conn = ActiveRecord::Base.connection + end + + def test_quote_bound_integer + assert_equal "'42'", @conn.quote_bound_value(42) + end + + def test_quote_bound_big_decimal + assert_equal "'4.2'", @conn.quote_bound_value(BigDecimal("4.2")) + end + + def test_quote_bound_true + assert_equal "'1'", @conn.quote_bound_value(true) + end + + def test_quote_bound_false + assert_equal "'0'", @conn.quote_bound_value(false) + end +end diff --git a/activerecord/test/cases/insert_all_test.rb b/activerecord/test/cases/insert_all_test.rb index 635861aee7..9b4660efd6 100644 --- a/activerecord/test/cases/insert_all_test.rb +++ b/activerecord/test/cases/insert_all_test.rb @@ -476,7 +476,7 @@ class InsertAllTest < ActiveRecord::TestCase def test_upsert_all_updates_using_provided_sql skip unless supports_insert_on_duplicate_update? - operator = sqlite? ? "MAX" : "GREATEST" + operator = current_adapter?(:SQLite3Adapter) ? "MAX" : "GREATEST" Book.upsert_all( [{ id: 1, status: 1 }, { id: 2, status: 1 }], @@ -506,8 +506,4 @@ class InsertAllTest < ActiveRecord::TestCase ActiveRecord::Base.logger = old_logger end end - - def sqlite? - ActiveRecord::Base.connection.adapter_name.match?(/sqlite/i) - end end diff --git a/activerecord/test/cases/relation/merging_test.rb b/activerecord/test/cases/relation/merging_test.rb index 8247d108a4..19ae172d29 100644 --- a/activerecord/test/cases/relation/merging_test.rb +++ b/activerecord/test/cases/relation/merging_test.rb @@ -205,12 +205,22 @@ class RelationMergingTest < ActiveRecord::TestCase only_david = Author.where("#{author_id} IN (?)", david) - assert_sql(/WHERE \(#{Regexp.escape(author_id)} IN \(1\)\)\z/) do - assert_equal [david], only_david.merge(only_david) - end + if current_adapter?(:Mysql2Adapter) + assert_sql(/WHERE \(#{Regexp.escape(author_id)} IN \('1'\)\)\z/) do + assert_equal [david], only_david.merge(only_david) + end - assert_sql(/WHERE \(#{Regexp.escape(author_id)} IN \(1\)\)\z/) do - assert_equal [david], only_david.merge(only_david, rewhere: true) + assert_sql(/WHERE \(#{Regexp.escape(author_id)} IN \('1'\)\)\z/) do + assert_equal [david], only_david.merge(only_david, rewhere: true) + end + else + assert_sql(/WHERE \(#{Regexp.escape(author_id)} IN \(1\)\)\z/) do + assert_equal [david], only_david.merge(only_david) + end + + assert_sql(/WHERE \(#{Regexp.escape(author_id)} IN \(1\)\)\z/) do + assert_equal [david], only_david.merge(only_david, rewhere: true) + end end end diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index 350acc0fe3..70051b58f4 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -473,7 +473,11 @@ class RelationTest < ActiveRecord::TestCase def test_finding_with_sanitized_order query = Tag.order([Arel.sql("field(id, ?)"), [1, 3, 2]]).to_sql - assert_match(/field\(id, 1,3,2\)/, query) + if current_adapter?(:Mysql2Adapter) + assert_match(/field\(id, '1','3','2'\)/, query) + else + assert_match(/field\(id, 1,3,2\)/, query) + end query = Tag.order([Arel.sql("field(id, ?)"), []]).to_sql assert_match(/field\(id, NULL\)/, query) diff --git a/activerecord/test/cases/sanitize_test.rb b/activerecord/test/cases/sanitize_test.rb index 6c884b4f45..baa7d1bdad 100644 --- a/activerecord/test/cases/sanitize_test.rb +++ b/activerecord/test/cases/sanitize_test.rb @@ -31,7 +31,11 @@ class SanitizeTest < ActiveRecord::TestCase def test_sanitize_sql_array_handles_named_bind_variables quoted_bambi = ActiveRecord::Base.connection.quote("Bambi") assert_equal "name=#{quoted_bambi}", Binary.sanitize_sql_array(["name=:name", name: "Bambi"]) - assert_equal "name=#{quoted_bambi} AND id=1", Binary.sanitize_sql_array(["name=:name AND id=:id", name: "Bambi", id: 1]) + if current_adapter?(:Mysql2Adapter) + assert_equal "name=#{quoted_bambi} AND id='1'", Binary.sanitize_sql_array(["name=:name AND id=:id", name: "Bambi", id: 1]) + else + assert_equal "name=#{quoted_bambi} AND id=1", Binary.sanitize_sql_array(["name=:name AND id=:id", name: "Bambi", id: 1]) + end quoted_bambi_and_thumper = ActiveRecord::Base.connection.quote("Bambi\nand\nThumper") assert_equal "name=#{quoted_bambi_and_thumper}", Binary.sanitize_sql_array(["name=:name", name: "Bambi\nand\nThumper"]) @@ -101,8 +105,13 @@ class SanitizeTest < ActiveRecord::TestCase end def test_named_bind_variables - assert_equal "1", bind(":a", a: 1) # ' ruby-mode - assert_equal "1 1", bind(":a :a", a: 1) # ' ruby-mode + if current_adapter?(:Mysql2Adapter) + assert_equal "'1'", bind(":a", a: 1) # ' ruby-mode + assert_equal "'1' '1'", bind(":a :a", a: 1) # ' ruby-mode + else + assert_equal "1", bind(":a", a: 1) # ' ruby-mode + assert_equal "1 1", bind(":a :a", a: 1) # ' ruby-mode + end assert_nothing_raised { bind("'+00:00'", foo: "bar") } end @@ -128,16 +137,32 @@ class SanitizeTest < ActiveRecord::TestCase def test_bind_enumerable quoted_abc = %(#{ActiveRecord::Base.connection.quote('a')},#{ActiveRecord::Base.connection.quote('b')},#{ActiveRecord::Base.connection.quote('c')}) - assert_equal "1,2,3", bind("?", [1, 2, 3]) + if current_adapter?(:Mysql2Adapter) + assert_equal "'1','2','3'", bind("?", [1, 2, 3]) + else + assert_equal "1,2,3", bind("?", [1, 2, 3]) + end assert_equal quoted_abc, bind("?", %w(a b c)) - assert_equal "1,2,3", bind(":a", a: [1, 2, 3]) + if current_adapter?(:Mysql2Adapter) + assert_equal "'1','2','3'", bind(":a", a: [1, 2, 3]) + else + assert_equal "1,2,3", bind(":a", a: [1, 2, 3]) + end assert_equal quoted_abc, bind(":a", a: %w(a b c)) # ' - assert_equal "1,2,3", bind("?", SimpleEnumerable.new([1, 2, 3])) + if current_adapter?(:Mysql2Adapter) + assert_equal "'1','2','3'", bind("?", SimpleEnumerable.new([1, 2, 3])) + else + assert_equal "1,2,3", bind("?", SimpleEnumerable.new([1, 2, 3])) + end assert_equal quoted_abc, bind("?", SimpleEnumerable.new(%w(a b c))) - assert_equal "1,2,3", bind(":a", a: SimpleEnumerable.new([1, 2, 3])) + if current_adapter?(:Mysql2Adapter) + assert_equal "'1','2','3'", bind(":a", a: SimpleEnumerable.new([1, 2, 3])) + else + assert_equal "1,2,3", bind(":a", a: SimpleEnumerable.new([1, 2, 3])) + end assert_equal quoted_abc, bind(":a", a: SimpleEnumerable.new(%w(a b c))) # ' end @@ -150,8 +175,13 @@ class SanitizeTest < ActiveRecord::TestCase def test_bind_range quoted_abc = %(#{ActiveRecord::Base.connection.quote('a')},#{ActiveRecord::Base.connection.quote('b')},#{ActiveRecord::Base.connection.quote('c')}) - assert_equal "0", bind("?", 0..0) - assert_equal "1,2,3", bind("?", 1..3) + if current_adapter?(:Mysql2Adapter) + assert_equal "'0'", bind("?", 0..0) + assert_equal "'1','2','3'", bind("?", 1..3) + else + assert_equal "0", bind("?", 0..0) + assert_equal "1,2,3", bind("?", 1..3) + end assert_equal quoted_abc, bind("?", "a"..."d") end