Simplify cookie_store by simply relying on cookies.signed.

actionpack/lib/action_dispatch/middleware/cookies.rb

@@ -52,6 +52,9 @@ def cookie_jar
   # * <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 @@ def method_missing(method, *arguments, &block)
     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 @@ def [](name)
         if signed_message = @parent_jar[name]
           @verifier.verify(signed_message)
         end
+      rescue ActiveSupport::MessageVerifier::InvalidSignature
+        nil
       end
 
       def []=(key, options)
@@ -186,12 +194,34 @@ def []=(key, options)
           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)

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 Session
     #
     # 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 @@ class CookieStore
       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 @@ def [](key)
 
       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 @@ def initialize(app, options = {})
         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 @@ def call(env)
 
         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 @@ def ensure_session_key(key)
           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

actionpack/test/abstract_unit.rb

@@ -162,6 +162,7 @@ def self.build_app(routes = nil)
       middleware.use "ActionDispatch::Cookies"
       middleware.use "ActionDispatch::Flash"
       middleware.use "ActionDispatch::Head"
+      yield(middleware) if block_given?
     end
   end
 

actionpack/test/controller/cookie_test.rb

@@ -58,6 +58,17 @@ def set_signed_cookie
       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 @@ def delete_and_set_cookie
 
   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 @@ def test_delete_and_set_cookie
     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"]

actionpack/test/controller/flash_test.rb

@@ -237,10 +237,19 @@ def test_flash
   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