From fb4486d5f1a8d40cd8dd8f3697fc0c3687300faf Mon Sep 17 00:00:00 2001 From: Rafael dos Santos Silva Date: Mon, 22 Mar 2021 16:00:25 -0300 Subject: [PATCH] FEATURE: Add CSP frame-ancestors support (#12404) --- app/controllers/embed_controller.rb | 6 ++--- config/locales/server.en.yml | 1 + config/site_settings.yml | 2 ++ lib/content_security_policy/default.rb | 13 ++++++++++ spec/lib/content_security_policy_spec.rb | 33 ++++++++++++++++++++++++ spec/requests/embed_controller_spec.rb | 20 ++++++++++++-- 6 files changed, 70 insertions(+), 5 deletions(-) diff --git a/app/controllers/embed_controller.rb b/app/controllers/embed_controller.rb index fd4702ba3a7..fc5ae4756e7 100644 --- a/app/controllers/embed_controller.rb +++ b/app/controllers/embed_controller.rb @@ -12,7 +12,7 @@ class EmbedController < ApplicationController layout 'embed' rescue_from Discourse::InvalidAccess do - response.headers['X-Frame-Options'] = "ALLOWALL" + response.headers.delete('X-Frame-Options') if current_user.try(:admin?) @setup_url = "#{Discourse.base_url}/admin/customize/embedding" @show_reason = true @@ -24,7 +24,7 @@ class EmbedController < ApplicationController def topics discourse_expires_in 1.minute - response.headers['X-Frame-Options'] = "ALLOWALL" + response.headers.delete('X-Frame-Options') unless SiteSetting.embed_topics_list? render 'embed_topics_error', status: 400 return @@ -157,7 +157,7 @@ class EmbedController < ApplicationController end end - response.headers['X-Frame-Options'] = "ALLOWALL" + response.headers.delete('X-Frame-Options') rescue URI::Error raise Discourse::InvalidAccess.new('invalid referer host') end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index fbed6eee272..cb3a7ca3e1e 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1602,6 +1602,7 @@ en: content_security_policy: "Enable Content-Security-Policy" content_security_policy_report_only: "Enable Content-Security-Policy-Report-Only" content_security_policy_collect_reports: "Enable CSP violation report collection at /csp_reports" + content_security_policy_frame_ancestors: "Restrict who can embed this site in iframes via CSP. Control allowed hosts on Embedding" content_security_policy_script_src: "Additional allowlisted script sources. The current host and CDN are included by default. See Mitigate XSS Attacks with Content Security Policy." invalidate_inactive_admin_email_after_days: "Admin accounts that have not visited the site in this number of days will need to re-validate their email address before logging in. Set to 0 to disable." top_menu: "Determine which items appear in the homepage navigation, and in what order. Example latest|new|unread|categories|top|read|posted|bookmarks" diff --git a/config/site_settings.yml b/config/site_settings.yml index c3c2801e1ff..33bd1327e4b 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1593,6 +1593,8 @@ security: default: false content_security_policy_collect_reports: default: false + content_security_policy_frame_ancestors: + default: false content_security_policy_script_src: type: simple_list default: "" diff --git a/lib/content_security_policy/default.rb b/lib/content_security_policy/default.rb index 1d5315709eb..6826de68e82 100644 --- a/lib/content_security_policy/default.rb +++ b/lib/content_security_policy/default.rb @@ -13,6 +13,7 @@ class ContentSecurityPolicy directives[:script_src] = script_src directives[:worker_src] = worker_src directives[:report_uri] = report_uri if SiteSetting.content_security_policy_collect_reports + directives[:frame_ancestors] = frame_ancestors if restrict_embed? end end @@ -73,5 +74,17 @@ class ContentSecurityPolicy def report_uri "#{base_url}/csp_reports" end + + def frame_ancestors + [ + "'self'", + *EmbeddableHost.pluck(:host).map { |host| "https://#{host}" } + ] + end + + def restrict_embed? + SiteSetting.content_security_policy_frame_ancestors && + !SiteSetting.embed_any_origin + end end end diff --git a/spec/lib/content_security_policy_spec.rb b/spec/lib/content_security_policy_spec.rb index f4a73c6bbc9..3f7b6ac12e3 100644 --- a/spec/lib/content_security_policy_spec.rb +++ b/spec/lib/content_security_policy_spec.rb @@ -154,6 +154,39 @@ describe ContentSecurityPolicy do end end + describe 'frame-ancestors' do + context 'with content_security_policy_frame_ancestors enabled' do + before do + SiteSetting.content_security_policy_frame_ancestors = true + Fabricate(:embeddable_host, host: 'https://a.org') + Fabricate(:embeddable_host, host: 'https://b.org') + end + + it 'always has self' do + frame_ancestors = parse(policy)['frame-ancestors'] + expect(frame_ancestors).to include("'self'") + end + + it 'includes all EmbeddableHost' do + EmbeddableHost + frame_ancestors = parse(policy)['frame-ancestors'] + expect(frame_ancestors).to include("https://a.org") + expect(frame_ancestors).to include("https://b.org") + end + end + + context 'with content_security_policy_frame_ancestors disabled' do + before do + SiteSetting.content_security_policy_frame_ancestors = false + end + + it 'does not set frame-ancestors' do + frame_ancestors = parse(policy)['frame-ancestors'] + expect(frame_ancestors).to be_nil + end + end + end + it 'can be extended by plugins' do plugin = Class.new(Plugin::Instance) do attr_accessor :enabled diff --git a/spec/requests/embed_controller_spec.rb b/spec/requests/embed_controller_spec.rb index 62e42b1d819..9d476ed3a1c 100644 --- a/spec/requests/embed_controller_spec.rb +++ b/spec/requests/embed_controller_spec.rb @@ -94,7 +94,7 @@ describe EmbedController do 'REFERER' => 'https://example.com/evil-trout' } expect(response.status).to eq(200) - expect(response.headers['X-Frame-Options']).to eq("ALLOWALL") + expect(response.headers['X-Frame-Options']).to be_nil expect(response.body).to match("data-embed-id=\"de-1234\"") expect(response.body).to match("data-topic-id=\"#{topic.id}\"") expect(response.body).to match("data-referer=\"https://example.com/evil-trout\"") @@ -157,7 +157,7 @@ describe EmbedController do context "success" do after do expect(response.status).to eq(200) - expect(response.headers['X-Frame-Options']).to eq("ALLOWALL") + expect(response.headers['X-Frame-Options']).to be_nil end it "tells the topic retriever to work when no previous embed is found" do @@ -249,5 +249,21 @@ describe EmbedController do expect(response.body).to match(I18n.t('embed.error')) end end + + context "CSP frame-ancestors enabled" do + before do + SiteSetting.content_security_policy_frame_ancestors = true + end + + it "includes all the hosts" do + get '/embed/comments', + params: { embed_url: embed_url }, + headers: { 'REFERER' => "http://eviltrout.com/wat/1-2-3.html" } + + expect(response.headers['Content-Security-Policy']).to match(/frame-ancestors.*https:\/\/discourse\.org/) + expect(response.headers['Content-Security-Policy']).to match(/frame-ancestors.*https:\/\/example\.com/) + end + end + end end