From 5560e2ba2e4d963a1a68da7c2262cb55c83c6c02 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 28 Jul 2023 17:03:06 -0400 Subject: [PATCH 1/2] feat(NODE-3920)!: validate options are not repeated in connection string --- src/connection_string.ts | 15 +++++++++-- test/unit/connection_string.spec.test.ts | 6 +---- test/unit/connection_string.test.ts | 34 ++++++++++++++++++++---- test/unit/mongo_client.test.js | 22 --------------- 4 files changed, 43 insertions(+), 34 deletions(-) diff --git a/src/connection_string.ts b/src/connection_string.ts index b71ec691494..b86477881fa 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -205,6 +205,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(/:(.*)/); @@ -291,9 +294,17 @@ 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('')) { + if (!isReadPreferenceTags && values.includes('')) { throw new MongoAPIError('URI cannot contain options with no value'); } diff --git a/test/unit/connection_string.spec.test.ts b/test/unit/connection_string.spec.test.ts index eecf8041679..30c0b99fbe3 100644 --- a/test/unit/connection_string.spec.test.ts +++ b/test/unit/connection_string.spec.test.ts @@ -11,7 +11,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 +21,7 @@ describe('Connection String spec tests', function () { return this.skip(); } - executeUriValidationTest( - test, - testsThatDoNotThrowOnWarn.some(t => t === test.description) - ); + executeUriValidationTest(test); }); } }); diff --git a/test/unit/connection_string.test.ts b/test/unit/connection_string.test.ts index 9659fe04a26..ceb2432f2f2 100644 --- a/test/unit/connection_string.test.ts +++ b/test/unit/connection_string.test.ts @@ -122,21 +122,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 +144,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 +166,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 +505,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, diff --git a/test/unit/mongo_client.test.js b/test/unit/mongo_client.test.js index c2199250f1f..10edaaff381 100644 --- a/test/unit/mongo_client.test.js +++ b/test/unit/mongo_client.test.js @@ -401,18 +401,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.' @@ -422,16 +410,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); From 234216b1da4e1e4831abab11152e6fc1d828a0fd Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 31 Jul 2023 09:59:28 -0400 Subject: [PATCH 2/2] feat: add option name to error --- src/connection_string.ts | 2 +- test/unit/connection_string.spec.test.ts | 1 - test/unit/connection_string.test.ts | 7 +++++++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/connection_string.ts b/src/connection_string.ts index b86477881fa..88b2bb6f9d5 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -305,7 +305,7 @@ export function parseOptions( } if (!isReadPreferenceTags && values.includes('')) { - throw new MongoAPIError('URI cannot contain options with no value'); + throw new MongoAPIError(`URI option "${key}" cannot be specified with no value`); } if (!urlOptions.has(key)) { diff --git a/test/unit/connection_string.spec.test.ts b/test/unit/connection_string.spec.test.ts index 30c0b99fbe3..91aec183374 100644 --- a/test/unit/connection_string.spec.test.ts +++ b/test/unit/connection_string.spec.test.ts @@ -10,7 +10,6 @@ const skipTests = [ ]; describe('Connection String spec tests', function () { - // TODO(NODE-3920): validate repeated options const suites = loadSpecTests('connection-string'); for (const suite of suites) { diff --git a/test/unit/connection_string.test.ts b/test/unit/connection_string.test.ts index ceb2432f2f2..0439d49984c 100644 --- a/test/unit/connection_string.test.ts +++ b/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;