diff --git a/errors/errors.json b/errors/errors.json index 5bf166b..71bb14d 100644 --- a/errors/errors.json +++ b/errors/errors.json @@ -6,6 +6,13 @@ "description": "Not a valid ObjectId", "statusCode": 400 }, + "MONGO_BLOCKED_OPERATOR": { + "data": { + "operator": "The blocked operator" + }, + "description": "A query contained a blocked MongoDB operator that could allow arbitrary code execution", + "statusCode": 400 + }, "MONGO_CONN_FAILED": { "data": { "error": "The error message" diff --git a/lib/MongoDBModule.js b/lib/MongoDBModule.js index eb73e8e..572a776 100644 --- a/lib/MongoDBModule.js +++ b/lib/MongoDBModule.js @@ -1,9 +1,6 @@ import { AbstractModule } from 'adapt-authoring-core' import { MongoClient } from 'mongodb' -import { convertObjectIds } from './utils/convertObjectIds.js' -import { findDuplicates } from './utils/findDuplicates.js' -import { isValidObjectId } from './utils/isValidObjectId.js' -import { parseObjectId } from './utils/parseObjectId.js' +import { findDuplicates, isValidObjectId, parseObjectId, processParams } from './utils.js' /** * Represents a single MongoDB server instance * @memberof mongodb @@ -94,25 +91,6 @@ class MongoDBModule extends AbstractModule { } } - /** - * Makes sure options are in the correct format. - * @param {Object} options The options to parse - */ - parseOptions (options) { - if (!options) { - return - } - ['limit', 'skip'].forEach(o => { - if (options[o] === undefined) return - try { - options[o] = parseInt(options[o]) - } catch (e) { - this.log('warn', `value for option '${o}' is in an unexpected format and will be ignored`) - delete options[o] - } - }) - } - /** * Adds a new object to the database * @param {String} collectionName The name of the MongoDB collection @@ -123,14 +101,9 @@ class MongoDBModule extends AbstractModule { * @see https://mongodb.github.io/node-mongodb-native/4.2/classes/Collection.html#insertOne */ async insert (collectionName, data, options = {}) { - const { preserveId, ...mongoOptions } = options - convertObjectIds(data) - this.parseOptions(mongoOptions) - if (!preserveId) { - delete data._id - } + const p = processParams({ data, options }) try { - const { insertedId } = await this.getCollection(collectionName).insertOne(data, mongoOptions) + const { insertedId } = await this.getCollection(collectionName).insertOne(p.data, p.options) const [doc] = await this.find(collectionName, { _id: insertedId }) return doc } catch (e) { @@ -148,11 +121,10 @@ class MongoDBModule extends AbstractModule { * @see https://mongodb.github.io/node-mongodb-native/4.2/classes/Collection.html#find */ async find (collectionName, query, options) { - convertObjectIds(query) - this.parseOptions(options) + const p = processParams({ query, options }) try { - const cursor = this.getCollection(collectionName).find(query, options) - return options?.returnCursor === true ? cursor : await cursor.toArray() + const cursor = this.getCollection(collectionName).find(p.query, p.options) + return p.options?.returnCursor === true ? cursor : await cursor.toArray() } catch (e) { this.log('error', `failed to find docs, ${e.message}`) throw this.getError(collectionName, 'find', e) @@ -169,15 +141,9 @@ class MongoDBModule extends AbstractModule { * @see https://mongodb.github.io/node-mongodb-native/4.2/classes/Collection.html#findOneAndUpdate */ async update (collectionName, query, data, options) { - const opts = Object.assign({ includeResultMetadata: false, returnDocument: 'after' }, options) - this.parseOptions(opts) - convertObjectIds(query) - convertObjectIds(data) - // MongoDB doesn't like the explicit setting of _id - delete data._id - if (data.$set) delete data.$set._id + const p = processParams({ query, data, options: { includeResultMetadata: false, returnDocument: 'after', ...options } }) try { - return await this.getCollection(collectionName).findOneAndUpdate(query, data, opts) + return await this.getCollection(collectionName).findOneAndUpdate(p.query, p.data, p.options) } catch (e) { this.log('error', `failed to update doc, ${e.message}`) throw this.getError(collectionName, 'update', e) @@ -193,15 +159,10 @@ class MongoDBModule extends AbstractModule { * @see https://mongodb.github.io/node-mongodb-native/4.2/classes/Collection.html#updateMany */ async updateMany (collectionName, query, data, options) { - this.parseOptions(options) - convertObjectIds(query) - convertObjectIds(data) - // MongoDB doesn't like the explicit setting of _id - delete data._id - if (data.$set) delete data.$set._id + const p = processParams({ query, data, options }) try { - await this.getCollection(collectionName).updateMany(query, data, options) - return this.find(collectionName, query) + await this.getCollection(collectionName).updateMany(p.query, p.data, p.options) + return this.find(collectionName, p.query) } catch (e) { this.log('error', `failed to update docs, ${e.message}`) throw this.getError(collectionName, 'update', e) @@ -218,15 +179,9 @@ class MongoDBModule extends AbstractModule { * @see https://mongodb.github.io/node-mongodb-native/4.2/classes/Collection.html#findOneAndReplace */ async replace (collectionName, query, data, options) { - const opts = Object.assign({ includeResultMetadata: false, returnDocument: 'after' }, options) - convertObjectIds(query) - convertObjectIds(data) - this.parseOptions(options) - // MongoDB doesn't like the explicit setting of _id - delete data._id - if (data.$set) delete data.$set._id + const p = processParams({ query, data, options: { includeResultMetadata: false, returnDocument: 'after', ...options } }) try { - return await this.getCollection(collectionName).findOneAndReplace(query, data, opts) + return await this.getCollection(collectionName).findOneAndReplace(p.query, p.data, p.options) } catch (e) { this.log('error', `failed to replace doc, ${e.message}`) throw this.getError(collectionName, 'replace', e) @@ -242,10 +197,9 @@ class MongoDBModule extends AbstractModule { * @see https://mongodb.github.io/node-mongodb-native/4.2/classes/Collection.html#deleteOne */ async delete (collectionName, query, options) { - convertObjectIds(query) - this.parseOptions(options) + const p = processParams({ query, options }) try { - await this.getCollection(collectionName).deleteOne(query, options) + await this.getCollection(collectionName).deleteOne(p.query, p.options) } catch (e) { this.log('error', `failed to delete doc, ${e.message}`) throw this.getError(collectionName, 'delete', e) @@ -261,10 +215,9 @@ class MongoDBModule extends AbstractModule { * @see https://mongodb.github.io/node-mongodb-native/4.2/classes/Collection.html#deleteMany */ async deleteMany (collectionName, query, options) { - convertObjectIds(query) - this.parseOptions(options) + const p = processParams({ query, options }) try { - await this.getCollection(collectionName).deleteMany(query, options) + await this.getCollection(collectionName).deleteMany(p.query, p.options) } catch (e) { this.log('error', `failed to delete docs, ${e.message}`) throw this.getError(collectionName, 'delete', e) diff --git a/lib/utils.js b/lib/utils.js index b02f417..f6ffdd0 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -1,5 +1,8 @@ +export { assertSafeQuery } from './utils/assertSafeQuery.js' +export { convertObjectIds } from './utils/convertObjectIds.js' export { createObjectId } from './utils/createObjectId.js' +export { findDuplicates, getFieldNames } from './utils/findDuplicates.js' export { isObjectId } from './utils/isObjectId.js' export { isValidObjectId } from './utils/isValidObjectId.js' export { parseObjectId } from './utils/parseObjectId.js' -export { convertObjectIds } from './utils/convertObjectIds.js' +export { processParams } from './utils/processParams.js' diff --git a/lib/utils/assertSafeQuery.js b/lib/utils/assertSafeQuery.js new file mode 100644 index 0000000..1a4a4a4 --- /dev/null +++ b/lib/utils/assertSafeQuery.js @@ -0,0 +1,31 @@ +/** + * Operators that allow arbitrary JavaScript execution on the MongoDB server. + * @type {Set} + */ +const BLOCKED_OPERATORS = new Set(['$where', '$accumulator', '$function']) + +/** + * Recursively checks a MongoDB query object for dangerous operators that could + * allow arbitrary code execution on the server. + * @param {*} input The query object (or nested value) to check + * @param {ErrorsModule} errors The app errors object + * @throws {AdaptError} If a blocked operator is found + * @memberof mongodb + */ +export function assertSafeQuery (input, errors) { + if (Array.isArray(input)) { + for (const item of input) { + assertSafeQuery(item, errors) + } + return + } + if (input === null || typeof input !== 'object') { + return + } + for (const key of Object.keys(input)) { + if (BLOCKED_OPERATORS.has(key)) { + throw errors.MONGO_BLOCKED_OPERATOR.setData({ operator: key }) + } + assertSafeQuery(input[key], errors) + } +} diff --git a/lib/utils/processParams.js b/lib/utils/processParams.js new file mode 100644 index 0000000..ded8f41 --- /dev/null +++ b/lib/utils/processParams.js @@ -0,0 +1,71 @@ +import { App, isObject } from 'adapt-authoring-core' +import { assertSafeQuery } from './assertSafeQuery.js' +import { convertObjectIds } from './convertObjectIds.js' + +function deepClone (v) { + if (Array.isArray(v)) return v.map(deepClone) + if (v !== null && typeof v === 'object' && v.constructor === Object) { + return Object.fromEntries(Object.entries(v).map(([k, v2]) => [k, deepClone(v2)])) + } + return v +} + +/** + * Validators for each allowed MongoDB driver option. + * Each function returns a parsed value if valid, or undefined to strip the option. + * @type {Object} + */ +const OPTION_VALIDATORS = { + collation: v => isObject(v) ? v : undefined, + includeResultMetadata: v => typeof v === 'boolean' ? v : undefined, + limit: v => { const n = Number(v); return Number.isInteger(n) ? n : undefined }, + projection: v => isObject(v) ? v : undefined, + returnCursor: v => typeof v === 'boolean' ? v : undefined, + returnDocument: v => v === 'before' || v === 'after' ? v : undefined, + skip: v => { const n = Number(v); return Number.isInteger(n) ? n : undefined }, + sort: v => isObject(v) ? v : undefined, + upsert: v => typeof v === 'boolean' ? v : undefined +} + +/** + * Validates and normalises query, data, and options before a database operation. + * Returns deep copies so the caller's originals are not mutated. + * @param {Object} params + * @param {Object} [params.query] The query object to validate and convert + * @param {Object} [params.data] The data object to convert and sanitise + * @param {Object} [params.options] The options object to parse + * @returns {{ query: Object, data: Object, options: Object }} + * @memberof mongodb + */ +export function processParams ({ query, data, options } = {}) { + const result = {} + + if (query) { + result.query = deepClone(query) + assertSafeQuery(result.query, App.instance.errors) + convertObjectIds(result.query) + } + + if (data) { + result.data = deepClone(data) + convertObjectIds(result.data) + if (!options?.preserveId) { + delete result.data._id + if (result.data.$set) delete result.data.$set._id + } + } + + if (options) { + result.options = {} + for (const [key, value] of Object.entries(options)) { + const validate = OPTION_VALIDATORS[key] + if (!validate) continue + const parsed = validate(value) + if (parsed !== undefined) { + result.options[key] = parsed + } + } + } + + return result +} diff --git a/tests/MongoDBModule.spec.js b/tests/MongoDBModule.spec.js index ecc5327..1fef1a9 100644 --- a/tests/MongoDBModule.spec.js +++ b/tests/MongoDBModule.spec.js @@ -4,7 +4,7 @@ import MongoDBModule from '../lib/MongoDBModule.js' /** * MongoDBModule extends AbstractModule and requires a running MongoDB connection. - * We test parseOptions and getError in isolation. + * We test getError in isolation (parseOptions was replaced by processParams utility). */ function createInstance () { @@ -34,48 +34,6 @@ function createInstance () { } describe('MongoDBModule', () => { - describe('#parseOptions()', () => { - it('should parse string limit to integer', () => { - const { instance } = createInstance() - const options = { limit: '10' } - instance.parseOptions(options) - assert.equal(options.limit, 10) - }) - - it('should parse string skip to integer', () => { - const { instance } = createInstance() - const options = { skip: '5' } - instance.parseOptions(options) - assert.equal(options.skip, 5) - }) - - it('should handle undefined options gracefully', () => { - const { instance } = createInstance() - instance.parseOptions(undefined) - }) - - it('should handle options without limit or skip', () => { - const { instance } = createInstance() - const options = { sort: { name: 1 } } - instance.parseOptions(options) - assert.deepEqual(options, { sort: { name: 1 } }) - }) - - it('should keep numeric limit as-is', () => { - const { instance } = createInstance() - const options = { limit: 10 } - instance.parseOptions(options) - assert.equal(options.limit, 10) - }) - - it('should skip undefined limit', () => { - const { instance } = createInstance() - const options = { limit: undefined } - instance.parseOptions(options) - assert.equal(options.limit, undefined) - }) - }) - describe('#getError()', () => { it('should return MONGO_IMMUTABLE_FIELD for error code 66', () => { const { instance } = createInstance() diff --git a/tests/utils-assertSafeQuery.spec.js b/tests/utils-assertSafeQuery.spec.js new file mode 100644 index 0000000..918c09a --- /dev/null +++ b/tests/utils-assertSafeQuery.spec.js @@ -0,0 +1,123 @@ +import { describe, it } from 'node:test' +import assert from 'node:assert/strict' +import { assertSafeQuery } from '../lib/utils/assertSafeQuery.js' + +const errors = { + MONGO_BLOCKED_OPERATOR: { + setData (data) { + const e = new Error('MONGO_BLOCKED_OPERATOR') + e.data = data + return e + } + } +} + +describe('assertSafeQuery()', () => { + it('should allow a simple field query', () => { + assert.doesNotThrow(() => assertSafeQuery({ name: 'test' }, errors)) + }) + + it('should allow safe query operators', () => { + assert.doesNotThrow(() => assertSafeQuery({ age: { $gt: 18, $lt: 65 } }, errors)) + }) + + it('should allow $or and $and operators', () => { + assert.doesNotThrow(() => assertSafeQuery({ + $or: [{ name: 'a' }, { name: 'b' }], + $and: [{ active: true }] + }, errors)) + }) + + it('should allow $regex operator', () => { + assert.doesNotThrow(() => assertSafeQuery({ name: { $regex: 'test', $options: 'i' } }, errors)) + }) + + it('should allow $in and $nin operators', () => { + assert.doesNotThrow(() => assertSafeQuery({ status: { $in: ['active', 'pending'] } }, errors)) + }) + + it('should allow $exists and $type operators', () => { + assert.doesNotThrow(() => assertSafeQuery({ field: { $exists: true, $type: 'string' } }, errors)) + }) + + it('should throw MONGO_BLOCKED_OPERATOR for $where', () => { + assert.throws( + () => assertSafeQuery({ $where: 'this.a > this.b' }, errors), + (err) => { + assert.equal(err.message, 'MONGO_BLOCKED_OPERATOR') + assert.deepEqual(err.data, { operator: '$where' }) + return true + } + ) + }) + + it('should throw MONGO_BLOCKED_OPERATOR for $accumulator', () => { + assert.throws( + () => assertSafeQuery({ field: { $accumulator: { init: 'function() {}' } } }, errors), + (err) => { + assert.equal(err.message, 'MONGO_BLOCKED_OPERATOR') + assert.deepEqual(err.data, { operator: '$accumulator' }) + return true + } + ) + }) + + it('should throw MONGO_BLOCKED_OPERATOR for $function', () => { + assert.throws( + () => assertSafeQuery({ field: { $function: { body: 'function() {}' } } }, errors), + (err) => { + assert.equal(err.message, 'MONGO_BLOCKED_OPERATOR') + assert.deepEqual(err.data, { operator: '$function' }) + return true + } + ) + }) + + it('should reject blocked operators nested inside $or', () => { + assert.throws( + () => assertSafeQuery({ $or: [{ $where: 'true' }] }, errors), + (err) => { + assert.equal(err.message, 'MONGO_BLOCKED_OPERATOR') + return true + } + ) + }) + + it('should reject blocked operators deeply nested', () => { + assert.throws( + () => assertSafeQuery({ a: { b: { $where: 'true' } } }, errors), + (err) => { + assert.equal(err.message, 'MONGO_BLOCKED_OPERATOR') + return true + } + ) + }) + + it('should reject blocked operators inside $and within $or', () => { + assert.throws( + () => assertSafeQuery({ $or: [{ $and: [{ $where: 'true' }] }] }, errors), + (err) => { + assert.equal(err.message, 'MONGO_BLOCKED_OPERATOR') + return true + } + ) + }) + + it('should handle null values gracefully', () => { + assert.doesNotThrow(() => assertSafeQuery({ field: null }, errors)) + }) + + it('should handle undefined input gracefully', () => { + assert.doesNotThrow(() => assertSafeQuery(undefined, errors)) + }) + + it('should handle empty object', () => { + assert.doesNotThrow(() => assertSafeQuery({}, errors)) + }) + + it('should handle primitive values', () => { + assert.doesNotThrow(() => assertSafeQuery('string', errors)) + assert.doesNotThrow(() => assertSafeQuery(42, errors)) + assert.doesNotThrow(() => assertSafeQuery(true, errors)) + }) +}) diff --git a/tests/utils-processParams.spec.js b/tests/utils-processParams.spec.js new file mode 100644 index 0000000..53a0d34 --- /dev/null +++ b/tests/utils-processParams.spec.js @@ -0,0 +1,244 @@ +import { describe, it, mock } from 'node:test' +import assert from 'node:assert/strict' +import { ObjectId } from 'mongodb' +import App from 'adapt-authoring-core/lib/App.js' + +mock.getter(App, 'instance', () => ({ + errors: { + INVALID_OBJECTID: { + setData (data) { + const e = new Error('INVALID_OBJECTID') + e.data = data + return e + } + }, + MONGO_BLOCKED_OPERATOR: { + setData (data) { + const e = new Error('MONGO_BLOCKED_OPERATOR') + e.data = data + return e + } + } + } +})) + +const { processParams } = await import('../lib/utils/processParams.js') + +describe('processParams()', () => { + describe('query processing', () => { + it('should return a copy of the query', () => { + const query = { name: 'test' } + const result = processParams({ query }) + assert.notEqual(result.query, query) + assert.deepEqual(result.query, query) + }) + + it('should not mutate the original query', () => { + const idStr = new ObjectId().toString() + const query = { _id: idStr } + processParams({ query }) + assert.equal(query._id, idStr) + }) + + it('should convert ObjectIds in the query copy', () => { + const idStr = new ObjectId().toString() + const result = processParams({ query: { _id: idStr } }) + assert.ok(result.query._id instanceof ObjectId) + }) + + it('should reject unsafe query operators', () => { + assert.throws( + () => processParams({ query: { $where: 'true' } }), + { message: 'MONGO_BLOCKED_OPERATOR' } + ) + }) + }) + + describe('data processing', () => { + it('should return a copy of the data', () => { + const data = { name: 'test' } + const result = processParams({ data }) + assert.notEqual(result.data, data) + assert.deepEqual(result.data, { name: 'test' }) + }) + + it('should not mutate the original data', () => { + const data = { _id: new ObjectId(), name: 'test' } + const originalId = data._id + processParams({ data }) + assert.deepEqual(data._id, originalId) + }) + + it('should strip _id from the copy by default', () => { + const data = { _id: new ObjectId(), name: 'test' } + const result = processParams({ data }) + assert.equal(result.data._id, undefined) + }) + + it('should strip _id from $set by default', () => { + const data = { $set: { _id: new ObjectId(), name: 'test' } } + const result = processParams({ data }) + assert.equal(result.data.$set._id, undefined) + }) + + it('should preserve _id when preserveId option is set', () => { + const id = new ObjectId() + const result = processParams({ data: { _id: id, name: 'test' }, options: { preserveId: true } }) + assert.deepEqual(result.data._id, id) + }) + + it('should convert ObjectIds in the data copy', () => { + const idStr = new ObjectId().toString() + const result = processParams({ data: { ref: idStr } }) + assert.ok(result.data.ref instanceof ObjectId) + }) + }) + + describe('options processing', () => { + it('should return a copy of the options', () => { + const options = { limit: 10 } + const result = processParams({ options }) + assert.notEqual(result.options, options) + }) + + it('should not mutate the original options', () => { + const options = { limit: '10' } + processParams({ options }) + assert.equal(options.limit, '10') + }) + + it('should parse limit as integer', () => { + const result = processParams({ options: { limit: '25' } }) + assert.equal(result.options.limit, 25) + }) + + it('should parse skip as integer', () => { + const result = processParams({ options: { skip: '5' } }) + assert.equal(result.options.skip, 5) + }) + + it('should strip preserveId from the returned options', () => { + const result = processParams({ data: { name: 'test' }, options: { preserveId: true, limit: 10 } }) + assert.equal(result.options.preserveId, undefined) + }) + + it('should pass through allowed options unchanged', () => { + const result = processParams({ options: { sort: { name: 1 }, limit: 10 } }) + assert.deepEqual(result.options.sort, { name: 1 }) + }) + + it('should allow all safe driver options', () => { + const options = { + collation: { locale: 'en' }, + includeResultMetadata: false, + limit: 10, + projection: { name: 1 }, + returnCursor: true, + returnDocument: 'after', + skip: 5, + sort: { name: 1 }, + upsert: true + } + const result = processParams({ options }) + assert.deepEqual(result.options, { ...options, limit: 10, skip: 5 }) + }) + + it('should strip disallowed options', () => { + const result = processParams({ options: { limit: 10, allowDiskUse: true, bypassDocumentValidation: true } }) + assert.equal(result.options.limit, 10) + assert.equal(result.options.allowDiskUse, undefined) + assert.equal(result.options.bypassDocumentValidation, undefined) + }) + + it('should strip hint option', () => { + const result = processParams({ options: { hint: { _id: 1 } } }) + assert.equal(result.options.hint, undefined) + }) + + it('should strip writeConcern option', () => { + const result = processParams({ options: { writeConcern: { w: 0 } } }) + assert.equal(result.options.writeConcern, undefined) + }) + + it('should strip maxTimeMS option', () => { + const result = processParams({ options: { maxTimeMS: 1 } }) + assert.equal(result.options.maxTimeMS, undefined) + }) + }) + + describe('options value validation', () => { + it('should strip limit if not a valid number', () => { + const result = processParams({ options: { limit: 'abc' } }) + assert.equal(result.options.limit, undefined) + }) + + it('should strip skip if not a valid number', () => { + const result = processParams({ options: { skip: 'abc' } }) + assert.equal(result.options.skip, undefined) + }) + + it('should strip limit if NaN', () => { + const result = processParams({ options: { limit: NaN } }) + assert.equal(result.options.limit, undefined) + }) + + it('should strip collation if not a plain object', () => { + const result = processParams({ options: { collation: 'en' } }) + assert.equal(result.options.collation, undefined) + }) + + it('should strip collation if array', () => { + const result = processParams({ options: { collation: [{ locale: 'en' }] } }) + assert.equal(result.options.collation, undefined) + }) + + it('should strip sort if not a plain object', () => { + const result = processParams({ options: { sort: 'name' } }) + assert.equal(result.options.sort, undefined) + }) + + it('should strip projection if not a plain object', () => { + const result = processParams({ options: { projection: 'name' } }) + assert.equal(result.options.projection, undefined) + }) + + it('should strip returnDocument if not before or after', () => { + const result = processParams({ options: { returnDocument: 'always' } }) + assert.equal(result.options.returnDocument, undefined) + }) + + it('should allow returnDocument before', () => { + const result = processParams({ options: { returnDocument: 'before' } }) + assert.equal(result.options.returnDocument, 'before') + }) + + it('should strip upsert if not boolean', () => { + const result = processParams({ options: { upsert: 'true' } }) + assert.equal(result.options.upsert, undefined) + }) + + it('should strip includeResultMetadata if not boolean', () => { + const result = processParams({ options: { includeResultMetadata: 1 } }) + assert.equal(result.options.includeResultMetadata, undefined) + }) + + it('should strip returnCursor if not boolean', () => { + const result = processParams({ options: { returnCursor: 1 } }) + assert.equal(result.options.returnCursor, undefined) + }) + }) + + describe('partial params', () => { + it('should handle query only', () => { + const result = processParams({ query: { name: 'test' } }) + assert.deepEqual(result.query, { name: 'test' }) + assert.equal(result.data, undefined) + assert.equal(result.options, undefined) + }) + + it('should handle empty call', () => { + const result = processParams() + assert.deepEqual(result, {}) + }) + }) +})