feat(NODE-6289): allow valid srv hostnames with less than 3 parts #4197

aditi-khare-mongoDB · 2024-08-15T15:49:15Z

Description

Downstream changes for DRIVERS-2922 (PR).

What is changing?

Remove logic asserting that SRV URIs need 3 parts
When we check if a returned address matches its parent SRV, an SRV with <3 parts must assert that the returned address contains an additional domain level
Add in prose tests.

Is there new documentation needed for these changes?

No

What is the motivation for this change?

Do not throw an error on valid URI formats pre-DNS resolution, and require stricter domain matching post-DNS resolution.

Release Highlight

Allow SRV hostnames with less than three `.` separated parts

The client now accepts SRV hostname strings with one or two . separated parts.

For example, the following code no longer throws an error.

await new MongoClient('mongodb+srv://localhost').connect();

For security reasons, the returned addresses of SRV strings with less than three parts must end with the entire SRV hostname and contain at least one additional domain level. This is because this added validation ensures that the returned address(es) are from a known host. In future releases, we plan on extending this validation to SRV strings with three or more parts, as well.

// Example 1: Validation fails since the returned address doesn't end with the entire SRV hostname
'mongodb+srv://mySite.com' => 'myEvilSite.com' 

// Example 2: Validation fails since the returned address is identical to the SRV hostname
'mongodb+srv://mySite.com' => 'mySite.com' 

// Example 3: Validation passes since the returned address ends with the entire SRV hostname and contains an additional domain level
'mongodb+srv://mySite.com' => 'cluster_1.mySite.com'

Double check the following

Ran npm run check:lint script
Self-review completed using the steps outlined here
PR title follows the correct format: type(NODE-xxxx)[!]: description
- Example: feat(NODE-1234)!: rewriting everything in coffeescript
Changes are covered by tests
New TODOs have a related JIRA ticket

W-A-James · 2024-09-19T19:40:25Z

test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.prose.test.ts

+
+describe(
+  'Initial DNS Seedlist Discovery (Prose Tests)',
+  { requires: { topology: 'single' } },


The metadata doesn't actually apply to context or describe blocks, only to it blocks, if you check this against a replica set, it still runs.

Also, why do we only want these tests to run against single topologies?

Not sure why I originally made it only single topologies, if it's running and working for all I'll remove this line.

W-A-James · 2024-09-19T19:47:01Z

test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.prose.test.ts

+        client.close();
+      });
+
+      it('do not error on an SRV because it has two domain levels', async function () {


Suggested change

it('do not error on an SRV because it has two domain levels', async function () {

it('does not error on an SRV because it has two domain levels', async function () {

Made the change!

W-A-James · 2024-09-19T19:47:08Z

test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.prose.test.ts

+        sinon.restore();
+      });
+
+      it('do not error on an SRV because it has one domain level', async function () {


Suggested change

it('do not error on an SRV because it has one domain level', async function () {

it('does not error on an SRV because it has one domain level', async function () {

W-A-James · 2024-09-19T19:52:11Z

test/unit/utils.test.ts

      });
    });

    context('when SRV host ends with a dot', () => {
      it('accepts address if it ends with a dot', () => {
-        expect(matchesParentDomain(exampleHostNamesWithDot, exampleSrvNameWithDot)).to.be.true;
+        expect(() => checkParentDomainMatch(exampleHostNamesWithDot, exampleSrvNameWithDot)).to.not
+          .throw;


throw is one of the few chai clauses that always needs to be called

Suggested change

.throw;

.throw();

W-A-James · 2024-09-19T19:54:06Z

test/unit/utils.test.ts

+        expect(() => checkParentDomainMatch(exampleHostNameWithoutDot, exampleSrvName)).to.not
+          .throw;
      });


Suggested change

expect(() => checkParentDomainMatch(exampleHostNameWithoutDot, exampleSrvName)).to.not

.throw;

});

expect(() => checkParentDomainMatch(exampleHostNameWithoutDot, exampleSrvName)).to.not

.throw();

});

W-A-James · 2024-09-19T19:54:19Z

test/unit/utils.test.ts

      });
    });

    context('when addresses in SRV record end without dots', () => {
      it('accepts address since it matches the parent domain', () => {
-        expect(matchesParentDomain(exampleHostNamesWithDot, exampleSrvName)).to.be.true;
+        expect(() => checkParentDomainMatch(exampleHostNamesWithDot, exampleSrvName)).to.not.throw;


Suggested change

expect(() => checkParentDomainMatch(exampleHostNamesWithDot, exampleSrvName)).to.not.throw;

expect(() => checkParentDomainMatch(exampleHostNamesWithDot, exampleSrvName)).to.not.throw();

W-A-James · 2024-09-19T19:54:35Z

test/unit/utils.test.ts

      });
    });

    context('when addresses in SRV record end with a dot', () => {
      it('accepts address since it is considered to still match the parent domain', () => {
-        expect(matchesParentDomain(exampleHostNamesWithDot, exampleSrvName)).to.be.true;
+        expect(() => checkParentDomainMatch(exampleHostNamesWithDot, exampleSrvName)).to.not.throw;


Suggested change

expect(() => checkParentDomainMatch(exampleHostNamesWithDot, exampleSrvName)).to.not.throw;

expect(() => checkParentDomainMatch(exampleHostNamesWithDot, exampleSrvName)).to.not.throw();

W-A-James · 2024-09-19T20:03:17Z

test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.prose.test.ts

+    });
+
+    context(
+      'When given a host from DNS resolution that does NOT end with the original SRVs domain name',


Can we add the number of the prose test. Makes it quicker to map from our test implementation to the test in the spec

Yep, added them in!

W-A-James · 2024-09-20T18:59:28Z

test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.prose.test.ts

+import { topologyWithPlaceholderClient } from '../../tools/utils';
+
+describe('Initial DNS Seedlist Discovery (Prose Tests)', () => {
+  context('1) When running validation on an SRV string before DNS resolution', function () {


Suggested change

context('1) When running validation on an SRV string before DNS resolution', function () {

context('1. When running validation on an SRV string before DNS resolution', function () {

W-A-James · 2024-09-20T18:59:43Z

test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.prose.test.ts

+  });
+
+  context(
+    '2) When given a host from DNS resolution that does NOT end with the original SRVs domain name',


Suggested change

'2) When given a host from DNS resolution that does NOT end with the original SRVs domain name',

'2. When given a host from DNS resolution that does NOT end with the original SRVs domain name',

W-A-James · 2024-09-20T19:00:02Z

test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.prose.test.ts

+  );
+
+  context(
+    '3) When given a host from DNS resolution that is identical to the original SRVs hostname',


Suggested change

'3) When given a host from DNS resolution that is identical to the original SRVs hostname',

'3. When given a host from DNS resolution that is identical to the original SRVs hostname',

W-A-James · 2024-09-20T19:01:21Z

test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.prose.test.ts

+
+    it('does not error on an SRV because it has one domain level', async function () {
+      const client = await this.configuration.newClient('mongodb+srv://localhost', {});
+      client.connect();


be sure to await client.connect() and client.close()

Suggested change

client.connect();

await client.connect();

W-A-James · 2024-09-20T19:02:41Z

test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.prose.test.ts

+    it('does not error on an SRV because it has one domain level', async function () {
+      const client = await this.configuration.newClient('mongodb+srv://localhost', {});
+      client.connect();
+      client.close();


For these tests, I'd move the client.close() call to an afterEach hook so that even if the connect fails, we clean up any resources created.

drivers-2922 downstream changes first pass

73ca011

aditi-khare-mongoDB mentioned this pull request Aug 15, 2024

DRIVERS-2922: Allow valid SRV hostnames with less than 3 parts mongodb/specifications#1628

Open

4 tasks

aditi-khare-mongoDB changed the title ~~feat(NODE-6289): DRIVERS-2922 Downstream Changes PoC~~ feat(NODE-6289): DRIVERS 2922 Downstream Changes PoC Aug 15, 2024

aditi-khare-mongoDB added 2 commits August 19, 2024 16:25

temp commit

ad151f7

added more prose tests

15dc8ee

aditi-khare-mongoDB changed the title ~~feat(NODE-6289): DRIVERS 2922 Downstream Changes PoC~~ feat(NODE-6289): Allow valid SRV hostnames with less than 3 parts Sep 5, 2024

wording fix

586f7c0

aditi-khare-mongoDB force-pushed the uri-validate-less branch from bda0451 to 586f7c0 Compare September 5, 2024 05:02

aditi-khare-mongoDB and others added 3 commits September 5, 2024 01:02

Merge branch 'main' into uri-validate-less

36b0b49

remove stray comment

bea037e

fix up

ae79ac5

aditi-khare-mongoDB changed the title ~~feat(NODE-6289): Allow valid SRV hostnames with less than 3 parts~~ feat(NODE-6289): allow valid srv hostnames with less than 3 parts Sep 5, 2024

aditi-khare-mongoDB and others added 4 commits September 5, 2024 01:34

add back in comment

b2f843e

fix failing unit tests

154e2ad

Merge branch 'main' into uri-validate-less

1059175

Merge branch 'main' into uri-validate-less

d1209df

aditi-khare-mongoDB marked this pull request as ready for review September 19, 2024 15:45

W-A-James self-assigned this Sep 19, 2024

W-A-James self-requested a review September 19, 2024 15:47

W-A-James added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Sep 19, 2024

W-A-James requested changes Sep 19, 2024

View reviewed changes

requested changes 1

9dd438e

aditi-khare-mongoDB requested a review from W-A-James September 19, 2024 21:49

W-A-James requested changes Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NODE-6289): allow valid srv hostnames with less than 3 parts #4197

feat(NODE-6289): allow valid srv hostnames with less than 3 parts #4197

aditi-khare-mongoDB commented Aug 15, 2024 •

edited

Loading

W-A-James Sep 19, 2024

aditi-khare-mongoDB Sep 19, 2024

W-A-James Sep 19, 2024

aditi-khare-mongoDB Sep 19, 2024

W-A-James Sep 19, 2024

W-A-James Sep 19, 2024

W-A-James Sep 19, 2024

W-A-James Sep 19, 2024

W-A-James Sep 19, 2024

W-A-James Sep 19, 2024

aditi-khare-mongoDB Sep 19, 2024

W-A-James Sep 20, 2024

W-A-James Sep 20, 2024

W-A-James Sep 20, 2024

W-A-James Sep 20, 2024

W-A-James Sep 20, 2024

	it('do not error on an SRV because it has two domain levels', async function () {
	it('does not error on an SRV because it has two domain levels', async function () {

	expect(() => checkParentDomainMatch(exampleHostNamesWithDot, exampleSrvName)).to.not.throw;
	expect(() => checkParentDomainMatch(exampleHostNamesWithDot, exampleSrvName)).to.not.throw();

	context('1) When running validation on an SRV string before DNS resolution', function () {
	context('1. When running validation on an SRV string before DNS resolution', function () {

	'2) When given a host from DNS resolution that does NOT end with the original SRVs domain name',
	'2. When given a host from DNS resolution that does NOT end with the original SRVs domain name',

	'3) When given a host from DNS resolution that is identical to the original SRVs hostname',
	'3. When given a host from DNS resolution that is identical to the original SRVs hostname',

feat(NODE-6289): allow valid srv hostnames with less than 3 parts #4197

Are you sure you want to change the base?

feat(NODE-6289): allow valid srv hostnames with less than 3 parts #4197

Conversation

aditi-khare-mongoDB commented Aug 15, 2024 • edited Loading

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Release Highlight

Allow SRV hostnames with less than three . separated parts

Double check the following

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditi-khare-mongoDB commented Aug 15, 2024 •

edited

Loading

Allow SRV hostnames with less than three `.` separated parts