Page MenuHomePhabricator

Setup phan for AbuseFilter
Closed, ResolvedPublic


In T192322 I learned about phan and about it not being setup for AbuseFilter. I think it would be great to set it up for AbuseFilter

Event Timeline

Legoktm renamed this task from Setu phan for AbuseFilter to Setup phan for AbuseFilter.Apr 17 2018, 1:37 AM
Legoktm removed a project: phan-taint-check-plugin.

Change 427851 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] [WIP] Add phan configuration

I'm working on this, but to me it's slow and painful. There are lots of errors which I'm not sure how to fix, nor I'd like to set exclusions if there may be an easy way to fix. @Legoktm since you know phan really well, could you please take a look?

ADD: More precisely, while the undeclared members are pretty easy to fix, I have no ideas for the remaining errors about tainted output.

Also adding @Bawolff in case you could please take a look. I managed to fix almost every error (although such fixes must be reviewed), apart from a couple of suppressed false positive and one waiting for a patch in the core. However, seccheck still has 7 errors which I'm not able to fix, nor I can figure out where they come from (line indications aren't precise). With some examples of common fixes we should be able to go on and fix new errors ourselves as phan will be enabled on I/C.

@Bawolff I still can't fix the remaining errors, and I have a doubt: may it be a problem that some OOUI widgets aren't listed here? They should remove any taint, but only some of them are there. I checked the code multiple time, but still can't see why seccheck is failing.

Change 429663 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add phan configuration

@Daimona for now at least I think normal phan is the priority for bundling (and great work so far!). T182599: Make jenkins run phan-taint-check-plugin non-voting and then voting tracks progress on the security check plugin.

@Legoktm yeah, in fact I sent a separate patch for normal phan, which is less controversial and with simpler modifications. I'd also like to have success from seccheck, but without knowing where to look to fix errors, there isn't much I can do. Also, I found a couple of points where custom javascript can be injected, and this should really be fixed.

Change 429663 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add phan configuration

Daimona added a project: Patch-For-Review.

Oh, actually we have the seccheck one

Change 427851 abandoned by Daimona Eaytoy:
[WIP] Add phan seccheck configuration

None of the fixes in this patch solve seccheck issues. The composer requirement has been moved in I094a8af4f97c03f8b538ede00420b123de25138a and the needed fixes will follow-up that one in a brand new patch & task.