feat: expire session cookie on destruction

HISTORY.md

@@ -1,6 +1,9 @@
 unreleased
 ==========
 
+  * Expire the session cookie on the response when the session is destroyed
+    - Applies to `req.session.destroy()` and to `unset: 'destroy'`, when the request
+      came in with a session cookie
   * Replace `uid-safe` dependency with built-in `crypto.randomBytes` for session ID generation
     - Session IDs keep the same format as before (32-character base64url strings)
 

README.md

@@ -337,7 +337,8 @@ etc.).
 
 The default value is `'keep'`.
 
-  - `'destroy'` The session will be destroyed (deleted) when the response ends.
+  - `'destroy'` The session will be destroyed (deleted) when the response ends,
+    and the response will set an expired cookie to remove it from the client.
   - `'keep'` The session in the store will be kept, but modifications made during
     the request are ignored and not saved.
 
@@ -381,7 +382,9 @@ req.session.regenerate(function(err) {
 #### Session.destroy(callback)
 
 Destroys the session and will unset the `req.session` property.
-Once complete, the `callback` will be invoked.
+Once complete, the `callback` will be invoked. If the request came
+in with a session cookie, the response will set an expired cookie
+to remove it from the client.
 
 ```js
 req.session.destroy(function(err) {

index.js

@@ -186,6 +186,7 @@ function session(options) {
       return
     }
 
+    var destroyed = false
     var originalHash;
     var originalId;
     var savedHash;
@@ -200,6 +201,31 @@ function session(options) {
     // set-cookie
     onHeaders(res, function(){
       if (!req.session) {
+        // expire the cookie when the session was destroyed
+        if (destroyed && cookieId) {
+          var expired = new Cookie(cookieOptions)
+          expired.expires = new Date(0)
+
+          if (expired.secure === 'auto') {
+            expired.secure = issecure(req, trustProxy)
+          }
+
+          // only send secure cookies via https
+          if (expired.secure && !issecure(req, trustProxy)) {
+            debug('not secured, cannot expire cookie');
+            return;
+          }
+
+          debug('expire cookie')
+
+          try {
+            setcookie(res, name, '', secrets[0], expired.data)
+          } catch (err) {
+            setImmediate(next, err)
+          }
+          return
+        }
+
         debug('no session');
         return;
       }
@@ -294,6 +320,7 @@ function session(options) {
       if (shouldDestroy(req)) {
         // destroy session
         debug('destroying');
+        destroyed = true
         store.destroy(req.sessionID, function ondestroy(err) {
           if (err) {
             setImmediate(next, err);
@@ -379,9 +406,16 @@ function session(options) {
 
     // wrap session methods
     function wrapmethods(sess) {
+      var _destroy = sess.destroy
       var _reload = sess.reload
       var _save = sess.save;
 
+      function destroy() {
+        debug('destroying %s', this.id)
+        destroyed = true
+        return _destroy.apply(this, arguments)
+      }
+
       function reload(callback) {
         debug('reloading %s', this.id)
         _reload.call(this, rewrapmethods(this, callback))
@@ -393,6 +427,13 @@ function session(options) {
         _save.apply(this, arguments);
       }
 
+      Object.defineProperty(sess, 'destroy', {
+        configurable: true,
+        enumerable: false,
+        value: destroy,
+        writable: true
+      })
+
       Object.defineProperty(sess, 'reload', {
         configurable: true,
         enumerable: false,

test/session.js

@@ -1351,6 +1351,7 @@ describe('session()', function(){
           request(server)
           .get('/')
           .set('Cookie', cookie(res))
+          .expect(shouldSetCookieToExpired('connect.sid'))
           .expect(200, function(err, res){
             if (err) return done(err);
             store.length(function(err, len){
@@ -1363,6 +1364,27 @@ describe('session()', function(){
       });
     });
 
+    it('should not expire cookie on req.session = null when set to keep', function(done){
+      var store = new session.MemoryStore();
+      var server = createServer({ store: store, unset: 'keep' }, function (req, res) {
+        req.session.count = req.session.count || 0
+        req.session.count++
+        if (req.session.count === 2) req.session = null
+        res.end()
+      })
+
+      request(server)
+      .get('/')
+      .expect(200, function(err, res){
+        if (err) return done(err);
+        request(server)
+        .get('/')
+        .set('Cookie', cookie(res))
+        .expect(shouldNotHaveHeader('Set-Cookie'))
+        .expect(200, done)
+      });
+    });
+
     it('should not set cookie if initial session destroyed', function(done){
       var store = new session.MemoryStore();
       var server = createServer({ store: store, unset: 'destroy' }, function (req, res) {
@@ -1604,6 +1626,124 @@ describe('session()', function(){
         .expect(shouldNotHaveHeader('Set-Cookie'))
         .expect(200, 'undefined', done)
       })
+
+      it('should expire the cookie of an existing session', function (done) {
+        var server = createServer(null, function (req, res) {
+          if (req.url === '/') {
+            req.session.active = true
+            res.end('session created')
+            return
+          }
+
+          req.session.destroy(function (err) {
+            if (err) res.statusCode = 500
+            res.end('destroyed')
+          })
+        })
+
+        request(server)
+        .get('/')
+        .expect(200, 'session created', function (err, res) {
+          if (err) return done(err)
+          request(server)
+          .get('/foo')
+          .set('Cookie', cookie(res))
+          .expect(shouldSetCookieToExpired('connect.sid'))
+          .expect(200, 'destroyed', done)
+        })
+      })
+
+      it('should not send expired secure cookie when insecure', function (done) {
+        function setup (req) {
+          req.secure = JSON.parse(req.headers['x-secure'])
+        }
+
+        var server = createServer(setup, { cookie: { secure: true, maxAge: min } }, function (req, res) {
+          if (req.url === '/') {
+            req.session.active = true
+            res.end('session created')
+            return
+          }
+
+          req.session.destroy(function (err) {
+            if (err) res.statusCode = 500
+            res.end('destroyed')
+          })
+        })
+
+        request(server)
+        .get('/')
+        .set('X-Secure', 'true')
+        .expect(shouldSetCookie('connect.sid'))
+        .expect(200, 'session created', function (err, res) {
+          if (err) return done(err)
+          request(server)
+          .get('/foo')
+          .set('Cookie', cookie(res))
+          .set('X-Secure', 'false')
+          .expect(shouldNotHaveHeader('Set-Cookie'))
+          .expect(200, 'destroyed', done)
+        })
+      })
+
+      describe('when cookie "secure" set to "auto"', function () {
+        function setup (req) {
+          req.secure = JSON.parse(req.headers['x-secure'])
+        }
+
+        function createDestroyServer () {
+          return createServer(setup, { cookie: { secure: 'auto', maxAge: min } }, function (req, res) {
+            if (req.url === '/') {
+              req.session.active = true
+              res.end('session created')
+              return
+            }
+
+            req.session.destroy(function (err) {
+              if (err) res.statusCode = 500
+              res.end('destroyed')
+            })
+          })
+        }
+
+        it('should expire cookie with Secure when connection is secure', function (done) {
+          var server = createDestroyServer()
+
+          request(server)
+          .get('/')
+          .set('X-Secure', 'true')
+          .expect(shouldSetCookie('connect.sid'))
+          .expect(200, 'session created', function (err, res) {
+            if (err) return done(err)
+            request(server)
+            .get('/foo')
+            .set('Cookie', cookie(res))
+            .set('X-Secure', 'true')
+            .expect(shouldSetCookieToExpired('connect.sid'))
+            .expect(shouldSetCookieWithAttribute('connect.sid', 'Secure'))
+            .expect(200, 'destroyed', done)
+          })
+        })
+
+        it('should expire cookie without Secure when connection is insecure', function (done) {
+          var server = createDestroyServer()
+
+          request(server)
+          .get('/')
+          .set('X-Secure', 'false')
+          .expect(shouldSetCookie('connect.sid'))
+          .expect(200, 'session created', function (err, res) {
+            if (err) return done(err)
+            request(server)
+            .get('/foo')
+            .set('Cookie', cookie(res))
+            .set('X-Secure', 'false')
+            .expect(shouldSetCookieToExpired('connect.sid'))
+            .expect(shouldSetCookieWithoutAttribute('connect.sid', 'Secure'))
+            .expect(200, 'destroyed', done)
+          })
+        })
+      })
     })
 
     describe('.regenerate()', function(){
@@ -2459,6 +2599,17 @@ function shouldSetCookieToDifferentSessionId (id) {
   }
 }
 
+function shouldSetCookieToExpired (name) {
+  return function (res) {
+    var header = cookie(res)
+    var data = header && utils.parseSetCookie(header)
+    assert.ok(header, 'should have a cookie header')
+    assert.strictEqual(data.name, name, 'should set cookie ' + name)
+    assert.ok(('expires' in data), 'should set cookie with attribute Expires')
+    assert.strictEqual(Date.parse(data.expires), 0, 'should set cookie ' + name + ' to expired')
+  }
+}
+
 function shouldSetCookieToExpireIn (name, delta) {
   return function (res) {
     var header = cookie(res)