Page MenuHomePhabricator

phan-taint-check-plugin should ignore /extensions/ folder
Open, Needs TriagePublic

Description

The TranslateNotification extensions installed the Translate extensions over composer and that creates an /extensions/ folder which seccheck reports errors from.

That means seccheck cannot installed on TranslateExtension until Translate is fixed.
Would be nice to ignore /extensions/

https://integration.wikimedia.org/ci/job/mwext-php70-phan-seccheck-docker/4217/console

<?xml version="1.0" encoding="ISO-8859-15"?>
<checkstyle version="6.5">
  <file name="./SpecialTranslatorSignup.php">
    <error line="168" severity="warning" message="HTMLForm option label needs escaping (for value 'always')" source="SecurityCheck-XSS"/>
    <error line="168" severity="warning" message="HTMLForm option label needs escaping (for value 'month')" source="SecurityCheck-XSS"/>
    <error line="168" severity="warning" message="HTMLForm option label needs escaping (for value 'monthly')" source="SecurityCheck-XSS"/>
    <error line="168" severity="warning" message="HTMLForm option label needs escaping (for value 'week')" source="SecurityCheck-XSS"/>
    <error line="168" severity="warning" message="HTMLForm option label needs escaping (for value 'weekly')" source="SecurityCheck-XSS"/>
  </file>
  <file name="./extensions/Translate/specials/CompatibleLinkRenderer.php">
    <error line="48" severity="warning" message="Calling method \Linker::linkKnown() in \CompatibleLinkRenderer::makeKnownLink that outputs using tainted argument $text. (Caused by: ./extensions/Translate/specials/CompatibleLinkRenderer.php +43; ./extensions/Translate/specials/CompatibleLinkRenderer.php +48)" source="SecurityCheck-XSS"/>
  </file>
  <file name="./extensions/Translate/specials/SpecialLanguageStats.php">
    <error line="263" severity="warning" message="Calling method \Message::rawParams() in \SpecialLanguageStats::outputIntroduction that outputs using tainted argument $rcInLangLink. (Caused by: ./extensions/Translate/specials/SpecialLanguageStats.php +253)" source="SecurityCheck-XSS"/>
  </file>
  <file name="./extensions/Translate/specials/SpecialSupportedLanguages.php">
    <error line="143" severity="warning" message="Calling method \OutputPage::addHTML() in \SpecialSupportedLanguages::showLanguage that outputs using tainted argument $[arg #1]. (Caused by: ./extensions/Translate/specials/SpecialSupportedLanguages.php +141)" source="SecurityCheck-XSS"/>
    <error line="378" severity="warning" message="Calling method \Message::rawParams() in \SpecialSupportedLanguages::makeUserList that outputs using tainted argument $linkList. (Caused by: ./extensions/Translate/specials/SpecialSupportedLanguages.php +376)" source="SecurityCheck-XSS"/>
  </file>
  <file name="./extensions/Translate/specials/SpecialTranslationStats.php">
    <error line="208" severity="warning" message="Calling method \Html::element() in \SpecialTranslationStats::form that outputs using tainted argument $[arg #3]. (Caused by: ./extensions/Translate/specials/SpecialTranslationStats.php +199; ./extensions/Translate/specials/SpecialTranslationStats.php +203)" source="SecurityCheck-DoubleEscaped"/>
    <error line="209" severity="warning" message="Calling method \Html::element() in \SpecialTranslationStats::form that outputs using tainted argument $[arg #3]. (Caused by: ./extensions/Translate/specials/SpecialTranslationStats.php +199; ./extensions/Translate/specials/SpecialTranslationStats.php +203)" source="SecurityCheck-DoubleEscaped"/>
    <error line="210" severity="warning" message="Calling method \Html::element() in \SpecialTranslationStats::form that outputs using tainted argument $[arg #3]. (Caused by: ./extensions/Translate/specials/SpecialTranslationStats.php +199; ./extensions/Translate/specials/SpecialTranslationStats.php +203)" source="SecurityCheck-DoubleEscaped"/>
  </file>
  <file name="./extensions/Translate/specials/SpecialTranslations.php">
    <error line="268" severity="warning" message="Calling method \OutputPage::addHTML() in \SpecialTranslations::showTranslations that outputs using tainted argument $out. (Caused by: ./extensions/Translate/specials/SpecialTranslations.php +208; ./extensions/Translate/specials/SpecialTranslations.php +203; ./extensions/Translate/specials/SpecialTranslations.php +204; ./extensions/Translate/specials/SpecialTranslations.php +205; ....)" source="SecurityCheck-XSS"/>
  </file>
  <file name="./extensions/Translate/tag/PageTranslationHooks.php">
    <error line="337" severity="warning" message="Assigning a tainted value to a variable that later does something unsafe with it (Caused by: ./extensions/Translate/tag/PageTranslationHooks.php +313; ./extensions/Translate/tag/PageTranslationHooks.php +337)" source="SecurityCheck-XSS"/>
    <error line="337" severity="warning" message="Calling method \Linker::linkKnown() in \PageTranslationHooks::languages that outputs using tainted argument $name. (Caused by: ./extensions/Translate/tag/PageTranslationHooks.php +313; ./extensions/Translate/tag/PageTranslationHooks.php +337)" source="SecurityCheck-XSS"/>
    <error line="354" severity="warning" message="Assigning a tainted value to a variable that later does something unsafe with it (Caused by: ./extensions/Translate/tag/PageTranslationHooks.php +313; ./extensions/Translate/tag/PageTranslationHooks.php +337; ./extensions/Translate/tag/PageTranslationHooks.php +354)" source="SecurityCheck-XSS"/>
    <error line="354" severity="warning" message="Calling method \Linker::linkKnown() in \PageTranslationHooks::languages that outputs using tainted argument $name. (Caused by: ./extensions/Translate/tag/PageTranslationHooks.php +313; ./extensions/Translate/tag/PageTranslationHooks.php +337; ./extensions/Translate/tag/PageTranslationHooks.php +354)" source="SecurityCheck-XSS"/>
    <error line="364" severity="warning" message="Assigning a tainted value to a variable that later does something unsafe with it (Caused by: ./extensions/Translate/tag/PageTranslationHooks.php +357; ./extensions/Translate/tag/PageTranslationHooks.php +313; ./extensions/Translate/tag/PageTranslationHooks.php +337; ./extensions/Translate/tag/PageTranslationHooks.php +354)" source="SecurityCheck-XSS"/>
    <error line="383" severity="warning" message="Outputting user controlled HTML from Parser tag hook \PageTranslationHooks::languages (Caused by: ./extensions/Translate/tag/PageTranslationHooks.php +374; ./extensions/Translate/tag/PageTranslationHooks.php +379)" source="SecurityCheck-XSS"/>
  </file>
  <file name="./extensions/Translate/tag/PageTranslationLogFormatter.php">
    <error line="77" severity="warning" message="Calling Method \LogFormatter::getComment in \PageTranslationLogFormatter::getComment that is always unsafe  (Caused by: ../../includes/logging/LogFormatter.php +706; ../../includes/logging/LogFormatter.php +703)" source="SecurityCheck-DoubleEscaped"/>
  </file>
  <file name="./extensions/Translate/utils/MessageWebImporter.php">
    <error line="316" severity="warning" message="Calling method \htmlspecialchars() in \MessageWebImporter::execute that outputs using tainted argument $key. (Caused by: ./extensions/Translate/utils/MessageWebImporter.php +278; ./extensions/Translate/utils/MessageWebImporter.php +278; ./extensions/Translate/utils/MessageWebImporter.php +280)" source="SecurityCheck-DoubleEscaped"/>
    <error line="316" severity="warning" message="Calling method \htmlspecialchars() in \MessageWebImporter::execute that outputs using tainted argument $key. (Caused by: ./extensions/Translate/utils/MessageWebImporter.php +278; ./extensions/Translate/utils/MessageWebImporter.php +280)" source="SecurityCheck-DoubleEscaped"/>
    <error line="350" severity="warning" message="Calling method \OutputPage::addHTML() in \MessageWebImporter::execute that outputs using tainted argument $[arg #1]. (Caused by: ./extensions/Translate/utils/MessageWebImporter.php +281; ./extensions/Translate/utils/MessageWebImporter.php +269; ./extensions/Translate/utils/MessageWebImporter.php +289; ./extensions/Translate/utils/MessageWebImporter.php +349)" source="SecurityCheck-XSS"/>
    <error line="364" severity="warning" message="Calling method \OutputPage::addHTML() in \MessageWebImporter::execute that outputs using tainted argument $[arg #1]. (Caused by: ./extensions/Translate/utils/MessageWebImporter.php +281; ./extensions/Translate/utils/MessageWebImporter.php +269; ./extensions/Translate/utils/MessageWebImporter.php +289; ./extensions/Translate/utils/MessageWebImporter.php +349)" source="SecurityCheck-XSS"/>
  </file>
</checkstyle>

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 12 2018, 6:40 PM

We could add it to exclude_analysis_directory_list in scripts/mwext-fast-config.php in phan-taint-plugin (Like how vendor currently is excluded)

But is it something we should assume in general. Seems like its not exactly a universal convention that an extensions subdirectory is full of things we should not scan.

Translate has type "mediawiki-extension" in composer.json. In combine with composer installer this results in the folder named /extensions/.

On CI the dependency Translate is already available on the same level as the extension, not inside it.

When using the composer-merge-plugin and running composer in the mediawiki/core-folder the extensions folder is correct, because it is not inside the TranslateNotification extension.

Maybe composer is called different here than on other CI jobs?

I checked all extensions in Gerrit, none of them legitimately have an extensions/ directory.

hashar added a subscriber: hashar.Aug 22 2018, 9:48 PM

TranslationNotifications has:

composer.json
{
	"type": "mediawiki-extension",
	"require": {
		"composer/installers": "*",
		"mediawiki/translate": "dev-master"
	},
}

That eventually causes composer to install Translate under the TranslationNotifications directory:

$ composer update
...
Installing mediawiki/translate (dev-master d08e3d7)

Hence we end up with a /extensions/TranslationNotifications/extensions/Translate

The type and require are remnants from a first try to use packagist/composer to fulfill extensions dependencies. I wrote the mediawiki-extension installer iirc and we tried using that pattern on a few extensions. Eventually that got dished out and the now standard way is to use extension registration.

Or in short:

And that would solve it :]

Change 503504 had a related patch set uploaded (by SBassett; owner: SBassett):
[mediawiki/extensions/TranslationNotifications@master] Transfer composer.json dependency to extension.json

https://gerrit.wikimedia.org/r/503504

Change 503504 merged by jenkins-bot:
[mediawiki/extensions/TranslationNotifications@master] Transfer composer.json dependency to extension.json

https://gerrit.wikimedia.org/r/503504