Page MenuHomePhabricator

Phan error inside ThanksLogFormatter
Closed, ResolvedPublic

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

Related Gerrit Patches:
mediawiki/tools/phan/SecurityCheckPlugin : masterFix some confusion over which group of taints to mask out in various places
mediawiki/extensions/Newsletter : masterRemove unneccessary @suppress after phan-taint-check-plugin upgrade
mediawiki/extensions/Thanks : masterRemove unneccessary @suppress after phan-taint-check-plugin upgrade
mediawiki/extensions/ThrottleOverride : masterRemove unneccessary @suppress after phan-taint-check-plugin upgrade
mediawiki/tools/phan/SecurityCheckPlugin : masterMake onlysafefor_html not mark things as exec_escaped.
integration/config : masterRevert "Make seccheck voting for Thanks"

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
JTannerWMF moved this task from Inbox to Current Sprint on the Growth-Team board.Aug 9 2018, 6:08 PM
JTannerWMF edited projects, added Growth-Team (Current Sprint); removed Growth-Team.
pmiazga added a subscriber: pmiazga.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.

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

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.

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

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

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

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

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

Mh-3110 added a subscriber: Mh-3110.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

D3r1ck01 added a subscriber: D3r1ck01.

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

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

This is fixed in 1.4.0.

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

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

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

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

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

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

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.

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