From 05344fc85399902f431768d2738cd1d50083314f Mon Sep 17 00:00:00 2001 From: Tim Wade Date: Tue, 16 Jun 2020 10:54:35 -0700 Subject: [PATCH 01/12] Generate content security policy for non-HTML responses One feature of the content security policy DSL, though undocumented, is that it will not generate headers for non-HTML responses, even if a configuration is explicitly provided. While it may not seem obvious that anyone would want to send this header in an API response, Mozilla Observatory, for instance, recommends the following for API responses: `Content-Security-Policy: default-src 'none'; frame-ancestors 'none'` (source: https://observatory.mozilla.org/faq/) The Secure Headers gem also makes recommendations about the content security policy for API responses: https://github.com/github/secure_headers#api-configurations As such, this removes the HTML guard clause from the `ContentSecurityPolicy` middleware. --- actionpack/CHANGELOG.md | 3 +++ .../http/content_security_policy.rb | 7 ------- .../test/dispatch/content_security_policy_test.rb | 15 +++++++++++++++ 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index d726b82e96663..0832663349d87 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,6 @@ +* Allow Content Security Policy DSL to generate for API responses. + *Tim Wade* + * When multiple domains are specified for a cookie, a domain will now be chosen only if it is equal to or is a superdomain of the request host. diff --git a/actionpack/lib/action_dispatch/http/content_security_policy.rb b/actionpack/lib/action_dispatch/http/content_security_policy.rb index e8cf1b95a5738..79c346af80f25 100644 --- a/actionpack/lib/action_dispatch/http/content_security_policy.rb +++ b/actionpack/lib/action_dispatch/http/content_security_policy.rb @@ -17,7 +17,6 @@ def call(env) request = ActionDispatch::Request.new env _, headers, _ = response = @app.call(env) - return response unless html_response?(headers) return response if policy_present?(headers) if policy = request.content_security_policy @@ -31,12 +30,6 @@ def call(env) end private - def html_response?(headers) - if content_type = headers[CONTENT_TYPE] - /html/.match?(content_type) - end - end - def header_name(request) if request.content_security_policy_report_only POLICY_REPORT_ONLY diff --git a/actionpack/test/dispatch/content_security_policy_test.rb b/actionpack/test/dispatch/content_security_policy_test.rb index 3d60dc1661fdf..2facbbf1e945b 100644 --- a/actionpack/test/dispatch/content_security_policy_test.rb +++ b/actionpack/test/dispatch/content_security_policy_test.rb @@ -377,6 +377,11 @@ class PolicyController < ActionController::Base content_security_policy_report_only only: :report_only + content_security_policy only: :api do |p| + p.default_src :none + p.frame_ancestors :none + end + def index head :ok end @@ -405,6 +410,10 @@ def no_policy head :ok end + def api + render json: {} + end + private def condition? params[:condition] == "true" @@ -421,6 +430,7 @@ def condition? get "/script-src", to: "policy#script_src" get "/style-src", to: "policy#style_src" get "/no-policy", to: "policy#no_policy" + get "/api", to: "policy#api" end end @@ -492,6 +502,11 @@ def test_generates_no_content_security_policy assert_nil response.headers["Content-Security-Policy-Report-Only"] end + def test_generates_api_security_policy + get "/api" + assert_policy "default-src 'none'; frame-ancestors 'none'" + end + private def assert_policy(expected, report_only: false) assert_response :success From b840a7d5d7c9266a8823483b466625781eba5664 Mon Sep 17 00:00:00 2001 From: Tim Wade Date: Wed, 1 Jul 2020 17:07:04 -0700 Subject: [PATCH 02/12] [wip] failing test for html --- .../content_security_policy_test.rb | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/railties/test/application/content_security_policy_test.rb b/railties/test/application/content_security_policy_test.rb index 0bb6ee917a76e..e740ea174b1c5 100644 --- a/railties/test/application/content_security_policy_test.rb +++ b/railties/test/application/content_security_policy_test.rb @@ -235,6 +235,35 @@ def index assert_policy "default-src 'self' https:" end + test "global content security policy for HTML requests" 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 |format| + format.html do |p| + p.default_src :self, :https + end + end + RUBY + + app_file "config/routes.rb", <<-RUBY + Rails.application.routes.draw do + root to: "pages#index" + end + RUBY + + app("development") + + get "/" + assert_policy "default-src 'self' https:" + end + private def assert_policy(expected, report_only: false) assert_equal 200, last_response.status From 2aa65857db9f92cbf5dc0367119483560c738062 Mon Sep 17 00:00:00 2001 From: Tim Wade Date: Mon, 6 Jul 2020 11:11:13 -0700 Subject: [PATCH 03/12] Add content_security_policy.html --- .../http/content_security_policy.rb | 4 + .../content_security_policy_test.rb | 126 +++++++++++++++++- 2 files changed, 129 insertions(+), 1 deletion(-) diff --git a/actionpack/lib/action_dispatch/http/content_security_policy.rb b/actionpack/lib/action_dispatch/http/content_security_policy.rb index 79c346af80f25..6c24b53906f69 100644 --- a/actionpack/lib/action_dispatch/http/content_security_policy.rb +++ b/actionpack/lib/action_dispatch/http/content_security_policy.rb @@ -163,6 +163,10 @@ def initialize_copy(other) end end + def html + yield self + end + def block_all_mixed_content(enabled = true) if enabled @directives["block-all-mixed-content"] = true diff --git a/railties/test/application/content_security_policy_test.rb b/railties/test/application/content_security_policy_test.rb index e740ea174b1c5..91857c29fbc49 100644 --- a/railties/test/application/content_security_policy_test.rb +++ b/railties/test/application/content_security_policy_test.rb @@ -235,7 +235,7 @@ def index assert_policy "default-src 'self' https:" end - test "global content security policy for HTML requests" do + test "global content security policy for HTML requests in an initializer" do controller :pages, <<-RUBY class PagesController < ApplicationController def index @@ -264,6 +264,130 @@ def index assert_policy "default-src 'self' https:" end + test "global report only content security policy for HTML requests in an initializer" 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 |format| + format.html do |p| + p.default_src :self, :https + end + end + + Rails.application.config.content_security_policy_report_only = true + RUBY + + app_file "config/routes.rb", <<-RUBY + Rails.application.routes.draw do + root to: "pages#index" + end + RUBY + + app("development") + + get "/" + assert_policy "default-src 'self' https:", report_only: true + end + + test "global content security policy for HTML requests nonce directives in an initializer" 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 |format| + format.html do |p| + p.default_src :self, :https + p.script_src :self, :https + p.style_src :self, :https + end + end + + Rails.application.config.content_security_policy_nonce_generator = proc { "iyhD0Yc0W+c=" } + Rails.application.config.content_security_policy_nonce_directives = %w(script-src) + RUBY + + app_file "config/routes.rb", <<-RUBY + Rails.application.routes.draw do + root to: "pages#index" + end + RUBY + + app("development") + + get "/" + assert_policy "default-src 'self' https:; script-src 'self' https: 'nonce-iyhD0Yc0W+c='; style-src 'self' https:" + end + + test "override content security policy for HTML requests in a controller" do + controller :pages, <<-RUBY + class PagesController < ApplicationController + content_security_policy do |format| + format.html do |p| + p.default_src "https://example.com" + end + end + + 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| + p.default_src :self, :https + end + RUBY + + app_file "config/routes.rb", <<-RUBY + Rails.application.routes.draw do + root to: "pages#index" + end + RUBY + + app("development") + + get "/" + assert_policy "default-src https://example.com" + end + + test "global content security policy for HTML requests added to rack app" do + app_file "config/initializers/content_security_policy.rb", <<-RUBY + Rails.application.config.content_security_policy do |format| + format.html do |p| + p.default_src :self, :https + end + end + RUBY + + app_file "config/routes.rb", <<-RUBY + Rails.application.routes.draw do + + app = ->(env) { + [200, { "Content-Type" => "text/html" }, ["

Hello, World!

"]] + } + + root to: app + end + RUBY + + app("development") + + get "/" + assert_policy "default-src 'self' https:" + end + private def assert_policy(expected, report_only: false) assert_equal 200, last_response.status From 2ea33e80c1075d1bc6a3af18c0abcfc972673fb1 Mon Sep 17 00:00:00 2001 From: Tim Wade Date: Mon, 6 Jul 2020 11:33:43 -0700 Subject: [PATCH 04/12] Add some tests for JSON, slime it for now --- .../http/content_security_policy.rb | 5 ++ .../content_security_policy_test.rb | 88 +++++++++++++++++++ 2 files changed, 93 insertions(+) diff --git a/actionpack/lib/action_dispatch/http/content_security_policy.rb b/actionpack/lib/action_dispatch/http/content_security_policy.rb index 6c24b53906f69..6370863e8bc75 100644 --- a/actionpack/lib/action_dispatch/http/content_security_policy.rb +++ b/actionpack/lib/action_dispatch/http/content_security_policy.rb @@ -167,6 +167,11 @@ def html yield self end + # TODO: this isn't going to pan out + def json + yield self + end + def block_all_mixed_content(enabled = true) if enabled @directives["block-all-mixed-content"] = true diff --git a/railties/test/application/content_security_policy_test.rb b/railties/test/application/content_security_policy_test.rb index 91857c29fbc49..3b3a5dcf65d5c 100644 --- a/railties/test/application/content_security_policy_test.rb +++ b/railties/test/application/content_security_policy_test.rb @@ -388,6 +388,94 @@ def index assert_policy "default-src 'self' https:" end + test "global content security policy for JSON requests in an initializer" do + controller :pages, <<-RUBY + class PagesController < ApplicationController + def index + render json: { some: "json" } + end + end + RUBY + + app_file "config/initializers/content_security_policy.rb", <<-RUBY + Rails.application.config.content_security_policy do |format| + format.json do |p| + p.default_src :none + end + end + RUBY + + app_file "config/routes.rb", <<-RUBY + Rails.application.routes.draw do + root to: "pages#index" + end + RUBY + + app("development") + + get "/" + assert_policy "default-src 'none'" + end + + test "override content security policy for JSON requests in a controller" do + controller :pages, <<-RUBY + class PagesController < ApplicationController + content_security_policy do |format| + format.json do |p| + p.default_src :none + end + end + + def index + render json: { some: "json" } + end + end + RUBY + + app_file "config/initializers/content_security_policy.rb", <<-RUBY + Rails.application.config.content_security_policy do |p| + p.default_src :self, :https + end + RUBY + + app_file "config/routes.rb", <<-RUBY + Rails.application.routes.draw do + root to: "pages#index" + end + RUBY + + app("development") + + get "/" + assert_policy "default-src 'none'" + end + + test "global content security policy for JSON requests added to rack app" do + app_file "config/initializers/content_security_policy.rb", <<-RUBY + Rails.application.config.content_security_policy do |format| + format.json do |p| + p.default_src :none + end + end + RUBY + + app_file "config/routes.rb", <<-RUBY + Rails.application.routes.draw do + + app = ->(env) { + [200, { "Content-Type" => "application/json" }, ["{\\"some\\": \\"json\\"}"]] + } + + root to: app + end + RUBY + + app("development") + + get "/" + assert_policy "default-src 'none'" + end + private def assert_policy(expected, report_only: false) assert_equal 200, last_response.status From 12a0048c4ec80f1db38bfa767a2d0a9019604c2f Mon Sep 17 00:00:00 2001 From: Tim Wade Date: Mon, 6 Jul 2020 15:51:22 -0700 Subject: [PATCH 05/12] Approximating a solution --- .../http/content_security_policy.rb | 327 ++++++++++-------- .../content_security_policy_test.rb | 40 +++ 2 files changed, 221 insertions(+), 146 deletions(-) diff --git a/actionpack/lib/action_dispatch/http/content_security_policy.rb b/actionpack/lib/action_dispatch/http/content_security_policy.rb index 6370863e8bc75..d3a12326690dc 100644 --- a/actionpack/lib/action_dispatch/http/content_security_policy.rb +++ b/actionpack/lib/action_dispatch/http/content_security_policy.rb @@ -23,7 +23,7 @@ def call(env) nonce = request.content_security_policy_nonce nonce_directives = request.content_security_policy_nonce_directives context = request.controller_instance || request - headers[header_name(request)] = policy.build(context, nonce, nonce_directives) + headers[header_name(request)] = policy.build(headers[CONTENT_TYPE], context, nonce, nonce_directives) end response @@ -98,191 +98,226 @@ def generate_content_security_policy_nonce end end - MAPPINGS = { - self: "'self'", - unsafe_eval: "'unsafe-eval'", - unsafe_inline: "'unsafe-inline'", - none: "'none'", - http: "http:", - https: "https:", - data: "data:", - mediastream: "mediastream:", - blob: "blob:", - filesystem: "filesystem:", - report_sample: "'report-sample'", - strict_dynamic: "'strict-dynamic'", - ws: "ws:", - wss: "wss:" - }.freeze - - DIRECTIVES = { - base_uri: "base-uri", - child_src: "child-src", - connect_src: "connect-src", - default_src: "default-src", - font_src: "font-src", - form_action: "form-action", - frame_ancestors: "frame-ancestors", - frame_src: "frame-src", - img_src: "img-src", - manifest_src: "manifest-src", - media_src: "media-src", - object_src: "object-src", - prefetch_src: "prefetch-src", - script_src: "script-src", - script_src_attr: "script-src-attr", - script_src_elem: "script-src-elem", - style_src: "style-src", - style_src_attr: "style-src-attr", - style_src_elem: "style-src-elem", - worker_src: "worker-src" - }.freeze - - DEFAULT_NONCE_DIRECTIVES = %w[script-src style-src].freeze - - private_constant :MAPPINGS, :DIRECTIVES, :DEFAULT_NONCE_DIRECTIVES - - attr_reader :directives - def initialize - @directives = {} yield self if block_given? end - def initialize_copy(other) - @directives = other.directives.deep_dup - end - - DIRECTIVES.each do |name, directive| - define_method(name) do |*sources| - if sources.first - @directives[directive] = apply_mappings(sources) - else - @directives.delete(directive) - end - end + def build(content_type, *args) + format = case content_type + when /html/ + html + when /json/ + json + else + html + end + format.build(*args) end def html - yield self - end - - # TODO: this isn't going to pan out - def json - yield self - end - - def block_all_mixed_content(enabled = true) - if enabled - @directives["block-all-mixed-content"] = true + @html ||= Format.new + if block_given? + yield @html else - @directives.delete("block-all-mixed-content") + @html end end - def plugin_types(*types) - if types.first - @directives["plugin-types"] = types + def json + @json ||= Format.new + if block_given? + yield @json else - @directives.delete("plugin-types") + @json end end - def report_uri(uri) - @directives["report-uri"] = [uri] + def method_missing(method, *args, &block) + html.send(method, *args, &block) end - def require_sri_for(*types) - if types.first - @directives["require-sri-for"] = types - else - @directives.delete("require-sri-for") - end + def respond_to_missing?(method, include_private = false) + html.respond_to?(method, include_private) end - def sandbox(*values) - if values.empty? - @directives["sandbox"] = true - elsif values.first - @directives["sandbox"] = values - else - @directives.delete("sandbox") + class Format + MAPPINGS = { + self: "'self'", + unsafe_eval: "'unsafe-eval'", + unsafe_inline: "'unsafe-inline'", + none: "'none'", + http: "http:", + https: "https:", + data: "data:", + mediastream: "mediastream:", + blob: "blob:", + filesystem: "filesystem:", + report_sample: "'report-sample'", + strict_dynamic: "'strict-dynamic'", + ws: "ws:", + wss: "wss:" + }.freeze + + DIRECTIVES = { + base_uri: "base-uri", + child_src: "child-src", + connect_src: "connect-src", + default_src: "default-src", + font_src: "font-src", + form_action: "form-action", + frame_ancestors: "frame-ancestors", + frame_src: "frame-src", + img_src: "img-src", + manifest_src: "manifest-src", + media_src: "media-src", + object_src: "object-src", + prefetch_src: "prefetch-src", + script_src: "script-src", + script_src_attr: "script-src-attr", + script_src_elem: "script-src-elem", + style_src: "style-src", + style_src_attr: "style-src-attr", + style_src_elem: "style-src-elem", + worker_src: "worker-src" + }.freeze + + DEFAULT_NONCE_DIRECTIVES = %w[script-src style-src].freeze + + private_constant :MAPPINGS, :DIRECTIVES, :DEFAULT_NONCE_DIRECTIVES + + attr_reader :directives + + def initialize + @directives = {} + yield self if block_given? end - end - def upgrade_insecure_requests(enabled = true) - if enabled - @directives["upgrade-insecure-requests"] = true - else - @directives.delete("upgrade-insecure-requests") + def initialize_copy(other) + @directives = other.directives.deep_dup end - end - - def build(context = nil, nonce = nil, nonce_directives = nil) - nonce_directives = DEFAULT_NONCE_DIRECTIVES if nonce_directives.nil? - build_directives(context, nonce, nonce_directives).compact.join("; ") - end - private - def apply_mappings(sources) - sources.map do |source| - case source - when Symbol - apply_mapping(source) - when String, Proc - source + DIRECTIVES.each do |name, directive| + define_method(name) do |*sources| + if sources.first + @directives[directive] = apply_mappings(sources) else - raise ArgumentError, "Invalid content security policy source: #{source.inspect}" + @directives.delete(directive) end end end - def apply_mapping(source) - MAPPINGS.fetch(source) do - raise ArgumentError, "Unknown content security policy source mapping: #{source.inspect}" + def block_all_mixed_content(enabled = true) + if enabled + @directives["block-all-mixed-content"] = true + else + @directives.delete("block-all-mixed-content") end end - def build_directives(context, nonce, nonce_directives) - @directives.map do |directive, sources| - if sources.is_a?(Array) - if nonce && nonce_directive?(directive, nonce_directives) - "#{directive} #{build_directive(sources, context).join(' ')} 'nonce-#{nonce}'" - else - "#{directive} #{build_directive(sources, context).join(' ')}" - end - elsif sources - directive - else - nil - end + def plugin_types(*types) + if types.first + @directives["plugin-types"] = types + else + @directives.delete("plugin-types") end end - def build_directive(sources, context) - sources.map { |source| resolve_source(source, context) } + def report_uri(uri) + @directives["report-uri"] = [uri] end - def resolve_source(source, context) - case source - when String - source - when Symbol - source.to_s - when Proc - if context.nil? - raise RuntimeError, "Missing context for the dynamic content security policy source: #{source.inspect}" - else - resolved = context.instance_exec(&source) - resolved.is_a?(Symbol) ? apply_mapping(resolved) : resolved - end + def require_sri_for(*types) + if types.first + @directives["require-sri-for"] = types else - raise RuntimeError, "Unexpected content security policy source: #{source.inspect}" + @directives.delete("require-sri-for") end end - def nonce_directive?(directive, nonce_directives) - nonce_directives.include?(directive) + def sandbox(*values) + if values.empty? + @directives["sandbox"] = true + elsif values.first + @directives["sandbox"] = values + else + @directives.delete("sandbox") + end end + + def upgrade_insecure_requests(enabled = true) + if enabled + @directives["upgrade-insecure-requests"] = true + else + @directives.delete("upgrade-insecure-requests") + end + end + + def build(context = nil, nonce = nil, nonce_directives = nil) + nonce_directives = DEFAULT_NONCE_DIRECTIVES if nonce_directives.nil? + build_directives(context, nonce, nonce_directives).compact.join("; ") + end + + private + def apply_mappings(sources) + sources.map do |source| + case source + when Symbol + apply_mapping(source) + when String, Proc + source + else + raise ArgumentError, "Invalid content security policy source: #{source.inspect}" + end + end + end + + def apply_mapping(source) + MAPPINGS.fetch(source) do + raise ArgumentError, "Unknown content security policy source mapping: #{source.inspect}" + end + end + + def build_directives(context, nonce, nonce_directives) + @directives.map do |directive, sources| + if sources.is_a?(Array) + if nonce && nonce_directive?(directive, nonce_directives) + "#{directive} #{build_directive(sources, context).join(' ')} 'nonce-#{nonce}'" + else + "#{directive} #{build_directive(sources, context).join(' ')}" + end + elsif sources + directive + else + nil + end + end + end + + def build_directive(sources, context) + sources.map { |source| resolve_source(source, context) } + end + + def resolve_source(source, context) + case source + when String + source + when Symbol + source.to_s + when Proc + if context.nil? + raise RuntimeError, "Missing context for the dynamic content security policy source: #{source.inspect}" + else + resolved = context.instance_exec(&source) + resolved.is_a?(Symbol) ? apply_mapping(resolved) : resolved + end + else + raise RuntimeError, "Unexpected content security policy source: #{source.inspect}" + end + end + + def nonce_directive?(directive, nonce_directives) + nonce_directives.include?(directive) + end + end end end diff --git a/railties/test/application/content_security_policy_test.rb b/railties/test/application/content_security_policy_test.rb index 3b3a5dcf65d5c..7205acae5e72a 100644 --- a/railties/test/application/content_security_policy_test.rb +++ b/railties/test/application/content_security_policy_test.rb @@ -476,6 +476,46 @@ def index assert_policy "default-src 'none'" end + test "global content security policy for mixed formats in an initializer" do + controller :pages, <<-RUBY + class PagesController < ApplicationController + def index + render html: "

Welcome to Rails!

" + end + + def api + render json: { some: "json" } + end + end + RUBY + + app_file "config/initializers/content_security_policy.rb", <<-RUBY + Rails.application.config.content_security_policy do |format| + format.html do |p| + p.default_src :self, :https + end + + format.json do |p| + p.default_src :none + end + end + RUBY + + app_file "config/routes.rb", <<-RUBY + Rails.application.routes.draw do + root to: "pages#index" + get "/api", to: "pages#api" + end + RUBY + + app("development") + + get "/" + assert_policy "default-src 'self' https:" + get "/api" + assert_policy "default-src 'none'" + end + private def assert_policy(expected, report_only: false) assert_equal 200, last_response.status From 07317225d87b21d43cbc7c55f1c21970a1a5273b Mon Sep 17 00:00:00 2001 From: Tim Wade Date: Mon, 6 Jul 2020 16:56:19 -0700 Subject: [PATCH 06/12] Parse the content_type --- .../lib/action_dispatch/http/content_security_policy.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/actionpack/lib/action_dispatch/http/content_security_policy.rb b/actionpack/lib/action_dispatch/http/content_security_policy.rb index d3a12326690dc..98f0458fe8639 100644 --- a/actionpack/lib/action_dispatch/http/content_security_policy.rb +++ b/actionpack/lib/action_dispatch/http/content_security_policy.rb @@ -103,10 +103,11 @@ def initialize end def build(content_type, *args) - format = case content_type - when /html/ + mime_type = Mime::Type.parse(content_type).first + format = case mime_type.to_sym + when :html html - when /json/ + when :json json else html From 418fa0ba1258eefa4158e6b48c22943dabe6c1fd Mon Sep 17 00:00:00 2001 From: Tim Wade Date: Wed, 8 Jul 2020 16:48:21 -0700 Subject: [PATCH 07/12] Some metaprogramming --- .../http/content_security_policy.rb | 43 ++++++++----------- .../dispatch/content_security_policy_test.rb | 8 ++-- 2 files changed, 22 insertions(+), 29 deletions(-) diff --git a/actionpack/lib/action_dispatch/http/content_security_policy.rb b/actionpack/lib/action_dispatch/http/content_security_policy.rb index 98f0458fe8639..cfa8f0e274517 100644 --- a/actionpack/lib/action_dispatch/http/content_security_policy.rb +++ b/actionpack/lib/action_dispatch/http/content_security_policy.rb @@ -23,7 +23,7 @@ def call(env) nonce = request.content_security_policy_nonce nonce_directives = request.content_security_policy_nonce_directives context = request.controller_instance || request - headers[header_name(request)] = policy.build(headers[CONTENT_TYPE], context, nonce, nonce_directives) + headers[header_name(request)] = policy.build(context, nonce, nonce_directives, headers[CONTENT_TYPE]) end response @@ -98,39 +98,30 @@ def generate_content_security_policy_nonce end end + FORMATS = Hash.new { |h,k| h[k] = Format.new } + def initialize yield self if block_given? end - def build(content_type, *args) + def build(context = nil, nonce = nil, nonce_directives = nil, content_type = "text/html") mime_type = Mime::Type.parse(content_type).first - format = case mime_type.to_sym - when :html - html - when :json - json - else - html - end - format.build(*args) + format = FORMATS.fetch(mime_type.to_sym, FORMATS[:html]) + format.build(context, nonce, nonce_directives) end - def html - @html ||= Format.new - if block_given? - yield @html - else - @html - end - end + (Mime::SET.map(&:to_sym) + [:any]).each do |type| + class_eval(<<-METHOD, __FILE__, __LINE__ + 1) + def #{type} + FORMATS[:#{type}] ||= Format.new - def json - @json ||= Format.new - if block_given? - yield @json - else - @json - end + if block_given? + yield FORMATS[:#{type}] + else + FORMATS[:#{type}] + end + end + METHOD end def method_missing(method, *args, &block) diff --git a/actionpack/test/dispatch/content_security_policy_test.rb b/actionpack/test/dispatch/content_security_policy_test.rb index 2facbbf1e945b..ce6333cd04651 100644 --- a/actionpack/test/dispatch/content_security_policy_test.rb +++ b/actionpack/test/dispatch/content_security_policy_test.rb @@ -377,9 +377,11 @@ class PolicyController < ActionController::Base content_security_policy_report_only only: :report_only - content_security_policy only: :api do |p| - p.default_src :none - p.frame_ancestors :none + content_security_policy only: :api do |format| + format.json do |p| + p.default_src :none + p.frame_ancestors :none + end end def index From 01059396633a1bf826dff25febdb49cf8bd4b034 Mon Sep 17 00:00:00 2001 From: Tim Wade Date: Thu, 9 Jul 2020 09:40:14 -0700 Subject: [PATCH 08/12] Refactory --- .../http/content_security_policy.rb | 31 ++++++++++--------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/actionpack/lib/action_dispatch/http/content_security_policy.rb b/actionpack/lib/action_dispatch/http/content_security_policy.rb index cfa8f0e274517..69043ae96892b 100644 --- a/actionpack/lib/action_dispatch/http/content_security_policy.rb +++ b/actionpack/lib/action_dispatch/http/content_security_policy.rb @@ -98,38 +98,39 @@ def generate_content_security_policy_nonce end end - FORMATS = Hash.new { |h,k| h[k] = Format.new } - def initialize + @formats = Hash.new { |h,k| h[k] = Format.new } yield self if block_given? end - def build(context = nil, nonce = nil, nonce_directives = nil, content_type = "text/html") - mime_type = Mime::Type.parse(content_type).first - format = FORMATS.fetch(mime_type.to_sym, FORMATS[:html]) - format.build(context, nonce, nonce_directives) - end - (Mime::SET.map(&:to_sym) + [:any]).each do |type| class_eval(<<-METHOD, __FILE__, __LINE__ + 1) def #{type} - FORMATS[:#{type}] ||= Format.new - if block_given? - yield FORMATS[:#{type}] + yield @formats[:#{type}] else - FORMATS[:#{type}] + @formats[:#{type}] end end METHOD end - def method_missing(method, *args, &block) - html.send(method, *args, &block) + def build(context = nil, nonce = nil, nonce_directives = nil, content_type = "text/html") + mime_type = Mime::Type.parse(content_type).first + format = @formats.fetch(mime_type.to_sym, @formats[:html]) + format.build(context, nonce, nonce_directives) + end + + def method_missing(method, *args) + if html.respond_to?(method) + html.send(method, *args) + else + super + end end def respond_to_missing?(method, include_private = false) - html.respond_to?(method, include_private) + html.respond_to?(method, include_private) || super end class Format From 9a7c51393a818b709a8a6e2e941b0432cc5b2456 Mon Sep 17 00:00:00 2001 From: Tim Wade Date: Thu, 9 Jul 2020 09:43:49 -0700 Subject: [PATCH 09/12] ...and we're green --- .../lib/action_dispatch/http/content_security_policy.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/actionpack/lib/action_dispatch/http/content_security_policy.rb b/actionpack/lib/action_dispatch/http/content_security_policy.rb index 69043ae96892b..29d737ada6330 100644 --- a/actionpack/lib/action_dispatch/http/content_security_policy.rb +++ b/actionpack/lib/action_dispatch/http/content_security_policy.rb @@ -98,11 +98,17 @@ def generate_content_security_policy_nonce end end + attr_reader :formats + def initialize @formats = Hash.new { |h,k| h[k] = Format.new } yield self if block_given? end + def initialize_copy(other) + @formats = other.formats.deep_dup + end + (Mime::SET.map(&:to_sym) + [:any]).each do |type| class_eval(<<-METHOD, __FILE__, __LINE__ + 1) def #{type} From d14792fb36b6147da4535e7db670c716b04b4460 Mon Sep 17 00:00:00 2001 From: Tim Wade Date: Thu, 9 Jul 2020 10:29:18 -0700 Subject: [PATCH 10/12] sort n merge --- .../http/content_security_policy.rb | 10 ++++----- .../dispatch/content_security_policy_test.rb | 21 +++++++++++++++++++ .../content_security_policy_test.rb | 8 +++++-- 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/actionpack/lib/action_dispatch/http/content_security_policy.rb b/actionpack/lib/action_dispatch/http/content_security_policy.rb index 29d737ada6330..1417d2f943c88 100644 --- a/actionpack/lib/action_dispatch/http/content_security_policy.rb +++ b/actionpack/lib/action_dispatch/http/content_security_policy.rb @@ -124,7 +124,7 @@ def #{type} def build(context = nil, nonce = nil, nonce_directives = nil, content_type = "text/html") mime_type = Mime::Type.parse(content_type).first format = @formats.fetch(mime_type.to_sym, @formats[:html]) - format.build(context, nonce, nonce_directives) + format.build(context, nonce, nonce_directives, @formats[:any].directives) end def method_missing(method, *args) @@ -251,9 +251,9 @@ def upgrade_insecure_requests(enabled = true) end end - def build(context = nil, nonce = nil, nonce_directives = nil) + def build(context = nil, nonce = nil, nonce_directives = nil, global_directives = {}) nonce_directives = DEFAULT_NONCE_DIRECTIVES if nonce_directives.nil? - build_directives(context, nonce, nonce_directives).compact.join("; ") + build_directives(context, nonce, nonce_directives, global_directives).compact.join("; ") end private @@ -276,8 +276,8 @@ def apply_mapping(source) end end - def build_directives(context, nonce, nonce_directives) - @directives.map do |directive, sources| + def build_directives(context, nonce, nonce_directives, global_directives) + @directives.reverse_merge(global_directives).sort.map do |directive, sources| if sources.is_a?(Array) if nonce && nonce_directive?(directive, nonce_directives) "#{directive} #{build_directive(sources, context).join(' ')} 'nonce-#{nonce}'" diff --git a/actionpack/test/dispatch/content_security_policy_test.rb b/actionpack/test/dispatch/content_security_policy_test.rb index ce6333cd04651..033a7604dcde7 100644 --- a/actionpack/test/dispatch/content_security_policy_test.rb +++ b/actionpack/test/dispatch/content_security_policy_test.rb @@ -384,6 +384,17 @@ class PolicyController < ActionController::Base end end + content_security_policy only: :merged do |format| + format.html do |p| + p.default_src :self + end + + format.any do |p| + p.default_src :none + p.frame_ancestors :none + end + end + def index head :ok end @@ -416,6 +427,10 @@ def api render json: {} end + def merged + head :ok + end + private def condition? params[:condition] == "true" @@ -433,6 +448,7 @@ def condition? get "/style-src", to: "policy#style_src" get "/no-policy", to: "policy#no_policy" get "/api", to: "policy#api" + get "/merged", to: "policy#merged" end end @@ -509,6 +525,11 @@ def test_generates_api_security_policy assert_policy "default-src 'none'; frame-ancestors 'none'" end + def test_generates_merged_security_policy + get "/merged" + assert_policy "default-src 'self'; frame-ancestors 'none'" + end + private def assert_policy(expected, report_only: false) assert_response :success diff --git a/railties/test/application/content_security_policy_test.rb b/railties/test/application/content_security_policy_test.rb index 7205acae5e72a..76b45c5f76050 100644 --- a/railties/test/application/content_security_policy_test.rb +++ b/railties/test/application/content_security_policy_test.rb @@ -498,6 +498,10 @@ def api format.json do |p| p.default_src :none end + + format.any do |p| + p.frame_ancestors :none + end end RUBY @@ -511,9 +515,9 @@ def api app("development") get "/" - assert_policy "default-src 'self' https:" + assert_policy "default-src 'self' https:; frame-ancestors 'none'" get "/api" - assert_policy "default-src 'none'" + assert_policy "default-src 'none'; frame-ancestors 'none'" end private From 8f54f18b6f2de2370c354aa7057e2ffc783efe09 Mon Sep 17 00:00:00 2001 From: Tim Wade Date: Wed, 22 Jul 2020 14:12:02 -0700 Subject: [PATCH 11/12] Warn on no policy --- .../action_dispatch/http/content_security_policy.rb | 11 ++++++++++- .../test/dispatch/content_security_policy_test.rb | 11 ++++++++++- railties/lib/rails/application/configuration.rb | 5 +++-- .../lib/rails/application/default_middleware_stack.rb | 3 ++- 4 files changed, 25 insertions(+), 5 deletions(-) diff --git a/actionpack/lib/action_dispatch/http/content_security_policy.rb b/actionpack/lib/action_dispatch/http/content_security_policy.rb index 1417d2f943c88..2b5d241bc5d72 100644 --- a/actionpack/lib/action_dispatch/http/content_security_policy.rb +++ b/actionpack/lib/action_dispatch/http/content_security_policy.rb @@ -9,8 +9,9 @@ class Middleware POLICY = "Content-Security-Policy" POLICY_REPORT_ONLY = "Content-Security-Policy-Report-Only" - def initialize(app) + def initialize(app, warn_on_no_content_security_policy = false) @app = app + @warn_on_no_content_security_policy = warn_on_no_content_security_policy end def call(env) @@ -24,6 +25,10 @@ def call(env) nonce_directives = request.content_security_policy_nonce_directives context = request.controller_instance || request headers[header_name(request)] = policy.build(context, nonce, nonce_directives, headers[CONTENT_TYPE]) + else + if (Rails.env.development? || Rails.env.test?) && logger(request) && @warn_on_no_content_security_policy + logger(request).warn "No Content Security Policy set" + end end response @@ -41,6 +46,10 @@ def header_name(request) def policy_present?(headers) headers[POLICY] || headers[POLICY_REPORT_ONLY] end + + def logger(request) + request.logger + end end module Request diff --git a/actionpack/test/dispatch/content_security_policy_test.rb b/actionpack/test/dispatch/content_security_policy_test.rb index 033a7604dcde7..73c9f14e3932e 100644 --- a/actionpack/test/dispatch/content_security_policy_test.rb +++ b/actionpack/test/dispatch/content_security_policy_test.rb @@ -473,7 +473,7 @@ def call(env) APP = build_app(ROUTES) do |middleware| middleware.use PolicyConfigMiddleware - middleware.use ActionDispatch::ContentSecurityPolicy::Middleware + middleware.use ActionDispatch::ContentSecurityPolicy::Middleware, true end def app @@ -520,6 +520,15 @@ def test_generates_no_content_security_policy assert_nil response.headers["Content-Security-Policy-Report-Only"] end + def test_warns_on_no_content_security_policy + log = StringIO.new + logger = Logger.new(log) + + get "/no-policy", env: {"action_dispatch.logger" => logger} + + assert_match("No Content Security Policy set", log.string) + end + def test_generates_api_security_policy get "/api" assert_policy "default-src 'none'; frame-ancestors 'none'" diff --git a/railties/lib/rails/application/configuration.rb b/railties/lib/rails/application/configuration.rb index ffe2c83c90f95..c43ab9660c596 100644 --- a/railties/lib/rails/application/configuration.rb +++ b/railties/lib/rails/application/configuration.rb @@ -21,8 +21,8 @@ class Configuration < ::Rails::Engine::Configuration :beginning_of_week, :filter_redirect, :x, :enable_dependency_loading, :read_encrypted_secrets, :log_level, :content_security_policy_report_only, :content_security_policy_nonce_generator, :content_security_policy_nonce_directives, - :require_master_key, :credentials, :disable_sandbox, :add_autoload_paths_to_load_path, - :rake_eager_load + :warn_on_no_content_security_policy, :require_master_key, :credentials, + :disable_sandbox, :add_autoload_paths_to_load_path, :rake_eager_load attr_reader :encoding, :api_only, :loaded_config_version, :autoloader @@ -64,6 +64,7 @@ def initialize(*) @content_security_policy_report_only = false @content_security_policy_nonce_generator = nil @content_security_policy_nonce_directives = nil + @warn_on_no_content_security_policy = false @require_master_key = false @loaded_config_version = nil @credentials = ActiveSupport::OrderedOptions.new diff --git a/railties/lib/rails/application/default_middleware_stack.rb b/railties/lib/rails/application/default_middleware_stack.rb index 563290da15617..ae2a6eb560817 100644 --- a/railties/lib/rails/application/default_middleware_stack.rb +++ b/railties/lib/rails/application/default_middleware_stack.rb @@ -46,6 +46,8 @@ def build_stack middleware.use ::ActionDispatch::RequestId middleware.use ::ActionDispatch::RemoteIp, config.action_dispatch.ip_spoofing_check, config.action_dispatch.trusted_proxies + middleware.use ::ActionDispatch::ContentSecurityPolicy::Middleware, config.warn_on_no_content_security_policy + middleware.use ::Rails::Rack::Logger, config.log_tags middleware.use ::ActionDispatch::ShowExceptions, show_exceptions_app middleware.use ::ActionDispatch::DebugExceptions, app, config.debug_exception_response_format @@ -67,7 +69,6 @@ def build_stack end unless config.api_only - middleware.use ::ActionDispatch::ContentSecurityPolicy::Middleware middleware.use ::ActionDispatch::FeaturePolicy::Middleware end From 88c02f9744f6e231f25c400a2a946433a9312429 Mon Sep 17 00:00:00 2001 From: Tim Wade Date: Wed, 22 Jul 2020 14:47:55 -0700 Subject: [PATCH 12/12] Fix default middleware tests --- railties/test/application/middleware_test.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/railties/test/application/middleware_test.rb b/railties/test/application/middleware_test.rb index dcec08899148f..f2345c1239204 100644 --- a/railties/test/application/middleware_test.rb +++ b/railties/test/application/middleware_test.rb @@ -35,6 +35,7 @@ def app "Rack::MethodOverride", "ActionDispatch::RequestId", "ActionDispatch::RemoteIp", + "ActionDispatch::ContentSecurityPolicy::Middleware", "Rails::Rack::Logger", "ActionDispatch::ShowExceptions", "ActionDispatch::DebugExceptions", @@ -45,7 +46,6 @@ def app "ActionDispatch::Cookies", "ActionDispatch::Session::CookieStore", "ActionDispatch::Flash", - "ActionDispatch::ContentSecurityPolicy::Middleware", "ActionDispatch::FeaturePolicy::Middleware", "Rack::Head", "Rack::ConditionalGet", @@ -69,6 +69,7 @@ def app "Rack::Runtime", "ActionDispatch::RequestId", "ActionDispatch::RemoteIp", + "ActionDispatch::ContentSecurityPolicy::Middleware", "Rails::Rack::Logger", "ActionDispatch::ShowExceptions", "ActionDispatch::DebugExceptions",