Merge pull request #900 from jaredhanson/fix-fixation

Address Session Fixation Concerns
jaredhanson · May 20, 2022 · 42630cb · 42630cb
2 parents 5e6d92f + 8dd79fe
commit 42630cb
Show file tree

Hide file tree

Showing 4 changed files with 724 additions and 27 deletions.
diff --git a/lib/http/request.js b/lib/http/request.js
@@ -36,7 +36,7 @@ req.logIn = function(user, options, done) {
     if (typeof done != 'function') { throw new Error('req#login requires a callback function'); }
 
     var self = this;
-    this._sessionManager.logIn(this, user, function(err) {
+    this._sessionManager.logIn(this, user, options, function(err) {
       if (err) { self[property] = null; return done(err); }
       done();
     });
@@ -51,12 +51,22 @@ req.logIn = function(user, options, done) {
  * @api public
  */
 req.logout =
-req.logOut = function() {
+req.logOut = function(options, done) {
+  if (typeof options == 'function') {
+    done = options;
+    options = {};
+  }
+  options = options || {};
+
   var property = this._userProperty || 'user';
 
   this[property] = null;
   if (this._sessionManager) {
-    this._sessionManager.logOut(this);
+    if (typeof done != 'function') { throw new Error('req#logout requires a callback function'); }
+
+    this._sessionManager.logOut(this, options, done);
+  } else {
+    done && done();
   }
 };
 

diff --git a/lib/sessionmanager.js b/lib/sessionmanager.js
@@ -1,3 +1,5 @@
+var merge = require('utils-merge');
+
 function SessionManager(options, serializeUser) {
   if (typeof options == 'function') {
     serializeUser = options;
@@ -9,30 +11,85 @@ function SessionManager(options, serializeUser) {
   this._serializeUser = serializeUser;
 }
 
-SessionManager.prototype.logIn = function(req, user, cb) {
+SessionManager.prototype.logIn = function(req, user, options, cb) {
+  if (typeof options == 'function') {
+    cb = options;
+    options = {};
+  }
+  options = options || {};
+
+  if (!req.session) { return cb(new Error('Login sessions require session support. Did you forget to use `express-session` middleware?')); }
+
   var self = this;
-  this._serializeUser(user, req, function(err, obj) {
+  var prevSession = req.session;
+
+  // regenerate the session, which is good practice to help
+  // guard against forms of session fixation
+  req.session.regenerate(function(err) {
     if (err) {
       return cb(err);
     }
-    // TODO: Error if session isn't available here.
-    if (!req.session) {
-      req.session = {};
-    }
-    if (!req.session[self._key]) {
-      req.session[self._key] = {};
-    }
-    req.session[self._key].user = obj;
-    cb();
+
+    self._serializeUser(user, req, function(err, obj) {
+      if (err) {
+        return cb(err);
+      }
+      if (options.keepSessionInfo) {
+        merge(req.session, prevSession);
+      }
+      if (!req.session[self._key]) {
+        req.session[self._key] = {};
+      }
+      // store user information in session, typically a user id
+      req.session[self._key].user = obj;
+      // save the session before redirection to ensure page
+      // load does not happen before session is saved
+      req.session.save(function(err) {
+        if (err) {
+          return cb(err);
+        }
+        cb();
+      });
+    });
   });
 }
 
-SessionManager.prototype.logOut = function(req, cb) {
-  if (req.session && req.session[this._key]) {
+SessionManager.prototype.logOut = function(req, options, cb) {
+  if (typeof options == 'function') {
+    cb = options;
+    options = {};
+  }
+  options = options || {};
+
+  if (!req.session) { return cb(new Error('Login sessions require session support. Did you forget to use `express-session` middleware?')); }
+
+  var self = this;
+
+  // clear the user from the session object and save.
+  // this will ensure that re-using the old session id
+  // does not have a logged in user
+  if (req.session[this._key]) {
     delete req.session[this._key].user;
   }
+  var prevSession = req.session;
 
-  cb && cb();
+  req.session.save(function(err) {
+    if (err) {
+      return cb(err)
+    }
+
+    // regenerate the session, which is good practice to help
+    // guard against forms of session fixation
+    req.session.regenerate(function(err) {
+      if (err) {
+        return cb(err);
+      }
+      if (options.keepSessionInfo) {
+        merge(req.session, prevSession);
+      }
+      cb();
+    });
+  });
 }
 
 

diff --git a/package.json b/package.json
@@ -36,7 +36,8 @@
   "main": "./lib",
   "dependencies": {
     "passport-strategy": "1.x.x",
-    "pause": "0.0.1"
+    "pause": "0.0.1",
+    "utils-merge": "^1.0.1"
   },
   "devDependencies": {
     "make-node": "0.3.x",