Browse files

Let Rack::Utils.set_cookie_header! create the Set-Cookie header inste…

…ad of manually fiddling with the response headers [#4941 state:resolved]

Signed-off-by: José Valim <jose.valim@gmail.com>
  • Loading branch information...
1 parent 2826324 commit e0eb8e9c65ededce64169948d4dd51b0079cdd10 @paukul paukul committed with josevalim Aug 25, 2010
View
18 actionpack/lib/action_controller/session/abstract_store.rb
@@ -195,22 +195,8 @@ def call(env)
request_cookies = env["rack.request.cookie_hash"]
if (request_cookies.nil? || request_cookies[@key] != sid) || options[:expire_after]
- cookie = Rack::Utils.escape(@key) + '=' + Rack::Utils.escape(sid)
- cookie << "; domain=#{options[:domain]}" if options[:domain]
- cookie << "; path=#{options[:path]}" if options[:path]
- if options[:expire_after]
- expiry = Time.now + options[:expire_after]
- cookie << "; expires=#{expiry.httpdate}"
- end
- cookie << "; secure" if options[:secure]
- cookie << "; HttpOnly" if options[:httponly]
-
- headers = response[1]
- unless headers[SET_COOKIE].blank?
- headers[SET_COOKIE] << "\n#{cookie}"
- else
- headers[SET_COOKIE] = cookie
- end
+ cookie = {:value => sid}
+ Rack::Utils.set_cookie_header!(response[1], @key, cookie.merge(options))
@gmarik
gmarik added a line comment Apr 4, 2011

This change breaks 2.3.11 session expiration.
Rack accepts :expire option which supposed to be a Time

Fixed in this commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
end
View
25 actionpack/lib/action_controller/session/cookie_store.rb
@@ -52,7 +52,6 @@ class CookieStore
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
@@ -116,9 +115,7 @@ def call(env)
cookie[:expires] = Time.now + options[:expire_after]
end
- cookie = build_cookie(@key, cookie.merge(options))
- headers[HTTP_SET_COOKIE] = [] if headers[HTTP_SET_COOKIE].blank?
- headers[HTTP_SET_COOKIE] << cookie
+ Rack::Utils.set_cookie_header!(headers, @key, cookie.merge(options))
end
[status, headers, body]
@@ -130,26 +127,6 @@ def prepare!(env)
env[ENV_SESSION_KEY] = AbstractStore::SessionHash.new(self, env)
env[ENV_SESSION_OPTIONS_KEY] = AbstractStore::OptionsHash.new(self, env, @default_options)
end
-
- # 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)
data = unpacked_cookie_data(env)
View
10 actionpack/test/controller/session/cookie_store_test.rb
@@ -106,7 +106,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
@@ -159,7 +159,7 @@ def test_does_set_secure_cookies_over_https
with_test_route_set(:secure => true) do
get '/set_session_value', nil, 'HTTPS' => 'on'
assert_response :success
- assert_equal ["_myapp_session=#{response.body}; path=/; secure; HttpOnly"],
+ assert_equal "_myapp_session=#{response.body}; path=/; secure; HttpOnly",
headers['Set-Cookie']
end
end
@@ -195,12 +195,12 @@ 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'
assert_response :success
- assert_not_equal [], headers['Set-Cookie']
+ assert_not_equal "", headers['Set-Cookie']
assert_not_equal session_payload, cookies[SessionKey]
get '/get_session_value'
@@ -214,7 +214,7 @@ def test_setting_session_value_after_session_clear
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_session_clear'

5 comments on commit e0eb8e9

@josh
Ruby on Rails member

Finally. Can you port this back to 2-3-stable. Cookies are still fucked up there.

@paukul

huh? isn't this 2-3-stable? do you mean 2-2-stable?

@josh
Ruby on Rails member

d'oh, thought this was master.

(assuming it was applied to master already)

@paukul

well, I'd be happy to work on a patch for master as well (if thats still an issue there) :-)

@paukul

looks right in master?!

Please sign in to comment.