From 7429700389191e49292a2164b069cb57877c2871 Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Tue, 28 May 2019 10:28:57 +1000 Subject: [PATCH] FIX: ensure we can download maxmind without redis or db config This also corrects FileHelper.download so it supports "follow_redirect" correctly (it used to always follow 1 redirect) and adds a `validate_url` param that will bypass all uri validation if set to false (default is true) --- app/controllers/uploads_controller.rb | 1 + lib/discourse_ip_info.rb | 4 ++- lib/file_helper.rb | 6 ++-- lib/final_destination.rb | 23 ++++++++------ lib/tasks/maxminddb.rake | 2 +- .../phpbb3/importers/avatar_importer.rb | 3 +- spec/components/file_helper_spec.rb | 31 +++++++++++++++++++ 7 files changed, 55 insertions(+), 15 deletions(-) diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 86e6e58762b..fb1ba6333d6 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -134,6 +134,7 @@ class UploadsController < ApplicationController maximum_upload_size = [SiteSetting.max_image_size_kb, SiteSetting.max_attachment_size_kb].max.kilobytes tempfile = FileHelper.download( url, + follow_redirect: true, max_file_size: maximum_upload_size, tmp_file_name: "discourse-upload-#{type}" ) rescue nil diff --git a/lib/discourse_ip_info.rb b/lib/discourse_ip_info.rb index 37b0fa2e74d..9daae979e63 100644 --- a/lib/discourse_ip_info.rb +++ b/lib/discourse_ip_info.rb @@ -30,7 +30,9 @@ class DiscourseIpInfo gz_file = FileHelper.download( "https://geolite.maxmind.com/geoip/databases/#{name}/update", max_file_size: 100.megabytes, - tmp_file_name: "#{name}.gz" + tmp_file_name: "#{name}.gz", + validate_uri: false, + follow_redirect: false ) Discourse::Utils.execute_command("gunzip", gz_file.path) diff --git a/lib/file_helper.rb b/lib/file_helper.rb index 6fc6f660b93..769c02c9c56 100644 --- a/lib/file_helper.rb +++ b/lib/file_helper.rb @@ -28,6 +28,7 @@ class FileHelper read_timeout: 5, skip_rate_limit: false, verbose: false, + validate_uri: true, retain_on_max_file_size_exceeded: false) url = "https:" + url if url.start_with?("//") @@ -37,9 +38,10 @@ class FileHelper fd = FinalDestination.new( url, - max_redirects: follow_redirect ? 5 : 1, + max_redirects: follow_redirect ? 5 : 0, skip_rate_limit: skip_rate_limit, - verbose: verbose + verbose: verbose, + validate_uri: validate_uri ) fd.get do |response, chunk, uri| diff --git a/lib/final_destination.rb b/lib/final_destination.rb index 3e1417f00b2..1278e99f126 100644 --- a/lib/final_destination.rb +++ b/lib/final_destination.rb @@ -41,21 +41,23 @@ class FinalDestination @opts[:lookup_ip] ||= lambda { |host| FinalDestination.lookup_ip(host) } @ignored = @opts[:ignore_hostnames] || [] + @limit = @opts[:max_redirects] - ignore_redirects = [Discourse.base_url_no_prefix] + if @limit > 0 + ignore_redirects = [Discourse.base_url_no_prefix] - if @opts[:ignore_redirects] - ignore_redirects.concat(@opts[:ignore_redirects]) - end + if @opts[:ignore_redirects] + ignore_redirects.concat(@opts[:ignore_redirects]) + end - ignore_redirects.each do |ignore_redirect| - ignore_redirect = uri(ignore_redirect) - if ignore_redirect.present? && ignore_redirect.hostname - @ignored << ignore_redirect.hostname + ignore_redirects.each do |ignore_redirect| + ignore_redirect = uri(ignore_redirect) + if ignore_redirect.present? && ignore_redirect.hostname + @ignored << ignore_redirect.hostname + end end end - @limit = @opts[:max_redirects] @status = :ready @http_verb = @force_get_hosts.any? { |host| hostname_matches?(host) } ? :get : :head @cookie = nil @@ -63,6 +65,7 @@ class FinalDestination @verbose = @opts[:verbose] || false @timeout = @opts[:timeout] || nil @preserve_fragment_url = @preserve_fragment_url_hosts.any? { |host| hostname_matches?(host) } + @validate_uri = @opts.fetch(:validate_uri) { true } end def self.connection_timeout @@ -252,7 +255,7 @@ class FinalDestination end def validate_uri - validate_uri_format && is_dest_valid? + !@validate_uri || (validate_uri_format && is_dest_valid?) end def validate_uri_format diff --git a/lib/tasks/maxminddb.rake b/lib/tasks/maxminddb.rake index f9bfa60d939..5023f1a98fb 100644 --- a/lib/tasks/maxminddb.rake +++ b/lib/tasks/maxminddb.rake @@ -3,7 +3,7 @@ require_dependency 'discourse_ip_info' desc "downloads MaxMind's GeoLite2-City database" -task "maxminddb:get" => :environment do +task "maxminddb:get" do puts "Downloading MaxMindDb's GeoLite2-City..." DiscourseIpInfo.mmdb_download('GeoLite2-City') diff --git a/script/import_scripts/phpbb3/importers/avatar_importer.rb b/script/import_scripts/phpbb3/importers/avatar_importer.rb index 41cac413c45..bb72572c0ba 100644 --- a/script/import_scripts/phpbb3/importers/avatar_importer.rb +++ b/script/import_scripts/phpbb3/importers/avatar_importer.rb @@ -70,7 +70,8 @@ module ImportScripts::PhpBB3 avatar_file = FileHelper.download( url, max_file_size: max_image_size_kb, - tmp_file_name: 'discourse-avatar' + tmp_file_name: 'discourse-avatar', + follow_redirect: true ) rescue StandardError => err warn "Error downloading avatar: #{err.message}. Skipping..." diff --git a/spec/components/file_helper_spec.rb b/spec/components/file_helper_spec.rb index 25dddec2d97..495bd02068b 100644 --- a/spec/components/file_helper_spec.rb +++ b/spec/components/file_helper_spec.rb @@ -33,6 +33,37 @@ describe FileHelper do end.to raise_error(OpenURI::HTTPError, "404 Error: 404") end + it "does not follow redirects if instructed not to" do + url2 = "https://test.com/image.png" + stub_request(:get, url).to_return(status: 302, body: "", headers: { location: url2 }) + + missing = FileHelper.download( + url, + max_file_size: 10000, + tmp_file_name: 'trouttmp', + follow_redirect: false + ) + + expect(missing).to eq(nil) + end + + it "does follow redirects if instructed to" do + url2 = "https://test.com/image.png" + stub_request(:get, url).to_return(status: 302, body: "", headers: { location: url2 }) + stub_request(:get, url2).to_return(status: 200, body: "i am the body") + + found = FileHelper.download( + url, + max_file_size: 10000, + tmp_file_name: 'trouttmp', + follow_redirect: true + ) + + expect(found.read).to eq("i am the body") + found.close + found.unlink + end + it "correctly raises an OpenURI HTTP error if it gets a 404" do url = "http://fourohfour.com/404"