Use correct CSP nonce behavior when 'unsafe-inline' is set (#1041)

waltjones · web-flow · commit 7a71c3566c09 · 2021-05-20T12:05:40.000-04:00
* fix: use correct CSP nonce behavior when 'unsafe-inline' is set

* chore: pin rexml to ruby v2.0 compatible version for Rails 3 ci config

* style: remove historical context from comment
diff --git a/Gemfile b/Gemfile
@@ -74,6 +74,7 @@ gem 'redis'
 gem 'resque', '< 2.0.0'
 gem 'rubocop', '~> 0.93.0', :require => false
 gem 'rubocop-performance', :require => false
+gem 'secure_headers', '~> 6.3.2', :require => false
 gem 'sinatra'
 gem 'webmock', :require => false
 gemspec
diff --git a/gemfiles/rails50.gemfile b/gemfiles/rails50.gemfile
@@ -45,6 +45,7 @@ gem 'generator_spec'
 gem 'girl_friday', '>= 0.11.1'
 gem 'redis', '<= 3.3.5'
 gem 'resque'
+gem 'secure_headers', '~> 6.3.2', :require => false
 
 unless is_jruby
   # JRuby doesn't support fork, which is required for this test helper.
diff --git a/gemfiles/rails51.gemfile b/gemfiles/rails51.gemfile
@@ -46,6 +46,7 @@ gem 'generator_spec'
 gem 'girl_friday', '>= 0.11.1'
 gem 'redis', '<= 3.3.5'
 gem 'resque'
+gem 'secure_headers', '~> 6.3.2', :require => false
 
 unless is_jruby
   # JRuby doesn't support fork, which is required for this test helper.
diff --git a/gemfiles/rails52.gemfile b/gemfiles/rails52.gemfile
@@ -38,6 +38,7 @@ gem 'generator_spec'
 gem 'girl_friday', '>= 0.11.1'
 gem 'redis'
 gem 'resque'
+gem 'secure_headers', '~> 6.3.2', :require => false
 gem 'simplecov', '<= 0.17.1'
 
 unless is_jruby
diff --git a/gemfiles/rails60.gemfile b/gemfiles/rails60.gemfile
@@ -35,6 +35,7 @@ gem 'generator_spec'
 gem 'girl_friday', '>= 0.11.1'
 gem 'redis'
 gem 'resque'
+gem 'secure_headers', '~> 6.3.2', :require => false
 gem 'simplecov'
 
 unless is_jruby
diff --git a/gemfiles/rails61.gemfile b/gemfiles/rails61.gemfile
@@ -34,6 +34,7 @@ gem 'generator_spec'
 gem 'girl_friday', '>= 0.11.1'
 gem 'redis'
 gem 'resque'
+gem 'secure_headers', '~> 6.3.2', :require => false
 gem 'simplecov'
 
 unless is_jruby
diff --git a/lib/rollbar/middleware/js.rb b/lib/rollbar/middleware/js.rb
@@ -157,8 +157,7 @@ def js_snippet
       def script_tag(content, env)
         if (nonce = rails5_nonce(env))
           script_tag_content = "\n<script type=\"text/javascript\" nonce=\"#{nonce}\">#{content}</script>"
-        elsif secure_headers_nonce?
-          nonce = ::SecureHeaders.content_security_policy_script_nonce(::Rack::Request.new(env))
+        elsif (nonce = secure_headers_nonce(env))
           script_tag_content = "\n<script type=\"text/javascript\" nonce=\"#{nonce}\">#{content}</script>"
         else
           script_tag_content = "\n<script type=\"text/javascript\">#{content}</script>"
@@ -172,28 +171,40 @@ def html_safe_if_needed(string)
         string
       end
 
-      # Rails 5.2 Secure Content Policy
+      # Rails 5.2+ Secure Content Policy
       def rails5_nonce(env)
-        # The nonce is the preferred method, however 'unsafe-inline' is also possible.
-        # The app gets to decide, so we handle both. If the script_src key is missing,
-        # Rails will not add the nonce to the headers, so we should not add it either.
-        # If the 'unsafe-inline' value is present, the app should not add a nonce and
-        # we should ignore it if they do.
-        req = ::ActionDispatch::Request.new env
+        req = ::ActionDispatch::Request.new(env)
+
+        # Rails will only return a nonce if the app has set a nonce generator.
+        # So if we get a valid nonce here, we know we should use it.
+        #
+        # Having both 'unsafe-inline' and a nonce is a valid and preferred
+        # browser compatibility configuration.
+        #
+        # If the script_src key is missing, Rails will not add the nonce to the headers,
+        # so we detect this and will not add it in this case.
         req.respond_to?(:content_security_policy) &&
           req.content_security_policy &&
           req.content_security_policy.directives['script-src'] &&
           req.content_security_policy_nonce
       end
 
       # Secure Headers gem
-      def secure_headers_nonce?
-        secure_headers.append_nonce?
+      def secure_headers_nonce(env)
+        req = ::Rack::Request.new(env)
+
+        return unless secure_headers(req).append_nonce?
+
+        ::SecureHeaders.content_security_policy_script_nonce(req)
       end
 
-      def secure_headers
+      def secure_headers(req)
         return SecureHeadersFalse.new unless defined?(::SecureHeaders::Configuration)
 
+        # If the nonce key has been set, the app is using nonces for this request.
+        # If it hasn't, we shouldn't cause one to be added to script_src, so return now.
+        return SecureHeadersFalse.new unless secure_headers_nonce_key(req)
+
         config = ::SecureHeaders::Configuration
 
         secure_headers_cls = nil
@@ -211,6 +222,10 @@ def secure_headers
         secure_headers_cls.new
       end
 
+      def secure_headers_nonce_key(req)
+        defined?(::SecureHeaders::NONCE_KEY) && req.env[::SecureHeaders::NONCE_KEY]
+      end
+
       class SecureHeadersResolver
         def append_nonce?
           csp_needs_nonce?(find_csp)
diff --git a/spec/dummyapp/app/controllers/home_controller.rb b/spec/dummyapp/app/controllers/home_controller.rb
@@ -48,6 +48,13 @@ def test_rollbar_js
     render 'js/test', :layout => 'simple'
   end
 
+  def test_rollbar_js_with_nonce
+    # Cause a secure_headers nonce to be added to script_src
+    ::SecureHeaders.content_security_policy_script_nonce(request)
+
+    render 'js/test', :layout => 'simple'
+  end
+
   def file_upload
     _this = will_crash
   end
diff --git a/spec/dummyapp/config/routes.rb b/spec/dummyapp/config/routes.rb
@@ -16,4 +16,5 @@
   get '/set_session_data' => 'home#set_session_data'
   get '/use_session_data' => 'home#use_session_data'
   get '/test_rollbar_js' => 'home#test_rollbar_js'
+  get '/test_rollbar_js_with_nonce' => 'home#test_rollbar_js_with_nonce'
 end
diff --git a/spec/rollbar/middleware/js_spec.rb b/spec/rollbar/middleware/js_spec.rb
@@ -34,7 +34,7 @@
 describe Rollbar::Middleware::Js do
   subject { described_class.new(app, config) }
 
-  let(:env) { {} }
+  let(:env) { { SecureHeadersMocks::NONCE_KEY => SecureHeadersMocks::NONCE } }
   let(:config) { {} }
   let(:app) do
     proc do |_|
@@ -421,7 +421,7 @@
         let(:body) { [html] }
         let(:status) { 200 }
         let(:headers) do
-          { 
+          {
             'Content-Type' => content_type,
             'Content-Length' => html.bytesize
           }
diff --git a/spec/rollbar/plugins/rails_js_spec.rb b/spec/rollbar/plugins/rails_js_spec.rb
@@ -1,6 +1,21 @@
 require 'spec_helper'
 
 describe ApplicationController, :type => 'request' do
+  def reset_secure_headers_config
+    return unless defined?(::SecureHeaders)
+
+    config = ::SecureHeaders::Configuration.new do |config|
+      config.csp = SecureHeaders::OPT_OUT
+      config.hsts = SecureHeaders::OPT_OUT
+      config.x_frame_options = SecureHeaders::OPT_OUT
+      config.x_content_type_options = SecureHeaders::OPT_OUT
+      config.x_xss_protection = SecureHeaders::OPT_OUT
+      config.x_permitted_cross_domain_policies = SecureHeaders::OPT_OUT
+    end
+
+    ::SecureHeaders::Configuration.instance_variable_set(:@default_config, config)
+  end
+
   shared_examples 'adds the snippet' do
     it 'renders the snippet and config in the response', :type => 'request' do
       get '/test_rollbar_js'
@@ -20,37 +35,65 @@
   end
 
   context 'using no security policy' do
+    before(:all) do
+      reset_secure_headers_config # disable secure_headers gem
+    end
+
     include_examples 'adds the snippet'
   end
 
   context 'using rails5 content_security_policy',
           :if => (Gem::Version.new(Rails.version) >= Gem::Version.new('5.2.0')) do
     def configure_csp(mode)
-      Rails.application.config.content_security_policy_nonce_generator = lambda { |_| SecureRandom.base64(16) }
       if mode == :nonce_present
         nonce_present
+      elsif mode == :nonce_not_present
+        nonce_not_present
       elsif mode == :script_src_not_present
         script_src_not_present
+      elsif mode == :unsafe_inline_present
+        unsafe_inline_present
       else
         raise 'Unknown CSP mode'
       end
     end
 
     def nonce_present
-      # Rails will add the nonce to script_src automatically, when script_src is present.
+      # Rails will add the nonce to script_src when the generator is set.
+      Rails.application.config.content_security_policy_nonce_generator = lambda { |_| SecureRandom.base64(16) }
+
+      Rails.application.config.content_security_policy do |policy|
+        policy.script_src :self, :https
+      end
+    end
+
+    def nonce_not_present
+      Rails.application.config.content_security_policy_nonce_generator = nil
+
       Rails.application.config.content_security_policy do |policy|
         policy.script_src :self, :https
       end
     end
 
     def script_src_not_present
+      Rails.application.config.content_security_policy_nonce_generator = lambda { |_| SecureRandom.base64(16) }
+
       # This is a valid policy, but Rails will not apply the nonce to script_src.
       Rails.application.config.content_security_policy do |policy|
         policy.default_src :self, :https
         policy.script_src nil
       end
     end
 
+    def unsafe_inline_present
+      # Rails will add the nonce to script_src when the generator is set.
+      Rails.application.config.content_security_policy_nonce_generator = lambda { |_| SecureRandom.base64(16) }
+
+      Rails.application.config.content_security_policy do |policy|
+        policy.script_src :unsafe_inline
+      end
+    end
+
     def nonce(response)
       response.request.content_security_policy_nonce
     end
@@ -77,6 +120,10 @@ def reset_rails_config
       configure_csp(nonce_mode)
     end
 
+    before(:all) do
+      reset_secure_headers_config # disable secure_headers gem
+    end
+
     after(:all) do
       # Ensure that later test groups have a clean rails config.
       # CSP settings could interfere with some other tests.
@@ -97,6 +144,19 @@ def reset_rails_config
       include_examples 'adds the snippet'
     end
 
+    context 'when nonce is not present' do
+      let(:nonce_mode) { :nonce_not_present }
+
+      it 'renders the snippet and config in the response without nonce in script tag' do
+        get '/test_rollbar_js'
+
+        expect(response.body).to_not include %[<script type="text/javascript" nonce="#{nonce(response)}">]
+        expect(response.body).to include '<script type="text/javascript">'
+      end
+
+      include_examples 'adds the snippet'
+    end
+
     context 'when CSP nonce is present' do
       let(:nonce_mode) { :nonce_present }
 
@@ -109,5 +169,116 @@ def reset_rails_config
 
       include_examples 'adds the snippet'
     end
+
+    context 'when CSP nonce and unsafe_inline are present' do
+      let(:nonce_mode) { :unsafe_inline_present }
+
+      it 'renders the snippet and config in the response with nonce in script tag' do
+        get '/test_rollbar_js'
+
+        expect(response.body).to include %[<script type="text/javascript" nonce="#{nonce(response)}">]
+        expect(response.body).to_not include '<script type="text/javascript">'
+      end
+
+      include_examples 'adds the snippet'
+    end
+  end
+
+  context 'using secure_headers',
+          :if => (Gem::Version.new(Rails.version) >= Gem::Version.new('5.0.0')) do
+
+    before do
+      configure_csp(nonce_mode)
+    end
+
+    after do
+      reset_secure_headers_config
+    end
+
+    def configure_csp(mode)
+      return unless defined?(::SecureHeaders)
+      if mode == :nonce_present
+        nonce_present
+      elsif mode == :nonce_not_present
+        nonce_not_present
+      elsif mode == :unsafe_inline_present
+        unsafe_inline_present
+      else
+        raise 'Unknown CSP mode'
+      end
+    end
+
+    def nonce_present
+      config = ::SecureHeaders::Configuration.new do |config|
+        config.csp = {
+          :default_src => %w('none'),
+          :script_src => %w('self')
+        }
+      end
+      ::SecureHeaders::Configuration.instance_variable_set(:@default_config, config)
+    end
+
+    def nonce_not_present
+      config = ::SecureHeaders::Configuration.new do |config|
+        config.csp = {
+          :default_src => %w('none'),
+          :script_src => %w('self')
+        }
+      end
+      ::SecureHeaders::Configuration.instance_variable_set(:@default_config, config)
+    end
+
+    def unsafe_inline_present
+      config = ::SecureHeaders::Configuration.new do |config|
+        config.csp = {
+          :default_src => %w('none'),
+          :script_src => %w('unsafe-inline')
+        }
+      end
+      ::SecureHeaders::Configuration.instance_variable_set(:@default_config, config)
+    end
+
+    def nonce(response)
+      ::SecureHeaders.content_security_policy_script_nonce(response.request)
+    end
+
+    context 'when nonce is not present' do
+      let(:nonce_mode) { :nonce_not_present }
+
+      it 'renders the snippet and config in the response without nonce in script tag' do
+        get '/test_rollbar_js'
+
+        expect(response.body).to_not include %[<script type="text/javascript" nonce="#{nonce(response)}">]
+        expect(response.body).to include '<script type="text/javascript">'
+      end
+
+      include_examples 'adds the snippet'
+    end
+
+    context 'when nonce is present' do
+      let(:nonce_mode) { :nonce_present }
+
+      it 'renders the snippet and config in the response with nonce in script tag' do
+        get '/test_rollbar_js_with_nonce'
+
+        expect(response.body).to include %[<script type="text/javascript" nonce="#{nonce(response)}">]
+        expect(response.body).to_not include '<script type="text/javascript">'
+      end
+
+      include_examples 'adds the snippet'
+    end
+
+    context 'when CSP nonce and unsafe_inline are present' do
+      let(:nonce_mode) { :unsafe_inline_present }
+
+      it 'renders the snippet and config in the response with nonce in script tag' do
+        get '/test_rollbar_js_with_nonce'
+
+        expect(response.body).to include %[<script type="text/javascript" nonce="#{nonce(response)}">]
+        expect(response.body).to_not include '<script type="text/javascript">'
+      end
+
+      include_examples 'adds the snippet'
+    end
   end
 end
diff --git a/spec/support/secure_headers.rb b/spec/support/secure_headers.rb
diff --git a/spec/support/secure_headers_mocks.rb b/spec/support/secure_headers_mocks.rb
