Haven't checked deeply, but a possible hint is that AF doesn't see automatic edit summaries (I think there was a task for that) which, however, it sees when retrospectively examinating edits.

In T250720#6072828, @Daimona wrote:

Haven't checked deeply, but a possible hint is that AF doesn't see automatic edit summaries (I think there was a task for that) which, however, it sees when retrospectively examinating edits.

But still, the edit summary it sees (the *real* summary) is something like /* undo:0||REVID|USERNAME */. The filter's summary-related part is designed to catch "undo:0" (not "Undo revision REVID by USERNAME", which is the automatically generated summary).

For information: I have now disabled this edit filter in favour of a set of softblocks on certain /16 IP ranges.

For testing, I re-used blank https://www.wikidata.org/wiki/Special:AbuseFilter/31 with

'undo:0' in summary

During the test, there were at least 9 undo actions (tagged as mw-undo, example) but none of them triggered the filter. So @Daimona may be right...

In T250720#6073559, @Ahmad252 wrote:

In T250720#6072828, @Daimona wrote:

Haven't checked deeply, but a possible hint is that AF doesn't see automatic edit summaries (I think there was a task for that) which, however, it sees when retrospectively examinating edits.

But still, the edit summary it sees (the *real* summary) is something like /* undo:0||REVID|USERNAME */.

It depends on how you define "real". I have to say, I'm not at all certain about the summary that the filter is effectively seeing, so... I don't even know where the 'undo:0 ...' part is coming from in Wikibase, so I can't tell whether it can be read by the AF.

In T250720#6074608, @matej_suchanek wrote:

For testing, I re-used blank https://www.wikidata.org/wiki/Special:AbuseFilter/31 with

'undo:0' in summary

During the test, there were at least 9 undo actions (tagged as mw-undo, example) but none of them triggered the filter. So @Daimona may be right...

Not surprising... I'd be glad to confirm this and investigate it locally, but I don't have Wikibase installed on my wiki, and it's also not so easy to install IIRC (also I'm not practical with it).

We are currently dealing with LTA IPs on Wikidata which harass users by undoing their edits. An abuse filter has been created to block them automatically, but it isn't effective, and the addresses evade it seamlessly.

This made me look at this problem again, and unfortunately, abuse filters are apparently not run at all when undoing edits to items on Wikidata.

I am protecting this as a security problem.

SubmitEntityAction (Wikibase), which handles undos, does not fire the EditFilterMergedContentHook, which is the entry point to having the change checked by edit filters.
It claims it does not have to do constraint checks, but edit filters had definitely not been run by then:

// NOTE: Constraint checks are performed automatically via EntityHandler::validateSave.
$status = $page->doUserEditContent(
	$content,
	$this->getUser(),
	$summary,
	/* flags */ 0,
	$originalRevId ?: false,
	/* tags */ [],
	$undidRevId
);

The least effort fix is to fire the hook from SubmitEntityAction::attemptSave:

diff --git a/repo/includes/Actions/SubmitEntityAction.php b/repo/includes/Actions/SubmitEntityAction.php
index 93fc60f7a0..93a583a8f5 100644
--- a/repo/includes/Actions/SubmitEntityAction.php
+++ b/repo/includes/Actions/SubmitEntityAction.php
@@ -5,6 +5,7 @@ namespace Wikibase\Repo\Actions;
 use Article;
 use Content;
 use IContextSource;
+use MediaWiki\HookContainer\ProtectedHookAccessorTrait;
 use MediaWiki\MediaWikiServices;
 use MediaWiki\Permissions\PermissionManager;
 use MediaWiki\Revision\RevisionRecord;
@@ -28,6 +29,7 @@ use Wikibase\Repo\WikibaseRepo;
  * @author Daniel Kinzler
  */
 class SubmitEntityAction extends EditEntityAction {
+       use ProtectedHookAccessorTrait;

        /**
         * @var SummaryFormatter
@@ -239,6 +241,13 @@ class SubmitEntityAction extends EditEntityAction {
                        return $status;
                }

+               if ( !$this->getHookContainer()->run(
+                       'EditFilterMergedContent',
+                       [ $this->getContext(), $content, &$status, $summary, $this->getUser(), false ]
+               ) ) {
+                       return $status;
+               }
+
                // save edit
                $page = MediaWikiServices::getInstance()->getWikiPageFactory()
                        ->newFromTitle( $title );

Or we should inject MediawikiEditFilterHookRunner which fires the hook, too. I am not sure.

In T250720#8830971, @matej_suchanek wrote:

SubmitEntityAction (Wikibase), which handles undos, does not fire the EditFilterMergedContentHook, which is the entry point to having the change checked by edit filters.
It claims it does not have to do constraint checks, but edit filters had definitely not been run by then:

I believe “constraint checks” refers to constraints like “no two items can have the same sitelink” (see docs: constraints); I don’t think it would include edit filters.

Or we should inject MediawikiEditFilterHookRunner which fires the hook, too. I am not sure.

Using that class sounds good to me. It can be injected as the WikibaseRepo.EditFilterHookRunner service; HistoryEntityAction looks like it’s possible to inject services into actions, so we can probably reuse some patterns from there.

FTR, according to Re-implement undo action based on EntityContent, the reason that this code doesn’t use the EditEntity interface (which would take care of edit filters and other things) is that EditEntity doesn’t support redirects (EntityDocument and EntityRedirect are disparate types), but undoing redirects needs to be supported.

Task Breakdown notes

• This justifies making sure this issue is not ocurring in other areas, e.g. EntitySchema
• We should also make sure it abuse filters work for rollbacks (but this is improbable since most users do not have permissions for rolling back)
• the current plan of action is mentioned in the comments above

NOTE This is a security patch, which should not go on Gerrit, but rather be attached to this task

We should also make sure it abuse filters work for rollbacks (but this is improbable since most users do not have permissions for rolling back)

AFAIK rollbacks bypass abuse filters on purpose.

In T250720#8927333, @noarave wrote:

NOTE This is a security patch, which should not go on Gerrit, but rather be attached to this task

The Security-Team is always happy to assist with a security deployment once CR looks good. Just ping us here if such assistance is needed.

Security patch for review:

I ran the repo PHPUnit tests locally and they still pass.

In T250720#8930829, @Lucas_Werkmeister_WMDE wrote:

Security patch for review:

0001-SECURITY-Run-edit-filters-on-submit-undo-restore.patch11 KBDownload

I ran the repo PHPUnit tests locally and they still pass.

Reviewed: +2

PS: The new exception in StatsdTimeRecordingEditFilterHookRunner should really be fine, but in general I would prefer a silent ignore or warning in that case for the backport (this is for stats tracking only, after all).

In T250720#8931585, @hoo wrote:

PS: The new exception in StatsdTimeRecordingEditFilterHookRunner should really be fine, but in general I would prefer a silent ignore or warning in that case for the backport (this is for stats tracking only, after all).

Fair enough, let’s switch it out. (We can convert this and other places to real union types once T328921: Drop PHP 7.4 support from MediaWiki is done.)

Diff between old and new patch:

diff --git a/repo/includes/EditEntity/StatsdTimeRecordingEditFilterHookRunner.php b/repo/includes/EditEntity/StatsdTimeRecordingEditFilterHookRunner.php
index 3fe3a1536d..3ed14bc8a6 100644
--- a/repo/includes/EditEntity/StatsdTimeRecordingEditFilterHookRunner.php
+++ b/repo/includes/EditEntity/StatsdTimeRecordingEditFilterHookRunner.php
@@ -57,7 +57,7 @@ public function run( $new, IContextSource $context, $summary ) {
 			} elseif ( $new instanceof EntityContent ) {
 				$entityType = $new->getEntityId()->getEntityType();
 			} else {
-				throw new InvalidArgumentException( '$new must be instance of EntityDocument, EntityRedirect or EntityContent' );
+				$entityType = 'UNKNOWN';
 			}
 			$this->stats->timing(
 				"{$this->timingPrefix}.run.{$entityType}",

Should be deployed now. @matej_suchanek can you confirm? (I guess we should see more matches in the filter 210 abuse log soon?)

Yes. I have just done the same experiment as previously:

In T250720#6074608, @matej_suchanek wrote:

For testing, I re-used blank https://www.wikidata.org/wiki/Special:AbuseFilter/31 with

'undo:0' in summary

After a few minutes, several edits were matched.
The variables dump also looks correct at a glance.

It sounds like the security patch is deployed and stable now? If so, can we track this for the next supplemental security release (T333626)? ext:Wikibase isn't bundled.

In T250720#8935764, @sbassett wrote:

It sounds like the security patch is deployed and stable now? If so, can we track this for the next supplemental security release (T333626)? ext:Wikibase isn't bundled.

I think so, yes. Tagging Wikibase Suite release crew for attention.

Is there any ETA for this next supplemental security release? Those are published quarterly in general, right? So end of June/early July?
Asking to anticipate when codes of fixes will be published so it could be released for non-Wikidata Wikibase users.

In T250720#8957433, @WMDE-leszek wrote:

Is there any ETA for this next supplemental security release? Those are published quarterly in general, right? So end of June/early July?
Asking to anticipate when codes of fixes will be published so it could be released for non-Wikidata Wikibase users.

The Security-Team are requesting CVEs yesterday and today for the next supplemental release, which we hope is out by next Friday, June 30th. Or, at worst, Wednesday, July 5th.

thank you @sbassett

Hello, the patch for this task failed to apply against 1.41.0-wmf.15:

Applying patch /srv/patches/1.41.0-wmf.15/extensions/Wikibase/01-T250720.patch in /srv/mediawiki-staging/php-1.41.0-wmf.15
ERROR: git am: error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch' to see the failed patch

[FAILED] /srv/patches/1.41.0-wmf.15/extensions/Wikibase/01-T250720.patch

In T250720#8966220, @hashar wrote:

Hello, the patch for this task failed to apply against 1.41.0-wmf.15:

Applying patch /srv/patches/1.41.0-wmf.15/extensions/Wikibase/01-T250720.patch in /srv/mediawiki-staging/php-1.41.0-wmf.15
ERROR: git am: error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch' to see the failed patch

[FAILED] /srv/patches/1.41.0-wmf.15/extensions/Wikibase/01-T250720.patch

New patch:

A --3way almost fixed it, but there was a minor conflict in the use statements in repo/tests/phpunit/includes/Actions/EditEntityActionTest.php. This applies fine to Wikibase@master for me, so I assume it will apply to 1.41.0-wmf.15.

Thanks. I've committed the above to /srv/patches.

In T250720#8968331, @sbassett wrote:

...
A --3way almost fixed it, but there was a minor conflict in the use statements in repo/tests/phpunit/includes/Actions/EditEntityActionTest.php. This applies fine to Wikibase@master for me, so I assume it will apply to 1.41.0-wmf.15.

Thank you very much for the rebase!

Change 933663 had a related patch set uploaded (by Mstyles; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] SECURITY: Run edit filters on submit (undo/restore)

https://gerrit.wikimedia.org/r/933663

Change 933663 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] SECURITY: Run edit filters on submit (undo/restore)

https://gerrit.wikimedia.org/r/933663