Skip to content

Commit

Permalink
reset_session should force a new session id to be generated [#2173]
Browse files Browse the repository at this point in the history
  • Loading branch information
josh committed Mar 10, 2009
1 parent 4458edc commit 224a534
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 16 deletions.
1 change: 1 addition & 0 deletions actionpack/lib/action_controller/request.rb
Expand Up @@ -442,6 +442,7 @@ def session=(session) #:nodoc:
end

def reset_session
@env['rack.session.options'].delete(:id)
@env['rack.session'] = {}
end

Expand Down
25 changes: 25 additions & 0 deletions actionpack/test/activerecord/active_record_store_test.rb
Expand Up @@ -21,8 +21,15 @@ def get_session_value
render :text => "foo: #{session[:foo].inspect}"
end

def get_session_id
session[:foo]
render :text => "#{request.session_options[:id]}"
end

def call_reset_session
session[:bar]
reset_session
session[:bar] = "baz"
head :ok
end

Expand Down Expand Up @@ -71,6 +78,7 @@ def test_setting_session_value_after_session_reset
get '/set_session_value'
assert_response :success
assert cookies['_session_id']
session_id = cookies['_session_id']

get '/call_reset_session'
assert_response :success
Expand All @@ -79,6 +87,23 @@ def test_setting_session_value_after_session_reset
get '/get_session_value'
assert_response :success
assert_equal 'foo: nil', response.body

get '/get_session_id'
assert_response :success
assert_not_equal session_id, response.body
end
end

def test_getting_session_id
with_test_route_set do
get '/set_session_value'
assert_response :success
assert cookies['_session_id']
session_id = cookies['_session_id']

get '/get_session_id'
assert_response :success
assert_equal session_id, response.body
end
end

Expand Down
40 changes: 24 additions & 16 deletions actionpack/test/controller/session/mem_cache_store_test.rb
Expand Up @@ -17,11 +17,14 @@ def get_session_value
end

def get_session_id
render :text => "foo: #{session[:foo].inspect}; id: #{request.session_options[:id]}"
session[:foo]
render :text => "#{request.session_options[:id]}"
end

def call_reset_session
session[:bar]
reset_session
session[:bar] = "baz"
head :ok
end

Expand Down Expand Up @@ -58,47 +61,52 @@ def test_getting_nil_session_value
end
end

def test_getting_session_id
def test_setting_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 '/get_session_id'
get '/call_reset_session'
assert_response :success
assert_equal "foo: \"bar\"; id: #{session_id}", response.body
end
end
assert_not_equal [], headers['Set-Cookie']

def test_prevents_session_fixation
with_test_route_set do
get '/get_session_value'
assert_response :success
assert_equal 'foo: nil', response.body
session_id = cookies['_session_id']

reset!

get '/set_session_value', :_session_id => session_id
get '/get_session_id'
assert_response :success
assert_equal nil, cookies['_session_id']
assert_not_equal session_id, response.body
end
end

def test_setting_session_value_after_session_reset
def test_getting_session_id
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'
get '/get_session_id'
assert_response :success
assert_not_equal [], headers['Set-Cookie']
assert_equal session_id, response.body
end
end

def test_prevents_session_fixation
with_test_route_set do
get '/get_session_value'
assert_response :success
assert_equal 'foo: nil', response.body
session_id = cookies['_session_id']

reset!

get '/set_session_value', :_session_id => session_id
assert_response :success
assert_equal nil, cookies['_session_id']
end
end
rescue LoadError, RuntimeError
Expand Down

3 comments on commit 224a534

@gaffo
Copy link
Contributor

@gaffo gaffo commented on 224a534 Mar 11, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work with the test session?

@josh
Copy link
Contributor Author

@josh josh commented on 224a534 Mar 11, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functional test’s session is just a mock and never get turns into a real session id or cookie.

@cch1
Copy link
Contributor

@cch1 cch1 commented on 224a534 Apr 14, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gaffo's question is still pretty valid though. If #reset_session "should force a new session id" (and I agree it should) then it's natural to build on that functionality. And if it doesn't behave that way in tests, then things go awry.

Please sign in to comment.