Here phan-taint-check-plugin is inferring that usernames are not safe to output (because they are user-controlled coming from the database; although I'm not sure how exploitable that is in practice, given that some symbols are not allowed in usernames). This is what most of the source references are pointing to. Then it finds the array_map in SpecialClaimMentee#192, but it doesn't really know how array_map works: T204911. So it just assumes that the array ($this->mentees) remains unchanged. But that's an array of User objects, so it thinks that we're outputting user objects, which would yield the username via __toString; and because it already knows usernames are potentially unsafe, it emits the issue. If it were able to read the array_map, it would see that the code is perfectly fine.

Ideally, the limitation would be addressed in T204911, but last time I tried that a few years ago it wasn't so easy. For the time being, the issue can be suppressed in place with // @phan-suppress-next-line SecurityCheck-XSS.

Change #1184557 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[mediawiki/extensions/GrowthExperiments@master] phan: supress unlikely xss warn

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

Thank you @Daimona that's very helpful. Now phan says the supression it's unsued, so it looks like a flaky phan check? Did I get the entry point right?

In T403614#11144600, @Sgs wrote:

Thank you @Daimona that's very helpful. Now phan says the supression it's unsued, so it looks like a flaky phan check? Did I get the entry point right?

You would need to suppress the issue in the same patch where it appears. Apparently it is not present in master, which is why you get an UnusedSuppression warning. Phan's results are deterministic for a given state of the repo, so you can't have flakiness, strictly speaking. However, "state of the repo" is kinda broad, and phan's analysis is affected by a variety of factors. One of them is the order in which classes/methods are analyzed, which in turn depends on the order in which references to those calls/methods are found in the source code. This is especially the case when a depth limit is involved (e.g., analysis stops due to reaching max dept when following a path, but not another). The upstream documentation also gives an example. r1075286 is likely affecting the order of analysis and making phan "discover" the issue, so the suppression should be added in that patch.

(Also, small clarification: the current code would be perfectly safe even if we allowed all symbols in usernames, because Linker::userLink would escape those. The problem is that taint-check doesn't see the userLink call, so it wrongly thinks the code is essentially equivalent to echo $user->getName())

Replaced array_map with loop as part of https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/1184575

Change #1184557 abandoned by Sergio Gimeno:

[mediawiki/extensions/GrowthExperiments@master] phan: supress unlikely xss warn

Reason:

Solved by I0d79f709067f75a7937f0c03050ab213cdf69afd

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