-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
It could be nice of passport.js not to litter in the session #279
Comments
I am facing this too. Any chance this could be fixed :) |
Wow no kidding. I just looked at my sessions 73,000 unused sessions. |
This issue breaks the |
+99 👍 |
@iliakan I tried creating a middleware function to do the same thing (since I don't want to fork passport at the moment) using the code in your PR. It works wonderfully, except when I'm trying to get a user logged in. It seems to always clean up the session object because it's always empty. Any suggestions?
EDIT: The problem was that it deleted |
That's what I'm using:
P.S. That's for Koa.JS |
Here's my version: var onHeaders = require('on-headers');
app.use(expressSession(...));
app.use(function cleanupPassportSession(req, res, next)
{
// hook me in right AFTER express-session
onHeaders(res, function()
{
if (Object.keys(req.session.passport).length === 0)
{
delete req.session.passport;
}
});
next();
}); on-headers is what express-session uses. Using this method the passport key will be cleared right before express-session determines if it should write a cookie (aka as late as possible). |
Nice, I wish i noticed this thread before... I changed passport to not initialize the session at all here but I like your solution until something gets pulled into the official package... thank you, |
I packaged up my solution to this problem, it is a hack, but it does the trick: https://github.com/wesleytodd/express-session-passport-cleanup I will maintain this at least until this gets merged if anyone wants to use it. It is similar to what @Joris-van-der-Wel did above, but using the |
@jaredhanson Any thoughts on this? |
I've fixed this slightly differently, by checking for empty objects in express before setting the cookie. inside index.js in express-session I've added the following check to the onHeaders handler:
|
The main benefit of the package I linked to is that it doesn't require any modifications to the other packages and you can just delete the module when it is fixed without any other changes. But the real solution is here, in this package. Anyone have an ETA on this? |
Fixed by merging #320. Published to npm as passport 0.3.0. |
In
initialize.js
:The problem is: it writes to
req.session
even if there's nothing to write.For projects that don't write empty sessions to DB, that hurts a little bit, cause passport.js makes sesson 'dirty' even if there's nothing to write in it.
Right now I workaround this by a one more middleware wrapper around
passport.initialize
, which checks for an emptysession.passport
and deletes it.But it could be nice of passport.js to remove that empty session.passport as it is actually a litter in the session.
P.S. Added PR
The text was updated successfully, but these errors were encountered: