Race condition on Passport Strategy when using express-session with custom store

[pragmaticivan]

Describe the bug
Using express-session the passport strategy is trying to associate value in the session in memory.
Right after that, it performs a redirect, which doesn't wait for the session to be updated in the store. Causing sometimes to the expected value after the redirect not be available in the store yet.

The express session, for instance, calls .save automatically when the request is finished. But recommends calling .save before redirecting to a different resource. Eg.: https://github.com/expressjs/session#sessionsavecallback

To Reproduce
It's challenging to reproduce that, because the store connection needs to be experiencing some latency.
I was able to do that by using with postgres + knex as store.

Expected behaviour
A clear and concise description of what you expected to happen.

Environment:

• openid-client version: 2.4.5
• node version: v8.11.2

Additional context
Add any other context about the problem here.

• the bug is happening on latest openid-client too.
• i have searched the issues tracker on github for similar issues and couldn't find anything related.

[pragmaticivan]

Related: expressjs/session#360

[panva]

Hi @pragmaticivan,

a quick search in passport shows this is a problem with express-session or passport itself and not each individual strategy.

For passport:

• Ensure session store complete before redirect jaredhanson/passport#477
• req.login redirect is done before the user info is saved in the session jaredhanson/passport#401
• Fix premature redirect when used with express-session jaredhanson/passport#680

The last one suggests there's a fork of passport that handles this, while I don't endorse that it might be something to look into for you, but you should voice your wish for this to be fixed in passport regardless.

For express-session, the code suggests that it delays the end of the request until session has been saved.

I am unsure as to what you expect to happen in a Strategy (or the hundreds of strategies) implementations. It seems, ultimately, express-session should ensure that the session is saved before completing the request.

Edit: I see you've done your research. Sadly i'm not keen to including specific library workarounds in a strategy, this ought to be solved on passport/express-sessions level.

[pragmaticivan]

Hi @panva Thank you very much for your really detailed answer, I really appreciate the time you took providing such a great project.

My concern is for anyone that uses Passport.js based on express-session (which seems to be standard).

In their doc, they mention that for redirect, you should be performing the .save. I know it should have been solved on express-session level, with its own redirect implementation like they did with res.end, but it seems to be a dangerous bug for production software.

I think it should be solved on express-session package, since passport implements session the same way it is implemented int he strategies.

[panva]

I understand, but passport doesn't force any one session package, neither does the strategy. So I am unsure how I can help.

This issue is the first thing https://github.com/passport-next/passport solved, by explicitly calling save before redirect happens.

That being said, I wasn't aware of this and want to thank you for bringing it up.

[zerbfra]

Hi, same issue here.
We have a workaround, since we use it in production and sometimes, with a redis store, the session is not being saved.
This is the workaround: zerbfra@2fdabf2

@panva are you sure to not implement a fix for this issue? Eventually, can you add an hook to the autenticate middleware in order to save the session in our web application code?