diff --git a/HISTORY.md b/HISTORY.md index fe50c562..5186a55d 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,12 +1,16 @@ unreleased ========== + ## ⚠️ BREAKING CHANGES * Change `saveUninitialized` option default from `true` to `false` - New sessions are no longer saved to the store (nor a cookie set) unless modified during the request; pass `saveUninitialized: true` to restore the previous behavior - The deprecation warning for omitting this option has been removed + * 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 ## Other changes diff --git a/README.md b/README.md index 2ef9558f..aecc1078 100644 --- a/README.md +++ b/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) { diff --git a/index.js b/index.js index f16d589a..894555d4 100644 --- a/index.js +++ b/index.js @@ -184,6 +184,7 @@ function session(options) { return } + var destroyed = false var originalHash; var originalId; var savedHash; @@ -198,6 +199,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; } @@ -292,6 +318,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); @@ -377,9 +404,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)) @@ -391,6 +425,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, diff --git a/test/session.js b/test/session.js index 2b8a312f..d0b0885e 100644 --- a/test/session.js +++ b/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(){ @@ -2463,6 +2603,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)