Skip to content
Browse files

Sessions should not be created until written to and session data shou…

…ld be destroyed on reset. [#4938 state:resolved]

Signed-off-by: José Valim <jose.valim@gmail.com>
  • Loading branch information...
1 parent 8298bef commit 257a29d3cca91913325a8bfef0f06b7ec6c4654b @lovitt lovitt committed with josevalim Jul 13, 2010
View
4 actionpack/lib/action_controller/request.rb
@@ -446,8 +446,8 @@ def session=(session) #:nodoc:
end
def reset_session
- @env['rack.session.options'].delete(:id)
- @env['rack.session'] = {}
+ session.destroy if session
+ self.session = {}
end
def session_options
View
173 actionpack/lib/action_controller/session/abstract_store.rb
@@ -2,13 +2,42 @@
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 @@ def session_id
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 @@ def data
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 @@ def initialize(app, options = {})
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 @@ def call(env)
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 @@ def get_session(env, sid)
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
View
60 actionpack/lib/action_controller/session/cookie_store.rb
@@ -36,6 +36,8 @@ module Session
#
# 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 @@ def initialize(app, options = {})
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 @@ def call(env)
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 @@ def build_cookie(key, value)
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 @@ def generate_sid
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
View
9 actionpack/lib/action_controller/session/mem_cache_store.rb
@@ -43,6 +43,15 @@ def set_session(env, sid, session_data)
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
View
17 actionpack/test/abstract_unit.rb
@@ -58,4 +58,21 @@ def locked?
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
View
73 actionpack/test/activerecord/active_record_store_test.rb
@@ -22,7 +22,6 @@ def get_session_value
end
def get_session_id
- session[:foo]
render :text => "#{request.session_options[:id]}"
end
@@ -45,23 +44,27 @@ def teardown
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 @@ def test_getting_session_id
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 @@ def test_doesnt_write_session_cookie_if_session_id_is_already_exists
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 @@ def with_test_route_set
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
View
42 actionpack/test/controller/session/cookie_store_test.rb
@@ -34,6 +34,10 @@ def get_session_id
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 @@ def test_getting_session_id
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 @@ def test_setting_session_value_after_session_clear
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=>#<SessionAutoloadTest::Foo bar:"baz">, :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: #<SessionAutoloadTest::Foo bar:"baz">', response.body, "should auto-load unloaded class"
+ end
+ end
+ end
+
def test_persistent_session_id
with_test_route_set do
cookies[SessionKey] = SignedBar
View
55 actionpack/test/controller/session/mem_cache_store_test.rb
@@ -12,12 +12,16 @@ def set_session_value
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 @@ def test_setting_session_value_after_session_reset
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 @@ def test_getting_session_id
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 @@ def test_doesnt_write_session_cookie_if_session_id_is_already_exists
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: #<SessionAutoloadTest::Foo bar:"baz">', response.body, "should auto-load unloaded class"
+ end
+ end
+ end
def test_prevents_session_fixation
with_test_route_set do
View
10 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
View
10 activerecord/lib/active_record/session_store.rb
@@ -184,7 +184,7 @@ def connection
# 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 @@ def set_session(env, sid, session_data)
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)

0 comments on commit 257a29d

Please sign in to comment.
Something went wrong with that request. Please try again.