<file name="includes/specials/MobileSpecialPageFeed.php"> <error line="45" severity="warning" message="Calling method \htmlspecialchars() in \MobileSpecialPageFeed::formatComment that outputs using tainted argument $[arg #1]. (Caused by: includes/specials/MobileSpecialPageFeed.php +43)" source="SecurityCheck-DoubleEscaped"/> </file>
I cannot really check at the moment, but if taint-check can't understand the taintedness of the return value, then we should add custom annotations to the docblock of stripAllTags, instead of hardcoding stuff in taintcheck itself.
First of all, MobileFrontend is reusing $comment with different levels of taintedness. The plugin is well known to be unable to handle this correctly, although the fix for that is not easy. But for this specific case, renaming the variables doesn't help.
And then, Linker::formatComment is doing the same thing, but again it doesn't matter.
As a side note, if the var reuse in Linker::formatComment had played a role, this would have been a core problem. And I believe there could be various cases like that (i.e. core causing false positives in extensions), given that taint-check is not running for core (T216348).
Anyway, the real cause here is a weird (?) bug: Linker::formatComment is returning an escaped comment. But Sanitizer::stripAllTags removes the escaping and returns tainted HTML. I didn't even check, but I bet that Remex's Tokenizer is too complicated for the plugin, and so it just thinks that the taintedness of the return value is the same as the parameter's (=escaped).
I think the best we can do is add @return-taint tainted to the docblock of Sanitizer::stripAllTags. Note that it could cause new problems to be detected, and thus CI could start failing for extensions. Unfortunately, we cannot use anything more specific: the output of stripAllTags will be tainted only (I think) if the unescaped version of the input was. But there's no way to detect it from taint-check. And actually, that's pretty bizarre... Maybe @Bawolff has better ideas?
I've submitted a test change to a bunch of extensions using stripAllTags to see if there were CI failures. None of them failed, so hooray. Leaving open in case some extension I didn't test is broken and/or we find a better solution.