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

chore: updates CRS tests to CRS 4.0.0-rc2 #899

Merged
merged 3 commits into from
Feb 6, 2024

Conversation

M4tteoP
Copy link
Member

@M4tteoP M4tteoP commented Nov 4, 2023

  • Updates CRS tests to CRS 4.0.0-rc2
    • /post has been added to the emulated httpbin behavior, being used for a couple of tests to match the response body.
    • Notably: 934131-5 and 934131-7 are failing. This is happening because:
      - we are urldecoding the cookie key values before storing them
      - The test comes with a + in the payload: Yy9vbmVycm9yJTNkYWxlcnQoMSk+Cg== (snippet of payload). The rule transformations are: t:none,t:urlDecodeUni,t:jsDecode,t:base64Decode,t:urlDecodeUni,t:jsDecode. Being the urldecode before the base64decode, the + is translated into a space, making the base64 fail. Further investigations (and likely comparisons with ModSec runs) are needed.

@M4tteoP M4tteoP requested a review from a team as a code owner November 4, 2023 18:24
Copy link

codecov bot commented Nov 4, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f1cfd13) 82.65% compared to head (2725717) 82.37%.
Report is 1 commits behind head on main.

❗ Current head 2725717 differs from pull request most recent head 399fcc6. Consider uploading reports for the commit 399fcc6 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #899      +/-   ##
==========================================
- Coverage   82.65%   82.37%   -0.28%     
==========================================
  Files         162      160       -2     
  Lines        9028     8988      -40     
==========================================
- Hits         7462     7404      -58     
- Misses       1317     1336      +19     
+ Partials      249      248       -1     
Flag Coverage Δ
default 77.64% <ø> (-0.13%) ⬇️
examples 26.56% <ø> (+0.06%) ⬆️
ftw 46.78% <ø> (-0.35%) ⬇️
ftw-multiphase 48.99% <ø> (-0.33%) ⬇️
tinygo 75.14% <ø> (-0.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jcchavezs
Copy link
Member

cc @anuraaga @fzipi @jptosso

@jcchavezs
Copy link
Member

Can we finish this @M4tteoP ?

@M4tteoP
Copy link
Member Author

M4tteoP commented Nov 21, 2023

  • The test comes with a + in the payload: Yy9vbmVycm9yJTNkYWxlcnQoMSk+Cg== (snippet of payload). The rule transformations are: t:none,t:urlDecodeUni,t:jsDecode,t:base64Decode,t:urlDecodeUni,t:jsDecode. Being the urldecode before the base64decode, the + is translated into a space, making the base64 fail. Further investigations (and likely comparisons with ModSec runs) are needed.

Further investigations led to these two root causes:

I added the links to the description of the ignored test and also mentioned that the rule that is tested by the two excluded tests is actually quite debated CRS side being very specific to a few payloads. More context: coreruleset/coreruleset#3376

http/middleware_test.go Outdated Show resolved Hide resolved
@@ -9,5 +9,3 @@ testoverride:
920290-1: 'Rule works, log contains 920290. Test expects status 400 (Apache behaviour)'
920430-8: 'Go/http does no allow HTTP/3.0 - 505 HTTP Version Not Supported'
932200-13: 'wip'
934131-5: 't:base64decode too strict https://github.com/corazawaf/coraza/issues/926 and https://github.com/corazawaf/coraza/issues/920. Rule itself is debated CRS side: https://github.com/coreruleset/coreruleset/issues/3376'
934131-7: 't:base64decode too strict https://github.com/corazawaf/coraza/issues/926 and https://github.com/corazawaf/coraza/issues/920. Rule itself is debated CRS side: https://github.com/coreruleset/coreruleset/issues/3376'
Copy link
Member Author

@M4tteoP M4tteoP Jan 3, 2024

Choose a reason for hiding this comment

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

Tests 934131-5 and 934131-7 are now fixed. Overall the PR comes with 3 fewer tests not compatible with Coraza

@jcchavezs jcchavezs merged commit 8993363 into corazawaf:main Feb 6, 2024
8 checks passed
@jcchavezs jcchavezs deleted the crs4_rc2 branch February 6, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants