From 705c898c2122ec7eff35575f12fa401f69821648 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Mon, 11 Feb 2019 12:32:04 +0000 Subject: [PATCH] FEATURE: Calculate CSP based on active themes (#6976) --- lib/content_security_policy.rb | 8 ++-- lib/content_security_policy/extension.rb | 11 ++--- lib/content_security_policy/middleware.rb | 6 +-- spec/lib/content_security_policy_spec.rb | 53 ++++++++++++++--------- 4 files changed, 45 insertions(+), 33 deletions(-) diff --git a/lib/content_security_policy.rb b/lib/content_security_policy.rb index 2f2b4c87a1b..fd38fd543ac 100644 --- a/lib/content_security_policy.rb +++ b/lib/content_security_policy.rb @@ -4,8 +4,8 @@ require_dependency 'content_security_policy/extension' class ContentSecurityPolicy class << self - def policy - new.build + def policy(theme_ids = []) + new.build(theme_ids) end def base_url @@ -14,10 +14,10 @@ class ContentSecurityPolicy attr_writer :base_url end - def build + def build(theme_ids) builder = Builder.new - Extension.theme_extensions.each { |extension| builder << extension } + Extension.theme_extensions(theme_ids).each { |extension| builder << extension } Extension.plugin_extensions.each { |extension| builder << extension } builder << Extension.site_setting_extension diff --git a/lib/content_security_policy/extension.rb b/lib/content_security_policy/extension.rb index cc319861558..60917888e58 100644 --- a/lib/content_security_policy/extension.rb +++ b/lib/content_security_policy/extension.rb @@ -17,12 +17,13 @@ class ContentSecurityPolicy THEME_SETTING = 'extend_content_security_policy' - def theme_extensions - cache['theme_extensions'] ||= find_theme_extensions + def theme_extensions(theme_ids) + key = "theme_extensions_#{Theme.transform_ids(theme_ids).join(',')}" + cache[key] ||= find_theme_extensions(theme_ids) end def clear_theme_extensions_cache! - cache['theme_extensions'] = nil + cache.clear end private @@ -31,10 +32,10 @@ class ContentSecurityPolicy @cache ||= DistributedCache.new('csp_extensions') end - def find_theme_extensions + def find_theme_extensions(theme_ids) extensions = [] - Theme.find_each do |theme| + Theme.where(id: Theme.transform_ids(theme_ids)).find_each do |theme| theme.cached_settings.each do |setting, value| extensions << build_theme_extension(value) if setting.to_s == THEME_SETTING end diff --git a/lib/content_security_policy/middleware.rb b/lib/content_security_policy/middleware.rb index d407ce0146b..d47babe35ad 100644 --- a/lib/content_security_policy/middleware.rb +++ b/lib/content_security_policy/middleware.rb @@ -12,11 +12,11 @@ class ContentSecurityPolicy _, headers, _ = response = @app.call(env) return response unless html_response?(headers) - ContentSecurityPolicy.base_url = request.host_with_port if Rails.env.development? - headers['Content-Security-Policy'] = policy if SiteSetting.content_security_policy - headers['Content-Security-Policy-Report-Only'] = policy if SiteSetting.content_security_policy_report_only + theme_ids = env[:resolved_theme_ids] + headers['Content-Security-Policy'] = policy(theme_ids) if SiteSetting.content_security_policy + headers['Content-Security-Policy-Report-Only'] = policy(theme_ids) if SiteSetting.content_security_policy_report_only response end diff --git a/spec/lib/content_security_policy_spec.rb b/spec/lib/content_security_policy_spec.rb index 9795698864b..df0b66a1d0b 100644 --- a/spec/lib/content_security_policy_spec.rb +++ b/spec/lib/content_security_policy_spec.rb @@ -120,31 +120,42 @@ describe ContentSecurityPolicy do Discourse.plugins.pop end - it 'can be extended by themes' do - policy # call this first to make sure further actions clear the cache + context "with a theme" do + let!(:theme) { + Fabricate(:theme).tap do |t| + settings = <<~YML + extend_content_security_policy: + type: list + default: 'script-src: from-theme.com' + YML + t.set_field(target: :settings, name: :yaml, value: settings) + t.save! + end + } - theme = Fabricate(:theme) - settings = <<~YML - extend_content_security_policy: - type: list - default: 'script-src: from-theme.com' - YML - theme.set_field(target: :settings, name: :yaml, value: settings) - theme.save! + def theme_policy + policy([theme.id]) + end - expect(parse(policy)['script-src']).to include('from-theme.com') + it 'can be extended by themes' do + policy # call this first to make sure further actions clear the cache - theme.update_setting(:extend_content_security_policy, "script-src: https://from-theme.net|worker-src: from-theme.com") - theme.save! + expect(parse(policy)['script-src']).not_to include('from-theme.com') - expect(parse(policy)['script-src']).to_not include('from-theme.com') - expect(parse(policy)['script-src']).to include('https://from-theme.net') - expect(parse(policy)['worker-src']).to include('from-theme.com') + expect(parse(theme_policy)['script-src']).to include('from-theme.com') - theme.destroy! + theme.update_setting(:extend_content_security_policy, "script-src: https://from-theme.net|worker-src: from-theme.com") + theme.save! - expect(parse(policy)['script-src']).to_not include('https://from-theme.net') - expect(parse(policy)['worker-src']).to_not include('from-theme.com') + expect(parse(theme_policy)['script-src']).to_not include('from-theme.com') + expect(parse(theme_policy)['script-src']).to include('https://from-theme.net') + expect(parse(theme_policy)['worker-src']).to include('from-theme.com') + + theme.destroy! + + expect(parse(theme_policy)['script-src']).to_not include('https://from-theme.net') + expect(parse(theme_policy)['worker-src']).to_not include('from-theme.com') + end end it 'can be extended by site setting' do @@ -160,7 +171,7 @@ describe ContentSecurityPolicy do end.to_h end - def policy - ContentSecurityPolicy.policy + def policy(theme_ids = []) + ContentSecurityPolicy.policy(theme_ids) end end