Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Ylianst opened this issue May 23, 2022 · 19 comments
Open

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

Ylianst opened this issue May 23, 2022 · 19 comments
Labels
bug Something isn't working enhancement New feature or request

Comments

@Ylianst
Copy link

Ylianst commented May 23, 2022

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.

@jaredhanson
Copy link
Owner

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 jaredhanson added bug Something isn't working enhancement New feature or request labels May 23, 2022
@Ylianst
Copy link
Author

Ylianst commented May 24, 2022

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

@simllll
Copy link

simllll commented May 30, 2022

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?

@soren121
Copy link

soren121 commented Jun 2, 2022

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

@zsepton
Copy link

zsepton commented Jul 15, 2022

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

DafyddLlyr added a commit to theopensystemslab/planx-new that referenced this issue Jul 23, 2022
- Also downgrade Passport from 0.6.0 -> 0.5.0
- Incompatability between the two - see details here jaredhanson/passport#904
@ICEPrey
Copy link

ICEPrey commented Jul 28, 2022

Also currently facing this error.

@alexshenker
Copy link

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

@MostlyFocusedMike
Copy link

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

@mingchia-andy-liu
Copy link

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.

@mtrezza
Copy link

mtrezza commented Sep 25, 2022

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

aragilar added a commit to FAIMS/FAIMS3-conductor that referenced this issue Oct 4, 2022
This reverts commit 13ced8b. This is
needed as 0.6.0 is broken, and needs a fix to how session handling
works. This is covered in
jaredhanson/passport#904

Signed-off-by: James Tocknell <aragilar@gmail.com>
bors bot added a commit to FAIMS/FAIMS3-conductor that referenced this issue Oct 4, 2022
246: Revert "Update passport to 0.6.0" r=aragilar a=aragilar

This reverts commit 13ced8b. This is needed as 0.6.0 is broken, and needs a fix to how session handling works. This is covered in
jaredhanson/passport#904

Co-authored-by: James Tocknell <aragilar@gmail.com>
@spicycode
Copy link

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?

@mannyvergel
Copy link

mannyvergel commented Nov 2, 2022

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

@ThomasCode92
Copy link

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

@philippkrauss
Copy link

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

@Yuri-Lima
Copy link

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.

@bernardo-rebelo
Copy link

Hi is this bug fixed?

YasharF added a commit to sahat/hackathon-starter that referenced this issue Jul 13, 2023
patch-package allows seamless patching of npm modules (dependencies) without having to fork them.  You can modify the files in node_modules and use patch-package to capture the changes and reapply them each time the module is installed.
passport.js 0.6.0 currently has a bug that breaks the logout function.  The maintainer hasn't had bandwidth to fix it, but there is a pull request in github that addresses the issue.  The patch here applies the changes from the pull request to 0.6.0.
Passport bug: jaredhanson/passport#904
Passport Fix/Pull Request: jaredhanson/passport#947
@morphy76
Copy link

morphy76 commented Feb 1, 2024

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?

@gabisinhas
Copy link

Hello , any news on this fix ???

@wayneleon1
Copy link

this works for me thank you so much

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests