Skip to content

added disable error log support #360

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

fatihusta
Copy link

modified old PR

#327

modified old PR

owasp-modsecurity#327

Signed-off-by: Fatih USTA <[email protected]>
@airween
Copy link
Member

airween commented Aug 13, 2025

Hi @fatihusta,

many thanks for update the mentioned patch.

Could you add a new test case to the CI workflow, like this one? Just turn on this new feature, send an attack which triggers a rule, and check that the error.log is empty.

Thanks!

@tomsommer
Copy link

tomsommer commented Aug 13, 2025

Thank you for this. I'm already using it :)

My only feedback would be that the name of the variable is a bit confusing, you turn it ON to turn something OFF. modsecurity_use_error_log with default on would maybe make more sense?

Just my two cents

@airween
Copy link
Member

airween commented Aug 13, 2025

@fatihusta thanks for adding the test.

What do you think about @tomsommer's idea. I think you should consider it - I agree with him, the current implementation has a bit weird logic.

Also, after we agreed what should be the final keyword, please add the documentation into our README (README is part of the repository).

- tests are changed with new directive name
- nginx.conf updated with new directive name
- added doc

Signed-off-by: Fatih USTA <[email protected]>
Copy link

@fatihusta
Copy link
Author

Hi
I changed the directive name as modsecurity_use_error_log. Default is on.

Thanks @tomsommer @airween

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

Successfully merging this pull request may close these issues.

3 participants