Race condition in logout function

[chr15m]

I encountered an intermittent issue in my app where two sessions were unexpectedly created for the same user between log in and log out. I wrote a test script to repeatedly execute the log in and log out code over and over so I could catch it reliably. Upon examining the most recent release's change I think a potential race condition has been missed in the 0.6.0 upgrade.

The source of the potential race condition can be found here: https://github.com/jaredhanson/passport/blob/master/lib/sessionmanager.js#L87-L90

The callback is called immediately after merge(req.session, prevSession); but in this case req.session may not yet be saved when the callback is called.

Fix

Add an additional req.session.save() call for the case where keepSessionInfo is true. The other code path can run the cb(); straight away as it currently does when there is no change to req.session.

Happy to provide a PR.

Expected behavior

If keepSessionInfo is set on the login and logout function, and a random number is persisted to the session, then you repeatedly log in and log out over and over again, you would expect only one session to exist with the random number you persisted.

Actual behavior

With an async database backend (such as postgres) and after logging in and out hundreds of times, one can observe the situation where a session is sometimes duplicated.

Steps to reproduce

Use an async database backend, persist a random number to a session, then log in and out many times. Eventually two sessions will be created that have the same random number in their session data.

Environment

• Operating System: Ubuntu Linux
• Node version: 16
• passport version: 0.6.0
• Sessions database backend: Postgres

[BaileyFirman]

Hi @chr15m I've recently experienced the same issue after a v0.6 upgrade and have been trying and failing to reproduce (using redis as a store). Are you able to confirm if the additional save that needs to happen after merge(req.session, prevSession); should have the cb passed as a parameter or just if leaving it as is on L90. Thank you.

[chr15m]

@BaileyFirman I believe the whole keepSessionInfo check block should look like this:

if (options.keepSessionInfo) {
  merge(req.session, prevSession);
  req.session.save(function(err) {
    if (err) {
      return cb(err);
    }
    cb();
  });
} else {
  cb();
}

This code is working on my stack but I have not run any comprehensive tests. I will issue a PR now.

[BaileyFirman]

@chr15m Thank you for the fast reply, and the PR. Hopefully it won't be too long before it's merged in and available on npm.

[BaileyFirman]

Updating to confirm that when we compare our environments with and without this fix applied, we can see the issue is resolved. I am using an internal fork but I'm happy to make a public fork if this isn't addressed soon and there is demand for one.

[chr15m]

@BaileyFirman thanks for testing the patch. I'm personally happy to wait for Jared to review, merge and release. I imagine he is incredibly busy and maintaining this repo is a thankless task.

This package is installed half a million times each month and yet only a handful of people sponsor Jared. It would be great if he was sponsored by more of the many commercial entities who depend on this code in production.

Which reminds me, as an individual, thank you @jaredhanson for creating and maintaining this repo! :pray:

[BaileyFirman]

Hi @chr15m just wanted to report back that we did see the issue again these last couple days although it did seem to be reduced substantially after applying your fix. I think any remaining issues may be a result of how we are handling destroying sessions as it relates to some of our logout flows. I will add any updates if I find them to be relevant to your PR or passport.

[BaileyFirman]

Following up on this thread after some time in case the information is still relevant for you @chr15m We continued to be unable to resolve our session conflicts for some time and had to stay on the latest non keepSessionInfo version. I had some more time to investigate and found wrapping logIn/logOut in my project with promises resolved the bulk of the issues:

const reqWrapper = async (logFunction, callback) => {
    let resolveResult = [];

    await logFunction(async (e) => {
        try {
            const result = await callback(e);
            // Errors within passport logIn are not thrown,
            // so we pass any error to the calling next if it exists.
            resolveResult = ([result, e]);
        } catch (error) {
            // If callback throws an error independently of passport logIn,
            // we pass it instead of e the to the calling next.
            resolveResult = ([null, error]);
        }
    });

    return resolveResult;
};

const reqLogInAsync = async (req, user, cb) => reqWrapper(
    async (callback) => await req.logIn({ user, options: { keepSessionInfo: true }, callback }),
    cb,
);

const reqLogOutAsync = async (req, cb) => reqWrapper(
    async (callback) => await req.logOut({ options: { keepSessionInfo: true }, callback }),
    cb,
);

This is clunky but it resolved 99% of our problems, but we would still get 1-3 instances of a session error per day. I vendored in passport and applied your patch https://github.com/jaredhanson/passport/pull/1005/files, this resolved the last remaining issues we had. I haven't determined if moving away from passport is a serious option for me yet, so I have taken some time to do some serious refactoring which initially stated out as refactoring passport.logIn/logOut and dependant functions to return promises like below e.g.

  async logIn({ req, user, options }) {
    if (!req.session) {
      return new Error(
        'Login sessions require session support. Did you forget to use `express-session` middleware?',
      );
    }

    const prevSession = req.session;

    const regenerateError = await new Promise((resolve) => {
      req.session.regenerate((err) => resolve(err));
    });

    if (regenerateError) {
      return regenerateError;
    }

    const serializeError = await new Promise((resolve) => {
      this._serializeUser(
        user,
        req,
        /** @type {(err: Error, obj: any) => void} */
        (err, obj) => {
          if (err) {
            return resolve(err);
          }

          if (options.keepSessionInfo) {
            merge(req.session, prevSession);
          }

          if (!req.session[this._key]) {
            req.session[this._key] = {};
          }

          req.session[this._key].user = obj;

          resolve(undefined);
        });
    });

    if (serializeError) {
      return serializeError;
    }

    return await new Promise((resolve) => {
      req.session.save((err) => resolve(err));
    });
  }

This has evolved into a full refactor, it has some slight but hopefully non-breaking tweaks to satisfy typing etc. My hope is to get the go-ahead to productionise these changes, starting with a company fork, or perhaps getting at least the minimum changes such at your patch merged into upstream. https://github.com/BaileyFirman/passport/pull/2 JS -> JS + JsDoc refactor https://github.com/BaileyFirman/passport/pull/3 JS + JsDoc -> Ts refactor

[chr15m]

Thanks for the update, that's very interesting.

I wonder if introducing promises added subtle delays which are covering up more timing or order of execution bugs hiding in there. 🤔 I'm glad the changes got rid of the issues for you!

[BaileyFirman]

Possibly, I tried a thread sleep that polled waiting for the callback from req.logIn to finish before continues execution (just hacky polling with a while) and it tended to wait up to 40ms which matched the redis calls in our logging, if I had to guess there's probably a magic number where if getting a session takes too long it all falls apart without promises.

[chr15m]

Interesting, and maybe it's not delays that promises introduce but simply enforcing the order of execution.

[jpstone]

So, is this gonna get merged or what?