From 0616699df6d2e8d5b44a055f9cd811835db268b7 Mon Sep 17 00:00:00 2001
From: "James M. Greene" <james.m.greene@gmail.com>
Date: Fri, 30 Sep 2016 23:47:37 -0500
Subject: [PATCH] Expire cookie immediately upon destroying session

Fixes #241.
---
 index.js        |  75 ++++++++--
 test/session.js | 386 ++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 436 insertions(+), 25 deletions(-)

diff --git a/index.js b/index.js
index 68ccf533..e869f427 100644
--- a/index.js
+++ b/index.js
@@ -157,11 +157,7 @@ function session(options) {
   store.generate = function(req){
     req.sessionID = generateId(req);
     req.session = new Session(req);
-    req.session.cookie = new Cookie(cookieOptions);
-
-    if (cookieOptions.secure === 'auto') {
-      req.session.cookie.secure = issecure(req, trustProxy);
-    }
+    req.session.cookie = createCookie(cookieOptions, req, trustProxy);
   };
 
   var storeImplementsTouch = typeof store.touch === 'function';
@@ -217,29 +213,47 @@ function session(options) {
 
     // set-cookie
     onHeaders(res, function(){
+
+      // Is this an existing session that is being destroyed?
+      var isDestroying = isBeingDestroyed(req);
+
       if (!req.session) {
         debug('no session');
-        return;
+        if (!isDestroying || isInitialSession(req)) {
+          return;
+        }
       }
 
-      if (!shouldSetCookie(req)) {
-        return;
+      var cookie = req.session ? req.session.cookie : undefined;
+      if (isDestroying) {
+        if (cookie == null) {
+          debug('creating expired cookie');
+          cookie = createCookie(cookieOptions, req, trustProxy);
+        }
+
+        // Set the cookie to immediately expire on the client
+        cookie.maxAge = 0;
       }
 
       // only send secure cookies via https
-      if (req.session.cookie.secure && !issecure(req, trustProxy)) {
+      if (cookie.secure && !issecure(req, trustProxy)) {
         debug('not secured');
         return;
       }
 
-      if (!touched) {
-        // touch session
-        req.session.touch()
-        touched = true
+      if (!isDestroying) {
+        if (!shouldSetCookie(req)) {
+          return;
+        }
+        else if (!touched) {
+          // touch session
+          req.session.touch();
+          touched = true;
+        }
       }
 
       // set cookie
-      setcookie(res, name, req.sessionID, secrets[0], req.session.cookie.data);
+      setcookie(res, name, req.sessionID, secrets[0], cookie.data);
     });
 
     // proxy end() to commit the session
@@ -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;
+    }
+
+    // check if session is the initial session
+    function isInitialSession(req) {
+      return req.sessionID && cookieId != req.sessionID;
+    }
+
     // check if session has been modified
     function isModified(sess) {
       return originalId !== sess.id || originalHash !== hash(sess);
@@ -437,8 +461,8 @@ function session(options) {
 
     // determine if cookie should be set on response
     function shouldSetCookie(req) {
-      // cannot set cookie without a session ID
-      if (typeof req.sessionID !== 'string') {
+      // cannot set cookie without a session ID and a session
+      if (typeof req.sessionID !== 'string' || !req.session) {
         return false;
       }
 
@@ -491,6 +515,25 @@ function session(options) {
   };
 };
 
+/**
+ * Create a new Cookie for this request.
+ *
+ * @param {Object} cookieOptions
+ * @param {Object} req
+ * @param {Boolean} [trustProxy]
+ * @return {Cookie}
+ * @private
+ */
+function createCookie(cookieOptions, req, trustProxy) {
+  var cookieOpts = cookieOptions || {};
+
+  var cookie = new Cookie(cookieOpts);
+  if (cookieOpts.secure === 'auto') {
+    cookie.secure = issecure(req, trustProxy);
+  }
+  return cookie;
+}
+
 /**
  * Generate a session ID for a new session.
  *
diff --git a/test/session.js b/test/session.js
index 6711df98..b3133732 100644
--- a/test/session.js
+++ b/test/session.js
@@ -1,4 +1,3 @@
-
 process.env.NO_DEPRECATION = 'express-session';
 
 var after = require('after')
@@ -1258,8 +1257,272 @@ describe('session()', function(){
       });
     });
 
+    it('should set expired cookie to override persistent cookie on req.session = null', function(done){
+      var store = new session.MemoryStore()
+      var app = express()
+        .use(session({ store: store, unset: 'destroy', secret: 'keyboard cat', cookie: { maxAge: min } }))
+        .use(function(req, res, next){
+          req.session.count = req.session.count || 0
+          req.session.count++
+          if (req.session.count === 2) req.session = null
+          res.end()
+        })
+
+      request(app)
+      .get('/')
+      .expect(shouldSetCookie('connect.sid'))
+      .expect(200, function(err, res){
+        if (err) return done(err)
+
+        var now = new Date()
+
+        var expiry = expires(res)
+        assert.ok(expiry)
+
+        var a = new Date(expiry)
+
+        var delta = a.valueOf() - now.valueOf()
+        assert.ok(delta > (min - 1000) && delta <= min)
+
+        request(app)
+        .get('/')
+        .set('Cookie', cookie(res))
+        .expect(shouldSetCookie('connect.sid'))
+        .expect(200, function(err, res){
+          if (err) return done(err)
+
+          now = new Date()
+
+          expiry = expires(res)
+          assert.ok(expiry)
+
+          var b = new Date(expiry)
+
+          delta = b.valueOf() - a.valueOf()
+          assert.ok(delta <= 0)
+
+          delta = b.valueOf() - now.valueOf()
+          assert.ok(delta <= 0)
+
+          done()
+        })
+      })
+    })
+
+    it('should set expired cookie to override session cookie on req.session = null', function(done){
+      var store = new session.MemoryStore()
+      var app = express()
+        .use(session({ store: store, unset: 'destroy', secret: 'keyboard cat' }))
+        .use(function(req, res, next){
+          req.session.count = req.session.count || 0
+          req.session.count++
+          if (req.session.count === 2) req.session = null
+          res.end()
+        });
+
+      request(app)
+      .get('/')
+      .expect(shouldSetCookie('connect.sid'))
+      .expect(200, function(err, res){
+        if (err) return done(err)
+
+        assert.ok(!expires(res))
+
+        request(app)
+        .get('/')
+        .set('Cookie', cookie(res))
+        .expect(shouldSetCookie('connect.sid'))
+        .expect(200, function(err, res){
+          if (err) return done(err)
+
+          var now = new Date()
+
+          var expiry = expires(res)
+          assert.ok(expiry)
+
+          var b = new Date(expiry)
+
+          var delta = b.valueOf() - now.valueOf()
+          assert.ok(delta <= 0)
+
+          done()
+        })
+      })
+    })
+
+    it('should set expired cookie to override persistent cookie on req.session.destroy()', function(done) {
+      var store = new session.MemoryStore()
+      var app = express()
+        .use(session({ store: store, unset: 'destroy', secret: 'keyboard cat', cookie: { maxAge: min } }))
+        .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()
+          }
+        })
+
+      request(app)
+      .get('/')
+      .expect(shouldSetCookie('connect.sid'))
+      .expect(200, function(err, res){
+        if (err) return done(err)
+
+        var now = new Date()
+
+        var expiry = expires(res)
+        assert.ok(expiry)
+
+        var a = new Date(expiry)
+
+        var delta = a.valueOf() - now.valueOf()
+        assert.ok(delta > (min - 1000) && delta <= min)
+
+        request(app)
+        .get('/')
+        .set('Cookie', cookie(res))
+        .expect(shouldSetCookie('connect.sid'))
+        .expect(200, function(err, res){
+          if (err) return done(err)
+
+          now = new Date()
+
+          expiry = expires(res)
+          assert.ok(expiry)
+
+          var b = new Date(expiry)
+
+          delta = b.valueOf() - a.valueOf()
+          assert.ok(delta <= 0)
+
+          delta = b.valueOf() - now.valueOf()
+          assert.ok(delta <= 0)
+
+          done()
+        })
+      })
+    })
+
+    it('should set expired cookie to override session cookie on req.session.destroy()', function(done) {
+      var store = new session.MemoryStore()
+      var app = express()
+        .use(session({ store: store, unset: 'destroy', secret: 'keyboard cat' }))
+        .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()
+          }
+        })
+
+      request(app)
+      .get('/')
+      .expect(shouldSetCookie('connect.sid'))
+      .expect(200, function(err, res){
+        if (err) return done(err)
+
+        assert.ok(!expires(res))
+
+        request(app)
+        .get('/')
+        .set('Cookie', cookie(res))
+        .expect(shouldSetCookie('connect.sid'))
+        .expect(200, function(err, res){
+          if (err) return done(err)
+
+          var now = new Date()
+
+          var expiry = expires(res)
+          assert.ok(expiry)
+
+          var b = new Date(expiry)
+
+          var delta = b.valueOf() - now.valueOf()
+          assert.ok(delta <= 0)
+
+          done()
+        })
+      })
+    })
+
+    it('should not set an expired cookie on destroyed session if session is regenerated', function(done){
+      var store = new session.MemoryStore()
+      var app = express()
+        .use(session({ store: store, unset: 'destroy', secret: 'keyboard cat', cookie: { maxAge: min } }))
+        .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 done(err)
+
+              req.sessionStore.regenerate(req, function(err) {
+                if (err) return done(err)
+
+                req.session.now = Date.now()
+                res.end()
+              })
+            })
+          }
+          else {
+            res.end()
+          }
+        })
+
+      request(app)
+      .get('/')
+      .expect(shouldSetCookie('connect.sid'))
+      .expect(200, function(err, res){
+        if (err) return done(err)
+
+        var now = new Date()
+
+        var expiry = expires(res)
+        assert.ok(expiry)
+
+        var a = new Date(expiry)
+
+        var delta = a.valueOf() - now.valueOf()
+        assert.ok(delta > (min - 1000) && delta <= min)
+
+        request(app)
+        .get('/')
+        .set('Cookie', cookie(res))
+        .expect(shouldSetCookie('connect.sid'))
+        .expect(200, function(err, res){
+          if (err) return done(err)
+
+          now = new Date()
+
+          expiry = expires(res)
+          assert.ok(expiry)
+
+          var b = new Date(expiry)
+
+          // The session regenerated so the expiration date should be the same as the original
+          delta = b.valueOf() - a.valueOf()
+          assert.equal(delta, 0)
+
+          delta = b.valueOf() - now.valueOf()
+          assert.ok(delta > 0)
+
+          done()
+        })
+      })
+    })
+
     it('should not set cookie if initial session destroyed', function(done){
-      var store = new session.MemoryStore();
+      var store = new session.MemoryStore()
       var server = createServer({ store: store, unset: 'destroy' }, function (req, res) {
         req.session = null
         res.end()
@@ -1269,14 +1532,14 @@ describe('session()', function(){
       .get('/')
       .expect(shouldNotHaveHeader('Set-Cookie'))
       .expect(200, function(err, res){
-        if (err) return done(err);
+        if (err) return done(err)
         store.length(function(err, len){
-          if (err) return done(err);
+          if (err) return done(err)
           assert.equal(len, 0)
-          done();
-        });
-      });
-    });
+          done()
+        })
+      })
+    })
 
     it('should pass session destroy error', function (done) {
       var cb = after(2, done)
@@ -1300,7 +1563,7 @@ describe('session()', function(){
       .get('/')
       .expect(200, 'session destroyed', cb)
     })
-  });
+  })
 
   describe('res.end patch', function () {
     it('should correctly handle res.end/res.write patched prior', function (done) {
@@ -1463,6 +1726,111 @@ describe('session()', function(){
         .expect(shouldNotHaveHeader('Set-Cookie'))
         .expect(200, 'undefined', done)
       })
+
+      it('should set expired cookie to override persistent cookie', function(done) {
+        var store = new session.MemoryStore()
+        var app = express()
+          .use(session({ store: store, secret: 'keyboard cat', cookie: { maxAge: min } }))
+          .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()
+            }
+          })
+
+        request(app)
+        .get('/')
+        .expect(shouldSetCookie('connect.sid'))
+        .expect(200, function(err, res){
+          if (err) return done(err)
+
+          var now = new Date()
+
+          var expiry = expires(res)
+          assert.ok(expiry)
+
+          var a = new Date(expiry)
+
+          var delta = a.valueOf() - now.valueOf()
+          assert.ok(delta > (min - 1000) && delta <= min)
+
+          request(app)
+          .get('/')
+          .set('Cookie', cookie(res))
+          .expect(shouldSetCookie('connect.sid'))
+          .expect(200, function(err, res){
+            if (err) return done(err)
+
+            now = new Date()
+
+            expiry = expires(res)
+            assert.ok(expiry)
+
+            var b = new Date(expiry)
+
+            delta = b.valueOf() - a.valueOf()
+            assert.ok(delta <= 0)
+
+            delta = b.valueOf() - now.valueOf()
+            assert.ok(delta <= 0)
+
+            done()
+          })
+        })
+      })
+
+      it('should set expired cookie to override session cookie', function(done) {
+        var store = new session.MemoryStore()
+        var app = express()
+          .use(session({ store: store, secret: 'keyboard cat' }))
+          .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()
+            }
+          })
+
+        request(app)
+        .get('/')
+        .expect(shouldSetCookie('connect.sid'))
+        .expect(200, function(err, res){
+          if (err) return done(err)
+
+          assert.ok(!expires(res))
+
+          request(app)
+          .get('/')
+          .set('Cookie', cookie(res))
+          .expect(shouldSetCookie('connect.sid'))
+          .expect(200, function(err, res){
+            if (err) return done(err)
+
+            var now = new Date()
+
+            var expiry = expires(res)
+            assert.ok(expiry)
+
+            var b = new Date(expiry)
+
+            var delta = b.valueOf() - now.valueOf()
+            assert.ok(delta <= 0)
+
+            done()
+          })
+        })
+      })
     })
 
     describe('.regenerate()', function(){