Update AppSec Settings to use the env, and type options
#2987
Conversation
GustavoCaso
force-pushed
the
update-appsec-settings-to-use-env-and-type
branch
from
c3fffa7 to
4bbad6a
Compare
Codecov Report
@@ Coverage Diff @@
## master #2987 +/- ##
=======================================
Coverage 98.08% 98.08%
=======================================
Files 1301 1301
Lines 72468 72461 -7
Branches 3349 3347 -2
=======================================
- Hits 71082 71076 -6
+ Misses 1386 1385 -1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| let(:track_user_events_mode) { :extended } | ||
| let(:track_user_events_mode) { 'extended' } |
There was a problem hiding this comment.
There's a small change related to these lines where, somewhere in our code, we are converting :extended to a String, but now we just pass the String directly.
Is this material to the application running appsec, or simply a testing artifact?
There was a problem hiding this comment.
On the first iteration, we accepted either string or symbol as a possible value for appsec.track_user_events.mode. Internally we stored it as a string, and what ever check we did against those values expected a string.
dd-trace-rb/lib/datadog/appsec/contrib/devise/event.rb
Lines 41 to 44 in 7a87710
With this change, we signal that we only support string values.
As a note, this feature hasn't ben release yet, so there are no users using it, so there is no breaking change or anything that could potentially affect users of appsec
| o.type :string | ||
| o.env 'DD_APPSEC_OBFUSCATION_PARAMETER_VALUE_REGEXP' | ||
| o.default DEFAULT_OBFUSCATOR_VALUE_REGEX |
There was a problem hiding this comment.
Not for this PR, but it kills me that we use REGEXP and REGEX lol
There was a problem hiding this comment.
I definitely relate to that feeling! These env vars typically come from external specifications.
| 'The appsec.track_user_events.mode value provided is not supported.' \ | ||
| "Supported values are: safe | extended.\n" \ | ||
| 'Using default value safe' |
There was a problem hiding this comment.
Minor: This was already like this, but since we're here, I suggest removing the newline from the log message, as a lot of logging tools can get tripped with newlines (e.g. show them out of order and whatnot), and thus having it all in a single line avoids any possible confusion.
| 'The appsec.track_user_events.mode value provided is not supported.' \ | |
| "Supported values are: safe | extended.\n" \ | |
| 'Using default value safe' | |
| 'The appsec.track_user_events.mode value provided is not supported.' \ | |
| "Supported values are: safe | extended. " \ | |
| 'Using default value safe' |
| context 'is not defined' do | ||
| let(:appsec_enabled) { nil } | ||
|
|
||
| it { is_expected.to be described_class::DEFAULT_APPSEC_ENABLED } | ||
| it { is_expected.to eq false } | ||
| end |
There was a problem hiding this comment.
👏 I really love having the defaults inlined in the tests. That's because it protects us from changing some default accidentally, or at least without considering its impact on the tests -- if the behavior of the implementation (default) changes, the tests should have to change too!
What does this PR do?
Build on top of #2983
Update the AppSec settings to use
env, andtypeoptions to improve appsec settings integrityMotivation
Additional Notes
How to test the change?