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

Jump to bottom

contrib/{echo,gorilla}: add AppSec response-status test #1177

Merged
merged 8 commits into from Feb 25, 2022
Merged

contrib/{echo,gorilla}: add AppSec response-status test #1177

merged 8 commits into from Feb 25, 2022

Conversation

Hellzy
Copy link
Contributor

@Hellzy Hellzy commented Feb 21, 2022

  • Add test to check response status with AppSec rules
  • Rework echo.v4 AppSec entry point to fix a bug uncovered by this test

@Hellzy
contrib/labstack/echo.v4: rework withAppSec (now useAppSec)
6adf1e0 
Use the same logic as for gin by defering the operation finish logic
to after other middleware executions. This solves, for example, the
problem of accurate response status code retrieval.
@Hellzy
contrib/labstack/echo.v4: add AppSec response-status test
9d02679
@Hellzy
contrib/gorilla/mux: add AppSec response-status test
d11cb22
@Hellzy Hellzy added the appsec label Feb 21, 2022
@Hellzy Hellzy added this to the 1.37.0 milestone Feb 21, 2022
@Hellzy Hellzy self-assigned this Feb 21, 2022
@Hellzy Hellzy requested a review from a team as a code owner February 21, 2022 09:53
@Hellzy Hellzy requested a review from a team February 21, 2022 09:53
Julio-Guerra
Julio-Guerra previously approved these changes Feb 22, 2022
View reviewed changes
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.

Good catch. I added some nitpicks on empty lines.
I also think that we should put all those tests in a common test package next time we add more so they get simpler to maintain.

contrib/gorilla/mux/mux_test.go Outdated Show resolved Hide resolved
contrib/labstack/echo.v4/echotrace.go Outdated Show resolved Hide resolved
contrib/labstack/echo.v4/echotrace.go Outdated Show resolved Hide resolved
contrib/labstack/echo.v4/echotrace_test.go Outdated Show resolved Hide resolved
@Hellzy @Julio-Guerra
Apply suggestions from code review
827d566 
Co-authored-by: Julio Guerra <julio@datadog.com>
@Hellzy
Copy link
Contributor Author

Hellzy commented Feb 23, 2022

@knusbaum @ajgajg1134 could one of you have a look at these changes?

@Hellzy Hellzy merged commit dd9c356 into v1 Feb 25, 2022
@Hellzy Hellzy deleted the francois.mazeau/rule-status-test branch February 25, 2022 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Julio-Guerra Julio-Guerra Julio-Guerra approved these changes

@knusbaum knusbaum knusbaum approved these changes

@ajgajg1134 ajgajg1134 Awaiting requested review from ajgajg1134

Assignees

@Hellzy Hellzy

Labels
appsec
Projects
None yet
Milestone
1.37.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Hellzy @Julio-Guerra @knusbaum