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

@@ -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");
 

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 <a href='%{base_path}/admin/customize/embedding'>Embedding</a>"
     content_security_policy_script_src: "Additional allowlisted script sources. The current host and CDN are included by default. See <a href='https://meta.discourse.org/t/mitigate-xss-attacks-with-content-security-policy/104243' target='_blank'>Mitigate XSS Attacks with Content Security Policy.</a> (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 <a href='https://meta.discourse.org/t/298172' target='_blank'>the announcement</a> 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."

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

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 &&
+      if %w[script_src worker_src].include?(directive.to_s)
-           %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

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 = []
+      sources = ["'strict-dynamic'"]
-
-      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 << :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

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

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)
-        <script src='https://example.com/myscript.js'></script>
-        <script src='https://example.com/myscript2.js?with=query'></script>
-        <script src='//example2.com/protocol-less-script.js'></script>
-        <script src='domain-only.com'></script>
-        <script>console.log('inline script')</script>
-      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'"
 

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")