Cookie should expire immediately when session is destroyed

[JamesMGreene]

I believe that the session cookie should be forced to immediately expire when the session is destroyed (i.e. when the user logs out) on the server-side.

While the Session Store I am using is correctly handling this such that a subsequent request from the same client would create a new cookie/session, having the existing cookie for the now-destroyed session be forcibly expired keeps things much cleaner and clearer on the client-side.

Doing so also avoids wasting some bytes on the network bandwidth of every outgoing request from the client by not including the irrelevant cookie. If the session has been destroyed but the cookie's normal expiration date has not yet been reached, this can contribute an undesirable amount of unnecessary upload bytes incurred from the client's outgoing requests due to always being required to include that irrelevant cookie in the Cookie header until it naturally expires.

When combined with my changes in PR #240 to fix the interaction between rolling: true and saveUnitialized: false, fixing this would keep things significantly cleaner on the client-side.

[JamesMGreene]

@dougwilson: Perhaps this should only be the expected behavior if the cookie would've been updated normally anyway, e.g. if the rolling: true option was enabled.

[dougwilson]

I think this idea makes sense. My only thought is if it should be configurable or not. I would assume that no one would be using the session ID from the cookie to mean something after the session gets destroyed, but not 100% sure (crazy hacks at times, I'm sure).

[JamesMGreene]

@dougwilson:

I'm fine proceeding any which way:

1. the state of the current PR where it is just the new default behavior to always update the cookie upon session termination,
2. only update the cookie upon session termination if rolling: true is set, or
3. adding some new configuration option to control whether the cookie is updated upon session termination.

Just advise me of which and I'll happily update the PR accordingly (if not Option 1). Thanks!

[dougwilson]

Hi @JamesMGreene , nothing to change currently. I want option # 1 like you implemented, as I think it makes the most sense. I'm just doing some digging real quick to make sure there is no obvious reason # 1 can't go forward :)

[JamesMGreene]

:+1:

[JamesMGreene]

@dougwilson: Have you given this any further thought yet? No worries if not. :christmas_tree: :snowman:

[dougwilson]

I'm so sorry I've let this hang like this on you. I did look and I didn't see any reason why we won't add this as the default behavior. The only edge case I saw was a user calling destroy and then regenerate in the same request. Just glancing at the changeset, it looks like this should be handled, but if you also wanted to give a look over for that edge case, that'd be awesome.

I'll make sure this gets released in a version Friday or Saturday.

[JamesMGreene]

The only edge case I saw was a user calling destroy and then regenerate in the same request. Just glancing at the changeset, it looks like this should be handled, but if you also wanted to give a look over for that edge case, that'd be awesome.

I'm not necessarily familiar with this case but I'd be happy to look into it if you can maybe point me to an existing test that exercises this functionality and provide some guidance as to the expected behavior in this new scenario.

Don't fret about the delay, your turnaround is still very quick compared to that of my own championed open source projects, and we are on a holiday break at work right now anyway. :smile:

[JamesMGreene]

@dougwilson Ping-a-ling-a-ling. :blush:

I would love to get this change integrated this week, if possible, as we are shipping a new release next week. :shipit: :heart:

[dougwilson]

Hi @JamesMGreene , sorry! So what I was talking about was the .destroy (https://github.com/expressjs/session#sessiondestroy) and the .regenerate (https://github.com/expressjs/session#sessionregenerate) APIs we provide. I haven't gotten around to testing it out myself yet. If you want to try/add a test to the PR for it, that would be awesome!

Essentially the following, given a cookie on the request that represents an existing session, should work correctly by still setting a cookie, but of course it would just have a new ID:

app.use(function (req, res, next) {
  req.session.destroy(function (err) {
    if (err) return next(err)
    req.session.regenerate(function (err) {
      if (err) return next(err)
      req.session.now = Date.now()
      res.end()
    })
  })
})

[JamesMGreene]

@dougwilson: That edge case is not actually possible to execute today as req.session.destroy causes the session to be deleted, and thus req.session.regenerate(...) results in:

Uncaught TypeError: Cannot read property 'regenerate' of undefined

[dougwilson]

Oops, sorry, I was just writing down. I went back and looked at that user's code and here it is, verbatim:

  // ...
  req.session.destroy(function (err) {
    if (err) return callback(err)
    req.sessionStore.regenerate(req, function (err) {
      if (err) return callback(err)
      // ...
    })
  })
  // ...

[JamesMGreene]

I was able to create a working unit test that is setup the same as your most recent code example above, which has now been pushed into the existing PR #242.

Please review the PR as soon as you can, thanks! :gift_heart:

[bsodmike]

I'm helping out with a code-base where the Session ID (SID) has been used to tie resources in the DB. We've got to the point where we are now handling killing off the session, simply to generate a new SID (as we do not want any further records to be persisted with the same SID).

The caveat is that we need to maintain any logged in users (via req.session.user a la Passport.js) and doing so via the regenerate() callback, does not help as the Cookie's SID in the browser is still the old SID.

1. calling destroy and regenerate - causes the user to be logged out, SID changes though.
2. calling regenerate - we are able to login the existing user via Passports logIn() facility, however the session SID stays the same.

Thoughts?

[JamesMGreene]

Ping @dougwilson & company. :bellhop_bell: 😄

[bsodmike]

@JamesMGreene I wonder if Express isn't that well supported now with the rise of Koa? Curious as to why this has been ignored for so longer :)

[JamesMGreene]

@bsodmike: Definitely not the case.

NPM download stats for the last month:

• express: 11,111,826
• hapi: 330,679
• restify: 261,022
• koa: 253,117
• sails: 85,738
• next: 29,596
• etc.

Collaborators just get busy. I can't bear any grudge over the delay as consumers of my popular open source libraries have been waiting for months and/or years for me to fix/enhance certain things and I literally just cannot find time to accommodate that anymore between work, family, gym, etc. 😧

[dougwilson]

Hi @bsodmike I never answered your question because I don't understand what it has to do with the PR and I was assuming you were talking to the OP of the PR. If it is not a direct question to the PR, please open a new issue or no one is going to try and assist.

[bsodmike]

Oh hey @dougwilson - No, sorry, do ignore my question as we worked around it. OP needs your attention :) Cheers!

[silverwind]

Looking forward to this. Right now, I manually delete the client's cookie on logout like this:

  const expireCookie = new expressSession.Cookie(req.session.cookie);
  expireCookie.expires = new Date(0);
  const cookies = cookie.parse(req.headers.cookie);
  res.header('set-cookie', expireCookie.serialize(appName, cookies[appName]));

appName is equal to the name option of express-session. Maybe there's a more elegant way than going through the Cookie constructor.

[Plochie]

If you want to expire cookie from server side then override it using response

res.cookie(cookie_name, cookie_value(optional), {
      expires:  new Date('Thu, 01 Jan 1970 00:00:00 UTC'),
      httpOnly: true
})

[davidjb]

Fwiw, manually unsetting / expiring the cookie didn't help me because the session has already been "touched" by my use of request.logout() from Passport. This means that express-session is going to call set-cookie later to "save" the session which has been modified. So, if you write your own cookie expiration code in a route, your expiration will come before the re-saving in express-session. This resulted in two set-cookie headers; mine which expired the cookie and express-session's which has the effect of "reinstating" it.

So what to do? express-session's use of onHeaders (https://github.com/expressjs/session/blob/master/index.js#L242) meant that you can't just attach your own onHeaders function because functions get called in reverse order, so mine would run before express-session's call, producing the same result.

A hack is to set req.sessionID = null (which causes shouldSetCookie to return false and no further cookie processing happens in express-session). Then, you can expire the cookie manually -- either or @silverwind or @Plochie's solutions would work.

👍 to expiring the cookie when the session gets destroyed.

[pszabop]

I have found all sorts of issues with reliably getting session logout to work reliably with Passport both on the client and server side.

This is the entirety of my logout code. It's similar to above cookie wipe code, solves another issue w/ Passport. and sanity checks some items that above sample code did not do.

I leave this here for others to critique or get advice from. However, I'm still seeing that occasionally a user session is still valid on the server even after they've hit the logout button.

PS: I'm using redis as the store.

app.post('/logout',
  // reloading / will start everything over in the user session, clearing the screen.
  // and users context.  This is desired.
  setAnonJwtToken,          // force switch to anonymous token
  function(req, res, next) {
    // attempt to expire the session cookie
    // @see https://github.com/expressjs/session/issues/241
    if (req.session && req.session.cookie) {
      res.cookie(config.auth.cookieName, null, {
        expires:  new Date('Thu, 01 Jan 1970 00:00:00 UTC'),
        httpOnly: true
      });
    }

    log.info('logging out', { user: req.user.id });
    // https://github.com/jaredhanson/passport-facebook/issues/202
    req.session.destroy(err => {
      if (err) {
        log.warning(err, 'session destroy error');
        return next(err);
      }
      req.logout();
      res.redirect(config.redirectRoot);
    });
  }
); 

[nicokaiser]

What is the state of this PR? I'd love to see a express-session version where this works, as we are experiencing quite some database requests with session IDs from expired sessions.

The hacks mentioned above do not apply for us, because sessions can expire at any point without the user hitting logout (e.g. user being locked, etc.), so an automatic cookie cleanup mechanism would be very helpful here.

[josh-renton]

+1

EDIT FROM @wesleytodd:

This is a place for productive conversation. Github added reactions for the purpose of avoiding comments like this. Adding +1 comments no longer has value, and all it does is spam contributor emails. I even went so far as to edit this comment instead of posting a new one to avoid more spam. Thank you all for being considerate and good OSS contributors in the future! 😀