From b9363494d4e87d60ffbd6b419452134f6a728916 Mon Sep 17 00:00:00 2001 From: Kelv Date: Tue, 11 Feb 2025 11:51:01 +0800 Subject: [PATCH] FIX: invalid CSP directive sources should allow site to boot with valid CSP directives (stable) (#31270) [Security patch](https://github.com/rails/rails/commit/5558e72f22fc69c1c407b31ac5fb3b4ce087b542) (for this [CVE](https://nvd.nist.gov/vuln/detail/CVE-2024-54133)) from rails actionpack was backported from [Rails 8.0.0.1](https://github.com/rails/rails/blob/v8.0.1/actionpack/CHANGELOG.md#rails-8001-december-10-2024) to previous stable versions including `7-1-stable` / `7-2-stable`. Any previous version of Discourse upgrading to v3.4.0.beta3 and above would have observed their sites crashing if they had invalid sources in their CSP directive extensions. This fix removes such invalid sources during our build of the CSP, and logs these at a warning level so devs are able to find out why their CSP sources were filtered out of the extendable directives. --- lib/content_security_policy/builder.rb | 13 ++++++++- spec/fixtures/plugins/csp_extension/plugin.rb | 6 +++- .../content_security_policy/builder_spec.rb | 7 +++++ spec/lib/content_security_policy_spec.rb | 1 + spec/system/content_security_policy_spec.rb | 29 +++++++++++++++++-- 5 files changed, 52 insertions(+), 4 deletions(-) diff --git a/lib/content_security_policy/builder.rb b/lib/content_security_policy/builder.rb index 8696973b7ff..83f56855872 100644 --- a/lib/content_security_policy/builder.rb +++ b/lib/content_security_policy/builder.rb @@ -83,7 +83,18 @@ class ContentSecurityPolicy sources.reject { |s| s == "'unsafe-inline'" || s == "'self'" || !s.start_with?("'") } end - @directives[directive].concat(sources) + sources.each do |source| + # mirrors validation check in ActionDispatch::ContentSecurityPolicy https://github.com/rails/rails/blob/5558e72f22fc69c1c407b31ac5fb3b4ce087b542/actionpack/lib/action_dispatch/http/content_security_policy.rb#L337 + if source.include?(";") || source != source.gsub(/[[:space:]]/, "") + Rails.logger.warn(<<~MSG.squish) + Skipping invalid Content Security Policy #{directive}: "#{source}". + Directive values must not contain whitespace or semicolons. + Please use multiple arguments or other directive methods instead. + MSG + next + end + @directives[directive] << source + end @directives[directive].delete(:none) if @directives[directive].count > 1 end diff --git a/spec/fixtures/plugins/csp_extension/plugin.rb b/spec/fixtures/plugins/csp_extension/plugin.rb index 0991754eb60..a8ff40cbaa4 100644 --- a/spec/fixtures/plugins/csp_extension/plugin.rb +++ b/spec/fixtures/plugins/csp_extension/plugin.rb @@ -6,7 +6,11 @@ # authors: xrav3nz extend_content_security_policy( - script_src: %w[https://from-plugin.com /local/path], + script_src: [ + "https://from-plugin.com", + "/local/path", + "'unsafe-eval' https://invalid.example.com", + ], object_src: ["https://test-stripping.com"], frame_ancestors: ["https://frame-ancestors-plugin.ext"], manifest_src: ["https://manifest-src.com"], diff --git a/spec/lib/content_security_policy/builder_spec.rb b/spec/lib/content_security_policy/builder_spec.rb index 27582efe20f..7da1ce9b323 100644 --- a/spec/lib/content_security_policy/builder_spec.rb +++ b/spec/lib/content_security_policy/builder_spec.rb @@ -24,6 +24,13 @@ RSpec.describe ContentSecurityPolicy::Builder do expect(builder.build).to_not include("invalid") end + it "skips invalid sources with whitespace or semicolons" do + invalid_sources = ["invalid source;", "'unsafe-eval' https://invalid.example.com'"] + builder << { script_src: invalid_sources } + script_srcs = parse(builder.build)["script-src"] + invalid_sources.each { |invalid_source| expect(script_srcs).not_to include(invalid_source) } + end + it "no-ops on invalid values" do previous = builder.build diff --git a/spec/lib/content_security_policy_spec.rb b/spec/lib/content_security_policy_spec.rb index eac5956bddb..19f3f300349 100644 --- a/spec/lib/content_security_policy_spec.rb +++ b/spec/lib/content_security_policy_spec.rb @@ -98,6 +98,7 @@ RSpec.describe ContentSecurityPolicy do let(:plugin_class) do Class.new(Plugin::Instance) do attr_accessor :enabled + def enabled? @enabled end diff --git a/spec/system/content_security_policy_spec.rb b/spec/system/content_security_policy_spec.rb index 52b41711240..ebf6f6c03db 100644 --- a/spec/system/content_security_policy_spec.rb +++ b/spec/system/content_security_policy_spec.rb @@ -1,11 +1,36 @@ # frozen_string_literal: true 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) + let(:plugin_class) do + Class.new(Plugin::Instance) do + attr_accessor :enabled + def enabled? + @enabled + end + end + end + + it "can boot the application in strict_dynamic mode even with invalid directives from CSP extensions" do + plugin = plugin_class.new(nil, "#{Rails.root}/spec/fixtures/plugins/csp_extension/plugin.rb") + + plugin.activate! + Discourse.plugins << plugin + + plugin.enabled = true + + expect(SiteSetting.content_security_policy).to eq(true) visit "/" expect(page).to have_css("#site-logo") + + get "/" + expect(response.headers["Content-Security-Policy"]).to include("'strict-dynamic'") + expect(response.headers["Content-Security-Policy"]).not_to include( + "'unsafe-eval' https://invalid.example.com'", + ) + + Discourse.plugins.delete plugin + DiscoursePluginRegistry.reset! end it "works for 'public exceptions' like RoutingError" do