Page MenuHomePhabricator
Create Task
Maniphest T248360

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

Description

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>

Details

SubjectRepoBranchLines +/-
Revert "Disable phan warning for now"mediawiki/extensions/MobileFrontendmaster+0 -2
Disable phan warning for nowmediawiki/extensions/MobileFrontendmaster+2 -0
Customize query in gerrit

Related Objects

Event Timeline

Jdlrobson created this task.Mar 23 2020, 10:38 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 23 2020, 10:38 PM
gerritbot added a comment.Mar 23 2020, 10:39 PM

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

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

gerritbot added a project: Patch-For-Review.Mar 23 2020, 10:39 PM
matmarex subscribed.Mar 24 2020, 12:31 AM

I actually found a different HTML escaping issue while investigating this: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/582909

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…?

Krinkle subscribed.Mar 24 2020, 12:34 AM

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.

matmarex added a comment.Mar 24 2020, 12:36 AM

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

gerritbot added a comment.Mar 24 2020, 12:46 AM

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

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

Jdlrobson edited projects, added Security, Security-Team, Web-Team-Backlog (Kanbanana-2019-20-Q3); removed Patch-For-Review.Mar 24 2020, 12:52 AM

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.

ReleaseTaggerBot added a project: MW-1.35-notes (1.35.0-wmf.25; 2020-03-24).Mar 24 2020, 1:00 AM
sbassett subscribed.Mar 24 2020, 2:34 AM

See also related security task: T236509.

xSavitar subscribed.Mar 24 2020, 11:08 AM
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.
Jdlrobson added a comment.Mar 24 2020, 8:07 PM

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

sbassett added a comment.Mar 25 2020, 1:52 AM
In T248360#5996245, @Jdlrobson wrote:

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.

gerritbot added a comment.Mar 25 2020, 4:08 PM

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

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

gerritbot added a project: Patch-For-Review.Mar 25 2020, 4:08 PM
Jdlrobson moved this task from Needs Analysis to Upcoming on the Web-Team-Backlog (Kanbanana-2019-20-Q3) board.Mar 25 2020, 4:27 PM

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

Jdlrobson added a comment.Mar 25 2020, 5:59 PM

It's passing. Looks like it was fixed by https://phabricator.wikimedia.org/T248360#5997215

sbassett added a comment.Mar 26 2020, 3:53 AM
In T248360#5998997, @Jdlrobson wrote:

@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.

gerritbot added a comment.Mar 26 2020, 2:00 PM

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

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

Daimona subscribed.Mar 26 2020, 2:04 PM

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

ReleaseTaggerBot edited projects, added MW-1.35-notes (1.35.0-wmf.26; 2020-03-31); removed MW-1.35-notes (1.35.0-wmf.25; 2020-03-24).Mar 26 2020, 10:01 PM
Jdlrobson closed this task as Resolved.Mar 30 2020, 11:02 PM
Jdlrobson claimed this task.
Jdlrobson moved this task from Upcoming to Ready for Signoff on the Web-Team-Backlog (Kanbanana-2019-20-Q3) board.
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL