Fix uncaught error from bad session data

closes #634
expressjs · Apr 11, 2019 · 85682a2 · 85682a2
1 parent 1e3fc39
commit 85682a2
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 31 deletions.
diff --git a/HISTORY.md b/HISTORY.md
@@ -2,6 +2,7 @@ unreleased
 ==========
 
   * Fix error passing `data` option to `Cookie` constructor
+  * Fix uncaught error from bad session data
 
 1.16.0 / 2019-04-10
 ===================

diff --git a/index.js b/index.js
@@ -363,6 +363,19 @@ function session(options) {
       wrapmethods(req.session);
     }
 
+    // inflate the session
+    function inflate (req, sess) {
+      store.createSession(req, sess)
+      originalId = req.sessionID
+      originalHash = hash(sess)
+
+      if (!resaveSession) {
+        savedHash = originalHash
+      }
+
+      wrapmethods(req.session)
+    }
+
     // wrap session methods
     function wrapmethods(sess) {
       var _reload = sess.reload
@@ -460,34 +473,26 @@ function session(options) {
     debug('fetching %s', req.sessionID);
     store.get(req.sessionID, function(err, sess){
       // error handling
-      if (err) {
+      if (err && err.code !== 'ENOENT') {
         debug('error %j', err);
+        next(err)
+        return
+      }
 
-        if (err.code !== 'ENOENT') {
-          next(err);
-          return;
-        }
-
-        generate();
-      // no session
-      } else if (!sess) {
-        debug('no session found');
-        generate();
-      // populate req.session
-      } else {
-        debug('session found');
-        store.createSession(req, sess);
-        originalId = req.sessionID;
-        originalHash = hash(sess);
-
-        if (!resaveSession) {
-          savedHash = originalHash
+      try {
+        if (err || !sess) {
+          debug('no session found')
+          generate()
+        } else {
+          debug('session found')
+          inflate(req, sess)
         }
-
-        wrapmethods(req.session);
+      } catch (e) {
+        next(e)
+        return
       }
 
-      next();
+      next()
     });
   };
 };

diff --git a/session/memory.js b/session/memory.js
@@ -171,14 +171,16 @@ function getSession(sessionId) {
   // parse
   sess = JSON.parse(sess)
 
-  var expires = typeof sess.cookie.expires === 'string'
-    ? new Date(sess.cookie.expires)
-    : sess.cookie.expires
-
-  // destroy expired session
-  if (expires && expires <= Date.now()) {
-    delete this.sessions[sessionId]
-    return
+  if (sess.cookie) {
+    var expires = typeof sess.cookie.expires === 'string'
+      ? new Date(sess.cookie.expires)
+      : sess.cookie.expires
+
+    // destroy expired session
+    if (expires && expires <= Date.now()) {
+      delete this.sessions[sessionId]
+      return
+    }
   }
 
   return sess

diff --git a/test/session.js b/test/session.js
@@ -610,6 +610,31 @@ describe('session()', function(){
     })
   })
 
+  describe('when session without cookie property in store', function () {
+    it('should pass error from inflate', function (done) {
+      var count = 0
+      var store = new session.MemoryStore()
+      var server = createServer({ store: store }, function (req, res) {
+        req.session.num = req.session.num || ++count
+        res.end('session ' + req.session.num)
+      })
+
+      request(server)
+      .get('/')
+      .expect(shouldSetCookie('connect.sid'))
+      .expect(200, 'session 1', function (err, res) {
+        if (err) return done(err)
+        store.set(sid(res), { foo: 'bar' }, function (err) {
+          if (err) return done(err)
+          request(server)
+          .get('/')
+          .set('Cookie', cookie(res))
+          .expect(500, /Cannot read property/, done)
+        })
+      })
+    })
+  })
+
   describe('proxy option', function(){
     describe('when enabled', function(){
       var server