Fix: Harden MongoDB query and options handling (fixes #58)

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"

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)

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'

lib/utils/assertSafeQuery.js

@@ -0,0 +1,31 @@
+/**
+ * Operators that allow arbitrary JavaScript execution on the MongoDB server.
+ * @type {Set<string>}
+ */
+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)
+  }
+}

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<string, function>}
+ */
+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
+}

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()