Do substitutions have to be enclosed in () if you want to apply ? to them?

@Deric-W: Have you seen comments on your PR?

Sorry for the very long delay, I totally forgot about this PR.
I will address them when I am back home.

I noticed that the group regex (?:(?:,?\s*"\w+":(?:"[^"]+"|\w+))*) is vulnerable against input containing quoted quotation marks (\") which causes it to think that a group has ended and fail the match when a group like ...,"url":"/login\"",... would appear in the part we want to match.
I am not sure if this is possible in practice since all groups we want to match in the example log lines are either not directly attacker controlled or url encoded.

I fixed it by modifying the group regex with (?:(?:,?\s*"\w+":(?:"(?:[^"\\]|\\"|\\\\)*"|\w+))*), can you spot any problems with this regex?

can you spot any problems with this regex?

yes, because now [^"\\] would stop on backslash too, and it can be used to escape any char, so theoretically it must be:

- (?:(?:,?\s*"\w+":(?:"(?:[^"\\]|\\"|\\\\)*"|\w+))*)
+ (?:(?:,?\s*"\w+":(?:"(?:[^"\\]+|\\.)*"|\w+))*)

(also note the + quantifier after [^"\\] - that makes regular case much faster)

Thanks for catching the mistake 👍
The performance optimization however seems to cause exponential backtracking which makes the test suite hang.
I removed the + quantifier and moved the date pattern and some parts to a nextcloud-common.conf (based on apache-common.conf).

One problem with using this filter - though not a problem with the regex or anything here per se - is that it is prone to false positives because the logs don't actually say anything about why the login failed. It could be because of a genuine invalid login and/or brute force attempt, sure, but the logs will look identical if there is some sort of problem with the backend authentication provider, triggering a slew of false alarms. The logged statement is true in both cases, in that the login did indeed fail, but some more detail (like what eg. Dovecot does) would allow to differentiate between these two very different problems, and make it possible to adjust the filter regex accordingly to achieve higher accuracy.

Not something that can be fixed here of course, as this is clearly a Nextcloud bug (or at least I would consider it to be one), but perhaps something to note as a comment, a possible gotcha worth being aware of? I at least had to disable my Nextcloud jail because I could not find any way to prevent this from happening.

I guess this PR makes #1913 outdated. Does it?

I think so, but I noticed that the mentioned PR seems to expect a different nextcloud log format (probably because it is 7 years old).

@yitzhaq Feel free to bring up your suggestions at the Nextcloud Server (if logging behavior changes are required) and/or Documentation (if rule changes are required) repositories. :-)