Page MenuHomePhabricator

UserGroupMembership::getLink() causes a significant portion of false positives for phan-taint-check-plugin
Open, Needs TriagePublic

Description

phan-taint-check-plugin does not take into account the actual values of variables or really do any flow analysis at all for that matter.

As a result, it cannot tell if the third argument to UserGroupMembership::getLink() is 'html' or 'wiki', even if its specified as a literal string in the method call. This results in a significant portion of the false positives from the tool.

So we could maybe try and special case phan-taint-check-plugin for this particular function (This would involve quite a bit of work to do properly. To do hackly would be less work). Or maybe we could split this function into UserGroupMembership::getLinkForWikitext() and UserGroupMembership::getLinkForHtml().

On one hand, tools should serve us not the other way around, we should not be forced to use coding constructs due to limitations of tools. On the other hand, the method is relatively new, and its a rather rare idiom in MediaWiki to control whether or not escaping happens via an argument (instead of different function name). I tend towards the splitting the function option, but I might be biased, and would like to hear futher opinions.

Event Timeline

Bawolff created this task.Dec 18 2017, 7:44 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 18 2017, 7:44 PM
TTO added a comment.Dec 19 2017, 12:29 AM

If only PHP supported proper enums...

The function was previously split when it belonged to the User class (User::makeGroupLinkHTML and User::makeGroupLinkWiki) but I merged the two functions into one to eliminate duplication of logic. I don't really have an opinion on what should be done here.

Nice work on the taint tool btw, this is something we have needed for years.

Legoktm added a subscriber: Legoktm.

+1 to splitting the methods again. We can use a private helper function to avoid duplicating logic.

I definitely agree that we should use private methods to avoid code duplication as much as possible if we split the method.

Change 456879 had a related patch set uploaded (by Brian Wolff; owner: Brian Wolff):
[mediawiki/core@master] Split UserGroupMembership::getLink into 2 functions

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