Do not try accessing the appsec settings if the appsec module is not loaded #2679

GustavoCaso · 2023-03-09T17:44:33Z

The issue is that trying to access settings.appsec when configuring Datadog is that if the appsec hasn't being required or if the customer did not require ddtrace the error we would get is undefined method :appsec for #Datadog::Core::Configuration::Settings`

The appsec setting is loaded here:

dd-trace-rb/lib/ddtrace.rb

Line 7 in 28c3c3e

require_relative 'datadog/appsec'

dd-trace-rb/lib/datadog/appsec.rb

Line 32 in 28c3c3e

Extensions.activate!

The fix makes sure to double check before if the appsec configuration module has been loaded, and if it hasn't, we will no-op.

I tested this change loading just datadog/ci in an irb session and calling Datadog.configure {}

irb -I lib -r datadog/ci
irb(main):001:0> Datadog.configure { |c| c.ci.enabled = true ; c.ci.instrument :rspec }
W, [2023-03-09T18:43:49.071225 #26424]  WARN -- ddtrace: [ddtrace] Unable to patch Datadog::CI::Contrib::RSpec::Integration (Available?: false, Loaded? false, Compatible? false, Patchable? false)
=>
#<#<Class:0x0000000107909428>:0x0000000107de8160
 @instrumented_integrations=
  {:rspec=>
    #<Datadog::CI::Contrib::RSpec::Integration:0x0000000107d41158
     @default_configuration=
      #<Datadog::CI::Contrib::RSpec::Configuration::Settings:0x0000000107de7e18
       @options=
        {:enabled=>
          #<Datadog::Core::Configuration::Option:0x0000000107fc7080
           @context=#<Datadog::CI::Contrib::RSpec::Configuration::Settings:0x0000000107de7e18 ...>,
           @definition=
            #<Datadog::Core::Configuration::OptionDefinition:0x0000000107e0a6e8
             @default=#<Proc:0x0000000107d47170 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/ci/contrib/rspec/configuration/settings.rb:13>,
             @delegate_to=nil,
             @depends_on=[],
             @lazy=true,
             @name=:enabled,
             @on_set=nil,
             @resetter=nil,
             @setter=#<Proc:0x0000000107969918 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/core/configuration/option_definition.rb:8 (lambda)>,
             @type=nil>,
           @is_set=true,
           @value=true>}>,
     @name=:rspec>},
 @integrations_pending_activation=#<Set: {}>,
 @options=
  {:test_mode=>
    #<Datadog::Core::Configuration::Option:0x0000000107fc6fe0
     @context=#<#<Class:0x0000000107909428>:0x0000000107de8160 ...>,
     @definition=
      #<Datadog::Core::Configuration::OptionDefinition:0x00000001079029e8
       @default=#<Proc:0x00000001078c8518 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/core/configuration/base.rb:31>,
       @delegate_to=nil,
       @depends_on=[],
       @lazy=true,
       @name=:test_mode,
       @on_set=nil,
       @resetter=#<Proc:0x00000001078c84f0 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/core/configuration/base.rb:34>,
       @setter=#<Proc:0x0000000107969918 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/core/configuration/option_definition.rb:8 (lambda)>,
       @type=#<Class:0x0000000107904428>>,
     @is_set=true,
     @value=
      #<#<Class:0x0000000107904428>:0x0000000107de5690
       @options=
        {:enabled=>
          #<Datadog::Core::Configuration::Option:0x0000000107fc6f90
           @context=#<#<Class:0x0000000107904428>:0x0000000107de5690 ...>,
           @definition=
            #<Datadog::Core::Configuration::OptionDefinition:0x0000000107902da8
             @default=#<Proc:0x00000001078caa70 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/tracing/configuration/settings.rb:347>,
             @delegate_to=nil,
             @depends_on=[],
             @lazy=true,
             @name=:enabled,
             @on_set=nil,
             @resetter=nil,
             @setter=#<Proc:0x0000000107969918 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/core/configuration/option_definition.rb:8 (lambda)>,
             @type=nil>,
           @is_set=true,
           @value=true>,
         :trace_flush=>
          #<Datadog::Core::Configuration::Option:0x0000000107fc6ef0
           @context=#<#<Class:0x0000000107904428>:0x0000000107de5690 ...>,
           @definition=
            #<Datadog::Core::Configuration::OptionDefinition:0x0000000107902c68
             @default=#<Proc:0x00000001078c9d78 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/tracing/configuration/settings.rb:352>,
             @delegate_to=nil,
             @depends_on=[],
             @lazy=true,
             @name=:trace_flush,
             @on_set=nil,
             @resetter=nil,
             @setter=#<Proc:0x0000000107969918 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/core/configuration/option_definition.rb:8 (lambda)>,
             @type=nil>,
           @is_set=true,
           @value=#<Datadog::CI::Flush::Finished:0x0000000107de4308>>,
         :writer_options=>
          #<Datadog::Core::Configuration::Option:0x0000000107fc6e50
           @context=#<#<Class:0x0000000107904428>:0x0000000107de5690 ...>,
           @definition=
            #<Datadog::Core::Configuration::OptionDefinition:0x0000000107902b28
             @default=#<Proc:0x00000001078c93a0 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/tracing/configuration/settings.rb:357>,
             @delegate_to=nil,
             @depends_on=[],
             @lazy=true,
             @name=:writer_options,
             @on_set=nil,
             @resetter=nil,
             @setter=#<Proc:0x0000000107969918 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/core/configuration/option_definition.rb:8 (lambda)>,
             @type=nil>,
           @is_set=true,
           @value={}>}>>,
   :transport_options=>
    #<Datadog::Core::Configuration::Option:0x0000000107fc6b80
     @context=#<#<Class:0x0000000107909428>:0x0000000107de8160 ...>,
     @definition=
      #<Datadog::Core::Configuration::OptionDefinition:0x0000000107902768
       @default=nil,
       @delegate_to=nil,
       @depends_on=[],
       @lazy=false,
       @name=:transport_options,
       @on_set=nil,
       @resetter=nil,
       @setter=#<Proc:0x0000000107969918 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/core/configuration/option_definition.rb:8 (lambda)>,
       @type=nil>,
     @is_set=true,
     @value=nil>,
   :instance=>
    #<Datadog::Core::Configuration::Option:0x0000000107fc6400
     @context=#<#<Class:0x0000000107909428>:0x0000000107de8160 ...>,
     @definition=
      #<Datadog::Core::Configuration::OptionDefinition:0x0000000107906c28
       @default=nil,
       @delegate_to=nil,
       @depends_on=[],
       @lazy=false,
       @name=:instance,
       @on_set=nil,
       @resetter=nil,
       @setter=#<Proc:0x0000000107969918 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/core/configuration/option_definition.rb:8 (lambda)>,
       @type=nil>,
     @is_set=true,
     @value=nil>,
   :enabled=>
    #<Datadog::Core::Configuration::Option:0x0000000107fc6180
     @context=#<#<Class:0x0000000107909428>:0x0000000107de8160 ...>,
     @definition=
      #<Datadog::Core::Configuration::OptionDefinition:0x0000000107906fe8
       @default=#<Proc:0x00000001048268d8 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/tracing/configuration/settings.rb:172>,
       @delegate_to=nil,
       @depends_on=[],
       @lazy=true,
       @name=:enabled,
       @on_set=nil,
       @resetter=nil,
       @setter=#<Proc:0x0000000107969918 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/core/configuration/option_definition.rb:8 (lambda)>,
       @type=nil>,
     @is_set=true,
     @value=true>,
   :sampling=>
    #<Datadog::Core::Configuration::Option:0x0000000107fc6130
     @context=#<#<Class:0x0000000107909428>:0x0000000107de8160 ...>,
     @definition=
      #<Datadog::Core::Configuration::OptionDefinition:0x00000001079044c8
       @default=#<Proc:0x00000001078cb5b0 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/core/configuration/base.rb:31>,
       @delegate_to=nil,
       @depends_on=[],
       @lazy=true,
       @name=:sampling,
       @on_set=nil,
       @resetter=#<Proc:0x00000001078cb588 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/core/configuration/base.rb:34>,
       @setter=#<Proc:0x0000000107969918 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/core/configuration/option_definition.rb:8 (lambda)>,
       @type=#<Class:0x00000001079051e8>>,
     @is_set=true,
     @value=
      #<#<Class:0x00000001079051e8>:0x0000000107ccaad0
       @options=
        {:span_rules=>
          #<Datadog::Core::Configuration::Option:0x0000000107fc60e0
           @context=#<#<Class:0x00000001079051e8>:0x0000000107ccaad0 ...>,
           @definition=
            #<Datadog::Core::Configuration::OptionDefinition:0x00000001079046a8
             @default=#<Proc:0x00000001078cb920 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/tracing/configuration/settings.rb:306>,
             @delegate_to=nil,
             @depends_on=[],
             @lazy=true,
             @name=:span_rules,
             @on_set=nil,
             @resetter=nil,
             @setter=#<Proc:0x0000000107969918 /Users/gustavo.caso/src/github.com/DataDog/dd-trace-rb/lib/datadog/core/configuration/option_definition.rb:8 (lambda)>,
             @type=nil>,
           @is_set=true,
           @value=nil>}>>}>
irb(main):002:0>

…loaded

ivoanjo

👍 Left a note and a suggestion of how to test this, but otherwise big thanks for the fast fix!

ivoanjo · 2023-03-09T19:12:11Z

lib/datadog/appsec/component.rb

-          return unless settings.enabled
+          return unless defined?(Datadog::AppSec::Configuration) && settings.appsec.enabled


I initially suggested defined?, but looking at the code here, I think we could probably get away with respond_to?(:appsec).

That would probably make it way easier to test -- we could just pass in a mock settings object that did not have an appsec entry.

That makes a lot of sense. I like the idea of being able to test that functionality as well.

Thanks for the suggestion 😄

…the corner case

codecov-commenter · 2023-03-10T07:52:00Z

Codecov Report

Merging #2679 (b11eaaa) into master (47ad37d) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2679      +/-   ##
==========================================
- Coverage   98.09%   98.09%   -0.01%     
==========================================
  Files        1168     1168              
  Lines       64216    64222       +6     
  Branches     2857     2857              
==========================================
+ Hits        62994    62999       +5     
- Misses       1222     1223       +1

Impacted Files	Coverage Δ
lib/datadog/appsec/component.rb	`100.00% <100.00%> (ø)`
lib/datadog/core/configuration/components.rb	`100.00% <100.00%> (ø)`
spec/datadog/appsec/component_spec.rb	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ivoanjo

👍 Looks great!

github-actions bot added appsec Application Security monitoring product core Involves Datadog core libraries labels Mar 9, 2023

GustavoCaso changed the title ~~Do not try accessing the appsec settings if the appsec module is not …~~ Do not try accessing the appsec settings if the appsec module is not loaded Mar 9, 2023

Do not try accessing the appsec settings if the appsec module is not …

397a1f3

…loaded

GustavoCaso force-pushed the appsec-make-sure-appsec-is-loaded-when-creating-appsec-component branch from 3b3eb80 to 397a1f3 Compare March 9, 2023 17:52

marcotc approved these changes Mar 9, 2023

View reviewed changes

GustavoCaso marked this pull request as ready for review March 9, 2023 18:35

GustavoCaso requested a review from a team March 9, 2023 18:35

ivoanjo approved these changes Mar 9, 2023

View reviewed changes

use respond_to? rather than defined?. It allow us to better test …

b11eaaa

…the corner case

ivoanjo approved these changes Mar 10, 2023

View reviewed changes

GustavoCaso merged commit bfbc8f6 into master Mar 10, 2023

GustavoCaso deleted the appsec-make-sure-appsec-is-loaded-when-creating-appsec-component branch March 10, 2023 09:20

github-actions bot added this to the 1.11.0 milestone Mar 10, 2023

lloeki modified the milestones: 1.11.0, 1.10.1 Mar 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not try accessing the appsec settings if the appsec module is not loaded #2679

Do not try accessing the appsec settings if the appsec module is not loaded #2679

GustavoCaso commented Mar 9, 2023 •

edited

Loading

ivoanjo left a comment

ivoanjo Mar 9, 2023

GustavoCaso Mar 10, 2023

codecov-commenter commented Mar 10, 2023

ivoanjo left a comment

		return unless settings.enabled
		return unless defined?(Datadog::AppSec::Configuration) && settings.appsec.enabled

Do not try accessing the appsec settings if the appsec module is not loaded #2679

Do not try accessing the appsec settings if the appsec module is not loaded #2679

Conversation

GustavoCaso commented Mar 9, 2023 • edited Loading

ivoanjo left a comment

Choose a reason for hiding this comment

ivoanjo Mar 9, 2023

Choose a reason for hiding this comment

GustavoCaso Mar 10, 2023

Choose a reason for hiding this comment

codecov-commenter commented Mar 10, 2023

Codecov Report

ivoanjo left a comment

Choose a reason for hiding this comment

GustavoCaso commented Mar 9, 2023 •

edited

Loading