fix(942200): reduce false positive - rollback regex#4525
fix(942200): reduce false positive - rollback regex#4525touchweb-vincent wants to merge 8 commits into
Conversation
Added new test cases for BingBot and GoogleBot user agents to validate against CRS 4.24.0 update.
touchweb-vincent
changed the title
|
📊 Quantitative test results for language: |
Removed test case for false positive on French addresses and adjusted test IDs.
Removed multiple SQL injection test cases from 942200.yaml.
|
For information, the rule is also triggered on brave browser (version 1.86.139) which have this User-agent: 
|
EsadCetiner
left a comment
EsadCetiner
left a comment
There was a problem hiding this comment.
I won't be approving this since I'm not an SQL expert, but I can see that you've rewritten multiple tests completely. Don't rewrite existing tests, if behavior has changed then instead change the expected output to reflect the changes in this PR and document known False Positives and False Negatives in the test description.
|
@EsadCetiner I'm not rewriting anything, i'm reverting this PR : 6f855a4 - that's all. We can discuss it later in a separate PR with new unit tests. |
|
@touchweb-vincent I know your reverting that PR, but that doesn't mean you should remove those tests. This rule has very few tests written for it, and is part of the reason why this false positive was missed. |
|
In my humble opinion, we should roll back, urgently release a 4.24.1, and revisit this discussion afterwards. |
|
@touchweb-vincent This isn't a discussion, you should never delete tests, they exist to prevent regressions and bugs. Please add them back, adjust as needed then add your new tests on top of that. |
Updated test cases to reflect changes in SQL injection detection scenarios, including modifying User-Agent headers, request methods, and expected outputs.
|
I have added back the unit tests from 6f855a4 so the regex is failing now. |
Make matching of parenthesis more restrictive Refs coreruleset#4476 Fixes coreruleset#4502 Closes coreruleset#4525
|
@touchweb-vincent Tests 4, 5, 6, and 7 never passed before 6f855a4 was merged, adjust the expected output of those tests and update the test description to document that they are known false positives/false negatives |
* fix(942200): prevent matches against user agent strings Make matching of parenthesis more restrictive Refs #4476 Fixes #4502 Closes #4525 * Apply suggestions from code review * Apply suggestion from @fzipi --------- Co-authored-by: Felipe Zipitría <[email protected]>
3 participants
Proposed changes
Hello,
The new regex (6f855a4) cannot be accepted in its current state since it triggers on common Bot (GoogleBot / BingBot).
I suggest performing an urgent rollback and revisiting the adjustment afterward.
closes #4502
PR Checklist
commentfield to write the expected behaviorFurther comments
For the reviewer
ctl:requestBodyAccess=Offwere used in the rule