mirror of
https://github.com/rails/rails.git
synced 2022-11-09 12:12:34 -05:00
Fixed session ID fixation for ActiveRecord::SessionStore
I have found that Rails will take an invalid session ID specified by the client and materialize a session based on that session ID. This means that it is possible, among other things, for a client to use an arbitrarily weak session ID or for a client to resurrect a previous used session ID. In other words, we cannot guarantee that all session IDs are generated by the server and that they are (statistically) unique through time. The fix is to always generate a new session ID in #get_session if an existing session cannot be found under the incoming session ID. Also added new tests that make sure that an invalid session ID is never materialized into a new session, regardless of whether it comes in via a cookie or a URL parameter (when :cookie_only => false).
This commit is contained in:
parent
e4479b2f1b
commit
66dee26930
2 changed files with 37 additions and 2 deletions
|
@ -226,6 +226,36 @@ class ActiveRecordStoreTest < ActionDispatch::IntegrationTest
|
|||
end
|
||||
end
|
||||
|
||||
def test_incoming_invalid_session_id_via_cookie_should_be_ignored
|
||||
with_test_route_set do
|
||||
open_session do |sess|
|
||||
sess.cookies['_session_id'] = 'INVALID'
|
||||
|
||||
sess.get '/set_session_value'
|
||||
new_session_id = sess.cookies['_session_id']
|
||||
assert_not_equal 'INVALID', new_session_id
|
||||
|
||||
sess.get '/get_session_value'
|
||||
new_session_id_2 = sess.cookies['_session_id']
|
||||
assert_equal new_session_id, new_session_id_2
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
def test_incoming_invalid_session_id_via_parameter_should_be_ignored
|
||||
with_test_route_set(:cookie_only => false) do
|
||||
open_session do |sess|
|
||||
sess.get '/set_session_value', :_session_id => 'INVALID'
|
||||
new_session_id = sess.cookies['_session_id']
|
||||
assert_not_equal 'INVALID', new_session_id
|
||||
|
||||
sess.get '/get_session_value'
|
||||
new_session_id_2 = sess.cookies['_session_id']
|
||||
assert_equal new_session_id, new_session_id_2
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def with_test_route_set(options = {})
|
||||
|
@ -247,6 +277,7 @@ class ActiveRecordStoreTest < ActionDispatch::IntegrationTest
|
|||
session_class, ActiveRecord::SessionStore.session_class =
|
||||
ActiveRecord::SessionStore.session_class, "ActiveRecord::SessionStore::#{class_name.camelize}".constantize
|
||||
yield
|
||||
ensure
|
||||
ActiveRecord::SessionStore.session_class = session_class
|
||||
end
|
||||
end
|
||||
|
|
|
@ -297,8 +297,12 @@ module ActiveRecord
|
|||
private
|
||||
def get_session(env, sid)
|
||||
Base.silence do
|
||||
sid ||= generate_sid
|
||||
session = find_session(sid)
|
||||
unless sid and session = @@session_class.find_by_session_id(sid)
|
||||
# If the sid was nil or if there is no pre-existing session under the sid,
|
||||
# force the generation of a new sid and associate a new session associated with the new sid
|
||||
sid = generate_sid
|
||||
session = @@session_class.new(:session_id => sid, :data => {})
|
||||
end
|
||||
env[SESSION_RECORD_KEY] = session
|
||||
[sid, session.data]
|
||||
end
|
||||
|
|
Loading…
Reference in a new issue