From 799d12bb40d620b6e5de88f215c8597350ca3ea3 Mon Sep 17 00:00:00 2001
From: fallwith <fallwith@gmail.com>
Date: Tue, 27 Feb 2024 00:59:49 -0800
Subject: [PATCH] browser monitoring injection logic hardening

These changes introduce hardening to the logic that determines whether
or not to inject the browser agent script tag.

In particular, the following 2 issues have been addressed:

1. If either `#traced_call` or `#should_instrument?` encounter any
   exceptions, these exceptions should be handled and logged without
   impacting the observed web application.
2. Allow a response headers hash to use symbols for values.

Closes #2462
---
 lib/new_relic/rack/browser_monitoring.rb      | 15 ++++++++---
 .../new_relic/rack/browser_monitoring_test.rb | 26 +++++++++++++++++++
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/lib/new_relic/rack/browser_monitoring.rb b/lib/new_relic/rack/browser_monitoring.rb
index 07cc598b06..52fce87879 100644
--- a/lib/new_relic/rack/browser_monitoring.rb
+++ b/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
@@ -56,6 +56,9 @@ def traced_call(env)
         else
           result
         end
+      rescue StandardError => e
+        NewRelic::Agent.logger.error("RUM instrumentation traced call failed on exception: #{e.class} - #{e.message}")
+        result
       end
 
       def should_instrument?(env, status, headers)
@@ -65,6 +68,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 +107,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)
diff --git a/test/new_relic/rack/browser_monitoring_test.rb b/test/new_relic/rack/browser_monitoring_test.rb
index 1555f5aa1c..f4cb736f57 100644
--- a/test/new_relic/rack/browser_monitoring_test.rb
+++ b/test/new_relic/rack/browser_monitoring_test.rb
@@ -222,6 +222,32 @@ def test_content_length_set_when_response_is_nil
       assert_equal '0', headers['Content-Length']
     end
 
+    # introduce an exception in the middle of #traced_call and verify that
+    # we did not crash but instead carried on with a response
+    def test_traced_call_method_cannot_crash_the_observed_application
+      NewRelic::Agent.stub :browser_timing_header, -> { raise 'kaboom' } do
+        result = get('/')
+
+        assert result
+      end
+    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
+
+    private
+
     def headers_from_request(headers, content)
       content = Array(content) if content