New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly reset the session on reset_session #7495

Merged
merged 4 commits into from Sep 2, 2012

Conversation

Projects
None yet
4 participants
@steveklabnik
Member

steveklabnik commented Aug 31, 2012

Fixes #7478

Well, it would if it were actually finished. There is one test failure:

  1) Failure:
test_setting_session_value_after_session_reset(CookieStoreTest) [/Users/steve/src/rails/actionpack/test/dispatch/session/cookie_store_test.rb:190]:
Expected "BAh7B0kiD3Nlc3Npb25faWQGOgZFRkkiJTlmZTczNjc4NThiNWZhODIzNjg2Mjk2NDVlZGNkYTMwBjsAVEkiCGZvbwY7AEZJIghiYXIGOwBG--84236482786035ade1225b520f9439784ea16fb1" to not be equal to "BAh7B0kiD3Nlc3Npb25faWQGOgZFRkkiJTlmZTczNjc4NThiNWZhODIzNjg2Mjk2NDVlZGNkYTMwBjsAVEkiCGZvbwY7AEZJIghiYXIGOwBG--84236482786035ade1225b520f9439784ea16fb1".

I am not sure why #destroy does not reset the session. It claims to, and following the trail of code, looks like it does. But it doesn't. Anybody have any ideas?

@reinh

This comment has been minimized.

Show comment
Hide comment
@reinh

reinh Aug 31, 2012

Doesn't this need indifferent access? Wasn't that the reason for #7478?

self.session = HashWithIndifferentAccess.new

Doesn't this need indifferent access? Wasn't that the reason for #7478?

self.session = HashWithIndifferentAccess.new

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Aug 31, 2012

Owner

It is not a HWIA, it's a Rack::Session::Abstract::SessionHash.

Also, in All Real Rails Code, the destroy path should be hit. The {} is because of tests.

Owner

steveklabnik replied Aug 31, 2012

It is not a HWIA, it's a Rack::Session::Abstract::SessionHash.

Also, in All Real Rails Code, the destroy path should be hit. The {} is because of tests.

This comment has been minimized.

Show comment
Hide comment
@reinh

reinh Aug 31, 2012

Ok, it's a magical HWIA. I still think we should maintain the class invariance.

reinh replied Aug 31, 2012

Ok, it's a magical HWIA. I still think we should maintain the class invariance.

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Aug 31, 2012

Owner

Sure. I'm trying to make Minimal Changes right now to just fix the bug. RSASH needs some stuff to instantiate it, and I'm not sure that I have access to it here?

Owner

steveklabnik replied Aug 31, 2012

Sure. I'm trying to make Minimal Changes right now to just fix the bug. RSASH needs some stuff to instantiate it, and I'm not sure that I have access to it here?

This comment has been minimized.

Show comment
Hide comment
@reinh

reinh Aug 31, 2012

You're right: if we're breaking the class invariance then it's happening elsewhere and probably shouldn't be fixed here: the fix ought to collapse this if branch since we will no longer be getting here without a RSASH. Maybe our tests depend on the duck-typing of RSASH to Hash which isn't valid because of #destroy?

reinh replied Aug 31, 2012

You're right: if we're breaking the class invariance then it's happening elsewhere and probably shouldn't be fixed here: the fix ought to collapse this if branch since we will no longer be getting here without a RSASH. Maybe our tests depend on the duck-typing of RSASH to Hash which isn't valid because of #destroy?

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Aug 31, 2012

Owner

Quite possible. I haven't gotten the chance to investigate the tests yet.

Owner

steveklabnik replied Aug 31, 2012

Quite possible. I haven't gotten the chance to investigate the tests yet.

This comment has been minimized.

Show comment
Hide comment
@reinh

reinh Aug 31, 2012

Seems fair. I think the type leak is worth finding and fixing. It's causing bugs and unnecessary complexity to deal with the unexpected polymorphism. Wonder where that bugger lives...

reinh replied Aug 31, 2012

Seems fair. I think the type leak is worth finding and fixing. It's causing bugs and unnecessary complexity to deal with the unexpected polymorphism. Wonder where that bugger lives...

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Aug 31, 2012

Member

I am not sure why #destroy does not reset the session. It claims to, and following the trail of code, looks like it does. But it doesn't. Anybody have any ideas?

My guess is that it's a bug in the test framework. Can you try generating an app and testing with real requests and responses? I'll bet that the id does get reset IRL, but that the test framework is reusing objects or something. I know for a fact the test framework is reusing requests and response objects, and that may impact this test.

Member

tenderlove commented Aug 31, 2012

I am not sure why #destroy does not reset the session. It claims to, and following the trail of code, looks like it does. But it doesn't. Anybody have any ideas?

My guess is that it's a bug in the test framework. Can you try generating an app and testing with real requests and responses? I'll bet that the id does get reset IRL, but that the test framework is reusing objects or something. I know for a fact the test framework is reusing requests and response objects, and that may impact this test.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Aug 31, 2012

Member

Word. I'll see if I can give that a shot after eating this delicious soup!

... of course, this is something you ALREADY TOLD ME on Campfire... duh. I got too lost in backtraces. :)

Member

steveklabnik commented Aug 31, 2012

Word. I'll see if I can give that a shot after eating this delicious soup!

... of course, this is something you ALREADY TOLD ME on Campfire... duh. I got too lost in backtraces. :)

@alup

This comment has been minimized.

Show comment
Hide comment
@alup

alup Sep 1, 2012

Contributor

@steveklabnik I have included two more changesets related to this fix (https://github.com/alup/rails/commits/). Feel free to pull them. I have also rebased and tested the latest master, but I got the same test failure. I haven't yet investigated the test, but I have tested the changes in a real app (@tenderlove) and the reset works fine. The session id is now regenerated and assigned correctly.

Contributor

alup commented Sep 1, 2012

@steveklabnik I have included two more changesets related to this fix (https://github.com/alup/rails/commits/). Feel free to pull them. I have also rebased and tested the latest master, but I got the same test failure. I haven't yet investigated the test, but I have tested the changes in a real app (@tenderlove) and the reset works fine. The session id is now regenerated and assigned correctly.

@alup

This comment has been minimized.

Show comment
Hide comment
@alup

alup Sep 1, 2012

Contributor

Update: Now with the latest commit alup@b22fe1d all the tests are passing :)

Contributor

alup commented Sep 1, 2012

Update: Now with the latest commit alup@b22fe1d all the tests are passing :)

alup added some commits Sep 1, 2012

Force reloading of the session after destroy
Use load_for_write! to ensure a refresh of the session object.
This way the new session_id and the empty data will be stored properly.
E.g. in the case of the session cookie store this means that a new
digest will be returned to the user.
@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Sep 1, 2012

Member

Awesome! I've added them into this PR. Seems legit to me. @tenderlove?

Member

steveklabnik commented Sep 1, 2012

Awesome! I've added them into this PR. Seems legit to me. @tenderlove?

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Sep 2, 2012

Member

Seems good to me. Do the tests pass?

Member

tenderlove commented Sep 2, 2012

Seems good to me. Do the tests pass?

@alup

This comment has been minimized.

Show comment
Hide comment
@alup

alup Sep 2, 2012

Contributor

All tests of actionpack are passing.

Contributor

alup commented Sep 2, 2012

All tests of actionpack are passing.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Sep 2, 2012

Member

Yep, the all pass for me.

Member

steveklabnik commented Sep 2, 2012

Yep, the all pass for me.

@alup

This comment has been minimized.

Show comment
Hide comment
@alup

alup Sep 2, 2012

Contributor

Except for the MemCacheStore tests which are skipped :S due to the lack of memcached-client gem (it has been replaced with dalli but the related refactoring to the tests has been reverted).

Contributor

alup commented Sep 2, 2012

Except for the MemCacheStore tests which are skipped :S due to the lack of memcached-client gem (it has been replaced with dalli but the related refactoring to the tests has been reverted).

@alup

This comment has been minimized.

Show comment
Hide comment
@alup

alup Sep 2, 2012

Contributor

Ok, managed to test against memcached, too. All tests are passing for actionpack.

Contributor

alup commented Sep 2, 2012

Ok, managed to test against memcached, too. All tests are passing for actionpack.

tenderlove added a commit that referenced this pull request Sep 2, 2012

Merge pull request #7495 from steveklabnik/issue_7478
Properly reset the session on reset_session

@tenderlove tenderlove merged commit abd47c1 into rails:master Sep 2, 2012

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 2, 2012

Just found this issue, as I was noticing some odd session behavior with 3.2.8 this evening.
I haven't studied this, but since this is being actively looked at, I wanted to share the two issues I'm seeing:

  1. For new sessions, in order to access session[:session_id], load_for_write! must be called first.
    This may be intentional, since if the session is empty, simply attempting to read a key value may not warrant a load.
  2. If reset_session is called, session returns a Hash - not a Rack::Session::Abstract::SessionHash.
    Not sure about this, but it does prevent using a write operation against the session in order to discover the new session id.

ghost commented Sep 2, 2012

Just found this issue, as I was noticing some odd session behavior with 3.2.8 this evening.
I haven't studied this, but since this is being actively looked at, I wanted to share the two issues I'm seeing:

  1. For new sessions, in order to access session[:session_id], load_for_write! must be called first.
    This may be intentional, since if the session is empty, simply attempting to read a key value may not warrant a load.
  2. If reset_session is called, session returns a Hash - not a Rack::Session::Abstract::SessionHash.
    Not sure about this, but it does prevent using a write operation against the session in order to discover the new session id.
@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Sep 2, 2012

Member

@burns: #2 should be solved, now that this has been merged.

Member

steveklabnik commented Sep 2, 2012

@burns: #2 should be solved, now that this has been merged.

@alup

This comment has been minimized.

Show comment
Hide comment
@alup

alup Sep 2, 2012

Contributor

@burns Better look at the master branch cause lots of code pieces have been moved (e.g. the session class has been moved to actionpack/lib/action_dispatch/request/session.rb) from 3.2.x to 4.0.0

Contributor

alup commented Sep 2, 2012

@burns Better look at the master branch cause lots of code pieces have been moved (e.g. the session class has been moved to actionpack/lib/action_dispatch/request/session.rb) from 3.2.x to 4.0.0

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Sep 2, 2012

Member

Yes, it's not a trivial backport for sure.

Member

steveklabnik commented Sep 2, 2012

Yes, it's not a trivial backport for sure.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 2, 2012

Would this be an acceptable solution to #1 ?

index a05a23d..721d67a 100644
--- a/actionpack/lib/action_dispatch/request/session.rb
+++ b/actionpack/lib/action_dispatch/request/session.rb
@@ -80,6 +80,8 @@ module ActionDispatch

       def [](key)
         load_for_read!
+        load_for_write! if key.to_s == 'session_id'
         @delegate[key.to_s]
       end

So, if exists? fails and there is no persisted id either in the current options or extracted from the
unpacked_cookie_data, then call load! to generate a new one.

I don't think this can simply be pulled from options[:id]. For one, it wouldn't be there for a new session. Second,
for a persisted session_id, it would seem that unless the session store is asked to extract this from the
unpacked_cookie_data, the session id from the rack env would be an id for a session that hasn't been verified against
the digest. Which is a bit concerning, since apparently many are currently using request.session_options[:id] which I believe would be the same situation.

Anyway, this would still prevent a session load for other keys when there is no active session. Of course, this should
only occur when the site is initially accessed or the session cookie is deleted/disabled. FWIW, the call to the
csrf_meta_tags helper call is going to write that token in the session. So unless that's removed, you're going to get
at least one write op that will force a session load. Of course this happens too late to help with accessing
session[:session_id] in a controller or other view. Honestly I'm not sure if I see a great benefit in distinguishing
between load_for_read/load_for_write, as there's probably very few requests that wouldn't perform at least this one
write to the session.

ghost commented Sep 2, 2012

Would this be an acceptable solution to #1 ?

index a05a23d..721d67a 100644
--- a/actionpack/lib/action_dispatch/request/session.rb
+++ b/actionpack/lib/action_dispatch/request/session.rb
@@ -80,6 +80,8 @@ module ActionDispatch

       def [](key)
         load_for_read!
+        load_for_write! if key.to_s == 'session_id'
         @delegate[key.to_s]
       end

So, if exists? fails and there is no persisted id either in the current options or extracted from the
unpacked_cookie_data, then call load! to generate a new one.

I don't think this can simply be pulled from options[:id]. For one, it wouldn't be there for a new session. Second,
for a persisted session_id, it would seem that unless the session store is asked to extract this from the
unpacked_cookie_data, the session id from the rack env would be an id for a session that hasn't been verified against
the digest. Which is a bit concerning, since apparently many are currently using request.session_options[:id] which I believe would be the same situation.

Anyway, this would still prevent a session load for other keys when there is no active session. Of course, this should
only occur when the site is initially accessed or the session cookie is deleted/disabled. FWIW, the call to the
csrf_meta_tags helper call is going to write that token in the session. So unless that's removed, you're going to get
at least one write op that will force a session load. Of course this happens too late to help with accessing
session[:session_id] in a controller or other view. Honestly I'm not sure if I see a great benefit in distinguishing
between load_for_read/load_for_write, as there's probably very few requests that wouldn't perform at least this one
write to the session.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment