Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

session cookie expiration problem #670

Closed
bvarga opened this Issue · 26 comments
@bvarga

In this issue there's a sentence

Imagine a situation where you want a users session to expire, if there is no activity for
1 hour from that user anymore. You go and set your cookie maxAge to an hour. The
current system will do this, if activity is defined as http requests

I think now it's not how it works, or I don't know how to make it work like that. In my installation, the cookie expiration in the browser never touched, here's a little code

app.use(express.cookieParser());
    app.use(express.session({ 
      secret: 'secret stuff',
      store: sessionStore, // redis one
      cookie: { 
        secure: true,
        maxAge: 60 * 1000 //1 minute
      }
    }));

The user cookie set once, I can perform a get within 1 minute any time, the cookie expiration won't be updated in the browser, so after 1 minute the first visit, the cookie expires, the corresponding session file becomes orphaned and the user is "logged out".

@tj
Owner
tj commented

with the current implementation now that we no longer set-cookie each time the sessions are no longer "rolling" sessions that keep up with user activity. you can do this many different ways, for example set the session to a very large max-age, then store a date in redis or similar, and check that at an interval, as far as a generic way of doing this im not sure yet, we maybe to add some options to session()

@lpinca

I like the idea of having an option for "rolling" sessions.
With the current implementation the session expiry time gets updated on the session store, but the corresponding cookie in the browser doesn't update.
This is inconsistent and may lead to orphaned sessions in the session store.
What about adding a simple check? Something like this else if (!rolling && originalHash == hash(req.session)) {...

@ryedin

OK, don't take this the wrong way, but um... OMFG (and I only say that because I just got done troubleshooting and fixing this in our app, having gone quite a while assuming connect handled this in a "rolling" manner automatically). This is our fault for assuming the cookie was being updated every request, but I'm pretty sure almost 100% (maybe even 110%) of all people who use a session module in any server environment expect that behavior (maybe I'm really really wrong in that, though).

We finally figured it out, and it was a 1-line fix for us to just add a 'session.noop = +new Date;' in a heartbeat request to keep sessions alive, but man is that counter-intuitive to have to do that - and it wasted a couple hours of our time to figure it out. Not complaining here, just explaining what our intuition in this regard cost us (relatively minor in the grand scheme of things but possibly something you'd want to be aware of as project maintainer).

THAT SAID, connect rocks and we thank you for this great module!

So this behavior of only sending the Set-Cookie header when the session is modified, why did you decide that should be how it works? Am I completely broken, having assumed the opposite?

@tj
Owner
tj commented

@ryedin I dont disagree, it goes both ways. If you set-cookie each request you get a lot of (valid) complaints about setting a cookie each time. Which is definitely wasteful, there's no question about that, but only setting the cookie on changes is a gotcha as well. I haven't decided on a reasonable approach so far, even if were were to do it say every 15th request, or at some interval, there's still no guarantee that it happens.

@ryedin

Good points. I can see people wanting it either way. So if that's true, why not follow what @lpinca suggested and make it configurable per store instance? I think that's very reasonable.

Perhaps it's something you're just waiting on a PR for?

@tj
Owner
tj commented

I'd rather come up with an acceptable solution for both, the session middleware is already pretty large, but if we can't come up with something that works for both cases then yeah maybe. We still need to hash the req.session.cookie as part of the hash, currently we're only hashing the session for changes, but once we have that you can do req.session.touch() to reset the maxage for a rolling session, easy to apply that in a two-line middleware

@ryedin

ok, yeah, that would have the same effect as our little noop middleware heartbeat thingie; having req.session.touch() trigger the Set-Cookie header being sent would be more elegant.

@natesilva

I too would like to see the option for “rolling” sessions. Using req.session.touch() would work fine for me. Currently we’re using an incrementing counter similar to the no-op workaround ryedin described.

@mzabaluev

What should be the expected behavior? I see three cases:

  1. Non-persistent sessions (expires == null). The cookie is pushed once to the browser, the session record is updated with a rolling expiration timestamp in the store with each request, using a reasonable expiration interval (connect-redis uses a ttl option defaulting to one day). This works as expected, except for bug #568 resulting in unnecessary cookie pushes whenever session data change.

  2. Persistent, rolling sessions. The cookie should be pushed with each response, its expiration timestamp advanced each time. The session record is also updated with the new expiration date each time. Currently the cookie refresh only works if session data are modified, which is non-intuitive and makes developers use tricks such as req.session.foo = Date.now() (see discussion in bug #568). It should work, as discussed earlier, by calling req.session.touch(), or adding an auto-rolling option.

  3. Persistent sessions with a fixed expiration date. The cookie is pushed only when the expiration timestamp is explicitly changed. The session record in the store only needs to be updated when something changes in the session (see #672). This is currently broken: resetMaxAge is called at the end of any request, causing the expiration timestamp to be advanced in the store (but not necessarily in the browser cookie). Conversely, explicit changes to the req.session.cookie timing parameters do not result in a cookie push to the browser unless other session data also change (bug #568 again).

I suggest introducing the auto-rolling option to sessions, and making it enabled by default to provide a smooth upgrade path from the current behavior. Also fix set-cookie to only update the cookie when its own parameters change, regardless of changes to any stored session data.

@bendiy

Just linking this to #328

@bendiy

I think the chatter can be address with a config option setting an interval for how ofter to update the expires property. I'm currently doing something like this with a 60000 interval.

// Stomping on express/connect's Cookie.prototype to only update the expires property
// once a minute. Otherwise it's hit on every session check. This cuts down on chatter.
require('express/node_modules/connect/lib/middleware/session/cookie').prototype.__defineSetter__("expires", function (date) {
  if (date === null || this._expires === null) {
    // Initialize "this._expires" when creating a new cookie.
    this._expires = date;
    this.originalMaxAge = this.maxAge;
  } else if (date instanceof Date) {
    // This captures a certain "set" call we are interested in.
    var expiresDate;

    if (typeof this._expires === 'string') {
      expiresDate = new Date(this._expires);
    }

    if (this._expires instanceof Date) {
      expiresDate = this._expires;
    }

    // If the difference between the new time, "date", and the old time, "this._expires",
    // is more than 1 minute, then we update it which updates the store.
    // OR if they match, we need to update "this._expires" so it's a instanceof Date.
    console.log("updates in: ", 60000 - (date - expiresDate));
    if ((date - expiresDate > 60000) || (JSON.stringify(date) === JSON.stringify(expiresDate))) {
      console.log("expires updated: ", date - expiresDate);
      this._expires = date;
      this.originalMaxAge = this.maxAge;
    }
  }
});

An options setting could be passed into this to allow us to choose something other than 60000 ms.

@zivester

+1. Spent hours banging my head on a wall trying to figure out why my ajax requests were not extending the session. Even put together a testing gist before stumbling upon this bug report https://gist.github.com/zivester/4743563

What exactly is the workaround that doesn't require setting something in req.session? I tried the following, but it doesn't resend the cookie:

app.use(function(req, res, next){
    req.session.touch();
    next();
});
@zaptree

Would it not make sense to set the cookie not to expire (expire in a year for now for example) and just control the session expiration server side? This way you can have rolling sessions by just updating the maxage in the session store (this already happens). This would basically solve the problem of always having to send the set-cookie header plus you would have rolling sessions and everything gets controlled server side. I can't see what the downside of doing this would be.

@bendiy

@zaptree That's basically what I'm doing here: #328 (comment)

@zivester

@zaptree well, thats another level of complexity. It seems wrong to me that maxAge being set is dependent on whether req.session is altered or not. What does changing a req.session value have to do with setting the age of the cookie? They should be independent of each other.

@zaptree

@bendiy so what you do is use the expires to set the length of you session but that does not get set to the cookie and the cookie will be a session cookie. This is an acceptable fix if I want a rolling session that does not persist after I close my browser (which is actually what I want most of the times, so it is a good fix), but I might want a rolling session that would persist if I close my browser and re-open it assuming that the set time has not passed, plus I don't want to have orphaned session in my store. What I really would like is a fix for this to be put in the session middleware which will be standard and not having to override module methods.

@zivester I totally agree with you, I am not sure I get the reason behind that myself.

@bendiy

@zaptree @zivester I totally agree too.

They should be two separate things, but currently are tied together and require hacks like this to get cookies and sessions to work differently.

@tj
Owner
tj commented

I'm happy to decouple the two, I'll try and get this done in the next few nights

@zaptree

That sounds great, thank you very much!

Just wanted to add the fix that I just put in my code for now to test what I was saying, and it seems to work as I assumed sessions should behave. I changed in the session middleware the res.on('header') to this:

    // set-cookie
    res.on('header', function () {
        if (!req.session) return;
        var cookie = req.session.cookie
            , proto = (req.headers['x-forwarded-proto'] || '').split(',')[0].toLowerCase().trim()
            , tls = req.connection.encrypted || (trustProxy && 'https' == proto)
            , secured = cookie.secure && tls
            , isNew = unsignedCookie != req.sessionID;

        // only send secure cookies via https
        if (cookie.secure && !secured) return debug('not secured');

        //we only need to set the cookie once when we first create the session,
        // session expiration gets handled server side
        if (!isNew) return debug('already set cookie');

        //set cookie maxAge to something really long (this does not seem to
        // affect the maxAge for the store since we do it on('header') which
        // runs last. Still, not the cleanest, but cookie.serialize does not
        // let us pass options...
        cookie.maxAge = 1000 * 60 * 60 * 24 * 365;
        var val = 's:' + signature.sign(req.sessionID, secret);
        val = cookie.serialize(key, val);
        debug('set-cookie %s', val);
        res.setHeader('Set-Cookie', val);
    });
@tj
Owner
tj commented

actually it's not too bad how it is now, for example you can pass { ttl: n } to connect-redis, otherwise it falls back on max-age. we could still possibly default the cookie max-age to something more reasonable and prevent set-cookie those configured with a pretty long max-age

@AlexanderHerlan

Sorry to ask this here but I am at my wits end trying to solve this issue:

if(remember == "true") {
    SESS_LENGTH = MONTH * 3; 
}

// tell the session cookie to use our custom session length (influenced by if the user wishes to be remembered or not)
req.session.cookie.expires = new Date(Date.now() + SESS_LENGTH);
req.session.cookie.maxAge = SESS_LENGTH;

What is supposed to happen is: If a user logs in with the 'remember me' checkbox checked, we increase the default session (that I have set in the middleware to 3 hours) to 3 months.

This no longer seems to work for me. It sets the correct TTL on the Redis session key, but does not effect the cookie in any way. What is the proper way to influence the session cookie after it has already been set by the middle-ware declairation?

@zaptree

@AlexanderHerlan there is no way unfortunately and that is the point of this thread. Even though your redis will be updated the actual cookie itself will not be updated so it will expire after whatever time it was set to originally, without ever being able to update the cookie. If you use the solution I put up in the previous post you will be able to get the functionality that you want. It will set the cookie expiration to 1 year (you could set it to longer) and then you will manage the expiration exclusively from the redis store. after 3 months your redis store will have expired so even though the cookie exists in the browser it will no longer be valid. The only problem is that if you are nearing the one year expiration the same problem you had will exist but this will only happen once per year (or however long you set the maxAge in the code above. Also the code I suggest requires you to overwrite the core of the the session middleware which is never a good idea...

@jorgearanda

I may be missing something: what is the point of updating the expiry of a cookie when there is session activity, if the updated cookie is not being sent to the browser?

@geoffb geoffb referenced this issue in NodeBB/NodeBB
Closed

Express session has no maxAge set #201

@valentinkostadinov

What's the point of checking the session hash without the cookie when deciding to "set-cookie"? It's the cookie that gets sent back in the response header, and nothing of the session.

It seems the logic should be the opposite:

  • if cookie alone is unchanged - don't "set-cookie", otherwise, always do.

To avoid wasteful set-cookie headers on every request, make "rolling session" optional. Rolling sessions could check lastAccessed and set-cookie once a minute for example.

@ilmeo

I solved it this way:
in ....connect\lib\middleware\session.js
[...]

function session(options){
  var options = options || {}
  , key = options.key || 'connect.sid'
  , store = options.store || new MemoryStore
  , cookie = options.cookie || {}
  , trustProxy = options.proxy
  , storeReady = true //; OLD
  // NEW!!
  , rollingSessions = options.rolling || false;

[...]
and
[...]

  if (!rollingSessions) { // NEW!!
    // long expires, handle expiry server-side
    if (!isNew && cookie.hasLongExpires) return debug('already set cookie');

    // browser-session length cookie
    if (null == cookie.expires) {
      if (!isNew) return debug('already set browser-session cookie');
    // compare hashes and ids
    } else if (originalHash == hash(req.session) && originalId == req.session.id) {
      return debug('unmodified session');
    }
  }

[...]

And I can simply decide whether to use or not rolling sessions by simply setting:

connect()
  .use(connect.cookieParser())
  .use(connect.session({ secret: 'keyboard cat', key: 'sid', cookie: { secure: true }, rolling : true}))

Does it make sense?

@jonathanong

moving this to expressjs/session#3. let me know if you're interested in contributing.

@mikesoylu mikesoylu referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@cesaregb cesaregb referenced this issue in jaredhanson/passport-twitter
Open

Error: failed to find request token in session #12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.