Cookie should expire immediately when session is destroyed

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.

Expire cookie immediately upon destroying session

@dougwilson

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

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).

@dougwilson

@dougwilson:

I'm fine proceeding any which way:

the state of the current PR where it is just the new default behavior to always update the cookie upon session termination,
only update the cookie upon session termination if rolling: true is set, or
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!

@JamesMGreene

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 :)

👍

@dougwilson

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

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.

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. 😄

@dougwilson

@dougwilson Ping-a-ling-a-ling. 😊

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

@JamesMGreene

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

@dougwilson

@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

@JamesMGreene

@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 :)

@bsodmike

@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. 😧

@bsodmike

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.

@dougwilson

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

Expire cookie immediately upon destroying session

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.

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

@silverwind

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.

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

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.

@wesleytodd

+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! 😀

Navigation Menu

Repository navigation

Cookie should expire immediately when session is destroyed #241

11 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Cookie should expire immediately when session is destroyed #241

Description

Activity

JamesMGreene commented on Dec 12, 2015

dougwilson commented on Dec 15, 2015

JamesMGreene commented on Dec 15, 2015

dougwilson commented on Dec 15, 2015

JamesMGreene commented on Dec 15, 2015

JamesMGreene commented on Dec 21, 2015

dougwilson commented on Dec 24, 2015

JamesMGreene commented on Dec 27, 2015

JamesMGreene commented on Jan 6, 2016

dougwilson commented on Jan 6, 2016

JamesMGreene commented on Jan 6, 2016

11 remaining items

bsodmike commented on Mar 15, 2017

JamesMGreene commented on Mar 15, 2017

dougwilson commented on Mar 15, 2017

bsodmike commented on Mar 16, 2017

silverwind commented on Jul 30, 2017

Plochie commented on Jan 17, 2018

davidjb commented on Feb 20, 2018

pszabop commented on May 26, 2018

nicokaiser commented on Aug 16, 2018

josh-renton commented on Sep 20, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions