Page MenuHomePhabricator

Phan warning in "includes/specials/SpecialMobileDiff.php" - working around by disabling
Closed, ResolvedPublic


15:08:39 <checkstyle version="6.5">
15:08:39 <file name="includes/specials/SpecialMobileDiff.php">
15:08:39 <error line="339" severity="warning" message="Calling method \OutputPage::addHTML() in \SpecialMobileDiff::showFooter that outputs using tainted argument $[arg #1]. (Caused by: Builtin-\OutputPage::addHTML) (Caused by: ../../includes/Html.php +304; ../../includes/page/ImagePage.php +878; includes/specials/SpecialMobileDiff.php +331; Builtin-\MediaWiki\Linker\LinkRenderer::makeLink; Builtin-\MediaWiki\Linker\LinkRenderer::makeLink; ../../includes/user/User.php +2282; ../../includes/Revision/RevisionStore.php +1874; ../../includes/user/User.php +1421; ../../includes/Revision/RevisionStore.php +1874; ../../includes/user/User.php +1421; includes/specials/SpecialMobileDiff.php +397; Builtin-\Message::parse; ../../includes/language/Message.php +940)" source="SecurityCheck-XSS"/>
15:08:39 </file>
15:08:39 </checkstyle>

Event Timeline

Change 582935 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Disable phan warning for now

I actually found a different HTML escaping issue while investigating this:

But I think the Phan error is a false positive. The methods it names look correct at a glance. And we don't even call anything from ImagePage.php…?

It seems to be complaining about output of SpecialMobileDiff::listGroups() being blindly output as HTML. This warning seems correct because that method returns the result of UserGroupMembership::getLink which in turn returns the result of Message::text() which is not secure HTML, but unparsed user wikitext.

I fixed that in the patch I linked, and the Phan error is still appearing.

Change 582935 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Disable phan warning for now

This is not a new issue with the code and was blocking some important merges today, so I've disabled it for now.

Now those merges have happened we should follow up with some analysis to pay off the technical debt we've accrued.

sbassett triaged this task as Medium priority.Mar 24 2020, 3:27 PM
sbassett moved this task from Incoming to Watching on the Security-Team board.

Can you add me to that ticket @sbassett ? Thanks in advance!

Can you add me to that ticket @sbassett ? Thanks in advance!

@matmarex subbed you. And it looks like that issue might be incidentally fixed with c582909.

Change 583376 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Revert "Disable phan warning for now"

@matmarex @sbassett looks like we can revert this change now?

SecCheckPlugin seems happy on the revert, so lgtm. Let me know if you want a +2 on the patch.

Change 583376 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Revert "Disable phan warning for now"

See also T183174. I haven't checked whether that issue is now fixed.