FIX: Gracefully handle DNS issued from SSRF lookup when inline oneboxing (#19631)

There is an issue where chat message processing breaks due to
unhandles `SocketError` exceptions originating in the SSRF check,
specifically in `FinalDestination::Resolver`.

This change gives `FinalDestination::SSRFDetector` a new error class
to wrap the `SocketError` in, and haves the `RetrieveTitle` class
handle that error gracefully.
This commit is contained in:
Ted Johansson 2022-12-28 10:30:20 +08:00 committed by GitHub
parent 462e14e279
commit 06db264f24
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 23 additions and 5 deletions

View File

@ -2,8 +2,8 @@
class FinalDestination
module SSRFDetector
class DisallowedIpError < SocketError
end
class DisallowedIpError < SocketError; end
class LookupFailedError < SocketError; end
def self.standard_private_ranges
@private_ranges ||= [
@ -61,7 +61,12 @@ class FinalDestination
end
def self.lookup_and_filter_ips(name, timeout: nil)
begin
ips = lookup_ips(name, timeout: timeout)
rescue SocketError
raise LookupFailedError, "FinalDestination: lookup failed"
end
return ips if host_bypasses_checks?(name)
ips.filter! { |ip| FinalDestination::SSRFDetector.ip_allowed?(ip) }

View File

@ -9,7 +9,7 @@ module RetrieveTitle
max_redirects: max_redirects,
initial_https_redirect_ignore_limit: initial_https_redirect_ignore_limit
)
rescue Net::ReadTimeout
rescue Net::ReadTimeout, FinalDestination::SSRFDetector::LookupFailedError
# do nothing for Net::ReadTimeout errors
end

View File

@ -95,6 +95,13 @@ describe FinalDestination::SSRFDetector do
)
end
it "raises an exception if lookup fails" do
subject.stubs(:lookup_ips).raises(SocketError)
expect { subject.lookup_and_filter_ips("example.com") }.to raise_error(
subject::LookupFailedError,
)
end
it "bypasses filtering for allowlisted hosts" do
SiteSetting.allowed_internal_hosts = "example.com"
subject.stubs(:lookup_ips).returns(["127.0.0.1"])

View File

@ -162,7 +162,13 @@ RSpec.describe RetrieveTitle do
it "it ignores Net::ReadTimeout errors" do
stub_request(:get, "https://example.com").to_raise(Net::ReadTimeout)
expect { RetrieveTitle.crawl("https://example.com") }.not_to raise_error
expect(RetrieveTitle.crawl("https://example.com")).to eq(nil)
end
it "ignores SSRF lookup errors" do
subject.stubs(:fetch_title).raises(FinalDestination::SSRFDetector::LookupFailedError)
expect(RetrieveTitle.crawl("https://example.com")).to eq(nil)
end
end