diff --git a/.changeset/purple-dolls-serve.md b/.changeset/purple-dolls-serve.md new file mode 100644 index 0000000000000..fc44faa60a385 --- /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 7ae585b89dfa7..9c56ecac01cbd 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 e19ef874db0fd..5b2b1923da75d 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 0000000000000..523667282d22b --- /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 d6112dd2416bb..e908baebd974b 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 0000000000000..647873b8ffbd1 --- /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 a807629819a6a..69cc6c43fa7fe 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 67c8f46888adf..c1ebbc28ca3b3 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 57cf9378ab72a..311593d8e9b71 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') }); }