Page MenuHomePhabricator
Create Task
Maniphest T291259

CI fails for GrowthExperiments's master branch
Closed, ResolvedPublic
Actions

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.

Details

Related Changes in Gerrit:
SubjectRepoBranchLines +/-
Suppress SecurityCheck-DoubleEscaped in QuestionStore::trimQuestionmediawiki/extensions/GrowthExperimentsmaster+5 -0
Customize query in gerrit

Related Objects

Event Timeline

Urbanecm_WMF created this task.Sep 17 2021, 11:28 AM
Restricted Application added a project: Growth-Team. · View Herald TranscriptSep 17 2021, 11:28 AM
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)
Urbanecm mentioned this in T291235: Requesting access to SRE / DevOps / CI/CD related privileges.Sep 17 2021, 11:43 AM
Urbanecm_WMF added a project: phan-taint-check-plugin.Sep 17 2021, 12:22 PM
Urbanecm_WMF moved this task from Backlog to Wikimedia deployed on the phan-taint-check-plugin board.
kostajh subscribed.Sep 17 2021, 12:35 PM
Daimona subscribed.Sep 17 2021, 12:39 PM

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.

gerritbot added a comment.Sep 17 2021, 12:53 PM

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

gerritbot added a project: Patch-For-Review.Sep 17 2021, 12:53 PM
Urbanecm_WMF added a comment.Sep 17 2021, 12:54 PM

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

Urbanecm_WMF claimed this task.Sep 17 2021, 12:56 PM
Urbanecm_WMF triaged this task as Unbreak Now! priority.
Urbanecm_WMF edited projects, added Growth-Team (Sprint 0 (Growth Team)); removed Growth-Team.
Urbanecm_WMF moved this task from Incoming to Code Review on the Growth-Team (Sprint 0 (Growth Team)) board.
Restricted Application added a project: User-Urbanecm_WMF. · View Herald TranscriptSep 17 2021, 12:56 PM
Urbanecm_WMF moved this task from Untriaged to Engineering on the User-Urbanecm_WMF board.Sep 17 2021, 12:56 PM
Urbanecm_WMF edited projects, added User-Urbanecm_WMF (Engineering); removed User-Urbanecm_WMF.
Urbanecm_WMF moved this task from Backlog to In progress on the User-Urbanecm_WMF (Engineering) board.
Urbanecm_WMF mentioned this in T291266: Remove suppression of SecurityCheck-DoubleEscaped from QuestionStore::trimQuestion.Sep 17 2021, 1:04 PM

Filled T291266 so we don't forget to remove it once it's time.

gerritbot added a comment.Sep 17 2021, 1:14 PM

Change 721810 merged by Urbanecm:

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

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

Urbanecm_WMF closed this task as Resolved.Sep 17 2021, 1:15 PM
ReleaseTaggerBot added a project: MW-1.38-notes (1.38.0-wmf.1; 2021-09-21).Sep 17 2021, 2:00 PM
Maintenance_bot removed a project: Patch-For-Review.Sep 17 2021, 2:10 PM
Daimona moved this task from Wikimedia deployed to Issues in MediaWiki code on the phan-taint-check-plugin board.Oct 15 2022, 4:47 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits