From 7e3615413632fbe97bb1ecef974a42ab239fe25e Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Mon, 2 Mar 2015 16:27:17 -0500 Subject: [PATCH] API: Improve validation in chunked uploading This fixes a few shortcomings in the chunked uploader: * Raises an error if offset + chunksize > filesize. * Enforces a minimum chunk size for non-final chunks. * Refuses additional chunks after seeing a final chunk. * Status of a chunked upload in progress is now available with 'checkstatus'. Bug: T91203 Bug: T91205 Change-Id: I36a571864ef4d5481df7e9867bd4750bb24c89eb --- RELEASE-NOTES-1.25 | 6 +++ includes/DefaultSettings.php | 8 ++++ includes/GlobalFunctions.php | 5 ++- includes/Setup.php | 9 +++++ includes/api/ApiQuerySiteinfo.php | 1 + includes/api/ApiUpload.php | 83 ++++++++++++++++++++++++++++++++------- 6 files changed, 96 insertions(+), 16 deletions(-) diff --git a/RELEASE-NOTES-1.25 b/RELEASE-NOTES-1.25 index 45e669a..901708f 100644 --- a/RELEASE-NOTES-1.25 +++ b/RELEASE-NOTES-1.25 @@ -45,6 +45,8 @@ production. * The 'daemonized' flag must be set to true in $wgJobTypeConf for any redis job queues. This means that mediawiki/services/jobrunner service has to be installed and running for any such queues to work. +* Added $wgMinUploadChunkSize to restrict the size of chunks in chunked file + uploading. === New features in 1.25 === * (T64861) Updated plural rules to CLDR 26. Includes incompatible changes @@ -228,6 +230,10 @@ production. Title::userCan() via the API. * Default type param for query list=watchlist and list=recentchanges has been changed from all types (e.g. including 'external') to 'edit|new|log'. +* action=upload will enforce a minimum chunk size (see $wgMinUploadChunkSize). +* action=upload will refuse chunks that would make the file larger than the + stated filesize, or any additional chunks after the stated size is reached. +* action=upload&checkstatus will now return status for chunked uploads in progress. === Action API internal changes in 1.25 === * ApiHelp has been rewritten to support i18n and paginated HTML output. diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index 6786f81..71b47d1 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -674,6 +674,14 @@ $wgCopyUploadAsyncTimeout = false; $wgMaxUploadSize = 1024 * 1024 * 100; # 100MB /** + * Minimum upload chunk size, in bytes. When using chunked upload, non-final + * chunks smaller than this will be rejected. May be reduced based on the + * 'upload_max_filesize' or 'post_max_size' PHP settings. + * @since 1.25 + */ +$wgMinUploadChunkSize = 1024 * 1024; # 1MB + +/** * Point the upload navigation link to an external URL * Useful if you want to use a shared repository by default * without disabling local uploads (use $wgEnableUploads = false for that). diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php index 5232413..eeb28d6 100644 --- a/includes/GlobalFunctions.php +++ b/includes/GlobalFunctions.php @@ -3913,12 +3913,13 @@ function wfMemoryLimit() { * Converts shorthand byte notation to integer form * * @param string $string + * @param int $default Returned if $string is empty * @return int */ -function wfShorthandToInteger( $string = '' ) { +function wfShorthandToInteger( $string = '', $default = -1 ) { $string = trim( $string ); if ( $string === '' ) { - return -1; + return $default; } $last = $string[strlen( $string ) - 1]; $val = intval( $string ); diff --git a/includes/Setup.php b/includes/Setup.php index 6939f95..b661224 100644 --- a/includes/Setup.php +++ b/includes/Setup.php @@ -368,6 +368,15 @@ if ( $wgResourceLoaderMaxQueryLength === false ) { $wgResourceLoaderMaxQueryLength = $maxValueLength > 0 ? $maxValueLength : -1; } +// Ensure the minimum chunk size is less than PHP upload limits or the maximum +// upload size. +$wgMinUploadChunkSize = min( + $wgMinUploadChunkSize, + $wgMaxUploadSize, + wfShorthandToInteger( ini_get( 'upload_max_filesize' ), 1e100 ), + wfShorthandToInteger( ini_get( 'post_max_size' ), 1e100) - 1024 # Leave room for other parameters +); + /** * Definitions of the NS_ constants are in Defines.php * @private diff --git a/includes/api/ApiQuerySiteinfo.php b/includes/api/ApiQuerySiteinfo.php index 5ac1036..e83d7b3 100644 --- a/includes/api/ApiQuerySiteinfo.php +++ b/includes/api/ApiQuerySiteinfo.php @@ -261,6 +261,7 @@ class ApiQuerySiteinfo extends ApiQueryBase { } $data['maxuploadsize'] = UploadBase::getMaxUploadSize(); + $data['minuploadchunksize'] = (int)$this->getConfig()->get( 'MinUploadChunkSize' ); $data['thumblimits'] = $config->get( 'ThumbLimits' ); $this->getResult()->setIndexedTagName( $data['thumblimits'], 'limit' ); diff --git a/includes/api/ApiUpload.php b/includes/api/ApiUpload.php index 78a4971..6f9355a 100644 --- a/includes/api/ApiUpload.php +++ b/includes/api/ApiUpload.php @@ -81,7 +81,7 @@ class ApiUpload extends ApiBase { // Check if the uploaded file is sane if ( $this->mParams['chunk'] ) { - $maxSize = $this->mUpload->getMaxUploadSize(); + $maxSize = UploadBase::getMaxUploadSize(); if ( $this->mParams['filesize'] > $maxSize ) { $this->dieUsage( 'The file you submitted was too large', 'file-too-large' ); } @@ -197,13 +197,30 @@ class ApiUpload extends ApiBase { private function getChunkResult( $warnings ) { $result = array(); - $result['result'] = 'Continue'; if ( $warnings && count( $warnings ) > 0 ) { $result['warnings'] = $warnings; } + $request = $this->getMain()->getRequest(); $chunkPath = $request->getFileTempname( 'chunk' ); $chunkSize = $request->getUpload( 'chunk' )->getSize(); + $totalSoFar = $this->mParams['offset'] + $chunkSize; + $minChunkSize = $this->getConfig()->get( 'MinUploadChunkSize' ); + + // Sanity check sizing + if ( $totalSoFar > $this->mParams['filesize'] ) { + $this->dieUsage( + 'Offset plus current chunk is greater than claimed file size', 'invalid-chunk' + ); + } + + // Enforce minimum chunk size + if ( $totalSoFar != $this->mParams['filesize'] && $chunkSize < $minChunkSize ) { + $this->dieUsage( + "Minimum chunk size is $minChunkSize bytes for non-final chunks", 'chunk-too-small' + ); + } + if ( $this->mParams['offset'] == 0 ) { try { $filekey = $this->performStash(); @@ -215,6 +232,18 @@ class ApiUpload extends ApiBase { } } else { $filekey = $this->mParams['filekey']; + + // Don't allow further uploads to an already-completed session + $progress = UploadBase::getSessionStatus( $this->getUser(), $filekey ); + if ( !$progress ) { + // Probably can't get here, but check anyway just in case + $this->dieUsage( 'No chunked upload session with this key', 'stashfailed' ); + } elseif ( $progress['result'] !== 'Continue' || $progress['stage'] !== 'uploading' ) { + $this->dieUsage( + 'Chunked upload is already completed, check status for details', 'stashfailed' + ); + } + $status = $this->mUpload->addChunk( $chunkPath, $chunkSize, $this->mParams['offset'] ); if ( !$status->isGood() ) { @@ -223,18 +252,12 @@ class ApiUpload extends ApiBase { ); $this->dieUsage( $status->getWikiText(), 'stashfailed', 0, $extradata ); - - return array(); } } // Check we added the last chunk: - if ( $this->mParams['offset'] + $chunkSize == $this->mParams['filesize'] ) { + if ( $totalSoFar == $this->mParams['filesize'] ) { if ( $this->mParams['async'] ) { - $progress = UploadBase::getSessionStatus( $this->getUser(), $filekey ); - if ( $progress && $progress['result'] === 'Poll' ) { - $this->dieUsage( "Chunk assembly already in progress.", 'stashfailed' ); - } UploadBase::setSessionStatus( $this->getUser(), $filekey, @@ -254,21 +277,38 @@ class ApiUpload extends ApiBase { } else { $status = $this->mUpload->concatenateChunks(); if ( !$status->isGood() ) { + UploadBase::setSessionStatus( + $this->getUser(), + $filekey, + array( 'result' => 'Failure', 'stage' => 'assembling', 'status' => $status ) + ); $this->dieUsage( $status->getWikiText(), 'stashfailed' ); - - return array(); } // The fully concatenated file has a new filekey. So remove // the old filekey and fetch the new one. + UploadBase::setSessionStatus( $this->getUser(), $filekey, false ); $this->mUpload->stash->removeFile( $filekey ); $filekey = $this->mUpload->getLocalFile()->getFileKey(); $result['result'] = 'Success'; } + } else { + UploadBase::setSessionStatus( + $this->getUser(), + $filekey, + array( + 'result' => 'Continue', + 'stage' => 'uploading', + 'offset' => $totalSoFar, + 'status' => Status::newGood(), + ) + ); + $result['result'] = 'Continue'; + $result['offset'] = $totalSoFar; } + $result['filekey'] = $filekey; - $result['offset'] = $this->mParams['offset'] + $chunkSize; return $result; } @@ -378,6 +418,10 @@ class ApiUpload extends ApiBase { // Chunk upload $this->mUpload = new UploadFromChunks(); if ( isset( $this->mParams['filekey'] ) ) { + if ( $this->mParams['offset'] === 0 ) { + $this->dieUsage( 'Cannot supply a filekey when offset is 0', 'badparams' ); + } + // handle new chunk $this->mUpload->continueChunks( $this->mParams['filename'], @@ -385,6 +429,10 @@ class ApiUpload extends ApiBase { $request->getUpload( 'chunk' ) ); } else { + if ( $this->mParams['offset'] !== 0 ) { + $this->dieUsage( 'Must supply a filekey when offset is non-zero', 'badparams' ); + } + // handle first chunk $this->mUpload->initialize( $this->mParams['filename'], @@ -760,8 +808,15 @@ class ApiUpload extends ApiBase { ), 'stash' => false, - 'filesize' => null, - 'offset' => null, + 'filesize' => array( + ApiBase::PARAM_TYPE => 'integer', + ApiBase::PARAM_MIN => 0, + ApiBase::PARAM_MAX => UploadBase::getMaxUploadSize(), + ), + 'offset' => array( + ApiBase::PARAM_TYPE => 'integer', + ApiBase::PARAM_MIN => 0, + ), 'chunk' => array( ApiBase::PARAM_TYPE => 'upload', ), -- 2.1.4