Merge "Validate imported data"

i18n/en.json

@@ -536,6 +536,7 @@
 	"abusefilter-diff-next": "Newer change",
 	"abusefilter-import-intro": "You can use this interface to import filters from other wikis.\nOn the source wiki, click \"{{int:abusefilter-edit-export}}\" under \"{{int:abusefilter-edit-tools}}\" on the editing interface.\nCopy from the textbox that appears, and paste it into this textbox, then click \"{{int:abusefilter-import-submit}}\".",
 	"abusefilter-import-submit": "Import data",
+	"abusefilter-import-invalid-data": "The data you tried to import is not valid",
 	"abusefilter-group-default": "Default",
 	"abusefilter-http-error": "An HTTP error occurred: $1.",
 	"abusefilter-view-privatedetails-submit": "View private details",

i18n/qqq.json

@@ -570,6 +570,7 @@
 	"abusefilter-diff-next": "Link to the diff view for the next change to this filter.\n\nSee also:\n* {{msg-mw|Abusefilter-diff-prev}}\n* {{msg-mw|Previousdiff}} and {{msg-mw|Nextdiff}}",
 	"abusefilter-import-intro": "{{doc-important|Do not translate <code><nowiki>{{int:abusefilter-edit-export}}</nowiki></code>, <code><nowiki>{{int:abusefilter-tools-subtitle}}</nowiki></code>, and <code><nowiki>{{int:abusefilter-import-submit}}</nowiki></code> unless you absolute must substitute any of them.}}\n\nRefers to:\n* {{msg-mw|Abusefilter-edit-export}}\n* {{msg-mw|Abusefilter-edit-tools}}\n* {{msg-mw|Abusefilter-import-submit}}",
 	"abusefilter-import-submit": "Used as label for the Submit button.\n\nPreceded by the textarea.\n\nUsed in:\n* {{msg-mw|Abusefilter-import-intro}}.",
+	"abusefilter-import-invalid-data": "Error message shown when provided data is invalid.",
 	"abusefilter-group-default": "The name for the default filter group. Most filters will be in this group.\n{{Identical|Default}}",
 	"abusefilter-http-error": "Error message for HTTP requests. Parameters:\n* $1 - HTTP response code.",
 	"abusefilter-view-privatedetails-submit": "Submit button label for viewing private details of an abuse log",

includes/AbuseFilter.php

@@ -641,6 +641,23 @@ class AbuseFilter {
 		return self::$filterCache[$filter];
 	}
 
+	/**
+	 * Checks whether the given object represents a full abuse_filter DB row
+	 * @param stdClass $row
+	 * @return bool
+	 */
+	public static function isFullAbuseFilterRow( stdClass $row ) {
+		$actual = array_keys( get_object_vars( $row ) );
+
+		if (
+			count( $actual ) !== count( self::ALL_ABUSE_FILTER_FIELDS )
+			|| array_diff( self::ALL_ABUSE_FILTER_FIELDS, $actual )
+		) {
+			return false;
+		}
+		return true;
+	}
+
 	/**
 	 * Saves an abuse_filter row in cache
 	 * @param string $id Filter ID (integer or "<GLOBAL_FILTER_PREFIX><integer>")
@@ -650,11 +667,7 @@ class AbuseFilter {
 	public static function cacheFilter( $id, $row ) {
 		// Check that all fields have been passed, otherwise using self::getFilter for this
 		// row will return partial data.
-		$actual = array_keys( get_object_vars( $row ) );
-
-		if ( count( $actual ) !== count( self::ALL_ABUSE_FILTER_FIELDS )
-			|| array_diff( self::ALL_ABUSE_FILTER_FIELDS, $actual )
-		) {
+		if ( !self::isFullAbuseFilterRow( $row ) ) {
 			throw new UnexpectedValueException( 'The specified row must be a full abuse_filter row.' );
 		}
 		self::$filterCache[$id] = $row;

includes/Views/AbuseFilterViewEdit.php

@@ -87,7 +87,18 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 
 		if ( $isImport || ( $request->wasPosted() && !$tokenMatches ) ) {
 			// Make sure to load from HTTP if the token doesn't match!
-			$data = $this->loadRequest( $filter );
+			$status = $this->loadRequest( $filter );
+			if ( !$status->isGood() ) {
+				$out->addHTML(
+					Xml::tags(
+						'p',
+						null,
+						Html::errorBox( $status->getMessage()->parse() )
+					)
+				);
+				return;
+			}
+			$data = $status->getValue();
 		} else {
 			$data = $this->loadFromDatabase( $filter, $history_id );
 		}
@@ -106,7 +117,12 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 	 */
 	private function saveCurrentFilter( $filter, $history_id ) : void {
 		$out = $this->getOutput();
-		list( $newRow, $actions ) = $this->loadRequest( $filter );
+		$reqStatus = $this->loadRequest( $filter );
+		if ( !$reqStatus->isGood() ) {
+			// In the current implementation, this cannot happen.
+			throw new LogicException( 'Should always be able to retrieve data for saving' );
+		}
+		list( $newRow, $actions ) = $reqStatus->getValue();
 		$dbw = wfGetDB( DB_MASTER );
 		$status = AbuseFilter::saveFilter( $this, $filter, $newRow, $actions, $dbw );
 
@@ -1168,9 +1184,10 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 	 * @throws BadMethodCallException If called without the request being POSTed or when trying
 	 *   to import a filter but $filter is not 'new'
 	 * @param int|string $filter The filter ID being requested.
-	 * @return array
+	 * @return Status If good, the value is the array [ row, actions ]. If not, it contains an
+	 * error message.
 	 */
-	public function loadRequest( $filter ): array {
+	public function loadRequest( $filter ): Status {
 		$request = $this->getRequest();
 		if ( !$request->wasPosted() ) {
 			// Sanity
@@ -1197,6 +1214,10 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 			}
 			$data = FormatJson::decode( $import );
 
+			if ( !$this->isValidImportData( $data ) ) {
+				return Status::newFatal( 'abusefilter-import-invalid-data' );
+			}
+
 			$importRow = $data->row;
 			$actions = wfObjectToArray( $data->actions );
 
@@ -1309,7 +1330,7 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 
 		$row->af_actions = implode( ',', array_keys( $actions ) );
 
-		return [ $row, $actions ];
+		return Status::newGood( [ $row, $actions ] );
 	}
 
 	/**
@@ -1348,4 +1369,42 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 			$this->getConfig()->get( 'AbuseFilterDefaultDisallowMessage' )
 		);
 	}
+
+	/**
+	 * Perform basic validation on the JSON-decoded import data. This doesn't check if parameters
+	 * are valid etc., but only if the shape of the object is right.
+	 *
+	 * @param mixed $data Already JSON-decoded
+	 * @return bool
+	 */
+	private function isValidImportData( $data ) {
+		global $wgAbuseFilterActions;
+
+		if ( !is_object( $data ) ) {
+			return false;
+		}
+
+		$arr = get_object_vars( $data );
+
+		$expectedKeys = [ 'row' => true, 'actions' => true ];
+		if ( count( $arr ) !== count( $expectedKeys ) || array_diff_key( $arr, $expectedKeys ) ) {
+			return false;
+		}
+
+		if ( !is_object( $arr['row'] ) || !is_object( $arr['actions'] ) ) {
+			return false;
+		}
+
+		foreach ( $arr['actions'] as $action => $params ) {
+			if ( !array_key_exists( $action, $wgAbuseFilterActions ) || !is_array( $params ) ) {
+				return false;
+			}
+		}
+
+		if ( !AbuseFilter::isFullAbuseFilterRow( $arr['row'] ) ) {
+			return false;
+		}
+
+		return true;
+	}
 }

includes/Views/AbuseFilterViewImport.php

@@ -17,6 +17,7 @@ class AbuseFilterViewImport extends AbuseFilterView {
 		$formDescriptor = [
 			'ImportText' => [
 				'type' => 'textarea',
+				'required' => true
 			]
 		];
 		HTMLForm::factory( 'ooui', $formDescriptor, $this->getContext() )

tests/phpunit/AbuseFilterSaveTest.php

@@ -209,7 +209,11 @@ class AbuseFilterSaveTest extends MediaWikiTestCase {
 		$existing = isset( $args['testData']['existing'] );
 		$viewEdit = $this->getViewEdit( $user, $params, $existing );
 
-		list( $newRow, $actions ) = $viewEdit->loadRequest( $filter );
+		$reqStatus = $viewEdit->loadRequest( $filter );
+		if ( !$reqStatus->isGood() ) {
+			$this->fail( 'Cannot retrieve request data correctly' );
+		}
+		list( $newRow, $actions ) = $reqStatus->getValue();
 
 		/** @var IDatabase|MockObject $dbw */
 		$dbw = $this->getMockForAbstractClass( IDatabase::class );