-
Notifications
You must be signed in to change notification settings - Fork 375
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
[APPSEC-7801] Appsec block request base on response addresses matches #2605
[APPSEC-7801] Appsec block request base on response addresses matches #2605
Conversation
When calling the waf with the reponse result if a rule matches and the action is to block we record the appsec event but, the customer will see a 403 on the result.
@@ -57,6 +57,10 @@ def call(env) | |||
|
|||
AppSec::Event.record(*context.events) | |||
|
|||
if response_action && response_action.any? { |action, _event| action == :block } |
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.
Looks good to me, I think this is the behavior change
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.
Yes that is 😄
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.
Exactly!
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.
Approved, but I think we should remove e03f436
@@ -57,6 +57,10 @@ def call(env) | |||
|
|||
AppSec::Event.record(*context.events) | |||
|
|||
if response_action && response_action.any? { |action, _event| action == :block } |
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.
Exactly!
@@ -24,11 +24,11 @@ def call(env) | |||
|
|||
request = ::Rack::Request.new(env) | |||
|
|||
request_return, request_response = Instrumentation.gateway.push('rack.request.body', request) do | |||
request_return, request_action = Instrumentation.gateway.push('rack.request.body', request) do |
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 disagree with this change, the gateway response
does contain an action but is a richer and different concept. In the future this return value should be an Instrumentation::Gateway::Response
Struct (or struct-like object) instead of a basic array of [action, event]
.
In addition this refactoring makes this file inconsistent with other places that use the instrumentation gateway.
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.
Totally valid point.
I was already working on a separate PR for the introduction of a new object, something down the lines of Instrumentation::Gateway::Response
or AppSec::Reactive::Result
. Still figuring out the name 😄
I removed that last commit which included the renaming
e03f436
to
be7bada
Compare
What does this PR do?
When calling the waf with the response result if a rule matches and
the action is to block we record the appsec event but, the customer
will see a 403 on the result.
Additional Notes
I rename some variables to better reflect what the stored information is.
How to test the change?
CI