Adds missing check for runtime ctl:ruleRemoveByTag (Issue #2099) #2102

airween · 2019-05-23T18:47:17Z

This PR fixed the remained bug from issue #2099:

Added some test cases to prevent the similar problems.

zimmerle

I have started a review in the code.

Question: Why you have sent this patch against that branch? As already explained to you we are working on 3.1. It seems like your patch is compatible only with the master branch.

zimmerle · 2019-05-27T13:59:41Z

src/rules.cc

@@ -248,6 +248,9 @@ int Rules::evaluate(int phase, Transaction *t) {
                    break;
                }
            }
+            if (remove_rule) {


Hi @airween,

I am assuming that you have copy && paste the remove_rule check from the block just above. Indeed, that was missing. However you may have pasted in the wrong place, that may lead to unnecessary loops. Can you make it exactly in the same fashion as the others checks?

Hi @zimmerle, sorry, I don't know what do you think about - why is this a wrong place, and which loops could occures?

Do you mean that it needs a plus condition before the for loop, as the previous variables?

Do you think about like this?
5497b86

Yes, exactly. There are three loops that aiming to do the very same thing. There is no reason to have one different for the others.

Right, thanks for the clarification.

zimmerle · 2019-05-27T14:11:20Z

General comments:

Try to give significative names to pull requests, so it can be searchable by other users.

A good title could be: Adds missing check for runtime SecRuleRemoveByTag (Issue #2099)

Whenever you make a reference to an issue, use the hash marking otherwise Github won't notice the reference.

Instead of 2099 use #2099

airween

Hi @airween,

I have started a review in the code.

Question: Why you have sent this patch against that branch? As already explained to you we are working on 3.1. It seems like your patch is compatible only with the master branch.

Where should I sent the patch?

I just read about the 3.1 from blogposts... not remember that we've discussed about the flow of sending of patches for "old" version :)

Edit: and one more note: the original issue affects 3.0 - I think it's better to apply to this first.

airween · 2019-05-27T14:18:00Z

src/rules.cc

@@ -248,6 +248,9 @@ int Rules::evaluate(int phase, Transaction *t) {
                    break;
                }
            }
+            if (remove_rule) {


Hi @zimmerle, sorry, I don't know what do you think about - why is this a wrong place, and which loops could occures?

Do you mean that it needs a plus condition before the for loop, as the previous variables?

airween · 2019-05-27T14:26:17Z

General comments:

* Try to give significative names to pull requests, so it can be searchable by other users.

A good title could be: Adds missing check for runtime SecRuleRemoveByTag (Issue #2099)

* Whenever you make a reference to an issue, use the hash marking otherwise Github won't notice the reference.

Instead of 2099 use #2099

Sorry, and thanks for the tips - I'll try to keep it mind :)

Edit: these had been modified too.

zimmerle · 2019-05-28T13:40:21Z

Hi @airween,
I have started a review in the code.
Question: Why you have sent this patch against that branch? As already explained to you we are working on 3.1. It seems like your patch is compatible only with the master branch.

Where should I sent the patch?

I just read about the 3.1 from blogposts... not remember that we've discussed about the flow of sending of patches for "old" version :)

Edit: and one more note: the original issue affects 3.0 - I think it's better to apply to this first.

Sure @airween. I think that reiterates the need to talk over GitHub issues instead of Hangouts/Skype and Slack. So that we don't lose track of what is going on with the project. With that, we are keeping the info not only ourselves but for others who may want to be up to date and participating in the discussions.

No worries I will modify your patch to fit 3.1.

airween · 2019-05-28T13:51:23Z

Hi @airween,
I have started a review in the code.
Question: Why you have sent this patch against that branch? As already explained to you we are working on 3.1. It seems like your patch is compatible only with the master branch.

Where should I sent the patch?
I just read about the 3.1 from blogposts... not remember that we've discussed about the flow of sending of patches for "old" version :)
Edit: and one more note: the original issue affects 3.0 - I think it's better to apply to this first.

Sure @airween. I think that reiterates the need to talk over GitHub issues instead of Hangouts/Skype and Slack. So that we don't lose track of what is going on with the project. With that, we are keeping the info not only ourselves but for others who may want to be up to date and participating in the discussions.

No worries I will modify your patch to fit 3.1.

Hi @zimmerle - thanks for details.

I've double checked on Slack (a long time ago we didn't used Skype and Hangouts :)), and mailing list. On the list, I've found a mention for 3.1 in a mail, nothing more.

Of course, it's important to keep the control over the project.

I've never checked out the 3.1 - sounds interesting. Would you like me to see that too?

zimmerle · 2019-05-30T13:29:07Z

Hi @airween,

Thank you for your patch. I have made some modifications in your patch in order to:

(a) merging all the commits that you were offering fixies within the very same branch;
(b) baptise the commits with more meaningful description;
(c) Adding the test case to the test case list.

The result is here:

Since you are a long-term contributor, it is important to highlight (c). If the test case is not on the make file list, it is not executed by the buildbots. Therefore not automagically tested. List is still manual, little trickier to get it done right.

Here is the list:
https://github.com/SpiderLabs/ModSecurity/blob/b5823d4e0c53375d56f7e5089d6c6bec948ac339/Makefile.am#L152

This patch is now merged!

airween · 2019-05-30T15:03:07Z

Hi @zimmerle, thanks for the tips - I'll try to keep them in mind :).

Note, that I've added many test cases (may be for all relevant modifications), but never met whit this issue. Thanks for the notification. :)

airween added 2 commits May 23, 2019 13:00

Fix 2099 / ruleRemoveByTag

c0a7906

Added some test cases

ebdb36b

This was referenced May 23, 2019

CRS and v3 incompatibility due to ruleRemoveByTag #2099

Closed

Exclusion rules doesn't work with libmodsecurity (v3) due to ctl:ruleRemoveTargetByTag SpiderLabs/owasp-modsecurity-crs#1419

Closed

victorhora requested a review from zimmerle May 23, 2019 22:55

victorhora assigned zimmerle and victorhora May 23, 2019

victorhora added the 3.x Related to ModSecurity version 3.x label May 23, 2019

victorhora added this to the v3.0.4 milestone May 23, 2019

zimmerle reviewed May 27, 2019

View reviewed changes

airween commented May 27, 2019

View reviewed changes

Added plus condition to prevent the unnecessary loops

5497b86

airween changed the title ~~Fixed issue 2099~~ Adds missing check for runtime SecRuleRemoveByTag (Issue #2099) May 27, 2019

airween changed the title ~~Adds missing check for runtime SecRuleRemoveByTag (Issue #2099)~~ Adds missing check for runtime ctl:ruleRemoveByTag (Issue #2099) May 27, 2019

zimmerle pushed a commit that referenced this pull request May 30, 2019

CHANGES: Adds info about #2099, #2102

b5823d4

zimmerle closed this May 30, 2019

This was referenced May 30, 2019

libmodsecurity (v3) incompatible ctl:ruleRemoveByTag in rules/REQUEST-903.9003-NEXTCLOUD-EXCLUSION-RULES.conf SpiderLabs/owasp-modsecurity-crs#1418

Closed

Fix libmodsecurity (v3) incompatible ctl:ruleRemoveByTag SpiderLabs/owasp-modsecurity-crs#1422

Closed

vladbukin pushed a commit to vladbukin/ModSecurity that referenced this pull request Apr 12, 2022

CHANGES: Adds info about owasp-modsecurity#2099, owasp-modsecurity#2102

4a20190

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Adds missing check for runtime ctl:ruleRemoveByTag (Issue #2099) #2102

Adds missing check for runtime ctl:ruleRemoveByTag (Issue #2099) #2102

Uh oh!

airween commented May 23, 2019 •

edited

Loading

Uh oh!

zimmerle left a comment

Uh oh!

zimmerle May 27, 2019

Uh oh!

airween May 27, 2019

Uh oh!

airween May 27, 2019

Uh oh!

zimmerle May 28, 2019

Uh oh!

airween May 28, 2019

Uh oh!

zimmerle commented May 27, 2019

Uh oh!

airween left a comment •

edited

Loading

Uh oh!

airween May 27, 2019

Uh oh!

airween commented May 27, 2019 •

edited

Loading

Uh oh!

zimmerle commented May 28, 2019

Uh oh!

airween commented May 28, 2019

Uh oh!

zimmerle commented May 30, 2019

Uh oh!

airween commented May 30, 2019

Uh oh!

Uh oh!

Adds missing check for runtime ctl:ruleRemoveByTag (Issue #2099) #2102

Adds missing check for runtime ctl:ruleRemoveByTag (Issue #2099) #2102

Uh oh!

Conversation

airween commented May 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zimmerle left a comment

Choose a reason for hiding this comment

Uh oh!

zimmerle May 27, 2019

Choose a reason for hiding this comment

Uh oh!

airween May 27, 2019

Choose a reason for hiding this comment

Uh oh!

airween May 27, 2019

Choose a reason for hiding this comment

Uh oh!

zimmerle May 28, 2019

Choose a reason for hiding this comment

Uh oh!

airween May 28, 2019

Choose a reason for hiding this comment

Uh oh!

zimmerle commented May 27, 2019

Uh oh!

airween left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

airween May 27, 2019

Choose a reason for hiding this comment

Uh oh!

airween commented May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zimmerle commented May 28, 2019

Uh oh!

airween commented May 28, 2019

Uh oh!

zimmerle commented May 30, 2019

Uh oh!

airween commented May 30, 2019

Uh oh!

Uh oh!

airween commented May 23, 2019 •

edited

Loading

airween left a comment •

edited

Loading

airween commented May 27, 2019 •

edited

Loading