From 4e995559d74ba38be0d04e95e4beccd7c04a2333 Mon Sep 17 00:00:00 2001 From: Prem Sichanugrist Date: Fri, 25 Jun 2010 03:22:41 +0700 Subject: [PATCH] Make sure that Rails doesn't resent session_id cookie over and over again if it's already there [#2485 state:resolved] This apply to only Active Record store and Memcached store, as they both store only the session_id, which will be unchanged, in the cookie. --- .../action_controller/session/abstract_store.rb | 32 ++++++++++--------- .../test/activerecord/active_record_store_test.rb | 12 +++++++ .../controller/session/mem_cache_store_test.rb | 12 +++++++ 3 files changed, 41 insertions(+), 15 deletions(-) diff --git a/actionpack/lib/action_controller/session/abstract_store.rb b/actionpack/lib/action_controller/session/abstract_store.rb index 7a20557..97f9cc5 100644 --- a/actionpack/lib/action_controller/session/abstract_store.rb +++ b/actionpack/lib/action_controller/session/abstract_store.rb @@ -139,21 +139,23 @@ module ActionController return response end - 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 + if (env["rack.request.cookie_hash"] && env["rack.request.cookie_hash"][@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 end end diff --git a/actionpack/test/activerecord/active_record_store_test.rb b/actionpack/test/activerecord/active_record_store_test.rb index bde36eb..c3d4bf1 100644 --- a/actionpack/test/activerecord/active_record_store_test.rb +++ b/actionpack/test/activerecord/active_record_store_test.rb @@ -107,6 +107,18 @@ class ActiveRecordStoreTest < ActionController::IntegrationTest end end + def test_doesnt_write_session_cookie_if_session_id_is_already_exists + with_test_route_set do + get '/set_session_value' + assert_response :success + assert cookies['_session_id'] + + get '/get_session_value' + assert_response :success + assert_equal nil, headers['Set-Cookie'], "should not resend the cookie again if session_id cookie is already exists" + end + end + def test_prevents_session_fixation with_test_route_set do get '/set_session_value' diff --git a/actionpack/test/controller/session/mem_cache_store_test.rb b/actionpack/test/controller/session/mem_cache_store_test.rb index 2f80a3c..e505874 100644 --- a/actionpack/test/controller/session/mem_cache_store_test.rb +++ b/actionpack/test/controller/session/mem_cache_store_test.rb @@ -95,6 +95,18 @@ class MemCacheStoreTest < ActionController::IntegrationTest end end + def test_doesnt_write_session_cookie_if_session_id_is_already_exists + with_test_route_set do + get '/set_session_value' + assert_response :success + assert cookies['_session_id'] + + get '/get_session_value' + assert_response :success + assert_equal nil, headers['Set-Cookie'], "should not resend the cookie again if session_id cookie is already exists" + end + end + def test_prevents_session_fixation with_test_route_set do get '/get_session_value' -- 1.7.0.4