From: Brian Wolff Date: Thu, 12 Jul 2018 02:48:34 +0000 (+0000) Subject: Start working on phan-taint-check warnings. Fix minor escaping issues. X-Git-Tag: 1.34.0-rc.0~4799^2 X-Git-Url: http://git.cyclocoop.org/%22%20.%20generer_url_ecrire%28%22calendrier%22%2C%22type=semaine%22%29%20.%20%22?a=commitdiff_plain;h=89b21847e151be15c41e667fa2a910ffbb72cf7d;p=lhc%2Fweb%2Fwiklou.git 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 543978b56e..3ea020fd3c 100644 --- a/includes/GlobalFunctions.php +++ b/includes/GlobalFunctions.php @@ -1628,6 +1628,7 @@ function wfClientAcceptsGzip( $force = false ) { * As required by the callers, "" 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 a499035e11..5ca0942291 100644 --- 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 9271e3f729..dc7d31e522 100644 --- 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 8ea50ece36..bd64a41479 100644 --- 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 a593d27c66..cdfbf56576 100644 --- 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 = '' . $rcObj->timestamp . ' '; + $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 bf140eae0d..e49a846679 100644 --- 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 2ba221faf4..739d9f21cf 100644 --- 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 dba1b60aef..010cf80177 100644 --- 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 81677d41ce..58f25d4650 100644 --- 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 438603a841..d178600781 100644 --- 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 e1909f5243..cab5a2ef49 100644 --- 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 26c9b06f4a..6dbc09bae0 100644 --- 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 b2f1487645..3d7148df58 100644 --- 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() {