From dd37ea1b35eb142c43927f08b1f19576d42c05a3 Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Wed, 21 Aug 2024 18:01:11 -0300 Subject: [PATCH] fix: validate username before registering user (#32743) --- .changeset/purple-dolls-serve.md | 7 ++ apps/meteor/app/api/server/v1/users.ts | 5 + .../app/lib/server/functions/setUsername.ts | 21 ++--- .../lib/server/functions/validateUsername.ts | 15 +++ apps/meteor/tests/end-to-end/api/users.ts | 31 ++++-- .../server/functions/validateUsername.spec.ts | 94 +++++++++++++++++++ packages/i18n/src/locales/en.i18n.json | 1 + packages/i18n/src/locales/pt-BR.i18n.json | 3 +- .../web-ui-registration/src/RegisterForm.tsx | 6 ++ 9 files changed, 161 insertions(+), 22 deletions(-) create mode 100644 .changeset/purple-dolls-serve.md create mode 100644 apps/meteor/app/lib/server/functions/validateUsername.ts create mode 100644 apps/meteor/tests/unit/app/lib/server/functions/validateUsername.spec.ts diff --git a/.changeset/purple-dolls-serve.md b/.changeset/purple-dolls-serve.md new file mode 100644 index 000000000000..fc44faa60a38 --- /dev/null +++ b/.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. diff --git a/apps/meteor/app/api/server/v1/users.ts b/apps/meteor/app/api/server/v1/users.ts index 7ae585b89dfa..9c56ecac01cb 100644 --- a/apps/meteor/app/api/server/v1/users.ts +++ b/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'); } diff --git a/apps/meteor/app/lib/server/functions/setUsername.ts b/apps/meteor/app/lib/server/functions/setUsername.ts index e19ef874db0f..5b2b1923da75 100644 --- a/apps/meteor/app/lib/server/functions/setUsername.ts +++ b/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 => { 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 { 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) { diff --git a/apps/meteor/app/lib/server/functions/validateUsername.ts b/apps/meteor/app/lib/server/functions/validateUsername.ts new file mode 100644 index 000000000000..523667282d22 --- /dev/null +++ b/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); +}; diff --git a/apps/meteor/tests/end-to-end/api/users.ts b/apps/meteor/tests/end-to-end/api/users.ts index d6112dd2416b..e908baebd974 100644 --- a/apps/meteor/tests/end-to-end/api/users.ts +++ b/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() { diff --git a/apps/meteor/tests/unit/app/lib/server/functions/validateUsername.spec.ts b/apps/meteor/tests/unit/app/lib/server/functions/validateUsername.spec.ts new file mode 100644 index 000000000000..647873b8ffbd --- /dev/null +++ b/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', () => { + const getStub = sinon.stub(); + + const proxySettings = { + settings: { + get: getStub, + }, + }; + + const { validateUsername } = proxyquire.noCallThru().load('../../../../../../app/lib/server/functions/validateUsername', { + '../../../settings/server': proxySettings, + }); + + beforeEach(() => { + getStub.reset(); + }); + + 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
$
'); + 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; + }); + }); +}); diff --git a/packages/i18n/src/locales/en.i18n.json b/packages/i18n/src/locales/en.i18n.json index a807629819a6..69cc6c43fa7f 100644 --- a/packages/i18n/src/locales/en.i18n.json +++ b/packages/i18n/src/locales/en.i18n.json @@ -6122,6 +6122,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.", diff --git a/packages/i18n/src/locales/pt-BR.i18n.json b/packages/i18n/src/locales/pt-BR.i18n.json index 67c8f46888ad..c1ebbc28ca3b 100644 --- a/packages/i18n/src/locales/pt-BR.i18n.json +++ b/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" -} \ No newline at end of file +} diff --git a/packages/web-ui-registration/src/RegisterForm.tsx b/packages/web-ui-registration/src/RegisterForm.tsx index 57cf9378ab72..311593d8e9b7 100644 --- a/packages/web-ui-registration/src/RegisterForm.tsx +++ b/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') }); }