Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(NODE-5496): remove client-side collection and database name check validation #3873

Merged
merged 15 commits into from
Oct 4, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
24 changes: 4 additions & 20 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 @@ -138,6 +138,7 @@ export class Db {
*
* @param client - The MongoClient for the database.
* @param databaseName - The name of the database this instance represents.
* databaseName validation occurs at operation time
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
* @param options - Optional settings for Db construction
*/
constructor(client: MongoClient, databaseName: string, options?: DbOptions) {
Expand All @@ -146,9 +147,6 @@ export class Db {
// Filter the options
options = filterOptions(options, DB_OPTIONS_ALLOW_LIST);

// Ensure we have a valid db name
validateDatabaseName(databaseName);
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved

// Internal state of the db object
this.s = {
// Options
Expand Down Expand Up @@ -218,6 +216,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 occurs at operation time
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
*
* @param name - The name of the collection to create
* @param options - Optional settings for the command
*/
Expand Down Expand Up @@ -519,19 +519,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]}'`);
}
}
1 change: 1 addition & 0 deletions src/mongo_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,7 @@ const kOptions = Symbol('options');
* client.db().collection('pets');
* await client.insertOne({ name: 'spot', kind: 'dog' });
* ```
* // Db namespace validation occurs at operation time
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
*/
export class MongoClient extends TypedEventEmitter<MongoClientEvents> {
/** @internal */
Expand Down
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
33 changes: 0 additions & 33 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
20 changes: 8 additions & 12 deletions test/integration/collection-management/collection.test.ts
Original file line number Diff line number Diff line change
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['name']).to.equal('MongoServerError');
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
expect(error['code']).to.equal(73);
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
expect(error['codeName']).to.equal('InvalidNamespace');
});

it('should correctly count on non-existent collection', function (done) {
Expand Down
45 changes: 21 additions & 24 deletions test/integration/node-specific/db.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,36 +2,33 @@

const { setupDatabase, assert: test } = require(`../shared`);
const { expect } = require('chai');
const { Db, MongoClient } = require('../../mongodb');
const { MongoClient } = require('../../mongodb');

describe('Db', function () {
before(function () {
return setupDatabase(this.configuration);
});

it('shouldCorrectlyHandleIllegalDbNames', {
metadata: {
requires: { topology: ['single', 'replicaset', 'sharded'] }
},

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('when given illegal db name', function () {
let client;
let db;

beforeEach(function () {
client = this.configuration.newClient();
});

afterEach(async function () {
db = undefined;
await client.close();
});

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['name']).to.equal('MongoServerError');
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
expect(error['code']).to.equal(73);
expect(error['codeName']).to.equal('InvalidNamespace');
});
});

it('shouldCorrectlyHandleFailedConnection', {
Expand Down
92 changes: 0 additions & 92 deletions test/integration/node-specific/operation_examples.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', {
W-A-James marked this conversation as resolved.
Show resolved Hide resolved
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.
*
Expand Down