From 385260590f9851b9f48c678b3d4313fce712c244 Mon Sep 17 00:00:00 2001 From: Dillon Welch Date: Mon, 19 Mar 2018 21:30:38 -0700 Subject: Only create an array with default options if we have default options MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the options passed in don't have a default key, there's no point in creating an array from those empty results when we can just go straight to creating an empty array. Benchmarks: ```ruby master_version with false {:FREE=>-2497, :T_STRING=>52, :T_ARRAY=>2000, :T_HASH=>1000, :T_IMEMO=>1} master_version with true {:FREE=>-3001, :T_ARRAY=>2000, :T_HASH=>1000} fast_version with false {:FREE=>-1001, :T_ARRAY=>1000} fast_version with true {:FREE=>-3001, :T_ARRAY=>2000, :T_HASH=>1000} Warming up -------------------------------------- master_version with false 104.985k i/100ms master_version with true 118.737k i/100ms fast_version with false 206.013k i/100ms fast_version with true 107.005k i/100ms Calculating ------------------------------------- master_version with false 1.970M (±24.6%) i/s - 8.924M in 5.010302s master_version with true 2.152M (±12.4%) i/s - 10.686M in 5.051588s fast_version with false 5.613M (±19.6%) i/s - 26.782M in 5.003740s fast_version with true 2.027M (±15.8%) i/s - 9.951M in 5.065670s Comparison: fast_version with false: 5613159.2 i/s master_version with true: 2152354.4 i/s - 2.61x slower fast_version with true: 2027296.0 i/s - 2.77x slower master_version with false: 1969824.9 i/s - 2.85x slower ``` Benchmark code: ```ruby begin require "bundler/inline" rescue LoadError => e $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler" raise e end gemfile(true) do source "https://rubygems.org" gem "benchmark-ips" gem "rails" end def allocate_count GC.disable before = ObjectSpace.count_objects yield after = ObjectSpace.count_objects after.each { |k,v| after[k] = v - before[k] } after[:T_HASH] -= 1 # probe effect - we created the before hash. GC.enable result = after.reject { |k,v| v == 0 } GC.start result end def master_version(key) Array({}.delete(:default)).compact end def fast_version(key) if key Array({}.delete(:default)).compact else [] end end def test puts "master_version with false" puts allocate_count { 1000.times { master_version(false) } } puts "master_version with true" puts allocate_count { 1000.times { master_version(true) } } puts "fast_version with false" puts allocate_count { 1000.times { fast_version(false) } } puts "fast_version with true" puts allocate_count { 1000.times { fast_version(true) } } Benchmark.ips do |x| x.report("master_version with false") { master_version(false) } x.report("master_version with true") { master_version(true) } x.report("fast_version with false") { fast_version(false) } x.report("fast_version with true") { fast_version(true) } x.compare! end end test ``` --- actionview/lib/action_view/helpers/translation_helper.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/actionview/lib/action_view/helpers/translation_helper.rb b/actionview/lib/action_view/helpers/translation_helper.rb index 1860bc4732..3ef9cb0fc3 100644 --- a/actionview/lib/action_view/helpers/translation_helper.rb +++ b/actionview/lib/action_view/helpers/translation_helper.rb @@ -60,7 +60,11 @@ module ActionView def translate(key, options = {}) options = options.dup has_default = options.has_key?(:default) - remaining_defaults = Array(options.delete(:default)).compact + if has_default + remaining_defaults = Array(options.delete(:default)).compact + else + remaining_defaults = [] + end if has_default && !remaining_defaults.first.kind_of?(Symbol) options[:default] = remaining_defaults -- cgit v1.2.3 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 From 319248c04916f3d89bcc77ffa84cf79b5fd2fee9 Mon Sep 17 00:00:00 2001 From: Paul <22789+thoughtless@users.noreply.github.com> Date: Tue, 17 Apr 2018 12:29:24 -0700 Subject: Correct docs on naming of sprockets manifest file Calling this an MD5 implies that it is generated consistently based on some input. However, this value is [completely random](https://github.com/rails/sprockets/blob/fbe6e450b6f25cf3ea494fcab0e34001d0b5a0b9/lib/sprockets/manifest_utils.rb#L11-L24). --- guides/source/asset_pipeline.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/guides/source/asset_pipeline.md b/guides/source/asset_pipeline.md index 2f5854fed0..88b87b78d2 100644 --- a/guides/source/asset_pipeline.md +++ b/guides/source/asset_pipeline.md @@ -728,8 +728,8 @@ Rails.application.config.assets.precompile += %w( admin.js admin.css ) NOTE. Always specify an expected compiled filename that ends with `.js` or `.css`, even if you want to add Sass or CoffeeScript files to the precompile array. -The task also generates a `.sprockets-manifest-md5hash.json` (where `md5hash` is -an MD5 hash) that contains a list with all your assets and their respective +The task also generates a `.sprockets-manifest-randomhex.json` (where `randomhex` is +a 16-byte random hex string) that contains a list with all your assets and their respective fingerprints. This is used by the Rails helper methods to avoid handing the mapping requests back to Sprockets. A typical manifest file looks like: -- cgit v1.2.3 From 47013a7126a92e1f2890b68e0fd2e7ba1b77c97c Mon Sep 17 00:00:00 2001 From: Yaroslav Markin Date: Tue, 17 Apr 2018 18:05:12 +0300 Subject: Add the `nonce: true` option for `javascript_include_tag` helper. --- actionview/CHANGELOG.md | 6 ++++++ actionview/lib/action_view/helpers/asset_tag_helper.rb | 8 ++++++++ actionview/test/template/asset_tag_helper_test.rb | 8 ++++++++ guides/source/security.md | 6 ++++++ 4 files changed, 28 insertions(+) diff --git a/actionview/CHANGELOG.md b/actionview/CHANGELOG.md index 6bb1efb0ac..d833f9cd98 100644 --- a/actionview/CHANGELOG.md +++ b/actionview/CHANGELOG.md @@ -1,3 +1,9 @@ +* Add the `nonce: true` option for `javascript_include_tag` helper to + support automatic nonce generation for Content Security Policy. + Works the same way as `javascript_tag nonce: true` does. + + *Yaroslav Markin* + * Remove `ActionView::Helpers::RecordTagHelper`. *Yoshiyuki Hirano* diff --git a/actionview/lib/action_view/helpers/asset_tag_helper.rb b/actionview/lib/action_view/helpers/asset_tag_helper.rb index 06fa1875fc..257080d902 100644 --- a/actionview/lib/action_view/helpers/asset_tag_helper.rb +++ b/actionview/lib/action_view/helpers/asset_tag_helper.rb @@ -55,6 +55,8 @@ module ActionView # that path. # * :skip_pipeline - This option is used to bypass the asset pipeline # when it is set to true. + # * :nonce - When set to true, adds an automatic nonce value if + # you have Content Security Policy enabled. # # ==== Examples # @@ -79,6 +81,9 @@ module ActionView # # javascript_include_tag "http://www.example.com/xmlhr.js" # # => + # + # javascript_include_tag "http://www.example.com/xmlhr.js", nonce: true + # # => def javascript_include_tag(*sources) options = sources.extract_options!.stringify_keys path_options = options.extract!("protocol", "extname", "host", "skip_pipeline").symbolize_keys @@ -90,6 +95,9 @@ module ActionView tag_options = { "src" => href }.merge!(options) + if tag_options["nonce"] == true + tag_options["nonce"] = content_security_policy_nonce + end content_tag("script".freeze, "", tag_options) }.join("\n").html_safe diff --git a/actionview/test/template/asset_tag_helper_test.rb b/actionview/test/template/asset_tag_helper_test.rb index 6d98eacfb8..e68f03d1f4 100644 --- a/actionview/test/template/asset_tag_helper_test.rb +++ b/actionview/test/template/asset_tag_helper_test.rb @@ -29,6 +29,10 @@ class AssetTagHelperTest < ActionView::TestCase "http://www.example.com" end + def content_security_policy_nonce + "iyhD0Yc0W+c=" + end + AssetPathToTag = { %(asset_path("")) => %(), %(asset_path(" ")) => %(), @@ -421,6 +425,10 @@ class AssetTagHelperTest < ActionView::TestCase assert_dom_equal %(), javascript_include_tag("prototype") end + def test_javascript_include_tag_nonce + assert_dom_equal %(), javascript_include_tag("bank", nonce: true) + end + def test_stylesheet_path StylePathToTag.each { |method, tag| assert_dom_equal(tag, eval(method)) } end diff --git a/guides/source/security.md b/guides/source/security.md index a21526d895..3ac50fb147 100644 --- a/guides/source/security.md +++ b/guides/source/security.md @@ -1182,6 +1182,12 @@ as part of `html_options`. Example: <% end -%> ``` +The same works with `javascript_include_tag`: + +```html+erb +<%= javascript_include_tag "script", nonce: true %> +``` + Use [`csp_meta_tag`](http://api.rubyonrails.org/classes/ActionView/Helpers/CspHelper.html#method-i-csp_meta_tag) helper to create a meta tag "csp-nonce" with the per-session nonce value for allowing inline `