-
Notifications
You must be signed in to change notification settings - Fork 377
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add AppSec::ActionHandler to unify action handling #4300
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 22064 Passed, 1477 Skipped, 5m 32.97s Total Time |
BenchmarksBenchmark execution time: 2025-01-20 15:52:20 Comparing candidate commit 11512de in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 30 metrics, 2 unstable metrics. scenario:profiler - sample timeline=false
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4300 +/- ##
=======================================
Coverage 97.71% 97.71%
=======================================
Files 1358 1359 +1
Lines 82501 82477 -24
Branches 4223 4198 -25
=======================================
- Hits 80614 80591 -23
+ Misses 1887 1886 -1 ☔ View full report in Codecov by Sentry. |
5d7a7f9
to
3343f03
Compare
Because we need to handle actions according their precedence.
We want to test that ActionsHandler handles actions correctly according their precedence.
These examples have to be rewritten with Hash#merge, since older versions of Ruby do not support double-splat operator.
4c28ff6
to
4d47360
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the overall change looks good and solid to use as an intermediate step to encapsulate the leaked logic 👍🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This method should replace both ActionsHandler#block_request and ActionsHandler#redirect_request
What does this PR do?
It adds
Datadog::AppSec::ActionHandler
module and unifies blocked response generation.https://github.com/DataDog/libddwaf/blob/master/UPGRADING.md#action-semantics
https://github.com/DataDog/appsec-diagrams/blob/main/action-handling.md
Motivation:
We want to have a simple and unified way of handling
libddwaf
actions.Change log entry
None.
Additional Notes:
Since Sinatra and Rails instrumentations always insert
Datadog::AppSec::Contrib::Rack::RequestMiddleware
, we don't need to catch interrupt signals anywhere other than in this Rack Middleware.How to test the change?
CI and manual testing with app generator.