DEV: Gracefully handle user avatar download SSRF errors (#21523)

### Background

When SSRF detection fails, the exception bubbles all the way up, causing a log alert. This isn't actionable, and should instead be ignored. The existing `rescue` does already ignore network errors, but fails to account for SSRF exceptions coming from `FinalDestination`.

### What is this change?

This PR does two things.

---

Firstly, it introduces a common root exception class, `FinalDestination::SSRFError` for SSRF errors. This serves two functions: 1) it makes it easier to rescue both errors at once, which is generally what one wants to do and 2) prevents having to dig deep into the class hierarchy for the constant.

This change is fully backwards compatible thanks to how inheritance and exception handling works.

---

Secondly, it rescues this new exception in `UserAvatar.import_url_for_user`, which is causing sporadic errors to be logged in production. After this SSRF errors are handled the same as network errors.
This commit is contained in:
Ted Johansson 2023-05-12 15:32:02 +08:00 committed by GitHub
parent e10b262eb9
commit 59867cc091
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 14 additions and 5 deletions

View File

@ -144,8 +144,8 @@ class UserAvatar < ActiveRecord::Base
user.update!(uploaded_avatar_id: upload.id)
end
end
rescue Net::ReadTimeout, OpenURI::HTTPError
# skip saving, we are not connected to the net
rescue Net::ReadTimeout, OpenURI::HTTPError, FinalDestination::SSRFError
# Skip saving. We are not connected to the net, or SSRF checks failed.
ensure
tempfile.close! if tempfile && tempfile.respond_to?(:close!)
end

View File

@ -8,6 +8,9 @@ require "url_helper"
# Determine the final endpoint for a Web URI, following redirects
class FinalDestination
class SSRFError < SocketError
end
MAX_REQUEST_TIME_SECONDS = 10
MAX_REQUEST_SIZE_BYTES = 5_242_880 # 1024 * 1024 * 5

View File

@ -2,9 +2,9 @@
class FinalDestination
module SSRFDetector
class DisallowedIpError < SocketError
class DisallowedIpError < SSRFError
end
class LookupFailedError < SocketError
class LookupFailedError < SSRFError
end
# This is a list of private IPv4 IP ranges that are not allowed to be globally reachable as given by

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, FinalDestination::SSRFDetector::LookupFailedError
rescue Net::ReadTimeout, FinalDestination::SSRFError
# do nothing for Net::ReadTimeout errors
end

View File

@ -175,6 +175,12 @@ RSpec.describe UserAvatar do
expect(user.user_avatar.custom_upload_id).to eq(nil)
end
end
it "doesn't error out if the remote request fails" do
FileHelper.stubs(:download).raises(FinalDestination::SSRFDetector::LookupFailedError.new)
expect { UserAvatar.import_url_for_user(anything, user) }.not_to raise_error
end
end
describe "ensure_consistency!" do