Disallow protected variable access on AbuseFilterViewTestBatch

A filter using a protected variable can be loaded via filter id
using testing tools even though the user might not have the right
to view protected variables. This can potentially leak PII and as
such, testing tools should check for the right before allowing
protected filters to be seen.

- Unload a filter asap if it uses protected variables and the
  requestor doesn't have viewing rights. This:
    + disallows loading of existing protected filters on page load
    + disallows testing against rules that use protected variables
    + disallows subsequent requests for protected filters (via API)

There is a known bug (see T369620) where no user feedback is
provided if an API request for a filter returns no result (typically
when no filter matches the requested id). This commit adds another
pathway to that bug (the filter exists but is protected and not
returned by the API) but does not update this UI/UX.

Bug: T364834
Change-Id: I6a572790edd743596d70c9c4a2ee52b4561e25f3

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 );
 	}