passport icon indicating copy to clipboard operation
passport copied to clipboard

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

Open Ylianst opened this issue 3 years ago • 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: https://github.com/expressjs/cookie-session/issues/166

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

Ylianst avatar May 23 '22 19:05 Ylianst

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.

jaredhanson avatar May 23 '22 20:05 jaredhanson

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

Ylianst avatar May 24 '22 00:05 Ylianst

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 avatar May 30 '22 21:05 simllll

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

soren121 avatar Jun 02 '22 23:06 soren121

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

zsepton avatar Jul 15 '22 12:07 zsepton

Also currently facing this error.

MohammedADev avatar Jul 28 '22 18:07 MohammedADev

@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?

alexshenker avatar Jul 31 '22 15:07 alexshenker

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. https://github.com/expressjs/cookie-session/issues/166

MostlyFocusedMike avatar Aug 01 '22 14:08 MostlyFocusedMike

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.

mingchia-andy-liu avatar Sep 25 '22 03:09 mingchia-andy-liu

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

mtrezza avatar Sep 25 '22 11:09 mtrezza

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?

spicycode avatar Oct 20 '22 13:10 spicycode

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

mannyvergel avatar Nov 02 '22 04:11 mannyvergel

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);
}

ThomasCode92 avatar Nov 04 '22 20:11 ThomasCode92

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()
})

philippkrauss avatar Nov 08 '22 17:11 philippkrauss

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.

Yuri-Lima avatar Dec 19 '22 19:12 Yuri-Lima

Hi is this bug fixed?

bernardo-rebelo avatar Apr 06 '23 19:04 bernardo-rebelo

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?

morphy76 avatar Feb 01 '24 16:02 morphy76

Hello , any news on this fix ???

gabisinhas avatar Mar 08 '24 20:03 gabisinhas