FIX: Invalid URLs could raise exceptions when calling UrlHelper.rails_route_from_url (#10782)

FIX: Invalid URLs could raise exceptions when calling UrlHelper.rails_route_from_url (#10782)

Upload.secure_media_url? raised an exceptions when the URL was invalid, which was a issue in some situations where secure media URLs must be removed.

For example, sending digests used PrettyText.strip_secure_media, which used Upload.secure_media_url? to replace secure media with placeholders. If the URL was invalid, then an exception would be raised and left unhandled.

Now instead in UrlHelper.rails_route_from_url we return nil if there is something wrong with the URL.

Co-authored-by: Bianca Nenciu nenciu.bianca@gmail.com

diff --git a/app/models/upload.rb b/app/models/upload.rb
index 1656fdb..36fc6da 100644
--- a/app/models/upload.rb
+++ b/app/models/upload.rb
@@ -167,6 +167,7 @@ class Upload < ActiveRecord::Base
     # we do not want to exclude topic links that for whatever reason
     # have secure-media-uploads in the URL e.g. /t/secure-media-uploads-are-cool/223452
     route = UrlHelper.rails_route_from_url(url)
+    return false if route.blank?
     route[:action] == "show_secure" && route[:controller] == "uploads" && FileHelper.is_supported_media?(url)
   rescue ActionController::RoutingError
     false
diff --git a/lib/url_helper.rb b/lib/url_helper.rb
index 1915b78..7968c9a 100644
--- a/lib/url_helper.rb
+++ b/lib/url_helper.rb
@@ -70,6 +70,8 @@ class UrlHelper
   def self.rails_route_from_url(url)
     path = URI.parse(encode(url)).path
     Rails.application.routes.recognize_path(path)
+  rescue Addressable::URI::InvalidURIError
+    nil
   end
 
   def self.s3_presigned_url?(url)
diff --git a/spec/components/url_helper_spec.rb b/spec/components/url_helper_spec.rb
index 25c5dbd..d56efc2 100644
--- a/spec/components/url_helper_spec.rb
+++ b/spec/components/url_helper_spec.rb
@@ -196,4 +196,14 @@ describe UrlHelper do
     end
   end
 
+  describe "rails_route_from_url" do
+    it "returns a rails route from the path" do
+      expect(described_class.rails_route_from_url("/u")).to eq({ controller: "users", action: "index" })
+    end
+
+    it "does not raise for invalid URLs" do
+      url = "http://URL:%20https://google.com"
+      expect(described_class.rails_route_from_url(url)).to eq(nil)
+    end
+  end
 end
diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb
index 10ddea1..d33ae4c 100644
--- a/spec/models/upload_spec.rb
+++ b/spec/models/upload_spec.rb
@@ -493,5 +493,10 @@ describe Upload do
       url = "/uploads/default/test_0/original/1X/e1864389d8252958586c76d747b069e9f68827e3.png"
       expect(Upload.secure_media_url?(url)).to eq(false)
     end
+
+    it "does not raise for invalid URLs" do
+      url = "http://URL:%20https://google.com"
+      expect(Upload.secure_media_url?(url)).to eq(false)
+    end
   end
 end

GitHub sha: 39b2fb86

1 Like

This commit appears in #10782 which was merged by martin.