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

Req.flash() automatic set session when it's not required to do so. #33

Open
ducdigital opened this issue May 4, 2016 · 5 comments
Open

Comments

@ducdigital
Copy link

When in read-only scenario. Which flash is not yet existed. running req.flash('name') will set req.session.flash to {} which will prevent caching.

Solution:

  • Make sure req.session.flash is set only when req.flash() have 2 args.
  • Make sure fetching flash data return empty array if req.session.flash does not exists
@brendonparker
Copy link

Here's a slightly more elegant solution:

module.exports = function flash(options) {
    options = options || {};
    var safe = (options.unsafe === undefined) ? true : !options.unsafe;

    return function (req, res, next) {
        if (req.flash && safe) { return next(); }
        req.flash = _flash;
        // BEGIN ADD
        var _end = res.end;
        res.end = function() {
            if (Object.keys(req.session.flash || {}) == 0) {
                delete req.session.flash;
            }
            _end.apply(this, arguments);
        };
        // END ADD
        next();
    }
}

This proxies the end response and removes the flash object from the session if it doesn't have any keys. A little less juggling than @ducdigital had in his PR

simllll referenced this issue in simllll/connect-flash Oct 21, 2016
@jmar777
Copy link

jmar777 commented Apr 4, 2017

We needed to patch connect-flash for similar reasons. Here's a quick one-line addition that should do the trick:

    function _flash(type, msg) {
      if (this.session === undefined) throw Error('req.flash() requires sessions');
+     if (arguments.length < 2 && !this.session.flash) return [];
      var msgs = this.session.flash = this.session.flash || {};
      ...
    }

@manuel-di-iorio
Copy link

Updates on this issue?

@pravinkalekar
Copy link

Hi,
I am using Session in conjunction with PassportJS with failureFlash set to true, which internally uses this connect-flash module.
I did not want the uninitialized sessions to be persisted in my session store, so I have my session setup with saveUninitialized set to false.
When I try to read any errors - req.flash('error'), connect-flash is adding an empty flash object to the session, which will be treated as a modification to the session, causing it to be saved.

A similar issue existed in Passport JS itself wherein it used to add an empty Passport object to the session for use after a user is authenticated, which was later fixed. I think that the session should not be touched until a value is stored in the flash area.

@lukaromih
Copy link

This issue really could get more attention. It's causing all those bot requests to create empty sessions as well as sessions (+cookies) for every user, even before they consent to any.
Setting saveUninitialized to false was a whole reason to prevent this behavior, but there's no way around not setting uninitialized sessions with this issue unresolved other than disabling flash altogether. I noticed there was 1 or 2 pull requests regarding this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants