Page MenuHomePhabricator

CI fails for GrowthExperiments's master branch
Closed, ResolvedPublic

Description

12:59:11 includes/HelpPanel/QuestionStore.php:274 SecurityCheck-DoubleEscaped Calling method \GrowthExperiments\HelpPanel\QuestionRecord::setQuestionText() in \GrowthExperiments\HelpPanel\QuestionStore::trimQuestion that outputs using tainted argument #1. (Caused by: includes/HelpPanel/QuestionFormatter.php +75; includes/HelpPanel/QuestionFormatter.php +59; includes/HelpPanel/QuestionStore.php +274; includes/HelpPanel/QuestionStore.php +270; ../../includes/language/Language.php +3552; ../../includes/language/Language.php +3586; ../../includes/language/Language.php +3583; Builtin-\Message::escaped; ../../includes/language/Language.php +3613; ../../includes/language/Language.php +3592; ../../includes/language/Language.php +3583; Builtin-\Message::escaped; ...) (Caused by: includes/HelpPanel/QuestionStore.php +270; ../../includes/language/Language.php +3552; ../../includes/language/Language.php +3586; ../../includes/language/Language.php +3583; Builtin-\Message::escaped; ../../includes/language/Language.php +3613; ../../includes/language/Language.php +3592; ../../includes/language/Language.php +3583; Builtin-\Message::escaped; ../../includes/language/Language.php +3600; ../../includes/language/Language.php +3583; Builtin-\Message::escaped; ...)

from https://integration.wikimedia.org/ci/job/mwext-php72-phan-docker/136400/console, executed on https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/721579.

Doesn't appear to be related to changed code. Also see https://integration.wikimedia.org/ci/job/mwext-php72-phan-docker/136429/console / https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/721668 for a test on an empty commit.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Urbanecm_WMF renamed this task from Unrelated CI failure in GrowthExperiments to CI fails for GrowthExperiments's master branch.Sep 17 2021, 11:42 AM
Urbanecm_WMF updated the task description. (Show Details)

Quick explanation of why this happens:

QuestionStore::trimQuestion
$trimmedQuestionText = $this->language->truncateForVisual(
	$question->getQuestionText(),
	self::QUESTION_CHAR_LIMIT
);
$question->setQuestionText( $trimmedQuestionText ?? '' );

The output of Language::truncateForVisual may contain escaped text (it uses wfMessage( 'ellipsis' )->inLanguage( $this )->escaped();, see T290624).

And then we have:

QuestionFormatter::formatArchived
Html::element(
	'span',
	[ 'class' => 'question-text' ],
	$this->questionRecord->getQuestionText()
)

so it detects double-escaping.

My recommendation is to suppress the issue, until T290624 is resolved. Although that's not trivial, I think truncateInternal should just use Message::text, at which point the suppression here would become unnecessary and you could remove it.

Change 721810 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/GrowthExperiments@master] Suppress SecurityCheck-DoubleEscaped in QuestionStore::trimQuestion

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

Suppressing sounds fine to me. Uploaded a patch. Can you review @Daimona? Thanks!

Urbanecm_WMF triaged this task as Unbreak Now! priority.
Urbanecm_WMF edited projects, added Growth-Team (Current Sprint); removed Growth-Team.
Urbanecm_WMF moved this task from Incoming to Code Review on the Growth-Team (Current Sprint) board.

Change 721810 merged by Urbanecm:

[mediawiki/extensions/GrowthExperiments@master] Suppress SecurityCheck-DoubleEscaped in QuestionStore::trimQuestion

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