fix: validate username before registering user

.changeset/purple-dolls-serve.md

@@ -0,0 +1,7 @@
+---
+'@rocket.chat/web-ui-registration': patch
+'@rocket.chat/i18n': patch
+'@rocket.chat/meteor': patch
+---
+
+Fixes an issue where creating a new user with an invalid username (containing special characters) resulted in an error message, but the user was still created. The user creation process now properly aborts when an invalid username is provided.

apps/meteor/app/api/server/v1/users.ts

@@ -45,6 +45,7 @@ import { setUserAvatar } from '../../../lib/server/functions/setUserAvatar';
 import { setUsernameWithValidation } from '../../../lib/server/functions/setUsername';
 import { validateCustomFields } from '../../../lib/server/functions/validateCustomFields';
 import { validateNameChars } from '../../../lib/server/functions/validateNameChars';
+import { validateUsername } from '../../../lib/server/functions/validateUsername';
 import { notifyOnUserChange, notifyOnUserChangeAsync } from '../../../lib/server/lib/notifyListener';
 import { generateAccessToken } from '../../../lib/server/methods/createToken';
 import { settings } from '../../../settings/server';
@@ -651,6 +652,10 @@ API.v1.addRoute(
 				return API.v1.failure('Name contains invalid characters');
 			}
 
+			if (!validateUsername(this.bodyParams.username)) {
+				return API.v1.failure(`The username provided is not valid`);
+			}
+
 			if (!(await checkUsernameAvailability(this.bodyParams.username))) {
 				return API.v1.failure('Username is already in use');
 			}

apps/meteor/app/lib/server/functions/setUsername.ts

@@ -17,6 +17,7 @@ import { getAvatarSuggestionForUser } from './getAvatarSuggestionForUser';
 import { joinDefaultChannels } from './joinDefaultChannels';
 import { saveUserIdentity } from './saveUserIdentity';
 import { setUserAvatar } from './setUserAvatar';
+import { validateUsername } from './validateUsername';
 
 export const setUsernameWithValidation = async (userId: string, username: string, joinDefaultChannelsSilenced?: boolean): Promise<void> => {
 	if (!username) {
@@ -37,14 +38,7 @@ export const setUsernameWithValidation = async (userId: string, username: string
 		return;
 	}
 
-	let nameValidation;
-	try {
-		nameValidation = new RegExp(`^${settings.get('UTF8_User_Names_Validation')}$`);
-	} catch (error) {
-		nameValidation = new RegExp('^[0-9a-zA-Z-_.]+$');
-	}
-
-	if (!nameValidation.test(username)) {
+	if (!validateUsername(username)) {
 		throw new Meteor.Error(
 			'username-invalid',
 			`${_.escape(username)} is not a valid username, use only letters, numbers, dots, hyphens and underscores`,
@@ -74,18 +68,15 @@ export const setUsernameWithValidation = async (userId: string, username: string
 
 export const _setUsername = async function (userId: string, u: string, fullUser: IUser): Promise<unknown> {
 	const username = u.trim();
+
 	if (!userId || !username) {
 		return false;
 	}
-	let nameValidation;
-	try {
-		nameValidation = new RegExp(`^${settings.get('UTF8_User_Names_Validation')}$`);
-	} catch (error) {
-		nameValidation = new RegExp('^[0-9a-zA-Z-_.]+$');
-	}
-	if (!nameValidation.test(username)) {
+
+	if (!validateUsername(username)) {
 		return false;
 	}
+
 	const user = fullUser || (await Users.findOneById(userId));
 	// User already has desired username, return
 	if (user.username === username) {

apps/meteor/app/lib/server/functions/validateUsername.ts

@@ -0,0 +1,15 @@
+import { settings } from '../../../settings/server';
+
+export const validateUsername = (username: string): boolean => {
+	const settingsRegExp = settings.get('UTF8_User_Names_Validation');
+	const defaultPattern = /^[0-9a-zA-Z-_.]+$/;
+
+	let usernameRegExp: RegExp;
+	try {
+		usernameRegExp = settingsRegExp ? new RegExp(`^${settingsRegExp}$`) : defaultPattern;
+	} catch (e) {
+		usernameRegExp = defaultPattern;
+	}
+
+	return usernameRegExp.test(username);
+};

apps/meteor/tests/end-to-end/api/users.ts

@@ -605,6 +605,25 @@ describe('[Users]', () => {
 				})
 				.end(done);
 		});
+
+		it('should return an error when trying register new user with an invalid username', (done) => {
+			void request
+				.post(api('users.register'))
+				.send({
+					email,
+					name: 'name',
+					username: 'test$username<>',
+					pass: 'test',
+				})
+				.expect('Content-Type', 'application/json')
+				.expect(400)
+				.expect((res) => {
+					expect(res.body).to.have.property('success', false);
+					expect(res.body).to.have.property('error').and.to.be.equal('The username provided is not valid');
+				})
+				.end(done);
+		});
+
 		it('should return an error when trying register new user with an existing username', (done) => {
 			void request
 				.post(api('users.register'))
@@ -3700,9 +3719,9 @@ describe('[Users]', () => {
 
 		it('should invalidate all active sesions', (done) => {
 			/* We want to validate that the login with the "old" credentials fails
-      		However, the removal of the tokens is done asynchronously.
-      		Thus, we check that within the next seconds, at least one try to
-      		access an authentication requiring route fails */
+				However, the removal of the tokens is done asynchronously.
+				Thus, we check that within the next seconds, at least one try to
+				access an authentication requiring route fails */
 			let counter = 0;
 
 			async function checkAuthenticationFails() {
@@ -4060,9 +4079,9 @@ describe('[Users]', () => {
 
 		it('should invalidate all active sesions', (done) => {
 			/* We want to validate that the login with the "old" credentials fails
-      		However, the removal of the tokens is done asynchronously.
-      		Thus, we check that within the next seconds, at least one try to
-      		access an authentication requiring route fails */
+				However, the removal of the tokens is done asynchronously.
+				Thus, we check that within the next seconds, at least one try to
+				access an authentication requiring route fails */
 			let counter = 0;
 
 			async function checkAuthenticationFails() {

apps/meteor/tests/unit/app/lib/server/functions/validateUsername.spec.ts

@@ -0,0 +1,94 @@
+import { expect } from 'chai';
+import proxyquire from 'proxyquire';
+import sinon from 'sinon';
+
+describe('validateUsername', () => {
+	let getStub: sinon.SinonStub;
+
+	const proxySettings = {
+		settings: {
+			get: () => null,
+		},
+	};
+
+	const { validateUsername } = proxyquire.noCallThru().load('../../../../../../app/lib/server/functions/validateUsername', {
+		'../../../settings/server': proxySettings,
+	});
+
+	beforeEach(() => {
+		getStub = sinon.stub(proxySettings.settings, 'get');
+	});
+
+	afterEach(() => {
+		sinon.restore();
+	});
+
+	describe('with default settings', () => {
+		beforeEach(() => {
+			getStub.withArgs('UTF8_User_Names_Validation').returns('[0-9a-zA-Z-_.]+');
+		});
+
+		it('should return true for a valid username', () => {
+			const result = validateUsername('valid_username.123');
+			expect(result).to.be.true;
+		});
+
+		it('should return false for an invalid username containing special HTML tags', () => {
+			const result = validateUsername('username<div>$</div>');
+			expect(result).to.be.false;
+		});
+
+		it('should return false for an empty username', () => {
+			const result = validateUsername('');
+			expect(result).to.be.false;
+		});
+
+		it('should return false for a username with invalid characters', () => {
+			const result = validateUsername('invalid*username!');
+			expect(result).to.be.false;
+		});
+
+		it('should return true for a username with allowed special characters', () => {
+			const result = validateUsername('username-_.');
+			expect(result).to.be.true;
+		});
+	});
+
+	describe('with custom regex settings', () => {
+		beforeEach(() => {
+			getStub.withArgs('UTF8_User_Names_Validation').returns('[a-zA-Z]+');
+		});
+
+		it('should return true for a username matching the custom regex', () => {
+			const result = validateUsername('ValidUsername');
+			expect(result).to.be.true;
+		});
+
+		it('should return false for a username that does not match the custom regex', () => {
+			const result = validateUsername('username123');
+			expect(result).to.be.false;
+		});
+	});
+
+	describe('with null regex settings', () => {
+		beforeEach(() => {
+			getStub.withArgs('UTF8_User_Names_Validation').returns(null);
+		});
+
+		it('should fallback to the default regex pattern if the settings value is null', () => {
+			const result = validateUsername('username');
+			expect(result).to.be.true;
+		});
+	});
+
+	describe('with invalid regex settings', () => {
+		beforeEach(() => {
+			getStub.withArgs('UTF8_User_Names_Validation').returns('invalid[');
+		});
+
+		it('should fallback to the default regex pattern if the settings value is invalid', () => {
+			const result = validateUsername('username');
+			expect(result).to.be.true;
+		});
+	});
+});

packages/i18n/src/locales/en.i18n.json

@@ -6120,6 +6120,7 @@
   "registration.component.form.username": "Username",
   "registration.component.form.name": "Name",
   "registration.component.form.nameContainsInvalidChars": "Name contains invalid characters",
+  "registration.component.form.usernameContainsInvalidChars": "Username contains invalid characters",
   "registration.component.form.nameOptional": "Name optional",
   "registration.component.form.createAnAccount": "Create an account",
   "registration.component.form.userAlreadyExist": "Username already exists. Please try another username.",

packages/i18n/src/locales/pt-BR.i18n.json

@@ -4914,6 +4914,7 @@
   "registration.component.form.username": "Nome de usuário",
   "registration.component.form.name": "Nome",
   "registration.component.form.nameContainsInvalidChars": "O nome contém caracteres inválidos",
+  "registration.component.form.usernameContainsInvalidChars": "O nome de usuário contém caracteres inválidos",
   "registration.component.form.userAlreadyExist": "O nome de usuário já existe. Tente outro nome de usuário.",
   "registration.component.form.emailAlreadyExists": "E-mail já existe",
   "registration.component.form.usernameAlreadyExists": "O nome de usuário já existe. Tente outro nome de usuário.",
@@ -5014,4 +5015,4 @@
   "Enterprise": "Enterprise",
   "UpgradeToGetMore_engagement-dashboard_Title": "Analytics",
   "UpgradeToGetMore_auditing_Title": "Auditoria de mensagem"
-}
+}

packages/web-ui-registration/src/RegisterForm.tsx

@@ -100,6 +100,12 @@ export const RegisterForm = ({ setLoginRoute }: { setLoginRoute: DispatchLoginRo
 					if (/Username is already in use/.test(error.error)) {
 						setError('username', { type: 'username-already-exists', message: t('registration.component.form.userAlreadyExist') });
 					}
+					if (/The username provided is not valid/.test(error.error)) {
+						setError('username', {
+							type: 'username-contains-invalid-chars',
+							message: t('registration.component.form.usernameContainsInvalidChars'),
+						});
+					}
 					if (/Name contains invalid characters/.test(error.error)) {
 						setError('name', { type: 'name-contains-invalid-chars', message: t('registration.component.form.nameContainsInvalidChars') });
 					}