Page MenuHomePhabricator

phan-taint-check-plugin blocking MobileFrontend merges
Closed, ResolvedPublic

Description

Currently all patches to MobileFrontend are erroring with the following warning:

<?xml version="1.0" encoding="ISO-8859-15"?>
18:36:24 <checkstyle version="6.5">
18:36:24   <file name="./includes/specials/SpecialMobileDiff.php">
18:36:24     <error line="196" severity="warning" message="Calling method \Linker::formatComment() in \SpecialMobileDiff::getCommentHTML that outputs using tainted argument $comment. (Caused by: ../../includes/Linker.php +1104)" source="SecurityCheck-DoubleEscaped"/>
18:36:24   </file>
18:36:24 </checkstyle>
18:36:25 Build step 'Execute shell' marked build as failure

The error is a little cryptic to me given I'm not too familiar with this plugin and it's not clear how $comment (which comes from Revision::getComment) is tainted.

It's not exactly clear what the problem is (and if it is a problem) and how to solve it.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 30 2018, 7:31 PM
matmarex claimed this task.Aug 30 2018, 8:07 PM
matmarex added a subscriber: matmarex.

Code snippet for reference:

	private function getCommentHTML( $unhide = false ) {
		$audience = $unhide ? Revision::FOR_THIS_USER : Revision::FOR_PUBLIC;
		$comment = $this->rev->getComment( $audience );

		if ( $this->rev->isDeleted( Revision::DELETED_COMMENT ) && !$unhide ) {
			$comment = $this->msg( 'rev-deleted-comment' )->escaped();
		} elseif ( $comment !== '' && $comment !== null ) {
			$comment = Linker::formatComment( $comment, $this->targetTitle );
		} else {
			$comment = $this->msg( 'mobile-frontend-changeslist-nocomment' )->escaped();
		}

		return Html::rawElement(
			'div',
			[ 'id' => 'mw-mf-diff-comment' ],
			$comment
		);
	}

I think Phan is confused because the $comment variable is reused to contain data with different taints. I vaguely recall a commit somewhere fixing a similar problem by renaming a variable. Let's see if that helps here.

Change 456436 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/MobileFrontend@master] SpecialMobileDiff: Work around Phan false positive by renaming a variable

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

(The issue might have been triggered by commit faf2e14517b05f8c2c5016c0be11b76a02c27efd in MW core)

Change 456436 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] SpecialMobileDiff: Work around Phan false positive by renaming a variable

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

matmarex closed this task as Resolved.Aug 30 2018, 10:25 PM
matmarex removed a project: Patch-For-Review.
sbassett triaged this task as Medium priority.Oct 15 2019, 7:10 PM