Regenerating the session id before logging in the user

[lukaszmakuch]

Hello everyone!

This suggested change is about regenerating the session id before putting there the user data. I find it very helpful as it frees the developer from the need of taking care of how the user data is stored in the session.

This significantly increases the level of security of apps using passport used by different people on the same device, like in a library on an internet cafe.

Unfortunately it has some disadvantages. There's a risk that some code may depend on the fact the session id doesn't change, or that some session data will be preserved after logging in. As far as I know, regenerating the session without losing the data is currently impossible with express-session.

I haven't checked all of the available strategies, so there's a risk that this change breaks some. Taking that and the previous paragraph into account I'd suggest mentioning this in the documentation or version number if it were merged.

I hope this could make an average node web app a little bit more secure.

I'm looking forward to hearing from you.

[lukaszmakuch]

I've just noticed Travis checks failed due to the usage of a Proxy in test cases. Its support was added in node 6.9, which of course is newer than the versions passport is supposed to run at. I use it to create a test double of the session object. Does anyone have an idea how to solve it in a nice way?

[coveralls]

Coverage increased (+0.3%) to 99.698% when pulling 794ac0a on lukaszmakuch:master into 8de1c66 on jaredhanson:master.

[lukaszmakuch]

Trying to find a way to make tests run even on old node versions, I changed the session test double in a way that instead of using Proxy it uses getters and setters. It's not so nice and versatile as the Proxy approach, but works well on even on ancient javascript.

[jeffwilcox]

Would you add a configuration option or feature flag to let Passport users opt in or out of the regenerate behavior? Love the progress on keeping users more secure.

[coveralls]

Coverage increased (+0.3%) to 99.702% when pulling 6e9c04e on lukaszmakuch:master into 8de1c66 on jaredhanson:master.

[lukaszmakuch]

Now it's possible to provide the following option

{ regenerate_session: false }

what will prevent the session from being regenerated.

[lukaszmakuch]

@jeffwilcox how do you think, is true a good default value, or it should rather be false for backward compatibility? Then it could be explained in the documentation why switching to true is a good idea.

Or maybe the whole thing with regenerating the id should be implemented as another middleware?

[rwky]

@lukaszmakuch if you create a PR against https://github.com/passport-next/passport this will get looked into.