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

contrib/echo: appsec: fix status code reporting when blocking users #1757

Merged
merged 2 commits into from
Feb 22, 2023

Conversation

Hellzy
Copy link
Contributor

@Hellzy Hellzy commented Feb 21, 2023

What does this PR do?

Fixes status code reporting when blocking users with echo.
In this specific case, we don't call echo's error handlers since the WAF actions will write the response instead, and calling
the echo handler would short circuit that, leading to incorrect status code reporting

Motivation

This is a bugfix.

Describe how to test/QA your changes

This scenario is tested in the appsec's echo related tests.

Reviewer's Checklist

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

@Hellzy Hellzy added this to the v1.48.0 milestone Feb 21, 2023
@Hellzy Hellzy added the appsec label Feb 21, 2023
@Hellzy Hellzy force-pushed the francois.mazeau/echo-status-code-fix branch from f123de5 to 91806f6 Compare February 21, 2023 17:05
@Hellzy Hellzy marked this pull request as ready for review February 21, 2023 17:06
@Hellzy Hellzy requested a review from a team as a code owner February 21, 2023 17:06
@pr-commenter
Copy link

pr-commenter bot commented Feb 21, 2023

Benchmarks

Comparing candidate commit 22f31b9 in PR branch francois.mazeau/echo-status-code-fix with baseline commit ccb1ff2 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 6 cases.

Copy link
Contributor

@Julio-Guerra Julio-Guerra left a comment

Choose a reason for hiding this comment

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

I had to check all of that locally to better understand how gRPC and other HTTP frameworks were working without this check. But I get it now and is a good example of why we will have to export our public error type when we add the HTTP body blocking (to have a single error check for both cases).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants