Switch to Rack::Response#set_cookie instead of using CGI::Cookie to build cookie headers

6 files changed, 66 insertions, 107 deletions

diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb
index eae17d6dd5..3e001a2ed6 100644
--- a/actionpack/lib/action_controller/base.rb
+++ b/actionpack/lib/action_controller/base.rb
@@ -254,7 +254,7 @@ module ActionController #:nodoc:
     cattr_reader :protected_instance_variables
     # Controller specific instance variables which will not be accessible inside views.
     @@protected_instance_variables = %w(@assigns @performed_redirect @performed_render @variables_added @request_origin @url @parent_controller
-                                        @action_name @before_filter_chain_aborted @action_cache_path @_session @_cookies @_headers @_params
+                                        @action_name @before_filter_chain_aborted @action_cache_path @_session @_headers @_params
                                         @_flash @_response)
 
     # Prepends all the URL-generating helpers from AssetHelper. This makes it possible to easily move javascripts, stylesheets,
@@ -1193,7 +1193,7 @@ module ActionController #:nodoc:
       end
 
       def assign_shortcuts(request, response)
-        @_request, @_params, @_cookies = request, request.parameters, request.cookies
+        @_request, @_params = request, request.parameters
 
         @_response         = response
         @_response.session = request.session
diff --git a/actionpack/lib/action_controller/cookies.rb b/actionpack/lib/action_controller/cookies.rb
index 0e058085ec..840ceb5abd 100644
--- a/actionpack/lib/action_controller/cookies.rb
+++ b/actionpack/lib/action_controller/cookies.rb
@@ -64,45 +64,31 @@ module ActionController #:nodoc:
 
     # Returns the value of the cookie by +name+, or +nil+ if no such cookie exists.
     def [](name)
-      cookie = @cookies[name.to_s]
-      if cookie && cookie.respond_to?(:value)
-        cookie.size > 1 ? cookie.value : cookie.value[0]
-      else
-        cookie
-      end
+      super(name.to_s)
     end
 
     # Sets the cookie named +name+. The second argument may be the very cookie
     # value, or a hash of options as documented above.
-    def []=(name, options)
+    def []=(key, options)
       if options.is_a?(Hash)
-        options = options.inject({}) { |options, pair| options[pair.first.to_s] = pair.last; options }
-        options["name"] = name.to_s
+        options.symbolize_keys!
       else
-        options = { "name" => name.to_s, "value" => options }
+        options = { :value => options }
       end
 
-      set_cookie(options)
+      options[:path] = "/" unless options.has_key?(:path)
+      super(key.to_s, options[:value])
+      @controller.response.set_cookie(key, options)
     end
 
     # Removes the cookie on the client machine by setting the value to an empty string
     # and setting its expiration date into the past. Like <tt>[]=</tt>, you can pass in
     # an options hash to delete cookies with extra data such as a <tt>:path</tt>.
-    def delete(name, options = {})
-      options.stringify_keys!
-      set_cookie(options.merge("name" => name.to_s, "value" => "", "expires" => Time.at(0)))
+    def delete(key, options = {})
+      options.symbolize_keys!
+      options[:path] = "/" unless options.has_key?(:path)
+      super(key.to_s)
+      @controller.response.delete_cookie(key, options)
     end
-
-    private
-      # Builds a CGI::Cookie object and adds the cookie to the response headers.
-      #
-      # The path of the cookie defaults to "/" if there's none in +options+, and
-      # everything is passed to the CGI::Cookie constructor.
-      def set_cookie(options) #:doc:
-        options["path"] = "/" unless options["path"]
-        cookie = CGI::Cookie.new(options)
-        @controller.logger.info "Cookie set: #{cookie}" unless @controller.logger.nil?
-        @controller.response.headers["cookie"] << cookie
-      end
   end
 end
diff --git a/actionpack/lib/action_controller/response.rb b/actionpack/lib/action_controller/response.rb
index 866616bac3..64319fe102 100644
--- a/actionpack/lib/action_controller/response.rb
+++ b/actionpack/lib/action_controller/response.rb
@@ -34,14 +34,14 @@ module ActionController # :nodoc:
     DEFAULT_HEADERS = { "Cache-Control" => "no-cache" }
     attr_accessor :request
 
-    attr_accessor :session, :cookies, :assigns, :template, :layout
+    attr_accessor :session, :assigns, :template, :layout
     attr_accessor :redirected_to, :redirected_to_method_params
 
     delegate :default_charset, :to => 'ActionController::Base'
 
     def initialize
       @status = 200
-      @header = DEFAULT_HEADERS.merge("cookie" => [])
+      @header = DEFAULT_HEADERS.dup
 
       @writer = lambda { |x| @body << x }
       @block = nil
@@ -143,10 +143,9 @@ module ActionController # :nodoc:
       handle_conditional_get!
       set_content_length!
       convert_content_type!
-
       convert_language!
       convert_expires!
-      set_cookies!
+      convert_cookies!
     end
 
     def each(&callback)
@@ -168,6 +167,35 @@ module ActionController # :nodoc:
       str
     end
 
+    # Over Rack::Response#set_cookie to add HttpOnly option
+    def set_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[:http_only]
+        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}"
+
+      case self["Set-Cookie"]
+      when Array
+        self["Set-Cookie"] << cookie
+      when String
+        self["Set-Cookie"] = [self["Set-Cookie"], cookie]
+      when nil
+        self["Set-Cookie"] = cookie
+      end
+    end
+
     private
       def handle_conditional_get!
         if etag? || last_modified?
@@ -217,22 +245,8 @@ module ActionController # :nodoc:
         headers["Expires"] = headers.delete("") if headers["expires"]
       end
 
-      def set_cookies!
-        # Convert 'cookie' header to 'Set-Cookie' headers.
-        # Because Set-Cookie header can appear more the once in the response body,
-        # we store it in a line break separated string that will be translated to
-        # multiple Set-Cookie header by the handler.
-        if cookie = headers.delete('cookie')
-          cookies = []
-
-          case cookie
-            when Array then cookie.each { |c| cookies << c.to_s }
-            when Hash  then cookie.each { |_, c| cookies << c.to_s }
-            else            cookies << cookie.to_s
-          end
-
-          headers['Set-Cookie'] = [headers['Set-Cookie'], cookies].flatten.compact
-        end
+      def convert_cookies!
+        headers['Set-Cookie'] = Array(headers['Set-Cookie']).compact
       end
   end
 end
diff --git a/actionpack/lib/action_controller/test_case.rb b/actionpack/lib/action_controller/test_case.rb
index 79a8e1364d..7ed1a3e160 100644
--- a/actionpack/lib/action_controller/test_case.rb
+++ b/actionpack/lib/action_controller/test_case.rb
@@ -93,10 +93,7 @@ module ActionController
   # and cookies, though. For sessions, you just do:
   #
   #   @request.session[:key] = "value"
-  #
-  # For cookies, you need to manually create the cookie, like this:
-  #
-  #   @request.cookies["key"] = CGI::Cookie.new("key", "value")
+  #   @request.cookies["key"] = "value"
   #
   # == Testing named routes
   #
diff --git a/actionpack/lib/action_controller/test_process.rb b/actionpack/lib/action_controller/test_process.rb
index c4d7d52951..45dcf8b2c2 100644
--- a/actionpack/lib/action_controller/test_process.rb
+++ b/actionpack/lib/action_controller/test_process.rb
@@ -260,14 +260,14 @@ module ActionController #:nodoc:
       !template_objects[name].nil?
     end
 
-    # Returns the response cookies, converted to a Hash of (name => CGI::Cookie) pairs
+    # Returns the response cookies, converted to a Hash of (name => value) pairs
     #
-    #   assert_equal ['AuthorOfNewPage'], r.cookies['author'].value
+    #   assert_equal 'AuthorOfNewPage', r.cookies['author']
     def cookies
       cookies = {}
       Array(headers['Set-Cookie']).each do |cookie|
         key, value = cookie.split(";").first.split("=")
-        cookies[key] = [value].compact
+        cookies[key] = value
       end
       cookies
     end
diff --git a/actionpack/test/controller/cookie_test.rb b/actionpack/test/controller/cookie_test.rb
index 4b969519c6..84d272b1c7 100644
--- a/actionpack/test/controller/cookie_test.rb
+++ b/actionpack/test/controller/cookie_test.rb
@@ -52,33 +52,33 @@ class CookieTest < Test::Unit::TestCase
   def test_setting_cookie
     get :authenticate
     assert_equal ["user_name=david; path=/"], @response.headers["Set-Cookie"]
-    assert_equal({"user_name" => ["david"]}, @response.cookies)
+    assert_equal({"user_name" => "david"}, @response.cookies)
   end
 
   def test_setting_cookie_for_fourteen_days
     get :authenticate_for_fourteen_days
-    assert_equal ["user_name=david; path=/; expires=Mon, 10 Oct 2005 05:00:00 GMT"], @response.headers["Set-Cookie"]
-    assert_equal({"user_name" => ["david"]}, @response.cookies)
+    assert_equal ["user_name=david; path=/; expires=Mon, 10-Oct-2005 05:00:00 GMT"], @response.headers["Set-Cookie"]
+    assert_equal({"user_name" => "david"}, @response.cookies)
   end
 
   def test_setting_cookie_for_fourteen_days_with_symbols
     get :authenticate_for_fourteen_days_with_symbols
-    assert_equal ["user_name=david; path=/; expires=Mon, 10 Oct 2005 05:00:00 GMT"], @response.headers["Set-Cookie"]
-    assert_equal({"user_name" => ["david"]}, @response.cookies)
+    assert_equal ["user_name=david; path=/; expires=Mon, 10-Oct-2005 05:00:00 GMT"], @response.headers["Set-Cookie"]
+    assert_equal({"user_name" => "david"}, @response.cookies)
   end
 
   def test_setting_cookie_with_http_only
     get :authenticate_with_http_only
     assert_equal ["user_name=david; path=/; HttpOnly"], @response.headers["Set-Cookie"]
-    assert_equal({"user_name" => ["david"]}, @response.cookies)
+    assert_equal({"user_name" => "david"}, @response.cookies)
   end
 
   def test_multiple_cookies
     get :set_multiple_cookies
     assert_equal 2, @response.cookies.size
-    assert_equal "user_name=david; path=/; expires=Mon, 10 Oct 2005 05:00:00 GMT", @response.headers["Set-Cookie"][0]
+    assert_equal "user_name=david; path=/; expires=Mon, 10-Oct-2005 05:00:00 GMT", @response.headers["Set-Cookie"][0]
     assert_equal "login=XJ-122; path=/", @response.headers["Set-Cookie"][1]
-    assert_equal({"login" => ["XJ-122"], "user_name" => ["david"]}, @response.cookies)
+    assert_equal({"login" => "XJ-122", "user_name" => "david"}, @response.cookies)
   end
 
   def test_setting_test_cookie
@@ -87,12 +87,12 @@ class CookieTest < Test::Unit::TestCase
 
   def test_expiring_cookie
     get :logout
-    assert_equal ["user_name=; path=/; expires=Thu, 01 Jan 1970 00:00:00 GMT"], @response.headers["Set-Cookie"]
-    assert_equal({"user_name" => []}, @response.cookies)
+    assert_equal ["user_name=; path=/; expires=Thu, 01-Jan-1970 00:00:00 GMT"], @response.headers["Set-Cookie"]
+    assert_equal({"user_name" => nil}, @response.cookies)
   end
 
   def test_cookiejar_accessor
-    @request.cookies["user_name"] = CGI::Cookie.new("name" => "user_name", "value" => "david", "expires" => Time.local(2025, 10, 10))
+    @request.cookies["user_name"] = "david"
     @controller.request = @request
     jar = ActionController::CookieJar.new(@controller)
     assert_equal "david", jar["user_name"]
@@ -100,52 +100,14 @@ class CookieTest < Test::Unit::TestCase
   end
 
   def test_cookiejar_accessor_with_array_value
-    a = %w{1 2 3}
-    @request.cookies["pages"] = CGI::Cookie.new("name" => "pages", "value" => a, "expires" => Time.local(2025, 10, 10))
+    @request.cookies["pages"] = %w{1 2 3}
     @controller.request = @request
     jar = ActionController::CookieJar.new(@controller)
-    assert_equal a, jar["pages"]
+    assert_equal %w{1 2 3}, jar["pages"]
   end
 
   def test_delete_cookie_with_path
     get :delete_cookie_with_path
-    assert_equal ["user_name=; path=/beaten; expires=Thu, 01 Jan 1970 00:00:00 GMT"], @response.headers["Set-Cookie"]
-  end
-
-  def test_cookie_to_s_simple_values
-    assert_equal 'myname=myvalue; path=', CGI::Cookie.new('myname', 'myvalue').to_s
-  end
-
-  def test_cookie_to_s_hash
-    cookie_str = CGI::Cookie.new(
-      'name' => 'myname',
-      'value' => 'myvalue',
-      'domain' => 'mydomain',
-      'path' => 'mypath',
-      'expires' => Time.utc(2007, 10, 20),
-      'secure' => true,
-      'http_only' => true).to_s
-    assert_equal 'myname=myvalue; domain=mydomain; path=mypath; expires=Sat, 20 Oct 2007 00:00:00 GMT; secure; HttpOnly', cookie_str
-  end
-
-  def test_cookie_to_s_hash_default_not_secure_not_http_only
-    cookie_str = CGI::Cookie.new(
-      'name' => 'myname',
-      'value' => 'myvalue',
-      'domain' => 'mydomain',
-      'path' => 'mypath',
-      'expires' => Time.utc(2007, 10, 20))
-    assert cookie_str !~ /secure/
-    assert cookie_str !~ /HttpOnly/
-  end
-
-  def test_cookies_should_not_be_split_on_ampersand_values
-    cookies = CGI::Cookie.parse('return_to=http://rubyonrails.org/search?term=api&scope=all&global=true')
-    assert_equal({"return_to" => ["http://rubyonrails.org/search?term=api&scope=all&global=true"]}, cookies)
-  end
-
-  def test_cookies_should_not_be_split_on_values_with_newlines
-    cookies = CGI::Cookie.new("name" => "val", "value" => "this\nis\na\ntest")
-    assert cookies.size == 1
+    assert_equal ["user_name=; path=/beaten; expires=Thu, 01-Jan-1970 00:00:00 GMT"], @response.headers["Set-Cookie"]
   end
 end