dont allow login with invalid ip, escape ip display on user/info page

src/controllers/authentication.js

@@ -6,6 +6,7 @@ var passport = require('passport');
 var nconf = require('nconf');
 var validator = require('validator');
 var _ = require('lodash');
+var ipaddr = require('ipaddr.js');
 
 var db = require('../database');
 var meta = require('../meta');
@@ -289,26 +290,30 @@ authenticationController.doLogin = function (req, uid, callback) {
 
 authenticationController.onSuccessfulLogin = function (req, uid, callback) {
 	var uuid = utils.generateUUID();
-	req.session.meta = {};
-
-	delete req.session.forceLogin;
-
-	// Associate IP used during login with user account
-	user.logIP(uid, req.ip);
-	req.session.meta.ip = req.ip;
-
-	// Associate metadata retrieved via user-agent
-	req.session.meta = _.extend(req.session.meta, {
-		uuid: uuid,
-		datetime: Date.now(),
-		platform: req.useragent.platform,
-		browser: req.useragent.browser,
-		version: req.useragent.version,
-	});
 
 	async.waterfall([
-		async.apply(meta.blacklist.test, req.ip),
 		function (next) {
+			meta.blacklist.test(req.ip, next);
+		},
+		function (next) {
+			user.logIP(uid, req.ip, next);
+		},
+		function (next) {
+			req.session.meta = {};
+
+			delete req.session.forceLogin;
+			// Associate IP used during login with user account
+			req.session.meta.ip = req.ip;
+
+			// Associate metadata retrieved via user-agent
+			req.session.meta = _.extend(req.session.meta, {
+				uuid: uuid,
+				datetime: Date.now(),
+				platform: req.useragent.platform,
+				browser: req.useragent.browser,
+				version: req.useragent.version,
+			});
+
 			async.parallel([
 				function (next) {
 					user.auth.addSession(uid, req.sessionID, next);

src/meta/blacklist.js

@@ -68,7 +68,12 @@ Blacklist.test = function (clientIp, callback) {
 	// clientIp = '127.0.15.1:3443'; // IPv4 with port strip port to not fail
 	clientIp = clientIp.split(':').length === 2 ? clientIp.split(':')[0] : clientIp;
 
-	var addr = ipaddr.parse(clientIp);
+	var addr;
+	try {
+		addr = ipaddr.parse(clientIp);
+	} catch (err) {
+		return callback(err);
+	}
 
 	if (
 		Blacklist._rules.ipv4.indexOf(clientIp) === -1 &&	// not explicitly specified in ipv4 list
@@ -88,11 +93,7 @@ Blacklist.test = function (clientIp, callback) {
 				analytics.increment('blacklist');
 			}
 
-			if (typeof callback === 'function') {
-				callback(err);
-			} else {
-				return !!err;
-			}
+			callback(err);
 		});
 	} else {
 		var err = new Error('[[error:blacklisted-ip]]');
@@ -100,11 +101,7 @@ Blacklist.test = function (clientIp, callback) {
 
 		analytics.increment('blacklist');
 
-		if (typeof callback === 'function') {
-			setImmediate(callback, err);
-		} else {
-			return true;
-		}
+		setImmediate(callback, err);
 	}
 };
 

src/user/admin.js

@@ -2,21 +2,41 @@
 'use strict';
 
 var async = require('async');
+var winston = require('winston');
+var validator = require('validator');
+
 var db = require('../database');
 var plugins = require('../plugins');
-var winston = require('winston');
 
 module.exports = function (User) {
-	User.logIP = function (uid, ip) {
+	User.logIP = function (uid, ip, callback) {
 		var now = Date.now();
-		db.sortedSetAdd('uid:' + uid + ':ip', now, ip || 'Unknown');
-		if (ip) {
-			db.sortedSetAdd('ip:' + ip + ':uid', now, uid);
-		}
+		async.waterfall([
+			function (next) {
+				db.sortedSetAdd('uid:' + uid + ':ip', now, ip || 'Unknown', next);
+			},
+			function (next) {
+				if (ip) {
+					db.sortedSetAdd('ip:' + ip + ':uid', now, uid, next);
+				} else {
+					next();
+				}
+			},
+		], callback);
 	};
 
 	User.getIPs = function (uid, stop, callback) {
-		db.getSortedSetRevRange('uid:' + uid + ':ip', 0, stop, callback);
+		async.waterfall([
+			function (next) {
+				db.getSortedSetRevRange('uid:' + uid + ':ip', 0, stop, next);
+			},
+			function (ips, next) {
+				ips = ips.map(function (ip) {
+					return validator.escape(String(ip));
+				});
+				next(null, ips);
+			},
+		], callback);
 	};
 
 	User.getUsersCSV = function (callback) {

src/user/auth.js

@@ -2,6 +2,7 @@
 
 var async = require('async');
 var winston = require('winston');
+var validator = require('validator');
 var db = require('../database');
 var meta = require('../meta');
 var events = require('../events');
@@ -126,12 +127,15 @@ module.exports = function (User) {
 					next(err, sessions);
 				});
 			},
-		], function (err, sessions) {
-			callback(err, sessions ? sessions.map(function (sessObj) {
-				sessObj.meta.datetimeISO = new Date(sessObj.meta.datetime).toISOString();
-				return sessObj.meta;
-			}) : undefined);
-		});
+			function (sessions, next) {
+				sessions = sessions.map(function (sessObj) {
+					sessObj.meta.datetimeISO = new Date(sessObj.meta.datetime).toISOString();
+					sessObj.meta.ip = validator.escape(String(sessObj.meta.ip));
+					return sessObj.meta;
+				});
+				next(null, sessions);
+			},
+		], callback);
 	};
 
 	User.auth.addSession = function (uid, sessionId, callback) {

test/authentication.js

@@ -235,6 +235,36 @@ describe('authentication', function () {
 		});
 	});
 
+	it('should fail to login if ip address if invalid', function (done) {
+		var jar = request.jar();
+		request({
+			url: nconf.get('url') + '/api/config',
+			json: true,
+			jar: jar,
+		}, function (err, response, body) {
+			if (err) {
+				return done(err);
+			}
+
+			request.post(nconf.get('url') + '/login', {
+				form: {
+					username: 'regular',
+					password: 'regularpwd',
+				},
+				json: true,
+				jar: jar,
+				headers: {
+					'x-csrf-token': body.csrf_token,
+					'x-forwarded-for': '<script>alert("xss")</script>',
+				},
+			}, function (err, response, body) {
+				assert.ifError(err);
+				assert.equal(response.statusCode, 500);
+				done();
+			});
+		});
+	});
+
 	it('should fail to login if user does not exist', function (done) {
 		loginUser('doesnotexist', 'nopassword', function (err, response, body) {
 			assert.ifError(err);

test/blacklist.js

@@ -49,32 +49,24 @@ describe('blacklist', function () {
 		});
 	});
 
-	it('should pass ip test against blacklist async', function (done) {
+	it('should pass ip test against blacklist', function (done) {
 		blacklist.test('3.3.3.3', function (err) {
 			assert.ifError(err);
 			done();
 		});
 	});
 
-	it('should pass ip test against blacklist sync', function (done) {
-		assert(!blacklist.test('3.3.3.3'));
-		done();
-	});
-
-	it('should fail ip test against blacklist async', function (done) {
+	it('should fail ip test against blacklist', function (done) {
 		blacklist.test('1.1.1.1', function (err) {
 			assert.equal(err.message, '[[error:blacklisted-ip]]');
 			done();
 		});
 	});
 
-	it('should fail ip test against blacklist sync', function (done) {
-		assert(blacklist.test('1.1.1.1'));
-		done();
-	});
-
 	it('should pass ip test and not crash with ipv6 address', function (done) {
-		assert(!blacklist.test('2001:db8:85a3:0:0:8a2e:370:7334'));
-		done();
+		blacklist.test('2001:db8:85a3:0:0:8a2e:370:7334', function (err) {
+			assert.ifError(err);
+			done();
+		});
 	});
 });