From b13c8373b17b88d22ef3db767c8164dacc424507 Mon Sep 17 00:00:00 2001
From: Patrik Simek <patrik@patriksimek.cz>
Date: Fri, 15 May 2015 17:24:05 +0200
Subject: [PATCH 1/6] Save session before the `location` header is sent to the
 browser

Discussion: https://github.com/expressjs/session/pull/69
---
 index.js        | 69 +++++++++++++++++++++++++++++++++++++++++++++++++
 test/session.js | 36 ++++++++++++++++++++++++++
 2 files changed, 105 insertions(+)

diff --git a/index.js b/index.js
index 6f6d1b52..dd24521e 100644
--- a/index.js
+++ b/index.js
@@ -199,6 +199,70 @@ function session(options){
 
       setcookie(res, name, req.sessionID, secrets[0], cookie.data);
     });
+    
+    // proxy redirect() to commit the session
+    var _redirect = res.redirect;
+    var redirected = false;
+    res.redirect = function redirect(url) {
+	  if (redirected) {
+        return;
+      }
+
+      redirected = true;
+
+      if (shouldDestroy(req)) {
+        // destroy session
+        debug('destroying');
+        store.destroy(req.sessionID, function ondestroy(err) {
+          if (err) {
+            defer(next, err);
+          }
+
+          debug('destroyed');
+          _redirect.call(res, url);
+        });
+        
+        return;
+      }
+      
+      // no session to save
+      if (!req.session) {
+        debug('no session');
+        _redirect.call(res, url);
+        
+        return;
+      }
+
+      // touch session
+      req.session.touch();
+      
+      if (shouldSave(req)) {
+        req.session.save(function onsave(err) {
+          if (err) {
+            defer(next, err);
+          }
+
+          _redirect.call(res, url);
+        });
+        
+        return;
+      } else if (storeImplementsTouch && shouldTouch(req)) {
+        // store implements touch method
+        debug('touching');
+        store.touch(req.sessionID, req.session, function ontouch(err) {
+          if (err) {
+            defer(next, err);
+          }
+
+          debug('touched');
+          _redirect.call(res, url);
+        });
+        
+        return;
+      }
+      
+      return _redirect.call(res, url);
+    };
 
     // proxy end() to commit the session
     var _end = res.end;
@@ -210,6 +274,11 @@ function session(options){
       }
 
       ended = true;
+      
+      if (redirected) {
+	    // we've done everything in res.redirect
+	    return _end.call(res, chunk, encoding);
+      }
 
       var ret;
       var sync = true;
diff --git a/test/session.js b/test/session.js
index f592ef3f..8cf52b0b 100644
--- a/test/session.js
+++ b/test/session.js
@@ -367,6 +367,42 @@ describe('session()', function(){
         done()
       })
     })
+
+    it('should have saved session before before res.redirect sends location header', function (done) {
+      var saved = false
+      var success = false
+      var store = new session.MemoryStore()
+      var app = express()
+        .use(session({ store: store, secret: 'keyboard cat', cookie: { maxAge: min }}))
+        .use(function(req, res){
+	      req.session.hit = true
+	      res.redirect('http://xxx.com');
+        });
+      app.set('env', 'test');
+      
+      var _set = store.set
+      store.set = function set(sid, sess, callback) {
+        setTimeout(function () {
+          _set.call(store, sid, sess, function (err) {
+            saved = true
+            callback(err)
+          })
+        }, 200)
+      }
+
+      request(app)
+      .get('/')
+      .expect(shouldSetCookie('connect.sid'))
+      .expect('location', 'http://xxx.com')
+      .expect(302, function (err) {
+        if (err) return done(err)
+        assert.ok(success)
+        done()
+      })
+      .req.on('response', function() {
+	      if (saved) success = true;
+      })
+    })
   })
 
   describe('when sid not in store', function () {

From 8b505927a5897b1a0ca3f1d2dc03bbcc4d365690 Mon Sep 17 00:00:00 2001
From: Patrik Simek <patrik@patriksimek.cz>
Date: Fri, 15 May 2015 17:56:35 +0200
Subject: [PATCH 2/6] Fix arguments + check if `res.redirect` exists before
 proxing it

---
 index.js | 118 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 61 insertions(+), 57 deletions(-)

diff --git a/index.js b/index.js
index dd24521e..da7c85d4 100644
--- a/index.js
+++ b/index.js
@@ -201,69 +201,73 @@ function session(options){
     });
     
     // proxy redirect() to commit the session
-    var _redirect = res.redirect;
     var redirected = false;
-    res.redirect = function redirect(url) {
-	  if (redirected) {
-        return;
-      }
-
-      redirected = true;
-
-      if (shouldDestroy(req)) {
-        // destroy session
-        debug('destroying');
-        store.destroy(req.sessionID, function ondestroy(err) {
-          if (err) {
-            defer(next, err);
-          }
-
-          debug('destroyed');
-          _redirect.call(res, url);
-        });
+    if ('function' == typeof(res.redirect)) {
+      var _redirect = res.redirect;
+      res.redirect = function redirect() {
+        var args = arguments;
         
-        return;
-      }
-      
-      // no session to save
-      if (!req.session) {
-        debug('no session');
-        _redirect.call(res, url);
+  	    if (redirected) {
+          return;
+        }
         
-        return;
-      }
-
-      // touch session
-      req.session.touch();
-      
-      if (shouldSave(req)) {
-        req.session.save(function onsave(err) {
-          if (err) {
-            defer(next, err);
-          }
-
-          _redirect.call(res, url);
-        });
+        redirected = true;
+  
+        if (shouldDestroy(req)) {
+          // destroy session
+          debug('destroying');
+          store.destroy(req.sessionID, function ondestroy(err) {
+            if (err) {
+              defer(next, err);
+            }
+  
+            debug('destroyed');
+            _redirect.apply(res, args);
+          });
+          
+          return;
+        }
         
-        return;
-      } else if (storeImplementsTouch && shouldTouch(req)) {
-        // store implements touch method
-        debug('touching');
-        store.touch(req.sessionID, req.session, function ontouch(err) {
-          if (err) {
-            defer(next, err);
-          }
-
-          debug('touched');
-          _redirect.call(res, url);
-        });
+        // no session to save
+        if (!req.session) {
+          debug('no session');
+          _redirect.apply(res, args);
+          
+          return;
+        }
+  
+        // touch session
+        req.session.touch();
         
-        return;
-      }
-      
-      return _redirect.call(res, url);
+        if (shouldSave(req)) {
+          req.session.save(function onsave(err) {
+            if (err) {
+              defer(next, err);
+            }
+  
+            _redirect.apply(res, args);
+          });
+          
+          return;
+        } else if (storeImplementsTouch && shouldTouch(req)) {
+          // store implements touch method
+          debug('touching');
+          store.touch(req.sessionID, req.session, function ontouch(err) {
+            if (err) {
+              defer(next, err);
+            }
+  
+            debug('touched');
+            _redirect.apply(res, args);
+          });
+          
+          return;
+        }
+        
+        return _redirect.apply(res, args);
+      };
     };
-
+    
     // proxy end() to commit the session
     var _end = res.end;
     var _write = res.write;

From ff6bc94719870e4aa19350da7d42f52eb3266fa4 Mon Sep 17 00:00:00 2001
From: Patrik Simek <patrik@patriksimek.cz>
Date: Sat, 16 May 2015 00:51:42 +0200
Subject: [PATCH 3/6] Option to buffer the response when `loation` header is
 set
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

… so the session can be saved before the redirect.
---
 README.md       |   8 ++++
 index.js        | 122 ++++++++++++++++++++++--------------------------
 test/session.js |  73 ++++++++++++++++++++++++-----
 3 files changed, 125 insertions(+), 78 deletions(-)

diff --git a/README.md b/README.md
index 239d49b7..9bb7dc3d 100644
--- a/README.md
+++ b/README.md
@@ -112,6 +112,14 @@ Force a cookie to be set on every response. This resets the expiration date.
 
 The default value is `false`.
 
+##### saveBeforeRedirect
+
+Tells the module to completly buffer the response when `location` header is set.
+This is because the browser is performing redirection immediately after the `location`
+header is parsed without waiting for the store to finish saving.
+
+The default value is `false`.
+
 ##### saveUninitialized
 
 Forces a session that is "uninitialized" to be saved to the store. A session is
diff --git a/index.js b/index.js
index da7c85d4..13bc6cfd 100644
--- a/index.js
+++ b/index.js
@@ -11,6 +11,7 @@
  * @private
  */
 
+var util = require('util');
 var cookie = require('cookie');
 var crc = require('crc').crc32;
 var debug = require('debug')('express-session');
@@ -89,7 +90,8 @@ function session(options){
     , cookie = options.cookie || {}
     , trustProxy = options.proxy
     , storeReady = true
-    , rollingSessions = options.rolling || false;
+    , rollingSessions = options.rolling || false
+    , saveBeforeLocationHeader = options.saveBeforeRedirect || false;
   var resaveSession = options.resave;
   var saveUninitializedSession = options.saveUninitialized;
   var secret = options.secret;
@@ -171,6 +173,9 @@ function session(options){
     var originalHash;
     var originalId;
     var savedHash;
+    var buffered;
+    var bufferedChunks;
+    var writeHeadLater;
 
     // expose store
     req.sessionStore = store;
@@ -200,77 +205,60 @@ function session(options){
       setcookie(res, name, req.sessionID, secrets[0], cookie.data);
     });
     
-    // proxy redirect() to commit the session
-    var redirected = false;
-    if ('function' == typeof(res.redirect)) {
-      var _redirect = res.redirect;
-      res.redirect = function redirect() {
-        var args = arguments;
+    var _write = res.write;
+
+    if (saveBeforeLocationHeader) {
+      var _writeHead = res.writeHead;
+      var headWritten = false;
+      res.writeHead = function writeHead(statusCode, reason, obj) {
+        if (headWritten) return;
+        headWritten = true;
         
-  	    if (redirected) {
-          return;
+        // search for location header only for 3xx status codes
+        if (statusCode < 300 && statusCode >= 400) {
+          return _writeHead.call(res, statusCode, reason, obj);
         }
         
-        redirected = true;
-  
-        if (shouldDestroy(req)) {
-          // destroy session
-          debug('destroying');
-          store.destroy(req.sessionID, function ondestroy(err) {
-            if (err) {
-              defer(next, err);
-            }
-  
-            debug('destroyed');
-            _redirect.apply(res, args);
-          });
-          
-          return;
+        if (!util.isString(reason)) {
+          obj = reason;
+          reason = undefined;
         }
         
-        // no session to save
-        if (!req.session) {
-          debug('no session');
-          _redirect.apply(res, args);
-          
-          return;
+        if (obj) {
+          var keys = Object.keys(obj);
+          for (var i = 0, l = keys.length; i < l; i++) {
+            var k = keys[i];
+            if (k) res.setHeader(k, obj[k]);
+          }
         }
-  
-        // touch session
-        req.session.touch();
         
-        if (shouldSave(req)) {
-          req.session.save(function onsave(err) {
-            if (err) {
-              defer(next, err);
-            }
-  
-            _redirect.apply(res, args);
-          });
-          
-          return;
-        } else if (storeImplementsTouch && shouldTouch(req)) {
-          // store implements touch method
-          debug('touching');
-          store.touch(req.sessionID, req.session, function ontouch(err) {
-            if (err) {
-              defer(next, err);
-            }
-  
-            debug('touched');
-            _redirect.apply(res, args);
-          });
-          
-          return;
+        // we have a `location` header so we must buffer all writes
+        if (res.getHeader('location') != null) {
+          debug('redirect found, buffering response')
+          buffered = true;
+          bufferedChunks = new Buffer(0);
+          writeHeadLater = function() { _writeHead.call(res, statusCode, reason); };
+        } else {
+          _writeHead.call(res, statusCode, reason);
         }
+      };
+      
+      var __write = _write;
+      res.write = _write = function write(chunk, encoding, callback) {
+        if (!headWritten) res.writeHead(this.statusCode);
         
-        return _redirect.apply(res, args);
+        if (buffered) {
+          bufferedChunks = Buffer.concat([bufferedChunks, !Buffer.isBuffer(chunk) ? new Buffer(chunk, encoding) : chunk]);
+          if ('function' === typeof(callback)) setImmediate(callback);
+          return true;
+        } else {
+          return __write.apply(res, arguments);
+        }
       };
     };
     
     // proxy end() to commit the session
     var _end = res.end;
-    var _write = res.write;
     var ended = false;
     res.end = function end(chunk, encoding) {
       if (ended) {
@@ -278,23 +266,23 @@ function session(options){
       }
 
       ended = true;
-      
-      if (redirected) {
-	    // we've done everything in res.redirect
-	    return _end.call(res, chunk, encoding);
-      }
 
       var ret;
       var sync = true;
 
       function writeend() {
         if (sync) {
-          ret = _end.call(res, chunk, encoding);
+          if (chunk) _write.call(res, chunk, encoding);
           sync = false;
-          return;
+        }
+        
+        if (buffered) {
+          if (writeHeadLater) writeHeadLater();
+          __write.call(res, bufferedChunks);
         }
 
-        _end.call(res);
+        ret = _end.call(res);
+        return ret;
       }
 
       function writetop() {
@@ -378,7 +366,7 @@ function session(options){
 
         return writetop();
       }
-
+      
       return _end.call(res, chunk, encoding);
     };
 
diff --git a/test/session.js b/test/session.js
index 8cf52b0b..e5472b23 100644
--- a/test/session.js
+++ b/test/session.js
@@ -367,19 +367,57 @@ describe('session()', function(){
         done()
       })
     })
+  })
+  
+  describe('when response redirect', function () {
+    it('should have saved session before location header is sent to the browser #1', function (done) {
+      var saved = false
+      var success = false
+      var store = new session.MemoryStore()
+      var server = createServer({ store: store, saveBeforeRedirect: true }, function (req, res) {
+        req.session.hit = true
+        res.setHeader('Location', 'http://xxx.com')
+        res.writeHead(308);
+        res.end('custom body');
+      })
 
-    it('should have saved session before before res.redirect sends location header', function (done) {
+      var _set = store.set
+      store.set = function set(sid, sess, callback) {
+        setTimeout(function () {
+          _set.call(store, sid, sess, function (err) {
+            saved = true
+            callback(err)
+          })
+        }, 200)
+      }
+
+      request(server)
+      .get('/')
+      .expect(shouldSetCookie('connect.sid'))
+      .expect('location', 'http://xxx.com')
+      .expect(308, 'custom body', function (err) {
+        if (err) return done(err)
+        assert.ok(success)
+        done()
+      })
+      .req.on('response', function() {
+	      if (saved) success = true;
+      })
+    })
+    
+    it('should have saved session before location header is sent to the browser #2', function (done) {
       var saved = false
       var success = false
       var store = new session.MemoryStore()
-      var app = express()
-        .use(session({ store: store, secret: 'keyboard cat', cookie: { maxAge: min }}))
-        .use(function(req, res){
-	      req.session.hit = true
-	      res.redirect('http://xxx.com');
-        });
-      app.set('env', 'test');
-      
+      var server = createServer({ store: store, saveBeforeRedirect: true }, function (req, res) {
+        req.session.hit = true
+        res.setHeader('Location', 'http://xxx.com')
+        res.statusCode = 308
+        res.write('a');
+        res.write('b');
+        res.end('c');
+      })
+
       var _set = store.set
       store.set = function set(sid, sess, callback) {
         setTimeout(function () {
@@ -390,11 +428,11 @@ describe('session()', function(){
         }, 200)
       }
 
-      request(app)
+      request(server)
       .get('/')
       .expect(shouldSetCookie('connect.sid'))
       .expect('location', 'http://xxx.com')
-      .expect(302, function (err) {
+      .expect(308, 'abc', function (err) {
         if (err) return done(err)
         assert.ok(success)
         done()
@@ -403,6 +441,19 @@ describe('session()', function(){
 	      if (saved) success = true;
       })
     })
+  
+    it('should have saved session before location header is sent to the browser #3 (synchronous store)', function(done){
+      var store = new SyncStore()
+      var server = createServer({ store: store, saveBeforeRedirect: true }, function (req, res) {
+        res.setHeader('Location', 'http://xxx.com')
+        res.statusCode = 308
+        res.end('response')
+      })
+
+      request(server)
+      .get('/')
+      .expect(308, 'response', done)
+    })
   })
 
   describe('when sid not in store', function () {

From 8b2267ddfb29835286d7bf899f18579060892b8b Mon Sep 17 00:00:00 2001
From: Patrik Simek <patrik@patriksimek.cz>
Date: Sat, 16 May 2015 01:08:39 +0200
Subject: [PATCH 4/6] Update tests

---
 index.js        |  7 +++----
 test/session.js | 45 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/index.js b/index.js
index 13bc6cfd..673736db 100644
--- a/index.js
+++ b/index.js
@@ -11,7 +11,6 @@
  * @private
  */
 
-var util = require('util');
 var cookie = require('cookie');
 var crc = require('crc').crc32;
 var debug = require('debug')('express-session');
@@ -213,13 +212,13 @@ function session(options){
       res.writeHead = function writeHead(statusCode, reason, obj) {
         if (headWritten) return;
         headWritten = true;
-        
+
         // search for location header only for 3xx status codes
-        if (statusCode < 300 && statusCode >= 400) {
+        if (statusCode < 300 || statusCode >= 400) {
           return _writeHead.call(res, statusCode, reason, obj);
         }
         
-        if (!util.isString(reason)) {
+        if ('string' != typeof(reason)) {
           obj = reason;
           reason = undefined;
         }
diff --git a/test/session.js b/test/session.js
index e5472b23..a4a9731b 100644
--- a/test/session.js
+++ b/test/session.js
@@ -369,15 +369,14 @@ describe('session()', function(){
     })
   })
   
-  describe('when response redirect', function () {
-    it('should have saved session before location header is sent to the browser #1', function (done) {
+  describe('when location header is set', function () {
+    it('should buffer the response #1', function (done) {
       var saved = false
       var success = false
       var store = new session.MemoryStore()
       var server = createServer({ store: store, saveBeforeRedirect: true }, function (req, res) {
         req.session.hit = true
-        res.setHeader('Location', 'http://xxx.com')
-        res.writeHead(308);
+        res.writeHead(308, {location: 'http://xxx.com'});
         res.end('custom body');
       })
 
@@ -405,7 +404,7 @@ describe('session()', function(){
       })
     })
     
-    it('should have saved session before location header is sent to the browser #2', function (done) {
+    it('should buffer the response #2', function (done) {
       var saved = false
       var success = false
       var store = new session.MemoryStore()
@@ -442,7 +441,7 @@ describe('session()', function(){
       })
     })
   
-    it('should have saved session before location header is sent to the browser #3 (synchronous store)', function(done){
+    it('should buffer the response #3 (synchronous store)', function(done){
       var store = new SyncStore()
       var server = createServer({ store: store, saveBeforeRedirect: true }, function (req, res) {
         res.setHeader('Location', 'http://xxx.com')
@@ -454,6 +453,40 @@ describe('session()', function(){
       .get('/')
       .expect(308, 'response', done)
     })
+  
+    it('should not buffer the response', function(done){
+      var saved = false
+      var success = true
+      var store = new session.MemoryStore()
+      var server = createServer({ store: store, saveBeforeRedirect: true }, function (req, res) {
+        req.session.hit = true
+        res.writeHead(200, {location: 'http://xxx.com'});
+        res.end('custom body');
+      })
+
+      var _set = store.set
+      store.set = function set(sid, sess, callback) {
+        setTimeout(function () {
+          _set.call(store, sid, sess, function (err) {
+            saved = true
+            callback(err)
+          })
+        }, 200)
+      }
+
+      request(server)
+      .get('/')
+      .expect(shouldSetCookie('connect.sid'))
+      .expect('location', 'http://xxx.com')
+      .expect(200, 'custom body', function (err) {
+        if (err) return done(err)
+        assert.ok(success)
+        done()
+      })
+      .req.on('response', function() {
+	      if (saved) success = false;
+      })
+    })
   })
 
   describe('when sid not in store', function () {

From 5423b10837c24823a4830e233846d11ec70db442 Mon Sep 17 00:00:00 2001
From: Patrik Simek <patrik@patriksimek.cz>
Date: Sat, 16 May 2015 01:13:16 +0200
Subject: [PATCH 5/6] Add more tests

---
 test/session.js | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/test/session.js b/test/session.js
index a4a9731b..ad7961fe 100644
--- a/test/session.js
+++ b/test/session.js
@@ -454,7 +454,7 @@ describe('session()', function(){
       .expect(308, 'response', done)
     })
   
-    it('should not buffer the response', function(done){
+    it('should not buffer the response #1', function(done){
       var saved = false
       var success = true
       var store = new session.MemoryStore()
@@ -487,6 +487,39 @@ describe('session()', function(){
 	      if (saved) success = false;
       })
     })
+  
+    it('should not buffer the response #2', function(done){
+      var saved = false
+      var success = true
+      var store = new session.MemoryStore()
+      var server = createServer({ store: store, saveBeforeRedirect: true }, function (req, res) {
+        req.session.hit = true
+        res.writeHead(302);
+        res.end('custom body');
+      })
+
+      var _set = store.set
+      store.set = function set(sid, sess, callback) {
+        setTimeout(function () {
+          _set.call(store, sid, sess, function (err) {
+            saved = true
+            callback(err)
+          })
+        }, 200)
+      }
+
+      request(server)
+      .get('/')
+      .expect(shouldSetCookie('connect.sid'))
+      .expect(302, 'custom body', function (err) {
+        if (err) return done(err)
+        assert.ok(success)
+        done()
+      })
+      .req.on('response', function() {
+	      if (saved) success = false;
+      })
+    })
   })
 
   describe('when sid not in store', function () {

From 6a4dc7b38e7030ca23880d54d0c84720ebbb5db7 Mon Sep 17 00:00:00 2001
From: Patrik Simek <patrik@patriksimek.cz>
Date: Fri, 19 Jun 2015 03:22:45 +0200
Subject: [PATCH 6/6] Set statusCode immediately in fake writeHead method

---
 index.js | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/index.js b/index.js
index 673736db..bef22db8 100644
--- a/index.js
+++ b/index.js
@@ -218,6 +218,8 @@ function session(options){
           return _writeHead.call(res, statusCode, reason, obj);
         }
         
+        this.statusCode = statusCode;
+        
         if ('string' != typeof(reason)) {
           obj = reason;
           reason = undefined;