Skip to content
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

Merged

Conversation

GustavoCaso
Copy link
Member

@GustavoCaso GustavoCaso commented Feb 8, 2023

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

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.
@GustavoCaso GustavoCaso requested a review from a team February 8, 2023 10:28
@github-actions github-actions bot added appsec Application Security monitoring product integrations Involves tracing integrations labels Feb 8, 2023
@@ -57,6 +57,10 @@ def call(env)

AppSec::Event.record(*context.events)

if response_action && response_action.any? { |action, _event| action == :block }
Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that is 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly!

Copy link
Contributor

@lloeki lloeki left a 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 }
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Member Author

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

@GustavoCaso GustavoCaso force-pushed the appsec-block-request-base-on-response-addresses-matches branch from e03f436 to be7bada Compare February 8, 2023 14:34
@GustavoCaso GustavoCaso merged commit 5e83a37 into master Feb 8, 2023
@GustavoCaso GustavoCaso deleted the appsec-block-request-base-on-response-addresses-matches branch February 8, 2023 15:10
@github-actions github-actions bot added this to the 1.10.0 milestone Feb 8, 2023
@TonyCTHsu TonyCTHsu mentioned this pull request Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
appsec Application Security monitoring product integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants