Lockdown Boolean configs (#2847)

* Correct Boolean config coercion 

Configs of type Boolean must contain either a boolean or a string/symbol of 'true', 'false', 'yes', 'no', 'on', or 'off'

CHANGELOG.md

@@ -57,6 +57,16 @@ Version <dev> adds Apache Kafka instrumentation for the rdkafka and ruby-kafka g
 
 [PR#2851](https://github.com/newrelic/newrelic-ruby-agent/pull/2851)
 
+- **Bugfix: Corrected Boolean coercion for `newrelic.yml` configuration**
+
+ Previously, any String assigned to New Relic configurations expecting a Boolean value were evaluated as `true`. This could lead to unexpected behavior. For example, setting `application_logging.enabled: 'false'` in `newrelic.yml` would incorrectly evaluate to `application_logging.enabled: true` due to the truthy nature of Strings. 
+
+ Now, the agent strictly interprets Boolean configuration values. It recognizes both actual Boolean values and certain Strings/Symbols:
+ - `'true'`, `'yes'`, or `'on'` (evaluates to `true`)
+ - `'false'`, `'no'`, or `'off'` (evaluates to `false`)
+
+ Any other inputs will revert to the setting's default configuration value. [PR#2847](https://github.com/newrelic/newrelic-ruby-agent/pull/2847)
+
 - **Bugfix: Jruby not saving configuration values correctly in configuration manager**
 
  Previously, a change made to fix a different JRuby bug caused the agent to not save configuration values correctly in the configuration manager when running on JRuby. This has been fixed. [PR#2848](https://github.com/newrelic/newrelic-ruby-agent/pull/2848)

lib/new_relic/agent/configuration/default_source.rb

@@ -35,6 +35,15 @@ def self.===(o)
  end
 
  class DefaultSource
+ BOOLEAN_MAP = {
+ 'true' => true,
+ 'yes' => true,
+ 'on' => true,
+ 'false' => false,
+ 'no' => false,
+ 'off' => false
+ }.freeze
+
  attr_reader :defaults
 
  extend Forwardable
@@ -64,6 +73,12 @@ def self.allowlist_for(key)
  value_from_defaults(key, :allowlist)
  end
 
+ def self.boolean_for(key, value)
+ string_value = (value.respond_to?(:call) ? value.call : value).to_s
+
+ BOOLEAN_MAP.fetch(string_value, nil)
+ end
+
  def self.default_for(key)
  value_from_defaults(key, :default)
  end
@@ -2268,7 +2283,7 @@ def self.notify
  :description => 'Enable or disable debugging version of JavaScript agent loader for browser monitoring instrumentation.'
  },
  :'browser_monitoring.ssl_for_http' => {
- :default => nil,
+ :default => false,
  :allow_nil => true,
  :public => false,
  :type => Boolean,

lib/new_relic/agent/configuration/manager.rb

@@ -142,6 +142,9 @@ def evaluate_and_apply_transformations(key, value)
  default = enforce_allowlist(key, evaluated)
  return default if default
 
+ boolean = enforce_boolean(key, value)
+ return boolean if [true, false].include?(boolean)
+
  apply_transformations(key, evaluated)
  end
 
@@ -167,6 +170,18 @@ def enforce_allowlist(key, value)
  default
  end
 
+ def enforce_boolean(key, value)
+ type = default_source.value_from_defaults(key, :type)
+ return unless type == Boolean
+
+ bool_value = default_source.boolean_for(key, value)
+ return bool_value unless bool_value.nil?
+
+ default = default_source.default_for(key)
+ NewRelic::Agent.logger.warn "Invalid value '#{value}' for #{key}, applying default value of '#{default}'"
+ default
+ end
+
  def transform_from_default(key)
  default_source.transform_for(key)
  end

lib/new_relic/agent/javascript_instrumentor.rb

@@ -164,9 +164,8 @@ def data_for_js_agent(transaction)
 
  def add_ssl_for_http(data)
  ssl_for_http = NewRelic::Agent.config[:'browser_monitoring.ssl_for_http']
- unless ssl_for_http.nil?
- data[SSL_FOR_HTTP_KEY] = ssl_for_http
- end
+
+ data[SSL_FOR_HTTP_KEY] = ssl_for_http if ssl_for_http
  end
 
  def add_attributes(data, txn)

test/new_relic/agent/configuration/default_source_test.rb

@@ -140,12 +140,6 @@ def test_config_search_path_in_warbler
  end
  end
 
- def test_application_logging_enabled_default
- with_config(:'application_logging.enabled' => :foo) do
- assert_equal :foo, NewRelic::Agent.config['application_logging.enabled']
- end
- end
-
  def test_agent_attribute_settings_convert_comma_delimited_strings_into_an_arrays
  types = %w[transaction_tracer. transaction_events. error_collector. browser_monitoring.]
  types << ''
@@ -313,6 +307,73 @@ def test_automatic_custom_instrumentation_method_list_supports_a_comma_delmited_
  end
  end
 
+ def test_boolean_configs_accepts_yes_on_and_true_as_strings
+ key = :'send_data_on_exit'
+ config_array = %w[yes on true]
+
+ config_array.each do |value|
+ with_config(key => value) do
+ assert NewRelic::Agent.config[key], "The '#{value}' value failed to evaluate as truthy!"
+ end
+ end
+ end
+
+ def test_boolean_configs_accepts_yes_on_and_true_as_symbols
+ key = :'send_data_on_exit'
+ config_array = %i[yes on true]
+
+ config_array.each do |value|
+ with_config(key => value) do
+ assert NewRelic::Agent.config[key], "The '#{value}' value failed to evaluate as truthy!"
+ end
+ end
+ end
+
+ def test_boolean_configs_accepts_no_off_and_false_as_strings
+ key = :'send_data_on_exit'
+
+ %w[no off false].each do |value|
+ with_config(key => value) do
+ refute NewRelic::Agent.config[key], "The '#{value}' value failed to evaluate as falsey!"
+ end
+ end
+ end
+
+ def test_boolean_configs_accepts_no_off_and_false_as_strings_as_symbols
+ key = :'send_data_on_exit'
+
+ %i[no off false].each do |value|
+ with_config(key => value) do
+ refute NewRelic::Agent.config[key], "The '#{value}' value failed to evaluate as falsey!"
+ end
+ end
+ end
+
+ def test_enforce_boolean_uses_defult_on_invalid_value
+ key = :'send_data_on_exit' # default value is `true`
+
+ with_config(key => 'invalid_value') do
+ assert NewRelic::Agent.config[key]
+ end
+ end
+
+ def test_enforce_boolean_logs_warning_on_invalid_value
+ key = :'send_data_on_exit'
+ default = ::NewRelic::Agent::Configuration::DefaultSource.default_for(key)
+
+ with_config(key => 'yikes!') do
+ expects_logging(:warn, includes("Invalid value 'yikes!' for #{key}, applying default value of '#{default}'"))
+ end
+ end
+
+ def test_boolean_config_evaluates_proc_configs
+ key = :agent_enabled # default value is a proc
+
+ with_config(key => 'off') do
+ refute NewRelic::Agent.config[key]
+ end
+ end
+
  def get_config_value_class(value)
  type = value.class