Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F37121027
T250720-rev3.patch
sbassett (Scott Bassett)
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Authored By
sbassett
Jun 27 2023, 2:05 PM
2023-06-27 14:05:59 (UTC+0)
Size
11 KB
Referenced Files
None
Subscribers
None
T250720-rev3.patch
View Options
From 9492df9d37bc7327369b43d7ef03fdf11622e7c1 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 | 38 +++++++++++++++++++
6 files changed, 87 insertions(+), 15 deletions(-)
diff --git a/repo/includes/Actions/SubmitEntityAction.php b/repo/includes/Actions/SubmitEntityAction.php
index 92452f4665..0dacb7fa75 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 Status;
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 @@ use Wikibase\Repo\WikibaseRepo;
*/
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 @@ class SubmitEntityAction extends EditEntityAction {
/**
* @param Title $title
- * @param Content $content
+ * @param EntityContent $content
* @param string $summary
* @param int $undidRevId
* @param int $originalRevId
@@ -222,7 +221,7 @@ class SubmitEntityAction extends EditEntityAction {
* @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 @@ class SubmitEntityAction extends EditEntityAction {
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 RuntimeException;
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 192eb40d0d..58d1393cde 100644
--- a/repo/includes/EditEntity/MediaWikiEditFilterHookRunner.php
+++ b/repo/includes/EditEntity/MediaWikiEditFilterHookRunner.php
@@ -13,6 +13,7 @@ use Wikibase\DataModel\Entity\EntityDocument;
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 @@ class MediaWikiEditFilterHookRunner implements 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
*
@@ -95,13 +96,23 @@ class MediaWikiEditFilterHookRunner implements EditFilterHookRunner {
$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 @@ class StatsdTimeRecordingEditFilterHookRunner implements EditFilterHookRunner {
}
/**
- * @param null|EntityDocument|EntityRedirect $new
+ * @param null|EntityDocument|EntityRedirect|EntityContent $new
* @param IContextSource $context
* @param string $summary
* @return Status
@@ -47,10 +49,21 @@ class StatsdTimeRecordingEditFilterHookRunner implements EditFilterHookRunner {
$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 b897828d87..41a305962c 100644
--- a/repo/tests/phpunit/includes/Actions/ActionTestCase.php
+++ b/repo/tests/phpunit/includes/Actions/ActionTestCase.php
@@ -91,6 +91,10 @@ class ActionTestCase extends MediaWikiIntegrationTestCase {
$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 453d7e2986..ac9d628c3a 100644
--- a/repo/tests/phpunit/includes/Actions/EditEntityActionTest.php
+++ b/repo/tests/phpunit/includes/Actions/EditEntityActionTest.php
@@ -2,12 +2,15 @@
namespace Wikibase\Repo\Tests\Actions;
+use IContextSource;
use MediaWiki\MediaWikiServices;
use RuntimeException;
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 +35,27 @@ class EditEntityActionTest extends ActionTestCase {
// 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 +869,20 @@ class EditEntityActionTest extends ActionTestCase {
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 (Apple Git-143)
File Metadata
Details
Attached
Mime Type
text/x-diff
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
10910440
Default Alt Text
T250720-rev3.patch (11 KB)
Attached To
Mode
T250720: CVE-2023-37301: Wikidata edit filter does not fire when test tool says it should
Attached
Detach File
Event Timeline
Log In to Comment