FEATURE: Stop checking referer for embeds (#13756)

Flips content_security_policy_frame_ancestors default to enabled, and
removes HTTP_REFERER checks on embed requests, as the new referer
privacy options made the check fragile.

app/controllers/embed_controller.rb

@@ -5,14 +5,12 @@ class EmbedController < ApplicationController
 
   skip_before_action :check_xhr, :preload_json, :verify_authenticity_token
 
-  before_action :ensure_embeddable, except: [ :info, :topics ]
   before_action :prepare_embeddable, except: [ :info ]
   before_action :ensure_api_request, only: [ :info ]
 
   layout 'embed'
 
   rescue_from Discourse::InvalidAccess do
-    response.headers.delete('X-Frame-Options')
     if current_user.try(:admin?)
       @setup_url = "#{Discourse.base_url}/admin/customize/embedding"
       @show_reason = true
@@ -24,7 +22,6 @@ class EmbedController < ApplicationController
   def topics
     discourse_expires_in 1.minute
 
-    response.headers.delete('X-Frame-Options')
     unless SiteSetting.embed_topics_list?
       render 'embed_topics_error', status: 400
       return
@@ -73,6 +70,11 @@ class EmbedController < ApplicationController
   def comments
     embed_url = params[:embed_url]
     embed_username = params[:discourse_username]
+    embed_topic_id = params[:topic_id]&.to_i
+
+    unless embed_topic_id || EmbeddableHost.url_allowed?(embed_url)
+      raise Discourse::InvalidAccess.new('invalid embed host')
+    end
 
     topic_id = nil
     if embed_url.present?
@@ -147,6 +149,7 @@ class EmbedController < ApplicationController
   private
 
   def prepare_embeddable
+    response.headers.delete('X-Frame-Options')
     @embeddable_css_class = ""
     embeddable_host = EmbeddableHost.record_for_url(request.referer)
     @embeddable_css_class = " class=\"#{embeddable_host.class_name}\"" if embeddable_host.present? && embeddable_host.class_name.present?
@@ -158,19 +161,4 @@ class EmbedController < ApplicationController
   def ensure_api_request
     raise Discourse::InvalidAccess.new('api key not set') if !is_api?
   end
-
-  def ensure_embeddable
-    if !(Rails.env.development? && current_user&.admin?)
-      referer = request.referer
-
-      unless referer && EmbeddableHost.url_allowed?(referer)
-        raise Discourse::InvalidAccess.new('invalid referer host')
-      end
-    end
-
-    response.headers.delete('X-Frame-Options')
-  rescue URI::Error
-    raise Discourse::InvalidAccess.new('invalid referer host')
-  end
-
 end

app/models/embeddable_host.rb

@@ -44,6 +44,8 @@ class EmbeddableHost < ActiveRecord::Base
   end
 
   def self.url_allowed?(url)
+    return false if url.nil?
+
     # Work around IFRAME reload on WebKit where the referer will be set to the Forum URL
     return true if url&.starts_with?(Discourse.base_url) && EmbeddableHost.exists?
 

config/site_settings.yml

@@ -1640,7 +1640,7 @@ security:
   content_security_policy_collect_reports:
     default: false
   content_security_policy_frame_ancestors:
-    default: false
+    default: true
   content_security_policy_script_src:
     type: simple_list
     default: ""

spec/components/topic_retriever_spec.rb

@@ -36,9 +36,9 @@ describe TopicRetriever do
       end
     end
 
-    context "when host is not invalid" do
+    context "when host is valid" do
       before do
-        topic_retriever.stubs(:invalid_url?).returns(false)
+        Fabricate(:embeddable_host, host: 'http://eviltrout.com/')
       end
 
       context "when topics have been retrieved recently" do
@@ -64,6 +64,17 @@ describe TopicRetriever do
       end
     end
 
+    context "when host is invalid" do
+      before do
+        Fabricate(:embeddable_host, host: 'http://not-eviltrout.com/')
+      end
+
+      it "does not perform_retrieve" do
+        topic_retriever.expects(:perform_retrieve).never
+        topic_retriever.retrieve
+      end
+    end
+
     it "works with URLs with whitespaces" do
       expect { TopicRetriever.new(" https://example.com ").retrieve }
         .not_to raise_error

spec/requests/embed_controller_spec.rb

@@ -150,9 +150,9 @@ describe EmbedController do
       Jobs.run_immediately!
     end
 
-    it "raises an error with no referer" do
+    it "doesn't raises an error with no referer" do
       get '/embed/comments', params: { embed_url: embed_url }
-      expect(response.body).to match(I18n.t('embed.error'))
+      expect(response.body).not_to match(I18n.t('embed.error'))
     end
 
     it "includes CSS from embedded_scss field" do
@@ -266,14 +266,6 @@ describe EmbedController do
 
         expect(response.body).to match('class="example"')
       end
-
-      it "doesn't work with a made up host" do
-        get '/embed/comments',
-          params: { embed_url: embed_url },
-          headers: { 'REFERER' => "http://codinghorror.com/invalid-url" }
-
-        expect(response.body).to match(I18n.t('embed.error'))
-      end
     end
 
     context "CSP frame-ancestors enabled" do