Cookie changes "revalidate" and "update" are creating new session id every time

[nesro]

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?

[julianlam]

My gut reaction was this was related to session reroll.

Wasn't this fixed by your PR NodeBB/NodeBB#11255?

[nesro]

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

[julianlam]

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...

[julianlam]

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.

[julianlam]

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...

[nesro]

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.

[julianlam]

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.

[julianlam]

v7.1.0