fix: group create/join/update name validation

src/groups/create.js

@@ -16,7 +16,7 @@ module.exports = function (Groups) {
 		const disableLeave = parseInt(data.disableLeave, 10) === 1 ? 1 : 0;
 		const isHidden = parseInt(data.hidden, 10) === 1;
 
-		validateGroupName(data.name);
+		Groups.validateGroupName(data.name);
 
 		const exists = await meta.userOrGroupExists(data.name);
 		if (exists) {
@@ -72,11 +72,15 @@ module.exports = function (Groups) {
 			Groups.isPrivilegeGroup(data.name);
 	}
 
-	function validateGroupName(name) {
+	Groups.validateGroupName = function (name) {
 		if (!name) {
 			throw new Error('[[error:group-name-too-short]]');
 		}
 
+		if (typeof name !== 'string') {
+			throw new Error('[[error:invalid-group-name]]');
+		}
+
 		if (!Groups.isPrivilegeGroup(name) && name.length > meta.config.maximumGroupNameLength) {
 			throw new Error('[[error:group-name-too-long]]');
 		}
@@ -88,5 +92,5 @@ module.exports = function (Groups) {
 		if (name.includes('/') || !utils.slugify(name)) {
 			throw new Error('[[error:invalid-group-name]]');
 		}
-	}
+	};
 };

src/groups/update.js

@@ -54,7 +54,10 @@ module.exports = function (Groups) {
 			payload.disableLeave = values.disableLeave ? '1' : '0';
 		}
 
-		await checkNameChange(groupName, values.name);
+		if (values.hasOwnProperty('name')) {
+			await checkNameChange(groupName, values.name);
+		}
+
 		if (values.hasOwnProperty('private')) {
 			await updatePrivacy(groupName, values.private);
 		}
@@ -125,6 +128,10 @@ module.exports = function (Groups) {
 	}
 
 	async function checkNameChange(currentName, newName) {
+		Groups.validateGroupName(newName);
+		if (Groups.isPrivilegeGroup(newName)) {
+			throw new Error('[[error:invalid-group-name]]');
+		}
 		const currentSlug = utils.slugify(currentName);
 		const newSlug = utils.slugify(newName);
 		if (currentName === newName || currentSlug === newSlug) {

src/socket.io/groups.js

@@ -22,6 +22,10 @@ SocketGroups.join = async (socket, data) => {
 		throw new Error('[[error:invalid-uid]]');
 	}
 
+	if (typeof data.groupName !== 'string') {
+		throw new Error('[[error:invalid-group-name]]');
+	}
+
 	if (data.groupName === 'administrators' || groups.isPrivilegeGroup(data.groupName)) {
 		throw new Error('[[error:not-allowed]]');
 	}
@@ -66,6 +70,10 @@ SocketGroups.leave = async (socket, data) => {
 		throw new Error('[[error:invalid-uid]]');
 	}
 
+	if (typeof data.groupName !== 'string') {
+		throw new Error('[[error:invalid-group-name]]');
+	}
+
 	if (data.groupName === 'administrators') {
 		throw new Error('[[error:cant-remove-self-as-admin]]');
 	}
@@ -104,6 +112,9 @@ SocketGroups.addMember = async (socket, data) => {
 };
 
 async function isOwner(socket, data) {
+	if (typeof data.groupName !== 'string') {
+		throw new Error('[[error:invalid-group-name]]');
+	}
 	const results = await utils.promiseParallel({
 		isAdmin: await user.isAdministrator(socket.uid),
 		isGlobalModerator: await user.isGlobalModerator(socket.uid),
@@ -118,6 +129,9 @@ async function isOwner(socket, data) {
 }
 
 async function isInvited(socket, data) {
+	if (typeof data.groupName !== 'string') {
+		throw new Error('[[error:invalid-group-name]]');
+	}
 	const invited = await groups.isInvited(socket.uid, data.groupName);
 	if (!invited) {
 		throw new Error('[[error:not-invited]]');
@@ -171,6 +185,9 @@ SocketGroups.rejectAll = async (socket, data) => {
 };
 
 async function acceptRejectAll(method, socket, data) {
+	if (typeof data.groupName !== 'string') {
+		throw new Error('[[error:invalid-group-name]]');
+	}
 	const uids = await groups.getPending(data.groupName);
 	await Promise.all(uids.map(async (uid) => {
 		await method(socket, { groupName: data.groupName, toUid: uid });
@@ -251,7 +268,7 @@ SocketGroups.kick = async (socket, data) => {
 SocketGroups.create = async (socket, data) => {
 	if (!socket.uid) {
 		throw new Error('[[error:no-privileges]]');
-	} else if (groups.isPrivilegeGroup(data.name)) {
+	} else if (typeof data.name !== 'string' || groups.isPrivilegeGroup(data.name)) {
 		throw new Error('[[error:invalid-group-name]]');
 	}
 
@@ -260,6 +277,7 @@ SocketGroups.create = async (socket, data) => {
 		throw new Error('[[error:no-privileges]]');
 	}
 	data.ownerUid = socket.uid;
+	data.system = false;
 	const groupData = await groups.create(data);
 	logGroupEvent(socket, 'group-create', {
 		groupName: data.name,
@@ -338,7 +356,6 @@ SocketGroups.cover.update = async (socket, data) => {
 	if (!socket.uid) {
 		throw new Error('[[error:no-privileges]]');
 	}
-
 	await canModifyGroup(socket.uid, data.groupName);
 	return await groups.updateCover(socket.uid, data);
 };
@@ -353,6 +370,9 @@ SocketGroups.cover.remove = async (socket, data) => {
 };
 
 async function canModifyGroup(uid, groupName) {
+	if (typeof groupName !== 'string') {
+		throw new Error('[[error:invalid-group-name]]');
+	}
 	const results = await utils.promiseParallel({
 		isOwner: groups.ownership.isOwner(uid, groupName),
 		isAdminOrGlobalMod: user.isAdminOrGlobalMod(uid),

test/groups.js

@@ -9,6 +9,8 @@ var db = require('./mocks/databasemock');
 var helpers = require('./helpers');
 var Groups = require('../src/groups');
 var User = require('../src/user');
+var socketGroups = require('../src/socket.io/groups');
+var meta = require('../src/meta');
 
 describe('Groups', function () {
 	var adminUid;
@@ -360,6 +362,31 @@ describe('Groups', function () {
 			});
 		});
 
+		it('should fail if group name is invalid', function (done) {
+			Groups.create({ name: ['array/'] }, function (err) {
+				assert.equal(err.message, '[[error:invalid-group-name]]');
+				done();
+			});
+		});
+
+		it('should fail if group name is invalid', function (done) {
+			socketGroups.create({ uid: adminUid }, { name: ['test', 'administrators'] }, function (err) {
+				assert.equal(err.message, '[[error:invalid-group-name]]');
+				done();
+			});
+		});
+
+		it('should not create a system group', function (done) {
+			socketGroups.create({ uid: adminUid }, { name: 'mysystemgroup', system: true }, function (err) {
+				assert.ifError(err);
+				Groups.getGroupData('mysystemgroup', function (err, data) {
+					assert.ifError(err);
+					assert.strictEqual(data.system, 0);
+					done();
+				});
+			});
+		});
+
 		it('should fail if group name is invalid', function (done) {
 			Groups.create({ name: 'not:valid' }, function (err) {
 				assert.equal(err.message, '[[error:invalid-group-name]]');
@@ -444,6 +471,62 @@ describe('Groups', function () {
 			});
 		});
 
+		it('should fail to rename if group name is invalid', function (done) {
+			socketGroups.update({ uid: adminUid }, { groupName: ['updateTestGroup?'], values: {} }, function (err) {
+				assert.strictEqual(err.message, '[[error:invalid-group-name]]');
+				done();
+			});
+		});
+
+		it('should fail to rename if group name is too short', function (done) {
+			socketGroups.update({ uid: adminUid }, { groupName: 'updateTestGroup?', values: { name: '' } }, function (err) {
+				assert.strictEqual(err.message, '[[error:group-name-too-short]]');
+				done();
+			});
+		});
+
+		it('should fail to rename if group name is invalid', function (done) {
+			socketGroups.update({ uid: adminUid }, { groupName: 'updateTestGroup?', values: { name: ['invalid'] } }, function (err) {
+				assert.strictEqual(err.message, '[[error:invalid-group-name]]');
+				done();
+			});
+		});
+
+		it('should fail to rename if group name is invalid', function (done) {
+			socketGroups.update({ uid: adminUid }, { groupName: 'updateTestGroup?', values: { name: 'cid:0:privileges:ban' } }, function (err) {
+				assert.strictEqual(err.message, '[[error:invalid-group-name]]');
+				done();
+			});
+		});
+
+		it('should fail to rename if group name is too long', function (done) {
+			socketGroups.update({ uid: adminUid }, { groupName: 'updateTestGroup?', values: { name: 'verylongstringverylongstringverylongstringverylongstringverylongstringverylongstringverylongstringverylongstringverylongstringverylongstringverylongstringverylongstringverylongstringverylongstringverylongstringverylongstringverylongstringverylongstringverylongstringverylongstring' } }, function (err) {
+				assert.strictEqual(err.message, '[[error:group-name-too-long]]');
+				done();
+			});
+		});
+
+		it('should fail to rename if group name is invalid', function (done) {
+			socketGroups.update({ uid: adminUid }, { groupName: 'updateTestGroup?', values: { name: 'test:test' } }, function (err) {
+				assert.strictEqual(err.message, '[[error:invalid-group-name]]');
+				done();
+			});
+		});
+
+		it('should fail to rename if group name is invalid', function (done) {
+			socketGroups.update({ uid: adminUid }, { groupName: 'updateTestGroup?', values: { name: 'another/test' } }, function (err) {
+				assert.strictEqual(err.message, '[[error:invalid-group-name]]');
+				done();
+			});
+		});
+
+		it('should fail to rename if group name is invalid', function (done) {
+			socketGroups.update({ uid: adminUid }, { groupName: 'updateTestGroup?', values: { name: '---' } }, function (err) {
+				assert.strictEqual(err.message, '[[error:invalid-group-name]]');
+				done();
+			});
+		});
+
 		it('should fail to rename group to an existing group', function (done) {
 			Groups.create({
 				name: 'group2',
@@ -538,6 +621,20 @@ describe('Groups', function () {
 			});
 		});
 
+		it('should fail to add user to admin group', async function () {
+			const oldValue = meta.config.allowPrivateGroups;
+			try {
+				meta.config.allowPrivateGroups = false;
+				const newUid = await User.create({ username: 'newadmin' });
+				await socketGroups.join({ uid: newUid }, { groupName: ['test', 'administrators'], uid: newUid }, 1);
+				const isMember = await Groups.isMember(newUid, 'administrators');
+				assert(!isMember);
+			} catch (err) {
+				assert.strictEqual(err.message, '[[error:invalid-group-name]]');
+			}
+			meta.config.allowPrivateGroups = oldValue;
+		});
+
 		it('should fail to add user to group if group name is invalid', function (done) {
 			Groups.join(0, 1, function (err) {
 				assert.equal(err.message, '[[error:invalid-data]]');
@@ -667,11 +764,7 @@ describe('Groups', function () {
 		});
 	});
 
-
 	describe('socket methods', function () {
-		var socketGroups = require('../src/socket.io/groups');
-		var meta = require('../src/meta');
-
 		it('should error if data is null', function (done) {
 			socketGroups.before({ uid: 0 }, 'groups.join', null, function (err) {
 				assert.equal(err.message, '[[error:invalid-data]]');
@@ -1012,7 +1105,7 @@ describe('Groups', function () {
 		});
 
 		it('should fail to create group if group creation is disabled', function (done) {
-			socketGroups.create({ uid: testUid }, {}, function (err) {
+			socketGroups.create({ uid: testUid }, { name: 'avalidname' }, function (err) {
 				assert.equal(err.message, '[[error:no-privileges]]');
 				done();
 			});
@@ -1296,7 +1389,7 @@ describe('Groups', function () {
 		it('should fail if user is not logged in or not owner', function (done) {
 			socketGroups.cover.update({ uid: 0 }, {}, function (err) {
 				assert.equal(err.message, '[[error:no-privileges]]');
-				socketGroups.cover.update({ uid: regularUid }, {}, function (err) {
+				socketGroups.cover.update({ uid: regularUid }, { groupName: 'Test' }, function (err) {
 					assert.equal(err.message, '[[error:no-privileges]]');
 					done();
 				});