Marking "Highest" till we determine if this leakage is something we're really concerned with.

Nx.devnull wrote:

Add action hidedetails which automatically hides log entries

This patch implements a restricted action called hidedetails that automatically hides the log entries created by the filter (i.e. sets afl_deleted to 1).

Unfortunately this also creates an inconsistency in user rights - users with abusefilter-modify-restricted can create a filter that autohides its log entries, even though they might not have abusefilter-hide-log that is normally required to hide and unhide log entries.

Please also see Bug 33390 to fully plug this leak.

attachment abusefilter_hidedetails.patch ignored as obsolete

r107454

Prodego wrote:

I don't like this idea - being able to hide things from yourself is a rather large problem as far as being responsible for your own false positives goes. Reviewing false positives would require the right to see the logs, and (I believe) on some WMF wikis with abusefilter enabled there are no users with the right to see hidden abusefilter log entries.

Nx.devnull wrote:

Well, you need to have abusefilter-modify-restricted to be able to use this action in the first place, is that enabled on any WMF wiki?

Reverted in r107456 based on Prodego's comment. I'm reopening and really, really not touching this again for a bit since I don't know enough about AbuseFilter.

Prodego wrote:

I didn't catch that it was added to the restricted list. There is already privilege inconsistency there with the degroup, block, and rangeblock abilities. So then this is a theoretical problem rather than a practical one, in a situation where lots of theoretical problems already exist. In that case, I was incorrect in saying this would be an immediate problem - it would just add to an existing mass of problems. I wouldn't mind including it then, though at some point, this whole thing should be reworked.

Nx.devnull wrote:

Fixed warning

Previous patch caused a warning about an unrecognized action.

attachment abusefilter_hidedetails.patch ignored as obsolete

Thehelpfulonewiki wrote:

This is needed for private filters, there are some log entries o

Thehelpfulonewiki wrote:

which should not be visible for the public, especially as Nikola has stated. This is a practical problem unfortunately, not one in theory. A fast fix would be appreciated.

Prodego wrote:

This cannot reasonably be enabled on WMF wikis for the reasons that Nikola and I both mentioned. A reworking of how the abusefilter handles restricted actions would be required before we can do that.

Nx.devnull wrote:

Ok, so on enwiki, oversighters can hide log entries, and sysops can view and edit private filters.

So the solution seems to be that private filters should create log entries where the details and examine page are hidden from people who don't have abusefilter-view-private (the right required to view private filters, not to be confused with abusefilter-private, the right to view IP addresses in details, which is not enabled on enwiki).

The oversighters can further hide those entries from sysops using the existing mechanism as well as hiding the entry completely from the abuse log.

Nx.devnull wrote:

Proposed patch 2

Ok, this patch makes log entries belonging to private/hidden filters behave as if the user did not have abusefilter-log-detail.

I've added a parameter, $filter_id, to SpecialAbuseLog::canSeeDetails. If it's not null, the function checks if the filter is hidden (by calling AbuseFilter::filterHidden), and in that case returns true only if AbuseFilterView::canViewPrivate() (i.e. the user is allowed to view private filters) is true (in addition to requiring abusefilter-log-detail).

I've made the methods canViewPrivate() and canEdit() of AbuseFilterView static, to avoid code duplication in the above. I hope that doesn't break anything, though it shouldn't since it already used a static variable.

Since the abuse log may contain entries generated by global filters, I've modified AbuseFilter::filterHidden to handle global filters as well. I wasn't able to test this though.

Whenever SpecialAbuseLog::canSeeDetails is called for a specific log entry, it is called with a filter id. However, it's called without a filter id for determining whether a user should be able to search for log entries belonging to a specific filter. This is allowed for public filters, but is disallowed for private ones. If the user tries to search for log entries belonging to a private filter, and canViewPrivate is false, the condition is not added to the query so it returns all log entries.

In addition, I hid the hitcount from the filter list for private filters from users who cannot see details of the filter. The link would search for log entries created by the filter, but if the user can't view the details then that doesn't work (even before this patch, the links would be displayed but not work for users who didn't have abusefilter-log-detail). I don't know if the hitcount itself should be displayed without the link though, I opted not to.

attachment abusefilter_private.patch ignored as obsolete

Nx.devnull wrote:

Proposed patch 2 improved

I got rid of some unnecessary db queries (when the function calling canSeeDetails already does a database query and has af_hidden, it passes it to canSeeDetails so it doesn't have to call filterHidden), and I changed the error message abusefilter-log-cannot-see-details, since it's shown when you try to view a single entry.

Attached:

Prodego wrote:

That's an excellent solution. I am trying to think if there would be a reason for the abuse log of private filters to be public, and I cannot immediately think of one.

Nx.devnull wrote:

Transparency.

But another reason why details of private filters shouldn't be public is that they give insights into how the filter works. The point of having private filters is obfuscating the code, so that a spammer or whatever doesn't know what to avoid. Right now they have a convenient link to all the filter's previous hits, with a diff for each and every case where the filter prevented an edit, making it easier to figure out how to circumvent the filter.

Thehelpfulonewiki wrote:

Okay. If this patch works, how long will it take to implement/do we need to get some consensus somewhere? A private filter having a private log seems to be a no-brainer for me.

r107906