From 52a1f1c226c2238e16d1a4d32faa8d1e6a36a26f Mon Sep 17 00:00:00 2001 From: Andrew White Date: Mon, 19 Feb 2018 12:00:29 +0000 Subject: Revert "Merge pull request #32045 from eagletmt/skip-csp-header" This reverts commit 86f7c269073a3a9e6ddec9b957deaa2716f2627d, reversing changes made to 5ece2e4a4459065b5efd976aebd209bbf0cab89b. If a policy is set then we should generate it even if it's empty. However what is happening is that we're accidentally generating an empty policy when the initializer is commented out by default. --- .../http/content_security_policy.rb | 12 ++---------- .../test/dispatch/content_security_policy_test.rb | 22 ++-------------------- .../application/content_security_policy_test.rb | 2 +- 3 files changed, 5 insertions(+), 31 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..4883e23d24 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..7c4a65a633 100644 --- a/actionpack/test/dispatch/content_security_policy_test.rb +++ b/actionpack/test/dispatch/content_security_policy_test.rb @@ -8,7 +8,7 @@ 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 @@ -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 @@ -348,13 +337,6 @@ class ContentSecurityPolicyIntegrationTest < ActionDispatch::IntegrationTest 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") - end - private def env_config diff --git a/railties/test/application/content_security_policy_test.rb b/railties/test/application/content_security_policy_test.rb index 1539bf4440..97f2957c33 100644 --- a/railties/test/application/content_security_policy_test.rb +++ b/railties/test/application/content_security_policy_test.rb @@ -34,7 +34,7 @@ module ApplicationTests app("development") get "/" - assert_not last_response.headers.key?("Content-Security-Policy") + assert_equal ";", last_response.headers["Content-Security-Policy"] end test "global content security policy in an initializer" do -- cgit v1.2.3 From 57f9c36387f371cfb791aa660c733e9690443d04 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Mon, 19 Feb 2018 12:17:51 +0000 Subject: Don't accidentally create an empty CSP Setting up the request environment was accidentally creating a CSP as a consequence of accessing the option - only set the instance variable if a block is passed. --- railties/lib/rails/application/configuration.rb | 6 ++++- .../application/content_security_policy_test.rb | 30 ++++++++++++++++++++-- 2 files changed, 33 insertions(+), 3 deletions(-) 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 97f2957c33..43f2b333f3 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_equal ";", last_response.headers["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: "

Welcome to Rails!

" + 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 -- cgit v1.2.3 From d85283cc42b1a965944047a2f602153804126f77 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Mon, 19 Feb 2018 12:20:43 +0000 Subject: Remove trailing semi-colon from CSP Although the spec[1] is defined in such a way that a trailing semi-colon is valid it also doesn't allow a semi-colon by itself to indicate an empty policy. Therefore it's easier (and valid) just to omit it rather than to detect whether the policy is empty or not. [1]: https://www.w3.org/TR/CSP2/#policy-syntax --- .../http/content_security_policy.rb | 2 +- .../test/dispatch/content_security_policy_test.rb | 64 +++++++++++----------- .../application/content_security_policy_test.rb | 12 ++-- 3 files changed, 39 insertions(+), 39 deletions(-) diff --git a/actionpack/lib/action_dispatch/http/content_security_policy.rb b/actionpack/lib/action_dispatch/http/content_security_policy.rb index 4883e23d24..ffac3b8d99 100644 --- a/actionpack/lib/action_dispatch/http/content_security_policy.rb +++ b/actionpack/lib/action_dispatch/http/content_security_policy.rb @@ -172,7 +172,7 @@ module ActionDispatch #:nodoc: end def build(context = nil) - build_directives(context).compact.join("; ") + ";" + 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 7c4a65a633..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_equal ";", @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 @@ -316,25 +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 + assert_policy "default-src 'self'; report-uri /violations", report_only: true end private diff --git a/railties/test/application/content_security_policy_test.rb b/railties/test/application/content_security_policy_test.rb index 43f2b333f3..0d28df16f8 100644 --- a/railties/test/application/content_security_policy_test.rb +++ b/railties/test/application/content_security_policy_test.rb @@ -60,7 +60,7 @@ module ApplicationTests app("development") get "/" - assert_policy ";" + assert_policy "" end test "global content security policy in an initializer" do @@ -87,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 @@ -116,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 @@ -147,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 @@ -176,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 @@ -200,7 +200,7 @@ module ApplicationTests app("development") get "/" - assert_policy "default-src 'self' https:;" + assert_policy "default-src 'self' https:" end private -- cgit v1.2.3