Cookie changes "revalidate" and "update" are creating new session id every time
Closed this issue · 8 comments
Hi all,
please note that this might not be a bug. I've been searching for some best-practices and it's not so easy, so I'm asking here too.
This line:
https://github.com/julianlam/nodebb-plugin-session-sharing/blob/master/library.js#L430
gets executed every time when the "session handling" -> "cookie changes" is set to either "revalidate" or "update". One of the things that's happening is that the user get's a new session with a new sessionId. This is causing us some performance issues.
I wonder if the logic here could change so that if there is no need for new session, it's not created?
My gut reaction was this was related to session reroll.
Wasn't this fixed by your PR NodeBB/NodeBB#11255?
I'm sorry, it seems like the mentioned fix did only half the job.
Passport login function will now always regenerate the session in the logIn function: https://github.com/jaredhanson/passport/blob/master/lib/sessionmanager.js#L28
The parameter keepSessionInfo merges the old data into the new session: https://github.com/jaredhanson/passport/blob/master/lib/sessionmanager.js#L37
Oh yes, that is true. A new session is created regardless, with a new id.
Unfortunately I don't think that is something we can counteract, as it is has always been handled by Passport...
I think at the end of the day, the fact that a new session is created is quite intentional in order to counter session fixation attacks. I cannot advise as to how commonplace session fixation attacks are, although it is definitely considered a vulnerability.
Hm, it could be that .doLogin need not be called on revalidate/update, since you're technically already logged in...
I will take a closer look...
I don't have much time this week, sorry for the delay.
it could be that .doLogin need not be called on revalidate/update, since you're technically already logged in
Yes I agree. I think the best way would be to mix up hasSession and loginLock so that it works just like revalidate/update behaviour are defined.
It seems to work fine — insofar as my limited testing and running of the test suite seems to tell me.
If you have revalidate/update set, it will return early after validating the cookie and ensuring that the uid matches.
v7.1.0