Bug with set_cookie_header!, Session::Cookie and Array header values

Bug with set_cookie_header!, Session::Cookie and Array header values

[Matthew Willson]

(with rack 1.1.0)

Rack::Session::Cookie is doing this:

status, headers, body = @app.call(env)
...
Utils.set_cookie_header!(headers, @key, cookie.merge(options))
...
[status, headers, body]


Problem is that Utils.set_cookie_header! expects to be used with a HeaderHash object.
If used with a plain hash object, it can result in an Array value if it's called more than once on the same headers object, eg

{"Set-Cookie"=>["x=y", "y=z"]}

This then flags a Rack::Lint error:

!! Unexpected error while processing request: a header value must be a String, but the value of 'Set-Cookie' is a Array.

One way to trigger it is to set another (non-session) cookie inside a request wrapped in Session::Cookie middleware.

My temporary fix was to add these lines to Rack::Session::Cookie#commit_session

          Utils.set_cookie_header!(headers, @key, cookie.merge(options))
+          value = headers['Set-Cookie']
+          headers['Set-Cookie'] = value.join("\n") if value.is_a?(Array)

But might it be worth changing set_cookie_header! to work correctly with a plain Hash if one is passed to it?
That would stop this kind of issue cropping up again in middleware.

Cheers
-Matt

Re: Bug with set_cookie_header!, Session::Cookie and Array header values

[Ryan Tomayko]

On Tue, Mar 9, 2010 at 3:33 AM, Matthew Willson
<matthew.willson@gmail.com> wrote:
> (with rack 1.1.0)
>
> Rack::Session::Cookie is doing this:
>
> status, headers, body = @app.call(env)
> ...
> Utils.set_cookie_header!(headers, @key, cookie.merge(options))
> ...
> [status, headers, body]
>
>
> Problem is that Utils.set_cookie_header! expects to be used with a HeaderHash object.
> If used with a plain hash object, it can result in an Array value if it's called more than once on the same headers object, eg
>
> {"Set-Cookie"=>["x=y", "y=z"]}
>
> This then flags a Rack::Lint error:
>
> !! Unexpected error while processing request: a header value must be a String, but the value of 'Set-Cookie' is a Array.
>
> One way to trigger it is to set another (non-session) cookie inside a request wrapped in Session::Cookie middleware.
>
> My temporary fix was to add these lines to Rack::Session::Cookie#commit_session
>
>          Utils.set_cookie_header!(headers, @key, cookie.merge(options))
> +          value = headers['Set-Cookie']
> +          headers['Set-Cookie'] = value.join("\n") if value.is_a?(Array)
>
> But might it be worth changing set_cookie_header! to work correctly with a plain Hash if one is passed to it?
> That would stop this kind of issue cropping up again in middleware.
>
> Cheers
> -Matt

Interesting. It looks like the cookie utility methods were never
changed when we moved from Arrays to lines for representing multiple
header values. As you mentioned, HeaderHash does this for us but I
think it's best that the utility methods follow the spec where
possible. I've modified them to use strings instead of arrays. Patch
applied to master follows.

From c028a23b36debbce1005347d4234fe6e32373223 Mon Sep 17 00:00:00 2001
From: Ryan Tomayko <rtomayko@gmail.com>
Date: Tue, 9 Mar 2010 06:54:15 -0800
Subject: [PATCH] cookie utility methods use multiline strings instead of arrays

---
 lib/rack/utils.rb          |   23 +++++++++++++++--------
 test/spec_rack_response.rb |   10 ++++++----
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/lib/rack/utils.rb b/lib/rack/utils.rb
index e1bc97c..50bee6e 100644
--- a/lib/rack/utils.rb
+++ b/lib/rack/utils.rb
@@ -193,12 +193,12 @@ module Rack
         "#{domain}#{path}#{expires}#{secure}#{httponly}"

       case header["Set-Cookie"]
-      when Array
-        header["Set-Cookie"] << cookie
-      when String
-        header["Set-Cookie"] = [header["Set-Cookie"], cookie]
-      when nil
+      when nil, ''
         header["Set-Cookie"] = cookie
+      when String
+        header["Set-Cookie"] = [header["Set-Cookie"], cookie].join("\n")
+      when Array
+        header["Set-Cookie"] = (header["Set-Cookie"] + [cookie]).join("\n")
       end

       nil
@@ -206,14 +206,21 @@ module Rack
     module_function :set_cookie_header!

     def delete_cookie_header!(header, key, value = {})
-      unless Array === header["Set-Cookie"]
-        header["Set-Cookie"] = [header["Set-Cookie"]].compact
+      case header["Set-Cookie"]
+      when nil, ''
+        cookies = []
+      when String
+        cookies = header["Set-Cookie"].split("\n")
+      when Array
+        cookies = header["Set-Cookie"]
       end

-      header["Set-Cookie"].reject! { |cookie|
+      cookies.reject! { |cookie|
         cookie =~ /\A#{escape(key)}=/
       }

+      header["Set-Cookie"] = cookies.join("\n")
+
       set_cookie_header!(header, key,
                  {:value => '', :path => nil, :domain => nil,
                    :expires => Time.at(0) }.merge(value))
diff --git a/test/spec_rack_response.rb b/test/spec_rack_response.rb
index 7989013..98f8289 100644
--- a/test/spec_rack_response.rb
+++ b/test/spec_rack_response.rb
@@ -50,9 +50,9 @@ context "Rack::Response" do
     response.set_cookie "foo", "bar"
     response["Set-Cookie"].should.equal "foo=bar"
     response.set_cookie "foo2", "bar2"
-    response["Set-Cookie"].should.equal ["foo=bar", "foo2=bar2"]
+    response["Set-Cookie"].should.equal ["foo=bar", "foo2=bar2"].join("\n")
     response.set_cookie "foo3", "bar3"
-    response["Set-Cookie"].should.equal ["foo=bar", "foo2=bar2", "foo3=bar3"]
+    response["Set-Cookie"].should.equal ["foo=bar", "foo2=bar2",
"foo3=bar3"].join("\n")
   end

   specify "formats the Cookie expiration date accordingly to RFC 2109" do
@@ -80,8 +80,10 @@ context "Rack::Response" do
     response.set_cookie "foo", "bar"
     response.set_cookie "foo2", "bar2"
     response.delete_cookie "foo"
-    response["Set-Cookie"].should.equal ["foo2=bar2",
-                                  "foo=; expires=Thu, 01-Jan-1970
00:00:00 GMT"]
+    response["Set-Cookie"].should.equal [
+      "foo2=bar2",
+      "foo=; expires=Thu, 01-Jan-1970 00:00:00 GMT"
+    ].join("\n")
   end

   specify "can do redirects" do
-- 
1.7.0.1

Thanks,
Ryan