FIX: invalid urls should not break store.has_been_uploaded?

FIX: invalid urls should not break store.has_been_uploaded?

Breaking this method has wide ramification including breaking search indexing.

diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb
index ef4717f..6dd6214 100644
--- a/lib/file_store/s3_store.rb
+++ b/lib/file_store/s3_store.rb
@@ -89,7 +89,10 @@ module FileStore
 
       begin
         parsed_url = URI.parse(UrlHelper.encode(url))
-      rescue URI::InvalidURIError, URI::InvalidComponentError
+      rescue
+        # There are many exceptions possible here including Addressable::URI:: excpetions
+        # and URI:: exceptions, catch all may seem wide, but it makes no sense to raise ever
+        # on an invalid url here
         return false
       end
 
diff --git a/spec/multisite/s3_store_spec.rb b/spec/multisite/s3_store_spec.rb
index c857e3e..f36a3a4 100644
--- a/spec/multisite/s3_store_spec.rb
+++ b/spec/multisite/s3_store_spec.rb
@@ -283,9 +283,10 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do
     let(:s3_bucket) { resource.bucket(SiteSetting.s3_upload_bucket) }
     let(:s3_helper) { store.s3_helper }
 
-    it "returns false for blank urls" do
-      url = ""
-      expect(store.has_been_uploaded?(url)).to eq(false)
+    it "returns false for blank urls and bad urls" do
+      expect(store.has_been_uploaded?("")).to eq(false)
+      expect(store.has_been_uploaded?("http://test@test.com:test/test.git")).to eq(false)
+      expect(store.has_been_uploaded?("http:///+test@test.com/test.git")).to eq(false)
     end
 
     it "returns true if the base hostname is the same for both urls" do

GitHub sha: 689568c2

1 Like