From 3e24b9c2fa61dc33ac3149ab32e72430c81cba8c Mon Sep 17 00:00:00 2001 From: Lucas Werkmeister Date: Wed, 30 Aug 2023 12:54:36 +0200 Subject: [PATCH] SECURITY: Add rate limits and edit filters to item merging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EntityRedirectCreationInteractor was missing rate limiting, and ItemMergeInteractor was missing that plus edit filter running (e.g. AbuseFilter). Add both. The way that errors are reported isn’t great, but I don’t want to do the larger refactoring that would be required to do it better in a security patch. (But once this is published on Gerrit, I think I’d like to change these classes to return a Status instead of throwing exceptions.) Bug: T345064 Change-Id: Ia2a4a248f891282f2aa03afd34f4643b517b8ef7 --- repo/WikibaseRepo.ServiceWiring.php | 3 +- .../EntityRedirectCreationInteractor.php | 9 +++ .../Interactors/ItemMergeInteractor.php | 39 ++++++++++--- .../phpunit/includes/Api/MergeItemsTest.php | 3 +- .../Interactors/ItemMergeInteractorTest.php | 56 ++++++++++++++++--- .../Specials/SpecialMergeItemsTest.php | 6 +- .../ServiceWiring/ItemMergeInteractorTest.php | 3 + 7 files changed, 100 insertions(+), 19 deletions(-) diff --git a/repo/WikibaseRepo.ServiceWiring.php b/repo/WikibaseRepo.ServiceWiring.php index da5abd67e1..99b78a2e32 100644 --- a/repo/WikibaseRepo.ServiceWiring.php +++ b/repo/WikibaseRepo.ServiceWiring.php @@ -1294,7 +1294,8 @@ function ( EntityNamespaceLookup $nsLookup, DatabaseEntitySource $source ): Enti WikibaseRepo::getSummaryFormatter( $services ), WikibaseRepo::getItemRedirectCreationInteractor( $services ), WikibaseRepo::getEntityTitleStoreLookup( $services ), - $services->getPermissionManager() + $services->getPermissionManager(), + WikibaseRepo::getEditFilterHookRunner( $services ) ); }, diff --git a/repo/includes/Interactors/EntityRedirectCreationInteractor.php b/repo/includes/Interactors/EntityRedirectCreationInteractor.php index 35d05d30cf..76086111db 100644 --- a/repo/includes/Interactors/EntityRedirectCreationInteractor.php +++ b/repo/includes/Interactors/EntityRedirectCreationInteractor.php @@ -108,6 +108,7 @@ public function createRedirect( ): EntityRedirect { $this->checkCompatible( $fromId, $toId ); $this->checkPermissions( $fromId, $context ); + $this->checkRateLimits( $context ); $this->checkExistsNoRedirect( $toId ); $this->checkCanCreateRedirect( $fromId ); @@ -143,6 +144,14 @@ private function checkPermissions( EntityId $entityId, IContextSource $context ) } } + private function checkRateLimits( IContextSource $context ): void { + if ( $context->getUser()->pingLimiter( 'edit' ) ) { + // use generic 'failed-save' because RedirectCreationException prepends 'wikibase-redirect-' for the message key, + // so we can’t easily use the correct actionthrottledtext message (using Status would solve this) + throw new RedirectCreationException( 'rate limit hit', 'permissiondenied' ); + } + } + /** * @param EntityId $entityId * diff --git a/repo/includes/Interactors/ItemMergeInteractor.php b/repo/includes/Interactors/ItemMergeInteractor.php index 439b994a8e..0835f7afb9 100644 --- a/repo/includes/Interactors/ItemMergeInteractor.php +++ b/repo/includes/Interactors/ItemMergeInteractor.php @@ -20,6 +20,7 @@ use Wikibase\Repo\ChangeOp\ChangeOpException; use Wikibase\Repo\ChangeOp\ChangeOpsMerge; use Wikibase\Repo\Content\EntityContent; +use Wikibase\Repo\EditEntity\EditFilterHookRunner; use Wikibase\Repo\Merge\MergeFactory; use Wikibase\Repo\Store\EntityPermissionChecker; use Wikibase\Repo\Store\EntityTitleStoreLookup; @@ -73,6 +74,8 @@ class ItemMergeInteractor { */ private $permissionManager; + private EditFilterHookRunner $editFilterHookRunner; + public function __construct( MergeFactory $mergeFactory, EntityRevisionLookup $entityRevisionLookup, @@ -81,7 +84,8 @@ public function __construct( SummaryFormatter $summaryFormatter, ItemRedirectCreationInteractor $interactorRedirect, EntityTitleStoreLookup $entityTitleLookup, - PermissionManager $permissionManager + PermissionManager $permissionManager, + EditFilterHookRunner $editFilterHookRunner ) { $this->mergeFactory = $mergeFactory; $this->entityRevisionLookup = $entityRevisionLookup; @@ -91,6 +95,7 @@ public function __construct( $this->interactorRedirect = $interactorRedirect; $this->entityTitleLookup = $entityTitleLookup; $this->permissionManager = $permissionManager; + $this->editFilterHookRunner = $editFilterHookRunner; } /** @@ -114,6 +119,14 @@ private function checkPermissions( EntityId $entityId, User $user ) { } } + private function checkRateLimits( User $user ): void { + if ( $user->pingLimiter( 'edit', 2 ) ) { // attemptSaveMerge() makes two edits + // use generic 'failed-save' because ItemMergeException prepends 'wikibase-itemmerge-' for the message key, + // so we can’t easily use the correct actionthrottledtext message (using Status would solve this) + throw new ItemMergeException( 'rate limit hit', 'failed-save' ); + } + } + /** * Merges the content of the first item into the second and creates a redirect if the first item * is empty after the merge. @@ -149,6 +162,7 @@ public function mergeItems( $user = $context->getUser(); $this->checkPermissions( $fromId, $user ); $this->checkPermissions( $toId, $user ); + $this->checkRateLimits( $user ); /** * @var Item $fromItem @@ -174,7 +188,7 @@ public function mergeItems( throw new ItemMergeException( $e->getMessage(), 'failed-modify', $e ); } - $result = $this->attemptSaveMerge( $fromItem, $toItem, $summary, $user, $bot, $tags ); + $result = $this->attemptSaveMerge( $fromItem, $toItem, $summary, $context, $bot, $tags ); $this->updateWatchlistEntries( $fromId, $toId ); $redirected = false; @@ -261,23 +275,26 @@ private function getSummary( $direction, ItemId $getId, $customSummary = null ) * @param Item $fromItem * @param Item $toItem * @param string|null $summary + * @param IContextSource $context * @param bool $bot * @param string[] $tags * * @return array A list of exactly two EntityRevision objects. The first one represents the * modified source item, the second one represents the modified target item. */ - private function attemptSaveMerge( Item $fromItem, Item $toItem, ?string $summary, User $user, bool $bot, array $tags ) { + private function attemptSaveMerge( Item $fromItem, Item $toItem, ?string $summary, IContextSource $context, bool $bot, array $tags ) { $toSummary = $this->getSummary( 'to', $toItem->getId(), $summary ); - $fromRev = $this->saveItem( $fromItem, $toSummary, $user, $bot, $tags ); + $fromRev = $this->saveItem( $fromItem, $toSummary, $context, $bot, $tags ); $fromSummary = $this->getSummary( 'from', $fromItem->getId(), $summary ); - $toRev = $this->saveItem( $toItem, $fromSummary, $user, $bot, $tags ); + $toRev = $this->saveItem( $toItem, $fromSummary, $context, $bot, $tags ); return [ $fromRev, $toRev ]; } - private function saveItem( Item $item, FormatableSummary $summary, User $user, bool $bot, array $tags ) { + private function saveItem( Item $item, FormatableSummary $summary, IContextSource $context, bool $bot, array $tags ) { + $user = $context->getUser(); + // Given we already check all constraints in ChangeOpsMerge, it's // fine to ignore them here. This is also needed to not run into // the constraints we're supposed to ignore (see ChangeOpsMerge::removeConflictsWithEntity @@ -287,10 +304,18 @@ private function saveItem( Item $item, FormatableSummary $summary, User $user, b $flags |= EDIT_FORCE_BOT; } + $formattedSummary = $this->summaryFormatter->formatSummary( $summary ); + + $status = $this->editFilterHookRunner->run( $item, $context, $formattedSummary ); + if ( !$status->isOK() ) { + // as in checkPermissions() above, it would be better to just pass the Status to the API + throw new ItemMergeException( $status->getWikiText(), 'failed-save' ); + } + try { return $this->entityStore->saveEntity( $item, - $this->summaryFormatter->formatSummary( $summary ), + $formattedSummary, $user, $flags, false, diff --git a/repo/tests/phpunit/includes/Api/MergeItemsTest.php b/repo/tests/phpunit/includes/Api/MergeItemsTest.php index 93b56cf01a..7126a6f69d 100644 --- a/repo/tests/phpunit/includes/Api/MergeItemsTest.php +++ b/repo/tests/phpunit/includes/Api/MergeItemsTest.php @@ -197,7 +197,8 @@ private function newMergeItemsApiModule( array $params, EntityRedirect $expected WikibaseRepo::getSummaryFormatter(), $this->getMockRedirectCreationInteractor( $expectedRedirect ), $this->getEntityTitleStoreLookup(), - MediaWikiServices::getInstance()->getPermissionManager() + MediaWikiServices::getInstance()->getPermissionManager(), + WikibaseRepo::getEditFilterHookRunner() ), $errorReporter, function ( $module ) use ( $apiResultBuilder ) { diff --git a/repo/tests/phpunit/includes/Interactors/ItemMergeInteractorTest.php b/repo/tests/phpunit/includes/Interactors/ItemMergeInteractorTest.php index d07c0c2d39..1a6d641635 100644 --- a/repo/tests/phpunit/includes/Interactors/ItemMergeInteractorTest.php +++ b/repo/tests/phpunit/includes/Interactors/ItemMergeInteractorTest.php @@ -3,7 +3,9 @@ namespace Wikibase\Repo\Tests\Interactors; use HashSiteStore; +use IContextSource; use MediaWiki\MediaWikiServices; +use MediaWiki\Permissions\RateLimiter; use MediaWiki\Title\Title; use MediaWikiIntegrationTestCase; use RequestContext; @@ -40,6 +42,12 @@ */ class ItemMergeInteractorTest extends MediaWikiIntegrationTestCase { + /** @var string User name that gets blocked by the permission checker. */ + private const USER_NAME_WITHOUT_PERMISSION = 'UserWithoutPermission'; + + /** @var string User name that gets blocked by the edit filter. */ + private const USER_NAME_WITH_EDIT_FILTER = 'UserWithEditFilter'; + /** * @var MockRepository|null */ @@ -79,7 +87,13 @@ public function getMockEditFilterHookRunner() { ->disableOriginalConstructor() ->getMock(); $mock->method( 'run' ) - ->willReturn( Status::newGood() ); + ->willReturnCallback( function ( $new, IContextSource $context, string $summary ) { + if ( $context->getUser()->getName() === self::USER_NAME_WITH_EDIT_FILTER ) { + return Status::newFatal( 'permissiondenied' ); + } else { + return Status::newGood(); + } + } ); return $mock; } @@ -92,9 +106,7 @@ private function getPermissionChecker() { $permissionChecker->method( 'getPermissionStatusForEntityId' ) ->willReturnCallback( function( User $user ) { - $userWithoutPermissionName = 'UserWithoutPermission'; - - if ( $user->getName() === $userWithoutPermissionName ) { + if ( $user->getName() === self::USER_NAME_WITHOUT_PERMISSION ) { return Status::newFatal( 'permissiondenied' ); } else { return Status::newGood(); @@ -147,6 +159,7 @@ private function newInteractor() { new HashSiteStore( TestSites::getSites() ) ); + $editFilterHookRunner = $this->getMockEditFilterHookRunner(); $interactor = new ItemMergeInteractor( $mergeFactory, $this->mockRepository, @@ -158,12 +171,13 @@ private function newInteractor() { $this->mockRepository, $this->getPermissionChecker(), $summaryFormatter, - $this->getMockEditFilterHookRunner(), + $editFilterHookRunner, $this->mockRepository, $this->getMockEntityTitleLookup() ), $this->getEntityTitleLookup(), - MediaWikiServices::getInstance()->getPermissionManager() + MediaWikiServices::getInstance()->getPermissionManager(), + $editFilterHookRunner ); return $interactor; @@ -457,10 +471,10 @@ public function testMergeItems_conflict( $fromData, $toData, $ignoreConflicts ) } } - public function testSetRedirect_noPermission() { + public function testMergeItems_noPermission() { $this->expectException( ItemMergeException::class ); - $user = User::newFromName( 'UserWithoutPermission' ); + $user = User::newFromName( self::USER_NAME_WITHOUT_PERMISSION ); $fromId = new ItemId( 'Q1' ); $toId = new ItemId( 'Q2' ); @@ -469,6 +483,32 @@ public function testSetRedirect_noPermission() { $interactor->mergeItems( $fromId, $toId, $this->getContext( $user ) ); } + public function testMergeItems_editFilter(): void { + $user = $this->getServiceContainer()->getUserFactory() + ->newFromName( self::USER_NAME_WITH_EDIT_FILTER ); + $fromId = new ItemId( 'Q1' ); + $toId = new ItemId( 'Q2' ); + $interactor = $this->newInteractor(); + + $this->expectException( ItemMergeException::class ); + $interactor->mergeItems( $fromId, $toId, $this->getContext( $user ) ); + } + + public function testMergeItems_rateLimit(): void { + $rateLimiter = $this->createConfiguredMock( RateLimiter::class, [ + 'isLimitable' => true, + 'limit' => true, // limit was exceeded + ] ); + $this->setService( 'RateLimiter', $rateLimiter ); + + $fromId = new ItemId( 'Q1' ); + $toId = new ItemId( 'Q2' ); + $interactor = $this->newInteractor(); + + $this->expectException( ItemMergeException::class ); + $interactor->mergeItems( $fromId, $toId, $this->getContext() ); + } + private function extractConcreteRevisionId( LatestRevisionIdResult $result ) { $shouldNotBeCalled = function () { $this->fail( 'Not a concrete revision result given' ); diff --git a/repo/tests/phpunit/includes/Specials/SpecialMergeItemsTest.php b/repo/tests/phpunit/includes/Specials/SpecialMergeItemsTest.php index ed6d0efe93..449e58b78e 100644 --- a/repo/tests/phpunit/includes/Specials/SpecialMergeItemsTest.php +++ b/repo/tests/phpunit/includes/Specials/SpecialMergeItemsTest.php @@ -145,6 +145,7 @@ protected function newSpecialPage() { } ); $titleLookup = $this->getEntityTitleLookup(); + $editFilterHookRunner = $this->getMockEditFilterHookRunner(); $specialPage = new SpecialMergeItems( WikibaseRepo::getEntityIdParser(), $titleLookup, @@ -160,12 +161,13 @@ protected function newSpecialPage() { $this->mockRepository, $this->getPermissionCheckers(), $summaryFormatter, - $this->getMockEditFilterHookRunner(), + $editFilterHookRunner, $this->mockRepository, $this->getMockEntityTitleLookup() ), $titleLookup, - MediaWikiServices::getInstance()->getPermissionManager() + MediaWikiServices::getInstance()->getPermissionManager(), + $editFilterHookRunner ), false, WikibaseRepo::getTokenCheckInteractor() diff --git a/repo/tests/phpunit/unit/ServiceWiring/ItemMergeInteractorTest.php b/repo/tests/phpunit/unit/ServiceWiring/ItemMergeInteractorTest.php index 2ecfbb2e19..e8f6a27450 100644 --- a/repo/tests/phpunit/unit/ServiceWiring/ItemMergeInteractorTest.php +++ b/repo/tests/phpunit/unit/ServiceWiring/ItemMergeInteractorTest.php @@ -6,6 +6,7 @@ use Wikibase\Lib\Store\EntityRevisionLookup; use Wikibase\Lib\Store\EntityStore; use Wikibase\Repo\ChangeOp\ChangeOpFactoryProvider; +use Wikibase\Repo\EditEntity\EditFilterHookRunner; use Wikibase\Repo\Interactors\ItemMergeInteractor; use Wikibase\Repo\Interactors\ItemRedirectCreationInteractor; use Wikibase\Repo\Merge\MergeFactory; @@ -40,6 +41,8 @@ public function testConstruction(): void { => $this->createMock( ItemRedirectCreationInteractor::class ), 'WikibaseRepo.EntityTitleStoreLookup' => $this->createMock( EntityTitleStoreLookup::class ), + 'WikibaseRepo.EditFilterHookRunner' + => $this->createMock( EditFilterHookRunner::class ), ] ); $this->serviceContainer -- 2.39.2