Skip to content

Adds support to multiple ranges in ctl:ruleRemoveTargetById (#2110) #2114

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Adds support to multiple ranges in ctl:ruleRemoveTargetById (#2110) #2114

wants to merge 2 commits into from

Conversation

j0k2r
Copy link

@j0k2r j0k2r commented Jun 4, 2019

No description provided.

@j0k2r j0k2r mentioned this pull request Jun 4, 2019
Closed
@zimmerle zimmerle self-requested a review June 4, 2019 16:24
@zimmerle zimmerle self-assigned this Jun 4, 2019
zimmerle
zimmerle reviewed Jun 4, 2019
View reviewed changes
CHANGES Outdated
@@ -1,6 +1,8 @@
v3.0.4 - YYYY-MMM-DD (to be released)
-------------------------------------

- Adds support to multiple ranges in ctl:ruleRemoveTargetById
[Issue #2110 - @theMiddleBlue, @zimmerle]
Copy link
Contributor

Choose a reason for hiding this comment

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

@j0k2r why you did put my name on it and where is yours? :)

Copy link
Author

@j0k2r j0k2r Jun 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, sorry I don't know the ModSecurity CHANGES rules, I took the the issue "participants"

@theMiddleBlue theMiddleBlue mentioned this pull request Jun 7, 2019
Closed
@dune73
Copy link
Member

dune73 commented Jun 7, 2019

Is this PR covering the use of multiple comma-separated IDs too? (-> ctl:ruleRemoveTargetById=1000,1999)

If it does, then adding a test case for that use would be helpful.

@j0k2r
Copy link
Author

j0k2r commented Jun 7, 2019

No, it only parse the Id range

@j0k2r
Copy link
Author

j0k2r commented Jun 7, 2019

@dune73, After doing some tests, it seems that neither ctl:ruleRemoveById nor ctl:ruleRemoveTargetById supports setting multiple IDs.

I'm not a Lexer expert but apparently with the https://github.com/SpiderLabs/ModSecurity/blob/cbd15ec1382f7e2c3e19a93ea9ca3e8b4ced44f6/src/parser/seclang-scanner.ll#L447 ruleRemoveBy does not support (,)

Don't know if I'm right.

@dune73
Copy link
Member

dune73 commented Jun 7, 2019

Thank you for checking. Much appreciated. So many subtle shortcomings with v3...

@victorhora victorhora added 3.x Related to ModSecurity version 3.x enhancement labels Jun 8, 2019
@victorhora victorhora added this to the v3.0.4 milestone Jun 8, 2019
@theMiddleBlue theMiddleBlue mentioned this pull request Jun 28, 2019
Closed
@dune73 dune73 mentioned this pull request Jul 17, 2019
Closed
Chive pushed a commit to agilentia/ModSecurity that referenced this pull request Dec 4, 2019
@j0k2r @Chive
Fix CHANGES after PR owasp-modsecurity#2114
f263fa6
This was referenced May 13, 2020
Closed
Closed
@zimmerle
Copy link
Contributor

zimmerle commented Nov 3, 2020

Thank you @j0k2r. Added this as part of #2430. Merged on 3.1-experimental

@zimmerle zimmerle closed this Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimmerle zimmerle zimmerle left review comments

Assignees

@zimmerle zimmerle

Labels
3.x Related to ModSecurity version 3.x enhancement
Projects
None yet
Milestone
v3.1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@j0k2r @dune73 @zimmerle @victorhora