Merge pull request #2465 from newrelic/nettleserovervåking

browser monitoring injection logic hardening

CHANGELOG.md

@@ -1,5 +1,13 @@
 # New Relic Ruby Agent Release Notes
 
+## dev
+
+Version <dev> hardens the browser agent insertion logic to better proactively anticipate errors.
+
+- **Bugfix: Harden the browser agent insertion logic**
+
+  With [Issue#2462](https://github.com/newrelic/newrelic-ruby-agent/issues/2462), community member [@miry](https://github.com/miry) explained that it was possible for an HTTP response headers hash to have symbols for values. Not only would these symbols prevent the inclusion of the New Relic browser agent tag in the response body, but more importantly they would cause an exception that would bubble up to the monitored web application itself. With [PR#2465](https://github.com/newrelic/newrelic-ruby-agent/pull/2465) symbol based values are now supported and all other potential future exceptions are now handled. Additionally, the refactor to support symbols has been shown through benchmarking to give the processing of string and mixed type hashes a slight speed boost too.
+
 
 ## v9.7.1
 

lib/new_relic/rack/browser_monitoring.rb

@@ -23,8 +23,8 @@ class BrowserMonitoring < AgentMiddleware
       CONTENT_TYPE = 'Content-Type'.freeze
       CONTENT_DISPOSITION = 'Content-Disposition'.freeze
       CONTENT_LENGTH = 'Content-Length'.freeze
-      ATTACHMENT = 'attachment'.freeze
-      TEXT_HTML = 'text/html'.freeze
+      ATTACHMENT = /attachment/.freeze
+      TEXT_HTML = %r{text/html}.freeze
 
       BODY_START = '<body'.freeze
       HEAD_START = '<head'.freeze
@@ -65,6 +65,10 @@ def should_instrument?(env, status, headers)
           html?(headers) &&
           !attachment?(headers) &&
           !streaming?(env, headers)
+      rescue StandardError => e
+        NewRelic::Agent.logger.error('RUM instrumentation applicability check failed on exception:' \
+                                     "#{e.class} - #{e.message}")
+        false
       end
 
       private
@@ -100,11 +104,11 @@ def modify_source(source, js_to_inject)
 
       def html?(headers)
         # needs else branch coverage
-        headers[CONTENT_TYPE] && headers[CONTENT_TYPE].include?(TEXT_HTML) # rubocop:disable Style/SafeNavigation
+        headers[CONTENT_TYPE]&.match?(TEXT_HTML)
       end
 
       def attachment?(headers)
-        headers[CONTENT_DISPOSITION]&.include?(ATTACHMENT)
+        headers[CONTENT_DISPOSITION]&.match?(ATTACHMENT)
       end
 
       def streaming?(env, headers)

test/new_relic/rack/browser_monitoring_test.rb

@@ -222,6 +222,28 @@ def test_content_length_set_when_response_is_nil
       assert_equal '0', headers['Content-Length']
     end
 
+    # give #should_instrument? a bogus int hash value guaranteed to raise an
+    # exception when `#match?` is called on it, and ensure that the error
+    # is caught and a boolean value still returned
+    def test_should_instrument_method_cannot_crash_the_observed_application
+      phony_logger = Minitest::Mock.new
+      phony_logger.expect :error, nil, [/applicability/]
+      NewRelic::Agent.stub :logger, phony_logger do
+        should = app.should_instrument?({}, 200, {'Content-Type' => 1138})
+
+        refute(should, 'Expected a #should_instrument? to handle errors and produce a false result')
+      end
+      phony_logger.verify
+    end
+
+    def test_html_check_works_with_symbol_based_values
+      headers = {NewRelic::Rack::BrowserMonitoring::CONTENT_TYPE => :'text/html'}
+
+      assert app.send(:html?, headers)
+    end
+
+    private
+
     def headers_from_request(headers, content)
       content = Array(content) if content