Page MenuHomePhabricator

Experiment using phan for static analysis
Closed, ResolvedPublic

Description

We currently use phan for some static analysis checks.

This is a task to experiment writing a plugin to extend phan to support security related checks, with an emphasis on doing mediawiki specific things that off the shelf tools can't do

Repo is at /mediawiki/tools/phan/SecurityCheckPlugin.git

Event Timeline

Bawolff created this task.Nov 29 2017, 5:46 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 29 2017, 5:46 PM
Bawolff moved this task from Backlog to In Progress on the Security-Team board.Nov 29 2017, 5:47 PM
Reedy added a subscriber: Reedy.Dec 7 2017, 2:35 AM

Now T182214: Get securityCheckPlugin on packagist is done... Can we add this repo to require-dev in composer.json?

Reedy added a comment.Dec 7 2017, 2:41 AM

Now T182214: Get securityCheckPlugin on packagist is done... Can we add this repo to require-dev in composer.json?

Duh. No we can't. Not until min PHP support is bumped to 7

Ill probably send out a wikitech-l email on monday encouraging people to test and give feedback.

It was suggested on irc to include it as non-voting jenkins test, or maybe a post merge hook (in fast mode it takes about 40 seconds to check an extension. In slow mode about 3 minutes and large mem requirements which might be too slow for pre-merge jenkins test). The idea to use non-voting is because false positives do happen and we probably wouldnt want to block commit on that

For mediawiki core, there are still too many false positives to be useful in an automated fashion. (About 240 at last count, mostly concentrated in the parser and db upgrader/installer. Although last week it was 310 so this number is coming down). I suspect false positives will be much lower in extensions but i have yet to test a wide variety of extension types.

Legoktm added a subscriber: Legoktm.Dec 9 2017, 2:12 AM

Can we implement a way to suppress false positives, e.g. @codingStandardsLineIgnore ? (using a different tag or something). Assuming the false positive rate is reasonable, I think something like this should definitely be voting after a bit of tuning and testing.

Can we implement a way to suppress false positives, e.g. @codingStandardsLineIgnore ? (using a different tag or something). Assuming the false positive rate is reasonable, I think something like this should definitely be voting after a bit of tuning and testing.

Yes. Its the same as how phan suppression works. if you do

@suppress SecurityCheck-XSS

In the doc-coment of the method in question, the issue gets suppressed. This is only supported in doc-comment blocks, and doesn't work from normal comments.

chasemp assigned this task to Bawolff.Sep 4 2018, 4:35 PM

I think we can close this as resolved? The experiment has been incredibly successful so far.

chasemp triaged this task as Normal priority.Sep 4 2018, 6:15 PM
chasemp added a subscriber: chasemp.

I think we can close this as resolved? The experiment has been incredibly successful so far.

I think yes, ping @Bawolff though since it's mainly his gig

sbassett closed this task as Resolved.Apr 12 2019, 7:38 PM
sbassett added a subscriber: sbassett.

Going to make an executive decision here and resolve this for now. Plenty of other related bugs are open for ongoing issues.

sbassett moved this task from In Progress to Done on the Security-Team board.Jun 11 2019, 6:05 PM