Skip to content

Commit

Permalink
fix(NODE-5496): remove client-side collection and database name check…
Browse files Browse the repository at this point in the history
… validation (#3873)
  • Loading branch information
aditi-khare-mongoDB authored Oct 4, 2023
1 parent 33933ba commit 98550c6
Show file tree
Hide file tree
Showing 12 changed files with 79 additions and 248 deletions.
9 changes: 4 additions & 5 deletions src/cmap/command_monitoring_events.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

/**
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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)
Expand Down
23 changes: 11 additions & 12 deletions src/cmap/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -55,7 +54,6 @@ export interface OpQueryOptions extends CommandOptions {
/** @internal */
export class Query {
ns: string;
query: Document;
numberToSkip: number;
numberToReturn: number;
returnFieldSelector?: Document;
Expand All @@ -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');

Expand All @@ -90,7 +91,6 @@ export class Query {

// Basic options
this.ns = ns;
this.query = query;

// Additional options
this.numberToSkip = options.numberToSkip || 0;
Expand Down Expand Up @@ -473,9 +473,6 @@ export interface OpMsgOptions {

/** @internal */
export class Msg {
ns: string;
command: Document;
options: OpQueryOptions;
requestId: number;
serializeFunctions: boolean;
ignoreUndefined: boolean;
Expand All @@ -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();
Expand Down
5 changes: 2 additions & 3 deletions src/cmap/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -539,10 +539,9 @@ export class Connection extends TypedEventEmitter<ConnectionEvents> {
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);
Expand Down
3 changes: 0 additions & 3 deletions src/collection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -164,8 +163,6 @@ export class Collection<TSchema extends Document = Document> {
* @internal
*/
constructor(db: Db, name: string, options?: CollectionOptions) {
checkCollectionName(name);

// Internal state
this.s = {
db,
Expand Down
34 changes: 13 additions & 21 deletions src/db.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -134,20 +134,24 @@ 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 ?? {};

// 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 = {
Expand Down Expand Up @@ -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
*/
Expand Down Expand Up @@ -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
*/
Expand Down Expand Up @@ -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]}'`);
}
}
3 changes: 1 addition & 2 deletions src/operations/rename.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -21,7 +21,6 @@ export class RenameOperation extends CommandOperation<Document> {
public newName: string,
public override options: RenameOptions
) {
checkCollectionName(newName);
super(collection, options);
this.ns = new MongoDBNamespace('admin', '$cmd');
}
Expand Down
38 changes: 0 additions & 38 deletions src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -386,11 +353,6 @@ export function maybeCallback<T>(
return;
}

/** @internal */
export function databaseNamespace(ns: string): string {
return ns.split('.')[0];
}

/**
* Synchronously Generate a UUIDv4
* @internal
Expand Down
22 changes: 9 additions & 13 deletions test/integration/collection-management/collection.test.ts
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -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) {
Expand Down
57 changes: 34 additions & 23 deletions test/integration/node-specific/db.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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', {
Expand Down
Loading

0 comments on commit 98550c6

Please sign in to comment.