Expire cookie immediately upon destroying session #242

JamesMGreene · 2015-12-10T18:43:35Z

Force an existing session's cookie to expire immediately upon destroying the session.

Fixes #241.

JamesMGreene · 2015-12-11T21:12:13Z

@dougwilson: I'm sure you're day-job-and-open-source-swamped like the rest of us but if there is any chance of getting PR #240 and this PR merged and released within the next 2 weeks or so, my team and I would appreciate it! 👍 💝

dougwilson · 2015-12-11T22:01:45Z

Hi James, sorry I haven't been on a computer since you made this. I will look at this pull request as soon as I can. I cannot comment on when this pull request would be accepted, because I need to understand first what the pull requests does and understand if this is a desired feature, if there are edges cases, etc.

I know you feel like it is a bug, but I need to look at it more, as to me, if feels like either a bug or a new feature.

JamesMGreene · 2015-12-12T03:10:05Z

Sounds good, @dougwilson. Thanks! 👍

If you do end up feeling it is a new feature and would rather implement it as an option rather than as the new default behavior, I'd be fine with refactoring it to work as such if you give me a bit of design suggestion/guidance on how you'd like it to work.

I'll keep an eye out for any feedback requiring changes on my part. 👀

JamesMGreene · 2015-12-12T03:13:11Z

@dougwilson: As mentioned in the issue (#241):

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.

JamesMGreene · 2016-01-08T04:20:56Z

@dougwilson: If there is any chance of this being merged and released before Monday, I'd 💛 you for it.

dougwilson · 2016-01-11T04:45:46Z

Hi @JamesMGreene , I'm sorry, I really wanted to (even did a few commits to this repo tonight, yay!) get this done tonight, before Monday for you. I just didn't get to, but what I did do is at least published the changes to npm that we had so far, and then I really want to get this change done on Monday for you.

JamesMGreene · 2016-01-11T05:49:35Z

@dougwilson: If it makes it in this week, I can probably still pull it into our release branch. Monday would be best if possible so I can just get it into the mainline branch, though. 👍

dougwilson · 2016-01-11T15:25:07Z

Yea, I'm working on it. Just looked at it and there are all kinds of nits, like the function in index.js is not alphabetically sorted like the rest, the tests are in the wrong location/their descriptions are too long. I'm working on adjusting.

dougwilson · 2016-01-11T15:25:36Z

And a reassignment of an argument, which results in a v8 de-opt.

dougwilson · 2016-01-11T15:50:39Z

Hi! It seems like if a user calls req.session.destroy() in their code, then it won't actually set the expired cookie. This seems very unexpected and one would expect that the method of destruction would not change if the cookie is expired or not. Example test app:

var app = express()
.use(session({ secret: 'keyboard cat', cookie: { maxAge: 86400000 } }))
.use(function(req, res, next){
  req.session.count = req.session.count || 0;
  req.session.count++;
  if (req.session.count === 2) {
    req.session.destroy(function(err) {
      if (err) return next(err);
      res.end();
    });
  } else {
    res.end();
  }
});

dougwilson · 2016-01-11T15:51:18Z

index.js

+ * @return {Cookie}
+ * @private
+ */
+function createCookie(cookieOptions, req, trustProxy) {


Can you move this to be alphabetical within all these functions while you're working on the destroy issue?

dougwilson · 2016-01-16T01:21:51Z

Hi @JamesMGreene , I'm not in a rush, but I know you seemed to be, so I just wanted to put a note here just in case you missed my comment in #242 (comment)

JamesMGreene · 2016-01-25T15:00:22Z

Thanks, @dougwilson. You caught me in the thralls of our busy release sprint, so I didn't have any time to address this.

I should be able to take a look this week. I'm OK with this change not going out with our next release; my PR #240 being merged into v1.13.0 was the more important change. 👍

JamesMGreene · 2016-06-14T15:08:22Z

the tests are in the wrong location/their descriptions are too long

@dougwilson: Can you fill me in more on this comment?

JamesMGreene · 2016-06-14T16:02:03Z

It seems like if a user calls req.session.destroy() in their code, then it won't actually set the expired cookie. This seems very unexpected and one would expect that the method of destruction would not change if the cookie is expired or not.

Working through this issue now.

JamesMGreene · 2016-06-14T17:39:49Z

Alright, so the problem with when the user calls req.session.destroy() — without also setting the unset: "destroy" option — is that shouldDestroy(req) returns false, ergo the isBeingDestroyed variable is also set to false:

// Is this an existing session that is being destroyed?
var isBeingDestroyed = !!cookieId && shouldDestroy(req);

Reminder of current shouldDestroy implementation:

// determine if session should be destroyed
function shouldDestroy(req) {
  return req.sessionID && unsetDestroy && req.session == null;
}

@dougwilson et al: Any advice on how to workaround this situation?

P.S. Note that if you do set the unset: "destroy" option, then it works as expected.

dougwilson · 2016-06-16T17:23:10Z

Hi @JamesMGreene, sorry I didn't reply right away; I was just trying to focus on pushing out Express 4.14 :) So I think that that is probably just not a full understanding of the current code, that that's OK! At least how it is designed right now, the shouldDestroy and similar is only there to track if the "auto saving" part of this module should basically destroy it instead of save it. This functionality is completely independent of tracking if the session has been manually destroyed or not. You probably have to add manual tracking for this, similar to how we manually track if a session has manually been saved or not.

JamesMGreene · 2016-06-17T04:53:47Z

OK, gotcha @dougwilson. I'll look into that further.

JamesMGreene · 2016-09-30T11:36:48Z

@dougwilson: Finally found a bit of time to dig into this further and get it all working. Please re-review the new version when you get a chance. Thanks!

dougwilson · 2016-10-01T00:41:46Z

Hi @JamesMGreene , it looks like the last commit was Dec 10, 2015. Is that the most recent version?

JamesMGreene · 2016-10-01T04:49:06Z

@dougwilson: No, sorry, that was because I did a git commit --amend (so it maintained the old commit's date). I've pushed a new replacement commit now dated as today for better accuracy.

JamesMGreene · 2016-10-22T05:38:10Z

gently pokes @dougwilson No rush but a gentle reminder. :)

bsodmike · 2017-03-16T01:33:42Z

@JamesMGreene You'll need to clean up re. the conflict 👍

Fixes expressjs#241.

JamesMGreene · 2017-03-16T02:17:34Z

Rebased and dealt with merge conflicts again. 👍

dougwilson · 2017-03-17T03:21:43Z

index.js

@@ -396,6 +410,16 @@ function session(options) {
      });
    }

+    // check if session has been or is being destroyed
+    function isBeingDestroyed(req) {
+      return req.sessionID && req.session == null;


Should this check that unsetDestroy is true here? That option is what makes the unsetting of req.session actually mean the session is to be destroyed, otherwise it does nothing.

Hmm, if I add that check here, the tests don't pass any longer, but if I don't add the check, then req.session = null will destroy the cookie even if the user doesn't want req.session = null to destroy the session...

Hmm, sounds like I must need another unit test for that last condition you mentioned. I'll take a peek when I get a chance.

dougwilson · 2017-03-17T03:25:11Z

test/session.js

@@ -1269,14 +1532,14 @@ describe('session()', function(){
      .get('/')
      .expect(shouldNotHaveHeader('Set-Cookie'))
      .expect(200, function(err, res){
-        if (err) return done(err);


Looks like a bunch of accidental changes here with the formatting; I can fix, but figured I'd comment here in case you had to make a change from the question above.

Yeah, that doesn't surprise me too much as it seemed like someone changed the test code style (e.g. to be semi-colon-less) in the time since my initial PR submission. I'll try to clean them up when I make my updates.

dougwilson · 2017-03-17T03:29:02Z

test/session.js

+          assert.ok(delta <= 0)
+
+          delta = b.valueOf() - now.valueOf()
+          assert.ok(delta <= 0)


There is a shouldSetCookieToExpireIn helper that could probably be used here if you want.

Thanks for letting me know. I'll take a peek and see if/where it makes sense to use.

dougwilson · 2019-10-11T02:58:51Z

Hi @JamesMGreene , I know it has been a long time. I let these PRs just sit and I'm trying to make it right by getting them all merged in. I went and rebased all your changes, and even made the adjustments I noted above. It doesn't seem like I'm able to push to your fork, though.

If you haven't already done so, checking off the allow on the right hand side (https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) would allow me to push up the changes + rebase and then merge the pull request.

But this is not a requirement to land your changes, it just will make the PR show as "merged" instead of "closed". If you're no longer around to do so / don't want to / whatever, it's no big deal. I will plan to merge it within a couple weeks if I don't hear back.

gabeio added the pr label Dec 10, 2015

JamesMGreene force-pushed the destroy_cookie branch from 9de1f74 to 7603648 Compare December 11, 2015 16:11

JamesMGreene mentioned this pull request Dec 11, 2015

Prevent the rolling: true option from overriding the saveUnitialized: false option #240

Closed

dougwilson self-assigned this Dec 15, 2015

JamesMGreene mentioned this pull request Jan 6, 2016

Cookie should expire immediately when session is destroyed #241

Open

JamesMGreene force-pushed the destroy_cookie branch 2 times, most recently from fe14c14 to f0acf18 Compare January 7, 2016 05:57

dougwilson reviewed Jan 11, 2016
View reviewed changes

JamesMGreene force-pushed the destroy_cookie branch from f0acf18 to 73bdaf9 Compare June 14, 2016 15:07

JamesMGreene force-pushed the destroy_cookie branch from 73bdaf9 to c19c384 Compare June 14, 2016 15:31

JamesMGreene force-pushed the destroy_cookie branch from c19c384 to ad8c8b8 Compare June 14, 2016 17:39

JamesMGreene force-pushed the destroy_cookie branch from ad8c8b8 to 4deb197 Compare June 14, 2016 17:47

JamesMGreene force-pushed the destroy_cookie branch from 4deb197 to 756301a Compare September 30, 2016 11:35

JamesMGreene force-pushed the destroy_cookie branch from 756301a to c9102e4 Compare October 1, 2016 04:48

dougwilson added the enhancement label Nov 17, 2016

JamesMGreene force-pushed the destroy_cookie branch from c9102e4 to f0cec99 Compare March 16, 2017 02:09

Expire cookie immediately upon destroying session

0616699

Fixes expressjs#241.

JamesMGreene force-pushed the destroy_cookie branch from f0cec99 to 0616699 Compare March 16, 2017 02:16

dougwilson approved these changes Mar 17, 2017

View reviewed changes

dougwilson reviewed Mar 17, 2017

View reviewed changes

dougwilson added the needs rebase label Nov 20, 2018

dougwilson force-pushed the master branch 2 times, most recently from 9d2e29b to 408229e Compare January 28, 2024 20:24

bjohansebas modified the milestone: 2.0.0 Feb 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expire cookie immediately upon destroying session #242

Expire cookie immediately upon destroying session #242

JamesMGreene commented Dec 10, 2015

JamesMGreene commented Dec 11, 2015

dougwilson commented Dec 11, 2015

JamesMGreene commented Dec 12, 2015

JamesMGreene commented Dec 12, 2015

JamesMGreene commented Jan 8, 2016

dougwilson commented Jan 11, 2016

JamesMGreene commented Jan 11, 2016

dougwilson commented Jan 11, 2016

dougwilson commented Jan 11, 2016

dougwilson commented Jan 11, 2016

dougwilson Jan 11, 2016

JamesMGreene Jun 14, 2016

dougwilson commented Jan 16, 2016

JamesMGreene commented Jan 25, 2016

JamesMGreene commented Jun 14, 2016

JamesMGreene commented Jun 14, 2016

JamesMGreene commented Jun 14, 2016 •

edited

Loading

dougwilson commented Jun 16, 2016

JamesMGreene commented Jun 17, 2016

JamesMGreene commented Sep 30, 2016

dougwilson commented Oct 1, 2016

JamesMGreene commented Oct 1, 2016

JamesMGreene commented Oct 22, 2016

bsodmike commented Mar 16, 2017

JamesMGreene commented Mar 16, 2017

dougwilson Mar 17, 2017

dougwilson Mar 17, 2017

JamesMGreene Mar 20, 2017

dougwilson Mar 17, 2017

JamesMGreene Mar 20, 2017

dougwilson Mar 17, 2017

JamesMGreene Mar 20, 2017

dougwilson commented Oct 11, 2019

Expire cookie immediately upon destroying session #242

Are you sure you want to change the base?

Expire cookie immediately upon destroying session #242

Conversation

JamesMGreene commented Dec 10, 2015

JamesMGreene commented Dec 11, 2015

dougwilson commented Dec 11, 2015

JamesMGreene commented Dec 12, 2015

JamesMGreene commented Dec 12, 2015

JamesMGreene commented Jan 8, 2016

dougwilson commented Jan 11, 2016

JamesMGreene commented Jan 11, 2016

dougwilson commented Jan 11, 2016

dougwilson commented Jan 11, 2016

dougwilson commented Jan 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougwilson commented Jan 16, 2016

JamesMGreene commented Jan 25, 2016

JamesMGreene commented Jun 14, 2016

JamesMGreene commented Jun 14, 2016

JamesMGreene commented Jun 14, 2016 • edited Loading

dougwilson commented Jun 16, 2016

JamesMGreene commented Jun 17, 2016

JamesMGreene commented Sep 30, 2016

dougwilson commented Oct 1, 2016

JamesMGreene commented Oct 1, 2016

JamesMGreene commented Oct 22, 2016

bsodmike commented Mar 16, 2017

JamesMGreene commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougwilson commented Oct 11, 2019

JamesMGreene commented Jun 14, 2016 •

edited

Loading