From cb57179a01ca0979bea7a8e32572481d81626858 Mon Sep 17 00:00:00 2001
From: Brian Wolff <bawolff+wn@gmail.com>
Date: Mon, 16 Oct 2017 05:53:30 +0000
Subject: [PATCH] Deprecate specialized file errors in OutputPage and fix
 escaping

OutputPage has a number of specialized error reporting methods
related to file handling. With exception of showFileDeleteError,
they are all unused. In my opinion such specialized error handling
does not belong in OutputPage, but in whatever class is generating
the error.

Futhermore, these functions do not appropriately escape their
arguments or their i18n messages. I replaced the one usage
in SpecialUpload with an equivalent that does escape properly.
This is not exploitable as the attacker is not in control of
the temporary file name, but it is very bad practice.

This deprecates the following methods:
* OutputPage::showFileDeleteError()
* OutputPage::showFileNotFoundError()
* OutputPage::showFileRenameError()
* OutputPage::showFileCopyError()
* OutputPage::showUnexpectedValueError()

[Discovered with the help of an experimental phan plugin]

Change-Id: I9e7aaa59ded66f32c78cfdfed1e59e073ffd5051
---
 RELEASE-NOTES-1.32                  |  5 ++++
 includes/OutputPage.php             | 36 +++++++++++++++++++++++++----
 includes/specials/SpecialUpload.php |  6 ++++-
 3 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/RELEASE-NOTES-1.32 b/RELEASE-NOTES-1.32
index 179e9700b3..c6107223d0 100644
--- a/RELEASE-NOTES-1.32
+++ b/RELEASE-NOTES-1.32
@@ -234,6 +234,11 @@ because of Phabricator reports.
 * ResourceLoaderStartUpModule::getStartupModules() and ::getLegacyModules()
   are deprecated. These concepts are obsolete and have no replacement.
 * String type for $lang of DifferenceEngine::setTextLanguage is deprecated.
+* The following methods of OutputPage are now deprecated in favour
+  of using showFatalError directly: OutputPage::showFileDeleteError()
+  OutputPage::showFileNotFoundError(), OutputPage::showFileRenameError()
+  OutputPage::showFileCopyError() and OutputPage::showUnexpectedValueError().
+
 
 === Other changes in 1.32 ===
 * …
diff --git a/includes/OutputPage.php b/includes/OutputPage.php
index 948e1ebcb6..34de7c6584 100644
--- a/includes/OutputPage.php
+++ b/includes/OutputPage.php
@@ -2677,30 +2677,56 @@ class OutputPage extends ContextSource {
 		}
 	}
 
+	/**
+	 * Output an error page
+	 *
+	 * @note FatalError exception class provides an alternative.
+	 * @param string $message Error to output. Must be escaped for HTML.
+	 */
 	public function showFatalError( $message ) {
 		$this->prepareErrorPage( $this->msg( 'internalerror' ) );
 
 		$this->addHTML( $message );
 	}
 
+	/**
+	 * @deprecated 1.32 Use OutputPage::showFatalError or throw FatalError instead.
+	 */
 	public function showUnexpectedValueError( $name, $val ) {
-		$this->showFatalError( $this->msg( 'unexpected', $name, $val )->text() );
+		wfDeprecated( __METHOD__, '1.32' );
+		$this->showFatalError( $this->msg( 'unexpected', $name, $val )->escaped() );
 	}
 
+	/**
+	 * @deprecated 1.32 Use OutputPage::showFatalError or throw FatalError instead.
+	 */
 	public function showFileCopyError( $old, $new ) {
-		$this->showFatalError( $this->msg( 'filecopyerror', $old, $new )->text() );
+		wfDeprecated( __METHOD__, '1.32' );
+		$this->showFatalError( $this->msg( 'filecopyerror', $old, $new )->escaped() );
 	}
 
+	/**
+	 * @deprecated 1.32 Use OutputPage::showFatalError or throw FatalError instead.
+	 */
 	public function showFileRenameError( $old, $new ) {
-		$this->showFatalError( $this->msg( 'filerenameerror', $old, $new )->text() );
+		wfDeprecated( __METHOD__, '1.32' );
+		$this->showFatalError( $this->msg( 'filerenameerror', $old, $new )->escpaed() );
 	}
 
+	/**
+	 * @deprecated 1.32 Use OutputPage::showFatalError or throw FatalError instead.
+	 */
 	public function showFileDeleteError( $name ) {
-		$this->showFatalError( $this->msg( 'filedeleteerror', $name )->text() );
+		wfDeprecated( __METHOD__, '1.32' );
+		$this->showFatalError( $this->msg( 'filedeleteerror', $name )->escaped() );
 	}
 
+	/**
+	 * @deprecated 1.32 Use OutputPage::showFatalError or throw FatalError instead.
+	 */
 	public function showFileNotFoundError( $name ) {
-		$this->showFatalError( $this->msg( 'filenotfound', $name )->text() );
+		wfDeprecated( __METHOD__, '1.32' );
+		$this->showFatalError( $this->msg( 'filenotfound', $name )->escaped() );
 	}
 
 	/**
diff --git a/includes/specials/SpecialUpload.php b/includes/specials/SpecialUpload.php
index e52945abac..c832f2d841 100644
--- a/includes/specials/SpecialUpload.php
+++ b/includes/specials/SpecialUpload.php
@@ -761,7 +761,11 @@ class SpecialUpload extends SpecialPage {
 		}
 		$success = $this->mUpload->unsaveUploadedFile();
 		if ( !$success ) {
-			$this->getOutput()->showFileDeleteError( $this->mUpload->getTempPath() );
+			$this->getOutput()->showFatalError(
+				$this->msg( 'filedeleteerror' )
+					->params( $this->mUpload->getTempPath() )
+					->escaped()
+			);
 
 			return false;
 		} else {
-- 
2.20.1