Re-write legacy (marshal) cookies on read

2 files changed, 57 insertions, 35 deletions

diff --git a/actionpack/lib/action_dispatch/middleware/cookies.rb b/actionpack/lib/action_dispatch/middleware/cookies.rb
index fa94f9c9e4..2af45d43bb 100644
--- a/actionpack/lib/action_dispatch/middleware/cookies.rb
+++ b/actionpack/lib/action_dispatch/middleware/cookies.rb
@@ -384,29 +384,48 @@ module ActionDispatch
       end
     end
 
-    class HybridSerializer < JsonSerializer
-      MARSHAL_SIGNATURE = "\x04\x08".freeze
-
+    # Passing the NullSerializer downstream to the Message{Encryptor,Verifier}
+    # allows us to handle the (de)serialization step within the cookie jar,
+    # which gives us the opportunity to detect and migrate legacy cookies.
+    class NullSerializer
       def self.load(value)
-        if value.start_with?(MARSHAL_SIGNATURE)
-          Marshal.load(value)
-        else
-          super
-        end
+        value
+      end
+
+      def self.dump(value)
+        value
       end
     end
 
     module SerializedCookieJars
+      MARSHAL_SIGNATURE = "\x04\x08".freeze
+
       protected
+        def needs_migration?(value)
+          @options[:serializer] == :hybrid && value.start_with?(MARSHAL_SIGNATURE)
+        end
+
+        def serialize(name, value)
+          serializer.dump(value)
+        end
+
+        def deserialize(name, value)
+          if value
+            if needs_migration?(value)
+              self[name] = Marshal.load(value)
+            else
+              serializer.load(value)
+            end
+          end
+        end
+
         def serializer
           serializer = @options[:serializer] || :marshal
           case serializer
           when :marshal
             Marshal
-          when :json
+          when :json, :hybrid
             JsonSerializer
-          when :hybrid
-            HybridSerializer
           else
             serializer
           end
@@ -421,21 +440,21 @@ module ActionDispatch
         @parent_jar = parent_jar
         @options = options
         secret = key_generator.generate_key(@options[:signed_cookie_salt])
-        @verifier = ActiveSupport::MessageVerifier.new(secret, serializer: serializer)
+        @verifier = ActiveSupport::MessageVerifier.new(secret, serializer: NullSerializer)
       end
 
       def [](name)
         if signed_message = @parent_jar[name]
-          verify(signed_message)
+          deserialize name, verify(signed_message)
         end
       end
 
       def []=(name, options)
         if options.is_a?(Hash)
           options.symbolize_keys!
-          options[:value] = @verifier.generate(options[:value])
+          options[:value] = @verifier.generate(serialize(name, options[:value]))
         else
-          options = { :value => @verifier.generate(options) }
+          options = { :value => @verifier.generate(serialize(name, options)) }
         end
 
         raise CookieOverflow if options[:value].size > MAX_COOKIE_SIZE
@@ -459,7 +478,7 @@ module ActionDispatch
 
       def [](name)
         if signed_message = @parent_jar[name]
-          verify(signed_message) || verify_and_upgrade_legacy_signed_message(name, signed_message)
+          deserialize(name, verify(signed_message)) || verify_and_upgrade_legacy_signed_message(name, signed_message)
         end
       end
     end
@@ -478,12 +497,12 @@ module ActionDispatch
         @options = options
         secret = key_generator.generate_key(@options[:encrypted_cookie_salt])
         sign_secret = key_generator.generate_key(@options[:encrypted_signed_cookie_salt])
-        @encryptor = ActiveSupport::MessageEncryptor.new(secret, sign_secret, serializer: serializer)
+        @encryptor = ActiveSupport::MessageEncryptor.new(secret, sign_secret, serializer: NullSerializer)
       end
 
       def [](name)
         if encrypted_message = @parent_jar[name]
-          decrypt_and_verify(encrypted_message)
+          deserialize name, decrypt_and_verify(encrypted_message)
         end
       end
 
@@ -493,7 +512,8 @@ module ActionDispatch
         else
           options = { :value => options }
         end
-        options[:value] = @encryptor.encrypt_and_sign(options[:value])
+
+        options[:value] = @encryptor.encrypt_and_sign(serialize(name, options[:value]))
 
         raise CookieOverflow if options[:value].size > MAX_COOKIE_SIZE
         @parent_jar[name] = options
@@ -516,7 +536,7 @@ module ActionDispatch
 
       def [](name)
         if encrypted_or_signed_message = @parent_jar[name]
-          decrypt_and_verify(encrypted_or_signed_message) || verify_and_upgrade_legacy_signed_message(name, encrypted_or_signed_message)
+          deserialize(name, decrypt_and_verify(encrypted_or_signed_message)) || verify_and_upgrade_legacy_signed_message(name, encrypted_or_signed_message)
         end
       end
     end
diff --git a/actionpack/test/dispatch/cookies_test.rb b/actionpack/test/dispatch/cookies_test.rb
index 0c3d3d687c..ba7aaa338d 100644
--- a/actionpack/test/dispatch/cookies_test.rb
+++ b/actionpack/test/dispatch/cookies_test.rb
@@ -415,8 +415,8 @@ class CookiesTest < ActionController::TestCase
     assert_not_equal 45, cookies[:user_id]
     assert_equal 45, cookies.signed[:user_id]
 
-    json_value = ActiveSupport::MessageVerifier.new(secret, serializer: JSON).generate(45)
-    assert_equal @response.cookies['user_id'], json_value
+    verifier = ActiveSupport::MessageVerifier.new(secret, serializer: JSON)
+    assert_equal 45, verifier.verify(@response.cookies['user_id'])
   end
 
   def test_signed_cookie_using_hybrid_serializer_can_read_from_json_dumped_value
@@ -433,6 +433,8 @@ class CookiesTest < ActionController::TestCase
     cookies = @controller.send :cookies
     assert_not_equal 45, cookies[:user_id]
     assert_equal 45, cookies.signed[:user_id]
+
+    assert_nil @response.cookies["user_id"]
   end
 
   def test_accessing_nonexistant_signed_cookie_should_not_raise_an_invalid_signature
@@ -475,7 +477,7 @@ class CookiesTest < ActionController::TestCase
   def test_encrypted_cookie_using_custom_serializer
     @request.env["action_dispatch.cookies_serializer"] = CustomSerializer
     get :set_encrypted_cookie
-    assert_not_equal 45, cookies.encrypted[:foo]
+    assert_not_equal 'bar', cookies.encrypted[:foo]
     assert_equal 'bar was dumped and loaded', cookies.encrypted[:foo]
   end
 
@@ -488,17 +490,17 @@ class CookiesTest < ActionController::TestCase
     secret = key_generator.generate_key(encrypted_cookie_salt)
     sign_secret = key_generator.generate_key(encrypted_signed_cookie_salt)
 
-    marshal_value = ActiveSupport::MessageEncryptor.new(secret, sign_secret, serializer: Marshal).encrypt_and_sign(45)
-    @request.headers["Cookie"] = "user_id=#{marshal_value}"
+    marshal_value = ActiveSupport::MessageEncryptor.new(secret, sign_secret, serializer: Marshal).encrypt_and_sign("bar")
+    @request.headers["Cookie"] = "foo=#{marshal_value}"
 
     get :get_encrypted_cookie
 
     cookies = @controller.send :cookies
-    assert_not_equal 45, cookies[:user_id]
-    assert_equal 45, cookies.encrypted[:user_id]
+    assert_not_equal "bar", cookies[:foo]
+    assert_equal "bar", cookies.encrypted[:foo]
 
-    json_value = ActiveSupport::MessageEncryptor.new(secret, sign_secret, serializer: JSON).encrypt_and_sign(45)
-    assert_equal @response.cookies["user_id"], json_value
+    encryptor = ActiveSupport::MessageEncryptor.new(secret, sign_secret, serializer: JSON)
+    assert_equal "bar", encryptor.decrypt_and_verify(@response.cookies["foo"])
   end
 
   def test_encrypted_cookie_using_hybrid_serializer_can_read_from_json_dumped_value
@@ -509,14 +511,16 @@ class CookiesTest < ActionController::TestCase
     encrypted_signed_cookie_salt = @request.env["action_dispatch.encrypted_signed_cookie_salt"]
     secret = key_generator.generate_key(encrypted_cookie_salt)
     sign_secret = key_generator.generate_key(encrypted_signed_cookie_salt)
-    json_value = ActiveSupport::MessageEncryptor.new(secret, sign_secret, serializer: JSON).encrypt_and_sign(45)
-    @request.headers["Cookie"] = "user_id=#{json_value}"
+    json_value = ActiveSupport::MessageEncryptor.new(secret, sign_secret, serializer: JSON).encrypt_and_sign("bar")
+    @request.headers["Cookie"] = "foo=#{json_value}"
 
     get :get_encrypted_cookie
 
     cookies = @controller.send :cookies
-    assert_not_equal 45, cookies[:user_id]
-    assert_equal 45, cookies.encrypted[:user_id]
+    assert_not_equal "bar", cookies[:foo]
+    assert_equal "bar", cookies.encrypted[:foo]
+
+    assert_nil @response.cookies["foo"]
   end
 
   def test_accessing_nonexistant_encrypted_cookie_should_not_raise_invalid_message
@@ -834,8 +838,6 @@ class CookiesTest < ActionController::TestCase
     assert_equal "dhh", cookies['user_name']
   end
 
-
-
   def test_setting_request_cookies_is_indifferent_access
     cookies.clear
     cookies[:user_name] = "andrew"