Start working on phan-taint-check warnings. Fix minor escaping issues.

This fixes 26 of the phan-taint-check warnings on MW core. Some
are outright fixed, others are false positives that were suppressed.

This really only covers some of the easy ones. There are still
314 warnings to go.

Change-Id: I30463bc3a09fd4324d190de8533f51784764dd3a

diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php
index 543978b..3ea020f 100644 (file)
--- a/includes/GlobalFunctions.php
+++ b/includes/GlobalFunctions.php
@@ -1628,6 +1628,7 @@ function wfClientAcceptsGzip( $force = false ) {
  * As required by the callers, "<nowiki>" is not used.
  *
  * @param string $text Text to be escaped
+ * @param-taint $text escapes_html
  * @return string
  */
 function wfEscapeWikiText( $text ) {

diff --git a/includes/Message.php b/includes/Message.php
index a499035..5ca0942 100644 (file)
--- a/includes/Message.php
+++ b/includes/Message.php
@@ -837,6 +837,7 @@ class Message implements MessageSpecifier, Serializable {
         *   the last time (this is for B/C and should be avoided).
         *
         * @return string HTML
+        * @suppress SecurityCheck-DoubleEscaped phan false positive
         */
        public function toString( $format = null ) {
                if ( $format === null ) {

diff --git a/includes/TemplateParser.php b/includes/TemplateParser.php
index 9271e3f..dc7d31e 100644 (file)
--- a/includes/TemplateParser.php
+++ b/includes/TemplateParser.php
@@ -209,8 +209,11 @@ class TemplateParser {
         *     );
         * @endcode
         * @param string $templateName The name of the template
+        * @param-taint $templateName exec_misc
         * @param mixed $args
+        * @param-taint $args none
         * @param array $scopes
+        * @param-taint $scopes none
         * @return string
         */
        public function processTemplate( $templateName, $args, array $scopes = [] ) {

diff --git a/includes/actions/HistoryAction.php b/includes/actions/HistoryAction.php
index 8ea50ec..bd64a41 100644 (file)
--- a/includes/actions/HistoryAction.php
+++ b/includes/actions/HistoryAction.php
@@ -836,7 +836,7 @@ class HistoryPager extends ReverseChronologicalPager {
                } else {
                        return MediaWikiServices::getInstance()->getLinkRenderer()->makeKnownLink(
                                $this->getTitle(),
-                               $cur,
+                               new HtmlArmor( $cur ),
                                [],
                                [
                                        'diff' => $this->getWikiPage()->getLatest(),
@@ -868,7 +868,7 @@ class HistoryPager extends ReverseChronologicalPager {
                        # Next row probably exists but is unknown, use an oldid=prev link
                        return $linkRenderer->makeKnownLink(
                                $this->getTitle(),
-                               $last,
+                               new HtmlArmor( $last ),
                                [],
                                [
                                        'diff' => $prevRev->getId(),
@@ -887,7 +887,7 @@ class HistoryPager extends ReverseChronologicalPager {
 
                return $linkRenderer->makeKnownLink(
                        $this->getTitle(),
-                       $last,
+                       new HtmlArmor( $last ),
                        [],
                        [
                                'diff' => $prevRev->getId(),

diff --git a/includes/changes/EnhancedChangesList.php b/includes/changes/EnhancedChangesList.php
index a593d27..cdfbf56 100644 (file)
--- a/includes/changes/EnhancedChangesList.php
+++ b/includes/changes/EnhancedChangesList.php
@@ -409,14 +409,14 @@ class EnhancedChangesList extends ChangesList {
 
                # Log timestamp
                if ( $type == RC_LOG ) {
-                       $link = $rcObj->timestamp;
+                       $link = htmlspecialchars( $rcObj->timestamp );
                        # Revision link
                } elseif ( !ChangesList::userCan( $rcObj, Revision::DELETED_TEXT, $this->getUser() ) ) {
-                       $link = '<span class="history-deleted">' . $rcObj->timestamp . '</span> ';
+                       $link = Html::element( 'span', [ 'class' => 'history-deleted' ], $rcObj->timestamp );
                } else {
                        $link = $this->linkRenderer->makeKnownLink(
                                $rcObj->getTitle(),
-                               new HtmlArmor( $rcObj->timestamp ),
+                               $rcObj->timestamp,
                                [],
                                $params
                        );
@@ -642,7 +642,7 @@ class EnhancedChangesList extends ChangesList {
                ];
                // timestamp is not really a link here, but is called timestampLink
                // for consistency with EnhancedChangesListModifyLineData
-               $data['timestampLink'] = $rcObj->timestamp;
+               $data['timestampLink'] = htmlspecialchars( $rcObj->timestamp );
 
                # Article or log link
                if ( $logType ) {

diff --git a/includes/installer/DatabaseUpdater.php b/includes/installer/DatabaseUpdater.php
index bf140ea..e49a846 100644 (file)
--- a/includes/installer/DatabaseUpdater.php
+++ b/includes/installer/DatabaseUpdater.php
@@ -208,6 +208,7 @@ abstract class DatabaseUpdater {
         * Output some text. If we're running from web, escape the text first.
         *
         * @param string $str Text to output
+        * @param-taint $str escapes_html
         */
        public function output( $str ) {
                if ( $this->maintenance->isQuiet() ) {

diff --git a/includes/installer/WebInstaller.php b/includes/installer/WebInstaller.php
index 2ba221f..739d9f2 100644 (file)
--- a/includes/installer/WebInstaller.php
+++ b/includes/installer/WebInstaller.php
@@ -166,18 +166,7 @@ class WebInstaller extends Installer {
                if ( ( $this->getVar( '_InstallDone' ) || $this->getVar( '_UpgradeDone' ) )
                        && $this->request->getVal( 'localsettings' )
                ) {
-                       $this->request->response()->header( 'Content-type: application/x-httpd-php' );
-                       $this->request->response()->header(
-                               'Content-Disposition: attachment; filename="LocalSettings.php"'
-                       );
-
-                       $ls = InstallerOverrides::getLocalSettingsGenerator( $this );
-                       $rightsProfile = $this->rightsProfiles[$this->getVar( '_RightsProfile' )];
-                       foreach ( $rightsProfile as $group => $rightsArr ) {
-                               $ls->setGroupRights( $group, $rightsArr );
-                       }
-                       echo $ls->getText();
-
+                       $this->outputLS();
                        return $this->session;
                }
 
@@ -1231,6 +1220,25 @@ class WebInstaller extends Installer {
                return WebRequest::detectServer();
        }
 
+       /**
+        * Actually output LocalSettings.php for download
+        *
+        * @suppress SecurityCheck-XSS
+        */
+       private function outputLS() {
+               $this->request->response()->header( 'Content-type: application/x-httpd-php' );
+               $this->request->response()->header(
+                       'Content-Disposition: attachment; filename="LocalSettings.php"'
+               );
+
+               $ls = InstallerOverrides::getLocalSettingsGenerator( $this );
+               $rightsProfile = $this->rightsProfiles[$this->getVar( '_RightsProfile' )];
+               foreach ( $rightsProfile as $group => $rightsArr ) {
+                       $ls->setGroupRights( $group, $rightsArr );
+               }
+               echo $ls->getText();
+       }
+
        /**
         * Output stylesheet for web installer pages
         */

diff --git a/includes/media/MediaTransformOutput.php b/includes/media/MediaTransformOutput.php
index dba1b60..010cf80 100644 (file)
--- a/includes/media/MediaTransformOutput.php
+++ b/includes/media/MediaTransformOutput.php
@@ -507,7 +507,7 @@ class TransformParameterError extends MediaTransformError {
 class TransformTooBigImageAreaError extends MediaTransformError {
        function __construct( $params, $maxImageArea ) {
                $msg = wfMessage( 'thumbnail_toobigimagearea' );
-               $msg->rawParams(
+               $msg->params(
                        $msg->getLanguage()->formatComputingNumbers( $maxImageArea, 1000, "size-$1pixel" )
                );
 

diff --git a/includes/page/ImagePage.php b/includes/page/ImagePage.php
index 81677d4..58f25d4 100644 (file)
--- a/includes/page/ImagePage.php
+++ b/includes/page/ImagePage.php
@@ -440,7 +440,7 @@ class ImagePage extends Article {
                                                // in the mediawiki.page.image.pagination module
                                                $link = Linker::linkKnown(
                                                        $this->getTitle(),
-                                                       $label,
+                                                       htmlspecialchars( $label ),
                                                        [],
                                                        [ 'page' => $page - 1 ]
                                                );
@@ -460,7 +460,7 @@ class ImagePage extends Article {
                                                $label = $this->getContext()->msg( 'imgmultipagenext' )->text();
                                                $link = Linker::linkKnown(
                                                        $this->getTitle(),
-                                                       $label,
+                                                       htmlspecialchars( $label ),
                                                        [],
                                                        [ 'page' => $page + 1 ]
                                                );

diff --git a/includes/parser/CoreTagHooks.php b/includes/parser/CoreTagHooks.php
index 438603a..d178600 100644 (file)
--- a/includes/parser/CoreTagHooks.php
+++ b/includes/parser/CoreTagHooks.php
@@ -46,6 +46,10 @@ class CoreTagHooks {
         * Text is treated roughly as 'nowiki' wrapped in an HTML 'pre' tag;
         * valid HTML attributes are passed on.
         *
+        * Uses custom html escaping which phan-taint-check won't recognize
+        * hence we suppress the error.
+        * @suppress SecurityCheck-XSS
+        *
         * @param string $text
         * @param array $attribs
         * @param Parser $parser
@@ -75,6 +79,7 @@ class CoreTagHooks {
         *
         * Uses undocumented extended tag hook return values, introduced in r61913.
         *
+        * @suppress SecurityCheck-XSS
         * @param string $content
         * @param array $attributes
         * @param Parser $parser
@@ -110,6 +115,10 @@ class CoreTagHooks {
         *
         * Uses undocumented extended tag hook return values, introduced in r61913.
         *
+        * Uses custom html escaping which phan-taint-check won't recognize
+        * hence we suppress the error.
+        * @suppress SecurityCheck-XSS
+        *
         * @param string $content
         * @param array $attributes
         * @param Parser $parser

diff --git a/includes/specials/SpecialAutoblockList.php b/includes/specials/SpecialAutoblockList.php
index e1909f5..cab5a2e 100644 (file)
--- a/includes/specials/SpecialAutoblockList.php
+++ b/includes/specials/SpecialAutoblockList.php
@@ -98,7 +98,7 @@ class SpecialAutoblockList extends SpecialPage {
        protected function showTotal( BlockListPager $pager ) {
                $out = $this->getOutput();
                $out->addHTML(
-                       Html::element( 'div', [ 'style' => 'font-weight: bold;' ],
+                       Html::rawElement( 'div', [ 'style' => 'font-weight: bold;' ],
                                $this->msg( 'autoblocklist-total-autoblocks', $pager->getTotalAutoblocks() )->parse() )
                        . "\n"
                );
@@ -119,7 +119,7 @@ class SpecialAutoblockList extends SpecialPage {
                # Not necessary in a standard installation without such extensions enabled
                if ( count( $otherAutoblockLink ) ) {
                        $out->addHTML(
-                               Html::element( 'h2', [], $this->msg( 'autoblocklist-localblocks',
+                               Html::rawElement( 'h2', [], $this->msg( 'autoblocklist-localblocks',
                                        $pager->getNumRows() )->parse() )
                                . "\n"
                        );

diff --git a/includes/specials/SpecialMediaStatistics.php b/includes/specials/SpecialMediaStatistics.php
index 26c9b06..6dbc09b 100644 (file)
--- a/includes/specials/SpecialMediaStatistics.php
+++ b/includes/specials/SpecialMediaStatistics.php
@@ -182,7 +182,7 @@ class MediaStatisticsPage extends QueryPage {
                        [],
                        $linkRenderer->makeLink( $mimeSearch, $mime )
                );
-               $row .= Html::element(
+               $row .= Html::rawElement(
                        'td',
                        [],
                        $this->getExtensionList( $mime )
@@ -245,7 +245,7 @@ class MediaStatisticsPage extends QueryPage {
                $extArray = explode( ' ', $exts );
                $extArray = array_unique( $extArray );
                foreach ( $extArray as &$ext ) {
-                       $ext = '.' . $ext;
+                       $ext = htmlspecialchars( '.' . $ext );
                }
 
                return $this->getLanguage()->commaList( $extArray );

diff --git a/includes/specials/pagers/ImageListPager.php b/includes/specials/pagers/ImageListPager.php
index b2f1487..3d7148d 100644 (file)
--- a/includes/specials/pagers/ImageListPager.php
+++ b/includes/specials/pagers/ImageListPager.php
@@ -159,6 +159,9 @@ class ImageListPager extends TablePager {
        }
 
        /**
+        * The array keys (but not the array values) are used in sql. Phan
+        * gets confused by this, so mark this method as being ok for sql in general.
+        * @return-taint onlysafefor_sql
         * @return array
         */
        function getFieldNames() {