Page MenuHomePhabricator
Create Task
Maniphest T197002

Make taint-check pass for AbuseFilter
Closed, ResolvedPublic
Actions

Description

Self explanatory. We currently have 23 warnings which I wasn't able to fix.

Details

SubjectRepoBranchLines +/-
Make seccheck voting for AbuseFilterintegration/configmaster+1 -1
Avoid variable reuse to pass taint-checkmediawiki/extensions/AbuseFiltermaster+21 -19
Update phan-taint-check to 1.3.0. Hopefully should pass nowmediawiki/extensions/AbuseFiltermaster+1 -1
ChangesList: HTML escape the timestampmediawiki/coremaster+2 -2
Force phan-taint-check to think LogFormatter stuff is safe for htmlmediawiki/coremaster+14 -0
Minor escaping fixesmediawiki/extensions/AbuseFiltermaster+36 -23
[WIP] Make AbuseFilter pass phan taint-checkmediawiki/extensions/AbuseFiltermaster+10 -9
Customize query in gerrit

Event Timeline

Daimona created this task.Jun 12 2018, 2:21 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 12 2018, 2:21 PM
gerritbot subscribed.Jun 12 2018, 2:30 PM

Change 439918 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] [WIP] Make AbuseFilter pass phan taint-check

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

gerritbot added a project: Patch-For-Review.Jun 12 2018, 2:30 PM
• Vvjjkkii renamed this task from Make taint-check pass for AbuseFilter to 16aaaaaaaa.Jul 1 2018, 1:04 AM
• Vvjjkkii triaged this task as High priority.
• Vvjjkkii edited projects, added CheckUser, Connected-Open-Heritage-Batch-uploads (RAÄ-KMB_1_2017-02), Tamil-Sites, Gamepress, Hashtags, Jade, KartoEditor, Language-2018-Apr-June, New-Editor-Experiences, Mail, TCB-Team (now WMDE-TechWish); removed AbuseFilter.
• Vvjjkkii updated the task description. (Show Details)
• Vvjjkkii removed subscribers: gerritbot, Aklapper.
Daimona renamed this task from 16aaaaaaaa to Make taint-check pass for AbuseFilter.Jul 1 2018, 10:07 AM
Daimona raised the priority of this task from High to Needs Triage.
Daimona edited projects, added AbuseFilter; removed TCB-Team (now WMDE-TechWish), Mail, New-Editor-Experiences, Language-2018-Apr-June, KartoEditor, Jade, Hashtags, Gamepress, Tamil-Sites, Connected-Open-Heritage-Batch-uploads (RAÄ-KMB_1_2017-02), CheckUser.
Daimona updated the task description. (Show Details)
Daimona added subscribers: gerritbot, Aklapper.
gerritbot added a comment.Jul 5 2018, 4:52 PM

Change 444020 had a related patch set uploaded (by Brian Wolff; owner: Brian Wolff):
[mediawiki/extensions/AbuseFilter@master] Minor escaping fixes

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

Bawolff subscribed.Jul 5 2018, 5:27 PM
In T197002#4400517, @gerritbot wrote:

Change 444020 had a related patch set uploaded (by Brian Wolff; owner: Brian Wolff):
[mediawiki/extensions/AbuseFilter@master] Minor escaping fixes

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

That patch + a patch to OOUI marking all the __construct parameters as escapes_html should fix all but 1 of the warnings.

The last warning seems to be a bug in how phan handles pass by reference parameters triggered by ChangesList::insertLog in core (For some unknown reason)

gerritbot added a comment.Jul 5 2018, 8:23 PM

Change 439918 abandoned by Daimona Eaytoy:
[WIP] Make AbuseFilter pass phan taint-check

Reason:
I7fd1798030d83292ce46543e25c0c431ec345a11 solved these problems

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

gerritbot added a comment.Jul 5 2018, 8:28 PM

Change 444020 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Minor escaping fixes

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

ReleaseTaggerBot added a project: MW-1.32-notes (WMF-deploy-2018-07-10 (1.32.0-wmf.12)).Jul 5 2018, 9:00 PM
gerritbot added a comment.Jul 8 2018, 4:56 PM

Change 444452 had a related patch set uploaded (by Brian Wolff; owner: Brian Wolff):
[mediawiki/core@master] Force phan-taint-check to think LogFormatter stuff is safe for html

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

gerritbot added a comment.Jul 8 2018, 4:57 PM

Change 444453 had a related patch set uploaded (by Brian Wolff; owner: Brian Wolff):
[mediawiki/core@master] minor escaping fix. html escape the timestamp in RC

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

Bawolff added a comment.Jul 9 2018, 3:57 PM

it passes locally for me, so once all these things are merged, and a new version of phan-taint-check is tagged, this bug should be done.

Daimona added a comment.Jul 9 2018, 4:13 PM

That's nice! Should we make it voting on CI as soon as it'll pass there?

Legoktm moved this task from Backlog to Wikimedia deployed on the phan-taint-check-plugin board.Aug 2 2018, 9:39 AM
gerritbot added a comment.Aug 2 2018, 10:15 AM

Change 444452 merged by jenkins-bot:
[mediawiki/core@master] Force phan-taint-check to think LogFormatter stuff is safe for html

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

gerritbot added a comment.Aug 2 2018, 10:22 AM

Change 444453 merged by jenkins-bot:
[mediawiki/core@master] ChangesList: HTML escape the timestamp

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

Daimona added a comment.Aug 2 2018, 10:48 AM

From https://integration.wikimedia.org/ci/job/mwext-php70-phan-seccheck-docker-non-voting/1143/console I see that we still have two errors. I can't determine the reason though.

ReleaseTaggerBot edited projects, added MW-1.32-notes (WMF-deploy-2018-08-07 (1.32.0-wmf.16)); removed MW-1.32-notes (WMF-deploy-2018-07-10 (1.32.0-wmf.12)).Aug 2 2018, 11:00 AM
Bawolff added a comment.Aug 3 2018, 5:45 PM
In T197002#4471857, @Daimona wrote:

From https://integration.wikimedia.org/ci/job/mwext-php70-phan-seccheck-docker-non-voting/1143/console I see that we still have two errors. I can't determine the reason though.

The "./includes/Views/AbuseFilterViewTestBatch.php" one will be fixed once next version pf phan-taint-check is released (which hasn't happened yet)

Legoktm subscribed.Aug 3 2018, 6:24 PM

https://integration.wikimedia.org/ci/job/mwext-php70-phan-seccheck-docker/2926/consoleFull is a run with master of phan-taint-check-plugin that shows no errors :)

@Bawolff should I tag a new release of the plugin? Or was there something you were waiting on?

Huji removed a project: Patch-For-Review.Aug 3 2018, 6:34 PM
Daimona added a comment.Aug 3 2018, 7:16 PM

Nice to know, we'll wait then. Do you think it'd be good to have it as voting, or is it enough to keep it as non-voting?

Legoktm added a comment.Aug 3 2018, 7:21 PM
In T197002#4477498, @Daimona wrote:

Nice to know, we'll wait then. Do you think it'd be good to have it as voting, or is it enough to keep it as non-voting?

I think it definitely should be voting.

gerritbot added a comment.Aug 13 2018, 11:57 PM

Change 452587 had a related patch set uploaded (by Brian Wolff; owner: Brian Wolff):
[mediawiki/extensions/AbuseFilter@master] Update phan-taint-check to 1.3.0. Hopefully should pass now

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

gerritbot added a project: Patch-For-Review.Aug 13 2018, 11:57 PM
Bawolff added a comment.Aug 14 2018, 12:04 AM

Hmm, now we have:

Calling method \OutputPage::addHTML() in \AbuseFilterViewExamine::showExaminer that outputs using tainted argument $html. (Caused by: Builtin-\OutputPage::addHTML) (Caused by: ./includes/Views/AbuseFilterViewExamine.php +194; ./includes/Views/AbuseFilterViewExamine.php +195; ./includes/Views/AbuseFilterViewExamine.php +213; ./includes/Views/AbuseFilterViewExamine.php +218; ./includes/Views/AbuseFilterViewExamine.php +220)

Guess I'll have to figure out what's up with that.

gerritbot added a comment.Aug 14 2018, 3:06 AM

Change 452587 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Update phan-taint-check to 1.3.0. Hopefully should pass now

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

ReleaseTaggerBot edited projects, added MW-1.32-notes (WMF-deploy-2018-08-14 (1.32.0-wmf.17)); removed MW-1.32-notes (WMF-deploy-2018-08-07 (1.32.0-wmf.16)).Aug 14 2018, 4:00 AM
Bawolff added a comment.Aug 14 2018, 4:58 AM

Seems to be caused by OOUI\FieldsetLayout

Jdforrester-WMF edited projects, added MW-1.32-notes (WMF-deploy-2018-08-21 (1.32.0-wmf.18)); removed MW-1.32-notes (WMF-deploy-2018-08-14 (1.32.0-wmf.17)).Aug 14 2018, 8:54 PM
Daimona added a comment.EditedAug 16 2018, 5:04 PM

Is this something wrong on OOUI's end?

Note: now we also have another error on SpecialAbuseLog. Seems to be caused by the legend element at line 502, maybe a minor message escaping problem?

gerritbot added a comment.Aug 20 2018, 5:10 PM

Change 454033 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/AbuseFilter@master] Avoid variable reuse to pass taint-check

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

Umherirrender subscribed.Aug 20 2018, 5:11 PM

It seems the value from WebRequest::getText is the "problem" here.
Does it needs escaping when passed to Xml::textarea?

Huji assigned this task to Umherirrender.Aug 20 2018, 10:56 PM
gerritbot added a comment.Aug 20 2018, 11:19 PM

Change 454033 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Avoid variable reuse to pass taint-check

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

Huji removed a project: Patch-For-Review.Aug 21 2018, 2:29 AM
Huji subscribed.

I think all open patches are merged, no?

gerritbot added a comment.Aug 21 2018, 7:52 AM

Change 454202 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[integration/config@master] Make seccheck voting for AbuseFilter

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

gerritbot added a project: Patch-For-Review.Aug 21 2018, 7:52 AM
gerritbot added a comment.Aug 22 2018, 6:19 AM

Change 454202 merged by jenkins-bot:
[integration/config@master] Make seccheck voting for AbuseFilter

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

Umherirrender closed this task as Resolved.Aug 22 2018, 7:06 AM
Daimona removed a project: Patch-For-Review.Aug 22 2018, 7:38 AM
Legoktm added a comment.Aug 22 2018, 5:39 PM

\o/ nice work everyone :)

Huji awarded a token.Aug 22 2018, 5:46 PM
sbassett moved this task from Wikimedia deployed to Done on the phan-taint-check-plugin board.Oct 15 2019, 6:55 PM
sbassett triaged this task as Medium priority.Oct 15 2019, 7:25 PM
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