CSHARP-6214: Implement srvAllowedHostsSuffix URI option - #2109
Conversation
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Implements the srvAllowedHostsSuffix URI option (including spec-mandated normalization and Public Suffix List validation) and wires it through client/cluster settings into SRV polling so that SRV host validation can be constrained to a user-configured parent domain without affecting which SRV records are queried.
Changes:
- Added
srvAllowedHostsSuffixparsing, validation, and normalization (trim dots → IDN/Punycode → lowercase → reject public suffix → prepend dot) plus updated SRV-host parent-domain validation to optionally use the configured suffix. - Vendored the Public Suffix List as an embedded resource and introduced a small matcher (
PublicSuffixList) with prose tests. - Propagated the option through
MongoUrlBuilder/MongoUrl/MongoClientSettings→ClusterSettings→DnsMonitorand updated relevant unit tests/mocks.
Reviewed changes
Copilot reviewed 43 out of 44 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| THIRD-PARTY-NOTICES | Adds MPL 2.0 notice for the embedded Public Suffix List. |
| .gitattributes | Pins LF line endings for the vendored PSL data file to keep embedded bytes stable across platforms. |
| src/MongoDB.Driver/MongoDB.Driver.csproj | Embeds public_suffix_list.dat with an explicit LogicalName for stable resource loading. |
| src/MongoDB.Driver/Core/Misc/public_suffix_list.dat | Vendored PSL rules file used to reject overly-broad suffixes. |
| src/MongoDB.Driver/Core/Misc/PublicSuffixList.cs | Adds PSL matcher/loader backed by the embedded resource. |
| src/MongoDB.Driver/Core/Configuration/ConnectionString.cs | Parses srvAllowedHostsSuffix, normalizes it, and uses it for SRV host parent-domain validation. |
| src/MongoDB.Driver/MongoUrlBuilder.cs | Adds SrvAllowedHostsSuffix option support (validation + serialization). |
| src/MongoDB.Driver/MongoUrl.cs | Surfaces SrvAllowedHostsSuffix on MongoUrl. |
| src/MongoDB.Driver/MongoClientSettings.cs | Adds SrvAllowedHostsSuffix setting, validation, cloning/equality/hash propagation, and cluster-key propagation. |
| src/MongoDB.Driver/Core/Configuration/ClusterSettings.cs | Stores/exposes SrvAllowedHostsSuffix and passes it through With(...). |
| src/MongoDB.Driver/ClusterKey.cs | Adds SrvAllowedHostsSuffix to the key for cluster identity comparisons. |
| src/MongoDB.Driver/ClusterRegistry.cs | Copies SrvAllowedHostsSuffix from ClusterKey into ClusterSettings (fixing SRV polling ignoring it). |
| src/MongoDB.Driver/Core/Clusters/IDnsMonitorFactory.cs | Extends factory contract to accept srvAllowedHostsSuffix. |
| src/MongoDB.Driver/Core/Clusters/DnsMonitorFactory.cs | Passes srvAllowedHostsSuffix through to DnsMonitor. |
| src/MongoDB.Driver/Core/Clusters/DnsMonitor.cs | Separates SRV query name from host-validation suffix; validates hosts against configured suffix when present. |
| src/MongoDB.Driver/Core/Clusters/MultiServerCluster.cs | Passes SrvAllowedHostsSuffix into the DNS monitor. |
| src/MongoDB.Driver/Core/Clusters/LoadBalancedCluster.cs | Passes SrvAllowedHostsSuffix into the DNS monitor. |
| tests/MongoDB.Driver.Tests/Specifications/public-suffix-list/PublicSuffixListTests.cs | Adds PSL prose tests (including internationalized rule handling). |
| tests/MongoDB.Driver.Tests/MongoUrlBuilderTests.cs | Adds MongoUrlBuilder tests for SrvAllowedHostsSuffix (including public-suffix rejection and non-SRV scheme rejection). |
| tests/MongoDB.Driver.Tests/Core/Configuration/ConnectionStringTests.cs | Adds unit tests verifying normalization of srvAllowedHostsSuffix (dots/case/IDN). |
| tests/MongoDB.Driver.Tests/ClusterRegistryTests.cs | Asserts cluster settings now receive SrvAllowedHostsSuffix. |
| tests/MongoDB.Driver.Tests/ClusterKeyTests.cs | Adds equality coverage for SrvAllowedHostsSuffix in ClusterKey test scaffolding. |
| tests/MongoDB.Driver.Tests/Core/Clusters/DnsMonitorTests.cs | Updates constructor usage for new DnsMonitor parameter. |
| tests/MongoDB.Driver.Tests/Core/Clusters/DnsMonitorFactoryTests.cs | Updates factory tests for new parameter. |
| tests/MongoDB.Driver.Tests/Core/Clusters/MultiServerClusterTests.cs | Updates mock IDnsMonitorFactory signature usage. |
| tests/MongoDB.Driver.Tests/Core/Clusters/LoadBalancedClusterTests.cs | Updates mock IDnsMonitorFactory signature usage. |
| specifications/connection-string/tests/invalid-uris.yml | Spec test updates (content excluded by policy). |
| specifications/connection-string/tests/invalid-uris.json | Spec test updates (content excluded by policy). |
| specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-with_dot.yml | Spec test additions (content excluded by policy). |
| specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-tld-only.yml | Spec test additions (content excluded by policy). |
| specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-mismatch.yml | Spec test additions (content excluded by policy). |
| specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-with_dot.json | Spec test additions (content excluded by policy). |
| specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-tld-only.json | Spec test additions (content excluded by policy). |
| specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-mismatch.json | Spec test additions (content excluded by policy). |
| specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-period-only.yml | Spec test additions (content excluded by policy). |
| specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-trailing-dot.yml | Spec test additions (content excluded by policy). |
| specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-period-only.json | Spec test additions (content excluded by policy). |
| specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-trailing-dot.json | Spec test additions (content excluded by policy). |
| specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-without_dot_pass.yml | Spec test additions (content excluded by policy). |
| specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-without_dot_fail.yml | Spec test additions (content excluded by policy). |
| specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-case-insensitive.yml | Spec test additions (content excluded by policy). |
| specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-without_dot_pass.json | Spec test additions (content excluded by policy). |
| specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-without_dot_fail.json | Spec test additions (content excluded by policy). |
| specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-case-insensitive.json | Spec test additions (content excluded by policy). |
Files excluded by content exclusion policy (18)
- specifications/connection-string/tests/invalid-uris.json
- specifications/connection-string/tests/invalid-uris.yml
- specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-case-insensitive.json
- specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-case-insensitive.yml
- specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-mismatch.json
- specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-mismatch.yml
- specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-period-only.json
- specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-period-only.yml
- specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-tld-only.json
- specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-tld-only.yml
- specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-trailing-dot.json
- specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-trailing-dot.yml
- specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-with_dot.json
- specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-with_dot.yml
- specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-without_dot_fail.json
- specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-without_dot_fail.yml
- specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-without_dot_pass.json
- specifications/initial-dns-seedlist-discovery/tests/replica-set/srvAllowedHostsSuffix-without_dot_pass.yml
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
21f8faf to
23c238b
Compare
| Optional<int> srvMaxHosts = default, | ||
| Optional<string> srvServiceName = default(Optional<string>)) | ||
| Optional<string> srvServiceName = default(Optional<string>), | ||
| Optional<string> srvAllowedHostsSuffix = default(Optional<string>)) |
There was a problem hiding this comment.
Isn't adding new optional parameter considered to be a breaking change?
There was a problem hiding this comment.
this is for the main branch where this is fine (4.0 work), I will change it for the 3.x branch PR
|
|
||
| namespace MongoDB.Driver.Core.Misc; | ||
|
|
||
| internal static class PublicSuffixList |
There was a problem hiding this comment.
Have you considered usage of some 3rd party package to handle the public suffix list? The problem with the embedded list that there is no way to populate it other then change the file and make a new version.
Should we use https://github.com/nager/Nager.PublicSuffix instead? This package can download the latest official list from publicsuffix.org
| } | ||
| if (_srvAllowedHostsSuffix != null) | ||
| { | ||
| query.AppendFormat("srvAllowedHostsSuffix={0}&", _srvAllowedHostsSuffix); |
There was a problem hiding this comment.
Should we encode _srvAllowedHostsSuffix?
…ffix List validation
… the Public Suffix List The suffix must contain at least two dot separated labels, or be a single label naming a private or special-use namespace. The vendored Public Suffix List, its matcher, and the MPL notice are removed. MongoUrlBuilder.ToString() escapes the option value. DnsMonitor no longer requires the SRV hostname to have three or more labels. ConnectionString accepts fewer and applies the stricter parent domain matching those hostnames require, and the single-label suffixes above make the shorter hostnames reachable through a supported configuration.
… option The specification requires drivers to make it clearly visible that relaxing SRV domain name validation is dangerous. Add the mandated warning to every public surface that exposes the option.
…idating them The specification requires a returned hostname, and the domain it is matched against, to have the trailing dot stripped, be converted to A-label form, and be ASCII lowercased before verification, so that none of those can affect the comparison. Previously only the trailing dot was stripped, so a mixed-case or U-label SRV target was rejected even when it matched. A hostname with no A-label form cannot be verified or connected to, so it is rejected: an error on initial seedlist resolution, and skipped with an SdamInformationEvent during SRV polling, which must not raise.
b61c090 to
6d1bf80
Compare
| var suffix = value.Trim('.'); | ||
| if (suffix.Length == 0) | ||
| { | ||
| errorMessage = "srvAllowedHostsSuffix must name at least one domain label."; |
There was a problem hiding this comment.
This error message is misleading: error message on line 1348 asks for 2 labels. Should we use the same message here too?
| @@ -1481,17 +1616,27 @@ private void ValidateResolvedHosts(string original, List<string> resolved) | |||
| var dnsEndPoint = (DnsEndPoint)endPoint; | |||
|
|
|||
| var host = ((DnsEndPoint)endPoint).Host; | |||
There was a problem hiding this comment.
It looks like unused variable
There was a problem hiding this comment.
thanks for the catch!
| } | ||
|
|
||
| return true; | ||
| // .NET Core rejects a label that starts with "xn--" and does not decode as Punycode, |
There was a problem hiding this comment.
Should we hide this code under #if net472?
There was a problem hiding this comment.
I don't think it's necessary since if netcore implementation fails then we just return from the method early and we don't run this code.
Implements the
srvAllowedHostsSuffixURI option and the validation that bounds how broad the configured suffix may be.Spec: Initial DNS Seedlist Discovery. Vendored spec tests are in sync with the merged spec.
The option
srvAllowedHostsSuffixlets a user state the parent domain that hosts returned by an SRV lookup are validated against, instead of having it inferred from the SRV hostname. The value is normalized in spec order — strip leading/trailing dots, convert to Punycode, ASCII lowercase, apply the label rule, prepend a dot — and is rejected on a non-SRV scheme, mirroringsrvServiceName.DnsMonitorpreviously usedlookupDomainNameboth to build the SRV query name and to validate returned hosts. Those are now separate: the option must not affect which records are queried, only which parent domain returned hosts are checked against.Validation
The spec bounds how broad the suffix may be, since the option relaxes a DNS-spoofing safeguard. This implements the label-count branch of the spec's rule 4: at least two
.separated labels, or a single label naming a private or special-use namespace —test,localhost,invalid,example(RFC 6761),local(RFC 6762),internal(ICANN-reserved),corp,home,mail.Worth stating plainly: this does not catch multi-label public suffixes.
co.uk,github.ioandcom.auare all accepted. That is the known cost of the label-count branch. Users wanting PSL-grade validation can layer it on through thesrvHostValidatorcallback in DRIVERS-3632.Also here: a pre-existing
DnsMonitorbugDnsMonitorrequired the SRV hostname to have at least three.separated labels, whileConnectionStringaccepts fewer and has tests saying it should. Somongodb+srv://mongo.local/parsed and then threwArgumentExceptionat cluster initialization.Not introduced by this feature, but the single-label rule above makes it reachable through a configuration the spec now says MUST be accepted:
mongodb+srv://mongo.corp/?srvAllowedHostsSuffix=corpis exactly the split-horizon case the carve-out serves, and it could not start. Fixing it separately would have meant shipping a supported option value into a path that throws.