From 92d357f91aec5248d935af1761521ada798506a0 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Thu, 7 Mar 2024 15:20:31 +0000 Subject: [PATCH] FEATURE: Enable strict-dynamic Content-Security-Policy by default (#26051) Ref https://meta.discourse.org/t/298172 and https://meta.discourse.org/t/295603 --- config/locales/server.en.yml | 4 +-- config/site_settings.yml | 2 +- lib/content_security_policy/builder.rb | 3 +- .../content_security_policy_spec.rb | 14 ++++++-- spec/lib/content_security_policy_spec.rb | 34 +++++++++++++------ spec/requests/application_controller_spec.rb | 6 ++-- 6 files changed, 43 insertions(+), 20 deletions(-) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 5c8fc5e1b6c..dac1b83cdbd 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1746,8 +1746,8 @@ en: content_security_policy_report_only: "Enable Content-Security-Policy-Report-Only (CSP)" 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. (CSP)" - content_security_policy_strict_dynamic: "EXPERIMENTAL: Use a strict-dynamic content security policy. This is more modern and more flexible than our default CSP configuration. This is fully functional, but not yet tested with all themes and plugins." + 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. (CSP). Host sources will be ignored when content_security_policy_strict_dynamic is enabled." + content_security_policy_strict_dynamic: "Use a strict-dynamic content security policy to reduce the need for manual configuration. Check the announcement for more information. The ability to disable this setting will be removed soon." 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." include_secure_categories_in_tag_counts: "When enabled, count of topics for a tag will include topics that are in read restricted categories for all users. When disabled, normal users are only shown a count of topics for a tag where all the topics are in public categories." display_personal_messages_tag_counts: "When enabled, count of personal messages tagged with a given tag will be displayed." diff --git a/config/site_settings.yml b/config/site_settings.yml index deeee1a87c0..b5c4849063c 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1992,7 +1992,7 @@ security: type: simple_list default: "" content_security_policy_strict_dynamic: - default: false + default: true invalidate_inactive_admin_email_after_days: default: 365 min: 0 diff --git a/lib/content_security_policy/builder.rb b/lib/content_security_policy/builder.rb index c9a898a1cf5..d7776bfafc9 100644 --- a/lib/content_security_policy/builder.rb +++ b/lib/content_security_policy/builder.rb @@ -75,7 +75,8 @@ class ContentSecurityPolicy sources = Array(sources).map { |s| normalize_source(s) } - if SiteSetting.content_security_policy_strict_dynamic + if SiteSetting.content_security_policy_strict_dynamic && + %w[script-src worker-src].include?(directive.to_s) # Strip any sources which are ignored under strict-dynamic # If/when we make strict-dynamic the only option, we could print deprecation warnings # asking plugin/theme authors to remove the unnecessary config diff --git a/spec/integration/content_security_policy_spec.rb b/spec/integration/content_security_policy_spec.rb index 658a50bb7a6..9a057dcb553 100644 --- a/spec/integration/content_security_policy_spec.rb +++ b/spec/integration/content_security_policy_spec.rb @@ -2,6 +2,8 @@ RSpec.describe "content security policy integration" do it "adds the csp headers correctly" do + Fabricate(:admin) # to avoid 'new installation' screen + SiteSetting.content_security_policy = false get "/" expect(response.headers["Content-Security-Policy"]).to eq(nil) @@ -9,9 +11,15 @@ RSpec.describe "content security policy integration" do SiteSetting.content_security_policy = true get "/" expect(response.headers["Content-Security-Policy"]).to be_present + + expect(response.headers["Content-Security-Policy"]).to match( + /script-src 'nonce-[^']+' 'strict-dynamic';/, + ) end - context "with different hostnames" do + context "with different hostnames - legacy" do + before { SiteSetting.content_security_policy_strict_dynamic = false } + before do SiteSetting.content_security_policy = true RailsMultisite::ConnectionManagement.stubs(:current_db_hostnames).returns( @@ -39,7 +47,9 @@ RSpec.describe "content security policy integration" do end end - context "with different protocols" do + context "with different protocols - legacy" do + before { SiteSetting.content_security_policy_strict_dynamic = false } + it "forces https when the site setting is enabled" do SiteSetting.force_https = true get "/" diff --git a/spec/lib/content_security_policy_spec.rb b/spec/lib/content_security_policy_spec.rb index f542340f12e..c9e7bfb703d 100644 --- a/spec/lib/content_security_policy_spec.rb +++ b/spec/lib/content_security_policy_spec.rb @@ -40,7 +40,8 @@ RSpec.describe ContentSecurityPolicy do end end - describe "worker-src" do + describe "legacy worker-src" do + before { SiteSetting.content_security_policy_strict_dynamic = false } it "has expected values" do worker_srcs = parse(policy)["worker-src"] expect(worker_srcs).to eq( @@ -54,7 +55,8 @@ RSpec.describe ContentSecurityPolicy do end end - describe "script-src" do + describe "legacy script-src" do + before { SiteSetting.content_security_policy_strict_dynamic = false } it "always has self, logster, sidekiq, and assets" do script_srcs = parse(policy)["script-src"] expect(script_srcs).to include( @@ -185,6 +187,18 @@ RSpec.describe ContentSecurityPolicy do end end + describe "strict-dynamic script-src and worker-src" do + it "includes strict-dynamic keyword" do + script_srcs = parse(policy)["script-src"] + expect(script_srcs).to include("'strict-dynamic'") + end + + it "does not set worker-src" do + worker_src = parse(policy)["worker-src"] + expect(worker_src).to eq(nil) + end + end + describe "manifest-src" do it "is set to self" do expect(parse(policy)["manifest-src"]).to eq(["'self'"]) @@ -231,7 +245,9 @@ RSpec.describe ContentSecurityPolicy do end end - it "can extend script-src, object-src, manifest-src" do + it "can extend script-src, object-src, manifest-src - legacy" do + SiteSetting.content_security_policy_strict_dynamic = false + plugin = plugin_class.new(nil, "#{Rails.root}/spec/fixtures/plugins/csp_extension/plugin.rb") plugin.activate! @@ -272,13 +288,9 @@ RSpec.describe ContentSecurityPolicy do end end - it "only includes unsafe-inline for qunit paths" do - expect(parse(policy(path_info: "/qunit"))["script-src"]).to include("'unsafe-eval'") - expect(parse(policy(path_info: "/wizard/qunit"))["script-src"]).to include("'unsafe-eval'") - expect(parse(policy(path_info: "/"))["script-src"]).to_not include("'unsafe-eval'") - end + context "with a theme - legacy" do + before { SiteSetting.content_security_policy_strict_dynamic = false } - context "with a theme" do let!(:theme) do Fabricate(:theme).tap do |t| settings = <<~YML @@ -379,9 +391,9 @@ RSpec.describe ContentSecurityPolicy do end it "can be extended by site setting" do - SiteSetting.content_security_policy_script_src = "from-site-setting.com|from-site-setting.net" + SiteSetting.content_security_policy_script_src = "'unsafe-eval'" - expect(parse(policy)["script-src"]).to include("from-site-setting.com", "from-site-setting.net") + expect(parse(policy)["script-src"]).to include("'unsafe-eval'") end def parse(csp_string) diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index fecf261f9a1..d61c446c12c 100644 --- a/spec/requests/application_controller_spec.rb +++ b/spec/requests/application_controller_spec.rb @@ -647,14 +647,14 @@ RSpec.describe ApplicationController do get "/" script_src = parse(response.headers["Content-Security-Policy"])["script-src"] - expect(script_src).to_not include("example.com") + expect(script_src).to_not include("'unsafe-inline'") - SiteSetting.content_security_policy_script_src = "example.com" + SiteSetting.content_security_policy_script_src = "'unsafe-inline'" get "/" script_src = parse(response.headers["Content-Security-Policy"])["script-src"] - expect(script_src).to include("example.com") + expect(script_src).to include("'unsafe-inline'") end it "does not set CSP when responding to non-HTML" do