Merge pull request #32602 from Envek/fix/csp-multiple-nonces

Output only one nonce in CSP header per request

3 files changed, 94 insertions, 13 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 152ec3700b..93dd532f07 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,9 @@
+*   Output only one Content-Security-Policy nonce header value per request.
+
+    Fixes [#35297](https://github.com/rails/rails/issues/32597).
+
+    *Andrey Novikov*, *Andrew White*
+
 *   Move default headers configuration into their own module that can be included in controllers.
 
     *Kevin Deisz*
diff --git a/actionpack/lib/action_dispatch/http/content_security_policy.rb b/actionpack/lib/action_dispatch/http/content_security_policy.rb
index c1f80a1ffc..82d348fa22 100644
--- a/actionpack/lib/action_dispatch/http/content_security_policy.rb
+++ b/actionpack/lib/action_dispatch/http/content_security_policy.rb
@@ -21,13 +21,8 @@ module ActionDispatch #:nodoc:
         return response if policy_present?(headers)
 
         if policy = request.content_security_policy
-          if policy.directives["script-src"]
-            if nonce = request.content_security_policy_nonce
-              policy.directives["script-src"] << "'nonce-#{nonce}'"
-            end
-          end
-
-          headers[header_name(request)] = policy.build(request.controller_instance)
+          nonce = request.content_security_policy_nonce
+          headers[header_name(request)] = policy.build(request.controller_instance, nonce)
         end
 
         response
@@ -136,7 +131,9 @@ module ActionDispatch #:nodoc:
       worker_src:      "worker-src"
     }.freeze
 
-    private_constant :MAPPINGS, :DIRECTIVES
+    NONCE_DIRECTIVES = %w[script-src].freeze
+
+    private_constant :MAPPINGS, :DIRECTIVES, :NONCE_DIRECTIVES
 
     attr_reader :directives
 
@@ -205,8 +202,8 @@ module ActionDispatch #:nodoc:
       end
     end
 
-    def build(context = nil)
-      build_directives(context).compact.join("; ")
+    def build(context = nil, nonce = nil)
+      build_directives(context, nonce).compact.join("; ")
     end
 
     private
@@ -229,10 +226,15 @@ module ActionDispatch #:nodoc:
         end
       end
 
-      def build_directives(context)
+      def build_directives(context, nonce)
         @directives.map do |directive, sources|
           if sources.is_a?(Array)
             "#{directive} #{build_directive(sources, context).join(' ')}"
+            if nonce && nonce_directive?(directive)
+              "#{directive} #{build_directive(sources, context).join(' ')} 'nonce-#{nonce}'"
+            else
+              "#{directive} #{build_directive(sources, context).join(' ')}"
+            end
           elsif sources
             directive
           else
@@ -261,5 +263,9 @@ module ActionDispatch #:nodoc:
           raise RuntimeError, "Unexpected content security policy source: #{source.inspect}"
         end
       end
+
+      def nonce_directive?(directive)
+        NONCE_DIRECTIVES.include?(directive)
+      end
   end
 end
diff --git a/actionpack/test/dispatch/content_security_policy_test.rb b/actionpack/test/dispatch/content_security_policy_test.rb
index 95fce39dad..c4c7f53903 100644
--- a/actionpack/test/dispatch/content_security_policy_test.rb
+++ b/actionpack/test/dispatch/content_security_policy_test.rb
@@ -200,7 +200,7 @@ class ContentSecurityPolicyTest < ActiveSupport::TestCase
   end
 
   def test_dynamic_directives
-    request = Struct.new(:host).new("www.example.com")
+    request = ActionDispatch::Request.new("HTTP_HOST" => "www.example.com")
     controller = Struct.new(:request).new(request)
 
     @policy.script_src -> { request.host }
@@ -209,7 +209,9 @@ class ContentSecurityPolicyTest < ActiveSupport::TestCase
 
   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)
+    request = ActionDispatch::Request.new({})
+    controller = Struct.new(:request).new(request)
+    assert_equal "script-src 'self' foo.com bar.com", @policy.build(controller)
   end
 
   def test_invalid_directive_source
@@ -241,6 +243,73 @@ class ContentSecurityPolicyTest < ActiveSupport::TestCase
   end
 end
 
+class DefaultContentSecurityPolicyIntegrationTest < ActionDispatch::IntegrationTest
+  class PolicyController < ActionController::Base
+    def index
+      head :ok
+    end
+  end
+
+  ROUTES = ActionDispatch::Routing::RouteSet.new
+  ROUTES.draw do
+    scope module: "default_content_security_policy_integration_test" do
+      get "/", to: "policy#index"
+    end
+  end
+
+  POLICY = ActionDispatch::ContentSecurityPolicy.new do |p|
+    p.default_src :self
+    p.script_src  :https
+  end
+
+  class PolicyConfigMiddleware
+    def initialize(app)
+      @app = app
+    end
+
+    def call(env)
+      env["action_dispatch.content_security_policy"] = POLICY
+      env["action_dispatch.content_security_policy_nonce_generator"] = proc { "iyhD0Yc0W+c=" }
+      env["action_dispatch.content_security_policy_report_only"] = false
+      env["action_dispatch.show_exceptions"] = false
+
+      @app.call(env)
+    end
+  end
+
+  APP = build_app(ROUTES) do |middleware|
+    middleware.use PolicyConfigMiddleware
+    middleware.use ActionDispatch::ContentSecurityPolicy::Middleware
+  end
+
+  def app
+    APP
+  end
+
+  def test_adds_nonce_to_script_src_content_security_policy_only_once
+    get "/"
+    get "/"
+    assert_policy "default-src 'self'; script-src https: 'nonce-iyhD0Yc0W+c='"
+  end
+
+  private
+
+    def assert_policy(expected, report_only: false)
+      assert_response :success
+
+      if report_only
+        expected_header = "Content-Security-Policy-Report-Only"
+        unexpected_header = "Content-Security-Policy"
+      else
+        expected_header = "Content-Security-Policy"
+        unexpected_header = "Content-Security-Policy-Report-Only"
+      end
+
+      assert_nil response.headers[unexpected_header]
+      assert_equal expected, response.headers[expected_header]
+    end
+end
+
 class ContentSecurityPolicyIntegrationTest < ActionDispatch::IntegrationTest
   class PolicyController < ActionController::Base
     content_security_policy only: :inline do |p|