From 357cd23d3aedabb99fd70b812ffcea2d1cc9893d Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Wed, 20 Feb 2019 20:55:09 +0900
Subject: Don't allow `where` with non numeric string matches to 0 values

This is a follow-up of #35310.

Currently `Topic.find_by(id: "not-a-number")` matches to a `id = 0`
record. That is considered as silently leaking information.

If non numeric string is given to find by an integer column, it should
not be matched to any record.

Related #12793.
---
 activemodel/lib/active_model/type/helpers/numeric.rb |  7 +++++--
 activemodel/lib/active_model/type/integer.rb         | 10 ++++------
 activemodel/test/cases/type/integer_test.rb          |  8 ++++++++
 3 files changed, 17 insertions(+), 8 deletions(-)

(limited to 'activemodel')

diff --git a/activemodel/lib/active_model/type/helpers/numeric.rb b/activemodel/lib/active_model/type/helpers/numeric.rb
index 444847a210..1d8171e25b 100644
--- a/activemodel/lib/active_model/type/helpers/numeric.rb
+++ b/activemodel/lib/active_model/type/helpers/numeric.rb
@@ -26,15 +26,18 @@ module ActiveModel
         private
 
           def number_to_non_number?(old_value, new_value_before_type_cast)
-            old_value != nil && non_numeric_string?(new_value_before_type_cast)
+            old_value != nil && non_numeric_string?(new_value_before_type_cast.to_s)
           end
 
           def non_numeric_string?(value)
             # 'wibble'.to_i will give zero, we want to make sure
             # that we aren't marking int zero to string zero as
             # changed.
-            !/\A[-+]?\d+/.match?(value.to_s)
+            !NUMERIC_REGEX.match?(value)
           end
+
+          NUMERIC_REGEX = /\A\s*[+-]?\d/
+          private_constant :NUMERIC_REGEX
       end
     end
   end
diff --git a/activemodel/lib/active_model/type/integer.rb b/activemodel/lib/active_model/type/integer.rb
index 5878b94171..a7ae57aede 100644
--- a/activemodel/lib/active_model/type/integer.rb
+++ b/activemodel/lib/active_model/type/integer.rb
@@ -24,11 +24,8 @@ module ActiveModel
       end
 
       def serialize(value)
-        result = super
-        if result
-          ensure_in_range(result)
-        end
-        result
+        return if value.is_a?(::String) && non_numeric_string?(value)
+        ensure_in_range(super)
       end
 
       private
@@ -39,9 +36,10 @@ module ActiveModel
         end
 
         def ensure_in_range(value)
-          unless range.cover?(value)
+          if value && !range.cover?(value)
             raise ActiveModel::RangeError, "#{value} is out of range for #{self.class} with limit #{_limit} bytes"
           end
+          value
         end
 
         def max_value
diff --git a/activemodel/test/cases/type/integer_test.rb b/activemodel/test/cases/type/integer_test.rb
index 9bd0110099..304ae33de0 100644
--- a/activemodel/test/cases/type/integer_test.rb
+++ b/activemodel/test/cases/type/integer_test.rb
@@ -50,6 +50,14 @@ module ActiveModel
         assert_equal 7200, type.cast(2.hours)
       end
 
+      test "casting string for database" do
+        type = Type::Integer.new
+        assert_nil type.serialize("wibble")
+        assert_equal 5, type.serialize("5wibble")
+        assert_equal 5, type.serialize(" +5")
+        assert_equal(-5, type.serialize(" -5"))
+      end
+
       test "casting empty string" do
         type = Type::Integer.new
         assert_nil type.cast("")
-- 
cgit v1.2.3