0
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2026-08-06 07:23:30 +08:00
discourse/spec/lib/discourse_ip_info_spec.rb
Alan Guo Xiang Tan c1408b6d5a
FIX: Add reverse DNS timeout to DiscourseIpInfo (#40593)
`DiscourseIpInfo#get` returns MaxMind IP data by default. The admin IP
info endpoint asks for a hostname with `resolve_hostname: true`, which
makes `DiscourseIpInfo` run a reverse DNS PTR lookup through
`Resolv::DNS#getname` and add `hostname` when that lookup succeeds.

That resolver call did not configure `Resolv::DNS#timeouts`, so Ruby
used its default retry schedule. Ruby 3.4's
[`Resolv::DNS::Config#generate_timeouts`](https://github.com/ruby/ruby/blob/v3_4_0/lib/resolv.rb#L1133-L1140)
starts at 5 seconds and derives the later attempts from the configured
nameserver count. With one configured nameserver, that generates `[5,
10, 20, 40]`. A reverse DNS server that does not answer can therefore
keep the request busy long enough to hit Discourse's production
[Pitchfork timeout of 30
seconds](https://github.com/discourse/discourse/blob/main/config/pitchfork.conf.rb#L34-L36).
When that happens, the whole `/admin/users/ip-info.json` request can
fail even though the MaxMind location data has already been collected.

This PR bounds the reverse DNS part of IP info lookups. The hostname is
useful context, but it is less critical than the IP location data, so it
is fine to leave it out when reverse DNS fails.

Key technical changes:

1. Add `HOSTNAME_LOOKUP_TIMEOUT_SECONDS` to `DiscourseIpInfo` and set it
to 1 second. The hostname lookup gets a short budget before Discourse
returns the location data without `hostname`.

2. Apply that timeout to the `Resolv::DNS` instance before requesting
the hostname. This keeps the timeout decision at the shared IP info
boundary instead of making individual callers handle DNS behavior.

3. Treat `Resolv::ResolvError` and `Timeout::Error` as a missing
hostname. Reverse DNS failures no longer discard the location fields
that were already populated from MaxMind.

4. Keep the existing API shape for `/admin/users/ip-info.json`.
Successful hostname lookups still include `hostname`, failed or
interrupted hostname lookups omit it, and the existing staff permission
behavior is unchanged.
2026-06-05 12:29:51 +08:00

127 lines
4.5 KiB
Ruby
Vendored

# frozen_string_literal: true
RSpec.describe DiscourseIpInfo do
describe ".get" do
let(:ip) { "81.2.69.142" }
let(:expected_ip_info) do
{
city: "London",
country: "United Kingdom",
country_code: "GB",
geoname_ids: [6_255_148, 2_635_167, 2_643_743, 6_269_131],
location: "London, England, United Kingdom",
region: "England",
latitude: 51.5142,
longitude: -0.0931,
}
end
before { described_class.open_db(Rails.root.join("spec/fixtures/mmdb").to_s) }
it "returns IP info without hostname when reverse DNS is interrupted" do
Resolv::DNS.any_instance.stubs(:getname).with(ip).raises(Timeout::Error)
result = described_class.get(ip, resolve_hostname: true)
expect(result).to eq(expected_ip_info)
end
it "sets a timeout for reverse DNS" do
resolver = Resolv::DNS.new
resolver
.expects(:timeouts=)
.with { |timeouts| Array(timeouts).present? && Array(timeouts).sum <= 5 }
resolver.stubs(:getname).with(ip).raises(Resolv::ResolvError)
Resolv::DNS.stubs(:new).returns(resolver)
result = described_class.get(ip, resolve_hostname: true)
expect(result).to eq(expected_ip_info)
end
end
describe ".mmdb_download" do
before { Discourse::Utils.stubs(:execute_command) }
it "should download the MaxMind databases from MaxMind's download permalinks when `maxmind_license_key` and `maxmind_account_id` global setting has been set" do
global_setting :maxmind_license_key, "license_key"
global_setting :maxmind_account_id, "account_id"
stub_request(
:get,
"https://download.maxmind.com/geoip/databases/GeoLite2-City/download?suffix=tar.gz",
).with(basic_auth: %w[account_id license_key]).to_return(
status: 302,
body: "",
headers: {
location:
"https://mm-prod-geoip-databases.a2649acb697e2c09b632799562c076f2.r2.cloudflarestorage.com/some-path",
},
)
stub_request(
:get,
"https://mm-prod-geoip-databases.a2649acb697e2c09b632799562c076f2.r2.cloudflarestorage.com/some-path",
).with { |req| expect(req.headers.key?("Authorization")).to eq(false) }.to_return(status: 200)
described_class.mmdb_download("GeoLite2-City")
end
it "should download the MaxMind databases from MaxMind's undocumented download URL when `maxmind_license_key` global setting has been set but not `maxmind_account_id` for backwards compatibility reasons" do
global_setting :maxmind_license_key, "license_key"
stub_request(
:get,
"https://download.maxmind.com/app/geoip_download?license_key=license_key&edition_id=GeoLite2-City&suffix=tar.gz",
).to_return(status: 200, body: "", headers: {})
described_class.mmdb_download("GeoLite2-City")
end
it "should download the MaxMind databases from the right URL when `maxmind_mirror_url` global setting has been configured" do
global_setting :maxmind_mirror_url, "https://b.www.example.com/mirror"
stub_request(:get, "https://b.www.example.com/mirror/GeoLite2-City.tar.gz").to_return(
status: 200,
body: "",
)
described_class.mmdb_download("GeoLite2-City")
end
it "should download the MaxMind databases from the right URL when `maxmind_mirror_url` global setting has been configured and has a trailing slash" do
global_setting :maxmind_mirror_url, "https://b.www.example.com/mirror/"
stub_request(:get, "https://b.www.example.com/mirror/GeoLite2-City.tar.gz").to_return(
status: 200,
body: "",
)
described_class.mmdb_download("GeoLite2-City")
end
it "should not throw an error and instead log the exception when database file fails to download" do
fake_logger = FakeLogger.new
Rails.logger.broadcast_to(fake_logger)
global_setting :maxmind_license_key, "license_key"
global_setting :maxmind_account_id, "account_id"
stub_request(
:get,
"https://download.maxmind.com/geoip/databases/GeoLite2-City/download?suffix=tar.gz",
).with(basic_auth: %w[account_id license_key]).to_return(status: 500, body: nil, headers: {})
expect do described_class.mmdb_download("GeoLite2-City") end.not_to raise_error
expect(fake_logger.warnings.length).to eq(1)
expect(fake_logger.warnings.first).to include(
"MaxMind database GeoLite2-City download failed. 500 Error",
)
ensure
Rails.logger.stop_broadcasting_to(fake_logger)
end
end
end