diff --git a/i18n/en.json b/i18n/en.json index f93b6b9e9..f79aa040c 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -386,6 +386,7 @@ "abusefilter-edit-builder-vars-movedto-last-edit-age": "Time since last move destination page edit in seconds ($1)", "abusefilter-edit-builder-vars-user-editcount": "Edit count of the user ($1)", "abusefilter-edit-builder-vars-user-age": "Age of the user account ($1)", + "abusefilter-edit-builder-vars-user-unnamed-ip": "IP of the user account (for logged-out users and temporary accounts only) ($1)", "abusefilter-edit-builder-vars-user-name": "Name of the user account ($1)", "abusefilter-edit-builder-vars-user-type": "Type of the user account ($1)", "abusefilter-edit-builder-vars-user-groups": "Groups (including implicit) the user is in ($1)", diff --git a/i18n/qqq.json b/i18n/qqq.json index 340ed5a17..6a5c3f797 100644 --- a/i18n/qqq.json +++ b/i18n/qqq.json @@ -431,6 +431,7 @@ "abusefilter-edit-builder-vars-movedto-last-edit-age": "Paraphrase: The number of seconds since the last revision was made on the destination of the page that is to be moved. Returns zero when page is non-existent. Abuse filter syntax option in a dropdown from the group {{msg-mw|abusefilter-edit-builder-group-vars}}.\n\nParameters:\n* $1 - text that will be inserted into the filter code editor", "abusefilter-edit-builder-vars-user-editcount": "Abuse filter syntax option in a dropdown from the group {{msg-mw|abusefilter-edit-builder-group-vars}}.\n\nParameters:\n* $1 - text that will be inserted into the filter code editor", "abusefilter-edit-builder-vars-user-age": "Abuse filter syntax option in a dropdown from the group {{msg-mw|abusefilter-edit-builder-group-vars}}.\n\nParameters:\n* $1 - text that will be inserted into the filter code editor", + "abusefilter-edit-builder-vars-user-unnamed-ip": "Abuse filter syntax option in a dropdown from the group {{msg-mw|abusefilter-edit-builder-group-vars}}.\n\nParameters:\n* $1 - text that will be inserted into the filter code editor", "abusefilter-edit-builder-vars-user-name": "Abuse filter syntax option in a dropdown from the group {{msg-mw|abusefilter-edit-builder-group-vars}}.\n\nParameters:\n* $1 - text that will be inserted into the filter code editor", "abusefilter-edit-builder-vars-user-type": "Abuse filter syntax option in a dropdown from the group {{msg-mw|abusefilter-edit-builder-group-vars}}.\n\nParameters:\n* $1 - text that will be inserted into the filter code editor", "abusefilter-edit-builder-vars-user-groups": "Parameters:\n* $1 - text that will be inserted into the filter code editor\n\nSee also:\n* {{msg-mw|Abusefilter-edit-builder-vars-global-user-groups}}", diff --git a/includes/Api/CheckMatch.php b/includes/Api/CheckMatch.php index c7f549646..24e20bfa8 100644 --- a/includes/Api/CheckMatch.php +++ b/includes/Api/CheckMatch.php @@ -120,7 +120,7 @@ class CheckMatch extends ApiBase { $this->dieWithError( 'apierror-permissiondenied-generic', 'deletedabuselog' ); } - $vars = $this->afVariablesBlobStore->loadVarDump( $row->afl_var_dump ); + $vars = $this->afVariablesBlobStore->loadVarDump( $row ); } if ( $vars === null ) { // @codeCoverageIgnoreStart diff --git a/includes/Api/QueryAbuseLog.php b/includes/Api/QueryAbuseLog.php index a48472ef0..094c5a37a 100644 --- a/includes/Api/QueryAbuseLog.php +++ b/includes/Api/QueryAbuseLog.php @@ -165,6 +165,7 @@ class QueryAbuseLog extends ApiQueryBase { $this->addFields( 'afl_deleted' ); $this->addFields( 'afl_filter_id' ); $this->addFields( 'afl_global' ); + $this->addFields( 'afl_ip' ); $this->addFieldsIf( 'afl_id', $fld_ids ); $this->addFieldsIf( 'afl_user_text', $fld_user ); $this->addFieldsIf( [ 'afl_namespace', 'afl_title' ], $fld_title ); @@ -319,7 +320,7 @@ class QueryAbuseLog extends ApiQueryBase { if ( $fld_details ) { $entry['details'] = []; if ( $canSeeDetails ) { - $vars = $this->afVariablesBlobStore->loadVarDump( $row->afl_var_dump ); + $vars = $this->afVariablesBlobStore->loadVarDump( $row ); $varManager = $this->afVariablesManager; $entry['details'] = $varManager->exportAllVars( $vars ); } diff --git a/includes/KeywordsManager.php b/includes/KeywordsManager.php index 439e09692..5aa4afc66 100644 --- a/includes/KeywordsManager.php +++ b/includes/KeywordsManager.php @@ -202,6 +202,8 @@ class KeywordsManager { 'user_editcount' => 'user-editcount', // Generates abusefilter-edit-builder-vars-user-age 'user_age' => 'user-age', + // Generates abusefilter-edit-builder-vars-user-unnamed-ip + 'user_unnamed_ip' => 'user-unnamed-ip', // Generates abusefilter-edit-builder-vars-user-name 'user_name' => 'user-name', // Generates abusefilter-edit-builder-vars-user-type diff --git a/includes/Pager/AbuseLogPager.php b/includes/Pager/AbuseLogPager.php index 8a184ffc6..f1b5337dd 100644 --- a/includes/Pager/AbuseLogPager.php +++ b/includes/Pager/AbuseLogPager.php @@ -347,6 +347,7 @@ class AbuseLogPager extends ReverseChronologicalPager { 'afl_global', 'afl_filter_id', 'afl_user', + 'afl_ip', 'afl_user_text', 'afl_action', 'afl_actions', diff --git a/includes/Special/SpecialAbuseLog.php b/includes/Special/SpecialAbuseLog.php index 90f2a103f..357d9e3ae 100644 --- a/includes/Special/SpecialAbuseLog.php +++ b/includes/Special/SpecialAbuseLog.php @@ -758,7 +758,8 @@ class SpecialAbuseLog extends AbuseFilterSpecialPage { $output .= Xml::tags( 'p', null, $pager->doFormatRow( $row, false ) ); // Load data - $vars = $this->varBlobStore->loadVarDump( $row->afl_var_dump ); + $vars = $this->varBlobStore->loadVarDump( $row ); + $out->addJsConfigVars( 'wgAbuseFilterVariables', $this->varManager->dumpAllVars( $vars, true ) ); $out->addModuleStyles( 'mediawiki.interface.helpers.styles' ); diff --git a/includes/VariableGenerator/VariableGenerator.php b/includes/VariableGenerator/VariableGenerator.php index 2db84ff7f..656146a9d 100644 --- a/includes/VariableGenerator/VariableGenerator.php +++ b/includes/VariableGenerator/VariableGenerator.php @@ -88,6 +88,15 @@ class VariableGenerator { $this->vars->setVar( 'user_name', $user->getName() ); + $this->vars->setLazyLoadVar( + 'user_unnamed_ip', + 'user-unnamed-ip', + [ + 'user' => $user, + 'rc' => $rc, + ] + ); + $this->vars->setLazyLoadVar( 'user_type', 'user-type', diff --git a/includes/Variables/LazyVariableComputer.php b/includes/Variables/LazyVariableComputer.php index a6603a196..c8efea631 100644 --- a/includes/Variables/LazyVariableComputer.php +++ b/includes/Variables/LazyVariableComputer.php @@ -345,6 +345,27 @@ class LazyVariableComputer { $title = $parameters['title']; $result = $this->restrictionStore->getRestrictions( $title, $action ); break; + case 'user-unnamed-ip': + $user = $parameters['user']; + $result = null; + + // Don't return an IP for past events (eg. revisions, logs) + // This could leak IPs to users who don't have IP viewing rights + if ( !$parameters['rc'] && + // Reveal IPs for: + // - temporary accounts: temporary account names will replace the IP in the `user_name` + // variable. This variable restores this access. + // - logged-out users: This supports the transition to the use of temporary accounts + // so that filter maintainers on pre-transition wikis can migrate `user_name` to `user_unnamed_ip` + // where necessary and see no disruption on transition. + // + // This variable should only ever be exposed for these use cases and shouldn't be extended + // to registered accounts, as that would leak account PII to users without the right to see + // that information + ( $this->userIdentityUtils->isTemp( $user ) || IPUtils::isIPAddress( $user->getName() ) ) ) { + $result = $user->getRequest()->getIP(); + } + break; case 'user-type': /** @var UserIdentity $userIdentity */ $userIdentity = $parameters['user-identity']; diff --git a/includes/Variables/VariablesBlobStore.php b/includes/Variables/VariablesBlobStore.php index 89de4c496..d49f5bc81 100644 --- a/includes/Variables/VariablesBlobStore.php +++ b/includes/Variables/VariablesBlobStore.php @@ -3,9 +3,11 @@ namespace MediaWiki\Extension\AbuseFilter\Variables; use FormatJson; +use InvalidArgumentException; use MediaWiki\Storage\BlobAccessException; use MediaWiki\Storage\BlobStore; use MediaWiki\Storage\BlobStoreFactory; +use stdClass; /** * This service is used to store and load var dumps to a BlobStore @@ -56,6 +58,14 @@ class VariablesBlobStore { // as those are needed for the diff view on top of the abuse log pages $vars = $this->varManager->dumpAllVars( $varsHolder, [ 'old_wikitext', 'new_wikitext' ] ); + // if user_unnamed_ip exists it can't be saved, as var dump blobs are stored in an append-only + // database and stored IPs eventually need to be cleared. + // Set the value to something safe here, as by now it's been used in the filter and if + // logs later need it, it can be reconstructed from afl_ip. + if ( isset( $vars[ 'user_unnamed_ip' ] ) && $vars[ 'user_unnamed_ip' ] ) { + $vars[ 'user_unnamed_ip' ] = true; + } + // Vars is an array with native PHP data types (non-objects) now $text = FormatJson::encode( $vars ); @@ -72,13 +82,22 @@ class VariablesBlobStore { /** * Retrieve a var dump from a BlobStore. * - * @param string $address + * The entire $row is passed through but only the following columns are actually required: + * - afl_var_dump: the main variable store to load + * - afl_ip: the IP value to use if necessary + * + * @param stdClass $row * * @return VariableHolder */ - public function loadVarDump( string $address ): VariableHolder { + public function loadVarDump( stdClass $row ): VariableHolder { + if ( !isset( $row->afl_var_dump ) || !isset( $row->afl_ip ) ) { + throw new InvalidArgumentException( 'Both afl_var_dump and afl_ip must be set' ); + } + try { - $blob = $this->blobStore->getBlob( $address ); + $varDump = $row->afl_var_dump; + $blob = $this->blobStore->getBlob( $varDump ); } catch ( BlobAccessException $ex ) { return new VariableHolder; } @@ -86,6 +105,17 @@ class VariablesBlobStore { $vars = FormatJson::decode( $blob, true ); $obj = VariableHolder::newFromArray( $vars ); $this->varManager->translateDeprecatedVars( $obj ); + + // If user_unnamed_ip was set when afl_var_dump was saved, it was saved as a visibility boolean + // and needs to be translated back into an IP + // user_unnamed_ip uses afl_ip instead of saving the value because afl_ip gets purged and the blob + // that contains user_unnamed_ip can't be modified + if ( + $this->varManager->getVar( $obj, 'user_unnamed_ip', $this->varManager::GET_LAX )->toNative() + ) { + $obj->setVar( 'user_unnamed_ip', $row->afl_ip ); + } + return $obj; } } diff --git a/includes/View/AbuseFilterViewExamine.php b/includes/View/AbuseFilterViewExamine.php index b112f3e8b..19d185b16 100644 --- a/includes/View/AbuseFilterViewExamine.php +++ b/includes/View/AbuseFilterViewExamine.php @@ -248,6 +248,7 @@ class AbuseFilterViewExamine extends AbuseFilterView { $row = $dbr->newSelectQueryBuilder() ->select( [ 'afl_deleted', + 'afl_ip', 'afl_var_dump', 'afl_rev_id', 'afl_filter_id', @@ -287,7 +288,8 @@ class AbuseFilterViewExamine extends AbuseFilterView { return; } - $vars = $this->varBlobStore->loadVarDump( $row->afl_var_dump ); + $vars = $this->varBlobStore->loadVarDump( $row ); + $out->addJsConfigVars( [ 'wgAbuseFilterVariables' => $this->varManager->dumpAllVars( $vars, true ), 'abuseFilterExamine' => [ 'type' => 'log', 'id' => $logid ] diff --git a/includes/View/AbuseFilterViewRevert.php b/includes/View/AbuseFilterViewRevert.php index f9466f8a1..b6a751a88 100644 --- a/includes/View/AbuseFilterViewRevert.php +++ b/includes/View/AbuseFilterViewRevert.php @@ -305,7 +305,7 @@ class AbuseFilterViewRevert extends AbuseFilterView { $actions = explode( ',', $row->afl_actions ); $currentReversibleActions = array_intersect( $actions, $reversibleActions ); if ( count( $currentReversibleActions ) ) { - $vars = $this->varBlobStore->loadVarDump( $row->afl_var_dump ); + $vars = $this->varBlobStore->loadVarDump( $row ); try { // The variable is not lazy-loaded $accountName = $vars->getComputedVariable( 'accountname' )->toNative(); diff --git a/tests/phpunit/AbuseFilterConsequencesTest.php b/tests/phpunit/AbuseFilterConsequencesTest.php index fb2252a8e..1069cc1ff 100644 --- a/tests/phpunit/AbuseFilterConsequencesTest.php +++ b/tests/phpunit/AbuseFilterConsequencesTest.php @@ -1215,14 +1215,14 @@ class AbuseFilterConsequencesTest extends MediaWikiIntegrationTestCase { $this->doAction( $actionParams ); // We just take a dump from a single filters, as they're all identical for the same action - $dumpID = $this->getDb()->newSelectQueryBuilder() - ->select( 'afl_var_dump' ) + $row = $this->getDb()->newSelectQueryBuilder() + ->select( 'afl_var_dump, afl_ip' ) ->from( 'abuse_filter_log' ) ->orderBy( 'afl_timestamp', SelectQueryBuilder::SORT_DESC ) ->caller( __METHOD__ ) - ->fetchField(); + ->fetchRow(); - $vars = AbuseFilterServices::getVariablesBlobStore()->loadVarDump( $dumpID )->getVars(); + $vars = AbuseFilterServices::getVariablesBlobStore()->loadVarDump( $row )->getVars(); $interestingVars = array_intersect_key( $vars, array_fill_keys( $usedVars, true ) ); diff --git a/tests/phpunit/unit/VariableGenerator/VariableGeneratorTest.php b/tests/phpunit/unit/VariableGenerator/VariableGeneratorTest.php index 7db0bbf77..b314a79ff 100644 --- a/tests/phpunit/unit/VariableGenerator/VariableGeneratorTest.php +++ b/tests/phpunit/unit/VariableGenerator/VariableGeneratorTest.php @@ -48,7 +48,8 @@ class VariableGeneratorTest extends MediaWikiUnitTestCase { 'user_groups', 'user_rights', 'user_blocked', - 'user_age' + 'user_age', + 'user_unnamed_ip', ]; $variableHolder = new VariableHolder(); diff --git a/tests/phpunit/unit/Variables/LazyVariableComputerTest.php b/tests/phpunit/unit/Variables/LazyVariableComputerTest.php index 1c9edd9f6..0cd407dc7 100644 --- a/tests/phpunit/unit/Variables/LazyVariableComputerTest.php +++ b/tests/phpunit/unit/Variables/LazyVariableComputerTest.php @@ -14,6 +14,7 @@ use MediaWiki\Extension\AbuseFilter\Variables\LazyVariableComputer; use MediaWiki\Extension\AbuseFilter\Variables\VariableHolder; use MediaWiki\Permissions\PermissionManager; use MediaWiki\Permissions\RestrictionStore; +use MediaWiki\Request\WebRequest; use MediaWiki\Revision\RevisionLookup; use MediaWiki\Revision\RevisionRecord; use MediaWiki\Revision\RevisionStore; @@ -150,7 +151,7 @@ class LazyVariableComputerTest extends MediaWikiUnitTestCase { $getUserVar = static function ( $user, $method ): LazyLoadedVariable { return new LazyLoadedVariable( $method, - [ 'user' => $user, 'user-identity' => $user ] + [ 'user' => $user, 'user-identity' => $user, 'rc' => null ] ); }; @@ -195,6 +196,30 @@ class LazyVariableComputerTest extends MediaWikiUnitTestCase { $var = $getUserVar( $user, 'user-type' ); yield 'user_type for unregistered username' => [ $var, 'unknown' ]; + $request = $this->createMock( WebRequest::class ); + $request->method( 'getIP' )->willReturn( '127.0.0.1' ); + $user = $this->createMock( User::class ); + $user->method( 'getRequest' )->willReturn( $request ); + $user->method( 'getName' )->willReturn( '127.0.0.1' ); + $var = $getUserVar( $user, 'user-unnamed-ip' ); + yield 'user_unnamed_ip for an anonymous user' => [ $var, '127.0.0.1' ]; + + $user = $this->createMock( User::class ); + $user->method( 'getName' )->willReturn( 'Test User' ); + $var = $getUserVar( $user, 'user-unnamed-ip' ); + yield 'user_unnamed_ip for a user' => [ $var, null ]; + + $mockUserIdentityUtils = $this->createMock( UserIdentityUtils::class ); + $mockUserIdentityUtils->method( 'isTemp' )->with( $user )->willReturn( true ); + $request = $this->createMock( WebRequest::class ); + $request->method( 'getIP' )->willReturn( '127.0.0.1' ); + $user = $this->createMock( User::class ); + $user->method( 'getRequest' )->willReturn( $request ); + $var = $getUserVar( $user, 'user-unnamed-ip' ); + yield 'user_unnamed_ip for a temp user' => [ + $var, '127.0.0.1', [ 'UserIdentityUtils' => $mockUserIdentityUtils ] + ]; + $user = $this->createMock( User::class ); $groups = [ '*', 'group1', 'group2' ]; $userGroupManager = $this->createMock( UserGroupManager::class ); diff --git a/tests/phpunit/unit/Variables/VariablesBlobStoreTest.php b/tests/phpunit/unit/Variables/VariablesBlobStoreTest.php index c2c59c6e5..eab8e1927 100644 --- a/tests/phpunit/unit/Variables/VariablesBlobStoreTest.php +++ b/tests/phpunit/unit/Variables/VariablesBlobStoreTest.php @@ -3,6 +3,7 @@ namespace MediaWiki\Extension\AbuseFilter\Tests\Unit\Variables; use FormatJson; +use InvalidArgumentException; use MediaWiki\Extension\AbuseFilter\KeywordsManager; use MediaWiki\Extension\AbuseFilter\Parser\AFPData; use MediaWiki\Extension\AbuseFilter\Variables\VariableHolder; @@ -12,6 +13,7 @@ use MediaWiki\Storage\BlobAccessException; use MediaWiki\Storage\BlobStore; use MediaWiki\Storage\BlobStoreFactory; use MediaWikiUnitTestCase; +use stdClass; use Wikimedia\TestingAccessWrapper; /** @@ -67,17 +69,86 @@ class VariablesBlobStoreTest extends MediaWikiUnitTestCase { $blob = FormatJson::encode( $vars ); $blobStore = $this->createMock( BlobStore::class ); $blobStore->expects( $this->once() )->method( 'getBlob' )->willReturn( $blob ); + + $row = new stdClass; + $row->afl_var_dump = $blob; + $row->afl_ip = ''; $varBlobStore = $this->getStore( null, $blobStore ); - $loadedVars = $varBlobStore->loadVarDump( 'foo' )->getVars(); + $loadedVars = $varBlobStore->loadVarDump( $row )->getVars(); $this->assertArrayHasKey( 'foo-variable', $loadedVars ); $this->assertSame( 42, $loadedVars['foo-variable']->toNative() ); } + /** + * @dataProvider provideLoadVarDumpVarTransformation + */ + public function testLoadVarDumpVarTransformation( $data, $expected ) { + $vars = [ + 'user_unnamed_ip' => $data[ 'user_unnamed_ip' ] + ]; + + $manager = $this->createMock( VariablesManager::class ); + $manager->method( 'getVar' )->willReturn( AFPData::newFromPHPVar( $data['user_unnamed_ip'] ) ); + $blob = FormatJson::encode( $vars ); + $blobStore = $this->createMock( BlobStore::class ); + $blobStore->expects( $this->once() )->method( 'getBlob' )->willReturn( $blob ); + $row = new stdClass; + $row->afl_var_dump = $blob; + $row->afl_ip = $data[ 'afl_ip' ]; + $varBlobStore = new VariablesBlobStore( + $manager, + $this->createMock( BlobStoreFactory::class ), + $blobStore, + null + ); + $loadedVars = $varBlobStore->loadVarDump( $row )->getVars(); + $this->assertArrayHasKey( 'user_unnamed_ip', $loadedVars ); + $this->assertSame( $expected, $loadedVars[ 'user_unnamed_ip' ]->toNative() ); + } + + /** + * Data provider for testLoadVarDumpVarTransformation + * + * @return array + */ + public static function provideLoadVarDumpVarTransformation() { + return [ + 'ip visible, ip available' => [ + [ + 'user_unnamed_ip' => true, + 'afl_ip' => '1.2.3.4', + ], + '1.2.3.4' + ], + 'ip visible, ip cleared' => [ + [ + 'user_unnamed_ip' => true, + 'afl_ip' => '', + ], + '' + ], + 'ip not visible, ip available' => [ + [ + 'user_unnamed_ip' => null, + 'afl_ip' => '1.2.3.4', + ], + null + ] + ]; + } + public function testLoadVarDump_fail() { $blobStore = $this->createMock( BlobStore::class ); $blobStore->expects( $this->once() )->method( 'getBlob' )->willThrowException( new BlobAccessException ); $varBlobStore = $this->getStore( null, $blobStore ); - $this->assertCount( 0, $varBlobStore->loadVarDump( 'foo' )->getVars() ); + $row = new stdClass; + $row->afl_var_dump = ''; + $row->afl_ip = ''; + $this->assertCount( 0, $varBlobStore->loadVarDump( $row )->getVars() ); + + $row = new stdClass; + $this->expectException( InvalidArgumentException::class ); + $varBlobStore->loadVarDump( $row )->getVars(); } private function getBlobStore(): BlobStore { @@ -117,7 +188,10 @@ class VariablesBlobStoreTest extends MediaWikiUnitTestCase { $storeID = $varBlobStore->storeVarDump( VariableHolder::newFromArray( $toStore ) ); $this->assertIsString( $storeID ); - $loadedVars = $varBlobStore->loadVarDump( $storeID )->getVars(); + $row = new stdClass; + $row->afl_var_dump = $storeID; + $row->afl_ip = ''; + $loadedVars = $varBlobStore->loadVarDump( $row )->getVars(); $nativeLoadedVars = array_map( static function ( AFPData $el ) { return $el->toNative(); }, $loadedVars ); @@ -197,7 +271,15 @@ class VariablesBlobStoreTest extends MediaWikiUnitTestCase { 'action' => 'createaccount', 'accountname' => 'XXX' ] - ] + ], + 'User IP' => [ + [ + 'user_unnamed_ip' => '1.2.3.4' + ], + [ + 'user_unnamed_ip' => true + ] + ], ]; } }