Don't Store Blank Session Keys in Redis

[mstruve]

When doing some testing on our application I found that we were storing a lot of blank session keys in Redis. This seemed like a waste of resources so I updated this method to skip storing the blank sessions.

There was not a specific test written for this method but I am open to writing one if you think it is needed. Thanks!

[tubbo]

Hey @mstruve, sorry it took me a little bit to review this. Refraining from adding blank session data to Redis seems like a good idea to me, but I would definitely like to see a test proving that it works. I'm also curious why you chose to make the change in #generate_unique_sid rather than #set_session if you're trying to prevent blank session keys from being stored in Redis?

[mstruve]

Hey @tubbo, honestly I didn't even look at #set_session. Once I found that #generate_unique_sid was what was putting the session into Redis I focused on that method and since it only took a simple guard to prevent it, I went with that.

[tubbo]

@mstruve that might not always work, since #setnx is only going to set the value if the key doesn't already exist. I think the reason for that code is to verify that one is getting a unique session ID, since the only real way to know that is to query Redis and see if the key was already written to the DB. According to the Rack::Session::Abstract::ID API, which is what Rack::Session::Redis is based on, the methods in use to actually read and write to the session store are #get_session and #set_session, so if you're preventing one storing a blank session, I would move the guard clause to the top of #set_session.

[mstruve]

Ok, I went back to the drawing board and tried to move the return statement to set_session but found that it never worked and here is what I think is happening. Before a session gets written there is a guard clause in Abstract::ID that calls commit_session? which actually does a check to see if the session is empty and I think since it is that returns false and we never hit set_session. However when we try to go and fetch the session we hit generate_unique_sid which since it cant find the session will write it.

[tubbo]

@mstruve That makes a lot of sense to me, thank you for diving into this and doing some detective work, because this API (as well as redis-rack's implementation of it) is definitely a little confusing. This all looks great to me, so I'm gonna merge to 'master’ branch. Can you make sure it works by installing on your app like so?

gem 'redis-rack', github: 'redis-store/redis-rack', branch: 'master'

[mstruve]

Pointed at master and it works like a charm! Thanks @tubbo!

[tubbo]

glad to hear it! thanks for your contribution, I’ll release in a few.

[mstruve]

Any thoughts on when a version bump will be released? Thanks!

[tubbo]

Sorry about that @mstruve, this totally fell off my radar. I just released it as redis-rack v2.0.5, give it a whirl! :)

…

On Nov 8, 2018, at 4:46 PM, Molly Struve ***@***.***> wrote: Any thoughts on when a version bump will be released? Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#37 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAG5gtQQPYMKik_xi1t9CCG6h8zcLgbBks5utKZEgaJpZM4W046_>.

[tubbo]

@mstruve Did you notice any kind of session ID collisions as a result of this change? Other users are reporting a potential collision in certain scenarios as a result of this change (since no one ever actually gets to the loop { ... } part due to the fact that session is always initialized as {}). Honestly I don't see why we even have this #generate_unique_sid method in the first place...thinking people can just use a better randomized value for the session ID so collisions are almost impossible...

[mstruve]

Interesting, we never ran into that issue.