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: use lowercase in case ignoring assembly files #3608

Closed

Conversation

theseion
Copy link
Contributor

@theseion theseion commented Mar 9, 2024

Preparation for the new format check in crs-toolchain.

@theseion theseion requested a review from fzipi March 9, 2024 14:21
Copy link
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

On the request side I think it is clear we should lowercase. I'm having doubts on the response side, where we probably want to remove the ignore case?

@@ -286,7 +286,7 @@ SecRule RESPONSE_BODY "@rx (?i)(?:System\.Data\.OleDb\.OleDbException|\[Microsof
# (consult https://coreruleset.org/docs/development/regex_assembly/ for details):
# crs-toolchain regex update 951230
#
SecRule RESPONSE_BODY "@rx (?i)(?:supplied argument is not a valid |SQL syntax.*)MySQL|Column count doesn't match(?: value count at row)?|mysql_fetch_array\(\)|on MySQL result index|You have an error in your SQL syntax(?:;| near)|MyS(?:QL server version for the right syntax to use|qlClient\.)|\[MySQL\]\[ODBC|(?:Table '[^']+' doesn't exis|valid MySQL resul)t|Warning.{1,10}mysql_(?:[\(\)_a-z]{1,26})?|(?:ERROR [0-9]{4} \([0-9a-z]{5}\)|XPATH syntax error):" \
SecRule RESPONSE_BODY "@rx (?i)s(?:upplied argument is not a valid |ql syntax.*)mysql|column count doesn't match(?: value count at row)?|mysql(?:_fetch_array\(\)| server version for the right syntax to use|client\.)|on mysql result index|you have an error in your sql syntax(?:;| near)|\[mysql\]\[odbc|(?:table '[^']+' doesn't exis|valid mysql resul)t|warning.{1,10}mysql_(?:[\(\)_a-z]{1,26})?|(?:error [0-9]{4} \([0-9a-z]{5}\)|xpath syntax error):" \
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if ignore case is wise in this situation 🤔

⚠️ Maybe we want to actually remove the ignore case here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that it matters. The strings are very unique already, making the match case insensitive doesn't increase the probability for FPs, IMO. We also lose a bit of information in the source files, of course. I'll remove the flag for both files.

@@ -311,7 +311,7 @@ SecRule RESPONSE_BODY "@rx (?i)(?:supplied argument is not a valid |SQL syntax.*
# (consult https://coreruleset.org/docs/development/regex_assembly/ for details):
# crs-toolchain regex update 951240
#
SecRule RESPONSE_BODY "@rx (?i)P(?:ostgreSQL(?: query failed:|.{1,20}ERROR)|G::[a-z]*Error)|pg_(?:query|exec)\(\) \[:|Warning.{1,20}\bpg_.*|valid PostgreSQL result|Npgsql\.|Supplied argument is not a valid PostgreSQL .*? resource|(?:Unable to connect to PostgreSQL serv|invalid input syntax for integ)er" \
SecRule RESPONSE_BODY "@rx (?i)p(?:ostgresql(?: query failed:|.{1,20}error)|g(?:_(?:query|exec)\(\) \[:|::[a-z]*error))|warning.{1,20}\bpg_.*|valid postgresql result|npgsql\.|supplied argument is not a valid postgresql .*? resource|(?:unable to connect to postgresql serv|invalid input syntax for integ)er" \
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Shall we remove the (?i)?

@theseion theseion marked this pull request as draft March 10, 2024 06:56
@github-actions github-actions bot added the Stale label Apr 10, 2024
@theseion theseion removed the Stale label Apr 20, 2024
@github-actions github-actions bot added the Stale label May 21, 2024
@github-actions github-actions bot closed this Jun 4, 2024
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.

None yet

2 participants