r61355 MediaWiki - Code Review archive

+			'enablechunks' => null,
+			'chunksessionkey' => null,
+			'chunk' => null,
+			'done' => false,
...
+			'enablechunks' => 'Set to use chunk mode; see [http://firefogg.org/dev/chunk_post.html http://firefogg.org/dev/chunk_post.html] for protocol',

You're adding four parameters but documenting only one of them; please document the rest as well. Also, enablechunks is a boolean parameter and should be declared with => false rather than => null, like the done parameter. If the chunk parameter is a boolean in nature (can't tell offhand), this applies to that one too.

Also, it seems like one or more of the chunk-related parameters may be required in chunk mode, in which case their presence should be checked.

-	'curlHttpRequest' => 'includes/HttpFunctions.php',
-	'phpHttpRequest' => 'includes/HttpFunctions.php',
-	'simpleFileWriter' => 'includes/HttpFunctions.php',

These changes seem unrelated, although if they really are classes that are only referenced inside HttpFunctions.php, they should indeed be removed; they were also out of alphabetical order.

I don't really like how you give a $request as parameter to initialize() instead of $done, $filename and $chunkSessionkey. This is more flexible for potential future uses.

UploadBase::initialize is exposed purposely to set mandatory variables.

In general I think we need to look critically to how initialization of the Upload objects is done before 1.16 is released. I'm in general not entirely convinced that the current way is the proper way to do it.

It needs more than just $done, $filename, and $chunkSessionKey, but ok.

+			exit( 0 );

Won't that skip all MediaWiki::restInPeace stuff?

Yes. I think $wgOut->disable(); is appropriate here.

Thanks, I basically just copied that and it did look ugly. I'll try to apply your advice.

Sending the editToken to the user expecting him to give it back in the url? I would use a specific token here.

Whole file review. In UploadFromChunks.php:

if ( !$sessionKey || empty( $sessionKey ) ) {

This is nonsensical since the two parts here do the same thing, just one of them has errors suppressed and one doesn't. Also it will fail once every 2^31 requests since $sessionKey will be 0. Suggest changing the data source from $request->getText() to $request->getVal() and using $sessionKey === null, and documenting this behaviour.

$this->status = Status::newFromFatal( 'Missing session data.' );

Wrong parameters again, same as HttpFunctions.php. One more instance. These error cases all need to be tested.

wfDebug( "\n\n\performUpload(chunked): sum:" . $comment . ' c: ' . $pageText . ' w:' . $watch );

Incomprehensible debug log entry. The debug log is there to help both sysadmins and developers, not the one guy on the planet who knows what "c" stands for.

			ob_clean();
			echo FormatJson::encode( array(
				'uploadUrl' => wfExpandUrl( wfScript( 'api' ) ) . "?action=upload&" .
				"token={$token}&format=json&enablechunks=true&chunksessionkey=" .
				$this->setupChunkSession( $comment, $pageText, $watch ) ) );
			$wgOut->disable();

It's not appropriate to be echoing things here, and formatting responses. Data should be made available to the API caller, which can handle JSON formatting and output buffering using the standard API interface. If Firefogg cannot understand normal API responses, then the api.php entry point should not be used. The URL should be constructed using wfArrayToCGI().

			return $status;
		} else {

I think it's better to save an indenting level by removing the "else".

			if ( $comment == '' )
				$comment = $this->comment;

			if ( $pageText == '' )
				$pageText = $this->pageText;

I'm not sure if this is intentionally suppressing empty comments or if it's meant to be $comment === null.

In ApiUpload.php

				if ( !$this->mUpload->status->isOK() ) {
					return $this->dieUsageMsg( $this->mUpload->status->getWikiText(),
						'chunked-error' );

dieUsageMsg() only accepts a single parameter and it's not either of those.

		// Finish up the exec command:
		$this->doExecUpload();
	}

	protected function doExecUpload() {

Pointless function split. Suggest recombining them.

		// FIXME: This should be in a try .. finally block with performUpload

PHP does not have "finally" like in Java, maybe Michael meant this figuratively. But nothing in performUpload() should be throwing exceptions on any normal error condition, so I'm not sure what the point in catching them would be.

		$verification = $this->mUpload->verifyUpload();

It's certainly wrong to be running verifyUpload() on a chunk. It only makes sense to run it on the whole file. Since UploadFromChunks sets $this->mTempPath to the path of the current chunk, not the path of the temporary file like what UploadFromStash does, the full file is never checked and this is an exploitable XSS vulnerability. All you have to do is straddle a script across a chunk boundary.

		if ( !$this->mParams['ignorewarnings'] ) {
			$warnings = $this->mUpload->checkWarnings();

It doesn't make sense to call checkWarnings() for every chunk either. This does things like computing the SHA-1 hash of the file and checking if there is a duplicate already uploaded.

		// Append imageinfo to the result
		$imParam = ApiQueryImageInfo::getPropertyNames();
		$result['imageinfo'] = ApiQueryImageInfo::getInfo( $file,
				array_flip( $imParam ), $this->getResult() );

Yet another thing that doesn't make sense for chunk upload.

Additional comments about dieUsageMsg():

• The single parameter accepted is array( 'messagekey', param1, param2, ... ). You can grab an array of such arrays with $status->getErrorsArray() and use the first element of that array, provided the function setting the error in $status passes errors as a message key with params and doesn't try to wfMsg() the error itself.
• dieUsage() has two mandatory parameters, the error text and the error code. This seems to be what you intended, but since the upload backend gives you a Status object you should use dieUsageMsg() instead: this is clearer to the client than passing non-machine-readable wikitext with a 'chunked-error' code.
• dieUsageMsg() relies on ApiBase::$messageMap to translate these messagekey-params arrays to API errors. Check that the message keys returned from the backend functions are present in the message map and add any missing ones. Missing message keys aren't the end of the world: they won't crash your code, but they'll result in 'unknownerror' errors.
• dieUsage() and dieUsageMsg() are functions that never return (much like exit()), so don't use return $this->dieUsageMsg( ... );

Also, test error cases if you're not already doing that. The broken usage of dieUsageMsg() strongly suggests that that error case wasn't tested.

see r62291 for all relevant fixes

err... not all, but the changeset that makes uploadchunks actually work against firefogg

see r62806 for the rest of the fixes.