Page MenuHomePhabricator

Special:AbuseLog pager does not work properly if there are abuse log entries with a revdeled revision
Open, Stalled, HighPublicBUG REPORT

Description

Reproduce: https://en.wikipedia.org/w/index.php?title=Special%3AAbuseLog&wpSearchUser=Popcountrysucks+Backup+3.0

Actual: the result is completely empty

Expected: either some results are displayed, or "No results" is shown (e.g. https://en.wikipedia.org/w/index.php?title=Special%3AAbuseLog&wpSearchUser=Example) if there are no result to show

Event Timeline

It is empty for you because you don't have permission to view the entries.

Johannnes89 subscribed.

Indeed, "no results" would be wrong, there are results, but not everyone can see them.

Dragoniez subscribed.
Empty abuse log.PNG (380×1 px, 36 KB)
<form method="GET" action="/wiki/Special:AbuseLog/hide">
   <ul class="plainlinks"></ul>
</form>

This doesn't look right to me. There being an empty <ul> element suggests that the early return logic in AbuseLogPager::doFormatRow isn't working properly.
I've confirmed that this happens when the viewing user doesn't have access to revdel'd revisions and the abuse log entry is associated with a revdel’d revision.

Bugreporter renamed this task from Inconsistent display on Special:Abuselog to Special:AbuseLog pager does not work properly if there are abuse log entries with a revdeled revision.Sat, Jan 17, 9:59 AM

There being an empty <ul> element suggests that the early return logic in AbuseLogPager::doFormatRow isn't working properly.

It turns out this wasn’t quite accurate. I’ll take a closer look at what’s actually happening.

I'm not sure this is something we can "fix". What's happening is that the query is run with a LIMIT, and then the special page drops entries the user isn't allowed to see.
To make the count match, we'd have to keep querying until we get enough viewable rows, which could get expensive performance-wise. This might be better looked at in a separate task if we want to dig into it further.

Or some placeholder can be displayed for entries the user isn't allowed to see.

Alright, here's what's happening.

I've confirmed that this happens when the viewing user doesn't have access to revdel'd revisions and the abuse log entry is associated with a revdel’d revision.

Note first that this doesn't happen when the viewer lacks suppressor rights and the abuse log entry itself is suppressed. That's because AbuseLogPager::getQueryInfo already filters out suppressed log entries for non-suppressors at the DB query level.

By contrast, revision visibility is checked after the SELECT runs, in SpecialAbuseLog::getEntryVisibilityForUser. Because of that, rows tied to revdel'd revisions are still fetched and only dropped later. To avoid this, we should also LEFT JOIN the rev_deleted field from the revision table and exclude abuse log entries the user can't see, directly in the query.

Dragoniez changed the subtype of this task from "Task" to "Bug Report".Sat, Jan 17, 12:23 PM

Change #1228270 had a related patch set uploaded (by Matěj Suchánek; author: Matěj Suchánek):

[mediawiki/extensions/AbuseFilter@master] Improve output from SpecialAbuseLog

https://gerrit.wikimedia.org/r/1228270

Change #1229117 had a related patch set uploaded (by Dragoniez; author: Dragoniez):

[mediawiki/extensions/AbuseFilter@master] Filter out unviewable abuse log entries at the DB query level

https://gerrit.wikimedia.org/r/1229117

Just leaving a note:

https://codesearch.wmcloud.org/search/?q=%5Cb%28audienceCan%7CuserCan%7CuserCanBitfield%29%5Cb&files=&excludeFiles=&repos=Extension%3AAbuseFilter

AbuseFilter uses ::audienceCan, ::userCan, and ::userCanBitfield from different classes in several places but it turned out that passing the composite bit field RevisionRecord::SUPPRESSED_ALL to RevisionRecord::audienceCan is incorrect, and caused a bug where abuse log entries with comment- or user-deleted revisions were not handled properly. We need to investigate whether all other uses are correct.

Dragoniez added a project: Security.
Dragoniez changed the task status from Open to Stalled.Sat, Jan 31, 6:00 AM

Temporarily marking this task as stalled because there's a blocker to address the issue