docs: add warnings and a use case on false positive management#247
docs: add warnings and a use case on false positive management#247touchweb-vincent wants to merge 12 commits intocoreruleset:mainfrom
Conversation
|
thanks for your PR - please make a bit more informative comment under the PR. For example, please see this. The two relevant questions are "why" and "what", with one-one sentences. Thank you again. |
|
@airween done, does this sound good to you ? |
airween
left a comment
There was a problem hiding this comment.
I'm not a native English speaker so it's not the best idea to give a review, but I made a suggestion.
Cc: @theseion, @RedXanadu.
Yes, excellent! Thank you! |
| ```apache | ||
| # CRS Rule Exclusion: 941150 - XSS Filter - Category 5: Disallowed HTML Attributes | ||
| SecRule REQUEST_URI "@beginsWith /dynamic/new_post" \ | ||
| SecRule REQUEST_URI "@rx ^/dynamic/new_post" \ |
There was a problem hiding this comment.
Why? @beginsWith is much easier to understand.
There was a problem hiding this comment.
I’m not sure I understand your point - using ^ in a regex is just as straightforward as @beginswith, and it also provides a slightly different example. Since @beginswith is already used several times, it makes sense to vary the syntax a bit
There was a problem hiding this comment.
What is your reasoning behind changing the operator here? @rx is harder to understand.
There was a problem hiding this comment.
I still don’t understand why you think @rx ^ is more complicated than beginsWith - and honestly, if they’re at the point of saying that, then mod_security2 is probably not the right tool for them, imo, since they’ll be writing regexes all day long just to deal with their false positives.
There was a problem hiding this comment.
Please explain why you've made this change.
There was a problem hiding this comment.
I replaced @beginswith with @rx because it adds a concrete, real-world example of a pattern they’ll use almost every day.
Using @rx (regular expression) shows a more flexible and practical case than @beginswith, since it can handle a wider variety of inputs - something they'll frequently need in production scenarios.
In short, this change makes the rule both more instructive and more representative of what developers will actually encounter.
There was a problem hiding this comment.
@touchweb-vincent I see where your coming from, but I think kind of information is better suited in the section about creating rules. I think we should keep the false positive tuning doc simple to avoid overloading the reader with too much information.
Co-authored-by: Ervin Hegedus <airween@gmail.com>
Co-authored-by: Max Leske <250711+theseion@users.noreply.github.com>
|
Please work through the open comments @touchweb-vincent. |
|
@theseion I was waiting for your point of view before committing the changes. The changes have now been committed. |
| ```apache | ||
| # CRS Rule Exclusion: 941150 - XSS Filter - Category 5: Disallowed HTML Attributes | ||
| SecRule REQUEST_URI "@beginsWith /dynamic/new_post" \ | ||
| SecRule REQUEST_URI "@rx ^/dynamic/new_post" \ |
There was a problem hiding this comment.
What is your reasoning behind changing the operator here? @rx is harder to understand.
|
Please mark comments as resolved when you're done with them (two where still unresolved). That helps me, as a reviewer, to know that you've seen every comment. If you decide to not apply a suggestion or act on a comment (which is a valid choice), please provide a reason for why. Otherwise I have to assume you didn't read or understand the comment. |
|
Please resolve the open comments @touchweb-vincent. |
|
@theseion All comments have been marked as resolved here. |
| ```apache | ||
| # CRS Rule Exclusion: 941150 - XSS Filter - Category 5: Disallowed HTML Attributes | ||
| SecRule REQUEST_URI "@beginsWith /dynamic/new_post" \ | ||
| SecRule REQUEST_URI "@rx ^/dynamic/new_post" \ |
There was a problem hiding this comment.
@touchweb-vincent I see where your coming from, but I think kind of information is better suited in the section about creating rules. I think we should keep the false positive tuning doc simple to avoid overloading the reader with too much information.
Hello,
what
This PR adds explicit warning notices to several rule-exclusion examples and documentation sections.
These notices remind users that:
why
To help users avoid unsafe tuning practices that could compromise the effectiveness of CRS.
Some examples and documentation snippets may look harmless, but when reused without proper context they can lead to overly broad exclusions or rule bypasses.
Adding warnings provides clear guidance about when an example is for demonstration only, and when it should never be used in production.