From 2f27912480a70ec45199143bc0ff1e054f9fbb90 Mon Sep 17 00:00:00 2001 From: Michael Lovitt Date: Tue, 13 Jul 2010 13:21:37 -0400 Subject: [PATCH] Sessions should not be created until written to and session data should be destroyed on reset. --- actionpack/lib/action_controller/request.rb | 4 +- .../action_controller/session/abstract_store.rb | 173 +++++++++++++++----- .../lib/action_controller/session/cookie_store.rb | 60 ++++++-- .../action_controller/session/mem_cache_store.rb | 9 + actionpack/test/abstract_unit.rb | 17 ++ .../test/activerecord/active_record_store_test.rb | 73 ++++++-- .../test/controller/session/cookie_store_test.rb | 42 +++++ .../controller/session/mem_cache_store_test.rb | 55 ++++++- .../session_autoload_test/foo.rb | 10 + activerecord/lib/active_record/session_store.rb | 10 +- 10 files changed, 376 insertions(+), 77 deletions(-) create mode 100644 actionpack/test/fixtures/session_autoload_test/session_autoload_test/foo.rb diff --git a/actionpack/lib/action_controller/request.rb b/actionpack/lib/action_controller/request.rb index 1c3c1c8..98c5e8c 100755 --- a/actionpack/lib/action_controller/request.rb +++ b/actionpack/lib/action_controller/request.rb @@ -446,8 +446,8 @@ EOM end def reset_session - @env['rack.session.options'].delete(:id) - @env['rack.session'] = {} + session.destroy if session + self.session = {} end def session_options diff --git a/actionpack/lib/action_controller/session/abstract_store.rb b/actionpack/lib/action_controller/session/abstract_store.rb index 119b139..11cb6c2 100644 --- a/actionpack/lib/action_controller/session/abstract_store.rb +++ b/actionpack/lib/action_controller/session/abstract_store.rb @@ -2,13 +2,42 @@ require 'rack/utils' module ActionController module Session - class AbstractStore + class AbstractStore ENV_SESSION_KEY = 'rack.session'.freeze ENV_SESSION_OPTIONS_KEY = 'rack.session.options'.freeze HTTP_COOKIE = 'HTTP_COOKIE'.freeze SET_COOKIE = 'Set-Cookie'.freeze + # thin wrapper around Hash that allows us to lazily + # load session id into session_options + class OptionsHash < Hash + def initialize(by, env, default_options) + @by = by + @env = env + @session_id_loaded = false + merge!(default_options) + end + + def [](key) + if key == :id + load_session_id! unless super(:id) || has_session_id? + end + super(key) + end + + private + + def has_session_id? + @session_id_loaded + end + + def load_session_id! + self[:id] = @by.send(:extract_session_id, @env) + @session_id_loaded = true + end + end + class SessionHash < Hash def initialize(by, env) super() @@ -25,26 +54,42 @@ module ActionController end def [](key) - load! unless @loaded + load_for_read! + super + end + + def has_key?(key) + load_for_read! super end def []=(key, value) - load! unless @loaded + load_for_write! super end def clear - load! unless @loaded + load_for_write! super end def to_hash + load_for_read! h = {}.replace(self) h.delete_if { |k,v| v.nil? } h end + def update(hash) + load_for_write! + super + end + + def delete(key) + load_for_write! + super + end + def data ActiveSupport::Deprecation.warn( "ActionController::Session::AbstractStore::SessionHash#data " + @@ -53,40 +98,43 @@ module ActionController end def inspect - load! unless @loaded + load_for_read! super end + def exists? + return @exists if instance_variable_defined?(:@exists) + @exists = @by.send(:exists?, @env) + end + + def loaded? + @loaded + end + + def destroy + clear + @by.send(:destroy, @env) if @by + @env[ENV_SESSION_OPTIONS_KEY][:id] = nil if @env && @env[ENV_SESSION_OPTIONS_KEY] + @loaded = false + end + private - def loaded? - @loaded + + def load_for_read! + load! if !loaded? && exists? end - def load! - stale_session_check! do - id, session = @by.send(:load_session, @env) - (@env[ENV_SESSION_OPTIONS_KEY] ||= {})[:id] = id - replace(session) - @loaded = true - end + def load_for_write! + load! unless loaded? end - def stale_session_check! - yield - rescue ArgumentError => argument_error - if argument_error.message =~ %r{undefined class/module ([\w:]*\w)} - begin - # Note that the regexp does not allow $1 to end with a ':' - $1.constantize - rescue LoadError, NameError => const_error - raise ActionController::SessionRestoreError, "Session contains objects whose class definition isn\\'t available.\nRemember to require the classes for all objects kept in the session.\n(Original exception: \#{const_error.message} [\#{const_error.class}])\n" - end - - retry - else - raise - end + def load! + id, session = @by.send(:load_session, @env) + @env[ENV_SESSION_OPTIONS_KEY][:id] = id + replace(session) + @loaded = true end + end DEFAULT_OPTIONS = { @@ -125,18 +173,14 @@ module ActionController end def call(env) - session = SessionHash.new(self, env) - - env[ENV_SESSION_KEY] = session - env[ENV_SESSION_OPTIONS_KEY] = @default_options.dup - + prepare!(env) response = @app.call(env) session_data = env[ENV_SESSION_KEY] options = env[ENV_SESSION_OPTIONS_KEY] - if !session_data.is_a?(AbstractStore::SessionHash) || session_data.send(:loaded?) || options[:expire_after] - session_data.send(:load!) if session_data.is_a?(AbstractStore::SessionHash) && !session_data.send(:loaded?) + if !session_data.is_a?(AbstractStore::SessionHash) || session_data.loaded? || options[:expire_after] + session_data.send(:load!) if session_data.is_a?(AbstractStore::SessionHash) && !session_data.loaded? sid = options[:id] || generate_sid @@ -168,18 +212,39 @@ module ActionController end private + + def prepare!(env) + env[ENV_SESSION_KEY] = SessionHash.new(self, env) + env[ENV_SESSION_OPTIONS_KEY] = OptionsHash.new(self, env, @default_options) + end + def generate_sid ActiveSupport::SecureRandom.hex(16) end def load_session(env) - request = Rack::Request.new(env) - sid = request.cookies[@key] - unless @cookie_only - sid ||= request.params[@key] + stale_session_check! do + sid = current_session_id(env) + sid, session = get_session(env, sid) + [sid, session] + end + end + + def extract_session_id(env) + stale_session_check! do + request = Rack::Request.new(env) + sid = request.cookies[@key] + sid ||= request.params[@key] unless @cookie_only + sid end - sid, session = get_session(env, sid) - [sid, session] + end + + def current_session_id(env) + env[ENV_SESSION_OPTIONS_KEY][:id] + end + + def exists?(env) + current_session_id(env).present? end def get_session(env, sid) @@ -189,6 +254,30 @@ module ActionController def set_session(env, sid, session_data) raise '#set_session needs to be implemented.' end + + def destroy(env) + raise '#destroy needs to be implemented.' + end + + module SessionUtils + private + def stale_session_check! + yield + rescue ArgumentError => argument_error + if argument_error.message =~ %r{undefined class/module ([\w:]*\w)} + begin + # Note that the regexp does not allow $1 to end with a ':' + $1.constantize + rescue LoadError, NameError => const_error + raise ActionController::SessionRestoreError, "Session contains objects whose class definition isn\\'t available.\nRemember to require the classes for all objects kept in the session.\n(Original exception: \#{const_error.message} [\#{const_error.class}])\n" + end + retry + else + raise + end + end + end + include SessionUtils end end end diff --git a/actionpack/lib/action_controller/session/cookie_store.rb b/actionpack/lib/action_controller/session/cookie_store.rb index 2fcee56..313307c 100644 --- a/actionpack/lib/action_controller/session/cookie_store.rb +++ b/actionpack/lib/action_controller/session/cookie_store.rb @@ -36,6 +36,8 @@ module ActionController # # Note that changing digest or secret invalidates all existing sessions! class CookieStore + include AbstractStore::SessionUtils + # Cookies can typically store 4096 bytes. MAX = 4096 SECRET_MIN_LENGTH = 30 # characters @@ -93,20 +95,20 @@ module ActionController end def call(env) - env[ENV_SESSION_KEY] = AbstractStore::SessionHash.new(self, env) - env[ENV_SESSION_OPTIONS_KEY] = @default_options.dup - + prepare!(env) + status, headers, body = @app.call(env) session_data = env[ENV_SESSION_KEY] options = env[ENV_SESSION_OPTIONS_KEY] - if !session_data.is_a?(AbstractStore::SessionHash) || session_data.send(:loaded?) || options[:expire_after] - session_data.send(:load!) if session_data.is_a?(AbstractStore::SessionHash) && !session_data.send(:loaded?) + if !session_data.is_a?(AbstractStore::SessionHash) || session_data.loaded? || options[:expire_after] + session_data.send(:load!) if session_data.is_a?(AbstractStore::SessionHash) && !session_data.loaded? + + persistent_session_id!(session_data) session_data = marshal(session_data.to_hash) raise CookieOverflow if session_data.size > MAX - cookie = Hash.new cookie[:value] = session_data unless options[:expire_after].nil? @@ -122,6 +124,12 @@ module ActionController end private + + 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 @@ -143,20 +151,46 @@ module ActionController end def load_session(env) - request = Rack::Request.new(env) - session_data = request.cookies[@key] - data = unmarshal(session_data) || persistent_session_id!({}) + data = unpacked_cookie_data(env) + data = persistent_session_id!(data) [data[:session_id], data] end + + def extract_session_id(env) + if data = unpacked_cookie_data(env) + persistent_session_id!(data) unless data.empty? + data[:session_id] + else + nil + end + end + + def current_session_id(env) + env[ENV_SESSION_OPTIONS_KEY][:id] + end + + def exists?(env) + current_session_id(env).present? + end + + def unpacked_cookie_data(env) + env["action_dispatch.request.unsigned_session_cookie"] ||= begin + stale_session_check! do + request = Rack::Request.new(env) + session_data = request.cookies[@key] + unmarshal(session_data) || {} + end + end + end # Marshal a session hash into safe cookie data. Include an integrity hash. def marshal(session) - @verifier.generate(persistent_session_id!(session)) + @verifier.generate(session) end # Unmarshal cookie data to a hash and verify its integrity. def unmarshal(cookie) - persistent_session_id!(@verifier.verify(cookie)) if cookie + @verifier.verify(cookie) if cookie rescue ActiveSupport::MessageVerifier::InvalidSignature nil end @@ -204,6 +238,10 @@ module ActionController ActiveSupport::SecureRandom.hex(16) end + def destroy(env) + # session data is stored on client; nothing to do here + end + def persistent_session_id!(data) (data ||= {}).merge!(inject_persistent_session_id(data)) end diff --git a/actionpack/lib/action_controller/session/mem_cache_store.rb b/actionpack/lib/action_controller/session/mem_cache_store.rb index f745715..402681c 100644 --- a/actionpack/lib/action_controller/session/mem_cache_store.rb +++ b/actionpack/lib/action_controller/session/mem_cache_store.rb @@ -43,6 +43,15 @@ begin rescue MemCache::MemCacheError, Errno::ECONNREFUSED return false end + + def destroy(env) + if sid = current_session_id(env) + @pool.delete(sid) + end + rescue MemCache::MemCacheError, Errno::ECONNREFUSED + false + end + end end end diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb index 096798d..0dce550 100644 --- a/actionpack/test/abstract_unit.rb +++ b/actionpack/test/abstract_unit.rb @@ -58,4 +58,21 @@ class DummyMutex end end +class ActionController::IntegrationTest < ActiveSupport::TestCase + def with_autoload_path(path) + path = File.join(File.dirname(__FILE__), "fixtures", path) + if ActiveSupport::Dependencies.autoload_paths.include?(path) + yield + else + begin + ActiveSupport::Dependencies.autoload_paths << path + yield + ensure + ActiveSupport::Dependencies.autoload_paths.reject! {|p| p == path} + ActiveSupport::Dependencies.clear + end + end + end +end + ActionController::Reloader.default_lock = DummyMutex.new diff --git a/actionpack/test/activerecord/active_record_store_test.rb b/actionpack/test/activerecord/active_record_store_test.rb index c3d4bf1..6f2fe32 100644 --- a/actionpack/test/activerecord/active_record_store_test.rb +++ b/actionpack/test/activerecord/active_record_store_test.rb @@ -22,7 +22,6 @@ class ActiveRecordStoreTest < ActionController::IntegrationTest end def get_session_id - session[:foo] render :text => "#{request.session_options[:id]}" end @@ -45,23 +44,27 @@ class ActiveRecordStoreTest < ActionController::IntegrationTest ActiveRecord::SessionStore.session_class.drop_table! end - def test_setting_and_getting_session_value - 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 'foo: "bar"', response.body - - get '/set_session_value', :foo => "baz" - assert_response :success - assert cookies['_session_id'] - - get '/get_session_value' - assert_response :success - assert_equal 'foo: "baz"', response.body + %w{ session sql_bypass }.each do |class_name| + define_method("test_setting_and_getting_session_value_with_#{class_name}_store") do + with_store class_name do + 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 'foo: "bar"', response.body + + get '/set_session_value', :foo => "baz" + assert_response :success + assert cookies['_session_id'] + + get '/get_session_value' + assert_response :success + assert_equal 'foo: "baz"', response.body + end + end end end @@ -107,7 +110,7 @@ class ActiveRecordStoreTest < ActionController::IntegrationTest end end - def test_doesnt_write_session_cookie_if_session_id_is_already_exists + def test_getting_session_value with_test_route_set do get '/set_session_value' assert_response :success @@ -116,6 +119,26 @@ class ActiveRecordStoreTest < ActionController::IntegrationTest 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" + session_id = cookies["_session_id"] + + get '/call_reset_session' + assert_response :success + assert_not_equal [], headers['Set-Cookie'] + + cookies["_session_id"] = session_id # replace our new session_id with our old, pre-reset session_id + + get '/get_session_value' + assert_response :success + assert_equal 'foo: nil', response.body, "data for this session should have been obliterated from the database" + end + end + + def test_getting_from_nonexistent_session + with_test_route_set do + get '/get_session_value' + assert_response :success + assert_equal 'foo: nil', response.body + assert_nil cookies['_session_id'], "should only create session on write, not read" end end @@ -183,4 +206,16 @@ class ActiveRecordStoreTest < ActionController::IntegrationTest yield end end + + def with_store(class_name) + begin + session_class = ActiveRecord::SessionStore.session_class + ActiveRecord::SessionStore.session_class = "ActiveRecord::SessionStore::#{class_name.camelize}".constantize + yield + rescue + ActiveRecord::SessionStore.session_class = session_class + raise + end + end + end diff --git a/actionpack/test/controller/session/cookie_store_test.rb b/actionpack/test/controller/session/cookie_store_test.rb index b45eb1e..f157ae4 100644 --- a/actionpack/test/controller/session/cookie_store_test.rb +++ b/actionpack/test/controller/session/cookie_store_test.rb @@ -34,6 +34,10 @@ class CookieStoreTest < ActionController::IntegrationTest render :text => "foo: #{session[:foo].inspect}; id: #{request.session_options[:id]}" end + def get_session_id_only + render :text => "id: #{request.session_options[:id]}" + end + def call_session_clear session.clear head :ok @@ -132,6 +136,10 @@ class CookieStoreTest < ActionController::IntegrationTest get '/get_session_id' assert_response :success assert_equal "foo: \"bar\"; id: #{session_id}", response.body + + get '/get_session_id_only' + assert_response :success + assert_equal "id: #{session_id}", response.body, "should be able to read session id without accessing the session hash" end end @@ -206,6 +214,40 @@ class CookieStoreTest < ActionController::IntegrationTest end end + def test_getting_from_nonexistent_session + with_test_route_set do + get '/get_session_value' + assert_response :success + assert_equal 'foo: nil', response.body + assert_nil headers['Set-Cookie'], "should only create session on write, not read" + end + end + + # {:foo=>#, :session_id=>"ce8b0752a6ab7c7af3cdb8a80e6b9e46"} + SignedSerializedCookie = "BAh7BzoIZm9vbzodU2Vzc2lvbkF1dG9sb2FkVGVzdDo6Rm9vBjoJQGJhciIIYmF6Og9zZXNzaW9uX2lkIiVjZThiMDc1MmE2YWI3YzdhZjNjZGI4YTgwZTZiOWU0Ng==--2bf3af1ae8bd4e52b9ac2099258ace0c380e601c" + + def test_deserializes_unloaded_classes_on_get_id + with_test_route_set do + with_autoload_path "session_autoload_test" do + cookies[SessionKey] = SignedSerializedCookie + get '/get_session_id_only' + assert_response :success + assert_equal 'id: ce8b0752a6ab7c7af3cdb8a80e6b9e46', response.body, "should auto-load unloaded class" + end + end + end + + def test_deserializes_unloaded_classes_on_get_value + with_test_route_set do + with_autoload_path "session_autoload_test" do + cookies[SessionKey] = SignedSerializedCookie + get '/get_session_value' + assert_response :success + assert_equal 'foo: #', response.body, "should auto-load unloaded class" + end + end + end + def test_persistent_session_id with_test_route_set do cookies[SessionKey] = SignedBar diff --git a/actionpack/test/controller/session/mem_cache_store_test.rb b/actionpack/test/controller/session/mem_cache_store_test.rb index e505874..cfb6a18 100644 --- a/actionpack/test/controller/session/mem_cache_store_test.rb +++ b/actionpack/test/controller/session/mem_cache_store_test.rb @@ -12,12 +12,16 @@ class MemCacheStoreTest < ActionController::IntegrationTest head :ok end + def set_serialized_session_value + session[:foo] = SessionAutoloadTest::Foo.new + head :ok + end + def get_session_value render :text => "foo: #{session[:foo].inspect}" end def get_session_id - session[:foo] render :text => "#{request.session_options[:id]}" end @@ -82,6 +86,34 @@ class MemCacheStoreTest < ActionController::IntegrationTest end end + def test_getting_session_value_after_session_reset + with_test_route_set do + get '/set_session_value' + assert_response :success + assert cookies['_session_id'] + session_id = cookies["_session_id"] + + get '/call_reset_session' + assert_response :success + assert_not_equal [], headers['Set-Cookie'] + + cookies["_session_id"] = session_id # replace our new session_id with our old, pre-reset session_id + + get '/get_session_value' + assert_response :success + assert_equal 'foo: nil', response.body, "data for this session should have been obliterated from memcached" + end + end + + def test_getting_from_nonexistent_session + with_test_route_set do + get '/get_session_value' + assert_response :success + assert_equal 'foo: nil', response.body + assert_nil cookies['_session_id'], "should only create session on write, not read" + end + end + def test_getting_session_id with_test_route_set do get '/set_session_value' @@ -91,7 +123,7 @@ class MemCacheStoreTest < ActionController::IntegrationTest get '/get_session_id' assert_response :success - assert_equal session_id, response.body + assert_equal session_id, response.body, "should be able to read session id without accessing the session hash" end end @@ -106,6 +138,25 @@ class MemCacheStoreTest < ActionController::IntegrationTest assert_equal nil, headers['Set-Cookie'], "should not resend the cookie again if session_id cookie is already exists" end end + + def test_deserializes_unloaded_class + with_test_route_set do + with_autoload_path "session_autoload_test" do + get '/set_serialized_session_value' + assert_response :success + assert cookies['_session_id'] + end + with_autoload_path "session_autoload_test" do + get '/get_session_id' + assert_response :success + end + with_autoload_path "session_autoload_test" do + get '/get_session_value' + assert_response :success + assert_equal 'foo: #', response.body, "should auto-load unloaded class" + end + end + end def test_prevents_session_fixation with_test_route_set do diff --git a/actionpack/test/fixtures/session_autoload_test/session_autoload_test/foo.rb b/actionpack/test/fixtures/session_autoload_test/session_autoload_test/foo.rb new file mode 100644 index 0000000..145c92e --- /dev/null +++ b/actionpack/test/fixtures/session_autoload_test/session_autoload_test/foo.rb @@ -0,0 +1,10 @@ +module SessionAutoloadTest + class Foo + def initialize(bar='baz') + @bar = bar + end + def inspect + "#<#{self.class} bar:#{@bar.inspect}>" + end + end +end \ No newline at end of file diff --git a/activerecord/lib/active_record/session_store.rb b/activerecord/lib/active_record/session_store.rb index c91b943..d6d0c6e 100644 --- a/activerecord/lib/active_record/session_store.rb +++ b/activerecord/lib/active_record/session_store.rb @@ -184,7 +184,7 @@ module ActiveRecord # Look up a session by id and unmarshal its data if found. def find_by_session_id(session_id) - if record = @@connection.select_one("SELECT * FROM #{@@table_name} WHERE #{@@session_id_column}=#{@@connection.quote(session_id)}") + if record = connection.select_one("SELECT * FROM #{@@table_name} WHERE #{@@session_id_column}=#{connection.quote(session_id)}") new(:session_id => session_id, :marshaled_data => record['data']) end end @@ -310,6 +310,14 @@ module ActiveRecord return true end + def destroy(env) + if sid = current_session_id(env) + Base.silence do + get_session_model(env, sid).destroy + end + end + end + def get_session_model(env, sid) if env[ENV_SESSION_OPTIONS_KEY][:id].nil? env[SESSION_RECORD_KEY] = find_session(sid) -- 1.7.1