jaredhanson/passport

Sessions do not always have regenerate() and save() cauing a fault

Ylianst opened this issue ยท 19 comments

I am the author of MeshCentral and use cookie-session along with Password. A recent commit in Password is causing crashes because cookie-session does not have a regenerate() or save() method.

See this issue: expressjs/cookie-session#166

Could Passport revert to the old style of setting session values unless these methods exist? Thanks.

Thanks for the report. My intent here is to make the session manager pluggable (it is already by setting passport._sm, but will add a public method in a future release). This would allow session management to be plugged in similarly to strategies, which would allow targeting different session implementations (such as cookie-session).

I'll look into implementing a cookie-session-based session manager. For now, I'd suggest pinning to the 0.5.x release of Passport.

Thanks. I am pinning to 0.5.x until then! Much appreciated.

We are also facing issues with the regenerate call happneing "automatically" during login and logout, would it be an option to add an option to disable this behaviour?

@simllll What jaredhanson proposed above would allow you to do that, by providing a SessionManager that does not call regenerate.

Hi, I was wondering if there are any updates on this issue, we need to upgrade to 0.6.0 due to snyk finding, however we can't due to this issue
image

Also currently facing this error.

@simllll What jaredhanson proposed above would allow you to do that, by providing a SessionManager that does not call regenerate.

@simllll What jaredhanson proposed above would allow you to do that, by providing a SessionManager that does not call regenerate.

What is an example of a session manager that does not call regenerate?

I think one of the main ones is cookie-session. They have also filed an issue to figure out next steps depending on what passport decides to do. expressjs/cookie-session#166

Hello, any update on this issue? The workaround to 0.5.x is vulnerable to CVE-2022-25896. Any plan to to support cookie-session-like session manager? It is listed in express's official docs for session management. It would great if both strategies are supported. Otherwise, a persistent storage is required to use passport.js for session management.

Can anyone suggest an alternative for this passport library? There doesn't seem to be any effort to fix this.

My intent here is to make the session manager pluggable (it is already by setting passport._sm, but will add a public method in a future release). This would allow session management to be plugged in similarly to strategies, which would allow targeting different session implementations (such as cookie-session).

We're looking for ways to mitigate the CVE while the ecosystem adopts passport 0.6, has the public method to set SessionManager been released?

Would be nice to have this fixed so that the lib can be updated to 0.6.x+ which includes a fix for security vulnerability with moderate severity

I faced the problem that my session was overwritten after the passport.authenticate method.
This is how I solved it:

function authenticate(req, res, next) {
  return passport.authenticate('local', (err, user) => {
    // Copy Initial Session Data
    const { flashedData, returnToUrl } = req.session;

    if (err) {
      return next(err);
    }

    if (!user) {
      const flashData = {
        status: 'error',
        message: 'Authentication failed!',
      };

      return flashDataToSession(req, flashData, () => {
        res.redirect('/auth/login');
      });
    }

    return req.login(user, loginErr => {
      if (loginErr) {
        return next(loginErr);
      }

      // Keep initial Session data
      req.session.flashedData = flashedData;
      req.session.returnToUrl = returnToUrl;

      return req.session.save(() => next());
    });
  })(req, res, next);
}

We have faced the same issue in our project and solved it with a non-intrusive workaround until it is fixed officially.
As the error message indicates, the regenerate function is missing. Subsequently, the save method is missing too. Therefore, we provide a dummy implementation of those two functions.

This workaround allowed us to upgrade to the newest version and therefore get rid of the security vulnerability CVE-2022-25896

Posting it here, maybe it helps somebody

app.use(cookieSession({
    // ...
}))
// register regenerate & save after the cookieSession middleware initialization
app.use(function(request, response, next) {
    if (request.session && !request.session.regenerate) {
        request.session.regenerate = (cb) => {
            cb()
        }
    }
    if (request.session && !request.session.save) {
        request.session.save = (cb) => {
            cb()
        }
    }
    next()
})

We have faced the same issue in our project and solved it with a non-intrusive workaround until it is fixed officially. As the error message indicates, the regenerate function is missing. Subsequently, the save method is missing too. Therefore, we provide a dummy implementation of those two functions.

This workaround allowed us to upgrade to the newest version and therefore get rid of the security vulnerability CVE-2022-25896

Posting it here, maybe it helps somebody

app.use(cookieSession({
    // ...
}))
// register regenerate & save after the cookieSession middleware initialization
app.use(function(request, response, next) {
    if (request.session && !request.session.regenerate) {
        request.session.regenerate = (cb) => {
            cb()
        }
    }
    if (request.session && !request.session.save) {
        request.session.save = (cb) => {
            cb()
        }
    }
    next()
})

Thanks for that. It has been working great.

Hi is this bug fixed?

I'm facing this issue with the MongoStore, the downgrade to 0.5.3 is working but I would like to upgrade to the latest... any news?

Hello , any news on this fix ???

this works for me thank you so much