Skip to content

Commit

Permalink
CookieStore should preserve the Set-Cookie header Array [#4743 state:…
Browse files Browse the repository at this point in the history 
…resolved]

Signed-off-by: Jeremy Kemper <jeremy@bitsweat.net>
  • Loading branch information
@jstorimer @jeremy
jstorimer authored and jeremy committed Jun 22, 2010
1 parent 5ed6a84 commit 85b6d79
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 7 deletions.
7 changes: 2 additions & 5 deletions actionpack/lib/action_controller/session/cookie_store.rb
View file
Expand Up @@ -114,11 +114,8 @@ def call(env)
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
headers[HTTP_SET_COOKIE] = [] if headers[HTTP_SET_COOKIE].blank?
headers[HTTP_SET_COOKIE] << cookie
end

[status, headers, body]
Expand Down
18 changes: 16 additions & 2 deletions actionpack/test/controller/session/cookie_store_test.rb
View file
Expand Up @@ -44,6 +44,12 @@ def raise_data_overflow
head :ok
end

def set_session_value_and_cookie
cookies["foo"] = "bar"
session[:foo] = "bar"
render :text => Rack::Utils.escape(Verifier.generate(session.to_hash))
end

def rescue_action(e) raise end
end

Expand Down Expand Up @@ -96,7 +102,7 @@ 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",
assert_equal ["_myapp_session=#{response.body}; path=/; HttpOnly"],
headers['Set-Cookie']
end
end
Expand Down Expand Up @@ -164,7 +170,7 @@ def test_setting_session_value_after_session_reset
get '/set_session_value'
assert_response :success
session_payload = response.body
assert_equal "_myapp_session=#{response.body}; path=/; HttpOnly",
assert_equal ["_myapp_session=#{response.body}; path=/; HttpOnly"],
headers['Set-Cookie']

get '/call_reset_session'
Expand Down Expand Up @@ -193,6 +199,14 @@ def test_persistent_session_id
end
end

def test_setting_session_value_and_cookie
with_test_route_set do
get '/set_session_value_and_cookie'
assert_response :success
assert_equal({"_myapp_session" => response.body, "foo" => "bar"}, cookies)
end
end

private
def with_test_route_set
with_routing do |set|
Expand Down

10 comments on commit 85b6d79

@josh
Copy link
Contributor

@josh josh commented on 85b6d79 Oct 26, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Header values should never be an Array. We should revert this.

@jstorimer
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, we should go the other direction and always force it to be a string. I'll write a patch.

@pixeltrix
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than just patch it up everywhere we should probably refactor the session code so that it goes through the CookieJar rather than writing out the session cookie directly itself. That way we're delegating the cookie handling to Rack, where it should be.

@josh
Copy link
Contributor

@josh josh commented on 85b6d79 Oct 27, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this upgrading an app to 2.3.9 and Rack::Lint went nuts. Most rack servers will still handle legacy Array headers fine so this isn't super critical.

@pixeltrix I extracted some helpers to Rack::Utils awhile back for this reason.

This seems super broken too. We're converting Set-Cookie to an Array on the way out. (I'm sure git blame says I wrote the code too)

Array headers are fine internally as long as we cast them to strings on the way out so other middleware doesn't have to deal with them.

@pixeltrix
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josh I saw those and now that 2-3-stable is requiring Rack 1.1 we can use them. I think there's at least three places where cookies are being built so it's ripe for refactoring. This commit needs reverting as well.

@jstorimer
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pixeltrix that commit followed the convention that I set up with this commit. I'll have a look at using set_cookie_header! where possible instead.

@pixeltrix
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jstorimer the session stores probably shouldn't be calling set_cookie_header! directly - we should let ActionController::CookieJar do the heavy lifting.

@josevalim
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pixeltrix, the cookie store in Rails master already delegates to the cookie jar. All other stores are coming from Rack (where we can't have any delegation).

The response is also broken in Rails 3. I remember there is a logic to convert the Set-Cookie from an Array to a String and this definitely needs to be fixed as well.

@boone
Copy link
Contributor

@boone boone commented on 85b6d79 Dec 2, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a ticket in Lighthouse where I can track this issue? Thanks.

@jstorimer
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@boone Looks like this was already taken care of in e0eb8e9

Please sign in to comment.