Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

fix setting session cookie with activerecord and memcache store

Commit f8f3653 broke setting the session ID cookie for requests without 'HTTP_COOKIE' header
when using activerecord or memcache store. Integration tests didn't catch this because they
always set the HTTP_COOKIE header for mock requests, so now this is changed to only set the
header if there are cookies.

[#5581 state:committed]

Signed-off-by: Santiago Pastorino <santiago@wyeworks.com>
  • Loading branch information...
commit c6e33d30c1fe02e5729a269ab577967fb59a5e6c 1 parent a61a39e
@mislav mislav authored spastorino committed
View
3  actionpack/lib/action_controller/integration.rb
@@ -287,7 +287,6 @@ def process(method, path, parameters = nil, headers = nil)
"REMOTE_ADDR" => remote_addr,
"CONTENT_TYPE" => "application/x-www-form-urlencoded",
"CONTENT_LENGTH" => data ? data.length.to_s : nil,
- "HTTP_COOKIE" => encode_cookies,
"HTTP_ACCEPT" => accept,
"rack.version" => [0,1],
@@ -298,6 +297,8 @@ def process(method, path, parameters = nil, headers = nil)
"rack.run_once" => false
)
+ env['HTTP_COOKIE'] = encode_cookies if cookies.any?
+
(headers || {}).each do |key, value|
key = key.to_s.upcase.gsub(/-/, "_")
key = "HTTP_#{key}" unless env.has_key?(key) || key =~ /^HTTP_/
View
4 actionpack/lib/action_controller/session/abstract_store.rb
@@ -188,7 +188,9 @@ def call(env)
return response
end
- if (env["rack.request.cookie_hash"] && env["rack.request.cookie_hash"][@key] != sid) || options[:expire_after]
+ request_cookies = env["rack.request.cookie_hash"]
+
+ if (request_cookies.nil? || request_cookies[@key] != sid) || options[:expire_after]
@isc
isc added a note

What is the reason for systematically writing the cookie header when the expire_after option is set ?
This prevents proper http caching via expire headers.

@mislav
mislav added a note

The server needs to constantly update the cookie when the expire_after option is set.

@isc
isc added a note

Ok, I thought this was only necessary when using the cookie store since the expiration of the data when using say the MemCacheStore is handled by memcached. But the cookie containing the session id needs to be updated if it has been set with an expiration date.
I got confused because in the 2.3.11 release the cookie header isn't set with an expiration date in the abstract store. I see that it has been fixed in trunk since then ( b0be721 ).
However in terms of http caching it's better to have a cookie header with a session scope and let the underlying store expire the data thus avoiding the systematic Set-Cookie header that prevents for instance the nginx proxy_cache directive to cache the answer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cookie = Rack::Utils.escape(@key) + '=' + Rack::Utils.escape(sid)
cookie << "; domain=#{options[:domain]}" if options[:domain]
cookie << "; path=#{options[:path]}" if options[:path]
Please sign in to comment.
Something went wrong with that request. Please try again.