From 2393234be5a4ebafde33ffc1218586514c1db613 Mon Sep 17 00:00:00 2001 From: Kelv Date: Tue, 18 Jun 2024 16:40:53 +0800 Subject: [PATCH] DEV: remove legacy CSP implementation to make strict-dynamic only accepted behaviour (#27486) * DEV: remove legacy CSP implementation that allowed for non-strict-dynamic behaviour --- app/assets/javascripts/custom-proxy/index.js | 15 - config/locales/server.en.yml | 1 - config/site_settings.yml | 2 - lib/content_security_policy/builder.rb | 3 +- lib/content_security_policy/default.rb | 41 +-- .../content_security_policy_spec.rb | 47 --- spec/lib/content_security_policy_spec.rb | 279 +----------------- spec/system/content_security_policy_spec.rb | 3 - 8 files changed, 9 insertions(+), 382 deletions(-) diff --git a/app/assets/javascripts/custom-proxy/index.js b/app/assets/javascripts/custom-proxy/index.js index 61a20ff817b..5e941b68b1a 100644 --- a/app/assets/javascripts/custom-proxy/index.js +++ b/app/assets/javascripts/custom-proxy/index.js @@ -173,21 +173,6 @@ async function handleRequest(proxy, baseURL, req, res, outputPath) { res.set("location", newLocation); } - const csp = response.headers.get("content-security-policy"); - if (csp && !csp.includes("'strict-dynamic'")) { - const emberCliAdditions = [ - `http://${originalHost}${baseURL}assets/`, - `http://${originalHost}${baseURL}ember-cli-live-reload.js`, - `http://${originalHost}${baseURL}_lr/`, - ].join(" "); - - const newCSP = csp - .replaceAll(proxy, `http://${originalHost}`) - .replaceAll("script-src ", `script-src ${emberCliAdditions} `); - - res.set("content-security-policy", newCSP); - } - const contentType = response.headers.get("content-type"); const isHTML = contentType?.startsWith("text/html"); diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 44c92c69451..02cae2665a0 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1844,7 +1844,6 @@ en: 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). 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 c8789cefe75..58510c4f7ee 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -2017,8 +2017,6 @@ security: content_security_policy_script_src: type: simple_list default: "" - content_security_policy_strict_dynamic: - 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 90fd8e1afc2..8696973b7ff 100644 --- a/lib/content_security_policy/builder.rb +++ b/lib/content_security_policy/builder.rb @@ -75,8 +75,7 @@ class ContentSecurityPolicy sources = Array(sources).map { |s| normalize_source(s) } - if SiteSetting.content_security_policy_strict_dynamic && - %w[script_src worker_src].include?(directive.to_s) + if %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/lib/content_security_policy/default.rb b/lib/content_security_policy/default.rb index 5a72d3a65ce..af1ddbc70d4 100644 --- a/lib/content_security_policy/default.rb +++ b/lib/content_security_policy/default.rb @@ -13,7 +13,7 @@ class ContentSecurityPolicy directives[:base_uri] = [:self] directives[:object_src] = [:none] directives[:script_src] = script_src - directives[:worker_src] = worker_src + directives[:worker_src] = [] directives[ :report_uri ] = report_uri if SiteSetting.content_security_policy_collect_reports @@ -60,49 +60,12 @@ class ContentSecurityPolicy end def script_src - sources = [] - - if SiteSetting.content_security_policy_strict_dynamic - sources << "'strict-dynamic'" - else - sources.push( - "#{base_url}/logs/", - "#{base_url}/sidekiq/", - "#{base_url}/mini-profiler-resources/", - *script_assets, - ) - - # Support Ember CLI Live reload - if Rails.env.development? - sources << "#{base_url}/ember-cli-live-reload.js" - sources << "#{base_url}/_lr/" - end - - # we need analytics.js still as gtag/js is a script wrapper for it - if SiteSetting.ga_universal_tracking_code.present? - sources << "https://www.google-analytics.com/analytics.js" - end - if SiteSetting.ga_universal_tracking_code.present? && SiteSetting.ga_version == "v4_gtag" - sources << "https://www.googletagmanager.com/gtag/js" - end - if SiteSetting.gtm_container_id.present? - sources << "https://www.googletagmanager.com/gtm.js" - end - end - + sources = ["'strict-dynamic'"] sources << :report_sample if SiteSetting.content_security_policy_collect_reports sources end - def worker_src - return [] if SiteSetting.content_security_policy_strict_dynamic - [ - "'self'", # For service worker - *script_assets(worker: true), - ] - end - def report_uri "#{base_url}/csp_reports" end diff --git a/spec/integration/content_security_policy_spec.rb b/spec/integration/content_security_policy_spec.rb index 9a057dcb553..8788a750220 100644 --- a/spec/integration/content_security_policy_spec.rb +++ b/spec/integration/content_security_policy_spec.rb @@ -16,51 +16,4 @@ RSpec.describe "content security policy integration" do /script-src 'nonce-[^']+' 'strict-dynamic';/, ) end - - 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( - %w[primary.example.com secondary.example.com], - ) - RailsMultisite::ConnectionManagement.stubs(:current_hostname).returns("primary.example.com") - end - - it "works with the primary domain" do - host! "primary.example.com" - get "/" - expect(response.headers["Content-Security-Policy"]).to include("http://primary.example.com") - end - - it "works with the secondary domain" do - host! "secondary.example.com" - get "/" - expect(response.headers["Content-Security-Policy"]).to include("http://secondary.example.com") - end - - it "uses the primary domain for unknown hosts" do - host! "unknown.example.com" - get "/" - expect(response.headers["Content-Security-Policy"]).to include("http://primary.example.com") - end - end - - 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 "/" - expect(response.headers["Content-Security-Policy"]).to include("https://test.localhost") - end - - it "uses https when the site setting is disabled, but request is ssl" do - SiteSetting.force_https = false - https! - get "/" - expect(response.headers["Content-Security-Policy"]).to include("https://test.localhost") - end - end end diff --git a/spec/lib/content_security_policy_spec.rb b/spec/lib/content_security_policy_spec.rb index d60b8e5b12f..4b7847e6776 100644 --- a/spec/lib/content_security_policy_spec.rb +++ b/spec/lib/content_security_policy_spec.rb @@ -40,153 +40,6 @@ RSpec.describe ContentSecurityPolicy do end end - 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( - %w[ - 'self' - http://test.localhost/assets/ - http://test.localhost/javascripts/ - http://test.localhost/plugins/ - ], - ) - end - end - - 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( - *%w[ - http://test.localhost/logs/ - http://test.localhost/sidekiq/ - http://test.localhost/mini-profiler-resources/ - http://test.localhost/assets/ - http://test.localhost/extra-locales/ - http://test.localhost/highlight-js/ - http://test.localhost/javascripts/ - http://test.localhost/plugins/ - http://test.localhost/theme-javascripts/ - http://test.localhost/svg-sprite/ - ], - ) - end - - it 'includes "report-sample" when report collection is enabled' do - SiteSetting.content_security_policy_collect_reports = true - script_srcs = parse(policy)["script-src"] - expect(script_srcs).to include("'report-sample'") - end - - context "for Google Analytics" do - before { SiteSetting.ga_universal_tracking_code = "UA-12345678-9" } - - it "allowlists Google Analytics v3 when integrated" do - SiteSetting.ga_version = "v3_analytics" - - script_srcs = parse(policy)["script-src"] - expect(script_srcs).to include("https://www.google-analytics.com/analytics.js") - expect(script_srcs).not_to include("https://www.googletagmanager.com/gtag/js") - end - - it "allowlists Google Analytics v4 when integrated" do - SiteSetting.ga_version = "v4_gtag" - - script_srcs = parse(policy)["script-src"] - expect(script_srcs).to include("https://www.google-analytics.com/analytics.js") - expect(script_srcs).to include("https://www.googletagmanager.com/gtag/js") - end - end - - it "allowlists Google Tag Manager when integrated" do - SiteSetting.gtm_container_id = "GTM-ABCDEF" - - script_srcs = parse(policy)["script-src"] - expect(script_srcs).to include("https://www.googletagmanager.com/gtm.js") - # nonce is added by the GtmScriptNonceInjector middleware to prevent the - # nonce from getting cached by AnonymousCache - expect(script_srcs.to_s).not_to include("nonce-") - end - - it "allowlists CDN assets when integrated" do - set_cdn_url("https://cdn.com") - - script_srcs = parse(policy)["script-src"] - expect(script_srcs).to include( - *%w[ - https://cdn.com/assets/ - https://cdn.com/highlight-js/ - https://cdn.com/javascripts/ - https://cdn.com/plugins/ - https://cdn.com/theme-javascripts/ - http://test.localhost/extra-locales/ - ], - ) - - global_setting(:s3_cdn_url, "https://s3-cdn.com") - - script_srcs = parse(policy)["script-src"] - expect(script_srcs).to include( - *%w[ - https://s3-cdn.com/assets/ - https://cdn.com/highlight-js/ - https://cdn.com/javascripts/ - https://cdn.com/plugins/ - https://cdn.com/theme-javascripts/ - http://test.localhost/extra-locales/ - ], - ) - - global_setting(:s3_asset_cdn_url, "https://s3-asset-cdn.com") - - script_srcs = parse(policy)["script-src"] - expect(script_srcs).to include( - *%w[ - https://s3-asset-cdn.com/assets/ - https://cdn.com/highlight-js/ - https://cdn.com/javascripts/ - https://cdn.com/plugins/ - https://cdn.com/theme-javascripts/ - http://test.localhost/extra-locales/ - ], - ) - end - - it "adds subfolder to CDN assets" do - set_cdn_url("https://cdn.com") - set_subfolder("/forum") - - script_srcs = parse(policy)["script-src"] - expect(script_srcs).to include( - *%w[ - https://cdn.com/forum/assets/ - https://cdn.com/forum/highlight-js/ - https://cdn.com/forum/javascripts/ - https://cdn.com/forum/plugins/ - https://cdn.com/forum/theme-javascripts/ - http://test.localhost/forum/extra-locales/ - ], - ) - - global_setting(:s3_cdn_url, "https://s3-cdn.com") - - script_srcs = parse(policy)["script-src"] - expect(script_srcs).to include( - *%w[ - https://s3-cdn.com/assets/ - https://cdn.com/forum/highlight-js/ - https://cdn.com/forum/javascripts/ - https://cdn.com/forum/plugins/ - https://cdn.com/forum/theme-javascripts/ - http://test.localhost/forum/extra-locales/ - ], - ) - end - end - describe "strict-dynamic script-src and worker-src" do it "includes strict-dynamic keyword" do script_srcs = parse(policy)["script-src"] @@ -197,6 +50,12 @@ RSpec.describe ContentSecurityPolicy do worker_src = parse(policy)["worker-src"] expect(worker_src).to eq(nil) end + + it 'includes "report-sample" when report collection is enabled' do + SiteSetting.content_security_policy_collect_reports = true + script_srcs = parse(policy)["script-src"] + expect(script_srcs).to include("'report-sample'") + end end describe "manifest-src" do @@ -245,30 +104,6 @@ RSpec.describe ContentSecurityPolicy do end end - 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! - Discourse.plugins << plugin - - plugin.enabled = true - expect(parse(policy)["script-src"]).to include("https://from-plugin.com") - expect(parse(policy)["script-src"]).to include("http://test.localhost/local/path") - expect(parse(policy)["object-src"]).to include("https://test-stripping.com") - expect(parse(policy)["object-src"]).to_not include("'none'") - expect(parse(policy)["manifest-src"]).to include("'self'") - expect(parse(policy)["manifest-src"]).to include("https://manifest-src.com") - - plugin.enabled = false - expect(parse(policy)["script-src"]).to_not include("https://from-plugin.com") - expect(parse(policy)["manifest-src"]).to_not include("https://manifest-src.com") - - Discourse.plugins.delete plugin - DiscoursePluginRegistry.reset! - end - it "can extend frame_ancestors" do SiteSetting.content_security_policy_frame_ancestors = true plugin = plugin_class.new(nil, "#{Rails.root}/spec/fixtures/plugins/csp_extension/plugin.rb") @@ -288,108 +123,6 @@ RSpec.describe ContentSecurityPolicy do end end - context "with a theme - legacy" do - before { SiteSetting.content_security_policy_strict_dynamic = false } - - let!(:theme) do - 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 - end - - def theme_policy - policy(theme.id) - end - - it "can be extended by themes" do - policy # call this first to make sure further actions clear the cache - - expect(parse(policy)["script-src"]).not_to include("from-theme.com") - - expect(parse(theme_policy)["script-src"]).to include("from-theme.com") - - theme.update_setting( - :extend_content_security_policy, - "script-src: https://from-theme.net|worker-src: from-theme.com", - ) - theme.save! - - 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 - - it "can be extended by theme modifiers" do - policy # call this first to make sure further actions clear the cache - - theme.theme_modifier_set.csp_extensions = [ - "script-src: https://from-theme-flag.script", - "worker-src: from-theme-flag.worker", - ] - theme.save! - - child_theme = Fabricate(:theme, component: true) - theme.add_relative_theme!(:child, child_theme) - child_theme.theme_modifier_set.csp_extensions = [ - "script-src: https://child-theme-flag.script", - "worker-src: child-theme-flag.worker", - ] - child_theme.save! - - expect(parse(theme_policy)["script-src"]).to include("https://from-theme-flag.script") - expect(parse(theme_policy)["script-src"]).to include("https://child-theme-flag.script") - expect(parse(theme_policy)["worker-src"]).to include("from-theme-flag.worker") - expect(parse(theme_policy)["worker-src"]).to include("child-theme-flag.worker") - - theme.destroy! - child_theme.destroy! - - expect(parse(theme_policy)["script-src"]).to_not include("https://from-theme-flag.script") - expect(parse(theme_policy)["worker-src"]).to_not include("from-theme-flag.worker") - expect(parse(theme_policy)["worker-src"]).to_not include("from-theme-flag.worker") - expect(parse(theme_policy)["worker-src"]).to_not include("child-theme-flag.worker") - end - - it "is extended automatically when themes reference external scripts" do - policy # call this first to make sure further actions clear the cache - - theme.set_field(target: :common, name: "header", value: <<~HTML) - - - - - - HTML - - theme.set_field(target: :desktop, name: "header", value: "") - theme.save! - - expect(parse(theme_policy)["script-src"]).to include("https://example.com/myscript.js") - expect(parse(theme_policy)["script-src"]).to include("https://example.com/myscript2.js") - expect(parse(theme_policy)["script-src"]).not_to include("?") - expect(parse(theme_policy)["script-src"]).to include("example2.com/protocol-less-script.js") - expect(parse(theme_policy)["script-src"]).not_to include("domain-only.com") - expect(parse(theme_policy)["script-src"]).not_to include( - a_string_matching %r{^/theme-javascripts} - ) - - theme.destroy! - - expect(parse(theme_policy)["script-src"]).to_not include("https://example.com/myscript.js") - end - end - it "can be extended by site setting" do SiteSetting.content_security_policy_script_src = "'unsafe-eval'" diff --git a/spec/system/content_security_policy_spec.rb b/spec/system/content_security_policy_spec.rb index 935a6aba911..52b41711240 100644 --- a/spec/system/content_security_policy_spec.rb +++ b/spec/system/content_security_policy_spec.rb @@ -3,7 +3,6 @@ describe "Content security policy", type: :system do it "can boot the application in strict_dynamic mode" do expect(SiteSetting.content_security_policy).to eq(true) - SiteSetting.content_security_policy_strict_dynamic = true visit "/" expect(page).to have_css("#site-logo") @@ -11,7 +10,6 @@ describe "Content security policy", type: :system do it "works for 'public exceptions' like RoutingError" do expect(SiteSetting.content_security_policy).to eq(true) - SiteSetting.content_security_policy_strict_dynamic = true SiteSetting.bootstrap_error_pages = true get "/nonexistent" @@ -25,7 +23,6 @@ describe "Content security policy", type: :system do it "can boot logster in strict_dynamic mode" do expect(SiteSetting.content_security_policy).to eq(true) sign_in Fabricate(:admin) - SiteSetting.content_security_policy_strict_dynamic = true visit "/logs" expect(page).to have_css("#log-table")