From 864d2b7e030735a7d52b8dc894c9d0f1c747a4eb Mon Sep 17 00:00:00 2001 From: Daimona Eaytoy Date: Wed, 3 Oct 2018 11:59:31 +0200 Subject: [PATCH] Don't run filters with null title As all title variables would be null, and the result pretty meaningless. NOTE: Please vote V+2 and submit manually. I359a618ffc4e45ce1fb70f2d should then be +2ed right after that. This way, there is no need to create two more patches just for a handful of tests being broken for a minute. Bug: T144265 Bug: T219030 Depends-On: If6b91711534c0d60e1aa27bd5748c3023e29f376 Change-Id: I5a9db6e7c4356c9662a0b0a51e66252555b3d998 --- includes/AbuseFilter.php | 86 +++++++++++++++++------------------ includes/AbuseFilterHooks.php | 20 ++++---- 2 files changed, 51 insertions(+), 55 deletions(-) diff --git a/includes/AbuseFilter.php b/includes/AbuseFilter.php index 1a683f2c7..1e05c0ba6 100644 --- a/includes/AbuseFilter.php +++ b/includes/AbuseFilter.php @@ -557,16 +557,16 @@ class AbuseFilter { * Returns an associative array of filters which were tripped * * @param AbuseFilterVariableHolder $vars + * @param Title $title * @param string $group The filter's group (as defined in $wgAbuseFilterValidGroups) - * @param Title|null $title * @param string $mode 'execute' for edits and logs, 'stash' for cached matches * * @return bool[] Map of (integer filter ID => bool) */ public static function checkAllFilters( AbuseFilterVariableHolder $vars, + Title $title, $group = 'default', - Title $title = null, $mode = 'execute' ) { global $wgAbuseFilterCentralDB, $wgAbuseFilterIsCentral, $wgAbuseFilterConditionLimit; @@ -639,7 +639,7 @@ class AbuseFilter { } } - if ( $title instanceof Title && self::$condCount > $wgAbuseFilterConditionLimit ) { + if ( self::$condCount > $wgAbuseFilterConditionLimit ) { $action = $vars->getVar( 'action' )->toString(); if ( strpos( $action, 'createaccount' ) === false ) { $username = $vars->getVar( 'user_name' )->toString(); @@ -664,7 +664,7 @@ class AbuseFilter { /** * @param stdClass $row * @param AbuseFilterVariableHolder $vars - * @param Title|null $title + * @param Title $title * @param string $prefix * @param string $mode 'execute' for edits and logs, 'stash' for cached matches * @return bool @@ -672,7 +672,7 @@ class AbuseFilter { public static function checkFilter( $row, AbuseFilterVariableHolder $vars, - Title $title = null, + Title $title, $prefix = '', $mode = 'execute' ) { @@ -726,20 +726,18 @@ class AbuseFilter { * @param float $runtime * @param int $totalConditions * @param bool $matched - * @param Title|null $title + * @param Title $title */ private static function recordSlowFilter( - $filterId, $runtime, $totalConditions, $matched, Title $title = null + $filterId, $runtime, $totalConditions, $matched, Title $title ) { - $title = $title ? $title->getPrefixedText() : ''; - $logger = LoggerFactory::getInstance( 'AbuseFilter' ); $logger->info( 'Edit filter {filter_id} on {wiki} is taking longer than expected', [ 'wiki' => wfWikiID(), 'filter_id' => $filterId, - 'title' => $title, + 'title' => $title->getPrefixedText(), 'runtime' => $runtime, 'matched' => $matched, 'total_conditions' => $totalConditions @@ -1193,7 +1191,7 @@ class AbuseFilter { $statsd->increment( 'abusefilter.check-stash.hit' ); } } else { - $filter_matched = self::checkAllFilters( $vars, $group, $title, $mode ); + $filter_matched = self::checkAllFilters( $vars, $title, $group, $mode ); if ( $isForEdit && $mode !== 'stash' ) { $logger->debug( __METHOD__ . ": cache miss for '$title' (key $stashKey)." ); $statsd->increment( 'abusefilter.check-stash.miss' ); @@ -1278,7 +1276,7 @@ class AbuseFilter { if ( $cacheData ) { DeferredUpdates::addCallableUpdate( function () use ( $vars, $group, $title, $actions_taken, $log_template ) { - self::checkAllFilters( $vars, $group, $title, 'execute' ); + self::checkAllFilters( $vars, $title, $group, 'execute' ); self::addLogEntries( $actions_taken, $log_template, $vars, $group ); } ); } else { @@ -3025,15 +3023,15 @@ class AbuseFilter { } /** - * @param Title|null $title + * @param Title $title * @param Page|null $page * @return AbuseFilterVariableHolder */ - public static function getEditVars( Title $title = null, Page $page = null ) { + public static function getEditVars( Title $title, Page $page = null ) { $vars = new AbuseFilterVariableHolder; // NOTE: $page may end up remaining null, e.g. if $title points to a special page. - if ( !$page && $title instanceof Title && $title->canExist() ) { + if ( !$page && $title->canExist() ) { $page = WikiPage::factory( $title ); } @@ -3062,36 +3060,34 @@ class AbuseFilter { $vars->setLazyLoadVar( 'new_text', 'strip-html', [ 'html-var' => 'new_html' ] ); - if ( $title instanceof Title ) { - $vars->setLazyLoadVar( 'all_links', 'links-from-wikitext', - [ - 'namespace' => $title->getNamespace(), - 'title' => $title->getText(), - 'text-var' => 'new_wikitext', - 'article' => $page - ] ); - $vars->setLazyLoadVar( 'old_links', 'links-from-wikitext-or-database', - [ - 'namespace' => $title->getNamespace(), - 'title' => $title->getText(), - 'text-var' => 'old_wikitext' - ] ); - $vars->setLazyLoadVar( 'new_pst', 'parse-wikitext', - [ - 'namespace' => $title->getNamespace(), - 'title' => $title->getText(), - 'wikitext-var' => 'new_wikitext', - 'article' => $page, - 'pst' => true, - ] ); - $vars->setLazyLoadVar( 'new_html', 'parse-wikitext', - [ - 'namespace' => $title->getNamespace(), - 'title' => $title->getText(), - 'wikitext-var' => 'new_wikitext', - 'article' => $page - ] ); - } + $vars->setLazyLoadVar( 'all_links', 'links-from-wikitext', + [ + 'namespace' => $title->getNamespace(), + 'title' => $title->getText(), + 'text-var' => 'new_wikitext', + 'article' => $page + ] ); + $vars->setLazyLoadVar( 'old_links', 'links-from-wikitext-or-database', + [ + 'namespace' => $title->getNamespace(), + 'title' => $title->getText(), + 'text-var' => 'old_wikitext' + ] ); + $vars->setLazyLoadVar( 'new_pst', 'parse-wikitext', + [ + 'namespace' => $title->getNamespace(), + 'title' => $title->getText(), + 'wikitext-var' => 'new_wikitext', + 'article' => $page, + 'pst' => true, + ] ); + $vars->setLazyLoadVar( 'new_html', 'parse-wikitext', + [ + 'namespace' => $title->getNamespace(), + 'title' => $title->getText(), + 'wikitext-var' => 'new_wikitext', + 'article' => $page + ] ); return $vars; } diff --git a/includes/AbuseFilterHooks.php b/includes/AbuseFilterHooks.php index 349d37ee5..a994e0acd 100644 --- a/includes/AbuseFilterHooks.php +++ b/includes/AbuseFilterHooks.php @@ -83,10 +83,11 @@ class AbuseFilterHooks { Status $status, $summary, $slot = SlotRecord::MAIN ) { $title = $context->getTitle(); - if ( !$title ) { - // T144265 + if ( $title === null ) { + // T144265: This *should* never happen. $logger = LoggerFactory::getInstance( 'AbuseFilter' ); - $logger->warning( __METHOD__ . ' received a null title. (T144265)' ); + $logger->warning( __METHOD__ . ' received a null title.' ); + return Status::newGood(); } self::$successful_action_vars = false; @@ -96,7 +97,7 @@ class AbuseFilterHooks { $oldContent = null; - if ( ( $title instanceof Title ) && $title->canExist() && $title->exists() ) { + if ( $title->canExist() && $title->exists() ) { // Make sure we load the latest text saved in database (bug 31656) $page = $context->getWikiPage(); $oldRevision = $page->getRevision(); @@ -775,13 +776,12 @@ class AbuseFilterHooks { $props, $summary, $text, &$error ) { $title = $upload->getTitle(); - if ( !$title ) { - // T144265 + if ( $title === null ) { + // T144265: This could happen for 'stashupload' if the specified title is invalid. + // Let UploadBase warn the user about that, and we'll filter later. $logger = LoggerFactory::getInstance( 'AbuseFilter' ); - $err = $upload->validateName()['status']; - $logger->warning( __METHOD__ . ' received a null title.' . - "Action: $action. Title error: $err. (T144265)" - ); + $logger->warning( __METHOD__ . " received a null title. Action: $action." ); + return true; } $mimeAnalyzer = MediaWikiServices::getInstance()->getMimeAnalyzer();