Ban variable variables

As explained on phab, it's not worth the effort of keeping this feature.

Bug: T229947
Change-Id: Ic6067cab8e1ede98545e704888c99e2ed9a004e4

i18n/en.json

@@ -445,6 +445,7 @@
 	"abusefilter-exception-unclosedcomment": "Unclosed comment at character $1.",
 	"abusefilter-exception-invalidiprange": "Invalid IP range \"$2\" provided at character $1.",
 	"abusefilter-exception-disabledvar": "Variable $2 at character $1 is no longer in use.",
+	"abusefilter-exception-variablevariable": "Variable variables are not allowed, found at character $1.",
 	"abusefilter-action-tag": "Tag",
 	"abusefilter-action-throttle": "Throttle",
 	"abusefilter-action-warn": "Warn",

i18n/qqq.json

@@ -479,6 +479,7 @@
 	"abusefilter-exception-unclosedcomment": "Error message from the abuse filter parser. Parameters:\n* $1 - Position in the string",
 	"abusefilter-exception-invalidiprange": "Error message from the abuse filter parser. Parameters:\n* $1 - Position in the string\n* $2 - String provided as an argument to a function",
 	"abusefilter-exception-disabledvar": "Error message from the abuse filter parser. Parameters:\n* $1 - Position in the string\n* $2 - Name of the disabled variable",
+	"abusefilter-exception-variablevariable": "Error message from the abuse filter parser. Parameters:\n* $1 - Position in the string",
 	"abusefilter-action-tag": "{{doc-abusefilter-action}}\n\nThe edit or change can be 'tagged' with a particular tag, which will be shown on Recent Changes, contributions, logs, new pages, history, and everywhere else. \n\nThis is a verb in the imperative form.\n\n{{Identical|Tag}}",
 	"abusefilter-action-throttle": "{{doc-abusefilter-action}}",
 	"abusefilter-action-warn": "{{doc-abusefilter-action}}",

includes/parser/AFPTreeParser.php

@@ -41,6 +41,16 @@ class AFPTreeParser {
 		list( $this->mCur, $this->mPos ) = $this->mTokens[$this->mPos];
 	}
 
+	/**
+	 * Get the next token. This is similar to move() but doesn't change class members,
+	 *   allowing to look ahead without rolling back the state.
+	 *
+	 * @return AFPToken
+	 */
+	protected function getNextToken() {
+		return $this->mTokens[$this->mPos][0];
+	}
+
 	/**
 	 * getState() function allows parser state to be rollbacked to several tokens
 	 * back.
@@ -528,6 +538,23 @@ class AFPTreeParser {
 				);
 			}
 
+			if ( ( $func === 'set' || $func === 'set_var' ) ) {
+				$state = $this->getState();
+				$this->move();
+				$next = $this->getNextToken();
+				if (
+					$this->mCur->type !== AFPToken::TSTRING ||
+					(
+						$next->type !== AFPToken::TCOMMA &&
+						// Let this fail later, when checking parameters count
+						!( $next->type === AFPToken::TBRACE && $next->value === ')' )
+					)
+				) {
+					throw new AFPUserVisibleException( 'variablevariable', $this->mPos, [] );
+				} else {
+					$this->setState( $state );
+				}
+			}
 			$args = [];
 			do {
 				$args[] = $this->doLevelSemicolon();

includes/parser/AFPUserVisibleException.php

@@ -39,6 +39,7 @@ class AFPUserVisibleException extends AFPException {
 		// abusefilter-exception-overridebuiltin, abusefilter-exception-outofbounds
 		// abusefilter-exception-notarray, abusefilter-exception-unclosedcomment
 		// abusefilter-exception-invalidiprange, abusefilter-exception-disabledvar
+		// abusefilter-exception-variablevariable
 		return wfMessage(
 			'abusefilter-exception-' . $this->mExceptionID,
 			$this->mPosition, ...$this->mParams

includes/parser/AbuseFilterCachingParser.php

@@ -363,7 +363,6 @@ class AbuseFilterCachingParser extends AbuseFilterParser {
 		) {
 			$varnameNode = $node->children[1];
 			if ( $varnameNode->type === AFPTreeNode::ATOM ) {
-				// @todo Handle expressions here
 				$this->setUserVariable( $varnameNode->children->value, new AFPData( AFPData::DUNDEFINED ) );
 			}
 		} elseif ( $node->type === AFPTreeNode::ATOM ) {

includes/parser/AbuseFilterParser.php

@@ -253,7 +253,6 @@ class AbuseFilterParser {
 					$braces++;
 					$next = $this->getNextToken();
 					if ( $next->type === AFPToken::TSTRING ) {
-						// @todo Handle expressions here
 						$this->setUserVariable( $next->value, new AFPData( AFPData::DUNDEFINED ) );
 					}
 				} else {
@@ -833,7 +832,8 @@ class AbuseFilterParser {
 	 */
 	protected function doLevelFunction( &$result ) {
 		if ( $this->mCur->type === AFPToken::TID && isset( self::$mFunctions[$this->mCur->value] ) ) {
-			$func = self::$mFunctions[$this->mCur->value];
+			$fname = $this->mCur->value;
+			$func = self::$mFunctions[$fname];
 			$this->move();
 			if ( $this->mCur->type !== AFPToken::TBRACE || $this->mCur->value !== '(' ) {
 				throw new AFPUserVisibleException( 'expectednotfound',
@@ -858,6 +858,23 @@ class AbuseFilterParser {
 			$args = [];
 			$next = $this->getNextToken();
 			if ( $next->type !== AFPToken::TBRACE || $next->value !== ')' ) {
+				if ( ( $fname === 'set' || $fname === 'set_var' ) ) {
+					$state = $this->getState();
+					$this->move();
+					$next = $this->getNextToken();
+					if (
+						$this->mCur->type !== AFPToken::TSTRING ||
+						(
+							$next->type !== AFPToken::TCOMMA &&
+							// Let this fail later, when checking parameters count
+							!( $next->type === AFPToken::TBRACE && $next->value === ')' )
+						)
+					) {
+						throw new AFPUserVisibleException( 'variablevariable', $this->mCur->pos, [] );
+					} else {
+						$this->setState( $state );
+					}
+				}
 				do {
 					$r = new AFPData( AFPData::DEMPTY );
 					$this->doLevelSemicolon( $r );

tests/phpunit/AbuseFilterParserTest.php

@@ -408,6 +408,33 @@ class AbuseFilterParserTest extends AbuseFilterParserTestCase {
 		];
 	}
 
+	/**
+	 * Test the 'variablevariable' exception
+	 *
+	 * @param string $expr The expression to test
+	 * @param string $caller The function where the exception is thrown
+	 * @dataProvider variableVariable
+	 */
+	public function testVariableVariableException( $expr, $caller ) {
+		$this->exceptionTest( 'variablevariable', $expr, $caller );
+	}
+
+	/**
+	 * Data provider for testVariableVariableException
+	 * The second parameter is the function where the exception is raised.
+	 * One expression for each throw.
+	 *
+	 * @return array
+	 */
+	public function variableVariable() {
+		return [
+			[ "set( 'x' + 'y', 1 )", 'doLevelFunction' ],
+			[ "set( 'x' + page_title, 1 )", 'doLevelFunction' ],
+			[ "set( page_title, 1 )", 'doLevelFunction' ],
+			[ "set( page_title + 'x' + ( page_namespace == 0 ? 'x' : 'y' )", 'doLevelFunction' ],
+		];
+	}
+
 	/**
 	 * Test the 'overridebuiltin' exception
 	 *