From 055d59373a300a1b59e1d56f1374d180119cec57 Mon Sep 17 00:00:00 2001 From: Kyle Zhao Date: Thu, 15 Nov 2018 12:14:16 -0500 Subject: [PATCH] CSP: drop 'self' in `script-src` (#6611) --- lib/content_security_policy.rb | 50 ++++++++++++++++--- spec/lib/content_security_policy_spec.rb | 52 ++++++++++++++------ spec/requests/application_controller_spec.rb | 1 - 3 files changed, 79 insertions(+), 24 deletions(-) diff --git a/lib/content_security_policy.rb b/lib/content_security_policy.rb index 53e5e3485cf..dc714935e3d 100644 --- a/lib/content_security_policy.rb +++ b/lib/content_security_policy.rb @@ -15,7 +15,7 @@ class ContentSecurityPolicy return response unless html_response?(headers) && ContentSecurityPolicy.enabled? - policy = ContentSecurityPolicy.new.build + policy = ContentSecurityPolicy.new(request).build headers['Content-Security-Policy'] = policy if SiteSetting.content_security_policy headers['Content-Security-Policy-Report-Only'] = policy if SiteSetting.content_security_policy_report_only @@ -33,7 +33,8 @@ class ContentSecurityPolicy SiteSetting.content_security_policy || SiteSetting.content_security_policy_report_only end - def initialize + def initialize(request = nil) + @request = request @directives = { script_src: script_src, } @@ -57,14 +58,47 @@ class ContentSecurityPolicy private - def script_src - sources = [:self, :unsafe_eval] + attr_reader :request - sources << :https if SiteSetting.force_https - sources << Discourse.asset_host if Discourse.asset_host.present? - sources << 'www.google-analytics.com' if SiteSetting.ga_universal_tracking_code.present? - sources << 'www.googletagmanager.com' if SiteSetting.gtm_container_id.present? + SCRIPT_ASSET_DIRECTORIES = [ + # [dir, can_use_s3_cdn, can_use_cdn] + ['/assets/', true, true], + ['/brotli_asset/', true, true], + ['/extra-locales/', false, false], + ['/highlight-js/', false, true], + ['/javascripts/', false, true], + ['/theme-javascripts/', false, true], + ] + + def script_assets(base = base_url, s3_cdn = GlobalSetting.s3_cdn_url, cdn = GlobalSetting.cdn_url) + SCRIPT_ASSET_DIRECTORIES.map do |dir, can_use_s3_cdn, can_use_cdn| + if can_use_s3_cdn && s3_cdn + s3_cdn + dir + elsif can_use_cdn && cdn + cdn + dir + else + base + dir + end + end + end + + def script_src + sources = [ + :unsafe_eval, + "#{base_url}/logs/", + "#{base_url}/sidekiq/", + "#{base_url}/mini-profiler-resources/", + ] + + sources.concat(script_assets) + + sources << 'https://www.google-analytics.com' if SiteSetting.ga_universal_tracking_code.present? + sources << 'https://www.googletagmanager.com' if SiteSetting.gtm_container_id.present? sources.concat(SiteSetting.content_security_policy_script_src.split('|')) end + + def base_url + @base_url ||= Rails.env.development? ? request.host_with_port : Discourse.base_url + end end diff --git a/spec/lib/content_security_policy_spec.rb b/spec/lib/content_security_policy_spec.rb index dd26fb2d94d..a73082ac7df 100644 --- a/spec/lib/content_security_policy_spec.rb +++ b/spec/lib/content_security_policy_spec.rb @@ -14,16 +14,20 @@ describe ContentSecurityPolicy do end describe 'script-src defaults' do - it 'always have self and unsafe-eval' do + it 'always have self, logster, sidekiq, and assets' do script_srcs = parse(ContentSecurityPolicy.new.build)['script-src'] - expect(script_srcs).to eq(%w['self' 'unsafe-eval']) - end - - it 'enforces https when SiteSetting.force_https' do - SiteSetting.force_https = true - - script_srcs = parse(ContentSecurityPolicy.new.build)['script-src'] - expect(script_srcs).to include('https:') + expect(script_srcs).to eq(%w[ + 'unsafe-eval' + http://test.localhost/logs/ + http://test.localhost/sidekiq/ + http://test.localhost/mini-profiler-resources/ + http://test.localhost/assets/ + http://test.localhost/brotli_asset/ + http://test.localhost/extra-locales/ + http://test.localhost/highlight-js/ + http://test.localhost/javascripts/ + http://test.localhost/theme-javascripts/ + ]) end it 'whitelists Google Analytics and Tag Manager when integrated' do @@ -31,15 +35,34 @@ describe ContentSecurityPolicy do SiteSetting.gtm_container_id = 'GTM-ABCDEF' script_srcs = parse(ContentSecurityPolicy.new.build)['script-src'] - expect(script_srcs).to include('www.google-analytics.com') - expect(script_srcs).to include('www.googletagmanager.com') + expect(script_srcs).to include('https://www.google-analytics.com') + expect(script_srcs).to include('https://www.googletagmanager.com') end - it 'whitelists CDN when integrated' do - set_cdn_url('cdn.com') + it 'whitelists CDN assets when integrated' do + set_cdn_url('https://cdn.com') script_srcs = parse(ContentSecurityPolicy.new.build)['script-src'] - expect(script_srcs).to include('cdn.com') + expect(script_srcs).to include(*%w[ + https://cdn.com/assets/ + https://cdn.com/brotli_asset/ + https://cdn.com/highlight-js/ + https://cdn.com/javascripts/ + https://cdn.com/theme-javascripts/ + http://test.localhost/extra-locales/ + ]) + + global_setting(:s3_cdn_url, 'https://s3-cdn.com') + + script_srcs = parse(ContentSecurityPolicy.new.build)['script-src'] + expect(script_srcs).to include(*%w[ + https://s3-cdn.com/assets/ + https://s3-cdn.com/brotli_asset/ + https://cdn.com/highlight-js/ + https://cdn.com/javascripts/ + https://cdn.com/theme-javascripts/ + http://test.localhost/extra-locales/ + ]) end it 'can be extended with more sources' do @@ -48,7 +71,6 @@ describe ContentSecurityPolicy do expect(script_srcs).to include('example.com') expect(script_srcs).to include('another.com') expect(script_srcs).to include("'unsafe-eval'") - expect(script_srcs).to include("'self'") end end diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index 30d2a45e8ba..1370518683e 100644 --- a/spec/requests/application_controller_spec.rb +++ b/spec/requests/application_controller_spec.rb @@ -259,7 +259,6 @@ RSpec.describe ApplicationController do script_src = parse(response.headers['Content-Security-Policy'])['script-src'] expect(script_src).to include('example.com') - expect(script_src).to include("'self'") expect(script_src).to include("'unsafe-eval'") end