feat(NODE-3920)!: validate options are not repeated in connection string (#3788)

Co-authored-by: Durran Jordan <[email protected]>

Author: nbbeeken

src/connection_string.ts

@@ -204,6 +204,9 @@ function getUIntFromOptions(name: string, value: unknown): number {
 }
 
 function* entriesFromString(value: string): Generator<[string, string]> {
+  if (value === '') {
+    return;
+  }
   const keyValuePairs = value.split(',');
   for (const keyValue of keyValuePairs) {
     const [key, value] = keyValue.split(/:(.*)/);
@@ -290,10 +293,18 @@ export function parseOptions(
   }
 
   for (const key of url.searchParams.keys()) {
-    const values = [...url.searchParams.getAll(key)];
+    const values = url.searchParams.getAll(key);
+
+    const isReadPreferenceTags = /readPreferenceTags/i.test(key);
+
+    if (!isReadPreferenceTags && values.length > 1) {
+      throw new MongoInvalidArgumentError(
+        `URI option "${key}" cannot appear more than once in the connection string`
+      );
+    }
 
-    if (values.includes('')) {
-      throw new MongoAPIError('URI cannot contain options with no value');
+    if (!isReadPreferenceTags && values.includes('')) {
+      throw new MongoAPIError(`URI option "${key}" cannot be specified with no value`);
     }
 
     if (!urlOptions.has(key)) {

test/unit/connection_string.spec.test.ts

@@ -10,8 +10,6 @@ const skipTests = [
 ];
 
 describe('Connection String spec tests', function () {
-  // TODO(NODE-3920): validate repeated options
-  const testsThatDoNotThrowOnWarn = ['Repeated option keys'];
   const suites = loadSpecTests('connection-string');
 
   for (const suite of suites) {
@@ -22,10 +20,7 @@ describe('Connection String spec tests', function () {
             return this.skip();
           }
 
-          executeUriValidationTest(
-            test,
-            testsThatDoNotThrowOnWarn.some(t => t === test.description)
-          );
+          executeUriValidationTest(test);
         });
       }
     });

test/unit/connection_string.test.ts

@@ -44,6 +44,13 @@ describe('Connection String', function () {
     });
   });
 
+  it('throws an error related to the option that was given an empty value', function () {
+    expect(() => parseOptions('mongodb://localhost?tls=', {})).to.throw(
+      MongoAPIError,
+      /tls" cannot/i
+    );
+  });
+
   it('should provide a default port if one is not provided', function () {
     const options = parseOptions('mongodb://hostname');
     expect(options.hosts[0].socketPath).to.be.undefined;
@@ -122,21 +129,18 @@ describe('Connection String', function () {
 
   describe('readPreferenceTags option', function () {
     context('when the option is passed in the uri', () => {
-      it('should throw an error if no value is passed for readPreferenceTags', () => {
-        expect(() => parseOptions('mongodb://hostname?readPreferenceTags=')).to.throw(
-          MongoAPIError
-        );
-      });
       it('should parse a single read preference tag', () => {
         const options = parseOptions('mongodb://hostname?readPreferenceTags=bar:foo');
         expect(options.readPreference.tags).to.deep.equal([{ bar: 'foo' }]);
       });
+
       it('should parse multiple readPreferenceTags', () => {
         const options = parseOptions(
           'mongodb://hostname?readPreferenceTags=bar:foo&readPreferenceTags=baz:bar'
         );
         expect(options.readPreference.tags).to.deep.equal([{ bar: 'foo' }, { baz: 'bar' }]);
       });
+
       it('should parse multiple readPreferenceTags for the same key', () => {
         const options = parseOptions(
           'mongodb://hostname?readPreferenceTags=bar:foo&readPreferenceTags=bar:banana&readPreferenceTags=baz:bar'
@@ -147,6 +151,19 @@ describe('Connection String', function () {
           { baz: 'bar' }
         ]);
       });
+
+      it('should parse multiple and empty readPreferenceTags', () => {
+        const options = parseOptions(
+          'mongodb://hostname?readPreferenceTags=bar:foo&readPreferenceTags=baz:bar&readPreferenceTags='
+        );
+        expect(options.readPreference.tags).to.deep.equal([{ bar: 'foo' }, { baz: 'bar' }, {}]);
+      });
+
+      it('will set "__proto__" as own property on readPreferenceTag', () => {
+        const options = parseOptions('mongodb://hostname?readPreferenceTags=__proto__:foo');
+        expect(options.readPreference.tags?.[0]).to.have.own.property('__proto__', 'foo');
+        expect(Object.getPrototypeOf(options.readPreference.tags?.[0])).to.be.null;
+      });
     });
 
     context('when the option is passed in the options object', () => {
@@ -156,12 +173,14 @@ describe('Connection String', function () {
         });
         expect(options.readPreference.tags).to.deep.equal([]);
       });
+
       it('should parse a single readPreferenceTags object', () => {
         const options = parseOptions('mongodb://hostname?', {
           readPreferenceTags: [{ bar: 'foo' }]
         });
         expect(options.readPreference.tags).to.deep.equal([{ bar: 'foo' }]);
       });
+
       it('should parse multiple readPreferenceTags', () => {
         const options = parseOptions('mongodb://hostname?', {
           readPreferenceTags: [{ bar: 'foo' }, { baz: 'bar' }]
@@ -493,6 +512,18 @@ describe('Connection String', function () {
       );
     });
 
+    it('throws an error for repeated options that can only appear once', function () {
+      // At the time of writing, readPreferenceTags is the only options that can be repeated
+      expect(() => parseOptions('mongodb://localhost/?compressors=zstd&compressors=zstd')).to.throw(
+        MongoInvalidArgumentError,
+        /cannot appear more than once/
+      );
+      expect(() => parseOptions('mongodb://localhost/?tls=true&tls=true')).to.throw(
+        MongoInvalidArgumentError,
+        /cannot appear more than once/
+      );
+    });
+
     it('should validate authMechanism', function () {
       expect(() => parseOptions('mongodb://localhost/?authMechanism=DOGS')).to.throw(
         MongoParseError,

test/unit/mongo_client.test.js

@@ -400,18 +400,6 @@ describe('MongoOptions', function () {
     });
   });
 
-  it('throws an error if multiple tls parameters are not all set to the same value', () => {
-    expect(() => parseOptions('mongodb://localhost?tls=true&tls=false')).to.throw(
-      'All values of tls/ssl must be the same.'
-    );
-  });
-
-  it('throws an error if multiple ssl parameters are not all set to the same value', () => {
-    expect(() => parseOptions('mongodb://localhost?ssl=true&ssl=false')).to.throw(
-      'All values of tls/ssl must be the same.'
-    );
-  });
-
   it('throws an error if tls and ssl parameters are not all set to the same value', () => {
     expect(() => parseOptions('mongodb://localhost?tls=true&ssl=false')).to.throw(
       'All values of tls/ssl must be the same.'
@@ -421,16 +409,6 @@ describe('MongoOptions', function () {
     );
   });
 
-  it('correctly sets tls if multiple tls parameters are all set to the same value', () => {
-    expect(parseOptions('mongodb://localhost?tls=true&tls=true')).to.have.property('tls', true);
-    expect(parseOptions('mongodb://localhost?tls=false&tls=false')).to.have.property('tls', false);
-  });
-
-  it('correctly sets tls if multiple ssl parameters are all set to the same value', () => {
-    expect(parseOptions('mongodb://localhost?ssl=true&ssl=true')).to.have.property('tls', true);
-    expect(parseOptions('mongodb://localhost?ssl=false&ssl=false')).to.have.property('tls', false);
-  });
-
   it('correctly sets tls if tls and ssl parameters are all set to the same value', () => {
     expect(parseOptions('mongodb://localhost?ssl=true&tls=true')).to.have.property('tls', true);
     expect(parseOptions('mongodb://localhost?ssl=false&tls=false')).to.have.property('tls', false);