Merge "Disallow protected variable access on AbuseFilterViewTestBatch"

i18n/en.json

@@ -531,6 +531,7 @@
 	"abusefilter-test-page": "Changes made to page:",
 	"abusefilter-test-shownegative": "Show changes that do not match the filter",
 	"abusefilter-test-syntaxerr": "The filter you entered contained a syntax error.\nYou can receive a full explanation by clicking the \"{{int:abusefilter-edit-check}}\" button.",
+	"abusefilter-test-protectedvarerr": "The filter is not shown, as it uses protected variables and is hidden from public view.",
 	"abusefilter-test-action": "Action type:",
 	"abusefilter-test-search-type-all": "All actions",
 	"abusefilter-test-search-type-edit": "Edits",

i18n/qqq.json

@@ -576,6 +576,7 @@
 	"abusefilter-test-page": "Used as label on [[Special:AbuseFilter/test]]",
 	"abusefilter-test-shownegative": "Used as label on [[Special:AbuseFilter/test]]",
 	"abusefilter-test-syntaxerr": "Refers to {{msg-mw|Abusefilter-edit-check}}.",
+	"abusefilter-test-protectedvarerr": "Error message shown to the user on [[Special:AbuseFilter/test]] if the filter they are trying to view uses protected variables and they do not have permission to view it.",
 	"abusefilter-test-action": "Used as label on [[Special:AbuseFilter/test]]\n{{Identical|Type of action}}",
 	"abusefilter-test-search-type-all": "Option allowing to show every type of action.",
 	"abusefilter-test-search-type-edit": "Option allowing to only show edits.",

includes/View/AbuseFilterViewTestBatch.php

@@ -90,6 +90,22 @@ class AbuseFilterViewTestBatch extends AbuseFilterView {
 
 		$this->loadParameters();
 
+		// Check if a loaded test pattern uses protected variables and if the user has the right
+		// to view protected variables. If they don't and protected variables are present, unset
+		// the test pattern to avoid leaking PII and notify the user.
+		// This is done as early as possible so that a filter with PII the user cannot access is
+		// never loaded.
+		if ( $this->testPattern !== '' ) {
+			$ruleChecker = $this->ruleCheckerFactory->newRuleChecker();
+			$usedVars = $ruleChecker->getUsedVars( $this->testPattern );
+			if ( $this->afPermManager->getForbiddenVariables( $this->getAuthority(), $usedVars ) ) {
+				$this->testPattern = '';
+				$out->addHtml(
+					Html::errorBox( $this->msg( 'abusefilter-test-protectedvarerr' )->parse() )
+				);
+			}
+		}
+
 		$out->setPageTitleMsg( $this->msg( 'abusefilter-test' ) );
 		$out->addHelpLink( 'Extension:AbuseFilter/Rules format' );
 		$out->addWikiMsg( 'abusefilter-test-intro', self::$mChangeLimit );

tests/phpunit/integration/Special/SpecialAbuseFilterTest.php

@@ -44,6 +44,55 @@ use Wikimedia\TestingAccessWrapper;
 class SpecialAbuseFilterTest extends SpecialPageTestBase {
 	use MockAuthorityTrait;
 
+	/**
+	 * @var SimpleAuthority
+	 */
+	private $authorityCannotViewProtectedVar;
+
+	/**
+	 * @var SimpleAuthority
+	 */
+	private $authorityCanViewProtectedVar;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		// Add filter to query for
+		$filter = [
+			'id' => '1',
+			'rules' => 'user_unnamed_ip = "1.2.3.4"',
+			'name' => 'Filter with protected variables',
+			'hidden' => Flags::FILTER_USES_PROTECTED_VARS,
+			'user' => 0,
+			'user_text' => 'FilterTester',
+			'timestamp' => '20190826000000',
+			'enabled' => 1,
+			'comments' => '',
+			'hit_count' => 0,
+			'throttled' => 0,
+			'deleted' => 0,
+			'actions' => [],
+			'global' => 0,
+			'group' => 'default'
+		];
+		$this->createFilter( $filter );
+
+		// Create the user to query for filters
+		$user = $this->getTestSysop()->getUser();
+
+		// Create an authority who can see private filters but not protected variables
+		$this->authorityCannotViewProtectedVar = new SimpleAuthority(
+			$user,
+			[ 'abusefilter-log-private', 'abusefilter-view-private' ]
+		);
+
+		// Create an authority who can see private and protected variables
+		$this->authorityCanViewProtectedVar = new SimpleAuthority(
+			$user,
+			[ 'abusefilter-access-protected-vars', 'abusefilter-log-private', 'abusefilter-view-private' ]
+		);
+	}
+
 	/**
 	 * @dataProvider provideInstantiateView
 	 */
@@ -140,42 +189,27 @@ class SpecialAbuseFilterTest extends SpecialPageTestBase {
 			->execute();
 	}
 
-	public function testProtectedVarsFilterVisibility() {
-		// Add filter to query for
-		$filter = [
-			'id' => '1',
-			'rules' => 'user_unnamed_ip = 1.2.3.4',
-			'name' => 'Filter with protected variables',
-			'hidden' => Flags::FILTER_USES_PROTECTED_VARS,
-			'user' => 0,
-			'user_text' => 'FilterTester',
-			'timestamp' => '20190826000000',
-			'enabled' => 1,
-			'comments' => '',
-			'hit_count' => 0,
-			'throttled' => 0,
-			'deleted' => 0,
-			'actions' => [],
-			'global' => 0,
-			'group' => 'default'
-		];
-		$this->createFilter( $filter );
-
-		// Create the user to query for filters
-		$user = $this->getTestSysop()->getUser();
-
-		// Create an authority who can see private filters but not protected variables
-		$authorityCannotViewProtectedVar = new SimpleAuthority(
-			$user,
-			[ 'abusefilter-log-private', 'abusefilter-view-private' ]
+	public function testViewTestBatchProtectedVarsFilterVisibility() {
+		// Assert that the user who cannot see protected variables cannot load the filter
+		[ $html, ] = $this->executeSpecialPage(
+			'test/1',
+			new FauxRequest(),
+			null,
+			$this->authorityCannotViewProtectedVar
 		);
+		$this->assertStringNotContainsString( '1.2.3.4', $html );
 
-		// Create an authority who can see private and protected variables
-		$authorityCanViewProtectedVar = new SimpleAuthority(
-			$user,
-			[ 'abusefilter-access-protected-vars', 'abusefilter-log-private', 'abusefilter-view-private' ]
+		// Assert that the user who can see protected variables can load the filter
+		[ $html, ] = $this->executeSpecialPage(
+			'test/1',
+			new FauxRequest(),
+			null,
+			$this->authorityCanViewProtectedVar
 		);
+		$this->assertStringContainsString( '1.2.3.4', $html );
+	}
 
+	public function testViewListProtectedVarsFilterVisibility() {
 		// Stub out a page with query results for a filter that uses protected variables
 		// &sort=af_id&limit=50&asc=&desc=1&deletedfilters=hide&querypattern=user_unnamed_ip&searchoption=LIKE
 		$requestWithProtectedVar = new FauxRequest( [
@@ -195,7 +229,7 @@ class SpecialAbuseFilterTest extends SpecialPageTestBase {
 			'',
 			$requestWithProtectedVar,
 			null,
-			$authorityCannotViewProtectedVar
+			$this->authorityCannotViewProtectedVar
 		);
 		$this->assertStringContainsString( 'table_pager_empty', $html );
 
@@ -204,7 +238,7 @@ class SpecialAbuseFilterTest extends SpecialPageTestBase {
 			'',
 			$requestWithProtectedVar,
 			null,
-			$authorityCanViewProtectedVar
+			$this->authorityCanViewProtectedVar
 		);
 		$this->assertStringContainsString( '1.2.3.4', $html );
 	}