Simplify cookie_store by simply relying on cookies.signed.

6 files changed, 130 insertions, 142 deletions

diff --git a/actionpack/lib/action_dispatch/middleware/cookies.rb b/actionpack/lib/action_dispatch/middleware/cookies.rb
index 42ab1d1ebb..1e49a307ed 100644
--- a/actionpack/lib/action_dispatch/middleware/cookies.rb
+++ b/actionpack/lib/action_dispatch/middleware/cookies.rb
@@ -52,6 +52,9 @@ module ActionDispatch
   # * <tt>:httponly</tt> - Whether this cookie is accessible via scripting or
   #   only HTTP. Defaults to +false+.
   class Cookies
+    # Raised when storing more than 4K of session data.
+    class CookieOverflow < StandardError; end
+
     class CookieJar < Hash #:nodoc:
       def self.build(request)
         secret = request.env["action_dispatch.secret_token"]
@@ -166,8 +169,11 @@ module ActionDispatch
     end
 
     class SignedCookieJar < CookieJar #:nodoc:
+      MAX_COOKIE_SIZE = 4096 # Cookies can typically store 4096 bytes.
+      SECRET_MIN_LENGTH = 30 # Characters
+
       def initialize(parent_jar, secret)
-        raise "You must set config.secret_token in your app's config" if secret.blank?
+        ensure_secret_secure(secret)
         @parent_jar = parent_jar
         @verifier   = ActiveSupport::MessageVerifier.new(secret)
       end
@@ -176,6 +182,8 @@ module ActionDispatch
         if signed_message = @parent_jar[name]
           @verifier.verify(signed_message)
         end
+      rescue ActiveSupport::MessageVerifier::InvalidSignature
+        nil
       end
 
       def []=(key, options)
@@ -186,12 +194,34 @@ module ActionDispatch
           options = { :value => @verifier.generate(options) }
         end
 
+        raise CookieOverflow if options[:value].size > MAX_COOKIE_SIZE
         @parent_jar[key] = options
       end
 
       def method_missing(method, *arguments, &block)
         @parent_jar.send(method, *arguments, &block)
       end
+
+    protected
+
+      # To prevent users from using something insecure like "Password" we make sure that the
+      # secret they've provided is at least 30 characters in length.
+      def ensure_secret_secure(secret)
+        if secret.blank?
+          raise ArgumentError, "A secret is required to generate an " +
+            "integrity hash for cookie session data. Use " +
+            "config.secret_token = \"some secret phrase of at " +
+            "least #{SECRET_MIN_LENGTH} characters\"" +
+            "in config/application.rb"
+        end
+
+        if secret.length < SECRET_MIN_LENGTH
+          raise ArgumentError, "Secret should be something secure, " +
+            "like \"#{ActiveSupport::SecureRandom.hex(16)}\".  The value you " +
+            "provided, \"#{secret}\", is shorter than the minimum length " +
+            "of #{SECRET_MIN_LENGTH} characters"
+        end
+      end
     end
 
     def initialize(app)
diff --git a/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb b/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb
index 7114f42003..0c1712bf84 100644
--- a/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb
+++ b/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb
@@ -1,3 +1,4 @@
+require 'action_dispatch/middleware/cookies'
 require 'active_support/core_ext/hash/keys'
 require 'active_support/core_ext/object/blank'
 
@@ -39,10 +40,6 @@ module ActionDispatch
     #
     # Note that changing digest or secret invalidates all existing sessions!
     class CookieStore
-      # Cookies can typically store 4096 bytes.
-      MAX = 4096
-      SECRET_MIN_LENGTH = 30 # characters
-
       DEFAULT_OPTIONS = {
         :key          => '_session_id',
         :domain       => nil,
@@ -54,7 +51,7 @@ module ActionDispatch
       class OptionsHash < Hash
         def initialize(by, env, default_options)
           @session_data = env[CookieStore::ENV_SESSION_KEY]
-          default_options.each { |key, value| self[key] = value }
+          merge!(default_options)
         end
 
         def [](key)
@@ -64,14 +61,12 @@ module ActionDispatch
 
       ENV_SESSION_KEY = "rack.session".freeze
       ENV_SESSION_OPTIONS_KEY = "rack.session.options".freeze
-      HTTP_SET_COOKIE = "Set-Cookie".freeze
-
-      # Raised when storing more than 4K of session data.
-      class CookieOverflow < StandardError; end
 
       def initialize(app, options = {})
         # Process legacy CGI options
+        # TODO Refactor and deprecate me
         options = options.symbolize_keys
+
         if options.has_key?(:session_path)
           options[:path] = options.delete(:session_path)
         end
@@ -88,15 +83,7 @@ module ActionDispatch
         ensure_session_key(options[:key])
         @key = options.delete(:key).freeze
 
-        # The secret option is required.
-        ensure_secret_secure(options[:secret])
-        @secret = options.delete(:secret).freeze
-
-        @digest = options.delete(:digest) || 'SHA1'
-        @verifier = verifier_for(@secret, @digest)
-
         @default_options = DEFAULT_OPTIONS.merge(options).freeze
-
         freeze
       end
 
@@ -111,66 +98,32 @@ module ActionDispatch
 
         if !session_data.is_a?(AbstractStore::SessionHash) || session_data.send(:loaded?) || options[:expire_after]
           session_data.send(:load!) if session_data.is_a?(AbstractStore::SessionHash) && !session_data.send(:loaded?)
-          session_data = marshal(session_data.to_hash)
-
-          raise CookieOverflow if session_data.size > MAX
+          session_data = persistent_session_id!(session_data.to_hash)
 
-          cookie = Hash.new
-          cookie[:value] = session_data
+          cookie = { :value => session_data }
           unless options[:expire_after].nil?
-            cookie[:expires] = Time.now + options[:expire_after]
+            cookie[:expires] = Time.now + options.delete(:expire_after)
           end
 
-          cookie = build_cookie(@key, cookie.merge(options))
-          unless headers[HTTP_SET_COOKIE].blank?
-            headers[HTTP_SET_COOKIE] << "\n#{cookie}"
-          else
-            headers[HTTP_SET_COOKIE] = cookie
-          end
+          request = ActionDispatch::Request.new(env)
+          request.cookie_jar.signed[@key] = cookie.merge!(options)
         end
 
         [status, headers, body]
       end
 
       private
-        # Should be in Rack::Utils soon
-        def build_cookie(key, value)
-          case value
-          when Hash
-            domain  = "; domain="  + value[:domain] if value[:domain]
-            path    = "; path="    + value[:path]   if value[:path]
-            # According to RFC 2109, we need dashes here.
-            # N.B.: cgi.rb uses spaces...
-            expires = "; expires=" + value[:expires].clone.gmtime.
-              strftime("%a, %d-%b-%Y %H:%M:%S GMT") if value[:expires]
-            secure = "; secure" if value[:secure]
-            httponly = "; HttpOnly" if value[:httponly]
-            value = value[:value]
-          end
-          value = [value] unless Array === value
-          cookie = Rack::Utils.escape(key) + "=" +
-            value.map { |v| Rack::Utils.escape(v) }.join("&") +
-            "#{domain}#{path}#{expires}#{secure}#{httponly}"
-        end
 
         def load_session(env)
-          request = Rack::Request.new(env)
-          session_data = request.cookies[@key]
-          data = unmarshal(session_data) || persistent_session_id!({})
+          request = ActionDispatch::Request.new(env)
+          data = request.cookie_jar.signed[@key]
+          data = persistent_session_id!(data || {})
           data.stringify_keys!
           [data["session_id"], data]
         end
 
-        # Marshal a session hash into safe cookie data. Include an integrity hash.
-        def marshal(session)
-          @verifier.generate(persistent_session_id!(session))
-        end
-
-        # Unmarshal cookie data to a hash and verify its integrity.
-        def unmarshal(cookie)
-          persistent_session_id!(@verifier.verify(cookie)) if cookie
-        rescue ActiveSupport::MessageVerifier::InvalidSignature
-          nil
+        def generate_sid
+          ActiveSupport::SecureRandom.hex(16)
         end
 
         def ensure_session_key(key)
@@ -182,38 +135,6 @@ module ActionDispatch
           end
         end
 
-        # To prevent users from using something insecure like "Password" we make sure that the
-        # secret they've provided is at least 30 characters in length.
-        def ensure_secret_secure(secret)
-          # There's no way we can do this check if they've provided a proc for the
-          # secret.
-          return true if secret.is_a?(Proc)
-
-          if secret.blank?
-            raise ArgumentError, "A secret is required to generate an " +
-              "integrity hash for cookie session data. Use " +
-              "config.secret_token = \"some secret phrase of at " +
-              "least #{SECRET_MIN_LENGTH} characters\"" +
-              "in config/application.rb"
-          end
-
-          if secret.length < SECRET_MIN_LENGTH
-            raise ArgumentError, "Secret should be something secure, " +
-              "like \"#{ActiveSupport::SecureRandom.hex(16)}\".  The value you " +
-              "provided, \"#{secret}\", is shorter than the minimum length " +
-              "of #{SECRET_MIN_LENGTH} characters"
-          end
-        end
-
-        def verifier_for(secret, digest)
-          key = secret.respond_to?(:call) ? secret.call : secret
-          ActiveSupport::MessageVerifier.new(key, digest)
-        end
-
-        def generate_sid
-          ActiveSupport::SecureRandom.hex(16)
-        end
-
         def persistent_session_id!(data)
           (data ||= {}).merge!(inject_persistent_session_id(data))
         end
diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb
index 479e62b23d..d2e5d2e965 100644
--- a/actionpack/test/abstract_unit.rb
+++ b/actionpack/test/abstract_unit.rb
@@ -162,6 +162,7 @@ class ActionController::IntegrationTest < ActiveSupport::TestCase
       middleware.use "ActionDispatch::Cookies"
       middleware.use "ActionDispatch::Flash"
       middleware.use "ActionDispatch::Head"
+      yield(middleware) if block_given?
     end
   end
 
diff --git a/actionpack/test/controller/cookie_test.rb b/actionpack/test/controller/cookie_test.rb
index 4971866e7c..f65eda5c69 100644
--- a/actionpack/test/controller/cookie_test.rb
+++ b/actionpack/test/controller/cookie_test.rb
@@ -58,6 +58,17 @@ class CookieTest < ActionController::TestCase
       head :ok
     end
 
+    def raise_data_overflow
+      cookies.signed[:foo] = 'bye!' * 1024
+      head :ok
+    end
+
+    def tampered_cookies
+      cookies[:tampered] = "BAh7BjoIZm9vIghiYXI%3D--123456780"
+      cookies.signed[:tampered]
+      head :ok
+    end
+
     def set_permanent_signed_cookie
       cookies.permanent.signed[:remember_me] = 100
       head :ok
@@ -74,7 +85,7 @@ class CookieTest < ActionController::TestCase
 
   def setup
     super
-    @request.env["action_dispatch.secret_token"] = "thisISverySECRET123"
+    @request.env["action_dispatch.secret_token"] = "b3c631c314c0bbca50c1b2843150fe33"
     @request.host = "www.nextangle.com"
   end
 
@@ -163,6 +174,48 @@ class CookieTest < ActionController::TestCase
     assert_equal({"user_name" => "david"}, @response.cookies)
   end
 
+  def test_raise_data_overflow
+    assert_raise(ActionDispatch::Cookies::CookieOverflow) do
+      get :raise_data_overflow
+    end
+  end
+
+  def test_tampered_cookies
+    assert_nothing_raised do
+      get :tampered_cookies
+      assert_response :success
+    end
+  end
+
+  def test_raises_argument_error_if_missing_secret
+    assert_raise(ArgumentError, nil.inspect) {
+      @request.env["action_dispatch.secret_token"] = nil
+      get :set_signed_cookie
+    }
+
+    assert_raise(ArgumentError, ''.inspect) {
+      @request.env["action_dispatch.secret_token"] = ""
+      get :set_signed_cookie
+    }
+  end
+
+  def test_raises_argument_error_if_secret_is_probably_insecure
+    assert_raise(ArgumentError, "password".inspect) {
+      @request.env["action_dispatch.secret_token"] = "password"
+      get :set_signed_cookie
+    }
+
+    assert_raise(ArgumentError, "secret".inspect) {
+      @request.env["action_dispatch.secret_token"] = "secret"
+      get :set_signed_cookie
+    }
+
+    assert_raise(ArgumentError, "12345678901234567890123456789".inspect) {
+      @request.env["action_dispatch.secret_token"] = "12345678901234567890123456789"
+      get :set_signed_cookie
+    }
+  end
+
   private
     def assert_cookie_header(expected)
       header = @response.headers["Set-Cookie"]
diff --git a/actionpack/test/controller/flash_test.rb b/actionpack/test/controller/flash_test.rb
index c662ce264b..01c8fd90a5 100644
--- a/actionpack/test/controller/flash_test.rb
+++ b/actionpack/test/controller/flash_test.rb
@@ -237,10 +237,19 @@ class FlashIntegrationTest < ActionController::IntegrationTest
   end
 
   private
+
+    # Overwrite get to send SessionSecret in env hash
+    def get(path, parameters = nil, env = {})
+      env["action_dispatch.secret_token"] ||= SessionSecret
+      super
+    end
+
     def with_test_route_set
       with_routing do |set|
         set.draw do |map|
-          match ':action', :to => ActionDispatch::Session::CookieStore.new(FlashIntegrationTest::TestController, :key => FlashIntegrationTest::SessionKey, :secret => FlashIntegrationTest::SessionSecret)
+          match ':action', :to => ActionDispatch::Session::CookieStore.new(
+            FlashIntegrationTest::TestController, :key => SessionKey, :secret => SessionSecret
+          )
         end
         yield
       end
diff --git a/actionpack/test/dispatch/session/cookie_store_test.rb b/actionpack/test/dispatch/session/cookie_store_test.rb
index d2c1758af1..a6cdbf8032 100644
--- a/actionpack/test/dispatch/session/cookie_store_test.rb
+++ b/actionpack/test/dispatch/session/cookie_store_test.rb
@@ -55,42 +55,13 @@ class CookieStoreTest < ActionController::IntegrationTest
     }
   end
 
-  def test_raises_argument_error_if_missing_secret
-    assert_raise(ArgumentError, nil.inspect) {
-      ActionDispatch::Session::CookieStore.new(nil,
-       :key => SessionKey, :secret => nil)
-    }
-
-    assert_raise(ArgumentError, ''.inspect) {
-      ActionDispatch::Session::CookieStore.new(nil,
-       :key => SessionKey, :secret => '')
-    }
-  end
-
-  def test_raises_argument_error_if_secret_is_probably_insecure
-    assert_raise(ArgumentError, "password".inspect) {
-      ActionDispatch::Session::CookieStore.new(nil,
-       :key => SessionKey, :secret => "password")
-    }
-
-    assert_raise(ArgumentError, "secret".inspect) {
-      ActionDispatch::Session::CookieStore.new(nil,
-       :key => SessionKey, :secret => "secret")
-    }
-
-    assert_raise(ArgumentError, "12345678901234567890123456789".inspect) {
-      ActionDispatch::Session::CookieStore.new(nil,
-       :key => SessionKey, :secret => "12345678901234567890123456789")
-    }
-  end
-
   def test_setting_session_value
     with_test_route_set do
       get '/set_session_value'
       assert_response :success
       assert_equal "_myapp_session=#{response.body}; path=/; HttpOnly",
         headers['Set-Cookie']
-   end
+    end
   end
 
   def test_getting_session_value
@@ -99,7 +70,7 @@ class CookieStoreTest < ActionController::IntegrationTest
       get '/get_session_value'
       assert_response :success
       assert_equal 'foo: "bar"', response.body
-   end
+    end
   end
 
   def test_getting_session_id
@@ -127,7 +98,7 @@ class CookieStoreTest < ActionController::IntegrationTest
 
   def test_close_raises_when_data_overflows
     with_test_route_set do
-      assert_raise(ActionDispatch::Session::CookieStore::CookieOverflow) {
+      assert_raise(ActionDispatch::Cookies::CookieOverflow) {
         get '/raise_data_overflow'
       }
     end
@@ -209,30 +180,33 @@ class CookieStoreTest < ActionController::IntegrationTest
       get '/no_session_access'
       assert_response :success
 
-      # Mystery bug that came up in 2.3 as well. What is this trying to test?!
-      # assert_equal "_myapp_session=#{cookie_body}; path=/; expires=#{expected_expiry}; HttpOnly",
-      #   headers['Set-Cookie']
+      assert_equal "_myapp_session=#{cookie_body}; path=/; expires=#{expected_expiry}; HttpOnly",
+        headers['Set-Cookie']
     end
   end
 
   private
+
+    # Overwrite get to send SessionSecret in env hash
+    def get(path, parameters = nil, env = {})
+      env["action_dispatch.secret_token"] ||= SessionSecret
+      super
+    end
+
     def with_test_route_set(options = {})
       with_routing do |set|
         set.draw do |map|
           match ':action', :to => ::CookieStoreTest::TestController
         end
-        options = {:key => SessionKey, :secret => SessionSecret}.merge(options)
-        @app = ActionDispatch::Session::CookieStore.new(set, options)
+
+        options = { :key => SessionKey, :secret => SessionSecret }.merge!(options)
+
+        @app = self.class.build_app(set) do |middleware|
+          middleware.use ActionDispatch::Session::CookieStore, options
+          middleware.delete "ActionDispatch::ShowExceptions"
+        end
+
         yield
       end
     end
-
-    def unmarshal_session(cookie_string)
-      session = Rack::Utils.parse_query(cookie_string, ';,').inject({}) {|h,(k,v)|
-        h[k] = Array === v ? v.first : v
-        h
-      }[SessionKey]
-      verifier = ActiveSupport::MessageVerifier.new(SessionSecret, 'SHA1')
-      verifier.verify(session)
-    end
 end