FIX: Use same code path for downloading images

lib/file_helper.rb

@@ -1,4 +1,5 @@
 require "open-uri"
+require "final_destination"
 
 class FileHelper
 
@@ -10,7 +11,7 @@ class FileHelper
     url = "https:" + url if url.start_with?("//")
     raise Discourse::InvalidParameters.new(:url) unless url =~ /^https?:\/\//
 
-    uri = parse_url(url)
+    uri = FinalDestination.new(url).resolve
     extension = File.extname(uri.path)
     tmp = Tempfile.new([tmp_file_name, extension])
 
@@ -36,15 +37,4 @@ class FileHelper
     @@images_regexp ||= /\.(#{images.to_a.join("|")})$/i
   end
 
-  # HACK to support underscores in URLs
-  # cf. http://stackoverflow.com/a/18938253/11983
-  def self.parse_url(url)
-    URI.parse(url)
-  rescue URI::InvalidURIError
-    host = url.match(".+\:\/\/([^\/]+)")[1]
-    uri = URI.parse(url.sub(host, 'valid-host'))
-    uri.instance_variable_set("@host", host)
-    uri
-  end
-
 end

lib/final_destination.rb

@@ -62,7 +62,7 @@ class FinalDestination
   end
 
   def validate_uri
-    validate_uri_format && is_public?
+    validate_uri_format && is_dest_valid?
   end
 
   def validate_uri_format
@@ -75,6 +75,10 @@ class FinalDestination
     (IPAddr.new(@uri.hostname) rescue nil).nil?
   end
 
+  def is_dest_valid?
+    is_public?
+  end
+
   def is_public?
     return false unless @uri && @uri.host
 
@@ -116,4 +120,5 @@ class FinalDestination
     end
     header[1] if header
   end
+
 end

spec/components/file_helper_spec.rb

@@ -7,6 +7,7 @@ describe FileHelper do
   let(:png) { Base64.decode64("R0lGODlhAQABALMAAAAAAIAAAACAAICAAAAAgIAAgACAgMDAwICAgP8AAAD/AP//AAAA//8A/wD//wBiZCH5BAEAAA8ALAAAAAABAAEAAAQC8EUAOw==") }
 
   before do
+    Excon.stub({ method: :head, hostname: 'eviltrout.com' }, {})
     stub_request(:get, url).to_return(body: png)
   end
 

spec/components/final_destination_spec.rb

@@ -10,9 +10,12 @@ describe FinalDestination do
         when 'eviltrout.com' then '52.84.143.152'
         when 'codinghorror.com' then '91.146.108.148'
         when 'discourse.org' then '104.25.152.10'
+        when 'some_thing.example.com' then '104.25.152.10'
         when 'private-host.com' then '192.168.10.1'
         when 'internal-ipv6.com' then '2001:abc:de:01:3:3d0:6a65:c2bf'
         else
+          as_ip = IPAddr.new(host) rescue nil
+          raise "couldn't lookup #{host}" if as_ip.nil?
           host
         end
       end
@@ -54,6 +57,20 @@ describe FinalDestination do
         expect(final.redirected?).to eq(false)
         expect(final.status).to eq(:resolved)
       end
+
+    end
+
+    context "underscores in URLs" do
+      before do
+        Excon.stub({ method: :head, hostname: 'some_thing.example.com' }, doc_response)
+      end
+
+      it "doesn't raise errors with underscores in urls" do
+        final = FinalDestination.new('https://some_thing.example.com', opts)
+        expect(final.resolve.to_s).to eq('https://some_thing.example.com')
+        expect(final.redirected?).to eq(false)
+        expect(final.status).to eq(:resolved)
+      end
     end
 
     context "with a couple of redirects" do

spec/controllers/uploads_controller_spec.rb

@@ -74,7 +74,7 @@ describe UploadsController do
         controller.stubs(:is_api?).returns(true)
 
         Jobs.expects(:enqueue).with(:create_avatar_thumbnails, anything)
-
+        Excon.stub({ method: :head, hostname: 'example.com' }, {})
         stub_request(:get, "http://example.com/image.png").to_return(body: File.read('spec/fixtures/images/logo.png'))
 
         xhr :post, :create, url: 'http://example.com/image.png', type: "avatar", synchronous: true
@@ -183,7 +183,7 @@ describe UploadsController do
 
     it "handles file without extension" do
       SiteSetting.authorized_extensions = "*"
-      upload = Fabricate(:upload, original_filename: "image_file", sha1: sha)
+      Fabricate(:upload, original_filename: "image_file", sha1: sha)
       controller.stubs(:render)
       controller.expects(:send_file)
 

spec/jobs/pull_hotlinked_images_spec.rb

@@ -6,6 +6,7 @@ describe Jobs::PullHotlinkedImages do
   before do
     png = Base64.decode64("R0lGODlhAQABALMAAAAAAIAAAACAAICAAAAAgIAAgACAgMDAwICAgP8AAAD/AP//AAAA//8A/wD//wBiZCH5BAEAAA8ALAAAAAABAAEAAAQC8EUAOw==")
     stub_request(:get, "http://wiki.mozilla.org/images/2/2e/Longcat1.png").to_return(body: png)
+    Excon.stub({ method: :head, hostname: 'wiki.mozilla.org' }, {})
     SiteSetting.download_remote_images_to_local = true
     FastImage.expects(:size).returns([100, 100]).at_least_once
   end