From dbf514e85d1508b95c007462a39b3cadd4ff391d Mon Sep 17 00:00:00 2001 From: David Luecke Date: Sun, 29 Jan 2023 09:16:37 -0800 Subject: [PATCH] fix(databases): Ensure that query sanitization is not necessary when using query schemas (#3022) --- packages/adapter-commons/src/index.ts | 2 +- packages/adapter-commons/src/query.ts | 36 ++++++++------ packages/knex/package.json | 1 + packages/knex/src/adapter.ts | 13 +++-- packages/knex/test/index.test.ts | 38 ++++++++++++++ packages/mongodb/package.json | 1 + packages/mongodb/src/adapter.ts | 9 ++-- packages/mongodb/test/index.test.ts | 71 ++++++++++++++++++++++----- packages/schema/src/json-schema.ts | 29 ++++++++--- 9 files changed, 158 insertions(+), 42 deletions(-) diff --git a/packages/adapter-commons/src/index.ts b/packages/adapter-commons/src/index.ts index c7d585a296..8007e44e00 100644 --- a/packages/adapter-commons/src/index.ts +++ b/packages/adapter-commons/src/index.ts @@ -3,7 +3,7 @@ import { Params } from '@feathersjs/feathers' export * from './declarations' export * from './service' -export { filterQuery, FILTERS, OPERATORS } from './query' +export * from './query' export * from './sort' // Return a function that filters a result object or array diff --git a/packages/adapter-commons/src/query.ts b/packages/adapter-commons/src/query.ts index f2001e15bb..0d2daa88c6 100644 --- a/packages/adapter-commons/src/query.ts +++ b/packages/adapter-commons/src/query.ts @@ -1,7 +1,7 @@ import { _ } from '@feathersjs/commons' import { BadRequest } from '@feathersjs/errors' import { Query } from '@feathersjs/feathers' -import { FilterQueryOptions, FilterSettings } from './declarations' +import { FilterQueryOptions, FilterSettings, PaginationParams } from './declarations' const parse = (value: any) => (typeof value !== 'undefined' ? parseInt(value, 10) : value) @@ -64,6 +64,26 @@ const getQuery = (query: Query, settings: FilterQueryOptions) => { }, {} as Query) } +/** + * Returns the converted `$limit` value based on the `paginate` configuration. + * @param _limit The limit value + * @param paginate The pagination options + * @returns The converted $limit value + */ +export const getLimit = (_limit: any, paginate?: PaginationParams) => { + const limit = parse(_limit) + + if (paginate && (paginate.default || paginate.max)) { + const base = paginate.default || 0 + const lower = typeof limit === 'number' && !isNaN(limit) && limit >= 0 ? limit : base + const upper = typeof paginate.max === 'number' ? paginate.max : Number.MAX_VALUE + + return Math.min(lower, upper) + } + + return limit +} + export const OPERATORS = ['$in', '$nin', '$lt', '$lte', '$gt', '$gte', '$ne', '$or'] export const FILTERS: FilterSettings = { @@ -79,19 +99,7 @@ export const FILTERS: FilterSettings = { return result }, {} as { [key: string]: number }) }, - $limit: (_limit: any, { paginate }: FilterQueryOptions) => { - const limit = parse(_limit) - - if (paginate && (paginate.default || paginate.max)) { - const base = paginate.default || 0 - const lower = typeof limit === 'number' && !isNaN(limit) && limit >= 0 ? limit : base - const upper = typeof paginate.max === 'number' ? paginate.max : Number.MAX_VALUE - - return Math.min(lower, upper) - } - - return limit - }, + $limit: (_limit: any, { paginate }: FilterQueryOptions) => getLimit(_limit, paginate), $select: (select: any) => { if (Array.isArray(select)) { return select.map((current) => `${current}`) diff --git a/packages/knex/package.json b/packages/knex/package.json index 111a4c88ad..de736fb9e1 100644 --- a/packages/knex/package.json +++ b/packages/knex/package.json @@ -61,6 +61,7 @@ }, "devDependencies": { "@feathersjs/adapter-tests": "^5.0.0-pre.35", + "@feathersjs/schema": "^5.0.0-pre.35", "@types/mocha": "^10.0.1", "@types/node": "^18.11.18", "knex": "^2.3.0", diff --git a/packages/knex/src/adapter.ts b/packages/knex/src/adapter.ts index 5024877006..65100c5e8b 100644 --- a/packages/knex/src/adapter.ts +++ b/packages/knex/src/adapter.ts @@ -1,6 +1,6 @@ import { Id, NullableId, Paginated, Query } from '@feathersjs/feathers' import { _ } from '@feathersjs/commons' -import { AdapterBase, PaginationOptions, filterQuery } from '@feathersjs/adapter-commons' +import { AdapterBase, PaginationOptions, AdapterQuery, getLimit } from '@feathersjs/adapter-commons' import { BadRequest, MethodNotAllowed, NotFound } from '@feathersjs/errors' import { Knex } from 'knex' @@ -50,7 +50,7 @@ export class KnexAdapter< ...options.filters, $and: (value: any) => value }, - operators: [...(options.operators || []), '$like', '$notlike', '$ilike', '$and', '$or'] + operators: [...(options.operators || []), '$like', '$notlike', '$ilike'] }) } @@ -150,9 +150,14 @@ export class KnexAdapter< filterQuery(params: ServiceParams) { const options = this.getOptions(params) - const { filters, query } = filterQuery(params?.query || {}, options) + const { $select, $sort, $limit: _limit, $skip = 0, ...query } = (params.query || {}) as AdapterQuery + const $limit = getLimit(_limit, options.paginate) - return { filters, query, paginate: options.paginate } + return { + paginate: options.paginate, + filters: { $select, $sort, $limit, $skip }, + query + } } async _find(params?: ServiceParams & { paginate?: PaginationOptions }): Promise> diff --git a/packages/knex/test/index.test.ts b/packages/knex/test/index.test.ts index c805ec8abd..4161561218 100644 --- a/packages/knex/test/index.test.ts +++ b/packages/knex/test/index.test.ts @@ -3,6 +3,7 @@ import assert from 'assert' import { feathers, HookContext, Service } from '@feathersjs/feathers' import adapterTests from '@feathersjs/adapter-tests' import { errors } from '@feathersjs/errors' +import { Ajv, getValidator, querySyntax, hooks } from '@feathersjs/schema' import connection from './connection' import { ERROR, KnexAdapterParams, KnexService, transaction } from '../src/index' @@ -126,6 +127,38 @@ const clean = async () => { }) } +const personSchema = { + $id: 'Person', + type: 'object', + additionalProperties: false, + required: ['_id', 'name', 'age'], + properties: { + id: { type: 'number' }, + name: { type: 'string' }, + age: { type: ['number', 'null'] }, + time: { type: 'string' }, + create: { type: 'boolean' } + } +} as const +const personQuery = { + $id: 'PersonQuery', + type: 'object', + additionalProperties: false, + properties: { + ...querySyntax(personSchema.properties, { + name: { + $like: { type: 'string' }, + $ilike: { type: 'string' }, + $notlike: { type: 'string' } + } + }) + } +} as const +const validator = new Ajv({ + coerceTypes: true +}) +const personQueryValidator = getValidator(personQuery, validator) + type Person = { id: number name: string @@ -195,6 +228,11 @@ describe('Feathers Knex Service', () => { .use('todos', todos) const peopleService = app.service('people') + peopleService.hooks({ + before: { + find: [hooks.validateQuery(personQueryValidator)] + } + }) before(() => { if (TYPE === 'sqlite') { // Attach the public database to mimic a "schema" diff --git a/packages/mongodb/package.json b/packages/mongodb/package.json index 6447abab05..b01644775b 100644 --- a/packages/mongodb/package.json +++ b/packages/mongodb/package.json @@ -61,6 +61,7 @@ }, "devDependencies": { "@feathersjs/adapter-tests": "^5.0.0-pre.35", + "@feathersjs/schema": "^5.0.0-pre.35", "@types/mocha": "^10.0.1", "@types/node": "^18.11.18", "mocha": "^10.2.0", diff --git a/packages/mongodb/src/adapter.ts b/packages/mongodb/src/adapter.ts index 7a88392590..877f72d898 100644 --- a/packages/mongodb/src/adapter.ts +++ b/packages/mongodb/src/adapter.ts @@ -17,7 +17,8 @@ import { AdapterParams, AdapterServiceOptions, PaginationOptions, - AdapterQuery + AdapterQuery, + getLimit } from '@feathersjs/adapter-commons' import { Id, Paginated } from '@feathersjs/feathers' import { errorHandler } from './error-handler' @@ -75,8 +76,9 @@ export class MongoDbAdapter< } filterQuery(id: NullableAdapterId, params: ServiceParams) { - const { $select, $sort, $limit, $skip, ...query } = (params.query || {}) as AdapterQuery - + const options = this.getOptions(params) + const { $select, $sort, $limit: _limit, $skip = 0, ...query } = (params.query || {}) as AdapterQuery + const $limit = getLimit(_limit, options.paginate) if (id !== null) { query.$and = (query.$and || []).concat({ [this.id]: this.getObjectId(id) @@ -226,7 +228,6 @@ export class MongoDbAdapter< const { paginate, useEstimatedDocumentCount } = this.getOptions(params) const { filters, query } = this.filterQuery(null, params) const useAggregation = !params.mongodb && filters.$limit !== 0 - const countDocuments = async () => { if (paginate && paginate.default) { const model = await this.getModel(params) diff --git a/packages/mongodb/test/index.test.ts b/packages/mongodb/test/index.test.ts index 9759b5e175..2b9e30d2ab 100644 --- a/packages/mongodb/test/index.test.ts +++ b/packages/mongodb/test/index.test.ts @@ -2,10 +2,10 @@ import { Db, MongoClient, ObjectId } from 'mongodb' import adapterTests from '@feathersjs/adapter-tests' import assert from 'assert' import { MongoMemoryServer } from 'mongodb-memory-server' - +import { Ajv, FromSchema, getValidator, hooks, querySyntax } from '@feathersjs/schema' import { feathers } from '@feathersjs/feathers' import errors from '@feathersjs/errors' -import { MongoDBService } from '../src' +import { MongoDBService, AdapterId } from '../src' const testSuite = adapterTests([ '.options', @@ -82,16 +82,43 @@ const testSuite = adapterTests([ ]) describe('Feathers MongoDB Service', () => { - type Person = { - _id: string - name: string - age: number - friends?: string[] - team: string - $push: { - friends: string + const personSchema = { + $id: 'Person', + type: 'object', + additionalProperties: false, + required: ['_id', 'name', 'age'], + properties: { + _id: { oneOf: [{ type: 'string' }, { type: 'object' }] }, + name: { type: 'string' }, + age: { type: 'number' }, + friends: { type: 'array', items: { type: 'string' } }, + team: { type: 'string' }, + $push: { + type: 'object', + properties: { + friends: { type: 'string' } + } + } } - } + } as const + const personQuery = { + $id: 'PersonQuery', + type: 'object', + additionalProperties: false, + properties: { + ...querySyntax(personSchema.properties, { + name: { + $regex: { type: 'string' } + } + }) + } + } as const + const validator = new Ajv({ + coerceTypes: true + }) + const personQueryValidator = getValidator(personQuery, validator) + + type Person = Omit, '_id'> & { _id: AdapterId } type Todo = { _id: string name: string @@ -135,6 +162,12 @@ describe('Feathers MongoDB Service', () => { }) ) + app.service('people').hooks({ + before: { + find: [hooks.validateQuery(personQueryValidator)] + } + }) + db.collection('people-customid').deleteMany({}) db.collection('people').deleteMany({}) db.collection('todos').deleteMany({}) @@ -441,6 +474,22 @@ describe('Feathers MongoDB Service', () => { }) }) + describe('query validation', () => { + it('validated queries are not sanitized', async () => { + const dave = await app.service('people').create({ name: 'Dave' }) + const result = await app.service('people').find({ + query: { + name: { + $regex: 'Da.*' + } + } + }) + assert.deepStrictEqual(result, [dave]) + + app.service('people').remove(dave._id) + }) + }) + testSuite(app, errors, 'people', '_id') testSuite(app, errors, 'people-customid', 'customid') }) diff --git a/packages/schema/src/json-schema.ts b/packages/schema/src/json-schema.ts index 5c871c84e6..b951e68e21 100644 --- a/packages/schema/src/json-schema.ts +++ b/packages/schema/src/json-schema.ts @@ -172,6 +172,25 @@ export const querySyntax = < ) => { const keys = Object.keys(definition) const props = queryProperties(definition, extensions) + const $or = { + type: 'array', + items: { + type: 'object', + additionalProperties: false, + properties: props + } + } as const + const $and = { + type: 'array', + items: { + type: 'object', + additionalProperties: false, + properties: { + ...props, + $or + } + } + } as const return { $limit: { @@ -203,14 +222,8 @@ export const querySyntax = < ...(keys.length > 0 ? { enum: keys as any as (keyof T)[] } : {}) } }, - $or: { - type: 'array', - items: { - type: 'object', - additionalProperties: false, - properties: props - } - }, + $or, + $and, ...props } as const }