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

[airween]

This PR fixed the remained bug from issue #2099:

Added some test cases to prevent the similar problems.

[zimmerle]

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.

[zimmerle]

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?

[airween]

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]

Do you think about like this?
5497b86

[zimmerle]

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.

[airween]

Right, thanks for the clarification.

[zimmerle]

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]

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]

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]

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]

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]

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:

• 2c136a2
• 7a93bea
• db29869
• b5823d4

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]

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. :)