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