-
Notifications
You must be signed in to change notification settings - Fork 7
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
Rule 911100 triggered as a false positive when using ios app and dismissing a notification #87
Comments
@jessebot Thanks for the report
This false positive might break functionality for dismissing notifications, although that would be a relatively small annoyance. I'd still fix the false positive as it can be an issue in the future.
It looks like that on first glance, but if you look closer the
Yes that should work, but I'd modify the regex for 9508701 instead so you don't have to introduce a new rule - SecRule REQUEST_FILENAME "@rx /ocs/v[0-9]+\.php/apps/notifications/api/v[0-9]/notifications(?:/[0-9]+)?$" \
+ SecRule REQUEST_FILENAME "@rx /ocs/v[0-9]+\.php/apps/notifications/api/v[0-9]/notifications(?:/[0-9]+|/push)?$" \ Try to avoid introducing unnecessary rules to prevent the number of rules from exploding, this plugin is already pretty big as it is. Do you want to try opening a PR? if you do, please add some tests if you can. |
@EsadCetiner Thank you! Unfortunately, I don't have time in my schedule to fix this right now, so if you can do this, that would be great. Otherwise, I probably won't have time for a week or two, as I'm not really sure how to write the tests yet, so I'd have to spend some time with it. |
@jessebot No problem, I just wanted to encourage you since you expressed interest in doing one yourself last time. I see in the transaction log you provided that there's a content type header, I don't see any request body, are you able to provide one for the tests (No big deal if you can't provide one)? You might have to add |
I appreciate that! 🙏 I will do my best to make time for this repo as soon as my schedule clears up a tiny bit. I think Request Body is enabled by default in my setup, unless I'm wrong (totally possible 🤦 ), but I think there's no body in this case, as the body did get logged in the instance of the nextcloud cookbook app here, #88 (comment), and both log entries would have been spawned by the same instance of nginx, I think. Here's my ingress-nginx modSecurity section which should be equivlent of Oh, btw, I'm a maintainer over at nextcloud/helm, and to help drive adoption of this plugin, I've created a discussion post to promote it here if you're interested (I also got my PR released so ingress-nginx is now using the latest version of CRS): nextcloud/helm#592 |
Just making sure since there's a known issue on libModSecurity3 where the request body won't be logged in the default configuration(This is the default on ModSecurity2).
I don't see a rule for https://github.com/owasp-modsecurity/ModSecurity/blob/3dda900ee9f86619354dd41eefd160794e1833dd/modsecurity.conf-recommended#L75, this rule should be triggered if a request with a content type is sent without a request body, that doesn't happen for you?
Sounds awesome! |
Hi friends!
This one might be a little more difficult to reproduce unless you own an iPad or iPhone, but I have the Nextcloud app installed on my iPad for uploading my drawings from procreate. Recently, when I tried to delete a file it was trying to upload, it showed an error that I didn't have network access and when I dismissed the error, I got the following in the logs:
Luckily, I don't think this actually breaks anything this time, just kind of clogs the logs a bit. What's weird is that this should already be caught, I think? Check out this section:
nextcloud-rule-exclusions-plugin/plugins/nextcloud-rule-exclusions-before.conf
Lines 1889 to 1916 in 27cee16
I'm unable to reproduce the same issue from my android, web, or native macOS apps. 🤷 Maybe we need something like this?
The text was updated successfully, but these errors were encountered: