Page MenuHomePhabricator

AbuseLog excludes revisions with any type of rev-del from non-oversighters
Closed, ResolvedPublicBUG REPORT

Description

As an admin,
When I visit Special:AbuseLog
And an entry has a revision associated with it that is revision-deleted at admin level

Expected result: as an admin, I can see the filter entry
Actual result: I don't

Notes:

SpecialAbuseLog::formatRow
public function formatRow( $row, $isListItem = true ) {
	$user = $this->getUser();
	[...]
	$isHidden = self::isHidden( $row );
	// @todo T224203 Try to show the details if the revision is deleted but the AbuseLog entry
	// is not. However, watch out to avoid showing too much stuff.
	if ( !self::canSeeHidden( $user ) && $isHidden ) {
		return '';
	}
	[...]
SpecialAbuseLog::isHidden
/**
 * Given a log entry row, decides whether or not it can be viewed by the public.
 *
 * @param stdClass $row The abuse_filter_log row object.
 *
 * @return bool|string true if the item is explicitly hidden, false if it is not.
 *    The string 'implicit' if it is hidden because the corresponding revision is hidden.
 */
public static function isHidden( $row ) {
	// First, check if the entry is hidden. Since this is an oversight-level deletion,
	// it's more important than the associated revision being deleted.
	if ( $row->afl_deleted ) {
		return true;
	}
	if ( $row->afl_rev_id ) {
		$revision = MediaWikiServices::getInstance()
			->getRevisionLookup()
			->getRevisionById( $row->afl_rev_id );
		if ( $revision && $revision->getVisibility() !== 0 ) {
			return 'implicit';
		}
	}

	return false;
}

Since the revision's visibility is not 0, it is implicitly hidden, which is silently converted to true in the check for whether the user can view the revision.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
DannyS712 changed the subtype of this task from "Task" to "Bug Report".Aug 28 2020, 9:50 PM
DannyS712 moved this task from Unsorted to Abuse Filter on the User-DannyS712 board.
DannyS712 updated the task description. (Show Details)
DannyS712 added a subscriber: Daimona.

This code path (together with others) has been responsible for countless info leaks, so I'm not very comfortable with touching it.

This is somewhat related to T233324 and the patches there. For now, I'm adding this task to the list of candidates for the upcoming AF overhaul. Any change to that code needs very careful review, ideally by more than one person.

Change 721375 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/extensions/AbuseFilter@master] Add method to properly check visibility of AbuseLog entries

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

Change 721375 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Add method to properly check visibility of AbuseLog entries

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