jenkins-bot has submitted this change and it was merged. ( https://gerrit.wikimedia.org/r/390136 )
Change subject: Verify string length before inserting into the database ...................................................................... Verify string length before inserting into the database Depending on configuration settings, the DB might quietly truncate long values. Failing loudly is preferable. Change-Id: Ia0bc3c11c30e60db96fba7bc001e34ec03b79273 --- M i18n/en.json M i18n/qqq.json M sql/readinglists.sql M src/ReadingListRepository.php M tests/src/ReadingListRepositoryTest.php 5 files changed, 47 insertions(+), 0 deletions(-) Approvals: jenkins-bot: Verified Mholloway: Looks good to me, approved diff --git a/i18n/en.json b/i18n/en.json index f8ee29d..afa13e0 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -23,6 +23,7 @@ "readinglists-db-error-user-required": "This method cannot be called without specifying the user.", "readinglists-db-error-list-limit": "Users cannot have more than $1 lists.", "readinglists-db-error-entry-limit": "List $1 cannot have more than $2 entries.", + "readinglists-db-error-too-long": "Value for field $1 cannot be longer than $2 bytes.", "readinglists-apierror-project-title-param": "<var>$1project</var> and <var>$1title</var> must be used together.", "readinglists-apierror-too-old": "Timestamps passed to <var>$1changedsince</var> cannot be older than <kbd>$2</kbd>.", "apihelp-query+readinglists-summary": "List or filter the user's reading lists and show metadata about them.", diff --git a/i18n/qqq.json b/i18n/qqq.json index 6c00bd5..8414fa4 100644 --- a/i18n/qqq.json +++ b/i18n/qqq.json @@ -24,6 +24,7 @@ "readinglists-db-error-user-required": "Error message used when calling a method that operates on a single user, but the user was not specified when the repository object was constructed.", "readinglists-db-error-list-limit": "Error message used when the user has as many or more lists than the maximum allowed, and tries to add another one.\n\nParameters:\n* $1 - the maximum allowed number of lists per user.", "readinglists-db-error-entry-limit": "Error message used when the user has as many or more entries in the given list than the maximum allowed, and tries to add another one.\n\nParameters:\n* $1 - the ID of the list in question.\n$2 - the maximum allowed number of entries per list.", + "readinglists-db-error-too-long": "Error message used when the user tries to set list/entry string fields to a longer value than what the database schema allows.\n\nParameters:\n* $1 - DB field name.\n$2 - DB field maximum length (in bytes).", "readinglists-apierror-project-title-param": "{{doc-apierror}}\n$1 is the module prefix.", "readinglists-apierror-too-old": "{{doc-apierror}}\n$1 is the module prefix, $2 is the expiry date for deleted lists/entries.", "apihelp-query+readinglists-summary": "{{doc-apihelp-summary|query+readinglists}}", diff --git a/sql/readinglists.sql b/sql/readinglists.sql index 8ce44b7..03285e7 100644 --- a/sql/readinglists.sql +++ b/sql/readinglists.sql @@ -1,3 +1,6 @@ +-- On the application level, column length limits are enforced via +-- ReadingListRepository::$fieldLength which should be kept in sync. + -- Lists. CREATE TABLE /*_*/reading_list ( rl_id INTEGER UNSIGNED NOT NULL PRIMARY KEY AUTO_INCREMENT, diff --git a/src/ReadingListRepository.php b/src/ReadingListRepository.php index a2dc7b2..dcf499b 100644 --- a/src/ReadingListRepository.php +++ b/src/ReadingListRepository.php @@ -32,6 +32,17 @@ */ class ReadingListRepository implements IDBAccessObject, LoggerAwareInterface { + /** @var array Database field lengths in bytes (only for the string types). */ + public static $fieldLength = [ + 'rl_name' => 255, + 'rl_description' => 767, + 'rl_color' => 6, + 'rl_image' => 255, + 'rl_icon' => 32, + 'rle_project' => 255, + 'rle_title' => 255, + ]; + /** @var int|null Max allowed lists per user */ private $listLimit; @@ -191,6 +202,11 @@ */ public function addList( $name, $description = '', $color = '', $image = '', $icon = '' ) { $this->assertUser(); + $this->assertFieldLength( 'rl_name', $name ); + $this->assertFieldLength( 'rl_description', $description ); + $this->assertFieldLength( 'rl_color', $color ); + $this->assertFieldLength( 'rl_image', $image ); + $this->assertFieldLength( 'rl_icon', $icon ); if ( !$this->isSetupForUser( self::READ_LOCKING ) ) { throw new ReadingListRepositoryException( 'readinglists-db-error-not-set-up' ); } @@ -274,6 +290,11 @@ $id, $name = null, $description = null, $color = null, $image = null, $icon = null ) { $this->assertUser(); + $this->assertFieldLength( 'rl_name', $name ); + $this->assertFieldLength( 'rl_description', $description ); + $this->assertFieldLength( 'rl_color', $color ); + $this->assertFieldLength( 'rl_image', $image ); + $this->assertFieldLength( 'rl_icon', $icon ); $this->selectValidList( $id, self::READ_LOCKING ); $data = array_filter( [ @@ -340,6 +361,8 @@ */ public function addListEntry( $id, $project, $title ) { $this->assertUser(); + $this->assertFieldLength( 'rle_project', $project ); + $this->assertFieldLength( 'rle_title', $title ); $this->selectValidList( $id, self::READ_LOCKING ); if ( $this->entryLimit && $this->getEntryCount( $id, self::READ_LATEST ) >= $this->entryLimit ) { throw new ReadingListRepositoryException( 'readinglists-db-error-entry-limit', @@ -999,6 +1022,21 @@ } /** + * Ensures that the value to be written to the database does not exceed the DB field length. + * @param string $field Field name. + * @param string $value Value to write. + * @throws ReadingListRepositoryException + */ + private function assertFieldLength( $field, $value ) { + if ( !isset( self::$fieldLength[$field] ) ) { + throw new LogicException( 'Tried to assert length for invalid field ' . $field ); + } + if ( strlen( $value ) > self::$fieldLength[$field] ) { + throw new ReadingListRepositoryException( 'readinglists-db-error-too-long', + [ $field, self::$fieldLength[$field] ] ); + } + } + /** * Get list data, and optionally lock the list. * List must exist, belong to the current user and not be deleted. * @param int $id List id diff --git a/tests/src/ReadingListRepositoryTest.php b/tests/src/ReadingListRepositoryTest.php index d2e8ef6..e6d4e12 100644 --- a/tests/src/ReadingListRepositoryTest.php +++ b/tests/src/ReadingListRepositoryTest.php @@ -178,6 +178,10 @@ 'rl_is_default' => '0', 'rl_deleted' => '0', ], $data ); + + $this->assertFailsWith( 'readinglists-db-error-too-long', function () use ( $repository ) { + $repository->addList( 'boom', str_pad( '', 1000, 'x' ) ); + } ); } // @codingStandardsIgnoreLine MediaWiki.NamingConventions.LowerCamelFunctionsName.FunctionName -- To view, visit https://gerrit.wikimedia.org/r/390136 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ia0bc3c11c30e60db96fba7bc001e34ec03b79273 Gerrit-PatchSet: 3 Gerrit-Project: mediawiki/extensions/ReadingLists Gerrit-Branch: master Gerrit-Owner: Gergő Tisza <gti...@wikimedia.org> Gerrit-Reviewer: BearND <bsitzm...@wikimedia.org> Gerrit-Reviewer: Gergő Tisza <gti...@wikimedia.org> Gerrit-Reviewer: Mholloway <mhollo...@wikimedia.org> Gerrit-Reviewer: Siebrand <siebr...@kitano.nl> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits