From 31ae30f30ad71d9e5a1b0cad494b3471a7dd8807 Mon Sep 17 00:00:00 2001 From: Guilherme Gazzo <guilhermegazzo@gmail.com> Date: Thu, 26 May 2022 19:11:41 -0300 Subject: [PATCH] Chore: Rest API query parameters handling (#25648) --- apps/meteor/.mocharc.api.js | 2 +- apps/meteor/app/api/server/api.d.ts | 2 +- apps/meteor/app/api/server/api.js | 3 + .../app/api/server/helpers/parseJsonQuery.js | 119 ---------- .../app/api/server/helpers/parseJsonQuery.ts | 142 ++++++++++++ apps/meteor/app/api/server/lib/cleanQuery.ts | 4 +- .../meteor/app/api/server/lib/isValidQuery.ts | 58 +++++ apps/meteor/app/api/server/v1/users.js | 23 +- .../api/server/v1/lib/isValidQuery.spec.ts | 216 ++++++++++++++++++ 9 files changed, 445 insertions(+), 124 deletions(-) delete mode 100644 apps/meteor/app/api/server/helpers/parseJsonQuery.js create mode 100644 apps/meteor/app/api/server/helpers/parseJsonQuery.ts create mode 100644 apps/meteor/app/api/server/lib/isValidQuery.ts create mode 100644 apps/meteor/tests/unit/app/api/server/v1/lib/isValidQuery.spec.ts diff --git a/apps/meteor/.mocharc.api.js b/apps/meteor/.mocharc.api.js index 982d47181fe..934fd1509ae 100644 --- a/apps/meteor/.mocharc.api.js +++ b/apps/meteor/.mocharc.api.js @@ -9,5 +9,5 @@ module.exports = { timeout: 10000, bail: true, file: 'tests/end-to-end/teardown.js', - spec: ['tests/end-to-end/api/*.js', 'tests/end-to-end/api/*.ts', 'tests/end-to-end/apps/*.js'], + spec: ['tests/unit/app/api/server/v1/*.spec.ts', 'tests/end-to-end/api/*.js', 'tests/end-to-end/api/*.ts', 'tests/end-to-end/apps/*.js'], }; diff --git a/apps/meteor/app/api/server/api.d.ts b/apps/meteor/app/api/server/api.d.ts index 70d28d6169c..fc9519b87ea 100644 --- a/apps/meteor/app/api/server/api.d.ts +++ b/apps/meteor/app/api/server/api.d.ts @@ -143,7 +143,7 @@ type Operations<TPathPattern extends PathPattern, TOptions extends Options = {}> }; declare class APIClass<TBasePath extends string = '/'> { - fieldSeparator(fieldSeparator: unknown): void; + fieldSeparator: string; limitedUserFieldsToExclude(fields: { [x: string]: unknown }, limitedUserFieldsToExclude: unknown): { [x: string]: unknown }; diff --git a/apps/meteor/app/api/server/api.js b/apps/meteor/app/api/server/api.js index e92d061ae33..c88592af1e1 100644 --- a/apps/meteor/app/api/server/api.js +++ b/apps/meteor/app/api/server/api.js @@ -425,6 +425,9 @@ export class APIClass extends Restivus { connection, }); + this.queryOperations = options.queryOperations; + this.queryFields = options.queryFields; + result = DDP._CurrentInvocation.withValue(invocation, () => Promise.await(originalAction.apply(this))) || API.v1.success(); log.http({ diff --git a/apps/meteor/app/api/server/helpers/parseJsonQuery.js b/apps/meteor/app/api/server/helpers/parseJsonQuery.js deleted file mode 100644 index 18b9e372dcb..00000000000 --- a/apps/meteor/app/api/server/helpers/parseJsonQuery.js +++ /dev/null @@ -1,119 +0,0 @@ -import { Meteor } from 'meteor/meteor'; -import { EJSON } from 'meteor/ejson'; - -import { hasPermission } from '../../../authorization'; -import { clean } from '../lib/cleanQuery'; -import { API } from '../api'; - -const pathAllowConf = { - '/api/v1/users.list': ['$or', '$regex', '$and'], - 'def': ['$or', '$and', '$regex'], -}; - -API.helperMethods.set('parseJsonQuery', function _parseJsonQuery() { - let sort; - if (this.queryParams.sort) { - try { - sort = JSON.parse(this.queryParams.sort); - Object.entries(sort).forEach(([key, value]) => { - if (value !== 1 && value !== -1) { - throw new Meteor.Error('error-invalid-sort-parameter', `Invalid sort parameter: ${key}`, { - helperMethod: 'parseJsonQuery', - }); - } - }); - } catch (e) { - this.logger.warn(`Invalid sort parameter provided "${this.queryParams.sort}":`, e); - throw new Meteor.Error('error-invalid-sort', `Invalid sort parameter provided: "${this.queryParams.sort}"`, { - helperMethod: 'parseJsonQuery', - }); - } - } - - let fields; - if (this.queryParams.fields) { - try { - fields = JSON.parse(this.queryParams.fields); - - Object.entries(fields).forEach(([key, value]) => { - if (value !== 1 && value !== 0) { - throw new Meteor.Error('error-invalid-sort-parameter', `Invalid fields parameter: ${key}`, { - helperMethod: 'parseJsonQuery', - }); - } - }); - } catch (e) { - this.logger.warn(`Invalid fields parameter provided "${this.queryParams.fields}":`, e); - throw new Meteor.Error('error-invalid-fields', `Invalid fields parameter provided: "${this.queryParams.fields}"`, { - helperMethod: 'parseJsonQuery', - }); - } - } - - // Verify the user's selected fields only contains ones which their role allows - if (typeof fields === 'object') { - let nonSelectableFields = Object.keys(API.v1.defaultFieldsToExclude); - if (this.request.route.includes('/v1/users.')) { - const getFields = () => - Object.keys( - hasPermission(this.userId, 'view-full-other-user-info') - ? API.v1.limitedUserFieldsToExcludeIfIsPrivilegedUser - : API.v1.limitedUserFieldsToExclude, - ); - nonSelectableFields = nonSelectableFields.concat(getFields()); - } - - Object.keys(fields).forEach((k) => { - if (nonSelectableFields.includes(k) || nonSelectableFields.includes(k.split(API.v1.fieldSeparator)[0])) { - delete fields[k]; - } - }); - } - - // Limit the fields by default - fields = Object.assign({}, fields, API.v1.defaultFieldsToExclude); - if (this.request.route.includes('/v1/users.')) { - if (hasPermission(this.userId, 'view-full-other-user-info')) { - fields = Object.assign(fields, API.v1.limitedUserFieldsToExcludeIfIsPrivilegedUser); - } else { - fields = Object.assign(fields, API.v1.limitedUserFieldsToExclude); - } - } - - let query = {}; - if (this.queryParams.query) { - try { - query = EJSON.parse(this.queryParams.query); - query = clean(query, pathAllowConf[this.request.route] || pathAllowConf.def); - } catch (e) { - this.logger.warn(`Invalid query parameter provided "${this.queryParams.query}":`, e); - throw new Meteor.Error('error-invalid-query', `Invalid query parameter provided: "${this.queryParams.query}"`, { - helperMethod: 'parseJsonQuery', - }); - } - } - - // Verify the user has permission to query the fields they are - if (typeof query === 'object') { - let nonQueryableFields = Object.keys(API.v1.defaultFieldsToExclude); - if (this.request.route.includes('/v1/users.')) { - if (hasPermission(this.userId, 'view-full-other-user-info')) { - nonQueryableFields = nonQueryableFields.concat(Object.keys(API.v1.limitedUserFieldsToExcludeIfIsPrivilegedUser)); - } else { - nonQueryableFields = nonQueryableFields.concat(Object.keys(API.v1.limitedUserFieldsToExclude)); - } - } - - Object.keys(query).forEach((k) => { - if (nonQueryableFields.includes(k) || nonQueryableFields.includes(k.split(API.v1.fieldSeparator)[0])) { - delete query[k]; - } - }); - } - - return { - sort, - fields, - query, - }; -}); diff --git a/apps/meteor/app/api/server/helpers/parseJsonQuery.ts b/apps/meteor/app/api/server/helpers/parseJsonQuery.ts new file mode 100644 index 00000000000..7e3592e765a --- /dev/null +++ b/apps/meteor/app/api/server/helpers/parseJsonQuery.ts @@ -0,0 +1,142 @@ +import { Meteor } from 'meteor/meteor'; +import { EJSON } from 'meteor/ejson'; + +import { hasPermission } from '../../../authorization/server'; +import { isValidQuery } from '../lib/isValidQuery'; +import { clean } from '../lib/cleanQuery'; +import { API } from '../api'; + +const pathAllowConf = { + '/api/v1/users.list': ['$or', '$regex', '$and'], + 'def': ['$or', '$and', '$regex'], +}; + +API.helperMethods.set( + 'parseJsonQuery', + function _parseJsonQuery(this: { + request: { + route: string; + }; + queryParams: { + query?: string; + sort?: string; + fields?: string; + }; + logger: any; + userId: string; + queryFields: string[]; + queryOperations: string[]; + }) { + let sort; + if (this.queryParams.sort) { + try { + sort = JSON.parse(this.queryParams.sort); + Object.entries(sort).forEach(([key, value]) => { + if (value !== 1 && value !== -1) { + throw new Meteor.Error('error-invalid-sort-parameter', `Invalid sort parameter: ${key}`, { + helperMethod: 'parseJsonQuery', + }); + } + }); + } catch (e) { + this.logger.warn(`Invalid sort parameter provided "${this.queryParams.sort}":`, e); + throw new Meteor.Error('error-invalid-sort', `Invalid sort parameter provided: "${this.queryParams.sort}"`, { + helperMethod: 'parseJsonQuery', + }); + } + } + + let fields: Record<string, 0 | 1> | undefined; + if (this.queryParams.fields) { + try { + fields = JSON.parse(this.queryParams.fields) as Record<string, 0 | 1>; + + Object.entries(fields).forEach(([key, value]) => { + if (value !== 1 && value !== 0) { + throw new Meteor.Error('error-invalid-sort-parameter', `Invalid fields parameter: ${key}`, { + helperMethod: 'parseJsonQuery', + }); + } + }); + } catch (e) { + this.logger.warn(`Invalid fields parameter provided "${this.queryParams.fields}":`, e); + throw new Meteor.Error('error-invalid-fields', `Invalid fields parameter provided: "${this.queryParams.fields}"`, { + helperMethod: 'parseJsonQuery', + }); + } + } + + // Verify the user's selected fields only contains ones which their role allows + if (typeof fields === 'object') { + let nonSelectableFields = Object.keys(API.v1.defaultFieldsToExclude); + if (this.request.route.includes('/v1/users.')) { + const getFields = () => + Object.keys( + hasPermission(this.userId, 'view-full-other-user-info') + ? API.v1.limitedUserFieldsToExcludeIfIsPrivilegedUser + : API.v1.limitedUserFieldsToExclude, + ); + nonSelectableFields = nonSelectableFields.concat(getFields()); + } + + Object.keys(fields).forEach((k) => { + if (nonSelectableFields.includes(k) || nonSelectableFields.includes(k.split(API.v1.fieldSeparator)[0])) { + fields && delete fields[k as keyof typeof fields]; + } + }); + } + + // Limit the fields by default + fields = Object.assign({}, fields, API.v1.defaultFieldsToExclude); + if (this.request.route.includes('/v1/users.')) { + if (hasPermission(this.userId, 'view-full-other-user-info')) { + fields = Object.assign(fields, API.v1.limitedUserFieldsToExcludeIfIsPrivilegedUser); + } else { + fields = Object.assign(fields, API.v1.limitedUserFieldsToExclude); + } + } + + let query: Record<string, any> = {}; + if (this.queryParams.query) { + this.logger.warn('attribute query is deprecated'); + try { + query = EJSON.parse(this.queryParams.query); + query = clean(query, pathAllowConf.def); + } catch (e) { + this.logger.warn(`Invalid query parameter provided "${this.queryParams.query}":`, e); + throw new Meteor.Error('error-invalid-query', `Invalid query parameter provided: "${this.queryParams.query}"`, { + helperMethod: 'parseJsonQuery', + }); + } + } + + // Verify the user has permission to query the fields they are + if (typeof query === 'object') { + let nonQueryableFields = Object.keys(API.v1.defaultFieldsToExclude); + + if (this.request.route.includes('/v1/users.')) { + if (hasPermission(this.userId, 'view-full-other-user-info')) { + nonQueryableFields = nonQueryableFields.concat(Object.keys(API.v1.limitedUserFieldsToExcludeIfIsPrivilegedUser)); + } else { + nonQueryableFields = nonQueryableFields.concat(Object.keys(API.v1.limitedUserFieldsToExclude)); + } + } + + if (this.queryFields && !isValidQuery(query, this.queryFields || ['*'], this.queryOperations ?? pathAllowConf.def)) { + throw new Meteor.Error('error-invalid-query', isValidQuery.errors.join('\n')); + } + + Object.keys(query).forEach((k) => { + if (nonQueryableFields.includes(k) || nonQueryableFields.includes(k.split(API.v1.fieldSeparator)[0])) { + query && delete query[k]; + } + }); + } + + return { + sort, + fields, + query, + }; + }, +); diff --git a/apps/meteor/app/api/server/lib/cleanQuery.ts b/apps/meteor/app/api/server/lib/cleanQuery.ts index 1fe250e9460..0b4d1d9952e 100644 --- a/apps/meteor/app/api/server/lib/cleanQuery.ts +++ b/apps/meteor/app/api/server/lib/cleanQuery.ts @@ -2,7 +2,7 @@ type Query = { [k: string]: any }; const denyList = ['constructor', '__proto__', 'prototype']; -const removeDangerousProps = (v: Query): Query => { +export const removeDangerousProps = (v: Query): Query => { const query = Object.create(null); for (const key in v) { if (v.hasOwnProperty(key) && !denyList.includes(key)) { @@ -12,7 +12,7 @@ const removeDangerousProps = (v: Query): Query => { return query; }; - +/* @deprecated */ export function clean(v: Query, allowList: string[] = []): Query { const typedParam = removeDangerousProps(v); if (v instanceof Object) { diff --git a/apps/meteor/app/api/server/lib/isValidQuery.ts b/apps/meteor/app/api/server/lib/isValidQuery.ts new file mode 100644 index 00000000000..361e49966f6 --- /dev/null +++ b/apps/meteor/app/api/server/lib/isValidQuery.ts @@ -0,0 +1,58 @@ +import { removeDangerousProps } from './cleanQuery'; + +type Query = { [k: string]: any }; + +export const isValidQuery: { + (query: Query, allowedAttributes: string[], allowedOperations: string[]): boolean; + errors: string[]; +} = Object.assign( + (query: Query, allowedAttributes: string[], allowedOperations: string[]): boolean => { + isValidQuery.errors = []; + if (!(query instanceof Object)) { + throw new Error('query must be an object'); + } + + // eslint-disable-next-line @typescript-eslint/no-use-before-define + return verifyQuery(query, allowedAttributes, allowedOperations); + }, + { + errors: [], + }, +); + +export 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}`); + return false; + } + if (!Array.isArray(value)) { + isValidQuery.errors.push(`Invalid parameter for operation: ${path} : ${value}`); + return false; + } + return value.every((v) => verifyQuery(v, allowedAttributes, allowedOperations)); + } + + if ( + !allowedAttributes.some((allowedAttribute) => { + if (allowedAttribute.endsWith('.*')) { + return path.startsWith(allowedAttribute.replace('.*', '')); + } + if (allowedAttribute.endsWith('*') && !allowedAttribute.endsWith('.*')) { + return true; + } + return path === allowedAttribute; + }) + ) { + isValidQuery.errors.push(`Invalid attribute: ${path}`); + return false; + } + + if (value instanceof Object) { + return verifyQuery(value, allowedAttributes, allowedOperations, path); + } + return true; + }); +}; diff --git a/apps/meteor/app/api/server/v1/users.js b/apps/meteor/app/api/server/v1/users.js index 9dad4a9811f..d57d477a4cf 100644 --- a/apps/meteor/app/api/server/v1/users.js +++ b/apps/meteor/app/api/server/v1/users.js @@ -27,6 +27,7 @@ import { resetUserE2EEncriptionKey } from '../../../../server/lib/resetUserE2EKe import { setUserStatus } from '../../../../imports/users-presence/server/activeUsers'; import { resetTOTP } from '../../../2fa/server/functions/resetTOTP'; import { Team } from '../../../../server/sdk'; +import { isValidQuery } from '../lib/isValidQuery'; API.v1.addRoute( 'users.create', @@ -269,7 +270,10 @@ API.v1.addRoute( API.v1.addRoute( 'users.list', - { authRequired: true }, + { + authRequired: true, + queryOperations: ['$or', '$and'], + }, { get() { if (!hasPermission(this.userId, 'view-d-room')) { @@ -284,6 +288,23 @@ API.v1.addRoute( const inclusiveFields = getInclusiveFields(nonEmptyFields); + const inclusiveFieldsKeys = Object.keys(inclusiveFields); + + if ( + !isValidQuery( + nonEmptyQuery, + [ + ...inclusiveFieldsKeys, + inclusiveFieldsKeys.includes('emails') && 'emails.address.*', + inclusiveFieldsKeys.includes('username') && 'username.*', + inclusiveFieldsKeys.includes('name') && 'name.*', + ].filter(Boolean), + this.queryOperations, + ) + ) { + throw new Meteor.Error('error-invalid-query', isValidQuery.errors.join('\n')); + } + const actualSort = sort && sort.name ? { nameInsensitive: sort.name, ...sort } : sort || { username: 1 }; const limit = 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 new file mode 100644 index 00000000000..175778f9fba --- /dev/null +++ b/apps/meteor/tests/unit/app/api/server/v1/lib/isValidQuery.spec.ts @@ -0,0 +1,216 @@ +import { expect } from 'chai'; +import { describe, it } from 'mocha'; + +import { isValidQuery } from '../../../../../../../app/api/server/lib/isValidQuery'; + +describe.only('isValidQuery', () => { + describe('shallow keys', () => { + it('should return false if the query contains an operation that is not in the props array', () => { + const props = ['_id', 'name']; + const query = { + $or: [ + { + _id: '123', + }, + { + name: '456', + }, + ], + }; + expect(isValidQuery(query, props, [])).to.be.false; + expect(isValidQuery.errors.length).to.be.equals(1); + }); + + it('should return true if the query contains operation and the attributes are set in the props array ', () => { + const props = ['_id', 'name']; + const query = { + $or: [ + { + _id: '123', + }, + { + name: '456', + }, + ], + }; + expect(isValidQuery(query, props, ['$or'])).to.be.true; + expect(isValidQuery.errors.length).to.be.equals(0); + }); + + it('should return false if the query contains operations allowed but some attributes are not set in the props array ', () => { + const props = ['name']; + const query = { + $or: [ + { + _id: '123', + }, + { + name: '456', + }, + ], + }; + expect(isValidQuery(query, props, ['$or'])).to.be.false; + expect(isValidQuery.errors.length).to.be.equals(1); + }); + + it('should return true if the query contains only attributes set in the props array ', () => { + const props = ['_id', 'name']; + const query = { + _id: '123', + name: '456', + }; + expect(isValidQuery(query, props, [])).to.be.true; + expect(isValidQuery.errors.length).to.be.equals(0); + }); + + it('should return false if the query contains an attribute that is not in the props array ', () => { + const props = ['_id']; + const query = { + _id: '123', + name: '456', + }; + expect(isValidQuery(query, props, [])).to.be.false; + expect(isValidQuery.errors.length).to.be.equals(1); + }); + }); + + describe('deep keys', () => { + it('should return false if the query contains deep attributes that are not set on allowed keys', () => { + const props = ['_id', 'name']; + const query = { + user: { + _id: '123', + name: '456', + }, + }; + expect(isValidQuery(query, props, [])).to.be.false; + expect(isValidQuery.errors.length).to.be.equals(1); + }); + + it('should return false if the query contains deep attributes that are and are not set as allowed', () => { + const props = ['user', '_id', 'name']; + const query = { + user: { + _id: '123', + name: '456', + }, + }; + expect(isValidQuery(query, props, [])).to.be.false; + expect(isValidQuery.errors.length).to.be.equals(1); + }); + + it('should return true if the query contains deep attributes that are set on allowed keys', () => { + const props = ['user', 'user._id', 'user.name']; + const query = { + user: { + _id: '123', + name: '456', + }, + }; + expect(isValidQuery(query, props, [])).to.be.true; + expect(isValidQuery.errors.length).to.be.equals(0); + }); + + it('should return true if the query contains deep attributes that are set on allowed keys even for many layers', () => { + const props = ['user', 'user._id', 'user.name', 'user.address', 'user.address.city']; + const query = { + user: { + _id: '123', + name: '456', + address: { + city: 'New York', + }, + }, + }; + expect(isValidQuery(query, props, [])).to.be.true; + expect(isValidQuery.errors.length).to.be.equals(0); + }); + }); + + describe('using .* for match keys', () => { + it('should return true if the query contains attributes and * are being used', () => { + const props = ['user', 'user.*']; + const query = { + user: { + _id: '123', + name: '456', + }, + }; + expect(isValidQuery(query, props, [])).to.be.true; + expect(isValidQuery.errors.length).to.be.equals(0); + }); + }); + + describe('using * for match keys', () => { + it('should return true if the query contains attributes and * are being used', () => { + const props = ['user', '*']; + const query = { + user: { + _id: '123', + name: '456', + }, + }; + expect(isValidQuery(query, props, [])).to.be.true; + expect(isValidQuery.errors.length).to.be.equals(0); + }); + it('should return false if query uses * but the operation is not allowed', () => { + const props = ['user', '*']; + const query = { + $or: [{ user: '123' }, { user: '456' }], + }; + expect(isValidQuery(query, props, [])).to.be.false; + expect(isValidQuery.errors.length).to.be.equals(1); + }); + }); + + describe('testing $regex', () => { + it('should return true if the query contains attributes and * are being used', () => { + const props = ['user.*']; + const query = { + user: { + _id: '123', + name: { + $regex: '*', + }, + }, + }; + expect(isValidQuery(query, props, ['$or'])).to.be.true; + expect(isValidQuery.errors.length).to.be.equals(0); + }); + + it('should return false for services.password.reset.token', () => { + const query = { + $or: [ + { 'emails.address': { $regex: '', $options: 'i' } }, + { username: { $regex: '', $options: 'i' } }, + { name: { $regex: '', $options: 'i' } }, + ], + $and: [{ username: 'g1' }, { 'services.password.reset.token': { $regex: '.*' } }], + }; + expect( + isValidQuery( + query, + ['name', 'username', 'emails', 'roles', 'status', 'active', 'avatarETag', 'lastLogin', 'email.address.*', 'username.*', 'name.*'], + ['$or', '$and'], + ), + ).to.be.false; + }); + it('should return false for services.totp.secret', () => { + const query = { + $or: [ + { 'emails.address': { $regex: '', $options: 'i' } }, + { username: { $regex: '', $options: 'i' } }, + { name: { $regex: '', $options: 'i' } }, + ], + $and: [{ username: 'g1' }, { 'services.totp.secret': { $regex: '.*' } }], + }; + expect( + isValidQuery( + query, + ['name', 'username', 'emails', 'roles', 'status', 'active', 'avatarETag', 'lastLogin', 'email.address.*', 'username.*', 'name.*'], + ['$or', '$and'], + ), + ).to.be.false; + }); + }); +}); -- GitLab