Validate max length of bp_restrictions and bp_grants

Bug: T260631
Bug: T260633
Change-Id: Ifc35e01c711f1394f45748f693e7a46695b2d471

RELEASE-NOTES-1.36

@@ -322,6 +322,8 @@ because of Phabricator reports.
   It had no known callers.
 * wfForeignMemcKey() and wfMemcKey(), deprecated in 1.35, have been removed.
 * MediaWiki now also requires the php-intl extension.
+* BotPassword::save() now returns a Status object for the result rather than
+  a bool.
 * …
 
 === Deprecations in 1.36 ===

includes/specials/SpecialBotPasswords.php

@@ -330,6 +330,11 @@ class SpecialBotPasswords extends FormSpecialPage {
 			)
 		] );
 
+		if ( $bp === null ) {
+			// Messages: botpasswords-insert-failed, botpasswords-update-failed
+			return Status::newFatal( "botpasswords-{$this->operation}-failed", $this->par );
+		}
+
 		if ( $this->operation === 'insert' || !empty( $data['resetPassword'] ) ) {
 			$this->password = BotPassword::generatePassword( $this->getConfig() );
 			$password = $this->passwordFactory->newFromPlaintext( $this->password );
@@ -337,24 +342,25 @@ class SpecialBotPasswords extends FormSpecialPage {
 			$password = null;
 		}
 
-		if ( $bp && $bp->save( $this->operation, $password ) ) {
-			$this->logger->info(
-				"Bot password {op} for {user}@{app_id}",
-				[
-					'op' => $this->operation,
-					'user' => $this->getUser()->getName(),
-					'app_id' => $this->par,
-					'centralId' => $this->userId,
-					'restrictions' => $data['restrictions'],
-					'grants' => $bp->getGrants(),
-					'client_ip' => $this->getRequest()->getIP()
-				]
-			);
-			return Status::newGood();
-		}
+		$res = $bp->save( $this->operation, $password );
 
-		// Messages: botpasswords-insert-failed, botpasswords-update-failed
-		return Status::newFatal( "botpasswords-{$this->operation}-failed", $this->par );
+		$success = $res->isGood();
+
+		$this->logger->info(
+			'Bot password {op} for {user}@{app_id} ' . ( $success ? 'succeeded' : 'failed' ),
+			[
+				'op' => $this->operation,
+				'user' => $this->getUser()->getName(),
+				'app_id' => $this->par,
+				'centralId' => $this->userId,
+				'restrictions' => $data['restrictions'],
+				'grants' => $bp->getGrants(),
+				'client_ip' => $this->getRequest()->getIP(),
+				'success' => $success,
+			]
+		);
+
+		return $res;
 	}
 
 	public function onSuccess() {

includes/user/BotPassword.php

@@ -38,6 +38,18 @@ class BotPassword implements IDBAccessObject {
 	 */
 	public const PASSWORD_MINLENGTH = 32;
 
+	/**
+	 * Maximum length of the json representation of restrictions
+	 * @since 1.36
+	 */
+	public const RESTRICTIONS_MAXLENGTH = 65535;
+
+	/**
+	 * Maximum length of the json representation of grants
+	 * @since 1.36
+	 */
+	public const GRANTS_MAXLENGTH = 65535;
+
 	/** @var bool */
 	private $isSaved;
 
@@ -277,22 +289,44 @@ class BotPassword implements IDBAccessObject {
 	 * Save the BotPassword to the database
 	 * @param string $operation 'update' or 'insert'
 	 * @param Password|null $password Password to set.
-	 * @return bool Success
+	 * @return Status
+	 * @throws UnexpectedValueException
 	 */
 	public function save( $operation, Password $password = null ) {
 		// Ensure operation is valid
 		if ( $operation !== 'insert' && $operation !== 'update' ) {
-			return false;
+			throw new UnexpectedValueException(
+				"Expected 'insert' or 'update'; got '{$operation}'."
+			);
 		}
 
 		$conds = [
 			'bp_user' => $this->centralId,
 			'bp_app_id' => $this->appId,
 		];
+
+		$res = Status::newGood();
+
+		$restrictions = $this->restrictions->toJson();
+
+		if ( strlen( $restrictions ) > self::RESTRICTIONS_MAXLENGTH ) {
+			$res->fatal( 'botpasswords-toolong-restrictions' );
+		}
+
+		$grants = FormatJson::encode( $this->grants );
+
+		if ( strlen( $grants ) > self::GRANTS_MAXLENGTH ) {
+			$res->fatal( 'botpasswords-toolong-grants' );
+		}
+
+		if ( !$res->isGood() ) {
+			return $res;
+		}
+
 		$fields = [
 			'bp_token' => MWCryptRand::generateHex( User::TOKEN_LENGTH ),
-			'bp_restrictions' => $this->restrictions->toJson(),
-			'bp_grants' => FormatJson::encode( $this->grants ),
+			'bp_restrictions' => $restrictions,
+			'bp_grants' => $grants,
 		];
 
 		if ( $password !== null ) {
@@ -302,6 +336,7 @@ class BotPassword implements IDBAccessObject {
 		}
 
 		$dbw = self::getDB( DB_MASTER );
+
 		if ( $operation === 'insert' ) {
 			$dbw->insert( 'bot_passwords', $fields + $conds, __METHOD__, [ 'IGNORE' ] );
 		} else {
@@ -313,8 +348,12 @@ class BotPassword implements IDBAccessObject {
 		if ( $ok ) {
 			$this->token = $dbw->selectField( 'bot_passwords', 'bp_token', $conds, __METHOD__ );
 			$this->isSaved = true;
+
+			return $res;
 		}
-		return $ok;
+
+		// Messages: botpasswords-insert-failed, botpasswords-update-failed
+		return Status::newFatal( "botpasswords-{$operation}-failed", $this->appId );
 	}
 
 	/**

languages/i18n/en.json

@@ -571,6 +571,8 @@
 	"botpasswords-help-grants": "Grants allow access to rights already held by your user account. Enabling a grant here does not provide access to any rights that your user account would not otherwise have. See the [[Special:ListGrants|table of grants]] for more information.",
 	"botpasswords-label-grants-column": "Granted",
 	"botpasswords-bad-appid": "The bot name \"$1\" is not valid.",
+	"botpasswords-toolong-restrictions": "There are too many IP addresses or ranges entered.",
+	"botpasswords-toolong-grants": "There are too many grants selected.",
 	"botpasswords-insert-failed": "Failed to add bot name \"$1\". Was it already added?",
 	"botpasswords-update-failed": "Failed to update bot name \"$1\". Was it deleted?",
 	"botpasswords-created-title": "Bot password created",

languages/i18n/qqq.json

@@ -790,6 +790,8 @@
 	"botpasswords-help-grants": "Help text for the grant selection checkmatrix.\n\nIdentical:\n* {{msg-mw|Mwoauth-consumer-grantshelp}}",
 	"botpasswords-label-grants-column": "Label for the checkbox column on the checkmatrix for selecting grants allowed when the bot password is used.",
 	"botpasswords-bad-appid": "Used as an error message when an invalid \"bot name\" is supplied on [[Special:BotPasswords]]. Parameters:\n* $1 - The rejected bot name.",
+	"botpasswords-toolong-restrictions": "Used when the bot password allowed IP ranges are too long when turned into JSON.",
+	"botpasswords-toolong-grants": "Error message when the selected grants are too long (usually means too many selected) when turned into JSON.",
 	"botpasswords-insert-failed": "Error message when saving a new bot password failed. It's likely that the failure was because the user resubmitted the form after a previous successful save. Parameters:\n* $1 - Bot name",
 	"botpasswords-update-failed": "Error message when saving changes to an existing bot password failed. It's likely that the failure was because the user deleted the bot password in another browser window. Parameters:\n* $1 - Bot name",
 	"botpasswords-created-title": "Title of the success page when a new bot password is created.",

maintenance/createBotPassword.php

@@ -26,7 +26,7 @@ require_once __DIR__ . '/Maintenance.php';
 
 class CreateBotPassword extends Maintenance {
 	/**
-	 * width of initial column of --showgrants output
+	 * Width of initial column of --showgrants output
 	 */
 	private const SHOWGRANTS_COLUMN_WIDTH = 20;
 
@@ -58,7 +58,7 @@ class CreateBotPassword extends Maintenance {
 	public function execute() {
 		if ( $this->hasOption( 'showgrants' ) ) {
 			$this->showGrants();
-			exit;
+			return;
 		}
 
 		$username = $this->getArg( 0 );
@@ -112,14 +112,21 @@ class CreateBotPassword extends Maintenance {
 			'grants' => $grants
 		] );
 
-		$passwordInstance = $passwordFactory->newFromPlaintext( $password );
-		$success = $bp->save( 'insert', $passwordInstance );
+		if ( $bp === null ) {
+			$this->fatalError( "Bot password creation failed." );
+		}
 
-		if ( $success ) {
+		$passwordInstance = $passwordFactory->newFromPlaintext( $password );
+		$status = $bp->save( 'insert', $passwordInstance );
+
+		if ( $status->isGood() ) {
 			$this->output( "Success.\n" );
 			$this->output( "Log in using username:'${username}@${appId}' and password:'${password}'.\n" );
 		} else {
-			$this->fatalError( "Bot password creation failed. Does this appid already exist for the user perhaps?" );
+			$this->fatalError(
+				"Bot password creation failed. Does this appid already exist for the user perhaps?\n\nErrors:\n" .
+				print_r( $status->getErrors(), true )
+			);
 		}
 	}
 

tests/phpunit/includes/user/BotPasswordTest.php

@@ -365,8 +365,9 @@ class BotPasswordTest extends MediaWikiIntegrationTestCase {
 		);
 
 		$passwordHash = $password ? $passwordFactory->newFromPlaintext( $password ) : null;
-		$this->assertFalse( $bp->save( 'update', $passwordHash ) );
-		$this->assertTrue( $bp->save( 'insert', $passwordHash ) );
+		$this->assertFalse( $bp->save( 'update', $passwordHash )->isGood() );
+		$this->assertTrue( $bp->save( 'insert', $passwordHash )->isGood() );
+
 		$bp2 = BotPassword::newFromCentralId( 42, 'TestSave', BotPassword::READ_LATEST );
 		$this->assertInstanceOf( BotPassword::class, $bp2 );
 		$this->assertEquals( $bp->getUserCentralId(), $bp2->getUserCentralId() );
@@ -374,6 +375,7 @@ class BotPasswordTest extends MediaWikiIntegrationTestCase {
 		$this->assertEquals( $bp->getToken(), $bp2->getToken() );
 		$this->assertEquals( $bp->getRestrictions(), $bp2->getRestrictions() );
 		$this->assertEquals( $bp->getGrants(), $bp2->getGrants() );
+
 		/** @var Password $pw */
 		$pw = TestingAccessWrapper::newFromObject( $bp )->getPassword();
 		if ( $password === null ) {
@@ -385,9 +387,10 @@ class BotPasswordTest extends MediaWikiIntegrationTestCase {
 		$token = $bp->getToken();
 		$this->assertEquals( 42, $bp->getUserCentralId() );
 		$this->assertEquals( 'TestSave', $bp->getAppId() );
-		$this->assertFalse( $bp->save( 'insert' ) );
-		$this->assertTrue( $bp->save( 'update' ) );
+		$this->assertFalse( $bp->save( 'insert' )->isGood() );
+		$this->assertTrue( $bp->save( 'update' )->isGood() );
 		$this->assertNotEquals( $token, $bp->getToken() );
+
 		$bp2 = BotPassword::newFromCentralId( 42, 'TestSave', BotPassword::READ_LATEST );
 		$this->assertInstanceOf( BotPassword::class, $bp2 );
 		$this->assertEquals( $bp->getToken(), $bp2->getToken() );
@@ -401,8 +404,9 @@ class BotPasswordTest extends MediaWikiIntegrationTestCase {
 
 		$passwordHash = $passwordFactory->newFromPlaintext( 'XXX' );
 		$token = $bp->getToken();
-		$this->assertTrue( $bp->save( 'update', $passwordHash ) );
+		$this->assertTrue( $bp->save( 'update', $passwordHash )->isGood() );
 		$this->assertNotEquals( $token, $bp->getToken() );
+
 		/** @var Password $pw */
 		$pw = TestingAccessWrapper::newFromObject( $bp )->getPassword();
 		$this->assertTrue( $pw->verify( 'XXX' ) );
@@ -411,7 +415,8 @@ class BotPasswordTest extends MediaWikiIntegrationTestCase {
 		$this->assertFalse( $bp->isSaved() );
 		$this->assertNull( BotPassword::newFromCentralId( 42, 'TestSave', BotPassword::READ_LATEST ) );
 
-		$this->assertFalse( $bp->save( 'foobar' ) );
+		$this->expectException( UnexpectedValueException::class );
+		$bp->save( 'foobar' )->isGood();
 	}
 
 	public static function provideSave() {
@@ -420,4 +425,47 @@ class BotPasswordTest extends MediaWikiIntegrationTestCase {
 			[ 'foobar' ],
 		];
 	}
+
+	/**
+	 * Tests for error handling when bp_restrictions and bp_grants are too long
+	 */
+	public function testSaveValidation() {
+		$lotsOfIPs = [
+			'IPAddresses' => array_fill(
+				0,
+				5000,
+				"127.0.0.0/8"
+			)
+		];
+
+		$bp = BotPassword::newUnsaved( [
+			'centralId' => 42,
+			'appId' => 'TestSave',
+			// When this becomes JSON, it'll be 70,017 characters, which is
+			// greater than BotPassword::GRANTS_MAXLENGTH, so it will cause an error.
+			'restrictions' => MWRestrictions::newFromArray( $lotsOfIPs ),
+			'grants' => [
+				// Maximum length of the JSON is BotPassword::RESTRICTIONS_MAXLENGTH characters.
+				// So one long grant name should be good. Turning it into JSON will add
+				// a couple of extra characters, taking it over BotPassword::RESTRICTIONS_MAXLENGTH
+				// characters long, so it will cause an error.
+				str_repeat( '*', BotPassword::RESTRICTIONS_MAXLENGTH )
+			],
+		] );
+
+		$status = $bp->save( 'insert' );
+
+		$this->assertFalse( $status->isGood() );
+		$this->assertNotEmpty( $status->getErrors() );
+
+		$this->assertSame(
+			'botpasswords-toolong-restrictions',
+			$status->getErrors()[0]['message']
+		);
+
+		$this->assertSame(
+			'botpasswords-toolong-grants',
+			$status->getErrors()[1]['message']
+		);
+	}
 }