From 4c1a0a4f95c76f2d5f8404160daa381bc09887a7 Mon Sep 17 00:00:00 2001 From: Dreamy Jazz Date: Wed, 18 Jun 2025 10:55:40 +0100 Subject: [PATCH] SECURITY: Update AbuseFilterViewTestBatch to log protected vars access Why: * Special:AbuseFilter/test allows users to match a given filter pattern against recentchanges table entries. * This correctly disallows users using protected variables (unless they have the permissions needed to use those variables). ** However, the system does not log that the user accessed the protected variable values for the users. * This is an issue because the user can use a trial-and-error method to determine the value of protected variables. ** For example, IPReputation variables can sometimes have a very few possible values that makes it feasible to make a few attempts determine the values for a bunch of changes. * Logging that the user accessed the value of the variables in the test pattern avoids this trial-and-error method causing no log entries. ** To prevent unnecessarily logs, this should only happen if the value of the variable is not `null`. This is the behaviour of used by other interfaces to determine whether to log. What: * Update AbuseFilterViewTestBatch::doTest to create log entry if the pattern contains protected variables and the entry being tested against has the protected variable value set (i.e. not `null`). * Add tests to verify this change. Bug: T397221 Change-Id: I429e155d6b5998ba47ef8929a457599cab12dee8 --- includes/Special/SpecialAbuseFilter.php | 2 + includes/View/AbuseFilterViewTestBatch.php | 32 +++++ .../Special/SpecialAbuseFilterTest.php | 113 ++++++++++++------ 3 files changed, 108 insertions(+), 39 deletions(-) diff --git a/includes/Special/SpecialAbuseFilter.php b/includes/Special/SpecialAbuseFilter.php index 33333460..b5e38ee6 100644 --- a/includes/Special/SpecialAbuseFilter.php +++ b/includes/Special/SpecialAbuseFilter.php @@ -103,6 +103,8 @@ class SpecialAbuseFilter extends AbuseFilterSpecialPage { EditBoxBuilderFactory::SERVICE_NAME, RuleCheckerFactory::SERVICE_NAME, VariableGeneratorFactory::SERVICE_NAME, + VariablesManager::SERVICE_NAME, + AbuseLoggerFactory::SERVICE_NAME, ], AbuseFilterViewTools::class => [ AbuseFilterPermissionManager::SERVICE_NAME, diff --git a/includes/View/AbuseFilterViewTestBatch.php b/includes/View/AbuseFilterViewTestBatch.php index 5bd635de..33babe2c 100644 --- a/includes/View/AbuseFilterViewTestBatch.php +++ b/includes/View/AbuseFilterViewTestBatch.php @@ -5,10 +5,12 @@ namespace MediaWiki\Extension\AbuseFilter\View; use MediaWiki\Context\IContextSource; use MediaWiki\Extension\AbuseFilter\AbuseFilterChangesList; use MediaWiki\Extension\AbuseFilter\AbuseFilterPermissionManager; +use MediaWiki\Extension\AbuseFilter\AbuseLoggerFactory; use MediaWiki\Extension\AbuseFilter\EditBox\EditBoxBuilderFactory; use MediaWiki\Extension\AbuseFilter\EditBox\EditBoxField; use MediaWiki\Extension\AbuseFilter\Parser\RuleCheckerFactory; use MediaWiki\Extension\AbuseFilter\VariableGenerator\VariableGeneratorFactory; +use MediaWiki\Extension\AbuseFilter\Variables\VariablesManager; use MediaWiki\Html\Html; use MediaWiki\HTMLForm\HTMLForm; use MediaWiki\Linker\LinkRenderer; @@ -47,6 +49,8 @@ class AbuseFilterViewTestBatch extends AbuseFilterView { * @var VariableGeneratorFactory */ private $varGeneratorFactory; + private VariablesManager $variablesManager; + private AbuseLoggerFactory $abuseLoggerFactory; /** * @param LBFactory $lbFactory @@ -54,6 +58,8 @@ class AbuseFilterViewTestBatch extends AbuseFilterView { * @param EditBoxBuilderFactory $boxBuilderFactory * @param RuleCheckerFactory $ruleCheckerFactory * @param VariableGeneratorFactory $varGeneratorFactory + * @param VariablesManager $variablesManager + * @param AbuseLoggerFactory $abuseLoggerFactory * @param IContextSource $context * @param LinkRenderer $linkRenderer * @param string $basePageName @@ -65,6 +71,8 @@ class AbuseFilterViewTestBatch extends AbuseFilterView { EditBoxBuilderFactory $boxBuilderFactory, RuleCheckerFactory $ruleCheckerFactory, VariableGeneratorFactory $varGeneratorFactory, + VariablesManager $variablesManager, + AbuseLoggerFactory $abuseLoggerFactory, IContextSource $context, LinkRenderer $linkRenderer, string $basePageName, @@ -75,6 +83,8 @@ class AbuseFilterViewTestBatch extends AbuseFilterView { $this->boxBuilderFactory = $boxBuilderFactory; $this->ruleCheckerFactory = $ruleCheckerFactory; $this->varGeneratorFactory = $varGeneratorFactory; + $this->variablesManager = $variablesManager; + $this->abuseLoggerFactory = $abuseLoggerFactory; } /** @@ -297,6 +307,28 @@ class AbuseFilterViewTestBatch extends AbuseFilterView { continue; } + // If the test filter pattern contains protected variables and this entry had a value set for the + // protected variables that were in the pattern, then log that protected variables were accessed. + // This is to avoid a user being able to know the value of the variable if they repeatedly try values to + // find the actual value through trial-and-error. + $usedVars = $ruleChecker->getUsedVars( $this->testPattern ); + $protectedVariableValuesShown = []; + $varsArray = $this->variablesManager->dumpAllVars( $vars, true ); + foreach ( $this->afPermManager->getUsedProtectedVariables( $usedVars ) as $protectedVariable ) { + if ( isset( $varsArray[$protectedVariable] ) ) { + $protectedVariableValuesShown[] = $protectedVariable; + } + } + + if ( count( $protectedVariableValuesShown ) ) { + $logger = $this->abuseLoggerFactory->getProtectedVarsAccessLogger(); + $logger->logViewProtectedVariableValue( + $this->getUser(), + $varsArray['user_name'] ?? $varsArray['accountname'], + $protectedVariableValuesShown + ); + } + $ruleChecker->setVariables( $vars ); $result = $ruleChecker->checkConditions( $this->testPattern )->getResult(); diff --git a/tests/phpunit/integration/Special/SpecialAbuseFilterTest.php b/tests/phpunit/integration/Special/SpecialAbuseFilterTest.php index f4fb2cc2..829bdca6 100644 --- a/tests/phpunit/integration/Special/SpecialAbuseFilterTest.php +++ b/tests/phpunit/integration/Special/SpecialAbuseFilterTest.php @@ -34,6 +34,8 @@ use MediaWiki\Permissions\Authority; use MediaWiki\Permissions\UltimateAuthority; use MediaWiki\Request\FauxRequest; use MediaWiki\Tests\Unit\Permissions\MockAuthorityTrait; +use MediaWiki\Title\Title; +use MediaWiki\User\UserIdentity; use SpecialPageTestBase; use Wikimedia\Parsoid\Utils\DOMCompat; use Wikimedia\Parsoid\Utils\DOMUtils; @@ -62,6 +64,7 @@ class SpecialAbuseFilterTest extends SpecialPageTestBase { private Authority $authorityCannotUseProtectedVar; private Authority $authorityCanUseProtectedVar; private static int $recentChangeId; + private static string $userWhoHitFilter; protected function setUp(): void { parent::setUp(); @@ -97,12 +100,21 @@ class SpecialAbuseFilterTest extends SpecialPageTestBase { ); } + protected function tearDown(): void { + parent::tearDown(); + + // Clear protected variable access logs between tests to avoid failures, as the + // test class does not truncate the 'logging' table between tests. + $this->dropProtectedVarAccessLogs(); + } + /** * @inheritDoc */ public function addDBDataOnce() { $filterStore = AbuseFilterServices::getFilterStore(); $performer = $this->getTestSysop()->getUserIdentity(); + $userWhoHitFilter = $this->getTestUser()->getUser(); $authority = new UltimateAuthority( $performer ); // Create a test filter where first revision is public, and the second two are protected. @@ -163,11 +175,11 @@ class SpecialAbuseFilterTest extends SpecialPageTestBase { $abuseFilterLoggerFactory = AbuseFilterServices::getAbuseLoggerFactory(); $abuseFilterLoggerFactory->newLogger( $this->getExistingTestPage()->getTitle(), - $this->getTestUser()->getUser(), + $userWhoHitFilter, VariableHolder::newFromArray( [ 'action' => 'edit', 'user_unnamed_ip' => '1.2.3.4', - 'user_name' => 'User1', + 'user_name' => $userWhoHitFilter->getName(), ] ) )->addLogEntries( [ 1 => [ 'warn' ] ] ); @@ -190,7 +202,7 @@ class SpecialAbuseFilterTest extends SpecialPageTestBase { // Create a testing recentchanges table row by creating a logging table row that is sent to recentchanges. $logEntry = new ManualLogEntry( 'move', 'move' ); - $logEntry->setPerformer( $this->getTestUser()->getUserIdentity() ); + $logEntry->setPerformer( $userWhoHitFilter ); $logEntry->setTarget( $this->getExistingTestPage()->getTitle() ); $logEntry->setComment( 'A very good reason' ); $logEntry->setParameters( [ @@ -209,6 +221,7 @@ class SpecialAbuseFilterTest extends SpecialPageTestBase { ->fetchField(); $this->assertNotFalse( $recentChangeId ); self::$recentChangeId = $recentChangeId; + self::$userWhoHitFilter = $userWhoHitFilter->getName(); } /** @@ -496,6 +509,60 @@ class SpecialAbuseFilterTest extends SpecialPageTestBase { $this->assertStringContainsString( '1.2.3.4', $html ); } + public function testViewTestBatchWhenSubmittedForProtectedFilter() { + $this->addCustomProtectedVariableToGenericVars(); + + // Assert that the user who can see protected variables can submit the form for a protected filter + // and that this submission causes protected variable access logs to be created + [ $html, ] = $this->executeSpecialPage( + 'test', + new FauxRequest( [ + 'wpFilterRules' => "custom_variable = 'custom_variable_value'", + 'wpTestAction' => 0, + 'wpTestUser' => '', + 'wpTestPeriodStart' => '', + 'wpTestPeriodEnd' => '', + 'wpTestPage' => '', + 'wpShowNegative' => 1, + ], true ), + null, + $this->authorityCanUseProtectedVar + ); + + $this->assertStringContainsString( 'custom_variable_value', $html ); + + // Verify that a protected variable access log was created as protected variable values were viewed. + $this->verifyProtectedVariableAccessLogExists( + $this->authorityCanUseProtectedVar->getUser(), self::$userWhoHitFilter, [ 'custom_variable' ] + ); + } + + private function verifyProtectedVariableAccessLogExists( + UserIdentity $performer, string $target, array $variablesViewed + ): void { + $result = $this->newSelectQueryBuilder() + ->select( 'log_params' ) + ->from( 'logging' ) + ->join( 'actor', null, 'actor_id=log_actor' ) + ->where( [ + 'log_action' => 'view-protected-var-value', + 'log_type' => ProtectedVarsAccessLogger::LOG_TYPE, + 'actor_name' => $performer->getName(), + 'log_title' => Title::newFromText( $target )->getDBkey(), + 'log_namespace' => NS_USER, + ] ) + ->caller( __METHOD__ ) + ->fetchResultSet(); + $this->assertSame( 1, $result->numRows() ); + $result->rewind(); + $this->assertArrayEquals( + [ 'variables' => $variablesViewed ], + LogEntryBase::extractParams( $result->fetchRow()['log_params'] ), + false, + true + ); + } + /** * Common test code used by tests which load the list of AbuseFilters, * used to verify that the headings on the table of AbuseFilters are @@ -995,25 +1062,9 @@ class SpecialAbuseFilterTest extends SpecialPageTestBase { $this->assertStringContainsString( '1.2.3.4', $abuseLogDetailsTableHtml ); // Verify that a protected variable access log was created as protected variable values were viewed. - $result = $this->newSelectQueryBuilder() - ->select( 'log_params' ) - ->from( 'logging' ) - ->where( [ - 'log_action' => 'view-protected-var-value', - 'log_type' => ProtectedVarsAccessLogger::LOG_TYPE, - ] ) - ->caller( __METHOD__ ) - ->fetchResultSet(); - $this->assertSame( 1, $result->numRows() ); - $result->rewind(); - $this->assertArrayEquals( - [ 'variables' => [ 'user_unnamed_ip' ] ], - LogEntryBase::extractParams( $result->fetchRow()['log_params'] ), - false, - true + $this->verifyProtectedVariableAccessLogExists( + $this->authorityCanUseProtectedVar->getUser(), self::$userWhoHitFilter, [ 'user_unnamed_ip' ] ); - - $this->dropProtectedVarAccessLogs(); } public function testViewExamineForRecentChangeWithMissingId() { @@ -1088,25 +1139,9 @@ class SpecialAbuseFilterTest extends SpecialPageTestBase { $this->assertStringContainsString( 'custom_variable_value', $customVariableTableRow ); // Verify that a protected variable access log was created as protected variable values were viewed. - $result = $this->newSelectQueryBuilder() - ->select( 'log_params' ) - ->from( 'logging' ) - ->where( [ - 'log_action' => 'view-protected-var-value', - 'log_type' => ProtectedVarsAccessLogger::LOG_TYPE, - ] ) - ->caller( __METHOD__ ) - ->fetchResultSet(); - $this->assertSame( 1, $result->numRows() ); - $result->rewind(); - $this->assertArrayEquals( - [ 'variables' => [ 'custom_variable' ] ], - LogEntryBase::extractParams( $result->fetchRow()['log_params'] ), - false, - true + $this->verifyProtectedVariableAccessLogExists( + $this->authorityCanUseProtectedVar->getUser(), static::$userWhoHitFilter, [ 'custom_variable' ] ); - - $this->dropProtectedVarAccessLogs(); } /** -- 2.25.1