Skip to content

added remove_comments_char transformation to address issue #971 #1098

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

Closed
wants to merge 2 commits into from
Closed

Conversation

bjh7242
Copy link
Contributor

@bjh7242 bjh7242 commented Mar 17, 2016

Implemented remove_comments_char transformation for libmodsecurity.

Removes the following characters

  • /*
  • */

@zimmerle zimmerle self-assigned this Mar 24, 2016
@zimmerle zimmerle modified the milestones: v3.0.0, v3.0.0 owasp compatible, v3.0.0 feature complete Mar 24, 2016
@bjh7242 bjh7242 closed this Apr 1, 2016
@bjh7242 bjh7242 reopened this Apr 1, 2016
@zimmerle
Copy link
Contributor

zimmerle commented Apr 4, 2016

Hi @bjh7242,

Thanks for the patch. I will try to have a look on it today.

zimmerle added a commit to owasp-modsecurity/secrules-language-tests that referenced this pull request Apr 4, 2016
zimmerle added a commit that referenced this pull request Apr 4, 2016
zimmerle added a commit that referenced this pull request Apr 4, 2016
Trimming the pull request #1098
@zimmerle
Copy link
Contributor

zimmerle commented Apr 4, 2016

Hi @bjh7242,

Your pull request is now merged. Thanks again for patch.

Few comments:

Usually, before add a functionality to version 3 we expect to have regression tests that covers its basic usage, plus a few corner cases. Those unit tests are very important, among of other things, those tests are executed by our buildbots, so we can make sure that the functionality is really working in all supported platforms.

Other detail that you may want to consider is the fact that we try to not use lines bigger than 80 characters.

Changes that I have made:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants