the plugin is built on top of phan 0.8.0 which is pretty old now. It would be good to update it on top of a modern version of phan, probably 1.2.x.
|Open||None||T207344 Phan-taint-check-plugin not available for PHP > 7.0|
|Open||None||T216974 Update phan-taint-check-plugin to a newer phan (1.2.x)|
|Open||None||T218719 Upgrade php-ast to 0.1.4|
|Resolved||Daimona||T218721 Have CI run seccheck tests|
This would indeed be awesome. However, see my comment in T207344#5033198. Basically, phan has many breaking changes even in patch versions, so upgrading to 0.8.13 would already break a lot of stuff. Today I tried to get there, but eventually gave up due to this.
For whoever will want to proceed with this, here are the main obstacles I found while upgrading to 0.8.13. Note that I don't really know how phan works so it could be fault of mine (but I'm still happy to help).
- Clazz::getPropertyByNameInContext now has an extra required parameter, $is_static. However, I couldn't find a way to determine if the property is static where we call it.
- This commit causes 2 troubles:
- It adds the new class UnaddressableTypedElement, which is extended by Variable. This breaks all typehints for TypedElementInterface in TaintednessBaseVisitor. Just removing them (and adding UnaddressableTypedElement to the docblock) seems to work fine, but it should be checked more deeply.
- Parameter::getContext() is replaced by Parameter::getFileRef(), which however doesn't have the getScope() method used in setTaintedness.
There are some other changes too, but all of them are easy to do.
After 0.8.13 one may upgrade to 0.9.6. I didn't really check it, but it sounds like there aren't many differences between those, so it's likely that the upgrade will be possible without changing anything.
At this point the plugin should be upgraded to PluginV2, as support for old-style plugins was dropped in 0.9.7 (once again, that's what I meant with "phan doesn't comply with semver"). I think this will require a decent amount of work, but it should make the plugin execute faster thanks to the new design.
After 0.9.7, I don't know, but I guess that the road to 1.2.6 will be pretty long.
Please note that all of the above assumes the goal to just make seccheck work with new phan versions. Using new phan methods to speed up the analysis/make it more precise/avoid hacks/you name it is a whole different story.
At any rate, IMVHO we should stop adding any new feature to seccheck before upgrading to 1.2.x, as it's likely that every addiction will add extra code to be reworked.
So I'm looking at the upgrade to PluginV2. The main change is that *Visitor classes are now instantiated by phan itself, which only wants a "get*ClassName" method instead of a constructor. This mostly means that we cannot pass in an instance of the plugin anymore. Moreover, Visitor and PreVisitor must inherit from different classes, which prevents them from having TaintednessBaseVisitor as common parent (should be fixable by turning TaintednessBaseVisitor into a trait). Plus, the MediaWiki checker runs both the generic visitor and the MW visitor, which again isn't possible in V2 because you can only provide a single class name. I'll probably do some gradual changes before the v2 upgrade.
Great! Thanks @sbassett and @Bawolff! I guess we're currently blocked on T218719 which makes https://gerrit.wikimedia.org/r/#/c/mediawiki/tools/phan/SecurityCheckPlugin/+/497583/ fail, plus any further fix which I may have missed and will show up from CI.
Is it helpful to see a bunch of failed instances of composer-package-php70-docker? I'm not sure how valuable that information would be. And the thought here would be to eventually use the composer-package-php7(2|3)-docker whenever we're ready.
It depends on the reason of the failure :-) Right now it fails because it has ast 0.1.2 installed, which is a very old version anyway, so it'd be better to just upgrade it. But yes, we'll have to use php7. later on due to different phan requirements.
Yay! but please please don't create branches that have the same exact format as tags - composer is going to get confused and now we can't actually have a 2.0.0 release. Typically we use "2.x" or something.