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

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

Change 456879 abandoned by Brian Wolff:
Refactor UserGroupMembership

Reason:
I don't really care about this anymore. Please feel free to take this over if anyone is interested.

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

This is still causing many false positives. I agree with everything that was written above, including that taint-check cannot properly handle this case, that adding a hack would be trivial but it shouldn't become the standard way to fix issues in code, and that splitting this method seems a very good idea, regardless of taint-check. So I'm submitting a slim patch that does just the splitting now.

Change 655688 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Split UserGroupMembership::getLink

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