-
Notifications
You must be signed in to change notification settings - Fork 530
RUBY-3519 Allow valid SRV hostnames with less than 3 parts #2972
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
Changes from 6 commits
f405c06
1727360
0b12109
618275d
a65c297
25845e7
2013f18
298afdc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -168,8 +168,7 @@ def parse!(remaining) | |
| # The hostname cannot include a port. | ||
| # | ||
| # The hostname must not begin with a dot, end with a dot, or have | ||
| # consecutive dots. The hostname must have a minimum of 3 total | ||
| # components (foo.bar.tld). | ||
| # consecutive dots. The hostname must have a minimum of 1 component | ||
| # | ||
| # Raises Error::InvalidURI if validation fails. | ||
| def validate_srv_hostname(hostname) | ||
|
|
@@ -185,8 +184,8 @@ def validate_srv_hostname(hostname) | |
| if parts.any?(&:empty?) | ||
| raise_invalid_error!("Hostname cannot have consecutive dots: #{hostname}") | ||
| end | ||
| if parts.length < 3 | ||
| raise_invalid_error!("Hostname must have a minimum of 3 components (foo.bar.tld): #{hostname}") | ||
| if parts.length == 0 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a little more idiomatic to ask In that case, though, I think
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
| raise_invalid_error!("Hostname cannot be empty: #{hostname}") | ||
| end | ||
| end | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,71 @@ | |
| expect(result.address_strs).to eq(['foo.bar.com:42']) | ||
| end | ||
| end | ||
|
|
||
| example_srv_names = ['i-love-rb', 'i-love-rb.mongodb', 'i-love-ruby.mongodb.io']; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❤️ |
||
| example_host_names = [ | ||
| 'rb-00.i-love-rb', | ||
| 'rb-00.i-love-rb.mongodb', | ||
| 'i-love-ruby-00.mongodb.io' | ||
| ]; | ||
| example_host_names_that_do_not_match_parent = [ | ||
| 'rb-00.i-love-rb-a-little', | ||
| 'rb-00.i-love-rb-a-little.mongodb', | ||
| 'i-love-ruby-00.evil-mongodb.io' | ||
| ]; | ||
|
|
||
| (0..2).each do |i| | ||
| context "when srvName has #{i+1} part#{i != 0 ? 's' : ''}" do | ||
| let(:srv_name) { example_srv_names[i] } | ||
| let(:host_name) { example_host_names[i] } | ||
| let(:mismatched_host_name) { example_host_names_that_do_not_match_parent[i] } | ||
| context 'when address does not match parent domain' do | ||
| it 'raises MismatchedDomain error' do | ||
| record = double('record').tap do |record| | ||
| allow(record).to receive(:target).and_return(mismatched_host_name) | ||
| allow(record).to receive(:port).and_return(42) | ||
| allow(record).to receive(:ttl).and_return(1) | ||
| end | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When writing tests with rspec, the recommended way of setting up test data is to use let(:record) do
double('record').tap do |record|
allow(record).to receive(:target).and_return(mismatched_host_name)
allow(record).to receive(:port).and_return(42)
allow(record).to receive(:ttl).and_return(1)
end
end
let(:result) { described_class.new(srv_name) }Then, the test itself reduces to just testing the expectations: it 'raises MismatchedDomain error' do
expect { result.add_record(record) }.to raise_error(Mongo::Error::MismatchedDomain)
endThis pattern is especially useful when the setup is repeated for multiple tests, but is still preferred even if it's only used in a single test.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done! |
||
|
|
||
| expect { | ||
| result = described_class.new(srv_name) | ||
| result.add_record(record) | ||
| }.to raise_error(Mongo::Error::MismatchedDomain) | ||
| end | ||
| end | ||
| context 'when address matches parent domain' do | ||
|
jamis marked this conversation as resolved.
|
||
| it 'adds the record' do | ||
| record = double('record').tap do |record| | ||
| allow(record).to receive(:target).and_return(host_name) | ||
| allow(record).to receive(:port).and_return(42) | ||
| allow(record).to receive(:ttl).and_return(1) | ||
| end | ||
|
|
||
| result = described_class.new(srv_name) | ||
| result.add_record(record) | ||
|
|
||
| expect(result.address_strs).to eq([host_name + ':42']) | ||
| end | ||
| end | ||
|
|
||
| if i < 2 | ||
| context 'when the address is less than 3 parts' do | ||
| it 'does not accept address if it does not contain an extra domain level' do | ||
| record = double('record').tap do |record| | ||
| allow(record).to receive(:target).and_return(srv_name) | ||
| allow(record).to receive(:port).and_return(42) | ||
| allow(record).to receive(:ttl).and_return(1) | ||
| end | ||
|
|
||
| expect { | ||
| result = described_class.new(srv_name) | ||
| result.add_record(record) | ||
| }.to raise_error(Mongo::Error::MismatchedDomain) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
|
|
||
| describe '#normalize_hostname' do | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,16 +56,6 @@ | |
| end | ||
| end | ||
|
|
||
| context 'when the host in URI does not have {hostname}, {domainname} and {tld}' do | ||
|
|
||
| let(:string) { "#{scheme}#{hosts}" } | ||
| let(:hosts) { '10gen.cc/' } | ||
|
|
||
| it 'raises an error' do | ||
| expect { uri }.to raise_error(Mongo::Error::InvalidURI) | ||
| end | ||
| end | ||
|
|
||
| context 'when the {tld} is empty' do | ||
|
|
||
| let(:string) { "#{scheme}#{hosts}" } | ||
|
|
@@ -220,24 +210,6 @@ | |
| expect { uri }.to raise_error(Mongo::Error::InvalidURI) | ||
| end | ||
| end | ||
|
|
||
| context 'mongodb+srv://example.com?w=1' do | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just removed this test, don't think this should throw under the new requirements. |
||
|
|
||
| let(:string) { "#{scheme}example.com?w=1" } | ||
|
|
||
| it 'raises an error' do | ||
| expect { uri }.to raise_error(Mongo::Error::InvalidURI) | ||
| end | ||
| end | ||
|
|
||
| context 'mongodb+srv://example.com/?w' do | ||
|
|
||
| let(:string) { "#{scheme}example.com/?w" } | ||
|
|
||
| it 'raises an error' do | ||
| expect { uri }.to raise_error(Mongo::Error::InvalidURI) | ||
| end | ||
| end | ||
| end | ||
|
|
||
| describe 'valid uris' do | ||
|
|
@@ -1302,8 +1274,8 @@ | |
| 'a' | ||
| end | ||
|
|
||
| it 'raises an error' do | ||
| expect { validate }.to raise_error(Mongo::Error::InvalidURI) | ||
| it 'does not raise an error' do | ||
| expect { validate }.to_not raise_error | ||
| end | ||
| end | ||
|
|
||
|
|
@@ -1313,8 +1285,8 @@ | |
| 'a.b' | ||
| end | ||
|
|
||
| it 'raises an error' do | ||
| expect { validate }.to raise_error(Mongo::Error::InvalidURI) | ||
| it 'validates the hostname' do | ||
| expect { validate }.not_to raise_error | ||
| end | ||
| end | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,14 +7,6 @@ tests: | |
| hosts: ~ | ||
| auth: ~ | ||
| options: ~ | ||
| - | ||
| description: "Invalid scheme" | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should be valid under the new SRV requirements, right?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new SRV requirements only apply to So, this test should remain unchanged -- and it should be passing. If it is failing with your changes, then you might have touched more than you intended...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed! thanks! |
||
| uri: "mongo://localhost:27017" | ||
| valid: false | ||
| warning: ~ | ||
| hosts: ~ | ||
| auth: ~ | ||
| options: ~ | ||
| - | ||
| description: "Missing host" | ||
| uri: "mongodb://" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For efficiency's sake, I'd recommend splitting the
query_hostnameonce and saving the parts, and then referring to them as needed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed!