Reject filters with invalid groups

It is currently possible to save a filter with an invalid group, if you
manually change the form data. So prevent this by validating the group
before saving.

Change-Id: I03f80b8c6ab583a357273f7b2679a424ac784db7

i18n/en.json

@@ -289,6 +289,7 @@
 	"abusefilter-edit-empty-throttlegroups": "At least one throttle group must be selected.",
 	"abusefilter-edit-duplicated-throttlegroups": "Throttle groups cannot have duplicates.",
 	"abusefilter-edit-invalid-throttlegroups": "The specified throttle groups are not valid.",
+	"abusefilter-edit-invalid-group": "The specified filter group ('$1') is not valid.",
 	"abusefilter-edit-builder-select": "Select an option to add it at the cursor",
 	"abusefilter-edit-builder-group-op-arithmetic": "Arithmetic operators",
 	"abusefilter-edit-builder-op-arithmetic-addition": "Addition (+)",

i18n/qqq.json

@@ -329,6 +329,7 @@
 	"abusefilter-edit-empty-throttlegroups": "Error message when trying to save a filter with \"throttle\" action enabled but no throttle groups selected.",
 	"abusefilter-edit-duplicated-throttlegroups": "Error message when trying to save a filter with \"throttle\" action enabled and duplicated throttle groups.",
 	"abusefilter-edit-invalid-throttlegroups": "Error message when trying to save a filter with \"throttle\" action enabled and invalid throttle groups.",
+	"abusefilter-edit-invalid-group": "Error message when trying to save a filter with an invalid group. Parameters:\n* $1 - the group\n\nSee also {{msg-mw|Abusefilter-edit-group}}.",
 	"abusefilter-edit-builder-select": "Default value for dropdown menu that allows inserting abuse filter syntax in the filter definition field.",
 	"abusefilter-edit-builder-group-op-arithmetic": "Group entry in dropdown menu.",
 	"abusefilter-edit-builder-op-arithmetic-addition": "Abuse filter syntax option in a dropdown from the group {{msg-mw|abusefilter-edit-builder-group-op-arithmetic}}.",

includes/FilterStore.php

@@ -219,16 +219,9 @@ class FilterStore {
 
 		$dbw->endAtomic( __METHOD__ );
 
-		// TODO: isset() shouldn't be necessary,
-		// invalid values should also be discarded upstream
-		$group = 'default';
-		if ( isset( $newRow['af_group'] ) && $newRow['af_group'] !== '' ) {
-			$group = $newRow['af_group'];
-		}
-
 		// Invalidate cache if this was a global rule
 		if ( $wasGlobal || $newRow['af_global'] ) {
-			$this->filterLookup->purgeGroupWANCache( $group );
+			$this->filterLookup->purgeGroupWANCache( $newRow['af_group'] );
 		}
 
 		// Logging
@@ -250,7 +243,7 @@ class FilterStore {
 
 		$this->filterProfiler->resetFilterProfile( $newID );
 		if ( $newRow['af_enabled'] ) {
-			$this->emergencyCache->setNewForFilter( $newID, $group );
+			$this->emergencyCache->setNewForFilter( $newID, $newRow['af_group'] );
 		}
 		return [ $newID, $historyID ];
 	}

includes/FilterValidator.php

@@ -2,6 +2,7 @@
 
 namespace MediaWiki\Extension\AbuseFilter;
 
+use MediaWiki\Config\ServiceOptions;
 use MediaWiki\Extension\AbuseFilter\ChangeTags\ChangeTagValidator;
 use MediaWiki\Extension\AbuseFilter\Filter\AbstractFilter;
 use MediaWiki\Extension\AbuseFilter\Parser\AFPUserVisibleException;
@@ -17,6 +18,11 @@ use User;
 class FilterValidator {
 	public const SERVICE_NAME = 'AbuseFilterFilterValidator';
 
+	public const CONSTRUCTOR_OPTIONS = [
+		'AbuseFilterValidGroups',
+		'AbuseFilterActionRestrictions'
+	];
+
 	/** @var ChangeTagValidator */
 	private $changeTagValidator;
 
@@ -29,22 +35,26 @@ class FilterValidator {
 	/** @var string[] */
 	private $restrictedActions;
 
+	/** @var string[] */
+	private $validGroups;
+
 	/**
 	 * @param ChangeTagValidator $changeTagValidator
 	 * @param ParserFactory $parserFactory
 	 * @param AbuseFilterPermissionManager $permManager
-	 * @param string[] $restrictedActions
+	 * @param ServiceOptions $options
 	 */
 	public function __construct(
 		ChangeTagValidator $changeTagValidator,
 		ParserFactory $parserFactory,
 		AbuseFilterPermissionManager $permManager,
-		array $restrictedActions
+		ServiceOptions $options
 	) {
 		$this->changeTagValidator = $changeTagValidator;
 		$this->parserFactory = $parserFactory;
 		$this->permManager = $permManager;
-		$this->restrictedActions = $restrictedActions;
+		$this->restrictedActions = array_keys( array_filter( $options->get( 'AbuseFilterActionRestrictions' ) ) );
+		$this->validGroups = $options->get( 'AbuseFilterValidGroups' );
 	}
 
 	/**
@@ -106,6 +116,11 @@ class FilterValidator {
 			return $restrictedActionsStatus;
 		}
 
+		$filterGroupStatus = $this->checkGroup( $newFilter );
+		if ( !$filterGroupStatus->isGood() ) {
+			return $filterGroupStatus;
+		}
+
 		return Status::newGood();
 	}
 
@@ -327,4 +342,17 @@ class FilterValidator {
 		}
 		return $ret;
 	}
+
+	/**
+	 * @param AbstractFilter $filter
+	 * @return Status
+	 */
+	public function checkGroup( AbstractFilter $filter ) : Status {
+		$ret = Status::newGood();
+		$group = $filter->getGroup();
+		if ( !in_array( $group, $this->validGroups, true ) ) {
+			$ret->error( 'abusefilter-edit-invalid-group' );
+		}
+		return $ret;
+	}
 }

includes/ServiceWiring.php

@@ -157,8 +157,10 @@ return [
 			$services->get( ChangeTagValidator::SERVICE_NAME ),
 			$services->get( ParserFactory::SERVICE_NAME ),
 			$services->get( PermManager::SERVICE_NAME ),
-			// Pass the cleaned list of enabled restrictions
-			array_keys( array_filter( $services->getMainConfig()->get( 'AbuseFilterActionRestrictions' ) ) )
+			new ServiceOptions(
+				FilterValidator::CONSTRUCTOR_OPTIONS,
+				$services->getMainConfig()
+			)
 		);
 	},
 	FilterCompare::SERVICE_NAME => function ( MediaWikiServices $services ): FilterCompare {

tests/phpunit/AbuseFilterSaveTest.php

@@ -22,6 +22,7 @@
  * @license GPL-2.0-or-later
  */
 
+use MediaWiki\Config\ServiceOptions;
 use MediaWiki\Extension\AbuseFilter\AbuseFilterPermissionManager;
 use MediaWiki\Extension\AbuseFilter\AbuseFilterServices;
 use MediaWiki\Extension\AbuseFilter\Filter\Filter;
@@ -231,7 +232,13 @@ class AbuseFilterSaveTest extends MediaWikiIntegrationTestCase {
 			AbuseFilterServices::getChangeTagValidator(),
 			$this->createMock( ParserFactory::class ),
 			$this->createMock( AbuseFilterPermissionManager::class ),
-			[]
+			new ServiceOptions(
+				FilterValidator::CONSTRUCTOR_OPTIONS,
+				[
+					'AbuseFilterActionRestrictions' => [],
+					'AbuseFilterValidGroups' => [ 'default' ]
+				]
+			)
 		);
 
 		$status = $validator->checkAllTags( $tags );

tests/phpunit/unit/FilterValidatorTest.php

@@ -3,6 +3,7 @@
 namespace MediaWiki\Extension\AbuseFilter\Tests\Unit;
 
 use Generator;
+use MediaWiki\Config\ServiceOptions;
 use MediaWiki\Extension\AbuseFilter\AbuseFilterPermissionManager;
 use MediaWiki\Extension\AbuseFilter\ChangeTags\ChangeTagValidator;
 use MediaWiki\Extension\AbuseFilter\Filter\AbstractFilter;
@@ -30,12 +31,14 @@ class FilterValidatorTest extends MediaWikiUnitTestCase {
 	 * @param AbuseFilterPermissionManager|null $permissionManager
 	 * @param AbuseFilterParser|null $parser
 	 * @param array $restrictions
+	 * @param array $validFilterGroups
 	 * @return FilterValidator
 	 */
 	private function getFilterValidator(
 		AbuseFilterPermissionManager $permissionManager = null,
 		AbuseFilterParser $parser = null,
-		array $restrictions = []
+		array $restrictions = [],
+		array $validFilterGroups = [ 'default' ]
 	) : FilterValidator {
 		if ( !$parser ) {
 			$parser = $this->createMock( AbuseFilterParser::class );
@@ -53,7 +56,13 @@ class FilterValidatorTest extends MediaWikiUnitTestCase {
 			$this->createMock( ChangeTagValidator::class ),
 			$parserFactory,
 			$permissionManager,
-			$restrictions
+			new ServiceOptions(
+				FilterValidator::CONSTRUCTOR_OPTIONS,
+				[
+					'AbuseFilterActionRestrictions' => array_fill_keys( $restrictions, true ),
+					'AbuseFilterValidGroups' => $validFilterGroups
+				]
+			)
 		);
 	}
 
@@ -427,9 +436,41 @@ class FilterValidatorTest extends MediaWikiUnitTestCase {
 			[ 'degroup' ]
 		];
 
+		$invalidGroupFilter = $this->createMock( AbstractFilter::class );
+		$invalidGroupFilter->method( 'getRules' )->willReturn( 'true' );
+		$invalidGroupFilter->method( 'getName' )->willReturn( 'Foo' );
+		$invalidGroupFilter->expects( $this->atLeastOnce() )->method( 'getGroup' )->willReturn( 'xxx-invalid' );
+		yield 'invalid group' => [ $invalidGroupFilter, 'abusefilter-edit-invalid-group' ];
+
 		$filter = $this->createMock( AbstractFilter::class );
 		$filter->method( 'getRules' )->willReturn( 'true' );
 		$filter->method( 'getName' )->willReturn( 'Foo' );
+		$filter->method( 'getGroup' )->willReturn( 'default' );
 		yield 'valid' => [ $filter, null ];
 	}
+
+	/**
+	 * @param string $group
+	 * @param string[] $validGroups
+	 * @param string|null $expected
+	 * @covers ::checkGroup
+	 * @dataProvider provideGroups
+	 */
+	public function testCheckGroup( string $group, array $validGroups, ?string $expected ) {
+		$filter = $this->createMock( AbstractFilter::class );
+		$filter->expects( $this->atLeastOnce() )->method( 'getGroup' )->willReturn( $group );
+		$this->assertStatusMessage(
+			$expected,
+			$this->getFilterValidator( null, null, [], $validGroups )->checkGroup( $filter )
+		);
+	}
+
+	public function provideGroups() : Generator {
+		$allowed = [ 'default' ];
+		yield 'Default, pass' => [ 'default', $allowed, null ];
+		$extraGroup = 'foobar';
+		$allowed[] = $extraGroup;
+		yield 'Extra, pass' => [ $extraGroup, $allowed, null ];
+		yield 'Unknown, fail' => [ 'some-unknown-group', $allowed, 'abusefilter-edit-invalid-group' ];
+	}
 }