From b9b660728ff771424e423ef917ad81ffadd50210 Mon Sep 17 00:00:00 2001 From: Andrey Novikov Date: Tue, 17 Apr 2018 12:48:29 +0300 Subject: Output only one nonce in CSP header per request --- actionpack/CHANGELOG.md | 6 +++ .../http/content_security_policy.rb | 47 ++++++++++++++-------- .../test/dispatch/content_security_policy_test.rb | 28 ++++++++++--- 3 files changed, 59 insertions(+), 22 deletions(-) diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 152ec3700b..c85add22a9 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* + * 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..0573f13f94 100644 --- a/actionpack/lib/action_dispatch/http/content_security_policy.rb +++ b/actionpack/lib/action_dispatch/http/content_security_policy.rb @@ -21,13 +21,7 @@ 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) + headers[header_name(request)] = policy.build(request) end response @@ -101,6 +95,14 @@ module ActionDispatch #:nodoc: end end + class NonceGenerator + def call(request) + if nonce = request&.content_security_policy_nonce + "'nonce-#{nonce}'" + end + end + end + MAPPINGS = { self: "'self'", unsafe_eval: "'unsafe-eval'", @@ -131,7 +133,7 @@ module ActionDispatch #:nodoc: manifest_src: "manifest-src", media_src: "media-src", object_src: "object-src", - script_src: "script-src", + # script_src handled differently style_src: "style-src", worker_src: "worker-src" }.freeze @@ -159,6 +161,15 @@ module ActionDispatch #:nodoc: end end + def script_src(*sources) + if sources.first + @directives["script-src"] = apply_mappings(sources) + @directives["script-src"] << NonceGenerator.new + else + @directives.delete("script-src") + end + end + def block_all_mixed_content(enabled = true) if enabled @directives["block-all-mixed-content"] = true @@ -205,8 +216,8 @@ module ActionDispatch #:nodoc: end end - def build(context = nil) - build_directives(context).compact.join("; ") + def build(request = nil) + build_directives(request).compact.join("; ") end private @@ -229,10 +240,10 @@ module ActionDispatch #:nodoc: end end - def build_directives(context) + def build_directives(request) @directives.map do |directive, sources| if sources.is_a?(Array) - "#{directive} #{build_directive(sources, context).join(' ')}" + "#{directive} #{build_directive(sources, request).compact.join(' ')}" elsif sources directive else @@ -241,22 +252,24 @@ module ActionDispatch #:nodoc: end end - def build_directive(sources, context) - sources.map { |source| resolve_source(source, context) } + def build_directive(sources, request) + sources.map { |source| resolve_source(source, request) } end - def resolve_source(source, context) + def resolve_source(source, request) case source when String source when Symbol source.to_s when Proc - if context.nil? + if request&.controller_instance.nil? raise RuntimeError, "Missing context for the dynamic content security policy source: #{source.inspect}" else - context.instance_exec(&source) + request.controller_instance.instance_exec(&source) end + when NonceGenerator + source.call(request) else raise RuntimeError, "Unexpected content security policy source: #{source.inspect}" end diff --git a/actionpack/test/dispatch/content_security_policy_test.rb b/actionpack/test/dispatch/content_security_policy_test.rb index 95fce39dad..eb0b930828 100644 --- a/actionpack/test/dispatch/content_security_policy_test.rb +++ b/actionpack/test/dispatch/content_security_policy_test.rb @@ -200,16 +200,18 @@ class ContentSecurityPolicyTest < ActiveSupport::TestCase end def test_dynamic_directives - request = Struct.new(:host).new("www.example.com") - controller = Struct.new(:request).new(request) + request = ActionDispatch::Request.new("HTTP_HOST" => "www.example.com") + request.controller_instance = 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(request) 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) + request = ActionDispatch::Request.new({}) + request.controller_instance = Struct.new(:request).new(request) + assert_equal "script-src 'self' foo.com bar.com", @policy.build(request) end def test_invalid_directive_source @@ -245,17 +247,21 @@ class ContentSecurityPolicyIntegrationTest < ActionDispatch::IntegrationTest class PolicyController < ActionController::Base content_security_policy only: :inline do |p| p.default_src "https://example.com" + p.script_src false end content_security_policy only: :conditional, if: :condition? do |p| p.default_src "https://true.example.com" + p.script_src false end content_security_policy only: :conditional, unless: :condition? do |p| p.default_src "https://false.example.com" + p.script_src false end content_security_policy only: :report_only do |p| + p.script_src false p.report_uri "/violations" end @@ -292,6 +298,10 @@ class ContentSecurityPolicyIntegrationTest < ActionDispatch::IntegrationTest head :ok end + def default_script_src + head :ok + end + private def condition? params[:condition] == "true" @@ -307,11 +317,13 @@ class ContentSecurityPolicyIntegrationTest < ActionDispatch::IntegrationTest get "/report-only", to: "policy#report_only" get "/script-src", to: "policy#script_src" get "/no-policy", to: "policy#no_policy" + get "/default-script-src", to: "policy#default_script_src" end end POLICY = ActionDispatch::ContentSecurityPolicy.new do |p| p.default_src :self + p.script_src :https end class PolicyConfigMiddleware @@ -340,7 +352,7 @@ class ContentSecurityPolicyIntegrationTest < ActionDispatch::IntegrationTest def test_generates_content_security_policy_header get "/" - assert_policy "default-src 'self'" + assert_policy "default-src 'self'; script-src https: 'nonce-iyhD0Yc0W+c='" end def test_generates_inline_content_security_policy @@ -366,6 +378,12 @@ class ContentSecurityPolicyIntegrationTest < ActionDispatch::IntegrationTest assert_policy "script-src 'self' 'nonce-iyhD0Yc0W+c='" end + def test_adds_nonce_to_script_src_content_security_policy_only_once + get "/default-script-src" + get "/default-script-src" + assert_policy "default-src 'self'; script-src https: 'nonce-iyhD0Yc0W+c='" + end + def test_generates_no_content_security_policy get "/no-policy" -- cgit v1.2.3