From 98550c6ad5b7c268707d3e9e2367ca56f71298ad Mon Sep 17 00:00:00 2001 From: aditi-khare-mongoDB <106987683+aditi-khare-mongoDB@users.noreply.github.com> Date: Wed, 4 Oct 2023 15:23:54 -0400 Subject: [PATCH] fix(NODE-5496): remove client-side collection and database name check validation (#3873) --- src/cmap/command_monitoring_events.ts | 9 +- src/cmap/commands.ts | 23 +++-- src/cmap/connection.ts | 5 +- src/collection.ts | 3 - src/db.ts | 34 +++---- src/operations/rename.ts | 3 +- src/utils.ts | 38 -------- .../collection-management/collection.test.ts | 22 ++--- test/integration/node-specific/db.test.js | 57 +++++++----- .../node-specific/operation_examples.test.ts | 92 ------------------- .../cmap/command_monitoring_events.test.js | 39 +------- test/unit/cmap/message_stream.test.ts | 2 +- 12 files changed, 79 insertions(+), 248 deletions(-) diff --git a/src/cmap/command_monitoring_events.ts b/src/cmap/command_monitoring_events.ts index f709c20abe..ada41b4304 100644 --- a/src/cmap/command_monitoring_events.ts +++ b/src/cmap/command_monitoring_events.ts @@ -7,7 +7,7 @@ import { LEGACY_HELLO_COMMAND_CAMEL_CASE } from '../constants'; import { calculateDurationInMs, deepCopy } from '../utils'; -import { Msg, type WriteProtocolMessageType } from './commands'; +import { Msg, type Query, type WriteProtocolMessageType } from './commands'; import type { Connection } from './connection'; /** @@ -49,7 +49,7 @@ export class CommandStartedEvent { this.connectionId = connectionId; this.serviceId = serviceId; this.requestId = command.requestId; - this.databaseName = databaseName(command); + this.databaseName = command.databaseName; this.commandName = commandName; this.command = maybeRedact(commandName, cmd, cmd); } @@ -181,9 +181,8 @@ const HELLO_COMMANDS = new Set(['hello', LEGACY_HELLO_COMMAND, LEGACY_HELLO_COMM // helper methods const extractCommandName = (commandDoc: Document) => Object.keys(commandDoc)[0]; -const namespace = (command: WriteProtocolMessageType) => command.ns; -const databaseName = (command: WriteProtocolMessageType) => command.ns.split('.')[0]; -const collectionName = (command: WriteProtocolMessageType) => command.ns.split('.')[1]; +const namespace = (command: Query) => command.ns; +const collectionName = (command: Query) => command.ns.split('.')[1]; const maybeRedact = (commandName: string, commandDoc: Document, result: Error | Document) => SENSITIVE_COMMANDS.has(commandName) || (HELLO_COMMANDS.has(commandName) && commandDoc.speculativeAuthenticate) diff --git a/src/cmap/commands.ts b/src/cmap/commands.ts index 8c1d47b1cd..15ecbfc8d6 100644 --- a/src/cmap/commands.ts +++ b/src/cmap/commands.ts @@ -3,7 +3,6 @@ import * as BSON from '../bson'; import { MongoInvalidArgumentError, MongoRuntimeError } from '../error'; import { ReadPreference } from '../read_preference'; import type { ClientSession } from '../sessions'; -import { databaseNamespace } from '../utils'; import type { CommandOptions } from './connection'; import { OP_MSG, OP_QUERY } from './wire_protocol/constants'; @@ -55,7 +54,6 @@ export interface OpQueryOptions extends CommandOptions { /** @internal */ export class Query { ns: string; - query: Document; numberToSkip: number; numberToReturn: number; returnFieldSelector?: Document; @@ -75,10 +73,13 @@ export class Query { partial: boolean; documentsReturnedIn?: string; - constructor(ns: string, query: Document, options: OpQueryOptions) { + constructor(public databaseName: string, public query: Document, options: OpQueryOptions) { // Basic options needed to be passed in // TODO(NODE-3483): Replace with MongoCommandError - if (ns == null) throw new MongoRuntimeError('Namespace must be specified for query'); + const ns = `${databaseName}.$cmd`; + if (typeof databaseName !== 'string') { + throw new MongoRuntimeError('Database name must be a string for a query'); + } // TODO(NODE-3483): Replace with MongoCommandError if (query == null) throw new MongoRuntimeError('A query document must be specified for query'); @@ -90,7 +91,6 @@ export class Query { // Basic options this.ns = ns; - this.query = query; // Additional options this.numberToSkip = options.numberToSkip || 0; @@ -473,9 +473,6 @@ export interface OpMsgOptions { /** @internal */ export class Msg { - ns: string; - command: Document; - options: OpQueryOptions; requestId: number; serializeFunctions: boolean; ignoreUndefined: boolean; @@ -485,15 +482,17 @@ export class Msg { moreToCome: boolean; exhaustAllowed: boolean; - constructor(ns: string, command: Document, options: OpQueryOptions) { + constructor( + public databaseName: string, + public command: Document, + public options: OpQueryOptions + ) { // Basic options needed to be passed in if (command == null) throw new MongoInvalidArgumentError('Query document must be specified for query'); // Basic options - this.ns = ns; - this.command = command; - this.command.$db = databaseNamespace(ns); + this.command.$db = databaseName; if (options.readPreference && options.readPreference.mode !== ReadPreference.PRIMARY) { this.command.$readPreference = options.readPreference.toJSON(); diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index 0159db9d8a..67dc42ee7f 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -539,10 +539,9 @@ export class Connection extends TypedEventEmitter { options ); - const cmdNs = `${ns.db}.$cmd`; const message = shouldUseOpMsg - ? new Msg(cmdNs, cmd, commandOptions) - : new Query(cmdNs, cmd, commandOptions); + ? new Msg(ns.db, cmd, commandOptions) + : new Query(ns.db, cmd, commandOptions); try { write(this, message, commandOptions, callback); diff --git a/src/collection.ts b/src/collection.ts index 57ede865c9..4aaa69f776 100644 --- a/src/collection.ts +++ b/src/collection.ts @@ -90,7 +90,6 @@ import { import { ReadConcern, type ReadConcernLike } from './read_concern'; import { ReadPreference, type ReadPreferenceLike } from './read_preference'; import { - checkCollectionName, DEFAULT_PK_FACTORY, MongoDBCollectionNamespace, normalizeHintField, @@ -164,8 +163,6 @@ export class Collection { * @internal */ constructor(db: Db, name: string, options?: CollectionOptions) { - checkCollectionName(name); - // Internal state this.s = { db, diff --git a/src/db.ts b/src/db.ts index 3c710fbab0..c12c22c724 100644 --- a/src/db.ts +++ b/src/db.ts @@ -6,7 +6,7 @@ import * as CONSTANTS from './constants'; import { AggregationCursor } from './cursor/aggregation_cursor'; import { ListCollectionsCursor } from './cursor/list_collections_cursor'; import { RunCommandCursor, type RunCursorCommandOptions } from './cursor/run_command_cursor'; -import { MongoAPIError, MongoInvalidArgumentError } from './error'; +import { MongoInvalidArgumentError } from './error'; import type { MongoClient, PkFactory } from './mongo_client'; import type { TODO_NODE_3286 } from './mongo_types'; import type { AggregateOptions } from './operations/aggregate'; @@ -134,11 +134,13 @@ export class Db { public static SYSTEM_JS_COLLECTION = CONSTANTS.SYSTEM_JS_COLLECTION; /** - * Creates a new Db instance + * Creates a new Db instance. + * + * Db name cannot contain a dot, the server may apply more restrictions when an operation is run. * * @param client - The MongoClient for the database. * @param databaseName - The name of the database this instance represents. - * @param options - Optional settings for Db construction + * @param options - Optional settings for Db construction. */ constructor(client: MongoClient, databaseName: string, options?: DbOptions) { options = options ?? {}; @@ -146,8 +148,10 @@ export class Db { // Filter the options options = filterOptions(options, DB_OPTIONS_ALLOW_LIST); - // Ensure we have a valid db name - validateDatabaseName(databaseName); + // Ensure there are no dots in database name + if (typeof databaseName === 'string' && databaseName.includes('.')) { + throw new MongoInvalidArgumentError(`Database names cannot contain the character '.'`); + } // Internal state of the db object this.s = { @@ -218,6 +222,8 @@ export class Db { * Create a new collection on a server with the specified options. Use this to create capped collections. * More information about command options available at https://www.mongodb.com/docs/manual/reference/command/create/ * + * Collection namespace validation is performed server-side. + * * @param name - The name of the collection to create * @param options - Optional settings for the command */ @@ -294,6 +300,8 @@ export class Db { /** * Returns a reference to a MongoDB Collection. If it does not exist it will be created implicitly. * + * Collection namespace validation is performed server-side. + * * @param name - the collection name we wish to access. * @returns return the new Collection instance */ @@ -519,19 +527,3 @@ export class Db { return new RunCommandCursor(this, command, options); } } - -// TODO(NODE-3484): Refactor into MongoDBNamespace -// Validate the database name -function validateDatabaseName(databaseName: string) { - if (typeof databaseName !== 'string') - throw new MongoInvalidArgumentError('Database name must be a string'); - if (databaseName.length === 0) - throw new MongoInvalidArgumentError('Database name cannot be the empty string'); - if (databaseName === '$external') return; - - const invalidChars = [' ', '.', '$', '/', '\\']; - for (let i = 0; i < invalidChars.length; i++) { - if (databaseName.indexOf(invalidChars[i]) !== -1) - throw new MongoAPIError(`database names cannot contain the character '${invalidChars[i]}'`); - } -} diff --git a/src/operations/rename.ts b/src/operations/rename.ts index e370259899..ec5ee7c8af 100644 --- a/src/operations/rename.ts +++ b/src/operations/rename.ts @@ -2,7 +2,7 @@ import type { Document } from '../bson'; import { Collection } from '../collection'; import type { Server } from '../sdam/server'; import type { ClientSession } from '../sessions'; -import { checkCollectionName, MongoDBNamespace } from '../utils'; +import { MongoDBNamespace } from '../utils'; import { CommandOperation, type CommandOperationOptions } from './command'; import { Aspect, defineAspects } from './operation'; @@ -21,7 +21,6 @@ export class RenameOperation extends CommandOperation { public newName: string, public override options: RenameOptions ) { - checkCollectionName(newName); super(collection, options); this.ns = new MongoDBNamespace('admin', '$cmd'); } diff --git a/src/utils.ts b/src/utils.ts index 436c2049da..be215ff880 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -78,39 +78,6 @@ export function hostMatchesWildcards(host: string, wildcards: string[]): boolean return false; } -/** - * Throws if collectionName is not a valid mongodb collection namespace. - * @internal - */ -export function checkCollectionName(collectionName: string): void { - if ('string' !== typeof collectionName) { - throw new MongoInvalidArgumentError('Collection name must be a String'); - } - - if (!collectionName || collectionName.indexOf('..') !== -1) { - throw new MongoInvalidArgumentError('Collection names cannot be empty'); - } - - if ( - collectionName.indexOf('$') !== -1 && - collectionName.match(/((^\$cmd)|(oplog\.\$main))/) == null - ) { - // TODO(NODE-3483): Use MongoNamespace static method - throw new MongoInvalidArgumentError("Collection names must not contain '$'"); - } - - if (collectionName.match(/^\.|\.$/) != null) { - // TODO(NODE-3483): Use MongoNamespace static method - throw new MongoInvalidArgumentError("Collection names must not start or end with '.'"); - } - - // Validate that we are not passing 0x00 in the collection name - if (collectionName.indexOf('\x00') !== -1) { - // TODO(NODE-3483): Use MongoNamespace static method - throw new MongoInvalidArgumentError('Collection names cannot contain a null character'); - } -} - /** * Ensure Hint field is in a shape we expect: * - object of index names mapping to 1 or -1 @@ -386,11 +353,6 @@ export function maybeCallback( return; } -/** @internal */ -export function databaseNamespace(ns: string): string { - return ns.split('.')[0]; -} - /** * Synchronously Generate a UUIDv4 * @internal diff --git a/test/integration/collection-management/collection.test.ts b/test/integration/collection-management/collection.test.ts index 0a9d240110..3dcbdeeacc 100644 --- a/test/integration/collection-management/collection.test.ts +++ b/test/integration/collection-management/collection.test.ts @@ -1,6 +1,6 @@ import { expect } from 'chai'; -import { Collection, type Db, isHello, type MongoClient } from '../../mongodb'; +import { Collection, type Db, isHello, type MongoClient, MongoServerError } from '../../mongodb'; import * as mock from '../../tools/mongodb-mock/index'; import { setupDatabase } from '../shared'; @@ -95,18 +95,14 @@ describe('Collection', function () { ]); }); - it('should fail due to illegal listCollections', function (done) { - expect(() => db.collection(5)).to.throw('Collection name must be a String'); - expect(() => db.collection('')).to.throw('Collection names cannot be empty'); - expect(() => db.collection('te$t')).to.throw("Collection names must not contain '$'"); - expect(() => db.collection('.test')).to.throw( - "Collection names must not start or end with '.'" - ); - expect(() => db.collection('test.')).to.throw( - "Collection names must not start or end with '.'" - ); - expect(() => db.collection('test..t')).to.throw('Collection names cannot be empty'); - done(); + it('fails on server due to invalid namespace', async function () { + const error = await db + .collection('a\x00b') + .insertOne({ a: 1 }) + .catch(error => error); + expect(error).to.be.instanceOf(MongoServerError); + expect(error).to.have.property('code', 73); + expect(error).to.have.property('codeName', 'InvalidNamespace'); }); it('should correctly count on non-existent collection', function (done) { diff --git a/test/integration/node-specific/db.test.js b/test/integration/node-specific/db.test.js index f5102110fc..70144ca5a8 100644 --- a/test/integration/node-specific/db.test.js +++ b/test/integration/node-specific/db.test.js @@ -2,36 +2,47 @@ const { setupDatabase, assert: test } = require(`../shared`); const { expect } = require('chai'); -const { Db, MongoClient } = require('../../mongodb'); +const { MongoClient, MongoInvalidArgumentError, MongoServerError } = require('../../mongodb'); describe('Db', function () { before(function () { return setupDatabase(this.configuration); }); - it('shouldCorrectlyHandleIllegalDbNames', { - metadata: { - requires: { topology: ['single', 'replicaset', 'sharded'] } - }, + context('when given illegal db name', function () { + let client; + let db; + + beforeEach(function () { + client = this.configuration.newClient(); + }); + + afterEach(async function () { + db = undefined; + await client.close(); + }); + + context('of type string, containing no dot characters', function () { + it('should throw error on server only', async function () { + db = client.db('a\x00b'); + const error = await db.createCollection('spider').catch(error => error); + expect(error).to.be.instanceOf(MongoServerError); + expect(error).to.have.property('code', 73); + expect(error).to.have.property('codeName', 'InvalidNamespace'); + }); + }); - test: done => { - const client = { bsonOptions: {} }; - expect(() => new Db(client, 5)).to.throw('Database name must be a string'); - expect(() => new Db(client, '')).to.throw('Database name cannot be the empty string'); - expect(() => new Db(client, 'te$t')).to.throw( - "database names cannot contain the character '$'" - ); - expect(() => new Db(client, '.test', function () {})).to.throw( - "database names cannot contain the character '.'" - ); - expect(() => new Db(client, '\\test', function () {})).to.throw( - "database names cannot contain the character '\\'" - ); - expect(() => new Db(client, 'test test', function () {})).to.throw( - "database names cannot contain the character ' '" - ); - done(); - } + context('of type string, containing a dot character', function () { + it('should throw MongoInvalidArgumentError', function () { + expect(() => client.db('a.b')).to.throw(MongoInvalidArgumentError); + }); + }); + + context('of type non-string type', function () { + it('should not throw client-side', function () { + expect(() => client.db(5)).to.not.throw(); + }); + }); }); it('shouldCorrectlyHandleFailedConnection', { diff --git a/test/integration/node-specific/operation_examples.test.ts b/test/integration/node-specific/operation_examples.test.ts index 179e7104a3..b21cd68213 100644 --- a/test/integration/node-specific/operation_examples.test.ts +++ b/test/integration/node-specific/operation_examples.test.ts @@ -1650,98 +1650,6 @@ describe('Operations', function () { } }); - /** - * An example of illegal and legal renaming of a collection using a Promise. - * - * example-class Collection - * example-method rename - */ - it('shouldCorrectlyRenameCollectionWithPromises', { - metadata: { - requires: { topology: ['single'] } - }, - - test: async function () { - const configuration = this.configuration; - const client: MongoClient = configuration.newClient(configuration.writeConcernMax(), { - maxPoolSize: 1 - }); - // LINE var MongoClient = require('mongodb').MongoClient, - // LINE test = require('assert'); - // LINE const client = new MongoClient('mongodb://localhost:27017/test'); - // LINE client.connect().then(() => { - // LINE var db = client.db('test); - // REPLACE configuration.writeConcernMax() WITH {w:1} - // REMOVE-LINE done(); - // BEGIN - // Open a couple of collections - - await client.connect(); - this.defer(async () => await client.close()); - const db = client.db(configuration.db); - /* es-lint-disable */ - let [collection1] = await Promise.all([ - db.createCollection('test_rename_collection_with_promise'), - db.createCollection('test_rename_collection2_with_promise') - ]); - - // Attempt to rename a collection to a number - // @ts-expect-error need to test that it will fail on number inputs - let err = await collection1.rename(5).catch(e => e); - expect(err).to.be.instanceOf(Error); - expect(err).to.have.property('message', 'Collection name must be a String'); - - // Attempt to rename a collection to an empty string - err = await collection1.rename('').catch(e => e); - expect(err).to.be.instanceOf(Error); - expect(err).to.have.property('message', 'Collection names cannot be empty'); - - // Attemp to rename a collection to an illegal name including the character $ - err = await collection1.rename('te$t').catch(e => e); - expect(err).to.be.instanceOf(Error); - expect(err).to.have.property('message', "Collection names must not contain '$'"); - - // Attempt to rename a collection to an illegal name starting with the character . - err = await collection1.rename('.test').catch(e => e); - expect(err).to.be.instanceOf(Error); - expect(err).to.have.property('message', "Collection names must not start or end with '.'"); - // Attempt to rename a collection to an illegal name ending with the character . - err = await collection1.rename('test.').catch(e => e); - expect(err).to.be.instanceOf(Error); - expect(err).to.have.property('message', "Collection names must not start or end with '.'"); - - // Attempt to rename a collection to an illegal name with an empty middle name - err = await collection1.rename('tes..t').catch(e => e); - expect(err).to.be.instanceOf(Error); - expect(err).to.have.property('message', 'Collection names cannot be empty'); - - // Attempt to rename a collection to an illegal name with an empty middle name - err = await collection1.rename('tes..t').catch(e => e); - expect(err).to.be.instanceOf(Error); - expect(err).to.have.property('message', 'Collection names cannot be empty'); - - // Insert a couple of documents - await collection1.insertMany([{ x: 1 }, { x: 2 }], configuration.writeConcernMax()); - - // Attempt to rename the first collection to the second one, this will fail - err = await collection1.rename('test_rename_collection2_with_promise').catch(e => e); - expect(err).to.be.instanceOf(Error); - expect(err.message).to.have.length.gt(0); - - // Attempt to rename the first collection to a name that does not exist - // this will be successful - collection1 = await collection1.rename('test_rename_collection3_with_promise'); - - // Ensure that the collection is pointing to the new one - expect(collection1.collectionName).to.equal('test_rename_collection3_with_promise'); - - expect(await collection1.count()).equals(2); - - /* es-lint-enable */ - // END - } - }); - /** * Example of a simple document update with safe set to false on an existing document using a Promise. * diff --git a/test/unit/cmap/command_monitoring_events.test.js b/test/unit/cmap/command_monitoring_events.test.js index 0f4334d530..7790d1e56b 100644 --- a/test/unit/cmap/command_monitoring_events.test.js +++ b/test/unit/cmap/command_monitoring_events.test.js @@ -6,9 +6,9 @@ const { expect } = require('chai'); describe('Command Monitoring Events - unit/cmap', function () { const commands = [ - new Query('admin.$cmd', { a: { b: 10 }, $query: { b: 10 } }, {}), + new Query('admin', { a: { b: 10 }, $query: { b: 10 } }, {}), new Query('hello', { a: { b: 10 }, $query: { b: 10 } }, {}), - new Msg('admin.$cmd', { b: { c: 20 } }, {}), + new Msg('admin', { b: { c: 20 } }, {}), new Msg('hello', { b: { c: 20 } }, {}), { ns: 'admin.$cmd', query: { $query: { a: 16 } } }, { ns: 'hello there', f1: { h: { a: 52, b: { c: 10, d: [1, 2, 3, 5] } } } } @@ -48,9 +48,8 @@ describe('Command Monitoring Events - unit/cmap', function () { it('should wrap a basic query option', function () { const db = 'test1'; - const coll = 'testingQuery'; const query = new Query( - `${db}.${coll}`, + `${db}`, { testCmd: 1, fizz: 'buzz', @@ -69,9 +68,8 @@ describe('Command Monitoring Events - unit/cmap', function () { it('should upconvert a Query wrapping a command into the corresponding command', function () { const db = 'admin'; - const coll = '$cmd'; const query = new Query( - `${db}.${coll}`, + `${db}`, { $query: { testCmd: 1, @@ -92,34 +90,5 @@ describe('Command Monitoring Events - unit/cmap', function () { expect(startEvent).to.have.property('connectionId').that.is.a('string'); expect(startEvent).to.have.property('command').that.deep.equals(query.query.$query); }); - - it('should upconvert a Query wrapping a query into a find command', function () { - const db = 'test5'; - const coll = 'testingFindCommand'; - const query = new Query( - `${db}.${coll}`, - { - $query: { - testCmd: 1, - fizz: 'buzz', - star: 'trek' - } - }, - {} - ); - - const startEvent = new CommandStartedEvent(conn, query); - - expect(startEvent).to.have.property('commandName', 'find'); - expect(startEvent).to.have.property('databaseName', db); - expect(startEvent).to.have.property('requestId', query.requestId); - expect(startEvent).to.have.property('connectionId').that.is.a('string'); - expect(startEvent).to.have.property('command').that.deep.equals({ - find: coll, - filter: query.query.$query, - batchSize: 0, - skip: 0 - }); - }); }); }); diff --git a/test/unit/cmap/message_stream.test.ts b/test/unit/cmap/message_stream.test.ts index 80104a105c..c6bc8f1660 100644 --- a/test/unit/cmap/message_stream.test.ts +++ b/test/unit/cmap/message_stream.test.ts @@ -139,7 +139,7 @@ describe('MessageStream', function () { const messageStream = new MessageStream(); messageStream.pipe(writeableStream); - const command = new Msg('admin.$cmd', { [LEGACY_HELLO_COMMAND]: 1 }, { requestId: 3 }); + const command = new Msg('admin', { [LEGACY_HELLO_COMMAND]: 1 }, { requestId: 3 }); messageStream.writeCommand(command, { started: 0, command: true,