Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions lib/mongo/srv/result.rb
Original file line number Diff line number Diff line change
Expand Up @@ -110,17 +110,28 @@ def normalize_hostname(host)
# A hostname's domain name consists of each of the '.' delineated
# parts after the first. For example, the hostname 'foo.bar.baz'
# has the domain name 'bar.baz'.
#
# If the hostname has less than three parts, its domain name is the hostname itself.
#
# @param [ String ] record_host The host of the SRV record.
#
# @raise [ Mongo::Error::MismatchedDomain ] If the record's domain name doesn't match that of
# the hostname.
def validate_same_origin!(record_host)
domain_name ||= query_hostname.split('.')[1..-1]
host_parts = record_host.split('.')
srv_is_less_than_three_parts = query_hostname.split('.').length < 3
srv_host_domain = if srv_is_less_than_three_parts
query_hostname.split('.')
else
query_hostname.split('.')[1..-1]
end
Copy link
Copy Markdown
Contributor

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_hostname once and saving the parts, and then referring to them as needed.

Suggested change
srv_is_less_than_three_parts = query_hostname.split('.').length < 3
srv_host_domain = if srv_is_less_than_three_parts
query_hostname.split('.')
else
query_hostname.split('.')[1..-1]
end
query_hostname_parts = query_hostname.split('.')
srv_is_less_than_three_parts = query_hostname_parts.length < 3
srv_host_domain = if srv_is_less_than_three_parts
query_hostname_parts
else
query_hostname_parts[1..-1]
end

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

record_host_parts = record_host.split('.')

if (srv_is_less_than_three_parts && record_host_parts.length <= srv_host_domain.length)
raise Error::MismatchedDomain.new(MISMATCHED_DOMAINNAME % [record_host, srv_host_domain])
end

unless (host_parts.size > domain_name.size) && (domain_name == host_parts[-domain_name.length..-1])
raise Error::MismatchedDomain.new(MISMATCHED_DOMAINNAME % [record_host, domain_name])
unless (record_host_parts.size > srv_host_domain.size) && (srv_host_domain == record_host_parts[-srv_host_domain.size..-1])
raise Error::MismatchedDomain.new(MISMATCHED_DOMAINNAME % [record_host, srv_host_domain])
end
end
end
Expand Down
7 changes: 3 additions & 4 deletions lib/mongo/uri/srv_protocol.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little more idiomatic to ask parts.empty? than to query on the length directly, but I can see an argument being made for the former here (since we're actually measuring the number of parts).

In that case, though, I think parts.length < 1 might be clearer, since that way we're making it explicit that the number of parts has a minimum of 1.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

raise_invalid_error!("Hostname cannot be empty: #{hostname}")
end
end

Expand Down
65 changes: 65 additions & 0 deletions spec/mongo/srv/result_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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, e.g.

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)
end

This 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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
Comment thread
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
Expand Down
36 changes: 4 additions & 32 deletions spec/mongo/uri/srv_protocol_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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}" }
Expand Down Expand Up @@ -220,24 +210,6 @@
expect { uri }.to raise_error(Mongo::Error::InvalidURI)
end
end

context 'mongodb+srv://example.com?w=1' do
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -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

Expand All @@ -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

Expand Down
8 changes: 0 additions & 8 deletions spec/spec_tests/data/connection_string/invalid-uris.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,6 @@ tests:
hosts: ~
auth: ~
options: ~
-
description: "Invalid scheme"
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be valid under the new SRV requirements, right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new SRV requirements only apply to mongodb+srv schemes. Note the scheme for this test is mongo, which is not a supported scheme. (We only support mongodb:// and mongodb+srv:// -- which is what this test is ensuring.)

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...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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://"
Expand Down
Loading