Merge pull request #32054 from rails/fix-generation-of-empty-csp

Fix generation of empty content security policy

4 files changed, 73 insertions, 69 deletions

diff --git a/actionpack/lib/action_dispatch/http/content_security_policy.rb b/actionpack/lib/action_dispatch/http/content_security_policy.rb
index 160c345361..ffac3b8d99 100644
--- a/actionpack/lib/action_dispatch/http/content_security_policy.rb
+++ b/actionpack/lib/action_dispatch/http/content_security_policy.rb
@@ -21,10 +21,7 @@ module ActionDispatch #:nodoc:
         return response if policy_present?(headers)
 
         if policy = request.content_security_policy
-          built_policy = policy.build(request.controller_instance)
-          if built_policy
-            headers[header_name(request)] = built_policy
-          end
+          headers[header_name(request)] = policy.build(request.controller_instance)
         end
 
         response
@@ -175,12 +172,7 @@ module ActionDispatch #:nodoc:
     end
 
     def build(context = nil)
-      built_directives = build_directives(context).compact
-      if built_directives.empty?
-        nil
-      else
-        built_directives.join("; ") + ";"
-      end
+      build_directives(context).compact.join("; ")
     end
 
     private
diff --git a/actionpack/test/dispatch/content_security_policy_test.rb b/actionpack/test/dispatch/content_security_policy_test.rb
index cfec81eeae..5184e4f960 100644
--- a/actionpack/test/dispatch/content_security_policy_test.rb
+++ b/actionpack/test/dispatch/content_security_policy_test.rb
@@ -8,10 +8,10 @@ class ContentSecurityPolicyTest < ActiveSupport::TestCase
   end
 
   def test_build
-    assert_nil @policy.build
+    assert_equal "", @policy.build
 
     @policy.script_src :self
-    assert_equal "script-src 'self';", @policy.build
+    assert_equal "script-src 'self'", @policy.build
   end
 
   def test_dup
@@ -25,34 +25,34 @@ class ContentSecurityPolicyTest < ActiveSupport::TestCase
 
   def test_mappings
     @policy.script_src :data
-    assert_equal "script-src data:;", @policy.build
+    assert_equal "script-src data:", @policy.build
 
     @policy.script_src :mediastream
-    assert_equal "script-src mediastream:;", @policy.build
+    assert_equal "script-src mediastream:", @policy.build
 
     @policy.script_src :blob
-    assert_equal "script-src blob:;", @policy.build
+    assert_equal "script-src blob:", @policy.build
 
     @policy.script_src :filesystem
-    assert_equal "script-src filesystem:;", @policy.build
+    assert_equal "script-src filesystem:", @policy.build
 
     @policy.script_src :self
-    assert_equal "script-src 'self';", @policy.build
+    assert_equal "script-src 'self'", @policy.build
 
     @policy.script_src :unsafe_inline
-    assert_equal "script-src 'unsafe-inline';", @policy.build
+    assert_equal "script-src 'unsafe-inline'", @policy.build
 
     @policy.script_src :unsafe_eval
-    assert_equal "script-src 'unsafe-eval';", @policy.build
+    assert_equal "script-src 'unsafe-eval'", @policy.build
 
     @policy.script_src :none
-    assert_equal "script-src 'none';", @policy.build
+    assert_equal "script-src 'none'", @policy.build
 
     @policy.script_src :strict_dynamic
-    assert_equal "script-src 'strict-dynamic';", @policy.build
+    assert_equal "script-src 'strict-dynamic'", @policy.build
 
     @policy.script_src :none, :report_sample
-    assert_equal "script-src 'none' 'report-sample';", @policy.build
+    assert_equal "script-src 'none' 'report-sample'", @policy.build
   end
 
   def test_fetch_directives
@@ -131,16 +131,16 @@ class ContentSecurityPolicyTest < ActiveSupport::TestCase
 
   def test_document_directives
     @policy.base_uri "https://example.com"
-    assert_match %r{base-uri https://example\.com;}, @policy.build
+    assert_match %r{base-uri https://example\.com}, @policy.build
 
     @policy.plugin_types "application/x-shockwave-flash"
-    assert_match %r{plugin-types application/x-shockwave-flash;}, @policy.build
+    assert_match %r{plugin-types application/x-shockwave-flash}, @policy.build
 
     @policy.sandbox
-    assert_match %r{sandbox;}, @policy.build
+    assert_match %r{sandbox}, @policy.build
 
     @policy.sandbox "allow-scripts", "allow-modals"
-    assert_match %r{sandbox allow-scripts allow-modals;}, @policy.build
+    assert_match %r{sandbox allow-scripts allow-modals}, @policy.build
 
     @policy.sandbox false
     assert_no_match %r{sandbox}, @policy.build
@@ -148,35 +148,35 @@ class ContentSecurityPolicyTest < ActiveSupport::TestCase
 
   def test_navigation_directives
     @policy.form_action :self
-    assert_match %r{form-action 'self';}, @policy.build
+    assert_match %r{form-action 'self'}, @policy.build
 
     @policy.frame_ancestors :self
-    assert_match %r{frame-ancestors 'self';}, @policy.build
+    assert_match %r{frame-ancestors 'self'}, @policy.build
   end
 
   def test_reporting_directives
     @policy.report_uri "/violations"
-    assert_match %r{report-uri /violations;}, @policy.build
+    assert_match %r{report-uri /violations}, @policy.build
   end
 
   def test_other_directives
     @policy.block_all_mixed_content
-    assert_match %r{block-all-mixed-content;}, @policy.build
+    assert_match %r{block-all-mixed-content}, @policy.build
 
     @policy.block_all_mixed_content false
     assert_no_match %r{block-all-mixed-content}, @policy.build
 
     @policy.require_sri_for :script, :style
-    assert_match %r{require-sri-for script style;}, @policy.build
+    assert_match %r{require-sri-for script style}, @policy.build
 
     @policy.require_sri_for "script", "style"
-    assert_match %r{require-sri-for script style;}, @policy.build
+    assert_match %r{require-sri-for script style}, @policy.build
 
     @policy.require_sri_for
     assert_no_match %r{require-sri-for}, @policy.build
 
     @policy.upgrade_insecure_requests
-    assert_match %r{upgrade-insecure-requests;}, @policy.build
+    assert_match %r{upgrade-insecure-requests}, @policy.build
 
     @policy.upgrade_insecure_requests false
     assert_no_match %r{upgrade-insecure-requests}, @policy.build
@@ -184,13 +184,13 @@ class ContentSecurityPolicyTest < ActiveSupport::TestCase
 
   def test_multiple_sources
     @policy.script_src :self, :https
-    assert_equal "script-src 'self' https:;", @policy.build
+    assert_equal "script-src 'self' https:", @policy.build
   end
 
   def test_multiple_directives
     @policy.script_src :self, :https
     @policy.style_src :self, :https
-    assert_equal "script-src 'self' https:; style-src 'self' https:;", @policy.build
+    assert_equal "script-src 'self' https:; style-src 'self' https:", @policy.build
   end
 
   def test_dynamic_directives
@@ -198,12 +198,12 @@ class ContentSecurityPolicyTest < ActiveSupport::TestCase
     controller = Struct.new(:request).new(request)
 
     @policy.script_src -> { request.host }
-    assert_equal "script-src www.example.com;", @policy.build(controller)
+    assert_equal "script-src www.example.com", @policy.build(controller)
   end
 
   def test_mixed_static_and_dynamic_directives
     @policy.script_src :self, -> { "foo.com" }, "bar.com"
-    assert_equal "script-src 'self' foo.com bar.com;", @policy.build(Object.new)
+    assert_equal "script-src 'self' foo.com bar.com", @policy.build(Object.new)
   end
 
   def test_invalid_directive_source
@@ -271,10 +271,6 @@ class ContentSecurityPolicyIntegrationTest < ActionDispatch::IntegrationTest
       head :ok
     end
 
-    def empty_policy
-      head :ok
-    end
-
     private
       def condition?
         params[:condition] == "true"
@@ -288,14 +284,12 @@ class ContentSecurityPolicyIntegrationTest < ActionDispatch::IntegrationTest
       get "/inline", to: "policy#inline"
       get "/conditional", to: "policy#conditional"
       get "/report-only", to: "policy#report_only"
-      get "/empty-policy", to: "policy#empty_policy"
     end
   end
 
   POLICY = ActionDispatch::ContentSecurityPolicy.new do |p|
     p.default_src :self
   end
-  EMPTY_POLICY = ActionDispatch::ContentSecurityPolicy.new
 
   class PolicyConfigMiddleware
     def initialize(app)
@@ -303,12 +297,7 @@ class ContentSecurityPolicyIntegrationTest < ActionDispatch::IntegrationTest
     end
 
     def call(env)
-      env["action_dispatch.content_security_policy"] =
-        if env["PATH_INFO"] == "/empty-policy"
-          EMPTY_POLICY
-        else
-          POLICY
-        end
+      env["action_dispatch.content_security_policy"] = POLICY
       env["action_dispatch.content_security_policy_report_only"] = false
       env["action_dispatch.show_exceptions"] = false
 
@@ -327,32 +316,25 @@ class ContentSecurityPolicyIntegrationTest < ActionDispatch::IntegrationTest
 
   def test_generates_content_security_policy_header
     get "/"
-    assert_policy "default-src 'self';"
+    assert_policy "default-src 'self'"
   end
 
   def test_generates_inline_content_security_policy
     get "/inline"
-    assert_policy "default-src https://example.com;"
+    assert_policy "default-src https://example.com"
   end
 
   def test_generates_conditional_content_security_policy
     get "/conditional", params: { condition: "true" }
-    assert_policy "default-src https://true.example.com;"
+    assert_policy "default-src https://true.example.com"
 
     get "/conditional", params: { condition: "false" }
-    assert_policy "default-src https://false.example.com;"
+    assert_policy "default-src https://false.example.com"
   end
 
   def test_generates_report_only_content_security_policy
     get "/report-only"
-    assert_policy "default-src 'self'; report-uri /violations;", report_only: true
-  end
-
-  def test_empty_policy
-    get "/empty-policy"
-    assert_response :success
-    assert_not response.headers.key?("Content-Security-Policy")
-    assert_not response.headers.key?("Content-Security-Policy-Report-Only")
+    assert_policy "default-src 'self'; report-uri /violations", report_only: true
   end
 
   private
diff --git a/railties/lib/rails/application/configuration.rb b/railties/lib/rails/application/configuration.rb
index 46ad3557e3..1f765f302c 100644
--- a/railties/lib/rails/application/configuration.rb
+++ b/railties/lib/rails/application/configuration.rb
@@ -241,7 +241,11 @@ module Rails
       end
 
       def content_security_policy(&block)
-        @content_security_policy ||= ActionDispatch::ContentSecurityPolicy.new(&block)
+        if block_given?
+          @content_security_policy = ActionDispatch::ContentSecurityPolicy.new(&block)
+        else
+          @content_security_policy
+        end
       end
 
       class Custom #:nodoc:
diff --git a/railties/test/application/content_security_policy_test.rb b/railties/test/application/content_security_policy_test.rb
index 1539bf4440..0d28df16f8 100644
--- a/railties/test/application/content_security_policy_test.rb
+++ b/railties/test/application/content_security_policy_test.rb
@@ -16,7 +16,7 @@ module ApplicationTests
       teardown_app
     end
 
-    test "default content security policy is empty" do
+    test "default content security policy is nil" do
       controller :pages, <<-RUBY
         class PagesController < ApplicationController
           def index
@@ -34,7 +34,33 @@ module ApplicationTests
       app("development")
 
       get "/"
-      assert_not last_response.headers.key?("Content-Security-Policy")
+      assert_nil last_response.headers["Content-Security-Policy"]
+    end
+
+    test "empty content security policy is generated" do
+      controller :pages, <<-RUBY
+        class PagesController < ApplicationController
+          def index
+            render html: "<h1>Welcome to Rails!</h1>"
+          end
+        end
+      RUBY
+
+      app_file "config/initializers/content_security_policy.rb", <<-RUBY
+        Rails.application.config.content_security_policy do |p|
+        end
+      RUBY
+
+      app_file "config/routes.rb", <<-RUBY
+        Rails.application.routes.draw do
+          root to: "pages#index"
+        end
+      RUBY
+
+      app("development")
+
+      get "/"
+      assert_policy ""
     end
 
     test "global content security policy in an initializer" do
@@ -61,7 +87,7 @@ module ApplicationTests
       app("development")
 
       get "/"
-      assert_policy "default-src 'self' https:;"
+      assert_policy "default-src 'self' https:"
     end
 
     test "global report only content security policy in an initializer" do
@@ -90,7 +116,7 @@ module ApplicationTests
       app("development")
 
       get "/"
-      assert_policy "default-src 'self' https:;", report_only: true
+      assert_policy "default-src 'self' https:", report_only: true
     end
 
     test "override content security policy in a controller" do
@@ -121,7 +147,7 @@ module ApplicationTests
       app("development")
 
       get "/"
-      assert_policy "default-src https://example.com;"
+      assert_policy "default-src https://example.com"
     end
 
     test "override content security policy to report only in a controller" do
@@ -150,7 +176,7 @@ module ApplicationTests
       app("development")
 
       get "/"
-      assert_policy "default-src 'self' https:;", report_only: true
+      assert_policy "default-src 'self' https:", report_only: true
     end
 
     test "global content security policy added to rack app" do
@@ -174,7 +200,7 @@ module ApplicationTests
       app("development")
 
       get "/"
-      assert_policy "default-src 'self' https:;"
+      assert_policy "default-src 'self' https:"
     end
 
     private