From e12b00eab74674443d09ecf1292be8c9a921a618 Mon Sep 17 00:00:00 2001 From: Rafael dos Santos Silva Date: Fri, 16 Jul 2021 15:25:49 -0300 Subject: [PATCH] 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 | 24 ++++++------------------ app/models/embeddable_host.rb | 2 ++ config/site_settings.yml | 2 +- spec/components/topic_retriever_spec.rb | 15 +++++++++++++-- spec/requests/embed_controller_spec.rb | 12 ++---------- 5 files changed, 24 insertions(+), 31 deletions(-) diff --git a/app/controllers/embed_controller.rb b/app/controllers/embed_controller.rb index 2a063adaed6..490ff8c698c 100644 --- a/app/controllers/embed_controller.rb +++ b/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 diff --git a/app/models/embeddable_host.rb b/app/models/embeddable_host.rb index e76dbce6436..212348bc3d1 100644 --- a/app/models/embeddable_host.rb +++ b/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? diff --git a/config/site_settings.yml b/config/site_settings.yml index 950cff638c9..e9855cdf535 100644 --- a/config/site_settings.yml +++ b/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: "" diff --git a/spec/components/topic_retriever_spec.rb b/spec/components/topic_retriever_spec.rb index e14379f5cce..c16be0c9878 100644 --- a/spec/components/topic_retriever_spec.rb +++ b/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 diff --git a/spec/requests/embed_controller_spec.rb b/spec/requests/embed_controller_spec.rb index ff89614634f..44b21a9c63c 100644 --- a/spec/requests/embed_controller_spec.rb +++ b/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