diff --git a/.changeset/fast-ligers-unite.md b/.changeset/fast-ligers-unite.md new file mode 100644 index 0000000000000..eacb88108a0f7 --- /dev/null +++ b/.changeset/fast-ligers-unite.md @@ -0,0 +1,5 @@ +--- +'@rocket.chat/meteor': patch +--- + +Security Hotfix (https://docs.rocket.chat/docs/security-fixes-and-updates) diff --git a/apps/meteor/app/api/server/lib/isValidQuery.ts b/apps/meteor/app/api/server/lib/isValidQuery.ts index b4dd70c2ae0d6..97620cdb1b9ef 100644 --- a/apps/meteor/app/api/server/lib/isValidQuery.ts +++ b/apps/meteor/app/api/server/lib/isValidQuery.ts @@ -23,16 +23,22 @@ export const isValidQuery: { const verifyQuery = (query: Query, allowedAttributes: string[], allowedOperations: string[], parent = ''): boolean => { return Object.entries(removeDangerousProps(query)).every(([key, value]) => { const path = parent ? `${parent}.${key}` : key; - if (parent === '' && path.startsWith('$')) { - if (!allowedOperations.includes(path)) { - isValidQuery.errors.push(`Invalid operation: ${path}`); + if (key.startsWith('$')) { + if (!allowedOperations.includes(key)) { + isValidQuery.errors.push(`Invalid operation: ${key}`); return false; } - if (!Array.isArray(value)) { - isValidQuery.errors.push(`Invalid parameter for operation: ${path} : ${value}`); - return false; + + if (Array.isArray(value)) { + return value.every((v) => verifyQuery(v, allowedAttributes, allowedOperations)); } - return value.every((v) => verifyQuery(v, allowedAttributes, allowedOperations)); + + if (value instanceof Object) { + return verifyQuery(value, allowedAttributes, allowedOperations, path); + } + + // handles primitive values (strings, numbers, booleans, etc.) + return true; } if ( diff --git a/apps/meteor/app/api/server/v1/users.ts b/apps/meteor/app/api/server/v1/users.ts index d4f0012cd5784..f6058085deeec 100644 --- a/apps/meteor/app/api/server/v1/users.ts +++ b/apps/meteor/app/api/server/v1/users.ts @@ -50,7 +50,7 @@ import { } from '../../../lib/server/functions/checkUsernameAvailability'; import { deleteUser } from '../../../lib/server/functions/deleteUser'; import { getAvatarSuggestionForUser } from '../../../lib/server/functions/getAvatarSuggestionForUser'; -import { getFullUserDataByIdOrUsernameOrImportId } from '../../../lib/server/functions/getFullUserData'; +import { getFullUserDataByIdOrUsernameOrImportId, defaultFields, fullFields } from '../../../lib/server/functions/getFullUserData'; import { generateUsernameSuggestion } from '../../../lib/server/functions/getUsernameSuggestion'; import { saveCustomFields } from '../../../lib/server/functions/saveCustomFields'; import { saveCustomFieldsWithoutValidation } from '../../../lib/server/functions/saveCustomFieldsWithoutValidation'; @@ -503,13 +503,18 @@ API.v1.addRoute( const { offset, count } = await getPaginationItems(this.queryParams); const { sort, fields, query } = await this.parseJsonQuery(); - const nonEmptyQuery = getNonEmptyQuery(query, await hasPermissionAsync(this.userId, 'view-full-other-user-info')); const nonEmptyFields = getNonEmptyFields(fields); const inclusiveFields = getInclusiveFields(nonEmptyFields); const inclusiveFieldsKeys = Object.keys(inclusiveFields); + const hasUserQuery = query && Object.keys(query).length > 0; + + const nonEmptyQuery = getNonEmptyQuery(query, await hasPermissionAsync(this.userId, 'view-full-other-user-info')); + + // if user provided a query, validate it with their allowed operators + // otherwise we use the default query (with $regex and $options) if ( !isValidQuery( nonEmptyQuery, @@ -521,7 +526,7 @@ API.v1.addRoute( inclusiveFieldsKeys.includes('type') && 'type.*', inclusiveFieldsKeys.includes('customFields') && 'customFields.*', ].filter(Boolean) as string[], - this.queryOperations, + hasUserQuery ? this.queryOperations : [...this.queryOperations, '$regex', '$options'], ) ) { throw new Meteor.Error('error-invalid-query', isValidQuery.errors.join('\n')); @@ -1144,8 +1149,13 @@ API.v1.addRoute( const selector: { exceptions: Required['username'][]; conditions: Filter; term: string } = JSON.parse(selectorRaw); try { - if (selector?.conditions && !isValidQuery(selector.conditions, ['*'], ['$or', '$and'])) { - throw new Error('error-invalid-query'); + if (selector?.conditions) { + const canViewFullInfo = await hasPermissionAsync(this.userId, 'view-full-other-user-info'); + const allowedFields = canViewFullInfo ? [...Object.keys(defaultFields), ...Object.keys(fullFields)] : Object.keys(defaultFields); + + if (!isValidQuery(selector.conditions, allowedFields, ['$and', '$ne', '$exists'])) { + throw new Error('error-invalid-query'); + } } } catch (e) { return API.v1.failure(e); diff --git a/apps/meteor/app/lib/server/functions/getFullUserData.ts b/apps/meteor/app/lib/server/functions/getFullUserData.ts index f66f8ecb49c66..c55a5464a2a8b 100644 --- a/apps/meteor/app/lib/server/functions/getFullUserData.ts +++ b/apps/meteor/app/lib/server/functions/getFullUserData.ts @@ -7,7 +7,7 @@ import { settings } from '../../../settings/server'; const logger = new Logger('getFullUserData'); -const defaultFields = { +export const defaultFields = { name: 1, username: 1, nickname: 1, @@ -24,7 +24,7 @@ const defaultFields = { statusLivechat: 1, } as const; -const fullFields = { +export const fullFields = { emails: 1, phone: 1, statusConnection: 1, diff --git a/apps/meteor/tests/unit/app/api/server/v1/lib/isValidQuery.spec.ts b/apps/meteor/tests/unit/app/api/server/v1/lib/isValidQuery.spec.ts index e67ad244066f2..5f421578ed480 100644 --- a/apps/meteor/tests/unit/app/api/server/v1/lib/isValidQuery.spec.ts +++ b/apps/meteor/tests/unit/app/api/server/v1/lib/isValidQuery.spec.ts @@ -174,7 +174,7 @@ describe('isValidQuery', () => { }, }, }; - expect(isValidQuery(query, props, ['$or'])).to.be.true; + expect(isValidQuery(query, props, ['$or', '$regex'])).to.be.true; expect(isValidQuery.errors.length).to.be.equals(0); }); @@ -212,5 +212,15 @@ describe('isValidQuery', () => { ), ).to.be.false; }); + + it('should return false if the query contains nested conditions with disallowed operators', () => { + const props = ['username']; + const allowedOps = ['$and', '$ne']; + const query = { + $and: [{ username: { $exists: true } }, { username: { $ne: '1000' } }], + }; + expect(isValidQuery(query, props, allowedOps)).to.be.false; + expect(isValidQuery.errors.length).to.be.equals(1); + }); }); });