From: Neil Kandalgaonkar <neilk@users.mediawiki.org>
Date: Tue, 16 Nov 2010 00:24:37 +0000 (+0000)
Subject: Followups mostly to r75906.
X-Git-Tag: 1.31.0-rc.0~33872
X-Git-Url: http://git.cyclocoop.org/%24action?a=commitdiff_plain;h=467db32fa1e97acf52092157ddf28c25c49fc270;p=lhc%2Fweb%2Fwiklou.git

Followups mostly to r75906.

* Improved logic around error messages, and understandability of those messages.

* Removed FIXME on Content-Length header. When we serve the file
this way, it seems to be necessary to explicitly set the Content-Length. I tried
removing it and unsurprisingly it did not appear.

I am unsure what RoanKattouw is referring to when he says PHP can do this.
How can PHP know what the Content-Length is going to be, unless we set this explicitly?
---

diff --git a/includes/specials/SpecialUploadStash.php b/includes/specials/SpecialUploadStash.php
index 983b846262..d6ed933e55 100644
--- a/includes/specials/SpecialUploadStash.php
+++ b/includes/specials/SpecialUploadStash.php
@@ -20,11 +20,16 @@ class SpecialUploadStash extends UnlistedSpecialPage {
 	// UploadStash
 	private $stash;
 
-	// we should not be reading in really big files and serving them out
-	private $maxServeFileSize = 262144; // 256K
+	// Since we are directly writing the file to STDOUT, 
+	// we should not be reading in really big files and serving them out.
+	//
+	// We also don't want people using this as a file drop, even if they
+	// share credentials.
+	//
+	// This service is really for thumbnails and other such previews while
+	// uploading.
+	const MAX_SERVE_BYTES = 262144; // 256K
 
-	// $request is the request (usually wgRequest)
-	// $subpage is everything in the URL after Special:UploadStash
 	public function __construct( ) {
 		parent::__construct( 'UploadStash', 'upload' );
 		try {
@@ -52,23 +57,43 @@ class SpecialUploadStash extends UnlistedSpecialPage {
 		// prevent callers from doing standard HTML output -- we'll take it from here
 		$wgOut->disable();
 
-		try {
-			$file = $this->getStashFile( $subPage );
-			if ( $file->getSize() > $this->maxServeFileSize ) {
-				throw new MWException( 'file size too large' );
+		$code = 500;
+		$message = 'Unknown error';
+
+		if ( !isset( $subPage ) or $subPage === '' ) {
+			// the user probably visited the page just to see what would happen, so explain it a bit.
+			$code = '400';
+			$message = "Missing key\n\n" 
+				   . 'This page provides access to temporarily stashed files for the user that '
+				   . 'uploaded those files. See the upload API documentation. To access a stashed file, '
+				   . 'use the URL of this page, with a slash and the key of the stashed file appended.';
+		} else {
+			try {
+				$file = $this->getStashFile( $subPage );
+				$size = $file->getSize();
+				if ( $size === 0 ) {
+					$code = 500;
+					$message = 'File is zero length';
+				} else if ( $size > self::MAX_SERVE_BYTES ) {
+					$code = 500;
+					$message = 'Cannot serve a file larger than ' . self::MAX_SERVE_BYTES . ' bytes';
+				} else {
+					$this->outputFile( $file );
+					return true;
+				}
+			} catch( UploadStashFileNotFoundException $e ) {
+				$code = 404; 
+				$message = $e->getMessage();
+			} catch( UploadStashBadPathException $e ) {
+				$code = 500;
+				$message = $e->getMessage();
+			} catch( Exception $e ) {
+				$code = 500;
+				$message = $e->getMessage();
 			}
-			$this->outputFile( $file );
-			return true;
-
-		} catch( UploadStashFileNotFoundException $e ) {
-			$code = 404;
-		} catch( UploadStashBadPathException $e ) {
-			$code = 403;
-		} catch( Exception $e ) {
-			$code = 500;
 		}
 
-		wfHttpError( $code, OutputPage::getStatusMessage( $code ), $e->getMessage() );
+		wfHttpError( $code, OutputPage::getStatusMessage( $code ), $message );
 		return false;
 	}
 
@@ -130,8 +155,7 @@ class SpecialUploadStash extends UnlistedSpecialPage {
 		header( 'Content-Type: ' . $file->getMimeType(), true );
 		header( 'Content-Transfer-Encoding: binary', true );
 		header( 'Expires: Sun, 17-Jan-2038 19:14:07 GMT', true );
-		header( 'Pragma: public', true );
-		header( 'Content-Length: ' . $file->getSize(), true ); // FIXME: PHP can handle Content-Length for you just fine --RK
+		header( 'Content-Length: ' . $file->getSize(), true ); 
 		readfile( $file->getPath() );
 	}
 }