Page MenuHomePhabricator
Create Task
Maniphest T201565

Phan error inside ThanksLogFormatter
Closed, ResolvedPublic
Actions

Description

This error blocks any kind of merge on the Thanks extension:
e.g. https://integration.wikimedia.org/ci/job/mwext-php70-phan-seccheck-docker/4225/console

<?xml version="1.0" encoding="ISO-8859-15"?>
21:06:36 <checkstyle version="6.5">
21:06:36   <file name="./includes/ThanksLogFormatter.php">
21:06:36     <error line="11" severity="warning" message="Calling Method \ThanksLogFormatter::makeUserLink in \ThanksLogFormatter::getMessageParameters that is always unsafe " source="SecurityCheck-DoubleEscaped"/>
21:06:36   </file>

Details

SubjectRepoBranchLines +/-
Fix some confusion over which group of taints to mask out in various placesmediawiki/tools/phan/SecurityCheckPluginmaster+57 -23
Remove unneccessary @suppress after phan-taint-check-plugin upgrademediawiki/extensions/Newslettermaster+0 -1
Remove unneccessary @suppress after phan-taint-check-plugin upgrademediawiki/extensions/Thanksmaster+0 -1
Remove unneccessary @suppress after phan-taint-check-plugin upgrademediawiki/extensions/ThrottleOverridemaster+0 -1
Make onlysafefor_html not mark things as exec_escaped.mediawiki/tools/phan/SecurityCheckPluginmaster+41 -4
Revert "Make seccheck voting for Thanks"integration/configmaster+1 -1
Customize query in gerrit

Related Objects
Search...

Event Timeline

Jdlrobson created this task.Aug 8 2018, 9:32 PM
Restricted Application added a project: Growth-Team. · View Herald TranscriptAug 8 2018, 9:32 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Jdlrobson updated the task description. (Show Details)Aug 8 2018, 9:32 PM
Umherirrender added subscribers: Legoktm, Bawolff, Umherirrender.Aug 9 2018, 5:07 PM

The taint was set with https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/444452/

JTannerWMF moved this task from Inbox to Sprint 0 (Growth Team) on the Growth-Team board.Aug 9 2018, 6:08 PM
JTannerWMF edited projects, added Growth-Team (Sprint 0 (Growth Team)); removed Growth-Team.
pmiazga subscribed.Aug 9 2018, 6:54 PM

Very similar construct has also the NewsletterExtension

Jdlrobson triaged this task as High priority.Aug 9 2018, 6:55 PM

This is blocking merges in Thanks right now. I'm not 100% sure what this issue is about, but it appears to be a pre-existing issue that's been surfaced by the introduction of phan.

Bawolff added a comment.Aug 9 2018, 7:43 PM

Im on vacation right now but will look into what the cause is when i get back in a couple days. It may just be a bug in the plugin. In the mean while if this is blocking stuff maybe turn seccheck non voting temporarily

t

Catrope subscribed.Aug 9 2018, 10:10 PM

This is strange. The offending code in ThanksLogFormatter is:

	protected function getMessageParameters() {
		$params = parent::getMessageParameters();
		// Convert target from a pageLink to a userLink since the target is
		// actually a user, not a page.
		$recipient = User::newFromName( $this->entry->getTarget()->getText(), false );
		$params[2] = Message::rawParam( $this->makeUserLink( $recipient ) ); // <--- HERE
		$params[3] = $recipient->getName();
		return $params;
	}

The complaint is that calling makeUserLink is "always unsafe", and the error code suggests a concern about double escaping (which seem to be to be contradictory concerns?!). The code looks correct to me though, because the result of makeUserLink() (which is flagged with the onlysafefor_html taint) is passed into Message::rawParam() (which is meant to take HTML). Moreover, there is other code that does the exact same thing (LogFormatter::formatParameterValue(), for example, calls Message::rawParam( $this->makeUserLink( $user ) )) and the taint checker doesn't flag that one as a problem.

gerritbot subscribed.Aug 9 2018, 10:11 PM

Change 451798 had a related patch set uploaded (by Catrope; owner: Catrope):
[integration/config@master] Revert "Make seccheck voting for Thanks"

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

gerritbot added a project: Patch-For-Review.Aug 9 2018, 10:11 PM
gerritbot added a comment.Aug 9 2018, 10:18 PM

Change 451798 merged by jenkins-bot:
[integration/config@master] Revert "Make seccheck voting for Thanks"

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

Catrope removed projects: Patch-For-Review, Growth-Team (Sprint 0 (Growth Team)).Aug 9 2018, 10:33 PM

Now that seccheck is non-voting, this no longer blocks merges in Thanks.

Restricted Application added a project: Growth-Team. · View Herald TranscriptAug 9 2018, 10:33 PM
Catrope moved this task from Inbox to External on the Growth-Team board.Aug 9 2018, 10:33 PM
Bawolff added a comment.Aug 10 2018, 2:27 AM

Theres a lot of weirdness in log formatters due to how base class handles irc log messages....

Umherirrender moved this task from Backlog to Wikimedia deployed on the phan-taint-check-plugin board.Aug 21 2018, 9:27 AM
Umherirrender mentioned this in T202373: Add phan-taint-check-plugin to Flow extension.Aug 21 2018, 10:34 AM
Umherirrender mentioned this in T202379: Add phan-taint-check-plugin to Translate extension.Aug 21 2018, 11:32 AM
Umherirrender added a parent task: T201219: Enable phan-taint-check-plugin on all Wikimedia-deployed repositories after getting it to pass.Aug 21 2018, 12:40 PM
Mh-3110 subscribed.EditedAug 21 2018, 3:17 PM

Hi,
This issue is also causing main test faillure. Case of the patch I've submitted for the Thanks extension. error log

xSavitar awarded a token.Aug 21 2018, 7:47 PM
xSavitar subscribed.
gerritbot added a comment.Aug 31 2018, 9:47 AM

Change 456581 had a related patch set uploaded (by Brian Wolff; owner: Brian Wolff):
[mediawiki/tools/phan/SecurityCheckPlugin@master] Make onlysafefor_html not mark things as exec_escaped.

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

gerritbot added a project: Patch-For-Review.Aug 31 2018, 9:47 AM
gerritbot added a comment.Aug 31 2018, 6:40 PM

Change 456581 merged by jenkins-bot:
[mediawiki/tools/phan/SecurityCheckPlugin@master] Make onlysafefor_html not mark things as exec_escaped.

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

Legoktm added a comment.Sep 1 2018, 5:13 AM

This is fixed in 1.4.0.

gerritbot added a comment.Sep 2 2018, 2:58 AM

Change 457073 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/extensions/Newsletter@master] Remove unneccessary @suppress after phan-taint-check-plugin upgrade

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

gerritbot added a comment.Sep 2 2018, 2:59 AM

Change 457074 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/extensions/Thanks@master] Remove unneccessary @suppress after phan-taint-check-plugin upgrade

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

gerritbot added a comment.Sep 2 2018, 2:59 AM

Change 457075 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/extensions/ThrottleOverride@master] Remove unneccessary @suppress after phan-taint-check-plugin upgrade

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

gerritbot added a comment.Sep 2 2018, 8:27 AM

Change 457075 merged by jenkins-bot:
[mediawiki/extensions/ThrottleOverride@master] Remove unneccessary @suppress after phan-taint-check-plugin upgrade

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

gerritbot added a comment.Sep 2 2018, 8:36 AM

Change 457074 merged by jenkins-bot:
[mediawiki/extensions/Thanks@master] Remove unneccessary @suppress after phan-taint-check-plugin upgrade

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

gerritbot added a comment.Sep 2 2018, 8:36 AM

Change 457073 merged by jenkins-bot:
[mediawiki/extensions/Newsletter@master] Remove unneccessary @suppress after phan-taint-check-plugin upgrade

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

ReleaseTaggerBot added a project: MW-1.32-notes (WMF-deploy-2018-09-04 (1.32.0-wmf.20)).Sep 2 2018, 9:00 AM
gerritbot added a comment.Sep 6 2018, 12:17 AM

Change 458334 had a related patch set uploaded (by Brian Wolff; owner: Brian Wolff):
[mediawiki/tools/phan/SecurityCheckPlugin@master] Fix some confusion over which group of taints to mask out in various places

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

Legoktm closed this task as Resolved.Sep 6 2018, 5:53 AM
Legoktm assigned this task to Bawolff.
gerritbot added a comment.Sep 6 2018, 6:10 AM

Change 458334 merged by jenkins-bot:
[mediawiki/tools/phan/SecurityCheckPlugin@master] Fix some confusion over which group of taints to mask out in various places

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

sbassett moved this task from Wikimedia deployed to Done on the phan-taint-check-plugin board.Oct 15 2019, 6:54 PM
sbassett removed a project: Patch-For-Review.Oct 15 2019, 7:43 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