From b09d8f6bb3a23cd907d084103fb5b4c02479a39b Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Mon, 18 Feb 2019 15:27:23 +0900
Subject: [PATCH] Don't allow `where` with invalid value matches to nil values

That is considered as silently leaking information.
If type casting doesn't return any actual value, it should not be
matched to any record.

Fixes #33624.
Closes #33946.
---
 activemodel/lib/active_model/type/time.rb                 | 4 ----
 activerecord/CHANGELOG.md                                 | 6 ++++++
 .../lib/active_record/relation/query_attribute.rb         | 6 ++++--
 activerecord/test/cases/adapters/postgresql/uuid_test.rb  | 6 ++++++
 activerecord/test/cases/relation/where_test.rb            | 8 ++++++--
 5 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/activemodel/lib/active_model/type/time.rb b/activemodel/lib/active_model/type/time.rb
index 16d3efb728c..61847a4ce74 100644
--- a/activemodel/lib/active_model/type/time.rb
+++ b/activemodel/lib/active_model/type/time.rb
@@ -13,10 +13,6 @@ module ActiveModel
         :time
       end
 
-      def serialize(value)
-        super || value
-      end
-
       def user_input_in_time_zone(value)
         return unless value.present?
 
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 8d8aa893688..c1ce01c3125 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+*   Don't allow `where` with invalid value matches to nil values.
+
+    Fixes #33624.
+
+    *Ryuta Kamizono*
+
 *   SQLite3: Implement `add_foreign_key` and `remove_foreign_key`.
 
     *Ryuta Kamizono*
diff --git a/activerecord/lib/active_record/relation/query_attribute.rb b/activerecord/lib/active_record/relation/query_attribute.rb
index 1dd6462d8df..cd18f27330d 100644
--- a/activerecord/lib/active_record/relation/query_attribute.rb
+++ b/activerecord/lib/active_record/relation/query_attribute.rb
@@ -18,8 +18,10 @@ module ActiveRecord
       end
 
       def nil?
-        !value_before_type_cast.is_a?(StatementCache::Substitute) &&
-          (value_before_type_cast.nil? || value_for_database.nil?)
+        unless value_before_type_cast.is_a?(StatementCache::Substitute)
+          value_before_type_cast.nil? ||
+            type.respond_to?(:subtype, true) && value_for_database.nil?
+        end
       rescue ::RangeError
       end
 
diff --git a/activerecord/test/cases/adapters/postgresql/uuid_test.rb b/activerecord/test/cases/adapters/postgresql/uuid_test.rb
index 9912763c1b9..6591d50d068 100644
--- a/activerecord/test/cases/adapters/postgresql/uuid_test.rb
+++ b/activerecord/test/cases/adapters/postgresql/uuid_test.rb
@@ -114,6 +114,12 @@ class PostgresqlUUIDTest < ActiveRecord::PostgreSQLTestCase
     assert_equal "foobar", uuid.guid_before_type_cast
   end
 
+  def test_invalid_uuid_dont_match_to_nil
+    UUIDType.create!
+    assert_empty UUIDType.where(guid: "")
+    assert_empty UUIDType.where(guid: "foobar")
+  end
+
   def test_acceptable_uuid_regex
     # Valid uuids
     ["A0EEBC99-9C0B-4EF8-BB6D-6BB9BD380A11",
diff --git a/activerecord/test/cases/relation/where_test.rb b/activerecord/test/cases/relation/where_test.rb
index d49ed092b24..bec204643b4 100644
--- a/activerecord/test/cases/relation/where_test.rb
+++ b/activerecord/test/cases/relation/where_test.rb
@@ -50,8 +50,12 @@ module ActiveRecord
       assert_equal [chef], chefs.to_a
     end
 
-    def test_where_with_casted_value_is_nil
-      assert_equal 4, Topic.where(last_read: "").count
+    def test_where_with_invalid_value
+      topics(:first).update!(written_on: nil, bonus_time: nil, last_read: nil)
+      assert_empty Topic.where(parent_id: Object.new)
+      assert_empty Topic.where(written_on: "")
+      assert_empty Topic.where(bonus_time: "")
+      assert_empty Topic.where(last_read: "")
     end
 
     def test_rewhere_on_root