mirror of
https://github.com/discourse/discourse.git
synced 2026-08-06 13:08:40 +08:00
`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.
127 lines
4.5 KiB
Ruby
Vendored
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
|