diff --git a/includes/AbuseFilter.php b/includes/AbuseFilter.php index 14a1d2b1f..1af2a6571 100644 --- a/includes/AbuseFilter.php +++ b/includes/AbuseFilter.php @@ -682,7 +682,7 @@ class AbuseFilter { * @param AbuseFilterVariableHolder $vars * @param bool $global * - * @return int|null + * @return int The insert ID. */ public static function storeVarDump( AbuseFilterVariableHolder $vars, $global = false ) { global $wgCompressRevisions; @@ -692,8 +692,8 @@ class AbuseFilter { $vars = $vars->dumpAllVars( [ 'old_wikitext', 'new_wikitext' ] ); // Vars is an array with native PHP data types (non-objects) now - $text = serialize( $vars ); - $flags = [ 'nativeDataArray', 'utf-8' ]; + $text = FormatJson::encode( $vars ); + $flags = [ 'utf-8' ]; if ( $wgCompressRevisions && function_exists( 'gzdeflate' ) ) { $text = gzdeflate( $text ); @@ -708,12 +708,8 @@ class AbuseFilter { } else { $text = ExternalStore::insertToDefault( $text ); } - $flags[] = 'external'; - if ( !$text ) { - // Not mission-critical, just return nothing - return null; - } + $flags[] = 'external'; } // Store to text table @@ -738,16 +734,27 @@ class AbuseFilter { * * @param string $stored_dump * - * @return array|object|AbuseFilterVariableHolder|bool + * @return AbuseFilterVariableHolder|array */ public static function loadVarDump( $stored_dump ) { - // Backward compatibility - if ( substr( $stored_dump, 0, strlen( 'stored-text:' ) ) !== 'stored-text:' ) { + // Backward compatibility for (old) blobs stored in the abuse_filter_log table + if ( !is_numeric( $stored_dump ) && + substr( $stored_dump, 0, strlen( 'stored-text:' ) ) !== 'stored-text:' && + substr( $stored_dump, 0, strlen( 'tt:' ) ) !== 'tt:' + ) { $data = unserialize( $stored_dump ); return is_array( $data ) ? AbuseFilterVariableHolder::newFromArray( $data ) : $data; } - $text_id = substr( $stored_dump, strlen( 'stored-text:' ) ); + if ( is_numeric( $stored_dump ) ) { + $text_id = (int)$stored_dump; + } elseif ( strpos( $stored_dump, 'stored-text:' ) !== false ) { + $text_id = (int)str_replace( 'stored-text:', '', $stored_dump ); + } elseif ( strpos( $stored_dump, 'tt:' ) !== false ) { + $text_id = (int)str_replace( 'tt:', '', $stored_dump ); + } else { + throw new LogicException( "Cannot understand format: $stored_dump" ); + } $dbr = wfGetDB( DB_REPLICA ); @@ -759,10 +766,12 @@ class AbuseFilter { ); if ( !$text_row ) { + $logger = LoggerFactory::getInstance( 'AbuseFilter' ); + $logger->warning( __METHOD__ . ": no text row found for input $stored_dump." ); return new AbuseFilterVariableHolder; } - $flags = explode( ',', $text_row->old_flags ); + $flags = $text_row->old_flags === '' ? [] : explode( ',', $text_row->old_flags ); $text = $text_row->old_text; if ( in_array( 'external', $flags ) ) { @@ -773,9 +782,17 @@ class AbuseFilter { $text = gzinflate( $text ); } - $obj = unserialize( $text ); + $obj = FormatJson::decode( $text, true ); + if ( $obj === null ) { + // Temporary code until all rows will be JSON-encoded + $obj = unserialize( $text ); + } - if ( in_array( 'nativeDataArray', $flags ) ) { + if ( in_array( 'nativeDataArray', $flags ) || + // Temporary condition: we don't add the flag anymore, but the updateVarDump + // script could be still running and we cannot assume that this branch is the default. + ( is_array( $obj ) && array_key_exists( 'action', $obj ) ) + ) { $vars = $obj; $obj = AbuseFilterVariableHolder::newFromArray( $vars ); // If old variable names are used, make sure to keep them diff --git a/includes/AbuseFilterRunner.php b/includes/AbuseFilterRunner.php index 0aba118f2..24fc4b3da 100644 --- a/includes/AbuseFilterRunner.php +++ b/includes/AbuseFilterRunner.php @@ -1208,8 +1208,7 @@ class AbuseFilterRunner { // Only store the var dump if we're actually going to add log rows. $varDump = AbuseFilter::storeVarDump( $this->vars ); - // To distinguish from stuff stored directly - $varDump = "stored-text:$varDump"; + $varDump = "tt:$varDump"; $localLogIDs = []; global $wgAbuseFilterNotifications, $wgAbuseFilterNotificationsPrivate; @@ -1272,7 +1271,7 @@ class AbuseFilterRunner { if ( count( $loggedGlobalFilters ) ) { $this->vars->computeDBVars(); $globalVarDump = AbuseFilter::storeVarDump( $this->vars, true ); - $globalVarDump = "stored-text:$globalVarDump"; + $globalVarDump = "tt:$globalVarDump"; foreach ( $centralLogRows as $index => $data ) { $centralLogRows[$index]['afl_var_dump'] = $globalVarDump; } diff --git a/tests/phpunit/AbuseFilterDBTest.php b/tests/phpunit/AbuseFilterDBTest.php index 0bd61fcb6..b37b44902 100644 --- a/tests/phpunit/AbuseFilterDBTest.php +++ b/tests/phpunit/AbuseFilterDBTest.php @@ -248,10 +248,7 @@ class AbuseFilterDBTest extends MediaWikiTestCase { public function testVarDump( $variables ) { global $wgCompressRevisions, $wgDefaultExternalStore; - $holder = new AbuseFilterVariableHolder(); - foreach ( $variables as $name => $value ) { - $holder->setVar( $name, $value ); - } + $holder = AbuseFilterVariableHolder::newFromArray( $variables ); if ( array_intersect_key( AbuseFilter::getDeprecatedVariables(), $variables ) ) { $holder->mVarsVersion = 1; } @@ -266,9 +263,9 @@ class AbuseFilterDBTest extends MediaWikiTestCase { [ 'ORDER BY' => 'old_id DESC' ] ); $this->assertNotFalse( $flags, 'The var dump has not been saved.' ); - $flags = explode( ',', $flags ); + $flags = $flags === '' ? [] : explode( ',', $flags ); - $expectedFlags = [ 'nativeDataArray', 'utf-8' ]; + $expectedFlags = [ 'utf-8' ]; if ( $wgCompressRevisions ) { $expectedFlags[] = 'gzip'; } @@ -291,12 +288,14 @@ class AbuseFilterDBTest extends MediaWikiTestCase { return [ 'Only basic variables' => [ [ + 'action' => 'edit', 'old_wikitext' => 'Old text', 'new_wikitext' => 'New text' ] ], - [ + 'Normal case' => [ [ + 'action' => 'edit', 'old_wikitext' => 'Old text', 'new_wikitext' => 'New text', 'user_editcount' => 15, @@ -305,22 +304,16 @@ class AbuseFilterDBTest extends MediaWikiTestCase { ], 'Deprecated variables' => [ [ + 'action' => 'edit', 'old_wikitext' => 'Old text', 'new_wikitext' => 'New text', 'article_articleid' => 11745, 'article_first_contributor' => 'Good guy' ] ], - [ - [ - 'old_wikitext' => 'Old text', - 'new_wikitext' => 'New text', - 'page_title' => 'Some title', - 'summary' => 'Fooooo' - ] - ], - [ + 'Move action' => [ [ + 'action' => 'move', 'old_wikitext' => 'Old text', 'new_wikitext' => 'New text', 'all_links' => [ 'https://en.wikipedia.org' ], @@ -329,7 +322,7 @@ class AbuseFilterDBTest extends MediaWikiTestCase { 'new_content_model' => CONTENT_MODEL_JAVASCRIPT ] ], - [ + 'Delete action' => [ [ 'old_wikitext' => 'Old text', 'new_wikitext' => 'New text', @@ -338,21 +331,20 @@ class AbuseFilterDBTest extends MediaWikiTestCase { 'page_namespace' => 114 ] ], - [ - [ - 'old_wikitext' => 'Old text', - 'new_wikitext' => 'New text', - 'new_html' => 'Foo bar lol.', - 'new_pst' => '[[Link|link]] test {{blah}}.' - ] - ], 'Disabled vars' => [ [ + 'action' => 'edit', 'old_wikitext' => 'Old text', 'new_wikitext' => 'New text', 'old_html' => 'Foo bar lol.', 'old_text' => 'Foobar' ] + ], + 'Account creation' => [ + [ + 'action' => 'createaccount', + 'accountname' => 'XXX' + ] ] ]; }