Page MenuHomePhabricator
Files F37103539

0001-SECURITY-Run-edit-filters-on-submit-undo-restore.patch
Lucas_Werkmeister_WMDE (Lucas Werkmeister (WMDE))
Actions

Authored By
Lucas_Werkmeister_WMDE
Jun 14 2023, 2:39 PM
Size
11 KB
Referenced Files
None
Subscribers
None

0001-SECURITY-Run-edit-filters-on-submit-undo-restore.patch
View Options

From 1ee86bfe2a0edcadee4b7f324a8bcf4a0078516d Mon Sep 17 00:00:00 2001
From: Lucas Werkmeister <lucas.werkmeister@wikimedia.de>
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 | 23 ++++++++---
.../includes/Actions/ActionTestCase.php | 4 ++
.../includes/Actions/EditEntityActionTest.php | 39 +++++++++++++++++++
6 files changed, 88 insertions(+), 15 deletions(-)
diff --git a/repo/includes/Actions/SubmitEntityAction.php b/repo/includes/Actions/SubmitEntityAction.php
index 93fc60f7a0..cc04e57688 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 MediaWiki\MediaWikiServices;
use MediaWiki\Permissions\PermissionManager;
@@ -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();
}
@@ -216,7 +215,7 @@ public function execute() {
/**
* @param Title $title
- * @param Content $content
+ * @param EntityContent $content
* @param string $summary
* @param int $undidRevId
* @param int $originalRevId
@@ -225,7 +224,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 );
@@ -239,6 +238,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 46967c9500..b919ef1b42 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 84309fd646..127beee022 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 42ed6daddc..3ed14bc8a6 100644
--- a/repo/includes/EditEntity/StatsdTimeRecordingEditFilterHookRunner.php
+++ b/repo/includes/EditEntity/StatsdTimeRecordingEditFilterHookRunner.php
@@ -3,10 +3,12 @@
namespace Wikibase\Repo\EditEntity;
use IContextSource;
+use InvalidArgumentException;
use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface;
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 +39,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 +49,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 e3966665c3..6a451c69fc 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 518d0998a0..f63cd1e5f4 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 MWException;
+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
+ ];
}
/**
--
2.39.2

File Metadata

Mime Type
text/x-diff
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
10897522
Default Alt Text
0001-SECURITY-Run-edit-filters-on-submit-undo-restore.patch (11 KB)

Event Timeline

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits