From f18e4cf47e756c9700446190a3bb43055218ce49 Mon Sep 17 00:00:00 2001 From: Dreamy Jazz Date: Tue, 17 Jun 2025 14:11:32 +0100 Subject: [PATCH] SECURITY: Apply protected variable access restrictions on RC examine Why: * The AbuseFilter 'examine' view allows users to get all variables for a RecentChanges entry. * This does not currently check for protected variable access restrictions, so all protected variables are shown. ** This is okay for user_unnamed_ip because it's value is set to null for examining RecentChanges entries. ** However, protected variables provided by other extensions may not do that and so could expose the value to users who cannot see it. What: * Update AbuseFilterViewExamine::showExaminerForRC to have code similar to ::showExaminerForLog. There are some differences, specifically: ** Protected variables the user cannot see are removed from the examiner instead of showing an access restriction error because the user did not specifically request those variables. * Add tests to verify the changes. Bug: T396750 Change-Id: I549dd55bc9d89daa8646aa58f9c1c9486af79ec2 --- includes/View/AbuseFilterViewExamine.php | 35 +++++ .../Special/SpecialAbuseFilterTest.php | 139 +++++++++++++++++- 2 files changed, 173 insertions(+), 1 deletion(-) diff --git a/includes/View/AbuseFilterViewExamine.php b/includes/View/AbuseFilterViewExamine.php index 8cd0be2e..dd5c4056 100644 --- a/includes/View/AbuseFilterViewExamine.php +++ b/includes/View/AbuseFilterViewExamine.php @@ -240,6 +240,41 @@ class AbuseFilterViewExamine extends AbuseFilterView { return; } + $varsArray = $this->varManager->dumpAllVars( $vars, true ); + + // Filter out any protected variables that the user cannot see. Keep any protected variables that the user + // can see. This will unconditionally generate log entries when viewing the examiner that contains protected + // variables, as there is no way to specify the variables to select in the RecentChanges examiner. + $protectedVariableValuesShown = []; + foreach ( $this->afPermManager->getProtectedVariables() as $protectedVariable ) { + if ( array_key_exists( $protectedVariable, $varsArray ) ) { + // Try each variable at a time, as the user may be able to see some but not all of the + // protected variables. + $canViewProtectedVariable = $this->afPermManager + ->canViewProtectedVariables( $this->getAuthority(), [ $protectedVariable ] )->isGood(); + if ( !$canViewProtectedVariable ) { + // Remove protected variables the user cannot see because they didn't specifically ask for them. + unset( $varsArray[$protectedVariable] ); + } else { + // Only log if there was a value set. This is to be consistent with + // self::showExaminerForLogEntry + if ( $varsArray[$protectedVariable] !== null ) { + $protectedVariableValuesShown[] = $protectedVariable; + } + } + } + } + $vars = VariableHolder::newFromArray( $varsArray ); + + if ( count( $protectedVariableValuesShown ) ) { + $logger = $this->abuseLoggerFactory->getProtectedVarsAccessLogger(); + $logger->logViewProtectedVariableValue( + $this->getUser(), + $varsArray['user_name'] ?? $varsArray['accountname'], + $protectedVariableValuesShown + ); + } + $out->addJsConfigVars( [ 'abuseFilterExamine' => [ 'type' => 'rc', 'id' => $rcid ] ] ); diff --git a/tests/phpunit/integration/Special/SpecialAbuseFilterTest.php b/tests/phpunit/integration/Special/SpecialAbuseFilterTest.php index e154d402..f4fb2cc2 100644 --- a/tests/phpunit/integration/Special/SpecialAbuseFilterTest.php +++ b/tests/phpunit/integration/Special/SpecialAbuseFilterTest.php @@ -28,6 +28,7 @@ use MediaWiki\Extension\AbuseFilter\View\AbuseFilterViewTools; use MediaWiki\Html\Html; use MediaWiki\Language\RawMessage; use MediaWiki\Logging\LogEntryBase; +use MediaWiki\Logging\ManualLogEntry; use MediaWiki\MediaWikiServices; use MediaWiki\Permissions\Authority; use MediaWiki\Permissions\UltimateAuthority; @@ -59,8 +60,8 @@ class SpecialAbuseFilterTest extends SpecialPageTestBase { use FilterFromSpecsTestTrait; private Authority $authorityCannotUseProtectedVar; - private Authority $authorityCanUseProtectedVar; + private static int $recentChangeId; protected function setUp(): void { parent::setUp(); @@ -186,6 +187,28 @@ class SpecialAbuseFilterTest extends SpecialPageTestBase { ->table( 'abuse_filter_log' ) ->caller( __METHOD__ ) ->assertFieldValue( 1 ); + + // 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->setTarget( $this->getExistingTestPage()->getTitle() ); + $logEntry->setComment( 'A very good reason' ); + $logEntry->setParameters( [ + '4::target' => wfRandomString(), + '5::noredir' => '0' + ] ); + $logId = $logEntry->insert(); + $logEntry->publish( $logId ); + + // Check that the recentchanges row for the log entry exists and get the ID for it. + $recentChangeId = $this->newSelectQueryBuilder() + ->select( 'rc_id' ) + ->from( 'recentchanges' ) + ->where( [ 'rc_logid' => $logId ] ) + ->caller( __METHOD__ ) + ->fetchField(); + $this->assertNotFalse( $recentChangeId ); + self::$recentChangeId = $recentChangeId; } /** @@ -989,5 +1012,119 @@ class SpecialAbuseFilterTest extends SpecialPageTestBase { false, true ); + + $this->dropProtectedVarAccessLogs(); + } + + public function testViewExamineForRecentChangeWithMissingId() { + [ $html, ] = $this->executeSpecialPage( + 'examine/1234', + new FauxRequest(), + null, + $this->authorityCannotUseProtectedVar + ); + + $this->verifyHasExamineIntroMessage( $html ); + $this->assertStringContainsString( + '(abusefilter-examine-notfound)', + $html, + 'Missing error message for unknown AbuseLog ID.' + ); + } + + private function addCustomProtectedVariableToGenericVars() { + $this->setTemporaryHook( 'AbuseFilterCustomProtectedVariables', static function ( &$variables ) { + $variables[] = 'custom_variable'; + } ); + $this->setTemporaryHook( 'AbuseFilter-builder', static function ( array &$realValues ) { + $realValues['vars']['custom_variable'] = 'custom-variable-test'; + } ); + $this->setTemporaryHook( 'AbuseFilter-generateGenericVars', static function ( VariableHolder $vars ) { + $vars->setVar( 'custom_variable', 'custom_variable_value' ); + } ); + $this->resetServices(); + } + + public function testViewExamineForRecentChangeWhereUserCannotSeeSpecificProtectedVariableDueToPermission() { + // Mock that all users lack access to the 'custom_variable' variable due to it being a protected variable. + $this->addCustomProtectedVariableToGenericVars(); + $this->setTemporaryHook( + 'AbuseFilterCanViewProtectedVariables', + static function ( Authority $performer, array $variables, AbuseFilterPermissionStatus $returnStatus ) { + if ( in_array( 'custom_variable', $variables ) ) { + $returnStatus->setPermission( 'test-permission' ); + } + } + ); + + [ $html, ] = $this->executeSpecialPage( + 'examine/' . self::$recentChangeId, null, null, $this->authorityCanUseProtectedVar + ); + + $this->verifyHasExamineIntroMessage( $html ); + $this->assertStringNotContainsString( + 'mw-abuselog-details-custom_variable', + $html, + 'The "custom_variable" variable was not unset, but it should ' . + 'have been because the user cannot see it.' + ); + } + + public function testViewExamineForRecentChangeWhenUserCanSeeRecentChange() { + $this->addCustomProtectedVariableToGenericVars(); + [ $html, ] = $this->executeSpecialPage( + 'examine/' . self::$recentChangeId, null, null, $this->authorityCanUseProtectedVar + ); + DeferredUpdates::doUpdates(); + + $this->verifyHasExamineIntroMessage( $html ); + + // Check that the test tools elements are loaded + $this->assertStringContainsString( '(abusefilter-examine-test', $html ); + $this->assertStringContainsString( '(abusefilter-examine-test-button', $html ); + + // Verify that the custom_variable variable is shown with it's value. + $customVariableTableRow = $this->assertAndGetByElementClass( $html, 'mw-abuselog-details-custom_variable' ); + $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->dropProtectedVarAccessLogs(); + } + + /** + * Drops the 'view-protected-var-value' logs from the 'logging' table. + * + * This is needed because in {@link self::addDBDataOnce} we added rows to the 'logging' table and so the table is + * not reset between tests. + * + * @return void + */ + private function dropProtectedVarAccessLogs(): void { + $this->getDb()->newDeleteQueryBuilder() + ->deleteFrom( 'logging' ) + ->where( [ + 'log_action' => 'view-protected-var-value', + 'log_type' => ProtectedVarsAccessLogger::LOG_TYPE, + ] ) + ->caller( __METHOD__ ) + ->execute(); } } -- 2.25.1