From da839e6d26b9a6cb411e47af3c0062fa508bc921 Mon Sep 17 00:00:00 2001
From: Blake Erickson <o.blakeerickson@gmail.com>
Date: Wed, 27 May 2020 09:23:55 -0600
Subject: [PATCH] SECURITY: Use FinalDestination for topic embeds

---
 app/models/topic_embed.rb       |  9 ++++++++-
 spec/models/topic_embed_spec.rb | 16 +++++++++-------
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/app/models/topic_embed.rb b/app/models/topic_embed.rb
index 362ee79f4ac..d8aef830806 100644
--- a/app/models/topic_embed.rb
+++ b/app/models/topic_embed.rb
@@ -109,7 +109,14 @@ class TopicEmbed < ActiveRecord::Base
 
     url = UrlHelper.escape_uri(url)
     original_uri = URI.parse(url)
-    raise URI::InvalidURIError unless original_uri.is_a?(URI::HTTP)
+    fd = FinalDestination.new(
+      url,
+      validate_uri: true,
+      max_redirects: 5
+    )
+
+    url = fd.resolve
+    raise URI::InvalidURIError if url.blank?
 
     opts = {
       tags: %w[div p code pre h1 h2 h3 b em i strong a img ul li ol blockquote],
diff --git a/spec/models/topic_embed_spec.rb b/spec/models/topic_embed_spec.rb
index c91781e6dd7..cfdddd389b0 100644
--- a/spec/models/topic_embed_spec.rb
+++ b/spec/models/topic_embed_spec.rb
@@ -166,6 +166,7 @@ describe TopicEmbed do
       before do
         file.stubs(:read).returns contents
         TopicEmbed.stubs(:open).returns file
+        stub_request(:head, url)
       end
 
       it "doesn't scrub the title by default" do
@@ -194,6 +195,7 @@ describe TopicEmbed do
         SiteSetting.embed_classname_whitelist = 'emoji, foo'
         file.stubs(:read).returns contents
         TopicEmbed.stubs(:open).returns file
+        stub_request(:head, url)
         response = TopicEmbed.find_remote(url)
       end
 
@@ -230,6 +232,7 @@ describe TopicEmbed do
       before(:each) do
         file.stubs(:read).returns contents
         TopicEmbed.stubs(:open).returns file
+        stub_request(:head, url)
         response = TopicEmbed.find_remote(url)
       end
 
@@ -252,6 +255,7 @@ describe TopicEmbed do
         SiteSetting.embed_classname_whitelist = ''
         file.stubs(:read).returns contents
         TopicEmbed.stubs(:open).returns file
+        stub_request(:head, url)
         response = TopicEmbed.find_remote(url)
       end
 
@@ -279,9 +283,8 @@ describe TopicEmbed do
       let!(:file) { StringIO.new }
 
       before do
-        file.stubs(:read).returns contents
-        TopicEmbed.stubs(:open)
-          .with('http://eviltrout.com/test/%D9%85%D8%A7%D9%87%DB%8C', allow_redirections: :safe).returns file
+        stub_request(:head, url)
+        stub_request(:get, url).to_return(body: contents).then.to_raise
       end
 
       it "doesn't throw an error" do
@@ -297,9 +300,8 @@ describe TopicEmbed do
       let!(:file) { StringIO.new }
 
       before do
-        file.stubs(:read).returns contents
-        TopicEmbed.stubs(:open)
-          .with('http://example.com/hello%20world', allow_redirections: :safe).returns file
+        stub_request(:head, url)
+        stub_request(:get, url).to_return(body: contents).then.to_raise
       end
 
       it "doesn't throw an error" do
@@ -310,7 +312,6 @@ describe TopicEmbed do
 
     context "non-http URL" do
       let(:url) { '/test.txt' }
-
       it "throws an error" do
         expect { TopicEmbed.find_remote(url) }.to raise_error(URI::InvalidURIError)
       end
@@ -328,6 +329,7 @@ describe TopicEmbed do
       end
 
       it "handles mailto links" do
+        stub_request(:head, url)
         response = TopicEmbed.find_remote(url)
         expect(response.body).to have_tag('a', with: { href: 'mailto:foo%40example.com' })
         expect(response.body).to have_tag('a', with: { href: 'mailto:bar@example.com' })