From aa173942c0bd723267475e4e16d54988450c8ecb Mon Sep 17 00:00:00 2001 From: Lucas Werkmeister Date: Wed, 14 Jun 2023 12:11:15 +0200 Subject: [PATCH] SECURITY: Run edit filters on submit (undo/restore) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SubmitEntityAction, which implements the “undo” and “restore” actions, doesn’t use the EditEntity interface (because SubmitEntityAction works with redirects but EditEntity doesn’t support them), so we need to call the EditFilterHookRunner ourselves to interact with e.g. AbuseFilter. The EditFilterHookRunner interface is extended to also allow passing in an EntityContent, so we don’t waste time (and complicate the code) by having SubmitEntityAction extract an EntityDocument or EntityRedirect from the EntityContent only for the hook runner to construct the content again. Note that StatsdTimeRecordingEditFilterHookRunner didn’t even properly support EntityRedirect yet (which doesn’t have a getType() method; the only reason this didn’t blow up before is that the StatsdTimeRecordingEditFilterHookRunner is only created and used by MediawikiEditEntityFactory, but the EditFilterHookRunner service just uses MediawikiEditFilterHookRunner directly), so fix that at the same time to avoid problems later. In the tests, to set up an edit filter that runs against all edits but only blocks the one test case we add here, use the “Oslo” item handle that doesn’t seem to have been used in any other tests as far as I can tell. To use it with “undo” functionality, that test item now needs a second revision added in ActionTestCase::makeTestItemData(). Bug: T250720 Change-Id: Id0260598b41c5c4683a446f478ac3dd84608b8fd --- repo/includes/Actions/SubmitEntityAction.php | 19 +++++---- .../EditEntity/EditFilterHookRunner.php | 3 +- .../MediaWikiEditFilterHookRunner.php | 15 ++++++- ...tatsdTimeRecordingEditFilterHookRunner.php | 22 ++++++++--- .../includes/Actions/ActionTestCase.php | 4 ++ .../includes/Actions/EditEntityActionTest.php | 39 +++++++++++++++++++ 6 files changed, 87 insertions(+), 15 deletions(-) diff --git a/repo/includes/Actions/SubmitEntityAction.php b/repo/includes/Actions/SubmitEntityAction.php index 92452f46655..0dacb7fa75c 100644 --- a/repo/includes/Actions/SubmitEntityAction.php +++ b/repo/includes/Actions/SubmitEntityAction.php @@ -3,7 +3,6 @@ namespace Wikibase\Repo\Actions; use Article; -use Content; use IContextSource; use LogicException; use MediaWiki\MediaWikiServices; @@ -14,6 +13,7 @@ use Title; use Wikibase\Lib\Summary; use Wikibase\Repo\Content\EntityContent; +use Wikibase\Repo\EditEntity\EditFilterHookRunner; use Wikibase\Repo\SummaryFormatter; use Wikibase\Repo\WikibaseRepo; @@ -29,10 +29,8 @@ */ class SubmitEntityAction extends EditEntityAction { - /** - * @var SummaryFormatter - */ - private $summaryFormatter; + private EditFilterHookRunner $editFilterHookRunner; + private SummaryFormatter $summaryFormatter; /** * @see EditEntityAction::__construct @@ -43,6 +41,7 @@ class SubmitEntityAction extends EditEntityAction { public function __construct( Article $article, IContextSource $context ) { parent::__construct( $article, $context ); + $this->editFilterHookRunner = WikibaseRepo::getEditFilterHookRunner(); $this->summaryFormatter = WikibaseRepo::getSummaryFormatter(); } @@ -213,7 +212,7 @@ public function execute() { /** * @param Title $title - * @param Content $content + * @param EntityContent $content * @param string $summary * @param int $undidRevId * @param int $originalRevId @@ -222,7 +221,7 @@ public function execute() { * @return Status */ private function attemptSave( - Title $title, Content $content, $summary, $undidRevId, $originalRevId, $editToken + Title $title, EntityContent $content, $summary, $undidRevId, $originalRevId, $editToken ) { $status = $this->getEditTokenStatus( $editToken ); @@ -236,6 +235,12 @@ private function attemptSave( return $status; } + $status = $this->editFilterHookRunner->run( $content, $this->getContext(), $summary ); + + if ( !$status->isOK() ) { + return $status; + } + // save edit $page = MediaWikiServices::getInstance()->getWikiPageFactory() ->newFromTitle( $title ); diff --git a/repo/includes/EditEntity/EditFilterHookRunner.php b/repo/includes/EditEntity/EditFilterHookRunner.php index 46967c9500b..b919ef1b42c 100644 --- a/repo/includes/EditEntity/EditFilterHookRunner.php +++ b/repo/includes/EditEntity/EditFilterHookRunner.php @@ -8,6 +8,7 @@ use Status; use Wikibase\DataModel\Entity\EntityDocument; use Wikibase\DataModel\Entity\EntityRedirect; +use Wikibase\Repo\Content\EntityContent; /** * Interface to run a hook before and edit is saved. @@ -20,7 +21,7 @@ interface EditFilterHookRunner { /** * Call EditFilterMergedContent hook, if registered. * - * @param EntityDocument|EntityRedirect|null $new The entity or redirect we are trying to save + * @param EntityDocument|EntityRedirect|EntityContent|null $new The entity or redirect (content) we are trying to save * @param IContextSource $context The request context for the edit * @param string $summary The edit summary * diff --git a/repo/includes/EditEntity/MediaWikiEditFilterHookRunner.php b/repo/includes/EditEntity/MediaWikiEditFilterHookRunner.php index 192eb40d0dd..58d1393cde5 100644 --- a/repo/includes/EditEntity/MediaWikiEditFilterHookRunner.php +++ b/repo/includes/EditEntity/MediaWikiEditFilterHookRunner.php @@ -13,6 +13,7 @@ use Wikibase\DataModel\Entity\EntityId; use Wikibase\DataModel\Entity\EntityRedirect; use Wikibase\Lib\Store\EntityNamespaceLookup; +use Wikibase\Repo\Content\EntityContent; use Wikibase\Repo\Content\EntityContentFactory; use Wikibase\Repo\Store\EntityTitleStoreLookup; @@ -59,7 +60,7 @@ public function __construct( /** * Call EditFilterMergedContent hook, if registered. * - * @param EntityDocument|EntityRedirect|null $new The entity or redirect we are trying to save + * @param EntityDocument|EntityRedirect|EntityContent|null $new The entity or redirect (content) we are trying to save * @param IContextSource $context The request context for the edit * @param string $summary The edit summary * @@ -95,13 +96,23 @@ public function run( $new, IContextSource $context, string $summary ) { $entityId = $new->getEntityId(); $entityType = $entityId->getEntityType(); + $context = $this->getContextForEditFilter( + $context, + $entityId, + $entityType + ); + } elseif ( $new instanceof EntityContent ) { + $entityContent = $new; + $entityId = $entityContent->getEntityId(); + $entityType = $entityId->getEntityType(); + $context = $this->getContextForEditFilter( $context, $entityId, $entityType ); } else { - throw new InvalidArgumentException( '$new must be instance of EntityDocument or EntityRedirect' ); + throw new InvalidArgumentException( '$new must be instance of EntityDocument, EntityRedirect or EntityContent' ); } $slotRole = $this->namespaceLookup->getEntitySlotRole( $entityType ); diff --git a/repo/includes/EditEntity/StatsdTimeRecordingEditFilterHookRunner.php b/repo/includes/EditEntity/StatsdTimeRecordingEditFilterHookRunner.php index 42ed6daddc5..9040c5e5ed0 100644 --- a/repo/includes/EditEntity/StatsdTimeRecordingEditFilterHookRunner.php +++ b/repo/includes/EditEntity/StatsdTimeRecordingEditFilterHookRunner.php @@ -7,6 +7,7 @@ use Status; use Wikibase\DataModel\Entity\EntityDocument; use Wikibase\DataModel\Entity\EntityRedirect; +use Wikibase\Repo\Content\EntityContent; /** * EditFilterHookRunning that collects stats for edits. @@ -37,7 +38,7 @@ public function __construct( } /** - * @param null|EntityDocument|EntityRedirect $new + * @param null|EntityDocument|EntityRedirect|EntityContent $new * @param IContextSource $context * @param string $summary * @return Status @@ -47,10 +48,21 @@ public function run( $new, IContextSource $context, $summary ) { $hookStatus = $this->hookRunner->run( $new, $context, $summary ); $attemptSaveFilterEnd = microtime( true ); - $this->stats->timing( - "{$this->timingPrefix}.run.{$new->getType()}", - ( $attemptSaveFilterEnd - $attemptSaveFilterStart ) * 1000 - ); + if ( $new !== null ) { + if ( $new instanceof EntityDocument ) { + $entityType = $new->getType(); + } elseif ( $new instanceof EntityRedirect ) { + $entityType = $new->getEntityId()->getEntityType(); + } elseif ( $new instanceof EntityContent ) { + $entityType = $new->getEntityId()->getEntityType(); + } else { + $entityType = 'UNKNOWN'; + } + $this->stats->timing( + "{$this->timingPrefix}.run.{$entityType}", + ( $attemptSaveFilterEnd - $attemptSaveFilterStart ) * 1000 + ); + } return $hookStatus; } diff --git a/repo/tests/phpunit/includes/Actions/ActionTestCase.php b/repo/tests/phpunit/includes/Actions/ActionTestCase.php index b897828d87a..41a305962c3 100644 --- a/repo/tests/phpunit/includes/Actions/ActionTestCase.php +++ b/repo/tests/phpunit/includes/Actions/ActionTestCase.php @@ -91,6 +91,10 @@ private function makeTestItemData() { $item->setLabel( 'de', 'London' ); $items['London'][] = $item; + $item = new Item(); + $item->setLabel( 'en', 'Oslo' ); + $items['Oslo'][] = $item; + $item = new Item(); $item->setLabel( 'de', 'Oslo' ); $item->setLabel( 'en', 'Oslo' ); diff --git a/repo/tests/phpunit/includes/Actions/EditEntityActionTest.php b/repo/tests/phpunit/includes/Actions/EditEntityActionTest.php index 453d7e29869..e904707f256 100644 --- a/repo/tests/phpunit/includes/Actions/EditEntityActionTest.php +++ b/repo/tests/phpunit/includes/Actions/EditEntityActionTest.php @@ -2,12 +2,16 @@ namespace Wikibase\Repo\Tests\Actions; +use IContextSource; use MediaWiki\MediaWikiServices; use RuntimeException; +use Status; use Title; use User; +use Wikibase\DataModel\Term\LabelsProvider; use Wikibase\Repo\Actions\EditEntityAction; use Wikibase\Repo\Actions\SubmitEntityAction; +use Wikibase\Repo\Content\EntityContent; use Wikibase\Repo\WikibaseRepo; use WikiPage; @@ -32,6 +36,27 @@ protected function setUp(): void { // Remove handlers for the "OutputPageParserOutput" hook $this->clearHook( 'OutputPageParserOutput' ); + + // Install EditFilterMergedContent hook blocking any edits to "Oslo"@en + $method = __METHOD__; + $this->setTemporaryHook( + 'EditFilterMergedContent', + function ( IContextSource $context, EntityContent $entityContent, Status $status ) use ( $method ) { + if ( !$entityContent->isRedirect() ) { + $entity = $entityContent->getEntity(); + if ( $entity instanceof LabelsProvider ) { + $labels = $entity->getLabels(); + if ( $labels->hasTermForLanguage( 'en' ) + && $labels->getByLanguage( 'en' )->getText() === 'Oslo' + ) { + $status->fatal( $method . ': block "Oslo"@en edit' ); + return false; + } + } + } + return true; + } + ); } public function testActionForPage() { @@ -845,6 +870,20 @@ public function provideUndoSubmit() { null, // user '/wikibase-undo-redirect-latestnoredirect/', // htmlPattern: should contain error ]; + + // blocked by edit filter (e.g. AbuseFilter, T250720), see setUp() + yield 'edit blocked by edit filter' => [ + 'submit', // action + 'Oslo', // handle + [ // params + 'wpSave' => 1, + 'wpEditToken' => true, // automatic token + 'undo' => 0, // current revision + ], + true, // post + null, // user + '/block "Oslo"@en edit/', // htmlPattern: should contain filter error message + ]; } /**