From 432c249f6e899d4df13507684bc1a14a63c8d6da Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Mon, 8 Jul 2024 10:28:49 -0300 Subject: [PATCH 1/7] fix: create validateUsername method --- apps/meteor/app/api/server/v1/users.ts | 5 +++++ .../app/lib/server/functions/setUsername.ts | 21 ++++++------------- .../lib/server/functions/validateUsername.ts | 9 ++++++++ 3 files changed, 20 insertions(+), 15 deletions(-) create mode 100644 apps/meteor/app/lib/server/functions/validateUsername.ts diff --git a/apps/meteor/app/api/server/v1/users.ts b/apps/meteor/app/api/server/v1/users.ts index c26957fa1991..e02916a2399d 100644 --- a/apps/meteor/app/api/server/v1/users.ts +++ b/apps/meteor/app/api/server/v1/users.ts @@ -43,6 +43,7 @@ import { setStatusText } from '../../../lib/server/functions/setStatusText'; import { setUserAvatar } from '../../../lib/server/functions/setUserAvatar'; import { setUsernameWithValidation } from '../../../lib/server/functions/setUsername'; import { validateCustomFields } from '../../../lib/server/functions/validateCustomFields'; +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'; @@ -635,6 +636,10 @@ API.v1.addRoute( return API.v1.failure('Username is already in use'); } + if (!validateUsername(this.bodyParams.username)) { + return API.v1.failure('Invalid username'); + } + const { secret: secretURL, ...params } = this.bodyParams; if (this.bodyParams.customFields) { 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..5fd0ac722ab6 --- /dev/null +++ b/apps/meteor/app/lib/server/functions/validateUsername.ts @@ -0,0 +1,9 @@ +import { settings } from '../../../settings/server'; + +export const validateUsername = (username: string): boolean => { + const settingsRegExp = settings.get('UTF8_User_Names_Validation'); + + const usernameRegExp = settingsRegExp ? new RegExp(`^${settingsRegExp}$`) : new RegExp('^[0-9a-zA-Z-_.]+$'); + + return usernameRegExp.test(username); +}; From 335e9b2df3a7a1b44f6ac4332aa95a38f06183e8 Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Mon, 8 Jul 2024 10:55:26 -0300 Subject: [PATCH 2/7] test: add e2e case to invalid username --- apps/meteor/app/api/server/v1/users.ts | 2 +- apps/meteor/tests/end-to-end/api/01-users.ts | 33 ++++++++++++++++---- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/apps/meteor/app/api/server/v1/users.ts b/apps/meteor/app/api/server/v1/users.ts index e02916a2399d..771414d165cf 100644 --- a/apps/meteor/app/api/server/v1/users.ts +++ b/apps/meteor/app/api/server/v1/users.ts @@ -637,7 +637,7 @@ API.v1.addRoute( } if (!validateUsername(this.bodyParams.username)) { - return API.v1.failure('Invalid username'); + return API.v1.failure(`The username provided is not valid. Use only letters, numbers, dots, hyphens and underscores`); } const { secret: secretURL, ...params } = this.bodyParams; diff --git a/apps/meteor/tests/end-to-end/api/01-users.ts b/apps/meteor/tests/end-to-end/api/01-users.ts index f34a424597af..eff229a5e3e5 100644 --- a/apps/meteor/tests/end-to-end/api/01-users.ts +++ b/apps/meteor/tests/end-to-end/api/01-users.ts @@ -605,6 +605,27 @@ 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. Use only letters, numbers, dots, hyphens and underscores'); + }) + .end(done); + }); + it('should return an error when trying register new user with an existing username', (done) => { void request .post(api('users.register')) @@ -3568,9 +3589,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() { @@ -3928,9 +3949,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() { From 61bab51406ed754263f004e0736d939daf8de2fc Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Thu, 15 Aug 2024 20:29:47 -0300 Subject: [PATCH 3/7] chore: enhance regex error handling and expand test coverage --- .../lib/server/functions/validateUsername.ts | 8 +- .../server/functions/validateUsername.spec.ts | 95 +++++++++++++++++++ 2 files changed, 102 insertions(+), 1 deletion(-) create mode 100644 apps/meteor/tests/unit/app/lib/server/functions/validateUsername.spec.ts diff --git a/apps/meteor/app/lib/server/functions/validateUsername.ts b/apps/meteor/app/lib/server/functions/validateUsername.ts index 5fd0ac722ab6..523667282d22 100644 --- a/apps/meteor/app/lib/server/functions/validateUsername.ts +++ b/apps/meteor/app/lib/server/functions/validateUsername.ts @@ -2,8 +2,14 @@ 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-_.]+$/; - const usernameRegExp = settingsRegExp ? new RegExp(`^${settingsRegExp}$`) : new RegExp('^[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/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..810a324cc9ac --- /dev/null +++ b/apps/meteor/tests/unit/app/lib/server/functions/validateUsername.spec.ts @@ -0,0 +1,95 @@ +import { expect } from 'chai'; +import proxyquire from 'proxyquire'; + +const proxySettings = { + settings: { + get: (key: string): string | null => { + if (key === 'UTF8_User_Names_Validation') { + // Default value set at apps/meteor/server/settings/general.ts + return '[0-9a-zA-Z-_.]+'; + } + return null; + }, + }, +}; + +const { validateUsername } = proxyquire.noCallThru().load('../../../../../../app/lib/server/functions/validateUsername', { + '../../../settings/server': proxySettings, +}); + +describe('validateUsername', () => { + describe('with default settings', () => { + 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(() => { + proxySettings.settings.get = (key: string) => { + if (key === 'UTF8_User_Names_Validation') { + return '[a-zA-Z]+'; + } + return null; + }; + }); + + 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(() => { + proxySettings.settings.get = () => 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(() => { + proxySettings.settings.get = (key: string) => { + if (key === 'UTF8_User_Names_Validation') { + return 'invalid['; + } + return null; + }; + }); + + it('should fallback to the default regex pattern if the settings value is invalid', () => { + const result = validateUsername('username'); + expect(result).to.be.true; + }); + }); +}); From c8baff6dd1cbc06aadcf58024ca16bdc5b11a93f Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Mon, 19 Aug 2024 00:02:31 -0300 Subject: [PATCH 4/7] fix: add username validation i18n keys --- .changeset/purple-dolls-serve.md | 7 +++++++ apps/meteor/app/api/server/v1/users.ts | 2 +- apps/meteor/tests/end-to-end/api/users.ts | 4 +--- packages/i18n/src/locales/en.i18n.json | 1 + packages/i18n/src/locales/pt-BR.i18n.json | 3 ++- packages/web-ui-registration/src/RegisterForm.tsx | 3 +++ 6 files changed, 15 insertions(+), 5 deletions(-) create mode 100644 .changeset/purple-dolls-serve.md 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 4dc03d3d7a28..9c56ecac01cb 100644 --- a/apps/meteor/app/api/server/v1/users.ts +++ b/apps/meteor/app/api/server/v1/users.ts @@ -653,7 +653,7 @@ API.v1.addRoute( } if (!validateUsername(this.bodyParams.username)) { - return API.v1.failure(`The username provided is not valid. Use only letters, numbers, dots, hyphens and underscores`); + return API.v1.failure(`The username provided is not valid`); } if (!(await checkUsernameAvailability(this.bodyParams.username))) { diff --git a/apps/meteor/tests/end-to-end/api/users.ts b/apps/meteor/tests/end-to-end/api/users.ts index 8fcc3b3996d7..e908baebd974 100644 --- a/apps/meteor/tests/end-to-end/api/users.ts +++ b/apps/meteor/tests/end-to-end/api/users.ts @@ -619,9 +619,7 @@ describe('[Users]', () => { .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. Use only letters, numbers, dots, hyphens and underscores'); + expect(res.body).to.have.property('error').and.to.be.equal('The username provided is not valid'); }) .end(done); }); diff --git a/packages/i18n/src/locales/en.i18n.json b/packages/i18n/src/locales/en.i18n.json index c270bb9bffb1..9373425a9fcb 100644 --- a/packages/i18n/src/locales/en.i18n.json +++ b/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.", 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..b6ecc0f02bf1 100644 --- a/packages/web-ui-registration/src/RegisterForm.tsx +++ b/packages/web-ui-registration/src/RegisterForm.tsx @@ -100,6 +100,9 @@ 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') }); } From 4186245c3db2386ce3a099c1b9aa58ef11d70754 Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Mon, 19 Aug 2024 00:03:46 -0300 Subject: [PATCH 5/7] test: add stub on validateUsername mocks --- .../server/functions/validateUsername.spec.ts | 53 +++++++++---------- 1 file changed, 26 insertions(+), 27 deletions(-) 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 index 810a324cc9ac..72239a5490d6 100644 --- a/apps/meteor/tests/unit/app/lib/server/functions/validateUsername.spec.ts +++ b/apps/meteor/tests/unit/app/lib/server/functions/validateUsername.spec.ts @@ -1,24 +1,33 @@ import { expect } from 'chai'; import proxyquire from 'proxyquire'; +import sinon from 'sinon'; -const proxySettings = { - settings: { - get: (key: string): string | null => { - if (key === 'UTF8_User_Names_Validation') { - // Default value set at apps/meteor/server/settings/general.ts - return '[0-9a-zA-Z-_.]+'; - } - return null; +describe('validateUsername', () => { + let getStub: sinon.SinonStub; + + const proxySettings = { + settings: { + get: () => null, }, - }, -}; + }; -const { validateUsername } = proxyquire.noCallThru().load('../../../../../../app/lib/server/functions/validateUsername', { - '../../../settings/server': proxySettings, -}); + const { validateUsername } = proxyquire.noCallThru().load('../../../../../../app/lib/server/functions/validateUsername', { + '../../../settings/server': proxySettings, + }); + + beforeEach(() => { + getStub = sinon.stub(proxySettings.settings, 'get'); + }); + + afterEach(() => { + sinon.restore(); + }); -describe('validateUsername', () => { 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; @@ -47,12 +56,7 @@ describe('validateUsername', () => { describe('with custom regex settings', () => { beforeEach(() => { - proxySettings.settings.get = (key: string) => { - if (key === 'UTF8_User_Names_Validation') { - return '[a-zA-Z]+'; - } - return null; - }; + getStub.withArgs('UTF8_User_Names_Validation').returns('[a-zA-Z]+'); }); it('should return true for a username matching the custom regex', () => { @@ -68,7 +72,7 @@ describe('validateUsername', () => { describe('with null regex settings', () => { beforeEach(() => { - proxySettings.settings.get = () => null; + getStub.withArgs('UTF8_User_Names_Validation').returns(null); }); it('should fallback to the default regex pattern if the settings value is null', () => { @@ -79,12 +83,7 @@ describe('validateUsername', () => { describe('with invalid regex settings', () => { beforeEach(() => { - proxySettings.settings.get = (key: string) => { - if (key === 'UTF8_User_Names_Validation') { - return 'invalid['; - } - return null; - }; + getStub.withArgs('UTF8_User_Names_Validation').returns('invalid['); }); it('should fallback to the default regex pattern if the settings value is invalid', () => { From b7dc58e97b88cdf1eac173632a3fcf8ee0e9a028 Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Mon, 19 Aug 2024 08:36:10 -0300 Subject: [PATCH 6/7] fix: linter --- packages/web-ui-registration/src/RegisterForm.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/web-ui-registration/src/RegisterForm.tsx b/packages/web-ui-registration/src/RegisterForm.tsx index b6ecc0f02bf1..311593d8e9b7 100644 --- a/packages/web-ui-registration/src/RegisterForm.tsx +++ b/packages/web-ui-registration/src/RegisterForm.tsx @@ -101,7 +101,10 @@ export const RegisterForm = ({ setLoginRoute }: { setLoginRoute: DispatchLoginRo 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') }); + 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') }); From a9d8049dad458501a9e90ed84b7d4e48772de89c Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Tue, 20 Aug 2024 16:35:13 -0300 Subject: [PATCH 7/7] test: change stub to be reusable --- .../unit/app/lib/server/functions/validateUsername.spec.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 index 72239a5490d6..647873b8ffbd 100644 --- a/apps/meteor/tests/unit/app/lib/server/functions/validateUsername.spec.ts +++ b/apps/meteor/tests/unit/app/lib/server/functions/validateUsername.spec.ts @@ -3,11 +3,11 @@ import proxyquire from 'proxyquire'; import sinon from 'sinon'; describe('validateUsername', () => { - let getStub: sinon.SinonStub; + const getStub = sinon.stub(); const proxySettings = { settings: { - get: () => null, + get: getStub, }, }; @@ -16,7 +16,7 @@ describe('validateUsername', () => { }); beforeEach(() => { - getStub = sinon.stub(proxySettings.settings, 'get'); + getStub.reset(); }); afterEach(() => {