From 745d1de40c897d99ff0ae40f62a8a1dbbbb2140f 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 951e2c9d37..ee61d64e52 100644
--- a/app/models/topic_embed.rb
+++ b/app/models/topic_embed.rb
@@ -105,7 +105,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 2d0c4037ef..9e29fff67c 100644
--- a/spec/models/topic_embed_spec.rb
+++ b/spec/models/topic_embed_spec.rb
@@ -149,6 +149,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
@@ -177,6 +178,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
 
@@ -213,6 +215,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
 
@@ -235,6 +238,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
 
@@ -262,9 +266,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
@@ -280,9 +283,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
@@ -293,7 +295,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
@@ -311,6 +312,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' })