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

diff --git a/RELEASE-NOTES-1.32 b/RELEASE-NOTES-1.32
index 179e970..c610722 100644 (file)
--- 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 948e1eb..34de7c6 100644 (file)
--- 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 e52945a..c832f2d 100644 (file)
--- 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 {