Race condition for express-session with external store

[pragmaticivan]

Using express-session passport is trying to associate value in the session by key instead of using req.session.save(cb). If there is latency on saving the session, the cb is called right after may happen before it is persisted and deserialized.

Any session store/delete should be used with express-session callback.

It mainly happens in this part of the code: https://github.com/jaredhanson/passport/blob/master/lib/sessionmanager.js#L25

Expected behavior

cb function should be called after the session is stored in store (db, redix, etc..)

Actual behavior

Sometimes, if there is latency between the application and the store (db), the redirect happens before the data is persisted in the db.

Steps to reproduce

I tried using multiple stores and dbs, and was able to get the same issue.

it can be reproduced by using express-session + external store (preferably a database)

Environment

• Operating System: Ubuntu | Mac OS | Docker Container
• Node version: v8.11.2
• passport version: 0.4.0

[pragmaticivan]

FYI, already tested it, it can be fixed by replacing the cb() line with:

if (typeof req.session.save == 'function') {
      req.session.save(function () {
        if (err) {
          cb(err);
        }
        cb();
      });
} else {
      cb();
}

Let me know if this is ok, I can send a PR. That seems to be a serious issue.

[pragmaticivan]

ping @jaredhanson

[pragmaticivan]

For instance, express-session only saves the data on res.end. Redirects are not included, and they recommend saving the session for redirect callbacks. https://github.com/expressjs/session/blob/master/index.js#L250

[pragmaticivan]

Related: expressjs/session#360

[unknowndomain]

@pragmaticivan This looks like you are making progress, are you planning to turn this into a pull request?

@jaredhanson is there a fix for this issue because right now using express, express-session, and connect-pg-simple as the session store (all latest versions) passport won't let me login from any variation of Chrome while doing local development because of this race condition, but Firefox and Safari are fine, the answer seems to lay in this: https://github.com/expressjs/session#sessionsavecallback

Which basically says you need to save your sessions yourself.

[pragmaticivan]

I ended up implementing my own changes for the project. The next one I ended up using Koa2 which has async support out of the box, and it's working like a charm.

cc #704

[domschab23]

Great job! I confirm that the above fix works

[mrahma04]

I'm a little confused regarding the fix. This is my code. How would I refactor this (near the bottom) to incorporate the fix?

router.post('/login', async (req, res) => {
    try {
        User.findOne({
            where: {
                username: req.body.username
            }
        }).then(user => {
            // console.log(user)
            if (!user) {
                req.status(400).json({ message: 'Username not found!' })
                return
            }
            // check password input in the req.body
            // against password stored in database...user.dataValues.password
            const validPassword = user.checkPassword(req.body.password, user.dataValues.password)
            if (!validPassword) {
                res.status(400).json({ message: 'Incorrect password!' })
                return
            }

            req.session.save(() => {
                req.session.user_id = user.id
                req.session.username = user.username
                req.session.loggedIn = true

                res.json({ user: user, message: 'You are now logged in!' })
                // res.status(200).redirect('dashboard')
            })
        })

    } catch (err) {
        console.error(err)
        res.status(500).json(err)
    }
})

[antishok]

@mrahma04 The issue is only relevant when doing a redirect.
And, you need to save the session only after making the changes to it.
In your current code you are saving it, and only then making changes to it.
Should be instead something like:

req.session.user_id = user.id;
req.session.username = user.username;
req.session.loggedIn = true;
req.session.save(() => {
  res.redirect('foo');
});

[mrahma04]

Moving the req.session parameters outside of the callback did the trick.

Here's my refactored code. Thank you so much!

req.session.user_id = user.id
req.session.username = user.username
req.session.loggedIn = true
req.session.save(() => {
    res.json({ user: user, message: 'You are now logged in!' })            
})